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

[PATCH v2 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 of this specific CODEC can be moved
outside the more generic ACP code.

Changes in v2:

- Removed patch 4: "ASoC: amd: acp: Improve support for speaker
  power events". May be resubmitted separately.
- Split the first commit that enabled support for S32 LE format and
  the MCLK div by 2 option into two separate commits.
- Removed the MCLK div by 2 DT property that was previously enabled by
  the machine driver. Now it's enabled by the CODEC driver if the MCLK
  frequency is equal or greater than 48Mhz.
- Used normal conditional statements.
- Removed constraint rates from the machine driver, the CODEC should be
  able to set them.
- Moved the DAI format configuration.
- Uncoupled the speaker and headphone GPIOs. Now they can be handled
  independently

Marian Postevca (4):
  ASoC: es8316: Enable support for S32 LE format
  ASoC: es8316: Enable support for 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

 sound/soc/amd/acp-config.c                    |  70 +++
 sound/soc/amd/acp/Makefile                    |   2 +-
 sound/soc/amd/acp/acp-legacy-mach.c           | 102 +++-
 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 | 449 ++++++++++++++++++
 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.h |  12 +
 sound/soc/codecs/es8316.c                     |  22 +-
 sound/soc/codecs/es8316.h                     |   3 +
 10 files changed, 724 insertions(+), 15 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.41.0

[PATCH v2 1/4] ASoC: es8316: Enable support for S32 LE format

[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.

Signed-off-by: Marian Postevca <posteuca@mutex.one>
---
 sound/soc/codecs/es8316.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/es8316.c b/sound/soc/codecs/es8316.c
index a8f347f1affb..09fc0b25f600 100644
--- a/sound/soc/codecs/es8316.c
+++ b/sound/soc/codecs/es8316.c
@@ -526,7 +526,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,
-- 
2.41.0

[PATCH v2 2/4] ASoC: es8316: Enable support for 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 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 | 20 ++++++++++++++++++--
 sound/soc/codecs/es8316.h |  3 +++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/es8316.c b/sound/soc/codecs/es8316.c
index 09fc0b25f600..b506cfb9bd5d 100644
--- a/sound/soc/codecs/es8316.c
+++ b/sound/soc/codecs/es8316.c
@@ -26,12 +26,19 @@
 /* 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 AMD SKU where MCLK is 48Mhz.
  */
 #define NR_SUPPORTED_MCLK_LRCK_RATIOS ARRAY_SIZE(supported_mclk_lrck_ratios)
 static const unsigned int supported_mclk_lrck_ratios[] = {
-	256, 384, 400, 500, 512, 768, 1024
+	256, 384, 400, 500, 512, 768, 1000, 1024
 };
 
+/* In at least one AMD laptop the internal timing of the codec goes off
+ * if the MCLK (48Mhz) is not divided by 2. So we will divide all MCLK
+ * frequencies above and equal to 48MHz by 2.
+ */
+#define MAX_SUPPORTED_MCLK_FREQ 48000000
+
 struct es8316_priv {
 	struct mutex lock;
 	struct clk *mclk;
@@ -470,6 +477,7 @@ static int es8316_pcm_hw_params(struct snd_pcm_substream *substream,
 	u8 bclk_divider;
 	u16 lrck_divider;
 	int i;
+	unsigned int mclk_div = 1;
 
 	/* Validate supported sample rates that are autodetected from MCLK */
 	for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) {
@@ -482,7 +490,15 @@ 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);
+
+	if (es8316->sysclk >= MAX_SUPPORTED_MCLK_FREQ) {
+		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:
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.41.0

[PATCH v2 3/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 2b3ec6594023..31f38ec4b1d1 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,
@@ -50,6 +54,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;
@@ -61,6 +73,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;
 };
@@ -69,4 +83,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.41.0

[PATCH v2 4/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           | 102 +++-
 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 | 449 ++++++++++++++++++
 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.h |  12 +
 8 files changed, 637 insertions(+), 12 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 f27c27580009..a58d646d28f6 100644
--- a/sound/soc/amd/acp-config.c
+++ b/sound/soc/amd/acp-config.c
@@ -61,6 +61,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 4e65fdbc8dca..dc70691bc293 100644
--- a/sound/soc/amd/acp/Makefile
+++ b/sound/soc/amd/acp/Makefile
@@ -17,7 +17,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 6d57d17ddfd7..1ab3edffe0ce 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,
@@ -75,6 +84,39 @@ static struct acp_card_drvdata rt5682s_rt1019_rmb_data = {
 	.tdm_mode = false,
 };
 
+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);
+	if (ret == 1)
+		return 0;
+	else
+		return ret;
+}
+
+static int acp_asoc_resume_post(struct snd_soc_card *card)
+{
+	int ret;
+
+	ret = acp_ops_resume_post(card);
+	if (ret == 1)
+		return 0;
+	else
+		return ret;
+}
+
 static int acp_asoc_probe(struct platform_device *pdev)
 {
 	struct snd_soc_card *card = NULL;
@@ -83,35 +125,68 @@ 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->drvdata = (struct acp_card_drvdata *)pdev->id_entry->driver_data;
-	/* Widgets and controls added per-codec in acp-mach-common.c */
 
-	acp_card_drvdata = card->drvdata;
+	acp_asoc_init_ops(card->drvdata);
+
+	/* If widgets and controls are not set in specific callback,
+	 * they will be added per-codec in acp-mach-common.c
+	 */
+	ret = acp_ops_configure_widgets(card);
+	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;
+		goto out;
 	}
-
-	return 0;
+out:
+	return ret;
 }
 
 static const struct platform_device_id board_ids[] = {
@@ -127,6 +202,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,
@@ -153,6 +232,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 a06af82b8056..8f968c12e54a 100644
--- a/sound/soc/amd/acp/acp-mach-common.c
+++ b/sound/soc/amd/acp/acp-mach-common.c
@@ -1513,6 +1513,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++;
@@ -1551,6 +1552,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 31f38ec4b1d1..b0a3f6bd172f 100644
--- a/sound/soc/amd/acp/acp-mach.h
+++ b/sound/soc/amd/acp/acp-mach.h
@@ -47,6 +47,7 @@ enum codec_endpoints {
 	NAU8825,
 	NAU8821,
 	MAX98388,
+	ES83XX,
 };
 
 enum platform_end_point {
@@ -74,6 +75,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 54235cad9cc9..b15cbdf7fa9b 100644
--- a/sound/soc/amd/acp/acp-renoir.c
+++ b/sound/soc/amd/acp/acp-renoir.c
@@ -69,6 +69,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..b4e0ea515352
--- /dev/null
+++ b/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
@@ -0,0 +1,449 @@
+// 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))
+
+#define DUAL_CHANNEL	2
+
+#define ES83XX_ENABLE_DMIC	BIT(4)
+#define ES83XX_48_MHZ_MCLK	BIT(5)
+
+struct acp3x_es83xx_private {
+	bool speaker_on;
+	bool headphone_on;
+	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 channels[] = {
+	DUAL_CHANNEL,
+};
+
+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_headphone_power_event(struct snd_soc_dapm_widget *w,
+					    struct snd_kcontrol *kcontrol, int event);
+static int acp3x_es83xx_speaker_power_event(struct snd_soc_dapm_widget *w,
+					    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_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");
+		freq = ES83xx_48_MHZ_MCLK_FREQ;
+	} else {
+		dev_dbg(priv->codec_dev, "using a 12.288Mhz MCLK\n");
+		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;
+	}
+
+	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("Headphone Power", SND_SOC_NOPM, 0, 0,
+			    acp3x_es83xx_headphone_power_event,
+			    SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMU),
+	SND_SOC_DAPM_SUPPLY("Speaker Power", SND_SOC_NOPM, 0, 0,
+			    acp3x_es83xx_speaker_power_event,
+			    SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMU),
+};
+
+static const struct snd_soc_dapm_route acp3x_es83xx_audio_map[] = {
+	{"Headphone", NULL, "HPOL"},
+	{"Headphone", NULL, "HPOR"},
+	{"Headphone", NULL, "Headphone Power"},
+
+	/*
+	 * There is no separate speaker output instead the speakers are muxed to
+	 * 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_headphone_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, "headphone power event = %d\n", event);
+	if (SND_SOC_DAPM_EVENT_ON(event))
+		priv->headphone_on = true;
+	else
+		priv->headphone_on = false;
+
+	acp3x_es83xx_set_gpios_values(priv, priv->speaker_on, priv->headphone_on);
+
+	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))
+		priv->speaker_on = true;
+	else
+		priv->speaker_on = false;
+
+	acp3x_es83xx_set_gpios_values(priv, priv->speaker_on, priv->headphone_on);
+
+	return 0;
+}
+
+static int acp3x_es83xx_suspend_pre(struct snd_soc_card *card)
+{
+	struct acp3x_es83xx_private *priv = get_mach_priv(card);
+
+	/* We need to disable the jack in the machine driver suspend
+	 * callback so that the CODEC suspend callback actually gets
+	 * called. Without doing it, the CODEC suspend/resume
+	 * callbacks do not get called if headphones are plugged in.
+	 * This is because plugging in headphones keeps some supplies
+	 * active, this in turn means that the lowest bias level
+	 * that the CODEC can go to is SND_SOC_BIAS_STANDBY.
+	 * If components do not set idle_bias_on to true then
+	 * their suspend/resume callbacks do not get called.
+	 */
+	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);
+
+	/* We disabled jack detection in suspend callback,
+	 * enable it back.
+	 */
+	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);
+	}
+
+	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;
+	link->dai_fmt = SND_SOC_DAIFMT_I2S
+		| SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBP_CFP;
+
+	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.41.0

Re: [PATCH v2 1/4] ASoC: es8316: Enable support for S32 LE format

[Mark Brown]

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

On Fri, Aug 25, 2023 at 12:01:32AM +0300, 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.

What's the issue?  The AMD code looks like it supports plenty of other
formats in most places.

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

Re: [PATCH v2 2/4] ASoC: es8316: Enable support for MCLK div by 2

[Mark Brown]

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

On Fri, Aug 25, 2023 at 12:01:33AM +0300, Marian Postevca wrote:

> +/* In at least one AMD laptop the internal timing of the codec goes off
> + * if the MCLK (48Mhz) is not divided by 2. So we will divide all MCLK
> + * frequencies above and equal to 48MHz by 2.
> + */
> +#define MAX_SUPPORTED_MCLK_FREQ 48000000

Given that the datasheet quotes a maximum MCLK of 51.2MHz I suspect that
this is far too high and that performance is degrading well before this
point, it sounds like it just so happens that you noticed issues on a
machine with this MCLK rather than that's based on the spec.  I would
instead suggest applying the MCLK divider in any case where we can do so
and still generate suitable clocking for the rest of the system, or at
least hit 256fs (the datasheet quotes 256/384fs on the front page which
suggests it's targetting 256fs, that'd be a fairly normal number, and
there's mention of 12/24MHz USB clocks being directly usable).  Doing
this should either make no odds or result in better performance.

It's probably also more power efficient to use a lower MCLK, though most
likely the difference is marginal.  The earlier in the clock tree the
divider is applied the lower more of the chip is clocked and all other
things being equal a lower clock usually means lower power.

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

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

[Mark Brown]

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

On Fri, Aug 25, 2023 at 12:01:35AM +0300, Marian Postevca wrote:

> +static int acp_asoc_suspend_pre(struct snd_soc_card *card)
> +{
> +	int ret;
> +
> +	ret = acp_ops_suspend_pre(card);
> +	if (ret == 1)
> +		return 0;
> +	else
> +		return ret;
> +}
> +
> +static int acp_asoc_resume_post(struct snd_soc_card *card)
> +{
> +	int ret;
> +
> +	ret = acp_ops_resume_post(card);
> +	if (ret == 1)
> +		return 0;
> +	else
> +		return ret;
> +}

This feels like it should've been part of the prior commit adding
support for more complex cards?

> +	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;

Similarly these changes in probe() - 

> +	{
> +		.name = "acp3x-es83xx",
> +		.driver_data = (kernel_ulong_t)&es83xx_rn_data,
> +	},

The main thing I'd expect to see in the generic code in a patch adding a
specific driver is table entries like this and the ones you had earlier.

> +		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;
> +			}
> +		}

This function should ideally have been using switch statemnts but that's
not an issue your patch introduced.

> +#define ES83XX_ENABLE_DMIC	BIT(4)
> +#define ES83XX_48_MHZ_MCLK	BIT(5)

> +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);
> +}

