diff mbox series

[v3,2/9] of: property: add of_graph_get_next_port_endpoint()

Message ID 87a5h0qa0g.wl-kuninori.morimoto.gx@renesas.com
State Superseded
Headers show
Series of: property: add of_graph_get_next_port/port_endpoint() | expand

Commit Message

Kuninori Morimoto Aug. 26, 2024, 2:43 a.m. UTC
We already have of_graph_get_next_endpoint(), but it is not
intuitive to use in some case.

(X)	node {
(Y)		ports {
(P0)			port@0 { endpoint { remote-endpoint = ...; };};
(P10)			port@1 { endpoint { remote-endpoint = ...; };
(P11)				 endpoint { remote-endpoint = ...; };};
(P2)			port@2 { endpoint { remote-endpoint = ...; };};
		};
	};

For example, if I want to handle port@1's 2 endpoints (= P10, P11),
I want to use like below

	P10 = of_graph_get_next_endpoint(port1, NULL);
	P11 = of_graph_get_next_endpoint(port1, P10);

But 1st one will be error, because of_graph_get_next_endpoint()
requested "parent" means "node" (X) or "ports" (Y), not "port".
Below works, but it will get P0

	/* These will be node/ports/port@0/endpoint */
	P0 = of_graph_get_next_endpoint(node,  NULL);
	P0 = of_graph_get_next_endpoint(ports, NULL);

In other words, we can't handle P10/P11 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 P10 pointer for some way, and if I want to
handle port@1 things, I would like use it like below

	/*
	 * "ep" is now P10, and handle port1 things here,
	 * but we don't know how many endpoints port1 has.
	 *
	 * Because "ep" is non NULL now, we can use port1
	 * as of_graph_get_next_endpoint(port1, xxx)
	 */
	do {
		/* do something for port1 specific things here */
	} while (ep = of_graph_get_next_endpoint(port1, ep))

But it also not worked as I expected.
I expect it will be P10 -> P11 -> NULL,
but      it will be P10 -> P11 -> P2,    because
of_graph_get_next_endpoint() will fetch "endpoint" beyond the "port".

It is not useful on generic driver.
It uses of_get_next_child() instead for now, but it is not intuitive.
And it doesn't check node name (= "endpoint").

To handle endpoint more intuitive, create of_graph_get_next_port_endpoint()

	of_graph_get_next_port_endpoint(port1, NULL); // P10
	of_graph_get_next_port_endpoint(port1, P10);  // P11
	of_graph_get_next_port_endpoint(port1, P11);  // NULL

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/of/property.c    | 22 ++++++++++++++++++++++
 include/linux/of_graph.h | 20 ++++++++++++++++++++
 2 files changed, 42 insertions(+)

Comments

Sakari Ailus Aug. 27, 2024, 10:41 a.m. UTC | #1
Rob, Kunimori-san,

On Mon, Aug 26, 2024 at 10:40:09AM -0500, Rob Herring wrote:
> On Mon, Aug 26, 2024 at 02:43:28AM +0000, Kuninori Morimoto wrote:
> > We already have of_graph_get_next_endpoint(), but it is not
> > intuitive to use in some case.
> 
> Can of_graph_get_next_endpoint() users be replaced with your new 
> helpers? I'd really like to get rid of the 3 remaining users.

The fwnode graph API has fwnode_graph_get_endpoint_by_id() which can also
be used to obtain endpoints within a port. It does the same than
of_graph_get_endpoint_by_regs() with the addition that it also has a
flags field to allow e.g. returning endpoints with regs higher than
requested (FWNODE_GRAPH_ENDPOINT_NEXT).

Most users dealing with endpoints on fwnode property API use this, could
something like this be done on OF as well? Probably a similar flag would be
needed though.
Rob Herring (Arm) Aug. 27, 2024, 1:54 p.m. UTC | #2
+Jonathan C for the naming

On Mon, Aug 26, 2024 at 7:14 PM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>
>
> Hi Rob
>
> > > We already have of_graph_get_next_endpoint(), but it is not
> > > intuitive to use in some case.
> >
> > Can of_graph_get_next_endpoint() users be replaced with your new
> > helpers? I'd really like to get rid of the 3 remaining users.
>
> Hmm...
> of_graph_get_next_endpoint() will fetch "endpoint" beyond the "port",
> but new helper doesn't have such feature.

