Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <87pn29pc5w.wl-kuninori.morimoto.gx@renesas.com>
In-Reply-To: <87pn29pc5w.wl-kuninori.morimoto.gx@renesas.com>
From:   Rob Herring <robh+dt@kernel.org>
Date:   Wed, 13 Jan 2021 19:10:41 -0600
Message-ID: <CAL_JsqLEDd57L9edqtHQGf92oJZ02w-C8kZ064ijdcWbohwOhA@mail.gmail.com>
Subject: Re: [PATCH] of: property: add port base loop
To:     Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Cc:     Frank Rowand <frowand.list@gmail.com>, devicetree@vger.kernel.org,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jan 13, 2021 at 12:53 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>
>
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> We have endpoint base functions
>         - of_graph_get_next_endpoint()
>         - of_graph_get_endpoint_count()
>         - for_each_endpoint_of_node()

I actually think these functions don't make sense. Iterating over
endpoints for a port make sense, but not all endpoints.

> Here, for_each_endpoint_of_node() loop finds endpoint
>
>         ports {
>                 port@0 {
> (1)                     endpoint {...};
>                 };
>                 port@1 {
> (2)                     endpoint {...};
>                 };
>                 ...
>         };
>
> In above case, for_each_endpoint_of_node() loop
> finds endpoint as (1) -> (2) -> ...
> We can check endpoint parent to get its port
> if user want to do something to it.
>
> But port can have multi endpoints.
> In such case, it is difficult to find
> port@0 -> port@1 -> ...
>
>         ports {
>                 port@0 {
> (1)                     endpoint@0 {...};
> (2)                     endpoint@1 {...};
>                 };
>                 port@1 {
> (3)                     endpoint {...};
>                 };
>                 ...
>         };
>
> In such case, people want to have port base loop
> instead of endpoints base loop.
> This patch adds such functions/macros.

I'm a bit hesitant on these too. A driver should generally know what
each port # is (since the binding has to define them), and it should
just request the port (or its connection) it wants. At least that was
the premise behind of_graph_get_remote_node() and the cleanups (mostly
DRM drivers) I did to use it. I'd rather see things move in that
direction.

In any case, this needs a user before merging.

>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/of/property.c    | 69 ++++++++++++++++++++++++++++++++++------
>  include/linux/of_graph.h | 14 ++++++++
>  2 files changed, 73 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 5f9eed79a8aa..9b511cfe97b3 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -631,15 +631,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
>          * parent port node.
>          */
>         if (!prev) {
> -               struct device_node *node;
> -
> -               node = of_get_child_by_name(parent, "ports");
> -               if (node)
> -                       parent = node;
> -
> -               port = of_get_child_by_name(parent, "port");
> -               of_node_put(node);
> -
> +               port = of_graph_get_next_port(parent, NULL);
>                 if (!port) {
>                         pr_err("graph: no port node found in %pOF\n", parent);
>                         return NULL;
> @@ -666,14 +658,59 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
>                 /* No more endpoints under this port, try the next one. */
>                 prev = NULL;
>
> +               port = of_graph_get_next_port(parent, port);
> +               if (!port)
> +                       return NULL;
> +       }
> +}
> +EXPORT_SYMBOL(of_graph_get_next_endpoint);
> +
> +/**
> + * 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 = prev;
> +
> +       if (!parent)
> +               return NULL;
> +
> +       /*
> +        * Start by locating the port node. If no previous endpoint is specified
> +        * search for the first port node, otherwise get the previous endpoint
> +        * parent port node.
> +        */
> +       if (!port) {
> +               struct device_node *node;
> +
> +               node = of_get_child_by_name(parent, "ports");
> +               if (node)
> +                       parent = node;
> +
> +               port = of_get_child_by_name(parent, "port");
> +               of_node_put(node);
> +
> +               if (!port) {
> +                       pr_err("graph: no port node found in %pOF\n", parent);
> +                       return NULL;
> +               }
> +       } else {
>                 do {
>                         port = of_get_next_child(parent, port);
>                         if (!port)
>                                 return NULL;
>                 } while (!of_node_name_eq(port, "port"));
>         }
> +
> +       return port;
>  }
> -EXPORT_SYMBOL(of_graph_get_next_endpoint);
> +EXPORT_SYMBOL(of_graph_get_next_port);
>
>  /**
>   * of_graph_get_endpoint_by_regs() - get endpoint node of specific identifiers
> @@ -800,6 +837,18 @@ int of_graph_get_endpoint_count(const struct device_node *np)
>  }
>  EXPORT_SYMBOL(of_graph_get_endpoint_count);
>
> +int of_graph_get_port_count(const struct device_node *np)
> +{
> +       struct device_node *port;
> +       int num = 0;
> +
> +       for_each_port_of_node(np, port)
> +               num++;
> +
> +       return num;
> +}
> +EXPORT_SYMBOL(of_graph_get_port_count);
> +
>  /**
>   * of_graph_get_remote_node() - get remote parent device_node for given port/endpoint
>   * @node: pointer to parent device_node containing graph port/endpoint
> diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
> index 4d7756087b6b..8cd3bd674ebd 100644
> --- a/include/linux/of_graph.h
> +++ b/include/linux/of_graph.h
> @@ -26,6 +26,17 @@ struct of_endpoint {
>         const struct device_node *local_node;
>  };
>
> +/**
> + * for_each_port_of_node - iterate over every port in a device node
> + * @parent: parent device node containing ports and port
> + * @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
> @@ -41,8 +52,11 @@ struct of_endpoint {
>  bool of_graph_is_present(const struct device_node *node);
>  int of_graph_parse_endpoint(const struct device_node *node,
>                                 struct of_endpoint *endpoint);
> +int of_graph_get_port_count(const struct device_node *np);
>  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(
> --
> 2.25.1
>