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

[Rafael J. Wysocki]

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.

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

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.