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.