Re: [PATCH v3 2/6] pwm: let pwm_get_state() return the last implemented state

["Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

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-rXY34ruvC2xidJT2blvkqNi2O/JbrIOy@public.gmane.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

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