Date:   Wed, 23 Aug 2023 20:02:11 +1200
From:   Luke Jones <luke@...nes.dev>
To:     Takashi Iwai <tiwai@...e.de>
Cc:     tiwai@...e.com, james.schulman@...rus.com, david.rhodes@...rus.com,
        rf@...nsource.cirrus.com, linux-kernel@...r.kernel.org,
        sbinding@...nsource.cirrus.com, Jonathan LoBue <jlobue10@...il.com>
Subject: Re: [PATCH] ALSA: hda: cs35l41: Support ASUS 2023 laptops with
 missing DSD



On Wed, Aug 23 2023 at 09:37:08 +02:00:00, Takashi Iwai <tiwai@...e.de> 
wrote:
> On Wed, 23 Aug 2023 09:28:39 +0200,
> Luke Jones wrote:
>> 
>> 
>> 
>>  On Wed, Aug 23 2023 at 08:24:51 +02:00:00, Takashi Iwai
>>  <tiwai@...e.de> wrote:
>>  > On Wed, 23 Aug 2023 03:10:08 +0200,
>>  > Luke D. Jones wrote:
>>  >>
>>  >>  Support adding the missing DSD properties required  for ASUS ROG
>>  >> 2023
>>  >>  laptops and other ASUS laptops to properly utilise the cs35l41.
>>  >>
>>  >>  This support includes both I2C and SPI connected amps.
>>  >>
>>  >>  The SPI connected amps may be required to use an external DSD 
>> patch
>>  >>  to fix or add the "cs-gpios" property.
>>  >>
>>  >>  Co-developed-by: Jonathan LoBue <jlobue10@...il.com>
>>  >>  Signed-off-by: Jonathan LoBue <jlobue10@...il.com>
>>  >>  Co-developed-by: Luke D. Jones <luke@...nes.dev>
>>  >>  Signed-off-by: Luke D. Jones <luke@...nes.dev>
>>  >>  ---
>>  >>   sound/pci/hda/cs35l41_hda_property.c | 26
>>  >> ++++++++++++++++++++++++++
>>  >>   1 file changed, 26 insertions(+)
>>  >>
>>  >>  diff --git a/sound/pci/hda/cs35l41_hda_property.c
>>  >> b/sound/pci/hda/cs35l41_hda_property.c
>>  >>  index 673f23257a09..69879ab57918 100644
>>  >>  --- a/sound/pci/hda/cs35l41_hda_property.c
>>  >>  +++ b/sound/pci/hda/cs35l41_hda_property.c
>>  >>  @@ -43,6 +43,31 @@ static int lenovo_legion_no_acpi(struct
>>  >> cs35l41_hda *cs35l41, struct device *phy
>>  >>   	return 0;
>>  >>   }
>>  >>
>>  >>  +/*
>>  >>  + * The CSC3551 is used in almost the entire ASUS ROG laptop 
>> range
>>  >> in 2023, this is likely to
>>  >>  + * also include many non ROG labelled laptops. It is also used
>>  >> with either I2C connection or
>>  >>  + * SPI connection. The SPI connected versions may be missing a
>>  >> chip select GPIO and require
>>  >>  + * an DSD table patch.
>>  >>  + */
>>  >>  +static int asus_rog_2023_no_acpi(struct cs35l41_hda *cs35l41,
>>  >> struct device *physdev, int id,
>>  >>  +				const char *hid)
>>  >>  +{
>>  >>  +	struct cs35l41_hw_cfg *hw_cfg = &cs35l41->hw_cfg;
>>  >>  +
>>  >>  +	/* check SPI or I2C address to assign the index */
>>  >>  +	cs35l41->index = (id == 0 || 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->gpio2.func = CS35L41_INTERRUPT;
>>  >>  +	hw_cfg->gpio2.valid = true;
>>  >>  +	hw_cfg->bst_type = CS35L41_EXT_BOOST_NO_VSPK_SWITCH;
>>  >>  +	hw_cfg->valid = true;
>>  >>  +
>>  >>  +	return 0;
>>  >>  +}
>>  >>  +
>>  >>   struct cs35l41_prop_model {
>>  >>   	const char *hid;
>>  >>   	const char *ssid;
>>  >>  @@ -53,6 +78,7 @@ struct cs35l41_prop_model {
>>  >>   const struct cs35l41_prop_model cs35l41_prop_model_table[] = {
>>  >>   	{ "CLSA0100", NULL, lenovo_legion_no_acpi },
>>  >>   	{ "CLSA0101", NULL, lenovo_legion_no_acpi },
>>  >>  +	{ "CSC3551", NULL, asus_rog_2023_no_acpi },
>>  >
>>  > I believe this breaks things badly.  cs35l41_add_dsd_properties() 
>> is
>>  > always called no matter whether _DSD is found or not.  So this 
>> will
>>  > override the setup of all machines with CSC3551 even if it has a
>>  > proper _DSD.
>> 
>>  These are the entries I know of so far since they definitely had to 
>> be
>>  added and have a DSD patch:
>> 
>>  - SPI_2
>>     - 0x1043, 0x1473, "ASUS GU604V"
>>     - 0x1043, 0x1483, "ASUS GU603V"
>>     - 0x1043, 0x1493, "ASUS GV601V"
>>     - 0x1043, 0x1573, "ASUS GZ301V"
>>     - 0x1043, 0x1c9f, "ASUS G614JI"
>>  - SPI_4
>>     - 0x1043, 0x1caf, "ASUS G634JYR/JZR"
>>  - I2C_2
>>     - 0x1043, 0x1d1f, "ASUS ROG Strix G17
>>     - 0x1043, 0x1463, "Asus GA402X"
>>     - 0x1043, 0x1433, "ASUS GX650P"
>>     - ROG ALLY
>> 
>>  You can see the variants are V, J, X, and P. A grep through the DSL
>>  dumps I have collected reveals that these machines are all indeed
>>  missing DSD entries. These are a mix of I2C and SPI.
>> 
>>  The patch I submitted was based on Stefan's work only, and tested 
>> on 3
>>  SPI machines plus 2 I2C machines with no issues except the SPI
>>  machines needing a chipselect DSD patch.
>> 
>>  It's worth stating that the DSD patches people were using all 
>> followed
>>  the exact same template except for the SPI number, or speaker ID.
>> 
>>  I'd wager it being a safe bet to assume that every one of the ASUS
>>  laptops this year using the CSC3551 will be missing the required DSD
>>  entries given the trend so far.
> 
> Yes, I know that there are tons of such devices that need the
> workaround for missing _DSD.  My point is, however, that your patch
> breaks the devices that *do have* _DSD, because the function always
> overrides.

Ah, because CSC3551 is not unique at all. I see.

> 
>>  > The existing entries of CLSA0100 and CLSA0101 are OK since
>>  > (supposedly) those never had _DSD.  The current code is a bit
>>  > misleading as if it's applicable easily, though.
>>  >
>>  > That said, we have to apply the setup only conditionally for each
>>  > specific device.  One easy thing would be to move the function 
>> call
>>  > after _DSD check.  But, I *guess* that Stefan applied the 
>> function at
>>  > the top so that it may cover the all cases including incorrect 
>> _DSD
>>  > properties.
>> 
>>  Given the trend of what I've seen this seems like a reasonable
>>  assumption and desired.
> 
> ... and it's not enough.  That's another point.  The parameters aren't
> always same for all devices but may vary.  That is, it must have some
> check of devices and models to identify which parameters may be used.
> 
>>  > And, the question is how to be specific to each device.  This can 
>> be
>>  > messy, as the sub-codec driver is probed independently from 
>> Realtek
>>  > codec driver, hence you can't pass any flag from Realtek to CS 
>> driver
>>  > at the probe time.  In the end, we might need to keep another 
>> table of
>>  > IDs (either the same SSID or DMI) to distinguish which machine 
>> needs
>>  > which properties.
>> 
>>  Is there some other ID we can use? I see:
>>  [   13.569242] cs35l41-hda spi0-CSC3551:00-cs35l41-hda.1: Cirrus 
>> Logic
>>  CS35L41 (35a40), Revision: B2
>>  and I'd assume that 35a40 is unique? A bit of searching on my 
>> discord
>>  reveals that all the machines I listed that require the same patch
>>  also have this identifier (including a ProArt laptop).
> 
> That's not unique to each *device* (not the chip model).  Say, we want
> to distinguish ASUS GU604V and ASUS G64JYR.  They may have different
> setups.
> 
> I guess only DMI is suitable at the time of probing this driver.

Oh, I'm sorry I should have paid more attention the first time. I can 
match against acpi_subsystem_id which would be for example 10431caf yes?

I'll begin working on a patch series for each.

Powered by blists - more mailing lists