[PATCH v2 00/13] of: property: add port base loop

[PATCH v2 00/13] of: property: add port base loop

[Kuninori Morimoto]

Hi Rob

This is v2 of port base loop patch-set

We have endpoint base functions
	- of_graph_get_next_endpoint()
	- of_graph_get_endpoint_count()
	- for_each_endpoint_of_node()

But to handling "port" base things, it is not useful. We want to have
"port" base functions, too. This patch-set adds it.

Because current existing drivers couldn't use "port" base functions,
it were implemented in a different way. This patch-set doesn't try
to full-replace to avoid unknown bug, try easy / quick replace only
for now, but easy to know how "port" base functions are needed.

Because I can't test the driver which I can't use, non-ASoC drivers
needs Tested-by, Acked-by.

v1 -> v2
	- tidyup function explain
	- add missing header on each files

Kuninori Morimoto (13):
  of: property: add port base loop
  of: property: use of_graph_get_next_port() on of_graph_get_next_endpoint()
  of: property: add of_graph_get_next_endpoint_raw()
  drm: omapdrm: use of_graph_get_next_endpoint_raw()
  media: xilinx-tpg: use of_graph_get_next_endpoint_raw()
  ASoC: audio-graph-card: use of_graph_get_next_endpoint_raw()
  ASoC: audio-graph-card2: use of_graph_get_next_port()
  ASoC: audio-graph-card2: use of_graph_get_next_endpoint_raw()
  ASoC: test-component: use for_each_port_of_node()
  fbdev: omapfb: use of_graph_get_remote_port()
  fbdev: omapfb: use of_graph_get_next_port()
  fbdev: omapfb: use of_graph_get_next_endpoint_raw()
  fbdev: omapfb: use of_graph_get_next_endpoint()

 drivers/gpu/drm/omapdrm/dss/dpi.c             |   2 +-
 drivers/gpu/drm/omapdrm/dss/sdi.c             |   2 +-
 drivers/media/platform/xilinx/xilinx-tpg.c    |   2 +-
 drivers/of/property.c                         |  92 +++++++++++++---
 drivers/video/fbdev/omap2/omapfb/dss/dpi.c    |   3 +-
 drivers/video/fbdev/omap2/omapfb/dss/dsi.c    |   3 +-
 drivers/video/fbdev/omap2/omapfb/dss/dss-of.c | 101 +-----------------
 drivers/video/fbdev/omap2/omapfb/dss/dss.c    |   9 +-
 drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c  |   3 +-
 drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c  |   3 +-
 drivers/video/fbdev/omap2/omapfb/dss/sdi.c    |   3 +-
 drivers/video/fbdev/omap2/omapfb/dss/venc.c   |   3 +-
 include/linux/of_graph.h                      |  23 ++++
 include/video/omapfb_dss.h                    |  11 --
 sound/soc/generic/audio-graph-card.c          |   2 +-
 sound/soc/generic/audio-graph-card2.c         |  31 ++----
 sound/soc/generic/test-component.c            |   2 +-
 17 files changed, 133 insertions(+), 162 deletions(-)

-- 
2.25.1

[PATCH v2 01/13] of: property: add port base loop

[Kuninori Morimoto]

We have endpoint base functions
	- of_graph_get_next_endpoint()
	- of_graph_get_endpoint_count()
	- for_each_endpoint_of_node()

Here, for_each_endpoint_of_node() loop finds each endpoints

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

In above case, for_each_endpoint_of_node() loop 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" by
for_each_endpoint_of_node(). 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 {...};
		};
		...
	};

Add "port" base functions.

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

diff --git a/drivers/of/property.c b/drivers/of/property.c
index afdaefbd03f6..9e670e99dbbb 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -631,6 +631,42 @@ 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_port() - get next port node
+ * @parent: pointer to the parent device node
+ * @port: current port node, or NULL to get first
+ *
+ * Return: An 'port' node pointer with refcount incremented. Refcount
+ * of the passed @prev node is decremented.
+ */
+struct device_node *of_graph_get_next_port(const struct device_node *parent,
+					   struct device_node *port)
+{
+	if (!parent)
+		return NULL;
+
+	if (!port) {
+		struct device_node *node;
+
+		node = of_get_child_by_name(parent, "ports");
+		if (node) {
+			parent = node;
+			of_node_put(node);
+		}
+
+		return of_get_child_by_name(parent, "port");
+	}
+
+	do {
+		port = of_get_next_child(parent, port);
+		if (!port)
+			break;
+	} while (!of_node_name_eq(port, "port"));
+
+	return port;
+}
+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 +859,18 @@ int of_graph_get_endpoint_count(const struct device_node *np)
 }
 EXPORT_SYMBOL(of_graph_get_endpoint_count);
 
