diff mbox series

[1/9] of: property: add of_graph_get_next_port()

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

Commit Message

Kuninori Morimoto Aug. 6, 2024, 4:58 a.m. UTC 
We have endpoint base functions
	- of_graph_get_next_device_endpoint()
	- of_graph_get_device_endpoint_count()
	- for_each_of_graph_device_endpoint()

Here, for_each_of_graph_device_endpoint() loop finds each endpoints

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

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

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

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

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

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

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

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

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

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

This loop works in case of "node" doesn't have "ports" also.

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

Comments

Kuninori Morimoto Aug. 20, 2024, 4:34 a.m. UTC | #1 
Hi Tomi

Thank you for the feedback, and sorry for my late responce.
I was under Summer Vacation

> If we have this, of_graph_get_next_ports() returns the ports@0, and that 
> makes sense:
> 
> parent {
> 	ports@0 {
> 		port@0 { };
> 	};
> };
> 
> If we have this, of_graph_get_next_ports() returns the parent, and 
> that's a bit surprising, but I can see the need, and it just needs to be 
> documented:
> 
> parent {
> 	port { };
> };

Thank you for understanding the situation

> But if we have this, does it make sense that of_graph_get_next_ports() 
> returns the parent, or should it return NULL:
> 
> parent {
> 	/* No ports or port */
> };

Yeah, it should return NULL in this case.

>  > 	port = of_graph_get_next_port(parent, NULL); // (A)
>  > 	port = of_graph_get_next_port(parent, port); // (B)
>  > 	port = of_graph_get_next_port(parent, port); // NULl
> 
> it does not feel right. Why does of_graph_get_next_port() return only 
> the ports of ports@0? I think it should either return nothing, as there 
> are no 'port' nodes in the parent, or it should return all the port 
> nodes from all the ports nodes.

As you also mentioned, having "ports" node is optional, and maybe this is
the reason you feel uncomfortable on current code, and I can agree about
it.

If the driver needs to care about multi "ports", it is obvious that the
driver cares "ports" node.

My opinion is because having "ports" is optional, we want to handle
"port" with/without "ports" by same function, especially if it doesn't
need to care about multi "ports".

I think your opinion (= "port" nodes strictly only in the given parent
node) means driver need to care both with/without "ports" node, but the
code will be pointlessly complex if it doesn't need to care multi "ports".

> bus {
> 	/* our display device */
> 	display {
> 		port { };
> 	};
> 
> 	/* some odd ports device */
> 	ports {
> 	};
> };
> 
> and you use of_graph_get_next_ports() for display, you'll end up getting 
> the 'ports' node.

This is interesting sample, but feels a little forced.
If you need to handle display::port, you call

	port = of_graph_get_next_port(display, NULL);

Indeed we will get "ports" node if it calls of_graph_get_next_ports(),
but it needs to use unrelated parameters, I think ?

	ports = of_graph_get_next_ports(bus, display);

> > 	parent {
> > 		ports@0 {
> > 			port@0 { };
> > 			port@1 { };
> > 		};
> > 		ports@1 {
> > 			port@0 { };
> > 		};
> > 	};
> > 
> > 	of_graph_get_port_count(parent); // 2 = number of ports@0
> 
> I think the above is a bit surprising, and in my opinion points that 
> there is a problem. Why does using 'parent' equate to only using 
> 'ports@0'? Again, I would expect either to get 0 (as there are no 'port' 
> nodes in parent, or 3.

This feature is for the driver which *doesn't* need to care about "ports".

	/* CASE1 */
	parent {
		port {...};
	};

	/* CASE2 */
	parent {
		ports {
			port {...};
		};
	};

both case (CASE1/2), we can use

	of_graph_get_port_count(parent);

If the driver need to care multi ports, it should use such code, IMO.

	nr = 0;
	for_each_of_graph_ports(parent, ports) {
		nr += of_graph_get_port_count(ports);

But of course we can handle it in v2 patch.

	/* of_graph_get_port_count() cares multi ports inside */
	of_graph_get_port_count(parent);

Thank you for your help !!

Best regards
---
Kuninori Morimoto
diff mbox series

Patch

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 164d77cb9445..3b2d09c0376a 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -625,8 +625,76 @@  struct device_node *of_graph_get_port_by_id(struct device_node *parent, u32 id)
 }
 EXPORT_SYMBOL(of_graph_get_port_by_id);
 
+/**
+ * of_graph_get_next_ports() - get next ports node.
+ * @parent: pointer to the parent device node
+ * @ports: current ports node, or NULL to get first
+ *
+ * Return: A 'ports' node pointer with refcount incremented. Refcount
+ * of the passed @prev node is decremented.
+ */
+struct device_node *of_graph_get_next_ports(struct device_node *parent,
+					    struct device_node *ports)
+{
+	if (!parent)
+		return NULL;
+
+	if (!ports) {
+		ports = of_get_child_by_name(parent, "ports");
+
+		/* use parent as its ports of this device if it not exist */
+		if (!ports) {
+			ports = parent;
+			of_node_get(ports);
+		}
+
+		return ports;
+	}
+
+	do {
+		ports = of_get_next_child(parent, ports);
+		if (!ports)
+			break;
+	} while (!of_node_name_eq(ports, "ports"));
+
+	return ports;
+}
+EXPORT_SYMBOL(of_graph_get_next_ports);
+
+/**
+ * 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: A 'port' node pointer with refcount incremented. Refcount
+ * of the passed @prev node is decremented.
+ */
+struct device_node *of_graph_get_next_port(struct device_node *parent,
+					   struct device_node *port)
+{
+	if (!parent)
+		return NULL;
+
+	if (!port) {
+		struct device_node *ports __free(device_node) =
+			of_graph_get_next_ports(parent, NULL);
+
+		return of_get_child_by_name(ports, "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
  * @prev: previous endpoint node, or NULL to get first
  *
@@ -823,6 +891,24 @@  unsigned int of_graph_get_endpoint_count(const struct device_node *np)
 }
 EXPORT_SYMBOL(of_graph_get_endpoint_count);
 
+/**
+ * of_graph_get_port_count() - get count of port
+ * @np: pointer to the parent device node
+ *
+ * Return: count of port of this device node
+ */
+unsigned int of_graph_get_port_count(struct device_node *np)
+{
+	struct device_node *port = NULL;
+	int num = 0;
+
+	for_each_of_graph_port(np, port)
+		num++;
+
+	return num;
+}
+EXPORT_SYMBOL(of_graph_get_port_count);
+
 /**
  * of_graph_get_remote_node() - get remote parent device_node for given port/endpoint
  * @node: pointer to parent device_node containing graph port/endpoint
diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
index a4bea62bfa29..30169b50b042 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -37,14 +37,42 @@  struct of_endpoint {
 	for (child = of_graph_get_next_endpoint(parent, NULL); child != NULL; \
 	     child = of_graph_get_next_endpoint(parent, child))
 
+/**
+ * for_each_of_graph_ports - iterate over every ports in a device node
+ * @parent: parent device node containing ports
+ * @child: loop variable pointing to the current ports node
+ *
+ * When breaking out of the loop, of_node_put(child) has to be called manually.
+ */
+#define for_each_of_graph_ports(parent, child)				\
+	for (child = of_graph_get_next_ports(parent, NULL); child != NULL; \
+	     child = of_graph_get_next_ports(parent, child))
+
+/**
+ * for_each_of_graph_port - iterate over every port in a device or ports node
+ * @parent: parent device or ports node containing port
+ * @child: loop variable pointing to the current port node
+ *
+ * When breaking out of the loop, of_node_put(child) has to be called manually.
+ */
+#define for_each_of_graph_port(parent, child)			\
+	for (child = of_graph_get_next_port(parent, NULL); child != NULL; \
+	     child = of_graph_get_next_port(parent, child))
+
 #ifdef CONFIG_OF
 bool of_graph_is_present(const struct device_node *node);
 int of_graph_parse_endpoint(const struct device_node *node,
 				struct of_endpoint *endpoint);
+
 unsigned int of_graph_get_endpoint_count(const struct device_node *np);
+unsigned int of_graph_get_port_count(struct device_node *np);
 struct device_node *of_graph_get_port_by_id(struct device_node *node, u32 id);
 struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 					struct device_node *previous);
+struct device_node *of_graph_get_next_ports(struct device_node *parent,
+					    struct device_node *ports);
+struct device_node *of_graph_get_next_port(struct device_node *parent,
+					   struct device_node *port);
 struct device_node *of_graph_get_endpoint_by_regs(
 		const struct device_node *parent, int port_reg, int reg);
 struct device_node *of_graph_get_remote_endpoint(
@@ -73,6 +101,11 @@  static inline unsigned int of_graph_get_endpoint_count(const struct device_node
 	return 0;
 }
 
+static inline unsigned int of_graph_get_port_count(struct device_node *np)
+{
+	return 0;
+}
+
 static inline struct device_node *of_graph_get_port_by_id(
 					struct device_node *node, u32 id)
 {
@@ -86,6 +119,20 @@  static inline struct device_node *of_graph_get_next_endpoint(
 	return NULL;
 }
 
+static inline struct device_node *of_graph_get_next_ports(
+					struct device_node *parent,
+					struct device_node *previous)
+{
+	return NULL;
+}
+
+static inline struct device_node *of_graph_get_next_port(
+					struct device_node *parent,
+					struct device_node *previous)
+{
+	return NULL;
+}
+
 static inline struct device_node *of_graph_get_endpoint_by_regs(
 		const struct device_node *parent, int port_reg, int reg)
 {