Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume functions
From: Boris Brezillon
Date: Mon Apr 10 2017 - 10:36:05 EST
On Mon, 10 Apr 2017 17:20:20 +0300
Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx> wrote:
> Implement suspend and resume power management specific
> function to allow PWM controller to correctly suspend
> and resume.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>
> ---
> drivers/pwm/pwm-atmel.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 81 insertions(+)
>
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 530d7dc..75177c6 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -58,6 +58,8 @@
> #define PWM_MAX_PRD 0xFFFF
> #define PRD_MAX_PRES 10
>
> +#define PWM_MAX_CH_NUM (4)
> +
> struct atmel_pwm_registers {
> u8 period;
> u8 period_upd;
> @@ -65,11 +67,18 @@ struct atmel_pwm_registers {
> u8 duty_upd;
> };
>
> +struct atmel_pwm_pm_ctx {
> + u32 cmr;
> + u32 cdty;
> + u32 cprd;
> +};
> +
> struct atmel_pwm_chip {
> struct pwm_chip chip;
> struct clk *clk;
> void __iomem *base;
> const struct atmel_pwm_registers *regs;
> + struct atmel_pwm_pm_ctx ctx[PWM_MAX_CH_NUM];
Hm, I'm pretty sure you can rely on the current PWM state and call
atmel_pwm_apply() at resume time instead of doing that. See what I did
here [1].
Thierry, maybe it's time to start thinking about a generic solution to
save/restore PWM states.
>
> unsigned int updated_pwms;
> /* ISR is cleared when read, ensure only one thread does that */
> @@ -333,6 +342,77 @@ atmel_pwm_get_driver_data(struct platform_device *pdev)
> return (struct atmel_pwm_registers *)id->driver_data;
> }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int atmel_pwm_suspend(struct device *dev)
> +{
> + struct atmel_pwm_chip *atmel_pwm = dev_get_drvdata(dev);
> + struct pwm_device *pwm = atmel_pwm->chip.pwms;
> + int i;
> + bool disable_clk = false;
> +
> + for (i = 0; i < atmel_pwm->chip.npwm; i++, pwm++) {
> + if (!pwm_is_enabled(pwm))
> + continue;
> +
> + disable_clk = true;
> + atmel_pwm->ctx[i].cdty =
> + atmel_pwm_ch_readl(atmel_pwm, i,
> + atmel_pwm->regs->duty);
> + atmel_pwm->ctx[i].cprd =
> + atmel_pwm_ch_readl(atmel_pwm, i,
> + atmel_pwm->regs->period);
> + atmel_pwm->ctx[i].cmr =
> + atmel_pwm_ch_readl(atmel_pwm, i, PWM_CMR);
> +
> + atmel_pwm_disable(&atmel_pwm->chip, pwm, false);
> + }
> +
> + if (disable_clk)
> + clk_disable(atmel_pwm->clk);
I'm not so sure we want to disable the PWM and the PWM chip clk when
entering suspend. What if the PWM is driving a critical device (like a
regulator) that has to stay enabled in suspend?
Shouldn't we delegate this responsibility to the PWM user?
[1]http://patchwork.ozlabs.org/patch/734306/