[PATCH 0/4] ASoC: amd: acp: Add sound support for a line of HUAWEI laptops

[PATCH 0/4] ASoC: amd: acp: Add sound support for a line of HUAWEI laptops

[Marian Postevca]

This series adds support for a line of HUAWEI laptops with
AMD CPUs that connect using the ACP3x module to a ES8336 codec.

The codec driver must be extended to support the S32 LE format
and the MCLK div by 2 option. MCLK div by 2 is needed for one specific
SKU which uses a 48Mhz MCLK which seems to be too high of a frequency
for the codec and must be divided by 2.

The acp legacy driver must also be extended by using callbacks so that
the more complicated handling for this specific codec can be moved
outside the more generic ACP code.

The last patch tries to avoid anoying pop sounds when the speaker/headphones
are enabled/disabled by delaying the handling of the GPIOs and using a mutex
to avoid race conditions between the speaker power event callback and the
trigger callback.

Marian Postevca (4):
  ASoC: es8316: Enable support for S32 LE format and MCLK div by 2
  ASoC: amd: acp: Add support for splitting the codec specific code from
    the ACP driver
  ASoC: amd: acp: Add machine driver that enables sound for systems with
    a ES8336 codec
  ASoC: amd: acp: Improve support for speaker power events

 sound/soc/amd/acp-config.c                    |  70 ++
 sound/soc/amd/acp/Makefile                    |   2 +-
 sound/soc/amd/acp/acp-legacy-mach.c           | 105 ++-
 sound/soc/amd/acp/acp-mach-common.c           |   8 +
 sound/soc/amd/acp/acp-mach.h                  |  67 ++
 sound/soc/amd/acp/acp-renoir.c                |   4 +
 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c | 615 ++++++++++++++++++
 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.h |  12 +
 sound/soc/codecs/es8316.c                     |  21 +-
 sound/soc/codecs/es8316.h                     |   3 +
 10 files changed, 886 insertions(+), 21 deletions(-)
 create mode 100644 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
 create mode 100644 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.h

-- 
2.39.1

[PATCH 1/4] ASoC: es8316: Enable support for S32 LE format and MCLK div by 2

[Marian Postevca]

To properly support a line of Huawei laptops with AMD CPU and a
ES8336 codec connected to the ACP3X module we need to enable
the S32 LE format and the codec option to divide the MCLK by 2.

The option to divide the MCLK will be enabled for one SKU with a
48Mhz MCLK. This frequency seems to be too high for the codec and
leads to distorted sounds when the option is not enabled.

Signed-off-by: Marian Postevca <posteuca@mutex.one>
---
 sound/soc/codecs/es8316.c | 21 +++++++++++++++++----
 sound/soc/codecs/es8316.h |  3 +++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/es8316.c b/sound/soc/codecs/es8316.c
index 056c3082fe02..acf21ef59b34 100644
--- a/sound/soc/codecs/es8316.c
+++ b/sound/soc/codecs/es8316.c
@@ -26,10 +26,11 @@
 /* In slave mode at single speed, the codec is documented as accepting 5
  * MCLK/LRCK ratios, but we also add ratio 400, which is commonly used on
  * Intel Cherry Trail platforms (19.2MHz MCLK, 48kHz LRCK).
+ * Ratio 1000 is needed for at least one SKU where MCLK is 48Mhz.
  */
-#define NR_SUPPORTED_MCLK_LRCK_RATIOS 6
+#define NR_SUPPORTED_MCLK_LRCK_RATIOS 7
 static const unsigned int supported_mclk_lrck_ratios[] = {
-	256, 384, 400, 512, 768, 1024
+	256, 384, 400, 512, 768, 1000, 1024
 };
 
 struct es8316_priv {
@@ -465,6 +466,8 @@ static int es8316_pcm_hw_params(struct snd_pcm_substream *substream,
 	u8 bclk_divider;
 	u16 lrck_divider;
 	int i;
+	bool mclk_div_option = false;
+	unsigned int mclk_div = 1;
 
 	/* Validate supported sample rates that are autodetected from MCLK */
 	for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) {
@@ -477,7 +480,17 @@ static int es8316_pcm_hw_params(struct snd_pcm_substream *substream,
 	}
 	if (i == NR_SUPPORTED_MCLK_LRCK_RATIOS)
 		return -EINVAL;
-	lrck_divider = es8316->sysclk / params_rate(params);
+
+	mclk_div_option = device_property_read_bool(component->dev,
+						    "everest,mclk-div-by-2");
+	if (mclk_div_option) {
+		snd_soc_component_update_bits(component, ES8316_CLKMGR_CLKSW,
+					      ES8316_CLKMGR_CLKSW_MCLK_DIV,
+					      ES8316_CLKMGR_CLKSW_MCLK_DIV);
+		mclk_div = 2;
+	}
+
+	lrck_divider = es8316->sysclk / params_rate(params) / mclk_div;
 	bclk_divider = lrck_divider / 4;
 	switch (params_format(params)) {
 	case SNDRV_PCM_FORMAT_S16_LE:
@@ -520,7 +533,7 @@ static int es8316_mute(struct snd_soc_dai *dai, int mute, int direction)
 }
 
 #define ES8316_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \
-			SNDRV_PCM_FMTBIT_S24_LE)
+			SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
 
 static const struct snd_soc_dai_ops es8316_ops = {
 	.startup = es8316_pcm_startup,
diff --git a/sound/soc/codecs/es8316.h b/sound/soc/codecs/es8316.h
index c335138e2837..0ff16f948690 100644
--- a/sound/soc/codecs/es8316.h
+++ b/sound/soc/codecs/es8316.h
@@ -129,4 +129,7 @@
 #define ES8316_GPIO_FLAG_GM_NOT_SHORTED		0x02
 #define ES8316_GPIO_FLAG_HP_NOT_INSERTED	0x04
 
+/* ES8316_CLKMGR_CLKSW */
+#define ES8316_CLKMGR_CLKSW_MCLK_DIV	0x80
+
 #endif
-- 
2.39.1

[PATCH 2/4] ASoC: amd: acp: Add support for splitting the codec specific code from the ACP driver

[Marian Postevca]

This commit adds support for splitting more complicated machine drivers,
that need special handling, from the generic ACP code.

By adding support for callbacks to configure and handle codec specific
implementation details, we can split them in separate files that don't
clutter the ACP code.

Signed-off-by: Marian Postevca <posteuca@mutex.one>
---
 sound/soc/amd/acp/acp-mach.h | 65 ++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h
index 165f407697c0..2cade68e6cc3 100644
--- a/sound/soc/amd/acp/acp-mach.h
+++ b/sound/soc/amd/acp/acp-mach.h
@@ -20,6 +20,10 @@
 
 #define TDM_CHANNELS	8
 
+#define ACP_OPS(priv, cb)	((priv)->ops.cb)
+
+#define acp_get_drvdata(card) ((struct acp_card_drvdata *)(card)->drvdata)
+
 enum be_id {
 	HEADSET_BE_ID = 0,
 	AMP_BE_ID,
@@ -48,6 +52,14 @@ enum platform_end_point {
 	REMBRANDT,
 };
 
+struct acp_mach_ops {
+	int (*probe)(struct snd_soc_card *card);
+	int (*configure_link)(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link);
+	int (*configure_widgets)(struct snd_soc_card *card);
+	int (*suspend_pre)(struct snd_soc_card *card);
+	int (*resume_post)(struct snd_soc_card *card);
+};
+
 struct acp_card_drvdata {
 	unsigned int hs_cpu_id;
 	unsigned int amp_cpu_id;
@@ -59,6 +71,8 @@ struct acp_card_drvdata {
 	unsigned int platform;
 	struct clk *wclk;
 	struct clk *bclk;
+	struct acp_mach_ops ops;
+	void *mach_priv;
 	bool soc_mclk;
 	bool tdm_mode;
 };
@@ -67,4 +81,55 @@ int acp_sofdsp_dai_links_create(struct snd_soc_card *card);
 int acp_legacy_dai_links_create(struct snd_soc_card *card);
 extern const struct dmi_system_id acp_quirk_table[];
 
+static inline int acp_ops_probe(struct snd_soc_card *card)
+{
+	int ret = 1;
+	struct acp_card_drvdata *priv = acp_get_drvdata(card);
+
+	if (ACP_OPS(priv, probe))
+		ret = ACP_OPS(priv, probe)(card);
+	return ret;
+}
+
+static inline int acp_ops_configure_link(struct snd_soc_card *card,
+					 struct snd_soc_dai_link *dai_link)
+{
+	int ret = 1;
+	struct acp_card_drvdata *priv = acp_get_drvdata(card);
+
+	if (ACP_OPS(priv, configure_link))
+		ret = ACP_OPS(priv, configure_link)(card, dai_link);
+	return ret;
+}
+
+static inline int acp_ops_configure_widgets(struct snd_soc_card *card)
+{
+	int ret = 1;
+	struct acp_card_drvdata *priv = acp_get_drvdata(card);
+
+	if (ACP_OPS(priv, configure_widgets))
+		ret = ACP_OPS(priv, configure_widgets)(card);
+	return ret;
+}
+
+static inline int acp_ops_suspend_pre(struct snd_soc_card *card)
+{
+	int ret = 1;
+	struct acp_card_drvdata *priv = acp_get_drvdata(card);
+
+	if (ACP_OPS(priv, suspend_pre))
+		ret = ACP_OPS(priv, suspend_pre)(card);
+	return ret;
+}
+
+static inline int acp_ops_resume_post(struct snd_soc_card *card)
+{
+	int ret = 1;
+	struct acp_card_drvdata *priv = acp_get_drvdata(card);
+
+	if (ACP_OPS(priv, resume_post))
+		ret = ACP_OPS(priv, resume_post)(card);
+	return ret;
+}
+
 #endif
-- 
2.39.1

[PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Marian Postevca]

This commit enables sound for a line of Huawei laptops that use
the ES8336 codec which is connected to the ACP3X module.

Signed-off-by: Marian Postevca <posteuca@mutex.one>
---
 sound/soc/amd/acp-config.c                    |  70 +++
 sound/soc/amd/acp/Makefile                    |   2 +-
 sound/soc/amd/acp/acp-legacy-mach.c           | 105 +++-
 sound/soc/amd/acp/acp-mach-common.c           |   8 +
 sound/soc/amd/acp/acp-mach.h                  |   2 +
 sound/soc/amd/acp/acp-renoir.c                |   4 +
 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c | 471 ++++++++++++++++++
 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.h |  12 +
 8 files changed, 657 insertions(+), 17 deletions(-)
 create mode 100644 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
 create mode 100644 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.h

diff --git a/sound/soc/amd/acp-config.c b/sound/soc/amd/acp-config.c
index 0932473b6394..76e65c0ad140 100644
--- a/sound/soc/amd/acp-config.c
+++ b/sound/soc/amd/acp-config.c
@@ -47,6 +47,76 @@ 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, "KLVL-WXXW"),
+					DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "M1010"),
+				},
+			},
+			{}
+		},
+	},
+	{
+		.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, "KLVL-WXX9"),
+					DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "M1010"),
+				},
+			},
+			{}
+		},
+	},
+	{
+		.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, "BOM-WXX9"),
+					DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "M1010"),
+				},
+			},
+			{}
+		},
+	},
+	{
+		.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, "HVY-WXX9"),
+					DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "M1020"),
+				},
+			},
+			{}
+		},
+	},
+	{
+		.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, "HVY-WXX9"),
+					DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "M1040"),
+				},
+			},
+			{}
+		},
+	},
 };
 
 int snd_amd_acp_find_config(struct pci_dev *pci)
diff --git a/sound/soc/amd/acp/Makefile b/sound/soc/amd/acp/Makefile
index d9abb0ee5218..1bf64a9289a2 100644
--- a/sound/soc/amd/acp/Makefile
+++ b/sound/soc/amd/acp/Makefile
@@ -16,7 +16,7 @@ snd-acp-rembrandt-objs  := acp-rembrandt.o
 
 #machine specific driver
 snd-acp-mach-objs     := acp-mach-common.o
-snd-acp-legacy-mach-objs     := acp-legacy-mach.o
+snd-acp-legacy-mach-objs     := acp-legacy-mach.o acp3x-es83xx/acp3x-es83xx.o
 snd-acp-sof-mach-objs     := acp-sof-mach.o
 
 obj-$(CONFIG_SND_SOC_AMD_ACP_PCM) += snd-acp-pcm.o
