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

[PATCH 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.

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)

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

[PATCH 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.

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)

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

[PATCH 2/2] backlight: pwm_bl: Don't disable the PWM to disable the backlight

[Uwe Kleine-König]

Most but not all PWMs drive the PWM pin to its inactive state when
disabled. Rely on the lowlevel PWM implementation to implement
duty_cycle = 0 in an energy efficient way and don't disable the PWM.

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

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

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 0509fecd5715..7bdc5d570a12 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -109,7 +109,7 @@ 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.enabled = true;
 		state.duty_cycle = 0;
 		pwm_apply_state(pb->pwm, &state);
 	}
-- 
2.39.0

[PATCH 2/2] backlight: pwm_bl: Don't disable the PWM to disable the backlight

[Uwe Kleine-König]

Most but not all PWMs drive the PWM pin to its inactive state when
disabled. Rely on the lowlevel PWM implementation to implement
duty_cycle = 0 in an energy efficient way and don't disable the PWM.

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

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

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 0509fecd5715..7bdc5d570a12 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -109,7 +109,7 @@ 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.enabled = true;
 		state.duty_cycle = 0;
 		pwm_apply_state(pb->pwm, &state);
 	}
-- 
2.39.0

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

[Daniel Thompson]

On Mon, Jan 09, 2023 at 09:47:57PM +0100, 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.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

This will reverse the order in which the regulator is toggled versus the
PWM starting/stopping. It would be nice to that in the description.

However I can't see why it would be a problem (since both remain in the
same place relative to the sleeps) so:
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

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

[Daniel Thompson]

On Mon, Jan 09, 2023 at 09:47:57PM +0100, 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.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

This will reverse the order in which the regulator is toggled versus the
PWM starting/stopping. It would be nice to that in the description.

However I can't see why it would be a problem (since both remain in the
same place relative to the sleeps) so:
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

Re: [PATCH 2/2] backlight: pwm_bl: Don't disable the PWM to disable the backlight

[Daniel Thompson]

On Mon, Jan 09, 2023 at 09:47:58PM +0100, Uwe Kleine-König wrote:
> Most but not all PWMs drive the PWM pin to its inactive state when
> disabled. Rely on the lowlevel PWM implementation to implement
> duty_cycle = 0 in an energy efficient way and don't disable the PWM.

I'm a little worried about this one.

I thought the PWM APIs allow the duty cycle to be rounded up or down
slightly during the apply.

So when you say "rely on the lowlevel to implement duty_cycle = 0 to..."
is it confirmed that this is true (and that all PWMs *can* implement
a duty_cycle of 0 without rounding up)?


Daniel.


> This fixes backlight disabling e.g. on i.MX6 when an inverted PWM is
> used.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/video/backlight/pwm_bl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 0509fecd5715..7bdc5d570a12 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -109,7 +109,7 @@ 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.enabled = true;
>  		state.duty_cycle = 0;
>  		pwm_apply_state(pb->pwm, &state);
>  	}
> --
> 2.39.0
>

Re: [PATCH 2/2] backlight: pwm_bl: Don't disable the PWM to disable the backlight

[Daniel Thompson]

On Mon, Jan 09, 2023 at 09:47:58PM +0100, Uwe Kleine-König wrote:
> Most but not all PWMs drive the PWM pin to its inactive state when
> disabled. Rely on the lowlevel PWM implementation to implement
> duty_cycle = 0 in an energy efficient way and don't disable the PWM.

I'm a little worried about this one.

I thought the PWM APIs allow the duty cycle to be rounded up or down
slightly during the apply.

So when you say "rely on the lowlevel to implement duty_cycle = 0 to..."
is it confirmed that this is true (and that all PWMs *can* implement
a duty_cycle of 0 without rounding up)?


Daniel.


> This fixes backlight disabling e.g. on i.MX6 when an inverted PWM is
> used.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/video/backlight/pwm_bl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 0509fecd5715..7bdc5d570a12 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -109,7 +109,7 @@ 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.enabled = true;
>  		state.duty_cycle = 0;
>  		pwm_apply_state(pb->pwm, &state);
>  	}
> --
> 2.39.0
>

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

[Uwe Kleine-König]

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

Hello Daniel,

On Tue, Jan 10, 2023 at 04:17:16PM +0000, Daniel Thompson wrote:
> On Mon, Jan 09, 2023 at 09:47:57PM +0100, 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.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> This will reverse the order in which the regulator is toggled versus the
> PWM starting/stopping. It would be nice to that in the description.

Oh, that wasn't a concious choice. I agree this should be noted. The
current state is also a bit confused because the duty cycle is setup
before the regulator but the PWM only gets enabled afterwards.

