Re: [PATCH v3 2/2] hwmon: add support for MCP998X
From: Guenter Roeck
Date: Mon Feb 02 2026 - 10:25:09 EST
On 2/2/26 00:15, Victor.Duicu@xxxxxxxxxxxxx wrote:
Hi Guenter,This warrants a comment in the code. Others won't know and might send
+static int mcp9982_read_limit(struct mcp9982_priv *priv, u8Consider using regmap_bulk_read().
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, ®_high);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(priv->regmap, address + 1,
®_low);
+ if (ret)
+ return ret;
+
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.
a patch to "fix" the code.
...As above.
+ 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), ®_high);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(priv->regmap,
MCP9982_HIGH_BYTE_ADDR(channel) + 1,
+ ®_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.
"Supported" is irrelevant. Question is what is written into and reported by
+ *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".
the chip. It may be "out of range", but the value is still written into
the chip. So the question is: How does the chip react to the "out of range"
values ? I suspect that it technically still works, even if the value is not
officially supported. That should be reflected in the reported values.
More specifically, if setting the hysteresis in your example to 19000
instead of 20000 triggers a different response from the chip, that needs
to be reflected in the reported values.
Guenter