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

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

Hi Laurent

Thank you for the review

> Having multiple "ports" nodes in a device node is not something I've
> ever seen before. There may be use cases, but how widespread are they ?
> I would prefer handling this in driver code instead of creating a helper
> function if the use case is rare.

In Sound, basically 1 CPU device will be 1 Sound Card, but sometimes we
want to handle 1 CPU device for 2 Sound Cards.
In such case, it needs multiple "ports".

1 example is
	(A) arch/arm64/boot/dts/renesas/ulcb-audio-graph-card.dtsi
	(B) arch/arm64/boot/dts/renesas/ulcb-kf-audio-graph-card.dtsi

We have ULCB board and its expand board (= KingFisher), and both have
Sound interface. In this case, we want to handle ULCB-Sound as 1st Sound
Card, and ULCB-KF-Sound as 2nd Sound Card. Before, it was handled as
1 big Sound Card, but it was not intuitive for user.

Our SoC's ports@0 is used for ULCB-Sound (= A), and ports@1 is used for
ULCB-KF-Sound (= B) now.


> > +struct device_node *of_graph_get_next_port(struct device_node *parent,
> > +					   struct device_node *prev)
> > +{
> > +	if (!parent)
> > +		return NULL;
> > +
> > +	if (!prev) {
> > +		struct device_node *ports __free(device_node) =
> > +			of_graph_get_next_ports(parent, NULL);
> 
> This also makes me quite uncomfortable. Iterating over all ports of a
> device node that contains multiple "ports" children seems an ill-defined
> use case.

This is because having "ports" node is optional. The driver code will be
pointlessly complicated without above.

Below 2 cases are indicating same things. So driver want to handle these
by same code.

	/* CASE1 */
	device {
		ports {
			port { ... };
		};
	};

	/* CASE2 */
	device {
		port { ... };
	};

	port = of_graph_get_next_port(device, NULL);


> > +#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))
> 
> I think I've proposed something similar a looooong time ago, and was
> told that iterating over ports is not something that drivers should do.
> The situation may have changed since though.

I guess you mean checking "endpoint" is enough ?
But unfortunately, it is not for us.

I guess this is mentioned in git-log, but we have *Generic* Sound Card
driver which supports many type of Sound connection.

	device {
		ports {
			port@0 {
				endpoint@0 { ... };
				endpoint@1 { ... };
			};
			port@1 {
				endpoint { ... };
			};
		};
	};

Because it is *Generic* Sound Card driver, it need to know how many
"port" are used. In this case, using "endpoint" loop is not useful.
It want to use "port" base loop.


Thank you for your help !!

Best regards
---
Kuninori Morimoto