Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Grant,

Am Mittwoch, den 26.02.2014, 11:37 +0000 schrieb Grant Likely:
[...]
> >  drivers/media/v4l2-core/v4l2-of.c             | 117 ----------------------
> >  drivers/of/Makefile                           |   1 +
> >  drivers/of/of_graph.c                         | 134 ++++++++++++++++++++++++++
> 
> Nah. Just put it into drivers/of/base.c. This isn't a separate subsystem
> and the functions are pretty basic.

Ok.

[...]
> > +struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
> > +					struct device_node *prev)
> > +{
> > +	struct device_node *endpoint;
> > +	struct device_node *port = NULL;
> > +
> > +	if (!parent)
> > +		return NULL;
> > +
> > +	if (!prev) {
> > +		struct device_node *node;
> > +		/*
> > +		 * It's the first call, we have to find a port subnode
> > +		 * within this node or within an optional 'ports' node.
> > +		 */
> > +		node = of_get_child_by_name(parent, "ports");
> > +		if (node)
> > +			parent = node;
> > +
> > +		port = of_get_child_by_name(parent, "port");
> 
> If you've got a "ports" node, then I would expect every single child to
> be a port. Should not need the _by_name variant.

The 'ports' node is optional. It is only needed if the parent node has
its own #address-cells and #size-cells properties. If the ports are
direct children of the device node, there might be other nodes than
ports:

	device {
		#address-cells = <1>;
		#size-cells = <0>;

		port@0 {
			endpoint { ... };
		};
		port@1 {
			endpoint { ... };
		};

		some-other-child { ... };
	};

	device {
		#address-cells = <x>;
		#size-cells = <y>;

		ports {
			#address-cells = <1>;
			#size-cells = <0>;

			port@0 {
				endpoint { ... };
			};
			port@1 {
				endpoint { ... };
			};
		};

		some-other-child { ... };
	};

The helper should find the two endpoints in both cases.
Tomi suggests an even more compact form for devices with just one port:

	device {
		endpoint { ... };

		some-other-child { ... };
	};

> It seems that this function is merely a helper to get all grandchildren
> of a node (with some very minor constraints). That could be generalized
> and simplified. If the function takes the "ports" node as an argument
> instead of the parent, then there is a greater likelyhood that other
> code can make use of it...
> 
> Thinking further. I think the semantics of this whole feature basically
> boil down to this:
> 
> #define for_each_grandchild_of_node(parent, child, grandchild) \
> 	for_each_child_of_node(parent, child) \
> 		for_each_child_of_node(child, grandchild)
> 
> Correct? Or in this specific case:
> 
> 	parent = of_get_child_by_name(np, "ports")
> 	for_each_grandchild_of_node(parent, child, grandchild) {
> 		...
> 	}

Hmm, that would indeed be a bit more generic, but it doesn't handle the
optional 'ports' subnode and doesn't allow for other child nodes in the
device node.

> Finally, looking at the actual patch, is any of this actually needed.
> All of the users updated by this patch only ever handle a single
> endpoint. Have I read it correctly? Are there any users supporting
> multiple endpoints?

Yes, mainline currently only contains simple cases. I have posted i.MX6
patches that use this scheme for the output path:
  http://www.spinics.net/lists/arm-kernel/msg310817.html
  http://www.spinics.net/lists/arm-kernel/msg310821.html

> > +
> > +		if (port) {
> > +			/* Found a port, get an endpoint. */
> > +			endpoint = of_get_next_child(port, NULL);
> > +			of_node_put(port);
> > +		} else {
> > +			endpoint = NULL;
> > +		}
> > +
> > +		if (!endpoint)
> > +			pr_err("%s(): no endpoint nodes specified for %s\n",
> > +			       __func__, parent->full_name);
> > +		of_node_put(node);
> 
> If you 'return endpoint' here, then the else block can go down a level.

Note that this patch is a straight move of existing code.
I can follow up with code beautification and ...

> > +	} else {
> > +		port = of_get_parent(prev);
> > +		if (!port)
> > +			/* Hm, has someone given us the root node ?... */
> > +			return NULL;
> 
> WARN_ONCE(). That's a very definite coding failure if that happens.

... with a fix for this.

> > +
> > +		/* Avoid dropping prev node refcount to 0. */
> > +		of_node_get(prev);
> > +		endpoint = of_get_next_child(port, prev);
> > +		if (endpoint) {
> > +			of_node_put(port);
> > +			return endpoint;
> > +		}
> > +
> > +		/* No more endpoints under this port, try the next one. */
> > +		do {
> > +			port = of_get_next_child(parent, port);
> > +			if (!port)
> > +				return NULL;
> > +		} while (of_node_cmp(port->name, "port"));
> > +
> > +		/* Pick up the first endpoint in this port. */
> > +		endpoint = of_get_next_child(port, NULL);
> > +		of_node_put(port);
> > +	}
> > +
> > +	return endpoint;
> > +}
> > +EXPORT_SYMBOL(of_graph_get_next_endpoint);
> > +
> > +/**
> > + * of_graph_get_remote_port_parent() - get remote port's parent node
> > + * @node: pointer to a local endpoint device_node
> > + *
> > + * Return: Remote device node associated with remote endpoint node linked
> > + *	   to @node. Use of_node_put() on it when done.
> > + */
> > +struct device_node *of_graph_get_remote_port_parent(
> > +			       const struct device_node *node)
> > +{
> > +	struct device_node *np;
> > +	unsigned int depth;
> > +
> > +	/* Get remote endpoint node. */
> > +	np = of_parse_phandle(node, "remote-endpoint", 0);
> > +
> > +	/* Walk 3 levels up only if there is 'ports' node. */
> 
> This needs a some explaining. My reading of the binding pattern is that
> it will always be a fixed number of levels. Why is this test fuzzy?
[...]

See above. The ports subnode level is optional. In most cases, the port
nodes will be direct children of the device node.
Walking up 3 levels from the endpoint node will return the device if
there was a ports node. If there is no ports node, we only have to walk
up two levels.

regards
Philipp