Message ID | 20201127141403.3433678-1-u.kleine-koenig@pengutronix.de |
---|---|
State | Changes Requested |
Headers | show |
Series | pwm: iqs620a: Fix overflow and optimize calculations | expand |
Hi Uwe, I tested this patch on actual hardware but the newly calculated register values are incorrect. We used to get: duty_cycle reg. 0xd2 bit 7 reg. 0xd8 pulse width ---------- --------------- --------- ---------------- 1000 0 N/A constant off 4000 1 0x00 3900 us 8000 1 0x01 7800 us 1000000 1 0xff constant on Now we get: duty_cycle reg. 0xd2 bit 7 reg. 0xd8 pulse width ---------- --------------- --------- ---------------- 1000 0 N/A constant off 4000 1 0x01 7800 us (x) 8000 1 0x02 11800 us (x) 1000000 1 0xff constant on A comment below. On Fri, Nov 27, 2020 at 03:14:04PM +0100, Uwe Kleine-König wrote: > If state->duty_cycle is 0x100000000000000, the previous calculation of > duty_scale overflows and yields a duty cycle ratio of 0% instead of > 100%. Fix this by comparing the requested duty cycle against the maximal > possible duty cycle first. This way it is possible to use a native > integer division instead of a (depending on the architecture) more > expensive 64bit division. Also duty_val cannot be bigger than 0xff which > allows to simplify the code a bit further down. > > Fixes: 6f0841a8197b ("pwm: Add support for Azoteq IQS620A PWM generator") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello, > > even though this is a fix, I don't consider it critical enough to apply > it before v5.10. > > Best regards > Uwe > > drivers/pwm/pwm-iqs620a.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c > index 7d33e3646436..0c9e2a876a05 100644 > --- a/drivers/pwm/pwm-iqs620a.c > +++ b/drivers/pwm/pwm-iqs620a.c > @@ -46,7 +46,7 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > { > struct iqs620_pwm_private *iqs620_pwm; > struct iqs62x_core *iqs62x; > - u64 duty_scale; > + u8 duty_val; > int ret; > > if (state->polarity != PWM_POLARITY_NORMAL) > @@ -70,20 +70,22 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > * For lower duty cycles (e.g. 0), the PWM output is simply disabled to > * allow an external pull-down resistor to hold the GPIO3/LTX pin low. > */ > - duty_scale = div_u64(state->duty_cycle * 256, IQS620_PWM_PERIOD_NS); > + > + if (state->duty_cycle < IQS620_PWM_PERIOD_NS) > + duty_val = ((unsigned int)state->duty_cycle * 256) / IQS620_PWM_PERIOD_NS; > + else > + duty_val = 0xff; > > mutex_lock(&iqs620_pwm->lock); > > - if (!state->enabled || !duty_scale) { > + if (!state->enabled || !duty_val) { > ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS, > IQS620_PWR_SETTINGS_PWM_OUT, 0); > if (ret) > goto err_mutex; > } > > - if (duty_scale) { > - u8 duty_val = min_t(u64, duty_scale - 1, 0xff); > - > + if (duty_val) { This is part of the problem; the device's formula for duty cycle has a plus one that is getting dropped now (see comments in iqs620_pwm_apply). But since you're clamping duty_val to 0xff ahead of the zero check now, simply writing duty_val - 1 instead of duty_val would prevent you from getting constant-on. > ret = regmap_write(iqs62x->regmap, IQS620_PWM_DUTY_CYCLE, > duty_val); > if (ret) > @@ -92,7 +94,7 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > iqs620_pwm->duty_val = duty_val; > } > > - if (state->enabled && duty_scale) { > + if (state->enabled && duty_val) { > ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS, > IQS620_PWR_SETTINGS_PWM_OUT, 0xff); > if (ret) > -- > 2.29.2 > Kind regards, Jeff LaBundy
diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c index 7d33e3646436..0c9e2a876a05 100644 --- a/drivers/pwm/pwm-iqs620a.c +++ b/drivers/pwm/pwm-iqs620a.c @@ -46,7 +46,7 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, { struct iqs620_pwm_private *iqs620_pwm; struct iqs62x_core *iqs62x; - u64 duty_scale; + u8 duty_val; int ret; if (state->polarity != PWM_POLARITY_NORMAL) @@ -70,20 +70,22 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, * For lower duty cycles (e.g. 0), the PWM output is simply disabled to * allow an external pull-down resistor to hold the GPIO3/LTX pin low. */ - duty_scale = div_u64(state->duty_cycle * 256, IQS620_PWM_PERIOD_NS); + + if (state->duty_cycle < IQS620_PWM_PERIOD_NS) + duty_val = ((unsigned int)state->duty_cycle * 256) / IQS620_PWM_PERIOD_NS; + else + duty_val = 0xff; mutex_lock(&iqs620_pwm->lock); - if (!state->enabled || !duty_scale) { + if (!state->enabled || !duty_val) { ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS, IQS620_PWR_SETTINGS_PWM_OUT, 0); if (ret) goto err_mutex; } - if (duty_scale) { - u8 duty_val = min_t(u64, duty_scale - 1, 0xff); - + if (duty_val) { ret = regmap_write(iqs62x->regmap, IQS620_PWM_DUTY_CYCLE, duty_val); if (ret) @@ -92,7 +94,7 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, iqs620_pwm->duty_val = duty_val; } - if (state->enabled && duty_scale) { + if (state->enabled && duty_val) { ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS, IQS620_PWR_SETTINGS_PWM_OUT, 0xff); if (ret)
If state->duty_cycle is 0x100000000000000, the previous calculation of duty_scale overflows and yields a duty cycle ratio of 0% instead of 100%. Fix this by comparing the requested duty cycle against the maximal possible duty cycle first. This way it is possible to use a native integer division instead of a (depending on the architecture) more expensive 64bit division. Also duty_val cannot be bigger than 0xff which allows to simplify the code a bit further down. Fixes: 6f0841a8197b ("pwm: Add support for Azoteq IQS620A PWM generator") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Hello, even though this is a fix, I don't consider it critical enough to apply it before v5.10. Best regards Uwe drivers/pwm/pwm-iqs620a.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)