+int of_graph_get_port_count(const struct device_node *np)
+{
+	struct device_node *port;
+	int num = 0;
+
+	for_each_port_of_node(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 4d7756087b6b..fff598640e93 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -37,14 +37,28 @@ struct of_endpoint {
 	for (child = of_graph_get_next_endpoint(parent, NULL); child != NULL; \
 	     child = of_graph_get_next_endpoint(parent, child))
 
+/**
+ * for_each_port_of_node - iterate over every port in a device node
+ * @parent: parent device node containing ports/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_port_of_node(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);
 int of_graph_get_endpoint_count(const struct device_node *np);
+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_endpoint(const struct device_node *parent,
 					struct device_node *previous);
+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(
 		const struct device_node *parent, int port_reg, int reg);
 struct device_node *of_graph_get_remote_endpoint(
@@ -86,6 +100,13 @@ static inline struct device_node *of_graph_get_next_endpoint(
 	return NULL;
 }
 
+static inline struct device_node *of_graph_get_next_port(
+					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.25.1

Re: [PATCH v2 01/13] of: property: add port base loop

[Tomi Valkeinen]

Hi,

On 29/01/2024 02:54, Kuninori Morimoto wrote:
> We have endpoint base functions
> 	- of_graph_get_next_endpoint()
> 	- of_graph_get_endpoint_count()
> 	- for_each_endpoint_of_node()
> 
> Here, for_each_endpoint_of_node() loop finds each endpoints
> 
> 	ports {
> 		port@0 {
> (1)			endpoint {...};
> 		};
> 		port@1 {
> (2)			endpoint {...};
> 		};
> 		...
> 	};
> 
> In above case, for_each_endpoint_of_node() loop 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" by
> for_each_endpoint_of_node(). 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 {...};
> 		};
> 		...
> 	};
> 
> Add "port" base functions.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>   drivers/of/property.c    | 48 ++++++++++++++++++++++++++++++++++++++++
>   include/linux/of_graph.h | 21 ++++++++++++++++++
>   2 files changed, 69 insertions(+)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index afdaefbd03f6..9e670e99dbbb 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -631,6 +631,42 @@ 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_port() - get next port node
> + * @parent: pointer to the parent device node
> + * @port: current port node, or NULL to get first
> + *
> + * Return: An 'port' node pointer with refcount incremented. Refcount

"A 'port'".

> + * of the passed @prev node is decremented.
> + */
> +struct device_node *of_graph_get_next_port(const struct device_node *parent,
> +					   struct device_node *port)
> +{
> +	if (!parent)
> +		return NULL;
> +
> +	if (!port) {
> +		struct device_node *node;
> +
> +		node = of_get_child_by_name(parent, "ports");
> +		if (node) {
> +			parent = node;
> +			of_node_put(node);

Here you of_node_put() the node, but use it below.

> +		}
> +
> +		return of_get_child_by_name(parent, "port");
> +	}

Maybe you can do:

	node = of_get_child_by_name(parent, "ports");
	if (node)
		parent = node;
	port = of_get_child_by_name(parent, "port");
	of_node_put(node);
	return port;

> +
> +	do {
> +		port = of_get_next_child(parent, port);
> +		if (!port)
> +			break;
> +	} while (!of_node_name_eq(port, "port"));
> +
> +	return port;
> +}
> +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 +859,18 @@ int of_graph_get_endpoint_count(const struct device_node *np)
>   }
>   EXPORT_SYMBOL(of_graph_get_endpoint_count);
>   
> +int of_graph_get_port_count(const struct device_node *np)

The kerneldoc is missing for this func.

The return type and the variable below should be unsigned.

I can see these are wrong with of_graph_get_endpoint_count() too, so 
maybe that should be fixed also.

> +{
> +	struct device_node *port;
> +	int num = 0;
> +
> +	for_each_port_of_node(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 4d7756087b6b..fff598640e93 100644
> --- a/include/linux/of_graph.h
> +++ b/include/linux/of_graph.h
> @@ -37,14 +37,28 @@ struct of_endpoint {
>   	for (child = of_graph_get_next_endpoint(parent, NULL); child != NULL; \
>   	     child = of_graph_get_next_endpoint(parent, child))
>   
> +/**
> + * for_each_port_of_node - iterate over every port in a device node
> + * @parent: parent device node containing ports/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_port_of_node(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);
>   int of_graph_get_endpoint_count(const struct device_node *np);
> +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_endpoint(const struct device_node *parent,
>   					struct device_node *previous);
> +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(
>   		const struct device_node *parent, int port_reg, int reg);
>   struct device_node *of_graph_get_remote_endpoint(
> @@ -86,6 +100,13 @@ static inline struct device_node *of_graph_get_next_endpoint(
>   	return NULL;
>   }
>   
> +static inline struct device_node *of_graph_get_next_port(
> +					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)
>   {

Re: [PATCH v2 01/13] of: property: add port base loop

[Rob Herring]

On Mon, Jan 29, 2024 at 12:54:44AM +0000, Kuninori Morimoto wrote:
> We have endpoint base functions
> 	- of_graph_get_next_endpoint()
> 	- of_graph_get_endpoint_count()
> 	- for_each_endpoint_of_node()
> 
> Here, for_each_endpoint_of_node() loop finds each endpoints
> 
> 	ports {
> 		port@0 {
> (1)			endpoint {...};
> 		};
> 		port@1 {
> (2)			endpoint {...};
> 		};
> 		...
> 	};
> 
> In above case, for_each_endpoint_of_node() loop 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" by
> for_each_endpoint_of_node(). 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 {...};
> 		};
> 		...
> 	};
> 
> Add "port" base functions.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/of/property.c    | 48 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_graph.h | 21 ++++++++++++++++++
>  2 files changed, 69 insertions(+)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index afdaefbd03f6..9e670e99dbbb 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -631,6 +631,42 @@ 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_port() - get next port node
> + * @parent: pointer to the parent device node
> + * @port: current port node, or NULL to get first
> + *
> + * Return: An 'port' node pointer with refcount incremented. Refcount
> + * of the passed @prev node is decremented.
> + */
> +struct device_node *of_graph_get_next_port(const struct device_node *parent,
> +					   struct device_node *port)
> +{
> +	if (!parent)
> +		return NULL;
> +
> +	if (!port) {
> +		struct device_node *node;
> +
> +		node = of_get_child_by_name(parent, "ports");
> +		if (node) {
> +			parent = node;
> +			of_node_put(node);

The original code had this right, but here you have it wrong.

You are releasing ports here, but then using it...

> +		}
> +
> +		return of_get_child_by_name(parent, "port");

...here. You have to get the child before you can put the parent.

> +	}
> +
> +	do {
> +		port = of_get_next_child(parent, port);
> +		if (!port)
> +			break;
> +	} while (!of_node_name_eq(port, "port"));
> +
> +	return port;
> +}
> +EXPORT_SYMBOL(of_graph_get_next_port);
> +

Re: [PATCH v2 01/13] of: property: add port base loop

[Kuninori Morimoto]

Hi Rob

Thank you for review

> > +/**
> > + * of_graph_get_next_port() - get next port node
> > + * @parent: pointer to the parent device node
> > + * @port: current port node, or NULL to get first
> > + *
> > + * Return: An 'port' node pointer with refcount incremented. Refcount
> > + * of the passed @prev node is decremented.
> > + */
> > +struct device_node *of_graph_get_next_port(const struct device_node *parent,
> > +					   struct device_node *port)
> > +{
> > +	if (!parent)
> > +		return NULL;
> > +
> > +	if (!port) {
> > +		struct device_node *node;
> > +
> > +		node = of_get_child_by_name(parent, "ports");
> > +		if (node) {
> > +			parent = node;
> > +			of_node_put(node);
> 
> The original code had this right, but here you have it wrong.
> 
> You are releasing ports here, but then using it...
> 
> > +		}
> > +
> > +		return of_get_child_by_name(parent, "port");
> 
> ...here. You have to get the child before you can put the parent.

You are reviewing v2, and it was already fixed in v3

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH v2 01/13] of: property: add port base loop

[Rob Herring]

On Mon, Jan 29, 2024 at 12:54:44AM +0000, Kuninori Morimoto wrote:
> We have endpoint base functions
> 	- of_graph_get_next_endpoint()
> 	- of_graph_get_endpoint_count()
> 	- for_each_endpoint_of_node()

I also noticed that your mail setup has an issue. You have some utf-8 
encoded email names, but the headers say it is ascii. git-send-email 
should do the right thing here, but maybe Exchange is messing things up.

Rob

[PATCH v2 02/13] of: property: use of_graph_get_next_port() on of_graph_get_next_endpoint()

[Kuninori Morimoto]

We have of_graph_get_next_port(), use it on of_graph_get_next_endpoint().

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/of/property.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 9e670e99dbbb..14ffd199c9b1 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -690,15 +690,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 	 * parent port node.
 	 */
 	if (!prev) {
-		struct device_node *node;
-
-		node = of_get_child_by_name(parent, "ports");
-		if (node)
-			parent = node;
-
-		port = of_get_child_by_name(parent, "port");
-		of_node_put(node);
-
+		port = of_graph_get_next_port(parent, NULL);
 		if (!port) {
 			pr_err("graph: no port node found in %pOF\n", parent);
 			return NULL;
@@ -725,11 +717,9 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 		/* No more endpoints under this port, try the next one. */
 		prev = NULL;
 
-		do {
-			port = of_get_next_child(parent, port);
-			if (!port)
-				return NULL;
-		} while (!of_node_name_eq(port, "port"));
+		port = of_graph_get_next_port(parent, port);
+		if (!port)
+			return NULL;
 	}
 }
 EXPORT_SYMBOL(of_graph_get_next_endpoint);
-- 
2.25.1

Re: [PATCH v2 02/13] of: property: use of_graph_get_next_port() on of_graph_get_next_endpoint()

[Tomi Valkeinen]

On 29/01/2024 02:54, Kuninori Morimoto wrote:
> We have of_graph_get_next_port(), use it on of_graph_get_next_endpoint().
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>   drivers/of/property.c | 18 ++++--------------
>   1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 9e670e99dbbb..14ffd199c9b1 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -690,15 +690,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
>   	 * parent port node.
>   	 */
>   	if (!prev) {
> -		struct device_node *node;
> -
> -		node = of_get_child_by_name(parent, "ports");
> -		if (node)
> -			parent = node;
> -
> -		port = of_get_child_by_name(parent, "port");
> -		of_node_put(node);
> -
> +		port = of_graph_get_next_port(parent, NULL);
>   		if (!port) {
>   			pr_err("graph: no port node found in %pOF\n", parent);
>   			return NULL;
> @@ -725,11 +717,9 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
>   		/* No more endpoints under this port, try the next one. */
>   		prev = NULL;
>   
> -		do {
> -			port = of_get_next_child(parent, port);
> -			if (!port)
> -				return NULL;
> -		} while (!of_node_name_eq(port, "port"));
> +		port = of_graph_get_next_port(parent, port);
> +		if (!port)
> +			return NULL;
>   	}
>   }
>   EXPORT_SYMBOL(of_graph_get_next_endpoint);

Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

  Tomi

[PATCH v2 03/13] of: property: add of_graph_get_next_endpoint_raw()

[Kuninori Morimoto]

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
+ * @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_raw(const struct device_node *port,
+						   struct device_node *endpoint)
+{
+	if (!port)
+		return NULL;
+
+	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_raw);
+
 /**
  * of_graph_get_next_endpoint() - get next endpoint node
  * @parent: pointer to the parent device node
@@ -708,7 +732,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 		 * 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_raw(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 fff598640e93..427905a6e8c3 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -57,6 +57,8 @@ 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_endpoint(const struct device_node *parent,
 					struct device_node *previous);
+struct device_node *of_graph_get_next_endpoint_raw(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(
-- 
2.25.1

Re: [PATCH v2 03/13] of: property: add of_graph_get_next_endpoint_raw()

[Tomi Valkeinen]

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()"?

> + * @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.
> + */

It might be good to highlight here the difference to the 
of_graph_get_next_endpoint().

> +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port,
> +						   struct device_node *endpoint)
> +{
> +	if (!port)
> +		return NULL;
> +
> +	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_raw);
> +
>   /**
>    * of_graph_get_next_endpoint() - get next endpoint node
>    * @parent: pointer to the parent device node
> @@ -708,7 +732,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
>   		 * 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_raw(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 fff598640e93..427905a6e8c3 100644
> --- a/include/linux/of_graph.h
> +++ b/include/linux/of_graph.h
> @@ -57,6 +57,8 @@ 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_endpoint(const struct device_node *parent,
>   					struct device_node *previous);
> +struct device_node *of_graph_get_next_endpoint_raw(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(

Re: [PATCH v2 03/13] of: property: add of_graph_get_next_endpoint_raw()

[Laurent Pinchart]

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.

> > + * @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.
> > + */
> 
> It might be good to highlight here the difference to the 
> of_graph_get_next_endpoint().

Yes, and the documentation of of_graph_get_next_endpoint() shoul also be
improved.

> > +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port,
> > +						   struct device_node *endpoint)
> > +{
> > +	if (!port)
> > +		return NULL;
> > +
> > +	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_raw);
> > +
> >   /**
> >    * of_graph_get_next_endpoint() - get next endpoint node
> >    * @parent: pointer to the parent device node
> > @@ -708,7 +732,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
> >   		 * 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_raw(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 fff598640e93..427905a6e8c3 100644
> > --- a/include/linux/of_graph.h
> > +++ b/include/linux/of_graph.h
> > @@ -57,6 +57,8 @@ 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_endpoint(const struct device_node *parent,
> >   					struct device_node *previous);
> > +struct device_node *of_graph_get_next_endpoint_raw(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(

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 03/13] of: property: add of_graph_get_next_endpoint_raw()

[Kuninori Morimoto]

Hi Laurent, Tomi

Thank you for your review

> > > +/**
> > > + * 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.

To be honest, from intuitive function naming point of view,
I prefer rename existing function name. But yes, it will be big patch.

Current of_graph_get_next_endpoint() will get next endpoint
beyond the port (A)
New function is not get next endpoint beyond the port (B)

Something like

(A) of_graph_get_next_endpoint() -> of_graph_get_next_port_endpoint()
(B)                                 of_graph_get_next_endpoint()

> > > + * @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.
> > > + */
> > 
> > It might be good to highlight here the difference to the 
> > of_graph_get_next_endpoint().
> 
> Yes, and the documentation of of_graph_get_next_endpoint() shoul also be
> improved.

Yes, Indeed.




Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH v2 03/13] of: property: add of_graph_get_next_endpoint_raw()

[Rob Herring]

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

Re: [PATCH v2 03/13] of: property: add of_graph_get_next_endpoint_raw()

[Kuninori Morimoto]

Hi Rob

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

I see.
I will split this patch-set to like below

	- patch-set for reduce/remove to using current next_endpoint()
	- patch-set for rename current next_endpoint() to next_device_endpoint()
	- patch-set for adding new next_port_endpoint()


Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH v2 03/13] of: property: add of_graph_get_next_endpoint_raw()

[Kuninori Morimoto]

Hi Rob

> > #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))

Hmm... I noticed it is impossible so for.
of_graph_get_endpoint_by_regs() (A) is based on for_each_endpoint_of_node() (B).
Thus, we can't replace for_each_endpoint_of_node() (B) with by_regs (A)

(A)	struct device_node *of_graph_get_endpoint_by_regs(...)
	{
		...
(B)		for_each_endpoint_of_node(parent, node) {
			...
		}

		return NULL;
	}

> 	- patch-set for reduce/remove to using current next_endpoint()
> 	- patch-set for rename current next_endpoint() to next_device_endpoint()
> 	- patch-set for adding new next_port_endpoint()

So, maybe we can do is,

	0) rename current endpoint functions to device_endpoint

	1) add new port base functions (port_endpoint) which has
	   for_each_of_graph_port_endpoint() loop. It is for port base endpoint loop
	   (I want to use new naming, using of_graph instead of _of_node).

	2) replace above (B) part with port base loops 

	-	for_each_endpoint_of_node(parent, node) {
	+	for_each_of_gprah_port(parent, port) {
	+		for_each_of_graph_port_endpoint(port, endpoint) {

	3) replace current next_endpoint() by next_endpoint_by_regs(),
	   and remove next_endpoint()

What do you think ?

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH v2 03/13] of: property: add of_graph_get_next_endpoint_raw()

[Sakari Ailus]

Hi Morimoto-san,

On Mon, Jan 29, 2024 at 12:54:59AM +0000, 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
> + * @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_raw(const struct device_node *port,
> +						   struct device_node *endpoint)
> +{
> +	if (!port)
> +		return NULL;

of_get_next_child() returns NULL if node is NULL, hence there's no need to
check this.

> +
> +	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_raw);
> +
>  /**
>   * of_graph_get_next_endpoint() - get next endpoint node
>   * @parent: pointer to the parent device node
> @@ -708,7 +732,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
>  		 * 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_raw(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 fff598640e93..427905a6e8c3 100644
> --- a/include/linux/of_graph.h
> +++ b/include/linux/of_graph.h
> @@ -57,6 +57,8 @@ 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_endpoint(const struct device_node *parent,
>  					struct device_node *previous);
> +struct device_node *of_graph_get_next_endpoint_raw(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(

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 03/13] of: property: add of_graph_get_next_endpoint_raw()

[Kuninori Morimoto]

Hi Sakari

> > +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port,
> > +						   struct device_node *endpoint)
> > +{
> > +	if (!port)
> > +		return NULL;
> 
> of_get_next_child() returns NULL if node is NULL, hence there's no need to
> check this.

Thanks, will fix in v3


Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

[PATCH v2 04/13] drm: omapdrm: use of_graph_get_next_endpoint_raw()

[Kuninori Morimoto]

We can now use of_graph_get_next_endpoint_raw(), let's use it.

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

diff --git a/drivers/gpu/drm/omapdrm/dss/dpi.c b/drivers/gpu/drm/omapdrm/dss/dpi.c
index 030f997eccd0..edcf7f4fb149 100644
--- a/drivers/gpu/drm/omapdrm/dss/dpi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dpi.c
@@ -709,7 +709,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_endpoint_raw(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 91eaae3b9481..0308dfc00058 100644
--- a/drivers/gpu/drm/omapdrm/dss/sdi.c
+++ b/drivers/gpu/drm/omapdrm/dss/sdi.c
@@ -346,7 +346,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_endpoint_raw(port, NULL);
 	if (!ep) {
 		r = 0;
 		goto err_free;
-- 
2.25.1

[PATCH v2 05/13] media: xilinx-tpg: use of_graph_get_next_endpoint_raw()

[Kuninori Morimoto]

We can now use of_graph_get_next_endpoint_raw(), let's use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/media/platform/xilinx/xilinx-tpg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/xilinx/xilinx-tpg.c b/drivers/media/platform/xilinx/xilinx-tpg.c
index 80353ca44402..97908533466c 100644
--- a/drivers/media/platform/xilinx/xilinx-tpg.c
+++ b/drivers/media/platform/xilinx/xilinx-tpg.c
@@ -745,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_endpoint_raw(port, NULL);
 			if (endpoint)
 				has_endpoint = true;
 			of_node_put(endpoint);
-- 
2.25.1

Re: [PATCH v2 05/13] media: xilinx-tpg: use of_graph_get_next_endpoint_raw()

[kernel test robot]

Hi Kuninori,

kernel test robot noticed the following build errors:

[auto build test ERROR on broonie-sound/for-next]
[also build test ERROR on drm-misc/drm-misc-next linus/master v6.8-rc2 next-20240130]
[cannot apply to robh/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kuninori-Morimoto/of-property-add-port-base-loop/20240129-085726
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
patch link:    https://lore.kernel.org/r/878r49klg1.wl-kuninori.morimoto.gx%40renesas.com
patch subject: [PATCH v2 05/13] media: xilinx-tpg: use of_graph_get_next_endpoint_raw()
config: mips-randconfig-r113-20240130 (https://download.01.org/0day-ci/archive/20240130/202401302148.K0ZR110q-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce: (https://download.01.org/0day-ci/archive/20240130/202401302148.K0ZR110q-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401302148.K0ZR110q-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

>> drivers/media/platform/xilinx/xilinx-tpg.c:747:15: error: implicit declaration of function 'of_graph_get_next_endpoint_raw' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
                           endpoint = of_graph_get_next_endpoint_raw(port, NULL);
                                      ^
   drivers/media/platform/xilinx/xilinx-tpg.c:747:15: note: did you mean 'acpi_graph_get_next_endpoint'?
   include/linux/acpi.h:1409:1: note: 'acpi_graph_get_next_endpoint' declared here
   acpi_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
   ^
>> drivers/media/platform/xilinx/xilinx-tpg.c:747:13: warning: incompatible integer to pointer conversion assigning to 'struct device_node *' from 'int' [-Wint-conversion]
                           endpoint = of_graph_get_next_endpoint_raw(port, NULL);
                                    ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning and 1 error generated.


vim +/of_graph_get_next_endpoint_raw +747 drivers/media/platform/xilinx/xilinx-tpg.c

   705	
   706	/* -----------------------------------------------------------------------------
   707	 * Platform Device Driver
   708	 */
   709	
   710	static int xtpg_parse_of(struct xtpg_device *xtpg)
   711	{
   712		struct device *dev = xtpg->xvip.dev;
   713		struct device_node *node = xtpg->xvip.dev->of_node;
   714		struct device_node *ports;
   715		struct device_node *port;
   716		unsigned int nports = 0;
   717		bool has_endpoint = false;
   718	
   719		ports = of_get_child_by_name(node, "ports");
   720		if (ports == NULL)
   721			ports = node;
   722	
   723		for_each_child_of_node(ports, port) {
   724			const struct xvip_video_format *format;
   725			struct device_node *endpoint;
   726	
   727			if (!of_node_name_eq(port, "port"))
   728				continue;
   729	
   730			format = xvip_of_get_format(port);
   731			if (IS_ERR(format)) {
   732				dev_err(dev, "invalid format in DT");
   733				of_node_put(port);
   734				return PTR_ERR(format);
   735			}
   736	
   737			/* Get and check the format description */
   738			if (!xtpg->vip_format) {
   739				xtpg->vip_format = format;
   740			} else if (xtpg->vip_format != format) {
   741				dev_err(dev, "in/out format mismatch in DT");
   742				of_node_put(port);
   743				return -EINVAL;
   744			}
   745	
   746			if (nports == 0) {
 > 747				endpoint = of_graph_get_next_endpoint_raw(port, NULL);
   748				if (endpoint)
   749					has_endpoint = true;
   750				of_node_put(endpoint);
   751			}
   752	
   753			/* Count the number of ports. */
   754			nports++;
   755		}
   756	
   757		if (nports != 1 && nports != 2) {
   758			dev_err(dev, "invalid number of ports %u\n", nports);
   759			return -EINVAL;
   760		}
   761	
   762		xtpg->npads = nports;
   763		if (nports == 2 && has_endpoint)
   764			xtpg->has_input = true;
   765	
   766		return 0;
   767	}
   768	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2 06/13] ASoC: audio-graph-card: use of_graph_get_next_endpoint_raw()

