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

[Sudeep Holla]

On Mon, Oct 20, 2025 at 06:07:02PM +0100, Jonathan Cameron wrote:
> On Fri, 17 Oct 2025 14:23:44 +0100
> Sudeep Holla <sudeep.holla at 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 at 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?
> 

No, you didn’t miss anything. You’ve articulated my analysis well - the same
points I had intended to include in my commit messages but had forgotten those
details of at the time of writing the commit message.

I retained the call to device_set_of_node_from_dev() primarily because it sets
dev->of_node_reused = true.

I’m not entirely certain about the implications of not setting this flag, and
it wouldn’t be set if we used only device_set_node().

Thank you for noticing this and bringing it up for discussion. I had planned
to raise this when I first observed it and made the change, but unfortunately
didn’t document it properly - that would have saved you from having to
rediscover the details yourself.

I’m fine with whichever solution we agree on. I retained both calls
intentionally to prompt exactly this kind of discussion.

-- 
Regards,
Sudeep