On Wed, Sep 15, 2021 at 08:55:40AM +0200, Wolfram Sang wrote:
> From: Duc Nguyen <duc.nguyen.ub@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> [wsa: split and reworded commit message]
> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> ---
> 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:
signature.asc
Description: PGP signature
