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@xxxxxxxxxxxxxxxxx> > > 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 -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | _______________________________________________ Linux-rockchip mailing list Linux-rockchip@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/linux-rockchip
