Messages in this thread | | | Date | Tue, 14 Jan 2020 08:47:10 +0100 | From | Uwe Kleine-König <> | Subject | Re: [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/ |
| |