[PATCH v2 0/2] backlight: pwm_bl: Two PWM releated changes

[PATCH v2 0/2] backlight: pwm_bl: Two PWM releated changes

[Uwe Kleine-König]

Hello,

here comes the promised v2 of this series. Implicit v1 is available at
https://lore.kernel.org/dri-devel/20230109204758.610400-1-u.kleine-koenig@pengutronix.de
.

Changes since (implicit) v1 all due to feedback by Daniel Thompson:

 - Update changelogs
 - Still disable PWM if there is an enable gpio or a regulator

Best regards
Uwe

Uwe Kleine-König (2):
  backlight: pwm_bl: Configure pwm only once per backlight toggle
  backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive state

 drivers/video/backlight/pwm_bl.c | 56 ++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 25 deletions(-)


base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
prerequisite-patch-id: a0c7497a32092d284bc47eda60e4b3690338ba6e
-- 
2.39.0

[PATCH v2 0/2] backlight: pwm_bl: Two PWM releated changes

[Uwe Kleine-König]

Hello,

here comes the promised v2 of this series. Implicit v1 is available at
https://lore.kernel.org/dri-devel/20230109204758.610400-1-u.kleine-koenig@pengutronix.de
.

Changes since (implicit) v1 all due to feedback by Daniel Thompson:

 - Update changelogs
 - Still disable PWM if there is an enable gpio or a regulator

Best regards
Uwe

Uwe Kleine-König (2):
  backlight: pwm_bl: Configure pwm only once per backlight toggle
  backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive state

 drivers/video/backlight/pwm_bl.c | 56 ++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 25 deletions(-)


base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
prerequisite-patch-id: a0c7497a32092d284bc47eda60e4b3690338ba6e
-- 
2.39.0

[PATCH v2 1/2] backlight: pwm_bl: Configure pwm only once per backlight toggle

[Uwe Kleine-König]

When the function pwm_backlight_update_status() was called with
brightness > 0, pwm_get_state() was called twice (once directly and once
in compute_duty_cycle). Also pwm_apply_state() was called twice (once in
pwm_backlight_power_on() and once directly).

Optimize this to do both calls only once.

Note that with this affects the order of regulator and PWM setup. It's
not expected to have a relevant effect on hardware. The rationale for
this is that the regulator (and the GPIO) are reasonable to switch in
pwm_backlight_power_on()/pwm_backlight_power_off() but the PWM has
nothing to do with power. (The post_pwm_on_delay and pwm_off_delay are
still there though.)

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/video/backlight/pwm_bl.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index d0b22158cd70..0509fecd5715 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -40,10 +40,8 @@ struct pwm_bl_data {
 
 static void pwm_backlight_power_on(struct pwm_bl_data *pb)
 {
-	struct pwm_state state;
 	int err;
 
-	pwm_get_state(pb->pwm, &state);
 	if (pb->enabled)
 		return;
 
@@ -51,9 +49,6 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb)
 	if (err < 0)
 		dev_err(pb->dev, "failed to enable power supply\n");
 
-	state.enabled = true;
-	pwm_apply_state(pb->pwm, &state);
-
 	if (pb->post_pwm_on_delay)
 		msleep(pb->post_pwm_on_delay);
 
@@ -65,9 +60,6 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb)
 
 static void pwm_backlight_power_off(struct pwm_bl_data *pb)
 {
-	struct pwm_state state;
-
-	pwm_get_state(pb->pwm, &state);
 	if (!pb->enabled)
 		return;
 
@@ -77,28 +69,21 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
 	if (pb->pwm_off_delay)
 		msleep(pb->pwm_off_delay);
 
-	state.enabled = false;
-	state.duty_cycle = 0;
-	pwm_apply_state(pb->pwm, &state);
-
 	regulator_disable(pb->power_supply);
 	pb->enabled = false;
 }
 
