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@xxxxxxxxxx>
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@xxxxxxxxxx>
---
-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);
