Staging/max77840 device driver #2815

joan-na-good · 2025-06-08T10:01:16Z

PR Description

This PR introduces initial support for the MAX77840 PMIC device driver.

The changes include:

MFD (Multi-Function Device) parent driver
Charger, Regulator, and Fuel Gauge sub-drivers
YAML-based Device Tree bindings with validation

All code has been built successfully, passed checkpatch.pl in strict mode. No external dependencies are required.

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)

drivers/mfd/Kconfig

Add device tree bindings to support the MAX77840 PMIC and its submodules, enabling configuration and control through device tree. Signed-off-by: joan-na-good <joan.na@analog.com>

Implement basic support for the MAX77840 PMIC core, including initialization, power management, and core functions necessary for submodules. Signed-off-by: joan-na-good <joan.na@analog.com>

…uel gauge This patch adds driver support for the MAX77840 charger, charger-detect, and fuel gauge functionalities, enabling proper battery management and monitoring on supported platforms. Signed-off-by: joan-na-good <joan.na@analog.com>

Add regulator support for the MAX77840 PMIC to control voltage and current regulation. This enables proper power management features required for various submodules dependent on this regulator. Signed-off-by: joan-na-good <joan.na@analog.com>

nunojsa

Some general notes from me but this needs a lot of rework before I can do a proper review. This seems like a very old, out of tree, driver which is using a lot of questionable things. Please look at another upstream examples in:

https://elixir.bootlin.com/linux/v6.15/source/drivers/mfd

and rework the drivers so they are modern. Otherwise it gets very time consuming (and difficult) for me to review this.

Also bear in mind that the goal is to send this upstream and get it accepted before merging it in our tree.

nunojsa · 2025-06-09T14:42:47Z

drivers/power/supply/Makefile

@@ -77,6 +77,7 @@ obj-$(CONFIG_CHARGER_MAX14577)	+= max14577_charger.o
 obj-$(CONFIG_CHARGER_DETECTOR_MAX14656)	+= max14656_charger_detector.o
 obj-$(CONFIG_CHARGER_MAX77650)	+= max77650-charger.o
 obj-$(CONFIG_CHARGER_MAX77693)	+= max77693_charger.o
+obj-$(CONFIG_CHARGER_MAX77840)	+= max77840-charger.o max77840-charger-detect.o max77840-battery.o


This adds three different drivers all as part of the supply subsystem? Can't we have just one driver?

The MAX77840 is a single chip, it integrates multiple functional blocks — charger, regulator, and fuel gauge.
Therefore, if possible, I’d prefer to maintain the current structure.

The MAX77840 is a single chip, it integrates multiple functional blocks — charger, regulator, and fuel gauge.
Therefore, if possible, I’d prefer to maintain the current structure.

Yes, but it is three drivers in the same subsystem... Anyways, if the maintainer accepts this, fine by me. But at least for this review add a driver per commit and not three drivers in one single commit.

nunojsa · 2025-06-09T14:43:36Z

include/linux/power/max77840-charger.h

+	int restart_threshold;
+	int termination_voltage;
+	int input_current_limit;
+};


Drop the above header files if they are only used by the respective source file.

nunojsa · 2025-06-09T14:44:03Z

include/linux/power/max77840-charger.h

+	CHG_INT_CHG_I,
+	CHG_INT_TOPOFF_I,
+	CHG_INT_CHGIN_I,
+	CHG_INT_AICL_CHGINI_I,


Typically, all the defines, enum, etc, are prefixed with the driver name

nunojsa · 2025-06-09T14:44:58Z

drivers/power/supply/max77840-battery.c

+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020 Maxim Integrated Products, Inc.
+ * Copyright (C) 2024 Analog Devices, Inc.


I guess 2025 will also be needed

nunojsa · 2025-06-09T14:45:23Z

include/linux/mfd/max77840.h

+#ifndef __MAX77840_MFD_H__
+#define __MAX77840_MFD_H__
+
+#define __TEST_DEVICE_NODE__


nunojsa · 2025-06-09T14:48:27Z

drivers/mfd/max77840.c

+#define I2C_ADDR_FUEL_GAUGE     (0x6C >> 1) /* Fuel Gauge */
+
+#define __lock(_me)    mutex_lock(&(_me)->lock)
+#define __unlock(_me)  mutex_unlock(&(_me)->lock)


drop the above lock defines. In fact, look at the cleanup.h header

