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

[Uwe Kleine-König]

Hello Thierry,

On Thu, Oct 17, 2019 at 02:59:32PM +0200, Thierry Reding wrote:
> On Thu, Oct 17, 2019 at 02:09:17PM +0200, Uwe Kleine-König wrote:
> > On Thu, Oct 17, 2019 at 01:11:31PM +0200, Thierry Reding wrote:
> > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> > > index ae11d8577f18..4113d5cd4c62 100644
> > > --- a/drivers/pwm/pwm-imx27.c
> > > +++ b/drivers/pwm/pwm-imx27.c
> > > [...]
> > 
> > I wonder if it would be more sensible to do this in the pwm core
> > instead. Currently there are two drivers known with this problem. I
> > wouldn't be surprised if there were more.
> 
> I've inspected all the drivers and didn't spot any beyond cros-ec and
> i.MX that have this problem.

I took a look, too, and I'd say pwm-atmel.c, pwm-imx-tpm.c, pwm-lpss.c,
pwm-meson.c, pwm-sifive.c, pwm-sprd.c and pwm-stm32-lp.c are affected.

> So the core would have to rely on state->duty_cycle that is passed in,
> but then the offending commit becomes useless because the whole point
> was to return the state as written to hardware (rather than the
> software state which was being returned before that patch).

I like allowing lowlevel drivers to implement the .enabled = false case
lazily as there is little value in calculating the needed register
values for a request like

	.duty_cycle = X, .period = Y, .enabled = false

even if the next request might be

	.duty_cycle = X, .period = Y, .enabled = true

because quite likely the same calculation is done for the second request
again and there is no benefit to save X and Y in the hardware (or the
driver if the hardware is incapable) apart from returning it to a
consumer who maybe even doesn't care because these values don't tell
anything at all about the implemented wave form and so it seems natural
to me that the lowlevel driver shouldn't care.

> > If we want to move clients to not rely on .period and .duty_cycle for a
> > disabled PWM (do we?) a single change in the core is also beneficial
> > compared to fixing several lowlevel drivers.
> 
> These are really two orthogonal problems. We don't currently consider
> enabled = 0 to be equivalent to duty_cycle = 0 at an API level. I'm not
> prepared to do that at this point in the release cycle either.

Yeah, I fully agree that we should not do that now. Given the number of
affected drivers I opt for reverting and retrying again more carefully
later.
 
> What this here has shown is that we have at least two drivers that don't
> behave the way they are supposed to according to the API and they break
> consumers. If they break for pwm-backlight, it's possible that they will
> break for other consumers as well. So the right thing to do is fix the
> two drivers that are broken.

In my eyes shifting the definition of "expected behaviour" and adapting
the core code accordingly to make this invisible to consumers is also a
viable option. Also shifting the definition and adapt all consumers not
to rely on the old behaviour is fine. But as above, this is not a good
idea at the current point in time.

Best regards
Uwe

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