[lm-sensors] [PATCH] ad7414 driver

[lm-sensors] [PATCH] ad7414 driver

[Sean MacLennan]

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.

Cheers,
   Sean

Signed-off-by: Sean MacLennan <smaclennan@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@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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] ad7414 driver

[Jean Delvare]

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@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@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@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors


-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] ad7414 driver

[Sean MacLennan]

On Sun, 15 Jun 2008 10:09:25 +0200
Jean Delvare <khali@linux-fr.org> wrote:

> 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

YIKES! I forgot to put that version back in PIKA's mainline kernel tree.

This should be the *correct* version, with the id table patch.

Cheers,
   Sean

Signed-off-by: Sean MacLennan <smaclennan@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..5408b04
--- /dev/null
+++ b/drivers/hwmon/ad7414.c
@@ -0,0 +1,251 @@
+/*
+ * 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@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/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+
+
+#define DRV_VERSION "0.3"
+
+/* 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 device		*hwmon_dev;
+	struct mutex		lock;	/* atomic read data updates */
+	char			valid;	/* !=0 if following fields are valid */
+	unsigned long		next_update;	/* In jiffies */
+	s16			temp_input;	/* Register values */
+	s8			temp_max;
+	s8			temp_min;
+};
+
+/* REG: (0.25C/bit, two's complement) << 6 */
+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 ((s64)reg / 64) * 250;
+}
+
+static inline 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 inline 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->next_update) || !data->valid) {
+		dev_dbg(&client->dev, "starting ad7414 update\n");
+
+		data->temp_input = ad7414_read(client, AD7414_REG_TEMP);
+		data->temp_max = ad7414_read(client, AD7414_REG_T_HIGH);
+		data->temp_min = ad7414_read(client, AD7414_REG_T_LOW);
+
+		data->next_update = jiffies + HZ + HZ / 2;
+		data->valid = 1;
+	}
+
+	mutex_unlock(&data->lock);
+
+	return data;
+}
+
+static ssize_t show_temp_input(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->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);
+
+#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);	\
+	long temp = simple_strtol(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 ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	int bitnr = to_sensor_dev_attr(attr)->index;
+	struct ad7414_data *data = ad7414_update_device(dev);
+	int value = (data->temp_input >> bitnr) & 1;
+
+	/* alert is active low */
+	if (bitnr = 5)
+		value ^= 1;
+
+
+	return sprintf(buf, "%d\n", value);
+}
+
+static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IRUGO, show_alarm, NULL, 3);
+static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, show_alarm, NULL, 4);
+static SENSOR_DEVICE_ATTR(temp1_alert, S_IRUGO, show_alarm, NULL, 5);
+
+
+static struct attribute *ad7414_attributes[] = {
+	&dev_attr_temp1_input.attr,
+	&dev_attr_temp1_max.attr,
+	&dev_attr_temp1_min.attr,
+	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_alert.dev_attr.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 *dev_id)
+{
+	struct ad7414_data *data;
+	int err = 0;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_READ_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->hwmon_dev = hwmon_device_register(&client->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		err = PTR_ERR(data->hwmon_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->hwmon_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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] ad7414 driver

[Jean Delvare]

Hi Sean,

On Sun, 15 Jun 2008 13:13:31 -0400, Sean MacLennan wrote:
> On Sun, 15 Jun 2008 10:09:25 +0200
> Jean Delvare <khali@linux-fr.org> wrote:
> 
> > 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
> 
> YIKES! I forgot to put that version back in PIKA's mainline kernel tree.
> 
> This should be the *correct* version, with the id table patch.
> 
> Cheers,
>    Sean

Review:

> 
> Signed-off-by: Sean MacLennan <smaclennan@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..5408b04
> --- /dev/null
> +++ b/drivers/hwmon/ad7414.c
> @@ -0,0 +1,251 @@
> +/*
> + * 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@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/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>

You should also include <linux/sysfs.h> (for sysfs_create_group).

> +
> +
> +#define DRV_VERSION "0.3"

Once a driver is merged in the kernel, having a version isn't terribly
useful. My experience is that nobody will remember updating it when
doing changes to the driver, so the kernel version is always more
reliable than the driver version.

> +
> +/* AD7414 registers */
> +#define AD7414_REG_TEMP		0x00
> +#define AD7414_REG_CONF		0x01

You never access the configuration register. The driver apparently
assumes that the chip is in continuous conversion mode, so it would
probably be a good idea to ensure that it really is, and switch it on
if not.

> +#define AD7414_REG_T_HIGH	0x02
> +#define AD7414_REG_T_LOW	0x03
> +
> +
> +struct ad7414_data {
> +	struct device		*hwmon_dev;
> +	struct mutex		lock;	/* atomic read data updates */
> +	char			valid;	/* !=0 if following fields are valid */
> +	unsigned long		next_update;	/* In jiffies */
> +	s16			temp_input;	/* Register values */
> +	s8			temp_max;
> +	s8			temp_min;
> +};
> +
> +/* REG: (0.25C/bit, two's complement) << 6 */
> +static inline int AD7414_TEMP_FROM_REG(u16 reg)

reg should be s16. I also suggest using lower-case for the function
name.

> +{
> +	/* use integer division instead of equivalent right shift to
> +	 * guarantee arithmetic shift and preserve the sign
> +	 */
> +	return ((s64)reg / 64) * 250;

This cast to s64 is a bad idea. 64-bit integers are costly on on 32-bit
machines. Casting to an int would be enough.

> +}
> +
> +static inline int ad7414_read(struct i2c_client *client, u8 reg)
> +{
> +	if (reg = AD7414_REG_TEMP)
> +		return swab16(i2c_smbus_read_word_data(client, reg));

This will return random values if i2c_smbus_read_word_data returns an
error. The lm75 driver has just been fixed:
http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023040.html
You could do something similar.

> +	else
> +		return i2c_smbus_read_byte_data(client, reg);
> +}
> +
> +static inline int ad7414_write(struct i2c_client *client, u8 reg, u16 value)

value could be u8, as you only ever write 8-bit values.

> +{
> +	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->next_update) || !data->valid) {
> +		dev_dbg(&client->dev, "starting ad7414 update\n");
> +
> +		data->temp_input = ad7414_read(client, AD7414_REG_TEMP);
> +		data->temp_max = ad7414_read(client, AD7414_REG_T_HIGH);
> +		data->temp_min = ad7414_read(client, AD7414_REG_T_LOW);
> +
> +		data->next_update = jiffies + HZ + HZ / 2;
> +		data->valid = 1;
> +	}
> +
> +	mutex_unlock(&data->lock);
> +
> +	return data;
> +}
> +
> +static ssize_t show_temp_input(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->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);		\

