On Tue, 27 Aug 2024 08:54:51 -0500
Rob Herring <robh@xxxxxxxxxx> wrote:
> +Jonathan C for the naming
>
> On Mon, Aug 26, 2024 at 7:14 PM Kuninori Morimoto
> <kuninori.morimoto.gx@xxxxxxxxxxx> 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.
Hmm. Up to you but I'd be concerned that the scoping stuff is non
obvious enough that it is worth making people really really aware
it is going on.
However I don't feel strongly about it.
For the other _scoped iterators there is some push back
on the churn using them is causing so I doubt we'll ever get rid
of the non scoped variants. For something new that's not a concern.
Jonathan
>
> > 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
