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