You are supposed to multiply the value by 1000! Was this driver never
tested with libsensors?

> +}
> +show_8(temp_max);
> +show_8(temp_min);
> +
> +#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);	\
> +	long temp = simple_strtol(buf, NULL, 10);		\
> +								\
> +	mutex_lock(&data->lock);				\
> +	data->value = temp;					\

This lacks conversion and boundary checking! Obviously writing to these
files was never tested :( For one thing you need to divide the input
value by 1000, and for another one you must clamp the values that don't
fit in the register (or that are below or above what the chip can
measure.)

> +	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);

Would be nice to use "dynamic" callbacks (as you do below for alarms)
instead of macro-generated ones. All you'd have to do is put the min
and max temperatures in an array (see the lm90 driver for an example.)
This would make a smaller and more readable driver.

> +
> +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 ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	int bitnr = to_sensor_dev_attr(attr)->index;
> +	struct ad7414_data *data = ad7414_update_device(dev);
> +	int value = (data->temp_input >> bitnr) & 1;
> +
> +	/* alert is active low */
> +	if (bitnr = 5)
> +		value ^= 1;

Alert is in fact active high or low depending on a bit in the
configuration file. You have to check it.

> +
> +
> +	return sprintf(buf, "%d\n", value);
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IRUGO, show_alarm, NULL, 3);
> +static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, show_alarm, NULL, 4);
> +static SENSOR_DEVICE_ATTR(temp1_alert, S_IRUGO, show_alarm, NULL, 5);

As explained before, temp1_alert is not a standard sysfs file name so
libsensors won't pick it. Also, this bit seems to trigger when the high
alarm flag triggers, so it's somewhat redundant. The only difference if
I read the datasheet correctly is that the high alarm flag clears
itself automatically while the alert flag does not. As the driver never
clears it, it doesn't seem very useful to me.

