Re: [patch 1/9]Four new i2c drivers and __init/__exit cleanup to

[Jeff Garzik]

Albert Cranford wrote:
> +static void
> +cpm_iic_interrupt(void *dev_id, void *regs)
> +{
> +	volatile i2c8xx_t *i2c = (i2c8xx_t *)dev_id;

no need for volatile

> +}
> +
> +static void
> +cpm_iic_init(struct i2c_algo_8xx_data *cpm_adap)
> +{
> +	volatile iic_t		*iip = cpm_adap->iip;
> +	volatile i2c8xx_t	*i2c = cpm_adap->i2c;

no need for volatile.  repeat this comment several times...

> +	if (cpm_adap->reloc == 0) {
> +		volatile cpm8xx_t *cp = cpm_adap->cp;
> +
> +		cp->cp_cpcr =
> +			mk_cr_cmd(CPM_CR_CH_I2C, CPM_CR_INIT_TRX) | CPM_CR_FLG;
> +		while (cp->cp_cpcr & CPM_CR_FLG)
> +			cpu_relax(); /* prevent Athlons self-destruct */
> +	}

don't busy-wait in a context where you can sleep

> +static void force_close(struct i2c_algo_8xx_data *cpm)
> +{
> +	if (cpm->reloc == 0) {
> +		volatile cpm8xx_t *cp = cpm->cp;
> +
> +		if (cpm_debug) printk("force_close()\n");
> +		cp->cp_cpcr =
> +			mk_cr_cmd(CPM_CR_CH_I2C, CPM_CR_CLOSE_RXBD) |
> +			CPM_CR_FLG;
> +
> +		while (cp->cp_cpcr & CPM_CR_FLG)
> +			cpu_relax(); /* prevent Athlons self-destruct */
> +	}

likewise for volatile and busy-wait

> +	tbdf->cbd_bufaddr = __pa(tb);
> +	tbdf->cbd_datlen = count + 1;
> +	tbdf->cbd_sc =
> +		BD_SC_READY | BD_SC_INTRPT | BD_SC_LAST |
> +		BD_SC_WRAP | BD_IIC_START;
> +
> +	rbdf->cbd_datlen = 0;
> +	rbdf->cbd_bufaddr = __pa(buf);
> +	rbdf->cbd_sc = BD_SC_EMPTY | BD_SC_WRAP;
> +
> +	dma_cache_inv((unsigned long) buf, (unsigned long) (buf+count));
> +
> +	/* 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 */

__pa should very rarely be used in drivers

> +	/* Wait for IIC transfer */
> +	interruptible_sleep_on(&iic_wait);
> +	local_irq_restore(flags);
> +	if (signal_pending(current))
> +		return -EIO;

local_irq_xxx should not be used...

And it is very likely you should be using a semaphore here...

> +	/* 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);
> +	local_irq_restore(flags);
> +	if (signal_pending(current))
> +		return -EIO;

likewise

> +	printk("i2c-algo-8xx.o: adapter unregistered: %s\n",adap->name);
> +
> +#ifdef MODULE
> +	MOD_DEC_USE_COUNT;
> +#endif
> +	return 0;

Is this MOD_DEC_USE_COUNT is for a file op, use ->owner instead.

> +#ifdef MODULE
> +MODULE_AUTHOR("Brad Parker <[e-mail]>");
> +MODULE_DESCRIPTION("I2C-Bus MPC8XX algorithm");
> +#ifdef MODULE_LICENSE
> +MODULE_LICENSE("GPL");
> +#endif
> +
> +int init_module(void) 
> +{
> +	return i2c_algo_8xx_init();
> +}
> +
> +void cleanup_module(void) 
> +{
> +}

kill ifdef MODULE.
use module_init, module_exit