[2/4] pwm: stm32-lp: Add power management support
diff mbox series

Message ID 1549370429-19116-3-git-send-email-fabrice.gasnier@st.com
State Superseded
Headers show
Series
  • Add PM support to STM32 LP Timer drivers
Related show

Commit Message

Fabrice Gasnier Feb. 5, 2019, 12:40 p.m. UTC
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(+)

Comments

Tomasz Duszynski Feb. 5, 2019, 6:30 p.m. UTC | #1
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
>
Uwe Kleine-König Feb. 5, 2019, 8:47 p.m. UTC | #2
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
Thierry Reding Feb. 5, 2019, 10:25 p.m. UTC | #3
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
Fabrice Gasnier Feb. 6, 2019, 8:42 a.m. UTC | #4
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
>>
Fabrice Gasnier Feb. 6, 2019, 8:42 a.m. UTC | #5
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
>
Uwe Kleine-König Feb. 6, 2019, 8:54 a.m. UTC | #6
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
Thierry Reding Feb. 6, 2019, 12:55 p.m. UTC | #7
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
Fabrice Gasnier Feb. 6, 2019, 2:54 p.m. UTC | #8
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
>

Patch
diff mbox series

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);
+}
+
+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;
+
+	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);