[PATCH 1/2] hwmon: Add driver tmp421 for TI's TMP421/422/423 sensor chips

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 at 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 at 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



[Index of Archives]     [Linux Kernel]     [Linux Hardware Monitoring]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux