linux-hwmon.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: Gilles Buloz <Gilles.Buloz@kontron.com>
Cc: "linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>
Subject: Re: hwmon: (nct7802) buggy VSEN1/2/3 alarm
Date: Tue, 26 Nov 2019 04:22:34 -0800	[thread overview]
Message-ID: <fc7c1f0b-abaf-0281-7b1b-726f08abcb16@roeck-us.net> (raw)
In-Reply-To: <5DDCF86F.7010703@kontron.com>

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


  reply	other threads:[~2019-11-26 12:22 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-25 13:13 hwmon: (nct7802) buggy VSEN1/2/3 alarm Gilles Buloz
2019-11-25 14:31 ` Guenter Roeck
2019-11-25 16:44   ` Gilles Buloz
2019-11-25 17:35     ` Guenter Roeck
2019-11-25 18:06       ` Gilles Buloz
2019-11-26 10:03         ` Gilles Buloz
2019-11-26 12:22           ` Guenter Roeck [this message]
2019-11-26 16:47             ` Gilles Buloz
2019-11-26 18:20               ` Guenter Roeck
2019-11-27 10:42                 ` Gilles Buloz
2019-11-27 14:41                 ` Gilles Buloz
2019-11-27 19:32                   ` Guenter Roeck
2019-11-28 10:47                     ` Gilles Buloz
2019-11-28 15:22                       ` Guenter Roeck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fc7c1f0b-abaf-0281-7b1b-726f08abcb16@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=Gilles.Buloz@kontron.com \
    --cc=linux-hwmon@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).