[lm-sensors] [PATCH 1/2] hwmon: Add driver tmp421 for TI's

[lm-sensors] [PATCH 1/2] hwmon: Add driver tmp421 for TI's

[Andre Prendel]

TI's TMP421/422/423 are I2C temperature sensor chips. These chips are
similar to TI's TMP401/411 chips, but with reduced functionality (only
temperature measurement). The chips have one local sensor and up to
three (TMP423) remote sensors.

Notes:
Hans, I've dropped two checks in tmp421_detect(). These checks came
from tmp401 (copied by your students).

1. Are reserved bits set to 0 in configuration register?
2. Are unused bits in conversion rate register set to 0?

I haven't any experience with real chips, so my question: Do we need
these checks. Isn't the device id check enough? If so, I will bring
back these checks.

Signed-off-by: Andre Prendel <andre.prendel@gmx.de>
---
 Kconfig  |   10 +
 Makefile |    1 
 tmp421.c |  331 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 342 insertions(+)

Index: linux-2.6/drivers/hwmon/tmp421.c
=================================--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6/drivers/hwmon/tmp421.c	2009-06-12 22:13:34.000000000 +0200
@@ -0,0 +1,331 @@
+/* tmp421.c
+ *
+ * Copyright (C) 2009 Andre Prendel <andre.prendel@gmx.de>
+ * Preliminary support by:
+ * Melvin Rook, Raymond Ng
+ *
+ * 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 TMP421 SMBUS temperature sensor IC.
+ * Supported models: TMP421, TMP422, TMP423
+ */
+
+#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[] = { 0x2a, 0x4c, 0x4d, 0x4e, 0x4f,
+				       I2C_CLIENT_END };
+
+/* Insmod parameters */
+I2C_CLIENT_INSMOD_3(tmp421, tmp422, tmp423);
+
+/* The TMP421 registers */
+#define TMP421_STATUS_REG			0x08
+#define TMP421_CONFIG_REG_1			0x09
+#define TMP421_CONFIG_REG_2			0x0A
+#define TMP421_CONVERSION_RATE_REG		0x0B
+#define TMP421_MANUFACTURER_ID_REG		0xFE
+#define TMP421_DEVICE_ID_REG			0xFF
+
+static const u8 TMP421_TEMP_MSB[4]		= { 0x00, 0x01, 0x02, 0x03 };
+static const u8 TMP421_TEMP_LSB[4]		= { 0x10, 0x11, 0x12, 0x13 };
+
+/* Flags */
+#define TMP421_CONFIG_SHUTDOWN			0x40
+#define TMP421_CONFIG_RANGE			0x04
+
+/* Manufacturer / Device ID's */
+#define TMP421_MANUFACTURER_ID			0x55
+#define TMP421_DEVICE_ID			0x21
+#define TMP422_DEVICE_ID			0x22
+#define TMP423_DEVICE_ID			0x23
+
+static int tmp421_probe(struct i2c_client *client,
+			const struct i2c_device_id *id);
+static int tmp421_detect(struct i2c_client *client, int kind,
+			 struct i2c_board_info *info);
+static int tmp421_remove(struct i2c_client *client);
+static struct tmp421_data *tmp421_update_device(struct device *dev);
+
+static const struct i2c_device_id tmp421_id[] = {
+	{ "tmp421", tmp421 },
+	{ "tmp422", tmp422 },
+	{ "tmp423", tmp423 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tmp421_id);
+
+static struct i2c_driver tmp421_driver = {
+	.class = I2C_CLASS_HWMON,
+	.driver = {
+		.name	= "tmp421",
+	},
+	.probe = tmp421_probe,
+	.remove = tmp421_remove,
+	.id_table = tmp421_id,
+	.detect = tmp421_detect,
+	.address_data = &addr_data,
+};
+
+struct tmp421_data {
+	struct device *hwmon_dev;
+	struct mutex update_lock;
+	char valid;
+	unsigned long last_updated;
+	int kind;
+	u8 config;
+	s16 temp[4];
+};
+
+static int temp_from_s16(s16 reg)
+{
+	int temp = reg;
+
+	return (temp * 1000 + 128) / 256;
+}
+
+static int temp_from_u16(u16 reg)
+{
+	int temp = reg;
+
+	/* Add offset for extended temperature range. */
+	temp -= 64 * 256;
+
+	return (temp * 1000 + 128) / 256;
+}
+
+static ssize_t show_temp_value(struct device *dev,
+			       struct device_attribute *devattr, char *buf)
+{
+	int index = to_sensor_dev_attr(devattr)->index;
+	struct tmp421_data *data = tmp421_update_device(dev);
+	int temp;
+
+	if (data->config & TMP421_CONFIG_RANGE)
+		temp = temp_from_u16(data->temp[index]);
+	else
+		temp = temp_from_s16(data->temp[index]);
+
+	return sprintf(buf, "%d\n", temp);
+}
+
+static ssize_t show_fault(struct device *dev,
+			  struct device_attribute *devattr, char *buf)
+{
+	int index = to_sensor_dev_attr(devattr)->index;
+	struct tmp421_data *data = tmp421_update_device(dev);
+
+	/*
+	 * The OPEN bit signals a fault. This is bit 0 of the temperature
+	 * register (low byte).
+	 */
+	if (data->temp[index] & 0x01)
+		return sprintf(buf, "1\n");
+	else
+		return sprintf(buf, "0\n");
+}
+
+static struct sensor_device_attribute tmp421_attr[] = {
+	SENSOR_ATTR(temp1_input, 0444, show_temp_value, NULL, 0),
+	SENSOR_ATTR(temp2_input, 0444, show_temp_value, NULL, 1),
+	SENSOR_ATTR(temp2_fault, 0444, show_fault, NULL, 1),
+	SENSOR_ATTR(temp3_input, 0444, show_temp_value, NULL, 2),
+	SENSOR_ATTR(temp3_fault, 0444, show_fault, NULL, 2),
+	SENSOR_ATTR(temp4_input, 0444, show_temp_value, NULL, 3),
+	SENSOR_ATTR(temp4_fault, 0444, show_fault, NULL, 3),
+};
+
+static void tmp421_init_client(struct i2c_client *client)
+{
+	int config, config_orig;
+
+	/* Set the conversion rate to 2 Hz */
+	i2c_smbus_write_byte_data(client, TMP421_CONVERSION_RATE_REG, 0x05);
+
+	/* Start conversions (disable shutdown if necessary) */
+	config = i2c_smbus_read_byte_data(client, TMP421_CONFIG_REG_1);
+	if (config < 0) {
+		dev_warn(&client->dev, "Could not read configuration"
+			 "register (%d)\n", config);
+		return;
+	}
+
+	config_orig = config;
+	config &= ~TMP421_CONFIG_SHUTDOWN;
+
+	if (config != config_orig)
+		i2c_smbus_write_byte_data(client, TMP421_CONFIG_REG_1, config);
+}
+
+static int tmp421_detect(struct i2c_client *client, int kind,
+			 struct i2c_board_info *info)
+{
+	struct i2c_adapter *adapter = client->adapter;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -ENODEV;
+
+	if (kind <= 0) {
+		u8 reg;
+
+		reg = i2c_smbus_read_byte_data(client,
+					       TMP421_MANUFACTURER_ID_REG);
+
+		if (reg != TMP421_MANUFACTURER_ID)
+			return -ENODEV;
+
+		reg = i2c_smbus_read_byte_data(client,
+					       TMP421_DEVICE_ID_REG);
+
+		switch (reg) {
+		case TMP421_DEVICE_ID:
+			kind = tmp421;
+			break;
+		case TMP422_DEVICE_ID:
+			kind = tmp422;
+			break;
+		case TMP423_DEVICE_ID:
+			kind = tmp423;
+			break;
+		default:
+			return -ENODEV;
+
+		}
+	}
+	strlcpy(info->type, tmp421_id[kind - 1].name, I2C_NAME_SIZE);
+
+	return 0;
+}
+
+static int tmp421_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct tmp421_data *data;
+	const char *names[] = { "TMP421", "TMP422", "TMP423" };
+	int i, err;
+
+	data = kzalloc(sizeof(struct tmp421_data), GFP_KERNEL);
+	if (!data)
+		return -ENODEV;
+
+	i2c_set_clientdata(client, data);
+	mutex_init(&data->update_lock);
+	data->kind = id->driver_data;
+
+	tmp421_init_client(client);
+
+	for (i = 0; i < ARRAY_SIZE(tmp421_attr); i++) {
+		err = device_create_file(&client->dev,
+					 &tmp421_attr[i].dev_attr);
+
+		if (err)
+			goto exit_remove;
+
+		/*
+		 * Create only necessary sysfs files.
+		 * TMP421 up to one remote sensor
+		 * TMP422 up to two remote sensors
+		 * TMP423 up to three remote sensors.
+		 */
+		if (i >= data->kind * 2)
+			break;
+	}
+
+	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_remove;
+	}
+
+	dev_info(&client->dev, "Detected TI %s chip\n", names[data->kind - 1]);
+	return 0;
+
+exit_remove:
+	tmp421_remove(client);
+	return err;
+}
+
+static int tmp421_remove(struct i2c_client *client)
+{
+	struct tmp421_data *data = i2c_get_clientdata(client);
+	int i;
+
+	if (data->hwmon_dev)
+		hwmon_device_unregister(data->hwmon_dev);
+
+	for (i = 0; i < ARRAY_SIZE(tmp421_attr); i++)
+		device_remove_file(&client->dev, &tmp421_attr[i].dev_attr);
+
+	kfree(data);
+
+	return 0;
+}
+
+static struct tmp421_data *tmp421_update_device(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct tmp421_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->config = i2c_smbus_read_byte_data(client,
+			TMP421_CONFIG_REG_1);
+
+		for (i = 0; i <= data->kind; i++) {
+			data->temp[i] = i2c_smbus_read_byte_data(client,
+				TMP421_TEMP_MSB[i]) << 8;
+			data->temp[i] |= i2c_smbus_read_byte_data(client,
+				TMP421_TEMP_LSB[i]);
+		}
+		data->last_updated = jiffies;
+		data->valid = 1;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return data;
+}
+
+static int __init tmp421_init(void)
+{
+	return i2c_add_driver(&tmp421_driver);
+}
+
+static void __exit tmp421_exit(void)
+{
+	i2c_del_driver(&tmp421_driver);
+}
+
+MODULE_AUTHOR("Andre Prendel");
+MODULE_DESCRIPTION("Texas Instruments TMP421/422/423 temperature sensor"
+		   "driver");
+MODULE_LICENSE("GPL");
+
+module_init(tmp421_init);
+module_exit(tmp421_exit);
Index: linux-2.6/drivers/hwmon/Kconfig
=================================--- linux-2.6.orig/drivers/hwmon/Kconfig	2009-06-11 21:50:46.000000000 +0200
+++ linux-2.6/drivers/hwmon/Kconfig	2009-06-11 21:50:46.000000000 +0200
@@ -797,6 +797,16 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called tmp401.
 
