Re: [PATCH] pwm: iqs620a: Correct a stale state variable

[Jeff LaBundy <jeff@labundy.com>]

Hi Uwe,

On Mon, Jan 18, 2021 at 09:02:19AM +0100, Uwe Kleine-König wrote:
> Hello Jeff,
> 
> On Sun, Jan 17, 2021 at 10:30:05PM -0600, Jeff LaBundy wrote:
> > Thank you for taking a look; actually I came across this problem while
> > testing your patch, so I owe you even more gratitude :)
> 
> :-)
> 
> > On Fri, Jan 15, 2021 at 08:45:09AM +0100, Uwe Kleine-König wrote:
> > > On Thu, Jan 14, 2021 at 11:00:34PM -0600, Jeff LaBundy wrote:
> > > > If duty cycle is first set to a value that is sufficiently high to
> > > > enable the output (e.g. 10000 ns) but then lowered to a value that
> > > > is quantized to zero (e.g. 1000 ns), the output is disabled as the
> > > > device cannot drive a constant zero (as expected).
> > > > 
> > > > However if the device is later re-initialized due to watchdog bite,
> > > > the output is re-enabled at the next-to-last duty cycle (10000 ns).
> > > > This is because the iqs620_pwm->out_en flag unconditionally tracks
> > > > state->enabled instead of what was actually written to the device.
> > > > 
> > > > To solve this problem, force the iqs620_pwm->out_en flag to follow
> > > > the IQS620_PWR_SETTINGS_PWM_OUT field instead, as was the original
> > > > design intent.
> > > > 
> > > > Fixes: 6f0841a8197b ("pwm: Add support for Azoteq IQS620A PWM generator")
> > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> > > > ---
> > > >  drivers/pwm/pwm-iqs620a.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
> > > > index 5ede825..5eb8fa4 100644
> > > > --- a/drivers/pwm/pwm-iqs620a.c
> > > > +++ b/drivers/pwm/pwm-iqs620a.c
> > > > @@ -79,6 +79,8 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > >  					 IQS620_PWR_SETTINGS_PWM_OUT, 0);
> > > >  		if (ret)
> > > >  			goto err_mutex;
> > > > +
> > > > +		iqs620_pwm->out_en = false;
> > > >  	}
> > > >  
> > > >  	if (duty_scale) {
> > > > @@ -97,9 +99,9 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > >  					 IQS620_PWR_SETTINGS_PWM_OUT, 0xff);
> > > >  		if (ret)
> > > >  			goto err_mutex;
> > > > -	}
> > > >  
> > > > -	iqs620_pwm->out_en = state->enabled;
> > > > +		iqs620_pwm->out_en = true;
> > > > +	}
> > > 
> > > I got the problem and I agree it needs fixing. Are you aware you change
> > > the semantic of out_en here and so the behaviour of .get_state()? IMHO
> > > the change is fine however, and unless I miss something this patch makes
> > > the comment in iqs620_pwm_get_state true.
> > 
> > Agreed on all counts; in fact I saw this as an improvement because the
> > get_state callback now reflects the actual state of the hardware under
> > all circumstances.
> > 
> > As you mention, the comment in iqs620_pwm_get_state() is fully correct
> > now too. Previously it was incorrect in this particular corner case.
> 
> ok, I wondered if I missed something.
> 
> > > Other than that I wonder if it would make more sense to track duty_scale
> > > in the driver struct instead of duty_val and out_en.
> > 
> > You would still have to cache state->enabled because it's required for
> > decoding duty_scale at the time it was cached, so there is not much to
> > gain. I prefer this solution because the get_state callback is correct
> > across all cases now, and the change is small.
> 
> Can't we cope for this by just doing
> 
> 	if (!state->enabled)
> 		duty_scale = 0;
> 
> ?

This is an excellent idea, and simplifies the driver significantly. I'll
send a v2 shortly.

> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Kind regards,
Jeff LaBundy