hwmon: (nct7802) buggy VSEN1/2/3 alarm

hwmon: (nct7802) buggy VSEN1/2/3 alarm

[Gilles Buloz]

Hi Guenter,

According to the NCT7802Y datasheet, the REG_VOLTAGE_LIMIT_LSB definition is wrong and leads to wrong threshold registers used. It 
should be :
static const u8 REG_VOLTAGE_LIMIT_LSB[2][5] = {
         { 0x46, 0x00, 0x40, 0x42, 0x44 },
         { 0x45, 0x00, 0x3f, 0x41, 0x43 },
};
With this definition, the right bit is set in "Voltage SMI Status Register @0x1e" for each threshold reached (using i2cget to check)

But I'm unable to get any "ALARM" reported by the command "sensors" for VSEN1/2/3 = in2,in3,in4 because status for in0 is read 
before (unless I set "ignore in0" in sensors file). The problem is that status bits in "Voltage SMI Status Register @0x1e" are 
cleared when reading, and a read is done for each inX processed, so only the first inX has a chance to get its alarm bit set.
For this problem I don't see how to fix this easily; just to let you know ...

Best regards

Gilles BULOZ
Senior software(/hardware) engineer | R&D
Kontron Modular Computers S.A.S.

Re: hwmon: (nct7802) buggy VSEN1/2/3 alarm

[Guenter Roeck]

On 11/25/19 5:13 AM, Gilles Buloz wrote:
> Hi Guenter,
> 
> According to the NCT7802Y datasheet, the REG_VOLTAGE_LIMIT_LSB definition is wrong and leads to wrong threshold registers used. It
> should be :
> static const u8 REG_VOLTAGE_LIMIT_LSB[2][5] = {
>           { 0x46, 0x00, 0x40, 0x42, 0x44 },
>           { 0x45, 0x00, 0x3f, 0x41, 0x43 },
> };
> With this definition, the right bit is set in "Voltage SMI Status Register @0x1e" for each threshold reached (using i2cget to check)
> 

Good catch. Care to send a patch ?

> But I'm unable to get any "ALARM" reported by the command "sensors" for VSEN1/2/3 = in2,in3,in4 because status for in0 is read
> before (unless I set "ignore in0" in sensors file). The problem is that status bits in "Voltage SMI Status Register @0x1e" are
> cleared when reading, and a read is done for each inX processed, so only the first inX has a chance to get its alarm bit set.
> For this problem I don't see how to fix this easily; just to let you know ...
> 
One possible fix would be to cache each alarm register and to clear the cache
either after reading it (bitwise) or after a timeout. The latter is probably
better to avoid stale information.

Guenter

Re: hwmon: (nct7802) buggy VSEN1/2/3 alarm

[Gilles Buloz]

Le 25/11/2019 15:31, Guenter Roeck a écrit :
> On 11/25/19 5:13 AM, Gilles Buloz wrote:
>> Hi Guenter,
>>
>> According to the NCT7802Y datasheet, the REG_VOLTAGE_LIMIT_LSB definition is wrong and leads to wrong threshold registers used. It
>> should be :
>> static const u8 REG_VOLTAGE_LIMIT_LSB[2][5] = {
>>           { 0x46, 0x00, 0x40, 0x42, 0x44 },
>>           { 0x45, 0x00, 0x3f, 0x41, 0x43 },
>> };
>> With this definition, the right bit is set in "Voltage SMI Status Register @0x1e" for each threshold reached (using i2cget to check)
>>
>
> Good catch. Care to send a patch ?
As a fix for this is only useful with a fix for the problem below, maybe a single patch for both would be better.
>> But I'm unable to get any "ALARM" reported by the command "sensors" for VSEN1/2/3 = in2,in3,in4 because status for in0 is read
>> before (unless I set "ignore in0" in sensors file). The problem is that status bits in "Voltage SMI Status Register @0x1e" are
>> cleared when reading, and a read is done for each inX processed, so only the first inX has a chance to get its alarm bit set.
>> For this problem I don't see how to fix this easily; just to let you know ...
>>
> One possible fix would be to cache each alarm register and to clear the cache
> either after reading it (bitwise) or after a timeout. The latter is probably
> better to avoid stale information.
As we have status registers cleared at byte level and we want them to be cleared at bit level when each bit is read, I think a cache 
would be better. I suggest this :
- have a cached value for each status register, by default at 0x00
- when reading a register to get a bit, "OR" its byte value with its cached value, then use its cached value for processing.
- then clear the bit that has been processed from the cached value.

I think a timeout would not be obvious to set : at least the time for sensors to read all info (including when terminal is a serial 
line and output is slower) and to deal with possible latencies, but not too long...
>
> Guenter
> .
>

Re: hwmon: (nct7802) buggy VSEN1/2/3 alarm

[Guenter Roeck]

On Mon, Nov 25, 2019 at 04:44:44PM +0000, Gilles Buloz wrote:
> Le 25/11/2019 15:31, Guenter Roeck a écrit :
> > On 11/25/19 5:13 AM, Gilles Buloz wrote:
> >> Hi Guenter,
> >>
> >> According to the NCT7802Y datasheet, the REG_VOLTAGE_LIMIT_LSB definition is wrong and leads to wrong threshold registers used. It
> >> should be :
> >> static const u8 REG_VOLTAGE_LIMIT_LSB[2][5] = {
> >>           { 0x46, 0x00, 0x40, 0x42, 0x44 },
> >>           { 0x45, 0x00, 0x3f, 0x41, 0x43 },
> >> };
> >> With this definition, the right bit is set in "Voltage SMI Status Register @0x1e" for each threshold reached (using i2cget to check)
> >>
> >
> > Good catch. Care to send a patch ?
> As a fix for this is only useful with a fix for the problem below, maybe a single patch for both would be better.

Not really. Those are two separate issues. The reported and selected
limits are wrong, period. This will require two patches.

> >> But I'm unable to get any "ALARM" reported by the command "sensors" for VSEN1/2/3 = in2,in3,in4 because status for in0 is read
> >> before (unless I set "ignore in0" in sensors file). The problem is that status bits in "Voltage SMI Status Register @0x1e" are
> >> cleared when reading, and a read is done for each inX processed, so only the first inX has a chance to get its alarm bit set.
> >> For this problem I don't see how to fix this easily; just to let you know ...
> >>
> > One possible fix would be to cache each alarm register and to clear the cache
> > either after reading it (bitwise) or after a timeout. The latter is probably
> > better to avoid stale information.
> As we have status registers cleared at byte level and we want them to be cleared at bit level when each bit is read, I think a cache 
> would be better. I suggest this :
> - have a cached value for each status register, by default at 0x00
> - when reading a register to get a bit, "OR" its byte value with its cached value, then use its cached value for processing.
> - then clear the bit that has been processed from the cached value.
> 
Both methods I suggested would have to involve a cache. The question is
when to clear the cache - either clear a bit after reporting it, or
clear it after a timeout.

> I think a timeout would not be obvious to set : at least the time for sensors to read all info (including when terminal is a serial 
> line and output is slower) and to deal with possible latencies, but not too long...

The timeout would be determined by the chip's conversion rate (register 0x26),
or, for simplicity, just be set to one second. I don't immediately see why
that would be difficult to implement. Not that it matters much, really;
I would accept patches with and without timeout. 

Guenter

Re: hwmon: (nct7802) buggy VSEN1/2/3 alarm

[Gilles Buloz]