+config SENSORS_TMP421
+	tristate "Texas Instruments TMP421 and compatibles"
+	depends on I2C && EXPERIMENTAL
+	help
+	  If you say yes here you get support for Texas Instruments TMP421
+	  temperature sensor chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called tmp421.
+
 config SENSORS_VIA686A
 	tristate "VIA686A"
 	depends on PCI
Index: linux-2.6/drivers/hwmon/Makefile
=================================--- linux-2.6.orig/drivers/hwmon/Makefile	2009-06-11 21:50:46.000000000 +0200
+++ linux-2.6/drivers/hwmon/Makefile	2009-06-11 21:50:46.000000000 +0200
@@ -83,6 +83,7 @@
 obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
 obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
 obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
+obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
 obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
 obj-$(CONFIG_SENSORS_VT1211)	+= vt1211.o
 obj-$(CONFIG_SENSORS_VT8231)	+= vt8231.o

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

Re: [lm-sensors] [PATCH 1/2] hwmon: Add driver tmp421 for TI's

[Jean Delvare]

Hi Andre,

On Mon, 15 Jun 2009 09:43:20 +0200, Andre Prendel wrote:
> TI's TMP421/422/423 are I2C temperature sensor chips. These chips are
> similar to TI's TMP401/411 chips, but with reduced functionality (only
> temperature measurement). The chips have one local sensor and up to
> three (TMP423) remote sensors.

Overall good, please see my review below.

> 
> Notes:
> Hans, I've dropped two checks in tmp421_detect(). These checks came
> from tmp401 (copied by your students).
> 
> 1. Are reserved bits set to 0 in configuration register?
> 2. Are unused bits in conversion rate register set to 0?
> 
> I haven't any experience with real chips, so my question: Do we need
> these checks. Isn't the device id check enough? If so, I will bring
> back these checks.

In favor of the checks:
* I2C has no standard chip identification method, so just because a
  chip match a few register checks doesn't mean it's the chip you were
  looking for.
* Your driver checks several addresses, which increases the risk of
  false positives.

Against the checks:
* Slow down driver loading.
* I2C probing is restricted by classes, which should limit the risk of
  false positives.

So there is no absolute rule, other than the fact that any false
positive reported will result in an improved (and slower) detection
routine to prevent it.

> 
> Signed-off-by: Andre Prendel <andre.prendel@gmx.de>
> ---
>  Kconfig  |   10 +
>  Makefile |    1 
>  tmp421.c |  331 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 342 insertions(+)
> 
> Index: linux-2.6/drivers/hwmon/tmp421.c
> =================================> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/drivers/hwmon/tmp421.c	2009-06-12 22:13:34.000000000 +0200
> @@ -0,0 +1,331 @@
> +/* tmp421.c
> + *
> + * Copyright (C) 2009 Andre Prendel <andre.prendel@gmx.de>
> + * Preliminary support by:
> + * Melvin Rook, Raymond Ng
> + *
> + * 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 TMP421 SMBUS temperature sensor IC.

Spelling: SMBus.

> + * Supported models: TMP421, TMP422, TMP423
> + */
> +
> +#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[] = { 0x2a, 0x4c, 0x4d, 0x4e, 0x4f,
> +				       I2C_CLIENT_END };
> +
> +/* Insmod parameters */
> +I2C_CLIENT_INSMOD_3(tmp421, tmp422, tmp423);
> +
> +/* The TMP421 registers */
> +#define TMP421_STATUS_REG			0x08
> +#define TMP421_CONFIG_REG_1			0x09
> +#define TMP421_CONFIG_REG_2			0x0A

You don't seem to use TMP421_STATUS_REG nor TMP421_CONFIG_REG_2
anywhere, so why define them?

> +#define TMP421_CONVERSION_RATE_REG		0x0B
> +#define TMP421_MANUFACTURER_ID_REG		0xFE
> +#define TMP421_DEVICE_ID_REG			0xFF
> +
> +static const u8 TMP421_TEMP_MSB[4]		= { 0x00, 0x01, 0x02, 0x03 };
> +static const u8 TMP421_TEMP_LSB[4]		= { 0x10, 0x11, 0x12, 0x13 };
> +
> +/* Flags */
> +#define TMP421_CONFIG_SHUTDOWN			0x40
> +#define TMP421_CONFIG_RANGE			0x04
> +
> +/* Manufacturer / Device ID's */
> +#define TMP421_MANUFACTURER_ID			0x55
> +#define TMP421_DEVICE_ID			0x21
> +#define TMP422_DEVICE_ID			0x22
> +#define TMP423_DEVICE_ID			0x23
> +
> +static int tmp421_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id);
> +static int tmp421_detect(struct i2c_client *client, int kind,
> +			 struct i2c_board_info *info);
> +static int tmp421_remove(struct i2c_client *client);
> +static struct tmp421_data *tmp421_update_device(struct device *dev);

Please reorder the functions so that these forward declarations are no
longer needed.

> +
> +static const struct i2c_device_id tmp421_id[] = {
> +	{ "tmp421", tmp421 },
> +	{ "tmp422", tmp422 },
> +	{ "tmp423", tmp423 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, tmp421_id);
> +
> +static struct i2c_driver tmp421_driver = {
> +	.class = I2C_CLASS_HWMON,
> +	.driver = {
> +		.name	= "tmp421",
> +	},
> +	.probe = tmp421_probe,
> +	.remove = tmp421_remove,
> +	.id_table = tmp421_id,
> +	.detect = tmp421_detect,
> +	.address_data = &addr_data,
> +};
> +
> +struct tmp421_data {
> +	struct device *hwmon_dev;
> +	struct mutex update_lock;
> +	char valid;
> +	unsigned long last_updated;
> +	int kind;
> +	u8 config;
> +	s16 temp[4];
> +};
> +
> +static int temp_from_s16(s16 reg)
> +{
> +	int temp = reg;
> +
> +	return (temp * 1000 + 128) / 256;
> +}
> +
> +static int temp_from_u16(u16 reg)
> +{
> +	int temp = reg;
> +
> +	/* Add offset for extended temperature range. */
> +	temp -= 64 * 256;
> +
> +	return (temp * 1000 + 128) / 256;
> +}
> +
> +static ssize_t show_temp_value(struct device *dev,
> +			       struct device_attribute *devattr, char *buf)
> +{
> +	int index = to_sensor_dev_attr(devattr)->index;
> +	struct tmp421_data *data = tmp421_update_device(dev);
> +	int temp;
> +
> +	if (data->config & TMP421_CONFIG_RANGE)
> +		temp = temp_from_u16(data->temp[index]);
> +	else
> +		temp = temp_from_s16(data->temp[index]);

This lacks protection: you have no guarantee that data->config and
data->temp[index] belong to the same read cycle.

> +
> +	return sprintf(buf, "%d\n", temp);
> +}
> +
> +static ssize_t show_fault(struct device *dev,
> +			  struct device_attribute *devattr, char *buf)
> +{
> +	int index = to_sensor_dev_attr(devattr)->index;
> +	struct tmp421_data *data = tmp421_update_device(dev);
> +
> +	/*
> +	 * The OPEN bit signals a fault. This is bit 0 of the temperature
> +	 * register (low byte).
> +	 */
> +	if (data->temp[index] & 0x01)
> +		return sprintf(buf, "1\n");
> +	else
> +		return sprintf(buf, "0\n");
> +}
> +
> +static struct sensor_device_attribute tmp421_attr[] = {
> +	SENSOR_ATTR(temp1_input, 0444, show_temp_value, NULL, 0),

This is S_IRUGO.

> +	SENSOR_ATTR(temp2_input, 0444, show_temp_value, NULL, 1),
> +	SENSOR_ATTR(temp2_fault, 0444, show_fault, NULL, 1),
> +	SENSOR_ATTR(temp3_input, 0444, show_temp_value, NULL, 2),
> +	SENSOR_ATTR(temp3_fault, 0444, show_fault, NULL, 2),
> +	SENSOR_ATTR(temp4_input, 0444, show_temp_value, NULL, 3),
> +	SENSOR_ATTR(temp4_fault, 0444, show_fault, NULL, 3),
> +};
> +
> +static void tmp421_init_client(struct i2c_client *client)
> +{
> +	int config, config_orig;
> +
> +	/* Set the conversion rate to 2 Hz */
> +	i2c_smbus_write_byte_data(client, TMP421_CONVERSION_RATE_REG, 0x05);
> +
> +	/* Start conversions (disable shutdown if necessary) */
> +	config = i2c_smbus_read_byte_data(client, TMP421_CONFIG_REG_1);
> +	if (config < 0) {
> +		dev_warn(&client->dev, "Could not read configuration"

Missing space between string fragments.

> +			 "register (%d)\n", config);
> +		return;
> +	}

Is this really only a warning? if you can't read from the chip, this is
a sign that something is really wrong, and it might be better to plain
fail.

> +
> +	config_orig = config;
> +	config &= ~TMP421_CONFIG_SHUTDOWN;
> +
> +	if (config != config_orig)
> +		i2c_smbus_write_byte_data(client, TMP421_CONFIG_REG_1, config);

Maybe log when you have to enable the monitoring?

> +}
> +
> +static int tmp421_detect(struct i2c_client *client, int kind,
> +			 struct i2c_board_info *info)
> +{
> +	struct i2c_adapter *adapter = client->adapter;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +		return -ENODEV;
> +
> +	if (kind <= 0) {
> +		u8 reg;
> +
> +		reg = i2c_smbus_read_byte_data(client,
> +					       TMP421_MANUFACTURER_ID_REG);
> +

Extra blank line.

> +		if (reg != TMP421_MANUFACTURER_ID)
> +			return -ENODEV;
> +
> +		reg = i2c_smbus_read_byte_data(client,
> +					       TMP421_DEVICE_ID_REG);
> +

Extra blank line.

> +		switch (reg) {
> +		case TMP421_DEVICE_ID:
> +			kind = tmp421;
> +			break;
> +		case TMP422_DEVICE_ID:
> +			kind = tmp422;
> +			break;
> +		case TMP423_DEVICE_ID:
> +			kind = tmp423;
> +			break;
> +		default:
> +			return -ENODEV;
> +

Extra blank line.

> +		}
> +	}
> +	strlcpy(info->type, tmp421_id[kind - 1].name, I2C_NAME_SIZE);
> +
> +	return 0;
> +}
> +
> +static int tmp421_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct tmp421_data *data;
> +	const char *names[] = { "TMP421", "TMP422", "TMP423" };
> +	int i, err;
> +
> +	data = kzalloc(sizeof(struct tmp421_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENODEV;

This would rather me -ENOMEM.

> +
> +	i2c_set_clientdata(client, data);
> +	mutex_init(&data->update_lock);
> +	data->kind = id->driver_data;
> +
> +	tmp421_init_client(client);
> +
> +	for (i = 0; i < ARRAY_SIZE(tmp421_attr); i++) {
> +		err = device_create_file(&client->dev,
> +					 &tmp421_attr[i].dev_attr);
> +

Extra blank line.

> +		if (err)
> +			goto exit_remove;
> +
> +		/*
> +		 * Create only necessary sysfs files.
> +		 * TMP421 up to one remote sensor
> +		 * TMP422 up to two remote sensors
> +		 * TMP423 up to three remote sensors.
> +		 */
> +		if (i >= data->kind * 2)

Huh, I don't like this, this is pretty fragile. Better have a struct
field dedicated to the number of inputs and fill it based on the chip
type. I also strongly suggests that you either rework your
tmp421_attr[] data structure, either by splitting the attributes to
per-sensor arrays, or by implementing an attribute group with
a .is_visible callback (the latter would probably be cleaner.)

> +			break;
> +	}
> +
> +	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_remove;
> +	}
> +
> +	dev_info(&client->dev, "Detected TI %s chip\n", names[data->kind - 1]);

You didn't really detect anything in this function, so this message
should either move to tmp421_detect(), or be reworded a bit.

> +	return 0;
> +
> +exit_remove:
> +	tmp421_remove(client);
> +	return err;
> +}
> +
> +static int tmp421_remove(struct i2c_client *client)
> +{
> +	struct tmp421_data *data = i2c_get_clientdata(client);
> +	int i;
> +
> +	if (data->hwmon_dev)
> +		hwmon_device_unregister(data->hwmon_dev);
> +
> +	for (i = 0; i < ARRAY_SIZE(tmp421_attr); i++)
> +		device_remove_file(&client->dev, &tmp421_attr[i].dev_attr);
> +

Please add:
	i2c_set_clientdata(client, NULL);
to avoid leaving a dangling pointer behind.

> +	kfree(data);
> +
> +	return 0;
> +}
> +
> +static struct tmp421_data *tmp421_update_device(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct tmp421_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->config = i2c_smbus_read_byte_data(client,
> +			TMP421_CONFIG_REG_1);
> +
> +		for (i = 0; i <= data->kind; i++) {
> +			data->temp[i] = i2c_smbus_read_byte_data(client,
> +				TMP421_TEMP_MSB[i]) << 8;
> +			data->temp[i] |= i2c_smbus_read_byte_data(client,
> +				TMP421_TEMP_LSB[i]);
> +		}
> +		data->last_updated = jiffies;
> +		data->valid = 1;
> +	}
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return data;
> +}
> +
> +static int __init tmp421_init(void)
> +{
> +	return i2c_add_driver(&tmp421_driver);
> +}
> +
> +static void __exit tmp421_exit(void)
> +{
> +	i2c_del_driver(&tmp421_driver);
> +}
> +
> +MODULE_AUTHOR("Andre Prendel");

No e-mail address?

> +MODULE_DESCRIPTION("Texas Instruments TMP421/422/423 temperature sensor"
> +		   "driver");

Missing space between string fragments.

