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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

On Thu, Oct 17, 2019 at 12:59:20PM +0200, Michal Vokáč wrote:
> On 17. 10. 19 11:48, Michal Vokáč wrote:
> > On 17. 10. 19 10:10, 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.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > > Hello,
> > > 
> > > There are now two reports about 01ccf903edd6 breaking a backlight. As
> > > far as I understand the problem this is a combination of the backend pwm
> > > driver yielding surprising results and the pwm-bl driver doing things
> > > more complicated than necessary.
> > > 
> > > So I guess this patch works around these problems. Still it would be
> > > interesting to find out the details in the imx driver that triggers the
> > > problem. So Adam, can you please instrument the pwm-imx27 driver to
> > > print *state at the beginning of pwm_imx27_apply() and the end of
> > > pwm_imx27_get_state() and provide the results?
> > > 
> > > Note I only compile tested this change.
> > 
> > Hi Uwe,
> > I was just about to respond to the "pwm_bl on i.MX6Q broken on 5.4-RC1+"
> > thread that I have a similar problem when you submitted this patch.
> > 
> > So here are my few cents:
> 
> Once again with updated and more detailed debug messages.
> 
> > My setup is as follows:
> >   - imx6dl-yapp4-draco with i.MX6Solo
> >   - backlight is controlled with inverted PWM signal
> >   - max brightness level = 32, default brightness level set to 32 in DT.
> > 
> > 1. Almost correct backlight behavior before 01ccf903edd6 ("pwm: Let
> >     pwm_get_state() return the last implemented state):
> > 
> >   - System boots to userspace and backlight is enabled all the time from
> >     power up.
> > 
>     -  $ dmesg | grep pwm_
>       [    1.761546] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0
>       [    5.012352] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0
>       [    5.021143] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 0
>       [    5.030182] pwm_imx27_apply: period: 500000, duty: 500000, polarity: 1, enabled: 1
> > 
> >   - $ cat brightness
> >     32
> > 
> >   - $ echo 32 > brightness # nothing happens, max. brightness
> > 
> >   - $ echo 1 > brightness # backlight goes down to lowest level
>       [   93.976354] pwm_imx27_apply: period: 500000, duty: 7843, polarity: 1, enabled: 1
> > 
> >   - $ echo 0 > brightness # backlight goes up to max. level, this is
> >                           # problem of the inverted PWM on i.MX we attempted
> >                           # to solve some time ago.
>       [  115.496350] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0
> > 
> > 2. Backlight behavior on v5.4-rc3:
> > 
> >   - System boots to userspace and backlight is enabled all the time from
> >     power up.
> > 
>     - $ dmesg | grep pwm_
>       [    1.774071] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0
>       [    5.003961] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0
>       [    5.012649] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0
>       [    5.021694] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 0
>       [    5.030732] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0
>       [    5.039643] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 1
>       [    5.049605] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 1
> > 
> >   - $ cat brightness
> >     32
> > 
> >   - $ echo 32 > brightness # backlight goes down
>       [  707.946970] pwm_imx27_apply: period: 992970, duty: 992970, polarity: 0, enabled: 1
>       [  707.958551] pwm_imx27_get_state: period: 992970, duty: 992970, polarity: 0, enabled: 1
> > 
> >   - $ echo 1 > brightness # backlight goes up to high level
>       [  757.516845] pwm_imx27_apply: period: 992970, duty: 15576, polarity: 0, enabled: 1
>       [  757.528438] pwm_imx27_get_state: period: 992970, duty: 15576, polarity: 0, enabled: 1
> > 
> >   - $ echo 0 > brightness # backlight goes up to highest level
>       [  783.386838] pwm_imx27_apply: period: 992970, duty: 0, polarity: 0, enabled: 0
>       [  783.398025] pwm_imx27_get_state: period: 496485, duty: 0, polarity: 0, enabled: 0
> > 
> > So the behavior is clearly inverted to how it worked prior to 01ccf903edd6
> > with the weird exception that the initial brightness level 32 is
> > not applied.
> > 
> > 3. Backlight behavior on v5.4-rc3 + this patch:
> > 
> >   - System boots with backlight enabled. In the middle of kernel boot
> >     backlight is disabled.
> > 
> >   - $ dmesg | grep state
>       [    1.773099] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0
>       [    5.002532] pwm_imx27_apply: period: 500000, duty: 0, polarity: 1, enabled: 0
>       [    5.011263] pwm_imx27_get_state: period: 992970, duty: 0, polarity: 0, enabled: 0

Here is another corner case: pwm_imx27_apply doesn't even set polarity
in hardware if .enabled is false. This way .polarity is lost as
obviously the wrong setting is returned by pwm_get_state() later.

I didn't check in detail, but at least atmel, atmel-hlcdc, fsl-ftm,
stm32 and stm32-lp seem to suffer from the same problem.

Best regards
Uwe

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