Re: [PATCH 2/2] ASoC: amd: acp: es83xx: Enable amplifiers after the codec output

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] ASoC: amd: acp: Headphone widget still powers up before
  ES8316 output drivers
- [Low] ASoC: amd: acp: missing Fixes trailer for commit 54fcd9dd44b2
- [Low] ASoC: amd: acp: coding style violation on if/else braces in
  power event handlers
--

commit e34ce394ae054b24a35c55d4522e17f836ad0f9d
Author: Samuel Leyva <samuelslg03idk@gmail.com>

> ASoC: amd: acp: es83xx: Enable amplifiers after the codec output
>
> The speaker and headphone amplifier GPIOs are driven from SUPPLY
> widgets. DAPM powers supplies before the rest of the path, so the
> amplifiers are enabled while the ES8316 headphone driver and charge
> pump are still powering up, which results in an audible pop every
> time the output is powered on.
>
> Move the GPIO handling to the Speaker and Headphone endpoint widgets,
> which DAPM powers up last and powers down first, and give the codec
> output a short time to settle before enabling the amplifier.
>
> Tested on a Huawei MateBook D 14 (NBM-WXX9).
>
> Signed-off-by: Samuel Leyva <samuelslg03idk@gmail.com>

[Severity: Low]
This fixes an audible pop on output power-up from the original machine
driver.  Should it include a Fixes tag for 54fcd9dd44b2 ("ASoC: amd: acp:
Add machine driver that enables sound for systems with a ES8336 codec")?

> diff --git a/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c b/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
> index fb675774fbff8..0a05f1f08663b 100644
> --- a/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
> +++ b/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c

[ ... ]

> @@ -114,23 +117,15 @@ static struct snd_soc_jack_pin es83xx_jack_pins[] = {
>  };
>  
>  static const struct snd_soc_dapm_widget acp3x_es83xx_widgets[] = {
> -	SND_SOC_DAPM_SPK("Speaker", NULL),
> -	SND_SOC_DAPM_HP("Headphone", NULL),
> +	SND_SOC_DAPM_SPK("Speaker", acp3x_es83xx_speaker_power_event),
> +	SND_SOC_DAPM_HP("Headphone", acp3x_es83xx_headphone_power_event),

[Severity: Medium]
Does the "Headphone" widget still power up before the ES8316 headphone
output drivers and charge pumps?

Unlike snd_soc_dapm_spk (rank 13), snd_soc_dapm_hp shares power-up sequence
rank 12 with snd_soc_dapm_out_drv:

sound/soc/soc-dapm.c:dapm_up_seq[] {
    ...
	[snd_soc_dapm_out_drv] = 12,
	[snd_soc_dapm_hp] = 12,
	[snd_soc_dapm_line] = 12,
	[snd_soc_dapm_sink] = 12,
	[snd_soc_dapm_spk] = 13,
    ...
}

In es8316_dapm_widgets[], "Left Headphone Charge Pump", "Right Headphone
Charge Pump", "Left Headphone Driver", and "Right Headphone Driver" are
SND_SOC_DAPM_OUT_DRV widgets with subseq 0 and reg ES8316_CPHP_OUTEN (0x17),
whereas SND_SOC_DAPM_HP("Headphone", ...) has subseq 0 and reg SND_SOC_NOPM
(-1).  When rank and subseq match, dapm_seq_compare() sorts by a->reg -
b->reg, placing "Headphone" (-1) before the ES8316 OUT_DRV widgets (0x17).
As a result, acp3x_es83xx_headphone_power_event() runs its 50 ms delay and
enables priv->gpio_headphone before ES8316_CPHP_OUTEN is written.

[ ... ]

> @@ -168,8 +162,11 @@ static int acp3x_es83xx_headphone_power_event(struct snd_soc_dapm_widget *w,
>  	struct acp3x_es83xx_private *priv = get_mach_priv(card);
>  
>  	dev_dbg(priv->codec_dev, "headphone power event = %d\n", event);
> -	if (SND_SOC_DAPM_EVENT_ON(event))
> +	if (SND_SOC_DAPM_EVENT_ON(event)) {
> +		/* Let the codec output settle before enabling the amplifier */
> +		msleep(ES83XX_AMP_ON_DELAY_MS);
>  		priv->headphone_on = true;
> +	}
>  	else
>  		priv->headphone_on = false;
>  
> @@ -186,8 +183,11 @@ static int acp3x_es83xx_speaker_power_event(struct snd_soc_dapm_widget *w,
>  	struct acp3x_es83xx_private *priv = get_mach_priv(card);
>  
>  	dev_dbg(priv->codec_dev, "speaker power event: %d\n", event);
> -	if (SND_SOC_DAPM_EVENT_ON(event))
> +	if (SND_SOC_DAPM_EVENT_ON(event)) {
> +		/* Let the codec output settle before enabling the amplifier */
> +		msleep(ES83XX_AMP_ON_DELAY_MS);
>  		priv->speaker_on = true;
> +	}
>  	else
>  		priv->speaker_on = false;

[Severity: Low]
This isn't a bug, but kernel coding style uses "} else {" with braces on
both branches when one branch of an if/else statement is multi-line.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107214714.11392-1-samuelslg03idk@gmail.com?part=2