> +
> +
> +static struct attribute *ad7414_attributes[] = {
> +	&dev_attr_temp1_input.attr,
> +	&dev_attr_temp1_max.attr,
> +	&dev_attr_temp1_min.attr,
> +	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp1_alert.dev_attr.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 *dev_id)
> +{
> +	struct ad7414_data *data;
> +	int err = 0;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_READ_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->hwmon_dev = hwmon_device_register(&client->dev);
> +	if (IS_ERR(data->hwmon_dev)) {
> +		err = PTR_ERR(data->hwmon_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->hwmon_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);


-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] ad7414 driver

[Sean MacLennan]

Jean, 

Thanks for the review... I will work on the changes.

I just wanted to make a couple of general comments. This driver was
based on the ad7418 driver, at least according to the copyrights at the
top. I assumed that the basic functionality (show/set for example) was
correct. I notice that many of the comments for this driver also apply
to the ad7418 ;)

I am using the ad7414 on the PIKA Warp product. The ad7414 is a very
important part of what we call DTM: Dynamic Thermal Management.
Basically DTM controls the fan and shuts down the machine if it gets
too hot.

The Warp does not have libsensors, let alone any apps to connect to
libsensors. All of the DTM is done in the kernel itself.

I could give you a link to the DTM code, but we had a bad thunderstorm
here and I can no longer connect to work :( I believe it is here:

http://svn.pikatech.com/appliance/branches/linux-2.6/arch/powerpc/platforms/44x/warp/warp.c

Cheers,
   Sean

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

Re: [lm-sensors] [PATCH] ad7414 driver

[Jean Delvare]

Hi Sean

On Sun, 15 Jun 2008 18:07:48 -0400, Sean MacLennan wrote:
> Thanks for the review... I will work on the changes.

Great.

> I just wanted to make a couple of general comments. This driver was
> based on the ad7418 driver, at least according to the copyrights at the
> top. I assumed that the basic functionality (show/set for example) was
> correct. I notice that many of the comments for this driver also apply
> to the ad7418 ;)

I just took a quick look at the current ad7418 driver and I didn't see
much that needs fixing, except for the missing inclusion of
<linux/sysfs.h>. Also, the ad7418 driver is a legacy i2c driver while
the ad7414 driver was turned into a new-style i2c driver, so all the
administrative part is different. Anyway, if you see things in the
ad7418 driver which you think needs fixing or could be improved, feel
free to send a patch and we'll review it.

> I am using the ad7414 on the PIKA Warp product. The ad7414 is a very
> important part of what we call DTM: Dynamic Thermal Management.
> Basically DTM controls the fan and shuts down the machine if it gets
> too hot.
> 
> The Warp does not have libsensors, let alone any apps to connect to
> libsensors. All of the DTM is done in the kernel itself.
> 
> I could give you a link to the DTM code, but we had a bad thunderstorm
> here and I can no longer connect to work :( I believe it is here:
> 
> http://svn.pikatech.com/appliance/branches/linux-2.6/arch/powerpc/platforms/44x/warp/warp.c

Almost...

http://svn.pikatech.com/appliance/branches/linux-2.6/arch/powerpc/platforms/44x/warp.c

Hmm, the code there is not exactly nice... Obviously the hwmon and i2c
subsystems were not designed with this use case in mind, there's room
for improvement. But that's a long way to go, and I really don't have
the time to dive into this at the moment. I'll try to keep in mind for
later though.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] ad7414 driver

[Sean MacLennan]

This should fix all the items mentioned in Jean's review.

Cheers,
   Sean

