Re: [PATCH 04/14] media: add V4L2 DT binding documentation

Re: [PATCH 04/14] media: add V4L2 DT binding documentation

[Guennadi Liakhovetski]

On Wed, 3 Oct 2012, Rob Herring wrote:

> On 10/02/2012 09:33 AM, Guennadi Liakhovetski wrote:
> > Hi Rob
> > 
> > On Tue, 2 Oct 2012, Rob Herring wrote:
> > 
> >> On 09/27/2012 09:07 AM, Guennadi Liakhovetski wrote:
> >>> This patch adds a document, describing common V4L2 device tree bindings.
> >>>
> >>> Co-authored-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> >>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >>> ---
> >>>  Documentation/devicetree/bindings/media/v4l2.txt |  162 ++++++++++++++++++++++
> >>>  1 files changed, 162 insertions(+), 0 deletions(-)
> >>>  create mode 100644 Documentation/devicetree/bindings/media/v4l2.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/media/v4l2.txt b/Documentation/devicetree/bindings/media/v4l2.txt
> >>> new file mode 100644
> >>> index 0000000..b8b3f41
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/media/v4l2.txt
> >>> @@ -0,0 +1,162 @@
> >>> +Video4Linux Version 2 (V4L2)
> >>
> >> DT describes the h/w, but V4L2 is Linux specific. I think the binding
> >> looks pretty good in terms of it is describing the h/w and not V4L2
> >> components or settings. So in this case it's really just the name of the
> >> file and title I have issue with.
> > 
> > Hm, I see your point, then, I guess, you'd also like the file name 
> > changed. What should we use then? Just "video?" But there's already a 
> > whole directory Documentation/devicetree/bindings/video dedicated to 
> > graphics output (drm, fbdev). "video-camera" or "video-capture?" But this 
> > file shall also be describing video output. Use "video.txt" and describe 
> > inside what exactly this file is for?
> 
> Video output will probably have a lot of overlap with the graphics side.
> How about video-interfaces.txt?

Hm, that's a bit too vague for me. Somewhere on the outskirts of my mind 
I'm still considering making just one standard for both V4L2 and fbdev / 
DRM? Just yesterday we were discussing some common properties with what is 
being proposed in

http://www.mail-archive.com/linux-media@vger.kernel.org/index.html#53322

Still, I think, these two subsystems deserve two separate standards and 
should just try to re-use properties wherever that makes sense. 
video-stream seems a bit better, but this too is just a convention - 
talking about video cameras and TV output as video streaming devices and 
considering displays more static devices. In principle displays can be 
considered taking streaming data just as well as TV encoders. What if we 
just call this camera-tv.txt?

> >> One other comment below:
> >>
> >>> +
> >>> +General concept
> >>> +---------------
> >>> +
> >>> +Video pipelines consist of external devices, e.g. camera sensors, controlled
> >>> +over an I2C, SPI or UART bus, and SoC internal IP blocks, including video DMA
> >>> +engines and video data processors.
> >>> +
> >>> +SoC internal blocks are described by DT nodes, placed similarly to other SoC
> >>> +blocks. External devices are represented as child nodes of their respective bus
> >>> +controller nodes, e.g. I2C.
> >>> +
> >>> +Data interfaces on all video devices are described by "port" child DT nodes.
> >>> +Configuration of a port depends on other devices participating in the data
> >>> +transfer and is described by "link" DT nodes, specified as children of the
> >>> +"port" nodes:
> >>> +
> >>> +/foo {
> >>> +	port@0 {
> >>> +		link@0 { ... };
> >>> +		link@1 { ... };
> >>> +	};
> >>> +	port@1 { ... };
> >>> +};
> >>> +
> >>> +If a port can be configured to work with more than one other device on the same
> >>> +bus, a "link" child DT node must be provided for each of them. If more than one
> >>> +port is present on a device or more than one link is connected to a port, a
> >>> +common scheme, using "#address-cells," "#size-cells" and "reg" properties is
> >>> +used.
> >>> +
> >>> +Optional link properties:
> >>> +- remote: phandle to the other endpoint link DT node.
> >>
> >> This name is a little vague. Perhaps "endpoint" would be better.
> > 
> > "endpoint" can also refer to something local like in USB case. Maybe 
> > rather the description of the "remote" property should be improved?
> 
> remote-endpoint?

