Hi Hans,
On Sat, 14 Jun 2008 17:15:13 +0200, Hans de Goede wrote:
> This is a new hwmon driver for TI's TMP401 temperature sensor IC. This driver
> was written on behalf of an embedded systems vendor under the
> linuxdriverproject.
>
> It has been tested using a TI TMP401 sample attached to a i2c-tiny-usb adapter.
> Which was provided by Till Harbaum, many thanks to him for this!
>
> Signed-off-by: Hans de Goede <j.w.r.degoede at hhs.nl>
Review:
> diff -up vanilla-2.6.26-rc5-git2/drivers/hwmon/Kconfig~ vanilla-2.6.26-rc5-git2/drivers/hwmon/Kconfig
> --- vanilla-2.6.26-rc5-git2/drivers/hwmon/Kconfig~ 2008-06-14 16:51:14.000000000 +0200
> +++ vanilla-2.6.26-rc5-git2/drivers/hwmon/Kconfig 2008-06-14 16:51:14.000000000 +0200
> @@ -633,6 +633,16 @@ config SENSORS_THMC50
> This driver can also be built as a module. If so, the module
> will be called thmc50.
>
> +config SENSORS_TMP401
> + tristate "Texas Instruments TMP401"
> + depends on I2C && EXPERIMENTAL
> + help
> + If you say yes here you get support for Texas Instruments TMP401
> + temperature sensor chips.
> +
> + This driver can also be built as a module. If so, the module
> + will be called tmp401.
> +
> config SENSORS_VIA686A
> tristate "VIA686A"
> depends on PCI
> diff -up vanilla-2.6.26-rc5-git2/drivers/hwmon/Makefile~ vanilla-2.6.26-rc5-git2/drivers/hwmon/Makefile
> --- vanilla-2.6.26-rc5-git2/drivers/hwmon/Makefile~ 2008-06-14 16:52:19.000000000 +0200
> +++ vanilla-2.6.26-rc5-git2/drivers/hwmon/Makefile 2008-06-14 16:52:19.000000000 +0200
> @@ -66,6 +66,7 @@ obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc4
> obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o
> obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
> obj-$(CONFIG_SENSORS_THMC50) += thmc50.o
> +obj-$(CONFIG_SENSORS_TMP401) += tmp401.o
> obj-$(CONFIG_SENSORS_VIA686A) += via686a.o
> obj-$(CONFIG_SENSORS_VT1211) += vt1211.o
> obj-$(CONFIG_SENSORS_VT8231) += vt8231.o
> diff -up vanilla-2.6.26-rc5-git2/drivers/hwmon/tmp401.c~ vanilla-2.6.26-rc5-git2/drivers/hwmon/tmp401.c
> --- vanilla-2.6.26-rc5-git2/drivers/hwmon/tmp401.c~ 2008-06-14 17:04:10.000000000 +0200
> +++ vanilla-2.6.26-rc5-git2/drivers/hwmon/tmp401.c 2008-06-14 17:10:13.000000000 +0200
> @@ -0,0 +1,566 @@
> +/* tmp401.c
> + *
> + * Copyright (C) 2007 Hans de Goede <j.w.r.degoede at hhs.nl>
You could add 2008 now.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +/*
> + * Driver for the Texas Instruments TMP401 SMBUS temperature sensor IC.
> + *
> + * Note this IC is in some aspect similar to the lm90, but it has quite a
LM90
> + * few differences too, for example the local temp has a higher resolution
> + * and thus has 16 bits registers for its value and limit instead of 8 bits.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/sysfs.h>
> +
> +/* Addresses to scan */
> +static unsigned short normal_i2c[] = { 0x4c, I2C_CLIENT_END };
Could be made const.
> +
> +/* Insmod parameters */
> +I2C_CLIENT_INSMOD_1(tmp401);
> +
> +
> +/*
> + * The TMP401 registers, note some registers have different addresses for
> + * reading and writing
> + */
> +#define TMP401_STATUS 0x02
> +#define TMP401_CONFIG_READ 0x03
> +#define TMP401_CONFIG_WRITE 0x09
> +#define TMP401_CONVERSION_RATE_READ 0x04
> +#define TMP401_CONVERSION_RATE_WRITE 0x0A
> +#define TMP401_ONE_SHOT_START_WRITE 0x0F
This one is never used.
> +#define TMP401_RESOLUTION 0x1A
Same for this one.
> +#define TMP401_TEMP_CRIT_HYST 0x21
> +#define TMP401_CONSECUTIVE_ALERT 0x22
> +#define TMP401_MANUFACTURER_ID_REG 0xFE
> +#define TMP401_DEVICE_ID_REG 0xFF
> +
> +static const u8 TMP401_TEMP_MSB[2] = { 0x00, 0x01 };
> +static const u8 TMP401_TEMP_LSB[2] = { 0x15, 0x10 };
> +static const u8 TMP401_TEMP_LOW_LIMIT_MSB_READ[2] = { 0x06, 0x08 };
> +static const u8 TMP401_TEMP_LOW_LIMIT_MSB_WRITE[2] = { 0x0C, 0x0E };
> +static const u8 TMP401_TEMP_LOW_LIMIT_LSB[2] = { 0x17, 0x14 };
> +static const u8 TMP401_TEMP_HIGH_LIMIT_MSB_READ[2] = { 0x05, 0x07 };
> +static const u8 TMP401_TEMP_HIGH_LIMIT_MSB_WRITE[2] = { 0x0B, 0x0D };
> +static const u8 TMP401_TEMP_HIGH_LIMIT_LSB[2] = { 0x16, 0x13 };
> +/* These are called the THERM limit / hysteresis / mask in the datasheets */
> +static const u8 TMP401_TEMP_CRIT_LIMIT[2] = { 0x20, 0x19 };
datasheet
> +
> +/* Bit masks */
> +#define TMP401_CONFIG_RANGE_MASK 0x04
> +#define TMP401_CONFIG_SHUTDOWN_MASK 0x40
> +#define TMP401_STATUS_LOCAL_CRIT_MASK 0x01
> +#define TMP401_STATUS_REMOTE_CRIT_MASK 0x02
> +#define TMP401_STATUS_REMOTE_OPEN_MASK 0x04
> +#define TMP401_STATUS_REMOTE_LOW_MASK 0x08
> +#define TMP401_STATUS_REMOTE_HIGH_MASK 0x10
> +#define TMP401_STATUS_LOCAL_LOW_MASK 0x20
> +#define TMP401_STATUS_LOCAL_HIGH_MASK 0x40
These are flags, not masks.
> +
> +/* Manufacturer / Device ID's */
> +#define TMP401_MANUFACTURER_ID 0x55
> +#define TMP401_DEVICE_ID 0x11
> +
> +/* our driver name */
> +#define TMP401_NAME "tmp401"
> +
> +/*
> + * Functions declarations
> + */
> +
> +static void tmp401_init_client(struct i2c_client *client);
If you move the code of this function, you can easily get away with this
forward declaration.
> +static int tmp401_attach_adapter(struct i2c_adapter *adapter);
> +static int tmp401_detach_client(struct i2c_client *client);
> +static struct tmp401_data *tmp401_update_device(struct device *dev);
> +
> +/*
> + * Driver data (common to all clients)
> + */
> +
> +static struct i2c_driver tmp401_driver = {
> + .driver = {
> + .name = TMP401_NAME,
> + },
> + .attach_adapter = tmp401_attach_adapter,
> + .detach_client = tmp401_detach_client,
> +};
This needs to be converted to the new i2c device driver binding. I can
take care of it if you do not have the time, just let me know.
> +
> +/*
> + * Client data (each client gets its own)
> + */
> +
> +struct tmp401_data {
> + struct i2c_client client;
> + struct device *hwmon_dev;
> + struct mutex update_lock;
> + char valid; /* zero until following fields are valid */
> + unsigned long last_updated; /* in jiffies */
> +
> + /* register values */
> + u8 status;
> + u8 config;
> + u16 temp[2];
> + u16 temp_low[2];
> + u16 temp_high[2];
> + u8 temp_crit[2];
> + u8 temp_crit_hyst;
> +};
> +
> +/*
> + * Sysfs attr show / store functions
> + */
> +
> +static int tmp401_register_to_temp(u16 reg, u8 config)
> +{
> + int temp = reg;
> +
> + if (config & TMP401_CONFIG_RANGE_MASK)
> + temp -= 64 * 256;
> +
> + return (temp * 625) / 160;
> +}
> +
> +static u16 tmp401_temp_to_register(long temp, u8 config)
> +{
> + if (config & TMP401_CONFIG_RANGE_MASK) {
> + temp = SENSORS_LIMIT(temp, -64000, 191000);
> + temp += 64000;
> + } else
> + temp = SENSORS_LIMIT(temp, 0, 127000);
> +
> + return (temp * 160) / 625;
You can get rounding with:
return (temp * 160 + 625 / 2) / 625;
Even though it probably doesn't matter too much for such high resolution
sensors.
> +}
> +
> +static int tmp401_crit_register_to_temp(u8 reg, u8 config)
> +{
> + int temp = reg;
> +
> + if (config & TMP401_CONFIG_RANGE_MASK)
> + temp -= 64;
> +
> + return temp * 1000;
> +}
> +
> +static u8 tmp401_temp_crit_to_register(long temp, u8 config)
This function name isn't consistent with the previous one. It should be
either tmp401_crit_temp_to_register or tmp401_temp_to_crit_register.
> +{
> + if (config & TMP401_CONFIG_RANGE_MASK) {
> + temp = SENSORS_LIMIT(temp, -64000, 191000);
> + temp += 64000;
> + } else
> + temp = SENSORS_LIMIT(temp, 0, 127000);
> +
> + return temp / 1000;
Here rounding is important:
return (temp + 500) / 1000;
> +}
> +
> +static ssize_t show_temp_value(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + int index = to_sensor_dev_attr(devattr)->index;
> + struct tmp401_data *data = tmp401_update_device(dev);
> +
> + return sprintf(buf, "%d\n",
> + tmp401_register_to_temp(data->temp[index], data->config));
> +}
> +
> +static ssize_t show_temp_min(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + int index = to_sensor_dev_attr(devattr)->index;
> + struct tmp401_data *data = tmp401_update_device(dev);
> +
> + return sprintf(buf, "%d\n",
> + tmp401_register_to_temp(data->temp_low[index], data->config));
> +}
> +
> +static ssize_t show_temp_max(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + int index = to_sensor_dev_attr(devattr)->index;
> + struct tmp401_data *data = tmp401_update_device(dev);
> +
> + return sprintf(buf, "%d\n",
> + tmp401_register_to_temp(data->temp_high[index], data->config));
> +}
> +
> +static ssize_t show_temp_crit(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + int index = to_sensor_dev_attr(devattr)->index;
> + struct tmp401_data *data = tmp401_update_device(dev);
> +
> + return sprintf(buf, "%d\n",
> + tmp401_crit_register_to_temp(data->temp_crit[index],
> + data->config));
> +}
> +
> +static ssize_t show_temp_crit_hyst(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + int temp, index = to_sensor_dev_attr(devattr)->index;
> + struct tmp401_data *data = tmp401_update_device(dev);
> +
> + mutex_lock(&data->update_lock);
> + temp = tmp401_crit_register_to_temp(data->temp_crit[index],
> + data->config);
> + temp -= data->temp_crit_hyst * 1000;
> + mutex_unlock(&data->update_lock);
> +
> + return sprintf(buf, "%d\n", temp);
> +}
> +
> +static ssize_t show_status(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + int mask = to_sensor_dev_attr(devattr)->index;
> + struct tmp401_data *data = tmp401_update_device(dev);
> +
> + if (data->status & mask)
> + return sprintf(buf, "1\n");
> + else
> + return sprintf(buf, "0\n");
> +}
> +
> +static ssize_t store_temp_min(struct device *dev, struct device_attribute
> + *devattr, const char *buf, size_t count)
> +{
> + int index = to_sensor_dev_attr(devattr)->index;
> + struct tmp401_data *data = tmp401_update_device(dev);
> + long v;
Please avoid 1-letter variable names (except for loop indexes.)
> + u16 reg;
> +
> + if (strict_strtol(buf, 10, &v))
> + return -EINVAL;
> +
> + reg = tmp401_temp_to_register(v, data->config);
> +
> + mutex_lock(&data->update_lock);
> +
> + i2c_smbus_write_byte_data(&data->client,
> + TMP401_TEMP_LOW_LIMIT_MSB_WRITE[index], reg >> 8);
> + i2c_smbus_write_byte_data(&data->client,
> + TMP401_TEMP_LOW_LIMIT_LSB[index], reg & 0xFF);
> +
> + data->temp_low[index] = reg;
> +
> + mutex_unlock(&data->update_lock);
> +
> + return count;
> +}
> +
> +static ssize_t store_temp_max(struct device *dev, struct device_attribute
> + *devattr, const char *buf, size_t count)
> +{
> + int index = to_sensor_dev_attr(devattr)->index;
> + struct tmp401_data *data = tmp401_update_device(dev);
> + long v;
> + u16 reg;
> +
> + if (strict_strtol(buf, 10, &v))
> + return -EINVAL;
> +
> + reg = tmp401_temp_to_register(v, data->config);
> +
> + mutex_lock(&data->update_lock);
> +
> + i2c_smbus_write_byte_data(&data->client,
> + TMP401_TEMP_HIGH_LIMIT_MSB_WRITE[index], reg >> 8);
> + i2c_smbus_write_byte_data(&data->client,
> + TMP401_TEMP_HIGH_LIMIT_LSB[index], reg & 0xFF);
> +
> + data->temp_high[index] = reg;
> +
> + mutex_unlock(&data->update_lock);
> +
> + return count;
> +}
> +
> +static ssize_t store_temp_crit(struct device *dev, struct device_attribute
> + *devattr, const char *buf, size_t count)
> +{
> + int index = to_sensor_dev_attr(devattr)->index;
> + struct tmp401_data *data = tmp401_update_device(dev);
> + long v;
> + u8 reg;
> +
> + if (strict_strtol(buf, 10, &v))
> + return -EINVAL;
> +
> + reg = tmp401_temp_crit_to_register(v, data->config);
> +
> + mutex_lock(&data->update_lock);
> +
> + i2c_smbus_write_byte_data(&data->client,
> + TMP401_TEMP_CRIT_LIMIT[index], reg);
> +
> + data->temp_crit[index] = reg;
> +
> + mutex_unlock(&data->update_lock);
> +
> + return count;
> +}
> +
> +static ssize_t store_temp_crit_hyst(struct device *dev, struct device_attribute
> + *devattr, const char *buf, size_t count)
> +{
> + int temp, index = to_sensor_dev_attr(devattr)->index;
> + struct tmp401_data *data = tmp401_update_device(dev);
> + long v;
> + u8 reg;
> +
> + if (strict_strtol(buf, 10, &v))
> + return -EINVAL;
> +
> + if (data->config & TMP401_CONFIG_RANGE_MASK)
> + v = SENSORS_LIMIT(v, -64000, 191000);
> + else
> + v = SENSORS_LIMIT(v, 0, 127000);
> +
> + mutex_lock(&data->update_lock);
> + temp = tmp401_crit_register_to_temp(data->temp_crit[index],
> + data->config);
> + v = SENSORS_LIMIT(v, temp - 255000, temp);
> + reg = (temp - v) / 1000;
This lacks rounding.
> +
> + i2c_smbus_write_byte_data(&data->client,
> + TMP401_TEMP_CRIT_HYST, reg);
> +
> + data->temp_crit_hyst = reg;
> +
> + mutex_unlock(&data->update_lock);
> +
> + return count;
> +}
> +
> +static struct sensor_device_attribute tmp401_attr[] = {
> + SENSOR_ATTR(temp1_input, 0444, show_temp_value, NULL, 0),
> + SENSOR_ATTR(temp1_min, 0644, show_temp_min, store_temp_min, 0),
> + SENSOR_ATTR(temp1_max, 0644, show_temp_max, store_temp_max, 0),
> + SENSOR_ATTR(temp1_crit, 0644, show_temp_crit, store_temp_crit, 0),
> + SENSOR_ATTR(temp1_crit_hyst, 0644, show_temp_crit_hyst,
> + store_temp_crit_hyst, 0),
> + SENSOR_ATTR(temp1_min_alarm, 0444, show_status, NULL,
> + TMP401_STATUS_LOCAL_LOW_MASK),
> + SENSOR_ATTR(temp1_max_alarm, 0444, show_status, NULL,
> + TMP401_STATUS_LOCAL_HIGH_MASK),
> + SENSOR_ATTR(temp1_crit_alarm, 0444, show_status, NULL,
> + TMP401_STATUS_LOCAL_CRIT_MASK),
> + SENSOR_ATTR(temp2_input, 0444, show_temp_value, NULL, 1),
> + SENSOR_ATTR(temp2_min, 0644, show_temp_min, store_temp_min, 1),
> + SENSOR_ATTR(temp2_max, 0644, show_temp_max, store_temp_max, 1),
> + SENSOR_ATTR(temp2_crit, 0644, show_temp_crit, store_temp_crit, 1),
> + SENSOR_ATTR(temp2_crit_hyst, 0444, show_temp_crit_hyst, NULL, 1),
> + SENSOR_ATTR(temp2_fault, 0444, show_status, NULL,
> + TMP401_STATUS_REMOTE_OPEN_MASK),
> + SENSOR_ATTR(temp2_min_alarm, 0444, show_status, NULL,
> + TMP401_STATUS_REMOTE_LOW_MASK),
> + SENSOR_ATTR(temp2_max_alarm, 0444, show_status, NULL,
> + TMP401_STATUS_REMOTE_HIGH_MASK),
> + SENSOR_ATTR(temp2_crit_alarm, 0444, show_status, NULL,
> + TMP401_STATUS_REMOTE_CRIT_MASK),
> +};
At page 11, in section "STATUS REGISTER", the datasheet suggests that
the hysteresis value also applies to the local and remote high limits.
Did you test that? If this is true then you should add files
temp1_max_hyst and temp2_max_hyst (read-only.)
> +
> +/*
> + * Real code
This old comment of mine is really bad... As if the rest of the code
wasn't real? Maybe you can come up with something better.
> + */
> +
> +static int tmp401_detect(struct i2c_adapter *adapter, int address, int kind)
> +{
> + int i, err = 0;
> + struct i2c_client *client;
> + struct tmp401_data *data;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> + return 0;
> +
> + /* OK. For now, we presume we have a valid client. We now create the
> + * client structure, even though we cannot fill it completely yet.
> + * But it allows us to access i2c_smbus_read_byte_data. */
> + if (!(data = kzalloc(sizeof(struct tmp401_data), GFP_KERNEL)))
checkpatch.pl suggests that the following syntax is preferred:
data = kzalloc(sizeof(struct tmp401_data), GFP_KERNEL);
if (!data)
I've changed almost all the hwmon drivers to that syntax already.
> + return -ENOMEM;
> +
> + client = &data->client;
> + i2c_set_clientdata(client, data);
> + client->addr = address;
> + client->adapter = adapter;
> + client->driver = &tmp401_driver;
> + mutex_init(&data->update_lock);
> +
> + /* Detect & Identify the chip */
and identify
> + if (kind <= 0) {
> + u8 manufacturer_id;
> + u8 device_id;
> +
> + manufacturer_id = i2c_smbus_read_byte_data(client,
> + TMP401_MANUFACTURER_ID_REG);
> + device_id = i2c_smbus_read_byte_data(client,
> + TMP401_DEVICE_ID_REG);
> +
> + if (manufacturer_id != TMP401_MANUFACTURER_ID ||
> + device_id != TMP401_DEVICE_ID)
> + goto exit_free;
> + }
The sensors-detect code also checks for the conversion rate register
value and unused bits in the configuration register. Maybe you can do
the same for more robust detection?
> +
> + /* Tell the I2C layer a new client has arrived */
> + if ((err = i2c_attach_client(client)))
Same as above, that would be:
err = i2c_attach_client(client);
if (err)
> + goto exit_free;
> +
> + /* Initialize the TMP401 chip */
> + tmp401_init_client(client);
> +
> + /* Register sysfs hooks */
> + for (i = 0; i < ARRAY_SIZE(tmp401_attr); i++) {
> + err = device_create_file(&client->dev,
> + &tmp401_attr[i].dev_attr);
> + if (err)
> + goto exit_detach;
> + }
Why don't you create an attribute group? You wouldn't have to loop over
it yourself.
> +
> + data->hwmon_dev = hwmon_device_register(&client->dev);
> + if (IS_ERR(data->hwmon_dev)) {
> + err = PTR_ERR(data->hwmon_dev);
> + data->hwmon_dev = NULL;
> + goto exit_detach;
> + }
> +
> + printk(KERN_INFO TMP401_NAME ": Detected TI TMP401 chip\n");
Please use dev_info() instead.
> +
> + return 0;
> +
> +exit_detach:
> + tmp401_detach_client(client); /* will also free data for us */
> + return err;
> +
> +exit_free:
> + kfree(data);
> + return err;
> +}
> +
> +static void tmp401_init_client(struct i2c_client *client)
> +{
> + int config, config_orig;
> +
> + /* Set the conversion rate to 2 Hz */
> + i2c_smbus_write_byte_data(client, TMP401_CONVERSION_RATE_WRITE, 5);
> +
> + /* Start conversions (disable shutdown if necessary) */
> + config = i2c_smbus_read_byte_data(client, TMP401_CONFIG_READ);
> + if (config < 0) {
> + dev_warn(&client->dev, "Initialization failed!\n");
> + return;
> + }
> +
> + config_orig = config;
> + config &= ~TMP401_CONFIG_SHUTDOWN_MASK;
> +
> + if (config != config_orig)
> + i2c_smbus_write_byte_data(client, TMP401_CONFIG_WRITE, config);
> +}
> +
> +static int tmp401_attach_adapter(struct i2c_adapter *adapter)
> +{
> + if (!(adapter->class & I2C_CLASS_HWMON))
> + return 0;
> + return i2c_probe(adapter, &addr_data, tmp401_detect);
> +}
> +
> +static int tmp401_detach_client(struct i2c_client *client)
> +{
> + struct tmp401_data *data = i2c_get_clientdata(client);
> + int i, err;
> +
> + /* Check if registered in case we're called from tmp401_detect
> + to cleanup after an error */
> + if (data->hwmon_dev)
> + hwmon_device_unregister(data->hwmon_dev);
> +
> + for (i = 0; i < ARRAY_SIZE(tmp401_attr); i++)
> + device_remove_file(&client->dev, &tmp401_attr[i].dev_attr);
> +
> + if ((err = i2c_detach_client(client)))
checkpatch warns on this as well.
> + return err;
> +
> + kfree(data);
> + return 0;
> +}
> +
> +static struct tmp401_data *tmp401_update_device(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct tmp401_data *data = i2c_get_clientdata(client);
> + int i;
> +
> + mutex_lock(&data->update_lock);
> +
> + if (time_after(jiffies, data->last_updated + 2 * HZ) || !data->valid) {
Why 2 seconds? According to the datasheet, the conversion takes 115 ms
per channel, so 230 ms total. And you've set the conversion rate to 2
Hz. So it would seem reasonable to let the user query the temperature
values every second.
> + data->status = i2c_smbus_read_byte_data(client, TMP401_STATUS);
> + data->config = i2c_smbus_read_byte_data(client,
> + TMP401_CONFIG_READ);
> + for (i = 0; i < 2; i++) {
> + data->temp[i] = i2c_smbus_read_byte_data(client,
> + TMP401_TEMP_MSB[i]) << 8;
> + data->temp[i] |= i2c_smbus_read_byte_data(client,
> + TMP401_TEMP_LSB[i]);
I would welcome a comment explaining that the high byte must be read
first and the low byte must be read immediately after, to ensure that
both bytes belong to the same measurement.
> + data->temp_low[i] = i2c_smbus_read_byte_data(client,
> + TMP401_TEMP_LOW_LIMIT_MSB_READ[i]) << 8;
> + data->temp_low[i] |= i2c_smbus_read_byte_data(client,
> + TMP401_TEMP_LOW_LIMIT_LSB[i]);
> + data->temp_high[i] = i2c_smbus_read_byte_data(client,
> + TMP401_TEMP_HIGH_LIMIT_MSB_READ[i]) << 8;
> + data->temp_high[i] |= i2c_smbus_read_byte_data(client,
> + TMP401_TEMP_HIGH_LIMIT_LSB[i]);
> + data->temp_crit[i] = i2c_smbus_read_byte_data(client,
> + TMP401_TEMP_CRIT_LIMIT[i]);
> + }
> +
> + data->temp_crit_hyst = i2c_smbus_read_byte_data(client,
> + TMP401_TEMP_CRIT_HYST);
> +
> + data->last_updated = jiffies;
> + data->valid = 1;
> + }
> +
> + mutex_unlock(&data->update_lock);
> +
> + return data;
> +}
> +
> +static int __init tmp401_init(void)
> +{
> + return i2c_add_driver(&tmp401_driver);
> +}
> +
> +static void __exit tmp401_exit(void)
> +{
> + i2c_del_driver(&tmp401_driver);
> +}
> +
> +MODULE_AUTHOR("Hans de Goede <j.w.r.degoede at hhs.nl>");
> +MODULE_DESCRIPTION("Texas Instruments TMP401 temperature sensor driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(tmp401_init);
> +module_exit(tmp401_exit);
Overall the drivers looks good to me.
Can we have a Documentation/hwmon/tmp401 file?
As a side note, it seems to me that your driver also supports the TI
TMP411 chip. Both chips have the same device ID and seem to be
essentially compatible. The only differences I can see are:
* The TMP411 lacks the one-shot mode.
* The TMP411 stores the minimum and maximum temperatures during runtime.
* The TMP411 can be reset by software.
--
Jean Delvare
