Re: [PATCH 1/8] firmware: arm_scmi: Set fwnode for the genrated SCMI platform device

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Fri, 17 Oct 2025 14:23:44 +0100
Sudeep Holla <sudeep.holla@arm.com> wrote:

generated (in patch title).

> Add a call to device_set_node() in the SCMI probe helper to associate
> generated SCMI platform device with the firmware node of its supplier
> transport device.
> 
> This complements device_set_of_node_from_dev() and ensures that
> firmware node information is propagated correctly for both Device Tree
> and non-DT (e.g. ACPI) based systems.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/common.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index 07b9e629276d..911941e6885d 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -473,6 +473,7 @@ static int __tag##_probe(struct platform_device *pdev)			       \
>  		return -ENOMEM;						       \
>  									       \
>  	device_set_of_node_from_dev(&spdev->dev, dev);			       \
> +	device_set_node(&spdev->dev, dev_fwnode(dev));			       \

device_set_node() is supposed to handle both dt and ACPI. So this surprised me
and I went digging.

dev_fnode for acpi is dev->fwnode, for of it is of_fwnode_handle(dev->of_node)
Which is dev->of_node->fwnode

So for acpi this is
	device_set_node(&spdev->dev, dev->fwnode);
which is:
	spdev->dev->fwnode = fwnode;
	spdev->dev->of_node = NULL; 

For dt
	device_set_node(&spdev->dev, dev->of_node->fwnode);
which is
	spdev->dev->fwnode = dev->of_node->fwnode;
	spdev->dev->opf-node = dev->of_node; (via some container of magic)


The device_set_of_node_from_dev(&spdev->dev, dev)
is same as:
	of_node_put(spdev->dev->of_node);
	spdev->dev->of_node = of_node_get(dev->of_node);
	spdev->dev->of_node_reused = true;

So subject to some reference counting that I don't think you need as the
spdev->dev parent is the dev here, the first call does nothing extra.

Maybe I missed something?

Jonathan




>  									       \
>  	strans.supplier = dev;						       \
>  	memcpy(&strans.desc, &(__desc), sizeof(strans.desc));		       \
>