Re: [PATCH v23 08/13] mfd: core: Add firmware-node support to MFD cells

[Shivendra Pratap]

On 21-07-2026 14:59, Bartosz Golaszewski wrote:
> On Tue, 14 Jul 2026 19:16:36 +0200, Shivendra Pratap
> <shivendra.pratap@xxxxxxxxxxxxxxxx> said:
> > MFD core has no way to register a child device using an explicit firmware
> > node. This prevents drivers from registering child nodes when those nodes
> > do not define a compatible string. One such example is the PSCI
> > "reboot-mode" node, which omits a compatible string as it describes
> > boot-states provided by the underlying firmware.
> >
> > Extend struct mfd_cell with a named firmware-node field to identify a
> > child node under the MFD parent. The node is added to the MFD child
> > device during registration when none is assigned by device tree, ACPI,
> > or software matching.
> >
> > Suggested-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Shivendra Pratap <shivendra.pratap@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/mfd/mfd-core.c   | 142 ++++++++++++++++++++++++++++++++++++++++-------
> >   include/linux/mfd/core.h |  10 ++++
> >   2 files changed, 131 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > index 7aa32b90cf1e..ba9682ead2ca 100644
> > --- a/drivers/mfd/mfd-core.c
> > +++ b/drivers/mfd/mfd-core.c
> > @@ -10,6 +10,7 @@
> >   #include <linux/kernel.h>
> >   #include <linux/platform_device.h>
> >   #include <linux/acpi.h>
> > +#include <linux/fwnode.h>
> >   #include <linux/list.h>
> >   #include <linux/property.h>
> >   #include <linux/mfd/core.h>
> > @@ -22,6 +23,7 @@
> >   #include <linux/regulator/consumer.h>
> >
> >   static LIST_HEAD(mfd_of_node_list);
> > +static LIST_HEAD(mfd_named_fwnode_list);
> >   static DEFINE_MUTEX(mfd_of_node_mutex);
> >
> >   struct mfd_of_node_entry {
> > @@ -30,10 +32,91 @@ struct mfd_of_node_entry {
> >   	struct device_node *np;
> >   };
> >
> > +struct mfd_named_fwnode_entry {
> > +	struct list_head list;
> > +	struct device *dev;
> > +	struct fwnode_handle *fwnode;
> > +};
> > +
> >   static const struct device_type mfd_dev_type = {
> >   	.name	= "mfd_device",
> >   };
> >
> > +static int mfd_claim_named_fwnode(struct platform_device *pdev,
> > +				  struct fwnode_handle *fwnode)
> > +{
> > +	struct mfd_named_fwnode_entry *entry, *iter;
> > +
> > +	entry = kzalloc_obj(*entry, GFP_KERNEL);
> > +	if (!entry)
> > +		return -ENOMEM;
> > +
> > +	entry->dev = &pdev->dev;
> > +	entry->fwnode = fwnode_handle_get(fwnode);
> > +
> > +	scoped_guard(mutex, &mfd_of_node_mutex) {
> > +		list_for_each_entry(iter, &mfd_named_fwnode_list, list)
> > +			if (iter->fwnode == fwnode) {
> > +				fwnode_handle_put(entry->fwnode);
> > +				kfree(entry);
> > +				return -EAGAIN;
> > +			}
> > +
> > +		list_add_tail(&entry->list, &mfd_named_fwnode_list);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Temporary MFD-local cleanup for named non-OF child fwnodes.
> > + * Remove/rework this when platform core starts owning and dropping
> > + * dev->fwnode references for these devices.
> > + */
> > +static void mfd_release_named_fwnode(struct platform_device *pdev)
> > +{
> > +	struct mfd_named_fwnode_entry *entry, *tmp;
> > +
> > +	scoped_guard(mutex, &mfd_of_node_mutex) {
> > +		list_for_each_entry_safe(entry, tmp, &mfd_named_fwnode_list, list)
> > +			if (entry->dev == &pdev->dev) {
> > +				if (dev_fwnode(&pdev->dev) == entry->fwnode)
> > +					device_set_node(&pdev->dev, NULL);
> > +				fwnode_handle_put(entry->fwnode);
> > +				list_del(&entry->list);
> > +				kfree(entry);
> > +			}
> > +	}
> > +}
> > +
> > +static int mfd_claim_of_node_to_dev(struct platform_device *pdev,
> > +				    struct device_node *np)
> > +{
> > +	struct mfd_of_node_entry *of_entry, *iter;
> > +
> > +	of_entry = kzalloc_obj(*of_entry, GFP_KERNEL);
> > +	if (!of_entry)
> > +		return -ENOMEM;
> > +
> > +	of_entry->dev = &pdev->dev;
> > +	of_entry->np = of_node_get(np);
> > +
> > +	/* Skip if OF node has previously been allocated to a device */
> > +	scoped_guard(mutex, &mfd_of_node_mutex) {
> > +		list_for_each_entry(iter, &mfd_of_node_list, list)
> > +			if (iter->np == np) {
> > +				of_node_put(of_entry->np);
> > +				kfree(of_entry);
> > +				return -EAGAIN;
> > +			}
> > +
> > +		list_add_tail(&of_entry->list, &mfd_of_node_list);
> > +	}
> > +
> > +	device_set_node(&pdev->dev, of_fwnode_handle(np));
> > +	return 0;
> > +}
> > +
> >   #if IS_ENABLED(CONFIG_ACPI)
> >   struct match_ids_walk_data {
> >   	struct acpi_device_id *ids;
> > @@ -111,19 +194,11 @@ static int mfd_match_of_node_to_dev(struct platform_device *pdev,
> >   				    struct device_node *np,
> >   				    const struct mfd_cell *cell)
> >   {
> > -	struct mfd_of_node_entry *of_entry;
> >   	u64 of_node_addr;
> >
> > -	/* Skip if OF node has previously been allocated to a device */
> > -	scoped_guard(mutex, &mfd_of_node_mutex) {
> > -		list_for_each_entry(of_entry, &mfd_of_node_list, list)
> > -			if (of_entry->np == np)
> > -				return -EAGAIN;
> > -	}
> > -
> >   	if (!cell->use_of_reg)
> >   		/* No of_reg defined - allocate first free compatible match */
> > -		goto allocate_of_node;
> > +		return mfd_claim_of_node_to_dev(pdev, np);
> >
> >   	/* We only care about each node's first defined address */
> >   	if (of_property_read_reg(np, 0, &of_node_addr, NULL))
> > @@ -134,18 +209,7 @@ static int mfd_match_of_node_to_dev(struct platform_device *pdev,
> >   		/* No match */
> >   		return -EAGAIN;
> >
> > -allocate_of_node:
> > -	of_entry = kzalloc(sizeof(*of_entry), GFP_KERNEL);
> > -	if (!of_entry)
> > -		return -ENOMEM;
> > -
> > -	of_entry->dev = &pdev->dev;
> > -	of_entry->np = of_node_get(np);
> > -	scoped_guard(mutex, &mfd_of_node_mutex)
> > -		list_add_tail(&of_entry->list, &mfd_of_node_list);
> > -
> > -	device_set_node(&pdev->dev, of_fwnode_handle(np));
> > -	return 0;
> > +	return mfd_claim_of_node_to_dev(pdev, np);
> >   }
> >
> >   static int mfd_add_device(struct device *parent, int id,
> > @@ -156,6 +220,7 @@ static int mfd_add_device(struct device *parent, int id,
> >   	struct resource *res;
> >   	struct platform_device *pdev;
> >   	struct mfd_of_node_entry *of_entry, *tmp;
> > +	struct fwnode_handle *fwnode;
> >   	bool disabled = false;
> >   	int ret = -ENOMEM;
> >   	int platform_id;
> > @@ -224,6 +289,37 @@ static int mfd_add_device(struct device *parent, int id,
> >
> >   	mfd_acpi_add_device(cell, pdev);
> >
> > +	/* named_fwnode is a fallback only when no OF/ACPI match and no swnode */
> > +	if (!pdev->dev.fwnode && !cell->swnode && cell->named_fwnode) {
>
> Use dev_fwnode() instead of pdev->dev.fwnode.

sure will update. thanks.

> > +		struct device_node *named_np;
> > +
> > +		fwnode = device_get_named_child_node(parent, cell->named_fwnode);
> > +		if (!fwnode) {
> > +			ret = -ENODEV;
> > +			goto fail_alias;
> > +		}
> > +
> > +		named_np = to_of_node(fwnode);
> > +		if (named_np) {
> > +			ret = mfd_claim_of_node_to_dev(pdev, named_np);
> > +			fwnode_handle_put(fwnode);
> > +			if (ret == -EAGAIN)
> > +				ret = -EBUSY;
> > +			if (ret)
> > +				goto fail_alias;
> > +		} else {
> > +			ret = mfd_claim_named_fwnode(pdev, fwnode);
> > +			if (ret) {
> > +				fwnode_handle_put(fwnode);
> > +				if (ret == -EAGAIN)
> > +					ret = -EBUSY;
> > +				goto fail_alias;
> > +			}
> > +			device_set_node(&pdev->dev, fwnode);
> > +			fwnode_handle_put(fwnode);
> > +		}
>
> What is the reason to have this split into OF and fwnode functions? I don't
> really see why you would need to do it twice, isn't fwnode sufficient for the
> OF node case too?

We do a duplicate check here before adding the named_fwnode and split it 
for two reasons:
1. of_node list pre-exists. We add a new list of fwnode. of_node based 
duplicate check go wrong, if its checked with the fwnode list.
2. of_node need to go to its respective list as fwnode list, release 
path, is different for us.

> > +	}
> > +
> >   	if (cell->pdata_size) {
> >   		ret = platform_device_add_data(pdev,
> >   					cell->platform_data, cell->pdata_size);
> > @@ -295,6 +391,7 @@ static int mfd_add_device(struct device *parent, int id,
> >   	if (cell->swnode)
> >   		device_remove_software_node(&pdev->dev);
> >   fail_of_entry:
> > +	mfd_release_named_fwnode(pdev);
> >   	scoped_guard(mutex, &mfd_of_node_mutex) {
> >   		list_for_each_entry_safe(of_entry, tmp, &mfd_of_node_list, list)
> >   			if (of_entry->dev == &pdev->dev) {
> > @@ -382,7 +479,10 @@ static int mfd_remove_devices_fn(struct device *dev, void *data)
> >   	regulator_bulk_unregister_supply_alias(dev, cell->parent_supplies,
> >   					       cell->num_parent_supplies);
> >
> > +	get_device(&pdev->dev);
> >   	platform_device_unregister(pdev);
> > +	mfd_release_named_fwnode(pdev);
> > +	put_device(&pdev->dev);
> >   	return 0;
> >   }
> >
> > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> > index faeea7abd688..8daa83dd31ea 100644
> > --- a/include/linux/mfd/core.h
> > +++ b/include/linux/mfd/core.h
> > @@ -80,6 +80,16 @@ struct mfd_cell {
> >
> >   	/* Software node for the device. */
> >   	const struct software_node *swnode;
> > +	/*
> > +	 * Name of a child firmware node under the MFD parent device.
> > +	 *
> > +	 * Used only as a fallback when no firmware node is assigned to MFD
> > +	 * child and no software node is provided.
> > +	 *
> > +	 * For Device Tree parents, lookup is by base node name only
> > +	 * (the part before '@'). Unit-addresses are not matched.
> > +	 */
>
> Side note: struct mfd_cell could really use a proper kernel doc but that's out
> of scope of this series.
>
> > +	const char *named_fwnode;
> >
> >   	/*
> >   	 * Device Tree compatible string
> >
> > --
> > 2.34.1
>
> Bartosz

thanks,
Shivendra