[lm-sensors] [PATCH] THMC50 support for 2.6 (2nd revision)

[lm-sensors] [PATCH] THMC50 support for 2.6 (2nd revision)

[Krzysztof Helt]

From: Krzysztof Helt <krzysztof.h1@wp.pl>

This patch adds support for THMC50 and ADM1022 chips to 2.6 kernels.

Special thanks to Jean Delavare and Mark M. Hoffman for review of the first patch.

Signed-off-by: Krzysztof Helt <krzysztof.h1 at wp.pl>

---
This patch incorporates all suggestions and requests to the previously sent patch.

diff -urpN linux-2.6.21/Documentation/hwmon/thmc50
linux-2.6.22/Documentation/hwmon/thmc50 ---
linux-2.6.21/Documentation/hwmon/thmc50	1970-01-01 01:00:00.000000000 +0100 +++
linux-2.6.22/Documentation/hwmon/thmc50	2007-07-02 19:46:42.029001269 +0200 @@
-0,0 +1,86 @@ +Kernel driver thmc50
+==========+
+Supported chips:
+  * Analog Devices ADM1022
+    Prefix: 'adm1022'
+    Addresses scanned: I2C 0x2c - 0x2e
+    Datasheet: http://www.analog.com/en/prod/0,2877,ADM1022,00.html
+  * Texas Instruments THMC50
+    Prefix: 'thmc50'
+    Addresses scanned: I2C 0x2c - 0x2e
+    Datasheet: http://focus.ti.com/docs/prod/folders/print/thmc50.html
+
+Author: Krzysztof Helt <krzysztof.h1@wp.pl>
+
+This driver was derived from the 2.4 kernel thmc50.c source file.
+
+Credits:
+  thmc50.c (2.4 kernel):
+	Frodo Looijaard <frodol@dds.nl>
+	Philip Edelbrock <phil@netroedge.com>
+
+Module Parameters
+-----------------
+
+* force: short array (min = 1, max = 48)
+  List of adapter,address pairs to boldly assume to be present
+* force_thmc50: short array (min = 1, max = 48)
+  List of adapter,address pairs which are unquestionably assumed to contain
+  a `thmc50' chip
+* ignore: short array (min = 1, max = 48)
+  List of adapter,address pairs not to scan
+* ignore_range: short array (min = 1, max = 48)
+  List of adapter,start-addr,end-addr triples not to scan
+* probe: short array (min = 1, max = 48)
+  List of adapter,address pairs to scan additionally
+* probe_range: short array (min = 1, max = 48)
+  List of adapter,start-addr,end-addr triples to scan additionally
+* adm1022_temp3: short array (min = 1, max = 3)
+  List of adapter,address pairs of ADM1022 chips to set into measuring
+  additional remote temperature.
+
+
+Description
+-----------
+
+The THMC50 implements: an internal temperature sensor, support for an
+external diode-type temperature sensor (compatible w/ the diode sensor inside
+many processors), and a controllable fan/analog_out DAC. For the temperature
+sensors, limits can be set through the appropriate Overtemperature Shutdown
+register and Hysteresis register. Each value can be set and read to half-degree
+accuracy.  An alarm is issued (usually to a connected LM78) when the
+temperature gets higher then the Overtemperature Shutdown value; it stays on
+until the temperature falls below the Hysteresis value. All temperatures are in
+degrees Celsius, and are guaranteed within a range of -55 to +125 degrees.
+
+The THMC50 only updates its values each 1.5 seconds; reading it more often
+will do no harm, but will return 'old' values.
+
+The THMC50 is usually used in combination with LM78-like chips, to measure
+the temperature of the processor(s).
+
+The ADM1022 works the same as THMC50 but it is faster (5 Hz instead of
+1 Hz for THMC50). It can be also put in a new mode to handle additional
+remote temperature sensor. Some pins change their meaning in this mode.
+If the sensor is wired to work in this mode but is not set with adm1022_temp3
+parameter a fan is forced to full output.
+
+Driver Features
+---------------
+
+The driver provides up to three temperatures:
+
+temp1		-- internal
+temp2		-- remote
+temp3		-- 2nd remote only for ADM1022
+
+pwm1		-- fan speed (0 = stop, 255 = full)
+pwm1_mode	-- always 0 (DC mode)
+
+The value of 0 for pwm1 also forces FAN_OFF signal from the chip,
+so it stops fans even if the value 0 into the ANALOG_OUT register does not.
+
+The driver was tested on Compaq AP550 with two ADM1022 chips (one works 
+in the temp3 mode), five temperature readings and two fans.
+
diff -urpN linux-2.6.21/drivers/hwmon/Kconfig linux-2.6.22/drivers/hwmon/Kconfig
--- linux-2.6.21/drivers/hwmon/Kconfig	2007-06-24 22:14:45.824824857 +0200
+++ linux-2.6.22/drivers/hwmon/Kconfig	2007-06-24 22:24:58.859759689 +0200
@@ -485,6 +485,16 @@ config SENSORS_SMSC47B397
 	  This driver can also be built as a module.  If so, the module
 	  will be called smsc47b397.
 
+config SENSORS_THMC50
+	tristate "Texas Instruments THMC50 / Analog Devices ADM1022"
+	depends on I2C && EXPERIMENTAL
+	help
+	  If you say yes here you get support for Texas Instruments THMC50
+	  sensor chips and clones: the Analog Devices ADM1022.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called thmc50.
+
 config SENSORS_VIA686A
 	tristate "VIA686A"
 	depends on I2C && PCI
diff -urpN linux-2.6.21/drivers/hwmon/Makefile linux-2.6.22/drivers/hwmon/Makefile
--- linux-2.6.21/drivers/hwmon/Makefile	2007-06-24 22:14:45.824824857 +0200
+++ linux-2.6.22/drivers/hwmon/Makefile	2007-06-24 22:22:56.852806945 +0200
@@ -53,6 +53,7 @@ obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595
 obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
 obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
 obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
+obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
 obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
 obj-$(CONFIG_SENSORS_VT1211)	+= vt1211.o
 obj-$(CONFIG_SENSORS_VT8231)	+= vt8231.o
diff -urpN linux-2.6.21/drivers/hwmon/thmc50.c linux-2.6.22/drivers/hwmon/thmc50.c
--- linux-2.6.21/drivers/hwmon/thmc50.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.22/drivers/hwmon/thmc50.c	2007-07-02 19:43:59.519740474 +0200
@@ -0,0 +1,454 @@
+/*
+    thmc50.c - Part of lm_sensors, Linux kernel modules for hardware
+             monitoring
+    Copyright (C) 2007 Krzysztof Helt <krzysztof.h1@wp.pl>
+    Based on 2.4 driver by Frodo Looijaard <frodol@dds.nl> and
+    Philip Edelbrock <phil@netroedge.com>
+
+    This program is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation; either version 2 of the License, or
+    (at your option) any later version.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with this program; if not, write to the Free Software
+    Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+*/
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+
+MODULE_LICENSE("GPL");
+
+/* Addresses to scan */
+static unsigned short normal_i2c[] = { 0x2c, 0x2d, 0x2e, I2C_CLIENT_END };
+
+/* Insmod parameters */
+I2C_CLIENT_INSMOD_2(thmc50,adm1022);
+I2C_CLIENT_MODULE_PARM(adm1022_temp3, "List of adapter,address pairs "
+			"to enable 3rd temperature (ADM1022 only)");
+
+/* Many THMC50 constants specified below */
+
+/* The THMC50 registers */
+#define	THMC50_REG_TEMP				0x27
+#define THMC50_REG_CONF				0x40
+#define THMC50_REG_TEMP_HYST			0x3A
+#define THMC50_REG_TEMP_OS			0x39
+
+#define	THMC50_REG_TEMP_TRIP			0x13
+#define THMC50_REG_TEMP_REMOTE_TRIP		0x14
+#define THMC50_REG_TEMP_DEFAULT_TRIP		0x17
+#define THMC50_REG_TEMP_REMOTE_DEFAULT_TRIP	0x18
+#define THMC50_REG_ANALOG_OUT			0x19
+#define THMC50_REG_REMOTE_TEMP			0x26
+#define THMC50_REG_REMOTE_TEMP_HYST		0x38
+#define THMC50_REG_REMOTE_TEMP_OS		0x37
+#define	ADM1022_REG_REMOTE_TEMP2		0x20
+#define	ADM1022_REG_REMOTE_TEMP2_HYST		0x2C
+#define ADM1022_REG_REMOTE_TEMP2_OS		0x2B
+
+#define THMC50_REG_INTER			0x41
+#define THMC50_REG_INTER_MIRROR			0x4C
+#define THMC50_REG_INTER_MASK			0x43
+
+#define THMC50_REG_COMPANY_ID			0x3E
+#define THMC50_REG_DIE_CODE			0x3F
+
+#define THMC50_REG_CONF_FANOFF			0x20
+
+/* Each client has this additional data */
+struct thmc50_data {
+	struct i2c_client client;
+	struct class_device *class_dev;
+
+	struct mutex update_lock;
+	char valid;		/* !=0 if following fields are valid */
+	enum chips type;
+	unsigned long last_updated;	/* In jiffies */
+
+	/* Register values */
+	s8 temp_input;
+	s8 temp_max;
+	s8 temp_hyst;
+	s8 remote_temp_input;
+	s8 remote_temp_max;
+	s8 remote_temp_hyst;
+	s8 remote_temp2_input;
+	s8 remote_temp2_max;
+	s8 remote_temp2_hyst;
+	u8 analog_out;
+	u8 config_reg;
+};
+
+static int thmc50_attach_adapter(struct i2c_adapter *adapter);
+static int thmc50_detach_client(struct i2c_client *client);
+static void thmc50_init_client(struct i2c_client *client);
+static struct thmc50_data *thmc50_update_device(struct device *dev);
+
+static struct i2c_driver thmc50_driver = {
+	.driver = {
+		.name		= "thmc50 sensor chip driver",
+	},
+	.attach_adapter	= thmc50_attach_adapter,
+	.detach_client	= thmc50_detach_client,
+};
+
+#define show(value)						\
+static ssize_t show_##value(struct device *dev, 		\
+			    struct device_attribute *attr,	\
+	   		    char *buf)				\
+{								\
+	struct thmc50_data *data = thmc50_update_device(dev);	\
+	return sprintf(buf, "%d\n", 1000 * data->value);	\
+}
+show(temp_input);
+show(temp_max);
+show(temp_hyst);
+show(remote_temp_input);
+show(remote_temp_max);
+show(remote_temp_hyst);
+show(remote_temp2_input);
+show(remote_temp2_max);
+show(remote_temp2_hyst);
+
+static ssize_t show_analog_out(struct device *dev,
+	       		       struct device_attribute *attr,
+			       char *buf)
+{
+	struct thmc50_data *data = thmc50_update_device(dev);
+	return sprintf(buf, "%d\n", data->analog_out);
+}
+
+#define set(value, reg)	\
+static ssize_t set_##value(struct device *dev,			\
+		       	   struct device_attribute *attr,	\
+	       		   const char *buf, size_t count)	\
+{								\
+	struct i2c_client *client = to_i2c_client(dev);		\
+	struct thmc50_data *data = i2c_get_clientdata(client);	\
+	int temp = simple_strtoul(buf, NULL, 10);		\
+								\
+	mutex_lock(&data->update_lock);				\
+	data->value = temp / 1000;				\
+	i2c_smbus_write_byte_data(client, reg, data->value);	\
+	mutex_unlock(&data->update_lock);			\
+	return count;						\
+}
+set(temp_max, THMC50_REG_TEMP_OS);
+set(temp_hyst, THMC50_REG_TEMP_HYST);
+set(remote_temp_max, THMC50_REG_REMOTE_TEMP_OS);
+set(remote_temp_hyst, THMC50_REG_REMOTE_TEMP_HYST);
+set(remote_temp2_max, ADM1022_REG_REMOTE_TEMP2_OS);
+set(remote_temp2_hyst, ADM1022_REG_REMOTE_TEMP2_HYST);
+
+static ssize_t set_analog_out(struct device *dev,
+				  struct device_attribute *attr, 
+				  const char *buf,
+			       	  size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct thmc50_data *data = i2c_get_clientdata(client);
+	int tmp = simple_strtoul(buf, NULL, 10);
+
+	mutex_lock(&data->update_lock);
+	data->analog_out = tmp;
+	i2c_smbus_write_byte_data(client, THMC50_REG_ANALOG_OUT,
+		       			data->analog_out);
+	if (tmp = 0)
+		data->config_reg &= ~THMC50_REG_CONF_FANOFF;
+	else
+		data->config_reg |= THMC50_REG_CONF_FANOFF;
+	i2c_smbus_write_byte_data(client, THMC50_REG_CONF, data->config_reg);
+
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
+/* There is only one PWM mode = DC */
+static ssize_t show_pwm_mode(struct device *dev,
+	       		     struct device_attribute *attr,
+			     char *buf)
+{
+	return sprintf(buf, "0\n");
+}
+
+static DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max);
+static DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp_hyst,
+						 set_temp_hyst);
+static DEVICE_ATTR(temp1_input, S_IRUGO, show_temp_input, NULL);
+static DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_remote_temp_max,
+	       				 set_remote_temp_max);
+static DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_remote_temp_hyst,
+						set_remote_temp_hyst);
+static DEVICE_ATTR(temp2_input, S_IRUGO, show_remote_temp_input, NULL);
+static DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_remote_temp2_max,
+						set_remote_temp2_max);
+static DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_remote_temp2_hyst,
+	       				set_remote_temp2_hyst);
+static DEVICE_ATTR(temp3_input, S_IRUGO, show_remote_temp2_input, NULL);
+static DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_analog_out,
+	       				 set_analog_out);
+static DEVICE_ATTR(pwm1_mode, S_IRUGO, show_pwm_mode, NULL);
+
+static struct attribute *thmc50_attributes[] = {
+	&dev_attr_temp1_max.attr,
+	&dev_attr_temp1_min.attr,
+	&dev_attr_temp1_input.attr,
+	&dev_attr_temp2_max.attr,
+	&dev_attr_temp2_min.attr,
+	&dev_attr_temp2_input.attr,
+	&dev_attr_pwm1.attr,
+	&dev_attr_pwm1_mode.attr,
+	NULL
+};
+
+static const struct attribute_group thmc50_group = {
+	.attrs = thmc50_attributes,
+};
+
+/* for ADM1022 3rd temperature mode */
+static struct attribute *adm1022_attributes[] = {
+	&dev_attr_temp3_max.attr,
+	&dev_attr_temp3_min.attr,
+	&dev_attr_temp3_input.attr,
+	NULL
+};
+
+static const struct attribute_group adm1022_group = {
+	.attrs = adm1022_attributes,
+};
+
+static int thmc50_detect(struct i2c_adapter *adapter, int address, int kind)
+{
+	int company;
+	int revision;
+	struct i2c_client *client;
+	struct thmc50_data *data;
+	struct device *dev;
+	int err = 0;
+	const char *type_name = "";
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
+		pr_debug("thmc50.o: detect failed, "
+				"smbus byte data not supported!\n");
+		goto exit;
+	}
+
+	/* OK. For now, we presume we have a valid client. We now create the
+	   client structure, even though we cannot fill it completely yet.
+	   But it allows us to access thmc50 registers. */
+	if (!(data = kzalloc(sizeof(struct thmc50_data), GFP_KERNEL))) {
+		pr_debug("thmc50.o: detect failed, kzalloc failed!\n");
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	client = &data->client;
+	i2c_set_clientdata(client, data);
+	client->addr = address;
+	client->adapter = adapter;
+	client->driver = &thmc50_driver;
+	client->flags = 0;
+	dev = &client->dev;
+
+	pr_debug("thmc50.o: Probing for THMC50 at 0x%2X on bus %d\n",
+	       client->addr, i2c_adapter_id(client->adapter));
+
+	/* Now, we do the remaining detection. */
+	company = i2c_smbus_read_byte_data(client, THMC50_REG_COMPANY_ID);
+	revision = i2c_smbus_read_byte_data(client, THMC50_REG_DIE_CODE);
+
+	if (company = 0x49) {
+		kind = thmc50;
+		pr_debug("thmc50.o: Detected THMC50 "
+				"(version %x, revision %x)\n",
+				(revision >> 4) - 0xc, revision & 0xf);
+	} else if (company = 0x41) {
+		kind = adm1022;
+		pr_debug("thmc50.o: Detected ADM1022 "
+				"(version %x, revision %x)\n",
+				(revision >> 4) - 0xc, revision & 0xf);
+	} else {
+		pr_debug("thmc50.o: Detection of THMC50/ADM1022 failed\n");
+		goto exit_free;
+	}
+
+	/* Determine the chip type - only one kind supported! */
+
+	if (kind = thmc50) {
+		type_name = "thmc50";
+	} else if (kind = adm1022) {
+		type_name = "adm1022";
+		if (adm1022_temp3_num > 0 && adm1022_temp3) {
+			int id = i2c_adapter_id(client->adapter);
+			int i;
+
+			for (i=0; i< adm1022_temp3_num - 1; i+=2) {
+				if (adm1022_temp3[i] = id
+				    && adm1022_temp3[i+1] = address) {
+					data->config_reg = 0x80;
+				}
+			}
+		}
+	}
+	data->type = kind;
+
+	/* Fill in the remaining client fields & put it into the global list */
+	strlcpy(client->name, type_name, I2C_NAME_SIZE);
+	data->valid = 0;
+	mutex_init(&data->update_lock);
+
+	/* Tell the I2C layer a new client has arrived */
+	if ((err = i2c_attach_client(client)))
+		goto exit_free;
+
+	thmc50_init_client(client);
+
+	/* Register sysfs hooks */
+	if ((err = sysfs_create_group(&client->dev.kobj, &thmc50_group)))
+		goto exit_detach;
+
+	/* Register ADM1022 sysfs hooks */
+	if (kind = adm1022)
+		if ((err = sysfs_create_group(&client->dev.kobj,
+					       	&adm1022_group)))
+			goto exit_remove_sysfs_thmc50;
+
+	/* Register a new directory entry with module sensors */
+	data->class_dev = hwmon_device_register(&client->dev);
+	if (IS_ERR(data->class_dev)) {
+		err = PTR_ERR(data->class_dev);
+		goto exit_remove_sysfs;
+	}
+
+	return 0;
+
+exit_remove_sysfs:
+	if (kind = adm1022)
+		sysfs_remove_group(&client->dev.kobj, &adm1022_group);
+exit_remove_sysfs_thmc50:
+	sysfs_remove_group(&client->dev.kobj, &thmc50_group);
+exit_detach:
+	i2c_detach_client(client);
+exit_free:
+	kfree(data);
+exit:
+	return err;
+}
+
+static int thmc50_attach_adapter(struct i2c_adapter *adapter)
+{
+	if (!(adapter->class & I2C_CLASS_HWMON))
+		return 0;
+	return i2c_probe(adapter, &addr_data, thmc50_detect);
+}
+
+static int thmc50_detach_client(struct i2c_client *client)
+{
+	struct thmc50_data *data = i2c_get_clientdata(client);
+	int err;
+
+	hwmon_device_unregister(data->class_dev);
+	sysfs_remove_group(&client->dev.kobj, &thmc50_group);
+	if (data->type = adm1022)
+		sysfs_remove_group(&client->dev.kobj, &adm1022_group);
+
+	if ((err = i2c_detach_client(client)))
+		return err;
+
+	kfree(data);
+
+	return 0;
+}
+
+static void thmc50_init_client(struct i2c_client *client)
+{
+	struct thmc50_data *data = i2c_get_clientdata(client);
+
+	data->config_reg |= 0x23;
+	i2c_smbus_write_byte_data(client, THMC50_REG_CONF, data->config_reg);
+	data->analog_out = i2c_smbus_read_byte_data(client,
+						THMC50_REG_ANALOG_OUT);
+	/* set up to at least 1 */
+	if (data->analog_out = 0 ) {
+		data->analog_out = 1;
+		i2c_smbus_write_byte_data(client, THMC50_REG_ANALOG_OUT,
+				       		data->analog_out);
+	}
+}
+
+static struct thmc50_data *thmc50_update_device(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct thmc50_data *data = i2c_get_clientdata(client);
+	int timeout = HZ / 5 + (data->type = thmc50 ? HZ : 0);
+
+	mutex_lock(&data->update_lock);
+
+	if (time_after(jiffies, data->last_updated + timeout)
+	    || !data->valid) {
+
+		data->temp_input = i2c_smbus_read_byte_data(client,
+						THMC50_REG_TEMP);
+		data->temp_max +		    i2c_smbus_read_byte_data(client, THMC50_REG_TEMP_OS);
+		data->temp_hyst +		    i2c_smbus_read_byte_data(client, THMC50_REG_TEMP_HYST);
+		data->remote_temp_input +		    i2c_smbus_read_byte_data(client, THMC50_REG_REMOTE_TEMP);
+		data->remote_temp_max +		    i2c_smbus_read_byte_data(client,
+				   		THMC50_REG_REMOTE_TEMP_OS);
+		data->remote_temp_hyst +		    i2c_smbus_read_byte_data(client,
+				   		THMC50_REG_REMOTE_TEMP_HYST);
+		data->analog_out +		    i2c_smbus_read_byte_data(client, THMC50_REG_ANALOG_OUT);
+		data->config_reg +		    i2c_smbus_read_byte_data(client, THMC50_REG_CONF);
+		if (data->type = adm1022) {
+			data->remote_temp2_input +			    i2c_smbus_read_byte_data(client,
+					    	ADM1022_REG_REMOTE_TEMP2);
+			data->remote_temp2_max +			    i2c_smbus_read_byte_data(client,
+					   	ADM1022_REG_REMOTE_TEMP2_OS);
+			data->remote_temp2_hyst +			    i2c_smbus_read_byte_data(client,
+					   	ADM1022_REG_REMOTE_TEMP2_HYST);
+		}
+		data->last_updated = jiffies;
+		data->valid = 1;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return data;
+}
+
+static int __init sm_thmc50_init(void)
+{
+	return i2c_add_driver(&thmc50_driver);
+}
+
+static void __exit sm_thmc50_exit(void)
+{
+	i2c_del_driver(&thmc50_driver);
+}
+
+MODULE_AUTHOR("Krzysztof Helt <krzysztof.h1@wp.pl>");
+MODULE_DESCRIPTION("THMC50 driver");
+
+module_init(sm_thmc50_init);
+module_exit(sm_thmc50_exit);

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] THMC50 support for 2.6 (2nd revision)

[Jean Delvare]

Hi Krzysztof,

On Mon, 2 Jul 2007 19:54:48 +0200, Krzysztof Helt wrote:
> From: Krzysztof Helt <krzysztof.h1@wp.pl>
> 
> This patch adds support for THMC50 and ADM1022 chips to 2.6 kernels.
> 
> Special thanks to Jean Delavare and Mark M. Hoffman for review of the first patch.
> 
> Signed-off-by: Krzysztof Helt <krzysztof.h1 at wp.pl>
> 
> ---
> This patch incorporates all suggestions and requests to the previously sent patch.
> 
> diff -urpN linux-2.6.21/Documentation/hwmon/thmc50
> linux-2.6.22/Documentation/hwmon/thmc50 ---
> linux-2.6.21/Documentation/hwmon/thmc50	1970-01-01 01:00:00.000000000 +0100 +++
> linux-2.6.22/Documentation/hwmon/thmc50	2007-07-02 19:46:42.029001269 +0200 @@
> -0,0 +1,86 @@ +Kernel driver thmc50
> +==========
Looks like your mailer broke your patch. This causes
Documentation/hwmon/thmc50 not to be created when I apply the patch.
Please check and fix it. The strange thing is that the rest of the
patch looks OK, just these first few lines were wrapped.

> +
> +Supported chips:
> +  * Analog Devices ADM1022
> +    Prefix: 'adm1022'
> +    Addresses scanned: I2C 0x2c - 0x2e
> +    Datasheet: http://www.analog.com/en/prod/0,2877,ADM1022,00.html
> +  * Texas Instruments THMC50
> +    Prefix: 'thmc50'
> +    Addresses scanned: I2C 0x2c - 0x2e
> +    Datasheet: http://focus.ti.com/docs/prod/folders/print/thmc50.html
> +
> +Author: Krzysztof Helt <krzysztof.h1@wp.pl>
> +
> +This driver was derived from the 2.4 kernel thmc50.c source file.
> +
> +Credits:
> +  thmc50.c (2.4 kernel):
> +	Frodo Looijaard <frodol@dds.nl>
> +	Philip Edelbrock <phil@netroedge.com>
> +
> +Module Parameters
> +-----------------
> +
> +* force: short array (min = 1, max = 48)
> +  List of adapter,address pairs to boldly assume to be present
> +* force_thmc50: short array (min = 1, max = 48)
> +  List of adapter,address pairs which are unquestionably assumed to contain
> +  a `thmc50' chip
> +* ignore: short array (min = 1, max = 48)
> +  List of adapter,address pairs not to scan
> +* ignore_range: short array (min = 1, max = 48)
> +  List of adapter,start-addr,end-addr triples not to scan
> +* probe: short array (min = 1, max = 48)
> +  List of adapter,address pairs to scan additionally
> +* probe_range: short array (min = 1, max = 48)
> +  List of adapter,start-addr,end-addr triples to scan additionally

I suggest that you don't list these. These are standard parameters
generated automatically, we don't document them in Linux 2.6. Only
document the parameters which are specific to your driver.

> +* adm1022_temp3: short array (min = 1, max = 3)

I doubt that the max is really "3". At least your code doesn't exhibit
such a limit.

> +  List of adapter,address pairs of ADM1022 chips to set into measuring
> +  additional remote temperature.
> +
> +
> +Description
> +-----------
> +
> +The THMC50 implements: an internal temperature sensor, support for an
> +external diode-type temperature sensor (compatible w/ the diode sensor inside
> +many processors), and a controllable fan/analog_out DAC. For the temperature
> +sensors, limits can be set through the appropriate Overtemperature Shutdown
> +register and Hysteresis register. Each value can be set and read to half-degree
> +accuracy.  An alarm is issued (usually to a connected LM78) when the
> +temperature gets higher then the Overtemperature Shutdown value; it stays on
> +until the temperature falls below the Hysteresis value. All temperatures are in
> +degrees Celsius, and are guaranteed within a range of -55 to +125 degrees.
> +
> +The THMC50 only updates its values each 1.5 seconds; reading it more often
> +will do no harm, but will return 'old' values.
> +
> +The THMC50 is usually used in combination with LM78-like chips, to measure
> +the temperature of the processor(s).
> +
> +The ADM1022 works the same as THMC50 but it is faster (5 Hz instead of
> +1 Hz for THMC50). It can be also put in a new mode to handle additional
> +remote temperature sensor. Some pins change their meaning in this mode.
> +If the sensor is wired to work in this mode but is not set with adm1022_temp3
> +parameter a fan is forced to full output.