Le 25/11/2019 18:35, Guenter Roeck a écrit :
> On Mon, Nov 25, 2019 at 04:44:44PM +0000, Gilles Buloz wrote:
>> Le 25/11/2019 15:31, Guenter Roeck a écrit :
>>> On 11/25/19 5:13 AM, Gilles Buloz wrote:
>>>> Hi Guenter,
>>>>
>>>> According to the NCT7802Y datasheet, the REG_VOLTAGE_LIMIT_LSB definition is wrong and leads to wrong threshold registers used. It
>>>> should be :
>>>> static const u8 REG_VOLTAGE_LIMIT_LSB[2][5] = {
>>>>            { 0x46, 0x00, 0x40, 0x42, 0x44 },
>>>>            { 0x45, 0x00, 0x3f, 0x41, 0x43 },
>>>> };
>>>> With this definition, the right bit is set in "Voltage SMI Status Register @0x1e" for each threshold reached (using i2cget to check)
>>>>
>>> Good catch. Care to send a patch ?
>> As a fix for this is only useful with a fix for the problem below, maybe a single patch for both would be better.
> Not really. Those are two separate issues. The reported and selected
> limits are wrong, period. This will require two patches.
OK
>>>> But I'm unable to get any "ALARM" reported by the command "sensors" for VSEN1/2/3 = in2,in3,in4 because status for in0 is read
>>>> before (unless I set "ignore in0" in sensors file). The problem is that status bits in "Voltage SMI Status Register @0x1e" are
>>>> cleared when reading, and a read is done for each inX processed, so only the first inX has a chance to get its alarm bit set.
>>>> For this problem I don't see how to fix this easily; just to let you know ...
>>>>
>>> One possible fix would be to cache each alarm register and to clear the cache
>>> either after reading it (bitwise) or after a timeout. The latter is probably
>>> better to avoid stale information.
>> As we have status registers cleared at byte level and we want them to be cleared at bit level when each bit is read, I think a cache
>> would be better. I suggest this :
>> - have a cached value for each status register, by default at 0x00
>> - when reading a register to get a bit, "OR" its byte value with its cached value, then use its cached value for processing.
>> - then clear the bit that has been processed from the cached value.
>>
> Both methods I suggested would have to involve a cache. The question is
> when to clear the cache - either clear a bit after reporting it, or
> clear it after a timeout.
>
>> I think a timeout would not be obvious to set : at least the time for sensors to read all info (including when terminal is a serial
>> line and output is slower) and to deal with possible latencies, but not too long...
> The timeout would be determined by the chip's conversion rate (register 0x26),
As I understand, the status must be kept in cache between the first read of status register @1E for in0 (clearing all status bits), 
and the last read for in4. This duration depends on the "sensors" execution time and I'can see any link with the conversion rate here.
> or, for simplicity, just be set to one second. I don't immediately see why
> that would be difficult to implement. Not that it matters much, really;
> I would accept patches with and without timeout.
>
> Guenter
> .
>

Re: hwmon: (nct7802) buggy VSEN1/2/3 alarm

[Gilles Buloz]

I have a functional patch (see below), but before going further (split and cleanup), I would like to have your opinion on how the 
NCT7802Y handles the thresholds status.
Except for temperatures and in "comparator interrupt mode", the status bits are NOT set after each ADC conversion, but only once 
when crossing a threshold. So an alarm for a threshold is reported only to the first process reading the status and not to the others.
For instance if you run "sensors" you only get "ALARM" once the nothing until the threshold is crossed again in the other direction. 
Maybe the expected behaviour would be to display "ALARM" as long as we are outside the thresholds, not only once.

--- nct7802.c.orig    2019-11-25 22:17:04.845718422 +0100
+++ nct7802.c    2019-11-25 23:22:00.905387154 +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 };
@@ -60,6 +60,9 @@
  #define REG_CHIP_ID        0xfe
  #define REG_VERSION_ID        0xff

+#define REG_CACHE_START        0x17
+#define REG_CACHE_END        0x20
+
  /*
   * Data structures and manipulation thereof
   */
@@ -67,6 +70,7 @@
  struct nct7802_data {
      struct regmap *regmap;
      struct mutex access_lock; /* for multi-byte read and write operations */
+    u8 reg_cache[REG_CACHE_END - REG_CACHE_START + 1];
  };

  static ssize_t show_temp_type(struct device *dev, struct device_attribute *attr,
@@ -467,6 +471,15 @@  static ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
      if (ret < 0)
          return ret;

+    /*
+     * For registers cleared on read, use a cache to keep all bits
+     * that are set until they are returned to the caller
+     */
+    if ((sattr->nr >= REG_CACHE_START) && (sattr->nr <= REG_CACHE_END)) {
+        val |= data->reg_cache[sattr->nr - REG_CACHE_START];
+        data->reg_cache[sattr->nr - REG_CACHE_START] = val & ~(1 << bit);
+    }
+
      return sprintf(buf, "%u\n", !!(val & (1 << bit)));
  }

Le 25/11/2019 19:06, Gilles BULOZ a écrit :
> Le 25/11/2019 18:35, Guenter Roeck a écrit :
>> On Mon, Nov 25, 2019 at 04:44:44PM +0000, Gilles Buloz wrote:
>>> Le 25/11/2019 15:31, Guenter Roeck a écrit :
>>>> On 11/25/19 5:13 AM, Gilles Buloz wrote:
>>>>> Hi Guenter,
>>>>>
>>>>> According to the NCT7802Y datasheet, the REG_VOLTAGE_LIMIT_LSB definition is wrong and leads to wrong threshold registers 
>>>>> used. It
>>>>> should be :
>>>>> static const u8 REG_VOLTAGE_LIMIT_LSB[2][5] = {
>>>>>            { 0x46, 0x00, 0x40, 0x42, 0x44 },
>>>>>            { 0x45, 0x00, 0x3f, 0x41, 0x43 },
>>>>> };
>>>>> With this definition, the right bit is set in "Voltage SMI Status Register @0x1e" for each threshold reached (using i2cget to 
>>>>> check)
>>>>>
>>>> Good catch. Care to send a patch ?
>>> As a fix for this is only useful with a fix for the problem below, maybe a single patch for both would be better.
>> Not really. Those are two separate issues. The reported and selected
>> limits are wrong, period. This will require two patches.
> OK
>>>>> But I'm unable to get any "ALARM" reported by the command "sensors" for VSEN1/2/3 = in2,in3,in4 because status for in0 is read
>>>>> before (unless I set "ignore in0" in sensors file). The problem is that status bits in "Voltage SMI Status Register @0x1e" are
>>>>> cleared when reading, and a read is done for each inX processed, so only the first inX has a chance to get its alarm bit set.
>>>>> For this problem I don't see how to fix this easily; just to let you know ...
>>>>>
>>>> One possible fix would be to cache each alarm register and to clear the cache
>>>> either after reading it (bitwise) or after a timeout. The latter is probably
>>>> better to avoid stale information.
>>> As we have status registers cleared at byte level and we want them to be cleared at bit level when each bit is read, I think a 
>>> cache
>>> would be better. I suggest this :
>>> - have a cached value for each status register, by default at 0x00
>>> - when reading a register to get a bit, "OR" its byte value with its cached value, then use its cached value for processing.
>>> - then clear the bit that has been processed from the cached value.
>>>
>> Both methods I suggested would have to involve a cache. The question is
>> when to clear the cache - either clear a bit after reporting it, or
>> clear it after a timeout.
>>
>>> I think a timeout would not be obvious to set : at least the time for sensors to read all info (including when terminal is a serial
>>> line and output is slower) and to deal with possible latencies, but not too long...
>> The timeout would be determined by the chip's conversion rate (register 0x26),
> As I understand, the status must be kept in cache between the first read of status register @1E for in0 (clearing all status 
> bits), and the last read for in4. This duration depends on the "sensors" execution time and I'can see any link with the conversion 
> rate here.
>> or, for simplicity, just be set to one second. I don't immediately see why
>> that would be difficult to implement. Not that it matters much, really;
>> I would accept patches with and without timeout.
>>
>> Guenter
>> .
>>
>

Re: hwmon: (nct7802) buggy VSEN1/2/3 alarm

[Guenter Roeck]

On 11/26/19 2:03 AM, Gilles Buloz wrote:
> I have a functional patch (see below), but before going further (split and cleanup), I would like to have your opinion on how the
> NCT7802Y handles the thresholds status.
> Except for temperatures and in "comparator interrupt mode", the status bits are NOT set after each ADC conversion, but only once
> when crossing a threshold. So an alarm for a threshold is reported only to the first process reading the status and not to the others.
> For instance if you run "sensors" you only get "ALARM" once the nothing until the threshold is crossed again in the other direction.
> Maybe the expected behaviour would be to display "ALARM" as long as we are outside the thresholds, not only once.
> 

