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 <eddie.huang@xxxxxxxxxxxx> 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 <eddie.huang@xxxxxxxxxxxx> > > --- > > > > 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 = <MT8173_PIN_45_SDA0__FUNC_SDA0>, > > + <MT8173_PIN_46_SCL0__FUNC_SCL0>; > > + bias-disable; > > + }; > > + }; > > + > > + i2c1_pins_a: i2c1@0 { > > + pins1 { > > + pinmux = <MT8173_PIN_125_SDA1__FUNC_SDA1>, > > + <MT8173_PIN_126_SCL1__FUNC_SCL1>; > > + bias-disable; > > + }; > > + }; > > + > > + i2c2_pins_a: i2c2@0 { > > + pins1 { > > + pinmux = <MT8173_PIN_43_SDA2__FUNC_SDA2>, > > + <MT8173_PIN_44_SCL2__FUNC_SCL2>; > > + bias-disable; > > + }; > > + }; > > + > > + i2c3_pins_a: i2c3@0 { > > + pins1 { > > + pinmux = <MT8173_PIN_106_SDA3__FUNC_SDA3>, > > + <MT8173_PIN_107_SCL3__FUNC_SCL3>; > > + bias-disable; > > + }; > > + }; > > + > > + i2c4_pins_a: i2c4@0 { > > + pins1 { > > + pinmux = <MT8173_PIN_133_SDA4__FUNC_SDA4>, > > + <MT8173_PIN_134_SCL4__FUNC_SCL4>; > > + bias-disable; > > + }; > > + }; > > + > > + i2c6_pins_a: i2c6@0 { > > + pins1 { > > + pinmux = <MT8173_PIN_100_MSDC2_DAT0__FUNC_SDA5>, > > + <MT8173_PIN_101_MSDC2_DAT1__FUNC_SCL5>; > > 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 = <GIC_SPI 76 IRQ_TYPE_LEVEL_LOW>; > > + 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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html