Skip to contentSkip to conversation

Update lm_sensors to handle more of the

PATCH New Charles Spirakis 3 participants 10 messages Aug 3, 2006 – Aug 24, 2006

lore
Share & tools
lore
#0 lore raw

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

#1 lore raw

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.c lm-sensors-r4079-20060730_sven_changes/prog/sensors/chips.c

prog/sensors/chips.c +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 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_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 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
#2 lore raw

PATCH 1/1 Update lm_sensors to handle more of the

In reply to Charles Spirakis

Jean --

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 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.c lm-sensors-r4079-20060730_sven_changes/prog/sensors/chips.c

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 +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_IN9 is 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

#3 lore raw

PATCH 1/1 Update lm_sensors to handle more of the

In reply to Charles Spirakis

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 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.

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
#4 lore raw

PATCH 1/1 Update lm_sensors to handle more of the

In reply to 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?

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 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.

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.

--
Jean Delvare

#5 lore raw

PATCH 1/1 Update lm_sensors to handle more of the

In reply to Charles Spirakis

Hi 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_sensors patch 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
#6 lore raw

PATCH 1/1 Update lm_sensors to handle more of the

In reply to Charles Spirakis

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>

---

 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

Applied
#7 lore raw

PATCH 1/1 Update lm_sensors to handle more of the

In reply to Charles Spirakis

Hi 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
#8 lore raw

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 <= mismatch

enabling 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 <= 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

       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 <= mismatch

enabling 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: 0x400000

enabling 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

#9 lore raw

PATCH 1/1 Update lm_sensors to handle more of the

In reply to Charles Spirakis

Beeping 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: 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 <= mismatch

enabling 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 <= 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

       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 <= mismatch

enabling 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: 0x400000

enabling 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

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