| Message ID | 87o9aa0whf.wl-kuninori.morimoto.gx@renesas.com |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | OF: add functions for port | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
On Tue, Nov 27, 2018 at 8:34 PM Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > > > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > We have for_each_endpoint_of_node(), but don't have port version of it. > It is useful if we have it. This patch adds it. I'd like to see the user for this first. Generally, I don't think iterating over port or endpoint nodes is the right way to walk the graph. A given port address has a defined function and you should be requesting the specific ports. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > drivers/of/property.c | 37 +++++++++++++++++++++++++++++++++++++ > include/linux/of_graph.h | 20 ++++++++++++++++++++ > 2 files changed, 57 insertions(+) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 3db01ee..2ec4c95 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -585,6 +585,43 @@ struct device_node *of_graph_get_port_by_id(struct device_node *parent, u32 id) > EXPORT_SYMBOL(of_graph_get_port_by_id); > > /** > + * of_graph_get_next_port() - get next port node > + * @parent: pointer to the parent device node > + * @prev: previous port node, or NULL to get first > + * > + * Return: An 'port' node pointer with refcount incremented. Refcount > + * of the passed @prev node is decremented. > + */ > +struct device_node *of_graph_get_next_port(const struct device_node *parent, > + struct device_node *prev) > +{ > + struct device_node *port; > + > + if (!parent) > + return NULL; > + > + if (!prev) { > + struct device_node *node; > + > + node = of_get_child_by_name(parent, "ports"); > + if (node) > + parent = node; > + port = NULL; > + } else { > + port = prev; > + } > + > + do { > + port = of_get_next_child(parent, port); > + if (!port) > + return NULL; > + } while (of_node_cmp(port->name, "port")); Use of_node_name_eq() > + > + return port; > +} > +EXPORT_SYMBOL(of_graph_get_next_port); > + > +/** > * of_graph_get_next_endpoint() - get next endpoint node > * @parent: pointer to the parent device node > * @prev: previous endpoint node, or NULL to get first > diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h > index 7fbbf80..d219cc7 100644 > --- a/include/linux/of_graph.h > +++ b/include/linux/of_graph.h > @@ -27,6 +27,17 @@ struct of_endpoint { > }; > > /** > + * for_each_port_of_node - iterate over every port in a device node > + * @parent: parent device node containing ports > + * @child: loop variable pointing to the current port node > + * > + * When breaking out of the loop, of_node_put(child) has to be called manually. > + */ > +#define for_each_port_of_node(parent, child) \ > + for (child = of_graph_get_next_port(parent, NULL); child != NULL; \ > + child = of_graph_get_next_port(parent, child)) > + > +/** > * for_each_endpoint_of_node - iterate over every endpoint in a device node > * @parent: parent device node containing ports and endpoints > * @child: loop variable pointing to the current endpoint node > @@ -44,6 +55,8 @@ int of_graph_parse_endpoint(const struct device_node *node, > struct of_endpoint *endpoint); > int of_graph_get_endpoint_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_port(const struct device_node *parent, > + struct device_node *previous); > struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, > struct device_node *previous); > struct device_node *of_graph_get_endpoint_by_regs( > @@ -75,6 +88,13 @@ static inline struct device_node *of_graph_get_port_by_id( > return NULL; > } > > +static inline struct device_node *of_graph_get_next_port( > + const struct device_node *parent, > + struct device_node *previous) > +{ > + return NULL; > +} > + > static inline struct device_node *of_graph_get_next_endpoint( > const struct device_node *parent, > struct device_node *previous) > -- > 2.7.4 >
Hi Rob again > > + do { > > + port = of_get_next_child(parent, port); > > + if (!port) > > + return NULL; > > + } while (of_node_cmp(port->name, "port")); > > Use of_node_name_eq() I tried this, but unfortunately it seems of_node_name_eq() is not good match to this purpose. Because it try to check with "port@n". # I noticed that I can cleanup this patch to more simpler. # I want to send v2 patch if this patch is not rejected # Please let me know Best regards --- Kuninori Morimoto
On Tue, Nov 27, 2018 at 10:46 PM Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > > > Hi Rob > > Thank you for reviewing > > > I'd like to see the user for this first. > > > > Generally, I don't think iterating over port or endpoint nodes is the > > right way to walk the graph. A given port address has a defined > > function and you should be requesting the specific ports. > > I attached patch which is using for_each_port_of_node(). > I want to know "specified" port location. > > ------------- > From 1abf55f28cc12c207bce646e579f19ded117d5c6 Mon Sep 17 00:00:00 2001 > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Date: Thu, 15 Nov 2018 11:18:31 +0900 > Subject: [PATCH] ASoC: simple-card-utils: fixup asoc_simple_card_get_dai_id() > counting > > asoc_simple_card_get_dai_id() returns DAI ID, but it is based on > DT node's "endpoint" count. > Almost all cases 1 port has 1 endpoint, thus, it was no problem. > But in reality, port : endpoint = 1 : N, thus, counting endpoint > is BUG, it should count "port". > This patch fixup it. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > sound/soc/generic/simple-card-utils.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c > index c69ce1e..119f831 100644 > --- a/sound/soc/generic/simple-card-utils.c > +++ b/sound/soc/generic/simple-card-utils.c > @@ -270,7 +270,8 @@ EXPORT_SYMBOL_GPL(asoc_simple_card_parse_dai); > static int asoc_simple_card_get_dai_id(struct device_node *ep) > { > struct device_node *node; > - struct device_node *endpoint; > + struct device_node *p; > + struct device_node *port; > int i, id; > int ret; > > @@ -279,20 +280,22 @@ static int asoc_simple_card_get_dai_id(struct device_node *ep) > return ret; > > node = of_graph_get_port_parent(ep); > + port = of_graph_get_port(ep); > > /* > - * Non HDMI sound case, counting port/endpoint on its DT > + * Non HDMI sound case, counting port on its DT > * is enough. Let's count it. > */ > i = 0; > id = -1; > - for_each_endpoint_of_node(node, endpoint) { > - if (endpoint == ep) > + for_each_port_of_node(node, p) { > + if (port == p) > id = i; > i++; > } This is fragile because it assumes 0-N port numbering. While that's usually true, it's not guaranteed. Just add a of_graph_get_port_id() that reads and returns 'reg' value or 0 if no 'reg'. Or just use of_graph_parse_endpoint(). Rob
Hi Rob > > - for_each_endpoint_of_node(node, endpoint) { > > - if (endpoint == ep) > > + for_each_port_of_node(node, p) { > > + if (port == p) > > id = i; > > i++; > > } > > This is fragile because it assumes 0-N port numbering. While that's > usually true, it's not guaranteed. > > Just add a of_graph_get_port_id() that reads and returns 'reg' value > or 0 if no 'reg'. Or just use of_graph_parse_endpoint(). OK, will think about this. How about [1/2] patch ? Is it good or not ? Best regards --- Kuninori Morimoto
Hi Rob, again > > > - for_each_endpoint_of_node(node, endpoint) { > > > - if (endpoint == ep) > > > + for_each_port_of_node(node, p) { > > > + if (port == p) > > > id = i; > > > i++; > > > } > > > > This is fragile because it assumes 0-N port numbering. While that's > > usually true, it's not guaranteed. > > > > Just add a of_graph_get_port_id() that reads and returns 'reg' value > > or 0 if no 'reg'. Or just use of_graph_parse_endpoint(). > > OK, will think about this. > How about [1/2] patch ? Is it good or not ? Thank you for your advice. of_graph_parse_endpoint() was good enough ! Best regards --- Kuninori Morimoto
diff --git a/drivers/of/property.c b/drivers/of/property.c index 3db01ee..2ec4c95 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -585,6 +585,43 @@ struct device_node *of_graph_get_port_by_id(struct device_node *parent, u32 id) EXPORT_SYMBOL(of_graph_get_port_by_id); /** + * of_graph_get_next_port() - get next port node + * @parent: pointer to the parent device node + * @prev: previous port node, or NULL to get first + * + * Return: An 'port' node pointer with refcount incremented. Refcount + * of the passed @prev node is decremented. + */ +struct device_node *of_graph_get_next_port(const struct device_node *parent, + struct device_node *prev) +{ + struct device_node *port; + + if (!parent) + return NULL; + + if (!prev) { + struct device_node *node; + + node = of_get_child_by_name(parent, "ports"); + if (node) + parent = node; + port = NULL; + } else { + port = prev; + } + + do { + port = of_get_next_child(parent, port); + if (!port) + return NULL; + } while (of_node_cmp(port->name, "port")); + + return port; +} +EXPORT_SYMBOL(of_graph_get_next_port); + +/** * of_graph_get_next_endpoint() - get next endpoint node * @parent: pointer to the parent device node * @prev: previous endpoint node, or NULL to get first diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h index 7fbbf80..d219cc7 100644 --- a/include/linux/of_graph.h +++ b/include/linux/of_graph.h @@ -27,6 +27,17 @@ struct of_endpoint { }; /** + * for_each_port_of_node - iterate over every port in a device node + * @parent: parent device node containing ports + * @child: loop variable pointing to the current port node + * + * When breaking out of the loop, of_node_put(child) has to be called manually. + */ +#define for_each_port_of_node(parent, child) \ + for (child = of_graph_get_next_port(parent, NULL); child != NULL; \ + child = of_graph_get_next_port(parent, child)) + +/** * for_each_endpoint_of_node - iterate over every endpoint in a device node * @parent: parent device node containing ports and endpoints * @child: loop variable pointing to the current endpoint node @@ -44,6 +55,8 @@ int of_graph_parse_endpoint(const struct device_node *node, struct of_endpoint *endpoint); int of_graph_get_endpoint_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_port(const struct device_node *parent, + struct device_node *previous); struct device_node *of_graph_get_next_endpoint(const struct device_node *parent, struct device_node *previous); struct device_node *of_graph_get_endpoint_by_regs( @@ -75,6 +88,13 @@ static inline struct device_node *of_graph_get_port_by_id( return NULL; } +static inline struct device_node *of_graph_get_next_port( + const struct device_node *parent, + struct device_node *previous) +{ + return NULL; +} + static inline struct device_node *of_graph_get_next_endpoint( const struct device_node *parent, struct device_node *previous)