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

[Guenter Roeck <linux@roeck-us.net>]

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