hwmon (dme1737): add support for SCH5027

PATCH In review Juerg Haefliger 2 participants 6 messages Apr 23, 2008 – Apr 27, 2008

Subsystems (3)

Documentation Hardware monitoring DME1737 hardware monitor driver

lore

Share & tools

lore

Juerg Haefliger

author

2008-04-23 05:25 UTC #0 lore raw

PATCH 3/3 hwmon (dme1737): add support for SCH5027

Add support for the SCH5027. The differences to the DME1737 are:
- No support for programmable temp offsets
- In auto mode, PWM outputs stay on min value if temp goes below low threshold and can't be programmed to fully turn off
- Different voltage scaling

Signed-off-by Juerg Haefliger <juergh at gmail.com>

drivers/hwmon/dme1737.c +97 ~~-40~~

Index: linux/drivers/hwmon/dme1737.c
===================================================================
@@ -1,6 +1,6 @@
 /*
- * dme1737.c - Driver for the SMSC DME1737, Asus A8000, and SMSC SCH311x
- *             Super-I/O chips integrated hardware monitoring features.
+ * dme1737.c - Driver for the SMSC DME1737, Asus A8000, SMSC SCH5027 and
+ *             SCH311x Super-I/O chips integrated hardware monitoring features.
  * Copyright (c) 2007 Juerg Haefliger <[email protected]>
  *
  * This driver is an I2C/ISA hybrid, meaning that it uses the I2C bus to access

@@ -57,7 +57,7 @@
 static const unsigned short normal_i2c[] = {0x2c, 0x2d, 0x2e, I2C_CLIENT_END};
 
 /* Insmod parameters */
-I2C_CLIENT_INSMOD_1(dme1737);
+I2C_CLIENT_INSMOD_2(dme1737, sch5027);
 
 /* ---------------------------------------------------------------------
  * Registers

@@ -163,6 +163,7 @@
 #define DME1737_VERSTEP		0x88
 #define DME1737_VERSTEP_MASK	0xf8
 #define SCH311X_DEVICE		0x8c
+#define SCH5027_VERSTEP		0x69
 
 /* Length of ISA address segment */
 #define DME1737_EXTENT	2

@@ -220,8 +221,12 @@
 					 3300};
 static const int IN_NOMINAL_SCH311x[] = {2500, 1500, 3300, 5000, 12000, 3300,
 					 3300};
+static const int IN_NOMINAL_SCH5027[] = {1125, 2250, 3300, 5000, 1125, 3300,
+					 3300};
 #define IN_NOMINAL(ix, type)	(((type) == dme1737) ? \
 				IN_NOMINAL_DME1737[(ix)] : \
+				((type) == sch5027) ? \
+				IN_NOMINAL_SCH5027[(ix)] : \
 				IN_NOMINAL_SCH311x[(ix)])
 
 /* Voltage input

@@ -565,7 +570,8 @@
 
 	/* Sample register contents every 1 sec */
 	if (time_after(jiffies, data->last_update + HZ) || !data->valid) {
-		data->vid = dme1737_read(client, DME1737_REG_VID) & 0x3f;
+		data->vid = dme1737_read(client, DME1737_REG_VID);
+		data->vid &= (data->type == sch5027) ? 0x38 : 0x3f;
 
 		/* In (voltage) registers */
 		for (ix = 0; ix < ARRAY_SIZE(data->in); ix++) {

@@ -593,8 +599,10 @@
 					DME1737_REG_TEMP_MIN(ix));
 			data->temp_max[ix] = dme1737_read(client,
 					DME1737_REG_TEMP_MAX(ix));
-			data->temp_offset[ix] = dme1737_read(client,
-					DME1737_REG_TEMP_OFFSET(ix));
+			if (data->type != sch5027) {
+				data->temp_offset[ix] = dme1737_read(client,
+						DME1737_REG_TEMP_OFFSET(ix));
+			}
 		}
 
 		/* In and temp LSB registers

@@ -1570,31 +1578,25 @@
 	&sensor_dev_attr_temp1_max.dev_attr.attr,
 	&sensor_dev_attr_temp1_alarm.dev_attr.attr,
 	&sensor_dev_attr_temp1_fault.dev_attr.attr,
-	&sensor_dev_attr_temp1_offset.dev_attr.attr,
 	&sensor_dev_attr_temp2_input.dev_attr.attr,
 	&sensor_dev_attr_temp2_min.dev_attr.attr,
 	&sensor_dev_attr_temp2_max.dev_attr.attr,
 	&sensor_dev_attr_temp2_alarm.dev_attr.attr,
 	&sensor_dev_attr_temp2_fault.dev_attr.attr,
-	&sensor_dev_attr_temp2_offset.dev_attr.attr,
 	&sensor_dev_attr_temp3_input.dev_attr.attr,
 	&sensor_dev_attr_temp3_min.dev_attr.attr,
 	&sensor_dev_attr_temp3_max.dev_attr.attr,
 	&sensor_dev_attr_temp3_alarm.dev_attr.attr,
 	&sensor_dev_attr_temp3_fault.dev_attr.attr,
-	&sensor_dev_attr_temp3_offset.dev_attr.attr,
 	/* Zones */
-	&sensor_dev_attr_zone1_auto_point1_temp_hyst.dev_attr.attr,
 	&sensor_dev_attr_zone1_auto_point1_temp.dev_attr.attr,
 	&sensor_dev_attr_zone1_auto_point2_temp.dev_attr.attr,
 	&sensor_dev_attr_zone1_auto_point3_temp.dev_attr.attr,
 	&sensor_dev_attr_zone1_auto_channels_temp.dev_attr.attr,
-	&sensor_dev_attr_zone2_auto_point1_temp_hyst.dev_attr.attr,
 	&sensor_dev_attr_zone2_auto_point1_temp.dev_attr.attr,
 	&sensor_dev_attr_zone2_auto_point2_temp.dev_attr.attr,
 	&sensor_dev_attr_zone2_auto_point3_temp.dev_attr.attr,
 	&sensor_dev_attr_zone2_auto_channels_temp.dev_attr.attr,
-	&sensor_dev_attr_zone3_auto_point1_temp_hyst.dev_attr.attr,
 	&sensor_dev_attr_zone3_auto_point1_temp.dev_attr.attr,
 	&sensor_dev_attr_zone3_auto_point2_temp.dev_attr.attr,
 	&sensor_dev_attr_zone3_auto_point3_temp.dev_attr.attr,

@@ -1609,6 +1611,25 @@
 	.attrs = dme1737_attr,
 };
 
+/* The following struct holds misc attributes, which are not available in all
+ * chips. Their creation depends on the chip type which is determined during
+ * module load. */
+static struct attribute *dme1737_misc_attr[] = {
+	/* Temperatures */
+	&sensor_dev_attr_temp1_offset.dev_attr.attr,
+	&sensor_dev_attr_temp2_offset.dev_attr.attr,
+	&sensor_dev_attr_temp3_offset.dev_attr.attr,
+	/* Zones */
+	&sensor_dev_attr_zone1_auto_point1_temp_hyst.dev_attr.attr,
+	&sensor_dev_attr_zone2_auto_point1_temp_hyst.dev_attr.attr,
+	&sensor_dev_attr_zone3_auto_point1_temp_hyst.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group dme1737_misc_group = {
+	.attrs = dme1737_misc_attr,
+};
+
 /* The following structs hold the PWM attributes, some of which are optional.
  * Their creation depends on the chip configuration which is determined during
  * module load. */

@@ -1618,7 +1639,6 @@
 	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
 	&sensor_dev_attr_pwm1_ramp_rate.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_channels_zone.dev_attr.attr,
-	&sensor_dev_attr_pwm1_auto_pwm_min.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr,
 	NULL

@@ -1629,7 +1649,6 @@
 	&sensor_dev_attr_pwm2_enable.dev_attr.attr,
 	&sensor_dev_attr_pwm2_ramp_rate.dev_attr.attr,
 	&sensor_dev_attr_pwm2_auto_channels_zone.dev_attr.attr,
-	&sensor_dev_attr_pwm2_auto_pwm_min.dev_attr.attr,
 	&sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm2_auto_point2_pwm.dev_attr.attr,
 	NULL

@@ -1640,7 +1659,6 @@
 	&sensor_dev_attr_pwm3_enable.dev_attr.attr,
 	&sensor_dev_attr_pwm3_ramp_rate.dev_attr.attr,
 	&sensor_dev_attr_pwm3_auto_channels_zone.dev_attr.attr,
-	&sensor_dev_attr_pwm3_auto_pwm_min.dev_attr.attr,
 	&sensor_dev_attr_pwm3_auto_point1_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm3_auto_point2_pwm.dev_attr.attr,
 	NULL

@@ -1667,6 +1685,15 @@
 	{ .attrs = dme1737_pwm6_attr },
 };
 
+/* The following struct holds misc PWM attributes, which are not available in
+ * all chips. Their creation depends on the chip type which is determined
+ * during module load. */
+static struct attribute *dme1737_pwm_misc_attr[] = {
+	&sensor_dev_attr_pwm1_auto_pwm_min.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_pwm_min.dev_attr.attr,
+	&sensor_dev_attr_pwm3_auto_pwm_min.dev_attr.attr,
+};
+
 /* The following structs hold the fan attributes, some of which are optional.
  * Their creation depends on the chip configuration which is determined during
  * module load. */

@@ -1722,31 +1749,23 @@
 	{ .attrs = dme1737_fan6_attr },
 };
 
