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

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

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