Re: [PATCH v2 03/13] of: property: add of_graph_get_next_endpoint_raw()

[Rob Herring <robh@kernel.org>]

On Mon, Jan 29, 2024 at 03:02:19PM +0200, Laurent Pinchart wrote:
> On Mon, Jan 29, 2024 at 02:29:22PM +0200, Tomi Valkeinen wrote:
> > On 29/01/2024 02:54, Kuninori Morimoto wrote:
> > > We already have of_graph_get_next_endpoint(), but it is not intuitive
> > > to use.
> > > 
> > > (X)	node {
> > > (Y)		ports {
> > > 			port@0 { endpoint { remote-endpoint = ...; };};
> > > (A1)			port@1 { endpoint { remote-endpoint = ...; };
> > > (A2)				 endpoint { remote-endpoint = ...; };};
> > > (B)			port@2 { endpoint { remote-endpoint = ...; };};
> > > 		};
> > > 	};
> > > 
> > > For example, if I want to handle port@1's 2 endpoints (= A1, A2),
> > > I want to use like below
> > > 
> > > 	A1 = of_graph_get_next_endpoint(port1, NULL);
> > > 	A2 = of_graph_get_next_endpoint(port1, A1);
> > > 
> > > But 1st one will be error, because of_graph_get_next_endpoint() requested
> > > "parent" means "node" (X) or "ports" (Y), not "port".
> > > Below are OK
> > > 
> > > 	of_graph_get_next_endpoint(node,  NULL); // node/ports/port@0/endpoint
> > > 	of_graph_get_next_endpoint(ports, NULL); // node/ports/port@0/endpoint
> > > 
> > > In other words, we can't handle A1/A2 directly via
> > > of_graph_get_next_endpoint() so far.
> > > 
> > > There is another non intuitive behavior on of_graph_get_next_endpoint().
> > > In case of if I could get A1 pointer for some way, and if I want to
> > > handle port@1 things, I would like use it like below
> > > 
> > > 	/*
> > > 	 * "endpoint" is now A1, and handle port1 things here,
> > > 	 * but we don't know how many endpoints port1 has.
> > > 	 *
> > > 	 * Because "endpoint" is non NULL, we can use port1
> > > 	 * as of_graph_get_next_endpoint(port1, xxx)
> > > 	 */
> > > 	do {
> > > 		/* do something for port1 specific things here */
> > > 	} while (endpoint = of_graph_get_next_endpoint(port1, endpoint))
> > > 
> > > But it also not worked as I expected.
> > > I expect it will be A1 -> A2 -> NULL,
> > > but      it will be A1 -> A2 -> B,    because of_graph_get_next_endpoint()
> > > will fetch endpoint beyond the port.
> > > 
> > > It is not useful on generic driver like Generic Sound Card.
> > > It uses of_get_next_child() instead for now, but it is not intuitive,
> > > and not check node name (= "endpoint").
> > > 
> > > To handle endpoint more intuitive, create of_graph_get_next_endpoint_raw()
> > > 
> > > 	of_graph_get_next_endpoint_raw(port1, NULL); // A1
> > > 	of_graph_get_next_endpoint_raw(port1, A1);   // A2
> > > 	of_graph_get_next_endpoint_raw(port1, A2);   // NULL
> > > 
> > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > > ---
> > >   drivers/of/property.c    | 26 +++++++++++++++++++++++++-
> > >   include/linux/of_graph.h |  2 ++
> > >   2 files changed, 27 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index 14ffd199c9b1..37dbb1b0e742 100644
> > > --- a/drivers/of/property.c
> > > +++ b/drivers/of/property.c
> > > @@ -667,6 +667,30 @@ struct device_node *of_graph_get_next_port(const struct device_node *parent,
> > >   }
> > >   EXPORT_SYMBOL(of_graph_get_next_port);
> > >   
> > > +/**
> > > + * of_graph_get_next_endpoint_raw() - get next endpoint node
> > 
> > How about "of_graph_get_next_port_endpoint()"?
> 
> We may want to also rename the existing of_graph_get_next_endpoint()
> function to of_graph_next_dev_endpoint() then. It would be a tree-wide
> patch, which is always annoying to get reviewed and merged, so if Rob
> would prefer avoiding the rename, I'm fine with that.

I think we should get rid of or minimize of_graph_get_next_endpoint() in 
its current form. In general, drivers should be asking for a specific 
port number because their function is fixed in the binding. Iterating 
over endpoints within a port is okay as that's usually a selecting 1 of 
N operation. 

Most cases are in the form of of_graph_get_next_endpoint(dev, NULL) 
which is equivalent to of_graph_get_endpoint_by_regs(dev, 0, 0). 
Technically, -1 instead of 0 is equivalent, but I'd argue is sloppy and 
wrong.

I also added of_graph_get_remote_node() for this reason and cleaned a 
lot of these (in DRM) up some time ago. Because in the end, a driver 
generally just wants the remote device it is connected to and details of 
parsing the graph should be mostly opaque.

Wouldn't something like this work for this case:

#define for_each_port_endpoint_of_node(parent, port, child) \
	for (child = of_graph_get_endpoint_by_regs(parent, port, -1); child != NULL; \
	     child = of_get_next_child(parent, child))

Rob