diff mbox

[1/5] of: introduce of_graph_get_remote_node

Message ID 20170204033635.10250-2-robh@kernel.org (mailing list archive)
State New, archived
Headers show

Commit Message

Rob Herring Feb. 4, 2017, 3:36 a.m. UTC
The OF graph API leaves too much of the graph walking to clients when
in many cases the driver doesn't care about accessing the port or
endpoint nodes. The drivers typically just want the device connected via
a particular graph connection. of_graph_get_remote_node provides this
functionality.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/of/base.c        | 28 ++++++++++++++++++++++++++++
 include/linux/of_graph.h |  8 ++++++++
 2 files changed, 36 insertions(+)

Comments

Daniel Vetter Feb. 6, 2017, 8:50 a.m. UTC | #1
On Fri, Feb 03, 2017 at 09:36:31PM -0600, Rob Herring wrote:
> The OF graph API leaves too much of the graph walking to clients when
> in many cases the driver doesn't care about accessing the port or
> endpoint nodes. The drivers typically just want the device connected via
> a particular graph connection. of_graph_get_remote_node provides this
> functionality.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>

Just a quick procedural comment: drm-misc for 4.11 is closed already, but
if we can get this core patch into 4.11 still then that would avoid
cross-tree sync pains in 4.12 ...
-Daniel

