[PATCH 0/4] ASoC: amd: acp: Add sound support for a line of HUAWEI laptops
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
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
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.
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.
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.
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
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 +
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?
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/810d03e0aecdf0caf58... 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; } }
+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/810d03e0aecdf0caf58... 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.
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/810d03e0aecdf0caf58... 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.
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/810d03e0aecdf0caf58... 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/b7f12e46a36b74a9992...)
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.
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?
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.
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.
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?
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.
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.
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.
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.
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.
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-E... 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@i...) 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/0562079546d40f50d025ea1c4dacd3... 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)
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;
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.
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.
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?
participants (4)
-
kernel test robot -
Marian Postevca -
Mark Brown -
Pierre-Louis Bossart