[PATCH 0/6] arm64: dts: qcom: disable x13s sound + cleanups

[PATCH 0/6] arm64: dts: qcom: disable x13s sound + cleanups

[Johan Hovold]

Apparently enabling the soundcard in the X13s devicetree was a bit
premature so disable it for now until driver support is in place (e.g.
to avoid probe failures during boot).

This series also clean up the sc8280xp sound nodes somewhat, for
example, by making sure they are disabled by default and moving the
wcd938x node which do not belong under 'soc' to the root node. Two other
devicetrees had the wcd938x node under 'soc' and the fixes are also
included here.

Johan


Johan Hovold (6):
  arm64: dts: qcom: sc8280xp-x13s: disable soundcard
  arm64: dts: qcom: sc8280xp: disable sound nodes
  arm64: dts: qcom: sc8280xp: clean up tx-macro node
  arm64: dts: qcom: sc8280xp-x13s: fix wcd938x codec node
  arm64: dts: qcom: sm8250-mtp: fix wcd938x codec node
  arm64: dts: qcom: sm8450-hdk: fix wcd938x codec node

 .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts    | 62 +++++++++++--------
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi        | 18 +++++-
 arch/arm64/boot/dts/qcom/sm8250-mtp.dts       | 40 ++++++------
 arch/arm64/boot/dts/qcom/sm8450-hdk.dts       | 52 ++++++++--------
 4 files changed, 96 insertions(+), 76 deletions(-)

-- 
2.37.4

[PATCH 1/6] arm64: dts: qcom: sc8280xp-x13s: disable soundcard

[Johan Hovold]

Driver support for the X13s soundcard is not yet in place so disable it
for now to avoid probe failures such as:

[   11.077727] qcom-prm gprsvc:service:2:2: DSP returned error[100100f] 1
[   11.077926] rx_macro: probe of 3200000.rxmacro failed with error -22
[   21.221104] platform 3210000.soundwire-controller: deferred probe pending

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts  | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
index 0201c6776746..97ff74d5095e 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -649,6 +649,8 @@ wcd938x: codec {
 		qcom,mbhc-headphone-vthreshold-microvolt = <50000>;
 		qcom,rx-device = <&wcd_rx>;
 		qcom,tx-device = <&wcd_tx>;
+
+		status = "disabled";
 	};
 };
 