-static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
+static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness, struct pwm_state *state)
 {
 	unsigned int lth = pb->lth_brightness;
-	struct pwm_state state;
 	u64 duty_cycle;
 
-	pwm_get_state(pb->pwm, &state);
-
 	if (pb->levels)
 		duty_cycle = pb->levels[brightness];
 	else
 		duty_cycle = brightness;
 
-	duty_cycle *= state.period - lth;
+	duty_cycle *= state->period - lth;
 	do_div(duty_cycle, pb->scale);
 
 	return duty_cycle + lth;
@@ -115,11 +100,18 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 
 	if (brightness > 0) {
 		pwm_get_state(pb->pwm, &state);
-		state.duty_cycle = compute_duty_cycle(pb, brightness);
+		state.duty_cycle = compute_duty_cycle(pb, brightness, &state);
+		state.enabled = true;
 		pwm_apply_state(pb->pwm, &state);
+
 		pwm_backlight_power_on(pb);
 	} else {
 		pwm_backlight_power_off(pb);
+
+		pwm_get_state(pb->pwm, &state);
+		state.enabled = false;
+		state.duty_cycle = 0;
+		pwm_apply_state(pb->pwm, &state);
 	}
 
 	if (pb->notify_after)
-- 
2.39.0

[PATCH v2 1/2] backlight: pwm_bl: Configure pwm only once per backlight toggle

[Uwe Kleine-König]

When the function pwm_backlight_update_status() was called with
brightness > 0, pwm_get_state() was called twice (once directly and once
in compute_duty_cycle). Also pwm_apply_state() was called twice (once in
pwm_backlight_power_on() and once directly).

Optimize this to do both calls only once.

Note that with this affects the order of regulator and PWM setup. It's
not expected to have a relevant effect on hardware. The rationale for
this is that the regulator (and the GPIO) are reasonable to switch in
pwm_backlight_power_on()/pwm_backlight_power_off() but the PWM has
nothing to do with power. (The post_pwm_on_delay and pwm_off_delay are
still there though.)

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/video/backlight/pwm_bl.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index d0b22158cd70..0509fecd5715 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -40,10 +40,8 @@ struct pwm_bl_data {
 
 static void pwm_backlight_power_on(struct pwm_bl_data *pb)
 {
-	struct pwm_state state;
 	int err;
 
-	pwm_get_state(pb->pwm, &state);
 	if (pb->enabled)
 		return;
 
@@ -51,9 +49,6 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb)
 	if (err < 0)
 		dev_err(pb->dev, "failed to enable power supply\n");
 
-	state.enabled = true;
-	pwm_apply_state(pb->pwm, &state);
-
 	if (pb->post_pwm_on_delay)
 		msleep(pb->post_pwm_on_delay);
 
@@ -65,9 +60,6 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb)
 
 static void pwm_backlight_power_off(struct pwm_bl_data *pb)
 {
-	struct pwm_state state;
-
-	pwm_get_state(pb->pwm, &state);
 	if (!pb->enabled)
 		return;
 
@@ -77,28 +69,21 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
 	if (pb->pwm_off_delay)
 		msleep(pb->pwm_off_delay);
 
-	state.enabled = false;
-	state.duty_cycle = 0;
-	pwm_apply_state(pb->pwm, &state);
-
 	regulator_disable(pb->power_supply);
 	pb->enabled = false;
 }
 
-static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
+static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness, struct pwm_state *state)
 {
 	unsigned int lth = pb->lth_brightness;
-	struct pwm_state state;
 	u64 duty_cycle;
 
-	pwm_get_state(pb->pwm, &state);
-
 	if (pb->levels)
 		duty_cycle = pb->levels[brightness];
 	else
 		duty_cycle = brightness;
 
-	duty_cycle *= state.period - lth;
+	duty_cycle *= state->period - lth;
 	do_div(duty_cycle, pb->scale);
 
 	return duty_cycle + lth;
@@ -115,11 +100,18 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 
 	if (brightness > 0) {
 		pwm_get_state(pb->pwm, &state);
-		state.duty_cycle = compute_duty_cycle(pb, brightness);
+		state.duty_cycle = compute_duty_cycle(pb, brightness, &state);
+		state.enabled = true;
 		pwm_apply_state(pb->pwm, &state);
+
 		pwm_backlight_power_on(pb);
 	} else {
 		pwm_backlight_power_off(pb);
+
+		pwm_get_state(pb->pwm, &state);
+		state.enabled = false;
+		state.duty_cycle = 0;
+		pwm_apply_state(pb->pwm, &state);
 	}
 
 	if (pb->notify_after)
-- 
2.39.0

Re: [PATCH v2 1/2] backlight: pwm_bl: Configure pwm only once per backlight toggle

[Lee Jones]

On Fri, 20 Jan 2023, Uwe Kleine-König wrote:

> When the function pwm_backlight_update_status() was called with
> brightness > 0, pwm_get_state() was called twice (once directly and once
> in compute_duty_cycle). Also pwm_apply_state() was called twice (once in
> pwm_backlight_power_on() and once directly).
> 
> Optimize this to do both calls only once.
> 
> Note that with this affects the order of regulator and PWM setup. It's
> not expected to have a relevant effect on hardware. The rationale for
> this is that the regulator (and the GPIO) are reasonable to switch in
> pwm_backlight_power_on()/pwm_backlight_power_off() but the PWM has
> nothing to do with power. (The post_pwm_on_delay and pwm_off_delay are
> still there though.)
> 
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/video/backlight/pwm_bl.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 1/2] backlight: pwm_bl: Configure pwm only once per backlight toggle