This last sentence makes no sense to me. Can you try to reformulate it?

BTW, I would expect the mode to be set properly by the BIOS. Wasn't it
the case for you?

> +
> +Driver Features
> +---------------
> +
> +The driver provides up to three temperatures:
> +
> +temp1		-- internal
> +temp2		-- remote
> +temp3		-- 2nd remote only for ADM1022
> +
> +pwm1		-- fan speed (0 = stop, 255 = full)
> +pwm1_mode	-- always 0 (DC mode)
> +
> +The value of 0 for pwm1 also forces FAN_OFF signal from the chip,
> +so it stops fans even if the value 0 into the ANALOG_OUT register does not.
> +
> +The driver was tested on Compaq AP550 with two ADM1022 chips (one works 
> +in the temp3 mode), five temperature readings and two fans.
> +
> diff -urpN linux-2.6.21/drivers/hwmon/Kconfig linux-2.6.22/drivers/hwmon/Kconfig
> --- linux-2.6.21/drivers/hwmon/Kconfig	2007-06-24 22:14:45.824824857 +0200
> +++ linux-2.6.22/drivers/hwmon/Kconfig	2007-06-24 22:24:58.859759689 +0200
> @@ -485,6 +485,16 @@ config SENSORS_SMSC47B397
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called smsc47b397.
>  
> +config SENSORS_THMC50
> +	tristate "Texas Instruments THMC50 / Analog Devices ADM1022"
> +	depends on I2C && EXPERIMENTAL
> +	help
> +	  If you say yes here you get support for Texas Instruments THMC50
> +	  sensor chips and clones: the Analog Devices ADM1022.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called thmc50.
> +
>  config SENSORS_VIA686A
>  	tristate "VIA686A"
>  	depends on I2C && PCI
> diff -urpN linux-2.6.21/drivers/hwmon/Makefile linux-2.6.22/drivers/hwmon/Makefile
> --- linux-2.6.21/drivers/hwmon/Makefile	2007-06-24 22:14:45.824824857 +0200
> +++ linux-2.6.22/drivers/hwmon/Makefile	2007-06-24 22:22:56.852806945 +0200
> @@ -53,6 +53,7 @@ obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595
>  obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
>  obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
>  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
> +obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
>  obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
>  obj-$(CONFIG_SENSORS_VT1211)	+= vt1211.o
>  obj-$(CONFIG_SENSORS_VT8231)	+= vt8231.o
> diff -urpN linux-2.6.21/drivers/hwmon/thmc50.c linux-2.6.22/drivers/hwmon/thmc50.c
> --- linux-2.6.21/drivers/hwmon/thmc50.c	1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.22/drivers/hwmon/thmc50.c	2007-07-02 19:43:59.519740474 +0200
> @@ -0,0 +1,454 @@
> +/*
> +    thmc50.c - Part of lm_sensors, Linux kernel modules for hardware
> +             monitoring
> +    Copyright (C) 2007 Krzysztof Helt <krzysztof.h1@wp.pl>
> +    Based on 2.4 driver by Frodo Looijaard <frodol@dds.nl> and
> +    Philip Edelbrock <phil@netroedge.com>
> +
> +    This program is free software; you can redistribute it and/or modify
> +    it under the terms of the GNU General Public License as published by
> +    the Free Software Foundation; either version 2 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +
> +    You should have received a copy of the GNU General Public License
> +    along with this program; if not, write to the Free Software
> +    Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +
> +MODULE_LICENSE("GPL");
> +
> +/* Addresses to scan */
> +static unsigned short normal_i2c[] = { 0x2c, 0x2d, 0x2e, I2C_CLIENT_END };
> +
> +/* Insmod parameters */
> +I2C_CLIENT_INSMOD_2(thmc50,adm1022);

