| Message ID | 20190824153707.13746-3-uwe@kleine-koenig.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: ensure pwm_apply_state() doesn't modify the state argument | expand |
Am Samstag, 24. August 2019, 17:37:03 CEST schrieb Uwe Kleine-König: > When pwm_apply_state() is called the lowlevel driver usually has to > apply some rounding because the hardware doesn't support nanosecond > resolution. So let pwm_get_state() return the actually implemented state > instead of the last applied one if possible. > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> With this applied, the display brightness on my rk3399-gru-scarlet gets inverted. Now it's very bright on level 1 and very dim on the max level. According to the debugfs, the inverted state changes: OLD STATE: ---------- root@localhost:~# cat /debug/pwm platform/ff420030.pwm, 1 PWM device pwm-0 (ppvar-bigcpu-pwm ): requested enabled period: 3334 ns duty: 331 ns polarity: normal platform/ff420020.pwm, 1 PWM device pwm-0 (ppvar-litcpu-pwm ): requested enabled period: 3334 ns duty: 414 ns polarity: normal platform/ff420010.pwm, 1 PWM device pwm-0 (backlight ): requested enabled period: 999996 ns duty: 941148 ns polarity: normal platform/ff420000.pwm, 1 PWM device pwm-0 (ppvar-gpu-pwm ): requested enabled period: 3334 ns duty: 3334 ns polarity: normal NEW STATE: ---------- root@localhost:~# cat /debug/pwm platform/ff420030.pwm, 1 PWM device pwm-0 (ppvar-bigcpu-pwm ): requested enabled period: 3334 ns duty: 331 ns polarity: normal platform/ff420020.pwm, 1 PWM device pwm-0 (ppvar-litcpu-pwm ): requested enabled period: 3334 ns duty: 414 ns polarity: normal platform/ff420010.pwm, 1 PWM device pwm-0 (backlight ): requested enabled period: 999996 ns duty: 941148 ns polarity: inverse platform/ff420000.pwm, 1 PWM device pwm-0 (ppvar-gpu-pwm ): requested enabled period: 3334 ns duty: 3334 ns polarity: normal And the reason is below. > --- > drivers/pwm/core.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index 72347ca4a647..92333b89bf02 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -473,7 +473,14 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) > if (err) > return err; > > - pwm->state = *state; > + /* > + * .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; This should probably become >- pwm->state = *state; > + > + /* > + * .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); so always initialize the state to the provided one and then let the driver override values? The inversion case stems from the Rockchip pwm driver (wrongly?) only setting the polarity field when actually inverted, so here the polarity field probably never actually got set at all. But while we should probably fix the rockchip driver to set polarity all the time, this is still being true for possible future state-fields, which also wouldn't get initialzed from all drivers, which might need an adaption first? Heiko > } else { > /* > * FIXME: restore the initial state in case of error. >
On Fri, Aug 30, 2019 at 07:48:35PM +0200, Heiko Stuebner wrote: > Am Samstag, 24. August 2019, 17:37:03 CEST schrieb Uwe Kleine-König: > > When pwm_apply_state() is called the lowlevel driver usually has to > > apply some rounding because the hardware doesn't support nanosecond > > resolution. So let pwm_get_state() return the actually implemented state > > instead of the last applied one if possible. > > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > > With this applied, the display brightness on my rk3399-gru-scarlet gets > inverted. Now it's very bright on level 1 and very dim on the max level. > > According to the debugfs, the inverted state changes: > > OLD STATE: > ---------- > root@localhost:~# cat /debug/pwm > platform/ff420030.pwm, 1 PWM device > pwm-0 (ppvar-bigcpu-pwm ): requested enabled period: 3334 ns duty: 331 ns polarity: normal > > platform/ff420020.pwm, 1 PWM device > pwm-0 (ppvar-litcpu-pwm ): requested enabled period: 3334 ns duty: 414 ns polarity: normal > > platform/ff420010.pwm, 1 PWM device > pwm-0 (backlight ): requested enabled period: 999996 ns duty: 941148 ns polarity: normal > > platform/ff420000.pwm, 1 PWM device > pwm-0 (ppvar-gpu-pwm ): requested enabled period: 3334 ns duty: 3334 ns polarity: normal > > NEW STATE: > ---------- > root@localhost:~# cat /debug/pwm > platform/ff420030.pwm, 1 PWM device > pwm-0 (ppvar-bigcpu-pwm ): requested enabled period: 3334 ns duty: 331 ns polarity: normal > > platform/ff420020.pwm, 1 PWM device > pwm-0 (ppvar-litcpu-pwm ): requested enabled period: 3334 ns duty: 414 ns polarity: normal > > platform/ff420010.pwm, 1 PWM device > pwm-0 (backlight ): requested enabled period: 999996 ns duty: 941148 ns polarity: inverse > > platform/ff420000.pwm, 1 PWM device > pwm-0 (ppvar-gpu-pwm ): requested enabled period: 3334 ns duty: 3334 ns polarity: normal > > > And the reason is below. > > > --- > > drivers/pwm/core.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > > index 72347ca4a647..92333b89bf02 100644 > > --- a/drivers/pwm/core.c > > +++ b/drivers/pwm/core.c > > @@ -473,7 +473,14 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) > > if (err) > > return err; > > > > - pwm->state = *state; > > + /* > > + * .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; > > This should probably become > >- pwm->state = *state; > > + > > + /* > > + * .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); > > so always initialize the state to the provided one and then let the driver > override values? This feels wrong. There is another thread that adds a developer knob that emits some warnings. Catching this kind of error with it would be a good idea IMHO. > The inversion case stems from the Rockchip pwm driver (wrongly?) only > setting the polarity field when actually inverted, so here the polarity field > probably never actually got set at all. > > But while we should probably fix the rockchip driver to set polarity all the > time, this is still being true for possible future state-fields, which also > wouldn't get initialzed from all drivers, which might need an adaption > first? Actually I would prefer that all drivers do it right and rely on this. Thanks for testing Uwe
On Mon, Sep 02, 2019 at 04:32:31PM +0200, Uwe Kleine-König wrote: > On Fri, Aug 30, 2019 at 07:48:35PM +0200, Heiko Stuebner wrote: > > Am Samstag, 24. August 2019, 17:37:03 CEST schrieb Uwe Kleine-König: > > > --- > > > drivers/pwm/core.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > > > index 72347ca4a647..92333b89bf02 100644 > > > --- a/drivers/pwm/core.c > > > +++ b/drivers/pwm/core.c > > > @@ -473,7 +473,14 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) > > > if (err) > > > return err; > > > > > > - pwm->state = *state; > > > + /* > > > + * .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; > > > > This should probably become > > >- pwm->state = *state; > > > + > > > + /* > > > + * .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); > > > > so always initialize the state to the provided one and then let the driver > > override values? > > This feels wrong. There is another thread that adds a developer knob > that emits some warnings. Catching this kind of error with it would be a > good idea IMHO. > > > The inversion case stems from the Rockchip pwm driver (wrongly?) only > > setting the polarity field when actually inverted, so here the polarity field > > probably never actually got set at all. > > > > But while we should probably fix the rockchip driver to set polarity all the > > time, this is still being true for possible future state-fields, which also > > wouldn't get initialzed from all drivers, which might need an adaption > > first? > > Actually I would prefer that all drivers do it right and rely on this. FTR: I sent a patch for the rockchip driver to do the right thing here. Best regards Uwe
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 72347ca4a647..92333b89bf02 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -473,7 +473,14 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) if (err) return err; - pwm->state = *state; + /* + * .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; } else { /* * FIXME: restore the initial state in case of error.
When pwm_apply_state() is called the lowlevel driver usually has to apply some rounding because the hardware doesn't support nanosecond resolution. So let pwm_get_state() return the actually implemented state instead of the last applied one if possible. Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> --- drivers/pwm/core.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)