Mark Brown broonie@kernel.org writes:
On Wed, Mar 22, 2023 at 12:17:24AM +0200, Marian Postevca wrote:
Mark Brown broonie@kernel.org writes:
- if (SND_SOC_DAPM_EVENT_ON(event))
acp3x_es83xx_set_gpios_values(priv, 1, 0);- else
acp3x_es83xx_set_gpios_values(priv, 0, 1);Why are these two GPIOs tied together like this?
These GPIOs represent the speaker and the headphone switches. When activating the speaker GPIO you have to deactivate the headphone GPIO and vice versa. The logic is taken from the discussion on the sofproject pull request: https://github.com/thesofproject/linux/pull/4112/commits/810d03e0aecdf0caf58... and https://github.com/thesofproject/linux/pull/4066
Sure, but that doesn't answer the question. What is the reason they're tied together - what if someone wants to play back from both speaker and headphones simultaneously?
The GPIO handling is not documented in the codec datasheet, so I constructed this logic by looking at the existing implementations of machine drivers for this codec (sof_es8336.c, bytcht_es8316.c) and comments of Everest Semiconductor engineers on the sofproject pull requests. I'm saying all of this because I don't know the reasons why these GPIOs work the way they do.
According to the Everest Semiconductor engineers this is the recommended way to switch these GPIOs:
+--------------+--------------+----------------+ | | Speaker GPIO | Headphone GPIO | +--------------+--------------+----------------+ | Speaker on | active | inactive | | Headphone on | inactive | active | | Suspended | inactive | inactive | +--------------+--------------+----------------+ (https://github.com/thesofproject/linux/pull/4066/commits/b7f12e46a36b74a9992...)
This lockstep between these two GPIOs can be seen in sof_es8336.c in pcm_pop_work_events() too.
Regarding playing the speaker and headphone simultaneously, is not something I took into account. Is this even a valid usecase? The intel driver for es8336 doesn't seem to support it.
Maybe someone from Everest Semiconductor can comment on this GPIO handling?
+static int acp3x_es83xx_suspend_pre(struct snd_soc_card *card) +{
- struct acp3x_es83xx_private *priv = get_mach_priv(card);
- dev_dbg(priv->codec_dev, "card suspend\n");
- snd_soc_component_set_jack(priv->codec, NULL, NULL);
- return 0;
+}
That's weird, why do that?
This is needed because if suspending the laptop with the headphones inserted, when resuming, the sound is not working anymore. Sound stops working on speakers and headphones. Reinsertion and removals of the headphone doesn't solve the problem.
This seems to be caused by the fact that the GPIO IRQ stops working in es8316_irq() after resume.
That's a bug that should be fixed.
Agreed, but I don't know how easy it is to fix, and I would like to first offer users of these laptops a working sound driver. Afterwards this issue can be analyzed and properly fixed.