Date:   Mon, 23 Oct 2023 09:35:55 -0700
From:   Jonathan LoBue <jlobue10@...il.com>
To:     Stefan Binding <sbinding@...nsource.cirrus.com>,
        Huayu Zhang <leviz.kernel.dev@...il.com>
Cc:     Luke Jones <luke@...nes.dev>, Takashi Iwai <tiwai@...e.de>,
        tiwai@...e.com, james.schulman@...rus.com, david.rhodes@...rus.com,
        rf@...nsource.cirrus.com, linux-kernel@...r.kernel.org,
        patches@...nsource.cirrus.com
Subject: Re: [PATCH] ALSA: hda: cs35l41: Support ASUS 2023 laptops with missing DSD

On Monday, October 23, 2023 12:38:42 AM PDT Jonathan LoBue wrote:
> On Sunday, October 8, 2023 10:19:18 AM PDT Huayu Zhang wrote:
> > Hi Stefan and all,
> > 
> > Thanks for examine my email. I'm just interesting in Linux kernel
> > development and met sound issue with my `21J8 Lenovo ThinkBook 16p Gen
> > 4`.
> > Sorry for not familiar with the email process if any.
> > 
> > I wrote following changes based on some discovery and the downside
> > speakers (bass) seems begin to work. But the volumn keys actually
> > adjusting the frequence (I suppose). (Louder for high freq, and lower
> > volumn for low freq)
> > 
> > Wondering if any suggestions on the patch or any plan for officially
> > supporting of Lenovo ThinkBook 16p Gen 4. ^_^
> > I'll also provide the alsa-info and dmesg output below. Thanks a lot~
> > 
> > Patch:
> > 
> > From 124161547483109cbb491a8e39d1b5ef0973cd80 Mon Sep 17 00:00:00 2001
> > From: Huayu Zhang <zhanghuayu.dev@...il.com>
> > Date: Mon, 9 Oct 2023 00:59:56 +0800
> > Subject: [PATCH] thinkbook 16p gen4 sound fix
> > 
> > ---
> > sound/pci/hda/cs35l41_hda_property.c | 41 ++++++++++++++++++++++++++++
> > sound/pci/hda/patch_realtek.c | 1 +
> > 2 files changed, 42 insertions(+)
> > 
> > diff --git a/sound/pci/hda/cs35l41_hda_property.c
> > b/sound/pci/hda/cs35l41_hda_property.c
> > index b62a4e6968e2..af359fbeb671 100644
> > --- a/sound/pci/hda/cs35l41_hda_property.c
> > +++ b/sound/pci/hda/cs35l41_hda_property.c
> > @@ -74,6 +74,46 @@ static int hp_vision_acpi_fix(struct cs35l41_hda
> > *cs35l41, struct device *physde
> > return 0;
> > }
> > +static int lenovo_thinkbook16pgen4_no_acpi(struct cs35l41_hda
> > *cs35l41, struct device *physdev, int id,
> > + const char *hid)
> > +{
> > + struct cs35l41_hw_cfg *hw_cfg = &cs35l41->hw_cfg;
> > +
> > + dev_info(cs35l41->dev, "Adding DSD properties for %s\n",
> > cs35l41->acpi_subsystem_id);
> > +
> > + printk("CSC3551: id == 0x%x\n", id);
> > +
> > + // cirrus,dev-index
> > + cs35l41->index = id == 0x40 ? 0 : 1;
> > + cs35l41->channel_index = 0;
> > +
> > + // cs35l41->reset_gpio = gpiod_get_index(physdev, NULL,
> > cs35l41->index, GPIOD_OUT_LOW);
> > + cs35l41->reset_gpio = gpiod_get_index(physdev, NULL, 0, GPIOD_OUT_HIGH);
> > + printk("CS3551: reset_gpio == 0x%x\n", cs35l41->reset_gpio);
> > +
> > + // cs35l41->speaker_id = cs35l41_get_speaker_id(physdev,
> > cs35l41->index, nval, -1);
> > + cs35l41->speaker_id = cs35l41_get_speaker_id(physdev, 0, 0, 2);
> > +
> > + // cirrus,speaker-position
> > + hw_cfg->spk_pos = cs35l41->index;
> > +
> > + // cirrus,gpio1-func
> > + hw_cfg->gpio1.func = CS35l41_VSPK_SWITCH;
> > + hw_cfg->gpio1.valid = true;
> > +
> > + // cirrus,gpio2-func
> > + hw_cfg->gpio2.func = CS35L41_INTERRUPT;
> > + hw_cfg->gpio2.valid = true;
> > +
> > + hw_cfg->bst_type = CS35L41_EXT_BOOST;
> > + hw_cfg->valid = true;
> > +
> > + put_device(physdev);
> > + printk("CSC3551: Done.\n");
> > +
> > + return 0;
> > +}
> > +
> > struct cs35l41_prop_model {
> > const char *hid;
> > const char *ssid;
> > @@ -85,6 +125,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", "17AA38A9", lenovo_thinkbook16pgen4_no_acpi },
> > {}
> > };
> > diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> > index 751783f3a15c..fc884fdcec5f 100644
> > --- a/sound/pci/hda/patch_realtek.c
> > +++ b/sound/pci/hda/patch_realtek.c
> > @@ -10031,6 +10031,7 @@ static const struct snd_pci_quirk
> > alc269_fixup_tbl[] = { SND_PCI_QUIRK(0x17aa, 0x3886, "Y780 VECO DUAL",
> > ALC287_FIXUP_TAS2781_I2C), SND_PCI_QUIRK(0x17aa, 0x38a7, "Y780P AMD YG
> > dual", ALC287_FIXUP_TAS2781_I2C), SND_PCI_QUIRK(0x17aa, 0x38a8, "Y780P AMD
> > VECO dual", ALC287_FIXUP_TAS2781_I2C), + SND_PCI_QUIRK(0x17aa, 0x38a9,
> > "Lenovo ThinkBook 16p Gen 4",
> > ALC287_FIXUP_CS35L41_I2C_2),
> > SND_PCI_QUIRK(0x17aa, 0x38ba, "Yoga S780-14.5 Air AMD quad YC",
> > ALC287_FIXUP_TAS2781_I2C),
> > SND_PCI_QUIRK(0x17aa, 0x38bb, "Yoga S780-14.5 Air AMD quad AAC",
> > ALC287_FIXUP_TAS2781_I2C),
> > SND_PCI_QUIRK(0x17aa, 0x38be, "Yoga S980-14.5 proX YC Dual",
> > ALC287_FIXUP_TAS2781_I2C),
> > 
> > > On 03/10/2023 15:45, Luke Jones wrote:
> > > > On Thu, Aug 24 2023 at 08:31:06 AM +12:00:00, Luke Jones
> > > > 
> > > > <luke@...nes.dev> wrote:
> > > >>> The second member variable in cs35l41_prop_model_table is the SSID
> > > >>> to
> > > >>> match against.
> > > >>> The Lenovo laptops in the initial patch didn't have different SSIDs
> > > >>> so
> > > >>> the entry was set to NULL for those.
> > > >>> Future entries using CSC3551 MUST always have an accompanying SSID
> > > >>> with this entry.
> > > >>> Takashi was correct, the implementation is intended to also be used
> > > >>> to
> > > >>> patch incorrect DSD.
> > > >>> 
> > > >>> We have a potential solution to workaround the SPI cs-gpios issue
> > > >>> inside here,
> > > >>> though the drawback for that is that it only works for laptops with
> > > >>> 2
> > > >>> SPI amps.
> > > >> 
> > > >> Can you provide me this so I can test? I have laptops with SPI 2 and
> > > >> 4 speaker setups.
> > > > 
> > > > Hi Stefan,
> > > > 
> > > > Do you have any further information about the status of this in
> > > > regards to the 2023 laptops?
> > > 
> > > Hi,
> > > 
> > > We are currently working on adding support for 2023 ASUS laptops without
> > > _DSD.
> > > 
> > > >>> I also took a look at the function for applying DSD properties for
> > > >>> the
> > > >>> 2023 ROG laptops.
> > > >>> Unfortunately the one-size-fits-all approach will not work, some of
> > > >>> these laptops are i2c
> > > >>> and some are SPI, meaning the GPIO indexes are different for
> > > >>> different
> > > >>> laptops.
> > > >> 
> > > >> Do you mean "spk-id-gpios"? For all the laptops I know of this seems
> > > >> to be
> > > >> Package () { "spk-id-gpios", Package () {
> > > >> 
> > > >>    SPK1, 0x02, Zero, Zero,
> > > >>    SPK1, 0x02, Zero, Zero
> > > >> 
> > > >> } },
> > > >> 
> > > >> There is one laptop where it is One not 0x02 (the GA402N)
> > > >> 
> > > >>> Some of the laptops do no have Speaker IDs.
> > > >>> Also, no laptop other than the 2 I added already should ever use
> > > >>> CS35L41_EXT_BOOST_NO_VSPK_SWITCH (in fact I believe all these
> > > >>> laptops
> > > >>> are internal
> > > >>> boost anyway).
> > > >> 
> > > >> Grazie.
> > > >> 
> > > >>> We are currently working internally on adding support for the 2023
> > > >>> ROG
> > > >>> laptops, so we
> > > >>> ask for you guys to hold off on trying to upstream support for these
> > > >>> laptops.
> > > >> 
> > > >> Ah great. Thank you. I apologise for trying to rush things, but I do
> > > >> have a discord server of over 4000 people, many of whom have laptops
> > > >> with cirrus amps.
> > > >> 
> > > >> For now I'm including a patch in my kernel builds with this mapping:
> > > >> 
> > > >> const struct cs35l41_prop_model cs35l41_prop_model_table[] = {
> > > >> 
> > > >>     { "CLSA0100", NULL, lenovo_legion_no_acpi },
> > > >>     { "CLSA0101", NULL, lenovo_legion_no_acpi },
> > > >>     { "CSC3551", "10431433", asus_rog_2023_no_acpi }, // ASUS GS650P
> > > >> 
> > > >> - i2c
> > > >> 
> > > >>     { "CSC3551", "10431463", asus_rog_2023_no_acpi }, // ASUS GA402X
> > > >> 
> > > >> - i2c
> > > >> 
> > > >>     { "CSC3551", "10431473", asus_rog_2023_no_acpi }, // ASUS GU604V
> > > >> 
> > > >> - spi
> > > >> 
> > > >>     { "CSC3551", "10431483", asus_rog_2023_no_acpi }, // ASUS GU603V
> > > >> 
> > > >> - spi
> > > >> 
> > > >>     { "CSC3551", "10431493", asus_rog_2023_no_acpi }, // ASUS GV601V
> > > >> 
> > > >> - spi
> > > >> 
> > > >>     { "CSC3551", "10431573", asus_rog_2023_no_acpi }, // ASUS GZ301V
> > > >> 
> > > >> - spi
> > > >> 
> > > >>     { "CSC3551", "104317F3", asus_rog_2023_no_acpi }, // ASUS ROG
> > > >> 
> > > >> ALLY - i2c
> > > >> 
> > > >>     { "CSC3551", "10431B93", asus_rog_2023_no_acpi }, // ASUS G614J -
> > > >> 
> > > >> spi
> > > >> 
> > > >>     { "CSC3551", "10431CAF", asus_rog_2023_no_acpi }, // ASUS G634J -
> > > >> 
> > > >> spi
> > > >> 
> > > >>     { "CSC3551", "10431C9F", asus_rog_2023_no_acpi }, // ASUS G614JI
> > > >> 
> > > >> -spi
> > > >> 
> > > >>     { "CSC3551", "10431D1F", asus_rog_2023_no_acpi }, // ASUS G713P -
> > > >> 
> > > >> i2c
> > > >> 
> > > >>     { "CSC3551", "10431F1F", asus_rog_2023_no_acpi }, // ASUS H7604JV
> > > >> 
> > > >> - spi
> > > >> 
> > > >>     {}
> > > >> 
> > > >> };
> > > >> 
> > > >> These are the machines I have verified the gpios and such for.
> > > > 
> > > > I have a new version of this patch with all listed models confirmed as
> > > > working, and with slightly different settings for some. The only thing
> > > > missing in a solution to the gpio-cs issue.
> > > > 
> > > > Can you please provide an update on where you are with ASUS support in
> > > > particular so that I may consider if it is worth my time submitting
> > > > the updated patch.
> > > 
> > > We would prefer for you to wait, as we are looking to push up this
> > > support in the coming weeks.
> > > 
> > > >> Cheers,
> > > >> Luke.
> 
> Stefan and all,
> 
> Thanks for the hard work getting the DSD properties sorted and installed on
> various BIOSes. The one that I'm most concerned with and what got me
> interested in this patching mechanism in the first place is the ASUS ROG
> ALLY. From a DSD dump, it's clear that the latest ROG ALLY BIOS (330) has
> the proper audio DSD properties. Unfortunately, certain things on many
> people's setups across numerous ROG ALLYs (using various Linux distros)
> have broken with the 330 BIOS. This led many of us to revert back to the
> previous BIOS, 323. Knowing the proper internal boost corresponding values
> now (cap, inductor, and peak current), it's ideal to push these properties
> out for those people who prefer to stay on an older BIOS for the ROG ALLY.
> I've developed a small patch that I've used on a custom 6.5.8 Nobara
> kernel. Line numbers and whatnot may be different according to latest on
> official Linux git, but I'd still like to share what I have. I think it is
> a good addition. There's basically a small logic check versus BIOS number.
> If the BIOS is 330 or greater, then the function to override DSD properties
> is exited. If the BIOS has a smaller number than 330, then the DSD
> overrides are applied. Here is my patch.
> 
> diff --git a/sound/pci/hda/cs35l41_hda_property.c b/sound/pci/hda/
> cs35l41_hda_property.c
> index b39f944..b67c636 100644
> --- a/sound/pci/hda/cs35l41_hda_property.c
> +++ b/sound/pci/hda/cs35l41_hda_property.c
> @@ -6,7 +6,9 @@
>  //
>  // Author: Stefan Binding <sbinding@...nsource.cirrus.com>
> 
> +#include <linux/dmi.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/kernel.h>
>  #include <linux/string.h>
>  #include "cs35l41_hda_property.h"
> 
> @@ -78,6 +80,40 @@ static int asus_rog_2023_spkr_id2(struct cs35l41_hda
> *cs35l41, struct device *ph
>  	return 0;
>  }
> 
> +static int asus_rog_2023_ally_fix(struct cs35l41_hda *cs35l41, struct
> device *physdev, int id,
> +				const char *hid)
> +{
> +	const char *rog_ally_bios_ver =
> dmi_get_system_info(DMI_BIOS_VERSION);
> +	const char *rog_ally_bios_num = rog_ally_bios_ver + 6; // Dropping
> the RC71L. part before the number
> +	int rog_ally_bios_int;
> +	kstrtoint(rog_ally_bios_num, 10, &rog_ally_bios_int);
> +	if(rog_ally_bios_int >= 330){
> +		printk(KERN_INFO "DSD properties exist in the %d
> BIOS\n", rog_ally_bios_int);
> +		return -ENOENT;
> +	}
> +
> +	struct cs35l41_hw_cfg *hw_cfg = &cs35l41->hw_cfg;
> +
> +	dev_info(cs35l41->dev, "Adding DSD properties for %s\n", cs35l41-
> 
> >acpi_subsystem_id);
> 
> +
> +	cs35l41->index = id == 0x40 ? 0 : 1;
> +	cs35l41->channel_index = 0;
> +	cs35l41->reset_gpio = gpiod_get_index(physdev, NULL, 0,
> GPIOD_OUT_HIGH);
> +	cs35l41->speaker_id = cs35l41_get_speaker_id(physdev, 0, 0, 2);
> +	hw_cfg->spk_pos = cs35l41->index;
> +	hw_cfg->gpio1.func = CS35L41_NOT_USED;
> +	hw_cfg->gpio1.valid = true;
> +	hw_cfg->gpio2.func = CS35L41_INTERRUPT;
> +	hw_cfg->gpio2.valid = true;
> +	hw_cfg->bst_type = CS35L41_INT_BOOST;
> +	hw_cfg->bst_ind = 1000; /* 1,000nH Inductance value */
> +	hw_cfg->bst_ipk = 4500; /* 4,500mA peak current */
> +	hw_cfg->bst_cap = 24; /* 24 microFarad cap value */
> +	hw_cfg->valid = true;
> +
> +	return 0;
> +}
> +
>  struct cs35l41_prop_model {
>  	const char *hid;
>  	const char *ssid;
> @@ -94,7 +130,7 @@ const struct cs35l41_prop_model
> cs35l41_prop_model_table[] = {
>  	{ "CSC3551", "10431483", asus_rog_2023_spkr_id2 }, // ASUS GU603V 
-
> spi, reset gpio 1
>  	{ "CSC3551", "10431493", asus_rog_2023_spkr_id2 }, // ASUS GV601V 
-
> spi, reset gpio 1
>  	{ "CSC3551", "10431573", asus_rog_2023_spkr_id2 }, // ASUS GZ301V 
-
> spi, reset gpio 0
> -	{ "CSC3551", "104317F3", asus_rog_2023_spkr_id2 }, // ASUS ROG 
ALLY
> - i2c
> +	{ "CSC3551", "104317F3", asus_rog_2023_ally_fix }, // ASUS ROG ALLY
> - i2c
>  	{ "CSC3551", "10431B93", asus_rog_2023_spkr_id2 }, // ASUS G614J -
> spi, reset gpio 0
>  	{ "CSC3551", "10431CAF", asus_rog_2023_spkr_id2 }, // ASUS G634J -
> spi, reset gpio 0
>  	{ "CSC3551", "10431C9F", asus_rog_2023_spkr_id2 }, // ASUS G614JI 
-
> spi, reset gpio 0
> 
> This patch is what my changes are after unpacking the kernel source RPM for
> a Nobara kernel and applying a version of Luke's patches.
> 
> I think a similar BIOS version checking logic may come in handy again in the
> future. Thanks.
> 
> Best Regards,
> Jon LoBue

One more minor change to my proposed patch is to replace the "return -ENOENT;" 
line in the patch routine not applying and exiting to "return 0; //Patch not 
applicable. Exiting successfully" since it's actually a successful exit if the 
BIOS version is detected as 330 or greater and no patch is applied. Thanks.


Best Regards,
Jon LoBue

Powered by blists - more mailing lists