Yes, that is the expected behavior.

Guenter

> --- nct7802.c.orig    2019-11-25 22:17:04.845718422 +0100
> +++ nct7802.c    2019-11-25 23:22:00.905387154 +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 };
> @@ -60,6 +60,9 @@
>    #define REG_CHIP_ID        0xfe
>    #define REG_VERSION_ID        0xff
> 
> +#define REG_CACHE_START        0x17
> +#define REG_CACHE_END        0x20
> +
>    /*
>     * Data structures and manipulation thereof
>     */
> @@ -67,6 +70,7 @@
>    struct nct7802_data {
>        struct regmap *regmap;
>        struct mutex access_lock; /* for multi-byte read and write operations */
> +    u8 reg_cache[REG_CACHE_END - REG_CACHE_START + 1];
>    };
> 
>    static ssize_t show_temp_type(struct device *dev, struct device_attribute *attr,
> @@ -467,6 +471,15 @@  static ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
>        if (ret < 0)
>            return ret;
> 
> +    /*
> +     * For registers cleared on read, use a cache to keep all bits
> +     * that are set until they are returned to the caller
> +     */
> +    if ((sattr->nr >= REG_CACHE_START) && (sattr->nr <= REG_CACHE_END)) {
> +        val |= data->reg_cache[sattr->nr - REG_CACHE_START];
> +        data->reg_cache[sattr->nr - REG_CACHE_START] = val & ~(1 << bit);
> +    }
> +
>        return sprintf(buf, "%u\n", !!(val & (1 << bit)));
>    }
> 
> Le 25/11/2019 19:06, Gilles BULOZ a écrit :
>> Le 25/11/2019 18:35, Guenter Roeck a écrit :
>>> On Mon, Nov 25, 2019 at 04:44:44PM +0000, Gilles Buloz wrote:
>>>> Le 25/11/2019 15:31, Guenter Roeck a écrit :
>>>>> On 11/25/19 5:13 AM, Gilles Buloz wrote:
>>>>>> Hi Guenter,
>>>>>>
>>>>>> According to the NCT7802Y datasheet, the REG_VOLTAGE_LIMIT_LSB definition is wrong and leads to wrong threshold registers
>>>>>> used. It
>>>>>> should be :
>>>>>> static const u8 REG_VOLTAGE_LIMIT_LSB[2][5] = {
>>>>>>             { 0x46, 0x00, 0x40, 0x42, 0x44 },
>>>>>>             { 0x45, 0x00, 0x3f, 0x41, 0x43 },
>>>>>> };
>>>>>> With this definition, the right bit is set in "Voltage SMI Status Register @0x1e" for each threshold reached (using i2cget to
>>>>>> check)
>>>>>>
>>>>> Good catch. Care to send a patch ?
>>>> As a fix for this is only useful with a fix for the problem below, maybe a single patch for both would be better.
>>> Not really. Those are two separate issues. The reported and selected
>>> limits are wrong, period. This will require two patches.
>> OK
>>>>>> But I'm unable to get any "ALARM" reported by the command "sensors" for VSEN1/2/3 = in2,in3,in4 because status for in0 is read
>>>>>> before (unless I set "ignore in0" in sensors file). The problem is that status bits in "Voltage SMI Status Register @0x1e" are
>>>>>> cleared when reading, and a read is done for each inX processed, so only the first inX has a chance to get its alarm bit set.
>>>>>> For this problem I don't see how to fix this easily; just to let you know ...
>>>>>>
>>>>> One possible fix would be to cache each alarm register and to clear the cache
>>>>> either after reading it (bitwise) or after a timeout. The latter is probably
>>>>> better to avoid stale information.
>>>> As we have status registers cleared at byte level and we want them to be cleared at bit level when each bit is read, I think a
>>>> cache
>>>> would be better. I suggest this :
>>>> - have a cached value for each status register, by default at 0x00
>>>> - when reading a register to get a bit, "OR" its byte value with its cached value, then use its cached value for processing.
>>>> - then clear the bit that has been processed from the cached value.
>>>>
>>> Both methods I suggested would have to involve a cache. The question is
>>> when to clear the cache - either clear a bit after reporting it, or
>>> clear it after a timeout.
>>>
>>>> I think a timeout would not be obvious to set : at least the time for sensors to read all info (including when terminal is a serial
>>>> line and output is slower) and to deal with possible latencies, but not too long...
>>> The timeout would be determined by the chip's conversion rate (register 0x26),
>> As I understand, the status must be kept in cache between the first read of status register @1E for in0 (clearing all status
>> bits), and the last read for in4. This duration depends on the "sensors" execution time and I'can see any link with the conversion
>> rate here.
>>> or, for simplicity, just be set to one second. I don't immediately see why
>>> that would be difficult to implement. Not that it matters much, really;
>>> I would accept patches with and without timeout.
>>>
>>> Guenter
>>> .
>>>
>>
> 

Re: hwmon: (nct7802) buggy VSEN1/2/3 alarm

[Gilles Buloz]

OK, so to have "ALARM" reported as long as we are outside limits, I did not find another method than checking against limits by 
software, but still clear the related status register to have a working interrupt.
The patch below is working for voltages.
If you're OK, I can extend it to the temperatures and fans

--- nct7802.c.orig    2019-11-26 10:37:08.753693088 +0100
+++ nct7802.c    2019-11-26 17:27:56.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 };
@@ -377,6 +377,32 @@
      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;
+    unsigned int val;
+
+    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;
+
+    /* also clear related status register to have functional interrupt */
+    ret = regmap_read(data->regmap, sattr->index, &val);
+    if (ret < 0)
+        return ret;
+
+    return sprintf(buf, "%u\n", (volt < min) || (volt > max));
+}
+
  static ssize_t show_temp(struct device *dev, struct device_attribute *attr,
               char *buf)
  {
@@ -714,7 +740,7 @@
                  0, 1);
  static SENSOR_DEVICE_ATTR_2(in0_max, S_IRUGO | S_IWUSR, show_in, store_in,
                  0, 2);
-static SENSOR_DEVICE_ATTR_2(in0_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 3);
+static SENSOR_DEVICE_ATTR_2(in0_alarm, S_IRUGO, show_in_alarm, NULL, 0, 0x1e);
  static SENSOR_DEVICE_ATTR_2(in0_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
                  0x5a, 3);

@@ -725,7 +751,7 @@
                  2, 1);
  static SENSOR_DEVICE_ATTR_2(in2_max, S_IRUGO | S_IWUSR, show_in, store_in,
                  2, 2);
-static SENSOR_DEVICE_ATTR_2(in2_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 0);
+static SENSOR_DEVICE_ATTR_2(in2_alarm, S_IRUGO, show_in_alarm, NULL, 2, 0x1e);
  static SENSOR_DEVICE_ATTR_2(in2_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
                  0x5a, 0);

@@ -734,7 +760,7 @@
                  3, 1);
  static SENSOR_DEVICE_ATTR_2(in3_max, S_IRUGO | S_IWUSR, show_in, store_in,
                  3, 2);
-static SENSOR_DEVICE_ATTR_2(in3_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 1);
+static SENSOR_DEVICE_ATTR_2(in3_alarm, S_IRUGO, show_in_alarm, NULL, 3, 0x1e);
  static SENSOR_DEVICE_ATTR_2(in3_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
                  0x5a, 1);

@@ -743,7 +769,7 @@
                  4, 1);
  static SENSOR_DEVICE_ATTR_2(in4_max, S_IRUGO | S_IWUSR, show_in, store_in,
                  4, 2);
-static SENSOR_DEVICE_ATTR_2(in4_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 2);
+static SENSOR_DEVICE_ATTR_2(in4_alarm, S_IRUGO, show_in_alarm, NULL, 4, 0x1e);
  static SENSOR_DEVICE_ATTR_2(in4_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
                  0x5a, 2);