[Lee Jones]

On Fri, 20 Jan 2023, Uwe Kleine-König wrote:

> When the function pwm_backlight_update_status() was called with
> brightness > 0, pwm_get_state() was called twice (once directly and once
> in compute_duty_cycle). Also pwm_apply_state() was called twice (once in
> pwm_backlight_power_on() and once directly).
> 
> Optimize this to do both calls only once.
> 
> Note that with this affects the order of regulator and PWM setup. It's
> not expected to have a relevant effect on hardware. The rationale for
> this is that the regulator (and the GPIO) are reasonable to switch in
> pwm_backlight_power_on()/pwm_backlight_power_off() but the PWM has
> nothing to do with power. (The post_pwm_on_delay and pwm_off_delay are
> still there though.)
> 
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/video/backlight/pwm_bl.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

[PATCH v2 2/2] backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive state

[Uwe Kleine-König]

Most but not all PWMs drive the PWM pin to its inactive state when
disabled. However if there is no enable_gpio and no regulator the PWM
must drive the inactive state to actually disable the backlight.

So keep the PWM on in this case.

Note that to determine if there is a regulator some effort is required
because it might happen that there isn't actually one but the regulator
core gave us a dummy. (A nice side effect is that this makes the
regulator actually optional even on fully constrained systems.)

This fixes backlight disabling e.g. on i.MX6 when an inverted PWM is
used.

Hint for the future: If this change results in a regression, the bug is
in the lowlevel PWM driver.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/video/backlight/pwm_bl.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 0509fecd5715..fb388148d98f 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -45,9 +45,11 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb)
 	if (pb->enabled)
 		return;
 
-	err = regulator_enable(pb->power_supply);
-	if (err < 0)
-		dev_err(pb->dev, "failed to enable power supply\n");
+	if (pb->power_supply) {
+		err = regulator_enable(pb->power_supply);
+		if (err < 0)
+			dev_err(pb->dev, "failed to enable power supply\n");
+	}
 
 	if (pb->post_pwm_on_delay)
 		msleep(pb->post_pwm_on_delay);
@@ -69,7 +71,8 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
 	if (pb->pwm_off_delay)
 		msleep(pb->pwm_off_delay);
 
-	regulator_disable(pb->power_supply);
+	if (pb->power_supply)
+		regulator_disable(pb->power_supply);
 	pb->enabled = false;
 }
 
@@ -109,8 +112,16 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 		pwm_backlight_power_off(pb);
 
 		pwm_get_state(pb->pwm, &state);
-		state.enabled = false;
 		state.duty_cycle = 0;
+		/*
+		 * We cannot assume a disabled PWM to drive its output to the
+		 * inactive state. If we have an enable GPIO and/or a regulator
+		 * we assume that this isn't relevant and we can disable the PWM
+		 * to save power. If however there is neither an enable GPIO nor
+		 * a regulator keep the PWM on be sure to get a constant
+		 * inactive output.
+		 */
+		state.enabled = !pb->power_supply && !pb->enable_gpio;
 		pwm_apply_state(pb->pwm, &state);
 	}
 
@@ -408,7 +419,7 @@ static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb)
 	if (pb->enable_gpio && gpiod_get_value_cansleep(pb->enable_gpio) == 0)
 		active = false;
 
-	if (!regulator_is_enabled(pb->power_supply))
+	if (pb->power_supply && !regulator_is_enabled(pb->power_supply))
 		active = false;
 
 	if (!pwm_is_enabled(pb->pwm))
@@ -489,10 +500,13 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		goto err_alloc;
 	}
 
