Hi Tomi
Thank you for your review
quoted
+/**
+ * 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().
OK, thanks. Will fix in v2
quoted
+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
Thanks, but unfortunately, get_next_ports() checks only "ports" and
doesn't check whether it has "port" node or not.
So correct comment here is maybe...
If "parent" doesn't have "ports", it returns "parent" itself as "ports"
I will add it on v2
quoted
+/**
+ * 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?
Yes
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()?
Hmm ? Do you mean you want access to ports@1 memeber port after ports@0 ?
I have tested below in my test environment
parent {
(X) ports@0 {
(A) port@0 { };
(B) port@1 { };
};
(Y) ports@1 {
(C) port@0 { };
};
};
In this case, if you call get_next_port() with parent,
you can get ports@0 member port.
/* use "paramet" and use "ports@0" are same result */
// use parent
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
// use ports@0
ports = of_graph_get_next_ports(parent, NULL); // (X)
port = of_graph_get_next_port(ports, NULL); // (A)
port = of_graph_get_next_port(ports, port); // (B)
port = of_graph_get_next_port(ports, port); // NULl
If you want to get ports@1 member port, you need to use ports@1.
// use ports@1
ports = of_graph_get_next_ports(parent, NULL); // (X)
ports = of_graph_get_next_ports(parent, ports); // (Y)
port = of_graph_get_next_port(ports, NULL); // (C)
I have confirmed in my test environment.
But please double check it. Is this clear for you ?
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.
OK, will do in v2
quoted
+ * @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.
In my test, it will be..
parent {
ports@0 {
port@0 { };
port@1 { };
};
ports@1 {
port@0 { };
};
};
of_graph_get_port_count(parent); // 2 = number of ports@0
of_graph_get_port_count(ports0); // 2 = number of ports@0
of_graph_get_port_count(ports1); // 1 = number of ports@1
Thank you for your help !!
Best regards
---
Kuninori Morimoto