Le 26/11/2019 13:22, Guenter Roeck a écrit :
> On 11/26/19 2:03 AM, Gilles Buloz wrote:
>> I have a functional patch (see below), but before going further (split and cleanup), I would like to have your opinion on how the
>> NCT7802Y handles the thresholds status.
>> Except for temperatures and in "comparator interrupt mode", the status bits are NOT set after each ADC conversion, but only once
>> when crossing a threshold. So an alarm for a threshold is reported only to the first process reading the status and not to the 
>> others.
>> For instance if you run "sensors" you only get "ALARM" once the nothing until the threshold is crossed again in the other direction.
>> Maybe the expected behaviour would be to display "ALARM" as long as we are outside the thresholds, not only once.
>>
>
> Yes, that is the expected behavior.
>
> Guenter
>
>> --- nct7802.c.orig    2019-11-25 22:17:04.845718422 +0100
>> +++ nct7802.c    2019-11-25 23:22:00.905387154 +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 };
>> @@ -60,6 +60,9 @@
>>    #define REG_CHIP_ID        0xfe
>>    #define REG_VERSION_ID        0xff
>>
>> +#define REG_CACHE_START        0x17
>> +#define REG_CACHE_END        0x20
>> +
>>    /*
>>     * Data structures and manipulation thereof
>>     */
>> @@ -67,6 +70,7 @@
>>    struct nct7802_data {
>>        struct regmap *regmap;
>>        struct mutex access_lock; /* for multi-byte read and write operations */
>> +    u8 reg_cache[REG_CACHE_END - REG_CACHE_START + 1];
>>    };
>>
>>    static ssize_t show_temp_type(struct device *dev, struct device_attribute *attr,
>> @@ -467,6 +471,15 @@  static ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
>>        if (ret < 0)
>>            return ret;
>>
>> +    /*
>> +     * For registers cleared on read, use a cache to keep all bits
>> +     * that are set until they are returned to the caller
>> +     */
>> +    if ((sattr->nr >= REG_CACHE_START) && (sattr->nr <= REG_CACHE_END)) {
>> +        val |= data->reg_cache[sattr->nr - REG_CACHE_START];
>> +        data->reg_cache[sattr->nr - REG_CACHE_START] = val & ~(1 << bit);
>> +    }
>> +
>>        return sprintf(buf, "%u\n", !!(val & (1 << bit)));
>>    }
>>
>> Le 25/11/2019 19:06, Gilles BULOZ a écrit :
>>> Le 25/11/2019 18:35, Guenter Roeck a écrit :
>>>> On Mon, Nov 25, 2019 at 04:44:44PM +0000, Gilles Buloz wrote:
>>>>> Le 25/11/2019 15:31, Guenter Roeck a écrit :
>>>>>> On 11/25/19 5:13 AM, Gilles Buloz wrote:
>>>>>>> Hi Guenter,
>>>>>>>
>>>>>>> According to the NCT7802Y datasheet, the REG_VOLTAGE_LIMIT_LSB definition is wrong and leads to wrong threshold registers
>>>>>>> used. It
>>>>>>> should be :
>>>>>>> static const u8 REG_VOLTAGE_LIMIT_LSB[2][5] = {
>>>>>>>             { 0x46, 0x00, 0x40, 0x42, 0x44 },
>>>>>>>             { 0x45, 0x00, 0x3f, 0x41, 0x43 },
>>>>>>> };
>>>>>>> With this definition, the right bit is set in "Voltage SMI Status Register @0x1e" for each threshold reached (using i2cget to
>>>>>>> check)
>>>>>>>
>>>>>> Good catch. Care to send a patch ?
>>>>> As a fix for this is only useful with a fix for the problem below, maybe a single patch for both would be better.
>>>> Not really. Those are two separate issues. The reported and selected
>>>> limits are wrong, period. This will require two patches.
>>> OK
>>>>>>> But I'm unable to get any "ALARM" reported by the command "sensors" for VSEN1/2/3 = in2,in3,in4 because status for in0 is read
>>>>>>> before (unless I set "ignore in0" in sensors file). The problem is that status bits in "Voltage SMI Status Register @0x1e" are
>>>>>>> cleared when reading, and a read is done for each inX processed, so only the first inX has a chance to get its alarm bit set.
>>>>>>> For this problem I don't see how to fix this easily; just to let you know ...
>>>>>>>
>>>>>> One possible fix would be to cache each alarm register and to clear the cache
>>>>>> either after reading it (bitwise) or after a timeout. The latter is probably
>>>>>> better to avoid stale information.
>>>>> As we have status registers cleared at byte level and we want them to be cleared at bit level when each bit is read, I think a
>>>>> cache
>>>>> would be better. I suggest this :
>>>>> - have a cached value for each status register, by default at 0x00
>>>>> - when reading a register to get a bit, "OR" its byte value with its cached value, then use its cached value for processing.
>>>>> - then clear the bit that has been processed from the cached value.
>>>>>
>>>> Both methods I suggested would have to involve a cache. The question is
>>>> when to clear the cache - either clear a bit after reporting it, or
>>>> clear it after a timeout.
>>>>
>>>>> I think a timeout would not be obvious to set : at least the time for sensors to read all info (including when terminal is a 
>>>>> serial
>>>>> line and output is slower) and to deal with possible latencies, but not too long...
>>>> The timeout would be determined by the chip's conversion rate (register 0x26),
>>> As I understand, the status must be kept in cache between the first read of status register @1E for in0 (clearing all status
>>> bits), and the last read for in4. This duration depends on the "sensors" execution time and I'can see any link with the conversion
>>> rate here.
>>>> or, for simplicity, just be set to one second. I don't immediately see why
>>>> that would be difficult to implement. Not that it matters much, really;
>>>> I would accept patches with and without timeout.
>>>>
>>>> Guenter
>>>> .
>>>>
>>>
>>
>
> .
>

Re: hwmon: (nct7802) buggy VSEN1/2/3 alarm

[Guenter Roeck]

On Tue, Nov 26, 2019 at 04:47:47PM +0000, Gilles Buloz wrote:
> OK, so to have "ALARM" reported as long as we are outside limits, I did not find another method than checking against limits by 
> software, but still clear the related status register to have a working interrupt.
> The patch below is working for voltages.
> If you're OK, I can extend it to the temperatures and fans
> 
> --- nct7802.c.orig    2019-11-26 10:37:08.753693088 +0100
> +++ nct7802.c    2019-11-26 17:27:56.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 };
> @@ -377,6 +377,32 @@
>       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;
> +    unsigned int val;
> +
> +    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;
> +
> +    /* also clear related status register to have functional interrupt */
> +    ret = regmap_read(data->regmap, sattr->index, &val);
> +    if (ret < 0)
> +        return ret;
> +

According to the datasheet, the status register bits should be set while
voltages are out of range. Are you sure that this is not the case ?

The next question is how the status registers behave. If the bits are set
whenever voltages cross a limit, we could use that knowledge and compare
voltages against limits only after a status register bit was set.
Something like
	if (status register bit is set) {
		alarm = (voltage is out of range);
		cache alarm;
	}
	print alarm;

Thanks,
Guenter

