Jordan Crouse wrote:
> Hi Mark -
>
> Attached is a patch for the ADT7475 that has been bouncing around on
> the list for some time now. It is suitable for the testing
> GIT tree, and I think it is a good candidate for the 2.6.26 push.
>
Hi,
Very sorry for the long silence, it seems all of us involved in the hwmon tree
have a bit too much on our plate. As can be witnessed from Mark Hoffman
stepping down as hwmon subsys maintainer.
Anyways I've started reviewing your driver, but I haven't come around to
reviewing the actual code. Thanks for writting good documentation :) As I now
already have found quite a few cases where your driver deviates from the
standard sysfs interface as documented in:
Documentation/hwmon/sysfs-interface
So in this initial review I'll only comment on your:
Documentation/hwmon/adt7475
And I kindly ask you todo a revised version of your driver fixing the interface
deviations I believe to be present. When thats done I'll do a full review and I
promise I'll be a lot quicker this time around (and when I promise something,
you can expect it to happen!).
+This describes the interface for the ADT7475 driver:
+
+(there are 4 fans, numbered fan1 to fan4):
+
+fanX_input Read the current speed of the fan (in RPMs)
+fanX_min Read/write the minimum speed of the fan. Dropping
+ below this sets an alarm.
+
+(there are three PWMs, numbered pwm1 to pwm3):
+
+pwmX Read/write the current duty cycle of the PWM. Writes
+ only have effect when auto mode is turned off (see
+ below).
+
Good, I assume (not looked at the code yet) this goes from 0 - 255 ? If not it
should and that should be scaled to whatever the IC wants.
+pwmX_enable Read/write the PWM configuration based on the following
+ table:
+
+ 0 - Remote1 temp controls PWMx (auto mode)
+ 1 - local temp controls PWMx (auto mode)
+ 2 - remote2 temp controls PWMx (auto mode)
+ 3 - PWMx runs at full speed
+ 4 - PWMx is disabled
+ 5 - Use fastest speed calculated by local and remote2
+ 6 - Use fastest speed calculated by all three channels
+ 7 - Manual mode
+
Erm, this is somewhat non standard, atleast 0 and 1 have a well defined meaning
in the sysfs-interface standard:
"pwm[1-*]_enable
Fan speed control method:
0: no fan speed control (i.e. fan at full speed)
1: manual fan speed control enabled (using pwm[1-*])
2+: automatic fan speed control enabled
Check individual chip documentation files for automatic mode
details."
Actually reading the datasheet, I think this is not what we want, what we want
pwmX_enable to be for the adt7475 is:
0: No fan speed control (fan at full speed, iow 3 in your old table)
1: Manual fan speed control (7 in your old table)
2: Automatic fan speed control (0-2, 5-6 in your old table, see below for
which one should be used when).
And then add a pwmX_auto_channels_temp atrribute to control which temp
sensor(s) influence the pwm, quoting our sysfs standard doc again:
"pwm[1-*]_auto_channels_temp
Select which temperature channels affect this PWM output in
auto mode. Bitfield, 1 is temp1, 2 is temp2, 4 is temp3 etc...
Which values are possible depend on the chip used.
RW"
So that would mean that valid values for the adt7475 are, with corresponding
number in your old table:
1 -> 0 (I assume remote1 is temp1, local temp2 and remote2 temp3)
2 -> 1
4 -> 2
6 -> 5
7 -> 6
Note that 4 from your old table is not used, we don't want people to be able to
turn of the pwm (and thus the fan) through pwm_enable _ever_. If they really
want todo something that stupid they should configure manual mode and write a
duty cycle of 0 %.
+pwmX_freq Read/write the PWM frequency. The value returned is
+ an index into the following table:
+
+ 0x0 - 11.0 Hz
+ 0x1 - 14.7 Hz
+ 0x2 - 22.1 Hz
+ 0x3 - 29.4 Hz
+ 0x4 - 35.3 Hz
+ 0x5 - 44.1 Hz
+ 0x6 - 58.8 Hz
+ 0x7 - 88.2 Hz
+
That table should be in the driver and userspace should read/write a value in
Hz, to quote the docs:
"pwm[1-*]_freq Base PWM frequency in Hz.
Only possibly available when pwmN_mode is PWM, but not always
present even then.
RW"
+pwmX_max Read/write the maximum PWM duty cycle. The PWM
+ duty cycle will never exceed this.
+
+pwmX_min Read/write the minimum PWM duty cycle in automatic mode
+
Good (but again 0 - 255 scale please).
+(there are three temperature settings numbered temp1 to temp3):
+
+tempX_input Read the current temperature. The value is in milli
+ degrees of Celsius.
+
+tempX_max Read/write the upper temperature limit - exceeding this
+ will cause an alarm.
+
+tempX_min Read/write the lower temperature limit - exceeding this
+ will cause an alarm.
+
+tempX_offset Read/write the temperature adjustment offset
+
I assume this are all milli degrees too? If not they should be.
+tempX_crit_max Read/write the THERM limit for remote1. Exceeding this
+ causes the chip to force the processor off.
+
The standardized name for this is tempX_crit, and drop the force the processor
off language, that really depends on how the IC is hooked up on the motherboard
and thus isn't always true.
+tempX_auto_min Read/write the minimum temperature where the fans will
+ turn on in automatic mode.
+
+tempX_auto_range Read/write the range over which the automatic fan
+ control will be executed. The value returned is a
+ index into the following table:
+
+ 0x0 - 2 C
+ 0x1 - 2.5 C
+ 0x2 - 3.33 C
+ 0x3 - 4 C
+ 0x4 - 5 C
+ 0x5 - 6.67 C
+ 0x6 - 8 C
+ 0x7 - 10 C
+ 0x8 - 13.33 C
+ 0x9 - 16 C
+ 0xA - 20 C
+ 0xB - 26.67 C
+ 0xC - 32 C
+ 0xD - 40 C
+ 0xE - 53.33 C
+ 0xF - 80 C
+
Please rename tempX_auto_min to
tempX_auto_point1_temp
And rename tempX_auto_range to tempX_auto_point2_temp and when
tempX_auto_point2_temp gets read show tempX_auto_point1_temp + the range value
looked up in the above table (in milli degrees please) and vica versa when
tempX_auto_point2_temp gets written.
+tempX_crit_hyst set the temperature range below crit_max where the
+ fans will stay on - this helps drive the temperature
+ low enough so it doesn't stay near the edge and
+ cause THERM to keep tripping.
+
Good, but note that this should not be an offset, but an absolute value so when
temp1_crit is for example 100000 and the temp must drop more then 10 degrees
before the crit alarm gets cleared, then reading temp1_crit_hyst should return
90000.
+tempX_alarm Read a 1 if the max/min alarm is set
+tempX_crit_alarm Read a 1 if the critical limit is exceeded
+tempX_fault Read a 1 if either temp1 or temp3 diode has a fault
+
All good.
+(There are two voltage settings, in1 and in2):
+
+inX_input Read the current voltage on VCC. Value is in
+ millivolts.
+
+inX_min read/write the minimum voltage limit.
+ Dropping below this causes an alarm.
+
+inX_max read/write the maximum voltage limit.
+ Exceeding this causes an alarm.
+
+inX_alarm Read a 1 if the max/min alarm is set.
Also all good.
Again, apologies for being so slow, esp since all I've done now is take a look
at the API and not even at the code :(
I'll be leaving tomorrow morning for a week vacation. I hope you will have been
able todo a new revision fixing all the API remarks I've made and then I'll do
a full review real soon!
Regards,
Hans
