Hi Sean, On Fri, 13 Jun 2008 22:12:21 -0400, Sean MacLennan wrote: > This is a driver for the ad7414 temperature chip. > > The driver has been through at least two reviews that I know of. It is > also in production use on the PIKA Warp Appliance. I'm a bit confused now... This version of the driver doesn't look as good as the one you submitted on April 30th: http://lists.lm-sensors.org/pipermail/lm-sensors/2008-April/022999.html That one was properly using s16 and s8 for temperature values, and was using dynamic sysfs callbacks to avoid some macro-generated functions (for the alarms flag in particular.) The patch that you just resubmitted has the macro-generated functions back, u8 for temperature limits (so I guess that writing negative temperature limits is broken again), alarms flags have non-standard names so libsensors won't see them, etc. Can you please clarify? I suspect that you didn't modify the right version of the driver before resubmitting. > > Cheers, > Sean > > Signed-off-by: Sean MacLennan <smaclennan at pikatech.com> > --- > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 00ff533..3c21d57 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -57,6 +57,16 @@ config SENSORS_ABITUGURU3 > This driver can also be built as a module. If so, the module > will be called abituguru3. > > +config SENSORS_AD7414 > + tristate "Analog Devices AD7414" > + depends on I2C && EXPERIMENTAL > + help > + If you say yes here you get support for the Analog Devices > + AD7414 temperature monitoring chip. > + > + This driver can also be built as a module. If so, the module > + will be called ad7414. > + > config SENSORS_AD7418 > tristate "Analog Devices AD7416, AD7417 and AD7418" > depends on I2C && EXPERIMENTAL > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index d098677..7943e5c 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -15,6 +15,7 @@ obj-$(CONFIG_SENSORS_W83791D) += w83791d.o > > obj-$(CONFIG_SENSORS_ABITUGURU) += abituguru.o > obj-$(CONFIG_SENSORS_ABITUGURU3)+= abituguru3.o > +obj-$(CONFIG_SENSORS_AD7414) += ad7414.o > obj-$(CONFIG_SENSORS_AD7418) += ad7418.o > obj-$(CONFIG_SENSORS_ADM1021) += adm1021.o > obj-$(CONFIG_SENSORS_ADM1025) += adm1025.o > diff --git a/drivers/hwmon/ad7414.c b/drivers/hwmon/ad7414.c > new file mode 100644 > index 0000000..e357f4f > --- /dev/null > +++ b/drivers/hwmon/ad7414.c > @@ -0,0 +1,259 @@ > +/* > + * An hwmon driver for the Analog Devices AD7414 > + * > + * Copyright 2006 Stefan Roese <sr at denx.de>, DENX Software Engineering > + * > + * Copyright (c) 2008 PIKA Technologies > + * Sean MacLennan <smaclennan at pikatech.com> > + * > + * Copyright (c) 2008 Spansion Inc. > + * Frank Edelhaeuser <frank.edelhaeuser at spansion.com> > + * (converted to "new style" I2C driver model, removed checkpatch.pl warnings) > + * > + * Based on ad7418.c > + * Copyright 2006 Tower Technologies, Alessandro Zummo <a.zummo at towertech.it> > + * > + * 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. > + */ > + > +#include <linux/module.h> > +#include <linux/jiffies.h> > +#include <linux/i2c.h> > +#include <linux/hwmon.h> > +#include <linux/err.h> > +#include <linux/mutex.h> > +#include <linux/delay.h> > + > + > +#define DRV_VERSION "0.3" > + > +/* straight from the datasheet */ > +#define AD7414_TEMP_MIN (-55000) > +#define AD7414_TEMP_MAX 125000 > + > +/* AD7414 registers */ > +#define AD7414_REG_TEMP 0x00 > +#define AD7414_REG_CONF 0x01 > +#define AD7414_REG_T_HIGH 0x02 > +#define AD7414_REG_T_LOW 0x03 > + > + > +struct ad7414_data { > + struct i2c_client client; > + struct device *dev; > + struct mutex lock; /* atomic read data updates */ > + char valid; /* !=0 if following fields are valid */ > + unsigned long last_updated; /* In jiffies */ > + u16 temp_input; /* Register values */ > + u8 temp_max; > + u8 temp_min; > + u8 temp_alert; > + u8 temp_max_flag; > + u8 temp_min_flag; > +}; > + > +/* > + * TEMP: 0.001C/bit (-55C to +125C) > + * REG: (0.5C/bit, two's complement) << 7 > + */ > +static inline int AD7414_TEMP_FROM_REG(u16 reg) > +{ > + /* use integer division instead of equivalent right shift to > + * guarantee arithmetic shift and preserve the sign > + */ > + return ((s16)reg / 128) * 500; > +} > + > +/* All registers are word-sized, except for the configuration registers. > + * AD7414 uses a high-byte first convention, which is exactly opposite to > + * the usual practice. > + */ > +static int ad7414_read(struct i2c_client *client, u8 reg) > +{ > + if (reg == AD7414_REG_TEMP) > + return swab16(i2c_smbus_read_word_data(client, reg)); > + else > + return i2c_smbus_read_byte_data(client, reg); > +} > + > +static int ad7414_write(struct i2c_client *client, u8 reg, u16 value) > +{ > + return i2c_smbus_write_byte_data(client, reg, value); > +} > + > +struct ad7414_data *ad7414_update_device(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct ad7414_data *data = i2c_get_clientdata(client); > + > + mutex_lock(&data->lock); > + > + if (time_after(jiffies, data->last_updated + HZ + HZ / 2) > + || !data->valid) { > + dev_dbg(&client->dev, "starting ad7414 update\n"); > + > + data->temp_input = ad7414_read(client, AD7414_REG_TEMP); > + data->temp_alert = (data->temp_input >> 5) & 0x01; > + data->temp_max_flag = (data->temp_input >> 4) & 0x01; > + data->temp_min_flag = (data->temp_input >> 3) & 0x01; > + data->temp_max = ad7414_read(client, AD7414_REG_T_HIGH); > + data->temp_min = ad7414_read(client, AD7414_REG_T_LOW); > + > + data->last_updated = jiffies; > + data->valid = 1; > + } > + > + mutex_unlock(&data->lock); > + > + return data; > +} > + > +#define show(value) \ > +static ssize_t show_##value(struct device *dev, \ > + struct device_attribute *attr, char *buf) \ > +{ \ > + struct ad7414_data *data = ad7414_update_device(dev); \ > + return sprintf(buf, "%d\n", AD7414_TEMP_FROM_REG(data->value)); \ > +} > +show(temp_input); > + > +#define show_8(value) \ > +static ssize_t show_##value(struct device *dev, \ > + struct device_attribute *attr, char *buf) \ > +{ \ > + struct ad7414_data *data = ad7414_update_device(dev); \ > + return sprintf(buf, "%d\n", data->value); \ > +} > +show_8(temp_max); > +show_8(temp_min); > +show_8(temp_alert); > +show_8(temp_max_flag); > +show_8(temp_min_flag); > + > +#define set(value, reg) \ > +static ssize_t set_##value(struct device *dev, \ > + struct device_attribute *attr, \ > + const char *buf, size_t count) \ > +{ \ > + struct i2c_client *client = to_i2c_client(dev); \ > + struct ad7414_data *data = i2c_get_clientdata(client); \ > + int temp = simple_strtoul(buf, NULL, 10); \ > + \ > + mutex_lock(&data->lock); \ > + data->value = temp; \ > + ad7414_write(client, reg, data->value); \ > + mutex_unlock(&data->lock); \ > + return count; \ > +} > +set(temp_max, AD7414_REG_T_HIGH); > +set(temp_min, AD7414_REG_T_LOW); > + > +static DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max); > +static DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min); > +static DEVICE_ATTR(temp1_input, S_IRUGO, show_temp_input, NULL); > +static DEVICE_ATTR(temp1_alert, S_IRUGO, show_temp_alert, NULL); > +static DEVICE_ATTR(temp1_max_flag, S_IRUGO, show_temp_max_flag, NULL); > +static DEVICE_ATTR(temp1_min_flag, S_IRUGO, show_temp_min_flag, NULL); > + > + > +static struct attribute *ad7414_attributes[] = { > + &dev_attr_temp1_input.attr, > + &dev_attr_temp1_max.attr, > + &dev_attr_temp1_min.attr, > + &dev_attr_temp1_alert.attr, > + &dev_attr_temp1_max_flag.attr, > + &dev_attr_temp1_min_flag.attr, > + NULL > +}; > + > +static const struct attribute_group ad7414_group = { > + .attrs = ad7414_attributes, > +}; > + > +static int ad7414_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct ad7414_data *data; > + int err = 0; > + > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA | > + I2C_FUNC_SMBUS_WORD_DATA)) > + goto exit; > + > + data = kzalloc(sizeof(struct ad7414_data), GFP_KERNEL); > + if (!data) { > + err = -ENOMEM; > + goto exit; > + } > + > + i2c_set_clientdata(client, data); > + mutex_init(&data->lock); > + > + dev_info(&client->dev, "chip found, driver version " DRV_VERSION "\n"); > + > + /* Register sysfs hooks */ > + err = sysfs_create_group(&client->dev.kobj, &ad7414_group); > + if (err) > + goto exit_free; > + > + data->dev = hwmon_device_register(&client->dev); > + if (IS_ERR(data->dev)) { > + err = PTR_ERR(data->dev); > + goto exit_remove; > + } > + > + return 0; > + > +exit_remove: > + sysfs_remove_group(&client->dev.kobj, &ad7414_group); > +exit_free: > + kfree(data); > +exit: > + return err; > +} > + > +static int __devexit ad7414_remove(struct i2c_client *client) > +{ > + struct ad7414_data *data = i2c_get_clientdata(client); > + > + hwmon_device_unregister(data->dev); > + sysfs_remove_group(&client->dev.kobj, &ad7414_group); > + kfree(data); > + return 0; > +} > + > +static const struct i2c_device_id ad7414_id[] = { > + { "ad7414", 0 }, > + {} > +}; > + > +static struct i2c_driver ad7414_driver = { > + .driver = { > + .name = "ad7414", > + }, > + .probe = ad7414_probe, > + .remove = __devexit_p(ad7414_remove), > + .id_table = ad7414_id, > +}; > + > +static int __init ad7414_init(void) > +{ > + return i2c_add_driver(&ad7414_driver); > +} > +module_init(ad7414_init); > + > +static void __exit ad7414_exit(void) > +{ > + i2c_del_driver(&ad7414_driver); > +} > +module_exit(ad7414_exit); > + > +MODULE_AUTHOR("Stefan Roese <sr at denx.de>, " > + "Frank Edelhaeuser <frank.edelhaeuser at spansion.com>"); > + > +MODULE_DESCRIPTION("AD7414 driver"); > +MODULE_LICENSE("GPL"); > +MODULE_VERSION(DRV_VERSION); > > _______________________________________________ > lm-sensors mailing list > lm-sensors at lm-sensors.org > http://lists.lm-sensors.org/mailman/listinfo/lm-sensors -- Jean Delvare
