RE: [PATCH] backlight: pwm_bl: Disable PWM on shutdown and suspend disabled PWM emiting inactive state")

[Aisheng Dong <aisheng.dong@nxp.com>]

> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: 2023年3月22日 16:51
> 
> Since commit 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only once
> per backlight toggle") calling pwm_backlight_power_off() doesn't disable the
> PWM any more. However this is necessary to suspend, because PWM drivers
> usually refuse to suspend if they are still enabled.
> 
> Also adapt shutdown to disable the PWM for similar reasons.
> 
> Fixes: 00e7e698bff1 ("backlight: pwm_bl: Configure pwm only once per
> backlight toggle")
> Reported-by: Aisheng Dong <aisheng.dong@nxp.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks for the quick fix.
Tested-by: Aisheng Dong <asheng.dong@nxp.com>

Regards
Aisheng

> ---
> On Wed, Mar 22, 2023 at 08:10:54AM +0000, Aisheng Dong wrote:
> > > From: Uwe Kleine-König <u.kleine-koenig@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.
> 
> Ah, I see, because of 00e7e698bff1 the pwm isn't modified any more if only
> pwm_backlight_power_off() (but not pwm_backlight_update_status()) is called.
> But that is what the suspend callback (and also shutdown) does.
> 
> > > 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.
> 
> OK, so a backlight with neither an enable-gpio nor a regulator. So this is a
> configuration where the PWM isn't disabled any more when
> brightness=0 is set. Iff the driver emits its inactive state when disabled (for
> both polarities), it might disable if .duty_cycle = 0 is requested to safe some
> power.
> 
> > > 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?
> 
> Sure. I expect this fixes your issue, but I didn't test it. So if you give it a shot
> that would be great.
> 
> Best regards
> Uwe
> 
>  drivers/video/backlight/pwm_bl.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c
> b/drivers/video/backlight/pwm_bl.c
> index fb388148d98f..577714e41694 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -643,8 +643,13 @@ static void pwm_backlight_shutdown(struct
> platform_device *pdev)  {
>  	struct backlight_device *bl = platform_get_drvdata(pdev);
>  	struct pwm_bl_data *pb = bl_get_data(bl);
> +	struct pwm_state state;
> 
>  	pwm_backlight_power_off(pb);
> +	pwm_get_state(pb->pwm, &state);
> +	state.duty_cycle = 0;
> +	state.enabled = false;
> +	pwm_apply_state(pb->pwm, &state);
>  }
> 
>  #ifdef CONFIG_PM_SLEEP
> @@ -652,12 +657,24 @@ static int pwm_backlight_suspend(struct device
> *dev)  {
>  	struct backlight_device *bl = dev_get_drvdata(dev);
>  	struct pwm_bl_data *pb = bl_get_data(bl);
> +	struct pwm_state state;
> 
>  	if (pb->notify)
>  		pb->notify(pb->dev, 0);
> 
>  	pwm_backlight_power_off(pb);
> 
> +	/*
> +	 * Note that disabling the PWM doesn't guarantee that the output stays
> +	 * at its inactive state. However without the PWM disabled, the PWM
> +	 * driver refuses to suspend. So disable here even though this might
> +	 * enable the backlight on poorly designed boards.
> +	 */
> +	pwm_get_state(pb->pwm, &state);
> +	state.duty_cycle = 0;
> +	state.enabled = false;
> +	pwm_apply_state(pb->pwm, &state);
> +
>  	if (pb->notify_after)
>  		pb->notify_after(pb->dev, 0);
> 
> --
> 2.39.2
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |