[lm-sensors] [PATCH] AD7416/17/18 driver

[Jean Delvare]

Hi Alessandro,

On Sun, 25 Mar 2007 22:53:00 +0200, Alessandro Zummo wrote:
>  A driver for the Analog Devices AD7416/17/18 chips.
> 
>  Signed-off-by: Alessandro Zummo <a.zummo at towertech.it>
> 
> ---
>  drivers/hwmon/Kconfig  |   10 +
>  drivers/hwmon/Makefile |    1 
>  drivers/hwmon/ad7418.c |  366 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 377 insertions(+)
> 
> --- linux-ixp4xx.orig/drivers/hwmon/Makefile	2007-03-25 19:55:07.000000000 +0200
> +++ linux-ixp4xx/drivers/hwmon/Makefile	2007-03-25 22:48:52.000000000 +0200
> @@ -14,6 +14,7 @@ obj-$(CONFIG_SENSORS_W83781D)	+= w83781d
>  obj-$(CONFIG_SENSORS_W83791D)	+= w83791d.o
>  
>  obj-$(CONFIG_SENSORS_ABITUGURU)	+= abituguru.o
> +obj-$(CONFIG_SENSORS_AD7418)	+= ad7418.o
>  obj-$(CONFIG_SENSORS_ADM1021)	+= adm1021.o
>  obj-$(CONFIG_SENSORS_ADM1025)	+= adm1025.o
>  obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-ixp4xx/drivers/hwmon/ad7418.c	2007-03-25 22:48:52.000000000 +0200
> @@ -0,0 +1,366 @@
> +/*
> + * An hwmon driver for the Analog Devices AD7416/17/18
> + * Copyright (C) 2006-07 Tower Technologies
> + *
> + * Author: Alessandro Zummo <a.zummo at towertech.it>
> + *
> + * Based on lm75.c
> + * Copyright (C) 1998-99 Frodo Looijaard <frodol at dds.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 - 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 "lm75.h"
> +
> +#define DRV_VERSION "0.3"
> +
> +/* Addresses to scan */
> +static unsigned short normal_i2c[] = { 0x28, 0x29, 0x2A, 0x2B, 0x2C, 0x2D, 0x2E, 0x2F,

Line too long.

> +				0x48, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x4E, 0x4F,
> +				I2C_CLIENT_END };
> +/* Insmod parameters */
> +I2C_CLIENT_INSMOD_3(ad7416, ad7417, ad7418);
> +
> +/* AD7418 registers */
> +#define AD7418_REG_TEMP		0x00
> +#define AD7418_REG_CONF		0x01
> +#define AD7418_REG_TEMP_HYST	0x02
> +#define AD7418_REG_TEMP_OS	0x03
> +#define AD7418_REG_ADC		0x04
> +#define AD7418_REG_CONF2	0x05
> +
> +#define AD7418_REG_ADC_CH(x)	((x) << 5)
> +#define AD7418_CH_TEMP		AD7418_REG_ADC_CH(0)
> +
> +struct ad7418_data {
> +	struct i2c_client	client;
> +	struct class_device	*class_dev;
> +	struct attribute_group	attrs;
> +	enum chips		type;
> +	struct mutex		lock;
> +	char			valid;		/* != 0 if following fields are valid */

Line too long.

> +	unsigned long		last_updated;	/* In jiffies */
> +	s16			temp[3];	/* Register values */
> +	u16			in[4];
> +};
> +
> +static int ad7418_attach_adapter(struct i2c_adapter *adapter);
> +static int ad7418_detect(struct i2c_adapter *adapter, int address, int kind);
> +static int ad7418_detach_client(struct i2c_client *client);
> +
> +static struct i2c_driver ad7418_driver = {
> +	.driver = {
> +		.name	= "ad7418",
> +	},
> +	.attach_adapter	= ad7418_attach_adapter,
> +	.detach_client	= ad7418_detach_client,
> +};
> +
> +/* All registers are word-sized, except for the configuration registers.
> + * AD7418 uses a high-byte first convention. Do NOT use those functions to
> + * access the configuration registers CONF and CONF2, as they are byte-sized.
> + */
> +static int ad7418_read(struct i2c_client *client, u8 reg)
> +{
> +	return swab16(i2c_smbus_read_word_data(client, reg));
> +}
> +
> +static int ad7418_write(struct i2c_client *client, u8 reg, u16 value)
> +{
> +	return i2c_smbus_write_word_data(client, reg, swab16(value));
> +}

You could inline these two functions for better performance (and, I
suspect, smaller driver.)

