Message-Id: <20140226110114.CF2C7C40A89@trevor.secretlab.ca>
Date:	Wed, 26 Feb 2014 11:01:14 +0000
From:	Grant Likely <grant.likely@...aro.org>
To:	Philipp Zabel <p.zabel@...gutronix.de>
Cc:	Sascha Hauer <s.hauer@...gutronix.de>,
	Rob Herring <robherring2@...il.com>,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	Mauro Carvalho Chehab <m.chehab@...sung.com>,
	Rob Herring <robh+dt@...nel.org>,
	Sylwester Nawrocki <s.nawrocki@...sung.com>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Tomi Valkeinen <tomi.valkeinen@...com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Philipp Zabel <philipp.zabel@...il.com>
Subject: Re: [RFC PATCH] [media]: of: move graph helpers from drivers/media/v4l2-core to drivers/of

On Mon, 24 Feb 2014 18:36:29 +0100, Philipp Zabel <p.zabel@...gutronix.de> wrote:
> Am Dienstag, den 18.02.2014, 16:26 +0000 schrieb Grant Likely:
> > > 
> > > You can find it under Documentation/devicetree/bindings/media/video-interfaces.txt
> > 
> > Okay, I think I'm okay with moving the helpers, but I will make one
> > requirement. I would like to have a short binding document describing
> > the pattern being used. The document above talks a lot about video
> > specific issues, but the helpers appear to be specifically about the
> > tree walking properties of the API.
> 
> Reusing the non-video-secific parts of video-interfaces.txt, how about
> the following:

This is good, but I have some comments. This document describes itself
as the common way for doing a device graph within the device tree, but
there is already a well established pattern for device graphs that is
used by the interrupts-extended, clocks and other bindings. Those are
all domain-specific bindings, but the core concept is one device uses
a resource provided by another device. The resource references construct
a graph independent from the natural FDT node graph. (ie. the interrupts
binding forms the interrupt graph. Same for the clock binding).

So, while this binding does describe a pattern for separate device
graphs, it is by no means the only common way of doing so.

I would like the document to acknowledge the difference from the
phandle+args pattern used elsewhere and a description of when it would
be appropriate to use this instead of a simpler binding.

> 
> "Common bindings for device graphs"
> 
> General concept
> ---------------
> 
> The hierarchical organisation of the device tree is well suited to describe
> control flow to devices, but data flow between devices that work together to
> form a logical compound device can follow arbitrarily complex graphs.

I would argue that this pattern isn't necessarily restricted to data
flow descriptions. It wants to describe linkage between devices that are
sufficiently complex that the simple binding doesn't do the job.

> The device tree graph bindings allow to describe data bus connections between
> individual devices, that can't be inferred from device tree parent-child
> relationships. The common bindings do not contain any information about the
> direction or type of data flow, they just map connections. Specific properties
> of the connections can be set depending on the type of connection. To see
> how this binding applies to video pipelines, see for example
> Documentation/device-tree/bindings/media/video-interfaces.txt.

Even if you don't want to declare the direction of data flow, there does
need to be some guidance as to how the binding is constructed. Does
device A point to device B? Or the other way around? Why would someone
choose one over the other? I don't want to see a situation where A & B
point to each other. Things get complex if the graph is allowed to be
cyclical.

> Data interfaces on devices are described by their child 'port' nodes. The port
> node contains an 'endpoint' subnode for each remote device connected to this
> port via a bus. If a port is connected to more than one remote device on the
> same bus, an 'endpoint' child node must be provided for each of them. If more
> than one port is present in a device node or there is more than one endpoint at
> a port, or port node needs to be associated with a selected hardware interface,
> a common scheme using '#address-cells', '#size-cells' and 'reg' properties is
> used.
> 
> device {
> 	...
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 
> 	port@0 {
> 		...
> 		endpoint@0 { ... };
> 		endpoint@1 { ... };
> 	};
> 
> 	port@1 { ... };
> };
> 
> All 'port' nodes can be grouped under optional 'ports' node, which allows to
> specify #address-cells, #size-cells properties independently for the 'port'
> and 'endpoint' nodes and any child device nodes a device might have.
> 
> device {
> 	...
> 	ports {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		port@0 {
> 			...
> 			endpoint@0 { ... };
> 			endpoint@1 { ... };
> 		};
> 
> 		port@1 { ... };
> 	};
> };
> 
> Each endpoint can contain a 'remote-endpoint' phandle property that points to
> the corresponding endpoint in the port of the remote device. Two 'endpoint'
> nodes are linked with each other through their 'remote-endpoint' phandles.

I really don't like this aspect. It is far too easy to get wrong. Graphs
should be one direction only.

> 
> device_1 {
> 	port {
> 		device_1_output: endpoint {
> 			remote-endpoint = <&device_2_input>;
> 		};
> 	};
> };
> 
> device_1 {
> 	port {
> 		device_2_input: endpoint {
> 			remote-endpoint = <&device_1_output>;
> 		};
> 	};
> };
> 
> 
> Required properties
> -------------------
> 
> If there is more than one 'port' or more than one 'endpoint' node or 'reg'
> property is present in port and/or endpoint nodes the following properties
> are required in a relevant parent node:
> 
>  - #address-cells : number of cells required to define port/endpoint
> 		    identifier, should be 1.
>  - #size-cells    : should be zero.
> 
> Optional endpoint properties
> ----------------------------
> 
> - remote-endpoint: phandle to an 'endpoint' subnode of a remote device node.
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists