Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1620022 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume  functions Thierry Reding <thierry.reding@gmail.com> - 2017-04-10 17:20 +0200
    Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume  functions Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-04-10 18:10 +0200
      Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume  functions Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-04-10 18:30 +0200
        Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume  functions m18063 <Claudiu.Beznea@microchip.com> - 2017-04-11 10:40 +0200
          Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume  functions Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-04-11 11:00 +0200
            Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume  functions m18063 <Claudiu.Beznea@microchip.com> - 2017-04-11 11:10 +0200

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

[Multipart message — attachments visible in raw view] — view raw

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.

Any thoughts on that?

Thierry

[toc] | [next] | [standalone]

#1620180

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.

This goes against my patch adding suspend/resume hooks to the
pwm-atmel-hlcdc driver, but we can easily drop the call to
atmel_hlcdc_pwm_apply() in ->resume() once we have patched the
pwm-backlight driver to take care of that.

[toc] | [prev] | [next] | [standalone]

#1620216

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().

[1]http://lxr.free-electrons.com/source/drivers/pwm/core.c#L466

[toc] | [prev] | [next] | [standalone]

#1621032

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
> 

[toc] | [prev] | [next] | [standalone]

#1621038

On Tue, 11 Apr 2017 11:33:44 +0300
m18063 <Claudiu.Beznea@microchip.com> wrote:

> 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.

Indeed. So it just leaves the solution where we implement ->get_state().
Honestly, it shouldn't be too hard to do that in the atmel driver.

Note that for drivers that do not implement ->get_state(), the first
pwm_apply_state() after the system has resumed should be harmless,
because the current PWM should exactly match the one the PWM user is
re-applying.

[toc] | [prev] | [next] | [standalone]

#1621047

On 11.04.2017 11:50, Boris Brezillon wrote:
> On Tue, 11 Apr 2017 11:33:44 +0300
> m18063 <Claudiu.Beznea@microchip.com> wrote:
> 
>> 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.
> 
> Indeed. So it just leaves the solution where we implement ->get_state().
> Honestly, it shouldn't be too hard to do that in the atmel driver.
I agree.
> 
> Note that for drivers that do not implement ->get_state(), the first
> pwm_apply_state() after the system has resumed should be harmless,
> because the current PWM should exactly match the one the PWM user is
> re-applying.
I agree.

> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web