> +    return sprintf(buf, "%u\n", (volt < min) || (volt > max));
> +}
> +
>   static ssize_t show_temp(struct device *dev, struct device_attribute *attr,
>                char *buf)
>   {
> @@ -714,7 +740,7 @@
>                   0, 1);
>   static SENSOR_DEVICE_ATTR_2(in0_max, S_IRUGO | S_IWUSR, show_in, store_in,
>                   0, 2);
> -static SENSOR_DEVICE_ATTR_2(in0_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 3);
> +static SENSOR_DEVICE_ATTR_2(in0_alarm, S_IRUGO, show_in_alarm, NULL, 0, 0x1e);
>   static SENSOR_DEVICE_ATTR_2(in0_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
>                   0x5a, 3);
> 
> @@ -725,7 +751,7 @@
>                   2, 1);
>   static SENSOR_DEVICE_ATTR_2(in2_max, S_IRUGO | S_IWUSR, show_in, store_in,
>                   2, 2);
> -static SENSOR_DEVICE_ATTR_2(in2_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 0);
> +static SENSOR_DEVICE_ATTR_2(in2_alarm, S_IRUGO, show_in_alarm, NULL, 2, 0x1e);
>   static SENSOR_DEVICE_ATTR_2(in2_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
>                   0x5a, 0);
> 
> @@ -734,7 +760,7 @@
>                   3, 1);
>   static SENSOR_DEVICE_ATTR_2(in3_max, S_IRUGO | S_IWUSR, show_in, store_in,
>                   3, 2);
> -static SENSOR_DEVICE_ATTR_2(in3_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 1);
> +static SENSOR_DEVICE_ATTR_2(in3_alarm, S_IRUGO, show_in_alarm, NULL, 3, 0x1e);
>   static SENSOR_DEVICE_ATTR_2(in3_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
>                   0x5a, 1);
> 
> @@ -743,7 +769,7 @@
>                   4, 1);
>   static SENSOR_DEVICE_ATTR_2(in4_max, S_IRUGO | S_IWUSR, show_in, store_in,
>                   4, 2);
> -static SENSOR_DEVICE_ATTR_2(in4_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 2);
> +static SENSOR_DEVICE_ATTR_2(in4_alarm, S_IRUGO, show_in_alarm, NULL, 4, 0x1e);
>   static SENSOR_DEVICE_ATTR_2(in4_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
>                   0x5a, 2);
> 
> 
> Le 26/11/2019 13:22, Guenter Roeck a écrit :
> > On 11/26/19 2:03 AM, Gilles Buloz wrote:
> >> I have a functional patch (see below), but before going further (split and cleanup), I would like to have your opinion on how the
> >> NCT7802Y handles the thresholds status.
> >> Except for temperatures and in "comparator interrupt mode", the status bits are NOT set after each ADC conversion, but only once
> >> when crossing a threshold. So an alarm for a threshold is reported only to the first process reading the status and not to the 
> >> others.
> >> For instance if you run "sensors" you only get "ALARM" once the nothing until the threshold is crossed again in the other direction.
> >> Maybe the expected behaviour would be to display "ALARM" as long as we are outside the thresholds, not only once.
> >>
> >
> > Yes, that is the expected behavior.
> >
> > Guenter
> >
> >> --- nct7802.c.orig    2019-11-25 22:17:04.845718422 +0100
> >> +++ nct7802.c    2019-11-25 23:22:00.905387154 +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 };
> >> @@ -60,6 +60,9 @@
> >>    #define REG_CHIP_ID        0xfe
> >>    #define REG_VERSION_ID        0xff
> >>
> >> +#define REG_CACHE_START        0x17
> >> +#define REG_CACHE_END        0x20
> >> +
> >>    /*
> >>     * Data structures and manipulation thereof
> >>     */
> >> @@ -67,6 +70,7 @@
> >>    struct nct7802_data {
> >>        struct regmap *regmap;
> >>        struct mutex access_lock; /* for multi-byte read and write operations */
> >> +    u8 reg_cache[REG_CACHE_END - REG_CACHE_START + 1];
> >>    };
> >>
> >>    static ssize_t show_temp_type(struct device *dev, struct device_attribute *attr,
> >> @@ -467,6 +471,15 @@  static ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
> >>        if (ret < 0)
> >>            return ret;
> >>
> >> +    /*
> >> +     * For registers cleared on read, use a cache to keep all bits
> >> +     * that are set until they are returned to the caller
> >> +     */
> >> +    if ((sattr->nr >= REG_CACHE_START) && (sattr->nr <= REG_CACHE_END)) {
> >> +        val |= data->reg_cache[sattr->nr - REG_CACHE_START];
> >> +        data->reg_cache[sattr->nr - REG_CACHE_START] = val & ~(1 << bit);
> >> +    }
> >> +
> >>        return sprintf(buf, "%u\n", !!(val & (1 << bit)));
> >>    }
> >>
> >> Le 25/11/2019 19:06, Gilles BULOZ a écrit :
> >>> Le 25/11/2019 18:35, Guenter Roeck a écrit :
> >>>> On Mon, Nov 25, 2019 at 04:44:44PM +0000, Gilles Buloz wrote:
> >>>>> Le 25/11/2019 15:31, Guenter Roeck a écrit :
> >>>>>> On 11/25/19 5:13 AM, Gilles Buloz wrote:
> >>>>>>> Hi Guenter,
> >>>>>>>
> >>>>>>> According to the NCT7802Y datasheet, the REG_VOLTAGE_LIMIT_LSB definition is wrong and leads to wrong threshold registers
> >>>>>>> used. It
> >>>>>>> should be :
> >>>>>>> static const u8 REG_VOLTAGE_LIMIT_LSB[2][5] = {
> >>>>>>>             { 0x46, 0x00, 0x40, 0x42, 0x44 },
> >>>>>>>             { 0x45, 0x00, 0x3f, 0x41, 0x43 },
> >>>>>>> };
> >>>>>>> With this definition, the right bit is set in "Voltage SMI Status Register @0x1e" for each threshold reached (using i2cget to
> >>>>>>> check)
> >>>>>>>
> >>>>>> Good catch. Care to send a patch ?
> >>>>> As a fix for this is only useful with a fix for the problem below, maybe a single patch for both would be better.
> >>>> Not really. Those are two separate issues. The reported and selected
> >>>> limits are wrong, period. This will require two patches.
> >>> OK
> >>>>>>> But I'm unable to get any "ALARM" reported by the command "sensors" for VSEN1/2/3 = in2,in3,in4 because status for in0 is read
> >>>>>>> before (unless I set "ignore in0" in sensors file). The problem is that status bits in "Voltage SMI Status Register @0x1e" are
> >>>>>>> cleared when reading, and a read is done for each inX processed, so only the first inX has a chance to get its alarm bit set.
> >>>>>>> For this problem I don't see how to fix this easily; just to let you know ...
> >>>>>>>
> >>>>>> One possible fix would be to cache each alarm register and to clear the cache
> >>>>>> either after reading it (bitwise) or after a timeout. The latter is probably
> >>>>>> better to avoid stale information.
> >>>>> As we have status registers cleared at byte level and we want them to be cleared at bit level when each bit is read, I think a
> >>>>> cache
> >>>>> would be better. I suggest this :
> >>>>> - have a cached value for each status register, by default at 0x00
> >>>>> - when reading a register to get a bit, "OR" its byte value with its cached value, then use its cached value for processing.
> >>>>> - then clear the bit that has been processed from the cached value.
> >>>>>
> >>>> Both methods I suggested would have to involve a cache. The question is
> >>>> when to clear the cache - either clear a bit after reporting it, or
> >>>> clear it after a timeout.
> >>>>
> >>>>> I think a timeout would not be obvious to set : at least the time for sensors to read all info (including when terminal is a 
> >>>>> serial
> >>>>> line and output is slower) and to deal with possible latencies, but not too long...
> >>>> The timeout would be determined by the chip's conversion rate (register 0x26),
> >>> As I understand, the status must be kept in cache between the first read of status register @1E for in0 (clearing all status
> >>> bits), and the last read for in4. This duration depends on the "sensors" execution time and I'can see any link with the conversion
> >>> rate here.
> >>>> or, for simplicity, just be set to one second. I don't immediately see why
> >>>> that would be difficult to implement. Not that it matters much, really;
> >>>> I would accept patches with and without timeout.
> >>>>
> >>>> Guenter
> >>>> .
> >>>>
> >>>
> >>
> >
> > .
> >

Re: hwmon: (nct7802) buggy VSEN1/2/3 alarm

[Gilles Buloz]

