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

[Jeff LaBundy <jeff@labundy.com>]

Hi Uwe,

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.

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

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

Kind regards,
Jeff LaBundy