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