diff mbox

[3/9] Doc/DT: Add DT binding documentation for DVI Connector

Message ID 1393590016-9361-4-git-send-email-tomi.valkeinen@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomi Valkeinen Feb. 28, 2014, 12:20 p.m. UTC
Add DT binding documentation for DVI Connector.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Archit Taneja <archit@ti.com>
---
 .../devicetree/bindings/video/dvi-connector.txt    | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/dvi-connector.txt

Comments

Philipp Zabel Feb. 28, 2014, 1:43 p.m. UTC | #1
Hi Tomi,

Am Freitag, den 28.02.2014, 14:20 +0200 schrieb Tomi Valkeinen:
> Add DT binding documentation for DVI Connector.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reviewed-by: Archit Taneja <archit@ti.com>
> ---
>  .../devicetree/bindings/video/dvi-connector.txt    | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/dvi-connector.txt
> 
> diff --git a/Documentation/devicetree/bindings/video/dvi-connector.txt b/Documentation/devicetree/bindings/video/dvi-connector.txt
> new file mode 100644
> index 000000000000..6a0aff866c78
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/dvi-connector.txt
> @@ -0,0 +1,26 @@
> +DVI Connector
> +==============
> +
> +Required properties:
> +- compatible: "dvi-connector"
> +
> +Optional properties:
> +- label: a symbolic name for the connector
> +- i2c-bus: phandle to the i2c bus that is connected to DVI DDC

For the i.MX bindings I had called this property 'ddc', but
Documentation/devicetree/bindings/panel/simple-panel.txt already
uses 'ddc-i2c-bus'. We should definitely standardize this.

> +Required nodes:
> +- Video port for DVI input
> +
> +Example
> +-------
> +
> +dvi0: connector@0 {
> +	compatible = "dvi-connector";
> +	label = "dvi";
> +
> +	i2c-bus = <&i2c3>;
> +
> +	dvi_connector_in: endpoint {
> +		remote-endpoint = <&tfp410_out>;
> +	};
> +};

regards
Philipp
Russell King - ARM Linux Feb. 28, 2014, 3:59 p.m. UTC | #2
On Fri, Feb 28, 2014 at 02:20:10PM +0200, Tomi Valkeinen wrote:
> Add DT binding documentation for DVI Connector.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reviewed-by: Archit Taneja <archit@ti.com>
> ---
>  .../devicetree/bindings/video/dvi-connector.txt    | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/dvi-connector.txt
> 
> diff --git a/Documentation/devicetree/bindings/video/dvi-connector.txt b/Documentation/devicetree/bindings/video/dvi-connector.txt
> new file mode 100644
> index 000000000000..6a0aff866c78
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/dvi-connector.txt
> @@ -0,0 +1,26 @@
> +DVI Connector
> +==============
> +
> +Required properties:
> +- compatible: "dvi-connector"
> +
> +Optional properties:
> +- label: a symbolic name for the connector
> +- i2c-bus: phandle to the i2c bus that is connected to DVI DDC
> +
> +Required nodes:
> +- Video port for DVI input
> +
> +Example
> +-------
> +
> +dvi0: connector@0 {
> +	compatible = "dvi-connector";
> +	label = "dvi";
> +
> +	i2c-bus = <&i2c3>;
> +
> +	dvi_connector_in: endpoint {
> +		remote-endpoint = <&tfp410_out>;
> +	};
> +};

This looks far too simplistic.  There are different classes of DVI
connector - there is:

DVI A - analogue only
DVI D - digital only (single and dual link)
DVI I - both (single and dual digital link)

DRM at least makes a distinction between these three classes, and this
disctinction is part of the user API.  How would a display system know
which kind of DVI connector is wired up on the board from this DT
description?
Tomi Valkeinen Feb. 28, 2014, 4:12 p.m. UTC | #3
On 28/02/14 17:59, Russell King - ARM Linux wrote:

>> +dvi0: connector@0 {
>> +	compatible = "dvi-connector";
>> +	label = "dvi";
>> +
>> +	i2c-bus = <&i2c3>;
>> +
>> +	dvi_connector_in: endpoint {
>> +		remote-endpoint = <&tfp410_out>;
>> +	};
>> +};
> 
> This looks far too simplistic.  There are different classes of DVI
> connector - there is:
> 
> DVI A - analogue only
> DVI D - digital only (single and dual link)
> DVI I - both (single and dual digital link)
> 
> DRM at least makes a distinction between these three classes, and this
> disctinction is part of the user API.  How would a display system know
> which kind of DVI connector is wired up on the board from this DT
> description?

Yes, I think that's a valid change. But do we also need to specify
single/dual link, in addition to the three types?

I guess the compatible string is the easiest way for differentation, at
least for the three main types, i.e. "dvi-d-connector" etc.

"dvi-d-1l-connector" and "dvi-d-2l-connector" for the single/dual link?
That looks a bit funny.

 Tomi
