[lm-sensors] [PATCH] First cut of a adt7470 driver

[lm-sensors] [PATCH] First cut of a adt7470 driver

[Darrick J. Wong]

[-- Attachment #1.1: Type: text/plain, Size: 35114 bytes --]

Hi there,

Below is a first draft of a driver for the ADT7470 chip.  I've tested
the driver against the adt7470 in the IBM IntelliStation Z30 and as far
as I can tell it's good for controlling the speed of the CPU heatsink
fans and monitor CPU temperature.  Please let me know what you think of
the driver.  The patch should apply cleanly against 2.6.22-rc7.

Please cc me as I'm not subscribed to lm-sensors.

Datasheets available here:
http://www.analog.com/en/prod/0,2877,ADT7470,00.html

--Darrick
---
adt7470: new hwmon driver for adt7470 chip.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 drivers/hwmon/Kconfig   |   10 +
 drivers/hwmon/Makefile  |    1 
 drivers/hwmon/adt7470.c |  875 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 886 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 13eea47..bb57beb 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -40,6 +40,16 @@ config SENSORS_ABITUGURU
 	  This driver can also be built as a module.  If so, the module
 	  will be called abituguru.
 
+config SENSORS_ADT7470
+	tristate "Analog Devices ADT7470"
+	depends on I2C && EXPERIMENTAL
+	help
+	  If you say yes here you get support for the Analog Devices
+	  ADT7470 temperature monitoring chips.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called adt7470.
+
 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 cfaf338..e5fc251 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_SENSORS_W83781D)	+= w83781d.o
 obj-$(CONFIG_SENSORS_W83791D)	+= w83791d.o
 
 obj-$(CONFIG_SENSORS_ABITUGURU)	+= abituguru.o
+obj-$(CONFIG_SENSORS_ADT7470)	+= adt7470.o
 obj-$(CONFIG_SENSORS_AD7418)	+= ad7418.o
 obj-$(CONFIG_SENSORS_ADM1021)	+= adm1021.o
 obj-$(CONFIG_SENSORS_ADM1025)	+= adm1025.o
diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
new file mode 100644
index 0000000..cb2ac60
--- /dev/null
+++ b/drivers/hwmon/adt7470.c
@@ -0,0 +1,875 @@
+/*
+ * A hwmon driver for the Analog Devices ADT7470
+ * Copyright (C) 2007 IBM
+ *
+ * Author: Darrick J. Wong <djwong@us.ibm.com>
+ *
+ * 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 - version 2.
+ */
+
+#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/delay.h>
+#include <linux/log2.h>
+
+#define DRV_VERSION "0.22"
+
+/* Addresses to scan */
+static unsigned short normal_i2c[] = { 0x2C, 0x2E, 0x2F, I2C_CLIENT_END };
+
+/* Insmod parameters */
+I2C_CLIENT_INSMOD_1(adt7470);
+
+/* ADT7470 registers */
+#define ADT7470_REG_BASE		0x20
+#define ADT7470_REG_TEMP_BASE		0x20
+#define ADT7470_REG_TEMP_MAX		0x29
+#define ADT7470_REG_FAN_BASE		0x2A
+#define ADT7470_REG_FAN_MAX		0x31
+#define ADT7470_REG_PWM_BASE		0x32
+//#define ADT7470_REG_PWM_MAX		0x35
+#define ADT7470_REG_PWM_MAX_BASE	0x38
+#define ADT7470_REG_PWM_MAX_MAX		0x3B
+#define ADT7470_REG_CFG			0x40
+#define		ADT7470_FSPD_MASK	0x04
+#define ADT7470_REG_ALARM1		0x41
+#define ADT7470_REG_ALARM2		0x42
+#define ADT7470_REG_TEMP_LIMITS_BASE	0x44
+#define ADT7470_REG_TEMP_LIMITS_MAX	0x57
+#define ADT7470_REG_FAN_MIN_BASE	0x58
+#define ADT7470_REG_FAN_MIN_MAX		0x5F
+#define ADT7470_REG_FAN_MAX_BASE	0x60
+#define ADT7470_REG_FAN_MAX_MAX		0x67
+#define ADT7470_REG_PWM_CFG_BASE	0x68
+#define ADT7470_REG_PWM12_CFG		0x68
+#define		ADT7470_PWM1_AUTO_MASK	0x40
+#define		ADT7470_PWM2_AUTO_MASK	0x80
+#define ADT7470_REG_PWM34_CFG		0x69
+#define		ADT7470_PWM3_AUTO_MASK	0x40
+#define		ADT7470_PWM4_AUTO_MASK	0x80
+#define	ADT7470_REG_PWM_MIN_BASE	0x6A
+#define ADT7470_REG_PWM_MIN_MAX		0x6D
+#define ADT7470_REG_PWM_TEMP_MIN_BASE	0x6E
+#define ADT7470_REG_PWM_TEMP_MIN_MAX	0x71
+#define ADT7470_REG_ACOUSTICS12		0x75
+#define ADT7470_REG_ACOUSTICS34		0x76
+#define ADT7470_REG_DEVICE		0x3D
+#define ADT7470_REG_VENDOR		0x3E
+#define ADT7470_REG_REVISION		0x3F
+#define ADT7470_REG_ALARM1_MASK		0x72
+#define ADT7470_REG_ALARM2_MASK		0x73
+#define ADT7470_REG_PWM_AUTO_TEMP_BASE	0x7C
+#define ADT7470_REG_PWM_AUTO_TEMP_MAX	0x7D
+#define ADT7470_REG_MAX			0x81
+
+#define ADT7470_TEMP_COUNT	10
+#define ADT7470_TEMP_REG(x)	(ADT7470_REG_TEMP_BASE + (x))
+#define ADT7470_TEMP_MIN_REG(x) (ADT7470_REG_TEMP_LIMITS_BASE + (2 * (x)))
+#define ADT7470_TEMP_MAX_REG(x) (ADT7470_REG_TEMP_LIMITS_BASE + (2 * (x)) + 1)
+
+#define ADT7470_FAN_COUNT	4
+
+#define ADT7470_PWM_COUNT	4
+#define ADT7470_REG_PWM(x)	(ADT7470_REG_PWM_BASE + (x))
+#define ADT7470_REG_PWM_MAX(x)	(ADT7470_REG_PWM_MAX_BASE + (x))
+#define ADT7470_REG_PWM_MIN(x)	(ADT7470_REG_PWM_MIN_BASE + (x))
+#define ADT7470_REG_PWM_TMIN(x)	(ADT7470_REG_PWM_TEMP_MIN_BASE + (x))
+
+#define ADT7470_VENDOR		0x41
+#define ADT7470_DEVICE		0x70
+#define ADT7470_REVISION	0x02
+
+#define ADT7470_PWM_ALL_TEMPS	0x3FF		/* "all temps" according to hwmon sysfs interface spec */
+
+#define REFRESH_INTERVAL	(2 * HZ)
+#define TEMP_COLLECTION_TIME	1000		/* sleep 1s while gathering temperature data */
+
+#define power_of_2(x)	(((x) & ((x) - 1)) == 0)
+
+struct adt7470_data {
+	struct i2c_client	client;
+	struct class_device	*class_dev;
+	struct attribute_group	attrs;
+	enum chips		type;
+	struct mutex		lock;
+	char			valid;
+	unsigned long		last_updated;	/* In jiffies */
+
+	u8			temp[ADT7470_TEMP_COUNT];
+	u8			temp_min[ADT7470_TEMP_COUNT];
+	u8			temp_max[ADT7470_TEMP_COUNT];
+	u16			fan[ADT7470_FAN_COUNT];
+	u16			fan_min[ADT7470_FAN_COUNT];
+	u16			fan_max[ADT7470_FAN_COUNT];
+	u16			alarms, alarms_mask;
+	u8			force_pwm_max;
+	u8			pwm[ADT7470_PWM_COUNT];
+	u8			pwm_max[ADT7470_PWM_COUNT];
+	u8			pwm_automatic[ADT7470_PWM_COUNT];
+	u8			pwm_min[ADT7470_PWM_COUNT];
+	u8			pwm_tmin[ADT7470_PWM_COUNT];
+	u8			pwm_auto_temp[ADT7470_PWM_COUNT];
+
+	u8			raw[256];
+};
+
+static int adt7470_attach_adapter(struct i2c_adapter *adapter);
+static int adt7470_detect(struct i2c_adapter *adapter, int address, int kind);
+static int adt7470_detach_client(struct i2c_client *client);
+
+static struct i2c_driver adt7470_driver = {
+	.driver = {
+		.name	= "adt7470",
+	},
+	.attach_adapter	= adt7470_attach_adapter,
+	.detach_client	= adt7470_detach_client,
+};
+
+/* 16-bit registers on the ADT7470 are low-byte first. */
+static inline int adt7470_read16(struct i2c_client *client, u8 reg)
+{
+	u16 foo;
+	foo = i2c_smbus_read_byte_data(client, reg) | ((u16)i2c_smbus_read_byte_data(client, reg + 1) << 8);
+	return foo;
+}
+
+static inline int adt7470_write16(struct i2c_client *client, u8 reg, u16 value)
+{
+	return i2c_smbus_write_word_data(client, reg, cpu_to_le16(value));
+}
+
+static void adt7470_init_client(struct i2c_client *client)
+{
+	//struct adt7470_data *data = i2c_get_clientdata(client);
+
+	int reg = i2c_smbus_read_byte_data(client, ADT7470_REG_CFG);
+	if (reg < 0) {
+		dev_err(&client->dev, "cannot read configuration register\n");
+	} else {
+		// start monitoring (and do a self-test)
+		i2c_smbus_write_byte_data(client, ADT7470_REG_CFG, reg | 3);
+	}
+}
+
+static u16 adt7470_read_fan_min(struct i2c_client *client, int fan)
+{
+	return adt7470_read16(client, ADT7470_REG_FAN_MIN_BASE + (2 * fan));
+}
+
+static u16 adt7470_read_fan_max(struct i2c_client *client, int fan)
+{
+	return adt7470_read16(client, ADT7470_REG_FAN_MAX_BASE + (2 * fan));
+}
+
+static u16 adt7470_read_fan(struct i2c_client *client, int fan)
+{
+	return adt7470_read16(client, ADT7470_REG_FAN_BASE + (2 * fan));
+}
+
+static struct adt7470_data *adt7470_update_device(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7470_data *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->lock);
+
+	if (time_after(jiffies, data->last_updated + REFRESH_INTERVAL)
+		|| !data->valid) {
+		u8 cfg;
+		int i;
+
+		/* start reading temperature sensors */
+		cfg = i2c_smbus_read_byte_data(client, ADT7470_REG_CFG);
+		cfg |= 0x80;
+		i2c_smbus_write_byte_data(client, ADT7470_REG_CFG, cfg);
+
+		/* delay is 200ms * number of tmp05 sensors */
+		udelay(TEMP_COLLECTION_TIME);
+
+		/* done reading temperature sensors */
+		cfg = i2c_smbus_read_byte_data(client, ADT7470_REG_CFG);
+		cfg &= ~0x80;
+		i2c_smbus_write_byte_data(client, ADT7470_REG_CFG, cfg);
+
+		for (i = 0; i < ADT7470_TEMP_COUNT; i++) {
+			data->temp[i] = i2c_smbus_read_byte_data(client, ADT7470_TEMP_REG(i));
+			data->temp_min[i] = i2c_smbus_read_byte_data(client, ADT7470_TEMP_MIN_REG(i));
+			data->temp_max[i] = i2c_smbus_read_byte_data(client, ADT7470_TEMP_MAX_REG(i));
+		}
+
+		for (i = 0; i < ADT7470_FAN_COUNT; i++) {
+			data->fan[i] = adt7470_read_fan(client, i);
+			data->fan_min[i] = adt7470_read_fan_min(client, i);
+			data->fan_max[i] = adt7470_read_fan_max(client, i);
+		}
+
+		for (i = ADT7470_REG_BASE; i <= ADT7470_REG_MAX; i++) {
+			data->raw[i - ADT7470_REG_BASE] = i2c_smbus_read_byte_data(client, i);
+		}
+
+		for (i = 0; i < ADT7470_PWM_COUNT; i++) {
+			int reg = ADT7470_REG_PWM_CFG_BASE + (i / 2);
+			int reg_mask = ADT7470_PWM1_AUTO_MASK;
+			if (!(i % 2))
+				reg_mask <<= 1;
+
+			data->pwm[i] = i2c_smbus_read_byte_data(client, ADT7470_REG_PWM(i));
+			data->pwm_max[i] = i2c_smbus_read_byte_data(client, ADT7470_REG_PWM_MAX(i));
+			data->pwm_min[i] = i2c_smbus_read_byte_data(client, ADT7470_REG_PWM_MIN(i));
+			data->pwm_tmin[i] = i2c_smbus_read_byte_data(client, ADT7470_REG_PWM_TMIN(i));
+			data->pwm_automatic[i] = ((i2c_smbus_read_byte_data(client, reg) & reg_mask) == reg_mask);
+			reg = ADT7470_REG_PWM_AUTO_TEMP_BASE + (i / 2);
+			if (!(i % 2))
+				data->pwm_auto_temp[i] = i2c_smbus_read_byte_data(client, reg) >> 4;
+			else
+				data->pwm_auto_temp[i] = i2c_smbus_read_byte_data(client, reg) & 0xF;
+		}
+
+		data->force_pwm_max = ((i2c_smbus_read_byte_data(client, ADT7470_REG_CFG) & ADT7470_FSPD_MASK) == ADT7470_FSPD_MASK);
+
+		data->alarms = adt7470_read16(client, ADT7470_REG_ALARM1);
+		data->alarms_mask = adt7470_read16(client, ADT7470_REG_ALARM1_MASK);
+
+		data->last_updated = jiffies;
+		data->valid = 1;
+	}
+
+	mutex_unlock(&data->lock);
+
+	return data;
+}
+
+static ssize_t show_temp_min(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct adt7470_data *data = adt7470_update_device(dev);
+	return sprintf(buf, "%d\n", 1000 * data->temp_min[attr->index]);
+}
+
+static ssize_t set_temp_min(struct device *dev, struct device_attribute *devattr,
+			const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7470_data *data = i2c_get_clientdata(client);
+	int temp = simple_strtol(buf, NULL, 10) / 1000;
+
+	mutex_lock(&data->lock);
+	data->temp_min[attr->index] = temp;
+	i2c_smbus_write_byte_data(client, ADT7470_TEMP_MIN_REG(attr->index), temp);
+	mutex_unlock(&data->lock);
+
+	return count;
+}
+
+static ssize_t show_temp_max(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct adt7470_data *data = adt7470_update_device(dev);
+	return sprintf(buf, "%d\n", 1000 * data->temp_max[attr->index]);
+}
+
+static ssize_t set_temp_max(struct device *dev, struct device_attribute *devattr,
+			const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7470_data *data = i2c_get_clientdata(client);
+	int temp = simple_strtol(buf, NULL, 10) / 1000;
+
+	mutex_lock(&data->lock);
+	data->temp_max[attr->index] = temp;
+	i2c_smbus_write_byte_data(client, ADT7470_TEMP_MAX_REG(attr->index), temp);
+	mutex_unlock(&data->lock);
+
+	return count;
+}
+
+static ssize_t show_temp(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct adt7470_data *data = adt7470_update_device(dev);
+	return sprintf(buf, "%d\n", 1000 * data->temp[attr->index]);
+}
+
+static ssize_t show_alarms(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct adt7470_data *data = adt7470_update_device(dev);
+
+	if (attr->index)
+		return sprintf(buf, "%x\n", data->alarms);
+	else
+		return sprintf(buf, "%x\n", data->alarms_mask);
+}
+
+static ssize_t show_fan_max(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct adt7470_data *data = adt7470_update_device(dev);
+
+	if (data->fan_max[attr->index])
+		return sprintf(buf, "%d\n", (90000 * 60) / data->fan_max[attr->index]);
+	else
+		return sprintf(buf, "0\n");
+}
+
+static ssize_t show_fan_min(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct adt7470_data *data = adt7470_update_device(dev);
+
+	if (data->fan_min[attr->index])
+		return sprintf(buf, "%d\n", (90000 * 60) / data->fan_min[attr->index]);
+	else
+		return sprintf(buf, "0\n");
+}
+
+static ssize_t show_fan(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct adt7470_data *data = adt7470_update_device(dev);
+
+	if (data->fan[attr->index] && data->fan[attr->index] != 65535)
+		return sprintf(buf, "%d\n", (90000 * 60) / data->fan[attr->index]);
+	else
+		return sprintf(buf, "0\n");
+}
+
+static ssize_t show_raw(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct adt7470_data *data = adt7470_update_device(dev);
+	int i;
+	char *b = buf;
+
+	for (i = ADT7470_REG_BASE; i <= ADT7470_REG_MAX; i++)
+		b += sprintf(b, "0x%X: 0x%X\n", i, data->raw[i - ADT7470_REG_BASE]);
+
+	return b - buf;
+}
+
+static ssize_t show_force_pwm_max(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct adt7470_data *data = adt7470_update_device(dev);
+	return sprintf(buf, "%d\n", data->force_pwm_max);
+}
+
+static ssize_t set_force_pwm_max(struct device *dev, struct device_attribute *devattr,
+			const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7470_data *data = i2c_get_clientdata(client);
+	int temp = simple_strtol(buf, NULL, 10);
+	u8 reg;
+
+	mutex_lock(&data->lock);
+	data->force_pwm_max = temp;
+	reg = i2c_smbus_read_byte_data(client, ADT7470_REG_CFG);
+	reg &= ~ADT7470_FSPD_MASK;
+	reg |= (temp ? ADT7470_FSPD_MASK : 0);
+	i2c_smbus_write_byte_data(client, ADT7470_REG_CFG, reg);
+	mutex_unlock(&data->lock);
+
+	return count;
+}
+
+static ssize_t show_pwm(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct adt7470_data *data = adt7470_update_device(dev);
+	return sprintf(buf, "%d\n", data->pwm[attr->index]);
+}
+
+static ssize_t set_pwm(struct device *dev, struct device_attribute *devattr,
+			const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7470_data *data = i2c_get_clientdata(client);
+	int temp = simple_strtol(buf, NULL, 10);
+
+	mutex_lock(&data->lock);
+	data->pwm[attr->index] = temp;
+	i2c_smbus_write_byte_data(client, ADT7470_REG_PWM(attr->index), temp);
+	mutex_unlock(&data->lock);
+
+	return count;
+}
+
+static ssize_t show_pwm_max(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct adt7470_data *data = adt7470_update_device(dev);
+	return sprintf(buf, "%d\n", data->pwm_max[attr->index]);
+}
+
+static ssize_t set_pwm_max(struct device *dev, struct device_attribute *devattr,
+			const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7470_data *data = i2c_get_clientdata(client);
+	int temp = simple_strtol(buf, NULL, 10);
+
+	mutex_lock(&data->lock);
+	data->pwm_max[attr->index] = temp;
+	i2c_smbus_write_byte_data(client, ADT7470_REG_PWM_MAX(attr->index), temp);
+	mutex_unlock(&data->lock);
+
+	return count;
+}
+
+static ssize_t show_pwm_min(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct adt7470_data *data = adt7470_update_device(dev);
+	return sprintf(buf, "%d\n", data->pwm_min[attr->index]);
+}
+
+static ssize_t set_pwm_min(struct device *dev, struct device_attribute *devattr,
+			const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7470_data *data = i2c_get_clientdata(client);
+	int temp = simple_strtol(buf, NULL, 10);
+
+	mutex_lock(&data->lock);
+	data->pwm_min[attr->index] = temp;
+	i2c_smbus_write_byte_data(client, ADT7470_REG_PWM_MIN(attr->index), temp);
+	mutex_unlock(&data->lock);
+
+	return count;
+}
+
+static ssize_t show_pwm_tmax(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct adt7470_data *data = adt7470_update_device(dev);
+	return sprintf(buf, "%d\n", 1000 * (20 + data->pwm_tmin[attr->index]));
+}
+
+static ssize_t show_pwm_tmin(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct adt7470_data *data = adt7470_update_device(dev);
+	return sprintf(buf, "%d\n", 1000 * data->pwm_tmin[attr->index]);
+}
+
+static ssize_t set_pwm_tmin(struct device *dev, struct device_attribute *devattr,
+			const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7470_data *data = i2c_get_clientdata(client);
+	int temp = simple_strtol(buf, NULL, 10) / 1000;
+
+	mutex_lock(&data->lock);
+	data->pwm_tmin[attr->index] = temp;
+	i2c_smbus_write_byte_data(client, ADT7470_REG_PWM_TMIN(attr->index), temp);
+	mutex_unlock(&data->lock);
+
+	return count;
+}
+
+static ssize_t show_pwm_auto(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct adt7470_data *data = adt7470_update_device(dev);
+	return sprintf(buf, "%d\n", 1 + data->pwm_automatic[attr->index]);
+}
+
+static ssize_t set_pwm_auto(struct device *dev, struct device_attribute *devattr,
+			const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7470_data *data = i2c_get_clientdata(client);
+	int temp = simple_strtol(buf, NULL, 10);
+	int pwm_auto_reg = ADT7470_REG_PWM_CFG_BASE + (attr->index / 2);
+	int pwm_auto_reg_mask = ADT7470_PWM1_AUTO_MASK;
+	u8 reg;
+
+	if (!(attr->index % 2))
+		pwm_auto_reg_mask <<= 1;
+
+	if (temp != 2 && temp != 1)
+		return -EINVAL;
+	temp--;
+
+	mutex_lock(&data->lock);
+	data->pwm_automatic[attr->index] = temp;
+	reg = i2c_smbus_read_byte_data(client, pwm_auto_reg);
+	reg &= ~pwm_auto_reg_mask;
+	reg |= (temp ? pwm_auto_reg_mask : 0);
+	i2c_smbus_write_byte_data(client, pwm_auto_reg, reg);
+	mutex_unlock(&data->lock);
+
+	return count;
+}
+
+static ssize_t show_pwm_auto_temp(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct adt7470_data *data = adt7470_update_device(dev);
+	u8 ctrl = data->pwm_auto_temp[attr->index];
+
+	if (ctrl)
+		return sprintf(buf, "%d\n", 1 << (ctrl - 1));
+	else
+		return sprintf(buf, "%d\n", ADT7470_PWM_ALL_TEMPS);
+}
+
+static int cvt_auto_temp(int input)
+{
+	if (input == ADT7470_PWM_ALL_TEMPS)
+		return 0;
+	if (input < 1 || !power_of_2(input))
+		return -EINVAL;
+	return ilog2(input) + 1;
+}
+
+static ssize_t set_pwm_auto_temp(struct device *dev, struct device_attribute *devattr,
+			const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7470_data *data = i2c_get_clientdata(client);
+	int temp = cvt_auto_temp(simple_strtol(buf, NULL, 10));
+	int pwm_auto_reg = ADT7470_REG_PWM_AUTO_TEMP_BASE + (attr->index / 2);
+	u8 reg;
+
+	if (temp < 0)
+		return temp;
+
+	mutex_lock(&data->lock);
+	data->pwm_automatic[attr->index] = temp;
+	reg = i2c_smbus_read_byte_data(client, pwm_auto_reg);
+
+	if (!(attr->index % 2)) {
+		reg &= 0xF;
+		reg |= (temp << 4) & 0xF0;
+	} else {
+		reg &= 0xF0;
+		reg |= temp & 0xF;
+	}
+
+	i2c_smbus_write_byte_data(client, pwm_auto_reg, reg);
+	mutex_unlock(&data->lock);
+
+	return count;
+}
+
+static SENSOR_DEVICE_ATTR(alarms, S_IRUGO, show_alarms, NULL, 0);
+static SENSOR_DEVICE_ATTR(alarm_mask, S_IRUGO, show_alarms, NULL, 1);
+
+static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 0);
+static SENSOR_DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 1);
+static SENSOR_DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 2);
+static SENSOR_DEVICE_ATTR(temp4_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 3);
+static SENSOR_DEVICE_ATTR(temp5_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 4);
+static SENSOR_DEVICE_ATTR(temp6_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 5);
+static SENSOR_DEVICE_ATTR(temp7_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 6);
+static SENSOR_DEVICE_ATTR(temp8_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 7);
+static SENSOR_DEVICE_ATTR(temp9_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 8);
+static SENSOR_DEVICE_ATTR(temp10_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 9);
+
+static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 0);
+static SENSOR_DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 1);
+static SENSOR_DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 2);
+static SENSOR_DEVICE_ATTR(temp4_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 3);
+static SENSOR_DEVICE_ATTR(temp5_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 4);
+static SENSOR_DEVICE_ATTR(temp6_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 5);
+static SENSOR_DEVICE_ATTR(temp7_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 6);
+static SENSOR_DEVICE_ATTR(temp8_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 7);
+static SENSOR_DEVICE_ATTR(temp9_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 8);
+static SENSOR_DEVICE_ATTR(temp10_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 9);
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp, NULL, 1);
+static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_temp, NULL, 2);
+static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, show_temp, NULL, 3);
+static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, show_temp, NULL, 4);
+static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, show_temp, NULL, 5);
+static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, show_temp, NULL, 6);
+static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, show_temp, NULL, 7);
+static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, show_temp, NULL, 8);
+static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, show_temp, NULL, 9);
+
+static SENSOR_DEVICE_ATTR(fan1_max, S_IRUGO, show_fan_max, NULL, 0);
+static SENSOR_DEVICE_ATTR(fan2_max, S_IRUGO, show_fan_max, NULL, 1);
+static SENSOR_DEVICE_ATTR(fan3_max, S_IRUGO, show_fan_max, NULL, 2);
+static SENSOR_DEVICE_ATTR(fan4_max, S_IRUGO, show_fan_max, NULL, 3);
+
+static SENSOR_DEVICE_ATTR(fan1_min, S_IRUGO, show_fan_min, NULL, 0);
+static SENSOR_DEVICE_ATTR(fan2_min, S_IRUGO, show_fan_min, NULL, 1);
+static SENSOR_DEVICE_ATTR(fan3_min, S_IRUGO, show_fan_min, NULL, 2);
+static SENSOR_DEVICE_ATTR(fan4_min, S_IRUGO, show_fan_min, NULL, 3);
+
+static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, show_fan, NULL, 0);
+static SENSOR_DEVICE_ATTR(fan2_input, S_IRUGO, show_fan, NULL, 1);
+static SENSOR_DEVICE_ATTR(fan3_input, S_IRUGO, show_fan, NULL, 2);
+static SENSOR_DEVICE_ATTR(fan4_input, S_IRUGO, show_fan, NULL, 3);
+
+static SENSOR_DEVICE_ATTR(raw_input, S_IRUGO, show_raw, NULL, 0);
+static SENSOR_DEVICE_ATTR(force_pwm_max, S_IWUSR | S_IRUGO, show_force_pwm_max, set_force_pwm_max, 0);
+
+static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm, set_pwm, 0);
+static SENSOR_DEVICE_ATTR(pwm2, S_IWUSR | S_IRUGO, show_pwm, set_pwm, 1);
+static SENSOR_DEVICE_ATTR(pwm3, S_IWUSR | S_IRUGO, show_pwm, set_pwm, 2);
+static SENSOR_DEVICE_ATTR(pwm4, S_IWUSR | S_IRUGO, show_pwm, set_pwm, 3);
+
+static SENSOR_DEVICE_ATTR(pwm1_auto_point1_pwm, S_IWUSR | S_IRUGO, show_pwm_min, set_pwm_min, 0);
+static SENSOR_DEVICE_ATTR(pwm2_auto_point1_pwm, S_IWUSR | S_IRUGO, show_pwm_min, set_pwm_min, 1);
+static SENSOR_DEVICE_ATTR(pwm3_auto_point1_pwm, S_IWUSR | S_IRUGO, show_pwm_min, set_pwm_min, 2);
+static SENSOR_DEVICE_ATTR(pwm4_auto_point1_pwm, S_IWUSR | S_IRUGO, show_pwm_min, set_pwm_min, 3);
+
+static SENSOR_DEVICE_ATTR(pwm1_auto_point2_pwm, S_IWUSR | S_IRUGO, show_pwm_max, set_pwm_max, 0);
+static SENSOR_DEVICE_ATTR(pwm2_auto_point2_pwm, S_IWUSR | S_IRUGO, show_pwm_max, set_pwm_max, 1);
+static SENSOR_DEVICE_ATTR(pwm3_auto_point2_pwm, S_IWUSR | S_IRUGO, show_pwm_max, set_pwm_max, 2);
+static SENSOR_DEVICE_ATTR(pwm4_auto_point2_pwm, S_IWUSR | S_IRUGO, show_pwm_max, set_pwm_max, 3);
+
+static SENSOR_DEVICE_ATTR(pwm1_auto_point1_temp, S_IWUSR | S_IRUGO, show_pwm_tmin, set_pwm_tmin, 0);
+static SENSOR_DEVICE_ATTR(pwm2_auto_point1_temp, S_IWUSR | S_IRUGO, show_pwm_tmin, set_pwm_tmin, 1);
+static SENSOR_DEVICE_ATTR(pwm3_auto_point1_temp, S_IWUSR | S_IRUGO, show_pwm_tmin, set_pwm_tmin, 2);
+static SENSOR_DEVICE_ATTR(pwm4_auto_point1_temp, S_IWUSR | S_IRUGO, show_pwm_tmin, set_pwm_tmin, 3);
+
+static SENSOR_DEVICE_ATTR(pwm1_auto_point2_temp, S_IRUGO, show_pwm_tmax, NULL, 0);
+static SENSOR_DEVICE_ATTR(pwm2_auto_point2_temp, S_IRUGO, show_pwm_tmax, NULL, 1);
+static SENSOR_DEVICE_ATTR(pwm3_auto_point2_temp, S_IRUGO, show_pwm_tmax, NULL, 2);
+static SENSOR_DEVICE_ATTR(pwm4_auto_point2_temp, S_IRUGO, show_pwm_tmax, NULL, 3);
+
+static SENSOR_DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO, show_pwm_auto, set_pwm_auto, 0);
+static SENSOR_DEVICE_ATTR(pwm2_enable, S_IWUSR | S_IRUGO, show_pwm_auto, set_pwm_auto, 1);
+static SENSOR_DEVICE_ATTR(pwm3_enable, S_IWUSR | S_IRUGO, show_pwm_auto, set_pwm_auto, 2);
+static SENSOR_DEVICE_ATTR(pwm4_enable, S_IWUSR | S_IRUGO, show_pwm_auto, set_pwm_auto, 3);
+
+static SENSOR_DEVICE_ATTR(pwm1_auto_channels_temp, S_IWUSR | S_IRUGO, show_pwm_auto_temp, set_pwm_auto_temp, 0);
+static SENSOR_DEVICE_ATTR(pwm2_auto_channels_temp, S_IWUSR | S_IRUGO, show_pwm_auto_temp, set_pwm_auto_temp, 1);
+static SENSOR_DEVICE_ATTR(pwm3_auto_channels_temp, S_IWUSR | S_IRUGO, show_pwm_auto_temp, set_pwm_auto_temp, 2);
+static SENSOR_DEVICE_ATTR(pwm4_auto_channels_temp, S_IWUSR | S_IRUGO, show_pwm_auto_temp, set_pwm_auto_temp, 3);
+
+static int adt7470_attach_adapter(struct i2c_adapter *adapter)
+{
+	if (!(adapter->class & I2C_CLASS_HWMON))
+		return 0;
+	return i2c_probe(adapter, &addr_data, adt7470_detect);
+}
+
+static struct attribute *adt7470_attributes[] = {
+	&sensor_dev_attr_force_pwm_max.dev_attr.attr,
+	&sensor_dev_attr_alarms.dev_attr.attr,
+	&sensor_dev_attr_alarm_mask.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_temp2_max.dev_attr.attr,
+	&sensor_dev_attr_temp3_max.dev_attr.attr,
+	&sensor_dev_attr_temp4_max.dev_attr.attr,
+	&sensor_dev_attr_temp5_max.dev_attr.attr,
+	&sensor_dev_attr_temp6_max.dev_attr.attr,
+	&sensor_dev_attr_temp7_max.dev_attr.attr,
+	&sensor_dev_attr_temp8_max.dev_attr.attr,
+	&sensor_dev_attr_temp9_max.dev_attr.attr,
+	&sensor_dev_attr_temp10_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
+	&sensor_dev_attr_temp2_min.dev_attr.attr,
+	&sensor_dev_attr_temp3_min.dev_attr.attr,
+	&sensor_dev_attr_temp4_min.dev_attr.attr,
+	&sensor_dev_attr_temp5_min.dev_attr.attr,
+	&sensor_dev_attr_temp6_min.dev_attr.attr,
+	&sensor_dev_attr_temp7_min.dev_attr.attr,
+	&sensor_dev_attr_temp8_min.dev_attr.attr,
+	&sensor_dev_attr_temp9_min.dev_attr.attr,
+	&sensor_dev_attr_temp10_min.dev_attr.attr,
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp2_input.dev_attr.attr,
+	&sensor_dev_attr_temp3_input.dev_attr.attr,
+	&sensor_dev_attr_temp4_input.dev_attr.attr,
+	&sensor_dev_attr_temp5_input.dev_attr.attr,
+	&sensor_dev_attr_temp6_input.dev_attr.attr,
+	&sensor_dev_attr_temp7_input.dev_attr.attr,
+	&sensor_dev_attr_temp8_input.dev_attr.attr,
+	&sensor_dev_attr_temp9_input.dev_attr.attr,
+	&sensor_dev_attr_temp10_input.dev_attr.attr,
+	&sensor_dev_attr_fan1_max.dev_attr.attr,
+	&sensor_dev_attr_fan2_max.dev_attr.attr,
+	&sensor_dev_attr_fan3_max.dev_attr.attr,
+	&sensor_dev_attr_fan4_max.dev_attr.attr,
+	&sensor_dev_attr_fan1_min.dev_attr.attr,
+	&sensor_dev_attr_fan2_min.dev_attr.attr,
+	&sensor_dev_attr_fan3_min.dev_attr.attr,
+	&sensor_dev_attr_fan4_min.dev_attr.attr,
+	&sensor_dev_attr_fan1_input.dev_attr.attr,
+	&sensor_dev_attr_fan2_input.dev_attr.attr,
+	&sensor_dev_attr_fan3_input.dev_attr.attr,
+	&sensor_dev_attr_fan4_input.dev_attr.attr,
+	&sensor_dev_attr_raw_input.dev_attr.attr,
+	&sensor_dev_attr_pwm1.dev_attr.attr,
+	&sensor_dev_attr_pwm2.dev_attr.attr,
+	&sensor_dev_attr_pwm3.dev_attr.attr,
+	&sensor_dev_attr_pwm4.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm3_auto_point1_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm4_auto_point1_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point2_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm3_auto_point2_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm4_auto_point2_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point1_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm3_auto_point1_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm4_auto_point1_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point2_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_point2_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm3_auto_point2_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm4_auto_point2_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm2_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm3_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm4_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_channels_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm2_auto_channels_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm3_auto_channels_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm4_auto_channels_temp.dev_attr.attr,
+	NULL
+};
+
+static int adt7470_detect(struct i2c_adapter *adapter, int address, int kind)
+{
+	struct i2c_client *client;
+	struct adt7470_data *data;
+	int err = 0;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_WORD_DATA))
+		goto exit;
+
+	if (!(data = kzalloc(sizeof(struct adt7470_data), GFP_KERNEL))) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	client = &data->client;
+	client->addr = address;
+	client->adapter = adapter;
+	client->driver = &adt7470_driver;
+
+	i2c_set_clientdata(client, data);
+
+	mutex_init(&data->lock);
+
+	if (kind <= 0) {
+		int vendor, device, revision;
+
+		vendor = i2c_smbus_read_byte_data(client, ADT7470_REG_VENDOR);
+		if (vendor != ADT7470_VENDOR) {
+			printk(KERN_ERR "%s: vendor=%x\n", __FUNCTION__, vendor);
+			err = -ENODEV;
+			goto exit_free;
+		}
+
+		device = i2c_smbus_read_byte_data(client, ADT7470_REG_DEVICE);
+		if (device != ADT7470_DEVICE) {
+			printk(KERN_ERR "%s: device=%x\n", __FUNCTION__, device);
+			err = -ENODEV;
+			goto exit_free;
+		}
+
+		revision = i2c_smbus_read_byte_data(client, ADT7470_REG_REVISION);
+		if (revision != ADT7470_REVISION) {
+			printk(KERN_ERR "%s: revision=%x\n", __FUNCTION__, revision);
+			err = -ENODEV;
+			goto exit_free;
+		}
+
+		data->type = adt7470;
+	} else {
+		dev_dbg(&adapter->dev, "detection forced\n");
+	}
+
+	if (kind > 0)
+		data->type = kind;
+
+	data->attrs.attrs = adt7470_attributes;
+	strlcpy(client->name, "adt7470", I2C_NAME_SIZE);
+
+	if ((err = i2c_attach_client(client)))
+		goto exit_free;
+
+	dev_info(&client->dev, "%s chip found\n", client->name);
+
+	/* Initialize the ADT7470 chip */
+	adt7470_init_client(client);
+
+	/* Register sysfs hooks */
+	if ((err = sysfs_create_group(&client->dev.kobj, &data->attrs)))
+		goto exit_detach;
+
+	data->class_dev = hwmon_device_register(&client->dev);
+	if (IS_ERR(data->class_dev)) {
+		err = PTR_ERR(data->class_dev);
+		goto exit_remove;
+	}
+
+	return 0;
+
+exit_remove:
+	sysfs_remove_group(&client->dev.kobj, &data->attrs);
+exit_detach:
+	i2c_detach_client(client);
+exit_free:
+	kfree(data);
+exit:
+	return err;
+}
+
+static int adt7470_detach_client(struct i2c_client *client)
+{
+	struct adt7470_data *data = i2c_get_clientdata(client);
+	hwmon_device_unregister(data->class_dev);
+	sysfs_remove_group(&client->dev.kobj, &data->attrs);
+	i2c_detach_client(client);
+	kfree(data);
+	return 0;
+}
+
+static int __init adt7470_init(void)
+{
+	return i2c_add_driver(&adt7470_driver);
+}
+
+static void __exit adt7470_exit(void)
+{
+	i2c_del_driver(&adt7470_driver);
+}
+
+MODULE_AUTHOR("Darrick J. Wong <djwong@us.ibm.com>");
+MODULE_DESCRIPTION("ADT7470 driver");
+MODULE_LICENSE("GPL");
+MODULE_VERSION(DRV_VERSION);
+
+module_init(adt7470_init);
+module_exit(adt7470_exit);

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Hans de Goede]

