[PATCH v2] backlight: pwm_bl: Disable PWM on shutdown and suspend

[PATCH v2] backlight: pwm_bl: Disable PWM on shutdown and suspend

[Uwe Kleine-König]

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>
Tested-by: Aisheng Dong <asheng.dong@nxp.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

(implicit) v1 was sent in the mail thread containing the patch that
became 00e7e698bff1 and had some copy-and-paste-failure in the Subject
line. Both are fixed here.

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 a51fbab96368..3ed7b76aa06c 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -638,8 +638,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
@@ -647,12 +652,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);
 

base-commit: 8fff9184d1b5810dca5dd1a02726d4f844af88fc
-- 
2.40.1

Re: [PATCH v2] backlight: pwm_bl: Disable PWM on shutdown and suspend

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]

On Tue, Sep 26, 2023 at 10:46:12AM +0200, 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>
> Tested-by: Aisheng Dong <asheng.dong@nxp.com>

The two email addresses should actually be identical, the first one is
the right one. (Failure introduced by
https://lore.kernel.org/dri-devel/DB9PR04MB8477D4BBF1B31035789DA08680869@DB9PR04MB8477.eurprd04.prod.outlook.com
and picked up by b4.) Lee, can you fixup or should I resend?

Best regards
Uwe


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] backlight: pwm_bl: Disable PWM on shutdown and suspend

[Lee Jones]

On Tue, 26 Sep 2023, Uwe Kleine-König wrote:

> On Tue, Sep 26, 2023 at 10:46:12AM +0200, 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>
> > Tested-by: Aisheng Dong <asheng.dong@nxp.com>
> 
> The two email addresses should actually be identical, the first one is
> the right one. (Failure introduced by
> https://lore.kernel.org/dri-devel/DB9PR04MB8477D4BBF1B31035789DA08680869@DB9PR04MB8477.eurprd04.prod.outlook.com
> and picked up by b4.) Lee, can you fixup or should I resend?

I'll probably forget by the time Daniel gets around to review.

Your call, but probably safer to turn it around yourself.

-- 
Lee Jones [李琼斯]

Re: [PATCH v2] backlight: pwm_bl: Disable PWM on shutdown and suspend

[Daniel Thompson]

On Tue, Sep 26, 2023 at 10:46:12AM +0200, 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>
> Tested-by: Aisheng Dong <asheng.dong@nxp.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Changes proposed look good (and the comment about badly designed boards
going to HiZ state we super helpful).

Only thing from my is why there is no attempt to disable the PWM
from the .remove_new() callback.


Daniel.