[PATCH v8 0/4] Add soundcard support for sc7280 based platforms.

[PATCH v8 0/4] Add soundcard support for sc7280 based platforms.

[Srinivasa Rao Mandadapu]

This patch set is to add bolero digital macros, WCD and maxim codecs nodes
for audio on sc7280 based platforms.

This patch set depends on:
    -- https://patchwork.kernel.org/project/linux-arm-msm/list/?series=631506
    -- https://patchwork.kernel.org/project/linux-arm-msm/list/?series=601249
    -- Clock reset control patches

Changes Since V7:
    -- Remove redundant interrupt names in soundwire node.
    -- Fix typo errors.
    -- Remove redundant reg property in sound node.
    -- Rebased on top of latest kernel tip.
Changes Since V6:
    -- Modify link-names and audio routing in a sound node.
    -- Move amp_en pin control node to appropriate consumer patch.
    -- Split patches as per digital macro codecs and board specific codecs and sort it.
    -- Modify label and node names to lpass specific.
Changes Since V5:
    -- Move soc specific bolero digital codec nodes to soc specific file.
    -- Bring wcd938x codec reset pin control and US/EURO HS selection nodes from other series.
    -- Change node name and remove redundant status property in sound node.
Changes Since V4:
    -- Update nodes in sorting order.
    -- Update DTS node names as per dt-bindings.
    -- Update Node properties in proper order.
    -- Update missing pinctrl properties like US/EURO HS selection, wcd reset control.
    -- Remove redundant labels.
    -- Remove unused size cells and address cells in tx macro node.
    -- Keep all same nodes at one place, which are defined in same file.
    -- Add max98360a codec node to herobrine board specific targets.
Changes Since V3:
    -- Move digital codec macro nodes to board specific dtsi file.
    -- Update pin controls in lpass cpu node.
    -- Update dependency patch list.
    -- Create patches on latest kernel.
Changes Since V2:
    -- Add power domains to digital codec macro nodes.
    -- Change clock node usage in lpass cpu node.
    -- Add codec mem clock to lpass cpu node.
    -- Modify the node names to be generic.
    -- Move sound and codec nodes to root node.
    -- sort dai links as per reg.
    -- Fix typo errors.
Changes Since V1:
    -- Update the commit message of cpu node patch.
    -- Add gpio control property to support Euro headset in wcd938x node.
    -- Fix clock properties in lpass cpu and digital codec macro node.

Srinivasa Rao Mandadapu (4):
  arm64: dts: qcom: sc7280: Add nodes for soundwire and va tx rx digital
    macro codecs
  arm64: dts: qcom: sc7280: Add nodes for wcd9385 and max98360a codec
  arm64: dts: qcom: sc7280: Add lpass cpu node
  arm64: dts: qcom: sc7280: Add dt nodes for sound card

 arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts     |  29 ++++
 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi |   8 +
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi       | 218 +++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc7280.dtsi           | 190 +++++++++++++++++++++
 4 files changed, 445 insertions(+)

-- 
2.7.4

[PATCH v8 1/4] arm64: dts: qcom: sc7280: Add nodes for soundwire and va tx rx digital macro codecs

[Srinivasa Rao Mandadapu]

SC7280 has VA, TX and RX macros with SoundWire Controllers to attach with
external codecs using soundwire masters. Add these nodes for sc7280 based
platforms audio use case.

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.dtsi | 128 +++++++++++++++++++++++++++++++++++
 1 file changed, 128 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index f428344..23e09fa 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -2035,6 +2035,114 @@
 			#clock-cells = <1>;
 		};
 
+		lpass_rx_macro: codec@3200000 {
+			compatible = "qcom,sc7280-lpass-rx-macro";
+			reg = <0 0x03200000 0 0x1000>;
+
+			pinctrl-names = "default";
+			pinctrl-0 = <&lpass_rx_swr>;
+
+			clocks = <&lpass_aon LPASS_AON_CC_TX_MCLK_CLK>,
+				 <&lpass_aon LPASS_AON_CC_TX_MCLK_2X_CLK>,
+				 <&lpass_va_macro>;
+			clock-names = "mclk", "npl", "fsgen";
+
+			power-domains = <&lpass_hm LPASS_CORE_CC_LPASS_CORE_HM_GDSC>,
+					<&lpass_aon LPASS_AON_CC_LPASS_AUDIO_HM_GDSC>;
+			power-domain-names = "macro", "dcodec";
+
+			#clock-cells = <0>;
+			#sound-dai-cells = <1>;
+
+			status = "disabled";
+		};
+
+		swr0: soundwire@3210000 {
+			compatible = "qcom,soundwire-v1.6.0";
+			reg = <0 0x03210000 0 0x2000>;
+
+			interrupts = <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&lpass_rx_macro>;
+			clock-names = "iface";
+
+			qcom,din-ports = <0>;
+			qcom,dout-ports = <5>;
+
+			resets = <&lpass_audiocc LPASS_AUDIO_SWR_RX_CGCR>;
+			reset-names = "swr_audio_cgcr";
+
+			qcom,ports-word-length =	/bits/ 8 <0x01 0x07 0x04 0xff 0xff>;
+			qcom,ports-sinterval-low =	/bits/ 8 <0x03 0x3f 0x1f 0x03 0x03>;
+			qcom,ports-offset1 =		/bits/ 8 <0x00 0x00 0x0b 0x01 0x01>;
+			qcom,ports-offset2 =		/bits/ 8 <0x00 0x00 0x0b 0x00 0x00>;
+			qcom,ports-lane-control =	/bits/ 8 <0x01 0x00 0x00 0x00 0x00>;
+			qcom,ports-block-pack-mode =	/bits/ 8 <0xff 0x00 0x01 0xff 0xff>;
+			qcom,ports-hstart =		/bits/ 8 <0xff 0x03 0xff 0xff 0xff>;
+			qcom,ports-hstop =		/bits/ 8 <0xff 0x06 0xff 0xff 0xff>;
+			qcom,ports-block-group-count =	/bits/ 8 <0xff 0xff 0xff 0xff 0x00>;
+
+			#sound-dai-cells = <1>;
+			#address-cells = <2>;
+			#size-cells = <0>;
+
+			status = "disabled";
+		};
+
+		lpass_tx_macro: codec@3220000 {
+			compatible = "qcom,sc7280-lpass-tx-macro";
+			reg = <0 0x03220000 0 0x1000>;
+
+			pinctrl-names = "default";
+			pinctrl-0 = <&lpass_tx_swr>;
+
+			clocks = <&lpass_aon LPASS_AON_CC_TX_MCLK_CLK>,
+				 <&lpass_aon LPASS_AON_CC_TX_MCLK_2X_CLK>,
+				 <&lpass_va_macro>;
+			clock-names = "mclk", "npl", "fsgen";
+
+			power-domains = <&lpass_hm LPASS_CORE_CC_LPASS_CORE_HM_GDSC>,
+					<&lpass_aon LPASS_AON_CC_LPASS_AUDIO_HM_GDSC>;
+			power-domain-names = "macro", "dcodec";
+
+			#clock-cells = <0>;
+			#sound-dai-cells = <1>;
+
+			status = "disabled";
+		};
+
+		swr1: soundwire@3230000 {
+			compatible = "qcom,soundwire-v1.6.0";
+			reg = <0 0x03230000 0 0x2000>;
+
+			interrupts-extended = <&intc GIC_SPI 496 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 130 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&lpass_tx_macro>;
+			clock-names = "iface";
+
+			qcom,din-ports = <3>;
+			qcom,dout-ports = <0>;
+
+			resets = <&lpass_audiocc LPASS_AUDIO_SWR_TX_CGCR>;
+			reset-names = "swr_audio_cgcr";
+
+			qcom,ports-sinterval-low =	/bits/ 8 <0x01 0x03 0x03>;
+			qcom,ports-offset1 =		/bits/ 8 <0x01 0x00 0x02>;
+			qcom,ports-offset2 =		/bits/ 8 <0x00 0x00 0x00>;
+			qcom,ports-hstart =		/bits/ 8 <0xff 0xff 0xff>;
+			qcom,ports-hstop =		/bits/ 8 <0xff 0xff 0xff>;
+			qcom,ports-word-length =	/bits/ 8 <0xff 0x00 0xff>;
+			qcom,ports-block-pack-mode =	/bits/ 8 <0xff 0xff 0xff>;
+			qcom,ports-block-group-count =	/bits/ 8 <0xff 0xff 0xff>;
+			qcom,ports-lane-control =	/bits/ 8 <0x00 0x01 0x00>;
+			qcom,port-offset = <1>;
+
+			#sound-dai-cells = <1>;
+			#address-cells = <2>;
+			#size-cells = <0>;
+
+			status = "disabled";
+		};
+
 		lpass_audiocc: clock-controller@3300000 {
 			compatible = "qcom,sc7280-lpassaudiocc";
 			reg = <0 0x03300000 0 0x30000>;
@@ -2046,6 +2154,26 @@
 			#power-domain-cells = <1>;
 		};
 
