[PATCH 1/2] ALSA: hda/realtek: Add speaker depop sequence to alc225_shutup

[PATCH 1/2] ALSA: hda/realtek: Add speaker depop sequence to alc225_shutup

[Zhang Heng]

Mute speaker pins and disable their pin widgets, with a 75 ms delay
after each step, before cutting EAPD in alc225_shutup().  This
prevents the pop noise that occurs on suspend when the amplifier
loses its input signal while still powered.

Reported-by: John Doe <coolcrew45@disroot.org>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=221616
Tested-by: John Doe <coolcrew45@disroot.org>
Signed-off-by: Zhang Heng <zhangheng@kylinos.cn>
---
 sound/hda/codecs/realtek/alc269.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sound/hda/codecs/realtek/alc269.c b/sound/hda/codecs/realtek/alc269.c
index 23329d5887a5..67ce5b130f81 100644
--- a/sound/hda/codecs/realtek/alc269.c
+++ b/sound/hda/codecs/realtek/alc269.c
@@ -665,6 +665,7 @@ static void alc225_shutup(struct hda_codec *codec)
 	struct alc_spec *spec = codec->spec;
 	hda_nid_t hp_pin = alc_get_hp_pin(spec);
 	bool hp1_pin_sense, hp2_pin_sense;
+	int i;
 
 	if (!hp_pin)
 		hp_pin = 0x21;
@@ -698,6 +699,16 @@ static void alc225_shutup(struct hda_codec *codec)
 		alc_update_coef_idx(codec, 0x4a, 3 << 10, 0);
 		alc_enable_headset_jack_key(codec);
 	}
+
+	for (i = 0; i < spec->gen.autocfg.speaker_outs; i++)
+		snd_hda_codec_write(codec, spec->gen.autocfg.speaker_pins[i], 0,
+				    AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_MUTE);
+	msleep(75);
+	for (i = 0; i < spec->gen.autocfg.speaker_outs; i++)
+		snd_hda_codec_write(codec, spec->gen.autocfg.speaker_pins[i], 0,
+				    AC_VERB_SET_PIN_WIDGET_CONTROL, 0x0);
+	msleep(75);
+
 	alc_auto_setup_eapd(codec, false);
 	alc_shutup_pins(codec);
 	if (spec->ultra_low_power) {
-- 
2.25.1

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

[Zhang Heng]

Replace the static coef-0x10 verb table with a function that mutes
pin 0x1b, writes coef 0x10, waits 75 ms, then unmutes — eliminating
the pop when the internal amplifier enables at init time.  Add
SSID 103c:84ee to the quirk table.

Reported-by: John Doe <coolcrew45@disroot.org>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221616
Tested-by: John Doe <coolcrew45@disroot.org>
Signed-off-by: Zhang Heng <zhangheng@kylinos.cn>
---
 sound/hda/codecs/realtek/alc269.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/sound/hda/codecs/realtek/alc269.c b/sound/hda/codecs/realtek/alc269.c
index 67ce5b130f81..3a1b664fca0e 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);
+}
+
 /* Forcibly assign NID 0x03 to HP/LO while NID 0x02 to SPK for EQ */
 static void alc274_fixup_bind_dacs(struct hda_codec *codec,
 				    const struct hda_fixup *fix, int action)
@@ -5987,12 +6001,8 @@ static const struct hda_fixup alc269_fixups[] = {
 		.chain_id = ALC269_FIXUP_HEADSET_MODE_NO_HP_MIC
 	},
 	[ALC225_FIXUP_HP_AIO_SPK] = {
-		.type = HDA_FIXUP_VERBS,
-		.v.verbs = (const struct hda_verb[]) {
-			{ 0x20, AC_VERB_SET_COEF_INDEX, 0x10 },
-			{ 0x20, AC_VERB_SET_PROC_COEF, 0x0220 },
-			{ }
-		},
+		.type = HDA_FIXUP_FUNC,
+		.v.func = alc225_fixup_hp_aio_spk,
 	},
 	[ALC286_FIXUP_ACER_AIO_HEADSET_MIC] = {
 		.type = HDA_FIXUP_VERBS,
@@ -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),
 	HDA_CODEC_QUIRK(0x103c, 0x85ba, "HP Pavilion AiO 24", ALC225_FIXUP_HP_AIO_SPK),
 	SND_PCI_QUIRK(0x103c, 0x85c6, "HP Pavilion x360 Convertible 14-dy1xxx", ALC295_FIXUP_HP_MUTE_LED_COEFBIT11),
 	SND_PCI_QUIRK(0x103c, 0x85de, "HP Envy x360 13-ar0xxx", ALC285_FIXUP_HP_ENVY_X360),
-- 
2.25.1

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

[sashiko-bot]

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

Re: [PATCH 1/2] ALSA: hda/realtek: Add speaker depop sequence to alc225_shutup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ALSA: hda/realtek: alc225_shutup() ignores spec->no_shutup_pins on speakers
- [Medium] ALSA: hda/realtek: alc225_shutup() misses auto_cfg_speaker_pins() and sleeps
--

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

