Re: [PATCH 1/3] ALSA: hda/realtek: Use AW88399 I2C fixup chain on Legion machines

Hi Aaron,

I have serious concerns about this series. I addressed some of the
points below in my previous reply to your review of my v2 patch 3,
but I will reiterate them here for the benefit of everyone else.

* Patch 1: Default DAC routing

> Retain the existing Lenovo routing

Previously, you stated that the default 0x17->0x03 routing didn't need
to be changed, but this is NOT the default on these Legions, and in fact
I think this is just a misunderstanding based on the R9000P.
That laptop shares the PCI SSID 17aa:38bb with the Yoga S780 entry,
which chains through to alc285_fixup_thinkpad_x1_gen7 and sets:

    static const hda_nid_t preferred_pairs[] = {
        0x14, 0x02, 0x17, 0x03, 0x21, 0x03, 0
    };

This is the Yoga/ThinkPad routing, not the Legion's. The actual
hardware default for pin 0x17 on all known Legion models is DAC 0x06.
This can be readily verified on a Legion model that boots without the PCI
SSID collision, and I can personally attest it's the default on my
Pro 7 16AFR10H (codec SSID 17aa:3938).
The HDA_CODEC_QUIRK entries in the current code exist precisely
to prevent this collision by matching on codec SSID instead of
PCI SSID. It's not impossible that some hardware revisions already have
the 0x17->0x02 rerouting, but even if such laptops exist,
on all tested hardware the default routing is the incorrect 0x17->0x06.

Also, your series calls alc285_fixup_thinkpad_x1_gen7 via
ALC287_FIXUP_LENOVO_XPAD_HEADSET_JACK, so the fact that volume controls
work at all under your patch is a consequence of the fact that you, too,
override the default routing (but with 0x17->0x03, which is the DAC
usually reserved for headphones on these realtek codecs).

In general, not having a DAC override for pin 0x17 WILL break volume
controls on all supported Legion models. DAC 0x06 has no volume amplifier;
without the override, users get binary 0/100% volume.
That the 0x17->0x02 fix works and is indeed needed has been verified by
every tester across all three supported models, confirmed by the Windows
driver codec dump which selects DAC 0x02 for both 0x14 and 0x17, and is
consistent with dozens of existing alc269.c entries which perform
the same override (with comments clearly stating that this is done to fix
broken volume controls). Also, many of these quirks share a single DAC
between tweeters and woofers (typically 0x02 for both 0x14 and 0x17)
without issue, meaning the currently accepted solution doesn't appear
to be particularly problematic.

Regarding the 0x1d pincfg override: I don't disagree that in practice
this makes no difference, and I'm happy to have it removed if a maintainer
asks. This line was simply added to ensure we match exactly the pincfg of
the official Windows driver.

* Patch 1, 2 and 3: architectural concerns

ALC287_FIXUP_AW88399_I2C_2 is the base fixup that registers the
AW88399 amplifiers, by binding to the i2c devices created by the SMI
driver. It should remain generic and applicable to any machine using this
chip. Stuffing Legion-specific behavior (suppress_auto_mic, headset mode,
jack renaming, channel maps) directly into this function means any future
non-Legion machine using AW88399 inherits all of it.

The current design intentionally separates the two new quirks:
ALC287_FIXUP_AW88399_I2C_2 handles generic amp registration,
while ALC287_FIXUP_LENOVO_LEGION_AW88399 chains to it and adds
model-specific fixups. This is the same pattern used e.g. by the CS35L41
driver quirks, which have a generic cs35l41_fixup_i2c_two and separate
per-vendor fixup entries that chain to it.

Additionally, replacing a single self-contained ~20-line function with
a chain of three separate fixup entries
(ALC287_FIXUP_AW88399_LIMIT_INT_MIC_BOOST ->
ALC287_FIXUP_AW88399_HEADSET_MIC ->
ALC287_FIXUP_LENOVO_XPAD_HEADSET_JACK), each with its own chains,
makes the code harder to follow, forcing a reader to chase through the
fixup array to understand what the chain does. There's no code reuse
benefit either, since these new chained entries aren't individually
shared with other devices.