nunojsa · 2025-06-09T14:48:45Z

drivers/mfd/max77840.c

+{
+	return regmap_bulk_write(regmap, (unsigned int)addr, src, (size_t)len);
+}
+EXPORT_SYMBOL(max77840_bulk_write);


drop the above

nunojsa · 2025-06-09T14:49:05Z

drivers/mfd/max77840.c

+/* PMIC */
+#define REG_PMICID			0x00
+
+/* Declare Interrupt */


Please drop all the unneeded comments

nunojsa · 2025-06-09T14:49:24Z

drivers/mfd/max77840.c

+		pr_err("<%s> failed to set up interrupt\n",
+		       client->name);
+		pr_err("<%s> and add sub-device [%d]\n",
+		       client->name, rc);


no pr_err(). Use dev_err()

nunojsa · 2025-06-09T14:49:48Z

drivers/mfd/max77840.c

+}
+
+module_init(max77840_init);
+module_exit(max77840_exit);


The above is not how it should be done

joan-na-good · 2025-06-10T01:53:06Z

Some general notes from me but this needs a lot of rework before I can do a proper review. This seems like a very old, out of tree, driver which is using a lot of questionable things. Please look at another upstream examples in:

https://elixir.bootlin.com/linux/v6.15/source/drivers/mfd

and rework the drivers so they are modern. Otherwise it gets very time consuming (and difficult) for me to review this.

Also bear in mind that the goal is to send this upstream and get it accepted before merging it in our tree.

Thank you for the feedback.
Sorry, I was not aware that the driver needs to be upstreamed and approved before it can be merged into the source tree.
I will review the examples you shared and revise the patch to meet the current kernel standards.
And I will proceed with upstreaming and obtaining the necessary approval.

gastmaier · 2025-06-10T07:02:40Z

@nuno should a mirror for git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git (for-mfd-next) be added to the cron.yml?
So Joan can open a draft pr against it to have all the implemented checking?

I did a manual at mirror/lee/mfd/for-mfd-next meanwhile

nunojsa · 2025-06-11T08:53:50Z

Sorry, I was not aware that the driver needs to be upstreamed and approved before it can be merged into the source tree.
I will review the examples you shared and revise the patch to meet the current kernel standards.
And I will proceed with upstreaming and obtaining the necessary approval.

Yes, the background story is that 99.9% of the times the driver will change after being upstreamed (due to review comments) and then we would need to backport those changes to sync the driver with upstream. Or, in some other cases, we would just forget to sync and then I would get tons of conflicts while updating to a new kernel version. This way, it makes it easier.

joan-na-good requested review from nunojsa, mhennerich, dbogdan, btogorean, chegbeli, buha, amiclaus, rbolboac, danmois, CiprianRegus and machschmitt as code owners June 8, 2025 10:01

joan-na-good force-pushed the staging/max77840-device-driver branch 8 times, most recently from 2522e7e to ac38438 Compare June 9, 2025 12:41

gastmaier requested changes Jun 9, 2025

View reviewed changes

drivers/mfd/Kconfig Outdated Show resolved Hide resolved

joan-na-good force-pushed the staging/max77840-device-driver branch from ac38438 to 2e29b07 Compare June 9, 2025 13:45

joan-na-good added 4 commits June 9, 2025 23:03

dt-bindings: mfd: Add bindings for MAX77840 PMIC and submodules

66628d2

Add device tree bindings to support the MAX77840 PMIC and its submodules, enabling configuration and control through device tree. Signed-off-by: joan-na-good <joan.na@analog.com>

mfd: max77840: Add support for MAX77840 PMIC core

c323c8b

Implement basic support for the MAX77840 PMIC core, including initialization, power management, and core functions necessary for submodules. Signed-off-by: joan-na-good <joan.na@analog.com>

joan-na-good force-pushed the staging/max77840-device-driver branch from 2e29b07 to 8cf13d6 Compare June 9, 2025 14:03

nunojsa requested changes Jun 9, 2025

View reviewed changes

Staging/max77840 device driver #2815

Are you sure you want to change the base?

Staging/max77840 device driver #2815

Uh oh!

Conversation

joan-na-good commented Jun 8, 2025

PR Description

PR Type

PR Checklist

Uh oh!

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!

joan-na-good commented Jun 10, 2025

Uh oh!

gastmaier commented Jun 10, 2025

Uh oh!

nunojsa commented Jun 11, 2025

Uh oh!

Uh oh!