Re: [PATCH v3 2/2] arm64: dts: renesas: condor/v3hsk: add DU/LVDS/HDMI support

[Simon Horman <horms@verge.net.au>]

On Mon, Aug 13, 2018 at 06:19:20PM +0300, Laurent Pinchart wrote:
> Hi Sergei,
> 
> Thank you for the patch.
> 
> On Wednesday, 13 June 2018 23:12:40 EEST Sergei Shtylyov wrote:
> > Define the Condor/V3HSK board dependent parts of the DU and  LVDS device
> > nodes. Also add the device nodes for Thine THC63LVD1024 LVDS decoder and
> > Analog Devices ADV7511W HDMI transmitter...
> > 
> > Based on the original (and large) patch by Vladimir Barinov.
> > 
> > Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
> > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> > 
> > ---
> > Changes in version 2:
> > - added the V3HSK DT update, reworded the description, renamed the patch;
> > - added a space between the HDMI node name and a brace.
> > 
> >  arch/arm64/boot/dts/renesas/r8a77980-condor.dts |  106 ++++++++++++++++++++
> >  arch/arm64/boot/dts/renesas/r8a77980-v3hsk.dts  |  120 ++++++++++++++++++++
> >  2 files changed, 226 insertions(+)
> 
> I would have split that in two patchees.

I take your point but I think one is fine.

Sergei, will you address the other review items?

