diff mbox series

[v7,1/6] of_graph: add of_graph_get_local_port()

Message ID 20200614172234.8856-2-digetx@gmail.com
State Not Applicable
Headers show
Series Support DRM bridges on NVIDIA Tegra | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Dmitry Osipenko June 14, 2020, 5:22 p.m. UTC
In some case, like a DRM display code for example, it's useful to silently
check whether port node exists at all in a device-tree before proceeding
with parsing the graph.

This patch adds of_graph_get_local_port() which returns pointer to a local
port node, or NULL if graph isn't specified in a device-tree for a given
device node.

Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/of/property.c    | 32 +++++++++++++++++++++++---------
 include/linux/of_graph.h |  7 +++++++
 2 files changed, 30 insertions(+), 9 deletions(-)

Comments

Laurent Pinchart June 16, 2020, 1:21 a.m. UTC | #1
Hi Dmitry,

Thank you for the patch.

On Sun, Jun 14, 2020 at 08:22:29PM +0300, Dmitry Osipenko wrote:
> In some case, like a DRM display code for example, it's useful to silently
> check whether port node exists at all in a device-tree before proceeding
> with parsing the graph.
> 
> This patch adds of_graph_get_local_port() which returns pointer to a local
> port node, or NULL if graph isn't specified in a device-tree for a given
> device node.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/of/property.c    | 32 +++++++++++++++++++++++---------
>  include/linux/of_graph.h |  7 +++++++
>  2 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 1f2086f4e7ce..05c5f619b8bb 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -608,15 +608,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_local_port(parent);
>  		if (!port) {
>  			pr_err("graph: no port node found in %pOF\n", parent);
>  			return NULL;
> @@ -765,6 +757,28 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node)
>  }
>  EXPORT_SYMBOL(of_graph_get_remote_port);
>  
> +/**
> + * of_graph_get_local_port() - get local port node
> + * @node: pointer to a local endpoint device_node
> + *
> + * Return: First local port node associated with local endpoint node linked
> + *	   to @node. Use of_node_put() on it when done.
> + */
> +struct device_node *of_graph_get_local_port(const struct device_node *node)
> +{
> +	struct device_node *ports, *port;
> +
> +	ports = of_get_child_by_name(node, "ports");
> +	if (ports)
> +		node = ports;
> +
> +	port = of_get_child_by_name(node, "port");
> +	of_node_put(ports);
> +
> +	return port;

The implementation doesn't seem to match the documentation. If node is a
pointer to an endpoint, it should not have any ports child.

> +}
> +EXPORT_SYMBOL(of_graph_get_local_port);
> +
>  int of_graph_get_endpoint_count(const struct device_node *np)
>  {
>  	struct device_node *endpoint;
> diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
> index 01038a6aade0..1fdeb72c7765 100644
> --- a/include/linux/of_graph.h
> +++ b/include/linux/of_graph.h
> @@ -54,6 +54,7 @@ struct device_node *of_graph_get_remote_port_parent(
>  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,
>  					     u32 port, u32 endpoint);
> +struct device_node *of_graph_get_local_port(const struct device_node *node);
>  #else
>  
>  static inline int of_graph_parse_endpoint(const struct device_node *node,
> @@ -116,6 +117,12 @@ static inline struct device_node *of_graph_get_remote_node(
>  	return NULL;
>  }
>  
> +static inline struct device_node *of_graph_get_local_port(
> +					const struct device_node *node)
> +{
> +	return NULL;
> +}
> +
>  #endif /* CONFIG_OF */
>  
>  #endif /* __LINUX_OF_GRAPH_H */
> -- 
> 2.26.0
>
Laurent Pinchart June 16, 2020, 1:25 a.m. UTC | #2
On Tue, Jun 16, 2020 at 04:21:12AM +0300, Laurent Pinchart wrote:
> Hi Dmitry,
> 
> Thank you for the patch.
> 
> On Sun, Jun 14, 2020 at 08:22:29PM +0300, Dmitry Osipenko wrote:
> > In some case, like a DRM display code for example, it's useful to silently
> > check whether port node exists at all in a device-tree before proceeding
> > with parsing the graph.
> > 
> > This patch adds of_graph_get_local_port() which returns pointer to a local
> > port node, or NULL if graph isn't specified in a device-tree for a given
> > device node.
> > 
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > ---
> >  drivers/of/property.c    | 32 +++++++++++++++++++++++---------
> >  include/linux/of_graph.h |  7 +++++++
> >  2 files changed, 30 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 1f2086f4e7ce..05c5f619b8bb 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -608,15 +608,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_local_port(parent);
> >  		if (!port) {
> >  			pr_err("graph: no port node found in %pOF\n", parent);
> >  			return NULL;
> > @@ -765,6 +757,28 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node)
> >  }
> >  EXPORT_SYMBOL(of_graph_get_remote_port);
> >  
> > +/**
> > + * of_graph_get_local_port() - get local port node
> > + * @node: pointer to a local endpoint device_node
> > + *
> > + * Return: First local port node associated with local endpoint node linked
> > + *	   to @node. Use of_node_put() on it when done.
> > + */
> > +struct device_node *of_graph_get_local_port(const struct device_node *node)

I forgot to mention that, given that there could be multiple 'port'
nodes, this function would be better named
of_graph_get_first_local_port(). 'first' here would refer to the nodes
order in the device tree, which I believe may not match the port number.
For instance, in the following case

	ports {
		#address-cells = <1>;
		#size-cells = <1>;
		port@1 {
			reg = <1>;
		};
		port@0 {
			reg = <0>;
		};
	};

the function would I believe return port@1. It may be a good idea to
explain this in the documentation. Depending on how you use this
function, if your only use case is to test for the presence of port
nodes, it may be best to return a bool and name it of_graph_has_port()
or something similar.

> > +{
> > +	struct device_node *ports, *port;
> > +
> > +	ports = of_get_child_by_name(node, "ports");
> > +	if (ports)
> > +		node = ports;
> > +
> > +	port = of_get_child_by_name(node, "port");
> > +	of_node_put(ports);
> > +
> > +	return port;
> 
> The implementation doesn't seem to match the documentation. If node is a
> pointer to an endpoint, it should not have any ports child.
> 
> > +}
> > +EXPORT_SYMBOL(of_graph_get_local_port);
> > +
> >  int of_graph_get_endpoint_count(const struct device_node *np)
> >  {
> >  	struct device_node *endpoint;
> > diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
> > index 01038a6aade0..1fdeb72c7765 100644
> > --- a/include/linux/of_graph.h
> > +++ b/include/linux/of_graph.h
> > @@ -54,6 +54,7 @@ struct device_node *of_graph_get_remote_port_parent(
> >  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,
> >  					     u32 port, u32 endpoint);
> > +struct device_node *of_graph_get_local_port(const struct device_node *node);
> >  #else
> >  
> >  static inline int of_graph_parse_endpoint(const struct device_node *node,
> > @@ -116,6 +117,12 @@ static inline struct device_node *of_graph_get_remote_node(
> >  	return NULL;
> >  }
> >  
> > +static inline struct device_node *of_graph_get_local_port(
> > +					const struct device_node *node)
> > +{
> > +	return NULL;
> > +}
> > +
> >  #endif /* CONFIG_OF */
> >  
> >  #endif /* __LINUX_OF_GRAPH_H */
Dmitry Osipenko June 16, 2020, 1:56 p.m. UTC | #3
16.06.2020 04:25, Laurent Pinchart пишет:
> On Tue, Jun 16, 2020 at 04:21:12AM +0300, Laurent Pinchart wrote:
>> Hi Dmitry,
>>
>> Thank you for the patch.
>>
>> On Sun, Jun 14, 2020 at 08:22:29PM +0300, Dmitry Osipenko wrote:
>>> In some case, like a DRM display code for example, it's useful to silently
>>> check whether port node exists at all in a device-tree before proceeding
>>> with parsing the graph.
>>>
>>> This patch adds of_graph_get_local_port() which returns pointer to a local
>>> port node, or NULL if graph isn't specified in a device-tree for a given
>>> device node.
>>>
>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/of/property.c    | 32 +++++++++++++++++++++++---------
>>>  include/linux/of_graph.h |  7 +++++++
>>>  2 files changed, 30 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/of/property.c b/drivers/of/property.c
>>> index 1f2086f4e7ce..05c5f619b8bb 100644
>>> --- a/drivers/of/property.c
>>> +++ b/drivers/of/property.c
>>> @@ -608,15 +608,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_local_port(parent);
>>>  		if (!port) {
>>>  			pr_err("graph: no port node found in %pOF\n", parent);
>>>  			return NULL;
>>> @@ -765,6 +757,28 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node)
>>>  }
>>>  EXPORT_SYMBOL(of_graph_get_remote_port);
>>>  
>>> +/**
>>> + * of_graph_get_local_port() - get local port node
>>> + * @node: pointer to a local endpoint device_node
>>> + *
>>> + * Return: First local port node associated with local endpoint node linked
>>> + *	   to @node. Use of_node_put() on it when done.
>>> + */
>>> +struct device_node *of_graph_get_local_port(const struct device_node *node)
> 
> I forgot to mention that, given that there could be multiple 'port'
> nodes, this function would be better named
> of_graph_get_first_local_port(). 'first' here would refer to the nodes
> order in the device tree, which I believe may not match the port number.
> For instance, in the following case
> 
> 	ports {
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		port@1 {
> 			reg = <1>;
> 		};
> 		port@0 {
> 			reg = <0>;
> 		};
> 	};
> 
> the function would I believe return port@1. It may be a good idea to
> explain this in the documentation.

Hello Laurent,

It's correct that the port@1 will be returned in yours example.

I'll improve the doc and the function's name in the next revision, thank
you for the suggestions!

> Depending on how you use this
> function, if your only use case is to test for the presence of port
> nodes, it may be best to return a bool and name it of_graph_has_port()
> or something similar.
> 
>>> +{
>>> +	struct device_node *ports, *port;
>>> +
>>> +	ports = of_get_child_by_name(node, "ports");
>>> +	if (ports)
>>> +		node = ports;
>>> +
>>> +	port = of_get_child_by_name(node, "port");
>>> +	of_node_put(ports);
>>> +
>>> +	return port;
>>
>> The implementation doesn't seem to match the documentation. If node is a
>> pointer to an endpoint, it should not have any ports child.

Right, I'll reword the doc in v8. This function doesn't differentiate
between start / end points. It's up to a user of this function to check
whether node is endpoint or something else if needed.

Thank you very much for the comments!
Dmitry Osipenko June 16, 2020, 2:14 p.m. UTC | #4
16.06.2020 16:56, Dmitry Osipenko пишет:
> 16.06.2020 04:25, Laurent Pinchart пишет:
>> On Tue, Jun 16, 2020 at 04:21:12AM +0300, Laurent Pinchart wrote:
>>> Hi Dmitry,
>>>
>>> Thank you for the patch.
>>>
>>> On Sun, Jun 14, 2020 at 08:22:29PM +0300, Dmitry Osipenko wrote:
>>>> In some case, like a DRM display code for example, it's useful to silently
>>>> check whether port node exists at all in a device-tree before proceeding
>>>> with parsing the graph.
>>>>
>>>> This patch adds of_graph_get_local_port() which returns pointer to a local
>>>> port node, or NULL if graph isn't specified in a device-tree for a given
>>>> device node.
>>>>
>>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  drivers/of/property.c    | 32 +++++++++++++++++++++++---------
>>>>  include/linux/of_graph.h |  7 +++++++
>>>>  2 files changed, 30 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/of/property.c b/drivers/of/property.c
>>>> index 1f2086f4e7ce..05c5f619b8bb 100644
>>>> --- a/drivers/of/property.c
>>>> +++ b/drivers/of/property.c
>>>> @@ -608,15 +608,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_local_port(parent);
>>>>  		if (!port) {
>>>>  			pr_err("graph: no port node found in %pOF\n", parent);
>>>>  			return NULL;
>>>> @@ -765,6 +757,28 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node)
>>>>  }
>>>>  EXPORT_SYMBOL(of_graph_get_remote_port);
>>>>  
>>>> +/**
>>>> + * of_graph_get_local_port() - get local port node
>>>> + * @node: pointer to a local endpoint device_node
>>>> + *
>>>> + * Return: First local port node associated with local endpoint node linked
>>>> + *	   to @node. Use of_node_put() on it when done.
>>>> + */
>>>> +struct device_node *of_graph_get_local_port(const struct device_node *node)
>>
>> I forgot to mention that, given that there could be multiple 'port'
>> nodes, this function would be better named
>> of_graph_get_first_local_port(). 'first' here would refer to the nodes
>> order in the device tree, which I believe may not match the port number.
>> For instance, in the following case
>>
>> 	ports {
>> 		#address-cells = <1>;
>> 		#size-cells = <1>;
>> 		port@1 {
>> 			reg = <1>;
>> 		};
>> 		port@0 {
>> 			reg = <0>;
>> 		};
>> 	};
>>
>> the function would I believe return port@1. It may be a good idea to
>> explain this in the documentation.
> 
> Hello Laurent,
> 
> It's correct that the port@1 will be returned in yours example.
> 
> I'll improve the doc and the function's name in the next revision, thank
> you for the suggestions!
> 
>> Depending on how you use this
>> function, if your only use case is to test for the presence of port
>> nodes, it may be best to return a bool and name it of_graph_has_port()
>> or something similar.
>>
>>>> +{
>>>> +	struct device_node *ports, *port;
>>>> +
>>>> +	ports = of_get_child_by_name(node, "ports");
>>>> +	if (ports)
>>>> +		node = ports;
>>>> +
>>>> +	port = of_get_child_by_name(node, "port");
>>>> +	of_node_put(ports);
>>>> +
>>>> +	return port;
>>>
>>> The implementation doesn't seem to match the documentation. If node is a
>>> pointer to an endpoint, it should not have any ports child.
> 
> Right, I'll reword the doc in v8. This function doesn't differentiate
> between start / end points. It's up to a user of this function to check
> whether node is endpoint or something else if needed.
> 
> Thank you very much for the comments!
> 

Although, I re-read the doc comment and compared it to the wording of
the other of_graph_*() functions and it's already good as-is to me
because the doc explicitly says the "Return: First local port associated
with local endpoint...", which is exactly what this function does.

But still the function name and it's brief description could be improved.
diff mbox series

Patch

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 1f2086f4e7ce..05c5f619b8bb 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -608,15 +608,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_local_port(parent);
 		if (!port) {
 			pr_err("graph: no port node found in %pOF\n", parent);
 			return NULL;
@@ -765,6 +757,28 @@  struct device_node *of_graph_get_remote_port(const struct device_node *node)
 }
 EXPORT_SYMBOL(of_graph_get_remote_port);
 
+/**
+ * of_graph_get_local_port() - get local port node
+ * @node: pointer to a local endpoint device_node
+ *
+ * Return: First local port node associated with local endpoint node linked
+ *	   to @node. Use of_node_put() on it when done.
+ */
+struct device_node *of_graph_get_local_port(const struct device_node *node)
+{
+	struct device_node *ports, *port;
+
+	ports = of_get_child_by_name(node, "ports");
+	if (ports)
+		node = ports;
+
+	port = of_get_child_by_name(node, "port");
+	of_node_put(ports);
+
+	return port;
+}
+EXPORT_SYMBOL(of_graph_get_local_port);
+
 int of_graph_get_endpoint_count(const struct device_node *np)
 {
 	struct device_node *endpoint;
diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
index 01038a6aade0..1fdeb72c7765 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -54,6 +54,7 @@  struct device_node *of_graph_get_remote_port_parent(
 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,
 					     u32 port, u32 endpoint);
+struct device_node *of_graph_get_local_port(const struct device_node *node);
 #else
 
 static inline int of_graph_parse_endpoint(const struct device_node *node,
@@ -116,6 +117,12 @@  static inline struct device_node *of_graph_get_remote_node(
 	return NULL;
 }
 
+static inline struct device_node *of_graph_get_local_port(
+					const struct device_node *node)
+{
+	return NULL;
+}
+
 #endif /* CONFIG_OF */
 
 #endif /* __LINUX_OF_GRAPH_H */