Please find the inlined patches according to your feedback. I've also fixed some typos and added a mutex_lock/unlock in in_alarm_show().
See also my replies to your questions in the previous mail below
From b663ac0dda5968ce7b43a55576639b5b28b2ebbb Mon Sep 17 00:00:00 2001
From: Gilles Buloz <gilles.buloz@xxxxxxxxxxx>
Date: Wed, 27 Nov 2019 18:09:34 +0100
Subject: [PATCH 1/2] hwmon: (nct7802) Fix voltage limits written to wrong
registers
in0 thresholds are written to the in2 thresholds registers
in2 thresholds to in3 thresholds
in3 thresholds to in4 thresholds
in4 thresholds to in0 thresholds
Signed-off-by: Gilles Buloz <gilles.buloz@xxxxxxxxxxx>
---
drivers/hwmon/nct7802.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c
index f3dd2a17bd42..7915c2f2c85d 100644
--- a/drivers/hwmon/nct7802.c
+++ b/drivers/hwmon/nct7802.c
@@ -23,8 +23,8 @@
static const u8 REG_VOLTAGE[5] = { 0x09, 0x0a, 0x0c, 0x0d, 0x0e };
static const u8 REG_VOLTAGE_LIMIT_LSB[2][5] = {
- { 0x40, 0x00, 0x42, 0x44, 0x46 },
- { 0x3f, 0x00, 0x41, 0x43, 0x45 },
+ { 0x46, 0x00, 0x40, 0x42, 0x44 },
+ { 0x45, 0x00, 0x3f, 0x41, 0x43 },
};
static const u8 REG_VOLTAGE_LIMIT_MSB[5] = { 0x48, 0x00, 0x47, 0x47, 0x48 };
--
2.17.1
From 24ac3207f7cfd0411aa47b0c48b5c657bc0daa48 Mon Sep 17 00:00:00 2001
From: Gilles Buloz <gilles.buloz@xxxxxxxxxxx>
Date: Wed, 27 Nov 2019 19:20:41 +0100
Subject: [PATCH 2/2] hwmon: (nct7802) Fix non-working alarm on voltages
No alarm is reported by /sys/.../inX_alarm
In detail:
The SMI Voltage status register is the only register giving a status
for voltages, but it does not work like the non-SMI status registers
used for temperatures and fans.
A bit is set for each input crossing a threshold, in both direction,
but the "inside" or "outside" limits info is not available.
Also this register is cleared on read.
Note : this is not explicitly spelled out in the datasheet, but from
experiment.
As a result if an input is crossing a threshold (min or max in any
direction), the alarm is reported only once even if the input is
still outside limits. Also if the alarm for another input is read
before the one of this input, no alarm is reported at all.
Signed-off-by: Gilles Buloz <gilles.buloz@xxxxxxxxxxx>
---
drivers/hwmon/nct7802.c | 71 ++++++++++++++++++++++++++++++++++++++---
1 file changed, 67 insertions(+), 4 deletions(-)
diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c
index 7915c2f2c85d..c558845deff4 100644
--- a/drivers/hwmon/nct7802.c
+++ b/drivers/hwmon/nct7802.c
@@ -58,6 +58,8 @@ static const u8 REG_VOLTAGE_LIMIT_MSB_SHIFT[2][5] = {
struct nct7802_data {
struct regmap *regmap;
struct mutex access_lock; /* for multi-byte read and write operations */
+ u8 in_status;
+ struct mutex in_alarm_lock;
};
static ssize_t temp_type_show(struct device *dev,
@@ -368,6 +370,66 @@ static ssize_t in_store(struct device *dev, struct device_attribute *attr,
return err ? : count;
}
+static ssize_t in_alarm_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+ struct nct7802_data *data = dev_get_drvdata(dev);
+ int volt, min, max, ret, i;
+ unsigned int val;
+
+ /*
+ * The SMI Voltage status register is the only register giving a status
+ * for voltages. A bit is set for each input crossing a threshold, in
+ * both direction, but the "inside" or "outside" limits info is not
+ * available. Also this register is cleared on read.
+ * Note : this is not explicitly spelled out in the datasheet, but
+ * from experiment.
+ * To deal with this we use a status cache with one validity bit and
+ * one status bit for each input. Validity is cleared at startup and
+ * each time the register reports a change, and the status is processed
+ * by software based on current input value and limits.
+ */
+ ret = regmap_read(data->regmap, 0x1e, &val); /* SMI Voltage status */
+ if (ret < 0)
+ return ret;
+
+ mutex_lock(&data->in_alarm_lock);
+
+ /* if status of an input has changed, invalidate its cached status */
+ for (i = 0; i <= 3; i++)
+ if (val & (1 << i))
+ data->in_status &= ~(0x10 << i);
+
+ /* if cached status for requested input is invalid, update it */
+ if (!(data->in_status & (0x10 << sattr->index))) {
+ mutex_unlock(&data->in_alarm_lock);
+
+ volt = nct7802_read_voltage(data, sattr->nr, 0);
+ if (volt < 0)
+ return volt;
+ min = nct7802_read_voltage(data, sattr->nr, 1);
+ if (min < 0)
+ return min;
+ max = nct7802_read_voltage(data, sattr->nr, 2);
+ if (max < 0)
+ return max;
+
+ mutex_lock(&data->in_alarm_lock);
+
+ if (volt < min || volt > max)
+ data->in_status |= (1 << sattr->index);
+ else
+ data->in_status &= ~(1 << sattr->index);
+
+ data->in_status |= 0x10 << sattr->index;
+ }
+
+ mutex_unlock(&data->in_alarm_lock);
+
+ return sprintf(buf, "%u\n", !!(data->in_status & (1 << sattr->index)));
+}
+
static ssize_t temp_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
@@ -660,7 +722,7 @@ static const struct attribute_group nct7802_temp_group = {
static SENSOR_DEVICE_ATTR_2_RO(in0_input, in, 0, 0);
static SENSOR_DEVICE_ATTR_2_RW(in0_min, in, 0, 1);
static SENSOR_DEVICE_ATTR_2_RW(in0_max, in, 0, 2);
-static SENSOR_DEVICE_ATTR_2_RO(in0_alarm, alarm, 0x1e, 3);
+static SENSOR_DEVICE_ATTR_2_RO(in0_alarm, in_alarm, 0, 3);
static SENSOR_DEVICE_ATTR_2_RW(in0_beep, beep, 0x5a, 3);
static SENSOR_DEVICE_ATTR_2_RO(in1_input, in, 1, 0);
@@ -668,19 +730,19 @@ static SENSOR_DEVICE_ATTR_2_RO(in1_input, in, 1, 0);
static SENSOR_DEVICE_ATTR_2_RO(in2_input, in, 2, 0);
static SENSOR_DEVICE_ATTR_2_RW(in2_min, in, 2, 1);
static SENSOR_DEVICE_ATTR_2_RW(in2_max, in, 2, 2);
-static SENSOR_DEVICE_ATTR_2_RO(in2_alarm, alarm, 0x1e, 0);
+static SENSOR_DEVICE_ATTR_2_RO(in2_alarm, in_alarm, 2, 0);
static SENSOR_DEVICE_ATTR_2_RW(in2_beep, beep, 0x5a, 0);
static SENSOR_DEVICE_ATTR_2_RO(in3_input, in, 3, 0);
static SENSOR_DEVICE_ATTR_2_RW(in3_min, in, 3, 1);
static SENSOR_DEVICE_ATTR_2_RW(in3_max, in, 3, 2);
-static SENSOR_DEVICE_ATTR_2_RO(in3_alarm, alarm, 0x1e, 1);
+static SENSOR_DEVICE_ATTR_2_RO(in3_alarm, in_alarm, 3, 1);
static SENSOR_DEVICE_ATTR_2_RW(in3_beep, beep, 0x5a, 1);
static SENSOR_DEVICE_ATTR_2_RO(in4_input, in, 4, 0);
static SENSOR_DEVICE_ATTR_2_RW(in4_min, in, 4, 1);
static SENSOR_DEVICE_ATTR_2_RW(in4_max, in, 4, 2);
-static SENSOR_DEVICE_ATTR_2_RO(in4_alarm, alarm, 0x1e, 2);
+static SENSOR_DEVICE_ATTR_2_RO(in4_alarm, in_alarm, 4, 2);
static SENSOR_DEVICE_ATTR_2_RW(in4_beep, beep, 0x5a, 2);
static struct attribute *nct7802_in_attrs[] = {
@@ -1011,6 +1073,7 @@ static int nct7802_probe(struct i2c_client *client,
return PTR_ERR(data->regmap);
mutex_init(&data->access_lock);
+ mutex_init(&data->in_alarm_lock);
ret = nct7802_init_chip(data);
if (ret < 0)
--
2.17.1
Le 27/11/2019 20:32, Guenter Roeck a écrit :
> On 11/27/19 6:41 AM, Gilles Buloz wrote:
>> According to your suggestions, I've made and tested this patch that works :
>>
>> --- nct7802.c.orig 2019-11-26 10:37:08.753693088 +0100
>> +++ nct7802.c 2019-11-27 15:15:51.000000000 +0100
>> @@ -32,8 +32,8 @@
>> static const u8 REG_VOLTAGE[5] = { 0x09, 0x0a, 0x0c, 0x0d, 0x0e };
>>
>> static const u8 REG_VOLTAGE_LIMIT_LSB[2][5] = {
>> - { 0x40, 0x00, 0x42, 0x44, 0x46 },
>> - { 0x3f, 0x00, 0x41, 0x43, 0x45 },
>> + { 0x46, 0x00, 0x40, 0x42, 0x44 },
>> + { 0x45, 0x00, 0x3f, 0x41, 0x43 },
>> };
>>
>> static const u8 REG_VOLTAGE_LIMIT_MSB[5] = { 0x48, 0x00, 0x47, 0x47, 0x48 };
>> @@ -67,6 +67,7 @@
>> struct nct7802_data {
>> struct regmap *regmap;
>> struct mutex access_lock; /* for multi-byte read and write operations */
>> + u8 in_status_cache;
>> };
>>
>> static ssize_t show_temp_type(struct device *dev, struct device_attribute *attr,
>> @@ -377,6 +378,56 @@
>> return err ? : count;
>> }
>>
>> +static ssize_t show_in_alarm(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
>> + struct nct7802_data *data = dev_get_drvdata(dev);
>> + int volt, min, max, ret, i;
>> + unsigned int val;
>> +
>> + /*
>> + * The SMI Voltage statys register is the only register giving a status
>> + * for volatges. A bit is set for each input crossing a threshold, in
>> + * both direction, but the "inside" or "outside" limits info is not
>> + * available. Also this register is cleared on read.
>
> Might add a note that this is form experiment, and not explicitly spelled
> out in the datasheet.
done
>
>> + * To deal with this we use a status cache with one validity bit and
>> + * one status bit for each input. Validity is cleared at startup and
>> + * each time the register reports a change, and the status is processed
>> + * by software based on current value and limits.
>> + */
>> + ret = regmap_read(data->regmap, 0x1E, &val); /* SMI Voltage status */
>
> We are using lowercase for hex numbers elsewhere in this driver,
> so please use lowercase here as well.
done
>
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* if status of an input has changed, invalidate its cached status */
>> + for (i=0; i<=3; i++)
>
> Spaces before and after assignments, please.
done (checkpatch already told that to me :-) )
>
>> + if (val & (1 << i))
>> + data->in_status_cache &= ~(0x10 << i);
>> +
>> + /* if cached status for requested input is invalid, update it */
>> + if (!(data->in_status_cache & (0x10 << sattr->index))) {
>> + volt = nct7802_read_voltage(data, sattr->nr, 0);
>> + if (volt < 0)
>> + return volt;
>> + min = nct7802_read_voltage(data, sattr->nr, 1);
>> + if (min < 0)
>> + return min;
>
> Do we need to check min ? The register description suggests "over limit".
Yes I know, but my experiments show that the bits in SMI Voltage status register are also set when crossing the "min" threshold.
I think that's why the datasheet does not show any register for "under limit".
> No idea though what the min limits are for in that case. Might be worth
Yes, it's usefull to know a system is powered from a voltage near the minimum because the risk of going under is increased and we
must warn the user about that. Any small glitch under the min may lead to unexpected system reset.
> running some checks to understand this better (if you did not do that
> already).
Sure I did that : I changed all voltage thresholds successively to have min above value or max below value in sensors.conf, then ran
"sensors -s" to update the thresholds, then ran "sensors" several times to make sure ALARM is displayed. Then switched them back to
normal values one by one to make sure ALARM is no more displayed. I also tried to load the driver with voltages already outside
limits to make sure ALARM is reported.
I did this on kernel 4.17.19 because I had no hardware running 5.4, but did that by backporting my patch that is for the latest
kernel to make sure this patch is correct.
>
>> + max = nct7802_read_voltage(data, sattr->nr, 2);
>> + if (max < 0)
>> + return max;
>> +
>> + if ((volt < min) || (volt > max))
>
> Unnecessary inner ( )
done
>
> Guenter
> .
>
