This patch set enhance the lm90 driver,
it make the driver more readable and easier to use thermal framework.
This series is v2, previous version patches:
[RFC]: http://thread.gmane.org/gmane.linux.power-management.general/31056
[v1]: http://thread.gmane.org/gmane.linux.ports.tegra/11710/
[v2]: http://www.mail-archive.com/[email protected]/msg465555.html
Changes from v2:
1. update the defines for status bit, and go into a separate patch.
2. introduce the new lm90_is_tripped() for lm90_irq_thread and lm90_alert.
Changes from v1:
1. change the string "irq" to "IRQ"
2. add macro defines for the alarm status
3. consider the shared IRQ.
Changes from RFC:
1. change _show_temp() to read_temp(), _set_temp() to write_temp().
2. simply return value for the read_temp(), not use pointer.
3. use devm_request_threaded_irq() to request irq and set flag IRQF_ONESHOT.
Wei Ni (4):
hwmon: (lm90) split set&show temp as common codes
hwmon: (lm90) use macro defines for the status bit
hwmon: (lm90) add support to handle IRQ
hwmon: (lm90) use enums for the indexes of temp8 and temp11
drivers/hwmon/lm90.c | 387 +++++++++++++++++++++++++++++++++-----------------
1 file changed, 256 insertions(+), 131 deletions(-)
--
1.7.9.5
Add bit defines for the status register.
Signed-off-by: Wei Ni <[email protected]>
---
drivers/hwmon/lm90.c | 72 ++++++++++++++++++++++++++++++++++----------------
1 file changed, 49 insertions(+), 23 deletions(-)
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 5f30f90..c90037f 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -179,6 +179,19 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
#define LM90_HAVE_TEMP3 (1 << 6) /* 3rd temperature sensor */
#define LM90_HAVE_BROKEN_ALERT (1 << 7) /* Broken alert */
+/* LM90 status */
+#define LM90_STATUS_LTHRM (1 << 0) /* local THERM limit tripped */
+#define LM90_STATUS_RTHRM (1 << 1) /* remote THERM limit tripped */
+#define LM90_STATUS_OPEN (1 << 2) /* remote is an open circuit */
+#define LM90_STATUS_RLOW (1 << 3) /* remote low temp limit tripped */
+#define LM90_STATUS_RHIGH (1 << 4) /* remote high temp limit tripped */
+#define LM90_STATUS_LLOW (1 << 5) /* local low temp limit tripped */
+#define LM90_STATUS_LHIGH (1 << 6) /* local high temp limit tripped */
+#define LM90_STATUS_BUSY (1 << 7) /* ADC is converting */
+
+#define MAX6696_STATUS2_RLOW (1 << 3) /* remote2 low temp limit tripped */
+#define MAX6696_STATUS2_RHIGH (1 << 4) /* remote2 high temp limit tripped */
+
/*
* Driver data (common to all clients)
*/
@@ -1417,6 +1430,36 @@ static void lm90_init_client(struct i2c_client *client)
i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
}
+static bool lm90_is_tripped(struct i2c_client *client)
+{
+ struct lm90_data *data = i2c_get_clientdata(client);
+ u8 status, status2 = 0;
+
+ lm90_read_reg(client, LM90_REG_R_STATUS, &status);
+
+ if (data->kind == max6696)
+ lm90_read_reg(client, MAX6696_REG_R_STATUS2, &status2);
+
+ if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0)
+ return false;
+
+ if (status & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH | LM90_STATUS_LTHRM))
+ dev_warn(&client->dev,
+ "temp%d out of range, please check!\n", 1);
+ if (status & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH | LM90_STATUS_RTHRM))
+ dev_warn(&client->dev,
+ "temp%d out of range, please check!\n", 2);
+ if (status & LM90_STATUS_OPEN)
+ dev_warn(&client->dev,
+ "temp%d diode open, please check!\n", 2);
+
+ if (status2 & (MAX6696_STATUS2_RLOW | MAX6696_STATUS2_RHIGH))
+ dev_warn(&client->dev,
+ "temp%d out of range, please check!\n", 3);
+
+ return true;
+}
+
static int lm90_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -1515,36 +1558,19 @@ 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, alarms2 = 0;
-
- lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
-
- if (data->kind == max6696)
- lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms2);
-
- if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) {
+ if (!lm90_is_tripped(client)) {
dev_info(&client->dev, "Everything OK\n");
} else {
- if (alarms & 0x61)
- dev_warn(&client->dev,
- "temp%d out of range, please check!\n", 1);
- if (alarms & 0x1a)
- dev_warn(&client->dev,
- "temp%d out of range, please check!\n", 2);
- if (alarms & 0x04)
- 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.
*/
+ struct lm90_data *data = i2c_get_clientdata(client);
+ u8 config, alarms;
+
+ lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
+
if ((data->flags & LM90_HAVE_BROKEN_ALERT)
&& (alarms & data->alert_alarms)) {
dev_dbg(&client->dev, "Disabling ALERT#\n");
--
1.7.9.5
Using enums for the indexes and nrs of temp8 and temp11.
This make the code much more readable.
Signed-off-by: Wei Ni <[email protected]>
---
drivers/hwmon/lm90.c | 179 ++++++++++++++++++++++++++++++++------------------
1 file changed, 114 insertions(+), 65 deletions(-)
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 1cc3d19..8cb5dd0 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -310,6 +310,59 @@ static const struct lm90_params lm90_params[] = {
};
/*
+ * TEMP8 register index
+ */
+enum lm90_temp8_reg_index {
+ TEMP8_LOCAL_LOW = 0, /* 0: local low limit */
+ TEMP8_LOCAL_HIGH, /* 1: local high limit */
+ TEMP8_LOCAL_CRIT, /* 2: local critical limit */
+ TEMP8_REMOTE_CRIT, /* 3: remote critical limit */
+ TEMP8_LOCAL_EMERG, /* 4: local emergency limit
+ * (max6659 and max6695/96)
+ */
+ TEMP8_REMOTE_EMERG, /* 5: remote emergency limit
+ * (max6659 and max6695/96)
+ */
+ TEMP8_REMOTE2_CRIT, /* 6: remote 2 critical limit
+ * (max6695/96 only)
+ */
+ TEMP8_REMOTE2_EMERG, /* 7: remote 2 emergency limit
+ * (max6695/96 only)
+ */
+ TEMP8_REG_NUM
+};
+
+/*
+ * TEMP11 register index
+ */
+enum lm90_temp11_reg_index {
+ TEMP11_REMOTE_TEMP = 0, /* 0: remote input */
+ TEMP11_REMOTE_LOW, /* 1: remote low limit */
+ TEMP11_REMOTE_HIGH, /* 2: remote high limit */
+ TEMP11_REMOTE_OFFSET, /* 3: remote offset
+ * (except max6646, max6657/58/59,
+ * and max6695/96)
+ */
+ TEMP11_LOCAL_TEMP, /* 4: local input */
+ TEMP11_REMOTE2_TEMP, /* 5: remote 2 input (max6695/96 only) */
+ TEMP11_REMOTE2_LOW, /* 6: remote 2 low limit (max6695/96 only) */
+ TEMP11_REMOTE2_HIGH, /* 7: remote 2 high limit (max6695/96 only) */
+ TEMP11_REG_NUM
+};
+
+/*
+ * TEMP11 register NR
+ */
+enum lm90_temp11_reg_nr {
+ NR_CHAN_0_REMOTE_LOW = 0, /* 0: channel 0, remote low limit */
+ NR_CHAN_0_REMOTE_HIGH, /* 1: channel 0, remote high limit */
+ NR_CHAN_0_REMOTE_OFFSET, /* 2: channel 0, remote offset */
+ NR_CHAN_1_REMOTE_LOW, /* 3: channel 1, remote low limit */
+ NR_CHAN_1_REMOTE_HIGH, /* 4: channel 1, remote high limit */
+ NR_NUM /* number of the NRs for temp11 */
+};
+
+/*
* Client data (each client gets its own)
*/
@@ -331,25 +384,8 @@ struct lm90_data {
u8 reg_local_ext; /* local extension register offset */
/* registers values */
- s8 temp8[8]; /* 0: local low limit
- * 1: local high limit
- * 2: local critical limit
- * 3: remote critical limit
- * 4: local emergency limit (max6659 and max6695/96)
- * 5: remote emergency limit (max6659 and max6695/96)
- * 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, max6657/58/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 (max6695/96 only)
- */
+ s8 temp8[TEMP8_REG_NUM];
+ s16 temp11[TEMP11_REG_NUM];
u8 temp_hyst;
u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
};
@@ -491,37 +527,42 @@ static struct lm90_data *lm90_update_device(struct device *dev)
u8 alarms;
dev_dbg(&client->dev, "Updating lm90 data.\n");
- lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]);
- lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, &data->temp8[1]);
- lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, &data->temp8[2]);
- lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]);
+ lm90_read_reg(client, LM90_REG_R_LOCAL_LOW,
+ &data->temp8[TEMP8_LOCAL_LOW]);
+ lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH,
+ &data->temp8[TEMP8_LOCAL_HIGH]);
+ lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT,
+ &data->temp8[TEMP8_LOCAL_CRIT]);
+ lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
+ &data->temp8[TEMP8_REMOTE_CRIT]);
lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst);
if (data->reg_local_ext) {
lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
data->reg_local_ext,
- &data->temp11[4]);
+ &data->temp11[TEMP11_LOCAL_TEMP]);
} else {
if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
&h) == 0)
- data->temp11[4] = h << 8;
+ data->temp11[TEMP11_LOCAL_TEMP] = h << 8;
}
lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
- LM90_REG_R_REMOTE_TEMPL, &data->temp11[0]);
+ LM90_REG_R_REMOTE_TEMPL,
+ &data->temp11[TEMP11_REMOTE_TEMP]);
if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) {
- data->temp11[1] = h << 8;
+ data->temp11[TEMP11_REMOTE_LOW] = h << 8;
if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
&& lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL,
&l) == 0)
- data->temp11[1] |= l;
+ data->temp11[TEMP11_REMOTE_LOW] |= l;
}
if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) {
- data->temp11[2] = h << 8;
+ data->temp11[TEMP11_REMOTE_HIGH] = h << 8;
if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
&& lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL,
&l) == 0)
- data->temp11[2] |= l;
+ data->temp11[TEMP11_REMOTE_HIGH] |= l;
}
if (data->flags & LM90_HAVE_OFFSET) {
@@ -529,13 +570,14 @@ static struct lm90_data *lm90_update_device(struct device *dev)
&h) == 0
&& lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL,
&l) == 0)
- data->temp11[3] = (h << 8) | l;
+ data->temp11[TEMP11_REMOTE_OFFSET] =
+ (h << 8) | l;
}
if (data->flags & LM90_HAVE_EMERGENCY) {
lm90_read_reg(client, MAX6659_REG_R_LOCAL_EMERG,
- &data->temp8[4]);
+ &data->temp8[TEMP8_LOCAL_EMERG]);
lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
- &data->temp8[5]);
+ &data->temp8[TEMP8_REMOTE_EMERG]);
}
lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
data->alarms = alarms; /* save as 16 bit value */
@@ -543,15 +585,16 @@ static struct lm90_data *lm90_update_device(struct device *dev)
if (data->kind == max6696) {
lm90_select_remote_channel(client, data, 1);
lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
- &data->temp8[6]);
+ &data->temp8[TEMP8_REMOTE2_CRIT]);
lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
- &data->temp8[7]);
+ &data->temp8[TEMP8_REMOTE2_EMERG]);
lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
- LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]);
+ LM90_REG_R_REMOTE_TEMPL,
+ &data->temp11[TEMP11_REMOTE2_TEMP]);
if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h))
- data->temp11[6] = h << 8;
+ data->temp11[TEMP11_REMOTE2_LOW] = h << 8;
if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h))
- data->temp11[7] = h << 8;
+ data->temp11[TEMP11_REMOTE2_HIGH] = h << 8;
lm90_select_remote_channel(client, data, 0);
if (!lm90_read_reg(client, MAX6696_REG_R_STATUS2,
@@ -745,7 +788,7 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
static void write_temp8(struct device *dev, int index, long val)
{
- static const u8 reg[8] = {
+ static const u8 reg[TEMP8_REG_NUM] = {
LM90_REG_W_LOCAL_LOW,
LM90_REG_W_LOCAL_HIGH,
LM90_REG_W_LOCAL_CRIT,
@@ -828,7 +871,7 @@ static void write_temp11(struct device *dev, int nr, int index, long val)
u8 high;
u8 low;
int channel;
- } reg[5] = {
+ } reg[NR_NUM] = {
{ LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 0 },
{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 0 },
{ LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL, 0 },
@@ -919,11 +962,12 @@ static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy,
mutex_lock(&data->update_lock);
if (data->kind == adt7461)
- temp = temp_from_u8_adt7461(data, data->temp8[2]);
+ temp = temp_from_u8_adt7461(data,
+ data->temp8[TEMP8_LOCAL_CRIT]);
else if (data->kind == max6646)
- temp = temp_from_u8(data->temp8[2]);
+ temp = temp_from_u8(data->temp8[TEMP8_LOCAL_CRIT]);
else
- temp = temp_from_s8(data->temp8[2]);
+ temp = temp_from_s8(data->temp8[TEMP8_LOCAL_CRIT]);
data->temp_hyst = hyst_to_reg(temp - val);
i2c_smbus_write_byte_data(client, LM90_REG_W_TCRIT_HYST,
@@ -977,25 +1021,28 @@ static ssize_t set_update_interval(struct device *dev,
return count;
}
-static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, 0, 4);
-static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, 0, 0);
+static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL,
+ NR_CHAN_0_REMOTE_LOW, TEMP11_LOCAL_TEMP);
+static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL,
+ NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_TEMP);
static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8,
- set_temp8, 0);
+ set_temp8, TEMP8_LOCAL_LOW);
static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11,
- set_temp11, 0, 1);
+ set_temp11, NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_LOW);
static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8,
- set_temp8, 1);
+ set_temp8, TEMP8_LOCAL_HIGH);
static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
- set_temp11, 1, 2);
+ set_temp11, NR_CHAN_0_REMOTE_HIGH, TEMP11_REMOTE_HIGH);
static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8,
- set_temp8, 2);
+ set_temp8, TEMP8_LOCAL_CRIT);
static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8,
- set_temp8, 3);
+ set_temp8, TEMP8_REMOTE_CRIT);
static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst,
- set_temphyst, 2);
-static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, 3);
+ set_temphyst, TEMP8_LOCAL_CRIT);
+static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL,
+ TEMP8_REMOTE_CRIT);
static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11,
- set_temp11, 2, 3);
+ set_temp11, NR_CHAN_0_REMOTE_OFFSET, TEMP11_REMOTE_OFFSET);
/* Individual alarm files */
static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0);
@@ -1043,13 +1090,13 @@ static const struct attribute_group lm90_group = {
* Additional attributes for devices with emergency sensors
*/
static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
- set_temp8, 4);
+ set_temp8, TEMP8_LOCAL_EMERG);
static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
- set_temp8, 5);
+ set_temp8, TEMP8_REMOTE_EMERG);
static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO, show_temphyst,
- NULL, 4);
+ NULL, TEMP8_LOCAL_EMERG);
static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO, show_temphyst,
- NULL, 5);
+ NULL, TEMP8_REMOTE_EMERG);
static struct attribute *lm90_emergency_attributes[] = {
&sensor_dev_attr_temp1_emergency.dev_attr.attr,
@@ -1079,18 +1126,20 @@ static const struct attribute_group lm90_emergency_alarm_group = {
/*
* Additional attributes for devices with 3 temperature sensors
*/
-static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, 0, 5);
+static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL,
+ NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE2_TEMP);
static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
- set_temp11, 3, 6);
+ set_temp11, NR_CHAN_1_REMOTE_LOW, TEMP11_REMOTE2_LOW);
static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
- set_temp11, 4, 7);
+ set_temp11, NR_CHAN_1_REMOTE_HIGH, TEMP11_REMOTE2_HIGH);
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, 6);
+ set_temp8, TEMP8_REMOTE2_CRIT);
+static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL,
+ TEMP8_REMOTE2_CRIT);
static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
- set_temp8, 7);
+ set_temp8, TEMP8_REMOTE2_EMERG);
static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO, show_temphyst,
- NULL, 7);
+ NULL, TEMP8_REMOTE2_EMERG);
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);
--
1.7.9.5
Split set&show temp codes as common functions, so we can use it directly when
implement linux thermal framework.
Signed-off-by: Wei Ni <[email protected]>
---
drivers/hwmon/lm90.c | 112 +++++++++++++++++++++++++++++++-------------------
1 file changed, 69 insertions(+), 43 deletions(-)
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 8eeb141..5f30f90 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -702,29 +702,34 @@ static u16 temp_to_u16_adt7461(struct lm90_data *data, long val)
* Sysfs stuff
*/
-static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
- char *buf)
+static int read_temp8(struct device *dev, int index)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
struct lm90_data *data = lm90_update_device(dev);
int temp;
if (data->kind == adt7461)
- temp = temp_from_u8_adt7461(data, data->temp8[attr->index]);
+ temp = temp_from_u8_adt7461(data, data->temp8[index]);
else if (data->kind == max6646)
- temp = temp_from_u8(data->temp8[attr->index]);
+ temp = temp_from_u8(data->temp8[index]);
else
- temp = temp_from_s8(data->temp8[attr->index]);
+ temp = temp_from_s8(data->temp8[index]);
/* +16 degrees offset for temp2 for the LM99 */
- if (data->kind == lm99 && attr->index == 3)
+ if (data->kind == lm99 && index == 3)
temp += 16000;
- return sprintf(buf, "%d\n", temp);
+ return temp;
}
-static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
- const char *buf, size_t count)
+static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
+ char *buf)
+{
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ return sprintf(buf, "%d\n", read_temp8(dev, attr->index));
+}
+
+static void write_temp8(struct device *dev, int index, long val)
{
static const u8 reg[8] = {
LM90_REG_W_LOCAL_LOW,
@@ -737,60 +742,73 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
MAX6659_REG_W_REMOTE_EMERG,
};
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
struct i2c_client *client = to_i2c_client(dev);
struct lm90_data *data = i2c_get_clientdata(client);
- int nr = attr->index;
- long val;
- int err;
-
- err = kstrtol(buf, 10, &val);
- if (err < 0)
- return err;
/* +16 degrees offset for temp2 for the LM99 */
- if (data->kind == lm99 && attr->index == 3)
+ if (data->kind == lm99 && index == 3)
val -= 16000;
mutex_lock(&data->update_lock);
if (data->kind == adt7461)
- data->temp8[nr] = temp_to_u8_adt7461(data, val);
+ data->temp8[index] = temp_to_u8_adt7461(data, val);
else if (data->kind == max6646)
- data->temp8[nr] = temp_to_u8(val);
+ data->temp8[index] = temp_to_u8(val);
else
- data->temp8[nr] = temp_to_s8(val);
+ data->temp8[index] = temp_to_s8(val);
- lm90_select_remote_channel(client, data, nr >= 6);
- i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
+ lm90_select_remote_channel(client, data, index >= 6);
+ i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]);
lm90_select_remote_channel(client, data, 0);
mutex_unlock(&data->update_lock);
+}
+
+static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int index = attr->index;
+ long val;
+ int err;
+
+ err = kstrtol(buf, 10, &val);
+ if (err < 0)
+ return err;
+
+ write_temp8(dev, index, val);
+
return count;
}
-static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
- char *buf)
+static int read_temp11(struct device *dev, int index)
{
- struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
struct lm90_data *data = lm90_update_device(dev);
int temp;
if (data->kind == adt7461)
- temp = temp_from_u16_adt7461(data, data->temp11[attr->index]);
+ temp = temp_from_u16_adt7461(data, data->temp11[index]);
else if (data->kind == max6646)
- temp = temp_from_u16(data->temp11[attr->index]);
+ temp = temp_from_u16(data->temp11[index]);
else
- temp = temp_from_s16(data->temp11[attr->index]);
+ temp = temp_from_s16(data->temp11[index]);
/* +16 degrees offset for temp2 for the LM99 */
- if (data->kind == lm99 && attr->index <= 2)
+ if (data->kind == lm99 && index <= 2)
temp += 16000;
- return sprintf(buf, "%d\n", temp);
+ return temp;
}
-static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
- const char *buf, size_t count)
+static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
+ char *buf)
+{
+ struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+
+ return sprintf(buf, "%d\n", read_temp11(dev, attr->index));
+}
+
+static void write_temp11(struct device *dev, int nr, int index, long val)
{
struct {
u8 high;
@@ -804,17 +822,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
};
- struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
struct i2c_client *client = to_i2c_client(dev);
struct lm90_data *data = i2c_get_clientdata(client);
- int nr = attr->nr;
- int index = attr->index;
- long val;
- int err;
-
- err = kstrtol(buf, 10, &val);
- if (err < 0)
- return err;
/* +16 degrees offset for temp2 for the LM99 */
if (data->kind == lm99 && index <= 2)
@@ -839,6 +848,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
lm90_select_remote_channel(client, data, 0);
mutex_unlock(&data->update_lock);
+}
+
+static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+ int nr = attr->nr;
+ int index = attr->index;
+ long val;
+ int err;
+
+ err = kstrtol(buf, 10, &val);
+ if (err < 0)
+ return err;
+
+ write_temp11(dev, nr, index, val);
+
return count;
}
--
1.7.9.5
When the temperature exceed the limit range value,
the driver can handle the interrupt.
Signed-off-by: Wei Ni <[email protected]>
---
drivers/hwmon/lm90.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index c90037f..1cc3d19 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -89,6 +89,7 @@
#include <linux/err.h>
#include <linux/mutex.h>
#include <linux/sysfs.h>
+#include <linux/interrupt.h>
/*
* Addresses to scan
@@ -1460,6 +1461,17 @@ static bool lm90_is_tripped(struct i2c_client *client)
return true;
}
+static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
+{
+ struct lm90_data *data = dev_id;
+ struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
+
+ if (lm90_is_tripped(client))
+ return IRQ_HANDLED;
+ else
+ return IRQ_NONE;
+}
+
static int lm90_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -1536,6 +1548,18 @@ static int lm90_probe(struct i2c_client *client,
goto exit_remove_files;
}
+ if (client->irq >= 0) {
+ dev_dbg(dev, "lm90 IRQ: %d\n", client->irq);
+ err = devm_request_threaded_irq(dev, client->irq,
+ NULL, lm90_irq_thread,
+ IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+ "lm90", data);
+ if (err < 0) {
+ dev_err(dev, "cannot request interrupt\n");
+ goto exit_remove_files;
+ }
+ }
+
return 0;
exit_remove_files:
--
1.7.9.5
Hi Wei, Guenter,
Guenter, thanks for reviewing the previous version of this patch.
Wei, thanks for incorporating review feedback and posting updated
patches so quickly, this is very appreciated, even though I'm too busy
these days to be that fast on my end, sorry about that.
On Fri, 12 Jul 2013 15:48:04 +0800, Wei Ni wrote:
> Split set&show temp codes as common functions, so we can use it directly when
> implement linux thermal framework.
Can I see a recent version of the code which will need this change? It
makes no sense to apply this first part which makes the code more
complex with no benefit, without the second part which needs it, so
they should be applied together or not at all.
One thing I am a little worried about (but maybe I'm wrong) is that I
seem to understand you want to register every LM90-like chip as both a
hwmon device and two thermal devices. I seem to recall that every
thermal device is also exposed automatically as a virtual hwmon
device, is that correct? If so we will be presenting the same values
twice to libsensors, which would be confusing.
> Signed-off-by: Wei Ni <[email protected]>
> ---
> drivers/hwmon/lm90.c | 112 +++++++++++++++++++++++++++++++-------------------
> 1 file changed, 69 insertions(+), 43 deletions(-)
The code changes look good, however I have one suggestion for
improvement (plus a minor cleanup request):
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 8eeb141..5f30f90 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> (...)
> -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> - const char *buf, size_t count)
> (...)
> +static void write_temp8(struct device *dev, int index, long val)
> {
> static const u8 reg[8] = {
> LM90_REG_W_LOCAL_LOW,
> @@ -737,60 +742,73 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> MAX6659_REG_W_REMOTE_EMERG,
> };
>
> - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> struct i2c_client *client = to_i2c_client(dev);
> struct lm90_data *data = i2c_get_clientdata(client);
> - int nr = attr->index;
> - long val;
> - int err;
> -
> - err = kstrtol(buf, 10, &val);
> - if (err < 0)
> - return err;
>
> /* +16 degrees offset for temp2 for the LM99 */
> - if (data->kind == lm99 && attr->index == 3)
> + if (data->kind == lm99 && index == 3)
> val -= 16000;
>
> mutex_lock(&data->update_lock);
> if (data->kind == adt7461)
> - data->temp8[nr] = temp_to_u8_adt7461(data, val);
> + data->temp8[index] = temp_to_u8_adt7461(data, val);
> else if (data->kind == max6646)
> - data->temp8[nr] = temp_to_u8(val);
> + data->temp8[index] = temp_to_u8(val);
> else
> - data->temp8[nr] = temp_to_s8(val);
> + data->temp8[index] = temp_to_s8(val);
>
> - lm90_select_remote_channel(client, data, nr >= 6);
> - i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
> + lm90_select_remote_channel(client, data, index >= 6);
> + i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]);
This write could fail. So far the lm90 driver has failed to handle
register write errors at all, and I will take the blame for that. But
if we want to integrate properly with the thermal subsystem, I suspect
we will have to properly report errors. So it might be the right time
to catch and return write errors here. Then set_temp8() below could
return it to user-space (either in this patch or in a separate patch,
as you prefer.)
And then as a next step, lm90_select_remote_channel should return
errors as they happen as well, so that they can be transmitted to the
caller.
> lm90_select_remote_channel(client, data, 0);
>
> mutex_unlock(&data->update_lock);
> +}
> +
> +static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> + int index = attr->index;
> + long val;
> + int err;
> +
> + err = kstrtol(buf, 10, &val);
> + if (err < 0)
> + return err;
> +
> + write_temp8(dev, index, val);
> +
> return count;
> }
>
> -static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
> - char *buf)
> +static int read_temp11(struct device *dev, int index)
> {
> - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> struct lm90_data *data = lm90_update_device(dev);
> int temp;
>
> if (data->kind == adt7461)
> - temp = temp_from_u16_adt7461(data, data->temp11[attr->index]);
> + temp = temp_from_u16_adt7461(data, data->temp11[index]);
> else if (data->kind == max6646)
> - temp = temp_from_u16(data->temp11[attr->index]);
> + temp = temp_from_u16(data->temp11[index]);
> else
> - temp = temp_from_s16(data->temp11[attr->index]);
> + temp = temp_from_s16(data->temp11[index]);
>
> /* +16 degrees offset for temp2 for the LM99 */
> - if (data->kind == lm99 && attr->index <= 2)
> + if (data->kind == lm99 && index <= 2)
There's a doubled space on this line. It isn't added by your patch, it
was already there before, but please fix it while you're here.
> temp += 16000;
>
> - return sprintf(buf, "%d\n", temp);
> + return temp;
> }
>
> -static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> - const char *buf, size_t count)
> (...)
> +static void write_temp11(struct device *dev, int nr, int index, long val)
Here too I would suggest returning errors from the I2C layer, and
handling them in set_temp11() now or later.
> {
> struct {
> u8 high;
> @@ -804,17 +822,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
> };
>
> - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> struct i2c_client *client = to_i2c_client(dev);
> struct lm90_data *data = i2c_get_clientdata(client);
> - int nr = attr->nr;
> - int index = attr->index;
> - long val;
> - int err;
> -
> - err = kstrtol(buf, 10, &val);
> - if (err < 0)
> - return err;
>
> /* +16 degrees offset for temp2 for the LM99 */
> if (data->kind == lm99 && index <= 2)
> @@ -839,6 +848,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> lm90_select_remote_channel(client, data, 0);
>
> mutex_unlock(&data->update_lock);
> +}
> +
> +static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> + int nr = attr->nr;
> + int index = attr->index;
> + long val;
> + int err;
> +
> + err = kstrtol(buf, 10, &val);
> + if (err < 0)
> + return err;
> +
> + write_temp11(dev, nr, index, val);
> +
> return count;
> }
>
--
Jean Delvare
On Fri, Jul 12, 2013 at 03:26:15PM +0200, Jean Delvare wrote:
> Hi Wei, Guenter,
>
> Guenter, thanks for reviewing the previous version of this patch.
>
> Wei, thanks for incorporating review feedback and posting updated
> patches so quickly, this is very appreciated, even though I'm too busy
> these days to be that fast on my end, sorry about that.
>
> On Fri, 12 Jul 2013 15:48:04 +0800, Wei Ni wrote:
> > Split set&show temp codes as common functions, so we can use it directly when
> > implement linux thermal framework.
>
> Can I see a recent version of the code which will need this change? It
> makes no sense to apply this first part which makes the code more
> complex with no benefit, without the second part which needs it, so
> they should be applied together or not at all.
>
> One thing I am a little worried about (but maybe I'm wrong) is that I
> seem to understand you want to register every LM90-like chip as both a
> hwmon device and two thermal devices. I seem to recall that every
> thermal device is also exposed automatically as a virtual hwmon
> device, is that correct? If so we will be presenting the same values
> twice to libsensors, which would be confusing.
>
Not sure if that is a good idea, but if I recall correctly, the thermal folks
plan to remove that path.
Guenter
> > Signed-off-by: Wei Ni <[email protected]>
> > ---
> > drivers/hwmon/lm90.c | 112 +++++++++++++++++++++++++++++++-------------------
> > 1 file changed, 69 insertions(+), 43 deletions(-)
>
> The code changes look good, however I have one suggestion for
> improvement (plus a minor cleanup request):
>
> > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > index 8eeb141..5f30f90 100644
> > --- a/drivers/hwmon/lm90.c
> > +++ b/drivers/hwmon/lm90.c
> > (...)
> > -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> > - const char *buf, size_t count)
> > (...)
> > +static void write_temp8(struct device *dev, int index, long val)
> > {
> > static const u8 reg[8] = {
> > LM90_REG_W_LOCAL_LOW,
> > @@ -737,60 +742,73 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> > MAX6659_REG_W_REMOTE_EMERG,
> > };
> >
> > - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > struct i2c_client *client = to_i2c_client(dev);
> > struct lm90_data *data = i2c_get_clientdata(client);
> > - int nr = attr->index;
> > - long val;
> > - int err;
> > -
> > - err = kstrtol(buf, 10, &val);
> > - if (err < 0)
> > - return err;
> >
> > /* +16 degrees offset for temp2 for the LM99 */
> > - if (data->kind == lm99 && attr->index == 3)
> > + if (data->kind == lm99 && index == 3)
> > val -= 16000;
> >
> > mutex_lock(&data->update_lock);
> > if (data->kind == adt7461)
> > - data->temp8[nr] = temp_to_u8_adt7461(data, val);
> > + data->temp8[index] = temp_to_u8_adt7461(data, val);
> > else if (data->kind == max6646)
> > - data->temp8[nr] = temp_to_u8(val);
> > + data->temp8[index] = temp_to_u8(val);
> > else
> > - data->temp8[nr] = temp_to_s8(val);
> > + data->temp8[index] = temp_to_s8(val);
> >
> > - lm90_select_remote_channel(client, data, nr >= 6);
> > - i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
> > + lm90_select_remote_channel(client, data, index >= 6);
> > + i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]);
>
> This write could fail. So far the lm90 driver has failed to handle
> register write errors at all, and I will take the blame for that. But
> if we want to integrate properly with the thermal subsystem, I suspect
> we will have to properly report errors. So it might be the right time
> to catch and return write errors here. Then set_temp8() below could
> return it to user-space (either in this patch or in a separate patch,
> as you prefer.)
>
> And then as a next step, lm90_select_remote_channel should return
> errors as they happen as well, so that they can be transmitted to the
> caller.
>
> > lm90_select_remote_channel(client, data, 0);
> >
> > mutex_unlock(&data->update_lock);
> > +}
> > +
> > +static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> > + const char *buf, size_t count)
> > +{
> > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > + int index = attr->index;
> > + long val;
> > + int err;
> > +
> > + err = kstrtol(buf, 10, &val);
> > + if (err < 0)
> > + return err;
> > +
> > + write_temp8(dev, index, val);
> > +
> > return count;
> > }
> >
> > -static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
> > - char *buf)
> > +static int read_temp11(struct device *dev, int index)
> > {
> > - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> > struct lm90_data *data = lm90_update_device(dev);
> > int temp;
> >
> > if (data->kind == adt7461)
> > - temp = temp_from_u16_adt7461(data, data->temp11[attr->index]);
> > + temp = temp_from_u16_adt7461(data, data->temp11[index]);
> > else if (data->kind == max6646)
> > - temp = temp_from_u16(data->temp11[attr->index]);
> > + temp = temp_from_u16(data->temp11[index]);
> > else
> > - temp = temp_from_s16(data->temp11[attr->index]);
> > + temp = temp_from_s16(data->temp11[index]);
> >
> > /* +16 degrees offset for temp2 for the LM99 */
> > - if (data->kind == lm99 && attr->index <= 2)
> > + if (data->kind == lm99 && index <= 2)
>
> There's a doubled space on this line. It isn't added by your patch, it
> was already there before, but please fix it while you're here.
>
> > temp += 16000;
> >
> > - return sprintf(buf, "%d\n", temp);
> > + return temp;
> > }
> >
> > -static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> > - const char *buf, size_t count)
> > (...)
> > +static void write_temp11(struct device *dev, int nr, int index, long val)
>
> Here too I would suggest returning errors from the I2C layer, and
> handling them in set_temp11() now or later.
>
> > {
> > struct {
> > u8 high;
> > @@ -804,17 +822,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> > { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
> > };
> >
> > - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> > struct i2c_client *client = to_i2c_client(dev);
> > struct lm90_data *data = i2c_get_clientdata(client);
> > - int nr = attr->nr;
> > - int index = attr->index;
> > - long val;
> > - int err;
> > -
> > - err = kstrtol(buf, 10, &val);
> > - if (err < 0)
> > - return err;
> >
> > /* +16 degrees offset for temp2 for the LM99 */
> > if (data->kind == lm99 && index <= 2)
> > @@ -839,6 +848,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> > lm90_select_remote_channel(client, data, 0);
> >
> > mutex_unlock(&data->update_lock);
> > +}
> > +
> > +static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> > + const char *buf, size_t count)
> > +{
> > + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> > + int nr = attr->nr;
> > + int index = attr->index;
> > + long val;
> > + int err;
> > +
> > + err = kstrtol(buf, 10, &val);
> > + if (err < 0)
> > + return err;
> > +
> > + write_temp11(dev, nr, index, val);
> > +
> > return count;
> > }
> >
>
> --
> Jean Delvare
>
Hi Guenter,
On Fri, 12 Jul 2013 06:50:00 -0700, Guenter Roeck wrote:
> On Fri, Jul 12, 2013 at 03:26:15PM +0200, Jean Delvare wrote:
> > One thing I am a little worried about (but maybe I'm wrong) is that I
> > seem to understand you want to register every LM90-like chip as both a
> > hwmon device and two thermal devices. I seem to recall that every
> > thermal device is also exposed automatically as a virtual hwmon
> > device, is that correct? If so we will be presenting the same values
> > twice to libsensors, which would be confusing.
>
> Not sure if that is a good idea, but if I recall correctly, the thermal folks
> plan to remove that path.
If that means that for example the ACPI thermal zone is no longer
displayed by "sensors", then I strongly object - unless it is
explicitly registered as a separate hwmon device from now on, of course.
My idea was to make the bridge optional - you decide when you register
a thermal device if it should be exposed as hwmon or not.
I don't have a strong opinion on the implementation, as long as each
input is listed by "sensors" once and only once.
--
Jean Delvare
On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
> Hi Guenter,
>
> On Fri, 12 Jul 2013 06:50:00 -0700, Guenter Roeck wrote:
> > On Fri, Jul 12, 2013 at 03:26:15PM +0200, Jean Delvare wrote:
> > > One thing I am a little worried about (but maybe I'm wrong) is that I
> > > seem to understand you want to register every LM90-like chip as both a
> > > hwmon device and two thermal devices. I seem to recall that every
> > > thermal device is also exposed automatically as a virtual hwmon
> > > device, is that correct? If so we will be presenting the same values
> > > twice to libsensors, which would be confusing.
> >
> > Not sure if that is a good idea, but if I recall correctly, the thermal folks
> > plan to remove that path.
>
> If that means that for example the ACPI thermal zone is no longer
> displayed by "sensors", then I strongly object - unless it is
> explicitly registered as a separate hwmon device from now on, of course.
>
If I recall correctly that was the idea. Of course, in practice that will mean
that devices will _not_ get exposed as hwmon devices, as implementers won't
bother doing both.
> My idea was to make the bridge optional - you decide when you register
> a thermal device if it should be exposed as hwmon or not.
>
Yes, that would be a much better solution.
> I don't have a strong opinion on the implementation, as long as each
> input is listed by "sensors" once and only once.
>
Agreed.
Guenter
On 07/12/2013 09:26 PM, Jean Delvare wrote:
> Hi Wei, Guenter,
>
> Guenter, thanks for reviewing the previous version of this patch.
>
> Wei, thanks for incorporating review feedback and posting updated
> patches so quickly, this is very appreciated, even though I'm too busy
> these days to be that fast on my end, sorry about that.
>
> On Fri, 12 Jul 2013 15:48:04 +0800, Wei Ni wrote:
>> Split set&show temp codes as common functions, so we can use it directly when
>> implement linux thermal framework.
>
> Can I see a recent version of the code which will need this change? It
> makes no sense to apply this first part which makes the code more
> complex with no benefit, without the second part which needs it, so
> they should be applied together or not at all.
In my RFC patches, there had many codes about thermal fw, which need
this patch, so I put them together.
And now I split the RFC patches, this series is preparing to use the
thermal fw.
As you said, I want to register lm90 as the thermal zone device, it need
to hook some callback, such as .get_temp. if apply this patch, I can
write the .get_temp simply, something like:
+static int lm90_read_temp2_temp(struct thermal_zone_device *thz,
unsigned long *temp)
+{
+ struct lm90_data *data = thz->devdata;
+ struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
+ struct device *dev = &client->dev;+
+
+ *temp = (long)read_temp11(dev, TEMP11_REMOTE_TEMP);
+
+ return 0;
+}
+
+static struct thermal_zone_device_ops remote_ops = {
+ .get_temp = lm90_read_temp2_temp,
+};
If without this patch, I have to rewrite the lm90_read_temp2_temp(),
which almost same as the show_temp11(), I think it's not good. When use
this patch and following 3/3 patch, the code will be more readable and
clear.
Anyway, if you want, I can send this patch as a separate one. :)
>
> One thing I am a little worried about (but maybe I'm wrong) is that I
> seem to understand you want to register every LM90-like chip as both a
> hwmon device and two thermal devices. I seem to recall that every
> thermal device is also exposed automatically as a virtual hwmon
> device, is that correct? If so we will be presenting the same values
> twice to libsensors, which would be confusing.
>
>> Signed-off-by: Wei Ni <[email protected]>
>> ---
>> drivers/hwmon/lm90.c | 112 +++++++++++++++++++++++++++++++-------------------
>> 1 file changed, 69 insertions(+), 43 deletions(-)
>
> The code changes look good, however I have one suggestion for
> improvement (plus a minor cleanup request):
>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index 8eeb141..5f30f90 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> (...)
>> -static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
>> - const char *buf, size_t count)
>> (...)
>> +static void write_temp8(struct device *dev, int index, long val)
>> {
>> static const u8 reg[8] = {
>> LM90_REG_W_LOCAL_LOW,
>> @@ -737,60 +742,73 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
>> MAX6659_REG_W_REMOTE_EMERG,
>> };
>>
>> - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> struct i2c_client *client = to_i2c_client(dev);
>> struct lm90_data *data = i2c_get_clientdata(client);
>> - int nr = attr->index;
>> - long val;
>> - int err;
>> -
>> - err = kstrtol(buf, 10, &val);
>> - if (err < 0)
>> - return err;
>>
>> /* +16 degrees offset for temp2 for the LM99 */
>> - if (data->kind == lm99 && attr->index == 3)
>> + if (data->kind == lm99 && index == 3)
>> val -= 16000;
>>
>> mutex_lock(&data->update_lock);
>> if (data->kind == adt7461)
>> - data->temp8[nr] = temp_to_u8_adt7461(data, val);
>> + data->temp8[index] = temp_to_u8_adt7461(data, val);
>> else if (data->kind == max6646)
>> - data->temp8[nr] = temp_to_u8(val);
>> + data->temp8[index] = temp_to_u8(val);
>> else
>> - data->temp8[nr] = temp_to_s8(val);
>> + data->temp8[index] = temp_to_s8(val);
>>
>> - lm90_select_remote_channel(client, data, nr >= 6);
>> - i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
>> + lm90_select_remote_channel(client, data, index >= 6);
>> + i2c_smbus_write_byte_data(client, reg[index], data->temp8[index]);
>
> This write could fail. So far the lm90 driver has failed to handle
> register write errors at all, and I will take the blame for that. But
> if we want to integrate properly with the thermal subsystem, I suspect
> we will have to properly report errors. So it might be the right time
> to catch and return write errors here. Then set_temp8() below could
> return it to user-space (either in this patch or in a separate patch,
> as you prefer.)
Ok, I will add error handler in my next version.
>
> And then as a next step, lm90_select_remote_channel should return
> errors as they happen as well, so that they can be transmitted to the
> caller.
>
>> lm90_select_remote_channel(client, data, 0);
>>
>> mutex_unlock(&data->update_lock);
>> +}
>> +
>> +static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
>> + const char *buf, size_t count)
>> +{
>> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> + int index = attr->index;
>> + long val;
>> + int err;
>> +
>> + err = kstrtol(buf, 10, &val);
>> + if (err < 0)
>> + return err;
>> +
>> + write_temp8(dev, index, val);
>> +
>> return count;
>> }
>>
>> -static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
>> - char *buf)
>> +static int read_temp11(struct device *dev, int index)
>> {
>> - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
>> struct lm90_data *data = lm90_update_device(dev);
>> int temp;
>>
>> if (data->kind == adt7461)
>> - temp = temp_from_u16_adt7461(data, data->temp11[attr->index]);
>> + temp = temp_from_u16_adt7461(data, data->temp11[index]);
>> else if (data->kind == max6646)
>> - temp = temp_from_u16(data->temp11[attr->index]);
>> + temp = temp_from_u16(data->temp11[index]);
>> else
>> - temp = temp_from_s16(data->temp11[attr->index]);
>> + temp = temp_from_s16(data->temp11[index]);
>>
>> /* +16 degrees offset for temp2 for the LM99 */
>> - if (data->kind == lm99 && attr->index <= 2)
>> + if (data->kind == lm99 && index <= 2)
>
> There's a doubled space on this line. It isn't added by your patch, it
> was already there before, but please fix it while you're here.
Oh, you are so carefully, I will fix it :)
>
>> temp += 16000;
>>
>> - return sprintf(buf, "%d\n", temp);
>> + return temp;
>> }
>>
>> -static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>> - const char *buf, size_t count)
>> (...)
>> +static void write_temp11(struct device *dev, int nr, int index, long val)
>
> Here too I would suggest returning errors from the I2C layer, and
> handling them in set_temp11() now or later.
>
>> {
>> struct {
>> u8 high;
>> @@ -804,17 +822,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>> { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
>> };
>>
>> - struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
>> struct i2c_client *client = to_i2c_client(dev);
>> struct lm90_data *data = i2c_get_clientdata(client);
>> - int nr = attr->nr;
>> - int index = attr->index;
>> - long val;
>> - int err;
>> -
>> - err = kstrtol(buf, 10, &val);
>> - if (err < 0)
>> - return err;
>>
>> /* +16 degrees offset for temp2 for the LM99 */
>> if (data->kind == lm99 && index <= 2)
>> @@ -839,6 +848,23 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>> lm90_select_remote_channel(client, data, 0);
>>
>> mutex_unlock(&data->update_lock);
>> +}
>> +
>> +static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>> + const char *buf, size_t count)
>> +{
>> + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
>> + int nr = attr->nr;
>> + int index = attr->index;
>> + long val;
>> + int err;
>> +
>> + err = kstrtol(buf, 10, &val);
>> + if (err < 0)
>> + return err;
>> +
>> + write_temp11(dev, nr, index, val);
>> +
>> return count;
>> }
>>
>
On 07/12/2013 10:40 PM, Guenter Roeck wrote:
> On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
>> Hi Guenter,
>>
>> On Fri, 12 Jul 2013 06:50:00 -0700, Guenter Roeck wrote:
>>> On Fri, Jul 12, 2013 at 03:26:15PM +0200, Jean Delvare wrote:
>>>> One thing I am a little worried about (but maybe I'm wrong) is that I
>>>> seem to understand you want to register every LM90-like chip as both a
>>>> hwmon device and two thermal devices. I seem to recall that every
>>>> thermal device is also exposed automatically as a virtual hwmon
>>>> device, is that correct? If so we will be presenting the same values
>>>> twice to libsensors, which would be confusing.
>>>
>>> Not sure if that is a good idea, but if I recall correctly, the thermal folks
>>> plan to remove that path.
Hi, Rui
As Jean said, if we want to register the lm90 as thermal device, it will
have two hwmon devices in the sysfs, one is registered by the lm90
driver, another one is registered by the thermal_zone_device_register(),
this would be confusing.
Do you have any ideas for it?
Thanks.
Wei.
>>
>> If that means that for example the ACPI thermal zone is no longer
>> displayed by "sensors", then I strongly object - unless it is
>> explicitly registered as a separate hwmon device from now on, of course.
>>
> If I recall correctly that was the idea. Of course, in practice that will mean
> that devices will _not_ get exposed as hwmon devices, as implementers won't
> bother doing both.
>
>> My idea was to make the bridge optional - you decide when you register
>> a thermal device if it should be exposed as hwmon or not.
>>
> Yes, that would be a much better solution.
I think we can decide it in the DT, we can add a dt property in the lm90
device node, such as:
sys-interface = SYS_HWMON;
or
sys-interface = SYS_THERMAL;
So we register it as the hwmon or thermal device
>
>> I don't have a strong opinion on the implementation, as long as each
>> input is listed by "sensors" once and only once.
>>
> Agreed.
>
> Guenter
>
On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
> On 07/12/2013 10:40 PM, Guenter Roeck wrote:
> > On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
> >> If that means that for example the ACPI thermal zone is no longer
> >> displayed by "sensors", then I strongly object - unless it is
> >> explicitly registered as a separate hwmon device from now on, of course.
> >
> > If I recall correctly that was the idea. Of course, in practice that will mean
> > that devices will _not_ get exposed as hwmon devices, as implementers won't
> > bother doing both.
> >
> >> My idea was to make the bridge optional - you decide when you register
> >> a thermal device if it should be exposed as hwmon or not.
> >
> > Yes, that would be a much better solution.
>
> I think we can decide it in the DT, we can add a dt property in the lm90
> device node, such as:
> sys-interface = SYS_HWMON;
> or
> sys-interface = SYS_THERMAL;
> So we register it as the hwmon or thermal device
This is an option, but please keep in mind that DT is not the only way
to instantiate LM90-like devices, and we should not expose duplicate
inputs by default. It is fine with me if the default is to create only a
HWMON device (as the lm90 driver was doing so far), and only
DT-instantiated devices have the choice.
Another option, as discussed before, would be that the thermal devices
registered by lm90 are specifically tagged as "do not expose as hwmon".
This would avoid the duplicate hwmon inputs in all cases.
Again - no strong opinion on the implementation, as long as it does the
right thing.
Oh, and we'll have to be careful with the Kconfig dependencies. I do
not want the lm90 driver to depend on the thermal framework.
--
Jean Delvare
Hi Wei,
On Mon, 15 Jul 2013 14:05:08 +0800, Wei Ni wrote:
> On 07/12/2013 09:26 PM, Jean Delvare wrote:
> > Can I see a recent version of the code which will need this change? It
> > makes no sense to apply this first part which makes the code more
> > complex with no benefit, without the second part which needs it, so
> > they should be applied together or not at all.
>
> In my RFC patches, there had many codes about thermal fw, which need
> this patch, so I put them together.
> And now I split the RFC patches, this series is preparing to use the
> thermal fw.
> As you said, I want to register lm90 as the thermal zone device, it need
> to hook some callback, such as .get_temp. if apply this patch, I can
> write the .get_temp simply, something like:
>
> +static int lm90_read_temp2_temp(struct thermal_zone_device *thz,
> unsigned long *temp)
> +{
> + struct lm90_data *data = thz->devdata;
> + struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
> + struct device *dev = &client->dev;+
> +
> + *temp = (long)read_temp11(dev, TEMP11_REMOTE_TEMP);
> +
> + return 0;
> +}
> +
> +static struct thermal_zone_device_ops remote_ops = {
> + .get_temp = lm90_read_temp2_temp,
> +};
>
> If without this patch, I have to rewrite the lm90_read_temp2_temp(),
> which almost same as the show_temp11(), I think it's not good. When use
> this patch and following 3/3 patch, the code will be more readable and
> clear.
I understand the idea.
> Anyway, if you want, I can send this patch as a separate one. :)
Yes please, I think it would help me do a better code review and
testing as well.
--
Jean Delvare
On 07/15/2013 03:24 PM, Jean Delvare wrote:
> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
>> On 07/12/2013 10:40 PM, Guenter Roeck wrote:
>>> On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
>>>> If that means that for example the ACPI thermal zone is no longer
>>>> displayed by "sensors", then I strongly object - unless it is
>>>> explicitly registered as a separate hwmon device from now on, of course.
>>>
>>> If I recall correctly that was the idea. Of course, in practice that will mean
>>> that devices will _not_ get exposed as hwmon devices, as implementers won't
>>> bother doing both.
>>>
>>>> My idea was to make the bridge optional - you decide when you register
>>>> a thermal device if it should be exposed as hwmon or not.
>>>
>>> Yes, that would be a much better solution.
>>
>> I think we can decide it in the DT, we can add a dt property in the lm90
>> device node, such as:
>> sys-interface = SYS_HWMON;
>> or
>> sys-interface = SYS_THERMAL;
>> So we register it as the hwmon or thermal device
>
> This is an option, but please keep in mind that DT is not the only way
> to instantiate LM90-like devices, and we should not expose duplicate
> inputs by default. It is fine with me if the default is to create only a
> HWMON device (as the lm90 driver was doing so far), and only
> DT-instantiated devices have the choice.
Yes, we should not expose duplicate inputs, we may have tree permutation:
1. only hwmon device:
for this items, we just need to call hwmon_device_register().
2. only thermal device + virtual hwmon device:
for this items, we just need to call thermal_zone_device_register().
We can set #1 as the default, and if use DT, we provide option to choice
#1 or #2.
3. hwmon device + thermal device:
for this items, we doesn't need the virtual hwmon which registered by
the thermal fw, how to handle this one? Add flag when register as
thermal device to indicate if want virtual hwmon device or not,
something like your below another option.
>
> Another option, as discussed before, would be that the thermal devices
> registered by lm90 are specifically tagged as "do not expose as hwmon".
> This would avoid the duplicate hwmon inputs in all cases.
It seems in current thermal fw, it can't be tagged as "do not expose as
hwmon", we need to add flag when register thermal device.
Rui, what do you think for it?
>
> Again - no strong opinion on the implementation, as long as it does the
> right thing.
I'm working on the Tegra platform, we uses nct1008 as the temperature
sensor, and we want to register it as thermal device, so that we can run
the throttle function. So I prepared these patches to enhance this driver.
>
> Oh, and we'll have to be careful with the Kconfig dependencies. I do
> not want the lm90 driver to depend on the thermal framework.
Yes, absolutely agree, lm90 should be independent.
Indeed, the thermal folks is trying to restructure the thermal fw, and
in this new fw, it's more easy to add the generic sensors to the thermal
fw. If you interest it, please refer
http://thread.gmane.org/gmane.linux.power-management.general/30692.
Thanks.
Wei.
>
Hi Wei, Guenter,
On Fri, 12 Jul 2013 15:48:05 +0800, Wei Ni wrote:
> Add bit defines for the status register.
Regarding the subject: for me these are constants, not macros. AFAIK
the term "macro" refers to defines with parameters only.
> Signed-off-by: Wei Ni <[email protected]>
> ---
> drivers/hwmon/lm90.c | 72 ++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 49 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 5f30f90..c90037f 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -179,6 +179,19 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
> #define LM90_HAVE_TEMP3 (1 << 6) /* 3rd temperature sensor */
> #define LM90_HAVE_BROKEN_ALERT (1 << 7) /* Broken alert */
>
> +/* LM90 status */
> +#define LM90_STATUS_LTHRM (1 << 0) /* local THERM limit tripped */
> +#define LM90_STATUS_RTHRM (1 << 1) /* remote THERM limit tripped */
> +#define LM90_STATUS_OPEN (1 << 2) /* remote is an open circuit */
> +#define LM90_STATUS_RLOW (1 << 3) /* remote low temp limit tripped */
> +#define LM90_STATUS_RHIGH (1 << 4) /* remote high temp limit tripped */
> +#define LM90_STATUS_LLOW (1 << 5) /* local low temp limit tripped */
> +#define LM90_STATUS_LHIGH (1 << 6) /* local high temp limit tripped */
> +#define LM90_STATUS_BUSY (1 << 7) /* ADC is converting */
LM90_STATUS_BUSY is never used anywhere so please don't define it.
> +
> +#define MAX6696_STATUS2_RLOW (1 << 3) /* remote2 low temp limit tripped */
> +#define MAX6696_STATUS2_RHIGH (1 << 4) /* remote2 high temp limit tripped */
> +
> /*
> * Driver data (common to all clients)
> */
> @@ -1417,6 +1430,36 @@ static void lm90_init_client(struct i2c_client *client)
> i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
> }
>
> +static bool lm90_is_tripped(struct i2c_client *client)
> +{
> + struct lm90_data *data = i2c_get_clientdata(client);
> + u8 status, status2 = 0;
> +
> + lm90_read_reg(client, LM90_REG_R_STATUS, &status);
> +
> + if (data->kind == max6696)
> + lm90_read_reg(client, MAX6696_REG_R_STATUS2, &status2);
> +
> + if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0)
> + return false;
It's a bit disappointing to not use the freshly introduced constants.
That being said I agree it would make the code hard to read, so you can
leave it as is.
Unrelated to this patch, but Guenter, I am worried about the MAX6696
handling here. I realize that I am the one who accepted your code, but
now it looks wrong. Specifically:
* We check for (status2 & 0xfe) i.e. 7 alarm bits, but the code below
only reports 2 alarms bits. So if any of the 5 other alarm bits in
STATUS2 are, we may return true (chip is tripped) but not print the
cause.
* At least bits 1 and 2 of STATUS 2 fit totally fine in the driver as
it currently exists, so I can't think of any reason for not handling
them. Why are we not? Ideally we should print a message for every
alarm bit so that we never return "true" without printing a message.
Even though OT2 limits aren't handled by the driver...
* If you think this piece of code shouldn't deal with OT/THERM limits
because they do not trigger an SMBus alarm, this can be discussed,
but all chips should be handled the same in this respect then.
* Why in the first place is max6696's data->alert_alarms set to 0x187c
and not 0x1c7c? Including 1OPEN but not 2OPEN makes no sense.
> +
> + if (status & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH | LM90_STATUS_LTHRM))
> + dev_warn(&client->dev,
> + "temp%d out of range, please check!\n", 1);
> + if (status & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH | LM90_STATUS_RTHRM))
> + dev_warn(&client->dev,
> + "temp%d out of range, please check!\n", 2);
> + if (status & LM90_STATUS_OPEN)
> + dev_warn(&client->dev,
> + "temp%d diode open, please check!\n", 2);
> +
> + if (status2 & (MAX6696_STATUS2_RLOW | MAX6696_STATUS2_RHIGH))
> + dev_warn(&client->dev,
> + "temp%d out of range, please check!\n", 3);
> +
> + return true;
> +}
> +
> static int lm90_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -1515,36 +1558,19 @@ 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, alarms2 = 0;
> -
> - lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> -
> - if (data->kind == max6696)
> - lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms2);
> -
> - if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) {
> + if (!lm90_is_tripped(client)) {
You could swap the success and failure cases to avoid this negation.
> dev_info(&client->dev, "Everything OK\n");
> } else {
> - if (alarms & 0x61)
> - dev_warn(&client->dev,
> - "temp%d out of range, please check!\n", 1);
> - if (alarms & 0x1a)
> - dev_warn(&client->dev,
> - "temp%d out of range, please check!\n", 2);
> - if (alarms & 0x04)
> - 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.
> */
> + struct lm90_data *data = i2c_get_clientdata(client);
> + u8 config, alarms;
> +
> + lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
You end up reading LM90_REG_R_STATUS, which is not OK. This register
contains self-clearing bits, so there is no guarantee that the second
read will return the same value as the first read. You'll have to come
up with a different approach that reads LM90_REG_R_STATUS only once.
> +
> if ((data->flags & LM90_HAVE_BROKEN_ALERT)
> && (alarms & data->alert_alarms)) {
> dev_dbg(&client->dev, "Disabling ALERT#\n");
--
Jean Delvare
On Mon, Jul 15, 2013 at 06:57:27PM +0200, Jean Delvare wrote:
> Hi Wei, Guenter,
>
> On Fri, 12 Jul 2013 15:48:05 +0800, Wei Ni wrote:
> > Add bit defines for the status register.
>
> Regarding the subject: for me these are constants, not macros. AFAIK
> the term "macro" refers to defines with parameters only.
>
> > Signed-off-by: Wei Ni <[email protected]>
> > ---
> > drivers/hwmon/lm90.c | 72 ++++++++++++++++++++++++++++++++++----------------
> > 1 file changed, 49 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > index 5f30f90..c90037f 100644
> > --- a/drivers/hwmon/lm90.c
> > +++ b/drivers/hwmon/lm90.c
> > @@ -179,6 +179,19 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
> > #define LM90_HAVE_TEMP3 (1 << 6) /* 3rd temperature sensor */
> > #define LM90_HAVE_BROKEN_ALERT (1 << 7) /* Broken alert */
> >
> > +/* LM90 status */
> > +#define LM90_STATUS_LTHRM (1 << 0) /* local THERM limit tripped */
> > +#define LM90_STATUS_RTHRM (1 << 1) /* remote THERM limit tripped */
> > +#define LM90_STATUS_OPEN (1 << 2) /* remote is an open circuit */
> > +#define LM90_STATUS_RLOW (1 << 3) /* remote low temp limit tripped */
> > +#define LM90_STATUS_RHIGH (1 << 4) /* remote high temp limit tripped */
> > +#define LM90_STATUS_LLOW (1 << 5) /* local low temp limit tripped */
> > +#define LM90_STATUS_LHIGH (1 << 6) /* local high temp limit tripped */
> > +#define LM90_STATUS_BUSY (1 << 7) /* ADC is converting */
>
> LM90_STATUS_BUSY is never used anywhere so please don't define it.
>
> > +
> > +#define MAX6696_STATUS2_RLOW (1 << 3) /* remote2 low temp limit tripped */
> > +#define MAX6696_STATUS2_RHIGH (1 << 4) /* remote2 high temp limit tripped */
> > +
> > /*
> > * Driver data (common to all clients)
> > */
> > @@ -1417,6 +1430,36 @@ static void lm90_init_client(struct i2c_client *client)
> > i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
> > }
> >
> > +static bool lm90_is_tripped(struct i2c_client *client)
> > +{
> > + struct lm90_data *data = i2c_get_clientdata(client);
> > + u8 status, status2 = 0;
> > +
> > + lm90_read_reg(client, LM90_REG_R_STATUS, &status);
> > +
> > + if (data->kind == max6696)
> > + lm90_read_reg(client, MAX6696_REG_R_STATUS2, &status2);
> > +
> > + if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0)
> > + return false;
>
> It's a bit disappointing to not use the freshly introduced constants.
> That being said I agree it would make the code hard to read, so you can
> leave it as is.
>
> Unrelated to this patch, but Guenter, I am worried about the MAX6696
> handling here. I realize that I am the one who accepted your code, but
> now it looks wrong. Specifically:
> * We check for (status2 & 0xfe) i.e. 7 alarm bits, but the code below
> only reports 2 alarms bits. So if any of the 5 other alarm bits in
> STATUS2 are, we may return true (chip is tripped) but not print the
> cause.
> * At least bits 1 and 2 of STATUS 2 fit totally fine in the driver as
> it currently exists, so I can't think of any reason for not handling
> them. Why are we not? Ideally we should print a message for every
> alarm bit so that we never return "true" without printing a message.
> Even though OT2 limits aren't handled by the driver...
> * If you think this piece of code shouldn't deal with OT/THERM limits
> because they do not trigger an SMBus alarm, this can be discussed,
> but all chips should be handled the same in this respect then.
> * Why in the first place is max6696's data->alert_alarms set to 0x187c
> and not 0x1c7c? Including 1OPEN but not 2OPEN makes no sense.
>
I am about to leave for vacation, so this will have to wait for a couple of
weeks. I'll look at it after I am back.
Guenter
> > +
> > + if (status & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH | LM90_STATUS_LTHRM))
> > + dev_warn(&client->dev,
> > + "temp%d out of range, please check!\n", 1);
> > + if (status & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH | LM90_STATUS_RTHRM))
> > + dev_warn(&client->dev,
> > + "temp%d out of range, please check!\n", 2);
> > + if (status & LM90_STATUS_OPEN)
> > + dev_warn(&client->dev,
> > + "temp%d diode open, please check!\n", 2);
> > +
> > + if (status2 & (MAX6696_STATUS2_RLOW | MAX6696_STATUS2_RHIGH))
> > + dev_warn(&client->dev,
> > + "temp%d out of range, please check!\n", 3);
> > +
> > + return true;
> > +}
> > +
> > static int lm90_probe(struct i2c_client *client,
> > const struct i2c_device_id *id)
> > {
> > @@ -1515,36 +1558,19 @@ 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, alarms2 = 0;
> > -
> > - lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> > -
> > - if (data->kind == max6696)
> > - lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms2);
> > -
> > - if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) {
> > + if (!lm90_is_tripped(client)) {
>
> You could swap the success and failure cases to avoid this negation.
>
> > dev_info(&client->dev, "Everything OK\n");
> > } else {
> > - if (alarms & 0x61)
> > - dev_warn(&client->dev,
> > - "temp%d out of range, please check!\n", 1);
> > - if (alarms & 0x1a)
> > - dev_warn(&client->dev,
> > - "temp%d out of range, please check!\n", 2);
> > - if (alarms & 0x04)
> > - 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.
> > */
> > + struct lm90_data *data = i2c_get_clientdata(client);
> > + u8 config, alarms;
> > +
> > + lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
>
> You end up reading LM90_REG_R_STATUS, which is not OK. This register
> contains self-clearing bits, so there is no guarantee that the second
> read will return the same value as the first read. You'll have to come
> up with a different approach that reads LM90_REG_R_STATUS only once.
>
> > +
> > if ((data->flags & LM90_HAVE_BROKEN_ALERT)
> > && (alarms & data->alert_alarms)) {
> > dev_dbg(&client->dev, "Disabling ALERT#\n");
>
>
> --
> Jean Delvare
>
Hi Wei,
On Mon, 15 Jul 2013 17:14:07 +0800, Wei Ni wrote:
> On 07/15/2013 03:24 PM, Jean Delvare wrote:
> > On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
> >> I think we can decide it in the DT, we can add a dt property in the lm90
> >> device node, such as:
> >> sys-interface = SYS_HWMON;
> >> or
> >> sys-interface = SYS_THERMAL;
> >> So we register it as the hwmon or thermal device
> >
> > This is an option, but please keep in mind that DT is not the only way
> > to instantiate LM90-like devices, and we should not expose duplicate
> > inputs by default. It is fine with me if the default is to create only a
> > HWMON device (as the lm90 driver was doing so far), and only
> > DT-instantiated devices have the choice.
>
> Yes, we should not expose duplicate inputs, we may have tree permutation:
> 1. only hwmon device:
> for this items, we just need to call hwmon_device_register().
> 2. only thermal device + virtual hwmon device:
> for this items, we just need to call thermal_zone_device_register().
>
> We can set #1 as the default, and if use DT, we provide option to choice
> #1 or #2.
#2 makes little sense IMHO, for a driver which properly implements
hwmon support. The point of the virtual hwmon device created for
thermal zones was to not put an extra burden on thermal driver authors
by asking them to additionally implement the hwmon interface. But the
hwmon interface it richer than the thermal interface in some respects
so native hwmon implementations are preferred when available. Thus I
think your option #3 below is what we want in addition to #1, and we
don't need #2.
> 3. hwmon device + thermal device:
> for this items, we doesn't need the virtual hwmon which registered by
> the thermal fw, how to handle this one? Add flag when register as
> thermal device to indicate if want virtual hwmon device or not,
> something like your below another option.
--
Jean Delvare
On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote:
> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
> > On 07/12/2013 10:40 PM, Guenter Roeck wrote:
> > > On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
> > >> If that means that for example the ACPI thermal zone is no longer
> > >> displayed by "sensors", then I strongly object - unless it is
> > >> explicitly registered as a separate hwmon device from now on, of course.
> > >
> > > If I recall correctly that was the idea. Of course, in practice that will mean
> > > that devices will _not_ get exposed as hwmon devices, as implementers won't
> > > bother doing both.
> > >
> > >> My idea was to make the bridge optional - you decide when you register
> > >> a thermal device if it should be exposed as hwmon or not.
> > >
> > > Yes, that would be a much better solution.
> >
> > I think we can decide it in the DT, we can add a dt property in the lm90
> > device node, such as:
> > sys-interface = SYS_HWMON;
> > or
> > sys-interface = SYS_THERMAL;
> > So we register it as the hwmon or thermal device
>
> This is an option, but please keep in mind that DT is not the only way
> to instantiate LM90-like devices, and we should not expose duplicate
> inputs by default. It is fine with me if the default is to create only a
> HWMON device (as the lm90 driver was doing so far), and only
> DT-instantiated devices have the choice.
I don't think this information belongs in the device tree. Whether the
device is exposed as hwmon or thermal device (or both) is entirely a
software issue whereas DT is a means to describe the hardware.
It seems to me that the earlier proposal of communicating to the bridge
whether or not a device should be exposed as hwmon device is the right
thing to do here.
Thierry
On Wed, Jul 17, 2013 at 06:26:20AM +0200, Thierry Reding wrote:
> On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote:
> > On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
> > > On 07/12/2013 10:40 PM, Guenter Roeck wrote:
> > > > On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
> > > >> If that means that for example the ACPI thermal zone is no longer
> > > >> displayed by "sensors", then I strongly object - unless it is
> > > >> explicitly registered as a separate hwmon device from now on, of course.
> > > >
> > > > If I recall correctly that was the idea. Of course, in practice that will mean
> > > > that devices will _not_ get exposed as hwmon devices, as implementers won't
> > > > bother doing both.
> > > >
> > > >> My idea was to make the bridge optional - you decide when you register
> > > >> a thermal device if it should be exposed as hwmon or not.
> > > >
> > > > Yes, that would be a much better solution.
> > >
> > > I think we can decide it in the DT, we can add a dt property in the lm90
> > > device node, such as:
> > > sys-interface = SYS_HWMON;
> > > or
> > > sys-interface = SYS_THERMAL;
> > > So we register it as the hwmon or thermal device
> >
> > This is an option, but please keep in mind that DT is not the only way
> > to instantiate LM90-like devices, and we should not expose duplicate
> > inputs by default. It is fine with me if the default is to create only a
> > HWMON device (as the lm90 driver was doing so far), and only
> > DT-instantiated devices have the choice.
>
> I don't think this information belongs in the device tree. Whether the
> device is exposed as hwmon or thermal device (or both) is entirely a
> software issue whereas DT is a means to describe the hardware.
>
Correct; this is exactly the type of information which does _not_ belong int
devicetree.
> It seems to me that the earlier proposal of communicating to the bridge
> whether or not a device should be exposed as hwmon device is the right
> thing to do here.
>
Agreed..
Guenter
On 07/17/2013 01:14 PM, Guenter Roeck wrote:
> On Wed, Jul 17, 2013 at 06:26:20AM +0200, Thierry Reding wrote:
>> On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote:
>>> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
>>>> On 07/12/2013 10:40 PM, Guenter Roeck wrote:
>>>>> On Fri, Jul 12, 2013 at 04:30:34PM +0200, Jean Delvare wrote:
>>>>>> If that means that for example the ACPI thermal zone is no longer
>>>>>> displayed by "sensors", then I strongly object - unless it is
>>>>>> explicitly registered as a separate hwmon device from now on, of course.
>>>>>
>>>>> If I recall correctly that was the idea. Of course, in practice that will mean
>>>>> that devices will _not_ get exposed as hwmon devices, as implementers won't
>>>>> bother doing both.
>>>>>
>>>>>> My idea was to make the bridge optional - you decide when you register
>>>>>> a thermal device if it should be exposed as hwmon or not.
>>>>>
>>>>> Yes, that would be a much better solution.
>>>>
>>>> I think we can decide it in the DT, we can add a dt property in the lm90
>>>> device node, such as:
>>>> sys-interface = SYS_HWMON;
>>>> or
>>>> sys-interface = SYS_THERMAL;
>>>> So we register it as the hwmon or thermal device
>>>
>>> This is an option, but please keep in mind that DT is not the only way
>>> to instantiate LM90-like devices, and we should not expose duplicate
>>> inputs by default. It is fine with me if the default is to create only a
>>> HWMON device (as the lm90 driver was doing so far), and only
>>> DT-instantiated devices have the choice.
>>
>> I don't think this information belongs in the device tree. Whether the
>> device is exposed as hwmon or thermal device (or both) is entirely a
>> software issue whereas DT is a means to describe the hardware.
>>
> Correct; this is exactly the type of information which does _not_ belong int
> devicetree.
>
>> It seems to me that the earlier proposal of communicating to the bridge
>> whether or not a device should be exposed as hwmon device is the right
>> thing to do here.
>>
> Agreed..
Sorry, what's the "bridge" mean, does it mean we need to add a flag in
the thermal_zone_device_register() to indicate if we want to register
virtual hwmon device or not?
Thanks.
Wei.
>
> Guenter
>
On 07/16/2013 12:57 AM, Jean Delvare wrote:
> Hi Wei, Guenter,
>
> On Fri, 12 Jul 2013 15:48:05 +0800, Wei Ni wrote:
>> Add bit defines for the status register.
>
> Regarding the subject: for me these are constants, not macros. AFAIK
> the term "macro" refers to defines with parameters only.
How about "Introduce status bits"
>
>> Signed-off-by: Wei Ni <[email protected]>
>> ---
>> drivers/hwmon/lm90.c | 72 ++++++++++++++++++++++++++++++++++----------------
>> 1 file changed, 49 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index 5f30f90..c90037f 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -179,6 +179,19 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
>> #define LM90_HAVE_TEMP3 (1 << 6) /* 3rd temperature sensor */
>> #define LM90_HAVE_BROKEN_ALERT (1 << 7) /* Broken alert */
>>
>> +/* LM90 status */
>> +#define LM90_STATUS_LTHRM (1 << 0) /* local THERM limit tripped */
>> +#define LM90_STATUS_RTHRM (1 << 1) /* remote THERM limit tripped */
>> +#define LM90_STATUS_OPEN (1 << 2) /* remote is an open circuit */
>> +#define LM90_STATUS_RLOW (1 << 3) /* remote low temp limit tripped */
>> +#define LM90_STATUS_RHIGH (1 << 4) /* remote high temp limit tripped */
>> +#define LM90_STATUS_LLOW (1 << 5) /* local low temp limit tripped */
>> +#define LM90_STATUS_LHIGH (1 << 6) /* local high temp limit tripped */
>> +#define LM90_STATUS_BUSY (1 << 7) /* ADC is converting */
>
> LM90_STATUS_BUSY is never used anywhere so please don't define it.
Ok, I will remove it.
>
>> +
>> +#define MAX6696_STATUS2_RLOW (1 << 3) /* remote2 low temp limit tripped */
>> +#define MAX6696_STATUS2_RHIGH (1 << 4) /* remote2 high temp limit tripped */
>> +
>> /*
>> * Driver data (common to all clients)
>> */
>> @@ -1417,6 +1430,36 @@ static void lm90_init_client(struct i2c_client *client)
>> i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
>> }
>>
>> +static bool lm90_is_tripped(struct i2c_client *client)
>> +{
>> + struct lm90_data *data = i2c_get_clientdata(client);
>> + u8 status, status2 = 0;
>> +
>> + lm90_read_reg(client, LM90_REG_R_STATUS, &status);
>> +
>> + if (data->kind == max6696)
>> + lm90_read_reg(client, MAX6696_REG_R_STATUS2, &status2);
>> +
>> + if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0)
>> + return false;
>
> It's a bit disappointing to not use the freshly introduced constants.
> That being said I agree it would make the code hard to read, so you can
> leave it as is.
Sorry, I forgot it.
How about to define:
#define LM90_STATUS_MASK 0x7f
#define MAX6696_STATUS2 0xfe
Or since Guenter is for vacation, I can just leave it as is, and wait
him back to talk about below issue.
>
> Unrelated to this patch, but Guenter, I am worried about the MAX6696
> handling here. I realize that I am the one who accepted your code, but
> now it looks wrong. Specifically:
> * We check for (status2 & 0xfe) i.e. 7 alarm bits, but the code below
> only reports 2 alarms bits. So if any of the 5 other alarm bits in
> STATUS2 are, we may return true (chip is tripped) but not print the
> cause.
> * At least bits 1 and 2 of STATUS 2 fit totally fine in the driver as
> it currently exists, so I can't think of any reason for not handling
> them. Why are we not? Ideally we should print a message for every
> alarm bit so that we never return "true" without printing a message.
> Even though OT2 limits aren't handled by the driver...
> * If you think this piece of code shouldn't deal with OT/THERM limits
> because they do not trigger an SMBus alarm, this can be discussed,
> but all chips should be handled the same in this respect then.
> * Why in the first place is max6696's data->alert_alarms set to 0x187c
> and not 0x1c7c? Including 1OPEN but not 2OPEN makes no sense.
>
>> +
>> + if (status & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH | LM90_STATUS_LTHRM))
>> + dev_warn(&client->dev,
>> + "temp%d out of range, please check!\n", 1);
>> + if (status & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH | LM90_STATUS_RTHRM))
>> + dev_warn(&client->dev,
>> + "temp%d out of range, please check!\n", 2);
>> + if (status & LM90_STATUS_OPEN)
>> + dev_warn(&client->dev,
>> + "temp%d diode open, please check!\n", 2);
>> +
>> + if (status2 & (MAX6696_STATUS2_RLOW | MAX6696_STATUS2_RHIGH))
>> + dev_warn(&client->dev,
>> + "temp%d out of range, please check!\n", 3);
>> +
>> + return true;
>> +}
>> +
>> static int lm90_probe(struct i2c_client *client,
>> const struct i2c_device_id *id)
>> {
>> @@ -1515,36 +1558,19 @@ 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, alarms2 = 0;
>> -
>> - lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
>> -
>> - if (data->kind == max6696)
>> - lm90_read_reg(client, MAX6696_REG_R_STATUS2, &alarms2);
>> -
>> - if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) {
>> + if (!lm90_is_tripped(client)) {
>
> You could swap the success and failure cases to avoid this negation.
Yes, I will change it.
>
>> dev_info(&client->dev, "Everything OK\n");
>> } else {
>> - if (alarms & 0x61)
>> - dev_warn(&client->dev,
>> - "temp%d out of range, please check!\n", 1);
>> - if (alarms & 0x1a)
>> - dev_warn(&client->dev,
>> - "temp%d out of range, please check!\n", 2);
>> - if (alarms & 0x04)
>> - 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.
>> */
>> + struct lm90_data *data = i2c_get_clientdata(client);
>> + u8 config, alarms;
>> +
>> + lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
>
> You end up reading LM90_REG_R_STATUS, which is not OK. This register
> contains self-clearing bits, so there is no guarantee that the second
> read will return the same value as the first read. You'll have to come
> up with a different approach that reads LM90_REG_R_STATUS only once.
Oh, yes, this is a problem, I didn't noticed it.
How about to use this:
bool lm90_alarms_tripped(*client, *status);
bool lm90_alarms2_tripped(*client, *status2);
So we can read the status only once and pass it.
>
>> +
>> if ((data->flags & LM90_HAVE_BROKEN_ALERT)
>> && (alarms & data->alert_alarms)) {
>> dev_dbg(&client->dev, "Disabling ALERT#\n");
>
>
On 07/17/2013 03:03 PM, Wei Ni wrote:
> On 07/16/2013 12:57 AM, Jean Delvare wrote:
>> Hi Wei, Guenter,
>>
>>> +
>>> + if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0)
>>> + return false;
>>
>> It's a bit disappointing to not use the freshly introduced constants.
>> That being said I agree it would make the code hard to read, so you can
>> leave it as is.
>
> Sorry, I forgot it.
> How about to define:
> #define LM90_STATUS_MASK 0x7f
> #define MAX6696_STATUS2 0xfe
Sorry, it should be "#define MAX6696_STATUS2_MASK 0xfe".
>
> Or since Guenter is for vacation, I can just leave it as is, and wait
> him back to talk about below issue.
>
>>
>
Hi Wei,
On Wed, 17 Jul 2013 15:03:35 +0800, Wei Ni wrote:
> On 07/16/2013 12:57 AM, Jean Delvare wrote:
> > On Fri, 12 Jul 2013 15:48:05 +0800, Wei Ni wrote:
> >> Add bit defines for the status register.
> >
> > Regarding the subject: for me these are constants, not macros. AFAIK
> > the term "macro" refers to defines with parameters only.
>
> How about "Introduce status bits"
I'd say "Define status bits" as this is exactly what you're doing ;-)
That being said, your patch actually does more than this, as you are
moving code around and to a separate function. The patch description
should say that and explain why.
> >> (...)
> >> + if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0)
> >> + return false;
> >
> > It's a bit disappointing to not use the freshly introduced constants.
> > That being said I agree it would make the code hard to read, so you can
> > leave it as is.
>
> Sorry, I forgot it.
> How about to define:
> #define LM90_STATUS_MASK 0x7f
> #define MAX6696_STATUS2_MASK 0xfe
I wouldn't bother. I suspect that this code will have to be reworked
soon anyway and these constants may no longer be needed then.
> Or since Guenter is for vacation, I can just leave it as is, and wait
> him back to talk about below issue.
I do maintain the lm90 driver, so the decision is up to me. Guenter did
a preliminary review of your patches and I am grateful for that, but I
do not intend to wait for his return to continue with your patches.
Otherwise he will have to do the same when he returns and I am gone,
and this may end up delaying your patches by one kernel version.
> >> (...)
> >> + struct lm90_data *data = i2c_get_clientdata(client);
> >> + u8 config, alarms;
> >> +
> >> + lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> >
> > You end up reading LM90_REG_R_STATUS, which is not OK. This register
> > contains self-clearing bits, so there is no guarantee that the second
> > read will return the same value as the first read. You'll have to come
> > up with a different approach that reads LM90_REG_R_STATUS only once.
>
> Oh, yes, this is a problem, I didn't noticed it.
> How about to use this:
> bool lm90_alarms_tripped(*client, *status);
> bool lm90_alarms2_tripped(*client, *status2);
> So we can read the status only once and pass it.
This is a good idea but you only need status, not status2, so it can be
made simpler:
bool lm90_is_tripped(*client, *status);
(handling both status and status 2 as you already do.)
--
Jean Delvare
On Wed, 17 Jul 2013 14:26:54 +0800, Wei Ni wrote:
> On 07/17/2013 01:14 PM, Guenter Roeck wrote:
> > On Wed, Jul 17, 2013 at 06:26:20AM +0200, Thierry Reding wrote:
> >> On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote:
> >>> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
> >>>> I think we can decide it in the DT, we can add a dt property in the lm90
> >>>> device node, such as:
> >>>> sys-interface = SYS_HWMON;
> >>>> or
> >>>> sys-interface = SYS_THERMAL;
> >>>> So we register it as the hwmon or thermal device
> >>>
> >>> This is an option, but please keep in mind that DT is not the only way
> >>> to instantiate LM90-like devices, and we should not expose duplicate
> >>> inputs by default. It is fine with me if the default is to create only a
> >>> HWMON device (as the lm90 driver was doing so far), and only
> >>> DT-instantiated devices have the choice.
> >>
> >> I don't think this information belongs in the device tree. Whether the
> >> device is exposed as hwmon or thermal device (or both) is entirely a
> >> software issue whereas DT is a means to describe the hardware.
> >>
> > Correct; this is exactly the type of information which does _not_ belong int
> > devicetree.
> >
> >> It seems to me that the earlier proposal of communicating to the bridge
> >> whether or not a device should be exposed as hwmon device is the right
> >> thing to do here.
> >
> > Agreed..
>
> Sorry, what's the "bridge" mean,
The code which creates a virtual hwmon input when a new thermal zone is
registered (this code is in thermal_core.c.)
> does it mean we need to add a flag in
> the thermal_zone_device_register() to indicate if we want to register
> virtual hwmon device or not?
I think so, yes.
Alternatively the flag could be added to struct
thermal_zone_device_ops, so that you don't have to update all the
callers. But I admit it's a hack as the flag doesn't really belong
there, so I suppose we don't really want to do that.
I have been thinking of an automatic approach, based on comparing the
type string passed to thermal_zone_device_register() with already
registered hwmon devices, but I couldn't come up with something good
and robust enough, so let's forget about it.
--
Jean Delvare
On 07/17/2013 04:28 PM, Jean Delvare wrote:
> Hi Wei,
>
> On Wed, 17 Jul 2013 15:03:35 +0800, Wei Ni wrote:
>> On 07/16/2013 12:57 AM, Jean Delvare wrote:
>>> On Fri, 12 Jul 2013 15:48:05 +0800, Wei Ni wrote:
>>>> Add bit defines for the status register.
>>>
>>> Regarding the subject: for me these are constants, not macros. AFAIK
>>> the term "macro" refers to defines with parameters only.
>>
>> How about "Introduce status bits"
>
> I'd say "Define status bits" as this is exactly what you're doing ;-)
> That being said, your patch actually does more than this, as you are
> moving code around and to a separate function. The patch description
> should say that and explain why.
ok, I will update it in my next version.
>
>>>> (...)
>>>> + if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0)
>>>> + return false;
>>>
>>> It's a bit disappointing to not use the freshly introduced constants.
>>> That being said I agree it would make the code hard to read, so you can
>>> leave it as is.
>>
>> Sorry, I forgot it.
>> How about to define:
>> #define LM90_STATUS_MASK 0x7f
>> #define MAX6696_STATUS2_MASK 0xfe
>
> I wouldn't bother. I suspect that this code will have to be reworked
> soon anyway and these constants may no longer be needed then.
Ok, let's leave it as is.
>
>> Or since Guenter is for vacation, I can just leave it as is, and wait
>> him back to talk about below issue.
>
> I do maintain the lm90 driver, so the decision is up to me. Guenter did
> a preliminary review of your patches and I am grateful for that, but I
> do not intend to wait for his return to continue with your patches.
> Otherwise he will have to do the same when he returns and I am gone,
> and this may end up delaying your patches by one kernel version.
I will send out patches soon :)
>
>>>> (...)
>>>> + struct lm90_data *data = i2c_get_clientdata(client);
>>>> + u8 config, alarms;
>>>> +
>>>> + lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
>>>
>>> You end up reading LM90_REG_R_STATUS, which is not OK. This register
>>> contains self-clearing bits, so there is no guarantee that the second
>>> read will return the same value as the first read. You'll have to come
>>> up with a different approach that reads LM90_REG_R_STATUS only once.
>>
>> Oh, yes, this is a problem, I didn't noticed it.
>> How about to use this:
>> bool lm90_alarms_tripped(*client, *status);
>> bool lm90_alarms2_tripped(*client, *status2);
>> So we can read the status only once and pass it.
>
> This is a good idea but you only need status, not status2, so it can be
> made simpler:
> bool lm90_is_tripped(*client, *status);
> (handling both status and status 2 as you already do.)
Yes this is simpler, but I think in the future we may need to handle the
status2, how to handle it ? Or we can define the status as
bit[0~7]->status and bit[8~15]->status2 .
>
Hi Wei,
On Wed, 17 Jul 2013 17:29:32 +0800, Wei Ni wrote:
> On 07/17/2013 04:28 PM, Jean Delvare wrote:
> > I do maintain the lm90 driver, so the decision is up to me. Guenter did
> > a preliminary review of your patches and I am grateful for that, but I
> > do not intend to wait for his return to continue with your patches.
> > Otherwise he will have to do the same when he returns and I am gone,
> > and this may end up delaying your patches by one kernel version.
>
> I will send out patches soon :)
You may want to wait until I'm done reviewing the whole v3 patch set. I
hope to have the time to do that today.
> >> (...)
> >> Oh, yes, this is a problem, I didn't noticed it.
> >> How about to use this:
> >> bool lm90_alarms_tripped(*client, *status);
> >> bool lm90_alarms2_tripped(*client, *status2);
> >> So we can read the status only once and pass it.
> >
> > This is a good idea but you only need status, not status2, so it can be
> > made simpler:
> > bool lm90_is_tripped(*client, *status);
> > (handling both status and status 2 as you already do.)
>
> Yes this is simpler, but I think in the future we may need to handle the
> status2, how to handle it ? Or we can define the status as
> bit[0~7]->status and bit[8~15]->status2 .
I hope we will never have to. We need it to work around a hardware
implementation bug, and I hope that newer chips implementing status2
will not have this bug.
That being said, yes, returning status and status2 combined in a 16-bit
value would make sense. We end up comparing with data->alert_alarms
which has exactly this format anyway (and so does data->alarms too.)
--
Jean Delvare
On 07/17/2013 05:11 PM, Jean Delvare wrote:
> On Wed, 17 Jul 2013 14:26:54 +0800, Wei Ni wrote:
>> On 07/17/2013 01:14 PM, Guenter Roeck wrote:
>>> On Wed, Jul 17, 2013 at 06:26:20AM +0200, Thierry Reding wrote:
>>>> On Mon, Jul 15, 2013 at 09:24:15AM +0200, Jean Delvare wrote:
>>>>> On Mon, 15 Jul 2013 14:25:29 +0800, Wei Ni wrote:
>>>>>> I think we can decide it in the DT, we can add a dt property in the lm90
>>>>>> device node, such as:
>>>>>> sys-interface = SYS_HWMON;
>>>>>> or
>>>>>> sys-interface = SYS_THERMAL;
>>>>>> So we register it as the hwmon or thermal device
>>>>>
>>>>> This is an option, but please keep in mind that DT is not the only way
>>>>> to instantiate LM90-like devices, and we should not expose duplicate
>>>>> inputs by default. It is fine with me if the default is to create only a
>>>>> HWMON device (as the lm90 driver was doing so far), and only
>>>>> DT-instantiated devices have the choice.
>>>>
>>>> I don't think this information belongs in the device tree. Whether the
>>>> device is exposed as hwmon or thermal device (or both) is entirely a
>>>> software issue whereas DT is a means to describe the hardware.
>>>>
>>> Correct; this is exactly the type of information which does _not_ belong int
>>> devicetree.
>>>
>>>> It seems to me that the earlier proposal of communicating to the bridge
>>>> whether or not a device should be exposed as hwmon device is the right
>>>> thing to do here.
>>>
>>> Agreed..
>>
>> Sorry, what's the "bridge" mean,
>
> The code which creates a virtual hwmon input when a new thermal zone is
> registered (this code is in thermal_core.c.)
>
>> does it mean we need to add a flag in
>> the thermal_zone_device_register() to indicate if we want to register
>> virtual hwmon device or not?
>
> I think so, yes.
>
> Alternatively the flag could be added to struct
> thermal_zone_device_ops, so that you don't have to update all the
> callers. But I admit it's a hack as the flag doesn't really belong
> there, so I suppose we don't really want to do that.
I think it's better to add it to struct thermal_zone_params, the
thermal_zone_device_ops is for the callback functions.
And I ask it with thermal fw engineers in
http://thread.gmane.org/gmane.linux.power-management.general/35874.
May be you can look it.
>
> I have been thinking of an automatic approach, based on comparing the
> type string passed to thermal_zone_device_register() with already
> registered hwmon devices, but I couldn't come up with something good
> and robust enough, so let's forget about it.
>
Hi Wei,
On Fri, 12 Jul 2013 15:48:06 +0800, Wei Ni wrote:
> When the temperature exceed the limit range value,
> the driver can handle the interrupt.
>
> Signed-off-by: Wei Ni <[email protected]>
> ---
> drivers/hwmon/lm90.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index c90037f..1cc3d19 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -89,6 +89,7 @@
> #include <linux/err.h>
> #include <linux/mutex.h>
> #include <linux/sysfs.h>
> +#include <linux/interrupt.h>
>
> /*
> * Addresses to scan
> @@ -1460,6 +1461,17 @@ static bool lm90_is_tripped(struct i2c_client *client)
> return true;
> }
>
> +static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
> +{
> + struct lm90_data *data = dev_id;
> + struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
Why are you passing data as the dev_id in the first place, instead of
client? It's easier to get the data when you have the client
(i2c_get_clientdata) than the other way around.
> +
> + if (lm90_is_tripped(client))
> + return IRQ_HANDLED;
> + else
> + return IRQ_NONE;
> +}
> +
> static int lm90_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -1536,6 +1548,18 @@ static int lm90_probe(struct i2c_client *client,
> goto exit_remove_files;
> }
>
> + if (client->irq >= 0) {
I though you had agreed to just check for (client->irq)?
> + dev_dbg(dev, "lm90 IRQ: %d\n", client->irq);
The "lm90" is redundant, dev_dbg will use the chip name as a prefix.
> + err = devm_request_threaded_irq(dev, client->irq,
> + NULL, lm90_irq_thread,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> + "lm90", data);
> + if (err < 0) {
> + dev_err(dev, "cannot request interrupt\n");
You should include the IRQ number in the error message, it is useful
for investigating the issue. Not everyone will have the debugging
message above enabled.
> + goto exit_remove_files;
> + }
> + }
> +
> return 0;
>
> exit_remove_files:
That's it for the code. Now I'm not sure I understand what you are
trying to do and what this is all good for.
First of all, how is the chip wired on your system? You are using an
NCT1008, right? Which output of the chip is connected to your interrupt
line, THERM or ALERT/THERM2? ALERT is normally used for SMBus Alert but
I suppose it could be used for an interrupt too. THERM and THERM2 OTOH
are comparator outputs, they stay low as long as the temperature are
above the therm limits. Reading the status register won't bring them
back up as I understand it, and contrary to the ALERT output, they
can't be masked. Won't this result in an interrupt flood if using
IRQF_TRIGGER_LOW? Have you tested your code already?
Also I don't think just logging an error message is the right thing to
do in the case of overheating. The code to handle SMBus alerts is from
me, and it does indeed just log the problems, but it was really only
meant as a proof of concept when I first added support for SMBus Alert.
Today we could do better than this, starting with issuing sysfs
notifications to the relevant alarm files (several other hwmon drivers
are doing that already.)
For a real system, if the THERM output is connected to an interrupt line
(instead of directly to a fan controller) I would expect the platform
to provide a callback to handle thermal events and take whatever
appropriate measure is needed (e.g. throttling.) Just logging the
problem won't save the system, by the time someone looks at the log it
might be too late.
--
Jean Delvare
On 07/18/2013 11:58 PM, Jean Delvare wrote:
> Hi Wei,
>
> On Fri, 12 Jul 2013 15:48:06 +0800, Wei Ni wrote:
>> When the temperature exceed the limit range value,
>> the driver can handle the interrupt.
>>
>> Signed-off-by: Wei Ni <[email protected]>
>> ---
>> drivers/hwmon/lm90.c | 24 ++++++++++++++++++++++++
>> 1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index c90037f..1cc3d19 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -89,6 +89,7 @@
>> #include <linux/err.h>
>> #include <linux/mutex.h>
>> #include <linux/sysfs.h>
>> +#include <linux/interrupt.h>
>>
>> /*
>> * Addresses to scan
>> @@ -1460,6 +1461,17 @@ static bool lm90_is_tripped(struct i2c_client *client)
>> return true;
>> }
>>
>> +static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
>> +{
>> + struct lm90_data *data = dev_id;
>> + struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
>
> Why are you passing data as the dev_id in the first place, instead of
> client? It's easier to get the data when you have the client
> (i2c_get_clientdata) than the other way around.
Oh, I'm stupid :)
I will pass the client.
>
>> +
>> + if (lm90_is_tripped(client))
>> + return IRQ_HANDLED;
>> + else
>> + return IRQ_NONE;
>> +}
>> +
>> static int lm90_probe(struct i2c_client *client,
>> const struct i2c_device_id *id)
>> {
>> @@ -1536,6 +1548,18 @@ static int lm90_probe(struct i2c_client *client,
>> goto exit_remove_files;
>> }
>>
>> + if (client->irq >= 0) {
>
> I though you had agreed to just check for (client->irq)?
Oh, yes, I forgot to change it, thanks, I will update it.
>
>> + dev_dbg(dev, "lm90 IRQ: %d\n", client->irq);
>
> The "lm90" is redundant, dev_dbg will use the chip name as a prefix.
Ok, I will remove it.
>
>> + err = devm_request_threaded_irq(dev, client->irq,
>> + NULL, lm90_irq_thread,
>> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>> + "lm90", data);
>> + if (err < 0) {
>> + dev_err(dev, "cannot request interrupt\n");
>
> You should include the IRQ number in the error message, it is useful
> for investigating the issue. Not everyone will have the debugging
> message above enabled.
Yes, you are right, I will add the IRQ number.
>
>> + goto exit_remove_files;
>> + }
>> + }
>> +
>> return 0;
>>
>> exit_remove_files:
>
> That's it for the code. Now I'm not sure I understand what you are
> trying to do and what this is all good for.
>
> First of all, how is the chip wired on your system? You are using an
> NCT1008, right? Which output of the chip is connected to your interrupt
> line, THERM or ALERT/THERM2? ALERT is normally used for SMBus Alert but
> I suppose it could be used for an interrupt too. THERM and THERM2 OTOH
> are comparator outputs, they stay low as long as the temperature are
> above the therm limits. Reading the status register won't bring them
> back up as I understand it, and contrary to the ALERT output, they
> can't be masked. Won't this result in an interrupt flood if using
> IRQF_TRIGGER_LOW? Have you tested your code already?
Let me explain why I want to add the IRQ thread.
In our tegra30 platform, we use an NCT1008, and in our downstream tree,
it programmed as following:
1. The pin THERM is connected to the PMIC, we will set the THERM limit
in the initialization, once the this limit is tripped, it will trigged
the PMIC, and the PMIC will shutdown the system immediately.
2. The pin ALERT/THERM2 is configured as ALERT, and it is connected to
the interrupt line. In the platform init, we will prepare some trip
temps, such as 20C, 40C,60C, 80C, and we will set 20C to the
remote-low-temp-limit, and set 40C to remote-high-temp-limit. When the
temperature exceed this rang, it will interrupt the system, then we will
update the low/high limit to next rang, for example, if the temp raise
to 50C, we will set 40C to low-limit, and set 60C to hight-limit, then
we will run the throttle functions, and update cooling state.
We wish to upstream these codes, but as you know, it's difficult to use
current lm90.c and thermal framework to implement it, and these codes
related many other things, such as throttling cpufreq, other clock freq.
So at first, I want to update the lm90.c, so that I can add it to the
thermal fw and use interrupt handler easily. And at the same time I
followed the thermal framework thread, there has new infrastructure
posted, which is more easy to add lm90 to thermal fw.
>
> Also I don't think just logging an error message is the right thing to
> do in the case of overheating. The code to handle SMBus alerts is from
> me, and it does indeed just log the problems, but it was really only
> meant as a proof of concept when I first added support for SMBus Alert.
> Today we could do better than this, starting with issuing sysfs
> notifications to the relevant alarm files (several other hwmon drivers
> are doing that already.)
yes, I can try to use sysfs_notify() for the alarm.
>
> For a real system, if the THERM output is connected to an interrupt line
> (instead of directly to a fan controller) I would expect the platform
> to provide a callback to handle thermal events and take whatever
> appropriate measure is needed (e.g. throttling.) Just logging the
> problem won't save the system, by the time someone looks at the log it
> might be too late.
In our downstream tree, we write a new driver nct1008.c, whci can handle
these interrupts and all other things, but as you know this driver can't
be upstreamed, because there already has the lm90.c :).
Anyway I think this patch is the first step of the implementation.
>
Hi, Jean
On 07/19/2013 02:41 PM, Wei Ni wrote:
> On 07/18/2013 11:58 PM, Jean Delvare wrote:
>> Hi Wei,
>>
>> On Fri, 12 Jul 2013 15:48:06 +0800, Wei Ni wrote:
>>> When the temperature exceed the limit range value,
>>> the driver can handle the interrupt.
>>>
>>> Signed-off-by: Wei Ni <[email protected]>
>>> ---
>>> drivers/hwmon/lm90.c | 24 ++++++++++++++++++++++++
>>> 1 file changed, 24 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>>> index c90037f..1cc3d19 100644
>>> --- a/drivers/hwmon/lm90.c
>>> +++ b/drivers/hwmon/lm90.c
>>> @@ -89,6 +89,7 @@
>>> #include <linux/err.h>
>>> #include <linux/mutex.h>
>>> #include <linux/sysfs.h>
>>> +#include <linux/interrupt.h>
>>>
>>> /*
>>> * Addresses to scan
>>> @@ -1460,6 +1461,17 @@ static bool lm90_is_tripped(struct i2c_client *client)
>>> return true;
>>> }
>>>
>>> +static irqreturn_t lm90_irq_thread(int irq, void *dev_id)
>>> +{
>>> + struct lm90_data *data = dev_id;
>>> + struct i2c_client *client = to_i2c_client(data->hwmon_dev->parent);
>>
>> Why are you passing data as the dev_id in the first place, instead of
>> client? It's easier to get the data when you have the client
>> (i2c_get_clientdata) than the other way around.
>
> Oh, I'm stupid :)
> I will pass the client.
>
>>
>>> +
>>> + if (lm90_is_tripped(client))
>>> + return IRQ_HANDLED;
>>> + else
>>> + return IRQ_NONE;
>>> +}
>>> +
>>> static int lm90_probe(struct i2c_client *client,
>>> const struct i2c_device_id *id)
>>> {
>>> @@ -1536,6 +1548,18 @@ static int lm90_probe(struct i2c_client *client,
>>> goto exit_remove_files;
>>> }
>>>
>>> + if (client->irq >= 0) {
>>
>> I though you had agreed to just check for (client->irq)?
>
> Oh, yes, I forgot to change it, thanks, I will update it.
>
>>
>>> + dev_dbg(dev, "lm90 IRQ: %d\n", client->irq);
>>
>> The "lm90" is redundant, dev_dbg will use the chip name as a prefix.
>
> Ok, I will remove it.
>
>>
>>> + err = devm_request_threaded_irq(dev, client->irq,
>>> + NULL, lm90_irq_thread,
>>> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>>> + "lm90", data);
>>> + if (err < 0) {
>>> + dev_err(dev, "cannot request interrupt\n");
>>
>> You should include the IRQ number in the error message, it is useful
>> for investigating the issue. Not everyone will have the debugging
>> message above enabled.
>
> Yes, you are right, I will add the IRQ number.
>
>>
>>> + goto exit_remove_files;
>>> + }
>>> + }
>>> +
>>> return 0;
>>>
>>> exit_remove_files:
>>
>> That's it for the code. Now I'm not sure I understand what you are
>> trying to do and what this is all good for.
>>
>> First of all, how is the chip wired on your system? You are using an
>> NCT1008, right? Which output of the chip is connected to your interrupt
>> line, THERM or ALERT/THERM2? ALERT is normally used for SMBus Alert but
>> I suppose it could be used for an interrupt too. THERM and THERM2 OTOH
>> are comparator outputs, they stay low as long as the temperature are
>> above the therm limits. Reading the status register won't bring them
>> back up as I understand it, and contrary to the ALERT output, they
>> can't be masked. Won't this result in an interrupt flood if using
>> IRQF_TRIGGER_LOW? Have you tested your code already?
>
> Let me explain why I want to add the IRQ thread.
> In our tegra30 platform, we use an NCT1008, and in our downstream tree,
> it programmed as following:
> 1. The pin THERM is connected to the PMIC, we will set the THERM limit
> in the initialization, once the this limit is tripped, it will trigged
> the PMIC, and the PMIC will shutdown the system immediately.
> 2. The pin ALERT/THERM2 is configured as ALERT, and it is connected to
> the interrupt line. In the platform init, we will prepare some trip
> temps, such as 20C, 40C,60C, 80C, and we will set 20C to the
> remote-low-temp-limit, and set 40C to remote-high-temp-limit. When the
> temperature exceed this rang, it will interrupt the system, then we will
> update the low/high limit to next rang, for example, if the temp raise
> to 50C, we will set 40C to low-limit, and set 60C to hight-limit, then
> we will run the throttle functions, and update cooling state.
>
> We wish to upstream these codes, but as you know, it's difficult to use
> current lm90.c and thermal framework to implement it, and these codes
> related many other things, such as throttling cpufreq, other clock freq.
> So at first, I want to update the lm90.c, so that I can add it to the
> thermal fw and use interrupt handler easily. And at the same time I
> followed the thermal framework thread, there has new infrastructure
> posted, which is more easy to add lm90 to thermal fw.
>
>>
>> Also I don't think just logging an error message is the right thing to
>> do in the case of overheating. The code to handle SMBus alerts is from
>> me, and it does indeed just log the problems, but it was really only
>> meant as a proof of concept when I first added support for SMBus Alert.
>> Today we could do better than this, starting with issuing sysfs
>> notifications to the relevant alarm files (several other hwmon drivers
>> are doing that already.)
>
> yes, I can try to use sysfs_notify() for the alarm.
>
>>
>> For a real system, if the THERM output is connected to an interrupt line
>> (instead of directly to a fan controller) I would expect the platform
>> to provide a callback to handle thermal events and take whatever
>> appropriate measure is needed (e.g. throttling.) Just logging the
>> problem won't save the system, by the time someone looks at the log it
>> might be too late.
>
> In our downstream tree, we write a new driver nct1008.c, whci can handle
> these interrupts and all other things, but as you know this driver can't
> be upstreamed, because there already has the lm90.c :).
> Anyway I think this patch is the first step of the implementation.
Do you have any more suggestions for this series, if no, I will prepare
v4 patches.
Thanks.
Wei.
>
>>
>
Hi Wei,
On Wed, 24 Jul 2013 15:46:46 +0800, Wei Ni wrote:
> Do you have any more suggestions for this series, if no, I will prepare
> v4 patches.
Sorry, I have a couple more replies "in progress" but had friends at
home last week end so I did not have the time to finish and send them.
I'll try to do that today, thanks for your patience.
--
Jean Delvare
Hi Wei,
Sorry for the late reply.
On Fri, 19 Jul 2013 14:41:54 +0800, Wei Ni wrote:
> On 07/18/2013 11:58 PM, Jean Delvare wrote:
> > First of all, how is the chip wired on your system? You are using an
> > NCT1008, right? Which output of the chip is connected to your interrupt
> > line, THERM or ALERT/THERM2? ALERT is normally used for SMBus Alert but
> > I suppose it could be used for an interrupt too. THERM and THERM2 OTOH
> > are comparator outputs, they stay low as long as the temperature are
> > above the therm limits. Reading the status register won't bring them
> > back up as I understand it, and contrary to the ALERT output, they
> > can't be masked. Won't this result in an interrupt flood if using
> > IRQF_TRIGGER_LOW? Have you tested your code already?
>
> Let me explain why I want to add the IRQ thread.
> In our tegra30 platform, we use an NCT1008, and in our downstream tree,
> it programmed as following:
> 1. The pin THERM is connected to the PMIC, we will set the THERM limit
> in the initialization, once the this limit is tripped, it will trigged
> the PMIC, and the PMIC will shutdown the system immediately.
OK, this is what the chip is designed for, good.
> 2. The pin ALERT/THERM2 is configured as ALERT, and it is connected to
> the interrupt line.
Why don't you use the SMBus alert mechanism then? It is already
implemented and allows you to reuse the interrupt of the SMBus
controller.
Of course this is a question for the hardware designers, not you... If
the board uses a separate interrupt pin for the NCT1008's ALERT output,
then the driver has to handle that interrupt explicitly, as done in your
patch.
One thing which worries me though is this explanation in the NCT1008
datasheet:
"The ALERT interrupt latch is not reset by reading the
status register. It resets when the ALERT output has been
serviced by the master reading the device address, provided
the error condition has gone away and the status register flag
bits are reset."
This suggests that connecting the ALERT output to a separate interrupt
pin will not work, as the ALERT output will never be de-asserted even
if the fault conditions are gone and the status register was read. But
as you say this is how your system is supposed to work, can you explain
how it can work?
> In the platform init, we will prepare some trip
> temps, such as 20C, 40C,60C, 80C, and we will set 20C to the
> remote-low-temp-limit, and set 40C to remote-high-temp-limit. When the
> temperature exceed this rang, it will interrupt the system, then we will
> update the low/high limit to next rang, for example, if the temp raise
> to 50C, we will set 40C to low-limit, and set 60C to hight-limit, then
> we will run the throttle functions, and update cooling state.
Hu ho, I have seen this kind of design in the past, and I must say I
don't quite like it. Moving critical thermal management handling to the
software makes me feel unsafe. System thermal safety should not rely on
the OS IMHO. It is best handled by the hardware, or if that can't be
done, by the BIOS. And these limit updates are tricky, they could fail
and then you're in trouble. Imagine for example that another chip on
the SMBus hugs the bus and delays or even plain prevents the change of
limits...
But once again I guess you're not responsible for this.
Another problem with this design, even if no such problem happens, is
that the limits are user(-space)-writable in the lm90 driver, while
with your design these limits are under full control of the platform
management code. You definitely don't want the user to come and adjust
the limits, this could result in overheating of the system. So, do you
have a plan to optionally switch limits to read only in the lm90
driver? If not, how do you intend to solve the problem?
The more I think about it, the more I wonder if a custom thermal driver
wouldn't be better for your case. Exposing a few read-only values to
user-space through the thermal/hwmon bridge would IMHO make more sense
than cluttering the lm90 driver with conditionals to limit what it
exposes to user-space.
> We wish to upstream these codes, but as you know, it's difficult to use
> current lm90.c and thermal framework to implement it, and these codes
> related many other things, such as throttling cpufreq, other clock freq.
> So at first, I want to update the lm90.c, so that I can add it to the
> thermal fw and use interrupt handler easily. And at the same time I
> followed the thermal framework thread, there has new infrastructure
> posted, which is more easy to add lm90 to thermal fw.
Don't get me wrong, I'm very happy with your efforts to upstream your
code, this is very welcome. And I am also very happy that you split it
into small chunks which are easier to review. But I also need to know
the big picture to see where you're ultimately going.
> > (...)
> > For a real system, if the THERM output is connected to an interrupt line
> > (instead of directly to a fan controller) I would expect the platform
> > to provide a callback to handle thermal events and take whatever
> > appropriate measure is needed (e.g. throttling.) Just logging the
> > problem won't save the system, by the time someone looks at the log it
> > might be too late.
>
> In our downstream tree, we write a new driver nct1008.c, whci can handle
> these interrupts and all other things, but as you know this driver can't
> be upstreamed, because there already has the lm90.c :).
> Anyway I think this patch is the first step of the implementation.
I'm curious how the nct1008 driver "handles these interrupts." IMHO
only the platform code knows what needs to be done upon reception of an
interrupt, in particular in your case where you'll do some magic with the
limit registers. There's no way this can be hard-coded in a hwmon
driver, lm90 or any other.
--
Jean Delvare
Hi Wei,
On Fri, 12 Jul 2013 15:48:07 +0800, Wei Ni wrote:
> Using enums for the indexes and nrs of temp8 and temp11.
> This make the code much more readable.
I can't say I'm thrilled by this patch. The improved readability is
questionable. In the original code, each line already had one constant
which made it clear what the code was doing. So the gain is marginal,
I'd say, and this costs almost 50 lines of code.
Let me review it nevertheless:
>
> Signed-off-by: Wei Ni <[email protected]>
> ---
> drivers/hwmon/lm90.c | 179 ++++++++++++++++++++++++++++++++------------------
> 1 file changed, 114 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 1cc3d19..8cb5dd0 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -310,6 +310,59 @@ static const struct lm90_params lm90_params[] = {
> };
>
> /*
> + * TEMP8 register index
> + */
> +enum lm90_temp8_reg_index {
> + TEMP8_LOCAL_LOW = 0, /* 0: local low limit */
> + TEMP8_LOCAL_HIGH, /* 1: local high limit */
> + TEMP8_LOCAL_CRIT, /* 2: local critical limit */
> + TEMP8_REMOTE_CRIT, /* 3: remote critical limit */
> + TEMP8_LOCAL_EMERG, /* 4: local emergency limit
> + * (max6659 and max6695/96)
> + */
> + TEMP8_REMOTE_EMERG, /* 5: remote emergency limit
> + * (max6659 and max6695/96)
> + */
> + TEMP8_REMOTE2_CRIT, /* 6: remote 2 critical limit
> + * (max6695/96 only)
> + */
> + TEMP8_REMOTE2_EMERG, /* 7: remote 2 emergency limit
> + * (max6695/96 only)
> + */
> + TEMP8_REG_NUM
> +};
> +
> +/*
> + * TEMP11 register index
> + */
> +enum lm90_temp11_reg_index {
> + TEMP11_REMOTE_TEMP = 0, /* 0: remote input */
> + TEMP11_REMOTE_LOW, /* 1: remote low limit */
> + TEMP11_REMOTE_HIGH, /* 2: remote high limit */
> + TEMP11_REMOTE_OFFSET, /* 3: remote offset
> + * (except max6646, max6657/58/59,
> + * and max6695/96)
> + */
> + TEMP11_LOCAL_TEMP, /* 4: local input */
> + TEMP11_REMOTE2_TEMP, /* 5: remote 2 input (max6695/96 only) */
> + TEMP11_REMOTE2_LOW, /* 6: remote 2 low limit (max6695/96 only) */
> + TEMP11_REMOTE2_HIGH, /* 7: remote 2 high limit (max6695/96 only) */
> + TEMP11_REG_NUM
> +};
The "TEMP8_" and "TEMP11_" prefixes aren't really needed, as there is no
overlapping between both sets. Removing these prefixes (except for the
terminators, where they are needed and make sense) would make the rest
of the code more readable IMHO, as it would avoid redundant information
on most lines, and avoid line splitting in some cases.
Also, the comments are mostly useless now, they were there to document
what each number was referring to, but now this is exactly what the new
constants are doing.
> +
> +/*
> + * TEMP11 register NR
> + */
> +enum lm90_temp11_reg_nr {
> + NR_CHAN_0_REMOTE_LOW = 0, /* 0: channel 0, remote low limit */
> + NR_CHAN_0_REMOTE_HIGH, /* 1: channel 0, remote high limit */
> + NR_CHAN_0_REMOTE_OFFSET, /* 2: channel 0, remote offset */
> + NR_CHAN_1_REMOTE_LOW, /* 3: channel 1, remote low limit */
> + NR_CHAN_1_REMOTE_HIGH, /* 4: channel 1, remote high limit */
The conventions used in the descriptions diverge from the ones used
above. "channel 0 remote" here is just "remove" above, and "channel 1
remote" is "remote 2" above. This is quite confusing.
> + NR_NUM /* number of the NRs for temp11 */
The fact that you were unable to come up with a proper name for this
number is a clear indication that this enum should not exist in the
first place.
These numbers are used only once, to pass specific information to
set_temp11. This was easy enough when these were just numbers and I
really had no reason not to do that.
If you now want to use clean constants, this should be done with logic
and consistency. Your proposal makes sense for the data->temp8 and
data->temp11 array indexing, because they are used more than once. But
introducing a new set of constants with weird names just for one use
case doesn't help readability, it makes things worse actually.
So if you insist of making the code more readable by the means of named
constants, then you should simply adjust the reg array in write_temp11
so that it can be indexed with your TEMP11_* constants above (as is
data->temp11.) This will leave some holes in the array but I don't
think we care about a few lost bytes here.
> +};
> +
> +/*
> * Client data (each client gets its own)
> */
>
> @@ -331,25 +384,8 @@ struct lm90_data {
> u8 reg_local_ext; /* local extension register offset */
>
> /* registers values */
> - s8 temp8[8]; /* 0: local low limit
> - * 1: local high limit
> - * 2: local critical limit
> - * 3: remote critical limit
> - * 4: local emergency limit (max6659 and max6695/96)
> - * 5: remote emergency limit (max6659 and max6695/96)
> - * 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, max6657/58/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 (max6695/96 only)
> - */
> + s8 temp8[TEMP8_REG_NUM];
> + s16 temp11[TEMP11_REG_NUM];
> u8 temp_hyst;
> u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
> };
> @@ -491,37 +527,42 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> u8 alarms;
>
> dev_dbg(&client->dev, "Updating lm90 data.\n");
> - lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]);
> - lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, &data->temp8[1]);
> - lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, &data->temp8[2]);
> - lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]);
> + lm90_read_reg(client, LM90_REG_R_LOCAL_LOW,
> + &data->temp8[TEMP8_LOCAL_LOW]);
> + lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH,
> + &data->temp8[TEMP8_LOCAL_HIGH]);
> + lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT,
> + &data->temp8[TEMP8_LOCAL_CRIT]);
> + lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
> + &data->temp8[TEMP8_REMOTE_CRIT]);
> lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst);
>
> if (data->reg_local_ext) {
> lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
> data->reg_local_ext,
> - &data->temp11[4]);
> + &data->temp11[TEMP11_LOCAL_TEMP]);
> } else {
> if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
> &h) == 0)
> - data->temp11[4] = h << 8;
> + data->temp11[TEMP11_LOCAL_TEMP] = h << 8;
> }
> lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
> - LM90_REG_R_REMOTE_TEMPL, &data->temp11[0]);
> + LM90_REG_R_REMOTE_TEMPL,
> + &data->temp11[TEMP11_REMOTE_TEMP]);
Please don't break alignment.
>
> if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) {
> - data->temp11[1] = h << 8;
> + data->temp11[TEMP11_REMOTE_LOW] = h << 8;
> if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
> && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL,
> &l) == 0)
> - data->temp11[1] |= l;
> + data->temp11[TEMP11_REMOTE_LOW] |= l;
> }
> if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) {
> - data->temp11[2] = h << 8;
> + data->temp11[TEMP11_REMOTE_HIGH] = h << 8;
> if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
> && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL,
> &l) == 0)
> - data->temp11[2] |= l;
> + data->temp11[TEMP11_REMOTE_HIGH] |= l;
> }
>
> if (data->flags & LM90_HAVE_OFFSET) {
> @@ -529,13 +570,14 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> &h) == 0
> && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL,
> &l) == 0)
> - data->temp11[3] = (h << 8) | l;
> + data->temp11[TEMP11_REMOTE_OFFSET] =
> + (h << 8) | l;
> }
> if (data->flags & LM90_HAVE_EMERGENCY) {
> lm90_read_reg(client, MAX6659_REG_R_LOCAL_EMERG,
> - &data->temp8[4]);
> + &data->temp8[TEMP8_LOCAL_EMERG]);
> lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
> - &data->temp8[5]);
> + &data->temp8[TEMP8_REMOTE_EMERG]);
> }
> lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> data->alarms = alarms; /* save as 16 bit value */
> @@ -543,15 +585,16 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> if (data->kind == max6696) {
> lm90_select_remote_channel(client, data, 1);
> lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
> - &data->temp8[6]);
> + &data->temp8[TEMP8_REMOTE2_CRIT]);
> lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
> - &data->temp8[7]);
> + &data->temp8[TEMP8_REMOTE2_EMERG]);
> lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
> - LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]);
> + LM90_REG_R_REMOTE_TEMPL,
> + &data->temp11[TEMP11_REMOTE2_TEMP]);
Please don't break alignment.
> if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h))
> - data->temp11[6] = h << 8;
> + data->temp11[TEMP11_REMOTE2_LOW] = h << 8;
> if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h))
> - data->temp11[7] = h << 8;
> + data->temp11[TEMP11_REMOTE2_HIGH] = h << 8;
> lm90_select_remote_channel(client, data, 0);
>
> if (!lm90_read_reg(client, MAX6696_REG_R_STATUS2,
> @@ -745,7 +788,7 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
>
> static void write_temp8(struct device *dev, int index, long val)
> {
> - static const u8 reg[8] = {
> + static const u8 reg[TEMP8_REG_NUM] = {
> LM90_REG_W_LOCAL_LOW,
> LM90_REG_W_LOCAL_HIGH,
> LM90_REG_W_LOCAL_CRIT,
> @@ -828,7 +871,7 @@ static void write_temp11(struct device *dev, int nr, int index, long val)
> u8 high;
> u8 low;
> int channel;
> - } reg[5] = {
> + } reg[NR_NUM] = {
> { LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 0 },
> { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 0 },
> { LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL, 0 },
> @@ -919,11 +962,12 @@ static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy,
>
> mutex_lock(&data->update_lock);
> if (data->kind == adt7461)
> - temp = temp_from_u8_adt7461(data, data->temp8[2]);
> + temp = temp_from_u8_adt7461(data,
> + data->temp8[TEMP8_LOCAL_CRIT]);
Please align on opening parenthesis.
> else if (data->kind == max6646)
> - temp = temp_from_u8(data->temp8[2]);
> + temp = temp_from_u8(data->temp8[TEMP8_LOCAL_CRIT]);
> else
> - temp = temp_from_s8(data->temp8[2]);
> + temp = temp_from_s8(data->temp8[TEMP8_LOCAL_CRIT]);
>
> data->temp_hyst = hyst_to_reg(temp - val);
> i2c_smbus_write_byte_data(client, LM90_REG_W_TCRIT_HYST,
> @@ -977,25 +1021,28 @@ static ssize_t set_update_interval(struct device *dev,
> return count;
> }
>
> -static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, 0, 4);
> -static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, 0, 0);
> +static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL,
> + NR_CHAN_0_REMOTE_LOW, TEMP11_LOCAL_TEMP);
> +static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL,
> + NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_TEMP);
NR_CHAN_0_REMOTE_LOW makes no sense for read-only attributes, as the
number is only used in write_temp11(). The original "0" was only
because we can't leave the parameter empty.
> static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8,
> - set_temp8, 0);
> + set_temp8, TEMP8_LOCAL_LOW);
> static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11,
> - set_temp11, 0, 1);
> + set_temp11, NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_LOW);
> static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8,
> - set_temp8, 1);
> + set_temp8, TEMP8_LOCAL_HIGH);
> static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
> - set_temp11, 1, 2);
> + set_temp11, NR_CHAN_0_REMOTE_HIGH, TEMP11_REMOTE_HIGH);
> static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8,
> - set_temp8, 2);
> + set_temp8, TEMP8_LOCAL_CRIT);
> static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8,
> - set_temp8, 3);
> + set_temp8, TEMP8_REMOTE_CRIT);
> static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst,
> - set_temphyst, 2);
> -static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, 3);
> + set_temphyst, TEMP8_LOCAL_CRIT);
> +static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL,
> + TEMP8_REMOTE_CRIT);
> static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11,
> - set_temp11, 2, 3);
> + set_temp11, NR_CHAN_0_REMOTE_OFFSET, TEMP11_REMOTE_OFFSET);
>
> /* Individual alarm files */
> static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0);
> @@ -1043,13 +1090,13 @@ static const struct attribute_group lm90_group = {
> * Additional attributes for devices with emergency sensors
> */
> static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
> - set_temp8, 4);
> + set_temp8, TEMP8_LOCAL_EMERG);
> static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
> - set_temp8, 5);
> + set_temp8, TEMP8_REMOTE_EMERG);
> static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO, show_temphyst,
> - NULL, 4);
> + NULL, TEMP8_LOCAL_EMERG);
> static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO, show_temphyst,
> - NULL, 5);
> + NULL, TEMP8_REMOTE_EMERG);
>
> static struct attribute *lm90_emergency_attributes[] = {
> &sensor_dev_attr_temp1_emergency.dev_attr.attr,
> @@ -1079,18 +1126,20 @@ static const struct attribute_group lm90_emergency_alarm_group = {
> /*
> * Additional attributes for devices with 3 temperature sensors
> */
> -static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, 0, 5);
> +static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL,
> + NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE2_TEMP);
Here again this NR_CHAN_0_REMOTE_LOW makes no sense for a read-only
attribute, it was really "0" for "we don't care".
> static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
> - set_temp11, 3, 6);
> + set_temp11, NR_CHAN_1_REMOTE_LOW, TEMP11_REMOTE2_LOW);
> static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
> - set_temp11, 4, 7);
> + set_temp11, NR_CHAN_1_REMOTE_HIGH, TEMP11_REMOTE2_HIGH);
> 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, 6);
> + set_temp8, TEMP8_REMOTE2_CRIT);
> +static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL,
> + TEMP8_REMOTE2_CRIT);
> static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
> - set_temp8, 7);
> + set_temp8, TEMP8_REMOTE2_EMERG);
> static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO, show_temphyst,
> - NULL, 7);
> + NULL, TEMP8_REMOTE2_EMERG);
>
> 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);
Will all these changes done, I may consider apply the modified patch,
but no guarantee, as I'm still not sure I like it. I'll make my mind
when I see the result.
--
Jean Delvare
On 07/27/2013 11:02 PM, Jean Delvare wrote:
> Hi Wei,
>
> Sorry for the late reply.
>
> On Fri, 19 Jul 2013 14:41:54 +0800, Wei Ni wrote:
>> On 07/18/2013 11:58 PM, Jean Delvare wrote:
>>> First of all, how is the chip wired on your system? You are using an
>>> NCT1008, right? Which output of the chip is connected to your interrupt
>>> line, THERM or ALERT/THERM2? ALERT is normally used for SMBus Alert but
>>> I suppose it could be used for an interrupt too. THERM and THERM2 OTOH
>>> are comparator outputs, they stay low as long as the temperature are
>>> above the therm limits. Reading the status register won't bring them
>>> back up as I understand it, and contrary to the ALERT output, they
>>> can't be masked. Won't this result in an interrupt flood if using
>>> IRQF_TRIGGER_LOW? Have you tested your code already?
>>
>> Let me explain why I want to add the IRQ thread.
>> In our tegra30 platform, we use an NCT1008, and in our downstream tree,
>> it programmed as following:
>> 1. The pin THERM is connected to the PMIC, we will set the THERM limit
>> in the initialization, once the this limit is tripped, it will trigged
>> the PMIC, and the PMIC will shutdown the system immediately.
>
> OK, this is what the chip is designed for, good.
>
>> 2. The pin ALERT/THERM2 is configured as ALERT, and it is connected to
>> the interrupt line.
>
> Why don't you use the SMBus alert mechanism then? It is already
> implemented and allows you to reuse the interrupt of the SMBus
> controller.
>
> Of course this is a question for the hardware designers, not you... If
> the board uses a separate interrupt pin for the NCT1008's ALERT output,
> then the driver has to handle that interrupt explicitly, as done in your
> patch.
>
> One thing which worries me though is this explanation in the NCT1008
> datasheet:
>
> "The ALERT interrupt latch is not reset by reading the
> status register. It resets when the ALERT output has been
> serviced by the master reading the device address, provided
> the error condition has gone away and the status register flag
> bits are reset."
>
> This suggests that connecting the ALERT output to a separate interrupt
> pin will not work, as the ALERT output will never be de-asserted even
> if the fault conditions are gone and the status register was read. But
> as you say this is how your system is supposed to work, can you explain
> how it can work?
Yes, we had met this problems, to fix this issue, we enabled one-shot
mode in the bottom half handler of nct interrupts to force a
conversion/comparison. This effectively stops repeated nct interrupts.
We will do following things in the IRQ thread:
1. stand by the nct1008. (set configure register bit 6)
2. update the limit value if needed.
3. write to one-shot resister.
4. give hardware necessary time to finish conversion
5. run the nct1008 (clear configure register bit 6)
>
>> In the platform init, we will prepare some trip
>> temps, such as 20C, 40C,60C, 80C, and we will set 20C to the
>> remote-low-temp-limit, and set 40C to remote-high-temp-limit. When the
>> temperature exceed this rang, it will interrupt the system, then we will
>> update the low/high limit to next rang, for example, if the temp raise
>> to 50C, we will set 40C to low-limit, and set 60C to hight-limit, then
>> we will run the throttle functions, and update cooling state.
>
> Hu ho, I have seen this kind of design in the past, and I must say I
> don't quite like it. Moving critical thermal management handling to the
> software makes me feel unsafe. System thermal safety should not rely on
> the OS IMHO. It is best handled by the hardware, or if that can't be
> done, by the BIOS. And these limit updates are tricky, they could fail
> and then you're in trouble. Imagine for example that another chip on
> the SMBus hugs the bus and delays or even plain prevents the change of
> limits...
>
> But once again I guess you're not responsible for this.
These trip-temps are not critical temperature, we used these temps to
update cooling states. For the critical-temp, we handle it like my
mentioned in #1.
Yes, it's not safe to rely on the software, so on our tegra114, we will
have soc thermal, which can handle these trip-temps on hardware.
>
> Another problem with this design, even if no such problem happens, is
> that the limits are user(-space)-writable in the lm90 driver, while
> with your design these limits are under full control of the platform
> management code. You definitely don't want the user to come and adjust
> the limits, this could result in overheating of the system. So, do you
> have a plan to optionally switch limits to read only in the lm90
> driver? If not, how do you intend to solve the problem?
Yes, I had consider it, but didn't have good solution yet, because the
lm90 is a generic driver, it's difficult to add a new feature to switch
to read only or not.
>
> The more I think about it, the more I wonder if a custom thermal driver
> wouldn't be better for your case. Exposing a few read-only values to
> user-space through the thermal/hwmon bridge would IMHO make more sense
> than cluttering the lm90 driver with conditionals to limit what it
> exposes to user-space.
>
>> We wish to upstream these codes, but as you know, it's difficult to use
>> current lm90.c and thermal framework to implement it, and these codes
>> related many other things, such as throttling cpufreq, other clock freq.
>> So at first, I want to update the lm90.c, so that I can add it to the
>> thermal fw and use interrupt handler easily. And at the same time I
>> followed the thermal framework thread, there has new infrastructure
>> posted, which is more easy to add lm90 to thermal fw.
>
> Don't get me wrong, I'm very happy with your efforts to upstream your
> code, this is very welcome. And I am also very happy that you split it
> into small chunks which are easier to review. But I also need to know
> the big picture to see where you're ultimately going.
Actually, we tried to submit the nct1008.c very long ago, which included
all we want to do, and you had reviewed it :)
http://lists.lm-sensors.org/pipermail/lm-sensors/2011-April/032024.html
>
>>> (...)
>>> For a real system, if the THERM output is connected to an interrupt line
>>> (instead of directly to a fan controller) I would expect the platform
>>> to provide a callback to handle thermal events and take whatever
>>> appropriate measure is needed (e.g. throttling.) Just logging the
>>> problem won't save the system, by the time someone looks at the log it
>>> might be too late.
>>
>> In our downstream tree, we write a new driver nct1008.c, whci can handle
>> these interrupts and all other things, but as you know this driver can't
>> be upstreamed, because there already has the lm90.c :).
>> Anyway I think this patch is the first step of the implementation.
>
> I'm curious how the nct1008 driver "handles these interrupts." IMHO
> only the platform code knows what needs to be done upon reception of an
> interrupt, in particular in your case where you'll do some magic with the
> limit registers. There's no way this can be hard-coded in a hwmon
> driver, lm90 or any other.
Yes, absolutely agree, we can't add any private codes to the generic driver.
I had talked about it with Durgadoss in his "[PATCH] Thermal Framework
Enhancements" series, and in his v3 series, it introduced threshold
concept, which used to set limit value, and the "framework is notified
about this interrupt to take appropriate action". But this function
still didn't be completed yet.
I think the thermal fw can expose callback like thermal_zone->alert, and
in the irq_thread, we can notify the thermal fw to call this alert
callback function, then the platform code can do anything in this callback.
Thanks.
Wei.
>
On 07/27/2013 11:38 PM, Jean Delvare wrote:
> Hi Wei,
>
> On Fri, 12 Jul 2013 15:48:07 +0800, Wei Ni wrote:
>> Using enums for the indexes and nrs of temp8 and temp11.
>> This make the code much more readable.
>
> I can't say I'm thrilled by this patch. The improved readability is
> questionable. In the original code, each line already had one constant
> which made it clear what the code was doing. So the gain is marginal,
> I'd say, and this costs almost 50 lines of code.
>
> Let me review it nevertheless:
>
>>
>> Signed-off-by: Wei Ni <[email protected]>
>> ---
>> drivers/hwmon/lm90.c | 179 ++++++++++++++++++++++++++++++++------------------
>> 1 file changed, 114 insertions(+), 65 deletions(-)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index 1cc3d19..8cb5dd0 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -310,6 +310,59 @@ static const struct lm90_params lm90_params[] = {
>> };
>>
>> /*
>> + * TEMP8 register index
>> + */
>> +enum lm90_temp8_reg_index {
>> + TEMP8_LOCAL_LOW = 0, /* 0: local low limit */
>> + TEMP8_LOCAL_HIGH, /* 1: local high limit */
>> + TEMP8_LOCAL_CRIT, /* 2: local critical limit */
>> + TEMP8_REMOTE_CRIT, /* 3: remote critical limit */
>> + TEMP8_LOCAL_EMERG, /* 4: local emergency limit
>> + * (max6659 and max6695/96)
>> + */
>> + TEMP8_REMOTE_EMERG, /* 5: remote emergency limit
>> + * (max6659 and max6695/96)
>> + */
>> + TEMP8_REMOTE2_CRIT, /* 6: remote 2 critical limit
>> + * (max6695/96 only)
>> + */
>> + TEMP8_REMOTE2_EMERG, /* 7: remote 2 emergency limit
>> + * (max6695/96 only)
>> + */
>> + TEMP8_REG_NUM
>> +};
>> +
>> +/*
>> + * TEMP11 register index
>> + */
>> +enum lm90_temp11_reg_index {
>> + TEMP11_REMOTE_TEMP = 0, /* 0: remote input */
>> + TEMP11_REMOTE_LOW, /* 1: remote low limit */
>> + TEMP11_REMOTE_HIGH, /* 2: remote high limit */
>> + TEMP11_REMOTE_OFFSET, /* 3: remote offset
>> + * (except max6646, max6657/58/59,
>> + * and max6695/96)
>> + */
>> + TEMP11_LOCAL_TEMP, /* 4: local input */
>> + TEMP11_REMOTE2_TEMP, /* 5: remote 2 input (max6695/96 only) */
>> + TEMP11_REMOTE2_LOW, /* 6: remote 2 low limit (max6695/96 only) */
>> + TEMP11_REMOTE2_HIGH, /* 7: remote 2 high limit (max6695/96 only) */
>> + TEMP11_REG_NUM
>> +};
>
> The "TEMP8_" and "TEMP11_" prefixes aren't really needed, as there is no
> overlapping between both sets. Removing these prefixes (except for the
> terminators, where they are needed and make sense) would make the rest
> of the code more readable IMHO, as it would avoid redundant information
> on most lines, and avoid line splitting in some cases.
Yes, make sense, I will change it.
>
> Also, the comments are mostly useless now, they were there to document
> what each number was referring to, but now this is exactly what the new
> constants are doing.
Yes, we can remove these comments, but I think it's better to remain
those exception and only things.
>
>> +
>> +/*
>> + * TEMP11 register NR
>> + */
>> +enum lm90_temp11_reg_nr {
>> + NR_CHAN_0_REMOTE_LOW = 0, /* 0: channel 0, remote low limit */
>> + NR_CHAN_0_REMOTE_HIGH, /* 1: channel 0, remote high limit */
>> + NR_CHAN_0_REMOTE_OFFSET, /* 2: channel 0, remote offset */
>> + NR_CHAN_1_REMOTE_LOW, /* 3: channel 1, remote low limit */
>> + NR_CHAN_1_REMOTE_HIGH, /* 4: channel 1, remote high limit */
>
> The conventions used in the descriptions diverge from the ones used
> above. "channel 0 remote" here is just "remove" above, and "channel 1
> remote" is "remote 2" above. This is quite confusing.
>
>> + NR_NUM /* number of the NRs for temp11 */
>
> The fact that you were unable to come up with a proper name for this
> number is a clear indication that this enum should not exist in the
> first place.
>
> These numbers are used only once, to pass specific information to
> set_temp11. This was easy enough when these were just numbers and I
> really had no reason not to do that.
Ok, so how about to remove these changes, and keep the original codes to
use numbers.
>
> If you now want to use clean constants, this should be done with logic
> and consistency. Your proposal makes sense for the data->temp8 and
> data->temp11 array indexing, because they are used more than once. But
> introducing a new set of constants with weird names just for one use
> case doesn't help readability, it makes things worse actually.
>
> So if you insist of making the code more readable by the means of named
> constants, then you should simply adjust the reg array in write_temp11
> so that it can be indexed with your TEMP11_* constants above (as is
> data->temp11.) This will leave some holes in the array but I don't
> think we care about a few lost bytes here.
>
>> +};
>> +
>> +/*
>> * Client data (each client gets its own)
>> */
>>
>> @@ -331,25 +384,8 @@ struct lm90_data {
>> u8 reg_local_ext; /* local extension register offset */
>>
>> /* registers values */
>> - s8 temp8[8]; /* 0: local low limit
>> - * 1: local high limit
>> - * 2: local critical limit
>> - * 3: remote critical limit
>> - * 4: local emergency limit (max6659 and max6695/96)
>> - * 5: remote emergency limit (max6659 and max6695/96)
>> - * 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, max6657/58/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 (max6695/96 only)
>> - */
>> + s8 temp8[TEMP8_REG_NUM];
>> + s16 temp11[TEMP11_REG_NUM];
>> u8 temp_hyst;
>> u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
>> };
>> @@ -491,37 +527,42 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>> u8 alarms;
>>
>> dev_dbg(&client->dev, "Updating lm90 data.\n");
>> - lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]);
>> - lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH, &data->temp8[1]);
>> - lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT, &data->temp8[2]);
>> - lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]);
>> + lm90_read_reg(client, LM90_REG_R_LOCAL_LOW,
>> + &data->temp8[TEMP8_LOCAL_LOW]);
>> + lm90_read_reg(client, LM90_REG_R_LOCAL_HIGH,
>> + &data->temp8[TEMP8_LOCAL_HIGH]);
>> + lm90_read_reg(client, LM90_REG_R_LOCAL_CRIT,
>> + &data->temp8[TEMP8_LOCAL_CRIT]);
>> + lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
>> + &data->temp8[TEMP8_REMOTE_CRIT]);
>> lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst);
>>
>> if (data->reg_local_ext) {
>> lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
>> data->reg_local_ext,
>> - &data->temp11[4]);
>> + &data->temp11[TEMP11_LOCAL_TEMP]);
>> } else {
>> if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
>> &h) == 0)
>> - data->temp11[4] = h << 8;
>> + data->temp11[TEMP11_LOCAL_TEMP] = h << 8;
>> }
>> lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
>> - LM90_REG_R_REMOTE_TEMPL, &data->temp11[0]);
>> + LM90_REG_R_REMOTE_TEMPL,
>> + &data->temp11[TEMP11_REMOTE_TEMP]);
>
> Please don't break alignment.
Yes, I will do it.
>
>>
>> if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) {
>> - data->temp11[1] = h << 8;
>> + data->temp11[TEMP11_REMOTE_LOW] = h << 8;
>> if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
>> && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL,
>> &l) == 0)
>> - data->temp11[1] |= l;
>> + data->temp11[TEMP11_REMOTE_LOW] |= l;
>> }
>> if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) {
>> - data->temp11[2] = h << 8;
>> + data->temp11[TEMP11_REMOTE_HIGH] = h << 8;
>> if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
>> && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL,
>> &l) == 0)
>> - data->temp11[2] |= l;
>> + data->temp11[TEMP11_REMOTE_HIGH] |= l;
>> }
>>
>> if (data->flags & LM90_HAVE_OFFSET) {
>> @@ -529,13 +570,14 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>> &h) == 0
>> && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL,
>> &l) == 0)
>> - data->temp11[3] = (h << 8) | l;
>> + data->temp11[TEMP11_REMOTE_OFFSET] =
>> + (h << 8) | l;
>> }
>> if (data->flags & LM90_HAVE_EMERGENCY) {
>> lm90_read_reg(client, MAX6659_REG_R_LOCAL_EMERG,
>> - &data->temp8[4]);
>> + &data->temp8[TEMP8_LOCAL_EMERG]);
>> lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
>> - &data->temp8[5]);
>> + &data->temp8[TEMP8_REMOTE_EMERG]);
>> }
>> lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
>> data->alarms = alarms; /* save as 16 bit value */
>> @@ -543,15 +585,16 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>> if (data->kind == max6696) {
>> lm90_select_remote_channel(client, data, 1);
>> lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
>> - &data->temp8[6]);
>> + &data->temp8[TEMP8_REMOTE2_CRIT]);
>> lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
>> - &data->temp8[7]);
>> + &data->temp8[TEMP8_REMOTE2_EMERG]);
>> lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
>> - LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]);
>> + LM90_REG_R_REMOTE_TEMPL,
>> + &data->temp11[TEMP11_REMOTE2_TEMP]);
>
> Please don't break alignment.
>
>> if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h))
>> - data->temp11[6] = h << 8;
>> + data->temp11[TEMP11_REMOTE2_LOW] = h << 8;
>> if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h))
>> - data->temp11[7] = h << 8;
>> + data->temp11[TEMP11_REMOTE2_HIGH] = h << 8;
>> lm90_select_remote_channel(client, data, 0);
>>
>> if (!lm90_read_reg(client, MAX6696_REG_R_STATUS2,
>> @@ -745,7 +788,7 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
>>
>> static void write_temp8(struct device *dev, int index, long val)
>> {
>> - static const u8 reg[8] = {
>> + static const u8 reg[TEMP8_REG_NUM] = {
>> LM90_REG_W_LOCAL_LOW,
>> LM90_REG_W_LOCAL_HIGH,
>> LM90_REG_W_LOCAL_CRIT,
>> @@ -828,7 +871,7 @@ static void write_temp11(struct device *dev, int nr, int index, long val)
>> u8 high;
>> u8 low;
>> int channel;
>> - } reg[5] = {
>> + } reg[NR_NUM] = {
>> { LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 0 },
>> { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 0 },
>> { LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL, 0 },
>> @@ -919,11 +962,12 @@ static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy,
>>
>> mutex_lock(&data->update_lock);
>> if (data->kind == adt7461)
>> - temp = temp_from_u8_adt7461(data, data->temp8[2]);
>> + temp = temp_from_u8_adt7461(data,
>> + data->temp8[TEMP8_LOCAL_CRIT]);
>
> Please align on opening parenthesis.
>
>> else if (data->kind == max6646)
>> - temp = temp_from_u8(data->temp8[2]);
>> + temp = temp_from_u8(data->temp8[TEMP8_LOCAL_CRIT]);
>> else
>> - temp = temp_from_s8(data->temp8[2]);
>> + temp = temp_from_s8(data->temp8[TEMP8_LOCAL_CRIT]);
>>
>> data->temp_hyst = hyst_to_reg(temp - val);
>> i2c_smbus_write_byte_data(client, LM90_REG_W_TCRIT_HYST,
>> @@ -977,25 +1021,28 @@ static ssize_t set_update_interval(struct device *dev,
>> return count;
>> }
>>
>> -static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, 0, 4);
>> -static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, 0, 0);
>> +static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL,
>> + NR_CHAN_0_REMOTE_LOW, TEMP11_LOCAL_TEMP);
>> +static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL,
>> + NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_TEMP);
>
> NR_CHAN_0_REMOTE_LOW makes no sense for read-only attributes, as the
> number is only used in write_temp11(). The original "0" was only
> because we can't leave the parameter empty.
>
>> static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8,
>> - set_temp8, 0);
>> + set_temp8, TEMP8_LOCAL_LOW);
>> static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11,
>> - set_temp11, 0, 1);
>> + set_temp11, NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE_LOW);
>> static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8,
>> - set_temp8, 1);
>> + set_temp8, TEMP8_LOCAL_HIGH);
>> static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
>> - set_temp11, 1, 2);
>> + set_temp11, NR_CHAN_0_REMOTE_HIGH, TEMP11_REMOTE_HIGH);
>> static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8,
>> - set_temp8, 2);
>> + set_temp8, TEMP8_LOCAL_CRIT);
>> static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8,
>> - set_temp8, 3);
>> + set_temp8, TEMP8_REMOTE_CRIT);
>> static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst,
>> - set_temphyst, 2);
>> -static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, 3);
>> + set_temphyst, TEMP8_LOCAL_CRIT);
>> +static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL,
>> + TEMP8_REMOTE_CRIT);
>> static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11,
>> - set_temp11, 2, 3);
>> + set_temp11, NR_CHAN_0_REMOTE_OFFSET, TEMP11_REMOTE_OFFSET);
>>
>> /* Individual alarm files */
>> static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0);
>> @@ -1043,13 +1090,13 @@ static const struct attribute_group lm90_group = {
>> * Additional attributes for devices with emergency sensors
>> */
>> static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
>> - set_temp8, 4);
>> + set_temp8, TEMP8_LOCAL_EMERG);
>> static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
>> - set_temp8, 5);
>> + set_temp8, TEMP8_REMOTE_EMERG);
>> static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO, show_temphyst,
>> - NULL, 4);
>> + NULL, TEMP8_LOCAL_EMERG);
>> static SENSOR_DEVICE_ATTR(temp2_emergency_hyst, S_IRUGO, show_temphyst,
>> - NULL, 5);
>> + NULL, TEMP8_REMOTE_EMERG);
>>
>> static struct attribute *lm90_emergency_attributes[] = {
>> &sensor_dev_attr_temp1_emergency.dev_attr.attr,
>> @@ -1079,18 +1126,20 @@ static const struct attribute_group lm90_emergency_alarm_group = {
>> /*
>> * Additional attributes for devices with 3 temperature sensors
>> */
>> -static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, 0, 5);
>> +static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL,
>> + NR_CHAN_0_REMOTE_LOW, TEMP11_REMOTE2_TEMP);
>
> Here again this NR_CHAN_0_REMOTE_LOW makes no sense for a read-only
> attribute, it was really "0" for "we don't care".
>
>> static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
>> - set_temp11, 3, 6);
>> + set_temp11, NR_CHAN_1_REMOTE_LOW, TEMP11_REMOTE2_LOW);
>> static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
>> - set_temp11, 4, 7);
>> + set_temp11, NR_CHAN_1_REMOTE_HIGH, TEMP11_REMOTE2_HIGH);
>> 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, 6);
>> + set_temp8, TEMP8_REMOTE2_CRIT);
>> +static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL,
>> + TEMP8_REMOTE2_CRIT);
>> static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
>> - set_temp8, 7);
>> + set_temp8, TEMP8_REMOTE2_EMERG);
>> static SENSOR_DEVICE_ATTR(temp3_emergency_hyst, S_IRUGO, show_temphyst,
>> - NULL, 7);
>> + NULL, TEMP8_REMOTE2_EMERG);
>>
>> 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);
>
> Will all these changes done, I may consider apply the modified patch,
> but no guarantee, as I'm still not sure I like it. I'll make my mind
> when I see the result.
I really appreciate you reviewed my patches so carefully.
I will update changes in my next version.
Thanks.
Wei.
>
> --
> Jean Delvare
>
Hi Wei,
On Mon, 29 Jul 2013 19:15:12 +0800, Wei Ni wrote:
> On 07/27/2013 11:38 PM, Jean Delvare wrote:
> > On Fri, 12 Jul 2013 15:48:07 +0800, Wei Ni wrote:
> >> +/*
> >> + * TEMP11 register index
> >> + */
> >> +enum lm90_temp11_reg_index {
> >> + TEMP11_REMOTE_TEMP = 0, /* 0: remote input */
> >> + TEMP11_REMOTE_LOW, /* 1: remote low limit */
> >> + TEMP11_REMOTE_HIGH, /* 2: remote high limit */
> >> + TEMP11_REMOTE_OFFSET, /* 3: remote offset
> >> + * (except max6646, max6657/58/59,
> >> + * and max6695/96)
> >> + */
> >> + TEMP11_LOCAL_TEMP, /* 4: local input */
> >> + TEMP11_REMOTE2_TEMP, /* 5: remote 2 input (max6695/96 only) */
> >> + TEMP11_REMOTE2_LOW, /* 6: remote 2 low limit (max6695/96 only) */
> >> + TEMP11_REMOTE2_HIGH, /* 7: remote 2 high limit (max6695/96 only) */
> >> + TEMP11_REG_NUM
> >> +};
> > (...)
> > Also, the comments are mostly useless now, they were there to document
> > what each number was referring to, but now this is exactly what the new
> > constants are doing.
>
> Yes, we can remove these comments, but I think it's better to remain
> those exception and only things.
Yes, I agree.
> >> +
> >> +/*
> >> + * TEMP11 register NR
> >> + */
> >> +enum lm90_temp11_reg_nr {
> >> + NR_CHAN_0_REMOTE_LOW = 0, /* 0: channel 0, remote low limit */
> >> + NR_CHAN_0_REMOTE_HIGH, /* 1: channel 0, remote high limit */
> >> + NR_CHAN_0_REMOTE_OFFSET, /* 2: channel 0, remote offset */
> >> + NR_CHAN_1_REMOTE_LOW, /* 3: channel 1, remote low limit */
> >> + NR_CHAN_1_REMOTE_HIGH, /* 4: channel 1, remote high limit */
> >
> > The conventions used in the descriptions diverge from the ones used
> > above. "channel 0 remote" here is just "remove" above, and "channel 1
> > remote" is "remote 2" above. This is quite confusing.
> >
> >> + NR_NUM /* number of the NRs for temp11 */
> >
> > The fact that you were unable to come up with a proper name for this
> > number is a clear indication that this enum should not exist in the
> > first place.
> >
> > These numbers are used only once, to pass specific information to
> > set_temp11. This was easy enough when these were just numbers and I
> > really had no reason not to do that.
>
> Ok, so how about to remove these changes, and keep the original codes to
> use numbers.
Fine with me. We can always change the code later to use the TEMP11
index values instead if anyone cares, this can be done separately.
--
Jean Delvare
Hi Wei,
On Mon, 29 Jul 2013 18:14:56 +0800, Wei Ni wrote:
> On 07/27/2013 11:02 PM, Jean Delvare wrote:
> > On Fri, 19 Jul 2013 14:41:54 +0800, Wei Ni wrote:
> >> 2. The pin ALERT/THERM2 is configured as ALERT, and it is connected to
> >> the interrupt line.
> >
> > Why don't you use the SMBus alert mechanism then? It is already
> > implemented and allows you to reuse the interrupt of the SMBus
> > controller.
> >
> > Of course this is a question for the hardware designers, not you... If
> > the board uses a separate interrupt pin for the NCT1008's ALERT output,
> > then the driver has to handle that interrupt explicitly, as done in your
> > patch.
> >
> > One thing which worries me though is this explanation in the NCT1008
> > datasheet:
> >
> > "The ALERT interrupt latch is not reset by reading the
> > status register. It resets when the ALERT output has been
> > serviced by the master reading the device address, provided
> > the error condition has gone away and the status register flag
> > bits are reset."
> >
> > This suggests that connecting the ALERT output to a separate interrupt
> > pin will not work, as the ALERT output will never be de-asserted even
> > if the fault conditions are gone and the status register was read. But
> > as you say this is how your system is supposed to work, can you explain
> > how it can work?
>
> Yes, we had met this problems, to fix this issue, we enabled one-shot
> mode in the bottom half handler of nct interrupts to force a
> conversion/comparison. This effectively stops repeated nct interrupts.
> We will do following things in the IRQ thread:
> 1. stand by the nct1008. (set configure register bit 6)
> 2. update the limit value if needed.
> 3. write to one-shot resister.
> 4. give hardware necessary time to finish conversion
> 5. run the nct1008 (clear configure register bit 6)
Doh, this is so ugly :(
Why don't you configure the pin as THERM2 instead of ALERT then? I'd
expect this to make things easier.
> (...)
> These trip-temps are not critical temperature, we used these temps to
> update cooling states. For the critical-temp, we handle it like my
> mentioned in #1.
I understand. But even if these interrupts are only used for managing
cooling states, a misbehavior could still have annoying consequences,
such as causing the thermal shutdown to trigger when this could have
been avoided, or throttling to stay enabled even though the system has
cooled down enough.
> Yes, it's not safe to rely on the software, so on our tegra114, we will
> have soc thermal, which can handle these trip-temps on hardware.
OK, good :)
> (...)
> Yes, absolutely agree, we can't add any private codes to the generic driver.
> I had talked about it with Durgadoss in his "[PATCH] Thermal Framework
> Enhancements" series, and in his v3 series, it introduced threshold
> concept, which used to set limit value, and the "framework is notified
> about this interrupt to take appropriate action". But this function
> still didn't be completed yet.
> I think the thermal fw can expose callback like thermal_zone->alert, and
> in the irq_thread, we can notify the thermal fw to call this alert
> callback function, then the platform code can do anything in this callback.
I didn't follow the discussions closely, but something like this would
be needed, yes.
--
Jean Delvare
On 07/29/2013 11:58 PM, Jean Delvare wrote:
> Hi Wei,
>
> On Mon, 29 Jul 2013 18:14:56 +0800, Wei Ni wrote:
>> On 07/27/2013 11:02 PM, Jean Delvare wrote:
>>> On Fri, 19 Jul 2013 14:41:54 +0800, Wei Ni wrote:
>>>> 2. The pin ALERT/THERM2 is configured as ALERT, and it is connected to
>>>> the interrupt line.
>>>
>>> Why don't you use the SMBus alert mechanism then? It is already
>>> implemented and allows you to reuse the interrupt of the SMBus
>>> controller.
>>>
>>> Of course this is a question for the hardware designers, not you... If
>>> the board uses a separate interrupt pin for the NCT1008's ALERT output,
>>> then the driver has to handle that interrupt explicitly, as done in your
>>> patch.
>>>
>>> One thing which worries me though is this explanation in the NCT1008
>>> datasheet:
>>>
>>> "The ALERT interrupt latch is not reset by reading the
>>> status register. It resets when the ALERT output has been
>>> serviced by the master reading the device address, provided
>>> the error condition has gone away and the status register flag
>>> bits are reset."
>>>
>>> This suggests that connecting the ALERT output to a separate interrupt
>>> pin will not work, as the ALERT output will never be de-asserted even
>>> if the fault conditions are gone and the status register was read. But
>>> as you say this is how your system is supposed to work, can you explain
>>> how it can work?
>>
>> Yes, we had met this problems, to fix this issue, we enabled one-shot
>> mode in the bottom half handler of nct interrupts to force a
>> conversion/comparison. This effectively stops repeated nct interrupts.
>> We will do following things in the IRQ thread:
>> 1. stand by the nct1008. (set configure register bit 6)
>> 2. update the limit value if needed.
>> 3. write to one-shot resister.
>> 4. give hardware necessary time to finish conversion
>> 5. run the nct1008 (clear configure register bit 6)
>
> Doh, this is so ugly :(
>
> Why don't you configure the pin as THERM2 instead of ALERT then? I'd
> expect this to make things easier.
If configure as THERM2, only the high temperature limits are relevant,
so when the temperature reduced, it will not trigger interrupt, and we
can't update the cooling state.
Or do you mean that we can configure the pin to THERM2 in the irq_thread
to avoid the repeated interrupt ? I tried it, but no help, the nct1008
will not run the conversion/comparison immediately, so the status
register will not be cleared.
>
>> (...)
>> These trip-temps are not critical temperature, we used these temps to
>> update cooling states. For the critical-temp, we handle it like my
>> mentioned in #1.
>
> I understand. But even if these interrupts are only used for managing
> cooling states, a misbehavior could still have annoying consequences,
> such as causing the thermal shutdown to trigger when this could have
> been avoided, or throttling to stay enabled even though the system has
> cooled down enough.
I think our driver are trying best to avoid these troubles. As I know in
our downstream codes, we didn't met these things.
I think since the lm90 support interrupt mode, then the driver should
have related interface to handle it, and it can call the callback
function to do what the platform driver want.
Thanks.
Wei.
>
>> Yes, it's not safe to rely on the software, so on our tegra114, we will
>> have soc thermal, which can handle these trip-temps on hardware.
>
> OK, good :)
>
>> (...)
>> Yes, absolutely agree, we can't add any private codes to the generic driver.
>> I had talked about it with Durgadoss in his "[PATCH] Thermal Framework
>> Enhancements" series, and in his v3 series, it introduced threshold
>> concept, which used to set limit value, and the "framework is notified
>> about this interrupt to take appropriate action". But this function
>> still didn't be completed yet.
>> I think the thermal fw can expose callback like thermal_zone->alert, and
>> in the irq_thread, we can notify the thermal fw to call this alert
>> callback function, then the platform code can do anything in this callback.
>
> I didn't follow the discussions closely, but something like this would
> be needed, yes.
>
Hi Wei,
Sorry for the late reply, catching up with the discussions from before
my vacation...
On Tue, 30 Jul 2013 16:18:35 +0800, Wei Ni wrote:
> On 07/29/2013 11:58 PM, Jean Delvare wrote:
> > On Mon, 29 Jul 2013 18:14:56 +0800, Wei Ni wrote:
> >> Yes, we had met this problems, to fix this issue, we enabled one-shot
> >> mode in the bottom half handler of nct interrupts to force a
> >> conversion/comparison. This effectively stops repeated nct interrupts.
> >> We will do following things in the IRQ thread:
> >> 1. stand by the nct1008. (set configure register bit 6)
> >> 2. update the limit value if needed.
> >> 3. write to one-shot resister.
> >> 4. give hardware necessary time to finish conversion
> >> 5. run the nct1008 (clear configure register bit 6)
> >
> > Doh, this is so ugly :(
> >
> > Why don't you configure the pin as THERM2 instead of ALERT then? I'd
> > expect this to make things easier.
>
> If configure as THERM2, only the high temperature limits are relevant,
> so when the temperature reduced, it will not trigger interrupt, and we
> can't update the cooling state.
Ah, indeed, I had not noticed this restriction.
> Or do you mean that we can configure the pin to THERM2 in the irq_thread
> to avoid the repeated interrupt ? I tried it, but no help, the nct1008
> will not run the conversion/comparison immediately, so the status
> register will not be cleared.
No, I didn't mean to suggest anything like that.
> >> (...)
> >> These trip-temps are not critical temperature, we used these temps to
> >> update cooling states. For the critical-temp, we handle it like my
> >> mentioned in #1.
> >
> > I understand. But even if these interrupts are only used for managing
> > cooling states, a misbehavior could still have annoying consequences,
> > such as causing the thermal shutdown to trigger when this could have
> > been avoided, or throttling to stay enabled even though the system has
> > cooled down enough.
>
> I think our driver are trying best to avoid these troubles. As I know in
> our downstream codes, we didn't met these things.
> I think since the lm90 support interrupt mode, then the driver should
> have related interface to handle it, and it can call the callback
> function to do what the platform driver want.
Yes, fair enough. I do not object to it, I was only trying to
understand how you were using it.
--
Jean Delvare
Hi Guenter,
On Mon, 15 Jul 2013 10:33:22 -0700, Guenter Roeck wrote:
> On Mon, Jul 15, 2013 at 06:57:27PM +0200, Jean Delvare wrote:
> > Unrelated to this patch, but Guenter, I am worried about the MAX6696
> > handling here. I realize that I am the one who accepted your code, but
> > now it looks wrong. Specifically:
> > * We check for (status2 & 0xfe) i.e. 7 alarm bits, but the code below
> > only reports 2 alarms bits. So if any of the 5 other alarm bits in
> > STATUS2 are, we may return true (chip is tripped) but not print the
> > cause.
> > * At least bits 1 and 2 of STATUS 2 fit totally fine in the driver as
> > it currently exists, so I can't think of any reason for not handling
> > them. Why are we not? Ideally we should print a message for every
> > alarm bit so that we never return "true" without printing a message.
> > Even though OT2 limits aren't handled by the driver...
> > * If you think this piece of code shouldn't deal with OT/THERM limits
> > because they do not trigger an SMBus alarm, this can be discussed,
> > but all chips should be handled the same in this respect then.
> > * Why in the first place is max6696's data->alert_alarms set to 0x187c
> > and not 0x1c7c? Including 1OPEN but not 2OPEN makes no sense.
>
> I am about to leave for vacation, so this will have to wait for a couple of
> weeks. I'll look at it after I am back.
Are you back now? ;-)
--
Jean Delvare
On Wed, Oct 30, 2013 at 04:33:26PM +0100, Jean Delvare wrote:
> Hi Guenter,
>
> On Mon, 15 Jul 2013 10:33:22 -0700, Guenter Roeck wrote:
> > On Mon, Jul 15, 2013 at 06:57:27PM +0200, Jean Delvare wrote:
> > > Unrelated to this patch, but Guenter, I am worried about the MAX6696
> > > handling here. I realize that I am the one who accepted your code, but
> > > now it looks wrong. Specifically:
> > > * We check for (status2 & 0xfe) i.e. 7 alarm bits, but the code below
> > > only reports 2 alarms bits. So if any of the 5 other alarm bits in
> > > STATUS2 are, we may return true (chip is tripped) but not print the
> > > cause.
> > > * At least bits 1 and 2 of STATUS 2 fit totally fine in the driver as
> > > it currently exists, so I can't think of any reason for not handling
> > > them. Why are we not? Ideally we should print a message for every
> > > alarm bit so that we never return "true" without printing a message.
> > > Even though OT2 limits aren't handled by the driver...
> > > * If you think this piece of code shouldn't deal with OT/THERM limits
> > > because they do not trigger an SMBus alarm, this can be discussed,
> > > but all chips should be handled the same in this respect then.
> > > * Why in the first place is max6696's data->alert_alarms set to 0x187c
> > > and not 0x1c7c? Including 1OPEN but not 2OPEN makes no sense.
> >
> > I am about to leave for vacation, so this will have to wait for a couple of
> > weeks. I'll look at it after I am back.
>
> Are you back now? ;-)
>
Yes, only I completely forgot about this :-). I'll add it to my task list.
Guenter
On Wed, Oct 30, 2013 at 04:33:26PM +0100, Jean Delvare wrote:
> Hi Guenter,
>
> On Mon, 15 Jul 2013 10:33:22 -0700, Guenter Roeck wrote:
> > On Mon, Jul 15, 2013 at 06:57:27PM +0200, Jean Delvare wrote:
> > > Unrelated to this patch, but Guenter, I am worried about the MAX6696
> > > handling here. I realize that I am the one who accepted your code, but
> > > now it looks wrong. Specifically:
> > > * We check for (status2 & 0xfe) i.e. 7 alarm bits, but the code below
> > > only reports 2 alarms bits. So if any of the 5 other alarm bits in
> > > STATUS2 are, we may return true (chip is tripped) but not print the
> > > cause.
Agreed, that doesn't make much sense, especially since we already check
for R1OT1 and display a message if it is set. I'll add checks for the other
bits.
> > > * At least bits 1 and 2 of STATUS 2 fit totally fine in the driver as
> > > it currently exists, so I can't think of any reason for not handling
> > > them. Why are we not? Ideally we should print a message for every
> > > alarm bit so that we never return "true" without printing a message.
> > > Even though OT2 limits aren't handled by the driver...
They actually are, through MAX6659_REG_R_REMOTE_EMERG and LM90_HAVE_EMERGENCY.
> > > * If you think this piece of code shouldn't deal with OT/THERM limits
> > > because they do not trigger an SMBus alarm, this can be discussed,
Yes, that was the logic. Presumably OT1 and OT2 are separate interrupts
(if connected to interrupt pins) and would have to be handled separately.
> > > but all chips should be handled the same in this respect then.
Agreed.
> > > * Why in the first place is max6696's data->alert_alarms set to 0x187c
> > > and not 0x1c7c? Including 1OPEN but not 2OPEN makes no sense.
Oversight. 2OPEN does trigger ALERT, so the bit should be set.
I'll send a patch in a minute.
Guenter