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

[sashiko-bot@kernel.org]

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