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

[Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>]

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.
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()

> > +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() ?

#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 ?
And in such case, I think we want to have non _scoped() loop too ?
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)


Thank you for your help !!

Best regards
---
Kuninori Morimoto