diff mbox series

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

Message ID 87jzguw8ln.wl-kuninori.morimoto.gx@renesas.com
State Superseded
Headers show
Series of: property: add of_graph_get_next_port/port_endpoint() | expand

Commit Message

Kuninori Morimoto Aug. 6, 2024, 4:58 a.m. UTC 
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

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

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

	/*
	 * "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_endpoint(port1, xxx)
	 */
	do {
		/* do something for port1 specific things here */
	} while (ep = of_graph_get_next_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_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 it doesn't check node name (= "endpoint").

To handle endpoint more intuitive, create of_graph_get_next_port_endpoint()

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

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

Comments

Tomi Valkeinen Aug. 8, 2024, 8:30 a.m. UTC | #1 
On 06/08/2024 07:58, 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
> 
> 	/* These will be node/ports/port@0/endpoint */
> 	of_graph_get_next_endpoint(node,  NULL);
> 	of_graph_get_next_endpoint(ports, NULL);
> 
> 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
> 
> 	/*
> 	 * "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_endpoint(port1, xxx)
> 	 */
> 	do {
> 		/* do something for port1 specific things here */
> 	} while (ep = of_graph_get_next_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_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 it doesn't check node name (= "endpoint").
> 
> To handle endpoint more intuitive, create of_graph_get_next_port_endpoint()
> 
> 	of_graph_get_next_port_endpoint(port1, NULL); // A1
> 	of_graph_get_next_port_endpoint(port1, A1);   // A2
> 	of_graph_get_next_port_endpoint(port1, A2);   // NULL
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>   drivers/of/property.c    | 22 ++++++++++++++++++++++
>   include/linux/of_graph.h | 20 ++++++++++++++++++++
>   2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 3b2d09c0376a..de56795a7362 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -692,6 +692,28 @@ struct device_node *of_graph_get_next_port(struct device_node *parent,
>   }
>   EXPORT_SYMBOL(of_graph_get_next_port);
>   
> +/**
> + * of_graph_get_next_port_endpoint() - get next endpoint node in port.
> + * 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.
> + */

Same issues here too. No "prev" parameter, and I suggest using 
"previous", not "current", to be consistent with 
of_graph_get_next_endpoint(). (or alternatively, change 
of_graph_get_next_endpoint()).

Oh, the declaration of the function uses "prev", but the implementation 
"endpoint". Please make the naming same.

> +struct device_node *of_graph_get_next_port_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_port_endpoint);
> +
>   /**
>    * of_graph_get_next_endpoint() - get next endpoint node
>    *
> diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
> index 30169b50b042..8b4777938c5e 100644
> --- a/include/linux/of_graph.h
> +++ b/include/linux/of_graph.h
> @@ -59,6 +59,17 @@ struct of_endpoint {
>   	for (child = of_graph_get_next_port(parent, NULL); child != NULL; \
>   	     child = of_graph_get_next_port(parent, child))
>   
> +/**
> + * for_each_of_graph_port_endpoint - iterate over every endpoint in a port node
> + * @parent: parent device or ports node

Hmm, shouldn't the parent be a 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.
> + */
> +#define for_each_of_graph_port_endpoint(parent, child)			\
> +		for (child = of_graph_get_next_port_endpoint(parent, NULL); child != NULL; \
> +		     child = of_graph_get_next_port_endpoint(parent, child))
> +
>   #ifdef CONFIG_OF
>   bool of_graph_is_present(const struct device_node *node);
>   int of_graph_parse_endpoint(const struct device_node *node,
> @@ -73,6 +84,8 @@ struct device_node *of_graph_get_next_ports(struct device_node *parent,
>   					    struct device_node *ports);
>   struct device_node *of_graph_get_next_port(struct device_node *parent,
>   					   struct device_node *port);
> +struct device_node *of_graph_get_next_port_endpoint(const struct device_node *port,
> +						    struct device_node *prev);
>   struct device_node *of_graph_get_endpoint_by_regs(
>   		const struct device_node *parent, int port_reg, int reg);
>   struct device_node *of_graph_get_remote_endpoint(
> @@ -133,6 +146,13 @@ static inline struct device_node *of_graph_get_next_port(
>   	return NULL;
>   }
>   
> +static inline struct device_node *of_graph_get_next_port_endpoint(
> +					const struct device_node *parent,
> +					struct device_node *previous)
> +{
> +	return NULL;
> +}
> +
>   static inline struct device_node *of_graph_get_endpoint_by_regs(
>   		const struct device_node *parent, int port_reg, int reg)
>   {

  Tomi
diff mbox series

Patch

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 3b2d09c0376a..de56795a7362 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -692,6 +692,28 @@  struct device_node *of_graph_get_next_port(struct device_node *parent,
 }
 EXPORT_SYMBOL(of_graph_get_next_port);
 
+/**
+ * of_graph_get_next_port_endpoint() - get next endpoint node in port.
+ * 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_port_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_port_endpoint);
+
 /**
  * of_graph_get_next_endpoint() - get next endpoint node
  *
diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
index 30169b50b042..8b4777938c5e 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -59,6 +59,17 @@  struct of_endpoint {
 	for (child = of_graph_get_next_port(parent, NULL); child != NULL; \
 	     child = of_graph_get_next_port(parent, child))
 
+/**
+ * for_each_of_graph_port_endpoint - iterate over every endpoint in a port node
+ * @parent: parent device or ports 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.
+ */
+#define for_each_of_graph_port_endpoint(parent, child)			\
+		for (child = of_graph_get_next_port_endpoint(parent, NULL); child != NULL; \
+		     child = of_graph_get_next_port_endpoint(parent, child))
+
 #ifdef CONFIG_OF
 bool of_graph_is_present(const struct device_node *node);
 int of_graph_parse_endpoint(const struct device_node *node,
@@ -73,6 +84,8 @@  struct device_node *of_graph_get_next_ports(struct device_node *parent,
 					    struct device_node *ports);
 struct device_node *of_graph_get_next_port(struct device_node *parent,
 					   struct device_node *port);
+struct device_node *of_graph_get_next_port_endpoint(const struct device_node *port,
+						    struct device_node *prev);
 struct device_node *of_graph_get_endpoint_by_regs(
 		const struct device_node *parent, int port_reg, int reg);
 struct device_node *of_graph_get_remote_endpoint(
@@ -133,6 +146,13 @@  static inline struct device_node *of_graph_get_next_port(
 	return NULL;
 }
 
+static inline struct device_node *of_graph_get_next_port_endpoint(
+					const struct device_node *parent,
+					struct device_node *previous)
+{
+	return NULL;
+}
+
 static inline struct device_node *of_graph_get_endpoint_by_regs(
 		const struct device_node *parent, int port_reg, int reg)
 {