Add driver for MAX42500 by kentlibe · Pull Request #2659 · analogdevicesinc/linux

kentlibe · 2024-12-01T23:05:34Z

PR Description

This adds new driver for the MAX42500 Industrial Power System Monitor Family, along with corresponding device tree bindings.
Datasheet MAX42500
Tested in RPI4 with MAX42500_EVKIT_P1.

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-12-02T10:11:39Z

Is this still WIP? As it stands it's not really ready for reviewing... Please make sure to address CI complains and to re-organize your commits. Merge commits don't really make sense (rebase your dev branch) and please run git lon --oneline <DIR_YOU'RE_CHANGING> so you can the git log style for that subsystem. Then, please follow the same style. Also note that a commit as Cleanup MAX42500 Linux driver. is not fine. You need to explain what you're actually cleaning and why...

kentlibe · 2024-12-17T05:45:36Z

v2

ready for review
address CI complaints
address style issues
re-organize commits
rebase dev branch

kister-jimenez

First review attempt.

I think you need to do some adjustments in you formatting especially the tabs to make the code more readable. I have commented some of them, but I see a lot needs to be adjusted.

kentlibe · 2025-03-03T09:00:36Z

v4

split into 3 drivers: mfd, hwmon and watchdog
implement the mfd as the main device with the i2c driver
rework the hwmon driver to follow latest ABI for sysfs platform
rework the watchdog driver to follow watchdog sysfs platform
add debugfs to handle all non-standard attr in hwmon and watchdog
implement the two gpios control in power management format
renamed the 3 driver files according to their usage and purpose.
add documenatation for the hwmon and watchdog sysfs atttr usage

@nunojsa kindly review.

nunojsa

Looks better to me... If you want we can still do another internal review. If not, take my latest inputs and send it upstream. Up to you :)

nunojsa · 2025-03-03T09:56:00Z

+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>


Typically the above header is not really needed and not encouraged because it includes a lot of things one usually do not need. It seems you're missing some headers but you're not realizing it because they are being included by kernel.h. Always prefer to explicitly include the needed headers...

nunojsa · 2025-03-03T10:02:01Z

+	if (st->fpsr[MAX42500_FPSR_FPS_EN1_START_TIMER_ENABLE])
+		gpiod_set_value(st->sleep_gpio, GPIOD_OUT_HIGH);
+	else
+		gpiod_set_value(st->sleep_gpio, GPIOD_OUT_LOW);


Why not just something like?

gpiod_set_value(st->sleep_gpio, !!st->fpsr[MAX42500_FPSR_FPS_EN1_START_TIMER_ENABLE])

nunojsa · 2025-03-03T10:02:23Z

+	if (st->fpsr[MAX42500_FPSR_FPS_EN1_START_TIMER_ENABLE])
+		gpiod_set_value(st->sleep_gpio, GPIOD_OUT_LOW);
+	else
+		gpiod_set_value(st->sleep_gpio, GPIOD_OUT_HIGH);


nunojsa · 2025-03-03T10:02:32Z

+	if (IS_ERR(st->sleep_gpio))
+		return PTR_ERR(st->sleep_gpio);
+
+	devm_mutex_init(dev, &st->lock);


error handling

nunojsa · 2025-03-03T10:45:43Z

+	st->client = client;
+	st->regmap = regmap;
+
+	devm_mutex_init(dev, &st->lock);


error handling...

nunojsa · 2025-03-03T10:46:40Z