> ---
>  drivers/of/base.c        | 28 ++++++++++++++++++++++++++++
>  include/linux/of_graph.h |  8 ++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index d4bea3c797d6..ea18ab16b92c 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2469,3 +2469,31 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node)
>  	return of_get_next_parent(np);
>  }
>  EXPORT_SYMBOL(of_graph_get_remote_port);
> +
> +struct device_node *of_graph_get_remote_node(const struct device_node *node,
> +					     int port, int endpoint)
> +{
> +	struct device_node *endpoint_node, *remote;
> +
> +	endpoint_node = of_graph_get_endpoint_by_regs(node, port, endpoint);
> +	if (!endpoint_node) {
> +		pr_debug("no valid endpoint (%d, %d) for node %s\n",
> +			 port, endpoint, node->full_name);
> +		return NULL;
> +	}
> +
> +	remote = of_graph_get_remote_port_parent(endpoint_node);
> +	of_node_put(endpoint);
> +	if (!remote) {
> +		pr_debug("no valid remote node\n");
> +		return NULL;
> +	}
> +
> +	if (!of_device_is_available(remote)) {
> +		pr_debug("not available for remote node\n");
> +		return NULL;
> +	}
> +
> +	return remote;
> +}
> +EXPORT_SYMBOL(of_graph_get_remote_node);
> diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
> index bb3a5a2cd570..7b71d3e09209 100644
> --- a/include/linux/of_graph.h
> +++ b/include/linux/of_graph.h
> @@ -51,6 +51,8 @@ struct device_node *of_graph_get_endpoint_by_regs(
>  struct device_node *of_graph_get_remote_port_parent(
>  					const struct device_node *node);
>  struct device_node *of_graph_get_remote_port(const struct device_node *node);
> +struct device_node *of_graph_get_remote_node(const struct device_node *node,
> +					     int port, int endpoint);
>  #else
>  
>  static inline int of_graph_parse_endpoint(const struct device_node *node,
> @@ -89,6 +91,12 @@ static inline struct device_node *of_graph_get_remote_port(
>  {
>  	return NULL;
>  }
> +static inline struct device_node *of_graph_get_remote_node(
> +					const struct device_node *node,
> +					int port, int endpoint)
> +{
> +	return NULL;
> +}
>  
>  #endif /* CONFIG_OF */
>  
> -- 
> 2.10.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Philipp Zabel Feb. 6, 2017, 10:32 a.m. UTC | #2
Hi Rob,

thanks for this clean-up series! I was not aware how far the duplication
has spread over time.

On Fri, 2017-02-03 at 21:36 -0600, Rob Herring wrote:
> The OF graph API leaves too much of the graph walking to clients when
> in many cases the driver doesn't care about accessing the port or
> endpoint nodes. The drivers typically just want the device connected via
> a particular graph connection. of_graph_get_remote_node provides this
> functionality.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  drivers/of/base.c        | 28 ++++++++++++++++++++++++++++
>  include/linux/of_graph.h |  8 ++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index d4bea3c797d6..ea18ab16b92c 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2469,3 +2469,31 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node)
>  	return of_get_next_parent(np);
>  }
>  EXPORT_SYMBOL(of_graph_get_remote_port);
> +
> +struct device_node *of_graph_get_remote_node(const struct device_node *node,
> +					     int port, int endpoint)

I think this should have a documentation comment, similar to the
of_graph_get_endpoint_by_regs one, as it is not really clear from the
function name that the returned device node is the parent (or
grandparent) device node containing the remote port to the specified
node & port & endpoint.
Also it might be interesting to the user that -1 is a wildcard value for
port / endpoint.

> +{
> +	struct device_node *endpoint_node, *remote;
> +
> +	endpoint_node = of_graph_get_endpoint_by_regs(node, port, endpoint);
> +	if (!endpoint_node) {
> +		pr_debug("no valid endpoint (%d, %d) for node %s\n",
> +			 port, endpoint, node->full_name);
> +		return NULL;
> +	}
> +
> +	remote = of_graph_get_remote_port_parent(endpoint_node);
> +	of_node_put(endpoint);

Vladimir pointed this out already. With that fixed and the missing doc
comment added,
Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

> +	if (!remote) {
> +		pr_debug("no valid remote node\n");
> +		return NULL;
> +	}
> +
> +	if (!of_device_is_available(remote)) {
> +		pr_debug("not available for remote node\n");
> +		return NULL;
> +	}
> +
> +	return remote;
> +}
> +EXPORT_SYMBOL(of_graph_get_remote_node);
> diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
> index bb3a5a2cd570..7b71d3e09209 100644
> --- a/include/linux/of_graph.h
> +++ b/include/linux/of_graph.h
> @@ -51,6 +51,8 @@ struct device_node *of_graph_get_endpoint_by_regs(
>  struct device_node *of_graph_get_remote_port_parent(
>  					const struct device_node *node);
>  struct device_node *of_graph_get_remote_port(const struct device_node *node);
> +struct device_node *of_graph_get_remote_node(const struct device_node *node,
> +					     int port, int endpoint);
>  #else
>  
>  static inline int of_graph_parse_endpoint(const struct device_node *node,
> @@ -89,6 +91,12 @@ static inline struct device_node *of_graph_get_remote_port(
>  {
>  	return NULL;
>  }
> +static inline struct device_node *of_graph_get_remote_node(
> +					const struct device_node *node,
> +					int port, int endpoint)
> +{
> +	return NULL;
> +}
>  
>  #endif /* CONFIG_OF */

regards
Philipp
Rob Herring Feb. 6, 2017, 1:41 p.m. UTC | #3
On Mon, Feb 6, 2017 at 2:50 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Feb 03, 2017 at 09:36:31PM -0600, Rob Herring wrote:
>> The OF graph API leaves too much of the graph walking to clients when
>> in many cases the driver doesn't care about accessing the port or
>> endpoint nodes. The drivers typically just want the device connected via
>> a particular graph connection. of_graph_get_remote_node provides this
>> functionality.
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>
> Just a quick procedural comment: drm-misc for 4.11 is closed already, but
> if we can get this core patch into 4.11 still then that would avoid
> cross-tree sync pains in 4.12 ...

Right, I was thinking the same thing.

Rob
Rob Herring Feb. 6, 2017, 1:54 p.m. UTC | #4
On Mon, Feb 6, 2017 at 4:32 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Rob,
>
> thanks for this clean-up series! I was not aware how far the duplication
> has spread over time.
>
> On Fri, 2017-02-03 at 21:36 -0600, Rob Herring wrote:
>> The OF graph API leaves too much of the graph walking to clients when
>> in many cases the driver doesn't care about accessing the port or
>> endpoint nodes. The drivers typically just want the device connected via
>> a particular graph connection. of_graph_get_remote_node provides this
>> functionality.
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>>  drivers/of/base.c        | 28 ++++++++++++++++++++++++++++
>>  include/linux/of_graph.h |  8 ++++++++
>>  2 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index d4bea3c797d6..ea18ab16b92c 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -2469,3 +2469,31 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node)
>>       return of_get_next_parent(np);
>>  }
>>  EXPORT_SYMBOL(of_graph_get_remote_port);
>> +
>> +struct device_node *of_graph_get_remote_node(const struct device_node *node,
>> +                                          int port, int endpoint)
>
> I think this should have a documentation comment, similar to the
> of_graph_get_endpoint_by_regs one, as it is not really clear from the
> function name that the returned device node is the parent (or
> grandparent) device node containing the remote port to the specified
> node & port & endpoint.
> Also it might be interesting to the user that -1 is a wildcard value for
> port / endpoint.

I really want to not allow using a wildcard here. Drivers should know
what port they want (or iterate over all of them). It didn't look like
any drivers were depending on the wildcard, but were just using -1 for
"no reg property" when really that should 0. Of course, I may have
missed something.

I guess I could enforce port/endpoint > 0 here as there's no existing users.

Rob
Philipp Zabel Feb. 6, 2017, 2:03 p.m. UTC | #5
On Mon, 2017-02-06 at 07:54 -0600, Rob Herring wrote:
> On Mon, Feb 6, 2017 at 4:32 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > Hi Rob,
> >
> > thanks for this clean-up series! I was not aware how far the duplication
> > has spread over time.
> >
> > On Fri, 2017-02-03 at 21:36 -0600, Rob Herring wrote:
> >> The OF graph API leaves too much of the graph walking to clients when
> >> in many cases the driver doesn't care about accessing the port or
> >> endpoint nodes. The drivers typically just want the device connected via
> >> a particular graph connection. of_graph_get_remote_node provides this
> >> functionality.
> >>
> >> Signed-off-by: Rob Herring <robh@kernel.org>
> >> ---
> >>  drivers/of/base.c        | 28 ++++++++++++++++++++++++++++
> >>  include/linux/of_graph.h |  8 ++++++++
> >>  2 files changed, 36 insertions(+)
> >>
> >> diff --git a/drivers/of/base.c b/drivers/of/base.c
> >> index d4bea3c797d6..ea18ab16b92c 100644
> >> --- a/drivers/of/base.c
> >> +++ b/drivers/of/base.c
> >> @@ -2469,3 +2469,31 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node)
> >>       return of_get_next_parent(np);
> >>  }
> >>  EXPORT_SYMBOL(of_graph_get_remote_port);
> >> +
> >> +struct device_node *of_graph_get_remote_node(const struct device_node *node,
> >> +                                          int port, int endpoint)
> >
> > I think this should have a documentation comment, similar to the
> > of_graph_get_endpoint_by_regs one, as it is not really clear from the
> > function name that the returned device node is the parent (or
> > grandparent) device node containing the remote port to the specified
> > node & port & endpoint.
> > Also it might be interesting to the user that -1 is a wildcard value for
> > port / endpoint.
> 
> I really want to not allow using a wildcard here. Drivers should know
> what port they want (or iterate over all of them). It didn't look like
> any drivers were depending on the wildcard, but were just using -1 for
> "no reg property" when really that should 0. Of course, I may have
> missed something.
> 
> I guess I could enforce port/endpoint > 0 here as there's no existing users.