> +MODULE_LICENSE("GPL");
> +
> +module_init(tmp421_init);
> +module_exit(tmp421_exit);
> Index: linux-2.6/drivers/hwmon/Kconfig
> =================================> --- linux-2.6.orig/drivers/hwmon/Kconfig	2009-06-11 21:50:46.000000000 +0200
> +++ linux-2.6/drivers/hwmon/Kconfig	2009-06-11 21:50:46.000000000 +0200
> @@ -797,6 +797,16 @@
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called tmp401.
>  
> +config SENSORS_TMP421
> +	tristate "Texas Instruments TMP421 and compatibles"
> +	depends on I2C && EXPERIMENTAL
> +	help
> +	  If you say yes here you get support for Texas Instruments TMP421
> +	  temperature sensor chips.

Please also list the TMP422 and TMP423.

> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called tmp421.
> +
>  config SENSORS_VIA686A
>  	tristate "VIA686A"
>  	depends on PCI
> Index: linux-2.6/drivers/hwmon/Makefile
> =================================> --- linux-2.6.orig/drivers/hwmon/Makefile	2009-06-11 21:50:46.000000000 +0200
> +++ linux-2.6/drivers/hwmon/Makefile	2009-06-11 21:50:46.000000000 +0200
> @@ -83,6 +83,7 @@
>  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>  obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
>  obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
> +obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
>  obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
>  obj-$(CONFIG_SENSORS_VT1211)	+= vt1211.o
>  obj-$(CONFIG_SENSORS_VT8231)	+= vt8231.o


-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH 1/2] hwmon: Add driver tmp421 for TI's

[Andre Prendel]

On Mon, Jun 15, 2009 at 10:52:24PM +0200, Jean Delvare wrote:
> Hi Andre,

Hi Jean,

> 
> On Mon, 15 Jun 2009 09:43:20 +0200, Andre Prendel wrote:
> > TI's TMP421/422/423 are I2C temperature sensor chips. These chips are
> > similar to TI's TMP401/411 chips, but with reduced functionality (only
> > temperature measurement). The chips have one local sensor and up to
> > three (TMP423) remote sensors.
> 
> Overall good, please see my review below.

I will fix the issues this week and send an updated version.

Thanks,
Andre

