On 06/08/2024 07:58, Kuninori Morimoto wrote:
We have endpoint base functions
- of_graph_get_next_device_endpoint()
- of_graph_get_device_endpoint_count()
- for_each_of_graph_device_endpoint()
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@xxxxxxxxxxx>
---
drivers/of/property.c | 86 ++++++++++++++++++++++++++++++++++++++++
include/linux/of_graph.h | 47 ++++++++++++++++++++++
2 files changed, 133 insertions(+)
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.
No "prev" argument in the code.
The of_graph_get_next_endpoint() function uses "previous" as the
argument name (well, the function declaration uses "previous", the
implementation uses "prev"...), and I would use the same naming here.
Also of_graph_get_next_endpoint() talks about "previous endpoint node",
whereas here it's "current ports node". I'd use the same style here, so
"previous ports node".
The same comments for the of_graph_get_next_port().
+ */
+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 */
I think this needs to be described in the kernel doc. I understand the
need for this, but it's somewhat counter-intuitive that this returns the
parent node if there are no ports nodes, so it must be highlighted in
the documentation.
I wonder if a bit more complexity here would be good... I think here we
could:
- If there are no 'ports' nodes in the parent, but there is a 'port'
node in the parent, return the parent node
- If there are no 'ports' nor 'port' nodes in the parent, return NULL
+ if (!ports) {
+ ports = parent;
+ of_node_get(ports);
+ }
You could just do "ports = of_node_get(parent);"
+
+ 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;
+}
Hmm... So if I call this with of_graph_get_next_port(dev_node, NULL)
(dev_node being the device node of the device), it'll give me the first
port in the first ports node, or the first port in the dev_node if there
are no ports nodes?
And if I then continue iterating with of_graph_get_next_port(dev_node,
prev_port)... The call will return NULL if the dev_node contains "ports"
node (because the dev_node does not contain any "port" nodes)?
So if I understand right, of_graph_get_next_port() must always be called
with a parent that contains port nodes. Sometimes that's the device's
node (if there's just one port) and sometimes that's ports node. If it's
called with a parent that contains ports node, it will not work correctly.
If the above is right, then should this just return
"of_get_child_by_name(parent, "port")" if !port, instead of calling
of_graph_get_next_ports()?
Or maybe I'm just getting confused here. But in any case, I think it
would be very good to describe the behavior on the kernel doc for the
different ports/port structure cases (also for
of_graph_get_next_ports()), and be clear on what the parameters can be,
i.e. what kind of device nodes can be given as parent, and how the
function iterates over the ports.
+EXPORT_SYMBOL(of_graph_get_next_port);
+
/**
* of_graph_get_next_endpoint() - get next endpoint node
+ *
Extra change.
* @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
Perhaps "get the number of port nodes".
+ * @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;
+}
I my analysis above is right, calling of_graph_get_port_count(dev_node)
will return 1, if the dev_node contains "ports" node which contains one
or more "port" nodes.
+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);
+
Extra change.
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)
{
Tomi
