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.