Coding style: space after comma.

> +I2C_CLIENT_MODULE_PARM(adm1022_temp3, "List of adapter,address pairs "
> +			"to enable 3rd temperature (ADM1022 only)");
> +
> +/* Many THMC50 constants specified below */
> +
> +/* The THMC50 registers */
> +#define	THMC50_REG_TEMP				0x27
> +#define THMC50_REG_CONF				0x40
> +#define THMC50_REG_TEMP_HYST			0x3A
> +#define THMC50_REG_TEMP_OS			0x39
> +
> +#define	THMC50_REG_TEMP_TRIP			0x13
> +#define THMC50_REG_TEMP_REMOTE_TRIP		0x14
> +#define THMC50_REG_TEMP_DEFAULT_TRIP		0x17
> +#define THMC50_REG_TEMP_REMOTE_DEFAULT_TRIP	0x18
> +#define THMC50_REG_ANALOG_OUT			0x19
> +#define THMC50_REG_REMOTE_TEMP			0x26
> +#define THMC50_REG_REMOTE_TEMP_HYST		0x38
> +#define THMC50_REG_REMOTE_TEMP_OS		0x37
> +#define	ADM1022_REG_REMOTE_TEMP2		0x20
> +#define	ADM1022_REG_REMOTE_TEMP2_HYST		0x2C
> +#define ADM1022_REG_REMOTE_TEMP2_OS		0x2B
> +
> +#define THMC50_REG_INTER			0x41
> +#define THMC50_REG_INTER_MIRROR			0x4C
> +#define THMC50_REG_INTER_MASK			0x43

You don't make any use of these THMC50_REG_INTER* defines, so why
define them? (What is this "inter" thing, BTW?)

> +
> +#define THMC50_REG_COMPANY_ID			0x3E
> +#define THMC50_REG_DIE_CODE			0x3F
> +
> +#define THMC50_REG_CONF_FANOFF			0x20
> +
> +/* Each client has this additional data */
> +struct thmc50_data {
> +	struct i2c_client client;
> +	struct class_device *class_dev;
> +
> +	struct mutex update_lock;
> +	char valid;		/* !=0 if following fields are valid */
> +	enum chips type;
> +	unsigned long last_updated;	/* In jiffies */
> +
> +	/* Register values */
> +	s8 temp_input;
> +	s8 temp_max;
> +	s8 temp_hyst;
> +	s8 remote_temp_input;
> +	s8 remote_temp_max;
> +	s8 remote_temp_hyst;
> +	s8 remote_temp2_input;
> +	s8 remote_temp2_max;
> +	s8 remote_temp2_hyst;
> +	u8 analog_out;
> +	u8 config_reg;
> +};
> +
> +static int thmc50_attach_adapter(struct i2c_adapter *adapter);
> +static int thmc50_detach_client(struct i2c_client *client);
> +static void thmc50_init_client(struct i2c_client *client);
> +static struct thmc50_data *thmc50_update_device(struct device *dev);
> +
> +static struct i2c_driver thmc50_driver = {
> +	.driver = {
> +		.name		= "thmc50 sensor chip driver",

Should be just "thmc50", as Mark told you already?

> +	},
> +	.attach_adapter	= thmc50_attach_adapter,
> +	.detach_client	= thmc50_detach_client,
> +};
> +
> +#define show(value)						\
> +static ssize_t show_##value(struct device *dev, 		\
> +			    struct device_attribute *attr,	\
> +	   		    char *buf)				\
> +{								\
> +	struct thmc50_data *data = thmc50_update_device(dev);	\
> +	return sprintf(buf, "%d\n", 1000 * data->value);	\
> +}
> +show(temp_input);
> +show(temp_max);
> +show(temp_hyst);
> +show(remote_temp_input);
> +show(remote_temp_max);
> +show(remote_temp_hyst);
> +show(remote_temp2_input);
> +show(remote_temp2_max);
> +show(remote_temp2_hyst);
> +
> +static ssize_t show_analog_out(struct device *dev,
> +	       		       struct device_attribute *attr,
> +			       char *buf)
> +{
> +	struct thmc50_data *data = thmc50_update_device(dev);
> +	return sprintf(buf, "%d\n", data->analog_out);
> +}
> +
> +#define set(value, reg)	\
> +static ssize_t set_##value(struct device *dev,			\
> +		       	   struct device_attribute *attr,	\
> +	       		   const char *buf, size_t count)	\
> +{								\
> +	struct i2c_client *client = to_i2c_client(dev);		\
> +	struct thmc50_data *data = i2c_get_clientdata(client);	\
> +	int temp = simple_strtoul(buf, NULL, 10);		\

Temperatures can be negative, so you should use simple_strtol(). Also,
this misses a range check. A value of 130000 (user wants to set the
limit to 130 degrees C) would result in a negative limit to be written
to the register. Please use SENSORS_LIMIT to constraint the user input
into a range that fits in the register.

> +								\
> +	mutex_lock(&data->update_lock);				\
> +	data->value = temp / 1000;				\
> +	i2c_smbus_write_byte_data(client, reg, data->value);	\
> +	mutex_unlock(&data->update_lock);			\
> +	return count;						\
> +}
> +set(temp_max, THMC50_REG_TEMP_OS);
> +set(temp_hyst, THMC50_REG_TEMP_HYST);
> +set(remote_temp_max, THMC50_REG_REMOTE_TEMP_OS);
> +set(remote_temp_hyst, THMC50_REG_REMOTE_TEMP_HYST);
> +set(remote_temp2_max, ADM1022_REG_REMOTE_TEMP2_OS);
> +set(remote_temp2_hyst, ADM1022_REG_REMOTE_TEMP2_HYST);

