[PATCH v12 0/4] Add lpass pin control support for audio on sc7280 based targets

[PATCH v12 0/4] Add lpass pin control support for audio on sc7280 based targets

[Srinivasa Rao Mandadapu]

This patch set is to add lpass pin control support for Audio over I2S,
wcd codec and digital mics.

Changes Since V11:
    -- Move CRD specific pinmux nodes to crd specific file.
Changes Since V10:
    -- Add lpass lpi pinmux and MI2S pinmux support for rev5+ boards.
    -- Remove dependency patches link in the cover-letter as it is merged.
Changes Since V9:
    -- Remove redundant prefix in node name.
Changes Since V8:
    -- Modify label and node names to lpass specific.
    -- Sort nodes as per node names and kind of nodes like pinctrl and device nodes.
Changes Since V7:
    -- Sort mi2s pincontrol nodes as per node name.
    -- Fix typo errors.
Changes Since V6:
    -- Move amp_en node to corresponding consumer patch.
    -- Update label and node names.
    -- Remove redundant drive-strengths.
    -- Remove herobrine crd specific mi2s configuration.
Changes Since V5:
    -- Remove redundant function property in amp_en node.
    -- Move board specific properties of lpass pin control node to board specific file.
    -- Remove redundant properties in pin control nodes.
    -- Move wcd938x codec reset and CTIA/OMTP pin control patches to other series.
Changes Since V4:
    -- Add primary and secondary I2S pinmux nodes for herobrine specific targets.
Changes Since V3:
    -- Add pinctrl nodes for wcd codec reset and CTIA/OMTP headset selection.
Changes Since V2:
    -- Move lpass pin control node to main dtsi file.
    -- Sort nodes alphabetically.
    -- Remove redundant wcd reset gpio nodes.
    -- Remove redundant input-enable field in dmic pin control nodes.
    -- Update amp_en node. 
    -- Fix typo errors.
    -- Modify node names.
    -- Create patches on latest kernel.    
Changes Since V1:
    -- Merge pinmux and pinconf properties in amp_en and wcd pin reset node.
    -- Split common i2s pin control nodes to functionality specific nodes.
    -- Move board specific properties to board specific dtsi file.
    -- Update dmic pin control node name.

Srinivasa Rao Mandadapu (4):
  arm64: dts: qcom: sc7280: Add pinmux for I2S speaker and Headset
  arm64: dts: qcom: sc7280: Add secondary MI2S pinmux specifications for
    CRD 3.0/3.1
  arm64: dts: qcom: sc7280: add lpass lpi pin controller node
  arm64: dts: qcom: sc7280-herobrine: Add lpi pinmux properties for CRD
    3.0/3.1

 arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts |  98 +++++++++++++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi          |  98 +++++++++++++++
 arch/arm64/boot/dts/qcom/sc7280.dtsi              | 147 ++++++++++++++++++++++
 3 files changed, 343 insertions(+)

-- 
2.7.4

[PATCH v12 1/4] arm64: dts: qcom: sc7280: Add pinmux for I2S speaker and Headset

[Srinivasa Rao Mandadapu]

Add pinmux nodes for primary and secondary I2S for SC7280 based platforms.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 14 +++++++++++
 arch/arm64/boot/dts/qcom/sc7280.dtsi     | 40 ++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index 015a347..fb1f4ca 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -359,6 +359,20 @@
 	bias-disable;
 };
 
