[PATCH 0/2][resend] OF: add functions for port

[PATCH 0/2][resend] OF: add functions for port

[Kuninori Morimoto]

Hi Rob, Frank

2weeks passed, I resend these patches again.

Kuninori Morimoto (2):
  OF: add of_graph_get_port()
  OF: add for_each_port_of_node()

 drivers/of/property.c    | 45 +++++++++++++++++++++++++++++++++++++++++----
 include/linux/of_graph.h | 22 ++++++++++++++++++++++
 2 files changed, 63 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH 1/2][resend] OF: add of_graph_get_port()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

We have of_graph_get_port_parent(), of_graph_get_port_by_id(),
but don't have of_graph_get_port().
Thus we are using of_get_parent() to getting port from endpoint,
but it is not understandable. Let's add of_graph_get_port().

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

diff --git a/drivers/of/property.c b/drivers/of/property.c
index f46828e..3db01ee 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -531,7 +531,7 @@ EXPORT_SYMBOL_GPL(of_prop_next_string);
 int of_graph_parse_endpoint(const struct device_node *node,
 			    struct of_endpoint *endpoint)
 {
-	struct device_node *port_node = of_get_parent(node);
+	struct device_node *port_node = of_graph_get_port(node);
 
 	WARN_ONCE(!port_node, "%s(): endpoint %pOF has no parent node\n",
 		  __func__, node);
@@ -621,7 +621,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 			return NULL;
 		}
 	} else {
-		port = of_get_parent(prev);
+		port = of_graph_get_port(prev);
 		if (WARN_ONCE(!port, "%s(): endpoint %pOF has no parent node\n",
 			      __func__, prev))
 			return NULL;
@@ -874,7 +874,7 @@ of_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
 static struct fwnode_handle *
 of_fwnode_get_parent(const struct fwnode_handle *fwnode)
 {
-	return of_fwnode_handle(of_get_parent(to_of_node(fwnode)));
+	return of_fwnode_handle(of_graph_get_port(to_of_node(fwnode)));
 }
 
 static struct fwnode_handle *
@@ -965,7 +965,7 @@ static int of_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 					  struct fwnode_endpoint *endpoint)
 {
 	const struct device_node *node = to_of_node(fwnode);
-	struct device_node *port_node = of_get_parent(node);
+	struct device_node *port_node = of_graph_get_port(node);
 
 	endpoint->local_fwnode = fwnode;
 
diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
index 01038a6..7fbbf80 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -37,6 +37,8 @@ struct of_endpoint {
 	for (child = of_graph_get_next_endpoint(parent, NULL); child != NULL; \
 	     child = of_graph_get_next_endpoint(parent, child))
 
+#define of_graph_get_port(endpoint) of_get_parent(endpoint)
+
 #ifdef CONFIG_OF
 int of_graph_parse_endpoint(const struct device_node *node,
 				struct of_endpoint *endpoint);
-- 
2.7.4

[PATCH 2/2][resend] OF: add for_each_port_of_node()

[Kuninori Morimoto]

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.

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"));
+
+	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

Re: [PATCH 2/2][resend] OF: add for_each_port_of_node()

[Rob Herring]

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
>

Re: [PATCH 2/2][resend] OF: add for_each_port_of_node()

[Kuninori Morimoto]

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++;
 	}
 
 	of_node_put(node);
+	of_node_put(port);
 
 	if (id < 0)
 		return -ENODEV;
-- 
2.7.4

Re: [PATCH 2/2][resend] OF: add for_each_port_of_node()

[Rob Herring]

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

Re: [PATCH 2/2][resend] OF: add for_each_port_of_node()

[Kuninori Morimoto]

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

Re: [PATCH 2/2][resend] OF: add for_each_port_of_node()

[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

Re: [PATCH 2/2][resend] OF: add for_each_port_of_node()

[Kuninori Morimoto]

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

Re: [PATCH 0/2][resend] OF: add functions for port

[Rob Herring]

On Tue, Nov 27, 2018 at 8:34 PM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>
>
> Hi Rob, Frank
>
> 2weeks passed, I resend these patches again.

Sorry, last 2 weeks has been conference and US holidays.

Rob

Re: [PATCH 0/2][resend] OF: add functions for port

[Kuninori Morimoto]

Hi Rob

> > Hi Rob, Frank
> >
> > 2weeks passed, I resend these patches again.
> 
> Sorry, last 2 weeks has been conference and US holidays.

OK :)
No problem

Best regards
---
Kuninori Morimoto