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

Jeff Garzik
2002-09-15T22:53:08Z

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 <brad@heeltoe.com>");
> +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