Hi Jean,
> On Wed, 15 Dec 2010 11:04:15 +0100, Juerg Haefliger wrote:
>> > On Fri, 10 Dec 2010 10:43:57 +0100, Juerg Haefliger wrote:
>> >> --- dme1737.c 2010-08-02 00:11:14.000000000 +0200
>> >> +++ /home/khali/dme1737.c 2010-12-10 14:39:04.000000000 +0100
>> >> @@ -75,16 +75,20 @@
>> >> * in4 +12V
>> >> * in5 VTR (+3.3V stby)
>> >> * in6 Vbat
>> >> + * in7 Vtrip (sch5127 only)
>> >> *
>> >> *
>> ---------------------------------------------------------------------
>> */
>> >>
>> >> -/* Voltages (in) numbered 0-6 (ix) */
>> >> -#define DME1737_REG_IN(ix) ((ix) < 5 ? 0x20 + (ix)
>> \
>> >> - : 0x94 + (ix))
>> >> -#define DME1737_REG_IN_MIN(ix) ((ix) < 5 ? 0x44 + (ix)
>> * 2 \
>> >> - : 0x91 + (ix) * 2)
>> >> -#define DME1737_REG_IN_MAX(ix) ((ix) < 5 ? 0x45 + (ix)
>> * 2 \
>> >> - : 0x92 + (ix) * 2)
>> >> +/* Voltages (in) numbered 0-7 (ix) */
>> >> +#define DME1737_REG_IN(ix) ((ix) < 5 ? 0x20 + (ix)
>> : \
>> >> + (ix) < 7 ? 0x94 + (ix) : \
>> >> + 0x1f)
>> >> +#define DME1737_REG_IN_MIN(ix) ((ix) < 5 ? 0x44 + (ix)
>> * 2 : \
>> >> + (ix) < 7 ? 0x91 + (ix) * 2 : \
>> >> + 0x9f)
>> >> +#define DME1737_REG_IN_MAX(ix) ((ix) < 5 ? 0x45 + (ix)
>> * 2 : \
>> >> + (ix) < 7 ? 0x92 + (ix) * 2 : \
>> >> + 0xa0)
>> >
>> > Maybe it is time to give up macros and go with const arrays? The
>> macros
>> > are never called with a constant parameters so the compiler can't
>> > optimize the calls.
>>
>> I thought about that too. I keep it for now for the REG_IN for
>> consistency reasons (since MIN and MAX don't need any modification as
>> you pointed out in a subsequent email. duh!).
>
> Agreed.
>
>> > (...)
>> > Shouldn't this be [0-7]?
>>
>> Yes. And increasing the actual in[] array in the 'data' structure
>> might be a good thing too to prevent array overruns :-) Duh! No wonder
>> Jeff's min/max values behave weird.
>
> D'oh, how did I manage to miss that in my review :(
>
> Jeff, please search for "struct dme1737_data" in the source code, and
> change:
>
> u16 in[7];
> u8 in_min[7];
> u8 in_max[7];
>
> to
>
> u16 in[8];
> u8 in_min[8];
> u8 in_max[8];
>
> then rebuild the driver and try again!
>
Changes made. Recompile successful. Old module removed. Module installs
without error. Output as follows:
[root@anas-01 sensors]# sensors
sch5127-isa-0870
Adapter: ISA adapter
in0: +1.78 V (min = +0.00 V, max = +3.32 V)
Vccp: +1.19 V (min = +1.08 V, max = +1.32 V)
VCC: +3.31 V (min = +2.97 V, max = +3.63 V)
in3: +1.13 V (min = +0.00 V, max = +1.49 V)
in4: +1.48 V (min = +0.00 V, max = +1.49 V)
VTR: +3.33 V (min = +2.97 V, max = +3.63 V)
VBAT: +3.25 V (min = +2.70 V, max = +3.30 V)
in7: +1.43 V (min = +1.35 V, max = +1.65 V)
Side Fan: 1905 RPM (min = 700 RPM)
MCH Fan: 5032 RPM (min = 4500 RPM)
CPU Temp: +41.8°C (low = +20.0°C, high = +60.0°C)
SYS Temp: +32.9°C (low = +20.0°C, high = +60.0°C)
1.43 volts isn't 1.5 volts, but within +- 10 percent.
>> > (...)
>> > checkpatch.pl will complain. It wants:
>> >
>> > if (data->has_features & HAS_IN7) {
>> > err = sysfs_create_group(&dev->kobj,
>> &dme1737_in7_group);
>> > if (err)
>> > goto exit_remove;
>> > }
>>
>> OK. Assignments in if statements are being used all over the place in
>> this driver. I guess I should clean those up, shouldn't I?
>
> If you have time, yes. OTOH there are some cases where it actually
> makes the code a lot more complex so I am not always following
> checkpatch.pl on this myself. So you might as well keep your code as is
> if it is consistent throughout the driver.
>
> --
> Jean Delvare
>
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
