Re: [PATCH v4 02/20] mfd: adp5585: only add devices given in FW

On Fri, 2025-05-23 at 16:19 +0100, Lee Jones wrote:

On Fri, 23 May 2025, Nuno Sá wrote:

quoted

On Fri, 2025-05-23 at 15:51 +0100, Lee Jones wrote:

quoted

On Wed, 21 May 2025, Nuno Sá via B4 Relay wrote:

quoted

From: Nuno Sá <nuno.sa@analog.com>

Not all devices (features) of the adp5585 device are mandatory to be
used in all platforms. Hence, check what's given in FW and dynamically
create the mfd_cell array to be given to devm_mfd_add_devices().

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/mfd/adp5585.c | 48 +++++++++++++++++++++++++++++++++++++++-----
----
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c

index
160e0b38106a6d78f7d4b7c866cb603d96ea673e..806867c56d6fb4ef1f461af26a424a
3a05
f46575 100644

--- a/drivers/mfd/adp5585.c
+++ b/drivers/mfd/adp5585.c

@@ -17,7 +17,13 @@

 #include <linux/regmap.h>
 #include <linux/types.h>
 
-static const struct mfd_cell adp5585_devs[] = {
+enum {
+	ADP5585_DEV_GPIO,
+	ADP5585_DEV_PWM,
+	ADP5585_DEV_MAX
+};
+
+static const struct mfd_cell adp5585_devs[ADP5585_DEV_MAX] = {
 	{ .name = "adp5585-gpio", },
 	{ .name = "adp5585-pwm", },
 };

@@ -110,6 +116,37 @@ static const struct regmap_config

adp5585_regmap_configs[] = {
 	},
 };
 
+static void adp5585_remove_devices(void *dev)
+{
+	mfd_remove_devices(dev);
+}
+
+static int adp5585_add_devices(struct device *dev)
+{
+	int ret;
+
+	if (device_property_present(dev, "#pwm-cells")) {
+		ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
+				      &adp5585_devs[ADP5585_DEV_PWM],
1,
NULL, 0, NULL);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to add
pwm
device\n");

PWM is an acronym, it should be capitalised.

quoted

+	}
+
+	if (device_property_present(dev, "#gpio-cells")) {
+		ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
+				      &adp5585_devs[ADP5585_DEV_GPIO],
1,
NULL, 0, NULL);
+		if (ret) {
+			ret = dev_err_probe(dev, ret, "Failed to add
gpio
device\n");

Same with GPIO.

quoted

+			goto out_error;
+		}
+	}
+
+	return devm_add_action_or_reset(dev, adp5585_remove_devices,
dev);

We have 2 of these now.

Why do we need lots of unbinding functions?

What's wrong .remove() or devm_*()?

I do mention in the cover why I did not used devm_mfd_add_devices(). We
would be
adding an action per device and mfd_remove_devices() removes all of them in
one
call. Not that is an issue (I believe subsequent calls with be kind of no-
ops)
but this way felt more correct.

I haven't seen another device add a .remove() equivalent per device.

Why do you need it?  What's the use-case where this would become critical?

No sure I'm following you. I don't need a .remove() per device (or it is not
critical to have one). I just went with this because devm_mfd_add_devices()
would be adding more devres_add() than what we need given that
mfd_remove_devices() removes all child devices at once. So, logically, the above
makes sense to me. Now, I'm ok if you say, don't bother with this and just use 
devm_mfd_add_devices() on every device we want to add.

- Nuno Sá