-/* The permissions of all of the following attributes are changed to read-
+/* The permissions of the following zone attributes are changed to read-
  * writeable if the chip is *not* locked. Otherwise they stay read-only. */
-static struct attribute *dme1737_misc_chmod_attr[] = {
-	/* Temperatures */
-	&sensor_dev_attr_temp1_offset.dev_attr.attr,
-	&sensor_dev_attr_temp2_offset.dev_attr.attr,
-	&sensor_dev_attr_temp3_offset.dev_attr.attr,
-	/* Zones */
-	&sensor_dev_attr_zone1_auto_point1_temp_hyst.dev_attr.attr,
+static struct attribute *dme1737_zone_chmod_attr[] = {
 	&sensor_dev_attr_zone1_auto_point1_temp.dev_attr.attr,
 	&sensor_dev_attr_zone1_auto_point2_temp.dev_attr.attr,
 	&sensor_dev_attr_zone1_auto_point3_temp.dev_attr.attr,
-	&sensor_dev_attr_zone2_auto_point1_temp_hyst.dev_attr.attr,
 	&sensor_dev_attr_zone2_auto_point1_temp.dev_attr.attr,
 	&sensor_dev_attr_zone2_auto_point2_temp.dev_attr.attr,
 	&sensor_dev_attr_zone2_auto_point3_temp.dev_attr.attr,
-	&sensor_dev_attr_zone3_auto_point1_temp_hyst.dev_attr.attr,
 	&sensor_dev_attr_zone3_auto_point1_temp.dev_attr.attr,
 	&sensor_dev_attr_zone3_auto_point2_temp.dev_attr.attr,
 	&sensor_dev_attr_zone3_auto_point3_temp.dev_attr.attr,
 	NULL
 };
 
-static const struct attribute_group dme1737_misc_chmod_group = {
-	.attrs = dme1737_misc_chmod_attr,
+static const struct attribute_group dme1737_zone_chmod_group = {
+	.attrs = dme1737_zone_chmod_attr,
 };
 
 /* The permissions of the following PWM attributes are changed to read-

@@ -1757,7 +1776,6 @@
 	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
 	&sensor_dev_attr_pwm1_ramp_rate.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_channels_zone.dev_attr.attr,
-	&sensor_dev_attr_pwm1_auto_pwm_min.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
 	NULL
 };

@@ -1766,7 +1784,6 @@
 	&sensor_dev_attr_pwm2_enable.dev_attr.attr,
 	&sensor_dev_attr_pwm2_ramp_rate.dev_attr.attr,
 	&sensor_dev_attr_pwm2_auto_channels_zone.dev_attr.attr,
-	&sensor_dev_attr_pwm2_auto_pwm_min.dev_attr.attr,
 	&sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr,
 	NULL
 };

@@ -1775,7 +1792,6 @@
 	&sensor_dev_attr_pwm3_enable.dev_attr.attr,
 	&sensor_dev_attr_pwm3_ramp_rate.dev_attr.attr,
 	&sensor_dev_attr_pwm3_auto_channels_zone.dev_attr.attr,
-	&sensor_dev_attr_pwm3_auto_pwm_min.dev_attr.attr,
 	&sensor_dev_attr_pwm3_auto_point1_pwm.dev_attr.attr,
 	NULL
 };

@@ -1875,9 +1891,17 @@
 		if (data->has_pwm & (1 << ix)) {
 			sysfs_remove_group(&dev->kobj,
 					   &dme1737_pwm_group[ix]);
+			if (data->type != sch5027 && ix < 4) {
+				sysfs_remove_file(&dev->kobj,
+						  dme1737_pwm_misc_attr[ix]);
+			}
 		}
 	}
 
+	if (data->type != sch5027) {
+		sysfs_remove_group(&dev->kobj, &dme1737_misc_group);
+	}
+
 	sysfs_remove_group(&dev->kobj, &dme1737_group);
 
 	if (!data->client.driver) {

@@ -1901,6 +1925,13 @@
 		goto exit_remove;
 	}
 
+	/* Create misc sysfs attributes */
+	if ((data->type != sch5027) &&
+	    (err = sysfs_create_group(&dev->kobj,
+				      &dme1737_misc_group))) {
+		goto exit_remove;
+	}
+
 	/* Create fan sysfs attributes */
 	for (ix = 0; ix < ARRAY_SIZE(dme1737_fan_group); ix++) {
 		if (data->has_fan & (1 << ix)) {

@@ -1918,6 +1949,11 @@
 						&dme1737_pwm_group[ix]))) {
 				goto exit_remove;
 			}
+			if (data->type != sch5027 && ix < 4 &&
+			    (err = sysfs_create_file(&dev->kobj,
+						dme1737_pwm_misc_attr[ix]))) {
+				goto exit_remove;
+			}
 		}
 	}