> > 
> > Notes:
> > Hans, I've dropped two checks in tmp421_detect(). These checks came
> > from tmp401 (copied by your students).
> > 
> > 1. Are reserved bits set to 0 in configuration register?
> > 2. Are unused bits in conversion rate register set to 0?
> > 
> > I haven't any experience with real chips, so my question: Do we need
> > these checks. Isn't the device id check enough? If so, I will bring
> > back these checks.
> 
> In favor of the checks:
> * I2C has no standard chip identification method, so just because a
>   chip match a few register checks doesn't mean it's the chip you were
>   looking for.
> * Your driver checks several addresses, which increases the risk of
>   false positives.
> 
> Against the checks:
> * Slow down driver loading.
> * I2C probing is restricted by classes, which should limit the risk of
>   false positives.
> 
> So there is no absolute rule, other than the fact that any false
> positive reported will result in an improved (and slower) detection
> routine to prevent it.
> 
> > 
> > Signed-off-by: Andre Prendel <andre.prendel@gmx.de>
> > ---
> >  Kconfig  |   10 +
> >  Makefile |    1 
> >  tmp421.c |  331 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 342 insertions(+)
> > 
> > Index: linux-2.6/drivers/hwmon/tmp421.c
> > =================================> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6/drivers/hwmon/tmp421.c	2009-06-12 22:13:34.000000000 +0200
> > @@ -0,0 +1,331 @@
> > +/* tmp421.c
> > + *
> > + * Copyright (C) 2009 Andre Prendel <andre.prendel@gmx.de>
> > + * Preliminary support by:
> > + * Melvin Rook, Raymond Ng
> > + *
> > + * 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 TMP421 SMBUS temperature sensor IC.
> 
> Spelling: SMBus.
> 
> > + * Supported models: TMP421, TMP422, TMP423
> > + */
> > +
> > +#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[] = { 0x2a, 0x4c, 0x4d, 0x4e, 0x4f,
> > +				       I2C_CLIENT_END };
> > +
> > +/* Insmod parameters */
> > +I2C_CLIENT_INSMOD_3(tmp421, tmp422, tmp423);
> > +
> > +/* The TMP421 registers */
> > +#define TMP421_STATUS_REG			0x08
> > +#define TMP421_CONFIG_REG_1			0x09
> > +#define TMP421_CONFIG_REG_2			0x0A
> 
> You don't seem to use TMP421_STATUS_REG nor TMP421_CONFIG_REG_2
> anywhere, so why define them?
> 
> > +#define TMP421_CONVERSION_RATE_REG		0x0B
> > +#define TMP421_MANUFACTURER_ID_REG		0xFE
> > +#define TMP421_DEVICE_ID_REG			0xFF
> > +
> > +static const u8 TMP421_TEMP_MSB[4]		= { 0x00, 0x01, 0x02, 0x03 };
> > +static const u8 TMP421_TEMP_LSB[4]		= { 0x10, 0x11, 0x12, 0x13 };
> > +
> > +/* Flags */
> > +#define TMP421_CONFIG_SHUTDOWN			0x40
> > +#define TMP421_CONFIG_RANGE			0x04
> > +
> > +/* Manufacturer / Device ID's */
> > +#define TMP421_MANUFACTURER_ID			0x55
> > +#define TMP421_DEVICE_ID			0x21
> > +#define TMP422_DEVICE_ID			0x22
> > +#define TMP423_DEVICE_ID			0x23
> > +
> > +static int tmp421_probe(struct i2c_client *client,
> > +			const struct i2c_device_id *id);
> > +static int tmp421_detect(struct i2c_client *client, int kind,
> > +			 struct i2c_board_info *info);
> > +static int tmp421_remove(struct i2c_client *client);
> > +static struct tmp421_data *tmp421_update_device(struct device *dev);
> 
> Please reorder the functions so that these forward declarations are no
> longer needed.
> 
> > +
> > +static const struct i2c_device_id tmp421_id[] = {
> > +	{ "tmp421", tmp421 },
> > +	{ "tmp422", tmp422 },
> > +	{ "tmp423", tmp423 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, tmp421_id);
> > +
> > +static struct i2c_driver tmp421_driver = {
> > +	.class = I2C_CLASS_HWMON,
> > +	.driver = {
> > +		.name	= "tmp421",
> > +	},
> > +	.probe = tmp421_probe,
> > +	.remove = tmp421_remove,
> > +	.id_table = tmp421_id,
> > +	.detect = tmp421_detect,
> > +	.address_data = &addr_data,
> > +};
> > +
> > +struct tmp421_data {
> > +	struct device *hwmon_dev;
> > +	struct mutex update_lock;
> > +	char valid;
> > +	unsigned long last_updated;
> > +	int kind;
> > +	u8 config;
> > +	s16 temp[4];
> > +};
> > +
> > +static int temp_from_s16(s16 reg)
> > +{
> > +	int temp = reg;
> > +
> > +	return (temp * 1000 + 128) / 256;
> > +}
> > +
> > +static int temp_from_u16(u16 reg)
> > +{
> > +	int temp = reg;
> > +
> > +	/* Add offset for extended temperature range. */
> > +	temp -= 64 * 256;
> > +
> > +	return (temp * 1000 + 128) / 256;
> > +}
> > +
> > +static ssize_t show_temp_value(struct device *dev,
> > +			       struct device_attribute *devattr, char *buf)
> > +{
> > +	int index = to_sensor_dev_attr(devattr)->index;
> > +	struct tmp421_data *data = tmp421_update_device(dev);
> > +	int temp;
> > +
> > +	if (data->config & TMP421_CONFIG_RANGE)
> > +		temp = temp_from_u16(data->temp[index]);
> > +	else
> > +		temp = temp_from_s16(data->temp[index]);
> 
> This lacks protection: you have no guarantee that data->config and
> data->temp[index] belong to the same read cycle.
> 
> > +
> > +	return sprintf(buf, "%d\n", temp);
> > +}
> > +
> > +static ssize_t show_fault(struct device *dev,
> > +			  struct device_attribute *devattr, char *buf)
> > +{
> > +	int index = to_sensor_dev_attr(devattr)->index;
> > +	struct tmp421_data *data = tmp421_update_device(dev);
> > +
> > +	/*
> > +	 * The OPEN bit signals a fault. This is bit 0 of the temperature
> > +	 * register (low byte).
> > +	 */
> > +	if (data->temp[index] & 0x01)
> > +		return sprintf(buf, "1\n");
> > +	else
> > +		return sprintf(buf, "0\n");
> > +}
> > +
> > +static struct sensor_device_attribute tmp421_attr[] = {
> > +	SENSOR_ATTR(temp1_input, 0444, show_temp_value, NULL, 0),
> 
> This is S_IRUGO.
> 
> > +	SENSOR_ATTR(temp2_input, 0444, show_temp_value, NULL, 1),
> > +	SENSOR_ATTR(temp2_fault, 0444, show_fault, NULL, 1),
> > +	SENSOR_ATTR(temp3_input, 0444, show_temp_value, NULL, 2),
> > +	SENSOR_ATTR(temp3_fault, 0444, show_fault, NULL, 2),
> > +	SENSOR_ATTR(temp4_input, 0444, show_temp_value, NULL, 3),
> > +	SENSOR_ATTR(temp4_fault, 0444, show_fault, NULL, 3),
> > +};
> > +
> > +static void tmp421_init_client(struct i2c_client *client)
> > +{
> > +	int config, config_orig;
> > +
> > +	/* Set the conversion rate to 2 Hz */
> > +	i2c_smbus_write_byte_data(client, TMP421_CONVERSION_RATE_REG, 0x05);
> > +
> > +	/* Start conversions (disable shutdown if necessary) */
> > +	config = i2c_smbus_read_byte_data(client, TMP421_CONFIG_REG_1);
> > +	if (config < 0) {
> > +		dev_warn(&client->dev, "Could not read configuration"
> 
> Missing space between string fragments.
> 
> > +			 "register (%d)\n", config);
> > +		return;
> > +	}
> 
> Is this really only a warning? if you can't read from the chip, this is
> a sign that something is really wrong, and it might be better to plain
> fail.
> 
> > +
> > +	config_orig = config;
> > +	config &= ~TMP421_CONFIG_SHUTDOWN;
> > +
> > +	if (config != config_orig)
> > +		i2c_smbus_write_byte_data(client, TMP421_CONFIG_REG_1, config);
> 
> Maybe log when you have to enable the monitoring?
> 
> > +}
> > +
> > +static int tmp421_detect(struct i2c_client *client, int kind,
> > +			 struct i2c_board_info *info)
> > +{
> > +	struct i2c_adapter *adapter = client->adapter;
> > +
> > +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> > +		return -ENODEV;
> > +
> > +	if (kind <= 0) {
> > +		u8 reg;
> > +
> > +		reg = i2c_smbus_read_byte_data(client,
> > +					       TMP421_MANUFACTURER_ID_REG);
> > +
> 
> Extra blank line.
> 
> > +		if (reg != TMP421_MANUFACTURER_ID)
> > +			return -ENODEV;
> > +
> > +		reg = i2c_smbus_read_byte_data(client,
> > +					       TMP421_DEVICE_ID_REG);
> > +
> 
> Extra blank line.
> 
> > +		switch (reg) {
> > +		case TMP421_DEVICE_ID:
> > +			kind = tmp421;
> > +			break;
> > +		case TMP422_DEVICE_ID:
> > +			kind = tmp422;
> > +			break;
> > +		case TMP423_DEVICE_ID:
> > +			kind = tmp423;
> > +			break;
> > +		default:
> > +			return -ENODEV;
> > +
> 
> Extra blank line.
> 
> > +		}
> > +	}
> > +	strlcpy(info->type, tmp421_id[kind - 1].name, I2C_NAME_SIZE);
> > +
> > +	return 0;
> > +}
> > +
> > +static int tmp421_probe(struct i2c_client *client,
> > +			const struct i2c_device_id *id)
> > +{
> > +	struct tmp421_data *data;
> > +	const char *names[] = { "TMP421", "TMP422", "TMP423" };
> > +	int i, err;
> > +
> > +	data = kzalloc(sizeof(struct tmp421_data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENODEV;
> 
> This would rather me -ENOMEM.
> 
> > +
> > +	i2c_set_clientdata(client, data);
> > +	mutex_init(&data->update_lock);
> > +	data->kind = id->driver_data;
> > +
> > +	tmp421_init_client(client);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(tmp421_attr); i++) {
> > +		err = device_create_file(&client->dev,
> > +					 &tmp421_attr[i].dev_attr);
> > +
> 
> Extra blank line.
> 
> > +		if (err)
> > +			goto exit_remove;
> > +
> > +		/*
> > +		 * Create only necessary sysfs files.
> > +		 * TMP421 up to one remote sensor
> > +		 * TMP422 up to two remote sensors
> > +		 * TMP423 up to three remote sensors.
> > +		 */
> > +		if (i >= data->kind * 2)
> 
> Huh, I don't like this, this is pretty fragile. Better have a struct
> field dedicated to the number of inputs and fill it based on the chip
> type. I also strongly suggests that you either rework your
> tmp421_attr[] data structure, either by splitting the attributes to
> per-sensor arrays, or by implementing an attribute group with
> a .is_visible callback (the latter would probably be cleaner.)
> 
> > +			break;
> > +	}
> > +
> > +	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_remove;
> > +	}
> > +
> > +	dev_info(&client->dev, "Detected TI %s chip\n", names[data->kind - 1]);
> 
> You didn't really detect anything in this function, so this message
> should either move to tmp421_detect(), or be reworded a bit.
> 
> > +	return 0;
> > +
> > +exit_remove:
> > +	tmp421_remove(client);
> > +	return err;
> > +}
> > +
> > +static int tmp421_remove(struct i2c_client *client)
> > +{
> > +	struct tmp421_data *data = i2c_get_clientdata(client);
> > +	int i;
> > +
> > +	if (data->hwmon_dev)
> > +		hwmon_device_unregister(data->hwmon_dev);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(tmp421_attr); i++)
> > +		device_remove_file(&client->dev, &tmp421_attr[i].dev_attr);
> > +
> 
> Please add:
> 	i2c_set_clientdata(client, NULL);
> to avoid leaving a dangling pointer behind.
> 
> > +	kfree(data);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct tmp421_data *tmp421_update_device(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct tmp421_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->config = i2c_smbus_read_byte_data(client,
> > +			TMP421_CONFIG_REG_1);
> > +
> > +		for (i = 0; i <= data->kind; i++) {
> > +			data->temp[i] = i2c_smbus_read_byte_data(client,
> > +				TMP421_TEMP_MSB[i]) << 8;
> > +			data->temp[i] |= i2c_smbus_read_byte_data(client,
> > +				TMP421_TEMP_LSB[i]);
> > +		}
> > +		data->last_updated = jiffies;
> > +		data->valid = 1;
> > +	}
> > +
> > +	mutex_unlock(&data->update_lock);
> > +
> > +	return data;
> > +}
> > +
> > +static int __init tmp421_init(void)
> > +{
> > +	return i2c_add_driver(&tmp421_driver);
> > +}
> > +
> > +static void __exit tmp421_exit(void)
> > +{
> > +	i2c_del_driver(&tmp421_driver);
> > +}
> > +
> > +MODULE_AUTHOR("Andre Prendel");
> 
> No e-mail address?
> 
> > +MODULE_DESCRIPTION("Texas Instruments TMP421/422/423 temperature sensor"
> > +		   "driver");
> 
> Missing space between string fragments.
> 
> > +MODULE_LICENSE("GPL");
> > +
> > +module_init(tmp421_init);
> > +module_exit(tmp421_exit);
> > Index: linux-2.6/drivers/hwmon/Kconfig
> > =================================> > --- linux-2.6.orig/drivers/hwmon/Kconfig	2009-06-11 21:50:46.000000000 +0200
> > +++ linux-2.6/drivers/hwmon/Kconfig	2009-06-11 21:50:46.000000000 +0200
> > @@ -797,6 +797,16 @@
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called tmp401.
> >  
> > +config SENSORS_TMP421
> > +	tristate "Texas Instruments TMP421 and compatibles"
> > +	depends on I2C && EXPERIMENTAL
> > +	help
> > +	  If you say yes here you get support for Texas Instruments TMP421
> > +	  temperature sensor chips.
> 
> Please also list the TMP422 and TMP423.
> 
> > +
> > +	  This driver can also be built as a module.  If so, the module
> > +	  will be called tmp421.
> > +
> >  config SENSORS_VIA686A
> >  	tristate "VIA686A"
> >  	depends on PCI
> > Index: linux-2.6/drivers/hwmon/Makefile
> > =================================> > --- linux-2.6.orig/drivers/hwmon/Makefile	2009-06-11 21:50:46.000000000 +0200
> > +++ linux-2.6/drivers/hwmon/Makefile	2009-06-11 21:50:46.000000000 +0200
> > @@ -83,6 +83,7 @@
> >  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
> >  obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
> >  obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
> > +obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
> >  obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
> >  obj-$(CONFIG_SENSORS_VT1211)	+= vt1211.o
> >  obj-$(CONFIG_SENSORS_VT8231)	+= vt8231.o
> 
> 
> -- 
> Jean Delvare

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