Patch 1 also removes the "alc287-lenovo-legion-aw88399" entry from
alc269_fixup_models[]. This entry allows users to force the Legion fixup
via the model= module boot parameter, which makes testing the new driver
much easier on new Legion models that may need it. Without it, users of
unsupported models have no way to easily verify whether our driver
applies to their hardware, and are instead forced to recompile the kernel.

* Patch 2: quad channel maps

I tested both patches on my 16AFR10H with music playback.

* With a 2.0 profile: both patches work correctly; tweeters and woofers
 play together as expected.
* With a 4.0 profile and my patch: music is fully silent (both tweeters
  and woofers).
* With a 4.0 profile and your patch: music plays through the
  tweeters only, woofers silent.
This is technically an improvement over full silence, but it reproduces
the exact broken state that motivated this entire driver effort: weak,
tinny audio from tweeters with no bass.

Even if the mapping were inverted so that only woofers played under 4.0,
it would still be wrong; bass-only audio without tweeters is equally as
broken.

In general, the only meaningful configuration for this hardware is stereo
2.0, where tweeters and woofers play the same signal together.
There is no useful way to split them into separate channels, because
they are not separate channels -- they are frequency-divided reproductions
of the same stereo signal. The bogus 4.0 profiles are an artifact of the
HDA parser, to be suppressed at userspace level via alsa-ucm-conf
(or simply ignored in practice), not by adding channel maps at the
realtek quirk-level. As such, I'm not convinced this patch is needed
in practice.

Also, as far as I can tell, there is no upstream precedent for mapping
a laptop's tweeter/woofer pairs as 4.0 surround channels.

* Patch 3: headphone jack rename

These laptops don't have docks. Renaming "Headphone Jack" to
"Dock Headphone Jack" is semantically incorrect, and I'm not convinced
the Thinkpad precedent applies here.
Regarding the need for this change, if there is a GNOME prompt issue
on plug events, that should be addressed at the userspace level, not by
giving hardware a misleading name.
More generally, I'm not sure I understand what problem is being solved
by this patch, as no Legion user has ever reported any issue
related to headphones and mic switching. I can also personally attest no
such issues on my Fedora 44 KDE install on the 16AFR10H.

* Summary

This series modifies a well-tested and necessary DAC override,
adds surround channel maps for hardware that isn't surround,
renames a jack to work around a quirk specific to one desktop environment,
and mixes generic and model-specific fixups in ways that will affect
future devices, while making the code harder to read and to test on
new devices.
None of these changes have been tested by the testers who validated
the current code.

Based on data from our github repo, I can attest that the current
fixup chain has likely been used by at least ~250 people in the last
~4 months across multiple kernel versions (from 6.19.10 to present)
and distros (Fedora, Arch, Cachy, Debian, Ubuntu, Nobara),
with no reports of mic switching or headset detection issues, instead
reporting that the 0x17->0x02 override was able to remove the need for
the alsa-ucm-conf workaround that was previously used to fix broken
volume controls on this hardware. As such, I am a bit surprised by this
patch series, and I admit I'm probably misunderstanding something or
missing some important context.

If the suppress_auto_mic and headset mode changes address a real mic or
headset issue on these Legions which I'm currently misunderstanding
and didn't happen to hear about previously, I'm happy to have them
incorporated as a separate patch on top of the existing fixup chain,
while preserving the crucial DAC 0x02 override needed for working
volume controls. But I'd first like to better understand what problem
they solve, as no Legion user has reported mic switching or
headset detection issues.

May I ask, on what Legion model (with what codec SSID) did you test
these changes? Is there a bug report thread I may read to get
more context?

Best regards,
Marco