@@ -1927,16 +1963,27 @@
 		dev_info(dev, "Device is locked. Some attributes "
 			 "will be read-only.\n");
 	} else {
-		/* Change permissions of standard sysfs attributes */
-		dme1737_chmod_group(dev, &dme1737_misc_chmod_group,
+		/* Change permissions of zone sysfs attributes */
+		dme1737_chmod_group(dev, &dme1737_zone_chmod_group,
 				    S_IRUGO | S_IWUSR);
 
+		/* Change permissions of misc sysfs attributes */
+		if (data->type != sch5027) {
+			dme1737_chmod_group(dev, &dme1737_misc_group,
+					    S_IRUGO | S_IWUSR);
+		}
+
 		/* Change permissions of PWM sysfs attributes */
 		for (ix = 0; ix < ARRAY_SIZE(dme1737_pwm_chmod_group); ix++) {
 			if (data->has_pwm & (1 << ix)) {
 				dme1737_chmod_group(dev,
 						&dme1737_pwm_chmod_group[ix],
 						S_IRUGO | S_IWUSR);
+				if (data->type != sch5027 && ix < 4) {
+					dme1737_chmod_file(dev,
+						dme1737_pwm_misc_attr[ix],
+						S_IRUGO | S_IWUSR);
+				}
 			}
 		}

@@ -2095,9 +2142,10 @@
 	dme1737_sio_enter(sio_cip);
 
 	/* Check device ID
-	 * The DME1737 can return either 0x78 or 0x77 as its device ID. */
+	 * The DME1737 can return either 0x78 or 0x77 as its device ID.
+	 * The SCH5027 returns 0x89 as its device ID. */
 	reg = force_id ? force_id : dme1737_sio_inb(sio_cip, 0x20);
-	if (!(reg == 0x77 || reg == 0x78)) {
+	if (!(reg == 0x77 || reg == 0x78 || reg == 0x89)) {
 		err = -ENODEV;
 		goto exit;
 	}

@@ -2166,15 +2214,23 @@
 		company = dme1737_read(client, DME1737_REG_COMPANY);
 		verstep = dme1737_read(client, DME1737_REG_VERSTEP);
 
-		if (!((company == DME1737_COMPANY_SMSC) &&
-		      ((verstep & DME1737_VERSTEP_MASK) == DME1737_VERSTEP))) {
+		if (company == DME1737_COMPANY_SMSC &&
+		    (verstep & DME1737_VERSTEP_MASK) == DME1737_VERSTEP) {
+			kind = dme1737;
+		} else if (company == DME1737_COMPANY_SMSC &&
+			   verstep == SCH5027_VERSTEP) {
+			kind = sch5027;
+		} else {
 			err = -ENODEV;
 			goto exit_kfree;
 		}
 	}
 
-	kind = dme1737;
-	name = "dme1737";
+	if (kind == dme1737) {
+		name = "dme1737";
+	} else {
+		name = "sch5027";
+	}
 	data->type = kind;
 
 	/* Fill in the remaining client fields and put it into the global

@@ -2187,8 +2243,9 @@
 		goto exit_kfree;
 	}
 
-	dev_info(dev, "Found a DME1737 chip at 0x%02x (rev 0x%02x).\n",
-		 client->addr, verstep);
+	dev_info(dev, "Found a %s chip at 0x%02x (rev 0x%02x).\n",
+		 kind == sch5027 ? "SCH5027" : "DME1737", client->addr,
+		 verstep);
 
 	/* Initialize the DME1737 chip */
 	if ((err = dme1737_init_device(dev))) {

Documentation/hwmon/dme1737 +19 ~~-16~~

Index: linux/Documentation/hwmon/dme1737
===================================================================
@@ -10,6 +10,11 @@
     Prefix: 'sch311x'
     Addresses scanned: none, address read from Super-I/O config space
     Datasheet: http://www.nuhorizons.com/FeaturedProducts/Volume1/SMSC/311x.pdf
+  * SMSC SCH5027
+    Prefix: 'sch5027'
+    Addresses scanned: I2C 0x2c, 0x2d, 0x2e
+    Datasheet: Provided by SMSC upon request and under NDA
+
 
 Authors:
     Juerg Haefliger <[email protected]>

@@ -27,33 +32,31 @@
 			following boards:
 			- VIA EPIA SN18000
 
-Note that there is no need to use this parameter if the driver loads without
-complaining. The driver will say so if it is necessary.
-
 
 Description
 -----------
 
 This driver implements support for the hardware monitoring capabilities of the
-SMSC DME1737 and Asus A8000 (which are the same) and SMSC SCH311x Super-I/O
-chips. These chips feature monitoring of 3 temp sensors temp[1-3] (2 remote
-diodes and 1 internal), 7 voltages in[0-6] (6 external and 1 internal) and up
-to 6 fan speeds fan[1-6]. Additionally, the chips implement up to 5 PWM
-outputs pwm[1-3,5-6] for controlling fan speeds both manually and
+SMSC DME1737 and Asus A8000 (which are the same), SMSC SCH5027, and SMSC
+SCH311x Super-I/O chips. These chips feature monitoring of 3 temp sensors
+temp[1-3] (2 remote diodes and 1 internal), 7 voltages in[0-6] (6 external and
+1 internal) and up to 6 fan speeds fan[1-6]. Additionally, the chips implement
+up to 5 PWM outputs pwm[1-3,5-6] for controlling fan speeds both manually and
 automatically.
 
-For the DME1737 and A8000, fan[1-2] and pwm[1-2] are always present. Fan[3-6]
-and pwm[3,5-6] are optional features and their availability depends on the
-configuration of the chip. The driver will detect which features are present
-during initialization and create the sysfs attributes accordingly.
+For the DME1737, A8000 and SCH5027, fan[1-2] and pwm[1-2] are always present.
+Fan[3-6] and pwm[3,5-6] are optional features and their availability depends on
+the configuration of the chip. The driver will detect which features are
+present during initialization and create the sysfs attributes accordingly.
 
 For the SCH311x, fan[1-3] and pwm[1-3] are always present and fan[4-6] and
 pwm[5-6] don't exist.
 
-The hardware monitoring features of the DME1737 and A8000 are only accessible
-via SMBus, while the SCH311x only provides access via the ISA bus. The driver
-will therefore register itself as an I2C client driver if it detects a DME1737
-or A8000 and as a platform driver if it detects a SCH311x chip.
+The hardware monitoring features of the DME1737, A8000, and SCH5027 are only
+accessible via SMBus, while the SCH311x only provides access via the ISA bus.
+The driver will therefore register itself as an I2C client driver if it detects
+a DME1737, A8000, or SCH5027 and as a platform driver if it detects a SCH311x
+chip.
 
 
 Voltage Monitoring

drivers/hwmon/Kconfig +2 -2

Index: linux/drivers/hwmon/Kconfig
===================================================================
@@ -549,8 +549,8 @@
 	select HWMON_VID
 	help
 	  If you say yes here you get support for the hardware monitoring
-	  and fan control features of the SMSC DME1737 (and compatibles
-	  like the Asus A8000) and SCH311x Super-I/O chips.
+	  and fan control features of the SMSC DME1737, SCH311x, SCH5027, and
+	  Asus A8000 Super-I/O chips.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called dme1737.

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Jean Delvare

2008-04-26 11:32 UTC #1 lore raw

Re: [lm-sensors] [PATCH 3/3] hwmon (dme1737): add support for

Hi Juerg,

Quoted 5 lines · Juerg Haefliger

Add support for the SCH5027. The differences to the DME1737 are:
- No support for programmable temp offsets
- In auto mode, PWM outputs stay on min value if temp goes below low threshold and can't be programmed to fully turn off
- Different voltage scaling

This would be worth mentioning in Documentation/hwmon/dme1737. Right now, this documentation file suggests that there are no differences between the SCH5027 and the DME1737.

Quoted 2 lines

Signed-off-by Juerg Haefliger <juergh at gmail.com>

Review:

drivers/hwmon/dme1737.c line 4

Quoted 11 lines

Index: linux/drivers/hwmon/dme1737.c
=================================> --- linux.orig/drivers/hwmon/dme1737.c 2008-04-22 22:09:10.000000000 -0700
+++ linux/drivers/hwmon/dme1737.c 2008-04-22 22:22:48.000000000 -0700
drivers/hwmon/dme1737.c +2 -2
@@ -1,6 +1,6 @@
 /*
- * dme1737.c - Driver for the SMSC DME1737, Asus A8000, and SMSC SCH311x
- *             Super-I/O chips integrated hardware monitoring features.
+ * dme1737.c - Driver for the SMSC DME1737, Asus A8000, SMSC SCH5027 and
+ *             SCH311x Super-I/O chips integrated hardware monitoring features.
  * Copyright (c) 2007 Juerg Haefliger <[email protected]>

+2008

Quoted 20 lines

drivers/hwmon/dme1737.c +2 -1

  *
  * This driver is an I2C/ISA hybrid, meaning that it uses the I2C bus to access

@@ -57,7 +57,7 @@
 static const unsigned short normal_i2c[] = {0x2c, 0x2d, 0x2e, I2C_CLIENT_END};
 
 /* Insmod parameters */
-I2C_CLIENT_INSMOD_1(dme1737);
+I2C_CLIENT_INSMOD_2(dme1737, sch5027);
 
 /* ---------------------------------------------------------------------
  * Registers

@@ -163,6 +163,7 @@
 #define DME1737_VERSTEP		0x88
 #define DME1737_VERSTEP_MASK	0xf8
 #define SCH311X_DEVICE		0x8c
+#define SCH5027_VERSTEP		0x69
 
 /* Length of ISA address segment */
 #define DME1737_EXTENT	2

drivers/hwmon/dme1737.c line 230

Quoted 10 lines

drivers/hwmon/dme1737.c +4 -0

@@ -220,8 +221,12 @@
 					 3300};
 static const int IN_NOMINAL_SCH311x[] = {2500, 1500, 3300, 5000, 12000, 3300,
 					 3300};
+static const int IN_NOMINAL_SCH5027[] = {1125, 2250, 3300, 5000, 1125, 3300,
+					 3300};
 #define IN_NOMINAL(ix, type)	(((type) = dme1737) ? \
 				IN_NOMINAL_DME1737[(ix)] : \
+				((type) = sch5027) ? \
+				IN_NOMINAL_SCH5027[(ix)] : \
 				IN_NOMINAL_SCH311x[(ix)])

This works, but this will become increasingly inefficient as you add support for new chip types. For better performance, you could either have an array pointing to the various IN_NOMINAL arrays and access it by index, or store a pointer to the right IN_NOMINAL array in dme1737_data. This doesn't need to be done in this patch though, it can be done later.

drivers/hwmon/dme1737.c line 574

Quoted 9 lines

drivers/hwmon/dme1737.c +2 -1

 /* Voltage input

@@ -565,7 +570,8 @@
 
 	/* Sample register contents every 1 sec */
 	if (time_after(jiffies, data->last_update + HZ) || !data->valid) {
-		data->vid = dme1737_read(client, DME1737_REG_VID) & 0x3f;
+		data->vid = dme1737_read(client, DME1737_REG_VID);
+		data->vid &= (data->type = sch5027) ? 0x38 : 0x3f;

This can't be correct. VID values are at least 4 bit wide and always right-padded. Masking with 0x38 just doesn't make sense. Can you explain?

Quoted 6 lines

drivers/hwmon/dme1737.c +0 -0

 		/* In (voltage) registers */
 		for (ix = 0; ix < ARRAY_SIZE(data->in); ix++) {

@@ -593,8 +599,10 @@
 					DME1737_REG_TEMP_MIN(ix));
 			data->temp_max[ix] = dme1737_read(client,

drivers/hwmon/dme1737.c line 608

Quoted 10 lines

drivers/hwmon/dme1737.c +4 -2

 					DME1737_REG_TEMP_MAX(ix));
-			data->temp_offset[ix] = dme1737_read(client,
-					DME1737_REG_TEMP_OFFSET(ix));
+			if (data->type != sch5027) {
+				data->temp_offset[ix] = dme1737_read(client,
+						DME1737_REG_TEMP_OFFSET(ix));
+			}
 		}
 
 		/* In and temp LSB registers

What about zone1_auto_point*_temp_hyst? When these files are not created it seems to me that you could skip reading from DME1737_REG_ZONE_HYST registers.

Quoted 159 lines

drivers/hwmon/dme1737.c +32 ~~-24~~

@@ -1570,31 +1578,25 @@
 	&sensor_dev_attr_temp1_max.dev_attr.attr,
 	&sensor_dev_attr_temp1_alarm.dev_attr.attr,
 	&sensor_dev_attr_temp1_fault.dev_attr.attr,
-	&sensor_dev_attr_temp1_offset.dev_attr.attr,
 	&sensor_dev_attr_temp2_input.dev_attr.attr,
 	&sensor_dev_attr_temp2_min.dev_attr.attr,
 	&sensor_dev_attr_temp2_max.dev_attr.attr,
 	&sensor_dev_attr_temp2_alarm.dev_attr.attr,
 	&sensor_dev_attr_temp2_fault.dev_attr.attr,
-	&sensor_dev_attr_temp2_offset.dev_attr.attr,
 	&sensor_dev_attr_temp3_input.dev_attr.attr,
 	&sensor_dev_attr_temp3_min.dev_attr.attr,
 	&sensor_dev_attr_temp3_max.dev_attr.attr,
 	&sensor_dev_attr_temp3_alarm.dev_attr.attr,
 	&sensor_dev_attr_temp3_fault.dev_attr.attr,
-	&sensor_dev_attr_temp3_offset.dev_attr.attr,
 	/* Zones */
-	&sensor_dev_attr_zone1_auto_point1_temp_hyst.dev_attr.attr,
 	&sensor_dev_attr_zone1_auto_point1_temp.dev_attr.attr,
 	&sensor_dev_attr_zone1_auto_point2_temp.dev_attr.attr,
 	&sensor_dev_attr_zone1_auto_point3_temp.dev_attr.attr,
 	&sensor_dev_attr_zone1_auto_channels_temp.dev_attr.attr,
-	&sensor_dev_attr_zone2_auto_point1_temp_hyst.dev_attr.attr,
 	&sensor_dev_attr_zone2_auto_point1_temp.dev_attr.attr,
 	&sensor_dev_attr_zone2_auto_point2_temp.dev_attr.attr,
 	&sensor_dev_attr_zone2_auto_point3_temp.dev_attr.attr,
 	&sensor_dev_attr_zone2_auto_channels_temp.dev_attr.attr,
-	&sensor_dev_attr_zone3_auto_point1_temp_hyst.dev_attr.attr,
 	&sensor_dev_attr_zone3_auto_point1_temp.dev_attr.attr,
 	&sensor_dev_attr_zone3_auto_point2_temp.dev_attr.attr,
 	&sensor_dev_attr_zone3_auto_point3_temp.dev_attr.attr,

@@ -1609,6 +1611,25 @@
 	.attrs = dme1737_attr,
 };
 
+/* The following struct holds misc attributes, which are not available in all
+ * chips. Their creation depends on the chip type which is determined during
+ * module load. */
+static struct attribute *dme1737_misc_attr[] = {
+	/* Temperatures */
+	&sensor_dev_attr_temp1_offset.dev_attr.attr,
+	&sensor_dev_attr_temp2_offset.dev_attr.attr,
+	&sensor_dev_attr_temp3_offset.dev_attr.attr,
+	/* Zones */
+	&sensor_dev_attr_zone1_auto_point1_temp_hyst.dev_attr.attr,
+	&sensor_dev_attr_zone2_auto_point1_temp_hyst.dev_attr.attr,
+	&sensor_dev_attr_zone3_auto_point1_temp_hyst.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group dme1737_misc_group = {
+	.attrs = dme1737_misc_attr,
+};
+
 /* The following structs hold the PWM attributes, some of which are optional.
  * Their creation depends on the chip configuration which is determined during
  * module load. */

@@ -1618,7 +1639,6 @@
 	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
 	&sensor_dev_attr_pwm1_ramp_rate.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_channels_zone.dev_attr.attr,
-	&sensor_dev_attr_pwm1_auto_pwm_min.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr,
 	NULL

@@ -1629,7 +1649,6 @@
 	&sensor_dev_attr_pwm2_enable.dev_attr.attr,
 	&sensor_dev_attr_pwm2_ramp_rate.dev_attr.attr,
 	&sensor_dev_attr_pwm2_auto_channels_zone.dev_attr.attr,
-	&sensor_dev_attr_pwm2_auto_pwm_min.dev_attr.attr,
 	&sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm2_auto_point2_pwm.dev_attr.attr,
 	NULL

@@ -1640,7 +1659,6 @@
 	&sensor_dev_attr_pwm3_enable.dev_attr.attr,
 	&sensor_dev_attr_pwm3_ramp_rate.dev_attr.attr,
 	&sensor_dev_attr_pwm3_auto_channels_zone.dev_attr.attr,
-	&sensor_dev_attr_pwm3_auto_pwm_min.dev_attr.attr,
 	&sensor_dev_attr_pwm3_auto_point1_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm3_auto_point2_pwm.dev_attr.attr,
 	NULL

@@ -1667,6 +1685,15 @@
 	{ .attrs = dme1737_pwm6_attr },
 };
 
+/* The following struct holds misc PWM attributes, which are not available in
+ * all chips. Their creation depends on the chip type which is determined
+ * during module load. */
+static struct attribute *dme1737_pwm_misc_attr[] = {
+	&sensor_dev_attr_pwm1_auto_pwm_min.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_pwm_min.dev_attr.attr,
+	&sensor_dev_attr_pwm3_auto_pwm_min.dev_attr.attr,
+};
+
 /* The following structs hold the fan attributes, some of which are optional.
  * Their creation depends on the chip configuration which is determined during
  * module load. */

@@ -1722,31 +1749,23 @@
 	{ .attrs = dme1737_fan6_attr },
 };
 
