[PATCH 1/2] ALSA: hda: cs35l41: Add fixups for machines without a valid ACPI _DSD
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@hotmail.com --- 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; 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;
On Fri, Jul 14 2023 at 00:29:54 +08:00:00, David Xu xuwd1@hotmail.com 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@hotmail.com
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 theirproprietary software
* thus leaving our Linux installation with no speaker sound atall while we see
* no hope those OEMs would fix it. So we apply a ssid specificfixup 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/
On Fri, Jul 14 2023 at 20:58:44 +12:00:00, Luke Jones luke@ljones.dev wrote:
On Fri, Jul 14 2023 at 00:29:54 +08:00:00, David Xu xuwd1@hotmail.com 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@hotmail.com
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 theirproprietary software
* thus leaving our Linux installation with no speaker sound atall while we see
* no hope those OEMs would fix it. So we apply a ssid specificfixup 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):
- without my DSD table additions (in acpi)
- With the CS part added (in acpi)
- 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/
With and without the chip select ssdt patch for SPI I get:
Serial bus multi instantiate pseudo device driver CSC3551:00: Found Fixed Speaker ID GPIO (index = 2) Serial bus multi instantiate pseudo device driver CSC3551:00: Speaker ID = 0 Serial bus multi instantiate pseudo device driver CSC3551:00: Found Fixed Speaker ID GPIO (index = 1) Serial bus multi instantiate pseudo device driver CSC3551:00: Speaker ID = 0 Serial bus multi instantiate pseudo device driver CSC3551:00: Fixup applied. cs35l41-hda spi1-CSC3551:00-cs35l41-hda.1: Reset line busy, assuming shared reset cs35l41-hda spi1-CSC3551:00-cs35l41-hda.1: OTP Boot status 80000000 error: 0 cs35l41-hda: probe of spi1-CSC3551:00-cs35l41-hda.1 failed with error -5
Hi Luke,
Thank you for replying.
I tried with the patch applied and what I thought was correct setup (in driver):
- without my DSD table additions (in acpi)
- With the CS part added (in acpi)
- 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
I see that you have some problem on making a working fixup table entry, with your key concern being the three fields you pointed out:
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},
As I could see you have written an incorrect fixup entry when compared with the ACPI patch you provided:
1. For the .reset_gpio_idx field, from your ACPI patch:
Package () { "reset-gpios", Package () { SPK1, One, Zero, Zero, SPK1, One, Zero, Zero } },
for your machine, the index of the reset GpioIo resource is 1, which means the correct form would be .reset_gpio_idx = {1, 1}. You could find this index by looking for the GpioIo resource descriptor that has a "IoRestrictionOutputOnly" IORestriction argument in your ACPI CSC3551 _CRS method, and I suppose this reset gpio should has the only GpioIo descriptor with this "IoRestrictionOutputOnly" argument. Besides, having taking this into consideration, the error you get is probably due to a incorrect reset.
2. For the value of .reset_gpio_flags, it needs more or less some guessing: it's common for chips to have their reset to be pin active low and this polarity can also be checked by reading the gpio resource buffer with kernel acpica infrastructures. Anyway, I think for most cases, .reset_gpio_flags = {GPIOD_OUT_LOW, GPIOD_OUT_LOW} is good.
3. For the value of .spkid_gpio_idx, by investigating the CSC3551 ACPI _CRS method and the driver code, my shallow thought is that it has 3 pins, with one being the reset pin which we have dealt with, one being the interrupt pin, which usually is declared with "GpioInt" if the pin is connected to PCH GPIO and is considered to be one of the cs35l41's configurable GPIO, and another one configurable GPIO whose function is usually assigned with "VSPK_SWITCH". For the spkid to be honest I still have not figured out how it was designed. I suspect that the spkid gpio is acually a reused funcion of one of the three gpios I mentioned. From the driver implementation, the .spkid_gpio_idx field should be the index of the GpioIo resource descriptor in the _CRS returned buffer. However also from the implementation of the driver, whether this field is configured correctly should not be the major factor affecting if the speakers would work, it only affects the selection of the cs35l41 firmware. Anyway, from your working ACPI patch:
Package () { "spk-id-gpios", Package () { SPK1, 0x02, Zero, Zero, SPK1, 0x02, Zero, Zero } },
you may try setting .spkid_gpio_idx = {2, 2}. If that still does not work you could try setting .spkid_gpio_idx = {-1, -1}, and this would make the driver try to find the "spk-id-gpios" package from the ACPI table. (however even though you did not provide this by patching the ACPI table and it fails to get it, the driver still should not fail.)
Please try to modify the fixup entry as the advice above to see if it works. If still not, feel free to futher reply me, and I think it would be very beneficial for me to help solving your problem if you could post your orignial CSC3551 ACPI table.
participants (2)
-
David Xu -
Luke Jones