Expect a v2 with an updated commit log.

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 2/2] backlight: pwm_bl: Don't disable the PWM to disable the backlight

[Uwe Kleine-König]

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

Hello Daniel,

On Tue, Jan 10, 2023 at 04:26:14PM +0000, Daniel Thompson wrote:
> On Mon, Jan 09, 2023 at 09:47:58PM +0100, Uwe Kleine-König wrote:
> > Most but not all PWMs drive the PWM pin to its inactive state when
> > disabled. Rely on the lowlevel PWM implementation to implement
> > duty_cycle = 0 in an energy efficient way and don't disable the PWM.
> 
> I'm a little worried about this one.
> 
> I thought the PWM APIs allow the duty cycle to be rounded up or down
> slightly during the apply.

In my book only rounding down is correct, but in practise there is some
deviation.

Nearly all PWMs can implement a zero duty cycle. Those that cannot but
emit a constant inactive signal when disabled are expected to disable
when .duty_cycle = 0 is requested. (And for those that can neither
implement a zero duty_cycle nor emit the inactive level (not sure there
is any) all bets are lost with and without my patch.)
So if this case will be hit (and noticed) this is fixable.

However there are hardware PWMs that just freeze in their current state
when disabled (e.g. mxs). That's why .duty_cycle=0 + .enabled=true is
the safer bet. Only disable a PWM if you don't rely on the output state.
See also commit 80a22fde803af6f390be49ee5ced6ee75595ba05.

> So when you say "rely on the lowlevel to implement duty_cycle = 0 to..."
> is it confirmed that this is true (and that all PWMs *can* implement
> a duty_cycle of 0 without rounding up)?

The scenario I had in mind that can realistically go wrong here is that
a lowlevel driver that has the property that the inactive level is
emitted for a disabled HW doesn't actually disable when .duty_cycle=0 is
requested and so might consume slightly more energy. But I'm confident
my patch is an improvement and I don't expect regressions. (Famous last
words :-)

I suggest to amend the commit log and add something like:

   If this change results in a regression, the bug is in the lowlevel
   pwm driver.

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 2/2] backlight: pwm_bl: Don't disable the PWM to disable the backlight

[Daniel Thompson]

On Tue, Jan 10, 2023 at 06:35:00PM +0100, Uwe Kleine-König wrote:
> Hello Daniel,
>
> On Tue, Jan 10, 2023 at 04:26:14PM +0000, Daniel Thompson wrote:
> > On Mon, Jan 09, 2023 at 09:47:58PM +0100, Uwe Kleine-König wrote:
> > > Most but not all PWMs drive the PWM pin to its inactive state when
> > > disabled. Rely on the lowlevel PWM implementation to implement
> > > duty_cycle = 0 in an energy efficient way and don't disable the PWM.
> >
> > I'm a little worried about this one.
> >
> > I thought the PWM APIs allow the duty cycle to be rounded up or down
> > slightly during the apply.
>
> In my book only rounding down is correct, but in practise there is some
> deviation.
>
> Nearly all PWMs can implement a zero duty cycle. Those that cannot but
> emit a constant inactive signal when disabled are expected to disable
> when .duty_cycle = 0 is requested. (And for those that can neither
> implement a zero duty_cycle nor emit the inactive level (not sure there
> is any) all bets are lost with and without my patch.)
> So if this case will be hit (and noticed) this is fixable.
>
> However there are hardware PWMs that just freeze in their current state
> when disabled (e.g. mxs). That's why .duty_cycle=0 + .enabled=true is
> the safer bet. Only disable a PWM if you don't rely on the output state.
> See also commit 80a22fde803af6f390be49ee5ced6ee75595ba05.

Reading this, it does strike me that if pwm_bl has a regulator or an
enable GPIO then it does not rely on the output state. We could use
the presence of either of these to choose to disable the PWM
(which could potentially undrive the pin to save power).


> > So when you say "rely on the lowlevel to implement duty_cycle = 0 to..."
> > is it confirmed that this is true (and that all PWMs *can* implement
> > a duty_cycle of 0 without rounding up)?
>
> The scenario I had in mind that can realistically go wrong here is that
> a lowlevel driver that has the property that the inactive level is
> emitted for a disabled HW doesn't actually disable when .duty_cycle=0 is
> requested and so might consume slightly more energy. But I'm confident
> my patch is an improvement and I don't expect regressions. (Famous last
> words :-)
>
> I suggest to amend the commit log and add something like:
>
>    If this change results in a regression, the bug is in the lowlevel
>    pwm driver.

I guess I can live with that :-) .

If the reasoning about regulator or enable GPIO makes sense then let's
implement that. If not, a terse comment in the code reminding some
future version of me that disabled PWM has undefined state (making
clear that the absense of enable = false is deliberate) would be useful!


Daniel.