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

[Bartosz Golaszewski]

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.

Bart