On Tue, 15 Dec 2020 at 12:56, Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> wrote: > > On Tue, Dec 15, 2020 at 11:29:39AM +0000, Paul Barker wrote: > > On Tue, 15 Dec 2020 at 09:23, Uwe Kleine-König > > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote: > > > > > > With MAX_PWM being defined to 255 the code > > > > > > unsigned long period; > > > ... > > > period = ctx->pwm->args.period; > > > state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM); > > > > Reviewing this I noticed that in pwm_fan_resume() we use > > DIV_ROUND_UP_ULL for what looks like essentially the same calculation. > > After my second patch this isn't true any more. With it applied > __set_pwm is the only place in the driver that calculates this stuff. > > > Could we just switch this line to DIV_ROUND_UP_ULL instead? > > Yes that would work, but actually I don't expect someone specifiying a > period big enough to justify the additional overhead of a 64 bit > division. So ULONG_MAX / (MAX_PWM + 1) is 16.7 million on 32-bit platforms. As the period is in nanoseconds (if I understand correctly), this would allow a period of up to 16.7ms and so a minimum frequency of around 60Hz. That does seem fairly reasonable to me but it's probably worth making a note of these limits in the commit message for future reference. Thanks, -- Paul Barker Konsulko Group
