Re: 2.6 - sysfs sensor nameing inconsistency

[Andrey Borzenkov <arvidjaar@mail.ru>]

[-- Attachment #1: Type: text/plain, Size: 4900 bytes --]

On Tuesday 19 August 2003 23:45, Greg KH wrote:
> On Tue, Aug 19, 2003 at 11:19:01PM +0400, Andrey Borzenkov wrote:
[...]
> > There are more issues than just type_name.
[...]
> > 1. I do not know where to get information on adapters (called busses
> > actually in libsensors) and algorithms. I.e. the information
> > corresponding to 2.4:
> >
> > {pts/2}% cat ~/tmp/i2c-bus
> > i2c-0   smbus           SMBus I801 adapter at e800              Non-I2C
> > SMBus adapter
>
> Look in /sys/class/i2c-adapter/
[...]
> Hm, a name file:
> $ cat /sys/class/i2c-adapter/i2c-0/device/name
> SMBus I801 adapter at 8000
>
> Ah, the same info as you showed for 2.4 :)
>

Sure it not the same info it is only part of it. I do not know if libsensors 
just using this for display or needs it internally. Thank you for pointer 
though.

> > 2. I do not know - and sysfs does not provide any information - how to
> > identify chips-of-interest as opposed to generic i2c devices. I.e.
> > w83781d has both clients and subclients (2.4 again):
> >
> > {pts/2}% cat ~/tmp/i2c-0-bus
> > 2d      AS99127F chip                           W83781D sensor driver
> > 48      AS99127F subclient                      W83781D sensor driver
> > 49      AS99127F subclient                      W83781D sensor driver
> >
[...]
>
> That's what you are going to have to set the name file to in the
> i2c_client structure, much like your patch did.  Then look at the
> different name files in each device directory to see what kind of device
> it is (chip, subclient, etc.)
>

OK attached patch sets all names to just chip name for chips themselves and 
"chipname subclient" when subclient ios registered.

> > 3. libsensors asks for hysteresis value. This one does not exist in sysfs
> > (so all temp readings fail). Is it emulated by kernel or read off chip?
>
> The kernel is exporting all of the info that it knows about through
> sysfs.

it just arbitrarily (re-)names them. "min" is not hysteresis; name is badly 
chosen.
[...]
> > 4. I do not have the slightest idea how ISA adapters look like in sysfs
> > and where they are located. Anyone can give me example?
>
> They show up on the legacy bus:
>
> $ tree /sys/class/i2c-adapter/i2c-1/
> /sys/class/i2c-adapter/i2c-1/
>
> |-- device -> ../../../devices/legacy/i2c-1
>

This does not help much. Libsensors expects as adapter identification either 
"i2c-N" or "isa". If I set it to "isa" I do not have any way to determine 
sysfs path except by rescanning /sys/class/i2c-adapter every time. Having 
/sys/class/i2-adapter/isa/... would be better, apparently it is assumed that 
only one such adapter can exist.
[...]
> > So the patch to ibsensors is actually trivial enough. Main issue is
> > contents of sysfs
>
> You might want to take a look at libsysfs, it makes finding all of the
> devices and attributes in the sysfs tree a whole lot easer.
>

No I did not want to make code dependent on libsysfs, besides arsing itself is 
trivial.

Attached is patch against 2.6.0-test3-bk8 that sets all chip names to simply 
chip names :) and patch for libsensors that makes use of it. libsensors still 
needs to be built under 2.4, no attempt is made to allow building under 2.6. 
I built it using make LINUX_HADERS=/home/bor/src/linux-2.4.21/linux 
COMPILE_KERNEL=0. It is possible to get rid of this dependency but I'd like 
to settle sysfs issue first. As can be seen the ugliest part is name 
translation between sysfs attributes and libsensors feature names ... if 
attributes were named consistently the whole would be much more simple.

