Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Thu, 14 Dec 2023 16:49:23 +0100
Message-ID: <87ttokpyws.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Stuart Henderson <stuarth@opensource.cirrus.com>
Cc:     <patches@opensource.cirrus.com>,
        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: <d98a3157-6e58-4dd9-a35a-759bcdcbe0c9@opensource.cirrus.com>
References: <20231212195243.10666-1-alex.vinarskis@gmail.com>
        <87fs05qc65.wl-tiwai@suse.de>
        <d98a3157-6e58-4dd9-a35a-759bcdcbe0c9@opensource.cirrus.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=ISO-8859-1
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Thu, 14 Dec 2023 16:39:58 +0100,
Stuart Henderson wrote:
> 
> 
> 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@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
> 
> 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.

It's fine to wait for a while for me.
Hopefully we can make it in 6.7, and we can catch up in the next
week.

(BTW, I'll be off from the next Tuesday, and the reply will be
delayed, but I can eventually check and merge patches remotely :)

> >> ---
> >>   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;
> >> +	}
> 
> 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.

I suppose the error-out was due to safety reasons, but the clock
adjustment works, it should be fine.  Let's see.

> >> +
> >> +	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.

Thanks, good to know.

I'm looking forward to your revised patches :)


Takashi