arm64: dts: qcom: disable x13s sound + cleanups

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 <[email protected]>

---
 .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts  | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +7 -1

index 0201c6776746..97ff74d5095e 100644
@@ -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";

Quoted 53 lines · 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 <[email protected]>

---
 .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts  | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +7 -1

index 0201c6776746..97ff74d5095e 100644
@@ -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";

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts wcd_rx: wcd9380-rx@0,4 { · line 776

Quoted 5 lines

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +1 -1

@@ -767,7 +773,7 @@ wcd_rx: wcd9380-rx@0,4 {
 };
 
 &swr2 {
-	status = "okay";
+	status = "disabled";

That's a double disable.

Quoted 1 line

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts &vamacro { · line 791

Quoted 8 lines

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +2 -0

 	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.

Quoted 63 lines · Krzysztof Kozlowski

Quoted 53 lines · 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 <[email protected]>

---
 .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts  | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +7 -1

index 0201c6776746..97ff74d5095e 100644
@@ -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";

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts wcd_rx: wcd9380-rx@0,4 { · line 776

Quoted 5 lines

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +1 -1

@@ -767,7 +773,7 @@ wcd_rx: wcd9380-rx@0,4 {
 };
 
 &swr2 {
-	status = "okay";
+	status = "disabled";

That's a double disable.

Quoted 1 line

Quoted 10 lines

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts &vamacro { · line 791

Quoted 8 lines

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +2 -0

 	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.

Quoted 75 lines · Johan Hovold

Quoted 73 lines · Krzysztof Kozlowski

Quoted 53 lines · 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 <[email protected]>

---
 .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts  | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +7 -1

index 0201c6776746..97ff74d5095e 100644
@@ -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";

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts wcd_rx: wcd9380-rx@0,4 { · line 776

Quoted 5 lines

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +1 -1

@@ -767,7 +773,7 @@ wcd_rx: wcd9380-rx@0,4 {
 };
 
 &swr2 {
-	status = "okay";
+	status = "disabled";

That's a double disable.

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts &vamacro { · line 791

Quoted 9 lines

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +2 -0

 	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.

Quoted 2 lines

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.

Quoted 2 lines

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?

Quoted 79 lines · Krzysztof Kozlowski

Quoted 77 lines · Johan Hovold

Quoted 63 lines · Krzysztof Kozlowski

Quoted 53 lines · 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 <[email protected]>

---
 .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts  | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +7 -1

index 0201c6776746..97ff74d5095e 100644
@@ -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";

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts wcd_rx: wcd9380-rx@0,4 { · line 776

Quoted 5 lines

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +1 -1

@@ -767,7 +773,7 @@ wcd_rx: wcd9380-rx@0,4 {
 };
 
 &swr2 {
-	status = "okay";
+	status = "disabled";

That's a double disable.

Quoted 1 line

Quoted 10 lines

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts &vamacro { · line 791

Quoted 8 lines

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +2 -0

 	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.

Quoted 3 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 34 lines · Johan Hovold

Quoted 32 lines · Krzysztof Kozlowski

Quoted 25 lines · Johan Hovold

Quoted 13 lines · Krzysztof Kozlowski

Quoted 11 lines · 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 <[email protected]>

---
 .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts  | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Quoted 10 lines

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts &vamacro { · line 791

Quoted 8 lines

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +2 -0

 	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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 11 lines

Quoted 6 lines

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 21 lines

Quoted 20 lines

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Review &vamacro { · line 791

Quoted 8 lines

(quoted diff) +2 -0

 	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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 12 lines

Quoted 12 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 25 lines · Johan Hovold

Quoted 24 lines

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Quoted 10 lines

Review &vamacro { · line 791

Quoted 8 lines

(quoted diff) +2 -0

 	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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 23 lines

Quoted 11 lines

Quoted 11 lines

Quoted 6 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 3 lines

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".

Quoted 1 line

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.

Quoted 6 lines

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.

Quoted 29 lines · Krzysztof Kozlowski

Quoted 27 lines · Johan Hovold

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

Quoted 11 lines

Quoted 10 lines

Review &vamacro { · line 791

Quoted 8 lines

(quoted diff) +2 -0

 	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.

Quoted 2 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 29 lines

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 1 line

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.

Quoted 8 lines

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 35 lines · Johan Hovold

Quoted 33 lines · Krzysztof Kozlowski

Quoted 21 lines · Johan Hovold

Quoted 21 lines

Quoted 20 lines

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Review &vamacro { · line 791

Quoted 8 lines

(quoted diff) +2 -0

 	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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 2 lines

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?

Quoted 28 lines

Quoted 28 lines

Quoted 20 lines

Quoted 11 lines

Quoted 11 lines

Quoted 6 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 8 lines

Quoted 1 line

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.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

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

Signed-off-by Johan Hovold <[email protected]>

---

 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

arch/arm64/boot/dts/qcom/sc8280xp.dtsi +16 -0

index ed1e2bee86ee..c1ce2d7b3675 100644
@@ -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";

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

Signed-off-by Johan Hovold <[email protected]>

---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

arch/arm64/boot/dts/qcom/sc8280xp.dtsi +0 -0

index ed1e2bee86ee..c1ce2d7b3675 100644

arch/arm64/boot/dts/qcom/sc8280xp.dtsi +8 -0

 		};
 
 		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 {

Quoted 13 lines · Johan Hovold

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

Signed-off-by Johan Hovold <[email protected]>

---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

arch/arm64/boot/dts/qcom/sc8280xp.dtsi +0 -0

index ed1e2bee86ee..c1ce2d7b3675 100644
@@ -1733,6 +1733,8 @@ rxmacro: rxmacro@3200000 {
 

arch/arm64/boot/dts/qcom/sc8280xp.dtsi rxmacro: rxmacro@3200000 { · line 1737

Quoted 4 lines

arch/arm64/boot/dts/qcom/sc8280xp.dtsi +2 -0

 			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.

Quoted 14 lines · Krzysztof Kozlowski

Quoted 13 lines · Johan Hovold

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

Signed-off-by Johan Hovold <[email protected]>

---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

arch/arm64/boot/dts/qcom/sc8280xp.dtsi +0 -0

index ed1e2bee86ee..c1ce2d7b3675 100644
@@ -1733,6 +1733,8 @@ rxmacro: rxmacro@3200000 {
 

Quoted 8 lines

arch/arm64/boot/dts/qcom/sc8280xp.dtsi rxmacro: rxmacro@3200000 { · line 1737

Quoted 4 lines

arch/arm64/boot/dts/qcom/sc8280xp.dtsi +2 -0

 			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.

Quoted 27 lines · Johan Hovold

Quoted 14 lines · Krzysztof Kozlowski

Quoted 13 lines · Johan Hovold

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

Signed-off-by Johan Hovold <[email protected]>

---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

arch/arm64/boot/dts/qcom/sc8280xp.dtsi +0 -0

index ed1e2bee86ee..c1ce2d7b3675 100644
@@ -1733,6 +1733,8 @@ rxmacro: rxmacro@3200000 {
 

Quoted 8 lines

arch/arm64/boot/dts/qcom/sc8280xp.dtsi rxmacro: rxmacro@3200000 { · line 1737

Quoted 4 lines

arch/arm64/boot/dts/qcom/sc8280xp.dtsi +2 -0

 			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.

Quoted 2 lines

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

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 <[email protected]>

---

 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 2 --
 1 file changed, 2 deletions(-)

arch/arm64/boot/dts/qcom/sc8280xp.dtsi +0 -2

index c1ce2d7b3675..11e3d560c627 100644
@@ -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";

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 <[email protected]>

---

 .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts    | 54 ++++++++++---------
 1 file changed, 28 insertions(+), 26 deletions(-)

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +28 -26

index 97ff74d5095e..ae67c11bfc22 100644
@@ -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";

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 <[email protected]>

---

 arch/arm64/boot/dts/qcom/sm8250-mtp.dts | 40 ++++++++++++-------------
 1 file changed, 19 insertions(+), 21 deletions(-)

arch/arm64/boot/dts/qcom/sm8250-mtp.dts +19 -21

index 3ed8c84e25b8..ea43dbdc3671 100644
@@ -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";

Quoted 3 lines · Johan Hovold

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

Quoted 4 lines

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

Signed-off-by Johan Hovold <[email protected]>

---

Could you rename it to audio-codec and separate/sort the properties like you did for x13s?

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 <[email protected]>

---
 arch/arm64/boot/dts/qcom/sm8250-mtp.dts | 40 ++++++++++++-------------
 1 file changed, 19 insertions(+), 21 deletions(-)

Quoted 5 lines · 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.

Quoted 4 lines

Signed-off-by Johan Hovold <[email protected]>

---
 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...

Quoted 11 lines · Krzysztof Kozlowski

Quoted 9 lines · 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 <[email protected]>

---
 arch/arm64/boot/dts/qcom/sm8250-mtp.dts | 40 ++++++++++++-------------
 1 file changed, 19 insertions(+), 21 deletions(-)

Quoted 2 lines

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.

Quoted 15 lines · Johan Hovold

Quoted 13 lines · Krzysztof Kozlowski

Quoted 9 lines · 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 <[email protected]>

---
 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...

Quoted 2 lines

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.

Quoted 16 lines · Krzysztof Kozlowski

Quoted 15 lines · Johan Hovold

Quoted 13 lines · Krzysztof Kozlowski

Quoted 9 lines · 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 <[email protected]>

---
 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...

Quoted 6 lines

Quoted 2 lines

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.

Quoted 24 lines · Johan Hovold

Quoted 22 lines · Krzysztof Kozlowski

Quoted 12 lines · Johan Hovold

Quoted 11 lines · Krzysztof Kozlowski

Quoted 9 lines · 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 <[email protected]>

---
 arch/arm64/boot/dts/qcom/sm8250-mtp.dts | 40 ++++++++++++-------------
 1 file changed, 19 insertions(+), 21 deletions(-)

Quoted 5 lines

Quoted 2 lines

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.

Quoted 1 line

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

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 <[email protected]>

---

 arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 52 ++++++++++++-------------
 1 file changed, 25 insertions(+), 27 deletions(-)

arch/arm64/boot/dts/qcom/sm8450-hdk.dts +25 -27

index 4de3e1f1c39c..217b2c654745 100644
@@ -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";

Quoted 3 lines · Johan Hovold

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

Quoted 4 lines

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

Signed-off-by Johan Hovold <[email protected]>

---

Third patch concerning the audio codec and third different order of properties :/ Please consolidate that.

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 <[email protected]>

---
 arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 52 ++++++++++++-------------
 1 file changed, 25 insertions(+), 27 deletions(-)

arch/arm64/boot/dts/qcom/sm8450-hdk.dts +0 -0

index 4de3e1f1c39c..217b2c654745 100644

Quoted 18 lines · 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 <[email protected]>

---
 arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 52 ++++++++++++-------------
 1 file changed, 25 insertions(+), 27 deletions(-)

arch/arm64/boot/dts/qcom/sm8450-hdk.dts +0 -0

index 4de3e1f1c39c..217b2c654745 100644
@@ -17,6 +17,31 @@ aliases {
 		serial0 = &uart7;
 	};
 

arch/arm64/boot/dts/qcom/sm8450-hdk.dts aliases { · line 20

Quoted 1 line

arch/arm64/boot/dts/qcom/sm8450-hdk.dts +1 -0

+	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.

Quoted 19 lines · Krzysztof Kozlowski

Quoted 18 lines · 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 <[email protected]>

---
 arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 52 ++++++++++++-------------
 1 file changed, 25 insertions(+), 27 deletions(-)

arch/arm64/boot/dts/qcom/sm8450-hdk.dts +0 -0

index 4de3e1f1c39c..217b2c654745 100644
@@ -17,6 +17,31 @@ aliases {
 		serial0 = &uart7;
 	};
 

Quoted 5 lines

arch/arm64/boot/dts/qcom/sm8450-hdk.dts aliases { · line 20

Quoted 1 line

arch/arm64/boot/dts/qcom/sm8450-hdk.dts +1 -0

+	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.

Quoted 26 lines · Krzysztof Kozlowski

Quoted 24 lines · Krzysztof Kozlowski

Quoted 15 lines · 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 <[email protected]>

---
 arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 52 ++++++++++++-------------
 1 file changed, 25 insertions(+), 27 deletions(-)

arch/arm64/boot/dts/qcom/sm8450-hdk.dts +0 -0

index 4de3e1f1c39c..217b2c654745 100644

arch/arm64/boot/dts/qcom/sm8450-hdk.dts aliases { · line 20

Quoted 4 lines

arch/arm64/boot/dts/qcom/sm8450-hdk.dts +1 -0

@@ -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.

Quoted 2 lines

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.

Quoted 27 lines · Johan Hovold

Quoted 26 lines · Krzysztof Kozlowski

Quoted 24 lines · Krzysztof Kozlowski

Quoted 15 lines · 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 <[email protected]>

---
 arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 52 ++++++++++++-------------
 1 file changed, 25 insertions(+), 27 deletions(-)

arch/arm64/boot/dts/qcom/sm8450-hdk.dts +0 -0

index 4de3e1f1c39c..217b2c654745 100644

arch/arm64/boot/dts/qcom/sm8450-hdk.dts aliases { · line 20

Quoted 4 lines

arch/arm64/boot/dts/qcom/sm8450-hdk.dts +1 -0

@@ -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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 31 lines · Krzysztof Kozlowski

Quoted 30 lines · Johan Hovold

Quoted 28 lines · Krzysztof Kozlowski

Quoted 19 lines · Krzysztof Kozlowski

Quoted 18 lines · 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 <[email protected]>

---
 arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 52 ++++++++++++-------------
 1 file changed, 25 insertions(+), 27 deletions(-)

arch/arm64/boot/dts/qcom/sm8450-hdk.dts +0 -0

index 4de3e1f1c39c..217b2c654745 100644
@@ -17,6 +17,31 @@ aliases {
 		serial0 = &uart7;
 	};
 

Quoted 5 lines

arch/arm64/boot/dts/qcom/sm8450-hdk.dts aliases { · line 20

Quoted 1 line

arch/arm64/boot/dts/qcom/sm8450-hdk.dts +1 -0

+	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.

Quoted 4 lines

Quoted 2 lines

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.