[PATCH 0/2] ASoC: amd: acp: Support Huawei MateBook D 14 (NBM-WXX9)

[PATCH 0/2] ASoC: amd: acp: Support Huawei MateBook D 14 (NBM-WXX9)

[Samuel Leyva]

The Huawei MateBook D 14 AMD (NBM-WXX9) has the same ES8336 + ACP3x
audio layout as the already supported BOM-WXX9, but is missing from
the DMI tables, so only the DMIC-only driver binds and the speakers
do not work.

Patch 1 adds the DMI quirk.

Patch 2 fixes a pop heard every time the output powers up: the
amplifier GPIOs were driven from SUPPLY widgets, which DAPM enables
before the codec output. It is independent of patch 1.

Both were tested as an out-of-tree DKMS module on the NBM-WXX9.

Samuel Leyva (2):
  ASoC: amd: acp: Add DMI quirk for Huawei MateBook D 14 (NBM-WXX9)
  ASoC: amd: acp: es83xx: Enable amplifiers after the codec output

 sound/soc/amd/acp-config.c                    | 14 ++++++++
 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c | 34 ++++++++++++-------
 2 files changed, 35 insertions(+), 13 deletions(-)

-- 
2.56.0

[PATCH 1/2] ASoC: amd: acp: Add DMI quirk for Huawei MateBook D 14 (NBM-WXX9)

[Samuel Leyva]

The Huawei MateBook D 14 AMD (NBM-WXX9, Lucienne) uses an Everest
ES8336 codec connected to the ACP3x, like the already supported
BOM-WXX9. Without a DMI match the ACP falls back to the DMIC-only
driver, so the speakers and the headphone jack do not work.

Add the system to the ACP config table and to the acp3x-es83xx quirk
table, with the same quirks as BOM-WXX9 (internal DMIC and 48 MHz
MCLK).

Tested speakers and internal microphone; the headphone jack could not
be tested.

Signed-off-by: Samuel Leyva <samuelslg03idk@gmail.com>
---
 sound/soc/amd/acp-config.c                    | 14 ++++++++++++++
 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c |  8 ++++++++
 2 files changed, 22 insertions(+)

diff --git a/sound/soc/amd/acp-config.c b/sound/soc/amd/acp-config.c
index 3d4c8526c..32c4d6319 100644
--- a/sound/soc/amd/acp-config.c
+++ b/sound/soc/amd/acp-config.c
@@ -237,6 +237,20 @@ static const struct config_entry config_table[] = {
 			{}
 		},
 	},
+	{
+		.flags = FLAG_AMD_LEGACY,
+		.device = ACP_PCI_DEV_ID,
+		.dmi_table = (const struct dmi_system_id []) {
+			{
+				.matches = {
+					DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "HUAWEI"),
+					DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "NBM-WXX9"),
+					DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "M1010"),
+				},
+			},
+			{}
+		},
+	},
 	{
 		.flags = FLAG_AMD_LEGACY,
 		.device = ACP_PCI_DEV_ID,
diff --git a/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c b/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
index eb0e9a6af..fb675774f 100644
--- a/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
+++ b/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
@@ -364,6 +364,14 @@ static const struct dmi_system_id acp3x_es83xx_dmi_table[] = {
 		},
 		.driver_data = (void *)(ES83XX_ENABLE_DMIC|ES83XX_48_MHZ_MCLK),
 	},
+	{
+		.matches = {
+			DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "HUAWEI"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "NBM-WXX9"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "M1010"),
+		},
+		.driver_data = (void *)(ES83XX_ENABLE_DMIC|ES83XX_48_MHZ_MCLK),
+	},
 	{
 		.matches = {
 			DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "HUAWEI"),
-- 
2.56.0

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

[Samuel Leyva]

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>
---
 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c | 26 +++++++++----------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c b/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
index fb675774f..0a05f1f08 100644
--- a/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
+++ b/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
@@ -18,6 +18,7 @@
 #include <linux/input.h>
 #include <linux/io.h>
 #include <linux/acpi.h>
+#include <linux/delay.h>
 #include <linux/dmi.h>
 #include <linux/string_choices.h>
 #include "../acp-mach.h"
@@ -60,6 +61,8 @@ static const struct snd_pcm_hw_constraint_list constraints_channels = {
 #define ES83xx_12288_KHZ_MCLK_FREQ   (48000 * 256)
 #define ES83xx_48_MHZ_MCLK_FREQ      (48000 * 1000)
 
+#define ES83XX_AMP_ON_DELAY_MS	50
+
 static int acp3x_es83xx_headphone_power_event(struct snd_soc_dapm_widget *w,
 					    struct snd_kcontrol *kcontrol, int event);
 static int acp3x_es83xx_speaker_power_event(struct snd_soc_dapm_widget *w,
@@ -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),
 	SND_SOC_DAPM_MIC("Headset Mic", NULL),
 	SND_SOC_DAPM_MIC("Internal Mic", NULL),
-
-	SND_SOC_DAPM_SUPPLY("Headphone Power", SND_SOC_NOPM, 0, 0,
-			    acp3x_es83xx_headphone_power_event,
-			    SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMU),
-	SND_SOC_DAPM_SUPPLY("Speaker Power", SND_SOC_NOPM, 0, 0,
-			    acp3x_es83xx_speaker_power_event,
-			    SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMU),
 };
 
 static const struct snd_soc_dapm_route acp3x_es83xx_audio_map[] = {
 	{"Headphone", NULL, "HPOL"},
 	{"Headphone", NULL, "HPOR"},
-	{"Headphone", NULL, "Headphone Power"},
 
 	/*
 	 * There is no separate speaker output instead the speakers are muxed to
@@ -138,7 +133,6 @@ static const struct snd_soc_dapm_route acp3x_es83xx_audio_map[] = {
 	 */
 	{"Speaker", NULL, "HPOL"},
 	{"Speaker", NULL, "HPOR"},
-	{"Speaker", NULL, "Speaker Power"},
 };
 
 
@@ -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;
 
-- 
2.56.0

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

[sashiko-bot]

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