[Kuninori Morimoto]

We can now use of_graph_get_next_endpoint_raw(), let's use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/generic/audio-graph-card.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c
index 83e3ba773fbd..29bd7c234fed 100644
--- a/sound/soc/generic/audio-graph-card.c
+++ b/sound/soc/generic/audio-graph-card.c
@@ -344,7 +344,7 @@ static int __graph_for_each_link(struct simple_util_priv *priv,
 
 		/* loop for all CPU endpoint */
 		while (1) {
-			cpu_ep = of_get_next_child(cpu_port, cpu_ep);
+			cpu_ep = of_graph_get_next_endpoint_raw(cpu_port, cpu_ep);
 			if (!cpu_ep)
 				break;
 
-- 
2.25.1

[PATCH v2 07/13] ASoC: audio-graph-card2: use of_graph_get_next_port()

[Kuninori Morimoto]

Now we can use of_graph_get_next_port() for port parsing.
Use it on audio-graph-card2 driver.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/generic/audio-graph-card2.c | 29 ++++++++-------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/sound/soc/generic/audio-graph-card2.c b/sound/soc/generic/audio-graph-card2.c
index 62606e20be9a..59a5db12bb5c 100644
--- a/sound/soc/generic/audio-graph-card2.c
+++ b/sound/soc/generic/audio-graph-card2.c
@@ -339,12 +339,7 @@ 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);
 		rep = of_graph_get_remote_endpoint(ep);
@@ -539,7 +534,8 @@ 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(of_get_parent(mcpu_port), NULL);
+	struct device_node *mcpu_ports		= of_get_parent(mcpu_port);
+	struct device_node *mcpu_port_top	= of_graph_get_next_port(mcpu_ports, NULL);
 	struct device_node *mcpu_ep_top		= port_to_endpoint(mcpu_port_top);
 	struct device_node *mcodec_ep_top	= of_graph_get_remote_endpoint(mcpu_ep_top);
 	struct device_node *mcodec_port_top	= of_get_parent(mcodec_ep_top);
@@ -572,12 +568,12 @@ static int graph_parse_node_multi_nm(struct snd_soc_dai_link *dai_link,
 			goto mcpu_err;
 
 		codec_idx = 0;
-		mcodec_port_i = of_get_next_child(mcodec_ports, NULL);
+		mcodec_port_i = of_graph_get_next_port(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);
+			mcodec_port_i = of_graph_get_next_port(mcodec_ports, mcodec_port_i);
 
 			if (!mcodec_port_i)
 				goto mcodec_err;
@@ -967,7 +963,7 @@ int audio_graph2_link_c2c(struct simple_util_priv *priv,
 	of_node_get(lnk);
 	port0 = lnk;
 	ports = of_get_parent(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.
@@ -1099,21 +1095,12 @@ static int graph_counter(struct device_node *lnk)
 	 */
 	if (graph_lnk_is_multi(lnk)) {
 		struct device_node *ports = of_get_parent(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
@@ -1197,7 +1184,7 @@ static int graph_count_c2c(struct simple_util_priv *priv,
 {
 	struct device_node *ports = of_get_parent(lnk);
 	struct device_node *port0 = lnk;
-	struct device_node *port1 = of_get_next_child(ports, lnk);
+	struct device_node *port1 = of_graph_get_next_port(ports, port0);
 	struct device_node *ep0 = port_to_endpoint(port0);
 	struct device_node *ep1 = port_to_endpoint(port1);
 	struct device_node *codec0 = of_graph_get_remote_port(ep0);
-- 
2.25.1

[PATCH v2 08/13] ASoC: audio-graph-card2: use of_graph_get_next_endpoint_raw()

[Kuninori Morimoto]

We can now use of_graph_get_next_endpoint_raw(), let's use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/generic/audio-graph-card2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/generic/audio-graph-card2.c b/sound/soc/generic/audio-graph-card2.c
index 59a5db12bb5c..d616a82f05b6 100644
--- a/sound/soc/generic/audio-graph-card2.c
+++ b/sound/soc/generic/audio-graph-card2.c
@@ -555,7 +555,7 @@ static int graph_parse_node_multi_nm(struct snd_soc_dai_link *dai_link,
 		if (*nm_idx > nm_max)
 			break;
 
-		mcpu_ep_n = of_get_next_child(mcpu_port, mcpu_ep_n);
+		mcpu_ep_n = of_graph_get_next_endpoint_raw(mcpu_port, mcpu_ep_n);
 		if (!mcpu_ep_n) {
 			ret = 0;
 			break;
-- 
2.25.1

[PATCH v2 09/13] ASoC: test-component: use for_each_port_of_node()

[Kuninori Morimoto]

Current test-component.c is using for_each_endpoint_of_node() for parsing,
but it should use "port" base loop instead of "endpoint", because
properties are "port" base instead of "endpoint".

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/generic/test-component.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/generic/test-component.c b/sound/soc/generic/test-component.c
index e4967540a2e1..4bc83f141fa2 100644
--- a/sound/soc/generic/test-component.c
+++ b/sound/soc/generic/test-component.c
@@ -600,7 +600,7 @@ static int test_driver_probe(struct platform_device *pdev)
 	}
 
 	i = 0;
-	for_each_endpoint_of_node(node, ep) {
+	for_each_port_of_node(node, ep) {
 		snprintf(dname[i].name, TEST_NAME_LEN, "%s.%d", node->name, i);
 		ddriv[i].name = dname[i].name;
 
-- 
2.25.1

[PATCH v2 10/13] fbdev: omapfb: use of_graph_get_remote_port()

[Kuninori Morimoto]

We already have of_graph_get_remote_port(), Let's use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/video/fbdev/omap2/omapfb/dss/dss-of.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
index 0282d4eef139..fe6c72d03216 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
@@ -117,19 +117,6 @@ u32 dss_of_port_get_port_number(struct device_node *port)
 	return reg;
 }
 
-static struct device_node *omapdss_of_get_remote_port(const struct device_node *node)
-{
-	struct device_node *np;
-
-	np = of_graph_get_remote_endpoint(node);
-	if (!np)
-		return NULL;
-
-	np = of_get_next_parent(np);
-
-	return np;
-}
-
 struct device_node *
 omapdss_of_get_first_endpoint(const struct device_node *parent)
 {
@@ -159,7 +146,7 @@ omapdss_of_find_source_for_first_ep(struct device_node *node)
 	if (!ep)
 		return ERR_PTR(-EINVAL);
 
-	src_port = omapdss_of_get_remote_port(ep);
+	src_port = of_graph_get_remote_port(ep);
 	if (!src_port) {
 		of_node_put(ep);
 		return ERR_PTR(-EINVAL);
-- 
2.25.1

[PATCH v2 11/13] fbdev: omapfb: use of_graph_get_next_port()

[Kuninori Morimoto]

Now we can use of_graph_get_next_port() for port parsing.
Use it on omapfb.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/video/fbdev/omap2/omapfb/dss/dss-of.c | 48 +------------------
 drivers/video/fbdev/omap2/omapfb/dss/dss.c    |  9 ++--
 include/video/omapfb_dss.h                    |  4 --
 3 files changed, 6 insertions(+), 55 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
index fe6c72d03216..321ae18f2747 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
@@ -15,52 +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)
@@ -122,7 +76,7 @@ omapdss_of_get_first_endpoint(const struct device_node *parent)
 {
 	struct device_node *port, *ep;
 
-	port = omapdss_of_get_next_port(parent, NULL);
+	port = of_graph_get_next_port(parent, NULL);
 
 	if (!port)
 		return NULL;
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss.c b/drivers/video/fbdev/omap2/omapfb/dss/dss.c
index d814e4baa4b3..5cab317011ee 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/include/video/omapfb_dss.h b/include/video/omapfb_dss.h
index e8eaac2cb7b8..426d12881132 100644
--- a/include/video/omapfb_dss.h
+++ b/include/video/omapfb_dss.h
@@ -811,10 +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);
-- 
2.25.1

[PATCH v2 12/13] fbdev: omapfb: use of_graph_get_next_endpoint_raw()

[Kuninori Morimoto]

We can now use of_graph_get_next_endpoint_raw(), let's 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 | 22 +------------------
 drivers/video/fbdev/omap2/omapfb/dss/sdi.c    |  3 ++-
 include/video/omapfb_dss.h                    |  4 ----
 4 files changed, 5 insertions(+), 27 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dpi.c b/drivers/video/fbdev/omap2/omapfb/dss/dpi.c
index 7c1b7d89389a..c42c00850f0c 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_endpoint_raw(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 321ae18f2747..8aa2bfc2825f 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
@@ -15,26 +15,6 @@
 
 #include "dss.h"
 
-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;
@@ -81,7 +61,7 @@ omapdss_of_get_first_endpoint(const struct device_node *parent)
 	if (!port)
 		return NULL;
 
-	ep = omapdss_of_get_next_endpoint(port, NULL);
+	ep = of_graph_get_next_endpoint_raw(port, NULL);
 
 	of_node_put(port);
 
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/sdi.c b/drivers/video/fbdev/omap2/omapfb/dss/sdi.c
index d527931b2b16..d25f6575e557 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_endpoint_raw(port, NULL);
 	if (!ep)
 		return 0;
 
diff --git a/include/video/omapfb_dss.h b/include/video/omapfb_dss.h
index 426d12881132..fc106aaa75bf 100644
--- a/include/video/omapfb_dss.h
+++ b/include/video/omapfb_dss.h
@@ -811,10 +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_endpoint(const struct device_node *parent,
-			     struct device_node *prev);
-
 struct device_node *
 omapdss_of_get_first_endpoint(const struct device_node *parent);
 
-- 
2.25.1

[PATCH v2 13/13] fbdev: omapfb: use of_graph_get_next_endpoint()

[Kuninori Morimoto]

omapdss_of_get_first_endpoint() is same as
of_graph_get_next_endpoint(xxx, NULL). Replcase it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/video/fbdev/omap2/omapfb/dss/dsi.c    |  3 ++-
 drivers/video/fbdev/omap2/omapfb/dss/dss-of.c | 20 +------------------
 drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c  |  3 ++-
 drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c  |  3 ++-
 drivers/video/fbdev/omap2/omapfb/dss/venc.c   |  3 ++-
 include/video/omapfb_dss.h                    |  3 ---
 6 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
index b7eb17a16ec4..50fdfb9e9411 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dsi.c
@@ -28,6 +28,7 @@
 #include <linux/debugfs.h>
 #include <linux/pm_runtime.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/of_platform.h>
 #include <linux/component.h>
 
@@ -5079,7 +5080,7 @@ static int dsi_probe_of(struct platform_device *pdev)
 	struct device_node *ep;
 	struct omap_dsi_pin_config pin_cfg;
 
-	ep = omapdss_of_get_first_endpoint(node);
+	ep = of_graph_get_next_endpoint(node, 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 8aa2bfc2825f..1cd3e7251964 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
@@ -51,24 +51,6 @@ u32 dss_of_port_get_port_number(struct device_node *port)
 	return reg;
 }
 
-struct device_node *
-omapdss_of_get_first_endpoint(const struct device_node *parent)
-{
-	struct device_node *port, *ep;
-
-	port = of_graph_get_next_port(parent, NULL);
-
-	if (!port)
-		return NULL;
-
-	ep = of_graph_get_next_endpoint_raw(port, NULL);
-
-	of_node_put(port);
-
-	return ep;
-}
-EXPORT_SYMBOL_GPL(omapdss_of_get_first_endpoint);
-
 struct omap_dss_device *
 omapdss_of_find_source_for_first_ep(struct device_node *node)
 {
@@ -76,7 +58,7 @@ omapdss_of_find_source_for_first_ep(struct device_node *node)
 	struct device_node *src_port;
 	struct omap_dss_device *src;
 
-	ep = omapdss_of_get_first_endpoint(node);
+	ep = of_graph_get_next_endpoint(node, NULL);
 	if (!ep)
 		return ERR_PTR(-EINVAL);
 
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c b/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c
index f05b4e35a842..83f53858e5b5 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c
@@ -20,6 +20,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/clk.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
 #include <linux/component.h>
 #include <video/omapfb_dss.h>
@@ -529,7 +530,7 @@ static int hdmi_probe_of(struct platform_device *pdev)
 	struct device_node *ep;
 	int r;
 
-	ep = omapdss_of_get_first_endpoint(node);
+	ep = of_graph_get_next_endpoint(node, NULL);
 	if (!ep)
 		return 0;
 
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c b/drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c
index 03292945b1d4..427258d04e18 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c
@@ -25,6 +25,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/clk.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
 #include <linux/component.h>
 #include <video/omapfb_dss.h>
@@ -561,7 +562,7 @@ static int hdmi_probe_of(struct platform_device *pdev)
 	struct device_node *ep;
 	int r;
 
-	ep = omapdss_of_get_first_endpoint(node);
+	ep = of_graph_get_next_endpoint(node, NULL);
 	if (!ep)
 		return 0;
 
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/venc.c b/drivers/video/fbdev/omap2/omapfb/dss/venc.c
index c9d40e28a06f..30d90476f90b 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/venc.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/venc.c
@@ -24,6 +24,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/component.h>
 
 #include <video/omapfb_dss.h>
@@ -764,7 +765,7 @@ static int venc_probe_of(struct platform_device *pdev)
 	u32 channels;
 	int r;
 
-	ep = omapdss_of_get_first_endpoint(node);
+	ep = of_graph_get_next_endpoint(node, NULL);
 	if (!ep)
 		return 0;
 
diff --git a/include/video/omapfb_dss.h b/include/video/omapfb_dss.h
index fc106aaa75bf..d133190e3143 100644
--- a/include/video/omapfb_dss.h
+++ b/include/video/omapfb_dss.h
@@ -811,9 +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_first_endpoint(const struct device_node *parent);
-
 struct omap_dss_device *
 omapdss_of_find_source_for_first_ep(struct device_node *node);
 #else
-- 
2.25.1

Re: [PATCH v2 00/13] of: property: add port base loop

[Laurent Pinchart]

Hello Morimoto-san,

(CC'ing Sakari Ailus)

On Mon, Jan 29, 2024 at 12:54:24AM +0000, Kuninori Morimoto wrote:
> 
> Hi Rob
> 
> This is v2 of port base loop patch-set
> 
> We have endpoint base functions
> 	- of_graph_get_next_endpoint()
> 	- of_graph_get_endpoint_count()
> 	- for_each_endpoint_of_node()
> 
> But to handling "port" base things, it is not useful. We want to have
> "port" base functions, too. This patch-set adds it.
> 
> Because current existing drivers couldn't use "port" base functions,
> it were implemented in a different way. This patch-set doesn't try
> to full-replace to avoid unknown bug, try easy / quick replace only
> for now, but easy to know how "port" base functions are needed.
> 
> Because I can't test the driver which I can't use, non-ASoC drivers
> needs Tested-by, Acked-by.

The strategy sounds good to me. However, I'm wondering if you shouldn't
take one more step in the core, and implement these as fwnode
operations. Or is there a reason why OF is special, and iterating over
ports would be useful for drivers on OF systems but not on other types
of systems ?

> v1 -> v2
> 	- tidyup function explain
> 	- add missing header on each files
> 
> Kuninori Morimoto (13):
>   of: property: add port base loop
>   of: property: use of_graph_get_next_port() on of_graph_get_next_endpoint()
>   of: property: add of_graph_get_next_endpoint_raw()
>   drm: omapdrm: use of_graph_get_next_endpoint_raw()
>   media: xilinx-tpg: use of_graph_get_next_endpoint_raw()
>   ASoC: audio-graph-card: use of_graph_get_next_endpoint_raw()
>   ASoC: audio-graph-card2: use of_graph_get_next_port()
>   ASoC: audio-graph-card2: use of_graph_get_next_endpoint_raw()
>   ASoC: test-component: use for_each_port_of_node()
>   fbdev: omapfb: use of_graph_get_remote_port()
>   fbdev: omapfb: use of_graph_get_next_port()
>   fbdev: omapfb: use of_graph_get_next_endpoint_raw()
>   fbdev: omapfb: use of_graph_get_next_endpoint()
> 
>  drivers/gpu/drm/omapdrm/dss/dpi.c             |   2 +-
>  drivers/gpu/drm/omapdrm/dss/sdi.c             |   2 +-
>  drivers/media/platform/xilinx/xilinx-tpg.c    |   2 +-
>  drivers/of/property.c                         |  92 +++++++++++++---
>  drivers/video/fbdev/omap2/omapfb/dss/dpi.c    |   3 +-
>  drivers/video/fbdev/omap2/omapfb/dss/dsi.c    |   3 +-
>  drivers/video/fbdev/omap2/omapfb/dss/dss-of.c | 101 +-----------------
>  drivers/video/fbdev/omap2/omapfb/dss/dss.c    |   9 +-
>  drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c  |   3 +-
>  drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c  |   3 +-
>  drivers/video/fbdev/omap2/omapfb/dss/sdi.c    |   3 +-
>  drivers/video/fbdev/omap2/omapfb/dss/venc.c   |   3 +-
>  include/linux/of_graph.h                      |  23 ++++
>  include/video/omapfb_dss.h                    |  11 --
>  sound/soc/generic/audio-graph-card.c          |   2 +-
>  sound/soc/generic/audio-graph-card2.c         |  31 ++----
>  sound/soc/generic/test-component.c            |   2 +-
>  17 files changed, 133 insertions(+), 162 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 00/13] of: property: add port base loop

[Sakari Ailus]

Hi Laurent, Morimoto-san,

On Mon, Jan 29, 2024 at 02:27:36PM +0200, Laurent Pinchart wrote:
> Hello Morimoto-san,
> 
> (CC'ing Sakari Ailus)

Thanks for cc'ing me.

> 
> On Mon, Jan 29, 2024 at 12:54:24AM +0000, Kuninori Morimoto wrote:
> > 
> > Hi Rob
> > 
> > This is v2 of port base loop patch-set
> > 
> > We have endpoint base functions
> > 	- of_graph_get_next_endpoint()
> > 	- of_graph_get_endpoint_count()
> > 	- for_each_endpoint_of_node()
> > 
> > But to handling "port" base things, it is not useful. We want to have
> > "port" base functions, too. This patch-set adds it.
> > 
> > Because current existing drivers couldn't use "port" base functions,
> > it were implemented in a different way. This patch-set doesn't try
> > to full-replace to avoid unknown bug, try easy / quick replace only
> > for now, but easy to know how "port" base functions are needed.
> > 
> > Because I can't test the driver which I can't use, non-ASoC drivers
> > needs Tested-by, Acked-by.
> 
> The strategy sounds good to me. However, I'm wondering if you shouldn't
> take one more step in the core, and implement these as fwnode
> operations. Or is there a reason why OF is special, and iterating over
> ports would be useful for drivers on OF systems but not on other types
> of systems ?

I'd prefer that, too.

Probably we could use the existing callbacks for endpoint enumeration, for
port enumeration, too, as I don't think this is performance critical in any
way.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 00/13] of: property: add port base loop

[Kuninori Morimoto]

Hi Laurent, Sakari

Thank you for your review

> > The strategy sounds good to me. However, I'm wondering if you shouldn't
> > take one more step in the core, and implement these as fwnode
> > operations. Or is there a reason why OF is special, and iterating over
> > ports would be useful for drivers on OF systems but not on other types
> > of systems ?
> 
> I'd prefer that, too.

It is very easy reason, because I'm not fwnode user ;P
I'm not familiar with fwnode, but in my quick check, it seems it is easy
to expand fwnode side functions if of_graph side function exist ?

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH v2 00/13] of: property: add port base loop

[Sakari Ailus]

Hi Morimoto-san,

On Tue, Jan 30, 2024 at 12:34:55AM +0000, Kuninori Morimoto wrote:
> 
> Hi Laurent, Sakari
> 
> Thank you for your review
> 
> > > The strategy sounds good to me. However, I'm wondering if you shouldn't
> > > take one more step in the core, and implement these as fwnode
> > > operations. Or is there a reason why OF is special, and iterating over
> > > ports would be useful for drivers on OF systems but not on other types
> > > of systems ?
> > 
> > I'd prefer that, too.
> 
> It is very easy reason, because I'm not fwnode user ;P
> I'm not familiar with fwnode, but in my quick check, it seems it is easy
> to expand fwnode side functions if of_graph side function exist ?

That would be one way to do that, yes, but I suggested using the existing
endpoint iterators as that would keep the firmware specific implementation
more simple. The (slight) drawback is that for each node returned, you'd
need to check its parent (i.e. port node) is the same as the port you're
interested in. The alternative may involve reworking the struct
fwnode_operations interface somewhat, including swnode, DT and ACPI
implementations.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 00/13] of: property: add port base loop

[Tomi Valkeinen]

On 30/01/2024 09:31, Sakari Ailus wrote:
> Hi Morimoto-san,
> 
> On Tue, Jan 30, 2024 at 12:34:55AM +0000, Kuninori Morimoto wrote:
>>
>> Hi Laurent, Sakari
>>
>> Thank you for your review
>>
>>>> The strategy sounds good to me. However, I'm wondering if you shouldn't
>>>> take one more step in the core, and implement these as fwnode
>>>> operations. Or is there a reason why OF is special, and iterating over
>>>> ports would be useful for drivers on OF systems but not on other types
>>>> of systems ?
>>>
>>> I'd prefer that, too.
>>
>> It is very easy reason, because I'm not fwnode user ;P
>> I'm not familiar with fwnode, but in my quick check, it seems it is easy
>> to expand fwnode side functions if of_graph side function exist ?
> 
> That would be one way to do that, yes, but I suggested using the existing
> endpoint iterators as that would keep the firmware specific implementation
> more simple. The (slight) drawback is that for each node returned, you'd
> need to check its parent (i.e. port node) is the same as the port you're
> interested in. The alternative may involve reworking the struct
> fwnode_operations interface somewhat, including swnode, DT and ACPI
> implementations.
> 

But we still need the of_* versions, don't we, for patches 4 to 13?

  Tomi

Re: [PATCH v2 00/13] of: property: add port base loop

[Sakari Ailus]

On Tue, Jan 30, 2024 at 09:37:42AM +0200, Tomi Valkeinen wrote:
> On 30/01/2024 09:31, Sakari Ailus wrote:
> > Hi Morimoto-san,
> > 
> > On Tue, Jan 30, 2024 at 12:34:55AM +0000, Kuninori Morimoto wrote:
> > > 
> > > Hi Laurent, Sakari
> > > 
> > > Thank you for your review
> > > 
> > > > > The strategy sounds good to me. However, I'm wondering if you shouldn't
> > > > > take one more step in the core, and implement these as fwnode
> > > > > operations. Or is there a reason why OF is special, and iterating over
> > > > > ports would be useful for drivers on OF systems but not on other types
> > > > > of systems ?
> > > > 
> > > > I'd prefer that, too.
> > > 
> > > It is very easy reason, because I'm not fwnode user ;P
> > > I'm not familiar with fwnode, but in my quick check, it seems it is easy
> > > to expand fwnode side functions if of_graph side function exist ?
> > 
> > That would be one way to do that, yes, but I suggested using the existing
> > endpoint iterators as that would keep the firmware specific implementation
> > more simple. The (slight) drawback is that for each node returned, you'd
> > need to check its parent (i.e. port node) is the same as the port you're
> > interested in. The alternative may involve reworking the struct
> > fwnode_operations interface somewhat, including swnode, DT and ACPI
> > implementations.
> > 
> 
> But we still need the of_* versions, don't we, for patches 4 to 13?

Yes, my comment was indeed about the fwnode property API only.

-- 
Sakari Ailus

Re: [PATCH v2 00/13] of: property: add port base loop

[Kuninori Morimoto]

Hi Sakari

> > > > I'm not familiar with fwnode, but in my quick check, it seems it is easy
> > > > to expand fwnode side functions if of_graph side function exist ?
> > > 
> > > That would be one way to do that, yes, but I suggested using the existing
> > > endpoint iterators as that would keep the firmware specific implementation
> > > more simple. The (slight) drawback is that for each node returned, you'd
> > > need to check its parent (i.e. port node) is the same as the port you're
> > > interested in. The alternative may involve reworking the struct
> > > fwnode_operations interface somewhat, including swnode, DT and ACPI
> > > implementations.
> > > 
> > 
> > But we still need the of_* versions, don't we, for patches 4 to 13?
> 
> Yes, my comment was indeed about the fwnode property API only.

Thank you for your suggestion.
But I'm not familiar with fwnode, and it seems we still need of_*,
I will keep current style (= non fwnode) in v3



Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH v2 00/13] of: property: add port base loop

[Sakari Ailus]

Hi Morimoto-san,

On Tue, Jan 30, 2024 at 11:24:07PM +0000, Kuninori Morimoto wrote:
> 
> Hi Sakari
> 
> > > > > I'm not familiar with fwnode, but in my quick check, it seems it is easy
> > > > > to expand fwnode side functions if of_graph side function exist ?
> > > > 
> > > > That would be one way to do that, yes, but I suggested using the existing
> > > > endpoint iterators as that would keep the firmware specific implementation
> > > > more simple. The (slight) drawback is that for each node returned, you'd
> > > > need to check its parent (i.e. port node) is the same as the port you're
> > > > interested in. The alternative may involve reworking the struct
> > > > fwnode_operations interface somewhat, including swnode, DT and ACPI
> > > > implementations.
> > > > 
> > > 
> > > But we still need the of_* versions, don't we, for patches 4 to 13?
> > 
> > Yes, my comment was indeed about the fwnode property API only.
> 
> Thank you for your suggestion.
> But I'm not familiar with fwnode, and it seems we still need of_*,
> I will keep current style (= non fwnode) in v3

The fwnode API should be kept in sync with the OF (and other firmware
specific) API. Merging your set in its current form would leave fwnode API
impaired. Therefore I'd very much prefer to see this set add similar fwnode
APIs, too.

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 00/13] of: property: add port base loop

[Kuninori Morimoto]

Hi Sakari

> > Thank you for your suggestion.
> > But I'm not familiar with fwnode, and it seems we still need of_*,
> > I will keep current style (= non fwnode) in v3
> 
> The fwnode API should be kept in sync with the OF (and other firmware
> specific) API. Merging your set in its current form would leave fwnode API
> impaired. Therefore I'd very much prefer to see this set add similar fwnode
> APIs, too.

I will keep current fwnode API behavior, but I can't test it.
Now, I'm separating the patch-set into small stages.
There is no problem for a while, but I think I will ask you to test it in the
final stage.


Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto

Re: [PATCH v2 00/13] of: property: add port base loop

[Laurent Pinchart]

Hello Morimoto-san,

On Mon, Feb 05, 2024 at 05:31:25AM +0000, Kuninori Morimoto wrote:
> 
> Hi Sakari
> 
> > > Thank you for your suggestion.
> > > But I'm not familiar with fwnode, and it seems we still need of_*,
> > > I will keep current style (= non fwnode) in v3
> > 
> > The fwnode API should be kept in sync with the OF (and other firmware
> > specific) API. Merging your set in its current form would leave fwnode API
> > impaired. Therefore I'd very much prefer to see this set add similar fwnode
> > APIs, too.
> 
> I will keep current fwnode API behavior, but I can't test it.

The fwnode API is an abstraction layer on top of the OF or ACPI APIs,
and allows drivers to work on both without needing to support OF and
ACPI explicitly and separately. You should be able to convert the
drivers you're using to the fwnode API, and it should work exactly the
same as when using the OF-specific functions. That will give you a way
to test the API.

For instance, if you look at the drivers/media/platform/rcar_drif.c
driver, you will see

        if (!fwnode_property_read_u32(fwnode, "sync-active", &val))

which, on OF platforms, is equivalent to

        if (!of_property_read_u32(np, "sync-active", &val))

This particular driver will never be used on an ACPI-based system, but
drivers are still encouraged to use the fwnode API. 

> Now, I'm separating the patch-set into small stages.
> There is no problem for a while, but I think I will ask you to test it in the
> final stage.

-- 
Regards,

Laurent Pinchart