| Message ID | 87jzguw8ln.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | of: property: add of_graph_get_next_port/port_endpoint() | expand |
On 06/08/2024 07:58, 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 > > /* These will be node/ports/port@0/endpoint */ > of_graph_get_next_endpoint(node, NULL); > of_graph_get_next_endpoint(ports, NULL); > > 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 > > /* > * "ep" is now A1, and handle port1 things here, > * but we don't know how many endpoints port1 has. > * > * Because "ep" is non NULL, 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 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 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); // A1 > of_graph_get_next_port_endpoint(port1, A1); // A2 > of_graph_get_next_port_endpoint(port1, A2); // 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(+) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 3b2d09c0376a..de56795a7362 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -692,6 +692,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 > + * @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. > + */ Same issues here too. No "prev" parameter, and I suggest using "previous", not "current", to be consistent with of_graph_get_next_endpoint(). (or alternatively, change of_graph_get_next_endpoint()). Oh, the declaration of the function uses "prev", but the implementation "endpoint". Please make the naming same. > +struct device_node *of_graph_get_next_port_endpoint(const struct device_node *port, > + struct device_node *endpoint) > +{ > + 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_port_endpoint); > + > /** > * of_graph_get_next_endpoint() - get next endpoint node > * > diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h > index 30169b50b042..8b4777938c5e 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 device or ports node Hmm, shouldn't the parent be a 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, > @@ -73,6 +84,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( > @@ -133,6 +146,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) > { Tomi
Hi Tomi > > +/** > > + * 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 > > + * @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. > > + */ > > Same issues here too. No "prev" parameter, and I suggest using > "previous", not "current", to be consistent with > of_graph_get_next_endpoint(). (or alternatively, change > of_graph_get_next_endpoint()). > > Oh, the declaration of the function uses "prev", but the implementation > "endpoint". Please make the naming same. Will fix in v2. But it will use "prev" for param same as of_graph_get_next_endpoint() > > +/** > > + * for_each_of_graph_port_endpoint - iterate over every endpoint in a port node > > + * @parent: parent device or ports node > > Hmm, shouldn't the parent be a port node? Thanks, will fix in v2 Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/drivers/of/property.c b/drivers/of/property.c index 3b2d09c0376a..de56795a7362 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -692,6 +692,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 + * @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_port_endpoint(const struct device_node *port, + struct device_node *endpoint) +{ + 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_port_endpoint); + /** * of_graph_get_next_endpoint() - get next endpoint node * diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h index 30169b50b042..8b4777938c5e 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 device or ports 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, @@ -73,6 +84,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( @@ -133,6 +146,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. (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 /* These will be node/ports/port@0/endpoint */ of_graph_get_next_endpoint(node, NULL); of_graph_get_next_endpoint(ports, NULL); 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 /* * "ep" is now A1, and handle port1 things here, * but we don't know how many endpoints port1 has. * * Because "ep" is non NULL, 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 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 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); // A1 of_graph_get_next_port_endpoint(port1, A1); // A2 of_graph_get_next_port_endpoint(port1, A2); // 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(+)