Hi Tomi
Thank you for your review
> > +/**
> > + * 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
> > +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
> > +/**
> > + * 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
> > + * @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
