[PATCH v3 0/9] of: property: add of_graph_get_next_port/port_endpoint()

Hi Rob, Saravana, Tomi, Laurent, Sakari

This is v3 patch-set

I have been posting to add new port base for loop function
as below steps.

[o] done
[@] this patch set

	[o] tidyup of_graph_get_endpoint_count()
	[o] replace endpoint func - use endpoint_by_regs()
	[o] replace endpoint func - use for_each()
	[@] add new port function

Current Of-graph has "endpoint base" for loop, but doesn't have
"port base" loop. "endpoint base" loop only is not enough.
This patch-set add new "port base" for loop, and use it.

v2 -> v3
	- return NULL if it it doesn't have ports / port
	- add visible comment on of_graph_get_next_ports()

v1 -> v2
	- add each Reviewed-by / Acked-by
	- tidyup/update Kernel Docs
	- use prev as parameter
	- update git-log explanation
	- remove extra changes


Kuninori Morimoto (9):
  of: property: add of_graph_get_next_port()
  of: property: add of_graph_get_next_port_endpoint()
  ASoC: test-component: use new of_graph functions
  ASoC: rcar_snd: use new of_graph functions
  ASoC: audio-graph-card: use new of_graph functions
  ASoC: audio-graph-card2: use new of_graph functions
  gpu: drm: omapdrm: use new of_graph functions
  fbdev: omapfb: use new of_graph functions
  media: xilinx-tpg: use new of_graph functions

 drivers/gpu/drm/omapdrm/dss/dpi.c             |   3 +-
 drivers/gpu/drm/omapdrm/dss/sdi.c             |   3 +-
 drivers/media/platform/xilinx/xilinx-tpg.c    |   3 +-
 drivers/of/property.c                         | 134 ++++++++++++++++++
 drivers/video/fbdev/omap2/omapfb/dss/dpi.c    |   3 +-
 drivers/video/fbdev/omap2/omapfb/dss/dss-of.c |  66 ---------
 drivers/video/fbdev/omap2/omapfb/dss/dss.c    |   9 +-
 drivers/video/fbdev/omap2/omapfb/dss/sdi.c    |   3 +-
 include/linux/of_graph.h                      |  66 +++++++++
 include/video/omapfb_dss.h                    |   8 --
 sound/soc/generic/audio-graph-card.c          |   5 +-
 sound/soc/generic/audio-graph-card2.c         | 111 +++++++--------
 sound/soc/generic/test-component.c            |   4 +-
 sound/soc/sh/rcar/core.c                      |  12 +-
 14 files changed, 270 insertions(+), 160 deletions(-)

-- 
2.43.0

We have endpoint base functions
	- of_graph_get_next_device_endpoint()
	- of_graph_get_device_endpoint_count()
	- for_each_of_graph_device_endpoint()

Here, for_each_of_graph_device_endpoint() loop finds each endpoints

	ports {
		port@0 {
(1)			endpoint {...};
		};
		port@1 {
(2)			endpoint {...};
		};
		...
	};

In above case, it finds endpoint as (1) -> (2) -> ...

Basically, user/driver knows which port is used for what, but not in
all cases. For example on flexible/generic driver case, how many ports
are used is not fixed.

For example Sound Generic Card driver which is used from many venders
can't know how many ports are used. Because the driver is very
flexible/generic, it is impossible to know how many ports are used,
it depends on each vender SoC and/or its used board.

And more, the port can have multi endpoints. For example Generic Sound
Card case, it supports many type of connection between CPU / Codec, and
some of them uses multi endpoint in one port.
Then, Generic Sound Card want to handle each connection via "port"
instead of "endpoint".
But, it is very difficult to handle each "port" via
for_each_of_graph_device_endpoint(). Getting "port" by using
of_get_parent() from "endpoint" doesn't work. see below.

	ports {
		port@0 {
(1)			endpoint@0 {...};
(2)			endpoint@1 {...};
		};
		port@1 {
(3)			endpoint {...};
		};
		...
	};

In the same time, same reason, we want to handle "ports" same as "port".

	node {
=>		ports@0 {
			port@0 {
				endpoint@0 {...};
				endpoint@1 {...};
				...
			};
			port@1 {
				endpoint@0 {...};
				endpoint@1 {...};
				...
			};
			...
		};
=>		ports@1 {
			...
		};
	};

Add "ports" / "port" base functions.
For above case, we can use

	for_each_of_graph_ports(node, ports) {
		for_each_of_graph_port(ports, port) {
			...
		}
	}

This loop works both "node" have / doesn't have "ports", like below

	node {
		port { };
	};

	node {
		ports {
			port { };
		};
	};

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

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 164d77cb94458..aec6ac9f70064 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c

@@ -625,6 +625,100 @@ struct device_node *of_graph_get_port_by_id(struct device_node *parent, u32 id)
 }
 EXPORT_SYMBOL(of_graph_get_port_by_id);
 
