| Message ID | 20191021105830.1357795-1-thierry.reding@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Revert "pwm: Let pwm_get_state() return the last implemented state" | expand |
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
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. >
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. >
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/ |
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
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.
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(-)