-	pb->power_supply = devm_regulator_get(&pdev->dev, "power");
+	pb->power_supply = devm_regulator_get_optional(&pdev->dev, "power");
 	if (IS_ERR(pb->power_supply)) {
 		ret = PTR_ERR(pb->power_supply);
-		goto err_alloc;
+		if (ret == -ENODEV)
+			pb->power_supply = NULL;
+		else
+			goto err_alloc;
 	}
 
 	pb->pwm = devm_pwm_get(&pdev->dev, NULL);
-- 
2.39.0

[PATCH v2 2/2] backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive state

[Uwe Kleine-König]

Most but not all PWMs drive the PWM pin to its inactive state when
disabled. However if there is no enable_gpio and no regulator the PWM
must drive the inactive state to actually disable the backlight.

So keep the PWM on in this case.

Note that to determine if there is a regulator some effort is required
because it might happen that there isn't actually one but the regulator
core gave us a dummy. (A nice side effect is that this makes the
regulator actually optional even on fully constrained systems.)

This fixes backlight disabling e.g. on i.MX6 when an inverted PWM is
used.

Hint for the future: If this change results in a regression, the bug is
in the lowlevel PWM driver.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/video/backlight/pwm_bl.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 0509fecd5715..fb388148d98f 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -45,9 +45,11 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb)
 	if (pb->enabled)
 		return;
 
-	err = regulator_enable(pb->power_supply);
-	if (err < 0)
-		dev_err(pb->dev, "failed to enable power supply\n");
+	if (pb->power_supply) {
+		err = regulator_enable(pb->power_supply);
+		if (err < 0)
+			dev_err(pb->dev, "failed to enable power supply\n");
+	}
 
 	if (pb->post_pwm_on_delay)
 		msleep(pb->post_pwm_on_delay);
@@ -69,7 +71,8 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
 	if (pb->pwm_off_delay)
 		msleep(pb->pwm_off_delay);
 
-	regulator_disable(pb->power_supply);
+	if (pb->power_supply)
+		regulator_disable(pb->power_supply);
 	pb->enabled = false;
 }
 
@@ -109,8 +112,16 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 		pwm_backlight_power_off(pb);
 
 		pwm_get_state(pb->pwm, &state);
-		state.enabled = false;
 		state.duty_cycle = 0;
+		/*
+		 * We cannot assume a disabled PWM to drive its output to the
+		 * inactive state. If we have an enable GPIO and/or a regulator
+		 * we assume that this isn't relevant and we can disable the PWM
+		 * to save power. If however there is neither an enable GPIO nor
+		 * a regulator keep the PWM on be sure to get a constant
+		 * inactive output.
+		 */
+		state.enabled = !pb->power_supply && !pb->enable_gpio;
 		pwm_apply_state(pb->pwm, &state);
 	}
 
@@ -408,7 +419,7 @@ static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb)
 	if (pb->enable_gpio && gpiod_get_value_cansleep(pb->enable_gpio) == 0)
 		active = false;
 
-	if (!regulator_is_enabled(pb->power_supply))
+	if (pb->power_supply && !regulator_is_enabled(pb->power_supply))
 		active = false;
 
 	if (!pwm_is_enabled(pb->pwm))
@@ -489,10 +500,13 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		goto err_alloc;
 	}
 
-	pb->power_supply = devm_regulator_get(&pdev->dev, "power");
+	pb->power_supply = devm_regulator_get_optional(&pdev->dev, "power");
 	if (IS_ERR(pb->power_supply)) {
 		ret = PTR_ERR(pb->power_supply);
-		goto err_alloc;
+		if (ret == -ENODEV)
+			pb->power_supply = NULL;
+		else
+			goto err_alloc;
 	}
 
 	pb->pwm = devm_pwm_get(&pdev->dev, NULL);
-- 
2.39.0

Re: [PATCH v2 2/2] backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive state

[Daniel Thompson]

On Fri, Jan 20, 2023 at 01:00:18PM +0100, Uwe Kleine-König wrote:
> Most but not all PWMs drive the PWM pin to its inactive state when
> disabled. However if there is no enable_gpio and no regulator the PWM
> must drive the inactive state to actually disable the backlight.
>
> So keep the PWM on in this case.
>
> Note that to determine if there is a regulator some effort is required
> because it might happen that there isn't actually one but the regulator
> core gave us a dummy. (A nice side effect is that this makes the
> regulator actually optional even on fully constrained systems.)
>
> This fixes backlight disabling e.g. on i.MX6 when an inverted PWM is
> used.
>
> Hint for the future: If this change results in a regression, the bug is
> in the lowlevel PWM driver.