Re: [lm-sensors] [PATCH 1/2] hwmon: Add driver tmp421 for TI's

[Andre Prendel]

On Mon, Jun 15, 2009 at 10:52:24PM +0200, Jean Delvare wrote:
> Hi Andre,

Hi again,

There is one remaining question, see below.

> On Mon, 15 Jun 2009 09:43:20 +0200, Andre Prendel wrote:
> > TI's TMP421/422/423 are I2C temperature sensor chips. These chips are
> > similar to TI's TMP401/411 chips, but with reduced functionality (only
> > temperature measurement). The chips have one local sensor and up to
> > three (TMP423) remote sensors.
> 
> Overall good, please see my review below.
> 
> > 
> > Notes:
> > Hans, I've dropped two checks in tmp421_detect(). These checks came
> > from tmp401 (copied by your students).
> > 
> > 1. Are reserved bits set to 0 in configuration register?
> > 2. Are unused bits in conversion rate register set to 0?
> > 
> > I haven't any experience with real chips, so my question: Do we need
> > these checks. Isn't the device id check enough? If so, I will bring
> > back these checks.
> 
> In favor of the checks:
> * I2C has no standard chip identification method, so just because a
>   chip match a few register checks doesn't mean it's the chip you were
>   looking for.
> * Your driver checks several addresses, which increases the risk of
>   false positives.
> 
> Against the checks:
> * Slow down driver loading.
> * I2C probing is restricted by classes, which should limit the risk of
>   false positives.
> 
> So there is no absolute rule, other than the fact that any false
> positive reported will result in an improved (and slower) detection
> routine to prevent it.
> 
> > 
> > Signed-off-by: Andre Prendel <andre.prendel@gmx.de>
> > ---
> >  Kconfig  |   10 +
> >  Makefile |    1 
> >  tmp421.c |  331 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 342 insertions(+)
> > 
> > Index: linux-2.6/drivers/hwmon/tmp421.c
> > =================================> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6/drivers/hwmon/tmp421.c	2009-06-12 22:13:34.000000000 +0200
> > @@ -0,0 +1,331 @@
> > +/* tmp421.c
> > + *
> > + * Copyright (C) 2009 Andre Prendel <andre.prendel@gmx.de>
> > + * Preliminary support by:
> > + * Melvin Rook, Raymond Ng
> > + *
> > + * 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 TMP421 SMBUS temperature sensor IC.
> 
> Spelling: SMBus.
> 
> > + * Supported models: TMP421, TMP422, TMP423
> > + */
> > +
> > +#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[] = { 0x2a, 0x4c, 0x4d, 0x4e, 0x4f,
> > +				       I2C_CLIENT_END };
> > +
> > +/* Insmod parameters */
> > +I2C_CLIENT_INSMOD_3(tmp421, tmp422, tmp423);
> > +
> > +/* The TMP421 registers */
> > +#define TMP421_STATUS_REG			0x08
> > +#define TMP421_CONFIG_REG_1			0x09
> > +#define TMP421_CONFIG_REG_2			0x0A
> 
> You don't seem to use TMP421_STATUS_REG nor TMP421_CONFIG_REG_2
> anywhere, so why define them?
> 
> > +#define TMP421_CONVERSION_RATE_REG		0x0B
> > +#define TMP421_MANUFACTURER_ID_REG		0xFE
> > +#define TMP421_DEVICE_ID_REG			0xFF
> > +
> > +static const u8 TMP421_TEMP_MSB[4]		= { 0x00, 0x01, 0x02, 0x03 };
> > +static const u8 TMP421_TEMP_LSB[4]		= { 0x10, 0x11, 0x12, 0x13 };
> > +
> > +/* Flags */
> > +#define TMP421_CONFIG_SHUTDOWN			0x40
> > +#define TMP421_CONFIG_RANGE			0x04
> > +
> > +/* Manufacturer / Device ID's */
> > +#define TMP421_MANUFACTURER_ID			0x55
> > +#define TMP421_DEVICE_ID			0x21
> > +#define TMP422_DEVICE_ID			0x22
> > +#define TMP423_DEVICE_ID			0x23
> > +
> > +static int tmp421_probe(struct i2c_client *client,
> > +			const struct i2c_device_id *id);
> > +static int tmp421_detect(struct i2c_client *client, int kind,
> > +			 struct i2c_board_info *info);
> > +static int tmp421_remove(struct i2c_client *client);
> > +static struct tmp421_data *tmp421_update_device(struct device *dev);
> 
> Please reorder the functions so that these forward declarations are no
> longer needed.
> 
> > +
> > +static const struct i2c_device_id tmp421_id[] = {
> > +	{ "tmp421", tmp421 },
> > +	{ "tmp422", tmp422 },
> > +	{ "tmp423", tmp423 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, tmp421_id);
> > +
> > +static struct i2c_driver tmp421_driver = {
> > +	.class = I2C_CLASS_HWMON,
> > +	.driver = {
> > +		.name	= "tmp421",
> > +	},
> > +	.probe = tmp421_probe,
> > +	.remove = tmp421_remove,
> > +	.id_table = tmp421_id,
> > +	.detect = tmp421_detect,
> > +	.address_data = &addr_data,
> > +};
> > +
> > +struct tmp421_data {
> > +	struct device *hwmon_dev;
> > +	struct mutex update_lock;
> > +	char valid;
> > +	unsigned long last_updated;
> > +	int kind;
> > +	u8 config;
> > +	s16 temp[4];
> > +};
> > +
> > +static int temp_from_s16(s16 reg)
> > +{
> > +	int temp = reg;
> > +
> > +	return (temp * 1000 + 128) / 256;
> > +}
> > +
> > +static int temp_from_u16(u16 reg)
> > +{
> > +	int temp = reg;
> > +
> > +	/* Add offset for extended temperature range. */
> > +	temp -= 64 * 256;
> > +
> > +	return (temp * 1000 + 128) / 256;
> > +}
> > +
> > +static ssize_t show_temp_value(struct device *dev,
> > +			       struct device_attribute *devattr, char *buf)
> > +{
> > +	int index = to_sensor_dev_attr(devattr)->index;
> > +	struct tmp421_data *data = tmp421_update_device(dev);
> > +	int temp;
> > +
> > +	if (data->config & TMP421_CONFIG_RANGE)
> > +		temp = temp_from_u16(data->temp[index]);
> > +	else
> > +		temp = temp_from_s16(data->temp[index]);
> 
> This lacks protection: you have no guarantee that data->config and
> data->temp[index] belong to the same read cycle.

The *data pointer comes from tmp421_update_device(dev). So this should
be from the same cycle, shouldn't be. I don't know how to
protect. Hardware access is protected in tmp421_update_device(). Maybe
I misunderstood something.

> > +
> > +	return sprintf(buf, "%d\n", temp);
> > +}
> > +
> > +static ssize_t show_fault(struct device *dev,
> > +			  struct device_attribute *devattr, char *buf)
> > +{
> > +	int index = to_sensor_dev_attr(devattr)->index;
> > +	struct tmp421_data *data = tmp421_update_device(dev);
> > +
> > +	/*
> > +	 * The OPEN bit signals a fault. This is bit 0 of the temperature
> > +	 * register (low byte).
> > +	 */
> > +	if (data->temp[index] & 0x01)
> > +		return sprintf(buf, "1\n");
> > +	else
> > +		return sprintf(buf, "0\n");
> > +}
> > +
> > +static struct sensor_device_attribute tmp421_attr[] = {
> > +	SENSOR_ATTR(temp1_input, 0444, show_temp_value, NULL, 0),
> 
> This is S_IRUGO.
> 
> > +	SENSOR_ATTR(temp2_input, 0444, show_temp_value, NULL, 1),
> > +	SENSOR_ATTR(temp2_fault, 0444, show_fault, NULL, 1),
> > +	SENSOR_ATTR(temp3_input, 0444, show_temp_value, NULL, 2),
> > +	SENSOR_ATTR(temp3_fault, 0444, show_fault, NULL, 2),
> > +	SENSOR_ATTR(temp4_input, 0444, show_temp_value, NULL, 3),
> > +	SENSOR_ATTR(temp4_fault, 0444, show_fault, NULL, 3),
> > +};
> > +
> > +static void tmp421_init_client(struct i2c_client *client)
> > +{
> > +	int config, config_orig;
> > +
> > +	/* Set the conversion rate to 2 Hz */
> > +	i2c_smbus_write_byte_data(client, TMP421_CONVERSION_RATE_REG, 0x05);
> > +
> > +	/* Start conversions (disable shutdown if necessary) */
> > +	config = i2c_smbus_read_byte_data(client, TMP421_CONFIG_REG_1);
> > +	if (config < 0) {
> > +		dev_warn(&client->dev, "Could not read configuration"
> 
> Missing space between string fragments.
> 
> > +			 "register (%d)\n", config);
> > +		return;
> > +	}
> 
> Is this really only a warning? if you can't read from the chip, this is
> a sign that something is really wrong, and it might be better to plain
> fail.
> 
> > +
> > +	config_orig = config;
> > +	config &= ~TMP421_CONFIG_SHUTDOWN;
> > +
> > +	if (config != config_orig)
> > +		i2c_smbus_write_byte_data(client, TMP421_CONFIG_REG_1, config);
> 
> Maybe log when you have to enable the monitoring?
> 
> > +}
> > +
> > +static int tmp421_detect(struct i2c_client *client, int kind,
> > +			 struct i2c_board_info *info)
> > +{
> > +	struct i2c_adapter *adapter = client->adapter;
> > +
> > +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> > +		return -ENODEV;
> > +
> > +	if (kind <= 0) {
> > +		u8 reg;
> > +
> > +		reg = i2c_smbus_read_byte_data(client,
> > +					       TMP421_MANUFACTURER_ID_REG);
> > +
> 
> Extra blank line.
> 
> > +		if (reg != TMP421_MANUFACTURER_ID)
> > +			return -ENODEV;
> > +
> > +		reg = i2c_smbus_read_byte_data(client,
> > +					       TMP421_DEVICE_ID_REG);
> > +
> 
> Extra blank line.
> 
> > +		switch (reg) {
> > +		case TMP421_DEVICE_ID:
> > +			kind = tmp421;
> > +			break;
> > +		case TMP422_DEVICE_ID:
> > +			kind = tmp422;
> > +			break;
> > +		case TMP423_DEVICE_ID:
> > +			kind = tmp423;
> > +			break;
> > +		default:
> > +			return -ENODEV;
> > +
> 
> Extra blank line.
> 
> > +		}
> > +	}
> > +	strlcpy(info->type, tmp421_id[kind - 1].name, I2C_NAME_SIZE);
> > +
> > +	return 0;
> > +}
> > +
> > +static int tmp421_probe(struct i2c_client *client,
> > +			const struct i2c_device_id *id)
> > +{
> > +	struct tmp421_data *data;
> > +	const char *names[] = { "TMP421", "TMP422", "TMP423" };
> > +	int i, err;
> > +
> > +	data = kzalloc(sizeof(struct tmp421_data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENODEV;
> 
> This would rather me -ENOMEM.
> 
> > +
> > +	i2c_set_clientdata(client, data);
> > +	mutex_init(&data->update_lock);
> > +	data->kind = id->driver_data;
> > +
> > +	tmp421_init_client(client);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(tmp421_attr); i++) {
> > +		err = device_create_file(&client->dev,
> > +					 &tmp421_attr[i].dev_attr);
> > +
> 
> Extra blank line.
> 
> > +		if (err)
> > +			goto exit_remove;
> > +
> > +		/*
> > +		 * Create only necessary sysfs files.
> > +		 * TMP421 up to one remote sensor
> > +		 * TMP422 up to two remote sensors
> > +		 * TMP423 up to three remote sensors.
> > +		 */
> > +		if (i >= data->kind * 2)
> 
> Huh, I don't like this, this is pretty fragile. Better have a struct
> field dedicated to the number of inputs and fill it based on the chip
> type. I also strongly suggests that you either rework your
> tmp421_attr[] data structure, either by splitting the attributes to
> per-sensor arrays, or by implementing an attribute group with
> a .is_visible callback (the latter would probably be cleaner.)
> 
> > +			break;
> > +	}
> > +
> > +	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_remove;
> > +	}
> > +
> > +	dev_info(&client->dev, "Detected TI %s chip\n", names[data->kind - 1]);
> 
> You didn't really detect anything in this function, so this message
> should either move to tmp421_detect(), or be reworded a bit.
> 
> > +	return 0;
> > +
> > +exit_remove:
> > +	tmp421_remove(client);
> > +	return err;
> > +}
> > +
> > +static int tmp421_remove(struct i2c_client *client)
> > +{
> > +	struct tmp421_data *data = i2c_get_clientdata(client);
> > +	int i;
> > +
> > +	if (data->hwmon_dev)
> > +		hwmon_device_unregister(data->hwmon_dev);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(tmp421_attr); i++)
> > +		device_remove_file(&client->dev, &tmp421_attr[i].dev_attr);
> > +
> 
> Please add:
> 	i2c_set_clientdata(client, NULL);
> to avoid leaving a dangling pointer behind.
> 
> > +	kfree(data);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct tmp421_data *tmp421_update_device(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct tmp421_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->config = i2c_smbus_read_byte_data(client,
> > +			TMP421_CONFIG_REG_1);
> > +
> > +		for (i = 0; i <= data->kind; i++) {
> > +			data->temp[i] = i2c_smbus_read_byte_data(client,
> > +				TMP421_TEMP_MSB[i]) << 8;
> > +			data->temp[i] |= i2c_smbus_read_byte_data(client,
> > +				TMP421_TEMP_LSB[i]);
> > +		}
> > +		data->last_updated = jiffies;
> > +		data->valid = 1;
> > +	}
> > +
> > +	mutex_unlock(&data->update_lock);
> > +
> > +	return data;
> > +}
> > +
> > +static int __init tmp421_init(void)
> > +{
> > +	return i2c_add_driver(&tmp421_driver);
> > +}
> > +
> > +static void __exit tmp421_exit(void)
> > +{
> > +	i2c_del_driver(&tmp421_driver);
> > +}
> > +
> > +MODULE_AUTHOR("Andre Prendel");
> 
> No e-mail address?
> 
> > +MODULE_DESCRIPTION("Texas Instruments TMP421/422/423 temperature sensor"
> > +		   "driver");
> 
> Missing space between string fragments.
> 
> > +MODULE_LICENSE("GPL");
> > +
> > +module_init(tmp421_init);
> > +module_exit(tmp421_exit);
> > Index: linux-2.6/drivers/hwmon/Kconfig
> > =================================> > --- linux-2.6.orig/drivers/hwmon/Kconfig	2009-06-11 21:50:46.000000000 +0200
> > +++ linux-2.6/drivers/hwmon/Kconfig	2009-06-11 21:50:46.000000000 +0200
> > @@ -797,6 +797,16 @@
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called tmp401.
> >  
> > +config SENSORS_TMP421
> > +	tristate "Texas Instruments TMP421 and compatibles"
> > +	depends on I2C && EXPERIMENTAL
> > +	help
> > +	  If you say yes here you get support for Texas Instruments TMP421
> > +	  temperature sensor chips.
> 
> Please also list the TMP422 and TMP423.
> 
> > +
> > +	  This driver can also be built as a module.  If so, the module
> > +	  will be called tmp421.
> > +
> >  config SENSORS_VIA686A
> >  	tristate "VIA686A"
> >  	depends on PCI
> > Index: linux-2.6/drivers/hwmon/Makefile
> > =================================> > --- linux-2.6.orig/drivers/hwmon/Makefile	2009-06-11 21:50:46.000000000 +0200
> > +++ linux-2.6/drivers/hwmon/Makefile	2009-06-11 21:50:46.000000000 +0200
> > @@ -83,6 +83,7 @@
> >  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
> >  obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
> >  obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
> > +obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
> >  obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
> >  obj-$(CONFIG_SENSORS_VT1211)	+= vt1211.o
> >  obj-$(CONFIG_SENSORS_VT8231)	+= vt8231.o
> 
> 
> -- 
> Jean Delvare

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

