lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <3381435.LMORAGhIb5@avalon>
Date:   Tue, 27 Jun 2017 16:57:10 +0300
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     kieran.bingham@...asonboard.com
Cc:     Kieran Bingham <kbingham@...nel.org>,
        Niklas Söderlund 
        <niklas.soderlund@...natech.se>, linux-renesas-soc@...r.kernel.org,
        linux-kernel@...r.kernel.org, Simon Horman <horms@...ge.net.au>,
        Magnus Damm <magnus.damm@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        "moderated list:ARM64 PORT (AARCH64 ARCHITECTURE)" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v5 2/2] arm64: dts: renesas: salvator-x: Add ADV7482 support

Hi Kieran,

On Tuesday 27 Jun 2017 10:25:39 Kieran Bingham wrote:
> On 23/06/17 08:43, Laurent Pinchart wrote:
> > On Wednesday 14 Jun 2017 20:58:13 Kieran Bingham wrote:
> >> From: Kieran Bingham <kieran.bingham+renesas@...asonboard.com>
> >> 
> >> The Salvator boards use an ADV7482 receiver for HDMI and CVBS inputs.
> >> 
> >> Provide ADV7482 node on the i2c4 bus, along with connectors for the
> >> hdmi and cvbs inputs, and link to the csi20 and csi40 nodes as outputs.
> >> 
> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@...asonboard.com>
> >> 
> >> v4:
> >>  - dt: Rebase to dts/renesas/salvator-x.dtsi
> >>  - dt: Use AIN0-7 rather than AIN1-8
> >> 
> >> v5:
> >>  - dt: Move to salvator-common.dtsi
> >> 
> >> ---
> >> 
> >>  arch/arm64/boot/dts/renesas/salvator-common.dtsi | 123 ++++++++++++++++-
> >>  1 file changed, 123 insertions(+)
> >> 
> >> diff --git a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> >> b/arch/arm64/boot/dts/renesas/salvator-common.dtsi index
> >> aef35e0b685a..d74fb55c38bc 100644
> >> --- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> >> +++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> >> @@ -65,6 +65,27 @@
> >>  		enable-gpios = <&gpio6 7 GPIO_ACTIVE_HIGH>;
> >>  	};
> >> 
> >> +	cvbs-in {
> >> +		compatible = "composite-video-connector";
> >> +		label = "CVBS IN";
> >> +
> >> +		port {
> >> +			cvbs_con: endpoint {
> >> +			};
> >> +		};
> >> +	};
> >> +
> >> +	hdmi-in {
> >> +		compatible = "hdmi-connector";
> >> +		label = "HDMI IN";
> >> +		type = "a";
> >> +
> >> +		port {
> >> +			hdmi_in_con: endpoint {
> >> +			};
> >> +		};
> >> +	};
> >> +
> >>  	reg_1p8v: regulator0 {
> >>  		compatible = "regulator-fixed";
> >>  		regulator-name = "fixed-1.8V";
> >> @@ -257,6 +278,59 @@
> >>  	};
> >>  };
> >> 
> >> +&csi20 {
> >> +	status = "okay";
> >> +
> >> +	ports {
> >> +		#address-cells = <1>;
> >> +		#size-cells = <0>;
> >> +
> >> +		port@0 {
> >> +			reg = <0>;
> > 
> > Aren't #address-cells, #size-cells and reg already present in the SoC
> > .dtsi ? And if they're not, shouldn't they be ? :-)
> 
> Hrm - I think I added them because I was getting warnings from the DTC.
> 
> I've removed them from the csi nodes and no warning returns :)
> So they're gone.

You need the #address-cells and #size-cells properties in the ports node, but 
as they're already added by the SoC .dtsi, you don't need to duplicate them 
here.

> >> +			csi20_in: endpoint {
> >> +				clock-lanes = <0>;
> >> +				data-lanes = <1>;
> >> +				remote-endpoint = <&adv7482_txb>;
> >> +			};
> >> +		};
> >> +	};
> >> +};
> >> +
> >> +&csi40 {
> >> +	status = "okay";
> >> +
> >> +	ports {
> >> +		#address-cells = <1>;
> >> +		#size-cells = <0>;
> 
> # Removed
> 
> >> +
> >> +		port@0 {
> >> +			reg = <0>;
> >> +
> >> +			csi40_in: endpoint {
> >> +				clock-lanes = <0>;
> >> +				data-lanes = <1 2 3 4>;
> >> +				remote-endpoint = <&adv7482_txa>;
> >> +			};
> >> +		};
> >> +	};
> >> +};
> >> +
> >> +&cvbs_con {
> >> +	port {
> >> +		cvbs_in: endpoint {
> >> +			remote-endpoint = <&adv7482_ain7>;
> >> +		};
> >> +	};
> >> +};
> > 
> > You could merge this with the cvbs_con node above.
> 
> What is the distinction that means cvbs_con can be merged where hdmi_in_con
> would not ?

No reason, the same comment applies to HDMI too :-)

> Similarly, I see that VGA out is 'merged' where hdmi outs are not ... is it
> just determined by whether the connector is on all boards that inherit
> salvator-common.dtsi ?

My point was that it seems a bit pointless to define a node and extend it with 
additional properties in the same file, when you could merge those properties 
in the node definition. You only need to extend nodes with additional 
properties when they're defined in one file and extended in another one.

> > Apart from that the patch looks good to me.
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > 
> >> +&hdmi_in_con {
> >> +	port {
> >> +		hdmi_in: endpoint {
> >> +			remote-endpoint = <&adv7482_hdmi>;
> >> +		};
> >> +	};
> >> +};
> >> +
> >>  &du {
> >>  	pinctrl-0 = <&du_pins>;
> >>  	pinctrl-names = "default";
> >> @@ -343,6 +417,55 @@
> >> 
> >>  		shunt-resistor-micro-ohms = <5000>;
> >>  	};
> >> +
> >> +	video-receiver@70 {
> >> +		compatible = "adi,adv7482";
> >> +		reg = <0x70>;
> >> +
> >> +		#address-cells = <1>;
> >> +		#size-cells = <0>;
> 
> But I can't remove these # without generating a warning message ...

The properties are needed, why should you remove them ? :-)

> >> +
> >> +		interrupt-parent = <&gpio6>;
> >> +		interrupt-names = "intrq1", "intrq2";
> >> +		interrupts = <30 IRQ_TYPE_LEVEL_LOW>,
> >> +			     <31 IRQ_TYPE_LEVEL_LOW>;
> >> +
> >> +		port@7 {
> >> +			reg = <7>;
> >> +
> >> +			adv7482_ain7: endpoint {
> >> +				remote-endpoint = <&cvbs_in>;
> >> +			};
> >> +		};
> >> +
> >> +		port@8 {
> >> +			reg = <8>;
> >> +
> >> +			adv7482_hdmi: endpoint {
> >> +				remote-endpoint = <&hdmi_in>;
> >> +			};
> >> +		};
> >> +
> >> +		port@10 {
> >> +			reg = <10>;
> >> +
> >> +			adv7482_txa: endpoint {
> >> +				clock-lanes = <0>;
> >> +				data-lanes = <1 2 3 4>;
> >> +				remote-endpoint = <&csi40_in>;
> >> +			};
> >> +		};
> >> +
> >> +		port@11 {
> >> +			reg = <11>;
> >> +
> >> +			adv7482_txb: endpoint {
> >> +				clock-lanes = <0>;
> >> +				data-lanes = <1>;
> >> +				remote-endpoint = <&csi20_in>;
> >> +			};
> >> +		};
> >> +	};
> >>  };
> >>  
> >>  &i2c_dvfs {

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