+		lpass_va_macro: codec@3370000 {
+			compatible = "qcom,sc7280-lpass-va-macro";
+			reg = <0 0x03370000 0 0x1000>;
+
+			pinctrl-0 = <&lpass_dmic01>;
+			pinctrl-names = "default";
+
+			clocks = <&lpass_aon LPASS_AON_CC_TX_MCLK_CLK>;
+			clock-names = "mclk";
+
+			power-domains = <&lpass_hm LPASS_CORE_CC_LPASS_CORE_HM_GDSC>,
+					<&lpass_aon LPASS_AON_CC_LPASS_AUDIO_HM_GDSC>;
+			power-domain-names = "macro", "dcodec";
+
+			#clock-cells = <0>;
+			#sound-dai-cells = <1>;
+
+			status = "disabled";
+		};
+
 		lpass_aon: clock-controller@3380000 {
 			compatible = "qcom,sc7280-lpassaoncc";
 			reg = <0 0x03380000 0 0x30000>;
-- 
2.7.4

[PATCH v8 2/4] arm64: dts: qcom: sc7280: Add nodes for wcd9385 and max98360a codec

[Srinivasa Rao Mandadapu]

Add wcd938x and  max98360a codecs for audio use case on
sc7280 based platforms.
Add tlmm gpio property in wcd938x node for switching CTIA/OMTP Headset.
Add amp_en node for  max98360a codec.

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-crd-r3.dts     |  6 ++
 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi |  8 +++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi       | 97 ++++++++++++++++++++++++++
 3 files changed, 111 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
index 344338a..aa0bf6e2 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
@@ -87,6 +87,12 @@ ap_ts_pen_1v8: &i2c13 {
 	pins = "gpio51";
 };
 
+&wcd938x {
+	pinctrl-names = "default";
+	pinctrl-0 = <&us_euro_hs_sel>;
+	us-euro-gpios = <&tlmm 81 GPIO_ACTIVE_HIGH>;
+};
+
 &tlmm {
 	tp_int_odl: tp-int-odl {
 		pins = "gpio7";
diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
index d58045d..f247403 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
@@ -20,6 +20,14 @@
 #include "sc7280-chrome-common.dtsi"
 
 / {
+	max98360a: audio-codec-0 {
+		compatible = "maxim,max98360a";
+		pinctrl-names = "default";
+		pinctrl-0 = <&amp_en>;
+		sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
+		#sound-dai-cells = <0>;
+	};
+
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index 2f863c0..8dad599 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -20,6 +20,42 @@
 		serial1 = &uart7;
 	};
 
+	max98360a: audio-codec-0 {
+		compatible = "maxim,max98360a";
+		pinctrl-names = "default";
+		pinctrl-0 = <&amp_en>;
+		sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
+		#sound-dai-cells = <0>;
+	};
+
+	wcd938x: audio-codec-1 {
+		compatible = "qcom,wcd9385-codec";
+		pinctrl-names = "default", "sleep";
+		pinctrl-0 = <&wcd_reset_n>;
+		pinctrl-1 = <&wcd_reset_n_sleep>;
+
+		reset-gpios = <&tlmm 83 GPIO_ACTIVE_HIGH>;
+
+		qcom,rx-device = <&wcd_rx>;
+		qcom,tx-device = <&wcd_tx>;
+
+		vdd-rxtx-supply = <&vreg_l18b_1p8>;
+		vdd-io-supply = <&vreg_l18b_1p8>;
+		vdd-buck-supply = <&vreg_l17b_1p8>;
+		vdd-mic-bias-supply = <&vreg_bob>;
+
+		qcom,micbias1-microvolt = <1800000>;
+		qcom,micbias2-microvolt = <1800000>;
+		qcom,micbias3-microvolt = <1800000>;
+		qcom,micbias4-microvolt = <1800000>;
+
+		qcom,mbhc-buttons-vthreshold-microvolt = <75000 150000 237000 500000 500000
+							  500000 500000 500000>;
+		qcom,mbhc-headset-vthreshold-microvolt = <1700000>;
+		qcom,mbhc-headphone-vthreshold-microvolt = <50000>;
+		#sound-dai-cells = <1>;
+	};
+
 	gpio-keys {
 		compatible = "gpio-keys";
 		label = "gpio-keys";
@@ -238,6 +274,19 @@
 	modem-init;
 };
 
+&lpass_rx_macro {
+	status = "okay";
+};
+
+&lpass_tx_macro {
+	status = "okay";
+};
+
+&lpass_va_macro {
+	status = "okay";
+	vdd-micb-supply = <&vreg_bob>;
+};
+
 &pcie1 {
 	status = "okay";
 	perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
@@ -298,6 +347,26 @@
 	cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;
 };
 
+&swr0 {
+	status = "okay";
+	wcd_rx: codec@0,4 {
+		compatible = "sdw20217010d00";
+		reg = <0 4>;
+		#sound-dai-cells = <1>;
+		qcom,rx-port-mapping = <1 2 3 4 5>;
+	};
+};
+
+&swr1 {
+	status = "okay";
+	wcd_tx: codec@0,3 {
+		compatible = "sdw20217010d00";
+		reg = <0 3>;
+		#sound-dai-cells = <1>;
+		qcom,tx-port-mapping = <1 2 3 4>;
+	};
+};
+
 &uart5 {
 	compatible = "qcom,geni-debug-uart";
 	status = "okay";
@@ -561,6 +630,12 @@
 };
 
 &tlmm {
+	amp_en: amp-en {
+		pins = "gpio63";
+		bias-pull-down;
+		drive-strength = <2>;
+	};
+
 	bt_en: bt-en {
 		pins = "gpio85";
 		function = "gpio";
@@ -643,6 +718,28 @@
 		function = "gpio";
 		bias-pull-down;
 	};
+
+	us_euro_hs_sel: us-euro-hs-sel {
+		pins = "gpio81";
+		function = "gpio";
+		bias-pull-down;
+		drive-strength = <2>;
+	};
+
+	wcd_reset_n: wcd-reset-n {
+		pins = "gpio83";
+		function = "gpio";
+		drive-strength = <8>;
+		output-high;
+	};
+
+	wcd_reset_n_sleep: wcd-reset-n-sleep {
+		pins = "gpio83";
+		function = "gpio";
+		drive-strength = <8>;
+		bias-disable;
+		output-low;
+	};
 };
 
 &remoteproc_wpss {
-- 
2.7.4

[PATCH v8 3/4] arm64: dts: qcom: sc7280: Add lpass cpu node

[Srinivasa Rao Mandadapu]

Add lpass cpu node for audio 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: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 28 +++++++++++++++
 arch/arm64/boot/dts/qcom/sc7280.dtsi     | 62 ++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index 8dad599..e880837 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -274,6 +274,34 @@
 	modem-init;
 };
 
+&lpass_cpu {
+	status = "okay";
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&mi2s1_data0>, <&mi2s1_sclk>, <&mi2s1_ws>;
+
+	dai-link@1 {
+		reg = <MI2S_SECONDARY>;
+		qcom,playback-sd-lines = <0>;
+	};
+
+	dai-link@5 {
+		reg = <LPASS_DP_RX>;
+	};
+
+	dai-link@6 {
+		reg = <LPASS_CDC_DMA_RX0>;
+	};
+
+	dai-link@19 {
+		reg = <LPASS_CDC_DMA_TX3>;
+	};
+
+	dai-link@25 {
+		reg = <LPASS_CDC_DMA_VA_TX0>;
+	};
+};
+
 &lpass_rx_macro {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 23e09fa..e355c33 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -21,6 +21,7 @@
 #include <dt-bindings/reset/qcom,sdm845-aoss.h>
 #include <dt-bindings/reset/qcom,sdm845-pdc.h>
 #include <dt-bindings/soc/qcom,rpmh-rsc.h>
+#include <dt-bindings/sound/qcom,lpass.h>
 #include <dt-bindings/thermal/thermal.h>
 
 / {
@@ -2195,6 +2196,67 @@
 			#power-domain-cells = <1>;
 		};
 
+		lpass_cpu: audio@3987000 {
+			compatible = "qcom,sc7280-lpass-cpu";
+
+			reg = <0 0x03987000 0 0x68000>,
+			      <0 0x03b00000 0 0x29000>,
+			      <0 0x03260000 0 0xc000>,
+			      <0 0x03280000 0 0x29000>,
+			      <0 0x03340000 0 0x29000>,
+			      <0 0x0336c000 0 0x3000>;
+			reg-names = "lpass-hdmiif",
+				    "lpass-lpaif",
+				    "lpass-rxtx-cdc-dma-lpm",
+				    "lpass-rxtx-lpaif",
+				    "lpass-va-lpaif",
+				    "lpass-va-cdc-dma-lpm";
+
+			iommus = <&apps_smmu 0x1820 0>,
+				 <&apps_smmu 0x1821 0>,
+				 <&apps_smmu 0x1832 0>;
+
+			power-domains =	<&rpmhpd SC7280_LCX>;
+			power-domain-names = "lcx";
+			required-opps = <&rpmhpd_opp_nom>;
+
+			clocks = <&lpass_aon LPASS_AON_CC_AUDIO_HM_H_CLK>,
+				 <&lpass_audiocc LPASS_AUDIO_CC_EXT_MCLK0_CLK>,
+				 <&lpasscore LPASS_CORE_CC_SYSNOC_MPORT_CORE_CLK>,
+				 <&lpasscore LPASS_CORE_CC_EXT_IF0_IBIT_CLK>,
+				 <&lpasscore LPASS_CORE_CC_EXT_IF1_IBIT_CLK>,
+				 <&lpass_audiocc LPASS_AUDIO_CC_CODEC_MEM_CLK>,
+				 <&lpass_audiocc LPASS_AUDIO_CC_CODEC_MEM0_CLK>,
+				 <&lpass_audiocc LPASS_AUDIO_CC_CODEC_MEM1_CLK>,
+				 <&lpass_audiocc LPASS_AUDIO_CC_CODEC_MEM2_CLK>,
+				 <&lpass_aon LPASS_AON_CC_VA_MEM0_CLK>;
+			clock-names = "aon_cc_audio_hm_h",
+				      "audio_cc_ext_mclk0",
+				      "core_cc_sysnoc_mport_core",
+				      "core_cc_ext_if0_ibit",
+				      "core_cc_ext_if1_ibit",
+				      "audio_cc_codec_mem",
+				      "audio_cc_codec_mem0",
+				      "audio_cc_codec_mem1",
+				      "audio_cc_codec_mem2",
+				      "aon_cc_va_mem0";
+
+			#sound-dai-cells = <1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 180 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "lpass-irq-lpaif",
+					  "lpass-irq-hdmi",
+					  "lpass-irq-vaif",
+					  "lpass-irq-rxtxif";
+
+			status = "disabled";
+		};
+
 		lpass_hm: clock-controller@3c00000 {
 			compatible = "qcom,sc7280-lpasshm";
 			reg = <0 0x3c00000 0 0x28>;
-- 
2.7.4

[PATCH v8 4/4] arm64: dts: qcom: sc7280: Add dt nodes for sound card

[Srinivasa Rao Mandadapu]

Add dt nodes for sound card support, which is using WCD938x headset
playback, capture, I2S speaker playback and DMICs via VA macro.

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-crd-r3.dts | 23 ++++++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi   | 93 ++++++++++++++++++++++++++++++
 2 files changed, 116 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
index aa0bf6e2..bf15bbe 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
@@ -87,6 +87,29 @@ ap_ts_pen_1v8: &i2c13 {
 	pins = "gpio51";
 };
 
+&sound {
+	audio-routing =
+		"IN1_HPHL", "HPHL_OUT",
+		"IN2_HPHR", "HPHR_OUT",
+		"AMIC1", "MIC BIAS1",
+		"AMIC2", "MIC BIAS2",
+		"VA DMIC0", "MIC BIAS1",
+		"VA DMIC1", "MIC BIAS1",
+		"VA DMIC2", "MIC BIAS3",
+		"VA DMIC3", "MIC BIAS3",
+		"TX SWR_ADC0", "ADC1_OUTPUT",
+		"TX SWR_ADC1", "ADC2_OUTPUT",
+		"TX SWR_ADC2", "ADC3_OUTPUT",
+		"TX SWR_DMIC0", "DMIC1_OUTPUT",
+		"TX SWR_DMIC1", "DMIC2_OUTPUT",
+		"TX SWR_DMIC2", "DMIC3_OUTPUT",
+		"TX SWR_DMIC3", "DMIC4_OUTPUT",
+		"TX SWR_DMIC4", "DMIC5_OUTPUT",
+		"TX SWR_DMIC5", "DMIC6_OUTPUT",
+		"TX SWR_DMIC6", "DMIC7_OUTPUT",
+		"TX SWR_DMIC7", "DMIC8_OUTPUT";
+};
+
 &wcd938x {
 	pinctrl-names = "default";
 	pinctrl-0 = <&us_euro_hs_sel>;
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index e880837..640b1338 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -85,6 +85,99 @@
 		pinctrl-names = "default";
 		pinctrl-0 = <&nvme_pwren>;
 	};
+
+	sound: sound {
+		compatible = "google,sc7280-herobrine";
+		model = "sc7280-wcd938x-max98360a-1mic";
+
+		audio-routing =
+			"IN1_HPHL", "HPHL_OUT",
+			"IN2_HPHR", "HPHR_OUT",
+			"AMIC1", "MIC BIAS1",
+			"AMIC2", "MIC BIAS2",
+			"VA DMIC0", "MIC BIAS3",
+			"VA DMIC1", "MIC BIAS3",
+			"VA DMIC2", "MIC BIAS1",
+			"VA DMIC3", "MIC BIAS1",
+			"TX SWR_ADC0", "ADC1_OUTPUT",
+			"TX SWR_ADC1", "ADC2_OUTPUT",
+			"TX SWR_ADC2", "ADC3_OUTPUT",
+			"TX SWR_DMIC0", "DMIC1_OUTPUT",
+			"TX SWR_DMIC1", "DMIC2_OUTPUT",
+			"TX SWR_DMIC2", "DMIC3_OUTPUT",
+			"TX SWR_DMIC3", "DMIC4_OUTPUT",
+			"TX SWR_DMIC4", "DMIC5_OUTPUT",
+			"TX SWR_DMIC5", "DMIC6_OUTPUT",
+			"TX SWR_DMIC6", "DMIC7_OUTPUT",
+			"TX SWR_DMIC7", "DMIC8_OUTPUT";
+
+		qcom,msm-mbhc-hphl-swh = <1>;
+		qcom,msm-mbhc-gnd-swh = <1>;
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+		#sound-dai-cells = <0>;
+
+		dai-link@1 {
+			link-name = "MAX98360A";
+
+			cpu {
+				sound-dai = <&lpass_cpu MI2S_SECONDARY>;
+			};
+
+			codec {
+				sound-dai = <&max98360a>;
+			};
+		};
+
+		dai-link@5 {
+			link-name = "DisplayPort";
+
+			cpu {
+				sound-dai = <&lpass_cpu LPASS_DP_RX>;
+			};
+
+			codec {
+				sound-dai = <&mdss_dp>;
+			};
+		};
+
+		dai-link@6 {
+			link-name = "WCD9385 Playback";
+
+			cpu {
+				sound-dai = <&lpass_cpu LPASS_CDC_DMA_RX0>;
+			};
+
+			codec {
+				sound-dai = <&wcd938x 0>, <&swr0 0>, <&lpass_rx_macro 0>;
+			};
+		};
+
+		dai-link@19 {
+			link-name = "WCD9385 Capture";
+
+			cpu {
+				sound-dai = <&lpass_cpu LPASS_CDC_DMA_TX3>;
+			};
+
+			codec {
+				sound-dai = <&wcd938x 1>, <&swr1 0>, <&lpass_tx_macro 0>;
+			};
+		};
+
+		dai-link@25 {
+			link-name = "DMIC";
+
+			cpu {
+				sound-dai = <&lpass_cpu LPASS_CDC_DMA_VA_TX0>;
+			};
+
+			codec {
+				sound-dai = <&lpass_va_macro 0>;
+			};
+		};
+	};
 };
 
 &apps_rsc {
-- 
2.7.4

Re: [PATCH v8 1/4] arm64: dts: qcom: sc7280: Add nodes for soundwire and va tx rx digital macro codecs

[Matthias Kaehlcke]

On Mon, Apr 18, 2022 at 07:44:09PM +0530, Srinivasa Rao Mandadapu wrote:
> SC7280 has VA, TX and RX macros with SoundWire Controllers to attach with
> external codecs using soundwire masters. Add these nodes for sc7280 based
> platforms audio use case.
> 
> 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>

Re: [PATCH v8 2/4] arm64: dts: qcom: sc7280: Add nodes for wcd9385 and max98360a codec

[Matthias Kaehlcke]

On Mon, Apr 18, 2022 at 07:44:10PM +0530, Srinivasa Rao Mandadapu wrote:
> Add wcd938x and  max98360a codecs for audio use case on
> sc7280 based platforms.
> Add tlmm gpio property in wcd938x node for switching CTIA/OMTP Headset.
> Add amp_en node for  max98360a codec.

General note: I don't think it's a good practice to add stuff like this to
multiple boards in a single patch. Why?

First the subject of such a patch tends to be vague ("arm64: dts: qcom:
sc7280: Add nodes for wcd9385 and max98360a codec"), in this case it gives
no hint about the boards. If someone was interested in picking changes
for a given board they can't easily identify from the subject that the
change is relevant for them.

Changes touching multiple boards are more likely to cause conflicts when
being picked (or reverted), both upstream and in downstream trees (which
unfortunately have to exist for product development). Downstream trees
might only pick changes for the board(s) they target, patches that touch
mutiple boards often cause conflicts due to context deltas in the
'irrelevant' boards.

Lastly it's usually easier to get a patch reviewed (in the sense of
getting a 'Reviewed-by' tag) and landed that does a single thing.

> 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-crd-r3.dts     |  6 ++
>  arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi |  8 +++
>  arch/arm64/boot/dts/qcom/sc7280-idp.dtsi       | 97 ++++++++++++++++++++++++++
>  3 files changed, 111 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> index 344338a..aa0bf6e2 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> @@ -87,6 +87,12 @@ ap_ts_pen_1v8: &i2c13 {
>  	pins = "gpio51";
>  };
>  
> +&wcd938x {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&us_euro_hs_sel>;
> +	us-euro-gpios = <&tlmm 81 GPIO_ACTIVE_HIGH>;
> +};

Since this is added for the CRD rev3 it probably should also be added to
sc7280-herobrine-crd.dts

> +
>  &tlmm {
>  	tp_int_odl: tp-int-odl {
>  		pins = "gpio7";
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> index d58045d..f247403 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> @@ -20,6 +20,14 @@
>  #include "sc7280-chrome-common.dtsi"
>  
>  / {
> +	max98360a: audio-codec-0 {
> +		compatible = "maxim,max98360a";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&amp_en>;
> +		sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
> +		#sound-dai-cells = <0>;
> +	};
> +
>  	chosen {
>  		stdout-path = "serial0:115200n8";
>  	};
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> index 2f863c0..8dad599 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> @@ -20,6 +20,42 @@
>  		serial1 = &uart7;
>  	};
>  
> +	max98360a: audio-codec-0 {
> +		compatible = "maxim,max98360a";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&amp_en>;
> +		sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
> +		#sound-dai-cells = <0>;
> +	};
> +
> +	wcd938x: audio-codec-1 {

Why 'wcd938x' and not 'wcd9385'?

> +		compatible = "qcom,wcd9385-codec";
> +		pinctrl-names = "default", "sleep";
> +		pinctrl-0 = <&wcd_reset_n>;
> +		pinctrl-1 = <&wcd_reset_n_sleep>;
> +
> +		reset-gpios = <&tlmm 83 GPIO_ACTIVE_HIGH>;
> +
> +		qcom,rx-device = <&wcd_rx>;
> +		qcom,tx-device = <&wcd_tx>;
> +
> +		vdd-rxtx-supply = <&vreg_l18b_1p8>;
> +		vdd-io-supply = <&vreg_l18b_1p8>;
> +		vdd-buck-supply = <&vreg_l17b_1p8>;
> +		vdd-mic-bias-supply = <&vreg_bob>;
> +
> +		qcom,micbias1-microvolt = <1800000>;
> +		qcom,micbias2-microvolt = <1800000>;
> +		qcom,micbias3-microvolt = <1800000>;
> +		qcom,micbias4-microvolt = <1800000>;
> +
> +		qcom,mbhc-buttons-vthreshold-microvolt = <75000 150000 237000 500000 500000
> +							  500000 500000 500000>;
> +		qcom,mbhc-headset-vthreshold-microvolt = <1700000>;
> +		qcom,mbhc-headphone-vthreshold-microvolt = <50000>;
> +		#sound-dai-cells = <1>;
> +	};

Also add this node to sc7280-herobrine-crd.dts?

> +
>  	gpio-keys {
>  		compatible = "gpio-keys";
>  		label = "gpio-keys";
> @@ -238,6 +274,19 @@
>  	modem-init;
>  };
>  
> +&lpass_rx_macro {
> +	status = "okay";
> +};
> +
> +&lpass_tx_macro {
> +	status = "okay";
> +};
> +
> +&lpass_va_macro {
> +	status = "okay";
> +	vdd-micb-supply = <&vreg_bob>;
> +};

Enable these also in sc7280-herobrine.dtsi if other nodes are added to
sc7280-herobrine-crd.dts.

> +
>  &pcie1 {
>  	status = "okay";
>  	perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
> @@ -298,6 +347,26 @@
>  	cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;
>  };
>  
> +&swr0 {
> +	status = "okay";

nit: add an empty line between properties and nodes

> +	wcd_rx: codec@0,4 {
> +		compatible = "sdw20217010d00";
> +		reg = <0 4>;
> +		#sound-dai-cells = <1>;
> +		qcom,rx-port-mapping = <1 2 3 4 5>;
> +	};
> +};
> +
> +&swr1 {
> +	status = "okay";

ditto

> +	wcd_tx: codec@0,3 {
> +		compatible = "sdw20217010d00";
> +		reg = <0 3>;
> +		#sound-dai-cells = <1>;
> +		qcom,tx-port-mapping = <1 2 3 4>;
> +	};
> +};

Also add these to sc7280-herobrine-crd.dts?

> +
>  &uart5 {
>  	compatible = "qcom,geni-debug-uart";
>  	status = "okay";
> @@ -561,6 +630,12 @@
>  };
>  
>  &tlmm {
> +	amp_en: amp-en {
> +		pins = "gpio63";
> +		bias-pull-down;
> +		drive-strength = <2>;
> +	};
> +
>  	bt_en: bt-en {
>  		pins = "gpio85";
>  		function = "gpio";
> @@ -643,6 +718,28 @@
>  		function = "gpio";
>  		bias-pull-down;
>  	};
> +
> +	us_euro_hs_sel: us-euro-hs-sel {
> +		pins = "gpio81";
> +		function = "gpio";
> +		bias-pull-down;
> +		drive-strength = <2>;
> +	};
> +
> +	wcd_reset_n: wcd-reset-n {
> +		pins = "gpio83";
> +		function = "gpio";
> +		drive-strength = <8>;
> +		output-high;
> +	};
> +
> +	wcd_reset_n_sleep: wcd-reset-n-sleep {
> +		pins = "gpio83";
> +		function = "gpio";
> +		drive-strength = <8>;
> +		bias-disable;
> +		output-low;
> +	};

Also add to sc7280-herobrine-crd.dts if the other nodes are added.

Re: [PATCH v8 4/4] arm64: dts: qcom: sc7280: Add dt nodes for sound card

[Matthias Kaehlcke]

On Mon, Apr 18, 2022 at 07:44:12PM +0530, Srinivasa Rao Mandadapu wrote:
> Add dt nodes for sound card support, which is using WCD938x headset
> playback, capture, I2S speaker playback and DMICs via VA macro.
> 
> 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-crd-r3.dts | 23 ++++++++
>  arch/arm64/boot/dts/qcom/sc7280-idp.dtsi   | 93 ++++++++++++++++++++++++++++++
>  2 files changed, 116 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> index aa0bf6e2..bf15bbe 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> @@ -87,6 +87,29 @@ ap_ts_pen_1v8: &i2c13 {
>  	pins = "gpio51";
>  };
>  
> +&sound {
> +	audio-routing =
> +		"IN1_HPHL", "HPHL_OUT",
> +		"IN2_HPHR", "HPHR_OUT",
> +		"AMIC1", "MIC BIAS1",
> +		"AMIC2", "MIC BIAS2",
> +		"VA DMIC0", "MIC BIAS1",
> +		"VA DMIC1", "MIC BIAS1",
> +		"VA DMIC2", "MIC BIAS3",
> +		"VA DMIC3", "MIC BIAS3",
> +		"TX SWR_ADC0", "ADC1_OUTPUT",
> +		"TX SWR_ADC1", "ADC2_OUTPUT",
> +		"TX SWR_ADC2", "ADC3_OUTPUT",
> +		"TX SWR_DMIC0", "DMIC1_OUTPUT",
> +		"TX SWR_DMIC1", "DMIC2_OUTPUT",
> +		"TX SWR_DMIC2", "DMIC3_OUTPUT",
> +		"TX SWR_DMIC3", "DMIC4_OUTPUT",
> +		"TX SWR_DMIC4", "DMIC5_OUTPUT",
> +		"TX SWR_DMIC5", "DMIC6_OUTPUT",
> +		"TX SWR_DMIC6", "DMIC7_OUTPUT",
> +		"TX SWR_DMIC7", "DMIC8_OUTPUT";
> +};

Should this also be added to sc7280-herobrine-crd.dts?

> +
>  &wcd938x {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&us_euro_hs_sel>;
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> index e880837..640b1338 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> @@ -85,6 +85,99 @@
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&nvme_pwren>;
>  	};
> +
> +	sound: sound {
> +		compatible = "google,sc7280-herobrine";
> +		model = "sc7280-wcd938x-max98360a-1mic";
> +
> +		audio-routing =
> +			"IN1_HPHL", "HPHL_OUT",
> +			"IN2_HPHR", "HPHR_OUT",
> +			"AMIC1", "MIC BIAS1",
> +			"AMIC2", "MIC BIAS2",
> +			"VA DMIC0", "MIC BIAS3",
> +			"VA DMIC1", "MIC BIAS3",
> +			"VA DMIC2", "MIC BIAS1",
> +			"VA DMIC3", "MIC BIAS1",
> +			"TX SWR_ADC0", "ADC1_OUTPUT",
> +			"TX SWR_ADC1", "ADC2_OUTPUT",
> +			"TX SWR_ADC2", "ADC3_OUTPUT",
> +			"TX SWR_DMIC0", "DMIC1_OUTPUT",
> +			"TX SWR_DMIC1", "DMIC2_OUTPUT",
> +			"TX SWR_DMIC2", "DMIC3_OUTPUT",
> +			"TX SWR_DMIC3", "DMIC4_OUTPUT",
> +			"TX SWR_DMIC4", "DMIC5_OUTPUT",
> +			"TX SWR_DMIC5", "DMIC6_OUTPUT",
> +			"TX SWR_DMIC6", "DMIC7_OUTPUT",
> +			"TX SWR_DMIC7", "DMIC8_OUTPUT";
> +
> +		qcom,msm-mbhc-hphl-swh = <1>;
> +		qcom,msm-mbhc-gnd-swh = <1>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		#sound-dai-cells = <0>;
> +
> +		dai-link@1 {

The '@1' is the address of the lpass CPU DAI, which doesn't seem correct
here. As I brought up on v7 [1] I think this value isn't even necessarily
unique, a SoC could have multiple IP blocks with audio buses, each with
their own DAI address space. The binding (currently) requires an
'address'/id, rather than using the CPU DAI id I suggest to enumerate the
links linearly, starting with 0.

[1] https://patchwork.kernel.org/project/linux-arm-msm/patch/1649863277-31615-5-git-send-email-quic_srivasam@quicinc.com/

> +			link-name = "MAX98360A";

The binding requires a 'reg' property, even though it isn't used (also
discussed on v7). I think the 'reg' property should be removed from the
binding and the DTs that use it, but maybe that should be done in a
separate series. In the meantime the value should match that of the
node.

> +
> +			cpu {
> +				sound-dai = <&lpass_cpu MI2S_SECONDARY>;
> +			};
> +
> +			codec {
> +				sound-dai = <&max98360a>;
> +			};
> +		};
> +
> +		dai-link@5 {
> +			link-name = "DisplayPort";
> +
> +			cpu {
> +				sound-dai = <&lpass_cpu LPASS_DP_RX>;
> +			};
> +
> +			codec {
> +				sound-dai = <&mdss_dp>;
> +			};
> +		};
> +
> +		dai-link@6 {
> +			link-name = "WCD9385 Playback";
> +
> +			cpu {
> +				sound-dai = <&lpass_cpu LPASS_CDC_DMA_RX0>;
> +			};
> +
> +			codec {
> +				sound-dai = <&wcd938x 0>, <&swr0 0>, <&lpass_rx_macro 0>;
> +			};
> +		};
> +
> +		dai-link@19 {
> +			link-name = "WCD9385 Capture";
> +
> +			cpu {
> +				sound-dai = <&lpass_cpu LPASS_CDC_DMA_TX3>;
> +			};
> +
> +			codec {
> +				sound-dai = <&wcd938x 1>, <&swr1 0>, <&lpass_tx_macro 0>;
> +			};
> +		};
> +
> +		dai-link@25 {
> +			link-name = "DMIC";
> +
> +			cpu {
> +				sound-dai = <&lpass_cpu LPASS_CDC_DMA_VA_TX0>;
> +			};
> +
> +			codec {
> +				sound-dai = <&lpass_va_macro 0>;
> +			};
> +		};
> +	};
>  };
>  
>  &apps_rsc {
> -- 
> 2.7.4
> 

Re: [PATCH v8 2/4] arm64: dts: qcom: sc7280: Add nodes for wcd9385 and max98360a codec

[Srinivasa Rao Mandadapu]

On 4/18/2022 10:15 PM, Matthias Kaehlcke wrote:
Thanks for your time and valuable inputs Matthias!!!
> On Mon, Apr 18, 2022 at 07:44:10PM +0530, Srinivasa Rao Mandadapu wrote:
>> Add wcd938x and  max98360a codecs for audio use case on
>> sc7280 based platforms.
>> Add tlmm gpio property in wcd938x node for switching CTIA/OMTP Headset.
>> Add amp_en node for  max98360a codec.
> General note: I don't think it's a good practice to add stuff like this to
> multiple boards in a single patch. Why?
>
> First the subject of such a patch tends to be vague ("arm64: dts: qcom:
> sc7280: Add nodes for wcd9385 and max98360a codec"), in this case it gives
> no hint about the boards. If someone was interested in picking changes
> for a given board they can't easily identify from the subject that the
> change is relevant for them.
>
> Changes touching multiple boards are more likely to cause conflicts when
> being picked (or reverted), both upstream and in downstream trees (which
> unfortunately have to exist for product development). Downstream trees
> might only pick changes for the board(s) they target, patches that touch
> mutiple boards often cause conflicts due to context deltas in the
> 'irrelevant' boards.
>
> Lastly it's usually easier to get a patch reviewed (in the sense of
> getting a 'Reviewed-by' tag) and landed that does a single thing.

Yes, agree to your opinion. In a nutshell, we will include board 
name(ex: herobrine)

in commit message and split the patches per external codec.

Actually, in Initial herobrine boards, EVT and IDP, has both maxim 
speaker and WCD codec,

hence we included in same patch.

>> 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-crd-r3.dts     |  6 ++
>>   arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi |  8 +++
>>   arch/arm64/boot/dts/qcom/sc7280-idp.dtsi       | 97 ++++++++++++++++++++++++++
>>   3 files changed, 111 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>> index 344338a..aa0bf6e2 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>> @@ -87,6 +87,12 @@ ap_ts_pen_1v8: &i2c13 {
>>   	pins = "gpio51";
>>   };
>>   
>> +&wcd938x {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&us_euro_hs_sel>;
>> +	us-euro-gpios = <&tlmm 81 GPIO_ACTIVE_HIGH>;
>> +};
> Since this is added for the CRD rev3 it probably should also be added to
> sc7280-herobrine-crd.dts
Okay. Will add in corresponding latest herobrine CRD dts file also.
>
>> +
>>   &tlmm {
>>   	tp_int_odl: tp-int-odl {
>>   		pins = "gpio7";
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>> index d58045d..f247403 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>> @@ -20,6 +20,14 @@
>>   #include "sc7280-chrome-common.dtsi"
>>   
>>   / {
>> +	max98360a: audio-codec-0 {
>> +		compatible = "maxim,max98360a";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&amp_en>;
>> +		sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
>> +		#sound-dai-cells = <0>;
>> +	};
>> +
>>   	chosen {
>>   		stdout-path = "serial0:115200n8";
>>   	};
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> index 2f863c0..8dad599 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> @@ -20,6 +20,42 @@
>>   		serial1 = &uart7;
>>   	};
>>   
>> +	max98360a: audio-codec-0 {
>> +		compatible = "maxim,max98360a";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&amp_en>;
>> +		sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
>> +		#sound-dai-cells = <0>;
>> +	};
>> +
>> +	wcd938x: audio-codec-1 {
> Why 'wcd938x' and not 'wcd9385'?

Actually same driver is used for both wcd9380 and wcd9385. Here we can 
use specific name as per board.

Will change accordingly.

>
>> +		compatible = "qcom,wcd9385-codec";
>> +		pinctrl-names = "default", "sleep";
>> +		pinctrl-0 = <&wcd_reset_n>;
>> +		pinctrl-1 = <&wcd_reset_n_sleep>;
>> +
>> +		reset-gpios = <&tlmm 83 GPIO_ACTIVE_HIGH>;
>> +
>> +		qcom,rx-device = <&wcd_rx>;
>> +		qcom,tx-device = <&wcd_tx>;
>> +
>> +		vdd-rxtx-supply = <&vreg_l18b_1p8>;
>> +		vdd-io-supply = <&vreg_l18b_1p8>;
>> +		vdd-buck-supply = <&vreg_l17b_1p8>;
>> +		vdd-mic-bias-supply = <&vreg_bob>;
>> +
>> +		qcom,micbias1-microvolt = <1800000>;
>> +		qcom,micbias2-microvolt = <1800000>;
>> +		qcom,micbias3-microvolt = <1800000>;
>> +		qcom,micbias4-microvolt = <1800000>;
>> +
>> +		qcom,mbhc-buttons-vthreshold-microvolt = <75000 150000 237000 500000 500000
>> +							  500000 500000 500000>;
>> +		qcom,mbhc-headset-vthreshold-microvolt = <1700000>;
>> +		qcom,mbhc-headphone-vthreshold-microvolt = <50000>;
>> +		#sound-dai-cells = <1>;
>> +	};
> Also add this node to sc7280-herobrine-crd.dts?
Okay. will do accordingly.
>
>> +
>>   	gpio-keys {
>>   		compatible = "gpio-keys";
>>   		label = "gpio-keys";
>> @@ -238,6 +274,19 @@
>>   	modem-init;
>>   };
>>   
>> +&lpass_rx_macro {
>> +	status = "okay";
>> +};
>> +
>> +&lpass_tx_macro {
>> +	status = "okay";
>> +};
>> +
>> +&lpass_va_macro {
>> +	status = "okay";
>> +	vdd-micb-supply = <&vreg_bob>;
>> +};
> Enable these also in sc7280-herobrine.dtsi if other nodes are added to
> sc7280-herobrine-crd.dts.
Okay.
>
>> +
>>   &pcie1 {
>>   	status = "okay";
>>   	perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
>> @@ -298,6 +347,26 @@
>>   	cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;
>>   };
>>   
>> +&swr0 {
>> +	status = "okay";
> nit: add an empty line between properties and nodes
Okay.
>
>> +	wcd_rx: codec@0,4 {
>> +		compatible = "sdw20217010d00";
>> +		reg = <0 4>;
>> +		#sound-dai-cells = <1>;
>> +		qcom,rx-port-mapping = <1 2 3 4 5>;
>> +	};
>> +};
>> +
>> +&swr1 {
>> +	status = "okay";
> ditto
Okay.
>
>> +	wcd_tx: codec@0,3 {
>> +		compatible = "sdw20217010d00";
>> +		reg = <0 3>;
>> +		#sound-dai-cells = <1>;
>> +		qcom,tx-port-mapping = <1 2 3 4>;
>> +	};
>> +};
> Also add these to sc7280-herobrine-crd.dts?
Okay.
>
>> +
>>   &uart5 {
>>   	compatible = "qcom,geni-debug-uart";
>>   	status = "okay";
>> @@ -561,6 +630,12 @@
>>   };
>>   
>>   &tlmm {
>> +	amp_en: amp-en {
>> +		pins = "gpio63";
>> +		bias-pull-down;
>> +		drive-strength = <2>;
>> +	};
>> +
>>   	bt_en: bt-en {
>>   		pins = "gpio85";
>>   		function = "gpio";
>> @@ -643,6 +718,28 @@
>>   		function = "gpio";
>>   		bias-pull-down;
>>   	};
>> +
>> +	us_euro_hs_sel: us-euro-hs-sel {
>> +		pins = "gpio81";
>> +		function = "gpio";
>> +		bias-pull-down;
>> +		drive-strength = <2>;
>> +	};
>> +
>> +	wcd_reset_n: wcd-reset-n {
>> +		pins = "gpio83";
>> +		function = "gpio";
>> +		drive-strength = <8>;
>> +		output-high;
>> +	};
>> +
>> +	wcd_reset_n_sleep: wcd-reset-n-sleep {
>> +		pins = "gpio83";
>> +		function = "gpio";
>> +		drive-strength = <8>;
>> +		bias-disable;
>> +		output-low;
>> +	};
> Also add to sc7280-herobrine-crd.dts if the other nodes are added.
Okay.

Re: [PATCH v8 4/4] arm64: dts: qcom: sc7280: Add dt nodes for sound card

[Srinivasa Rao Mandadapu]

On 4/18/2022 11:32 PM, Matthias Kaehlcke wrote:
Thanks for your time Matthias!!!
> On Mon, Apr 18, 2022 at 07:44:12PM +0530, Srinivasa Rao Mandadapu wrote:
>> Add dt nodes for sound card support, which is using WCD938x headset
>> playback, capture, I2S speaker playback and DMICs via VA macro.
>>
>> 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-crd-r3.dts | 23 ++++++++
>>   arch/arm64/boot/dts/qcom/sc7280-idp.dtsi   | 93 ++++++++++++++++++++++++++++++
>>   2 files changed, 116 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>> index aa0bf6e2..bf15bbe 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>> @@ -87,6 +87,29 @@ ap_ts_pen_1v8: &i2c13 {
>>   	pins = "gpio51";
>>   };
>>   
>> +&sound {
>> +	audio-routing =
>> +		"IN1_HPHL", "HPHL_OUT",
>> +		"IN2_HPHR", "HPHR_OUT",
>> +		"AMIC1", "MIC BIAS1",
>> +		"AMIC2", "MIC BIAS2",
>> +		"VA DMIC0", "MIC BIAS1",
>> +		"VA DMIC1", "MIC BIAS1",
>> +		"VA DMIC2", "MIC BIAS3",
>> +		"VA DMIC3", "MIC BIAS3",
>> +		"TX SWR_ADC0", "ADC1_OUTPUT",
>> +		"TX SWR_ADC1", "ADC2_OUTPUT",
>> +		"TX SWR_ADC2", "ADC3_OUTPUT",
>> +		"TX SWR_DMIC0", "DMIC1_OUTPUT",
>> +		"TX SWR_DMIC1", "DMIC2_OUTPUT",
>> +		"TX SWR_DMIC2", "DMIC3_OUTPUT",
>> +		"TX SWR_DMIC3", "DMIC4_OUTPUT",
>> +		"TX SWR_DMIC4", "DMIC5_OUTPUT",
>> +		"TX SWR_DMIC5", "DMIC6_OUTPUT",
>> +		"TX SWR_DMIC6", "DMIC7_OUTPUT",
>> +		"TX SWR_DMIC7", "DMIC8_OUTPUT";
>> +};
> Should this also be added to sc7280-herobrine-crd.dts?
Okay. Will add it corresponding dts file.
>
>> +
>>   &wcd938x {
>>   	pinctrl-names = "default";
>>   	pinctrl-0 = <&us_euro_hs_sel>;
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> index e880837..640b1338 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> @@ -85,6 +85,99 @@
>>   		pinctrl-names = "default";
>>   		pinctrl-0 = <&nvme_pwren>;
>>   	};
>> +
>> +	sound: sound {
>> +		compatible = "google,sc7280-herobrine";
>> +		model = "sc7280-wcd938x-max98360a-1mic";
>> +
>> +		audio-routing =
>> +			"IN1_HPHL", "HPHL_OUT",
>> +			"IN2_HPHR", "HPHR_OUT",
>> +			"AMIC1", "MIC BIAS1",
>> +			"AMIC2", "MIC BIAS2",
>> +			"VA DMIC0", "MIC BIAS3",
>> +			"VA DMIC1", "MIC BIAS3",
>> +			"VA DMIC2", "MIC BIAS1",
>> +			"VA DMIC3", "MIC BIAS1",
>> +			"TX SWR_ADC0", "ADC1_OUTPUT",
>> +			"TX SWR_ADC1", "ADC2_OUTPUT",
>> +			"TX SWR_ADC2", "ADC3_OUTPUT",
>> +			"TX SWR_DMIC0", "DMIC1_OUTPUT",
>> +			"TX SWR_DMIC1", "DMIC2_OUTPUT",
>> +			"TX SWR_DMIC2", "DMIC3_OUTPUT",
>> +			"TX SWR_DMIC3", "DMIC4_OUTPUT",
>> +			"TX SWR_DMIC4", "DMIC5_OUTPUT",
>> +			"TX SWR_DMIC5", "DMIC6_OUTPUT",
>> +			"TX SWR_DMIC6", "DMIC7_OUTPUT",
>> +			"TX SWR_DMIC7", "DMIC8_OUTPUT";
>> +
>> +		qcom,msm-mbhc-hphl-swh = <1>;
>> +		qcom,msm-mbhc-gnd-swh = <1>;
>> +
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		#sound-dai-cells = <0>;
>> +
>> +		dai-link@1 {
> The '@1' is the address of the lpass CPU DAI, which doesn't seem correct
> here. As I brought up on v7 [1] I think this value isn't even necessarily
> unique, a SoC could have multiple IP blocks with audio buses, each with
> their own DAI address space. The binding (currently) requires an
> 'address'/id, rather than using the CPU DAI id I suggest to enumerate the
> links linearly, starting with 0.
Okay. Will update accordingly.
>
> [1] https://patchwork.kernel.org/project/linux-arm-msm/patch/1649863277-31615-5-git-send-email-quic_srivasam@quicinc.com/
>
>> +			link-name = "MAX98360A";
> The binding requires a 'reg' property, even though it isn't used (also
> discussed on v7). I think the 'reg' property should be removed from the
> binding and the DTs that use it, but maybe that should be done in a
> separate series. In the meantime the value should match that of the
> node.
Okay. for now will revert this change. and will take care your 
suggestion in a separate patch set.
>
>> +
>> +			cpu {
>> +				sound-dai = <&lpass_cpu MI2S_SECONDARY>;
>> +			};
>> +
>> +			codec {
>> +				sound-dai = <&max98360a>;
>> +			};
>> +		};
>> +
>> +		dai-link@5 {
>> +			link-name = "DisplayPort";
>> +
>> +			cpu {
>> +				sound-dai = <&lpass_cpu LPASS_DP_RX>;
>> +			};
>> +
>> +			codec {
>> +				sound-dai = <&mdss_dp>;
>> +			};
>> +		};
>> +
>> +		dai-link@6 {
>> +			link-name = "WCD9385 Playback";
>> +
>> +			cpu {
>> +				sound-dai = <&lpass_cpu LPASS_CDC_DMA_RX0>;
>> +			};
>> +
>> +			codec {
>> +				sound-dai = <&wcd938x 0>, <&swr0 0>, <&lpass_rx_macro 0>;
>> +			};
>> +		};
>> +
>> +		dai-link@19 {
>> +			link-name = "WCD9385 Capture";
>> +
>> +			cpu {
>> +				sound-dai = <&lpass_cpu LPASS_CDC_DMA_TX3>;
>> +			};
>> +
>> +			codec {
>> +				sound-dai = <&wcd938x 1>, <&swr1 0>, <&lpass_tx_macro 0>;
>> +			};
>> +		};
>> +
>> +		dai-link@25 {
>> +			link-name = "DMIC";
>> +
>> +			cpu {
>> +				sound-dai = <&lpass_cpu LPASS_CDC_DMA_VA_TX0>;
>> +			};
>> +
>> +			codec {
>> +				sound-dai = <&lpass_va_macro 0>;
>> +			};
>> +		};
>> +	};
>>   };
>>   
>>   &apps_rsc {
>> -- 
>> 2.7.4
>>

Re: [PATCH v8 2/4] arm64: dts: qcom: sc7280: Add nodes for wcd9385 and max98360a codec

[Srinivasa Rao Mandadapu]

On 4/19/2022 4:54 PM, Srinivasa Rao Mandadapu wrote:
>
> On 4/18/2022 10:15 PM, Matthias Kaehlcke wrote:
> Thanks for your time and valuable inputs Matthias!!!
>> On Mon, Apr 18, 2022 at 07:44:10PM +0530, Srinivasa Rao Mandadapu wrote:
>>> Add wcd938x and  max98360a codecs for audio use case on
>>> sc7280 based platforms.
>>> Add tlmm gpio property in wcd938x node for switching CTIA/OMTP Headset.
>>> Add amp_en node for  max98360a codec.
>> General note: I don't think it's a good practice to add stuff like 
>> this to
>> multiple boards in a single patch. Why?
>>
>> First the subject of such a patch tends to be vague ("arm64: dts: qcom:
>> sc7280: Add nodes for wcd9385 and max98360a codec"), in this case it 
>> gives
>> no hint about the boards. If someone was interested in picking changes
>> for a given board they can't easily identify from the subject that the
>> change is relevant for them.
>>
>> Changes touching multiple boards are more likely to cause conflicts when
>> being picked (or reverted), both upstream and in downstream trees (which
>> unfortunately have to exist for product development). Downstream trees
>> might only pick changes for the board(s) they target, patches that touch
>> mutiple boards often cause conflicts due to context deltas in the
>> 'irrelevant' boards.
>>
>> Lastly it's usually easier to get a patch reviewed (in the sense of
>> getting a 'Reviewed-by' tag) and landed that does a single thing.
>
> Yes, agree to your opinion. In a nutshell, we will include board 
> name(ex: herobrine)
>
> in commit message and split the patches per external codec.
>
> Actually, in Initial herobrine boards, EVT and IDP, has both maxim 
> speaker and WCD codec,
>
> hence we included in same patch.
>
>>> 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-crd-r3.dts     |  6 ++
>>>   arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi |  8 +++
>>>   arch/arm64/boot/dts/qcom/sc7280-idp.dtsi       | 97 
>>> ++++++++++++++++++++++++++
>>>   3 files changed, 111 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts 
>>> b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>>> index 344338a..aa0bf6e2 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>>> @@ -87,6 +87,12 @@ ap_ts_pen_1v8: &i2c13 {
>>>       pins = "gpio51";
>>>   };
>>>   +&wcd938x {
>>> +    pinctrl-names = "default";
>>> +    pinctrl-0 = <&us_euro_hs_sel>;
>>> +    us-euro-gpios = <&tlmm 81 GPIO_ACTIVE_HIGH>;
>>> +};
>> Since this is added for the CRD rev3 it probably should also be added to
>> sc7280-herobrine-crd.dts
> Okay. Will add in corresponding latest herobrine CRD dts file also.
>>
>>> +
>>>   &tlmm {
>>>       tp_int_odl: tp-int-odl {
>>>           pins = "gpio7";
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi 
>>> b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>> index d58045d..f247403 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>> @@ -20,6 +20,14 @@
>>>   #include "sc7280-chrome-common.dtsi"
>>>     / {
>>> +    max98360a: audio-codec-0 {
>>> +        compatible = "maxim,max98360a";
>>> +        pinctrl-names = "default";
>>> +        pinctrl-0 = <&amp_en>;
>>> +        sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
>>> +        #sound-dai-cells = <0>;
>>> +    };
>>> +
>>>       chosen {
>>>           stdout-path = "serial0:115200n8";
>>>       };
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi 
>>> b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>> index 2f863c0..8dad599 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>> @@ -20,6 +20,42 @@
>>>           serial1 = &uart7;
>>>       };
>>>   +    max98360a: audio-codec-0 {
>>> +        compatible = "maxim,max98360a";
>>> +        pinctrl-names = "default";
>>> +        pinctrl-0 = <&amp_en>;
>>> +        sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
>>> +        #sound-dai-cells = <0>;
>>> +    };
>>> +
>>> +    wcd938x: audio-codec-1 {
>> Why 'wcd938x' and not 'wcd9385'?
>
> Actually same driver is used for both wcd9380 and wcd9385. Here we can 
> use specific name as per board.
>
> Will change accordingly.

At present, dt-bindgs also has wcd938x. So will update the name in 
bindings and here post this series.

is it okay?

>
>>
>>> +        compatible = "qcom,wcd9385-codec";
>>> +        pinctrl-names = "default", "sleep";
>>> +        pinctrl-0 = <&wcd_reset_n>;
>>> +        pinctrl-1 = <&wcd_reset_n_sleep>;
>>> +
>>> +        reset-gpios = <&tlmm 83 GPIO_ACTIVE_HIGH>;
>>> +
>>> +        qcom,rx-device = <&wcd_rx>;
>>> +        qcom,tx-device = <&wcd_tx>;
>>> +
>>> +        vdd-rxtx-supply = <&vreg_l18b_1p8>;
>>> +        vdd-io-supply = <&vreg_l18b_1p8>;
>>> +        vdd-buck-supply = <&vreg_l17b_1p8>;
>>> +        vdd-mic-bias-supply = <&vreg_bob>;
>>> +
>>> +        qcom,micbias1-microvolt = <1800000>;
>>> +        qcom,micbias2-microvolt = <1800000>;
>>> +        qcom,micbias3-microvolt = <1800000>;
>>> +        qcom,micbias4-microvolt = <1800000>;
>>> +
>>> +        qcom,mbhc-buttons-vthreshold-microvolt = <75000 150000 
>>> 237000 500000 500000
>>> +                              500000 500000 500000>;
>>> +        qcom,mbhc-headset-vthreshold-microvolt = <1700000>;
>>> +        qcom,mbhc-headphone-vthreshold-microvolt = <50000>;
>>> +        #sound-dai-cells = <1>;
>>> +    };
>> Also add this node to sc7280-herobrine-crd.dts?
> Okay. will do accordingly.
>>
>>> +
>>>       gpio-keys {
>>>           compatible = "gpio-keys";
>>>           label = "gpio-keys";
>>> @@ -238,6 +274,19 @@
>>>       modem-init;
>>>   };
>>>   +&lpass_rx_macro {
>>> +    status = "okay";
>>> +};
>>> +
>>> +&lpass_tx_macro {
>>> +    status = "okay";
>>> +};
>>> +
>>> +&lpass_va_macro {
>>> +    status = "okay";
>>> +    vdd-micb-supply = <&vreg_bob>;
>>> +};
>> Enable these also in sc7280-herobrine.dtsi if other nodes are added to
>> sc7280-herobrine-crd.dts.
> Okay.
>>
>>> +
>>>   &pcie1 {
>>>       status = "okay";
>>>       perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
>>> @@ -298,6 +347,26 @@
>>>       cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;
>>>   };
>>>   +&swr0 {
>>> +    status = "okay";
>> nit: add an empty line between properties and nodes
> Okay.
>>
>>> +    wcd_rx: codec@0,4 {
>>> +        compatible = "sdw20217010d00";
>>> +        reg = <0 4>;
>>> +        #sound-dai-cells = <1>;
>>> +        qcom,rx-port-mapping = <1 2 3 4 5>;
>>> +    };
>>> +};
>>> +
>>> +&swr1 {
>>> +    status = "okay";
>> ditto
> Okay.
>>
>>> +    wcd_tx: codec@0,3 {
>>> +        compatible = "sdw20217010d00";
>>> +        reg = <0 3>;
>>> +        #sound-dai-cells = <1>;
>>> +        qcom,tx-port-mapping = <1 2 3 4>;
>>> +    };
>>> +};
>> Also add these to sc7280-herobrine-crd.dts?
> Okay.
>>
>>> +
>>>   &uart5 {
>>>       compatible = "qcom,geni-debug-uart";
>>>       status = "okay";
>>> @@ -561,6 +630,12 @@
>>>   };
>>>     &tlmm {
>>> +    amp_en: amp-en {
>>> +        pins = "gpio63";
>>> +        bias-pull-down;
>>> +        drive-strength = <2>;
>>> +    };
>>> +
>>>       bt_en: bt-en {
>>>           pins = "gpio85";
>>>           function = "gpio";
>>> @@ -643,6 +718,28 @@
>>>           function = "gpio";
>>>           bias-pull-down;
>>>       };
>>> +
>>> +    us_euro_hs_sel: us-euro-hs-sel {
>>> +        pins = "gpio81";
>>> +        function = "gpio";
>>> +        bias-pull-down;
>>> +        drive-strength = <2>;
>>> +    };
>>> +
>>> +    wcd_reset_n: wcd-reset-n {
>>> +        pins = "gpio83";
>>> +        function = "gpio";
>>> +        drive-strength = <8>;
>>> +        output-high;
>>> +    };
>>> +
>>> +    wcd_reset_n_sleep: wcd-reset-n-sleep {
>>> +        pins = "gpio83";
>>> +        function = "gpio";
>>> +        drive-strength = <8>;
>>> +        bias-disable;
>>> +        output-low;
>>> +    };
>> Also add to sc7280-herobrine-crd.dts if the other nodes are added.
> Okay.

Re: [PATCH v8 2/4] arm64: dts: qcom: sc7280: Add nodes for wcd9385 and max98360a codec

[Matthias Kaehlcke]

On Thu, Apr 21, 2022 at 12:31:52PM +0530, Srinivasa Rao Mandadapu wrote:
> 
> On 4/19/2022 4:54 PM, Srinivasa Rao Mandadapu wrote:
> > 
> > On 4/18/2022 10:15 PM, Matthias Kaehlcke wrote:
> > Thanks for your time and valuable inputs Matthias!!!
> > > On Mon, Apr 18, 2022 at 07:44:10PM +0530, Srinivasa Rao Mandadapu wrote:
> > > > Add wcd938x and  max98360a codecs for audio use case on
> > > > sc7280 based platforms.
> > > > Add tlmm gpio property in wcd938x node for switching CTIA/OMTP Headset.
> > > > Add amp_en node for  max98360a codec.
> > > General note: I don't think it's a good practice to add stuff like
> > > this to
> > > multiple boards in a single patch. Why?
> > > 
> > > First the subject of such a patch tends to be vague ("arm64: dts: qcom:
> > > sc7280: Add nodes for wcd9385 and max98360a codec"), in this case it
> > > gives
> > > no hint about the boards. If someone was interested in picking changes
> > > for a given board they can't easily identify from the subject that the
> > > change is relevant for them.
> > > 
> > > Changes touching multiple boards are more likely to cause conflicts when
> > > being picked (or reverted), both upstream and in downstream trees (which
> > > unfortunately have to exist for product development). Downstream trees
> > > might only pick changes for the board(s) they target, patches that touch
> > > mutiple boards often cause conflicts due to context deltas in the
> > > 'irrelevant' boards.
> > > 
> > > Lastly it's usually easier to get a patch reviewed (in the sense of
> > > getting a 'Reviewed-by' tag) and landed that does a single thing.
> > 
> > Yes, agree to your opinion. In a nutshell, we will include board
> > name(ex: herobrine)
> > 
> > in commit message and split the patches per external codec.
> > 
> > Actually, in Initial herobrine boards, EVT and IDP, has both maxim
> > speaker and WCD codec,
> > 
> > hence we included in same patch.
> > 
> > > > 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-crd-r3.dts     |  6 ++
> > > >   arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi |  8 +++
> > > >   arch/arm64/boot/dts/qcom/sc7280-idp.dtsi       | 97
> > > > ++++++++++++++++++++++++++
> > > >   3 files changed, 111 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> > > > b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> > > > index 344338a..aa0bf6e2 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> > > > @@ -87,6 +87,12 @@ ap_ts_pen_1v8: &i2c13 {
> > > >       pins = "gpio51";
> > > >   };
> > > >   +&wcd938x {
> > > > +    pinctrl-names = "default";
> > > > +    pinctrl-0 = <&us_euro_hs_sel>;
> > > > +    us-euro-gpios = <&tlmm 81 GPIO_ACTIVE_HIGH>;
> > > > +};
> > > Since this is added for the CRD rev3 it probably should also be added to
> > > sc7280-herobrine-crd.dts
> > Okay. Will add in corresponding latest herobrine CRD dts file also.
> > > 
> > > > +
> > > >   &tlmm {
> > > >       tp_int_odl: tp-int-odl {
> > > >           pins = "gpio7";
> > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > > > b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > > > index d58045d..f247403 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > > > @@ -20,6 +20,14 @@
> > > >   #include "sc7280-chrome-common.dtsi"
> > > >     / {
> > > > +    max98360a: audio-codec-0 {
> > > > +        compatible = "maxim,max98360a";
> > > > +        pinctrl-names = "default";
> > > > +        pinctrl-0 = <&amp_en>;
> > > > +        sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
> > > > +        #sound-dai-cells = <0>;
> > > > +    };
> > > > +
> > > >       chosen {
> > > >           stdout-path = "serial0:115200n8";
> > > >       };
> > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> > > > b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> > > > index 2f863c0..8dad599 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> > > > @@ -20,6 +20,42 @@
> > > >           serial1 = &uart7;
> > > >       };
> > > >   +    max98360a: audio-codec-0 {
> > > > +        compatible = "maxim,max98360a";
> > > > +        pinctrl-names = "default";
> > > > +        pinctrl-0 = <&amp_en>;
> > > > +        sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
> > > > +        #sound-dai-cells = <0>;
> > > > +    };
> > > > +
> > > > +    wcd938x: audio-codec-1 {
> > > Why 'wcd938x' and not 'wcd9385'?
> > 
> > Actually same driver is used for both wcd9380 and wcd9385. Here we can
> > use specific name as per board.
> > 
> > Will change accordingly.
> 
> At present, dt-bindgs also has wcd938x. So will update the name in bindings
> and here post this series.
> 
> is it okay?

I don't think it's strictly necessary to update the binding, as
'wcd938x' is only used in the example, and also it's not really
wrong. Then again, if the example in the binding uses a
specific wcd version it might make it less likely that the
wildcard name is used in future board DTs. Up to you :)

Re: [PATCH v8 2/4] arm64: dts: qcom: sc7280: Add nodes for wcd9385 and max98360a codec

[Srinivasa Rao Mandadapu]

On 4/21/2022 9:12 PM, Matthias Kaehlcke wrote:
Thanks for your time Matthias!!
> On Thu, Apr 21, 2022 at 12:31:52PM +0530, Srinivasa Rao Mandadapu wrote:
>> On 4/19/2022 4:54 PM, Srinivasa Rao Mandadapu wrote:
>>> On 4/18/2022 10:15 PM, Matthias Kaehlcke wrote:
>>> Thanks for your time and valuable inputs Matthias!!!
>>>> On Mon, Apr 18, 2022 at 07:44:10PM +0530, Srinivasa Rao Mandadapu wrote:
>>>>> Add wcd938x and  max98360a codecs for audio use case on
>>>>> sc7280 based platforms.
>>>>> Add tlmm gpio property in wcd938x node for switching CTIA/OMTP Headset.
>>>>> Add amp_en node for  max98360a codec.
>>>> General note: I don't think it's a good practice to add stuff like
>>>> this to
>>>> multiple boards in a single patch. Why?
>>>>
>>>> First the subject of such a patch tends to be vague ("arm64: dts: qcom:
>>>> sc7280: Add nodes for wcd9385 and max98360a codec"), in this case it
>>>> gives
>>>> no hint about the boards. If someone was interested in picking changes
>>>> for a given board they can't easily identify from the subject that the
>>>> change is relevant for them.
>>>>
>>>> Changes touching multiple boards are more likely to cause conflicts when
>>>> being picked (or reverted), both upstream and in downstream trees (which
>>>> unfortunately have to exist for product development). Downstream trees
>>>> might only pick changes for the board(s) they target, patches that touch
>>>> mutiple boards often cause conflicts due to context deltas in the
>>>> 'irrelevant' boards.
>>>>
>>>> Lastly it's usually easier to get a patch reviewed (in the sense of
>>>> getting a 'Reviewed-by' tag) and landed that does a single thing.
>>> Yes, agree to your opinion. In a nutshell, we will include board
>>> name(ex: herobrine)
>>>
>>> in commit message and split the patches per external codec.
>>>
>>> Actually, in Initial herobrine boards, EVT and IDP, has both maxim
>>> speaker and WCD codec,
>>>
>>> hence we included in same patch.
>>>
>>>>> 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-crd-r3.dts     |  6 ++
>>>>>    arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi |  8 +++
>>>>>    arch/arm64/boot/dts/qcom/sc7280-idp.dtsi       | 97
>>>>> ++++++++++++++++++++++++++
>>>>>    3 files changed, 111 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>>>>> b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>>>>> index 344338a..aa0bf6e2 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>>>>> @@ -87,6 +87,12 @@ ap_ts_pen_1v8: &i2c13 {
>>>>>        pins = "gpio51";
>>>>>    };
>>>>>    +&wcd938x {
>>>>> +    pinctrl-names = "default";
>>>>> +    pinctrl-0 = <&us_euro_hs_sel>;
>>>>> +    us-euro-gpios = <&tlmm 81 GPIO_ACTIVE_HIGH>;
>>>>> +};
>>>> Since this is added for the CRD rev3 it probably should also be added to
>>>> sc7280-herobrine-crd.dts
>>> Okay. Will add in corresponding latest herobrine CRD dts file also.
>>>>> +
>>>>>    &tlmm {
>>>>>        tp_int_odl: tp-int-odl {
>>>>>            pins = "gpio7";
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>>> b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>>> index d58045d..f247403 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>>> @@ -20,6 +20,14 @@
>>>>>    #include "sc7280-chrome-common.dtsi"
>>>>>      / {
>>>>> +    max98360a: audio-codec-0 {
>>>>> +        compatible = "maxim,max98360a";
>>>>> +        pinctrl-names = "default";
>>>>> +        pinctrl-0 = <&amp_en>;
>>>>> +        sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
>>>>> +        #sound-dai-cells = <0>;
>>>>> +    };
>>>>> +
>>>>>        chosen {
>>>>>            stdout-path = "serial0:115200n8";
>>>>>        };
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>>>> b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>>>> index 2f863c0..8dad599 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>>>> @@ -20,6 +20,42 @@
>>>>>            serial1 = &uart7;
>>>>>        };
>>>>>    +    max98360a: audio-codec-0 {
>>>>> +        compatible = "maxim,max98360a";
>>>>> +        pinctrl-names = "default";
>>>>> +        pinctrl-0 = <&amp_en>;
>>>>> +        sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
>>>>> +        #sound-dai-cells = <0>;
>>>>> +    };
>>>>> +
>>>>> +    wcd938x: audio-codec-1 {
>>>> Why 'wcd938x' and not 'wcd9385'?
>>> Actually same driver is used for both wcd9380 and wcd9385. Here we can
>>> use specific name as per board.
>>>
>>> Will change accordingly.
>> At present, dt-bindgs also has wcd938x. So will update the name in bindings
>> and here post this series.
>>
>> is it okay?
> I don't think it's strictly necessary to update the binding, as
> 'wcd938x' is only used in the example, and also it's not really
> wrong. Then again, if the example in the binding uses a
> specific wcd version it might make it less likely that the
> wildcard name is used in future board DTs. Up to you :)
Okay. Will update accordingly.