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