Side note: all these macro-generated functions should replaced by nice
dynamic sysfs callbacks.

> +
> +static ssize_t set_analog_out(struct device *dev,
> +				  struct device_attribute *attr, 
> +				  const char *buf,
> +			       	  size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct thmc50_data *data = i2c_get_clientdata(client);
> +	int tmp = simple_strtoul(buf, NULL, 10);

You should check the input value for validity. Right now, writing 256
to the file would stop the fan, that's not very user-friendly. Either
map all values greater than 255 to 255 (other drivers to that) or
reject them, at your option.

> +
> +	mutex_lock(&data->update_lock);
> +	data->analog_out = tmp;
> +	i2c_smbus_write_byte_data(client, THMC50_REG_ANALOG_OUT,
> +		       			data->analog_out);
> +	if (tmp = 0)
> +		data->config_reg &= ~THMC50_REG_CONF_FANOFF;
> +	else
> +		data->config_reg |= THMC50_REG_CONF_FANOFF;
> +	i2c_smbus_write_byte_data(client, THMC50_REG_CONF, data->config_reg);
> +
> +	mutex_unlock(&data->update_lock);
> +	return count;
> +}
> +
> +/* There is only one PWM mode = DC */
> +static ssize_t show_pwm_mode(struct device *dev,
> +	       		     struct device_attribute *attr,
> +			     char *buf)
> +{
> +	return sprintf(buf, "0\n");
> +}
> +
> +static DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max);
> +static DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp_hyst,
> +						 set_temp_hyst);
> +static DEVICE_ATTR(temp1_input, S_IRUGO, show_temp_input, NULL);
> +static DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_remote_temp_max,
> +	       				 set_remote_temp_max);
> +static DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_remote_temp_hyst,
> +						set_remote_temp_hyst);
> +static DEVICE_ATTR(temp2_input, S_IRUGO, show_remote_temp_input, NULL);
> +static DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_remote_temp2_max,
> +						set_remote_temp2_max);
> +static DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_remote_temp2_hyst,
> +	       				set_remote_temp2_hyst);
> +static DEVICE_ATTR(temp3_input, S_IRUGO, show_remote_temp2_input, NULL);
> +static DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_analog_out,
> +	       				 set_analog_out);
> +static DEVICE_ATTR(pwm1_mode, S_IRUGO, show_pwm_mode, NULL);
> +
> +static struct attribute *thmc50_attributes[] = {
> +	&dev_attr_temp1_max.attr,
> +	&dev_attr_temp1_min.attr,
> +	&dev_attr_temp1_input.attr,
> +	&dev_attr_temp2_max.attr,
> +	&dev_attr_temp2_min.attr,
> +	&dev_attr_temp2_input.attr,
> +	&dev_attr_pwm1.attr,
> +	&dev_attr_pwm1_mode.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group thmc50_group = {
> +	.attrs = thmc50_attributes,
> +};
> +
> +/* for ADM1022 3rd temperature mode */
> +static struct attribute *adm1022_attributes[] = {
> +	&dev_attr_temp3_max.attr,
> +	&dev_attr_temp3_min.attr,
> +	&dev_attr_temp3_input.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group adm1022_group = {
> +	.attrs = adm1022_attributes,
> +};
> +
> +static int thmc50_detect(struct i2c_adapter *adapter, int address, int kind)
> +{
> +	int company;
> +	int revision;
> +	struct i2c_client *client;
> +	struct thmc50_data *data;
> +	struct device *dev;
> +	int err = 0;
> +	const char *type_name = "";
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> +		pr_debug("thmc50.o: detect failed, "
> +				"smbus byte data not supported!\n");
> +		goto exit;
> +	}
> +
> +	/* OK. For now, we presume we have a valid client. We now create the
> +	   client structure, even though we cannot fill it completely yet.
> +	   But it allows us to access thmc50 registers. */
> +	if (!(data = kzalloc(sizeof(struct thmc50_data), GFP_KERNEL))) {
> +		pr_debug("thmc50.o: detect failed, kzalloc failed!\n");

Please discard the ".o" suffix. It really doesn't add any value.

> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	client = &data->client;
> +	i2c_set_clientdata(client, data);
> +	client->addr = address;
> +	client->adapter = adapter;
> +	client->driver = &thmc50_driver;
> +	client->flags = 0;

Note that this initialization to 0 isn't actually needed: the kzalloc
above did it for you.

> +	dev = &client->dev;
> +
> +	pr_debug("thmc50.o: Probing for THMC50 at 0x%2X on bus %d\n",
> +	       client->addr, i2c_adapter_id(client->adapter));
> +
> +	/* Now, we do the remaining detection. */
> +	company = i2c_smbus_read_byte_data(client, THMC50_REG_COMPANY_ID);
> +	revision = i2c_smbus_read_byte_data(client, THMC50_REG_DIE_CODE);
> +
> +	if (company = 0x49) {
> +		kind = thmc50;
> +		pr_debug("thmc50.o: Detected THMC50 "
> +				"(version %x, revision %x)\n",
> +				(revision >> 4) - 0xc, revision & 0xf);
> +	} else if (company = 0x41) {
> +		kind = adm1022;
> +		pr_debug("thmc50.o: Detected ADM1022 "
> +				"(version %x, revision %x)\n",
> +				(revision >> 4) - 0xc, revision & 0xf);
> +	} else {
> +		pr_debug("thmc50.o: Detection of THMC50/ADM1022 failed\n");
> +		goto exit_free;
> +	}

This detection is a bit weak, you rely solely on the manufacturer ID,
so your driver would recognize many other chips. I suggest that you
improve it by checking that (revision & 0xf0) is at least 0xc0. If
that's not the case then this has to be a misdetection.

The detection code for these chips in sensors-detect additionally
checks that the MSB of the configuration register isn't set. You may
want to do the same in your driver.

> +
> +	/* Determine the chip type - only one kind supported! */

Comment copied from the original driver, but this is no longer true!

> +
> +	if (kind = thmc50) {
> +		type_name = "thmc50";
> +	} else if (kind = adm1022) {
> +		type_name = "adm1022";
> +		if (adm1022_temp3_num > 0 && adm1022_temp3) {

I fail to see how adm1022_temp3 could evaluate to false. It is a static
array, it has to have an address even if it isn't used. So I think you
can drop this part from the test. And you don't need the first part
either - the for loop below will be skipped automatically if
adm1022_temp3_num = 0.

> +			int id = i2c_adapter_id(client->adapter);
> +			int i;
> +
> +			for (i=0; i< adm1022_temp3_num - 1; i+=2) {

The condition is better expressed "i + 1 < adm1022_temp3_num", to avoid
possible wrapping if adm1022_temp3_num is an unsigned type (I guess it
is.)

Watch your coding style too: missing spaces around some operators.

> +				if (adm1022_temp3[i] = id
> +				    && adm1022_temp3[i+1] = address) {
> +					data->config_reg = 0x80;
> +				}
> +			}
> +		}

This should be moved to thmc50_init_client. Setting data->config_reg
here and reusing it later in thmc50_init_client is error-prone. Also,
you seem to only set the 3-temp mode if the user asked for it. You
should also preserve it if it was already set before the driver was
loaded! In fact this is the primary way this should work. The module
parameter should only be needed to workaround broken BIOSes which do
not properly initialize the chip.

> +	}
> +	data->type = kind;
> +
> +	/* Fill in the remaining client fields & put it into the global list */
> +	strlcpy(client->name, type_name, I2C_NAME_SIZE);
> +	data->valid = 0;

Already zeroed by kzalloc.

> +	mutex_init(&data->update_lock);
> +
> +	/* Tell the I2C layer a new client has arrived */
> +	if ((err = i2c_attach_client(client)))
> +		goto exit_free;
> +
> +	thmc50_init_client(client);
> +
> +	/* Register sysfs hooks */
> +	if ((err = sysfs_create_group(&client->dev.kobj, &thmc50_group)))
> +		goto exit_detach;
> +
> +	/* Register ADM1022 sysfs hooks */
> +	if (kind = adm1022)
> +		if ((err = sysfs_create_group(&client->dev.kobj,
> +					       	&adm1022_group)))
> +			goto exit_remove_sysfs_thmc50;
> +
> +	/* Register a new directory entry with module sensors */
> +	data->class_dev = hwmon_device_register(&client->dev);
> +	if (IS_ERR(data->class_dev)) {
> +		err = PTR_ERR(data->class_dev);
> +		goto exit_remove_sysfs;
> +	}
> +
> +	return 0;
> +
> +exit_remove_sysfs:
> +	if (kind = adm1022)
> +		sysfs_remove_group(&client->dev.kobj, &adm1022_group);
> +exit_remove_sysfs_thmc50:
> +	sysfs_remove_group(&client->dev.kobj, &thmc50_group);
> +exit_detach:
> +	i2c_detach_client(client);
> +exit_free:
> +	kfree(data);
> +exit:
> +	return err;
> +}
> +
> +static int thmc50_attach_adapter(struct i2c_adapter *adapter)
> +{
> +	if (!(adapter->class & I2C_CLASS_HWMON))
> +		return 0;
> +	return i2c_probe(adapter, &addr_data, thmc50_detect);
> +}
> +
> +static int thmc50_detach_client(struct i2c_client *client)
> +{
> +	struct thmc50_data *data = i2c_get_clientdata(client);
> +	int err;
> +
> +	hwmon_device_unregister(data->class_dev);
> +	sysfs_remove_group(&client->dev.kobj, &thmc50_group);
> +	if (data->type = adm1022)
> +		sysfs_remove_group(&client->dev.kobj, &adm1022_group);
> +
> +	if ((err = i2c_detach_client(client)))
> +		return err;
> +
> +	kfree(data);
> +
> +	return 0;
> +}
> +
> +static void thmc50_init_client(struct i2c_client *client)
> +{
> +	struct thmc50_data *data = i2c_get_clientdata(client);
> +
> +	data->config_reg |= 0x23;
> +	i2c_smbus_write_byte_data(client, THMC50_REG_CONF, data->config_reg);

As explained above, you should preserve the chip configuration as much
as possible. Forcing the configuration register to an arbitrary value
is no good. You should implement this as a read-modify-write cycle.
Please explain (in a comment) which configuration bits you are changing
and why.

> +	data->analog_out = i2c_smbus_read_byte_data(client,
> +						THMC50_REG_ANALOG_OUT);
> +	/* set up to at least 1 */
> +	if (data->analog_out = 0 ) {
> +		data->analog_out = 1;
> +		i2c_smbus_write_byte_data(client, THMC50_REG_ANALOG_OUT,
> +				       		data->analog_out);
> +	}
> +}
> +
> +static struct thmc50_data *thmc50_update_device(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct thmc50_data *data = i2c_get_clientdata(client);
> +	int timeout = HZ / 5 + (data->type = thmc50 ? HZ : 0);
> +
> +	mutex_lock(&data->update_lock);
> +
> +	if (time_after(jiffies, data->last_updated + timeout)
> +	    || !data->valid) {
> +
> +		data->temp_input = i2c_smbus_read_byte_data(client,
> +						THMC50_REG_TEMP);
> +		data->temp_max > +		    i2c_smbus_read_byte_data(client, THMC50_REG_TEMP_OS);
> +		data->temp_hyst > +		    i2c_smbus_read_byte_data(client, THMC50_REG_TEMP_HYST);
> +		data->remote_temp_input > +		    i2c_smbus_read_byte_data(client, THMC50_REG_REMOTE_TEMP);
> +		data->remote_temp_max > +		    i2c_smbus_read_byte_data(client,
> +				   		THMC50_REG_REMOTE_TEMP_OS);
> +		data->remote_temp_hyst > +		    i2c_smbus_read_byte_data(client,
> +				   		THMC50_REG_REMOTE_TEMP_HYST);
> +		data->analog_out > +		    i2c_smbus_read_byte_data(client, THMC50_REG_ANALOG_OUT);
> +		data->config_reg > +		    i2c_smbus_read_byte_data(client, THMC50_REG_CONF);

As far as I can see, you're only using this register value in
set_analog_out, and actually to write it, not to read it. So you
shouldn't cache this value. You should read-modify-write it in
set_analog_out directly. This is both more efficient (you don't read it
at every cycle) and more correct (you don't rely on an old value when
you modify and write the updated value).

> +		if (data->type = adm1022) {
> +			data->remote_temp2_input > +			    i2c_smbus_read_byte_data(client,
> +					    	ADM1022_REG_REMOTE_TEMP2);
> +			data->remote_temp2_max > +			    i2c_smbus_read_byte_data(client,
> +					   	ADM1022_REG_REMOTE_TEMP2_OS);
> +			data->remote_temp2_hyst > +			    i2c_smbus_read_byte_data(client,
> +					   	ADM1022_REG_REMOTE_TEMP2_HYST);
> +		}
> +		data->last_updated = jiffies;
> +		data->valid = 1;
> +	}
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return data;
> +}
> +
> +static int __init sm_thmc50_init(void)
> +{
> +	return i2c_add_driver(&thmc50_driver);
> +}
> +
> +static void __exit sm_thmc50_exit(void)
> +{
> +	i2c_del_driver(&thmc50_driver);
> +}
> +
> +MODULE_AUTHOR("Krzysztof Helt <krzysztof.h1@wp.pl>");
> +MODULE_DESCRIPTION("THMC50 driver");
> +
> +module_init(sm_thmc50_init);
> +module_exit(sm_thmc50_exit);

Please address my comments and resubmit. This starts looking good, so
hopefully this is the last big iteration. Thanks for your patience :)

What about user-space support? Looking at the libsensors/sensors code,
I see that at least the 3rd temperature sensor of the ADM1022 is not
supported in libsensors. And support is entirely missing from sensors?
Do you have a patch to add support? At least the code in the
lm-sensors-3.0.0 branch should support it right away.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] THMC50 support for 2.6 (2nd revision)

