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.. > 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. > 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... > > An alternate solution would be to break the ACPI and OF slave initialization > into two separate files (slave-acpi.c and slave-of.c), that way there is a > cleaner split. > > Or we put all those quirks in a dedicated slave-dmi-quirks.c and use your > solution. That may be more manageable since the list of quirks is > historically likely to grow. > > It's really ugly in all cases. > > I try to look at the positive side of things: if we have quirks to handle > it's an indicator that more platforms are moving to SoundWire... > > I hope though that it doesn't reach the level of Baytrail where most of the > machine driver is a dictionary of quirks. Hope that it wont, but having seen things, I think it may come to it! -- ~Vinod
