lkml.org 
[lkml]   [2018]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume functions
From
Date


On 05.12.2017 11:06, Thierry Reding wrote:
> On Tue, Apr 11, 2017 at 11:53:11AM +0200, Boris Brezillon wrote:
>> On Tue, 11 Apr 2017 12:41:59 +0300
>> m18063 <Claudiu.Beznea@microchip.com> wrote:
>>
>>> On 11.04.2017 11:56, Boris Brezillon wrote:
>>>> On Tue, 11 Apr 2017 11:22:39 +0300
>>>> m18063 <Claudiu.Beznea@microchip.com> 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@microchip.com> 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@microchip.com>
>>>>>>> ---
>>>>>>> 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?
>
> I just noticed this thread while cleaning up patchwork. I think I had
> already mentioned in an earlier reply that in my opinion we should leave
> PWM suspend/resume to users.
What about the case where PWM was requested via sysfs?
>
> I'm totally fine if we add helpers to the PWM core to help with that
> task. Maybe something like this would work:
>
> void pwm_suspend(struct pwm_device *pwm)
> {
> pwm_get_state(pwm, &pwm->suspend);
> pwm_disable(pwm);
> }
>
> void pwm_resume(struct pwm_device *pwm)
> {
> pwm_apply_state(pwm, &pwm->suspend);
> }
>
> Though, quite frankly, this is so trivial that drivers could just do
> that themselves. Also, the helpers above aren't flexible at all with
> respect to any special sequences the PWM might need to go through on
> suspend. I suspect that this doesn't matter at all in most cases but
> given how trivial they are we might as well just make drivers do it.
> Also we don't burden users that don't care about suspend/resume with
> the extra suspend state in struct pwm_device.
>
> Thierry
>

\
 
 \ /
  Last update: 2018-01-14 23:23    [W:0.069 / U:1.600 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site