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

[Johan Hovold]

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