| Message ID | 20201105164146.182254-1-pbarker@konsulko.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] hwmon: pwm-fan: Fix RPM calculation | expand |
On Thu, Nov 05, 2020 at 04:41:46PM +0000, Paul Barker wrote: > To convert the number of pulses counted into an RPM estimation, we need > to divide by the width of our measurement interval instead of > multiplying by it. > > We also don't need to do 64-bit division, with 32-bits we can handle a > fan running at over 4 million RPM. > > As the sample_timer() function is called once per second, delta will usually > be just over 1000 and should never be zero, avoiding the risk of a divide by > zero exception. > > Signed-off-by: Paul Barker <pbarker@konsulko.com> > --- > drivers/hwmon/pwm-fan.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > index bdba2143021a..2f2950629892 100644 > --- a/drivers/hwmon/pwm-fan.c > +++ b/drivers/hwmon/pwm-fan.c > @@ -54,14 +54,13 @@ static irqreturn_t pulse_handler(int irq, void *dev_id) > static void sample_timer(struct timer_list *t) > { > struct pwm_fan_ctx *ctx = from_timer(ctx, t, rpm_timer); > + unsigned int delta = ktime_ms_delta(ktime_get(), ctx->sample_start); > int pulses; > - u64 tmp; > > pulses = atomic_read(&ctx->pulses); > atomic_sub(pulses, &ctx->pulses); > - tmp = (u64)pulses * ktime_ms_delta(ktime_get(), ctx->sample_start) * 60; > - do_div(tmp, ctx->pulses_per_revolution * 1000); > - ctx->rpm = tmp; > + ctx->rpm = (unsigned int)(pulses * 1000 * 60) / > + (ctx->pulses_per_revolution * delta); delta can, at least in theory, be 0. While that is quite unlikely to happen, the code should handle this situation. > > ctx->sample_start = ktime_get(); > mod_timer(&ctx->rpm_timer, jiffies + HZ);
On Sun, 8 Nov 2020 at 03:47, Guenter Roeck <linux@roeck-us.net> wrote: > > On Thu, Nov 05, 2020 at 04:41:46PM +0000, Paul Barker wrote: > > To convert the number of pulses counted into an RPM estimation, we need > > to divide by the width of our measurement interval instead of > > multiplying by it. > > > > We also don't need to do 64-bit division, with 32-bits we can handle a > > fan running at over 4 million RPM. > > > > As the sample_timer() function is called once per second, delta will usually > > be just over 1000 and should never be zero, avoiding the risk of a divide by > > zero exception. > > > > Signed-off-by: Paul Barker <pbarker@konsulko.com> > > --- > > drivers/hwmon/pwm-fan.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > > index bdba2143021a..2f2950629892 100644 > > --- a/drivers/hwmon/pwm-fan.c > > +++ b/drivers/hwmon/pwm-fan.c > > @@ -54,14 +54,13 @@ static irqreturn_t pulse_handler(int irq, void *dev_id) > > static void sample_timer(struct timer_list *t) > > { > > struct pwm_fan_ctx *ctx = from_timer(ctx, t, rpm_timer); > > + unsigned int delta = ktime_ms_delta(ktime_get(), ctx->sample_start); > > int pulses; > > - u64 tmp; > > > > pulses = atomic_read(&ctx->pulses); > > atomic_sub(pulses, &ctx->pulses); > > - tmp = (u64)pulses * ktime_ms_delta(ktime_get(), ctx->sample_start) * 60; > > - do_div(tmp, ctx->pulses_per_revolution * 1000); > > - ctx->rpm = tmp; > > + ctx->rpm = (unsigned int)(pulses * 1000 * 60) / > > + (ctx->pulses_per_revolution * delta); > > delta can, at least in theory, be 0. While that is quite unlikely to happen, > the code should handle this situation. I'll send a v3 which bails out if delta=0. Thanks,
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c index bdba2143021a..2f2950629892 100644 --- a/drivers/hwmon/pwm-fan.c +++ b/drivers/hwmon/pwm-fan.c @@ -54,14 +54,13 @@ static irqreturn_t pulse_handler(int irq, void *dev_id) static void sample_timer(struct timer_list *t) { struct pwm_fan_ctx *ctx = from_timer(ctx, t, rpm_timer); + unsigned int delta = ktime_ms_delta(ktime_get(), ctx->sample_start); int pulses; - u64 tmp; pulses = atomic_read(&ctx->pulses); atomic_sub(pulses, &ctx->pulses); - tmp = (u64)pulses * ktime_ms_delta(ktime_get(), ctx->sample_start) * 60; - do_div(tmp, ctx->pulses_per_revolution * 1000); - ctx->rpm = tmp; + ctx->rpm = (unsigned int)(pulses * 1000 * 60) / + (ctx->pulses_per_revolution * delta); ctx->sample_start = ktime_get(); mod_timer(&ctx->rpm_timer, jiffies + HZ);
To convert the number of pulses counted into an RPM estimation, we need to divide by the width of our measurement interval instead of multiplying by it. We also don't need to do 64-bit division, with 32-bits we can handle a fan running at over 4 million RPM. As the sample_timer() function is called once per second, delta will usually be just over 1000 and should never be zero, avoiding the risk of a divide by zero exception. Signed-off-by: Paul Barker <pbarker@konsulko.com> --- drivers/hwmon/pwm-fan.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)