@@ -669,6 +671,8 @@ &sound {
 		"TX DMIC2", "MIC BIAS3",
 		"TX SWR_ADC1", "ADC2_OUTPUT";
 
+	status = "disabled";
+
 	wcd-playback-dai-link {
 		link-name = "WCD Playback";
 		cpu {
@@ -731,6 +735,8 @@ codec {
 };
 
 &swr0 {
+	status = "disabled";
+
 	left_spkr: wsa8830-left@0,1 {
 		compatible = "sdw10217020200";
 		reg = <0 1>;
@@ -757,7 +763,7 @@ right_spkr: wsa8830-right@0,2{
 };
 
 &swr1 {
-	status = "okay";
+	status = "disabled";
 
 	wcd_rx: wcd9380-rx@0,4 {
 		compatible = "sdw20217010d00";
@@ -767,7 +773,7 @@ wcd_rx: wcd9380-rx@0,4 {
 };
 
 &swr2 {
-	status = "okay";
+	status = "disabled";
 
 	wcd_tx: wcd9380-tx@0,3 {
 		compatible = "sdw20217010d00";
@@ -781,6 +787,8 @@ &vamacro {
 	pinctrl-names = "default";
 	vdd-micb-supply = <&vreg_s10b>;
 	qcom,dmic-sample-rate = <600000>;
+
+	status = "disabled";
 };
 
 &usb_0 {
-- 
2.37.4

[PATCH 2/6] arm64: dts: qcom: sc8280xp: disable sound nodes

[Johan Hovold]

The sound nodes in the SoC dtsi should be disabled by default.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index ed1e2bee86ee..c1ce2d7b3675 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -1733,6 +1733,8 @@ rxmacro: rxmacro@3200000 {
 
 			pinctrl-names = "default";
 			pinctrl-0 = <&rx_swr_default>;
+
+			status = "disabled";
 		};
 
 		/* RX */
@@ -1760,6 +1762,8 @@ swr1: soundwire-controller@3210000 {
 			#sound-dai-cells = <1>;
 			#address-cells = <2>;
 			#size-cells = <0>;
+
+			status = "disabled";
 		};
 
 		txmacro: txmacro@3220000 {
@@ -1783,6 +1787,8 @@ txmacro: txmacro@3220000 {
 			#address-cells = <2>;
 			#size-cells = <2>;
 			#sound-dai-cells = <1>;
+
+			status = "disabled";
 		};
 
 		wsamacro: codec@3240000 {
@@ -1804,6 +1810,8 @@ wsamacro: codec@3240000 {
 
 			pinctrl-names = "default";
 			pinctrl-0 = <&wsa_swr_default>;
+
+			status = "disabled";
 		};
 
 		/* WSA */
@@ -1830,6 +1838,8 @@ swr0: soundwire-controller@3250000 {
 			#sound-dai-cells = <1>;
 			#address-cells = <2>;
 			#size-cells = <0>;
+
+			status = "disabled";
 		};
 
 		/* TX */
@@ -1858,6 +1868,8 @@ swr2: soundwire-controller@3330000 {
 			qcom,ports-word-length =	/bits/ 8 <0xff 0x00 0xff 0xff>;
 			qcom,ports-block-group-count =	/bits/ 8 <0xff 0xff 0xff 0xff>;
 			qcom,ports-lane-control =	/bits/ 8 <0x00 0x01 0x00 0x00>;
+
+			status = "disabled";
 		};
 
 		vamacro: codec@3370000 {
@@ -1874,6 +1886,8 @@ vamacro: codec@3370000 {
 			#clock-cells = <0>;
 			clock-output-names = "fsgen";
 			#sound-dai-cells = <1>;
+
+			status = "disabled";
 		};
 
 		lpass_tlmm: pinctrl@33c0000 {
@@ -1888,6 +1902,8 @@ lpass_tlmm: pinctrl@33c0000 {
 				 <&q6prmcc LPASS_HW_DCODEC_VOTE LPASS_CLK_ATTRIBUTE_COUPLE_NO>;
 			clock-names = "core", "audio";
 
+			status = "disabled";
+
 			tx_swr_default: tx-swr-default-state {
 				clk-pins {
 					pins = "gpio0";
-- 
2.37.4

[PATCH 3/6] arm64: dts: qcom: sc8280xp: clean up tx-macro node

[Johan Hovold]

Drop the bogus address and size-cells properties from the tx-macro node,
which do not have any child nodes.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index c1ce2d7b3675..11e3d560c627 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -1784,8 +1784,6 @@ txmacro: txmacro@3220000 {
 			clock-output-names = "mclk";
 
 			#clock-cells = <0>;
-			#address-cells = <2>;
-			#size-cells = <2>;
 			#sound-dai-cells = <1>;
 
 			status = "disabled";
-- 
2.37.4

[PATCH 4/6] arm64: dts: qcom: sc8280xp-x13s: fix wcd938x codec node

[Johan Hovold]

The wcd938x codec is not a memory-mapped device and does not belong
under the soc node.

Move the node to the root node to avoid DT validation failures.

While at it, clean up the node somewhat by adding newline separators,
reordering properties and renaming it 'audio-codec'.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts    | 54 ++++++++++---------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
index 97ff74d5095e..ae67c11bfc22 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -21,6 +21,34 @@ / {
 	model = "Lenovo ThinkPad X13s";
 	compatible = "lenovo,thinkpad-x13s", "qcom,sc8280xp";
 
+	wcd938x: audio-codec {
+		compatible = "qcom,wcd9380-codec";
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&wcd_default>;
+
+		reset-gpios = <&tlmm 106 GPIO_ACTIVE_LOW>;
+
+		vdd-buck-supply = <&vreg_s10b>;
+		vdd-rxtx-supply = <&vreg_s10b>;
+		vdd-io-supply = <&vreg_s10b>;
+		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>;
+		qcom,rx-device = <&wcd_rx>;
+		qcom,tx-device = <&wcd_tx>;
+
+		#sound-dai-cells = <1>;
+
+		status = "disabled";
+	};
+
 	backlight {
 		compatible = "pwm-backlight";
 		pwms = <&pmc8280c_lpg 3 1000000>;
@@ -628,32 +656,6 @@ &remoteproc_nsp0 {
 	status = "okay";
 };
 
-&soc {
-	wcd938x: codec {
-		compatible = "qcom,wcd9380-codec";
-		pinctrl-names = "default";
-		pinctrl-0 = <&wcd_default>;
-		reset-gpios = <&tlmm 106 GPIO_ACTIVE_LOW>;
-		#sound-dai-cells = <1>;
-
-		vdd-buck-supply = <&vreg_s10b>;
-		vdd-rxtx-supply = <&vreg_s10b>;
-		vdd-io-supply = <&vreg_s10b>;
-		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>;
-		qcom,rx-device = <&wcd_rx>;
-		qcom,tx-device = <&wcd_tx>;
-
-		status = "disabled";
-	};
-};
-
 &sound {
 	compatible = "qcom,sc8280xp-sndcard";
 	model = "SC8280XP-LENOVO-X13S";
-- 
2.37.4

[PATCH 5/6] arm64: dts: qcom: sm8250-mtp: fix wcd938x codec node

[Johan Hovold]

The wcd938x codec is not a memory-mapped device and does not belong
under the soc node.

Move the node to the root node to avoid DT validation failures.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/arm64/boot/dts/qcom/sm8250-mtp.dts | 40 ++++++++++++-------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8250-mtp.dts b/arch/arm64/boot/dts/qcom/sm8250-mtp.dts
index 3ed8c84e25b8..ea43dbdc3671 100644
--- a/arch/arm64/boot/dts/qcom/sm8250-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sm8250-mtp.dts
@@ -23,6 +23,25 @@ aliases {
 		serial0 = &uart12;
 	};
 
+	wcd938x: codec {
+		compatible = "qcom,wcd9380-codec";
+		#sound-dai-cells = <1>;
+		reset-gpios = <&tlmm 32 GPIO_ACTIVE_LOW>;
+		vdd-buck-supply = <&vreg_s4a_1p8>;
+		vdd-rxtx-supply = <&vreg_s4a_1p8>;
+		vdd-io-supply = <&vreg_s4a_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>;
+		qcom,rx-device = <&wcd_rx>;
+		qcom,tx-device = <&wcd_tx>;
+	};
+
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
@@ -631,27 +650,6 @@ &slpi {
 	firmware-name = "qcom/sm8250/slpi.mbn";
 };
 
-&soc {
-	wcd938x: codec {
-		compatible = "qcom,wcd9380-codec";
-		#sound-dai-cells = <1>;
-		reset-gpios = <&tlmm 32 GPIO_ACTIVE_LOW>;
-		vdd-buck-supply = <&vreg_s4a_1p8>;
-		vdd-rxtx-supply = <&vreg_s4a_1p8>;
-		vdd-io-supply = <&vreg_s4a_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>;
-		qcom,rx-device = <&wcd_rx>;
-		qcom,tx-device = <&wcd_tx>;
-	};
-};
-
 &sound {
 	compatible = "qcom,sm8250-sndcard";
 	model = "SM8250-MTP-WCD9380-WSA8810-VA-DMIC";
-- 
2.37.4

[PATCH 6/6] arm64: dts: qcom: sm8450-hdk: fix wcd938x codec node

[Johan Hovold]

The wcd938x codec is not a memory-mapped device and does not belong
under the soc node.

Move the node to the root node to avoid DT validation failures.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 52 ++++++++++++-------------
 1 file changed, 25 insertions(+), 27 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
index 4de3e1f1c39c..217b2c654745 100644
--- a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
+++ b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
@@ -17,6 +17,31 @@ aliases {
 		serial0 = &uart7;
 	};
 
+	wcd938x: codec {
+		compatible = "qcom,wcd9380-codec";
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&wcd_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 43 GPIO_ACTIVE_LOW>;
+		#sound-dai-cells = <1>;
+
+		vdd-buck-supply = <&vreg_s10b_1p8>;
+		vdd-rxtx-supply = <&vreg_s10b_1p8>;
+		vdd-io-supply = <&vreg_s10b_1p8>;
+		vdd-mic-bias-supply = <&vreg_bob>;
+	};
+
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
@@ -407,33 +432,6 @@ &sdhc_2 {
 	status = "okay";
 };
 
-&soc {
-	wcd938x: codec {
-		compatible = "qcom,wcd9380-codec";
-
-		pinctrl-names = "default";
-		pinctrl-0 = <&wcd_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 43 GPIO_ACTIVE_LOW>;
-		#sound-dai-cells = <1>;
-
-		vdd-buck-supply = <&vreg_s10b_1p8>;
-		vdd-rxtx-supply = <&vreg_s10b_1p8>;
-		vdd-io-supply = <&vreg_s10b_1p8>;
-		vdd-mic-bias-supply = <&vreg_bob>;
-	};
-};
-
 &sound {
 	compatible = "qcom,sm8450-sndcard";
 	model = "SM8450-HDK";
-- 
2.37.4

Re: [PATCH 2/6] arm64: dts: qcom: sc8280xp: disable sound nodes

[Konrad Dybcio]

On 2.01.2023 11:50, Johan Hovold wrote:
> The sound nodes in the SoC dtsi should be disabled by default.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index ed1e2bee86ee..c1ce2d7b3675 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -1733,6 +1733,8 @@ rxmacro: rxmacro@3200000 {
>  
>  			pinctrl-names = "default";
>  			pinctrl-0 = <&rx_swr_default>;
> +
> +			status = "disabled";
>  		};
>  
>  		/* RX */
> @@ -1760,6 +1762,8 @@ swr1: soundwire-controller@3210000 {
>  			#sound-dai-cells = <1>;
>  			#address-cells = <2>;
>  			#size-cells = <0>;
> +
> +			status = "disabled";
>  		};
>  
>  		txmacro: txmacro@3220000 {
> @@ -1783,6 +1787,8 @@ txmacro: txmacro@3220000 {
>  			#address-cells = <2>;
>  			#size-cells = <2>;
>  			#sound-dai-cells = <1>;
> +
> +			status = "disabled";
>  		};
>  
>  		wsamacro: codec@3240000 {
> @@ -1804,6 +1810,8 @@ wsamacro: codec@3240000 {
>  
>  			pinctrl-names = "default";
>  			pinctrl-0 = <&wsa_swr_default>;
> +
> +			status = "disabled";
>  		};
>  
>  		/* WSA */
> @@ -1830,6 +1838,8 @@ swr0: soundwire-controller@3250000 {
>  			#sound-dai-cells = <1>;
>  			#address-cells = <2>;
>  			#size-cells = <0>;
> +
> +			status = "disabled";
>  		};
>  
>  		/* TX */
> @@ -1858,6 +1868,8 @@ swr2: soundwire-controller@3330000 {
>  			qcom,ports-word-length =	/bits/ 8 <0xff 0x00 0xff 0xff>;
>  			qcom,ports-block-group-count =	/bits/ 8 <0xff 0xff 0xff 0xff>;
>  			qcom,ports-lane-control =	/bits/ 8 <0x00 0x01 0x00 0x00>;
> +
> +			status = "disabled";
>  		};
>  
>  		vamacro: codec@3370000 {
> @@ -1874,6 +1886,8 @@ vamacro: codec@3370000 {
>  			#clock-cells = <0>;
>  			clock-output-names = "fsgen";
>  			#sound-dai-cells = <1>;
> +
> +			status = "disabled";
>  		};
>  
>  		lpass_tlmm: pinctrl@33c0000 {
> @@ -1888,6 +1902,8 @@ lpass_tlmm: pinctrl@33c0000 {
>  				 <&q6prmcc LPASS_HW_DCODEC_VOTE LPASS_CLK_ATTRIBUTE_COUPLE_NO>;
>  			clock-names = "core", "audio";
>  
> +			status = "disabled";
> +
>  			tx_swr_default: tx-swr-default-state {
>  				clk-pins {
>  					pins = "gpio0";

Re: [PATCH 3/6] arm64: dts: qcom: sc8280xp: clean up tx-macro node

[Konrad Dybcio]

On 2.01.2023 11:50, Johan Hovold wrote:
> Drop the bogus address and size-cells properties from the tx-macro node,
> which do not have any child nodes.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index c1ce2d7b3675..11e3d560c627 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -1784,8 +1784,6 @@ txmacro: txmacro@3220000 {
>  			clock-output-names = "mclk";
>  
>  			#clock-cells = <0>;
> -			#address-cells = <2>;
> -			#size-cells = <2>;
>  			#sound-dai-cells = <1>;
>  
>  			status = "disabled";

Re: [PATCH 4/6] arm64: dts: qcom: sc8280xp-x13s: fix wcd938x codec node

[Konrad Dybcio]

On 2.01.2023 11:50, Johan Hovold wrote:
> The wcd938x codec is not a memory-mapped device and does not belong
> under the soc node.
> 
> Move the node to the root node to avoid DT validation failures.
> 
> While at it, clean up the node somewhat by adding newline separators,
> reordering properties and renaming it 'audio-codec'.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts    | 54 ++++++++++---------
>  1 file changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> index 97ff74d5095e..ae67c11bfc22 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> @@ -21,6 +21,34 @@ / {
>  	model = "Lenovo ThinkPad X13s";
>  	compatible = "lenovo,thinkpad-x13s", "qcom,sc8280xp";
>  
> +	wcd938x: audio-codec {
> +		compatible = "qcom,wcd9380-codec";
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&wcd_default>;
> +
> +		reset-gpios = <&tlmm 106 GPIO_ACTIVE_LOW>;
> +
> +		vdd-buck-supply = <&vreg_s10b>;
> +		vdd-rxtx-supply = <&vreg_s10b>;
> +		vdd-io-supply = <&vreg_s10b>;
> +		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>;
> +		qcom,rx-device = <&wcd_rx>;
> +		qcom,tx-device = <&wcd_tx>;
> +
> +		#sound-dai-cells = <1>;
> +
> +		status = "disabled";
> +	};
> +
>  	backlight {
>  		compatible = "pwm-backlight";
>  		pwms = <&pmc8280c_lpg 3 1000000>;
> @@ -628,32 +656,6 @@ &remoteproc_nsp0 {
>  	status = "okay";
>  };
>  
> -&soc {
> -	wcd938x: codec {
> -		compatible = "qcom,wcd9380-codec";
> -		pinctrl-names = "default";
> -		pinctrl-0 = <&wcd_default>;
> -		reset-gpios = <&tlmm 106 GPIO_ACTIVE_LOW>;
> -		#sound-dai-cells = <1>;
> -
> -		vdd-buck-supply = <&vreg_s10b>;
> -		vdd-rxtx-supply = <&vreg_s10b>;
> -		vdd-io-supply = <&vreg_s10b>;
> -		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>;
> -		qcom,rx-device = <&wcd_rx>;
> -		qcom,tx-device = <&wcd_tx>;
> -
> -		status = "disabled";
> -	};
> -};
> -
>  &sound {
>  	compatible = "qcom,sc8280xp-sndcard";
>  	model = "SC8280XP-LENOVO-X13S";

Re: [PATCH 5/6] arm64: dts: qcom: sm8250-mtp: fix wcd938x codec node

[Konrad Dybcio]

On 2.01.2023 11:50, Johan Hovold wrote:
> The wcd938x codec is not a memory-mapped device and does not belong
> under the soc node.
> 
> Move the node to the root node to avoid DT validation failures.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
Could you rename it to audio-codec and separate/sort the properties
like you did for x13s?

Konrad

>  arch/arm64/boot/dts/qcom/sm8250-mtp.dts | 40 ++++++++++++-------------
>  1 file changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8250-mtp.dts b/arch/arm64/boot/dts/qcom/sm8250-mtp.dts
> index 3ed8c84e25b8..ea43dbdc3671 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sm8250-mtp.dts
> @@ -23,6 +23,25 @@ aliases {
>  		serial0 = &uart12;
>  	};
>  
> +	wcd938x: codec {
> +		compatible = "qcom,wcd9380-codec";
> +		#sound-dai-cells = <1>;
> +		reset-gpios = <&tlmm 32 GPIO_ACTIVE_LOW>;
> +		vdd-buck-supply = <&vreg_s4a_1p8>;
> +		vdd-rxtx-supply = <&vreg_s4a_1p8>;
> +		vdd-io-supply = <&vreg_s4a_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>;
> +		qcom,rx-device = <&wcd_rx>;
> +		qcom,tx-device = <&wcd_tx>;
> +	};
> +
>  	chosen {
>  		stdout-path = "serial0:115200n8";
>  	};
> @@ -631,27 +650,6 @@ &slpi {
>  	firmware-name = "qcom/sm8250/slpi.mbn";
>  };
>  
> -&soc {
> -	wcd938x: codec {
> -		compatible = "qcom,wcd9380-codec";
> -		#sound-dai-cells = <1>;
> -		reset-gpios = <&tlmm 32 GPIO_ACTIVE_LOW>;
> -		vdd-buck-supply = <&vreg_s4a_1p8>;
> -		vdd-rxtx-supply = <&vreg_s4a_1p8>;
> -		vdd-io-supply = <&vreg_s4a_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>;
> -		qcom,rx-device = <&wcd_rx>;
> -		qcom,tx-device = <&wcd_tx>;
> -	};
> -};
> -
>  &sound {
>  	compatible = "qcom,sm8250-sndcard";
>  	model = "SM8250-MTP-WCD9380-WSA8810-VA-DMIC";

Re: [PATCH 6/6] arm64: dts: qcom: sm8450-hdk: fix wcd938x codec node

[Konrad Dybcio]

On 2.01.2023 11:50, Johan Hovold wrote:
> The wcd938x codec is not a memory-mapped device and does not belong
> under the soc node.
> 
> Move the node to the root node to avoid DT validation failures.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
Third patch concerning the audio codec and third different
order of properties :/ Please consolidate that.

Konrad
>  arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 52 ++++++++++++-------------
>  1 file changed, 25 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
> index 4de3e1f1c39c..217b2c654745 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
> +++ b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
> @@ -17,6 +17,31 @@ aliases {
>  		serial0 = &uart7;
>  	};
>  
> +	wcd938x: codec {
> +		compatible = "qcom,wcd9380-codec";
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&wcd_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 43 GPIO_ACTIVE_LOW>;
> +		#sound-dai-cells = <1>;
> +
> +		vdd-buck-supply = <&vreg_s10b_1p8>;
> +		vdd-rxtx-supply = <&vreg_s10b_1p8>;
> +		vdd-io-supply = <&vreg_s10b_1p8>;
> +		vdd-mic-bias-supply = <&vreg_bob>;
> +	};
> +
>  	chosen {
>  		stdout-path = "serial0:115200n8";
>  	};
> @@ -407,33 +432,6 @@ &sdhc_2 {
>  	status = "okay";
>  };
>  
> -&soc {
> -	wcd938x: codec {
> -		compatible = "qcom,wcd9380-codec";
> -
> -		pinctrl-names = "default";
> -		pinctrl-0 = <&wcd_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 43 GPIO_ACTIVE_LOW>;
> -		#sound-dai-cells = <1>;
> -
> -		vdd-buck-supply = <&vreg_s10b_1p8>;
> -		vdd-rxtx-supply = <&vreg_s10b_1p8>;
> -		vdd-io-supply = <&vreg_s10b_1p8>;
> -		vdd-mic-bias-supply = <&vreg_bob>;
> -	};
> -};
> -
>  &sound {
>  	compatible = "qcom,sm8450-sndcard";
>  	model = "SM8450-HDK";

Re: [PATCH 5/6] arm64: dts: qcom: sm8250-mtp: fix wcd938x codec node

[Johan Hovold]

On Mon, Jan 02, 2023 at 12:15:50PM +0100, Konrad Dybcio wrote:
> 
> 
> On 2.01.2023 11:50, Johan Hovold wrote:
> > The wcd938x codec is not a memory-mapped device and does not belong
> > under the soc node.
> > 
> > Move the node to the root node to avoid DT validation failures.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> Could you rename it to audio-codec and separate/sort the properties
> like you did for x13s?

Possibly, but unlike for sc8280xp, I don't really care about these
platforms and how their DT authors have chosen to order properties so I
left out the clean up bits on purpose.

Johan

Re: [PATCH 6/6] arm64: dts: qcom: sm8450-hdk: fix wcd938x codec node

[Johan Hovold]

On Mon, Jan 02, 2023 at 12:16:44PM +0100, Konrad Dybcio wrote:
> 
> 
> On 2.01.2023 11:50, Johan Hovold wrote:
> > The wcd938x codec is not a memory-mapped device and does not belong
> > under the soc node.
> > 
> > Move the node to the root node to avoid DT validation failures.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> Third patch concerning the audio codec and third different
> order of properties :/ Please consolidate that.

It's arguably a separate change (even if I included it for the disabled
x13s node). I'm fixing the fact that the node should not have been added
under 'soc' in the first place.

Reordering properties can be done as a follow up if anyone cares enough,
even if I could possibly have renamed the node when moving it.

Johan

Re: [PATCH 5/6] arm64: dts: qcom: sm8250-mtp: fix wcd938x codec node

[Krzysztof Kozlowski]

On 02/01/2023 11:50, Johan Hovold wrote:
> The wcd938x codec is not a memory-mapped device and does not belong
> under the soc node.
> 
> Move the node to the root node to avoid DT validation failures.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8250-mtp.dts | 40 ++++++++++++-------------
>  1 file changed, 19 insertions(+), 21 deletions(-)

Don't you base on some older tree? It was alreadt moved. Maybe other
changes were also implemented...

Best regards,
Krzysztof

Re: [PATCH 6/6] arm64: dts: qcom: sm8450-hdk: fix wcd938x codec node

[Krzysztof Kozlowski]

On 02/01/2023 11:50, Johan Hovold wrote:
> The wcd938x codec is not a memory-mapped device and does not belong
> under the soc node.
> 
> Move the node to the root node to avoid DT validation failures.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 52 ++++++++++++-------------
>  1 file changed, 25 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
> index 4de3e1f1c39c..217b2c654745 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
> +++ b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
> @@ -17,6 +17,31 @@ aliases {
>  		serial0 = &uart7;
>  	};
>  
> +	wcd938x: codec {

Let's call it audio-codec and put it somewhere ordered alphabetically.
This avoids useless reshuffles immediately. Moving things back and forth
does not make sense.

Best regards,
Krzysztof

Re: [PATCH 6/6] arm64: dts: qcom: sm8450-hdk: fix wcd938x codec node

[Krzysztof Kozlowski]

On 02/01/2023 13:23, Krzysztof Kozlowski wrote:
> On 02/01/2023 11:50, Johan Hovold wrote:
>> The wcd938x codec is not a memory-mapped device and does not belong
>> under the soc node.
>>
>> Move the node to the root node to avoid DT validation failures.
>>
>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>> ---
>>  arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 52 ++++++++++++-------------
>>  1 file changed, 25 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
>> index 4de3e1f1c39c..217b2c654745 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
>> +++ b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
>> @@ -17,6 +17,31 @@ aliases {
>>  		serial0 = &uart7;
>>  	};
>>  
>> +	wcd938x: codec {
> 
> Let's call it audio-codec and put it somewhere ordered alphabetically.
> This avoids useless reshuffles immediately. Moving things back and forth
> does not make sense.

Eh, on the other hand SM8250-MTP uses name codec so maybe the rename in
SC8280xp should be dropped.

Best regards,
Krzysztof

Re: [PATCH 1/6] arm64: dts: qcom: sc8280xp-x13s: disable soundcard

[Krzysztof Kozlowski]

On 02/01/2023 11:50, Johan Hovold wrote:
> Driver support for the X13s soundcard is not yet in place so disable it
> for now to avoid probe failures such as:
> 
> [   11.077727] qcom-prm gprsvc:service:2:2: DSP returned error[100100f] 1
> [   11.077926] rx_macro: probe of 3200000.rxmacro failed with error -22
> [   21.221104] platform 3210000.soundwire-controller: deferred probe pending
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts  | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> index 0201c6776746..97ff74d5095e 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> @@ -649,6 +649,8 @@ wcd938x: codec {
>  		qcom,mbhc-headphone-vthreshold-microvolt = <50000>;
>  		qcom,rx-device = <&wcd_rx>;
>  		qcom,tx-device = <&wcd_tx>;
> +
> +		status = "disabled";
>  	};
>  };
>  
> @@ -669,6 +671,8 @@ &sound {
>  		"TX DMIC2", "MIC BIAS3",
>  		"TX SWR_ADC1", "ADC2_OUTPUT";
>  
> +	status = "disabled";
> +
>  	wcd-playback-dai-link {
>  		link-name = "WCD Playback";
>  		cpu {
> @@ -731,6 +735,8 @@ codec {
>  };
>  
>  &swr0 {
> +	status = "disabled";
> +
>  	left_spkr: wsa8830-left@0,1 {
>  		compatible = "sdw10217020200";
>  		reg = <0 1>;
> @@ -757,7 +763,7 @@ right_spkr: wsa8830-right@0,2{
>  };
>  
>  &swr1 {
> -	status = "okay";
> +	status = "disabled";
>  
>  	wcd_rx: wcd9380-rx@0,4 {
>  		compatible = "sdw20217010d00";
> @@ -767,7 +773,7 @@ wcd_rx: wcd9380-rx@0,4 {
>  };
>  
>  &swr2 {
> -	status = "okay";
> +	status = "disabled";

That's a double disable.

>  
>  	wcd_tx: wcd9380-tx@0,3 {
>  		compatible = "sdw20217010d00";
> @@ -781,6 +787,8 @@ &vamacro {
>  	pinctrl-names = "default";
>  	vdd-micb-supply = <&vreg_s10b>;
>  	qcom,dmic-sample-rate = <600000>;
> +
> +	status = "disabled";

That's a double disable.

Best regards,
Krzysztof

Re: [PATCH 2/6] arm64: dts: qcom: sc8280xp: disable sound nodes

[Krzysztof Kozlowski]

On 02/01/2023 11:50, Johan Hovold wrote:
> The sound nodes in the SoC dtsi should be disabled by default.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index ed1e2bee86ee..c1ce2d7b3675 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -1733,6 +1733,8 @@ rxmacro: rxmacro@3200000 {
>  
>  			pinctrl-names = "default";
>  			pinctrl-0 = <&rx_swr_default>;
> +
> +			status = "disabled";

There is no reason this should be disabled by default. No external (per
board) configuration is needed and by default SoC components should be
enabled if they do not need anything from the board.

>  		};
>  
>  		/* RX */
> @@ -1760,6 +1762,8 @@ swr1: soundwire-controller@3210000 {
>  			#sound-dai-cells = <1>;
>  			#address-cells = <2>;
>  			#size-cells = <0>;
> +
> +			status = "disabled";

For soundwires disabling makes sense - these are busses so they need to
be explicitly enabled and populated with children.

>  		};
>  
>  		txmacro: txmacro@3220000 {
> @@ -1783,6 +1787,8 @@ txmacro: txmacro@3220000 {
>  			#address-cells = <2>;
>  			#size-cells = <2>;
>  			#sound-dai-cells = <1>;
> +
> +			status = "disabled";
>  		};
>  
>  		wsamacro: codec@3240000 {
> @@ -1804,6 +1810,8 @@ wsamacro: codec@3240000 {
>  
>  			pinctrl-names = "default";
>  			pinctrl-0 = <&wsa_swr_default>;
> +
> +			status = "disabled";
>  		};
>  
>  		/* WSA */
> @@ -1830,6 +1838,8 @@ swr0: soundwire-controller@3250000 {
>  			#sound-dai-cells = <1>;
>  			#address-cells = <2>;
>  			#size-cells = <0>;
> +
> +			status = "disabled";
>  		};
>  
>  		/* TX */
> @@ -1858,6 +1868,8 @@ swr2: soundwire-controller@3330000 {
>  			qcom,ports-word-length =	/bits/ 8 <0xff 0x00 0xff 0xff>;
>  			qcom,ports-block-group-count =	/bits/ 8 <0xff 0xff 0xff 0xff>;
>  			qcom,ports-lane-control =	/bits/ 8 <0x00 0x01 0x00 0x00>;
> +
> +			status = "disabled";
>  		};
>  
>  		vamacro: codec@3370000 {
> @@ -1874,6 +1886,8 @@ vamacro: codec@3370000 {
>  			#clock-cells = <0>;
>  			clock-output-names = "fsgen";
>  			#sound-dai-cells = <1>;
> +
> +			status = "disabled";

vamacro needs supply from the board so this one also makes sense.

Best regards,
Krzysztof

Re: [PATCH 5/6] arm64: dts: qcom: sm8250-mtp: fix wcd938x codec node

[Johan Hovold]

On Mon, Jan 02, 2023 at 01:20:03PM +0100, Krzysztof Kozlowski wrote:
> On 02/01/2023 11:50, Johan Hovold wrote:
> > The wcd938x codec is not a memory-mapped device and does not belong
> > under the soc node.
> > 
> > Move the node to the root node to avoid DT validation failures.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sm8250-mtp.dts | 40 ++++++++++++-------------
> >  1 file changed, 19 insertions(+), 21 deletions(-)
> 
> Don't you base on some older tree? It was alreadt moved. Maybe other
> changes were also implemented...

If you've moved this node is hasn't made it into linux-next (or the qcom
tree) yet.

Johan

Re: [PATCH 6/6] arm64: dts: qcom: sm8450-hdk: fix wcd938x codec node

[Johan Hovold]

On Mon, Jan 02, 2023 at 01:24:34PM +0100, Krzysztof Kozlowski wrote:
> On 02/01/2023 13:23, Krzysztof Kozlowski wrote:
> > On 02/01/2023 11:50, Johan Hovold wrote:
> >> The wcd938x codec is not a memory-mapped device and does not belong
> >> under the soc node.
> >>
> >> Move the node to the root node to avoid DT validation failures.
> >>
> >> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> >> ---
> >>  arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 52 ++++++++++++-------------
> >>  1 file changed, 25 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
> >> index 4de3e1f1c39c..217b2c654745 100644
> >> --- a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
> >> +++ b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
> >> @@ -17,6 +17,31 @@ aliases {
> >>  		serial0 = &uart7;
> >>  	};
> >>  
> >> +	wcd938x: codec {
> > 
> > Let's call it audio-codec and put it somewhere ordered alphabetically.
> > This avoids useless reshuffles immediately. Moving things back and forth
> > does not make sense.
> 
> Eh, on the other hand SM8250-MTP uses name codec so maybe the rename in
> SC8280xp should be dropped.

Nah, let's go with audio-codec. Your MTP change hasn't been merged yet
so we can still drop or respin that one.

Johan

Re: [PATCH 1/6] arm64: dts: qcom: sc8280xp-x13s: disable soundcard

[Johan Hovold]

On Mon, Jan 02, 2023 at 01:25:38PM +0100, Krzysztof Kozlowski wrote:
> On 02/01/2023 11:50, Johan Hovold wrote:
> > Driver support for the X13s soundcard is not yet in place so disable it
> > for now to avoid probe failures such as:
> > 
> > [   11.077727] qcom-prm gprsvc:service:2:2: DSP returned error[100100f] 1
> > [   11.077926] rx_macro: probe of 3200000.rxmacro failed with error -22
> > [   21.221104] platform 3210000.soundwire-controller: deferred probe pending
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts  | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> > index 0201c6776746..97ff74d5095e 100644
> > --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> > +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> > @@ -649,6 +649,8 @@ wcd938x: codec {
> >  		qcom,mbhc-headphone-vthreshold-microvolt = <50000>;
> >  		qcom,rx-device = <&wcd_rx>;
> >  		qcom,tx-device = <&wcd_tx>;
> > +
> > +		status = "disabled";
> >  	};
> >  };
> >  
> > @@ -669,6 +671,8 @@ &sound {
> >  		"TX DMIC2", "MIC BIAS3",
> >  		"TX SWR_ADC1", "ADC2_OUTPUT";
> >  
> > +	status = "disabled";
> > +
> >  	wcd-playback-dai-link {
> >  		link-name = "WCD Playback";
> >  		cpu {
> > @@ -731,6 +735,8 @@ codec {
> >  };
> >  
> >  &swr0 {
> > +	status = "disabled";
> > +
> >  	left_spkr: wsa8830-left@0,1 {
> >  		compatible = "sdw10217020200";
> >  		reg = <0 1>;
> > @@ -757,7 +763,7 @@ right_spkr: wsa8830-right@0,2{
> >  };
> >  
> >  &swr1 {
> > -	status = "okay";
> > +	status = "disabled";
> >  
> >  	wcd_rx: wcd9380-rx@0,4 {
> >  		compatible = "sdw20217010d00";
> > @@ -767,7 +773,7 @@ wcd_rx: wcd9380-rx@0,4 {
> >  };
> >  
> >  &swr2 {
> > -	status = "okay";
> > +	status = "disabled";
> 
> That's a double disable.
> 
> >  
> >  	wcd_tx: wcd9380-tx@0,3 {
> >  		compatible = "sdw20217010d00";
> > @@ -781,6 +787,8 @@ &vamacro {
> >  	pinctrl-names = "default";
> >  	vdd-micb-supply = <&vreg_s10b>;
> >  	qcom,dmic-sample-rate = <600000>;
> > +
> > +	status = "disabled";
> 
> That's a double disable.

Yes, that's on purpose. We're temporarily disabling these nodes instead
of reverting the series which should not have been merged.

Once we have driver support, these properties will be updated again.

Johan

Re: [PATCH 5/6] arm64: dts: qcom: sm8250-mtp: fix wcd938x codec node

[Krzysztof Kozlowski]

On 02/01/2023 16:03, Johan Hovold wrote:
> On Mon, Jan 02, 2023 at 01:20:03PM +0100, Krzysztof Kozlowski wrote:
>> On 02/01/2023 11:50, Johan Hovold wrote:
>>> The wcd938x codec is not a memory-mapped device and does not belong
>>> under the soc node.
>>>
>>> Move the node to the root node to avoid DT validation failures.
>>>
>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>>> ---
>>>  arch/arm64/boot/dts/qcom/sm8250-mtp.dts | 40 ++++++++++++-------------
>>>  1 file changed, 19 insertions(+), 21 deletions(-)
>>
>> Don't you base on some older tree? It was alreadt moved. Maybe other
>> changes were also implemented...
> 
> If you've moved this node is hasn't made it into linux-next (or the qcom
> tree) yet.

So you need to update your tree/rebase as this is already in Qualcomm
tree. There was no linux-next release since a week, so that's not a
solution.

Best regards,
Krzysztof

Re: [PATCH 6/6] arm64: dts: qcom: sm8450-hdk: fix wcd938x codec node

[Krzysztof Kozlowski]

On 02/01/2023 16:05, Johan Hovold wrote:
> On Mon, Jan 02, 2023 at 01:24:34PM +0100, Krzysztof Kozlowski wrote:
>> On 02/01/2023 13:23, Krzysztof Kozlowski wrote:
>>> On 02/01/2023 11:50, Johan Hovold wrote:
>>>> The wcd938x codec is not a memory-mapped device and does not belong
>>>> under the soc node.
>>>>
>>>> Move the node to the root node to avoid DT validation failures.
>>>>
>>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>>>> ---
>>>>  arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 52 ++++++++++++-------------
>>>>  1 file changed, 25 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
>>>> index 4de3e1f1c39c..217b2c654745 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
>>>> +++ b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
>>>> @@ -17,6 +17,31 @@ aliases {
>>>>  		serial0 = &uart7;
>>>>  	};
>>>>  
>>>> +	wcd938x: codec {
>>>
>>> Let's call it audio-codec and put it somewhere ordered alphabetically.
>>> This avoids useless reshuffles immediately. Moving things back and forth
>>> does not make sense.
>>
>> Eh, on the other hand SM8250-MTP uses name codec so maybe the rename in
>> SC8280xp should be dropped.
> 
> Nah, let's go with audio-codec. Your MTP change hasn't been merged yet
> so we can still drop or respin that one.

It was, ~5 days ago, so please rebase.

Best regards,
Krzysztof

Re: [PATCH 1/6] arm64: dts: qcom: sc8280xp-x13s: disable soundcard

[Krzysztof Kozlowski]

On 02/01/2023 16:07, Johan Hovold wrote:
> On Mon, Jan 02, 2023 at 01:25:38PM +0100, Krzysztof Kozlowski wrote:
>> On 02/01/2023 11:50, Johan Hovold wrote:
>>> Driver support for the X13s soundcard is not yet in place so disable it
>>> for now to avoid probe failures such as:
>>>
>>> [   11.077727] qcom-prm gprsvc:service:2:2: DSP returned error[100100f] 1
>>> [   11.077926] rx_macro: probe of 3200000.rxmacro failed with error -22
>>> [   21.221104] platform 3210000.soundwire-controller: deferred probe pending
>>>
>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>>> ---
>>>  .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts  | 12 ++++++++++--
>>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
>>> index 0201c6776746..97ff74d5095e 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
>>> @@ -649,6 +649,8 @@ wcd938x: codec {
>>>  		qcom,mbhc-headphone-vthreshold-microvolt = <50000>;
>>>  		qcom,rx-device = <&wcd_rx>;
>>>  		qcom,tx-device = <&wcd_tx>;
>>> +
>>> +		status = "disabled";
>>>  	};
>>>  };
>>>  
>>> @@ -669,6 +671,8 @@ &sound {
>>>  		"TX DMIC2", "MIC BIAS3",
>>>  		"TX SWR_ADC1", "ADC2_OUTPUT";
>>>  
>>> +	status = "disabled";
>>> +
>>>  	wcd-playback-dai-link {
>>>  		link-name = "WCD Playback";
>>>  		cpu {
>>> @@ -731,6 +735,8 @@ codec {
>>>  };
>>>  
>>>  &swr0 {
>>> +	status = "disabled";
>>> +
>>>  	left_spkr: wsa8830-left@0,1 {
>>>  		compatible = "sdw10217020200";
>>>  		reg = <0 1>;
>>> @@ -757,7 +763,7 @@ right_spkr: wsa8830-right@0,2{
>>>  };
>>>  
>>>  &swr1 {
>>> -	status = "okay";
>>> +	status = "disabled";
>>>  
>>>  	wcd_rx: wcd9380-rx@0,4 {
>>>  		compatible = "sdw20217010d00";
>>> @@ -767,7 +773,7 @@ wcd_rx: wcd9380-rx@0,4 {
>>>  };
>>>  
>>>  &swr2 {
>>> -	status = "okay";
>>> +	status = "disabled";
>>
>> That's a double disable.
>>
>>>  
>>>  	wcd_tx: wcd9380-tx@0,3 {
>>>  		compatible = "sdw20217010d00";
>>> @@ -781,6 +787,8 @@ &vamacro {
>>>  	pinctrl-names = "default";
>>>  	vdd-micb-supply = <&vreg_s10b>;
>>>  	qcom,dmic-sample-rate = <600000>;
>>> +
>>> +	status = "disabled";
>>
>> That's a double disable.
> 
> Yes, that's on purpose. We're temporarily disabling these nodes instead
> of reverting the series which should not have been merged.

I don't get why disabling something twice is anyhow related to
"temporarily disable". One disable is enough for temporary or permanent
disables.

> 
> Once we have driver support, these properties will be updated again.

Linux kernel is not the only consumer of DTS, thus having or not having
the support in the kernel is not reason to disable pieces of it.
Assuming the DTS is correct, of course, because maybe that's the problem?

Best regards,
Krzysztof

Re: [PATCH 2/6] arm64: dts: qcom: sc8280xp: disable sound nodes

[Johan Hovold]

On Mon, Jan 02, 2023 at 01:29:38PM +0100, Krzysztof Kozlowski wrote:
> On 02/01/2023 11:50, Johan Hovold wrote:
> > The sound nodes in the SoC dtsi should be disabled by default.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > index ed1e2bee86ee..c1ce2d7b3675 100644
> > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > @@ -1733,6 +1733,8 @@ rxmacro: rxmacro@3200000 {
> >  
> >  			pinctrl-names = "default";
> >  			pinctrl-0 = <&rx_swr_default>;
> > +
> > +			status = "disabled";
> 
> There is no reason this should be disabled by default. No external (per
> board) configuration is needed and by default SoC components should be
> enabled if they do not need anything from the board.

This node is one of the nodes for which driver support is not yet in
place so that's one reason for at least disabling it temporarily.

Since all other pinconfig lives in the board dts, if we decide to move
also the sound pinconfig then then that may be a second.

Johan

Re: [PATCH 5/6] arm64: dts: qcom: sm8250-mtp: fix wcd938x codec node

[Johan Hovold]

On Mon, Jan 02, 2023 at 04:09:14PM +0100, Krzysztof Kozlowski wrote:
> On 02/01/2023 16:03, Johan Hovold wrote:
> > On Mon, Jan 02, 2023 at 01:20:03PM +0100, Krzysztof Kozlowski wrote:
> >> On 02/01/2023 11:50, Johan Hovold wrote:
> >>> The wcd938x codec is not a memory-mapped device and does not belong
> >>> under the soc node.
> >>>
> >>> Move the node to the root node to avoid DT validation failures.
> >>>
> >>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> >>> ---
> >>>  arch/arm64/boot/dts/qcom/sm8250-mtp.dts | 40 ++++++++++++-------------
> >>>  1 file changed, 19 insertions(+), 21 deletions(-)
> >>
> >> Don't you base on some older tree? It was alreadt moved. Maybe other
> >> changes were also implemented...
> > 
> > If you've moved this node is hasn't made it into linux-next (or the qcom
> > tree) yet.
> 
> So you need to update your tree/rebase as this is already in Qualcomm
> tree. There was no linux-next release since a week, so that's not a
> solution.

As I wrote above, it's not in the qcom tree either.

Johan

Re: [PATCH 6/6] arm64: dts: qcom: sm8450-hdk: fix wcd938x codec node

[Johan Hovold]

On Mon, Jan 02, 2023 at 04:09:59PM +0100, Krzysztof Kozlowski wrote:
> On 02/01/2023 16:05, Johan Hovold wrote:
> > On Mon, Jan 02, 2023 at 01:24:34PM +0100, Krzysztof Kozlowski wrote:
> >> On 02/01/2023 13:23, Krzysztof Kozlowski wrote:
> >>> On 02/01/2023 11:50, Johan Hovold wrote:
> >>>> The wcd938x codec is not a memory-mapped device and does not belong
> >>>> under the soc node.
> >>>>
> >>>> Move the node to the root node to avoid DT validation failures.
> >>>>
> >>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> >>>> ---
> >>>>  arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 52 ++++++++++++-------------
> >>>>  1 file changed, 25 insertions(+), 27 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
> >>>> index 4de3e1f1c39c..217b2c654745 100644
> >>>> --- a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
> >>>> +++ b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
> >>>> @@ -17,6 +17,31 @@ aliases {
> >>>>  		serial0 = &uart7;
> >>>>  	};
> >>>>  
> >>>> +	wcd938x: codec {
> >>>
> >>> Let's call it audio-codec and put it somewhere ordered alphabetically.
> >>> This avoids useless reshuffles immediately. Moving things back and forth
> >>> does not make sense.
> >>
> >> Eh, on the other hand SM8250-MTP uses name codec so maybe the rename in
> >> SC8280xp should be dropped.
> > 
> > Nah, let's go with audio-codec. Your MTP change hasn't been merged yet
> > so we can still drop or respin that one.
> 
> It was, ~5 days ago, so please rebase.

Again, not in the qcom tree yet AFAICS.

Johan

Re: [PATCH 5/6] arm64: dts: qcom: sm8250-mtp: fix wcd938x codec node

[Krzysztof Kozlowski]

On 02/01/2023 16:18, Johan Hovold wrote:
> On Mon, Jan 02, 2023 at 04:09:14PM +0100, Krzysztof Kozlowski wrote:
>> On 02/01/2023 16:03, Johan Hovold wrote:
>>> On Mon, Jan 02, 2023 at 01:20:03PM +0100, Krzysztof Kozlowski wrote:
>>>> On 02/01/2023 11:50, Johan Hovold wrote:
>>>>> The wcd938x codec is not a memory-mapped device and does not belong
>>>>> under the soc node.
>>>>>
>>>>> Move the node to the root node to avoid DT validation failures.
>>>>>
>>>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>>>>> ---
>>>>>  arch/arm64/boot/dts/qcom/sm8250-mtp.dts | 40 ++++++++++++-------------
>>>>>  1 file changed, 19 insertions(+), 21 deletions(-)
>>>>
>>>> Don't you base on some older tree? It was alreadt moved. Maybe other
>>>> changes were also implemented...
>>>
>>> If you've moved this node is hasn't made it into linux-next (or the qcom
>>> tree) yet.
>>
>> So you need to update your tree/rebase as this is already in Qualcomm
>> tree. There was no linux-next release since a week, so that's not a
>> solution.
> 
> As I wrote above, it's not in the qcom tree either.

Really?

https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/commit/?h=for-next&id=e5b8c08245307a82cdf180cd5d385a34ba1cfd9d

Best regards,
Krzysztof

Re: [PATCH 6/6] arm64: dts: qcom: sm8450-hdk: fix wcd938x codec node

[Krzysztof Kozlowski]

On 02/01/2023 16:18, Johan Hovold wrote:
> On Mon, Jan 02, 2023 at 04:09:59PM +0100, Krzysztof Kozlowski wrote:
>> On 02/01/2023 16:05, Johan Hovold wrote:
>>> On Mon, Jan 02, 2023 at 01:24:34PM +0100, Krzysztof Kozlowski wrote:
>>>> On 02/01/2023 13:23, Krzysztof Kozlowski wrote:
>>>>> On 02/01/2023 11:50, Johan Hovold wrote:
>>>>>> The wcd938x codec is not a memory-mapped device and does not belong
>>>>>> under the soc node.
>>>>>>
>>>>>> Move the node to the root node to avoid DT validation failures.
>>>>>>
>>>>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>>>>>> ---
>>>>>>  arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 52 ++++++++++++-------------
>>>>>>  1 file changed, 25 insertions(+), 27 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
>>>>>> index 4de3e1f1c39c..217b2c654745 100644
>>>>>> --- a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
>>>>>> @@ -17,6 +17,31 @@ aliases {
>>>>>>  		serial0 = &uart7;
>>>>>>  	};
>>>>>>  
>>>>>> +	wcd938x: codec {
>>>>>
>>>>> Let's call it audio-codec and put it somewhere ordered alphabetically.
>>>>> This avoids useless reshuffles immediately. Moving things back and forth
>>>>> does not make sense.
>>>>
>>>> Eh, on the other hand SM8250-MTP uses name codec so maybe the rename in
>>>> SC8280xp should be dropped.
>>>
>>> Nah, let's go with audio-codec. Your MTP change hasn't been merged yet
>>> so we can still drop or respin that one.
>>
>> It was, ~5 days ago, so please rebase.
> 
> Again, not in the qcom tree yet AFAICS.

It's clearly visible there, since a week, so please update your trees
and rebase.

Best regards,
Krzysztof

Re: [PATCH 1/6] arm64: dts: qcom: sc8280xp-x13s: disable soundcard

[Johan Hovold]

On Mon, Jan 02, 2023 at 04:12:35PM +0100, Krzysztof Kozlowski wrote:
> On 02/01/2023 16:07, Johan Hovold wrote:
> > On Mon, Jan 02, 2023 at 01:25:38PM +0100, Krzysztof Kozlowski wrote:
> >> On 02/01/2023 11:50, Johan Hovold wrote:
> >>> Driver support for the X13s soundcard is not yet in place so disable it
> >>> for now to avoid probe failures such as:
> >>>
> >>> [   11.077727] qcom-prm gprsvc:service:2:2: DSP returned error[100100f] 1
> >>> [   11.077926] rx_macro: probe of 3200000.rxmacro failed with error -22
> >>> [   21.221104] platform 3210000.soundwire-controller: deferred probe pending
> >>>
> >>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> >>> ---
> >>>  .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts  | 12 ++++++++++--
> >>>  1 file changed, 10 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> >>> index 0201c6776746..97ff74d5095e 100644
> >>> --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> >>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> >>> @@ -649,6 +649,8 @@ wcd938x: codec {
> >>>  		qcom,mbhc-headphone-vthreshold-microvolt = <50000>;
> >>>  		qcom,rx-device = <&wcd_rx>;
> >>>  		qcom,tx-device = <&wcd_tx>;
> >>> +
> >>> +		status = "disabled";
> >>>  	};
> >>>  };
> >>>  
> >>> @@ -669,6 +671,8 @@ &sound {
> >>>  		"TX DMIC2", "MIC BIAS3",
> >>>  		"TX SWR_ADC1", "ADC2_OUTPUT";
> >>>  
> >>> +	status = "disabled";
> >>> +
> >>>  	wcd-playback-dai-link {
> >>>  		link-name = "WCD Playback";
> >>>  		cpu {
> >>> @@ -731,6 +735,8 @@ codec {
> >>>  };
> >>>  
> >>>  &swr0 {
> >>> +	status = "disabled";
> >>> +
> >>>  	left_spkr: wsa8830-left@0,1 {
> >>>  		compatible = "sdw10217020200";
> >>>  		reg = <0 1>;
> >>> @@ -757,7 +763,7 @@ right_spkr: wsa8830-right@0,2{
> >>>  };
> >>>  
> >>>  &swr1 {
> >>> -	status = "okay";
> >>> +	status = "disabled";
> >>>  
> >>>  	wcd_rx: wcd9380-rx@0,4 {
> >>>  		compatible = "sdw20217010d00";
> >>> @@ -767,7 +773,7 @@ wcd_rx: wcd9380-rx@0,4 {
> >>>  };
> >>>  
> >>>  &swr2 {
> >>> -	status = "okay";
> >>> +	status = "disabled";
> >>
> >> That's a double disable.
> >>
> >>>  
> >>>  	wcd_tx: wcd9380-tx@0,3 {
> >>>  		compatible = "sdw20217010d00";
> >>> @@ -781,6 +787,8 @@ &vamacro {
> >>>  	pinctrl-names = "default";
> >>>  	vdd-micb-supply = <&vreg_s10b>;
> >>>  	qcom,dmic-sample-rate = <600000>;
> >>> +
> >>> +	status = "disabled";
> >>
> >> That's a double disable.
> > 
> > Yes, that's on purpose. We're temporarily disabling these nodes instead
> > of reverting the series which should not have been merged.
> 
> I don't get why disabling something twice is anyhow related to
> "temporarily disable". One disable is enough for temporary or permanent
> disables.

It clearly shows that this was done on purpose and indicates which
properties need to be changed to "okay" once we have actual support.
 
> > 
> > Once we have driver support, these properties will be updated again.
> 
> Linux kernel is not the only consumer of DTS, thus having or not having
> the support in the kernel is not reason to disable pieces of it.
> Assuming the DTS is correct, of course, because maybe that's the problem?

Okay, let's revert these sound dts changes then until we have support.
We have no idea if the dts changes are correct as sound still depends
on out-of-tree hacks.

People are using -next for development and I don't want to see them
toast their speakers because we failed get the dependencies merged
before merging the dts changes which is how we normally do this.

So shall I just send a revert instead? I really don't care as long as
this is disabled again today.

Johan

Re: [PATCH 5/6] arm64: dts: qcom: sm8250-mtp: fix wcd938x codec node

[Johan Hovold]

On Mon, Jan 02, 2023 at 04:22:30PM +0100, Krzysztof Kozlowski wrote:
> On 02/01/2023 16:18, Johan Hovold wrote:
> > On Mon, Jan 02, 2023 at 04:09:14PM +0100, Krzysztof Kozlowski wrote:
> >> On 02/01/2023 16:03, Johan Hovold wrote:
> >>> On Mon, Jan 02, 2023 at 01:20:03PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 02/01/2023 11:50, Johan Hovold wrote:
> >>>>> The wcd938x codec is not a memory-mapped device and does not belong
> >>>>> under the soc node.
> >>>>>
> >>>>> Move the node to the root node to avoid DT validation failures.
> >>>>>
> >>>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> >>>>> ---
> >>>>>  arch/arm64/boot/dts/qcom/sm8250-mtp.dts | 40 ++++++++++++-------------
> >>>>>  1 file changed, 19 insertions(+), 21 deletions(-)
> >>>>
> >>>> Don't you base on some older tree? It was alreadt moved. Maybe other
> >>>> changes were also implemented...
> >>>
> >>> If you've moved this node is hasn't made it into linux-next (or the qcom
> >>> tree) yet.
> >>
> >> So you need to update your tree/rebase as this is already in Qualcomm
> >> tree. There was no linux-next release since a week, so that's not a
> >> solution.
> > 
> > As I wrote above, it's not in the qcom tree either.
> 
> Really?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/commit/?h=for-next&id=e5b8c08245307a82cdf180cd5d385a34ba1cfd9d

Sorry, most have looked at on old branch or something.

Johan

Re: [PATCH 1/6] arm64: dts: qcom: sc8280xp-x13s: disable soundcard

[Krzysztof Kozlowski]

On 02/01/2023 16:24, Johan Hovold wrote:
> On Mon, Jan 02, 2023 at 04:12:35PM +0100, Krzysztof Kozlowski wrote:
>> On 02/01/2023 16:07, Johan Hovold wrote:
>>> On Mon, Jan 02, 2023 at 01:25:38PM +0100, Krzysztof Kozlowski wrote:
>>>> On 02/01/2023 11:50, Johan Hovold wrote:
>>>>> Driver support for the X13s soundcard is not yet in place so disable it
>>>>> for now to avoid probe failures such as:
>>>>>
>>>>> [   11.077727] qcom-prm gprsvc:service:2:2: DSP returned error[100100f] 1
>>>>> [   11.077926] rx_macro: probe of 3200000.rxmacro failed with error -22
>>>>> [   21.221104] platform 3210000.soundwire-controller: deferred probe pending
>>>>>
>>>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>>>>> ---
>>>>>  .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts  | 12 ++++++++++--
>>>>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
>>>>> index 0201c6776746..97ff74d5095e 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
>>>>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
>>>>> @@ -649,6 +649,8 @@ wcd938x: codec {
>>>>>  		qcom,mbhc-headphone-vthreshold-microvolt = <50000>;
>>>>>  		qcom,rx-device = <&wcd_rx>;
>>>>>  		qcom,tx-device = <&wcd_tx>;
>>>>> +
>>>>> +		status = "disabled";
>>>>>  	};
>>>>>  };
>>>>>  
>>>>> @@ -669,6 +671,8 @@ &sound {
>>>>>  		"TX DMIC2", "MIC BIAS3",
>>>>>  		"TX SWR_ADC1", "ADC2_OUTPUT";
>>>>>  
>>>>> +	status = "disabled";
>>>>> +
>>>>>  	wcd-playback-dai-link {
>>>>>  		link-name = "WCD Playback";
>>>>>  		cpu {
>>>>> @@ -731,6 +735,8 @@ codec {
>>>>>  };
>>>>>  
>>>>>  &swr0 {
>>>>> +	status = "disabled";
>>>>> +
>>>>>  	left_spkr: wsa8830-left@0,1 {
>>>>>  		compatible = "sdw10217020200";
>>>>>  		reg = <0 1>;
>>>>> @@ -757,7 +763,7 @@ right_spkr: wsa8830-right@0,2{
>>>>>  };
>>>>>  
>>>>>  &swr1 {
>>>>> -	status = "okay";
>>>>> +	status = "disabled";
>>>>>  
>>>>>  	wcd_rx: wcd9380-rx@0,4 {
>>>>>  		compatible = "sdw20217010d00";
>>>>> @@ -767,7 +773,7 @@ wcd_rx: wcd9380-rx@0,4 {
>>>>>  };
>>>>>  
>>>>>  &swr2 {
>>>>> -	status = "okay";
>>>>> +	status = "disabled";
>>>>
>>>> That's a double disable.
>>>>
>>>>>  
>>>>>  	wcd_tx: wcd9380-tx@0,3 {
>>>>>  		compatible = "sdw20217010d00";
>>>>> @@ -781,6 +787,8 @@ &vamacro {
>>>>>  	pinctrl-names = "default";
>>>>>  	vdd-micb-supply = <&vreg_s10b>;
>>>>>  	qcom,dmic-sample-rate = <600000>;
>>>>> +
>>>>> +	status = "disabled";
>>>>
>>>> That's a double disable.
>>>
>>> Yes, that's on purpose. We're temporarily disabling these nodes instead
>>> of reverting the series which should not have been merged.
>>
>> I don't get why disabling something twice is anyhow related to
>> "temporarily disable". One disable is enough for temporary or permanent
>> disables.
> 
> It clearly shows that this was done on purpose and indicates which
> properties need to be changed to "okay" once we have actual support.

No, it shows nothing clearly as from time to time we got duplicated
properties and it's a simply mistake. The double disable without any
comment looks like mistake, not intentional code.

>  
>>>
>>> Once we have driver support, these properties will be updated again.
>>
>> Linux kernel is not the only consumer of DTS, thus having or not having
>> the support in the kernel is not reason to disable pieces of it.
>> Assuming the DTS is correct, of course, because maybe that's the problem?
> 
> Okay, let's revert these sound dts changes then until we have support.
> We have no idea if the dts changes are correct as sound still depends
> on out-of-tree hacks.
> 
> People are using -next for development and I don't want to see them
> toast their speakers because we failed get the dependencies merged
> before merging the dts changes which is how we normally do this.

If the error is in DTS, yeah, revert or disable is a way. But if the
issue is in the incomplete or broken Linux drivers, then these should be
changed, e.g. intentionally fail probing, skip new devices, drop new
compatible etc.

> So shall I just send a revert instead? I really don't care as long as
> this is disabled again today.


Best regards,
Krzysztof

Re: [PATCH 1/6] arm64: dts: qcom: sc8280xp-x13s: disable soundcard

[Johan Hovold]

On Mon, Jan 02, 2023 at 04:28:56PM +0100, Krzysztof Kozlowski wrote:
> On 02/01/2023 16:24, Johan Hovold wrote:
> > On Mon, Jan 02, 2023 at 04:12:35PM +0100, Krzysztof Kozlowski wrote:
> >> On 02/01/2023 16:07, Johan Hovold wrote:
> >>> On Mon, Jan 02, 2023 at 01:25:38PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 02/01/2023 11:50, Johan Hovold wrote:
> >>>>> Driver support for the X13s soundcard is not yet in place so disable it
> >>>>> for now to avoid probe failures such as:
> >>>>>
> >>>>> [   11.077727] qcom-prm gprsvc:service:2:2: DSP returned error[100100f] 1
> >>>>> [   11.077926] rx_macro: probe of 3200000.rxmacro failed with error -22
> >>>>> [   21.221104] platform 3210000.soundwire-controller: deferred probe pending
> >>>>>
> >>>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> >>>>> ---
> >>>>>  .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts  | 12 ++++++++++--
> >>>>>  1 file changed, 10 insertions(+), 2 deletions(-)

> >>>>>  	wcd_tx: wcd9380-tx@0,3 {
> >>>>>  		compatible = "sdw20217010d00";
> >>>>> @@ -781,6 +787,8 @@ &vamacro {
> >>>>>  	pinctrl-names = "default";
> >>>>>  	vdd-micb-supply = <&vreg_s10b>;
> >>>>>  	qcom,dmic-sample-rate = <600000>;
> >>>>> +
> >>>>> +	status = "disabled";
> >>>>
> >>>> That's a double disable.
> >>>
> >>> Yes, that's on purpose. We're temporarily disabling these nodes instead
> >>> of reverting the series which should not have been merged.
> >>
> >> I don't get why disabling something twice is anyhow related to
> >> "temporarily disable". One disable is enough for temporary or permanent
> >> disables.
> > 
> > It clearly shows that this was done on purpose and indicates which
> > properties need to be changed to "okay" once we have actual support.
> 
> No, it shows nothing clearly as from time to time we got duplicated
> properties and it's a simply mistake. The double disable without any
> comment looks like mistake, not intentional code.

It's not a mistake. It's intentional. And I don't want to spend hours on
this because of someone else's cock-up.

> >>>
> >>> Once we have driver support, these properties will be updated again.
> >>
> >> Linux kernel is not the only consumer of DTS, thus having or not having
> >> the support in the kernel is not reason to disable pieces of it.
> >> Assuming the DTS is correct, of course, because maybe that's the problem?
> > 
> > Okay, let's revert these sound dts changes then until we have support.
> > We have no idea if the dts changes are correct as sound still depends
> > on out-of-tree hacks.
> > 
> > People are using -next for development and I don't want to see them
> > toast their speakers because we failed get the dependencies merged
> > before merging the dts changes which is how we normally do this.
> 
> If the error is in DTS, yeah, revert or disable is a way. But if the
> issue is in the incomplete or broken Linux drivers, then these should be
> changed, e.g. intentionally fail probing, skip new devices, drop new
> compatible etc.

And how long does it take for that to propagate and isn't the response
just going go to be "well then fix the driver".

I think you're just being unreasonable here.

If Bjorn could rebase his tree, he could simply drop these for now as
sound support was clearly not ready. Since that isn't the case we need
to at least try to be constructive and figure out a reasonable
alternative. While "Linux isn't the only consumer" is a true statement,
it really is not relevant just because there are some dts changes in
Bjorn's tree which should not be there.

Johan

Re: [PATCH 1/6] arm64: dts: qcom: sc8280xp-x13s: disable soundcard

[Krzysztof Kozlowski]

On 02/01/2023 16:39, Johan Hovold wrote:
>>>>>>>  	wcd_tx: wcd9380-tx@0,3 {
>>>>>>>  		compatible = "sdw20217010d00";
>>>>>>> @@ -781,6 +787,8 @@ &vamacro {
>>>>>>>  	pinctrl-names = "default";
>>>>>>>  	vdd-micb-supply = <&vreg_s10b>;
>>>>>>>  	qcom,dmic-sample-rate = <600000>;
>>>>>>> +
>>>>>>> +	status = "disabled";
>>>>>>
>>>>>> That's a double disable.
>>>>>
>>>>> Yes, that's on purpose. We're temporarily disabling these nodes instead
>>>>> of reverting the series which should not have been merged.
>>>>
>>>> I don't get why disabling something twice is anyhow related to
>>>> "temporarily disable". One disable is enough for temporary or permanent
>>>> disables.
>>>
>>> It clearly shows that this was done on purpose and indicates which
>>> properties need to be changed to "okay" once we have actual support.
>>
>> No, it shows nothing clearly as from time to time we got duplicated
>> properties and it's a simply mistake. The double disable without any
>> comment looks like mistake, not intentional code.
> 
> It's not a mistake. It's intentional. And I don't want to spend hours on
> this because of someone else's cock-up.

To you it looks intentional, but for the reader of DTS which has
disabled node in DTSI and in DTS - so in two places - it looks like a
pure bug. Just because you know the reason behind the change does not
make the code readable.

> 
>>>>>
>>>>> Once we have driver support, these properties will be updated again.
>>>>
>>>> Linux kernel is not the only consumer of DTS, thus having or not having
>>>> the support in the kernel is not reason to disable pieces of it.
>>>> Assuming the DTS is correct, of course, because maybe that's the problem?
>>>
>>> Okay, let's revert these sound dts changes then until we have support.
>>> We have no idea if the dts changes are correct as sound still depends
>>> on out-of-tree hacks.
>>>
>>> People are using -next for development and I don't want to see them
>>> toast their speakers because we failed get the dependencies merged
>>> before merging the dts changes which is how we normally do this.
>>
>> If the error is in DTS, yeah, revert or disable is a way. But if the
>> issue is in the incomplete or broken Linux drivers, then these should be
>> changed, e.g. intentionally fail probing, skip new devices, drop new
>> compatible etc.
> 
> And how long does it take for that to propagate and isn't the response
> just going go to be "well then fix the driver".
> 
> I think you're just being unreasonable here.

I did not propose to fix the driver. I proposed to fail the driver's
probe or remove the compatible from it.

Such change propagate the same speed as DTS change.

> If Bjorn could rebase his tree, he could simply drop these for now as
> sound support was clearly not ready. Since that isn't the case we need
> to at least try to be constructive and figure out a reasonable
> alternative. While "Linux isn't the only consumer" is a true statement,
> it really is not relevant just because there are some dts changes in
> Bjorn's tree which should not be there.

The SC8280XP audio DTS looks in general correct, except some style
issues, redundant properties and never tested against DT bindings.
Therefore it looks as accurate and more-or-less correct representation
of the hardware, unless you have some more details on this.

Is the driver is to blame, focus there, not on DTS.

Best regards,
Krzysztof

Re: [PATCH 1/6] arm64: dts: qcom: sc8280xp-x13s: disable soundcard

[Johan Hovold]

On Mon, Jan 02, 2023 at 04:46:40PM +0100, Krzysztof Kozlowski wrote:
> On 02/01/2023 16:39, Johan Hovold wrote:
> >>>>>>>  	wcd_tx: wcd9380-tx@0,3 {
> >>>>>>>  		compatible = "sdw20217010d00";
> >>>>>>> @@ -781,6 +787,8 @@ &vamacro {
> >>>>>>>  	pinctrl-names = "default";
> >>>>>>>  	vdd-micb-supply = <&vreg_s10b>;
> >>>>>>>  	qcom,dmic-sample-rate = <600000>;
> >>>>>>> +
> >>>>>>> +	status = "disabled";
> >>>>>>
> >>>>>> That's a double disable.
> >>>>>
> >>>>> Yes, that's on purpose. We're temporarily disabling these nodes instead
> >>>>> of reverting the series which should not have been merged.
> >>>>
> >>>> I don't get why disabling something twice is anyhow related to
> >>>> "temporarily disable". One disable is enough for temporary or permanent
> >>>> disables.
> >>>
> >>> It clearly shows that this was done on purpose and indicates which
> >>> properties need to be changed to "okay" once we have actual support.
> >>
> >> No, it shows nothing clearly as from time to time we got duplicated
> >> properties and it's a simply mistake. The double disable without any
> >> comment looks like mistake, not intentional code.
> > 
> > It's not a mistake. It's intentional. And I don't want to spend hours on
> > this because of someone else's cock-up.
> 
> To you it looks intentional, but for the reader of DTS which has
> disabled node in DTSI and in DTS - so in two places - it looks like a
> pure bug. Just because you know the reason behind the change does not
> make the code readable.

Calling a (temporary) redundant property a 'pure bug' seems like a bit
of stretch, and it has nothing to do with readability.

> >>>>>
> >>>>> Once we have driver support, these properties will be updated again.
> >>>>
> >>>> Linux kernel is not the only consumer of DTS, thus having or not having
> >>>> the support in the kernel is not reason to disable pieces of it.
> >>>> Assuming the DTS is correct, of course, because maybe that's the problem?
> >>>
> >>> Okay, let's revert these sound dts changes then until we have support.
> >>> We have no idea if the dts changes are correct as sound still depends
> >>> on out-of-tree hacks.
> >>>
> >>> People are using -next for development and I don't want to see them
> >>> toast their speakers because we failed get the dependencies merged
> >>> before merging the dts changes which is how we normally do this.
> >>
> >> If the error is in DTS, yeah, revert or disable is a way. But if the
> >> issue is in the incomplete or broken Linux drivers, then these should be
> >> changed, e.g. intentionally fail probing, skip new devices, drop new
> >> compatible etc.
> > 
> > And how long does it take for that to propagate and isn't the response
> > just going go to be "well then fix the driver".
> > 
> > I think you're just being unreasonable here.
> 
> I did not propose to fix the driver. I proposed to fail the driver's
> probe or remove the compatible from it.
> 
> Such change propagate the same speed as DTS change.

But the DTS changes are in Bjorn branch and Bjorn and I discussed it and
decided to disable them temporarily instead of reverting.

Now you're asking me to figure out all the dependent driver and patch
them individually. And this may not reach next before the DTS changes
do.

> > If Bjorn could rebase his tree, he could simply drop these for now as
> > sound support was clearly not ready. Since that isn't the case we need
> > to at least try to be constructive and figure out a reasonable
> > alternative. While "Linux isn't the only consumer" is a true statement,
> > it really is not relevant just because there are some dts changes in
> > Bjorn's tree which should not be there.
> 
> The SC8280XP audio DTS looks in general correct, except some style
> issues, redundant properties and never tested against DT bindings.
> Therefore it looks as accurate and more-or-less correct representation
> of the hardware, unless you have some more details on this.

Only that the drivers fail to probe in multiple ways, some which may
require updating the bindings to address. There's also an indication
that some further driver support is needed for proper speaker
protection. That really should be in place before we enable this.

Johan

Re: [PATCH 1/6] arm64: dts: qcom: sc8280xp-x13s: disable soundcard

[Krzysztof Kozlowski]

On 02/01/2023 16:58, Johan Hovold wrote:
> On Mon, Jan 02, 2023 at 04:46:40PM +0100, Krzysztof Kozlowski wrote:
>> On 02/01/2023 16:39, Johan Hovold wrote:
>>>>>>>>>  	wcd_tx: wcd9380-tx@0,3 {
>>>>>>>>>  		compatible = "sdw20217010d00";
>>>>>>>>> @@ -781,6 +787,8 @@ &vamacro {
>>>>>>>>>  	pinctrl-names = "default";
>>>>>>>>>  	vdd-micb-supply = <&vreg_s10b>;
>>>>>>>>>  	qcom,dmic-sample-rate = <600000>;
>>>>>>>>> +
>>>>>>>>> +	status = "disabled";
>>>>>>>>
>>>>>>>> That's a double disable.
>>>>>>>
>>>>>>> Yes, that's on purpose. We're temporarily disabling these nodes instead
>>>>>>> of reverting the series which should not have been merged.
>>>>>>
>>>>>> I don't get why disabling something twice is anyhow related to
>>>>>> "temporarily disable". One disable is enough for temporary or permanent
>>>>>> disables.
>>>>>
>>>>> It clearly shows that this was done on purpose and indicates which
>>>>> properties need to be changed to "okay" once we have actual support.
>>>>
>>>> No, it shows nothing clearly as from time to time we got duplicated
>>>> properties and it's a simply mistake. The double disable without any
>>>> comment looks like mistake, not intentional code.
>>>
>>> It's not a mistake. It's intentional. And I don't want to spend hours on
>>> this because of someone else's cock-up.
>>
>> To you it looks intentional, but for the reader of DTS which has
>> disabled node in DTSI and in DTS - so in two places - it looks like a
>> pure bug. Just because you know the reason behind the change does not
>> make the code readable.
> 
> Calling a (temporary) redundant property a 'pure bug' seems like a bit
> of stretch, and it has nothing to do with readability.

Redundant properties is not a code which we want to have anywhere. Why
you are so opposed to documenting this oddity?

> 
>>>>>>>
>>>>>>> Once we have driver support, these properties will be updated again.
>>>>>>
>>>>>> Linux kernel is not the only consumer of DTS, thus having or not having
>>>>>> the support in the kernel is not reason to disable pieces of it.
>>>>>> Assuming the DTS is correct, of course, because maybe that's the problem?
>>>>>
>>>>> Okay, let's revert these sound dts changes then until we have support.
>>>>> We have no idea if the dts changes are correct as sound still depends
>>>>> on out-of-tree hacks.
>>>>>
>>>>> People are using -next for development and I don't want to see them
>>>>> toast their speakers because we failed get the dependencies merged
>>>>> before merging the dts changes which is how we normally do this.
>>>>
>>>> If the error is in DTS, yeah, revert or disable is a way. But if the
>>>> issue is in the incomplete or broken Linux drivers, then these should be
>>>> changed, e.g. intentionally fail probing, skip new devices, drop new
>>>> compatible etc.
>>>
>>> And how long does it take for that to propagate and isn't the response
>>> just going go to be "well then fix the driver".
>>>
>>> I think you're just being unreasonable here.
>>
>> I did not propose to fix the driver. I proposed to fail the driver's
>> probe or remove the compatible from it.
>>
>> Such change propagate the same speed as DTS change.
> 
> But the DTS changes are in Bjorn branch and Bjorn and I discussed it and
> decided to disable them temporarily instead of reverting.
> 
> Now you're asking me to figure out all the dependent driver and patch
> them individually. And this may not reach next before the DTS changes
> do.

Users do not work on linux-next. linux-next is integration tree for
developers. Pretty often broken and not stable, so anyone using it
accepts the risks. Using now linux-next argument for a change is not
appropriate. The change should be reasonable regardless of users of
linux-next.

> 
>>> If Bjorn could rebase his tree, he could simply drop these for now as
>>> sound support was clearly not ready. Since that isn't the case we need
>>> to at least try to be constructive and figure out a reasonable
>>> alternative. While "Linux isn't the only consumer" is a true statement,
>>> it really is not relevant just because there are some dts changes in
>>> Bjorn's tree which should not be there.
>>
>> The SC8280XP audio DTS looks in general correct, except some style
>> issues, redundant properties and never tested against DT bindings.
>> Therefore it looks as accurate and more-or-less correct representation
>> of the hardware, unless you have some more details on this.
> 
> Only that the drivers fail to probe in multiple ways, some which may
> require updating the bindings to address. 

I don't think there is anything needed to fix in bindings in
incompatible way. I was working on them as well (for HDK8450) and I
don't recall any issues.

If you see anything specific, use specific arguments, because otherwise
it is just FUD.

> There's also an indication
> that some further driver support is needed for proper speaker
> protection. That really should be in place before we enable this.

There is easy solution for this - drop the compatible from drivers. Or
if driver is SC8280xp specific, mark it as BROKEN in Kconfig. Or fail
the probe so it won't bother your system.
Best regards,
Krzysztof

Re: [PATCH 1/6] arm64: dts: qcom: sc8280xp-x13s: disable soundcard

[Johan Hovold]

On Mon, Jan 02, 2023 at 05:13:24PM +0100, Krzysztof Kozlowski wrote:
> On 02/01/2023 16:58, Johan Hovold wrote:
> > On Mon, Jan 02, 2023 at 04:46:40PM +0100, Krzysztof Kozlowski wrote:
> >> On 02/01/2023 16:39, Johan Hovold wrote:
> >>>>>>>>>  	wcd_tx: wcd9380-tx@0,3 {
> >>>>>>>>>  		compatible = "sdw20217010d00";
> >>>>>>>>> @@ -781,6 +787,8 @@ &vamacro {
> >>>>>>>>>  	pinctrl-names = "default";
> >>>>>>>>>  	vdd-micb-supply = <&vreg_s10b>;
> >>>>>>>>>  	qcom,dmic-sample-rate = <600000>;
> >>>>>>>>> +
> >>>>>>>>> +	status = "disabled";
> >>>>>>>>
> >>>>>>>> That's a double disable.
> >>>>>>>
> >>>>>>> Yes, that's on purpose. We're temporarily disabling these nodes instead
> >>>>>>> of reverting the series which should not have been merged.
> >>>>>>
> >>>>>> I don't get why disabling something twice is anyhow related to
> >>>>>> "temporarily disable". One disable is enough for temporary or permanent
> >>>>>> disables.
> >>>>>
> >>>>> It clearly shows that this was done on purpose and indicates which
> >>>>> properties need to be changed to "okay" once we have actual support.
> >>>>
> >>>> No, it shows nothing clearly as from time to time we got duplicated
> >>>> properties and it's a simply mistake. The double disable without any
> >>>> comment looks like mistake, not intentional code.
> >>>
> >>> It's not a mistake. It's intentional. And I don't want to spend hours on
> >>> this because of someone else's cock-up.
> >>
> >> To you it looks intentional, but for the reader of DTS which has
> >> disabled node in DTSI and in DTS - so in two places - it looks like a
> >> pure bug. Just because you know the reason behind the change does not
> >> make the code readable.
> > 
> > Calling a (temporary) redundant property a 'pure bug' seems like a bit
> > of stretch, and it has nothing to do with readability.
> 
> Redundant properties is not a code which we want to have anywhere. Why
> you are so opposed to documenting this oddity?

I'm not at all opposed to adding a comment that this is a temporary
disable. Hopefully we can even get the driver support ready, things
tested, and enable these nodes before 6.3 is released.

> >>>>>>>
> >>>>>>> Once we have driver support, these properties will be updated again.
> >>>>>>
> >>>>>> Linux kernel is not the only consumer of DTS, thus having or not having
> >>>>>> the support in the kernel is not reason to disable pieces of it.
> >>>>>> Assuming the DTS is correct, of course, because maybe that's the problem?
> >>>>>
> >>>>> Okay, let's revert these sound dts changes then until we have support.
> >>>>> We have no idea if the dts changes are correct as sound still depends
> >>>>> on out-of-tree hacks.
> >>>>>
> >>>>> People are using -next for development and I don't want to see them
> >>>>> toast their speakers because we failed get the dependencies merged
> >>>>> before merging the dts changes which is how we normally do this.
> >>>>
> >>>> If the error is in DTS, yeah, revert or disable is a way. But if the
> >>>> issue is in the incomplete or broken Linux drivers, then these should be
> >>>> changed, e.g. intentionally fail probing, skip new devices, drop new
> >>>> compatible etc.
> >>>
> >>> And how long does it take for that to propagate and isn't the response
> >>> just going go to be "well then fix the driver".
> >>>
> >>> I think you're just being unreasonable here.
> >>
> >> I did not propose to fix the driver. I proposed to fail the driver's
> >> probe or remove the compatible from it.
> >>
> >> Such change propagate the same speed as DTS change.
> > 
> > But the DTS changes are in Bjorn branch and Bjorn and I discussed it and
> > decided to disable them temporarily instead of reverting.
> > 
> > Now you're asking me to figure out all the dependent driver and patch
> > them individually. And this may not reach next before the DTS changes
> > do.
> 
> Users do not work on linux-next. linux-next is integration tree for
> developers. Pretty often broken and not stable, so anyone using it
> accepts the risks. Using now linux-next argument for a change is not
> appropriate. The change should be reasonable regardless of users of
> linux-next.

The argument is that this should never have been merged last week. And
we do have users running linux-next as support for x13s is very much
still under development. Sure, they don't expect things to always work
perfectly, but I still want to avoid toasting there speakers if I can.

> >>> If Bjorn could rebase his tree, he could simply drop these for now as
> >>> sound support was clearly not ready. Since that isn't the case we need
> >>> to at least try to be constructive and figure out a reasonable
> >>> alternative. While "Linux isn't the only consumer" is a true statement,
> >>> it really is not relevant just because there are some dts changes in
> >>> Bjorn's tree which should not be there.
> >>
> >> The SC8280XP audio DTS looks in general correct, except some style
> >> issues, redundant properties and never tested against DT bindings.
> >> Therefore it looks as accurate and more-or-less correct representation
> >> of the hardware, unless you have some more details on this.
> > 
> > Only that the drivers fail to probe in multiple ways, some which may
> > require updating the bindings to address. 
> 
> I don't think there is anything needed to fix in bindings in
> incompatible way. I was working on them as well (for HDK8450) and I
> don't recall any issues.
> 
> If you see anything specific, use specific arguments, because otherwise
> it is just FUD.

You can call it FUD if you want, I just call it being cautious.

> > There's also an indication
> > that some further driver support is needed for proper speaker
> > protection. That really should be in place before we enable this.
> 
> There is easy solution for this - drop the compatible from drivers. Or
> if driver is SC8280xp specific, mark it as BROKEN in Kconfig. Or fail
> the probe so it won't bother your system.

Or we just revert or disable it temporarily in the x13s dts until we
better understand the missing driver bits.

Johan

Re: [PATCH 2/6] arm64: dts: qcom: sc8280xp: disable sound nodes

[Johan Hovold]

On Mon, Jan 02, 2023 at 04:15:42PM +0100, Johan Hovold wrote:
> On Mon, Jan 02, 2023 at 01:29:38PM +0100, Krzysztof Kozlowski wrote:
> > On 02/01/2023 11:50, Johan Hovold wrote:
> > > The sound nodes in the SoC dtsi should be disabled by default.
> > > 
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > index ed1e2bee86ee..c1ce2d7b3675 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > @@ -1733,6 +1733,8 @@ rxmacro: rxmacro@3200000 {
> > >  
> > >  			pinctrl-names = "default";
> > >  			pinctrl-0 = <&rx_swr_default>;
> > > +
> > > +			status = "disabled";
> > 
> > There is no reason this should be disabled by default. No external (per
> > board) configuration is needed and by default SoC components should be
> > enabled if they do not need anything from the board.
> 
> This node is one of the nodes for which driver support is not yet in
> place so that's one reason for at least disabling it temporarily.
> 
> Since all other pinconfig lives in the board dts, if we decide to move
> also the sound pinconfig then then that may be a second.

Also note that these depend on q6prmcc which is in turn depends on
remoteproc_adsp being enabled by the x13s dts.

So keeping them disabled by default seems justified.

Johan

Re: [PATCH 2/6] arm64: dts: qcom: sc8280xp: disable sound nodes

[Konrad Dybcio]

On 2.01.2023 18:17, Johan Hovold wrote:
> On Mon, Jan 02, 2023 at 04:15:42PM +0100, Johan Hovold wrote:
>> On Mon, Jan 02, 2023 at 01:29:38PM +0100, Krzysztof Kozlowski wrote:
>>> On 02/01/2023 11:50, Johan Hovold wrote:
>>>> The sound nodes in the SoC dtsi should be disabled by default.
>>>>
>>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>>>> ---
>>>>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 16 ++++++++++++++++
>>>>  1 file changed, 16 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>>> index ed1e2bee86ee..c1ce2d7b3675 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>>> @@ -1733,6 +1733,8 @@ rxmacro: rxmacro@3200000 {
>>>>  
>>>>  			pinctrl-names = "default";
>>>>  			pinctrl-0 = <&rx_swr_default>;
>>>> +
>>>> +			status = "disabled";
>>>
>>> There is no reason this should be disabled by default. No external (per
>>> board) configuration is needed and by default SoC components should be
>>> enabled if they do not need anything from the board.
>>
>> This node is one of the nodes for which driver support is not yet in
>> place so that's one reason for at least disabling it temporarily.
>>
>> Since all other pinconfig lives in the board dts, if we decide to move
>> also the sound pinconfig then then that may be a second.
> 
> Also note that these depend on q6prmcc which is in turn depends on
> remoteproc_adsp being enabled by the x13s dts.
> 
> So keeping them disabled by default seems justified.
This I agree with, having sound-related nodes enabled by default
results in a big chunk of "deferred probe pending" spam when you
boot with adsp disabled (for example when you don't have fw for
your board in your rootfs)..

Konrad
> 
> Johan