[PATCH 0/2] arm64: dts: qcom: sm8750: Add sound

[PATCH 0/2] arm64: dts: qcom: sm8750: Add sound

[Krzysztof Kozlowski]

Dependencies were merged, so this brings the necessary sound on SM8750
MTP.  I have patches work-in-progress for QRD and also USB headset, but
this have dependencies and need some cleanup. I will be posting these
separately.

Best regards,
Krzysztof

---
Krzysztof Kozlowski (2):
      arm64: dts: qcom: sm8750: Add Soundwire nodes
      arm64: dts: qcom: sm8750-mtp: Add sound (speakers, headset codec, dmics)

 arch/arm64/boot/dts/qcom/sm8750-mtp.dts | 214 ++++++++++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sm8750.dtsi    | 122 ++++++++++++++++++
 2 files changed, 336 insertions(+)
---
base-commit: 393d0c54cae31317deaa9043320c5fd9454deabc
change-id: 20241122-sm8750-audio-part-2-943277d85302

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

[PATCH 1/2] arm64: dts: qcom: sm8750: Add Soundwire nodes

[Krzysztof Kozlowski]

Add Soundwire controllers on SM8750, fully compatible with earlier
SM8650 generation.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8750.dtsi | 122 +++++++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
index 149d2ed17641a085d510f3a8eab5a96304787f0c..1e7aa25c675e76ce6aa571e04d7117b8c2ab25f8 100644
--- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
@@ -2257,6 +2257,36 @@ lpass_wsa2macro: codec@6aa0000 {
 			#sound-dai-cells = <1>;
 		};
 
