| Message ID | 1479477016-28450-1-git-send-email-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday, November 18, 2016 3:50:16 PM CET 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 Arnd and others, > > The patch is tested with CONFIG_PM set, as the system does I was testing > on did not boot with CONFIG_PM disabled. I'm not really too worried about > this though, the patch is very simple. > > static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev) > { > struct smiapp_hwconfig *hwcfg; > @@ -2915,7 +2906,11 @@ static int smiapp_probe(struct i2c_client *client, > > pm_runtime_enable(&client->dev); > > +#ifdef CONFIG_PM > rval = pm_runtime_get_sync(&client->dev); > +#else > + rval = smiapp_power_on(&client->dev); > +#endif > if (rval < 0) { > rval = -ENODEV; > goto out_power_off; I would suggest writing this as if (IS_ENABLED(CONFIG_PM)) rval = pm_runtime_get_sync(&client->dev); else rval = smiapp_power_on(&client->dev); though that is a purely cosmetic change. I think you are missing one other warning: with CONFIG_PM=y and CONFIG_PM_SLEEP=n, the smiapp_suspend/smiapp_resume functions are now unused and need to be marked as __maybe_unused. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, On Friday 18 Nov 2016 17:09:01 Arnd Bergmann wrote: > On Friday, November 18, 2016 3:50:16 PM CET 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 Arnd and others, > > > > The patch is tested with CONFIG_PM set, as the system does I was testing > > on did not boot with CONFIG_PM disabled. I'm not really too worried about > > this though, the patch is very simple. > > > > static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev) > > { > > struct smiapp_hwconfig *hwcfg; > > @@ -2915,7 +2906,11 @@ static int smiapp_probe(struct i2c_client *client, > > > > pm_runtime_enable(&client->dev); > > > > +#ifdef CONFIG_PM > > rval = pm_runtime_get_sync(&client->dev); > > +#else > > + rval = smiapp_power_on(&client->dev); > > +#endif > > > > if (rval < 0) { > > rval = -ENODEV; > > goto out_power_off; > > I would suggest writing this as > > if (IS_ENABLED(CONFIG_PM)) > rval = pm_runtime_get_sync(&client->dev); > else > rval = smiapp_power_on(&client->dev); > > though that is a purely cosmetic change. Are all drivers really supposed to code this kind of construct ? Shouldn't this be handled in the PM core ? A very naive approach would be to call .runtime_resume() and .runtime_suspend() from the non-CONFIG_PM versions of pm_runtime_enable() and pm_runtime_disable() respectively. I assume that would break things, but can't we implement something similar to that that wouldn't require all drivers to open-code it ? > I think you are missing one other warning: with CONFIG_PM=y and > CONFIG_PM_SLEEP=n, the smiapp_suspend/smiapp_resume functions > are now unused and need to be marked as __maybe_unused.
On Tuesday, November 22, 2016 8:31:42 PM CET Laurent Pinchart wrote: > > > @@ -2915,7 +2906,11 @@ static int smiapp_probe(struct i2c_client *client, > > > > > > pm_runtime_enable(&client->dev); > > > > > > +#ifdef CONFIG_PM > > > rval = pm_runtime_get_sync(&client->dev); > > > +#else > > > + rval = smiapp_power_on(&client->dev); > > > +#endif > > > > > > if (rval < 0) { > > > rval = -ENODEV; > > > goto out_power_off; > > > > I would suggest writing this as > > > > if (IS_ENABLED(CONFIG_PM)) > > rval = pm_runtime_get_sync(&client->dev); > > else > > rval = smiapp_power_on(&client->dev); > > > > though that is a purely cosmetic change. > > Are all drivers really supposed to code this kind of construct ? Shouldn't > this be handled in the PM core ? A very naive approach would be to call > .runtime_resume() and .runtime_suspend() from the non-CONFIG_PM versions of > pm_runtime_enable() and pm_runtime_disable() respectively. I assume that would > break things, but can't we implement something similar to that that wouldn't > require all drivers to open-code it ? I know nothing about the details of how the suspend/resume code should do this, I was just commenting on the syntax above, preferring an IS_ENABLED() check over an #ifdef. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, (CC'ing the linux-pm mailing list) On Tuesday 22 Nov 2016 21:58:32 Arnd Bergmann wrote: > On Tuesday, November 22, 2016 8:31:42 PM CET Laurent Pinchart wrote: > >>> @@ -2915,7 +2906,11 @@ static int smiapp_probe(struct i2c_client > >>> *client, > >>> > >>> pm_runtime_enable(&client->dev); > >>> > >>> +#ifdef CONFIG_PM > >>> rval = pm_runtime_get_sync(&client->dev); > >>> +#else > >>> + rval = smiapp_power_on(&client->dev); > >>> +#endif > >>> if (rval < 0) { > >>> rval = -ENODEV; > >>> goto out_power_off; > >> > >> I would suggest writing this as > >> > >> if (IS_ENABLED(CONFIG_PM)) > >> rval = pm_runtime_get_sync(&client->dev); > >> else > >> rval = smiapp_power_on(&client->dev); > >> > >> though that is a purely cosmetic change. > > > > Are all drivers really supposed to code this kind of construct ? Shouldn't > > this be handled in the PM core ? A very naive approach would be to call > > .runtime_resume() and .runtime_suspend() from the non-CONFIG_PM versions > > of pm_runtime_enable() and pm_runtime_disable() respectively. I assume > > that would break things, but can't we implement something similar to that > > that wouldn't require all drivers to open-code it ? > > I know nothing about the details of how the suspend/resume code should > do this, I was just commenting on the syntax above, preferring an > IS_ENABLED() check over an #ifdef. Dear linux-pm developers, what's the suggested way to ensure that a runtime- pm-enabled driver can run fine on a system with CONFIG_PM disabled ?
On Fri, 25 Nov 2016, Laurent Pinchart wrote: > Dear linux-pm developers, what's the suggested way to ensure that a runtime- > pm-enabled driver can run fine on a system with CONFIG_PM disabled ? The exact point of your question isn't entirely clear. In the most literal sense, the best ways to ensure this are (1) audit the code, and (2) actually try it. I have a feeling this doesn't quite answer your question, however. :-) Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan and others, On Thu, Nov 24, 2016 at 09:15:39PM -0500, Alan Stern wrote: > On Fri, 25 Nov 2016, Laurent Pinchart wrote: > > > Dear linux-pm developers, what's the suggested way to ensure that a runtime- > > pm-enabled driver can run fine on a system with CONFIG_PM disabled ? > > The exact point of your question isn't entirely clear. In the most > literal sense, the best ways to ensure this are (1) audit the code, and > (2) actually try it. > > I have a feeling this doesn't quite answer your question, however. :-) The question is related to devices that require certain power-up and power-down sequences that are now implemented as PM runtime hooks that, without CONFIG_PM defined, will not be executed. Is there a better way than to handle this than have an implementation in the driver for the PM runtime and non-PM runtime case separately?
On Fri, 25 Nov 2016, Sakari Ailus wrote: > Hi Alan and others, > > On Thu, Nov 24, 2016 at 09:15:39PM -0500, Alan Stern wrote: > > On Fri, 25 Nov 2016, Laurent Pinchart wrote: > > > > > Dear linux-pm developers, what's the suggested way to ensure that a runtime- > > > pm-enabled driver can run fine on a system with CONFIG_PM disabled ? > > > > The exact point of your question isn't entirely clear. In the most > > literal sense, the best ways to ensure this are (1) audit the code, and > > (2) actually try it. > > > > I have a feeling this doesn't quite answer your question, however. :-) > > The question is related to devices that require certain power-up and > power-down sequences that are now implemented as PM runtime hooks that, > without CONFIG_PM defined, will not be executed. Is there a better way than > to handle this than have an implementation in the driver for the PM runtime > and non-PM runtime case separately? Yes, there is a better way. For the initial power-up and final power-down sequences, don't rely on the PM core to invoke the callbacks. Just call them directly, yourself. For example, as part of the probe routine, instead of doing this: pm_runtime_set_suspended(dev); pm_runtime_enable(dev); pm_runtime_get_sync(dev); Do this: pm_runtime_set_active(dev); pm_runtime_get_noresume(dev); pm_runtime_enable(dev); /* * In case CONFIG_PM is disabled, invoke the runtime-resume * callback directly. */ my_runtime_resume(dev); Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan, On Friday 25 Nov 2016 10:21:21 Alan Stern wrote: > On Fri, 25 Nov 2016, Sakari Ailus wrote: > > On Thu, Nov 24, 2016 at 09:15:39PM -0500, Alan Stern wrote: > >> On Fri, 25 Nov 2016, Laurent Pinchart wrote: > >>> Dear linux-pm developers, what's the suggested way to ensure that a > >>> runtime- pm-enabled driver can run fine on a system with CONFIG_PM > >>> disabled ? > >> > >> The exact point of your question isn't entirely clear. In the most > >> literal sense, the best ways to ensure this are (1) audit the code, and > >> (2) actually try it. > >> > >> I have a feeling this doesn't quite answer your question, however. :-) > > > > The question is related to devices that require certain power-up and > > power-down sequences that are now implemented as PM runtime hooks that, > > without CONFIG_PM defined, will not be executed. Is there a better way > > than to handle this than have an implementation in the driver for the PM > > runtime and non-PM runtime case separately? > > Yes, there is a better way. For the initial power-up and final > power-down sequences, don't rely on the PM core to invoke the > callbacks. Just call them directly, yourself. > > For example, as part of the probe routine, instead of doing this: > > pm_runtime_set_suspended(dev); > pm_runtime_enable(dev); > pm_runtime_get_sync(dev); > > Do this: > > pm_runtime_set_active(dev); > pm_runtime_get_noresume(dev); > pm_runtime_enable(dev); > /* > * In case CONFIG_PM is disabled, invoke the runtime-resume > * callback directly. > */ > my_runtime_resume(dev); Wouldn't it be cleaner for drivers not to have to handle this manually (which gives an opportunity to get it wrong) but instead have pm_runtime_enable() call the runtime resume callback when CONFIG_PM is disabled ?
On Fri, 25 Nov 2016, Laurent Pinchart wrote: > Hi Alan, Hello. > On Friday 25 Nov 2016 10:21:21 Alan Stern wrote: > > On Fri, 25 Nov 2016, Sakari Ailus wrote: > > > On Thu, Nov 24, 2016 at 09:15:39PM -0500, Alan Stern wrote: > > >> On Fri, 25 Nov 2016, Laurent Pinchart wrote: > > >>> Dear linux-pm developers, what's the suggested way to ensure that a > > >>> runtime- pm-enabled driver can run fine on a system with CONFIG_PM > > >>> disabled ? > > >> > > >> The exact point of your question isn't entirely clear. In the most > > >> literal sense, the best ways to ensure this are (1) audit the code, and > > >> (2) actually try it. > > >> > > >> I have a feeling this doesn't quite answer your question, however. :-) > > > > > > The question is related to devices that require certain power-up and > > > power-down sequences that are now implemented as PM runtime hooks that, > > > without CONFIG_PM defined, will not be executed. Is there a better way > > > than to handle this than have an implementation in the driver for the PM > > > runtime and non-PM runtime case separately? > > > > Yes, there is a better way. For the initial power-up and final > > power-down sequences, don't rely on the PM core to invoke the > > callbacks. Just call them directly, yourself. > > > > For example, as part of the probe routine, instead of doing this: > > > > pm_runtime_set_suspended(dev); > > pm_runtime_enable(dev); > > pm_runtime_get_sync(dev); > > > > Do this: > > > > pm_runtime_set_active(dev); > > pm_runtime_get_noresume(dev); > > pm_runtime_enable(dev); > > /* > > * In case CONFIG_PM is disabled, invoke the runtime-resume > > * callback directly. > > */ > > my_runtime_resume(dev); > > Wouldn't it be cleaner for drivers not to have to handle this manually (which > gives an opportunity to get it wrong) but instead have pm_runtime_enable() > call the runtime resume callback when CONFIG_PM is disabled ? Well, I admit it would be nicer if drivers didn't have to worry about whether or not CONFIG_PM was enabled. A slightly cleaner approach from the one outlined above would have the probe routine do this: my_power_up(dev); pm_runtime_set_active(dev); pm_runtime_get_noresume(dev); pm_runtime_enable(dev); and have the runtime-resume callback routine call my_power_up() to do its work. (Or make my_power_up() actually be the runtime-resume callback routine.) That's pretty straightforward and hard to mess up. In theory, we could have pm_runtime_enable() invoke the runtime-resume callback when CONFIG_PM is disabled. In practice, it would be rather awkward. drivers/base/power/runtime.c, which is where pm_runtime_enable() is defined and the runtime-PM callbacks are invoked, doesn't even get compiled if CONFIG_PM is off. (Also, it would run against the grain. CONFIG_PM=n means the kernel ignores runtime PM, so pm_runtime_enable() shouldn't do anything.) There's a corollary aspect to this. If you depend on runtime PM for powering up your device during probe, does that mean you also depend on runtime PM for powering down the device during remove? That is likely not to work, because the user can prevent runtime suspends by writing to /sys/.../power/control. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan, On Saturday 26 Nov 2016 15:10:28 Alan Stern wrote: > On Fri, 25 Nov 2016, Laurent Pinchart wrote: > > On Friday 25 Nov 2016 10:21:21 Alan Stern wrote: > >> On Fri, 25 Nov 2016, Sakari Ailus wrote: > >>> On Thu, Nov 24, 2016 at 09:15:39PM -0500, Alan Stern wrote: > >>>> On Fri, 25 Nov 2016, Laurent Pinchart wrote: > >>>>> Dear linux-pm developers, what's the suggested way to ensure that a > >>>>> runtime- pm-enabled driver can run fine on a system with CONFIG_PM > >>>>> disabled ? > >>>> > >>>> The exact point of your question isn't entirely clear. In the most > >>>> literal sense, the best ways to ensure this are (1) audit the code, > >>>> and (2) actually try it. > >>>> > >>>> I have a feeling this doesn't quite answer your question, however. > >>>> :-) > >>> > >>> The question is related to devices that require certain power-up and > >>> power-down sequences that are now implemented as PM runtime hooks > >>> that, without CONFIG_PM defined, will not be executed. Is there a > >>> better way than to handle this than have an implementation in the > >>> driver for the PM runtime and non-PM runtime case separately? > >> > >> Yes, there is a better way. For the initial power-up and final > >> power-down sequences, don't rely on the PM core to invoke the > >> callbacks. Just call them directly, yourself. > >> > >> For example, as part of the probe routine, instead of doing this: > >> pm_runtime_set_suspended(dev); > >> pm_runtime_enable(dev); > >> pm_runtime_get_sync(dev); > >> > >> Do this: > >> pm_runtime_set_active(dev); > >> pm_runtime_get_noresume(dev); > >> pm_runtime_enable(dev); > >> /* > >> * In case CONFIG_PM is disabled, invoke the runtime-resume > >> * callback directly. > >> */ > >> my_runtime_resume(dev); > > > > Wouldn't it be cleaner for drivers not to have to handle this manually > > (which gives an opportunity to get it wrong) but instead have > > pm_runtime_enable() call the runtime resume callback when CONFIG_PM is > > disabled ? > > Well, I admit it would be nicer if drivers didn't have to worry about > whether or not CONFIG_PM was enabled. A slightly cleaner approach > from the one outlined above would have the probe routine do this: > > my_power_up(dev); > pm_runtime_set_active(dev); > pm_runtime_get_noresume(dev); > pm_runtime_enable(dev); > > and have the runtime-resume callback routine call my_power_up() to do > its work. (Or make my_power_up() actually be the runtime-resume > callback routine.) That's pretty straightforward and hard to mess up. You'd be surprised how easy drivers can mess simple things up ;-) We'd still have to get the message out there, that would be the most difficult part. > In theory, we could have pm_runtime_enable() invoke the runtime-resume > callback when CONFIG_PM is disabled. In practice, it would be rather > awkward. drivers/base/power/runtime.c, which is where > pm_runtime_enable() is defined and the runtime-PM callbacks are > invoked, doesn't even get compiled if CONFIG_PM is off. Sure, but that can easily be fixed. > (Also, it would run against the grain. CONFIG_PM=n means the kernel > ignores runtime PM, so pm_runtime_enable() shouldn't do anything.) I'd argue that CONFIG_PM=n should mean that the runtime PM API doesn't perform runtime PM, not that it should do absolutely nothing. If semantics is the biggest concern, we could introduce a helper (whose name is TBD) that would enable runtime PM when CONFIG_PM=y or power on the device when CONFIG_PM=n I want to make it as easy as possible for drivers to make sure they won't get this wrong, which in my opinion requires a simple and straightforward API with no code in the driver that would depend on the value of CONFIG_PM. > There's a corollary aspect to this. If you depend on runtime PM for > powering up your device during probe, does that mean you also depend on > runtime PM for powering down the device during remove? That is likely > not to work, because the user can prevent runtime suspends by writing > to /sys/.../power/control. Yes, I do, and I expect most runtime PM-enabled driver to do the same. When runtime suspend is disabled through /sys/.../power/control does pm_runtime_disable() invoke the runtime PM suspend handler if the device is powered on ?
On Mon, 28 Nov 2016, Laurent Pinchart wrote: > > Well, I admit it would be nicer if drivers didn't have to worry about > > whether or not CONFIG_PM was enabled. A slightly cleaner approach > > from the one outlined above would have the probe routine do this: > > > > my_power_up(dev); > > pm_runtime_set_active(dev); > > pm_runtime_get_noresume(dev); > > pm_runtime_enable(dev); > > > > and have the runtime-resume callback routine call my_power_up() to do > > its work. (Or make my_power_up() actually be the runtime-resume > > callback routine.) That's pretty straightforward and hard to mess up. > > You'd be surprised how easy drivers can mess simple things up ;-) No -- I wouldn't! :-) > We'd still > have to get the message out there, that would be the most difficult part. Agreed. > > In theory, we could have pm_runtime_enable() invoke the runtime-resume > > callback when CONFIG_PM is disabled. In practice, it would be rather > > awkward. drivers/base/power/runtime.c, which is where > > pm_runtime_enable() is defined and the runtime-PM callbacks are > > invoked, doesn't even get compiled if CONFIG_PM is off. > > Sure, but that can easily be fixed. > > > (Also, it would run against the grain. CONFIG_PM=n means the kernel > > ignores runtime PM, so pm_runtime_enable() shouldn't do anything.) > > I'd argue that CONFIG_PM=n should mean that the runtime PM API doesn't perform > runtime PM, not that it should do absolutely nothing. If semantics is the > biggest concern, we could introduce a helper (whose name is TBD) that would > enable runtime PM when CONFIG_PM=y or power on the device when CONFIG_PM=n Or have the driver call _both_ the helper routine and pm_runtime_enable() -- the helper would do nothing if CONFIG_PM=y, and it would invoke the runtime-resume callback if CONFIG_PM=n. Either way would be a good approach. Having pm_runtime_enable() call the runtime-resume handler wouldn't work well if the driver has already powered-up the device or the device starts out in the power-on state (which is often the case). > I want to make it as easy as possible for drivers to make sure they won't get > this wrong, which in my opinion requires a simple and straightforward API with > no code in the driver that would depend on the value of CONFIG_PM. Well, the approach I outlined above is pretty simple and it doesn't depend on the value of CONFIG_PM. Your proposal is just as simple, but it does require drivers to remember to call the new helper routine. > > There's a corollary aspect to this. If you depend on runtime PM for > > powering up your device during probe, does that mean you also depend on > > runtime PM for powering down the device during remove? That is likely > > not to work, because the user can prevent runtime suspends by writing > > to /sys/.../power/control. > > Yes, I do, and I expect most runtime PM-enabled driver to do the same. When > runtime suspend is disabled through /sys/.../power/control does > pm_runtime_disable() invoke the runtime PM suspend handler if the device is > powered on ? No, it doesn't, and neither does pm_runtime_put(). After all, if the user has told the system not to do runtime PM on that device, it doesn't make sense to call the runtime-suspend handler. But you can always blame the user when this happens. :-) Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c index 59872b3..8624dc4 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,11 @@ static int smiapp_probe(struct i2c_client *client, pm_runtime_enable(&client->dev); +#ifdef CONFIG_PM rval = pm_runtime_get_sync(&client->dev); +#else + rval = smiapp_power_on(&client->dev); +#endif if (rval < 0) { rval = -ENODEV; goto out_power_off; @@ -3113,7 +3108,11 @@ static int smiapp_probe(struct i2c_client *client, smiapp_cleanup(sensor); out_power_off: +#ifdef CONFIG_PM pm_runtime_put(&client->dev); +#else + smiapp_power_off(&client->dev); +#endif pm_runtime_disable(&client->dev); return rval; @@ -3127,7 +3126,11 @@ static int smiapp_remove(struct i2c_client *client) v4l2_async_unregister_subdev(subdev); +#ifdef CONFIG_PM pm_runtime_suspend(&client->dev); +#else + smiapp_power_off(&client->dev); +#endif pm_runtime_disable(&client->dev); for (i = 0; i < sensor->ssds_used; i++) {
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 Arnd and others, The patch is tested with CONFIG_PM set, as the system does I was testing on did not boot with CONFIG_PM disabled. I'm not really too worried about this though, the patch is very simple. Kind regards, Sakari drivers/media/i2c/smiapp/smiapp-core.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)