[v7,1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

Commit Message

Jacopo Mondi April 6, 2018, 12:41 p.m. UTC

Document Thine THC63LVD1024 LVDS decoder device tree bindings.

Signed-off-by: Jacopo Mondi <[e-mail]>
Reviewed-by: Andrzej Hajda <[e-mail]>
Reviewed-by: Niklas Söderlund <[e-mail]>
Reviewed-by: Laurent Pinchart <[e-mail]>
---
 .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 ++++++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt

--
2.7.4

Comments

Laurent Pinchart April 6, 2018, 1:15 p.m. UTC | #1

Hi Jacopo,

Thank you for the patch.

On Friday, 6 April 2018 15:41:56 EEST Jacopo Mondi wrote:
> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> 
> Signed-off-by: Jacopo Mondi <[e-mail]>
> Reviewed-by: Andrzej Hajda <[e-mail]>
> Reviewed-by: Niklas Söderlund <[e-mail]>
> Reviewed-by: Laurent Pinchart <[e-mail]>
> ---
>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 +++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> 
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> new file mode 100644
> index 0000000..1191f17
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -0,0 +1,60 @@
> +Thine Electronics THC63LVD1024 LVDS decoder
> +-------------------------------------------
> +
> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS
> streams
> +to parallel data outputs. The chip supports single/dual input/output modes,
> +handling up to two two input LVDS stream and up to two digital CMOS/TTL

s/to two two/to two/
s/stream/streams/

> outputs.
> +
> +Single or dual operation modes, output data mapping and DDR output modes
> are
> +configured through input signals and the chip does not expose any control
> bus.
> +
> +Required properties:
> +- compatible: Shall be "thine,thc63lvd1024"
> +
> +Optional properties:
> +- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
> +  PPL and digital circuitry
> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> +- enable-gpios: Output enable GPIO signal, pin name "OE". Active high

As Rob mentioned in a reply to v6, we currently use "enable" as the inverse of 
"powerdown". I would call this one oe-gpios instead. Quoting Rob:

"Debating "oe" vs. "output-enable" is bikeshedding IMO. Anyone familiar with 
h/w design should recognize OE."

> +
> +The THC63LVD1024 video port connections are modeled according
> +to OF graph bindings specified by Documentation/devicetree/bindings/
> graph.txt
> +
> +Required video port nodes:
> +- port@0: First LVDS input port
> +- port@2: First digital CMOS/TTL parallel output
> +
> +Optional video port nodes:
> +- port@1: Second LVDS input port
> +- port@3: Second digital CMOS/TTL parallel output
> +
> +Example:
> +--------
> +
> +	thc63lvd1024: lvds-decoder {
> +		compatible = "thine,thc63lvd1024";
> +
> +		vcc-supply = <&reg_lvds_vcc>;
> +		powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +
> +				lvds_dec_in_0: endpoint {
> +					remote-endpoint = <&lvds_out>;
> +				};
> +			};
> +
> +			port@2{
> +				reg = <2>;
> +
> +				lvds_dec_out_2: endpoint {
> +					remote-endpoint = <&adv7511_in>;
> +				};
> +			};
> +		};
> +	};

Jacopo Mondi April 6, 2018, 2:25 p.m. UTC | #2

Hi Laurent,

On Fri, Apr 06, 2018 at 04:15:35PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Friday, 6 April 2018 15:41:56 EEST Jacopo Mondi wrote:
> > Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> >
> > Signed-off-by: Jacopo Mondi <[e-mail]>
> > Reviewed-by: Andrzej Hajda <[e-mail]>
> > Reviewed-by: Niklas Söderlund <[e-mail]>
> > Reviewed-by: Laurent Pinchart <[e-mail]>
> > ---
> >  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 +++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > new file mode 100644
> > index 0000000..1191f17
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > @@ -0,0 +1,60 @@
> > +Thine Electronics THC63LVD1024 LVDS decoder
> > +-------------------------------------------
> > +
> > +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS
> > streams
> > +to parallel data outputs. The chip supports single/dual input/output modes,
> > +handling up to two two input LVDS stream and up to two digital CMOS/TTL
>
> s/to two two/to two/
> s/stream/streams/
>
> > outputs.
> > +
> > +Single or dual operation modes, output data mapping and DDR output modes
> > are
> > +configured through input signals and the chip does not expose any control
> > bus.
> > +
> > +Required properties:
> > +- compatible: Shall be "thine,thc63lvd1024"
> > +
> > +Optional properties:
> > +- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
> > +  PPL and digital circuitry
> > +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> > +- enable-gpios: Output enable GPIO signal, pin name "OE". Active high
>
> As Rob mentioned in a reply to v6, we currently use "enable" as the inverse of
> "powerdown". I would call this one oe-gpios instead. Quoting Rob:
>
> "Debating "oe" vs. "output-enable" is bikeshedding IMO. Anyone familiar with
> h/w design should recognize OE."
>

I got a different understanding of what Rob meant. I thought "anyone
familiar with h/w design should recognize OE" as that nobody would get
confused if a pin named OE in the chip manual is descibed by an
'enable' property.

But as discussed offline, enable has probably to be used as the
opposite of powerdown for complete chip sleep, not just for output
pad.

Anyway, we spent enough time on naming issues, starting from my first
stupid 'pdwn' permutations then on this semi-standard names.

I'll send next version with 'powerdown-gpios' and 'oe-gpios'
properties hoping that would be finally accepted by everyone.

Same on the mandatory/optional VCC supply thing. Let's try to make
next version the final one. If the optional property with the dummy
regulator doesn't satisfy you and it is preferred to have a fixed-regulator
anyhow in DT I'll do in next version, othewise let's try not to change
it again. I'll just remark here that in the current Eagle design vcc is
connected to a power rail with no regulator at all :)

Thanks
   j

> > +
> > +The THC63LVD1024 video port connections are modeled according
> > +to OF graph bindings specified by Documentation/devicetree/bindings/
> > graph.txt
> > +
> > +Required video port nodes:
> > +- port@0: First LVDS input port
> > +- port@2: First digital CMOS/TTL parallel output
> > +
> > +Optional video port nodes:
> > +- port@1: Second LVDS input port
> > +- port@3: Second digital CMOS/TTL parallel output
> > +
> > +Example:
> > +--------
> > +
> > +	thc63lvd1024: lvds-decoder {
> > +		compatible = "thine,thc63lvd1024";
> > +
> > +		vcc-supply = <&reg_lvds_vcc>;
> > +		powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
> > +
> > +		ports {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			port@0 {
> > +				reg = <0>;
> > +
> > +				lvds_dec_in_0: endpoint {
> > +					remote-endpoint = <&lvds_out>;
> > +				};
> > +			};
> > +
> > +			port@2{
> > +				reg = <2>;
> > +
> > +				lvds_dec_out_2: endpoint {
> > +					remote-endpoint = <&adv7511_in>;
> > +				};
> > +			};
> > +		};
> > +	};
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

Laurent Pinchart April 6, 2018, 3:40 p.m. UTC | #3

Hi Jacopo,

