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

[Vinod Koul]

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