Le 26/11/2019 19:20, Guenter Roeck a écrit :
> On Tue, Nov 26, 2019 at 04:47:47PM +0000, Gilles Buloz wrote:
>> OK, so to have "ALARM" reported as long as we are outside limits, I did not find another method than checking against limits by
>> software, but still clear the related status register to have a working interrupt.
>> The patch below is working for voltages.
>> If you're OK, I can extend it to the temperatures and fans
>>
>> --- nct7802.c.orig    2019-11-26 10:37:08.753693088 +0100
>> +++ nct7802.c    2019-11-26 17:27:56.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 };
>> @@ -377,6 +377,32 @@
>>        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;
>> +    unsigned int val;
>> +
>> +    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;
>> +
>> +    /* also clear related status register to have functional interrupt */
>> +    ret = regmap_read(data->regmap, sattr->index, &val);
>> +    if (ret < 0)
>> +        return ret;
>> +
> According to the datasheet, the status register bits should be set while
> voltages are out of range. Are you sure that this is not the case ?
Yes you are right, except for Voltages for which we only have the "SMI Voltage status" @1E to get a status, and as this is an SMI 
status register it does not work as simple non-SMI status registers.
So the good nexs is that there's nothing to fix for temperatures and fans because the driver is only using the non-SMI status 
registers for them, and they have their bits set as long as we are out of range (not cleared by read).
> The next question is how the status registers behave. If the bits are set
> whenever voltages cross a limit, we could use that knowledge and compare
> voltages against limits only after a status register bit was set.
> Something like
> 	if (status register bit is set) {
> 		alarm = (voltage is out of range);
> 		cache alarm;
> 	}
> 	print alarm;
>
> Thanks,
> Guenter
Good idea, I'm going to work on it and make sure there's no risk to miss a transition and then have a staled status.
>> +    return sprintf(buf, "%u\n", (volt < min) || (volt > max));
>> +}
>> +
>>    static ssize_t show_temp(struct device *dev, struct device_attribute *attr,
>>                 char *buf)
>>    {
>> @@ -714,7 +740,7 @@
>>                    0, 1);
>>    static SENSOR_DEVICE_ATTR_2(in0_max, S_IRUGO | S_IWUSR, show_in, store_in,
>>                    0, 2);
>> -static SENSOR_DEVICE_ATTR_2(in0_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 3);
>> +static SENSOR_DEVICE_ATTR_2(in0_alarm, S_IRUGO, show_in_alarm, NULL, 0, 0x1e);
>>    static SENSOR_DEVICE_ATTR_2(in0_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
>>                    0x5a, 3);
>>
>> @@ -725,7 +751,7 @@
>>                    2, 1);
>>    static SENSOR_DEVICE_ATTR_2(in2_max, S_IRUGO | S_IWUSR, show_in, store_in,
>>                    2, 2);
>> -static SENSOR_DEVICE_ATTR_2(in2_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 0);
>> +static SENSOR_DEVICE_ATTR_2(in2_alarm, S_IRUGO, show_in_alarm, NULL, 2, 0x1e);
>>    static SENSOR_DEVICE_ATTR_2(in2_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
>>                    0x5a, 0);
>>
>> @@ -734,7 +760,7 @@
>>                    3, 1);
>>    static SENSOR_DEVICE_ATTR_2(in3_max, S_IRUGO | S_IWUSR, show_in, store_in,
>>                    3, 2);
>> -static SENSOR_DEVICE_ATTR_2(in3_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 1);
>> +static SENSOR_DEVICE_ATTR_2(in3_alarm, S_IRUGO, show_in_alarm, NULL, 3, 0x1e);
>>    static SENSOR_DEVICE_ATTR_2(in3_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
>>                    0x5a, 1);
>>
>> @@ -743,7 +769,7 @@
>>                    4, 1);
>>    static SENSOR_DEVICE_ATTR_2(in4_max, S_IRUGO | S_IWUSR, show_in, store_in,
>>                    4, 2);
>> -static SENSOR_DEVICE_ATTR_2(in4_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 2);
>> +static SENSOR_DEVICE_ATTR_2(in4_alarm, S_IRUGO, show_in_alarm, NULL, 4, 0x1e);
>>    static SENSOR_DEVICE_ATTR_2(in4_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
>>                    0x5a, 2);
>>
>>
>> Le 26/11/2019 13:22, Guenter Roeck a écrit :
>>> On 11/26/19 2:03 AM, Gilles Buloz wrote:
>>>> I have a functional patch (see below), but before going further (split and cleanup), I would like to have your opinion on how the
>>>> NCT7802Y handles the thresholds status.
>>>> Except for temperatures and in "comparator interrupt mode", the status bits are NOT set after each ADC conversion, but only once
>>>> when crossing a threshold. So an alarm for a threshold is reported only to the first process reading the status and not to the
>>>> others.
>>>> For instance if you run "sensors" you only get "ALARM" once the nothing until the threshold is crossed again in the other direction.
>>>> Maybe the expected behaviour would be to display "ALARM" as long as we are outside the thresholds, not only once.
>>>>
>>> Yes, that is the expected behavior.
>>>
>>> Guenter
>>>
>>>> --- nct7802.c.orig    2019-11-25 22:17:04.845718422 +0100
>>>> +++ nct7802.c    2019-11-25 23:22:00.905387154 +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 };
>>>> @@ -60,6 +60,9 @@
>>>>     #define REG_CHIP_ID        0xfe
>>>>     #define REG_VERSION_ID        0xff
>>>>
>>>> +#define REG_CACHE_START        0x17
>>>> +#define REG_CACHE_END        0x20
>>>> +
>>>>     /*
>>>>      * Data structures and manipulation thereof
>>>>      */
>>>> @@ -67,6 +70,7 @@
>>>>     struct nct7802_data {
>>>>         struct regmap *regmap;
>>>>         struct mutex access_lock; /* for multi-byte read and write operations */
>>>> +    u8 reg_cache[REG_CACHE_END - REG_CACHE_START + 1];
>>>>     };
>>>>
>>>>     static ssize_t show_temp_type(struct device *dev, struct device_attribute *attr,
>>>> @@ -467,6 +471,15 @@  static ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
>>>>         if (ret < 0)
>>>>             return ret;
>>>>
>>>> +    /*
>>>> +     * For registers cleared on read, use a cache to keep all bits
>>>> +     * that are set until they are returned to the caller
>>>> +     */
>>>> +    if ((sattr->nr >= REG_CACHE_START) && (sattr->nr <= REG_CACHE_END)) {
>>>> +        val |= data->reg_cache[sattr->nr - REG_CACHE_START];
>>>> +        data->reg_cache[sattr->nr - REG_CACHE_START] = val & ~(1 << bit);
>>>> +    }
>>>> +
>>>>         return sprintf(buf, "%u\n", !!(val & (1 << bit)));
>>>>     }
>>>>
>>>> Le 25/11/2019 19:06, Gilles BULOZ a écrit :
>>>>> Le 25/11/2019 18:35, Guenter Roeck a écrit :
>>>>>> On Mon, Nov 25, 2019 at 04:44:44PM +0000, Gilles Buloz wrote:
>>>>>>> Le 25/11/2019 15:31, Guenter Roeck a écrit :
>>>>>>>> On 11/25/19 5:13 AM, Gilles Buloz wrote:
>>>>>>>>> Hi Guenter,
>>>>>>>>>
>>>>>>>>> According to the NCT7802Y datasheet, the REG_VOLTAGE_LIMIT_LSB definition is wrong and leads to wrong threshold registers
>>>>>>>>> used. It
>>>>>>>>> should be :
>>>>>>>>> static const u8 REG_VOLTAGE_LIMIT_LSB[2][5] = {
>>>>>>>>>              { 0x46, 0x00, 0x40, 0x42, 0x44 },
>>>>>>>>>              { 0x45, 0x00, 0x3f, 0x41, 0x43 },
>>>>>>>>> };
>>>>>>>>> With this definition, the right bit is set in "Voltage SMI Status Register @0x1e" for each threshold reached (using i2cget to
>>>>>>>>> check)
>>>>>>>>>
>>>>>>>> Good catch. Care to send a patch ?
>>>>>>> As a fix for this is only useful with a fix for the problem below, maybe a single patch for both would be better.
>>>>>> Not really. Those are two separate issues. The reported and selected
>>>>>> limits are wrong, period. This will require two patches.
>>>>> OK
>>>>>>>>> But I'm unable to get any "ALARM" reported by the command "sensors" for VSEN1/2/3 = in2,in3,in4 because status for in0 is read
>>>>>>>>> before (unless I set "ignore in0" in sensors file). The problem is that status bits in "Voltage SMI Status Register @0x1e" are
>>>>>>>>> cleared when reading, and a read is done for each inX processed, so only the first inX has a chance to get its alarm bit set.
>>>>>>>>> For this problem I don't see how to fix this easily; just to let you know ...
>>>>>>>>>
>>>>>>>> One possible fix would be to cache each alarm register and to clear the cache
>>>>>>>> either after reading it (bitwise) or after a timeout. The latter is probably
>>>>>>>> better to avoid stale information.
>>>>>>> As we have status registers cleared at byte level and we want them to be cleared at bit level when each bit is read, I think a
>>>>>>> cache
>>>>>>> would be better. I suggest this :
>>>>>>> - have a cached value for each status register, by default at 0x00
>>>>>>> - when reading a register to get a bit, "OR" its byte value with its cached value, then use its cached value for processing.
>>>>>>> - then clear the bit that has been processed from the cached value.
>>>>>>>
>>>>>> Both methods I suggested would have to involve a cache. The question is
>>>>>> when to clear the cache - either clear a bit after reporting it, or
>>>>>> clear it after a timeout.
>>>>>>
>>>>>>> I think a timeout would not be obvious to set : at least the time for sensors to read all info (including when terminal is a
>>>>>>> serial
>>>>>>> line and output is slower) and to deal with possible latencies, but not too long...
>>>>>> The timeout would be determined by the chip's conversion rate (register 0x26),
>>>>> As I understand, the status must be kept in cache between the first read of status register @1E for in0 (clearing all status
>>>>> bits), and the last read for in4. This duration depends on the "sensors" execution time and I'can see any link with the conversion
>>>>> rate here.
>>>>>> or, for simplicity, just be set to one second. I don't immediately see why
>>>>>> that would be difficult to implement. Not that it matters much, really;
>>>>>> I would accept patches with and without timeout.
>>>>>>
>>>>>> Guenter
>>>>>> .
>>>>>>
>>> .
>>>
> .
>