diff --git a/sound/soc/amd/acp/acp-legacy-mach.c b/sound/soc/amd/acp/acp-legacy-mach.c
index 676ad50638d0..8ecda4780d4a 100644
--- a/sound/soc/amd/acp/acp-legacy-mach.c
+++ b/sound/soc/amd/acp/acp-legacy-mach.c
@@ -20,6 +20,7 @@
 #include <linux/module.h>
 
 #include "acp-mach.h"
+#include "acp3x-es83xx/acp3x-es83xx.h"
 
 static struct acp_card_drvdata rt5682_rt1019_data = {
 	.hs_cpu_id = I2S_SP,
@@ -51,6 +52,14 @@ static struct acp_card_drvdata rt5682s_rt1019_data = {
 	.tdm_mode = false,
 };
 
+static struct acp_card_drvdata es83xx_rn_data = {
+	.hs_cpu_id = I2S_SP,
+	.dmic_cpu_id = DMIC,
+	.hs_codec_id = ES83XX,
+	.dmic_codec_id = DMIC,
+	.platform = RENOIR,
+};
+
 static struct acp_card_drvdata max_nau8825_data = {
 	.hs_cpu_id = I2S_HS,
 	.amp_cpu_id = I2S_HS,
@@ -92,6 +101,33 @@ static const struct snd_soc_dapm_widget acp_widgets[] = {
 	SND_SOC_DAPM_SPK("Right Spk", NULL),
 };
 
+static bool acp_asoc_init_ops(struct acp_card_drvdata *priv)
+{
+	bool has_ops = false;
+
+	if (priv->hs_codec_id == ES83XX) {
+		has_ops = true;
+		acp3x_es83xx_init_ops(&priv->ops);
+	}
+	return has_ops;
+}
+
+static int acp_asoc_suspend_pre(struct snd_soc_card *card)
+{
+	int ret;
+
+	ret = acp_ops_suspend_pre(card);
+	return ret == 1 ? 0 : ret;
+}
+
+static int acp_asoc_resume_post(struct snd_soc_card *card)
+{
+	int ret;
+
+	ret = acp_ops_resume_post(card);
+	return ret == 1 ? 0 : ret;
+}
+
 static int acp_asoc_probe(struct platform_device *pdev)
 {
 	struct snd_soc_card *card = NULL;
@@ -100,38 +136,70 @@ static int acp_asoc_probe(struct platform_device *pdev)
 	struct acp_card_drvdata *acp_card_drvdata;
 	int ret;
 
-	if (!pdev->id_entry)
-		return -EINVAL;
+	if (!pdev->id_entry) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	card = devm_kzalloc(dev, sizeof(*card), GFP_KERNEL);
-	if (!card)
-		return -ENOMEM;
+	if (!card) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
+	card->drvdata = (struct acp_card_drvdata *)pdev->id_entry->driver_data;
+	acp_card_drvdata = card->drvdata;
+	acp_card_drvdata->acpi_mach = (struct snd_soc_acpi_mach *)pdev->dev.platform_data;
 	card->dev = dev;
 	card->owner = THIS_MODULE;
 	card->name = pdev->id_entry->name;
-	card->dapm_widgets = acp_widgets;
-	card->num_dapm_widgets = ARRAY_SIZE(acp_widgets);
-	card->controls = acp_controls;
-	card->num_controls = ARRAY_SIZE(acp_controls);
-	card->drvdata = (struct acp_card_drvdata *)pdev->id_entry->driver_data;
 
-	acp_card_drvdata = card->drvdata;
+	acp_asoc_init_ops(card->drvdata);
+
+	ret = acp_ops_configure_widgets(card);
+	if (ret == 1) {
+		card->dapm_widgets = acp_widgets;
+		card->num_dapm_widgets = ARRAY_SIZE(acp_widgets);
+		card->controls = acp_controls;
+		card->num_controls = ARRAY_SIZE(acp_controls);
+	} else if (ret < 0) {
+		dev_err(&pdev->dev,
+			"Cannot configure widgets for card (%s): %d\n",
+			card->name, ret);
+		goto out;
+	}
+	card->suspend_pre = acp_asoc_suspend_pre;
+	card->resume_post = acp_asoc_resume_post;
+
+	ret = acp_ops_probe(card);
+	if (ret < 0) {
+		dev_err(&pdev->dev,
+			"Cannot probe card (%s): %d\n",
+			card->name, ret);
+		goto out;
+	}
+
 	dmi_id = dmi_first_match(acp_quirk_table);
 	if (dmi_id && dmi_id->driver_data)
 		acp_card_drvdata->tdm_mode = dmi_id->driver_data;
 
-	acp_legacy_dai_links_create(card);
+	ret = acp_legacy_dai_links_create(card);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"Cannot create dai links for card (%s): %d\n",
+			card->name, ret);
+		goto out;
+	}
 
 	ret = devm_snd_soc_register_card(&pdev->dev, card);
 	if (ret) {
 		dev_err(&pdev->dev,
-				"devm_snd_soc_register_card(%s) failed: %d\n",
-				card->name, ret);
-		return ret;
+			"devm_snd_soc_register_card(%s) failed: %d\n",
+			card->name, ret);
+		goto out;
 	}
-
-	return 0;
+out:
+	return ret;
 }
 
 static const struct platform_device_id board_ids[] = {
@@ -147,6 +215,10 @@ static const struct platform_device_id board_ids[] = {
 		.name = "acp3xalc5682s1019",
 		.driver_data = (kernel_ulong_t)&rt5682s_rt1019_data,
 	},
+	{
+		.name = "acp3x-es83xx",
+		.driver_data = (kernel_ulong_t)&es83xx_rn_data,
+	},
 	{
 		.name = "rmb-nau8825-max",
 		.driver_data = (kernel_ulong_t)&max_nau8825_data,
@@ -173,6 +245,7 @@ MODULE_DESCRIPTION("ACP chrome audio support");
 MODULE_ALIAS("platform:acp3xalc56821019");
 MODULE_ALIAS("platform:acp3xalc5682sm98360");
 MODULE_ALIAS("platform:acp3xalc5682s1019");
+MODULE_ALIAS("platform:acp3x-es83xx");
 MODULE_ALIAS("platform:rmb-nau8825-max");
 MODULE_ALIAS("platform:rmb-rt5682s-rt1019");
 MODULE_LICENSE("GPL v2");
diff --git a/sound/soc/amd/acp/acp-mach-common.c b/sound/soc/amd/acp/acp-mach-common.c
index b4dcce4fbae9..16dfebfbb4b6 100644
--- a/sound/soc/amd/acp/acp-mach-common.c
+++ b/sound/soc/amd/acp/acp-mach-common.c
@@ -1053,6 +1053,7 @@ int acp_legacy_dai_links_create(struct snd_soc_card *card)
 	struct device *dev = card->dev;
 	struct acp_card_drvdata *drv_data = card->drvdata;
 	int i = 0, num_links = 0;
+	int rc;
 
 	if (drv_data->hs_cpu_id)
 		num_links++;
@@ -1091,6 +1092,13 @@ int acp_legacy_dai_links_create(struct snd_soc_card *card)
 			links[i].init = acp_card_rt5682s_init;
 			links[i].ops = &acp_card_rt5682s_ops;
 		}
+		if (drv_data->hs_codec_id == ES83XX) {
+			rc = acp_ops_configure_link(card, &links[i]);
+			if (rc != 0) {
+				dev_err(dev, "Failed to configure link for ES83XX: %d\n", rc);
+				return rc;
+			}
+		}
 		i++;
 	}
 
diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h
index 2cade68e6cc3..bb93c8b2dbfa 100644
--- a/sound/soc/amd/acp/acp-mach.h
+++ b/sound/soc/amd/acp/acp-mach.h
@@ -45,6 +45,7 @@ enum codec_endpoints {
 	MAX98360A,
 	RT5682S,
 	NAU8825,
+	ES83XX,
 };
 
 enum platform_end_point {
@@ -72,6 +73,7 @@ struct acp_card_drvdata {
 	struct clk *wclk;
 	struct clk *bclk;
 	struct acp_mach_ops ops;
+	struct snd_soc_acpi_mach *acpi_mach;
 	void *mach_priv;
 	bool soc_mclk;
 	bool tdm_mode;
diff --git a/sound/soc/amd/acp/acp-renoir.c b/sound/soc/amd/acp/acp-renoir.c
index 2a89a0d2e601..1f04e08e15d4 100644
--- a/sound/soc/amd/acp/acp-renoir.c
+++ b/sound/soc/amd/acp/acp-renoir.c
@@ -83,6 +83,10 @@ static struct snd_soc_acpi_mach snd_soc_acpi_amd_acp_machines[] = {
 		.id = "AMDI1019",
 		.drv_name = "renoir-acp",
 	},
+	{
+		.id = "ESSX8336",
+		.drv_name = "acp3x-es83xx",
+	},
 	{},
 };
 
diff --git a/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c b/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
new file mode 100644
index 000000000000..138a7c12669b
--- /dev/null
+++ b/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
@@ -0,0 +1,471 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Machine driver for AMD ACP Audio engine using ES8336 codec.
+//
+// Copyright 2023 Marian Postevca <posteuca@mutex.one>
+
+#include <sound/core.h>
+#include <sound/soc.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc-dapm.h>
+#include <sound/jack.h>
+#include <sound/soc-acpi.h>
+#include <linux/clk.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/io.h>
+#include <linux/acpi.h>
+#include <linux/dmi.h>
+#include "../acp-mach.h"
+
+#define get_mach_priv(card) ((struct acp3x_es83xx_private *)((acp_get_drvdata(card))->mach_priv))
+
+/* mclk-div-by-2 + terminating entry */
+#define MAX_NO_PROPS 2
+
+#define DUAL_CHANNEL	2
+
+#define ES83XX_ENABLE_DMIC	BIT(4)
+#define ES83XX_48_MHZ_MCLK	BIT(5)
+
+struct acp3x_es83xx_private {
+	unsigned long quirk;
+	struct snd_soc_component *codec;
+	struct device *codec_dev;
+	struct gpio_desc *gpio_speakers, *gpio_headphone;
+	struct acpi_gpio_params enable_spk_gpio, enable_hp_gpio;
+	struct acpi_gpio_mapping gpio_mapping[3];
+	struct snd_soc_dapm_route mic_map[2];
+};
+
+static const unsigned int rates[] = {
+	8000, 11025, 16000, 22050, 32000,
+	44100, 48000, 64000, 88200, 96000,
+};
+
+static const unsigned int rates_48mhz_mclk[] = {
+	48000, 96000,
+};
+
+static const unsigned int channels[] = {
+	DUAL_CHANNEL,
+};
+
+static const struct snd_pcm_hw_constraint_list hw_constraint_rates_normal = {
+	.count = ARRAY_SIZE(rates),
+	.list = rates,
+	.mask = 0,
+};
+
+static const struct snd_pcm_hw_constraint_list hw_constraint_rates_48mhz = {
+	.count = ARRAY_SIZE(rates_48mhz_mclk),
+	.list = rates_48mhz_mclk,
+	.mask = 0,
+};
+
+static const struct snd_pcm_hw_constraint_list constraints_channels = {
+	.count = ARRAY_SIZE(channels),
+	.list = channels,
+	.mask = 0,
+};
+
+#define ES83xx_12288_KHZ_MCLK_FREQ   (48000 * 256)
+#define ES83xx_48_MHZ_MCLK_FREQ      (48000 * 1000)
+
+static int acp3x_es83xx_speaker_power_event(struct snd_soc_dapm_widget *w,
+					    struct snd_kcontrol *kcontrol, int event);
+
+static void acp3x_es83xx_set_gpios_values(struct acp3x_es83xx_private *priv,
+					  bool speaker, bool headphone)
+{
+	gpiod_set_value_cansleep(priv->gpio_speakers, speaker);
+	gpiod_set_value_cansleep(priv->gpio_headphone, headphone);
+}
+
+static int acp3x_es83xx_create_swnode(struct device *codec_dev)
+{
+	struct property_entry props[MAX_NO_PROPS] = {};
+	struct fwnode_handle *fwnode;
+	int ret;
+
+	props[0] = PROPERTY_ENTRY_BOOL("everest,mclk-div-by-2");
+
+	fwnode = fwnode_create_software_node(props, NULL);
+	if (IS_ERR(fwnode))
+		return PTR_ERR(fwnode);
+
+	ret = device_add_software_node(codec_dev, to_software_node(fwnode));
+
+	fwnode_handle_put(fwnode);
+	return ret;
+}
+
+static int acp3x_es83xx_codec_startup(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime;
+	struct snd_soc_pcm_runtime *rtd;
+	struct snd_soc_dai *codec_dai;
+	struct acp3x_es83xx_private *priv;
+	unsigned int freq;
+	int ret;
+
+	runtime = substream->runtime;
+	rtd = asoc_substream_to_rtd(substream);
+	codec_dai = asoc_rtd_to_codec(rtd, 0);
+	priv = get_mach_priv(rtd->card);
+
+	if (priv->quirk & ES83XX_48_MHZ_MCLK) {
+		dev_dbg(priv->codec_dev, "using a 48Mhz MCLK\n");
+		snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
+					   &hw_constraint_rates_48mhz);
+		freq = ES83xx_48_MHZ_MCLK_FREQ;
+	} else {
+		dev_dbg(priv->codec_dev, "using a 12.288Mhz MCLK\n");
+		snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
+					   &hw_constraint_rates_normal);
+		freq = ES83xx_12288_KHZ_MCLK_FREQ;
+	}
+
+	ret = snd_soc_dai_set_sysclk(codec_dai, 0, freq, SND_SOC_CLOCK_OUT);
+	if (ret < 0) {
+		dev_err(rtd->dev, "can't set codec sysclk: %d\n", ret);
+		return ret;
+	}
+
+	ret =  snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF
+				   | SND_SOC_DAIFMT_CBP_CFP);
+	if (ret < 0) {
+		dev_err(rtd->dev, "failed to set DAI fmt: %d\n", ret);
+		return ret;
+	}
+
+	runtime->hw.channels_max = DUAL_CHANNEL;
+	snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
+				   &constraints_channels);
+	runtime->hw.formats = SNDRV_PCM_FMTBIT_S32_LE;
+
+	return 0;
+}
+
+static struct snd_soc_jack es83xx_jack;
+
+static struct snd_soc_jack_pin es83xx_jack_pins[] = {
+	{
+		.pin	= "Headphone",
+		.mask	= SND_JACK_HEADPHONE,
+	},
+	{
+		.pin	= "Headset Mic",
+		.mask	= SND_JACK_MICROPHONE,
+	},
+};
+
+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_MIC("Headset Mic", NULL),
+	SND_SOC_DAPM_MIC("Internal Mic", NULL),
+
+	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"},
+
+	/*
+	 * There is no separate speaker output instead the speakers are muxed to
+	 * the HP outputs. The mux is controlled Speaker and/or headphone switch.
+	 */
+	{"Speaker", NULL, "HPOL"},
+	{"Speaker", NULL, "HPOR"},
+	{"Speaker", NULL, "Speaker Power"},
+};
+
+
+static const struct snd_kcontrol_new acp3x_es83xx_controls[] = {
+	SOC_DAPM_PIN_SWITCH("Speaker"),
+	SOC_DAPM_PIN_SWITCH("Headphone"),
+	SOC_DAPM_PIN_SWITCH("Headset Mic"),
+	SOC_DAPM_PIN_SWITCH("Internal Mic"),
+};
+
+static int acp3x_es83xx_configure_widgets(struct snd_soc_card *card)
+{
+	card->dapm_widgets = acp3x_es83xx_widgets;
+	card->num_dapm_widgets = ARRAY_SIZE(acp3x_es83xx_widgets);
+	card->controls = acp3x_es83xx_controls;
+	card->num_controls = ARRAY_SIZE(acp3x_es83xx_controls);
+	card->dapm_routes = acp3x_es83xx_audio_map;
+	card->num_dapm_routes = ARRAY_SIZE(acp3x_es83xx_audio_map);
+
+	return 0;
+}
+
+static int acp3x_es83xx_speaker_power_event(struct snd_soc_dapm_widget *w,
+					    struct snd_kcontrol *kcontrol, int event)
+{
+	struct acp3x_es83xx_private *priv = get_mach_priv(w->dapm->card);
+
+	dev_dbg(priv->codec_dev, "speaker power event: %d\n", event);
+	if (SND_SOC_DAPM_EVENT_ON(event))
+		acp3x_es83xx_set_gpios_values(priv, 1, 0);
+	else
+		acp3x_es83xx_set_gpios_values(priv, 0, 1);
+
+	return 0;
+}
+
+static int acp3x_es83xx_suspend_pre(struct snd_soc_card *card)
+{
+	struct acp3x_es83xx_private *priv = get_mach_priv(card);
+
+	dev_dbg(priv->codec_dev, "card suspend\n");
+	snd_soc_component_set_jack(priv->codec, NULL, NULL);
+	return 0;
+}
+
+static int acp3x_es83xx_resume_post(struct snd_soc_card *card)
+{
+	struct acp3x_es83xx_private *priv = get_mach_priv(card);
+
+	dev_dbg(priv->codec_dev, "card resume\n");
+	snd_soc_component_set_jack(priv->codec, &es83xx_jack, NULL);
+	return 0;
+}
+
+static int acp3x_es83xx_configure_gpios(struct acp3x_es83xx_private *priv)
+{
+	int ret = 0;
+
+	priv->enable_spk_gpio.crs_entry_index = 0;
+	priv->enable_hp_gpio.crs_entry_index = 1;
+
+	priv->enable_spk_gpio.active_low = false;
+	priv->enable_hp_gpio.active_low = false;
+
+	priv->gpio_mapping[0].name = "speakers-enable-gpios";
+	priv->gpio_mapping[0].data = &priv->enable_spk_gpio;
+	priv->gpio_mapping[0].size = 1;
+	priv->gpio_mapping[0].quirks = ACPI_GPIO_QUIRK_ONLY_GPIOIO;
+
+	priv->gpio_mapping[1].name = "headphone-enable-gpios";
+	priv->gpio_mapping[1].data = &priv->enable_hp_gpio;
+	priv->gpio_mapping[1].size = 1;
+	priv->gpio_mapping[1].quirks = ACPI_GPIO_QUIRK_ONLY_GPIOIO;
+
+	dev_info(priv->codec_dev, "speaker gpio %d active %s, headphone gpio %d active %s\n",
+		 priv->enable_spk_gpio.crs_entry_index,
+		 priv->enable_spk_gpio.active_low ? "low" : "high",
+		 priv->enable_hp_gpio.crs_entry_index,
+		 priv->enable_hp_gpio.active_low ? "low" : "high");
+	return ret;
+}
+
+static int acp3x_es83xx_configure_mics(struct acp3x_es83xx_private *priv)
+{
+	int num_routes = 0;
+	int i;
+
+	if (!(priv->quirk & ES83XX_ENABLE_DMIC)) {
+		priv->mic_map[num_routes].sink = "MIC1";
+		priv->mic_map[num_routes].source = "Internal Mic";
+		num_routes++;
+	}
+
+	priv->mic_map[num_routes].sink = "MIC2";
+	priv->mic_map[num_routes].source = "Headset Mic";
+	num_routes++;
+
+	for (i = 0; i < num_routes; i++)
+		dev_info(priv->codec_dev, "%s is %s\n",
+			 priv->mic_map[i].source, priv->mic_map[i].sink);
+
+	return num_routes;
+}
+
+static int acp3x_es83xx_init(struct snd_soc_pcm_runtime *runtime)
+{
+	struct snd_soc_component *codec = asoc_rtd_to_codec(runtime, 0)->component;
+	struct snd_soc_card *card = runtime->card;
+	struct acp3x_es83xx_private *priv = get_mach_priv(card);
+	int ret = 0;
+	int num_routes;
+
+	ret = snd_soc_card_jack_new_pins(card, "Headset",
+					 SND_JACK_HEADSET | SND_JACK_BTN_0,
+					 &es83xx_jack, es83xx_jack_pins,
+					 ARRAY_SIZE(es83xx_jack_pins));
+	if (ret) {
+		dev_err(card->dev, "jack creation failed %d\n", ret);
+		return ret;
+	}
+
+	snd_jack_set_key(es83xx_jack.jack, SND_JACK_BTN_0, KEY_PLAYPAUSE);
+
+	snd_soc_component_set_jack(codec, &es83xx_jack, NULL);
+
+	priv->codec = codec;
+	acp3x_es83xx_configure_gpios(priv);
+
+	ret = devm_acpi_dev_add_driver_gpios(priv->codec_dev, priv->gpio_mapping);
+	if (ret)
+		dev_warn(priv->codec_dev, "failed to add speaker gpio\n");
+
+	priv->gpio_speakers = gpiod_get_optional(priv->codec_dev, "speakers-enable",
+				priv->enable_spk_gpio.active_low ? GPIOD_OUT_LOW : GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->gpio_speakers)) {
+		dev_err(priv->codec_dev, "could not get speakers-enable GPIO\n");
+		return PTR_ERR(priv->gpio_speakers);
+	}
+
+	priv->gpio_headphone = gpiod_get_optional(priv->codec_dev, "headphone-enable",
+				priv->enable_hp_gpio.active_low ? GPIOD_OUT_LOW : GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->gpio_headphone)) {
+		dev_err(priv->codec_dev, "could not get headphone-enable GPIO\n");
+		return PTR_ERR(priv->gpio_headphone);
+	}
+
+	if (priv->quirk & ES83XX_48_MHZ_MCLK) {
+		ret = acp3x_es83xx_create_swnode(priv->codec_dev);
+		if (ret != 0) {
+			dev_err(priv->codec_dev,
+				"could not create software node inside codec: %d\n", ret);
+			return ret;
+		}
+	}
+
+	num_routes = acp3x_es83xx_configure_mics(priv);
+	if (num_routes > 0) {
+		ret = snd_soc_dapm_add_routes(&card->dapm, priv->mic_map, num_routes);
+		if (ret != 0)
+			device_remove_software_node(priv->codec_dev);
+	}
+
+	return ret;
+}
+
+static const struct snd_soc_ops acp3x_es83xx_ops = {
+	.startup = acp3x_es83xx_codec_startup,
+};
+
+
+SND_SOC_DAILINK_DEF(codec,
+		    DAILINK_COMP_ARRAY(COMP_CODEC("i2c-ESSX8336:00", "ES8316 HiFi")));
+
+static const struct dmi_system_id acp3x_es83xx_dmi_table[] = {
+	{
+		.matches = {
+			DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "HUAWEI"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "KLVL-WXXW"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "M1010"),
+		},
+		.driver_data = (void *)(ES83XX_ENABLE_DMIC),
+	},
+	{
+		.matches = {
+			DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "HUAWEI"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "KLVL-WXX9"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "M1010"),
+		},
+		.driver_data = (void *)(ES83XX_ENABLE_DMIC),
+	},
+	{
+		.matches = {
+			DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "HUAWEI"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "BOM-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"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "HVY-WXX9"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "M1020"),
+		},
+		.driver_data = (void *)(ES83XX_ENABLE_DMIC),
+	},
+	{
+		.matches = {
+			DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "HUAWEI"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "HVY-WXX9"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "M1040"),
+		},
+		.driver_data = (void *)(ES83XX_ENABLE_DMIC),
+	},
+	{}
+};
+
+static int acp3x_es83xx_configure_link(struct snd_soc_card *card, struct snd_soc_dai_link *link)
+{
+	link->codecs = codec;
+	link->num_codecs = ARRAY_SIZE(codec);
+	link->init = acp3x_es83xx_init;
+	link->ops = &acp3x_es83xx_ops;
+
+	return 0;
+}
+
+static int acp3x_es83xx_probe(struct snd_soc_card *card)
+{
+	int ret = 0;
+	struct device *dev = card->dev;
+	const struct dmi_system_id *dmi_id;
+
+	dmi_id = dmi_first_match(acp3x_es83xx_dmi_table);
+	if (dmi_id && dmi_id->driver_data) {
+		struct acp3x_es83xx_private *priv;
+		struct acp_card_drvdata *acp_drvdata;
+		struct acpi_device *adev;
+		struct device *codec_dev;
+
+		acp_drvdata = (struct acp_card_drvdata *)card->drvdata;
+
+		dev_info(dev, "matched DMI table with this system, trying to register sound card\n");
+
+		adev = acpi_dev_get_first_match_dev(acp_drvdata->acpi_mach->id, NULL, -1);
+		if (!adev) {
+			dev_err(dev, "Error cannot find '%s' dev\n", acp_drvdata->acpi_mach->id);
+			return -ENXIO;
+		}
+
+		codec_dev = acpi_get_first_physical_node(adev);
+		acpi_dev_put(adev);
+		if (!codec_dev) {
+			dev_warn(dev, "Error cannot find codec device, will defer probe\n");
+			return -EPROBE_DEFER;
+		}
+
+		priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+		if (!priv) {
+			put_device(codec_dev);
+			return -ENOMEM;
+		}
+
+		priv->codec_dev = codec_dev;
+		priv->quirk = (unsigned long)dmi_id->driver_data;
+		acp_drvdata->mach_priv = priv;
+		dev_info(dev, "successfully probed the sound card\n");
+	} else {
+		ret = -ENODEV;
+		dev_warn(dev, "this system has a ES83xx codec defined in ACPI, but the driver doesn't have this system registered in DMI table\n");
+	}
+	return ret;
+}
+
+
+void acp3x_es83xx_init_ops(struct acp_mach_ops *ops)
+{
+	ops->probe = acp3x_es83xx_probe;
+	ops->configure_widgets = acp3x_es83xx_configure_widgets;
+	ops->configure_link = acp3x_es83xx_configure_link;
+	ops->suspend_pre = acp3x_es83xx_suspend_pre;
+	ops->resume_post = acp3x_es83xx_resume_post;
+}
diff --git a/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.h b/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.h
new file mode 100644
index 000000000000..03551ffdd9da
--- /dev/null
+++ b/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright 2023 Marian Postevca <posteuca@mutex.one>
+ */
+
+#ifndef __ACP3X_ES83XX_H
+#define __ACP3X_ES83XX_H
+
+void acp3x_es83xx_init_ops(struct acp_mach_ops *ops);
+
+#endif
+
-- 
2.39.1