ALSA: hda/realtek: Add speaker depop sequence to alc225_shutup

This commit mutes speaker pins and disables their pin widgets with 75 ms
delays before cutting EAPD in alc225_shutup() to prevent pop noise on
suspend.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=221616

> diff --git a/sound/hda/codecs/realtek/alc269.c b/sound/hda/codecs/realtek/alc269.c
> index 23329d5887a56..67ce5b130f81f 100644
> --- a/sound/hda/codecs/realtek/alc269.c
> +++ b/sound/hda/codecs/realtek/alc269.c
> @@ -665,6 +665,7 @@ static void alc225_shutup(struct hda_codec *codec)
>  	struct alc_spec *spec = codec->spec;
>  	hda_nid_t hp_pin = alc_get_hp_pin(spec);
>  	bool hp1_pin_sense, hp2_pin_sense;
> +	int i;
>  
>  	if (!hp_pin)
>  		hp_pin = 0x21;
> @@ -698,6 +699,16 @@ static void alc225_shutup(struct hda_codec *codec)
>  		alc_update_coef_idx(codec, 0x4a, 3 << 10, 0);
>  		alc_enable_headset_jack_key(codec);
>  	}
> +
> +	for (i = 0; i < spec->gen.autocfg.speaker_outs; i++)
> +		snd_hda_codec_write(codec, spec->gen.autocfg.speaker_pins[i], 0,
> +				    AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_MUTE);
> +	msleep(75);
> +	for (i = 0; i < spec->gen.autocfg.speaker_outs; i++)
> +		snd_hda_codec_write(codec, spec->gen.autocfg.speaker_pins[i], 0,
> +				    AC_VERB_SET_PIN_WIDGET_CONTROL, 0x0);

[Severity: Medium]
Should this loop check !spec->no_shutup_pins before clearing
AC_VERB_SET_PIN_WIDGET_CONTROL on spec->gen.autocfg.speaker_pins[i]?

Previously, alc225_shutup() only cleared speaker pin widget controls via
alc_shutup_pins() in sound/hda/codecs/realtek/realtek.c:

void alc_shutup_pins(struct hda_codec *codec)
{
	struct alc_spec *spec = codec->spec;

	if (spec->no_shutup_pins)
		return;

Other shutup callbacks in sound/hda/codecs/realtek/alc269.c, such as
alc256_shutup(), alc282_shutup(), alc283_shutup(), and alc_default_shutup(),
also guard AC_VERB_SET_PIN_WIDGET_CONTROL = 0x0 writes with
if (!spec->no_shutup_pins).

Several quirks for codecs using alc225_shutup() (such as ALC285, ALC287, and
ALC295) set spec->no_shutup_pins via alc_fixup_no_shutup(), including
alc_fixup_thinkpad_acpi() and alc295_fixup_dell_inspiron_top_speakers().
When hda_call_codec_suspend() -> alc269_suspend() -> alc_suspend() ->
alc_shutup() -> alc225_shutup() runs on those machines, clearing the speaker
pin widget controls unconditionally bypasses the no_shutup_pins quirk while
alc_shutup_pins() still returns early without setting codec->pins_shutup, so
restore_shutup_pins() will also skip restoring the pins on early resume.

> +	msleep(75);

[Severity: Medium]
What happens in alc225_shutup() when spec->gen.autocfg.speaker_outs is 0, or
when the speaker pins were moved to line_out_pins by
snd_hda_parse_pin_defcfg()?

In sound/hda/common/auto_parser.c, snd_hda_parse_pin_defcfg() moves
speaker_pins into line_out_pins when no line-out pins are present:

		if (cfg->speaker_outs) {
			cfg->line_outs = cfg->speaker_outs;
			memcpy(cfg->line_out_pins, cfg->speaker_pins,
			       sizeof(cfg->speaker_pins));
			cfg->speaker_outs = 0;
			memset(cfg->speaker_pins, 0, sizeof(cfg->speaker_pins));
			cfg->line_out_type = AUTO_PIN_SPEAKER_OUT;
		}

And in sound/hda/codecs/generic.c, check_auto_mute_availability() only copies
them back to speaker_pins if at least two different output types are present
and suppress_auto_mute is not set. Otherwise, speaker_outs remains 0 while the
speaker pins reside in line_out_pins.

Furthermore, because both msleep(75) calls run outside any speaker count
check, systems with no speakers (or with speaker_outs == 0) will still incur
150 ms of unconditional sleep on every runtime PM and system suspend.

Would it make sense to use auto_cfg_speaker_outs() and
auto_cfg_speaker_pins() from sound/hda/common/hda_auto_parser.h (similar to
snd_hda_gen_shutup_speakers()), and only run the msleep(75) delays when there
are speaker pins to shut up?

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