-/* The permissions of all of the following attributes are changed to read-
+/* The permissions of the following zone attributes are changed to read-
  * writeable if the chip is *not* locked. Otherwise they stay read-only. */
-static struct attribute *dme1737_misc_chmod_attr[] = {
-	/* Temperatures */
-	&sensor_dev_attr_temp1_offset.dev_attr.attr,
-	&sensor_dev_attr_temp2_offset.dev_attr.attr,
-	&sensor_dev_attr_temp3_offset.dev_attr.attr,
-	/* Zones */
-	&sensor_dev_attr_zone1_auto_point1_temp_hyst.dev_attr.attr,
+static struct attribute *dme1737_zone_chmod_attr[] = {
 	&sensor_dev_attr_zone1_auto_point1_temp.dev_attr.attr,
 	&sensor_dev_attr_zone1_auto_point2_temp.dev_attr.attr,
 	&sensor_dev_attr_zone1_auto_point3_temp.dev_attr.attr,
-	&sensor_dev_attr_zone2_auto_point1_temp_hyst.dev_attr.attr,
 	&sensor_dev_attr_zone2_auto_point1_temp.dev_attr.attr,
 	&sensor_dev_attr_zone2_auto_point2_temp.dev_attr.attr,
 	&sensor_dev_attr_zone2_auto_point3_temp.dev_attr.attr,
-	&sensor_dev_attr_zone3_auto_point1_temp_hyst.dev_attr.attr,
 	&sensor_dev_attr_zone3_auto_point1_temp.dev_attr.attr,
 	&sensor_dev_attr_zone3_auto_point2_temp.dev_attr.attr,
 	&sensor_dev_attr_zone3_auto_point3_temp.dev_attr.attr,
 	NULL
 };
 
-static const struct attribute_group dme1737_misc_chmod_group = {
-	.attrs = dme1737_misc_chmod_attr,
+static const struct attribute_group dme1737_zone_chmod_group = {
+	.attrs = dme1737_zone_chmod_attr,
 };
 
 /* The permissions of the following PWM attributes are changed to read-

@@ -1757,7 +1776,6 @@
 	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
 	&sensor_dev_attr_pwm1_ramp_rate.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_channels_zone.dev_attr.attr,
-	&sensor_dev_attr_pwm1_auto_pwm_min.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
 	NULL
 };

@@ -1766,7 +1784,6 @@
 	&sensor_dev_attr_pwm2_enable.dev_attr.attr,
 	&sensor_dev_attr_pwm2_ramp_rate.dev_attr.attr,
 	&sensor_dev_attr_pwm2_auto_channels_zone.dev_attr.attr,
-	&sensor_dev_attr_pwm2_auto_pwm_min.dev_attr.attr,
 	&sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr,
 	NULL
 };

@@ -1775,7 +1792,6 @@
 	&sensor_dev_attr_pwm3_enable.dev_attr.attr,
 	&sensor_dev_attr_pwm3_ramp_rate.dev_attr.attr,
 	&sensor_dev_attr_pwm3_auto_channels_zone.dev_attr.attr,
-	&sensor_dev_attr_pwm3_auto_pwm_min.dev_attr.attr,
 	&sensor_dev_attr_pwm3_auto_point1_pwm.dev_attr.attr,
 	NULL
 };

drivers/hwmon/dme1737.c line 1897

Quoted 7 lines

drivers/hwmon/dme1737.c +4 -0

@@ -1875,9 +1891,17 @@
 		if (data->has_pwm & (1 << ix)) {
 			sysfs_remove_group(&dev->kobj,
 					   &dme1737_pwm_group[ix]);
+			if (data->type != sch5027 && ix < 4) {
+				sysfs_remove_file(&dev->kobj,
+						  dme1737_pwm_misc_attr[ix]);
+			}

dme1737_pwm_misc_attr[3] doesn't exist, so you should use "&& ix < 3" in the test above. I agree that "data->has_pwm & (1 << 3)" is always false in the current state of things, so it doesn't change anything right now, but it would still feel safer.

Quoted 25 lines

drivers/hwmon/dme1737.c +11 -0

 		}
 	}
 
+	if (data->type != sch5027) {
+		sysfs_remove_group(&dev->kobj, &dme1737_misc_group);
+	}
+
 	sysfs_remove_group(&dev->kobj, &dme1737_group);
 
 	if (!data->client.driver) {

@@ -1901,6 +1925,13 @@
 		goto exit_remove;
 	}
 
+	/* Create misc sysfs attributes */
+	if ((data->type != sch5027) &&
+	    (err = sysfs_create_group(&dev->kobj,
+				      &dme1737_misc_group))) {
+		goto exit_remove;
+	}
+
 	/* Create fan sysfs attributes */
 	for (ix = 0; ix < ARRAY_SIZE(dme1737_fan_group); ix++) {
 		if (data->has_fan & (1 << ix)) {

drivers/hwmon/dme1737.c line 1952

Quoted 4 lines

drivers/hwmon/dme1737.c +1 -0

@@ -1918,6 +1949,11 @@
 						&dme1737_pwm_group[ix]))) {
 				goto exit_remove;
 			}
+			if (data->type != sch5027 && ix < 4 &&

Same here.

Quoted 24 lines

drivers/hwmon/dme1737.c +12 -2

+			    (err = sysfs_create_file(&dev->kobj,
+						dme1737_pwm_misc_attr[ix]))) {
+				goto exit_remove;
+			}
 		}
 	}

