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

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

[Jeff LaBundy]

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;
+	}
 
 err_mutex:
 	mutex_unlock(&iqs620_pwm->lock);
-- 
2.7.4

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2282 bytes --]

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.

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.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jeff LaBundy]

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

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3501 bytes --]

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;

?

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jeff LaBundy]

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