[PATCH 4/4] ASoC: amd: acp: Improve support for speaker power events

[Marian Postevca]

In order to reduce the audible pops when speaker or headphones
are activated or disabled we need to delay the switching of the
GPIOs.
We need to also disable/enable the speaker/headphones GPIOs when
the audio stream is stopped/started. To avoid race conditions
between the speaker power event callback and the trigger callback
we use a ring buffer to save the events that we need to process
in the delayed work callback.

Signed-off-by: Marian Postevca <posteuca@mutex.one>
---
 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c | 154 +++++++++++++++++-
 1 file changed, 149 insertions(+), 5 deletions(-)

diff --git a/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c b/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
index 138a7c12669b..033c94e91928 100644
--- a/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
+++ b/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
@@ -20,8 +20,18 @@
 #include <linux/io.h>
 #include <linux/acpi.h>
 #include <linux/dmi.h>
+#include <linux/circ_buf.h>
+
 #include "../acp-mach.h"
 
+#define RB_SIZE         32
+
+#define circ_count(circ)	  \
+	(CIRC_CNT((circ)->head, (circ)->tail, RB_SIZE))
+
+#define circ_space(circ)	  \
+	(CIRC_SPACE((circ)->head, (circ)->tail, RB_SIZE))
+
 #define get_mach_priv(card) ((struct acp3x_es83xx_private *)((acp_get_drvdata(card))->mach_priv))
 
 /* mclk-div-by-2 + terminating entry */
@@ -32,14 +42,35 @@
 #define ES83XX_ENABLE_DMIC	BIT(4)
 #define ES83XX_48_MHZ_MCLK	BIT(5)
 
+enum {
+	SPEAKER_ON = 0,
+	SPEAKER_OFF,
+	SPEAKER_SUSPEND,
+	SPEAKER_RESUME,
+	SPEAKER_MAX
+};
+
+const char *msg[SPEAKER_MAX] = {
+	"SPEAKER_ON",
+	"SPEAKER_OFF",
+	"SPEAKER_SUSPEND",
+	"SPEAKER_RESUME"
+};
+
 struct acp3x_es83xx_private {
 	unsigned long quirk;
+	bool speaker_on;
+	bool stream_suspended;
 	struct snd_soc_component *codec;
 	struct device *codec_dev;
 	struct gpio_desc *gpio_speakers, *gpio_headphone;
 	struct acpi_gpio_params enable_spk_gpio, enable_hp_gpio;
 	struct acpi_gpio_mapping gpio_mapping[3];
 	struct snd_soc_dapm_route mic_map[2];
+	struct delayed_work jack_work;
+	struct mutex rb_lock;
+	struct circ_buf gpio_rb;
+	u8 gpio_events_buf[RB_SIZE];
 };
 
 static const unsigned int rates[] = {
@@ -86,6 +117,107 @@ static void acp3x_es83xx_set_gpios_values(struct acp3x_es83xx_private *priv,
 	gpiod_set_value_cansleep(priv->gpio_headphone, headphone);
 }
 
+static void acp3x_es83xx_rb_insert_evt(struct circ_buf *rb, u8 val)
+{
+	u8 *buf = rb->buf;
+
+	if (circ_space(rb) == 0) {
+		/* make some space by dropping the oldest entry, we are more
+		 * interested in the last event
+		 */
+		rb->tail = (rb->tail + 1) & (RB_SIZE - 1);
+	}
+	buf[rb->head] = val;
+	rb->head = (rb->head + 1) & (RB_SIZE - 1);
+}
+
+static int acp3x_es83xx_rb_remove_evt(struct circ_buf *rb)
+{
+	u8 *buf = rb->buf;
+	int rc = -1;
+
+	if (circ_count(rb)) {
+		rc = buf[rb->tail];
+		rb->tail = (rb->tail + 1) & (RB_SIZE - 1);
+	}
+	return rc;
+}
+
+static void acp3x_es83xx_jack_events(struct work_struct *work)
+{
+	struct acp3x_es83xx_private *priv =
+		container_of(work, struct acp3x_es83xx_private, jack_work.work);
+	int evt;
+
+	mutex_lock(&priv->rb_lock);
+	do {
+		evt = acp3x_es83xx_rb_remove_evt(&priv->gpio_rb);
+		dev_dbg(priv->codec_dev, "jack event, %s\n", msg[evt]);
+		switch (evt) {
+		case SPEAKER_SUSPEND:
+			acp3x_es83xx_set_gpios_values(priv, 0, 0);
+			break;
+		case SPEAKER_RESUME:
+			acp3x_es83xx_set_gpios_values(priv, priv->speaker_on, !priv->speaker_on);
+			break;
+		case SPEAKER_ON:
+			priv->speaker_on = true;
+			acp3x_es83xx_set_gpios_values(priv, 1, 0);
+			break;
+		case SPEAKER_OFF:
+			priv->speaker_on = false;
+			acp3x_es83xx_set_gpios_values(priv, 0, 1);
+			break;
+		}
+	} while (evt != -1);
+	mutex_unlock(&priv->rb_lock);
+}
+
+static int acp3x_es83xx_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+	struct snd_soc_card *card = rtd->card;
+	struct acp3x_es83xx_private *priv = get_mach_priv(card);
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+	case SNDRV_PCM_TRIGGER_RESUME:
+		if (substream->stream == 0) {
+			dev_dbg(priv->codec_dev, "trigger start/release/resume, activating GPIOs\n");
+			mutex_lock(&priv->rb_lock);
+			if (priv->stream_suspended) {
+				priv->stream_suspended = false;
+				acp3x_es83xx_rb_insert_evt(&priv->gpio_rb, SPEAKER_RESUME);
+				mutex_unlock(&priv->rb_lock);
+				queue_delayed_work(system_wq, &priv->jack_work,
+						   msecs_to_jiffies(1));
+			} else {
+				mutex_unlock(&priv->rb_lock);
+			}
+		}
+
+		break;
+
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_STOP:
+		if (substream->stream == 0) {
+			dev_dbg(priv->codec_dev, "trigger pause/suspend/stop deactivating GPIOs\n");
+			mutex_lock(&priv->rb_lock);
+			priv->stream_suspended = true;
+			acp3x_es83xx_rb_insert_evt(&priv->gpio_rb, SPEAKER_SUSPEND);
+			mutex_unlock(&priv->rb_lock);
+			queue_delayed_work(system_wq, &priv->jack_work, msecs_to_jiffies(1));
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int acp3x_es83xx_create_swnode(struct device *codec_dev)
 {
 	struct property_entry props[MAX_NO_PROPS] = {};
@@ -212,12 +344,17 @@ static int acp3x_es83xx_speaker_power_event(struct snd_soc_dapm_widget *w,
 					    struct snd_kcontrol *kcontrol, int event)
 {
 	struct acp3x_es83xx_private *priv = get_mach_priv(w->dapm->card);
+	u8 val;
+
+	val = SND_SOC_DAPM_EVENT_ON(event) ? SPEAKER_ON : SPEAKER_OFF;
 
-	dev_dbg(priv->codec_dev, "speaker power event: %d\n", event);
-	if (SND_SOC_DAPM_EVENT_ON(event))
-		acp3x_es83xx_set_gpios_values(priv, 1, 0);
-	else
-		acp3x_es83xx_set_gpios_values(priv, 0, 1);
+	dev_dbg(priv->codec_dev, "speaker power event = %s\n", msg[val]);
+
+	mutex_lock(&priv->rb_lock);
+	acp3x_es83xx_rb_insert_evt(&priv->gpio_rb, val);
+	mutex_unlock(&priv->rb_lock);
+
+	queue_delayed_work(system_wq, &priv->jack_work, msecs_to_jiffies(70));
 
 	return 0;
 }
@@ -353,6 +490,7 @@ static int acp3x_es83xx_init(struct snd_soc_pcm_runtime *runtime)
 
 static const struct snd_soc_ops acp3x_es83xx_ops = {
 	.startup = acp3x_es83xx_codec_startup,
+	.trigger = acp3x_es83xx_trigger,
 };
 
 
@@ -452,6 +590,12 @@ static int acp3x_es83xx_probe(struct snd_soc_card *card)
 		priv->codec_dev = codec_dev;
 		priv->quirk = (unsigned long)dmi_id->driver_data;
 		acp_drvdata->mach_priv = priv;
+
+		priv->gpio_rb.buf = priv->gpio_events_buf;
+		priv->gpio_rb.head = priv->gpio_rb.tail = 0;
+		mutex_init(&priv->rb_lock);
+
+		INIT_DELAYED_WORK(&priv->jack_work, acp3x_es83xx_jack_events);
 		dev_info(dev, "successfully probed the sound card\n");
 	} else {
 		ret = -ENODEV;
-- 
2.39.1

Re: [PATCH 1/4] ASoC: es8316: Enable support for S32 LE format and MCLK div by 2

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 796 bytes --]

On Mon, Mar 20, 2023 at 10:35:16PM +0200, Marian Postevca wrote:

> To properly support a line of Huawei laptops with AMD CPU and a
> ES8336 codec connected to the ACP3X module we need to enable
> the S32 LE format and the codec option to divide the MCLK by 2.

The 32 bit support and MCLK division are two separate changes so should
be two separate patches.

> -	lrck_divider = es8316->sysclk / params_rate(params);
> +
> +	mclk_div_option = device_property_read_bool(component->dev,
> +						    "everest,mclk-div-by-2");
> +	if (mclk_div_option) {

This introduces a DT property but there's no documentation for it, but I
don't see why we'd want this in the bindings - the driver should be able
to tell from the input clock rate and required output/internal clocks if
it needs to divide MCLK.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] ASoC: es8316: Enable support for S32 LE format and MCLK div by 2

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 796 bytes --]

On Mon, Mar 20, 2023 at 10:35:16PM +0200, Marian Postevca wrote:

> To properly support a line of Huawei laptops with AMD CPU and a
> ES8336 codec connected to the ACP3X module we need to enable
> the S32 LE format and the codec option to divide the MCLK by 2.

The 32 bit support and MCLK division are two separate changes so should
be two separate patches.

> -	lrck_divider = es8316->sysclk / params_rate(params);
> +
> +	mclk_div_option = device_property_read_bool(component->dev,
> +						    "everest,mclk-div-by-2");
> +	if (mclk_div_option) {

This introduces a DT property but there's no documentation for it, but I
don't see why we'd want this in the bindings - the driver should be able
to tell from the input clock rate and required output/internal clocks if
it needs to divide MCLK.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1818 bytes --]

On Mon, Mar 20, 2023 at 10:35:18PM +0200, Marian Postevca wrote:

> +static int acp_asoc_suspend_pre(struct snd_soc_card *card)
> +{
> +	int ret;
> +
> +	ret = acp_ops_suspend_pre(card);
> +	return ret == 1 ? 0 : ret;

Please write normal conditional statements to improve legibility (or
just have the function that's being called return a directly usable
value?).

> +	if (priv->quirk & ES83XX_48_MHZ_MCLK) {
> +		dev_dbg(priv->codec_dev, "using a 48Mhz MCLK\n");
> +		snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
> +					   &hw_constraint_rates_48mhz);
> +		freq = ES83xx_48_MHZ_MCLK_FREQ;
> +	} else {
> +		dev_dbg(priv->codec_dev, "using a 12.288Mhz MCLK\n");
> +		snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
> +					   &hw_constraint_rates_normal);
> +		freq = ES83xx_12288_KHZ_MCLK_FREQ;
> +	}

The CODEC driver should be able to set these constraints for itself.

> +	ret =  snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF
> +				   | SND_SOC_DAIFMT_CBP_CFP);

