RE: [PATCH] ALSA: hda: cs35l41: Support ASUS 2023 laptops with missing DSD

[Stefan Binding]

> -----Original Message-----
> From: Takashi Iwai <tiwai@xxxxxxx>
> Sent: Wednesday, August 23, 2023 9:44 AM
> To: Luke Jones <luke@xxxxxxxxxx>
> Cc: Takashi Iwai <tiwai@xxxxxxx>; tiwai@xxxxxxxx;
> james.schulman@xxxxxxxxxx; david.rhodes@xxxxxxxxxx;
> rf@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> sbinding@xxxxxxxxxxxxxxxxxxxxx; Jonathan LoBue <jlobue10@xxxxxxxxx>
> Subject: Re: [PATCH] ALSA: hda: cs35l41: Support ASUS 2023 laptops
with
> missing DSD
> 
> On Wed, 23 Aug 2023 10:02:11 +0200,
> Luke Jones wrote:
> >
> >
> >
> > On Wed, Aug 23 2023 at 09:37:08 +02:00:00, Takashi Iwai
> > <tiwai@xxxxxxx> 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@xxxxxxx> 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@xxxxxxxxx>
> > >>  >>  Signed-off-by: Jonathan LoBue <jlobue10@xxxxxxxxx>
> > >>  >>  Co-developed-by: Luke D. Jones <luke@xxxxxxxxxx>
> > >>  >>  Signed-off-by: Luke D. Jones <luke@xxxxxxxxxx>
> > >>  >>  ---
> > >>  >>   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?

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.

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

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.

Thanks,
Stefan Binding

> 
> I believe many other drivers match with vendor name, product name,
or
> whatever strings.
> 
> > I'll begin working on a patch series for each.
> 
> Thanks!
> 
> 
> Takashi