+&mi2s1_data0 {
+	drive-strength = <6>;
+	bias-disable;
+};
+
+&mi2s1_sclk {
+	drive-strength = <6>;
+	bias-disable;
+};
+
+&mi2s1_ws {
+	drive-strength = <6>;
+};
+
 &pm7325_gpios {
 	key_vol_up_default: key-vol-up-default {
 		pins = "gpio6";
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 00bacc4..0242f1d 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -3713,6 +3713,46 @@
 				function = "edp_hot";
 			};
 
+			mi2s0_data0: mi2s0-data0 {
+				pins = "gpio98";
+				function = "mi2s0_data0";
+			};
+
+			mi2s0_data1: mi2s0-data1 {
+				pins = "gpio99";
+				function = "mi2s0_data1";
+			};
+
+			mi2s0_mclk: mi2s0-mclk {
+				pins = "gpio96";
+				function = "pri_mi2s";
+			};
+
+			mi2s0_sclk: mi2s0-sclk {
+				pins = "gpio97";
+				function = "mi2s0_sck";
+			};
+
+			mi2s0_ws: mi2s0-ws {
+				pins = "gpio100";
+				function = "mi2s0_ws";
+			};
+
+			mi2s1_data0: mi2s1-data0 {
+				pins = "gpio107";
+				function = "mi2s1_data0";
+			};
+
+			mi2s1_sclk: mi2s1-sclk {
+				pins = "gpio106";
+				function = "mi2s1_sck";
+			};
+
+			mi2s1_ws: mi2s1-ws {
+				pins = "gpio108";
+				function = "mi2s1_ws";
+			};
+
 			pcie1_clkreq_n: pcie1-clkreq-n {
 				pins = "gpio79";
 				function = "pcie1_clkreqn";
-- 
2.7.4

[PATCH v12 2/4] arm64: dts: qcom: sc7280: Add secondary MI2S pinmux specifications for CRD 3.0/3.1

[Srinivasa Rao Mandadapu]

Add drive strength property for secondary MI2S on
sc7280 based platforms of rev5+ (aka CRD 3.0/3.1) boards.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
index b06f61e..deaea3a 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
@@ -111,6 +111,20 @@ ap_ts_pen_1v8: &i2c13 {
  * - If a pin is not hooked up on Qcard, it gets no name.
  */
 
+&mi2s1_data0 {
+	drive-strength = <6>;
+	bias-disable;
+};
+
+&mi2s1_sclk {
+	drive-strength = <6>;
+	bias-disable;
+};
+
+&mi2s1_ws {
+	drive-strength = <6>;
+};
+
 &pm8350c_gpios {
 	gpio-line-names = "FLASH_STROBE_1",		/* 1 */
 			  "AP_SUSPEND",
-- 
2.7.4

Re: [PATCH v12 2/4] arm64: dts: qcom: sc7280: Add secondary MI2S pinmux specifications for CRD 3.0/3.1

[Matthias Kaehlcke]

On Wed, Apr 27, 2022 at 10:39:41PM +0530, Srinivasa Rao Mandadapu wrote:
> Add drive strength property for secondary MI2S on
> sc7280 based platforms of rev5+ (aka CRD 3.0/3.1) boards.
> 
> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
> Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

[PATCH v12 3/4] arm64: dts: qcom: sc7280: add lpass lpi pin controller node

[Srinivasa Rao Mandadapu]

Add LPASS LPI pinctrl node required for Audio functionality on sc7280
based platforms.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi |  84 ++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc7280.dtsi     | 107 +++++++++++++++++++++++++++++++
 2 files changed, 191 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index fb1f4ca..2f863c0 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -359,6 +359,90 @@
 	bias-disable;
 };
 
+&lpass_dmic01 {
+	clk {
+		drive-strength = <8>;
+	};
+};
+
+&lpass_dmic01_sleep {
+	clk {
+		drive-strength = <2>;
+		bias-disable;
+	};
+
+	data {
+		pull-down;
+	};
+};
+
+&lpass_dmic23 {
+	clk {
+		drive-strength = <8>;
+	};
+};
+
+&lpass_dmic23_sleep {
+	clk {
+		drive-strength = <2>;
+		bias-disable;
+	};
+
+	data {
+		pull-down;
+	};
+};
+
+&lpass_rx_swr {
+	clk {
+		drive-strength = <2>;
+		slew-rate = <1>;
+		bias-disable;
+	};
+
+	data {
+		drive-strength = <2>;
+		slew-rate = <1>;
+		bias-bus-hold;
+	};
+};
+
+&lpass_rx_swr_sleep {
+	clk {
+		drive-strength = <2>;
+		bias-pull-down;
+	};
+
+	data {
+		drive-strength = <2>;
+		bias-pull-down;
+	};
+};
+
+&lpass_tx_swr {
+	clk {
+		drive-strength = <2>;
+		slew-rate = <1>;
+		bias-disable;
+	};
+
+	data {
+		slew-rate = <1>;
+		bias-bus-hold;
+	};
+};
+
+&lpass_tx_swr_sleep {
+	clk {
+		drive-strength = <2>;
+		bias-pull-down;
+	};
+
+	data {
+		bias-bus-hold;
+	};
+};
+
 &mi2s1_data0 {
 	drive-strength = <6>;
 	bias-disable;
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 0242f1d..fe500f4 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -2083,6 +2083,113 @@
 			qcom,bcm-voters = <&apps_bcm_voter>;
 		};
 
+		lpass_tlmm: pinctrl@33c0000 {
+			compatible = "qcom,sc7280-lpass-lpi-pinctrl";
+			reg = <0 0x033c0000 0x0 0x20000>,
+				<0 0x03550000 0x0 0x10000>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-ranges = <&lpass_tlmm 0 0 15>;
+
+			#clock-cells = <1>;
+
+			lpass_dmic01: dmic01 {
+				clk {
+					pins = "gpio6";
+					function = "dmic1_clk";
+				};
+
+				data {
+					pins = "gpio7";
+					function = "dmic1_data";
+				};
+			};
+
+			lpass_dmic01_sleep: dmic01-sleep {
+				clk {
+					pins = "gpio6";
+					function = "dmic1_clk";
+				};
+
+				data {
+					pins = "gpio7";
+					function = "dmic1_data";
+				};
+			};
+
+			lpass_dmic23: dmic23 {
+				clk {
+					pins = "gpio8";
+					function = "dmic2_clk";
+				};
+
+				data {
+					pins = "gpio9";
+					function = "dmic2_data";
+				};
+			};
+
+			lpass_dmic23_sleep: dmic23-sleep {
+				clk {
+					pins = "gpio8";
+					function = "dmic2_clk";
+				};
+
+				data {
+					pins = "gpio9";
+					function = "dmic2_data";
+				};
+			};
+
+			lpass_rx_swr: rx-swr {
+				clk {
+					pins = "gpio3";
+					function = "swr_rx_clk";
+				};
+
+				data {
+					pins = "gpio4", "gpio5";
+					function = "swr_rx_data";
+				};
+			};
+
+			lpass_rx_swr_sleep: rx-swr-sleep {
+				clk {
+					pins = "gpio3";
+					function = "swr_rx_clk";
+				};
+
+				data {
+					pins = "gpio4", "gpio5";
+					function = "swr_rx_data";
+				};
+			};
+
+			lpass_tx_swr: tx-swr {
+				clk {
+					pins = "gpio0";
+					function = "swr_tx_clk";
+				};
+
+				data {
+					pins = "gpio1", "gpio2", "gpio14";
+					function = "swr_tx_data";
+				};
+			};
+
+			lpass_tx_swr_sleep: tx-swr-sleep {
+				clk {
+					pins = "gpio0";
+					function = "swr_tx_clk";
+				};
+
+				data {
+					pins = "gpio1", "gpio2", "gpio14";
+					function = "swr_tx_data";
+				};
+			};
+		};
+
 		gpu: gpu@3d00000 {
 			compatible = "qcom,adreno-635.0", "qcom,adreno";
 			reg = <0 0x03d00000 0 0x40000>,
-- 
2.7.4

[PATCH v12 4/4] arm64: dts: qcom: sc7280-herobrine: Add lpi pinmux properties for CRD 3.0/3.1

[Srinivasa Rao Mandadapu]

Add LPASS LPI pinctrl properties, which are required for Audio
functionality on herobrine based platforms of rev5+
(aka CRD 3.0/3.1) boards.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 84 +++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
index deaea3a..dfc42df 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
@@ -111,6 +111,90 @@ ap_ts_pen_1v8: &i2c13 {
  * - If a pin is not hooked up on Qcard, it gets no name.
  */
 
+&lpass_dmic01 {
+	clk {
+		drive-strength = <8>;
+	};
+};
+
+&lpass_dmic01_sleep {
+	clk {
+		drive-strength = <2>;
+		bias-disable;
+	};
+
+	data {
+		pull-down;
+	};
+};
+
+&lpass_dmic23 {
+	clk {
+		drive-strength = <8>;
+	};
+};
+
+&lpass_dmic23_sleep {
+	clk {
+		drive-strength = <2>;
+		bias-disable;
+	};
+
+	data {
+		pull-down;
+	};
+};
+
+&lpass_rx_swr {
+	clk {
+		drive-strength = <2>;
+		slew-rate = <1>;
+		bias-disable;
+	};
+
+	data {
+		drive-strength = <2>;
+		slew-rate = <1>;
+		bias-bus-hold;
+	};
+};
+
+&lpass_rx_swr_sleep {
+	clk {
+		drive-strength = <2>;
+		bias-pull-down;
+	};
+
+	data {
+		drive-strength = <2>;
+		bias-pull-down;
+	};
+};
+
+&lpass_tx_swr {
+	clk {
+		drive-strength = <2>;
+		slew-rate = <1>;
+		bias-disable;
+	};
+
+	data {
+		slew-rate = <1>;
+		bias-bus-hold;
+	};
+};
+
+&lpass_tx_swr_sleep {
+	clk {
+		drive-strength = <2>;
+		bias-pull-down;
+	};
+
+	data {
+		bias-bus-hold;
+	};
+};
+
 &mi2s1_data0 {
 	drive-strength = <6>;
 	bias-disable;
-- 
2.7.4

Re: [PATCH v12 4/4] arm64: dts: qcom: sc7280-herobrine: Add lpi pinmux properties for CRD 3.0/3.1

[Matthias Kaehlcke]

On Wed, Apr 27, 2022 at 10:39:43PM +0530, Srinivasa Rao Mandadapu wrote:
> Add LPASS LPI pinctrl properties, which are required for Audio
> functionality on herobrine based platforms of rev5+
> (aka CRD 3.0/3.1) boards.
> 
> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
> Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>

I'm not super firm in pinctrl territory, a few maybe silly questions
below.

>  arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 84 +++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> index deaea3a..dfc42df 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> @@ -111,6 +111,90 @@ ap_ts_pen_1v8: &i2c13 {
>   * - If a pin is not hooked up on Qcard, it gets no name.
>   */
>  
> +&lpass_dmic01 {
> +	clk {
> +		drive-strength = <8>;
> +	};
> +};
> +
> +&lpass_dmic01_sleep {
> +	clk {
> +		drive-strength = <2>;

Does the drive strength really matter in the sleep state, is the SoC actively
driving the pin?

> +		bias-disable;

What should this be in active/default state? If I understand correctly
after a transition from 'sleep' to 'default' this setting will remain,
since the default config doesn't specify a setting for bias.

> +	};
> +
> +	data {
> +		pull-down;

Same here, I think the pull-down will still be enabled after a switch from
'sleep' to 'default'.

If I'm not mistaken then the rest of the pins also need to be reviewed.

> +	};
> +};
> +
> +&lpass_dmic23 {
> +	clk {
> +		drive-strength = <8>;
> +	};
> +};
> +
> +&lpass_dmic23_sleep {
> +	clk {
> +		drive-strength = <2>;
> +		bias-disable;
> +	};
> +
> +	data {
> +		pull-down;
> +	};
> +};
> +
> +&lpass_rx_swr {
> +	clk {
> +		drive-strength = <2>;
> +		slew-rate = <1>;
> +		bias-disable;
> +	};
> +
> +	data {
> +		drive-strength = <2>;
> +		slew-rate = <1>;
> +		bias-bus-hold;
> +	};
> +};
> +
> +&lpass_rx_swr_sleep {
> +	clk {
> +		drive-strength = <2>;
> +		bias-pull-down;
> +	};
> +
> +	data {
> +		drive-strength = <2>;
> +		bias-pull-down;
> +	};
> +};
> +
> +&lpass_tx_swr {
> +	clk {
> +		drive-strength = <2>;
> +		slew-rate = <1>;
> +		bias-disable;
> +	};
> +
> +	data {
> +		slew-rate = <1>;
> +		bias-bus-hold;
> +	};
> +};
> +
> +&lpass_tx_swr_sleep {
> +	clk {
> +		drive-strength = <2>;
> +		bias-pull-down;
> +	};
> +
> +	data {
> +		bias-bus-hold;
> +	};
> +};
> +
>  &mi2s1_data0 {
>  	drive-strength = <6>;
>  	bias-disable;
> -- 
> 2.7.4
> 

Re: [PATCH v12 4/4] arm64: dts: qcom: sc7280-herobrine: Add lpi pinmux properties for CRD 3.0/3.1

[Doug Anderson]

Hi,

On Thu, Apr 28, 2022 at 5:02 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Wed, Apr 27, 2022 at 10:39:43PM +0530, Srinivasa Rao Mandadapu wrote:
> > Add LPASS LPI pinctrl properties, which are required for Audio
> > functionality on herobrine based platforms of rev5+
> > (aka CRD 3.0/3.1) boards.
> >
> > Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> > Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
> > Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
>
> I'm not super firm in pinctrl territory, a few maybe silly questions
> below.
>
> >  arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 84 +++++++++++++++++++++++
> >  1 file changed, 84 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > index deaea3a..dfc42df 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > @@ -111,6 +111,90 @@ ap_ts_pen_1v8: &i2c13 {
> >   * - If a pin is not hooked up on Qcard, it gets no name.
> >   */
> >
> > +&lpass_dmic01 {
> > +     clk {
> > +             drive-strength = <8>;
> > +     };

Ugh, I've been distracted and I hadn't realized we were back to the
two-level syntax. Definitely not my favorite for all the reasons I
talked about [1]. I guess you took Bjorn's silence to my response to
mean that you should switch back to this way? :(

Bjorn: can you clarify?

[1] https://lore.kernel.org/r/CAD=FV=VicFiX6QkBksZs1KLwJ5x4eCte6j5RWOBPN+WwiXm2Cw@mail.gmail.com/

> > +};
> > +
> > +&lpass_dmic01_sleep {
> > +     clk {
> > +             drive-strength = <2>;
>
> Does the drive strength really matter in the sleep state, is the SoC actively
> driving the pin?

My understanding is that if a pin is left as an output in sleep state
that there is a slight benefit to switching it to drive-strength 2.


> > +             bias-disable;
>
> What should this be in active/default state? If I understand correctly
> after a transition from 'sleep' to 'default' this setting will remain,
> since the default config doesn't specify a setting for bias.

Your understanding matches mine but I haven't tested it and I remember
sometimes being surprised in this corner of pinmux before. I think
it's better to put the bias in the default state if it should be that
way all the time, or have a bias in both the default and sleep state
if they need to be different.

Re: [PATCH v12 4/4] arm64: dts: qcom: sc7280-herobrine: Add lpi pinmux properties for CRD 3.0/3.1

[Srinivasa Rao Mandadapu]

On 4/29/2022 9:40 PM, Doug Anderson wrote:
Thanks for your time Doug Anderson!!!
> Hi,
>
> On Thu, Apr 28, 2022 at 5:02 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>> On Wed, Apr 27, 2022 at 10:39:43PM +0530, Srinivasa Rao Mandadapu wrote:
>>> Add LPASS LPI pinctrl properties, which are required for Audio
>>> functionality on herobrine based platforms of rev5+
>>> (aka CRD 3.0/3.1) boards.
>>>
>>> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
>>> Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
>>> Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
>> I'm not super firm in pinctrl territory, a few maybe silly questions
>> below.
>>
>>>   arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 84 +++++++++++++++++++++++
>>>   1 file changed, 84 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
>>> index deaea3a..dfc42df 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
>>> @@ -111,6 +111,90 @@ ap_ts_pen_1v8: &i2c13 {
>>>    * - If a pin is not hooked up on Qcard, it gets no name.
>>>    */
>>>
>>> +&lpass_dmic01 {
>>> +     clk {
>>> +             drive-strength = <8>;
>>> +     };
> Ugh, I've been distracted and I hadn't realized we were back to the
> two-level syntax. Definitely not my favorite for all the reasons I
> talked about [1]. I guess you took Bjorn's silence to my response to
> mean that you should switch back to this way? :(
>
> Bjorn: can you clarify?
>
> [1] https://lore.kernel.org/r/CAD=FV=VicFiX6QkBksZs1KLwJ5x4eCte6j5RWOBPN+WwiXm2Cw@mail.gmail.com/
Actually Your comment addressed for MI2S pin control nodes, but missed 
here. Will address same here.
>>> +};
>>> +
>>> +&lpass_dmic01_sleep {
>>> +     clk {
>>> +             drive-strength = <2>;
>> Does the drive strength really matter in the sleep state, is the SoC actively
>> driving the pin?
> My understanding is that if a pin is left as an output in sleep state
> that there is a slight benefit to switching it to drive-strength 2.
Okay. Will keep this setting as it is. Please correct me if my 
understanding is wrong.
>
>
>>> +             bias-disable;
>> What should this be in active/default state? If I understand correctly
>> after a transition from 'sleep' to 'default' this setting will remain,
>> since the default config doesn't specify a setting for bias.
> Your understanding matches mine but I haven't tested it and I remember
> sometimes being surprised in this corner of pinmux before. I think
> it's better to put the bias in the default state if it should be that
> way all the time, or have a bias in both the default and sleep state
> if they need to be different.
Okay. Will update accordingly.

Re: [PATCH v12 4/4] arm64: dts: qcom: sc7280-herobrine: Add lpi pinmux properties for CRD 3.0/3.1

[Bjorn Andersson]

On Fri 29 Apr 11:10 CDT 2022, Doug Anderson wrote:

> Hi,
> 
> On Thu, Apr 28, 2022 at 5:02 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > On Wed, Apr 27, 2022 at 10:39:43PM +0530, Srinivasa Rao Mandadapu wrote:
> > > Add LPASS LPI pinctrl properties, which are required for Audio
> > > functionality on herobrine based platforms of rev5+
> > > (aka CRD 3.0/3.1) boards.
> > >
> > > Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> > > Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
> > > Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
> >
> > I'm not super firm in pinctrl territory, a few maybe silly questions
> > below.
> >
> > >  arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 84 +++++++++++++++++++++++
> > >  1 file changed, 84 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > > index deaea3a..dfc42df 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > > @@ -111,6 +111,90 @@ ap_ts_pen_1v8: &i2c13 {
> > >   * - If a pin is not hooked up on Qcard, it gets no name.
> > >   */
> > >
> > > +&lpass_dmic01 {
> > > +     clk {
> > > +             drive-strength = <8>;
> > > +     };
> 
> Ugh, I've been distracted and I hadn't realized we were back to the
> two-level syntax. Definitely not my favorite for all the reasons I
> talked about [1]. I guess you took Bjorn's silence to my response to
> mean that you should switch back to this way? :(
> 
> Bjorn: can you clarify?
> 

I didn't think through the fact that &mi2s0_state was specified in the
.dtsi and as such will be partially be overridden by the baord dts.


I do prefer the two level style and describing full "states", but as you
say whenever we provide something that will have to be overwritten it's
suboptimal.

As such, I think your flattened model is preferred in this case - but it
makes me dislike the partial definition between the dtsi and dts even
more (but I don't have any better suggestion).

Regards,
Bjorn

> [1] https://lore.kernel.org/r/CAD=FV=VicFiX6QkBksZs1KLwJ5x4eCte6j5RWOBPN+WwiXm2Cw@mail.gmail.com/
> 
> > > +};
> > > +
> > > +&lpass_dmic01_sleep {
> > > +     clk {
> > > +             drive-strength = <2>;
> >
> > Does the drive strength really matter in the sleep state, is the SoC actively
> > driving the pin?
> 
> My understanding is that if a pin is left as an output in sleep state
> that there is a slight benefit to switching it to drive-strength 2.
> 
> 
> > > +             bias-disable;
> >
> > What should this be in active/default state? If I understand correctly
> > after a transition from 'sleep' to 'default' this setting will remain,
> > since the default config doesn't specify a setting for bias.
> 
> Your understanding matches mine but I haven't tested it and I remember
> sometimes being surprised in this corner of pinmux before. I think
> it's better to put the bias in the default state if it should be that
> way all the time, or have a bias in both the default and sleep state
> if they need to be different.

Re: [PATCH v12 4/4] arm64: dts: qcom: sc7280-herobrine: Add lpi pinmux properties for CRD 3.0/3.1

[Doug Anderson]

Hi,

On Wed, May 4, 2022 at 10:07 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Fri 29 Apr 11:10 CDT 2022, Doug Anderson wrote:
>
> > Hi,
> >
> > On Thu, Apr 28, 2022 at 5:02 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> > >
> > > On Wed, Apr 27, 2022 at 10:39:43PM +0530, Srinivasa Rao Mandadapu wrote:
> > > > Add LPASS LPI pinctrl properties, which are required for Audio
> > > > functionality on herobrine based platforms of rev5+
> > > > (aka CRD 3.0/3.1) boards.
> > > >
> > > > Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> > > > Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
> > > > Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
> > >
> > > I'm not super firm in pinctrl territory, a few maybe silly questions
> > > below.
> > >
> > > >  arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 84 +++++++++++++++++++++++
> > > >  1 file changed, 84 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > > > index deaea3a..dfc42df 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > > > @@ -111,6 +111,90 @@ ap_ts_pen_1v8: &i2c13 {
> > > >   * - If a pin is not hooked up on Qcard, it gets no name.
> > > >   */
> > > >
> > > > +&lpass_dmic01 {
> > > > +     clk {
> > > > +             drive-strength = <8>;
> > > > +     };
> >
> > Ugh, I've been distracted and I hadn't realized we were back to the
> > two-level syntax. Definitely not my favorite for all the reasons I
> > talked about [1]. I guess you took Bjorn's silence to my response to
> > mean that you should switch back to this way? :(
> >
> > Bjorn: can you clarify?
> >
>
> I didn't think through the fact that &mi2s0_state was specified in the
> .dtsi and as such will be partially be overridden by the baord dts.
>
>
> I do prefer the two level style and describing full "states", but as you
> say whenever we provide something that will have to be overwritten it's
> suboptimal.
>
> As such, I think your flattened model is preferred in this case

How about for future patches we just provided labels at both levels
(I'm not suggesting we churn this patch series more):

lpass_dmic01_sleep: dmic01-sleep {
  lpass_dmic01_sleep_clk: clk {
    pins = "gpio6";
    function = "dmic1_clk";
  };

  lpass_dmic01_sleep_data: data {
    pins = "gpio7";
    function = "dmic1_data";
  };
};

Then you can in your pinctrl reference you can just reference the
top-level node but boards can override without having to replicate
hierarchy...

> but it
> makes me dislike the partial definition between the dtsi and dts even
> more (but I don't have any better suggestion).

One other proposal I'd make is that maybe we should change the rules
about never putting drive strength in the soc.dtsi file. While it
should still be OK for boards to override the drive strength, it seems
like a whole lot of biolerplate code to have every board override
every pin and say that its drive strength is 2. Similarly, if there's
a high speed interface (like eMMC) where a drive strength of 2 is
nonsense for any board, it doesn't seem ridiculous to specify a
default drive strength of something higher in the soc.dtsi file.

I would like to say the same thing goes for for pulls, but it's
unfortunately uglier for pulls. :( For instance, nearly everyone has
an external pullup for i2c busses. The strength of the pullup needs to
be tuned for the i2c bus speed and the impedance of the line. Thus, it
would ideally make sense to specify this in the soc.dtsi file.
Unfortunately, if we do that and some board _wants_ to use the
internal pulls (maybe they're running at a really low speed and/or
forgot to add external pulls) then they have to do an ugly
"/delete-property/ bias-disable" because adding the "bias-pull-up"
doesn't delete the other property and you end up with both. :( That
seems bad, so I guess I'd vote to keep banning bias definitions in the
soc.dtsi file.

Anyway, I'd love your opinion on this.

-Doug

Re: [PATCH v12 4/4] arm64: dts: qcom: sc7280-herobrine: Add lpi pinmux properties for CRD 3.0/3.1

[Matthias Kaehlcke]

On Thu, May 05, 2022 at 05:06:08PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, May 4, 2022 at 10:07 AM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Fri 29 Apr 11:10 CDT 2022, Doug Anderson wrote:
> >
> > > Hi,
> > >
> > > On Thu, Apr 28, 2022 at 5:02 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> > > >
> > > > On Wed, Apr 27, 2022 at 10:39:43PM +0530, Srinivasa Rao Mandadapu wrote:
> > > > > Add LPASS LPI pinctrl properties, which are required for Audio
> > > > > functionality on herobrine based platforms of rev5+
> > > > > (aka CRD 3.0/3.1) boards.
> > > > >
> > > > > Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> > > > > Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
> > > > > Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
> > > >
> > > > I'm not super firm in pinctrl territory, a few maybe silly questions
> > > > below.
> > > >
> > > > >  arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 84 +++++++++++++++++++++++
> > > > >  1 file changed, 84 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > > > > index deaea3a..dfc42df 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > > > > @@ -111,6 +111,90 @@ ap_ts_pen_1v8: &i2c13 {
> > > > >   * - If a pin is not hooked up on Qcard, it gets no name.
> > > > >   */
> > > > >
> > > > > +&lpass_dmic01 {
> > > > > +     clk {
> > > > > +             drive-strength = <8>;
> > > > > +     };
> > >
> > > Ugh, I've been distracted and I hadn't realized we were back to the
> > > two-level syntax. Definitely not my favorite for all the reasons I
> > > talked about [1]. I guess you took Bjorn's silence to my response to
> > > mean that you should switch back to this way? :(
> > >
> > > Bjorn: can you clarify?
> > >
> >
> > I didn't think through the fact that &mi2s0_state was specified in the
> > .dtsi and as such will be partially be overridden by the baord dts.
> >
> >
> > I do prefer the two level style and describing full "states", but as you
> > say whenever we provide something that will have to be overwritten it's
> > suboptimal.
> >
> > As such, I think your flattened model is preferred in this case
> 
> How about for future patches we just provided labels at both levels
> (I'm not suggesting we churn this patch series more):
> 
> lpass_dmic01_sleep: dmic01-sleep {

is the outer label ('lpass_dmic01_sleep') actually needed if we don't
intend to replicate the hierarchy?

>   lpass_dmic01_sleep_clk: clk {
>     pins = "gpio6";
>     function = "dmic1_clk";
>   };
> 
>   lpass_dmic01_sleep_data: data {
>     pins = "gpio7";
>     function = "dmic1_data";
>   };
> };
> 
> Then you can in your pinctrl reference you can just reference the
> top-level node but boards can override without having to replicate
> hierarchy...
> 
> > but it
> > makes me dislike the partial definition between the dtsi and dts even
> > more (but I don't have any better suggestion).
> 
> One other proposal I'd make is that maybe we should change the rules
> about never putting drive strength in the soc.dtsi file. While it
> should still be OK for boards to override the drive strength, it seems
> like a whole lot of biolerplate code to have every board override
> every pin and say that its drive strength is 2. Similarly, if there's
> a high speed interface (like eMMC) where a drive strength of 2 is
> nonsense for any board, it doesn't seem ridiculous to specify a
> default drive strength of something higher in the soc.dtsi file.

Indeed, that could make sense.

> I would like to say the same thing goes for for pulls, but it's
> unfortunately uglier for pulls. :( For instance, nearly everyone has
> an external pullup for i2c busses. The strength of the pullup needs to
> be tuned for the i2c bus speed and the impedance of the line. Thus, it
> would ideally make sense to specify this in the soc.dtsi file.
> Unfortunately, if we do that and some board _wants_ to use the
> internal pulls (maybe they're running at a really low speed and/or
> forgot to add external pulls) then they have to do an ugly
> "/delete-property/ bias-disable" because adding the "bias-pull-up"
> doesn't delete the other property and you end up with both. :( That
> seems bad, so I guess I'd vote to keep banning bias definitions in the
> soc.dtsi file.

I agree, having to use 'delete-property' to change a pull setting
doesn't seem a good idea.

Re: [PATCH v12 4/4] arm64: dts: qcom: sc7280-herobrine: Add lpi pinmux properties for CRD 3.0/3.1

[Bjorn Andersson]

On Thu 05 May 17:46 PDT 2022, Matthias Kaehlcke wrote:

> On Thu, May 05, 2022 at 05:06:08PM -0700, Doug Anderson wrote:
> > Hi,
> > 
> > On Wed, May 4, 2022 at 10:07 AM Bjorn Andersson
> > <bjorn.andersson@linaro.org> wrote:
> > >
> > > On Fri 29 Apr 11:10 CDT 2022, Doug Anderson wrote:
> > >
> > > > Hi,
> > > >
> > > > On Thu, Apr 28, 2022 at 5:02 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> > > > >
> > > > > On Wed, Apr 27, 2022 at 10:39:43PM +0530, Srinivasa Rao Mandadapu wrote:
> > > > > > Add LPASS LPI pinctrl properties, which are required for Audio
> > > > > > functionality on herobrine based platforms of rev5+
> > > > > > (aka CRD 3.0/3.1) boards.
> > > > > >
> > > > > > Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> > > > > > Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
> > > > > > Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
> > > > >
> > > > > I'm not super firm in pinctrl territory, a few maybe silly questions
> > > > > below.
> > > > >
> > > > > >  arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 84 +++++++++++++++++++++++
> > > > > >  1 file changed, 84 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > > > > > index deaea3a..dfc42df 100644
> > > > > > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > > > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > > > > > @@ -111,6 +111,90 @@ ap_ts_pen_1v8: &i2c13 {
> > > > > >   * - If a pin is not hooked up on Qcard, it gets no name.
> > > > > >   */
> > > > > >
> > > > > > +&lpass_dmic01 {
> > > > > > +     clk {
> > > > > > +             drive-strength = <8>;
> > > > > > +     };
> > > >
> > > > Ugh, I've been distracted and I hadn't realized we were back to the
> > > > two-level syntax. Definitely not my favorite for all the reasons I
> > > > talked about [1]. I guess you took Bjorn's silence to my response to
> > > > mean that you should switch back to this way? :(
> > > >
> > > > Bjorn: can you clarify?
> > > >
> > >
> > > I didn't think through the fact that &mi2s0_state was specified in the
> > > .dtsi and as such will be partially be overridden by the baord dts.
> > >
> > >
> > > I do prefer the two level style and describing full "states", but as you
> > > say whenever we provide something that will have to be overwritten it's
> > > suboptimal.
> > >
> > > As such, I think your flattened model is preferred in this case
> > 
> > How about for future patches we just provided labels at both levels
> > (I'm not suggesting we churn this patch series more):
> > 
> > lpass_dmic01_sleep: dmic01-sleep {
> 
> is the outer label ('lpass_dmic01_sleep') actually needed if we don't
> intend to replicate the hierarchy?
> 

Yes, that's what we put in the pinctrl-N reference from the device node.

> >   lpass_dmic01_sleep_clk: clk {
> >     pins = "gpio6";
> >     function = "dmic1_clk";
> >   };
> > 
> >   lpass_dmic01_sleep_data: data {
> >     pins = "gpio7";
> >     function = "dmic1_data";
> >   };
> > };
> > 

I like this suggestion.

> > Then you can in your pinctrl reference you can just reference the
> > top-level node but boards can override without having to replicate
> > hierarchy...
> > 
> > > but it
> > > makes me dislike the partial definition between the dtsi and dts even
> > > more (but I don't have any better suggestion).
> > 
> > One other proposal I'd make is that maybe we should change the rules
> > about never putting drive strength in the soc.dtsi file. While it
> > should still be OK for boards to override the drive strength, it seems
> > like a whole lot of biolerplate code to have every board override
> > every pin and say that its drive strength is 2. Similarly, if there's
> > a high speed interface (like eMMC) where a drive strength of 2 is
> > nonsense for any board, it doesn't seem ridiculous to specify a
> > default drive strength of something higher in the soc.dtsi file.
> 
> Indeed, that could make sense.
> 

Sounds good to me.

> > I would like to say the same thing goes for for pulls, but it's
> > unfortunately uglier for pulls. :( For instance, nearly everyone has
> > an external pullup for i2c busses. The strength of the pullup needs to
> > be tuned for the i2c bus speed and the impedance of the line. Thus, it
> > would ideally make sense to specify this in the soc.dtsi file.
> > Unfortunately, if we do that and some board _wants_ to use the
> > internal pulls (maybe they're running at a really low speed and/or
> > forgot to add external pulls) then they have to do an ugly
> > "/delete-property/ bias-disable" because adding the "bias-pull-up"
> > doesn't delete the other property and you end up with both. :( That
> > seems bad, so I guess I'd vote to keep banning bias definitions in the
> > soc.dtsi file.
> 
> I agree, having to use 'delete-property' to change a pull setting
> doesn't seem a good idea.

Same.

Regards,
Bjorn