[Krzysztof Helt]

On Wed, 4 Jul 2007 11:36:07 +0200
Jean Delvare <khali@linux-fr.org> wrote:

> Hi Krzysztof,
> 
> On Mon, 2 Jul 2007 19:54:48 +0200, Krzysztof Helt wrote:
> > From: Krzysztof Helt <krzysztof.h1@wp.pl>
> > +
> > +The ADM1022 works the same as THMC50 but it is faster (5 Hz instead of
> > +1 Hz for THMC50). It can be also put in a new mode to handle additional
> > +remote temperature sensor. Some pins change their meaning in this mode.
> > +If the sensor is wired to work in this mode but is not set with adm1022_temp3
> > +parameter a fan is forced to full output.
> 
> This last sentence makes no sense to me. Can you try to reformulate it?
> 

I can remove it completely. It was thought as a hint for someone with the fan settings
after loading the driver.
Is it better: "
+If the sensor is supposed to work in this mode but is not set with adm1022_temp3
+parameter a fan can be forced to full speed.

> BTW, I would expect the mode to be set properly by the BIOS. Wasn't it
> the case for you?
> 

I will read the configuration register in the probe function, so it will be preserved.
Good hint.

> > +#define THMC50_REG_INTER			0x41
> > +#define THMC50_REG_INTER_MIRROR			0x4C
> > +#define THMC50_REG_INTER_MASK			0x43
> 
> You don't make any use of these THMC50_REG_INTER* defines, so why
> define them? (What is this "inter" thing, BTW?)
> 

They are interrupt registers (mask, status and status mirror).
I'll remove them as THMC50's datasheet is still available.

> > +		.name		= "thmc50 sensor chip driver",
> 
> Should be just "thmc50", as Mark told you already?

I misunderstood him and changed only letters from capital ones.
 
> > +set(temp_max, THMC50_REG_TEMP_OS);
> > +set(temp_hyst, THMC50_REG_TEMP_HYST);
> > +set(remote_temp_max, THMC50_REG_REMOTE_TEMP_OS);
> > +set(remote_temp_hyst, THMC50_REG_REMOTE_TEMP_HYST);
> > +set(remote_temp2_max, ADM1022_REG_REMOTE_TEMP2_OS);
> > +set(remote_temp2_hyst, ADM1022_REG_REMOTE_TEMP2_HYST);
> 
> Side note: all these macro-generated functions should replaced by nice
> dynamic sysfs callbacks.
> 

Should I rewrite it before posting a driver?

> > +	client->driver = &thmc50_driver;
> > +	client->flags = 0;
> 
> Note that this initialization to 0 isn't actually needed: the kzalloc
> above did it for you.
> 

