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

[Laurent Pinchart]

Hi Niklas,

On Monday, 14 May 2018 13:23:26 EEST Niklas Söderlund wrote:
> On 2018-05-14 09:39:34 +0200, Jacopo Mondi wrote:
> > On Sun, May 13, 2018 at 02:57:55PM +0200, 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@xxxxxxxxxx>
> >>> ---
> >>> 
> >>>  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

[snip]

> >>> +&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>;
> > 
> > The VIN driver does not parse them, so there is no value in having
> > them there, if not confusing people as it happened to me reading the
> > Gen2 DT.
> 
> I have no objection removing them. Trying to understand why the
> description differed from Gen2.
> 
> >> 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.
> > 
> > Exactly.
> > 
> > On a side note. I have not seen a way to configure the pixel clock
> > sampling level in the interface datasheet. The register used to
> > configure synchronism signals polarities is VnDMR2, and there I read
> > we can configure HSYNC/VSYNC and CLOCKENB (which is data enable, not
> > pixel clock) polarities. Is it configured through some other
> > register?
> 
> I have not seen such a register no.
> 
> >> 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.
> > 
> > Oh I see. SW-49 to SW-52 enables the HDMI input, SW53-SW54 CVBS one.
> > And actually, reading the 'initial setting of slide switches' in the
> > Draak board manual, it turns out that the board default configuration
> > is with CVBS input selected... What should we do here? reflect
> > defaults in the DT, or prioritize HDMI?
> 
> I feel this is a question for Laurent. My feeling for how we handled
> this in other cases is to go with the board default settings. I'm
> however sure there are exceptions to the rule. So maybe we should go
> with the most useful (what ever that is) configuration?

I think I'd go with CVBS as I don't think HDMI would be considered as the most 
useful configuration here. The Draak board is unlikely to be used by us as a 
reference platform to test HDMI capture, is it ?

This being said, you can instantiate the adv7612 and HDMI connector in DT, 
without connecting them to the VIN. That would make it easy to quickly change 
the configuration.

> >>> +
> >>> +				remote-endpoint = <&adv7612_out>;
> >>> +			};
> >>> +		};
> >>> +	};
> >>> +};

-- 
Regards,

Laurent Pinchart