On 2/5/19 11:25 PM, Thierry Reding wrote: > On Tue, Feb 05, 2019 at 09:47:32PM +0100, Uwe Kleine-König wrote: >> Hello, >> >> On Tue, Feb 05, 2019 at 01:40:27PM +0100, Fabrice Gasnier wrote: >>> Add suspend/resume PM sleep ops. When going to low power, disable >>> active PWM channel. Active PWM channel is resumed, by calling >>> pwm_apply_state(). This is inspired by Thierry's comment in [1]. >>> Don't touch inactive channels, as it may be used by other LPTimer MFD >>> child driver. >>> [1]https://lkml.org/lkml/2017/12/5/175 >>> >>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxx> >>> --- >>> drivers/pwm/pwm-stm32-lp.c | 38 ++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 38 insertions(+) >>> >>> diff --git a/drivers/pwm/pwm-stm32-lp.c b/drivers/pwm/pwm-stm32-lp.c >>> index 0059b24c..0c40d48 100644 >>> --- a/drivers/pwm/pwm-stm32-lp.c >>> +++ b/drivers/pwm/pwm-stm32-lp.c >>> @@ -13,6 +13,7 @@ >>> #include <linux/mfd/stm32-lptimer.h> >>> #include <linux/module.h> >>> #include <linux/of.h> >>> +#include <linux/pinctrl/consumer.h> >>> #include <linux/platform_device.h> >>> #include <linux/pwm.h> >>> >>> @@ -20,6 +21,8 @@ struct stm32_pwm_lp { >>> struct pwm_chip chip; >>> struct clk *clk; >>> struct regmap *regmap; >>> + struct pwm_state suspend; >>> + bool suspended; >>> }; >>> >>> static inline struct stm32_pwm_lp *to_stm32_pwm_lp(struct pwm_chip *chip) >>> @@ -223,6 +226,40 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev) >>> return pwmchip_remove(&priv->chip); >>> } >>> >>> +#ifdef CONFIG_PM_SLEEP >>> +static int stm32_pwm_lp_suspend(struct device *dev) >>> +{ >>> + struct stm32_pwm_lp *priv = dev_get_drvdata(dev); >>> + >>> + pwm_get_state(&priv->chip.pwms[0], &priv->suspend); >>> + priv->suspended = priv->suspend.enabled; >>> + >>> + /* safe to call pwm_disable() for already disabled pwm */ >>> + pwm_disable(&priv->chip.pwms[0]); >>> + >>> + return pinctrl_pm_select_sleep_state(dev); >> >> IMHO a PWM should not stop if the PWM user didn't call pwm_disable() (or >> pwm_apply_state() with .enabled = false). >> >> I don't understand all the PM details, but I think there is no defined >> order in which devices are signalled to suspend. If so the PWM might be >> stopped before its consumer. Then the PWM changes state without the >> consumer being aware of that. >> >> I understand Thierry's position in the link you provided in the commit >> log consistant with my position here. > > Agreed, we should let users of the PWM take care of resuming the PWM in > the state and at the point in time that they require so. PWM users will > also likely do a pwm_disable() during their suspend implementation, so > doing this again in a PWM ->suspend() is not necessary, even if perhaps > harmless. > > So this leaves only the pinctrl_pm_select_sleep_state() call. That seems > fine, but I'm not sure that that's currently guaranteed to work. I think > we may need to implement device link support in the PWM framework to > guarantee the proper suspend/resume sequencing. Otherwise you may end up > in a situation where the PWM is actually suspended before the user and > glitch the pins before the user has a chance to disable the PWM. Hi Uwe, Thierry, I agree with both of you on the analysis. > > I think it'd be fine to merge the driver change here first before we add > device link support if this works for you. Just mentioning the issue > here in case you ever run into it. If you agree with the current approach, I can send a V2 with Tomasz's suggestion to remove the ifdefs and use __maybe_unused instead. Thanks for reviewing, Best Regards, Fabrice > > Thierry >
