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

[Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>]

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