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

From: Sven Van Asbroeck
Date: Sat Nov 21 2020 - 17:00:46 EST


On Sat, Nov 21, 2020 at 9:58 AM Clemens Gruber
<clemens.gruber@xxxxxxxxxxxx> wrote:
>
> 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)

Absolutely, makes sense. If you do add support for !CONFIG_PM, then it's
important that both PM and !PM cases get tested by you.

>
> 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

I don't think we need the #ifdef CONFIG_PM, because all pm_runtime_xxx
functions become no-ops when !CONFIG_PM.

Also, I believe "if (IS_ENABLED(CONFIG_XXX))" is preferred, because
it allows the compiler to syntax-check disabled code.

How about the following? It should be correct, short, and easy to understand.
Yes, there's one single unnecessary register write (+ 500us delay if !PM) when
the chip is already active on probe(). But maybe that's worth it if it makes
the code easier to understand?

probe()
{
...
pm_runtime_set_active(&client->dev);
pm_runtime_enable(&client->dev);

if (!IS_ENABLED(CONFIG_PM))
pca9685_set_sleep_mode(pca, false);

return 0;
}

remove()
{
...
pm_runtime_disable(&client->dev);

if (!IS_ENABLED(CONFIG_PM))
pca9685_set_sleep_mode(pca, true);

return 0;
}

>
> 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?

Good suggestion, I agree.