Regression in deaeeda2051f ("backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive state")

[Aisheng Dong]

Hi Uwe,

> From: Uwe Kleine-König <u.kleine-koenig at pengutronix.de>
> Sent: 2023年3月22日 15:04
> 
> Hello,
> 
> hmm, the subject is wrong, this is about commit deaeeda2051f
> ("backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive
> state") and not 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only once
> per backlight toggle"). I fixed it accordingly.

I just double checked that only revert deaeeda2051f can't fix the issue.
I have to revert 00e7e698bff1 as well.
Below are my top commits:
2c8b0985593a (HEAD -> next-20230315) Revert "backlight: pwm_bl: Configure pwm only once per backlight toggle"
e6d0aba366a7 Revert "backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive state"
225b6b81afe6 (tag: next-20230315, linux-next-cu/master) Add linux-next specific files for 20230315
...

> 
> On Wed, Mar 22, 2023 at 05:13:24AM +0000, Aisheng Dong wrote:
> > It seems this patch changed the behavior of pwm_backlight_suspend a
> > little bit in
> > pwm_backlight_power_off() that pwm state keep unchanged during
> suspend.
> > Then pwm_imx_tpm_suspend() will return -Ebusy due to
> tpm->enable_count > 0.
> > Was this intended behavior? Should we fix pwm core or the driver?
> 
> A I see. The problem is the combination of the following facts:
> 
>  - Some PWMs don't emit a constant inactive signal when disabled, so a
>    consumer who wants a constant inactive signal must not disable the
>    PWM.
> 
>  - A used PWM is supposed to be disabled by its consumer on suspend.
>    (This is right IMHO because on suspend the PWM is likely to stop
>    oscillating and if the consumer requested some output wave form a
>    suspend usually stops to adhere to the consumer's request.)
> 
> So the options to fix this are (in order of my preference):
> 
>  a) Improve the check in the pwm_bl driver when it's safe to disable the
>     PWM.
> 
>  b) Disable the PWM on suspend in the pwm_bl driver.
> 
>  c) If the pwm-imx-tpm's PWM output is configured with duty_cycle = 0 and
>     is known not to continue driving a constant inactive signal on
>     suspend, it might continue to suspend.
> 
> I think a) is not possible in general. To determine that: Which machine do you
> experience this regression on?

Imx7ulp evk.

> 
> b) is the right one from the PWM framework's POV. If you have a PWM like
> pwm-imx27 that might result in the backlight going on on suspend. That's bad,
> but compared to the pre-deaeeda2051f state it's still an improvement (as
> there the backlight went on on disable *and* suspend).
> Depending on the machine the backlight might or might not go off again later
> when suspend progresses.
> 

This seems like the previous working behavior on mx7ulp without this patch.
Would you help make a patch to fix it?

Regards
Aisheng

> c) isn't that nice because that's an a bit special behaviour and people who
> intend to write code that is correct for all PWMs but only have an
> pwm-imx-tpm to test might fail to do so.
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |