| Message ID | 20171101175335.22123-5-damien.riegel@savoirfairelinux.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Andy Gross |
| Headers | show |
On Wed 01 Nov 10:53 PDT 2017, Damien Riegel wrote: I think it's better to use the word "nodes" (add nodes...) > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com> > --- > arch/arm64/boot/dts/qcom/msm8916-pins.dtsi | 72 ++++++++++++++++++++++++++++++ > arch/arm64/boot/dts/qcom/msm8916.dtsi | 45 +++++++++++++++++++ > 2 files changed, 117 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi > index c67ad8ed8b60..1cec5b30ed6e 100644 > --- a/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi > @@ -270,6 +270,30 @@ > }; > }; > > + i2c1_default: i2c1_default { > + pinmux { > + function = "blsp_i2c1"; > + pins = "gpio2", "gpio3"; > + }; > + pinconf { > + pins = "gpio2", "gpio3"; > + drive-strength = <16>; > + bias-disable; > + }; pinconf is typically board specific, so please leave these out from the base dtsi. > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi > index de25bd6070f5..bdc4cb6f66d4 100644 > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi > @@ -455,6 +455,21 @@ > status = "disabled"; > }; > > + blsp_i2c1: i2c@78b5000 { > + compatible = "qcom,i2c-qup-v2.2.1"; > + reg = <0x078b5000 0x600>; Size is 0x500. > + interrupts = <GIC_SPI 95 0>; > + clocks = <&gcc GCC_BLSP1_AHB_CLK>, > + <&gcc GCC_BLSP1_QUP1_I2C_APPS_CLK>; > + clock-names = "iface", "core"; > + pinctrl-names = "default", "sleep"; > + pinctrl-0 = <&i2c1_default>; > + pinctrl-1 = <&i2c1_sleep>; Please omit the pinctrl-* properties from the base dtsi (when it's not hard coded things for the platform). > + #address-cells = <1>; > + #size-cells = <0>; > + status = "disabled"; > + }; > + > blsp_i2c2: i2c@78b6000 { > compatible = "qcom,i2c-qup-v2.2.1"; > reg = <0x078b6000 0x600>; Otherwise this looks good! Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, On Thu, Nov 09, 2017 at 09:00:16AM -0800, Bjorn Andersson wrote: > On Wed 01 Nov 10:53 PDT 2017, Damien Riegel wrote: > > I think it's better to use the word "nodes" (add nodes...) Will reword that. > > > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com> > > --- > > arch/arm64/boot/dts/qcom/msm8916-pins.dtsi | 72 ++++++++++++++++++++++++++++++ > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 45 +++++++++++++++++++ > > 2 files changed, 117 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi > > index c67ad8ed8b60..1cec5b30ed6e 100644 > > --- a/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi > > +++ b/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi > > @@ -270,6 +270,30 @@ > > }; > > }; > > > > + i2c1_default: i2c1_default { > > + pinmux { > > + function = "blsp_i2c1"; > > + pins = "gpio2", "gpio3"; > > + }; > > + pinconf { > > + pins = "gpio2", "gpio3"; > > + drive-strength = <16>; > > + bias-disable; > > + }; > > pinconf is typically board specific, so please leave these out from the > base dtsi. I don't mind dropping that, but pinconf for i2c{2,4,6} is already in msm8916-pins.dtsi, so we should either drop them all, or add pinconf for i2c{1,3,5} for consistency. And if I read the pinctrl driver correctly, I2C cannot be routed to other pins, the only properties that users may want to modify are drive-strength and bias. Let me know which option you prefer. > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi > > index de25bd6070f5..bdc4cb6f66d4 100644 > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi > > @@ -455,6 +455,21 @@ > > status = "disabled"; > > }; > > > > + blsp_i2c1: i2c@78b5000 { > > + compatible = "qcom,i2c-qup-v2.2.1"; > > + reg = <0x078b5000 0x600>; > > Size is 0x500. Will update that (and will also do that for patch 3) > > + interrupts = <GIC_SPI 95 0>; > > + clocks = <&gcc GCC_BLSP1_AHB_CLK>, > > + <&gcc GCC_BLSP1_QUP1_I2C_APPS_CLK>; > > + clock-names = "iface", "core"; > > + pinctrl-names = "default", "sleep"; > > + pinctrl-0 = <&i2c1_default>; > > + pinctrl-1 = <&i2c1_sleep>; > > Please omit the pinctrl-* properties from the base dtsi (when it's not > hard coded things for the platform). > > > + #address-cells = <1>; > > + #size-cells = <0>; > > + status = "disabled"; > > + }; > > + > > blsp_i2c2: i2c@78b6000 { > > compatible = "qcom,i2c-qup-v2.2.1"; > > reg = <0x078b6000 0x600>; > > Otherwise this looks good! Thank you for the review,
On Thu 09 Nov 09:14 PST 2017, Damien Riegel wrote: > Hi Bjorn, > > > On Thu, Nov 09, 2017 at 09:00:16AM -0800, Bjorn Andersson wrote: > > On Wed 01 Nov 10:53 PDT 2017, Damien Riegel wrote: > > > > I think it's better to use the word "nodes" (add nodes...) > > Will reword that. > > > > > > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com> > > > --- > > > arch/arm64/boot/dts/qcom/msm8916-pins.dtsi | 72 ++++++++++++++++++++++++++++++ > > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 45 +++++++++++++++++++ > > > 2 files changed, 117 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi > > > index c67ad8ed8b60..1cec5b30ed6e 100644 > > > --- a/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi > > > @@ -270,6 +270,30 @@ > > > }; > > > }; > > > > > > + i2c1_default: i2c1_default { > > > + pinmux { > > > + function = "blsp_i2c1"; > > > + pins = "gpio2", "gpio3"; > > > + }; > > > + pinconf { > > > + pins = "gpio2", "gpio3"; > > > + drive-strength = <16>; > > > + bias-disable; > > > + }; > > > > pinconf is typically board specific, so please leave these out from the > > base dtsi. > > I don't mind dropping that, but pinconf for i2c{2,4,6} is already in > msm8916-pins.dtsi, so we should either drop them all, or add pinconf for > i2c{1,3,5} for consistency. You're right, this needs to be consistent. So I would like us to update the existing nodes, > And if I read the pinctrl driver correctly, > I2C cannot be routed to other pins, the only properties that users may > want to modify are drive-strength and bias. Let me know which option you > prefer. > I discussed the matter with my colleagues and we concluded that we want to keep the muxing in the platform.dtsi and move the specification of the electrical properties (pinconf) to the board.dts(i). We did discuss having some "default values" in the platform file, but pushing these down to the board file gives an indication to others that these values are board specific. PS. If you're willing to provide a patch for the existing nodes I would be happy to review this as well! Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 10, 2017 at 11:56:12PM -0800, Bjorn Andersson wrote: > On Thu 09 Nov 09:14 PST 2017, Damien Riegel wrote: > > > Hi Bjorn, > > > > > > On Thu, Nov 09, 2017 at 09:00:16AM -0800, Bjorn Andersson wrote: > > > On Wed 01 Nov 10:53 PDT 2017, Damien Riegel wrote: > > > > > > I think it's better to use the word "nodes" (add nodes...) > > > > Will reword that. > > > > > > > > > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com> > > > > --- > > > > arch/arm64/boot/dts/qcom/msm8916-pins.dtsi | 72 ++++++++++++++++++++++++++++++ > > > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 45 +++++++++++++++++++ > > > > 2 files changed, 117 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi > > > > index c67ad8ed8b60..1cec5b30ed6e 100644 > > > > --- a/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi > > > > +++ b/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi > > > > @@ -270,6 +270,30 @@ > > > > }; > > > > }; > > > > > > > > + i2c1_default: i2c1_default { > > > > + pinmux { > > > > + function = "blsp_i2c1"; > > > > + pins = "gpio2", "gpio3"; > > > > + }; > > > > + pinconf { > > > > + pins = "gpio2", "gpio3"; > > > > + drive-strength = <16>; > > > > + bias-disable; > > > > + }; > > > > > > pinconf is typically board specific, so please leave these out from the > > > base dtsi. > > > > I don't mind dropping that, but pinconf for i2c{2,4,6} is already in > > msm8916-pins.dtsi, so we should either drop them all, or add pinconf for > > i2c{1,3,5} for consistency. > > You're right, this needs to be consistent. So I would like us to update > the existing nodes, > > > And if I read the pinctrl driver correctly, > > I2C cannot be routed to other pins, the only properties that users may > > want to modify are drive-strength and bias. Let me know which option you > > prefer. > > > > I discussed the matter with my colleagues and we concluded that we want > to keep the muxing in the platform.dtsi and move the specification of > the electrical properties (pinconf) to the board.dts(i). > > We did discuss having some "default values" in the platform file, but > pushing these down to the board file gives an indication to others that > these values are board specific. Okay that makes sense. I'll work on a patchset to do that. Regards,
diff --git a/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi index c67ad8ed8b60..1cec5b30ed6e 100644 --- a/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi +++ b/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi @@ -270,6 +270,30 @@ }; }; + i2c1_default: i2c1_default { + pinmux { + function = "blsp_i2c1"; + pins = "gpio2", "gpio3"; + }; + pinconf { + pins = "gpio2", "gpio3"; + drive-strength = <16>; + bias-disable; + }; + }; + + i2c1_sleep: i2c1_sleep { + pinmux { + function = "gpio"; + pins = "gpio2", "gpio3"; + }; + pinconf { + pins = "gpio2", "gpio3"; + drive-strength = <2>; + bias-disable; + }; + }; + i2c2_default: i2c2_default { pinmux { function = "blsp_i2c2"; @@ -294,6 +318,30 @@ }; }; + i2c3_default: i2c3_default { + pinmux { + function = "blsp_i2c3"; + pins = "gpio10", "gpio11"; + }; + pinconf { + pins = "gpio10", "gpio11"; + drive-strength = <16>; + bias-disable; + }; + }; + + i2c3_sleep: i2c3_sleep { + pinmux { + function = "gpio"; + pins = "gpio10", "gpio11"; + }; + pinconf { + pins = "gpio10", "gpio11"; + drive-strength = <2>; + bias-disable; + }; + }; + i2c4_default: i2c4_default { pinmux { function = "blsp_i2c4"; @@ -318,6 +366,30 @@ }; }; + i2c5_default: i2c5_default { + pinmux { + function = "blsp_i2c5"; + pins = "gpio18", "gpio19"; + }; + pinconf { + pins = "gpio18", "gpio19"; + drive-strength = <16>; + bias-disable; + }; + }; + + i2c5_sleep: i2c5_sleep { + pinmux { + function = "gpio"; + pins = "gpio18", "gpio19"; + }; + pinconf { + pins = "gpio18", "gpio19"; + drive-strength = <2>; + bias-disable; + }; + }; + i2c6_default: i2c6_default { pinmux { function = "blsp_i2c6"; diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi index de25bd6070f5..bdc4cb6f66d4 100644 --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi @@ -455,6 +455,21 @@ status = "disabled"; }; + blsp_i2c1: i2c@78b5000 { + compatible = "qcom,i2c-qup-v2.2.1"; + reg = <0x078b5000 0x600>; + interrupts = <GIC_SPI 95 0>; + clocks = <&gcc GCC_BLSP1_AHB_CLK>, + <&gcc GCC_BLSP1_QUP1_I2C_APPS_CLK>; + clock-names = "iface", "core"; + pinctrl-names = "default", "sleep"; + pinctrl-0 = <&i2c1_default>; + pinctrl-1 = <&i2c1_sleep>; + #address-cells = <1>; + #size-cells = <0>; + status = "disabled"; + }; + blsp_i2c2: i2c@78b6000 { compatible = "qcom,i2c-qup-v2.2.1"; reg = <0x078b6000 0x600>; @@ -470,6 +485,21 @@ status = "disabled"; }; + blsp_i2c3: i2c@78b7000 { + compatible = "qcom,i2c-qup-v2.2.1"; + reg = <0x078b7000 0x600>; + interrupts = <GIC_SPI 97 0>; + clocks = <&gcc GCC_BLSP1_AHB_CLK>, + <&gcc GCC_BLSP1_QUP3_I2C_APPS_CLK>; + clock-names = "iface", "core"; + pinctrl-names = "default", "sleep"; + pinctrl-0 = <&i2c3_default>; + pinctrl-1 = <&i2c3_sleep>; + #address-cells = <1>; + #size-cells = <0>; + status = "disabled"; + }; + blsp_i2c4: i2c@78b8000 { compatible = "qcom,i2c-qup-v2.2.1"; reg = <0x078b8000 0x600>; @@ -485,6 +515,21 @@ status = "disabled"; }; + blsp_i2c5: i2c@78b9000 { + compatible = "qcom,i2c-qup-v2.2.1"; + reg = <0x078b9000 0x600>; + interrupts = <GIC_SPI 99 0>; + clocks = <&gcc GCC_BLSP1_AHB_CLK>, + <&gcc GCC_BLSP1_QUP5_I2C_APPS_CLK>; + clock-names = "iface", "core"; + pinctrl-names = "default", "sleep"; + pinctrl-0 = <&i2c5_default>; + pinctrl-1 = <&i2c5_sleep>; + #address-cells = <1>; + #size-cells = <0>; + status = "disabled"; + }; + blsp_i2c6: i2c@78ba000 { compatible = "qcom,i2c-qup-v2.2.1"; reg = <0x078ba000 0x600>;
Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com> --- arch/arm64/boot/dts/qcom/msm8916-pins.dtsi | 72 ++++++++++++++++++++++++++++++ arch/arm64/boot/dts/qcom/msm8916.dtsi | 45 +++++++++++++++++++ 2 files changed, 117 insertions(+)