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

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

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.
participants (2)
-
Mark Brown
-
Zhu Ning