[lm-sensors] PATCH: hwmon-new-driver-ti-tmp401.patch

[lm-sensors] PATCH: hwmon-new-driver-ti-tmp401.patch

[Hans de Goede]

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

Hi All,

This is a new hwmon driver for TI's TMP401 temperature sensor IC. This driver
was written on behalf of an embedded systems vendor under the
linuxdriverproject.

It has been tested using a TI TMP401 sample attached to a i2c-tiny-usb adapter.
Which was provided by Till Harbaum, many thanks to him for this!

Signed-off-by: Hans de Goede <j.w.r.degoede@hhs.nl>

Regards,

Hans

[-- Attachment #2: hwmon-new-driver-ti-tmp401.patch --]
[-- Type: text/plain, Size: 18271 bytes --]

This is a new hwmon driver for TI's TMP401 temperature sensor IC. This driver
was written on behalf of an embedded systems vendor under the
linuxdriverproject.

It has been tested using a TI TMP401 sample attached to a i2c-tiny-usb adapter.
Which was provided by Till Harbaum, many thanks to him for this! 

Signed-off-by: Hans de Goede <j.w.r.degoede@hhs.nl>
diff -up vanilla-2.6.26-rc5-git2/drivers/hwmon/Kconfig~ vanilla-2.6.26-rc5-git2/drivers/hwmon/Kconfig
--- vanilla-2.6.26-rc5-git2/drivers/hwmon/Kconfig~	2008-06-14 16:51:14.000000000 +0200
+++ vanilla-2.6.26-rc5-git2/drivers/hwmon/Kconfig	2008-06-14 16:51:14.000000000 +0200
@@ -633,6 +633,16 @@ config SENSORS_THMC50
 	  This driver can also be built as a module.  If so, the module
 	  will be called thmc50.
 
+config SENSORS_TMP401
+	tristate "Texas Instruments TMP401"
+	depends on I2C && EXPERIMENTAL
+	help
+	  If you say yes here you get support for Texas Instruments TMP401
+	  temperature sensor chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called tmp401.
+
 config SENSORS_VIA686A
 	tristate "VIA686A"
 	depends on PCI
diff -up vanilla-2.6.26-rc5-git2/drivers/hwmon/Makefile~ vanilla-2.6.26-rc5-git2/drivers/hwmon/Makefile
--- vanilla-2.6.26-rc5-git2/drivers/hwmon/Makefile~	2008-06-14 16:52:19.000000000 +0200
+++ vanilla-2.6.26-rc5-git2/drivers/hwmon/Makefile	2008-06-14 16:52:19.000000000 +0200
@@ -66,6 +66,7 @@ obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc4
 obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
 obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
 obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
+obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
 obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
 obj-$(CONFIG_SENSORS_VT1211)	+= vt1211.o
 obj-$(CONFIG_SENSORS_VT8231)	+= vt8231.o
diff -up vanilla-2.6.26-rc5-git2/drivers/hwmon/tmp401.c~ vanilla-2.6.26-rc5-git2/drivers/hwmon/tmp401.c
--- vanilla-2.6.26-rc5-git2/drivers/hwmon/tmp401.c~	2008-06-14 17:04:10.000000000 +0200
+++ vanilla-2.6.26-rc5-git2/drivers/hwmon/tmp401.c	2008-06-14 17:10:13.000000000 +0200
@@ -0,0 +1,566 @@
+/* tmp401.c
+ *
+ * Copyright (C) 2007 Hans de Goede <j.w.r.degoede@hhs.nl>
+ *
+ * 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.
+ */
+
+/*
+ * Driver for the Texas Instruments TMP401 SMBUS temperature sensor IC.
+ *
+ * Note this IC is in some aspect similar to the lm90, but it has quite a
+ * few differences too, for example the local temp has a higher resolution
+ * and thus has 16 bits registers for its value and limit instead of 8 bits.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/sysfs.h>
+
+/* Addresses to scan */
+static unsigned short normal_i2c[] = { 0x4c, I2C_CLIENT_END };
+
+/* Insmod parameters */
+I2C_CLIENT_INSMOD_1(tmp401);
+
+
+/*
+ * The TMP401 registers, note some registers have different addresses for
+ * reading and writing
+ */
+#define TMP401_STATUS				0x02
+#define TMP401_CONFIG_READ			0x03
+#define TMP401_CONFIG_WRITE			0x09
+#define TMP401_CONVERSION_RATE_READ		0x04
+#define TMP401_CONVERSION_RATE_WRITE		0x0A
+#define TMP401_ONE_SHOT_START_WRITE		0x0F
+#define TMP401_RESOLUTION			0x1A
+#define TMP401_TEMP_CRIT_HYST			0x21
+#define TMP401_CONSECUTIVE_ALERT		0x22
+#define TMP401_MANUFACTURER_ID_REG		0xFE
+#define TMP401_DEVICE_ID_REG			0xFF
+
+static const u8 TMP401_TEMP_MSB[2]			= { 0x00, 0x01 };
+static const u8 TMP401_TEMP_LSB[2]			= { 0x15, 0x10 };
+static const u8 TMP401_TEMP_LOW_LIMIT_MSB_READ[2]	= { 0x06, 0x08 };
+static const u8 TMP401_TEMP_LOW_LIMIT_MSB_WRITE[2]	= { 0x0C, 0x0E };
+static const u8 TMP401_TEMP_LOW_LIMIT_LSB[2]		= { 0x17, 0x14 };
+static const u8 TMP401_TEMP_HIGH_LIMIT_MSB_READ[2]	= { 0x05, 0x07 };
+static const u8 TMP401_TEMP_HIGH_LIMIT_MSB_WRITE[2]	= { 0x0B, 0x0D };
+static const u8 TMP401_TEMP_HIGH_LIMIT_LSB[2]		= { 0x16, 0x13 };
+/* These are called the THERM limit / hysteresis / mask in the datasheets */
+static const u8 TMP401_TEMP_CRIT_LIMIT[2]		= { 0x20, 0x19 };
+
+/* Bit masks */
+#define TMP401_CONFIG_RANGE_MASK		0x04
+#define TMP401_CONFIG_SHUTDOWN_MASK		0x40
+#define TMP401_STATUS_LOCAL_CRIT_MASK		0x01
+#define TMP401_STATUS_REMOTE_CRIT_MASK		0x02
+#define TMP401_STATUS_REMOTE_OPEN_MASK		0x04
+#define TMP401_STATUS_REMOTE_LOW_MASK		0x08
+#define TMP401_STATUS_REMOTE_HIGH_MASK		0x10
+#define TMP401_STATUS_LOCAL_LOW_MASK		0x20
+#define TMP401_STATUS_LOCAL_HIGH_MASK		0x40
+
+/* Manufacturer / Device ID's */
+#define TMP401_MANUFACTURER_ID			0x55
+#define TMP401_DEVICE_ID			0x11
+
+/* our driver name */
+#define TMP401_NAME "tmp401"
+
+/*
+ * Functions declarations
+ */
+
+static void tmp401_init_client(struct i2c_client *client);
+static int tmp401_attach_adapter(struct i2c_adapter *adapter);
+static int tmp401_detach_client(struct i2c_client *client);
+static struct tmp401_data *tmp401_update_device(struct device *dev);
+
+/*
+ * Driver data (common to all clients)
+ */
+
+static struct i2c_driver tmp401_driver = {
+	.driver = {
+		.name	= TMP401_NAME,
+	},
+	.attach_adapter	= tmp401_attach_adapter,
+	.detach_client	= tmp401_detach_client,
+};
+
+/*
+ * Client data (each client gets its own)
+ */
+
+struct tmp401_data {
+	struct i2c_client client;
+	struct device *hwmon_dev;
+	struct mutex update_lock;
+	char valid; /* zero until following fields are valid */
+	unsigned long last_updated; /* in jiffies */
+
+	/* register values */
+	u8 status;
+	u8 config;
+	u16 temp[2];
+	u16 temp_low[2];
+	u16 temp_high[2];
+	u8 temp_crit[2];
+	u8 temp_crit_hyst;
+};
+
+/*
+ * Sysfs attr show / store functions
+ */
+
+static int tmp401_register_to_temp(u16 reg, u8 config)
+{
+	int temp = reg;
+
+	if (config & TMP401_CONFIG_RANGE_MASK)
+		temp -= 64 * 256;
+
+	return (temp * 625) / 160;
+}
+
+static u16 tmp401_temp_to_register(long temp, u8 config)
+{
+	if (config & TMP401_CONFIG_RANGE_MASK) {
+		temp = SENSORS_LIMIT(temp, -64000, 191000);
+		temp += 64000;
+	} else
+		temp = SENSORS_LIMIT(temp, 0, 127000);
+
+	return (temp * 160) / 625;
+}
+
+static int tmp401_crit_register_to_temp(u8 reg, u8 config)
+{
+	int temp = reg;
+
+	if (config & TMP401_CONFIG_RANGE_MASK)
+		temp -= 64;
+
+	return temp * 1000;
+}
+
+static u8 tmp401_temp_crit_to_register(long temp, u8 config)
+{
+	if (config & TMP401_CONFIG_RANGE_MASK) {
+		temp = SENSORS_LIMIT(temp, -64000, 191000);
+		temp += 64000;
+	} else
+		temp = SENSORS_LIMIT(temp, 0, 127000);
+
+	return temp / 1000;
+}
+
+static ssize_t show_temp_value(struct device *dev,
+	struct device_attribute *devattr, char *buf)
+{
+	int index = to_sensor_dev_attr(devattr)->index;
+	struct tmp401_data *data = tmp401_update_device(dev);
+
+	return sprintf(buf, "%d\n",
+		tmp401_register_to_temp(data->temp[index], data->config));
+}
+
+static ssize_t show_temp_min(struct device *dev,
+	struct device_attribute *devattr, char *buf)
+{
+	int index = to_sensor_dev_attr(devattr)->index;
+	struct tmp401_data *data = tmp401_update_device(dev);
+
+	return sprintf(buf, "%d\n",
+		tmp401_register_to_temp(data->temp_low[index], data->config));
+}
+
+static ssize_t show_temp_max(struct device *dev,
+	struct device_attribute *devattr, char *buf)
+{
+	int index = to_sensor_dev_attr(devattr)->index;
+	struct tmp401_data *data = tmp401_update_device(dev);
+
+	return sprintf(buf, "%d\n",
+		tmp401_register_to_temp(data->temp_high[index], data->config));
+}
+
+static ssize_t show_temp_crit(struct device *dev,
+	struct device_attribute *devattr, char *buf)
+{
+	int index = to_sensor_dev_attr(devattr)->index;
+	struct tmp401_data *data = tmp401_update_device(dev);
+
+	return sprintf(buf, "%d\n",
+			tmp401_crit_register_to_temp(data->temp_crit[index],
+							data->config));
+}
+
+static ssize_t show_temp_crit_hyst(struct device *dev,
+	struct device_attribute *devattr, char *buf)
+{
+	int temp, index = to_sensor_dev_attr(devattr)->index;
+	struct tmp401_data *data = tmp401_update_device(dev);
+
+	mutex_lock(&data->update_lock);
+	temp = tmp401_crit_register_to_temp(data->temp_crit[index],
+						data->config);
+	temp -= data->temp_crit_hyst * 1000;
+	mutex_unlock(&data->update_lock);
+
+	return sprintf(buf, "%d\n", temp);
+}
+
+static ssize_t show_status(struct device *dev,
+	struct device_attribute *devattr, char *buf)
+{
+	int mask = to_sensor_dev_attr(devattr)->index;
+	struct tmp401_data *data = tmp401_update_device(dev);
+
+	if (data->status & mask)
+		return sprintf(buf, "1\n");
+	else
+		return sprintf(buf, "0\n");
+}
+
+static ssize_t store_temp_min(struct device *dev, struct device_attribute
+	*devattr, const char *buf, size_t count)
+{
+	int index = to_sensor_dev_attr(devattr)->index;
+	struct tmp401_data *data = tmp401_update_device(dev);
+	long v;
+	u16 reg;
+
+	if (strict_strtol(buf, 10, &v))
+		return -EINVAL;
+
+	reg = tmp401_temp_to_register(v, data->config);
+
+	mutex_lock(&data->update_lock);
+
+	i2c_smbus_write_byte_data(&data->client,
+		TMP401_TEMP_LOW_LIMIT_MSB_WRITE[index], reg >> 8);
+	i2c_smbus_write_byte_data(&data->client,
+		TMP401_TEMP_LOW_LIMIT_LSB[index], reg & 0xFF);
+
+	data->temp_low[index] = reg;
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static ssize_t store_temp_max(struct device *dev, struct device_attribute
+	*devattr, const char *buf, size_t count)
+{
+	int index = to_sensor_dev_attr(devattr)->index;
+	struct tmp401_data *data = tmp401_update_device(dev);
+	long v;
+	u16 reg;
+
+	if (strict_strtol(buf, 10, &v))
+		return -EINVAL;
+
+	reg = tmp401_temp_to_register(v, data->config);
+
+	mutex_lock(&data->update_lock);
+
+	i2c_smbus_write_byte_data(&data->client,
+		TMP401_TEMP_HIGH_LIMIT_MSB_WRITE[index], reg >> 8);
+	i2c_smbus_write_byte_data(&data->client,
+		TMP401_TEMP_HIGH_LIMIT_LSB[index], reg & 0xFF);
+
+	data->temp_high[index] = reg;
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static ssize_t store_temp_crit(struct device *dev, struct device_attribute
+	*devattr, const char *buf, size_t count)
+{
+	int index = to_sensor_dev_attr(devattr)->index;
+	struct tmp401_data *data = tmp401_update_device(dev);
+	long v;
+	u8 reg;
+
+	if (strict_strtol(buf, 10, &v))
+		return -EINVAL;
+
+	reg = tmp401_temp_crit_to_register(v, data->config);
+
+	mutex_lock(&data->update_lock);
+
+	i2c_smbus_write_byte_data(&data->client,
+		TMP401_TEMP_CRIT_LIMIT[index], reg);
+
+	data->temp_crit[index] = reg;
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static ssize_t store_temp_crit_hyst(struct device *dev, struct device_attribute
+	*devattr, const char *buf, size_t count)
+{
+	int temp, index = to_sensor_dev_attr(devattr)->index;
+	struct tmp401_data *data = tmp401_update_device(dev);
+	long v;
+	u8 reg;
+
+	if (strict_strtol(buf, 10, &v))
+		return -EINVAL;
+
+	if (data->config & TMP401_CONFIG_RANGE_MASK)
+		v = SENSORS_LIMIT(v, -64000, 191000);
+	else
+		v = SENSORS_LIMIT(v, 0, 127000);
+
+	mutex_lock(&data->update_lock);
+	temp = tmp401_crit_register_to_temp(data->temp_crit[index],
+						data->config);
+	v = SENSORS_LIMIT(v, temp - 255000, temp);
+	reg = (temp - v) / 1000;
+
+	i2c_smbus_write_byte_data(&data->client,
+		TMP401_TEMP_CRIT_HYST, reg);
+
+	data->temp_crit_hyst = reg;
+
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
+static struct sensor_device_attribute tmp401_attr[] = {
+	SENSOR_ATTR(temp1_input, 0444, show_temp_value, NULL, 0),
+	SENSOR_ATTR(temp1_min, 0644, show_temp_min, store_temp_min, 0),
+	SENSOR_ATTR(temp1_max, 0644, show_temp_max, store_temp_max, 0),
+	SENSOR_ATTR(temp1_crit, 0644, show_temp_crit, store_temp_crit, 0),
+	SENSOR_ATTR(temp1_crit_hyst, 0644, show_temp_crit_hyst,
+			store_temp_crit_hyst, 0),
+	SENSOR_ATTR(temp1_min_alarm, 0444, show_status, NULL,
+			TMP401_STATUS_LOCAL_LOW_MASK),
+	SENSOR_ATTR(temp1_max_alarm, 0444, show_status, NULL,
+			TMP401_STATUS_LOCAL_HIGH_MASK),
+	SENSOR_ATTR(temp1_crit_alarm, 0444, show_status, NULL,
+			TMP401_STATUS_LOCAL_CRIT_MASK),
+	SENSOR_ATTR(temp2_input, 0444, show_temp_value, NULL, 1),
+	SENSOR_ATTR(temp2_min, 0644, show_temp_min, store_temp_min, 1),
+	SENSOR_ATTR(temp2_max, 0644, show_temp_max, store_temp_max, 1),
+	SENSOR_ATTR(temp2_crit, 0644, show_temp_crit, store_temp_crit, 1),
+	SENSOR_ATTR(temp2_crit_hyst, 0444, show_temp_crit_hyst, NULL, 1),
+	SENSOR_ATTR(temp2_fault, 0444, show_status, NULL,
+			TMP401_STATUS_REMOTE_OPEN_MASK),
+	SENSOR_ATTR(temp2_min_alarm, 0444, show_status, NULL,
+			TMP401_STATUS_REMOTE_LOW_MASK),
+	SENSOR_ATTR(temp2_max_alarm, 0444, show_status, NULL,
+			TMP401_STATUS_REMOTE_HIGH_MASK),
+	SENSOR_ATTR(temp2_crit_alarm, 0444, show_status, NULL,
+			TMP401_STATUS_REMOTE_CRIT_MASK),
+};
+
+/*
+ * Real code
+ */
+
+static int tmp401_detect(struct i2c_adapter *adapter, int address, int kind)
+{
+	int i, err = 0;
+	struct i2c_client *client;
+	struct tmp401_data *data;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return 0;
+
+	/* 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 i2c_smbus_read_byte_data. */
+	if (!(data = kzalloc(sizeof(struct tmp401_data), GFP_KERNEL)))
+		return -ENOMEM;
+
+	client = &data->client;
+	i2c_set_clientdata(client, data);
+	client->addr = address;
+	client->adapter = adapter;
+	client->driver = &tmp401_driver;
+	mutex_init(&data->update_lock);
+
+	/* Detect & Identify the chip */
+	if (kind <= 0) {
+		u8 manufacturer_id;
+		u8 device_id;
+
+		manufacturer_id = i2c_smbus_read_byte_data(client,
+						TMP401_MANUFACTURER_ID_REG);
+		device_id = i2c_smbus_read_byte_data(client,
+						TMP401_DEVICE_ID_REG);
+
+		if (manufacturer_id != TMP401_MANUFACTURER_ID ||
+				device_id != TMP401_DEVICE_ID)
+			goto exit_free;
+	}
+
+	/* Tell the I2C layer a new client has arrived */
+	if ((err = i2c_attach_client(client)))
+		goto exit_free;
+
+	/* Initialize the TMP401 chip */
+	tmp401_init_client(client);
+
+	/* Register sysfs hooks */
+	for (i = 0; i < ARRAY_SIZE(tmp401_attr); i++) {
+		err = device_create_file(&client->dev,
+					&tmp401_attr[i].dev_attr);
+		if (err)
+			goto exit_detach;
+	}
+
+	data->hwmon_dev = hwmon_device_register(&client->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		err = PTR_ERR(data->hwmon_dev);
+		data->hwmon_dev = NULL;
+		goto exit_detach;
+	}
+
+	printk(KERN_INFO TMP401_NAME ": Detected TI TMP401 chip\n");
+
+	return 0;
+
+exit_detach:
+	tmp401_detach_client(client); /* will also free data for us */
+	return err;
+
+exit_free:
+	kfree(data);
+	return err;
+}
+
+static void tmp401_init_client(struct i2c_client *client)
+{
+	int config, config_orig;
+
+	/* Set the conversion rate to 2 Hz */
+	i2c_smbus_write_byte_data(client, TMP401_CONVERSION_RATE_WRITE, 5);
+
+	/* Start conversions (disable shutdown if necessary) */
+	config = i2c_smbus_read_byte_data(client, TMP401_CONFIG_READ);
+	if (config < 0) {
+		dev_warn(&client->dev, "Initialization failed!\n");
+		return;
+	}
+
+	config_orig = config;
+	config &= ~TMP401_CONFIG_SHUTDOWN_MASK;
+
+	if (config != config_orig)
+		i2c_smbus_write_byte_data(client, TMP401_CONFIG_WRITE, config);
+}
+
+static int tmp401_attach_adapter(struct i2c_adapter *adapter)
+{
+	if (!(adapter->class & I2C_CLASS_HWMON))
+		return 0;
+	return i2c_probe(adapter, &addr_data, tmp401_detect);
+}
+
+static int tmp401_detach_client(struct i2c_client *client)
+{
+	struct tmp401_data *data = i2c_get_clientdata(client);
+	int i, err;
+
+	/* Check if registered in case we're called from tmp401_detect
+	   to cleanup after an error */
+	if (data->hwmon_dev)
+		hwmon_device_unregister(data->hwmon_dev);
+
+	for (i = 0; i < ARRAY_SIZE(tmp401_attr); i++)
+		device_remove_file(&client->dev, &tmp401_attr[i].dev_attr);
+
+	if ((err = i2c_detach_client(client)))
+		return err;
+
+	kfree(data);
+	return 0;
+}
+
+static struct tmp401_data *tmp401_update_device(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct tmp401_data *data = i2c_get_clientdata(client);
+	int i;
+
+	mutex_lock(&data->update_lock);
+
+	if (time_after(jiffies, data->last_updated + 2 * HZ) || !data->valid) {
+		data->status = i2c_smbus_read_byte_data(client, TMP401_STATUS);
+		data->config = i2c_smbus_read_byte_data(client,
+						TMP401_CONFIG_READ);
+		for (i = 0; i < 2; i++) {
+			data->temp[i] = i2c_smbus_read_byte_data(client,
+						TMP401_TEMP_MSB[i]) << 8;
+			data->temp[i] |= i2c_smbus_read_byte_data(client,
+						TMP401_TEMP_LSB[i]);
+			data->temp_low[i] = i2c_smbus_read_byte_data(client,
+				TMP401_TEMP_LOW_LIMIT_MSB_READ[i]) << 8;
+			data->temp_low[i] |= i2c_smbus_read_byte_data(client,
+						TMP401_TEMP_LOW_LIMIT_LSB[i]);
+			data->temp_high[i] = i2c_smbus_read_byte_data(client,
+				TMP401_TEMP_HIGH_LIMIT_MSB_READ[i]) << 8;
+			data->temp_high[i] |= i2c_smbus_read_byte_data(client,
+						TMP401_TEMP_HIGH_LIMIT_LSB[i]);
+			data->temp_crit[i] = i2c_smbus_read_byte_data(client,
+						TMP401_TEMP_CRIT_LIMIT[i]);
+		}
+
+		data->temp_crit_hyst = i2c_smbus_read_byte_data(client,
+						TMP401_TEMP_CRIT_HYST);
+
+		data->last_updated = jiffies;
+		data->valid = 1;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return data;
+}
+
+static int __init tmp401_init(void)
+{
+	return i2c_add_driver(&tmp401_driver);
+}
+
+static void __exit tmp401_exit(void)
+{
+	i2c_del_driver(&tmp401_driver);
+}
+
+MODULE_AUTHOR("Hans de Goede <j.w.r.degoede@hhs.nl>");
+MODULE_DESCRIPTION("Texas Instruments TMP401 temperature sensor driver");
+MODULE_LICENSE("GPL");
+
+module_init(tmp401_init);
+module_exit(tmp401_exit);

[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

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

Re: [lm-sensors] PATCH: hwmon-new-driver-ti-tmp401.patch

[Ben Dooks]

On Sat, Jun 14, 2008 at 05:15:13PM +0200, Hans de Goede wrote:
> Hi All,
> 
> This is a new hwmon driver for TI's TMP401 temperature sensor IC. This 
> driver
> was written on behalf of an embedded systems vendor under the
> linuxdriverproject.
> 
> It has been tested using a TI TMP401 sample attached to a i2c-tiny-usb 
> adapter.
> Which was provided by Till Harbaum, many thanks to him for this!
> 
> Signed-off-by: Hans de Goede <j.w.r.degoede@hhs.nl>
> 
> Regards,
> 
> Hans

> This is a new hwmon driver for TI's TMP401 temperature sensor IC. This driver
> was written on behalf of an embedded systems vendor under the
> linuxdriverproject.
> 
> It has been tested using a TI TMP401 sample attached to a i2c-tiny-usb adapter.
> Which was provided by Till Harbaum, many thanks to him for this! 
> 
> Signed-off-by: Hans de Goede <j.w.r.degoede@hhs.nl>
> diff -up vanilla-2.6.26-rc5-git2/drivers/hwmon/Kconfig~ vanilla-2.6.26-rc5-git2/drivers/hwmon/Kconfig
> --- vanilla-2.6.26-rc5-git2/drivers/hwmon/Kconfig~	2008-06-14 16:51:14.000000000 +0200
> +++ vanilla-2.6.26-rc5-git2/drivers/hwmon/Kconfig	2008-06-14 16:51:14.000000000 +0200
> @@ -633,6 +633,16 @@ config SENSORS_THMC50
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called thmc50.
>  
> +config SENSORS_TMP401
> +	tristate "Texas Instruments TMP401"
> +	depends on I2C && EXPERIMENTAL
> +	help
> +	  If you say yes here you get support for Texas Instruments TMP401
> +	  temperature sensor chips.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called tmp401.
> +
>  config SENSORS_VIA686A
>  	tristate "VIA686A"
>  	depends on PCI
> diff -up vanilla-2.6.26-rc5-git2/drivers/hwmon/Makefile~ vanilla-2.6.26-rc5-git2/drivers/hwmon/Makefile
> --- vanilla-2.6.26-rc5-git2/drivers/hwmon/Makefile~	2008-06-14 16:52:19.000000000 +0200
> +++ vanilla-2.6.26-rc5-git2/drivers/hwmon/Makefile	2008-06-14 16:52:19.000000000 +0200
> @@ -66,6 +66,7 @@ obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc4
>  obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
>  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>  obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
> +obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
>  obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
>  obj-$(CONFIG_SENSORS_VT1211)	+= vt1211.o
>  obj-$(CONFIG_SENSORS_VT8231)	+= vt8231.o
> diff -up vanilla-2.6.26-rc5-git2/drivers/hwmon/tmp401.c~ vanilla-2.6.26-rc5-git2/drivers/hwmon/tmp401.c
> --- vanilla-2.6.26-rc5-git2/drivers/hwmon/tmp401.c~	2008-06-14 17:04:10.000000000 +0200
> +++ vanilla-2.6.26-rc5-git2/drivers/hwmon/tmp401.c	2008-06-14 17:10:13.000000000 +0200
> @@ -0,0 +1,566 @@
> +/* tmp401.c
> + *
> + * Copyright (C) 2007 Hans de Goede <j.w.r.degoede@hhs.nl>
> + *
> + * 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.
> + */
> +
> +/*
> + * Driver for the Texas Instruments TMP401 SMBUS temperature sensor IC.
> + *
> + * Note this IC is in some aspect similar to the lm90, but it has quite a
> + * few differences too, for example the local temp has a higher resolution
> + * and thus has 16 bits registers for its value and limit instead of 8 bits.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/sysfs.h>
> +
> +/* Addresses to scan */
> +static unsigned short normal_i2c[] = { 0x4c, I2C_CLIENT_END };
> +
> +/* Insmod parameters */
> +I2C_CLIENT_INSMOD_1(tmp401);
> +
> +
> +/*
> + * The TMP401 registers, note some registers have different addresses for
> + * reading and writing
> + */
> +#define TMP401_STATUS				0x02
> +#define TMP401_CONFIG_READ			0x03
> +#define TMP401_CONFIG_WRITE			0x09
> +#define TMP401_CONVERSION_RATE_READ		0x04
> +#define TMP401_CONVERSION_RATE_WRITE		0x0A
> +#define TMP401_ONE_SHOT_START_WRITE		0x0F
> +#define TMP401_RESOLUTION			0x1A
> +#define TMP401_TEMP_CRIT_HYST			0x21
> +#define TMP401_CONSECUTIVE_ALERT		0x22
> +#define TMP401_MANUFACTURER_ID_REG		0xFE
> +#define TMP401_DEVICE_ID_REG			0xFF
> +
> +static const u8 TMP401_TEMP_MSB[2]			= { 0x00, 0x01 };
> +static const u8 TMP401_TEMP_LSB[2]			= { 0x15, 0x10 };
> +static const u8 TMP401_TEMP_LOW_LIMIT_MSB_READ[2]	= { 0x06, 0x08 };
> +static const u8 TMP401_TEMP_LOW_LIMIT_MSB_WRITE[2]	= { 0x0C, 0x0E };
> +static const u8 TMP401_TEMP_LOW_LIMIT_LSB[2]		= { 0x17, 0x14 };
> +static const u8 TMP401_TEMP_HIGH_LIMIT_MSB_READ[2]	= { 0x05, 0x07 };
> +static const u8 TMP401_TEMP_HIGH_LIMIT_MSB_WRITE[2]	= { 0x0B, 0x0D };
> +static const u8 TMP401_TEMP_HIGH_LIMIT_LSB[2]		= { 0x16, 0x13 };
> +/* These are called the THERM limit / hysteresis / mask in the datasheets */
> +static const u8 TMP401_TEMP_CRIT_LIMIT[2]		= { 0x20, 0x19 };
> +
> +/* Bit masks */
> +#define TMP401_CONFIG_RANGE_MASK		0x04
> +#define TMP401_CONFIG_SHUTDOWN_MASK		0x40
> +#define TMP401_STATUS_LOCAL_CRIT_MASK		0x01
> +#define TMP401_STATUS_REMOTE_CRIT_MASK		0x02
> +#define TMP401_STATUS_REMOTE_OPEN_MASK		0x04
> +#define TMP401_STATUS_REMOTE_LOW_MASK		0x08
> +#define TMP401_STATUS_REMOTE_HIGH_MASK		0x10
> +#define TMP401_STATUS_LOCAL_LOW_MASK		0x20
> +#define TMP401_STATUS_LOCAL_HIGH_MASK		0x40
> +
> +/* Manufacturer / Device ID's */
> +#define TMP401_MANUFACTURER_ID			0x55
> +#define TMP401_DEVICE_ID			0x11
> +
> +/* our driver name */
> +#define TMP401_NAME "tmp401"
> +
> +/*
> + * Functions declarations
> + */
> +
> +static void tmp401_init_client(struct i2c_client *client);
> +static int tmp401_attach_adapter(struct i2c_adapter *adapter);
> +static int tmp401_detach_client(struct i2c_client *client);
> +static struct tmp401_data *tmp401_update_device(struct device *dev);
> +
> +/*
> + * Driver data (common to all clients)
> + */
> +
> +static struct i2c_driver tmp401_driver = {
> +	.driver = {
> +		.name	= TMP401_NAME,

the .owner field is missing, ie:
		.owner	= THIS_MODULE,

> +	},
> +	.attach_adapter	= tmp401_attach_adapter,
> +	.detach_client	= tmp401_detach_client,
> +};

you should be using the new method of attaching the clients, as the
old driver methods are being removed as soon as possible. 

[snip]


> +
> +	if (data->status & mask)
> +		return sprintf(buf, "1\n");
> +	else
> +		return sprintf(buf, "0\n");
> +}

you should be using snprintf() or some other bounded printf method
against PAGE_SIZE, the return should be the length of the buffer.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.


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

Re: [lm-sensors] PATCH: hwmon-new-driver-ti-tmp401.patch

[Jean Delvare]

Hi Ben,

On Sun, 15 Jun 2008 19:23:11 +0100, Ben Dooks wrote:
> On Sat, Jun 14, 2008 at 05:15:13PM +0200, Hans de Goede wrote:
> > Hi All,
> > 
> > This is a new hwmon driver for TI's TMP401 temperature sensor IC. This 
> > driver
> > was written on behalf of an embedded systems vendor under the
> > linuxdriverproject.
> > 
> > It has been tested using a TI TMP401 sample attached to a i2c-tiny-usb 
> > adapter.
> > Which was provided by Till Harbaum, many thanks to him for this!
> > 
> > Signed-off-by: Hans de Goede <j.w.r.degoede@hhs.nl>
> > 
> > Regards,
> > 
> > Hans
> 
> > This is a new hwmon driver for TI's TMP401 temperature sensor IC. This driver
> > was written on behalf of an embedded systems vendor under the
> > linuxdriverproject.
> > 
> > It has been tested using a TI TMP401 sample attached to a i2c-tiny-usb adapter.
> > Which was provided by Till Harbaum, many thanks to him for this! 
> > 
> > Signed-off-by: Hans de Goede <j.w.r.degoede@hhs.nl>
> > (...)
> > +static struct i2c_driver tmp401_driver = {
> > +	.driver = {
> > +		.name	= TMP401_NAME,
> 
> the .owner field is missing, ie:
> 		.owner	= THIS_MODULE,

It's optional (i2c_add_driver adds it anyway). I don't like it and I
might revert this behavior someday, but for now that's how things are,
so Hans' code is correct.

> > +	},
> > +	.attach_adapter	= tmp401_attach_adapter,
> > +	.detach_client	= tmp401_detach_client,
> > +};
> 
> you should be using the new method of attaching the clients, as the
> old driver methods are being removed as soon as possible. 

If and only if the only users of this driver are embedded platforms. If
this chip is found on PCs or other systems where you can't instantiate
the I2C device, then the legacy methods must still be used. If both
types of users exist then you need two i2c_driver instances (for now;
I'm working on something better for 2.6.27.)

> > +
> > +	if (data->status & mask)
> > +		return sprintf(buf, "1\n");
> > +	else
> > +		return sprintf(buf, "0\n");
> > +}
> 
> you should be using snprintf() or some other bounded printf method
> against PAGE_SIZE, the return should be the length of the buffer.

I doubt it. snprintf returns the number of bytes written (just like
sprintf), not the size of the buffer, and I fail to see why you would
want to return the buffer size. That would cause sysfs to pass
uninitialized data to user-space.

I agree that in theory we should be using snprintf in these functions,
but the fact is that no hwmon driver does this, and frankly this sounds
overkill. There's just no way PAGE_SIZE can be smaller than room it
takes to print an integer.

-- 
Jean Delvare

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

Re: [lm-sensors] PATCH: hwmon-new-driver-ti-tmp401.patch

[Jean Delvare]

Hi Hans,

On Sat, 14 Jun 2008 17:15:13 +0200, Hans de Goede wrote:
> This is a new hwmon driver for TI's TMP401 temperature sensor IC. This driver
> was written on behalf of an embedded systems vendor under the
> linuxdriverproject.
> 
> It has been tested using a TI TMP401 sample attached to a i2c-tiny-usb adapter.
> Which was provided by Till Harbaum, many thanks to him for this!
> 
> Signed-off-by: Hans de Goede <j.w.r.degoede@hhs.nl>

Review:

> diff -up vanilla-2.6.26-rc5-git2/drivers/hwmon/Kconfig~ vanilla-2.6.26-rc5-git2/drivers/hwmon/Kconfig
> --- vanilla-2.6.26-rc5-git2/drivers/hwmon/Kconfig~	2008-06-14 16:51:14.000000000 +0200
> +++ vanilla-2.6.26-rc5-git2/drivers/hwmon/Kconfig	2008-06-14 16:51:14.000000000 +0200
> @@ -633,6 +633,16 @@ config SENSORS_THMC50
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called thmc50.
>  
> +config SENSORS_TMP401
> +	tristate "Texas Instruments TMP401"
> +	depends on I2C && EXPERIMENTAL
> +	help
> +	  If you say yes here you get support for Texas Instruments TMP401
> +	  temperature sensor chips.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called tmp401.
> +
>  config SENSORS_VIA686A
>  	tristate "VIA686A"
>  	depends on PCI
> diff -up vanilla-2.6.26-rc5-git2/drivers/hwmon/Makefile~ vanilla-2.6.26-rc5-git2/drivers/hwmon/Makefile
> --- vanilla-2.6.26-rc5-git2/drivers/hwmon/Makefile~	2008-06-14 16:52:19.000000000 +0200
> +++ vanilla-2.6.26-rc5-git2/drivers/hwmon/Makefile	2008-06-14 16:52:19.000000000 +0200
> @@ -66,6 +66,7 @@ obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc4
>  obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
>  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>  obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
> +obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
>  obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
>  obj-$(CONFIG_SENSORS_VT1211)	+= vt1211.o
>  obj-$(CONFIG_SENSORS_VT8231)	+= vt8231.o
> diff -up vanilla-2.6.26-rc5-git2/drivers/hwmon/tmp401.c~ vanilla-2.6.26-rc5-git2/drivers/hwmon/tmp401.c
> --- vanilla-2.6.26-rc5-git2/drivers/hwmon/tmp401.c~	2008-06-14 17:04:10.000000000 +0200
> +++ vanilla-2.6.26-rc5-git2/drivers/hwmon/tmp401.c	2008-06-14 17:10:13.000000000 +0200
> @@ -0,0 +1,566 @@
> +/* tmp401.c
> + *
> + * Copyright (C) 2007 Hans de Goede <j.w.r.degoede@hhs.nl>

You could add 2008 now.

> + *
> + * 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.
> + */
> +
> +/*
> + * Driver for the Texas Instruments TMP401 SMBUS temperature sensor IC.
> + *
> + * Note this IC is in some aspect similar to the lm90, but it has quite a

LM90

> + * few differences too, for example the local temp has a higher resolution
> + * and thus has 16 bits registers for its value and limit instead of 8 bits.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/sysfs.h>
> +
> +/* Addresses to scan */
> +static unsigned short normal_i2c[] = { 0x4c, I2C_CLIENT_END };

Could be made const.

> +
> +/* Insmod parameters */
> +I2C_CLIENT_INSMOD_1(tmp401);
> +
> +
> +/*
> + * The TMP401 registers, note some registers have different addresses for
> + * reading and writing
> + */
> +#define TMP401_STATUS				0x02
> +#define TMP401_CONFIG_READ			0x03
> +#define TMP401_CONFIG_WRITE			0x09
> +#define TMP401_CONVERSION_RATE_READ		0x04
> +#define TMP401_CONVERSION_RATE_WRITE		0x0A
> +#define TMP401_ONE_SHOT_START_WRITE		0x0F

This one is never used.

> +#define TMP401_RESOLUTION			0x1A

Same for this one.

> +#define TMP401_TEMP_CRIT_HYST			0x21
> +#define TMP401_CONSECUTIVE_ALERT		0x22
> +#define TMP401_MANUFACTURER_ID_REG		0xFE
> +#define TMP401_DEVICE_ID_REG			0xFF
> +
> +static const u8 TMP401_TEMP_MSB[2]			= { 0x00, 0x01 };
> +static const u8 TMP401_TEMP_LSB[2]			= { 0x15, 0x10 };
> +static const u8 TMP401_TEMP_LOW_LIMIT_MSB_READ[2]	= { 0x06, 0x08 };
> +static const u8 TMP401_TEMP_LOW_LIMIT_MSB_WRITE[2]	= { 0x0C, 0x0E };
> +static const u8 TMP401_TEMP_LOW_LIMIT_LSB[2]		= { 0x17, 0x14 };
> +static const u8 TMP401_TEMP_HIGH_LIMIT_MSB_READ[2]	= { 0x05, 0x07 };
> +static const u8 TMP401_TEMP_HIGH_LIMIT_MSB_WRITE[2]	= { 0x0B, 0x0D };
> +static const u8 TMP401_TEMP_HIGH_LIMIT_LSB[2]		= { 0x16, 0x13 };
> +/* These are called the THERM limit / hysteresis / mask in the datasheets */
> +static const u8 TMP401_TEMP_CRIT_LIMIT[2]		= { 0x20, 0x19 };

datasheet

> +
> +/* Bit masks */
> +#define TMP401_CONFIG_RANGE_MASK		0x04
> +#define TMP401_CONFIG_SHUTDOWN_MASK		0x40
> +#define TMP401_STATUS_LOCAL_CRIT_MASK		0x01
> +#define TMP401_STATUS_REMOTE_CRIT_MASK		0x02
> +#define TMP401_STATUS_REMOTE_OPEN_MASK		0x04
> +#define TMP401_STATUS_REMOTE_LOW_MASK		0x08
> +#define TMP401_STATUS_REMOTE_HIGH_MASK		0x10
> +#define TMP401_STATUS_LOCAL_LOW_MASK		0x20
> +#define TMP401_STATUS_LOCAL_HIGH_MASK		0x40

These are flags, not masks.

> +
> +/* Manufacturer / Device ID's */
> +#define TMP401_MANUFACTURER_ID			0x55
> +#define TMP401_DEVICE_ID			0x11
> +
> +/* our driver name */
> +#define TMP401_NAME "tmp401"
> +
> +/*
> + * Functions declarations
> + */
> +
> +static void tmp401_init_client(struct i2c_client *client);

If you move the code of this function, you can easily get away with this
forward declaration.

> +static int tmp401_attach_adapter(struct i2c_adapter *adapter);
> +static int tmp401_detach_client(struct i2c_client *client);
> +static struct tmp401_data *tmp401_update_device(struct device *dev);
> +
> +/*
> + * Driver data (common to all clients)
> + */
> +
> +static struct i2c_driver tmp401_driver = {
> +	.driver = {
> +		.name	= TMP401_NAME,
> +	},
> +	.attach_adapter	= tmp401_attach_adapter,
> +	.detach_client	= tmp401_detach_client,
> +};

This needs to be converted to the new i2c device driver binding. I can
take care of it if you do not have the time, just let me know.

> +
> +/*
> + * Client data (each client gets its own)
> + */
> +
> +struct tmp401_data {
> +	struct i2c_client client;
> +	struct device *hwmon_dev;
> +	struct mutex update_lock;
> +	char valid; /* zero until following fields are valid */
> +	unsigned long last_updated; /* in jiffies */
> +
> +	/* register values */
> +	u8 status;
> +	u8 config;
> +	u16 temp[2];
> +	u16 temp_low[2];
> +	u16 temp_high[2];
> +	u8 temp_crit[2];
> +	u8 temp_crit_hyst;
> +};
> +
> +/*
> + * Sysfs attr show / store functions
> + */
> +
> +static int tmp401_register_to_temp(u16 reg, u8 config)
> +{
> +	int temp = reg;
> +
> +	if (config & TMP401_CONFIG_RANGE_MASK)
> +		temp -= 64 * 256;
> +
> +	return (temp * 625) / 160;
> +}
> +
> +static u16 tmp401_temp_to_register(long temp, u8 config)
> +{
> +	if (config & TMP401_CONFIG_RANGE_MASK) {
> +		temp = SENSORS_LIMIT(temp, -64000, 191000);
> +		temp += 64000;
> +	} else
> +		temp = SENSORS_LIMIT(temp, 0, 127000);
> +
> +	return (temp * 160) / 625;

You can get rounding with:

	return (temp * 160 + 625 / 2) / 625;

Even though it probably doesn't matter too much for such high resolution
sensors.

> +}
> +
> +static int tmp401_crit_register_to_temp(u8 reg, u8 config)
> +{
> +	int temp = reg;
> +
> +	if (config & TMP401_CONFIG_RANGE_MASK)
> +		temp -= 64;
> +
> +	return temp * 1000;
> +}
> +
> +static u8 tmp401_temp_crit_to_register(long temp, u8 config)

This function name isn't consistent with the previous one. It should be
either tmp401_crit_temp_to_register or tmp401_temp_to_crit_register.

> +{
> +	if (config & TMP401_CONFIG_RANGE_MASK) {
> +		temp = SENSORS_LIMIT(temp, -64000, 191000);
> +		temp += 64000;
> +	} else
> +		temp = SENSORS_LIMIT(temp, 0, 127000);
> +
> +	return temp / 1000;

Here rounding is important:

	return (temp + 500) / 1000;

> +}
> +
> +static ssize_t show_temp_value(struct device *dev,
> +	struct device_attribute *devattr, char *buf)
> +{
> +	int index = to_sensor_dev_attr(devattr)->index;
> +	struct tmp401_data *data = tmp401_update_device(dev);
> +
> +	return sprintf(buf, "%d\n",
> +		tmp401_register_to_temp(data->temp[index], data->config));
> +}
> +
> +static ssize_t show_temp_min(struct device *dev,
> +	struct device_attribute *devattr, char *buf)
> +{
> +	int index = to_sensor_dev_attr(devattr)->index;
> +	struct tmp401_data *data = tmp401_update_device(dev);
> +
> +	return sprintf(buf, "%d\n",
> +		tmp401_register_to_temp(data->temp_low[index], data->config));
> +}
> +
> +static ssize_t show_temp_max(struct device *dev,
> +	struct device_attribute *devattr, char *buf)
> +{
> +	int index = to_sensor_dev_attr(devattr)->index;
> +	struct tmp401_data *data = tmp401_update_device(dev);
> +
> +	return sprintf(buf, "%d\n",
> +		tmp401_register_to_temp(data->temp_high[index], data->config));
> +}
> +
> +static ssize_t show_temp_crit(struct device *dev,
> +	struct device_attribute *devattr, char *buf)
> +{
> +	int index = to_sensor_dev_attr(devattr)->index;
> +	struct tmp401_data *data = tmp401_update_device(dev);
> +
> +	return sprintf(buf, "%d\n",
> +			tmp401_crit_register_to_temp(data->temp_crit[index],
> +							data->config));
> +}
> +
> +static ssize_t show_temp_crit_hyst(struct device *dev,
> +	struct device_attribute *devattr, char *buf)
> +{
> +	int temp, index = to_sensor_dev_attr(devattr)->index;
> +	struct tmp401_data *data = tmp401_update_device(dev);
> +
> +	mutex_lock(&data->update_lock);
> +	temp = tmp401_crit_register_to_temp(data->temp_crit[index],
> +						data->config);
> +	temp -= data->temp_crit_hyst * 1000;
> +	mutex_unlock(&data->update_lock);
> +
> +	return sprintf(buf, "%d\n", temp);
> +}
> +
> +static ssize_t show_status(struct device *dev,
> +	struct device_attribute *devattr, char *buf)
> +{
> +	int mask = to_sensor_dev_attr(devattr)->index;
> +	struct tmp401_data *data = tmp401_update_device(dev);
> +
> +	if (data->status & mask)
> +		return sprintf(buf, "1\n");
> +	else
> +		return sprintf(buf, "0\n");
> +}
> +
> +static ssize_t store_temp_min(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count)
> +{
> +	int index = to_sensor_dev_attr(devattr)->index;
> +	struct tmp401_data *data = tmp401_update_device(dev);
> +	long v;

Please avoid 1-letter variable names (except for loop indexes.)

> +	u16 reg;
> +
> +	if (strict_strtol(buf, 10, &v))
> +		return -EINVAL;
> +
> +	reg = tmp401_temp_to_register(v, data->config);
> +
> +	mutex_lock(&data->update_lock);
> +
> +	i2c_smbus_write_byte_data(&data->client,
> +		TMP401_TEMP_LOW_LIMIT_MSB_WRITE[index], reg >> 8);
> +	i2c_smbus_write_byte_data(&data->client,
> +		TMP401_TEMP_LOW_LIMIT_LSB[index], reg & 0xFF);
> +
> +	data->temp_low[index] = reg;
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +static ssize_t store_temp_max(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count)
> +{
> +	int index = to_sensor_dev_attr(devattr)->index;
> +	struct tmp401_data *data = tmp401_update_device(dev);
> +	long v;
> +	u16 reg;
> +
> +	if (strict_strtol(buf, 10, &v))
> +		return -EINVAL;
> +
> +	reg = tmp401_temp_to_register(v, data->config);
> +
> +	mutex_lock(&data->update_lock);
> +
> +	i2c_smbus_write_byte_data(&data->client,
> +		TMP401_TEMP_HIGH_LIMIT_MSB_WRITE[index], reg >> 8);
> +	i2c_smbus_write_byte_data(&data->client,
> +		TMP401_TEMP_HIGH_LIMIT_LSB[index], reg & 0xFF);
> +
> +	data->temp_high[index] = reg;
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +static ssize_t store_temp_crit(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count)
> +{
> +	int index = to_sensor_dev_attr(devattr)->index;
> +	struct tmp401_data *data = tmp401_update_device(dev);
> +	long v;
> +	u8 reg;
> +
> +	if (strict_strtol(buf, 10, &v))
> +		return -EINVAL;
> +
> +	reg = tmp401_temp_crit_to_register(v, data->config);
> +
> +	mutex_lock(&data->update_lock);
> +
> +	i2c_smbus_write_byte_data(&data->client,
> +		TMP401_TEMP_CRIT_LIMIT[index], reg);
> +
> +	data->temp_crit[index] = reg;
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +static ssize_t store_temp_crit_hyst(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count)
> +{
> +	int temp, index = to_sensor_dev_attr(devattr)->index;
> +	struct tmp401_data *data = tmp401_update_device(dev);
> +	long v;
> +	u8 reg;
> +
> +	if (strict_strtol(buf, 10, &v))
> +		return -EINVAL;
> +
> +	if (data->config & TMP401_CONFIG_RANGE_MASK)
> +		v = SENSORS_LIMIT(v, -64000, 191000);
> +	else
> +		v = SENSORS_LIMIT(v, 0, 127000);
> +
> +	mutex_lock(&data->update_lock);
> +	temp = tmp401_crit_register_to_temp(data->temp_crit[index],
> +						data->config);
> +	v = SENSORS_LIMIT(v, temp - 255000, temp);
> +	reg = (temp - v) / 1000;

This lacks rounding.

> +
> +	i2c_smbus_write_byte_data(&data->client,
> +		TMP401_TEMP_CRIT_HYST, reg);
> +
> +	data->temp_crit_hyst = reg;
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +static struct sensor_device_attribute tmp401_attr[] = {
> +	SENSOR_ATTR(temp1_input, 0444, show_temp_value, NULL, 0),
> +	SENSOR_ATTR(temp1_min, 0644, show_temp_min, store_temp_min, 0),
> +	SENSOR_ATTR(temp1_max, 0644, show_temp_max, store_temp_max, 0),
> +	SENSOR_ATTR(temp1_crit, 0644, show_temp_crit, store_temp_crit, 0),
> +	SENSOR_ATTR(temp1_crit_hyst, 0644, show_temp_crit_hyst,
> +			store_temp_crit_hyst, 0),
> +	SENSOR_ATTR(temp1_min_alarm, 0444, show_status, NULL,
> +			TMP401_STATUS_LOCAL_LOW_MASK),
> +	SENSOR_ATTR(temp1_max_alarm, 0444, show_status, NULL,
> +			TMP401_STATUS_LOCAL_HIGH_MASK),
> +	SENSOR_ATTR(temp1_crit_alarm, 0444, show_status, NULL,
> +			TMP401_STATUS_LOCAL_CRIT_MASK),
> +	SENSOR_ATTR(temp2_input, 0444, show_temp_value, NULL, 1),
> +	SENSOR_ATTR(temp2_min, 0644, show_temp_min, store_temp_min, 1),
> +	SENSOR_ATTR(temp2_max, 0644, show_temp_max, store_temp_max, 1),
> +	SENSOR_ATTR(temp2_crit, 0644, show_temp_crit, store_temp_crit, 1),
> +	SENSOR_ATTR(temp2_crit_hyst, 0444, show_temp_crit_hyst, NULL, 1),
> +	SENSOR_ATTR(temp2_fault, 0444, show_status, NULL,
> +			TMP401_STATUS_REMOTE_OPEN_MASK),
> +	SENSOR_ATTR(temp2_min_alarm, 0444, show_status, NULL,
> +			TMP401_STATUS_REMOTE_LOW_MASK),
> +	SENSOR_ATTR(temp2_max_alarm, 0444, show_status, NULL,
> +			TMP401_STATUS_REMOTE_HIGH_MASK),
> +	SENSOR_ATTR(temp2_crit_alarm, 0444, show_status, NULL,
> +			TMP401_STATUS_REMOTE_CRIT_MASK),
> +};

At page 11, in section "STATUS REGISTER", the datasheet suggests that
the hysteresis value also applies to the local and remote high limits.
Did you test that? If this is true then you should add files
temp1_max_hyst and temp2_max_hyst (read-only.)

> +
> +/*
> + * Real code

This old comment of mine is really bad... As if the rest of the code
wasn't real? Maybe you can come up with something better.

> + */
> +
> +static int tmp401_detect(struct i2c_adapter *adapter, int address, int kind)
> +{
> +	int i, err = 0;
> +	struct i2c_client *client;
> +	struct tmp401_data *data;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +		return 0;
> +
> +	/* 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 i2c_smbus_read_byte_data. */
> +	if (!(data = kzalloc(sizeof(struct tmp401_data), GFP_KERNEL)))

checkpatch.pl suggests that the following syntax is preferred:

	data = kzalloc(sizeof(struct tmp401_data), GFP_KERNEL);
	if (!data)

I've changed almost all the hwmon drivers to that syntax already.

> +		return -ENOMEM;
> +
> +	client = &data->client;
> +	i2c_set_clientdata(client, data);
> +	client->addr = address;
> +	client->adapter = adapter;
> +	client->driver = &tmp401_driver;
> +	mutex_init(&data->update_lock);
> +
> +	/* Detect & Identify the chip */

and identify

> +	if (kind <= 0) {
> +		u8 manufacturer_id;
> +		u8 device_id;
> +
> +		manufacturer_id = i2c_smbus_read_byte_data(client,
> +						TMP401_MANUFACTURER_ID_REG);
> +		device_id = i2c_smbus_read_byte_data(client,
> +						TMP401_DEVICE_ID_REG);
> +
> +		if (manufacturer_id != TMP401_MANUFACTURER_ID ||
> +				device_id != TMP401_DEVICE_ID)
> +			goto exit_free;
> +	}

The sensors-detect code also checks for the conversion rate register
value and unused bits in the configuration register. Maybe you can do
the same for more robust detection?

> +
> +	/* Tell the I2C layer a new client has arrived */
> +	if ((err = i2c_attach_client(client)))

Same as above, that would be:

	err = i2c_attach_client(client);
	if (err)

> +		goto exit_free;
> +
> +	/* Initialize the TMP401 chip */
> +	tmp401_init_client(client);
> +
> +	/* Register sysfs hooks */
> +	for (i = 0; i < ARRAY_SIZE(tmp401_attr); i++) {
> +		err = device_create_file(&client->dev,
> +					&tmp401_attr[i].dev_attr);
> +		if (err)
> +			goto exit_detach;
> +	}

Why don't you create an attribute group? You wouldn't have to loop over
it yourself.

> +
> +	data->hwmon_dev = hwmon_device_register(&client->dev);
> +	if (IS_ERR(data->hwmon_dev)) {
> +		err = PTR_ERR(data->hwmon_dev);
> +		data->hwmon_dev = NULL;
> +		goto exit_detach;
> +	}
> +
> +	printk(KERN_INFO TMP401_NAME ": Detected TI TMP401 chip\n");

Please use dev_info() instead.

> +
> +	return 0;
> +
> +exit_detach:
> +	tmp401_detach_client(client); /* will also free data for us */
> +	return err;
> +
> +exit_free:
> +	kfree(data);
> +	return err;
> +}
> +
> +static void tmp401_init_client(struct i2c_client *client)
> +{
> +	int config, config_orig;
> +
> +	/* Set the conversion rate to 2 Hz */
> +	i2c_smbus_write_byte_data(client, TMP401_CONVERSION_RATE_WRITE, 5);
> +
> +	/* Start conversions (disable shutdown if necessary) */
> +	config = i2c_smbus_read_byte_data(client, TMP401_CONFIG_READ);
> +	if (config < 0) {
> +		dev_warn(&client->dev, "Initialization failed!\n");
> +		return;
> +	}
> +
> +	config_orig = config;
> +	config &= ~TMP401_CONFIG_SHUTDOWN_MASK;
> +
> +	if (config != config_orig)
> +		i2c_smbus_write_byte_data(client, TMP401_CONFIG_WRITE, config);
> +}
> +
> +static int tmp401_attach_adapter(struct i2c_adapter *adapter)
> +{
> +	if (!(adapter->class & I2C_CLASS_HWMON))
> +		return 0;
> +	return i2c_probe(adapter, &addr_data, tmp401_detect);
> +}
> +
> +static int tmp401_detach_client(struct i2c_client *client)
> +{
> +	struct tmp401_data *data = i2c_get_clientdata(client);
> +	int i, err;
> +
> +	/* Check if registered in case we're called from tmp401_detect
> +	   to cleanup after an error */
> +	if (data->hwmon_dev)
> +		hwmon_device_unregister(data->hwmon_dev);
> +
> +	for (i = 0; i < ARRAY_SIZE(tmp401_attr); i++)
> +		device_remove_file(&client->dev, &tmp401_attr[i].dev_attr);
> +
> +	if ((err = i2c_detach_client(client)))

checkpatch warns on this as well.

> +		return err;
> +
> +	kfree(data);
> +	return 0;
> +}
> +
> +static struct tmp401_data *tmp401_update_device(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct tmp401_data *data = i2c_get_clientdata(client);
> +	int i;
> +
> +	mutex_lock(&data->update_lock);
> +
> +	if (time_after(jiffies, data->last_updated + 2 * HZ) || !data->valid) {

Why 2 seconds? According to the datasheet, the conversion takes 115 ms
per channel, so 230 ms total. And you've set the conversion rate to 2
Hz. So it would seem reasonable to let the user query the temperature
values every second.

> +		data->status = i2c_smbus_read_byte_data(client, TMP401_STATUS);
> +		data->config = i2c_smbus_read_byte_data(client,
> +						TMP401_CONFIG_READ);
> +		for (i = 0; i < 2; i++) {
> +			data->temp[i] = i2c_smbus_read_byte_data(client,
> +						TMP401_TEMP_MSB[i]) << 8;
> +			data->temp[i] |= i2c_smbus_read_byte_data(client,
> +						TMP401_TEMP_LSB[i]);

I would welcome a comment explaining that the high byte must be read
first and the low byte must be read immediately after, to ensure that
both bytes belong to the same measurement.

> +			data->temp_low[i] = i2c_smbus_read_byte_data(client,
> +				TMP401_TEMP_LOW_LIMIT_MSB_READ[i]) << 8;
> +			data->temp_low[i] |= i2c_smbus_read_byte_data(client,
> +						TMP401_TEMP_LOW_LIMIT_LSB[i]);
> +			data->temp_high[i] = i2c_smbus_read_byte_data(client,
> +				TMP401_TEMP_HIGH_LIMIT_MSB_READ[i]) << 8;
> +			data->temp_high[i] |= i2c_smbus_read_byte_data(client,
> +						TMP401_TEMP_HIGH_LIMIT_LSB[i]);
> +			data->temp_crit[i] = i2c_smbus_read_byte_data(client,
> +						TMP401_TEMP_CRIT_LIMIT[i]);
> +		}
> +
> +		data->temp_crit_hyst = i2c_smbus_read_byte_data(client,
> +						TMP401_TEMP_CRIT_HYST);
> +
> +		data->last_updated = jiffies;
> +		data->valid = 1;
> +	}
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return data;
> +}
> +
> +static int __init tmp401_init(void)
> +{
> +	return i2c_add_driver(&tmp401_driver);
> +}
> +
> +static void __exit tmp401_exit(void)
> +{
> +	i2c_del_driver(&tmp401_driver);
> +}
> +
> +MODULE_AUTHOR("Hans de Goede <j.w.r.degoede@hhs.nl>");
> +MODULE_DESCRIPTION("Texas Instruments TMP401 temperature sensor driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(tmp401_init);
> +module_exit(tmp401_exit);

Overall the drivers looks good to me.

Can we have a Documentation/hwmon/tmp401 file?

As a side note, it seems to me that your driver also supports the TI
TMP411 chip. Both chips have the same device ID and seem to be
essentially compatible. The only differences I can see are:
* The TMP411 lacks the one-shot mode.
* The TMP411 stores the minimum and maximum temperatures during runtime.
* The TMP411 can be reset by software.

-- 
Jean Delvare

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

Re: [lm-sensors] PATCH: hwmon-new-driver-ti-tmp401.patch

[Jean Delvare]

On Wed, 20 Aug 2008 13:20:32 +0200, Jean Delvare wrote:
> As a side note, it seems to me that your driver also supports the TI
> TMP411 chip. Both chips have the same device ID and seem to be
> essentially compatible. The only differences I can see are:
> * The TMP411 lacks the one-shot mode.
> * The TMP411 stores the minimum and maximum temperatures during runtime.
> * The TMP411 can be reset by software.

Oops, scratch this. I had an old version of the datasheet. The new
version has different IDs for the TMP411 (3 IDs, one for each possible
I2C address) and it also shows the one-shot mode. I'll fix sensors-detect
and the wiki in a minute.

-- 
Jean Delvare

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

Re: [lm-sensors] PATCH: hwmon-new-driver-ti-tmp401.patch

[Jean Delvare]

Hi Hans,

On Sat, 14 Jun 2008 17:15:13 +0200, Hans de Goede wrote:
> Hi All,
> 
> This is a new hwmon driver for TI's TMP401 temperature sensor IC. This driver
> was written on behalf of an embedded systems vendor under the
> linuxdriverproject.
> 
> It has been tested using a TI TMP401 sample attached to a i2c-tiny-usb adapter.
> Which was provided by Till Harbaum, many thanks to him for this!
> 
> Signed-off-by: Hans de Goede <j.w.r.degoede@hhs.nl>

One more thing that was brought to my attention by a friend of mine who
tested your code:

> +static int tmp401_detect(struct i2c_adapter *adapter, int address, int kind)
> +{
> +	int i, err = 0;
> +	struct i2c_client *client;
> +	struct tmp401_data *data;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +		return 0;
> +
> +	/* 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 i2c_smbus_read_byte_data. */
> +	if (!(data = kzalloc(sizeof(struct tmp401_data), GFP_KERNEL)))
> +		return -ENOMEM;
> +
> +	client = &data->client;
> +	i2c_set_clientdata(client, data);
> +	client->addr = address;
> +	client->adapter = adapter;
> +	client->driver = &tmp401_driver;
> +	mutex_init(&data->update_lock);
> +
> +	/* Detect & Identify the chip */
> +	if (kind <= 0) {
> +		u8 manufacturer_id;
> +		u8 device_id;
> +
> +		manufacturer_id = i2c_smbus_read_byte_data(client,
> +						TMP401_MANUFACTURER_ID_REG);
> +		device_id = i2c_smbus_read_byte_data(client,
> +						TMP401_DEVICE_ID_REG);
> +
> +		if (manufacturer_id != TMP401_MANUFACTURER_ID ||
> +				device_id != TMP401_DEVICE_ID)
> +			goto exit_free;
> +	}
> +

You forgot to set the client name. libsensors will complain...

> +	/* Tell the I2C layer a new client has arrived */
> +	if ((err = i2c_attach_client(client)))
> +		goto exit_free;
> +
> +	/* Initialize the TMP401 chip */
> +	tmp401_init_client(client);
> +
> +	/* Register sysfs hooks */
> +	for (i = 0; i < ARRAY_SIZE(tmp401_attr); i++) {
> +		err = device_create_file(&client->dev,
> +					&tmp401_attr[i].dev_attr);
> +		if (err)
> +			goto exit_detach;
> +	}
> +
> +	data->hwmon_dev = hwmon_device_register(&client->dev);
> +	if (IS_ERR(data->hwmon_dev)) {
> +		err = PTR_ERR(data->hwmon_dev);
> +		data->hwmon_dev = NULL;
> +		goto exit_detach;
> +	}
> +
> +	printk(KERN_INFO TMP401_NAME ": Detected TI TMP401 chip\n");
> +
> +	return 0;
> +
> +exit_detach:
> +	tmp401_detach_client(client); /* will also free data for us */
> +	return err;
> +
> +exit_free:
> +	kfree(data);
> +	return err;
> +}

-- 
Jean Delvare

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

Re: [lm-sensors] PATCH: hwmon-new-driver-ti-tmp401.patch

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
> On Sat, 14 Jun 2008 17:15:13 +0200, Hans de Goede wrote:
>> Hi All,
>>
>> This is a new hwmon driver for TI's TMP401 temperature sensor IC. This driver
>> was written on behalf of an embedded systems vendor under the
>> linuxdriverproject.
>>
>> It has been tested using a TI TMP401 sample attached to a i2c-tiny-usb adapter.
>> Which was provided by Till Harbaum, many thanks to him for this!
>>
>> Signed-off-by: Hans de Goede <j.w.r.degoede@hhs.nl>
> 
> One more thing that was brought to my attention by a friend of mine who
> tested your code:
> 

Cool, someone is actually using it :) (I wrote it for a linuxdriverproject.org 
request, but it the company who wanted it never followed through).

ANd many thanks for this review and for the fschmd watchdog patch review!!

Unfortunately currently I'm rather swamped. But I will do new version in the 
next couple of weeks.

Regards,

Hans

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

Re: [lm-sensors] PATCH: hwmon-new-driver-ti-tmp401.patch

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
> On Sat, 14 Jun 2008 17:15:13 +0200, Hans de Goede wrote:
>> This is a new hwmon driver for TI's TMP401 temperature sensor IC. This driver
>> was written on behalf of an embedded systems vendor under the
>> linuxdriverproject.
>>
>> It has been tested using a TI TMP401 sample attached to a i2c-tiny-usb adapter.
>> Which was provided by Till Harbaum, many thanks to him for this!
>>
>> Signed-off-by: Hans de Goede <j.w.r.degoede@hhs.nl>
> 
> Review:
> 

Sorry for the slow response and thanks for the review, I now have a local 
version ready with all issues fixed except for one:

<snip>

>> +static struct sensor_device_attribute tmp401_attr[] = {
>> +	SENSOR_ATTR(temp1_input, 0444, show_temp_value, NULL, 0),
>> +	SENSOR_ATTR(temp1_min, 0644, show_temp_min, store_temp_min, 0),
>> +	SENSOR_ATTR(temp1_max, 0644, show_temp_max, store_temp_max, 0),
>> +	SENSOR_ATTR(temp1_crit, 0644, show_temp_crit, store_temp_crit, 0),
>> +	SENSOR_ATTR(temp1_crit_hyst, 0644, show_temp_crit_hyst,
>> +			store_temp_crit_hyst, 0),
>> +	SENSOR_ATTR(temp1_min_alarm, 0444, show_status, NULL,
>> +			TMP401_STATUS_LOCAL_LOW_MASK),
>> +	SENSOR_ATTR(temp1_max_alarm, 0444, show_status, NULL,
>> +			TMP401_STATUS_LOCAL_HIGH_MASK),
>> +	SENSOR_ATTR(temp1_crit_alarm, 0444, show_status, NULL,
>> +			TMP401_STATUS_LOCAL_CRIT_MASK),
>> +	SENSOR_ATTR(temp2_input, 0444, show_temp_value, NULL, 1),
>> +	SENSOR_ATTR(temp2_min, 0644, show_temp_min, store_temp_min, 1),
>> +	SENSOR_ATTR(temp2_max, 0644, show_temp_max, store_temp_max, 1),
>> +	SENSOR_ATTR(temp2_crit, 0644, show_temp_crit, store_temp_crit, 1),
>> +	SENSOR_ATTR(temp2_crit_hyst, 0444, show_temp_crit_hyst, NULL, 1),
>> +	SENSOR_ATTR(temp2_fault, 0444, show_status, NULL,
>> +			TMP401_STATUS_REMOTE_OPEN_MASK),
>> +	SENSOR_ATTR(temp2_min_alarm, 0444, show_status, NULL,
>> +			TMP401_STATUS_REMOTE_LOW_MASK),
>> +	SENSOR_ATTR(temp2_max_alarm, 0444, show_status, NULL,
>> +			TMP401_STATUS_REMOTE_HIGH_MASK),
>> +	SENSOR_ATTR(temp2_crit_alarm, 0444, show_status, NULL,
>> +			TMP401_STATUS_REMOTE_CRIT_MASK),
>> +};
> 
> At page 11, in section "STATUS REGISTER", the datasheet suggests that
> the hysteresis value also applies to the local and remote high limits.
> Did you test that? If this is true then you should add files
> temp1_max_hyst and temp2_max_hyst (read-only.)
> 

This only is true if the AL/TH bit in the config register is 1. I choose to 
ignore this non default mode for now. I think it indeed makes sense to 
conditionally add temp1_max_hyst and temp2_max_hyst (read-only) when this is 
the case, but I'm not sure if its worth the additional code, what do you think?

> As a side note, it seems to me that your driver also supports the TI
> TMP411 chip. Both chips seem to be essentially compatible.

They are I've ordered a few samples and adding support for this chip is on my 
to do list.

Regards,

Hans

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

Re: [lm-sensors] PATCH: hwmon-new-driver-ti-tmp401.patch

[Jean Delvare]

Hi Hans,

On Thu, 23 Oct 2008 22:13:17 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > Hi Hans,
> > 
> > On Sat, 14 Jun 2008 17:15:13 +0200, Hans de Goede wrote:
> >> This is a new hwmon driver for TI's TMP401 temperature sensor IC. This driver
> >> was written on behalf of an embedded systems vendor under the
> >> linuxdriverproject.
> >>
> >> It has been tested using a TI TMP401 sample attached to a i2c-tiny-usb adapter.
> >> Which was provided by Till Harbaum, many thanks to him for this!
> >>
> >> Signed-off-by: Hans de Goede <j.w.r.degoede@hhs.nl>
> > 
> > Review:
> > 
> 
> Sorry for the slow response and thanks for the review, I now have a local 
> version ready with all issues fixed except for one:
> 
> <snip>
> 
> >> +static struct sensor_device_attribute tmp401_attr[] = {
> >> +	SENSOR_ATTR(temp1_input, 0444, show_temp_value, NULL, 0),
> >> +	SENSOR_ATTR(temp1_min, 0644, show_temp_min, store_temp_min, 0),
> >> +	SENSOR_ATTR(temp1_max, 0644, show_temp_max, store_temp_max, 0),
> >> +	SENSOR_ATTR(temp1_crit, 0644, show_temp_crit, store_temp_crit, 0),
> >> +	SENSOR_ATTR(temp1_crit_hyst, 0644, show_temp_crit_hyst,
> >> +			store_temp_crit_hyst, 0),
> >> +	SENSOR_ATTR(temp1_min_alarm, 0444, show_status, NULL,
> >> +			TMP401_STATUS_LOCAL_LOW_MASK),
> >> +	SENSOR_ATTR(temp1_max_alarm, 0444, show_status, NULL,
> >> +			TMP401_STATUS_LOCAL_HIGH_MASK),
> >> +	SENSOR_ATTR(temp1_crit_alarm, 0444, show_status, NULL,
> >> +			TMP401_STATUS_LOCAL_CRIT_MASK),
> >> +	SENSOR_ATTR(temp2_input, 0444, show_temp_value, NULL, 1),
> >> +	SENSOR_ATTR(temp2_min, 0644, show_temp_min, store_temp_min, 1),
> >> +	SENSOR_ATTR(temp2_max, 0644, show_temp_max, store_temp_max, 1),
> >> +	SENSOR_ATTR(temp2_crit, 0644, show_temp_crit, store_temp_crit, 1),
> >> +	SENSOR_ATTR(temp2_crit_hyst, 0444, show_temp_crit_hyst, NULL, 1),
> >> +	SENSOR_ATTR(temp2_fault, 0444, show_status, NULL,
> >> +			TMP401_STATUS_REMOTE_OPEN_MASK),
> >> +	SENSOR_ATTR(temp2_min_alarm, 0444, show_status, NULL,
> >> +			TMP401_STATUS_REMOTE_LOW_MASK),
> >> +	SENSOR_ATTR(temp2_max_alarm, 0444, show_status, NULL,
> >> +			TMP401_STATUS_REMOTE_HIGH_MASK),
> >> +	SENSOR_ATTR(temp2_crit_alarm, 0444, show_status, NULL,
> >> +			TMP401_STATUS_REMOTE_CRIT_MASK),
> >> +};
> > 
> > At page 11, in section "STATUS REGISTER", the datasheet suggests that
> > the hysteresis value also applies to the local and remote high limits.
> > Did you test that? If this is true then you should add files
> > temp1_max_hyst and temp2_max_hyst (read-only.)
> > 
> 
> This only is true if the AL/TH bit in the config register is 1. I choose to 
> ignore this non default mode for now. I think it indeed makes sense to 
> conditionally add temp1_max_hyst and temp2_max_hyst (read-only) when this is 
> the case, but I'm not sure if its worth the additional code, what do you think?

I have no objection to omitting this for now. Said files can be added
later if someone needs them.

> > As a side note, it seems to me that your driver also supports the TI
> > TMP411 chip. Both chips seem to be essentially compatible.
> 
> They are I've ordered a few samples and adding support for this chip is on my 
> to do list.

Thanks,
-- 
Jean Delvare

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