I wonder if it is worth to go for kmalloc instead because most of fields (if not all)
are set anyway.

> > +	if (company = 0x49) {
> > +		kind = thmc50;
> > +	} else if (company = 0x41) {
> > +		kind = adm1022;
> > +	} else {
> > +		pr_debug("thmc50.o: Detection of THMC50/ADM1022 failed\n");
> > +		goto exit_free;
> > +	}
> 
> [...]
> 
> The detection code for these chips in sensors-detect additionally
> checks that the MSB of the configuration register isn't set. You may
> want to do the same in your driver.
> 

I thought about it but I have to know what this kind (data->type) value means. The
driver works with two chips (thmc50 & adm1022) and the adm1022 chips has two modes.
The adm1022 is identical to thmc50 in the first mode and different (additional remote
temp) in the second mode.

If I use the MSB bit to detect adm1022 it means that it is the adm1022 chip in the
second mode. The adm1022 in the first mode would be detected as the thmc50.

If I use the manufacturer code for recognition of the chip, the adm1022 is always
adm1022 regardless the MSB in the configuration register (so that bit can be completely
ignored).

Should the data->type store the chip type or its working mode?


> What about user-space support? Looking at the libsensors/sensors code,
> I see that at least the 3rd temperature sensor of the ADM1022 is not
> supported in libsensors. And support is entirely missing from sensors?
> Do you have a patch to add support? At least the code in the
> lm-sensors-3.0.0 branch should support it right away.
> 

It works with sensors3 and after addition to the sensors.conf it works with the AP550
MB (motherboard). 

I have problem how to name temperature values. There are 5 temperature values:

1 & 2 - inside CPUs (I have to find which one is CPU0)
3 - in the chip which is mounted on the MB next to CPUs (corner of the MB - the air from
CPUs is ducted there)
4 - in the chip which is mounted on the MB next to memory slot (which is in the half
length of the MB)
5 - remote temperature from the second chip. I don't know where the sensor is located
but it is 5 degrees higher then the temperature 4 (maybe inside RIMM memory or next
to graphics card?).

Any help appreciated.

Regards,
Krzysztof

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] THMC50 support for 2.6 (2nd revision)

[Mark M. Hoffman]

Hi Krzysztof:

* Krzysztof Helt <krzysztof.h1@wp.pl> [2007-07-04 19:16:18 +0200]:
> On Wed, 4 Jul 2007 11:36:07 +0200
> Jean Delvare <khali@linux-fr.org> wrote:
> 
> > Hi Krzysztof,
> > 
> > On Mon, 2 Jul 2007 19:54:48 +0200, Krzysztof Helt wrote:
> > > From: Krzysztof Helt <krzysztof.h1@wp.pl>
> > > +
> > > +The ADM1022 works the same as THMC50 but it is faster (5 Hz instead of
> > > +1 Hz for THMC50). It can be also put in a new mode to handle additional
> > > +remote temperature sensor. Some pins change their meaning in this mode.
> > > +If the sensor is wired to work in this mode but is not set with adm1022_temp3
> > > +parameter a fan is forced to full output.
> > 
> > This last sentence makes no sense to me. Can you try to reformulate it?
> > 
> 
> I can remove it completely. It was thought as a hint for someone with the fan settings
> after loading the driver.
> Is it better: "
> +If the sensor is supposed to work in this mode but is not set with adm1022_temp3
> +parameter a fan can be forced to full speed.
> 
> > BTW, I would expect the mode to be set properly by the BIOS. Wasn't it
> > the case for you?
> > 
> 
> I will read the configuration register in the probe function, so it will be preserved.
> Good hint.
> 
> > > +#define THMC50_REG_INTER			0x41
> > > +#define THMC50_REG_INTER_MIRROR			0x4C
> > > +#define THMC50_REG_INTER_MASK			0x43
> > 
> > You don't make any use of these THMC50_REG_INTER* defines, so why
> > define them? (What is this "inter" thing, BTW?)
> > 
> 
> They are interrupt registers (mask, status and status mirror).
> I'll remove them as THMC50's datasheet is still available.

I personally do not mind seeing macros for unused registers.

> > > +		.name		= "thmc50 sensor chip driver",
> > 
> > Should be just "thmc50", as Mark told you already?
> 
> I misunderstood him and changed only letters from capital ones.
>  
> > > +set(temp_max, THMC50_REG_TEMP_OS);
> > > +set(temp_hyst, THMC50_REG_TEMP_HYST);
> > > +set(remote_temp_max, THMC50_REG_REMOTE_TEMP_OS);
> > > +set(remote_temp_hyst, THMC50_REG_REMOTE_TEMP_HYST);
> > > +set(remote_temp2_max, ADM1022_REG_REMOTE_TEMP2_OS);
> > > +set(remote_temp2_hyst, ADM1022_REG_REMOTE_TEMP2_HYST);
> > 
> > Side note: all these macro-generated functions should replaced by nice
> > dynamic sysfs callbacks.
> > 
> 
> Should I rewrite it before posting a driver?
> 
> > > +	client->driver = &thmc50_driver;
> > > +	client->flags = 0;
> > 
> > Note that this initialization to 0 isn't actually needed: the kzalloc
> > above did it for you.
> > 
> 
> I wonder if it is worth to go for kmalloc instead because most of fields (if not all)
> are set anyway.

You still want kzalloc because struct i2c_client contains a whole struct device.

> > > +	if (company = 0x49) {
> > > +		kind = thmc50;
> > > +	} else if (company = 0x41) {
> > > +		kind = adm1022;
> > > +	} else {
> > > +		pr_debug("thmc50.o: Detection of THMC50/ADM1022 failed\n");
> > > +		goto exit_free;
> > > +	}
> > 
> > [...]
> > 
> > The detection code for these chips in sensors-detect additionally
> > checks that the MSB of the configuration register isn't set. You may
> > want to do the same in your driver.
> > 
> 
> I thought about it but I have to know what this kind (data->type) value means. The
> driver works with two chips (thmc50 & adm1022) and the adm1022 chips has two modes.
> The adm1022 is identical to thmc50 in the first mode and different (additional remote
> temp) in the second mode.
> 
> If I use the MSB bit to detect adm1022 it means that it is the adm1022 chip in the
> second mode. The adm1022 in the first mode would be detected as the thmc50.

I think that would be OK, actually.

> If I use the manufacturer code for recognition of the chip, the adm1022 is always
> adm1022 regardless the MSB in the configuration register (so that bit can be completely
> ignored).
> 
> Should the data->type store the chip type or its working mode?
> 
> 
> > What about user-space support? Looking at the libsensors/sensors code,
> > I see that at least the 3rd temperature sensor of the ADM1022 is not
> > supported in libsensors. And support is entirely missing from sensors?
> > Do you have a patch to add support? At least the code in the
> > lm-sensors-3.0.0 branch should support it right away.
> > 
> 
> It works with sensors3 and after addition to the sensors.conf it works with the AP550
> MB (motherboard). 
> 
> I have problem how to name temperature values. There are 5 temperature values:
> 
> 1 & 2 - inside CPUs (I have to find which one is CPU0)
> 3 - in the chip which is mounted on the MB next to CPUs (corner of the MB - the air from
> CPUs is ducted there)
> 4 - in the chip which is mounted on the MB next to memory slot (which is in the half
> length of the MB)
> 5 - remote temperature from the second chip. I don't know where the sensor is located
> but it is 5 degrees higher then the temperature 4 (maybe inside RIMM memory or next
> to graphics card?).
> 
> Any help appreciated.

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] THMC50 support for 2.6 (2nd revision)

[Jean Delvare]

Hi Krzysztof,

On Wed, 4 Jul 2007 19:16:18 +0200, Krzysztof Helt wrote:
> On Wed, 4 Jul 2007 11:36:07 +0200 Jean Delvare wrote:
> > > +set(temp_max, THMC50_REG_TEMP_OS);
> > > +set(temp_hyst, THMC50_REG_TEMP_HYST);
> > > +set(remote_temp_max, THMC50_REG_REMOTE_TEMP_OS);
> > > +set(remote_temp_hyst, THMC50_REG_REMOTE_TEMP_HYST);
> > > +set(remote_temp2_max, ADM1022_REG_REMOTE_TEMP2_OS);
> > > +set(remote_temp2_hyst, ADM1022_REG_REMOTE_TEMP2_HYST);
> > 
> > Side note: all these macro-generated functions should replaced by nice
> > dynamic sysfs callbacks.
> 
> Should I rewrite it before posting a driver?

No, your code works as is and I don't want to delay the inclusion of
your work into the kernel. But once your driver is accepted upstream,
you could work on a patch to make this part better.

