Re: [PATCH v4 1/4] mfd: wm831x: Support software nodes for DCDC regulators

[Lee Jones]

On Wed, 04 Nov 2026, Dmitry Torokhov wrote:

> Hi Lee,
> 
> On Wed, Nov 04, 2026 at 06:14:29PM +0000, Lee Jones wrote:
> > On Sat, 24 Oct 2026, Dmitry Torokhov wrote:
> > > +static int wm831x_add_devices(struct wm831x *wm831x, int wm831x_num,
> > > +			      const struct mfd_cell *cells, int num_cells)
> > > +{
> > > +	struct wm831x_pdata *pdata = &wm831x->pdata;
> > > +	int i;
> > > +
> > > +	struct mfd_cell *local_cells __free(kfree) =
> > > +		kmemdup_array(cells, num_cells, sizeof(*cells), GFP_KERNEL);
> > > +	if (!local_cells)
> > > +		return -ENOMEM;
> > > +
> > > +	for (i = 0; i < num_cells; i++) {
> > > +		struct mfd_cell *cell = &local_cells[i];
> > > +
> > > +		if (strcmp(cell->name, "wm831x-buckv") == 0) {
> > > +			int id = cell->id - 1;
> > > +
> > > +			if (id >= 0 && id < WM831X_MAX_DCDC)
> > > +				cell->swnode = pdata->dcdc_swnodes[id];
> > > +		}
> > > +	}
> >
> > I can't begin to tell you how much I hate this.
> >
> > Why are we duplicating massive static const structs just to add a S/W
> > node?  Is there no other way we can do this statically?
> 
> The software node itself is defined statically in the board file
> (crag_dcdc1_swnode in arch/arm/mach-s3c/mach-crag6410.c in patch 2/4),
> as it describes board-specific and PMIC-instance-specific wiring -
> specifically the host SoC GPIO wired to the PMIC's DVS pin (GPK(0) on
> Cragganmore 6410).
> 
> Because wm831x-core.c is a generic PMIC driver that can be instantiated
> on different boards or even multiple times on the same board (for
> example, Cragganmore 6410 has two WM831x PMIC instances, only one of
> which has a DVS GPIO wired to DCDC1), the shared static mfd_cell arrays
> in wm831x-core.c cannot point to a board's software node at compile
> time.
> 
> Note that local_cells is annotated with __free(kfree), so the duplicate
> array only lives for the duration of the mfd_add_devices() call (which
> makes its own per-device copy of each mfd_cell anyway) and is freed
> immediately before wm831x_add_devices() returns.
> 
> > Since pdata is being populated already, why not just pass that on to the
> > consuming child device (the regulator)?
> 
> The regulator driver (wm831x_buckv_dvs_init() in
> drivers/regulator/wm831x-dcdc.c) acquires the DVS GPIO using
> devm_gpiod_get(&pdev->dev, "dvs", ...). When converting the board file
> away from legacy gpiod_lookup_table to software nodes (to drop the
> fragile MFD device name matching and fix the GPIO chip label lookup),
> gpiolib resolves the "dvs-gpios" property against the consumer device's
> fwnode (&pdev->dev), so the software node needs to be attached to the
> wm831x-buckv platform device.
> 
> In v2 of this series I actually tried passing the software_node pointer
> through wm831x_buckv_pdata and attaching it to &pdev->dev inside the
> regulator driver's probe() [1]. However, attaching a software node to
> an already-registered platform device inside probe() is awkward and
> requires manual teardown in the child driver, whereas mfd-core already
> has built-in support for cell->swnode in mfd_add_device() (attaching
> the node via device_add_software_node() before platform_device_add(),
> and cleaning it up via device_remove_software_node() on error or
> removal). Populating cell->swnode lets mfd-core manage the software
> node lifecycle cleanly and keeps the regulator driver untouched.

Why do you need to attach the software_node to &pdev->dev at all?

All of the other DVS configuration (dvs_control_src, dvs_init_state) is
already passed to wm831x_buckv_dvs_init() via pdata.  If you put the
software node pointer in 'wm831x_buckv_pdata' (with the rest of the DVS
parameters, where it belongs), wm831x_buckv_dvs_init() can simply use
devm_fwnode_gpiod_get() to fetch it.

This then requires no device_add_software_node(), no manual tear-down in
the regulator driver and no "middle-man" changes to MFD at all.

> If you prefer not to kmemdup the full cell table, another option would
> be to split the buckv cells out of the main wm831x cell arrays and
> register them via a separate mfd_add_devices() call, though that seemed
> more intrusive across wm8310/wm8311/wm8312/wm8320. Please let me know
> what you would prefer here.

If there is a good reason why the above cannot be done in the regulator
driver (which I doubt), then yes, factoring the two identical
wm831x-buckv cells out of wm8310_devs/wm8311_devs/wm8312_devs/wm8320_devs
(just like touch_devs, rtc_devs and backlight_devs already are) is still
far better than kmemdup'ing the static const tables and matching cell
names with strcmp() in a loop.

> [1] https://lore.kernel.org/all/20260708-samsung-kp-v2-7-3c6ed4c9b3b6@xxxxxxxxx/

[2] Code talks, BS walks  =:-)

diff --git a/arch/arm/mach-s3c/mach-crag6410.c b/arch/arm/mach-s3c/mach-crag6410.c
index 2264beef6dda..9657d74ca0c5 100644
--- a/arch/arm/mach-s3c/mach-crag6410.c
+++ b/arch/arm/mach-s3c/mach-crag6410.c
@@ -456,9 +456,21 @@ static struct pca953x_platform_data crag6410_pca_data = {
 	.irq_base	= -1,
 };
 
