Re: [PATCH 1/2] driver core: provide device_match_fwnode_ext()

[Andy Shevchenko]

On Fri, Feb 20, 2026 at 03:35:28PM +0100, Bartosz Golaszewski wrote:
> On Fri, Feb 20, 2026 at 1:08 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> > On Fri, Feb 20, 2026 at 12:25 PM Bartosz Golaszewski <brgl@xxxxxxxxxx> wrote:
> > > On Fri, 20 Feb 2026 08:36:58 +0100, Andy Shevchenko
> > > <andriy.shevchenko@xxxxxxxxxxxxxxx> said:
> > > > On Thu, Feb 19, 2026 at 04:21:59PM -0500, Bartosz Golaszewski wrote:
> > > >> On Thu, 19 Feb 2026 20:58:32 +0100, Andy Shevchenko
> > > >> <andriy.shevchenko@xxxxxxxxxxxxxxx> said:
> > > >> > On Thu, Feb 19, 2026 at 05:31:22PM +0100, Bartosz Golaszewski wrote:
> > > >> >> Provide an extended variant of device_match_fwnode() that also tries to
> > > >> >> match the device's secondary fwnode.

...

> > > >> >> +int device_match_fwnode_ext(struct device *dev, const void *fwnode)
> > > >> >> +{
> > > >> >> + struct fwnode_handle *dev_node = dev_fwnode(dev);
> > > >> >
> > > >> >> + if (!fwnode)
> > > >> >
> > > >> > IS_ERR_OR_NULL()
> > > >> > If supplied @fwnode is secondary, it might be an error pointer.
> > > >>
> > > >> I mirrored existing device_match_fwnode(), should it be fixed too?
> > > >
> > > > The answer is "I don't know". Strictly speaking this should be done everywhere
> > > > in the generic code when we can't guarantee that fwnode that comes to the
> > > > function is pure NULL or valid one.
> > > >
> > > >> >> +         return 0;
> > > >> >
> > > >> >> + if (dev_node == fwnode)
> > > >> >> +         return 1;
> > > >> >> +
> > > >> >> + return fwnode_is_primary(dev_node) && dev_node->secondary == fwnode;
> > > >> >> +}
> > > >> >
> > > >> > I think we can refactor this.
> > > >> >
> > > >> >    struct fwnode_handle *node;
> > > >> >
> > > >> > // I would name it like this, because in 3 cases in drivers/base/property.c
> > > >> > // 2 with node and 1 with dev_node when the same API is called.
> > > >>
> > > >> Haystack's node is "node" and the needle is "fwnode"? Seems confusing to me.
> > > >
> > > > But we need some consistency. drivers/base/property.c is inconsistent to begin
> > > > with and here the code chose the least used one for unknown reasons to me.
> > > >
> > > > I'm fine with "node" that is inside the function.
> > > >
> > > >> >    if (IS_ERR(fwnode))
> > > >> >            return 0;
> > > >> >
> > > >> >    if (device_match_fwnode(dev, fwnode)) // NULL check is inside
> > > >> >            return 1;
> > > >>
> > > >> Yeah, and it too can be supplied a secondary fwnode. Let's say we resolve
> > > >> a reference to a secondary software node and try to lookup a GPIO through it,
> > > >> we'll end up with an IS_ERR() fwnode with existing code, right?
> > > >
> > > > I'm not sure I understood the use case you are trying to describe here.
> > > >
> > > > The very first check guarantees that fwnode is either NULL or valid one.
> > > > When it's a valid one, the comparison with error pointer will be false.
> > > > What did I miss?
> > > >
> > >
> > > I mean: device_match_fwnode() has a NULL check but not an IS_ERR() check and
> > > can be passed a secondary fwnode as argument and that can be -ENODEV. It will
> > > probably not fail terribly but is still incorrect.
> > >
> > > I was speaking about the existing implementation, not addressing your comments.
> > >
> > > >> >    node = dev_fwnode(dev);
> > > >> >
> > > >> >    return fwnode_is_primary(node) && node->secondary == fwnode; // NULL check is inside
> > > >> >
> > > >> >
> > > >> >> + if (!fwnode)
> > > >> >> +         return 0;
> > > >> >
> > > >> >> + if (dev_node == fwnode)
> > > >> >> +         return 1;
> > > >> >> +
> > > >> >> + return fwnode_is_primary(dev_node) && dev_node->secondary == fwnode;
> > > >> >> +}
> > > >
> > > > ...
> > > >
> > > >> >> +int device_match_fwnode_ext(struct device *dev, const void *fwnode);
> > > >> >
> > > >> > Perhaps ext --> or_secondary ?
> > > >>
> > > >> I thought about it but it would make it sound like it only matches the
> > > >> secondary to me. Maybe device_match_all_fwnodes()? Would be future-proof if
> > > >> we end up doing the linked list approach.
> > > >
> > > > Danilo proposed _full, but in my opinion it's not better than _ext unless you
> > > > know very deep how fwnode structure is designed. Same with _all. It's confusing.
> > > >
> > > > fwnode_or_secondary (the key part is "or") sounds more precise. But if you come
> > > > up with something else that makes less ambiguity I will be glad.
> > > >
> > >
> > > device_match_fwnode_or_secondary() sounds good to me but shouldn't we try to
> > > limit the propagation of the "secondary" token in namespaces if our goal is to
> > > get rid of the whole "secondary fwnode" concept?
> >
> > I'm a bit late to this, but here's some background.
> >
> > Secondary fwnodes were not intended for device matching in the first
> > place.  The idea was to use them as a secondary supply of device
> > properties that may be missing from the primary node (think of an ACPI
> > device object accompanied by a software node supplying properties that
> > cannot be derived from the former).  They could be added by a driver
> > after matching the primary node for the benefit of a generic
> > framework.
> >
> > IMV, the example with children inheriting the fwnode from their parent
> > where the user wants to add a different secondary node to each child
> > doesn't really match the picture described above.  At least it was not
> > anticipated.  The idea was to allow the parent's fwnode to be extended
> > and then (possibly) inherited.
> >
> > That's why the secondary fwnode pointer is there under the primary one.
> >
> 
> That's evolution in practice for you. :) It turned out software nodes
> are a good alternative for platform data in board files or MFD
> sub-devices and can serve as the primary firmware node of a device.
> That's alright - we have a common fwnode interface and it works fine.
> 
> > So all of this goes beyond the original anticipated use of secondary
> > fwnodes and it seems to be calling for a list of equivalent (not
> > primary and secondary) fwnodes in struct device, but then of course
> > there's the question about duplicate properties and whether or not the
> > fwnode used for driver binding should be preferred (I don't see why
> > not).
> 
> I think that was Andy's initial proposal: treat the DT or ACPI node as
> the primary "main" node and any software node as an additional source
> of properties. In the absence of "real" nodes, I propose to treat the
> first software node as the primary.
> 
> > Until all of this is resolved, I wouldn't even add a generic helper
> > for matching secondary nodes.  I'd just put this code directly into
> > gpio_chip_match_by_fwnode() along with a big fat comment explaining
> > why exactly secondary nodes need to be taken into account there.
> >
> 
> IMO that's a recipe for keeping it local forever but I don't mind it.
> I think I proposed to add the helper because Andy suggested doing it
> in driver core.

Hmm...
Participating in the discussions for the implementation, yes, true.
Don't remember suggesting that, but if you tell, I might have done
t If it was the case, hat and forgot...

Whatever, for now we have a compromise solution and since Rafael
is the author and maintainer of device properties we may follow
his suggestion.

-- 
With Best Regards,
Andy Shevchenko