Re: hwmon: (nct7802) buggy VSEN1/2/3 alarm

[Gilles Buloz]

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.
+     * 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 */
+    if (ret < 0)
+        return ret;
+
+    /* if status of an input has changed, invalidate its cached status */
+    for (i=0; i<=3; i++)
+        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;
+        max = nct7802_read_voltage(data, sattr->nr, 2);
+        if (max < 0)
+            return max;
+
+        if ((volt < min) || (volt > max))
+            data->in_status_cache |= (1 << sattr->index);
+        else
+            data->in_status_cache &= ~(1 << sattr->index);
+
+        data->in_status_cache |= 0x10 << sattr->index;
+    }
+
+    return sprintf(buf, "%u\n", !!(data->in_status_cache & (1 << sattr->index)));
+}
+
  static ssize_t show_temp(struct device *dev, struct device_attribute *attr,
               char *buf)
  {
@@ -714,7 +765,7 @@
                  0, 1);
  static SENSOR_DEVICE_ATTR_2(in0_max, S_IRUGO | S_IWUSR, show_in, store_in,
                  0, 2);
-static SENSOR_DEVICE_ATTR_2(in0_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 3);
+static SENSOR_DEVICE_ATTR_2(in0_alarm, S_IRUGO, show_in_alarm, NULL, 0, 3);
  static SENSOR_DEVICE_ATTR_2(in0_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
                  0x5a, 3);

@@ -725,7 +776,7 @@
                  2, 1);
  static SENSOR_DEVICE_ATTR_2(in2_max, S_IRUGO | S_IWUSR, show_in, store_in,
                  2, 2);
-static SENSOR_DEVICE_ATTR_2(in2_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 0);
+static SENSOR_DEVICE_ATTR_2(in2_alarm, S_IRUGO, show_in_alarm, NULL, 2, 0);
  static SENSOR_DEVICE_ATTR_2(in2_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
                  0x5a, 0);

@@ -734,7 +785,7 @@
                  3, 1);
  static SENSOR_DEVICE_ATTR_2(in3_max, S_IRUGO | S_IWUSR, show_in, store_in,
                  3, 2);
-static SENSOR_DEVICE_ATTR_2(in3_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 1);
+static SENSOR_DEVICE_ATTR_2(in3_alarm, S_IRUGO, show_in_alarm, NULL, 3, 1);
  static SENSOR_DEVICE_ATTR_2(in3_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
                  0x5a, 1);

@@ -743,7 +794,7 @@
                  4, 1);
  static SENSOR_DEVICE_ATTR_2(in4_max, S_IRUGO | S_IWUSR, show_in, store_in,
                  4, 2);
-static SENSOR_DEVICE_ATTR_2(in4_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 2);
+static SENSOR_DEVICE_ATTR_2(in4_alarm, S_IRUGO, show_in_alarm, NULL, 4, 2);
  static SENSOR_DEVICE_ATTR_2(in4_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
                  0x5a, 2);


