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

[Gilles Buloz <Gilles.Buloz@kontron.com>]

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