| Message ID | 20230102105038.8074-2-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: qcom: disable x13s sound + cleanups | expand |
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
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
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
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
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
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
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
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
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
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
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 {
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(-)