[PATCH 1/2] soundwire: slave: introduce DMI quirks for HP Spectre x360 Convertible

[Pierre-Louis Bossart]

On 2/6/21 4:55 AM, Vinod Koul wrote:
> Hi Pierre,
> 
> On 05-02-21, 09:15, Pierre-Louis Bossart wrote:
>> Thanks for the review Vinod.
>>
>>> On 04-02-21, 14:48, Pierre-Louis Bossart wrote:
>>>> HP Spectre x360 Convertible devices expose invalid _ADR fields in the
>>>> DSDT, which prevents codec drivers from probing. A possible solution
>>>> is to override the DSDT, but that's just too painful for users.
>>>>
>>>> This patch suggests a simple DMI-based quirk to remap the existing
>>>> invalid ADR information into valid ones.
>>>
>>> While I agree with the approach, I do not like the implementation. The
>>> quirks in firmware should not reside in core code. Drivers are the right
>>> place, of course we need to add callbacks into driver for this.
>>>
>>> So I did a quick hacking and added the below patch, I think you can add
>>> the quirks in Intel driver based on DMI for this.
>>
>> I thought about this, but the Intel driver is about the *master*
>> configuration. It's not really about slave-related _ADR. If and when the IP
>> configuration changes, it'll be problematic to have such quirks in the
>> middle.
> 
> Okay, but ADR is not really a slave configuration either. I feel it is system
> wide description..

It's a partial description only useful for enumeration.

the _ADR field is the means by which we probe a slave driver, which 
happens when we add a device in slave.c. It doesn't carry any other 
information beyond enumeration. the only part you could view as 
system-dependent is the link_id and the unique_id, but that's far from a 
complete description.

More specifically, it doesn't tell you if the device is left or right, 
if it's meant to be used in an 'aggregated' way or if the different 
devices are independent.

In fact the entire existing DisCo specification only describes 'devices' 
(in the SoundWire sense), not how they are supposed to be connected and 
used in a system. We will have to come-up with a new spec for this so 
that we can have a true 'platform'/system description allowing us to use 
a generic machine driver (similar to Morimoto-san's generic graph)

> 
>> At the end of the day, the problem is an ACPI one, not an Intel master one,
>> and I put the code where it's protected by CONFIG_ACPI.
> 
> Right, to be more specific it is a buggy firmware implementation and not
> following of the specs by device vendors.

It's an OEM/BIOS vendor/integrator issue.

>> I don't mind doing the change, but the notion of conflating Intel master and
>> list of slave quirks isn't without its own problems.
> 
> Same is true for soundwire core (slave/slave-apci or whatever). The
> issue I have is that this will sure become big, so I would like this to
> be moved away from all soundwire core files. The right place IMO for
> this is respective drivers, intel/codec/machine please do take your
> pick. My attempt here was to move this to Intel driver here as I felt
> that was the right place for platform issues here.. do feel free to move
> any other place except core files...

thanks, I will combine your suggestion to add an 'override' callback but 
stick these quirks in a separate files. That will avoid collisions and 
make the code more manageable.