* [PATCH] hwmon: Add support for max6695 and max6696 to lm90 driver
@ 2010-09-04 22:34 Guenter Roeck
2010-09-06 16:12 ` Jean Delvare
0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2010-09-04 22:34 UTC (permalink / raw)
To: Jean Delvare; +Cc: Andrew Morton, lm-sensors, linux-kernel, Guenter Roeck
Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
To apply this patch, the previously submitted lm90 cleanup patch has to be
applied first.
My main concern with this patch is the chip detection code, specifically if it
is able to safely distinguish between MAX6680/81 and MAX6695/96.
Would be great to get some test coverage from a system with one of those chips.
Sample sensors output:
max6695-i2c-0-19
Adapter: SMBus I801 adapter at 5080
temp1: +24.5 C (low = -55.0 C, high = +70.0 C)
(crit = +70.0 C, hyst = +60.0 C)
temp2: +26.5 C (low = -55.0 C, high = +70.0 C)
(crit = +90.0 C, hyst = +80.0 C)
temp3: +24.1 C (low = -54.1 C, high = +70.2 C)
(crit = +90.0 C, hyst = +80.0 C)
drivers/hwmon/lm90.c | 280 +++++++++++++++++++++++++++++++++++++++++++++-----
1 files changed, 252 insertions(+), 28 deletions(-)
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index aafed28..52ed792 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -42,6 +42,11 @@
* chips. The MAX6680 and MAX6681 only differ in the pinout so they can
* be treated identically.
*
+ * This driver also supports the MAX6695 and MAX6696, two other sensor
+ * chips made by Maxim. These are also quite similar to other Maxim
+ * chips, but support three temperature sensors instead of two. MAX6695
+ * and MAX6696 only differ in the pinout so they can be treated identically.
+ *
* This driver also supports the ADT7461 chip from Analog Devices.
* It's supported in both compatibility and extended mode. It is mostly
* compatible with LM90 except for a data format difference for the
@@ -94,7 +99,7 @@ static const unsigned short normal_i2c[] = {
0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
- w83l771 };
+ max6695, w83l771 };
/*
* The LM90 registers
@@ -109,6 +114,7 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
#define LM90_REG_R_CONVRATE 0x04
#define LM90_REG_W_CONVRATE 0x0A
#define LM90_REG_R_STATUS 0x02
+#define LM90_REG_R_STATUS2 0x12
#define LM90_REG_R_LOCAL_TEMP 0x00
#define LM90_REG_R_LOCAL_HIGH 0x05
#define LM90_REG_W_LOCAL_HIGH 0x0B
@@ -130,12 +136,16 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
#define LM90_REG_W_REMOTE_LOWH 0x0E
#define LM90_REG_R_REMOTE_LOWL 0x14
#define LM90_REG_W_REMOTE_LOWL 0x14
+#define LM90_REG_R_REMOTE_EMERG 0x16
+#define LM90_REG_W_REMOTE_EMERG 0x16
+#define LM90_REG_R_LOCAL_EMERG 0x17
+#define LM90_REG_W_LOCAL_EMERG 0x17
#define LM90_REG_R_REMOTE_CRIT 0x19
#define LM90_REG_W_REMOTE_CRIT 0x19
#define LM90_REG_R_TCRIT_HYST 0x21
#define LM90_REG_W_TCRIT_HYST 0x21
-/* MAX6646/6647/6649/6657/6658/6659 registers */
+/* MAX6646/6647/6649/6657/6658/6659/6695/6696 registers */
#define MAX6657_REG_R_LOCAL_TEMPL 0x11
@@ -148,6 +158,7 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
* Functions declaration
*/
+static int lm90_read_reg(struct i2c_client *client, u8 reg, u8 *value);
static int lm90_detect(struct i2c_client *client, struct i2c_board_info *info);
static int lm90_probe(struct i2c_client *client,
const struct i2c_device_id *id);
@@ -157,6 +168,23 @@ static int lm90_remove(struct i2c_client *client);
static struct lm90_data *lm90_update_device(struct device *dev);
/*
+ * Some useful macros
+ */
+#define lm90_have_offset(data) \
+ (data->kind != max6657 && data->kind != max6646 \
+ && data->kind != max6695)
+
+#define lm90_have_local_temp_ext(data) \
+ (data->kind == max6657 || data->kind == max6646 \
+ || data->kind == max6695)
+
+#define lm90_have_remote_limit_ext(data) \
+ (data->kind != max6657 && data->kind != max6646 \
+ && data->kind != max6680 && data->kind != max6695)
+
+#define lm90_have_emergency(data) (data->kind == max6695)
+
+/*
* Driver data (common to all clients)
*/
@@ -175,6 +203,8 @@ static const struct i2c_device_id lm90_id[] = {
{ "max6659", max6657 },
{ "max6680", max6680 },
{ "max6681", max6680 },
+ { "max6695", max6695 },
+ { "max6696", max6695 },
{ "w83l771", w83l771 },
{ }
};
@@ -206,20 +236,29 @@ struct lm90_data {
int flags;
u8 config_orig; /* Original configuration register value */
- u8 alert_alarms; /* Which alarm bits trigger ALERT# */
+ u16 alert_alarms; /* Which alarm bits trigger ALERT# */
+ /* Upper 8 bits from max6695 STATUS2 register */
/* registers values */
- s8 temp8[4]; /* 0: local low limit
+ s8 temp8[8]; /* 0: local low limit
1: local high limit
2: local critical limit
- 3: remote critical limit */
- s16 temp11[5]; /* 0: remote input
+ 3: remote critical limit
+ 4: local emergency limit (max6695/96 only)
+ 5: remote emergency limit (max6695/96 only)
+ 6: remote 2 critical limit (max6695/96 only)
+ 7: remote 2 emergency limit (max6695/96 only) */
+ s16 temp11[8]; /* 0: remote input
1: remote low limit
2: remote high limit
- 3: remote offset (except max6646 and max6657)
- 4: local input */
+ 3: remote offset (except max6646, max6657/59,
+ and max6695/96)
+ 4: local input
+ 5: remote 2 input (max6695/96 only)
+ 6: remote 2 low limit (max6695/96 only)
+ 7: remote 2 high limit (ma6695/96 only) */
u8 temp_hyst;
- u8 alarms; /* bitvector */
+ u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
};
/*
@@ -377,11 +416,15 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
const char *buf, size_t count)
{
- static const u8 reg[4] = {
+ static const u8 reg[8] = {
LM90_REG_W_LOCAL_LOW,
LM90_REG_W_LOCAL_HIGH,
LM90_REG_W_LOCAL_CRIT,
LM90_REG_W_REMOTE_CRIT,
+ LM90_REG_W_LOCAL_EMERG,
+ LM90_REG_W_REMOTE_EMERG,
+ LM90_REG_W_REMOTE_CRIT,
+ LM90_REG_W_REMOTE_EMERG,
};
struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
@@ -390,6 +433,7 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
int nr = attr->index;
long val;
int err;
+ u8 config;
err = strict_strtol(buf, 10, &val);
if (err < 0)
@@ -406,7 +450,18 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
data->temp8[nr] = temp_to_u8(val);
else
data->temp8[nr] = temp_to_s8(val);
+
+ if (data->kind == max6695 && nr >= 6) {
+ lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
+ i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
+ config | 0x08);
+ }
+
i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
+
+ if (data->kind == max6695 && nr >= 6)
+ i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
+
mutex_unlock(&data->update_lock);
return count;
}
@@ -450,6 +505,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
int nr = attr->index;
long val;
int err;
+ int offset = 1;
+ u8 config;
err = strict_strtol(buf, 10, &val);
if (err < 0)
@@ -462,19 +519,31 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
mutex_lock(&data->update_lock);
if (data->kind == adt7461)
data->temp11[nr] = temp_to_u16_adt7461(data, val);
- else if (data->kind == max6657 || data->kind == max6680)
- data->temp11[nr] = temp_to_s8(val) << 8;
else if (data->kind == max6646)
data->temp11[nr] = temp_to_u8(val) << 8;
+ else if (!lm90_have_remote_limit_ext(data))
+ data->temp11[nr] = temp_to_s8(val) << 8;
else
data->temp11[nr] = temp_to_s16(val);
- i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2],
+ if (data->kind == max6695 && nr >= 6) {
+ /* select output channel 2 */
+ lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
+ i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
+ config | 0x08);
+ offset = 6;
+ }
+
+ i2c_smbus_write_byte_data(client, reg[(nr - offset) * 2],
data->temp11[nr] >> 8);
- if (data->kind != max6657 && data->kind != max6680
- && data->kind != max6646)
- i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2 + 1],
+ if (lm90_have_remote_limit_ext(data))
+ i2c_smbus_write_byte_data(client, reg[(nr - offset) * 2 + 1],
data->temp11[nr] & 0xff);
+
+ if (data->kind == max6695 && nr >= 6)
+ i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
+ config);
+
mutex_unlock(&data->update_lock);
return count;
}
@@ -604,6 +673,62 @@ static const struct attribute_group lm90_group = {
.attrs = lm90_attributes,
};
+/*
+ * Additional attributes for devices with emergency sensors
+ */
+static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
+ set_temp8, 4);
+static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
+ set_temp8, 5);
+
+/*
+ * Additional attributes for devices with 3 temperature sensors
+ */
+static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_temp11, NULL, 5);
+static SENSOR_DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
+ set_temp11, 6);
+static SENSOR_DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
+ set_temp11, 7);
+static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8,
+ set_temp8, 6);
+static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 4);
+static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
+ set_temp8, 7);
+
+static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO, show_alarm, NULL, 9);
+static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_alarm, NULL, 10);
+static SENSOR_DEVICE_ATTR(temp3_min_alarm, S_IRUGO, show_alarm, NULL, 11);
+static SENSOR_DEVICE_ATTR(temp3_max_alarm, S_IRUGO, show_alarm, NULL, 12);
+
+static struct attribute *lm90_emergency_attributes[] = {
+ &sensor_dev_attr_temp1_emergency.dev_attr.attr,
+ &sensor_dev_attr_temp2_emergency.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group lm90_emergency_group = {
+ .attrs = lm90_emergency_attributes,
+};
+
+static struct attribute *lm90_temp3_attributes[] = {
+ &sensor_dev_attr_temp3_input.dev_attr.attr,
+ &sensor_dev_attr_temp3_min.dev_attr.attr,
+ &sensor_dev_attr_temp3_max.dev_attr.attr,
+ &sensor_dev_attr_temp3_crit.dev_attr.attr,
+ &sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
+ &sensor_dev_attr_temp3_emergency.dev_attr.attr,
+
+ &sensor_dev_attr_temp3_crit_alarm.dev_attr.attr,
+ &sensor_dev_attr_temp3_fault.dev_attr.attr,
+ &sensor_dev_attr_temp3_min_alarm.dev_attr.attr,
+ &sensor_dev_attr_temp3_max_alarm.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group lm90_temp3_group = {
+ .attrs = lm90_temp3_attributes,
+};
+
/* pec used for ADM1032 only */
static ssize_t show_pec(struct device *dev, struct device_attribute *dummy,
char *buf)
@@ -688,7 +813,7 @@ static int lm90_detect(struct i2c_client *new_client,
struct i2c_adapter *adapter = new_client->adapter;
int address = new_client->addr;
const char *name = NULL;
- int man_id, chip_id, reg_config1, reg_convrate;
+ int man_id, chip_id, reg_config1, reg_convrate, reg_emerg;
if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
return -ENODEV;
@@ -704,6 +829,11 @@ static int lm90_detect(struct i2c_client *new_client,
LM90_REG_R_CONVRATE)) < 0)
return -ENODEV;
+ reg_emerg = i2c_smbus_read_byte_data(new_client,
+ LM90_REG_R_REMOTE_EMERG);
+ if (reg_emerg < 0)
+ return -ENODEV;
+
if ((address == 0x4C || address == 0x4D)
&& man_id == 0x01) { /* National Semiconductor */
int reg_config2;
@@ -770,6 +900,22 @@ static int lm90_detect(struct i2c_client *new_client,
name = "max6657";
} else
/*
+ * Even though MAX6695 and MAX6696 do not have a chip ID
+ * register, reading it returns 0x01. Bit 4 of the config1
+ * register is unused and should return zero when read.
+ *
+ * MAX6695 and MAX6696 have an additional set of temperature
+ * limit registers. We can detect those chips by checking if
+ * one of those registers exists (and thus returns a value
+ * different to the previous reading).
+ */
+ if (chip_id == 0x01
+ && (reg_config1 & 0x10) == 0x00
+ && reg_emerg != reg_convrate
+ && reg_convrate <= 0x07) {
+ name = "max6695";
+ } else
+ /*
* The chip_id register of the MAX6680 and MAX6681 holds the
* revision of the chip. The lowest bit of the config1 register
* is unused and should return zero when read, so should the
@@ -842,6 +988,9 @@ static int lm90_probe(struct i2c_client *new_client,
case lm86:
data->alert_alarms = 0x7b;
break;
+ case max6695:
+ data->alert_alarms = (0x18 << 8) | 0x7c;
+ break;
default:
data->alert_alarms = 0x7c;
break;
@@ -854,12 +1003,27 @@ static int lm90_probe(struct i2c_client *new_client,
err = sysfs_create_group(&new_client->dev.kobj, &lm90_group);
if (err)
goto exit_free;
+
+ if (lm90_have_emergency(data)) {
+ err = sysfs_create_group(&new_client->dev.kobj,
+ &lm90_emergency_group);
+ if (err)
+ goto exit_remove_base;
+ }
+
+ if (data->kind == max6695) {
+ err = sysfs_create_group(&new_client->dev.kobj,
+ &lm90_temp3_group);
+ if (err)
+ goto exit_remove_emergency;
+ }
+
if (new_client->flags & I2C_CLIENT_PEC) {
err = device_create_file(&new_client->dev, &dev_attr_pec);
if (err)
goto exit_remove_files;
}
- if (data->kind != max6657 && data->kind != max6646) {
+ if (lm90_have_offset(data)) {
err = device_create_file(&new_client->dev,
&sensor_dev_attr_temp2_offset.dev_attr);
if (err)
@@ -875,6 +1039,13 @@ static int lm90_probe(struct i2c_client *new_client,
return 0;
exit_remove_files:
+ if (data->kind == max6695)
+ sysfs_remove_group(&new_client->dev.kobj, &lm90_temp3_group);
+exit_remove_emergency:
+ if (lm90_have_emergency(data))
+ sysfs_remove_group(&new_client->dev.kobj,
+ &lm90_emergency_group);
+exit_remove_base:
sysfs_remove_group(&new_client->dev.kobj, &lm90_group);
device_remove_file(&new_client->dev, &dev_attr_pec);
exit_free:
@@ -913,6 +1084,12 @@ static void lm90_init_client(struct i2c_client *client)
if (data->kind == max6680)
config |= 0x18;
+ /*
+ * Select external channel 1 for max6695
+ */
+ if (data->kind == max6695)
+ config &= ~0x08;
+
config &= 0xBF; /* run */
if (config != data->config_orig) /* Only write if changed */
i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
@@ -923,9 +1100,13 @@ static int lm90_remove(struct i2c_client *client)
struct lm90_data *data = i2c_get_clientdata(client);
hwmon_device_unregister(data->hwmon_dev);
+ if (lm90_have_emergency(data))
+ sysfs_remove_group(&client->dev.kobj, &lm90_emergency_group);
+ if (data->kind == max6695)
+ sysfs_remove_group(&client->dev.kobj, &lm90_temp3_group);
sysfs_remove_group(&client->dev.kobj, &lm90_group);
device_remove_file(&client->dev, &dev_attr_pec);
- if (data->kind != max6657 && data->kind != max6646)
+ if (lm90_have_offset(data))
device_remove_file(&client->dev,
&sensor_dev_attr_temp2_offset.dev_attr);
@@ -940,10 +1121,14 @@ static int lm90_remove(struct i2c_client *client)
static void lm90_alert(struct i2c_client *client, unsigned int flag)
{
struct lm90_data *data = i2c_get_clientdata(client);
- u8 config, alarms;
+ u8 config, alarms, alarms2 = 0;
lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
- if ((alarms & 0x7f) == 0) {
+
+ if (data->kind == max6695)
+ lm90_read_reg(client, LM90_REG_R_STATUS2, &alarms2);
+
+ if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) {
dev_info(&client->dev, "Everything OK\n");
} else {
if (alarms & 0x61)
@@ -956,6 +1141,10 @@ static void lm90_alert(struct i2c_client *client, unsigned int flag)
dev_warn(&client->dev,
"temp%d diode open, please check!\n", 2);
+ if (alarms2 & 0x18)
+ dev_warn(&client->dev,
+ "temp%d out of range, please check!\n", 3);
+
/* Disable ALERT# output, because these chips don't implement
SMBus alert correctly; they should only hold the alert line
low briefly. */
@@ -1011,6 +1200,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
if (time_after(jiffies, data->last_updated + HZ / 2 + HZ / 10)
|| !data->valid) {
u8 h, l;
+ u8 alarms, alarms2 = 0;
dev_dbg(&client->dev, "Updating lm90 data.\n");
lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]);
@@ -1019,7 +1209,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]);
lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst);
- if (data->kind == max6657 || data->kind == max6646) {
+ if (lm90_have_local_temp_ext(data)) {
lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
MAX6657_REG_R_LOCAL_TEMPL,
&data->temp11[4]);
@@ -1033,29 +1223,63 @@ static struct lm90_data *lm90_update_device(struct device *dev)
if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) {
data->temp11[1] = h << 8;
- if (data->kind != max6657 && data->kind != max6680
- && data->kind != max6646
+ if (lm90_have_remote_limit_ext(data)
&& lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL,
&l) == 0)
data->temp11[1] |= l;
}
if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) {
data->temp11[2] = h << 8;
- if (data->kind != max6657 && data->kind != max6680
- && data->kind != max6646
+ if (lm90_have_remote_limit_ext(data)
&& lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL,
&l) == 0)
data->temp11[2] |= l;
}
- if (data->kind != max6657 && data->kind != max6646) {
+ if (lm90_have_offset(data)) {
if (lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSH,
&h) == 0
&& lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL,
&l) == 0)
data->temp11[3] = (h << 8) | l;
}
- lm90_read_reg(client, LM90_REG_R_STATUS, &data->alarms);
+
+ if (data->kind == max6695) {
+ u8 config;
+
+ lm90_read_reg(client, LM90_REG_R_LOCAL_EMERG,
+ &data->temp8[4]);
+ lm90_read_reg(client, LM90_REG_R_REMOTE_EMERG,
+ &data->temp8[5]);
+
+ lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
+ i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
+ config | 0x08);
+
+ lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
+ &data->temp8[6]);
+ lm90_read_reg(client, LM90_REG_R_REMOTE_EMERG,
+ &data->temp8[7]);
+ lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
+ LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]);
+ if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h)
+ && !lm90_read_reg(client,
+ LM90_REG_R_REMOTE_LOWL, &l))
+ data->temp11[6] = (h << 8) | l;
+ if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h)
+ && !lm90_read_reg(client,
+ LM90_REG_R_REMOTE_HIGHL, &l))
+ data->temp11[7] = (h << 8) | l;
+
+ i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
+ config);
+
+ lm90_read_reg(client, LM90_REG_R_STATUS2,
+ &alarms2);
+ }
+
+ lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
+ data->alarms = (alarms2 << 8) | alarms;
/* Re-enable ALERT# output if it was originally enabled and
* relevant alarms are all clear */
--
1.7.0.87.g0901d
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: Add support for max6695 and max6696 to lm90 driver
2010-09-04 22:34 [PATCH] hwmon: Add support for max6695 and max6696 to lm90 driver Guenter Roeck
@ 2010-09-06 16:12 ` Jean Delvare
2010-09-07 2:00 ` Guenter Roeck
2010-09-08 10:28 ` Guenter Roeck
0 siblings, 2 replies; 10+ messages in thread
From: Jean Delvare @ 2010-09-06 16:12 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Andrew Morton, lm-sensors, linux-kernel, Guenter Roeck
Hi Guenter,
On Sat, 4 Sep 2010 15:34:35 -0700, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
> To apply this patch, the previously submitted lm90 cleanup patch has to be
> applied first.
>
> My main concern with this patch is the chip detection code, specifically if it
> is able to safely distinguish between MAX6680/81 and MAX6695/96.
> Would be great to get some test coverage from a system with one of those chips.
Unfortunately I don't have any of these Maxim chips at hand. I have an
ADM1032 but it won't offer much coverage obviously. And I have dumps of
Maxim chips, but the real chips behave differently, so it's of little
help.
Can you please add detection support to sensors-detect as well (and
then update wiki/Devices)?
Review below:
>
> Sample sensors output:
>
> max6695-i2c-0-19
> Adapter: SMBus I801 adapter at 5080
> temp1: +24.5 C (low = -55.0 C, high = +70.0 C)
> (crit = +70.0 C, hyst = +60.0 C)
> temp2: +26.5 C (low = -55.0 C, high = +70.0 C)
> (crit = +90.0 C, hyst = +80.0 C)
> temp3: +24.1 C (low = -54.1 C, high = +70.2 C)
> (crit = +90.0 C, hyst = +80.0 C)
>
> drivers/hwmon/lm90.c | 280 +++++++++++++++++++++++++++++++++++++++++++++-----
> 1 files changed, 252 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index aafed28..52ed792 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -42,6 +42,11 @@
> * chips. The MAX6680 and MAX6681 only differ in the pinout so they can
> * be treated identically.
> *
> + * This driver also supports the MAX6695 and MAX6696, two other sensor
> + * chips made by Maxim. These are also quite similar to other Maxim
> + * chips, but support three temperature sensors instead of two. MAX6695
> + * and MAX6696 only differ in the pinout so they can be treated identically.
> + *
Please also update Documentation/hwmon/lm90 and drivers/hwmon/Kconfig.
You could also mention the additional emergency temperature limits, as
this is a feature unique to these chips.
> * This driver also supports the ADT7461 chip from Analog Devices.
> * It's supported in both compatibility and extended mode. It is mostly
> * compatible with LM90 except for a data format difference for the
> @@ -94,7 +99,7 @@ static const unsigned short normal_i2c[] = {
> 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
>
> enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> - w83l771 };
> + max6695, w83l771 };
>
> /*
> * The LM90 registers
> @@ -109,6 +114,7 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> #define LM90_REG_R_CONVRATE 0x04
> #define LM90_REG_W_CONVRATE 0x0A
> #define LM90_REG_R_STATUS 0x02
> +#define LM90_REG_R_STATUS2 0x12
> #define LM90_REG_R_LOCAL_TEMP 0x00
> #define LM90_REG_R_LOCAL_HIGH 0x05
> #define LM90_REG_W_LOCAL_HIGH 0x0B
> @@ -130,12 +136,16 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> #define LM90_REG_W_REMOTE_LOWH 0x0E
> #define LM90_REG_R_REMOTE_LOWL 0x14
> #define LM90_REG_W_REMOTE_LOWL 0x14
> +#define LM90_REG_R_REMOTE_EMERG 0x16
> +#define LM90_REG_W_REMOTE_EMERG 0x16
> +#define LM90_REG_R_LOCAL_EMERG 0x17
> +#define LM90_REG_W_LOCAL_EMERG 0x17
> #define LM90_REG_R_REMOTE_CRIT 0x19
> #define LM90_REG_W_REMOTE_CRIT 0x19
> #define LM90_REG_R_TCRIT_HYST 0x21
> #define LM90_REG_W_TCRIT_HYST 0x21
>
> -/* MAX6646/6647/6649/6657/6658/6659 registers */
> +/* MAX6646/6647/6649/6657/6658/6659/6695/6696 registers */
>
> #define MAX6657_REG_R_LOCAL_TEMPL 0x11
>
> @@ -148,6 +158,7 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> * Functions declaration
> */
>
> +static int lm90_read_reg(struct i2c_client *client, u8 reg, u8 *value);
> static int lm90_detect(struct i2c_client *client, struct i2c_board_info *info);
> static int lm90_probe(struct i2c_client *client,
> const struct i2c_device_id *id);
> @@ -157,6 +168,23 @@ static int lm90_remove(struct i2c_client *client);
> static struct lm90_data *lm90_update_device(struct device *dev);
>
> /*
> + * Some useful macros
> + */
> +#define lm90_have_offset(data) \
> + (data->kind != max6657 && data->kind != max6646 \
> + && data->kind != max6695)
> +
> +#define lm90_have_local_temp_ext(data) \
> + (data->kind == max6657 || data->kind == max6646 \
> + || data->kind == max6695)
> +
> +#define lm90_have_remote_limit_ext(data) \
> + (data->kind != max6657 && data->kind != max6646 \
> + && data->kind != max6680 && data->kind != max6695)
> +
> +#define lm90_have_emergency(data) (data->kind == max6695)
Makes the code more readable, I agree, but OTOH it hides complexity.
Such tests are OK during probe or remove, as they happen only once, but
seeing them in runtime code and in particular in the update function,
seems wrong (even though I can't disagree that the overhead is quite
small compared to the cost of SMBus transactions.)
I am wondering if it wouldn't be better to use data->flag to carry such
feature information, which would be computed at probe time, once and
for all. What do you think?
Also, these macros could have been introduced in a separate patch, to
make this one smaller, as they are good to have even without the
max6695/96 support.
> +
> +/*
> * Driver data (common to all clients)
> */
>
> @@ -175,6 +203,8 @@ static const struct i2c_device_id lm90_id[] = {
> { "max6659", max6657 },
> { "max6680", max6680 },
> { "max6681", max6680 },
> + { "max6695", max6695 },
> + { "max6696", max6695 },
> { "w83l771", w83l771 },
> { }
> };
> @@ -206,20 +236,29 @@ struct lm90_data {
> int flags;
>
> u8 config_orig; /* Original configuration register value */
> - u8 alert_alarms; /* Which alarm bits trigger ALERT# */
> + u16 alert_alarms; /* Which alarm bits trigger ALERT# */
> + /* Upper 8 bits from max6695 STATUS2 register */
The comment isn't quite correct. The contents of the STATUS2 register
go to struct member alarms below, not alert_alarms. alert_alarms is set
by the driver at initialization time.
>
> /* registers values */
> - s8 temp8[4]; /* 0: local low limit
> + s8 temp8[8]; /* 0: local low limit
> 1: local high limit
> 2: local critical limit
> - 3: remote critical limit */
> - s16 temp11[5]; /* 0: remote input
> + 3: remote critical limit
> + 4: local emergency limit (max6695/96 only)
> + 5: remote emergency limit (max6695/96 only)
> + 6: remote 2 critical limit (max6695/96 only)
> + 7: remote 2 emergency limit (max6695/96 only) */
> + s16 temp11[8]; /* 0: remote input
> 1: remote low limit
> 2: remote high limit
> - 3: remote offset (except max6646 and max6657)
> - 4: local input */
> + 3: remote offset (except max6646, max6657/59,
And 58 too, no?
> + and max6695/96)
> + 4: local input
> + 5: remote 2 input (max6695/96 only)
> + 6: remote 2 low limit (max6695/96 only)
> + 7: remote 2 high limit (ma6695/96 only) */
> u8 temp_hyst;
> - u8 alarms; /* bitvector */
> + u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
> };
>
> /*
> @@ -377,11 +416,15 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
> static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> const char *buf, size_t count)
> {
> - static const u8 reg[4] = {
> + static const u8 reg[8] = {
> LM90_REG_W_LOCAL_LOW,
> LM90_REG_W_LOCAL_HIGH,
> LM90_REG_W_LOCAL_CRIT,
> LM90_REG_W_REMOTE_CRIT,
> + LM90_REG_W_LOCAL_EMERG,
> + LM90_REG_W_REMOTE_EMERG,
> + LM90_REG_W_REMOTE_CRIT,
> + LM90_REG_W_REMOTE_EMERG,
> };
>
> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> @@ -390,6 +433,7 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> int nr = attr->index;
> long val;
> int err;
> + u8 config;
>
> err = strict_strtol(buf, 10, &val);
> if (err < 0)
> @@ -406,7 +450,18 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> data->temp8[nr] = temp_to_u8(val);
> else
> data->temp8[nr] = temp_to_s8(val);
> +
> + if (data->kind == max6695 && nr >= 6) {
> + lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
> + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> + config | 0x08);
> + }
> +
> i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
> +
> + if (data->kind == max6695 && nr >= 6)
> + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
> +
> mutex_unlock(&data->update_lock);
> return count;
> }
> @@ -450,6 +505,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> int nr = attr->index;
> long val;
> int err;
> + int offset = 1;
> + u8 config;
>
> err = strict_strtol(buf, 10, &val);
> if (err < 0)
> @@ -462,19 +519,31 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> mutex_lock(&data->update_lock);
> if (data->kind == adt7461)
> data->temp11[nr] = temp_to_u16_adt7461(data, val);
> - else if (data->kind == max6657 || data->kind == max6680)
> - data->temp11[nr] = temp_to_s8(val) << 8;
> else if (data->kind == max6646)
> data->temp11[nr] = temp_to_u8(val) << 8;
> + else if (!lm90_have_remote_limit_ext(data))
> + data->temp11[nr] = temp_to_s8(val) << 8;
> else
> data->temp11[nr] = temp_to_s16(val);
>
> - i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2],
> + if (data->kind == max6695 && nr >= 6) {
> + /* select output channel 2 */
> + lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
> + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> + config | 0x08);
> + offset = 6;
> + }
> +
> + i2c_smbus_write_byte_data(client, reg[(nr - offset) * 2],
> data->temp11[nr] >> 8);
This all gets a little tricky... Maybe it is time to rethink the whole
thing.
> - if (data->kind != max6657 && data->kind != max6680
> - && data->kind != max6646)
> - i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2 + 1],
> + if (lm90_have_remote_limit_ext(data))
> + i2c_smbus_write_byte_data(client, reg[(nr - offset) * 2 + 1],
> data->temp11[nr] & 0xff);
> +
> + if (data->kind == max6695 && nr >= 6)
> + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> + config);
> +
> mutex_unlock(&data->update_lock);
> return count;
> }
> @@ -604,6 +673,62 @@ static const struct attribute_group lm90_group = {
> .attrs = lm90_attributes,
> };
>
> +/*
> + * Additional attributes for devices with emergency sensors
> + */
> +static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
> + set_temp8, 4);
> +static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
> + set_temp8, 5);
> +
> +/*
> + * Additional attributes for devices with 3 temperature sensors
> + */
> +static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_temp11, NULL, 5);
> +static SENSOR_DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
> + set_temp11, 6);
> +static SENSOR_DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
> + set_temp11, 7);
> +static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8,
> + set_temp8, 6);
> +static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 4);
> +static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
> + set_temp8, 7);
> +
> +static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO, show_alarm, NULL, 9);
> +static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_alarm, NULL, 10);
> +static SENSOR_DEVICE_ATTR(temp3_min_alarm, S_IRUGO, show_alarm, NULL, 11);
> +static SENSOR_DEVICE_ATTR(temp3_max_alarm, S_IRUGO, show_alarm, NULL, 12);
No alarms files for emergency limits?
> +
> +static struct attribute *lm90_emergency_attributes[] = {
> + &sensor_dev_attr_temp1_emergency.dev_attr.attr,
> + &sensor_dev_attr_temp2_emergency.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group lm90_emergency_group = {
> + .attrs = lm90_emergency_attributes,
> +};
> +
> +static struct attribute *lm90_temp3_attributes[] = {
> + &sensor_dev_attr_temp3_input.dev_attr.attr,
> + &sensor_dev_attr_temp3_min.dev_attr.attr,
> + &sensor_dev_attr_temp3_max.dev_attr.attr,
> + &sensor_dev_attr_temp3_crit.dev_attr.attr,
> + &sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
> + &sensor_dev_attr_temp3_emergency.dev_attr.attr,
> +
> + &sensor_dev_attr_temp3_crit_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp3_fault.dev_attr.attr,
> + &sensor_dev_attr_temp3_min_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp3_max_alarm.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group lm90_temp3_group = {
> + .attrs = lm90_temp3_attributes,
> +};
> +
> /* pec used for ADM1032 only */
> static ssize_t show_pec(struct device *dev, struct device_attribute *dummy,
> char *buf)
> @@ -688,7 +813,7 @@ static int lm90_detect(struct i2c_client *new_client,
> struct i2c_adapter *adapter = new_client->adapter;
> int address = new_client->addr;
> const char *name = NULL;
> - int man_id, chip_id, reg_config1, reg_convrate;
> + int man_id, chip_id, reg_config1, reg_convrate, reg_emerg;
>
> if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> return -ENODEV;
> @@ -704,6 +829,11 @@ static int lm90_detect(struct i2c_client *new_client,
> LM90_REG_R_CONVRATE)) < 0)
> return -ENODEV;
>
> + reg_emerg = i2c_smbus_read_byte_data(new_client,
> + LM90_REG_R_REMOTE_EMERG);
> + if (reg_emerg < 0)
> + return -ENODEV;
> +
Seems like a rude action, considering that not all supported devices
even have this register. In fact, even reading this register at that
point of the detection is undesirable. At the very least, it will slow
down driver probing for other devices. You should read the register
only on Maxim chips.
> if ((address == 0x4C || address == 0x4D)
> && man_id == 0x01) { /* National Semiconductor */
> int reg_config2;
> @@ -770,6 +900,22 @@ static int lm90_detect(struct i2c_client *new_client,
> name = "max6657";
> } else
> /*
> + * Even though MAX6695 and MAX6696 do not have a chip ID
> + * register, reading it returns 0x01.
Regardless of the last read register value?
Bad Maxim, they really should learn from their past mistakes. Having a
device ID register really isn't that hard :(
> Bit 4 of the config1
> + * register is unused and should return zero when read.
> + *
> + * MAX6695 and MAX6696 have an additional set of temperature
> + * limit registers. We can detect those chips by checking if
> + * one of those registers exists (and thus returns a value
> + * different to the previous reading).
> + */
> + if (chip_id == 0x01
> + && (reg_config1 & 0x10) == 0x00
> + && reg_emerg != reg_convrate
Note that there is a remote chance that both values are equal even
though the registers are different. Of course this would mean a very
low emergency limit (below 10°C), is this the reason why you're
ignoring this case?
I'm not even sure what you are trying to protect yourself against.
Given the code flow, the MAX6657/58/59 have already been handled. Are
you aware of other Maxim chips, not handled by the lm90 driver,
behaving the same way?
> + && reg_convrate <= 0x07) {
> + name = "max6695";
> + } else
> + /*
As detection is weak, you may also want to check that bit 0 of status2
register is 0. Will slow things down a bit but... that's what you get
for poorly identifiable chips.
> * The chip_id register of the MAX6680 and MAX6681 holds the
> * revision of the chip. The lowest bit of the config1 register
> * is unused and should return zero when read, so should the
> @@ -842,6 +988,9 @@ static int lm90_probe(struct i2c_client *new_client,
> case lm86:
> data->alert_alarms = 0x7b;
> break;
> + case max6695:
> + data->alert_alarms = (0x18 << 8) | 0x7c;
I think 0x187c would be just as readable, wouldn't it?
> + break;
> default:
> data->alert_alarms = 0x7c;
> break;
> @@ -854,12 +1003,27 @@ static int lm90_probe(struct i2c_client *new_client,
> err = sysfs_create_group(&new_client->dev.kobj, &lm90_group);
> if (err)
> goto exit_free;
> +
> + if (lm90_have_emergency(data)) {
> + err = sysfs_create_group(&new_client->dev.kobj,
> + &lm90_emergency_group);
> + if (err)
> + goto exit_remove_base;
> + }
> +
> + if (data->kind == max6695) {
Don't we want lm90_have_temp3(data) or similar for this?
> + err = sysfs_create_group(&new_client->dev.kobj,
> + &lm90_temp3_group);
Please align on opening parenthesis as the rest of the code does.
> + if (err)
> + goto exit_remove_emergency;
> + }
> +
> if (new_client->flags & I2C_CLIENT_PEC) {
> err = device_create_file(&new_client->dev, &dev_attr_pec);
> if (err)
> goto exit_remove_files;
> }
> - if (data->kind != max6657 && data->kind != max6646) {
> + if (lm90_have_offset(data)) {
> err = device_create_file(&new_client->dev,
> &sensor_dev_attr_temp2_offset.dev_attr);
> if (err)
> @@ -875,6 +1039,13 @@ static int lm90_probe(struct i2c_client *new_client,
> return 0;
>
> exit_remove_files:
> + if (data->kind == max6695)
> + sysfs_remove_group(&new_client->dev.kobj, &lm90_temp3_group);
> +exit_remove_emergency:
> + if (lm90_have_emergency(data))
> + sysfs_remove_group(&new_client->dev.kobj,
> + &lm90_emergency_group);
> +exit_remove_base:
You know, it's always OK to remove files you didn't create, so you
don't have to add these labels. Every error path can basically point to
exit_remove_files. As a matter of fact, dev_attr_pec is created last
and removed last too.
> sysfs_remove_group(&new_client->dev.kobj, &lm90_group);
> device_remove_file(&new_client->dev, &dev_attr_pec);
> exit_free:
> @@ -913,6 +1084,12 @@ static void lm90_init_client(struct i2c_client *client)
> if (data->kind == max6680)
> config |= 0x18;
>
> + /*
> + * Select external channel 1 for max6695
> + */
> + if (data->kind == max6695)
> + config &= ~0x08;
> +
> config &= 0xBF; /* run */
> if (config != data->config_orig) /* Only write if changed */
> i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
> @@ -923,9 +1100,13 @@ static int lm90_remove(struct i2c_client *client)
> struct lm90_data *data = i2c_get_clientdata(client);
>
> hwmon_device_unregister(data->hwmon_dev);
> + if (lm90_have_emergency(data))
> + sysfs_remove_group(&client->dev.kobj, &lm90_emergency_group);
> + if (data->kind == max6695)
> + sysfs_remove_group(&client->dev.kobj, &lm90_temp3_group);
> sysfs_remove_group(&client->dev.kobj, &lm90_group);
> device_remove_file(&client->dev, &dev_attr_pec);
> - if (data->kind != max6657 && data->kind != max6646)
> + if (lm90_have_offset(data))
> device_remove_file(&client->dev,
> &sensor_dev_attr_temp2_offset.dev_attr);
BTW, we (you) may want to move all file removal code to a separate
function so that the code can be shared between lm90_probe and
lm90_remove.
>
> @@ -940,10 +1121,14 @@ static int lm90_remove(struct i2c_client *client)
> static void lm90_alert(struct i2c_client *client, unsigned int flag)
> {
> struct lm90_data *data = i2c_get_clientdata(client);
> - u8 config, alarms;
> + u8 config, alarms, alarms2 = 0;
>
> lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> - if ((alarms & 0x7f) == 0) {
> +
> + if (data->kind == max6695)
> + lm90_read_reg(client, LM90_REG_R_STATUS2, &alarms2);
> +
> + if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) {
> dev_info(&client->dev, "Everything OK\n");
> } else {
> if (alarms & 0x61)
> @@ -956,6 +1141,10 @@ static void lm90_alert(struct i2c_client *client, unsigned int flag)
> dev_warn(&client->dev,
> "temp%d diode open, please check!\n", 2);
>
> + if (alarms2 & 0x18)
> + dev_warn(&client->dev,
> + "temp%d out of range, please check!\n", 3);
> +
> /* Disable ALERT# output, because these chips don't implement
> SMBus alert correctly; they should only hold the alert line
> low briefly. */
> @@ -1011,6 +1200,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> if (time_after(jiffies, data->last_updated + HZ / 2 + HZ / 10)
> || !data->valid) {
> u8 h, l;
> + u8 alarms, alarms2 = 0;
You don't really need alarms, only alarms2. alarms only adds a copy for
all chips, which could be avoided.
>
> dev_dbg(&client->dev, "Updating lm90 data.\n");
> lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]);
> @@ -1019,7 +1209,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]);
> lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst);
>
> - if (data->kind == max6657 || data->kind == max6646) {
> + if (lm90_have_local_temp_ext(data)) {
> lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
> MAX6657_REG_R_LOCAL_TEMPL,
> &data->temp11[4]);
> @@ -1033,29 +1223,63 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>
> if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) {
> data->temp11[1] = h << 8;
> - if (data->kind != max6657 && data->kind != max6680
> - && data->kind != max6646
> + if (lm90_have_remote_limit_ext(data)
> && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL,
> &l) == 0)
> data->temp11[1] |= l;
> }
> if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) {
> data->temp11[2] = h << 8;
> - if (data->kind != max6657 && data->kind != max6680
> - && data->kind != max6646
> + if (lm90_have_remote_limit_ext(data)
> && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL,
> &l) == 0)
> data->temp11[2] |= l;
> }
>
> - if (data->kind != max6657 && data->kind != max6646) {
> + if (lm90_have_offset(data)) {
> if (lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSH,
> &h) == 0
> && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL,
> &l) == 0)
> data->temp11[3] = (h << 8) | l;
> }
> - lm90_read_reg(client, LM90_REG_R_STATUS, &data->alarms);
> +
> + if (data->kind == max6695) {
> + u8 config;
> +
> + lm90_read_reg(client, LM90_REG_R_LOCAL_EMERG,
> + &data->temp8[4]);
> + lm90_read_reg(client, LM90_REG_R_REMOTE_EMERG,
> + &data->temp8[5]);
These two should be read if (lm90_have_emergency()), as this is the
condition under which the corresponding attributes have been created.
> +
> + lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
> + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> + config | 0x08);
> +
> + lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
> + &data->temp8[6]);
> + lm90_read_reg(client, LM90_REG_R_REMOTE_EMERG,
> + &data->temp8[7]);
> + lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
> + LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]);
> + if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h)
> + && !lm90_read_reg(client,
> + LM90_REG_R_REMOTE_LOWL, &l))
> + data->temp11[6] = (h << 8) | l;
> + if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h)
> + && !lm90_read_reg(client,
> + LM90_REG_R_REMOTE_HIGHL, &l))
> + data->temp11[7] = (h << 8) | l;
Alignment of && is slightly different from what is done in the rest of
the driver.
> +
> + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> + config);
> +
> + lm90_read_reg(client, LM90_REG_R_STATUS2,
> + &alarms2);
> + }
> +
> + lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> + data->alarms = (alarms2 << 8) | alarms;
>
> /* Re-enable ALERT# output if it was originally enabled and
> * relevant alarms are all clear */
Overall it looks pretty good. Too bad these changes are heavily
underlining the design limitations of my driver. It has grown way
beyond what I imagined when writing it, and supports many more devices
with different features than it originally did.
If you are motivated to improve the driver's code to be more robust and
readable, feel free, I have no objection!
--
Jean Delvare
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: Add support for max6695 and max6696 to lm90 driver
2010-09-06 16:12 ` Jean Delvare
@ 2010-09-07 2:00 ` Guenter Roeck
2010-09-07 7:43 ` Jean Delvare
2010-09-08 10:28 ` Guenter Roeck
1 sibling, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2010-09-07 2:00 UTC (permalink / raw)
To: Jean Delvare
Cc: Andrew Morton, lm-sensors@lm-sensors.org,
linux-kernel@vger.kernel.org
Hi Jean,
On Mon, Sep 06, 2010 at 12:12:29PM -0400, Jean Delvare wrote:
> Hi Guenter,
>
> On Sat, 4 Sep 2010 15:34:35 -0700, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> > To apply this patch, the previously submitted lm90 cleanup patch has to be
> > applied first.
> >
> > My main concern with this patch is the chip detection code, specifically if it
> > is able to safely distinguish between MAX6680/81 and MAX6695/96.
> > Would be great to get some test coverage from a system with one of those chips.
>
> Unfortunately I don't have any of these Maxim chips at hand. I have an
> ADM1032 but it won't offer much coverage obviously. And I have dumps of
> Maxim chips, but the real chips behave differently, so it's of little
> help.
>
> Can you please add detection support to sensors-detect as well (and
> then update wiki/Devices)?
>
Sure. I planned to do that after the review is complete, but it makes sense
to add it to sensors-detect now.
> Review below:
>
> >
> > Sample sensors output:
> >
> > max6695-i2c-0-19
> > Adapter: SMBus I801 adapter at 5080
> > temp1: +24.5 C (low = -55.0 C, high = +70.0 C)
> > (crit = +70.0 C, hyst = +60.0 C)
> > temp2: +26.5 C (low = -55.0 C, high = +70.0 C)
> > (crit = +90.0 C, hyst = +80.0 C)
> > temp3: +24.1 C (low = -54.1 C, high = +70.2 C)
> > (crit = +90.0 C, hyst = +80.0 C)
> >
> > drivers/hwmon/lm90.c | 280 +++++++++++++++++++++++++++++++++++++++++++++-----
> > 1 files changed, 252 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > index aafed28..52ed792 100644
> > --- a/drivers/hwmon/lm90.c
> > +++ b/drivers/hwmon/lm90.c
> > @@ -42,6 +42,11 @@
> > * chips. The MAX6680 and MAX6681 only differ in the pinout so they can
> > * be treated identically.
> > *
> > + * This driver also supports the MAX6695 and MAX6696, two other sensor
> > + * chips made by Maxim. These are also quite similar to other Maxim
> > + * chips, but support three temperature sensors instead of two. MAX6695
> > + * and MAX6696 only differ in the pinout so they can be treated identically.
> > + *
>
> Please also update Documentation/hwmon/lm90 and drivers/hwmon/Kconfig.
>
Ok.
> You could also mention the additional emergency temperature limits, as
> this is a feature unique to these chips.
>
Ok.
> > * This driver also supports the ADT7461 chip from Analog Devices.
> > * It's supported in both compatibility and extended mode. It is mostly
> > * compatible with LM90 except for a data format difference for the
> > @@ -94,7 +99,7 @@ static const unsigned short normal_i2c[] = {
> > 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
> >
> > enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> > - w83l771 };
> > + max6695, w83l771 };
> >
> > /*
> > * The LM90 registers
> > @@ -109,6 +114,7 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> > #define LM90_REG_R_CONVRATE 0x04
> > #define LM90_REG_W_CONVRATE 0x0A
> > #define LM90_REG_R_STATUS 0x02
> > +#define LM90_REG_R_STATUS2 0x12
> > #define LM90_REG_R_LOCAL_TEMP 0x00
> > #define LM90_REG_R_LOCAL_HIGH 0x05
> > #define LM90_REG_W_LOCAL_HIGH 0x0B
> > @@ -130,12 +136,16 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> > #define LM90_REG_W_REMOTE_LOWH 0x0E
> > #define LM90_REG_R_REMOTE_LOWL 0x14
> > #define LM90_REG_W_REMOTE_LOWL 0x14
> > +#define LM90_REG_R_REMOTE_EMERG 0x16
> > +#define LM90_REG_W_REMOTE_EMERG 0x16
> > +#define LM90_REG_R_LOCAL_EMERG 0x17
> > +#define LM90_REG_W_LOCAL_EMERG 0x17
> > #define LM90_REG_R_REMOTE_CRIT 0x19
> > #define LM90_REG_W_REMOTE_CRIT 0x19
> > #define LM90_REG_R_TCRIT_HYST 0x21
> > #define LM90_REG_W_TCRIT_HYST 0x21
> >
> > -/* MAX6646/6647/6649/6657/6658/6659 registers */
> > +/* MAX6646/6647/6649/6657/6658/6659/6695/6696 registers */
> >
> > #define MAX6657_REG_R_LOCAL_TEMPL 0x11
> >
> > @@ -148,6 +158,7 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> > * Functions declaration
> > */
> >
> > +static int lm90_read_reg(struct i2c_client *client, u8 reg, u8 *value);
> > static int lm90_detect(struct i2c_client *client, struct i2c_board_info *info);
> > static int lm90_probe(struct i2c_client *client,
> > const struct i2c_device_id *id);
> > @@ -157,6 +168,23 @@ static int lm90_remove(struct i2c_client *client);
> > static struct lm90_data *lm90_update_device(struct device *dev);
> >
> > /*
> > + * Some useful macros
> > + */
> > +#define lm90_have_offset(data) \
> > + (data->kind != max6657 && data->kind != max6646 \
> > + && data->kind != max6695)
> > +
> > +#define lm90_have_local_temp_ext(data) \
> > + (data->kind == max6657 || data->kind == max6646 \
> > + || data->kind == max6695)
> > +
> > +#define lm90_have_remote_limit_ext(data) \
> > + (data->kind != max6657 && data->kind != max6646 \
> > + && data->kind != max6680 && data->kind != max6695)
> > +
> > +#define lm90_have_emergency(data) (data->kind == max6695)
>
> Makes the code more readable, I agree, but OTOH it hides complexity.
> Such tests are OK during probe or remove, as they happen only once, but
> seeing them in runtime code and in particular in the update function,
> seems wrong (even though I can't disagree that the overhead is quite
> small compared to the cost of SMBus transactions.)
>
Ultimately, hiding complexity was the reason to introduce the macros.
I figured it was better than having the comparisons spread through the code.
> I am wondering if it wouldn't be better to use data->flag to carry such
> feature information, which would be computed at probe time, once and
> for all. What do you think?
>
> Also, these macros could have been introduced in a separate patch, to
> make this one smaller, as they are good to have even without the
> max6695/96 support.
>
Makes sense. I'll do that (using flags) and resubmit as two separate patches.
> > +
> > +/*
> > * Driver data (common to all clients)
> > */
> >
> > @@ -175,6 +203,8 @@ static const struct i2c_device_id lm90_id[] = {
> > { "max6659", max6657 },
> > { "max6680", max6680 },
> > { "max6681", max6680 },
> > + { "max6695", max6695 },
> > + { "max6696", max6695 },
> > { "w83l771", w83l771 },
> > { }
> > };
> > @@ -206,20 +236,29 @@ struct lm90_data {
> > int flags;
> >
> > u8 config_orig; /* Original configuration register value */
> > - u8 alert_alarms; /* Which alarm bits trigger ALERT# */
> > + u16 alert_alarms; /* Which alarm bits trigger ALERT# */
> > + /* Upper 8 bits from max6695 STATUS2 register */
>
> The comment isn't quite correct. The contents of the STATUS2 register
> go to struct member alarms below, not alert_alarms. alert_alarms is set
> by the driver at initialization time.
>
ok
> >
> > /* registers values */
> > - s8 temp8[4]; /* 0: local low limit
> > + s8 temp8[8]; /* 0: local low limit
> > 1: local high limit
> > 2: local critical limit
> > - 3: remote critical limit */
> > - s16 temp11[5]; /* 0: remote input
> > + 3: remote critical limit
> > + 4: local emergency limit (max6695/96 only)
> > + 5: remote emergency limit (max6695/96 only)
> > + 6: remote 2 critical limit (max6695/96 only)
> > + 7: remote 2 emergency limit (max6695/96 only) */
> > + s16 temp11[8]; /* 0: remote input
> > 1: remote low limit
> > 2: remote high limit
> > - 3: remote offset (except max6646 and max6657)
> > - 4: local input */
> > + 3: remote offset (except max6646, max6657/59,
>
> And 58 too, no?
>
Correct.
> > + and max6695/96)
> > + 4: local input
> > + 5: remote 2 input (max6695/96 only)
> > + 6: remote 2 low limit (max6695/96 only)
> > + 7: remote 2 high limit (ma6695/96 only) */
> > u8 temp_hyst;
> > - u8 alarms; /* bitvector */
> > + u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
> > };
> >
> > /*
> > @@ -377,11 +416,15 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
> > static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> > const char *buf, size_t count)
> > {
> > - static const u8 reg[4] = {
> > + static const u8 reg[8] = {
> > LM90_REG_W_LOCAL_LOW,
> > LM90_REG_W_LOCAL_HIGH,
> > LM90_REG_W_LOCAL_CRIT,
> > LM90_REG_W_REMOTE_CRIT,
> > + LM90_REG_W_LOCAL_EMERG,
> > + LM90_REG_W_REMOTE_EMERG,
> > + LM90_REG_W_REMOTE_CRIT,
> > + LM90_REG_W_REMOTE_EMERG,
> > };
> >
> > struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > @@ -390,6 +433,7 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> > int nr = attr->index;
> > long val;
> > int err;
> > + u8 config;
> >
> > err = strict_strtol(buf, 10, &val);
> > if (err < 0)
> > @@ -406,7 +450,18 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> > data->temp8[nr] = temp_to_u8(val);
> > else
> > data->temp8[nr] = temp_to_s8(val);
> > +
> > + if (data->kind == max6695 && nr >= 6) {
> > + lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
> > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> > + config | 0x08);
> > + }
> > +
> > i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
> > +
> > + if (data->kind == max6695 && nr >= 6)
> > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
> > +
> > mutex_unlock(&data->update_lock);
> > return count;
> > }
> > @@ -450,6 +505,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> > int nr = attr->index;
> > long val;
> > int err;
> > + int offset = 1;
> > + u8 config;
> >
> > err = strict_strtol(buf, 10, &val);
> > if (err < 0)
> > @@ -462,19 +519,31 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> > mutex_lock(&data->update_lock);
> > if (data->kind == adt7461)
> > data->temp11[nr] = temp_to_u16_adt7461(data, val);
> > - else if (data->kind == max6657 || data->kind == max6680)
> > - data->temp11[nr] = temp_to_s8(val) << 8;
> > else if (data->kind == max6646)
> > data->temp11[nr] = temp_to_u8(val) << 8;
> > + else if (!lm90_have_remote_limit_ext(data))
> > + data->temp11[nr] = temp_to_s8(val) << 8;
> > else
> > data->temp11[nr] = temp_to_s16(val);
> >
> > - i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2],
> > + if (data->kind == max6695 && nr >= 6) {
> > + /* select output channel 2 */
> > + lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
> > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> > + config | 0x08);
> > + offset = 6;
> > + }
> > +
> > + i2c_smbus_write_byte_data(client, reg[(nr - offset) * 2],
> > data->temp11[nr] >> 8);
>
> This all gets a little tricky... Maybe it is time to rethink the whole
> thing.
>
You mean using the offset variable ? I would agree. No idea right now
how to improve it, though. I'll think about it.
> > - if (data->kind != max6657 && data->kind != max6680
> > - && data->kind != max6646)
> > - i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2 + 1],
> > + if (lm90_have_remote_limit_ext(data))
> > + i2c_smbus_write_byte_data(client, reg[(nr - offset) * 2 + 1],
> > data->temp11[nr] & 0xff);
> > +
> > + if (data->kind == max6695 && nr >= 6)
> > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> > + config);
> > +
> > mutex_unlock(&data->update_lock);
> > return count;
> > }
> > @@ -604,6 +673,62 @@ static const struct attribute_group lm90_group = {
> > .attrs = lm90_attributes,
> > };
> >
> > +/*
> > + * Additional attributes for devices with emergency sensors
> > + */
> > +static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
> > + set_temp8, 4);
> > +static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
> > + set_temp8, 5);
> > +
> > +/*
> > + * Additional attributes for devices with 3 temperature sensors
> > + */
> > +static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_temp11, NULL, 5);
> > +static SENSOR_DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
> > + set_temp11, 6);
> > +static SENSOR_DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
> > + set_temp11, 7);
> > +static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8,
> > + set_temp8, 6);
> > +static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 4);
> > +static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
> > + set_temp8, 7);
> > +
> > +static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO, show_alarm, NULL, 9);
> > +static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_alarm, NULL, 10);
> > +static SENSOR_DEVICE_ATTR(temp3_min_alarm, S_IRUGO, show_alarm, NULL, 11);
> > +static SENSOR_DEVICE_ATTR(temp3_max_alarm, S_IRUGO, show_alarm, NULL, 12);
>
> No alarms files for emergency limits?
>
Actually, there should be. Status register bits are there. No idea why I missed those.
Oh well, another ABI change. Is tempX_emergency_alarm ok ?
> > +
> > +static struct attribute *lm90_emergency_attributes[] = {
> > + &sensor_dev_attr_temp1_emergency.dev_attr.attr,
> > + &sensor_dev_attr_temp2_emergency.dev_attr.attr,
> > + NULL
> > +};
> > +
> > +static const struct attribute_group lm90_emergency_group = {
> > + .attrs = lm90_emergency_attributes,
> > +};
> > +
> > +static struct attribute *lm90_temp3_attributes[] = {
> > + &sensor_dev_attr_temp3_input.dev_attr.attr,
> > + &sensor_dev_attr_temp3_min.dev_attr.attr,
> > + &sensor_dev_attr_temp3_max.dev_attr.attr,
> > + &sensor_dev_attr_temp3_crit.dev_attr.attr,
> > + &sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
> > + &sensor_dev_attr_temp3_emergency.dev_attr.attr,
> > +
> > + &sensor_dev_attr_temp3_crit_alarm.dev_attr.attr,
> > + &sensor_dev_attr_temp3_fault.dev_attr.attr,
> > + &sensor_dev_attr_temp3_min_alarm.dev_attr.attr,
> > + &sensor_dev_attr_temp3_max_alarm.dev_attr.attr,
> > + NULL
> > +};
> > +
> > +static const struct attribute_group lm90_temp3_group = {
> > + .attrs = lm90_temp3_attributes,
> > +};
> > +
> > /* pec used for ADM1032 only */
> > static ssize_t show_pec(struct device *dev, struct device_attribute *dummy,
> > char *buf)
> > @@ -688,7 +813,7 @@ static int lm90_detect(struct i2c_client *new_client,
> > struct i2c_adapter *adapter = new_client->adapter;
> > int address = new_client->addr;
> > const char *name = NULL;
> > - int man_id, chip_id, reg_config1, reg_convrate;
> > + int man_id, chip_id, reg_config1, reg_convrate, reg_emerg;
> >
> > if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> > return -ENODEV;
> > @@ -704,6 +829,11 @@ static int lm90_detect(struct i2c_client *new_client,
> > LM90_REG_R_CONVRATE)) < 0)
> > return -ENODEV;
> >
> > + reg_emerg = i2c_smbus_read_byte_data(new_client,
> > + LM90_REG_R_REMOTE_EMERG);
> > + if (reg_emerg < 0)
> > + return -ENODEV;
> > +
>
> Seems like a rude action, considering that not all supported devices
> even have this register. In fact, even reading this register at that
> point of the detection is undesirable. At the very least, it will slow
> down driver probing for other devices. You should read the register
> only on Maxim chips.
>
Ok, makes sense. Basic idea was that we read chip_id all the time even if
the register doesn't exist, and react to it the same way. But you are right,
it should only be read for maxim chips.
> > if ((address == 0x4C || address == 0x4D)
> > && man_id == 0x01) { /* National Semiconductor */
> > int reg_config2;
> > @@ -770,6 +900,22 @@ static int lm90_detect(struct i2c_client *new_client,
> > name = "max6657";
> > } else
> > /*
> > + * Even though MAX6695 and MAX6696 do not have a chip ID
> > + * register, reading it returns 0x01.
>
> Regardless of the last read register value?
>
Unfortunately, yes. I thought the read would return man_id, but it doesn't.
> Bad Maxim, they really should learn from their past mistakes. Having a
> device ID register really isn't that hard :(
>
> > Bit 4 of the config1
> > + * register is unused and should return zero when read.
> > + *
> > + * MAX6695 and MAX6696 have an additional set of temperature
> > + * limit registers. We can detect those chips by checking if
> > + * one of those registers exists (and thus returns a value
> > + * different to the previous reading).
> > + */
> > + if (chip_id == 0x01
> > + && (reg_config1 & 0x10) == 0x00
> > + && reg_emerg != reg_convrate
>
> Note that there is a remote chance that both values are equal even
> though the registers are different. Of course this would mean a very
> low emergency limit (below 10°C), is this the reason why you're
> ignoring this case?
>
Yes. I'll think about it some more - maybe I find something better.
> I'm not even sure what you are trying to protect yourself against.
> Given the code flow, the MAX6657/58/59 have already been handled. Are
> you aware of other Maxim chips, not handled by the lm90 driver,
> behaving the same way?
>
There is still max6680, tested afterwards. I wanted to make sure as good as I can
that I don't detect max6680 as max6695.
Of course, who knows what max6680 returns when reading registers 0x16 / 0x17.
> > + && reg_convrate <= 0x07) {
> > + name = "max6695";
> > + } else
> > + /*
>
> As detection is weak, you may also want to check that bit 0 of status2
> register is 0. Will slow things down a bit but... that's what you get
> for poorly identifiable chips.
>
Ok. Maybe I can read the additional registers only after max6657 was detected.
> > * The chip_id register of the MAX6680 and MAX6681 holds the
> > * revision of the chip. The lowest bit of the config1 register
> > * is unused and should return zero when read, so should the
> > @@ -842,6 +988,9 @@ static int lm90_probe(struct i2c_client *new_client,
> > case lm86:
> > data->alert_alarms = 0x7b;
> > break;
> > + case max6695:
> > + data->alert_alarms = (0x18 << 8) | 0x7c;
>
> I think 0x187c would be just as readable, wouldn't it?
>
Yes.
> > + break;
> > default:
> > data->alert_alarms = 0x7c;
> > break;
> > @@ -854,12 +1003,27 @@ static int lm90_probe(struct i2c_client *new_client,
> > err = sysfs_create_group(&new_client->dev.kobj, &lm90_group);
> > if (err)
> > goto exit_free;
> > +
> > + if (lm90_have_emergency(data)) {
> > + err = sysfs_create_group(&new_client->dev.kobj,
> > + &lm90_emergency_group);
> > + if (err)
> > + goto exit_remove_base;
> > + }
> > +
> > + if (data->kind == max6695) {
>
> Don't we want lm90_have_temp3(data) or similar for this?
>
Ok.
> > + err = sysfs_create_group(&new_client->dev.kobj,
> > + &lm90_temp3_group);
>
> Please align on opening parenthesis as the rest of the code does.
>
Sure.
> > + if (err)
> > + goto exit_remove_emergency;
> > + }
> > +
> > if (new_client->flags & I2C_CLIENT_PEC) {
> > err = device_create_file(&new_client->dev, &dev_attr_pec);
> > if (err)
> > goto exit_remove_files;
> > }
> > - if (data->kind != max6657 && data->kind != max6646) {
> > + if (lm90_have_offset(data)) {
> > err = device_create_file(&new_client->dev,
> > &sensor_dev_attr_temp2_offset.dev_attr);
> > if (err)
> > @@ -875,6 +1039,13 @@ static int lm90_probe(struct i2c_client *new_client,
> > return 0;
> >
> > exit_remove_files:
> > + if (data->kind == max6695)
> > + sysfs_remove_group(&new_client->dev.kobj, &lm90_temp3_group);
> > +exit_remove_emergency:
> > + if (lm90_have_emergency(data))
> > + sysfs_remove_group(&new_client->dev.kobj,
> > + &lm90_emergency_group);
> > +exit_remove_base:
>
> You know, it's always OK to remove files you didn't create, so you
> don't have to add these labels. Every error path can basically point to
> exit_remove_files. As a matter of fact, dev_attr_pec is created last
> and removed last too.
>
Ok.
> > sysfs_remove_group(&new_client->dev.kobj, &lm90_group);
> > device_remove_file(&new_client->dev, &dev_attr_pec);
> > exit_free:
> > @@ -913,6 +1084,12 @@ static void lm90_init_client(struct i2c_client *client)
> > if (data->kind == max6680)
> > config |= 0x18;
> >
> > + /*
> > + * Select external channel 1 for max6695
> > + */
> > + if (data->kind == max6695)
> > + config &= ~0x08;
> > +
> > config &= 0xBF; /* run */
> > if (config != data->config_orig) /* Only write if changed */
> > i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
> > @@ -923,9 +1100,13 @@ static int lm90_remove(struct i2c_client *client)
> > struct lm90_data *data = i2c_get_clientdata(client);
> >
> > hwmon_device_unregister(data->hwmon_dev);
> > + if (lm90_have_emergency(data))
> > + sysfs_remove_group(&client->dev.kobj, &lm90_emergency_group);
> > + if (data->kind == max6695)
> > + sysfs_remove_group(&client->dev.kobj, &lm90_temp3_group);
> > sysfs_remove_group(&client->dev.kobj, &lm90_group);
> > device_remove_file(&client->dev, &dev_attr_pec);
> > - if (data->kind != max6657 && data->kind != max6646)
> > + if (lm90_have_offset(data))
> > device_remove_file(&client->dev,
> > &sensor_dev_attr_temp2_offset.dev_attr);
>
> BTW, we (you) may want to move all file removal code to a separate
> function so that the code can be shared between lm90_probe and
> lm90_remove.
>
Makes sense. I'll check if it also makes sense to put that into a separate patch.
> >
> > @@ -940,10 +1121,14 @@ static int lm90_remove(struct i2c_client *client)
> > static void lm90_alert(struct i2c_client *client, unsigned int flag)
> > {
> > struct lm90_data *data = i2c_get_clientdata(client);
> > - u8 config, alarms;
> > + u8 config, alarms, alarms2 = 0;
> >
> > lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> > - if ((alarms & 0x7f) == 0) {
> > +
> > + if (data->kind == max6695)
> > + lm90_read_reg(client, LM90_REG_R_STATUS2, &alarms2);
> > +
> > + if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) {
> > dev_info(&client->dev, "Everything OK\n");
> > } else {
> > if (alarms & 0x61)
> > @@ -956,6 +1141,10 @@ static void lm90_alert(struct i2c_client *client, unsigned int flag)
> > dev_warn(&client->dev,
> > "temp%d diode open, please check!\n", 2);
> >
> > + if (alarms2 & 0x18)
> > + dev_warn(&client->dev,
> > + "temp%d out of range, please check!\n", 3);
> > +
> > /* Disable ALERT# output, because these chips don't implement
> > SMBus alert correctly; they should only hold the alert line
> > low briefly. */
> > @@ -1011,6 +1200,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> > if (time_after(jiffies, data->last_updated + HZ / 2 + HZ / 10)
> > || !data->valid) {
> > u8 h, l;
> > + u8 alarms, alarms2 = 0;
>
> You don't really need alarms, only alarms2. alarms only adds a copy for
> all chips, which could be avoided.
>
You are right. I'll move alarms2 into the max6695 path and keep it local there.
> >
> > dev_dbg(&client->dev, "Updating lm90 data.\n");
> > lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]);
> > @@ -1019,7 +1209,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> > lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]);
> > lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst);
> >
> > - if (data->kind == max6657 || data->kind == max6646) {
> > + if (lm90_have_local_temp_ext(data)) {
> > lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
> > MAX6657_REG_R_LOCAL_TEMPL,
> > &data->temp11[4]);
> > @@ -1033,29 +1223,63 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> >
> > if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) {
> > data->temp11[1] = h << 8;
> > - if (data->kind != max6657 && data->kind != max6680
> > - && data->kind != max6646
> > + if (lm90_have_remote_limit_ext(data)
> > && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL,
> > &l) == 0)
> > data->temp11[1] |= l;
> > }
> > if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) {
> > data->temp11[2] = h << 8;
> > - if (data->kind != max6657 && data->kind != max6680
> > - && data->kind != max6646
> > + if (lm90_have_remote_limit_ext(data)
> > && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL,
> > &l) == 0)
> > data->temp11[2] |= l;
> > }
> >
> > - if (data->kind != max6657 && data->kind != max6646) {
> > + if (lm90_have_offset(data)) {
> > if (lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSH,
> > &h) == 0
> > && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL,
> > &l) == 0)
> > data->temp11[3] = (h << 8) | l;
> > }
> > - lm90_read_reg(client, LM90_REG_R_STATUS, &data->alarms);
> > +
> > + if (data->kind == max6695) {
> > + u8 config;
> > +
> > + lm90_read_reg(client, LM90_REG_R_LOCAL_EMERG,
> > + &data->temp8[4]);
> > + lm90_read_reg(client, LM90_REG_R_REMOTE_EMERG,
> > + &data->temp8[5]);
>
> These two should be read if (lm90_have_emergency()), as this is the
> condition under which the corresponding attributes have been created.
>
Ok.
> > +
> > + lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
> > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> > + config | 0x08);
> > +
> > + lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
> > + &data->temp8[6]);
> > + lm90_read_reg(client, LM90_REG_R_REMOTE_EMERG,
> > + &data->temp8[7]);
> > + lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
> > + LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]);
> > + if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h)
> > + && !lm90_read_reg(client,
> > + LM90_REG_R_REMOTE_LOWL, &l))
> > + data->temp11[6] = (h << 8) | l;
> > + if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h)
> > + && !lm90_read_reg(client,
> > + LM90_REG_R_REMOTE_HIGHL, &l))
> > + data->temp11[7] = (h << 8) | l;
>
> Alignment of && is slightly different from what is done in the rest of
> the driver.
>
I'll make sure it matches.
> > +
> > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> > + config);
> > +
> > + lm90_read_reg(client, LM90_REG_R_STATUS2,
> > + &alarms2);
> > + }
> > +
> > + lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> > + data->alarms = (alarms2 << 8) | alarms;
> >
> > /* Re-enable ALERT# output if it was originally enabled and
> > * relevant alarms are all clear */
>
> Overall it looks pretty good. Too bad these changes are heavily
> underlining the design limitations of my driver. It has grown way
> beyond what I imagined when writing it, and supports many more devices
> with different features than it originally did.
>
Unfortunately that is true. I was struggling for a while if I should write
a separate driver.
> If you are motivated to improve the driver's code to be more robust and
> readable, feel free, I have no objection!
>
I'll see if I can do some more cleanup.
Thanks a lot for the review!
Guenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: Add support for max6695 and max6696 to lm90 driver
2010-09-07 2:00 ` Guenter Roeck
@ 2010-09-07 7:43 ` Jean Delvare
0 siblings, 0 replies; 10+ messages in thread
From: Jean Delvare @ 2010-09-07 7:43 UTC (permalink / raw)
To: Guenter Roeck
Cc: Andrew Morton, lm-sensors@lm-sensors.org,
linux-kernel@vger.kernel.org
Guten Morgen Guenter,
On Mon, 6 Sep 2010 19:00:53 -0700, Guenter Roeck wrote:
> On Mon, Sep 06, 2010 at 12:12:29PM -0400, Jean Delvare wrote:
> > On Sat, 4 Sep 2010 15:34:35 -0700, Guenter Roeck wrote:
> > > @@ -450,6 +505,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> > > int nr = attr->index;
> > > long val;
> > > int err;
> > > + int offset = 1;
> > > + u8 config;
> > >
> > > err = strict_strtol(buf, 10, &val);
> > > if (err < 0)
> > > @@ -462,19 +519,31 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> > > mutex_lock(&data->update_lock);
> > > if (data->kind == adt7461)
> > > data->temp11[nr] = temp_to_u16_adt7461(data, val);
> > > - else if (data->kind == max6657 || data->kind == max6680)
> > > - data->temp11[nr] = temp_to_s8(val) << 8;
> > > else if (data->kind == max6646)
> > > data->temp11[nr] = temp_to_u8(val) << 8;
> > > + else if (!lm90_have_remote_limit_ext(data))
> > > + data->temp11[nr] = temp_to_s8(val) << 8;
> > > else
> > > data->temp11[nr] = temp_to_s16(val);
> > >
> > > - i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2],
> > > + if (data->kind == max6695 && nr >= 6) {
> > > + /* select output channel 2 */
> > > + lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
> > > + i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> > > + config | 0x08);
> > > + offset = 6;
> > > + }
> > > +
> > > + i2c_smbus_write_byte_data(client, reg[(nr - offset) * 2],
> > > data->temp11[nr] >> 8);
> >
> > This all gets a little tricky... Maybe it is time to rethink the whole
> > thing.
>
> You mean using the offset variable ? I would agree. No idea right now
> how to improve it, though. I'll think about it.
One option would be to switch to SENSOR_ATTR_2 and use the second
parameter as an index into the reg array, which in turn could become a
2-D array (or 1-D array of pair structs). I wasn't a big fan of
SENSOR_ATTR_2 originally because its attribute are small (u8) and
easily confused (index vs. nr) but it has its technical merits. Maybe
someday we'll unify SENSOR_ATTR and SENSOR_ATTR_2 into a single
structure with reasonably sized and named attributes. That will be a
lot of work though.
> > (...)
> > No alarms files for emergency limits?
>
> Actually, there should be. Status register bits are there. No idea why I missed those.
> Oh well, another ABI change. Is tempX_emergency_alarm ok ?
Of course it is.
> > (...)
> > Regardless of the last read register value?
>
> Unfortunately, yes. I thought the read would return man_id, but it doesn't.
It's not that bad. A constant, non-00, non-ff device ID, even
non-documented, has some value.
> > (...)
> > I'm not even sure what you are trying to protect yourself against.
> > Given the code flow, the MAX6657/58/59 have already been handled. Are
> > you aware of other Maxim chips, not handled by the lm90 driver,
> > behaving the same way?
>
> There is still max6680, tested afterwards. I wanted to make sure as good as I can
> that I don't detect max6680 as max6695.
Oh right. Same device ID, of course :(
> Of course, who knows what max6680 returns when reading registers 0x16 / 0x17.
I don't. But I can try obtaining the information for you, stay tuned.
> > (...)
> > As detection is weak, you may also want to check that bit 0 of status2
> > register is 0. Will slow things down a bit but... that's what you get
> > for poorly identifiable chips.
>
> Ok. Maybe I can read the additional registers only after max6657 was detected.
Yes, delaying register reads until they are really needed is good
practice.
> > (...)
> > BTW, we (you) may want to move all file removal code to a separate
> > function so that the code can be shared between lm90_probe and
> > lm90_remove.
>
> Makes sense. I'll check if it also makes sense to put that into a separate patch.
Yes, a separate patch would be good.
> (...)
> Thanks a lot for the review!
Welcome :)
--
Jean Delvare
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: Add support for max6695 and max6696 to lm90 driver
2010-09-06 16:12 ` Jean Delvare
2010-09-07 2:00 ` Guenter Roeck
@ 2010-09-08 10:28 ` Guenter Roeck
2010-09-08 11:48 ` Jean Delvare
1 sibling, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2010-09-08 10:28 UTC (permalink / raw)
To: Jean Delvare
Cc: Andrew Morton, lm-sensors@lm-sensors.org,
linux-kernel@vger.kernel.org
Hi Jean,
On Mon, Sep 06, 2010 at 12:12:29PM -0400, Jean Delvare wrote:
> Hi Guenter,
>
> On Sat, 4 Sep 2010 15:34:35 -0700, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> > To apply this patch, the previously submitted lm90 cleanup patch has to be
> > applied first.
> >
> > My main concern with this patch is the chip detection code, specifically if it
> > is able to safely distinguish between MAX6680/81 and MAX6695/96.
> > Would be great to get some test coverage from a system with one of those chips.
>
> Unfortunately I don't have any of these Maxim chips at hand. I have an
> ADM1032 but it won't offer much coverage obviously. And I have dumps of
> Maxim chips, but the real chips behave differently, so it's of little
> help.
>
Do you by any chance have register dumps of max6657/58/59 ?
max6659 also supports a 3rd upper limit. Turns out it is convenient to add support
for this limit first before adding support for max6696. To do that, I'll need
to be able to distinguish between max6657/58 and max6659. My thought is to check
for the address and if register 0x16 exists.
Also, do you happen to remember why address 0x4e for max6659 is not supported by the driver ?
The code only says that it isn't supported, and it does not detect it, but there
is not reason. sensors-detect does detect it at address 0x4e.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: Add support for max6695 and max6696 to lm90 driver
2010-09-08 10:28 ` Guenter Roeck
@ 2010-09-08 11:48 ` Jean Delvare
2010-09-08 13:56 ` Guenter Roeck
0 siblings, 1 reply; 10+ messages in thread
From: Jean Delvare @ 2010-09-08 11:48 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Andrew Morton, lm-sensors, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2220 bytes --]
On Wed, 8 Sep 2010 03:28:16 -0700, Guenter Roeck wrote:
> Hi Jean,
>
> On Mon, Sep 06, 2010 at 12:12:29PM -0400, Jean Delvare wrote:
> > Hi Guenter,
> >
> > On Sat, 4 Sep 2010 15:34:35 -0700, Guenter Roeck wrote:
> > > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > > ---
> > > To apply this patch, the previously submitted lm90 cleanup patch has to be
> > > applied first.
> > >
> > > My main concern with this patch is the chip detection code, specifically if it
> > > is able to safely distinguish between MAX6680/81 and MAX6695/96.
> > > Would be great to get some test coverage from a system with one of those chips.
> >
> > Unfortunately I don't have any of these Maxim chips at hand. I have an
> > ADM1032 but it won't offer much coverage obviously. And I have dumps of
> > Maxim chips, but the real chips behave differently, so it's of little
> > help.
> >
> Do you by any chance have register dumps of max6657/58/59 ?
Yes I do. I have dumps of max6658 and max6659 chips, I'm attaching
them. Please keep in mind that registers are read sequentially, in
order, and this matters because these chips return the last read value
whenever you access a nonexistent register. This even holds for
registers with fewer than 8 bits, e.g. config1.
> max6659 also supports a 3rd upper limit. Turns out it is convenient to add support
> for this limit first before adding support for max6696. To do that, I'll need
> to be able to distinguish between max6657/58 and max6659. My thought is to check
> for the address and if register 0x16 exists.
>
> Also, do you happen to remember why address 0x4e for max6659 is not supported by the driver ?
> The code only says that it isn't supported, and it does not detect it, but there
> is not reason. sensors-detect does detect it at address 0x4e.
I think the reason is historical. As we never had a report of anyone
with a MAX6659 chip at 0x4e, and detection for this device is weak, and
the driver didn't support any other device at this address, it seemed
more reasonable to not scan the address at all. Now that the driver
supports more devices at this address and thus scans it anyway, I have
no objection supporting the MAX6659 at 0x4e.
--
Jean Delvare
[-- Attachment #2: max6658.dump --]
[-- Type: application/octet-stream, Size: 1241 bytes --]
0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
00: 1a 1f 00 20 05 46 c9 50 c9 c9 c9 c9 c9 c9 c9 c9 ??. ?F?P????????
10: 60 80 80 80 80 80 55 55 00 55 55 55 55 55 55 55 `?????UU.UUUUUUU
20: 55 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a U???????????????
30: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
40: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
50: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
60: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
70: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
80: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
90: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
a0: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
b0: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
c0: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
d0: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
e0: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
f0: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 4d 4d ??????????????MM
[-- Attachment #3: max6659.dump --]
[-- Type: application/octet-stream, Size: 1504 bytes --]
From: Stefan Assmann <S.Assmann@gmx.de>
asus:/sys/bus/i2c/devices/0-004c# i2cdump 0 0x4c
No size specified (using byte-data access)
WARNING! This program can confuse your I2C bus, cause data loss and worse!
I will probe file /dev/i2c-0, address 0x4c, mode byte
Continue? [Y/n] y
0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
00: 39 32 00 00 04 46 00 37 00 00 00 00 00 00 00 00 92..?F.7........
10: 80 00 00 00 00 00 55 55 00 64 64 64 64 64 64 64 ?.....UU.ddddddd
20: 64 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a d???????????????
30: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
40: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
50: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
60: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
70: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
80: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
90: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
a0: 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a 0a ????????????????
b0: 0a 0a 0a 0a 0a 0a 0a 00 00 00 00 00 00 00 00 00 ???????.........
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 4d 4d ..............MM
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: Add support for max6695 and max6696 to lm90 driver
2010-09-08 11:48 ` Jean Delvare
@ 2010-09-08 13:56 ` Guenter Roeck
2010-09-08 15:29 ` Jean Delvare
0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2010-09-08 13:56 UTC (permalink / raw)
To: Jean Delvare
Cc: Andrew Morton, lm-sensors@lm-sensors.org,
linux-kernel@vger.kernel.org
Hi Jean,
On Wed, Sep 08, 2010 at 07:48:54AM -0400, Jean Delvare wrote:
> On Wed, 8 Sep 2010 03:28:16 -0700, Guenter Roeck wrote:
> > Hi Jean,
> >
> > On Mon, Sep 06, 2010 at 12:12:29PM -0400, Jean Delvare wrote:
> > > Hi Guenter,
> > >
> > > On Sat, 4 Sep 2010 15:34:35 -0700, Guenter Roeck wrote:
> > > > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > > > ---
> > > > To apply this patch, the previously submitted lm90 cleanup patch has to be
> > > > applied first.
> > > >
> > > > My main concern with this patch is the chip detection code, specifically if it
> > > > is able to safely distinguish between MAX6680/81 and MAX6695/96.
> > > > Would be great to get some test coverage from a system with one of those chips.
> > >
> > > Unfortunately I don't have any of these Maxim chips at hand. I have an
> > > ADM1032 but it won't offer much coverage obviously. And I have dumps of
> > > Maxim chips, but the real chips behave differently, so it's of little
> > > help.
> > >
> > Do you by any chance have register dumps of max6657/58/59 ?
>
> Yes I do. I have dumps of max6658 and max6659 chips, I'm attaching
> them. Please keep in mind that registers are read sequentially, in
> order, and this matters because these chips return the last read value
> whenever you access a nonexistent register. This even holds for
> registers with fewer than 8 bits, e.g. config1.
>
> > max6659 also supports a 3rd upper limit. Turns out it is convenient to add support
> > for this limit first before adding support for max6696. To do that, I'll need
> > to be able to distinguish between max6657/58 and max6659. My thought is to check
> > for the address and if register 0x16 exists.
> >
> > Also, do you happen to remember why address 0x4e for max6659 is not supported by the driver ?
> > The code only says that it isn't supported, and it does not detect it, but there
> > is not reason. sensors-detect does detect it at address 0x4e.
>
> I think the reason is historical. As we never had a report of anyone
> with a MAX6659 chip at 0x4e, and detection for this device is weak, and
> the driver didn't support any other device at this address, it seemed
> more reasonable to not scan the address at all. Now that the driver
> supports more devices at this address and thus scans it anyway, I have
> no objection supporting the MAX6659 at 0x4e.
>
Too bad - registers 0x16 and 0x17 exist on both 6658 and 6659. So the only way to detect 6659
would be the address (0x4d or 0x4e), and we would mis-detect it on 0x4c. Is that worth it ?
Guenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: Add support for max6695 and max6696 to lm90 driver
2010-09-08 13:56 ` Guenter Roeck
@ 2010-09-08 15:29 ` Jean Delvare
2010-09-08 18:30 ` Guenter Roeck
0 siblings, 1 reply; 10+ messages in thread
From: Jean Delvare @ 2010-09-08 15:29 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Andrew Morton, lm-sensors, linux-kernel
On Wed, 8 Sep 2010 06:56:54 -0700, Guenter Roeck wrote:
> Too bad - registers 0x16 and 0x17 exist on both 6658 and 6659. So the only way to detect 6659
> would be the address (0x4d or 0x4e), and we would mis-detect it on 0x4c. Is that worth it ?
I'd say adding support for the MAX6659 is worth it. Just don't add
detection. That is, all of MAX6657, 6658 and 6658 should be detected as
max6657, which has the minimum set of features. But if someone declares
a "max6659" device either as part of the platform data or from
user-space, then the driver should expose all the chip features.
Deal?
--
Jean Delvare
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: Add support for max6695 and max6696 to lm90 driver
2010-09-08 15:29 ` Jean Delvare
@ 2010-09-08 18:30 ` Guenter Roeck
2010-09-08 19:44 ` Jean Delvare
0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2010-09-08 18:30 UTC (permalink / raw)
To: Jean Delvare
Cc: Andrew Morton, lm-sensors@lm-sensors.org,
linux-kernel@vger.kernel.org
On Wed, Sep 08, 2010 at 11:29:02AM -0400, Jean Delvare wrote:
> On Wed, 8 Sep 2010 06:56:54 -0700, Guenter Roeck wrote:
> > Too bad - registers 0x16 and 0x17 exist on both 6658 and 6659. So the only way to detect 6659
> > would be the address (0x4d or 0x4e), and we would mis-detect it on 0x4c. Is that worth it ?
>
> I'd say adding support for the MAX6659 is worth it. Just don't add
> detection. That is, all of MAX6657, 6658 and 6658 should be detected as
> max6657, which has the minimum set of features. But if someone declares
> a "max6659" device either as part of the platform data or from
> user-space, then the driver should expose all the chip features.
>
> Deal?
>
I'd say yes, but then we would deliberately mis-detect the 6659 on address 0x4d and 0x4e,
which kind of hurts my consciousness.
How about a middle ground - mis-detect it on address 0x4c, but detect it correctly
on 0x4d and 0x4e ? Should be ok if we add a note into the file and into the documentation,
and we would do as good as we can.
Guenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: Add support for max6695 and max6696 to lm90 driver
2010-09-08 18:30 ` Guenter Roeck
@ 2010-09-08 19:44 ` Jean Delvare
0 siblings, 0 replies; 10+ messages in thread
From: Jean Delvare @ 2010-09-08 19:44 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Andrew Morton, lm-sensors, linux-kernel
On Wed, 8 Sep 2010 11:30:19 -0700, Guenter Roeck wrote:
> On Wed, Sep 08, 2010 at 11:29:02AM -0400, Jean Delvare wrote:
> > On Wed, 8 Sep 2010 06:56:54 -0700, Guenter Roeck wrote:
> > > Too bad - registers 0x16 and 0x17 exist on both 6658 and 6659. So the only way to detect 6659
> > > would be the address (0x4d or 0x4e), and we would mis-detect it on 0x4c. Is that worth it ?
> >
> > I'd say adding support for the MAX6659 is worth it. Just don't add
> > detection. That is, all of MAX6657, 6658 and 6658 should be detected as
> > max6657, which has the minimum set of features. But if someone declares
> > a "max6659" device either as part of the platform data or from
> > user-space, then the driver should expose all the chip features.
> >
> > Deal?
> >
> I'd say yes, but then we would deliberately mis-detect the 6659 on address 0x4d and 0x4e,
> which kind of hurts my consciousness.
We've been doing it for years already, and it didn't hurt mine ;)
> How about a middle ground - mis-detect it on address 0x4c, but detect it correctly
> on 0x4d and 0x4e ? Should be ok if we add a note into the file and into the documentation,
> and we would do as good as we can.
If that makes you feel better, sure, I have no objection!
--
Jean Delvare
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-09-08 19:45 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-04 22:34 [PATCH] hwmon: Add support for max6695 and max6696 to lm90 driver Guenter Roeck
2010-09-06 16:12 ` Jean Delvare
2010-09-07 2:00 ` Guenter Roeck
2010-09-07 7:43 ` Jean Delvare
2010-09-08 10:28 ` Guenter Roeck
2010-09-08 11:48 ` Jean Delvare
2010-09-08 13:56 ` Guenter Roeck
2010-09-08 15:29 ` Jean Delvare
2010-09-08 18:30 ` Guenter Roeck
2010-09-08 19:44 ` Jean Delvare
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox