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

[Johan Hovold <johan@kernel.org>]

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