Re: [PATCH v6 3/7] pwm: pca9685: Improve runtime PM behavior

[Clemens Gruber]

Hi Uwe,

On Mon, Mar 29, 2021 at 05:55:27PM +0200, Uwe Kleine-König wrote:
> On Mon, Mar 29, 2021 at 02:57:03PM +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.
> 
> What is the effect of sleep state? Does it continue to oscilate it the
> bootloader set up some configuration?

The datasheet says: "When the oscillator is off (Sleep mode) the LEDn
outputs cannot be turned on, off or dimmed/blinked."

At the moment, we reset the output registers anyway, so everything is
turned off at probe time, even if the bootloader did set something up.

When removing the resets in the future, I would read out the state of
the SLEEP bit at probe time and set the pm runtime state accordingly.

> 
> 
> > Signed-off-by: Clemens Gruber <clemens.gruber@xxxxxxxxxxxx>
> > ---
> >  drivers/pwm/pwm-pca9685.c | 20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index fb026a25fb61..4d6684b90819 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -469,14 +469,19 @@ 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
> > +	 * The chip comes out of power-up in the sleep state,
> > +	 * but force it to sleep in case it was woken up before
> >  	 */
> > +	pca9685_set_sleep_mode(pca, true);
> > +	pm_runtime_set_suspended(&client->dev);
> >  	pm_runtime_enable(&client->dev);
> >  
> > +	if (!IS_ENABLED(CONFIG_PM)) {
> > +		/* Wake the chip up on non-PM environments */
> > +		pca9685_set_sleep_mode(pca, false);
> 
> I admit I didn't grasp all the PM framework details, but I wonder if
> it's right to first call set_sleep_mode(true) and then in some cases to
> false again.

That was done for readability reasons, however, I admit that after we no
longer reset the period (deemed not necessary by me due to the planned
removal of the resets) it would probably be as readable to have:

if (IS_ENABLED(CONFIG_PM)) {
	pca9685_set_sleep_mode(pca, true);
	pm_runtime_set_suspended..
	pm_runtime_enable..
} else
	pca9685_set_sleep_mode(pca, false);

Thanks,
Clemens