Russell King - ARM Linux Feb. 28, 2014, 4:23 p.m. UTC | #4
On Fri, Feb 28, 2014 at 06:12:23PM +0200, Tomi Valkeinen wrote:
> On 28/02/14 17:59, Russell King - ARM Linux wrote:
> 
> >> +dvi0: connector@0 {
> >> +	compatible = "dvi-connector";
> >> +	label = "dvi";
> >> +
> >> +	i2c-bus = <&i2c3>;
> >> +
> >> +	dvi_connector_in: endpoint {
> >> +		remote-endpoint = <&tfp410_out>;
> >> +	};
> >> +};
> > 
> > This looks far too simplistic.  There are different classes of DVI
> > connector - there is:
> > 
> > DVI A - analogue only
> > DVI D - digital only (single and dual link)
> > DVI I - both (single and dual digital link)
> > 
> > DRM at least makes a distinction between these three classes, and this
> > disctinction is part of the user API.  How would a display system know
> > which kind of DVI connector is wired up on the board from this DT
> > description?
> 
> Yes, I think that's a valid change. But do we also need to specify
> single/dual link, in addition to the three types?

I would argue that as it's a difference in physical hardware, then it
should be described in DT, even if we don't use it.  The reasoning is
that although we may not use it today, we may need to use it in the
future, and as we're describing what the hardware actually is - and
even in this case what pins may be present or missing on the connector,
it's unlikely to be problematical (the only problem is when someone
omits it...)

> I guess the compatible string is the easiest way for differentation, at
> least for the three main types, i.e. "dvi-d-connector" etc.
> 
> "dvi-d-1l-connector" and "dvi-d-2l-connector" for the single/dual link?
> That looks a bit funny.

I think that starts getting a tad messy:

	dvi-a-connector
	dvi-d-1l-connector
	dvi-d-2l-connector
	dvi-i-1l-connector
	dvi-i-2l-connector

That's rather a lot of compatible strings.  Another possibility is:

	compatible = "dvi-connector";
	analog;
	digital;
	single-link;
	dual-link;

I'm debating whether "-signalling" should be on the 2nd and 3rd (or...
-signaling depending on how you prefer to spell that word.)  At least
one of "analog" and/or "digital" must be specified, and if "digital"
is specified, then exactly one of "single-link" or "dual-link" should
be specified.

So, this would mean we end up with:

	compatible = "dvi-connector";
	analog;
	digital;
	dual-link;

for a DVI-I dual-link connector.
Sebastian Reichel Feb. 28, 2014, 4:25 p.m. UTC | #5
On Fri, Feb 28, 2014 at 06:12:23PM +0200, Tomi Valkeinen wrote:
> On 28/02/14 17:59, Russell King - ARM Linux wrote:
> 
> >> +dvi0: connector@0 {
> >> +	compatible = "dvi-connector";
> >> +	label = "dvi";
> >> +
> >> +	i2c-bus = <&i2c3>;
> >> +
> >> +	dvi_connector_in: endpoint {
> >> +		remote-endpoint = <&tfp410_out>;
> >> +	};
> >> +};
> > 
> > This looks far too simplistic.  There are different classes of DVI
> > connector - there is:
> > 
> > DVI A - analogue only
> > DVI D - digital only (single and dual link)
> > DVI I - both (single and dual digital link)
> > 
> > DRM at least makes a distinction between these three classes, and this
> > disctinction is part of the user API.  How would a display system know
> > which kind of DVI connector is wired up on the board from this DT
> > description?
> 
> Yes, I think that's a valid change. But do we also need to specify
> single/dual link, in addition to the three types?
> 
> I guess the compatible string is the easiest way for differentation, at
> least for the three main types, i.e. "dvi-d-connector" etc.
> 
> "dvi-d-1l-connector" and "dvi-d-2l-connector" for the single/dual link?
> That looks a bit funny.

maybe like this:

Required Properties:
 - compatible: should contain one of the following:
  * "dvi-d-connector"
  * "dvi-a-connector"
  * "dvi-i-connector"

Optional Properties:
 - dual-link: Should be set for dual-link capable connectors

