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 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

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