On Tue, 10 Nov 2020 at 16:08, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On Tue, Nov 10, 2020 at 11:28:17AM +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. If the width of the measurement interval is zero we
> > bail out instead of dividing 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.
> >
> > Signed-off-by: Paul Barker <pbarker@xxxxxxxxxxxx>
> > ---
> > drivers/hwmon/pwm-fan.c | 17 +++++++++++------
> > 1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> > index edc0453be25a..24cfed4d625e 100644
> > --- a/drivers/hwmon/pwm-fan.c
> > +++ b/drivers/hwmon/pwm-fan.c
> > @@ -55,14 +55,19 @@ 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;
> > + if (delta) {
> > + pulses = atomic_read(&ctx->pulses);
> > + atomic_sub(pulses, &ctx->pulses);
> > + ctx->rpm = (unsigned int)(pulses * 1000 * 60) /
> > + (ctx->pulses_per_revolution * delta);
> > + } else {
> > + dev_err(ctx->dev,
> > + "Cannot determine fan RPM as time delta is zero\n");
> > + ctx->rpm = 0;
>
> I don't think that warrants an error message. At best it should be a debug
> message, but even that seems not worth it. I would suggest to not update
> rpm if that happens. After all, it is pretty much a theoretic case.
My thought process was that setting an RPM value of zero would be
confusing - it could be caused due to fan failure or due to this
(theoretical) error. I'm happy to drop the error message though - is
the patch acceptable other than that?
--
Paul Barker
Konsulko Group
