* [PATCH v2 0/2] smiapp fix for w/o CONFIG_PM, compiler warnings @ 2016-11-19 22:24 Sakari Ailus 2016-11-19 22:24 ` [PATCH v2 1/2] smiapp: Implement power-on and power-off sequences without runtime PM Sakari Ailus 2016-11-19 22:24 ` [PATCH v2 2/2] smiapp: Make suspend and resume functions __maybe_unused Sakari Ailus 0 siblings, 2 replies; 7+ messages in thread From: Sakari Ailus @ 2016-11-19 22:24 UTC (permalink / raw) To: linux-media; +Cc: arnd Hi, The two patches fix the PM / compiler warning issues Arnd found in the smiapp driver. since v1: - Use IS_ENABLED() instead of preprocessor macros. - Add another patch to fix compiler warnings. -- Kind regards, Sakari ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] smiapp: Implement power-on and power-off sequences without runtime PM 2016-11-19 22:24 [PATCH v2 0/2] smiapp fix for w/o CONFIG_PM, compiler warnings Sakari Ailus @ 2016-11-19 22:24 ` Sakari Ailus 2016-11-29 17:28 ` [PATCH v2.1 " Sakari Ailus 2016-11-19 22:24 ` [PATCH v2 2/2] smiapp: Make suspend and resume functions __maybe_unused Sakari Ailus 1 sibling, 1 reply; 7+ messages in thread From: Sakari Ailus @ 2016-11-19 22:24 UTC (permalink / raw) To: linux-media; +Cc: arnd Power on the sensor when the module is loaded and power it off when it is removed. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/i2c/smiapp/smiapp-core.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c index 59872b3..b20886f 100644 --- a/drivers/media/i2c/smiapp/smiapp-core.c +++ b/drivers/media/i2c/smiapp/smiapp-core.c @@ -2741,8 +2741,6 @@ static const struct v4l2_subdev_internal_ops smiapp_internal_ops = { * I2C Driver */ -#ifdef CONFIG_PM - static int smiapp_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); @@ -2783,13 +2781,6 @@ static int smiapp_resume(struct device *dev) return rval; } -#else - -#define smiapp_suspend NULL -#define smiapp_resume NULL - -#endif /* CONFIG_PM */ - static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev) { struct smiapp_hwconfig *hwcfg; @@ -2915,7 +2906,10 @@ static int smiapp_probe(struct i2c_client *client, pm_runtime_enable(&client->dev); - rval = pm_runtime_get_sync(&client->dev); + if (IS_ENABLED(CONFIG_PM)) + rval = pm_runtime_get_sync(&client->dev); + else + rval = smiapp_power_on(&client->dev); if (rval < 0) { rval = -ENODEV; goto out_power_off; @@ -3113,7 +3107,10 @@ static int smiapp_probe(struct i2c_client *client, smiapp_cleanup(sensor); out_power_off: - pm_runtime_put(&client->dev); + if (IS_ENABLED(CONFIG_PM)) + pm_runtime_put(&client->dev); + else + smiapp_power_off(&client->dev); pm_runtime_disable(&client->dev); return rval; @@ -3127,7 +3124,10 @@ static int smiapp_remove(struct i2c_client *client) v4l2_async_unregister_subdev(subdev); - pm_runtime_suspend(&client->dev); + if (IS_ENABLED(CONFIG_PM)) + pm_runtime_suspend(&client->dev); + else + smiapp_power_off(&client->dev); pm_runtime_disable(&client->dev); for (i = 0; i < sensor->ssds_used; i++) { -- 2.1.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2.1 1/2] smiapp: Implement power-on and power-off sequences without runtime PM 2016-11-19 22:24 ` [PATCH v2 1/2] smiapp: Implement power-on and power-off sequences without runtime PM Sakari Ailus @ 2016-11-29 17:28 ` Sakari Ailus 2016-11-29 17:35 ` Laurent Pinchart 2016-11-30 16:11 ` [PATCH v2.2 " Sakari Ailus 0 siblings, 2 replies; 7+ messages in thread From: Sakari Ailus @ 2016-11-29 17:28 UTC (permalink / raw) To: stern; +Cc: laurent.pinchart, linux-media, linux-pm Power on the sensor when the module is loaded and power it off when it is removed. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- Hi Alan and Laurent, I hope this should be good then. I'm only enabling runtime PM at the end of probe() when all is well, which reduces need for error handling. Regards, Sakari drivers/media/i2c/smiapp/smiapp-core.c | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c index 59872b3..683a3e0 100644 --- a/drivers/media/i2c/smiapp/smiapp-core.c +++ b/drivers/media/i2c/smiapp/smiapp-core.c @@ -2741,8 +2741,6 @@ static const struct v4l2_subdev_internal_ops smiapp_internal_ops = { * I2C Driver */ -#ifdef CONFIG_PM - static int smiapp_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); @@ -2783,13 +2781,6 @@ static int smiapp_resume(struct device *dev) return rval; } -#else - -#define smiapp_suspend NULL -#define smiapp_resume NULL - -#endif /* CONFIG_PM */ - static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev) { struct smiapp_hwconfig *hwcfg; @@ -2913,13 +2904,9 @@ static int smiapp_probe(struct i2c_client *client, if (IS_ERR(sensor->xshutdown)) return PTR_ERR(sensor->xshutdown); - pm_runtime_enable(&client->dev); - - rval = pm_runtime_get_sync(&client->dev); - if (rval < 0) { - rval = -ENODEV; - goto out_power_off; - } + rval = smiapp_power_on(&client->dev); + if (rval < 0) + return rval; rval = smiapp_identify_module(sensor); if (rval) { @@ -3100,6 +3087,9 @@ static int smiapp_probe(struct i2c_client *client, if (rval < 0) goto out_media_entity_cleanup; + pm_runtime_set_active(&client->dev); + pm_runtime_get_noresume(&client->dev); + pm_runtime_enable(&client->dev); pm_runtime_set_autosuspend_delay(&client->dev, 1000); pm_runtime_use_autosuspend(&client->dev); pm_runtime_put_autosuspend(&client->dev); @@ -3113,8 +3103,7 @@ static int smiapp_probe(struct i2c_client *client, smiapp_cleanup(sensor); out_power_off: - pm_runtime_put(&client->dev); - pm_runtime_disable(&client->dev); + smiapp_power_off(&client->dev); return rval; } @@ -3127,8 +3116,9 @@ static int smiapp_remove(struct i2c_client *client) v4l2_async_unregister_subdev(subdev); - pm_runtime_suspend(&client->dev); pm_runtime_disable(&client->dev); + pm_runtime_set_suspended(&client->dev); + smiapp_power_off(&client->dev); for (i = 0; i < sensor->ssds_used; i++) { v4l2_device_unregister_subdev(&sensor->ssds[i].sd); -- 2.1.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2.1 1/2] smiapp: Implement power-on and power-off sequences without runtime PM 2016-11-29 17:28 ` [PATCH v2.1 " Sakari Ailus @ 2016-11-29 17:35 ` Laurent Pinchart 2016-11-30 11:29 ` Sakari Ailus 2016-11-30 16:11 ` [PATCH v2.2 " Sakari Ailus 1 sibling, 1 reply; 7+ messages in thread From: Laurent Pinchart @ 2016-11-29 17:35 UTC (permalink / raw) To: Sakari Ailus; +Cc: stern, linux-media, linux-pm Hi Sakari, Thank you for the patch. On Tuesday 29 Nov 2016 19:28:53 Sakari Ailus wrote: > Power on the sensor when the module is loaded and power it off when it is > removed. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > Hi Alan and Laurent, > > I hope this should be good then. I'm only enabling runtime PM at the end > of probe() when all is well, which reduces need for error handling. > > Regards, > Sakari > > drivers/media/i2c/smiapp/smiapp-core.c | 28 +++++++++------------------- > 1 file changed, 9 insertions(+), 19 deletions(-) > > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c > b/drivers/media/i2c/smiapp/smiapp-core.c index 59872b3..683a3e0 100644 > --- a/drivers/media/i2c/smiapp/smiapp-core.c > +++ b/drivers/media/i2c/smiapp/smiapp-core.c > @@ -2741,8 +2741,6 @@ static const struct v4l2_subdev_internal_ops > smiapp_internal_ops = { * I2C Driver > */ > > -#ifdef CONFIG_PM > - > static int smiapp_suspend(struct device *dev) > { > struct i2c_client *client = to_i2c_client(dev); > @@ -2783,13 +2781,6 @@ static int smiapp_resume(struct device *dev) > return rval; > } > > -#else > - > -#define smiapp_suspend NULL > -#define smiapp_resume NULL > - > -#endif /* CONFIG_PM */ > - > static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev) > { > struct smiapp_hwconfig *hwcfg; > @@ -2913,13 +2904,9 @@ static int smiapp_probe(struct i2c_client *client, > if (IS_ERR(sensor->xshutdown)) > return PTR_ERR(sensor->xshutdown); > > - pm_runtime_enable(&client->dev); > - > - rval = pm_runtime_get_sync(&client->dev); > - if (rval < 0) { > - rval = -ENODEV; > - goto out_power_off; > - } > + rval = smiapp_power_on(&client->dev); > + if (rval < 0) > + return rval; > > rval = smiapp_identify_module(sensor); > if (rval) { > @@ -3100,6 +3087,9 @@ static int smiapp_probe(struct i2c_client *client, > if (rval < 0) > goto out_media_entity_cleanup; > > + pm_runtime_set_active(&client->dev); > + pm_runtime_get_noresume(&client->dev); > + pm_runtime_enable(&client->dev); > pm_runtime_set_autosuspend_delay(&client->dev, 1000); > pm_runtime_use_autosuspend(&client->dev); > pm_runtime_put_autosuspend(&client->dev); This looks better to me, although these 6 lines really call for a new helper function. However, I still believe a helper that calls the runtime PM handlers directly when CONFIG_PM=n and rely on runtime PM when CONFIG_PM=y would be the cleanest solution from a driver point of view. > @@ -3113,8 +3103,7 @@ static int smiapp_probe(struct i2c_client *client, > smiapp_cleanup(sensor); > > out_power_off: > - pm_runtime_put(&client->dev); > - pm_runtime_disable(&client->dev); > + smiapp_power_off(&client->dev); > > return rval; > } > @@ -3127,8 +3116,9 @@ static int smiapp_remove(struct i2c_client *client) > > v4l2_async_unregister_subdev(subdev); > > - pm_runtime_suspend(&client->dev); > pm_runtime_disable(&client->dev); > + pm_runtime_set_suspended(&client->dev); > + smiapp_power_off(&client->dev); The device could be powered off already. > > for (i = 0; i < sensor->ssds_used; i++) { > v4l2_device_unregister_subdev(&sensor->ssds[i].sd); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2.1 1/2] smiapp: Implement power-on and power-off sequences without runtime PM 2016-11-29 17:35 ` Laurent Pinchart @ 2016-11-30 11:29 ` Sakari Ailus 0 siblings, 0 replies; 7+ messages in thread From: Sakari Ailus @ 2016-11-30 11:29 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Sakari Ailus, stern, linux-media, linux-pm Hi Laurent, On Tue, Nov 29, 2016 at 07:35:17PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Tuesday 29 Nov 2016 19:28:53 Sakari Ailus wrote: > > Power on the sensor when the module is loaded and power it off when it is > > removed. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > Hi Alan and Laurent, > > > > I hope this should be good then. I'm only enabling runtime PM at the end > > of probe() when all is well, which reduces need for error handling. > > > > Regards, > > Sakari > > > > drivers/media/i2c/smiapp/smiapp-core.c | 28 +++++++++------------------- > > 1 file changed, 9 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c > > b/drivers/media/i2c/smiapp/smiapp-core.c index 59872b3..683a3e0 100644 > > --- a/drivers/media/i2c/smiapp/smiapp-core.c > > +++ b/drivers/media/i2c/smiapp/smiapp-core.c > > @@ -2741,8 +2741,6 @@ static const struct v4l2_subdev_internal_ops > > smiapp_internal_ops = { * I2C Driver > > */ > > > > -#ifdef CONFIG_PM > > - > > static int smiapp_suspend(struct device *dev) > > { > > struct i2c_client *client = to_i2c_client(dev); > > @@ -2783,13 +2781,6 @@ static int smiapp_resume(struct device *dev) > > return rval; > > } > > > > -#else > > - > > -#define smiapp_suspend NULL > > -#define smiapp_resume NULL > > - > > -#endif /* CONFIG_PM */ > > - > > static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev) > > { > > struct smiapp_hwconfig *hwcfg; > > @@ -2913,13 +2904,9 @@ static int smiapp_probe(struct i2c_client *client, > > if (IS_ERR(sensor->xshutdown)) > > return PTR_ERR(sensor->xshutdown); > > > > - pm_runtime_enable(&client->dev); > > - > > - rval = pm_runtime_get_sync(&client->dev); > > - if (rval < 0) { > > - rval = -ENODEV; > > - goto out_power_off; > > - } > > + rval = smiapp_power_on(&client->dev); > > + if (rval < 0) > > + return rval; > > > > rval = smiapp_identify_module(sensor); > > if (rval) { > > @@ -3100,6 +3087,9 @@ static int smiapp_probe(struct i2c_client *client, > > if (rval < 0) > > goto out_media_entity_cleanup; > > > > + pm_runtime_set_active(&client->dev); > > + pm_runtime_get_noresume(&client->dev); > > + pm_runtime_enable(&client->dev); > > pm_runtime_set_autosuspend_delay(&client->dev, 1000); > > pm_runtime_use_autosuspend(&client->dev); > > pm_runtime_put_autosuspend(&client->dev); > > This looks better to me, although these 6 lines really call for a new helper > function. They're somewhat unrelated, especially the autosuspend part. Few drivers use it. > > However, I still believe a helper that calls the runtime PM handlers directly > when CONFIG_PM=n and rely on runtime PM when CONFIG_PM=y would be the cleanest > solution from a driver point of view. > > > @@ -3113,8 +3103,7 @@ static int smiapp_probe(struct i2c_client *client, > > smiapp_cleanup(sensor); > > > > out_power_off: > > - pm_runtime_put(&client->dev); > > - pm_runtime_disable(&client->dev); > > + smiapp_power_off(&client->dev); > > > > return rval; > > } > > @@ -3127,8 +3116,9 @@ static int smiapp_remove(struct i2c_client *client) > > > > v4l2_async_unregister_subdev(subdev); > > > > - pm_runtime_suspend(&client->dev); > > pm_runtime_disable(&client->dev); > > + pm_runtime_set_suspended(&client->dev); > > + smiapp_power_off(&client->dev); > > The device could be powered off already. Good point. The autosuspend must be explicitly disabled as well which wasn't previously done. I'll add another patch for that. -- Sakari Ailus e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2.2 1/2] smiapp: Implement power-on and power-off sequences without runtime PM 2016-11-29 17:28 ` [PATCH v2.1 " Sakari Ailus 2016-11-29 17:35 ` Laurent Pinchart @ 2016-11-30 16:11 ` Sakari Ailus 1 sibling, 0 replies; 7+ messages in thread From: Sakari Ailus @ 2016-11-30 16:11 UTC (permalink / raw) To: stern; +Cc: laurent.pinchart, linux-media, linux-pm Power on the sensor when the module is loaded and power it off when it is removed. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- since v2.1: - Power off conditionally, i.e. only if the device was powered. pm_runtime_status_suspended() returns false if CONFIG_PM is not set. drivers/media/i2c/smiapp/smiapp-core.c | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c index 59872b3..620f8ce 100644 --- a/drivers/media/i2c/smiapp/smiapp-core.c +++ b/drivers/media/i2c/smiapp/smiapp-core.c @@ -2741,8 +2741,6 @@ static const struct v4l2_subdev_internal_ops smiapp_internal_ops = { * I2C Driver */ -#ifdef CONFIG_PM - static int smiapp_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); @@ -2783,13 +2781,6 @@ static int smiapp_resume(struct device *dev) return rval; } -#else - -#define smiapp_suspend NULL -#define smiapp_resume NULL - -#endif /* CONFIG_PM */ - static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev) { struct smiapp_hwconfig *hwcfg; @@ -2913,13 +2904,9 @@ static int smiapp_probe(struct i2c_client *client, if (IS_ERR(sensor->xshutdown)) return PTR_ERR(sensor->xshutdown); - pm_runtime_enable(&client->dev); - - rval = pm_runtime_get_sync(&client->dev); - if (rval < 0) { - rval = -ENODEV; - goto out_power_off; - } + rval = smiapp_power_on(&client->dev); + if (rval < 0) + return rval; rval = smiapp_identify_module(sensor); if (rval) { @@ -3100,6 +3087,9 @@ static int smiapp_probe(struct i2c_client *client, if (rval < 0) goto out_media_entity_cleanup; + pm_runtime_set_active(&client->dev); + pm_runtime_get_noresume(&client->dev); + pm_runtime_enable(&client->dev); pm_runtime_set_autosuspend_delay(&client->dev, 1000); pm_runtime_use_autosuspend(&client->dev); pm_runtime_put_autosuspend(&client->dev); @@ -3113,8 +3103,7 @@ static int smiapp_probe(struct i2c_client *client, smiapp_cleanup(sensor); out_power_off: - pm_runtime_put(&client->dev); - pm_runtime_disable(&client->dev); + smiapp_power_off(&client->dev); return rval; } @@ -3127,8 +3116,10 @@ static int smiapp_remove(struct i2c_client *client) v4l2_async_unregister_subdev(subdev); - pm_runtime_suspend(&client->dev); pm_runtime_disable(&client->dev); + if (!pm_runtime_status_suspended(&client->dev)) + smiapp_power_off(&client->dev); + pm_runtime_set_suspended(&client->dev); for (i = 0; i < sensor->ssds_used; i++) { v4l2_device_unregister_subdev(&sensor->ssds[i].sd); -- 2.1.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] smiapp: Make suspend and resume functions __maybe_unused 2016-11-19 22:24 [PATCH v2 0/2] smiapp fix for w/o CONFIG_PM, compiler warnings Sakari Ailus 2016-11-19 22:24 ` [PATCH v2 1/2] smiapp: Implement power-on and power-off sequences without runtime PM Sakari Ailus @ 2016-11-19 22:24 ` Sakari Ailus 1 sibling, 0 replies; 7+ messages in thread From: Sakari Ailus @ 2016-11-19 22:24 UTC (permalink / raw) To: linux-media; +Cc: arnd The smiapp_suspend() and smiapp_resume() functions will end up being unused if CONFIG_PM is enabled but CONFIG_PM_SLEEP is disabled, causing a compiler warning from both of the function definitions. Fix this by marking the functions with __maybe_unused. Suggested-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/i2c/smiapp/smiapp-core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c index b20886f..ab48a85 100644 --- a/drivers/media/i2c/smiapp/smiapp-core.c +++ b/drivers/media/i2c/smiapp/smiapp-core.c @@ -2741,7 +2741,7 @@ static const struct v4l2_subdev_internal_ops smiapp_internal_ops = { * I2C Driver */ -static int smiapp_suspend(struct device *dev) +static int __maybe_unused smiapp_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); struct v4l2_subdev *subdev = i2c_get_clientdata(client); @@ -2766,7 +2766,7 @@ static int smiapp_suspend(struct device *dev) return 0; } -static int smiapp_resume(struct device *dev) +static int __maybe_unused smiapp_resume(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); struct v4l2_subdev *subdev = i2c_get_clientdata(client); -- 2.1.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-11-30 16:11 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-19 22:24 [PATCH v2 0/2] smiapp fix for w/o CONFIG_PM, compiler warnings Sakari Ailus 2016-11-19 22:24 ` [PATCH v2 1/2] smiapp: Implement power-on and power-off sequences without runtime PM Sakari Ailus 2016-11-29 17:28 ` [PATCH v2.1 " Sakari Ailus 2016-11-29 17:35 ` Laurent Pinchart 2016-11-30 11:29 ` Sakari Ailus 2016-11-30 16:11 ` [PATCH v2.2 " Sakari Ailus 2016-11-19 22:24 ` [PATCH v2 2/2] smiapp: Make suspend and resume functions __maybe_unused Sakari Ailus
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).