[PATCH] ASoC: hda/realtek: Add support for Lenovo Hudson

[PATCH] ASoC: hda/realtek: Add support for Lenovo Hudson

[guanlan]

From: Tao Xiao <guanlan@awinic.com>

The Lenovo Hudson (SSID 17aa:3814) uses the same ALC287 codec with two
AW88399 HDA side-codec amplifiers as the existing Legion models, so it
needs the same speaker setup.

Apply ALC287_FIXUP_LENOVO_LEGION_AW88399 for this machine and add the
matching entry to the aw88399_prop_model_table so that the reversed I2C
wiring is corrected in the same way.

Without this, the internal speakers do not work correctly. Tested on a
Lenovo Hudson laptop.

Signed-off-by: Tao Xiao <guanlan@awinic.com>
---
 sound/hda/codecs/realtek/alc269.c          | 2 ++
 sound/hda/codecs/side-codecs/aw88399_hda.c | 1 +
 2 files changed, 3 insertions(+)

diff --git a/sound/hda/codecs/realtek/alc269.c b/sound/hda/codecs/realtek/alc269.c
index a756e0076021..b33bd4efb51c 100644
--- a/sound/hda/codecs/realtek/alc269.c
+++ b/sound/hda/codecs/realtek/alc269.c
@@ -8664,8 +8664,10 @@ static const struct hda_quirk alc269_fixup_tbl[] = {
 	HDA_CODEC_QUIRK(0x17aa, 0x3937, "Legion R9000P ADR10H", ALC287_FIXUP_LENOVO_LEGION_AW88399),
 	HDA_CODEC_QUIRK(0x17aa, 0x3938, "Legion Pro 7 16AFR10H", ALC287_FIXUP_LENOVO_LEGION_AW88399),
 	HDA_CODEC_QUIRK(0x17aa, 0x3939, "Legion Pro 7 16AFR10H", ALC287_FIXUP_LENOVO_LEGION_AW88399),
+	HDA_CODEC_QUIRK(0x17aa, 0x3814, "Lenovo Hudson", ALC287_FIXUP_LENOVO_LEGION_AW88399),
 	SND_PCI_QUIRK(0x17aa, 0x393e, "Lenovo ThinkBook 14 G8+ IPH", ALC287_FIXUP_LENOVO_XPAD_HEADSET_JACK),
 	HDA_CODEC_QUIRK(0x17aa, 0x394c, "Lenovo Yoga Slim 7 14AGP11", ALC287_FIXUP_YOGA9_14IAP7_BASS_SPK_PIN),
+	SND_PCI_QUIRK(0x17aa, 0x3814, "Lenovo Hudson", ALC287_FIXUP_LENOVO_LEGION_AW88399),
 	SND_PCI_QUIRK(0x17aa, 0x3977, "IdeaPad S210", ALC283_FIXUP_INT_MIC),
 	SND_PCI_QUIRK(0x17aa, 0x3978, "Lenovo B50-70", ALC269_FIXUP_DMIC_THINKPAD_ACPI),
 	SND_PCI_QUIRK(0x17aa, 0x3bf8, "Quanta FL1", ALC269_FIXUP_PCM_44K),
diff --git a/sound/hda/codecs/side-codecs/aw88399_hda.c b/sound/hda/codecs/side-codecs/aw88399_hda.c
index 81ee8a3987fd..b204aabc5572 100644
--- a/sound/hda/codecs/side-codecs/aw88399_hda.c
+++ b/sound/hda/codecs/side-codecs/aw88399_hda.c
@@ -168,6 +168,7 @@ struct aw88399_prop_model {
 };
 
 static const struct aw88399_prop_model aw88399_prop_model_table[] = {
+	{ "17AA3814", aw88399_swap_channels },
 	{ "17AA3906", aw88399_swap_channels },
 	{ "17AA3907", aw88399_swap_channels },
 	{ "17AA3927", aw88399_swap_channels },

base-commit: 067c5c24cef23d913b67eaaf71ae67e7169a6620
-- 
2.34.1

Re: [PATCH] ASoC: hda/realtek: Add support for Lenovo Hudson

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] ALSA: hda/realtek: duplicate and unsorted 17aa:3814 quirks in
  alc269_fixup_tbl
--

commit 7ce4d2a12d39225f2afc7745135304f988537c37
Author: Tao Xiao <guanlan@awinic.com>

ASoC: hda/realtek: Add support for Lenovo Hudson

This commit adds ALC287_FIXUP_LENOVO_LEGION_AW88399 quirk entries and an
AW88399 channel-swap entry for the Lenovo Hudson (SSID 17aa:3814).

> diff --git a/sound/hda/codecs/realtek/alc269.c b/sound/hda/codecs/realtek/alc269.c
> index a756e0076021a..b33bd4efb51c8 100644
> --- a/sound/hda/codecs/realtek/alc269.c
> +++ b/sound/hda/codecs/realtek/alc269.c
> @@ -8664,8 +8664,10 @@ static const struct hda_quirk alc269_fixup_tbl[] = {
>  	HDA_CODEC_QUIRK(0x17aa, 0x3937, "Legion R9000P ADR10H", ALC287_FIXUP_LENOVO_LEGION_AW88399),
>  	HDA_CODEC_QUIRK(0x17aa, 0x3938, "Legion Pro 7 16AFR10H", ALC287_FIXUP_LENOVO_LEGION_AW88399),
>  	HDA_CODEC_QUIRK(0x17aa, 0x3939, "Legion Pro 7 16AFR10H", ALC287_FIXUP_LENOVO_LEGION_AW88399),
> +	HDA_CODEC_QUIRK(0x17aa, 0x3814, "Lenovo Hudson", ALC287_FIXUP_LENOVO_LEGION_AW88399),
>  	SND_PCI_QUIRK(0x17aa, 0x393e, "Lenovo ThinkBook 14 G8+ IPH", ALC287_FIXUP_LENOVO_XPAD_HEADSET_JACK),
>  	HDA_CODEC_QUIRK(0x17aa, 0x394c, "Lenovo Yoga Slim 7 14AGP11", ALC287_FIXUP_YOGA9_14IAP7_BASS_SPK_PIN),
> +	SND_PCI_QUIRK(0x17aa, 0x3814, "Lenovo Hudson", ALC287_FIXUP_LENOVO_LEGION_AW88399),

[Severity: Low]
Why are both HDA_CODEC_QUIRK and SND_PCI_QUIRK added for 0x17aa:0x3814 in
alc269_fixup_tbl, and placed after the 0x39xx entries?

snd_hda_pick_fixup() scans alc269_fixup_tbl sequentially and falls back to
hda_quirk_lookup_id() on the codec SSID across all entries, so one entry is
redundant when both entries map 0x17aa:0x3814 to the same fixup.  In
addition, alc269_fixup_tbl is sorted by subvendor and subdevice ID (so
0x3814 belongs between 0x3813 and 0x3818), and placing HDA_CODEC_QUIRK at
the end of the 0x39xx block means any shared PCI SSID earlier in the
0x17aa range would match before this HDA_CODEC_QUIRK is reached.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111043601.306427-1-guanlan@awinic.com?part=1

Re: [PATCH] ASoC: hda/realtek: Add support for Lenovo Hudson

[Marco Giunta]

Hi Tao,

Thank you for your contribution, it's amazing to see Awinic use our
driver.
The changes in aw88399_hda.c are fine, but the rest deserves some comment.

Mandatory fixes:
----------------
* Typo in the title: it should be "ALSA: hda/realtek", not "ASoC".

* As sashiko pointed out, both entries map to the same fixup, so one is
  redundant and should be removed.
  You didn't specify whether 17aa:3814 is the codec or PCI SSID, but given
  that this value is also the ACPI SSID, and that all currently supported
  Lenovo models have matching codec and ACPI SSIDs, I'd guess it's the
  former. In this case, I would recommend just keeping the HDA_CODEC_QUIRK
  entry: by matching based on the codec ID, the connection between the
  corresponding bits of code in alc269.c and aw88399_hda.c is immediately
  obvious, and using HDA_CODEC_QUIRK() makes the code self-documenting
  that this isn't a PCI SSID, which is useful if in the future ID
  collisions pop up.
  Also, statistically speaking, these Lenovo laptops tend to share PCI
  SSIDs more often than they do codec ones, so it should be safer overall
  to use the latter. Or at least, this is the rationale behind my past
  commits. Therefore, overall, I'd argue in favor of sticking to this one
  convention for all AW88399 HDA devices' realtek quirks.
  Alternatively, if you have arguments in favor of keeping the
  SND_PCI_QUIRK() entry instead, of course it's also fine to keep that
  one, possibly with the PCI SSID instead if needed (see below).
  The important part is to fix the duplicated entries.

* Again as sashiko said, new quirks shouldn't mess with the sorting of the
  quirk table. Exceptions are allowed if this is done to prevent SSID
  collisions; unless your device's PCI SSID is already taken and its
  value comes before the codec ID in the sorted table, please move the
  quirk entry accordingly.
  If you do need to avoid a collision, you can take a look at
  commit 494978ae8243 ("ALSA: hda/realtek: enable AW88399 on Lenovo Legion
  Pro") for reference.

Optional fixes:
---------------
* I would suggest explicitly mentioning the AW88399 in the title of the
  commit message, so that it's immediately obvious from the commit history
  that this isn't simply one of the usual realtek quirks, i.e. this commit
  touches the AW88399 HDA driver as well despite being labelled as
  "ALSA: hda/realtek".
  More generally, "add support for device x" feels a bit generic; in the
  past, I used something explicit like "ALSA: hda/realtek: enable AW88399
  on Lenovo Legion R9000P AFR10". It's not strictly necessary that you
  change the title to something with this style, but I suggest you do.

* Related to the previous point, naming the device as "Lenovo Hudson" in
  the quirk entry feels a bit generic and unclear. I'd suggest appending
  the product_family ID after the commercial name. This is also useful
  to distinguish future products in the same family that may need quirks
  as well.
  Again, this isn't strictly mandatory, but I think it makes the code
  clearer and more self-documenting, so I recommend it.

With these changes, a v2 gets:

Reviewed-by: Marco Giunta <marco_giunta@outlook.it>

Thanks again for your contribution.

Best regards,
Marco