Re: [PATCH] adm1026: fix setting fan_div

[Phil Pokorny <ppokorny@xxxxxxxxxxxxxxxxxxxx>]

On Tue, Nov 30, 2010 at 11:10 AM, Gabriele Gorla <gorlik@xxxxxxxxxxxxxxx> wrote:

On Tue, Nov 30, 2010 at 10:17:13AM +0100, Jean Delvare wrote:

> > +   if(val<1 || val>8) {
> > +           return -EINVAL;
>
> As underlined by Phil already, this is a little inconsistent. You
> should reject all invalid values, as documented at the end of
> Documentation/hwmon/sysfs-interface. But this should be done in a
> separate patch, as this isn't really a bug fix and changes the driver's
> behavior on invalid input.

Original code will reject 1 as input. I think this is a bug as div=1 is
valid for adm1026.
Anyway, if you think I should separate the patch and reject all invalid
values it's ok for me.

Agree.  Thanks
 

> > +                   adm1026_write_value(client, ADM1026_REG_FAN_DIV_4_7,
> > +                               (DIV_TO_REG(data->fan_div[4]) << 0) |
> > +                               (DIV_TO_REG(data->fan_div[5]) << 2) |
> > +                               (DIV_TO_REG(data->fan_div[6]) << 4) |
> > +                               (DIV_TO_REG(data->fan_div[7]) << 6) );
> > +           }
>
> Note: this is horribly inefficient. In my opinion, data->fan_div should
> hold split but un-decoded register values. Calling DIV_FROM_REG() is
> cheap, calling DIV_TO_REG() is expensive. In fact DIV_TO_REG()
> shouldn't exist in the first place, as the conversion should happen in
> a single place. Again, this is material for a separate patch.

agree.

 

NACK.

I disagree as I said before.  fan speed reading code gets called many more times than the fan_div setting code and there is no savings in changing fan_div from "div" to "reg" values.

Phil P.
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors