Re: [PATCH 2/2] ALSA: hda/realtek: Convert HP AIO SPK fixup to INIT-stage depop

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ALSA: hda/realtek: unconditional unmute in alc225_fixup_hp_aio_spk()
- [Low] ALSA: hda/realtek: duplicate 103c:84ee quirk entry in alc269_fixup_tbl
--

commit 2bd7f803bf3acde738482eabfd02d74f96a4d957
Author: Zhang Heng <zhangheng@kylinos.cn>

ALSA: hda/realtek: Convert HP AIO SPK fixup to INIT-stage depop

This commit replaces the static coef 0x10 verb table for
ALC225_FIXUP_HP_AIO_SPK with an init-stage fixup function that mutes pin 0x1b,
writes coef 0x10, sleeps 75 ms, and unmutes pin 0x1b. It also adds a quirk
entry for SSID 103c:84ee in alc269_fixup_tbl.

> diff --git a/sound/hda/codecs/realtek/alc269.c b/sound/hda/codecs/realtek/alc269.c
> index 67ce5b130f81f..3a1b664fca0ef 100644
> --- a/sound/hda/codecs/realtek/alc269.c
> +++ b/sound/hda/codecs/realtek/alc269.c
> @@ -2902,6 +2902,20 @@ static void alc225_fixup_s3_pop_noise(struct hda_codec *codec,
>  	codec->power_save_node = 1;
>  }
>  
> +static void alc225_fixup_hp_aio_spk(struct hda_codec *codec,
> +				    const struct hda_fixup *fix, int action)
> +{
> +	if (action != HDA_FIXUP_ACT_INIT)
> +		return;
> +
> +	snd_hda_codec_write(codec, 0x1b, 0,
> +			    AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_MUTE);
> +	alc_write_coef_idx(codec, 0x10, 0x0220);
> +	msleep(75);
> +	snd_hda_codec_write(codec, 0x1b, 0,
> +			    AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_UNMUTE);
> +}

[Severity: Medium]
Does unconditionally writing AMP_OUT_UNMUTE via snd_hda_codec_write() here
override the mixer or headphone auto-mute state and desynchronize
codec->core.regmap on init and resume?

During codec initialization in sound/hda/codecs/realtek/realtek.c:alc_init(),
snd_hda_gen_init() runs before HDA_FIXUP_ACT_INIT fixups are applied:

sound/hda/codecs/realtek/realtek.c:alc_init() {
	...
	snd_hda_gen_init(codec);
	alc_fix_pll(codec);
	alc_auto_init_amp(codec, spec->init_amp);
	snd_hda_apply_verbs(codec); /* apply verbs here after own init */

	snd_hda_apply_fixup(codec, HDA_FIXUP_ACT_INIT);
	...
}

Inside sound/hda/codecs/generic.c:snd_hda_gen_init(), the output paths and
auto-mute state are initialized in codec->core.regmap and flushed to hardware:

sound/hda/codecs/generic.c:snd_hda_gen_init() {
	...
	/* call init functions of standard auto-mute helpers */
	update_automute_all(codec);

	snd_hda_regmap_sync(codec);
}

When alc225_fixup_hp_aio_spk() then runs at HDA_FIXUP_ACT_INIT, calling
snd_hda_codec_write() sends raw verbs via snd_hdac_codec_write(), bypassing
codec->core.regmap. If the Speaker or Master Playback Switch was muted (or
auto-muted by headphones), hardware pin 0x1b is left unmuted while regmap
still caches HDA_AMP_MUTE. Because snd_hda_gen_init() already cleared the
regmap dirty state, the trailing snd_hda_regmap_sync() in alc269_resume() is
a no-op, and subsequent userspace attempts to mute via
snd_hda_mixer_amp_switch_put() -> snd_hda_codec_amp_update() see no change
against the cached value and suppress the hardware write.

Should alc225_fixup_hp_aio_spk() restore pin 0x1b to its cached regmap amp
state instead of unconditionally writing AMP_OUT_UNMUTE?

[ ... ]

> @@ -7737,6 +7747,7 @@ static const struct hda_quirk alc269_fixup_tbl[] = {
>  	SND_PCI_QUIRK(0x103c, 0x854a, "HP EliteBook 830 G6", ALC285_FIXUP_HP_GPIO_LED),
>  	SND_PCI_QUIRK(0x103c, 0x854d, "HP EliteBook 840 G6", ALC285_FIXUP_HP_GPIO_LED),
>  	SND_PCI_QUIRK(0x103c, 0x856a, "HP Pavilion 15-cs1xxx", ALC295_FIXUP_HP_PAVILION_MUTE_LED_1B),
> +	HDA_CODEC_QUIRK(0x103c, 0x84ee, "HP Pavilion AiO 24", ALC225_FIXUP_HP_AIO_SPK),

[Severity: Low]
Doesn't this add a duplicate quirk entry to alc269_fixup_tbl[]?

Commit fd47de58575a ("ALSA: hda/realtek: Add quirk for HP Pavilion AiO 24
(ALC225)") already added an identical entry further down in
alc269_fixup_tbl[]:

	SND_PCI_QUIRK(0x103c, 0x8e43, "HP Laptop 15-fd2xxx", ALC236_FIXUP_HP_MUTE_LED_MICMUTE_GPIO),
	HDA_CODEC_QUIRK(0x103c, 0x84ee, "HP Pavilion AiO 24", ALC225_FIXUP_HP_AIO_SPK),
	SND_PCI_QUIRK(0x103c, 0x8e60, "HP OmniBook 7 Laptop 16-bh0xxx", ALC245_FIXUP_CS35L41_I2C_2_MUTE_LED),

In addition, placing 0x84ee after 0x856a leaves it out of ascending subdevice
ID order (right after 0x84e7). Should the pre-existing misplaced entry be
removed when moving it to its sorted position?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029055945.188723-1-zhangheng@kylinos.cn?part=2