Messages in this thread | | | Date | Wed, 21 May 2008 09:30:59 +0800 | From | "Dave Young" <> | Subject | Re: [PATCH 1/3] i2c : use class_for_each_device |
| |
On Wed, May 21, 2008 at 9:29 AM, Dave Young <hidave.darkstar@gmail.com> wrote: > On Wed, May 21, 2008 at 1:31 AM, Jean Delvare <khali@linux-fr.org> wrote: >> Hi Dave, >> >> On Fri, 9 May 2008 15:20:29 +0800, Dave Young wrote: >>> Use class_for_each_device for iteration. >>> >>> Signed-off-by: Dave Young <hidave.darkstar@gmail.com> >>> >>> --- >>> drivers/i2c/i2c-core.c | 96 ++++++++++++++++++++++++++----------------------- >>> 1 file changed, 52 insertions(+), 44 deletions(-) >>> >>> diff -upr a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c >>> --- a/drivers/i2c/i2c-core.c 2008-05-09 13:55:41.000000000 +0800 >>> +++ b/drivers/i2c/i2c-core.c 2008-05-09 14:24:07.000000000 +0800 >>> @@ -35,7 +35,6 @@ >>> #include <linux/completion.h> >>> #include <linux/hardirq.h> >>> #include <linux/irqflags.h> >>> -#include <linux/semaphore.h> >>> #include <asm/uaccess.h> >>> >>> #include "i2c-core.h" >>> @@ -655,6 +654,16 @@ EXPORT_SYMBOL(i2c_del_adapter); >>> >>> >>> /* ------------------------------------------------------------------------- */ >>> +static int __attach_adapter(struct device *dev, void *data) >>> +{ >>> + struct i2c_adapter *adapter; >>> + struct i2c_driver *driver = data; >>> + >>> + adapter = container_of(dev, struct i2c_adapter, dev); >> >> You could use to_i2c_adapter(dev). >> >>> + driver->attach_adapter(adapter); >>> + >>> + return 0; >>> +} >>> >>> /* >>> * An i2c_driver is used with one or more i2c_client (device) nodes to access >>> @@ -696,21 +705,52 @@ int i2c_register_driver(struct module *o >>> pr_debug("i2c-core: driver [%s] registered\n", driver->driver.name); >>> >>> /* legacy drivers scan i2c busses directly */ >>> - if (driver->attach_adapter) { >>> - struct i2c_adapter *adapter; >>> + if (driver->attach_adapter) >>> + class_for_each_device(&i2c_adapter_class, driver, >>> + __attach_adapter); >>> + >>> + mutex_unlock(&core_lock); >>> + return 0; >>> +} >>> +EXPORT_SYMBOL(i2c_register_driver); >>> + >>> +static int __detach_adapter(struct device *dev, void *data) >>> +{ >>> + struct list_head *item2, *_n; >>> + struct i2c_client *client; >>> + struct i2c_adapter *adap; >>> + struct i2c_driver *driver = data; >>> >>> - down(&i2c_adapter_class.sem); >>> - list_for_each_entry(adapter, &i2c_adapter_class.devices, >>> - dev.node) { >>> - driver->attach_adapter(adapter); >>> + /* Have a look at each adapter, if clients of this driver >>> + * are still attached. If so, detach them to be able to >>> + * kill the driver afterwards. >>> + */ >>> + adap = container_of(dev, struct i2c_adapter, dev); >> >> You could use to_i2c_adapter(dev). >> >>> + if (driver->detach_adapter) { >> >> Testing for a method... >> >>> + if (driver->attach_adapter(adap)) >> >> ... but calling another one? Can't be correct. Which raises a question: >> you didn't test your patch, did you? I'm also surprised how you managed >> to mess this up, given that all you had to do was to move already >> existing code around. > > Thanks for review, It's only boot-tested with i2c part as built-in by > me because I have the i2c device to test.
should be "have no the i2c device to test" ;)
> >> >>> + dev_err(&adap->dev, "detach_adapter failed " >>> + "for driver [%s]\n", >>> + driver->driver.name); >>> + } else { >>> + list_for_each_safe(item2, _n, &adap->clients) { >>> + client = list_entry(item2, struct i2c_client, >>> + list); >>> + if (client->driver != driver) >>> + continue; >>> + dev_dbg(&adap->dev, "detaching client [%s] " >>> + "at 0x%02x\n", client->name, >>> + client->addr); >>> + if (driver->detach_client(client)) { >>> + dev_err(&adap->dev, "detach_client " >>> + "failed for client [%s] at " >>> + "0x%02x\n", client->name, >>> + client->addr); >>> + } >>> } >>> - up(&i2c_adapter_class.sem); >>> } >>> >>> - mutex_unlock(&core_lock); >>> return 0; >>> } >>> -EXPORT_SYMBOL(i2c_register_driver); >>> >>> /** >>> * i2c_del_driver - unregister I2C driver >>> @@ -719,46 +759,14 @@ EXPORT_SYMBOL(i2c_register_driver); >>> */ >>> void i2c_del_driver(struct i2c_driver *driver) >>> { >>> - struct list_head *item2, *_n; >>> - struct i2c_client *client; >>> - struct i2c_adapter *adap; >>> - >>> mutex_lock(&core_lock); >>> >>> /* new-style driver? */ >>> if (is_newstyle_driver(driver)) >>> goto unregister; >>> >>> - /* Have a look at each adapter, if clients of this driver are still >>> - * attached. If so, detach them to be able to kill the driver >>> - * afterwards. >>> - */ >>> - down(&i2c_adapter_class.sem); >>> - list_for_each_entry(adap, &i2c_adapter_class.devices, dev.node) { >>> - if (driver->detach_adapter) { >>> - if (driver->detach_adapter(adap)) { >>> - dev_err(&adap->dev, "detach_adapter failed " >>> - "for driver [%s]\n", >>> - driver->driver.name); >>> - } >>> - } else { >>> - list_for_each_safe(item2, _n, &adap->clients) { >>> - client = list_entry(item2, struct i2c_client, list); >>> - if (client->driver != driver) >>> - continue; >>> - dev_dbg(&adap->dev, "detaching client [%s] " >>> - "at 0x%02x\n", client->name, >>> - client->addr); >>> - if (driver->detach_client(client)) { >>> - dev_err(&adap->dev, "detach_client " >>> - "failed for client [%s] at " >>> - "0x%02x\n", client->name, >>> - client->addr); >>> - } >>> - } >>> - } >>> - } >>> - up(&i2c_adapter_class.sem); >>> + class_for_each_device(&i2c_adapter_class, driver, >>> + __detach_adapter); >>> >>> unregister: >>> driver_unregister(&driver->driver); >> >> The rest looks OK. I'll fix the patch myself and add it to my i2c tree >> now. > > Glad to see that, thanks. > > Regards > dave >
| |