Set this in the dai_link.

> +static int acp3x_es83xx_speaker_power_event(struct snd_soc_dapm_widget *w,
> +					    struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct acp3x_es83xx_private *priv = get_mach_priv(w->dapm->card);
> +
> +	dev_dbg(priv->codec_dev, "speaker power event: %d\n", event);
> +	if (SND_SOC_DAPM_EVENT_ON(event))
> +		acp3x_es83xx_set_gpios_values(priv, 1, 0);
> +	else
> +		acp3x_es83xx_set_gpios_values(priv, 0, 1);

Why are these two GPIOs tied together like this?

> +static int acp3x_es83xx_suspend_pre(struct snd_soc_card *card)
> +{
> +	struct acp3x_es83xx_private *priv = get_mach_priv(card);
> +
> +	dev_dbg(priv->codec_dev, "card suspend\n");
> +	snd_soc_component_set_jack(priv->codec, NULL, NULL);
> +	return 0;
> +}

That's weird, why do that?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1818 bytes --]

On Mon, Mar 20, 2023 at 10:35:18PM +0200, Marian Postevca wrote:

> +static int acp_asoc_suspend_pre(struct snd_soc_card *card)
> +{
> +	int ret;
> +
> +	ret = acp_ops_suspend_pre(card);
> +	return ret == 1 ? 0 : ret;

Please write normal conditional statements to improve legibility (or
just have the function that's being called return a directly usable
value?).

> +	if (priv->quirk & ES83XX_48_MHZ_MCLK) {
> +		dev_dbg(priv->codec_dev, "using a 48Mhz MCLK\n");
> +		snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
> +					   &hw_constraint_rates_48mhz);
> +		freq = ES83xx_48_MHZ_MCLK_FREQ;
> +	} else {
> +		dev_dbg(priv->codec_dev, "using a 12.288Mhz MCLK\n");
> +		snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
> +					   &hw_constraint_rates_normal);
> +		freq = ES83xx_12288_KHZ_MCLK_FREQ;
> +	}

The CODEC driver should be able to set these constraints for itself.

> +	ret =  snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF
> +				   | SND_SOC_DAIFMT_CBP_CFP);

Set this in the dai_link.

> +static int acp3x_es83xx_speaker_power_event(struct snd_soc_dapm_widget *w,
> +					    struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct acp3x_es83xx_private *priv = get_mach_priv(w->dapm->card);
> +
> +	dev_dbg(priv->codec_dev, "speaker power event: %d\n", event);
> +	if (SND_SOC_DAPM_EVENT_ON(event))
> +		acp3x_es83xx_set_gpios_values(priv, 1, 0);
> +	else
> +		acp3x_es83xx_set_gpios_values(priv, 0, 1);

Why are these two GPIOs tied together like this?

> +static int acp3x_es83xx_suspend_pre(struct snd_soc_card *card)
> +{
> +	struct acp3x_es83xx_private *priv = get_mach_priv(card);
> +
> +	dev_dbg(priv->codec_dev, "card suspend\n");
> +	snd_soc_component_set_jack(priv->codec, NULL, NULL);
> +	return 0;
> +}

That's weird, why do that?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/4] ASoC: amd: acp: Improve support for speaker power events

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1556 bytes --]

On Mon, Mar 20, 2023 at 10:35:19PM +0200, Marian Postevca wrote:
> In order to reduce the audible pops when speaker or headphones
> are activated or disabled we need to delay the switching of the
> GPIOs.

The usual mechanism for doing this is with the standard kernel delay
functions.  Why not use them in the DAPM event?

> We need to also disable/enable the speaker/headphones GPIOs when
> the audio stream is stopped/started. To avoid race conditions
> between the speaker power event callback and the trigger callback
> we use a ring buffer to save the events that we need to process
> in the delayed work callback.

Why is this required?  DAPM is integrated with stream start and stop,
and there's a mute callback to mask any noise played back from the SoC
while it stops and starts without requiring all this complexity.  If
there is any audible noise then why would it only affect the speaker?

> +static int acp3x_es83xx_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> +	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> +	struct snd_soc_card *card = rtd->card;
> +	struct acp3x_es83xx_private *priv = get_mach_priv(card);
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +		if (substream->stream == 0) {
> +			dev_dbg(priv->codec_dev, "trigger start/release/resume, activating GPIOs\n");
> +			mutex_lock(&priv->rb_lock);

Triggers run in atomic context, you can't use mutexes in atomic context.
lockdep should tell you this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/4] ASoC: amd: acp: Improve support for speaker power events

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1556 bytes --]

On Mon, Mar 20, 2023 at 10:35:19PM +0200, Marian Postevca wrote:
> In order to reduce the audible pops when speaker or headphones
> are activated or disabled we need to delay the switching of the
> GPIOs.

The usual mechanism for doing this is with the standard kernel delay
functions.  Why not use them in the DAPM event?

> We need to also disable/enable the speaker/headphones GPIOs when
> the audio stream is stopped/started. To avoid race conditions
> between the speaker power event callback and the trigger callback
> we use a ring buffer to save the events that we need to process
> in the delayed work callback.

Why is this required?  DAPM is integrated with stream start and stop,
and there's a mute callback to mask any noise played back from the SoC
while it stops and starts without requiring all this complexity.  If
there is any audible noise then why would it only affect the speaker?

> +static int acp3x_es83xx_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> +	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> +	struct snd_soc_card *card = rtd->card;
> +	struct acp3x_es83xx_private *priv = get_mach_priv(card);
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +		if (substream->stream == 0) {
> +			dev_dbg(priv->codec_dev, "trigger start/release/resume, activating GPIOs\n");
> +			mutex_lock(&priv->rb_lock);

Triggers run in atomic context, you can't use mutexes in atomic context.
lockdep should tell you this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[kernel test robot]

Hi Marian,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on broonie-sound/for-next]
[also build test WARNING on linus/master v6.3-rc3 next-20230320]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Marian-Postevca/ASoC-es8316-Enable-support-for-S32-LE-format-and-MCLK-div-by-2/20230321-043847
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
patch link:    https://lore.kernel.org/r/20230320203519.20137-4-posteuca%40mutex.one
patch subject: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230321/202303210820.JYm7LOnS-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/0562079546d40f50d025ea1c4dacd31120b8b0bc
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Marian-Postevca/ASoC-es8316-Enable-support-for-S32-LE-format-and-MCLK-div-by-2/20230321-043847
        git checkout 0562079546d40f50d025ea1c4dacd31120b8b0bc
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash sound/soc/amd/acp/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303210820.JYm7LOnS-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c:464:6: warning: no previous prototype for 'acp3x_es83xx_init_ops' [-Wmissing-prototypes]
     464 | void acp3x_es83xx_init_ops(struct acp_mach_ops *ops)
         |      ^~~~~~~~~~~~~~~~~~~~~


vim +/acp3x_es83xx_init_ops +464 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c

   462	
   463	
 > 464	void acp3x_es83xx_init_ops(struct acp_mach_ops *ops)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 1/4] ASoC: es8316: Enable support for S32 LE format and MCLK div by 2

[Marian Postevca]

Thanks for taking the time to review this series of patches.

Mark Brown <broonie@kernel.org> writes:

> On Mon, Mar 20, 2023 at 10:35:16PM +0200, Marian Postevca wrote:
>
>> To properly support a line of Huawei laptops with AMD CPU and a
>> ES8336 codec connected to the ACP3X module we need to enable
>> the S32 LE format and the codec option to divide the MCLK by 2.
>
> The 32 bit support and MCLK division are two separate changes so should
> be two separate patches.
>

Ok, no problem, I just thought that a separate commit for one line was overkill.

>> -	lrck_divider = es8316->sysclk / params_rate(params);
>> +
>> +	mclk_div_option = device_property_read_bool(component->dev,
>> +						    "everest,mclk-div-by-2");
>> +	if (mclk_div_option) {
>
> This introduces a DT property but there's no documentation for it, but I
> don't see why we'd want this in the bindings - the driver should be able
> to tell from the input clock rate and required output/internal clocks if
> it needs to divide MCLK.

The problem here is that I have no knowledge what is the maximum MCLK
that the codec accepts. According to the datasheet the maximum supported
frequency of MCLK is 51.2 Mhz. But this doesn't seem to be the case in
practice since a MCLK of 48Mhz causes noises in the sound output.
The idea to divide the MCLK by 2 was proposed by a Everest Semiconductor
engineer.
So I don't know how to make this generic enough to be activated from the
codec driver.

I cced the Everest Semiconductor engineers, maybe they have a proposal
on how to activate this.

Re: [PATCH 1/4] ASoC: es8316: Enable support for S32 LE format and MCLK div by 2

[Marian Postevca]

Thanks for taking the time to review this series of patches.

Mark Brown <broonie@kernel.org> writes:

> On Mon, Mar 20, 2023 at 10:35:16PM +0200, Marian Postevca wrote:
>
>> To properly support a line of Huawei laptops with AMD CPU and a
>> ES8336 codec connected to the ACP3X module we need to enable
>> the S32 LE format and the codec option to divide the MCLK by 2.
>
> The 32 bit support and MCLK division are two separate changes so should
> be two separate patches.
>

Ok, no problem, I just thought that a separate commit for one line was overkill.

>> -	lrck_divider = es8316->sysclk / params_rate(params);
>> +
>> +	mclk_div_option = device_property_read_bool(component->dev,
>> +						    "everest,mclk-div-by-2");
>> +	if (mclk_div_option) {
>
> This introduces a DT property but there's no documentation for it, but I
> don't see why we'd want this in the bindings - the driver should be able
> to tell from the input clock rate and required output/internal clocks if
> it needs to divide MCLK.

The problem here is that I have no knowledge what is the maximum MCLK
that the codec accepts. According to the datasheet the maximum supported
frequency of MCLK is 51.2 Mhz. But this doesn't seem to be the case in
practice since a MCLK of 48Mhz causes noises in the sound output.
The idea to divide the MCLK by 2 was proposed by a Everest Semiconductor
engineer.
So I don't know how to make this generic enough to be activated from the
codec driver.

I cced the Everest Semiconductor engineers, maybe they have a proposal
on how to activate this.

Re: [PATCH 1/4] ASoC: es8316: Enable support for S32 LE format and MCLK div by 2

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]

On Tue, Mar 21, 2023 at 07:09:43PM +0200, Marian Postevca wrote:
> Mark Brown <broonie@kernel.org> writes:
> > On Mon, Mar 20, 2023 at 10:35:16PM +0200, Marian Postevca wrote:

> > This introduces a DT property but there's no documentation for it, but I
> > don't see why we'd want this in the bindings - the driver should be able
> > to tell from the input clock rate and required output/internal clocks if
> > it needs to divide MCLK.

> The problem here is that I have no knowledge what is the maximum MCLK
> that the codec accepts. According to the datasheet the maximum supported
> frequency of MCLK is 51.2 Mhz. But this doesn't seem to be the case in
> practice since a MCLK of 48Mhz causes noises in the sound output.
> The idea to divide the MCLK by 2 was proposed by a Everest Semiconductor
> engineer.
> So I don't know how to make this generic enough to be activated from the
> codec driver.

The usual constraint would be that MCLK can be at most some multiple of
LRCLK or something similar (are all the other dividers in the chip set
sensibly for the full scale MCLK?).  In any case you're clearly aware of
a specific case where it needs to be divided down which can be
identified even if you're concerned about dividing down for other cases.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] ASoC: es8316: Enable support for S32 LE format and MCLK div by 2

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]

On Tue, Mar 21, 2023 at 07:09:43PM +0200, Marian Postevca wrote:
> Mark Brown <broonie@kernel.org> writes:
> > On Mon, Mar 20, 2023 at 10:35:16PM +0200, Marian Postevca wrote:

> > This introduces a DT property but there's no documentation for it, but I
> > don't see why we'd want this in the bindings - the driver should be able
> > to tell from the input clock rate and required output/internal clocks if
> > it needs to divide MCLK.

> The problem here is that I have no knowledge what is the maximum MCLK
> that the codec accepts. According to the datasheet the maximum supported
> frequency of MCLK is 51.2 Mhz. But this doesn't seem to be the case in
> practice since a MCLK of 48Mhz causes noises in the sound output.
> The idea to divide the MCLK by 2 was proposed by a Everest Semiconductor
> engineer.
> So I don't know how to make this generic enough to be activated from the
> codec driver.

The usual constraint would be that MCLK can be at most some multiple of
LRCLK or something similar (are all the other dividers in the chip set
sensibly for the full scale MCLK?).  In any case you're clearly aware of
a specific case where it needs to be divided down which can be
identified even if you're concerned about dividing down for other cases.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

>> +static int acp3x_es83xx_speaker_power_event(struct snd_soc_dapm_widget *w,
>> +					    struct snd_kcontrol *kcontrol, int event)
>> +{
>> +	struct acp3x_es83xx_private *priv = get_mach_priv(w->dapm->card);
>> +
>> +	dev_dbg(priv->codec_dev, "speaker power event: %d\n", event);
>> +	if (SND_SOC_DAPM_EVENT_ON(event))
>> +		acp3x_es83xx_set_gpios_values(priv, 1, 0);
>> +	else
>> +		acp3x_es83xx_set_gpios_values(priv, 0, 1);
>
> Why are these two GPIOs tied together like this?
>

These GPIOs represent the speaker and the headphone switches. When
activating the speaker GPIO you have to deactivate the headphone GPIO
and vice versa. The logic is taken from the discussion on the sofproject
pull request:
https://github.com/thesofproject/linux/pull/4112/commits/810d03e0aecdf0caf580a5179ee6873fb33485ab
and
https://github.com/thesofproject/linux/pull/4066

>> +static int acp3x_es83xx_suspend_pre(struct snd_soc_card *card)
>> +{
>> +	struct acp3x_es83xx_private *priv = get_mach_priv(card);
>> +
>> +	dev_dbg(priv->codec_dev, "card suspend\n");
>> +	snd_soc_component_set_jack(priv->codec, NULL, NULL);
>> +	return 0;
>> +}
>
> That's weird, why do that?

This is needed because if suspending the laptop with the headphones
inserted, when resuming, the sound is not working anymore. Sound stops
working on speakers and headphones. Reinsertion and removals of the
headphone doesn't solve the problem.

This seems to be caused by the fact
that the GPIO IRQ stops working in es8316_irq() after resume.
Now the call to snd_soc_component_set_jack() in suspend disables the
GPIO IRQ and in resume the GPIO IRQ is reactivated.
By the way this sequence is also used in bytcht_es8316.c in suspend and
resume:

static int byt_cht_es8316_suspend(struct snd_soc_card *card)
{
	struct snd_soc_component *component;

	for_each_card_components(card, component) {
		if (!strcmp(component->name, codec_name)) {
			dev_dbg(component->dev, "disabling jack detect before suspend\n");
			snd_soc_component_set_jack(component, NULL, NULL);
			break;
		}
	}

	return 0;
}

static int byt_cht_es8316_resume(struct snd_soc_card *card)
{
	struct byt_cht_es8316_private *priv = snd_soc_card_get_drvdata(card);
	struct snd_soc_component *component;

	for_each_card_components(card, component) {
		if (!strcmp(component->name, codec_name)) {
			dev_dbg(component->dev, "re-enabling jack detect after resume\n");
			snd_soc_component_set_jack(component, &priv->jack, NULL);
			break;
		}
	}

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

>> +static int acp3x_es83xx_speaker_power_event(struct snd_soc_dapm_widget *w,
>> +					    struct snd_kcontrol *kcontrol, int event)
>> +{
>> +	struct acp3x_es83xx_private *priv = get_mach_priv(w->dapm->card);
>> +
>> +	dev_dbg(priv->codec_dev, "speaker power event: %d\n", event);
>> +	if (SND_SOC_DAPM_EVENT_ON(event))
>> +		acp3x_es83xx_set_gpios_values(priv, 1, 0);
>> +	else
>> +		acp3x_es83xx_set_gpios_values(priv, 0, 1);
>
> Why are these two GPIOs tied together like this?
>

These GPIOs represent the speaker and the headphone switches. When
activating the speaker GPIO you have to deactivate the headphone GPIO
and vice versa. The logic is taken from the discussion on the sofproject
pull request:
https://github.com/thesofproject/linux/pull/4112/commits/810d03e0aecdf0caf580a5179ee6873fb33485ab
and
https://github.com/thesofproject/linux/pull/4066

>> +static int acp3x_es83xx_suspend_pre(struct snd_soc_card *card)
>> +{
>> +	struct acp3x_es83xx_private *priv = get_mach_priv(card);
>> +
>> +	dev_dbg(priv->codec_dev, "card suspend\n");
>> +	snd_soc_component_set_jack(priv->codec, NULL, NULL);
>> +	return 0;
>> +}
>
> That's weird, why do that?

This is needed because if suspending the laptop with the headphones
inserted, when resuming, the sound is not working anymore. Sound stops
working on speakers and headphones. Reinsertion and removals of the
headphone doesn't solve the problem.

This seems to be caused by the fact
that the GPIO IRQ stops working in es8316_irq() after resume.
Now the call to snd_soc_component_set_jack() in suspend disables the
GPIO IRQ and in resume the GPIO IRQ is reactivated.
By the way this sequence is also used in bytcht_es8316.c in suspend and
resume:

static int byt_cht_es8316_suspend(struct snd_soc_card *card)
{
	struct snd_soc_component *component;

	for_each_card_components(card, component) {
		if (!strcmp(component->name, codec_name)) {
			dev_dbg(component->dev, "disabling jack detect before suspend\n");
			snd_soc_component_set_jack(component, NULL, NULL);
			break;
		}
	}

	return 0;
}

static int byt_cht_es8316_resume(struct snd_soc_card *card)
{
	struct byt_cht_es8316_private *priv = snd_soc_card_get_drvdata(card);
	struct snd_soc_component *component;

	for_each_card_components(card, component) {
		if (!strcmp(component->name, codec_name)) {
			dev_dbg(component->dev, "re-enabling jack detect after resume\n");
			snd_soc_component_set_jack(component, &priv->jack, NULL);
			break;
		}
	}

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Pierre-Louis Bossart]

>>> +static int acp3x_es83xx_speaker_power_event(struct snd_soc_dapm_widget *w,
>>> +					    struct snd_kcontrol *kcontrol, int event)
>>> +{
>>> +	struct acp3x_es83xx_private *priv = get_mach_priv(w->dapm->card);
>>> +
>>> +	dev_dbg(priv->codec_dev, "speaker power event: %d\n", event);
>>> +	if (SND_SOC_DAPM_EVENT_ON(event))
>>> +		acp3x_es83xx_set_gpios_values(priv, 1, 0);
>>> +	else
>>> +		acp3x_es83xx_set_gpios_values(priv, 0, 1);
>>
>> Why are these two GPIOs tied together like this?
>>
> 
> These GPIOs represent the speaker and the headphone switches. When
> activating the speaker GPIO you have to deactivate the headphone GPIO
> and vice versa. The logic is taken from the discussion on the sofproject
> pull request:
> https://github.com/thesofproject/linux/pull/4112/commits/810d03e0aecdf0caf580a5179ee6873fb33485ab
> and
> https://github.com/thesofproject/linux/pull/4066

These threads didn't exactly lead to a firm conclusion on how the GPIOs
should be used, IIRC there are cases where the levels are inverted and
all kinds of issues still not clear at all even after reading the tables
from ACPI _DSM methods.

I personally gave up, and I would recommend you take these threads as
inputs rather than firm directions.

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1589 bytes --]

On Wed, Mar 22, 2023 at 12:17:24AM +0200, Marian Postevca wrote:
> Mark Brown <broonie@kernel.org> writes:

> >> +	if (SND_SOC_DAPM_EVENT_ON(event))
> >> +		acp3x_es83xx_set_gpios_values(priv, 1, 0);
> >> +	else
> >> +		acp3x_es83xx_set_gpios_values(priv, 0, 1);

> > Why are these two GPIOs tied together like this?

> These GPIOs represent the speaker and the headphone switches. When
> activating the speaker GPIO you have to deactivate the headphone GPIO
> and vice versa. The logic is taken from the discussion on the sofproject
> pull request:
> https://github.com/thesofproject/linux/pull/4112/commits/810d03e0aecdf0caf580a5179ee6873fb33485ab
> and
> https://github.com/thesofproject/linux/pull/4066

Sure, but that doesn't answer the question.  What is the reason
they're tied together - what if someone wants to play back from
both speaker and headphones simultaneously?

> >> +static int acp3x_es83xx_suspend_pre(struct snd_soc_card *card)
> >> +{
> >> +	struct acp3x_es83xx_private *priv = get_mach_priv(card);
> >> +
> >> +	dev_dbg(priv->codec_dev, "card suspend\n");
> >> +	snd_soc_component_set_jack(priv->codec, NULL, NULL);
> >> +	return 0;
> >> +}

> > That's weird, why do that?

> This is needed because if suspending the laptop with the headphones
> inserted, when resuming, the sound is not working anymore. Sound stops
> working on speakers and headphones. Reinsertion and removals of the
> headphone doesn't solve the problem.

> This seems to be caused by the fact
> that the GPIO IRQ stops working in es8316_irq() after resume.

That's a bug that should be fixed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1589 bytes --]

On Wed, Mar 22, 2023 at 12:17:24AM +0200, Marian Postevca wrote:
> Mark Brown <broonie@kernel.org> writes:

> >> +	if (SND_SOC_DAPM_EVENT_ON(event))
> >> +		acp3x_es83xx_set_gpios_values(priv, 1, 0);
> >> +	else
> >> +		acp3x_es83xx_set_gpios_values(priv, 0, 1);

> > Why are these two GPIOs tied together like this?

> These GPIOs represent the speaker and the headphone switches. When
> activating the speaker GPIO you have to deactivate the headphone GPIO
> and vice versa. The logic is taken from the discussion on the sofproject
> pull request:
> https://github.com/thesofproject/linux/pull/4112/commits/810d03e0aecdf0caf580a5179ee6873fb33485ab
> and
> https://github.com/thesofproject/linux/pull/4066

Sure, but that doesn't answer the question.  What is the reason
they're tied together - what if someone wants to play back from
both speaker and headphones simultaneously?

> >> +static int acp3x_es83xx_suspend_pre(struct snd_soc_card *card)
> >> +{
> >> +	struct acp3x_es83xx_private *priv = get_mach_priv(card);
> >> +
> >> +	dev_dbg(priv->codec_dev, "card suspend\n");
> >> +	snd_soc_component_set_jack(priv->codec, NULL, NULL);
> >> +	return 0;
> >> +}

> > That's weird, why do that?

> This is needed because if suspending the laptop with the headphones
> inserted, when resuming, the sound is not working anymore. Sound stops
> working on speakers and headphones. Reinsertion and removals of the
> headphone doesn't solve the problem.

> This seems to be caused by the fact
> that the GPIO IRQ stops working in es8316_irq() after resume.

That's a bug that should be fixed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

> On Wed, Mar 22, 2023 at 12:17:24AM +0200, Marian Postevca wrote:
>> Mark Brown <broonie@kernel.org> writes:
>
>> >> +	if (SND_SOC_DAPM_EVENT_ON(event))
>> >> +		acp3x_es83xx_set_gpios_values(priv, 1, 0);
>> >> +	else
>> >> +		acp3x_es83xx_set_gpios_values(priv, 0, 1);
>
>> > Why are these two GPIOs tied together like this?
>
>> These GPIOs represent the speaker and the headphone switches. When
>> activating the speaker GPIO you have to deactivate the headphone GPIO
>> and vice versa. The logic is taken from the discussion on the sofproject
>> pull request:
>> https://github.com/thesofproject/linux/pull/4112/commits/810d03e0aecdf0caf580a5179ee6873fb33485ab
>> and
>> https://github.com/thesofproject/linux/pull/4066
>
> Sure, but that doesn't answer the question.  What is the reason
> they're tied together - what if someone wants to play back from
> both speaker and headphones simultaneously?
>

The GPIO handling is not documented in the codec datasheet, so I
constructed this logic by looking at the existing implementations of
machine drivers for this codec (sof_es8336.c, bytcht_es8316.c) and
comments of Everest Semiconductor engineers on the sofproject
pull requests. I'm saying all of this because I don't know the reasons
why these GPIOs work the way they do.

According to the Everest Semiconductor engineers this is the recommended
way to switch these GPIOs:

+--------------+--------------+----------------+
|              | Speaker GPIO | Headphone GPIO |
+--------------+--------------+----------------+
| Speaker on   | active       | inactive       |
| Headphone on | inactive     | active         |
| Suspended    | inactive     | inactive       |
+--------------+--------------+----------------+
(https://github.com/thesofproject/linux/pull/4066/commits/b7f12e46a36b74a9992920154a65cd55f5b0cdb4#r1041693056)

This lockstep between these two GPIOs can be seen in sof_es8336.c in
pcm_pop_work_events() too.

Regarding playing the speaker and headphone simultaneously, is not
something I took into account. Is this even a valid usecase? The intel driver
for es8336 doesn't seem to support it.

Maybe someone from Everest Semiconductor can comment on this GPIO handling?

>> >> +static int acp3x_es83xx_suspend_pre(struct snd_soc_card *card)
>> >> +{
>> >> +	struct acp3x_es83xx_private *priv = get_mach_priv(card);
>> >> +
>> >> +	dev_dbg(priv->codec_dev, "card suspend\n");
>> >> +	snd_soc_component_set_jack(priv->codec, NULL, NULL);
>> >> +	return 0;
>> >> +}
>
>> > That's weird, why do that?
>
>> This is needed because if suspending the laptop with the headphones
>> inserted, when resuming, the sound is not working anymore. Sound stops
>> working on speakers and headphones. Reinsertion and removals of the
>> headphone doesn't solve the problem.
>
>> This seems to be caused by the fact
>> that the GPIO IRQ stops working in es8316_irq() after resume.
>
> That's a bug that should be fixed.

Agreed, but I don't know how easy it is to fix, and I would like to
first offer users of these laptops a working sound driver.
Afterwards this issue can be analyzed and properly fixed.

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

> On Wed, Mar 22, 2023 at 12:17:24AM +0200, Marian Postevca wrote:
>> Mark Brown <broonie@kernel.org> writes:
>
>> >> +	if (SND_SOC_DAPM_EVENT_ON(event))
>> >> +		acp3x_es83xx_set_gpios_values(priv, 1, 0);
>> >> +	else
>> >> +		acp3x_es83xx_set_gpios_values(priv, 0, 1);
>
>> > Why are these two GPIOs tied together like this?
>
>> These GPIOs represent the speaker and the headphone switches. When
>> activating the speaker GPIO you have to deactivate the headphone GPIO
>> and vice versa. The logic is taken from the discussion on the sofproject
>> pull request:
>> https://github.com/thesofproject/linux/pull/4112/commits/810d03e0aecdf0caf580a5179ee6873fb33485ab
>> and
>> https://github.com/thesofproject/linux/pull/4066
>
> Sure, but that doesn't answer the question.  What is the reason
> they're tied together - what if someone wants to play back from
> both speaker and headphones simultaneously?
>

The GPIO handling is not documented in the codec datasheet, so I
constructed this logic by looking at the existing implementations of
machine drivers for this codec (sof_es8336.c, bytcht_es8316.c) and
comments of Everest Semiconductor engineers on the sofproject
pull requests. I'm saying all of this because I don't know the reasons
why these GPIOs work the way they do.

According to the Everest Semiconductor engineers this is the recommended
way to switch these GPIOs:

+--------------+--------------+----------------+
|              | Speaker GPIO | Headphone GPIO |
+--------------+--------------+----------------+
| Speaker on   | active       | inactive       |
| Headphone on | inactive     | active         |
| Suspended    | inactive     | inactive       |
+--------------+--------------+----------------+
(https://github.com/thesofproject/linux/pull/4066/commits/b7f12e46a36b74a9992920154a65cd55f5b0cdb4#r1041693056)

This lockstep between these two GPIOs can be seen in sof_es8336.c in
pcm_pop_work_events() too.

Regarding playing the speaker and headphone simultaneously, is not
something I took into account. Is this even a valid usecase? The intel driver
for es8336 doesn't seem to support it.

Maybe someone from Everest Semiconductor can comment on this GPIO handling?

>> >> +static int acp3x_es83xx_suspend_pre(struct snd_soc_card *card)
>> >> +{
>> >> +	struct acp3x_es83xx_private *priv = get_mach_priv(card);
>> >> +
>> >> +	dev_dbg(priv->codec_dev, "card suspend\n");
>> >> +	snd_soc_component_set_jack(priv->codec, NULL, NULL);
>> >> +	return 0;
>> >> +}
>
>> > That's weird, why do that?
>
>> This is needed because if suspending the laptop with the headphones
>> inserted, when resuming, the sound is not working anymore. Sound stops
>> working on speakers and headphones. Reinsertion and removals of the
>> headphone doesn't solve the problem.
>
>> This seems to be caused by the fact
>> that the GPIO IRQ stops working in es8316_irq() after resume.
>
> That's a bug that should be fixed.

Agreed, but I don't know how easy it is to fix, and I would like to
first offer users of these laptops a working sound driver.
Afterwards this issue can be analyzed and properly fixed.

Re: [PATCH 4/4] ASoC: amd: acp: Improve support for speaker power events

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

> On Mon, Mar 20, 2023 at 10:35:19PM +0200, Marian Postevca wrote:
>> In order to reduce the audible pops when speaker or headphones
>> are activated or disabled we need to delay the switching of the
>> GPIOs.
>
> The usual mechanism for doing this is with the standard kernel delay
> functions.  Why not use them in the DAPM event?
>

I just followed the logic from sof_es8336.c, the reason for the change
there is given in commit log of 89cdb224f2abe37ec:

commit 89cdb224f2abe37ec4ac21ba0d9ddeb5a6a9cf68
Author: Zhu Ning <zhuning0077@gmail.com>
Date:   Fri Oct 28 10:04:56 2022 +0800

    ASoC: sof_es8336: reduce pop noise on speaker
    
    The Speaker GPIO needs to be turned on slightly behind the codec turned on.
    It also need to be turned off slightly before the codec turned down.
    Current code uses delay in DAPM_EVENT to do it but the mdelay delays the
    DAPM itself and thus has no effect. A delayed_work is added to turn on the
    speaker.
    The Speaker is turned off in .trigger since trigger is called slightly
    before the DAPM events.
    
  
>> We need to also disable/enable the speaker/headphones GPIOs when
>> the audio stream is stopped/started. To avoid race conditions
>> between the speaker power event callback and the trigger callback
>> we use a ring buffer to save the events that we need to process
>> in the delayed work callback.
>
> Why is this required?  DAPM is integrated with stream start and stop,
> and there's a mute callback to mask any noise played back from the SoC
> while it stops and starts without requiring all this complexity.  If
> there is any audible noise then why would it only affect the speaker?
>

Same reason as above, just followed the logic in sof_es8336.c

>> +static int acp3x_es83xx_trigger(struct snd_pcm_substream *substream, int cmd)
>> +{
>> +	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
>> +	struct snd_soc_card *card = rtd->card;
>> +	struct acp3x_es83xx_private *priv = get_mach_priv(card);
>> +
>> +	switch (cmd) {
>> +	case SNDRV_PCM_TRIGGER_START:
>> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> +	case SNDRV_PCM_TRIGGER_RESUME:
>> +		if (substream->stream == 0) {
>> +			dev_dbg(priv->codec_dev, "trigger start/release/resume, activating GPIOs\n");
>> +			mutex_lock(&priv->rb_lock);
>
> Triggers run in atomic context, you can't use mutexes in atomic context.
> lockdep should tell you this.

Sorry, I didn't run lockdep before sending the patches, I will rework
the locking code.

Re: [PATCH 4/4] ASoC: amd: acp: Improve support for speaker power events

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

> On Mon, Mar 20, 2023 at 10:35:19PM +0200, Marian Postevca wrote:
>> In order to reduce the audible pops when speaker or headphones
>> are activated or disabled we need to delay the switching of the
>> GPIOs.
>
> The usual mechanism for doing this is with the standard kernel delay
> functions.  Why not use them in the DAPM event?
>

I just followed the logic from sof_es8336.c, the reason for the change
there is given in commit log of 89cdb224f2abe37ec:

commit 89cdb224f2abe37ec4ac21ba0d9ddeb5a6a9cf68
Author: Zhu Ning <zhuning0077@gmail.com>
Date:   Fri Oct 28 10:04:56 2022 +0800

    ASoC: sof_es8336: reduce pop noise on speaker
    
    The Speaker GPIO needs to be turned on slightly behind the codec turned on.
    It also need to be turned off slightly before the codec turned down.
    Current code uses delay in DAPM_EVENT to do it but the mdelay delays the
    DAPM itself and thus has no effect. A delayed_work is added to turn on the
    speaker.
    The Speaker is turned off in .trigger since trigger is called slightly
    before the DAPM events.
    
  
>> We need to also disable/enable the speaker/headphones GPIOs when
>> the audio stream is stopped/started. To avoid race conditions
>> between the speaker power event callback and the trigger callback
>> we use a ring buffer to save the events that we need to process
>> in the delayed work callback.
>
> Why is this required?  DAPM is integrated with stream start and stop,
> and there's a mute callback to mask any noise played back from the SoC
> while it stops and starts without requiring all this complexity.  If
> there is any audible noise then why would it only affect the speaker?
>

Same reason as above, just followed the logic in sof_es8336.c

>> +static int acp3x_es83xx_trigger(struct snd_pcm_substream *substream, int cmd)
>> +{
>> +	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
>> +	struct snd_soc_card *card = rtd->card;
>> +	struct acp3x_es83xx_private *priv = get_mach_priv(card);
>> +
>> +	switch (cmd) {
>> +	case SNDRV_PCM_TRIGGER_START:
>> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> +	case SNDRV_PCM_TRIGGER_RESUME:
>> +		if (substream->stream == 0) {
>> +			dev_dbg(priv->codec_dev, "trigger start/release/resume, activating GPIOs\n");
>> +			mutex_lock(&priv->rb_lock);
>
> Triggers run in atomic context, you can't use mutexes in atomic context.
> lockdep should tell you this.

Sorry, I didn't run lockdep before sending the patches, I will rework
the locking code.

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]

On Wed, Mar 22, 2023 at 10:48:28PM +0200, Marian Postevca wrote:

> Regarding playing the speaker and headphone simultaneously, is not
> something I took into account. Is this even a valid usecase? The intel driver
> for es8336 doesn't seem to support it.

Yes, for example consider a critical notification - the system
may wish to ensure it is audible even if the user has taken off
their headphones for some reason.

> >> This is needed because if suspending the laptop with the headphones
> >> inserted, when resuming, the sound is not working anymore. Sound stops
> >> working on speakers and headphones. Reinsertion and removals of the
> >> headphone doesn't solve the problem.

> >> This seems to be caused by the fact
> >> that the GPIO IRQ stops working in es8316_irq() after resume.

> > That's a bug that should be fixed.

> Agreed, but I don't know how easy it is to fix, and I would like to
> first offer users of these laptops a working sound driver.
> Afterwards this issue can be analyzed and properly fixed.

Surely if nothing else a good first step would be to have the
CODEC driver do whatever disabling the jack does on suspend
without needing the machine driver to bodge things?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]

On Wed, Mar 22, 2023 at 10:48:28PM +0200, Marian Postevca wrote:

> Regarding playing the speaker and headphone simultaneously, is not
> something I took into account. Is this even a valid usecase? The intel driver
> for es8336 doesn't seem to support it.

Yes, for example consider a critical notification - the system
may wish to ensure it is audible even if the user has taken off
their headphones for some reason.

> >> This is needed because if suspending the laptop with the headphones
> >> inserted, when resuming, the sound is not working anymore. Sound stops
> >> working on speakers and headphones. Reinsertion and removals of the
> >> headphone doesn't solve the problem.

> >> This seems to be caused by the fact
> >> that the GPIO IRQ stops working in es8316_irq() after resume.

> > That's a bug that should be fixed.

> Agreed, but I don't know how easy it is to fix, and I would like to
> first offer users of these laptops a working sound driver.
> Afterwards this issue can be analyzed and properly fixed.

Surely if nothing else a good first step would be to have the
CODEC driver do whatever disabling the jack does on suspend
without needing the machine driver to bodge things?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/4] ASoC: amd: acp: Improve support for speaker power events

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]

On Wed, Mar 22, 2023 at 11:01:48PM +0200, Marian Postevca wrote:
> Mark Brown <broonie@kernel.org> writes:

> > The usual mechanism for doing this is with the standard kernel delay
> > functions.  Why not use them in the DAPM event?

> I just followed the logic from sof_es8336.c, the reason for the change
> there is given in commit log of 89cdb224f2abe37ec:

> commit 89cdb224f2abe37ec4ac21ba0d9ddeb5a6a9cf68
> Author: Zhu Ning <zhuning0077@gmail.com>
> Date:   Fri Oct 28 10:04:56 2022 +0800

>     ASoC: sof_es8336: reduce pop noise on speaker

>     The Speaker GPIO needs to be turned on slightly behind the codec turned on.
>     It also need to be turned off slightly before the codec turned down.
>     Current code uses delay in DAPM_EVENT to do it but the mdelay delays the
>     DAPM itself and thus has no effect. A delayed_work is added to turn on the
>     speaker.
>     The Speaker is turned off in .trigger since trigger is called slightly
>     before the DAPM events.

This just sounds like a complicated way of implementing a DAPM
POST event?  Or now I think about it possibly we just need to
tweak the current sorting such that speakers aren't run in
parallel with headphones and line outputs, that should cover any
issues with external speaker amplifiers.  AFAICT the issue here
is a speaker driver amplifying a pop in a line output from the
CODEC?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/4] ASoC: amd: acp: Improve support for speaker power events

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]

On Wed, Mar 22, 2023 at 11:01:48PM +0200, Marian Postevca wrote:
> Mark Brown <broonie@kernel.org> writes:

> > The usual mechanism for doing this is with the standard kernel delay
> > functions.  Why not use them in the DAPM event?

> I just followed the logic from sof_es8336.c, the reason for the change
> there is given in commit log of 89cdb224f2abe37ec:

> commit 89cdb224f2abe37ec4ac21ba0d9ddeb5a6a9cf68
> Author: Zhu Ning <zhuning0077@gmail.com>
> Date:   Fri Oct 28 10:04:56 2022 +0800

>     ASoC: sof_es8336: reduce pop noise on speaker

>     The Speaker GPIO needs to be turned on slightly behind the codec turned on.
>     It also need to be turned off slightly before the codec turned down.
>     Current code uses delay in DAPM_EVENT to do it but the mdelay delays the
>     DAPM itself and thus has no effect. A delayed_work is added to turn on the
>     speaker.
>     The Speaker is turned off in .trigger since trigger is called slightly
>     before the DAPM events.

This just sounds like a complicated way of implementing a DAPM
POST event?  Or now I think about it possibly we just need to
tweak the current sorting such that speakers aren't run in
parallel with headphones and line outputs, that should cover any
issues with external speaker amplifiers.  AFAICT the issue here
is a speaker driver amplifying a pop in a line output from the
CODEC?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

>> >> This is needed because if suspending the laptop with the headphones
>> >> inserted, when resuming, the sound is not working anymore. Sound stops
>> >> working on speakers and headphones. Reinsertion and removals of the
>> >> headphone doesn't solve the problem.
>
>> >> This seems to be caused by the fact
>> >> that the GPIO IRQ stops working in es8316_irq() after resume.
>
>> > That's a bug that should be fixed.
>
>> Agreed, but I don't know how easy it is to fix, and I would like to
>> first offer users of these laptops a working sound driver.
>> Afterwards this issue can be analyzed and properly fixed.
>
> Surely if nothing else a good first step would be to have the
> CODEC driver do whatever disabling the jack does on suspend
> without needing the machine driver to bodge things?

I would go for properly analyzing what is going on and try to correctly fix it,
but it's going to take some time for me to do it. During this time
people with these laptops will not have working sound. Wouldn't it make
more sense to first deliver a working solution(even though it's not
perfect) than to make them wait? Also this workaround is already present
in the kernel, so it's not that critical that another driver uses it.

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

>> >> This is needed because if suspending the laptop with the headphones
>> >> inserted, when resuming, the sound is not working anymore. Sound stops
>> >> working on speakers and headphones. Reinsertion and removals of the
>> >> headphone doesn't solve the problem.
>
>> >> This seems to be caused by the fact
>> >> that the GPIO IRQ stops working in es8316_irq() after resume.
>
>> > That's a bug that should be fixed.
>
>> Agreed, but I don't know how easy it is to fix, and I would like to
>> first offer users of these laptops a working sound driver.
>> Afterwards this issue can be analyzed and properly fixed.
>
> Surely if nothing else a good first step would be to have the
> CODEC driver do whatever disabling the jack does on suspend
> without needing the machine driver to bodge things?

I would go for properly analyzing what is going on and try to correctly fix it,
but it's going to take some time for me to do it. During this time
people with these laptops will not have working sound. Wouldn't it make
more sense to first deliver a working solution(even though it's not
perfect) than to make them wait? Also this workaround is already present
in the kernel, so it's not that critical that another driver uses it.

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

On Tue, Mar 28, 2023 at 12:20:55AM +0300, Marian Postevca wrote:
> Mark Brown <broonie@kernel.org> writes:

> > Surely if nothing else a good first step would be to have the
> > CODEC driver do whatever disabling the jack does on suspend
> > without needing the machine driver to bodge things?

> I would go for properly analyzing what is going on and try to correctly fix it,
> but it's going to take some time for me to do it. During this time
> people with these laptops will not have working sound. Wouldn't it make

Did you try my suggestion of having the CODEC driver do whatever
is triggered by disabling the jack on suspend?  That should just
be simple code motion shouldn't it, not something that requires
extensive investigation?

> more sense to first deliver a working solution(even though it's not
> perfect) than to make them wait? Also this workaround is already present
> in the kernel, so it's not that critical that another driver uses it.

On the other hand if we're getting the same bodge done repeatedly
in a lot of machines that suggests it's something we should stop
spreading...

There was also the issue with automatically muting the speaker
when headphones are inserted which will affect UCM files.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

On Tue, Mar 28, 2023 at 12:20:55AM +0300, Marian Postevca wrote:
> Mark Brown <broonie@kernel.org> writes:

> > Surely if nothing else a good first step would be to have the
> > CODEC driver do whatever disabling the jack does on suspend
> > without needing the machine driver to bodge things?

> I would go for properly analyzing what is going on and try to correctly fix it,
> but it's going to take some time for me to do it. During this time
> people with these laptops will not have working sound. Wouldn't it make

Did you try my suggestion of having the CODEC driver do whatever
is triggered by disabling the jack on suspend?  That should just
be simple code motion shouldn't it, not something that requires
extensive investigation?

> more sense to first deliver a working solution(even though it's not
> perfect) than to make them wait? Also this workaround is already present
> in the kernel, so it's not that critical that another driver uses it.

On the other hand if we're getting the same bodge done repeatedly
in a lot of machines that suggests it's something we should stop
spreading...

There was also the issue with automatically muting the speaker
when headphones are inserted which will affect UCM files.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Marian Postevca]

Sorry for taking so long to look into this.
I think I understand why sound doesn't work if suspending with
headphones inserted.
When the jack is inserted, function
es8316_enable_micbias_for_mic_gnd_short_detect() is called and 3 pins
are enabled: Bias, Analog power, Mic Bias.

Since these 3 are supplies, the loop for_each_card_widgets() in
dapm_power_widgets() will always force the target_bias_level to be
SND_SOC_BIAS_STANDBY. So the codec component will never enter
SND_SOC_BIAS_OFF bias level.

When a suspend happens (with the headphones inserted) in
snd_soc_suspend(), in the for_each_rtd_components() loop, since the
bias level is SND_SOC_BIAS_STANDBY and dapm->idle_bias_off is true the
codec's suspend/resume functions are not called. Because of this the
codec stops running correctly.

Now I'm not sure what a proper fix would be. Enabling idle_bias_on in
struct snd_soc_component_driver fixes this issue. Would enabling
this option have any unwanted side effects?

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1785 bytes --]

On Fri, Aug 18, 2023 at 12:09:19AM +0300, Marian Postevca wrote:

> Sorry for taking so long to look into this.

You didn't quote any context here so it's not 100% clear what "this"
is...

> I think I understand why sound doesn't work if suspending with
> headphones inserted.
> When the jack is inserted, function
> es8316_enable_micbias_for_mic_gnd_short_detect() is called and 3 pins
> are enabled: Bias, Analog power, Mic Bias.

> Since these 3 are supplies, the loop for_each_card_widgets() in
> dapm_power_widgets() will always force the target_bias_level to be
> SND_SOC_BIAS_STANDBY. So the codec component will never enter
> SND_SOC_BIAS_OFF bias level.

> When a suspend happens (with the headphones inserted) in
> snd_soc_suspend(), in the for_each_rtd_components() loop, since the
> bias level is SND_SOC_BIAS_STANDBY and dapm->idle_bias_off is true the
> codec's suspend/resume functions are not called. Because of this the
> codec stops running correctly.

This is saying that the machine driver should disable jack detection
over suspend and restart it during resume.  The machine driver should
suspend before the rest of the card which should mean that the CODEC
gets powered off then.  The core can't tell if jack detection is
supposed to work over suspend, it is a standard wake event on systems
like phones, but it sounds like on this system the power gets removed
from the device so that can't work.

> Now I'm not sure what a proper fix would be. Enabling idle_bias_on in
> struct snd_soc_component_driver fixes this issue. Would enabling
> this option have any unwanted side effects?

I don't understand why that would be expected to help?  The main effect
of keeping the bias on all the time would be to consume more power.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:
>
> You didn't quote any context here so it's not 100% clear what "this"
> is...
>
The context was your question on why the machine driver in patch 3
disables the jack detection in suspend and enables it in resume.
I explained back then, that the reason for this handling,
is that during suspend with inserted headphones the sound stops
working after resume. I couldn't explain back then why that happened
but other machine drivers using the same codec also disabled the jack
detection during suspend and enabled it during resume.

> This is saying that the machine driver should disable jack detection
> over suspend and restart it during resume.  The machine driver should
> suspend before the rest of the card which should mean that the CODEC
> gets powered off then.  The core can't tell if jack detection is
> supposed to work over suspend, it is a standard wake event on systems
> like phones, but it sounds like on this system the power gets removed
> from the device so that can't work.
>

Sorry, I don't understand what you are trying to say here. My intention
is to find a way to have sound working when suspending/resuming
with jack inserted by not fudging the jack in the machine driver but
fixing it in the CODEC.

> I don't understand why that would be expected to help?  The main effect
> of keeping the bias on all the time would be to consume more power.

I don't fully understand the whole bias thing (I did try hard to
understand it from the code), but in this specific instance it helps for
suspending the CODEC. If idle_bias_on is true then idle_bias_off will be
false, that would mean that during suspend (with jack inserted)
in snd_soc_suspend() in switch (snd_soc_dapm_get_bias_level(dapm))
the flow goes in case SND_SOC_BIAS_STANDBY and since dapm->idle_bias_off
is false it doesn't break from the case but falls through to
SND_SOC_BIAS_OFF case which in turn calls the CODEC component's suspend
callback.

The reason the sound stops working with jack inserted is that the CODEC
suspend callback never gets called. It only gets called when the jack
is not inserted.

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1623 bytes --]

On Fri, Aug 18, 2023 at 01:20:06AM +0300, Marian Postevca wrote:
> Mark Brown <broonie@kernel.org> writes:

> > This is saying that the machine driver should disable jack detection
> > over suspend and restart it during resume.  The machine driver should
> > suspend before the rest of the card which should mean that the CODEC
> > gets powered off then.  The core can't tell if jack detection is
> > supposed to work over suspend, it is a standard wake event on systems
> > like phones, but it sounds like on this system the power gets removed
> > from the device so that can't work.

> Sorry, I don't understand what you are trying to say here. My intention
> is to find a way to have sound working when suspending/resuming
> with jack inserted by not fudging the jack in the machine driver but
> fixing it in the CODEC.

You'd need to pull the relevant supplies out of DAPM and handle them in
the CODEC suspend/resume callback.

> > I don't understand why that would be expected to help?  The main effect
> > of keeping the bias on all the time would be to consume more power.

> I don't fully understand the whole bias thing (I did try hard to
> understand it from the code), but in this specific instance it helps for
> suspending the CODEC. If idle_bias_on is true then idle_bias_off will be