This works perfectly for reading, writing does not work here but it is not my 
patch. Reading:
{pts/2}% LD_LIBRARY_PATH=./lib ./prog/sensors/sensors
as99127f-i2c-0-2d
Adapter: SMBus I801 adapter at e800
Algorithm: Not available via sysfs
VCore 1:   +1.70 V  (min =  +1.49 V, max =  +1.81 V)
+3.3V:     +3.47 V  (min =  +2.98 V, max =  +3.63 V)
+5V:       +5.00 V  (min =  +4.52 V, max =  +5.48 V)
+12V:     +11.37 V  (min = +10.82 V, max = +13.13 V)
-12V:     -11.57 V  (min = -12.33 V, max = -15.07 V)       ALARM
-5V:       -5.03 V  (min =  -4.50 V, max =  -5.49 V)
CPU:      4787 RPM  (min = 3000 RPM, div = 2)
Front:    2922 RPM  (min = 3000 RPM, div = 2)              ALARM
MB:          +28╟C  (limit = +127╟C, hysteresis =  +60╟C)
CPU:       +36.0╟C  (limit = +111╟C, hysteresis =  +97╟C)
HDD:       +37.0╟C  (limit = +120╟C, hysteresis = +100╟C)
vid:      +1.650 V
alarms:
beep_enable:
          Sound alarm enabled

writing:

{pts/2}% cat /sys/class/i2c-adapter/i2c-0/device/0-002d/in_max2
3632
{pts/2}% sudo zsh -c 'echo 3500 > 
/sys/class/i2c-adapter/i2c-0/device/0-002d/in_max2'
{pts/2}% cat /sys/class/i2c-adapter/i2c-0/device/0-002d/in_max2
400

so writing is broken at least for w83781d driver

-andrey