+{
+	struct device *parent_dev = pdev->dev.parent;
+	struct regmap *regmap = dev_get_drvdata(parent_dev);
+	struct i2c_client *client = to_i2c_client(parent_dev);


do you need the i2c_client?

will be removed

nunojsa · 2025-03-03T10:48:14Z

+	const u8 clk_div = MAX42500_WDOG_WD_CLOCK_DIVIDER;
+	const u8 win_mode = MAX42500_WDOG_WD_SIMPLE_MODE_ENABLE;
+	const u8 opn_win = MAX42500_WDOG_WD_OPEN_WINDOW_INTERVAL;
+	const u8 clo_win = MAX42500_WDOG_WD_CLOSE_WINDOW_INTERVAL;


I would drop the const qualifier. It does not give you much in here and the compiler should be more than capable of realizing it anyways without the qualifier

nunojsa · 2025-03-03T10:49:27Z

+
+	ret = regmap_write(st->regmap, MAX42500_REG_WDKEY, reg_val);
+	if (ret)
+		return ret;


For simple mode, yes can update this. But for challenge-response mode, we need to read the current key first, apply the LFSR algorithm and write back the new key. Maybe we could leave this here or go with the change?

Hmm, now that I look better into this, simple mode looks odd. We read the key and then write the same key back. Is this expected?

For simple mode, any key value would do (whether same or not) as long as the WDKEY register is updated before the watchdog resets. In the datasheet it states:
The watchdog can operate in challenge/response mode (in which a specific key value must be written to WDKEY) or in simple mode (in which any write to WDKEY will update the watchdog).

I see. Then I would say/ask. why not just writing 0 in the simple mode case? I mean, there's no real need for a register read right? And do the RMW only for the more complex case?

yes, I see that would be simpler and optimal. So, code would now look like:

guard(mutex)(&st->lock); switch (mode) { case MAX42500_WD_MODE_CH_RESP: ret = regmap_read(st->regmap, MAX42500_REG_WDKEY, &key); if (ret) return ret; /* Linear-Feedback Shift Register (LFSR) Polynomial Equation */ lfsr = ((key >> 7) ^ (key >> 5) ^ (key >> 4) ^ (key >> 3)) & 1; reg_val = (key << 1) | lfsr; return regmap_write(st->regmap, MAX42500_REG_WDKEY, reg_val); case MAX42500_WD_MODE_SIMPLE: return regmap_write(st->regmap, MAX42500_REG_WDKEY, 0); default: return -EINVAL; }

kentlibe · 2025-03-04T10:26:44Z

@nunojsa could you take one final review before we upstream?

kentlibe · 2025-03-05T05:25:59Z

@kister-jimenez is the dt-binding check error caused by a CI script error?

nunojsa · 2025-03-05T11:21:25Z

@kister-jimenez is the dt-binding check error caused by a CI script error?

Yes, our CI needs to be fixed for the bindings check

nunojsa · 2025-03-05T11:22:35Z

+properties:
+  compatible:
+    enum:
+      - adi,max42500-mfd


Typically, you do not need the -mfd for the real device (meaning for the top device). Keep it only for -hwmon and -wdt

nunojsa · 2025-03-05T11:27:23Z

+	gpiod_set_value(st->power_gpio, GPIOD_OUT_LOW);
+
+	gpiod_set_value(st->sleep_gpio,
+			st->fpsr[MAX42500_FPSR_FPS_EN1_START_TIMER_ENABLE]);


nit: I would do !!st->fpsr[MAX42500_FPSR_FPS_EN1_START_TIMER_ENABLE] just to be sure to pass 0 or 1 into gpiod_set_value()

nunojsa · 2025-03-05T11:37:27Z

+
+static int max42500_wdt_store_wd_rst2_cnt_enable_log(void *arg, u64 val)
+{
+	return max42500_wdt_wdog_write(arg, (long)val,


this explicit cast should not be needed

nunojsa · 2025-03-05T11:43:40Z

+
+static int max42500_wdt_show_wd_rst2_cnt_enable_log(void *arg, u64 *val)
+{
+	return max42500_wdt_wdog_read(arg, (long *)val,


not sure this is correct. This will go wrong in big endian 32bit archs... I would use a dedicated variable. This:

long __val; int ret; ret = max42500_wdt_wdog_read(arg, &__val, ...); if (ret) return ret; *val = __val;

Same for the other places where this applies. Note this is mostly a problem fro getters and not setters since getters use pointers which this cast to potentially smaller types could be problematic.

nunojsa · 2025-03-05T11:52:34Z

+
+	ret = regmap_write(st->regmap, MAX42500_REG_WDKEY, reg_val);
+	if (ret)
+		return ret;


Hmm, now that I look better into this, simple mode looks odd. We read the key and then write the same key back. Is this expected?

kentlibe · 2025-03-05T15:46:34Z

+		return ret;
+
+	/* Update after successful register write */
+	st->wdog[MAX42500_WDOG_WD_KEY] = FIELD_GET(GENMASK(7, 0), reg_val);


And I think I would also remove this line.

yeah, no need to cache all values you write even more when you can read them from the device

kentlibe · 2025-03-05T15:49:39Z

+	/* Update after successful register write */
+	st->wdog[MAX42500_WDOG_WD_KEY] = FIELD_GET(GENMASK(7, 0), reg_val);
+
+	return 0;


and this as well

nunojsa

Feel free to send this upstream. As another advice, I would improve the commit messages for the bindings patch. Saying "adding bindings file" or something like that is only stating the obvious, You should add a short description of the device being added (similar to the driver patch)

nunojsa · 2025-03-07T07:44:10Z

+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        max42500@28 {


I do not think this will be accepted... Needs to be more generic but honestly i'm not sure what can we put in here. I know I was the one to mention this but maybe just send this upstream as you had it before. Meaning, "hwmon" since the primary function of the device is monitoring.

ok will do. Previously I wrote "mfd" then "mfdev", but I thought it was too generic since all other mfd drivers are also too.

Saw something similar in richtek and/or ricoh mfd dt-bindings and was not sure if same for max42500. But I guess not.

nunojsa · 2025-03-07T07:45:11Z

+				   max42500_subdev, ARRAY_SIZE(max42500_subdev),
+				   NULL, 0, NULL);
+
+	return ret;


you can just return devm_mfd_add_devices(...)

nunojsa · 2025-03-07T07:45:47Z

+#include <linux/mutex.h>
+#include <linux/mfd/max42500.h>
+#include <linux/platform_device.h>
+#include <linux/mod_devicetable.h>


alphabetical order

nunojsa · 2025-03-07T07:46:41Z

+static struct platform_driver max42500_hwmon_driver = {
+	.driver = {
+		.name	= "max42500-hwmon",
+		.pm = &max42500_pm_ops,


use pm_sleep_ptr()

nunojsa · 2025-03-07T07:47:15Z

+	return 0;
+}
+
+EXPORT_SIMPLE_DEV_PM_OPS(max42500_pm_ops, max42500_suspend, max42500_resume);


why exporting this? Do you need this function anywhere else? I guess not...

will remove this

nunojsa · 2025-03-07T07:49:23Z

+	int ret;
+
+	ret = max42500_wdt_wdog_read(watchdog_get_drvdata(wdd), &wdog_val,
+				      MAX42500_WDOG_WD_STATUS);


alignment seems a bit off here

nunojsa · 2025-03-07T07:50:52Z

+	guard(mutex)(&st->lock);
+
+	switch (mode) {
+	case MAX42500_WD_MODE_CH_RESP:


nit: the mutex is only needed in this case

when the mutex is moved in the case, there is build error:

In file included from drivers/watchdog/max42500-wdt.c:8: drivers/watchdog/max42500-wdt.c: In function ‘max42500_wdt_set_watchdog_key’: ./include/linux/cleanup.h:86:2: error: a label can only be part of a statement and a declaration is not a statement 86 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \ | ^~~~~~ ./include/linux/cleanup.h:131:2: note: in expansion of macro ‘CLASS’ 131 | CLASS(_name, __UNIQUE_ID(guard)) | ^~~~~ drivers/watchdog/max42500-wdt.c:94:3: note: in expansion of macro ‘guard’ 94 | guard(mutex)(&st->lock); | ^~~~~ make[4]: *** [scripts/Makefile.build:243: drivers/watchdog/max42500-wdt.o] Error 1 make[3]: *** [scripts/Makefile.build:480: drivers/watchdog] Error 2 make[3]: *** Waiting for unfinished jobs....

You need the scoped version...

nunojsa · 2025-03-07T07:52:10Z

+		reg_val = (key << 1) | lfsr;
+
+		return regmap_write(st->regmap, MAX42500_REG_WDKEY, reg_val);
+	case MAX42500_WD_MODE_SIMPLE:


nit: I would be tempted to put a comment stating that we just need to do a random write in this case (just to avoid potential questions) - not sure if the intent is already obvious.

should I add the mutex in the case here too?

no, regmap already has an internal lock unless you disable it in the config (which you're not doing)

The MAX42500 component structure of the nodes and properties of the serial interfaces and the I/O ports to access, control and configure the voltage monitors, and to program the power sequence timestamp recordings and the windowed watchdog timing. Signed-off-by: Kent Libetario <Kent.Libetario@analog.com>

The MAX42500 is a multiple function device for power system monitoring, power sequence timestamp recordings and windowed watchdog combined all- together in one SoC package. These features can be used independently or at the same time in a wide range of applications. Signed-off-by: Kent Libetario <Kent.Libetario@analog.com>

The MAX42500 supports standard power management, hwmon and watchdog ABI and sysfs attribute entries to expose the sensor data of the voltage monitors, power sequence timestamp recordings and windowed watchdog to the userspace. Signed-off-by: Kent Libetario <Kent.Libetario@analog.com>

The MAX42500 is a SoC power-system monitor with up to seven voltage monitor inputs that has programmable OV/UV thresholds and support DVS by I2C interface. Also, it has a programmable flexible power sequence recorder that stores the power-up and power-down timestamps separately. Signed-off-by: Kent Libetario <Kent.Libetario@analog.com>

The MAX42500 contains a programmable windowed watchdog and can be used either as simple mode windowed watchdog or challenge-and-response mode watchdog, which is accessible through the I2C interface, along with a configurable RESET output. Signed-off-by: Kent Libetario <Kent.Libetario@analog.com>

Add entry for the MAX42500 driver Signed-off-by: Kent Libetario <Kent.Libetario@analog.com>

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-02-06T10:21:32Z

@kentlibe ping

kentlibe requested review from CiprianRegus, amiclaus, btogorean, buha, chegbeli, danmois, dbogdan, machschmitt, mhennerich, nunojsa and rbolboac as code owners December 1, 2024 23:05

kentlibe force-pushed the dev/max42500 branch 3 times, most recently from a3dad5b to a6008ba Compare December 10, 2024 07:49

kentlibe changed the title ~~Add driver support for MAX42500~~ Add driver for MAX42500 Dec 10, 2024

kentlibe force-pushed the dev/max42500 branch 4 times, most recently from 32dc5ba to 5920e35 Compare December 11, 2024 02:02

kister-jimenez suggested changes Dec 20, 2024

View reviewed changes

kentlibe force-pushed the dev/max42500 branch 8 times, most recently from 40e89e5 to d468dbd Compare January 31, 2025 08:09

kentlibe force-pushed the dev/max42500 branch 2 times, most recently from 870d915 to 144fac7 Compare March 3, 2025 08:38

nunojsa reviewed Mar 3, 2025

View reviewed changes

kentlibe force-pushed the dev/max42500 branch 2 times, most recently from 576aa49 to e305f27 Compare March 4, 2025 10:12

nunojsa reviewed Mar 5, 2025

View reviewed changes

kentlibe commented Mar 5, 2025

View reviewed changes

kentlibe force-pushed the dev/max42500 branch 3 times, most recently from 93c684e to 6560334 Compare March 7, 2025 07:40

kentlibe commented Mar 7, 2025

View reviewed changes

Comment thread drivers/watchdog/max42500-wdt.c

nunojsa reviewed Mar 7, 2025

View reviewed changes

kentlibe force-pushed the dev/max42500 branch from 6560334 to d006927 Compare March 8, 2025 16:47

kentlibe added 2 commits March 9, 2025 02:00

kentlibe force-pushed the dev/max42500 branch from d006927 to fcc0686 Compare March 8, 2025 18:02

kentlibe force-pushed the dev/max42500 branch 2 times, most recently from 5baf2de to 8772b88 Compare March 11, 2025 09:09

kentlibe added 2 commits March 11, 2025 17:44

Kconfig.adi: imply SENSORS_MAX42500

ff692c0

Add entry for the MAX42500 driver Signed-off-by: Kent Libetario <Kent.Libetario@analog.com>

kentlibe force-pushed the dev/max42500 branch from 8772b88 to ff692c0 Compare March 11, 2025 09:46

Conversation

kentlibe commented Dec 1, 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 Dec 2, 2024

Uh oh!

kentlibe commented Dec 17, 2024

Uh oh!

kister-jimenez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kentlibe commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nunojsa left a comment

Choose a reason for hiding this comment

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!

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!

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!

kentlibe commented Mar 4, 2025

Uh oh!

kentlibe commented Mar 5, 2025

Uh oh!

nunojsa commented Mar 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

kentlibe commented Dec 1, 2024 •

edited

Loading

kentlibe commented Mar 3, 2025 •

edited

Loading

nunojsa Mar 5, 2025 •

edited

Loading