-- Sebastian
Philipp Zabel Feb. 28, 2014, 4:25 p.m. UTC | #6
Am Freitag, den 28.02.2014, 15:59 +0000 schrieb Russell King - ARM
Linux:
> On Fri, Feb 28, 2014 at 02:20:10PM +0200, Tomi Valkeinen wrote:
> > Add DT binding documentation for DVI Connector.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > Reviewed-by: Archit Taneja <archit@ti.com>
> > ---
> >  .../devicetree/bindings/video/dvi-connector.txt    | 26 ++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/video/dvi-connector.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/video/dvi-connector.txt b/Documentation/devicetree/bindings/video/dvi-connector.txt
> > new file mode 100644
> > index 000000000000..6a0aff866c78
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/dvi-connector.txt
> > @@ -0,0 +1,26 @@
> > +DVI Connector
> > +==============
> > +
> > +Required properties:
> > +- compatible: "dvi-connector"
> > +
> > +Optional properties:
> > +- label: a symbolic name for the connector
> > +- i2c-bus: phandle to the i2c bus that is connected to DVI DDC
> > +
> > +Required nodes:
> > +- Video port for DVI input
> > +
> > +Example
> > +-------
> > +
> > +dvi0: connector@0 {
> > +	compatible = "dvi-connector";
> > +	label = "dvi";
> > +
> > +	i2c-bus = <&i2c3>;
> > +
> > +	dvi_connector_in: endpoint {
> > +		remote-endpoint = <&tfp410_out>;
> > +	};
> > +};
> 
> This looks far too simplistic.  There are different classes of DVI
> connector - there is:
> 
> DVI A - analogue only
> DVI D - digital only (single and dual link)
> DVI I - both (single and dual digital link)
> 
> DRM at least makes a distinction between these three classes, and this
> disctinction is part of the user API.  How would a display system know
> which kind of DVI connector is wired up on the board from this DT
> description?

Maybe this could be inferred from the sources connected to it. For
example a i.MX5 board with the SoC internal TV Encoder and an external
SiI902x HDMI encoder connected to the same DVI I connector:

ipu {
	port@2 {
		ipu_di0_disp0: endpoint {
			remote-endpoint = <&sii902x_in>;
		};
	};
	port@3 {
		ipu_di1_tve: endpoint {
			remote-endpoint = <&tve_in>;
		};
	};
};

&sii902x {
	compatible = "si,sii9022";

	port@0 {
		sii902x_in: endpoint {
			remote-endpoint = <&ipu_di0>;
		};
	};
	port@1 {
		sii902x_out: endpoint {
			remote-endpoint = <&dvi_d_in>;
		};
	};
};

&tve {
	compatible = "fsl,imx53-tve";
	port@0 {
		tve_in: endpoint {
			remote-endpoint = <&ipu_di1>;
		};
	};
	port@1 {
		tve_out: endpoint {
			remote-endpoint = <&dvi_a_in>;
		};
	};
};

dvi-connector {
	compatible = "dvi-connector";
	ddc-i2c-bus = <&i2c3>;

	port {
		dvi_d_in: endpoint@0 {
			remote-endpoint = <&sii902x_out>;
		};
		dvi_a_in: endpoint@1 {
			remote-endpoint = <&tve_out>;
		};
	};
};

It should be possible to let the connector know that those two endpoints
are connected to a TMDS source and to a VGA source, respectively.

regards
Philipp
Warner Losh Feb. 28, 2014, 4:28 p.m. UTC | #7
On Feb 28, 2014, at 9:23 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Fri, Feb 28, 2014 at 06:12:23PM +0200, Tomi Valkeinen wrote:
>> On 28/02/14 17:59, Russell King - ARM Linux wrote:
>> 
>>>> +dvi0: connector@0 {
>>>> +	compatible = "dvi-connector";
>>>> +	label = "dvi";
>>>> +
>>>> +	i2c-bus = <&i2c3>;
>>>> +
>>>> +	dvi_connector_in: endpoint {
>>>> +		remote-endpoint = <&tfp410_out>;
>>>> +	};
>>>> +};
>>> 
>>> This looks far too simplistic.  There are different classes of DVI
>>> connector - there is:
>>> 
>>> DVI A - analogue only
>>> DVI D - digital only (single and dual link)
>>> DVI I - both (single and dual digital link)
>>> 
>>> DRM at least makes a distinction between these three classes, and this
>>> disctinction is part of the user API.  How would a display system know
>>> which kind of DVI connector is wired up on the board from this DT
>>> description?
>> 
>> Yes, I think that's a valid change. But do we also need to specify
>> single/dual link, in addition to the three types?
> 
> I would argue that as it's a difference in physical hardware, then it
> should be described in DT, even if we don't use it.  The reasoning is
> that although we may not use it today, we may need to use it in the
> future, and as we're describing what the hardware actually is - and
> even in this case what pins may be present or missing on the connector,
> it's unlikely to be problematical (the only problem is when someone
> omits it...)

And the “we” that uses the DT files is larger than just the Linux, and one of
those systems may use it.

