Four new i2c drivers and __init/__exit cleanup toi2c
PATCH 1/9 Four new i2c drivers and __init/__exit cleanup toi2c
This specific change set was for i2c-algo-8xx.c and 2.5.34. I propose the following changes to satisfy 2.5 kernel requirements. Of course the #include changes and MODULE_LICENSE will not be checked into CVS.
Jeff, here was the original code which saved flags and interrupted. The goal was to remove the cli, so the new code that Ingo suggested is:local_irq_save(flags); Redesigning with semaphores is not possible.
/* Chip bug, set enable here */
save_flags(flags); cli();
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);
restore_flags(flags);Jeff, the busy-wait works fine, but Russell commented that Athlon cpu's would destroy itself, so cpu_relax was added. I don't think its needed on this chip myself.
Anyone see a problem with 2.2 or 2.4 and these changes? Regards,
Albert
usr/src/linux/drivers/i2c/i2c-algo-8xx.c 2002-09-17 09:04:53.000000000 -0400 renamed from i2c-algo-8xx.c 2002-08-03 18:48:18.000000000 -0400 +31 -35
@@ -54,7 +54,7 @@ static void cpm_iic_interrupt(void *dev_id, void *regs) { - volatile i2c8xx_t *i2c = (i2c8xx_t *)dev_id; + i2c8xx_t *i2c = (i2c8xx_t *)dev_id; if (cpm_debug > 1) printk("cpm_iic_interrupt(dev_id=%p)\n", dev_id);
@@ -75,8 +75,8 @@ 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; + iic_t *iip = cpm_adap->iip; + i2c8xx_t *i2c = cpm_adap->i2c; if (cpm_debug) printk("cpm_iic_init() - iip=%p\n",iip);
@@ -110,11 +110,12 @@ /* Initialize Tx/Rx parameters. */ if (cpm_adap->reloc = 0) { - volatile cpm8xx_t *cp = cpm_adap->cp; + 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); + while (cp->cp_cpcr & CPM_CR_FLG) + cpu_relax(); /* prevent Athlons self-destruct */ } /* Select an arbitrary address. Just make sure it is unique.
@@ -145,7 +146,7 @@ static int cpm_iic_shutdown(struct i2c_algo_8xx_data *cpm_adap) { - volatile i2c8xx_t *i2c = cpm_adap->i2c; + i2c8xx_t *i2c = cpm_adap->i2c; /* Shut down IIC. */
@@ -157,7 +158,7 @@ } static void -cpm_reset_iic_params(volatile iic_t *iip) +cpm_reset_iic_params(iic_t *iip) { iip->iic_tbase = r_tbase; iip->iic_rbase = r_rbase;
@@ -185,14 +186,15 @@ static void force_close(struct i2c_algo_8xx_data *cpm) { if (cpm->reloc = 0) { - volatile cpm8xx_t *cp = cpm->cp; + 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); + while (cp->cp_cpcr & CPM_CR_FLG) + cpu_relax(); /* prevent Athlons self-destruct */ } }
@@ -203,10 +205,10 @@ static int cpm_iic_read(struct i2c_algo_8xx_data *cpm, u_char abyte, char *buf, int count) { - volatile iic_t *iip = cpm->iip; - volatile i2c8xx_t *i2c = cpm->i2c; - volatile cpm8xx_t *cp = cpm->cp; - volatile cbd_t *tbdf, *rbdf; + iic_t *iip = cpm->iip; + i2c8xx_t *i2c = cpm->i2c; + cpm8xx_t *cp = cpm->cp; + cbd_t *tbdf, *rbdf; u_char *tb; unsigned long flags;
@@ -246,7 +248,7 @@ rbdf->cbd_bufaddr = __pa(buf); rbdf->cbd_sc = BD_SC_EMPTY | BD_SC_WRAP; - invalidate_dcache_range((unsigned long) buf, (unsigned long) (buf+count)); + dma_cache_inv((unsigned long) buf, (unsigned long) (buf+count)); /* Chip bug, set enable here */ local_irq_save(flags);
@@ -287,7 +289,7 @@ } - invalidate_dcache_range((unsigned long) buf, (unsigned long) (buf+count)); + dma_cache_inv((unsigned long) buf, (unsigned long) (buf+count)); return count; }
@@ -298,10 +300,10 @@ static int cpm_iic_write(struct i2c_algo_8xx_data *cpm, u_char abyte, char *buf,int count) { - volatile iic_t *iip = cpm->iip; - volatile i2c8xx_t *i2c = cpm->i2c; - volatile cpm8xx_t *cp = cpm->cp; - volatile cbd_t *tbdf; + iic_t *iip = cpm->iip; + i2c8xx_t *i2c = cpm->i2c; + cpm8xx_t *cp = cpm->cp; + cbd_t *tbdf; u_char *tb; unsigned long flags;
@@ -366,10 +368,10 @@ static int cpm_iic_tryaddress(struct i2c_algo_8xx_data *cpm, int addr) { - volatile iic_t *iip = cpm->iip; - volatile i2c8xx_t *i2c = cpm->i2c; - volatile cpm8xx_t *cp = cpm->cp; - volatile cbd_t *tbdf, *rbdf; + iic_t *iip = cpm->iip; + i2c8xx_t *i2c = cpm->i2c; + cpm8xx_t *cp = cpm->cp; + cbd_t *tbdf, *rbdf; u_char *tb; unsigned long flags, len;
@@ -573,20 +575,14 @@ return 0; } +static void __exit i2c_algo_8xx_exit(void) +{ +} + -#ifdef MODULE MODULE_AUTHOR("Brad Parker <[email protected]>"); 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) -{ -} -#endif +module_init(i2c_algo_8xx_init); +module_exit(i2c_algo_8xx_exit);
Quoted 116 lines · Jeff Garzik
Review
Quoted 4 lines · Albert Cranford
(quoted diff) +4
-0+static void +cpm_iic_interrupt(void *dev_id, void *regs) +{ + volatile i2c8xx_t *i2c = (i2c8xx_t *)dev_id;no need for volatile
Review
Quoted 7 lines
(quoted diff) +7
-0+} + +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...
Review
Quoted 7 lines
(quoted diff) +7
-0+ 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
Review
Quoted 12 lines
(quoted diff) +12
-0+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
Quoted 7 lines
(quoted diff) +7
-0+ 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);Review
Quoted 10 lines
(quoted diff) +10
-0+ 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 */
__pashould very rarely be used in driversReview
Quoted 5 lines
(quoted diff) +5
-0+ /* Wait for IIC transfer */ + interruptible_sleep_on(&iic_wait); + local_irq_restore(flags); + if (signal_pending(current)) + return -EIO;
local_irq_xxxshould not be used...And it is very likely you should be using a semaphore here...
Review
Quoted 12 lines
(quoted diff) +12
-0+ /* 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
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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_COUNTis for a file op, use ->owner instead.Quoted 5 lines
(quoted diff) +5
-0+#ifdef MODULE +MODULE_AUTHOR("Brad Parker <[email protected]>"); +MODULE_DESCRIPTION("I2C-Bus MPC8XX algorithm"); +#ifdef MODULE_LICENSE +MODULE_LICENSE("GPL");Review
Quoted 10 lines
(quoted diff) +10
-0+#endif + +int init_module(void) +{ + return i2c_algo_8xx_init(); +} + +void cleanup_module(void) +{ +}kill ifdef MODULE.
usemodule_init,module_exit
Signature · 2 lines
Albert Cranford Deerfield Beach FL USA [email protected]
PATCH 1/9 Four new i2c drivers and __init/__exit cleanup toi2c
Quoted 5 lines · Albert Cranford
Jeff, here was the original code which saved flags and interrupted. The goal was to remove the cli, so the new code that Ingo suggested is:local_irq_save(flags); Redesigning with semaphores is not possible.
Quoted 10 lines
/* Chip bug, set enable here */
save_flags(flags); cli();
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);
restore_flags(flags);Well, shrug. In 2.5, tests _are_ being put into the scheduler at this very moment by Linus to prevent sleeping with interrupts disabled, so it will have to be fixed properly sooner or later.
You simply aren't allowed to sleep with interrupts disabled anymore.
A possible solution would be _something_ like:
int ret = 0;
interrupt_happened = 0;
i2c->i2c_i2cmr = 0x13;
i2c->i2c_i2cer = 0xff;
i2c->i2c_i2mod = 1;
i2c->i2c_i2com = 0x81;
wait_event_interruptible(&iic_wait, interrupt_happened = 1, ret);
if (ret) {
/* I was interrupted while I was sleeping */
/* clean up */
}and in your interrupt handler:
interrupt_happened = 1; wake_up_interruptible(&iic_wait);
If you can guarantee that this function will only be called by thread at any one time, you can get the exact same effect with:
sema_init(&sem, 0);
i2c->i2c_i2cmr = 0x13;
i2c->i2c_i2cer = 0xff;
i2c->i2c_i2mod = 1;
i2c->i2c_i2com = 0x81;
ret = down_interruptible(&sem);
if (ret) {
/* I was interrupted while I was sleeping */
/* clean up */
}and in your interrupt handler:
up(&sem);
Since you're frobbing with the hardware, you should be able to make this guarantee. If not, the driver is buggy, and would need the following:
ret = down_interruptible(&hw_access_sem);
if (ret)
return ret;
sema_init(&sem, 0);
i2c->i2c_i2cmr = 0x13;
i2c->i2c_i2cer = 0xff;
i2c->i2c_i2mod = 1;
i2c->i2c_i2com = 0x81;
ret = down_interruptible(&sem);
up(&hw_access_sem);
if (ret) {
/* I was interrupted while I was sleeping */
/* clean up */
}The patch you attach is fine as far as making things better than they are at present.
Quoted 3 lines
Jeff, the busy-wait works fine, but Russell commented that Athlon cpu's would destroy itself, so
cpu_relaxwas added. I don't think its needed on this chip myself.
Note that I didn't analyse whether they were always in process context or not.
Signature · 2 lines
Russell King ([email protected]) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html