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

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Wed, 2 Sep 2026 08:51:10 +0100
Sudeep Holla <sudeep.holla@kernel.org> wrote:

> 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));
> 

To me, if the reference counting is useful (and I'm a bit doubtful about that)
platform_device_set_fwnode() should be doing it. +CC those who might have
more insight on this.

Jonathan