Right, but the "feature" is somewhat awkward as you said. You
generally should know what port you are operating on.

> Even though I try to replace it with new helper, I guess it will be
> almost same as current of_graph_get_next_endpoint() anyway.
>
> Alternative idea is...
> One of the big user of of_graph_get_next_endpoint() is
> for_each_endpoint_of_node() loop.
>
> So we can replace it to..
>
> -       for_each_endpoint_of_node(parent, endpoint) {
> +       for_each_of_graph_port(parent, port) {
> +               for_each_of_graph_port_endpoint(port, endpoint) {
>
> Above is possible, but it replaces single loop to multi loops.
>
> And, we still need to consider about of_fwnode_graph_get_next_endpoint()
> which is the last user of of_graph_get_next_endpoint()

I missed fwnode_graph_get_next_endpoint() which has lots of users.
Though almost all of those are just "get the endpoint" and assume
there is only 1. In any case, it's a lot more than 3, so nevermind for
now.

> > > +struct device_node *of_graph_get_next_port_endpoint(const struct device_node *port,
> > > +                                               struct device_node *prev)
> > > +{
> > > +   do {
> > > +           prev = of_get_next_child(port, prev);
> > > +           if (!prev)
> > > +                   break;
> > > +   } while (!of_node_name_eq(prev, "endpoint"));
> >
> > Really, this check is validation as no other name is valid in a
> > port node. The kernel is not responsible for validation, but okay.
> > However, if we are going to keep this, might as well make it WARN().
>
> OK, will do in v4
>
> > > +/**
> > > + * for_each_of_graph_port_endpoint - iterate over every endpoint in a port node
> > > + * @parent: parent port node
> > > + * @child: loop variable pointing to the current endpoint node
> > > + *
> > > + * When breaking out of the loop, of_node_put(child) has to be called manually.
> >
> > No need for this requirement anymore. Use cleanup.h so this is
> > automatic.
>
> Do you mean it should include __free() inside this loop, like _scoped() ?

Yes.

> #define for_each_child_of_node_scoped(parent, child) \
>         for (struct device_node *child __free(device_node) =            \
>              of_get_next_child(parent, NULL);                           \
>              child != NULL;                                             \
>              child = of_get_next_child(parent, child))
>
> In such case, I wonder does it need to have _scoped() in loop name ?

Well, we added that to avoid changing all the users at once.

> And in such case, I think we want to have non _scoped() loop too ?

Do we have a user? I don't think we need it because anywhere we need
the node iterator pointer outside the loop that can be done explicitly
(no_free_ptr()).

So back to the name, I don't think we need _scoped in it. I think if
any user treats the iterator like it's the old style, the compiler is
going to complain.

> For example, when user want to use the param.
>
>         for_each_of_graph_port_endpoint(port, endpoint)
>                 if (xxx == yyy)
>                         return endpoint;
>
>         for_each_of_graph_port_endpoint_scoped(port, endpoint)
>                 if (xxx == yyy)
>                         return of_node_get(endpoint)

Actually, you would do "return_ptr(endpoint)" here.

Rob
Rob Herring (Arm) Aug. 27, 2024, 2:05 p.m. UTC | #3
On Tue, Aug 27, 2024 at 5:47 AM Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Rob, Kunimori-san,
>
> On Mon, Aug 26, 2024 at 10:40:09AM -0500, Rob Herring wrote:
> > On Mon, Aug 26, 2024 at 02:43:28AM +0000, Kuninori Morimoto wrote:
> > > We already have of_graph_get_next_endpoint(), but it is not
> > > intuitive to use in some case.
> >
> > Can of_graph_get_next_endpoint() users be replaced with your new
> > helpers? I'd really like to get rid of the 3 remaining users.
>
> The fwnode graph API has fwnode_graph_get_endpoint_by_id() which can also
> be used to obtain endpoints within a port. It does the same than
> of_graph_get_endpoint_by_regs() with the addition that it also has a
> flags field to allow e.g. returning endpoints with regs higher than
> requested (FWNODE_GRAPH_ENDPOINT_NEXT).

Looks to me like FWNODE_GRAPH_ENDPOINT_NEXT is always used with
endpoint #0. That's equivalent to passing -1 for the endpoint number
with the OF API.

> Most users dealing with endpoints on fwnode property API use this, could
> something like this be done on OF as well? Probably a similar flag would be
> needed though.

I had fixed almost all the OF cases at one point. Unfortunately, there
were a few corner cases that I didn't address to eliminate the API. So
now it has proliferated with the fwnode API.

Rob
Sakari Ailus Aug. 27, 2024, 2:15 p.m. UTC | #4
Hi Rob,

On Tue, Aug 27, 2024 at 09:05:02AM -0500, Rob Herring wrote:
> On Tue, Aug 27, 2024 at 5:47 AM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> > Rob, Kunimori-san,
> >
> > On Mon, Aug 26, 2024 at 10:40:09AM -0500, Rob Herring wrote:
> > > On Mon, Aug 26, 2024 at 02:43:28AM +0000, Kuninori Morimoto wrote:
> > > > We already have of_graph_get_next_endpoint(), but it is not
> > > > intuitive to use in some case.
> > >
> > > Can of_graph_get_next_endpoint() users be replaced with your new
> > > helpers? I'd really like to get rid of the 3 remaining users.
> >
> > The fwnode graph API has fwnode_graph_get_endpoint_by_id() which can also
> > be used to obtain endpoints within a port. It does the same than
> > of_graph_get_endpoint_by_regs() with the addition that it also has a
> > flags field to allow e.g. returning endpoints with regs higher than
> > requested (FWNODE_GRAPH_ENDPOINT_NEXT).
> 
> Looks to me like FWNODE_GRAPH_ENDPOINT_NEXT is always used with
> endpoint #0. That's equivalent to passing -1 for the endpoint number
> with the OF API.

If the caller needs a single endpoint only, then the two are the same, yes.
The NEXT flag can still be used for obtaining further endpoints, unlike
setting endpoint to -1 in of_graph_get_endpoint_by_regs(). 

> 
> > Most users dealing with endpoints on fwnode property API use this, could
> > something like this be done on OF as well? Probably a similar flag would be
> > needed though.
> 
> I had fixed almost all the OF cases at one point. Unfortunately, there
> were a few corner cases that I didn't address to eliminate the API. So
> now it has proliferated with the fwnode API.

Much of the usage of fwnode_graph_get_next_endpoint() is conversion from
the OF API but there are some newer drivers, too. I admit I've sometimes
missed this in review. At the same time I can say most users in the media
tree do employ fwnode_graph_get_endpoint_by_id() already.

The good thing is that almost all current users are camera sensors and
converting them is fairly trivial. I can post patches but it'll take a
while...
Kuninori Morimoto Aug. 28, 2024, 12:56 a.m. UTC | #5
Hi Rob

> > And, we still need to consider about of_fwnode_graph_get_next_endpoint()
> > which is the last user of of_graph_get_next_endpoint()
> 
> I missed fwnode_graph_get_next_endpoint() which has lots of users.
> Though almost all of those are just "get the endpoint" and assume
> there is only 1. In any case, it's a lot more than 3, so nevermind for
> now.

OK, thanks.

> So back to the name, I don't think we need _scoped in it. I think if
> any user treats the iterator like it's the old style, the compiler is
> going to complain.
> 
> > For example, when user want to use the param.
> >
> >         for_each_of_graph_port_endpoint(port, endpoint)
> >                 if (xxx == yyy)
> >                         return endpoint;
> >
> >         for_each_of_graph_port_endpoint_scoped(port, endpoint)
> >                 if (xxx == yyy)
> >                         return of_node_get(endpoint)
> 
> Actually, you would do "return_ptr(endpoint)" here.

OK, nice to know about this
I will try to use this style on v4


Thank you for your help !!

Best regards
---
Kuninori Morimoto
Jonathan Cameron Aug. 31, 2024, 10:24 a.m. UTC | #6
On Tue, 27 Aug 2024 08:54:51 -0500
Rob Herring <robh@kernel.org> wrote:

> +Jonathan C for the naming
> 
> On Mon, Aug 26, 2024 at 7:14 PM Kuninori Morimoto
> <kuninori.morimoto.gx@renesas.com> wrote:
> >
> >
> > Hi Rob
> >  
> > > > We already have of_graph_get_next_endpoint(), but it is not
> > > > intuitive to use in some case.  
> > >
> > > Can of_graph_get_next_endpoint() users be replaced with your new
> > > helpers? I'd really like to get rid of the 3 remaining users.  
> >
> > Hmm...
> > of_graph_get_next_endpoint() will fetch "endpoint" beyond the "port",
> > but new helper doesn't have such feature.  
> 
> Right, but the "feature" is somewhat awkward as you said. You
> generally should know what port you are operating on.
> 
> > Even though I try to replace it with new helper, I guess it will be
> > almost same as current of_graph_get_next_endpoint() anyway.
> >
> > Alternative idea is...
> > One of the big user of of_graph_get_next_endpoint() is
> > for_each_endpoint_of_node() loop.
> >
> > So we can replace it to..
> >
> > -       for_each_endpoint_of_node(parent, endpoint) {
> > +       for_each_of_graph_port(parent, port) {
> > +               for_each_of_graph_port_endpoint(port, endpoint) {
> >
> > Above is possible, but it replaces single loop to multi loops.
> >
> > And, we still need to consider about of_fwnode_graph_get_next_endpoint()
> > which is the last user of of_graph_get_next_endpoint()  
> 
> I missed fwnode_graph_get_next_endpoint() which has lots of users.
> Though almost all of those are just "get the endpoint" and assume
> there is only 1. In any case, it's a lot more than 3, so nevermind for
> now.
> 
> > > > +struct device_node *of_graph_get_next_port_endpoint(const struct device_node *port,
> > > > +                                               struct device_node *prev)
> > > > +{
> > > > +   do {
> > > > +           prev = of_get_next_child(port, prev);
> > > > +           if (!prev)
> > > > +                   break;
> > > > +   } while (!of_node_name_eq(prev, "endpoint"));  
> > >
> > > Really, this check is validation as no other name is valid in a
> > > port node. The kernel is not responsible for validation, but okay.
> > > However, if we are going to keep this, might as well make it WARN().  
> >
> > OK, will do in v4
> >  
> > > > +/**
> > > > + * for_each_of_graph_port_endpoint - iterate over every endpoint in a port node
> > > > + * @parent: parent port node
> > > > + * @child: loop variable pointing to the current endpoint node
> > > > + *
> > > > + * When breaking out of the loop, of_node_put(child) has to be called manually.  
> > >
> > > No need for this requirement anymore. Use cleanup.h so this is
> > > automatic.  
> >
> > Do you mean it should include __free() inside this loop, like _scoped() ?  
> 
> Yes.
> 
> > #define for_each_child_of_node_scoped(parent, child) \
> >         for (struct device_node *child __free(device_node) =            \
> >              of_get_next_child(parent, NULL);                           \
> >              child != NULL;                                             \
> >              child = of_get_next_child(parent, child))
> >
> > In such case, I wonder does it need to have _scoped() in loop name ?  
> 
> Well, we added that to avoid changing all the users at once.
> 
> > And in such case, I think we want to have non _scoped() loop too ?  
> 
> Do we have a user? I don't think we need it because anywhere we need
> the node iterator pointer outside the loop that can be done explicitly
> (no_free_ptr()).
> 
> So back to the name, I don't think we need _scoped in it. I think if
> any user treats the iterator like it's the old style, the compiler is
> going to complain.

Hmm.  Up to you but I'd be concerned that the scoping stuff is non
obvious enough that it is worth making people really really aware
it is going on.

However I don't feel strongly about it.
For the other _scoped iterators there is some push back
on the churn using them is causing so I doubt we'll ever get rid
of the non scoped variants.  For something new that's not a concern.

Jonathan

> 
> > For example, when user want to use the param.
> >
> >         for_each_of_graph_port_endpoint(port, endpoint)
> >                 if (xxx == yyy)
> >                         return endpoint;
> >
> >         for_each_of_graph_port_endpoint_scoped(port, endpoint)
> >                 if (xxx == yyy)
> >                         return of_node_get(endpoint)  
> 
> Actually, you would do "return_ptr(endpoint)" here.
> 
> Rob
Kuninori Morimoto Sept. 2, 2024, 4 a.m. UTC | #7
Hi Jonathan, Rob

> > > Do you mean it should include __free() inside this loop, like _scoped() ?
(snip)
> > > In such case, I wonder does it need to have _scoped() in loop name ?
(snip)
> > So back to the name, I don't think we need _scoped in it. I think if
> > any user treats the iterator like it's the old style, the compiler is
> > going to complain.
> 
> Hmm.  Up to you but I'd be concerned that the scoping stuff is non
> obvious enough that it is worth making people really really aware
> it is going on.
> 
> However I don't feel strongly about it.
> For the other _scoped iterators there is some push back
> on the churn using them is causing so I doubt we'll ever get rid
> of the non scoped variants.  For something new that's not a concern.

I noticed that we can write below code, and then, and there is no waning/error
from compiler.

Now for_each macro is using __free()

	#define for_each_of_graph_port(parent, child)	\
		for (... *child __free(device_node) = ...)

(A)	struct device_node *node = xxx;

	for_each_of_graph_port(parent, node) {
(B)		/* do something */
	}

(C)	xxx = node;

In this case, "(A) node" and "(C) node" are same, but "(B) node" are different.
New user might confuse about this behavior.

Thank you for your help !!

Best regards
---
Kuninori Morimoto
diff mbox series

Patch

diff --git a/drivers/of/property.c b/drivers/of/property.c
index aec6ac9f70064..90820e43bc973 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -719,6 +719,28 @@  struct device_node *of_graph_get_next_port(struct device_node *parent,
 }
 EXPORT_SYMBOL(of_graph_get_next_port);
 
+/**
+ * of_graph_get_next_port_endpoint() - get next endpoint node in port.
+ * If it reached to end of the port, it will return NULL.
+ * @port: pointer to the target port node
+ * @prev: previous 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_port_endpoint(const struct device_node *port,
+						    struct device_node *prev)
+{
+	do {
+		prev = of_get_next_child(port, prev);
+		if (!prev)
+			break;
+	} while (!of_node_name_eq(prev, "endpoint"));
+
+	return prev;
+}
+EXPORT_SYMBOL(of_graph_get_next_port_endpoint);
+
 /**
  * of_graph_get_next_endpoint() - get next endpoint node
  * @parent: pointer to the parent device node
diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
index a6b91577700a8..967ee14a1ff37 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -59,6 +59,17 @@  struct of_endpoint {
 	for (child = of_graph_get_next_port(parent, NULL); child != NULL; \
 	     child = of_graph_get_next_port(parent, child))
 
+/**
+ * for_each_of_graph_port_endpoint - iterate over every endpoint in a port node
+ * @parent: parent port node
+ * @child: loop variable pointing to the current endpoint node
+ *
+ * When breaking out of the loop, of_node_put(child) has to be called manually.
+ */
+#define for_each_of_graph_port_endpoint(parent, child)			\
+		for (child = of_graph_get_next_port_endpoint(parent, NULL); child != NULL; \
+		     child = of_graph_get_next_port_endpoint(parent, child))
+
 #ifdef CONFIG_OF
 bool of_graph_is_present(const struct device_node *node);
 int of_graph_parse_endpoint(const struct device_node *node,
@@ -72,6 +83,8 @@  struct device_node *of_graph_get_next_ports(struct device_node *parent,
 					    struct device_node *ports);
 struct device_node *of_graph_get_next_port(struct device_node *parent,
 					   struct device_node *port);
+struct device_node *of_graph_get_next_port_endpoint(const struct device_node *port,
+						    struct device_node *prev);
 struct device_node *of_graph_get_endpoint_by_regs(
 		const struct device_node *parent, int port_reg, int reg);
 struct device_node *of_graph_get_remote_endpoint(
@@ -132,6 +145,13 @@  static inline struct device_node *of_graph_get_next_port(
 	return NULL;
 }
 
+static inline struct device_node *of_graph_get_next_port_endpoint(
+					const struct device_node *parent,
+					struct device_node *previous)
+{
+	return NULL;
+}
+
 static inline struct device_node *of_graph_get_endpoint_by_regs(
 		const struct device_node *parent, int port_reg, int reg)
 {