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

["Arnd Bergmann" <arnd@arndb.de>]

On Thu, Nov 5, 2026, at 09:53, Lee Jones wrote:
> On Wed, 04 Nov 2026, Dmitry Torokhov wrote:

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

I think realistically this driver is not being shared across boards
and the documented but unused DT binding in
Documentation/devicetree/bindings/mfd/wm831x.txt does not describe
the gpio or actually match what the driver does.

I'm still not sure what your goal is with this series, but I feel
like there is no point in over-designing something that is not
going to matter anyway and that would be replaced with a different
DT binding if any new users should show up.

Why can't we just put the gpio property in the wm831x node itself
for simplicity?

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

I like this better than Dmitry's version, but it still feels like
adding complexity without a clear direction of how this should be
done for a complete driver.

> +
> +static const struct software_node crag_dcdc1_swnode = {
> +	.name = "wm831x-buckv-dcdc1",
> +	.properties = crag_dcdc1_properties,
> +};

specifically, this still adds a swnode with properties that don't
match either the documented binding or a potentially better one,
it's just another ad-hoc way of passing the gpio line.

> @@ -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);
> +	}
> +

and this now has two ways of getting the gpio line, only one of which is
actually used, and neither of them matches the DT binding.

Instead of the piecemeal conversion, I'd prefer if we could either
do the entire wm831x driver properly or only do the absolute minimum:

- if there is a reason to expect any future user of the driver,
  extend the wlf,arizona.yaml binding to cover wm831x as well,
  replace all the platform data with swnode properties according
  to that binding and have the drivers use those.

- If crag6410 is the only thing that is ever going to use
  any of it, stop worrying about doing it cleanly. We can probably
  just stick a gpio_desc pointer in the platform_data and be
  done with removing the lookup table.

As far as I understand the situation, wm831x was used on the
reference board (Samsung smdk6410, removed in 2019) in addition
to crag6410 but never anywhere else upstream. The question
then is whether we'll ever see a DT conversion of crag6410.
This would require finding someone willing to actually do
the work, and coming up with a way to handle the autodetection
in mach-crag6410-module.c so the correct spi/i2c codec
child devices are probed.

      Arnd