Patchwork [1/2] i2c: designware: add CONFIG_PM_SLEEP to suspend/resume functions

login
register
mail settings
Submitter Jingoo Han
Date March 22, 2013, 2:13 a.m.
Message ID <00b901ce26a2$cb3c57d0$61b50770$%han@samsung.com>
Download mbox | patch
Permalink /patch/229867/
State Changes Requested
Headers show

Comments

Jingoo Han - March 22, 2013, 2:13 a.m.
Add CONFIG_PM_SLEEP to suspend/resume functions to fix the following
build warning when CONFIG_PM_SLEEP is not selected. This is because
sleep PM callbacks defined by SIMPLE_DEV_PM_OPS are only used when
the CONFIG_PM_SLEEP is enabled.

drivers/i2c/busses/i2c-designware-platdrv.c:253:12: warning: 'dw_i2c_suspend' defined but not used [-Wunused-function]
drivers/i2c/busses/i2c-designware-platdrv.c:263:12: warning: 'dw_i2c_resume' defined but not used [-Wunused-function]

Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Wolfram Sang - March 22, 2013, 11:46 a.m.
On Fri, Mar 22, 2013 at 11:13:07AM +0900, Jingoo Han wrote:
> Add CONFIG_PM_SLEEP to suspend/resume functions to fix the following
> build warning when CONFIG_PM_SLEEP is not selected. This is because
> sleep PM callbacks defined by SIMPLE_DEV_PM_OPS are only used when
> the CONFIG_PM_SLEEP is enabled.
> 
> drivers/i2c/busses/i2c-designware-platdrv.c:253:12: warning: 'dw_i2c_suspend' defined but not used [-Wunused-function]
> drivers/i2c/busses/i2c-designware-platdrv.c:263:12: warning: 'dw_i2c_resume' defined but not used [-Wunused-function]
> 
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>

What about doing it like the s3c2410 driver which also adds CONFIG_PM
around the whole structure? (comment applies for both patches)

Thanks,

   Wolfram

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jingoo Han - April 9, 2013, 12:07 a.m.
On Friday, March 22, 2013 8:47 PM, Wolfram Sang wrote:
> 
> On Fri, Mar 22, 2013 at 11:13:07AM +0900, Jingoo Han wrote:
> > Add CONFIG_PM_SLEEP to suspend/resume functions to fix the following
> > build warning when CONFIG_PM_SLEEP is not selected. This is because
> > sleep PM callbacks defined by SIMPLE_DEV_PM_OPS are only used when
> > the CONFIG_PM_SLEEP is enabled.
> >
> > drivers/i2c/busses/i2c-designware-platdrv.c:253:12: warning: 'dw_i2c_suspend' defined but not used [-
> Wunused-function]
> > drivers/i2c/busses/i2c-designware-platdrv.c:263:12: warning: 'dw_i2c_resume' defined but not used [-
> Wunused-function]
> >
> > Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> 
> What about doing it like the s3c2410 driver which also adds CONFIG_PM
> around the whole structure? (comment applies for both patches)

Hi Wolfram Sang,
Sorry for being late.

It is similar with s3c2410 driver.
However, CONFIG_PM is not necessary, when only SIMPLE_DEV_PM_OPS is
used.

Please refer to 'include/linux/pm.h'.
SIMPLE_DEV_PM_OPS macro uses SET_SYSTEM_SLEEP_PM_OPS macro.
Also, SET_SYSTEM_SLEEP_PM_OPS macro is defined as below.
Thus, adding CONFIG_PM is not necessary, when only SIMPLE_DEV_PM_OPS
is used.

327 #define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
328 const struct dev_pm_ops name = { \
329         SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
330 }

302 #ifdef CONFIG_PM_SLEEP
303 #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
304         .suspend = suspend_fn, \
305         .resume = resume_fn, \
306         .freeze = suspend_fn, \
307         .thaw = resume_fn, \
308         .poweroff = suspend_fn, \
309         .restore = resume_fn,
310 #else
311 #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
312 #endif

Best regards,
Jingoo Han


> 
> Thanks,
> 
>    Wolfram

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang - April 9, 2013, 8:50 a.m.
On Tue, Apr 09, 2013 at 09:07:10AM +0900, Jingoo Han wrote:
> On Friday, March 22, 2013 8:47 PM, Wolfram Sang wrote:
> > 
> > On Fri, Mar 22, 2013 at 11:13:07AM +0900, Jingoo Han wrote:
> > > Add CONFIG_PM_SLEEP to suspend/resume functions to fix the following
> > > build warning when CONFIG_PM_SLEEP is not selected. This is because
> > > sleep PM callbacks defined by SIMPLE_DEV_PM_OPS are only used when
> > > the CONFIG_PM_SLEEP is enabled.
> > >
> > > drivers/i2c/busses/i2c-designware-platdrv.c:253:12: warning: 'dw_i2c_suspend' defined but not used [-
> > Wunused-function]
> > > drivers/i2c/busses/i2c-designware-platdrv.c:263:12: warning: 'dw_i2c_resume' defined but not used [-
> > Wunused-function]
> > >
> > > Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> > 
> > What about doing it like the s3c2410 driver which also adds CONFIG_PM
> > around the whole structure? (comment applies for both patches)
> 
> Hi Wolfram Sang,
> Sorry for being late.
> 
> It is similar with s3c2410 driver.
> However, CONFIG_PM is not necessary, when only SIMPLE_DEV_PM_OPS is
> used.