+		swr3: soundwire@6ab0000 {
+			compatible = "qcom,soundwire-v2.0.0";
+			reg = <0 0x06ab0000 0 0x10000>;
+			interrupts = <GIC_SPI 171 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&lpass_wsa2macro>;
+			clock-names = "iface";
+			label = "WSA2";
+
+			pinctrl-0 = <&wsa2_swr_active>;
+			pinctrl-names = "default";
+
+			qcom,din-ports = <4>;
+			qcom,dout-ports = <9>;
+
+			qcom,ports-sinterval =		/bits/ 16 <0x07 0x1f 0x3f 0x07 0x1f 0x3f 0x18f 0x18f 0x18f 0x0f 0x0f 0xff 0x31f>;
+			qcom,ports-offset1 =		/bits/ 8 <0x01 0x03 0x05 0x02 0x04 0x15 0x00 0x00 0x00 0x06 0x0d 0xff 0x00>;
+			qcom,ports-offset2 =		/bits/ 8 <0xff 0x07 0x1f 0xff 0x07 0x1f 0xff 0xff 0xff 0xff 0xff 0xff 0xff>;
+			qcom,ports-hstart =		/bits/ 8 <0xff 0xff 0xff 0xff 0xff 0xff 0x08 0x0e 0x0e 0xff 0xff 0xff 0x0f>;
+			qcom,ports-hstop =		/bits/ 8 <0xff 0xff 0xff 0xff 0xff 0xff 0x08 0x0e 0x0e 0xff 0xff 0xff 0x0f>;
+			qcom,ports-word-length =	/bits/ 8 <0xff 0xff 0xff 0xff 0xff 0xff 0x08 0x0f 0x0f 0x00 0xff 0xff 0x18>;
+			qcom,ports-block-pack-mode =	/bits/ 8 <0x00 0x01 0x01 0x00 0x01 0x01 0x00 0x01 0x01 0x01 0x01 0x00 0x00>;
+			qcom,ports-block-group-count =	/bits/ 8 <0xff 0xff 0xff 0xff 0xff 0xff 0xff 0x00 0x00 0xff 0xff 0xff 0xff>;
+			qcom,ports-lane-control =	/bits/ 8 <0xff 0xff 0xff 0xff 0xff 0xff 0xff 0x00 0x00 0xff 0xff 0xff 0xff>;
+
+			#address-cells = <2>;
+			#size-cells = <0>;
+			#sound-dai-cells = <1>;
+			status = "disabled";
+		};
+
 		lpass_rxmacro: codec@6ac0000 {
 			compatible = "qcom,sm8750-lpass-rx-macro", "qcom,sm8550-lpass-rx-macro";
 			reg = <0x0 0x06ac0000 0x0 0x1000>;
@@ -2274,6 +2304,36 @@ lpass_rxmacro: codec@6ac0000 {
 			#sound-dai-cells = <1>;
 		};
 
+		swr1: soundwire@6ad0000 {
+			compatible = "qcom,soundwire-v2.0.0";
+			reg = <0 0x06ad0000 0 0x10000>;
+			interrupts = <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&lpass_rxmacro>;
+			clock-names = "iface";
+			label = "RX";
+
+			pinctrl-0 = <&rx_swr_active>;
+			pinctrl-names = "default";
+
+			qcom,din-ports = <1>;
+			qcom,dout-ports = <11>;
+
+			qcom,ports-sinterval =		/bits/ 16 <0x03 0x3f 0x1f 0x07 0x00 0x18f 0xff 0xff 0x31 0xff 0xff 0xff>;
+			qcom,ports-offset1 =		/bits/ 8 <0x00 0x00 0x0b 0x01 0x00 0x00 0xff 0xff 0x00 0xff 0xff 0xff>;
+			qcom,ports-offset2 =		/bits/ 8 <0x00 0x00 0x0b 0x00 0x00 0x00 0xff 0xff 0x00 0xff 0xff 0xff>;
+			qcom,ports-hstart =		/bits/ 8 <0xff 0x03 0xff 0xff 0xff 0x08 0xff 0xff 0x00 0xff 0xff 0xff>;
+			qcom,ports-hstop =		/bits/ 8 <0xff 0x06 0xff 0xff 0xff 0x08 0xff 0xff 0x0f 0xff 0xff 0xff>;
+			qcom,ports-word-length =	/bits/ 8 <0x01 0x07 0x04 0xff 0xff 0x0f 0xff 0xff 0x18 0xff 0xff 0xff>;
+			qcom,ports-block-pack-mode =	/bits/ 8 <0xff 0x00 0x01 0xff 0xff 0x00 0xff 0xff 0x01 0xff 0xff 0xff>;
+			qcom,ports-block-group-count =	/bits/ 8 <0xff 0xff 0xff 0xff 0x00 0x00 0xff 0xff 0x00 0xff 0xff 0xff>;
+			qcom,ports-lane-control =	/bits/ 8 <0x01 0x00 0x00 0x00 0x00 0x00 0xff 0xff 0x01 0xff 0xff 0xff>;
+
+			#address-cells = <2>;
+			#size-cells = <0>;
+			#sound-dai-cells = <1>;
+			status = "disabled";
+		};
+
 		lpass_txmacro: codec@6ae0000 {
 			compatible = "qcom,sm8750-lpass-tx-macro", "qcom,sm8550-lpass-tx-macro";
 			reg = <0x0 0x06ae0000 0x0 0x1000>;
@@ -2308,6 +2368,36 @@ lpass_wsamacro: codec@6b00000 {
 			#sound-dai-cells = <1>;
 		};
 
+		swr0: soundwire@6b10000 {
+			compatible = "qcom,soundwire-v2.0.0";
+			reg = <0 0x06b10000 0 0x10000>;
+			interrupts = <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&lpass_wsamacro>;
+			clock-names = "iface";
+			label = "WSA";
+
+			pinctrl-0 = <&wsa_swr_active>;
+			pinctrl-names = "default";
+
+			qcom,din-ports = <4>;
+			qcom,dout-ports = <9>;
+
+			qcom,ports-sinterval =		/bits/ 16 <0x07 0x1f 0x3f 0x07 0x1f 0x3f 0x18f 0x18f 0x18f 0x0f 0x0f 0xff 0x31f>;
+			qcom,ports-offset1 =		/bits/ 8 <0x01 0x03 0x05 0x02 0x04 0x15 0x00 0x00 0x00 0x06 0x0d 0xff 0x00>;
+			qcom,ports-offset2 =		/bits/ 8 <0xff 0x07 0x1f 0xff 0x07 0x1f 0xff 0xff 0xff 0xff 0xff 0xff 0xff>;
+			qcom,ports-hstart =		/bits/ 8 <0xff 0xff 0xff 0xff 0xff 0xff 0x08 0x0e 0x0e 0xff 0xff 0xff 0x0f>;
+			qcom,ports-hstop =		/bits/ 8 <0xff 0xff 0xff 0xff 0xff 0xff 0x08 0x0e 0x0e 0xff 0xff 0xff 0x0f>;
+			qcom,ports-word-length =	/bits/ 8 <0xff 0xff 0xff 0xff 0xff 0xff 0x08 0x0f 0x0f 0x00 0xff 0xff 0x18>;
+			qcom,ports-block-pack-mode =	/bits/ 8 <0x00 0x01 0x01 0x00 0x01 0x01 0x00 0x01 0x01 0x01 0x01 0x00 0x00>;
+			qcom,ports-block-group-count =	/bits/ 8 <0xff 0xff 0xff 0xff 0xff 0xff 0xff 0x00 0x00 0xff 0xff 0xff 0xff>;
+			qcom,ports-lane-control =	/bits/ 8 <0xff 0xff 0xff 0xff 0xff 0xff 0xff 0x00 0x00 0xff 0xff 0xff 0xff>;
+
+			#address-cells = <2>;
+			#size-cells = <0>;
+			#sound-dai-cells = <1>;
+			status = "disabled";
+		};
+
 		lpass_ag_noc: interconnect@7e40000 {
 			compatible = "qcom,sm8750-lpass-ag-noc";
 			reg = <0x0 0x07e40000 0x0 0xe080>;
@@ -2329,6 +2419,38 @@ lpass_lpicx_noc: interconnect@7420000 {
 			#interconnect-cells = <2>;
 		};
 
+		swr2: soundwire@7630000 {
+			compatible = "qcom,soundwire-v2.0.0";
+			reg = <0 0x07630000 0 0x10000>;
+			interrupts = <GIC_SPI 761 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 785 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "core", "wakeup";
+			clocks = <&lpass_txmacro>;
+			clock-names = "iface";
+			label = "TX";
+
+			pinctrl-0 = <&tx_swr_active>;
+			pinctrl-names = "default";
+
+			qcom,din-ports = <4>;
+			qcom,dout-ports = <0>;
+
+			qcom,ports-sinterval-low =	/bits/ 8 <0x01 0x01 0x03 0x03>;
+			qcom,ports-offset1 =		/bits/ 8 <0x00 0x00 0x01 0x01>;
+			qcom,ports-offset2 =		/bits/ 8 <0x00 0x00 0x00 0x00>;
+			qcom,ports-hstart =		/bits/ 8 <0xff 0xff 0xff 0xff>;
+			qcom,ports-hstop =		/bits/ 8 <0xff 0xff 0xff 0xff>;
+			qcom,ports-word-length =	/bits/ 8 <0xff 0xff 0xff 0xff>;
+			qcom,ports-block-pack-mode =	/bits/ 8 <0xff 0xff 0xff 0xff>;
+			qcom,ports-block-group-count =	/bits/ 8 <0xff 0xff 0xff 0xff>;
+			qcom,ports-lane-control =	/bits/ 8 <0x01 0x02 0x00 0x00>;
+
+			#address-cells = <2>;
+			#size-cells = <0>;
+			#sound-dai-cells = <1>;
+			status = "disabled";
+		};
+
 		lpass_vamacro: codec@7660000 {
 			compatible = "qcom,sm8750-lpass-va-macro", "qcom,sm8550-lpass-va-macro";
 			reg = <0x0 0x07660000 0x0 0x2000>;

-- 
2.45.2

[PATCH 2/2] arm64: dts: qcom: sm8750-mtp: Add sound (speakers, headset codec, dmics)

[Krzysztof Kozlowski]

Add device nodes for most of the sound support - WSA883x smart speakers,
WCD9395 audio codec (headset) and sound card - which allows sound
playback via speakers and recording via DMIC microphones.  Changes bring
necessary foundation for headset playback/recording via USB, but that
part is not yet ready.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8750-mtp.dts | 214 ++++++++++++++++++++++++++++++++
 1 file changed, 214 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8750-mtp.dts b/arch/arm64/boot/dts/qcom/sm8750-mtp.dts
index 72f081a890dfe49bfbee5e91b9e51da53b9d8baf..47645f15e6e327620840de8c3e03105540faf9cc 100644
--- a/arch/arm64/boot/dts/qcom/sm8750-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sm8750-mtp.dts
@@ -29,6 +29,32 @@ aliases {
 		serial0 = &uart7;
 	};
 
+	wcd939x: audio-codec {
+		compatible = "qcom,wcd9395-codec", "qcom,wcd9390-codec";
+
+		pinctrl-0 = <&wcd_default>;
+		pinctrl-names = "default";
+
+		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>;
+		qcom,rx-device = <&wcd_rx>;
+		qcom,tx-device = <&wcd_tx>;
+
+		reset-gpios = <&tlmm 101 GPIO_ACTIVE_LOW>;
+
+		vdd-buck-supply = <&vreg_l15b_1p8>;
+		vdd-rxtx-supply = <&vreg_l15b_1p8>;
+		vdd-io-supply = <&vreg_l15b_1p8>;
+		vdd-mic-bias-supply = <&vreg_bob1>;
+
+		#sound-dai-cells = <1>;
+	};
+
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
@@ -81,6 +107,89 @@ key-volume-up {
 		};
 	};
 
+	sound {
+		compatible = "qcom,sm8750-sndcard", "qcom,sm8450-sndcard";
+		model = "SM8750-MTP";
+		audio-routing = "SpkrLeft IN", "WSA_SPK1 OUT",
+				"SpkrRight IN", "WSA_SPK2 OUT",
+				"IN1_HPHL", "HPHL_OUT",
+				"IN2_HPHR", "HPHR_OUT",
+				"AMIC2", "MIC BIAS2",
+				"VA DMIC0", "MIC BIAS3", /* MIC4 on schematics */
+				"VA DMIC1", "MIC BIAS3", /* MIC1 on schematics */
+				"VA DMIC2", "MIC BIAS1",
+				"VA DMIC3", "MIC BIAS1",
+				"VA DMIC0", "VA MIC BIAS3",
+				"VA DMIC1", "VA MIC BIAS3",
+				"VA DMIC2", "VA MIC BIAS1",
+				"VA DMIC3", "VA MIC BIAS1",
+				"TX SWR_INPUT1", "ADC2_OUTPUT";
+
+		wcd-playback-dai-link {
+			link-name = "WCD Playback";
+
+			cpu {
+				sound-dai = <&q6apmbedai RX_CODEC_DMA_RX_0>;
+			};
+
+			codec {
+				sound-dai = <&wcd939x 0>, <&swr1 0>, <&lpass_rxmacro 0>;
+			};
+
+			platform {
+				sound-dai = <&q6apm>;
+			};
+		};
+
+		wcd-capture-dai-link {
+			link-name = "WCD Capture";
+
+			cpu {
+				sound-dai = <&q6apmbedai TX_CODEC_DMA_TX_3>;
+			};
+
+			codec {
+				sound-dai = <&wcd939x 1>, <&swr2 0>, <&lpass_txmacro 0>;
+			};
+
+			platform {
+				sound-dai = <&q6apm>;
+			};
+		};
+
+		wsa-dai-link {
+			link-name = "WSA Playback";
+
+			cpu {
+				sound-dai = <&q6apmbedai WSA_CODEC_DMA_RX_0>;
+			};
+
+			codec {
+				sound-dai = <&left_spkr>, <&right_spkr>, <&swr0 0>, <&lpass_wsamacro 0>;
+			};
+
+			platform {
+				sound-dai = <&q6apm>;
+			};
+		};
+
+		va-dai-link {
+			link-name = "VA Capture";
+
+			cpu {
+				sound-dai = <&q6apmbedai VA_CODEC_DMA_TX_0>;
+			};
+
+			codec {
+				sound-dai = <&lpass_vamacro 0>;
+			};
+
+			platform {
+				sound-dai = <&q6apm>;
+			};
+		};
+	};
+
 	vph_pwr: vph-pwr-regulator {
 		compatible = "regulator-fixed";
 
@@ -702,6 +811,14 @@ vreg_l7n_3p3: ldo7 {
 	};
 };
 
+&lpass_vamacro {
+	pinctrl-0 = <&dmic01_default>, <&dmic23_default>;
+	pinctrl-names = "default";
+
+	vdd-micb-supply = <&vreg_l1b_1p8>;
+	qcom,dmic-sample-rate = <4800000>;
+};
+
 &pm8550_flash {
 	status = "okay";
 
@@ -806,6 +923,74 @@ &remoteproc_mpss {
 	status = "fail";
 };
 
+&swr0 {
+	status = "okay";
+
+	/* WSA883x, left/front speaker */
+	left_spkr: speaker@0,1 {
+		compatible = "sdw10217020200";
+		reg = <0 1>;
+		pinctrl-0 = <&spkr_0_sd_n_active>;
+		pinctrl-names = "default";
+		powerdown-gpios = <&lpass_tlmm 17 GPIO_ACTIVE_LOW>;
+		#sound-dai-cells = <0>;
+		sound-name-prefix = "SpkrLeft";
+		#thermal-sensor-cells = <0>;
+		vdd-supply = <&vreg_l15b_1p8>;
+	};
+
+	/* WSA883x, right/back speaker */
+	right_spkr: speaker@0,2 {
+		compatible = "sdw10217020200";
+		reg = <0 2>;
+		pinctrl-0 = <&spkr_1_sd_n_active>;
+		pinctrl-names = "default";
+		powerdown-gpios = <&lpass_tlmm 18 GPIO_ACTIVE_LOW>;
+		#sound-dai-cells = <0>;
+		sound-name-prefix = "SpkrRight";
+		#thermal-sensor-cells = <0>;
+		vdd-supply = <&vreg_l15b_1p8>;
+	};
+};
+
+&swr1 {
+	status = "okay";
+
+	/* WCD9395 RX */
+	wcd_rx: codec@0,4 {
+		compatible = "sdw20217010e00";
+		reg = <0 4>;
+
+		/*
+		 * WCD9395 RX Port 1 (HPH_L/R)      <=> SWR1 Port 1 (HPH_L/R)
+		 * WCD9395 RX Port 2 (CLSH)         <=> SWR1 Port 2 (CLSH)
+		 * WCD9395 RX Port 3 (COMP_L/R)     <=> SWR1 Port 3 (COMP_L/R)
+		 * WCD9395 RX Port 4 (LO)           <=> SWR1 Port 4 (LO)
+		 * WCD9395 RX Port 5 (DSD_L/R)      <=> SWR1 Port 5 (DSD_L/R)
+		 * WCD9395 RX Port 6 (HIFI_PCM_L/R) <=> SWR1 Port 9 (HIFI_PCM_L/R)
+		 */
+		qcom,rx-port-mapping = <1 2 3 4 5 9>;
+	};
+};
+
+&swr2 {
+	status = "okay";
+
+	/* WCD9395 TX */
+	wcd_tx: codec@0,3 {
+		compatible = "sdw20217010e00";
+		reg = <0 3>;
+
+		/*
+		 * WCD9395 TX Port 1 (ADC1,2,3,4)         <=> SWR2 Port 2 (TX SWR_INPUT 0,1,2,3)
+		 * WCD9395 TX Port 2 (ADC3,4 & DMIC0,1)   <=> SWR2 Port 2 (TX SWR_INPUT 0,1,2,3)
+		 * WCD9395 TX Port 3 (DMIC0,1,2,3 & MBHC) <=> SWR2 Port 3 (TX SWR_INPUT 4,5,6,7)
+		 * WCD9395 TX Port 4 (DMIC4,5,6,7)        <=> SWR2 Port 4 (TX SWR_INPUT 8,9,10,11)
+		 */
+		qcom,tx-port-mapping = <2 2 3 4>;
+	};
+};
+
 &tlmm {
 	/* reserved for secure world */
 	gpio-reserved-ranges = <36 4>, <74 1>;
@@ -814,3 +999,32 @@ &tlmm {
 &uart7 {
 	status = "okay";
 };
+
+/* Pinctrl */
+&lpass_tlmm {
+	spkr_0_sd_n_active: spkr-0-sd-n-active-state {
+		pins = "gpio17";
+		function = "gpio";
+		drive-strength = <16>;
+		bias-disable;
+		output-low;
+	};
+
+	spkr_1_sd_n_active: spkr-1-sd-n-active-state {
+		pins = "gpio18";
+		function = "gpio";
+		drive-strength = <16>;
+		bias-disable;
+		output-low;
+	};
+};
+
+&tlmm {
+	wcd_default: wcd-reset-n-active-state {
+		pins = "gpio101";
+		function = "gpio";
+		drive-strength = <16>;
+		bias-disable;
+		output-low;
+	};
+};

-- 
2.45.2

Re: [PATCH 1/2] arm64: dts: qcom: sm8750: Add Soundwire nodes

[Konrad Dybcio]

On 4/24/25 11:40 AM, Krzysztof Kozlowski wrote:
> Add Soundwire controllers on SM8750, fully compatible with earlier
> SM8650 generation.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8750.dtsi | 122 +++++++++++++++++++++++++++++++++++
>  1 file changed, 122 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
> index 149d2ed17641a085d510f3a8eab5a96304787f0c..1e7aa25c675e76ce6aa571e04d7117b8c2ab25f8 100644
> --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
> @@ -2257,6 +2257,36 @@ lpass_wsa2macro: codec@6aa0000 {
>  			#sound-dai-cells = <1>;
>  		};
>  
> +		swr3: soundwire@6ab0000 {
> +			compatible = "qcom,soundwire-v2.0.0";

They're v2.1.0, same on 8650, there's a number of new registers

[...]


> +		swr2: soundwire@7630000 {
> +			compatible = "qcom,soundwire-v2.0.0";
> +			reg = <0 0x07630000 0 0x10000>;
> +			interrupts = <GIC_SPI 761 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 785 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "core", "wakeup";
> +			clocks = <&lpass_txmacro>;
> +			clock-names = "iface";
> +			label = "TX";
> +
> +			pinctrl-0 = <&tx_swr_active>;
> +			pinctrl-names = "default";
> +
> +			qcom,din-ports = <4>;
> +			qcom,dout-ports = <0>;

There's 1 OUT port on this instance

otherwise (modulo the settings I don't have a reference for)

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH 2/2] arm64: dts: qcom: sm8750-mtp: Add sound (speakers, headset codec, dmics)

[Konrad Dybcio]

On 4/24/25 11:40 AM, Krzysztof Kozlowski wrote:
> Add device nodes for most of the sound support - WSA883x smart speakers,
> WCD9395 audio codec (headset) and sound card - which allows sound
> playback via speakers and recording via DMIC microphones.  Changes bring
> necessary foundation for headset playback/recording via USB, but that
> part is not yet ready.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---

[...]

> +	sound {
> +		compatible = "qcom,sm8750-sndcard", "qcom,sm8450-sndcard";
> +		model = "SM8750-MTP";
> +		audio-routing = "SpkrLeft IN", "WSA_SPK1 OUT",
> +				"SpkrRight IN", "WSA_SPK2 OUT",
> +				"IN1_HPHL", "HPHL_OUT",
> +				"IN2_HPHR", "HPHR_OUT",
> +				"AMIC2", "MIC BIAS2",
> +				"VA DMIC0", "MIC BIAS3", /* MIC4 on schematics */
> +				"VA DMIC1", "MIC BIAS3", /* MIC1 on schematics */

Is this a mistake in what the codec driver exposes, or just a fumble
in numbering $somewhere?

> +				"VA DMIC2", "MIC BIAS1",
> +				"VA DMIC3", "MIC BIAS1",
> +				"VA DMIC0", "VA MIC BIAS3",
> +				"VA DMIC1", "VA MIC BIAS3",
> +				"VA DMIC2", "VA MIC BIAS1",
> +				"VA DMIC3", "VA MIC BIAS1",
> +				"TX SWR_INPUT1", "ADC2_OUTPUT";
> +
> +		wcd-playback-dai-link {
> +			link-name = "WCD Playback";
> +
> +			cpu {
> +				sound-dai = <&q6apmbedai RX_CODEC_DMA_RX_0>;
> +			};
> +
> +			codec {

'co'dec < 'cp'u

[...]

> +		/*
> +		 * WCD9395 RX Port 1 (HPH_L/R)      <=> SWR1 Port 1 (HPH_L/R)
> +		 * WCD9395 RX Port 2 (CLSH)         <=> SWR1 Port 2 (CLSH)
> +		 * WCD9395 RX Port 3 (COMP_L/R)     <=> SWR1 Port 3 (COMP_L/R)
> +		 * WCD9395 RX Port 4 (LO)           <=> SWR1 Port 4 (LO)
> +		 * WCD9395 RX Port 5 (DSD_L/R)      <=> SWR1 Port 5 (DSD_L/R)
> +		 * WCD9395 RX Port 6 (HIFI_PCM_L/R) <=> SWR1 Port 9 (HIFI_PCM_L/R)
> +		 */
> +		qcom,rx-port-mapping = <1 2 3 4 5 9>;

Does this deserve some dt-bindings constants?

Konrad

Re: [PATCH 2/2] arm64: dts: qcom: sm8750-mtp: Add sound (speakers, headset codec, dmics)

[Krzysztof Kozlowski]

On 25/04/2025 11:30, Konrad Dybcio wrote:
> On 4/24/25 11:40 AM, Krzysztof Kozlowski wrote:
>> Add device nodes for most of the sound support - WSA883x smart speakers,
>> WCD9395 audio codec (headset) and sound card - which allows sound
>> playback via speakers and recording via DMIC microphones.  Changes bring
>> necessary foundation for headset playback/recording via USB, but that
>> part is not yet ready.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
> 
> [...]
> 
>> +	sound {
>> +		compatible = "qcom,sm8750-sndcard", "qcom,sm8450-sndcard";
>> +		model = "SM8750-MTP";
>> +		audio-routing = "SpkrLeft IN", "WSA_SPK1 OUT",
>> +				"SpkrRight IN", "WSA_SPK2 OUT",
>> +				"IN1_HPHL", "HPHL_OUT",
>> +				"IN2_HPHR", "HPHR_OUT",
>> +				"AMIC2", "MIC BIAS2",
>> +				"VA DMIC0", "MIC BIAS3", /* MIC4 on schematics */
>> +				"VA DMIC1", "MIC BIAS3", /* MIC1 on schematics */
> 
> Is this a mistake in what the codec driver exposes, or just a fumble
> in numbering $somewhere?

Which mistake? MIC4? Schematics call name things differently. They
always were, so to make it clear for people without schematics I wrote
which MIC it actually is.

> 
>> +				"VA DMIC2", "MIC BIAS1",
>> +				"VA DMIC3", "MIC BIAS1",
>> +				"VA DMIC0", "VA MIC BIAS3",
>> +				"VA DMIC1", "VA MIC BIAS3",
>> +				"VA DMIC2", "VA MIC BIAS1",
>> +				"VA DMIC3", "VA MIC BIAS1",
>> +				"TX SWR_INPUT1", "ADC2_OUTPUT";
>> +
>> +		wcd-playback-dai-link {
>> +			link-name = "WCD Playback";
>> +
>> +			cpu {
>> +				sound-dai = <&q6apmbedai RX_CODEC_DMA_RX_0>;
>> +			};
>> +
>> +			codec {
> 
> 'co'dec < 'cp'u
> 
> [...]

That was the convention so far, but we can start a new one, sure. Just
ask the same all other patch contributors, because each of them will be
copying old code, which means cpu->codec->platform

> 
>> +		/*
>> +		 * WCD9395 RX Port 1 (HPH_L/R)      <=> SWR1 Port 1 (HPH_L/R)
>> +		 * WCD9395 RX Port 2 (CLSH)         <=> SWR1 Port 2 (CLSH)
>> +		 * WCD9395 RX Port 3 (COMP_L/R)     <=> SWR1 Port 3 (COMP_L/R)
>> +		 * WCD9395 RX Port 4 (LO)           <=> SWR1 Port 4 (LO)
>> +		 * WCD9395 RX Port 5 (DSD_L/R)      <=> SWR1 Port 5 (DSD_L/R)
>> +		 * WCD9395 RX Port 6 (HIFI_PCM_L/R) <=> SWR1 Port 9 (HIFI_PCM_L/R)
>> +		 */
>> +		qcom,rx-port-mapping = <1 2 3 4 5 9>;
> 
> Does this deserve some dt-bindings constants?

No, because these are hardware details/constants. Drivers do not use them.

Best regards,
Krzysztof

Re: [PATCH 2/2] arm64: dts: qcom: sm8750-mtp: Add sound (speakers, headset codec, dmics)

[Konrad Dybcio]

On 4/28/25 4:41 PM, Krzysztof Kozlowski wrote:
> On 25/04/2025 11:30, Konrad Dybcio wrote:
>> On 4/24/25 11:40 AM, Krzysztof Kozlowski wrote:
>>> Add device nodes for most of the sound support - WSA883x smart speakers,
>>> WCD9395 audio codec (headset) and sound card - which allows sound
>>> playback via speakers and recording via DMIC microphones.  Changes bring
>>> necessary foundation for headset playback/recording via USB, but that
>>> part is not yet ready.
>>>
>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> ---
>>
>> [...]
>>
>>> +	sound {
>>> +		compatible = "qcom,sm8750-sndcard", "qcom,sm8450-sndcard";
>>> +		model = "SM8750-MTP";
>>> +		audio-routing = "SpkrLeft IN", "WSA_SPK1 OUT",
>>> +				"SpkrRight IN", "WSA_SPK2 OUT",
>>> +				"IN1_HPHL", "HPHL_OUT",
>>> +				"IN2_HPHR", "HPHR_OUT",
>>> +				"AMIC2", "MIC BIAS2",
>>> +				"VA DMIC0", "MIC BIAS3", /* MIC4 on schematics */
>>> +				"VA DMIC1", "MIC BIAS3", /* MIC1 on schematics */
>>
>> Is this a mistake in what the codec driver exposes, or just a fumble
>> in numbering $somewhere?
> 
> Which mistake? MIC4? Schematics call name things differently. They
> always were, so to make it clear for people without schematics I wrote
> which MIC it actually is.

I'm not sure how to parse your response

are you saying that there are MIC[0..4] that are/may be connected
to different codec ports, and that the MIC4/1 lines are plumbed to
VA DMIC0/1 respectively?

I think I got confused about the MIC BIAS3 going to both and none
matching the index, but perhaps that's just because it comes from
the WCD (which is the third piece of hw involved beyond VA and the
mic itself)

> 
>>
>>> +				"VA DMIC2", "MIC BIAS1",
>>> +				"VA DMIC3", "MIC BIAS1",
>>> +				"VA DMIC0", "VA MIC BIAS3",
>>> +				"VA DMIC1", "VA MIC BIAS3",
>>> +				"VA DMIC2", "VA MIC BIAS1",
>>> +				"VA DMIC3", "VA MIC BIAS1",
>>> +				"TX SWR_INPUT1", "ADC2_OUTPUT";
>>> +
>>> +		wcd-playback-dai-link {
>>> +			link-name = "WCD Playback";
>>> +
>>> +			cpu {
>>> +				sound-dai = <&q6apmbedai RX_CODEC_DMA_RX_0>;
>>> +			};
>>> +
>>> +			codec {
>>
>> 'co'dec < 'cp'u
>>
>> [...]
> 
> That was the convention so far, but we can start a new one, sure. Just
> ask the same all other patch contributors, because each of them will be
> copying old code, which means cpu->codec->platform

I've been doing just that for the past couple weeks indeed

>>> +		/*
>>> +		 * WCD9395 RX Port 1 (HPH_L/R)      <=> SWR1 Port 1 (HPH_L/R)
>>> +		 * WCD9395 RX Port 2 (CLSH)         <=> SWR1 Port 2 (CLSH)
>>> +		 * WCD9395 RX Port 3 (COMP_L/R)     <=> SWR1 Port 3 (COMP_L/R)
>>> +		 * WCD9395 RX Port 4 (LO)           <=> SWR1 Port 4 (LO)
>>> +		 * WCD9395 RX Port 5 (DSD_L/R)      <=> SWR1 Port 5 (DSD_L/R)
>>> +		 * WCD9395 RX Port 6 (HIFI_PCM_L/R) <=> SWR1 Port 9 (HIFI_PCM_L/R)
>>> +		 */
>>> +		qcom,rx-port-mapping = <1 2 3 4 5 9>;
>>
>> Does this deserve some dt-bindings constants?
> 
> No, because these are hardware details/constants. Drivers do not use them.

I'd argue it makes sense here - it makes more sense to pass meaningfully
named constants to the driver, rather than blobs with a comment

Konrad

Re: [PATCH 2/2] arm64: dts: qcom: sm8750-mtp: Add sound (speakers, headset codec, dmics)

[Krzysztof Kozlowski]

On 29/04/2025 21:11, Konrad Dybcio wrote:
> On 4/28/25 4:41 PM, Krzysztof Kozlowski wrote:
>> On 25/04/2025 11:30, Konrad Dybcio wrote:
>>> On 4/24/25 11:40 AM, Krzysztof Kozlowski wrote:
>>>> Add device nodes for most of the sound support - WSA883x smart speakers,
>>>> WCD9395 audio codec (headset) and sound card - which allows sound
>>>> playback via speakers and recording via DMIC microphones.  Changes bring
>>>> necessary foundation for headset playback/recording via USB, but that
>>>> part is not yet ready.
>>>>
>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>> ---
>>>
>>> [...]
>>>
>>>> +	sound {
>>>> +		compatible = "qcom,sm8750-sndcard", "qcom,sm8450-sndcard";
>>>> +		model = "SM8750-MTP";
>>>> +		audio-routing = "SpkrLeft IN", "WSA_SPK1 OUT",
>>>> +				"SpkrRight IN", "WSA_SPK2 OUT",
>>>> +				"IN1_HPHL", "HPHL_OUT",
>>>> +				"IN2_HPHR", "HPHR_OUT",
>>>> +				"AMIC2", "MIC BIAS2",
>>>> +				"VA DMIC0", "MIC BIAS3", /* MIC4 on schematics */
>>>> +				"VA DMIC1", "MIC BIAS3", /* MIC1 on schematics */
>>>
>>> Is this a mistake in what the codec driver exposes, or just a fumble
>>> in numbering $somewhere?
>>
>> Which mistake? MIC4? Schematics call name things differently. They
>> always were, so to make it clear for people without schematics I wrote
>> which MIC it actually is.
> 
> I'm not sure how to parse your response
> 
> are you saying that there are MIC[0..4] that are/may be connected
> to different codec ports, and that the MIC4/1 lines are plumbed to
> VA DMIC0/1 respectively?

Yes, as always. Nothing weird here.

> 
> I think I got confused about the MIC BIAS3 going to both and none

What is both and none?

> matching the index, but perhaps that's just because it comes from
> the WCD (which is the third piece of hw involved beyond VA and the
> mic itself)

Again, what is the mistake you are pointing here?

> 
>>
>>>
>>>> +				"VA DMIC2", "MIC BIAS1",
>>>> +				"VA DMIC3", "MIC BIAS1",
>>>> +				"VA DMIC0", "VA MIC BIAS3",
>>>> +				"VA DMIC1", "VA MIC BIAS3",
>>>> +				"VA DMIC2", "VA MIC BIAS1",
>>>> +				"VA DMIC3", "VA MIC BIAS1",
>>>> +				"TX SWR_INPUT1", "ADC2_OUTPUT";
>>>> +
>>>> +		wcd-playback-dai-link {
>>>> +			link-name = "WCD Playback";
>>>> +
>>>> +			cpu {
>>>> +				sound-dai = <&q6apmbedai RX_CODEC_DMA_RX_0>;
>>>> +			};
>>>> +
>>>> +			codec {
>>>
>>> 'co'dec < 'cp'u
>>>
>>> [...]
>>
>> That was the convention so far, but we can start a new one, sure. Just
>> ask the same all other patch contributors, because each of them will be
>> copying old code, which means cpu->codec->platform
> 
> I've been doing just that for the past couple weeks indeed
> 
>>>> +		/*
>>>> +		 * WCD9395 RX Port 1 (HPH_L/R)      <=> SWR1 Port 1 (HPH_L/R)
>>>> +		 * WCD9395 RX Port 2 (CLSH)         <=> SWR1 Port 2 (CLSH)
>>>> +		 * WCD9395 RX Port 3 (COMP_L/R)     <=> SWR1 Port 3 (COMP_L/R)
>>>> +		 * WCD9395 RX Port 4 (LO)           <=> SWR1 Port 4 (LO)
>>>> +		 * WCD9395 RX Port 5 (DSD_L/R)      <=> SWR1 Port 5 (DSD_L/R)
>>>> +		 * WCD9395 RX Port 6 (HIFI_PCM_L/R) <=> SWR1 Port 9 (HIFI_PCM_L/R)
>>>> +		 */
>>>> +		qcom,rx-port-mapping = <1 2 3 4 5 9>;
>>>
>>> Does this deserve some dt-bindings constants?
>>
>> No, because these are hardware details/constants. Drivers do not use them.
> 
> I'd argue it makes sense here - it makes more sense to pass meaningfully
> named constants to the driver, rather than blobs with a comment

Sense of what? You want to make it a binding then answer what does it
bind, what part of ABI for driver is here a binding (answer none:
because driver does not use it)?



Best regards,
Krzysztof

Re: [PATCH 2/2] arm64: dts: qcom: sm8750-mtp: Add sound (speakers, headset codec, dmics)

[Konrad Dybcio]

On 4/30/25 8:19 AM, Krzysztof Kozlowski wrote:
> On 29/04/2025 21:11, Konrad Dybcio wrote:
>> On 4/28/25 4:41 PM, Krzysztof Kozlowski wrote:
>>> On 25/04/2025 11:30, Konrad Dybcio wrote:
>>>> On 4/24/25 11:40 AM, Krzysztof Kozlowski wrote:
>>>>> Add device nodes for most of the sound support - WSA883x smart speakers,
>>>>> WCD9395 audio codec (headset) and sound card - which allows sound
>>>>> playback via speakers and recording via DMIC microphones.  Changes bring
>>>>> necessary foundation for headset playback/recording via USB, but that
>>>>> part is not yet ready.
>>>>>
>>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>> ---
>>>>
>>>> [...]
>>>>
>>>>> +	sound {
>>>>> +		compatible = "qcom,sm8750-sndcard", "qcom,sm8450-sndcard";
>>>>> +		model = "SM8750-MTP";
>>>>> +		audio-routing = "SpkrLeft IN", "WSA_SPK1 OUT",
>>>>> +				"SpkrRight IN", "WSA_SPK2 OUT",
>>>>> +				"IN1_HPHL", "HPHL_OUT",
>>>>> +				"IN2_HPHR", "HPHR_OUT",
>>>>> +				"AMIC2", "MIC BIAS2",
>>>>> +				"VA DMIC0", "MIC BIAS3", /* MIC4 on schematics */
>>>>> +				"VA DMIC1", "MIC BIAS3", /* MIC1 on schematics */
>>>>
>>>> Is this a mistake in what the codec driver exposes, or just a fumble
>>>> in numbering $somewhere?
>>>
>>> Which mistake? MIC4? Schematics call name things differently. They
>>> always were, so to make it clear for people without schematics I wrote
>>> which MIC it actually is.
>>
>> I'm not sure how to parse your response
>>
>> are you saying that there are MIC[0..4] that are/may be connected
>> to different codec ports, and that the MIC4/1 lines are plumbed to
>> VA DMIC0/1 respectively?
> 
> Yes, as always. Nothing weird here.
> 
>>
>> I think I got confused about the MIC BIAS3 going to both and none
> 
> What is both and none?

missing Oxford comma I suppose.. MIC BIAS3 goes to both MIC4 and MIC1
and neither of them has a '3' in the name. I was wondering whether
that's intentional.

> 
>> matching the index, but perhaps that's just because it comes from
>> the WCD (which is the third piece of hw involved beyond VA and the
>> mic itself)
> 
> Again, what is the mistake you are pointing here?

I'm not necessarily saying this is a mistake, just thinking out loud
how I understand the non-obvious plumbing

[...]

>>>>> +		/*
>>>>> +		 * WCD9395 RX Port 1 (HPH_L/R)      <=> SWR1 Port 1 (HPH_L/R)
>>>>> +		 * WCD9395 RX Port 2 (CLSH)         <=> SWR1 Port 2 (CLSH)
>>>>> +		 * WCD9395 RX Port 3 (COMP_L/R)     <=> SWR1 Port 3 (COMP_L/R)
>>>>> +		 * WCD9395 RX Port 4 (LO)           <=> SWR1 Port 4 (LO)
>>>>> +		 * WCD9395 RX Port 5 (DSD_L/R)      <=> SWR1 Port 5 (DSD_L/R)
>>>>> +		 * WCD9395 RX Port 6 (HIFI_PCM_L/R) <=> SWR1 Port 9 (HIFI_PCM_L/R)
>>>>> +		 */
>>>>> +		qcom,rx-port-mapping = <1 2 3 4 5 9>;
>>>>
>>>> Does this deserve some dt-bindings constants?
>>>
>>> No, because these are hardware details/constants. Drivers do not use them.
>>
>> I'd argue it makes sense here - it makes more sense to pass meaningfully
>> named constants to the driver, rather than blobs with a comment
> 
> Sense of what? You want to make it a binding then answer what does it
> bind, what part of ABI for driver is here a binding (answer none:
> because driver does not use it)?

Sense of the magic numbers that otherwise require a comment.

dt-bindings don't exclusively contain enums-turned-defines that are
indices of C arrays - some contain various forms of hardware ABI, be
it register addresses, or names for magic values

Konrad

Re: [PATCH 2/2] arm64: dts: qcom: sm8750-mtp: Add sound (speakers, headset codec, dmics)

[neil.armstrong]

On 30/04/2025 12:48, Konrad Dybcio wrote:
> On 4/30/25 8:19 AM, Krzysztof Kozlowski wrote:
>> On 29/04/2025 21:11, Konrad Dybcio wrote:
>>> On 4/28/25 4:41 PM, Krzysztof Kozlowski wrote:
>>>> On 25/04/2025 11:30, Konrad Dybcio wrote:
>>>>> On 4/24/25 11:40 AM, Krzysztof Kozlowski wrote:
>>>>>> Add device nodes for most of the sound support - WSA883x smart speakers,
>>>>>> WCD9395 audio codec (headset) and sound card - which allows sound
>>>>>> playback via speakers and recording via DMIC microphones.  Changes bring
>>>>>> necessary foundation for headset playback/recording via USB, but that
>>>>>> part is not yet ready.
>>>>>>
>>>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>>> ---
>>>>>
>>>>> [...]
>>>>>
>>>>>> +	sound {
>>>>>> +		compatible = "qcom,sm8750-sndcard", "qcom,sm8450-sndcard";
>>>>>> +		model = "SM8750-MTP";
>>>>>> +		audio-routing = "SpkrLeft IN", "WSA_SPK1 OUT",
>>>>>> +				"SpkrRight IN", "WSA_SPK2 OUT",
>>>>>> +				"IN1_HPHL", "HPHL_OUT",
>>>>>> +				"IN2_HPHR", "HPHR_OUT",
>>>>>> +				"AMIC2", "MIC BIAS2",
>>>>>> +				"VA DMIC0", "MIC BIAS3", /* MIC4 on schematics */
>>>>>> +				"VA DMIC1", "MIC BIAS3", /* MIC1 on schematics */
>>>>>
>>>>> Is this a mistake in what the codec driver exposes, or just a fumble
>>>>> in numbering $somewhere?
>>>>
>>>> Which mistake? MIC4? Schematics call name things differently. They
>>>> always were, so to make it clear for people without schematics I wrote
>>>> which MIC it actually is.
>>>
>>> I'm not sure how to parse your response
>>>
>>> are you saying that there are MIC[0..4] that are/may be connected
>>> to different codec ports, and that the MIC4/1 lines are plumbed to
>>> VA DMIC0/1 respectively?
>>
>> Yes, as always. Nothing weird here.
>>
>>>
>>> I think I got confused about the MIC BIAS3 going to both and none
>>
>> What is both and none?
> 
> missing Oxford comma I suppose.. MIC BIAS3 goes to both MIC4 and MIC1
> and neither of them has a '3' in the name. I was wondering whether
> that's intentional.
> 
>>
>>> matching the index, but perhaps that's just because it comes from
>>> the WCD (which is the third piece of hw involved beyond VA and the
>>> mic itself)
>>
>> Again, what is the mistake you are pointing here?
> 
> I'm not necessarily saying this is a mistake, just thinking out loud
> how I understand the non-obvious plumbing
> 
> [...]
> 
>>>>>> +		/*
>>>>>> +		 * WCD9395 RX Port 1 (HPH_L/R)      <=> SWR1 Port 1 (HPH_L/R)
>>>>>> +		 * WCD9395 RX Port 2 (CLSH)         <=> SWR1 Port 2 (CLSH)
>>>>>> +		 * WCD9395 RX Port 3 (COMP_L/R)     <=> SWR1 Port 3 (COMP_L/R)
>>>>>> +		 * WCD9395 RX Port 4 (LO)           <=> SWR1 Port 4 (LO)
>>>>>> +		 * WCD9395 RX Port 5 (DSD_L/R)      <=> SWR1 Port 5 (DSD_L/R)
>>>>>> +		 * WCD9395 RX Port 6 (HIFI_PCM_L/R) <=> SWR1 Port 9 (HIFI_PCM_L/R)
>>>>>> +		 */
>>>>>> +		qcom,rx-port-mapping = <1 2 3 4 5 9>;
>>>>>
>>>>> Does this deserve some dt-bindings constants?
>>>>
>>>> No, because these are hardware details/constants. Drivers do not use them.
>>>
>>> I'd argue it makes sense here - it makes more sense to pass meaningfully
>>> named constants to the driver, rather than blobs with a comment
>>
>> Sense of what? You want to make it a binding then answer what does it
>> bind, what part of ABI for driver is here a binding (answer none:
>> because driver does not use it)?
> 
> Sense of the magic numbers that otherwise require a comment.

There's no magic numbers, index of qcom,rx-port-mapping is the RX port,
value is the SWR1 port index. As the property name says, it maps RX ports.

The comment is here to understand why we map as-is, and what the ports are
used for, but for the soundwire perspective only the numbers matters.

> 
> dt-bindings don't exclusively contain enums-turned-defines that are
> indices of C arrays - some contain various forms of hardware ABI, be
> it register addresses, or names for magic values
> 
> Konrad
> 

Re: [PATCH 2/2] arm64: dts: qcom: sm8750-mtp: Add sound (speakers, headset codec, dmics)

[Konrad Dybcio]

On 4/30/25 1:07 PM, neil.armstrong@linaro.org wrote:
> On 30/04/2025 12:48, Konrad Dybcio wrote:
>> On 4/30/25 8:19 AM, Krzysztof Kozlowski wrote:
>>> On 29/04/2025 21:11, Konrad Dybcio wrote:
>>>> On 4/28/25 4:41 PM, Krzysztof Kozlowski wrote:
>>>>> On 25/04/2025 11:30, Konrad Dybcio wrote:
>>>>>> On 4/24/25 11:40 AM, Krzysztof Kozlowski wrote:
>>>>>>> Add device nodes for most of the sound support - WSA883x smart speakers,
>>>>>>> WCD9395 audio codec (headset) and sound card - which allows sound
>>>>>>> playback via speakers and recording via DMIC microphones.  Changes bring
>>>>>>> necessary foundation for headset playback/recording via USB, but that
>>>>>>> part is not yet ready.
>>>>>>>
>>>>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>>>> ---

[...]

>>>>>>> +        /*
>>>>>>> +         * WCD9395 RX Port 1 (HPH_L/R)      <=> SWR1 Port 1 (HPH_L/R)
>>>>>>> +         * WCD9395 RX Port 2 (CLSH)         <=> SWR1 Port 2 (CLSH)
>>>>>>> +         * WCD9395 RX Port 3 (COMP_L/R)     <=> SWR1 Port 3 (COMP_L/R)
>>>>>>> +         * WCD9395 RX Port 4 (LO)           <=> SWR1 Port 4 (LO)
>>>>>>> +         * WCD9395 RX Port 5 (DSD_L/R)      <=> SWR1 Port 5 (DSD_L/R)
>>>>>>> +         * WCD9395 RX Port 6 (HIFI_PCM_L/R) <=> SWR1 Port 9 (HIFI_PCM_L/R)
>>>>>>> +         */
>>>>>>> +        qcom,rx-port-mapping = <1 2 3 4 5 9>;
>>>>>>
>>>>>> Does this deserve some dt-bindings constants?
>>>>>
>>>>> No, because these are hardware details/constants. Drivers do not use them.
>>>>
>>>> I'd argue it makes sense here - it makes more sense to pass meaningfully
>>>> named constants to the driver, rather than blobs with a comment
>>>
>>> Sense of what? You want to make it a binding then answer what does it
>>> bind, what part of ABI for driver is here a binding (answer none:
>>> because driver does not use it)?
>>
>> Sense of the magic numbers that otherwise require a comment.
> 
> There's no magic numbers, index of qcom,rx-port-mapping is the RX port,
> value is the SWR1 port index. As the property name says, it maps RX ports.
> 
> The comment is here to understand why we map as-is, and what the ports are
> used for, but for the soundwire perspective only the numbers matters.

OK so it's the indices on the WCD side that are hardwired, IIUC.

So perhaps that comment could be included in qcom,wcd939x-sdw.yaml
under items: for qcom,rx-port-mapping

Konrad

Re: [PATCH 2/2] arm64: dts: qcom: sm8750-mtp: Add sound (speakers, headset codec, dmics)

[Krzysztof Kozlowski]

On 30/04/2025 12:48, Konrad Dybcio wrote:
>>>>>> +		/*
>>>>>> +		 * WCD9395 RX Port 1 (HPH_L/R)      <=> SWR1 Port 1 (HPH_L/R)
>>>>>> +		 * WCD9395 RX Port 2 (CLSH)         <=> SWR1 Port 2 (CLSH)
>>>>>> +		 * WCD9395 RX Port 3 (COMP_L/R)     <=> SWR1 Port 3 (COMP_L/R)
>>>>>> +		 * WCD9395 RX Port 4 (LO)           <=> SWR1 Port 4 (LO)
>>>>>> +		 * WCD9395 RX Port 5 (DSD_L/R)      <=> SWR1 Port 5 (DSD_L/R)
>>>>>> +		 * WCD9395 RX Port 6 (HIFI_PCM_L/R) <=> SWR1 Port 9 (HIFI_PCM_L/R)
>>>>>> +		 */
>>>>>> +		qcom,rx-port-mapping = <1 2 3 4 5 9>;
>>>>>
>>>>> Does this deserve some dt-bindings constants?
>>>>
>>>> No, because these are hardware details/constants. Drivers do not use them.
>>>
>>> I'd argue it makes sense here - it makes more sense to pass meaningfully
>>> named constants to the driver, rather than blobs with a comment
>>
>> Sense of what? You want to make it a binding then answer what does it
>> bind, what part of ABI for driver is here a binding (answer none:
>> because driver does not use it)?
> 
> Sense of the magic numbers that otherwise require a comment.
> 
> dt-bindings don't exclusively contain enums-turned-defines that are

No, they don't.


Best regards,
Krzysztof

Re: [PATCH 1/2] arm64: dts: qcom: sm8750: Add Soundwire nodes

[Krzysztof Kozlowski]

On 25/04/2025 11:24, Konrad Dybcio wrote:
> On 4/24/25 11:40 AM, Krzysztof Kozlowski wrote:
>> Add Soundwire controllers on SM8750, fully compatible with earlier
>> SM8650 generation.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>  arch/arm64/boot/dts/qcom/sm8750.dtsi | 122 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 122 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
>> index 149d2ed17641a085d510f3a8eab5a96304787f0c..1e7aa25c675e76ce6aa571e04d7117b8c2ab25f8 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
>> @@ -2257,6 +2257,36 @@ lpass_wsa2macro: codec@6aa0000 {
>>  			#sound-dai-cells = <1>;
>>  		};
>>  
>> +		swr3: soundwire@6ab0000 {
>> +			compatible = "qcom,soundwire-v2.0.0";
> 
> They're v2.1.0, same on 8650, there's a number of new registers

Sorry, but no. This the "generic" compatible and it is correct. Devices
expose versions, which is perfectly usable, thus changing compatible to
different one is not useful. We could go with soc specific compatibles
and new generic one, but what would that solve? This one is generic
enough - the device is compatible with v2.0.


> 
> [...]
> 
> 
>> +		swr2: soundwire@7630000 {
>> +			compatible = "qcom,soundwire-v2.0.0";
>> +			reg = <0 0x07630000 0 0x10000>;
>> +			interrupts = <GIC_SPI 761 IRQ_TYPE_LEVEL_HIGH>,
>> +				     <GIC_SPI 785 IRQ_TYPE_LEVEL_HIGH>;
>> +			interrupt-names = "core", "wakeup";
>> +			clocks = <&lpass_txmacro>;
>> +			clock-names = "iface";
>> +			label = "TX";
>> +
>> +			pinctrl-0 = <&tx_swr_active>;
>> +			pinctrl-names = "default";
>> +
>> +			qcom,din-ports = <4>;
>> +			qcom,dout-ports = <0>;
> 
> There's 1 OUT port on this instance
> 
> otherwise (modulo the settings I don't have a reference for)

Omitted on purpose, no port configuration available.


Best regards,
Krzysztof

Re: [PATCH 1/2] arm64: dts: qcom: sm8750: Add Soundwire nodes

[Konrad Dybcio]

On 5/8/25 12:37 PM, Krzysztof Kozlowski wrote:
> On 25/04/2025 11:24, Konrad Dybcio wrote:
>> On 4/24/25 11:40 AM, Krzysztof Kozlowski wrote:
>>> Add Soundwire controllers on SM8750, fully compatible with earlier
>>> SM8650 generation.
>>>
>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> ---
>>>  arch/arm64/boot/dts/qcom/sm8750.dtsi | 122 +++++++++++++++++++++++++++++++++++
>>>  1 file changed, 122 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
>>> index 149d2ed17641a085d510f3a8eab5a96304787f0c..1e7aa25c675e76ce6aa571e04d7117b8c2ab25f8 100644
>>> --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
>>> @@ -2257,6 +2257,36 @@ lpass_wsa2macro: codec@6aa0000 {
>>>  			#sound-dai-cells = <1>;
>>>  		};
>>>  
>>> +		swr3: soundwire@6ab0000 {
>>> +			compatible = "qcom,soundwire-v2.0.0";
>>
>> They're v2.1.0, same on 8650, there's a number of new registers
> 
> Sorry, but no. This the "generic" compatible and it is correct. Devices
> expose versions, which is perfectly usable, thus changing compatible to
> different one is not useful. We could go with soc specific compatibles
> and new generic one, but what would that solve? This one is generic
> enough - the device is compatible with v2.0.

Well, I'd expect a "2.1.0", "2.0.0" fallback there..

[...]

>>> +		swr2: soundwire@7630000 {
>>> +			compatible = "qcom,soundwire-v2.0.0";
>>> +			reg = <0 0x07630000 0 0x10000>;
>>> +			interrupts = <GIC_SPI 761 IRQ_TYPE_LEVEL_HIGH>,
>>> +				     <GIC_SPI 785 IRQ_TYPE_LEVEL_HIGH>;
>>> +			interrupt-names = "core", "wakeup";
>>> +			clocks = <&lpass_txmacro>;
>>> +			clock-names = "iface";
>>> +			label = "TX";
>>> +
>>> +			pinctrl-0 = <&tx_swr_active>;
>>> +			pinctrl-names = "default";
>>> +
>>> +			qcom,din-ports = <4>;
>>> +			qcom,dout-ports = <0>;
>>
>> There's 1 OUT port on this instance
>>
>> otherwise (modulo the settings I don't have a reference for)
> 
> Omitted on purpose, no port configuration available.

alright

Konrad

Re: [PATCH 1/2] arm64: dts: qcom: sm8750: Add Soundwire nodes

[Krzysztof Kozlowski]

On 12/05/2025 21:38, Konrad Dybcio wrote:
>>>>  arch/arm64/boot/dts/qcom/sm8750.dtsi | 122 +++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 122 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
>>>> index 149d2ed17641a085d510f3a8eab5a96304787f0c..1e7aa25c675e76ce6aa571e04d7117b8c2ab25f8 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
>>>> @@ -2257,6 +2257,36 @@ lpass_wsa2macro: codec@6aa0000 {
>>>>  			#sound-dai-cells = <1>;
>>>>  		};
>>>>  
>>>> +		swr3: soundwire@6ab0000 {
>>>> +			compatible = "qcom,soundwire-v2.0.0";
>>>
>>> They're v2.1.0, same on 8650, there's a number of new registers
>>
>> Sorry, but no. This the "generic" compatible and it is correct. Devices
>> expose versions, which is perfectly usable, thus changing compatible to
>> different one is not useful. We could go with soc specific compatibles
>> and new generic one, but what would that solve? This one is generic
>> enough - the device is compatible with v2.0.
> 
> Well, I'd expect a "2.1.0", "2.0.0" fallback there..

OK, let's see if any DT maintainer will ack such thing. :)

Best regards,
Krzysztof

Re: [PATCH 1/2] arm64: dts: qcom: sm8750: Add Soundwire nodes

[Konrad Dybcio]

On 5/13/25 10:26 AM, Krzysztof Kozlowski wrote:
> On 12/05/2025 21:38, Konrad Dybcio wrote:
>>>>>  arch/arm64/boot/dts/qcom/sm8750.dtsi | 122 +++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 122 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
>>>>> index 149d2ed17641a085d510f3a8eab5a96304787f0c..1e7aa25c675e76ce6aa571e04d7117b8c2ab25f8 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
>>>>> @@ -2257,6 +2257,36 @@ lpass_wsa2macro: codec@6aa0000 {
>>>>>  			#sound-dai-cells = <1>;
>>>>>  		};
>>>>>  
>>>>> +		swr3: soundwire@6ab0000 {
>>>>> +			compatible = "qcom,soundwire-v2.0.0";
>>>>
>>>> They're v2.1.0, same on 8650, there's a number of new registers
>>>
>>> Sorry, but no. This the "generic" compatible and it is correct. Devices
>>> expose versions, which is perfectly usable, thus changing compatible to
>>> different one is not useful. We could go with soc specific compatibles
>>> and new generic one, but what would that solve? This one is generic
>>> enough - the device is compatible with v2.0.
>>
>> Well, I'd expect a "2.1.0", "2.0.0" fallback there..
> 
> OK, let's see if any DT maintainer will ack such thing. :)

They sure did in e.g.

83adc98ec0d8 ("dt-bindings: dma: Add support for SM6115 and QCM2290 SoCs")

Konrad

Re: [PATCH 1/2] arm64: dts: qcom: sm8750: Add Soundwire nodes

[Dmitry Baryshkov]

On Mon, May 12, 2025 at 09:38:01PM +0200, Konrad Dybcio wrote:
> On 5/8/25 12:37 PM, Krzysztof Kozlowski wrote:
> > On 25/04/2025 11:24, Konrad Dybcio wrote:
> >> On 4/24/25 11:40 AM, Krzysztof Kozlowski wrote:
> >>> Add Soundwire controllers on SM8750, fully compatible with earlier
> >>> SM8650 generation.
> >>>
> >>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >>> ---
> >>>  arch/arm64/boot/dts/qcom/sm8750.dtsi | 122 +++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 122 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
> >>> index 149d2ed17641a085d510f3a8eab5a96304787f0c..1e7aa25c675e76ce6aa571e04d7117b8c2ab25f8 100644
> >>> --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
> >>> +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
> >>> @@ -2257,6 +2257,36 @@ lpass_wsa2macro: codec@6aa0000 {
> >>>  			#sound-dai-cells = <1>;
> >>>  		};
> >>>  
> >>> +		swr3: soundwire@6ab0000 {
> >>> +			compatible = "qcom,soundwire-v2.0.0";
> >>
> >> They're v2.1.0, same on 8650, there's a number of new registers
> > 
> > Sorry, but no. This the "generic" compatible and it is correct. Devices
> > expose versions, which is perfectly usable, thus changing compatible to
> > different one is not useful. We could go with soc specific compatibles
> > and new generic one, but what would that solve? This one is generic
> > enough - the device is compatible with v2.0.
> 
> Well, I'd expect a "2.1.0", "2.0.0" fallback there..

+1. I think there should be a version-specific entry in addition to the
fallback.

-- 
With best wishes
Dmitry