diff mbox series

[v3,14/24] of: property: add of_graph_get_next_endpoint()

Message ID 874jeu6qhv.wl-kuninori.morimoto.gx@renesas.com
State New
Headers show
Series of: property: add port base loop | expand

Commit Message

Kuninori Morimoto Jan. 31, 2024, 5:06 a.m. UTC
We already have of_graph_get_next_device_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_device_endpoint(port1, NULL);
	A2 = of_graph_get_next_device_endpoint(port1, A1);

But 1st one will be error, because of_graph_get_next_device_endpoint()
requested "parent" means "node" (X) or "ports" (Y), not "port".
Below are OK

	/* These will be node/ports/port@0/endpoint */
	of_graph_get_next_device_endpoint(node,  NULL);
	of_graph_get_next_device_endpoint(ports, NULL);

In other words, we can't handle A1/A2 directly via
of_graph_get_next_device_endpoint() so far.

There is another non intuitive behavior on
of_graph_get_next_device_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

	/*
	 * "ep" is now A1, and handle port1 things here,
	 * but we don't know how many endpoints port1 has.
	 *
	 * Because "ep" is non NULL, we can use port1
	 * as of_graph_get_next_device_endpoint(port1, xxx)
	 */
	do {
		/* do something for port1 specific things here */
	} while (ep = of_graph_get_next_device_endpoint(port1, ep))

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

	of_graph_get_next_endpoint(port1, NULL); // A1
	of_graph_get_next_endpoint(port1, A1);   // A2
	of_graph_get_next_endpoint(port1, A2);   // NULL

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/of/property.c    | 24 +++++++++++++++++++++++-
 include/linux/of_graph.h |  9 +++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

Comments

Kuninori Morimoto Jan. 31, 2024, 11:26 p.m. UTC | #1
Hi Luca

Thank you for your review

> > To handle endpoint more intuitive, create of_graph_get_next_endpoint()
> > 
> > 	of_graph_get_next_endpoint(port1, NULL); // A1
> > 	of_graph_get_next_endpoint(port1, A1);   // A2
> > 	of_graph_get_next_endpoint(port1, A2);   // NULL
> 
> The idea looks good. My only concern is about reusing the
> of_graph_get_next_endpoint() name after having removed the old, different
> function having the same name. This can be confusing in the first
> place to who is used to the old function, and also to anybody rebasing
> their patches on top of a new kernel to find their code behaving
> differently.
> 
> Also, as now we'd have two similar variants of this function, it would
> be good if each of them were having a name that clearly identifies in
> which way they differ from the other.
> 
> So a better name for this function would probably be
> of_graph_get_next_port_endpoint() I guess, to clearly differentiate from
> of_graph_get_next_device_endpoint().

Yes, Indeed, Thank you for pointing it !
I will update it on v4


Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
diff mbox series

Patch

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 083f92513f5e..ff37fd5194c1 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -669,6 +669,28 @@  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() - get next endpoint node. If it reached to end of the port,
+ * it will return NULL.
+ * @port: pointer to the target port node
+ * @endpoint: current endpoint node, or NULL to get first
+ *
+ * Return: An 'endpoint' node pointer with refcount incremented. Refcount
+ * of the passed @prev node is decremented.
+ */
+struct device_node *of_graph_get_next_endpoint(const struct device_node *port,
+					       struct device_node *endpoint)
+{
+	do {
+		endpoint = of_get_next_child(port, endpoint);
+		if (!endpoint)
+			break;
+	} while (!of_node_name_eq(endpoint, "endpoint"));
+
+	return endpoint;
+}
+EXPORT_SYMBOL(of_graph_get_next_endpoint);
+
 /**
  * of_graph_get_next_device_endpoint() - get next endpoint node. If it reached to end of the port,
  * it gets next endpoint from next port.
@@ -712,7 +734,7 @@  struct device_node *of_graph_get_next_device_endpoint(const struct device_node *
 		 * getting the next child. If the previous endpoint is NULL this
 		 * will return the first child.
 		 */
-		endpoint = of_get_next_child(port, prev);
+		endpoint = of_graph_get_next_endpoint(port, prev);
 		if (endpoint) {
 			of_node_put(port);
 			return endpoint;
diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
index 48f7701feab1..ee4b219594f1 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -57,6 +57,8 @@  unsigned int of_graph_get_port_count(const struct device_node *np);
 struct device_node *of_graph_get_port_by_id(struct device_node *node, u32 id);
 struct device_node *of_graph_get_next_device_endpoint(const struct device_node *parent,
 						      struct device_node *previous);
+struct device_node *of_graph_get_next_endpoint(const struct device_node *port,
+					       struct device_node *prev);
 struct device_node *of_graph_get_next_port(const struct device_node *parent,
 					   struct device_node *previous);
 struct device_node *of_graph_get_endpoint_by_regs(
@@ -105,6 +107,13 @@  static inline struct device_node *of_graph_get_next_device_endpoint(
 	return NULL;
 }
 
+static inline struct device_node *of_graph_get_next_endpoint(
+					const struct device_node *parent,
+					struct device_node *previous)
+{
+	return NULL;
+}
+
 static inline struct device_node *of_graph_get_next_port(
 					const struct device_node *parent,
 					struct device_node *previous)