Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume functions

[Boris Brezillon]

On Tue, 11 Apr 2017 12:41:59 +0300
m18063 <Claudiu.Beznea@xxxxxxxxxxxxx> wrote:

> On 11.04.2017 11:56, Boris Brezillon wrote:
> > On Tue, 11 Apr 2017 11:22:39 +0300
> > m18063 <Claudiu.Beznea@xxxxxxxxxxxxx> wrote:
> >   
> >> Hi Boris,
> >>
> >> On 10.04.2017 17:35, Boris Brezillon wrote:  
> >>> 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].    
> >>
> >> I agree with the approach you propose but the thing is the atmel_pwm_apply()
> >> take care of both, current PWM state and the new state received as argument
> >> in order to change only duty factor without disabling the PWM channel (if
> >> channel is enabled) and then returns. Changing PWM duty and period and polarity
> >> in the same step without disabling + enabling the PWM channel (with atomic
> >> approach) may lead to intermediary unwanted output waveforms (the IP doesn't
> >> support this for ordinary PWM channels). To take advantage of atmel_pwm_apply()
> >> (in the formit is today) in resume() hook might need to first call it to disable
> >> channel and then to enable it. Or atmel_pwm_apply() should be changed to also
> >> disable + enable the channel when user changes the duty factor at runtime.  
> > 
> > Nope. Just save the state at suspend time, implement ->get_state() and
> > use it to retrieve the real PWM state when resuming before restoring
> > the state you saved during suspend.  
> Ok.
> > But anyway, as Thierry explained, I'm not sure we should take the
> > 're-apply PWM state' action here. It's probably better to leave this
> > decision to the PWM user.   
> Do you thinks we should proceed with restoring the registers behind
> the re-apply as other drivers does at this moment?

Nope. IMO we'd better start patching PWM users to restore the states
rather than supporting suspend/resume in all PWM drivers.

Thierry, what's your opinion?