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

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

[Zhu Ning]

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

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

[Zhu Ning]

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

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

[Mark Brown]

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

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.

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

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

[Mark Brown]

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

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.

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

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

[Mark Brown]

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

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

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

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

Put it in the machine driver.

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

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

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

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

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

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

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

[Mark Brown]

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

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

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

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

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

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

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

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

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

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

[Zhu Ning]

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

Hi Postevca, I just reproduced the bug on my side. You are right that
when jack inserted,
the codec driver will never enter  SND_SOC_BIAS_OFF or suspend or resume. This
makes sense because the codec needs micbias power to detect button. Problem is
that  ALSA or dapm thought the codec power is on during suspend, but
the system power
is turned off and the registers are reset. The system power is
controlled by EC and neither
dapm or codec driver can do anything about it. The system power is
just turned off.

Disabling the jack detection before suspend is ok because the system
stops working during
suspend anyway. Maybe a detailed comment is needed to explain why?

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

[Marian Postevca]

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

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

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

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

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

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

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

[Mark Brown]

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

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

> Sorry for taking so long to look into this.

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

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

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

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

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

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

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

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

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

[Marian Postevca]

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

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

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

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

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

[Mark Brown]

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

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

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

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

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

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

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

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

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

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

[Mark Brown]

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

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

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

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

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

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

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

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

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

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

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

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

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

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

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

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

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

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

[Mark Brown]

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

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?

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

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

[Mark Brown]

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

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?

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

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

[沈一超]

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

Re: [PATCH 3/4] ASoC: amd: acp: Add machine driver that enables sound for systems with a ES8336 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.

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

[Mark Brown]

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

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

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

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

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

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

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

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

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

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

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

[Mark Brown]

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

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

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

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

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

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

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

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

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

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

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

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

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

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

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

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

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

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

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

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

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

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

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

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

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

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

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

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

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

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

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

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

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

[Mark Brown]

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

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

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

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

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

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

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

> > That's weird, why do that?

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

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

That's a bug that should be fixed.

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

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

[Mark Brown]

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

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

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

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

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

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

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

> > That's weird, why do that?

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

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

That's a bug that should be fixed.

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

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

[Pierre-Louis Bossart]

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

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

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

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

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

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

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

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

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

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

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

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

	return 0;
}

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

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

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

[Marian Postevca]

Mark Brown <broonie@kernel.org> writes:

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

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

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

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

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

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

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

	return 0;
}

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

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

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

[kernel test robot]

Hi Marian,

Thank you for the patch! Perhaps something to improve:

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

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

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

All warnings (new ones prefixed by >>):

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


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

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

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

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

[Mark Brown]

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

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

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

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

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

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

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

Set this in the dai_link.

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

Why are these two GPIOs tied together like this?

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

That's weird, why do that?

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

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

[Mark Brown]

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

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

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

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

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

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

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

Set this in the dai_link.

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

Why are these two GPIOs tied together like this?

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

That's weird, why do that?

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

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

[Marian Postevca]

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

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

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