Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Date:   Thu, 14 Dec 2023 12:02:58 +0100
Message-ID: <87fs05qc65.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     patches@opensource.cirrus.com
Cc:     Aleksandrs Vinarskis <alex.vinarskis@gmail.com>, perex@perex.cz,
        tiwai@suse.com, sbinding@opensource.cirrus.com,
        james.schulman@cirrus.com, david.rhodes@cirrus.com,
        alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
        Jasper Smet <josbeir@gmail.com>
Subject: Re: [PATCH 1/1] ALSA: hda: cs35l41: Dell Fiorano add missing _DSD properties
In-Reply-To: <20231212195243.10666-1-alex.vinarskis@gmail.com>
References: <20231212195243.10666-1-alex.vinarskis@gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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@gmail.com>
> Signed-off-by: Jasper Smet <josbeir@gmail.com>
> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>

Can Cirrus team review this?


thanks,

Takashi

> ---
>  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@opensource.cirrus.com>
>  
>  #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;
> +	}
> +
> +	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);
> +
> +	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
>