[PATCH] backlight: pwm_bl: configure pwm only once per backlight toggle

[Adam Ford]

On Thu, Oct 17, 2019 at 7:34 AM Adam Ford <aford173 at gmail.com> wrote:
>
> On Thu, Oct 17, 2019 at 7:19 AM Uwe Kleine-König
> <u.kleine-koenig at pengutronix.de> wrote:
> >
> > On Thu, Oct 17, 2019 at 12:47:27PM +0100, Daniel Thompson wrote:
> > > On Thu, Oct 17, 2019 at 10:10:59AM +0200, Uwe Kleine-König wrote:
> > > > A previous change in the pwm core (namely 01ccf903edd6 ("pwm: Let
> > > > pwm_get_state() return the last implemented state")) changed the
> > > > semantic of pwm_get_state() and disclosed an (as it seems) common
> > > > problem in lowlevel PWM drivers. By not relying on the period and duty
> > > > cycle being retrievable from a disabled PWM this type of problem is
> > > > worked around.
> > > >
> > > > Apart from this issue only calling the pwm_get_state/pwm_apply_state
> > > > combo once is also more effective.
> > >
> > > I'm only interested in the second paragraph here.
> > >
> > > There seems to be a reasonable consensus that the i.MX27 and cros-ec
> > > PWM drivers should be fixed for the benefit of other PWM clients.
> > > So we make this change because it makes the pwm-bl better... not to
> > > work around bugs ;-).
> >
> > That's fine, still I think it's fair to explain the motivation of
> > creating this patch.
> >
> > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > index 746eebc411df..ddebd62b3978 100644
> > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > @@ -67,40 +62,27 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb)
> > > >
> > > >  static void pwm_backlight_power_off(struct pwm_bl_data *pb)
> > > >  {
> > > > -   struct pwm_state state;
> > > > -
> > > > -   pwm_get_state(pb->pwm, &state);
> > > > -   if (!pb->enabled)
> > > > -           return;
> > > > -
> > >
> > > Why remove the pb->enabled check? I thought that was there to ensure we
> > > don't mess up the regular reference counts.
> >
> > I havn't looked yet, but I guess I have to respin. Expect a v2 later
> > today.
>
> I would agree that a high-level fix is better than a series of low
> level driver fixes.  For what its worth, your V1 patch worked fine on
> my i.MX6Q.  I can test the V2 patch when its ready.

I may have spoken too soon.  The patch fixes the display in that it
comes on when it previously did not, but changes to brightness do not
appear to do anything anymore.  I don't have an oscilloscope where I
am now, so I cannot verify whether or not the PWM duty cycle changes.

To my eye, the following do not appear to change the brightness of the screen:
     echo 7 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness
     echo 2 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness
     echo 5 > /sys/devices/soc0/backlight-lvds/backlight/backlight-lvds/brightness


adam
>
> adam
> >
> > Best regards
> > Uwe
> >
> > --
> > Pengutronix e.K.                           | Uwe Kleine-König            |
> > Industrial Linux Solutions                 | http://www.pengutronix.de/  |