Update lm_sensors to handle more of the
PATCH 1/1 Update lm_sensors to handle more of the
From: Charles Spirakis <bezaur at gmail.com>
Modified patch from Sven Anders to enhance w83791d support. Partial support was already implemented, but it was missing IN9/FAN4/FAN5 output.
Signed-off by: Sven Anders <anders at anduras.de> Signed-off by: Charles Spirakis <bezaur at gmail.com>
---
kernel/chips/w83781d.c | 16 ++++++++++----- prog/sensors/chips.c | 50
+++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 59
insertions(+), 7 deletions(-)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lm-sensors-r4079-20060730_sven_changes.patch
Type: text/x-patch
Size: 6216 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060803/920c40e7/attachment-0001.bin
PATCH 1/1 Update lm_sensors to handle more of the
Hi Charles, Sven,
Sorry for the late answer.
Review static struct i2c_driver w83781d_driver · line 390
Quoted 13 lines
Modified patch from Sven Anders to enhance w83791d support. Partial support was already implemented, but it was missing IN9/FAN4/FAN5 output.
/* PWM 782D (1-4) and 783S (1-2) only */
(quoted diff) +3
-2@@ -385,16 +385,22 @@ static struct i2c_driver w83781d_driver #define W83781D_ALARM_IN4 0x0100 #define W83781D_ALARM_IN5 0x0200 #define W83781D_ALARM_IN6 0x0400 -#define W83782D_ALARM_IN7 0x10000 -#define W83782D_ALARM_IN8 0x20000 +#define W83782D_ALARM_IN7 0x10000 /* 782D/791D */ +#define W83782D_ALARM_IN8 0x20000 /* 782D/791D */ +#define W83791D_ALARM_IN9 0x40000 /* 791D only */
This does NOT match the W83791D datasheet I have here. These bits are OK for the 782D, but the 791D seems to use a different mapping:
5VSB = in7 = register 0xAB bit 3 -> 0x80000
VBAT = in8 = register 0xAB bit 4 -> 0x100000
VINR1 = in9 = register 0xAA bit 6 -> 0x4000
Review static struct i2c_driver w83781d_driver · line 395
Quoted 5 lines
(quoted diff) +2
-0#define W83781D_ALARM_FAN1 0x0040 #define W83781D_ALARM_FAN2 0x0080 #define W83781D_ALARM_FAN3 0x0800 +#define W83791D_ALARM_FAN4 0x200000 /* 791D only */ +#define W83791D_ALARM_FAN5 0x400000 /* 791D only */
I'm OK with these ones.
Review static struct i2c_driver w83781d_driver · line 399
Quoted 6 lines
(quoted diff) +2
-2#define W83781D_ALARM_TEMP1 0x0010 #define W83781D_ALARM_TEMP23 0x0020 /* 781D only */ -#define W83781D_ALARM_TEMP2 0x0020 /* 782D/783S */ -#define W83781D_ALARM_TEMP3 0x2000 /* 782D only */ +#define W83781D_ALARM_TEMP2 0x0020 /* 782D/783S/791D */ +#define W83781D_ALARM_TEMP3 0x2000 /* 782D/791D */
I'm OK here as well.
Review static struct i2c_driver w83781d_driver · line 403
Quoted 4 lines
(quoted diff) +3
-0#define W83781D_ALARM_CHAS 0x1000 +#define W83791D_ALARM_VINR1 0x04000 /* 782D/791D */ +#define W83791D_ALARM_VSB 0x80000 /* 791D only */ +#define W83791D_ALARM_VBAT 0x100000 /* 791D only */
And here you have the same values I have above. These are the ones we want to use! But I'd rather name them W83791D_ALARM_IN{7,8,9} to keep some consistency in the naming.
Quoted 33 lines
diff -urpN
lm-sensors-r4079-20060730/prog/sensors/chips.clm-sensors-r4079-20060730_sven_changes/prog/sensors/chips.cprog/sensors/chips.c +3
-2@@ -2187,7 +2187,7 @@ void print_w83781d(const sensors_chip_nachar *label;double cur,min,max,fdiv,sens;int alarms,beeps; -int is81d, is82d, is83s, is697hf, is627thf, valid; +int is81d, is82d, is83s, is91d, is697hf, is627thf, valid; is81d = !strcmp (name->prefix,"w83781d" ); is82d = (!strcmp(name->prefix,"w83782d")) ||@@ -2196,6 +2196,7 @@ void print_w83781d(const sensors_chip_na (!strcmp (name->prefix,"w83627thf" )) || (!strcmp (name->prefix,"w83687thf" )); is83s = !strcmp (name->prefix,"w83783s" ); + is91d = !strcmp (name->prefix,"w83791d" ); is627thf = (!strcmp (name->prefix,"w83627thf" )) || (!strcmp (name->prefix,"w83637hf" )) || (!strcmp (name->prefix,"w83687thf" ));@@ -2310,7 +2311,7 @@ void print_w83781d(const sensors_chip_naprintf ("ERROR: Can't get IN6 data!\n" );free (label); }/* !is627thf */ -if (is82d || is697hf || is627thf) { +if (is82d || is697hf || is627thf || is91d) { if (!sensors_get_label_and_valid(*name,SENSORS_W83782D_IN7,&label,&valid) && !sensors_get_feature(*name,SENSORS_W83782D_IN7,&cur) && !sensors_get_feature(*name,SENSORS_W83782D_IN7_MIN,&min) &&@@ -2339,6 +2340,22 @@ void print_w83781d(const sensors_chip_nafree (label); }
prog/sensors/chips.c void print_w83781d(const sensors_chip_na · line 2352
Quoted 10 lines
prog/sensors/chips.c +10
-0+if (is91d) { +if (!sensors_get_label_and_valid (*name,SENSORS_W83791D_IN9 ,&label,&valid) && + !sensors_get_feature (*name,SENSORS_W83791D_IN9 ,&cur) && + !sensors_get_feature (*name,SENSORS_W83791D_IN9_MIN ,&min) && + !sensors_get_feature (*name,SENSORS_W83791D_IN9_MAX ,&max)) { +if (valid) { +print_label (label,10 ); +printf ("%+6.2f V (min = %+6.2f V, max = %+6.2f V) %s %s\n" , + cur,min,max,alarms&W83791D_ALARM_IN9 ?"ALARM" :" " , + beeps&W83791D_ALARM_IN9 ?"(beep)" :"" );
OK if and only if W83791D_ALARM_IN9 is set to the right value.
Quoted 35 lines
prog/sensors/chips.c +28
-0+ } + }else +printf ("ERROR: Can't get IN9 data!\n" ); +free (label); + } + if (!sensors_get_label_and_valid(*name,SENSORS_W83781D_FAN1,&label,&valid) && !sensors_get_feature(*name,SENSORS_W83781D_FAN1,&cur) && !sensors_get_feature(*name,SENSORS_W83781D_FAN1_DIV,&fdiv) &&@@ -2382,6 +2399,35 @@ void print_w83781d(const sensors_chip_nafree (label); } +if (is91d) { +if (!sensors_get_label_and_valid (*name,SENSORS_W83791D_FAN4 ,&label,&valid) && + !sensors_get_feature (*name,SENSORS_W83791D_FAN4 ,&cur) && + !sensors_get_feature (*name,SENSORS_W83791D_FAN4_DIV ,&fdiv) && + !sensors_get_feature (*name,SENSORS_W83791D_FAN4_MIN ,&min)) { +if (valid) { +print_label (label,10 ); +printf ("%4.0f RPM (min = %4.0f RPM, div = %1.0f) %s %s\n" , + cur,min,fdiv, alarms&W83791D_ALARM_FAN4 ?"ALARM" :" " , + beeps&W83791D_ALARM_FAN4 ?"(beep)" :"" ); + } + }else +printf ("ERROR: Can't get FAN4 data!\n" ); +free (label); +if (!sensors_get_label_and_valid (*name,SENSORS_W83791D_FAN5 ,&label,&valid) && + !sensors_get_feature (*name,SENSORS_W83791D_FAN5 ,&cur) && + !sensors_get_feature (*name,SENSORS_W83791D_FAN5_DIV ,&fdiv) && + !sensors_get_feature (*name,SENSORS_W83791D_FAN5_MIN ,&min)) { +if (valid) { +print_label (label,10 ); + printf("%4.0f RPM (min = %4.0f RPM, div = %1.0f) %s %s\n", + cur,min,fdiv, alarms&W83791D_ALARM_FAN5?"ALARM":" ",
prog/sensors/chips.c void print_w83781d(const sensors_chip_na · line 2433
Quoted 10 lines
prog/sensors/chips.c +7
-0+ beeps&W83791D_ALARM_FAN5?"(beep)":""); + } + }else +printf ("ERROR: Can't get FAN5 data!\n" ); +free (label); + } + if (!sensors_get_label_and_valid(*name,SENSORS_W83781D_TEMP1,&label,&valid) && !sensors_get_feature(*name,SENSORS_W83781D_TEMP1,&cur) && !sensors_get_feature(*name,SENSORS_W83781D_TEMP1_HYST,&min) &&
This is OK but not sufficient as far as I can see. Because of the difference in alarm bits for in7 and in8, you also need to add specific code for the w83791d in the in7 and in8 cases.
I'd like to have these improvements merged soon now, can you please provide a new patch?
Thanks,
Signature · 1 line
Jean Delvare
PATCH 1/1 Update lm_sensors to handle more of the
In reply to Charles SpirakisJean --
I will try to get an update to you in the next few days. However, there is some ambiguity in the documentation which is causing some of the confusion...
If you look at the 791d beep enable register (0xa3) and compare those bits to the alarm status register (0xab) you will see a discrepancy in the bit ordering.
Sven and I tried to figure out if the documentation was correct or if one of the bit orderings provided were correct and it was hard to tell. It seemed like the bits for the enable did correspond to the bits for the alarm and we had reason to believe the enable bits were correct (which puts in7 and in8 at 0x10000 and 0x20000 respectively - the same as the w83782d). In a side note, if you look at the interrupt mask bits (0x9d) they seem to follow the alarm status bit pattern and not the enable bit pattern but since the driver isn't using that register it's not overly significant other than interesting information.
I will go back and test this again just to verify what we think we saw is what we actually saw (or heard as the case may be).
If anyone from winbond is reading this, would it be possible to get a definitive answer as to whether the documentation is right and (if not), to let us know what the correct bit patterns are for the alarm vs. the enable?
Along these same lines, the bit position for bit 1 and bit 13 are also swapped between what is shown for the enable vs. alarm. In that case, the documentation seems to be correct and the patch for the 2.6 driver is updated to reflect this.
Which leads to a different question. The assumption that was made for the driver patch was that we wanted the alarm and enable bits to match in user-space (and the driver would hide the problem if the chip didn't do this). Is this the correct assumption? Or should userspace get the values directly from the chip (so it matches the chip documentation) and let the user-space code deal with the bit twiddling?
Oh, and I just noticed the inconsistency in the 2.6 w83791d driver patch in the Documentation file - needs to be:
14 - in9 (VINR1)
Depending on your answer to the previous question (how do we handle the situation where the enable bit ordering is different from the alarm bit ordering), I will update or remove the patch... And yes, I understand this is yet another argument for updating this code to the standardized sysfs beep/alarm model. :)
-- charles
Quoted 154 lines · Jean Delvare
Hi Charles, Sven,
Sorry for the late answer.
Review static struct i2c_driver w83781d_driver · line 390
Quoted 13 lines
Modified patch from Sven Anders to enhance w83791d support. Partial support was already implemented, but it was missing IN9/FAN4/FAN5 output.
/* PWM 782D (1-4) and 783S (1-2) only */
(quoted diff) +3
-2@@ -385,16 +385,22 @@ static struct i2c_driver w83781d_driver #define W83781D_ALARM_IN4 0x0100 #define W83781D_ALARM_IN5 0x0200 #define W83781D_ALARM_IN6 0x0400 -#define W83782D_ALARM_IN7 0x10000 -#define W83782D_ALARM_IN8 0x20000 +#define W83782D_ALARM_IN7 0x10000 /* 782D/791D */ +#define W83782D_ALARM_IN8 0x20000 /* 782D/791D */ +#define W83791D_ALARM_IN9 0x40000 /* 791D only */This does NOT match the W83791D datasheet I have here. These bits are OK for the 782D, but the 791D seems to use a different mapping:
5VSB = in7 = register
0xABbit 3 ->0x80000
VBAT = in8 = register0xABbit 4 ->0x100000
VINR1 = in9 = register0xAAbit 6 ->0x4000Review static struct i2c_driver w83781d_driver · line 395
Quoted 5 lines
(quoted diff) +2
-0#define W83781D_ALARM_FAN1 0x0040 #define W83781D_ALARM_FAN2 0x0080 #define W83781D_ALARM_FAN3 0x0800 +#define W83791D_ALARM_FAN4 0x200000 /* 791D only */ +#define W83791D_ALARM_FAN5 0x400000 /* 791D only */I'm OK with these ones.
Review static struct i2c_driver w83781d_driver · line 399
Quoted 6 lines
(quoted diff) +2
-2#define W83781D_ALARM_TEMP1 0x0010 #define W83781D_ALARM_TEMP23 0x0020 /* 781D only */ -#define W83781D_ALARM_TEMP2 0x0020 /* 782D/783S */ -#define W83781D_ALARM_TEMP3 0x2000 /* 782D only */ +#define W83781D_ALARM_TEMP2 0x0020 /* 782D/783S/791D */ +#define W83781D_ALARM_TEMP3 0x2000 /* 782D/791D */I'm OK here as well.
Review static struct i2c_driver w83781d_driver · line 403
Quoted 4 lines
(quoted diff) +3
-0#define W83781D_ALARM_CHAS 0x1000 +#define W83791D_ALARM_VINR1 0x04000 /* 782D/791D */ +#define W83791D_ALARM_VSB 0x80000 /* 791D only */ +#define W83791D_ALARM_VBAT 0x100000 /* 791D only */And here you have the same values I have above. These are the ones we want to use! But I'd rather name them W83791D_ALARM_IN{7,8,9} to keep some consistency in the naming.
Quoted 33 lines
diff -urpN
lm-sensors-r4079-20060730/prog/sensors/chips.clm-sensors-r4079-20060730_sven_changes/prog/sensors/chips.cprog/sensors/chips.c 2006-07-30 22:26:05.000000000 -0700 renamed from prog/sensors/chips.c 2006-07-29 21:10:45.000000000 -0700 +3
-2@@ -2187,7 +2187,7 @@ void print_w83781d(const sensors_chip_na char *label; double cur,min,max,fdiv,sens; int alarms,beeps; - int is81d, is82d, is83s, is697hf, is627thf, valid; + int is81d, is82d, is83s, is91d, is697hf, is627thf, valid; is81d = !strcmp(name->prefix,"w83781d"); is82d = (!strcmp(name->prefix,"w83782d")) ||@@ -2196,6 +2196,7 @@ void print_w83781d(const sensors_chip_na (!strcmp(name->prefix, "w83627thf")) || (!strcmp(name->prefix, "w83687thf")); is83s = !strcmp(name->prefix,"w83783s"); + is91d = !strcmp(name->prefix,"w83791d"); is627thf = (!strcmp(name->prefix,"w83627thf")) || (!strcmp(name->prefix, "w83637hf")) || (!strcmp(name->prefix, "w83687thf"));@@ -2310,7 +2311,7 @@ void print_w83781d(const sensors_chip_na printf("ERROR: Can't get IN6 data!\n"); free(label); } /* !is627thf */ - if (is82d || is697hf || is627thf) { + if (is82d || is697hf || is627thf || is91d) { if (!sensors_get_label_and_valid(*name,SENSORS_W83782D_IN7,&label,&valid) && !sensors_get_feature(*name,SENSORS_W83782D_IN7,&cur) && !sensors_get_feature(*name,SENSORS_W83782D_IN7_MIN,&min) &&@@ -2339,6 +2340,22 @@ void print_w83781d(const sensors_chip_na free(label); }prog/sensors/chips.c 2006-07-30 22:26:05.000000000 -0700 void print_w83781d(const sensors_chip_na · line 2352
Quoted 10 lines
prog/sensors/chips.c 2006-07-30 22:26:05.000000000 -0700 renamed from prog/sensors/chips.c 2006-07-29 21:10:45.000000000 -0700 +10
-0+ if (is91d) { + if (!sensors_get_label_and_valid(*name,SENSORS_W83791D_IN9,&label,&valid) && + !sensors_get_feature(*name,SENSORS_W83791D_IN9,&cur) && + !sensors_get_feature(*name,SENSORS_W83791D_IN9_MIN,&min) && + !sensors_get_feature(*name,SENSORS_W83791D_IN9_MAX,&max)) { + if (valid) { + print_label(label,10); + printf("%+6.2f V (min = %+6.2f V, max = %+6.2f V) %s %s\n", + cur,min,max,alarms&W83791D_ALARM_IN9?"ALARM":" ", + beeps&W83791D_ALARM_IN9?"(beep)":"");OK if and only if
W83791D_ALARM_IN9is set to the right value.Quoted 35 lines
prog/sensors/chips.c 2006-07-30 22:26:05.000000000 -0700 renamed from prog/sensors/chips.c 2006-07-29 21:10:45.000000000 -0700 +28
-0+ } + } else + printf("ERROR: Can't get IN9 data!\n"); + free(label); + } + if (!sensors_get_label_and_valid(*name,SENSORS_W83781D_FAN1,&label,&valid) && !sensors_get_feature(*name,SENSORS_W83781D_FAN1,&cur) && !sensors_get_feature(*name,SENSORS_W83781D_FAN1_DIV,&fdiv) &&@@ -2382,6 +2399,35 @@ void print_w83781d(const sensors_chip_na free(label); } + if(is91d) { + if (!sensors_get_label_and_valid(*name,SENSORS_W83791D_FAN4,&label,&valid) && + !sensors_get_feature(*name,SENSORS_W83791D_FAN4,&cur) && + !sensors_get_feature(*name,SENSORS_W83791D_FAN4_DIV,&fdiv) && + !sensors_get_feature(*name,SENSORS_W83791D_FAN4_MIN,&min)) { + if (valid) { + print_label(label,10); + printf("%4.0f RPM (min = %4.0f RPM, div = %1.0f) %s %s\n", + cur,min,fdiv, alarms&W83791D_ALARM_FAN4?"ALARM":" ", + beeps&W83791D_ALARM_FAN4?"(beep)":""); + } + } else + printf("ERROR: Can't get FAN4 data!\n"); + free(label); + if (!sensors_get_label_and_valid(*name,SENSORS_W83791D_FAN5,&label,&valid) && + !sensors_get_feature(*name,SENSORS_W83791D_FAN5,&cur) && + !sensors_get_feature(*name,SENSORS_W83791D_FAN5_DIV,&fdiv) && + !sensors_get_feature(*name,SENSORS_W83791D_FAN5_MIN,&min)) { + if (valid) { + print_label(label,10); + printf("%4.0f RPM (min = %4.0f RPM, div = %1.0f) %s %s\n", + cur,min,fdiv, alarms&W83791D_ALARM_FAN5?"ALARM":" ",prog/sensors/chips.c 2006-07-30 22:26:05.000000000 -0700 void print_w83781d(const sensors_chip_na · line 2433
Quoted 10 lines
prog/sensors/chips.c 2006-07-30 22:26:05.000000000 -0700 renamed from prog/sensors/chips.c 2006-07-29 21:10:45.000000000 -0700 +7
-0+ beeps&W83791D_ALARM_FAN5?"(beep)":""); + } + } else + printf("ERROR: Can't get FAN5 data!\n"); + free(label); + } + if (!sensors_get_label_and_valid(*name,SENSORS_W83781D_TEMP1,&label,&valid) && !sensors_get_feature(*name,SENSORS_W83781D_TEMP1,&cur) && !sensors_get_feature(*name,SENSORS_W83781D_TEMP1_HYST,&min) &&This is OK but not sufficient as far as I can see. Because of the difference in alarm bits for in7 and in8, you also need to add specific code for the w83791d in the in7 and in8 cases.
I'd like to have these improvements merged soon now, can you please provide a new patch?
Thanks,
--
Jean Delvare
PATCH 1/1 Update lm_sensors to handle more of the
In reply to Charles SpirakisCharles,
Quoted 19 lines
I will try to get an update to you in the next few days. However, there is some ambiguity in the documentation which is causing some of the confusion...
If you look at the 791d beep enable register (
0xa3) and compare those bits to the alarm status register (0xab) you will see a discrepancy in the bit ordering.Sven and I tried to figure out if the documentation was correct or if one of the bit orderings provided were correct and it was hard to tell. It seemed like the bits for the enable did correspond to the bits for the alarm and we had reason to believe the enable bits were correct (which puts in7 and in8 at
0x10000and0x20000respectively - the same as the w83782d). In a side note, if you look at the interrupt mask bits (0x9d) they seem to follow the alarm status bit pattern and not the enable bit pattern but since the driver isn't using that register it's not overly significant other than interesting information.
Quoted 2 lines
I will go back and test this again just to verify what we think we saw is what we actually saw (or heard as the case may be).
Yeah I agree that there's some confusion in the datasheet. I don't have a chip to test myself. All I want is that the bits we define in the 2.4 driver and use in sensors/chips.c match the hardware reality. Else they are quite useless.
Quoted 4 lines
If anyone from winbond is reading this, would it be possible to get a definitive answer as to whether the documentation is right and (if not), to let us know what the correct bit patterns are for the alarm vs. the enable?
We have people from Winbond reading the list and helping where they can, but they certainly don't have the time to read everything. Please prepare precise questions, then we can ask them. Keep in mind though that the W83791D is a relatively old chip, and not very successful from a commercial perspective as far as can see, so Winbond must have it at low priority on their lists.
Quoted 5 lines
Along these same lines, the bit position for bit 1 and bit 13 are also swapped between what is shown for the enable vs. alarm. In that case, the documentation seems to be correct and the patch for the 2.6 driver is updated to reflect this.
Quoted 7 lines
Which leads to a different question. The assumption that was made for the driver patch was that we wanted the alarm and enable bits to match in user-space (and the driver would hide the problem if the chip didn't do this). Is this the correct assumption? Or should userspace get the values directly from the chip (so it matches the chip documentation) and let the user-space code deal with the bit twiddling?
That's probably the first time we encounter this problem so we have to decide now. The rule for alarms and beeps in 2.4 was indeed to export them in the most simple way for the driver (only masking out irrelevant bits.) And so far it happened that beep registers always matched alarm registers so it just happened that a single set of bit definitions would fit them both. If this ain't true anymore, we have to break one of the rules, i.e. either introduce new bit definitions for beeps, or cook the beep register values in the driver.
The most important thing to keep in mind here is that in the future, we want individual alarm and beep files anyway, so the decision we make here only matters for the 2.4 kernel drivers in the long run. In other words, it doesn't really matter. I'd go for the most simple solution, i.e. keep exporting the raw register values for both alarms and beeps, and introduce new defines for beeps where needed.
Quoted 4 lines
Oh, and I just noticed the inconsistency in the 2.6 w83791d driver patch in the Documentation file - needs to be:
14 - in9 (VINR1)
You're right.
Quoted 5 lines
Depending on your answer to the previous question (how do we handle the situation where the enable bit ordering is different from the alarm bit ordering), I will update or remove the patch... And yes, I understand this is yet another argument for updating this code to the standardized sysfs beep/alarm model. :)
Definitely. The w83791d currently only creates the old style alarm files, I would welcome a patch adding individual alarm and beep files.
Signature · 1 line
Jean Delvare
PATCH 1/1 Update lm_sensors to handle more of the
In reply to Charles SpirakisI wrote an ugly little python script to cause each alarm to trigger one at a time (I can provide it if anyone is interested). Based on the script, what I found was that the documentation was completely correct for the values I tested (in0-9, temp1-3, fan1-5). My board doesn't have the pwm stuff and the driver doesn't have that support, so I didn't validate the tart1-3 alarms/enables, but since everything else was correct, it seems likely those are as well.
Sven - Can you confirm this result?
Based on this result and the comment earlier that the driver should do a pass through of the values, I will create a documentation only change for the 2.6 driver and clean up the lm_sensors patch that started this thread.
Does this sound reasonable?
-- charles
in0 (VCORE) : alarms: 0x000001 beep_enable: 0x000001 in1 (VINR0) : alarms: 0x000002 beep_enable: 0x002000 <= mismatch in2 (+3.3VIN): alarms: 0x000004 beep_enable: 0x000004 in3 (5VDD) : alarms: 0x000008 beep_enable: 0x000008 in4 (+12VIN) : alarms: 0x000100 beep_enable: 0x000100 in5 (-12VIN) : alarms: 0x000200 beep_enable: 0x000200 in6 (-5VIN) : alarms: 0x000400 beep_enable: 0x000400 in7 (VSB) : alarms: 0x080000 beep_enable: 0x010000 <= mismatch in8 (VBAT) : alarms: 0x100000 beep_enable: 0x020000 <= mismatch in9 (VINR1) : alarms: 0x004000 beep_enable: 0x004000 temp1 : alarms: 0x000010 beep_enable: 0x000010 temp2 : alarms: 0x000020 beep_enable: 0x000020 temp3 : alarms: 0x002000 beep_enable: 0x000002 <= mismatch fan1 : alarms: 0x000040 beep_enable: 0x000040 fan2 : alarms: 0x000080 beep_enable: 0x000080 fan3 : alarms: 0x000800 beep_enable: 0x000800 fan4 : alarms: 0x200000 beep_enable: 0x200000 fan5 : alarms: 0x400000 beep_enable: 0x400000 tart1 : alarms: 0x010000 beep_enable: 0x040000 <= mismatch tart2 : alarms: 0x020000 beep_enable: 0x080000 <= mismatch tart3 : alarms: 0x040000 beep_enable: 0x100000 <= mismatch case open : alarms: 0x001000 beep_enable: 0x001000 user enable : alarms: -------- beep_enable: 0x800000 alarm_rsvd : alarms: 0x800000 beep_enable: -------- reserved : alarms: 0x008000 beep_enable: 0x008000
Quoted 89 lines · Jean Delvare
Charles,
Quoted 19 lines
I will try to get an update to you in the next few days. However, there is some ambiguity in the documentation which is causing some of the confusion...
If you look at the 791d beep enable register (
0xa3) and compare those bits to the alarm status register (0xab) you will see a discrepancy in the bit ordering.Sven and I tried to figure out if the documentation was correct or if one of the bit orderings provided were correct and it was hard to tell. It seemed like the bits for the enable did correspond to the bits for the alarm and we had reason to believe the enable bits were correct (which puts in7 and in8 at
0x10000and0x20000respectively - the same as the w83782d). In a side note, if you look at the interrupt mask bits (0x9d) they seem to follow the alarm status bit pattern and not the enable bit pattern but since the driver isn't using that register it's not overly significant other than interesting information.Quoted 2 lines
I will go back and test this again just to verify what we think we saw is what we actually saw (or heard as the case may be).
Yeah I agree that there's some confusion in the datasheet. I don't have a chip to test myself. All I want is that the bits we define in the 2.4 driver and use in
sensors/chips.cmatch the hardware reality. Else they are quite useless.Quoted 4 lines
If anyone from winbond is reading this, would it be possible to get a definitive answer as to whether the documentation is right and (if not), to let us know what the correct bit patterns are for the alarm vs. the enable?
We have people from Winbond reading the list and helping where they can, but they certainly don't have the time to read everything. Please prepare precise questions, then we can ask them. Keep in mind though that the W83791D is a relatively old chip, and not very successful from a commercial perspective as far as can see, so Winbond must have it at low priority on their lists.
Quoted 5 lines
Along these same lines, the bit position for bit 1 and bit 13 are also swapped between what is shown for the enable vs. alarm. In that case, the documentation seems to be correct and the patch for the 2.6 driver is updated to reflect this.
Quoted 7 lines
Which leads to a different question. The assumption that was made for the driver patch was that we wanted the alarm and enable bits to match in user-space (and the driver would hide the problem if the chip didn't do this). Is this the correct assumption? Or should userspace get the values directly from the chip (so it matches the chip documentation) and let the user-space code deal with the bit twiddling?
That's probably the first time we encounter this problem so we have to decide now. The rule for alarms and beeps in 2.4 was indeed to export them in the most simple way for the driver (only masking out irrelevant bits.) And so far it happened that beep registers always matched alarm registers so it just happened that a single set of bit definitions would fit them both. If this ain't true anymore, we have to break one of the rules, i.e. either introduce new bit definitions for beeps, or cook the beep register values in the driver.
The most important thing to keep in mind here is that in the future, we want individual alarm and beep files anyway, so the decision we make here only matters for the 2.4 kernel drivers in the long run. In other words, it doesn't really matter. I'd go for the most simple solution, i.e. keep exporting the raw register values for both alarms and beeps, and introduce new defines for beeps where needed.
Quoted 4 lines
Oh, and I just noticed the inconsistency in the 2.6 w83791d driver patch in the Documentation file - needs to be:
14 - in9 (VINR1)
You're right.
Quoted 5 lines
Depending on your answer to the previous question (how do we handle the situation where the enable bit ordering is different from the alarm bit ordering), I will update or remove the patch... And yes, I understand this is yet another argument for updating this code to the standardized sysfs beep/alarm model. :)
Definitely. The w83791d currently only creates the old style alarm files, I would welcome a patch adding individual alarm and beep files.
--
Jean Delvare
PATCH 1/1 Update lm_sensors to handle more of the
In reply to Charles SpirakisHi Charles,
Quoted 9 lines
I wrote an ugly little python script to cause each alarm to trigger one at a time (I can provide it if anyone is interested). Based on the script, what I found was that the documentation was completely correct for the values I tested (in0-9, temp1-3, fan1-5). My board doesn't have the pwm stuff and the driver doesn't have that support, so I didn't validate the tart1-3 alarms/enables, but since everything else was correct, it seems likely those are as well.
Sven - Can you confirm this result?
Maybe you can share your script with Sven so that he can test quickly. A confirmation would indeed be welcome, as I know there are different revisions of the W83791D chip.
Quoted 6 lines
Based on this result and the comment earlier that the driver should do a pass through of the values, I will create a documentation only change for the 2.6 driver and clean up the
lm_sensorspatch that started this thread.Does this sound reasonable?
Yes, sounds like a good plan to me. I'm waiting for your patches :)
Signature · 1 line
Jean Delvare
PATCH 1/1 Update lm_sensors to handle more of the
In reply to Charles SpirakisFrom: Charles Spirakis <bezaur at gmail.com>
Modified patch from Sven Anders to enhance w83791d support. Adds output for in7, in8, in9, fan4 and fan5. Also updated to display the (beep) properly due to the fact that the w83791d beep enable mask is different than the alarm mask.
Signed-off by: Sven Anders <anders at anduras.de> Signed-off by: Charles Spirakis <bezaur at gmail.com>
---
kernel/chips/w83781d.c | 18 +++++++-- prog/sensors/chips.c | 94
++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 103 insertions(+), 9 deletions(-)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lm-sensors-r4079-20060730_sven_changes.patch
Type: text/x-patch
Size: 8721 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060816/43b7b6d7/attachment.bin
Jean Delvare
AppliedPATCH 1/1 Update lm_sensors to handle more of the
In reply to Charles SpirakisHi Charles, Sven,
Quoted 9 lines
From: Charles Spirakis <bezaur at gmail.com>
Modified patch from Sven Anders to enhance w83791d support. Adds output for in7, in8, in9, fan4 and fan5. Also updated to display the (beep) properly due to the fact that the w83791d beep enable mask is different than the alarm mask.
Signed-off by: Sven Anders <anders at anduras.de> Signed-off by: Charles Spirakis <bezaur at gmail.com>
Applied, thanks.
Signature · 1 line
Jean Delvare
PATCH 1/1 Update lm_sensors to handle more of the
In reply to Charles Spirakis-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 9 lines · Charles Spirakis
I wrote an ugly little python script to cause each alarm to trigger one at a time (I can provide it if anyone is interested). Based on the script, what I found was that the documentation was completely correct for the values I tested (in0-9, temp1-3, fan1-5). My board doesn't have the pwm stuff and the driver doesn't have that support, so I didn't validate the tart1-3 alarms/enables, but since everything else was correct, it seems likely those are as well.
Sven - Can you confirm this result?
Hello Charles!
I can confirm in1 and temp3, but for in7/in8 the beeps comes on the first "enabling beep"...
Hope this helps.
Regards
Sven
- --8X-------------------------------------------------------------------
me-fw-int-01:/root # ./check_beep.py
in0_input: alarm: 0x000001 enable: 0x000001
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
in1_input: alarm: 0x000002 enable: 0x002000 <= mismatchenabling beep via enable bit for 10 seconds disabling beep via enable bit
due to misatch, enabling beep via alarm bit for 10 seconds disabling beep via alarm bit
in2_input: alarm: 0x000004 enable: 0x000004
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
in3_input: alarm: 0x000008 enable: 0x000008
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
in4_input: alarm: 0x000100 enable: 0x000100
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
in5_input: alarm: 0x000200 enable: 0x000200
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
in6_input: alarm: 0x000400 enable: 0x000400
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
in7_input: alarm: 0x080000 enable: 0x010000 <= mismatchenabling beep via enable bit for 10 seconds ** <= here it beeps!
disabling beep via enable bit
due to misatch, enabling beep via alarm bit for 10 seconds disabling beep via alarm bit
in8_input: alarm: 0x100000 enable: 0x020000 <= mismatch
enabling beep via enable bit for 10 seconds ** <== here it beeps!
disabling beep via enable bit
due to misatch, enabling beep via alarm bit for 10 seconds disabling beep via alarm bit
in9_input: alarm: 0x004000 enable: 0x004000
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
temp1_input: alarm: 0x000010 enable: 0x000010
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
temp2_input: alarm: 0x000020 enable: 0x000020
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
temp3_input: alarm: 0x002000 enable: 0x000002 <= mismatchenabling beep via enable bit for 10 seconds disabling beep via enable bit
due to misatch, enabling beep via alarm bit for 10 seconds disabling beep via alarm bit
fan1_input: alarm: 0x000040 enable: 0x000040
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
fan1_input: alarm: 0x000040 enable: 0x000040
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
fan2_input: alarm: 0x000080 enable: 0x000080
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
fan3_input: alarm: 0x000800 enable: 0x000800
enabling beep via enable bit for 10 seconds
disabling beep via enable bit
fan4_input: alarm: 0x200000 enable: 0x200000
enabling beep via enable bit for 10 seconds ?? <== Strange beep here!
disabling beep via enable bit
fan5_input: alarm: 0x400000 enable: 0x400000enabling beep via enable bit for 10 seconds skipping tart1_input, enable bit is 18 (262144 = 0x040000) skipping tart2_input, enable bit is 19 (524288 = 0x080000) skipping tart3_input, enable bit is 20 (1048576 = 0x100000) skipping case_open, enable bit is 12 (4096 = 0x001000) skipping user_enable, enable bit is 23 (8388608 = 0x800000)
- --
Sven Anders <anders at anduras.de> () Ascii Ribbon Campaign
/\ Support plain text e-mail
ANDURAS service solutions AG
Innstra?e 71 - 94036 Passau - Germany
Web: www.anduras.de - Tel: +49 (0)851-4 90 50-0 - Fax: +49 (0)851-4 90 50-55
Rechtsform: Aktiengesellschaft - Sitz: Passau - Amtsgericht Passau HRB 6032 Mitglieder des Vorstands: Sven Anders, Marcus Junker, Michael Sch?n Vorsitzender des Aufsichtsrats: Dipl. Kfm. Thomas Tr?ger -----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.1 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFE7bLG5lKZ7Feg4EcRApgwAJ9/SdxWJi3ebaMXaTlkiJWPNTtIDwCeOnwD GtCbEfQGpGUy2FGc40nSdJs=La5G
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: anders.vcf
Type: text/x-vcard
Size: 339 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060824/e089f1b9/attachment-0001.vcf
PATCH 1/1 Update lm_sensors to handle more of the
In reply to Charles SpirakisBeeping on "enable bit" means that the enable bit position in the document turned on the beep noise vs. "alarm bit" which tries using the alarm bit position. So it sounds like you are confirming the documentation is correct.
Thanks!
-- charles
Quoted 117 lines · Sven Anders
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1Quoted 9 lines · Charles Spirakis
I wrote an ugly little python script to cause each alarm to trigger one at a time (I can provide it if anyone is interested). Based on the script, what I found was that the documentation was completely correct for the values I tested (in0-9, temp1-3, fan1-5). My board doesn't have the pwm stuff and the driver doesn't have that support, so I didn't validate the tart1-3 alarms/enables, but since everything else was correct, it seems likely those are as well.
Sven - Can you confirm this result?
Hello Charles!
I can confirm in1 and temp3, but for in7/in8 the beeps comes on the first "enabling beep"...
Hope this helps.
Regards
Sven- --8X-------------------------------------------------------------------
me-fw-int-01:/root # ./check_beep.py
in0_input: alarm: 0x000001 enable: 0x000001 enabling beep via enable bit for 10 seconds disabling beep via enable bit in1_input: alarm: 0x000002 enable: 0x002000 <= mismatchenabling beep via enable bit for 10 seconds disabling beep via enable bit
due to misatch, enabling beep via alarm bit for 10 seconds disabling beep via alarm bitin2_input: alarm: 0x000004 enable: 0x000004 enabling beep via enable bit for 10 seconds disabling beep via enable bit in3_input: alarm: 0x000008 enable: 0x000008 enabling beep via enable bit for 10 seconds disabling beep via enable bit in4_input: alarm: 0x000100 enable: 0x000100 enabling beep via enable bit for 10 seconds disabling beep via enable bit in5_input: alarm: 0x000200 enable: 0x000200 enabling beep via enable bit for 10 seconds disabling beep via enable bit in6_input: alarm: 0x000400 enable: 0x000400 enabling beep via enable bit for 10 seconds disabling beep via enable bit in7_input: alarm: 0x080000 enable: 0x010000 <= mismatchenabling beep via enable bit for 10 seconds ** <= here it beeps!
disabling beep via enable bit
due to misatch, enabling beep via alarm bit for 10 seconds disabling beep via alarm bitin8_input: alarm: 0x100000 enable: 0x020000 <= mismatchenabling beep via enable bit for 10 seconds ** <== here it beeps!
disabling beep via enable bit
due to misatch, enabling beep via alarm bit for 10 seconds disabling beep via alarm bitin9_input: alarm: 0x004000 enable: 0x004000 enabling beep via enable bit for 10 seconds disabling beep via enable bit temp1_input: alarm: 0x000010 enable: 0x000010 enabling beep via enable bit for 10 seconds disabling beep via enable bit temp2_input: alarm: 0x000020 enable: 0x000020 enabling beep via enable bit for 10 seconds disabling beep via enable bit temp3_input: alarm: 0x002000 enable: 0x000002 <= mismatchenabling beep via enable bit for 10 seconds disabling beep via enable bit
due to misatch, enabling beep via alarm bit for 10 seconds disabling beep via alarm bitfan1_input: alarm: 0x000040 enable: 0x000040 enabling beep via enable bit for 10 seconds disabling beep via enable bit fan1_input: alarm: 0x000040 enable: 0x000040 enabling beep via enable bit for 10 seconds disabling beep via enable bit fan2_input: alarm: 0x000080 enable: 0x000080 enabling beep via enable bit for 10 seconds disabling beep via enable bit fan3_input: alarm: 0x000800 enable: 0x000800 enabling beep via enable bit for 10 seconds disabling beep via enable bit fan4_input: alarm: 0x200000 enable: 0x200000 enabling beep via enable bit for 10 seconds ?? <== Strange beep here! disabling beep via enable bit fan5_input: alarm: 0x400000 enable: 0x400000enabling beep via enable bit for 10 seconds skipping
tart1_input, enable bit is 18 (262144 =0x040000) skippingtart2_input, enable bit is 19 (524288 =0x080000) skippingtart3_input, enable bit is 20 (1048576 =0x100000) skippingcase_open, enable bit is 12 (4096 =0x001000) skippinguser_enable, enable bit is 23 (8388608 =0x800000)- --
Sven Anders <anders at anduras.de> () Ascii Ribbon Campaign
/\ Support plain text
ANDURAS service solutions AG
Innstra?e 71 - 94036 Passau - Germany
Web: www.anduras.de - Tel: +49 (0)851-4 90 50-0 - Fax: +49 (0)851-4 90 50-55Rechtsform: Aktiengesellschaft - Sitz: Passau - Amtsgericht Passau HRB 6032
Mitglieder des Vorstands: Sven Anders, Marcus Junker, Michael Sch?n Vorsitzender des Aufsichtsrats: Dipl. Kfm. Thomas Tr?ger -----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.1 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
Quoted 6 lines
iD8DBQFE7bLG5lKZ7Feg4EcRApgwAJ9/SdxWJi3ebaMXaTlkiJWPNTtIDwCeOnwD GtCbEfQGpGUy2FGc40nSdJs> =La5G
-----END PGP SIGNATURE-----
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060824/6c4713b1/attachment-0001.html