| Message ID | 20230103231841.1548913-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: ab8500: | expand |
Hello, oops, I fatfingered the subject. Something like pwm: ab8500: Properly handle duty cycle > 1024 ns and period would make sense. I'll wait a bit for more feedback before resending. A confirmation that my guess about the constant period from someone with access to the relevant documentation would be great. Best regards Uwe
On 04.01.23 00:18, Uwe Kleine-König wrote: > The .apply() callback only considered the 10 low bits of .duty_cycle and > didn't check .period at all. > > My best guess is the period is fixed at 1024 ns = 0x400 ns. Based on > that assumption refuse configurations that request a lower period (as > usual for PWM drivers) and configure a duty cycle of 0x3ff ns for all > bigger requests. > > This improves behaviour for a few requests: > > request | previous result | new result > -----------+-----------------+------------ > 1024/1024 | 0/1024 | 1023/1024 > 4000/5000 | 928/1024 | 1023/1024 > 5000/5000 | 904/1024 | 1023/1024 > > (Values specified as duty_cycle / period in ns) > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/pwm/pwm-ab8500.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/pwm/pwm-ab8500.c b/drivers/pwm/pwm-ab8500.c > index ad37bc46f272..a7f64371449b 100644 > --- a/drivers/pwm/pwm-ab8500.c > +++ b/drivers/pwm/pwm-ab8500.c > @@ -37,6 +37,7 @@ static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > u8 reg; > unsigned int higher_val, lower_val; > struct ab8500_pwm_chip *ab8500 = ab8500_pwm_from_chip(chip); > + unsigned int duty_cycle; > > if (state->polarity != PWM_POLARITY_NORMAL) > return -EINVAL; > @@ -52,16 +53,25 @@ static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > return ret; > } > > + /* The period is fixed at 0x400 ns */ > + if (state->period < 0x400) If it's fixed, why < instead of == ? > + return -EINVAL; > + > + if (state->duty_cycle >= 0x400) > + duty_cycle = 0x3ff; > + else > + duty_cycle = state->duty_cycle; You can use duty_cycle = min(state->duty_cycle, 0x3ff); here > + > /* > * get the first 8 bits that are be written to > * AB8500_PWM_OUT_CTRL1_REG[0:7] > */ > - lower_val = state->duty_cycle & 0x00FF; > + lower_val = duty_cycle & 0x00FF; > /* > * get bits [9:10] that are to be written to > * AB8500_PWM_OUT_CTRL2_REG[0:1] > */ > - higher_val = ((state->duty_cycle & 0x0300) >> 8); > + higher_val = (duty_cycle & 0x0300) >> 8; > > reg = AB8500_PWM_OUT_CTRL1_REG + (ab8500->hwid * 2); >
On Wed, Jan 4, 2023 at 12:18 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > The .apply() callback only considered the 10 low bits of .duty_cycle and > didn't check .period at all. > > My best guess is the period is fixed at 1024 ns = 0x400 ns. Based on > that assumption refuse configurations that request a lower period (as > usual for PWM drivers) and configure a duty cycle of 0x3ff ns for all > bigger requests. > > This improves behaviour for a few requests: > > request | previous result | new result > -----------+-----------------+------------ > 1024/1024 | 0/1024 | 1023/1024 > 4000/5000 | 928/1024 | 1023/1024 > 5000/5000 | 904/1024 | 1023/1024 > > (Values specified as duty_cycle / period in ns) > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Looks correct to me: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Believe it or not but there is a hard-to-find public datasheet: https://web.archive.org/web/20130614115108/http://www.stericsson.com/developers/CD00291561_UM1031_AB8500_user_manual-rev5_CTDS_public.pdf These register bits are described on page 282-283. Yours, Linus Walleij
Hello Ahmad, On Wed, Jan 04, 2023 at 10:21:56AM +0100, Ahmad Fatoum wrote: > On 04.01.23 00:18, Uwe Kleine-König wrote: > > The .apply() callback only considered the 10 low bits of .duty_cycle and > > didn't check .period at all. > > > > My best guess is the period is fixed at 1024 ns = 0x400 ns. Based on > > that assumption refuse configurations that request a lower period (as > > usual for PWM drivers) and configure a duty cycle of 0x3ff ns for all > > bigger requests. > > > > This improves behaviour for a few requests: > > > > request | previous result | new result > > -----------+-----------------+------------ > > 1024/1024 | 0/1024 | 1023/1024 > > 4000/5000 | 928/1024 | 1023/1024 > > 5000/5000 | 904/1024 | 1023/1024 > > > > (Values specified as duty_cycle / period in ns) > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/pwm/pwm-ab8500.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pwm/pwm-ab8500.c b/drivers/pwm/pwm-ab8500.c > > index ad37bc46f272..a7f64371449b 100644 > > --- a/drivers/pwm/pwm-ab8500.c > > +++ b/drivers/pwm/pwm-ab8500.c > > @@ -37,6 +37,7 @@ static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > u8 reg; > > unsigned int higher_val, lower_val; > > struct ab8500_pwm_chip *ab8500 = ab8500_pwm_from_chip(chip); > > + unsigned int duty_cycle; > > > > if (state->polarity != PWM_POLARITY_NORMAL) > > return -EINVAL; > > @@ -52,16 +53,25 @@ static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > return ret; > > } > > > > + /* The period is fixed at 0x400 ns */ > > + if (state->period < 0x400) > > If it's fixed, why < instead of == ? The usual policy for a driver is to configure the hightest period not bigger than the requested policy. As this HW only does 0x400 ns, it implements 0x400 if the request is >= 0x400 and fails otherwise. > > + return -EINVAL; > > + > > + if (state->duty_cycle >= 0x400) > > + duty_cycle = 0x3ff; > > + else > > + duty_cycle = state->duty_cycle; > > You can use duty_cycle = min(state->duty_cycle, 0x3ff); here yes, (though I'd have to use duty_cycle = min_t(u64, state->duty_cycle, 0x3ff); ) but I think the original suggestion is easier to read. Best regards Uwe
Hello, On Thu, Jan 05, 2023 at 01:14:33AM +0100, Linus Walleij wrote: > On Wed, Jan 4, 2023 at 12:18 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > The .apply() callback only considered the 10 low bits of .duty_cycle and > > didn't check .period at all. > > > > My best guess is the period is fixed at 1024 ns = 0x400 ns. Based on > > that assumption refuse configurations that request a lower period (as > > usual for PWM drivers) and configure a duty cycle of 0x3ff ns for all > > bigger requests. > > > > This improves behaviour for a few requests: > > > > request | previous result | new result > > -----------+-----------------+------------ > > 1024/1024 | 0/1024 | 1023/1024 > > 4000/5000 | 928/1024 | 1023/1024 > > 5000/5000 | 904/1024 | 1023/1024 > > > > (Values specified as duty_cycle / period in ns) > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Looks correct to me: > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Believe it or not but there is a hard-to-find public datasheet: > https://web.archive.org/web/20130614115108/http://www.stericsson.com/developers/CD00291561_UM1031_AB8500_user_manual-rev5_CTDS_public.pdf > > These register bits are described on page 282-283. Hmm, there are three settings related to the PWM (page 70): EnaPWMOut(i) FreqPWMOut(i) DutyPWMOut(i) If I understand the manual correctly, the driver is totaly bogous. With FreqPWMOut the period is defined, this is always set to 4b0, so the frequency is 293 Hz ≃ 3412969 ns. So if .duty_cycle = 1 ns is requested, we currently get 2 * 3412969 / 1024 ns ≃ 6666 ns?! The EnaPWMOut bits are not handled at all. I assume when this is unset, the output is inactive? Probably the three PWMs are better represented as one chip with 3 outputs instead of three chips with one PWM each, given there is only one register for the EnaPWMOut bits. Best regards Uwe
Hello, On Thu, Jan 05, 2023 at 10:02:15PM +0100, Uwe Kleine-König wrote: > On Thu, Jan 05, 2023 at 01:14:33AM +0100, Linus Walleij wrote: > > Believe it or not but there is a hard-to-find public datasheet: > > https://web.archive.org/web/20130614115108/http://www.stericsson.com/developers/CD00291561_UM1031_AB8500_user_manual-rev5_CTDS_public.pdf > > > > These register bits are described on page 282-283. > > Hmm, there are three settings related to the PWM (page 70): > > EnaPWMOut(i) > FreqPWMOut(i) > DutyPWMOut(i) > > If I understand the manual correctly, the driver is totaly bogous. > With FreqPWMOut the period is defined, this is always set to 4b0, so the > frequency is 293 Hz ≃ 3412969 ns. So if .duty_cycle = 1 ns is requested, > we currently get 2 * 3412969 / 1024 ns ≃ 6666 ns?! > > The EnaPWMOut bits are not handled at all. I assume when this is unset, > the output is inactive? That's wrong, the EnaPWMOut bit is handled. Don't know why I missed that. > Probably the three PWMs are better represented as one chip with 3 > outputs instead of three chips with one PWM each, given there is only > one register for the EnaPWMOut bits. Earlier today I looked again into this driver and (I hope) fixed the problems I was seening. I'm marking the patch in this thread as superseeded. Best regards Uwe
diff --git a/drivers/pwm/pwm-ab8500.c b/drivers/pwm/pwm-ab8500.c index ad37bc46f272..a7f64371449b 100644 --- a/drivers/pwm/pwm-ab8500.c +++ b/drivers/pwm/pwm-ab8500.c @@ -37,6 +37,7 @@ static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, u8 reg; unsigned int higher_val, lower_val; struct ab8500_pwm_chip *ab8500 = ab8500_pwm_from_chip(chip); + unsigned int duty_cycle; if (state->polarity != PWM_POLARITY_NORMAL) return -EINVAL; @@ -52,16 +53,25 @@ static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, return ret; } + /* The period is fixed at 0x400 ns */ + if (state->period < 0x400) + return -EINVAL; + + if (state->duty_cycle >= 0x400) + duty_cycle = 0x3ff; + else + duty_cycle = state->duty_cycle; + /* * get the first 8 bits that are be written to * AB8500_PWM_OUT_CTRL1_REG[0:7] */ - lower_val = state->duty_cycle & 0x00FF; + lower_val = duty_cycle & 0x00FF; /* * get bits [9:10] that are to be written to * AB8500_PWM_OUT_CTRL2_REG[0:1] */ - higher_val = ((state->duty_cycle & 0x0300) >> 8); + higher_val = (duty_cycle & 0x0300) >> 8; reg = AB8500_PWM_OUT_CTRL1_REG + (ab8500->hwid * 2);
The .apply() callback only considered the 10 low bits of .duty_cycle and didn't check .period at all. My best guess is the period is fixed at 1024 ns = 0x400 ns. Based on that assumption refuse configurations that request a lower period (as usual for PWM drivers) and configure a duty cycle of 0x3ff ns for all bigger requests. This improves behaviour for a few requests: request | previous result | new result -----------+-----------------+------------ 1024/1024 | 0/1024 | 1023/1024 4000/5000 | 928/1024 | 1023/1024 5000/5000 | 904/1024 | 1023/1024 (Values specified as duty_cycle / period in ns) Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pwm/pwm-ab8500.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)