[PATCH v3 2/9] of: property: add of_graph_get_next_port_endpoint()

[Rob Herring]

+Jonathan C for the naming

On Mon, Aug 26, 2024 at 7:14 PM Kuninori Morimoto
<kuninori.morimoto.gx at renesas.com> wrote:
>
>
> Hi Rob
>
> > > We already have of_graph_get_next_endpoint(), but it is not
> > > intuitive to use in some case.
> >
> > Can of_graph_get_next_endpoint() users be replaced with your new
> > helpers? I'd really like to get rid of the 3 remaining users.
>
> Hmm...
> of_graph_get_next_endpoint() will fetch "endpoint" beyond the "port",
> but new helper doesn't have such feature.

Right, but the "feature" is somewhat awkward as you said. You
generally should know what port you are operating on.

> Even though I try to replace it with new helper, I guess it will be
> almost same as current of_graph_get_next_endpoint() anyway.
>
> Alternative idea is...
> One of the big user of of_graph_get_next_endpoint() is
> for_each_endpoint_of_node() loop.
>
> So we can replace it to..
>
> -       for_each_endpoint_of_node(parent, endpoint) {
> +       for_each_of_graph_port(parent, port) {
> +               for_each_of_graph_port_endpoint(port, endpoint) {
>
> Above is possible, but it replaces single loop to multi loops.
>
> And, we still need to consider about of_fwnode_graph_get_next_endpoint()
> which is the last user of of_graph_get_next_endpoint()

I missed fwnode_graph_get_next_endpoint() which has lots of users.
Though almost all of those are just "get the endpoint" and assume
there is only 1. In any case, it's a lot more than 3, so nevermind for
now.

> > > +struct device_node *of_graph_get_next_port_endpoint(const struct device_node *port,
> > > +                                               struct device_node *prev)
> > > +{
> > > +   do {
> > > +           prev = of_get_next_child(port, prev);
> > > +           if (!prev)
> > > +                   break;
> > > +   } while (!of_node_name_eq(prev, "endpoint"));
> >
> > Really, this check is validation as no other name is valid in a
> > port node. The kernel is not responsible for validation, but okay.
> > However, if we are going to keep this, might as well make it WARN().
>
> OK, will do in v4
>
> > > +/**
> > > + * for_each_of_graph_port_endpoint - iterate over every endpoint in a port node
> > > + * @parent: parent port node
> > > + * @child: loop variable pointing to the current endpoint node
> > > + *
> > > + * When breaking out of the loop, of_node_put(child) has to be called manually.
> >
> > No need for this requirement anymore. Use cleanup.h so this is
> > automatic.
>
> Do you mean it should include __free() inside this loop, like _scoped() ?

Yes.

> #define for_each_child_of_node_scoped(parent, child) \
>         for (struct device_node *child __free(device_node) =            \
>              of_get_next_child(parent, NULL);                           \
>              child != NULL;                                             \
>              child = of_get_next_child(parent, child))
>
> In such case, I wonder does it need to have _scoped() in loop name ?

Well, we added that to avoid changing all the users at once.

> And in such case, I think we want to have non _scoped() loop too ?

Do we have a user? I don't think we need it because anywhere we need
the node iterator pointer outside the loop that can be done explicitly
(no_free_ptr()).

So back to the name, I don't think we need _scoped in it. I think if
any user treats the iterator like it's the old style, the compiler is
going to complain.

> For example, when user want to use the param.
>
>         for_each_of_graph_port_endpoint(port, endpoint)
>                 if (xxx == yyy)
>                         return endpoint;
>
>         for_each_of_graph_port_endpoint_scoped(port, endpoint)
>                 if (xxx == yyy)
>                         return of_node_get(endpoint)

Actually, you would do "return_ptr(endpoint)" here.

Rob