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

Sudeep Holla
2025-10-21T09:03:27Z

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@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?
>

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