Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751672AbdF0N5V (ORCPT ); Tue, 27 Jun 2017 09:57:21 -0400 Received: from galahad.ideasonboard.com ([185.26.127.97]:32877 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751592AbdF0N5Q (ORCPT ); Tue, 27 Jun 2017 09:57:16 -0400 From: Laurent Pinchart To: kieran.bingham@ideasonboard.com Cc: Kieran Bingham , Niklas =?ISO-8859-1?Q?S=F6derlund?= , linux-renesas-soc@vger.kernel.org, linux-kernel@vger.kernel.org, Simon Horman , Magnus Damm , Rob Herring , Mark Rutland , Catalin Marinas , Will Deacon , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , "moderated list:ARM64 PORT (AARCH64 ARCHITECTURE)" Subject: Re: [PATCH v5 2/2] arm64: dts: renesas: salvator-x: Add ADV7482 support Date: Tue, 27 Jun 2017 16:57:10 +0300 Message-ID: <3381435.LMORAGhIb5@avalon> User-Agent: KMail/4.14.10 (Linux/4.9.16-gentoo; KDE/4.14.32; x86_64; ; ) In-Reply-To: References: <10057741.K1MBojqRFe@avalon> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5487 Lines: 221 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 > >> > >> 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 > >> > >> 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 > > > >> +&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