[v3,2/6] pwm: let pwm_get_state() return the last implemented state
diff mbox series

Message ID 20190824153707.13746-3-uwe@kleine-koenig.org
State New
Headers show
Series
  • pwm: ensure pwm_apply_state() doesn't modify the state argument
Related show

Commit Message

Uwe Kleine-König Aug. 24, 2019, 3:37 p.m. UTC
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(-)

Comments

Heiko Stuebner Aug. 30, 2019, 5:48 p.m. UTC | #1
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.
>
Uwe Kleine-König Sept. 2, 2019, 2:32 p.m. UTC | #2
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
Uwe Kleine-König Sept. 2, 2019, 8:20 p.m. UTC | #3
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

Patch
diff mbox series

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.