* [PATCH 0/2] pwm: renesas-tpu: minor improvements @ 2021-09-15 6:55 Wolfram Sang 2021-09-15 6:55 ` [PATCH 1/2] pwm: renesas-tpu: better errno for impossible rates Wolfram Sang 2021-09-15 6:55 ` [PATCH 2/2] pwm: renesas-tpu: don't allow no duty if duty_ns is given Wolfram Sang 0 siblings, 2 replies; 7+ messages in thread From: Wolfram Sang @ 2021-09-15 6:55 UTC (permalink / raw) To: linux-pwm; +Cc: linux-renesas-soc, Wolfram Sang, linux-kernel Here is a super-small series with improvements for the Renesas TPU driver from our BSP. Please look at the patch descriptions for details. Looking forward to comments or them simply being applied :) Thanks and happy hacking, Wolfram Duc Nguyen (2): pwm: renesas-tpu: better errno for impossible rates pwm: renesas-tpu: don't allow no duty if duty_ns is given drivers/pwm/pwm-renesas-tpu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] pwm: renesas-tpu: better errno for impossible rates 2021-09-15 6:55 [PATCH 0/2] pwm: renesas-tpu: minor improvements Wolfram Sang @ 2021-09-15 6:55 ` Wolfram Sang 2021-09-15 9:36 ` Sergei Shtylyov 2021-09-17 8:25 ` Uwe Kleine-König 2021-09-15 6:55 ` [PATCH 2/2] pwm: renesas-tpu: don't allow no duty if duty_ns is given Wolfram Sang 1 sibling, 2 replies; 7+ messages in thread From: Wolfram Sang @ 2021-09-15 6:55 UTC (permalink / raw) To: linux-pwm Cc: linux-renesas-soc, Duc Nguyen, Wolfram Sang, Thierry Reding, Uwe Kleine-König, Lee Jones, linux-kernel From: Duc Nguyen <duc.nguyen.ub@renesas.com> ENOTSUP has confused users. EINVAL has been considered clearer. Change the errno, we were the only ones using ENOTSUP in this subsystem anyhow. Signed-off-by: Duc Nguyen <duc.nguyen.ub@renesas.com> [wsa: split and reworded commit message] Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/pwm/pwm-renesas-tpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c index 4381df90a527..754440194650 100644 --- a/drivers/pwm/pwm-renesas-tpu.c +++ b/drivers/pwm/pwm-renesas-tpu.c @@ -269,7 +269,7 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *_pwm, if (prescaler == ARRAY_SIZE(prescalers) || period == 0) { dev_err(&tpu->pdev->dev, "clock rate mismatch\n"); - return -ENOTSUPP; + return -EINVAL; } if (duty_ns) { -- 2.30.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] pwm: renesas-tpu: better errno for impossible rates 2021-09-15 6:55 ` [PATCH 1/2] pwm: renesas-tpu: better errno for impossible rates Wolfram Sang @ 2021-09-15 9:36 ` Sergei Shtylyov 2021-09-17 8:25 ` Uwe Kleine-König 1 sibling, 0 replies; 7+ messages in thread From: Sergei Shtylyov @ 2021-09-15 9:36 UTC (permalink / raw) To: Wolfram Sang, linux-pwm Cc: linux-renesas-soc, Duc Nguyen, Thierry Reding, Uwe Kleine-König, Lee Jones, linux-kernel On 15.09.2021 9:55, Wolfram Sang wrote: > From: Duc Nguyen <duc.nguyen.ub@renesas.com> > > ENOTSUP has confused users. EINVAL has been considered clearer. Change > the errno, we were the only ones using ENOTSUP in this subsystem anyhow. It's ENOTSUPP in the code. :-) > > Signed-off-by: Duc Nguyen <duc.nguyen.ub@renesas.com> > [wsa: split and reworded commit message] > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/pwm/pwm-renesas-tpu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c > index 4381df90a527..754440194650 100644 > --- a/drivers/pwm/pwm-renesas-tpu.c > +++ b/drivers/pwm/pwm-renesas-tpu.c > @@ -269,7 +269,7 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *_pwm, > > if (prescaler == ARRAY_SIZE(prescalers) || period == 0) { > dev_err(&tpu->pdev->dev, "clock rate mismatch\n"); > - return -ENOTSUPP; > + return -EINVAL; > } > > if (duty_ns) { MBR, Sergei ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] pwm: renesas-tpu: better errno for impossible rates 2021-09-15 6:55 ` [PATCH 1/2] pwm: renesas-tpu: better errno for impossible rates Wolfram Sang 2021-09-15 9:36 ` Sergei Shtylyov @ 2021-09-17 8:25 ` Uwe Kleine-König 2021-09-20 9:08 ` Wolfram Sang 1 sibling, 1 reply; 7+ messages in thread From: Uwe Kleine-König @ 2021-09-17 8:25 UTC (permalink / raw) To: Wolfram Sang Cc: linux-pwm, linux-renesas-soc, Duc Nguyen, Thierry Reding, Lee Jones, linux-kernel, kernel [-- Attachment #1: Type: text/plain, Size: 3668 bytes --] On Wed, Sep 15, 2021 at 08:55:40AM +0200, Wolfram Sang wrote: > From: Duc Nguyen <duc.nguyen.ub@renesas.com> > > ENOTSUP has confused users. EINVAL has been considered clearer. Change > the errno, we were the only ones using ENOTSUP in this subsystem anyhow. > > Signed-off-by: Duc Nguyen <duc.nguyen.ub@renesas.com> > [wsa: split and reworded commit message] > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/pwm/pwm-renesas-tpu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c > index 4381df90a527..754440194650 100644 > --- a/drivers/pwm/pwm-renesas-tpu.c > +++ b/drivers/pwm/pwm-renesas-tpu.c > @@ -269,7 +269,7 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *_pwm, > > if (prescaler == ARRAY_SIZE(prescalers) || period == 0) { > dev_err(&tpu->pdev->dev, "clock rate mismatch\n"); > - return -ENOTSUPP; > + return -EINVAL; > } prescaler == ARRAY_SIZE(prescalers) means that period_ns * clk_rate is too big for the hardware. Given that the driver considers clk_rate as constant, the interpretation is that period_ns is too big to be implemented. In this case the expectation for a new driver would be to round down to the biggest possible rate. period == 0 means the requested period is too small, in this case -EINVAL is right. The danger to make the driver behave like new drivers should is that it ends in regressions, but when we touch the behaviour this might be a good opportunity to "fix" this driver? This would look as follows: max_period_ns = 0xffff * NSEC_PER_SEC * 64 / clk_rate; period_ns = min(period_ns, max_period_ns); duty_ns = min(duty_ns, period_ns); /* * First assume a prescaler factor of 1 to calculate the period * value, if this yields a value that doesn't fit into the 16 * bit wide register field, pick a bigger prescaler. The valid * range for the prescaler register value is [0..3] and yields a * factor of (1 << (2 * prescaler)). */ period = clk_rate * period_ns / NSEC_PER_SEC; if (period == 0) return -EINVAL; if (period <= 0xffff) prescaler = 0; else { prescaler = (ilog2(period) - 14) / 2; BUG_ON(prescaler > 3); } period >>= 2 * prescaler; duty = clk_rate * duty_ns >> (2 * prescaler) / NSEC_PER_SEC; (Note: This needs more care to handle overflows, e.g. 0xffff * NSEC_PER_SEC * 64 doesn't fit into a long, also clk_rate * period_ns might overflow. I skipped this for the purpose of this mail.) The code comment "TODO: Pick the highest acceptable prescaler." is unclear to me, as a smaller prescaler allows more possible settings for the duty_cycle and I don't see any reason to pick a bigger prescaler. If we choose to not adapt the behaviour, I suggest to replace if (duty_ns) { duty = clk_rate / prescalers[prescaler] / (NSEC_PER_SEC / duty_ns); if (duty > period) return -EINVAL; } else { duty = 0; } by: duty = clk_rate * duty_ns >> (2 * prescaler) / NSEC_PER_SEC; (probably using u64 math after asserting that period_ns * clk_rate doesn't overflow a u64. Then given that duty_ns <= period_ns the case duty > period cannot happen, the special case for duty_ns == 0 doesn't need to be explicitly handled and precision is improved. 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] pwm: renesas-tpu: better errno for impossible rates 2021-09-17 8:25 ` Uwe Kleine-König @ 2021-09-20 9:08 ` Wolfram Sang 0 siblings, 0 replies; 7+ messages in thread From: Wolfram Sang @ 2021-09-20 9:08 UTC (permalink / raw) To: Uwe Kleine-König Cc: linux-pwm, linux-renesas-soc, Duc Nguyen, Thierry Reding, Lee Jones, linux-kernel, kernel [-- Attachment #1: Type: text/plain, Size: 292 bytes --] Hi Uwe, thank you for your detailed review, much appreciated! I will look into your suggestions. However, it will probably not be before October because it seems some more work and internal discussion is needed beforehand. I'll get back to you. Thanks again and happy hacking, Wolfram [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] pwm: renesas-tpu: don't allow no duty if duty_ns is given 2021-09-15 6:55 [PATCH 0/2] pwm: renesas-tpu: minor improvements Wolfram Sang 2021-09-15 6:55 ` [PATCH 1/2] pwm: renesas-tpu: better errno for impossible rates Wolfram Sang @ 2021-09-15 6:55 ` Wolfram Sang 2021-09-16 16:47 ` Uwe Kleine-König 1 sibling, 1 reply; 7+ messages in thread From: Wolfram Sang @ 2021-09-15 6:55 UTC (permalink / raw) To: linux-pwm Cc: linux-renesas-soc, Duc Nguyen, Wolfram Sang, Thierry Reding, Uwe Kleine-König, Lee Jones, linux-kernel From: Duc Nguyen <duc.nguyen.ub@renesas.com> We have special code if duty_ns is 0. But if non-zero is given, then the calculation should not result in zero duty. Signed-off-by: Duc Nguyen <duc.nguyen.ub@renesas.com> [wsa: split and reworded commit message] Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/pwm/pwm-renesas-tpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c index 754440194650..bb51156e4bda 100644 --- a/drivers/pwm/pwm-renesas-tpu.c +++ b/drivers/pwm/pwm-renesas-tpu.c @@ -275,7 +275,7 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *_pwm, if (duty_ns) { duty = clk_rate / prescalers[prescaler] / (NSEC_PER_SEC / duty_ns); - if (duty > period) + if (duty > period || duty == 0) return -EINVAL; } else { duty = 0; -- 2.30.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] pwm: renesas-tpu: don't allow no duty if duty_ns is given 2021-09-15 6:55 ` [PATCH 2/2] pwm: renesas-tpu: don't allow no duty if duty_ns is given Wolfram Sang @ 2021-09-16 16:47 ` Uwe Kleine-König 0 siblings, 0 replies; 7+ messages in thread From: Uwe Kleine-König @ 2021-09-16 16:47 UTC (permalink / raw) To: Wolfram Sang Cc: linux-pwm, linux-renesas-soc, Duc Nguyen, Thierry Reding, Lee Jones, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1665 bytes --] On Wed, Sep 15, 2021 at 08:55:41AM +0200, Wolfram Sang wrote: > From: Duc Nguyen <duc.nguyen.ub@renesas.com> > > We have special code if duty_ns is 0. But if non-zero is given, then the > calculation should not result in zero duty. Why not? Assuming a PWM that supports multiples of say 100 ns for duty_cycle, rounding a request for 550 ns down to 500 ns isn't worse than rounding down a request for 50 ns to 0 ns is it? > Signed-off-by: Duc Nguyen <duc.nguyen.ub@renesas.com> > [wsa: split and reworded commit message] > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/pwm/pwm-renesas-tpu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c > index 754440194650..bb51156e4bda 100644 > --- a/drivers/pwm/pwm-renesas-tpu.c > +++ b/drivers/pwm/pwm-renesas-tpu.c > @@ -275,7 +275,7 @@ static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *_pwm, > if (duty_ns) { > duty = clk_rate / prescalers[prescaler] > / (NSEC_PER_SEC / duty_ns); Unrelated to the change under discussion here: Dividing by the result of a division is bad. Consider: clk_rate = 1333333333 prescalers[prescaler] = 43 duty_ns = 500000001 The exact result is 15503875.996124031, with the above formula you get 31007751 which is off by nearly a factor of two. These numbers are probably not relevant, but they show the problem. 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-09-20 9:12 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-15 6:55 [PATCH 0/2] pwm: renesas-tpu: minor improvements Wolfram Sang 2021-09-15 6:55 ` [PATCH 1/2] pwm: renesas-tpu: better errno for impossible rates Wolfram Sang 2021-09-15 9:36 ` Sergei Shtylyov 2021-09-17 8:25 ` Uwe Kleine-König 2021-09-20 9:08 ` Wolfram Sang 2021-09-15 6:55 ` [PATCH 2/2] pwm: renesas-tpu: don't allow no duty if duty_ns is given Wolfram Sang 2021-09-16 16:47 ` Uwe Kleine-König
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.