Signed-off-by: Sean MacLennan <smaclennan@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..1b1e343
--- /dev/null
+++ b/drivers/hwmon/ad7414.c
@@ -0,0 +1,261 @@
+/*
+ * 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@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/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/sysfs.h>
+
+
+/* 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 device		*hwmon_dev;
+	struct mutex		lock;	/* atomic read data updates */
+	char			valid;	/* !=0 if following fields are valid */
+	unsigned long		next_update;	/* In jiffies */
+	s16			temp_input;	/* Register values */
+	s8			temps[2];
+};
+
+/* REG: (0.25C/bit, two's complement) << 6 */
+static inline int ad7414_temp_from_reg(s16 reg)
+{
+	/* use integer division instead of equivalent right shift to
+	 * guarantee arithmetic shift and preserve the sign
+	 */
+	return ((int)reg / 64) * 250;
+}
+
+static inline int ad7414_read(struct i2c_client *client, u8 reg)
+{
+	if (reg = AD7414_REG_TEMP) {
+		int value = i2c_smbus_read_word_data(client, reg);
+		return (value < 0) ? value : swab16(value);
+	} else
+		return i2c_smbus_read_byte_data(client, reg);
+}
+
+static inline int ad7414_write(struct i2c_client *client, u8 reg, u8 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->next_update) || !data->valid) {
+		int value;
+		dev_dbg(&client->dev, "starting ad7414 update\n");
+
+		value = ad7414_read(client, AD7414_REG_TEMP);
+		if (value < 0)
+			dev_dbg(&client->dev, "AD7414_REG_TEMP err %d\n",
+				value);
+		else
+			data->temp_input = value;
+		value = ad7414_read(client, AD7414_REG_T_HIGH);
+		if (value < 0)
+			dev_dbg(&client->dev, "AD7414_REG_T_HIGH err %d\n",
+				value);
+		else
+			data->temps[0] = value;
+		value = ad7414_read(client, AD7414_REG_T_LOW);
+		if (value < 0)
+			dev_dbg(&client->dev, "AD7414_REG_T_LOW err %d\n",
+				value);
+		else
+			data->temps[1] = value;
+
+		data->next_update = jiffies + HZ + HZ / 2;
+		data->valid = 1;
+	}
+
+	mutex_unlock(&data->lock);
+
+	return data;
+}
+
+static ssize_t show_temp_input(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->temp_input));
+}
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp_input, NULL, 0);
+
+static ssize_t show_max_min(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	int index = to_sensor_dev_attr(attr)->index;
+	struct ad7414_data *data = ad7414_update_device(dev);
+	return sprintf(buf, "%d\n", data->temps[index] * 1000);
+}
+
+static ssize_t set_max_min(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 index = to_sensor_dev_attr(attr)->index;
+	u8 reg = index ? AD7414_REG_T_LOW : AD7414_REG_T_HIGH;
+	long temp = simple_strtol(buf, NULL, 10);
+
+	temp = SENSORS_LIMIT(temp, -40000, 85000);
+	temp /= 1000;
+
+	mutex_lock(&data->lock);
+	data->temps[index] = temp;
+	ad7414_write(client, reg, temp);
+	mutex_unlock(&data->lock);
+	return count;
+}
+
+static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR |  S_IRUGO,
+			  show_max_min, set_max_min, 0);
+static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO,
+			  show_max_min, set_max_min, 1);
+
+static ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	int bitnr = to_sensor_dev_attr(attr)->index;
+	struct ad7414_data *data = ad7414_update_device(dev);
+	int value = (data->temp_input >> bitnr) & 1;
+	return sprintf(buf, "%d\n", value);
+}
+
+static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IRUGO, show_alarm, NULL, 3);
+static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, show_alarm, NULL, 4);
+
+static struct attribute *ad7414_attributes[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
+	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_min_alarm.dev_attr.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 *dev_id)
+{
+	struct ad7414_data *data;
+	int err = 0;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_READ_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\n");
+
+	/* Put the chip in the default power up state. */
+	i2c_smbus_write_byte_data(client, AD7414_REG_CONF, 0x40);
+
+	/* Register sysfs hooks */
+	err = sysfs_create_group(&client->dev.kobj, &ad7414_group);
+	if (err)
+		goto exit_free;
+
+	data->hwmon_dev = hwmon_device_register(&client->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		err = PTR_ERR(data->hwmon_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->hwmon_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");

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

Re: [lm-sensors] [PATCH] ad7414 driver

[Jean Delvare]

Hi Sean,

On Mon, 16 Jun 2008 19:11:15 -0400, Sean MacLennan wrote:
> This should fix all the items mentioned in Jean's review.

Note: when you resubmit patches, please always include a description
suitable for the git commit.

It's much better, but I have a couple comments left, one of which
_must_ be addressed before your driver can go upstream:

> Signed-off-by: Sean MacLennan <smaclennan@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..1b1e343
> --- /dev/null
> +++ b/drivers/hwmon/ad7414.c
> @@ -0,0 +1,261 @@
> +/*
> + * 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@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/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/sysfs.h>
> +
> +
> +/* 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 device		*hwmon_dev;
> +	struct mutex		lock;	/* atomic read data updates */
> +	char			valid;	/* !=0 if following fields are valid */
> +	unsigned long		next_update;	/* In jiffies */
> +	s16			temp_input;	/* Register values */
> +	s8			temps[2];
> +};
> +
> +/* REG: (0.25C/bit, two's complement) << 6 */
> +static inline int ad7414_temp_from_reg(s16 reg)
> +{
> +	/* use integer division instead of equivalent right shift to
> +	 * guarantee arithmetic shift and preserve the sign
> +	 */
> +	return ((int)reg / 64) * 250;
> +}
> +
> +static inline int ad7414_read(struct i2c_client *client, u8 reg)
> +{
> +	if (reg = AD7414_REG_TEMP) {
> +		int value = i2c_smbus_read_word_data(client, reg);
> +		return (value < 0) ? value : swab16(value);
> +	} else
> +		return i2c_smbus_read_byte_data(client, reg);
> +}
> +
> +static inline int ad7414_write(struct i2c_client *client, u8 reg, u8 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->next_update) || !data->valid) {
> +		int value;
> +		dev_dbg(&client->dev, "starting ad7414 update\n");
> +
> +		value = ad7414_read(client, AD7414_REG_TEMP);
> +		if (value < 0)
> +			dev_dbg(&client->dev, "AD7414_REG_TEMP err %d\n",
> +				value);
> +		else
> +			data->temp_input = value;
> +		value = ad7414_read(client, AD7414_REG_T_HIGH);
> +		if (value < 0)
> +			dev_dbg(&client->dev, "AD7414_REG_T_HIGH err %d\n",
> +				value);
> +		else
> +			data->temps[0] = value;
> +		value = ad7414_read(client, AD7414_REG_T_LOW);
> +		if (value < 0)
> +			dev_dbg(&client->dev, "AD7414_REG_T_LOW err %d\n",
> +				value);
> +		else
> +			data->temps[1] = value;
> +
> +		data->next_update = jiffies + HZ + HZ / 2;
> +		data->valid = 1;
> +	}
> +
> +	mutex_unlock(&data->lock);
> +
> +	return data;
> +}
> +
> +static ssize_t show_temp_input(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->temp_input));
> +}
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp_input, NULL, 0);

