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