I'm not clear what this function buys us other than just calling gpiolib
directly in the DAPM events?

Otherewise the actual machine driver looks good.

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

Re: [PATCH v2 1/4] ASoC: es8316: Enable support for S32 LE format

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

>> 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.
>
> What's the issue?  The AMD code looks like it supports plenty of other
> formats in most places.

In previous version of the machine driver I used a different CPU
component than acp-i2s-sp. For that one, I couldn't get it to have
sound unless I specifically requested S32 LE format.

I removed S32_LE from the CODEC to test if it works and it
seems it does work with acp-is2-sp. Format S16_LE is chosen by
both components and sound can be heard. I guess this patch is
not really needed.

Re: [PATCH v2 1/4] ASoC: es8316: Enable support for S32 LE format

[Mark Brown]

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

On Sat, Aug 26, 2023 at 12:55:21AM +0300, Marian Postevca wrote:

> I removed S32_LE from the CODEC to test if it works and it
> seems it does work with acp-is2-sp. Format S16_LE is chosen by
> both components and sound can be heard. I guess this patch is
> not really needed.

If the device supports it it's a good change, it was just the bit about
it being a fix that I was querying.

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

Re: [PATCH v2 2/4] ASoC: es8316: Enable support for MCLK div by 2

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:


> Given that the datasheet quotes a maximum MCLK of 51.2MHz I suspect that
> this is far too high and that performance is degrading well before this
> point, it sounds like it just so happens that you noticed issues on a
> machine with this MCLK rather than that's based on the spec.  I would
> instead suggest applying the MCLK divider in any case where we can do so
> and still generate suitable clocking for the rest of the system, or at
> least hit 256fs (the datasheet quotes 256/384fs on the front page which
> suggests it's targetting 256fs, that'd be a fairly normal number, and
> there's mention of 12/24MHz USB clocks being directly usable).  Doing
> this should either make no odds or result in better performance.

Not 100% sure what checks should be done for a MCLK to determine if it
generates suitable clocking. Would something along this patch make
sense?

diff --git a/sound/soc/codecs/es8316.c b/sound/soc/codecs/es8316.c
index a8f347f1affb..667648de8105 100644
--- a/sound/soc/codecs/es8316.c
+++ b/sound/soc/codecs/es8316.c
@@ -470,19 +470,36 @@ static int es8316_pcm_hw_params(struct snd_pcm_substream *substream,
 	u8 bclk_divider;
 	u16 lrck_divider;
 	int i;
+	unsigned int clk = es8316->sysclk / 2;
+	bool clk_valid = false;
+
+	do {
+		/* Validate supported sample rates that are autodetected from MCLK */
+		for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) {
+			const unsigned int ratio = supported_mclk_lrck_ratios[i];
+
+			if (clk % ratio != 0)
+				continue;
+			if (clk / ratio == params_rate(params))
+				break;
+		}
+		if (i == NR_SUPPORTED_MCLK_LRCK_RATIOS) {
+			if (clk == es8316->sysclk)
+				return -EINVAL;
+			else
+				clk = es8316->sysclk;
+		} else {
+			clk_valid = true;
+		}
+	} while(!clk_valid);
 
-	/* Validate supported sample rates that are autodetected from MCLK */
-	for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) {
-		const unsigned int ratio = supported_mclk_lrck_ratios[i];
-
-		if (es8316->sysclk % ratio != 0)
-			continue;
-		if (es8316->sysclk / ratio == params_rate(params))
-			break;
+	if (clk != es8316->sysclk) {
+		snd_soc_component_update_bits(component, ES8316_CLKMGR_CLKSW,
+					      ES8316_CLKMGR_CLKSW_MCLK_DIV,
+					      ES8316_CLKMGR_CLKSW_MCLK_DIV);
 	}
-	if (i == NR_SUPPORTED_MCLK_LRCK_RATIOS)
-		return -EINVAL;
-	lrck_divider = es8316->sysclk / params_rate(params);
+
+	lrck_divider = clk / params_rate(params);
 	bclk_divider = lrck_divider / 4;
 	switch (params_format(params)) {
 	case SNDRV_PCM_FORMAT_S16_LE:

Re: [PATCH v2 2/4] ASoC: es8316: Enable support for MCLK div by 2

[Mark Brown]

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

On Mon, Aug 28, 2023 at 12:50:45AM +0300, Marian Postevca wrote:
> Mark Brown <broonie@kernel.org> writes:

> > machine with this MCLK rather than that's based on the spec.  I would
> > instead suggest applying the MCLK divider in any case where we can do so
> > and still generate suitable clocking for the rest of the system, or at
> > least hit 256fs (the datasheet quotes 256/384fs on the front page which
> > suggests it's targetting 256fs, that'd be a fairly normal number, and
> > there's mention of 12/24MHz USB clocks being directly usable).  Doing
> > this should either make no odds or result in better performance.

> Not 100% sure what checks should be done for a MCLK to determine if it
> generates suitable clocking. Would something along this patch make
> sense?

In general a MCLK that allows you to configure the dividers in the CODEC
appropriately for use.  So long as it works your change looks fine I
think modulo.

> +	do {
> +		/* Validate supported sample rates that are autodetected from MCLK */
> +		for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) {
> +			const unsigned int ratio = supported_mclk_lrck_ratios[i];
> +
> +			if (clk % ratio != 0)
> +				continue;
> +			if (clk / ratio == params_rate(params))
> +				break;
> +		}

Use ARRAY_SIZE()

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

Re: [PATCH v2 2/4] ASoC: es8316: Enable support for MCLK div by 2

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

>
> In general a MCLK that allows you to configure the dividers in the CODEC
> appropriately for use.  So long as it works your change looks fine I
> think modulo.
Sorry, maybe this question is dumb, but I am not familiar with this
expression. What do you mean by "your change looks fine I think modulo"?

>> +	do {
>> +		/* Validate supported sample rates that are autodetected from MCLK */
>> +		for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) {
>> +			const unsigned int ratio = supported_mclk_lrck_ratios[i];
>> +
>> +			if (clk % ratio != 0)
>> +				continue;
>> +			if (clk / ratio == params_rate(params))
>> +				break;
>> +		}
>
> Use ARRAY_SIZE()
>
Do you mean instead of all instances of NR_SUPPORTED_MCLK_LRCK_RATIOS?
If so, it's already defined as:
#define NR_SUPPORTED_MCLK_LRCK_RATIOS ARRAY_SIZE(supported_mclk_lrck_ratios)

If not, then I don't see where else I could use it.

Re: [PATCH v2 2/4] ASoC: es8316: Enable support for MCLK div by 2

[Mark Brown]

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

On Mon, Aug 28, 2023 at 11:22:15PM +0300, Marian Postevca wrote:
> Mark Brown <broonie@kernel.org> writes:

> > In general a MCLK that allows you to configure the dividers in the CODEC
> > appropriately for use.  So long as it works your change looks fine I
> > think modulo.

> Sorry, maybe this question is dumb, but I am not familiar with this
> expression. What do you mean by "your change looks fine I think modulo"?

"modulo" means "apart from".

> > Use ARRAY_SIZE()

> Do you mean instead of all instances of NR_SUPPORTED_MCLK_LRCK_RATIOS?
> If so, it's already defined as:
> #define NR_SUPPORTED_MCLK_LRCK_RATIOS ARRAY_SIZE(supported_mclk_lrck_ratios)

Yes, that's what I meant - it might be as well to just drop the define
then since it's prompting this question at use sites?  One of the goals
of ARRAY_SIZE() is to avoid having such defines and keep them in sync.

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