Darrick J. Wong wrote:
> Hi there,
> 
> Below is a first draft of a driver for the ADT7470 chip.  I've tested
> the driver against the adt7470 in the IBM IntelliStation Z30 and as far
> as I can tell it's good for controlling the speed of the CPU heatsink
> fans and monitor CPU temperature.  Please let me know what you think of
> the driver.  The patch should apply cleanly against 2.6.22-rc7.
> 

Hi,

Below is a review of your driver.

> --- /dev/null
> +++ b/drivers/hwmon/adt7470.c
> @@ -0,0 +1,875 @@
> +/*
> + * A hwmon driver for the Analog Devices ADT7470
> + * Copyright (C) 2007 IBM
> + *
> + * Author: Darrick J. Wong <djwong@us.ibm.com>
> + *
> + * 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 - version 2.
> + */
> +

Are you sure you want this to be GPL version 2 only? The kernel as a whole is 
GPL version 2 only, but many drivers / subsystems have the "or any later 
version" language, this will make it much easier for the kerenl to go GPL v3, 
if the kernel community ever decides todo so.

> +#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/delay.h>
> +#include <linux/log2.h>
> +
> +#define DRV_VERSION "0.22"
> +

No driver version defines and prints please. Once the driver is in the kernel 
it may seem fixes by others too and your own versioning + changelog will become 
stale and thus useless.

> +/* Addresses to scan */
> +static unsigned short normal_i2c[] = { 0x2C, 0x2E, 0x2F, I2C_CLIENT_END };
> +
> +/* Insmod parameters */
> +I2C_CLIENT_INSMOD_1(adt7470);
> +
> +/* ADT7470 registers */
> +#define ADT7470_REG_BASE		0x20
> +#define ADT7470_REG_TEMP_BASE		0x20
> +#define ADT7470_REG_TEMP_MAX		0x29
> +#define ADT7470_REG_FAN_BASE		0x2A
> +#define ADT7470_REG_FAN_MAX		0x31
> +#define ADT7470_REG_PWM_BASE		0x32
> +//#define ADT7470_REG_PWM_MAX		0x35

No c++ style comments please, also even if this is not used its fine to leave 
it uncommented.

> +#define ADT7470_REG_PWM_MAX_BASE	0x38
> +#define ADT7470_REG_PWM_MAX_MAX		0x3B
> +#define ADT7470_REG_CFG			0x40
> +#define		ADT7470_FSPD_MASK	0x04
> +#define ADT7470_REG_ALARM1		0x41
> +#define ADT7470_REG_ALARM2		0x42
> +#define ADT7470_REG_TEMP_LIMITS_BASE	0x44
> +#define ADT7470_REG_TEMP_LIMITS_MAX	0x57
> +#define ADT7470_REG_FAN_MIN_BASE	0x58
> +#define ADT7470_REG_FAN_MIN_MAX		0x5F
> +#define ADT7470_REG_FAN_MAX_BASE	0x60
> +#define ADT7470_REG_FAN_MAX_MAX		0x67
> +#define ADT7470_REG_PWM_CFG_BASE	0x68
> +#define ADT7470_REG_PWM12_CFG		0x68
> +#define		ADT7470_PWM1_AUTO_MASK	0x40
> +#define		ADT7470_PWM2_AUTO_MASK	0x80
> +#define ADT7470_REG_PWM34_CFG		0x69
> +#define		ADT7470_PWM3_AUTO_MASK	0x40
> +#define		ADT7470_PWM4_AUTO_MASK	0x80
> +#define	ADT7470_REG_PWM_MIN_BASE	0x6A
> +#define ADT7470_REG_PWM_MIN_MAX		0x6D
> +#define ADT7470_REG_PWM_TEMP_MIN_BASE	0x6E
> +#define ADT7470_REG_PWM_TEMP_MIN_MAX	0x71
> +#define ADT7470_REG_ACOUSTICS12		0x75
> +#define ADT7470_REG_ACOUSTICS34		0x76
> +#define ADT7470_REG_DEVICE		0x3D
> +#define ADT7470_REG_VENDOR		0x3E
> +#define ADT7470_REG_REVISION		0x3F
> +#define ADT7470_REG_ALARM1_MASK		0x72
> +#define ADT7470_REG_ALARM2_MASK		0x73
> +#define ADT7470_REG_PWM_AUTO_TEMP_BASE	0x7C
> +#define ADT7470_REG_PWM_AUTO_TEMP_MAX	0x7D
> +#define ADT7470_REG_MAX			0x81
> +
> +#define ADT7470_TEMP_COUNT	10
> +#define ADT7470_TEMP_REG(x)	(ADT7470_REG_TEMP_BASE + (x))
> +#define ADT7470_TEMP_MIN_REG(x) (ADT7470_REG_TEMP_LIMITS_BASE + (2 * (x)))
> +#define ADT7470_TEMP_MAX_REG(x) (ADT7470_REG_TEMP_LIMITS_BASE + (2 * (x)) + 1)
> +
> +#define ADT7470_FAN_COUNT	4
> +
> +#define ADT7470_PWM_COUNT	4
> +#define ADT7470_REG_PWM(x)	(ADT7470_REG_PWM_BASE + (x))
> +#define ADT7470_REG_PWM_MAX(x)	(ADT7470_REG_PWM_MAX_BASE + (x))
> +#define ADT7470_REG_PWM_MIN(x)	(ADT7470_REG_PWM_MIN_BASE + (x))
> +#define ADT7470_REG_PWM_TMIN(x)	(ADT7470_REG_PWM_TEMP_MIN_BASE + (x))
> +

Why all the base defines? Why not put the base address directly in the () 
macros? You do not seem to use them anywhere else.

> +#define ADT7470_VENDOR		0x41
> +#define ADT7470_DEVICE		0x70
> +#define ADT7470_REVISION	0x02
> +

Are you sure you only want to support revision 2? What are the differences? 
Shouldn't the driver be able to support other revisions too? Maybe only print a 
warning when its used with an untested revision?

> +#define ADT7470_PWM_ALL_TEMPS	0x3FF		/* "all temps" according to hwmon sysfs interface spec */
> +
> +#define REFRESH_INTERVAL	(2 * HZ)
> +#define TEMP_COLLECTION_TIME	1000		/* sleep 1s while gathering temperature data */
> +

Keep lines within 80 chars please, put long comments above the defines.

> +#define power_of_2(x)	(((x) & ((x) - 1)) = 0)
> +
> +struct adt7470_data {
> +	struct i2c_client	client;
> +	struct class_device	*class_dev;
> +	struct attribute_group	attrs;
> +	enum chips		type;

You do not seem to have an enum chips defined and you do not use type anywhere, 
maybe this can be removed?

> +	struct mutex		lock;
> +	char			valid;
> +	unsigned long		last_updated;	/* In jiffies */
> +
> +	u8			temp[ADT7470_TEMP_COUNT];
> +	u8			temp_min[ADT7470_TEMP_COUNT];
> +	u8			temp_max[ADT7470_TEMP_COUNT];
> +	u16			fan[ADT7470_FAN_COUNT];
> +	u16			fan_min[ADT7470_FAN_COUNT];
> +	u16			fan_max[ADT7470_FAN_COUNT];
> +	u16			alarms, alarms_mask;
> +	u8			force_pwm_max;
> +	u8			pwm[ADT7470_PWM_COUNT];
> +	u8			pwm_max[ADT7470_PWM_COUNT];
> +	u8			pwm_automatic[ADT7470_PWM_COUNT];
> +	u8			pwm_min[ADT7470_PWM_COUNT];
> +	u8			pwm_tmin[ADT7470_PWM_COUNT];
> +	u8			pwm_auto_temp[ADT7470_PWM_COUNT];
> +
> +	u8			raw[256];
> +};
> +
> +static int adt7470_attach_adapter(struct i2c_adapter *adapter);
> +static int adt7470_detect(struct i2c_adapter *adapter, int address, int kind);
> +static int adt7470_detach_client(struct i2c_client *client);
> +
> +static struct i2c_driver adt7470_driver = {
> +	.driver = {
> +		.name	= "adt7470",
> +	},
> +	.attach_adapter	= adt7470_attach_adapter,
> +	.detach_client	= adt7470_detach_client,
> +};
> +
> +/* 16-bit registers on the ADT7470 are low-byte first. */
> +static inline int adt7470_read16(struct i2c_client *client, u8 reg)
> +{
> +	u16 foo;
> +	foo = i2c_smbus_read_byte_data(client, reg) | ((u16)i2c_smbus_read_byte_data(client, reg + 1) << 8);
> +	return foo;
> +}
> +
> +static inline int adt7470_write16(struct i2c_client *client, u8 reg, u16 value)
> +{
> +	return i2c_smbus_write_word_data(client, reg, cpu_to_le16(value));
> +}
> +

Why use read_byte twice in read16, but write_word in write16?

> +static void adt7470_init_client(struct i2c_client *client)
> +{
> +	//struct adt7470_data *data = i2c_get_clientdata(client);
> +

Again no c+= style comments please, and why not just remove this line?

> +	int reg = i2c_smbus_read_byte_data(client, ADT7470_REG_CFG);

Insert a white line between declarations and statements please

> +	if (reg < 0) {
> +		dev_err(&client->dev, "cannot read configuration register\n");
> +	} else {
> +		// start monitoring (and do a self-test)

No c++ style comments please

> +		i2c_smbus_write_byte_data(client, ADT7470_REG_CFG, reg | 3);
> +	}
> +}
> +
> +static u16 adt7470_read_fan_min(struct i2c_client *client, int fan)
> +{
> +	return adt7470_read16(client, ADT7470_REG_FAN_MIN_BASE + (2 * fan));
> +}
> +
> +static u16 adt7470_read_fan_max(struct i2c_client *client, int fan)
> +{
> +	return adt7470_read16(client, ADT7470_REG_FAN_MAX_BASE + (2 * fan));
> +}
> +
> +static u16 adt7470_read_fan(struct i2c_client *client, int fan)
> +{
> +	return adt7470_read16(client, ADT7470_REG_FAN_BASE + (2 * fan));
> +}
> +

Why not have ADT7470_REG_FAN()  ADT7470_REG_FAN_MIN() and ADT7470_REG_FAN_MAX() 
macros, like you have for example ADT7470_TEMP_REG(x), ADT7470_TEMP_MIN_REG()

Also notice that your not being consistent with where you put the REG please 
either put the REG always directly behind ADT7470_ or always at the end.