Note that you don't really need to use SENSOR_DEVICE_ATTR for this one,
as you make no use of the index value. No big deal either though.

> +
> +static ssize_t show_max_min(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	int index = to_sensor_dev_attr(attr)->index;
> +	struct ad7414_data *data = ad7414_update_device(dev);
> +	return sprintf(buf, "%d\n", data->temps[index] * 1000);
> +}
> +
> +static ssize_t set_max_min(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 index = to_sensor_dev_attr(attr)->index;
> +	u8 reg = index ? AD7414_REG_T_LOW : AD7414_REG_T_HIGH;

Note: your code is correct but if you have some more time to spend on
it, here's a way you can improve it. Define AD7414_REG_LIMIT as an array of
2 values { AD7414_REG_T_HIGH, AD7414_REG_T_LOW }. Then you can use
index to get the right register address directly:
AD7414_REG_LIMIT[index]. This is a small win here, but a bigger one in
ad7414_update_device(), because now you can use a loop to read the HIGH
and LOW limits.

Do as you want though - your driver, your call.

> +	long temp = simple_strtol(buf, NULL, 10);
> +
> +	temp = SENSORS_LIMIT(temp, -40000, 85000);
> +	temp /= 1000;

Other drivers round properly when a user sets a limit. That would be
done with:

	temp = temp + ((temp < 0 ? -500 : 500)) / 1000;

Maybe someday we'll have universal helper functions for temperature
conversions in the hwmon module.

> +
> +	mutex_lock(&data->lock);
> +	data->temps[index] = temp;
> +	ad7414_write(client, reg, temp);
> +	mutex_unlock(&data->lock);
> +	return count;
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR |  S_IRUGO,

Doubled space.

> +			  show_max_min, set_max_min, 0);
> +static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO,
> +			  show_max_min, set_max_min, 1);
> +
> +static ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	int bitnr = to_sensor_dev_attr(attr)->index;
> +	struct ad7414_data *data = ad7414_update_device(dev);
> +	int value = (data->temp_input >> bitnr) & 1;
> +	return sprintf(buf, "%d\n", value);
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IRUGO, show_alarm, NULL, 3);
> +static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, show_alarm, NULL, 4);
> +
> +static struct attribute *ad7414_attributes[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max.dev_attr.attr,
> +	&sensor_dev_attr_temp1_min.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp1_min_alarm.dev_attr.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 *dev_id)
> +{
> +	struct ad7414_data *data;
> +	int err = 0;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_READ_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\n");
> +
> +	/* Put the chip in the default power up state. */
> +	i2c_smbus_write_byte_data(client, AD7414_REG_CONF, 0x40);

This is overwriting _all_ the bits of the configuration file, including
changes that could have been done by the the BIOS or firmware or
whatever. That's not OK! You must read the configuration register
value, check bit 2, and if it's set, write the value back with just bit
2 cleared.

> +
> +	/* Register sysfs hooks */
> +	err = sysfs_create_group(&client->dev.kobj, &ad7414_group);
> +	if (err)
> +		goto exit_free;
> +
> +	data->hwmon_dev = hwmon_device_register(&client->dev);
> +	if (IS_ERR(data->hwmon_dev)) {
> +		err = PTR_ERR(data->hwmon_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->hwmon_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");

All the rest looks OK now.

Thanks,
-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] ad7414 driver

[Sean MacLennan]

On Thu, 19 Jun 2008 12:46:50 +0200
Jean Delvare <khali@linux-fr.org> wrote:

> Hi Sean,
> > +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp_input,
> > NULL, 0);
> 
> Note that you don't really need to use SENSOR_DEVICE_ATTR for this
> one, as you make no use of the index value. No big deal either though.

Agreed, but I thought it *looked* better if they all used
SENSOR_DEVICE_ATTR than a mix of SENSOR_DEVICE and DEVICE.

> > +	/* Put the chip in the default power up state. */
> > +	i2c_smbus_write_byte_data(client, AD7414_REG_CONF, 0x40);
> 
> This is overwriting _all_ the bits of the configuration file,
> including changes that could have been done by the the BIOS or
> firmware or whatever. That's not OK! You must read the configuration
> register value, check bit 2, and if it's set, write the value back
> with just bit 2 cleared.

I work with, what we call, "board drivers" too much. We always reset
everything to a know state ;)

