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

Luca Ceresoli Jan. 31, 2024, 9:07 a.m. UTC | #1 
Hello Kuninori Morimoto,

On Wed, 31 Jan 2024 05:06:36 +0000
Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:

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

Luca
Kuninori Morimoto Jan. 31, 2024, 11:26 p.m. UTC | #2 
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)