Sorry, I really don't want to pull in yet another term here. We've got 
ports and links already, now you're proposing to also use "endpoind." 
Until now everyone was happy with "remote," any more opinions on this?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 04/14] media: add V4L2 DT binding documentation

[Hans Verkuil]

On Fri October 5 2012 11:43:27 Guennadi Liakhovetski wrote:
> On Wed, 3 Oct 2012, Rob Herring wrote:
> 
> > On 10/02/2012 09:33 AM, Guennadi Liakhovetski wrote:
> > > Hi Rob
> > > 
> > > On Tue, 2 Oct 2012, Rob Herring wrote:
> > > 
> > >> On 09/27/2012 09:07 AM, Guennadi Liakhovetski wrote:
> > >>> This patch adds a document, describing common V4L2 device tree bindings.
> > >>>
> > >>> Co-authored-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> > >>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > >>> ---
> > >>>  Documentation/devicetree/bindings/media/v4l2.txt |  162 ++++++++++++++++++++++
> > >>>  1 files changed, 162 insertions(+), 0 deletions(-)
> > >>>  create mode 100644 Documentation/devicetree/bindings/media/v4l2.txt
> > >>>
> > >>> diff --git a/Documentation/devicetree/bindings/media/v4l2.txt b/Documentation/devicetree/bindings/media/v4l2.txt
> > >>> new file mode 100644
> > >>> index 0000000..b8b3f41
> > >>> --- /dev/null
> > >>> +++ b/Documentation/devicetree/bindings/media/v4l2.txt
> > >>> @@ -0,0 +1,162 @@
> > >>> +Video4Linux Version 2 (V4L2)
> > >>
> > >> DT describes the h/w, but V4L2 is Linux specific. I think the binding
> > >> looks pretty good in terms of it is describing the h/w and not V4L2
> > >> components or settings. So in this case it's really just the name of the
> > >> file and title I have issue with.
> > > 
> > > Hm, I see your point, then, I guess, you'd also like the file name 
> > > changed. What should we use then? Just "video?" But there's already a 
> > > whole directory Documentation/devicetree/bindings/video dedicated to 
> > > graphics output (drm, fbdev). "video-camera" or "video-capture?" But this 
> > > file shall also be describing video output. Use "video.txt" and describe 
> > > inside what exactly this file is for?
> > 
> > Video output will probably have a lot of overlap with the graphics side.
> > How about video-interfaces.txt?
> 
> Hm, that's a bit too vague for me. Somewhere on the outskirts of my mind 
> I'm still considering making just one standard for both V4L2 and fbdev / 
> DRM? Just yesterday we were discussing some common properties with what is 
> being proposed in
> 
> http://www.mail-archive.com/linux-media@vger.kernel.org/index.html#53322
> 
> Still, I think, these two subsystems deserve two separate standards and 
> should just try to re-use properties wherever that makes sense. 
> video-stream seems a bit better, but this too is just a convention - 
> talking about video cameras and TV output as video streaming devices and 
> considering displays more static devices. In principle displays can be 
> considered taking streaming data just as well as TV encoders. What if we 
> just call this camera-tv.txt?
> 
> > >> One other comment below:
> > >>
> > >>> +
> > >>> +General concept
> > >>> +---------------
> > >>> +
> > >>> +Video pipelines consist of external devices, e.g. camera sensors, controlled
> > >>> +over an I2C, SPI or UART bus, and SoC internal IP blocks, including video DMA
> > >>> +engines and video data processors.
> > >>> +
> > >>> +SoC internal blocks are described by DT nodes, placed similarly to other SoC
> > >>> +blocks. External devices are represented as child nodes of their respective bus
> > >>> +controller nodes, e.g. I2C.
> > >>> +
> > >>> +Data interfaces on all video devices are described by "port" child DT nodes.
> > >>> +Configuration of a port depends on other devices participating in the data
> > >>> +transfer and is described by "link" DT nodes, specified as children of the
> > >>> +"port" nodes:
> > >>> +
> > >>> +/foo {
> > >>> +	port@0 {
> > >>> +		link@0 { ... };
> > >>> +		link@1 { ... };
> > >>> +	};
> > >>> +	port@1 { ... };
> > >>> +};
> > >>> +
> > >>> +If a port can be configured to work with more than one other device on the same
> > >>> +bus, a "link" child DT node must be provided for each of them. If more than one
> > >>> +port is present on a device or more than one link is connected to a port, a
> > >>> +common scheme, using "#address-cells," "#size-cells" and "reg" properties is
> > >>> +used.
> > >>> +
> > >>> +Optional link properties:
> > >>> +- remote: phandle to the other endpoint link DT node.
> > >>
> > >> This name is a little vague. Perhaps "endpoint" would be better.
> > > 
> > > "endpoint" can also refer to something local like in USB case. Maybe 
> > > rather the description of the "remote" property should be improved?
> > 
> > remote-endpoint?
> 
> Sorry, I really don't want to pull in yet another term here. We've got 
> ports and links already, now you're proposing to also use "endpoind." 
> Until now everyone was happy with "remote," any more opinions on this?

