Re: [PATCH v4 03/10] software node: allow referencing firmware nodes

[Bartosz Golaszewski]

On Mon, Nov 3, 2025 at 10:49 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Mon, Nov 03, 2025 at 10:35:23AM +0100, Bartosz Golaszewski wrote:
> >
> > At the moment software nodes can only reference other software nodes.
> > This is a limitation for devices created, for instance, on the auxiliary
> > bus with a dynamic software node attached which cannot reference devices
> > the firmware node of which is "real" (as an OF node or otherwise).
> >
> > Make it possible for a software node to reference all firmware nodes in
> > addition to static software nodes. To that end: add a second pointer to
> > struct software_node_ref_args of type struct fwnode_handle. The core
> > swnode code will first check the swnode pointer and if it's NULL, it
> > will assume the fwnode pointer should be set. Rework the helper macros
> > and deprecate the existing ones whose names don't indicate the reference
> > type.
> >
> > Software node graphs remain the same, as in: the remote endpoints still
> > have to be software nodes.
>
> ...
>
> > +     /*
> > +      * A software node can reference other software nodes or firmware
> > +      * nodes (which are the abstraction layer sitting on top of them).
> > +      * This is done to ensure we can create references to static software
> > +      * nodes before they're registered with the firmware node framework.
> > +      * At the time the reference is being resolved, we expect the swnodes
> > +      * in question to already have been registered and to be backed by
> > +      * a firmware node. This is why we use the fwnode API below to read the
>
> A nit-pick (since anyway it requires a new version): move 'the' to the next
> line to make them more equal in the length.
>
> > +      * relevant properties and bump the reference count.
> > +      */
>
> ...
>
> > -#define SOFTWARE_NODE_REFERENCE(_ref_, ...)                  \
> > +#define __SOFTWARE_NODE_REF(_ref, ...)                               \
>
> No, NAK. The renaming of the parameters is not related to this change _at all_.
> Why do you change established style here? Did I miss your answer to my question
> in the previous rounds?
>

Ah, my brain just filtered out the trailing '_'.

> >  (const struct software_node_ref_args) {                              \
> > -     .node = _ref_,                                          \
> > +     .swnode = _Generic(_ref,                                \
> > +                        const struct software_node *: _ref,  \
> > +                        default: NULL),                      \
> > +     .fwnode = _Generic(_ref,                                \
> > +                        struct fwnode_handle *: _ref,        \
> > +                        default: NULL),                      \
> >       .nargs = COUNT_ARGS(__VA_ARGS__),                       \
> >       .args = { __VA_ARGS__ },                                \
> >  }
>
> ...
>
> > +#define SOFTWARE_NODE_REF_SWNODE(_ref, ...)                  \
> > +     __SOFTWARE_NODE_REF(_ref, __VA_ARGS__)
> > +
> > +#define SOFTWARE_NODE_REF_FWNODE(_ref, ...)                  \
> > +     __SOFTWARE_NODE_REF(_ref, __VA_ARGS__)
> > +
> > +/* DEPRECATED, use SOFTWARE_NODE_REF_SWNODE() instead. */
> > +#define SOFTWARE_NODE_REFERENCE(_ref, ...)                   \
> > +     SOFTWARE_NODE_REF_SWNODE(_ref, __VA_ARGS__)
>
> Now, useless.
>

No, why? With these changes, SOFTWARE_NODE_REFERENCE()'s name is a bit
misleading or incomplete, so I'm proposing to start replacing it with
SOFTWARE_NODE_REF_SWNODE() which is compatible with the former but has
a better name.

> ...
>
>
> > -#define PROPERTY_ENTRY_REF(_name_, _ref_, ...)                               \
> > +#define __PROPERTY_ENTRY_REF(_type, _name, _ref, ...)                        \
> >  (struct property_entry) {                                            \
> > -     .name = _name_,                                                 \
> > +     .name = _name,                                                  \
> >       .length = sizeof(struct software_node_ref_args),                \
> >       .type = DEV_PROP_REF,                                           \
> > -     { .pointer = &SOFTWARE_NODE_REFERENCE(_ref_, ##__VA_ARGS__), }, \
> > +     { .pointer = &_type(_ref, ##__VA_ARGS__), },                    \
> >  }
>
> Do we need this now? I assume that _Generic() takes case of this.
>

Ah, right, it should be done here as well.

> ...
>
> > +#define PROPERTY_ENTRY_REF_SWNODE(_name, _ref, ...)                  \
> > +     __PROPERTY_ENTRY_REF(SOFTWARE_NODE_REF_SWNODE,                  \
> > +                          _name, _ref, __VA_ARGS__)
> > +
> > +#define PROPERTY_ENTRY_REF_FWNODE(_name, _ref, ...)                  \
> > +     __PROPERTY_ENTRY_REF(SOFTWARE_NODE_REF_FWNODE,                  \
> > +                         _name, _ref, __VA_ARGS__)
> > +
> > +/* DEPRECATED, use PROPERTY_ENTRY_REF_SWNODE() instead. */
> > +#define PROPERTY_ENTRY_REF(_name, _ref, ...)                         \
> > +     PROPERTY_ENTRY_REF_SWNODE(_name, _ref, __VA_ARGS__)
>
> Seems like useless churn.
>

This is the same argument as with SOFTWARE_NODE_REF_SWNODE(). It's not
clear from the name what PROPERTY_ENTRY_REF() is really referencing.

Bart