lkml.org 
[lkml]   [2020]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 1/1] pwm: Convert period and duty cycle to u64
On Mon, Jan 13, 2020 at 03:53:31PM -0800, Guru Das Srinagesh wrote:
> Because period and duty cycle are defined as ints with units of
> nanoseconds, the maximum time duration that can be set is limited to
> ~2.147 seconds. Change their definitions to u64 in the structs of the
> PWM framework so that higher durations may be set.
>
> Also make the relevant fixes to those drivers that use the period and
> duty_cycle struct members in division operations, viz. replacing the
> division operations with 64-bit division macros as appropriate.

I like the goal of this patch (i.e. expanding the domain of period and
duty-cycle). I wonder however if we need a more sophisticated approach
here. The problem I see is that if something breaks (and the fix isn't
easy) we will have to revert the whole patch touching (now) 15 files.

I didn't thought about that much, but it would be great if we could
prepare the affected drivers to work with both, int and u64 and switch
in a separate commit. Reverting would then become cheaper.
The conversion to 64-bit division macros could be done even without
actually converting period and duty cycle, couldn't it?

> Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> Reported-by: kbuild test robot <lkp@intel.com>

This Reported-by: looks wrong. It gave some hints about what had to be
improved in an earlier revision of this patch, but usually this means
that the patch is a fix for an earlier commit. So I would put this in
the text, something like:

The kbuild test robot helped to improve this patch series to
(hopefully) catch all code sites having to be adapted.

> [...]
> diff --git a/drivers/pwm/pwm-clps711x.c b/drivers/pwm/pwm-clps711x.c
> index 924d39a..ba9500a 100644
> --- a/drivers/pwm/pwm-clps711x.c
> +++ b/drivers/pwm/pwm-clps711x.c
> @@ -43,7 +43,7 @@ static void clps711x_pwm_update_val(struct clps711x_chip *priv, u32 n, u32 v)
> static unsigned int clps711x_get_duty(struct pwm_device *pwm, unsigned int v)
> {
> /* Duty cycle 0..15 max */
> - return DIV_ROUND_CLOSEST(v * 0xf, pwm->args.period);
> + return DIV64_U64_ROUND_CLOSEST(v * 0xf, pwm->args.period);
> }

In ir-rx51.c you used DIV_ROUND_CLOSEST_ULL to replace
DIV_ROUND_CLOSEST, here it is DIV64_U64_ROUND_CLOSEST. Maybe it is worth
to describe the relevant difference shortly in the commit log.

> static int clps711x_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |

\
 
 \ /
  Last update: 2020-01-14 08:48    [W:0.121 / U:0.288 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site