That sounds reasonable. If it works for all users, enforcing >= 0 should
be fine, but in that case I'd change the parameters to be unsigned.

regards
Philipp
diff mbox

Patch

diff --git a/drivers/of/base.c b/drivers/of/base.c
index d4bea3c797d6..ea18ab16b92c 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2469,3 +2469,31 @@  struct device_node *of_graph_get_remote_port(const struct device_node *node)
 	return of_get_next_parent(np);
 }
 EXPORT_SYMBOL(of_graph_get_remote_port);
+
+struct device_node *of_graph_get_remote_node(const struct device_node *node,
+					     int port, int endpoint)
+{
+	struct device_node *endpoint_node, *remote;
+
+	endpoint_node = of_graph_get_endpoint_by_regs(node, port, endpoint);
+	if (!endpoint_node) {
+		pr_debug("no valid endpoint (%d, %d) for node %s\n",
+			 port, endpoint, node->full_name);
+		return NULL;
+	}
+
+	remote = of_graph_get_remote_port_parent(endpoint_node);
+	of_node_put(endpoint);
+	if (!remote) {
+		pr_debug("no valid remote node\n");
+		return NULL;
+	}
+
+	if (!of_device_is_available(remote)) {
+		pr_debug("not available for remote node\n");
+		return NULL;
+	}
+
+	return remote;
+}
+EXPORT_SYMBOL(of_graph_get_remote_node);
diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
index bb3a5a2cd570..7b71d3e09209 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -51,6 +51,8 @@  struct device_node *of_graph_get_endpoint_by_regs(
 struct device_node *of_graph_get_remote_port_parent(
 					const struct device_node *node);
 struct device_node *of_graph_get_remote_port(const struct device_node *node);
+struct device_node *of_graph_get_remote_node(const struct device_node *node,
+					     int port, int endpoint);
 #else
 
 static inline int of_graph_parse_endpoint(const struct device_node *node,
@@ -89,6 +91,12 @@  static inline struct device_node *of_graph_get_remote_port(
 {
 	return NULL;
 }
+static inline struct device_node *of_graph_get_remote_node(
+					const struct device_node *node,
+					int port, int endpoint)
+{
+	return NULL;
+}
 
 #endif /* CONFIG_OF */