add max31732 driver #2959

sdivarci · 2025-09-26T15:46:45Z

PR Description

Add MAX31732 hwmod driver and documentation.

PR Type

Bug fix (a change that fixes an issue)
New feature (a change that adds new functionality)
Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

I have conducted a self-review of my own code changes
I have tested the changes on the relevant hardware
I have updated the documentation outside this repo accordingly (if there is the case)

Add support for Analog Devices MAX31732, a five-channel temperature monitor with per-channel alarms, calibration helpers, optional IRQ handling, and MTP configuration. Signed-off-by: Sinan Divarci <sinan.divarci@analog.com>

Add documentation for the Analog Devices MAX31732 temperature sensor driver under Documentation/hwmon. Signed-off-by: Sinan Divarci <sinan.divarci@analog.com>

Add device tree bindings for the Analog Devices MAX31732 temperature sensor driver under Documentation/devicetree/bindings/hwmon/. Signed-off-by: Sinan Divarci <sinan.divarci@analog.com>

nunojsa

Hi Sinan,

Just a first high level review. We first need to have this driver using modern hwmon API and have the userspace interface figured.

Also note that the expectation is for this to be first upstreamed (and accepted) before merging it in our tree

nunojsa · 2025-09-29T14:48:44Z

Documentation/hwmon/max31732.rst

+
+* All temperatures reported via sysfs are in milli-degree Celsius.
+* ``stop`` halts conversions but does not power-cycle the part; use ``soft_por``
+  to restore all the registers to their default values.


this patch can typically be squashed with the one adding the driver

nunojsa · 2025-09-29T14:52:30Z

drivers/hwmon/max31732.c

+static const struct attribute_group mtp_attr_group = {
+	.name  = "mtp",
+	.attrs = mtp_attrs,
+};


The above needs to be re-thought. The above is not the way to expose the hwmon interface. The below is what you need to be using (an example):

https://elixir.bootlin.com/linux/v6.17/source/drivers/hwmon/ltc4282.c#L1563

Then you need to look at the standard hwmon ABI:

https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface

and try to map as much as you can. The reason is that you'll have to justify very carefully why you need some non standard interface to be exposed to userspace. Often we don't really need to expose everything.

nunojsa · 2025-09-29T14:53:37Z

drivers/hwmon/max31732.c

+	s32 ret;
+	u32 alarm_que;
+
+	if (fwnode_property_read_bool(dev_fwnode(dev), "adi,alarm1-interrupt-mode"))


just as an hint for future versions of the patches you have device_property_read_bool(dev, ...). So no need for all of those dev_fwnode()

nunojsa · 2025-09-29T14:54:40Z

drivers/hwmon/max31732.c

+
+	ret = devm_device_add_group(dev, &mtp_attr_group);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to create mtp sysfs group\n");


not how you should be adding attr groups. Needs to be done through the hwmon API (when registering the device)

nunojsa · 2025-09-29T14:55:19Z

drivers/hwmon/max31732.c

+	if (reg_val == 0)
+		ret = regmap_set_bits(data->regmap, MAX31732_REG_CONF1, MAX31732_CONF1_STOP);
+	else
+		ret = regmap_clear_bits(data->regmap, MAX31732_REG_CONF1, MAX31732_CONF1_STOP);


for the above you can use regmap_update_bits()

nunojsa · 2025-09-29T14:56:49Z

drivers/hwmon/max31732.c

+};
+
+MODULE_DEVICE_TABLE(i2c, max31732_ids);
+


nunojsa · 2025-09-29T14:57:00Z

drivers/hwmon/max31732.c

+	{ .compatible = "adi,max31732", },
+	{ },
+};
+


nunojsa · 2025-09-29T14:57:16Z

drivers/hwmon/max31732.c

+	.class		= I2C_CLASS_HWMON,
+	.driver = {
+		.name	= "max31732-driver",
+		.of_match_table = of_match_ptr(max31732_of_match),


drop of_match_ptr()

nunojsa · 2025-09-29T15:01:19Z

drivers/hwmon/max31732.c

+	.driver = {
+		.name	= "max31732-driver",
+		.of_match_table = of_match_ptr(max31732_of_match),
+		.pm	= &max31732_pm_ops,


use pm_sleep_ptr() together with DEFINE_SIMPLE_DEV_PM_OPS() and then no need of __maybe_unused

nunojsa · 2025-09-29T15:01:40Z

drivers/hwmon/max31732.c

+MODULE_DESCRIPTION("MAX31732 driver");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("1.0");
+MODULE_SOFTDEP("pre: regmap_i2c");


typically we dont really use the above two but I guess it does not harm

from man modprobe.d

The softdep command allows you to specify soft, or optional, module dependencies. modulename can be used without these optional modules installed, but usually with some features missing.

but

static int max31732_probe(struct i2c_client *client) { // ... data->regmap = devm_regmap_init_i2c(client, &regmap_config); if (IS_ERR(data->regmap)) return dev_err_probe(dev, PTR_ERR(data->regmap), "regmap init failed\n");

doesn't look optional to me.

M-Kamalesh · 2025-10-21T09:15:18Z

Documentation/devicetree/bindings/hwmon/adi,max31732.yaml

+  - compatible
+  - reg
+
+additionalProperties: false


The alarm properties are given as optional. But when I gave only compatible and reg in my dts and ignore the alarm part, it throws error.

[ 1.928192] max31732-driver 6-001c: error -EINVAL: cannot get ALARM1 IRQ [ 1.935765] max31732-driver 6-001c: error -EINVAL: failed to setup ALARM1 irq [ 1.944222] max31732-driver: probe of 6-001c failed with error -22

below is my dts,

&i2c6 { status = "okay"; tempsnr@1c { compatible = "adi,max31732"; reg = <0x1c>; };

machschmitt

Hi @sdivarci,

A few comments from me.
I'm not very familiar with hwmon so feel safe to disregard any potentially misleading suggestion.

machschmitt · 2025-10-16T21:30:44Z

Documentation/devicetree/bindings/hwmon/adi,max31732.yaml

+  reg:
+    description: I2C address of the device.
+    maxItems: 1
+


The guideline is to make dt docs as complete as possible. So, we can also describe the voltage supply here.

vcc-supply: true

machschmitt · 2025-10-16T21:32:19Z

Documentation/devicetree/bindings/hwmon/adi,max31732.yaml

+
+required:
+  - compatible
+  - reg


Unless I'm missing something, the device requires a power supply to operate, so

required: - compatible - reg - vcc-supply

machschmitt · 2025-10-16T21:34:46Z

Documentation/devicetree/bindings/hwmon/adi,max31732.yaml

+
+        sensor@1c {
+            compatible = "adi,max31732";
+            reg = <0x1c>;


and it also needs to appear in the example. e.g.

vcc-supply = <&supply_3_3V>;

machschmitt · 2025-10-21T13:49:35Z

drivers/hwmon/max31732.c

+#define MAX31732_TEMP_STEP_MC           1000
+#define MAX31732_TEMP_DIVISOR           16
+#define MAX31732_EXTENDED_RANGE_OFFSET  64000


nitpicking: these three are using spaces for indentation, while most of the other defines are using tabs. Standardize to using either spaces or tabs in all declarations (slight (personal) preference for tabs).

machschmitt · 2025-10-21T13:51:52Z

drivers/hwmon/max31732.c

+#define MAX31732_REG_CUSTOM_IDEAL_R1	0x11
+#define MAX31732_REG_CUST_IDEAL_ENABLE	0x15
+#define MAX31732_REG_TEMP_OFFSET	0x16
+#define MAX31732_REG_OFFSET_ENABLE	0x17
+#define MAX31732_REG_FILTER_ENABLE	0x18
+#define MAX31732_REG_BETA_COMP_ENABLE	0x19
+#define MAX31732_REG_HIGHEST_TEMP_ENABLE 0x1A
+#define MAX31732_REG_ALARM1_MASK	0x1B
+#define MAX31732_REG_ALARM2_MASK	0x1C
+#define MAX31732_REG_PRIM_HIGH_TEMP_R	0x1D
+#define MAX31732_REG_PRIM_HIGH_TEMP_L	0x25
+#define MAX31732_REG_PRIM_LOW_TEMP	0x27
+#define MAX31732_REG_SECOND_HIGH_TEMP_R	0x29
+#define MAX31732_REG_SECOND_HIGH_TEMP_L	0x2D


nitpicking: One additional tab would make these align nicely with #define MAX31732_CONF2_IGNORE_ISC2GND BIT(0) above.

machschmitt · 2025-10-21T14:01:18Z

drivers/hwmon/max31732.c

+static const struct regmap_config regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.cache_type = REGCACHE_RBTREE,


Do the device perform better with red-black tree cache compared to other cache types?
Regmap doc suggests new regmap clients to use other types of caches [1].
[1]: https://elixir.bootlin.com/linux/v6.17/source/include/linux/regmap.h#L62
Most other hwmon drivers that set cache type for regmap use REGCACHE_MAPLE.

machschmitt · 2025-10-21T14:46:24Z

drivers/hwmon/max31732.c

+static bool max31732_volatile_reg(struct device *dev, u32 reg)
+{
+	if ((reg >= MAX31732_REG_TEMP_R && reg <= MAX31732_REG_PRIM_LOW_STATUS) ||
+	    (reg >= MAX31732_MTP_REG_TEMP_R && reg <= MAX31732_MTP_REG_PRIM_LOW_STATUS))
+		return true;
+
+	if (reg == MAX31732_REG_SECOND_HIGH_STATUS || reg == MAX31732_REG_SECOND_LOW_STATUS)
+		return true;
+
+	if (reg == MAX31732_REG_TEMP_FAULT)
+		return true;
+
+	if (reg >= MAX31732_REG_BETA_COMPEN_R1 && reg <= MAX31732_REG_BETA_COMPEN_R4)
+		return true;
+
+	if (reg == MAX31732_REG_HIGHEST_TEMP)
+		return true;
+
+	if (reg == MAX31732_REG_MTP_CONF2)
+		return true;
+
+	if (reg >= MAX31732_MTP_REG_USER_SW_REV)
+		return true;
+	return false;
+}


Can we use regmap ranges to describe the volatile regs?
Something like

static const struct regmap_range max31732_regmap_volatile_ranges[] = { regmap_reg_range(MAX31732_REG_TEMP_R, MAX31732_REG_PRIM_LOW_STATUS), regmap_reg_range(MAX31732_MTP_REG_TEMP_R, MAX31732_MTP_REG_PRIM_LOW_STATUS), regmap_reg_range(MAX31732_REG_SECOND_HIGH_STATUS, MAX31732_REG_SECOND_HIGH_STATUS), regmap_reg_range(MAX31732_REG_SECOND_LOW_STATUS, MAX31732_REG_SECOND_LOW_STATUS), ... };

then the volatile function could be more concise

static bool max31732_volatile_reg(struct device *dev, u32 reg) return regmap_reg_in_ranges(reg, max31732_regmap_volatile_ranges, ARRAY_SIZE(max31732_regmap_volatile_ranges)); }

machschmitt · 2025-10-21T14:55:06Z

drivers/hwmon/max31732.c

+	dev_set_drvdata(dev, data);
+
+	if (fwnode_property_read_bool(dev_fwnode(dev), "adi,extended-range"))
+		ret = regmap_set_bits(data->regmap, MAX31732_REG_CONF1, MAX31732_CONF1_EXTRANGE);


The extended_range bit is only set here (at probe) and never changed after that. Then, there are a few places where it's read, (e.g. on max31732_read(), max31732_write(), max31732_highest_temperature_show(), ...). What about having a field for that in struct max31732_data so we can simplify code in those places where we currently do regmap_test_bits(data->regmap, MAX31732_REG_CONF1, MAX31732_CONF1_EXTRANGE);?

Also, to me, adi,extended-range doesn't seem very related to how hardware is setup. It's mostly a tweak in data offset. I worry dt maintainers might object to accepting that. Do hwmon offer any interface for changing data offset at runtime?

machschmitt · 2025-10-21T15:08:08Z

drivers/hwmon/max31732.c

+	ret = max31732_setup_irq(dev, data->regmap, "ALARM1", &data->irqs[0],
+				 MAX31732_REG_ALARM1_MASK, alarm1_mask);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to setup ALARM1 irq\n");


check irq is available before trying to set it up?

if (data->irqs[0]) { ret = max31732_setup_irq(dev, data->regmap, "ALARM1", &data->irqs[0], MAX31732_REG_ALARM1_MASK, alarm1_mask); ... }

machschmitt · 2025-10-21T15:09:03Z

drivers/hwmon/max31732.c

+	if (ret)
+		return dev_err_probe(dev, ret, "failed to setup ALARM1 irq\n");
+
+	ret = max31732_setup_irq(dev, data->regmap, "ALARM2", &data->irqs[1],


same here.
These might solve the issue reported by @M-Kamalesh .

sdivarci added 3 commits September 25, 2025 06:45

drivers: hwmon: add max31732 driver

97e1329

Add support for Analog Devices MAX31732, a five-channel temperature monitor with per-channel alarms, calibration helpers, optional IRQ handling, and MTP configuration. Signed-off-by: Sinan Divarci <sinan.divarci@analog.com>

docs: hwmon: add max31732 documentation

bda881a

Add documentation for the Analog Devices MAX31732 temperature sensor driver under Documentation/hwmon. Signed-off-by: Sinan Divarci <sinan.divarci@analog.com>

dt-bindings: hwmon: add bindings for max31732

1d17693

Add device tree bindings for the Analog Devices MAX31732 temperature sensor driver under Documentation/devicetree/bindings/hwmon/. Signed-off-by: Sinan Divarci <sinan.divarci@analog.com>

sdivarci requested review from CiprianRegus, amiclaus, btogorean, buha, chegbeli, danmois, dbogdan, machschmitt, mhennerich, nunojsa and rbolboac as code owners September 26, 2025 15:46

nunojsa reviewed Sep 29, 2025

View reviewed changes

M-Kamalesh reviewed Oct 21, 2025

View reviewed changes

machschmitt reviewed Oct 21, 2025

View reviewed changes

		};

		MODULE_DEVICE_TABLE(i2c, max31732_ids);

add max31732 driver #2959

Are you sure you want to change the base?

add max31732 driver #2959

Uh oh!

Conversation

sdivarci commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

PR Type

PR Checklist

Uh oh!

nunojsa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

machschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sdivarci commented Sep 26, 2025 •

edited

Loading