Re: [PATCH v3 1/9] firmware: arm_scmi: Set fwnode for the generated SCMI platform device

[Sudeep Holla <sudeep.holla@kernel.org>]

On Mon, Aug 24, 2026 at 09:51:33AM -0700, Jonathan Cameron wrote:
> On Thu, 13 Aug 2026 12:32:56 +0100
> Sudeep Holla <sudeep.holla@kernel.org> wrote:
> 
> > 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.
> 
> Hi Sudeep,
> 

Hi Jonathan,

Thanks for all the reviews. I was away for a while and started to look at
addressing these now.

> When you say 'complement' I'd like to see a little more.  The guts
> of both calls includes setting dev->ofnode() adn the extra bit
> in device_set_of_node_from_dev() is about ensuring we drop existing
> node reference (there isn't one) and increment the reference for what
> is the parent of_node.
> 

Yes I agree. At the time, when I hacked this up, I was supposed to take
a look at it again which I never did.

> I may be wrong, but I think that is unnecessary and if I'm wrong I'd
> like to see more info here on why it is needed.
> 

No your concerns are valid. For reference-counted non-OF fwnodes like ACPI or
software node, platform_device_release() drops the fwnode reference,
but device_set_node() does not acquire one.

> Also, if this is commonly needed why doesn't device_set_node() include
> that dance?
> 

I think there are new helpers added recently in the tree, thinking of using
then. See below.

> Unless I fumbled the command, a grep suggests there are no instances
> in tree of these both being called near each other.
> 

I did that too before adding but somehow kept it for future investigation.


> Jonathan
> 
> > 
> > Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> > ---
> >  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 fe8c22cfb9f7..cc7d11c3c1f3 100644
> > --- a/drivers/firmware/arm_scmi/common.h
> > +++ b/drivers/firmware/arm_scmi/common.h
> > @@ -650,6 +650,7 @@ static int __tag##_probe(struct platform_device *pdev)			       \
> >  	}								       \
> >  									       \
> >  	device_set_of_node_from_dev(&spdev->dev, dev);			       \
> > +	device_set_node(&spdev->dev, dev_fwnode(dev));			       \

How about something like this instead of above 2 lines:

if (is_of_node(dev_fwnode(dev)))
	platform_device_set_of_node_from_dev(spdev, dev);
else
	platform_device_set_fwnode(spdev, dev_fwnode(dev));

-- 
Regards,
Sudeep