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

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.

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.

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.

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.

-- >8 --

From 20af8100025637ea5e295877d28f3afb9dbd4814 Mon Sep 17 00:00:00 2001 From: Vinod Koul vkoul@kernel.org Date: Fri, 5 Feb 2021 12:38:21 +0530 Subject: [PATCH] soundwire: add override addr ops

Some firmware can have buggy _ADR values causing the scanning of devices to fail. Add the override_ops which if implemented by master driver would be invoked instead of reading _ADR. The drivers can implement quirks based on DMI etc to override the addr values.

Signed-off-by: Vinod Koul vkoul@kernel.org

-compile tested only, pls validate

drivers/soundwire/slave.c | 25 +++++++++++++++++++++---- include/linux/soundwire/sdw.h | 3 +++ 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c index a08f4081c1c4..2b0d646c5c2f 100644 --- a/drivers/soundwire/slave.c +++ b/drivers/soundwire/slave.c @@ -91,12 +91,11 @@ int sdw_slave_add(struct sdw_bus *bus,

#if IS_ENABLED(CONFIG_ACPI)

-static bool find_slave(struct sdw_bus *bus,

•        struct acpi_device *adev,
  

•        struct sdw_slave_id *id)
  

+static bool _find_slave(struct sdw_bus *bus,

•         struct acpi_device *adev,
  

•         struct sdw_slave_id *id)
  

  { unsigned long long addr;

• unsigned int link_id; acpi_status status;

  status = acpi_evaluate_integer(adev->handle,

@@ -108,6 +107,24 @@ static bool find_slave(struct sdw_bus *bus, return false; }

• return addr;

+}

+static bool find_slave(struct sdw_bus *bus,

•        struct acpi_device *adev,
  

•        struct sdw_slave_id *id)
  

+{

• unsigned long long addr;
• unsigned int link_id;
• if (bus->ops->override_adr)

• addr = bus->ops->override_adr(bus);
  

• else

• addr = _find_slave(bus, adev, id);
  

• if (!addr)

• return false;
  

• /* Extract link id from ADR, Bit 51 to 48 (included) */ link_id = SDW_DISCO_LINK_ID(addr);

diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index f0b01b728640..d0ad3404ca95 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -804,6 +804,8 @@ struct sdw_defer { /**

• struct sdw_master_ops - Master driver ops
• @read_prop: Read Master properties

• • @override_adr: Override reading address from firmware and read from
• • driver instead (quirk for buggy firmware)
  • @xfer_msg: Transfer message callback
  • @xfer_msg_defer: Defer version of transfer message callback
  • @reset_page_addr: Reset the SCP page address registers

@@ -813,6 +815,7 @@ struct sdw_defer { */ struct sdw_master_ops { int (*read_prop)(struct sdw_bus *bus);

• unsigned long long (*override_adr)(struct sdw_bus *bus);

  enum sdw_command_response (*xfer_msg) (struct sdw_bus *bus, struct sdw_msg *msg);