@@ -1927,16 +1963,27 @@
 		dev_info(dev, "Device is locked. Some attributes "
 			 "will be read-only.\n");
 	} else {
-		/* Change permissions of standard sysfs attributes */
-		dme1737_chmod_group(dev, &dme1737_misc_chmod_group,
+		/* Change permissions of zone sysfs attributes */
+		dme1737_chmod_group(dev, &dme1737_zone_chmod_group,
 				    S_IRUGO | S_IWUSR);
 
+		/* Change permissions of misc sysfs attributes */
+		if (data->type != sch5027) {
+			dme1737_chmod_group(dev, &dme1737_misc_group,
+					    S_IRUGO | S_IWUSR);
+		}
+
 		/* Change permissions of PWM sysfs attributes */

drivers/hwmon/dme1737.c line 1986

Quoted 10 lines

drivers/hwmon/dme1737.c +5 -0

 		for (ix = 0; ix < ARRAY_SIZE(dme1737_pwm_chmod_group); ix++) {
 			if (data->has_pwm & (1 << ix)) {
 				dme1737_chmod_group(dev,
 						&dme1737_pwm_chmod_group[ix],
 						S_IRUGO | S_IWUSR);
+				if (data->type != sch5027 && ix < 4) {
+					dme1737_chmod_file(dev,
+						dme1737_pwm_misc_attr[ix],
+						S_IRUGO | S_IWUSR);
+				}

And here.

Quoted 32 lines

drivers/hwmon/dme1737.c +10 -4

 			}
 		}

@@ -2095,9 +2142,10 @@
 	dme1737_sio_enter(sio_cip);
 
 	/* Check device ID
-	 * The DME1737 can return either 0x78 or 0x77 as its device ID. */
+	 * The DME1737 can return either 0x78 or 0x77 as its device ID.
+	 * The SCH5027 returns 0x89 as its device ID. */
 	reg = force_id ? force_id : dme1737_sio_inb(sio_cip, 0x20);
-	if (!(reg = 0x77 || reg = 0x78)) {
+	if (!(reg = 0x77 || reg = 0x78 || reg = 0x89)) {
 		err = -ENODEV;
 		goto exit;
 	}

@@ -2166,15 +2214,23 @@
 		company = dme1737_read(client, DME1737_REG_COMPANY);
 		verstep = dme1737_read(client, DME1737_REG_VERSTEP);
 
-		if (!((company = DME1737_COMPANY_SMSC) &&
-		      ((verstep & DME1737_VERSTEP_MASK) = DME1737_VERSTEP))) {
+		if (company = DME1737_COMPANY_SMSC &&
+		    (verstep & DME1737_VERSTEP_MASK) = DME1737_VERSTEP) {
+			kind = dme1737;
+		} else if (company = DME1737_COMPANY_SMSC &&
+			   verstep = SCH5027_VERSTEP) {
+			kind = sch5027;
+		} else {
 			err = -ENODEV;
 			goto exit_kfree;
 		}

drivers/hwmon/dme1737.c line 2234

Quoted 10 lines

drivers/hwmon/dme1737.c +5 -2

 	}
 
-	kind = dme1737;
-	name = "dme1737";
+	if (kind = dme1737) {
+		name = "dme1737";
+	} else {
+		name = "sch5027";
+	}
 	data->type = kind;

kind could be 0 (driver loaded with the "force" parameter.) In this case you are using the name "sch5027", but with the feature set of the DME1737 and the voltage scaling of the SCH311x. That's pretty confusing isn't it? I suggest that you instead treat force as equivalent to force_dme1737, as you were doing before.

Quoted 18 lines

drivers/hwmon/dme1737.c +3 -2
 	/* Fill in the remaining client fields and put it into the global
@@ -2187,8 +2243,9 @@
 		goto exit_kfree;
 	}
 
-	dev_info(dev, "Found a DME1737 chip at 0x%02x (rev 0x%02x).\n",
-		 client->addr, verstep);
+	dev_info(dev, "Found a %s chip at 0x%02x (rev 0x%02x).\n",
+		 kind = sch5027 ? "SCH5027" : "DME1737", client->addr,
+		 verstep);
 
 	/* Initialize the DME1737 chip */
 	if ((err = dme1737_init_device(dev))) {
Index: linux/Documentation/hwmon/dme1737
=================================> --- linux.orig/Documentation/hwmon/dme1737 2008-04-22 22:02:40.000000000 -0700
+++ linux/Documentation/hwmon/dme1737 2008-04-22 22:15:48.000000000 -0700

Documentation/hwmon/dme1737 line 18

Quoted 9 lines

Documentation/hwmon/dme1737 +5 -0

@@ -10,6 +10,11 @@
     Prefix: 'sch311x'
     Addresses scanned: none, address read from Super-I/O config space
     Datasheet: http://www.nuhorizons.com/FeaturedProducts/Volume1/SMSC/311x.pdf
+  * SMSC SCH5027
+    Prefix: 'sch5027'
+    Addresses scanned: I2C 0x2c, 0x2d, 0x2e
+    Datasheet: Provided by SMSC upon request and under NDA
+

One less blank line?

</nitpicking>

Quoted 64 lines

Documentation/hwmon/dme1737 +14 ~~-16~~

 Authors:
     Juerg Haefliger <[email protected]>

@@ -27,33 +32,31 @@
 			following boards:
 			- VIA EPIA SN18000
 
-Note that there is no need to use this parameter if the driver loads without
-complaining. The driver will say so if it is necessary.
-
 
 Description
 -----------
 
 This driver implements support for the hardware monitoring capabilities of the
-SMSC DME1737 and Asus A8000 (which are the same) and SMSC SCH311x Super-I/O
-chips. These chips feature monitoring of 3 temp sensors temp[1-3] (2 remote
-diodes and 1 internal), 7 voltages in[0-6] (6 external and 1 internal) and up
-to 6 fan speeds fan[1-6]. Additionally, the chips implement up to 5 PWM
-outputs pwm[1-3,5-6] for controlling fan speeds both manually and
+SMSC DME1737 and Asus A8000 (which are the same), SMSC SCH5027, and SMSC
+SCH311x Super-I/O chips. These chips feature monitoring of 3 temp sensors
+temp[1-3] (2 remote diodes and 1 internal), 7 voltages in[0-6] (6 external and
+1 internal) and up to 6 fan speeds fan[1-6]. Additionally, the chips implement
+up to 5 PWM outputs pwm[1-3,5-6] for controlling fan speeds both manually and
 automatically.
 
-For the DME1737 and A8000, fan[1-2] and pwm[1-2] are always present. Fan[3-6]
-and pwm[3,5-6] are optional features and their availability depends on the
-configuration of the chip. The driver will detect which features are present
-during initialization and create the sysfs attributes accordingly.
+For the DME1737, A8000 and SCH5027, fan[1-2] and pwm[1-2] are always present.
+Fan[3-6] and pwm[3,5-6] are optional features and their availability depends on
+the configuration of the chip. The driver will detect which features are
+present during initialization and create the sysfs attributes accordingly.
 
 For the SCH311x, fan[1-3] and pwm[1-3] are always present and fan[4-6] and
 pwm[5-6] don't exist.
 
-The hardware monitoring features of the DME1737 and A8000 are only accessible
-via SMBus, while the SCH311x only provides access via the ISA bus. The driver
-will therefore register itself as an I2C client driver if it detects a DME1737
-or A8000 and as a platform driver if it detects a SCH311x chip.
+The hardware monitoring features of the DME1737, A8000, and SCH5027 are only
+accessible via SMBus, while the SCH311x only provides access via the ISA bus.
+The driver will therefore register itself as an I2C client driver if it detects
+a DME1737, A8000, or SCH5027 and as a platform driver if it detects a SCH311x
+chip.
 
 
 Voltage Monitoring

Index: linux/drivers/hwmon/Kconfig
=================================> --- linux.orig/drivers/hwmon/Kconfig 2008-04-22 22:02:40.000000000 -0700
+++ linux/drivers/hwmon/Kconfig 2008-04-22 22:15:48.000000000 -0700

drivers/hwmon/Kconfig +2 -2

@@ -549,8 +549,8 @@
 	select HWMON_VID
 	help
 	  If you say yes here you get support for the hardware monitoring
-	  and fan control features of the SMSC DME1737 (and compatibles
-	  like the Asus A8000) and SCH311x Super-I/O chips.
+	  and fan control features of the SMSC DME1737, SCH311x, SCH5027, and
+	  Asus A8000 Super-I/O chips.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called dme1737.

Signature · 6 lines

Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Juerg Haefliger

author

2008-04-27 19:18 UTC #2 lore raw

Re: [lm-sensors] [PATCH 3/3] hwmon (dme1737): add support for

In reply to Juerg Haefliger

Hi Jean,

Quoted 10 lines

Hi Juerg,
Quoted 5 lines · Juerg Haefliger
Add support for the SCH5027. The differences to the DME1737 are:
- No support for programmable temp offsets
- In auto mode, PWM outputs stay on min value if temp goes below low threshold and can't be programmed to fully turn off
- Different voltage scaling

Quoted 3 lines

This would be worth mentioning in Documentation/hwmon/dme1737. Right now, this documentation file suggests that there are no differences between the SCH5027 and the DME1737.

OK, will do.

Quoted 49 lines

Quoted 3 lines

Signed-off-by Juerg Haefliger <juergh at gmail.com>

Review:

drivers/hwmon/dme1737.c line 4

Quoted 11 lines

Index: linux/drivers/hwmon/dme1737.c
=================================> > --- linux.orig/drivers/hwmon/dme1737.c 2008-04-22 22:09:10.000000000 -0700
+++ linux/drivers/hwmon/dme1737.c 2008-04-22 22:22:48.000000000 -0700
drivers/hwmon/dme1737.c +2 -2
@@ -1,6 +1,6 @@
 /*
- * dme1737.c - Driver for the SMSC DME1737, Asus A8000, and SMSC SCH311x
- *             Super-I/O chips integrated hardware monitoring features.
+ * dme1737.c - Driver for the SMSC DME1737, Asus A8000, SMSC SCH5027 and
+ *             SCH311x Super-I/O chips integrated hardware monitoring features.
  * Copyright (c) 2007 Juerg Haefliger <[email protected]>

+2008

Quoted 29 lines

drivers/hwmon/dme1737.c +6 -1

  *
  * This driver is an I2C/ISA hybrid, meaning that it uses the I2C bus to access

@@ -57,7 +57,7 @@
 static const unsigned short normal_i2c[] = {0x2c, 0x2d, 0x2e, I2C_CLIENT_END};
 
 /* Insmod parameters */
-I2C_CLIENT_INSMOD_1(dme1737);
+I2C_CLIENT_INSMOD_2(dme1737, sch5027);
 
 /* ---------------------------------------------------------------------
  * Registers

@@ -163,6 +163,7 @@
 #define DME1737_VERSTEP              0x88
 #define DME1737_VERSTEP_MASK 0xf8
 #define SCH311X_DEVICE               0x8c
+#define SCH5027_VERSTEP              0x69
 
 /* Length of ISA address segment */
 #define DME1737_EXTENT       2

@@ -220,8 +221,12 @@
                                       3300};
 static const int IN_NOMINAL_SCH311x[] = {2500, 1500, 3300, 5000, 12000, 3300,
                                       3300};
+static const int IN_NOMINAL_SCH5027[] = {1125, 2250, 3300, 5000, 1125, 3300,
+                                      3300};
 #define IN_NOMINAL(ix, type) (((type) = dme1737) ? \
                              IN_NOMINAL_DME1737[(ix)] : \
+                             ((type) = sch5027) ? \
+                             IN_NOMINAL_SCH5027[(ix)] : \

Quoted 8 lines

drivers/hwmon/dme1737.c line 230
Quoted 1 line
drivers/hwmon/dme1737.c +0 -0
                              IN_NOMINAL_SCH311x[(ix)])
This works, but this will become increasingly inefficient as you add support for new chip types. For better performance, you could either have an array pointing to the various IN_NOMINAL arrays and access it by index, or store a pointer to the right IN_NOMINAL array in dme1737_data. This doesn't need to be done in this patch though, it can be done later.

Makes sense. I'll fix it.

Quoted 4 lines

Quoted 4 lines
drivers/hwmon/dme1737.c +0 -0
 /* Voltage input
@@ -565,7 +570,8 @@
 

Quoted 9 lines

drivers/hwmon/dme1737.c
Quoted 5 lines
drivers/hwmon/dme1737.c +2 -1
      /* Sample register contents every 1 sec */
      if (time_after(jiffies, data->last_update + HZ) || !data->valid) {
-             data->vid = dme1737_read(client, DME1737_REG_VID) & 0x3f;
+             data->vid = dme1737_read(client, DME1737_REG_VID);
+             data->vid &= (data->type = sch5027) ? 0x38 : 0x3f;
This can't be correct. VID values are at least 4 bit wide and always right-padded. Masking with 0x38 just doesn't make sense. Can you explain?

The sch5027 has only 3 VID inputs VID3-5. Bits0-2 of the VID register are reserved.

Quoted 184 lines

Quoted 6 lines

drivers/hwmon/dme1737.c +0 -0

              /* In (voltage) registers */
              for (ix = 0; ix < ARRAY_SIZE(data->in); ix++) {

@@ -593,8 +599,10 @@
                                      DME1737_REG_TEMP_MIN(ix));
                      data->temp_max[ix] = dme1737_read(client,

drivers/hwmon/dme1737.c line 608

Quoted 10 lines

drivers/hwmon/dme1737.c +4 -2

                                      DME1737_REG_TEMP_MAX(ix));
-                     data->temp_offset[ix] = dme1737_read(client,
-                                     DME1737_REG_TEMP_OFFSET(ix));
+                     if (data->type != sch5027) {
+                             data->temp_offset[ix] = dme1737_read(client,
+                                             DME1737_REG_TEMP_OFFSET(ix));
+                     }
              }
 
              /* In and temp LSB registers

What about zone1_auto_point*_temp_hyst? When these files are not created it seems to me that you could skip reading from DME1737_REG_ZONE_HYST registers.

Quoted 163 lines

drivers/hwmon/dme1737.c +33 ~~-24~~

@@ -1570,31 +1578,25 @@
      &sensor_dev_attr_temp1_max.dev_attr.attr,
      &sensor_dev_attr_temp1_alarm.dev_attr.attr,
      &sensor_dev_attr_temp1_fault.dev_attr.attr,
-     &sensor_dev_attr_temp1_offset.dev_attr.attr,
      &sensor_dev_attr_temp2_input.dev_attr.attr,
      &sensor_dev_attr_temp2_min.dev_attr.attr,
      &sensor_dev_attr_temp2_max.dev_attr.attr,
      &sensor_dev_attr_temp2_alarm.dev_attr.attr,
      &sensor_dev_attr_temp2_fault.dev_attr.attr,
-     &sensor_dev_attr_temp2_offset.dev_attr.attr,
      &sensor_dev_attr_temp3_input.dev_attr.attr,
      &sensor_dev_attr_temp3_min.dev_attr.attr,
      &sensor_dev_attr_temp3_max.dev_attr.attr,
      &sensor_dev_attr_temp3_alarm.dev_attr.attr,
      &sensor_dev_attr_temp3_fault.dev_attr.attr,
-     &sensor_dev_attr_temp3_offset.dev_attr.attr,
      /* Zones */
-     &sensor_dev_attr_zone1_auto_point1_temp_hyst.dev_attr.attr,
      &sensor_dev_attr_zone1_auto_point1_temp.dev_attr.attr,
      &sensor_dev_attr_zone1_auto_point2_temp.dev_attr.attr,
      &sensor_dev_attr_zone1_auto_point3_temp.dev_attr.attr,
      &sensor_dev_attr_zone1_auto_channels_temp.dev_attr.attr,
-     &sensor_dev_attr_zone2_auto_point1_temp_hyst.dev_attr.attr,
      &sensor_dev_attr_zone2_auto_point1_temp.dev_attr.attr,
      &sensor_dev_attr_zone2_auto_point2_temp.dev_attr.attr,
      &sensor_dev_attr_zone2_auto_point3_temp.dev_attr.attr,
      &sensor_dev_attr_zone2_auto_channels_temp.dev_attr.attr,
-     &sensor_dev_attr_zone3_auto_point1_temp_hyst.dev_attr.attr,
      &sensor_dev_attr_zone3_auto_point1_temp.dev_attr.attr,
      &sensor_dev_attr_zone3_auto_point2_temp.dev_attr.attr,
      &sensor_dev_attr_zone3_auto_point3_temp.dev_attr.attr,

@@ -1609,6 +1611,25 @@
      .attrs = dme1737_attr,
 };
 
+/* The following struct holds misc attributes, which are not available in all
+ * chips. Their creation depends on the chip type which is determined during
+ * module load. */
+static struct attribute *dme1737_misc_attr[] = {
+     /* Temperatures */
+     &sensor_dev_attr_temp1_offset.dev_attr.attr,
+     &sensor_dev_attr_temp2_offset.dev_attr.attr,
+     &sensor_dev_attr_temp3_offset.dev_attr.attr,
+     /* Zones */
+     &sensor_dev_attr_zone1_auto_point1_temp_hyst.dev_attr.attr,
+     &sensor_dev_attr_zone2_auto_point1_temp_hyst.dev_attr.attr,
+     &sensor_dev_attr_zone3_auto_point1_temp_hyst.dev_attr.attr,
+     NULL
+};
+
+static const struct attribute_group dme1737_misc_group = {
+     .attrs = dme1737_misc_attr,
+};
+
 /* The following structs hold the PWM attributes, some of which are optional.
  * Their creation depends on the chip configuration which is determined during
  * module load. */

@@ -1618,7 +1639,6 @@
      &sensor_dev_attr_pwm1_enable.dev_attr.attr,
      &sensor_dev_attr_pwm1_ramp_rate.dev_attr.attr,
      &sensor_dev_attr_pwm1_auto_channels_zone.dev_attr.attr,
-     &sensor_dev_attr_pwm1_auto_pwm_min.dev_attr.attr,
      &sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
      &sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr,
      NULL

@@ -1629,7 +1649,6 @@
      &sensor_dev_attr_pwm2_enable.dev_attr.attr,
      &sensor_dev_attr_pwm2_ramp_rate.dev_attr.attr,
      &sensor_dev_attr_pwm2_auto_channels_zone.dev_attr.attr,
-     &sensor_dev_attr_pwm2_auto_pwm_min.dev_attr.attr,
      &sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr,
      &sensor_dev_attr_pwm2_auto_point2_pwm.dev_attr.attr,
      NULL

@@ -1640,7 +1659,6 @@
      &sensor_dev_attr_pwm3_enable.dev_attr.attr,
      &sensor_dev_attr_pwm3_ramp_rate.dev_attr.attr,
      &sensor_dev_attr_pwm3_auto_channels_zone.dev_attr.attr,
-     &sensor_dev_attr_pwm3_auto_pwm_min.dev_attr.attr,
      &sensor_dev_attr_pwm3_auto_point1_pwm.dev_attr.attr,
      &sensor_dev_attr_pwm3_auto_point2_pwm.dev_attr.attr,
      NULL

@@ -1667,6 +1685,15 @@
      { .attrs = dme1737_pwm6_attr },
 };
 