Warner
Tomi Valkeinen March 3, 2014, 6:25 a.m. UTC | #8
On 28/02/14 15:43, Philipp Zabel wrote:
> Hi Tomi,
> 
> Am Freitag, den 28.02.2014, 14:20 +0200 schrieb Tomi Valkeinen:
>> Add DT binding documentation for DVI Connector.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Reviewed-by: Archit Taneja <archit@ti.com>
>> ---
>>  .../devicetree/bindings/video/dvi-connector.txt    | 26 ++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/video/dvi-connector.txt
>>
>> diff --git a/Documentation/devicetree/bindings/video/dvi-connector.txt b/Documentation/devicetree/bindings/video/dvi-connector.txt
>> new file mode 100644
>> index 000000000000..6a0aff866c78
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/video/dvi-connector.txt
>> @@ -0,0 +1,26 @@
>> +DVI Connector
>> +==============
>> +
>> +Required properties:
>> +- compatible: "dvi-connector"
>> +
>> +Optional properties:
>> +- label: a symbolic name for the connector
>> +- i2c-bus: phandle to the i2c bus that is connected to DVI DDC
> 
> For the i.MX bindings I had called this property 'ddc', but
> Documentation/devicetree/bindings/panel/simple-panel.txt already
> uses 'ddc-i2c-bus'. We should definitely standardize this.

I like 'ddc-i2c-bus'.

 Tomi
Tomi Valkeinen March 3, 2014, 6:42 a.m. UTC | #9
On 28/02/14 18:23, Russell King - ARM Linux wrote:

>> I guess the compatible string is the easiest way for differentation, at
>> least for the three main types, i.e. "dvi-d-connector" etc.
>>
>> "dvi-d-1l-connector" and "dvi-d-2l-connector" for the single/dual link?
>> That looks a bit funny.
> 
> I think that starts getting a tad messy:
> 
> 	dvi-a-connector
> 	dvi-d-1l-connector
> 	dvi-d-2l-connector
> 	dvi-i-1l-connector
> 	dvi-i-2l-connector

Yes, it's messy. Pondering this over the weekend, I think it makes sense
to have just one compatible string, as all those connectors are still
the same DVI connector, just different variations of the same.

> That's rather a lot of compatible strings.  Another possibility is:
> 
> 	compatible = "dvi-connector";
> 	analog;
> 	digital;
> 	single-link;
> 	dual-link;
> 
> I'm debating whether "-signalling" should be on the 2nd and 3rd (or...
> -signaling depending on how you prefer to spell that word.)  At least
> one of "analog" and/or "digital" must be specified, and if "digital"
> is specified, then exactly one of "single-link" or "dual-link" should
> be specified.
> 
> So, this would mean we end up with:
> 
> 	compatible = "dvi-connector";
> 	analog;
> 	digital;
> 	dual-link;
> 
> for a DVI-I dual-link connector.

Another option would be:

num-links = 2;

But I like your suggestion more. We could also optimize it, "digital" is
extra as both "single-link" and "dual-link" mean also digital. But... I
don't see much point in optimizing that way. So I agree with your
suggestion as is.

 Tomi
Daniel Vetter March 4, 2014, 12:54 p.m. UTC | #10
On Fri, Feb 28, 2014 at 04:23:27PM +0000, Russell King - ARM Linux wrote:
> On Fri, Feb 28, 2014 at 06:12:23PM +0200, Tomi Valkeinen wrote:
> > On 28/02/14 17:59, Russell King - ARM Linux wrote:
> > 
> > >> +dvi0: connector@0 {
> > >> +	compatible = "dvi-connector";
> > >> +	label = "dvi";
> > >> +
> > >> +	i2c-bus = <&i2c3>;
> > >> +
> > >> +	dvi_connector_in: endpoint {
> > >> +		remote-endpoint = <&tfp410_out>;
> > >> +	};
> > >> +};
> > > 
> > > This looks far too simplistic.  There are different classes of DVI
> > > connector - there is:
> > > 
> > > DVI A - analogue only
> > > DVI D - digital only (single and dual link)
> > > DVI I - both (single and dual digital link)
> > > 
> > > DRM at least makes a distinction between these three classes, and this
> > > disctinction is part of the user API.  How would a display system know
> > > which kind of DVI connector is wired up on the board from this DT
> > > description?
> > 
> > Yes, I think that's a valid change. But do we also need to specify
> > single/dual link, in addition to the three types?
> 
> I would argue that as it's a difference in physical hardware, then it
> should be described in DT, even if we don't use it.  The reasoning is
> that although we may not use it today, we may need to use it in the
> future, and as we're describing what the hardware actually is - and
> even in this case what pins may be present or missing on the connector,
> it's unlikely to be problematical (the only problem is when someone
> omits it...)

If you plug a dual-link dvi screen into a soc which can do dual-link but
the actual connector is cheap and doesn't have this wired up
(differentiate wtf) then the kernel needs to know. Otherwise it can't
correctly filter out the modes with dotclocks high enough to require dual
link and the user will look at a black screen.

3.14 has a regression in drm/i915 where we've screwed this up ;-)
-Daniel
Tomi Valkeinen March 5, 2014, 8:41 a.m. UTC | #11
On 28/02/14 18:23, Russell King - ARM Linux wrote:

> That's rather a lot of compatible strings.  Another possibility is:
> 
> 	compatible = "dvi-connector";
> 	analog;
> 	digital;
> 	single-link;
> 	dual-link;

