| Message ID | 1549370429-19116-1-git-send-email-fabrice.gasnier@st.com |
|---|---|
| Headers | show |
| Series | Add PM support to STM32 LP Timer drivers | expand |
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@st.com> > --- > 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 You might consider dropping ifdefs and marking pm functions with __maybe_unused instead. In case CONFIG_PM_SLEEP=n then these two guys will be removed and pm ops structure will be empty. > +static int stm32_pwm_lp_suspend(struct device *dev) > +{ > + struct stm32_pwm_lp *priv = dev_get_drvdata(dev); > + I guess you first need to get platform_device from dev and eventually stm32_pwm_lp. Wondering how this works now. > + 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); > +} > + > +static int stm32_pwm_lp_resume(struct device *dev) > +{ > + struct stm32_pwm_lp *priv = dev_get_drvdata(dev); > + int ret; > + > + ret = pinctrl_pm_select_default_state(dev); > + if (ret) > + return ret; > + > + /* Only restore suspended pwm, not to disrupt other MFD child */ > + if (!priv->suspended) > + return 0; Would it make sense to use suspend.enabled directly? > + > + return pwm_apply_state(&priv->chip.pwms[0], &priv->suspend); > +} > +#endif > + > +static SIMPLE_DEV_PM_OPS(stm32_pwm_lp_pm_ops, stm32_pwm_lp_suspend, > + stm32_pwm_lp_resume); > + > static const struct of_device_id stm32_pwm_lp_of_match[] = { > { .compatible = "st,stm32-pwm-lp", }, > {}, > @@ -235,6 +272,7 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev) > .driver = { > .name = "stm32-pwm-lp", > .of_match_table = of_match_ptr(stm32_pwm_lp_of_match), > + .pm = &stm32_pwm_lp_pm_ops, > }, > }; > module_platform_driver(stm32_pwm_lp_driver); > -- > 1.9.1 >
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@st.com> > --- > 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. Best regards Uwe
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@st.com> > > --- > > 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. 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. Thierry
On 2/5/19 7:30 PM, Tomasz Duszynski wrote: > 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@st.com> >> --- >> 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 > > You might consider dropping ifdefs and marking pm functions with > __maybe_unused instead. In case CONFIG_PM_SLEEP=n then these two guys > will be removed and pm ops structure will be empty. Hi Tomasz, Thanks for this suggestion. I can do this change. I'll wait for more feedback from Uwe and Thierry before sending a v2 with that. > >> +static int stm32_pwm_lp_suspend(struct device *dev) >> +{ >> + struct stm32_pwm_lp *priv = dev_get_drvdata(dev); >> + > > I guess you first need to get platform_device from dev and eventually > stm32_pwm_lp. Wondering how this works now. This should be safe for now. This works because the probe routine calls: platform_set_drvdata(pdev, priv); And the underlying call is dev_set_drvdata() static inline void platform_set_drvdata(struct platform_device *pdev, void *data) { dev_set_drvdata(&pdev->dev, data); } > >> + 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); >> +} >> + >> +static int stm32_pwm_lp_resume(struct device *dev) >> +{ >> + struct stm32_pwm_lp *priv = dev_get_drvdata(dev); >> + int ret; >> + >> + ret = pinctrl_pm_select_default_state(dev); >> + if (ret) >> + return ret; >> + >> + /* Only restore suspended pwm, not to disrupt other MFD child */ >> + if (!priv->suspended) >> + return 0; > > Would it make sense to use suspend.enabled directly? I propose to keep priv->suspended. Using 'suspend.enabled' directly would simply not work as the pwm_disable() call in stm32_pwm_lp_suspend() routine marks the 'suspend' state.enabled = false. That's why it's saved in the suspend routine, to be restored upon resume. Thanks for reviewing, Best regards, Fabrice > >> + >> + return pwm_apply_state(&priv->chip.pwms[0], &priv->suspend); >> +} >> +#endif >> + >> +static SIMPLE_DEV_PM_OPS(stm32_pwm_lp_pm_ops, stm32_pwm_lp_suspend, >> + stm32_pwm_lp_resume); >> + >> static const struct of_device_id stm32_pwm_lp_of_match[] = { >> { .compatible = "st,stm32-pwm-lp", }, >> {}, >> @@ -235,6 +272,7 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev) >> .driver = { >> .name = "stm32-pwm-lp", >> .of_match_table = of_match_ptr(stm32_pwm_lp_of_match), >> + .pm = &stm32_pwm_lp_pm_ops, >> }, >> }; >> module_platform_driver(stm32_pwm_lp_driver); >> -- >> 1.9.1 >>
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@st.com> >>> --- >>> 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 >
On Wed, Feb 06, 2019 at 09:42:48AM +0100, Fabrice Gasnier wrote: > 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. I think the suspend callback should have something like: if (is_still_enabled) { /* * The consumer didn't stop us, so refuse to suspend. */ dev_err(dev, "The consumer didn't stop us, so refuse to suspend.\n"); return -EBUSY; } This way there are no bad surprises if the pwm is suspended before its consumer and it's obvious what is missing. Best regards Uwe
On Wed, Feb 06, 2019 at 09:54:05AM +0100, Uwe Kleine-König wrote: > On Wed, Feb 06, 2019 at 09:42:48AM +0100, Fabrice Gasnier wrote: > > 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. > > I think the suspend callback should have something like: > > if (is_still_enabled) { > /* > * The consumer didn't stop us, so refuse to suspend. > */ > dev_err(dev, "The consumer didn't stop us, so refuse to suspend.\n"); > return -EBUSY; > } > > This way there are no bad surprises if the pwm is suspended before its > consumer and it's obvious what is missing. Something that just occurred to me: perhaps as part of pwm_get() we should track where we were being requested from so that we could give hints in situations like this as to where the consumer is that forgot to suspend the PWM. I suppose we already have pwm_device.label to help with this, but perhaps we could improve things if we stored __builtin_return_address during pwm_get() to help users pinpoint where they need to look. Thierry
On 2/6/19 1:55 PM, Thierry Reding wrote: > On Wed, Feb 06, 2019 at 09:54:05AM +0100, Uwe Kleine-König wrote: >> On Wed, Feb 06, 2019 at 09:42:48AM +0100, Fabrice Gasnier wrote: >>> 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. >> >> I think the suspend callback should have something like: >> >> if (is_still_enabled) { >> /* >> * The consumer didn't stop us, so refuse to suspend. >> */ >> dev_err(dev, "The consumer didn't stop us, so refuse to suspend.\n"); >> return -EBUSY; >> } >> >> This way there are no bad surprises if the pwm is suspended before its >> consumer and it's obvious what is missing. Thierry, Uwe, When the pwm is suspended before its consumer, the bad surprise is the suspend request will fail... I'm not sure a new attempt may be better. So, it looks like the only way to have this clean is by implementing the device link e.g. via pwm_get() ? > > Something that just occurred to me: perhaps as part of pwm_get() we > should track where we were being requested from so that we could give > hints in situations like this as to where the consumer is that forgot > to suspend the PWM. The current approach handles the situation where the consumer forgot to suspend the PWM... I can add some warning about that in the suspend routine, incl the label. What do you think? What's the best approach ? Please advise, BR, Fabrice > > I suppose we already have pwm_device.label to help with this, but > perhaps we could improve things if we stored __builtin_return_address > during pwm_get() to help users pinpoint where they need to look. > > Thierry >
On Tue, 5 Feb 2019 13:40:29 +0100 Fabrice Gasnier <fabrice.gasnier@st.com> wrote: > Add suspend/resume PM sleep ops. When going to low power, disable > active counter. Only active counter should be resumed: don't touch > disabled counter, as it may be used by other LPTimer MFD child driver. > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> I think this looks fine. I'm not totally sure if patches 3,4 are separable from 1,2 though so please let me know whether I should take these now, or let all 4 go in together via some path? Thanks, Jonathan > --- > drivers/iio/counter/stm32-lptimer-cnt.c | 55 +++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/drivers/iio/counter/stm32-lptimer-cnt.c b/drivers/iio/counter/stm32-lptimer-cnt.c > index 42fb8ba..2a49cce 100644 > --- a/drivers/iio/counter/stm32-lptimer-cnt.c > +++ b/drivers/iio/counter/stm32-lptimer-cnt.c > @@ -14,6 +14,7 @@ > #include <linux/iio/iio.h> > #include <linux/mfd/stm32-lptimer.h> > #include <linux/module.h> > +#include <linux/pinctrl/consumer.h> > #include <linux/platform_device.h> > > struct stm32_lptim_cnt { > @@ -23,6 +24,7 @@ struct stm32_lptim_cnt { > u32 preset; > u32 polarity; > u32 quadrature_mode; > + bool enabled; > }; > > static int stm32_lptim_is_enabled(struct stm32_lptim_cnt *priv) > @@ -50,6 +52,7 @@ static int stm32_lptim_set_enable_state(struct stm32_lptim_cnt *priv, > > if (!enable) { > clk_disable(priv->clk); > + priv->enabled = false; > return 0; > } > > @@ -79,6 +82,7 @@ static int stm32_lptim_set_enable_state(struct stm32_lptim_cnt *priv, > regmap_write(priv->regmap, STM32_LPTIM_CR, 0); > return ret; > } > + priv->enabled = true; > > /* Start LP timer in continuous mode */ > return regmap_update_bits(priv->regmap, STM32_LPTIM_CR, > @@ -361,6 +365,56 @@ static int stm32_lptim_cnt_probe(struct platform_device *pdev) > return devm_iio_device_register(&pdev->dev, indio_dev); > } > > +#ifdef CONFIG_PM_SLEEP > +static int stm32_lptim_cnt_suspend(struct device *dev) > +{ > + struct stm32_lptim_cnt *priv = dev_get_drvdata(dev); > + int ret; > + > + /* Only take care of enabled counter: don't disturb other MFD child */ > + if (priv->enabled) { > + ret = stm32_lptim_setup(priv, 0); > + if (ret) > + return ret; > + > + ret = stm32_lptim_set_enable_state(priv, 0); > + if (ret) > + return ret; > + > + /* Force enable state for later resume */ > + priv->enabled = true; > + } > + > + return pinctrl_pm_select_sleep_state(dev); > +} > + > +static int stm32_lptim_cnt_resume(struct device *dev) > +{ > + struct stm32_lptim_cnt *priv = dev_get_drvdata(dev); > + int ret; > + > + ret = pinctrl_pm_select_default_state(dev); > + if (ret) > + return ret; > + > + if (priv->enabled) { > + priv->enabled = false; > + ret = stm32_lptim_setup(priv, 1); > + if (ret) > + return ret; > + > + ret = stm32_lptim_set_enable_state(priv, 1); > + if (ret) > + return ret; > + } > + > + return 0; > +} > +#endif > + > +static SIMPLE_DEV_PM_OPS(stm32_lptim_cnt_pm_ops, stm32_lptim_cnt_suspend, > + stm32_lptim_cnt_resume); > + > static const struct of_device_id stm32_lptim_cnt_of_match[] = { > { .compatible = "st,stm32-lptimer-counter", }, > {}, > @@ -372,6 +426,7 @@ static int stm32_lptim_cnt_probe(struct platform_device *pdev) > .driver = { > .name = "stm32-lptimer-counter", > .of_match_table = stm32_lptim_cnt_of_match, > + .pm = &stm32_lptim_cnt_pm_ops, > }, > }; > module_platform_driver(stm32_lptim_cnt_driver);
On Sat, Feb 09, 2019 at 04:21:33PM +0000, Jonathan Cameron wrote: > On Tue, 5 Feb 2019 13:40:29 +0100 > Fabrice Gasnier <fabrice.gasnier@st.com> wrote: > > > Add suspend/resume PM sleep ops. When going to low power, disable > > active counter. Only active counter should be resumed: don't touch > > disabled counter, as it may be used by other LPTimer MFD child driver. > > > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> > I think this looks fine. I'm not totally sure if patches 3,4 are > separable from 1,2 though so please let me know whether I should take > these now, or let all 4 go in together via some path? I'd say these are orthogonal, so if you are happy with patches 3 and 4 you can take them without bothering for patches 1 and 2. Best regards Uwe
On 2/10/19 10:33 PM, Uwe Kleine-König wrote: > On Sat, Feb 09, 2019 at 04:21:33PM +0000, Jonathan Cameron wrote: >> On Tue, 5 Feb 2019 13:40:29 +0100 >> Fabrice Gasnier <fabrice.gasnier@st.com> wrote: >> >>> Add suspend/resume PM sleep ops. When going to low power, disable >>> active counter. Only active counter should be resumed: don't touch >>> disabled counter, as it may be used by other LPTimer MFD child driver. >>> >>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> >> I think this looks fine. I'm not totally sure if patches 3,4 are >> separable from 1,2 though so please let me know whether I should take >> these now, or let all 4 go in together via some path? > > I'd say these are orthogonal, so if you are happy with patches 3 and 4 > you can take them without bothering for patches 1 and 2. Hi Uwe, Jonathan, Thanks for reviewing. I confirm you can take patches 3 and 4. I'll send a v2 for patches 1 and 2 separately. Best regards, Fabrice > > Best regards > Uwe >