We don't need to worry about bit 2. The chip always runs in continuous
conversion mode unless it is powered down. The one shot just *forces*
an immediate conversion rather than waiting 800ms.

The one shot is really more useful in the powered down mode.

All we have to guarantee is that the chip is powered up. Below is the
new code:

	/* Make sure the chip is powered up. */
	conf = i2c_smbus_read_byte_data(client, AD7414_REG_CONF);
	if (conf < 0)
		printk(KERN_WARNING
		       "ad7414_probe unable to read config register.\n");
	else {
		conf &= ~(1 << 7);
		i2c_smbus_write_byte_data(client, AD7414_REG_CONF, conf);
	}

I do not fail the driver if the config read fails. The ad7414 is very
important for our board and will always come up with the reset
value. So the above code is really a NOP for us. I would hate to fail
the driver due to an i2c bus glitch.

If we can agree on the above patch, I can submit a patch with the code
changes.

I also added the AD7414_REG_LIMIT array and the round off change.

Cheers,
   Sean

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

Re: [lm-sensors] [PATCH] ad7414 driver

[Jean Delvare]

Hi Sean,

On Thu, 19 Jun 2008 12:37:04 -0400, Sean MacLennan wrote:
> On Thu, 19 Jun 2008 12:46:50 +0200, Jean Delvare wrote:
> > This is overwriting _all_ the bits of the configuration file,
> > including changes that could have been done by the the BIOS or
> > firmware or whatever. That's not OK! You must read the configuration
> > register value, check bit 2, and if it's set, write the value back
> > with just bit 2 cleared.
> 
> I work with, what we call, "board drivers" too much. We always reset
> everything to a know state ;)
> 
> We don't need to worry about bit 2. The chip always runs in continuous
> conversion mode unless it is powered down. The one shot just *forces*
> an immediate conversion rather than waiting 800ms.
> 
> The one shot is really more useful in the powered down mode.