Re: [lm-sensors] [PATCH 1/2] hwmon: Add driver tmp421 for TI's

[Hans de Goede]

On 06/16/2009 09:58 AM, Andre Prendel wrote:
> On Mon, Jun 15, 2009 at 10:52:24PM +0200, Jean Delvare wrote:
>> Hi Andre,
>
> Hi again,
>
> There is one remaining question, see below.
>

See below for my answer.

<snip>

>>> +static ssize_t show_temp_value(struct device *dev,
>>> +			       struct device_attribute *devattr, char *buf)
>>> +{
>>> +	int index = to_sensor_dev_attr(devattr)->index;
>>> +	struct tmp421_data *data = tmp421_update_device(dev);
>>> +	int temp;
>>> +
>>> +	if (data->config&  TMP421_CONFIG_RANGE)
>>> +		temp = temp_from_u16(data->temp[index]);
>>> +	else
>>> +		temp = temp_from_s16(data->temp[index]);
>> This lacks protection: you have no guarantee that data->config and
>> data->temp[index] belong to the same read cycle.
>
> The *data pointer comes from tmp421_update_device(dev). So this should
> be from the same cycle, shouldn't be. I don't know how to
> protect. Hardware access is protected in tmp421_update_device(). Maybe
> I misunderstood something.
>

Yes, but 2 sysfs attributes can be read (by different processes / threads) at
the same time, this could lead to the following:

Proc 1:
Read temp1_value
tmp421_update_device() does not do anything as it isn't time yet
Check data->config
<task switch>

Proc 2:
Read temp1_value
tmp421_update_device() reads the entire chip
Calculate temp and return to userspace
<task switch>

Proc 1:
Read data->temp[index] and do calulation.
return temp to userspace

Now Proc 1 has used data->config and data->temp from 2
different read cycles. Since data->config should never
change this is a bit of a theoretical problem here. But
still you should protect against it, even if for no other reason
as to be good example code for people looking for an example.

The way to protect against this is like this:

	mutex_lock(&data->update_lock);
	if (data->config&  TMP421_CONFIG_RANGE)
		temp = temp_from_u16(data->temp[index]);
	else
		temp = temp_from_s16(data->temp[index]);
	mutex_unlock(&data->update_lock);


<snip>

Regards,

Hans

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

Re: [lm-sensors] [PATCH 1/2] hwmon: Add driver tmp421 for TI's

[Jean Delvare]

On Tue, 16 Jun 2009 09:58:31 +0200, Andre Prendel wrote:
> On Mon, Jun 15, 2009 at 10:52:24PM +0200, Jean Delvare wrote:
> > On Mon, 15 Jun 2009 09:43:20 +0200, Andre Prendel wrote:
> > > +static int temp_from_s16(s16 reg)
> > > +{
> > > +	int temp = reg;
> > > +
> > > +	return (temp * 1000 + 128) / 256;
> > > +}
> > > +
> > > +static int temp_from_u16(u16 reg)
> > > +{
> > > +	int temp = reg;
> > > +
> > > +	/* Add offset for extended temperature range. */
> > > +	temp -= 64 * 256;
> > > +
> > > +	return (temp * 1000 + 128) / 256;
> > > +}
> > > +
> > > +static ssize_t show_temp_value(struct device *dev,
> > > +			       struct device_attribute *devattr, char *buf)
> > > +{
> > > +	int index = to_sensor_dev_attr(devattr)->index;
> > > +	struct tmp421_data *data = tmp421_update_device(dev);
> > > +	int temp;
> > > +
> > > +	if (data->config & TMP421_CONFIG_RANGE)
> > > +		temp = temp_from_u16(data->temp[index]);
> > > +	else
> > > +		temp = temp_from_s16(data->temp[index]);
> > 
> > This lacks protection: you have no guarantee that data->config and
> > data->temp[index] belong to the same read cycle.
> 
> The *data pointer comes from tmp421_update_device(dev). So this should
> be from the same cycle, shouldn't be. I don't know how to
> protect. Hardware access is protected in tmp421_update_device(). Maybe
> I misunderstood something.

You can have concurrent accesses to the sysfs attribute files. Let's
say you have two files being accessed, the first access when the cache
is still valid (tmp421_update_device is a no-op) and the second when
the cache just wore off and needs to be refreshed (tmp421_update_device
reads from registers).

The first access will test data->config based on the cached value. At
this point, the second access could cause the cache to be refreshed. I
agree this is all highly unlikely, but in theory this could still
happen. Then the first access will use data->temp[index], from the
refreshed cache. If data->config changed meanwhile, then you just
called the wrong conversion function for the temperature value.

It's pretty easy to fix:

	mutex_lock(&data->update_lock);
	if (data->config & TMP421_CONFIG_RANGE)
		temp = temp_from_u16(data->temp[index]);
	else
		temp = temp_from_s16(data->temp[index]);
	mutex_unlock(&data->update_lock);

This is sufficient to prevent a cache update while you're using the
cached values.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH 1/2] hwmon: Add driver tmp421 for TI's

[Andre Prendel]

On Tue, Jun 16, 2009 at 10:10:27AM +0200, Jean Delvare wrote:
> On Tue, 16 Jun 2009 09:58:31 +0200, Andre Prendel wrote:
> > On Mon, Jun 15, 2009 at 10:52:24PM +0200, Jean Delvare wrote:
> > > On Mon, 15 Jun 2009 09:43:20 +0200, Andre Prendel wrote:
> > > > +static int temp_from_s16(s16 reg)
> > > > +{
> > > > +	int temp = reg;
> > > > +
> > > > +	return (temp * 1000 + 128) / 256;
> > > > +}
> > > > +
> > > > +static int temp_from_u16(u16 reg)
> > > > +{
> > > > +	int temp = reg;
> > > > +
> > > > +	/* Add offset for extended temperature range. */
> > > > +	temp -= 64 * 256;
> > > > +
> > > > +	return (temp * 1000 + 128) / 256;
> > > > +}
> > > > +
> > > > +static ssize_t show_temp_value(struct device *dev,
> > > > +			       struct device_attribute *devattr, char *buf)
> > > > +{
> > > > +	int index = to_sensor_dev_attr(devattr)->index;
> > > > +	struct tmp421_data *data = tmp421_update_device(dev);
> > > > +	int temp;
> > > > +
> > > > +	if (data->config & TMP421_CONFIG_RANGE)
> > > > +		temp = temp_from_u16(data->temp[index]);
> > > > +	else
> > > > +		temp = temp_from_s16(data->temp[index]);
> > > 
> > > This lacks protection: you have no guarantee that data->config and
> > > data->temp[index] belong to the same read cycle.
> > 
> > The *data pointer comes from tmp421_update_device(dev). So this should
> > be from the same cycle, shouldn't be. I don't know how to
> > protect. Hardware access is protected in tmp421_update_device(). Maybe
> > I misunderstood something.
> 
> You can have concurrent accesses to the sysfs attribute files. Let's
> say you have two files being accessed, the first access when the cache
> is still valid (tmp421_update_device is a no-op) and the second when
> the cache just wore off and needs to be refreshed (tmp421_update_device
> reads from registers).
> 
> The first access will test data->config based on the cached value. At
> this point, the second access could cause the cache to be refreshed. I
> agree this is all highly unlikely, but in theory this could still
> happen. Then the first access will use data->temp[index], from the
> refreshed cache. If data->config changed meanwhile, then you just
> called the wrong conversion function for the temperature value.
> 
> It's pretty easy to fix:
> 
> 	mutex_lock(&data->update_lock);
> 	if (data->config & TMP421_CONFIG_RANGE)
> 		temp = temp_from_u16(data->temp[index]);
> 	else
> 		temp = temp_from_s16(data->temp[index]);
> 	mutex_unlock(&data->update_lock);
> 
> This is sufficient to prevent a cache update while you're using the
> cached values.

Hans, Jean, thanks for the explanation. I'll fix it too.

Andre

> 
> -- 
> Jean Delvare

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