This series adds support for a line of HUAWEI laptops with
AMD CPUs that connect using the ACP3x module to a ES8336 codec.
The codec driver must be extended to support the S32 LE format
and the MCLK div by 2 option. MCLK div by 2 is needed for one specific
SKU which uses a 48Mhz MCLK which seems to be too high of a frequency
for the codec and must be divided by 2.
The acp legacy driver must also be extended by using callbacks so that
the more complicated handling for this specific codec can be moved
outside the more generic ACP code.
The last patch tries to avoid anoying pop sounds when the speaker/headphones
are enabled/disabled by delaying the handling of the GPIOs and using a mutex
to avoid race conditions between the speaker power event callback and the
trigger callback.
Marian Postevca (4):
ASoC: es8316: Enable support for S32 LE format and MCLK div by 2
ASoC: amd: acp: Add support for splitting the codec specific code from
the ACP driver
ASoC: amd: acp: Add machine driver that enables sound for systems with
a ES8336 codec
ASoC: amd: acp: Improve support for speaker power events
sound/soc/amd/acp-config.c | 70 ++
sound/soc/amd/acp/Makefile | 2 +-
sound/soc/amd/acp/acp-legacy-mach.c | 105 ++-
sound/soc/amd/acp/acp-mach-common.c | 8 +
sound/soc/amd/acp/acp-mach.h | 67 ++
sound/soc/amd/acp/acp-renoir.c | 4 +
sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c | 615 ++++++++++++++++++
sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.h | 12 +
sound/soc/codecs/es8316.c | 21 +-
sound/soc/codecs/es8316.h | 3 +
10 files changed, 886 insertions(+), 21 deletions(-)
create mode 100644 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
create mode 100644 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.h
--
2.39.1
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 <[email protected]>
---
sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c | 154 +++++++++++++++++-
1 file changed, 149 insertions(+), 5 deletions(-)
diff --git a/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c b/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
index 138a7c12669b..033c94e91928 100644
--- a/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
+++ b/sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
@@ -20,8 +20,18 @@
#include <linux/io.h>
#include <linux/acpi.h>
#include <linux/dmi.h>
+#include <linux/circ_buf.h>
+
#include "../acp-mach.h"
+#define RB_SIZE 32
+
+#define circ_count(circ) \
+ (CIRC_CNT((circ)->head, (circ)->tail, RB_SIZE))
+
+#define circ_space(circ) \
+ (CIRC_SPACE((circ)->head, (circ)->tail, RB_SIZE))
+
#define get_mach_priv(card) ((struct acp3x_es83xx_private *)((acp_get_drvdata(card))->mach_priv))
/* mclk-div-by-2 + terminating entry */
@@ -32,14 +42,35 @@
#define ES83XX_ENABLE_DMIC BIT(4)
#define ES83XX_48_MHZ_MCLK BIT(5)
+enum {
+ SPEAKER_ON = 0,
+ SPEAKER_OFF,
+ SPEAKER_SUSPEND,
+ SPEAKER_RESUME,
+ SPEAKER_MAX
+};
+
+const char *msg[SPEAKER_MAX] = {
+ "SPEAKER_ON",
+ "SPEAKER_OFF",
+ "SPEAKER_SUSPEND",
+ "SPEAKER_RESUME"
+};
+
struct acp3x_es83xx_private {
unsigned long quirk;
+ bool speaker_on;
+ bool stream_suspended;
struct snd_soc_component *codec;
struct device *codec_dev;
struct gpio_desc *gpio_speakers, *gpio_headphone;
struct acpi_gpio_params enable_spk_gpio, enable_hp_gpio;
struct acpi_gpio_mapping gpio_mapping[3];
struct snd_soc_dapm_route mic_map[2];
+ struct delayed_work jack_work;
+ struct mutex rb_lock;
+ struct circ_buf gpio_rb;
+ u8 gpio_events_buf[RB_SIZE];
};
static const unsigned int rates[] = {
@@ -86,6 +117,107 @@ static void acp3x_es83xx_set_gpios_values(struct acp3x_es83xx_private *priv,
gpiod_set_value_cansleep(priv->gpio_headphone, headphone);
}
+static void acp3x_es83xx_rb_insert_evt(struct circ_buf *rb, u8 val)
+{
+ u8 *buf = rb->buf;
+
+ if (circ_space(rb) == 0) {
+ /* make some space by dropping the oldest entry, we are more
+ * interested in the last event
+ */
+ rb->tail = (rb->tail + 1) & (RB_SIZE - 1);
+ }
+ buf[rb->head] = val;
+ rb->head = (rb->head + 1) & (RB_SIZE - 1);
+}
+
+static int acp3x_es83xx_rb_remove_evt(struct circ_buf *rb)
+{
+ u8 *buf = rb->buf;
+ int rc = -1;
+
+ if (circ_count(rb)) {
+ rc = buf[rb->tail];
+ rb->tail = (rb->tail + 1) & (RB_SIZE - 1);
+ }
+ return rc;
+}
+
+static void acp3x_es83xx_jack_events(struct work_struct *work)
+{
+ struct acp3x_es83xx_private *priv =
+ container_of(work, struct acp3x_es83xx_private, jack_work.work);
+ int evt;
+
+ mutex_lock(&priv->rb_lock);
+ do {
+ evt = acp3x_es83xx_rb_remove_evt(&priv->gpio_rb);
+ dev_dbg(priv->codec_dev, "jack event, %s\n", msg[evt]);
+ switch (evt) {
+ case SPEAKER_SUSPEND:
+ acp3x_es83xx_set_gpios_values(priv, 0, 0);
+ break;
+ case SPEAKER_RESUME:
+ acp3x_es83xx_set_gpios_values(priv, priv->speaker_on, !priv->speaker_on);
+ break;
+ case SPEAKER_ON:
+ priv->speaker_on = true;
+ acp3x_es83xx_set_gpios_values(priv, 1, 0);
+ break;
+ case SPEAKER_OFF:
+ priv->speaker_on = false;
+ acp3x_es83xx_set_gpios_values(priv, 0, 1);
+ break;
+ }
+ } while (evt != -1);
+ mutex_unlock(&priv->rb_lock);
+}
+
+static int acp3x_es83xx_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+ struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+ struct snd_soc_card *card = rtd->card;
+ struct acp3x_es83xx_private *priv = get_mach_priv(card);
+
+ switch (cmd) {
+ case SNDRV_PCM_TRIGGER_START:
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ case SNDRV_PCM_TRIGGER_RESUME:
+ if (substream->stream == 0) {
+ dev_dbg(priv->codec_dev, "trigger start/release/resume, activating GPIOs\n");
+ mutex_lock(&priv->rb_lock);
+ if (priv->stream_suspended) {
+ priv->stream_suspended = false;
+ acp3x_es83xx_rb_insert_evt(&priv->gpio_rb, SPEAKER_RESUME);
+ mutex_unlock(&priv->rb_lock);
+ queue_delayed_work(system_wq, &priv->jack_work,
+ msecs_to_jiffies(1));
+ } else {
+ mutex_unlock(&priv->rb_lock);
+ }
+ }
+
+ break;
+
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_STOP:
+ if (substream->stream == 0) {
+ dev_dbg(priv->codec_dev, "trigger pause/suspend/stop deactivating GPIOs\n");
+ mutex_lock(&priv->rb_lock);
+ priv->stream_suspended = true;
+ acp3x_es83xx_rb_insert_evt(&priv->gpio_rb, SPEAKER_SUSPEND);
+ mutex_unlock(&priv->rb_lock);
+ queue_delayed_work(system_wq, &priv->jack_work, msecs_to_jiffies(1));
+ }
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int acp3x_es83xx_create_swnode(struct device *codec_dev)
{
struct property_entry props[MAX_NO_PROPS] = {};
@@ -212,12 +344,17 @@ static int acp3x_es83xx_speaker_power_event(struct snd_soc_dapm_widget *w,
struct snd_kcontrol *kcontrol, int event)
{
struct acp3x_es83xx_private *priv = get_mach_priv(w->dapm->card);
+ u8 val;
+
+ val = SND_SOC_DAPM_EVENT_ON(event) ? SPEAKER_ON : SPEAKER_OFF;
- dev_dbg(priv->codec_dev, "speaker power event: %d\n", event);
- if (SND_SOC_DAPM_EVENT_ON(event))
- acp3x_es83xx_set_gpios_values(priv, 1, 0);
- else
- acp3x_es83xx_set_gpios_values(priv, 0, 1);
+ dev_dbg(priv->codec_dev, "speaker power event = %s\n", msg[val]);
+
+ mutex_lock(&priv->rb_lock);
+ acp3x_es83xx_rb_insert_evt(&priv->gpio_rb, val);
+ mutex_unlock(&priv->rb_lock);
+
+ queue_delayed_work(system_wq, &priv->jack_work, msecs_to_jiffies(70));
return 0;
}
@@ -353,6 +490,7 @@ static int acp3x_es83xx_init(struct snd_soc_pcm_runtime *runtime)
static const struct snd_soc_ops acp3x_es83xx_ops = {
.startup = acp3x_es83xx_codec_startup,
+ .trigger = acp3x_es83xx_trigger,
};
@@ -452,6 +590,12 @@ static int acp3x_es83xx_probe(struct snd_soc_card *card)
priv->codec_dev = codec_dev;
priv->quirk = (unsigned long)dmi_id->driver_data;
acp_drvdata->mach_priv = priv;
+
+ priv->gpio_rb.buf = priv->gpio_events_buf;
+ priv->gpio_rb.head = priv->gpio_rb.tail = 0;
+ mutex_init(&priv->rb_lock);
+
+ INIT_DELAYED_WORK(&priv->jack_work, acp3x_es83xx_jack_events);
dev_info(dev, "successfully probed the sound card\n");
} else {
ret = -ENODEV;
--
2.39.1
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 <[email protected]>
---
sound/soc/amd/acp/acp-mach.h | 65 ++++++++++++++++++++++++++++++++++++
1 file changed, 65 insertions(+)
diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h
index 165f407697c0..2cade68e6cc3 100644
--- a/sound/soc/amd/acp/acp-mach.h
+++ b/sound/soc/amd/acp/acp-mach.h
@@ -20,6 +20,10 @@
#define TDM_CHANNELS 8
+#define ACP_OPS(priv, cb) ((priv)->ops.cb)
+
+#define acp_get_drvdata(card) ((struct acp_card_drvdata *)(card)->drvdata)
+
enum be_id {
HEADSET_BE_ID = 0,
AMP_BE_ID,
@@ -48,6 +52,14 @@ enum platform_end_point {
REMBRANDT,
};
+struct acp_mach_ops {
+ int (*probe)(struct snd_soc_card *card);
+ int (*configure_link)(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link);
+ int (*configure_widgets)(struct snd_soc_card *card);
+ int (*suspend_pre)(struct snd_soc_card *card);
+ int (*resume_post)(struct snd_soc_card *card);
+};
+
struct acp_card_drvdata {
unsigned int hs_cpu_id;
unsigned int amp_cpu_id;
@@ -59,6 +71,8 @@ struct acp_card_drvdata {
unsigned int platform;
struct clk *wclk;
struct clk *bclk;
+ struct acp_mach_ops ops;
+ void *mach_priv;
bool soc_mclk;
bool tdm_mode;
};
@@ -67,4 +81,55 @@ int acp_sofdsp_dai_links_create(struct snd_soc_card *card);
int acp_legacy_dai_links_create(struct snd_soc_card *card);
extern const struct dmi_system_id acp_quirk_table[];
+static inline int acp_ops_probe(struct snd_soc_card *card)
+{
+ int ret = 1;
+ struct acp_card_drvdata *priv = acp_get_drvdata(card);
+
+ if (ACP_OPS(priv, probe))
+ ret = ACP_OPS(priv, probe)(card);
+ return ret;
+}
+
+static inline int acp_ops_configure_link(struct snd_soc_card *card,
+ struct snd_soc_dai_link *dai_link)
+{
+ int ret = 1;
+ struct acp_card_drvdata *priv = acp_get_drvdata(card);
+
+ if (ACP_OPS(priv, configure_link))
+ ret = ACP_OPS(priv, configure_link)(card, dai_link);
+ return ret;
+}
+
+static inline int acp_ops_configure_widgets(struct snd_soc_card *card)
+{
+ int ret = 1;
+ struct acp_card_drvdata *priv = acp_get_drvdata(card);
+
+ if (ACP_OPS(priv, configure_widgets))
+ ret = ACP_OPS(priv, configure_widgets)(card);
+ return ret;
+}
+
+static inline int acp_ops_suspend_pre(struct snd_soc_card *card)
+{
+ int ret = 1;
+ struct acp_card_drvdata *priv = acp_get_drvdata(card);
+
+ if (ACP_OPS(priv, suspend_pre))
+ ret = ACP_OPS(priv, suspend_pre)(card);
+ return ret;
+}
+
+static inline int acp_ops_resume_post(struct snd_soc_card *card)
+{
+ int ret = 1;
+ struct acp_card_drvdata *priv = acp_get_drvdata(card);
+
+ if (ACP_OPS(priv, resume_post))
+ ret = ACP_OPS(priv, resume_post)(card);
+ return ret;
+}
+
#endif
--
2.39.1
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 <[email protected]>
---
sound/soc/codecs/es8316.c | 21 +++++++++++++++++----
sound/soc/codecs/es8316.h | 3 +++
2 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/es8316.c b/sound/soc/codecs/es8316.c
index 056c3082fe02..acf21ef59b34 100644
--- a/sound/soc/codecs/es8316.c
+++ b/sound/soc/codecs/es8316.c
@@ -26,10 +26,11 @@
/* In slave mode at single speed, the codec is documented as accepting 5
* MCLK/LRCK ratios, but we also add ratio 400, which is commonly used on
* Intel Cherry Trail platforms (19.2MHz MCLK, 48kHz LRCK).
+ * Ratio 1000 is needed for at least one SKU where MCLK is 48Mhz.
*/
-#define NR_SUPPORTED_MCLK_LRCK_RATIOS 6
+#define NR_SUPPORTED_MCLK_LRCK_RATIOS 7
static const unsigned int supported_mclk_lrck_ratios[] = {
- 256, 384, 400, 512, 768, 1024
+ 256, 384, 400, 512, 768, 1000, 1024
};
struct es8316_priv {
@@ -465,6 +466,8 @@ static int es8316_pcm_hw_params(struct snd_pcm_substream *substream,
u8 bclk_divider;
u16 lrck_divider;
int i;
+ bool mclk_div_option = false;
+ unsigned int mclk_div = 1;
/* Validate supported sample rates that are autodetected from MCLK */
for (i = 0; i < NR_SUPPORTED_MCLK_LRCK_RATIOS; i++) {
@@ -477,7 +480,17 @@ static int es8316_pcm_hw_params(struct snd_pcm_substream *substream,
}
if (i == NR_SUPPORTED_MCLK_LRCK_RATIOS)
return -EINVAL;
- lrck_divider = es8316->sysclk / params_rate(params);
+
+ mclk_div_option = device_property_read_bool(component->dev,
+ "everest,mclk-div-by-2");
+ if (mclk_div_option) {
+ snd_soc_component_update_bits(component, ES8316_CLKMGR_CLKSW,
+ ES8316_CLKMGR_CLKSW_MCLK_DIV,
+ ES8316_CLKMGR_CLKSW_MCLK_DIV);
+ mclk_div = 2;
+ }
+
+ lrck_divider = es8316->sysclk / params_rate(params) / mclk_div;
bclk_divider = lrck_divider / 4;
switch (params_format(params)) {
case SNDRV_PCM_FORMAT_S16_LE:
@@ -520,7 +533,7 @@ static int es8316_mute(struct snd_soc_dai *dai, int mute, int direction)
}
#define ES8316_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \
- SNDRV_PCM_FMTBIT_S24_LE)
+ SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
static const struct snd_soc_dai_ops es8316_ops = {
.startup = es8316_pcm_startup,
diff --git a/sound/soc/codecs/es8316.h b/sound/soc/codecs/es8316.h
index c335138e2837..0ff16f948690 100644
--- a/sound/soc/codecs/es8316.h
+++ b/sound/soc/codecs/es8316.h
@@ -129,4 +129,7 @@
#define ES8316_GPIO_FLAG_GM_NOT_SHORTED 0x02
#define ES8316_GPIO_FLAG_HP_NOT_INSERTED 0x04
+/* ES8316_CLKMGR_CLKSW */
+#define ES8316_CLKMGR_CLKSW_MCLK_DIV 0x80
+
#endif
--
2.39.1
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 <[email protected]>
---
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 <[email protected]>
+
+#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 <[email protected]>
+ */
+
+#ifndef __ACP3X_ES83XX_H
+#define __ACP3X_ES83XX_H
+
+void acp3x_es83xx_init_ops(struct acp_mach_ops *ops);
+
+#endif
+
--
2.39.1
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?
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.
Hi Marian,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on broonie-sound/for-next]
[also build test WARNING on linus/master v6.3-rc3 next-20230320]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Marian-Postevca/ASoC-es8316-Enable-support-for-S32-LE-format-and-MCLK-div-by-2/20230321-043847
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
patch link: https://lore.kernel.org/r/20230320203519.20137-4-posteuca%40mutex.one
patch subject: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 codec
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230321/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/0562079546d40f50d025ea1c4dacd31120b8b0bc
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Marian-Postevca/ASoC-es8316-Enable-support-for-S32-LE-format-and-MCLK-div-by-2/20230321-043847
git checkout 0562079546d40f50d025ea1c4dacd31120b8b0bc
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash sound/soc/amd/acp/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c:464:6: warning: no previous prototype for 'acp3x_es83xx_init_ops' [-Wmissing-prototypes]
464 | void acp3x_es83xx_init_ops(struct acp_mach_ops *ops)
| ^~~~~~~~~~~~~~~~~~~~~
vim +/acp3x_es83xx_init_ops +464 sound/soc/amd/acp/acp3x-es83xx/acp3x-es83xx.c
462
463
> 464 void acp3x_es83xx_init_ops(struct acp_mach_ops *ops)
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Tue, Mar 21, 2023 at 07:09:43PM +0200, Marian Postevca wrote:
> Mark Brown <[email protected]> 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.
Mark Brown <[email protected]> writes:
>> +static int acp3x_es83xx_speaker_power_event(struct snd_soc_dapm_widget *w,
>> + struct snd_kcontrol *kcontrol, int event)
>> +{
>> + struct acp3x_es83xx_private *priv = get_mach_priv(w->dapm->card);
>> +
>> + dev_dbg(priv->codec_dev, "speaker power event: %d\n", event);
>> + if (SND_SOC_DAPM_EVENT_ON(event))
>> + acp3x_es83xx_set_gpios_values(priv, 1, 0);
>> + else
>> + acp3x_es83xx_set_gpios_values(priv, 0, 1);
>
> Why are these two GPIOs tied together like this?
>
These GPIOs represent the speaker and the headphone switches. When
activating the speaker GPIO you have to deactivate the headphone GPIO
and vice versa. The logic is taken from the discussion on the sofproject
pull request:
https://github.com/thesofproject/linux/pull/4112/commits/810d03e0aecdf0caf580a5179ee6873fb33485ab
and
https://github.com/thesofproject/linux/pull/4066
>> +static int acp3x_es83xx_suspend_pre(struct snd_soc_card *card)
>> +{
>> + struct acp3x_es83xx_private *priv = get_mach_priv(card);
>> +
>> + dev_dbg(priv->codec_dev, "card suspend\n");
>> + snd_soc_component_set_jack(priv->codec, NULL, NULL);
>> + return 0;
>> +}
>
> That's weird, why do that?
This is needed because if suspending the laptop with the headphones
inserted, when resuming, the sound is not working anymore. Sound stops
working on speakers and headphones. Reinsertion and removals of the
headphone doesn't solve the problem.
This seems to be caused by the fact
that the GPIO IRQ stops working in es8316_irq() after resume.
Now the call to snd_soc_component_set_jack() in suspend disables the
GPIO IRQ and in resume the GPIO IRQ is reactivated.
By the way this sequence is also used in bytcht_es8316.c in suspend and
resume:
static int byt_cht_es8316_suspend(struct snd_soc_card *card)
{
struct snd_soc_component *component;
for_each_card_components(card, component) {
if (!strcmp(component->name, codec_name)) {
dev_dbg(component->dev, "disabling jack detect before suspend\n");
snd_soc_component_set_jack(component, NULL, NULL);
break;
}
}
return 0;
}
static int byt_cht_es8316_resume(struct snd_soc_card *card)
{
struct byt_cht_es8316_private *priv = snd_soc_card_get_drvdata(card);
struct snd_soc_component *component;
for_each_card_components(card, component) {
if (!strcmp(component->name, codec_name)) {
dev_dbg(component->dev, "re-enabling jack detect after resume\n");
snd_soc_component_set_jack(component, &priv->jack, NULL);
break;
}
}
>>> +static int acp3x_es83xx_speaker_power_event(struct snd_soc_dapm_widget *w,
>>> + struct snd_kcontrol *kcontrol, int event)
>>> +{
>>> + struct acp3x_es83xx_private *priv = get_mach_priv(w->dapm->card);
>>> +
>>> + dev_dbg(priv->codec_dev, "speaker power event: %d\n", event);
>>> + if (SND_SOC_DAPM_EVENT_ON(event))
>>> + acp3x_es83xx_set_gpios_values(priv, 1, 0);
>>> + else
>>> + acp3x_es83xx_set_gpios_values(priv, 0, 1);
>>
>> Why are these two GPIOs tied together like this?
>>
>
> These GPIOs represent the speaker and the headphone switches. When
> activating the speaker GPIO you have to deactivate the headphone GPIO
> and vice versa. The logic is taken from the discussion on the sofproject
> pull request:
> https://github.com/thesofproject/linux/pull/4112/commits/810d03e0aecdf0caf580a5179ee6873fb33485ab
> and
> https://github.com/thesofproject/linux/pull/4066
These threads didn't exactly lead to a firm conclusion on how the GPIOs
should be used, IIRC there are cases where the levels are inverted and
all kinds of issues still not clear at all even after reading the tables
from ACPI _DSM methods.
I personally gave up, and I would recommend you take these threads as
inputs rather than firm directions.
On Wed, Mar 22, 2023 at 12:17:24AM +0200, Marian Postevca wrote:
> Mark Brown <[email protected]> writes:
> >> + if (SND_SOC_DAPM_EVENT_ON(event))
> >> + acp3x_es83xx_set_gpios_values(priv, 1, 0);
> >> + else
> >> + acp3x_es83xx_set_gpios_values(priv, 0, 1);
> > Why are these two GPIOs tied together like this?
> These GPIOs represent the speaker and the headphone switches. When
> activating the speaker GPIO you have to deactivate the headphone GPIO
> and vice versa. The logic is taken from the discussion on the sofproject
> pull request:
> https://github.com/thesofproject/linux/pull/4112/commits/810d03e0aecdf0caf580a5179ee6873fb33485ab
> and
> https://github.com/thesofproject/linux/pull/4066
Sure, but that doesn't answer the question. What is the reason
they're tied together - what if someone wants to play back from
both speaker and headphones simultaneously?
> >> +static int acp3x_es83xx_suspend_pre(struct snd_soc_card *card)
> >> +{
> >> + struct acp3x_es83xx_private *priv = get_mach_priv(card);
> >> +
> >> + dev_dbg(priv->codec_dev, "card suspend\n");
> >> + snd_soc_component_set_jack(priv->codec, NULL, NULL);
> >> + return 0;
> >> +}
> > That's weird, why do that?
> This is needed because if suspending the laptop with the headphones
> inserted, when resuming, the sound is not working anymore. Sound stops
> working on speakers and headphones. Reinsertion and removals of the
> headphone doesn't solve the problem.
> This seems to be caused by the fact
> that the GPIO IRQ stops working in es8316_irq() after resume.
That's a bug that should be fixed.
Mark Brown <[email protected]> writes:
> On Wed, Mar 22, 2023 at 12:17:24AM +0200, Marian Postevca wrote:
>> Mark Brown <[email protected]> writes:
>
>> >> + if (SND_SOC_DAPM_EVENT_ON(event))
>> >> + acp3x_es83xx_set_gpios_values(priv, 1, 0);
>> >> + else
>> >> + acp3x_es83xx_set_gpios_values(priv, 0, 1);
>
>> > Why are these two GPIOs tied together like this?
>
>> These GPIOs represent the speaker and the headphone switches. When
>> activating the speaker GPIO you have to deactivate the headphone GPIO
>> and vice versa. The logic is taken from the discussion on the sofproject
>> pull request:
>> https://github.com/thesofproject/linux/pull/4112/commits/810d03e0aecdf0caf580a5179ee6873fb33485ab
>> and
>> https://github.com/thesofproject/linux/pull/4066
>
> Sure, but that doesn't answer the question. What is the reason
> they're tied together - what if someone wants to play back from
> both speaker and headphones simultaneously?
>
The GPIO handling is not documented in the codec datasheet, so I
constructed this logic by looking at the existing implementations of
machine drivers for this codec (sof_es8336.c, bytcht_es8316.c) and
comments of Everest Semiconductor engineers on the sofproject
pull requests. I'm saying all of this because I don't know the reasons
why these GPIOs work the way they do.
According to the Everest Semiconductor engineers this is the recommended
way to switch these GPIOs:
+--------------+--------------+----------------+
| | Speaker GPIO | Headphone GPIO |
+--------------+--------------+----------------+
| Speaker on | active | inactive |
| Headphone on | inactive | active |
| Suspended | inactive | inactive |
+--------------+--------------+----------------+
(https://github.com/thesofproject/linux/pull/4066/commits/b7f12e46a36b74a9992920154a65cd55f5b0cdb4#r1041693056)
This lockstep between these two GPIOs can be seen in sof_es8336.c in
pcm_pop_work_events() too.
Regarding playing the speaker and headphone simultaneously, is not
something I took into account. Is this even a valid usecase? The intel driver
for es8336 doesn't seem to support it.
Maybe someone from Everest Semiconductor can comment on this GPIO handling?
>> >> +static int acp3x_es83xx_suspend_pre(struct snd_soc_card *card)
>> >> +{
>> >> + struct acp3x_es83xx_private *priv = get_mach_priv(card);
>> >> +
>> >> + dev_dbg(priv->codec_dev, "card suspend\n");
>> >> + snd_soc_component_set_jack(priv->codec, NULL, NULL);
>> >> + return 0;
>> >> +}
>
>> > That's weird, why do that?
>
>> This is needed because if suspending the laptop with the headphones
>> inserted, when resuming, the sound is not working anymore. Sound stops
>> working on speakers and headphones. Reinsertion and removals of the
>> headphone doesn't solve the problem.
>
>> This seems to be caused by the fact
>> that the GPIO IRQ stops working in es8316_irq() after resume.
>
> That's a bug that should be fixed.
Agreed, but I don't know how easy it is to fix, and I would like to
first offer users of these laptops a working sound driver.
Afterwards this issue can be analyzed and properly fixed.
Mark Brown <[email protected]> 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 <[email protected]>
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 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?
On Wed, Mar 22, 2023 at 11:01:48PM +0200, Marian Postevca wrote:
> Mark Brown <[email protected]> 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 <[email protected]>
> 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?
> > 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.
In practice the speaker and the headphone are conflicting devices, see
https://github.com/alsa-project/alsa-ucm-conf/blob/master/ucm2/Intel/sof-essx8336/HiFi.conf
There is no such situation, and the system doesn't produce sound from speaker
when headphones are plugged in. The user may manually open speaker using
amixer sset 'Speaker' on or pavucontrol.
The GPIO thing on Huawei Intel platform is too complicated, they use two GPIOs
for headphone and speaker, and the headphone GPIO is inverted, which means low
means on and high means off. Luckily there should be only one hardware config
for the AMD acp3x platform GPIO and you may just choose the correct one.
On Thu, Mar 23, 2023 at 09:56:56AM +0800, 沈一超 wrote:
> > 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.
> In practice the speaker and the headphone are conflicting devices, see
> https://github.com/alsa-project/alsa-ucm-conf/blob/master/ucm2/Intel/sof-essx8336/HiFi.conf
That's a particular UCM configuration that someone has created, someone
with a different set of use cases might create a different UCM setup (or
not use UCM at all). This is a large part of the point of having use
case configuration in userspace.
> There is no such situation, and the system doesn't produce sound from speaker
> when headphones are plugged in. The user may manually open speaker using
> amixer sset 'Speaker' on or pavucontrol.
Again, you're describing a specific configuration - someone might want
to do something different.
> The GPIO thing on Huawei Intel platform is too complicated, they use two GPIOs
> for headphone and speaker, and the headphone GPIO is inverted, which means low
> means on and high means off. Luckily there should be only one hardware config
> for the AMD acp3x platform GPIO and you may just choose the correct one.
There being two different GPIOs sounds like it just allows the headphone
and speaker to be controlled separately - that seems more flexible, not
a problem?
> > The GPIO thing on Huawei Intel platform is too complicated, they use two GPIOs
> > for headphone and speaker, and the headphone GPIO is inverted, which means low
> > means on and high means off. Luckily there should be only one hardware config
> > for the AMD acp3x platform GPIO and you may just choose the correct one.
>
> There being two different GPIOs sounds like it just allows the headphone
> and speaker to be controlled separately - that seems more flexible, not
> a problem?
Yes it's called multi stream in Windows. However, extra GPIO causes
more confusion
in the driver.
> > There is no such situation, and the system doesn't produce sound from speaker
> > when headphones are plugged in. The user may manually open speaker using
> > amixer sset 'Speaker' on or pavucontrol.
>
> Again, you're describing a specific configuration - someone might want
> to do something different.
Hi Marian Postevca you may want to separate the GPIO control by adding Headphone
Power SND_SOC_DAPM_SUPPLY. You may also want to change the gpio
handling function
in the acp3x_es83xx_jack_events function.
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"},
{"Speaker", NULL, "HPOL"},
{"Speaker", NULL, "HPOR"},
{"Speaker", NULL, "Speaker Power"},
};
+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, "speaker power event: %d\n", event);
+ if (SND_SOC_DAPM_EVENT_ON(event))
+ gpiod_set_value_cansleep(priv->gpio_headphone, true);
+ else
+ gpiod_set_value_cansleep(priv->gpio_headphone, false);
+
+ 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))
+ gpiod_set_value_cansleep(priv->gpio_speakers, true);
+ else
+ gpiod_set_value_cansleep(priv->gpio_speakers, false);
+
+ return 0;
+}
Mark Brown <[email protected]> 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 <[email protected]> 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.
On Fri, Mar 24, 2023 at 09:54:41AM +0800, Zhu Ning wrote:
> > There being two different GPIOs sounds like it just allows the headphone
> > and speaker to be controlled separately - that seems more flexible, not
> > a problem?
> Yes it's called multi stream in Windows. However, extra GPIO causes
> more confusion
> in the driver.
That might be true on Windows, however with these being
representable as DAPM widgets on Linux I would be surprised if
there were much burden on Linux.
> + 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),
Note that both of these would be much better represented as
events on the actual headphone or speaker widget, these have
event callbacks. This will ensure that they are sequenced after
the CODEC minimising pops and clicks.
On Fri, Aug 18, 2023 at 01:20:06AM +0300, Marian Postevca wrote:
> Mark Brown <[email protected]> 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.
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?