Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

-- 
Regards,

Laurent Pinchart