Messages in this thread | | | Date | Wed, 23 Aug 2023 10:43:52 +0200 | From | Takashi Iwai <> | 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@suse.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@suse.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@gmail.com> > >> >> Signed-off-by: Jonathan LoBue <jlobue10@gmail.com> > >> >> Co-developed-by: Luke D. Jones <luke@ljones.dev> > >> >> Signed-off-by: Luke D. Jones <luke@ljones.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 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
| |