Re: [PATCH 1/3] i2c : use class_for_each_device

["Dave Young" <hidave.darkstar@gmail.com>]

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
>