I made the following changes compared to the posted version. I decided
to leave the "single-link" out, as it's implied if "digital" is set.

 Tomi

@@ -6,11 +6,16 @@ Required properties:

 Optional properties:
 - label: a symbolic name for the connector
-- i2c-bus: phandle to the i2c bus that is connected to DVI DDC
+- ddc-i2c-bus: phandle to the i2c bus that is connected to DVI DDC
+- analog: the connector has DVI analog pins
+- digital: the connector has DVI digital pins
+- dual-link: the connector has pins for DVI dual-link

 Required nodes:
 - Video port for DVI input

+Note: One (or both) of 'analog' or 'digital' must be set.
+
 Example
 -------

@@ -18,7 +23,9 @@ dvi0: connector@0 {
        compatible = "dvi-connector";
        label = "dvi";

-       i2c-bus = <&i2c3>;
+       digital;
+
+       ddc-i2c-bus = <&i2c3>;

        dvi_connector_in: endpoint {
                remote-endpoint = <&tfp410_out>;
Geert Uytterhoeven March 6, 2014, 8:39 a.m. UTC | #12
On Wed, Mar 5, 2014 at 9:41 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 28/02/14 18:23, Russell King - ARM Linux wrote:
>
>> That's rather a lot of compatible strings.  Another possibility is:
>>
>>       compatible = "dvi-connector";
>>       analog;
>>       digital;
>>       single-link;
>>       dual-link;
>
> I made the following changes compared to the posted version. I decided
> to leave the "single-link" out, as it's implied if "digital" is set.
>
>  Tomi
>
> @@ -6,11 +6,16 @@ Required properties:
>
>  Optional properties:
>  - label: a symbolic name for the connector
> -- i2c-bus: phandle to the i2c bus that is connected to DVI DDC
> +- ddc-i2c-bus: phandle to the i2c bus that is connected to DVI DDC
> +- analog: the connector has DVI analog pins
> +- digital: the connector has DVI digital pins
> +- dual-link: the connector has pins for DVI dual-link
>
>  Required nodes:
>  - Video port for DVI input
>
> +Note: One (or both) of 'analog' or 'digital' must be set.

So dual-link needs both "digital" and "dual-link"?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Tomi Valkeinen March 6, 2014, 8:52 a.m. UTC | #13
On 06/03/14 10:39, Geert Uytterhoeven wrote:
> On Wed, Mar 5, 2014 at 9:41 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On 28/02/14 18:23, Russell King - ARM Linux wrote:
>>
>>> That's rather a lot of compatible strings.  Another possibility is:
>>>
>>>       compatible = "dvi-connector";
>>>       analog;
>>>       digital;
>>>       single-link;
>>>       dual-link;
>>
>> I made the following changes compared to the posted version. I decided
>> to leave the "single-link" out, as it's implied if "digital" is set.
>>
>>  Tomi
>>
>> @@ -6,11 +6,16 @@ Required properties:
>>
>>  Optional properties:
>>  - label: a symbolic name for the connector
>> -- i2c-bus: phandle to the i2c bus that is connected to DVI DDC
>> +- ddc-i2c-bus: phandle to the i2c bus that is connected to DVI DDC
>> +- analog: the connector has DVI analog pins
>> +- digital: the connector has DVI digital pins
>> +- dual-link: the connector has pins for DVI dual-link
>>
>>  Required nodes:
>>  - Video port for DVI input
>>
>> +Note: One (or both) of 'analog' or 'digital' must be set.
> 
> So dual-link needs both "digital" and "dual-link"?

Yes. It is extra, but it felt clearer to me to have 'digital' as a
matching property for 'analog'.

Alternatively we could have three options:

analog;
digital-single-link;
digital-dual-link;

My reasoning to the format I chose was basically that when a connector
supports 'digital', it contains TMDS clock and TMDS data for link 1.
Adding dual link to that adds only TMDS data for link 2, so the second
data link is kind of an additional feature, marked with a flag.

Not a very big argument, and I'm fine with other format suggestions.

 Tomi
Philipp Zabel March 7, 2014, 2:17 p.m. UTC | #14
Hi,

Am Donnerstag, den 06.03.2014, 10:52 +0200 schrieb Tomi Valkeinen:
> On 06/03/14 10:39, Geert Uytterhoeven wrote:
> > On Wed, Mar 5, 2014 at 9:41 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >> On 28/02/14 18:23, Russell King - ARM Linux wrote:
> >>
> >>> That's rather a lot of compatible strings.  Another possibility is:
> >>>
> >>>       compatible = "dvi-connector";
> >>>       analog;
> >>>       digital;
> >>>       single-link;
> >>>       dual-link;
> >>
> >> I made the following changes compared to the posted version. I decided
> >> to leave the "single-link" out, as it's implied if "digital" is set.
> >>
> >>  Tomi
> >>
> >> @@ -6,11 +6,16 @@ Required properties:
> >>
> >>  Optional properties:
> >>  - label: a symbolic name for the connector
> >> -- i2c-bus: phandle to the i2c bus that is connected to DVI DDC
> >> +- ddc-i2c-bus: phandle to the i2c bus that is connected to DVI DDC
> >> +- analog: the connector has DVI analog pins
> >> +- digital: the connector has DVI digital pins
> >> +- dual-link: the connector has pins for DVI dual-link
> >>
> >>  Required nodes:
> >>  - Video port for DVI input
> >>
> >> +Note: One (or both) of 'analog' or 'digital' must be set.
> > 
> > So dual-link needs both "digital" and "dual-link"?
> 
> Yes. It is extra, but it felt clearer to me to have 'digital' as a
> matching property for 'analog'.
> 
> Alternatively we could have three options:
> 
> analog;
> digital-single-link;
> digital-dual-link;
> 
> My reasoning to the format I chose was basically that when a connector
> supports 'digital', it contains TMDS clock and TMDS data for link 1.
> Adding dual link to that adds only TMDS data for link 2, so the second
> data link is kind of an additional feature, marked with a flag.
> 
> Not a very big argument, and I'm fine with other format suggestions.

I'd prefer the analog / digital / dual-link variant for aesthetic
reasons. But looking at other connector types, I wonder if this should
be generalized even more:

For HDMI/DVI (digital) single-link means one clock pair and 3 TMDS data
pairs, dual-link means one clock pair and 6 data pairs.

On LVDS connectors, there usually are one clock pair and three (18-bit)
or four (24-bit) LVDS data pairs, in dual channel configuration two
clock pairs and 6 or 8 data pairs are used.

For DisplayPort there is no separate clock pair, but 1 to 4 data pairs,
and MIPI DSI again has one clock pair and a one or more data pairs.

There are already optional endpoint configuration properties
'data-lanes' and 'clock-lanes' for MIPI CSI-2 defined in
Documentation/devicetree/bindings/media/video-interfaces.txt.
Could/should this be aligned with the same?

regards
Philipp
Tomi Valkeinen March 10, 2014, 10:32 a.m. UTC | #15
On 07/03/14 16:17, Philipp Zabel wrote:
> Hi,
> 
> Am Donnerstag, den 06.03.2014, 10:52 +0200 schrieb Tomi Valkeinen:

>> analog;
>> digital-single-link;
>> digital-dual-link;
>>
>> My reasoning to the format I chose was basically that when a connector
>> supports 'digital', it contains TMDS clock and TMDS data for link 1.
>> Adding dual link to that adds only TMDS data for link 2, so the second
>> data link is kind of an additional feature, marked with a flag.
>>
>> Not a very big argument, and I'm fine with other format suggestions.
> 
> I'd prefer the analog / digital / dual-link variant for aesthetic
> reasons. But looking at other connector types, I wonder if this should
> be generalized even more:
> 
> For HDMI/DVI (digital) single-link means one clock pair and 3 TMDS data
> pairs, dual-link means one clock pair and 6 data pairs.
> 
> On LVDS connectors, there usually are one clock pair and three (18-bit)
> or four (24-bit) LVDS data pairs, in dual channel configuration two
> clock pairs and 6 or 8 data pairs are used.
> 
> For DisplayPort there is no separate clock pair, but 1 to 4 data pairs,
> and MIPI DSI again has one clock pair and a one or more data pairs.
> 
> There are already optional endpoint configuration properties
> 'data-lanes' and 'clock-lanes' for MIPI CSI-2 defined in
> Documentation/devicetree/bindings/media/video-interfaces.txt.
> Could/should this be aligned with the same?

Hmm. Well, at least for HDMI and DP we anyway need the connector type,
which tells the form factor, and it also tells the TMDS details. So, we
could define the lanes in a common way, but we'd still need extra
information.

For MIPI DSI and (I believe) LVDS, we don't need connectors. Connectors,
as described in this binding, are meant for standard hotpluggable
connectors to which you can connect any device that has that same connector.

 Tomi
Rob Herring March 10, 2014, 9:45 p.m. UTC | #16
On Fri, Feb 28, 2014 at 10:25 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Am Freitag, den 28.02.2014, 15:59 +0000 schrieb Russell King - ARM
> Linux:
>> On Fri, Feb 28, 2014 at 02:20:10PM +0200, Tomi Valkeinen wrote:
>> > Add DT binding documentation for DVI Connector.
>> >
>> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> > Reviewed-by: Archit Taneja <archit@ti.com>
>> > ---
>> >  .../devicetree/bindings/video/dvi-connector.txt    | 26 ++++++++++++++++++++++
>> >  1 file changed, 26 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/video/dvi-connector.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/video/dvi-connector.txt b/Documentation/devicetree/bindings/video/dvi-connector.txt
>> > new file mode 100644
>> > index 000000000000..6a0aff866c78
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/video/dvi-connector.txt
>> > @@ -0,0 +1,26 @@
>> > +DVI Connector
>> > +==============
>> > +
>> > +Required properties:
>> > +- compatible: "dvi-connector"
>> > +
>> > +Optional properties:
>> > +- label: a symbolic name for the connector
>> > +- i2c-bus: phandle to the i2c bus that is connected to DVI DDC
>> > +
>> > +Required nodes:
>> > +- Video port for DVI input
>> > +
>> > +Example
>> > +-------
>> > +
>> > +dvi0: connector@0 {
>> > +   compatible = "dvi-connector";
>> > +   label = "dvi";
>> > +
>> > +   i2c-bus = <&i2c3>;
>> > +
>> > +   dvi_connector_in: endpoint {
>> > +           remote-endpoint = <&tfp410_out>;
>> > +   };
>> > +};
>>
>> This looks far too simplistic.  There are different classes of DVI
>> connector - there is:
>>
>> DVI A - analogue only
>> DVI D - digital only (single and dual link)
>> DVI I - both (single and dual digital link)
>>
>> DRM at least makes a distinction between these three classes, and this
>> disctinction is part of the user API.  How would a display system know
>> which kind of DVI connector is wired up on the board from this DT
>> description?
>
> Maybe this could be inferred from the sources connected to it. For
> example a i.MX5 board with the SoC internal TV Encoder and an external
> SiI902x HDMI encoder connected to the same DVI I connector:
>
> ipu {
>         port@2 {
>                 ipu_di0_disp0: endpoint {
>                         remote-endpoint = <&sii902x_in>;
>                 };
>         };
>         port@3 {
>                 ipu_di1_tve: endpoint {
>                         remote-endpoint = <&tve_in>;
>                 };
>         };
> };
>
> &sii902x {
>         compatible = "si,sii9022";
>
>         port@0 {
>                 sii902x_in: endpoint {
>                         remote-endpoint = <&ipu_di0>;
>                 };
>         };
>         port@1 {
>                 sii902x_out: endpoint {
>                         remote-endpoint = <&dvi_d_in>;
>                 };
>         };
> };
>
> &tve {
>         compatible = "fsl,imx53-tve";
>         port@0 {
>                 tve_in: endpoint {
>                         remote-endpoint = <&ipu_di1>;
>                 };
>         };
>         port@1 {
>                 tve_out: endpoint {
>                         remote-endpoint = <&dvi_a_in>;
>                 };
>         };
> };
>
> dvi-connector {
>         compatible = "dvi-connector";
>         ddc-i2c-bus = <&i2c3>;
>
>         port {
>                 dvi_d_in: endpoint@0 {
>                         remote-endpoint = <&sii902x_out>;
>                 };
>                 dvi_a_in: endpoint@1 {
>                         remote-endpoint = <&tve_out>;
>                 };
>         };
> };
>
> It should be possible to let the connector know that those two endpoints
> are connected to a TMDS source and to a VGA source, respectively.

I like this proposal over the others. Although, would dual link be a
single endpoint or 2 endpoints? How would you differentiate that?

The port node seems a bit pointless.

Rob
Tomi Valkeinen March 11, 2014, 6:39 a.m. UTC | #17
On 10/03/14 23:45, Rob Herring wrote:

> I like this proposal over the others. Although, would dual link be a

I don't like inferring the information. With the above, you can't find
out that the DVI connector has digital and analog support before all the
drivers are loaded.

> single endpoint or 2 endpoints? How would you differentiate that?

Hmm, well endpoints for a single port are exclusive. So it's either a
single port and a single endpoint, or two ports and two endpoints. I
think dual link has to be single port & endpoint, as the TMDS links need
to be driven together as a single bus.

And dual-link is not really "two links". DVI dual-link means 1 clock
lane and 6 data lanes, compared to 1 clock lane and 3 data lanes for
single-link.

> The port node seems a bit pointless.

There's another thread discussing the ports and endpoints.

The port node represents, for example, the pins for the connection for
that device. And an endpoint-endpoint link represents wires between two
ports.

 Tomi
Tomi Valkeinen March 11, 2014, 6:43 a.m. UTC | #18
On 28/02/14 18:25, Philipp Zabel wrote:

> dvi-connector {
> 	compatible = "dvi-connector";
> 	ddc-i2c-bus = <&i2c3>;
> 
> 	port {
> 		dvi_d_in: endpoint@0 {
> 			remote-endpoint = <&sii902x_out>;
> 		};
> 		dvi_a_in: endpoint@1 {
> 			remote-endpoint = <&tve_out>;
> 		};
> 	};
> };
> 
> It should be possible to let the connector know that those two endpoints
> are connected to a TMDS source and to a VGA source, respectively.

I have not worked with boards that would have the analog part, so just
wondering about the above.

The above would mean that either digital or analog is in use, but not
both. Was that the intention? From the connector's perspective, the
analog and digital pins are separate, and I think they can be used at
the same time. That kind of sounds like the analog and digital pins
should be represented as separate ports.

 Tomi
Geert Uytterhoeven March 11, 2014, 8 a.m. UTC | #19
On Tue, Mar 11, 2014 at 7:39 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 10/03/14 23:45, Rob Herring wrote:
>> I like this proposal over the others. Although, would dual link be a
>
> I don't like inferring the information. With the above, you can't find
> out that the DVI connector has digital and analog support before all the
> drivers are loaded.
>
>> single endpoint or 2 endpoints? How would you differentiate that?
>
> Hmm, well endpoints for a single port are exclusive. So it's either a
> single port and a single endpoint, or two ports and two endpoints. I
> think dual link has to be single port & endpoint, as the TMDS links need
> to be driven together as a single bus.
>
> And dual-link is not really "two links". DVI dual-link means 1 clock
> lane and 6 data lanes, compared to 1 clock lane and 3 data lanes for
> single-link.

What about having a property for the number of data lanes?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Tomi Valkeinen March 11, 2014, 8:04 a.m. UTC | #20
On 11/03/14 10:00, Geert Uytterhoeven wrote:
> On Tue, Mar 11, 2014 at 7:39 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On 10/03/14 23:45, Rob Herring wrote:
>>> I like this proposal over the others. Although, would dual link be a
>>
>> I don't like inferring the information. With the above, you can't find
>> out that the DVI connector has digital and analog support before all the
>> drivers are loaded.
>>
>>> single endpoint or 2 endpoints? How would you differentiate that?
>>
>> Hmm, well endpoints for a single port are exclusive. So it's either a
>> single port and a single endpoint, or two ports and two endpoints. I
>> think dual link has to be single port & endpoint, as the TMDS links need
>> to be driven together as a single bus.
>>
>> And dual-link is not really "two links". DVI dual-link means 1 clock
>> lane and 6 data lanes, compared to 1 clock lane and 3 data lanes for
>> single-link.
> 
> What about having a property for the number of data lanes?

That was already suggested by Philipp in this thread. I don't see
anything wrong with that, but I don't really see benefit either.
"dual-link" is a standard term for 6 data lanes for the DVI connector.
And the choices are 3 or 6 data lanes, nothing else.

 Tomi
Philipp Zabel March 11, 2014, 11:19 a.m. UTC | #21
Am Dienstag, den 11.03.2014, 10:04 +0200 schrieb Tomi Valkeinen:
> On 11/03/14 10:00, Geert Uytterhoeven wrote:
> > On Tue, Mar 11, 2014 at 7:39 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >> On 10/03/14 23:45, Rob Herring wrote:
> >>> I like this proposal over the others. Although, would dual link be a
> >>
> >> I don't like inferring the information. With the above, you can't find
> >> out that the DVI connector has digital and analog support before all the
> >> drivers are loaded.
> >>
> >>> single endpoint or 2 endpoints? How would you differentiate that?
> >>
> >> Hmm, well endpoints for a single port are exclusive. So it's either a
> >> single port and a single endpoint, or two ports and two endpoints. I
> >> think dual link has to be single port & endpoint, as the TMDS links need
> >> to be driven together as a single bus.
> >>
> >> And dual-link is not really "two links". DVI dual-link means 1 clock
> >> lane and 6 data lanes, compared to 1 clock lane and 3 data lanes for
> >> single-link.
> > 
> > What about having a property for the number of data lanes?
> 
> That was already suggested by Philipp in this thread. I don't see
> anything wrong with that, but I don't really see benefit either.
> "dual-link" is a standard term for 6 data lanes for the DVI connector.
> And the choices are 3 or 6 data lanes, nothing else.

The number of lanes of a DisplayPort connector could be 1 to 4. Also,
there's dual-mode DP which can use four lanes to drive
somewhat-like-HDMI single link TMDS signals.

regards
Philipp
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/video/dvi-connector.txt b/Documentation/devicetree/bindings/video/dvi-connector.txt
new file mode 100644
index 000000000000..6a0aff866c78
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/dvi-connector.txt
@@ -0,0 +1,26 @@ 
+DVI Connector
+==============
+
+Required properties:
+- compatible: "dvi-connector"
+
+Optional properties:
+- label: a symbolic name for the connector
+- i2c-bus: phandle to the i2c bus that is connected to DVI DDC
+
+Required nodes:
+- Video port for DVI input
+
+Example
+-------
+
+dvi0: connector@0 {
+	compatible = "dvi-connector";
+	label = "dvi";
+
+	i2c-bus = <&i2c3>;
+
+	dvi_connector_in: endpoint {
+		remote-endpoint = <&tfp410_out>;
+	};
+};