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

[Sven Van Asbroeck]

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.