* [PATCH v3 0/2] add support in hwmon for MCP998X
@ 2026-01-27 15:18 victor.duicu
2026-01-27 15:18 ` [PATCH v3 1/2] dt-bindings: hwmon: add support " victor.duicu
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: victor.duicu @ 2026-01-27 15:18 UTC (permalink / raw)
To: linux, robh, krzk+dt, conor+dt, corbet
Cc: marius.cristea, victor.duicu, linux-hwmon, linux-kernel,
devicetree, linux-doc
From: Victor Duicu <victor.duicu@microchip.com>
Add support in hwmon for Microchip MCP998X/33 and MCP998XD/33D Multichannel
Automotive Temperature Monitor Family.
The chips in the family have different numbers of external channels,
ranging from 1 (MCP9982) to 4 channels (MCP9985).
Reading diodes in anti-parallel connection is supported by MCP9984/85/33
and MCP9984D/85D/33D. Dedicated hardware shutdown circuitry is present
only in MCP998XD and MCP9933D.
The driver supports reading the temperature channels, the temperature
limits and their corresponding alarms. The user can set the limits,
the update interval and the hysteresis.
This driver is based on the IIO driver for MCP998X:
https://lore.kernel.org/all/20250930133131.13797-1-victor.duicu@microchip.com/
Differences related to previous patch:
v3:
- update copyright year.
- add tempX_max_hyst and tempX_crit_hyst attributes and document
them in mcp9982.rst.
- in include list add byteorder/generic.h and remove unaligned.h.
- remove definitions for temperature memory block
and status memory block.
- remove individual definitions for register addresses 1Dh->21h.
- add constants MCP9982_WAKE_UP_TIME_MAX_US and
MCP9982_TIMER_BUFFER_US.
- add checks to ensure that values read from registers are on 8 bits.
- in mcp9982_read_limit() simplify calculation, replace bulk read
with individual operations and add comment.
- in mcp9982_read_limit() add explicit case branches for limits
that are on 16 bits.
- in mcp9982_read() replace mdelay() with usleep_range().
- in mcp9982_read() replace block reading for temperature values with
individual operations, add comment and remove unnecessary
mask variable.
- in regmap_read_poll_timeout() add final timeout.
- in mcp9982_read_label() remove label check.
- in mcp9982_write_limit() replace put_unaligned_be16() with cpu_to_be16().
- in mcp9982_write_limit() add explicit case branches for limits
that are on 16 bits.
- in mcp9982_init() write default value for diode alert mask register.
- in mcp9982_parse_fw_config() replace E2BIG with EINVAL.
v2:
- in Kconfig add select REGMAP_I2C.
- in yaml add power state attribute. For chips with "D" in the name
check that Run mode is set in yaml and driver.
- in the include list: remove cleanup.h, add math.h, minmax.h and
util_macros.h.
- add min, max and crit limits for all channels. These attributes can
be read and written. In mcp9982_init() set default values for limits.
- add alarms for limits.
- edit regmap ranges to add the limit registers.
- when writing update interval, don't force the user to set exact value.
Search for closest valid value.
- in mcp9982_parse_fw_config() check value from fwnode_property_read_u32().
- edit coding style and comments.
- remove constant MCP9982_SCALE.
- rename variable sampl_idx from mcp9982_priv to interval_idx.
- in mcp9982_write() rename variable use_previous_freq
to use_previous_interval.
v1:
- initial version for review.
Victor Duicu (2):
dt-bindings: hwmon: add support for MCP998X
hwmon: add support for MCP998X
.../bindings/hwmon/microchip,mcp9982.yaml | 205 ++++
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/mcp9982.rst | 105 ++
MAINTAINERS | 8 +
drivers/hwmon/Kconfig | 11 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/mcp9982.c | 1022 +++++++++++++++++
7 files changed, 1353 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
create mode 100644 Documentation/hwmon/mcp9982.rst
create mode 100644 drivers/hwmon/mcp9982.c
base-commit: 0f61b1860cc3f52aef9036d7235ed1f017632193
--
2.51.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 1/2] dt-bindings: hwmon: add support for MCP998X
2026-01-27 15:18 [PATCH v3 0/2] add support in hwmon for MCP998X victor.duicu
@ 2026-01-27 15:18 ` victor.duicu
2026-02-03 19:15 ` Guenter Roeck
2026-02-06 16:47 ` Krzysztof Kozlowski
2026-01-27 15:18 ` [PATCH v3 2/2] " victor.duicu
2026-02-06 16:55 ` [PATCH v3 0/2] add support in hwmon " Krzysztof Kozlowski
2 siblings, 2 replies; 18+ messages in thread
From: victor.duicu @ 2026-01-27 15:18 UTC (permalink / raw)
To: linux, robh, krzk+dt, conor+dt, corbet
Cc: marius.cristea, victor.duicu, linux-hwmon, linux-kernel,
devicetree, linux-doc
From: Victor Duicu <victor.duicu@microchip.com>
This is the devicetree schema for Microchip MCP998X/33 and
MCP998XD/33D Multichannel Automotive Temperature Monitor Family.
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
---
.../bindings/hwmon/microchip,mcp9982.yaml | 205 ++++++++++++++++++
MAINTAINERS | 6 +
2 files changed, 211 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
diff --git a/Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml b/Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
new file mode 100644
index 000000000000..05ea3c6a5618
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
@@ -0,0 +1,205 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/microchip,mcp9982.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip MCP998X/33 and MCP998XD/33D Temperature Monitor
+
+maintainers:
+ - Victor Duicu <victor.duicu@microchip.com>
+
+description: |
+ The MCP998X/33 and MCP998XD/33D family is a high-accuracy 2-wire
+ multichannel automotive temperature monitor.
+ The datasheet can be found here:
+ https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
+
+properties:
+ compatible:
+ enum:
+ - microchip,mcp9933
+ - microchip,mcp9933d
+ - microchip,mcp9982
+ - microchip,mcp9982d
+ - microchip,mcp9983
+ - microchip,mcp9983d
+ - microchip,mcp9984
+ - microchip,mcp9984d
+ - microchip,mcp9985
+ - microchip,mcp9985d
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ items:
+ - description: Signal coming from ALERT/THERM pin.
+ - description: Signal coming from THERM/ADDR pin.
+ - description: Signal coming from SYS_SHDN pin.
+
+ interrupt-names:
+ items:
+ - const: alert-therm
+ - const: therm-addr
+ - const: sys-shutdown
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+ microchip,enable-anti-parallel:
+ description:
+ Enable anti-parallel diode mode operation.
+ MCP9984/84D/85/85D and MCP9933/33D support reading two external diodes
+ in anti-parallel connection on the same set of pins.
+ type: boolean
+
+ microchip,parasitic-res-on-channel1-2:
+ description:
+ Indicates that the chip and the diodes/transistors are sufficiently far
+ apart that a parasitic resistance is added to the wires, which can affect
+ the measurements. Due to the anti-parallel diode connections, channels
+ 1 and 2 are affected together.
+ type: boolean
+
+ microchip,parasitic-res-on-channel3-4:
+ description:
+ Indicates that the chip and the diodes/transistors are sufficiently far
+ apart that a parasitic resistance is added to the wires, which can affect
+ the measurements. Due to the anti-parallel diode connections, channels
+ 3 and 4 are affected together.
+ type: boolean
+
+ microchip,power-state:
+ description:
+ The chip can be set in Run state or Standby state. In Run state the ADC
+ is converting on all channels at the programmed conversion rate.
+ In Standby state the host must initiate a conversion cycle by writing
+ to the One-Shot register.
+ True value sets Run state.
+ Chips with "D" in the name can only be set in Run mode.
+ type: boolean
+
+ vdd-supply: true
+
+patternProperties:
+ "^channel@[1-4]$":
+ description:
+ Represents the external temperature channels to which
+ a remote diode is connected.
+ type: object
+
+ properties:
+ reg:
+ items:
+ minItems: 1
+ maxItems: 4
+
+ label:
+ description: Unique name to identify which channel this is.
+
+ required:
+ - reg
+
+ additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - vdd-supply
+
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - microchip,mcp9982d
+ - microchip,mcp9983d
+ - microchip,mcp9984d
+ - microchip,mcp9985d
+ - microchip,mcp9933d
+ then:
+ properties:
+ interrupts-names:
+ items:
+ - const: alert-therm
+ - const: sys-shutdown
+ required:
+ - microchip,power-state
+ else:
+ properties:
+ interrupts-names:
+ items:
+ - const: alert-therm
+ - const: therm-addr
+ microchip,power-state: true
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ pattern: '^microchip,mcp998(2|3)$'
+ then:
+ properties:
+ microchip,enable-anti-parallel: false
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ pattern: '^microchip,mcp998(2|3)d$'
+ then:
+ properties:
+ microchip,enable-anti-parallel: false
+ required:
+ - microchip,parasitic-res-on-channel1-2
+ - microchip,parasitic-res-on-channel3-4
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ pattern: '^microchip,mcp99(33|8[4-5])d$'
+ then:
+ required:
+ - microchip,parasitic-res-on-channel1-2
+ - microchip,parasitic-res-on-channel3-4
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ temperature-sensor@4c {
+ compatible = "microchip,mcp9985";
+ reg = <0x4c>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ microchip,enable-anti-parallel;
+ microchip,parasitic-res-on-channel1-2;
+ microchip,parasitic-res-on-channel3-4;
+
+ vdd-supply = <&vdd>;
+
+ channel@1 {
+ reg = <1>;
+ label = "Room Temperature";
+ };
+
+ channel@2 {
+ reg = <2>;
+ label = "GPU Temperature";
+ };
+ };
+ };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 0d044a58cbfe..f88d837cb586 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17149,6 +17149,12 @@ S: Maintained
F: Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
F: drivers/iio/adc/mcp3911.c
+MICROCHIP MCP9982 TEMPERATURE DRIVER
+M: Victor Duicu <victor.duicu@microchip.com>
+L: linux-hwmon@vger.kernel.org
+S: Supported
+F: Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
+
MICROCHIP MMC/SD/SDIO MCI DRIVER
M: Aubin Constans <aubin.constans@microchip.com>
S: Maintained
--
2.51.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v3 2/2] hwmon: add support for MCP998X
2026-01-27 15:18 [PATCH v3 0/2] add support in hwmon for MCP998X victor.duicu
2026-01-27 15:18 ` [PATCH v3 1/2] dt-bindings: hwmon: add support " victor.duicu
@ 2026-01-27 15:18 ` victor.duicu
2026-01-27 18:51 ` Guenter Roeck
` (4 more replies)
2026-02-06 16:55 ` [PATCH v3 0/2] add support in hwmon " Krzysztof Kozlowski
2 siblings, 5 replies; 18+ messages in thread
From: victor.duicu @ 2026-01-27 15:18 UTC (permalink / raw)
To: linux, robh, krzk+dt, conor+dt, corbet
Cc: marius.cristea, victor.duicu, linux-hwmon, linux-kernel,
devicetree, linux-doc
From: Victor Duicu <victor.duicu@microchip.com>
This is the driver for Microchip MCP998X/33 and MCP998XD/33D
Multichannel Automotive Temperature Monitor Family.
Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
---
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/mcp9982.rst | 105 ++++
MAINTAINERS | 2 +
drivers/hwmon/Kconfig | 11 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/mcp9982.c | 1022 +++++++++++++++++++++++++++++++
6 files changed, 1142 insertions(+)
create mode 100644 Documentation/hwmon/mcp9982.rst
create mode 100644 drivers/hwmon/mcp9982.c
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 85d7a686883e..b02709fc216e 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -173,6 +173,7 @@ Hardware Monitoring Kernel Drivers
mc33xs2410_hwmon
mc34vr500
mcp3021
+ mcp9982
menf21bmc
mlxreg-fan
mp2856
diff --git a/Documentation/hwmon/mcp9982.rst b/Documentation/hwmon/mcp9982.rst
new file mode 100644
index 000000000000..dee2947f9044
--- /dev/null
+++ b/Documentation/hwmon/mcp9982.rst
@@ -0,0 +1,105 @@
+.. SPDX-License-Identifier: GPL-2.0+
+Kernel driver MCP998X
+=====================
+
+Supported chips:
+
+ * Microchip Technology MCP998X/MCP9933 and MCP998XD/MCP9933D
+
+ Prefix: 'mcp9982'
+
+ Datasheet:
+ https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
+
+Authors:
+
+ - Victor Duicu <victor.duicu@microchip.com>
+
+Description
+-----------
+
+This driver implements support for the MCP998X family containing: MCP9982,
+MCP9982D, MCP9983, MCP9983D, MCP9984, MCP9984D, MCP9985, MCP9985D,
+MCP9933 and MCP9933D.
+
+The MCP998X Family is a high accuracy 2-wire multichannel automotive
+temperature monitor.
+
+The chips in the family have different numbers of external channels,
+ranging from 1 (MCP9982) to 4 channels (MCP9985). Reading diodes in
+anti-parallel connection is supported by MCP9984/85/33 and
+MCP9984D/85D/33D. Dedicated hardware shutdown circuitry is present
+only in MCP998XD and MCP9933D.
+
+Temperatures are read in millidegrees Celsius, ranging from -64 to
+191.875 with 0.125 precision.
+
+Each channel has a minimum, maximum, and critical limit alongside associated alarms.
+The chips also implement a hysteresis mechanism which applies only to the maximum
+and critical limits. The relative difference between a limit and its hysteresis
+is the same for both and the value is kept in a single register.
+
+The chips measure temperatures with a variable conversion rate.
+Update_interval = Conversion/Second, so the available options are:
+- 16000 (ms) = 1 conv/16 sec
+- 8000 (ms) = 1 conv/8 sec
+- 4000 (ms) = 1 conv/4 sec
+- 2000 (ms) = 1 conv/2 sec
+- 1000 (ms) = 1 conv/sec
+- 500 (ms) = 2 conv/sec
+- 250 (ms) = 4 conv/sec
+- 125 (ms) = 8 conv/sec
+- 64 (ms) = 16 conv/sec
+- 32 (ms) = 32 conv/sec
+- 16 (ms) = 64 conv/sec
+
+Usage Notes
+-----------
+
+Parameters that can be configured in devicetree:
+- anti-parallel diode mode operation
+- resistance error correction on channels 1 and 2
+- resistance error correction on channels 3 and 4
+Chips 82/83 and 82D/83D do not support anti-parallel diode mode.
+For chips with "D" in the name resistance error correction must be on.
+Please see Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
+for details.
+
+There are two power states:
+- Active state: in which the chip is converting on all channels at the
+ programmed rate.
+- Standby state: in which the host must initiate a conversion cycle.
+Chips with "D" in the name work in Active state only and those without
+can work in either state.
+
+Chips with "D" in the name can't set update interval slower than 1 second.
+
+Among the hysteresis attributes, only the tempX_crit_hyst ones are writeable
+while the others are read only. Setting tempX_crit_hyst writes the difference
+between tempX_crit and tempX_crit_hyst in the hysteresis register. The new value
+applies automatically to the other limits. At power up the device starts with
+the value 10 in the hysteresis register.
+
+Sysfs entries
+-------------
+
+The following attributes are supported. The limits and update_interval are
+read-write, while the rest are read only.
+
+======================= ==================================================
+temp[1-5]_label User name for channel.
+temp[1-5]_input Measured temperature for channel.
+
+temp[1-5]_crit Critical temperature limit.
+temp[1-5]_crit_alarm Critical temperature limit alarm.
+temp[1-5]_crit_hyst Critical temperature limit hysteresis.
+
+temp[1-5]_max High temperature limit.
+temp[1-5]_max_alarm High temperature limit alarm.
+temp[1-5]_max_hyst High temperature limit hysteresis.
+
+temp[1-5]_min Low temperature limit.
+temp[1-5]_min_alarm Low temperature limit alarm.
+
+update_interval The interval at which the chip will update readings.
+======================= ==================================================
diff --git a/MAINTAINERS b/MAINTAINERS
index f88d837cb586..c39ab12b6bc8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17154,6 +17154,8 @@ M: Victor Duicu <victor.duicu@microchip.com>
L: linux-hwmon@vger.kernel.org
S: Supported
F: Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
+F: Documentation/hwmon/mcp9982.rst
+F: drivers/hwmon/mcp9982.c
MICROCHIP MMC/SD/SDIO MCI DRIVER
M: Aubin Constans <aubin.constans@microchip.com>
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 157678b821fc..c758ab2d5fdf 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1388,6 +1388,17 @@ config SENSORS_MCP3021
This driver can also be built as a module. If so, the module
will be called mcp3021.
+config SENSORS_MCP9982
+ tristate "Microchip Technology MCP9982 driver"
+ depends on I2C
+ select REGMAP_I2C
+ help
+ Say yes here to include support for Microchip Technology's MCP998X/33
+ and MCP998XD/33D Multichannel Automotive Temperature Monitor Family.
+
+ This driver can also be built as a module. If so, the module
+ will be called mcp9982.
+
config SENSORS_MLXREG_FAN
tristate "Mellanox FAN driver"
depends on MELLANOX_PLATFORM
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index eade8e3b1bde..cec33da29a68 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -170,6 +170,7 @@ obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
obj-$(CONFIG_SENSORS_MC33XS2410) += mc33xs2410_hwmon.o
obj-$(CONFIG_SENSORS_MC34VR500) += mc34vr500.o
obj-$(CONFIG_SENSORS_MCP3021) += mcp3021.o
+obj-$(CONFIG_SENSORS_MCP9982) += mcp9982.o
obj-$(CONFIG_SENSORS_TC654) += tc654.o
obj-$(CONFIG_SENSORS_TPS23861) += tps23861.o
obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o
diff --git a/drivers/hwmon/mcp9982.c b/drivers/hwmon/mcp9982.c
new file mode 100644
index 000000000000..55701655daa0
--- /dev/null
+++ b/drivers/hwmon/mcp9982.c
@@ -0,0 +1,1022 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * HWMON driver for MCP998X/33 and MCP998XD/33D Multichannel Automotive
+ * Temperature Monitor Family
+ *
+ * Copyright (C) 2026 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Victor Duicu <victor.duicu@microchip.com>
+ *
+ * Datasheet can be found here:
+ * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/bits.h>
+#include <linux/byteorder/generic.h>
+#include <linux/delay.h>
+#include <linux/device/devres.h>
+#include <linux/device.h>
+#include <linux/dev_printk.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/i2c.h>
+#include <linux/math.h>
+#include <linux/minmax.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/time64.h>
+#include <linux/util_macros.h>
+
+/* MCP9982 Registers */
+#define MCP9982_HIGH_BYTE_ADDR(index) (2 * (index))
+#define MCP9982_ONE_SHOT_ADDR 0x0A
+#define MCP9982_INTERNAL_HIGH_LIMIT_ADDR 0x0B
+#define MCP9982_INTERNAL_LOW_LIMIT_ADDR 0x0C
+#define MCP9982_EXT_HIGH_LIMIT_ADDR(index) (4 * ((index) - 1) + 0x0D)
+#define MCP9982_EXT_LOW_LIMIT_ADDR(index) (4 * ((index) - 1) + 0x0F)
+#define MCP9982_THERM_LIMIT_ADDR(index) ((index) + 0x1D)
+#define MCP9982_CFG_ADDR 0x22
+#define MCP9982_CONV_ADDR 0x24
+#define MCP9982_HYS_ADDR 0x25
+#define MCP9982_CONSEC_ALRT_ADDR 0x26
+#define MCP9982_ALRT_CFG_ADDR 0x27
+#define MCP9982_RUNNING_AVG_ADDR 0x28
+#define MCP9982_HOTTEST_CFG_ADDR 0x29
+#define MCP9982_STATUS_ADDR 0x2A
+#define MCP9982_EXT_FAULT_STATUS_ADDR 0x2B
+#define MCP9982_HIGH_LIMIT_STATUS_ADDR 0x2C
+#define MCP9982_LOW_LIMIT_STATUS_ADDR 0x2D
+#define MCP9982_THERM_LIMIT_STATUS_ADDR 0x2E
+#define MCP9982_HOTTEST_HIGH_BYTE_ADDR 0x2F
+#define MCP9982_HOTTEST_LOW_BYTE_ADDR 0x30
+#define MCP9982_HOTTEST_STATUS_ADDR 0x31
+#define MCP9982_THERM_SHTDWN_CFG_ADDR 0x32
+#define MCP9982_HRDW_THERM_SHTDWN_LIMIT_ADDR 0x33
+#define MCP9982_EXT_BETA_CFG_ADDR(index) ((index) + 0x33)
+#define MCP9982_EXT_IDEAL_ADDR(index) ((index) + 0x35)
+
+/* MCP9982 Bits */
+#define MCP9982_CFG_MSKAL BIT(7)
+#define MCP9982_CFG_RS BIT(6)
+#define MCP9982_CFG_ATTHM BIT(5)
+#define MCP9982_CFG_RECD12 BIT(4)
+#define MCP9982_CFG_RECD34 BIT(3)
+#define MCP9982_CFG_RANGE BIT(2)
+#define MCP9982_CFG_DA_ENA BIT(1)
+#define MCP9982_CFG_APDD BIT(0)
+
+#define MCP9982_STATUS_BUSY BIT(5)
+
+/* Constants and default values */
+#define MCP9982_MAX_NUM_CHANNELS 5
+#define MCP9982_BETA_AUTODETECT 16
+#define MCP9982_IDEALITY_DEFAULT 18
+#define MCP9982_OFFSET 64
+#define MCP9982_DEFAULT_CONSEC_ALRT_VAL 112
+#define MCP9982_DEFAULT_HYS_VAL 10
+#define MCP9982_DEFAULT_CONV_VAL 6
+#define MCP9982_WAKE_UP_TIME_US 125000
+#define MCP9982_WAKE_UP_TIME_MAX_US 130000
+#define MCP9982_TIMER_BUFFER_US 10000
+#define MCP9982_CONVERSION_TIME_MS 125
+#define MCP9982_HIGH_LIMIT_DEFAULT 21000
+#define MCP9982_LOW_LIMIT_DEFAULT 0
+
+static const struct hwmon_channel_info * const mcp9985_info[] = {
+ HWMON_CHANNEL_INFO(temp,
+ HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MIN |
+ HWMON_T_MIN_ALARM | HWMON_T_MAX | HWMON_T_MAX_ALARM |
+ HWMON_T_MAX_HYST | HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
+ HWMON_T_CRIT_HYST,
+ HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MIN |
+ HWMON_T_MIN_ALARM | HWMON_T_MAX | HWMON_T_MAX_ALARM |
+ HWMON_T_MAX_HYST | HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
+ HWMON_T_CRIT_HYST,
+ HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MIN |
+ HWMON_T_MIN_ALARM | HWMON_T_MAX | HWMON_T_MAX_ALARM |
+ HWMON_T_MAX_HYST | HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
+ HWMON_T_CRIT_HYST,
+ HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MIN |
+ HWMON_T_MIN_ALARM | HWMON_T_MAX | HWMON_T_MAX_ALARM |
+ HWMON_T_MAX_HYST | HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
+ HWMON_T_CRIT_HYST,
+ HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MIN |
+ HWMON_T_MIN_ALARM | HWMON_T_MAX | HWMON_T_MAX_ALARM |
+ HWMON_T_MAX_HYST | HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
+ HWMON_T_CRIT_HYST),
+ HWMON_CHANNEL_INFO(chip,
+ HWMON_C_UPDATE_INTERVAL),
+ NULL
+};
+
+/**
+ * struct mcp9982_features - features of a mcp9982 instance
+ * @name: chip's name
+ * @phys_channels: number of physical channels supported by the chip
+ * @hw_thermal_shutdown: presence of hardware thermal shutdown circuitry
+ * @allow_apdd: whether the chip supports enabling APDD
+ */
+struct mcp9982_features {
+ const char *name;
+ u8 phys_channels;
+ bool hw_thermal_shutdown;
+ bool allow_apdd;
+};
+
+static const struct mcp9982_features mcp9933_chip_config = {
+ .name = "mcp9933",
+ .phys_channels = 3,
+ .hw_thermal_shutdown = false,
+ .allow_apdd = true,
+};
+
+static const struct mcp9982_features mcp9933d_chip_config = {
+ .name = "mcp9933d",
+ .phys_channels = 3,
+ .hw_thermal_shutdown = true,
+ .allow_apdd = true,
+};
+
+static const struct mcp9982_features mcp9982_chip_config = {
+ .name = "mcp9982",
+ .phys_channels = 2,
+ .hw_thermal_shutdown = false,
+ .allow_apdd = false,
+};
+
+static const struct mcp9982_features mcp9982d_chip_config = {
+ .name = "mcp9982d",
+ .phys_channels = 2,
+ .hw_thermal_shutdown = true,
+ .allow_apdd = false,
+};
+
+static const struct mcp9982_features mcp9983_chip_config = {
+ .name = "mcp9983",
+ .phys_channels = 3,
+ .hw_thermal_shutdown = false,
+ .allow_apdd = false,
+};
+
+static const struct mcp9982_features mcp9983d_chip_config = {
+ .name = "mcp9983d",
+ .phys_channels = 3,
+ .hw_thermal_shutdown = true,
+ .allow_apdd = false,
+};
+
+static const struct mcp9982_features mcp9984_chip_config = {
+ .name = "mcp9984",
+ .phys_channels = 4,
+ .hw_thermal_shutdown = false,
+ .allow_apdd = true,
+};
+
+static const struct mcp9982_features mcp9984d_chip_config = {
+ .name = "mcp9984d",
+ .phys_channels = 4,
+ .hw_thermal_shutdown = true,
+ .allow_apdd = true,
+};
+
+static const struct mcp9982_features mcp9985_chip_config = {
+ .name = "mcp9985",
+ .phys_channels = 5,
+ .hw_thermal_shutdown = false,
+ .allow_apdd = true,
+};
+
+static const struct mcp9982_features mcp9985d_chip_config = {
+ .name = "mcp9985d",
+ .phys_channels = 5,
+ .hw_thermal_shutdown = true,
+ .allow_apdd = true,
+};
+
+static const unsigned int mcp9982_update_interval[11] = {
+ 16000, 8000, 4000, 2000, 1000, 500, 250, 125, 64, 32, 16
+};
+
+/* MCP9982 regmap configuration */
+static const struct regmap_range mcp9982_regmap_wr_ranges[] = {
+ regmap_reg_range(MCP9982_ONE_SHOT_ADDR, MCP9982_CFG_ADDR),
+ regmap_reg_range(MCP9982_CONV_ADDR, MCP9982_HOTTEST_CFG_ADDR),
+ regmap_reg_range(MCP9982_THERM_SHTDWN_CFG_ADDR, MCP9982_THERM_SHTDWN_CFG_ADDR),
+ regmap_reg_range(MCP9982_EXT_BETA_CFG_ADDR(1), MCP9982_EXT_IDEAL_ADDR(4)),
+};
+
+static const struct regmap_access_table mcp9982_regmap_wr_table = {
+ .yes_ranges = mcp9982_regmap_wr_ranges,
+ .n_yes_ranges = ARRAY_SIZE(mcp9982_regmap_wr_ranges),
+};
+
+static const struct regmap_range mcp9982_regmap_rd_ranges[] = {
+ regmap_reg_range(MCP9982_HIGH_BYTE_ADDR(0), MCP9982_CFG_ADDR),
+ regmap_reg_range(MCP9982_CONV_ADDR, MCP9982_EXT_IDEAL_ADDR(4)),
+};
+
+static const struct regmap_access_table mcp9982_regmap_rd_table = {
+ .yes_ranges = mcp9982_regmap_rd_ranges,
+ .n_yes_ranges = ARRAY_SIZE(mcp9982_regmap_rd_ranges),
+};
+
+static bool mcp9982_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case MCP9982_ONE_SHOT_ADDR:
+ case MCP9982_INTERNAL_HIGH_LIMIT_ADDR:
+ case MCP9982_INTERNAL_LOW_LIMIT_ADDR:
+ case MCP9982_EXT_LOW_LIMIT_ADDR(1):
+ case MCP9982_EXT_LOW_LIMIT_ADDR(1) + 1:
+ case MCP9982_EXT_LOW_LIMIT_ADDR(2):
+ case MCP9982_EXT_LOW_LIMIT_ADDR(2) + 1:
+ case MCP9982_EXT_LOW_LIMIT_ADDR(3):
+ case MCP9982_EXT_LOW_LIMIT_ADDR(3) + 1:
+ case MCP9982_EXT_LOW_LIMIT_ADDR(4):
+ case MCP9982_EXT_LOW_LIMIT_ADDR(4) + 1:
+ case MCP9982_EXT_HIGH_LIMIT_ADDR(1):
+ case MCP9982_EXT_HIGH_LIMIT_ADDR(1) + 1:
+ case MCP9982_EXT_HIGH_LIMIT_ADDR(2):
+ case MCP9982_EXT_HIGH_LIMIT_ADDR(2) + 1:
+ case MCP9982_EXT_HIGH_LIMIT_ADDR(3):
+ case MCP9982_EXT_HIGH_LIMIT_ADDR(3) + 1:
+ case MCP9982_EXT_HIGH_LIMIT_ADDR(4):
+ case MCP9982_EXT_HIGH_LIMIT_ADDR(4) + 1:
+ 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):
+ case MCP9982_CFG_ADDR:
+ case MCP9982_CONV_ADDR:
+ case MCP9982_HYS_ADDR:
+ case MCP9982_CONSEC_ALRT_ADDR:
+ case MCP9982_ALRT_CFG_ADDR:
+ case MCP9982_RUNNING_AVG_ADDR:
+ case MCP9982_HOTTEST_CFG_ADDR:
+ case MCP9982_THERM_SHTDWN_CFG_ADDR:
+ return false;
+ default:
+ return true;
+ }
+}
+
+static const struct regmap_config mcp9982_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .rd_table = &mcp9982_regmap_rd_table,
+ .wr_table = &mcp9982_regmap_wr_table,
+ .volatile_reg = mcp9982_is_volatile_reg,
+ .max_register = MCP9982_EXT_IDEAL_ADDR(4),
+ .cache_type = REGCACHE_MAPLE,
+};
+
+/**
+ * struct mcp9992_priv - information about chip parameters
+ * @regmap: device register map
+ * @chip: pointer to structure holding chip features
+ * @labels: labels of the channels
+ * @interval_idx: index representing the current update interval
+ * @time_limit: time when it is safe to read
+ * @enabled_channel_mask: mask containing which channels should be enabled
+ * @num_channels: number of active physical channels
+ * @recd34_enable: state of Resistance Error Correction(REC) on channels 3 and 4
+ * @recd12_enable: state of Resistance Error Correction(REC) on channels 1 and 2
+ * @apdd_enable: state of anti-parallel diode mode
+ * @run_state: chip is in Run state, otherwise is in Standby state
+ * @wait_before_read: whether we need to wait a delay before reading a new value
+ */
+struct mcp9982_priv {
+ struct regmap *regmap;
+ const struct mcp9982_features *chip;
+ const char *labels[MCP9982_MAX_NUM_CHANNELS];
+ unsigned int interval_idx;
+ unsigned long time_limit;
+ unsigned long enabled_channel_mask;
+ u8 num_channels;
+ bool recd34_enable;
+ bool recd12_enable;
+ bool apdd_enable;
+ bool run_state;
+ bool wait_before_read;
+};
+
+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, ®_high);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(priv->regmap, address + 1, ®_low);
+ if (ret)
+ return ret;
+
+ reg_high = reg_high & 0xFF;
+ reg_low = reg_low & 0xFF;
+ *val = ((reg_high << 8) + reg_low) >> 5;
+ *val = (*val - (MCP9982_OFFSET << 3)) * 125;
+
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int mcp9982_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
+ long *val)
+{
+ struct mcp9982_priv *priv = dev_get_drvdata(dev);
+ unsigned int reg_status, reg_high, reg_low;
+ unsigned long sleep_time;
+ int ret, hyst;
+ u8 addr;
+
+ /*
+ * When working in Run mode, after modifying a parameter (like update
+ * interval) we have to wait a delay before reading the new values.
+ * We can't determine when the conversion is done based on the BUSY bit.
+ */
+ if (priv->run_state) {
+ if (priv->wait_before_read) {
+ sleep_time = jiffies_to_msecs(priv->time_limit - jiffies) * USEC_PER_MSEC;
+ if (!time_after(jiffies, priv->time_limit))
+ usleep_range(sleep_time, sleep_time + MCP9982_TIMER_BUFFER_US);
+
+ priv->wait_before_read = false;
+ }
+ } else {
+ ret = regmap_write(priv->regmap, MCP9982_ONE_SHOT_ADDR, 1);
+ if (ret)
+ return ret;
+
+ /*
+ * In Standby state after writing in OneShot register wait for
+ * the start of conversion and then poll the BUSY bit.
+ */
+ sleep_time = mcp9982_update_interval[priv->interval_idx] * USEC_PER_MSEC;
+ usleep_range(MCP9982_WAKE_UP_TIME_US, MCP9982_WAKE_UP_TIME_MAX_US);
+
+ ret = regmap_read_poll_timeout(priv->regmap, MCP9982_STATUS_ADDR,
+ reg_status, !(reg_status & MCP9982_STATUS_BUSY),
+ sleep_time, sleep_time * 2);
+ if (ret)
+ return ret;
+ }
+
+ switch (type) {
+ case hwmon_temp:
+ 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;
+
+ *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;
+ *val = clamp_val(*val, -64000, 191875);
+
+ return 0;
+ case hwmon_temp_min:
+ if (channel)
+ addr = MCP9982_EXT_LOW_LIMIT_ADDR(channel);
+ else
+ addr = MCP9982_INTERNAL_LOW_LIMIT_ADDR;
+
+ return mcp9982_read_limit(priv, addr, val);
+ case hwmon_temp_min_alarm:
+ *val = regmap_test_bits(priv->regmap, MCP9982_LOW_LIMIT_STATUS_ADDR,
+ BIT(channel));
+ if (*val < 0)
+ return *val;
+
+ return 0;
+ case hwmon_temp_crit:
+ return mcp9982_read_limit(priv, MCP9982_THERM_LIMIT_ADDR(channel), val);
+ case hwmon_temp_crit_alarm:
+ *val = regmap_test_bits(priv->regmap, MCP9982_THERM_LIMIT_STATUS_ADDR,
+ BIT(channel));
+ if (*val < 0)
+ return *val;
+
+ return 0;
+ case hwmon_temp_crit_hyst:
+ ret = mcp9982_read_limit(priv, MCP9982_THERM_LIMIT_ADDR(channel), val);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(priv->regmap, MCP9982_HYS_ADDR, &hyst);
+ if (ret)
+ return ret;
+
+ *val -= (hyst & 0xFF) * 1000;
+ *val = clamp_val(*val, -64000, 191875);
+
+ return 0;
+ default:
+ return -EINVAL;
+ }
+ case hwmon_chip:
+ switch (attr) {
+ case hwmon_chip_update_interval:
+ *val = mcp9982_update_interval[priv->interval_idx];
+ return 0;
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
+static int mcp9982_read_label(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+ int channel, const char **str)
+{
+ struct mcp9982_priv *priv = dev_get_drvdata(dev);
+
+ switch (type) {
+ case hwmon_temp:
+ switch (attr) {
+ case hwmon_temp_label:
+ *str = priv->labels[channel];
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int mcp9982_write_limit(struct mcp9982_priv *priv, u8 address, long val)
+{
+ int ret;
+
+ /* Range is always -64 to 191.875°C. */
+ val = clamp_val(val, -64000, 191875);
+ val = (val + MCP9982_OFFSET * 1000) << 5;
+ val = DIV_ROUND_CLOSEST(val, 125);
+
+ 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):
+ val = val >> 8;
+ val = val & 0xFF;
+ ret = regmap_write(priv->regmap, address, val);
+ if (ret)
+ return ret;
+
+ 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):
+ val = cpu_to_be16(val);
+ ret = regmap_bulk_write(priv->regmap, address, &val, 2);
+ if (ret)
+ return ret;
+
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int mcp9982_write_hyst(struct mcp9982_priv *priv, int channel, long val)
+{
+ int hyst, ret;
+ int limit;
+
+ val = clamp_val(val, -64000, 191875);
+ val = (val + MCP9982_OFFSET * 1000) << 5;
+ val = DIV_ROUND_CLOSEST(val, 125);
+ val = val >> 8;
+
+ /* Therm register is 8 bits and so it keeps only the integer part of the temperature. */
+ ret = regmap_read(priv->regmap, MCP9982_THERM_LIMIT_ADDR(channel), &limit);
+ if (ret)
+ return ret;
+
+ hyst = clamp_val(limit - val, 0, 255);
+
+ ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, hyst);
+
+ return ret;
+}
+
+static int mcp9982_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
+ long val)
+{
+ struct mcp9982_priv *priv = dev_get_drvdata(dev);
+ unsigned int previous_interval_idx, idx;
+ bool use_previous_interval = false;
+ unsigned long new_time_limit;
+ u8 addr;
+ int ret;
+
+ switch (type) {
+ case hwmon_chip:
+ switch (attr) {
+ case hwmon_chip_update_interval:
+ previous_interval_idx = priv->interval_idx;
+
+ /*
+ * For MCP998XD and MCP9933D update interval
+ * can't be slower than 1 second.
+ */
+ if (priv->chip->hw_thermal_shutdown)
+ val = clamp(val, 0, 1000);
+
+ idx = find_closest_descending(val, mcp9982_update_interval,
+ ARRAY_SIZE(mcp9982_update_interval));
+
+ ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, idx);
+ if (ret)
+ return ret;
+
+ priv->interval_idx = idx;
+
+ /*
+ * When changing the interval time in Run mode, wait a delay based
+ * on the previous value to ensure the new value becomes active.
+ */
+ if (priv->run_state)
+ use_previous_interval = true;
+ else
+ return 0;
+ break;
+ default:
+ return -EINVAL;
+ }
+ break;
+ case hwmon_temp:
+ switch (attr) {
+ case hwmon_temp_max:
+ if (channel)
+ addr = MCP9982_EXT_HIGH_LIMIT_ADDR(channel);
+ else
+ addr = MCP9982_INTERNAL_HIGH_LIMIT_ADDR;
+
+ return ret = mcp9982_write_limit(priv, addr, val);
+ case hwmon_temp_min:
+ if (channel)
+ addr = MCP9982_EXT_LOW_LIMIT_ADDR(channel);
+ else
+ addr = MCP9982_INTERNAL_LOW_LIMIT_ADDR;
+
+ return mcp9982_write_limit(priv, addr, val);
+ case hwmon_temp_crit:
+ return mcp9982_write_limit(priv, MCP9982_THERM_LIMIT_ADDR(channel), val);
+ case hwmon_temp_crit_hyst:
+ return mcp9982_write_hyst(priv, channel, val);
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
+
+ /*
+ * Calculate the time point when it would be safe to read after
+ * the current write operation in Run mode.
+ * If, for example, we change update interval from a slower time
+ * to a faster time, the change will become active after the
+ * conversion with the slower time is finished. If we read before
+ * the end of conversion, the value will be from the previous cycle.
+ */
+ if (use_previous_interval) {
+ new_time_limit = msecs_to_jiffies(mcp9982_update_interval[previous_interval_idx]);
+ use_previous_interval = false;
+ } else {
+ new_time_limit = msecs_to_jiffies(mcp9982_update_interval[priv->interval_idx]);
+ }
+
+ new_time_limit += jiffies + msecs_to_jiffies(MCP9982_CONVERSION_TIME_MS);
+
+ if (time_after(new_time_limit, priv->time_limit)) {
+ priv->time_limit = new_time_limit;
+ priv->wait_before_read = true;
+ }
+
+ return 0;
+}
+
+static umode_t mcp9982_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr,
+ int channel)
+{
+ const struct mcp9982_priv *priv = _data;
+
+ if (!test_bit(channel, &priv->enabled_channel_mask))
+ return 0;
+
+ switch (type) {
+ case hwmon_temp:
+ switch (attr) {
+ case hwmon_temp_label:
+ if (priv->labels[channel])
+ return 0444;
+ else
+ return 0;
+ case hwmon_temp_input:
+ case hwmon_temp_min_alarm:
+ case hwmon_temp_max_alarm:
+ case hwmon_temp_max_hyst:
+ case hwmon_temp_crit_alarm:
+ return 0444;
+ case hwmon_temp_min:
+ case hwmon_temp_max:
+ case hwmon_temp_crit:
+ case hwmon_temp_crit_hyst:
+ return 0644;
+ default:
+ return 0;
+ }
+ case hwmon_chip:
+ switch (attr) {
+ case hwmon_chip_update_interval:
+ return 0644;
+ default:
+ return 0;
+ }
+ default:
+ return 0;
+ }
+}
+
+static const struct hwmon_ops mcp9982_hwmon_ops = {
+ .is_visible = mcp9982_is_visible,
+ .read = mcp9982_read,
+ .read_string = mcp9982_read_label,
+ .write = mcp9982_write,
+};
+
+static int mcp9982_init(struct device *dev, struct mcp9982_priv *priv)
+{
+ unsigned int i;
+ int ret;
+ u8 val;
+
+ /* Chips 82/83 and 82D/83D do not support anti-parallel diode mode. */
+ if (!priv->chip->allow_apdd && priv->apdd_enable == 1)
+ return dev_err_probe(dev, -EINVAL, "Incorrect setting of APDD.\n");
+
+ /* Chips with "D" work only in Run state. */
+ if (priv->chip->hw_thermal_shutdown && !priv->run_state)
+ return dev_err_probe(dev, -EINVAL, "Incorrect setting of Power State.\n");
+
+ /*
+ * For chips with "D" in the name, resistance error correction must be on
+ * so that hardware shutdown feature can't be overridden.
+ */
+ if (priv->chip->hw_thermal_shutdown)
+ if (!priv->recd34_enable || !priv->recd12_enable)
+ return dev_err_probe(dev, -EINVAL, "Incorrect setting of RECD.\n");
+
+ /*
+ * Set default values in registers.
+ * APDD, RECD12 and RECD34 are active on 0.
+ */
+ val = FIELD_PREP(MCP9982_CFG_MSKAL, 1) |
+ FIELD_PREP(MCP9982_CFG_RS, !priv->run_state) |
+ FIELD_PREP(MCP9982_CFG_ATTHM, 1) |
+ FIELD_PREP(MCP9982_CFG_RECD12, !priv->recd12_enable) |
+ FIELD_PREP(MCP9982_CFG_RECD34, !priv->recd34_enable) |
+ FIELD_PREP(MCP9982_CFG_RANGE, 1) | FIELD_PREP(MCP9982_CFG_DA_ENA, 0) |
+ FIELD_PREP(MCP9982_CFG_APDD, !priv->apdd_enable);
+
+ ret = regmap_write(priv->regmap, MCP9982_CFG_ADDR, val);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, MCP9982_DEFAULT_CONV_VAL);
+ if (ret)
+ return ret;
+ priv->interval_idx = MCP9982_DEFAULT_CONV_VAL;
+
+ ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, MCP9982_DEFAULT_HYS_VAL);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(priv->regmap, MCP9982_CONSEC_ALRT_ADDR, MCP9982_DEFAULT_CONSEC_ALRT_VAL);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(priv->regmap, MCP9982_ALRT_CFG_ADDR, 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, 0);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(priv->regmap, MCP9982_HOTTEST_CFG_ADDR, 0);
+ if (ret)
+ return ret;
+
+ /*
+ * Only external channels 1 and 2 support beta compensation.
+ * Set beta auto-detection.
+ */
+ for (i = 1; i < 3; i++)
+ if (test_bit(i, &priv->enabled_channel_mask)) {
+ ret = regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(i),
+ MCP9982_BETA_AUTODETECT);
+ if (ret)
+ return ret;
+ }
+
+ /* Set default values for internal channel limits. */
+ if (test_bit(0, &priv->enabled_channel_mask)) {
+ ret = mcp9982_write_limit(priv, MCP9982_INTERNAL_HIGH_LIMIT_ADDR,
+ MCP9982_HIGH_LIMIT_DEFAULT);
+ if (ret)
+ return ret;
+
+ ret = mcp9982_write_limit(priv, MCP9982_INTERNAL_LOW_LIMIT_ADDR,
+ MCP9982_LOW_LIMIT_DEFAULT);
+ if (ret)
+ return ret;
+
+ ret = mcp9982_write_limit(priv, MCP9982_THERM_LIMIT_ADDR(0),
+ MCP9982_HIGH_LIMIT_DEFAULT);
+ if (ret)
+ return ret;
+ }
+
+ /* Set ideality factor and limits to default for external channels. */
+ for (i = 1; i < MCP9982_MAX_NUM_CHANNELS; i++)
+ if (test_bit(i, &priv->enabled_channel_mask)) {
+ ret = regmap_write(priv->regmap, MCP9982_EXT_IDEAL_ADDR(i),
+ MCP9982_IDEALITY_DEFAULT);
+ if (ret)
+ return ret;
+
+ ret = mcp9982_write_limit(priv, MCP9982_EXT_HIGH_LIMIT_ADDR(i),
+ MCP9982_HIGH_LIMIT_DEFAULT);
+ if (ret)
+ return ret;
+
+ ret = mcp9982_write_limit(priv, MCP9982_EXT_LOW_LIMIT_ADDR(i),
+ MCP9982_LOW_LIMIT_DEFAULT);
+ if (ret)
+ return ret;
+
+ ret = mcp9982_write_limit(priv, MCP9982_THERM_LIMIT_ADDR(i),
+ MCP9982_HIGH_LIMIT_DEFAULT);
+ if (ret)
+ return ret;
+ }
+
+ priv->wait_before_read = false;
+ priv->time_limit = jiffies;
+
+ return 0;
+}
+
+static int mcp9982_parse_fw_config(struct device *dev, int device_nr_channels)
+{
+ unsigned int reg_nr;
+ struct mcp9982_priv *priv = dev_get_drvdata(dev);
+ int ret;
+
+ /* Initialise internal channel( which is always present ). */
+ priv->labels[0] = "internal diode";
+ priv->enabled_channel_mask = 1;
+
+ /* Default values to work on systems without devicetree or firmware nodes. */
+ if (!dev_fwnode(dev)) {
+ priv->num_channels = device_nr_channels;
+ priv->enabled_channel_mask = BIT(priv->num_channels) - 1;
+ priv->apdd_enable = false;
+ priv->recd12_enable = true;
+ priv->recd34_enable = true;
+ priv->run_state = true;
+ return 0;
+ }
+
+ priv->apdd_enable =
+ device_property_read_bool(dev, "microchip,enable-anti-parallel");
+
+ priv->recd12_enable =
+ device_property_read_bool(dev, "microchip,parasitic-res-on-channel1-2");
+
+ priv->recd34_enable =
+ device_property_read_bool(dev, "microchip,parasitic-res-on-channel3-4");
+
+ priv->run_state =
+ device_property_read_bool(dev, "microchip,power-state");
+
+ priv->num_channels = device_get_child_node_count(dev) + 1;
+
+ if (priv->num_channels > device_nr_channels)
+ return dev_err_probe(dev, -EINVAL,
+ "More channels than the chip supports.\n");
+
+ /* Read information about the external channels. */
+ device_for_each_child_node_scoped(dev, child) {
+ reg_nr = 0;
+ ret = fwnode_property_read_u32(child, "reg", ®_nr);
+ if (ret || !reg_nr || reg_nr >= device_nr_channels)
+ return dev_err_probe(dev, -EINVAL,
+ "Channel reg is incorrectly set.\n");
+
+ fwnode_property_read_string(child, "label", &priv->labels[reg_nr]);
+ set_bit(reg_nr, &priv->enabled_channel_mask);
+ }
+
+ return 0;
+}
+
+static int mcp9982_probe(struct i2c_client *client)
+{
+ struct hwmon_chip_info mcp998x_chip_info;
+ const struct mcp9982_features *chip;
+ struct device *dev = &client->dev;
+ struct mcp9982_priv *priv;
+ struct device *hwmon_dev;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(struct mcp9982_priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->regmap = devm_regmap_init_i2c(client, &mcp9982_regmap_config);
+
+ if (IS_ERR(priv->regmap))
+ return dev_err_probe(dev, PTR_ERR(priv->regmap),
+ "Cannot initialize register map.\n");
+
+ dev_set_drvdata(dev, priv);
+
+ chip = i2c_get_match_data(client);
+ if (!chip)
+ return -EINVAL;
+ priv->chip = chip;
+
+ ret = mcp9982_parse_fw_config(dev, chip->phys_channels);
+ if (ret)
+ return ret;
+
+ ret = mcp9982_init(dev, priv);
+ if (ret)
+ return ret;
+
+ mcp998x_chip_info.ops = &mcp9982_hwmon_ops;
+ mcp998x_chip_info.info = mcp9985_info;
+
+ hwmon_dev = devm_hwmon_device_register_with_info(dev, chip->name, priv,
+ &mcp998x_chip_info, NULL);
+
+ return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct i2c_device_id mcp9982_id[] = {
+ { .name = "mcp9933", .driver_data = (kernel_ulong_t)&mcp9933_chip_config },
+ { .name = "mcp9933d", .driver_data = (kernel_ulong_t)&mcp9933d_chip_config },
+ { .name = "mcp9982", .driver_data = (kernel_ulong_t)&mcp9982_chip_config },
+ { .name = "mcp9982d", .driver_data = (kernel_ulong_t)&mcp9982d_chip_config },
+ { .name = "mcp9983", .driver_data = (kernel_ulong_t)&mcp9983_chip_config },
+ { .name = "mcp9983d", .driver_data = (kernel_ulong_t)&mcp9983d_chip_config },
+ { .name = "mcp9984", .driver_data = (kernel_ulong_t)&mcp9984_chip_config },
+ { .name = "mcp9984d", .driver_data = (kernel_ulong_t)&mcp9984d_chip_config },
+ { .name = "mcp9985", .driver_data = (kernel_ulong_t)&mcp9985_chip_config },
+ { .name = "mcp9985d", .driver_data = (kernel_ulong_t)&mcp9985d_chip_config },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, mcp9982_id);
+
+static const struct of_device_id mcp9982_of_match[] = {
+ {
+ .compatible = "microchip,mcp9933",
+ .data = &mcp9933_chip_config,
+ }, {
+ .compatible = "microchip,mcp9933d",
+ .data = &mcp9933d_chip_config,
+ }, {
+ .compatible = "microchip,mcp9982",
+ .data = &mcp9982_chip_config,
+ }, {
+ .compatible = "microchip,mcp9982d",
+ .data = &mcp9982d_chip_config,
+ }, {
+ .compatible = "microchip,mcp9983",
+ .data = &mcp9983_chip_config,
+ }, {
+ .compatible = "microchip,mcp9983d",
+ .data = &mcp9983d_chip_config,
+ }, {
+ .compatible = "microchip,mcp9984",
+ .data = &mcp9984_chip_config,
+ }, {
+ .compatible = "microchip,mcp9984d",
+ .data = &mcp9984d_chip_config,
+ }, {
+ .compatible = "microchip,mcp9985",
+ .data = &mcp9985_chip_config,
+ }, {
+ .compatible = "microchip,mcp9985d",
+ .data = &mcp9985d_chip_config,
+ },
+ { }
+};
+MODULE_DEVICE_TABLE(of, mcp9982_of_match);
+
+static struct i2c_driver mcp9982_driver = {
+ .driver = {
+ .name = "mcp9982",
+ .of_match_table = mcp9982_of_match,
+ },
+ .probe = mcp9982_probe,
+ .id_table = mcp9982_id,
+};
+module_i2c_driver(mcp9982_driver);
+
+MODULE_AUTHOR("Victor Duicu <victor.duicu@microchip.com>");
+MODULE_DESCRIPTION("MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Driver");
+MODULE_LICENSE("GPL");
--
2.51.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] hwmon: add support for MCP998X
2026-01-27 15:18 ` [PATCH v3 2/2] " victor.duicu
@ 2026-01-27 18:51 ` Guenter Roeck
2026-02-02 8:15 ` Victor.Duicu
2026-01-27 22:23 ` kernel test robot
` (3 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2026-01-27 18:51 UTC (permalink / raw)
To: victor.duicu, robh, krzk+dt, conor+dt, corbet
Cc: marius.cristea, linux-hwmon, linux-kernel, devicetree, linux-doc
On 1/27/26 07:18, victor.duicu@microchip.com wrote:
> From: Victor Duicu <victor.duicu@microchip.com>
>
> This is the driver for Microchip MCP998X/33 and MCP998XD/33D
> Multichannel Automotive Temperature Monitor Family.
>
> Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
> ---
> Documentation/hwmon/index.rst | 1 +
> Documentation/hwmon/mcp9982.rst | 105 ++++
> MAINTAINERS | 2 +
> drivers/hwmon/Kconfig | 11 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/mcp9982.c | 1022 +++++++++++++++++++++++++++++++
> 6 files changed, 1142 insertions(+)
> create mode 100644 Documentation/hwmon/mcp9982.rst
> create mode 100644 drivers/hwmon/mcp9982.c
>
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 85d7a686883e..b02709fc216e 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -173,6 +173,7 @@ Hardware Monitoring Kernel Drivers
> mc33xs2410_hwmon
> mc34vr500
> mcp3021
> + mcp9982
> menf21bmc
> mlxreg-fan
> mp2856
> diff --git a/Documentation/hwmon/mcp9982.rst b/Documentation/hwmon/mcp9982.rst
> new file mode 100644
> index 000000000000..dee2947f9044
> --- /dev/null
> +++ b/Documentation/hwmon/mcp9982.rst
> @@ -0,0 +1,105 @@
> +.. SPDX-License-Identifier: GPL-2.0+
> +Kernel driver MCP998X
> +=====================
> +
> +Supported chips:
> +
> + * Microchip Technology MCP998X/MCP9933 and MCP998XD/MCP9933D
> +
> + Prefix: 'mcp9982'
> +
> + Datasheet:
> + https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
> +
> +Authors:
> +
> + - Victor Duicu <victor.duicu@microchip.com>
> +
> +Description
> +-----------
> +
> +This driver implements support for the MCP998X family containing: MCP9982,
> +MCP9982D, MCP9983, MCP9983D, MCP9984, MCP9984D, MCP9985, MCP9985D,
> +MCP9933 and MCP9933D.
> +
> +The MCP998X Family is a high accuracy 2-wire multichannel automotive
> +temperature monitor.
> +
> +The chips in the family have different numbers of external channels,
> +ranging from 1 (MCP9982) to 4 channels (MCP9985). Reading diodes in
> +anti-parallel connection is supported by MCP9984/85/33 and
> +MCP9984D/85D/33D. Dedicated hardware shutdown circuitry is present
> +only in MCP998XD and MCP9933D.
> +
> +Temperatures are read in millidegrees Celsius, ranging from -64 to
> +191.875 with 0.125 precision.
> +
> +Each channel has a minimum, maximum, and critical limit alongside associated alarms.
> +The chips also implement a hysteresis mechanism which applies only to the maximum
> +and critical limits. The relative difference between a limit and its hysteresis
> +is the same for both and the value is kept in a single register.
> +
> +The chips measure temperatures with a variable conversion rate.
> +Update_interval = Conversion/Second, so the available options are:
> +- 16000 (ms) = 1 conv/16 sec
> +- 8000 (ms) = 1 conv/8 sec
> +- 4000 (ms) = 1 conv/4 sec
> +- 2000 (ms) = 1 conv/2 sec
> +- 1000 (ms) = 1 conv/sec
> +- 500 (ms) = 2 conv/sec
> +- 250 (ms) = 4 conv/sec
> +- 125 (ms) = 8 conv/sec
> +- 64 (ms) = 16 conv/sec
> +- 32 (ms) = 32 conv/sec
> +- 16 (ms) = 64 conv/sec
> +
> +Usage Notes
> +-----------
> +
> +Parameters that can be configured in devicetree:
> +- anti-parallel diode mode operation
> +- resistance error correction on channels 1 and 2
> +- resistance error correction on channels 3 and 4
> +Chips 82/83 and 82D/83D do not support anti-parallel diode mode.
> +For chips with "D" in the name resistance error correction must be on.
> +Please see Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> +for details.
> +
> +There are two power states:
> +- Active state: in which the chip is converting on all channels at the
> + programmed rate.
> +- Standby state: in which the host must initiate a conversion cycle.
> +Chips with "D" in the name work in Active state only and those without
> +can work in either state.
> +
> +Chips with "D" in the name can't set update interval slower than 1 second.
> +
> +Among the hysteresis attributes, only the tempX_crit_hyst ones are writeable
> +while the others are read only. Setting tempX_crit_hyst writes the difference
> +between tempX_crit and tempX_crit_hyst in the hysteresis register. The new value
> +applies automatically to the other limits. At power up the device starts with
> +the value 10 in the hysteresis register.
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported. The limits and update_interval are
> +read-write, while the rest are read only.
> +
> +======================= ==================================================
> +temp[1-5]_label User name for channel.
> +temp[1-5]_input Measured temperature for channel.
> +
> +temp[1-5]_crit Critical temperature limit.
> +temp[1-5]_crit_alarm Critical temperature limit alarm.
> +temp[1-5]_crit_hyst Critical temperature limit hysteresis.
> +
> +temp[1-5]_max High temperature limit.
> +temp[1-5]_max_alarm High temperature limit alarm.
> +temp[1-5]_max_hyst High temperature limit hysteresis.
> +
> +temp[1-5]_min Low temperature limit.
> +temp[1-5]_min_alarm Low temperature limit alarm.
> +
> +update_interval The interval at which the chip will update readings.
> +======================= ==================================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f88d837cb586..c39ab12b6bc8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17154,6 +17154,8 @@ M: Victor Duicu <victor.duicu@microchip.com>
> L: linux-hwmon@vger.kernel.org
> S: Supported
> F: Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> +F: Documentation/hwmon/mcp9982.rst
> +F: drivers/hwmon/mcp9982.c
>
> MICROCHIP MMC/SD/SDIO MCI DRIVER
> M: Aubin Constans <aubin.constans@microchip.com>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 157678b821fc..c758ab2d5fdf 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1388,6 +1388,17 @@ config SENSORS_MCP3021
> This driver can also be built as a module. If so, the module
> will be called mcp3021.
>
> +config SENSORS_MCP9982
> + tristate "Microchip Technology MCP9982 driver"
> + depends on I2C
> + select REGMAP_I2C
> + help
> + Say yes here to include support for Microchip Technology's MCP998X/33
> + and MCP998XD/33D Multichannel Automotive Temperature Monitor Family.
> +
> + This driver can also be built as a module. If so, the module
> + will be called mcp9982.
> +
> config SENSORS_MLXREG_FAN
> tristate "Mellanox FAN driver"
> depends on MELLANOX_PLATFORM
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index eade8e3b1bde..cec33da29a68 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -170,6 +170,7 @@ obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
> obj-$(CONFIG_SENSORS_MC33XS2410) += mc33xs2410_hwmon.o
> obj-$(CONFIG_SENSORS_MC34VR500) += mc34vr500.o
> obj-$(CONFIG_SENSORS_MCP3021) += mcp3021.o
> +obj-$(CONFIG_SENSORS_MCP9982) += mcp9982.o
> obj-$(CONFIG_SENSORS_TC654) += tc654.o
> obj-$(CONFIG_SENSORS_TPS23861) += tps23861.o
> obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o
> diff --git a/drivers/hwmon/mcp9982.c b/drivers/hwmon/mcp9982.c
> new file mode 100644
> index 000000000000..55701655daa0
> --- /dev/null
> +++ b/drivers/hwmon/mcp9982.c
> @@ -0,0 +1,1022 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * HWMON driver for MCP998X/33 and MCP998XD/33D Multichannel Automotive
> + * Temperature Monitor Family
> + *
> + * Copyright (C) 2026 Microchip Technology Inc. and its subsidiaries
> + *
> + * Author: Victor Duicu <victor.duicu@microchip.com>
> + *
> + * Datasheet can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/bits.h>
> +#include <linux/byteorder/generic.h>
> +#include <linux/delay.h>
> +#include <linux/device/devres.h>
> +#include <linux/device.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/i2c.h>
> +#include <linux/math.h>
> +#include <linux/minmax.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/time64.h>
> +#include <linux/util_macros.h>
> +
> +/* MCP9982 Registers */
> +#define MCP9982_HIGH_BYTE_ADDR(index) (2 * (index))
> +#define MCP9982_ONE_SHOT_ADDR 0x0A
> +#define MCP9982_INTERNAL_HIGH_LIMIT_ADDR 0x0B
> +#define MCP9982_INTERNAL_LOW_LIMIT_ADDR 0x0C
> +#define MCP9982_EXT_HIGH_LIMIT_ADDR(index) (4 * ((index) - 1) + 0x0D)
> +#define MCP9982_EXT_LOW_LIMIT_ADDR(index) (4 * ((index) - 1) + 0x0F)
> +#define MCP9982_THERM_LIMIT_ADDR(index) ((index) + 0x1D)
> +#define MCP9982_CFG_ADDR 0x22
> +#define MCP9982_CONV_ADDR 0x24
> +#define MCP9982_HYS_ADDR 0x25
> +#define MCP9982_CONSEC_ALRT_ADDR 0x26
> +#define MCP9982_ALRT_CFG_ADDR 0x27
> +#define MCP9982_RUNNING_AVG_ADDR 0x28
> +#define MCP9982_HOTTEST_CFG_ADDR 0x29
> +#define MCP9982_STATUS_ADDR 0x2A
> +#define MCP9982_EXT_FAULT_STATUS_ADDR 0x2B
> +#define MCP9982_HIGH_LIMIT_STATUS_ADDR 0x2C
> +#define MCP9982_LOW_LIMIT_STATUS_ADDR 0x2D
> +#define MCP9982_THERM_LIMIT_STATUS_ADDR 0x2E
> +#define MCP9982_HOTTEST_HIGH_BYTE_ADDR 0x2F
> +#define MCP9982_HOTTEST_LOW_BYTE_ADDR 0x30
> +#define MCP9982_HOTTEST_STATUS_ADDR 0x31
> +#define MCP9982_THERM_SHTDWN_CFG_ADDR 0x32
> +#define MCP9982_HRDW_THERM_SHTDWN_LIMIT_ADDR 0x33
> +#define MCP9982_EXT_BETA_CFG_ADDR(index) ((index) + 0x33)
> +#define MCP9982_EXT_IDEAL_ADDR(index) ((index) + 0x35)
> +
> +/* MCP9982 Bits */
> +#define MCP9982_CFG_MSKAL BIT(7)
> +#define MCP9982_CFG_RS BIT(6)
> +#define MCP9982_CFG_ATTHM BIT(5)
> +#define MCP9982_CFG_RECD12 BIT(4)
> +#define MCP9982_CFG_RECD34 BIT(3)
> +#define MCP9982_CFG_RANGE BIT(2)
> +#define MCP9982_CFG_DA_ENA BIT(1)
> +#define MCP9982_CFG_APDD BIT(0)
> +
> +#define MCP9982_STATUS_BUSY BIT(5)
> +
> +/* Constants and default values */
> +#define MCP9982_MAX_NUM_CHANNELS 5
> +#define MCP9982_BETA_AUTODETECT 16
> +#define MCP9982_IDEALITY_DEFAULT 18
> +#define MCP9982_OFFSET 64
> +#define MCP9982_DEFAULT_CONSEC_ALRT_VAL 112
> +#define MCP9982_DEFAULT_HYS_VAL 10
> +#define MCP9982_DEFAULT_CONV_VAL 6
> +#define MCP9982_WAKE_UP_TIME_US 125000
> +#define MCP9982_WAKE_UP_TIME_MAX_US 130000
> +#define MCP9982_TIMER_BUFFER_US 10000
> +#define MCP9982_CONVERSION_TIME_MS 125
> +#define MCP9982_HIGH_LIMIT_DEFAULT 21000
> +#define MCP9982_LOW_LIMIT_DEFAULT 0
> +
> +static const struct hwmon_channel_info * const mcp9985_info[] = {
> + HWMON_CHANNEL_INFO(temp,
> + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MIN |
> + HWMON_T_MIN_ALARM | HWMON_T_MAX | HWMON_T_MAX_ALARM |
> + HWMON_T_MAX_HYST | HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
> + HWMON_T_CRIT_HYST,
> + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MIN |
> + HWMON_T_MIN_ALARM | HWMON_T_MAX | HWMON_T_MAX_ALARM |
> + HWMON_T_MAX_HYST | HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
> + HWMON_T_CRIT_HYST,
> + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MIN |
> + HWMON_T_MIN_ALARM | HWMON_T_MAX | HWMON_T_MAX_ALARM |
> + HWMON_T_MAX_HYST | HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
> + HWMON_T_CRIT_HYST,
> + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MIN |
> + HWMON_T_MIN_ALARM | HWMON_T_MAX | HWMON_T_MAX_ALARM |
> + HWMON_T_MAX_HYST | HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
> + HWMON_T_CRIT_HYST,
> + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MIN |
> + HWMON_T_MIN_ALARM | HWMON_T_MAX | HWMON_T_MAX_ALARM |
> + HWMON_T_MAX_HYST | HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
> + HWMON_T_CRIT_HYST),
> + HWMON_CHANNEL_INFO(chip,
> + HWMON_C_UPDATE_INTERVAL),
> + NULL
> +};
> +
> +/**
> + * struct mcp9982_features - features of a mcp9982 instance
> + * @name: chip's name
> + * @phys_channels: number of physical channels supported by the chip
> + * @hw_thermal_shutdown: presence of hardware thermal shutdown circuitry
> + * @allow_apdd: whether the chip supports enabling APDD
> + */
> +struct mcp9982_features {
> + const char *name;
> + u8 phys_channels;
> + bool hw_thermal_shutdown;
> + bool allow_apdd;
> +};
> +
> +static const struct mcp9982_features mcp9933_chip_config = {
> + .name = "mcp9933",
> + .phys_channels = 3,
> + .hw_thermal_shutdown = false,
> + .allow_apdd = true,
> +};
> +
> +static const struct mcp9982_features mcp9933d_chip_config = {
> + .name = "mcp9933d",
> + .phys_channels = 3,
> + .hw_thermal_shutdown = true,
> + .allow_apdd = true,
> +};
> +
> +static const struct mcp9982_features mcp9982_chip_config = {
> + .name = "mcp9982",
> + .phys_channels = 2,
> + .hw_thermal_shutdown = false,
> + .allow_apdd = false,
> +};
> +
> +static const struct mcp9982_features mcp9982d_chip_config = {
> + .name = "mcp9982d",
> + .phys_channels = 2,
> + .hw_thermal_shutdown = true,
> + .allow_apdd = false,
> +};
> +
> +static const struct mcp9982_features mcp9983_chip_config = {
> + .name = "mcp9983",
> + .phys_channels = 3,
> + .hw_thermal_shutdown = false,
> + .allow_apdd = false,
> +};
> +
> +static const struct mcp9982_features mcp9983d_chip_config = {
> + .name = "mcp9983d",
> + .phys_channels = 3,
> + .hw_thermal_shutdown = true,
> + .allow_apdd = false,
> +};
> +
> +static const struct mcp9982_features mcp9984_chip_config = {
> + .name = "mcp9984",
> + .phys_channels = 4,
> + .hw_thermal_shutdown = false,
> + .allow_apdd = true,
> +};
> +
> +static const struct mcp9982_features mcp9984d_chip_config = {
> + .name = "mcp9984d",
> + .phys_channels = 4,
> + .hw_thermal_shutdown = true,
> + .allow_apdd = true,
> +};
> +
> +static const struct mcp9982_features mcp9985_chip_config = {
> + .name = "mcp9985",
> + .phys_channels = 5,
> + .hw_thermal_shutdown = false,
> + .allow_apdd = true,
> +};
> +
> +static const struct mcp9982_features mcp9985d_chip_config = {
> + .name = "mcp9985d",
> + .phys_channels = 5,
> + .hw_thermal_shutdown = true,
> + .allow_apdd = true,
> +};
> +
> +static const unsigned int mcp9982_update_interval[11] = {
> + 16000, 8000, 4000, 2000, 1000, 500, 250, 125, 64, 32, 16
> +};
> +
> +/* MCP9982 regmap configuration */
> +static const struct regmap_range mcp9982_regmap_wr_ranges[] = {
> + regmap_reg_range(MCP9982_ONE_SHOT_ADDR, MCP9982_CFG_ADDR),
> + regmap_reg_range(MCP9982_CONV_ADDR, MCP9982_HOTTEST_CFG_ADDR),
> + regmap_reg_range(MCP9982_THERM_SHTDWN_CFG_ADDR, MCP9982_THERM_SHTDWN_CFG_ADDR),
> + regmap_reg_range(MCP9982_EXT_BETA_CFG_ADDR(1), MCP9982_EXT_IDEAL_ADDR(4)),
> +};
> +
> +static const struct regmap_access_table mcp9982_regmap_wr_table = {
> + .yes_ranges = mcp9982_regmap_wr_ranges,
> + .n_yes_ranges = ARRAY_SIZE(mcp9982_regmap_wr_ranges),
> +};
> +
> +static const struct regmap_range mcp9982_regmap_rd_ranges[] = {
> + regmap_reg_range(MCP9982_HIGH_BYTE_ADDR(0), MCP9982_CFG_ADDR),
> + regmap_reg_range(MCP9982_CONV_ADDR, MCP9982_EXT_IDEAL_ADDR(4)),
> +};
> +
> +static const struct regmap_access_table mcp9982_regmap_rd_table = {
> + .yes_ranges = mcp9982_regmap_rd_ranges,
> + .n_yes_ranges = ARRAY_SIZE(mcp9982_regmap_rd_ranges),
> +};
> +
> +static bool mcp9982_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case MCP9982_ONE_SHOT_ADDR:
> + case MCP9982_INTERNAL_HIGH_LIMIT_ADDR:
> + case MCP9982_INTERNAL_LOW_LIMIT_ADDR:
> + case MCP9982_EXT_LOW_LIMIT_ADDR(1):
> + case MCP9982_EXT_LOW_LIMIT_ADDR(1) + 1:
> + case MCP9982_EXT_LOW_LIMIT_ADDR(2):
> + case MCP9982_EXT_LOW_LIMIT_ADDR(2) + 1:
> + case MCP9982_EXT_LOW_LIMIT_ADDR(3):
> + case MCP9982_EXT_LOW_LIMIT_ADDR(3) + 1:
> + case MCP9982_EXT_LOW_LIMIT_ADDR(4):
> + case MCP9982_EXT_LOW_LIMIT_ADDR(4) + 1:
> + case MCP9982_EXT_HIGH_LIMIT_ADDR(1):
> + case MCP9982_EXT_HIGH_LIMIT_ADDR(1) + 1:
> + case MCP9982_EXT_HIGH_LIMIT_ADDR(2):
> + case MCP9982_EXT_HIGH_LIMIT_ADDR(2) + 1:
> + case MCP9982_EXT_HIGH_LIMIT_ADDR(3):
> + case MCP9982_EXT_HIGH_LIMIT_ADDR(3) + 1:
> + case MCP9982_EXT_HIGH_LIMIT_ADDR(4):
> + case MCP9982_EXT_HIGH_LIMIT_ADDR(4) + 1:
> + 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):
> + case MCP9982_CFG_ADDR:
> + case MCP9982_CONV_ADDR:
> + case MCP9982_HYS_ADDR:
> + case MCP9982_CONSEC_ALRT_ADDR:
> + case MCP9982_ALRT_CFG_ADDR:
> + case MCP9982_RUNNING_AVG_ADDR:
> + case MCP9982_HOTTEST_CFG_ADDR:
> + case MCP9982_THERM_SHTDWN_CFG_ADDR:
> + return false;
> + default:
> + return true;
> + }
> +}
> +
> +static const struct regmap_config mcp9982_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .rd_table = &mcp9982_regmap_rd_table,
> + .wr_table = &mcp9982_regmap_wr_table,
> + .volatile_reg = mcp9982_is_volatile_reg,
> + .max_register = MCP9982_EXT_IDEAL_ADDR(4),
> + .cache_type = REGCACHE_MAPLE,
> +};
> +
> +/**
> + * struct mcp9992_priv - information about chip parameters
> + * @regmap: device register map
> + * @chip: pointer to structure holding chip features
> + * @labels: labels of the channels
> + * @interval_idx: index representing the current update interval
> + * @time_limit: time when it is safe to read
> + * @enabled_channel_mask: mask containing which channels should be enabled
> + * @num_channels: number of active physical channels
> + * @recd34_enable: state of Resistance Error Correction(REC) on channels 3 and 4
> + * @recd12_enable: state of Resistance Error Correction(REC) on channels 1 and 2
> + * @apdd_enable: state of anti-parallel diode mode
> + * @run_state: chip is in Run state, otherwise is in Standby state
> + * @wait_before_read: whether we need to wait a delay before reading a new value
> + */
> +struct mcp9982_priv {
> + struct regmap *regmap;
> + const struct mcp9982_features *chip;
> + const char *labels[MCP9982_MAX_NUM_CHANNELS];
> + unsigned int interval_idx;
> + unsigned long time_limit;
> + unsigned long enabled_channel_mask;
> + u8 num_channels;
> + bool recd34_enable;
> + bool recd12_enable;
> + bool apdd_enable;
> + bool run_state;
> + bool wait_before_read;
> +};
> +
> +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, ®_high);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(priv->regmap, address + 1, ®_low);
> + if (ret)
> + return ret;
> +
Consider using regmap_bulk_read().
> + reg_high = reg_high & 0xFF;
> + reg_low = reg_low & 0xFF;
The masks seem pointless. Same pretty much everywhere. If not, provide explanation
why they are needed ("I don't trust regmap" is not a reason).
> + *val = ((reg_high << 8) + reg_low) >> 5;
> + *val = (*val - (MCP9982_OFFSET << 3)) * 125;
> +
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mcp9982_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
> + long *val)
> +{
> + struct mcp9982_priv *priv = dev_get_drvdata(dev);
> + unsigned int reg_status, reg_high, reg_low;
> + unsigned long sleep_time;
> + int ret, hyst;
> + u8 addr;
> +
> + /*
> + * When working in Run mode, after modifying a parameter (like update
> + * interval) we have to wait a delay before reading the new values.
> + * We can't determine when the conversion is done based on the BUSY bit.
> + */
> + if (priv->run_state) {
> + if (priv->wait_before_read) {
> + sleep_time = jiffies_to_msecs(priv->time_limit - jiffies) * USEC_PER_MSEC;
> + if (!time_after(jiffies, priv->time_limit))
> + usleep_range(sleep_time, sleep_time + MCP9982_TIMER_BUFFER_US);
> +
According to the datasheet, the actual conversion time is always 25 ms per channel,
and does not depend on the conversion interval.
Even if it the conversion time would depend on the conversion interval, forcing
the driver to wait until the next conversion is complete seems excessive. Why wait
and not just return the previous value like literally every other hardware
monitoring driver ?
If all this complexity is indeed required (I don't see it in the datasheet),
please provide a detailed explanation.
> + priv->wait_before_read = false;
> + }
> + } else {
> + ret = regmap_write(priv->regmap, MCP9982_ONE_SHOT_ADDR, 1);
> + if (ret)
> + return ret;
> +
> + /*
> + * In Standby state after writing in OneShot register wait for
> + * the start of conversion and then poll the BUSY bit.
> + */
> + sleep_time = mcp9982_update_interval[priv->interval_idx] * USEC_PER_MSEC;
> + usleep_range(MCP9982_WAKE_UP_TIME_US, MCP9982_WAKE_UP_TIME_MAX_US);
> +
> + ret = regmap_read_poll_timeout(priv->regmap, MCP9982_STATUS_ADDR,
> + reg_status, !(reg_status & MCP9982_STATUS_BUSY),
> + sleep_time, sleep_time * 2);
Same as above. The actual conversion time is constant. Why force the driver
to wait up to 16 seconds ?
> + if (ret)
> + return ret;
> + }
> +
> + switch (type) {
> + case hwmon_temp:
> + 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().
> + *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 ?
> +
> + return 0;
> + case hwmon_temp_min:
> + if (channel)
> + addr = MCP9982_EXT_LOW_LIMIT_ADDR(channel);
> + else
> + addr = MCP9982_INTERNAL_LOW_LIMIT_ADDR;
> +
> + return mcp9982_read_limit(priv, addr, val);
> + case hwmon_temp_min_alarm:
> + *val = regmap_test_bits(priv->regmap, MCP9982_LOW_LIMIT_STATUS_ADDR,
> + BIT(channel));
> + if (*val < 0)
> + return *val;
> +
> + return 0;
> + case hwmon_temp_crit:
> + return mcp9982_read_limit(priv, MCP9982_THERM_LIMIT_ADDR(channel), val);
> + case hwmon_temp_crit_alarm:
> + *val = regmap_test_bits(priv->regmap, MCP9982_THERM_LIMIT_STATUS_ADDR,
> + BIT(channel));
> + if (*val < 0)
> + return *val;
> +
> + return 0;
> + case hwmon_temp_crit_hyst:
> + ret = mcp9982_read_limit(priv, MCP9982_THERM_LIMIT_ADDR(channel), val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(priv->regmap, MCP9982_HYS_ADDR, &hyst);
> + if (ret)
> + return ret;
> +
> + *val -= (hyst & 0xFF) * 1000;
> + *val = clamp_val(*val, -64000, 191875);
> +
> + return 0;
> + default:
> + return -EINVAL;
> + }
> + case hwmon_chip:
> + switch (attr) {
> + case hwmon_chip_update_interval:
> + *val = mcp9982_update_interval[priv->interval_idx];
> + return 0;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mcp9982_read_label(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> + int channel, const char **str)
> +{
> + struct mcp9982_priv *priv = dev_get_drvdata(dev);
> +
> + switch (type) {
> + case hwmon_temp:
> + switch (attr) {
> + case hwmon_temp_label:
> + *str = priv->labels[channel];
> + return 0;
> + default:
> + return -EOPNOTSUPP;
> + }
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int mcp9982_write_limit(struct mcp9982_priv *priv, u8 address, long val)
> +{
> + int ret;
> +
> + /* Range is always -64 to 191.875°C. */
> + val = clamp_val(val, -64000, 191875);
> + val = (val + MCP9982_OFFSET * 1000) << 5;
> + val = DIV_ROUND_CLOSEST(val, 125);
> +
> + 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):
> + val = val >> 8;
> + val = val & 0xFF;
> + ret = regmap_write(priv->regmap, address, val);
> + if (ret)
> + return ret;
> +
> + 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):
> + val = cpu_to_be16(val);
> + ret = regmap_bulk_write(priv->regmap, address, &val, 2);
> + if (ret)
> + return ret;
> +
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mcp9982_write_hyst(struct mcp9982_priv *priv, int channel, long val)
> +{
> + int hyst, ret;
> + int limit;
> +
> + val = clamp_val(val, -64000, 191875);
> + val = (val + MCP9982_OFFSET * 1000) << 5;
> + val = DIV_ROUND_CLOSEST(val, 125);
> + val = val >> 8;
> +
> + /* Therm register is 8 bits and so it keeps only the integer part of the temperature. */
> + ret = regmap_read(priv->regmap, MCP9982_THERM_LIMIT_ADDR(channel), &limit);
> + if (ret)
> + return ret;
> +
> + hyst = clamp_val(limit - val, 0, 255);
> +
> + ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, hyst);
> +
> + return ret;
> +}
> +
> +static int mcp9982_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
> + long val)
> +{
> + struct mcp9982_priv *priv = dev_get_drvdata(dev);
> + unsigned int previous_interval_idx, idx;
> + bool use_previous_interval = false;
> + unsigned long new_time_limit;
> + u8 addr;
> + int ret;
> +
> + switch (type) {
> + case hwmon_chip:
> + switch (attr) {
> + case hwmon_chip_update_interval:
> + previous_interval_idx = priv->interval_idx;
> +
> + /*
> + * For MCP998XD and MCP9933D update interval
> + * can't be slower than 1 second.
> + */
> + if (priv->chip->hw_thermal_shutdown)
> + val = clamp(val, 0, 1000);
> +
> + idx = find_closest_descending(val, mcp9982_update_interval,
> + ARRAY_SIZE(mcp9982_update_interval));
> +
> + ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, idx);
> + if (ret)
> + return ret;
> +
> + priv->interval_idx = idx;
> +
> + /*
> + * When changing the interval time in Run mode, wait a delay based
> + * on the previous value to ensure the new value becomes active.
> + */
> + if (priv->run_state)
> + use_previous_interval = true;
> + else
> + return 0;
> + break;
> + default:
> + return -EINVAL;
> + }
> + break;
> + case hwmon_temp:
> + switch (attr) {
> + case hwmon_temp_max:
> + if (channel)
> + addr = MCP9982_EXT_HIGH_LIMIT_ADDR(channel);
> + else
> + addr = MCP9982_INTERNAL_HIGH_LIMIT_ADDR;
> +
> + return ret = mcp9982_write_limit(priv, addr, val);
> + case hwmon_temp_min:
> + if (channel)
> + addr = MCP9982_EXT_LOW_LIMIT_ADDR(channel);
> + else
> + addr = MCP9982_INTERNAL_LOW_LIMIT_ADDR;
> +
> + return mcp9982_write_limit(priv, addr, val);
> + case hwmon_temp_crit:
> + return mcp9982_write_limit(priv, MCP9982_THERM_LIMIT_ADDR(channel), val);
> + case hwmon_temp_crit_hyst:
> + return mcp9982_write_hyst(priv, channel, val);
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +
> + /*
> + * Calculate the time point when it would be safe to read after
> + * the current write operation in Run mode.
> + * If, for example, we change update interval from a slower time
> + * to a faster time, the change will become active after the
> + * conversion with the slower time is finished. If we read before
> + * the end of conversion, the value will be from the previous cycle.
> + */
> + if (use_previous_interval) {
> + new_time_limit = msecs_to_jiffies(mcp9982_update_interval[previous_interval_idx]);
> + use_previous_interval = false;
> + } else {
> + new_time_limit = msecs_to_jiffies(mcp9982_update_interval[priv->interval_idx]);
> + }
> +
> + new_time_limit += jiffies + msecs_to_jiffies(MCP9982_CONVERSION_TIME_MS);
> +
> + if (time_after(new_time_limit, priv->time_limit)) {
> + priv->time_limit = new_time_limit;
> + priv->wait_before_read = true;
> + }
> +
> + return 0;
> +}
> +
> +static umode_t mcp9982_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr,
> + int channel)
> +{
> + const struct mcp9982_priv *priv = _data;
> +
> + if (!test_bit(channel, &priv->enabled_channel_mask))
> + return 0;
> +
> + switch (type) {
> + case hwmon_temp:
> + switch (attr) {
> + case hwmon_temp_label:
> + if (priv->labels[channel])
> + return 0444;
> + else
> + return 0;
> + case hwmon_temp_input:
> + case hwmon_temp_min_alarm:
> + case hwmon_temp_max_alarm:
> + case hwmon_temp_max_hyst:
> + case hwmon_temp_crit_alarm:
> + return 0444;
> + case hwmon_temp_min:
> + case hwmon_temp_max:
> + case hwmon_temp_crit:
> + case hwmon_temp_crit_hyst:
> + return 0644;
> + default:
> + return 0;
> + }
> + case hwmon_chip:
> + switch (attr) {
> + case hwmon_chip_update_interval:
> + return 0644;
> + default:
> + return 0;
> + }
> + default:
> + return 0;
> + }
> +}
> +
> +static const struct hwmon_ops mcp9982_hwmon_ops = {
> + .is_visible = mcp9982_is_visible,
> + .read = mcp9982_read,
> + .read_string = mcp9982_read_label,
> + .write = mcp9982_write,
> +};
> +
> +static int mcp9982_init(struct device *dev, struct mcp9982_priv *priv)
> +{
> + unsigned int i;
> + int ret;
> + u8 val;
> +
> + /* Chips 82/83 and 82D/83D do not support anti-parallel diode mode. */
> + if (!priv->chip->allow_apdd && priv->apdd_enable == 1)
> + return dev_err_probe(dev, -EINVAL, "Incorrect setting of APDD.\n");
> +
> + /* Chips with "D" work only in Run state. */
> + if (priv->chip->hw_thermal_shutdown && !priv->run_state)
> + return dev_err_probe(dev, -EINVAL, "Incorrect setting of Power State.\n");
> +
> + /*
> + * For chips with "D" in the name, resistance error correction must be on
> + * so that hardware shutdown feature can't be overridden.
> + */
> + if (priv->chip->hw_thermal_shutdown)
> + if (!priv->recd34_enable || !priv->recd12_enable)
> + return dev_err_probe(dev, -EINVAL, "Incorrect setting of RECD.\n");
> +
> + /*
> + * Set default values in registers.
> + * APDD, RECD12 and RECD34 are active on 0.
> + */
> + val = FIELD_PREP(MCP9982_CFG_MSKAL, 1) |
> + FIELD_PREP(MCP9982_CFG_RS, !priv->run_state) |
> + FIELD_PREP(MCP9982_CFG_ATTHM, 1) |
> + FIELD_PREP(MCP9982_CFG_RECD12, !priv->recd12_enable) |
> + FIELD_PREP(MCP9982_CFG_RECD34, !priv->recd34_enable) |
> + FIELD_PREP(MCP9982_CFG_RANGE, 1) | FIELD_PREP(MCP9982_CFG_DA_ENA, 0) |
> + FIELD_PREP(MCP9982_CFG_APDD, !priv->apdd_enable);
> +
> + ret = regmap_write(priv->regmap, MCP9982_CFG_ADDR, val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, MCP9982_DEFAULT_CONV_VAL);
> + if (ret)
> + return ret;
> + priv->interval_idx = MCP9982_DEFAULT_CONV_VAL;
> +
> + ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, MCP9982_DEFAULT_HYS_VAL);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_CONSEC_ALRT_ADDR, MCP9982_DEFAULT_CONSEC_ALRT_VAL);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_ALRT_CFG_ADDR, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_HOTTEST_CFG_ADDR, 0);
> + if (ret)
> + return ret;
> +
> + /*
> + * Only external channels 1 and 2 support beta compensation.
> + * Set beta auto-detection.
> + */
> + for (i = 1; i < 3; i++)
> + if (test_bit(i, &priv->enabled_channel_mask)) {
> + ret = regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(i),
> + MCP9982_BETA_AUTODETECT);
> + if (ret)
> + return ret;
> + }
> +
> + /* Set default values for internal channel limits. */
> + if (test_bit(0, &priv->enabled_channel_mask)) {
> + ret = mcp9982_write_limit(priv, MCP9982_INTERNAL_HIGH_LIMIT_ADDR,
> + MCP9982_HIGH_LIMIT_DEFAULT);
> + if (ret)
> + return ret;
> +
> + ret = mcp9982_write_limit(priv, MCP9982_INTERNAL_LOW_LIMIT_ADDR,
> + MCP9982_LOW_LIMIT_DEFAULT);
> + if (ret)
> + return ret;
> +
> + ret = mcp9982_write_limit(priv, MCP9982_THERM_LIMIT_ADDR(0),
> + MCP9982_HIGH_LIMIT_DEFAULT);
> + if (ret)
> + return ret;
> + }
> +
> + /* Set ideality factor and limits to default for external channels. */
> + for (i = 1; i < MCP9982_MAX_NUM_CHANNELS; i++)
> + if (test_bit(i, &priv->enabled_channel_mask)) {
> + ret = regmap_write(priv->regmap, MCP9982_EXT_IDEAL_ADDR(i),
> + MCP9982_IDEALITY_DEFAULT);
> + if (ret)
> + return ret;
> +
> + ret = mcp9982_write_limit(priv, MCP9982_EXT_HIGH_LIMIT_ADDR(i),
> + MCP9982_HIGH_LIMIT_DEFAULT);
> + if (ret)
> + return ret;
> +
> + ret = mcp9982_write_limit(priv, MCP9982_EXT_LOW_LIMIT_ADDR(i),
> + MCP9982_LOW_LIMIT_DEFAULT);
> + if (ret)
> + return ret;
> +
> + ret = mcp9982_write_limit(priv, MCP9982_THERM_LIMIT_ADDR(i),
> + MCP9982_HIGH_LIMIT_DEFAULT);
> + if (ret)
> + return ret;
> + }
> +
> + priv->wait_before_read = false;
> + priv->time_limit = jiffies;
> +
> + return 0;
> +}
> +
> +static int mcp9982_parse_fw_config(struct device *dev, int device_nr_channels)
> +{
> + unsigned int reg_nr;
> + struct mcp9982_priv *priv = dev_get_drvdata(dev);
> + int ret;
> +
> + /* Initialise internal channel( which is always present ). */
> + priv->labels[0] = "internal diode";
> + priv->enabled_channel_mask = 1;
> +
> + /* Default values to work on systems without devicetree or firmware nodes. */
> + if (!dev_fwnode(dev)) {
> + priv->num_channels = device_nr_channels;
> + priv->enabled_channel_mask = BIT(priv->num_channels) - 1;
> + priv->apdd_enable = false;
> + priv->recd12_enable = true;
> + priv->recd34_enable = true;
> + priv->run_state = true;
> + return 0;
> + }
> +
> + priv->apdd_enable =
> + device_property_read_bool(dev, "microchip,enable-anti-parallel");
> +
> + priv->recd12_enable =
> + device_property_read_bool(dev, "microchip,parasitic-res-on-channel1-2");
> +
> + priv->recd34_enable =
> + device_property_read_bool(dev, "microchip,parasitic-res-on-channel3-4");
> +
> + priv->run_state =
> + device_property_read_bool(dev, "microchip,power-state");
> +
> + priv->num_channels = device_get_child_node_count(dev) + 1;
> +
> + if (priv->num_channels > device_nr_channels)
> + return dev_err_probe(dev, -EINVAL,
> + "More channels than the chip supports.\n");
> +
> + /* Read information about the external channels. */
> + device_for_each_child_node_scoped(dev, child) {
> + reg_nr = 0;
> + ret = fwnode_property_read_u32(child, "reg", ®_nr);
> + if (ret || !reg_nr || reg_nr >= device_nr_channels)
> + return dev_err_probe(dev, -EINVAL,
> + "Channel reg is incorrectly set.\n");
> +
> + fwnode_property_read_string(child, "label", &priv->labels[reg_nr]);
> + set_bit(reg_nr, &priv->enabled_channel_mask);
> + }
> +
> + return 0;
> +}
> +
> +static int mcp9982_probe(struct i2c_client *client)
> +{
> + struct hwmon_chip_info mcp998x_chip_info;
> + const struct mcp9982_features *chip;
> + struct device *dev = &client->dev;
> + struct mcp9982_priv *priv;
> + struct device *hwmon_dev;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(struct mcp9982_priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->regmap = devm_regmap_init_i2c(client, &mcp9982_regmap_config);
> +
> + if (IS_ERR(priv->regmap))
> + return dev_err_probe(dev, PTR_ERR(priv->regmap),
> + "Cannot initialize register map.\n");
> +
> + dev_set_drvdata(dev, priv);
> +
> + chip = i2c_get_match_data(client);
> + if (!chip)
> + return -EINVAL;
> + priv->chip = chip;
> +
> + ret = mcp9982_parse_fw_config(dev, chip->phys_channels);
> + if (ret)
> + return ret;
> +
> + ret = mcp9982_init(dev, priv);
> + if (ret)
> + return ret;
> +
> + mcp998x_chip_info.ops = &mcp9982_hwmon_ops;
> + mcp998x_chip_info.info = mcp9985_info;
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(dev, chip->name, priv,
> + &mcp998x_chip_info, NULL);
> +
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct i2c_device_id mcp9982_id[] = {
> + { .name = "mcp9933", .driver_data = (kernel_ulong_t)&mcp9933_chip_config },
> + { .name = "mcp9933d", .driver_data = (kernel_ulong_t)&mcp9933d_chip_config },
> + { .name = "mcp9982", .driver_data = (kernel_ulong_t)&mcp9982_chip_config },
> + { .name = "mcp9982d", .driver_data = (kernel_ulong_t)&mcp9982d_chip_config },
> + { .name = "mcp9983", .driver_data = (kernel_ulong_t)&mcp9983_chip_config },
> + { .name = "mcp9983d", .driver_data = (kernel_ulong_t)&mcp9983d_chip_config },
> + { .name = "mcp9984", .driver_data = (kernel_ulong_t)&mcp9984_chip_config },
> + { .name = "mcp9984d", .driver_data = (kernel_ulong_t)&mcp9984d_chip_config },
> + { .name = "mcp9985", .driver_data = (kernel_ulong_t)&mcp9985_chip_config },
> + { .name = "mcp9985d", .driver_data = (kernel_ulong_t)&mcp9985d_chip_config },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, mcp9982_id);
> +
> +static const struct of_device_id mcp9982_of_match[] = {
> + {
> + .compatible = "microchip,mcp9933",
> + .data = &mcp9933_chip_config,
> + }, {
> + .compatible = "microchip,mcp9933d",
> + .data = &mcp9933d_chip_config,
> + }, {
> + .compatible = "microchip,mcp9982",
> + .data = &mcp9982_chip_config,
> + }, {
> + .compatible = "microchip,mcp9982d",
> + .data = &mcp9982d_chip_config,
> + }, {
> + .compatible = "microchip,mcp9983",
> + .data = &mcp9983_chip_config,
> + }, {
> + .compatible = "microchip,mcp9983d",
> + .data = &mcp9983d_chip_config,
> + }, {
> + .compatible = "microchip,mcp9984",
> + .data = &mcp9984_chip_config,
> + }, {
> + .compatible = "microchip,mcp9984d",
> + .data = &mcp9984d_chip_config,
> + }, {
> + .compatible = "microchip,mcp9985",
> + .data = &mcp9985_chip_config,
> + }, {
> + .compatible = "microchip,mcp9985d",
> + .data = &mcp9985d_chip_config,
> + },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, mcp9982_of_match);
> +
> +static struct i2c_driver mcp9982_driver = {
> + .driver = {
> + .name = "mcp9982",
> + .of_match_table = mcp9982_of_match,
> + },
> + .probe = mcp9982_probe,
> + .id_table = mcp9982_id,
> +};
> +module_i2c_driver(mcp9982_driver);
> +
> +MODULE_AUTHOR("Victor Duicu <victor.duicu@microchip.com>");
> +MODULE_DESCRIPTION("MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] hwmon: add support for MCP998X
2026-01-27 15:18 ` [PATCH v3 2/2] " victor.duicu
2026-01-27 18:51 ` Guenter Roeck
@ 2026-01-27 22:23 ` kernel test robot
2026-02-03 19:45 ` Guenter Roeck
` (2 subsequent siblings)
4 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2026-01-27 22:23 UTC (permalink / raw)
To: victor.duicu, linux, robh, krzk+dt, conor+dt, corbet
Cc: oe-kbuild-all, marius.cristea, victor.duicu, linux-hwmon,
linux-kernel, devicetree, linux-doc
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 0f61b1860cc3f52aef9036d7235ed1f017632193]
url: https://github.com/intel-lab-lkp/linux/commits/victor-duicu-microchip-com/dt-bindings-hwmon-add-support-for-MCP998X/20260127-234206
base: 0f61b1860cc3f52aef9036d7235ed1f017632193
patch link: https://lore.kernel.org/r/20260127151823.9728-3-victor.duicu%40microchip.com
patch subject: [PATCH v3 2/2] hwmon: add support for MCP998X
config: hexagon-randconfig-r071-20260128 (https://download.01.org/0day-ci/archive/20260128/202601280635.9DHNZdk6-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 9b8addffa70cee5b2acc5454712d9cf78ce45710)
smatch version: v0.5.0-8994-gd50c5a4c
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260128/202601280635.9DHNZdk6-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202601280635.9DHNZdk6-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> Warning: drivers/hwmon/mcp9982.c:304 expecting prototype for struct mcp9992_priv. Prototype was for struct mcp9982_priv instead
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] hwmon: add support for MCP998X
2026-01-27 18:51 ` Guenter Roeck
@ 2026-02-02 8:15 ` Victor.Duicu
2026-02-02 15:18 ` Guenter Roeck
0 siblings, 1 reply; 18+ messages in thread
From: Victor.Duicu @ 2026-02-02 8:15 UTC (permalink / raw)
To: linux, robh, krzk+dt, conor+dt, corbet
Cc: Marius.Cristea, linux-hwmon, devicetree, linux-kernel, linux-doc
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, ®_high);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_read(priv->regmap, address + 1,
> > ®_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), ®_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.
> > + *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".
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] hwmon: add support for MCP998X
2026-02-02 8:15 ` Victor.Duicu
@ 2026-02-02 15:18 ` Guenter Roeck
2026-02-03 13:31 ` Victor.Duicu
0 siblings, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2026-02-02 15:18 UTC (permalink / raw)
To: Victor.Duicu, robh, krzk+dt, conor+dt, corbet
Cc: Marius.Cristea, linux-hwmon, devicetree, linux-kernel, linux-doc
On 2/2/26 00:15, Victor.Duicu@microchip.com wrote:
> 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, ®_high);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = regmap_read(priv->regmap, address + 1,
>>> ®_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.
>
This warrants a comment in the code. Others won't know and might send
a patch to "fix" the code.
> ...
>
>>>
>>> + 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.
>
As above.
>
>>> + *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".
>
"Supported" is irrelevant. Question is what is written into and reported by
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] hwmon: add support for MCP998X
2026-02-02 15:18 ` Guenter Roeck
@ 2026-02-03 13:31 ` Victor.Duicu
0 siblings, 0 replies; 18+ messages in thread
From: Victor.Duicu @ 2026-02-03 13:31 UTC (permalink / raw)
To: linux, robh, krzk+dt, conor+dt, corbet
Cc: Marius.Cristea, linux-hwmon, devicetree, linux-kernel, linux-doc
On Mon, 2026-02-02 at 07:18 -0800, Guenter Roeck wrote:
>
...
> > > > + 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".
> >
> "Supported" is irrelevant. Question is what is written into and
> reported by
> 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.
The driver reads the limit and hysteresis values and calculates
their difference. The result is only displayed and not saved in a
register. The result is clamped to be within the range of
supported values.
In my example setting hysteresis value to 19000 does not trigger
a different response from the chip.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: hwmon: add support for MCP998X
2026-01-27 15:18 ` [PATCH v3 1/2] dt-bindings: hwmon: add support " victor.duicu
@ 2026-02-03 19:15 ` Guenter Roeck
2026-02-06 14:17 ` Victor.Duicu
2026-02-06 16:47 ` Krzysztof Kozlowski
1 sibling, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2026-02-03 19:15 UTC (permalink / raw)
To: victor.duicu
Cc: robh, krzk+dt, conor+dt, corbet, marius.cristea, linux-hwmon,
linux-kernel, devicetree, linux-doc
On Tue, Jan 27, 2026 at 05:18:21PM +0200, victor.duicu@microchip.com wrote:
> From: Victor Duicu <victor.duicu@microchip.com>
>
> This is the devicetree schema for Microchip MCP998X/33 and
> MCP998XD/33D Multichannel Automotive Temperature Monitor Family.
>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
Some AI generated feedback inline, generated using Gemini 3 and Chris Mason's
prompts as base. I don't know much if anything about devicetree properties,
but it does seem to me that the AI has a point.
> ---
> .../bindings/hwmon/microchip,mcp9982.yaml | 205 ++++++++++++++++++
> MAINTAINERS | 6 +
> 2 files changed, 211 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml b/Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> new file mode 100644
> index 000000000000..05ea3c6a5618
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> @@ -0,0 +1,205 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/microchip,mcp9982.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip MCP998X/33 and MCP998XD/33D Temperature Monitor
> +
> +maintainers:
> + - Victor Duicu <victor.duicu@microchip.com>
> +
> +description: |
> + The MCP998X/33 and MCP998XD/33D family is a high-accuracy 2-wire
> + multichannel automotive temperature monitor.
> + The datasheet can be found here:
> + https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
> +
> +properties:
> + compatible:
> + enum:
> + - microchip,mcp9933
> + - microchip,mcp9933d
> + - microchip,mcp9982
> + - microchip,mcp9982d
> + - microchip,mcp9983
> + - microchip,mcp9983d
> + - microchip,mcp9984
> + - microchip,mcp9984d
> + - microchip,mcp9985
> + - microchip,mcp9985d
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + items:
> + - description: Signal coming from ALERT/THERM pin.
> + - description: Signal coming from THERM/ADDR pin.
> + - description: Signal coming from SYS_SHDN pin.
> +
> + interrupt-names:
> + items:
> + - const: alert-therm
> + - const: therm-addr
> + - const: sys-shutdown
The top-level definition of interrupt-names specifies exactly 3 items.
How does this interact with variants that only have 2 interrupts?
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> + microchip,enable-anti-parallel:
> + description:
> + Enable anti-parallel diode mode operation.
> + MCP9984/84D/85/85D and MCP9933/33D support reading two external diodes
> + in anti-parallel connection on the same set of pins.
> + type: boolean
> +
> + microchip,parasitic-res-on-channel1-2:
> + description:
> + Indicates that the chip and the diodes/transistors are sufficiently far
> + apart that a parasitic resistance is added to the wires, which can affect
> + the measurements. Due to the anti-parallel diode connections, channels
> + 1 and 2 are affected together.
> + type: boolean
> +
> + microchip,parasitic-res-on-channel3-4:
> + description:
> + Indicates that the chip and the diodes/transistors are sufficiently far
> + apart that a parasitic resistance is added to the wires, which can affect
> + the measurements. Due to the anti-parallel diode connections, channels
> + 3 and 4 are affected together.
> + type: boolean
> +
> + microchip,power-state:
> + description:
> + The chip can be set in Run state or Standby state. In Run state the ADC
> + is converting on all channels at the programmed conversion rate.
> + In Standby state the host must initiate a conversion cycle by writing
> + to the One-Shot register.
> + True value sets Run state.
> + Chips with "D" in the name can only be set in Run mode.
> + type: boolean
> +
> + vdd-supply: true
> +
> +patternProperties:
> + "^channel@[1-4]$":
> + description:
> + Represents the external temperature channels to which
> + a remote diode is connected.
> + type: object
> +
> + properties:
> + reg:
> + items:
> + minItems: 1
> + maxItems: 4
Should the reg property in a channel node allow up to 4 items? Since the
node naming ^channel@[1-4]$ implies a single channel per node, it seems
reg should have maxItems: 1.
Second feedback:
Is this reg schema correct? For a single cell index (e.g. reg = <1>), the
items are integers, and minItems/maxItems are not valid constraints on
integers. Did you mean to use minimum/maximum here, or just maxItems: 1
on the reg itself?
> +
> + label:
> + description: Unique name to identify which channel this is.
> +
> + required:
> + - reg
> +
> + additionalProperties: false
> +
> +required:
> + - compatible
> + - reg
> + - vdd-supply
> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - microchip,mcp9982d
> + - microchip,mcp9983d
> + - microchip,mcp9984d
> + - microchip,mcp9985d
> + - microchip,mcp9933d
> + then:
> + properties:
> + interrupts-names:
Is "interrupts-names" a typo for "interrupt-names"? The plural form
does not match the property defined at the top level.
Also, if the top-level interrupt-names requires 3 items, will a 2-item
override here conflict during validation?
> + items:
> + - const: alert-therm
> + - const: sys-shutdown
> + required:
> + - microchip,power-state
> + else:
> + properties:
> + interrupts-names:
Same question here regarding the typo; should this be interrupt-names?
> + items:
> + - const: alert-therm
> + - const: therm-addr
> + microchip,power-state: true
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + pattern: '^microchip,mcp998(2|3)$'
> + then:
> + properties:
> + microchip,enable-anti-parallel: false
If "D" variants only support Run mode as described in the property
description, why is this property required in the devicetree?
Second feedback:
Since the description says "D" versions can only be set in Run mode
(where True sets Run state), should this property also have a const: true
constraint here?
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + pattern: '^microchip,mcp998(2|3)d$'
> + then:
> + properties:
> + microchip,enable-anti-parallel: false
> + required:
> + - microchip,parasitic-res-on-channel1-2
> + - microchip,parasitic-res-on-channel3-4
Should the parasitic resistance compensation properties be required? These
represent board-specific parasitics and may not be present on all
designs using these chip variants.
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + pattern: '^microchip,mcp99(33|8[4-5])d$'
> + then:
> + required:
> + - microchip,parasitic-res-on-channel1-2
> + - microchip,parasitic-res-on-channel3-4
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + temperature-sensor@4c {
> + compatible = "microchip,mcp9985";
> + reg = <0x4c>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + microchip,enable-anti-parallel;
> + microchip,parasitic-res-on-channel1-2;
> + microchip,parasitic-res-on-channel3-4;
> +
> + vdd-supply = <&vdd>;
> +
> + channel@1 {
> + reg = <1>;
> + label = "Room Temperature";
> + };
> +
> + channel@2 {
> + reg = <2>;
> + label = "GPU Temperature";
> + };
> + };
> + };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0d044a58cbfe..f88d837cb586 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17149,6 +17149,12 @@ S: Maintained
> F: Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
> F: drivers/iio/adc/mcp3911.c
>
> +MICROCHIP MCP9982 TEMPERATURE DRIVER
> +M: Victor Duicu <victor.duicu@microchip.com>
> +L: linux-hwmon@vger.kernel.org
> +S: Supported
> +F: Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> +
> MICROCHIP MMC/SD/SDIO MCI DRIVER
> M: Aubin Constans <aubin.constans@microchip.com>
> S: Maintained
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] hwmon: add support for MCP998X
2026-01-27 15:18 ` [PATCH v3 2/2] " victor.duicu
2026-01-27 18:51 ` Guenter Roeck
2026-01-27 22:23 ` kernel test robot
@ 2026-02-03 19:45 ` Guenter Roeck
2026-02-03 20:09 ` Guenter Roeck
2026-02-03 20:45 ` Guenter Roeck
4 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2026-02-03 19:45 UTC (permalink / raw)
To: victor.duicu
Cc: robh, krzk+dt, conor+dt, corbet, marius.cristea, linux-hwmon,
linux-kernel, devicetree, linux-doc
On Tue, Jan 27, 2026 at 05:18:22PM +0200, victor.duicu@microchip.com wrote:
> From: Victor Duicu <victor.duicu@microchip.com>
>
> This is the driver for Microchip MCP998X/33 and MCP998XD/33D
> Multichannel Automotive Temperature Monitor Family.
>
> Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
I ran the patch through an AI code review. Findings (some already reported) inline,
plus some additional feedback from me.
> ---
> Documentation/hwmon/index.rst | 1 +
> Documentation/hwmon/mcp9982.rst | 105 ++++
> MAINTAINERS | 2 +
> drivers/hwmon/Kconfig | 11 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/mcp9982.c | 1022 +++++++++++++++++++++++++++++++
> 6 files changed, 1142 insertions(+)
> create mode 100644 Documentation/hwmon/mcp9982.rst
> create mode 100644 drivers/hwmon/mcp9982.c
>
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 85d7a686883e..b02709fc216e 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -173,6 +173,7 @@ Hardware Monitoring Kernel Drivers
> mc33xs2410_hwmon
> mc34vr500
> mcp3021
> + mcp9982
> menf21bmc
> mlxreg-fan
> mp2856
> diff --git a/Documentation/hwmon/mcp9982.rst b/Documentation/hwmon/mcp9982.rst
> new file mode 100644
> index 000000000000..dee2947f9044
> --- /dev/null
> +++ b/Documentation/hwmon/mcp9982.rst
> @@ -0,0 +1,105 @@
> +.. SPDX-License-Identifier: GPL-2.0+
> +Kernel driver MCP998X
> +=====================
> +
> +Supported chips:
> +
> + * Microchip Technology MCP998X/MCP9933 and MCP998XD/MCP9933D
> +
> + Prefix: 'mcp9982'
> +
> + Datasheet:
> + https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
> +
> +Authors:
> +
> + - Victor Duicu <victor.duicu@microchip.com>
> +
> +Description
> +-----------
> +
> +This driver implements support for the MCP998X family containing: MCP9982,
> +MCP9982D, MCP9983, MCP9983D, MCP9984, MCP9984D, MCP9985, MCP9985D,
> +MCP9933 and MCP9933D.
> +
> +The MCP998X Family is a high accuracy 2-wire multichannel automotive
> +temperature monitor.
> +
> +The chips in the family have different numbers of external channels,
> +ranging from 1 (MCP9982) to 4 channels (MCP9985). Reading diodes in
> +anti-parallel connection is supported by MCP9984/85/33 and
> +MCP9984D/85D/33D. Dedicated hardware shutdown circuitry is present
> +only in MCP998XD and MCP9933D.
> +
> +Temperatures are read in millidegrees Celsius, ranging from -64 to
> +191.875 with 0.125 precision.
> +
> +Each channel has a minimum, maximum, and critical limit alongside associated alarms.
> +The chips also implement a hysteresis mechanism which applies only to the maximum
> +and critical limits. The relative difference between a limit and its hysteresis
> +is the same for both and the value is kept in a single register.
> +
> +The chips measure temperatures with a variable conversion rate.
> +Update_interval = Conversion/Second, so the available options are:
> +- 16000 (ms) = 1 conv/16 sec
> +- 8000 (ms) = 1 conv/8 sec
> +- 4000 (ms) = 1 conv/4 sec
> +- 2000 (ms) = 1 conv/2 sec
> +- 1000 (ms) = 1 conv/sec
> +- 500 (ms) = 2 conv/sec
> +- 250 (ms) = 4 conv/sec
> +- 125 (ms) = 8 conv/sec
> +- 64 (ms) = 16 conv/sec
> +- 32 (ms) = 32 conv/sec
> +- 16 (ms) = 64 conv/sec
> +
> +Usage Notes
> +-----------
> +
> +Parameters that can be configured in devicetree:
> +- anti-parallel diode mode operation
> +- resistance error correction on channels 1 and 2
> +- resistance error correction on channels 3 and 4
> +Chips 82/83 and 82D/83D do not support anti-parallel diode mode.
> +For chips with "D" in the name resistance error correction must be on.
> +Please see Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> +for details.
> +
> +There are two power states:
> +- Active state: in which the chip is converting on all channels at the
> + programmed rate.
> +- Standby state: in which the host must initiate a conversion cycle.
> +Chips with "D" in the name work in Active state only and those without
> +can work in either state.
> +
> +Chips with "D" in the name can't set update interval slower than 1 second.
> +
> +Among the hysteresis attributes, only the tempX_crit_hyst ones are writeable
> +while the others are read only. Setting tempX_crit_hyst writes the difference
> +between tempX_crit and tempX_crit_hyst in the hysteresis register. The new value
> +applies automatically to the other limits. At power up the device starts with
> +the value 10 in the hysteresis register.
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported. The limits and update_interval are
> +read-write, while the rest are read only.
> +
> +======================= ==================================================
> +temp[1-5]_label User name for channel.
> +temp[1-5]_input Measured temperature for channel.
> +
> +temp[1-5]_crit Critical temperature limit.
> +temp[1-5]_crit_alarm Critical temperature limit alarm.
> +temp[1-5]_crit_hyst Critical temperature limit hysteresis.
> +
> +temp[1-5]_max High temperature limit.
> +temp[1-5]_max_alarm High temperature limit alarm.
> +temp[1-5]_max_hyst High temperature limit hysteresis.
> +
> +temp[1-5]_min Low temperature limit.
> +temp[1-5]_min_alarm Low temperature limit alarm.
> +
> +update_interval The interval at which the chip will update readings.
> +======================= ==================================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f88d837cb586..c39ab12b6bc8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17154,6 +17154,8 @@ M: Victor Duicu <victor.duicu@microchip.com>
> L: linux-hwmon@vger.kernel.org
> S: Supported
> F: Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> +F: Documentation/hwmon/mcp9982.rst
> +F: drivers/hwmon/mcp9982.c
>
> MICROCHIP MMC/SD/SDIO MCI DRIVER
> M: Aubin Constans <aubin.constans@microchip.com>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 157678b821fc..c758ab2d5fdf 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1388,6 +1388,17 @@ config SENSORS_MCP3021
> This driver can also be built as a module. If so, the module
> will be called mcp3021.
>
> +config SENSORS_MCP9982
> + tristate "Microchip Technology MCP9982 driver"
> + depends on I2C
> + select REGMAP_I2C
> + help
> + Say yes here to include support for Microchip Technology's MCP998X/33
> + and MCP998XD/33D Multichannel Automotive Temperature Monitor Family.
> +
> + This driver can also be built as a module. If so, the module
> + will be called mcp9982.
> +
> config SENSORS_MLXREG_FAN
> tristate "Mellanox FAN driver"
> depends on MELLANOX_PLATFORM
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index eade8e3b1bde..cec33da29a68 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -170,6 +170,7 @@ obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
> obj-$(CONFIG_SENSORS_MC33XS2410) += mc33xs2410_hwmon.o
> obj-$(CONFIG_SENSORS_MC34VR500) += mc34vr500.o
> obj-$(CONFIG_SENSORS_MCP3021) += mcp3021.o
> +obj-$(CONFIG_SENSORS_MCP9982) += mcp9982.o
> obj-$(CONFIG_SENSORS_TC654) += tc654.o
> obj-$(CONFIG_SENSORS_TPS23861) += tps23861.o
> obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o
> diff --git a/drivers/hwmon/mcp9982.c b/drivers/hwmon/mcp9982.c
> new file mode 100644
> index 000000000000..55701655daa0
> --- /dev/null
> +++ b/drivers/hwmon/mcp9982.c
> @@ -0,0 +1,1022 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * HWMON driver for MCP998X/33 and MCP998XD/33D Multichannel Automotive
> + * Temperature Monitor Family
> + *
> + * Copyright (C) 2026 Microchip Technology Inc. and its subsidiaries
> + *
> + * Author: Victor Duicu <victor.duicu@microchip.com>
> + *
> + * Datasheet can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/bits.h>
> +#include <linux/byteorder/generic.h>
> +#include <linux/delay.h>
> +#include <linux/device/devres.h>
> +#include <linux/device.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/i2c.h>
> +#include <linux/math.h>
> +#include <linux/minmax.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/time64.h>
> +#include <linux/util_macros.h>
> +
> +/* MCP9982 Registers */
> +#define MCP9982_HIGH_BYTE_ADDR(index) (2 * (index))
> +#define MCP9982_ONE_SHOT_ADDR 0x0A
> +#define MCP9982_INTERNAL_HIGH_LIMIT_ADDR 0x0B
> +#define MCP9982_INTERNAL_LOW_LIMIT_ADDR 0x0C
> +#define MCP9982_EXT_HIGH_LIMIT_ADDR(index) (4 * ((index) - 1) + 0x0D)
> +#define MCP9982_EXT_LOW_LIMIT_ADDR(index) (4 * ((index) - 1) + 0x0F)
> +#define MCP9982_THERM_LIMIT_ADDR(index) ((index) + 0x1D)
> +#define MCP9982_CFG_ADDR 0x22
> +#define MCP9982_CONV_ADDR 0x24
> +#define MCP9982_HYS_ADDR 0x25
> +#define MCP9982_CONSEC_ALRT_ADDR 0x26
> +#define MCP9982_ALRT_CFG_ADDR 0x27
> +#define MCP9982_RUNNING_AVG_ADDR 0x28
> +#define MCP9982_HOTTEST_CFG_ADDR 0x29
> +#define MCP9982_STATUS_ADDR 0x2A
> +#define MCP9982_EXT_FAULT_STATUS_ADDR 0x2B
> +#define MCP9982_HIGH_LIMIT_STATUS_ADDR 0x2C
> +#define MCP9982_LOW_LIMIT_STATUS_ADDR 0x2D
> +#define MCP9982_THERM_LIMIT_STATUS_ADDR 0x2E
> +#define MCP9982_HOTTEST_HIGH_BYTE_ADDR 0x2F
> +#define MCP9982_HOTTEST_LOW_BYTE_ADDR 0x30
> +#define MCP9982_HOTTEST_STATUS_ADDR 0x31
> +#define MCP9982_THERM_SHTDWN_CFG_ADDR 0x32
> +#define MCP9982_HRDW_THERM_SHTDWN_LIMIT_ADDR 0x33
> +#define MCP9982_EXT_BETA_CFG_ADDR(index) ((index) + 0x33)
> +#define MCP9982_EXT_IDEAL_ADDR(index) ((index) + 0x35)
> +
> +/* MCP9982 Bits */
> +#define MCP9982_CFG_MSKAL BIT(7)
> +#define MCP9982_CFG_RS BIT(6)
> +#define MCP9982_CFG_ATTHM BIT(5)
> +#define MCP9982_CFG_RECD12 BIT(4)
> +#define MCP9982_CFG_RECD34 BIT(3)
> +#define MCP9982_CFG_RANGE BIT(2)
> +#define MCP9982_CFG_DA_ENA BIT(1)
> +#define MCP9982_CFG_APDD BIT(0)
> +
> +#define MCP9982_STATUS_BUSY BIT(5)
> +
> +/* Constants and default values */
> +#define MCP9982_MAX_NUM_CHANNELS 5
> +#define MCP9982_BETA_AUTODETECT 16
> +#define MCP9982_IDEALITY_DEFAULT 18
> +#define MCP9982_OFFSET 64
> +#define MCP9982_DEFAULT_CONSEC_ALRT_VAL 112
> +#define MCP9982_DEFAULT_HYS_VAL 10
> +#define MCP9982_DEFAULT_CONV_VAL 6
> +#define MCP9982_WAKE_UP_TIME_US 125000
> +#define MCP9982_WAKE_UP_TIME_MAX_US 130000
> +#define MCP9982_TIMER_BUFFER_US 10000
> +#define MCP9982_CONVERSION_TIME_MS 125
> +#define MCP9982_HIGH_LIMIT_DEFAULT 21000
> +#define MCP9982_LOW_LIMIT_DEFAULT 0
> +
> +static const struct hwmon_channel_info * const mcp9985_info[] = {
> + HWMON_CHANNEL_INFO(temp,
> + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MIN |
> + HWMON_T_MIN_ALARM | HWMON_T_MAX | HWMON_T_MAX_ALARM |
> + HWMON_T_MAX_HYST | HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
> + HWMON_T_CRIT_HYST,
> + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MIN |
> + HWMON_T_MIN_ALARM | HWMON_T_MAX | HWMON_T_MAX_ALARM |
> + HWMON_T_MAX_HYST | HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
> + HWMON_T_CRIT_HYST,
> + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MIN |
> + HWMON_T_MIN_ALARM | HWMON_T_MAX | HWMON_T_MAX_ALARM |
> + HWMON_T_MAX_HYST | HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
> + HWMON_T_CRIT_HYST,
> + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MIN |
> + HWMON_T_MIN_ALARM | HWMON_T_MAX | HWMON_T_MAX_ALARM |
> + HWMON_T_MAX_HYST | HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
> + HWMON_T_CRIT_HYST,
> + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MIN |
> + HWMON_T_MIN_ALARM | HWMON_T_MAX | HWMON_T_MAX_ALARM |
> + HWMON_T_MAX_HYST | HWMON_T_CRIT | HWMON_T_CRIT_ALARM |
> + HWMON_T_CRIT_HYST),
> + HWMON_CHANNEL_INFO(chip,
> + HWMON_C_UPDATE_INTERVAL),
> + NULL
> +};
> +
> +/**
> + * struct mcp9982_features - features of a mcp9982 instance
> + * @name: chip's name
> + * @phys_channels: number of physical channels supported by the chip
> + * @hw_thermal_shutdown: presence of hardware thermal shutdown circuitry
> + * @allow_apdd: whether the chip supports enabling APDD
> + */
> +struct mcp9982_features {
> + const char *name;
> + u8 phys_channels;
> + bool hw_thermal_shutdown;
> + bool allow_apdd;
> +};
> +
> +static const struct mcp9982_features mcp9933_chip_config = {
> + .name = "mcp9933",
> + .phys_channels = 3,
> + .hw_thermal_shutdown = false,
> + .allow_apdd = true,
> +};
> +
> +static const struct mcp9982_features mcp9933d_chip_config = {
> + .name = "mcp9933d",
> + .phys_channels = 3,
> + .hw_thermal_shutdown = true,
> + .allow_apdd = true,
> +};
> +
> +static const struct mcp9982_features mcp9982_chip_config = {
> + .name = "mcp9982",
> + .phys_channels = 2,
> + .hw_thermal_shutdown = false,
> + .allow_apdd = false,
> +};
> +
> +static const struct mcp9982_features mcp9982d_chip_config = {
> + .name = "mcp9982d",
> + .phys_channels = 2,
> + .hw_thermal_shutdown = true,
> + .allow_apdd = false,
> +};
> +
> +static const struct mcp9982_features mcp9983_chip_config = {
> + .name = "mcp9983",
> + .phys_channels = 3,
> + .hw_thermal_shutdown = false,
> + .allow_apdd = false,
> +};
> +
> +static const struct mcp9982_features mcp9983d_chip_config = {
> + .name = "mcp9983d",
> + .phys_channels = 3,
> + .hw_thermal_shutdown = true,
> + .allow_apdd = false,
> +};
> +
> +static const struct mcp9982_features mcp9984_chip_config = {
> + .name = "mcp9984",
> + .phys_channels = 4,
> + .hw_thermal_shutdown = false,
> + .allow_apdd = true,
> +};
> +
> +static const struct mcp9982_features mcp9984d_chip_config = {
> + .name = "mcp9984d",
> + .phys_channels = 4,
> + .hw_thermal_shutdown = true,
> + .allow_apdd = true,
> +};
> +
> +static const struct mcp9982_features mcp9985_chip_config = {
> + .name = "mcp9985",
> + .phys_channels = 5,
> + .hw_thermal_shutdown = false,
> + .allow_apdd = true,
> +};
> +
> +static const struct mcp9982_features mcp9985d_chip_config = {
> + .name = "mcp9985d",
> + .phys_channels = 5,
> + .hw_thermal_shutdown = true,
> + .allow_apdd = true,
> +};
> +
> +static const unsigned int mcp9982_update_interval[11] = {
> + 16000, 8000, 4000, 2000, 1000, 500, 250, 125, 64, 32, 16
> +};
> +
> +/* MCP9982 regmap configuration */
> +static const struct regmap_range mcp9982_regmap_wr_ranges[] = {
> + regmap_reg_range(MCP9982_ONE_SHOT_ADDR, MCP9982_CFG_ADDR),
> + regmap_reg_range(MCP9982_CONV_ADDR, MCP9982_HOTTEST_CFG_ADDR),
> + regmap_reg_range(MCP9982_THERM_SHTDWN_CFG_ADDR, MCP9982_THERM_SHTDWN_CFG_ADDR),
> + regmap_reg_range(MCP9982_EXT_BETA_CFG_ADDR(1), MCP9982_EXT_IDEAL_ADDR(4)),
> +};
> +
> +static const struct regmap_access_table mcp9982_regmap_wr_table = {
> + .yes_ranges = mcp9982_regmap_wr_ranges,
> + .n_yes_ranges = ARRAY_SIZE(mcp9982_regmap_wr_ranges),
> +};
> +
> +static const struct regmap_range mcp9982_regmap_rd_ranges[] = {
> + regmap_reg_range(MCP9982_HIGH_BYTE_ADDR(0), MCP9982_CFG_ADDR),
> + regmap_reg_range(MCP9982_CONV_ADDR, MCP9982_EXT_IDEAL_ADDR(4)),
> +};
> +
> +static const struct regmap_access_table mcp9982_regmap_rd_table = {
> + .yes_ranges = mcp9982_regmap_rd_ranges,
> + .n_yes_ranges = ARRAY_SIZE(mcp9982_regmap_rd_ranges),
> +};
> +
> +static bool mcp9982_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case MCP9982_ONE_SHOT_ADDR:
> + case MCP9982_INTERNAL_HIGH_LIMIT_ADDR:
> + case MCP9982_INTERNAL_LOW_LIMIT_ADDR:
> + case MCP9982_EXT_LOW_LIMIT_ADDR(1):
> + case MCP9982_EXT_LOW_LIMIT_ADDR(1) + 1:
> + case MCP9982_EXT_LOW_LIMIT_ADDR(2):
> + case MCP9982_EXT_LOW_LIMIT_ADDR(2) + 1:
> + case MCP9982_EXT_LOW_LIMIT_ADDR(3):
> + case MCP9982_EXT_LOW_LIMIT_ADDR(3) + 1:
> + case MCP9982_EXT_LOW_LIMIT_ADDR(4):
> + case MCP9982_EXT_LOW_LIMIT_ADDR(4) + 1:
> + case MCP9982_EXT_HIGH_LIMIT_ADDR(1):
> + case MCP9982_EXT_HIGH_LIMIT_ADDR(1) + 1:
> + case MCP9982_EXT_HIGH_LIMIT_ADDR(2):
> + case MCP9982_EXT_HIGH_LIMIT_ADDR(2) + 1:
> + case MCP9982_EXT_HIGH_LIMIT_ADDR(3):
> + case MCP9982_EXT_HIGH_LIMIT_ADDR(3) + 1:
> + case MCP9982_EXT_HIGH_LIMIT_ADDR(4):
> + case MCP9982_EXT_HIGH_LIMIT_ADDR(4) + 1:
> + 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):
> + case MCP9982_CFG_ADDR:
> + case MCP9982_CONV_ADDR:
> + case MCP9982_HYS_ADDR:
> + case MCP9982_CONSEC_ALRT_ADDR:
> + case MCP9982_ALRT_CFG_ADDR:
> + case MCP9982_RUNNING_AVG_ADDR:
> + case MCP9982_HOTTEST_CFG_ADDR:
> + case MCP9982_THERM_SHTDWN_CFG_ADDR:
> + return false;
> + default:
> + return true;
> + }
> +}
> +
> +static const struct regmap_config mcp9982_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .rd_table = &mcp9982_regmap_rd_table,
> + .wr_table = &mcp9982_regmap_wr_table,
> + .volatile_reg = mcp9982_is_volatile_reg,
> + .max_register = MCP9982_EXT_IDEAL_ADDR(4),
> + .cache_type = REGCACHE_MAPLE,
> +};
> +
> +/**
> + * struct mcp9992_priv - information about chip parameters
> + * @regmap: device register map
> + * @chip: pointer to structure holding chip features
> + * @labels: labels of the channels
> + * @interval_idx: index representing the current update interval
> + * @time_limit: time when it is safe to read
> + * @enabled_channel_mask: mask containing which channels should be enabled
> + * @num_channels: number of active physical channels
> + * @recd34_enable: state of Resistance Error Correction(REC) on channels 3 and 4
> + * @recd12_enable: state of Resistance Error Correction(REC) on channels 1 and 2
> + * @apdd_enable: state of anti-parallel diode mode
> + * @run_state: chip is in Run state, otherwise is in Standby state
> + * @wait_before_read: whether we need to wait a delay before reading a new value
> + */
> +struct mcp9982_priv {
> + struct regmap *regmap;
> + const struct mcp9982_features *chip;
> + const char *labels[MCP9982_MAX_NUM_CHANNELS];
> + unsigned int interval_idx;
> + unsigned long time_limit;
> + unsigned long enabled_channel_mask;
> + u8 num_channels;
> + bool recd34_enable;
> + bool recd12_enable;
> + bool apdd_enable;
> + bool run_state;
> + bool wait_before_read;
> +};
> +
> +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".
> + */
The above comment is confusing and doesn't seem to provide a useful purpose,
or at least none that I can see.
> + ret = regmap_read(priv->regmap, address, ®_high);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(priv->regmap, address + 1, ®_low);
> + if (ret)
> + return ret;
> +
> + reg_high = reg_high & 0xFF;
> + reg_low = reg_low & 0xFF;
> + *val = ((reg_high << 8) + reg_low) >> 5;
> + *val = (*val - (MCP9982_OFFSET << 3)) * 125;
> +
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mcp9982_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
> + long *val)
> +{
> + struct mcp9982_priv *priv = dev_get_drvdata(dev);
> + unsigned int reg_status, reg_high, reg_low;
> + unsigned long sleep_time;
> + int ret, hyst;
> + u8 addr;
> +
> + /*
> + * When working in Run mode, after modifying a parameter (like update
> + * interval) we have to wait a delay before reading the new values.
> + * We can't determine when the conversion is done based on the BUSY bit.
> + */
> + if (priv->run_state) {
> + if (priv->wait_before_read) {
> + sleep_time = jiffies_to_msecs(priv->time_limit - jiffies) * USEC_PER_MSEC;
> + if (!time_after(jiffies, priv->time_limit))
> + usleep_range(sleep_time, sleep_time + MCP9982_TIMER_BUFFER_US);
> +
AI: Could this result in excessive delays when the update interval is set to its
maximum value (16 seconds)? It appears this would block sysfs reads for a
significant amount of time.
> + priv->wait_before_read = false;
> + }
> + } else {
> + ret = regmap_write(priv->regmap, MCP9982_ONE_SHOT_ADDR, 1);
> + if (ret)
> + return ret;
> +
> + /*
> + * In Standby state after writing in OneShot register wait for
> + * the start of conversion and then poll the BUSY bit.
> + */
> + sleep_time = mcp9982_update_interval[priv->interval_idx] * USEC_PER_MSEC;
> + usleep_range(MCP9982_WAKE_UP_TIME_US, MCP9982_WAKE_UP_TIME_MAX_US);
> +
> + ret = regmap_read_poll_timeout(priv->regmap, MCP9982_STATUS_ADDR,
> + reg_status, !(reg_status & MCP9982_STATUS_BUSY),
> + sleep_time, sleep_time * 2);
AI: Similarly, using the update interval as the sleep time for polling a one-shot
conversion seems like it could lead to 16-second blocks if mcp9982_read() is
called in Standby mode with a slow update interval configured.
> + if (ret)
> + return ret;
> + }
> +
> + switch (type) {
> + case hwmon_temp:
> + 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;
> +
> + *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;
> + *val = clamp_val(*val, -64000, 191875);
> +
> + return 0;
> + case hwmon_temp_min:
> + if (channel)
> + addr = MCP9982_EXT_LOW_LIMIT_ADDR(channel);
> + else
> + addr = MCP9982_INTERNAL_LOW_LIMIT_ADDR;
> +
> + return mcp9982_read_limit(priv, addr, val);
> + case hwmon_temp_min_alarm:
> + *val = regmap_test_bits(priv->regmap, MCP9982_LOW_LIMIT_STATUS_ADDR,
> + BIT(channel));
> + if (*val < 0)
> + return *val;
> +
> + return 0;
> + case hwmon_temp_crit:
> + return mcp9982_read_limit(priv, MCP9982_THERM_LIMIT_ADDR(channel), val);
> + case hwmon_temp_crit_alarm:
> + *val = regmap_test_bits(priv->regmap, MCP9982_THERM_LIMIT_STATUS_ADDR,
> + BIT(channel));
> + if (*val < 0)
> + return *val;
> +
> + return 0;
> + case hwmon_temp_crit_hyst:
> + ret = mcp9982_read_limit(priv, MCP9982_THERM_LIMIT_ADDR(channel), val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(priv->regmap, MCP9982_HYS_ADDR, &hyst);
> + if (ret)
> + return ret;
> +
> + *val -= (hyst & 0xFF) * 1000;
> + *val = clamp_val(*val, -64000, 191875);
> +
> + return 0;
> + default:
> + return -EINVAL;
> + }
> + case hwmon_chip:
> + switch (attr) {
> + case hwmon_chip_update_interval:
> + *val = mcp9982_update_interval[priv->interval_idx];
> + return 0;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mcp9982_read_label(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> + int channel, const char **str)
> +{
> + struct mcp9982_priv *priv = dev_get_drvdata(dev);
> +
> + switch (type) {
> + case hwmon_temp:
> + switch (attr) {
> + case hwmon_temp_label:
> + *str = priv->labels[channel];
> + return 0;
> + default:
> + return -EOPNOTSUPP;
> + }
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int mcp9982_write_limit(struct mcp9982_priv *priv, u8 address, long val)
> +{
> + int ret;
> +
> + /* Range is always -64 to 191.875°C. */
> + val = clamp_val(val, -64000, 191875);
> + val = (val + MCP9982_OFFSET * 1000) << 5;
> + val = DIV_ROUND_CLOSEST(val, 125);
> +
> + 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):
> + val = val >> 8;
> + val = val & 0xFF;
> + ret = regmap_write(priv->regmap, address, val);
> + if (ret)
> + return ret;
> +
> + 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):
> + val = cpu_to_be16(val);
> + ret = regmap_bulk_write(priv->regmap, address, &val, 2);
AI:
Does this code work correctly on Big Endian systems? Passing the address of
the 'long val' to regmap_bulk_write() after storing a be16 value in it appears
problematic, as regmap_bulk_write() will read from the beginning of the
variable.
Second feedback:
This is incorrect for several reasons:
- 'val' is a long (8 bytes on many systems), but the code passes its
address to regmap_bulk_write() for a 2-byte write.
- cpu_to_be16(val) returns a u16. Assigning this back to the 'long val'
and then taking its address results in different data being sent
depending on the host's endianness. On a Little Endian system, the
u16 will be stored in the first two bytes of the long, and
regmap_bulk_write() will send them in Little Endian order to the bus,
effectively nullifying the cpu_to_be16() conversion.
A local u16 or __be16 variable should be used instead:
__be16 reg_val = cpu_to_be16(val);
ret = regmap_bulk_write(priv->regmap, address | 0x80, ®_val, 2);
Third feedback:
The address variable does not have the MSB set (e.g., for
MCP9982_EXT_HIGH_LIMIT_ADDR(1), it is 0x0D). Based on the datasheet
description provided in the comment, this should likely be
(address | 0x80).
> + if (ret)
> + return ret;
> +
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mcp9982_write_hyst(struct mcp9982_priv *priv, int channel, long val)
> +{
> + int hyst, ret;
> + int limit;
> +
> + val = clamp_val(val, -64000, 191875);
> + val = (val + MCP9982_OFFSET * 1000) << 5;
> + val = DIV_ROUND_CLOSEST(val, 125);
> + val = val >> 8;
> +
> + /* Therm register is 8 bits and so it keeps only the integer part of the temperature. */
> + ret = regmap_read(priv->regmap, MCP9982_THERM_LIMIT_ADDR(channel), &limit);
> + if (ret)
> + return ret;
> +
> + hyst = clamp_val(limit - val, 0, 255);
> +
> + ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, hyst);
> +
> + return ret;
> +}
> +
> +static int mcp9982_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
> + long val)
> +{
> + struct mcp9982_priv *priv = dev_get_drvdata(dev);
> + unsigned int previous_interval_idx, idx;
> + bool use_previous_interval = false;
> + unsigned long new_time_limit;
> + u8 addr;
> + int ret;
> +
> + switch (type) {
> + case hwmon_chip:
> + switch (attr) {
> + case hwmon_chip_update_interval:
> + previous_interval_idx = priv->interval_idx;
> +
> + /*
> + * For MCP998XD and MCP9933D update interval
> + * can't be slower than 1 second.
> + */
> + if (priv->chip->hw_thermal_shutdown)
> + val = clamp(val, 0, 1000);
> +
> + idx = find_closest_descending(val, mcp9982_update_interval,
> + ARRAY_SIZE(mcp9982_update_interval));
> +
> + ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, idx);
> + if (ret)
> + return ret;
> +
> + priv->interval_idx = idx;
> +
> + /*
> + * When changing the interval time in Run mode, wait a delay based
> + * on the previous value to ensure the new value becomes active.
> + */
> + if (priv->run_state)
> + use_previous_interval = true;
> + else
> + return 0;
> + break;
> + default:
> + return -EINVAL;
> + }
> + break;
> + case hwmon_temp:
> + switch (attr) {
> + case hwmon_temp_max:
> + if (channel)
> + addr = MCP9982_EXT_HIGH_LIMIT_ADDR(channel);
> + else
> + addr = MCP9982_INTERNAL_HIGH_LIMIT_ADDR;
> +
> + return ret = mcp9982_write_limit(priv, addr, val);
> + case hwmon_temp_min:
> + if (channel)
> + addr = MCP9982_EXT_LOW_LIMIT_ADDR(channel);
> + else
> + addr = MCP9982_INTERNAL_LOW_LIMIT_ADDR;
> +
> + return mcp9982_write_limit(priv, addr, val);
> + case hwmon_temp_crit:
> + return mcp9982_write_limit(priv, MCP9982_THERM_LIMIT_ADDR(channel), val);
> + case hwmon_temp_crit_hyst:
> + return mcp9982_write_hyst(priv, channel, val);
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +
> + /*
> + * Calculate the time point when it would be safe to read after
> + * the current write operation in Run mode.
> + * If, for example, we change update interval from a slower time
> + * to a faster time, the change will become active after the
> + * conversion with the slower time is finished. If we read before
> + * the end of conversion, the value will be from the previous cycle.
> + */
> + if (use_previous_interval) {
> + new_time_limit = msecs_to_jiffies(mcp9982_update_interval[previous_interval_idx]);
> + use_previous_interval = false;
> + } else {
> + new_time_limit = msecs_to_jiffies(mcp9982_update_interval[priv->interval_idx]);
> + }
> +
> + new_time_limit += jiffies + msecs_to_jiffies(MCP9982_CONVERSION_TIME_MS);
> +
> + if (time_after(new_time_limit, priv->time_limit)) {
> + priv->time_limit = new_time_limit;
> + priv->wait_before_read = true;
> + }
> +
> + return 0;
> +}
> +
> +static umode_t mcp9982_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr,
> + int channel)
> +{
> + const struct mcp9982_priv *priv = _data;
> +
> + if (!test_bit(channel, &priv->enabled_channel_mask))
> + return 0;
> +
> + switch (type) {
> + case hwmon_temp:
> + switch (attr) {
> + case hwmon_temp_label:
> + if (priv->labels[channel])
> + return 0444;
> + else
> + return 0;
> + case hwmon_temp_input:
> + case hwmon_temp_min_alarm:
> + case hwmon_temp_max_alarm:
> + case hwmon_temp_max_hyst:
> + case hwmon_temp_crit_alarm:
> + return 0444;
> + case hwmon_temp_min:
> + case hwmon_temp_max:
> + case hwmon_temp_crit:
> + case hwmon_temp_crit_hyst:
> + return 0644;
> + default:
> + return 0;
> + }
> + case hwmon_chip:
> + switch (attr) {
> + case hwmon_chip_update_interval:
> + return 0644;
> + default:
> + return 0;
> + }
> + default:
> + return 0;
> + }
> +}
> +
> +static const struct hwmon_ops mcp9982_hwmon_ops = {
> + .is_visible = mcp9982_is_visible,
> + .read = mcp9982_read,
> + .read_string = mcp9982_read_label,
> + .write = mcp9982_write,
> +};
> +
> +static int mcp9982_init(struct device *dev, struct mcp9982_priv *priv)
> +{
> + unsigned int i;
> + int ret;
> + u8 val;
> +
> + /* Chips 82/83 and 82D/83D do not support anti-parallel diode mode. */
> + if (!priv->chip->allow_apdd && priv->apdd_enable == 1)
> + return dev_err_probe(dev, -EINVAL, "Incorrect setting of APDD.\n");
> +
> + /* Chips with "D" work only in Run state. */
> + if (priv->chip->hw_thermal_shutdown && !priv->run_state)
> + return dev_err_probe(dev, -EINVAL, "Incorrect setting of Power State.\n");
> +
> + /*
> + * For chips with "D" in the name, resistance error correction must be on
> + * so that hardware shutdown feature can't be overridden.
> + */
> + if (priv->chip->hw_thermal_shutdown)
> + if (!priv->recd34_enable || !priv->recd12_enable)
> + return dev_err_probe(dev, -EINVAL, "Incorrect setting of RECD.\n");
> +
> + /*
> + * Set default values in registers.
> + * APDD, RECD12 and RECD34 are active on 0.
> + */
> + val = FIELD_PREP(MCP9982_CFG_MSKAL, 1) |
> + FIELD_PREP(MCP9982_CFG_RS, !priv->run_state) |
> + FIELD_PREP(MCP9982_CFG_ATTHM, 1) |
> + FIELD_PREP(MCP9982_CFG_RECD12, !priv->recd12_enable) |
> + FIELD_PREP(MCP9982_CFG_RECD34, !priv->recd34_enable) |
> + FIELD_PREP(MCP9982_CFG_RANGE, 1) | FIELD_PREP(MCP9982_CFG_DA_ENA, 0) |
> + FIELD_PREP(MCP9982_CFG_APDD, !priv->apdd_enable);
> +
> + ret = regmap_write(priv->regmap, MCP9982_CFG_ADDR, val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, MCP9982_DEFAULT_CONV_VAL);
> + if (ret)
> + return ret;
> + priv->interval_idx = MCP9982_DEFAULT_CONV_VAL;
> +
> + ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, MCP9982_DEFAULT_HYS_VAL);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_CONSEC_ALRT_ADDR, MCP9982_DEFAULT_CONSEC_ALRT_VAL);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_ALRT_CFG_ADDR, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_HOTTEST_CFG_ADDR, 0);
> + if (ret)
> + return ret;
> +
> + /*
> + * Only external channels 1 and 2 support beta compensation.
> + * Set beta auto-detection.
> + */
> + for (i = 1; i < 3; i++)
> + if (test_bit(i, &priv->enabled_channel_mask)) {
> + ret = regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(i),
> + MCP9982_BETA_AUTODETECT);
> + if (ret)
> + return ret;
> + }
> +
> + /* Set default values for internal channel limits. */
> + if (test_bit(0, &priv->enabled_channel_mask)) {
> + ret = mcp9982_write_limit(priv, MCP9982_INTERNAL_HIGH_LIMIT_ADDR,
> + MCP9982_HIGH_LIMIT_DEFAULT);
> + if (ret)
> + return ret;
> +
> + ret = mcp9982_write_limit(priv, MCP9982_INTERNAL_LOW_LIMIT_ADDR,
> + MCP9982_LOW_LIMIT_DEFAULT);
> + if (ret)
> + return ret;
> +
> + ret = mcp9982_write_limit(priv, MCP9982_THERM_LIMIT_ADDR(0),
> + MCP9982_HIGH_LIMIT_DEFAULT);
> + if (ret)
> + return ret;
> + }
> +
> + /* Set ideality factor and limits to default for external channels. */
> + for (i = 1; i < MCP9982_MAX_NUM_CHANNELS; i++)
> + if (test_bit(i, &priv->enabled_channel_mask)) {
> + ret = regmap_write(priv->regmap, MCP9982_EXT_IDEAL_ADDR(i),
> + MCP9982_IDEALITY_DEFAULT);
> + if (ret)
> + return ret;
> +
> + ret = mcp9982_write_limit(priv, MCP9982_EXT_HIGH_LIMIT_ADDR(i),
> + MCP9982_HIGH_LIMIT_DEFAULT);
> + if (ret)
> + return ret;
> +
> + ret = mcp9982_write_limit(priv, MCP9982_EXT_LOW_LIMIT_ADDR(i),
> + MCP9982_LOW_LIMIT_DEFAULT);
> + if (ret)
> + return ret;
> +
> + ret = mcp9982_write_limit(priv, MCP9982_THERM_LIMIT_ADDR(i),
> + MCP9982_HIGH_LIMIT_DEFAULT);
> + if (ret)
> + return ret;
> + }
> +
> + priv->wait_before_read = false;
> + priv->time_limit = jiffies;
> +
> + return 0;
> +}
> +
> +static int mcp9982_parse_fw_config(struct device *dev, int device_nr_channels)
> +{
> + unsigned int reg_nr;
> + struct mcp9982_priv *priv = dev_get_drvdata(dev);
> + int ret;
> +
> + /* Initialise internal channel( which is always present ). */
> + priv->labels[0] = "internal diode";
> + priv->enabled_channel_mask = 1;
> +
> + /* Default values to work on systems without devicetree or firmware nodes. */
> + if (!dev_fwnode(dev)) {
> + priv->num_channels = device_nr_channels;
> + priv->enabled_channel_mask = BIT(priv->num_channels) - 1;
> + priv->apdd_enable = false;
> + priv->recd12_enable = true;
> + priv->recd34_enable = true;
> + priv->run_state = true;
> + return 0;
> + }
> +
> + priv->apdd_enable =
> + device_property_read_bool(dev, "microchip,enable-anti-parallel");
> +
> + priv->recd12_enable =
> + device_property_read_bool(dev, "microchip,parasitic-res-on-channel1-2");
> +
> + priv->recd34_enable =
> + device_property_read_bool(dev, "microchip,parasitic-res-on-channel3-4");
> +
> + priv->run_state =
> + device_property_read_bool(dev, "microchip,power-state");
> +
> + priv->num_channels = device_get_child_node_count(dev) + 1;
> +
> + if (priv->num_channels > device_nr_channels)
> + return dev_err_probe(dev, -EINVAL,
> + "More channels than the chip supports.\n");
> +
> + /* Read information about the external channels. */
> + device_for_each_child_node_scoped(dev, child) {
> + reg_nr = 0;
> + ret = fwnode_property_read_u32(child, "reg", ®_nr);
> + if (ret || !reg_nr || reg_nr >= device_nr_channels)
> + return dev_err_probe(dev, -EINVAL,
> + "Channel reg is incorrectly set.\n");
> +
> + fwnode_property_read_string(child, "label", &priv->labels[reg_nr]);
> + set_bit(reg_nr, &priv->enabled_channel_mask);
> + }
> +
> + return 0;
> +}
> +
> +static int mcp9982_probe(struct i2c_client *client)
> +{
> + struct hwmon_chip_info mcp998x_chip_info;
> + const struct mcp9982_features *chip;
> + struct device *dev = &client->dev;
> + struct mcp9982_priv *priv;
> + struct device *hwmon_dev;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(struct mcp9982_priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->regmap = devm_regmap_init_i2c(client, &mcp9982_regmap_config);
> +
> + if (IS_ERR(priv->regmap))
> + return dev_err_probe(dev, PTR_ERR(priv->regmap),
> + "Cannot initialize register map.\n");
> +
> + dev_set_drvdata(dev, priv);
> +
> + chip = i2c_get_match_data(client);
> + if (!chip)
> + return -EINVAL;
> + priv->chip = chip;
> +
> + ret = mcp9982_parse_fw_config(dev, chip->phys_channels);
> + if (ret)
> + return ret;
> +
> + ret = mcp9982_init(dev, priv);
> + if (ret)
> + return ret;
> +
> + mcp998x_chip_info.ops = &mcp9982_hwmon_ops;
> + mcp998x_chip_info.info = mcp9985_info;
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(dev, chip->name, priv,
> + &mcp998x_chip_info, NULL);
AI: Is it safe to pass a pointer to the stack-allocated mcp998x_chip_info to
devm_hwmon_device_register_with_info()? The hwmon core appears to store this
pointer and uses it in hwmon_thermal_get_temp(), which could lead to a crash
after probe() returns and the stack frame is reclaimed.
Me: Outch. Yes.
> +
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct i2c_device_id mcp9982_id[] = {
> + { .name = "mcp9933", .driver_data = (kernel_ulong_t)&mcp9933_chip_config },
> + { .name = "mcp9933d", .driver_data = (kernel_ulong_t)&mcp9933d_chip_config },
> + { .name = "mcp9982", .driver_data = (kernel_ulong_t)&mcp9982_chip_config },
> + { .name = "mcp9982d", .driver_data = (kernel_ulong_t)&mcp9982d_chip_config },
> + { .name = "mcp9983", .driver_data = (kernel_ulong_t)&mcp9983_chip_config },
> + { .name = "mcp9983d", .driver_data = (kernel_ulong_t)&mcp9983d_chip_config },
> + { .name = "mcp9984", .driver_data = (kernel_ulong_t)&mcp9984_chip_config },
> + { .name = "mcp9984d", .driver_data = (kernel_ulong_t)&mcp9984d_chip_config },
> + { .name = "mcp9985", .driver_data = (kernel_ulong_t)&mcp9985_chip_config },
> + { .name = "mcp9985d", .driver_data = (kernel_ulong_t)&mcp9985d_chip_config },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, mcp9982_id);
> +
> +static const struct of_device_id mcp9982_of_match[] = {
> + {
> + .compatible = "microchip,mcp9933",
> + .data = &mcp9933_chip_config,
> + }, {
> + .compatible = "microchip,mcp9933d",
> + .data = &mcp9933d_chip_config,
> + }, {
> + .compatible = "microchip,mcp9982",
> + .data = &mcp9982_chip_config,
> + }, {
> + .compatible = "microchip,mcp9982d",
> + .data = &mcp9982d_chip_config,
> + }, {
> + .compatible = "microchip,mcp9983",
> + .data = &mcp9983_chip_config,
> + }, {
> + .compatible = "microchip,mcp9983d",
> + .data = &mcp9983d_chip_config,
> + }, {
> + .compatible = "microchip,mcp9984",
> + .data = &mcp9984_chip_config,
> + }, {
> + .compatible = "microchip,mcp9984d",
> + .data = &mcp9984d_chip_config,
> + }, {
> + .compatible = "microchip,mcp9985",
> + .data = &mcp9985_chip_config,
> + }, {
> + .compatible = "microchip,mcp9985d",
> + .data = &mcp9985d_chip_config,
> + },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, mcp9982_of_match);
> +
> +static struct i2c_driver mcp9982_driver = {
> + .driver = {
> + .name = "mcp9982",
> + .of_match_table = mcp9982_of_match,
> + },
> + .probe = mcp9982_probe,
> + .id_table = mcp9982_id,
> +};
> +module_i2c_driver(mcp9982_driver);
> +
> +MODULE_AUTHOR("Victor Duicu <victor.duicu@microchip.com>");
> +MODULE_DESCRIPTION("MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] hwmon: add support for MCP998X
2026-01-27 15:18 ` [PATCH v3 2/2] " victor.duicu
` (2 preceding siblings ...)
2026-02-03 19:45 ` Guenter Roeck
@ 2026-02-03 20:09 ` Guenter Roeck
2026-02-03 20:45 ` Guenter Roeck
4 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2026-02-03 20:09 UTC (permalink / raw)
To: victor.duicu
Cc: robh, krzk+dt, conor+dt, corbet, marius.cristea, linux-hwmon,
linux-kernel, devicetree, linux-doc
On Tue, Jan 27, 2026 at 05:18:22PM +0200, victor.duicu@microchip.com wrote:
> From: Victor Duicu <victor.duicu@microchip.com>
>
> This is the driver for Microchip MCP998X/33 and MCP998XD/33D
> Multichannel Automotive Temperature Monitor Family.
>
> Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
> ---
More feedback, with compliments from Gemini :-).
[ ... ]
> + case hwmon_temp:
> + switch (attr) {
> + case hwmon_temp_max:
> + if (channel)
> + addr = MCP9982_EXT_HIGH_LIMIT_ADDR(channel);
> + else
> + addr = MCP9982_INTERNAL_HIGH_LIMIT_ADDR;
> +
> + return ret = mcp9982_write_limit(priv, addr, val);
Pointless assignment to ret.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] hwmon: add support for MCP998X
2026-01-27 15:18 ` [PATCH v3 2/2] " victor.duicu
` (3 preceding siblings ...)
2026-02-03 20:09 ` Guenter Roeck
@ 2026-02-03 20:45 ` Guenter Roeck
4 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2026-02-03 20:45 UTC (permalink / raw)
To: victor.duicu
Cc: robh, krzk+dt, conor+dt, corbet, marius.cristea, linux-hwmon,
linux-kernel, devicetree, linux-doc
On Tue, Jan 27, 2026 at 05:18:22PM +0200, victor.duicu@microchip.com wrote:
> From: Victor Duicu <victor.duicu@microchip.com>
>
> This is the driver for Microchip MCP998X/33 and MCP998XD/33D
> Multichannel Automotive Temperature Monitor Family.
>
> Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
Gemini is having fun. Another good point.
> +#define MCP9982_HIGH_LIMIT_DEFAULT 21000
AI:
The default high limit of 21 degrees C is extremely low and likely to cause
immediate alarms on many systems (e.g., room temperature is often > 21C,
and inside a case it is almost certainly higher). Standard practice is to
leave limits as configured by BIOS/firmware, or set them to the chip's
maximum rating if unconfigured. Overwriting them unconditionally to 21C
is a regression in expected behavior and usability.
Me: I would not call this a regression, but Gemini does have a point.
Why such a low high limit ? At the very least that warrants an
explanation/comment.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: hwmon: add support for MCP998X
2026-02-03 19:15 ` Guenter Roeck
@ 2026-02-06 14:17 ` Victor.Duicu
2026-02-06 16:49 ` Krzysztof Kozlowski
0 siblings, 1 reply; 18+ messages in thread
From: Victor.Duicu @ 2026-02-06 14:17 UTC (permalink / raw)
To: linux
Cc: corbet, linux-hwmon, devicetree, robh, linux-kernel, krzk+dt,
linux-doc, Marius.Cristea, conor+dt
On Tue, 2026-02-03 at 11:15 -0800, Guenter Roeck wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Tue, Jan 27, 2026 at 05:18:21PM +0200,
> victor.duicu@microchip.com wrote:
> > From: Victor Duicu <victor.duicu@microchip.com>
> >
> > This is the devicetree schema for Microchip MCP998X/33 and
> > MCP998XD/33D Multichannel Automotive Temperature Monitor Family.
> >
> > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> > Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
>
> Some AI generated feedback inline, generated using Gemini 3 and Chris
> Mason's
> prompts as base. I don't know much if anything about devicetree
> properties,
> but it does seem to me that the AI has a point.
>
> > ---
> > .../bindings/hwmon/microchip,mcp9982.yaml | 205
> > ++++++++++++++++++
> > MAINTAINERS | 6 +
> > 2 files changed, 211 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> > b/Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> > new file mode 100644
> > index 000000000000..05ea3c6a5618
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> > @@ -0,0 +1,205 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwmon/microchip,mcp9982.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Microchip MCP998X/33 and MCP998XD/33D Temperature Monitor
> > +
> > +maintainers:
> > + - Victor Duicu <victor.duicu@microchip.com>
> > +
> > +description: |
> > + The MCP998X/33 and MCP998XD/33D family is a high-accuracy 2-wire
> > + multichannel automotive temperature monitor.
> > + The datasheet can be found here:
> > +
> > https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - microchip,mcp9933
> > + - microchip,mcp9933d
> > + - microchip,mcp9982
> > + - microchip,mcp9982d
> > + - microchip,mcp9983
> > + - microchip,mcp9983d
> > + - microchip,mcp9984
> > + - microchip,mcp9984d
> > + - microchip,mcp9985
> > + - microchip,mcp9985d
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + items:
> > + - description: Signal coming from ALERT/THERM pin.
> > + - description: Signal coming from THERM/ADDR pin.
> > + - description: Signal coming from SYS_SHDN pin.
> > +
> > + interrupt-names:
> > + items:
> > + - const: alert-therm
> > + - const: therm-addr
> > + - const: sys-shutdown
>
> The top-level definition of interrupt-names specifies exactly 3
> items.
> How does this interact with variants that only have 2 interrupts?
>
The chips with "D" in the family have the sys-shutdown and alert-therm
interrupt pins. The rest have alert-therm and therm-addr interrupt
pins. The conditional assigns the interrupt names depending on the
chip.
...
>
> > + items:
> > + - const: alert-therm
> > + - const: therm-addr
> > + microchip,power-state: true
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + pattern: '^microchip,mcp998(2|3)$'
> > + then:
> > + properties:
> > + microchip,enable-anti-parallel: false
>
> If "D" variants only support Run mode as described in the property
> description, why is this property required in the devicetree?
>
> Second feedback:
>
> Since the description says "D" versions can only be set in Run mode
> (where True sets Run state), should this property also have a const:
> true
> constraint here?
>
The property that sets the operation mode is microchip,power-state.
Chips with "D" can work only in Run mode, but the other ones can
work in Run or Standby mode.
In the conditions we force Run mode on the chips that require it
and accept either mode in the other half of the family.
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + pattern: '^microchip,mcp998(2|3)d$'
> > + then:
> > + properties:
> > + microchip,enable-anti-parallel: false
> > + required:
> > + - microchip,parasitic-res-on-channel1-2
> > + - microchip,parasitic-res-on-channel3-4
>
> Should the parasitic resistance compensation properties be required?
> These
> represent board-specific parasitics and may not be present on all
> designs using these chip variants.
>
As noted in the documentation, for chips with "D" parasitic resistance
error correction must be enabled so that the hardware shutdown feature
can't be overridden.
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: hwmon: add support for MCP998X
2026-01-27 15:18 ` [PATCH v3 1/2] dt-bindings: hwmon: add support " victor.duicu
2026-02-03 19:15 ` Guenter Roeck
@ 2026-02-06 16:47 ` Krzysztof Kozlowski
2026-02-09 14:46 ` Victor.Duicu
1 sibling, 1 reply; 18+ messages in thread
From: Krzysztof Kozlowski @ 2026-02-06 16:47 UTC (permalink / raw)
To: victor.duicu, linux, robh, krzk+dt, conor+dt, corbet
Cc: marius.cristea, linux-hwmon, linux-kernel, devicetree, linux-doc
On 27/01/2026 16:18, victor.duicu@microchip.com wrote:
> From: Victor Duicu <victor.duicu@microchip.com>
>
> This is the devicetree schema for Microchip MCP998X/33 and
> MCP998XD/33D Multichannel Automotive Temperature Monitor Family.
>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
> ---
> .../bindings/hwmon/microchip,mcp9982.yaml | 205 ++++++++++++++++++
> MAINTAINERS | 6 +
> 2 files changed, 211 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml b/Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> new file mode 100644
> index 000000000000..05ea3c6a5618
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/microchip,mcp9982.yaml
> @@ -0,0 +1,205 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/microchip,mcp9982.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip MCP998X/33 and MCP998XD/33D Temperature Monitor
> +
> +maintainers:
> + - Victor Duicu <victor.duicu@microchip.com>
> +
> +description: |
> + The MCP998X/33 and MCP998XD/33D family is a high-accuracy 2-wire
> + multichannel automotive temperature monitor.
> + The datasheet can be found here:
> + https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
> +
> +properties:
> + compatible:
> + enum:
> + - microchip,mcp9933
> + - microchip,mcp9933d
> + - microchip,mcp9982
> + - microchip,mcp9982d
> + - microchip,mcp9983
> + - microchip,mcp9983d
> + - microchip,mcp9984
> + - microchip,mcp9984d
> + - microchip,mcp9985
> + - microchip,mcp9985d
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + items:
> + - description: Signal coming from ALERT/THERM pin.
> + - description: Signal coming from THERM/ADDR pin.
> + - description: Signal coming from SYS_SHDN pin.
As Guenter pointed out - code is wrong (thanks Guenter!)
This does not match your if:then:.
> +
> + interrupt-names:
> + items:
> + - const: alert-therm
> + - const: therm-addr
> + - const: sys-shutdown
Neither this.
...
> + then:
> + properties:
> + interrupts-names:
> + items:
> + - const: alert-therm
> + - const: sys-shutdown
So three interrupts, but two AND three interrupt-names? This is mess.
> + required:
> + - microchip,power-state
> + else:
> + properties:
> + interrupts-names:
> + items:
> + - const: alert-therm
> + - const: therm-addr
Where are all b4 lore links for previous versions so I can trace the
actual review happening here? Why aren't you using b4?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: hwmon: add support for MCP998X
2026-02-06 14:17 ` Victor.Duicu
@ 2026-02-06 16:49 ` Krzysztof Kozlowski
2026-02-06 16:51 ` Krzysztof Kozlowski
0 siblings, 1 reply; 18+ messages in thread
From: Krzysztof Kozlowski @ 2026-02-06 16:49 UTC (permalink / raw)
To: Victor.Duicu, linux
Cc: corbet, linux-hwmon, devicetree, robh, linux-kernel, krzk+dt,
linux-doc, Marius.Cristea, conor+dt
On 06/02/2026 15:17, Victor.Duicu@microchip.com wrote:
>>> +
>>> + interrupts:
>>> + items:
>>> + - description: Signal coming from ALERT/THERM pin.
>>> + - description: Signal coming from THERM/ADDR pin.
>>> + - description: Signal coming from SYS_SHDN pin.
>>> +
>>> + interrupt-names:
>>> + items:
>>> + - const: alert-therm
>>> + - const: therm-addr
>>> + - const: sys-shutdown
>>
>> The top-level definition of interrupt-names specifies exactly 3
>> items.
>> How does this interact with variants that only have 2 interrupts?
>>
>
> The chips with "D" in the family have the sys-shutdown and alert-therm
> interrupt pins. The rest have alert-therm and therm-addr interrupt
> pins. The conditional assigns the interrupt names depending on the
> chip.
No, the top level says you have three interrupts. Do not create bindings
which contradict themselves.
More important I am 100% sure this fails tests if you wrote proper, so a
complete example. It passes only because you made a limited example,
without properties.
No, drop review, fix and request re-review.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: hwmon: add support for MCP998X
2026-02-06 16:49 ` Krzysztof Kozlowski
@ 2026-02-06 16:51 ` Krzysztof Kozlowski
0 siblings, 0 replies; 18+ messages in thread
From: Krzysztof Kozlowski @ 2026-02-06 16:51 UTC (permalink / raw)
To: Victor.Duicu, linux
Cc: corbet, linux-hwmon, devicetree, robh, linux-kernel, krzk+dt,
linux-doc, Marius.Cristea, conor+dt
On 06/02/2026 17:49, Krzysztof Kozlowski wrote:
> On 06/02/2026 15:17, Victor.Duicu@microchip.com wrote:
>>>> +
>>>> + interrupts:
>>>> + items:
>>>> + - description: Signal coming from ALERT/THERM pin.
>>>> + - description: Signal coming from THERM/ADDR pin.
>>>> + - description: Signal coming from SYS_SHDN pin.
>>>> +
>>>> + interrupt-names:
>>>> + items:
>>>> + - const: alert-therm
>>>> + - const: therm-addr
>>>> + - const: sys-shutdown
>>>
>>> The top-level definition of interrupt-names specifies exactly 3
>>> items.
>>> How does this interact with variants that only have 2 interrupts?
>>>
>>
>> The chips with "D" in the family have the sys-shutdown and alert-therm
>> interrupt pins. The rest have alert-therm and therm-addr interrupt
>> pins. The conditional assigns the interrupt names depending on the
>> chip.
>
>
> No, the top level says you have three interrupts. Do not create bindings
> which contradict themselves.
>
> More important I am 100% sure this fails tests if you wrote proper, so a
> complete example. It passes only because you made a limited example,
> without properties.
>
> No, drop review, fix and request re-review.
And I already TOLD YOU THIS!
https://lore.kernel.org/all/20250901-piquant-rousing-skunk-14da73@kuoka/
Which you completely ignored!
So you received review, you ignored it and kept pushing buggy patch.
NAK
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 0/2] add support in hwmon for MCP998X
2026-01-27 15:18 [PATCH v3 0/2] add support in hwmon for MCP998X victor.duicu
2026-01-27 15:18 ` [PATCH v3 1/2] dt-bindings: hwmon: add support " victor.duicu
2026-01-27 15:18 ` [PATCH v3 2/2] " victor.duicu
@ 2026-02-06 16:55 ` Krzysztof Kozlowski
2 siblings, 0 replies; 18+ messages in thread
From: Krzysztof Kozlowski @ 2026-02-06 16:55 UTC (permalink / raw)
To: victor.duicu, linux, robh, krzk+dt, conor+dt, corbet
Cc: marius.cristea, linux-hwmon, linux-kernel, devicetree, linux-doc
On 27/01/2026 16:18, victor.duicu@microchip.com wrote:
> From: Victor Duicu <victor.duicu@microchip.com>
>
> Add support in hwmon for Microchip MCP998X/33 and MCP998XD/33D Multichannel
> Automotive Temperature Monitor Family.
>
> The chips in the family have different numbers of external channels,
> ranging from 1 (MCP9982) to 4 channels (MCP9985).
> Reading diodes in anti-parallel connection is supported by MCP9984/85/33
> and MCP9984D/85D/33D. Dedicated hardware shutdown circuitry is present
> only in MCP998XD and MCP9933D.
>
> The driver supports reading the temperature channels, the temperature
> limits and their corresponding alarms. The user can set the limits,
> the update interval and the hysteresis.
>
> This driver is based on the IIO driver for MCP998X:
> https://lore.kernel.org/all/20250930133131.13797-1-victor.duicu@microchip.com/
>
> Differences related to previous patch:
> v3:
That's v9 and I already asked you to start numbering this correctly and
include ENTIRE previous changelog.
What's more important you ignored several people's feedback over the
time. Did not implement it and did not respond to it. These were several
people, David, me, Andy, Jonathan - you ignored all of them?
Lack of responses from you mean this review is pointless and I will be
just NAKing your patches.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: hwmon: add support for MCP998X
2026-02-06 16:47 ` Krzysztof Kozlowski
@ 2026-02-09 14:46 ` Victor.Duicu
0 siblings, 0 replies; 18+ messages in thread
From: Victor.Duicu @ 2026-02-09 14:46 UTC (permalink / raw)
To: linux, robh, krzk+dt, conor+dt, corbet, krzk
Cc: Marius.Cristea, linux-hwmon, devicetree, linux-kernel, linux-doc
Hi Krzysztof,
> > +
> > + interrupts:
> > + items:
> > + - description: Signal coming from ALERT/THERM pin.
> > + - description: Signal coming from THERM/ADDR pin.
> > + - description: Signal coming from SYS_SHDN pin.
>
> As Guenter pointed out - code is wrong (thanks Guenter!)
>
> This does not match your if:then:.
>
> > +
> > + interrupt-names:
> > + items:
> > + - const: alert-therm
> > + - const: therm-addr
> > + - const: sys-shutdown
>
> Neither this.
>
>
> ...
>
> > + then:
> > + properties:
> > + interrupts-names:
> > + items:
> > + - const: alert-therm
> > + - const: sys-shutdown
>
> So three interrupts, but two AND three interrupt-names? This is mess.
I was mistakenly under the impression that I could set two out of three
possible interrupts depending on the chip. But I understand now that
in the code written as is all chips have three interrupts.
Indeed, this error was present multiple versions in the past and you
identified it in v4 of the IIO driver. I failed to fix it then, I
apologize.
I will use two interrupts only and remove the if:then code regarding
them.
Kind Regards,
Victor
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2026-02-09 14:47 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-01-27 15:18 [PATCH v3 0/2] add support in hwmon for MCP998X victor.duicu
2026-01-27 15:18 ` [PATCH v3 1/2] dt-bindings: hwmon: add support " victor.duicu
2026-02-03 19:15 ` Guenter Roeck
2026-02-06 14:17 ` Victor.Duicu
2026-02-06 16:49 ` Krzysztof Kozlowski
2026-02-06 16:51 ` Krzysztof Kozlowski
2026-02-06 16:47 ` Krzysztof Kozlowski
2026-02-09 14:46 ` Victor.Duicu
2026-01-27 15:18 ` [PATCH v3 2/2] " victor.duicu
2026-01-27 18:51 ` Guenter Roeck
2026-02-02 8:15 ` Victor.Duicu
2026-02-02 15:18 ` Guenter Roeck
2026-02-03 13:31 ` Victor.Duicu
2026-01-27 22:23 ` kernel test robot
2026-02-03 19:45 ` Guenter Roeck
2026-02-03 20:09 ` Guenter Roeck
2026-02-03 20:45 ` Guenter Roeck
2026-02-06 16:55 ` [PATCH v3 0/2] add support in hwmon " Krzysztof Kozlowski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox