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

[<Victor.Duicu@microchip.com>]

Hi Guenter,


> > +static int mcp9982_read_limit(struct mcp9982_priv *priv, u8
> > address, long *val)
> > +{
> > +     unsigned int limit, reg_high, reg_low;
> > +     int ret;
> > +
> > +     switch (address) {
> > +     case MCP9982_INTERNAL_HIGH_LIMIT_ADDR:
> > +     case MCP9982_INTERNAL_LOW_LIMIT_ADDR:
> > +     case MCP9982_THERM_LIMIT_ADDR(0):
> > +     case MCP9982_THERM_LIMIT_ADDR(1):
> > +     case MCP9982_THERM_LIMIT_ADDR(2):
> > +     case MCP9982_THERM_LIMIT_ADDR(3):
> > +     case MCP9982_THERM_LIMIT_ADDR(4):
> > +             ret = regmap_read(priv->regmap, address, &limit);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             *val = limit & 0xFF;
> > +             *val = (*val - MCP9982_OFFSET) * 1000;
> > +
> > +             return 0;
> > +     case MCP9982_EXT_HIGH_LIMIT_ADDR(1):
> > +     case MCP9982_EXT_HIGH_LIMIT_ADDR(2):
> > +     case MCP9982_EXT_HIGH_LIMIT_ADDR(3):
> > +     case MCP9982_EXT_HIGH_LIMIT_ADDR(4):
> > +     case MCP9982_EXT_LOW_LIMIT_ADDR(1):
> > +     case MCP9982_EXT_LOW_LIMIT_ADDR(2):
> > +     case MCP9982_EXT_LOW_LIMIT_ADDR(3):
> > +     case MCP9982_EXT_LOW_LIMIT_ADDR(4):
> > +             /*
> > +              * The register address determines whether a single
> > byte or
> > +              * multiple byte (block) operation is run. For a
> > single byte
> > +              * operation, the MSB of the register address is set
> > to "0".
> > +              * For a multiple byte operation, it is set to "1".
> > The addresses
> > +              * quoted in the register map and throughout the data
> > sheet assume
> > +              * single byte operation. For multiple byte
> > operations, the user
> > +              * must set the MSB of each register address to "1".
> > +              */
> > +             ret = regmap_read(priv->regmap, address, &reg_high);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = regmap_read(priv->regmap, address + 1,
> > &reg_low);
> > +             if (ret)
> > +                     return ret;
> > +
> Consider using regmap_bulk_read().

  The MCP998X family is designed so that block reading is allowed only
on the dedicated temperature and memory blocks. Reading from
those memory areas uses the SMBus protocol, which returns count
and the data. From any other memory region, reading only one byte
is allowed. This behavior is described in the documentation at page 26.

  In V2 patch, block reading was used in this function, however this
was an exploit. After reading one byte the chip returns NACK to finish
the read, that will force the Linux driver to issue another 1 byte read
for the second byte, which will return the value and stop.
  For the addresses 0x00 to 0x09 (temperature registers) the chip will
not return NACK after the first byte, it will just go to sleep and
return invalid data 0xff. That was a design choice to be backwards
compatible with older parts.

...

> > 
> > +             switch (attr) {
> > +             case hwmon_temp_input:
> > +                     /*
> > +                      * The chips support block reading only on
> > the temperature and
> > +                      * status memory blocks. The driver uses only
> > individual read commands.
> > +                      */
> > +                     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;
> > +
> 
> Consider using regmap_bulk_read().

  In V2 patch, block reading was used to read the temperatures from the
dedicated memory. However, the operation would use SMBus protocol
and return count alongside the data.

  Regmap_bulk_read() in this context uses SMBus protocol, while in the
context of reading the temperature limits uses I2C protocol(and is an
invalid request).

  In order to avoid having one function with multiple behaviors and
to keep the driver more generic all block reads were removed.


> > +                     *val = ((reg_high << 8) + reg_low) >> 5;
> > +                     *val = (*val - (MCP9982_OFFSET << 3)) * 125;
> > +
> > +                     return 0;
> > +             case hwmon_temp_max:
> > +                     if (channel)
> > +                             addr =
> > MCP9982_EXT_HIGH_LIMIT_ADDR(channel);
> > +                     else
> > +                             addr =
> > MCP9982_INTERNAL_HIGH_LIMIT_ADDR;
> > +
> > +                     return mcp9982_read_limit(priv, addr, val);
> > +             case hwmon_temp_max_alarm:
> > +                     *val = regmap_test_bits(priv->regmap,
> > MCP9982_HIGH_LIMIT_STATUS_ADDR,
> > +                                             BIT(channel));
> > +                     if (*val < 0)
> > +                             return *val;
> > +
> > +                     return 0;
> > +             case hwmon_temp_max_hyst:
> > +                     if (channel)
> > +                             addr =
> > MCP9982_EXT_HIGH_LIMIT_ADDR(channel);
> > +                     else
> > +                             addr =
> > MCP9982_INTERNAL_HIGH_LIMIT_ADDR;
> > +                     ret = mcp9982_read_limit(priv, addr, val);
> > +                     if (ret)
> > +                             return ret;
> > +
> > +                     ret = regmap_read(priv->regmap,
> > MCP9982_HYS_ADDR, &hyst);
> > +                     if (ret)
> > +                             return ret;
> > +
> > +                     *val -= (hyst & 0xFF) * 1000;
> 
> What is the mask for ? The chip registers are 8 bit wide.
> 
> > +                     *val = clamp_val(*val, -64000, 191875);
> 
> Clamping on reads is highly unusual. Why is this needed ?

  There are instances when the hysteresis limit could be outside
the range of temperatures.

  For example, if the high limit is set to -45000 and the hysteresis
is set to 20000, the high limit hysteresis is -65000 which is outside
the range of supported temperatures.

  The hysteresis is set related to the critical temperature (that is
higher then the "high limit") but it will be applied also to the "high
temperature". In this case the hysteresis is valid for critical but it
will be out of range for the "high temp".

>