+/**
+ * of_graph_get_next_ports() - get next ports node.
+ * @parent: pointer to the parent device node
+ * @prev: previous ports node, or NULL to get first
+ *
+ * If "parent" node doesn't have "ports" node, it returns "parent" node itself as "ports" node.
+ *
+ * Return: A 'ports' node pointer with refcount incremented. Refcount
+ * of the passed @prev node is decremented.
+ */
+struct device_node *of_graph_get_next_ports(struct device_node *parent,
+					    struct device_node *prev)
+{
+	if (!parent)
+		return NULL;
+
+	if (!prev) {
+		/*
+		 * Find "ports" node from parent
+		 *
+		 *	parent {
+		 * =>		ports {
+		 *			port {...};
+		 *		};
+		 *	};
+		 */
+		prev = of_get_child_by_name(parent, "ports");
+
+		/*
+		 * Use parent as its ports if it not exist
+		 *
+		 * =>	parent {
+		 *		port {...};
+		 *	};
+		 */
+		if (!prev) {
+			prev = of_node_get(parent);
+
+			/* check wether it has port node */
+			struct device_node *port __free(device_node) =
+				of_get_child_by_name(prev, "port");
+
+			if (!port)
+				prev = NULL;
+		}
+
+		return prev;
+	}
+
+	/* Find next ports */
+	do {
+		prev = of_get_next_child(parent, prev);
+		if (!prev)
+			break;
+	} while (!of_node_name_eq(prev, "ports"));
+
+	return prev;
+}
+EXPORT_SYMBOL(of_graph_get_next_ports);
+
+/**
+ * of_graph_get_next_port() - get next port node.
+ * @parent: pointer to the parent device node, or parent ports node
+ * @prev: previous port node, or NULL to get first
+ *
+ * Parent device node can be used as @parent whether device node has ports node or not.
+ * It will work same as ports@0 node.
+ *
+ * Return: A 'port' node pointer with refcount incremented. Refcount
+ * of the passed @prev node is decremented.
+ */
+struct device_node *of_graph_get_next_port(struct device_node *parent,
+					   struct device_node *prev)
+{
+	if (!parent)
+		return NULL;
+
+	if (!prev) {
+		struct device_node *ports __free(device_node) =
+			of_graph_get_next_ports(parent, NULL);
+
+		return of_get_child_by_name(ports, "port");
+	}
+
+	do {
+		prev = of_get_next_child(parent, prev);
+		if (!prev)
+			break;
+	} while (!of_node_name_eq(prev, "port"));
+
+	return prev;
+}
+EXPORT_SYMBOL(of_graph_get_next_port);
+
 /**
  * of_graph_get_next_endpoint() - get next endpoint node
  * @parent: pointer to the parent device node

@@ -823,6 +917,24 @@ unsigned int of_graph_get_endpoint_count(const struct device_node *np)
 }
 EXPORT_SYMBOL(of_graph_get_endpoint_count);
 
+/**
+ * of_graph_get_port_count() - get the number of port in a device node
+ * @np: pointer to the parent device node
+ *
+ * Return: count of port of this device node
+ */
+unsigned int of_graph_get_port_count(struct device_node *np)
+{
+	struct device_node *port = NULL;
+	unsigned int num = 0;
+
+	for_each_of_graph_port(np, port)
+		num++;
+
+	return num;
+}
+EXPORT_SYMBOL(of_graph_get_port_count);
+
 /**
  * of_graph_get_remote_node() - get remote parent device_node for given port/endpoint
  * @node: pointer to parent device_node containing graph port/endpoint

diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
index a4bea62bfa290..a6b91577700a8 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h

@@ -37,14 +37,41 @@ struct of_endpoint {
 	for (child = of_graph_get_next_endpoint(parent, NULL); child != NULL; \
 	     child = of_graph_get_next_endpoint(parent, child))
 
+/**
+ * for_each_of_graph_ports - iterate over every ports in a device node
+ * @parent: parent device node containing ports
+ * @child: loop variable pointing to the current ports node
+ *
+ * When breaking out of the loop, of_node_put(child) has to be called manually.
+ */
+#define for_each_of_graph_ports(parent, child)				\
+	for (child = of_graph_get_next_ports(parent, NULL); child != NULL; \
+	     child = of_graph_get_next_ports(parent, child))
+
+/**
+ * for_each_of_graph_port - iterate over every port in a device or ports node
+ * @parent: parent device or ports node containing port
+ * @child: loop variable pointing to the current port node
+ *
+ * When breaking out of the loop, of_node_put(child) has to be called manually.
+ */
+#define for_each_of_graph_port(parent, child)			\
+	for (child = of_graph_get_next_port(parent, NULL); child != NULL; \
+	     child = of_graph_get_next_port(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,
 				struct of_endpoint *endpoint);
 unsigned int of_graph_get_endpoint_count(const struct device_node *np);
+unsigned int of_graph_get_port_count(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_endpoint(const struct device_node *parent,
 					struct device_node *previous);
+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_endpoint_by_regs(
 		const struct device_node *parent, int port_reg, int reg);
 struct device_node *of_graph_get_remote_endpoint(

@@ -73,6 +100,11 @@ static inline unsigned int of_graph_get_endpoint_count(const struct device_node
 	return 0;
 }
 
+static inline unsigned int of_graph_get_port_count(struct device_node *np)
+{
+	return 0;
+}
+
 static inline struct device_node *of_graph_get_port_by_id(
 					struct device_node *node, u32 id)
 {

@@ -86,6 +118,20 @@ static inline struct device_node *of_graph_get_next_endpoint(
 	return NULL;
 }
 
+static inline struct device_node *of_graph_get_next_ports(
+					struct device_node *parent,
+					struct device_node *previous)
+{
+	return NULL;
+}
+
+static inline struct device_node *of_graph_get_next_port(
+					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)
 {

-- 
2.43.0

We already have of_graph_get_next_endpoint(), but it is not
intuitive to use in some case.

(X)	node {
(Y)		ports {
(P0)			port@0 { endpoint { remote-endpoint = ...; };};
(P10)			port@1 { endpoint { remote-endpoint = ...; };
(P11)				 endpoint { remote-endpoint = ...; };};
(P2)			port@2 { endpoint { remote-endpoint = ...; };};
		};
	};

For example, if I want to handle port@1's 2 endpoints (= P10, P11),
I want to use like below

	P10 = of_graph_get_next_endpoint(port1, NULL);
	P11 = of_graph_get_next_endpoint(port1, P10);

But 1st one will be error, because of_graph_get_next_endpoint()
requested "parent" means "node" (X) or "ports" (Y), not "port".
Below works, but it will get P0

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

In other words, we can't handle P10/P11 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 P10 pointer for some way, and if I want to
handle port@1 things, I would like use it like below

	/*
	 * "ep" is now P10, and handle port1 things here,
	 * but we don't know how many endpoints port1 has.
	 *
	 * Because "ep" is non NULL now, 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 P10 -> P11 -> NULL,
but      it will be P10 -> P11 -> P2,    because
of_graph_get_next_endpoint() will fetch "endpoint" beyond the "port".

It is not useful on generic driver.
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); // P10
	of_graph_get_next_port_endpoint(port1, P10);  // P11
	of_graph_get_next_port_endpoint(port1, P11);  // 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 aec6ac9f70064..90820e43bc973 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c

@@ -719,6 +719,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
+ * @prev: previous 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 *prev)
+{
+	do {
+		prev = of_get_next_child(port, prev);
+		if (!prev)
+			break;
+	} while (!of_node_name_eq(prev, "endpoint"));
+
+	return prev;
+}
+EXPORT_SYMBOL(of_graph_get_next_port_endpoint);
+
 /**
  * of_graph_get_next_endpoint() - get next endpoint node
  * @parent: pointer to the parent device node

diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
index a6b91577700a8..967ee14a1ff37 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 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,

@@ -72,6 +83,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(

@@ -132,6 +145,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)
 {

-- 
2.43.0

Current test-component.c is using for_each_endpoint_of_node()
for parsing "port", because there was no "port" base loop before.
It has been assuming 1 port has 1 endpoint here.

But now we can use "port" base loop (= for_each_of_graph_port()).
Let's replace for_each function from "endpoint" base to "port" base.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Acked-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/generic/test-component.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/generic/test-component.c b/sound/soc/generic/test-component.c
index df2487b700cca..93288f7fa3861 100644
--- a/sound/soc/generic/test-component.c
+++ b/sound/soc/generic/test-component.c

@@ -521,7 +521,7 @@ static int test_driver_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *node = dev->of_node;
-	struct device_node *ep;
+	struct device_node *port;
 	const struct test_adata *adata = of_device_get_match_data(&pdev->dev);
 	struct snd_soc_component_driver *cdriv;
 	struct snd_soc_dai_driver *ddriv;

@@ -591,7 +591,7 @@ static int test_driver_probe(struct platform_device *pdev)
 	}
 
 	i = 0;
-	for_each_endpoint_of_node(node, ep) {
+	for_each_of_graph_port(node, port) {
 		snprintf(dname[i].name, TEST_NAME_LEN, "%s.%d", node->name, i);
 		ddriv[i].name = dname[i].name;
 

-- 
2.43.0

Now we can use new port related functions for port parsing. Use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Acked-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/sh/rcar/core.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c
index 15cb5e7008f9f..dd8e9bfee6c0c 100644
--- a/sound/soc/sh/rcar/core.c
+++ b/sound/soc/sh/rcar/core.c

@@ -1277,11 +1277,8 @@ static int rsnd_dai_of_node(struct rsnd_priv *priv, int *is_graph)
 	/*
 	 * Audio-Graph-Card
 	 */
-	for_each_child_of_node(np, ports) {
-		if (!of_node_name_eq(ports, "ports") &&
-		    !of_node_name_eq(ports, "port"))
-			continue;
-		priv->component_dais[i] = of_graph_get_endpoint_count(ports);
+	for_each_of_graph_ports(np, ports) {
+		priv->component_dais[i] = of_graph_get_port_count(ports);
 		nr += priv->component_dais[i];
 		i++;
 		if (i >= RSND_MAX_COMPONENT) {

@@ -1489,10 +1486,7 @@ static int rsnd_dai_probe(struct rsnd_priv *priv)
 		struct device_node *ports;
 		struct device_node *dai_np;
 
-		for_each_child_of_node(np, ports) {
-			if (!of_node_name_eq(ports, "ports") &&
-			    !of_node_name_eq(ports, "port"))
-				continue;
+		for_each_of_graph_ports(np, ports) {
 			for_each_endpoint_of_node(ports, dai_np) {
 				__rsnd_dai_probe(priv, dai_np, dai_np, 0, dai_i);
 				if (!rsnd_is_gen1(priv) && !rsnd_is_gen2(priv)) {

-- 
2.43.0

Now we can use new port related functions for port parsing. Use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Acked-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/generic/audio-graph-card.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c
index 1bdcfc4d4222e..efc5e86a914ca 100644
--- a/sound/soc/generic/audio-graph-card.c
+++ b/sound/soc/generic/audio-graph-card.c

@@ -374,10 +374,7 @@ static int __graph_for_each_link(struct simple_util_priv *priv,
 		cpu_ep	 = NULL;
 
 		/* loop for all CPU endpoint */
-		while (1) {
-			cpu_ep = of_get_next_child(cpu_port, cpu_ep);
-			if (!cpu_ep)
-				break;
+		for_each_of_graph_port_endpoint(cpu_port, cpu_ep) {
 
 			/* get codec */
 			codec_ep = of_graph_get_remote_endpoint(cpu_ep);

-- 
2.43.0

Now we can use new port related functions for port parsing. Use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Acked-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/generic/audio-graph-card2.c | 111 ++++++++++++--------------
 1 file changed, 49 insertions(+), 62 deletions(-)

diff --git a/sound/soc/generic/audio-graph-card2.c b/sound/soc/generic/audio-graph-card2.c
index 051adb5673972..e5bcccfb49242 100644
--- a/sound/soc/generic/audio-graph-card2.c
+++ b/sound/soc/generic/audio-graph-card2.c

@@ -234,8 +234,6 @@ enum graph_type {
 #define GRAPH_NODENAME_DPCM	"dpcm"
 #define GRAPH_NODENAME_C2C	"codec2codec"
 
-#define port_to_endpoint(port) of_get_child_by_name(port, "endpoint")
-
 #define ep_to_port(ep)	of_get_parent(ep)
 static struct device_node *port_to_ports(struct device_node *port)
 {

@@ -351,14 +349,9 @@ static struct device_node *graph_get_next_multi_ep(struct device_node **port)
 	 *	port@1 { rep1 };
 	 * };
 	 */
-	do {
-		*port = of_get_next_child(ports, *port);
-		if (!*port)
-			break;
-	} while (!of_node_name_eq(*port, "port"));
-
+	*port = of_graph_get_next_port(ports, *port);
 	if (*port) {
-		ep  = port_to_endpoint(*port);
+		ep  = of_graph_get_next_port_endpoint(*port, NULL);
 		rep = of_graph_get_remote_endpoint(ep);
 	}
 

@@ -530,67 +523,70 @@ static int graph_parse_node_multi_nm(struct snd_soc_dai_link *dai_link,
 	 *	};
 	 * };
 	 */
-	struct device_node *mcpu_ep		= port_to_endpoint(mcpu_port);
-	struct device_node *mcpu_ep_n		= mcpu_ep;
-	struct device_node *mcpu_port_top	= of_get_next_child(port_to_ports(mcpu_port), NULL);
-	struct device_node *mcpu_ep_top		= port_to_endpoint(mcpu_port_top);
+	struct device_node *mcpu_ep_n;
+	struct device_node *mcpu_ep		= of_graph_get_next_port_endpoint(mcpu_port, NULL);
+	struct device_node *mcpu_ports		= port_to_ports(mcpu_port);
+	struct device_node *mcpu_port_top	= of_graph_get_next_port(mcpu_ports, NULL);
+	struct device_node *mcpu_ep_top		= of_graph_get_next_port_endpoint(mcpu_port_top, NULL);
 	struct device_node *mcodec_ep_top	= of_graph_get_remote_endpoint(mcpu_ep_top);
 	struct device_node *mcodec_port_top	= ep_to_port(mcodec_ep_top);
 	struct device_node *mcodec_ports	= port_to_ports(mcodec_port_top);
 	int nm_max = max(dai_link->num_cpus, dai_link->num_codecs);
-	int ret = -EINVAL;
+	int ret = 0;
 
-	if (cpu_idx > dai_link->num_cpus)
+	if (cpu_idx > dai_link->num_cpus) {
+		ret = -EINVAL;
 		goto mcpu_err;
+	}
 
-	while (1) {
+	for_each_of_graph_port_endpoint(mcpu_port, mcpu_ep_n) {
 		struct device_node *mcodec_ep_n;
 		struct device_node *mcodec_port_i;
 		struct device_node *mcodec_port;
 		int codec_idx;
 
-		if (*nm_idx > nm_max)
-			break;
+		/* ignore 1st ep which is for element */
+		if (mcpu_ep_n == mcpu_ep)
+			continue;
 
-		mcpu_ep_n = of_get_next_child(mcpu_port, mcpu_ep_n);
-		if (!mcpu_ep_n) {
-			ret = 0;
+		if (*nm_idx > nm_max)
 			break;
-		}
 
 		mcodec_ep_n	= of_graph_get_remote_endpoint(mcpu_ep_n);
 		mcodec_port	= ep_to_port(mcodec_ep_n);
 
-		if (mcodec_ports != port_to_ports(mcodec_port))
+		if (mcodec_ports != port_to_ports(mcodec_port)) {
+			ret = -EINVAL;
 			goto mcpu_err;
+		}
 
 		codec_idx = 0;
-		mcodec_port_i = of_get_next_child(mcodec_ports, NULL);
-		while (1) {
-			if (codec_idx > dai_link->num_codecs)
-				goto mcodec_err;
-
-			mcodec_port_i = of_get_next_child(mcodec_ports, mcodec_port_i);
+		ret = -EINVAL;
+		for_each_of_graph_port(mcodec_ports, mcodec_port_i) {
 
-			if (!mcodec_port_i)
-				goto mcodec_err;
+			/* ignore 1st port which is for pair connection */
+			if (mcodec_port_top == mcodec_port_i)
+				continue;
 
-			if (mcodec_port_i == mcodec_port)
+			if (codec_idx > dai_link->num_codecs)
 				break;
 
+			if (mcodec_port_i == mcodec_port) {
+				dai_link->ch_maps[*nm_idx].cpu	 = cpu_idx;
+				dai_link->ch_maps[*nm_idx].codec = codec_idx;
+
+				(*nm_idx)++;
+				ret = 0;
+				break;
+			}
 			codec_idx++;
 		}
-
-		dai_link->ch_maps[*nm_idx].cpu		= cpu_idx;
-		dai_link->ch_maps[*nm_idx].codec	= codec_idx;
-
-		(*nm_idx)++;
-
 		of_node_put(mcodec_port_i);
-mcodec_err:
 		of_node_put(mcodec_port);
 		of_node_put(mcpu_ep_n);
 		of_node_put(mcodec_ep_n);
+		if (ret < 0)
+			break;
 	}
 mcpu_err:
 	of_node_put(mcpu_ep);

@@ -674,7 +670,7 @@ static int graph_parse_node_single(struct simple_util_priv *priv,
 				   struct device_node *port,
 				   struct link_info *li, int is_cpu)
 {
-	struct device_node *ep = port_to_endpoint(port);
+	struct device_node *ep = of_graph_get_next_port_endpoint(port, NULL);
 	int ret = __graph_parse_node(priv, gtype, ep, li, is_cpu, 0);
 
 	of_node_put(ep);

@@ -769,7 +765,7 @@ static void graph_link_init(struct simple_util_priv *priv,
 		of_node_put(port_cpu);
 		port_cpu = ep_to_port(ep_cpu);
 	} else {
-		ep_cpu = port_to_endpoint(port_cpu);
+		ep_cpu = of_graph_get_next_port_endpoint(port_cpu, NULL);
 	}
 	ports_cpu = port_to_ports(port_cpu);
 

@@ -779,7 +775,7 @@ static void graph_link_init(struct simple_util_priv *priv,
 		of_node_put(port_cpu);
 		port_codec = ep_to_port(ep_codec);
 	} else {
-		ep_codec = port_to_endpoint(port_codec);
+		ep_codec = of_graph_get_next_port_endpoint(port_codec, NULL);
 	}
 	ports_codec = port_to_ports(port_codec);
 

@@ -850,7 +846,7 @@ int audio_graph2_link_normal(struct simple_util_priv *priv,
 			     struct link_info *li)
 {
 	struct device_node *cpu_port = lnk;
-	struct device_node *cpu_ep = port_to_endpoint(cpu_port);
+	struct device_node *cpu_ep = of_graph_get_next_port_endpoint(cpu_port, NULL);
 	struct device_node *codec_port = of_graph_get_remote_port(cpu_ep);
 	int ret;
 

@@ -883,7 +879,7 @@ int audio_graph2_link_dpcm(struct simple_util_priv *priv,
 			   struct device_node *lnk,
 			   struct link_info *li)
 {
-	struct device_node *ep = port_to_endpoint(lnk);
+	struct device_node *ep = of_graph_get_next_port_endpoint(lnk, NULL);
 	struct device_node *rep = of_graph_get_remote_endpoint(ep);
 	struct device_node *cpu_port = NULL;
 	struct device_node *codec_port = NULL;

@@ -1007,7 +1003,7 @@ int audio_graph2_link_c2c(struct simple_util_priv *priv,
 	of_node_get(lnk);
 	port0 = lnk;
 	ports = port_to_ports(port0);
-	port1 = of_get_next_child(ports, lnk);
+	port1 = of_graph_get_next_port(ports, port0);
 
 	/*
 	 * Card2 can use original Codec2Codec settings if DT has.

@@ -1037,8 +1033,8 @@ int audio_graph2_link_c2c(struct simple_util_priv *priv,
 		dai_link->num_c2c_params	= 1;
 	}
 
-	ep0 = port_to_endpoint(port0);
-	ep1 = port_to_endpoint(port1);
+	ep0 = of_graph_get_next_port_endpoint(port0, NULL);
+	ep1 = of_graph_get_next_port_endpoint(port1, NULL);
 
 	codec0_port = of_graph_get_remote_port(ep0);
 	codec1_port = of_graph_get_remote_port(ep1);

@@ -1139,21 +1135,12 @@ static int graph_counter(struct device_node *lnk)
 	 */
 	if (graph_lnk_is_multi(lnk)) {
 		struct device_node *ports = port_to_ports(lnk);
-		struct device_node *port = NULL;
-		int cnt = 0;
 
 		/*
 		 * CPU/Codec = N:M case has many endpoints.
 		 * We can't use of_graph_get_endpoint_count() here
 		 */
-		while(1) {
-			port = of_get_next_child(ports, port);
-			if (!port)
-				break;
-			cnt++;
-		}
-
-		return cnt - 1;
+		return of_graph_get_port_count(ports) - 1;
 	}
 	/*
 	 * Single CPU / Codec

@@ -1167,7 +1154,7 @@ static int graph_count_normal(struct simple_util_priv *priv,
 			      struct link_info *li)
 {
 	struct device_node *cpu_port = lnk;
-	struct device_node *cpu_ep = port_to_endpoint(cpu_port);
+	struct device_node *cpu_ep = of_graph_get_next_port_endpoint(cpu_port, NULL);
 	struct device_node *codec_port = of_graph_get_remote_port(cpu_ep);
 
 	/*

@@ -1195,7 +1182,7 @@ static int graph_count_dpcm(struct simple_util_priv *priv,
 			    struct device_node *lnk,
 			    struct link_info *li)
 {
-	struct device_node *ep = port_to_endpoint(lnk);
+	struct device_node *ep = of_graph_get_next_port_endpoint(lnk, NULL);
 	struct device_node *rport = of_graph_get_remote_port(ep);
 
 	/*

@@ -1237,9 +1224,9 @@ static int graph_count_c2c(struct simple_util_priv *priv,
 {
 	struct device_node *ports = port_to_ports(lnk);
 	struct device_node *port0 = lnk;
-	struct device_node *port1 = of_get_next_child(ports, of_node_get(lnk));
-	struct device_node *ep0 = port_to_endpoint(port0);
-	struct device_node *ep1 = port_to_endpoint(port1);
+	struct device_node *port1 = of_graph_get_next_port(ports, of_node_get(port0));
+	struct device_node *ep0 = of_graph_get_next_port_endpoint(port0, NULL);
+	struct device_node *ep1 = of_graph_get_next_port_endpoint(port1, NULL);
 	struct device_node *codec0 = of_graph_get_remote_port(ep0);
 	struct device_node *codec1 = of_graph_get_remote_port(ep1);
 

-- 
2.43.0

Now we can use new port related functions for port parsing. Use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/dss/dpi.c | 3 ++-
 drivers/gpu/drm/omapdrm/dss/sdi.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dpi.c b/drivers/gpu/drm/omapdrm/dss/dpi.c
index 030f997eccd00..b17e77f700ddd 100644
--- a/drivers/gpu/drm/omapdrm/dss/dpi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dpi.c

@@ -16,6 +16,7 @@
 #include <linux/export.h>
 #include <linux/kernel.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 #include <linux/string.h>

@@ -709,7 +710,7 @@ int dpi_init_port(struct dss_device *dss, struct platform_device *pdev,
 	if (!dpi)
 		return -ENOMEM;
 
-	ep = of_get_next_child(port, NULL);
+	ep = of_graph_get_next_port_endpoint(port, NULL);
 	if (!ep)
 		return 0;
 

diff --git a/drivers/gpu/drm/omapdrm/dss/sdi.c b/drivers/gpu/drm/omapdrm/dss/sdi.c
index 91eaae3b94812..f9ae358e8e521 100644
--- a/drivers/gpu/drm/omapdrm/dss/sdi.c
+++ b/drivers/gpu/drm/omapdrm/dss/sdi.c

@@ -11,6 +11,7 @@
 #include <linux/export.h>
 #include <linux/kernel.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 #include <linux/string.h>

@@ -346,7 +347,7 @@ int sdi_init_port(struct dss_device *dss, struct platform_device *pdev,
 	if (!sdi)
 		return -ENOMEM;
 
-	ep = of_get_next_child(port, NULL);
+	ep = of_graph_get_next_port_endpoint(port, NULL);
 	if (!ep) {
 		r = 0;
 		goto err_free;

-- 
2.43.0

Now we can use new port related functions for port parsing. Use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/video/fbdev/omap2/omapfb/dss/dpi.c    |  3 +-
 drivers/video/fbdev/omap2/omapfb/dss/dss-of.c | 66 -------------------
 drivers/video/fbdev/omap2/omapfb/dss/dss.c    |  9 +--
 drivers/video/fbdev/omap2/omapfb/dss/sdi.c    |  3 +-
 include/video/omapfb_dss.h                    |  8 ---
 5 files changed, 9 insertions(+), 80 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dpi.c b/drivers/video/fbdev/omap2/omapfb/dss/dpi.c
index 7c1b7d89389aa..395b1139a5ae7 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dpi.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dpi.c

@@ -20,6 +20,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/string.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/clk.h>
 #include <linux/component.h>
 

@@ -845,7 +846,7 @@ int dpi_init_port(struct platform_device *pdev, struct device_node *port)
 	if (!dpi)
 		return -ENOMEM;
 
-	ep = omapdss_of_get_next_endpoint(port, NULL);
+	ep = of_graph_get_next_port_endpoint(port, NULL);
 	if (!ep)
 		return 0;
 

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
index 4040e247e026e..efb7d2e4ce85d 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c

@@ -15,72 +15,6 @@
 
 #include "dss.h"
 
-struct device_node *
-omapdss_of_get_next_port(const struct device_node *parent,
-			 struct device_node *prev)
-{
-	struct device_node *port = NULL;
-
-	if (!parent)
-		return NULL;
-
-	if (!prev) {
-		struct device_node *ports;
-		/*
-		 * It's the first call, we have to find a port subnode
-		 * within this node or within an optional 'ports' node.
-		 */
-		ports = of_get_child_by_name(parent, "ports");
-		if (ports)
-			parent = ports;
-
-		port = of_get_child_by_name(parent, "port");
-
-		/* release the 'ports' node */
-		of_node_put(ports);
-	} else {
-		struct device_node *ports;
-
-		ports = of_get_parent(prev);
-		if (!ports)
-			return NULL;
-
-		do {
-			port = of_get_next_child(ports, prev);
-			if (!port) {
-				of_node_put(ports);
-				return NULL;
-			}
-			prev = port;
-		} while (!of_node_name_eq(port, "port"));
-
-		of_node_put(ports);
-	}
-
-	return port;
-}
-EXPORT_SYMBOL_GPL(omapdss_of_get_next_port);
-
-struct device_node *
-omapdss_of_get_next_endpoint(const struct device_node *parent,
-			     struct device_node *prev)
-{
-	struct device_node *ep = NULL;
-
-	if (!parent)
-		return NULL;
-
-	do {
-		ep = of_get_next_child(parent, prev);
-		if (!ep)
-			return NULL;
-		prev = ep;
-	} while (!of_node_name_eq(ep, "endpoint"));
-
-	return ep;
-}
-EXPORT_SYMBOL_GPL(omapdss_of_get_next_endpoint);
-
 struct device_node *dss_of_port_get_parent_device(struct device_node *port)
 {
 	struct device_node *np;

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss.c b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
index d814e4baa4b33..5cab317011eeb 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dss.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dss.c

@@ -26,6 +26,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
 #include <linux/suspend.h>
 #include <linux/component.h>

@@ -922,7 +923,7 @@ static int dss_init_ports(struct platform_device *pdev)
 	if (parent == NULL)
 		return 0;
 
-	port = omapdss_of_get_next_port(parent, NULL);
+	port = of_graph_get_next_port(parent, NULL);
 	if (!port)
 		return 0;
 

@@ -953,7 +954,7 @@ static int dss_init_ports(struct platform_device *pdev)
 			break;
 		}
 	} while (!ret &&
-		 (port = omapdss_of_get_next_port(parent, port)) != NULL);
+		 (port = of_graph_get_next_port(parent, port)) != NULL);
 
 	if (ret)
 		dss_uninit_ports(pdev);

@@ -969,7 +970,7 @@ static void dss_uninit_ports(struct platform_device *pdev)
 	if (parent == NULL)
 		return;
 
-	port = omapdss_of_get_next_port(parent, NULL);
+	port = of_graph_get_next_port(parent, NULL);
 	if (!port)
 		return;
 

@@ -1000,7 +1001,7 @@ static void dss_uninit_ports(struct platform_device *pdev)
 		default:
 			break;
 		}
-	} while ((port = omapdss_of_get_next_port(parent, port)) != NULL);
+	} while ((port = of_graph_get_next_port(parent, port)) != NULL);
 }
 
 static int dss_video_pll_probe(struct platform_device *pdev)

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/sdi.c b/drivers/video/fbdev/omap2/omapfb/dss/sdi.c
index d527931b2b165..22a6243d7abfb 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/sdi.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/sdi.c

@@ -16,6 +16,7 @@
 #include <linux/platform_device.h>
 #include <linux/string.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/component.h>
 
 #include <video/omapfb_dss.h>

@@ -405,7 +406,7 @@ int sdi_init_port(struct platform_device *pdev, struct device_node *port)
 	u32 datapairs;
 	int r;
 
-	ep = omapdss_of_get_next_endpoint(port, NULL);
+	ep = of_graph_get_next_port_endpoint(port, NULL);
 	if (!ep)
 		return 0;
 

diff --git a/include/video/omapfb_dss.h b/include/video/omapfb_dss.h
index a8c0c3eeeb5ba..d133190e31438 100644
--- a/include/video/omapfb_dss.h
+++ b/include/video/omapfb_dss.h

@@ -811,14 +811,6 @@ static inline bool omapdss_device_is_enabled(struct omap_dss_device *dssdev)
 	return dssdev->state == OMAP_DSS_DISPLAY_ACTIVE;
 }
 
-struct device_node *
-omapdss_of_get_next_port(const struct device_node *parent,
-			 struct device_node *prev);
-
-struct device_node *
-omapdss_of_get_next_endpoint(const struct device_node *parent,
-			     struct device_node *prev);
-
 struct omap_dss_device *
 omapdss_of_find_source_for_first_ep(struct device_node *node);
 #else

-- 
2.43.0

Now we can use new port related functions for port parsing. Use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/media/platform/xilinx/xilinx-tpg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/xilinx/xilinx-tpg.c b/drivers/media/platform/xilinx/xilinx-tpg.c
index e05e528ffc6f7..a25f216b2513c 100644
--- a/drivers/media/platform/xilinx/xilinx-tpg.c
+++ b/drivers/media/platform/xilinx/xilinx-tpg.c

@@ -13,6 +13,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/platform_device.h>
 #include <linux/xilinx-v4l2-controls.h>
 

@@ -744,7 +745,7 @@ static int xtpg_parse_of(struct xtpg_device *xtpg)
 		}
 
 		if (nports == 0) {
-			endpoint = of_get_next_child(port, NULL);
+			endpoint = of_graph_get_next_port_endpoint(port, NULL);
 			if (endpoint)
 				has_endpoint = true;
 			of_node_put(endpoint);

-- 
2.43.0

On Mon, Aug 26, 2024 at 02:43:23AM +0000, Kuninori Morimoto wrote:

We have endpoint base functions
	- of_graph_get_next_device_endpoint()
	- of_graph_get_device_endpoint_count()
	- for_each_of_graph_device_endpoint()

+	if (!prev) {
+		/*
+		 * Find "ports" node from parent
+		 *
+		 *	parent {
+		 * =>		ports {
+		 *			port {...};
+		 *		};
+		 *	};
+		 */
+		prev = of_get_child_by_name(parent, "ports");
+
+		/*
+		 * Use parent as its ports if it not exist
+		 *
+		 * =>	parent {
+		 *		port {...};
+		 *	};
+		 */
+		if (!prev) {
+			prev = of_node_get(parent);
+
+			/* check wether it has port node */
+			struct device_node *port __free(device_node) =
+				of_get_child_by_name(prev, "port");
+
+			if (!port)
+				prev = NULL;

It looks like you leak here "prev".

+		}
+
+		return prev;
+	}

Best regards,
Krzysztof

Hi Krzysztof

quoted

+		prev = of_get_child_by_name(parent, "ports");

(snip)

quoted

+		if (!prev) {
+			prev = of_node_get(parent);
+
+			/* check wether it has port node */
+			struct device_node *port __free(device_node) =
+				of_get_child_by_name(prev, "port");
+
+			if (!port)
+				prev = NULL;

It looks like you leak here "prev".

Oops, yes ineed.
Thank you for pointing it, will fix it in v4

Best regards
---
Kuninori Morimoto

On Mon, Aug 26, 2024 at 02:43:28AM +0000, Kuninori Morimoto wrote:

We already have of_graph_get_next_endpoint(), but it is not
intuitive to use in some case.

Can of_graph_get_next_endpoint() users be replaced with your new 
helpers? I'd really like to get rid of the 3 remaining users.

Really, this check is validation as no other name is valid in a 
port node. The kernel is not responsible for validation, but okay. 
However, if we are going to keep this, might as well make it WARN().

No need for this requirement anymore. Use cleanup.h so this is 
automatic.

Hi Rob

quoted

We already have of_graph_get_next_endpoint(), but it is not
intuitive to use in some case.

Can of_graph_get_next_endpoint() users be replaced with your new 
helpers? I'd really like to get rid of the 3 remaining users.

Hmm...
of_graph_get_next_endpoint() will fetch "endpoint" beyond the "port",
but new helper doesn't have such feature.
Even though I try to replace it with new helper, I guess it will be
almost same as current of_graph_get_next_endpoint() anyway.

Alternative idea is...
One of the big user of of_graph_get_next_endpoint() is
for_each_endpoint_of_node() loop.

So we can replace it to..

-	for_each_endpoint_of_node(parent, endpoint) {
+	for_each_of_graph_port(parent, port) {
+		for_each_of_graph_port_endpoint(port, endpoint) {

Above is possible, but it replaces single loop to multi loops.

And, we still need to consider about of_fwnode_graph_get_next_endpoint()
which is the last user of of_graph_get_next_endpoint()

quoted

+struct device_node *of_graph_get_next_port_endpoint(const struct device_node *port,
+						    struct device_node *prev)
+{
+	do {
+		prev = of_get_next_child(port, prev);
+		if (!prev)
+			break;
+	} while (!of_node_name_eq(prev, "endpoint"));

Really, this check is validation as no other name is valid in a 
port node. The kernel is not responsible for validation, but okay. 
However, if we are going to keep this, might as well make it WARN().

OK, will do in v4

quoted

+/**
+ * for_each_of_graph_port_endpoint - iterate over every endpoint in a port node
+ * @parent: parent 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.

No need for this requirement anymore. Use cleanup.h so this is 
automatic.

Do you mean it should include __free() inside this loop, like _scoped() ?

#define for_each_child_of_node_scoped(parent, child) \
	for (struct device_node *child __free(device_node) =		\
	     of_get_next_child(parent, NULL);				\
	     child != NULL;						\
	     child = of_get_next_child(parent, child))

In such case, I wonder does it need to have _scoped() in loop name ?
And in such case, I think we want to have non _scoped() loop too ?
For example, when user want to use the param.

	for_each_of_graph_port_endpoint(port, endpoint)
		if (xxx == yyy)
			return endpoint;

	for_each_of_graph_port_endpoint_scoped(port, endpoint)
		if (xxx == yyy)
			return of_node_get(endpoint)


Thank you for your help !!

Best regards
---
Kuninori Morimoto

Rob, Kunimori-san,

On Mon, Aug 26, 2024 at 10:40:09AM -0500, Rob Herring wrote:

On Mon, Aug 26, 2024 at 02:43:28AM +0000, Kuninori Morimoto wrote:

quoted

We already have of_graph_get_next_endpoint(), but it is not
intuitive to use in some case.

Can of_graph_get_next_endpoint() users be replaced with your new 
helpers? I'd really like to get rid of the 3 remaining users.

The fwnode graph API has fwnode_graph_get_endpoint_by_id() which can also
be used to obtain endpoints within a port. It does the same than
of_graph_get_endpoint_by_regs() with the addition that it also has a
flags field to allow e.g. returning endpoints with regs higher than
requested (FWNODE_GRAPH_ENDPOINT_NEXT).

Most users dealing with endpoints on fwnode property API use this, could
something like this be done on OF as well? Probably a similar flag would be
needed though.

-- 
Kind regards,

Sakari Ailus

+Jonathan C for the naming

On Mon, Aug 26, 2024 at 7:14 PM Kuninori Morimoto
[off-list ref] wrote:

Hi Rob

quoted

quoted

We already have of_graph_get_next_endpoint(), but it is not
intuitive to use in some case.

Can of_graph_get_next_endpoint() users be replaced with your new
helpers? I'd really like to get rid of the 3 remaining users.

Hmm...
of_graph_get_next_endpoint() will fetch "endpoint" beyond the "port",
but new helper doesn't have such feature.

Right, but the "feature" is somewhat awkward as you said. You
generally should know what port you are operating on.

Even though I try to replace it with new helper, I guess it will be
almost same as current of_graph_get_next_endpoint() anyway.

Alternative idea is...
One of the big user of of_graph_get_next_endpoint() is
for_each_endpoint_of_node() loop.

So we can replace it to..

-       for_each_endpoint_of_node(parent, endpoint) {
+       for_each_of_graph_port(parent, port) {
+               for_each_of_graph_port_endpoint(port, endpoint) {

Above is possible, but it replaces single loop to multi loops.

And, we still need to consider about of_fwnode_graph_get_next_endpoint()
which is the last user of of_graph_get_next_endpoint()

I missed fwnode_graph_get_next_endpoint() which has lots of users.
Though almost all of those are just "get the endpoint" and assume
there is only 1. In any case, it's a lot more than 3, so nevermind for
now.

quoted

quoted

+struct device_node *of_graph_get_next_port_endpoint(const struct device_node *port,
+                                               struct device_node *prev)
+{
+   do {
+           prev = of_get_next_child(port, prev);
+           if (!prev)
+                   break;
+   } while (!of_node_name_eq(prev, "endpoint"));

Really, this check is validation as no other name is valid in a
port node. The kernel is not responsible for validation, but okay.
However, if we are going to keep this, might as well make it WARN().

OK, will do in v4

quoted

quoted

+/**
+ * for_each_of_graph_port_endpoint - iterate over every endpoint in a port node
+ * @parent: parent 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.

No need for this requirement anymore. Use cleanup.h so this is
automatic.

Do you mean it should include __free() inside this loop, like _scoped() ?

Yes.

#define for_each_child_of_node_scoped(parent, child) \
        for (struct device_node *child __free(device_node) =            \
             of_get_next_child(parent, NULL);                           \
             child != NULL;                                             \
             child = of_get_next_child(parent, child))

In such case, I wonder does it need to have _scoped() in loop name ?

Well, we added that to avoid changing all the users at once.

And in such case, I think we want to have non _scoped() loop too ?

Do we have a user? I don't think we need it because anywhere we need
the node iterator pointer outside the loop that can be done explicitly
(no_free_ptr()).

So back to the name, I don't think we need _scoped in it. I think if
any user treats the iterator like it's the old style, the compiler is
going to complain.

For example, when user want to use the param.

        for_each_of_graph_port_endpoint(port, endpoint)
                if (xxx == yyy)
                        return endpoint;

        for_each_of_graph_port_endpoint_scoped(port, endpoint)
                if (xxx == yyy)
                        return of_node_get(endpoint)

Actually, you would do "return_ptr(endpoint)" here.

Rob

On Tue, Aug 27, 2024 at 5:47 AM Sakari Ailus [off-list ref] wrote:

Rob, Kunimori-san,

On Mon, Aug 26, 2024 at 10:40:09AM -0500, Rob Herring wrote:

quoted

On Mon, Aug 26, 2024 at 02:43:28AM +0000, Kuninori Morimoto wrote:

quoted

We already have of_graph_get_next_endpoint(), but it is not
intuitive to use in some case.

Can of_graph_get_next_endpoint() users be replaced with your new
helpers? I'd really like to get rid of the 3 remaining users.

The fwnode graph API has fwnode_graph_get_endpoint_by_id() which can also
be used to obtain endpoints within a port. It does the same than
of_graph_get_endpoint_by_regs() with the addition that it also has a
flags field to allow e.g. returning endpoints with regs higher than
requested (FWNODE_GRAPH_ENDPOINT_NEXT).

Looks to me like FWNODE_GRAPH_ENDPOINT_NEXT is always used with
endpoint #0. That's equivalent to passing -1 for the endpoint number
with the OF API.

Most users dealing with endpoints on fwnode property API use this, could
something like this be done on OF as well? Probably a similar flag would be
needed though.

I had fixed almost all the OF cases at one point. Unfortunately, there
were a few corner cases that I didn't address to eliminate the API. So
now it has proliferated with the fwnode API.

Rob

Hi Rob,

On Tue, Aug 27, 2024 at 09:05:02AM -0500, Rob Herring wrote:

On Tue, Aug 27, 2024 at 5:47 AM Sakari Ailus [off-list ref] wrote:

quoted

Rob, Kunimori-san,

On Mon, Aug 26, 2024 at 10:40:09AM -0500, Rob Herring wrote:

quoted

On Mon, Aug 26, 2024 at 02:43:28AM +0000, Kuninori Morimoto wrote:

quoted

We already have of_graph_get_next_endpoint(), but it is not
intuitive to use in some case.

Can of_graph_get_next_endpoint() users be replaced with your new
helpers? I'd really like to get rid of the 3 remaining users.

The fwnode graph API has fwnode_graph_get_endpoint_by_id() which can also
be used to obtain endpoints within a port. It does the same than
of_graph_get_endpoint_by_regs() with the addition that it also has a
flags field to allow e.g. returning endpoints with regs higher than
requested (FWNODE_GRAPH_ENDPOINT_NEXT).

Looks to me like FWNODE_GRAPH_ENDPOINT_NEXT is always used with
endpoint #0. That's equivalent to passing -1 for the endpoint number
with the OF API.

If the caller needs a single endpoint only, then the two are the same, yes.
The NEXT flag can still be used for obtaining further endpoints, unlike
setting endpoint to -1 in of_graph_get_endpoint_by_regs(). 

quoted

Most users dealing with endpoints on fwnode property API use this, could
something like this be done on OF as well? Probably a similar flag would be
needed though.

I had fixed almost all the OF cases at one point. Unfortunately, there
were a few corner cases that I didn't address to eliminate the API. So
now it has proliferated with the fwnode API.

Much of the usage of fwnode_graph_get_next_endpoint() is conversion from
the OF API but there are some newer drivers, too. I admit I've sometimes
missed this in review. At the same time I can say most users in the media
tree do employ fwnode_graph_get_endpoint_by_id() already.

The good thing is that almost all current users are camera sensors and
converting them is fairly trivial. I can post patches but it'll take a
while...

-- 
Kind regards,

Sakari Ailus

Hi Rob

quoted

And, we still need to consider about of_fwnode_graph_get_next_endpoint()
which is the last user of of_graph_get_next_endpoint()

I missed fwnode_graph_get_next_endpoint() which has lots of users.
Though almost all of those are just "get the endpoint" and assume
there is only 1. In any case, it's a lot more than 3, so nevermind for
now.

OK, thanks.

So back to the name, I don't think we need _scoped in it. I think if
any user treats the iterator like it's the old style, the compiler is
going to complain.

quoted

For example, when user want to use the param.

        for_each_of_graph_port_endpoint(port, endpoint)
                if (xxx == yyy)
                        return endpoint;

        for_each_of_graph_port_endpoint_scoped(port, endpoint)
                if (xxx == yyy)
                        return of_node_get(endpoint)

Actually, you would do "return_ptr(endpoint)" here.

OK, nice to know about this
I will try to use this style on v4


Thank you for your help !!

Best regards
---
Kuninori Morimoto

On Tue, 27 Aug 2024 08:54:51 -0500
Rob Herring [off-list ref] wrote:

+Jonathan C for the naming

On Mon, Aug 26, 2024 at 7:14 PM Kuninori Morimoto
[off-list ref] wrote:

quoted

Hi Rob
 

quoted

quoted

We already have of_graph_get_next_endpoint(), but it is not
intuitive to use in some case.  

Can of_graph_get_next_endpoint() users be replaced with your new
helpers? I'd really like to get rid of the 3 remaining users.  

Hmm...
of_graph_get_next_endpoint() will fetch "endpoint" beyond the "port",
but new helper doesn't have such feature.  

Right, but the "feature" is somewhat awkward as you said. You
generally should know what port you are operating on.

quoted

Even though I try to replace it with new helper, I guess it will be
almost same as current of_graph_get_next_endpoint() anyway.

Alternative idea is...
One of the big user of of_graph_get_next_endpoint() is
for_each_endpoint_of_node() loop.

So we can replace it to..

-       for_each_endpoint_of_node(parent, endpoint) {
+       for_each_of_graph_port(parent, port) {
+               for_each_of_graph_port_endpoint(port, endpoint) {

Above is possible, but it replaces single loop to multi loops.

And, we still need to consider about of_fwnode_graph_get_next_endpoint()
which is the last user of of_graph_get_next_endpoint()  

I missed fwnode_graph_get_next_endpoint() which has lots of users.
Though almost all of those are just "get the endpoint" and assume
there is only 1. In any case, it's a lot more than 3, so nevermind for
now.

quoted

quoted

quoted

+struct device_node *of_graph_get_next_port_endpoint(const struct device_node *port,
+                                               struct device_node *prev)
+{
+   do {
+           prev = of_get_next_child(port, prev);
+           if (!prev)
+                   break;
+   } while (!of_node_name_eq(prev, "endpoint"));  

Really, this check is validation as no other name is valid in a
port node. The kernel is not responsible for validation, but okay.
However, if we are going to keep this, might as well make it WARN().  

OK, will do in v4
 

quoted

quoted

+/**
+ * for_each_of_graph_port_endpoint - iterate over every endpoint in a port node
+ * @parent: parent 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.  

No need for this requirement anymore. Use cleanup.h so this is
automatic.  

Do you mean it should include __free() inside this loop, like _scoped() ?  

Yes.

quoted

#define for_each_child_of_node_scoped(parent, child) \
        for (struct device_node *child __free(device_node) =            \
             of_get_next_child(parent, NULL);                           \
             child != NULL;                                             \
             child = of_get_next_child(parent, child))

In such case, I wonder does it need to have _scoped() in loop name ?  

Well, we added that to avoid changing all the users at once.

quoted

And in such case, I think we want to have non _scoped() loop too ?  

Do we have a user? I don't think we need it because anywhere we need
the node iterator pointer outside the loop that can be done explicitly
(no_free_ptr()).

So back to the name, I don't think we need _scoped in it. I think if
any user treats the iterator like it's the old style, the compiler is
going to complain.

Hmm.  Up to you but I'd be concerned that the scoping stuff is non
obvious enough that it is worth making people really really aware
it is going on.

However I don't feel strongly about it.
For the other _scoped iterators there is some push back
on the churn using them is causing so I doubt we'll ever get rid
of the non scoped variants.  For something new that's not a concern.

Jonathan

quoted

For example, when user want to use the param.

        for_each_of_graph_port_endpoint(port, endpoint)
                if (xxx == yyy)
                        return endpoint;

        for_each_of_graph_port_endpoint_scoped(port, endpoint)
                if (xxx == yyy)
                        return of_node_get(endpoint)  

Actually, you would do "return_ptr(endpoint)" here.

Rob

Hi Jonathan, Rob

quoted

quoted

Do you mean it should include __free() inside this loop, like _scoped() ?

(snip)

quoted

quoted

In such case, I wonder does it need to have _scoped() in loop name ?

(snip)

quoted

So back to the name, I don't think we need _scoped in it. I think if
any user treats the iterator like it's the old style, the compiler is
going to complain.

Hmm.  Up to you but I'd be concerned that the scoping stuff is non
obvious enough that it is worth making people really really aware
it is going on.

However I don't feel strongly about it.
For the other _scoped iterators there is some push back
on the churn using them is causing so I doubt we'll ever get rid
of the non scoped variants.  For something new that's not a concern.

I noticed that we can write below code, and then, and there is no waning/error
from compiler.

Now for_each macro is using __free()

	#define for_each_of_graph_port(parent, child)	\
		for (... *child __free(device_node) = ...)

(A)	struct device_node *node = xxx;

	for_each_of_graph_port(parent, node) {
(B)		/* do something */
	}

(C)	xxx = node;

In this case, "(A) node" and "(C) node" are same, but "(B) node" are different.
New user might confuse about this behavior.

Thank you for your help !!

Best regards
---
Kuninori Morimoto