[PATCH] Revert "pwm: Let pwm_get_state() return the last implemented state"

[PATCH] Revert "pwm: Let pwm_get_state() return the last implemented state"

[Thierry Reding]

It turns out that commit 01ccf903edd6 ("pwm: Let pwm_get_state() return
the last implemented state") causes backlight failures on a number of
boards. The reason is that some of the drivers do not write the full
state through to the hardware registers, which means that ->get_state()
subsequently does not return the correct state. Consumers which rely on
pwm_get_state() returning the current state will therefore get confused
and subsequently try to program a bad state.

Before this change can be made, existing drivers need to be more
carefully audited and fixed to behave as the framework expects. Until
then, keep the original behaviour of returning the software state that
was applied rather than reading the state back from hardware.

Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
---
 drivers/pwm/core.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 6ad51aa60c03..f877e77d9184 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -472,14 +472,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 		if (err)
 			return err;
 
-		/*
-		 * .apply might have to round some values in *state, if possible
-		 * read the actually implemented value back.
-		 */
-		if (chip->ops->get_state)
-			chip->ops->get_state(chip, pwm, &pwm->state);
-		else
-			pwm->state = *state;
+		pwm->state = *state;
 	} else {
 		/*
 		 * FIXME: restore the initial state in case of error.
-- 
2.23.0

Re: [PATCH] Revert "pwm: Let pwm_get_state() return the last implemented state"

[Uwe Kleine-König]

On Mon, Oct 21, 2019 at 12:58:30PM +0200, Thierry Reding wrote:
> It turns out that commit 01ccf903edd6 ("pwm: Let pwm_get_state() return
> the last implemented state") causes backlight failures on a number of
> boards. The reason is that some of the drivers do not write the full
> state through to the hardware registers, which means that ->get_state()
> subsequently does not return the correct state. Consumers which rely on
> pwm_get_state() returning the current state will therefore get confused
> and subsequently try to program a bad state.
> 
> Before this change can be made, existing drivers need to be more
> carefully audited and fixed to behave as the framework expects. Until
> then, keep the original behaviour of returning the software state that
> was applied rather than reading the state back from hardware.

I would really prefer to fix that in the framework instead. This is
something that affects several drivers (cros-ec, imx27, atmel, imx-tpm,
lpss, meson, sifive, sprd and stm32-lp). This is im my eyes really
sufficient to justify a framework wide solution instead of adapting
several drivers in a way that doesn't affect the values programmed to
hardware.

> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>

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

Thanks
Uwe

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

Re: [PATCH] Revert "pwm: Let pwm_get_state() return the last implemented state"

[Thierry Reding]

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

On Mon, Oct 21, 2019 at 01:18:47PM +0200, Uwe Kleine-König wrote:
> On Mon, Oct 21, 2019 at 12:58:30PM +0200, Thierry Reding wrote:
> > It turns out that commit 01ccf903edd6 ("pwm: Let pwm_get_state() return
> > the last implemented state") causes backlight failures on a number of
> > boards. The reason is that some of the drivers do not write the full
> > state through to the hardware registers, which means that ->get_state()
> > subsequently does not return the correct state. Consumers which rely on
> > pwm_get_state() returning the current state will therefore get confused
> > and subsequently try to program a bad state.
> > 
> > Before this change can be made, existing drivers need to be more
> > carefully audited and fixed to behave as the framework expects. Until
> > then, keep the original behaviour of returning the software state that
> > was applied rather than reading the state back from hardware.
> 
> I would really prefer to fix that in the framework instead. This is

There's nothing to fix in the framework. The framework isn't broken, the
drivers are.

> something that affects several drivers (cros-ec, imx27, atmel, imx-tpm,
> lpss, meson, sifive, sprd and stm32-lp). This is im my eyes really
> sufficient to justify a framework wide solution instead of adapting
> several drivers in a way that doesn't affect the values programmed to
> hardware.

Can you come up with a proposal for how to want to implement this in the
framework?

Thierry

> > Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> 
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Thanks
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH] Revert "pwm: Let pwm_get_state() return the last implemented state"

[Uwe Kleine-König]

On Mon, Oct 21, 2019 at 04:17:21PM +0200, Thierry Reding wrote:
> On Mon, Oct 21, 2019 at 01:18:47PM +0200, Uwe Kleine-König wrote:
> > On Mon, Oct 21, 2019 at 12:58:30PM +0200, Thierry Reding wrote:
> > > It turns out that commit 01ccf903edd6 ("pwm: Let pwm_get_state() return
> > > the last implemented state") causes backlight failures on a number of
> > > boards. The reason is that some of the drivers do not write the full
> > > state through to the hardware registers, which means that ->get_state()
> > > subsequently does not return the correct state. Consumers which rely on
> > > pwm_get_state() returning the current state will therefore get confused
> > > and subsequently try to program a bad state.
> > > 
> > > Before this change can be made, existing drivers need to be more
> > > carefully audited and fixed to behave as the framework expects. Until
> > > then, keep the original behaviour of returning the software state that
> > > was applied rather than reading the state back from hardware.
> > 
> > I would really prefer to fix that in the framework instead. This is
> 
> There's nothing to fix in the framework. The framework isn't broken, the
> drivers are.

The drivers behave in a way that result in unexpected behaviour for
consumers. There are several ways to fix this, adapting the drivers to
the consumer's expectations is only one of them.

> > something that affects several drivers (cros-ec, imx27, atmel, imx-tpm,
> > lpss, meson, sifive, sprd and stm32-lp). This is im my eyes really
> > sufficient to justify a framework wide solution instead of adapting
> > several drivers in a way that doesn't affect the values programmed to
> > hardware.
> 
> Can you come up with a proposal for how to want to implement this in the
> framework?

My preferred solution is to make consumers not expect that
pwm_get_state() keeps .duty_cycle and .period for disabled PWMs. For
that consumers must be moved away from the idiom:

	pwm_get_state(mypwm, &state);
	state.enabled = true;
	pwm_apply_state(mypwm, &state);

IMHO it is very artificial from a lowlevel driver's POV to differentiate
between

	.period = 1000000, .duty_cycle = 50000, .enabled = false

and

	.period = 100, .duty_cycle = 0, .enabled = false

because the respective expected behaviour is completely identical (apart
from the requirement under discussion). So I'd do something like:

	if (!state->enabled) {
		state->period = 1;
		state->duty_cycle = 0;
	}

after calling the driver's .get_state callback once all in-tree
consumers are converted to provide a uniform behaviour to consumers.

The next best solution is to cache the values for .period and
.duty_cycle for disabled PWMs only. But the ugly detail then is that
pwm_get_state() sometimes returns actually implemented values and
sometimes requested values. (The same holds true for the approach to
make lowlevel drivers cache these values.)

So it makes probably more sense to introduce two new functions

	pwm_get_applied_state()
	pwm_get_implemented_state()

with the first having the semantic of pwm_get_state() before
01ccf903edd6 and the latter of after 01ccf903edd6. Then today's users of
pwm_get_state can be converted to the right of these two. But I'm not
convinced this is a good idea now. Maybe we should first clean up the
already started stuff (like converting lowlevel drivers and consumers to
the atomic API). Maybe renaming pwm_get_state to pwm_get_applied_state
is something that is easy enough to justify already today?

Best regards
Uwe

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

Re: [PATCH] Revert "pwm: Let pwm_get_state() return the last implemented state"

[Enric Balletbo i Serra]

On 21/10/19 12:58, Thierry Reding wrote:
> It turns out that commit 01ccf903edd6 ("pwm: Let pwm_get_state() return
> the last implemented state") causes backlight failures on a number of
> boards. The reason is that some of the drivers do not write the full
> state through to the hardware registers, which means that ->get_state()
> subsequently does not return the correct state. Consumers which rely on
> pwm_get_state() returning the current state will therefore get confused
> and subsequently try to program a bad state.
> 
> Before this change can be made, existing drivers need to be more
> carefully audited and fixed to behave as the framework expects. Until
> then, keep the original behaviour of returning the software state that
> was applied rather than reading the state back from hardware.
> 
> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>

Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Thanks,
 Enric.

> ---
>  drivers/pwm/core.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 6ad51aa60c03..f877e77d9184 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -472,14 +472,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
>  		if (err)
>  			return err;
>  
> -		/*
> -		 * .apply might have to round some values in *state, if possible
> -		 * read the actually implemented value back.
> -		 */
> -		if (chip->ops->get_state)
> -			chip->ops->get_state(chip, pwm, &pwm->state);
> -		else
> -			pwm->state = *state;
> +		pwm->state = *state;
>  	} else {
>  		/*
>  		 * FIXME: restore the initial state in case of error.
> 

Re: [PATCH] Revert "pwm: Let pwm_get_state() return the last implemented state"

[Michal Vokáč]

On 21. 10. 19 12:58, Thierry Reding wrote:
> It turns out that commit 01ccf903edd6 ("pwm: Let pwm_get_state() return
> the last implemented state") causes backlight failures on a number of
> boards. The reason is that some of the drivers do not write the full
> state through to the hardware registers, which means that ->get_state()
> subsequently does not return the correct state. Consumers which rely on
> pwm_get_state() returning the current state will therefore get confused
> and subsequently try to program a bad state.
> 
> Before this change can be made, existing drivers need to be more
> carefully audited and fixed to behave as the framework expects. Until
> then, keep the original behaviour of returning the software state that
> was applied rather than reading the state back from hardware.

Backlight on our imx6dl-yapp4-draco board works fine again when
this is reverted.

Tested-by: Michal Vokáč <michal.vokac@ysoft.com>

> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> ---
>   drivers/pwm/core.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 6ad51aa60c03..f877e77d9184 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -472,14 +472,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
>   		if (err)
>   			return err;
>   
> -		/*
> -		 * .apply might have to round some values in *state, if possible
> -		 * read the actually implemented value back.
> -		 */
> -		if (chip->ops->get_state)
> -			chip->ops->get_state(chip, pwm, &pwm->state);
> -		else
> -			pwm->state = *state;
> +		pwm->state = *state;
>   	} else {
>   		/*
>   		 * FIXME: restore the initial state in case of error.
>