Re: [PATCH] I2C: Add I2C support for the MPC8260

[Christoph Hellwig <hch@lst.de>]

> + * Release 0.1

please don't put such versaison comments in, the SCM has the history.

> +#include <linux/config.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/sched.h>
> +#include <linux/init.h>
> +#include <linux/pci.h>
> +#include <asm/io.h>
> +#include <linux/fsl_devices.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <asm/immap_cpm2.h>
> +#include <asm/mpc8260.h>
> +#include <asm/cpm2.h>
> +
> +#include <linux/i2c-algo-cpm2.h>
> +#include <linux/platform_device.h>

<asm/*.h> always after <linux/*.h> please.

> +#define CPM_MAX_READ	513
> +
> +static wait_queue_head_t iic_wait;
> +static ushort r_tbase, r_rbase;
> +
> +int cpm_scan = 0;
> +int cpm_debug = 0;
> +
> +struct mpc_i2c {
> +	u32 interrupt;
> +	wait_queue_head_t queue;
> +	struct i2c_adapter adap;
> +	int irq;
> +	u32 flags;
> +	struct i2c_algo_cpm2_data *data;
> +};
> +
> +static struct i2c_algo_cpm2_data cpm2_data;
> +
> +static void
> +cpm2_iic_init(struct i2c_algo_cpm2_data *data)
> +{
> +	volatile cpm_cpm2_t *cp;
> +	volatile cpm2_map_t *immap;
> +
> +	cp = cpmp;	/* Get pointer to Communication Processor */
> +	immap = (cpm2_map_t *)CPM_MAP_ADDR;	/* and to internal registers */
> +
> +	*(ushort *)(&immap->im_dprambase[PROFF_I2C_BASE]) = PROFF_I2C;
> +	data->iip = (iic_t *)&immap->im_dprambase[PROFF_I2C];

Please stop volatile abuse and use ioremap & friends to properly to access
I/O memory.

> +	/* Chip bug, set enable here */
> +	local_irq_save(flags);
> +	i2c->i2c_i2cmr = 0x13;	/* Enable some interupts */
> +	i2c->i2c_i2cer = 0xff;
> +	i2c->i2c_i2mod |= 1;	/* Enable */
> +	i2c->i2c_i2com = 0x81;	/* Start master */
> +
> +	/* Wait for IIC transfer */
> +	interruptible_sleep_on(&iic_wait);

never use interruptible_sleep_on() and the local_irq_save usage here is
bogus aswell, please use proper irq disabling spinlocks and make sure
you don't sleep with interrupts disabled.

> +	if (cpm_debug) {
> +			int u;
> +			for (u = 0; u < count; u++) {
> +				printk(KERN_DEBUG "buf[%d] = 0x%x\n", u, buf[u]);
> +			}
> +			}


broken indentation.

> +			if (cpm_debug)
> +				printk(KERN_DEBUG "i2c-cpm2.o: wrote %d\n", ret);

could you please use dev_dbg() instead of all this if (cpm_debug) mess?

> +			if (ret < pmsg->len ) {
> +				return (ret<0) ? ret : -EREMOTEIO;

indentation problems again.

> +	int result = 0;
> +	struct mpc_i2c *i2c;
> +	struct platform_device *pdev = to_platform_device(device);
> +	struct fsl_i2c_platform_data *pdata;
> +
> +	pdata = (struct fsl_i2c_platform_data *) pdev->dev.platform_data;

generally no need to cast here.  Also please use platform_get_drvdata()

> +
> +	if (!(i2c = kmalloc(sizeof(*i2c), GFP_KERNEL))) {
> +		return -ENOMEM;
> +	}
> +	memset(i2c, 0, sizeof(*i2c));

please use kzalloc()

> +	if ((result = i2c_add_adapter(&i2c->adap)) < 0) {

please split such lines into two.

> +  fail_add:

goto lables should either be in column 0 or 1 (and that consitantly
through a source file)