+/* The following struct holds misc PWM attributes, which are not available in
+ * all chips. Their creation depends on the chip type which is determined
+ * during module load. */
+static struct attribute *dme1737_pwm_misc_attr[] = {
+     &sensor_dev_attr_pwm1_auto_pwm_min.dev_attr.attr,
+     &sensor_dev_attr_pwm2_auto_pwm_min.dev_attr.attr,
+     &sensor_dev_attr_pwm3_auto_pwm_min.dev_attr.attr,
+};
+
 /* The following structs hold the fan attributes, some of which are optional.
  * Their creation depends on the chip configuration which is determined during
  * module load. */

@@ -1722,31 +1749,23 @@
      { .attrs = dme1737_fan6_attr },
 };
 
-/* The permissions of all of the following attributes are changed to read-
+/* The permissions of the following zone attributes are changed to read-
  * writeable if the chip is *not* locked. Otherwise they stay read-only. */
-static struct attribute *dme1737_misc_chmod_attr[] = {
-     /* Temperatures */
-     &sensor_dev_attr_temp1_offset.dev_attr.attr,
-     &sensor_dev_attr_temp2_offset.dev_attr.attr,
-     &sensor_dev_attr_temp3_offset.dev_attr.attr,
-     /* Zones */
-     &sensor_dev_attr_zone1_auto_point1_temp_hyst.dev_attr.attr,
+static struct attribute *dme1737_zone_chmod_attr[] = {
      &sensor_dev_attr_zone1_auto_point1_temp.dev_attr.attr,
      &sensor_dev_attr_zone1_auto_point2_temp.dev_attr.attr,
      &sensor_dev_attr_zone1_auto_point3_temp.dev_attr.attr,
-     &sensor_dev_attr_zone2_auto_point1_temp_hyst.dev_attr.attr,
      &sensor_dev_attr_zone2_auto_point1_temp.dev_attr.attr,
      &sensor_dev_attr_zone2_auto_point2_temp.dev_attr.attr,
      &sensor_dev_attr_zone2_auto_point3_temp.dev_attr.attr,
-     &sensor_dev_attr_zone3_auto_point1_temp_hyst.dev_attr.attr,
      &sensor_dev_attr_zone3_auto_point1_temp.dev_attr.attr,
      &sensor_dev_attr_zone3_auto_point2_temp.dev_attr.attr,
      &sensor_dev_attr_zone3_auto_point3_temp.dev_attr.attr,
      NULL
 };
 
-static const struct attribute_group dme1737_misc_chmod_group = {
-     .attrs = dme1737_misc_chmod_attr,
+static const struct attribute_group dme1737_zone_chmod_group = {
+     .attrs = dme1737_zone_chmod_attr,
 };
 
 /* The permissions of the following PWM attributes are changed to read-

@@ -1757,7 +1776,6 @@
      &sensor_dev_attr_pwm1_enable.dev_attr.attr,
      &sensor_dev_attr_pwm1_ramp_rate.dev_attr.attr,
      &sensor_dev_attr_pwm1_auto_channels_zone.dev_attr.attr,
-     &sensor_dev_attr_pwm1_auto_pwm_min.dev_attr.attr,
      &sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
      NULL
 };

@@ -1766,7 +1784,6 @@
      &sensor_dev_attr_pwm2_enable.dev_attr.attr,
      &sensor_dev_attr_pwm2_ramp_rate.dev_attr.attr,
      &sensor_dev_attr_pwm2_auto_channels_zone.dev_attr.attr,
-     &sensor_dev_attr_pwm2_auto_pwm_min.dev_attr.attr,
      &sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr,
      NULL
 };

@@ -1775,7 +1792,6 @@
      &sensor_dev_attr_pwm3_enable.dev_attr.attr,
      &sensor_dev_attr_pwm3_ramp_rate.dev_attr.attr,
      &sensor_dev_attr_pwm3_auto_channels_zone.dev_attr.attr,
-     &sensor_dev_attr_pwm3_auto_pwm_min.dev_attr.attr,
      &sensor_dev_attr_pwm3_auto_point1_pwm.dev_attr.attr,
      NULL
 };

@@ -1875,9 +1891,17 @@
              if (data->has_pwm & (1 << ix)) {
                      sysfs_remove_group(&dev->kobj,
                                         &dme1737_pwm_group[ix]);
+                     if (data->type != sch5027 && ix < 4) {

Quoted 8 lines

drivers/hwmon/dme1737.c line 1897
Quoted 3 lines
drivers/hwmon/dme1737.c +3 -0
+                             sysfs_remove_file(&dev->kobj,
+                                               dme1737_pwm_misc_attr[ix]);
+                     }
dme1737_pwm_misc_attr[3] doesn't exist, so you should use "&& ix < 3" in the test above. I agree that "data->has_pwm & (1 << 3)" is always false in the current state of things, so it doesn't change anything right now, but it would still feel safer.

Good catch. Will fix.

Quoted 103 lines

Quoted 25 lines

drivers/hwmon/dme1737.c +11 -0

              }
      }
 
+     if (data->type != sch5027) {
+             sysfs_remove_group(&dev->kobj, &dme1737_misc_group);
+     }
+
      sysfs_remove_group(&dev->kobj, &dme1737_group);
 
      if (!data->client.driver) {

@@ -1901,6 +1925,13 @@
              goto exit_remove;
      }
 
+     /* Create misc sysfs attributes */
+     if ((data->type != sch5027) &&
+         (err = sysfs_create_group(&dev->kobj,
+                                   &dme1737_misc_group))) {
+             goto exit_remove;
+     }
+
      /* Create fan sysfs attributes */
      for (ix = 0; ix < ARRAY_SIZE(dme1737_fan_group); ix++) {
              if (data->has_fan & (1 << ix)) {

drivers/hwmon/dme1737.c line 1952

Quoted 4 lines

drivers/hwmon/dme1737.c +1 -0

@@ -1918,6 +1949,11 @@
                                              &dme1737_pwm_group[ix]))) {
                              goto exit_remove;
                      }
+                     if (data->type != sch5027 && ix < 4 &&

Same here.

Quoted 24 lines

drivers/hwmon/dme1737.c +12 -2

+                         (err = sysfs_create_file(&dev->kobj,
+                                             dme1737_pwm_misc_attr[ix]))) {
+                             goto exit_remove;
+                     }
              }
      }

