lkml.org 
[lkml]   [2005]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH: 2.6.11-rc5] i2c chips: add adt7461 support to lm90 driver
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:10    [W:0.042 / U:1.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site