Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754662AbbFLROp (ORCPT ); Fri, 12 Jun 2015 13:14:45 -0400 Received: from mail-wg0-f52.google.com ([74.125.82.52]:34142 "EHLO mail-wg0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753176AbbFLROk (ORCPT ); Fri, 12 Jun 2015 13:14:40 -0400 From: Matthias Brugger To: Daniel Kurtz Cc: Eddie Huang , Sascha Hauer , "open list:OPEN FIRMWARE AND..." , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , linux-mediatek@lists.infradead.org, Linus Walleij Subject: Re: [PATCH v3 2/2] arm64: dts: mt8173: Add I2C device node Date: Fri, 12 Jun 2015 19:13:53 +0200 Message-ID: <1863259.t4daGE7nbQ@ubix> User-Agent: KMail/4.13.3 (Linux/3.13.0-54-generic; KDE/4.13.3; x86_64; ; ) In-Reply-To: References: <1434101229-32695-1-git-send-email-eddie.huang@mediatek.com> <1434101229-32695-3-git-send-email-eddie.huang@mediatek.com> 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: 6481 Lines: 185 On Friday, June 12, 2015 08:28:51 PM Daniel Kurtz wrote: > Hi Eddie, > > On Fri, Jun 12, 2015 at 5:27 PM, Eddie Huang wrote: > > Add MT8173 I2C device nodes, include I2C controllers and pins. > > MT8173 has six I2C controllers, from i2c0 to i2c6, exclude i2c5. > > The 6th I2C controller register base doesn't next to 5th I2C, > > and there is a hardware between 5th and 6th I2C controller. So > > SoC designer name 6th controller as "i2c6", not "i2c5". > > This is slightly misleading. There are in fact 7 I2C controllers, but > "i2c5" is dedicated for use by HDMI for ddc & hdcp. > Is there a reason why the HDMI I2C port cannot be controlled by the > generic i2c driver? > > Of course the hdmiddc / i2c5 node can always be added in a later > patch, so this is no reason to hold up this patch. > > > Signed-off-by: Eddie Huang > > --- > > > > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 50 ++++++++++++++++++++ > > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 72 > > +++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts > > b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts index 43d5401..2e01988 > > 100644 > > --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts > > +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts > > @@ -33,6 +33,56 @@ > > > > chosen { }; > > > > }; > > > > +&pio { > > I don't think we needed to move these i2c pinmux descriptions from > mt8173.dtsi to the board .dts file. > > AFAICT, the "i2cN_pins_a" nodes defined here are the pinctl > configuration that the corresponding enabled i2c nodes would choose. > Thus, they are generic to the MT8173 SoC, not specific to any board. > By themselves, these nodes do not actually select a pin configuration. > > It is the nodes that *enable* the individual i2c nodes, and hence > activate those pin settings, which is board specific. > > Hence, if your intent is to have the evb enable all i2c nodes, it > would have a set of nodes like this: > > > &i2c0 { > pinctrl-names = "default"; > pinctrl-0 = <&i2c0_pins_a>; > status = "okay"; > }; > > &i2c1 { > pinctrl-names = "default"; > pinctrl-0 = <&i2c1_pins_a>; > status = "okay"; > }; > > ... > > > + i2c0_pins_a: i2c0@0 { > > Do these nodes need the "@0"? > > > + pins1 { > > + pinmux = , > > + ; > > + bias-disable; > > + }; > > + }; > > + > > + i2c1_pins_a: i2c1@0 { > > + pins1 { > > + pinmux = , > > + ; > > + bias-disable; > > + }; > > + }; > > + > > + i2c2_pins_a: i2c2@0 { > > + pins1 { > > + pinmux = , > > + ; > > + bias-disable; > > + }; > > + }; > > + > > + i2c3_pins_a: i2c3@0 { > > + pins1 { > > + pinmux = , > > + ; > > + bias-disable; > > + }; > > + }; > > + > > + i2c4_pins_a: i2c4@0 { > > + pins1 { > > + pinmux = , > > + ; > > + bias-disable; > > + }; > > + }; > > + > > + i2c6_pins_a: i2c6@0 { > > + pins1 { > > + pinmux = , > > + ; > > These are the SDA/SCL pins for i2c port 6, so they should really be > _SDA6 / _SCL6. > However... I checked, and these settings are labeled "SDA5 & SCL5" in > the datasheet. > I recommend marking them correctly as 6 here and fixing the datasheet :-). > > > + bias-disable; > > + }; > > + }; > > +}; > > + > > > > &uart0 { > > > > status = "okay"; > > > > }; > > > > diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi > > b/arch/arm64/boot/dts/mediatek/mt8173.dtsi index b52ec43..6d3dbbdd 100644 > > --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi > > +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi > > @@ -229,6 +229,78 @@ > > > > clocks = <&uart_clk>; > > status = "disabled"; > > > > }; > > > > + > > + i2c0: i2c@11007000 { > > + compatible = "mediatek,mt8173-i2c"; > > + reg = <0 0x11007000 0 0x70>, > > + <0 0x11000100 0 0x80>; > > + interrupts = ; > > + clock-div = <16>; > > + clocks = <&pericfg CLK_PERI_I2C0>, > > + <&pericfg CLK_PERI_AP_DMA>; > > + clock-names = "main", "dma"; > > The following fields must also be selected by the i2c nodes when they > are enabled: > > clock-frequency = <100000>; > pinctrl-names = "default"; > pinctrl-0 = <&i2c0_pins_a>; > #address-cells = <1>; > #size-cells = <0>; > > So, is there any reason not to also include them here in mt8173.dtsi > so we can use them as defaults? > This would simplify the i2c nodes in the board specific .dts files. > The only field that might be board specific would be clock-frequency, > but that is trivial to override on a port-by-port basis in board files > as necessary. The problem is, that most of the pins have several modes (up to eight). In the future when all the device drivers are implemented, this gets you a really huge dtsi. AFAIK this can cause a somewhat long parsing time of the dtb when booting the system. Sascha, please correct me, if I'm wrong. Thanks, Matthias -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/