(CC'ing Mark Brown)

On Friday, 6 April 2018 17:25:58 EEST jacopo mondi wrote:
> On Fri, Apr 06, 2018 at 04:15:35PM +0300, Laurent Pinchart wrote:
> > On Friday, 6 April 2018 15:41:56 EEST Jacopo Mondi wrote:
> >> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> >> 
> >> Signed-off-by: Jacopo Mondi <[e-mail]>
> >> Reviewed-by: Andrzej Hajda <[e-mail]>
> >> Reviewed-by: Niklas Söderlund <[e-mail]>
> >> Reviewed-by: Laurent Pinchart <[e-mail]>
> >> ---
> >> 
> >>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60
> >>  +++++++++++++++++++
> >>  1 file changed, 60 insertions(+)
> >>  create mode 100644
> >> 
> >> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.tx
> >> t
> >> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.tx
> >> t
> >> new file mode 100644
> >> index 0000000..1191f17
> >> --- /dev/null
> >> +++
> >> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.tx
> >> t
> >> @@ -0,0 +1,60 @@
> >> +Thine Electronics THC63LVD1024 LVDS decoder
> >> +-------------------------------------------
> >> +
> >> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS
> >> streams
> >> +to parallel data outputs. The chip supports single/dual input/output
> >> modes,
> >> +handling up to two two input LVDS stream and up to two digital CMOS/TTL
> > 
> > s/to two two/to two/
> > s/stream/streams/
> > 
> >> outputs.
> >> +
> >> +Single or dual operation modes, output data mapping and DDR output
> >> modes are
> >> +configured through input signals and the chip does not expose any
> >> control bus.
> >> +
> >> +Required properties:
> >> +- compatible: Shall be "thine,thc63lvd1024"
> >> +
> >> +Optional properties:
> >> +- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS
> >> input,
> >> +  PPL and digital circuitry
> >> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> >> +- enable-gpios: Output enable GPIO signal, pin name "OE". Active high
> > 
> > As Rob mentioned in a reply to v6, we currently use "enable" as the
> > inverse of "powerdown". I would call this one oe-gpios instead. Quoting
> > Rob:
> > 
> > "Debating "oe" vs. "output-enable" is bikeshedding IMO. Anyone familiar
> > with h/w design should recognize OE."
> 
> I got a different understanding of what Rob meant. I thought "anyone
> familiar with h/w design should recognize OE" as that nobody would get
> confused if a pin named OE in the chip manual is descibed by an
> 'enable' property.
> 
> But as discussed offline, enable has probably to be used as the
> opposite of powerdown for complete chip sleep, not just for output
> pad.
> 
> Anyway, we spent enough time on naming issues, starting from my first
> stupid 'pdwn' permutations then on this semi-standard names.
> 
> I'll send next version with 'powerdown-gpios' and 'oe-gpios'
> properties hoping that would be finally accepted by everyone.

I certainly won't complain (as long as you write pwdn instead of pdwn in the 
driver :-)).

> Same on the mandatory/optional VCC supply thing. Let's try to make
> next version the final one. If the optional property with the dummy
> regulator doesn't satisfy you and it is preferred to have a fixed-regulator
> anyhow in DT I'll do in next version, othewise let's try not to change
> it again. I'll just remark here that in the current Eagle design vcc is
> connected to a power rail with no regulator at all :)

I don't like the dummy regulator much, as it generates a dev_warn(), which 
makes me believe that it's a hack rather than a proper solution. You might 
want to ask Mark Brown for his opinion.

> >> +
> >> +The THC63LVD1024 video port connections are modeled according
> >> +to OF graph bindings specified by Documentation/devicetree/bindings/
> >> graph.txt
> >> +
> >> +Required video port nodes:
> >> +- port@0: First LVDS input port
> >> +- port@2: First digital CMOS/TTL parallel output
> >> +
> >> +Optional video port nodes:
> >> +- port@1: Second LVDS input port
> >> +- port@3: Second digital CMOS/TTL parallel output
> >> +
> >> +Example:
> >> +--------
> >> +
> >> +	thc63lvd1024: lvds-decoder {
> >> +		compatible = "thine,thc63lvd1024";
> >> +
> >> +		vcc-supply = <&reg_lvds_vcc>;
> >> +		powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
> >> +
> >> +		ports {
> >> +			#address-cells = <1>;
> >> +			#size-cells = <0>;
> >> +
> >> +			port@0 {
> >> +				reg = <0>;
> >> +
> >> +				lvds_dec_in_0: endpoint {
> >> +					remote-endpoint = <&lvds_out>;
> >> +				};
> >> +			};
> >> +
> >> +			port@2{
> >> +				reg = <2>;
> >> +
> >> +				lvds_dec_out_2: endpoint {
> >> +					remote-endpoint = <&adv7511_in>;
> >> +				};
> >> +			};
> >> +		};
> >> +	};

Jacopo Mondi April 7, 2018, 9:45 a.m. UTC | #4

Hi Laurent


