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