Messages in this thread | | | From | Sven Van Asbroeck <> | Date | Sat, 21 Nov 2020 17:00:10 -0500 | Subject | Re: [PATCH 1/3] pwm: pca9685: Switch to atomic API |
| |
On Sat, Nov 21, 2020 at 9:58 AM Clemens Gruber <clemens.gruber@pqgruber.com> 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.
| |