Also why have these functions at all, why not put the single line the consist 
of directly at the place where you call them (especially if you have 
ADT7470_REG_FAN() & friends macros

> +static struct adt7470_data *adt7470_update_device(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct adt7470_data *data = i2c_get_clientdata(client);
> +
> +	mutex_lock(&data->lock);
> +
> +	if (time_after(jiffies, data->last_updated + REFRESH_INTERVAL)
> +		|| !data->valid) {
> +		u8 cfg;
> +		int i;
> +
> +		/* start reading temperature sensors */
> +		cfg = i2c_smbus_read_byte_data(client, ADT7470_REG_CFG);
> +		cfg |= 0x80;
> +		i2c_smbus_write_byte_data(client, ADT7470_REG_CFG, cfg);
> +
> +		/* delay is 200ms * number of tmp05 sensors */
> +		udelay(TEMP_COLLECTION_TIME);
> +

ugh, sleep here please.

> +		/* done reading temperature sensors */
> +		cfg = i2c_smbus_read_byte_data(client, ADT7470_REG_CFG);
> +		cfg &= ~0x80;
> +		i2c_smbus_write_byte_data(client, ADT7470_REG_CFG, cfg);
> +
> +		for (i = 0; i < ADT7470_TEMP_COUNT; i++) {
> +			data->temp[i] = i2c_smbus_read_byte_data(client, ADT7470_TEMP_REG(i));
> +			data->temp_min[i] = i2c_smbus_read_byte_data(client, ADT7470_TEMP_MIN_REG(i));
> +			data->temp_max[i] = i2c_smbus_read_byte_data(client, ADT7470_TEMP_MAX_REG(i));
> +		}
> +
> +		for (i = 0; i < ADT7470_FAN_COUNT; i++) {
> +			data->fan[i] = adt7470_read_fan(client, i);
> +			data->fan_min[i] = adt7470_read_fan_min(client, i);
> +			data->fan_max[i] = adt7470_read_fan_max(client, i);
> +		}
> +

again, you're inconsistent, notice how you use i2c_smbus_read_byte_data 
directly for the temp sensors, but utility functions for the fans

> +		for (i = ADT7470_REG_BASE; i <= ADT7470_REG_MAX; i++) {
> +			data->raw[i - ADT7470_REG_BASE] = i2c_smbus_read_byte_data(client, i);
> +		}
> +
> +		for (i = 0; i < ADT7470_PWM_COUNT; i++) {
> +			int reg = ADT7470_REG_PWM_CFG_BASE + (i / 2);
> +			int reg_mask = ADT7470_PWM1_AUTO_MASK;
> +			if (!(i % 2))
> +				reg_mask <<= 1;
> +
> +			data->pwm[i] = i2c_smbus_read_byte_data(client, ADT7470_REG_PWM(i));
> +			data->pwm_max[i] = i2c_smbus_read_byte_data(client, ADT7470_REG_PWM_MAX(i));
> +			data->pwm_min[i] = i2c_smbus_read_byte_data(client, ADT7470_REG_PWM_MIN(i));
> +			data->pwm_tmin[i] = i2c_smbus_read_byte_data(client, ADT7470_REG_PWM_TMIN(i));
> +			data->pwm_automatic[i] = ((i2c_smbus_read_byte_data(client, reg) & reg_mask) = reg_mask);
> +			reg = ADT7470_REG_PWM_AUTO_TEMP_BASE + (i / 2);
> +			if (!(i % 2))
> +				data->pwm_auto_temp[i] = i2c_smbus_read_byte_data(client, reg) >> 4;
> +			else
> +				data->pwm_auto_temp[i] = i2c_smbus_read_byte_data(client, reg) & 0xF;
> +		}
> +
> +		data->force_pwm_max = ((i2c_smbus_read_byte_data(client, ADT7470_REG_CFG) & ADT7470_FSPD_MASK) = ADT7470_FSPD_MASK);
> +
> +		data->alarms = adt7470_read16(client, ADT7470_REG_ALARM1);
> +		data->alarms_mask = adt7470_read16(client, ADT7470_REG_ALARM1_MASK);
> +
> +		data->last_updated = jiffies;
> +		data->valid = 1;
> +	}
> +
> +	mutex_unlock(&data->lock);
> +
> +	return data;
> +}
> +
> +static ssize_t show_temp_min(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct adt7470_data *data = adt7470_update_device(dev);
> +	return sprintf(buf, "%d\n", 1000 * data->temp_min[attr->index]);
> +}
> +
> +static ssize_t set_temp_min(struct device *dev, struct device_attribute *devattr,
> +			const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct adt7470_data *data = i2c_get_clientdata(client);
> +	int temp = simple_strtol(buf, NULL, 10) / 1000;
> +
> +	mutex_lock(&data->lock);
> +	data->temp_min[attr->index] = temp;
> +	i2c_smbus_write_byte_data(client, ADT7470_TEMP_MIN_REG(attr->index), temp);
> +	mutex_unlock(&data->lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_temp_max(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct adt7470_data *data = adt7470_update_device(dev);
> +	return sprintf(buf, "%d\n", 1000 * data->temp_max[attr->index]);
> +}
> +
> +static ssize_t set_temp_max(struct device *dev, struct device_attribute *devattr,
> +			const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct adt7470_data *data = i2c_get_clientdata(client);
> +	int temp = simple_strtol(buf, NULL, 10) / 1000;
> +
> +	mutex_lock(&data->lock);
> +	data->temp_max[attr->index] = temp;
> +	i2c_smbus_write_byte_data(client, ADT7470_TEMP_MAX_REG(attr->index), temp);
> +	mutex_unlock(&data->lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_temp(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct adt7470_data *data = adt7470_update_device(dev);
> +	return sprintf(buf, "%d\n", 1000 * data->temp[attr->index]);
> +}
> +
> +static ssize_t show_alarms(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct adt7470_data *data = adt7470_update_device(dev);
> +
> +	if (attr->index)
> +		return sprintf(buf, "%x\n", data->alarms);
> +	else
> +		return sprintf(buf, "%x\n", data->alarms_mask);
> +}
> +
> +static ssize_t show_fan_max(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct adt7470_data *data = adt7470_update_device(dev);
> +
> +	if (data->fan_max[attr->index])
> +		return sprintf(buf, "%d\n", (90000 * 60) / data->fan_max[attr->index]);
> +	else
> +		return sprintf(buf, "0\n");
> +}
> +
> +static ssize_t show_fan_min(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct adt7470_data *data = adt7470_update_device(dev);
> +
> +	if (data->fan_min[attr->index])
> +		return sprintf(buf, "%d\n", (90000 * 60) / data->fan_min[attr->index]);
> +	else
> +		return sprintf(buf, "0\n");
> +}
> +

This may be a stupid question (I haven't fully read the datasheet) but why no 
store functions for fan_min and fan_max?

> +static ssize_t show_fan(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct adt7470_data *data = adt7470_update_device(dev);
> +
> +	if (data->fan[attr->index] && data->fan[attr->index] != 65535)
> +		return sprintf(buf, "%d\n", (90000 * 60) / data->fan[attr->index]);
> +	else
> +		return sprintf(buf, "0\n");
> +}
> +
> +static ssize_t show_raw(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct adt7470_data *data = adt7470_update_device(dev);
> +	int i;
> +	char *b = buf;
> +
> +	for (i = ADT7470_REG_BASE; i <= ADT7470_REG_MAX; i++)
> +		b += sprintf(b, "0x%X: 0x%X\n", i, data->raw[i - ADT7470_REG_BASE]);
> +
> +	return b - buf;
> +}
> +

Don't do this please, we have i2cdump for this

> +static ssize_t show_force_pwm_max(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct adt7470_data *data = adt7470_update_device(dev);
> +	return sprintf(buf, "%d\n", data->force_pwm_max);
> +}
> +
> +static ssize_t set_force_pwm_max(struct device *dev, struct device_attribute *devattr,
> +			const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct adt7470_data *data = i2c_get_clientdata(client);
> +	int temp = simple_strtol(buf, NULL, 10);
> +	u8 reg;
> +
> +	mutex_lock(&data->lock);
> +	data->force_pwm_max = temp;
> +	reg = i2c_smbus_read_byte_data(client, ADT7470_REG_CFG);

> +	reg &= ~ADT7470_FSPD_MASK;
> +	reg |= (temp ? ADT7470_FSPD_MASK : 0);

I think this would be (much) easier to read as:
if (temp)
   reg |= ADT7470_FSPD_MASK;
else
   reg &= ~ADT7470_FSPD_MASK;

> +	i2c_smbus_write_byte_data(client, ADT7470_REG_CFG, reg);
> +	mutex_unlock(&data->lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_pwm(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct adt7470_data *data = adt7470_update_device(dev);
> +	return sprintf(buf, "%d\n", data->pwm[attr->index]);
> +}
> +
> +static ssize_t set_pwm(struct device *dev, struct device_attribute *devattr,
> +			const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct adt7470_data *data = i2c_get_clientdata(client);
> +	int temp = simple_strtol(buf, NULL, 10);
> +
> +	mutex_lock(&data->lock);
> +	data->pwm[attr->index] = temp;
> +	i2c_smbus_write_byte_data(client, ADT7470_REG_PWM(attr->index), temp);
> +	mutex_unlock(&data->lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_pwm_max(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct adt7470_data *data = adt7470_update_device(dev);
> +	return sprintf(buf, "%d\n", data->pwm_max[attr->index]);
> +}
> +
> +static ssize_t set_pwm_max(struct device *dev, struct device_attribute *devattr,
> +			const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct adt7470_data *data = i2c_get_clientdata(client);
> +	int temp = simple_strtol(buf, NULL, 10);
> +
> +	mutex_lock(&data->lock);
> +	data->pwm_max[attr->index] = temp;
> +	i2c_smbus_write_byte_data(client, ADT7470_REG_PWM_MAX(attr->index), temp);
> +	mutex_unlock(&data->lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_pwm_min(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct adt7470_data *data = adt7470_update_device(dev);
> +	return sprintf(buf, "%d\n", data->pwm_min[attr->index]);
> +}
> +
> +static ssize_t set_pwm_min(struct device *dev, struct device_attribute *devattr,
> +			const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct adt7470_data *data = i2c_get_clientdata(client);
> +	int temp = simple_strtol(buf, NULL, 10);
> +
> +	mutex_lock(&data->lock);
> +	data->pwm_min[attr->index] = temp;
> +	i2c_smbus_write_byte_data(client, ADT7470_REG_PWM_MIN(attr->index), temp);
> +	mutex_unlock(&data->lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_pwm_tmax(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct adt7470_data *data = adt7470_update_device(dev);
> +	return sprintf(buf, "%d\n", 1000 * (20 + data->pwm_tmin[attr->index]));
> +}
> +
> +static ssize_t show_pwm_tmin(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct adt7470_data *data = adt7470_update_device(dev);
> +	return sprintf(buf, "%d\n", 1000 * data->pwm_tmin[attr->index]);
> +}
> +
> +static ssize_t set_pwm_tmin(struct device *dev, struct device_attribute *devattr,
> +			const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct adt7470_data *data = i2c_get_clientdata(client);
> +	int temp = simple_strtol(buf, NULL, 10) / 1000;
> +
> +	mutex_lock(&data->lock);
> +	data->pwm_tmin[attr->index] = temp;
> +	i2c_smbus_write_byte_data(client, ADT7470_REG_PWM_TMIN(attr->index), temp);
> +	mutex_unlock(&data->lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_pwm_auto(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct adt7470_data *data = adt7470_update_device(dev);
> +	return sprintf(buf, "%d\n", 1 + data->pwm_automatic[attr->index]);
> +}
> +
> +static ssize_t set_pwm_auto(struct device *dev, struct device_attribute *devattr,
> +			const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct adt7470_data *data = i2c_get_clientdata(client);
> +	int temp = simple_strtol(buf, NULL, 10);
> +	int pwm_auto_reg = ADT7470_REG_PWM_CFG_BASE + (attr->index / 2);

please create and use a ADT7470_REG_PWM_CFG() macro for this. Again consistency 
is key here, to make it easier for others to read your code.

> +	int pwm_auto_reg_mask = ADT7470_PWM1_AUTO_MASK;
> +	u8 reg;
> +
> +	if (!(attr->index % 2))
> +		pwm_auto_reg_mask <<= 1;

It took me a while to understand what you're doing here, because its a bit 
convoluted, and its wrong.

pwm1 has attr->index = 0, so the if is true, thus the mask gets shifted making 
it ADT7470_PWM2_AUTO_MASK, but for pwm1 we want ADT7470_PWM1_AUTO_MASK 
ofcourse. So the test should be inverted / the '!' should be removed.

I believe this error is caused by the current code being hard to read, I 
believe that something like this is better:
int pwm_auto_reg_mask;
u8 reg;

if (attr->index & 0x01)
   pwm_auto_reg_mask = ADT7470_PWM2_AUTO_MASK;
else
   pwm_auto_reg_mask = ADT7470_PWM1_AUTO_MASK;

> +
> +	if (temp != 2 && temp != 1)
> +		return -EINVAL;
> +	temp--;
> +
> +	mutex_lock(&data->lock);
> +	data->pwm_automatic[attr->index] = temp;
> +	reg = i2c_smbus_read_byte_data(client, pwm_auto_reg);

> +	reg &= ~pwm_auto_reg_mask;
> +	reg |= (temp ? pwm_auto_reg_mask : 0);
Same as with the FSPD mask.

> +	i2c_smbus_write_byte_data(client, pwm_auto_reg, reg);
> +	mutex_unlock(&data->lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_pwm_auto_temp(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct adt7470_data *data = adt7470_update_device(dev);
> +	u8 ctrl = data->pwm_auto_temp[attr->index];
> +
> +	if (ctrl)
> +		return sprintf(buf, "%d\n", 1 << (ctrl - 1));
> +	else
> +		return sprintf(buf, "%d\n", ADT7470_PWM_ALL_TEMPS);
> +}
> +
> +static int cvt_auto_temp(int input)
> +{
> +	if (input = ADT7470_PWM_ALL_TEMPS)
> +		return 0;
> +	if (input < 1 || !power_of_2(input))
> +		return -EINVAL;
> +	return ilog2(input) + 1;
> +}
> +
> +static ssize_t set_pwm_auto_temp(struct device *dev, struct device_attribute *devattr,
> +			const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct adt7470_data *data = i2c_get_clientdata(client);
> +	int temp = cvt_auto_temp(simple_strtol(buf, NULL, 10));
> +	int pwm_auto_reg = ADT7470_REG_PWM_AUTO_TEMP_BASE + (attr->index / 2);
Again, for consistency use a ADT7470_REG_PWM_AUTO_TEMP() macro please

> +	u8 reg;
> +
> +	if (temp < 0)
> +		return temp;
> +
> +	mutex_lock(&data->lock);
> +	data->pwm_automatic[attr->index] = temp;
> +	reg = i2c_smbus_read_byte_data(client, pwm_auto_reg);
> +
> +	if (!(attr->index % 2)) {
> +		reg &= 0xF;
> +		reg |= (temp << 4) & 0xF0;
> +	} else {
> +		reg &= 0xF0;
> +		reg |= temp & 0xF;
> +	}
> +
> +	i2c_smbus_write_byte_data(client, pwm_auto_reg, reg);
> +	mutex_unlock(&data->lock);
> +
> +	return count;
> +}
> +
> +static SENSOR_DEVICE_ATTR(alarms, S_IRUGO, show_alarms, NULL, 0);
> +static SENSOR_DEVICE_ATTR(alarm_mask, S_IRUGO, show_alarms, NULL, 1);
> +
> +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 0);
> +static SENSOR_DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 1);
> +static SENSOR_DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 2);
> +static SENSOR_DEVICE_ATTR(temp4_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 3);
> +static SENSOR_DEVICE_ATTR(temp5_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 4);
> +static SENSOR_DEVICE_ATTR(temp6_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 5);
> +static SENSOR_DEVICE_ATTR(temp7_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 6);
> +static SENSOR_DEVICE_ATTR(temp8_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 7);
> +static SENSOR_DEVICE_ATTR(temp9_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 8);
> +static SENSOR_DEVICE_ATTR(temp10_max, S_IWUSR | S_IRUGO, show_temp_max, set_temp_max, 9);
> +
> +static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 0);
> +static SENSOR_DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 1);
> +static SENSOR_DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 2);
> +static SENSOR_DEVICE_ATTR(temp4_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 3);
> +static SENSOR_DEVICE_ATTR(temp5_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 4);
> +static SENSOR_DEVICE_ATTR(temp6_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 5);
> +static SENSOR_DEVICE_ATTR(temp7_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 6);
> +static SENSOR_DEVICE_ATTR(temp8_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 7);
> +static SENSOR_DEVICE_ATTR(temp9_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 8);
> +static SENSOR_DEVICE_ATTR(temp10_min, S_IWUSR | S_IRUGO, show_temp_min, set_temp_min, 9);
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp, NULL, 1);
> +static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_temp, NULL, 2);
> +static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, show_temp, NULL, 3);
> +static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, show_temp, NULL, 4);
> +static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, show_temp, NULL, 5);
> +static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, show_temp, NULL, 6);
> +static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, show_temp, NULL, 7);
> +static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, show_temp, NULL, 8);
> +static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, show_temp, NULL, 9);
> +
> +static SENSOR_DEVICE_ATTR(fan1_max, S_IRUGO, show_fan_max, NULL, 0);
> +static SENSOR_DEVICE_ATTR(fan2_max, S_IRUGO, show_fan_max, NULL, 1);
> +static SENSOR_DEVICE_ATTR(fan3_max, S_IRUGO, show_fan_max, NULL, 2);
> +static SENSOR_DEVICE_ATTR(fan4_max, S_IRUGO, show_fan_max, NULL, 3);
> +
> +static SENSOR_DEVICE_ATTR(fan1_min, S_IRUGO, show_fan_min, NULL, 0);
> +static SENSOR_DEVICE_ATTR(fan2_min, S_IRUGO, show_fan_min, NULL, 1);
> +static SENSOR_DEVICE_ATTR(fan3_min, S_IRUGO, show_fan_min, NULL, 2);
> +static SENSOR_DEVICE_ATTR(fan4_min, S_IRUGO, show_fan_min, NULL, 3);
> +
> +static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, show_fan, NULL, 0);
> +static SENSOR_DEVICE_ATTR(fan2_input, S_IRUGO, show_fan, NULL, 1);
> +static SENSOR_DEVICE_ATTR(fan3_input, S_IRUGO, show_fan, NULL, 2);
> +static SENSOR_DEVICE_ATTR(fan4_input, S_IRUGO, show_fan, NULL, 3);
> +
> +static SENSOR_DEVICE_ATTR(raw_input, S_IRUGO, show_raw, NULL, 0);
> +static SENSOR_DEVICE_ATTR(force_pwm_max, S_IWUSR | S_IRUGO, show_force_pwm_max, set_force_pwm_max, 0);
> +
> +static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm, set_pwm, 0);
> +static SENSOR_DEVICE_ATTR(pwm2, S_IWUSR | S_IRUGO, show_pwm, set_pwm, 1);
> +static SENSOR_DEVICE_ATTR(pwm3, S_IWUSR | S_IRUGO, show_pwm, set_pwm, 2);
> +static SENSOR_DEVICE_ATTR(pwm4, S_IWUSR | S_IRUGO, show_pwm, set_pwm, 3);
> +
> +static SENSOR_DEVICE_ATTR(pwm1_auto_point1_pwm, S_IWUSR | S_IRUGO, show_pwm_min, set_pwm_min, 0);
> +static SENSOR_DEVICE_ATTR(pwm2_auto_point1_pwm, S_IWUSR | S_IRUGO, show_pwm_min, set_pwm_min, 1);
> +static SENSOR_DEVICE_ATTR(pwm3_auto_point1_pwm, S_IWUSR | S_IRUGO, show_pwm_min, set_pwm_min, 2);
> +static SENSOR_DEVICE_ATTR(pwm4_auto_point1_pwm, S_IWUSR | S_IRUGO, show_pwm_min, set_pwm_min, 3);
> +
> +static SENSOR_DEVICE_ATTR(pwm1_auto_point2_pwm, S_IWUSR | S_IRUGO, show_pwm_max, set_pwm_max, 0);
> +static SENSOR_DEVICE_ATTR(pwm2_auto_point2_pwm, S_IWUSR | S_IRUGO, show_pwm_max, set_pwm_max, 1);
> +static SENSOR_DEVICE_ATTR(pwm3_auto_point2_pwm, S_IWUSR | S_IRUGO, show_pwm_max, set_pwm_max, 2);
> +static SENSOR_DEVICE_ATTR(pwm4_auto_point2_pwm, S_IWUSR | S_IRUGO, show_pwm_max, set_pwm_max, 3);
> +
> +static SENSOR_DEVICE_ATTR(pwm1_auto_point1_temp, S_IWUSR | S_IRUGO, show_pwm_tmin, set_pwm_tmin, 0);
> +static SENSOR_DEVICE_ATTR(pwm2_auto_point1_temp, S_IWUSR | S_IRUGO, show_pwm_tmin, set_pwm_tmin, 1);
> +static SENSOR_DEVICE_ATTR(pwm3_auto_point1_temp, S_IWUSR | S_IRUGO, show_pwm_tmin, set_pwm_tmin, 2);
> +static SENSOR_DEVICE_ATTR(pwm4_auto_point1_temp, S_IWUSR | S_IRUGO, show_pwm_tmin, set_pwm_tmin, 3);
> +
> +static SENSOR_DEVICE_ATTR(pwm1_auto_point2_temp, S_IRUGO, show_pwm_tmax, NULL, 0);
> +static SENSOR_DEVICE_ATTR(pwm2_auto_point2_temp, S_IRUGO, show_pwm_tmax, NULL, 1);
> +static SENSOR_DEVICE_ATTR(pwm3_auto_point2_temp, S_IRUGO, show_pwm_tmax, NULL, 2);
> +static SENSOR_DEVICE_ATTR(pwm4_auto_point2_temp, S_IRUGO, show_pwm_tmax, NULL, 3);
> +
> +static SENSOR_DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO, show_pwm_auto, set_pwm_auto, 0);
> +static SENSOR_DEVICE_ATTR(pwm2_enable, S_IWUSR | S_IRUGO, show_pwm_auto, set_pwm_auto, 1);
> +static SENSOR_DEVICE_ATTR(pwm3_enable, S_IWUSR | S_IRUGO, show_pwm_auto, set_pwm_auto, 2);
> +static SENSOR_DEVICE_ATTR(pwm4_enable, S_IWUSR | S_IRUGO, show_pwm_auto, set_pwm_auto, 3);
> +
> +static SENSOR_DEVICE_ATTR(pwm1_auto_channels_temp, S_IWUSR | S_IRUGO, show_pwm_auto_temp, set_pwm_auto_temp, 0);
> +static SENSOR_DEVICE_ATTR(pwm2_auto_channels_temp, S_IWUSR | S_IRUGO, show_pwm_auto_temp, set_pwm_auto_temp, 1);
> +static SENSOR_DEVICE_ATTR(pwm3_auto_channels_temp, S_IWUSR | S_IRUGO, show_pwm_auto_temp, set_pwm_auto_temp, 2);
> +static SENSOR_DEVICE_ATTR(pwm4_auto_channels_temp, S_IWUSR | S_IRUGO, show_pwm_auto_temp, set_pwm_auto_temp, 3);
> +
> +static int adt7470_attach_adapter(struct i2c_adapter *adapter)
> +{
> +	if (!(adapter->class & I2C_CLASS_HWMON))
> +		return 0;
> +	return i2c_probe(adapter, &addr_data, adt7470_detect);
> +}
> +
> +static struct attribute *adt7470_attributes[] = {
> +	&sensor_dev_attr_force_pwm_max.dev_attr.attr,
> +	&sensor_dev_attr_alarms.dev_attr.attr,
> +	&sensor_dev_attr_alarm_mask.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max.dev_attr.attr,
> +	&sensor_dev_attr_temp2_max.dev_attr.attr,
> +	&sensor_dev_attr_temp3_max.dev_attr.attr,
> +	&sensor_dev_attr_temp4_max.dev_attr.attr,
> +	&sensor_dev_attr_temp5_max.dev_attr.attr,
> +	&sensor_dev_attr_temp6_max.dev_attr.attr,
> +	&sensor_dev_attr_temp7_max.dev_attr.attr,
> +	&sensor_dev_attr_temp8_max.dev_attr.attr,
> +	&sensor_dev_attr_temp9_max.dev_attr.attr,
> +	&sensor_dev_attr_temp10_max.dev_attr.attr,
> +	&sensor_dev_attr_temp1_min.dev_attr.attr,
> +	&sensor_dev_attr_temp2_min.dev_attr.attr,
> +	&sensor_dev_attr_temp3_min.dev_attr.attr,
> +	&sensor_dev_attr_temp4_min.dev_attr.attr,
> +	&sensor_dev_attr_temp5_min.dev_attr.attr,
> +	&sensor_dev_attr_temp6_min.dev_attr.attr,
> +	&sensor_dev_attr_temp7_min.dev_attr.attr,
> +	&sensor_dev_attr_temp8_min.dev_attr.attr,
> +	&sensor_dev_attr_temp9_min.dev_attr.attr,
> +	&sensor_dev_attr_temp10_min.dev_attr.attr,
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_temp2_input.dev_attr.attr,
> +	&sensor_dev_attr_temp3_input.dev_attr.attr,
> +	&sensor_dev_attr_temp4_input.dev_attr.attr,
> +	&sensor_dev_attr_temp5_input.dev_attr.attr,
> +	&sensor_dev_attr_temp6_input.dev_attr.attr,
> +	&sensor_dev_attr_temp7_input.dev_attr.attr,
> +	&sensor_dev_attr_temp8_input.dev_attr.attr,
> +	&sensor_dev_attr_temp9_input.dev_attr.attr,
> +	&sensor_dev_attr_temp10_input.dev_attr.attr,
> +	&sensor_dev_attr_fan1_max.dev_attr.attr,
> +	&sensor_dev_attr_fan2_max.dev_attr.attr,
> +	&sensor_dev_attr_fan3_max.dev_attr.attr,
> +	&sensor_dev_attr_fan4_max.dev_attr.attr,
> +	&sensor_dev_attr_fan1_min.dev_attr.attr,
> +	&sensor_dev_attr_fan2_min.dev_attr.attr,
> +	&sensor_dev_attr_fan3_min.dev_attr.attr,
> +	&sensor_dev_attr_fan4_min.dev_attr.attr,
> +	&sensor_dev_attr_fan1_input.dev_attr.attr,
> +	&sensor_dev_attr_fan2_input.dev_attr.attr,
> +	&sensor_dev_attr_fan3_input.dev_attr.attr,
> +	&sensor_dev_attr_fan4_input.dev_attr.attr,
> +	&sensor_dev_attr_raw_input.dev_attr.attr,
> +	&sensor_dev_attr_pwm1.dev_attr.attr,
> +	&sensor_dev_attr_pwm2.dev_attr.attr,
> +	&sensor_dev_attr_pwm3.dev_attr.attr,
> +	&sensor_dev_attr_pwm4.dev_attr.attr,
> +	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
> +	&sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr,
> +	&sensor_dev_attr_pwm3_auto_point1_pwm.dev_attr.attr,
> +	&sensor_dev_attr_pwm4_auto_point1_pwm.dev_attr.attr,
> +	&sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr,
> +	&sensor_dev_attr_pwm2_auto_point2_pwm.dev_attr.attr,
> +	&sensor_dev_attr_pwm3_auto_point2_pwm.dev_attr.attr,
> +	&sensor_dev_attr_pwm4_auto_point2_pwm.dev_attr.attr,
> +	&sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr,
> +	&sensor_dev_attr_pwm2_auto_point1_temp.dev_attr.attr,
> +	&sensor_dev_attr_pwm3_auto_point1_temp.dev_attr.attr,
> +	&sensor_dev_attr_pwm4_auto_point1_temp.dev_attr.attr,
> +	&sensor_dev_attr_pwm1_auto_point2_temp.dev_attr.attr,
> +	&sensor_dev_attr_pwm2_auto_point2_temp.dev_attr.attr,
> +	&sensor_dev_attr_pwm3_auto_point2_temp.dev_attr.attr,
> +	&sensor_dev_attr_pwm4_auto_point2_temp.dev_attr.attr,
> +	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
> +	&sensor_dev_attr_pwm2_enable.dev_attr.attr,
> +	&sensor_dev_attr_pwm3_enable.dev_attr.attr,
> +	&sensor_dev_attr_pwm4_enable.dev_attr.attr,
> +	&sensor_dev_attr_pwm1_auto_channels_temp.dev_attr.attr,
> +	&sensor_dev_attr_pwm2_auto_channels_temp.dev_attr.attr,
> +	&sensor_dev_attr_pwm3_auto_channels_temp.dev_attr.attr,
> +	&sensor_dev_attr_pwm4_auto_channels_temp.dev_attr.attr,
> +	NULL
> +};
> +