Actually, when I was reviewing the patch series today I got confused as
well by 'remote'. What about 'remote-link'?

And v4l2_of_get_remote() can be renamed to v4l2_of_get_remote_link() which
I think is a lot clearer.

The text can be improved as well since this:

- remote: phandle to the other endpoint link DT node.

is a bit vague. How about:

- remote-link: phandle to the remote end of this link.

Regards,

	Hans

Re: [PATCH 04/14] media: add V4L2 DT binding documentation

[Guennadi Liakhovetski]

On Fri, 5 Oct 2012, Hans Verkuil wrote:

> On Fri October 5 2012 11:43:27 Guennadi Liakhovetski wrote:
> > On Wed, 3 Oct 2012, Rob Herring wrote:
> > 
> > > On 10/02/2012 09:33 AM, Guennadi Liakhovetski wrote:
> > > > Hi Rob
> > > > 
> > > > On Tue, 2 Oct 2012, Rob Herring wrote:
> > > > 
> > > >> On 09/27/2012 09:07 AM, Guennadi Liakhovetski wrote:

[snip]

> > > >>> +Optional link properties:
> > > >>> +- remote: phandle to the other endpoint link DT node.
> > > >>
> > > >> This name is a little vague. Perhaps "endpoint" would be better.
> > > > 
> > > > "endpoint" can also refer to something local like in USB case. Maybe 
> > > > rather the description of the "remote" property should be improved?
> > > 
> > > remote-endpoint?
> > 
> > Sorry, I really don't want to pull in yet another term here. We've got 
> > ports and links already, now you're proposing to also use "endpoind." 
> > Until now everyone was happy with "remote," any more opinions on this?
> 
> Actually, when I was reviewing the patch series today I got confused as
> well by 'remote'. What about 'remote-link'?

Yes, I was thinking about this one too, it looks a bit clumsy, but it does 
make it clearer, what is meant.

> And v4l2_of_get_remote() can be renamed to v4l2_of_get_remote_link() which
> I think is a lot clearer.
> 
> The text can be improved as well since this:
> 
> - remote: phandle to the other endpoint link DT node.
> 
> is a bit vague. How about:
> 
> - remote-link: phandle to the remote end of this link.

Looks good to me.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/