Very old devices required keeping a reference voltage maintained at half
the analog supply voltage, this supply is called the bias.  It couldn't
be powered on/off quickly so needed to be kept on all the time.  I can't
tell which driver you're using here so I can't tell if it's maintaining
any system level power like that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

> You'd need to pull the relevant supplies out of DAPM and handle them in
> the CODEC suspend/resume callback.

Can you please suggest an approach that would be acceptable to you?
In the original patch series I sent, you didn't agree to the approach
to disable the jack detection in the machine driver suspend callback
and re-enable it in the resume callback. You suggested to do it in the
CODEC suspend/resume callbacks. As I explained previously (and Zhu Ning
confirmed in his email) this wouldn't work, since as long as the jack
detection is enabled, the CODEC suspend/resume callbacks do not get
called.
The second option (which you also do not seem to agree) is to enable
idle_bias_on.

So I don't know how to continue with this and merge this new machine
driver.

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 468 bytes --]

On Sun, Aug 20, 2023 at 12:32:52PM +0300, Marian Postevca wrote:
> Mark Brown <broonie@kernel.org> writes:

> > You'd need to pull the relevant supplies out of DAPM and handle them in
> > the CODEC suspend/resume callback.

> Can you please suggest an approach that would be acceptable to you?
> In the original patch series I sent, you didn't agree to the approach
> to disable the jack detection in the machine driver suspend callback

Put it in the machine driver.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]