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

[Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>]

On 21-07-2026 14:59, Bartosz Golaszewski wrote:
> On Tue, 14 Jul 2026 19:16:36 +0200, Shivendra Pratap
> <shivendra.pratap@oss.qualcomm.com> 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@oss.qualcomm.com>
>> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
>> ---
>>   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