On Fri, Apr 06, 2018 at 06:40:14PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> (CC'ing Mark Brown)

Hi Mark

[snip]
> >
> > Anyway, we spent enough time on naming issues, starting from my first
> > stupid 'pdwn' permutations then on this semi-standard names.
> >
> > I'll send next version with 'powerdown-gpios' and 'oe-gpios'
> > properties hoping that would be finally accepted by everyone.
>
> I certainly won't complain (as long as you write pwdn instead of pdwn in the
> driver :-)).
>

Oh, so you won't complain if I address your comments? Thank you! :D
By the way, the dumb pdwn name comes, again, from the chip name. I can
change it to a saner name for sure...

> > Same on the mandatory/optional VCC supply thing. Let's try to make
> > next version the final one. If the optional property with the dummy
> > regulator doesn't satisfy you and it is preferred to have a fixed-regulator
> > anyhow in DT I'll do in next version, othewise let's try not to change
> > it again. I'll just remark here that in the current Eagle design vcc is
> > connected to a power rail with no regulator at all :)
>
> I don't like the dummy regulator much, as it generates a dev_warn(), which
> makes me believe that it's a hack rather than a proper solution. You might
> want to ask Mark Brown for his opinion.
>

Sure: Hi Mark, a bit of context here to save you a long(er) reading.

Unsurprisingly, the chip for which I'm writing this small driver needs
a power supply to be properly functional :) In the development board
it is installed on, the power supply is connected to a power rail,
with no regulator. At the same time, some other designs may instead
include a regulator. I assume that's a very common situation. I
started by describing the regulator as optional in DT bindings, and
use regulator_get_optional(). If that function returns PTR_ERR, the
regulator is then ignored in driver's power management routines.

In this last version I thought I was acting smart and copied what happens
in other DRM bridges like adv7511, which use 'regulator_get()' and work
with a dummy if no regulator is provided in DT. Laurent has the above
doubts on using a dummy, and I actually share some of his concerns
(that dev_warn() is how I found out adv7511 was using a dummy, actually).

To sum up: when a regulator is described as optional in DT, do you
suggest to work with a dummy if it's not there, or use the _optional()
version of regulator_get() and manually set it to NULL and ignore it
in the enable/disable driver's routines?

Bonus question: Laurent likes to have the regulator described as required,
and thus require it to be described in DT also in cases where it is not
actually there using a 'fixed-regulator' and refuse to probe if the regulator
is not available. Do you encourage this approach, or prefer to have it
optional and handle it in the driver in one of the above described
ways?

Thank you
    j

Laurent Pinchart April 7, 2018, 9:56 a.m. UTC | #5

Hi Jacopo,

On Saturday, 7 April 2018 12:45:56 EEST jacopo mondi wrote:
> On Fri, Apr 06, 2018 at 06:40:14PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> > 
> > (CC'ing Mark Brown)
> 
> Hi Mark
> 
> [snip]
> 
> >> Anyway, we spent enough time on naming issues, starting from my first
> >> stupid 'pdwn' permutations then on this semi-standard names.
> >> 
> >> I'll send next version with 'powerdown-gpios' and 'oe-gpios'
> >> properties hoping that would be finally accepted by everyone.
> > 
> > I certainly won't complain (as long as you write pwdn instead of pdwn in
> > the driver :-)).
> 
> Oh, so you won't complain if I address your comments? Thank you! :D
> By the way, the dumb pdwn name comes, again, from the chip name. I can
> change it to a saner name for sure...

And I've just realized that, I thought it was a typo :-/ If it comes with the 
datasheet I'm fine with either.

> >> Same on the mandatory/optional VCC supply thing. Let's try to make
> >> next version the final one. If the optional property with the dummy
> >> regulator doesn't satisfy you and it is preferred to have a
> >> fixed-regulator anyhow in DT I'll do in next version, othewise let's try
> >> not to change it again. I'll just remark here that in the current Eagle
> >> design vcc is connected to a power rail with no regulator at all :)
> > 
> > I don't like the dummy regulator much, as it generates a dev_warn(), which
> > makes me believe that it's a hack rather than a proper solution. You might
> > want to ask Mark Brown for his opinion.
> 
> Sure: Hi Mark, a bit of context here to save you a long(er) reading.
> 
> Unsurprisingly, the chip for which I'm writing this small driver needs
> a power supply to be properly functional :) In the development board
> it is installed on, the power supply is connected to a power rail,
> with no regulator. At the same time, some other designs may instead
> include a regulator.

