Re: [PATCH 1/3] pwm: pca9685: Switch to atomic API

[Clemens Gruber]

On Thu, Nov 19, 2020 at 12:29:26PM -0500, Sven Van Asbroeck wrote:
> On Thu, Nov 19, 2020 at 11:00 AM Clemens Gruber
> <clemens.gruber@xxxxxxxxxxxx> wrote:
> >
> > One thing I noticed: The driver currently assumes that it comes out of
> > POR in "active" state (comment at end of probe and PM calls).
> > However, the SLEEP bit is set by default / after POR.
> 
> Very good point, the comment is probably not correct.
> 
> It would be wrong to assume that the chip is in any particular
> state when probe() runs. There is no reset pin, so the CPU running
> Linux could have reset while manipulating the chip, there could even
> be leftover state from a bootloader talking to the chip, etc...
> 
> I remember running into this myself at the time.
> 
> However, we want to make sure that the runtime pm puts the chip to sleep,
> no matter its initial state. So the current code is correct, but the
> comment can be changed to:
> 
> /*
>  * Chip activity state unknown. Tell the runtime pm that the chip is
>  * active, so pm_runtime_enable() will force it into suspend.
>  * Which is what we want as all outputs are disabled at this point.
>  */

I think it is better if we set the correct runtime pm state in .probe,
depending on the SLEEP bit being set. Then, if the chip is already in
SLEEP state, there is no need for the suspend function to be called.

> > 2) If !CONFIG_PM: Clear the SLEEP bit in .probe
> 
> Is anyone likely to use this driver without CONFIG_PM? My kernel won't even
> build without it...
> 
> If you personally have no use for it, then I would not bother with the
> !CONFIG_PM case. Just formalize in Kconfig that the driver needs PM.
> 
> I think we can add "depends on PM" or "select PM", I am not sure which one
> to use here.

I'd rather continue supporting this driver with !CONFIG_PM. (In our
company we have a product with a !CONFIG_PM build using this driver)

I am thinking about the following solution:
#ifdef CONFIG_PM
  /* Set runtime PM status according to chip sleep state */
  if (reg & MODE1_SLEEP)
    pm_runtime_set_suspended(..);
  else
    pm_runtime_set_active(..);

  pm_runtime_enable(..);
#else
  /* If in SLEEP state on non-PM environments, wake the chip up */
  if (reg & MODE1_SLEEP)
    pca9685_set_sleep_mode(.., false)
#endif

--

About the regmap cache: I looked into it and think it is a good idea but
it's probably best to get these patches merged first and then rework the
driver to using the regmap cache?

Thanks for your help!

Clemens