> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> 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@xxxxxxx>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
Thanks for the quick fix.
Tested-by: Aisheng Dong <asheng.dong@xxxxxxx>
Regards
Aisheng
> ---
> On Wed, Mar 22, 2023 at 08:10:54AM +0000, Aisheng Dong wrote:
> > > From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > > 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/ |
