Re: [PATCH 1/1] ALSA: hda: cs35l41: Dell Fiorano add missing _DSD properties
From: Stuart Henderson
Date: Thu Dec 14 2023 - 10:40:20 EST
On 14/12/2023 11:02, Takashi Iwai wrote:
On Tue, 12 Dec 2023 20:52:43 +0100,
Aleksandrs Vinarskis wrote:
Dell XPS 9530 (2023) has two SPI connected CS35L41 amplifiers, however
is missing _DSD properties, cs-gpios and has a firmware bug which caps SPI
controller's speed to unusable 3051Hz. This patch adds _DSD properties and
sets second cs-gpio. In case SPI speed bug is detected, it will not
initialize the device to avoid hangs on wake up.
Resolution of SPI speed bug requires either a patch to `intel-lpss.c` or an
UEFI update with corrected values from Dell. Tested with locally applied
patch to `intel-lpss` on multiple XPS 9530 devices.
Co-developed-by: Jasper Smet <josbeir@xxxxxxxxx>
Signed-off-by: Jasper Smet <josbeir@xxxxxxxxx>
Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@xxxxxxxxx>
Can Cirrus team review this?
thanks,
Takashi
The patch looks sensible, with some minor comments below, however we're
just at the tail end of testing a patch chain that genericises a lot of
this code and adds support for a rather large batch of other laptops
with incomplete DSD. We're hoping to push this upstream on Monday.
Can I be awkward and ask that we hold off on this patch chain until
then? Then we can add this laptop using the new approach.
If/when the chain is accepted, we will add support for a few Dell
laptops as well, including this one.
---
sound/pci/hda/cs35l41_hda_property.c | 47 ++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
diff --git a/sound/pci/hda/cs35l41_hda_property.c b/sound/pci/hda/cs35l41_hda_property.c
index c83328971728..69446a794397 100644
--- a/sound/pci/hda/cs35l41_hda_property.c
+++ b/sound/pci/hda/cs35l41_hda_property.c
@@ -7,9 +7,55 @@
// Author: Stefan Binding <sbinding@xxxxxxxxxxxxxxxxxxxxx>
#include <linux/gpio/consumer.h>
+#include <linux/spi/spi.h>
#include <linux/string.h>
#include "cs35l41_hda_property.h"
+/*
+ * Device 10280BEB (Dell XPS 9530) doesn't have _DSD at all. Moreover, pin that is typically
+ * used for `speaker_id` is missing. SPI's cs-gpios definitions are also missing.
+ */
+static int dell_fiorano_no_acpi(struct cs35l41_hda *cs35l41, struct device *physdev, int id,
+ const char *hid)
+{
+ struct cs35l41_hw_cfg *hw_cfg = &cs35l41->hw_cfg;
+ struct spi_device *spi = to_spi_device(cs35l41->dev);
+
+ /*
+ * 10280BEB has a firmware bug, which wrongly enables clock divider for intel-lpss
+ * Resultant SPI clock is 100Mhz/32767=3051Hz, which leads to ~3 minute hang on boot/wake up
+ * Avoid initializing device if lpss was not patched/fixed UEFI was not installed
+ */
+ if (spi->max_speed_hz < CS35L41_SPI_MAX_FREQ) {
+ dev_err(cs35l41->dev, "SPI's max_speed_hz is capped at %u Hz, will not continue to avoid hanging\n",
+ spi->max_speed_hz);
+ return -EINVAL;
+ }
Instead of erroring out, I wonder if we can noodle our way to the
appropriate clk and clk_set_rate it up to 4MHz for this particular
laptop only? Stefan's taking a look at that.
Also, any SPI rate >~100k is probably just about usable, so we don't
want to error on <4MHz. Quite often the spi clock is set at some value
just below 4MHz. It's unclear if this is going to get fixed in the BIOS
at this point, so we don't know what exact rate we'd eventually receive.
+
+ dev_info(cs35l41->dev, "Adding DSD properties for %s\n", cs35l41->acpi_subsystem_id);
+
+ /* check SPI address to assign the index */
+ cs35l41->index = id;
+ cs35l41->channel_index = 0;
+ /* 10280BEB is missing pin which is typically assigned to `spk-id-gpios` */
+ cs35l41->speaker_id = cs35l41_get_speaker_id(physdev, cs35l41->index, 2, -1);
+ cs35l41->reset_gpio = gpiod_get_index(physdev, NULL, 1, GPIOD_OUT_LOW);
+
+ hw_cfg->spk_pos = cs35l41->index ? 1 : 0; // 0th L, 1st R
+ hw_cfg->bst_type = CS35L41_EXT_BOOST;
+ hw_cfg->gpio1.func = CS35l41_VSPK_SWITCH;
+ hw_cfg->gpio1.valid = true;
+ hw_cfg->gpio2.func = CS35L41_INTERRUPT;
+ hw_cfg->gpio2.valid = true;
+ hw_cfg->valid = true;
+
+ /* Add second cs-gpio here */
+ if (cs35l41->index)
+ spi->cs_gpiod = gpiod_get_index(physdev, NULL, 0, GPIOD_OUT_HIGH);
This will break once you pick up AMD's multi-cs patches, we should use
spi_set_csgpiod instead.
+
+ 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
@@ -92,6 +138,7 @@ static const struct cs35l41_prop_model cs35l41_prop_model_table[] = {
{ "CLSA0100", NULL, lenovo_legion_no_acpi },
{ "CLSA0101", NULL, lenovo_legion_no_acpi },
{ "CSC3551", "103C89C6", hp_vision_acpi_fix },
+ { "CSC3551", "10280BEB", dell_fiorano_no_acpi },
{}
};
--
2.40.1