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

[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@xxxxxxxxxx>
> > Reviewed-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
> > Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> > ---
> >  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 */

-- 
Regards,

Laurent Pinchart