Maybe just but all the sensor_attr directly into an array and use a for loop on 
that array to register / unregister instead of sysfs attribute groups?

That would save you the above array.


> +static int adt7470_detect(struct i2c_adapter *adapter, int address, int kind)
> +{
> +	struct i2c_client *client;
> +	struct adt7470_data *data;
> +	int err = 0;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_WORD_DATA))
> +		goto exit;
> +
> +	if (!(data = kzalloc(sizeof(struct adt7470_data), GFP_KERNEL))) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	client = &data->client;
> +	client->addr = address;
> +	client->adapter = adapter;
> +	client->driver = &adt7470_driver;
> +
> +	i2c_set_clientdata(client, data);
> +
> +	mutex_init(&data->lock);
> +
> +	if (kind <= 0) {
> +		int vendor, device, revision;
> +
> +		vendor = i2c_smbus_read_byte_data(client, ADT7470_REG_VENDOR);
> +		if (vendor != ADT7470_VENDOR) {
> +			printk(KERN_ERR "%s: vendor=%x\n", __FUNCTION__, vendor);
> +			err = -ENODEV;
> +			goto exit_free;
> +		}
> +
> +		device = i2c_smbus_read_byte_data(client, ADT7470_REG_DEVICE);
> +		if (device != ADT7470_DEVICE) {
> +			printk(KERN_ERR "%s: device=%x\n", __FUNCTION__, device);
> +			err = -ENODEV;
> +			goto exit_free;
> +		}
> +
> +		revision = i2c_smbus_read_byte_data(client, ADT7470_REG_REVISION);
> +		if (revision != ADT7470_REVISION) {
> +			printk(KERN_ERR "%s: revision=%x\n", __FUNCTION__, revision);
> +			err = -ENODEV;
> +			goto exit_free;
> +		}
> +
> +		data->type = adt7470;
> +	} else {
> +		dev_dbg(&adapter->dev, "detection forced\n");
> +	}
> +
> +	if (kind > 0)
> +		data->type = kind;
> +
> +	data->attrs.attrs = adt7470_attributes;
> +	strlcpy(client->name, "adt7470", I2C_NAME_SIZE);
> +
> +	if ((err = i2c_attach_client(client)))
> +		goto exit_free;
> +
> +	dev_info(&client->dev, "%s chip found\n", client->name);
> +
> +	/* Initialize the ADT7470 chip */
> +	adt7470_init_client(client);
> +
> +	/* Register sysfs hooks */
> +	if ((err = sysfs_create_group(&client->dev.kobj, &data->attrs)))
> +		goto exit_detach;
> +
> +	data->class_dev = hwmon_device_register(&client->dev);
> +	if (IS_ERR(data->class_dev)) {
> +		err = PTR_ERR(data->class_dev);
> +		goto exit_remove;
> +	}
> +
> +	return 0;
> +
> +exit_remove:
> +	sysfs_remove_group(&client->dev.kobj, &data->attrs);
> +exit_detach:
> +	i2c_detach_client(client);
> +exit_free:
> +	kfree(data);
> +exit:
> +	return err;
> +}
> +
> +static int adt7470_detach_client(struct i2c_client *client)
> +{
> +	struct adt7470_data *data = i2c_get_clientdata(client);
> +	hwmon_device_unregister(data->class_dev);
> +	sysfs_remove_group(&client->dev.kobj, &data->attrs);
> +	i2c_detach_client(client);
> +	kfree(data);
> +	return 0;
> +}
> +
> +static int __init adt7470_init(void)
> +{
> +	return i2c_add_driver(&adt7470_driver);
> +}
> +
> +static void __exit adt7470_exit(void)
> +{
> +	i2c_del_driver(&adt7470_driver);
> +}
> +
> +MODULE_AUTHOR("Darrick J. Wong <djwong@us.ibm.com>");
> +MODULE_DESCRIPTION("ADT7470 driver");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(DRV_VERSION);
> +
> +module_init(adt7470_init);
> +module_exit(adt7470_exit);
> 
> 

Regards,

Hans

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

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Hans de Goede]

p.s.

I would be much obliged if in return you could review my latest driver:
http://lists.lm-sensors.org/pipermail/lm-sensors/2007-July/020297.html

Thanks,

Hans


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

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Jean Delvare]

Hi Darrick,

On Fri, 6 Jul 2007 14:11:45 -0700, Darrick J. Wong wrote:
> Below is a first draft of a driver for the ADT7470 chip.  I've tested
> the driver against the adt7470 in the IBM IntelliStation Z30 and as far
> as I can tell it's good for controlling the speed of the CPU heatsink
> fans and monitor CPU temperature.  Please let me know what you think of
> the driver.  The patch should apply cleanly against 2.6.22-rc7.

I've added an entry in our wiki Devices page to track this one.

Note that back in January this year, someone (Vadim Zeitlin, Cc'd)
asked for support for this chip. So maybe Vadim would be interested in
testing your driver.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Darrick J. Wong]

[-- Attachment #1.1: Type: text/plain, Size: 2402 bytes --]

On Sat, Jul 07, 2007 at 10:21:57AM +0200, Hans de Goede wrote:

> Hi,
> 
> Below is a review of your driver.

Thanks for taking the time to review it.  In general I've found the
points you make are pretty good and have implemented them.  That said,
there were a few that I have some questions about, so I'll skip down to
those.  I intend to post a revised patch shortly, and review your driver
after that.

> Why all the base defines? Why not put the base address directly in the () 
> macros? You do not seem to use them anywhere else.

I prefer to put all the register and bit-field definitions in one place
so that later I can construct a map of all known registers after I've
forgotten which ones the driver talks to and which ones it doesn't.  The
accessor macros that come after that are merely sugar, which is why I
don't like encoding the base register address directly into the macro.

> Are you sure you only want to support revision 2? What are the differences? 
> Shouldn't the driver be able to support other revisions too? Maybe only 
> print a warning when its used with an untested revision?

I don't have a data sheet for other revisions, so the safest thing to do is
to reject that which we don't know about; an interested user can always
bypass those safety checks with force_adt7470= at a later point in time
and post a patch if the driver still works ok.

> Why use read_byte twice in read16, but write_word in write16?

A "16 bit" register is really two 8 bit register reads/writes, and the
register with the lower address has to be read/written first.  Hence the
weird helper functions.

> >+		/* delay is 200ms * number of tmp05 sensors */
> >+		udelay(TEMP_COLLECTION_TIME);
> 
> ugh, sleep here please.

Would msleep_interruptible() work here?  Some informal testing shows
that it seems to work ok, though I've not subjected it to a rigorous ^C
test to see if I can hit weird crash conditions.

> This may be a stupid question (I haven't fully read the datasheet) but why 
> no store functions for fan_min and fan_max?

Silly thinko on my part; the v2 patch will have those store functions.

> Maybe just but all the sensor_attr directly into an array and use a for 
> loop on that array to register / unregister instead of sysfs attribute 
> groups?

I implemented that, using your f71882fg driver as an example.

--D

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Darrick J. Wong]

[-- Attachment #1.1: Type: text/plain, Size: 35886 bytes --]

On Sat, Jul 07, 2007 at 10:23:45AM +0200, Hans de Goede wrote:
> p.s.
> 
> I would be much obliged if in return you could review my latest driver:
> http://lists.lm-sensors.org/pipermail/lm-sensors/2007-July/020297.html

Review follows below.

> This is a new driver for the hardware monitoring features of the Fintek
> F71882FG and F71883FG Super-I/O chips.
> 
> This version of the driver does not support the pwm part of these chips (yet).
> I'll first design a sysfs api for this and post that for discussion, and then
> implement pwm support as an incremental patch over this one.
> 
> This driver supports all sensors of this chip, except for the vid inputs. The
> vid inputs are somewhat documented in the datasheet, but I know nothing about
> vid/vrm stuff. Help with this would be much appreciated.
> 
> Signed-off-by: Hans de Goede <j.w.r.degoede@hhs.nl>

Hmm... no Signed-off-by from Mr. Edgington?

> diff -up linux-2.6.22-rc4/drivers/hwmon/f71882fg.c.newdriver linux-2.6.22-rc4/drivers/hwmon/f71882fg.c
> --- linux-2.6.22-rc4/drivers/hwmon/f71882fg.c.newdriver	2007-07-06 15:38:45.000000000 +0200
> +++ linux-2.6.22-rc4/drivers/hwmon/f71882fg.c	2007-07-06 15:38:34.000000000 +0200
> @@ -0,0 +1,949 @@
> +/***************************************************************************
> + *   Copyright (C) 2006 by Hans Edgington <hans@edgington.nl>              *
> + *   Copyright (C) 2007 by Hans de Goede  <j.w.r.degoede@hhs.nl>           *
> + *                                                                         *
> + *   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.,                                       *
> + *   59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.             *
> + ***************************************************************************/
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/platform_device.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <asm/io.h>
> +
> +#define DRVNAME "f71882fg"
> +
> +#define SIO_F71882FG_LD_HWM	0x04	/* Hardware monitor logical device*/
> +#define SIO_UNLOCK_KEY		0x87	/* Key to enable Super-I/O */
> +#define SIO_LOCK_KEY		0xAA	/* Key to diasble Super-I/O */
> +
> +#define SIO_REG_LDSEL		0x07	/* Logical device select */
> +#define SIO_REG_DEVID		0x20	/* Device ID (2 bytes) */
> +#define SIO_REG_DEVREV		0x22	/* Device revision */
> +#define SIO_REG_MANID		0x23	/* Fintek ID (2 bytes) */
> +#define SIO_REG_ENABLE		0x30	/* Logical device enable */
> +#define SIO_REG_ADDR		0x60	/* Logical device address (2 bytes) */
> +
> +#define SIO_FINTEK_ID		0x1934	/* Manufacturers ID */
> +#define SIO_F71882_ID		0x0541	/* Chipset ID */
> +
> +#define REGION_LENGTH		8
> +#define ADDR_REG_OFFSET		5
> +#define DATA_REG_OFFSET		6
> +
> +#define F71882FG_REG_PECI		0x0A
> +
> +#define F71882FG_REG_IN_STATUS		0x12
> +#define F71882FG_REG_IN_BEEP		0x13
> +#define F71882FG_REG_IN(nr)		(0x20  + (nr))
> +#define F71882FG_REG_IN1_HIGH		0x32
> +
> +#define F71882FG_REG_FAN(nr)		(0xA0 + (16 * (nr)))
> +#define F71882FG_REG_FAN_STATUS		0x92
> +#define F71882FG_REG_FAN_BEEP		0x93
> +
> +#define F71882FG_REG_TEMP(nr)		(0x72 + 2 * (nr))
> +#define F71882FG_REG_TEMP_OVT(nr)	(0x82 + 2 * (nr))
> +#define F71882FG_REG_TEMP_HIGH(nr)	(0x83 + 2 * (nr))
> +#define F71882FG_REG_TEMP_STATUS	0x62
> +#define F71882FG_REG_TEMP_BEEP		0x63
> +#define F71882FG_REG_TEMP_HYST1		0x6C
> +#define F71882FG_REG_TEMP_HYST23	0x6D
> +#define F71882FG_REG_TEMP_TYPE		0x6B
> +#define F71882FG_REG_TEMP_DIODE_OPEN	0x6F
> +
> +#define	F71882FG_REG_START		0x01
> +
> +#define FAN_MIN_DETECT			366 /* Lowest detectable fanspeed */
> +
> +static struct platform_device *f71882fg_pdev = NULL;
> +
> +/* Super-I/O Function prototypes */
> +static inline int superio_inb(int base, int reg);
> +static inline int superio_inw(int base, int reg);
> +static inline void superio_enter(int base);
> +static inline void superio_select(int base, int ld);
> +static inline void superio_exit(int base);
> +
> +static inline u16 fan_from_reg ( u16 reg );
> +
> +struct f71882fg_data {
> +	unsigned short addr;
> +	struct class_device *class_dev;
> +
> +	struct mutex update_lock;
> +	char valid;			/* !=0 if following fields are valid */
> +	unsigned long last_updated;	/* In jiffies */
> +	unsigned long last_limits;	/* In jiffies */
> +
> +	/* Register Values */
> +	u8	in[9];
> +	u8	in1_max;
> +	u8	in_status;
> +	u8	in_beep;
> +	u16	fan[4];
> +	u8	fan_status;
> +	u8	fan_beep;
> +	u8	temp[3];
> +	u8	temp_ovt[3];
> +	u8	temp_high[3];
> +	u8	temp_hyst[3];
> +	u8	temp_type[3];
> +	u8	temp_status;
> +	u8	temp_beep;
> +	u8	temp_diode_open;
> +};
> +
> +static u8 f71882fg_read8(struct f71882fg_data *data, u8 reg);
> +static u16 f71882fg_read16(struct f71882fg_data *data, u8 reg);
> +static void f71882fg_write8(struct f71882fg_data *data, u8 reg, u8 val);
> +
> +/* Sysfs in*/
> +static ssize_t show_in(struct device *dev, struct device_attribute *devattr,
> +	char *buf);
> +static ssize_t show_in_max(struct device *dev, struct device_attribute
> +	*devattr, char *buf);
> +static ssize_t store_in_max(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count);
> +static ssize_t show_in_beep(struct device *dev, struct device_attribute
> +	*devattr, char *buf);
> +static ssize_t store_in_beep(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count);
> +static ssize_t show_in_alarm(struct device *dev, struct device_attribute
> +	*devattr, char *buf);
> +/* Sysfs Fan */
> +static ssize_t show_fan(struct device *dev, struct device_attribute *devattr,
> +	char *buf);
> +static ssize_t show_fan_beep(struct device *dev, struct device_attribute
> +	*devattr, char *buf);
> +static ssize_t store_fan_beep(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count);
> +static ssize_t show_fan_alarm(struct device *dev, struct device_attribute
> +	*devattr, char *buf);
> +/* Sysfs Temp */
> +static ssize_t show_temp(struct device *dev, struct device_attribute
> +	*devattr, char *buf);
> +static ssize_t show_temp_max(struct device *dev, struct device_attribute
> +	*devattr, char *buf);
> +static ssize_t store_temp_max(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count);
> +static ssize_t show_temp_max_hyst(struct device *dev, struct device_attribute
> +	*devattr, char *buf);
> +static ssize_t store_temp_max_hyst(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count);
> +static ssize_t show_temp_crit(struct device *dev, struct device_attribute
> +	*devattr, char *buf);
> +static ssize_t store_temp_crit(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count);
> +static ssize_t show_temp_crit_hyst(struct device *dev, struct device_attribute
> +	*devattr, char *buf);
> +static ssize_t show_temp_type(struct device *dev, struct device_attribute
> +	*devattr, char *buf);
> +static ssize_t show_temp_beep(struct device *dev, struct device_attribute
> +	*devattr, char *buf);
> +static ssize_t store_temp_beep(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count);
> +static ssize_t show_temp_alarm(struct device *dev, struct device_attribute
> +	*devattr, char *buf);
> +static ssize_t show_temp_fault(struct device *dev, struct device_attribute
> +	*devattr, char *buf);
> +/* Sysfs misc */
> +static ssize_t show_name(struct device *dev, struct device_attribute *devattr,
> +	char *buf);
> +
> +static int __devinit f71882fg_probe(struct platform_device * pdev);
> +static int __devexit f71882fg_remove(struct platform_device *pdev);
> +static int __init f71882fg_init(void);
> +static int __init f71882fg_find(int sioaddr, unsigned short *address);
> +static int __init f71882fg_device_add(unsigned short address);
> +static void __exit f71882fg_exit(void);
> +
> +static struct platform_driver f71882fg_driver = {
> +	.driver = {
> +		.owner	= THIS_MODULE,
> +		.name	= DRVNAME,
> +	},
> +	.probe		= f71882fg_probe,
> +	.remove		= __devexit_p(f71882fg_remove),
> +};
> +
> +static struct device_attribute f71882fg_dev_attr[] =
> +{
> +	__ATTR( name, S_IRUGO, show_name, NULL ),
> +};
> +
> +static struct sensor_device_attribute f71882fg_in_temp_attr[] =
> +{
> +	SENSOR_ATTR(in0_input, S_IRUGO, show_in, NULL, 0),
> +	SENSOR_ATTR(in1_input, S_IRUGO, show_in, NULL, 1),
> +	SENSOR_ATTR(in1_max, S_IRUGO|S_IWUSR, show_in_max, store_in_max, 1),
> +	SENSOR_ATTR(in1_beep, S_IRUGO|S_IWUSR, show_in_beep, store_in_beep, 1),
> +	SENSOR_ATTR(in1_alarm, S_IRUGO, show_in_alarm, NULL, 1),
> +	SENSOR_ATTR(in2_input, S_IRUGO, show_in, NULL, 2),
> +	SENSOR_ATTR(in3_input, S_IRUGO, show_in, NULL, 3),
> +	SENSOR_ATTR(in4_input, S_IRUGO, show_in, NULL, 4),
> +	SENSOR_ATTR(in5_input, S_IRUGO, show_in, NULL, 5),
> +	SENSOR_ATTR(in6_input, S_IRUGO, show_in, NULL, 6),
> +	SENSOR_ATTR(in7_input, S_IRUGO, show_in, NULL, 7),
> +	SENSOR_ATTR(in8_input, S_IRUGO, show_in, NULL, 8),
> +	SENSOR_ATTR(temp1_input, S_IRUGO, show_temp, NULL, 0),
> +	SENSOR_ATTR(temp1_max, S_IRUGO|S_IWUSR, show_temp_max,
> +		store_temp_max, 0),
> +	SENSOR_ATTR(temp1_max_hyst, S_IRUGO|S_IWUSR, show_temp_max_hyst,
> +		store_temp_max_hyst, 0),
> +	SENSOR_ATTR(temp1_crit, S_IRUGO|S_IWUSR, show_temp_crit,
> +		store_temp_crit, 0),
> +	SENSOR_ATTR(temp1_crit_hyst, S_IRUGO, show_temp_crit_hyst, NULL, 0),
> +	SENSOR_ATTR(temp1_type, S_IRUGO, show_temp_type, NULL, 0),
> +	SENSOR_ATTR(temp1_beep, S_IRUGO|S_IWUSR, show_temp_beep,
> +		store_temp_beep, 0),
> +	SENSOR_ATTR(temp1_alarm, S_IRUGO, show_temp_alarm, NULL, 0),
> +	SENSOR_ATTR(temp1_fault, S_IRUGO, show_temp_fault, NULL, 0),
> +	SENSOR_ATTR(temp2_input, S_IRUGO, show_temp, NULL, 1),
> +	SENSOR_ATTR(temp2_max, S_IRUGO|S_IWUSR, show_temp_max,
> +		store_temp_max, 1),
> +	SENSOR_ATTR(temp2_max_hyst, S_IRUGO|S_IWUSR, show_temp_max_hyst,
> +		store_temp_max_hyst, 1),
> +	SENSOR_ATTR(temp2_crit, S_IRUGO|S_IWUSR, show_temp_crit,
> +		store_temp_crit, 1),
> +	SENSOR_ATTR(temp2_crit_hyst, S_IRUGO, show_temp_crit_hyst, NULL, 1),
> +	SENSOR_ATTR(temp2_type, S_IRUGO, show_temp_type, NULL, 1),
> +	SENSOR_ATTR(temp2_beep, S_IRUGO|S_IWUSR, show_temp_beep,
> +		store_temp_beep, 1),
> +	SENSOR_ATTR(temp2_alarm, S_IRUGO, show_temp_alarm, NULL, 1),
> +	SENSOR_ATTR(temp2_fault, S_IRUGO, show_temp_fault, NULL, 1),
> +	SENSOR_ATTR(temp3_input, S_IRUGO, show_temp, NULL, 2),
> +	SENSOR_ATTR(temp3_max, S_IRUGO|S_IWUSR, show_temp_max,
> +		store_temp_max, 2),
> +	SENSOR_ATTR(temp3_max_hyst, S_IRUGO|S_IWUSR, show_temp_max_hyst,
> +		store_temp_max_hyst, 2),
> +	SENSOR_ATTR(temp3_crit, S_IRUGO|S_IWUSR, show_temp_crit,
> +		store_temp_crit, 2),
> +	SENSOR_ATTR(temp3_crit_hyst, S_IRUGO, show_temp_crit_hyst, NULL, 2),
> +	SENSOR_ATTR(temp3_type, S_IRUGO, show_temp_type, NULL, 2),
> +	SENSOR_ATTR(temp3_beep, S_IRUGO|S_IWUSR, show_temp_beep,
> +		store_temp_beep, 2),
> +	SENSOR_ATTR(temp3_alarm, S_IRUGO, show_temp_alarm, NULL, 2),
> +	SENSOR_ATTR(temp3_fault, S_IRUGO, show_temp_fault, NULL, 2)
> +};
> +
> +static struct sensor_device_attribute f71882fg_fan_attr[] =
> +{
> +	SENSOR_ATTR(fan1_input, S_IRUGO, show_fan, NULL, 0),
> +	SENSOR_ATTR(fan1_beep, S_IRUGO|S_IWUSR, show_fan_beep,
> +		store_fan_beep, 0),
> +	SENSOR_ATTR(fan1_alarm, S_IRUGO, show_fan_alarm, NULL, 0),
> +	SENSOR_ATTR(fan2_input, S_IRUGO, show_fan, NULL, 1),
> +	SENSOR_ATTR(fan2_beep, S_IRUGO|S_IWUSR, show_fan_beep,
> +		store_fan_beep, 1),
> +	SENSOR_ATTR(fan2_alarm, S_IRUGO, show_fan_alarm, NULL, 1),
> +	SENSOR_ATTR(fan3_input, S_IRUGO, show_fan, NULL, 2),
> +	SENSOR_ATTR(fan3_beep, S_IRUGO|S_IWUSR, show_fan_beep,
> +		store_fan_beep, 2),
> +	SENSOR_ATTR(fan3_alarm, S_IRUGO, show_fan_alarm, NULL, 2),
> +	SENSOR_ATTR(fan4_input, S_IRUGO, show_fan, NULL, 3),
> +	SENSOR_ATTR(fan4_beep, S_IRUGO|S_IWUSR, show_fan_beep,
> +		store_fan_beep, 3),
> +	SENSOR_ATTR(fan4_alarm, S_IRUGO, show_fan_alarm, NULL, 3)
> +};

If you moved these structs to the bottom, you could avoid having to
declare the function prototypes for static functions, thus making
your driver shorter.  gcc will still catch error cases where the static
function's signature doesn't match whatever SENSOR_ATTR wants.

It doesn't really matter to me, but I'm curious to learn your
reasons for choosing this method.

> +/* Super I/O functions */
> +static inline int superio_inb(int base, int reg)
> +{
> +	outb(reg, base);
> +	return inb(base + 1);
> +}
> +
> +static int superio_inw(int base, int reg)
> +{
> +	int val;
> +	outb(reg++, base);
> +	val = inb(base + 1) << 8;
> +	outb(reg, base);
> +	val |= inb(base + 1);
> +	return val;
> +}
> +
> +static inline void superio_enter(int base)
> +{
> +	outb( SIO_UNLOCK_KEY, base);
> +	outb( SIO_UNLOCK_KEY, base);

Are you deliberately doing this twice?  If so, a comment to that
effect would be a good idea.

> +}
> +
> +static inline void superio_select( int base, int ld)
> +{
> +	outb(SIO_REG_LDSEL, base);
> +	outb(ld, base + 1);
> +}
> +
> +static inline void superio_exit(int base)
> +{
> +	outb(SIO_LOCK_KEY, base);
> +}
> +
> +static inline u16 fan_from_reg(u16 reg)
> +{
> +	return (1500000 / reg);
> +}

I've not read the spec sheet, but are you sure reg != 0?  I've observed
overheating conditions with the adt7470 where it starts returning bogus
values that the data sheets say it shouldn't ever return, and crashing
the kernel would not be a good way to react to that.

> +static u8 f71882fg_read8(struct f71882fg_data *data, u8 reg)
> +{
> +	u8 val;
> +
> +	outb(reg, data->addr + ADDR_REG_OFFSET);
> +	val = inb(data->addr + DATA_REG_OFFSET);
> +
> +	return val;
> +}
> +
> +static u16 f71882fg_read16(struct f71882fg_data *data, u8 reg)
> +{
> +	u16 val;
> +
> +	outb(reg, data->addr + ADDR_REG_OFFSET);
> +	val = inb(data->addr + DATA_REG_OFFSET) << 8;
> +	outb(++reg, data->addr + ADDR_REG_OFFSET);

superio_inw() used "reg++" and then "reg"; here you use "reg" then
"++reg".  Though the two are functionally equivalent here, I wonder
about the inconsistency.

> +	val |= inb(data->addr + DATA_REG_OFFSET);
> +
> +	return val;
> +}
> +
> +static void f71882fg_write8(struct f71882fg_data *data, u8 reg, u8 val)
> +{
> +	outb(reg, data->addr + ADDR_REG_OFFSET);
> +	outb(val, data->addr + DATA_REG_OFFSET);	
> +}
> +
> +static struct f71882fg_data *f71882fg_update_device(struct device * dev)
> +{
> +	struct f71882fg_data *data = dev_get_drvdata(dev);
> +	int nr, reg, reg2;
> +
> +	mutex_lock(&data->update_lock);
> +	
> +	/* Update once every 60 seconds */
> +	if ( time_after(jiffies, data->last_updated + 60 * HZ ) ||
> +			!data->valid) {

You test last_updated here but update last_limits below.  Is that
really what you intended?  last_limits isn't used anywhere in
the driver.

> +		data->in1_max = f71882fg_read8(data, F71882FG_REG_IN1_HIGH);
> +		data->in_beep = f71882fg_read8(data, F71882FG_REG_IN_BEEP);
> +			
> +		/* Get High & boundary temps*/
> +		for (nr = 0; nr < 3; nr++) {
> +			data->temp_ovt[nr] = f71882fg_read8(data,
> +						F71882FG_REG_TEMP_OVT(nr));
> +			data->temp_high[nr] = f71882fg_read8(data,
> +						F71882FG_REG_TEMP_HIGH(nr));
> +		}
> +
> +		/* Have to hardcode hyst*/
> +		data->temp_hyst[0] = f71882fg_read8(data,
> +						F71882FG_REG_TEMP_HYST1) >> 4;
> +		/* Hyst temps 2 & 3 stored in same register */
> +		reg = f71882fg_read8(data, F71882FG_REG_TEMP_HYST23);
> +		data->temp_hyst[1] = reg & 0x0F;
> +		data->temp_hyst[2] = reg >> 4;
> +
> +		/* Have to hardcode type, because temp1 is special */
> +		reg  = f71882fg_read8(data, F71882FG_REG_TEMP_TYPE);
> +		reg2 = f71882fg_read8(data, F71882FG_REG_PECI);
> +		if ((reg2 & 0x03) == 0x01) 
> +			data->temp_type[0] = 6 /* PECI */;
> +		else if ((reg2 & 0x03) == 0x02)
> +			data->temp_type[0] = 5 /* AMDSI */;
> +		else
> +			data->temp_type[0] = (reg & 0x02) ? 2 : 4;
> +		
> +		data->temp_type[1] = (reg & 0x04) ? 2 : 4;
> +		data->temp_type[2] = (reg & 0x08) ? 2 : 4;
> +		
> +		data->temp_beep = f71882fg_read8(data, F71882FG_REG_TEMP_BEEP);
> +		
> +		data->fan_beep = f71882fg_read8(data, F71882FG_REG_FAN_BEEP);
> +
> +		data->last_limits = jiffies;
> +	}
> +	
> +	/* Update every second */
> +	if (time_after(jiffies, data->last_updated + HZ) || !data->valid)  {
> +		data->temp_status = f71882fg_read8(data,
> +						F71882FG_REG_TEMP_STATUS);
> +		data->temp_diode_open = f71882fg_read8(data,
> +						F71882FG_REG_TEMP_DIODE_OPEN);
> +		for (nr = 0; nr < 3; nr++)
> +			data->temp[nr] = f71882fg_read8(data,
> +						F71882FG_REG_TEMP(nr));
> +
> +		data->fan_status = f71882fg_read8(data,
> +						F71882FG_REG_FAN_STATUS);
> +		for (nr = 0; nr < 4; nr++)
> +			data->fan[nr] = f71882fg_read16(data,
> +						F71882FG_REG_FAN(nr));
> +
> +		data->in_status = f71882fg_read8(data,
> +						F71882FG_REG_IN_STATUS);
> +		for (nr = 0; nr < 9; nr++)
> +			data->in[nr] = f71882fg_read8(data,
> +						F71882FG_REG_IN(nr));
> +
> +		data->last_updated = jiffies;
> +		data->valid = 1;
> +	}
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return data;
> +}
> +
> +/* Sysfs Interface */
> +static ssize_t show_fan(struct device *dev, struct device_attribute *devattr,
> +	char *buf)
> +{
> +	struct f71882fg_data *data = f71882fg_update_device(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +	int speed = fan_from_reg(data->fan[nr]);
> +
> +	if (speed == FAN_MIN_DETECT)
> +		speed = 0;
> +
> +	return sprintf(buf, "%d\n", speed);
> +}
> +
> +static ssize_t show_fan_beep(struct device *dev, struct device_attribute
> +	*devattr, char *buf)
> +{
> +	struct f71882fg_data *data = f71882fg_update_device(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +
> +	if (data->fan_beep & (1 << nr))
> +		return sprintf(buf, "1\n");
> +	else
> +		return sprintf(buf, "0\n");
> +}
> +
> +static ssize_t store_fan_beep(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count)
> +{
> +	struct f71882fg_data *data = dev_get_drvdata(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +	int val = simple_strtoul(buf, NULL, 10);
> +	
> +	mutex_lock(&data->update_lock);
> +	if (val)
> +		data->fan_beep |= 1 << nr;
> +	else
> +		data->fan_beep &= ~(1 << nr);
> +	
> +	f71882fg_write8(data, F71882FG_REG_FAN_BEEP, data->fan_beep);
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_fan_alarm(struct device *dev, struct device_attribute
> +	*devattr, char *buf)
> +{
> +	struct f71882fg_data *data = f71882fg_update_device(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +
> +	if (data->fan_status & (1 << nr))
> +		return sprintf(buf, "1\n");
> +	else
> +		return sprintf(buf, "0\n");
> +}
> +
> +static ssize_t show_in(struct device *dev, struct device_attribute *devattr,
> +	char *buf)
> +{
> +	struct f71882fg_data *data = f71882fg_update_device(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +
> +	return sprintf(buf, "%d\n", data->in[nr] * 8);
> +}
> +
> +static ssize_t show_in_max(struct device *dev, struct device_attribute
> +	*devattr, char *buf)
> +{
> +	struct f71882fg_data *data = f71882fg_update_device(dev);
> +
> +	return sprintf(buf, "%d\n", data->in1_max * 8);
> +}
> +
> +static ssize_t store_in_max(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count)
> +{
> +	struct f71882fg_data *data = dev_get_drvdata(dev);
> +	int val = simple_strtoul(buf, NULL, 10) / 8;
> +	
> +	if (val > 255)
> +		val = 255;

This check won't catch the case where I feed your sysfs file "-4096"
and the value inside f71882fg_data won't match what gets written to the
port.  Granted, the maxim "If you write garbage to the control systems
you deserve what you get" might apply here too.

> +	
> +	mutex_lock(&data->update_lock);
> +	f71882fg_write8(data, F71882FG_REG_IN1_HIGH, val);
> +	data->in1_max = val;
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_in_beep(struct device *dev, struct device_attribute
> +	*devattr, char *buf)
> +{
> +	struct f71882fg_data *data = f71882fg_update_device(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +
> +	if (data->in_beep & (1 << nr))
> +		return sprintf(buf, "1\n");
> +	else
> +		return sprintf(buf, "0\n");
> +}
> +
> +static ssize_t store_in_beep(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count)
> +{
> +	struct f71882fg_data *data = dev_get_drvdata(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +	int val = simple_strtoul(buf, NULL, 10);
> +	
> +	mutex_lock(&data->update_lock);
> +	if (val)
> +		data->in_beep |= 1 << nr;
> +	else
> +		data->in_beep &= ~(1 << nr);
> +	
> +	f71882fg_write8(data, F71882FG_REG_IN_BEEP, data->in_beep);
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_in_alarm(struct device *dev, struct device_attribute
> +	*devattr, char *buf)
> +{
> +	struct f71882fg_data *data = f71882fg_update_device(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +
> +	if (data->in_status & (1 << nr))
> +		return sprintf(buf, "1\n");
> +	else
> +		return sprintf(buf, "0\n");
> +}
> +
> +static ssize_t show_temp(struct device *dev, struct device_attribute *devattr,
> +	char *buf)
> +{
> +	struct f71882fg_data *data = f71882fg_update_device(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +
> +	return sprintf(buf, "%d\n", data->temp[nr] * 1000);
> +}
> +
> +static ssize_t show_temp_max(struct device *dev, struct device_attribute
> +	*devattr, char *buf)
> +{
> +	struct f71882fg_data *data = f71882fg_update_device(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +
> +	return sprintf(buf, "%d\n", data->temp_high[nr] * 1000);
> +}
> +
> +static ssize_t store_temp_max(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count)
> +{
> +	struct f71882fg_data *data = dev_get_drvdata(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +	int val = simple_strtoul(buf, NULL, 10) / 1000;
> +	
> +	if (val > 255)
> +		val = 255;
> +	
> +	mutex_lock(&data->update_lock);
> +	f71882fg_write8(data, F71882FG_REG_TEMP_HIGH(nr), val);
> +	data->temp_high[nr] = val;
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_temp_max_hyst(struct device *dev, struct device_attribute
> +	*devattr, char *buf)
> +{
> +	struct f71882fg_data *data = f71882fg_update_device(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +
> +	return sprintf(buf, "%d\n",
> +		(data->temp_high[nr] - data->temp_hyst[nr]) * 1000);
> +}
> +
> +static ssize_t store_temp_max_hyst(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count)
> +{
> +	struct f71882fg_data *data = dev_get_drvdata(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +	int val = simple_strtoul(buf, NULL, 10) / 1000;
> +	ssize_t ret = count;
> +	
> +	mutex_lock(&data->update_lock);
> +
> +	/* convert abs to relative and check */
> +	val = data->temp_high[nr] - val;
> +	if (val < 0 || val > 15) {
> +		ret = -EINVAL;
> +		goto store_temp_max_hyst_exit;
> +	}
> +	
> +	data->temp_hyst[nr] = val;
> +	
> +	/* convert value to register contents */
> +	switch(nr) {
> +		case 0:
> +			val = val << 4;
> +			break;
> +		case 1:
> +			val = val | (data->temp_hyst[2] << 4);
> +			break;
> +		case 2:
> +			val = data->temp_hyst[1] | (val << 4);
> +			break;
> +	}
> +
> +	f71882fg_write8(data, nr ? F71882FG_REG_TEMP_HYST23 :
> +		F71882FG_REG_TEMP_HYST1, val);
> +	
> +store_temp_max_hyst_exit:
> +	mutex_unlock(&data->update_lock);
> +	return ret;
> +}
> +
> +static ssize_t show_temp_crit(struct device *dev, struct device_attribute
> +	*devattr, char *buf)
> +{
> +	struct f71882fg_data *data = f71882fg_update_device(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +
> +	return sprintf(buf, "%d\n", data->temp_ovt[nr] * 1000);
> +}
> +
> +static ssize_t store_temp_crit(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count)
> +{
> +	struct f71882fg_data *data = dev_get_drvdata(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +	int val = simple_strtoul(buf, NULL, 10) / 1000;
> +	
> +	if (val > 255)
> +		val = 255;
> +	
> +	mutex_lock(&data->update_lock);
> +	f71882fg_write8(data, F71882FG_REG_TEMP_OVT(nr), val);
> +	data->temp_ovt[nr] = val;
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_temp_crit_hyst(struct device *dev, struct device_attribute
> +	*devattr, char *buf)
> +{
> +	struct f71882fg_data *data = f71882fg_update_device(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +
> +	return sprintf(buf, "%d\n",
> +		(data->temp_ovt[nr] - data->temp_hyst[nr]) * 1000);
> +}
> +
> +static ssize_t show_temp_type(struct device *dev, struct device_attribute
> +	*devattr, char *buf)
> +{
> +	struct f71882fg_data *data = f71882fg_update_device(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +
> +	return sprintf(buf, "%d\n", data->temp_type[nr]);
> +}
> +
> +static ssize_t show_temp_beep(struct device *dev, struct device_attribute
> +	*devattr, char *buf)
> +{
> +	struct f71882fg_data *data = f71882fg_update_device(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +
> +	if (data->temp_beep & (1 << (nr + 1)))
> +		return sprintf(buf, "1\n");
> +	else
> +		return sprintf(buf, "0\n");
> +}
> +
> +static ssize_t store_temp_beep(struct device *dev, struct device_attribute
> +	*devattr, const char *buf, size_t count)
> +{
> +	struct f71882fg_data *data = dev_get_drvdata(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +	int val = simple_strtoul(buf, NULL, 10);
> +	
> +	mutex_lock(&data->update_lock);
> +	if (val)
> +		data->temp_beep |= 1 << (nr + 1);
> +	else
> +		data->temp_beep &= ~(1 << (nr + 1));
> +	
> +	f71882fg_write8(data, F71882FG_REG_TEMP_BEEP, data->temp_beep);
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_temp_alarm(struct device *dev, struct device_attribute
> +	*devattr, char *buf)
> +{
> +	struct f71882fg_data *data = f71882fg_update_device(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +
> +	if (data->temp_status & (1 << (nr + 1)))
> +		return sprintf(buf, "1\n");
> +	else
> +		return sprintf(buf, "0\n");
> +}
> +
> +static ssize_t show_temp_fault(struct device *dev, struct device_attribute
> +	*devattr, char *buf)
> +{
> +	struct f71882fg_data *data = f71882fg_update_device(dev);
> +	int nr = to_sensor_dev_attr(devattr)->index;
> +
> +	if (data->temp_diode_open & (1 << (nr + 1)))
> +		return sprintf(buf, "1\n");
> +	else
> +		return sprintf(buf, "0\n");
> +}
> +
> +static ssize_t show_name(struct device *dev, struct device_attribute *devattr,
> +	char *buf)
> +{
> +	return sprintf(buf, DRVNAME "\n");
> +}
> +
> +
> +static int __devinit f71882fg_probe(struct platform_device * pdev)
> +{
> +	struct f71882fg_data *data;
> +	int err, i;
> +	u8 start_reg;
> +
> +	if(!(data = kzalloc(sizeof(struct f71882fg_data), GFP_KERNEL)))
> +		return -ENOMEM;
> +
> +	data->addr = platform_get_resource(pdev, IORESOURCE_IO, 0)->start;
> +	mutex_init(&data->update_lock);
> +	platform_set_drvdata(pdev, data);
> +
> +	/* Register sysfs interface files */
> +	for (i = 0; i < ARRAY_SIZE(f71882fg_dev_attr); i++) {
> +		err = device_create_file(&pdev->dev, &f71882fg_dev_attr[i]);
> +		if (err)
> +			goto exit_unregister_sysfs;
> +	}
> +	
> +	start_reg = f71882fg_read8(data, F71882FG_REG_START);
> +	if (start_reg & 0x01) {
> +		for (i = 0; i < ARRAY_SIZE(f71882fg_in_temp_attr); i++) {
> +			err = device_create_file(&pdev->dev,
> +					&f71882fg_in_temp_attr[i].dev_attr);
> +			if (err)
> +				goto exit_unregister_sysfs;
> +		}
> +	}
> +
> +	if (start_reg & 0x02) {
> +		for (i = 0; i < ARRAY_SIZE(f71882fg_fan_attr); i++) {
> +			err = device_create_file(&pdev->dev,
> +					&f71882fg_fan_attr[i].dev_attr);
> +			if (err)
> +				goto exit_unregister_sysfs;
> +		}
> +	}
> +	
> +	data->class_dev = hwmon_device_register(&pdev->dev);
> +	if (IS_ERR(data->class_dev)) {
> +		err = PTR_ERR(data->class_dev);
> +		goto exit_unregister_sysfs;
> +	}
> +
> +	return 0;
> +
> +exit_unregister_sysfs:
> +	for (i = 0; i < ARRAY_SIZE(f71882fg_dev_attr); i++)
> +		device_remove_file(&pdev->dev, &f71882fg_dev_attr[i]);
> +	
> +	for (i = 0; i < ARRAY_SIZE(f71882fg_in_temp_attr); i++)
> +		device_remove_file(&pdev->dev,
> +					&f71882fg_in_temp_attr[i].dev_attr);
> +	
> +	for (i = 0; i < ARRAY_SIZE(f71882fg_fan_attr); i++)
> +		device_remove_file(&pdev->dev, &f71882fg_fan_attr[i].dev_attr);
> +
> +	kfree(data);
> +
> +	return err;
> +}
> +
> +static int __devexit f71882fg_remove(struct platform_device *pdev)
> +{
> +	int i;
> +	struct f71882fg_data *data = platform_get_drvdata(pdev);
> +
> +	platform_set_drvdata(pdev, NULL);
> +	hwmon_device_unregister(data->class_dev);
> +
> +	for (i = 0; i < ARRAY_SIZE(f71882fg_dev_attr); i++)
> +		device_remove_file(&pdev->dev, &f71882fg_dev_attr[i]);
> +	
> +	for (i = 0; i < ARRAY_SIZE(f71882fg_in_temp_attr); i++)
> +		device_remove_file(&pdev->dev,
> +					&f71882fg_in_temp_attr[i].dev_attr);
> +	
> +	for (i = 0; i < ARRAY_SIZE(f71882fg_fan_attr); i++)
> +		device_remove_file(&pdev->dev, &f71882fg_fan_attr[i].dev_attr);
> +
> +	kfree(data);
> +
> +	return 0;
> +}
> +
> +static int __init f71882fg_find(int sioaddr, unsigned short *address)
> +{
> +	int err = -ENODEV;
> +	u16 devid;
> +	u8 start_reg;
> +	struct f71882fg_data data;
> +
> +	superio_enter(sioaddr);
> +	
> +	devid = superio_inw(sioaddr, SIO_REG_MANID);
> +	if (devid != SIO_FINTEK_ID) {
> +		printk(KERN_INFO DRVNAME ": Not a Fintek device\n");
> +		goto exit;
> +	}
> +
> +	devid = superio_inw(sioaddr, SIO_REG_DEVID);
> +	if (devid != SIO_F71882_ID) {
> +		printk(KERN_INFO DRVNAME ": Unsupported Fintek device\n");
> +		goto exit;
> +	}
> +
> +	superio_select(sioaddr, SIO_F71882FG_LD_HWM);
> +	if (!(superio_inb(sioaddr, SIO_REG_ENABLE) & 0x01))  {
> +		printk(KERN_WARNING DRVNAME ": Device not activated\n");
> +		goto exit;
> +	}
> +
> +	*address = superio_inw(sioaddr, SIO_REG_ADDR);
> +	if (*address == 0) 
> +	{
> +		printk(KERN_WARNING DRVNAME ": Base address not set\n");
> +		goto exit;
> +	}
> +	*address &= ~(REGION_LENGTH - 1);	/* Ignore 3 LSB */
> +	
> +	data.addr = *address;
> +	start_reg = f71882fg_read8(&data, F71882FG_REG_START);
> +	if (!(start_reg & 0x03)) {
> +		printk(KERN_WARNING DRVNAME
> +			": Hardware monitoring not activated\n");
> +		goto exit;
> +	}
> +
> +	err = 0;
> +	printk(KERN_INFO DRVNAME ": Found F71882FG chip at %#x, revision %d\n",
> +		(unsigned int)*address,
> +		(int)superio_inb(sioaddr, SIO_REG_DEVREV));
> +exit:
> +	superio_exit(sioaddr);
> +	return err;
> +}
> +
> +static int __init f71882fg_device_add(unsigned short address)
> +{
> +	struct resource res = {
> +		.start	= address,
> +		.end	= address + REGION_LENGTH - 1,
> +		.flags	= IORESOURCE_IO,
> +	};
> +	int err;
> +
> +	f71882fg_pdev = platform_device_alloc(DRVNAME, address);
> +	if(!f71882fg_pdev)
> +		return -ENOMEM;
> +
> +	res.name = f71882fg_pdev->name;
> +	err = platform_device_add_resources(f71882fg_pdev, &res, 1);
> +	if(err) {
> +		printk(KERN_ERR DRVNAME ": Device resource addition failed\n");
> +		goto exit_device_put;
> +	}
> +
> +	err = platform_device_add(f71882fg_pdev);
> +	if(err) {
> +		printk(KERN_ERR DRVNAME ": Device addition failed\n");
> +		goto exit_device_put;
> +	}
> +	
> +	return 0;
> +
> +exit_device_put:
> +	platform_device_put(f71882fg_pdev);
> +
> +	return err;
> +}
> +
> +static int __init f71882fg_init(void)
> +{
> +	int err = -ENODEV;
> +	unsigned short address;
> +
> +	if (f71882fg_find(0x2e, &address) && f71882fg_find(0x4e, &address))
> +		goto exit;
> +
> +	if ((err = platform_driver_register(&f71882fg_driver)))
> +		goto exit;
> +	
> +	if ((err = f71882fg_device_add(address)))
> +		goto exit_driver;
> +
> +	return 0;
> +
> +exit_driver:
> +	platform_driver_unregister(&f71882fg_driver);
> +exit:
> +	return err;
> +}
> +
> +static void __exit f71882fg_exit(void)
> +{
> +	platform_device_unregister(f71882fg_pdev);
> +	platform_driver_unregister(&f71882fg_driver);
> +}
> +
> +MODULE_DESCRIPTION("F71882FG Hardware Monitoring Driver");
> +MODULE_AUTHOR("Hans Edgington (hans@edgington.nl)");
> +MODULE_LICENSE("GPL");
> +
> +module_init(f71882fg_init);
> +module_exit(f71882fg_exit);
> diff -up linux-2.6.22-rc4/drivers/hwmon/Kconfig.newdriver linux-2.6.22-rc4/drivers/hwmon/Kconfig
> --- linux-2.6.22-rc4/drivers/hwmon/Kconfig.newdriver	2007-07-06 15:39:00.000000000 +0200
> +++ linux-2.6.22-rc4/drivers/hwmon/Kconfig	2007-07-06 15:41:07.000000000 +0200
> @@ -199,6 +199,16 @@ config SENSORS_F71805F
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called f71805f.
>  
> +config SENSORS_F71882FG
> +	tristate "Fintek F71882FG and F71883FG"
> +	depends on EXPERIMENTAL
> +	help
> +	  If you say yes here you get support for hardware monitoring
> +	  features of the Fintek F71882FG and F71883FG Super-I/O chips.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called f71882fg.
> +
>  config SENSORS_FSCHER
>  	tristate "FSC Hermes"
>  	depends on I2C
> diff -up linux-2.6.22-rc4/drivers/hwmon/Makefile.newdriver linux-2.6.22-rc4/drivers/hwmon/Makefile
> --- linux-2.6.22-rc4/drivers/hwmon/Makefile.newdriver	2007-07-06 15:39:10.000000000 +0200
> +++ linux-2.6.22-rc4/drivers/hwmon/Makefile	2007-07-06 15:41:52.000000000 +0200
> @@ -27,6 +27,7 @@ obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
>  obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
>  obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
>  obj-$(CONFIG_SENSORS_F71805F)	+= f71805f.o
> +obj-$(CONFIG_SENSORS_F71882FG)	+= f71882fg.o
>  obj-$(CONFIG_SENSORS_FSCHER)	+= fscher.o
>  obj-$(CONFIG_SENSORS_FSCPOS)	+= fscpos.o
>  obj-$(CONFIG_SENSORS_GL518SM)	+= gl518sm.o

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Hans de Goede]

Darrick J. Wong wrote:
> On Sat, Jul 07, 2007 at 10:21:57AM +0200, Hans de Goede wrote:
> 
>> Why all the base defines? Why not put the base address directly in the () 
>> macros? You do not seem to use them anywhere else.
> 
> I prefer to put all the register and bit-field definitions in one place
> so that later I can construct a map of all known registers after I've
> forgotten which ones the driver talks to and which ones it doesn't.  The
> accessor macros that come after that are merely sugar, which is why I
> don't like encoding the base register address directly into the macro.
> 

Ok, leave them in then.

>> Are you sure you only want to support revision 2? What are the differences? 
>> Shouldn't the driver be able to support other revisions too? Maybe only 
>> print a warning when its used with an untested revision?
> 
> I don't have a data sheet for other revisions, so the safest thing to do is
> to reject that which we don't know about; an interested user can always
> bypass those safety checks with force_adt7470= at a later point in time
> and post a patch if the driver still works ok.
> 

Good point about using the force parameter, ok.

>> Why use read_byte twice in read16, but write_word in write16?
> 
> A "16 bit" register is really two 8 bit register reads/writes, and the
> register with the lower address has to be read/written first.  Hence the
> weird helper functions.
> 




>>> +		/* delay is 200ms * number of tmp05 sensors */
>>> +		udelay(TEMP_COLLECTION_TIME);
>> ugh, sleep here please.
> 
> Would msleep_interruptible() work here?  Some informal testing shows
> that it seems to work ok, though I've not subjected it to a rigorous ^C
> test to see if I can hit weird crash conditions.
> 

Well msleep_interruptible() can be interrupted (you check the return value for 
this) so if a user presses control-C, then your sleep will be cut short. So 
either you must be prepared to handle this, or sleep uninterruptible. I would 
sleep uninterruptible if I were you, using regular msleep.


Regards,

Hans

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

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Hans de Goede]

Darrick J. Wong wrote:

I just realized I failed to respond to this point, here is my response:

>> Why use read_byte twice in read16, but write_word in write16?
> 
> A "16 bit" register is really two 8 bit register reads/writes, and the
> register with the lower address has to be read/written first.  Hence the
> weird helper functions.
> 

Here is the code you use for this:

+static inline int adt7470_read_word_data(struct i2c_client *client, u8 reg)
+{
+	u16 foo;
+	foo = i2c_smbus_read_byte_data(client, reg);
+	foo |= ((u16)i2c_smbus_read_byte_data(client, reg + 1) << 8);
+	return foo;
+}
+
+static inline int adt7470_write_word_data(struct i2c_client *client, u8 reg,
+					  u16 value)
+{
+	u16 x = cpu_to_le16(value);
+
+	return i2c_smbus_write_word_data(client, reg, x & 0xFF)
+	       && i2c_smbus_write_word_data(client, reg + 1, x >> 8);
+}
+

I understand that you need to somehow read / write a byte 2 times to get a 
word, but why in adt7470_read_word_data() call i2c_smbus_read_byte_data twice? 
while in adt7470_write_word_data() you use cpu_to_le16() and then 
i2c_smbus_write_word_data(). Why not use i2c_smbus_read_word_data() in 
adt7470_read_word_data() instead of calling i2c_smbus_read_byte_data twice?

BTW I have serious doubts about you calling cpu_to_le16(), I would expect that 
i2c_smbus_write_word_data() wants the data to be in cpu endianness.

Regards,

Hans



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

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Hans de Goede]

Hi All,

While actually updating the driver I realized I didn't respond to all points 
raised (I think), so here is my reply to the other points.

>> +static struct f71882fg_data *f71882fg_update_device(struct device * dev)
>> +{
>> +	struct f71882fg_data *data = dev_get_drvdata(dev);
>> +	int nr, reg, reg2;
>> +
>> +	mutex_lock(&data->update_lock);
>> +	
>> +	/* Update once every 60 seconds */
>> +	if ( time_after(jiffies, data->last_updated + 60 * HZ ) ||
>> +			!data->valid) {
> 
> You test last_updated here but update last_limits below.  Is that
> really what you intended?  last_limits isn't used anywhere in
> the driver.
> 

Good catch, will fix.

>> +static ssize_t store_in_max(struct device *dev, struct device_attribute
>> +	*devattr, const char *buf, size_t count)
>> +{
>> +	struct f71882fg_data *data = dev_get_drvdata(dev);
>> +	int val = simple_strtoul(buf, NULL, 10) / 8;
>> +	
>> +	if (val > 255)
>> +		val = 255;
> 
> This check won't catch the case where I feed your sysfs file "-4096"
> and the value inside f71882fg_data won't match what gets written to the
> port.  Granted, the maxim "If you write garbage to the control systems
> you deserve what you get" might apply here too.
> 

Notice that I use simple_strtoul, so val will never be < 0, if you write -4096 
strtoul will not recognize the - and return 0.

I know it looks strange to store the return value in an int then, but in some 
of the other store methods I need val to be signed, and for consistency I've 
thus stored it into an int everywhere.


Regards,

Hans


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

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Vadim Zeitlin]

On Sun, 8 Jul 2007 18:53:12 +0200 Jean Delvare <khali@linux-fr.org> wrote:

JD> On Fri, 6 Jul 2007 14:11:45 -0700, Darrick J. Wong wrote:
JD> > Below is a first draft of a driver for the ADT7470 chip.  I've tested
JD> > the driver against the adt7470 in the IBM IntelliStation Z30 and as far
JD> > as I can tell it's good for controlling the speed of the CPU heatsink
JD> > fans and monitor CPU temperature.  Please let me know what you think of
JD> > the driver.  The patch should apply cleanly against 2.6.22-rc7.
JD> 
JD> I've added an entry in our wiki Devices page to track this one.
JD> 
JD> Note that back in January this year, someone (Vadim Zeitlin, Cc'd)
JD> asked for support for this chip. So maybe Vadim would be interested in
JD> testing your driver.

 Hello,

 Thanks a lot Jean for cc'ing this to me and, of course, thanks a lot to
Darrick for writing the driver (and sorry for never getting around to
writing it myself...)! I was away during this week so I couldn't test it
before but now I got back and so I've just compiled the driver for my
current 2.6.20 kernel. The module loads (accompanied by a brief switch to
maximal fan speed judging by the noise) and creates its directory under
/sys/bus/i2c/drivers/adt7470. I don't really know what do I have to do to
make it work with lm-sensors (do I need the svn version? Debian only has
2.10.1 currently) so for now I just tried to manually check the files in
this directory.


 The first thing I noticed is that accessing any of them takes almost 3
seconds:

# time cat fan1_input
2001
cat fan1_input  0.00s user 0.00s system 0% cpu 2.760 total

I wonder if this is normal? I see a 1 sec sleep in adt7470_update_device()
but the delay happens not only for the temperatures and it's also much
bigger than 1 second.

 Next, the fan speed detection seems to work as expected, i.e. the number
it gives (like 2000 above) seem to be RPM. But reading the temperature
files produces strange results:

# grep . temp?_input
temp1_input:0
temp2_input:228000
temp3_input:59000
temp4_input:54000
temp5_input:226000
temp6_input:228000
temp7_input:58000
temp8_input:54000
temp9_input:49000

Again, I see the factor of 1000 in adt7470.c and, accounting for it, all
numbers except 227000 may be correct -- but what about those latter ones?
To confuse the matters (or at least me) further, temp_min files contain
129000 while temp_max files have 127000 (no typo) so something seems wrong
here... At the very least, the factor of 1000 seems to be unnecessary:

# grep . pwm1*
pwm1:0
pwm1_auto_channels_temp:1023
pwm1_auto_point1_pwm:128
pwm1_auto_point1_temp:90000
pwm1_auto_point2_pwm:255
pwm1_auto_point2_temp:110000
pwm1_enable:2

The values here clearly should be 90 and 110 degrees, shouldn't they?

 Please let me know what else should I test, thanks in advance!
VZ


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

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Jean Delvare]

Hi Hans,

On Fri, 13 Jul 2007 14:08:53 +0200, Hans de Goede wrote:
> Notice that I use simple_strtoul, so val will never be < 0, if you write -4096 
> strtoul will not recognize the - and return 0.
> 
> I know it looks strange to store the return value in an int then, but in some 
> of the other store methods I need val to be signed, and for consistency I've 
> thus stored it into an int everywhere.

But wouldn't then a large input value (fitting in a long but not in an
int) be silently turned into a negative value, possibly doing crazy
things in the rest of the code?

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
> On Fri, 13 Jul 2007 14:08:53 +0200, Hans de Goede wrote:
>> Notice that I use simple_strtoul, so val will never be < 0, if you write -4096 
>> strtoul will not recognize the - and return 0.
>>
>> I know it looks strange to store the return value in an int then, but in some 
>> of the other store methods I need val to be signed, and for consistency I've 
>> thus stored it into an int everywhere.
> 
> But wouldn't then a large input value (fitting in a long but not in an
> int) be silently turned into a negative value, possibly doing crazy
> things in the rest of the code?
> 

Yes,

That could happen, but then people are _really_ asking for it. Notice that most 
hwmon drivers store functions do even less checking then this. We really need 
to discuss this and make a decision on it for all hwmon drivers, starting with 
the question wether or not to check if the user input actually is a number?

Currently a user can do:
echo -n foo > /sys/class/hwmon/hwmon0/in0_max
and not get an error (instead in0_max typically gets set to 0

Regards,

Hans

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

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Darrick J. Wong]

[-- Attachment #1.1: Type: text/plain, Size: 2322 bytes --]

On Sat, Jul 14, 2007 at 04:23:16PM +0200, Vadim Zeitlin wrote:
>  The first thing I noticed is that accessing any of them takes almost 3
> seconds:
> 
> # time cat fan1_input
> 2001
> cat fan1_input  0.00s user 0.00s system 0% cpu 2.760 total

Hm.... I see this with the v3 patch:
root@elm3a181:/usr/local/src/lm_sensors-2.10.3/prog# time cat
/sys/class/hwmon/hwmon0/device/temp1_input 
47000

real    0m1.701s
user    0m0.000s
sys     0m0.000s

Not sure why it takes 700ms to read the sensors after the 1s sleep,
perhaps that could just be the cost of reading the registers?  (I'd hope
not...)

> I wonder if this is normal? I see a 1 sec sleep in adt7470_update_device()
> but the delay happens not only for the temperatures and it's also much
> bigger than 1 second.
> 
>  Next, the fan speed detection seems to work as expected, i.e. the number
> it gives (like 2000 above) seem to be RPM. But reading the temperature
> files produces strange results:
> 
> # grep . temp?_input
> temp1_input:0
> temp2_input:228000
> temp3_input:59000
> temp4_input:54000
> temp5_input:226000
> temp6_input:228000
> temp7_input:58000
> temp8_input:54000
> temp9_input:49000
> 
> Again, I see the factor of 1000 in adt7470.c and, accounting for it, all

Documentation/hwmon/sysfs-interface says that the units for temperatures
are millidegrees C, hence the 1000 factor both here and for the pwm
controls below.

> numbers except 227000 may be correct -- but what about those latter ones?
> To confuse the matters (or at least me) further, temp_min files contain
> 129000 while temp_max files have 127000 (no typo) so something seems wrong

Mine are programmed like that too.  I suppose it's possible that the
datasheet could be wrong?  Page 14 of the adt7470 datasheet says that
the default temp low limit is 0x81 and the default temp high limit is
0x7F.  Weird...unless I'm misinterpreting the data sheet?

> here... At the very least, the factor of 1000 seems to be unnecessary:
> 
> # grep . pwm1*
> pwm1:0
> pwm1_auto_channels_temp:1023
> pwm1_auto_point1_pwm:128
> pwm1_auto_point1_temp:90000
> pwm1_auto_point2_pwm:255
> pwm1_auto_point2_temp:110000
> pwm1_enable:2
> 
> The values here clearly should be 90 and 110 degrees, shouldn't they?

(See above)

--D

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Philip Pokorny]

[-- Attachment #1: Type: text/plain, Size: 215 bytes --]

--===============9088171502961478393==
Content-class: urn:content-classes:message
Content-Type: multipart/alternative;
	boundary="----_=_NextPart_001_01C7C7F7.5181AC1C"

This is a multi-part message in MIME format.

[-- Attachment #2: Type: text/plain, Size: 2790 bytes --]

The values 0x81 and 0x7f are *signed* 8 bit quantities.  0x81 whould be considered -127

:v)

-- 
Philip Pokorny, RHCE
Director of Field Engineering 
Penguin Computing  http://www.penguincomputing.com

 -----Original Message-----
From: 	Darrick J. Wong [mailto:djwong@us.ibm.com]
Sent:	Monday, July 16, 2007 01:21 PM Pacific Standard Time
To:	Vadim Zeitlin
Cc:	lm-sensors@lm-sensors.org
Subject:	Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

On Sat, Jul 14, 2007 at 04:23:16PM +0200, Vadim Zeitlin wrote:
>  The first thing I noticed is that accessing any of them takes almost 3
> seconds:
> 
> # time cat fan1_input
> 2001
> cat fan1_input  0.00s user 0.00s system 0% cpu 2.760 total

Hm.... I see this with the v3 patch:
root@elm3a181:/usr/local/src/lm_sensors-2.10.3/prog# time cat
/sys/class/hwmon/hwmon0/device/temp1_input 
47000

real    0m1.701s
user    0m0.000s
sys     0m0.000s

Not sure why it takes 700ms to read the sensors after the 1s sleep,
perhaps that could just be the cost of reading the registers?  (I'd hope
not...)

> I wonder if this is normal? I see a 1 sec sleep in adt7470_update_device()
> but the delay happens not only for the temperatures and it's also much
> bigger than 1 second.
> 
>  Next, the fan speed detection seems to work as expected, i.e. the number
> it gives (like 2000 above) seem to be RPM. But reading the temperature
> files produces strange results:
> 
> # grep . temp?_input
> temp1_input:0
> temp2_input:228000
> temp3_input:59000
> temp4_input:54000
> temp5_input:226000
> temp6_input:228000
> temp7_input:58000
> temp8_input:54000
> temp9_input:49000
> 
> Again, I see the factor of 1000 in adt7470.c and, accounting for it, all

Documentation/hwmon/sysfs-interface says that the units for temperatures
are millidegrees C, hence the 1000 factor both here and for the pwm
controls below.

> numbers except 227000 may be correct -- but what about those latter ones?
> To confuse the matters (or at least me) further, temp_min files contain
> 129000 while temp_max files have 127000 (no typo) so something seems wrong

Mine are programmed like that too.  I suppose it's possible that the
datasheet could be wrong?  Page 14 of the adt7470 datasheet says that
the default temp low limit is 0x81 and the default temp high limit is
0x7F.  Weird...unless I'm misinterpreting the data sheet?

> here... At the very least, the factor of 1000 seems to be unnecessary:
> 
> # grep . pwm1*
> pwm1:0
> pwm1_auto_channels_temp:1023
> pwm1_auto_point1_pwm:128
> pwm1_auto_point1_temp:90000
> pwm1_auto_point2_pwm:255
> pwm1_auto_point2_temp:110000
> pwm1_enable:2
> 
> The values here clearly should be 90 and 110 degrees, shouldn't they?

(See above)

--D

[-- Attachment #3: Type: text/html, Size: 3794 bytes --]

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Jean Delvare]

Hi Vadim,

On Sat, 14 Jul 2007 16:23:16 +0200, Vadim Zeitlin wrote:
> # grep . temp?_input
> temp1_input:0
> temp2_input:228000
> temp3_input:59000
> temp4_input:54000
> temp5_input:226000
> temp6_input:228000
> temp7_input:58000
> temp8_input:54000
> temp9_input:49000

Thanks for this tip. I've always wondered how to easily list sysfs
files and their content all at once, and this grep trick works really
well :)

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Jean Delvare]

Hi Hans,

Sorry for the late answer, I'm swamped :(

On Sat, 14 Jul 2007 22:17:34 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > On Fri, 13 Jul 2007 14:08:53 +0200, Hans de Goede wrote:
> >> I know it looks strange to store the return value in an int then, but in some 
> >> of the other store methods I need val to be signed, and for consistency I've 
> >> thus stored it into an int everywhere.
> > 
> > But wouldn't then a large input value (fitting in a long but not in an
> > int) be silently turned into a negative value, possibly doing crazy
> > things in the rest of the code?
> 
> That could happen, but then people are _really_ asking for it. Notice that most 
> hwmon drivers store functions do even less checking then this. We really need 
> to discuss this and make a decision on it for all hwmon drivers, starting with 
> the question wether or not to check if the user input actually is a number?
> 
> Currently a user can do:
> echo -n foo > /sys/class/hwmon/hwmon0/in0_max
> and not get an error (instead in0_max typically gets set to 0

I am totally fine handling really invalid values as 0. As you said,
people doing that are asking for trouble, and handling this case would
slow down and/or enlarge our drivers quite a bit.

OTOH, the reason why I think that numbers should always be treated
correctly, even if out of bounds, is that they may be written by
libsensors rather than directly by a human. And libsensors may have
done some arithmetic operations on the values based
on /etc/sensors.conf, in particular for voltages. This can easily
result in a negative value being written to a voltage sysfs file. This
is really important that we correctly trim the negative value to 0 mV
and not to 4080 mV, otherwise we trigger an unwanted alarm. This is
only an example but it shows why we have to be careful.

That being said, this is kind of low priority and I am fine fixing the
drivers when people report such problems. I really don't have the time
to go thought all drivers to make sure that they are always correct
(but wouldn't mind if someone else does, of course.)

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
> Sorry for the late answer, I'm swamped :(
> 

No problem, I'm very happy that you've found the time eventually, thanks!

> On Sat, 14 Jul 2007 22:17:34 +0200, Hans de Goede wrote:
>> Jean Delvare wrote:
>>> On Fri, 13 Jul 2007 14:08:53 +0200, Hans de Goede wrote:
>>>> I know it looks strange to store the return value in an int then, but in some 
>>>> of the other store methods I need val to be signed, and for consistency I've 
>>>> thus stored it into an int everywhere.
>>> But wouldn't then a large input value (fitting in a long but not in an
>>> int) be silently turned into a negative value, possibly doing crazy
>>> things in the rest of the code?
>> That could happen, but then people are _really_ asking for it. Notice that most 
>> hwmon drivers store functions do even less checking then this. We really need 
>> to discuss this and make a decision on it for all hwmon drivers, starting with 
>> the question wether or not to check if the user input actually is a number?
>>
>> Currently a user can do:
>> echo -n foo > /sys/class/hwmon/hwmon0/in0_max
>> and not get an error (instead in0_max typically gets set to 0
> 
> I am totally fine handling really invalid values as 0. As you said,
> people doing that are asking for trouble, and handling this case would
> slow down and/or enlarge our drivers quite a bit.
> 
> OTOH, the reason why I think that numbers should always be treated
> correctly, even if out of bounds, is that they may be written by
> libsensors rather than directly by a human. And libsensors may have
> done some arithmetic operations on the values based
> on /etc/sensors.conf, in particular for voltages. This can easily
> result in a negative value being written to a voltage sysfs file. This
> is really important that we correctly trim the negative value to 0 mV
> and not to 4080 mV, otherwise we trigger an unwanted alarm. This is
> only an example but it shows why we have to be careful.
> 

I agree that we should do some sanity checking, but not too much as to not 
clutter the drivers. However, I think we first need to define some kind of 
standard as to what sanity checking we will do, and what not, and how to handle 
out of range values (clamp versus EINVAL), actually I've written and send a 
proposal for this to the list already as our discussion got me thinking about this.

> That being said, this is kind of low priority and I am fine fixing the
> drivers when people report such problems. I really don't have the time
> to go thought all drivers to make sure that they are always correct
> (but wouldn't mind if someone else does, of course.)
> 

I agree that this is somewhat of a minor issue, and I do not advocate to go in 
and fix everything immediately once my proposal is accepted, one of the main 
purposes of my proposal is to have some guidelines / rules for input checking 
when reviewing patches. If we find the time and energy for it we could also fix 
existing drivers, but thats not high priority.

Regards,

Hans

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

Re: [lm-sensors] [PATCH] First cut of a adt7470 driver

[Jean Delvare]

On Thu, 19 Jul 2007 19:11:04 +0200, Hans de Goede wrote:
> I agree that we should do some sanity checking, but not too much as to not 
> clutter the drivers. However, I think we first need to define some kind of 
> standard as to what sanity checking we will do, and what not, and how to handle 
> out of range values (clamp versus EINVAL), actually I've written and send a 
> proposal for this to the list already as our discussion got me thinking about this.

What I've been telling contributors so far is: clamp for "continuous"
values those boundaries aren't known to the user, -EINVAL for the rest.
The rationale is essentially the same I mentioned earlier in this
thread: limits can be computed by libsensors, and we do not want a
slightly out-of-bound values to be ignored or trigger errors. But for
pretty much all the rest, returning an error on invalid values seems to
be the sane thing to do.

I agree that having these rules written somewhere would probably save
some question from new driver contributors.

-- 
Jean Delvare

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