Le 26/11/2019 19:20, Guenter Roeck a écrit :
> On Tue, Nov 26, 2019 at 04:47:47PM +0000, Gilles Buloz wrote:
>> OK, so to have "ALARM" reported as long as we are outside limits, I did not find another method than checking against limits by
>> software, but still clear the related status register to have a working interrupt.
>> The patch below is working for voltages.
>> If you're OK, I can extend it to the temperatures and fans
>>
>> --- nct7802.c.orig    2019-11-26 10:37:08.753693088 +0100
>> +++ nct7802.c    2019-11-26 17:27:56.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 };
>> @@ -377,6 +377,32 @@
>>        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;
>> +    unsigned int val;
>> +
>> +    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;
>> +
>> +    /* also clear related status register to have functional interrupt */
>> +    ret = regmap_read(data->regmap, sattr->index, &val);
>> +    if (ret < 0)
>> +        return ret;
>> +
> According to the datasheet, the status register bits should be set while
> voltages are out of range. Are you sure that this is not the case ?
>
> The next question is how the status registers behave. If the bits are set
> whenever voltages cross a limit, we could use that knowledge and compare
> voltages against limits only after a status register bit was set.
> Something like
> 	if (status register bit is set) {
> 		alarm = (voltage is out of range);
> 		cache alarm;
> 	}
> 	print alarm;
>
> Thanks,
> Guenter
>
>> +    return sprintf(buf, "%u\n", (volt < min) || (volt > max));
>> +}
>> +
>>    static ssize_t show_temp(struct device *dev, struct device_attribute *attr,
>>                 char *buf)
>>    {
>> @@ -714,7 +740,7 @@
>>                    0, 1);
>>    static SENSOR_DEVICE_ATTR_2(in0_max, S_IRUGO | S_IWUSR, show_in, store_in,
>>                    0, 2);
>> -static SENSOR_DEVICE_ATTR_2(in0_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 3);
>> +static SENSOR_DEVICE_ATTR_2(in0_alarm, S_IRUGO, show_in_alarm, NULL, 0, 0x1e);
>>    static SENSOR_DEVICE_ATTR_2(in0_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
>>                    0x5a, 3);
>>
>> @@ -725,7 +751,7 @@
>>                    2, 1);
>>    static SENSOR_DEVICE_ATTR_2(in2_max, S_IRUGO | S_IWUSR, show_in, store_in,
>>                    2, 2);
>> -static SENSOR_DEVICE_ATTR_2(in2_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 0);
>> +static SENSOR_DEVICE_ATTR_2(in2_alarm, S_IRUGO, show_in_alarm, NULL, 2, 0x1e);
>>    static SENSOR_DEVICE_ATTR_2(in2_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
>>                    0x5a, 0);
>>
>> @@ -734,7 +760,7 @@
>>                    3, 1);
>>    static SENSOR_DEVICE_ATTR_2(in3_max, S_IRUGO | S_IWUSR, show_in, store_in,
>>                    3, 2);
>> -static SENSOR_DEVICE_ATTR_2(in3_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 1);
>> +static SENSOR_DEVICE_ATTR_2(in3_alarm, S_IRUGO, show_in_alarm, NULL, 3, 0x1e);
>>    static SENSOR_DEVICE_ATTR_2(in3_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
>>                    0x5a, 1);
>>
>> @@ -743,7 +769,7 @@
>>                    4, 1);
>>    static SENSOR_DEVICE_ATTR_2(in4_max, S_IRUGO | S_IWUSR, show_in, store_in,
>>                    4, 2);
>> -static SENSOR_DEVICE_ATTR_2(in4_alarm, S_IRUGO, show_alarm, NULL, 0x1e, 2);
>> +static SENSOR_DEVICE_ATTR_2(in4_alarm, S_IRUGO, show_in_alarm, NULL, 4, 0x1e);
>>    static SENSOR_DEVICE_ATTR_2(in4_beep, S_IRUGO | S_IWUSR, show_beep, store_beep,
>>                    0x5a, 2);
>>
>>
>> Le 26/11/2019 13:22, Guenter Roeck a écrit :
>>> On 11/26/19 2:03 AM, Gilles Buloz wrote:
>>>> I have a functional patch (see below), but before going further (split and cleanup), I would like to have your opinion on how the
>>>> NCT7802Y handles the thresholds status.
>>>> Except for temperatures and in "comparator interrupt mode", the status bits are NOT set after each ADC conversion, but only once
>>>> when crossing a threshold. So an alarm for a threshold is reported only to the first process reading the status and not to the
>>>> others.
>>>> For instance if you run "sensors" you only get "ALARM" once the nothing until the threshold is crossed again in the other direction.
>>>> Maybe the expected behaviour would be to display "ALARM" as long as we are outside the thresholds, not only once.
>>>>
>>> Yes, that is the expected behavior.
>>>
>>> Guenter
>>>
>>>> --- nct7802.c.orig    2019-11-25 22:17:04.845718422 +0100
>>>> +++ nct7802.c    2019-11-25 23:22:00.905387154 +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 };
>>>> @@ -60,6 +60,9 @@
>>>>     #define REG_CHIP_ID        0xfe
>>>>     #define REG_VERSION_ID        0xff
>>>>
>>>> +#define REG_CACHE_START        0x17
>>>> +#define REG_CACHE_END        0x20
>>>> +
>>>>     /*
>>>>      * Data structures and manipulation thereof
>>>>      */
>>>> @@ -67,6 +70,7 @@
>>>>     struct nct7802_data {
>>>>         struct regmap *regmap;
>>>>         struct mutex access_lock; /* for multi-byte read and write operations */
>>>> +    u8 reg_cache[REG_CACHE_END - REG_CACHE_START + 1];
>>>>     };
>>>>
>>>>     static ssize_t show_temp_type(struct device *dev, struct device_attribute *attr,
>>>> @@ -467,6 +471,15 @@  static ssize_t show_alarm(struct device *dev, struct device_attribute *attr,
>>>>         if (ret < 0)
>>>>             return ret;
>>>>
>>>> +    /*
>>>> +     * For registers cleared on read, use a cache to keep all bits
>>>> +     * that are set until they are returned to the caller
>>>> +     */
>>>> +    if ((sattr->nr >= REG_CACHE_START) && (sattr->nr <= REG_CACHE_END)) {
>>>> +        val |= data->reg_cache[sattr->nr - REG_CACHE_START];
>>>> +        data->reg_cache[sattr->nr - REG_CACHE_START] = val & ~(1 << bit);
>>>> +    }
>>>> +
>>>>         return sprintf(buf, "%u\n", !!(val & (1 << bit)));
>>>>     }
>>>>
>>>> Le 25/11/2019 19:06, Gilles BULOZ a écrit :
>>>>> Le 25/11/2019 18:35, Guenter Roeck a écrit :
>>>>>> On Mon, Nov 25, 2019 at 04:44:44PM +0000, Gilles Buloz wrote:
>>>>>>> Le 25/11/2019 15:31, Guenter Roeck a écrit :
>>>>>>>> On 11/25/19 5:13 AM, Gilles Buloz wrote:
>>>>>>>>> Hi Guenter,
>>>>>>>>>
>>>>>>>>> According to the NCT7802Y datasheet, the REG_VOLTAGE_LIMIT_LSB definition is wrong and leads to wrong threshold registers
>>>>>>>>> used. It
>>>>>>>>> should be :
>>>>>>>>> static const u8 REG_VOLTAGE_LIMIT_LSB[2][5] = {
>>>>>>>>>              { 0x46, 0x00, 0x40, 0x42, 0x44 },
>>>>>>>>>              { 0x45, 0x00, 0x3f, 0x41, 0x43 },
>>>>>>>>> };
>>>>>>>>> With this definition, the right bit is set in "Voltage SMI Status Register @0x1e" for each threshold reached (using i2cget to
>>>>>>>>> check)
>>>>>>>>>
>>>>>>>> Good catch. Care to send a patch ?
>>>>>>> As a fix for this is only useful with a fix for the problem below, maybe a single patch for both would be better.
>>>>>> Not really. Those are two separate issues. The reported and selected
>>>>>> limits are wrong, period. This will require two patches.
>>>>> OK
>>>>>>>>> But I'm unable to get any "ALARM" reported by the command "sensors" for VSEN1/2/3 = in2,in3,in4 because status for in0 is read
>>>>>>>>> before (unless I set "ignore in0" in sensors file). The problem is that status bits in "Voltage SMI Status Register @0x1e" are
>>>>>>>>> cleared when reading, and a read is done for each inX processed, so only the first inX has a chance to get its alarm bit set.
>>>>>>>>> For this problem I don't see how to fix this easily; just to let you know ...
>>>>>>>>>
>>>>>>>> One possible fix would be to cache each alarm register and to clear the cache
>>>>>>>> either after reading it (bitwise) or after a timeout. The latter is probably
>>>>>>>> better to avoid stale information.
>>>>>>> As we have status registers cleared at byte level and we want them to be cleared at bit level when each bit is read, I think a
>>>>>>> cache
>>>>>>> would be better. I suggest this :
>>>>>>> - have a cached value for each status register, by default at 0x00
>>>>>>> - when reading a register to get a bit, "OR" its byte value with its cached value, then use its cached value for processing.
>>>>>>> - then clear the bit that has been processed from the cached value.
>>>>>>>
>>>>>> Both methods I suggested would have to involve a cache. The question is
>>>>>> when to clear the cache - either clear a bit after reporting it, or
>>>>>> clear it after a timeout.
>>>>>>
>>>>>>> I think a timeout would not be obvious to set : at least the time for sensors to read all info (including when terminal is a
>>>>>>> serial
>>>>>>> line and output is slower) and to deal with possible latencies, but not too long...
>>>>>> The timeout would be determined by the chip's conversion rate (register 0x26),
>>>>> As I understand, the status must be kept in cache between the first read of status register @1E for in0 (clearing all status
>>>>> bits), and the last read for in4. This duration depends on the "sensors" execution time and I'can see any link with the conversion
>>>>> rate here.
>>>>>> or, for simplicity, just be set to one second. I don't immediately see why
>>>>>> that would be difficult to implement. Not that it matters much, really;
>>>>>> I would accept patches with and without timeout.
>>>>>>
>>>>>> Guenter
>>>>>> .
>>>>>>
>>> .
>>>
> .
>

Re: hwmon: (nct7802) buggy VSEN1/2/3 alarm

[Guenter Roeck]

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.

> +     * 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.

> +    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.

> +        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".
No idea though what the min limits are for in that case. Might be worth
running some checks to understand this better (if you did not do that
already).

> +        max = nct7802_read_voltage(data, sattr->nr, 2);
> +        if (max < 0)
> +            return max;
> +
> +        if ((volt < min) || (volt > max))

Unnecessary inner ( )

Guenter

Re: hwmon: (nct7802) buggy VSEN1/2/3 alarm

[Gilles Buloz]

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@kontron.com>
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@kontron.com>
---
  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@kontron.com>
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@kontron.com>
---
  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
> .
>

Re: hwmon: (nct7802) buggy VSEN1/2/3 alarm

[Guenter Roeck]

On 11/28/19 2:47 AM, Gilles Buloz wrote:
> 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
> 

Please don't send final patches inline, nor as reply to previous e-mails.
Doing this violates Documentation/process/submitting-patches.rst.
Besides, I use patchwork for my workflow, patchwork gets confused by it,
and the patches don't show up there.

On top of that, the patches are whitespace damaged (tabs replaced with spaces)
and would not apply, even if I tried.

Guenter