| 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 |
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.
+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
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
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...
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
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
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 --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) {
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(+)