diff mbox

[v3,4/4,media] dt-bindings: add TI VPIF documentation

Message ID 20161122155244.802-5-khilman@baylibre.com (mailing list archive)
State Superseded, archived
Delegated to: Hans Verkuil
Headers

Commit Message

Kevin Hilman Nov. 22, 2016, 3:52 p.m. UTC 
  Signed-off-by: Kevin Hilman <khilman@baylibre.com>
---
 .../bindings/media/ti,da850-vpif-capture.txt       | 65 ++++++++++++++++++++++
 .../devicetree/bindings/media/ti,da850-vpif.txt    |  8 +++
 2 files changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt

Comments

Rob Herring Nov. 28, 2016, 9:38 p.m. UTC | #1 
On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> ---
>  .../bindings/media/ti,da850-vpif-capture.txt       | 65 ++++++++++++++++++++++
>  .../devicetree/bindings/media/ti,da850-vpif.txt    |  8 +++
>  2 files changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> new file mode 100644
> index 000000000000..c447ac482c1d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> @@ -0,0 +1,65 @@
> +Texas Instruments VPIF Capture
> +------------------------------
> +
> +The TI Video Port InterFace (VPIF) capture component is the primary
> +component for video capture on the DA850 family of TI DaVinci SoCs.
> +
> +TI Document number reference: SPRUH82C
> +
> +Required properties:
> +- compatible: must be "ti,da850-vpif-capture"
> +- reg: physical base address and length of the registers set for the device;
> +- interrupts: should contain IRQ line for the VPIF
> +
> +VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
> +channels or a single 16-bit channel.  It should contain at least one
> +port child node with child 'endpoint' node. Please refer to the
> +bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example using 2 8-bit input channels, one of which is connected to an
> +I2C-connected TVP5147 decoder:
> +
> +	vpif_capture: video-capture@0x00217000 {

Drop the 0x00.

> +		compatible = "ti,da850-vpif-capture";
> +		reg = <0x00217000 0x1000>;
> +		interrupts = <92>;
> +
> +		port {
> +			vpif_ch0: endpoint@0 {
> +				  reg = <0>;

This is missing #size-cells and #addr-cells.

> +				  bus-width = <8>;
> +				  remote-endpoint = <&composite>;
> +			};
> +
> +			vpif_ch1: endpoint@1 {

I think probably channels here should be ports rather than endpoints. 
AIUI, having multiple endpoints is for cases like a mux or 1 to many 
connections. There's only one data flow, but multiple sources or sinks.

> +				  reg = <1>;
> +				  bus-width = <8>;
> +				  data-shift = <8>;
> +			};
> +		};
> +	};
> +
> +[ ... ]
> +
> +&i2c0 {
> +
> +	tvp5147@5d {
> +		compatible = "ti,tvp5147";
> +		reg = <0x5d>;
> +		status = "okay";
> +
> +		port {
> +			composite: endpoint {
> +				hsync-active = <1>;
> +				vsync-active = <1>;
> +				pclk-sample = <0>;
> +
> +				/* VPIF channel 0 (lower 8-bits) */
> +				remote-endpoint = <&vpif_ch0>;
> +				bus-width = <8>;
> +			};
> +		};
> +	};
> +
> +};
> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
> new file mode 100644
> index 000000000000..d004e600aabe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
> @@ -0,0 +1,8 @@
> +Texas Instruments VPIF
> +----------------------
> +
> +The Video Port InterFace (VPIF) is the core component for video output
> +and capture on DA850 TI Davinci SoCs.
> +
> +- compatible: must be "ti,da850-vpif"
> +- reg: physical base address and length of the registers set for the device;

That's it? How does this block relate to the capture block?

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Hilman Nov. 28, 2016, 10:16 p.m. UTC | #2 
Rob Herring <robh@kernel.org> writes:

> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>> ---
>>  .../bindings/media/ti,da850-vpif-capture.txt       | 65 ++++++++++++++++++++++
>>  .../devicetree/bindings/media/ti,da850-vpif.txt    |  8 +++
>>  2 files changed, 73 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> new file mode 100644
>> index 000000000000..c447ac482c1d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> @@ -0,0 +1,65 @@
>> +Texas Instruments VPIF Capture
>> +------------------------------
>> +
>> +The TI Video Port InterFace (VPIF) capture component is the primary
>> +component for video capture on the DA850 family of TI DaVinci SoCs.
>> +
>> +TI Document number reference: SPRUH82C
>> +
>> +Required properties:
>> +- compatible: must be "ti,da850-vpif-capture"
>> +- reg: physical base address and length of the registers set for the device;
>> +- interrupts: should contain IRQ line for the VPIF
>> +
>> +VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
>> +channels or a single 16-bit channel.  It should contain at least one
>> +port child node with child 'endpoint' node. Please refer to the
>> +bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Example using 2 8-bit input channels, one of which is connected to an
>> +I2C-connected TVP5147 decoder:
>> +
>> +	vpif_capture: video-capture@0x00217000 {
>
> Drop the 0x00.
>
>> +		compatible = "ti,da850-vpif-capture";
>> +		reg = <0x00217000 0x1000>;
>> +		interrupts = <92>;
>> +
>> +		port {
>> +			vpif_ch0: endpoint@0 {
>> +				  reg = <0>;
>
> This is missing #size-cells and #addr-cells.
>

Yup.

>> +				  bus-width = <8>;
>> +				  remote-endpoint = <&composite>;
>> +			};
>> +
>> +			vpif_ch1: endpoint@1 {
>
> I think probably channels here should be ports rather than endpoints. 
> AIUI, having multiple endpoints is for cases like a mux or 1 to many 
> connections. There's only one data flow, but multiple sources or sinks.

OK.

>> +				  reg = <1>;
>> +				  bus-width = <8>;
>> +				  data-shift = <8>;
>> +			};
>> +		};
>> +	};
>> +
>> +[ ... ]
>> +
>> +&i2c0 {
>> +
>> +	tvp5147@5d {
>> +		compatible = "ti,tvp5147";
>> +		reg = <0x5d>;
>> +		status = "okay";
>> +
>> +		port {
>> +			composite: endpoint {
>> +				hsync-active = <1>;
>> +				vsync-active = <1>;
>> +				pclk-sample = <0>;
>> +
>> +				/* VPIF channel 0 (lower 8-bits) */
>> +				remote-endpoint = <&vpif_ch0>;
>> +				bus-width = <8>;
>> +			};
>> +		};
>> +	};
>> +
>> +};
>> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
>> new file mode 100644
>> index 000000000000..d004e600aabe
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
>> @@ -0,0 +1,8 @@
>> +Texas Instruments VPIF
>> +----------------------
>> +
>> +The Video Port InterFace (VPIF) is the core component for video output
>> +and capture on DA850 TI Davinci SoCs.
>> +
>> +- compatible: must be "ti,da850-vpif"
>> +- reg: physical base address and length of the registers set for the device;
>
> That's it? How does this block relate to the capture block?

I separated them because the current legacy drivers are separated into 3
different platform drivers.

However, after some discussions with Laurent, I'm going to just create a
single VPIF node with input (capture) and output (display) ports, and
then have to tweak the existing drivers a bit more than I had wanted to.

IOW, I was lazy.

Kevin

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Hilman Nov. 28, 2016, 10:30 p.m. UTC | #3 
Hi Rob,

Rob Herring <robh@kernel.org> writes:

> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>> ---
>>  .../bindings/media/ti,da850-vpif-capture.txt       | 65 ++++++++++++++++++++++
>>  .../devicetree/bindings/media/ti,da850-vpif.txt    |  8 +++
>>  2 files changed, 73 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> new file mode 100644
>> index 000000000000..c447ac482c1d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> @@ -0,0 +1,65 @@
>> +Texas Instruments VPIF Capture
>> +------------------------------
>> +
>> +The TI Video Port InterFace (VPIF) capture component is the primary
>> +component for video capture on the DA850 family of TI DaVinci SoCs.
>> +
>> +TI Document number reference: SPRUH82C
>> +
>> +Required properties:
>> +- compatible: must be "ti,da850-vpif-capture"
>> +- reg: physical base address and length of the registers set for the device;
>> +- interrupts: should contain IRQ line for the VPIF
>> +
>> +VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
>> +channels or a single 16-bit channel.  It should contain at least one
>> +port child node with child 'endpoint' node. Please refer to the
>> +bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Example using 2 8-bit input channels, one of which is connected to an
>> +I2C-connected TVP5147 decoder:
>> +
>> +	vpif_capture: video-capture@0x00217000 {
>> +		reg = <0x00217000 0x1000>;
>> +		interrupts = <92>;
>> +
>> +		port {
>> +			vpif_ch0: endpoint@0 {
>> +				  reg = <0>;
>> +				  bus-width = <8>;
>> +				  remote-endpoint = <&composite>;
>> +			};
>> +
>> +			vpif_ch1: endpoint@1 {
>
> I think probably channels here should be ports rather than endpoints. 
> AIUI, having multiple endpoints is for cases like a mux or 1 to many 
> connections. There's only one data flow, but multiple sources or sinks.

Looking at this closer... , I used an endpoint because it's bascially a
16-bit parallel bus, that can be configured as (up to) 2 8-bit
"channels.  So, based on the video-interfaces.txt doc, I configured this
as a single port, with (up to) 2 endpoints.  That also allows me to
connect output of the decoder directly, using the remote-endpoint
property.

So I guess I'm not fully understanding your suggestion.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Nov. 29, 2016, 2:41 p.m. UTC | #4 
On Mon, Nov 28, 2016 at 4:30 PM, Kevin Hilman <khilman@baylibre.com> wrote:
> Hi Rob,
>
> Rob Herring <robh@kernel.org> writes:
>
>> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
>>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>>> ---
>>>  .../bindings/media/ti,da850-vpif-capture.txt       | 65 ++++++++++++++++++++++
>>>  .../devicetree/bindings/media/ti,da850-vpif.txt    |  8 +++
>>>  2 files changed, 73 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>>>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>>> new file mode 100644
>>> index 000000000000..c447ac482c1d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>>> @@ -0,0 +1,65 @@
>>> +Texas Instruments VPIF Capture
>>> +------------------------------
>>> +
>>> +The TI Video Port InterFace (VPIF) capture component is the primary
>>> +component for video capture on the DA850 family of TI DaVinci SoCs.
>>> +
>>> +TI Document number reference: SPRUH82C
>>> +
>>> +Required properties:
>>> +- compatible: must be "ti,da850-vpif-capture"
>>> +- reg: physical base address and length of the registers set for the device;
>>> +- interrupts: should contain IRQ line for the VPIF
>>> +
>>> +VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
>>> +channels or a single 16-bit channel.  It should contain at least one
>>> +port child node with child 'endpoint' node. Please refer to the
>>> +bindings defined in
>>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>>> +
>>> +Example using 2 8-bit input channels, one of which is connected to an
>>> +I2C-connected TVP5147 decoder:
>>> +
>>> +    vpif_capture: video-capture@0x00217000 {
>>> +            reg = <0x00217000 0x1000>;
>>> +            interrupts = <92>;
>>> +
>>> +            port {
>>> +                    vpif_ch0: endpoint@0 {
>>> +                              reg = <0>;
>>> +                              bus-width = <8>;
>>> +                              remote-endpoint = <&composite>;
>>> +                    };
>>> +
>>> +                    vpif_ch1: endpoint@1 {
>>
>> I think probably channels here should be ports rather than endpoints.
>> AIUI, having multiple endpoints is for cases like a mux or 1 to many
>> connections. There's only one data flow, but multiple sources or sinks.
>
> Looking at this closer... , I used an endpoint because it's bascially a
> 16-bit parallel bus, that can be configured as (up to) 2 8-bit
> "channels.  So, based on the video-interfaces.txt doc, I configured this
> as a single port, with (up to) 2 endpoints.  That also allows me to
> connect output of the decoder directly, using the remote-endpoint
> property.
>
> So I guess I'm not fully understanding your suggestion.

NM, looks like video-interfaces.txt actually spells out this case and
defines doing it as you did.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sakari Ailus Nov. 30, 2016, 9:48 p.m. UTC | #5 
Hi Rob and Kevin,

On Tue, Nov 29, 2016 at 08:41:44AM -0600, Rob Herring wrote:
> On Mon, Nov 28, 2016 at 4:30 PM, Kevin Hilman <khilman@baylibre.com> wrote:
> > Hi Rob,
> >
> > Rob Herring <robh@kernel.org> writes:
> >
> >> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
> >>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> >>> ---
> >>>  .../bindings/media/ti,da850-vpif-capture.txt       | 65 ++++++++++++++++++++++
> >>>  .../devicetree/bindings/media/ti,da850-vpif.txt    |  8 +++
> >>>  2 files changed, 73 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> >>>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> >>> new file mode 100644
> >>> index 000000000000..c447ac482c1d
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> >>> @@ -0,0 +1,65 @@
> >>> +Texas Instruments VPIF Capture
> >>> +------------------------------
> >>> +
> >>> +The TI Video Port InterFace (VPIF) capture component is the primary
> >>> +component for video capture on the DA850 family of TI DaVinci SoCs.
> >>> +
> >>> +TI Document number reference: SPRUH82C
> >>> +
> >>> +Required properties:
> >>> +- compatible: must be "ti,da850-vpif-capture"
> >>> +- reg: physical base address and length of the registers set for the device;
> >>> +- interrupts: should contain IRQ line for the VPIF
> >>> +
> >>> +VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
> >>> +channels or a single 16-bit channel.  It should contain at least one
> >>> +port child node with child 'endpoint' node. Please refer to the
> >>> +bindings defined in
> >>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> >>> +
> >>> +Example using 2 8-bit input channels, one of which is connected to an
> >>> +I2C-connected TVP5147 decoder:
> >>> +
> >>> +    vpif_capture: video-capture@0x00217000 {
> >>> +            reg = <0x00217000 0x1000>;
> >>> +            interrupts = <92>;
> >>> +
> >>> +            port {
> >>> +                    vpif_ch0: endpoint@0 {
> >>> +                              reg = <0>;
> >>> +                              bus-width = <8>;
> >>> +                              remote-endpoint = <&composite>;
> >>> +                    };
> >>> +
> >>> +                    vpif_ch1: endpoint@1 {
> >>
> >> I think probably channels here should be ports rather than endpoints.
> >> AIUI, having multiple endpoints is for cases like a mux or 1 to many
> >> connections. There's only one data flow, but multiple sources or sinks.
> >
> > Looking at this closer... , I used an endpoint because it's bascially a
> > 16-bit parallel bus, that can be configured as (up to) 2 8-bit
> > "channels.  So, based on the video-interfaces.txt doc, I configured this
> > as a single port, with (up to) 2 endpoints.  That also allows me to
> > connect output of the decoder directly, using the remote-endpoint
> > property.
> >
> > So I guess I'm not fully understanding your suggestion.
> 
> NM, looks like video-interfaces.txt actually spells out this case and
> defines doing it as you did.

It's actually the first time I read that portion (at least so that I could
remember) of video-interfaces.txt. I'm not sure if anyone has implemented
that previously, nor how we ended up with the text. The list archive could
probably tell. Cc Guennadi who wrote it. :-) I couldn't immediately find DT
source with this arrangement.

In case of splitting the port into two parallel interfaces, how do you
determine which wires belong to which endpoint? I guess they'd be particular
sets of wires but as there's just a single port it isn't defined by the
port.
Kevin Hilman Nov. 30, 2016, 11:48 p.m. UTC | #6 
Sakari Ailus <sakari.ailus@iki.fi> writes:

> Hi Rob and Kevin,
>
> On Tue, Nov 29, 2016 at 08:41:44AM -0600, Rob Herring wrote:
>> On Mon, Nov 28, 2016 at 4:30 PM, Kevin Hilman <khilman@baylibre.com> wrote:
>> > Hi Rob,
>> >
>> > Rob Herring <robh@kernel.org> writes:
>> >
>> >> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
>> >>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>> >>> ---
>> >>>  .../bindings/media/ti,da850-vpif-capture.txt       | 65 ++++++++++++++++++++++
>> >>>  .../devicetree/bindings/media/ti,da850-vpif.txt    |  8 +++
>> >>>  2 files changed, 73 insertions(+)
>> >>>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> >>>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt
>> >>>
>> >>> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> >>> new file mode 100644
>> >>> index 000000000000..c447ac482c1d
>> >>> --- /dev/null
>> >>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> >>> @@ -0,0 +1,65 @@
>> >>> +Texas Instruments VPIF Capture
>> >>> +------------------------------
>> >>> +
>> >>> +The TI Video Port InterFace (VPIF) capture component is the primary
>> >>> +component for video capture on the DA850 family of TI DaVinci SoCs.
>> >>> +
>> >>> +TI Document number reference: SPRUH82C
>> >>> +
>> >>> +Required properties:
>> >>> +- compatible: must be "ti,da850-vpif-capture"
>> >>> +- reg: physical base address and length of the registers set for the device;
>> >>> +- interrupts: should contain IRQ line for the VPIF
>> >>> +
>> >>> +VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
>> >>> +channels or a single 16-bit channel.  It should contain at least one
>> >>> +port child node with child 'endpoint' node. Please refer to the
>> >>> +bindings defined in
>> >>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> >>> +
>> >>> +Example using 2 8-bit input channels, one of which is connected to an
>> >>> +I2C-connected TVP5147 decoder:
>> >>> +
>> >>> +    vpif_capture: video-capture@0x00217000 {
>> >>> +            reg = <0x00217000 0x1000>;
>> >>> +            interrupts = <92>;
>> >>> +
>> >>> +            port {
>> >>> +                    vpif_ch0: endpoint@0 {
>> >>> +                              reg = <0>;
>> >>> +                              bus-width = <8>;
>> >>> +                              remote-endpoint = <&composite>;
>> >>> +                    };
>> >>> +
>> >>> +                    vpif_ch1: endpoint@1 {
>> >>
>> >> I think probably channels here should be ports rather than endpoints.
>> >> AIUI, having multiple endpoints is for cases like a mux or 1 to many
>> >> connections. There's only one data flow, but multiple sources or sinks.
>> >
>> > Looking at this closer... , I used an endpoint because it's bascially a
>> > 16-bit parallel bus, that can be configured as (up to) 2 8-bit
>> > "channels.  So, based on the video-interfaces.txt doc, I configured this
>> > as a single port, with (up to) 2 endpoints.  That also allows me to
>> > connect output of the decoder directly, using the remote-endpoint
>> > property.
>> >
>> > So I guess I'm not fully understanding your suggestion.
>> 
>> NM, looks like video-interfaces.txt actually spells out this case and
>> defines doing it as you did.
>
> It's actually the first time I read that portion (at least so that I could
> remember) of video-interfaces.txt. I'm not sure if anyone has implemented
> that previously, nor how we ended up with the text. The list archive could
> probably tell. Cc Guennadi who wrote it. :-) I couldn't immediately find DT
> source with this arrangement.
>
> In case of splitting the port into two parallel interfaces, how do you
> determine which wires belong to which endpoint? I guess they'd be particular
> sets of wires but as there's just a single port it isn't defined by the
> port.

Isn't that the point of data-shift?

e.g. it's a single 16-bit parallel bus, where the lower 8 bits are for
channel 0 and the upper 8 bits are for channel 1.  Alternately, the port
can also be configured as a single 16-bit channel (e.g. for raw
capture.)

If you want more details on this hardware, it's pretty well described in
Chapter 35 of http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf.

FWIW, I'm not really picky about how to do this.  I'm trying to learn
"the right way" and am happy to do that, but the feedback so far has
been confusing (at least for someone relatively new to the DT side of
the media framework.)

Kevin

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sakari Ailus Dec. 1, 2016, 8:01 a.m. UTC | #7 
Hi Kevin,

On Wed, Nov 30, 2016 at 03:48:51PM -0800, Kevin Hilman wrote:
> Sakari Ailus <sakari.ailus@iki.fi> writes:
> 
> > Hi Rob and Kevin,
> >
> > On Tue, Nov 29, 2016 at 08:41:44AM -0600, Rob Herring wrote:
> >> On Mon, Nov 28, 2016 at 4:30 PM, Kevin Hilman <khilman@baylibre.com> wrote:
> >> > Hi Rob,
> >> >
> >> > Rob Herring <robh@kernel.org> writes:
> >> >
> >> >> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
> >> >>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> >> >>> ---
> >> >>>  .../bindings/media/ti,da850-vpif-capture.txt       | 65 ++++++++++++++++++++++
> >> >>>  .../devicetree/bindings/media/ti,da850-vpif.txt    |  8 +++
> >> >>>  2 files changed, 73 insertions(+)
> >> >>>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> >> >>>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt
> >> >>>
> >> >>> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> >> >>> new file mode 100644
> >> >>> index 000000000000..c447ac482c1d
> >> >>> --- /dev/null
> >> >>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> >> >>> @@ -0,0 +1,65 @@
> >> >>> +Texas Instruments VPIF Capture
> >> >>> +------------------------------
> >> >>> +
> >> >>> +The TI Video Port InterFace (VPIF) capture component is the primary
> >> >>> +component for video capture on the DA850 family of TI DaVinci SoCs.
> >> >>> +
> >> >>> +TI Document number reference: SPRUH82C
> >> >>> +
> >> >>> +Required properties:
> >> >>> +- compatible: must be "ti,da850-vpif-capture"
> >> >>> +- reg: physical base address and length of the registers set for the device;
> >> >>> +- interrupts: should contain IRQ line for the VPIF
> >> >>> +
> >> >>> +VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
> >> >>> +channels or a single 16-bit channel.  It should contain at least one
> >> >>> +port child node with child 'endpoint' node. Please refer to the
> >> >>> +bindings defined in
> >> >>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> >> >>> +
> >> >>> +Example using 2 8-bit input channels, one of which is connected to an
> >> >>> +I2C-connected TVP5147 decoder:
> >> >>> +
> >> >>> +    vpif_capture: video-capture@0x00217000 {
> >> >>> +            reg = <0x00217000 0x1000>;
> >> >>> +            interrupts = <92>;
> >> >>> +
> >> >>> +            port {
> >> >>> +                    vpif_ch0: endpoint@0 {
> >> >>> +                              reg = <0>;
> >> >>> +                              bus-width = <8>;
> >> >>> +                              remote-endpoint = <&composite>;
> >> >>> +                    };
> >> >>> +
> >> >>> +                    vpif_ch1: endpoint@1 {
> >> >>
> >> >> I think probably channels here should be ports rather than endpoints.
> >> >> AIUI, having multiple endpoints is for cases like a mux or 1 to many
> >> >> connections. There's only one data flow, but multiple sources or sinks.
> >> >
> >> > Looking at this closer... , I used an endpoint because it's bascially a
> >> > 16-bit parallel bus, that can be configured as (up to) 2 8-bit
> >> > "channels.  So, based on the video-interfaces.txt doc, I configured this
> >> > as a single port, with (up to) 2 endpoints.  That also allows me to
> >> > connect output of the decoder directly, using the remote-endpoint
> >> > property.
> >> >
> >> > So I guess I'm not fully understanding your suggestion.
> >> 
> >> NM, looks like video-interfaces.txt actually spells out this case and
> >> defines doing it as you did.
> >
> > It's actually the first time I read that portion (at least so that I could
> > remember) of video-interfaces.txt. I'm not sure if anyone has implemented
> > that previously, nor how we ended up with the text. The list archive could
> > probably tell. Cc Guennadi who wrote it. :-) I couldn't immediately find DT
> > source with this arrangement.
> >
> > In case of splitting the port into two parallel interfaces, how do you
> > determine which wires belong to which endpoint? I guess they'd be particular
> > sets of wires but as there's just a single port it isn't defined by the
> > port.
> 
> Isn't that the point of data-shift?

Right.

> 
> e.g. it's a single 16-bit parallel bus, where the lower 8 bits are for
> channel 0 and the upper 8 bits are for channel 1.  Alternately, the port
> can also be configured as a single 16-bit channel (e.g. for raw
> capture.)
> 
> If you want more details on this hardware, it's pretty well described in
> Chapter 35 of http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf.
> 
> FWIW, I'm not really picky about how to do this.  I'm trying to learn
> "the right way" and am happy to do that, but the feedback so far has
> been confusing (at least for someone relatively new to the DT side of
> the media framework.)

I have to admit I'm more familiar with cameras and serial busses than TV
tuners and parallel busses. Yeah, the data-shift is there for that purpose.

I think you should document which properties are expected to be found in the
port / endpoint nodes, the video-interfaces.txt has very many of them and I
don't think many of those are even relevant in this case.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
new file mode 100644
index 000000000000..c447ac482c1d
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
@@ -0,0 +1,65 @@ 
+Texas Instruments VPIF Capture
+------------------------------
+
+The TI Video Port InterFace (VPIF) capture component is the primary
+component for video capture on the DA850 family of TI DaVinci SoCs.
+
+TI Document number reference: SPRUH82C
+
+Required properties:
+- compatible: must be "ti,da850-vpif-capture"
+- reg: physical base address and length of the registers set for the device;
+- interrupts: should contain IRQ line for the VPIF
+
+VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
+channels or a single 16-bit channel.  It should contain at least one
+port child node with child 'endpoint' node. Please refer to the
+bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example using 2 8-bit input channels, one of which is connected to an
+I2C-connected TVP5147 decoder:
+
+	vpif_capture: video-capture@0x00217000 {
+		compatible = "ti,da850-vpif-capture";
+		reg = <0x00217000 0x1000>;
+		interrupts = <92>;
+
+		port {
+			vpif_ch0: endpoint@0 {
+				  reg = <0>;
+				  bus-width = <8>;
+				  remote-endpoint = <&composite>;
+			};
+
+			vpif_ch1: endpoint@1 {
+				  reg = <1>;
+				  bus-width = <8>;
+				  data-shift = <8>;
+			};
+		};
+	};
+
+[ ... ]
+
+&i2c0 {
+
+	tvp5147@5d {
+		compatible = "ti,tvp5147";
+		reg = <0x5d>;
+		status = "okay";
+
+		port {
+			composite: endpoint {
+				hsync-active = <1>;
+				vsync-active = <1>;
+				pclk-sample = <0>;
+
+				/* VPIF channel 0 (lower 8-bits) */
+				remote-endpoint = <&vpif_ch0>;
+				bus-width = <8>;
+			};
+		};
+	};
+
+};
diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
new file mode 100644
index 000000000000..d004e600aabe
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
@@ -0,0 +1,8 @@ 
+Texas Instruments VPIF
+----------------------
+
+The Video Port InterFace (VPIF) is the core component for video output
+and capture on DA850 TI Davinci SoCs.
+
+- compatible: must be "ti,da850-vpif"
+- reg: physical base address and length of the registers set for the device;