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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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