2.5.30 i2c updates 3/4
PATCH 1/1 2.5.30 i2c updates 3/4
Hello Linus,
Please apply the following four patches that update 2.5.30 with these I2C changes:
o Support for SMBus 2.0 PEC Packet Error Checking
o New algorithm-i2c-algo-8xxx for MPC8XXX
o New algorithm-i2c-algo-ibm_ocp for IBM PPC 405
o New adapter-i2c-adap-ibm_ocp for IBM PPC 405
o New adapter-i2c-frodo for SA 1110 board
o New adapter-i2c-pcf-epp for PCF8584
o New adapter-i2c-pport for parallel port
o New adapter-i2c-rpx for embeded MPC8XX
o Replace depreciated cli()&sti() with spin_{un}lock_irq()
o Updated documentation
Thanks,
Albert
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +401 -0
@@ -0,0 +1,401 @@ +/* + ------------------------------------------------------------------------- + i2c-adap-ibm_ocp.c i2c-hw access for the IIC peripheral on the IBM PPC 405 + ------------------------------------------------------------------------- + + Ian DaSilva, MontaVista Software, Inc. + [email protected] or [email protected] + + Copyright 2000 MontaVista Software Inc. + + Changes made to support the IIC peripheral on the IBM PPC 405 + + + ---------------------------------------------------------------------------- + This file was highly leveraged from i2c-elektor.c, which was created + by Simon G. Vogl and Hans Berglund: + + + Copyright (C) 1995-97 Simon G. Vogl + 1998-99 Hans Berglund + + With some changes from Kyösti Mälkki <[email protected]> and even + Frodo Looijaard <[email protected]> + + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 2 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + ---------------------------------------------------------------------------- + + History: 01/20/12 - Armin + [email protected] + ported up to 2.4.16+ + + Version 02/03/25 - Armin + converted to ocp format + removed commented out or #if 0 code + + TODO: convert to ocp_register + add PM hooks + +*/ + + +#include <linux/kernel.h> +#include <linux/ioport.h> +#include <linux/module.h> +#include <linux/delay.h> +#include <linux/slab.h> +#include <linux/version.h> +#include <linux/init.h> +#include <asm/irq.h> +#include <asm/io.h> +#include <linux/i2c.h> +#include "i2c-algo-ibm_ocp.h" +#include <linux/i2c-id.h> +#include <asm/ocp.h> + +#ifdef MODULE_LICENSE +MODULE_LICENSE("GPL"); +#endif + +/* + * This next section is configurable, and it is used to set the number + * of i2c controllers in the system. The default number of instances is 1, + * however, this should be changed to reflect your system's configuration. + */ + +/* + * The STB03xxx, with a PPC405 core, has two i2c controllers. + */ +//(sizeof(IIC_ADDR)/sizeof(struct iic_regs)) +extern iic_t *IIC_ADDR[]; +static struct iic_ibm iic_ibmocp_adaps[IIC_NUMS][5]; + +static struct i2c_algo_iic_data *iic_ibmocp_data[IIC_NUMS]; +static struct i2c_adapter *iic_ibmocp_ops[IIC_NUMS]; + +static int i2c_debug=0; +static wait_queue_head_t iic_wait[IIC_NUMS]; +static int iic_pending; + + +/* ----- global defines ----------------------------------------------- */ +#define DEB(x) if (i2c_debug>=1) x +#define DEB2(x) if (i2c_debug>=2) x +#define DEB3(x) if (i2c_debug>=3) x +#define DEBE(x) x /* error messages */ + +/* ----- local functions ---------------------------------------------- */ + +// +// Description: Write a byte to IIC hardware +// +static void iic_ibmocp_setbyte(void *data, int ctl, int val) +{ + // writeb resolves to a write to the specified memory location + // plus a call to eieio. eieio ensures that all instructions + // preceding it are completed before any further stores are + // completed. + // Delays at this level (to protect writes) are not needed here. + writeb(val, ctl); +} + + +// +// Description: Read a byte from IIC hardware +// +static int iic_ibmocp_getbyte(void *data, int ctl) +{ + int val; + + val = readb(ctl); + return (val); +} + + +// +// Description: Return our slave address. This is the address +// put on the I2C bus when another master on the bus wants to address us +// as a slave +// +static int iic_ibmocp_getown(void *data) +{ + return(((struct iic_ibm *)(data))->iic_own); +} + + +// +// Description: Return the clock rate +// +static int iic_ibmocp_getclock(void *data) +{ + return(((struct iic_ibm *)(data))->iic_clock); +} + + +#if 0 +static void iic_ibmocp_sleep(unsigned long timeout) +{ + schedule_timeout( timeout * HZ); +} +#endif + + +// +// Description: Put this process to sleep. We will wake up when the +// IIC controller interrupts. +// +static void iic_ibmocp_waitforpin(void *data) { + + int timeout = 2; + struct iic_ibm *priv_data = data; + spinlock_t driver_lock = SPIN_LOCK_UNLOCKED; + + // + // If interrupts are enabled (which they are), then put the process to + // sleep. This process will be awakened by two events -- either the + // the IIC peripheral interrupts or the timeout expires. + // + if (priv_data->iic_irq > 0) { + spin_lock_irq(&driver_lock); + if (iic_pending == 0) { + interruptible_sleep_on_timeout(&(iic_wait[priv_data->index]), timeout*HZ ); + } else + iic_pending = 0; + spin_unlock_irq(&driver_lock); + } else { + // + // If interrupts are not enabled then delay for a reasonable amount + // of time and return. We expect that by time we return to the calling + // function that the IIC has finished our requested transaction and + // the status bit reflects this. + // + // udelay is probably not the best choice for this since it is + // the equivalent of a busy wait + // + udelay(100); + } + //printk("iic_ibmocp_waitforpin: exitting\n"); +} + + +// +// Description: The registered interrupt handler +// +static void iic_ibmocp_handler(int this_irq, void *dev_id, struct pt_regs *regs) +{ + int ret; + struct iic_regs *iic; + struct iic_ibm *priv_data = dev_id; + iic = (struct iic_regs *) priv_data->iic_base; + iic_pending = 1; + DEB2(printk("iic_ibmocp_handler: in interrupt handler\n")); + // Read status register + ret = readb((int) &(iic->sts)); + DEB2(printk("iic_ibmocp_handler: status = %x\n", ret)); + // Clear status register. See IBM PPC 405 reference manual for details + writeb(0x0a, (int) &(iic->sts)); + wake_up_interruptible(&(iic_wait[priv_data->index])); +} + + +// +// Description: This function is very hardware dependent. First, we lock +// the region of memory where out registers exist. Next, we request our +// interrupt line and register its associated handler. Our IIC peripheral +// uses interrupt number 2, as specified by the 405 reference manual. +// +static int iic_hw_resrc_init(int instance) +{ + + DEB(printk("iic_hw_resrc_init: Physical Base address: 0x%x\n", (u32) IIC_ADDR[instance] )); + iic_ibmocp_adaps[instance]->iic_base = (u32)ioremap((unsigned long)IIC_ADDR[instance],PAGE_SIZE); + + DEB(printk("iic_hw_resrc_init: ioremapped base address: 0x%x\n", iic_ibmocp_adaps[instance]->iic_base)); + + if (iic_ibmocp_adaps[instance]->iic_irq > 0) { + + if (request_irq(iic_ibmocp_adaps[instance]->iic_irq, iic_ibmocp_handler, + 0, "IBM OCP IIC", iic_ibmocp_adaps[instance]) < 0) { + printk(KERN_ERR "iic_hw_resrc_init: Request irq%d failed\n", + iic_ibmocp_adaps[instance]->iic_irq); + iic_ibmocp_adaps[instance]->iic_irq = 0; + } else { + DEB3(printk("iic_hw_resrc_init: Enabled interrupt\n")); + } + } + return 0; +} + + +// +// Description: Release irq and memory +// +static void iic_ibmocp_release(void) +{ + int i; + + for(i=0; i<IIC_NUMS; i++) { + struct iic_ibm *priv_data = (struct iic_ibm *)iic_ibmocp_data[i]->data; + if (priv_data->iic_irq > 0) { + disable_irq(priv_data->iic_irq); + free_irq(priv_data->iic_irq, 0); + } + kfree(iic_ibmocp_data[i]); + kfree(iic_ibmocp_ops[i]); + } +} + + +// +// Description: Does nothing +// +static int iic_ibmocp_reg(struct i2c_client *client) +{ + return 0; +} + + +// +// Description: Does nothing +// +static int iic_ibmocp_unreg(struct i2c_client *client) +{ + return 0; +} + + +// +// Description: If this compiled as a module, then increment the count +// +static void iic_ibmocp_inc_use(struct i2c_adapter *adap) +{ +#ifdef MODULE + MOD_INC_USE_COUNT; +#endif +} + + +// +// Description: If this is a module, then decrement the count +// +static void iic_ibmocp_dec_use(struct i2c_adapter *adap) +{ +#ifdef MODULE + MOD_DEC_USE_COUNT; +#endif +} + +// +// Description: Called when the module is loaded. This function starts the +// cascade of calls up through the heirarchy of i2c modules (i.e. up to the +// algorithm layer and into to the core layer) +// +static int __init iic_ibmocp_init(void) +{ + int i; + + printk(KERN_INFO "iic_ibmocp_init: IBM on-chip iic adapter module\n"); + + for(i=0; i<IIC_NUMS; i++) { + iic_ibmocp_data[i] = kmalloc(sizeof(struct i2c_algo_iic_data),GFP_KERNEL); + if(iic_ibmocp_data[i] == NULL) { + return -ENOMEM; + } + memset(iic_ibmocp_data[i], 0, sizeof(struct i2c_algo_iic_data)); + + switch (i) { + case 0: + iic_ibmocp_adaps[i]->iic_irq = IIC_IRQ(0); + break; + case 1: + iic_ibmocp_adaps[i]->iic_irq = IIC_IRQ(1); + break; + } + iic_ibmocp_adaps[i]->iic_clock = IIC_CLOCK; + iic_ibmocp_adaps[i]->iic_own = IIC_OWN; + iic_ibmocp_adaps[i]->index = i; + + DEB(printk("irq %x\n", iic_ibmocp_adaps[i]->iic_irq)); + DEB(printk("clock %x\n", iic_ibmocp_adaps[i]->iic_clock)); + DEB(printk("own %x\n", iic_ibmocp_adaps[i]->iic_own)); + DEB(printk("index %x\n", iic_ibmocp_adaps[i]->index)); + + + iic_ibmocp_data[i]->data = (struct iic_regs *)iic_ibmocp_adaps[i]; + iic_ibmocp_data[i]->setiic = iic_ibmocp_setbyte; + iic_ibmocp_data[i]->getiic = iic_ibmocp_getbyte; + iic_ibmocp_data[i]->getown = iic_ibmocp_getown; + iic_ibmocp_data[i]->getclock = iic_ibmocp_getclock; + iic_ibmocp_data[i]->waitforpin = iic_ibmocp_waitforpin; + iic_ibmocp_data[i]->udelay = 80; + iic_ibmocp_data[i]->mdelay = 80; + iic_ibmocp_data[i]->timeout = 100; + + iic_ibmocp_ops[i] = kmalloc(sizeof(struct i2c_adapter), GFP_KERNEL); + if(iic_ibmocp_ops[i] == NULL) { + return -ENOMEM; + } + memset(iic_ibmocp_ops[i], 0, sizeof(struct i2c_adapter)); + strcpy(iic_ibmocp_ops[i]->name, "IBM OCP IIC adapter"); + iic_ibmocp_ops[i]->id = I2C_HW_OCP; + iic_ibmocp_ops[i]->algo = NULL; + iic_ibmocp_ops[i]->algo_data = iic_ibmocp_data[i]; + iic_ibmocp_ops[i]->inc_use = iic_ibmocp_inc_use; + iic_ibmocp_ops[i]->dec_use = iic_ibmocp_dec_use; + iic_ibmocp_ops[i]->client_register = iic_ibmocp_reg; + iic_ibmocp_ops[i]->client_unregister = iic_ibmocp_unreg; + + + init_waitqueue_head(&(iic_wait[i])); + if (iic_hw_resrc_init(i) == 0) { + if (i2c_iic_add_bus(iic_ibmocp_ops[i]) < 0) + return -ENODEV; + } else { + return -ENODEV; + } + DEB(printk(KERN_INFO "iic_ibmocp_init: found device at %#x.\n\n", iic_ibmocp_adaps[i]->iic_base)); + } + return 0; +} + + +static void iic_ibmocp_exit(void) +{ + int i; + + for(i=0; i<IIC_NUMS; i++) { + i2c_iic_del_bus(iic_ibmocp_ops[i]); + } + iic_ibmocp_release(); +} + +EXPORT_NO_SYMBOLS; + +// +// If modules is NOT defined when this file is compiled, then the MODULE_* +// macros will resolve to nothing +// +MODULE_AUTHOR("MontaVista Software <www.mvista.com>"); +MODULE_DESCRIPTION("I2C-Bus adapter routines for PPC 405 IIC bus adapter"); +MODULE_PARM(base, "i"); +MODULE_PARM(irq, "i"); +MODULE_PARM(clock, "i"); +MODULE_PARM(own, "i"); +MODULE_PARM(i2c_debug,"i"); + + +module_init(iic_ibmocp_init); +module_exit(iic_ibmocp_exit);
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +592 -0
592 lines not on this page · raw message
include/linux/i2c-algo-8xx.h 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +43 -0
43 lines not on this page · raw message
drivers/i2c/i2c-algo-bit.c 2002-07-07 20:41:49.000000000 -0400 renamed from drivers/i2c/i2c-algo-bit.c.orig 2002-07-05 19:42:28.000000000 -0400 +2 -2
@@ -21,7 +21,7 @@ /* With some changes from Kyösti Mälkki <[email protected]> and even Frodo Looijaard <[email protected]> */ -/* $Id: i2c-algo-bit.c,v 1.34 2001/11/19 18:45:02 mds Exp $ */ +/* $Id: i2c-algo-bit.c,v 1.37 2002/07/08 00:41:49 mds Exp $ */ #include <linux/kernel.h> #include <linux/module.h>
@@ -143,7 +148,7 @@ /* scl, sda may not be high */ DEBPROTO(printk(" Sr ")); setsda(adap,1); - setscl(adap,1); + sclhi(adap); udelay(adap->udelay); sdalo(adap);
drivers/i2c/i2c-algo-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +966 -0
966 lines not on this page · raw message
drivers/i2c/i2c-algo-ibm_ocp.h 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +55 -0
55 lines not on this page · raw message
drivers/i2c/i2c-elektor.c 2001-11-19 13:45:02.000000000 -0500 renamed from drivers/i2c/i2c-elektor.c.orig 2002-07-05 19:42:02.000000000 -0400 +4 -2
@@ -119,14 +119,15 @@ static void pcf_isa_waitforpin(void) { int timeout = 2; + spinlock_t driver_lock = SPIN_LOCK_UNLOCKED; if (irq > 0) { - cli(); + spin_lock_irq(&driver_lock); if (pcf_pending == 0) { interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ ); } else pcf_pending = 0; - sti(); + spin_unlock_irq(&driver_lock); } else { udelay(100); }
@@ -297,6 +298,7 @@ return 0; } +EXPORT_NO_SYMBOLS; #ifdef MODULE MODULE_AUTHOR("Hans Berglund <[email protected]>");
drivers/i2c/i2c-frodo.c 2002-07-09 08:39:21.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +118 -0
118 lines not on this page · raw message
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +316 -0
316 lines not on this page · raw message
drivers/i2c/i2c-pport.c 2001-10-13 14:09:01.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +254 -0
254 lines not on this page · raw message
drivers/i2c/i2c-rpx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +136 -0
136 lines not on this page · raw message
Signature · 2 lines
Albert Cranford Deerfield Beach FL USA [email protected]
The following are _some_ comments from a quick read through; they're not a thorough review, so there's probably lots of stuff I've missed. But I felt I couldn't carry on reading anything else on lkml... 8)
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 164
Quoted 5 lines · Albert Cranford
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+static void iic_ibmocp_waitforpin(void *data) { + + int timeout = 2; + struct iic_ibm *priv_data = data; + spinlock_t driver_lock = SPIN_LOCK_UNLOCKED;
What's the point of a spinlock on the stack? It doesn't gain you any protection against other threads on a SMP system.
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 174
Quoted 4 lines
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +4
-0+ if (priv_data->iic_irq > 0) { + spin_lock_irq(&driver_lock); + if (iic_pending == 0) { + interruptible_sleep_on_timeout(&(iic_wait[priv_data->index]), timeout*HZ );
You shouldn't be scheduling with interrupts disabled, spinlock locked. Using a derivative of sleep_on(). Please look at wait_event() and interruptible_wait_event(). Also, interruptible_sleep_on() returns a value to tell you if it was interrupted...
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 254
Quoted 5 lines
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+ for(i=0; i<IIC_NUMS; i++) { + struct iic_ibm *priv_data = (struct iic_ibm *)iic_ibmocp_data[i]->data; + if (priv_data->iic_irq > 0) { + disable_irq(priv_data->iic_irq); + free_irq(priv_data->iic_irq, 0);
You shouldn't be calling disable_irq() just before free_irq(). Firstly, if the interrupt is being shared, then you just killed the other devices using that IRQ. Secondly, free_irq will disable the interrupt source for you, so its redundant anyway.
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 195
Quoted 9 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +9
-0+ 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);
Ouch. There should be a call to cpu_relax() in this (and any other such) while loops. (IIRC Athlons tend to self-destruct without this.)
Quoted 1 line
+
invalidate_dcache_range((unsigned long) buf, (unsigned long) (buf+count));
This is only defined on ARM and PPC; on ARM its not intended to be called by any driver directly (it should be using the pci_* DMA consistency stuff). On PPC, it looks like it should be a call to dma_cache_inv() which is the more accepted interface. You can find them in include/asm-ppc/io.h
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 259
Quoted 10 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +10
-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);
Again, sleeping with interrupts disabled...
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 317
Quoted 2 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +2
-0+ flush_dcache_range((unsigned long) tb, (unsigned long) (tb+1)); + flush_dcache_range((unsigned long) buf, (unsigned long) (buf+count));
Same comments as invalidate_dcache_range(); dma_cache_wback_inv().
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 138
Quoted 13 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +13
-0+static void pcf_epp_waitforpin(void) { + int timeout = 10; + spinlock_t driver_lock = SPIN_LOCK_UNLOCKED; + + if (gpe.pe_irq > 0) { + spin_lock_irq(&driver_lock); + if (pcf_pending == 0) { + interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ); + //udelay(100); + } else { + pcf_pending = 0; + } + spin_unlock_irq(&driver_lock);
See above.
Quoted 1 line
+ if (gpe.pe_irq > 0) {
An IRQ number of zero is completely valid on some systems.
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 169
Quoted 6 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +6
-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);
The indentation here makes the code look broken. However, I suspect the bug is balanced out because of the following bit of code:
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 186
Quoted 5 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+static void pcf_epp_exit(void) +{ + if (gpe.pe_irq > 0) { + disable_irq(gpe.pe_irq); + free_irq(gpe.pe_irq, 0);
See comments about disable_irq/free_irq above.
drivers/i2c/i2c-pport.c 2001-10-13 14:09:01.000000000 -0400 line 115
Quoted 3 lines
drivers/i2c/i2c-pport.c 2001-10-13 14:09:01.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +3
-0+static int bit_pport_init(void) +{ + //release_region( (base+2) ,1);
Eww. Please don't give people ideas about releasing someone elses resources.
Quoted 1 line
+ if (check_region((base+2),1) < 0 ) {
You should request the region first, then probe. If the probe fails, release the region. Using check_region is not safe on any 2.2 or later kernel (hint: you can sleep in request_region).
And finally, I think keeping all the ifdef crap for "I want i2c to build across any kernel what so ever" is really bad. For an example how to handle this, please see the jffs2/mtd code which has more or less the same requirement, but without the pain of having to put lots of ifdefs in the code. The code is written to support the latest kernel, and the compatibility stuff is tucked away inside a header file.
Signature · 2 lines
Russell King ([email protected]) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html
Linus, please hold off incorporating all 4 patches until we can get corrections. Thanks.
Rusty,
Couple of quick comments while I forward to I2C authors. First thanks for your effort to review our code. It is very much appreciated. Next time I'll post to kernel list before submitting to Linus.
I meant to remove the compatibility stuff from 2.5. Something I forgot to check in the new drivers.
The cli&sti replacement with driver_lock should be at the module level and not proc level for SMP safety. I'll change this in all three drivers.
The requirement to support I2C and Sensors for releases 2.2 through current is very challenging. I took your advice from the last time, but haven't completed the work yet.
The important clues you provided will be reviewed and incorporated in the next patch attempt. Thanks again,
Albert
Quoted 135 lines · Russell King
The following are _some_ comments from a quick read through; they're not a thorough review, so there's probably lots of stuff I've missed. But I felt I couldn't carry on reading anything else on lkml... 8)
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 164
Quoted 5 lines · Albert Cranford
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+static void iic_ibmocp_waitforpin(void *data) { + + int timeout = 2; + struct iic_ibm *priv_data = data; + spinlock_t driver_lock = SPIN_LOCK_UNLOCKED;What's the point of a spinlock on the stack? It doesn't gain you any protection against other threads on a SMP system.
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 174
Quoted 4 lines
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +4
-0+ if (priv_data->iic_irq > 0) { + spin_lock_irq(&driver_lock); + if (iic_pending == 0) { + interruptible_sleep_on_timeout(&(iic_wait[priv_data->index]), timeout*HZ );You shouldn't be scheduling with interrupts disabled, spinlock locked. Using a derivative of
sleep_on(). Please look atwait_event()andinterruptible_wait_event(). Also,interruptible_sleep_on()returns a value to tell you if it was interrupted...drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 254
Quoted 5 lines
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+ for(i=0; i<IIC_NUMS; i++) { + struct iic_ibm *priv_data = (struct iic_ibm *)iic_ibmocp_data[i]->data; + if (priv_data->iic_irq > 0) { + disable_irq(priv_data->iic_irq); + free_irq(priv_data->iic_irq, 0);You shouldn't be calling
disable_irq()just beforefree_irq(). Firstly, if the interrupt is being shared, then you just killed the other devices using that IRQ. Secondly,free_irqwill disable the interrupt source for you, so its redundant anyway.drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 195
Quoted 9 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +9
-0+ 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);Ouch. There should be a call to
cpu_relax()in this (and any other such) while loops. (IIRC Athlons tend to self-destruct without this.)Quoted 1 line
+
invalidate_dcache_range((unsigned long) buf, (unsigned long) (buf+count));This is only defined on ARM and PPC; on ARM its not intended to be called by any driver directly (it should be using the pci_* DMA consistency stuff). On PPC, it looks like it should be a call to
dma_cache_inv()which is the more accepted interface. You can find them ininclude/asm-ppc/io.hdrivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 259
Quoted 10 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +10
-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);Again, sleeping with interrupts disabled...
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 317
Quoted 2 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +2
-0+ flush_dcache_range((unsigned long) tb, (unsigned long) (tb+1)); + flush_dcache_range((unsigned long) buf, (unsigned long) (buf+count));Same comments as
invalidate_dcache_range();dma_cache_wback_inv().drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 138
Quoted 13 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +13
-0+static void pcf_epp_waitforpin(void) { + int timeout = 10; + spinlock_t driver_lock = SPIN_LOCK_UNLOCKED; + + if (gpe.pe_irq > 0) { + spin_lock_irq(&driver_lock); + if (pcf_pending == 0) { + interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ); + //udelay(100); + } else { + pcf_pending = 0; + } + spin_unlock_irq(&driver_lock);See above.
Quoted 1 line
+ if (gpe.pe_irq > 0) {
An IRQ number of zero is completely valid on some systems.
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 169
Quoted 6 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +6
-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);The indentation here makes the code look broken. However, I suspect the bug is balanced out because of the following bit of code:
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 186
Quoted 5 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+static void pcf_epp_exit(void) +{ + if (gpe.pe_irq > 0) { + disable_irq(gpe.pe_irq); + free_irq(gpe.pe_irq, 0);See comments about disable_irq/free_irq above.
drivers/i2c/i2c-pport.c 2001-10-13 14:09:01.000000000 -0400 line 115
Quoted 3 lines
drivers/i2c/i2c-pport.c 2001-10-13 14:09:01.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +3
-0+static int bit_pport_init(void) +{ + //release_region( (base+2) ,1);Eww. Please don't give people ideas about releasing someone elses resources.
Quoted 1 line
+ if (check_region((base+2),1) < 0 ) {
You should request the region first, then probe. If the probe fails, release the region. Using
check_regionis not safe on any 2.2 or later kernel (hint: you can sleep inrequest_region).And finally, I think keeping all the ifdef crap for "I want i2c to build across any kernel what so ever" is really bad. For an example how to handle this, please see the jffs2/mtd code which has more or less the same requirement, but without the pain of having to put lots of ifdefs in the code. The code is written to support the latest kernel, and the compatibility stuff is tucked away inside a header file.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
Signature · 2 lines
Albert Cranford Deerfield Beach FL USA [email protected]
PATCH 1/1 2.5.30 i2c updates 3/4
Armin, Tom,
As you know we checked in the 405 drivers you submitted to us in late March
and they are now in our 2.6.4 release.
We are attempting to fix up the locking in the 405 drivers for submission
to Linus. Can you help?
thanks
mds
Quoted 165 lines · Albert Cranford
Linus, please hold off incorporating all 4 patches until we can get corrections. Thanks.
Rusty,
Couple of quick comments while I forward to I2C authors. First thanks for your effort to review our code. It is very much appreciated. Next time I'll post to kernel list before submitting to Linus.I meant to remove the compatibility stuff from 2.5. Something I forgot to check in the new drivers.
The cli&sti replacement with
driver_lockshould be at the module level and not proc level for SMP safety. I'll change this in all three drivers.The requirement to support I2C and Sensors for releases 2.2 through current is very challenging. I took your advice from the last time, but haven't completed the work yet.
The important clues you provided will be reviewed and incorporated in the next patch attempt. Thanks again,
AlbertQuoted 131 lines · Russell King
The following are _some_ comments from a quick read through; they're not a thorough review, so there's probably lots of stuff I've missed. But I felt I couldn't carry on reading anything else on lkml... 8)
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 164
Quoted 5 lines · Albert Cranford
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+static void iic_ibmocp_waitforpin(void *data) { + + int timeout = 2; + struct iic_ibm *priv_data = data; + spinlock_t driver_lock = SPIN_LOCK_UNLOCKED;What's the point of a spinlock on the stack? It doesn't gain you any protection against other threads on a SMP system.
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 174
Quoted 4 lines
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +4
-0+ if (priv_data->iic_irq > 0) { + spin_lock_irq(&driver_lock); + if (iic_pending = 0) { + interruptible_sleep_on_timeout(&(iic_wait[priv_data->index]), timeout*HZ );You shouldn't be scheduling with interrupts disabled, spinlock locked. Using a derivative of
sleep_on(). Please look atwait_event()andinterruptible_wait_event(). Also,interruptible_sleep_on()returns a value to tell you if it was interrupted...drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 254
Quoted 5 lines
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+ for(i=0; i<IIC_NUMS; i++) { + struct iic_ibm *priv_data = (struct iic_ibm *)iic_ibmocp_data[i]->data; + if (priv_data->iic_irq > 0) { + disable_irq(priv_data->iic_irq); + free_irq(priv_data->iic_irq, 0);You shouldn't be calling
disable_irq()just beforefree_irq(). Firstly, if the interrupt is being shared, then you just killed the other devices using that IRQ. Secondly,free_irqwill disable the interrupt source for you, so its redundant anyway.drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 195
Quoted 8 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +8
-0+ 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);Ouch. There should be a call to
cpu_relax()in this (and any other such) while loops. (IIRC Athlons tend to self-destruct without this.)Quoted 1 line
+
invalidate_dcache_range((unsigned long) buf, (unsigned long) (buf+count));This is only defined on ARM and PPC; on ARM its not intended to be called by any driver directly (it should be using the pci_* DMA consistency stuff). On PPC, it looks like it should be a call to
dma_cache_inv()which is the more accepted interface. You can find them ininclude/asm-ppc/io.hdrivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 259
Quoted 10 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +10
-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);Again, sleeping with interrupts disabled...
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 317
Quoted 2 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +2
-0+ flush_dcache_range((unsigned long) tb, (unsigned long) (tb+1)); + flush_dcache_range((unsigned long) buf, (unsigned long) (buf+count));Same comments as
invalidate_dcache_range();dma_cache_wback_inv().drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 138
Quoted 13 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +13
-0+static void pcf_epp_waitforpin(void) { + int timeout = 10; + spinlock_t driver_lock = SPIN_LOCK_UNLOCKED; + + if (gpe.pe_irq > 0) { + spin_lock_irq(&driver_lock); + if (pcf_pending = 0) { + interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ); + //udelay(100); + } else { + pcf_pending = 0; + } + spin_unlock_irq(&driver_lock);See above.
Quoted 1 line
+ if (gpe.pe_irq > 0) {
An IRQ number of zero is completely valid on some systems.
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 169
Quoted 6 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +6
-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);The indentation here makes the code look broken. However, I suspect the bug is balanced out because of the following bit of code:
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 186
Quoted 5 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+static void pcf_epp_exit(void) +{ + if (gpe.pe_irq > 0) { + disable_irq(gpe.pe_irq); + free_irq(gpe.pe_irq, 0);See comments about disable_irq/free_irq above.
drivers/i2c/i2c-pport.c 2001-10-13 14:09:01.000000000 -0400 line 115
Quoted 3 lines
drivers/i2c/i2c-pport.c 2001-10-13 14:09:01.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +3
-0+static int bit_pport_init(void) +{ + //release_region( (base+2) ,1);Eww. Please don't give people ideas about releasing someone elses resources.
Quoted 1 line
+ if (check_region((base+2),1) < 0 ) {
You should request the region first, then probe. If the probe fails, release the region. Using
check_regionis not safe on any 2.2 or later kernel (hint: you can sleep inrequest_region).And finally, I think keeping all the ifdef crap for "I want i2c to build across any kernel what so ever" is really bad. For an example how to handle this, please see the jffs2/mtd code which has more or less the same requirement, but without the pain of having to put lots of ifdefs in the code. The code is written to support the latest kernel, and the compatibility stuff is tucked away inside a header file.
Quoted 3 lines
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html--
Albert Cranford Deerfield Beach FL USA
[email protected]
PATCH 1/1 2.5.30 i2c updates 3/4
In reply to Albert CranfordQuoted 9 lines · Mark D. Studebaker
Armin, Tom,
As you know we checked in the 405 drivers you submitted to us in late March
and they are now in our 2.6.4 release.We are attempting to fix up the locking in the 405 drivers for submission
to Linus. Can you help?
Quoted 185 lines
thanks
mdsQuoted 178 lines · Albert Cranford
Linus, please hold off incorporating all 4 patches until we can get corrections. Thanks.
Rusty,
Couple of quick comments while I forward to I2C authors. First thanks for your effort to review our code. It is very much appreciated. Next time I'll post to kernel list before submitting to Linus.I meant to remove the compatibility stuff from 2.5. Something I forgot to check in the new drivers.
The cli&sti replacement with
driver_lockshould be at the module level and not proc level for SMP safety. I'll change this in all three drivers.The requirement to support I2C and Sensors for releases 2.2 through current is very challenging. I took your advice from the last time, but haven't completed the work yet.
The important clues you provided will be reviewed and incorporated in the next patch attempt. Thanks again,
AlbertQuoted 143 lines · Russell King
The following are _some_ comments from a quick read through; they're not a thorough review, so there's probably lots of stuff I've missed. But I felt I couldn't carry on reading anything else on lkml... 8)
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 164
Quoted 6 lines · Albert Cranford
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+static void iic_ibmocp_waitforpin(void *data) { + + int timeout = 2; + struct iic_ibm *priv_data = data; + spinlock_t driver_lock = SPIN_LOCK_UNLOCKED;What's the point of a spinlock on the stack? It doesn't gain you any protection against other threads on a SMP system.
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 174
Quoted 5 lines
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +4
-0+ if (priv_data->iic_irq > 0) { + spin_lock_irq(&driver_lock); + if (iic_pending = 0) { + interruptible_sleep_on_timeout(&(iic_wait[priv_data->index]), timeout*HZ );You shouldn't be scheduling with interrupts disabled, spinlock locked. Using a derivative of
sleep_on(). Please look atwait_event()andinterruptible_wait_event(). Also,interruptible_sleep_on()returns a value to tell you if it was interrupted...drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 254
Quoted 6 lines
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+ for(i=0; i<IIC_NUMS; i++) { + struct iic_ibm *priv_data = (struct iic_ibm *)iic_ibmocp_data[i]->data; + if (priv_data->iic_irq > 0) { + disable_irq(priv_data->iic_irq); + free_irq(priv_data->iic_irq, 0);You shouldn't be calling
disable_irq()just beforefree_irq(). Firstly, if the interrupt is being shared, then you just killed the other devices using that IRQ. Secondly,free_irqwill disable the interrupt source for you, so its redundant anyway.drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 195
Quoted 9 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +8
-0+ 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);Ouch. There should be a call to
cpu_relax()in this (and any other such) while loops. (IIRC Athlons tend to self-destruct without this.)Quoted 2 lines
+
invalidate_dcache_range((unsigned long) buf, (unsigned long) (buf+count));This is only defined on ARM and PPC; on ARM its not intended to be called by any driver directly (it should be using the pci_* DMA consistency stuff). On PPC, it looks like it should be a call to
dma_cache_inv()which is the more accepted interface. You can find them ininclude/asm-ppc/io.hdrivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 259
Quoted 11 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +10
-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);Again, sleeping with interrupts disabled...
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 317
Quoted 3 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +2
-0+ flush_dcache_range((unsigned long) tb, (unsigned long) (tb+1)); + flush_dcache_range((unsigned long) buf, (unsigned long) (buf+count));Same comments as
invalidate_dcache_range();dma_cache_wback_inv().Quoted 4 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +3
-0+static void pcf_epp_waitforpin(void) { + int timeout = 10; + spinlock_t driver_lock = SPIN_LOCK_UNLOCKED;drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 138
Quoted 10 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +10
-0+ + if (gpe.pe_irq > 0) { + spin_lock_irq(&driver_lock); + if (pcf_pending = 0) { + interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ); + //udelay(100); + } else { + pcf_pending = 0; + } + spin_unlock_irq(&driver_lock);See above.
Quoted 2 lines
+ if (gpe.pe_irq > 0) {
An IRQ number of zero is completely valid on some systems.
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 169
Quoted 7 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +6
-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);The indentation here makes the code look broken. However, I suspect the bug is balanced out because of the following bit of code:
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 186
Quoted 6 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+static void pcf_epp_exit(void) +{ + if (gpe.pe_irq > 0) { + disable_irq(gpe.pe_irq); + free_irq(gpe.pe_irq, 0);See comments about disable_irq/free_irq above.
drivers/i2c/i2c-pport.c 2001-10-13 14:09:01.000000000 -0400 line 115
Quoted 4 lines
drivers/i2c/i2c-pport.c 2001-10-13 14:09:01.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +3
-0+static int bit_pport_init(void) +{ + //release_region( (base+2) ,1);Eww. Please don't give people ideas about releasing someone elses resources.
Quoted 2 lines
+ if (check_region((base+2),1) < 0 ) {
You should request the region first, then probe. If the probe fails, release the region. Using
check_regionis not safe on any 2.2 or later kernel (hint: you can sleep inrequest_region).And finally, I think keeping all the ifdef crap for "I want i2c to build across any kernel what so ever" is really bad. For an example how to handle this, please see the jffs2/mtd code which has more or less the same requirement, but without the pain of having to put lots of ifdefs in the code. The code is written to support the latest kernel, and the compatibility stuff is tucked away inside a header file.
Quoted 4 lines
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html--
Albert Cranford Deerfield Beach FL USA
[email protected]
Mark & others,
By fix up, you mean whats described in this mail thread? or is there something else.
Armin
PATCH 1/1 2.5.30 i2c updates 3/4
In reply to Albert CranfordSpecifically it was sti/cli removal.
I guess some of the issues raised in the mail below were related and some were not.
Forgive me, I am irq/locking stupid.
Albert Cranford, who is on the sensors mailing list, is creating and submitting the patches;
his patches are here
http://personal.atl.bellsouth.net/mia/a/c/ac9410/albert/albert.html
and his email is at the bottom of that page.
I think the 8xx driver also needs mods; Albert can fill you in. Any help you can give Albert directly, and (especially) pointers to anybody else doing the work or if you can verify that the 405/8xx drivers we have are the latest would be most helpful.
thanks
mds
Quoted 203 lines · akuster
Quoted 9 lines · Mark D. Studebaker
Armin, Tom,
As you know we checked in the 405 drivers you submitted to us in late March
and they are now in our 2.6.4 release.We are attempting to fix up the locking in the 405 drivers for submission
to Linus. Can you help?Quoted 185 lines
thanks
mdsQuoted 178 lines · Albert Cranford
Linus, please hold off incorporating all 4 patches until we can get corrections. Thanks.
Rusty,
Couple of quick comments while I forward to I2C authors. First thanks for your effort to review our code. It is very much appreciated. Next time I'll post to kernel list before submitting to Linus.I meant to remove the compatibility stuff from 2.5. Something I forgot to check in the new drivers.
The cli&sti replacement with
driver_lockshould be at the module level and not proc level for SMP safety. I'll change this in all three drivers.The requirement to support I2C and Sensors for releases 2.2 through current is very challenging. I took your advice from the last time, but haven't completed the work yet.
The important clues you provided will be reviewed and incorporated in the next patch attempt. Thanks again,
AlbertQuoted 143 lines · Russell King
The following are _some_ comments from a quick read through; they're not a thorough review, so there's probably lots of stuff I've missed. But I felt I couldn't carry on reading anything else on lkml... 8)
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 164
Quoted 6 lines · Albert Cranford
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+static void iic_ibmocp_waitforpin(void *data) { + + int timeout = 2; + struct iic_ibm *priv_data = data; + spinlock_t driver_lock = SPIN_LOCK_UNLOCKED;What's the point of a spinlock on the stack? It doesn't gain you any protection against other threads on a SMP system.
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 174
Quoted 5 lines
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +4
-0+ if (priv_data->iic_irq > 0) { + spin_lock_irq(&driver_lock); + if (iic_pending = 0) { + interruptible_sleep_on_timeout(&(iic_wait[priv_data->index]), timeout*HZ );You shouldn't be scheduling with interrupts disabled, spinlock locked. Using a derivative of
sleep_on(). Please look atwait_event()andinterruptible_wait_event(). Also,interruptible_sleep_on()returns a value to tell you if it was interrupted...drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 254
Quoted 6 lines
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+ for(i=0; i<IIC_NUMS; i++) { + struct iic_ibm *priv_data = (struct iic_ibm *)iic_ibmocp_data[i]->data; + if (priv_data->iic_irq > 0) { + disable_irq(priv_data->iic_irq); + free_irq(priv_data->iic_irq, 0);You shouldn't be calling
disable_irq()just beforefree_irq(). Firstly, if the interrupt is being shared, then you just killed the other devices using that IRQ. Secondly,free_irqwill disable the interrupt source for you, so its redundant anyway.drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 195
Quoted 9 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +8
-0+ 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);Ouch. There should be a call to
cpu_relax()in this (and any other such) while loops. (IIRC Athlons tend to self-destruct without this.)Quoted 2 lines
+
invalidate_dcache_range((unsigned long) buf, (unsigned long) (buf+count));This is only defined on ARM and PPC; on ARM its not intended to be called by any driver directly (it should be using the pci_* DMA consistency stuff). On PPC, it looks like it should be a call to
dma_cache_inv()which is the more accepted interface. You can find them ininclude/asm-ppc/io.hdrivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 259
Quoted 11 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +10
-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);Again, sleeping with interrupts disabled...
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 317
Quoted 3 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +2
-0+ flush_dcache_range((unsigned long) tb, (unsigned long) (tb+1)); + flush_dcache_range((unsigned long) buf, (unsigned long) (buf+count));Same comments as
invalidate_dcache_range();dma_cache_wback_inv().Quoted 4 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +3
-0+static void pcf_epp_waitforpin(void) { + int timeout = 10; + spinlock_t driver_lock = SPIN_LOCK_UNLOCKED;drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 138
Quoted 10 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +10
-0+ + if (gpe.pe_irq > 0) { + spin_lock_irq(&driver_lock); + if (pcf_pending = 0) { + interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ); + //udelay(100); + } else { + pcf_pending = 0; + } + spin_unlock_irq(&driver_lock);See above.
Quoted 2 lines
+ if (gpe.pe_irq > 0) {
An IRQ number of zero is completely valid on some systems.
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 169
Quoted 7 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +6
-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);The indentation here makes the code look broken. However, I suspect the bug is balanced out because of the following bit of code:
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 186
Quoted 6 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+static void pcf_epp_exit(void) +{ + if (gpe.pe_irq > 0) { + disable_irq(gpe.pe_irq); + free_irq(gpe.pe_irq, 0);See comments about disable_irq/free_irq above.
drivers/i2c/i2c-pport.c 2001-10-13 14:09:01.000000000 -0400 line 115
Quoted 4 lines
drivers/i2c/i2c-pport.c 2001-10-13 14:09:01.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +3
-0+static int bit_pport_init(void) +{ + //release_region( (base+2) ,1);Eww. Please don't give people ideas about releasing someone elses resources.
Quoted 2 lines
+ if (check_region((base+2),1) < 0 ) {
You should request the region first, then probe. If the probe fails, release the region. Using
check_regionis not safe on any 2.2 or later kernel (hint: you can sleep inrequest_region).And finally, I think keeping all the ifdef crap for "I want i2c to build across any kernel what so ever" is really bad. For an example how to handle this, please see the jffs2/mtd code which has more or less the same requirement, but without the pain of having to put lots of ifdefs in the code. The code is written to support the latest kernel, and the compatibility stuff is tucked away inside a header file.
Quoted 4 lines
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html--
Albert Cranford Deerfield Beach FL USA
[email protected]Mark & others,
By fix up, you mean whats described in this mail thread? or is there something else.
Armin
PATCH 1/1 2.5.30 i2c updates 3/4
In reply to Albert CranfordI have checked in CVS the fix for the cli locking on the stack. That was my mistake.
I have removed the "#ifdef" compatibility stuff.
I was refering to Russels comments about:
1) Scheduling with interrupts disabled.
2) Calling disable_irq before free_irq.
3) Loop without relax_cpu.
4) Calling invalidate_dcache_range improperly.
5) Checking if irq > 0 while 0 is valid.
6) Checking region before probing.
Quoted 225 lines · Mark D. Studebaker
Specifically it was sti/cli removal.
I guess some of the issues raised in the mail below were related and some were not.Forgive me, I am irq/locking stupid.
Albert Cranford, who is on the sensors mailing list, is creating and submitting the patches;
his patches are here
http://personal.atl.bellsouth.net/mia/a/c/ac9410/albert/albert.html
and his email is at the bottom of that page.I think the 8xx driver also needs mods; Albert can fill you in. Any help you can give Albert directly, and (especially) pointers to anybody else doing the work or if you can verify that the 405/8xx drivers we have are the latest would be most helpful.
thanks
mdsQuoted 203 lines · akuster
Quoted 9 lines · Mark D. Studebaker
Armin, Tom,
As you know we checked in the 405 drivers you submitted to us in late March
and they are now in our 2.6.4 release.We are attempting to fix up the locking in the 405 drivers for submission
to Linus. Can you help?Quoted 185 lines
thanks
mdsQuoted 178 lines · Albert Cranford
Linus, please hold off incorporating all 4 patches until we can get corrections. Thanks.
Rusty,
Couple of quick comments while I forward to I2C authors. First thanks for your effort to review our code. It is very much appreciated. Next time I'll post to kernel list before submitting to Linus.I meant to remove the compatibility stuff from 2.5. Something I forgot to check in the new drivers.
The cli&sti replacement with
driver_lockshould be at the module level and not proc level for SMP safety. I'll change this in all three drivers.The requirement to support I2C and Sensors for releases 2.2 through current is very challenging. I took your advice from the last time, but haven't completed the work yet.
The important clues you provided will be reviewed and incorporated in the next patch attempt. Thanks again,
AlbertQuoted 143 lines · Russell King
The following are _some_ comments from a quick read through; they're not a thorough review, so there's probably lots of stuff I've missed. But I felt I couldn't carry on reading anything else on lkml... 8)
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 164
Quoted 6 lines · Albert Cranford
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+static void iic_ibmocp_waitforpin(void *data) { + + int timeout = 2; + struct iic_ibm *priv_data = data; + spinlock_t driver_lock = SPIN_LOCK_UNLOCKED;What's the point of a spinlock on the stack? It doesn't gain you any protection against other threads on a SMP system.
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 174
Quoted 5 lines
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +4
-0+ if (priv_data->iic_irq > 0) { + spin_lock_irq(&driver_lock); + if (iic_pending = 0) { + interruptible_sleep_on_timeout(&(iic_wait[priv_data->index]), timeout*HZ );You shouldn't be scheduling with interrupts disabled, spinlock locked. Using a derivative of
sleep_on(). Please look atwait_event()andinterruptible_wait_event(). Also,interruptible_sleep_on()returns a value to tell you if it was interrupted...drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 line 254
Quoted 6 lines
drivers/i2c/i2c-adap-ibm_ocp.c 2002-07-03 21:00:38.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+ for(i=0; i<IIC_NUMS; i++) { + struct iic_ibm *priv_data = (struct iic_ibm *)iic_ibmocp_data[i]->data; + if (priv_data->iic_irq > 0) { + disable_irq(priv_data->iic_irq); + free_irq(priv_data->iic_irq, 0);You shouldn't be calling
disable_irq()just beforefree_irq(). Firstly, if the interrupt is being shared, then you just killed the other devices using that IRQ. Secondly,free_irqwill disable the interrupt source for you, so its redundant anyway.drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 195
Quoted 9 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +8
-0+ 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);Ouch. There should be a call to
cpu_relax()in this (and any other such) while loops. (IIRC Athlons tend to self-destruct without this.)Quoted 2 lines
+
invalidate_dcache_range((unsigned long) buf, (unsigned long) (buf+count));This is only defined on ARM and PPC; on ARM its not intended to be called by any driver directly (it should be using the pci_* DMA consistency stuff). On PPC, it looks like it should be a call to
dma_cache_inv()which is the more accepted interface. You can find them ininclude/asm-ppc/io.hdrivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 259
Quoted 11 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +10
-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);Again, sleeping with interrupts disabled...
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 line 317
Quoted 3 lines
drivers/i2c/i2c-algo-8xx.c 2002-06-11 22:29:54.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +2
-0+ flush_dcache_range((unsigned long) tb, (unsigned long) (tb+1)); + flush_dcache_range((unsigned long) buf, (unsigned long) (buf+count));Same comments as
invalidate_dcache_range();dma_cache_wback_inv().Quoted 4 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +3
-0+static void pcf_epp_waitforpin(void) { + int timeout = 10; + spinlock_t driver_lock = SPIN_LOCK_UNLOCKED;drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 138
Quoted 10 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +10
-0+ + if (gpe.pe_irq > 0) { + spin_lock_irq(&driver_lock); + if (pcf_pending = 0) { + interruptible_sleep_on_timeout(&pcf_wait, timeout*HZ); + //udelay(100); + } else { + pcf_pending = 0; + } + spin_unlock_irq(&driver_lock);See above.
Quoted 2 lines
+ if (gpe.pe_irq > 0) {
An IRQ number of zero is completely valid on some systems.
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 169
Quoted 7 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +6
-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);The indentation here makes the code look broken. However, I suspect the bug is balanced out because of the following bit of code:
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 line 186
Quoted 6 lines
drivers/i2c/i2c-pcf-epp.c 2002-07-09 21:29:56.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +5
-0+static void pcf_epp_exit(void) +{ + if (gpe.pe_irq > 0) { + disable_irq(gpe.pe_irq); + free_irq(gpe.pe_irq, 0);See comments about disable_irq/free_irq above.
drivers/i2c/i2c-pport.c 2001-10-13 14:09:01.000000000 -0400 line 115
Quoted 4 lines
drivers/i2c/i2c-pport.c 2001-10-13 14:09:01.000000000 -0400 renamed from dev/null 1994-07-17 19:46:18.000000000 -0400 +3
-0+static int bit_pport_init(void) +{ + //release_region( (base+2) ,1);Eww. Please don't give people ideas about releasing someone elses resources.
Quoted 2 lines
+ if (check_region((base+2),1) < 0 ) {
You should request the region first, then probe. If the probe fails, release the region. Using
check_regionis not safe on any 2.2 or later kernel (hint: you can sleep inrequest_region).And finally, I think keeping all the ifdef crap for "I want i2c to build across any kernel what so ever" is really bad. For an example how to handle this, please see the jffs2/mtd code which has more or less the same requirement, but without the pain of having to put lots of ifdefs in the code. The code is written to support the latest kernel, and the compatibility stuff is tucked away inside a header file.
Quoted 4 lines
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html--
Albert Cranford Deerfield Beach FL USA
[email protected]Mark & others,
By fix up, you mean whats described in this mail thread? or is there something else.
Armin
Signature · 2 lines
Albert Cranford Deerfield Beach FL USA [email protected]