Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Daniel Thompson]

On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote:
> Hi Uwe,
> 
> Adding Daniel and Lee to the discussion ...

Thanks!

> On 8/10/19 22:31, Uwe Kleine-König wrote:
> > On Tue, Oct 08, 2019 at 06:33:15PM +0200, Enric Balletbo i Serra wrote:
> >>> A few thoughts to your approach here ...:
> >>>
> >>>  - Would it make sense to only store duty_cycle and enabled in the
> >>>    driver struct?
> >>>
> >>
> >> Yes, in fact, my first approach (that I didn't send) was only storing enabled
> >> and duty cycle. For some reason I ended storing the full pwm_state struct, but I
> >> guess is not really needed.
> >>
> >>
> >>>  - Which driver is the consumer of your pwm? If I understand correctly
> >>>    the following sequence is the bad one:
> >>>
> >>
> >> The consumer is the pwm_bl driver. Actually I'n trying to identify
> >> other consumers.
> > 
> 
> So far, the pwm_bl driver is the only consumer of cros-ec-pwm.
> 
> > Ah, I see why I missed to identify the problem back when I checked this
> > driver. The problem is not that .duty_cycle isn't set but there .enabled
> > isn't set. So maybe we just want:
> > 
> > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > index 2201b8c78641..0468c6ee4448 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -123,6 +123,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
> >         if (brightness > 0) {
> >                 pwm_get_state(pb->pwm, &state);
> >                 state.duty_cycle = compute_duty_cycle(pb, brightness);
> > +               state.enabled = true;
> >                 pwm_apply_state(pb->pwm, &state);
> >                 pwm_backlight_power_on(pb);
> >         } else
> > 
> > ? On a side note: It's IMHO strange that pwm_backlight_power_on
> > reconfigures the PWM once more.
> > 
> 
> Looking again to the pwm_bl code, now, I am not sure this is correct (although
> it probably solves the problem for me).

Looking at the pwm_bl code I wouldn't accept the above as it is but I'd
almost certainly accept a patch to pwm_bl to move the PWM enable/disable
out of both the power on/off functions so the duty-cycle/enable or
disable can happen in one go within the update_status function. I don't
think such a change would interfere with the power and enable sequencing
needed by panels and it would therefore be a nice continuation of the
work to convert over to the pwm_apply_state() API.

None of the above has anything to do with what is right or wrong for
the PWM API evolution. Of course, if this thread does conclude that it
is OK the duty cycle of a disabled PWM to be retained for some drivers
and not others then I'd hope to see some WARN_ON()s added to the PWM
framework to help bring problems to the surface with all drivers.


Daniel.