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

Laurent Pinchart
2018-05-14T02:49:28Z

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