> +
> +static void ad7418_init_client(struct i2c_client *client)
> +{
> +	struct ad7418_data *data = i2c_get_clientdata(client);
> +
> +	int reg = i2c_smbus_read_byte_data(client, AD7418_REG_CONF);
> +	if (reg < 0) {
> +		dev_err(&client->dev, "cannot read configuration register\n");
> +	} else {
> +		dev_info(&client->dev, "configuring for mode 1\n");
> +		i2c_smbus_write_byte_data(client, AD7418_REG_CONF, reg & 0xfe);
> +
> +		if (data->type == ad7417 || data->type == ad7418)
> +			i2c_smbus_write_byte_data(client, AD7418_REG_CONF2, 0x00);

Line too long.

> +	}
> +}
> +
> +static struct ad7418_data *ad7418_update_device(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ad7418_data *data = i2c_get_clientdata(client);
> +
> +	mutex_lock(&data->lock);
> +
> +	if (time_after(jiffies, data->last_updated + HZ + HZ / 2)
> +		|| !data->valid) {
> +		u8 cfg;
> +		int i;
> +
> +		/* read config register and clear channel bits */
> +		cfg = i2c_smbus_read_byte_data(client, AD7418_REG_CONF);
> +		cfg &= 0x1F;
> +
> +		i2c_smbus_write_byte_data(client, AD7418_REG_CONF, cfg | AD7418_CH_TEMP);

Line too long.

> +		udelay(30);
> +
> +		data->temp[0] = ad7418_read(client, AD7418_REG_TEMP);
> +		data->temp[1] = ad7418_read(client, AD7418_REG_TEMP_HYST);
> +		data->temp[2] = ad7418_read(client, AD7418_REG_TEMP_OS);
> +
> +		if (data->type == ad7417) {
> +			for (i = 0; i < 4; i++) {
> +				i2c_smbus_write_byte_data(client, AD7418_REG_CONF,

Line too long.

> +					cfg | AD7418_REG_ADC_CH(i+1));
> +				udelay(15);
> +				data->in[i] = ad7418_read(client, AD7418_REG_ADC);

Line too long.

> +			}
> +		} else if (data->type == ad7418) {
> +			i2c_smbus_write_byte_data(client, AD7418_REG_CONF,
> +				cfg | AD7418_REG_ADC_CH(4));
> +			udelay(15);
> +			data->in[0] = ad7418_read(client, AD7418_REG_ADC);
> +		}

There is some redundancy here, which could be avoided. You could store
the number of voltage input channels (0 for ad7416, 1 for ad7418, 4 for
ad7417), and use that as the upper boundary of the for loop.

> +
> +		/* restore old configuration value */
> +		ad7418_write(client, AD7418_REG_CONF, cfg);
> +
> +		data->last_updated = jiffies;
> +		data->valid = 1;
> +	}
> +
> +	mutex_unlock(&data->lock);
> +
> +	return data;
> +}
> +
> +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 ad7418_data *data = ad7418_update_device(dev);
> +	return sprintf(buf, "%d\n", LM75_TEMP_FROM_REG(data->temp[attr->index]));

Line too long.

> +}
> +
> +static ssize_t show_adc(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct ad7418_data *data = ad7418_update_device(dev);
> +
> +	int nr = (data->type == ad7418) ? 0 : attr->index;

attr->index will be 0 for the ad7418 anyway, so this test isn't needed.

> +
> +	return sprintf(buf, "%d\n", ((data->in[nr] >> 6) * 2500 + 512) / 1024);
> +}
> +
> +static ssize_t set_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 ad7418_data *data = i2c_get_clientdata(client);
> +	int temp = simple_strtoul(buf, NULL, 10);

Should be strtol, otherwise you cannot set negative limits.

> +
> +	mutex_lock(&data->lock);
> +	data->temp[attr->index] = LM75_TEMP_TO_REG(temp);
> +	ad7418_write(client, attr->index + 1, data->temp[attr->index]);

This is a hack, "attr->index + 1" is AD7418_REG_TEMP_HYST or
AD7418_REG_TEMP_OS. It works, but it's fragile.

I think that you want to define AD7418_REG_TEMP as a static const array
of three register values (0x00, 0x02 and 0x03). Then you can use
AD7418_REG_TEMP[attr->index] here, which is much cleaner. And you can
loop over that array in ad7418_update_device, too.

> +	mutex_unlock(&data->lock);
> +	return count;
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO, show_temp, set_temp, 1);

Line too long.

> +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp, set_temp, 2);
> +
> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, show_adc, NULL, 0);
> +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, show_adc, NULL, 1);
> +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, show_adc, NULL, 2);
> +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, show_adc, NULL, 3);
> +
> +static int ad7418_attach_adapter(struct i2c_adapter *adapter)
> +{
> +	if (!(adapter->class & I2C_CLASS_HWMON))
> +		return 0;
> +	return i2c_probe(adapter, &addr_data, ad7418_detect);
> +}
> +
> +static struct attribute *ad7416_attributes[] = {
> +	&sensor_dev_attr_temp1_max.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute *ad7417_attributes[] = {
> +	&sensor_dev_attr_temp1_max.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_in1_input.dev_attr.attr,
> +	&sensor_dev_attr_in2_input.dev_attr.attr,
> +	&sensor_dev_attr_in3_input.dev_attr.attr,
> +	&sensor_dev_attr_in4_input.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute *ad7418_attributes[] = {
> +	&sensor_dev_attr_temp1_max.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_in1_input.dev_attr.attr,
> +	NULL
> +};
> +
> +static int ad7418_detect(struct i2c_adapter *adapter, int address, int kind)
> +{
> +	struct i2c_client *client;
> +	struct ad7418_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 ad7418_data), GFP_KERNEL))) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	client = &data->client;
> +	client->addr = address;
> +	client->adapter = adapter;
> +	client->driver = &ad7418_driver;
> +
> +	i2c_set_clientdata(client, data);
> +
> +	mutex_init(&data->lock);
> +
> +	/* AD7418 has a curious behaviour on registers 6 and 7. They
> +	 * both always read 0xC071 and are not documented on the datasheet.
> +	 * We use them to detect the chip.
> +	 */
> +	if (kind <= 0) {
> +		int reg, reg6, reg7;
> +
> +		/* the AD7416 lies within this address range, but I have
> +		 * no means to check.
> +		 */
> +		if (address >= 0x48 && address <= 0x4f) {
> +			/* XXX add tests for AD7416 here */
> +			/* data->type = ad7416; */
> +		}

So for now the only way to get an ad7416 is using module parameters?
If so, there's no point in leaving the 0x48 - 0x4f address range in
normal_i2c.

> +		/* here we might have AD7417 or AD7418 */
> +		else if (address >= 0x28 && address <= 0x2f) {
> +			reg6 = i2c_smbus_read_word_data(client, 0x06);
> +			reg7 = i2c_smbus_read_word_data(client, 0x07);
> +
> +			if (address == 0x28 && reg6 == 0xC071 && reg7 == 0xC071)
> +				data->type = ad7418;
> +
> +			/* XXX add tests for AD7417 here */
> +
> +
> +			/* both AD7417 and AD7418 have bits 0-5 of
> +			 * the CONF2 register at 0
> +			 */
> +			reg = i2c_smbus_read_byte_data(client, AD7418_REG_CONF2);

Line too long.

> +			if (reg & 0x3F)
> +				data->type = any_chip; /* detection failed */
> +		}

Here too, the only way to get an ad7417 is using a module parameter?
Then you can leave the range 0x29 - 0x2f out of normal_i2c for now.

> +	} else {
> +		dev_dbg(&adapter->class_dev.dev, "detection forced\n");

adapter->class_dev.dev no longer exists, please use adapter->dev
instead.

> +	}
> +
> +	if (kind > 0)
> +		data->type = kind;
> +	else if (kind < 0 && data->type == any_chip) {
> +		err = -ENODEV;
> +		goto exit_free;
> +	}
> +
> +	switch (data->type) {
> +	case any_chip:
> +	case ad7416:
> +		data->attrs.attrs = ad7416_attributes;
> +		strlcpy(client->name, "ad7416", I2C_NAME_SIZE);
> +		break;
> +
> +	case ad7417:
> +		data->attrs.attrs = ad7417_attributes;
> +		strlcpy(client->name, "ad7417", I2C_NAME_SIZE);
> +		break;
> +
> +	case ad7418:
> +		data->attrs.attrs = ad7418_attributes;
> +		strlcpy(client->name, "ad7418", I2C_NAME_SIZE);
> +		break;
> +	}
> +
> +	if ((err = i2c_attach_client(client)))
> +		goto exit_free;
> +
> +	dev_info(&client->dev, "%s chip found\n", client->name);
> +
> +	/* Initialize the AD7418 chip */
> +	ad7418_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 ad7418_detach_client(struct i2c_client *client)
> +{
> +	struct ad7418_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 ad7418_init(void)
> +{
> +	return i2c_add_driver(&ad7418_driver);
> +}
> +
> +static void __exit ad7418_exit(void)
> +{
> +	i2c_del_driver(&ad7418_driver);
> +}
> +
> +MODULE_AUTHOR("Alessandro Zummo <a.zummo at towertech.it>");
> +MODULE_DESCRIPTION("AD7416/17/18 driver");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(DRV_VERSION);
> +
> +module_init(ad7418_init);
> +module_exit(ad7418_exit);
> --- linux-ixp4xx.orig/drivers/hwmon/Kconfig	2007-03-25 19:55:07.000000000 +0200
> +++ linux-ixp4xx/drivers/hwmon/Kconfig	2007-03-25 22:48:52.000000000 +0200
> @@ -574,6 +574,16 @@ config SENSORS_W83627EHF
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called w83627ehf.
>  
> +config SENSORS_AD7418
> +	tristate "Analog Devices AD7416/17/18"
> +	depends on HWMON && I2C && EXPERIMENTAL
> +	help
> +	  If you say yes here you get support for the Analog Devices
> +	  AD7416, AD7417 and AD7418 temperature monitoring chips.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called ad7418.
> +

Please add this entry in alphabetical order. I also tend to prefer
device names fully spelled out, so that they can be grep'd for, but
that's up to you.

>  config SENSORS_HDAPS
>  	tristate "IBM Hard Drive Active Protection System (hdaps)"
>  	depends on HWMON && INPUT && X86

Otherwise it looks OK. Please send an updated patch addressing the last
few problems, and I'll add it to my hwmon patch stack.

Can we have a user-space support patch for lm-sensors, too?

Thanks,
-- 
Jean Delvare