On Tue, 10 Nov 2020 at 17:32, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On 11/10/20 8:20 AM, Paul Barker wrote:
> > 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
>
> Yes, that is why I suggested to keep the old speed in that situation.
> After all, it _will_ be updated shortly afterwards. Either case, people
> won't typically look into the kernel log if they see the 0 rpm.
Ah, I misunderstood your earlier reply then. Yes - this makes sense,
clearly I haven't had enough coffee as that solution should have been
obvious!
v4 incoming.
--
Paul Barker
Konsulko Group
