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 <[email protected]>
---
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;
--
2.41.0
On Fri, Jul 14 2023 at 00:29:54 +08:00:00, David Xu <[email protected]>
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 <[email protected]>
> ---
> 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/
On Fri, Jul 14 2023 at 20:58:44 +12:00:00, Luke Jones <[email protected]>
wrote:
>
>
> On Fri, Jul 14 2023 at 00:29:54 +08:00:00, David Xu
> <[email protected]> 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 <[email protected]>
>> ---
>> 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/
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):
> 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
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.