Re: [PATCH v10 2/2] hwmon: add support for MCP998X

[<Victor.Duicu@microchip.com>]

Hi Guenter,

...

> > +     }
> > +
> > +     switch (type) {
> > +     case hwmon_temp:
> > +             switch (attr) {
> > +             case hwmon_temp_input:
> > +                     /* Block reading from addresses 0x00->0x09 is
> > not allowed. */
> > +                     ret = regmap_read(priv->regmap,
> > MCP9982_HIGH_BYTE_ADDR(channel), &reg_high);
> > +                     if (ret)
> > +                             return ret;
> > +
> > +                     ret = regmap_read(priv->regmap,
> > MCP9982_HIGH_BYTE_ADDR(channel) + 1,
> > +                                       &reg_low);
> > +                     if (ret)
> > +                             return ret;
> 
> Reading the 11-bit temperature value involves two separate 8-bit
> register reads.
> If the chip updates the temperature between these two reads, the
> resulting value
> may be torn. While some chips latch the low byte upon reading the
> high byte,
> the driver does not explicitly rely on or document this behavior, and
> it's safer
> to use regmap_bulk_read if supported, or at least ensure the correct
> order and
> atomicity if possible.
> 
> Note: Maybe the low temperature is latched, but there is no
> indication in the
> datasheet that this would be the case. Even if it is, the code above
> is
> inefficient.

The low temperature register is latched. In the documentation at
page 32 it is described that when reading the high byte register,
the value from the low byte register is copied into a 'shadow'
register. In this way it is guaranteed that when we read the low byte,
it will correspond to the high byte.

Regarding the bulk read, the chip has a number of design quirks and
because of that different commands are supported only on some
particular memory regions.

According to the documentation page 26, the only areas of memory that
support SMBus block read are 80h->89h(temperature memory block) and
90h->97h(status memory block). In order to block read the temperatures,
the area of memory targeted has to be the temperature memory block. In
this context the read operation uses SMBus protocol and the first value
returned will be the number of addresses that can be read (in our
particular case a max value of 10 bytes).

In v8 of the driver
https://lore.kernel.org/all/20251120071248.3767-1-victor.duicu@microchip.com/
,
the temperature values were read with regmap_bulk_read(). In that
version, regmap_bulk_read() was also used to read the temperature
limits, without returning count (this is an undocumented feature of the
chip and because of that we could assume is not supported).
In order to avoid this behaviour and avoid mixing the SMBus and I2C
protocols all block readings were removed.

In the hopes of bypassing a long chain of replies, I tested the
behaviour of the chip with different read instructions.
Regmap_bulk_read() when applied to the temperature memory block
(80h->89h) returns count and the high and low bytes. When it is applied
to the 00h->09h memory, it uses I2C. It returns one temperature byte,
but all other bytes are returned as 0xFF. The chip behaves as if
it is at the last register location in the temperature block while the
host continues to ACK.(behaviour described at page 26).
If we set use_single_read in regmap_config and apply regmap_bulk_read()
to the 00h->09h register area the high and low temperature bytes are
read successfully without count.

Regmap_multi_reg_read() reads a number of registers one by one. When
applied to the 00h->09h area, I2C is used and it returns only the high
and low temperature bytes. When applied to the temperature memory block
(80h->89h), because it is not a bulk function, returns the count till
the end of the temperature memory block (aka SMBus count).

I2c_smbus_read_block_data() when applied to the temperature block (80h-
89h) returns the count, the driver replies with an NACK and the
communication is stopped. In our case, the board we are using to test
the driver has an AT91 adapter and supports
I2C_FUNC_SMBUS_READ_BLOCK_DATA. It seems that the I2C driver for AT91
does not modify the buff length of the message, leaving it 1.

I2c_smbus_read_i2c_block_data() when applied to the temperature block
(80h-89h) returns count and the temperature values.

If you are of the opinion that block reading the temperatures is worth
introducing (even in case we need to skip count) then I can add it, but
we should come to an agreement on which function to use.
Please let me know your thoughts.

Kind regards,
Victor