> > The detection code for these chips in sensors-detect additionally
> > checks that the MSB of the configuration register isn't set. You may
> > want to do the same in your driver.
> 
> I thought about it but I have to know what this kind (data->type) value means. The
> driver works with two chips (thmc50 & adm1022) and the adm1022 chips has two modes.
> The adm1022 is identical to thmc50 in the first mode and different (additional remote
> temp) in the second mode.
> 
> If I use the MSB bit to detect adm1022 it means that it is the adm1022 chip in the
> second mode. The adm1022 in the first mode would be detected as the thmc50.

Oops, sorry. I'm now realizing that the sensors-detect code is wrong,
and that confused me. The bit I was thinking of was not bit 7 but bit 4
of the configuration register (Soft Reset). This bit is essentially
write-only so it should always read 0. This can be used to improve the
detection for both chips.

I just fixed sensors-detect so that it checks bit 4 instead of bit 7.
Bit 7 can also be checked for the THMC50 (and the ADM1028) but not the
ADM1022, as you found out. I guess that sensors-detect was only
detecting one of your 2 ADM1022 chips? Can you please try the
sensors-detect version from SVN and confirm that it detects your two
ADM1022 chips properly now?

> If I use the manufacturer code for recognition of the chip, the adm1022 is always
> adm1022 regardless the MSB in the configuration register (so that bit can be completely
> ignored).
> 
> Should the data->type store the chip type or its working mode?

Other drivers store the chip type in data->type (if needed), and add
dedicated fields to the data structure if they need to remember working
modes. This sounds sensible. If you prefer to store the working mode,
than I suggest that you drop the "type" field and have for example a
"has_temp3" field instead.

> > What about user-space support? Looking at the libsensors/sensors code,
> > I see that at least the 3rd temperature sensor of the ADM1022 is not
> > supported in libsensors. And support is entirely missing from sensors?
> > Do you have a patch to add support? At least the code in the
> > lm-sensors-3.0.0 branch should support it right away.
> 
> It works with sensors3 and after addition to the sensors.conf it works with the AP550
> MB (motherboard).

Don't you want to submit support for lm-sensor 2.10.x too? We have no
clear idea when lm-sensors 3.0.0 will be released.

For sensors.conf, I suggest that you write a dedicated configuration
file for your motherboard rather than adding to sensors.conf.eg.

> I have problem how to name temperature values. There are 5 temperature values:
> 
> 1 & 2 - inside CPUs (I have to find which one is CPU0)
> 3 - in the chip which is mounted on the MB next to CPUs (corner of the MB - the air from
> CPUs is ducted there)
> 4 - in the chip which is mounted on the MB next to memory slot (which is in the half
> length of the MB)
> 5 - remote temperature from the second chip. I don't know where the sensor is located
> but it is 5 degrees higher then the temperature 4 (maybe inside RIMM memory or next
> to graphics card?).

Sensor locations are sometimes mentioned on the motherboard manuals, so
you may take a look. Otherwise you can "scan" you motherboard with a
hair-dryer ;) The last remote temperature could be the north bridge or
south bridge temperature.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] THMC50 support for 2.6 (2nd revision)

[Hans de Goede]

Jean Delvare wrote:
> Hi Krzysztof,
> 
>>> What about user-space support? Looking at the libsensors/sensors code,
>>> I see that at least the 3rd temperature sensor of the ADM1022 is not
>>> supported in libsensors. And support is entirely missing from sensors?
>>> Do you have a patch to add support? At least the code in the
>>> lm-sensors-3.0.0 branch should support it right away.
>> It works with sensors3 and after addition to the sensors.conf it works with the AP550
>> MB (motherboard).
> 
> Don't you want to submit support for lm-sensor 2.10.x too? We have no
> clear idea when lm-sensors 3.0.0 will be released.
> 

I know writing support for 2.10.x is not a fun job (see my grumpy mails about 
this a couple of days ago). But I think we will be stuck with 2.10.x for a 
while to come. So it would be best if you could write 2.10.x support soon, 
preferably before the july 8th deadline for the 2.10.4 release svn freeze. That 
way by the time your driver gets into the kernel, people will hopefully also 
have a libsensors + sensors supporting it.

Regards,

Hans


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] THMC50 support for 2.6 (2nd revision)

[Krzysztof Helt]

Dnia 6-07-2007 o godz. 12:01 Jean Delvare napisa³(a):
> Hi Krzysztof,
> 
> On Wed, 4 Jul 2007 19:16:18 +0200, Krzysztof Helt wrote:
> > On Wed, 4 Jul 2007 11:36:07 +0200 Jean Delvare wrote:
> > > > +set(temp_max, THMC50_REG_TEMP_OS);
> > > > +set(temp_hyst, THMC50_REG_TEMP_HYST);
> > > > +set(remote_temp_max, THMC50_REG_REMOTE_TEMP_OS);
> > > > +set(remote_temp_hyst, THMC50_REG_REMOTE_TEMP_HYST);
> > > > +set(remote_temp2_max, ADM1022_REG_REMOTE_TEMP2_OS);
> > > > +set(remote_temp2_hyst, ADM1022_REG_REMOTE_TEMP2_HYST);
> > > 
> > > Side note: all these macro-generated functions should
replaced by nice
> > > dynamic sysfs callbacks.
> > 
> > Should I rewrite it before posting a driver?
> 
> No, 

I changed them after your latest patch which added "dynamic sysfs
callbacks".

> > If I use the MSB bit to detect adm1022 it means that it is
the adm1022 chip in the
> > second mode. The adm1022 in the first mode would be detected
as the thmc50.
> 
> Oops, sorry. I'm now realizing that the sensors-detect code is
wrong,
> and that confused me. The bit I was thinking of was not bit 7
but bit 4
> of the configuration register (Soft Reset). This bit is essentially
> write-only so it should always read 0. This can be used to
improve the
> detection for both chips.
> 

Ok. 

> I just fixed sensors-detect so that it checks bit 4 instead of
bit 7.
> Bit 7 can also be checked for the THMC50 (and the ADM1028) but
not the
> ADM1022, as you found out. I guess that sensors-detect was only
> detecting one of your 2 ADM1022 chips? 

Yes. 

> Can you please try the
> sensors-detect version from SVN and confirm that it detects
your two
> ADM1022 chips properly now?
> 

Yes.

> > Should the data->type store the chip type or its working mode?
> 
> Other drivers store the chip type in data->type (if needed),
and add
> dedicated fields to the data structure if they need to remember
working
> modes. 

I'll think of it.

> > It works with sensors3 and after addition to the sensors.conf
it works with the AP550
> > MB (motherboard).
> 
> Don't you want to submit support for lm-sensor 2.10.x too? We
have no
> clear idea when lm-sensors 3.0.0 will be released.
> 

If I have the driver in the kernel I'll try. 

> For sensors.conf, I suggest that you write a dedicated
configuration
> file for your motherboard rather than adding to sensors.conf.eg.
> 
> > I have problem how to name temperature values. There are 5
temperature values:
> > 
> > 1 & 2 - inside CPUs (I have to find which one is CPU0)
> > 3 - in the chip which is mounted on the MB next to CPUs
(corner of the MB - the air from
> > CPUs is ducted there)
> > 4 - in the chip which is mounted on the MB next to memory
slot (which is in the half
> > length of the MB)
> > 5 - remote temperature from the second chip. I don't know
where the sensor is located
> > but it is 5 degrees higher then the temperature 4 (maybe
inside RIMM memory or next
> > to graphics card?).
> 
> Sensor locations are sometimes mentioned on the motherboard
manuals, [...]


I need help how to name the temperature 3 and 4. They are both on
the MB. Do you have any standard for naming temperatures in the
lmsensors lib (like CPU are always "Temp CPUx")?

Thanks for the tip with hair-dryer.

Regards,
Krzysztof 

----------------------------------------------------
ROLLING STONES - A BIGGER BANG
Warszawa S³u¿ewiec 25 lipca 2007 r. 
http://klik.wp.pl/?adr=http%3A%2F%2Fadv.reklama.wp.pl%2Fas%2Frollingstones.html&sid\x1213



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] THMC50 support for 2.6 (2nd revision)

[Jean Delvare]

Hi Krzysztof,

On Fri, 06 Jul 2007 13:39:47 +0200, Krzysztof Helt wrote:
> I need help how to name the temperature 3 and 4. They are both on
> the MB. Do you have any standard for naming temperatures in the
> lmsensors lib (like CPU are always "Temp CPUx")?

Sensor names don't go into the library. The only place where you give
names to the sensors is in sensors.conf. As you're writing a dedicated
configuration file for your motherboard, you are free to choose
whatever names you like. You can look at other configuration files for
examples (etc/sensors.conf.eg or the ones in the wiki on
lm-sensors.org.)

> Thanks for the tip with hair-dryer.

Be careful when trying this. Electronic components don't like abrupt
temperature changes, so make sure that the air coming out of the
hair-dryer isn't too hot. If it's too hot, you'll need to find a way to
mix it with fresher air, or you might damage your hardware.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors