Re: [PATCH v2 03/13] of: property: add of_graph_get_next_endpoint_raw()

[Sakari Ailus <sakari.ailus@xxxxxx>]

Hi Morimoto-san,

On Mon, Jan 29, 2024 at 12:54:59AM +0000, Kuninori Morimoto wrote:
> We already have of_graph_get_next_endpoint(), but it is not intuitive
> to use.
> 
> (X)	node {
> (Y)		ports {
> 			port@0 { endpoint { remote-endpoint = ...; };};
> (A1)			port@1 { endpoint { remote-endpoint = ...; };
> (A2)				 endpoint { remote-endpoint = ...; };};
> (B)			port@2 { endpoint { remote-endpoint = ...; };};
> 		};
> 	};
> 
> For example, if I want to handle port@1's 2 endpoints (= A1, A2),
> I want to use like below
> 
> 	A1 = of_graph_get_next_endpoint(port1, NULL);
> 	A2 = of_graph_get_next_endpoint(port1, A1);
> 
> But 1st one will be error, because of_graph_get_next_endpoint() requested
> "parent" means "node" (X) or "ports" (Y), not "port".
> Below are OK
> 
> 	of_graph_get_next_endpoint(node,  NULL); // node/ports/port@0/endpoint
> 	of_graph_get_next_endpoint(ports, NULL); // node/ports/port@0/endpoint
> 
> In other words, we can't handle A1/A2 directly via
> of_graph_get_next_endpoint() so far.
> 
> There is another non intuitive behavior on of_graph_get_next_endpoint().
> In case of if I could get A1 pointer for some way, and if I want to
> handle port@1 things, I would like use it like below
> 
> 	/*
> 	 * "endpoint" is now A1, and handle port1 things here,
> 	 * but we don't know how many endpoints port1 has.
> 	 *
> 	 * Because "endpoint" is non NULL, we can use port1
> 	 * as of_graph_get_next_endpoint(port1, xxx)
> 	 */
> 	do {
> 		/* do something for port1 specific things here */
> 	} while (endpoint = of_graph_get_next_endpoint(port1, endpoint))
> 
> But it also not worked as I expected.
> I expect it will be A1 -> A2 -> NULL,
> but      it will be A1 -> A2 -> B,    because of_graph_get_next_endpoint()
> will fetch endpoint beyond the port.
> 
> It is not useful on generic driver like Generic Sound Card.
> It uses of_get_next_child() instead for now, but it is not intuitive,
> and not check node name (= "endpoint").
> 
> To handle endpoint more intuitive, create of_graph_get_next_endpoint_raw()
> 
> 	of_graph_get_next_endpoint_raw(port1, NULL); // A1
> 	of_graph_get_next_endpoint_raw(port1, A1);   // A2
> 	of_graph_get_next_endpoint_raw(port1, A2);   // NULL
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> ---
>  drivers/of/property.c    | 26 +++++++++++++++++++++++++-
>  include/linux/of_graph.h |  2 ++
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 14ffd199c9b1..37dbb1b0e742 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -667,6 +667,30 @@ struct device_node *of_graph_get_next_port(const struct device_node *parent,
>  }
>  EXPORT_SYMBOL(of_graph_get_next_port);
>  
> +/**
> + * of_graph_get_next_endpoint_raw() - get next endpoint node
> + * @port: pointer to the target port node
> + * @endpoint: current endpoint node, or NULL to get first
> + *
> + * Return: An 'endpoint' node pointer with refcount incremented. Refcount
> + * of the passed @prev node is decremented.
> + */
> +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port,
> +						   struct device_node *endpoint)
> +{
> +	if (!port)
> +		return NULL;

of_get_next_child() returns NULL if node is NULL, hence there's no need to
check this.

> +
> +	do {
> +		endpoint = of_get_next_child(port, endpoint);
> +		if (!endpoint)
> +			break;
> +	} while (!of_node_name_eq(endpoint, "endpoint"));
> +
> +	return endpoint;
> +}
> +EXPORT_SYMBOL(of_graph_get_next_endpoint_raw);
> +
>  /**
>   * of_graph_get_next_endpoint() - get next endpoint node
>   * @parent: pointer to the parent device node
> @@ -708,7 +732,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
>  		 * getting the next child. If the previous endpoint is NULL this
>  		 * will return the first child.
>  		 */
> -		endpoint = of_get_next_child(port, prev);
> +		endpoint = of_graph_get_next_endpoint_raw(port, prev);
>  		if (endpoint) {
>  			of_node_put(port);
>  			return endpoint;
> diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
> index fff598640e93..427905a6e8c3 100644
> --- a/include/linux/of_graph.h
> +++ b/include/linux/of_graph.h
> @@ -57,6 +57,8 @@ int of_graph_get_port_count(const struct device_node *np);
>  struct device_node *of_graph_get_port_by_id(struct device_node *node, u32 id);
>  struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
>  					struct device_node *previous);
> +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port,
> +						   struct device_node *prev);
>  struct device_node *of_graph_get_next_port(const struct device_node *parent,
>  					   struct device_node *previous);
>  struct device_node *of_graph_get_endpoint_by_regs(

-- 
Kind regards,

Sakari Ailus