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

[PATCH v7 0/2] 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.
This patch set depends on:
	-- Lpass-lpi pinctrl patches [https://patchwork.kernel.org/project/alsa-devel/list/?series=623951&archive=both&state=*]

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 (2):
  arm64: dts: qcom: sc7280: Add pinmux for I2S speaker and Headset
  arm64: dts: qcom: sc7280: add lpass lpi pin controller node

 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi |  98 ++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc7280.dtsi     | 148 +++++++++++++++++++++++++++++++
 2 files changed, 246 insertions(+)

-- 
2.7.4

[PATCH v7 1/2] 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>
---
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 14 +++++++++++
 arch/arm64/boot/dts/qcom/sc7280.dtsi     | 41 ++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index ecbf2b8..4ba2274 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -462,6 +462,20 @@
 	drive-strength = <10>;
 };
 
+&mi2s1_data0 {
+	drive-strength = <6>;
+	bias-disable;
+};
+
+&mi2s1_sclk {
+	drive-strength = <6>;
+	bias-disable;
+};
+
+&mi2s1_ws {
+	drive-strength = <6>;
+};
+
 &tlmm {
 	bt_en: bt-en {
 		pins = "gpio85";
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index f0b64be..8099c80 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -3527,6 +3527,31 @@
 				function = "pcie1_clkreqn";
 			};
 
+			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";
+			};
+
 			qspi_clk: qspi-clk {
 				pins = "gpio14";
 				function = "qspi_clk";
@@ -4261,6 +4286,22 @@
 				drive-strength = <2>;
 				bias-bus-hold;
 			};
+
+			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";
+			};
+
 		};
 
 		imem@146a5000 {
-- 
2.7.4

Re: [PATCH v7 1/2] arm64: dts: qcom: sc7280: Add pinmux for I2S speaker and Headset

[Matthias Kaehlcke]

On Mon, Apr 11, 2022 at 07:23:03PM +0530, Srinivasa Rao Mandadapu wrote:
> 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>
> ---
>  arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 14 +++++++++++
>  arch/arm64/boot/dts/qcom/sc7280.dtsi     | 41 ++++++++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> index ecbf2b8..4ba2274 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> @@ -462,6 +462,20 @@
>  	drive-strength = <10>;
>  };
>  
> +&mi2s1_data0 {
> +	drive-strength = <6>;
> +	bias-disable;
> +};
> +
> +&mi2s1_sclk {
> +	drive-strength = <6>;
> +	bias-disable;
> +};
> +
> +&mi2s1_ws {
> +	drive-strength = <6>;
> +};
> +

With the new names the nodes should be inserted between 'dp_hot_plug_det' and
'pm7325_gpios'.

>  &tlmm {
>  	bt_en: bt-en {
>  		pins = "gpio85";
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index f0b64be..8099c80 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -3527,6 +3527,31 @@
>  				function = "pcie1_clkreqn";
>  			};
>  
> +			mi2s0_data0: mi2s0-data0 {

Similar as above, the new nodes should be inserted between
'edp_hot_plug_det' and 'pcie1_clkreq_n'.

> +				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";
> +			};
> +
>  			qspi_clk: qspi-clk {
>  				pins = "gpio14";
>  				function = "qspi_clk";
> @@ -4261,6 +4286,22 @@
>  				drive-strength = <2>;
>  				bias-bus-hold;
>  			};
> +
> +			mi2s1_data0: mi2s1-data0 {

see above

> +				pins = "gpio107";
> +				function = "mi2s1_data0";
> +			};
> +
> +			mi2s1_sclk: mi2s1-sclk {
> +				pins = "gpio106";
> +				function = "mi2s1_sck";
> +			};
> +
> +			mi2s1_ws: mi2s1-ws {
> +				pins = "gpio108";
> +				function = "mi2s1_ws";
> +			};

Re: [PATCH v7 1/2] arm64: dts: qcom: sc7280: Add pinmux for I2S speaker and Headset

[Srinivasa Rao Mandadapu]

On 4/12/2022 12:35 AM, Matthias Kaehlcke wrote:
Thanks for your time Matthias!!!
> On Mon, Apr 11, 2022 at 07:23:03PM +0530, Srinivasa Rao Mandadapu wrote:
>> 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>
>> ---
>>   arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 14 +++++++++++
>>   arch/arm64/boot/dts/qcom/sc7280.dtsi     | 41 ++++++++++++++++++++++++++++++++
>>   2 files changed, 55 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> index ecbf2b8..4ba2274 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> @@ -462,6 +462,20 @@
>>   	drive-strength = <10>;
>>   };
>>   
>> +&mi2s1_data0 {
>> +	drive-strength = <6>;
>> +	bias-disable;
>> +};
>> +
>> +&mi2s1_sclk {
>> +	drive-strength = <6>;
>> +	bias-disable;
>> +};
>> +
>> +&mi2s1_ws {
>> +	drive-strength = <6>;
>> +};
>> +
> With the new names the nodes should be inserted between 'dp_hot_plug_det' and
> 'pm7325_gpios'.
Okay. will change accordingly.
>
>>   &tlmm {
>>   	bt_en: bt-en {
>>   		pins = "gpio85";
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index f0b64be..8099c80 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -3527,6 +3527,31 @@
>>   				function = "pcie1_clkreqn";
>>   			};
>>   
>> +			mi2s0_data0: mi2s0-data0 {
> Similar as above, the new nodes should be inserted between
> 'edp_hot_plug_det' and 'pcie1_clkreq_n'.
Okay.
>
>> +				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";
>> +			};
>> +
>>   			qspi_clk: qspi-clk {
>>   				pins = "gpio14";
>>   				function = "qspi_clk";
>> @@ -4261,6 +4286,22 @@
>>   				drive-strength = <2>;
>>   				bias-bus-hold;
>>   			};
>> +
>> +			mi2s1_data0: mi2s1-data0 {
> see above
Okay.
>
>> +				pins = "gpio107";
>> +				function = "mi2s1_data0";
>> +			};
>> +
>> +			mi2s1_sclk: mi2s1-sclk {
>> +				pins = "gpio106";
>> +				function = "mi2s1_sck";
>> +			};
>> +
>> +			mi2s1_ws: mi2s1-ws {
>> +				pins = "gpio108";
>> +				function = "mi2s1_ws";
>> +			};

[PATCH v7 2/2] 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>
---
 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 4ba2274..ea751dc 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -238,6 +238,90 @@
 	modem-init;
 };
 
