Re: [PATCH v7 3/8] pwm: pca9685: Improve runtime PM behavior

[Clemens Gruber]

On Fri, Apr 09, 2021 at 03:03:20PM +0200, Thierry Reding wrote:
> On Tue, Apr 06, 2021 at 06:41:35PM +0200, Clemens Gruber wrote:
> > The chip does not come out of POR in active state but in sleep state.
> > To be sure (in case the bootloader woke it up) we force it to sleep in
> > probe.
> > 
> > On kernels without CONFIG_PM, we wake the chip in .probe and put it to
> > sleep in .remove.
> > 
> > Signed-off-by: Clemens Gruber <clemens.gruber@xxxxxxxxxxxx>
> > ---
> > Changes since v6:
> > - Improved !CONFIG_PM handling (wake it up without putting it to sleep
> >   first)
> > 
> >  drivers/pwm/pwm-pca9685.c | 26 +++++++++++++++++++-------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index d4474c5ff96f..0bcec04b138a 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -474,13 +474,18 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> >  		return ret;
> >  	}
> >  
> > -	/* The chip comes out of power-up in the active state */
> > -	pm_runtime_set_active(&client->dev);
> > -	/*
> > -	 * Enable will put the chip into suspend, which is what we
> > -	 * want as all outputs are disabled at this point
> > -	 */
> > -	pm_runtime_enable(&client->dev);
> > +	if (IS_ENABLED(CONFIG_PM)) {
> 
> This looks odd to me. I've seen similar constructs, but they usually go
> something like this (I think):
> 
> 	pm_runtime_enable(&client->dev);
> 
> 	if (!pm_runtime_enabled(&client->dev)) {
> 		/* resume device */
> 	}
> 
> Which I guess in your would be somewhat the opposite and it wouldn't
> actually resume the device but rather put it to sleep.

Yes, I wanted to keep it in sleep mode if runtime PM is supported (to be
woken up later) and otherwise just wake it up in probe.

> 
> Perhaps something like this:
> 
> 	pm_runtime_enable(&client->dev);
> 
> 	if (pm_runtime_enabled(&client->dev)) {
> 		pca9685_set_sleep_mode(pca, true);
> 		pm_runtime_set_suspended(&client->dev);
> 	} else {
> 		/* wake the chip up on non-PM environments */
> 		pca9685_set_sleep_mode(pca, false);
> 	}
> 
> ? I think that's slightly more correct than your original because it
> takes into account things like sysfs power control and such. It also
> doesn't rely on the config option alone but instead uses the runtime
> PM API to achieve this more transparently.

Ah, yes, I missed the fact that runtime could be disabled 'at runtime'
via sysfs as well, so yes, that's more correct and pm_runtime_enabled
will just return false if !CONFIG_PM, so that should work as well.

Thanks,
Clemens