Still, you are always creating a dev_pm_ops structure, even if empty in
case of no CONFIG_PM. The s3c driver skips that and uses NULL for
setting the pm member of the driver struct then.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jingoo Han - April 10, 2013, 12:26 a.m.
On Tuesday, April 09, 2013 5:51 PM, Wolfram Sang wrote:
> On Tue, Apr 09, 2013 at 09:07:10AM +0900, Jingoo Han wrote:
> > On Friday, March 22, 2013 8:47 PM, Wolfram Sang wrote:
> > >
> > > On Fri, Mar 22, 2013 at 11:13:07AM +0900, Jingoo Han wrote:
> > > > Add CONFIG_PM_SLEEP to suspend/resume functions to fix the following
> > > > build warning when CONFIG_PM_SLEEP is not selected. This is because
> > > > sleep PM callbacks defined by SIMPLE_DEV_PM_OPS are only used when
> > > > the CONFIG_PM_SLEEP is enabled.
> > > >
> > > > drivers/i2c/busses/i2c-designware-platdrv.c:253:12: warning: 'dw_i2c_suspend' defined but not used
> [-
> > > Wunused-function]
> > > > drivers/i2c/busses/i2c-designware-platdrv.c:263:12: warning: 'dw_i2c_resume' defined but not used
> [-
> > > Wunused-function]
> > > >
> > > > Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> > >
> > > What about doing it like the s3c2410 driver which also adds CONFIG_PM
> > > around the whole structure? (comment applies for both patches)
> >
> > Hi Wolfram Sang,
> > Sorry for being late.
> >
> > It is similar with s3c2410 driver.
> > However, CONFIG_PM is not necessary, when only SIMPLE_DEV_PM_OPS is
> > used.
> 
> Still, you are always creating a dev_pm_ops structure, even if empty in
> case of no CONFIG_PM. The s3c driver skips that and uses NULL for
> setting the pm member of the driver struct then.

I know it.

When SIMPLE_DEV_PM_OPS is used and CONFIG_PM is NOT selected,
call back functions (suspend, resume) are empty.
Then it cannot be called by PM framework.

Of course in this case, it requires additional NULL checking by
PM framework.

However, in my opinion, it would not be critical.
Now, many drivers are using SIMPLE_DEV_PM_OPS macro in this way,
not adding CONFIG_PM. There is no side effect.


Best regards,
Jingoo Han


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jingoo Han - April 10, 2013, 12:31 a.m.
On Tuesday, April 09, 2013 5:51 PM, Wolfram Sang wrote:
> -----Original Message-----
> From: Wolfram Sang [mailto:wsa@the-dreams.de]
> Sent: Tuesday, April 09, 2013 5:51 PM
> To: Jingoo Han
> Cc: 'Wolfram Sang'; linux-i2c@vger.kernel.org; 'Ben Dooks'
> Subject: Re: [PATCH 1/2] i2c: designware: add CONFIG_PM_SLEEP to suspend/resume functions
> 
> On Tue, Apr 09, 2013 at 09:07:10AM +0900, Jingoo Han wrote:
> > On Friday, March 22, 2013 8:47 PM, Wolfram Sang wrote:
> > >
> > > On Fri, Mar 22, 2013 at 11:13:07AM +0900, Jingoo Han wrote:
> > > > Add CONFIG_PM_SLEEP to suspend/resume functions to fix the following
> > > > build warning when CONFIG_PM_SLEEP is not selected. This is because
> > > > sleep PM callbacks defined by SIMPLE_DEV_PM_OPS are only used when
> > > > the CONFIG_PM_SLEEP is enabled.
> > > >
> > > > drivers/i2c/busses/i2c-designware-platdrv.c:253:12: warning: 'dw_i2c_suspend' defined but not used
> [-
> > > Wunused-function]
> > > > drivers/i2c/busses/i2c-designware-platdrv.c:263:12: warning: 'dw_i2c_resume' defined but not used
> [-
> > > Wunused-function]
> > > >
> > > > Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> > >
> > > What about doing it like the s3c2410 driver which also adds CONFIG_PM
> > > around the whole structure? (comment applies for both patches)
> >
> > Hi Wolfram Sang,
> > Sorry for being late.
> >
> > It is similar with s3c2410 driver.
> > However, CONFIG_PM is not necessary, when only SIMPLE_DEV_PM_OPS is
> > used.
> 
> Still, you are always creating a dev_pm_ops structure, even if empty in
> case of no CONFIG_PM. The s3c driver skips that and uses NULL for
> setting the pm member of the driver struct then.

CC'ed Rafael J. Wysocki


I know it.

When SIMPLE_DEV_PM_OPS is used and CONFIG_PM is NOT selected,
call back functions (suspend, resume) are empty.
Then it cannot be called by PM framework.

Of course in this case, it requires additional NULL checking by
PM framework.

However, in my opinion, it would not be critical.
Now, many drivers are using SIMPLE_DEV_PM_OPS macro in this way,
not adding CONFIG_PM. There is no side effect.


Best regards,
Jingoo Han


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0ceb6e1..f45b9cc 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -249,7 +249,7 @@  static const struct of_device_id dw_i2c_of_match[] = {
 MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
 #endif
 
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
 static int dw_i2c_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);