Re: [PATCH 3/3] arm64: dts: renesas: draak: Describe HDMI input

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello,

On Sunday, 13 May 2018 15:57:55 EEST Niklas Söderlund wrote:
> On 2018-05-11 12:00:02 +0200, Jacopo Mondi wrote:
> > Describe HDMI input connected to VIN4 interface for R-Car D3 Draak
> > development board.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> > 
> >  arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 68 +++++++++++++++++++++
> >  1 file changed, 68 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> > b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts index d03f194..e0ce462
> > 100644
> > --- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> > +++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> > @@ -59,6 +59,17 @@
> > 
> >  		};
> >  	
> >  	};
> > 
> > +	hdmi-in {
> > +		compatible = "hdmi-connector";
> > +		type = "a";
> > +
> > +		port {
> > +			hdmi_con_in: endpoint {
> > +				remote-endpoint = <&adv7612_in>;
> > +			};
> > +		};
> > +	};
> > +
> > 
> >  	memory@48000000 {
> >  	
> >  		device_type = "memory";
> >  		/* first 128MB is reserved for secure area. */
> > 
> > @@ -142,6 +153,11 @@
> > 
> >  		groups = "usb0";
> >  		function = "usb0";
> >  	
> >  	};
> > 
> > +
> > +	vin4_pins: vin4 {
> > +		groups = "vin4_data24", "vin4_sync", "vin4_clk", "vin4_clkenb";
> > +		function = "vin4";
> > +	};
> > 
> >  };
> >  
> >  &i2c0 {
> > 
> > @@ -154,6 +170,35 @@
> > 
> >  		reg = <0x50>;
> >  		pagesize = <8>;
> >  	
> >  	};
> > 
> > +
> > +	hdmi-decoder@4c {
> > +		compatible = "adi,adv7612";
> > +		reg = <0x4c>;
> > +		default-input = <0>;
> > +
> > +		ports {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			port@0 {
> > +				reg = <0>;
> > +				adv7612_in: endpoint {
> > +					remote-endpoint = <&hdmi_con_in>;
> > +				};
> > +			};
> > +
> > +			port@2 {
> > +				reg = <2>;
> > +				adv7612_out: endpoint {
> > +					pclk-sample = <0>;
> > +					hsync-active = <0>;
> > +					vsync-active = <0>;
> 
> This differs from the Gen2 DT bindings which is a very similar hardware
> setup using the same components. Defining these properties will make the
> bus marked as V4L2_MBUS_PARALLEL instead of V4L2_MBUS_BT656.
> 
> This will change how the hardware is configured for capture if the media
> bus is in a UYVY format, see VNMC_INF register in rvin_setup(). Maybe
> this it not an issue here but still I'm curious to why this differ
> between Gen2 and Gen3 :-)
> 
> > +
> > +					remote-endpoint = <&vin4_in>;
> > +				};
> > +			};
> > +		};
> > +	};
> > 
> >  };
> >  
> >  &i2c1 {
> > 
> > @@ -246,3 +291,26 @@
> > 
> >  	timeout-sec = <60>;
> >  	status = "okay";
> >  
> >  };
> > 
> > +
> > +&vin4 {
> > +	pinctrl-0 = <&vin4_pins>;
> > +	pinctrl-names = "default";
> > +
> > +	status = "okay";
> > +
> > +	ports {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		port@0 {
> > +			reg = <0>;
> > +
> > +			vin4_in: endpoint {
> > +				hsync-active = <0>;
> > +				vsync-active = <0>;
> 
> Comparing this to the Gen2 bindings some properties are missing,
> 
> bus-width = <24>;
> pclk-sample = <1>;
> data-active = <1>;
> 
> This is not a big deal as the VIN driver don't use these properties so
> no functional change should come of this but still a difference.

I think the VIN DT bindings should be updated to explicitly list the endpoint 
properties that are mandatory, optional, or not allowed.

> Over all I'm happy with this change but before I add my tag I would like
> to understand why it differs from the Gen2 configuration for the adv7612
> properties.
> 
> Also on a side not it is possible with hardware switches on the board
> switch the VIN4 source to a completely different pipeline CVBS connector
> -> adv7180 -> VIN4. But I think it's best we keep the HDMI as default as
> this seems to be how the boards are shipped. But maybe mentioning this
> in the commit message would not hurt if you end-up resending the patch.
> 
> > +
> > +				remote-endpoint = <&adv7612_out>;
> > +			};
> > +		};
> > +	};
> > +};

-- 
Regards,

Laurent Pinchart