My bad, I really meant bit 7, not bit 2. I read the datasheet in a
hurry it seems.

> All we have to guarantee is that the chip is powered up. Below is the
> new code:
> 
> 	/* Make sure the chip is powered up. */
> 	conf = i2c_smbus_read_byte_data(client, AD7414_REG_CONF);
> 	if (conf < 0)
> 		printk(KERN_WARNING
> 		       "ad7414_probe unable to read config register.\n");
> 	else {
> 		conf &= ~(1 << 7);
> 		i2c_smbus_write_byte_data(client, AD7414_REG_CONF, conf);
> 	}
> 
> I do not fail the driver if the config read fails. The ad7414 is very
> important for our board and will always come up with the reset
> value. So the above code is really a NOP for us. I would hate to fail
> the driver due to an i2c bus glitch.
> 
> If we can agree on the above patch, I can submit a patch with the code
> changes.

Yes, that's totally fine with me. Except that you want to use dev_warn
instead of printk, of course.

> I also added the AD7414_REG_LIMIT array and the round off change.

Great, thanks.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] ad7414 driver

[Sean MacLennan]

Driver for the Analog Devices AD7414 temperature monitoring chip.


Signed-off-by: Sean MacLennan <smaclennan@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..ce8d94f
--- /dev/null
+++ b/drivers/hwmon/ad7414.c
@@ -0,0 +1,268 @@
+/*
+ * 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@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/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/sysfs.h>
+
+
+/* AD7414 registers */
+#define AD7414_REG_TEMP		0x00
+#define AD7414_REG_CONF		0x01
+#define AD7414_REG_T_HIGH	0x02
+#define AD7414_REG_T_LOW	0x03
+
+static u8 AD7414_REG_LIMIT[] = { AD7414_REG_T_HIGH, AD7414_REG_T_LOW };
+
+struct ad7414_data {
+	struct device		*hwmon_dev;
+	struct mutex		lock;	/* atomic read data updates */
+	char			valid;	/* !=0 if following fields are valid */
+	unsigned long		next_update;	/* In jiffies */
+	s16			temp_input;	/* Register values */
+	s8			temps[ARRAY_SIZE(AD7414_REG_LIMIT)];
+};
+
+/* REG: (0.25C/bit, two's complement) << 6 */
+static inline int ad7414_temp_from_reg(s16 reg)
+{
+	/* use integer division instead of equivalent right shift to
+	 * guarantee arithmetic shift and preserve the sign
+	 */
+	return ((int)reg / 64) * 250;
+}
+
+static inline int ad7414_read(struct i2c_client *client, u8 reg)
+{
+	if (reg = AD7414_REG_TEMP) {
+		int value = i2c_smbus_read_word_data(client, reg);
+		return (value < 0) ? value : swab16(value);
+	} else
+		return i2c_smbus_read_byte_data(client, reg);
+}
+
+static inline int ad7414_write(struct i2c_client *client, u8 reg, u8 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->next_update) || !data->valid) {
+		int value, i;
+
+		dev_dbg(&client->dev, "starting ad7414 update\n");
+
+		value = ad7414_read(client, AD7414_REG_TEMP);
+		if (value < 0)
+			dev_dbg(&client->dev, "AD7414_REG_TEMP err %d\n",
+				value);
+		else
+			data->temp_input = value;
+
+		for (i = 0; i < ARRAY_SIZE(AD7414_REG_LIMIT); ++i) {
+			value = ad7414_read(client, AD7414_REG_LIMIT[i]);
+			if (value < 0)
+				dev_dbg(&client->dev, "AD7414 reg %d err %d\n",
+					AD7414_REG_LIMIT[i], value);
+			else
+				data->temps[i] = value;
+		}
+
+		data->next_update = jiffies + HZ + HZ / 2;
+		data->valid = 1;
+	}
+
+	mutex_unlock(&data->lock);
+
+	return data;
+}
+
+static ssize_t show_temp_input(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->temp_input));
+}
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp_input, NULL, 0);
+
+static ssize_t show_max_min(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	int index = to_sensor_dev_attr(attr)->index;
+	struct ad7414_data *data = ad7414_update_device(dev);
+	return sprintf(buf, "%d\n", data->temps[index] * 1000);
+}
+
+static ssize_t set_max_min(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 index = to_sensor_dev_attr(attr)->index;
+	u8 reg = AD7414_REG_LIMIT[index];
+	long temp = simple_strtol(buf, NULL, 10);
+
+	temp = SENSORS_LIMIT(temp, -40000, 85000);
+	temp = (temp + (temp < 0 ? -500 : 500)) / 1000;
+
+	mutex_lock(&data->lock);
+	data->temps[index] = temp;
+	ad7414_write(client, reg, temp);
+	mutex_unlock(&data->lock);
+	return count;
+}
+
+static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO,
+			  show_max_min, set_max_min, 0);
+static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO,
+			  show_max_min, set_max_min, 1);
+
+static ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	int bitnr = to_sensor_dev_attr(attr)->index;
+	struct ad7414_data *data = ad7414_update_device(dev);
+	int value = (data->temp_input >> bitnr) & 1;
+	return sprintf(buf, "%d\n", value);
+}
+
+static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IRUGO, show_alarm, NULL, 3);
+static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, show_alarm, NULL, 4);
+
+static struct attribute *ad7414_attributes[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
+	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_min_alarm.dev_attr.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 *dev_id)
+{
+	struct ad7414_data *data;
+	int conf;
+	int err = 0;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_READ_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\n");
+
+	/* Make sure the chip is powered up. */
+	conf = i2c_smbus_read_byte_data(client, AD7414_REG_CONF);
+	if (conf < 0)
+		dev_warn(&client->dev,
+			 "ad7414_probe unable to read config register.\n");
+	else {
+		conf &= ~(1 << 7);
+		i2c_smbus_write_byte_data(client, AD7414_REG_CONF, conf);
+	}
+
+	/* Register sysfs hooks */
+	err = sysfs_create_group(&client->dev.kobj, &ad7414_group);
+	if (err)
+		goto exit_free;
+
+	data->hwmon_dev = hwmon_device_register(&client->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		err = PTR_ERR(data->hwmon_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->hwmon_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");

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

Re: [lm-sensors] [PATCH] ad7414 driver

[Jean Delvare]

Hi Sean,

On Thu, 19 Jun 2008 14:02:22 -0400, Sean MacLennan wrote:
> Driver for the Analog Devices AD7414 temperature monitoring chip.
> 
> 
> Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>

Looks totally fine this time.

Acked-by: Jean Delvare <khali@linux-fr.org>

Thanks Sean for your good work and patience going through all the
iterations!

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] ad7414 driver

[Sean MacLennan]

On Mon, 23 Jun 2008 14:33:18 +0200
Jean Delvare <khali@linux-fr.org> wrote:

> Hi Sean,
> 
> On Thu, 19 Jun 2008 14:02:22 -0400, Sean MacLennan wrote:
> > Driver for the Analog Devices AD7414 temperature monitoring chip.
> > 
> > 
> > Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>
> 
> Looks totally fine this time.
> 
> Acked-by: Jean Delvare <khali@linux-fr.org>
> 
> Thanks Sean for your good work and patience going through all the
> iterations!
> 

Anybody know what is happening with this driver?

Cheers,
   Sean

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

Re: [lm-sensors] [PATCH] ad7414 driver

[Jean Delvare]

Hi Sean,

On Fri, 1 Aug 2008 23:52:53 -0400, Sean MacLennan wrote:
> On Mon, 23 Jun 2008 14:33:18 +0200
> Jean Delvare <khali@linux-fr.org> wrote:
> 
> > Hi Sean,
> > 
> > On Thu, 19 Jun 2008 14:02:22 -0400, Sean MacLennan wrote:
> > > Driver for the Analog Devices AD7414 temperature monitoring chip.
> > > 
> > > 
> > > Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>
> > 
> > Looks totally fine this time.
> > 
> > Acked-by: Jean Delvare <khali@linux-fr.org>
> > 
> > Thanks Sean for your good work and patience going through all the
> > iterations!
> > 
> 
> Anybody know what is happening with this driver?

I've pushed it to Linus:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;hl633c3025c75f5fcf3a76d375faff34e3be021b

-- 
Jean Delvare

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