Add support for MAX31875 by jemfgeronimo · Pull Request #2635 · analogdevicesinc/linux

jemfgeronimo · 2024-10-27T09:27:04Z

PR Description

This adds support for the MAX31875 temperature sensor within the MAX31827 driver, along with corresponding device tree bindings.
Datasheet: MAX31875
Tested on RPI4 with MAX31875EVKIT

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)

nunojsa · 2024-10-28T06:56:28Z

Why the new PR? Should we close #2600?

jemfgeronimo · 2024-10-28T07:00:42Z

Why the new PR? Should we close #2600?

I've created this separate PR that introduces an alternative method to add driver support for the MAX31875. This involves using the existing MAX31827 driver to support the MAX31875.

nunojsa

I think most of the complexity could go away with a chi_info struture where you have things like different register masks per device. Then accessing it in the code would be for example: st->info->cfg_reg.

Anyways, i think we can support this new device in the existing driver so I'll close the other PR

nunojsa · 2024-11-04T15:24:35Z

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


 maintainers:
-  - Daniel Matyas <daniel.matyas@analog.com>
+  - John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>


You should mention this change in the commit message. You'll likely be asked why if we can have a fallback device for this one (one of the devices that the driver already supports). So you should also mention in the commit message why that is not possible.

Also speaking about the commit message, you're only stating the obvious there :). Give a small description of the HW.

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

drivers/hwmon/max31827.c

jemfgeronimo · 2024-11-13T05:51:18Z

V2

dt-bindings: reverted enum of adi,fault-q
dt-bindings: dropped else branch in adi,fault-q
drivers: put error handling of fault queue values before its encoding
renamed max31827_chip_info to max31827_hwmon_chip_info
add chip_info structure to replace enum chips

nunojsa

Ok, the chip info structure grew quite a bit but IMHO, it's still better than all the if() else stuff we had before.

Take my inputs and send this upstream.

drivers/hwmon/max31827.c

nunojsa · 2024-11-29T08:36:44Z

drivers/hwmon/max31827.c

+	long (*from_16_bit_to_m_dgr)(unsigned int x);
+	unsigned int (*from_m_dgr_to_16_bit)(long x);
+	int (*shutdown)(struct max31827_state *st, unsigned int *cnv_rate);
+	int (*wakeup)(struct max31827_state *st, unsigned int cnv_rate);


Ok, I did not realized at all that this would need so much fields. That said, I think you can make this much better. See some examples below and then see what you can do...

nunojsa · 2024-11-29T08:44:19Z

drivers/hwmon/max31827.c

-			*val = !!uval;
+			uval = max31827_field_get(st->chip_info->shutdown_mask,
+						  uval);
+			*val = st->chip_info->is_enabled(uval);


For example... In here, you already have an is_enabled() callback per chip. Hence, just implement it in a different way per chip so that you don't really need config_reg or shutdown_mask per chip_info. Do you see my point? And if the function is pretty much the same for all the variants and you'r worried about code repetition, just implement a core function that accepts a config_reg and a shutdown_mask and build on top of it. Example:

bool __max31827_is_enabled(struct max31827_state *st, unsigned int reg, unsigned int mask) { //core generic implementation } // This is the per chip callback bool maxxxxx_is_enabled(struct max31827_state *st, unsigned int reg, unsigned int mask) { return __max31827_is_enabled(st, MAXXXXX_REG_CFGm MAXXXX_SHUTDOWN_MASK) }

See the idea? You might be able to reduce the number of fields significantly...

nunojsa · 2024-11-29T08:53:09Z

drivers/hwmon/max31827.c


 				ret = regmap_update_bits(st->regmap,
-							 MAX31827_CONFIGURATION_REG,
+							 st->chip_info->config_reg,


Hmm, I see you still need config_reg in a lot other places but the above point still stands. You could also negate the enable state in proper variant callback (which is the only thing you're doing now in the is_enabled() function which is a bit odd)

Think about it and see if we can reduce the number of fields somehow.

nunojsa · 2024-11-29T08:55:03Z

drivers/hwmon/max31827.c


 	st->enable = true;
-	res |= MAX31827_DEVICE_ENABLE(1);
+	res |= st->chip_info->device_enable(1);


maybe an init_client callback per chip (in the same way I explained above) would also reduce things a bit...

…_info Introduced chip_info structure to replace enum chips Signed-off-by: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>

Add max31875 to dt-bindings of max31827 MAX31875 is low-power I2C temperature sensor similar to MAX31827 Signed-off-by: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>

Add support for MAX31875 Low-Power I2C Temperature Sensor in MAX31827 driver Signed-off-by: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>

jemfgeronimo · 2024-12-05T07:58:27Z

V3

reduced the fields of the chip_info

nunojsa

Looked at it very superficially now. Seems decent enough to be sent upstream

nunojsa · 2025-01-22T11:27:49Z

drivers/hwmon/max31827.c

+
+	val = FIELD_GET(MAX31875_CONFIGURATION_RESOLUTION_MASK, val);
+
+	return scnprintf(buf, PAGE_SIZE, "%u\n", max31827_resolutions[val]);


sysfs_emit()

nunojsa · 2025-01-22T11:28:55Z

drivers/hwmon/max31827.c

+						 MAX31875_CONFIGURATION_SHUTDOWN_MASK,
+						 MAX31875_DEVICE_ENABLE(val));
+
+			mutex_unlock(&st->lock);


consider the cleanup.h auto lock primitives. Might be introduced in a different path if the lock already existed in the driver. But this is optional and up to you

nunojsa · 2025-01-22T11:29:38Z

drivers/hwmon/max31827.c

+			 */
+			if (max31827_resolutions[st->resolution] == 12 &&
+			    st->update_interval == 125)
+				usleep_range(15000, 20000);


nit: could use fsleep()

nunojsa · 2025-07-02T13:31:19Z

Hi, I'm just checking in on the status of this pull request. Is there anything I can do to help move it forward?

nunojsa · 2026-01-16T11:03:28Z

@jemfgeronimo Pinging this one again? I guess I'll close it if no one takes ownership of this one...

jemfgeronimo · 2026-01-16T11:14:25Z

@jemfgeronimo Pinging this one again? I guess I'll close it if no one takes ownership of this one...

Will update in the coming weeks.

nunojsa · 2026-01-16T14:04:10Z

Will update in the coming weeks.

Can you see my comment in #2751. Is it not the same? Which one should I close (or am I missing something)?

jemfgeronimo · 2026-01-20T02:20:31Z

Will use #2751

jemfgeronimo changed the title ~~Add support for MAX31875~~ Add support for MAX31875 v2 Oct 27, 2024

jemfgeronimo marked this pull request as ready for review October 27, 2024 09:32

jemfgeronimo requested review from CiprianRegus, amiclaus, btogorean, buha, chegbeli, danmois, dbogdan, machschmitt, mhennerich, nunojsa and rbolboac as code owners October 27, 2024 09:32

jemfgeronimo force-pushed the dev/max31875-v2 branch from 7165f8f to 4e669bc Compare October 27, 2024 09:42

jemfgeronimo changed the title ~~Add support for MAX31875 v2~~ Add support for MAX31875 Oct 27, 2024

jemfgeronimo mentioned this pull request Oct 28, 2024

Add support for MAX31875 #2600

Closed

6 tasks

jemfgeronimo force-pushed the dev/max31875-v2 branch from 4e669bc to 31e479a Compare October 28, 2024 13:00

nunojsa reviewed Nov 4, 2024

View reviewed changes

jemfgeronimo force-pushed the dev/max31875-v2 branch 4 times, most recently from 4b56b21 to 1a82903 Compare November 13, 2024 04:13

jemfgeronimo requested a review from nunojsa November 13, 2024 05:51

nunojsa reviewed Nov 29, 2024

View reviewed changes

jemfgeronimo added 2 commits December 5, 2024 13:31

drivers: hwmon: max31827: refactor enum chips to struct max31827_chip…

f6c8e16

…_info Introduced chip_info structure to replace enum chips Signed-off-by: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>

dt-bindings: hwmon: adi,max31827: add MAX31875

a59ea35

Add max31875 to dt-bindings of max31827 MAX31875 is low-power I2C temperature sensor similar to MAX31827 Signed-off-by: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>

jemfgeronimo force-pushed the dev/max31875-v2 branch from 1a82903 to 89c000e Compare December 5, 2024 07:48

drivers: hwmon: max31827: add max31875

ca0fff8

Add support for MAX31875 Low-Power I2C Temperature Sensor in MAX31827 driver Signed-off-by: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>

jemfgeronimo force-pushed the dev/max31875-v2 branch from 89c000e to ca0fff8 Compare December 5, 2024 07:53

nunojsa reviewed Jan 22, 2025

View reviewed changes

nunojsa mentioned this pull request Jan 16, 2026

Dev/lm75 add max31875 #2751

Draft

6 tasks

jemfgeronimo closed this Jan 20, 2026


		val = FIELD_GET(MAX31875_CONFIGURATION_RESOLUTION_MASK, val);

		return scnprintf(buf, PAGE_SIZE, "%u\n", max31827_resolutions[val]);

Conversation

jemfgeronimo commented Oct 27, 2024 • 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 commented Oct 28, 2024

Uh oh!

jemfgeronimo commented Oct 28, 2024

Uh oh!

nunojsa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jemfgeronimo commented Nov 13, 2024

Uh oh!

nunojsa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

jemfgeronimo commented Dec 5, 2024

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!

nunojsa commented Jul 2, 2025

Uh oh!

nunojsa commented Jan 16, 2026

Uh oh!

jemfgeronimo commented Jan 16, 2026

Uh oh!

nunojsa commented Jan 16, 2026

Uh oh!

jemfgeronimo commented Jan 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jemfgeronimo commented Oct 27, 2024 •

edited

Loading