Re: [PATCH v4 1/1] pwm: Convert period and duty cycle to u64

[Uwe Kleine-König]

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@xxxxxxxxxxxxxx>
> Reported-by: kbuild test robot <lkp@xxxxxxxxx>

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/ |