| Message ID | 1610686834-6149-1-git-send-email-jeff@labundy.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: iqs620a: Correct a stale state variable | expand |
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
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
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
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
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);
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(-)