[-- Attachment #2: 2.6.0-test3-bk8-sensors_type_name-3.patch --]
[-- Type: text/x-diff, Size: 6288 bytes --]

--- linux-2.6.0-test3-bk8/drivers/i2c/chips/adm1021.c.chip_name	2003-08-20 20:58:39.000000000 +0400
+++ linux-2.6.0-test3-bk8/drivers/i2c/chips/adm1021.c	2003-08-23 21:30:57.000000000 +0400
@@ -225,7 +225,6 @@ static int adm1021_detect(struct i2c_ada
 	struct adm1021_data *data;
 	int err = 0;
 	const char *type_name = "";
-	const char *client_name = "";
 
 	/* Make sure we aren't probing the ISA bus!! This is just a safety check
 	   at this moment; i2c_detect really won't call us. */
@@ -291,28 +290,20 @@ static int adm1021_detect(struct i2c_ada
 
 	if (kind == max1617) {
 		type_name = "max1617";
-		client_name = "MAX1617 chip";
 	} else if (kind == max1617a) {
 		type_name = "max1617a";
-		client_name = "MAX1617A chip";
 	} else if (kind == adm1021) {
 		type_name = "adm1021";
-		client_name = "ADM1021 chip";
 	} else if (kind == adm1023) {
 		type_name = "adm1023";
-		client_name = "ADM1023 chip";
 	} else if (kind == thmc10) {
 		type_name = "thmc10";
-		client_name = "THMC10 chip";
 	} else if (kind == lm84) {
 		type_name = "lm84";
-		client_name = "LM84 chip";
 	} else if (kind == gl523sm) {
 		type_name = "gl523sm";
-		client_name = "GL523SM chip";
 	} else if (kind == mc1066) {
 		type_name = "mc1066";
-		client_name = "MC1066 chip";
 	} else {
 		dev_err(&adapter->dev, "Internal error: unknown kind (%d)?!?",
 			kind);
@@ -320,7 +311,7 @@ static int adm1021_detect(struct i2c_ada
 	}
 
 	/* Fill in the remaining client fields and put it into the global list */
-	strlcpy(new_client->name, client_name, DEVICE_NAME_SIZE);
+	strlcpy(new_client->name, type_name, DEVICE_NAME_SIZE);
 	data->type = kind;
 
 	new_client->id = adm1021_id++;
--- linux-2.6.0-test3-bk8/drivers/i2c/chips/it87.c.chip_name	2003-08-09 13:10:04.000000000 +0400
+++ linux-2.6.0-test3-bk8/drivers/i2c/chips/it87.c	2003-08-23 21:31:30.000000000 +0400
@@ -592,7 +592,6 @@ int it87_detect(struct i2c_adapter *adap
 	struct it87_data *data;
 	int err = 0;
 	const char *name = "";
-	const char *client_name = "";
 	int is_isa = i2c_is_isa_adapter(adapter);
 
 	if (!is_isa && 
@@ -681,10 +680,8 @@ int it87_detect(struct i2c_adapter *adap
 
 	if (kind == it87) {
 		name = "it87";
-		client_name = "IT87 chip";
 	} /* else if (kind == it8712) {
 		name = "it8712";
-		client_name = "IT87-J chip";
 	} */ else {
 		dev_dbg(&adapter->dev, "Internal error: unknown kind (%d)?!?",
 			kind);
--- linux-2.6.0-test3-bk8/drivers/i2c/chips/lm78.c.chip_name	2003-08-09 13:10:04.000000000 +0400
+++ linux-2.6.0-test3-bk8/drivers/i2c/chips/lm78.c	2003-08-23 21:32:17.000000000 +0400
@@ -625,11 +625,11 @@ int lm78_detect(struct i2c_adapter *adap
 	}
 
 	if (kind == lm78) {
-		client_name = "LM78 chip";
+		client_name = "lm78";
 	} else if (kind == lm78j) {
-		client_name = "LM78-J chip";
+		client_name = "lm78-j";
 	} else if (kind == lm79) {
-		client_name = "LM79 chip";
+		client_name = "lm79";
 	} else {
 		dev_dbg(&adapter->dev, "Internal error: unknown kind (%d)?!?",
 			kind);
--- linux-2.6.0-test3-bk8/drivers/i2c/chips/lm85.c.chip_name	2003-08-09 13:10:04.000000000 +0400
+++ linux-2.6.0-test3-bk8/drivers/i2c/chips/lm85.c	2003-08-23 21:32:59.000000000 +0400
@@ -853,24 +853,20 @@ int lm85_detect(struct i2c_adapter *adap
 	/* Fill in the chip specific driver values */
 	if ( kind == any_chip ) {
 		type_name = "lm85";
-		strlcpy(new_client->name, "Generic LM85", DEVICE_NAME_SIZE);
 	} else if ( kind == lm85b ) {
 		type_name = "lm85b";
-		strlcpy(new_client->name, "National LM85-B", DEVICE_NAME_SIZE);
 	} else if ( kind == lm85c ) {
 		type_name = "lm85c";
-		strlcpy(new_client->name, "National LM85-C", DEVICE_NAME_SIZE);
 	} else if ( kind == adm1027 ) {
 		type_name = "adm1027";
-		strlcpy(new_client->name, "Analog Devices ADM1027", DEVICE_NAME_SIZE);
 	} else if ( kind == adt7463 ) {
 		type_name = "adt7463";
-		strlcpy(new_client->name, "Analog Devices ADT7463", DEVICE_NAME_SIZE);
 	} else {
 		dev_dbg(&adapter->dev, "Internal error, invalid kind (%d)!", kind);
 		err = -EFAULT ;
 		goto ERROR1;
 	}
+	strlcpy(new_client->name, type_name, DEVICE_NAME_SIZE);
 
 	/* Fill in the remaining client fields */
 	new_client->id = lm85_id++;
--- linux-2.6.0-test3-bk8/drivers/i2c/chips/via686a.c.chip_name	2003-08-09 13:10:05.000000000 +0400
+++ linux-2.6.0-test3-bk8/drivers/i2c/chips/via686a.c	2003-08-23 21:33:22.000000000 +0400
@@ -671,7 +671,7 @@ static int via686a_detect(struct i2c_ada
 	struct i2c_client *new_client;
 	struct via686a_data *data;
 	int err = 0;
-	const char client_name[] = "via686a chip";
+	const char client_name[] = "via686a";
 	u16 val;
 
 	/* Make sure we are probing the ISA bus!!  */
--- linux-2.6.0-test3-bk8/drivers/i2c/chips/w83781d.c.chip_name	2003-08-09 13:10:05.000000000 +0400
+++ linux-2.6.0-test3-bk8/drivers/i2c/chips/w83781d.c	2003-08-23 21:34:51.000000000 +0400
@@ -1098,15 +1098,15 @@ w83781d_detect_subclients(struct i2c_ada
 	}
 
 	if (kind == w83781d)
-		client_name = "W83781D subclient";
+		client_name = "w83781d subclient";
 	else if (kind == w83782d)
-		client_name = "W83782D subclient";
+		client_name = "w83782d subclient";
 	else if (kind == w83783s)
-		client_name = "W83783S subclient";
+		client_name = "w83783s subclient";
 	else if (kind == w83627hf)
-		client_name = "W83627HF subclient";
+		client_name = "w83627hf subclient";
 	else if (kind == as99127f)
-		client_name = "AS99127F subclient";
+		client_name = "as99127f subclient";
 	else
 		client_name = "unknown subclient?";
 
@@ -1304,20 +1304,20 @@ w83781d_detect(struct i2c_adapter *adapt
 	}
 
 	if (kind == w83781d) {
-		client_name = "W83781D chip";
+		client_name = "w83781d";
 	} else if (kind == w83782d) {
-		client_name = "W83782D chip";
+		client_name = "w83782d";
 	} else if (kind == w83783s) {
-		client_name = "W83783S chip";
+		client_name = "w83783s";
 	} else if (kind == w83627hf) {
 		if (val1 == 0x90)
-			client_name = "W83627THF chip";
+			client_name = "w83627thf";
 		else
-			client_name = "W83627HF chip";
+			client_name = "w83627hf";
 	} else if (kind == as99127f) {
-		client_name = "AS99127F chip";
+		client_name = "as99127f";
 	} else if (kind == w83697hf) {
-		client_name = "W83697HF chip";
+		client_name = "w83697hf";
 	} else {
 		dev_err(&new_client->dev, "Internal error: unknown "
 						"kind (%d)?!?", kind);

[-- Attachment #3: lm_sensors-2.8.0-sysfs.patch --]
[-- Type: text/x-diff, Size: 7303 bytes --]

--- lm_sensors-2.8.0/lib/proc.c.sysfs	2003-06-09 01:33:38.000000000 +0400
+++ lm_sensors-2.8.0/lib/proc.c	2003-08-23 22:23:33.000000000 +0400
@@ -20,6 +20,8 @@
 #include <stddef.h>
 #include <stdio.h>
 #include <string.h>
+#include <dirent.h>
+#include <unistd.h>
 #include <sys/sysctl.h>
 #include "kernel/include/sensors.h"
 #include "data.h"
@@ -59,6 +61,158 @@ static int sensors_get_chip_id(sensors_c
                                        &sensors_proc_bus_max,\
                                        sizeof(struct sensors_bus))
 
+static int using_sysfs;
+static char sysfs_i2c_bus[] = "/sys/bus/i2c/devices";
+static char sysfs_i2c_class[] = "/sys/class/i2c-adapter";
+
+/* This parses sysfs tree for available i2c chips */
+static int sensors_parse_sysfs_chips(void)
+{
+  DIR *dir;
+  struct dirent *dentry;
+  char path[256], buf[256];
+  FILE *f;
+  int len;
+  sensors_proc_chips_entry entry;
+
+  if ((dir = opendir(sysfs_i2c_bus)) == NULL)
+    return -SENSORS_ERR_PROC;
+
+  using_sysfs = 1;
+
+  while ((dentry = readdir(dir)) != NULL) {
+    if (dentry->d_name[0] == '.' || dentry->d_ino <= 0)
+      continue;
+    snprintf(path, sizeof(path), "%s/%s/name", sysfs_i2c_bus, dentry->d_name);
+    if ((f = fopen(path, "r")) == NULL)
+      continue;
+    buf[0] = '\0';
+    fgets(buf, sizeof(buf), f);
+    fclose(f);
+    len = strlen(buf) - 1;
+    if (buf[len] == '\n')
+      buf[len] = '\0';
+    if (strchr(buf, ' '))
+      continue; /* e.g. "as99127f subclient" */
+    sscanf(dentry->d_name, "%d-%x", &entry.name.bus, &entry.name.addr);
+    entry.name.prefix = strdup(buf);
+
+    add_proc_chips(&entry);
+  }
+  closedir(dir);
+  return 0;
+}
+
+static int sensors_parse_sysfs_class(void)
+{
+  DIR *dir;
+  struct dirent *dentry;
+  char path[256], buf[256];
+  FILE *f;
+  int len;
+  sensors_bus entry;
+
+  if ((dir = opendir(sysfs_i2c_class)) == NULL)
+    return -SENSORS_ERR_PROC;
+
+  while ((dentry = readdir(dir)) != NULL) {
+    if (dentry->d_name[0] == '.' || dentry->d_ino <= 0)
+      continue;
+
+    sscanf(dentry->d_name, "i2c-%d", &entry.number);
+    entry.algorithm = strdup("Not available via sysfs");
+    sprintf(path, "%s/%s/device/name", sysfs_i2c_class, dentry->d_name);
+    if ((f = fopen(path, "r")) == NULL) {
+      entry.adapter = strdup("UNKNOWN");
+      continue;
+    }
+    fgets(buf, sizeof(buf), f);
+    fclose(f);
+    len = strlen(buf) - 1;
+    if (buf[len] == '\n')
+      buf[len] = '\0';
+    entry.adapter = strdup(buf);
+    add_bus(&entry);
+  }
+
+  return 0;
+}
+  
+static int ends_with(const char *str, const char *suffix)
+{
+  int len = strlen(str);
+  int slen = strlen(suffix);
+
+  if (len <= slen)
+    return 0;
+
+  return strcmp(str + len - slen, suffix) == 0;
+}
+
+static void
+sensors_sysfs_convert_feature_name(const char *name, char *buf)
+{
+  char part1[256], part2[256];
+  int n;
+
+  if (sscanf(name, "temp%d", &n) == 1) {
+    if (ends_with(name, "_hyst"))
+      sprintf(buf, "temp_min%d", n);
+    else if (ends_with(name, "_over"))
+      sprintf(buf, "temp_max%d", n);
+    else
+      sprintf(buf, "temp_input%d", n);
+  } else if (sscanf(name, "%[^0-9]%d_%s", part1, &n, part2) == 3)
+    sprintf(buf, "%s_%s%d", part1, part2, n);
+  else if (sscanf(name, "%[^0-9]%d", part1, &n) == 2)
+    sprintf(buf, "%s_input%d", part1, n);
+  else if (strcmp(name, "beeps") == 0)
+    strcpy(buf, "beep_mask");
+  else
+    strcpy(buf, name);
+}
+
+/* This reads a feature /proc file */
+static int sensors_read_sysfs(sensors_chip_name name,
+  const sensors_chip_feature *the_feature, double *value)
+{
+  char path[256], buf[256];
+  FILE *f;
+  long l;
+  
+  sensors_sysfs_convert_feature_name(the_feature->name, buf);
+  snprintf(path, sizeof(path), "%s/%d-%04x/%s", sysfs_i2c_bus, name.bus, name.addr, buf);
+  if ((f = fopen(path, "r")) == NULL)
+    return -SENSORS_ERR_PROC;
+  fscanf(f, "%ld", &l);
+  fclose(f);
+  *value = l;
+  if (strncmp(buf, "in_", strlen("in_")) == 0
+      || strncmp(buf, "temp_", strlen("temp_")) == 0)
+    *value /= 10.0;
+  return 0;
+}
+  
+static int sensors_write_sysfs(sensors_chip_name name,
+  const sensors_chip_feature *the_feature, double value)
+{
+  char path[256], buf[256];
+  FILE *f;
+  long l;
+  
+  sensors_sysfs_convert_feature_name(the_feature->name, buf);
+  snprintf(path, sizeof(path), "%s/%d-%04x/%s", sysfs_i2c_bus, name.bus, name.addr, buf);
+  if (strncmp(buf, "in_", strlen("in_")) == 0
+      || strncmp(buf, "temp_", strlen("temp_")) == 0)
+    value *= 10.0;
+  l = (long) value;
+  if ((f = fopen(path, "w")) == NULL)
+    return -SENSORS_ERR_PROC;
+  fprintf(f, "%ld", l);
+  fclose(f);
+
+  return 0;
+}
 /* This reads /proc/sys/dev/sensors/chips into memory */
 int sensors_read_proc_chips(void)
 {
@@ -69,7 +223,7 @@ int sensors_read_proc_chips(void)
   int res,lineno;
 
   if (sysctl(name, 3, bufptr, &buflen, NULL, 0))
-    return -SENSORS_ERR_PROC;
+    return sensors_parse_sysfs_chips();
 
   lineno = 1;
   while (buflen >= sizeof(struct i2c_chips_data)) {
@@ -96,6 +250,9 @@ int sensors_read_proc_bus(void)
   sensors_bus entry;
   int lineno;
 
+  if (using_sysfs)
+    return sensors_parse_sysfs_class();
+
   f = fopen("/proc/bus/i2c","r");
   if (!f)
     return -SENSORS_ERR_PROC;
@@ -158,10 +315,17 @@ int sensors_read_proc(sensors_chip_name 
     return sysctl_name[2];
   if (! (the_feature = sensors_lookup_feature_nr(name.prefix,feature)))
     return -SENSORS_ERR_NO_ENTRY;
-  sysctl_name[3] = the_feature->sysctl;
-  if (sysctl(sysctl_name, 4, buf, &buflen, NULL, 0))
-    return -SENSORS_ERR_PROC;
-  *value = *((long *) (buf + the_feature->offset));
+
+  if (using_sysfs) {
+    int ret = sensors_read_sysfs(name, the_feature, value);
+    if (ret)
+      return ret;
+  } else {
+    sysctl_name[3] = the_feature->sysctl;
+    if (sysctl(sysctl_name, 4, buf, &buflen, NULL, 0))
+      return -SENSORS_ERR_PROC;
+    *value = *((long *) (buf + the_feature->offset));
+  }
   for (mag = the_feature->scaling; mag > 0; mag --)
     *value /= 10.0;
   for (; mag < 0; mag ++)
@@ -180,17 +344,22 @@ int sensors_write_proc(sensors_chip_name
     return sysctl_name[2];
   if (! (the_feature = sensors_lookup_feature_nr(name.prefix,feature)))
     return -SENSORS_ERR_NO_ENTRY;
-  sysctl_name[3] = the_feature->sysctl;
-  if (sysctl(sysctl_name, 4, buf, &buflen, NULL, 0))
-    return -SENSORS_ERR_PROC;
-  if (sysctl_name[0] != CTL_DEV) { sysctl_name[0] = CTL_DEV ; }
   for (mag = the_feature->scaling; mag > 0; mag --)
     value *= 10.0;
   for (; mag < 0; mag ++)
     value /= 10.0;
-  * ((long *) (buf + the_feature->offset)) = (long) value;
-  buflen = the_feature->offset + sizeof(long);
-  if (sysctl(sysctl_name, 4, NULL, 0, buf, buflen))
-    return -SENSORS_ERR_PROC;
-  return 0;
+
+  if (using_sysfs) {
+    return sensors_write_sysfs(name, the_feature, value);
+  } else {
+    sysctl_name[3] = the_feature->sysctl;
+    if (sysctl(sysctl_name, 4, buf, &buflen, NULL, 0))
+      return -SENSORS_ERR_PROC;
+    if (sysctl_name[0] != CTL_DEV) { sysctl_name[0] = CTL_DEV ; }
+    * ((long *) (buf + the_feature->offset)) = (long) value;
+    buflen = the_feature->offset + sizeof(long);
+    if (sysctl(sysctl_name, 4, NULL, 0, buf, buflen))
+      return -SENSORS_ERR_PROC;
+    return 0;
+  }
 }