Re: [PATCH 1/2] ALSA: hda: cs35l41: Add fixups for machines without a valid ACPI _DSD

From: Luke Jones
Date: Fri Jul 14 2023 - 04:59:25 EST



On Fri, Jul 14 2023 at 00:29:54 +08:00:00, David Xu <xuwd1@xxxxxxxxxxx> wrote:
As the comments added in commit 4d4c4bff4f8ed79d95e05 ("ALSA: hda:
cs35l41: Clarify support for CSC3551 without _DSD Properties"), CSC3551
requires a valid _DSD to work and the current implementation just
fails when no _DSD can be found for CSC3551. However it is a fact
that many OEMs hardcoded the configurations needed by CSC3551 into their
proprietary software for various 2022 and later laptop models,
and this makes the Linux installations on these models cannot make
any speaker sound. Meanwhile, at this point of time, we see no hope
that these OEMs would ever fix this problem via a BIOS update. So
to fix this bothering problem it might be worthwhile to add some
model-specific fixups to apply some proper configurations
to the cs35l41.

To address the above problem, a configuration fixup function
apply_cs35l41_fixup_cfg that would be called in cs35l41_no_acpi_dsd,
along with a fixup table cs35l41_fixup_cfgtbl which is a array of
fixup entry struct cs35l41_fixup_cfg are introduced. Each fixup entry
records the ACPI _SUB(vender and device ID) to be matched, and a
configuration to be applied to each of the cs35l41 device in CSC3551.

More specifically for the design of this fixup mechanism,
the maximum number of cs35l41 configurations inside a fixup entry
is defined as a macro CS35L41_FIXUP_CFG_MAX_DEVICES, and the actual
number of cs35l41 devices in a CSC3551 system is recorded in the
num_device field in the fixup entry. The apply_cs35l41_fixup_cfg
function is responsible for finding and applying a fixup for a
specific model according to the model's ACPI _SUB. If no valid fixup
can be applied, the fixup function fails and returns to the normal
cs35l41_no_acpi_dsd execution flow.

This patch now contains only several fixups for three Lenovo laptop
models, namely 16IAH7, 16IAX7, and 16ARHA7 and this fixup mechanism
has been verified to work on 16IAH7. As far as is known, several other
laptop models from ASUS and HP also suffer from this no valid _DSD
problem and could have it addressed with this fixup mechanism when
proper fixup entries are inserted.

Signed-off-by: David Xu <xuwd1@xxxxxxxxxxx>
---
sound/pci/hda/cs35l41_hda.c | 160 ++++++++++++++++++++++++++++++++++++
1 file changed, 160 insertions(+)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index ce5faa620517..d957458dd4e6 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -1211,6 +1211,159 @@ static int cs35l41_get_speaker_id(struct device *dev, int amp_index,
return speaker_id;
}

+#define CS35L41_FIXUP_CFG_MAX_DEVICES 4
+
+struct cs35l41_fixup_cfg {
+ unsigned short vender;
+ unsigned short device;
+ unsigned int num_device; /* The num of cs35l41 instances */
+ /* cs35l41 instance ids, can be i2c index or spi index */
+ int ids[CS35L41_FIXUP_CFG_MAX_DEVICES];
+ unsigned int reset_gpio_idx[CS35L41_FIXUP_CFG_MAX_DEVICES];
+ enum gpiod_flags reset_gpio_flags[CS35L41_FIXUP_CFG_MAX_DEVICES];
+ int spkid_gpio_idx[CS35L41_FIXUP_CFG_MAX_DEVICES];
+ unsigned int spk_pos[CS35L41_FIXUP_CFG_MAX_DEVICES];
+ enum cs35l41_hda_gpio_function gpio1_func[CS35L41_FIXUP_CFG_MAX_DEVICES];
+ enum cs35l41_hda_gpio_function gpio2_func[CS35L41_FIXUP_CFG_MAX_DEVICES];
+ enum cs35l41_boost_type bst_type[CS35L41_FIXUP_CFG_MAX_DEVICES];
+};
+
+static const struct cs35l41_fixup_cfg cs35l41_fixup_cfgtbl[] = {
+ { // Lenovo Legion Slim 7i 16IAH7
+ .vender = 0x17aa,
+ .device = 0x386e,
+ .num_device = 2,
+ .ids = {0x40, 0x41},
+ .reset_gpio_idx = {0, 0},
+ .reset_gpio_flags = {GPIOD_OUT_LOW, GPIOD_OUT_LOW},
+ .spkid_gpio_idx = {1, 1},
+ .spk_pos = {0, 1},
+ .gpio1_func = {CS35l41_VSPK_SWITCH, CS35l41_VSPK_SWITCH},
+ .gpio2_func = {CS35L41_INTERRUPT, CS35L41_NOT_USED},
+ .bst_type = {CS35L41_EXT_BOOST, CS35L41_EXT_BOOST}
+ },
+ { // Lenovo Legion Slim 7i 16IAH7 type2
+ .vender = 0x17aa,
+ .device = 0x3803,
+ .num_device = 2,
+ .ids = {0x40, 0x41},
+ .reset_gpio_idx = {0, 0},
+ .reset_gpio_flags = {GPIOD_OUT_LOW, GPIOD_OUT_LOW},
+ .spkid_gpio_idx = {1, 1},
+ .spk_pos = {0, 1},
+ .gpio1_func = {CS35l41_VSPK_SWITCH, CS35l41_VSPK_SWITCH},
+ .gpio2_func = {CS35L41_INTERRUPT, CS35L41_NOT_USED},
+ .bst_type = {CS35L41_EXT_BOOST, CS35L41_EXT_BOOST}
+ },
+ { // Lenovo Legion 7i 16IAX7
+ .vender = 0x17aa,
+ .device = 0x3874,
+ .num_device = 2,
+ .ids = {0x40, 0x41},
+ .reset_gpio_idx = {0, 0},
+ .reset_gpio_flags = {GPIOD_OUT_LOW, GPIOD_OUT_LOW},
+ .spkid_gpio_idx = {1, 1},
+ .spk_pos = {0, 1},
+ .gpio1_func = {CS35l41_VSPK_SWITCH, CS35l41_VSPK_SWITCH},
+ .gpio2_func = {CS35L41_INTERRUPT, CS35L41_INTERRUPT},
+ .bst_type = {CS35L41_EXT_BOOST, CS35L41_EXT_BOOST}
+ },
+ { // Lenovo Legion 7i 16IAX7 type 2
+ .vender = 0x17aa,
+ .device = 0x386f,
+ .num_device = 2,
+ .ids = {0x40, 0x41},
+ .reset_gpio_idx = {0, 0},
+ .reset_gpio_flags = {GPIOD_OUT_LOW, GPIOD_OUT_LOW},
+ .spkid_gpio_idx = {1, 1},
+ .spk_pos = {0, 1},
+ .gpio1_func = {CS35l41_VSPK_SWITCH, CS35l41_VSPK_SWITCH},
+ .gpio2_func = {CS35L41_INTERRUPT, CS35L41_INTERRUPT},
+ .bst_type = {CS35L41_EXT_BOOST, CS35L41_EXT_BOOST}
+ },
+ { // Lenovo Legion Slim 7 16ARHA7
+ .vender = 0x17aa,
+ .device = 0x3877,
+ .num_device = 2,
+ .ids = {0x40, 0x41},
+ .reset_gpio_idx = {0, 0},
+ .reset_gpio_flags = {GPIOD_OUT_LOW, GPIOD_OUT_LOW},
+ .spkid_gpio_idx = {1, 1},
+ .spk_pos = {0, 1},
+ .gpio1_func = {CS35l41_VSPK_SWITCH, CS35l41_VSPK_SWITCH},
+ .gpio2_func = {CS35L41_INTERRUPT, CS35L41_INTERRUPT},
+ .bst_type = {CS35L41_EXT_BOOST, CS35L41_EXT_BOOST}
+ },
+ {} // terminator
+};
+
+static inline int cs35l41_fixup_get_index(const struct cs35l41_fixup_cfg *fixup, int cs35l41_addr)
+{
+ int i;
+
+ for (i = 0; i < fixup->num_device; i++) {
+ if (fixup->ids[i] == cs35l41_addr)
+ return i;
+ }
+ return -ENODEV;
+}
+
+static int apply_cs35l41_fixup_cfg(struct cs35l41_hda *cs35l41,
+ struct device *physdev,
+ int cs35l41_addr,
+ const struct cs35l41_fixup_cfg *fixup_tbl)
+{
+ const char *ssid;
+ unsigned int vendid;
+ unsigned int devid;
+ const struct cs35l41_fixup_cfg *cur_fixup;
+ struct cs35l41_hw_cfg *hw_cfg;
+ int cs35l41_index;
+ int ret;
+ int i;
+
+ ssid = cs35l41->acpi_subsystem_id;
+ ret = sscanf(ssid, "%04x%04x", &vendid, &devid);
+ if (ret != 2)
+ return -EINVAL;
+
+ hw_cfg = &cs35l41->hw_cfg;
+ for (cur_fixup = fixup_tbl; cur_fixup->vender; cur_fixup++) {
+ if (cur_fixup->vender == vendid && cur_fixup->device == devid) {
+ cs35l41_index = cs35l41_fixup_get_index(cur_fixup, cs35l41_addr);
+ if (cs35l41_index == -ENODEV)
+ return -ENODEV;
+ cs35l41->index = cs35l41_index;
+ cs35l41->reset_gpio = gpiod_get_index(
+ physdev,
+ NULL,
+ cur_fixup->reset_gpio_idx[cs35l41_index],
+ cur_fixup->reset_gpio_flags[cs35l41_index]
+ );
+ cs35l41->speaker_id = cs35l41_get_speaker_id(physdev,
+ cs35l41_index,
+ cur_fixup->num_device,
+ cur_fixup->spkid_gpio_idx[cs35l41_index]
+ );
+ hw_cfg->spk_pos = cur_fixup->spk_pos[cs35l41_index];
+ cs35l41->channel_index = 0;
+ for (i = 0; i < cs35l41->index; i++)
+ if (cur_fixup->spk_pos[i] == hw_cfg->spk_pos)
+ cs35l41->channel_index++;
+
+ hw_cfg->gpio1.func = cur_fixup->gpio1_func[cs35l41_index];
+ hw_cfg->gpio1.valid = true;
+ hw_cfg->gpio2.func = cur_fixup->gpio2_func[cs35l41_index];
+ hw_cfg->gpio2.valid = true;
+ hw_cfg->bst_type = cur_fixup->bst_type[cs35l41_index];
+ dev_dbg(physdev, "Fixup applied.\n");
+ break;
+ }
+ }
+ return 0;
+
+}
+
/*
* Device CLSA010(0/1) doesn't have _DSD so a gpiod_get by the label reset won't work.
* And devices created by serial-multi-instantiate don't have their device struct
@@ -1221,6 +1374,7 @@ static int cs35l41_get_speaker_id(struct device *dev, int amp_index,
static int cs35l41_no_acpi_dsd(struct cs35l41_hda *cs35l41, struct device *physdev, int id,
const char *hid)
{
+ int ret;

This ret is unused.

struct cs35l41_hw_cfg *hw_cfg = &cs35l41->hw_cfg;

/* check I2C address to assign the index */
@@ -1243,7 +1397,13 @@ static int cs35l41_no_acpi_dsd(struct cs35l41_hda *cs35l41, struct device *physd
/*
* Note: CLSA010(0/1) are special cases which use a slightly different design.
* All other HIDs e.g. CSC3551 require valid ACPI _DSD properties to be supported.
+ * However many OEMs hardcoded the configurations into their proprietary software
+ * thus leaving our Linux installation with no speaker sound at all while we see
+ * no hope those OEMs would fix it. So we apply a ssid specific fixup to fix it.
*/
+ if (apply_cs35l41_fixup_cfg(cs35l41, physdev, id, cs35l41_fixup_cfgtbl) == 0)
+ return 0;
+
dev_err(cs35l41->dev, "Error: ACPI _DSD Properties are missing for HID %s.\n", hid);
hw_cfg->valid = false;
hw_cfg->gpio1.valid = false;
--
2.41.0


Hi David,

Thank you for working on this, it is something I was considering doing myself as there are approximately 56 laptops in the ASUS range, not counting the variants, which need similar. Some I2C connected, some SPI connected. There is zero chance all the vendors will take action to include the correct entries in DSD.

I have tried your patch on an SPI connected Strix G634. As there is a lack of detail on some things I am unsure if I got the setup correct. And regardless of that there are issues - I don't think I could confirm if this worked due to those.

I tried with the patch applied and what I thought was correct setup (in driver):
1. without my DSD table additions (in acpi)
2. With the CS part added (in acpi)
3. With my full DSD table added (in acpi)

all 3 instances failed with:

cs35l41-hda spi1-CSC3551:00-cs35l41-hda.0: OTP Boot status 80000000 error: 0
cs35l41-hda: probe of spi1-CSC3551:00-cs35l41-hda.0 failed with error -5

The driver table entry I used was:

+ { // ROG Strix Scar
+ .vender = 0x1043,
+ .device = 0x1caf,
+ .num_device = 2,
+ .ids = {0x00, 0x01},
+ .reset_gpio_idx = {0, 0},
+ .reset_gpio_flags = {GPIOD_OUT_LOW, GPIOD_OUT_LOW},
+ .spkid_gpio_idx = {1, 1},
+ .spk_pos = {0, 1},
+ .gpio1_func = {CS35l41_VSPK_SWITCH, CS35l41_VSPK_SWITCH},
+ .gpio2_func = {CS35L41_INTERRUPT, CS35L41_INTERRUPT},
+ .bst_type = {CS35L41_EXT_BOOST, CS35L41_EXT_BOOST}
+ },

Compared to my ACPI patch:

DefinitionBlock ("", "SSDT", 1, "CUSTOM", "CSC3551", 0x00000001)
{
External (_SB_.PC00.SPI3, DeviceObj)
External (_SB_.PC00.SPI3.SPK1, DeviceObj)

Scope (_SB.PC00.SPI3.SPK1)
{
Name (_DSD, Package ()
{
ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package ()
{
Package () { "cirrus,dev-index", Package () { Zero, One }},
Package () { "reset-gpios", Package () {
SPK1, One, Zero, Zero,
SPK1, One, Zero, Zero
} },
Package () { "spk-id-gpios", Package () {
SPK1, 0x02, Zero, Zero,
SPK1, 0x02, Zero, Zero
} },
Package () { "cirrus,speaker-position", Package () { Zero, One } },
// gpioX-func: 0 not used, 1 VPSK_SWITCH, 2: INTERRUPT, 3: SYNC
Package () { "cirrus,gpio1-func", Package () { One, One } },
Package () { "cirrus,gpio2-func", Package () { 0x02, 0x02 } },
// boost-type: 0 internal, 1 external
Package () { "cirrus,boost-type", Package () { Zero, Zero } }
}
})
}

Scope (_SB.PC00.SPI3)
{
Name (_DSD, Package ()
{
ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package ()
{
Package () { "cs-gpios", Package () {
Zero, // Native CS
SPK1, Zero, Zero, Zero // GPIO CS
} }
}
})
}
}


The key thing I am concerned about is how the values for the following are gained:
+ .reset_gpio_idx = {0, 0},
+ .reset_gpio_flags = {GPIOD_OUT_LOW, GPIOD_OUT_LOW},
+ .spkid_gpio_idx = {1, 1},

there wasn't much detail provided on this, and I tried multiple variations, all ending up with the same errors as before. I will do a debug build and get some more info.



Some previous conversation: https://lore.kernel.org/all/1991650.PYKUYFuaPT@fedora/