To be precise, with an always-on regulator that can't be software-controlled.

> I assume that's a very common situation. I started by describing the
> regulator as optional in DT bindings, and use regulator_get_optional(). If
> that function returns PTR_ERR, the regulator is then ignored in driver's
> power management routines.
> 
> In this last version I thought I was acting smart and copied what happens
> in other DRM bridges like adv7511, which use 'regulator_get()' and work
> with a dummy if no regulator is provided in DT. Laurent has the above
> doubts on using a dummy, and I actually share some of his concerns
> (that dev_warn() is how I found out adv7511 was using a dummy, actually).
> 
> To sum up: when a regulator is described as optional in DT, do you
> suggest to work with a dummy if it's not there, or use the _optional()
> version of regulator_get() and manually set it to NULL and ignore it
> in the enable/disable driver's routines?
> 
> Bonus question: Laurent likes to have the regulator described as required,
> and thus require it to be described in DT also in cases where it is not
> actually there using a 'fixed-regulator' and refuse to probe if the
> regulator is not available. Do you encourage this approach, or prefer to
> have it optional and handle it in the driver in one of the above described
> ways?

Mark Brown April 9, 2018, 10:22 a.m. UTC | #6

On Fri, Apr 06, 2018 at 06:40:14PM +0300, Laurent Pinchart wrote:
> On Friday, 6 April 2018 17:25:58 EEST jacopo mondi wrote:

> > Same on the mandatory/optional VCC supply thing. Let's try to make
> > next version the final one. If the optional property with the dummy
> > regulator doesn't satisfy you and it is preferred to have a fixed-regulator
> > anyhow in DT I'll do in next version, othewise let's try not to change
> > it again. I'll just remark here that in the current Eagle design vcc is
> > connected to a power rail with no regulator at all :)

> I don't like the dummy regulator much, as it generates a dev_warn(), which 
> makes me believe that it's a hack rather than a proper solution. You might 
> want to ask Mark Brown for his opinion.

The DT should describe the physical supplies the device needs, if that's
a fixed regulator with no software control then the DT should have a
fixed regulator with no software control.

Patch

diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
new file mode 100644
index 0000000..1191f17
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
@@ -0,0 +1,60 @@ 
+Thine Electronics THC63LVD1024 LVDS decoder
+-------------------------------------------
+
+The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS streams
+to parallel data outputs. The chip supports single/dual input/output modes,
+handling up to two two input LVDS stream and up to two digital CMOS/TTL outputs.
+
+Single or dual operation modes, output data mapping and DDR output modes are
+configured through input signals and the chip does not expose any control bus.
+
+Required properties:
+- compatible: Shall be "thine,thc63lvd1024"
+
+Optional properties:
+- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
+  PPL and digital circuitry
+- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
+- enable-gpios: Output enable GPIO signal, pin name "OE". Active high
+
+The THC63LVD1024 video port connections are modeled according
+to OF graph bindings specified by Documentation/devicetree/bindings/graph.txt
+
+Required video port nodes:
+- port@0: First LVDS input port
+- port@2: First digital CMOS/TTL parallel output
+
+Optional video port nodes:
+- port@1: Second LVDS input port
+- port@3: Second digital CMOS/TTL parallel output
+
+Example:
+--------
+
+	thc63lvd1024: lvds-decoder {
+		compatible = "thine,thc63lvd1024";
+
+		vcc-supply = <&reg_lvds_vcc>;
+		powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+
+				lvds_dec_in_0: endpoint {
+					remote-endpoint = <&lvds_out>;
+				};
+			};
+
+			port@2{
+				reg = <2>;
+
+				lvds_dec_out_2: endpoint {
+					remote-endpoint = <&adv7511_in>;
+				};
+			};
+		};
+	};