[lm-sensors] [PATCH] Add MAX6650 support
hjk at linutronix.de
Thu Mar 1 11:11:12 CET 2007
Am Donnerstag 01 März 2007 08:34 schrieb 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.
All right, I'm looking forward to his review.
> 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
The problem is that all these parameters have to fit together and
match the fan used. If you have a BIOS that configures the chip for
one special fan, and you have to replace that fan with a different
type, you have to calculate new values for prescaler and counttime.
Or, you might notice your board becomes too hot in a certain
application and decide to change the mode from "closed loop" to "full
on". All these things happen frequently in industrial applications.
Having all the values as module parameters doesn't harm as you can
safely ignore them if the BIOS values work for you. But to have them
as sysfs files seems to be an advantage.
> General remark: the module parameters are driver-wide and not
> device-specific, so they must me checked at module load time (in
I'll have a look at that.
> 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.
Agreed, that would be nice to have. I'll add that.
> 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
OK, I agree, that looks better.
> (BTW, is this an output voltage? input voltage? both?)
Neither nor. It's simply an information for the chip. It measures the
voltage at the low side of the fan. The parameter tells it whether
there are 5V or 12V at the high side. If it measures e.g. 5V at the
low side, a 5V fan is standing still while a 12V fan still has 7V
across it and is running at almost half speed.
If you accidentally set 12V where you really have a 5V fan, this
doesn't damage the fan. The fan will probably always run with full
speed, because the chip desperately tries to bring the voltage up.
> 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.
The chip generates a reference frequency which is determined by the
speed register, the prescaler, and the internal clock (nominal value
254kHz). The regulator then tries to make the tacho frequency equal
to this reference frequency.
As clock is fixed and speed has a well defined meaning, the prescaler
is the only way to adapt to different tacho generators. The data
sheet gives hints about how to select a prescaler value to achieve
optimal range and resolution. If you get that wrong, the fan might
not reach it's full speed, switch between two speed values only or
I read Documentation/hwmon/sysfs-interface, but I have to admit that I
don't really understand if fan[1-*]_div has this meaning. If it has,
we could possibly make prescaler such a sysfs file.
> clock: OK.
This is the only one where I think it could be omitted. The internal
254kHz oscillator has a tolerance of +/-10%, so fan speed can also be
off +/-10%. This parameter is just there to compensate for that. I
doubt it will ever be used in practical applications.
> 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.
That looks good. We can implement pwm1_enable that way. Problem is
that we can have up to four fan speeds (fan[1-4]_input), but the
control values are there only once (pwm1, pwm1_enable). Is that
> counttime: This one too seems to be related to a fan clock divider?
> How does it functionally differ from prescaler?
counttime is the time interval during which tacho pulses are counted.
It determines the resolution of the measured speed values. It should
be set to the longest time that still ensures the 8-bit counter
doesn't overflow under worst case conditions.
More information about the lm-sensors