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

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

[sonic zhang]

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.

Signed-off-by: Sonic Zhang <sonic.zhang-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
---
 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;
+	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;
+	if (!pm || !pm->resume)
+		return 0;
+	return pm->resume(dev);
+}
+#else
+# define i2c_device_pm_suspend	NULL
+# define i2c_device_pm_resume	NULL
+#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 = {
+	.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);
 
-- 
1.6.0

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

[Sonic Zhang]

Any comments?

Thanks.

Sonic

On Wed, Dec 2, 2009 at 5:08 PM, sonic zhang <sonic.adi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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.
>
> Signed-off-by: Sonic Zhang <sonic.zhang-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> ---
>  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;
> +       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;
> +       if (!pm || !pm->resume)
> +               return 0;
> +       return pm->resume(dev);
> +}
> +#else
> +# define i2c_device_pm_suspend NULL
> +# define i2c_device_pm_resume  NULL
> +#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 = {
> +       .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);
>
> --
> 1.6.0
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

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

[Jean Delvare]

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

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

[Sonic Zhang]

You changes to the patch work OK for me.


Sonic Zhang


On Mon, Dec 14, 2009 at 5:12 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.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-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>> ---
>>  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
>