Message ID | 20230322085129.jxxz55tbcxkc6usd@pengutronix.de |
---|---|
State | New |
Headers | show |
Series | backlight: pwm_bl: Disable PWM on shutdown and suspend disabled PWM emiting inactive state") | expand |
> 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/ |
On Tue, 26 Sep 2023, Lee Jones wrote: > On Tue, 26 Sep 2023, Uwe Kleine-König wrote: > > > Hello, > > > > On Wed, Mar 22, 2023 at 09:51:29AM +0100, Uwe Kleine-König wrote: > > > 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> > > > > This patch was never applied but I think it is still needed. I assume it > > fell through the cracks? > > This "patch" was sent half way through a thread and when opened in my > mail client looks like a mail reply due to the quotes below the '---'. > > I'd suggest sending this again. You also have a copy/paste error in the subject line.
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);