> 
> > 
> > Index: renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
> > ===================================================================
> > --- renesas.orig/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
> > +++ renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
> > @@ -45,6 +45,56 @@
> >  		regulator-boot-on;
> >  		regulator-always-on;
> >  	};
> > +
> > +	d1_8v: regulator-2 {
> 
> Nitpicking, the naming for the regulator and clock nodes seems a bit weird. 
> That shouldn't block this patch, but the issue should still be discussed with 
> DT maintainers. A clear rule should be enacted on how to name top level nodes 
> for which no register value makes sense.
> 
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "D1.8V";
> > +		regulator-min-microvolt = <1800000>;
> > +		regulator-max-microvolt = <1800000>;
> > +		regulator-boot-on;
> > +		regulator-always-on;
> > +	};
> > +
> > +	hdmi-out {
> > +		compatible = "hdmi-connector";
> > +		type = "a";
> > +
> > +		port {
> > +			hdmi_con: endpoint {
> > +				remote-endpoint = <&adv7511_out>;
> > +			};
> > +		};
> > +	};
> > +
> > +	lvds-decoder {
> > +		compatible = "thine,thc63lvd1024";
> > +		vcc-supply = <&d3_3v>;
> > +
> > +		ports {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			port@0 {
> > +				reg = <0>;
> > +				thc63lvd1024_in: endpoint {
> > +					remote-endpoint = <&lvds0_out>;
> > +				};
> > +			};
> > +
> > +			port@2 {
> > +				reg = <2>;
> > +				thc63lvd1024_out: endpoint {
> > +					remote-endpoint = <&adv7511_in>;
> > +				};
> > +			};
> > +		};
> > +	};
> > +
> > +	x1_clk: x1-clock {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <148500000>;
> > +	};
> >  };
> > 
> >  &avb {
> > @@ -74,6 +124,13 @@
> >  	};
> >  };
> > 
> > +&du {
> > +	clocks = <&cpg CPG_MOD 724>,
> > +		 <&x1_clk>;
> > +	clock-names = "du.0", "dclkin.0";
> > +	status = "okay";
> > +};
> > +
> >  &extal_clk {
> >  	clock-frequency = <16666666>;
> >  };
> > @@ -102,6 +159,55 @@
> >  		gpio-controller;
> >  		#gpio-cells = <2>;
> >  	};
> > +
> > +	hdmi@39 {
> > +		compatible = "adi,adv7511w";
> > +		reg = <0x39>;
> > +		interrupt-parent = <&gpio1>;
> > +		interrupts = <20 IRQ_TYPE_LEVEL_LOW>;
> > +		avdd-supply = <&d1_8v>;
> > +		dvdd-supply = <&d1_8v>;
> > +		pvdd-supply = <&d1_8v>;
> > +		bgvdd-supply = <&d1_8v>;
> > +		dvdd-3v-supply = <&d3_3v>;
> > +
> > +		adi,input-depth = <8>;
> > +		adi,input-colorspace = "rgb";
> > +		adi,input-clock = "1x";
> > +		adi,input-style = <1>;
> > +		adi,input-justification = "evenly";
> > +
> > +		ports {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			port@0 {
> > +				reg = <0>;
> > +				adv7511_in: endpoint {
> > +					remote-endpoint = <&thc63lvd1024_out>;
> > +				};
> > +			};
> > +
> > +			port@1 {
> > +				reg = <1>;
> > +				adv7511_out: endpoint {
> > +					remote-endpoint = <&hdmi_con>;
> > +				};
> > +			};
> > +		};
> > +	};
> > +};
> > +
> > +&lvds0 {
> > +	status = "okay";
> > +
> > +	ports {
> > +		port@1 {
> > +			lvds0_out: endpoint {
> > +				remote-endpoint = <&thc63lvd1024_in>;
> > +			};
> > +		};
> > +	};
> >  };
> > 
> >  &mmc0 {
> > Index: renesas/arch/arm64/boot/dts/renesas/r8a77980-v3hsk.dts
> > ===================================================================
> > --- renesas.orig/arch/arm64/boot/dts/renesas/r8a77980-v3hsk.dts
> > +++ renesas/arch/arm64/boot/dts/renesas/r8a77980-v3hsk.dts
> > @@ -27,6 +27,63 @@
> >  		/* first 128MB is reserved for secure area. */
> >  		reg = <0 0x48000000 0 0x78000000>;
> >  	};
> > +
> > +	hdmi-out {
> > +		compatible = "hdmi-connector";
> > +		type = "a";
> > +
> > +		port {
> > +			hdmi_con: endpoint {
> > +				remote-endpoint = <&adv7511_out>;
> > +			};
> > +		};
> > +	};
> > +
> > +	lvds-decoder {
> > +		compatible = "thine,thc63lvd1024";
> > +		vcc-supply = <&vcc3v3_d5>;
> > +
> > +		ports {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			port@0 {
> > +				reg = <0>;
> > +				thc63lvd1024_in: endpoint {
> > +					remote-endpoint = <&lvds0_out>;
> > +				};
> > +			};
> > +
> > +			port@2 {
> > +				reg = <2>;
> > +				thc63lvd1024_out: endpoint {
> > +					remote-endpoint = <&adv7511_in>;
> > +				};
> > +			};
> > +		};
> > +	};
> > +
> > +	vcc1v8_d4: regulator-0 {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "VCC1V8_D4";
> > +		regulator-min-microvolt = <1800000>;
> > +		regulator-max-microvolt = <1800000>;
> > +		regulator-boot-on;
> > +		regulator-always-on;
> > +	};
> > +
> > +	vcc3v3_d5: regulator-1 {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "VCC3V3_D5";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		regulator-boot-on;
> > +		regulator-always-on;
> > +	};
> > +};
> > +
> > +&du {
> > +	status = "okay";
> 
> No dot clock for the DU ?
> 
> Apart from that,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >  };
> > 
> >  &extal_clk {
> > @@ -53,6 +110,64 @@
> >  	};
> >  };
> > 
> > +&lvds0 {
> > +	status = "okay";
> > +
> > +	ports {
> > +		port@1 {
> > +			lvds0_out: endpoint {
> > +				remote-endpoint = <&thc63lvd1024_in>;
> > +			};
> > +		};
> > +	};
> > +};
> > +
> > +&i2c0 {
> > +	pinctrl-0 = <&i2c0_pins>;
> > +	pinctrl-names = "default";
> > +
> > +	status = "okay";
> > +	clock-frequency = <400000>;
> > +
> > +	hdmi@39 {
> > +		compatible = "adi,adv7511w";
> > +		#sound-dai-cells = <0>;
> > +		reg = <0x39>;
> > +		interrupt-parent = <&gpio1>;
> > +		interrupts = <20 IRQ_TYPE_LEVEL_LOW>;
> > +		avdd-supply = <&vcc1v8_d4>;
> > +		dvdd-supply = <&vcc1v8_d4>;
> > +		pvdd-supply = <&vcc1v8_d4>;
> > +		bgvdd-supply = <&vcc1v8_d4>;
> > +		dvdd-3v-supply = <&vcc3v3_d5>;
> > +
> > +		adi,input-depth = <8>;
> > +		adi,input-colorspace = "rgb";
> > +		adi,input-clock = "1x";
> > +		adi,input-style = <1>;
> > +		adi,input-justification = "evenly";
> > +
> > +		ports {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			port@0 {
> > +				reg = <0>;
> > +				adv7511_in: endpoint {
> > +					remote-endpoint = <&thc63lvd1024_out>;
> > +				};
> > +			};
> > +
> > +			port@1 {
> > +				reg = <1>;
> > +				adv7511_out: endpoint {
> > +					remote-endpoint = <&hdmi_con>;
> > +				};
> > +			};
> > +		};
> > +	};
> > +};
> > +
> >  &pfc {
> >  	gether_pins: gether {
> >  		groups = "gether_mdio_a", "gether_rgmii",
> > @@ -60,6 +175,11 @@
> >  		function = "gether";
> >  	};
> > 
> > +	i2c0_pins: i2c0 {
> > +		groups = "i2c0";
> > +		function = "i2c0";
> > +	};
> > +
> >  	scif0_pins: scif0 {
> >  		groups = "scif0_data";
> >  		function = "scif0";
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
>