+&dmic01 {
+	clk {
+		drive-strength = <8>;
+	};
+};
+
+&dmic01_sleep {
+	clk {
+		drive-strength = <2>;
+		bias-disable;
+	};
+
+	data {
+		pull-down;
+	};
+};
+
+&dmic23 {
+	clk {
+		drive-strength = <8>;
+	};
+};
+
+&dmic23_sleep {
+	clk {
+		drive-strength = <2>;
+		bias-disable;
+	};
+
+	data {
+		pull-down;
+	};
+};
+
+&rx_swr {
+	clk {
+		drive-strength = <2>;
+		slew-rate = <1>;
+		bias-disable;
+	};
+
+	data {
+		drive-strength = <2>;
+		slew-rate = <1>;
+		bias-bus-hold;
+	};
+};
+
+&rx_swr_sleep {
+	clk {
+		drive-strength = <2>;
+		bias-pull-down;
+	};
+
+	data {
+		drive-strength = <2>;
+		bias-pull-down;
+	};
+};
+
+&tx_swr {
+	clk {
+		drive-strength = <2>;
+		slew-rate = <1>;
+		bias-disable;
+	};
+
+	data {
+		slew-rate = <1>;
+		bias-bus-hold;
+	};
+};
+
+&tx_swr_sleep {
+	clk {
+		drive-strength = <2>;
+		bias-pull-down;
+	};
+
+	data {
+		bias-bus-hold;
+	};
+};
+
 &pcie1 {
 	status = "okay";
 	perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 8099c80..c692420 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -1987,6 +1987,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>;
+
+			dmic01: dmic01 {
+				clk {
+					pins = "gpio6";
+					function = "dmic1_clk";
+				};
+
+				data {
+					pins = "gpio7";
+					function = "dmic1_data";
+				};
+			};
+
+			dmic01_sleep: dmic01-sleep {
+				clk {
+					pins = "gpio6";
+					function = "dmic1_clk";
+				};
+
+				data {
+					pins = "gpio7";
+					function = "dmic1_data";
+				};
+			};
+
+			dmic23: dmic23 {
+				clk {
+					pins = "gpio8";
+					function = "dmic2_clk";
+				};
+
+				data {
+					pins = "gpio9";
+					function = "dmic2_data";
+				};
+			};
+
+			dmic23_sleep: dmic23_sleep {
+				clk {
+					pins = "gpio8";
+					function = "dmic2_clk";
+				};
+
+				data {
+					pins = "gpio9";
+					function = "dmic2_data";
+				};
+			};
+
+			rx_swr: rx-swr {
+				clk {
+					pins = "gpio3";
+					function = "swr_rx_clk";
+				};
+
+				data {
+					pins = "gpio4", "gpio5";
+					function = "swr_rx_data";
+				};
+			};
+
+			rx_swr_sleep: rx-swr-sleep {
+				clk {
+					pins = "gpio3";
+					function = "swr_rx_clk";
+				};
+
+				data {
+					pins = "gpio4", "gpio5";
+					function = "swr_rx_data";
+				};
+			};
+
+			tx_swr: tx-swr {
+				clk {
+					pins = "gpio0";
+					function = "swr_tx_clk";
+				};
+
+				data {
+					pins = "gpio1", "gpio2", "gpio14";
+					function = "swr_tx_data";
+				};
+			};
+
+			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

Re: [PATCH v7 2/2] arm64: dts: qcom: sc7280: add lpass lpi pin controller node

[Matthias Kaehlcke]

On Mon, Apr 11, 2022 at 07:23:04PM +0530, Srinivasa Rao Mandadapu wrote:
> 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>
> ---
>  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 4ba2274..ea751dc 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> @@ -238,6 +238,90 @@
>  	modem-init;
>  };
>  
> +&dmic01 {

Shouldn't these nodes be in the PINCTRL section at their respective
positions in alphabetical order?

nit: since you are keeping the groups the group names are a bit generic IMO.
e.g. it is fairly obvious that 'dmic01_clk' refers to a clock pin, however
just 'dmic01' is a bit vague. You could consider adding the prefix 'lpass_'
to the group names for more clarity.

> +	clk {
> +		drive-strength = <8>;
> +	};
> +};
> +
> +&dmic01_sleep {
> +	clk {
> +		drive-strength = <2>;
> +		bias-disable;
> +	};
> +
> +	data {
> +		pull-down;
> +	};
> +};
> +
> +&dmic23 {
> +	clk {
> +		drive-strength = <8>;
> +	};
> +};
> +
> +&dmic23_sleep {
> +	clk {
> +		drive-strength = <2>;
> +		bias-disable;
> +	};
> +
> +	data {
> +		pull-down;
> +	};
> +};
> +
> +&rx_swr {
> +	clk {
> +		drive-strength = <2>;
> +		slew-rate = <1>;
> +		bias-disable;
> +	};
> +
> +	data {
> +		drive-strength = <2>;
> +		slew-rate = <1>;
> +		bias-bus-hold;
> +	};
> +};
> +
> +&rx_swr_sleep {
> +	clk {
> +		drive-strength = <2>;
> +		bias-pull-down;
> +	};
> +
> +	data {
> +		drive-strength = <2>;
> +		bias-pull-down;
> +	};
> +};
> +
> +&tx_swr {
> +	clk {
> +		drive-strength = <2>;
> +		slew-rate = <1>;
> +		bias-disable;
> +	};
> +
> +	data {
> +		slew-rate = <1>;
> +		bias-bus-hold;
> +	};
> +};
> +
> +&tx_swr_sleep {
> +	clk {
> +		drive-strength = <2>;
> +		bias-pull-down;
> +	};
> +
> +	data {
> +		bias-bus-hold;
> +	};
> +};
> +
>  &pcie1 {
>  	status = "okay";
>  	perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 8099c80..c692420 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -1987,6 +1987,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>;
> +
> +			dmic01: dmic01 {
> +				clk {
> +					pins = "gpio6";

From the schematics I interpret that the LPASS GPIOs 0-9 are mapped to the
SC7280 GPIOs 144-153. Is that correct?

> +					function = "dmic1_clk";
> +				};
> +
> +				data {
> +					pins = "gpio7";
> +					function = "dmic1_data";
> +				};
> +			};
> +
> +			dmic01_sleep: dmic01-sleep {
> +				clk {
> +					pins = "gpio6";
> +					function = "dmic1_clk";
> +				};
> +
> +				data {
> +					pins = "gpio7";
> +					function = "dmic1_data";
> +				};
> +			};
> +
> +			dmic23: dmic23 {
> +				clk {
> +					pins = "gpio8";
> +					function = "dmic2_clk";
> +				};
> +
> +				data {
> +					pins = "gpio9";
> +					function = "dmic2_data";
> +				};
> +			};
> +
> +			dmic23_sleep: dmic23_sleep {

s/dmic23_sleep/dmic23-sleep/ for the node name.

> +				clk {
> +					pins = "gpio8";
> +					function = "dmic2_clk";
> +				};
> +
> +				data {
> +					pins = "gpio9";
> +					function = "dmic2_data";
> +				};
> +			};
> +
> +			rx_swr: rx-swr {
> +				clk {
> +					pins = "gpio3";
> +					function = "swr_rx_clk";
> +				};
> +
> +				data {
> +					pins = "gpio4", "gpio5";
> +					function = "swr_rx_data";
> +				};
> +			};
> +
> +			rx_swr_sleep: rx-swr-sleep {
> +				clk {
> +					pins = "gpio3";
> +					function = "swr_rx_clk";
> +				};
> +
> +				data {
> +					pins = "gpio4", "gpio5";
> +					function = "swr_rx_data";
> +				};
> +			};
> +
> +			tx_swr: tx-swr {
> +				clk {
> +					pins = "gpio0";
> +					function = "swr_tx_clk";
> +				};
> +
> +				data {
> +					pins = "gpio1", "gpio2", "gpio14";
> +					function = "swr_tx_data";
> +				};
> +			};
> +
> +			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
> 

Re: [PATCH v7 2/2] arm64: dts: qcom: sc7280: add lpass lpi pin controller node

[Srinivasa Rao Mandadapu]

On 4/12/2022 1:02 AM, Matthias Kaehlcke wrote:
Thanks for your time Matthias!!!
> On Mon, Apr 11, 2022 at 07:23:04PM +0530, Srinivasa Rao Mandadapu wrote:
>> 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>
>> ---
>>   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 4ba2274..ea751dc 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> @@ -238,6 +238,90 @@
>>   	modem-init;
>>   };
>>   
>> +&dmic01 {
> Shouldn't these nodes be in the PINCTRL section at their respective
> positions in alphabetical order?

These are not part of tlmm pin control section. These are part of 
lpass_tlmm section.

In your previous comment you asked to remove &lpass_tlmm. Hence brought out.

>
> nit: since you are keeping the groups the group names are a bit generic IMO.
> e.g. it is fairly obvious that 'dmic01_clk' refers to a clock pin, however
> just 'dmic01' is a bit vague. You could consider adding the prefix 'lpass_'
> to the group names for more clarity.
as dmic01 has both clk and data section, I don't think keeping clk is 
appropriate here.
>
>> +	clk {
>> +		drive-strength = <8>;
>> +	};
>> +};
>> +
>> +&dmic01_sleep {
>> +	clk {
>> +		drive-strength = <2>;
>> +		bias-disable;
>> +	};
>> +
>> +	data {
>> +		pull-down;
>> +	};
>> +};
>> +
>> +&dmic23 {
>> +	clk {
>> +		drive-strength = <8>;
>> +	};
>> +};
>> +
>> +&dmic23_sleep {
>> +	clk {
>> +		drive-strength = <2>;
>> +		bias-disable;
>> +	};
>> +
>> +	data {
>> +		pull-down;
>> +	};
>> +};
>> +
>> +&rx_swr {
>> +	clk {
>> +		drive-strength = <2>;
>> +		slew-rate = <1>;
>> +		bias-disable;
>> +	};
>> +
>> +	data {
>> +		drive-strength = <2>;
>> +		slew-rate = <1>;
>> +		bias-bus-hold;
>> +	};
>> +};
>> +
>> +&rx_swr_sleep {
>> +	clk {
>> +		drive-strength = <2>;
>> +		bias-pull-down;
>> +	};
>> +
>> +	data {
>> +		drive-strength = <2>;
>> +		bias-pull-down;
>> +	};
>> +};
>> +
>> +&tx_swr {
>> +	clk {
>> +		drive-strength = <2>;
>> +		slew-rate = <1>;
>> +		bias-disable;
>> +	};
>> +
>> +	data {
>> +		slew-rate = <1>;
>> +		bias-bus-hold;
>> +	};
>> +};
>> +
>> +&tx_swr_sleep {
>> +	clk {
>> +		drive-strength = <2>;
>> +		bias-pull-down;
>> +	};
>> +
>> +	data {
>> +		bias-bus-hold;
>> +	};
>> +};
>> +
>>   &pcie1 {
>>   	status = "okay";
>>   	perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index 8099c80..c692420 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -1987,6 +1987,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>;
>> +
>> +			dmic01: dmic01 {
>> +				clk {
>> +					pins = "gpio6";
>  From the schematics I interpret that the LPASS GPIOs 0-9 are mapped to the
> SC7280 GPIOs 144-153. Is that correct?
Yes. But we refer with GPIOs 0-9 in driver.
>
>> +					function = "dmic1_clk";
>> +				};
>> +
>> +				data {
>> +					pins = "gpio7";
>> +					function = "dmic1_data";
>> +				};
>> +			};
>> +
>> +			dmic01_sleep: dmic01-sleep {
>> +				clk {
>> +					pins = "gpio6";
>> +					function = "dmic1_clk";
>> +				};
>> +
>> +				data {
>> +					pins = "gpio7";
>> +					function = "dmic1_data";
>> +				};
>> +			};
>> +
>> +			dmic23: dmic23 {
>> +				clk {
>> +					pins = "gpio8";
>> +					function = "dmic2_clk";
>> +				};
>> +
>> +				data {
>> +					pins = "gpio9";
>> +					function = "dmic2_data";
>> +				};
>> +			};
>> +
>> +			dmic23_sleep: dmic23_sleep {
> s/dmic23_sleep/dmic23-sleep/ for the node name.
Okay.
>
>> +				clk {
>> +					pins = "gpio8";
>> +					function = "dmic2_clk";
>> +				};
>> +
>> +				data {
>> +					pins = "gpio9";
>> +					function = "dmic2_data";
>> +				};
>> +			};
>> +
>> +			rx_swr: rx-swr {
>> +				clk {
>> +					pins = "gpio3";
>> +					function = "swr_rx_clk";
>> +				};
>> +
>> +				data {
>> +					pins = "gpio4", "gpio5";
>> +					function = "swr_rx_data";
>> +				};
>> +			};
>> +
>> +			rx_swr_sleep: rx-swr-sleep {
>> +				clk {
>> +					pins = "gpio3";
>> +					function = "swr_rx_clk";
>> +				};
>> +
>> +				data {
>> +					pins = "gpio4", "gpio5";
>> +					function = "swr_rx_data";
>> +				};
>> +			};
>> +
>> +			tx_swr: tx-swr {
>> +				clk {
>> +					pins = "gpio0";
>> +					function = "swr_tx_clk";
>> +				};
>> +
>> +				data {
>> +					pins = "gpio1", "gpio2", "gpio14";
>> +					function = "swr_tx_data";
>> +				};
>> +			};
>> +
>> +			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
>>

Re: [PATCH v7 2/2] arm64: dts: qcom: sc7280: add lpass lpi pin controller node

[Srinivasa Rao Mandadapu]

On 4/12/2022 6:18 PM, Srinivasa Rao Mandadapu wrote:
>
> On 4/12/2022 1:02 AM, Matthias Kaehlcke wrote:
> Thanks for your time Matthias!!!
>> On Mon, Apr 11, 2022 at 07:23:04PM +0530, Srinivasa Rao Mandadapu wrote:
>>> 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>
>>> ---
>>>   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 4ba2274..ea751dc 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>> @@ -238,6 +238,90 @@
>>>       modem-init;
>>>   };
>>>   +&dmic01 {
>> Shouldn't these nodes be in the PINCTRL section at their respective
>> positions in alphabetical order?
>
> These are not part of tlmm pin control section. These are part of 
> lpass_tlmm section.
>
> In your previous comment you asked to remove &lpass_tlmm. Hence 
> brought out.
>
>>
>> nit: since you are keeping the groups the group names are a bit 
>> generic IMO.
>> e.g. it is fairly obvious that 'dmic01_clk' refers to a clock pin, 
>> however
>> just 'dmic01' is a bit vague. You could consider adding the prefix 
>> 'lpass_'
>> to the group names for more clarity.
> as dmic01 has both clk and data section, I don't think keeping clk is 
> appropriate here.

As these nodes are part of SC7280, i.e. qcom specific chipset, I feel 
lpass_ is redundant.

If we add lpass_ to all dmic nodes, some node names are too lengthy.

>>
>>> +    clk {
>>> +        drive-strength = <8>;
>>> +    };
>>> +};
>>> +
>>> +&dmic01_sleep {
>>> +    clk {
>>> +        drive-strength = <2>;
>>> +        bias-disable;
>>> +    };
>>> +
>>> +    data {
>>> +        pull-down;
>>> +    };
>>> +};
>>> +
>>> +&dmic23 {
>>> +    clk {
>>> +        drive-strength = <8>;
>>> +    };
>>> +};
>>> +
>>> +&dmic23_sleep {
>>> +    clk {
>>> +        drive-strength = <2>;
>>> +        bias-disable;
>>> +    };
>>> +
>>> +    data {
>>> +        pull-down;
>>> +    };
>>> +};
>>> +
>>> +&rx_swr {
>>> +    clk {
>>> +        drive-strength = <2>;
>>> +        slew-rate = <1>;
>>> +        bias-disable;
>>> +    };
>>> +
>>> +    data {
>>> +        drive-strength = <2>;
>>> +        slew-rate = <1>;
>>> +        bias-bus-hold;
>>> +    };
>>> +};
>>> +
>>> +&rx_swr_sleep {
>>> +    clk {
>>> +        drive-strength = <2>;
>>> +        bias-pull-down;
>>> +    };
>>> +
>>> +    data {
>>> +        drive-strength = <2>;
>>> +        bias-pull-down;
>>> +    };
>>> +};
>>> +
>>> +&tx_swr {
>>> +    clk {
>>> +        drive-strength = <2>;
>>> +        slew-rate = <1>;
>>> +        bias-disable;
>>> +    };
>>> +
>>> +    data {
>>> +        slew-rate = <1>;
>>> +        bias-bus-hold;
>>> +    };
>>> +};
>>> +
>>> +&tx_swr_sleep {
>>> +    clk {
>>> +        drive-strength = <2>;
>>> +        bias-pull-down;
>>> +    };
>>> +
>>> +    data {
>>> +        bias-bus-hold;
>>> +    };
>>> +};
>>> +
>>>   &pcie1 {
>>>       status = "okay";
>>>       perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi 
>>> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> index 8099c80..c692420 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> @@ -1987,6 +1987,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>;
>>> +
>>> +            dmic01: dmic01 {
>>> +                clk {
>>> +                    pins = "gpio6";
>>  From the schematics I interpret that the LPASS GPIOs 0-9 are mapped 
>> to the
>> SC7280 GPIOs 144-153. Is that correct?
> Yes. But we refer with GPIOs 0-9 in driver.
>>
>>> +                    function = "dmic1_clk";
>>> +                };
>>> +
>>> +                data {
>>> +                    pins = "gpio7";
>>> +                    function = "dmic1_data";
>>> +                };
>>> +            };
>>> +
>>> +            dmic01_sleep: dmic01-sleep {
>>> +                clk {
>>> +                    pins = "gpio6";
>>> +                    function = "dmic1_clk";
>>> +                };
>>> +
>>> +                data {
>>> +                    pins = "gpio7";
>>> +                    function = "dmic1_data";
>>> +                };
>>> +            };
>>> +
>>> +            dmic23: dmic23 {
>>> +                clk {
>>> +                    pins = "gpio8";
>>> +                    function = "dmic2_clk";
>>> +                };
>>> +
>>> +                data {
>>> +                    pins = "gpio9";
>>> +                    function = "dmic2_data";
>>> +                };
>>> +            };
>>> +
>>> +            dmic23_sleep: dmic23_sleep {
>> s/dmic23_sleep/dmic23-sleep/ for the node name.
> Okay.
>>
>>> +                clk {
>>> +                    pins = "gpio8";
>>> +                    function = "dmic2_clk";
>>> +                };
>>> +
>>> +                data {
>>> +                    pins = "gpio9";
>>> +                    function = "dmic2_data";
>>> +                };
>>> +            };
>>> +
>>> +            rx_swr: rx-swr {
>>> +                clk {
>>> +                    pins = "gpio3";
>>> +                    function = "swr_rx_clk";
>>> +                };
>>> +
>>> +                data {
>>> +                    pins = "gpio4", "gpio5";
>>> +                    function = "swr_rx_data";
>>> +                };
>>> +            };
>>> +
>>> +            rx_swr_sleep: rx-swr-sleep {
>>> +                clk {
>>> +                    pins = "gpio3";
>>> +                    function = "swr_rx_clk";
>>> +                };
>>> +
>>> +                data {
>>> +                    pins = "gpio4", "gpio5";
>>> +                    function = "swr_rx_data";
>>> +                };
>>> +            };
>>> +
>>> +            tx_swr: tx-swr {
>>> +                clk {
>>> +                    pins = "gpio0";
>>> +                    function = "swr_tx_clk";
>>> +                };
>>> +
>>> +                data {
>>> +                    pins = "gpio1", "gpio2", "gpio14";
>>> +                    function = "swr_tx_data";
>>> +                };
>>> +            };
>>> +
>>> +            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
>>>

Re: [PATCH v7 2/2] arm64: dts: qcom: sc7280: add lpass lpi pin controller node

[Matthias Kaehlcke]

On Tue, Apr 12, 2022 at 06:41:25PM +0530, Srinivasa Rao Mandadapu wrote:
> 
> On 4/12/2022 6:18 PM, Srinivasa Rao Mandadapu wrote:
> > 
> > On 4/12/2022 1:02 AM, Matthias Kaehlcke wrote:
> > Thanks for your time Matthias!!!
> > > On Mon, Apr 11, 2022 at 07:23:04PM +0530, Srinivasa Rao Mandadapu wrote:
> > > > 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>
> > > > ---
> > > >   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 4ba2274..ea751dc 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> > > > @@ -238,6 +238,90 @@
> > > >       modem-init;
> > > >   };
> > > >   +&dmic01 {
> > > Shouldn't these nodes be in the PINCTRL section at their respective
> > > positions in alphabetical order?
> > 
> > These are not part of tlmm pin control section. These are part of
> > lpass_tlmm section.
> > 
> > In your previous comment you asked to remove &lpass_tlmm. Hence brought
> > out.
> > 
> > > 
> > > nit: since you are keeping the groups the group names are a bit
> > > generic IMO.
> > > e.g. it is fairly obvious that 'dmic01_clk' refers to a clock pin,
> > > however
> > > just 'dmic01' is a bit vague. You could consider adding the prefix
> > > 'lpass_'
> > > to the group names for more clarity.
> > as dmic01 has both clk and data section, I don't think keeping clk is
> > appropriate here.
> 
> As these nodes are part of SC7280, i.e. qcom specific chipset, I feel lpass_
> is redundant.

It helps to provide some context about the pins which might not be evident
from their short names like 'dmic01' or 'rx_swr'. A nice side effect is that
the pins/groups would grouped automatically together in alphabetic ordering.

In terms of 'redundancy' it is similar to 'qup_' prefix for the I2C/SPI/UART
pins.

> If we add lpass_ to all dmic nodes, some node names are too lengthy.

The longest would be like 'lpass_dmic01_sleep' or 'lpass_rx_swr_sleep', which
doesn't seem outrageous.

In any case it's not super important. If it bothers someone enough later
on they can always send a patch that changes it.

Re: [PATCH v7 2/2] arm64: dts: qcom: sc7280: add lpass lpi pin controller node

[Srinivasa Rao Mandadapu]

On 4/13/2022 5:29 AM, Matthias Kaehlcke wrote:
Thanks for your time and valuable suggestions Matthias!!!
> On Tue, Apr 12, 2022 at 06:41:25PM +0530, Srinivasa Rao Mandadapu wrote:
>> On 4/12/2022 6:18 PM, Srinivasa Rao Mandadapu wrote:
>>> On 4/12/2022 1:02 AM, Matthias Kaehlcke wrote:
>>> Thanks for your time Matthias!!!
>>>> On Mon, Apr 11, 2022 at 07:23:04PM +0530, Srinivasa Rao Mandadapu wrote:
>>>>> 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>
>>>>> ---
>>>>>    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 4ba2274..ea751dc 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>>>> @@ -238,6 +238,90 @@
>>>>>        modem-init;
>>>>>    };
>>>>>    +&dmic01 {
>>>> Shouldn't these nodes be in the PINCTRL section at their respective
>>>> positions in alphabetical order?
>>> These are not part of tlmm pin control section. These are part of
>>> lpass_tlmm section.
>>>
>>> In your previous comment you asked to remove &lpass_tlmm. Hence brought
>>> out.
>>>
>>>> nit: since you are keeping the groups the group names are a bit
>>>> generic IMO.
>>>> e.g. it is fairly obvious that 'dmic01_clk' refers to a clock pin,
>>>> however
>>>> just 'dmic01' is a bit vague. You could consider adding the prefix
>>>> 'lpass_'
>>>> to the group names for more clarity.
>>> as dmic01 has both clk and data section, I don't think keeping clk is
>>> appropriate here.
>> As these nodes are part of SC7280, i.e. qcom specific chipset, I feel lpass_
>> is redundant.
> It helps to provide some context about the pins which might not be evident
> from their short names like 'dmic01' or 'rx_swr'. A nice side effect is that
> the pins/groups would grouped automatically together in alphabetic ordering.
>
> In terms of 'redundancy' it is similar to 'qup_' prefix for the I2C/SPI/UART
> pins.
Agree. Will change accordingly. similarly will append lpass_ torx/tx/va 
mcro device node names.
>
>> If we add lpass_ to all dmic nodes, some node names are too lengthy.
> The longest would be like 'lpass_dmic01_sleep' or 'lpass_rx_swr_sleep', which
> doesn't seem outrageous.
>
> In any case it's not super important. If it bothers someone enough later
> on they can always send a patch that changes it.
Okay.

Re: [PATCH v7 2/2] arm64: dts: qcom: sc7280: add lpass lpi pin controller node

[Matthias Kaehlcke]

On Wed, Apr 13, 2022 at 08:12:20PM +0530, Srinivasa Rao Mandadapu wrote:
> 
> On 4/13/2022 5:29 AM, Matthias Kaehlcke wrote:
> Thanks for your time and valuable suggestions Matthias!!!
> > On Tue, Apr 12, 2022 at 06:41:25PM +0530, Srinivasa Rao Mandadapu wrote:
> > > On 4/12/2022 6:18 PM, Srinivasa Rao Mandadapu wrote:
> > > > On 4/12/2022 1:02 AM, Matthias Kaehlcke wrote:
> > > > Thanks for your time Matthias!!!
> > > > > On Mon, Apr 11, 2022 at 07:23:04PM +0530, Srinivasa Rao Mandadapu wrote:
> > > > > > 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>
> > > > > > ---
> > > > > >    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 4ba2274..ea751dc 100644
> > > > > > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> > > > > > @@ -238,6 +238,90 @@
> > > > > >        modem-init;
> > > > > >    };
> > > > > >    +&dmic01 {
> > > > > Shouldn't these nodes be in the PINCTRL section at their respective
> > > > > positions in alphabetical order?
> > > > These are not part of tlmm pin control section. These are part of
> > > > lpass_tlmm section.
> > > > 
> > > > In your previous comment you asked to remove &lpass_tlmm. Hence brought
> > > > out.
> > > > 
> > > > > nit: since you are keeping the groups the group names are a bit
> > > > > generic IMO.
> > > > > e.g. it is fairly obvious that 'dmic01_clk' refers to a clock pin,
> > > > > however
> > > > > just 'dmic01' is a bit vague. You could consider adding the prefix
> > > > > 'lpass_'
> > > > > to the group names for more clarity.
> > > > as dmic01 has both clk and data section, I don't think keeping clk is
> > > > appropriate here.
> > > As these nodes are part of SC7280, i.e. qcom specific chipset, I feel lpass_
> > > is redundant.
> > It helps to provide some context about the pins which might not be evident
> > from their short names like 'dmic01' or 'rx_swr'. A nice side effect is that
> > the pins/groups would grouped automatically together in alphabetic ordering.
> > 
> > In terms of 'redundancy' it is similar to 'qup_' prefix for the I2C/SPI/UART
> > pins.
> Agree. Will change accordingly. similarly will append lpass_ torx/tx/va mcro
> device node names.
> > 
> > > If we add lpass_ to all dmic nodes, some node names are too lengthy.
> > The longest would be like 'lpass_dmic01_sleep' or 'lpass_rx_swr_sleep', which
> > doesn't seem outrageous.
> > 
> > In any case it's not super important. If it bothers someone enough later
> > on they can always send a patch that changes it.
> Okay.

I meant to say whether you change it or not is not super important, it's up
to you :)

