Re: [PATCH] driver i2c-core: i2c bus should support PM entries in struct dev_pm_ops.

[Sonic Zhang <sonic.adi@gmail.com>]

You changes to the patch work OK for me.


Sonic Zhang


On Mon, Dec 14, 2009 at 5:12 AM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Sonic,
>
> On Wed, 2 Dec 2009 17:08:40 +0800, sonic zhang wrote:
>> Struct dev_pm_ops is not configured in current i2c bus type. i2c drivers
>> only depends on suspend/resume entries in struct dev_pm_ops are not
>> informed of PM suspend and resume events by i2c framework.
>
> Good point. I was wondering some times ago what was the status of
> pm_ops. I seem to understand this is the future of power management,
> and "direct" .suspend and .resume callbacks will stop being supported
> at some point in the future? If so, what is the migration plan? I don't
> really want to support both forever.
>
> Your patch introduces the following warnings:
>
> drivers/i2c/i2c-core.c: In function ‘i2c_device_pm_suspend’:
> drivers/i2c/i2c-core.c:167: warning: assignment discards qualifiers from pointer target type
> drivers/i2c/i2c-core.c: In function ‘i2c_device_pm_resume’:
> drivers/i2c/i2c-core.c:181: warning: assignment discards qualifiers from pointer target type
>
> You will obviously have to fix them before I can accept your patch.
> Missing const...
>
>>
>> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
>> ---
>>  drivers/i2c/i2c-core.c |   39 +++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 39 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index 2965043..9713c0d 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -155,6 +155,39 @@ static void i2c_device_shutdown(struct device *dev)
>>               driver->shutdown(client);
>>  }
>>
>> +#ifdef CONFIG_SUSPEND
>> +static int i2c_device_pm_suspend(struct device *dev)
>> +{
>> +     struct i2c_driver *driver;
>> +     struct dev_pm_ops *pm;
>> +
>> +     if (!dev->driver)
>> +             return 0;
>> +     driver = to_i2c_driver(dev->driver);
>> +     pm = driver->driver.pm;
>
> This makes little sense, sorry. Why go from device_driver to i2c_driver
> and then again to device_driver? The following is equivalent and more
> efficient:
>
>        pm = dev->driver->pm;
>
>> +     if (!pm || !pm->suspend)
>> +             return 0;
>> +     return pm->suspend(dev);
>> +}
>> +
>> +static int i2c_device_pm_resume(struct device *dev)
>> +{
>> +     struct i2c_driver *driver;
>> +     struct dev_pm_ops *pm;
>> +
>> +     if (!dev->driver)
>> +             return 0;
>> +     driver = to_i2c_driver(dev->driver);
>> +     pm = driver->driver.pm;
>
> Same here.
>
>> +     if (!pm || !pm->resume)
>> +             return 0;
>> +     return pm->resume(dev);
>> +}
>> +#else
>> +# define i2c_device_pm_suspend       NULL
>> +# define i2c_device_pm_resume        NULL
>
> No space between "#" and "define", please.
>
>> +#endif
>> +
>>  static int i2c_device_suspend(struct device *dev, pm_message_t mesg)
>>  {
>>       struct i2c_client *client = i2c_verify_client(dev);
>> @@ -219,6 +252,11 @@ static const struct attribute_group *i2c_dev_attr_groups[] = {
>>       NULL
>>  };
>>
>> +static struct dev_pm_ops i2c_device_pm_ops = {
>
> Could be const.
>
>> +     .suspend = i2c_device_pm_suspend,
>> +     .resume = i2c_device_pm_resume,
>> +};
>> +
>>  struct bus_type i2c_bus_type = {
>>       .name           = "i2c",
>>       .match          = i2c_device_match,
>> @@ -227,6 +265,7 @@ struct bus_type i2c_bus_type = {
>>       .shutdown       = i2c_device_shutdown,
>>       .suspend        = i2c_device_suspend,
>>       .resume         = i2c_device_resume,
>> +     .pm             = &i2c_device_pm_ops,
>>  };
>>  EXPORT_SYMBOL_GPL(i2c_bus_type);
>>
>
> I've made all the changes above myself, it build OK but I can't test.
> Modified patch is here:
> ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-core-i2c-bus-should-support-pm-entries-in-struct-dev_pm_ops.patch
>
> Please test and report if anything's wrong.
>
> Thanks,
> --
> Jean Delvare
>