+static const struct property_entry crag_dcdc1_properties[] = {
+	PROPERTY_ENTRY_GPIO("dvs-gpios", SAMSUNG_GPIO_NODE('K'), 0, GPIO_ACTIVE_HIGH),
+	{ }
+};
+
+static const struct software_node crag_dcdc1_swnode = {
+	.name = "wm831x-buckv-dcdc1",
+	.properties = crag_dcdc1_properties,
+};
+
 /* VDDARM is controlled by DVS1 connected to GPK(0) */
 static struct wm831x_buckv_pdata vddarm_pdata = {
 	.dvs_control_src = 1,
+	.swnode = &crag_dcdc1_swnode,
 };
 
 static struct regulator_consumer_supply vddarm_consumers[] = {
@@ -656,24 +668,6 @@ static struct wm831x_pdata crag_pmic_pdata = {
 	.touch = &touch_pdata,
 };
 
-/*
- * VDDARM is eventually ending up as a regulator hanging on the MFD cell device
- * "wm831x-buckv.1" spawn from drivers/mfd/wm831x-core.c.
- *
- * From the note on the platform data we can see that this is clearly DVS1
- * and assigned as dcdc1 resource to the MFD core which sets .id of the cell
- * spawning the DVS1 platform device to 1, then the cell platform device
- * name is calculated from 10*instance + id resulting in the device name
- * "wm831x-buckv.11"
- */
-static struct gpiod_lookup_table crag_pmic_gpiod_table = {
-	.dev_id = "wm831x-buckv.11",
-	.table = {
-		GPIO_LOOKUP("GPIOK", 0, "dvs", GPIO_ACTIVE_HIGH),
-		{ },
-	},
-};
-
 static struct i2c_board_info i2c_devs0[] = {
 	{ I2C_BOARD_INFO("24c08", 0x50), },
 	{ I2C_BOARD_INFO("tca6408", 0x20),
@@ -931,7 +925,7 @@ static void __init crag6410_machine_init(void)
 	s3c_fb_set_platdata(&crag6410_lcd_pdata);
 	dwc2_hsotg_set_platdata(&crag6410_hsotg_pdata);
 
-	gpiod_add_lookup_table(&crag_pmic_gpiod_table);
+	software_node_register(&crag_dcdc1_swnode);
 	i2c_register_board_info(0, i2c_devs0, ARRAY_SIZE(i2c_devs0));
 	gpiod_add_lookup_table(&crag_wm1250_ev1_gpiod_table);
 	i2c_register_board_info(1, i2c_devs1, ARRAY_SIZE(i2c_devs1));
diff --git a/drivers/regulator/wm831x-dcdc.c b/drivers/regulator/wm831x-dcdc.c
index 834d7c181971..957da3195974 100644
--- a/drivers/regulator/wm831x-dcdc.c
+++ b/drivers/regulator/wm831x-dcdc.c
@@ -16,6 +16,7 @@
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 #include <linux/gpio/consumer.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 
 #include <linux/mfd/wm831x/core.h>
@@ -330,6 +331,8 @@ static void wm831x_buckv_dvs_init(struct platform_device *pdev,
 				 struct wm831x_buckv_pdata *pdata)
 {
 	struct wm831x *wm831x = dcdc->wm831x;
+	struct fwnode_handle *fwnode;
+	enum gpiod_flags flags;
 	int ret;
 	u16 ctrl;
 
@@ -341,8 +344,15 @@ static void wm831x_buckv_dvs_init(struct platform_device *pdev,
 	*/
 	dcdc->dvs_gpio_state = pdata->dvs_init_state;
 
-	dcdc->dvs_gpiod = devm_gpiod_get(&pdev->dev, "dvs",
-			dcdc->dvs_gpio_state ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
+	flags = dcdc->dvs_gpio_state ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
+	if (pdata->swnode) {
+		fwnode = software_node_fwnode(pdata->swnode);
+		dcdc->dvs_gpiod = devm_fwnode_gpiod_get(&pdev->dev, fwnode,
+						"dvs", flags, dcdc->name);
+	} else {
+		dcdc->dvs_gpiod = devm_gpiod_get(&pdev->dev, "dvs", flags);
+	}
+
 	if (IS_ERR(dcdc->dvs_gpiod)) {
 		dev_err(wm831x->dev, "Failed to get %s DVS GPIO: %ld\n",
 			dcdc->name, PTR_ERR(dcdc->dvs_gpiod));
diff --git a/include/linux/mfd/wm831x/pdata.h b/include/linux/mfd/wm831x/pdata.h
index 75aa94dadf1c..49501f155cc5 100644
--- a/include/linux/mfd/wm831x/pdata.h
+++ b/include/linux/mfd/wm831x/pdata.h
@@ -12,6 +12,7 @@
 
 struct wm831x;
 struct regulator_init_data;
+struct software_node;
 
 struct wm831x_backlight_pdata {
 	int isink;     /** ISINK to use, 1 or 2 */
@@ -50,6 +51,7 @@ struct wm831x_buckv_pdata {
 	int dvs_control_src; /** Hardware DVS source to use (1 or 2) */
 	int dvs_init_state;  /** DVS state to expect on startup */
 	int dvs_state_gpio;  /** CPU GPIO to use for monitoring status */
+	const struct software_node *swnode; /** Software node for DVS GPIOs */
 };
 
 /* Sources for status LED configuration.  Values are register values

-- 
Lee Jones