:-)


> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks for the changes. Looks good:
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

Re: [PATCH v2 2/2] backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive state

[Daniel Thompson]

On Fri, Jan 20, 2023 at 01:00:18PM +0100, Uwe Kleine-König wrote:
> Most but not all PWMs drive the PWM pin to its inactive state when
> disabled. However if there is no enable_gpio and no regulator the PWM
> must drive the inactive state to actually disable the backlight.
>
> So keep the PWM on in this case.
>
> Note that to determine if there is a regulator some effort is required
> because it might happen that there isn't actually one but the regulator
> core gave us a dummy. (A nice side effect is that this makes the
> regulator actually optional even on fully constrained systems.)
>
> This fixes backlight disabling e.g. on i.MX6 when an inverted PWM is
> used.
>
> Hint for the future: If this change results in a regression, the bug is
> in the lowlevel PWM driver.

:-)


> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks for the changes. Looks good:
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

Re: [PATCH v2 2/2] backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive state

[Lee Jones]

On Fri, 20 Jan 2023, Uwe Kleine-König wrote:

> Most but not all PWMs drive the PWM pin to its inactive state when
> disabled. However if there is no enable_gpio and no regulator the PWM
> must drive the inactive state to actually disable the backlight.
> 
> So keep the PWM on in this case.
> 
> Note that to determine if there is a regulator some effort is required
> because it might happen that there isn't actually one but the regulator
> core gave us a dummy. (A nice side effect is that this makes the
> regulator actually optional even on fully constrained systems.)
> 
> This fixes backlight disabling e.g. on i.MX6 when an inverted PWM is
> used.
> 
> Hint for the future: If this change results in a regression, the bug is
> in the lowlevel PWM driver.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/video/backlight/pwm_bl.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 2/2] backlight: pwm_bl: Don't rely on a disabled PWM emiting inactive state

[Lee Jones]

On Fri, 20 Jan 2023, Uwe Kleine-König wrote:

> Most but not all PWMs drive the PWM pin to its inactive state when
> disabled. However if there is no enable_gpio and no regulator the PWM
> must drive the inactive state to actually disable the backlight.
> 
> So keep the PWM on in this case.
> 
> Note that to determine if there is a regulator some effort is required
> because it might happen that there isn't actually one but the regulator
> core gave us a dummy. (A nice side effect is that this makes the
> regulator actually optional even on fully constrained systems.)
> 
> This fixes backlight disabling e.g. on i.MX6 when an inverted PWM is
> used.
> 
> Hint for the future: If this change results in a regression, the bug is
> in the lowlevel PWM driver.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/video/backlight/pwm_bl.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 1/2] backlight: pwm_bl: Configure pwm only once per backlight toggle

[Aisheng Dong]

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

On Fri, Jan 20, 2023 at 01:00:18PM +0100, Uwe Kleine-König wrote:
> Most but not all PWMs drive the PWM pin to its inactive state when
> disabled. However if there is no enable_gpio and no regulator the PWM
> must drive the inactive state to actually disable the backlight.
>
> So keep the PWM on in this case.
>
> Note that to determine if there is a regulator some effort is required
> because it might happen that there isn't actually one but the regulator
> core gave us a dummy. (A nice side effect is that this makes the
> regulator actually optional even on fully constrained systems.)
>
> This fixes backlight disabling e.g. on i.MX6 when an inverted PWM is
> used.
>
> Hint for the future: If this change results in a regression, the bug is
> in the lowlevel PWM driver.
> Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de<mailto:u.kleine-koenig@pengutronix.de>

This patch broke imx7ulp suspend resume and got the following error:
Freezing remaining freezable tasks
Freezing remaining freezable tasks completed (elapsed 0.001 seconds)
printk: Suspending console(s) (use no_console_suspend to debug)
asix 1-1:1.0 eth0: Link is Down
imx7ulp-tpm-pwm 40250000.pwm: PM: dpm_run_callback(): platform_pm_suspend+0x0/0x48 returns -16
imx7ulp-tpm-pwm 40250000.pwm: PM: failed to suspend: error -16
...

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?

Regards
Aisheng

[-- Attachment #2: Type: text/html, Size: 5796 bytes --]