Re: [PATCH v7 2/2] arm64: dts: qcom: sc7280: add lpass lpi pin controller node

[Matthias Kaehlcke]

On Tue, Apr 12, 2022 at 06:18:33PM +0530, Srinivasa Rao Mandadapu wrote:
> 
> On 4/12/2022 1:02 AM, Matthias Kaehlcke wrote:
> Thanks for your time Matthias!!!
> > On Mon, Apr 11, 2022 at 07:23:04PM +0530, Srinivasa Rao Mandadapu wrote:
> > > 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>
> > > ---
> > >   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 4ba2274..ea751dc 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> > > @@ -238,6 +238,90 @@
> > >   	modem-init;
> > >   };
> > > +&dmic01 {
> > Shouldn't these nodes be in the PINCTRL section at their respective
> > positions in alphabetical order?
> 
> These are not part of tlmm pin control section. These are part of lpass_tlmm
> section.

Agreed, these shouldn't be in the tlmm section, but it still seems nice to
separate them from device nodes.

Some sc7180-trogdor devices have a section like this:

/* PINCTRL - modifications to sc7180-trogdor.dtsi */

In any case I don't care too much about the IDP, but this might come up again
for sc7280-herobrine boards.

> In your previous comment you asked to remove &lpass_tlmm. Hence brought out.
> 
> > 
> > nit: since you are keeping the groups the group names are a bit generic IMO.
> > e.g. it is fairly obvious that 'dmic01_clk' refers to a clock pin, however
> > just 'dmic01' is a bit vague. You could consider adding the prefix 'lpass_'
> > to the group names for more clarity.
> as dmic01 has both clk and data section, I don't think keeping clk is
> appropriate here.

Agreed, _clk isn't appropriate as long as it's a group of pins, that would be
for a label per pin.

> >  From the schematics I interpret that the LPASS GPIOs 0-9 are mapped to the
> > SC7280 GPIOs 144-153. Is that correct?
> Yes. But we refer with GPIOs 0-9 in driver.

Thanks, just wanted to make sure my understanding is correct.