You changes to the patch work OK for me.
Sonic Zhang
On Mon, Dec 14, 2009 at 5:12 AM, Jean Delvare <khali@xxxxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
