diff mbox series

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

Message ID 87bk95klgc.wl-kuninori.morimoto.gx@renesas.com (mailing list archive)
State Superseded
Headers show
Series of: property: add port base loop | expand

Commit Message

Kuninori Morimoto Jan. 29, 2024, 12:54 a.m. UTC
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@renesas.com>
---
 drivers/of/property.c    | 26 +++++++++++++++++++++++++-
 include/linux/of_graph.h |  2 ++
 2 files changed, 27 insertions(+), 1 deletion(-)

Comments

Tomi Valkeinen Jan. 29, 2024, 12:29 p.m. UTC | #1
On 29/01/2024 02:54, 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@renesas.com>
> ---
>   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

How about "of_graph_get_next_port_endpoint()"?

> + * @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.
> + */

It might be good to highlight here the difference to the 
of_graph_get_next_endpoint().

> +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port,
> +						   struct device_node *endpoint)
> +{
> +	if (!port)
> +		return NULL;
> +
> +	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(
Laurent Pinchart Jan. 29, 2024, 1:02 p.m. UTC | #2
On Mon, Jan 29, 2024 at 02:29:22PM +0200, Tomi Valkeinen wrote:
> On 29/01/2024 02:54, 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@renesas.com>
> > ---
> >   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
> 
> How about "of_graph_get_next_port_endpoint()"?

We may want to also rename the existing of_graph_get_next_endpoint()
function to of_graph_next_dev_endpoint() then. It would be a tree-wide
patch, which is always annoying to get reviewed and merged, so if Rob
would prefer avoiding the rename, I'm fine with that.

> > + * @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.
> > + */
> 
> It might be good to highlight here the difference to the 
> of_graph_get_next_endpoint().

Yes, and the documentation of of_graph_get_next_endpoint() shoul also be
improved.

> > +struct device_node *of_graph_get_next_endpoint_raw(const struct device_node *port,
> > +						   struct device_node *endpoint)
> > +{
> > +	if (!port)
> > +		return NULL;
> > +
> > +	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(
Sakari Ailus Jan. 29, 2024, 1:37 p.m. UTC | #3
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@renesas.com>
> ---
>  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(
Kuninori Morimoto Jan. 30, 2024, 12:08 a.m. UTC | #4
Hi Laurent, Tomi

Thank you for your review

> > > +/**
> > > + * of_graph_get_next_endpoint_raw() - get next endpoint node
> > 
> > How about "of_graph_get_next_port_endpoint()"?
> 
> We may want to also rename the existing of_graph_get_next_endpoint()
> function to of_graph_next_dev_endpoint() then. It would be a tree-wide
> patch, which is always annoying to get reviewed and merged, so if Rob
> would prefer avoiding the rename, I'm fine with that.

To be honest, from intuitive function naming point of view,
I prefer rename existing function name. But yes, it will be big patch.

Current of_graph_get_next_endpoint() will get next endpoint
beyond the port (A)
New function is not get next endpoint beyond the port (B)

Something like

(A) of_graph_get_next_endpoint() -> of_graph_get_next_port_endpoint()
(B)                                 of_graph_get_next_endpoint()

> > > + * @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.
> > > + */
> > 
> > It might be good to highlight here the difference to the 
> > of_graph_get_next_endpoint().
> 
> Yes, and the documentation of of_graph_get_next_endpoint() shoul also be
> improved.

Yes, Indeed.




Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
Kuninori Morimoto Jan. 30, 2024, 12:37 a.m. UTC | #5
Hi Sakari

> > +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.

Thanks, will fix in v3


Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
Rob Herring Jan. 31, 2024, 6:43 p.m. UTC | #6
On Mon, Jan 29, 2024 at 03:02:19PM +0200, Laurent Pinchart wrote:
> On Mon, Jan 29, 2024 at 02:29:22PM +0200, Tomi Valkeinen wrote:
> > On 29/01/2024 02:54, 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@renesas.com>
> > > ---
> > >   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
> > 
> > How about "of_graph_get_next_port_endpoint()"?
> 
> We may want to also rename the existing of_graph_get_next_endpoint()
> function to of_graph_next_dev_endpoint() then. It would be a tree-wide
> patch, which is always annoying to get reviewed and merged, so if Rob
> would prefer avoiding the rename, I'm fine with that.

I think we should get rid of or minimize of_graph_get_next_endpoint() in 
its current form. In general, drivers should be asking for a specific 
port number because their function is fixed in the binding. Iterating 
over endpoints within a port is okay as that's usually a selecting 1 of 
N operation. 

Most cases are in the form of of_graph_get_next_endpoint(dev, NULL) 
which is equivalent to of_graph_get_endpoint_by_regs(dev, 0, 0). 
Technically, -1 instead of 0 is equivalent, but I'd argue is sloppy and 
wrong.

I also added of_graph_get_remote_node() for this reason and cleaned a 
lot of these (in DRM) up some time ago. Because in the end, a driver 
generally just wants the remote device it is connected to and details of 
parsing the graph should be mostly opaque.

Wouldn't something like this work for this case:

#define for_each_port_endpoint_of_node(parent, port, child) \
	for (child = of_graph_get_endpoint_by_regs(parent, port, -1); child != NULL; \
	     child = of_get_next_child(parent, child))

Rob
Kuninori Morimoto Feb. 1, 2024, 12:43 a.m. UTC | #7
Hi Rob

> I think we should get rid of or minimize of_graph_get_next_endpoint() in 
> its current form. In general, drivers should be asking for a specific 
> port number because their function is fixed in the binding. Iterating 
> over endpoints within a port is okay as that's usually a selecting 1 of 
> N operation. 
> 
> Most cases are in the form of of_graph_get_next_endpoint(dev, NULL) 
> which is equivalent to of_graph_get_endpoint_by_regs(dev, 0, 0). 
> Technically, -1 instead of 0 is equivalent, but I'd argue is sloppy and 
> wrong.
> 
> I also added of_graph_get_remote_node() for this reason and cleaned a 
> lot of these (in DRM) up some time ago. Because in the end, a driver 
> generally just wants the remote device it is connected to and details of 
> parsing the graph should be mostly opaque.
> 
> Wouldn't something like this work for this case:
> 
> #define for_each_port_endpoint_of_node(parent, port, child) \
> 	for (child = of_graph_get_endpoint_by_regs(parent, port, -1); child != NULL; \
> 	     child = of_get_next_child(parent, child))

I see.
I will split this patch-set to like below

	- patch-set for reduce/remove to using current next_endpoint()
	- patch-set for rename current next_endpoint() to next_device_endpoint()
	- patch-set for adding new next_port_endpoint()


Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
Kuninori Morimoto Feb. 1, 2024, 1:48 a.m. UTC | #8
Hi Rob

> > #define for_each_port_endpoint_of_node(parent, port, child) \
> > 	for (child = of_graph_get_endpoint_by_regs(parent, port, -1); child != NULL; \
> > 	     child = of_get_next_child(parent, child))

Hmm... I noticed it is impossible so for.
of_graph_get_endpoint_by_regs() (A) is based on for_each_endpoint_of_node() (B).
Thus, we can't replace for_each_endpoint_of_node() (B) with by_regs (A)

(A)	struct device_node *of_graph_get_endpoint_by_regs(...)
	{
		...
(B)		for_each_endpoint_of_node(parent, node) {
			...
		}

		return NULL;
	}

> 	- patch-set for reduce/remove to using current next_endpoint()
> 	- patch-set for rename current next_endpoint() to next_device_endpoint()
> 	- patch-set for adding new next_port_endpoint()

So, maybe we can do is,

	0) rename current endpoint functions to device_endpoint

	1) add new port base functions (port_endpoint) which has
	   for_each_of_graph_port_endpoint() loop. It is for port base endpoint loop
	   (I want to use new naming, using of_graph instead of _of_node).

	2) replace above (B) part with port base loops 

	-	for_each_endpoint_of_node(parent, node) {
	+	for_each_of_gprah_port(parent, port) {
	+		for_each_of_graph_port_endpoint(port, endpoint) {

	3) replace current next_endpoint() by next_endpoint_by_regs(),
	   and remove next_endpoint()

What do you think ?

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
diff mbox series

Patch

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;
+
+	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(