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. > > (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@xxxxxxxxxxx> > --- > drivers/of/property.c | 22 ++++++++++++++++++++++ > include/linux/of_graph.h | 20 ++++++++++++++++++++ > 2 files changed, 42 insertions(+) > > 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")); 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(). > + > + 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. No need for this requirement anymore. Use cleanup.h so this is automatic. > + */ > +#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) > { > -- > 2.43.0 >