Messages in this thread | | | Date | Sun, 15 Sep 2002 18:57:31 -0400 | From | Jeff Garzik <> | Subject | Re: [patch 1/9]Four new i2c drivers and __init/__exit cleanup to i2c |
| |
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
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |