Add support for MAX31875 #2635

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

…_info Introduced chip_info structure to replace enum chips Signed-off-by: John Erasmus Mari Geronimo <[email protected]>

Add max31875 to dt-bindings of max31827 MAX31875 is low-power I2C temperature sensor similar to MAX31827 Signed-off-by: John Erasmus Mari Geronimo <[email protected]>

Add support for MAX31875 Low-Power I2C Temperature Sensor in MAX31827 driver Signed-off-by: John Erasmus Mari Geronimo <[email protected]>

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.

nunojsa · 2024-11-29T08:30:50Z

drivers/hwmon/max31827.c

+struct max31827_state;
+
+struct max31827_chip_info {
+	u32 alarm_pol_default : 1;


Drop the bit assignment. Just make it u8 and don't overcomplicate :). Arguably, you could also already add the hwmon_info struture in this patch.

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

drivers/hwmon/max31827.c

-	{ "max31829", max31829 },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, max31827_i2c_ids);


While I agree with moving this to it's natural place, this change is not strictly needed. So, bear in mind that you can get some comments about this.

nunojsa · 2024-11-29T08:33:22Z

drivers/hwmon/max31827.c

 static const struct of_device_id max31827_of_match[] = {
 	{
 		.compatible = "adi,max31827",
-		.data = (void *)max31827
+		.data = (void *)&max31827


no need for the void cast...

nunojsa · 2024-11-29T08:35:45Z

drivers/hwmon/max31827.c

@@ -616,6 +589,8 @@ static int max31827_probe(struct i2c_client *client)
 	if (!st)
 		return -ENOMEM;

+	st->chip_info = (struct max31827_chip_info *)(uintptr_t)device_get_match_data(dev);


Just do device_get_match_data() and drop the casts (they are not fine either way)... Also take the opportunity to add some error checking:

if (!st->chip_info) return -ENODEV;

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

@@ -234,7 +372,7 @@ static int max31827_read(struct device *dev, enum hwmon_sensor_types type,
 				 */

 				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

@@ -503,15 +641,19 @@ static int max31827_init_client(struct max31827_state *st,
 	fwnode = dev_fwnode(dev);

 	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...

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 nunojsa, mhennerich, dbogdan, btogorean, chegbeli, buha, amiclaus, rbolboac, danmois, CiprianRegus and machschmitt 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 from 31e479a to c810a02 Compare November 13, 2024 03:25

jemfgeronimo added 2 commits November 13, 2024 11:36

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

1867a1b

…_info Introduced chip_info structure to replace enum chips Signed-off-by: John Erasmus Mari Geronimo <[email protected]>

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

b5df72b

Add max31875 to dt-bindings of max31827 MAX31875 is low-power I2C temperature sensor similar to MAX31827 Signed-off-by: John Erasmus Mari Geronimo <[email protected]>

jemfgeronimo force-pushed the dev/max31875-v2 branch 2 times, most recently from 2995aed to 4b56b21 Compare November 13, 2024 03:45

drivers: hwmon: max31827: add max31875

1a82903

Add support for MAX31875 Low-Power I2C Temperature Sensor in MAX31827 driver Signed-off-by: John Erasmus Mari Geronimo <[email protected]>

jemfgeronimo force-pushed the dev/max31875-v2 branch 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for MAX31875 #2635

Add support for MAX31875 #2635

jemfgeronimo commented Oct 27, 2024 •

edited

Loading

nunojsa commented Oct 28, 2024

jemfgeronimo commented Oct 28, 2024

nunojsa left a comment

nunojsa Nov 4, 2024

jemfgeronimo commented Nov 13, 2024

nunojsa left a comment

nunojsa Nov 29, 2024

nunojsa Nov 29, 2024

nunojsa Nov 29, 2024

nunojsa Nov 29, 2024

nunojsa Nov 29, 2024

nunojsa Nov 29, 2024

nunojsa Nov 29, 2024

nunojsa Nov 29, 2024

Add support for MAX31875 #2635

Are you sure you want to change the base?

Add support for MAX31875 #2635

Conversation

jemfgeronimo commented Oct 27, 2024 • edited Loading

PR Description

PR Type

PR Checklist

nunojsa commented Oct 28, 2024

jemfgeronimo commented Oct 28, 2024

nunojsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jemfgeronimo commented Nov 13, 2024

nunojsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jemfgeronimo commented Oct 27, 2024 •

edited

Loading