Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i5ec1447f:Fastmail
Date:   Fri, 14 Jul 2023 21:27:56 +1200
From:   Luke Jones <luke@ljones.dev>
Subject: Re: [PATCH 1/2] ALSA: hda: cs35l41: Add fixups for machines without a
 valid ACPI _DSD
To:     David Xu <xuwd1@hotmail.com>
Cc:     James Schulman <james.schulman@cirrus.com>,
        David Rhodes <david.rhodes@cirrus.com>,
        Richard Fitzgerald <rf@opensource.cirrus.com>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Stefan Binding <sbinding@opensource.cirrus.com>,
        Andy Chi <andy.chi@canonical.com>,
        Tim Crawford <tcrawford@system76.com>,
        Philipp Jungkamp <p.jungkamp@gmx.net>,
        Kacper =?iso-8859-2?q?Michaj=B3ow?= <kasper93@gmail.com>,
        Matthew Anderson <ruinairas1992@gmail.com>,
        Yuchi Yang <yangyuchi66@gmail.com>,
        Yang Yingliang <yangyingliang@huawei.com>,
        alsa-devel@alsa-project.org, patches@opensource.cirrus.com,
        linux-kernel@vger.kernel.org
Message-Id: <KY4SXR.AVPNR1Y9HZSE3@ljones.dev>
In-Reply-To: <WL3SXR.02LV303LBEC33@ljones.dev>
References: <20230713162955.34842-1-xuwd1@hotmail.com>
        <SY4P282MB183539E5A91B351249A2F8F1E037A@SY4P282MB1835.AUSP282.PROD.OUTLOOK.COM>
        <WL3SXR.02LV303LBEC33@ljones.dev>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Precedence: bulk


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