@@ -1927,16 +1963,27 @@
              dev_info(dev, "Device is locked. Some attributes "
                       "will be read-only.\n");
      } else {
-             /* Change permissions of standard sysfs attributes */
-             dme1737_chmod_group(dev, &dme1737_misc_chmod_group,
+             /* Change permissions of zone sysfs attributes */
+             dme1737_chmod_group(dev, &dme1737_zone_chmod_group,
                                  S_IRUGO | S_IWUSR);
 
+             /* Change permissions of misc sysfs attributes */
+             if (data->type != sch5027) {
+                     dme1737_chmod_group(dev, &dme1737_misc_group,
+                                         S_IRUGO | S_IWUSR);
+             }
+
              /* Change permissions of PWM sysfs attributes */

drivers/hwmon/dme1737.c line 1986

Quoted 10 lines

drivers/hwmon/dme1737.c +5 -0

              for (ix = 0; ix < ARRAY_SIZE(dme1737_pwm_chmod_group); ix++) {
                      if (data->has_pwm & (1 << ix)) {
                              dme1737_chmod_group(dev,
                                              &dme1737_pwm_chmod_group[ix],
                                              S_IRUGO | S_IWUSR);
+                             if (data->type != sch5027 && ix < 4) {
+                                     dme1737_chmod_file(dev,
+                                             dme1737_pwm_misc_attr[ix],
+                                             S_IRUGO | S_IWUSR);
+                             }

And here.

Quoted 34 lines

drivers/hwmon/dme1737.c +10 -4

                      }
              }

@@ -2095,9 +2142,10 @@
      dme1737_sio_enter(sio_cip);
 
      /* Check device ID
-      * The DME1737 can return either 0x78 or 0x77 as its device ID. */
+      * The DME1737 can return either 0x78 or 0x77 as its device ID.
+      * The SCH5027 returns 0x89 as its device ID. */
      reg = force_id ? force_id : dme1737_sio_inb(sio_cip, 0x20);
-     if (!(reg = 0x77 || reg = 0x78)) {
+     if (!(reg = 0x77 || reg = 0x78 || reg = 0x89)) {
              err = -ENODEV;
              goto exit;
      }

@@ -2166,15 +2214,23 @@
              company = dme1737_read(client, DME1737_REG_COMPANY);
              verstep = dme1737_read(client, DME1737_REG_VERSTEP);
 
-             if (!((company = DME1737_COMPANY_SMSC) &&
-                   ((verstep & DME1737_VERSTEP_MASK) = DME1737_VERSTEP))) {
+             if (company = DME1737_COMPANY_SMSC &&
+                 (verstep & DME1737_VERSTEP_MASK) = DME1737_VERSTEP) {
+                     kind = dme1737;
+             } else if (company = DME1737_COMPANY_SMSC &&
+                        verstep = SCH5027_VERSTEP) {
+                     kind = sch5027;
+             } else {
                      err = -ENODEV;
                      goto exit_kfree;
              }
      }

Quoted 14 lines

drivers/hwmon/dme1737.c line 2234
Quoted 8 lines
drivers/hwmon/dme1737.c +5 -2
-     kind = dme1737;
-     name = "dme1737";
+     if (kind = dme1737) {
+             name = "dme1737";
+     } else {
+             name = "sch5027";
+     }
      data->type = kind;
kind could be 0 (driver loaded with the "force" parameter.) In this case you are using the name "sch5027", but with the feature set of the DME1737 and the voltage scaling of the SCH311x. That's pretty confusing isn't it? I suggest that you instead treat force as equivalent to force_dme1737, as you were doing before.

Again, good catch. Will fix.

Quoted 94 lines

Quoted 18 lines

drivers/hwmon/dme1737.c +3 -2

      /* Fill in the remaining client fields and put it into the global

@@ -2187,8 +2243,9 @@
              goto exit_kfree;
      }
 
-     dev_info(dev, "Found a DME1737 chip at 0x%02x (rev 0x%02x).\n",
-              client->addr, verstep);
+     dev_info(dev, "Found a %s chip at 0x%02x (rev 0x%02x).\n",
+              kind = sch5027 ? "SCH5027" : "DME1737", client->addr,
+              verstep);
 
      /* Initialize the DME1737 chip */
      if ((err = dme1737_init_device(dev))) {

Index: linux/Documentation/hwmon/dme1737
=================================> > --- linux.orig/Documentation/hwmon/dme1737 2008-04-22 22:02:40.000000000 -0700
+++ linux/Documentation/hwmon/dme1737 2008-04-22 22:15:48.000000000 -0700

Documentation/hwmon/dme1737 line 18

Quoted 9 lines

Documentation/hwmon/dme1737 +5 -0

@@ -10,6 +10,11 @@
     Prefix: 'sch311x'
     Addresses scanned: none, address read from Super-I/O config space
     Datasheet: http://www.nuhorizons.com/FeaturedProducts/Volume1/SMSC/311x.pdf
+  * SMSC SCH5027
+    Prefix: 'sch5027'
+    Addresses scanned: I2C 0x2c, 0x2d, 0x2e
+    Datasheet: Provided by SMSC upon request and under NDA
+

One less blank line?

</nitpicking>

Quoted 62 lines

Documentation/hwmon/dme1737 +14 ~~-16~~

 Authors:
     Juerg Haefliger <[email protected]>

@@ -27,33 +32,31 @@
                      following boards:
                      - VIA EPIA SN18000
 
-Note that there is no need to use this parameter if the driver loads without
-complaining. The driver will say so if it is necessary.
-
 
 Description
 -----------
 
 This driver implements support for the hardware monitoring capabilities of the
-SMSC DME1737 and Asus A8000 (which are the same) and SMSC SCH311x Super-I/O
-chips. These chips feature monitoring of 3 temp sensors temp[1-3] (2 remote
-diodes and 1 internal), 7 voltages in[0-6] (6 external and 1 internal) and up
-to 6 fan speeds fan[1-6]. Additionally, the chips implement up to 5 PWM
-outputs pwm[1-3,5-6] for controlling fan speeds both manually and
+SMSC DME1737 and Asus A8000 (which are the same), SMSC SCH5027, and SMSC
+SCH311x Super-I/O chips. These chips feature monitoring of 3 temp sensors
+temp[1-3] (2 remote diodes and 1 internal), 7 voltages in[0-6] (6 external and
+1 internal) and up to 6 fan speeds fan[1-6]. Additionally, the chips implement
+up to 5 PWM outputs pwm[1-3,5-6] for controlling fan speeds both manually and
 automatically.
 
-For the DME1737 and A8000, fan[1-2] and pwm[1-2] are always present. Fan[3-6]
-and pwm[3,5-6] are optional features and their availability depends on the
-configuration of the chip. The driver will detect which features are present
-during initialization and create the sysfs attributes accordingly.
+For the DME1737, A8000 and SCH5027, fan[1-2] and pwm[1-2] are always present.
+Fan[3-6] and pwm[3,5-6] are optional features and their availability depends on
+the configuration of the chip. The driver will detect which features are
+present during initialization and create the sysfs attributes accordingly.
 
 For the SCH311x, fan[1-3] and pwm[1-3] are always present and fan[4-6] and
 pwm[5-6] don't exist.
 
-The hardware monitoring features of the DME1737 and A8000 are only accessible
-via SMBus, while the SCH311x only provides access via the ISA bus. The driver
-will therefore register itself as an I2C client driver if it detects a DME1737
-or A8000 and as a platform driver if it detects a SCH311x chip.
+The hardware monitoring features of the DME1737, A8000, and SCH5027 are only
+accessible via SMBus, while the SCH311x only provides access via the ISA bus.
+The driver will therefore register itself as an I2C client driver if it detects
+a DME1737, A8000, or SCH5027 and as a platform driver if it detects a SCH311x
+chip.
 
 
 Voltage Monitoring

Index: linux/drivers/hwmon/Kconfig
=================================> > --- linux.orig/drivers/hwmon/Kconfig 2008-04-22 22:02:40.000000000 -0700
+++ linux/drivers/hwmon/Kconfig 2008-04-22 22:15:48.000000000 -0700

drivers/hwmon/Kconfig +2 -2

@@ -549,8 +549,8 @@
      select HWMON_VID
      help
        If you say yes here you get support for the hardware monitoring
-       and fan control features of the SMSC DME1737 (and compatibles
-       like the Asus A8000) and SCH311x Super-I/O chips.
+       and fan control features of the SMSC DME1737, SCH311x, SCH5027, and
+       Asus A8000 Super-I/O chips.

Quoted 7 lines

drivers/hwmon/Kconfig line 556
Quoted 2 lines
drivers/hwmon/Kconfig +0 -0
        This driver can also be built as a module.  If so, the module
        will be called dme1737.
--
Jean Delvare

Thanks for the review. I owe you three :-)

...juerg

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Jean Delvare

2008-04-27 19:25 UTC #3 lore raw

Re: [lm-sensors] [PATCH 3/3] hwmon (dme1737): add support for

In reply to Juerg Haefliger

Quoted 9 lines · Juerg Haefliger

Quoted 7 lines

drivers/hwmon/dme1737.c +2 -1

@@ -565,7 +570,8 @@
 
      /* Sample register contents every 1 sec */
      if (time_after(jiffies, data->last_update + HZ) || !data->valid) {
-             data->vid = dme1737_read(client, DME1737_REG_VID) & 0x3f;
+             data->vid = dme1737_read(client, DME1737_REG_VID);
+             data->vid &= (data->type = sch5027) ? 0x38 : 0x3f;

Quoted 6 lines

Quoted 3 lines
This can't be correct. VID values are at least 4 bit wide and always right-padded. Masking with 0x38 just doesn't make sense. Can you explain?
The sch5027 has only 3 VID inputs VID3-5. Bits0-2 of the VID register are reserved.

This doesn't make much sense. 3 high VID bits without the corresponding 3 low VID bits is pretty useless. I'm really curious what the designer had in mind... Anyway, what you have now doesn't add any value and could even be confusing. I'd rather not expose any VID value at all for the SCH5027 - my random guess is that these pins won't be wired in practice anyway.

Signature · 6 lines

Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Juerg Haefliger

author

2008-04-27 19:31 UTC #4 lore raw

Re: [lm-sensors] [PATCH 3/3] hwmon (dme1737): add support for

In reply to Juerg Haefliger

Jean,

Quoted 18 lines · Jean Delvare

Quoted 10 lines · Juerg Haefliger
Quoted 10 lines · Juerg Haefliger
Quoted 8 lines
drivers/hwmon/dme1737.c +2 -1
@@ -565,7 +570,8 @@
 
      /* Sample register contents every 1 sec */
      if (time_after(jiffies, data->last_update + HZ) || !data->valid) {
-             data->vid = dme1737_read(client, DME1737_REG_VID) & 0x3f;
+             data->vid = dme1737_read(client, DME1737_REG_VID);
+             data->vid &= (data->type = sch5027) ? 0x38 : 0x3f;
Quoted 6 lines
Quoted 3 lines
This can't be correct. VID values are at least 4 bit wide and always right-padded. Masking with 0x38 just doesn't make sense. Can you explain?
The sch5027 has only 3 VID inputs VID3-5. Bits0-2 of the VID register are reserved.

Quoted 6 lines

This doesn't make much sense. 3 high VID bits without the corresponding 3 low VID bits is pretty useless. I'm really curious what the designer had in mind... Anyway, what you have now doesn't add any value and could even be confusing. I'd rather not expose any VID value at all for the SCH5027 - my random guess is that these pins won't be wired in practice anyway.

So you're saying I should remove the vid & vrm sysfs attributes for this chip?

...juerg

Quoted 3 lines

--
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Jean Delvare

2008-04-27 19:46 UTC #5 lore raw

Re: [lm-sensors] [PATCH 3/3] hwmon (dme1737): add support for

In reply to Juerg Haefliger

Quoted 5 lines · Juerg Haefliger

Quoted 4 lines · Jean Delvare
Quoted 2 lines · Juerg Haefliger
The sch5027 has only 3 VID inputs VID3-5. Bits0-2 of the VID register are reserved.

Quoted 8 lines

Quoted 6 lines
This doesn't make much sense. 3 high VID bits without the corresponding 3 low VID bits is pretty useless. I'm really curious what the designer had in mind... Anyway, what you have now doesn't add any value and could even be confusing. I'd rather not expose any VID value at all for the SCH5027 - my random guess is that these pins won't be wired in practice anyway.
So you're saying I should remove the vid & vrm sysfs attributes for this chip?

Yes, this is what I'm suggesting.

Signature · 6 lines

Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors