[PATCH: 2.6.11-rc5] i2c chips: add adt7461 support to lm90 driver

[PATCH: 2.6.11-rc5] i2c chips: add adt7461 support to lm90 driver

[James Chapman]

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

Add ADT7461 (temperature sensor) support to LM90 driver.

Signed-off-by: James Chapman <jchapman@katalix.com>






[-- Attachment #2: lm90.patch --]
[-- Type: text/plain, Size: 847 bytes --]

diff -Nru a/drivers/i2c/chips/lm90.c b/drivers/i2c/chips/lm90.c
--- a/drivers/i2c/chips/lm90.c	2005-02-27 13:24:11 +00:00
+++ b/drivers/i2c/chips/lm90.c	2005-02-27 13:24:11 +00:00
@@ -85,7 +85,7 @@
  * Insmod parameters
  */
 
-SENSORS_INSMOD_5(lm90, adm1032, lm99, lm86, max6657);
+SENSORS_INSMOD_6(lm90, adm1032, lm99, lm86, max6657, adt7461);
 
 /*
  * The LM90 registers
@@ -386,7 +386,10 @@
 			 && (reg_config1 & 0x3F) == 0x00
 			 && reg_convrate <= 0x0A) {
 				kind = adm1032;
-			}
+			} else
+			if (address == 0x4c
+			 && chip_id == 0x51) /* ADT7461 */
+				kind = adt7461;
 		} else
 		if (man_id == 0x4D) { /* Maxim */
 			/*
@@ -423,6 +426,8 @@
 		name = "lm86";
 	} else if (kind == max6657) {
 		name = "max6657";
+	} else if (kind == adt7461) {
+		name = "adt7461";
 	}
 
 	/* We can fill in the remaining client fields */

Re: [PATCH: 2.6.11-rc5] i2c chips: add adt7461 support to lm90 driver

[Greg KH]

On Mon, Feb 28, 2005 at 05:13:35PM +0000, James Chapman wrote:
> Add ADT7461 (temperature sensor) support to LM90 driver.
> 
> Signed-off-by: James Chapman <jchapman@katalix.com>

Applied, thanks.

greg k-h

Re: [PATCH: 2.6.11-rc5] i2c chips: add adt7461 support to lm90 driver

[Jean Delvare]

Hi James,

> > Add ADT7461 (temperature sensor) support to LM90 driver.
> > 
> > Signed-off-by: James Chapman <jchapman@katalix.com>
> 
> Applied, thanks.

I had Greg drop this patch because it doesn't seem to be correct to me.
The patch assumes that the ADT7461 is 100% compatible with the ADM1032.
The datasheets mentions that, but only in the default confiuration of
the chip. If I read it correctly, the ADT7461 can also be switched to an
extended temperature range mode, where the register formats are
different. In this mode, the chip is NOT compatible with the ADM1032 and
needs specific handling.

I wonder how the limits are affected by the extended mode. The datasheet
doesn't give much details about this. I would assume that they are
affected too though, or it wouldn't make much sense.

I also see that the ADT7461 datasheet explicitely says that temperature
measurements below 0 return 0 and over 127 return 127, in compatibility
mode. Then we probably should not allow limits to be set outside of this
range, as it might fool the user into thinking that the device can
actually measure this. The ADM1032 datasheet isn't as clear on the
topic, so I'm not sure what should be done for this one.

Please modify your patch so that it works properly for both modes. I do
not request that you implement mode change, nor even that you support
extended range mode if you don't need it. But I want you to ensure that
the driver will behave properly on an ADT7461 found in this
non-compatible range. This might be as easy as to not recognize the chip
as supported if found in that mode, providing you don't care about the
extended range.

I would also like you do update the documentation located on top of the
lm90 driver source file. I listed all supported chips with links to
datasheet and some additional per-chip information. The ADT7461
definitely needs something similar. Then please update Kconfig to
mention the new chip.

I would also appreciate a patch to lm_sensors' sensors-detect script for
the ADT7461, and possibly a patch for libsensors and sensors as well,
unless you are not interested in these (in which case you would need to
explicitely mention in Kconfig that the ADT7461 has no user-space
support at the moment).

Thanks,
-- 
Jean Delvare

Re: [PATCH: 2.6.11-rc5] i2c chips: add adt7461 support to lm90 driver

[James Chapman]

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

A revised adt7461 patch addressing all of Jean's comments is
attached.

This driver will detect the adt7461 chip only if boot firmware
has left the chip in its default lm90-compatible mode.



[-- Attachment #2: adt7461.patch --]
[-- Type: text/plain, Size: 3648 bytes --]

i2c: add adt7461 chip support

Signed-off-by: James Chapman <jchapman@katalix.com>

The Analog Devices ADT7461 temperature sensor chip is compatible with
the lm90 device provided its extended temperature range is not
enabled.  The chip will be ignored if the boot firmware enables
extended temperature range.

Also, since the adt7461 treats temp values <0 as 0 and >127 as 127,
the driver prevents temperature values outside the supported range
from being set.

Index: linux-2.6.i2c/drivers/i2c/chips/lm90.c
===================================================================
--- linux-2.6.i2c.orig/drivers/i2c/chips/lm90.c	2005-03-03 15:02:40.000000000 +0000
+++ linux-2.6.i2c/drivers/i2c/chips/lm90.c	2005-03-03 15:44:34.000000000 +0000
@@ -43,6 +43,12 @@
  * variants. The extra address and features of the MAX6659 are not
  * supported by this driver.
  *
+ * This driver also supports the ADT7461 chip from Analog Devices but
+ * only in its "compatability mode". If an ADT7461 chip is found but
+ * is configured in non-compatible mode (where its temperature
+ * register values are decoded differently) it is ignored by this
+ * driver.
+ *
  * Since the LM90 was the first chipset supported by this driver, most
  * comments will refer to this chipset, but are actually general and
  * concern all supported chipsets, unless mentioned otherwise.
@@ -76,6 +82,7 @@
  * LM86, LM89, LM90, LM99, ADM1032, MAX6657 and MAX6658 have address 0x4c.
  * LM89-1, and LM99-1 have address 0x4d.
  * MAX6659 can have address 0x4c, 0x4d or 0x4e (unsupported).
+ * ADT7461 always has address 0x4c.
  */
 
 static unsigned short normal_i2c[] = { 0x4c, 0x4d, I2C_CLIENT_END };
@@ -85,7 +92,7 @@
  * Insmod parameters
  */
 
-SENSORS_INSMOD_5(lm90, adm1032, lm99, lm86, max6657);
+SENSORS_INSMOD_6(lm90, adm1032, lm99, lm86, max6657, adt7461);
 
 /*
  * The LM90 registers
@@ -180,6 +187,7 @@
 	struct semaphore update_lock;
 	char valid; /* zero until following fields are valid */
 	unsigned long last_updated; /* in jiffies */
+	int kind;
 
 	/* registers values */
 	s8 temp_input1, temp_low1, temp_high1; /* local */
@@ -221,6 +229,8 @@
 	struct i2c_client *client = to_i2c_client(dev); \
 	struct lm90_data *data = i2c_get_clientdata(client); \
 	long val = simple_strtol(buf, NULL, 10); \
+	if ((data->kind == adt7461) && ((val < 0) || (val > 127000))) \
+		return -EINVAL; \
 	data->value = TEMP1_TO_REG(val); \
 	i2c_smbus_write_byte_data(client, reg, data->value); \
 	return count; \
@@ -232,6 +242,8 @@
 	struct i2c_client *client = to_i2c_client(dev); \
 	struct lm90_data *data = i2c_get_clientdata(client); \
 	long val = simple_strtol(buf, NULL, 10); \
+	if ((data->kind == adt7461) && ((val < 0) || (val > 127000))) \
+		return -EINVAL; \
 	data->value = TEMP2_TO_REG(val); \
 	i2c_smbus_write_byte_data(client, regh, data->value >> 8); \
 	i2c_smbus_write_byte_data(client, regl, data->value & 0xff); \
@@ -386,6 +398,12 @@
 			 && (reg_config1 & 0x3F) == 0x00
 			 && reg_convrate <= 0x0A) {
 				kind = adm1032;
+			} else
+			if (address == 0x4c
+			 && chip_id == 0x51 /* ADT7461 */
+			 && (reg_config1 & 0x3F) == 0x00
+			 && reg_convrate <= 0x0A) {
+				kind = adt7461;
 			}
 		} else
 		if (man_id == 0x4D) { /* Maxim */
@@ -423,12 +441,15 @@
 		name = "lm86";
 	} else if (kind == max6657) {
 		name = "max6657";
+	} else if (kind == adt7461) {
+		name = "adt7461";
 	}
 
 	/* We can fill in the remaining client fields */
 	strlcpy(new_client->name, name, I2C_NAME_SIZE);
 	new_client->id = lm90_id++;
 	data->valid = 0;
+	data->kind = kind;
 	init_MUTEX(&data->update_lock);
 
 	/* Tell the I2C layer a new client has arrived */

Re: [PATCH: 2.6.11-rc5] i2c chips: add adt7461 support to lm90 driver

[Jean Delvare]

Hi James,

> A revised adt7461 patch addressing all of Jean's comments is
> attached.
> 
> This driver will detect the adt7461 chip only if boot firmware
> has left the chip in its default lm90-compatible mode.

I'm fine with the idea but not quite with your implementation:

> @@ -221,6 +229,8 @@
>  	struct i2c_client *client = to_i2c_client(dev); \
>  	struct lm90_data *data = i2c_get_clientdata(client); \
>  	long val = simple_strtol(buf, NULL, 10); \
> +	if ((data->kind == adt7461) && ((val < 0) || (val > 127000))) \
> +		return -EINVAL; \
>  	data->value = TEMP1_TO_REG(val); \
>  	i2c_smbus_write_byte_data(client, reg, data->value); \
>  	return count; \
> @@ -232,6 +242,8 @@
>  	struct i2c_client *client = to_i2c_client(dev); \
>  	struct lm90_data *data = i2c_get_clientdata(client); \
>  	long val = simple_strtol(buf, NULL, 10); \
> +	if ((data->kind == adt7461) && ((val < 0) || (val > 127000))) \
> +		return -EINVAL; \
>  	data->value = TEMP2_TO_REG(val); \
>  	i2c_smbus_write_byte_data(client, regh, data->value >> 8); \
>  	i2c_smbus_write_byte_data(client, regl, data->value & 0xff); \

This is inconsistent with the rest of the interface. For continuous
values, we do not return errors on out-of-range writes. Instead, we
force the value to whatever range boundary makes sense. See TEMP1_TO_REG
and TEMP2_TO_REG. So I would suggest that you implement
TEMP1_TO_REG_ADT7461 and TEMP2_TO_REG_ADT7461 the same way with
different boundaries, and call them.

> +			if (address == 0x4c
> +			 && chip_id == 0x51 /* ADT7461 */
> +			 && (reg_config1 & 0x3F) == 0x00

That could be broaden to "& 0x1F" is I am not mistaken. We don't really
care about bit 5, do we? And maybe put a comment explaining that we
check for compatibility at this point (similar checks for the other
chips are only for unused bits, not for specific configuration).

Thanks,
-- 
Jean Delvare