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

[Krzysztof Kozlowski]

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