2002-09-15 22:34:31

by Albert Cranford

[permalink] [raw]
Subject: [patch 4/9]Four new i2c drivers and __init/__exit cleanup to i2c

Hello Linus,
New I2C drivers that have been adjusted after Russell King comments of August.
o i2c-algo-8xx.c
o i2c-pport.c
o i2c-adap-ibm_ocp.c
o i2c-pcf-epp.c
o Add new drivers to Config.in and Makefile.
o Add new drivers to i2c-core for initialization.
o Remove EXPORT_NO_SYMBOLS statement from i2c-dev, i2c-elektor and i2c-frodo.
o Cleanup init_module and cleanup_module adding __init and __exit to most drivers.
o Adjust i2c-elektor with cli/sti replacement.
--
[email protected]


Attachments:
47-i2c-3d-patch (7.87 kB)

2002-09-15 23:06:21

by Jeff Garzik

[permalink] [raw]
Subject: Re: [patch 4/9]Four new i2c drivers and __init/__exit cleanup to i2c

Albert Cranford wrote:

> +static void pcf_epp_waitforpin(void) {
> + int timeout = 10;
> +
> + if (gpe.pe_irq > 0) {
> + local_irq_disable();
> + if (pcf_pending == 0) {
> + interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ);
> + //udelay(100);
> + } else {
> + pcf_pending = 0;
> + }
> + local_irq_enable();
> + } else {
> + udelay(100);
> + }
> +}

use a semaphore... don't ever use local_irq_xxx() when you clearly
don't need to


> +static void pcf_epp_handler(int this_irq, void *dev_id, struct pt_regs *regs) {
> + pcf_pending = 1;
> + wake_up_interruptible(&pcf_wait);
> + DEB3(printk(KERN_DEBUG "i2c-pcf-epp.o: in interrupt handler.\n"));
> +}
> +
> +
> +static int pcf_epp_init(void *data)
> +{
> + if (check_region(gpe.pe_base, 5) < 0 ) {
> +
> + printk(KERN_WARNING "Could not request port region with base 0x%x\n", gpe.pe_base);
> + return -ENODEV;
> + } else {
> + request_region(gpe.pe_base, 5, "i2c (EPP parallel port adapter)");
> + }

race. never use check_region



> + DEB3(printk(KERN_DEBUG "i2c-pcf-epp.o: init status port = 0x%x\n", inb(0x379)));
> +
> + if (gpe.pe_irq > 0) {
> + if (request_irq(gpe.pe_irq, pcf_epp_handler, 0, "PCF8584", 0) < 0) {
> + printk(KERN_NOTICE "i2c-pcf-epp.o: Request irq%d failed\n", gpe.pe_irq);
> + gpe.pe_irq = 0;
> + } else
> + disable_irq(gpe.pe_irq);
> + enable_irq(gpe.pe_irq);
> + }

why do you disable then enable the irq here?



> +static void pcf_epp_inc_use(struct i2c_adapter *adap)
> +{
> +#ifdef MODULE
> + MOD_INC_USE_COUNT;
> +#endif
> +}
> +
> +static void pcf_epp_dec_use(struct i2c_adapter *adap)
> +{
> +#ifdef MODULE
> + MOD_DEC_USE_COUNT;
> +#endif
> +}

kill ifdefs, use ->owner if possible


> +#ifdef MODULE
> +MODULE_AUTHOR("Hans Berglund <[email protected]> \n modified by Ryosuke Tajima <[email protected]>");
> +MODULE_DESCRIPTION("I2C-Bus adapter routines for PCF8584 EPP parallel port adapter");
> +
> +MODULE_PARM(base, "i");
> +MODULE_PARM(irq, "i");
> +MODULE_PARM(clock, "i");
> +MODULE_PARM(own, "i");
> +MODULE_PARM(i2c_debug, "i");
> +
> +int init_module(void)
> +{
> + return i2c_pcfepp_init();
> +}
> +
> +void cleanup_module(void)
> +{
> + i2c_pcf_del_bus(&pcf_epp_ops);
> + pcf_epp_exit();
> +}
> +
> +#endif

kill ifdef, use module_init, module_exit