Messages in this thread | | | Subject | Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume functions | From | m18063 <> | Date | Tue, 11 Apr 2017 11:33:44 +0300 |
| |
On 10.04.2017 19:27, Boris Brezillon wrote: > On Mon, 10 Apr 2017 18:01:37 +0200 > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > >> On Mon, 10 Apr 2017 17:10:11 +0200 >> Thierry Reding <thierry.reding@gmail.com> wrote: >> >>> On Mon, Apr 10, 2017 at 04:35:58PM +0200, 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]. >>>> >>>> Thierry, maybe it's time to start thinking about a generic solution to >>>> save/restore PWM states. >>> >>> Generally speaking I think applying the states are the right way to go. >>> Ideally the PWM core could simply resume all of the PWM channels that a >>> device exports and the ->apply() callback would be enough to restore >>> that. I'm not sure if that's going to work with current implementations, >>> though. Your pwm-atmel-hlcdc patch certainly indicates that we're not >>> quite there yet. >>> >>> On the other hand, I'm beginning to think that maybe PWMs are too low- >>> level for this kind of suspend/resume. For example if you use the PWM to >>> control a backlight brightness, restoring it via the driver core's >>> resume hook is potentially going to turn it back on at the wrong time. I >>> have a feeling that we might be better off just pushing this up to the >>> PWM users. A slight special case might be sysfs, for which no external >>> user driver exists. But we already have separate data structures to keep >>> track of sysfs-related context, so suspend/resume support could be added >>> there. >> >> Yep, you're probably right, we should let the PWM user take care of >> re-applying the PWM state, because it's the only one having enough >> knowledge about what the PWM is really driving to take a wise decision. > > Note that we need drivers to implement both ->apply() and ->get_state() > for this approach to work correctly, and we also need some help from > the core to reset the PWM states at resume time, otherwise > pwm_apply_state() will just compare the old state to the new one, see > that they match and never call the ->apply() method. > > Another solution would be to remove the memcmp here [1] and > unconditionally call ->apply(). There are drivers which checks, in ->apply() hooks, the current PWM state before applying the new state or take actions based on differences b/w current and new PWM states. Removing memcmp without resetting the PWM state would lead to wrong states in those drivers.
> > [1]http://lxr.free-electrons.com/source/drivers/pwm/core.c#L466 >
| |