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
From: Duc Nguyen <[email protected]>
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 <[email protected]>
[wsa: split and reworded commit message]
Signed-off-by: Wolfram Sang <[email protected]>
---
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
On 15.09.2021 9:55, Wolfram Sang wrote:
> From: Duc Nguyen <[email protected]>
>
> 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 <[email protected]>
> [wsa: split and reworded commit message]
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> 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
On Wed, Sep 15, 2021 at 08:55:40AM +0200, Wolfram Sang wrote:
> From: Duc Nguyen <[email protected]>
>
> 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 <[email protected]>
> [wsa: split and reworded commit message]
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> 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/ |
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