[lm-sensors] [PATCH] Add MAX6650 support

[khali@linux-fr.org (Jean Delvare)]

Hi Hans-J?rgen,

On Wed, 28 Feb 2007 17:08:16 +0100, Hans-J?rgen Koch wrote:
> Thanks a lot for testing, Matej! Jean, what do you say to that? If you think 
> you can apply this I'll update the documentation file.

Thanks for all the work you've been doing on the driver lately. I am
waiting for Corentin Labbe to comment before I do the final review and
merge the driver - I know he found some more things that could be fixed
or improved since his first review, some of which I think you addressed
yesterday, some not.

I look briefly at the new options you implemented. They are definitely
better as module options than hardcoded in the driver code, however I'm
not sure we really want all of them to be module options.

General remark: the module parameters are driver-wide and not
device-specific, so they must me checked at module load time (in
sensors_max6650_init). Also, for all settings that are not forced by
the user, it might be convenient to print in the logs the value that
was found for each chip.

voltage_12V: Must indeed be an option, although the convention you
chose is a bit strange IMHO. What about a parameter named "fan_voltage"
with possible values 0 (default, no change), 5 and 12?

(BTW, is this an output voltage? input voltage? both?)

prescaler: What does it do? It smells like a fan clock divider to me.
In that case it would be better implemented as a fan1_div sysfs file,
which the user can adjust at runtime.

clock: OK.

mode: This smells like fan speed control parameters, which we typically
implement as the pwm1_enable sysfs file. Please take a look at the
description in Documentation/hwmon/sysfs-interface and tell me what
you think.

counttime: This one too seems to be related to a fan clock divider? How
does it functionally differ from prescaler?

Thanks,
-- 
Jean Delvare