[PATCH] Revert "ASoC: amd: acp: Power on/off the speaker enable gpio pin based on DAPM callback."

[PATCH] Revert "ASoC: amd: acp: Power on/off the speaker enable gpio pin based on DAPM callback."

[Curtis Malainey]

This reverts commit 5c5f08f7fc0bee9a1bc3fbdcb7a21cfd0648ab14.

This commit hard codes GPIO for max98357a drivers at a board specific
level in the machine driver in the kernel itself. When used with a
board that properly defines the GPIO pins in ACPI there is a resource
contention over the GPIO pin and the card fails to probe.

The amplifier driver has handled this pin lookup long before the
this change landed and it the pin should continue to be owned by the
amplifier as it is specific to that component. As a result this should
be reverted.

Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
Cc: Rob Barnes <robbarnes@google.com>
Cc: Eric Peers <epeers@google.com>
---
 sound/soc/amd/acp/Kconfig           |  6 +++---
 sound/soc/amd/acp/acp-legacy-mach.c | 19 +++----------------
 sound/soc/amd/acp/acp-mach-common.c | 25 -------------------------
 sound/soc/amd/acp/acp-mach.h        | 10 +---------
 sound/soc/amd/acp/acp-sof-mach.c    | 21 +++------------------
 5 files changed, 10 insertions(+), 71 deletions(-)

diff --git a/sound/soc/amd/acp/Kconfig b/sound/soc/amd/acp/Kconfig
index d5838df3064b..154be5e70821 100644
--- a/sound/soc/amd/acp/Kconfig
+++ b/sound/soc/amd/acp/Kconfig
@@ -32,7 +32,7 @@ config SND_AMD_ASOC_RENOIR
 
 config SND_SOC_AMD_MACH_COMMON
 	tristate
-	depends on X86 && PCI && I2C && GPIOLIB
+	depends on X86 && PCI && I2C
 	select CLK_FIXED_FCH
 	select SND_SOC_RT5682_I2C
 	select SND_SOC_DMIC
@@ -44,14 +44,14 @@ config SND_SOC_AMD_MACH_COMMON
 
 config SND_SOC_AMD_LEGACY_MACH
 	tristate "AMD Legacy Machine Driver Support"
-	depends on X86 && PCI && I2C && GPIOLIB
+	depends on X86 && PCI && I2C
 	select SND_SOC_AMD_MACH_COMMON
 	help
 	  This option enables legacy sound card support for ACP audio.
 
 config SND_SOC_AMD_SOF_MACH
 	tristate "AMD SOF Machine Driver Support"
-	depends on X86 && PCI && I2C && GPIOLIB
+	depends on X86 && PCI && I2C
 	select SND_SOC_AMD_MACH_COMMON
 	help
 	  This option enables SOF sound card support for ACP audio.
diff --git a/sound/soc/amd/acp/acp-legacy-mach.c b/sound/soc/amd/acp/acp-legacy-mach.c
index 0ad1cf41b308..de0f8024e2fb 100644
--- a/sound/soc/amd/acp/acp-legacy-mach.c
+++ b/sound/soc/amd/acp/acp-legacy-mach.c
@@ -27,7 +27,6 @@ static struct acp_card_drvdata rt5682_rt1019_data = {
 	.hs_codec_id = RT5682,
 	.amp_codec_id = RT1019,
 	.dmic_codec_id = NONE,
-	.gpio_spkr_en = EN_SPKR_GPIO_GB,
 };
 
 static const struct snd_kcontrol_new acp_controls[] = {
@@ -42,16 +41,15 @@ static const struct snd_kcontrol_new acp_controls[] = {
 static const struct snd_soc_dapm_widget acp_widgets[] = {
 	SND_SOC_DAPM_HP("Headphone Jack", NULL),
 	SND_SOC_DAPM_MIC("Headset Mic", NULL),
-	SND_SOC_DAPM_SPK("Spk", event_spkr_handler),
-	SND_SOC_DAPM_SPK("Left Spk", event_spkr_handler),
-	SND_SOC_DAPM_SPK("Right Spk", event_spkr_handler),
+	SND_SOC_DAPM_SPK("Spk", NULL),
+	SND_SOC_DAPM_SPK("Left Spk", NULL),
+	SND_SOC_DAPM_SPK("Right Spk", NULL),
 };
 
 static int acp_asoc_probe(struct platform_device *pdev)
 {
 	struct snd_soc_card *card = NULL;
 	struct device *dev = &pdev->dev;
-	unsigned int spkr_gpio;
 	int ret;
 
 	if (!pdev->id_entry)
@@ -69,20 +67,9 @@ static int acp_asoc_probe(struct platform_device *pdev)
 	card->controls = acp_controls;
 	card->num_controls = ARRAY_SIZE(acp_controls);
 	card->drvdata = (struct acp_card_drvdata *)pdev->id_entry->driver_data;
-	spkr_gpio = ((struct acp_card_drvdata *)(card->drvdata))->gpio_spkr_en;
 
 	acp_legacy_dai_links_create(card);
 
-	if (gpio_is_valid(spkr_gpio)) {
-		ret = devm_gpio_request(dev, spkr_gpio, "spkren");
-		if (ret) {
-			dev_err(dev, "(%s) gpio request failed: %d\n",
-				__func__, ret);
-			return ret;
-		}
-		gpio_direction_output(spkr_gpio, 0);
-	}
-
 	ret = devm_snd_soc_register_card(&pdev->dev, card);
 	if (ret) {
 		dev_err(&pdev->dev,
diff --git a/sound/soc/amd/acp/acp-mach-common.c b/sound/soc/amd/acp/acp-mach-common.c
index c9caade5cb74..7386e5bb61b5 100644
--- a/sound/soc/amd/acp/acp-mach-common.c
+++ b/sound/soc/amd/acp/acp-mach-common.c
@@ -71,31 +71,6 @@ static const struct snd_soc_dapm_route rt5682_map[] = {
 	{ "IN1P", NULL, "Headset Mic" },
 };
 
-int event_spkr_handler(struct snd_soc_dapm_widget *w,
-			struct snd_kcontrol *k, int event)
-{
-	struct snd_soc_dapm_context *dapm = w->dapm;
-	struct snd_soc_card *card = dapm->card;
-	struct acp_card_drvdata *drvdata = snd_soc_card_get_drvdata(card);
-
-	if (!gpio_is_valid(drvdata->gpio_spkr_en))
-		return 0;
-
-	switch (event) {
-	case SND_SOC_DAPM_POST_PMU:
-		gpio_set_value(drvdata->gpio_spkr_en, 1);
-		break;
-	case SND_SOC_DAPM_PRE_PMD:
-		gpio_set_value(drvdata->gpio_spkr_en, 0);
-		break;
-	default:
-		dev_warn(card->dev, "%s invalid setting\n", __func__);
-		break;
-	}
-	return 0;
-}
-EXPORT_SYMBOL_NS_GPL(event_spkr_handler, SND_SOC_AMD_MACH);
-
 /* Define card ops for RT5682 CODEC */
 static int acp_card_rt5682_init(struct snd_soc_pcm_runtime *rtd)
 {
diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h
index fd6299844ebe..5dc47cfbff10 100644
--- a/sound/soc/amd/acp/acp-mach.h
+++ b/sound/soc/amd/acp/acp-mach.h
@@ -17,12 +17,6 @@
 #include <linux/input.h>
 #include <linux/module.h>
 #include <sound/soc.h>
-#include <linux/gpio.h>
-#include <linux/gpio/consumer.h>
-
-#define EN_SPKR_GPIO_GB                0x11F
-#define EN_SPKR_GPIO_NK                0x146
-#define EN_SPKR_GPIO_NONE      -EINVAL
 
 enum be_id {
 	HEADSET_BE_ID = 0,
@@ -55,11 +49,9 @@ struct acp_card_drvdata {
 	unsigned int dai_fmt;
 	struct clk *wclk;
 	struct clk *bclk;
-	unsigned int gpio_spkr_en;
 };
 
 int acp_sofdsp_dai_links_create(struct snd_soc_card *card);
 int acp_legacy_dai_links_create(struct snd_soc_card *card);
-int event_spkr_handler(struct snd_soc_dapm_widget *w,
-			struct snd_kcontrol *k, int event);
+
 #endif
diff --git a/sound/soc/amd/acp/acp-sof-mach.c b/sound/soc/amd/acp/acp-sof-mach.c
index c1d9650fc222..dc2786cc81ef 100644
--- a/sound/soc/amd/acp/acp-sof-mach.c
+++ b/sound/soc/amd/acp/acp-sof-mach.c
@@ -27,7 +27,6 @@ static struct acp_card_drvdata sof_rt5682_rt1019_data = {
 	.hs_codec_id = RT5682,
 	.amp_codec_id = RT1019,
 	.dmic_codec_id = DMIC,
-	.gpio_spkr_en = EN_SPKR_GPIO_GB,
 };
 
 static struct acp_card_drvdata sof_rt5682_max_data = {
@@ -37,7 +36,6 @@ static struct acp_card_drvdata sof_rt5682_max_data = {
 	.hs_codec_id = RT5682,
 	.amp_codec_id = MAX98360A,
 	.dmic_codec_id = DMIC,
-	.gpio_spkr_en = EN_SPKR_GPIO_NK,
 };
 
 static struct acp_card_drvdata sof_rt5682s_rt1019_data = {
@@ -56,7 +54,6 @@ static struct acp_card_drvdata sof_rt5682s_max_data = {
 	.hs_codec_id = RT5682S,
 	.amp_codec_id = MAX98360A,
 	.dmic_codec_id = DMIC,
-	.gpio_spkr_en = EN_SPKR_GPIO_NK,
 };
 
 static const struct snd_kcontrol_new acp_controls[] = {
@@ -70,16 +67,15 @@ static const struct snd_kcontrol_new acp_controls[] = {
 static const struct snd_soc_dapm_widget acp_widgets[] = {
 	SND_SOC_DAPM_HP("Headphone Jack", NULL),
 	SND_SOC_DAPM_MIC("Headset Mic", NULL),
-	SND_SOC_DAPM_SPK("Spk", event_spkr_handler),
-	SND_SOC_DAPM_SPK("Left Spk", event_spkr_handler),
-	SND_SOC_DAPM_SPK("Right Spk", event_spkr_handler),
+	SND_SOC_DAPM_SPK("Spk", NULL),
+	SND_SOC_DAPM_SPK("Left Spk", NULL),
+	SND_SOC_DAPM_SPK("Right Spk", NULL),
 };
 
 static int acp_sof_probe(struct platform_device *pdev)
 {
 	struct snd_soc_card *card = NULL;
 	struct device *dev = &pdev->dev;
-	unsigned int spkr_gpio;
 	int ret;
 
 	if (!pdev->id_entry)
@@ -97,20 +93,9 @@ static int acp_sof_probe(struct platform_device *pdev)
 	card->controls = acp_controls;
 	card->num_controls = ARRAY_SIZE(acp_controls);
 	card->drvdata = (struct acp_card_drvdata *)pdev->id_entry->driver_data;
-	spkr_gpio = ((struct acp_card_drvdata *)(card->drvdata))->gpio_spkr_en;
 
 	acp_sofdsp_dai_links_create(card);
 
-	if (gpio_is_valid(spkr_gpio)) {
-		ret = devm_gpio_request(dev, spkr_gpio, "spkren");
-		if (ret) {
-			dev_err(dev, "(%s) gpio request failed: %d\n",
-				__func__, ret);
-			return ret;
-		}
-		gpio_direction_output(spkr_gpio, 0);
-	}
-
 	ret = devm_snd_soc_register_card(&pdev->dev, card);
 	if (ret) {
 		dev_err(&pdev->dev,
-- 
2.32.0

Re: [PATCH] Revert "ASoC: amd: acp: Power on/off the speaker enable gpio pin based on DAPM callback."

[Ajit Kumar Pandey]

On 1/29/2022 5:38 AM, Curtis Malainey wrote:
> [CAUTION: External Email]
> 
> This reverts commit 5c5f08f7fc0bee9a1bc3fbdcb7a21cfd0648ab14.
> 
> This commit hard codes GPIO for max98357a drivers at a board specific
> level in the machine driver in the kernel itself. When used with a
> board that properly defines the GPIO pins in ACPI there is a resource
> contention over the GPIO pin and the card fails to probe.
> 
> The amplifier driver has handled this pin lookup long before the
> this change landed and it the pin should continue to be owned by the
> amplifier as it is specific to that component. As a result this should
> be reverted.
> 
> Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
> Cc: Rob Barnes <robbarnes@google.com>
> Cc: Eric Peers <epeers@google.com>
> ---
>   sound/soc/amd/acp/Kconfig           |  6 +++---
>   sound/soc/amd/acp/acp-legacy-mach.c | 19 +++----------------
>   sound/soc/amd/acp/acp-mach-common.c | 25 -------------------------
>   sound/soc/amd/acp/acp-mach.h        | 10 +---------
>   sound/soc/amd/acp/acp-sof-mach.c    | 21 +++------------------
>   5 files changed, 10 insertions(+), 71 deletions(-)
> 
> diff --git a/sound/soc/amd/acp/Kconfig b/sound/soc/amd/acp/Kconfig
> index d5838df3064b..154be5e70821 100644
> --- a/sound/soc/amd/acp/Kconfig
> +++ b/sound/soc/amd/acp/Kconfig
> @@ -32,7 +32,7 @@ config SND_AMD_ASOC_RENOIR
> 
>   config SND_SOC_AMD_MACH_COMMON
>          tristate
> -       depends on X86 && PCI && I2C && GPIOLIB
> +       depends on X86 && PCI && I2C
>          select CLK_FIXED_FCH
>          select SND_SOC_RT5682_I2C
>          select SND_SOC_DMIC
> @@ -44,14 +44,14 @@ config SND_SOC_AMD_MACH_COMMON
> 
>   config SND_SOC_AMD_LEGACY_MACH
>          tristate "AMD Legacy Machine Driver Support"
> -       depends on X86 && PCI && I2C && GPIOLIB
> +       depends on X86 && PCI && I2C
>          select SND_SOC_AMD_MACH_COMMON
>          help
>            This option enables legacy sound card support for ACP audio.
> 
>   config SND_SOC_AMD_SOF_MACH
>          tristate "AMD SOF Machine Driver Support"
> -       depends on X86 && PCI && I2C && GPIOLIB
> +       depends on X86 && PCI && I2C
>          select SND_SOC_AMD_MACH_COMMON
>          help
>            This option enables SOF sound card support for ACP audio.
> diff --git a/sound/soc/amd/acp/acp-legacy-mach.c b/sound/soc/amd/acp/acp-legacy-mach.c
> index 0ad1cf41b308..de0f8024e2fb 100644
> --- a/sound/soc/amd/acp/acp-legacy-mach.c
> +++ b/sound/soc/amd/acp/acp-legacy-mach.c
> @@ -27,7 +27,6 @@ static struct acp_card_drvdata rt5682_rt1019_data = {
>          .hs_codec_id = RT5682,
>          .amp_codec_id = RT1019,
>          .dmic_codec_id = NONE,
> -       .gpio_spkr_en = EN_SPKR_GPIO_GB,
>   };
> 
>   static const struct snd_kcontrol_new acp_controls[] = {
> @@ -42,16 +41,15 @@ static const struct snd_kcontrol_new acp_controls[] = {
>   static const struct snd_soc_dapm_widget acp_widgets[] = {
>          SND_SOC_DAPM_HP("Headphone Jack", NULL),
>          SND_SOC_DAPM_MIC("Headset Mic", NULL),
> -       SND_SOC_DAPM_SPK("Spk", event_spkr_handler),
> -       SND_SOC_DAPM_SPK("Left Spk", event_spkr_handler),
> -       SND_SOC_DAPM_SPK("Right Spk", event_spkr_handler),
> +       SND_SOC_DAPM_SPK("Spk", NULL),
> +       SND_SOC_DAPM_SPK("Left Spk", NULL),
> +       SND_SOC_DAPM_SPK("Right Spk", NULL),
>   };
> 
>   static int acp_asoc_probe(struct platform_device *pdev)
>   {
>          struct snd_soc_card *card = NULL;
>          struct device *dev = &pdev->dev;
> -       unsigned int spkr_gpio;
>          int ret;
> 
>          if (!pdev->id_entry)
> @@ -69,20 +67,9 @@ static int acp_asoc_probe(struct platform_device *pdev)
>          card->controls = acp_controls;
>          card->num_controls = ARRAY_SIZE(acp_controls);
>          card->drvdata = (struct acp_card_drvdata *)pdev->id_entry->driver_data;
> -       spkr_gpio = ((struct acp_card_drvdata *)(card->drvdata))->gpio_spkr_en;
> 
>          acp_legacy_dai_links_create(card);
> 
> -       if (gpio_is_valid(spkr_gpio)) {
> -               ret = devm_gpio_request(dev, spkr_gpio, "spkren");
> -               if (ret) {
> -                       dev_err(dev, "(%s) gpio request failed: %d\n",
> -                               __func__, ret);
> -                       return ret;
> -               }
> -               gpio_direction_output(spkr_gpio, 0);
> -       }
> -
>          ret = devm_snd_soc_register_card(&pdev->dev, card);
>          if (ret) {
>                  dev_err(&pdev->dev,
> diff --git a/sound/soc/amd/acp/acp-mach-common.c b/sound/soc/amd/acp/acp-mach-common.c
> index c9caade5cb74..7386e5bb61b5 100644
> --- a/sound/soc/amd/acp/acp-mach-common.c
> +++ b/sound/soc/amd/acp/acp-mach-common.c
> @@ -71,31 +71,6 @@ static const struct snd_soc_dapm_route rt5682_map[] = {
>          { "IN1P", NULL, "Headset Mic" },
>   };
> 
> -int event_spkr_handler(struct snd_soc_dapm_widget *w,
> -                       struct snd_kcontrol *k, int event)
> -{
> -       struct snd_soc_dapm_context *dapm = w->dapm;
> -       struct snd_soc_card *card = dapm->card;
> -       struct acp_card_drvdata *drvdata = snd_soc_card_get_drvdata(card);
> -
> -       if (!gpio_is_valid(drvdata->gpio_spkr_en))
> -               return 0;
> -
> -       switch (event) {
> -       case SND_SOC_DAPM_POST_PMU:
> -               gpio_set_value(drvdata->gpio_spkr_en, 1);
> -               break;
> -       case SND_SOC_DAPM_PRE_PMD:
> -               gpio_set_value(drvdata->gpio_spkr_en, 0);
> -               break;
> -       default:
> -               dev_warn(card->dev, "%s invalid setting\n", __func__);
> -               break;
> -       }
> -       return 0;
> -}
> -EXPORT_SYMBOL_NS_GPL(event_spkr_handler, SND_SOC_AMD_MACH);
> -
>   /* Define card ops for RT5682 CODEC */
>   static int acp_card_rt5682_init(struct snd_soc_pcm_runtime *rtd)
>   {
> diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h
> index fd6299844ebe..5dc47cfbff10 100644
> --- a/sound/soc/amd/acp/acp-mach.h
> +++ b/sound/soc/amd/acp/acp-mach.h
> @@ -17,12 +17,6 @@
>   #include <linux/input.h>
>   #include <linux/module.h>
>   #include <sound/soc.h>
> -#include <linux/gpio.h>
> -#include <linux/gpio/consumer.h>
> -
> -#define EN_SPKR_GPIO_GB                0x11F
> -#define EN_SPKR_GPIO_NK                0x146
> -#define EN_SPKR_GPIO_NONE      -EINVAL
> 
>   enum be_id {
>          HEADSET_BE_ID = 0,
> @@ -55,11 +49,9 @@ struct acp_card_drvdata {
>          unsigned int dai_fmt;
>          struct clk *wclk;
>          struct clk *bclk;
> -       unsigned int gpio_spkr_en;
>   };
> 
>   int acp_sofdsp_dai_links_create(struct snd_soc_card *card);
>   int acp_legacy_dai_links_create(struct snd_soc_card *card);
> -int event_spkr_handler(struct snd_soc_dapm_widget *w,
> -                       struct snd_kcontrol *k, int event);
> +
>   #endif
> diff --git a/sound/soc/amd/acp/acp-sof-mach.c b/sound/soc/amd/acp/acp-sof-mach.c
> index c1d9650fc222..dc2786cc81ef 100644
> --- a/sound/soc/amd/acp/acp-sof-mach.c
> +++ b/sound/soc/amd/acp/acp-sof-mach.c
> @@ -27,7 +27,6 @@ static struct acp_card_drvdata sof_rt5682_rt1019_data = {
>          .hs_codec_id = RT5682,
>          .amp_codec_id = RT1019,
>          .dmic_codec_id = DMIC,
> -       .gpio_spkr_en = EN_SPKR_GPIO_GB,
I guess we still need this for rt1019 codec case as rt1019 codec driver 
is i2c controlled so machine driver need to power off/on spk_en gpio.
>   };
> 
>   static struct acp_card_drvdata sof_rt5682_max_data = {
> @@ -37,7 +36,6 @@ static struct acp_card_drvdata sof_rt5682_max_data = {
>          .hs_codec_id = RT5682,
>          .amp_codec_id = MAX98360A,
>          .dmic_codec_id = DMIC,
> -       .gpio_spkr_en = EN_SPKR_GPIO_NK,
>   };
> 
>   static struct acp_card_drvdata sof_rt5682s_rt1019_data = {
> @@ -56,7 +54,6 @@ static struct acp_card_drvdata sof_rt5682s_max_data = {
>          .hs_codec_id = RT5682S,
>          .amp_codec_id = MAX98360A,
>          .dmic_codec_id = DMIC,
> -       .gpio_spkr_en = EN_SPKR_GPIO_NK,
>   };
> 
>   static const struct snd_kcontrol_new acp_controls[] = {
> @@ -70,16 +67,15 @@ static const struct snd_kcontrol_new acp_controls[] = {
>   static const struct snd_soc_dapm_widget acp_widgets[] = {
>          SND_SOC_DAPM_HP("Headphone Jack", NULL),
>          SND_SOC_DAPM_MIC("Headset Mic", NULL),
> -       SND_SOC_DAPM_SPK("Spk", event_spkr_handler),
> -       SND_SOC_DAPM_SPK("Left Spk", event_spkr_handler),
> -       SND_SOC_DAPM_SPK("Right Spk", event_spkr_handler),
> +       SND_SOC_DAPM_SPK("Spk", NULL),
> +       SND_SOC_DAPM_SPK("Left Spk", NULL),
> +       SND_SOC_DAPM_SPK("Right Spk", NULL),
>   };
> 
>   static int acp_sof_probe(struct platform_device *pdev)
>   {
>          struct snd_soc_card *card = NULL;
>          struct device *dev = &pdev->dev;
> -       unsigned int spkr_gpio;
>          int ret;
> 
>          if (!pdev->id_entry)
> @@ -97,20 +93,9 @@ static int acp_sof_probe(struct platform_device *pdev)
>          card->controls = acp_controls;
>          card->num_controls = ARRAY_SIZE(acp_controls);
>          card->drvdata = (struct acp_card_drvdata *)pdev->id_entry->driver_data;
> -       spkr_gpio = ((struct acp_card_drvdata *)(card->drvdata))->gpio_spkr_en;
> 
>          acp_sofdsp_dai_links_create(card);
> 
> -       if (gpio_is_valid(spkr_gpio)) {
> -               ret = devm_gpio_request(dev, spkr_gpio, "spkren");
> -               if (ret) {
> -                       dev_err(dev, "(%s) gpio request failed: %d\n",
> -                               __func__, ret);
> -                       return ret;
> -               }
> -               gpio_direction_output(spkr_gpio, 0);
> -       }
> -
>          ret = devm_snd_soc_register_card(&pdev->dev, card);
>          if (ret) {
>                  dev_err(&pdev->dev,
> --
> 2.32.0
> 
I feel instead of reverting this complete patch we can quickly submit a 
new patch set with "gpio_spk_en = NONE" for maxim codec case. As codec 
driver is anyhow controlling that gpio we don't need to do it from 
machine driver. We've already done that here 
https://patchwork.kernel.org/project/alsa-devel/patch/20220131203225.1418648-1-vsujithkumar.reddy@amd.com/

Re: [PATCH] Revert "ASoC: amd: acp: Power on/off the speaker enable gpio pin based on DAPM callback."

[Curtis Malainey]

> > --
> > 2.32.0
> >
> I feel instead of reverting this complete patch we can quickly submit a
> new patch set with "gpio_spk_en = NONE" for maxim codec case. As codec
> driver is anyhow controlling that gpio we don't need to do it from
> machine driver. We've already done that here
> https://patchwork.kernel.org/project/alsa-devel/patch/20220131203225.1418648-1-vsujithkumar.reddy@amd.com/

I'm pretty sure the proper solution is to shove this logic into the
alc1019 driver like it is in the max98357 driver. The header is
already there for gpio which makes me think it was planned but
forgotten. Otherwise everyone else who uses this codec and associated
pin will have to duplicate this logic in their machine driver.

Re: [PATCH] Revert "ASoC: amd: acp: Power on/off the speaker enable gpio pin based on DAPM callback."

[Mark Brown]

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

On Fri, Jan 28, 2022 at 04:08:26PM -0800, Curtis Malainey wrote:

> This reverts commit 5c5f08f7fc0bee9a1bc3fbdcb7a21cfd0648ab14.

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.

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

Re: [PATCH] Revert "ASoC: amd: acp: Power on/off the speaker enable gpio pin based on DAPM callback."

[Mark Brown]

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

On Mon, Jan 31, 2022 at 10:39:00AM -0800, Curtis Malainey wrote:

> > I feel instead of reverting this complete patch we can quickly submit a
> > new patch set with "gpio_spk_en = NONE" for maxim codec case. As codec
> > driver is anyhow controlling that gpio we don't need to do it from
> > machine driver. We've already done that here
> > https://patchwork.kernel.org/project/alsa-devel/patch/20220131203225.1418648-1-vsujithkumar.reddy@amd.com/

> I'm pretty sure the proper solution is to shove this logic into the
> alc1019 driver like it is in the max98357 driver. The header is
> already there for gpio which makes me think it was planned but
> forgotten. Otherwise everyone else who uses this codec and associated
> pin will have to duplicate this logic in their machine driver.

In general if there's something like a speaker amplifier that's just
controlled via GPIO I'd expect that to be something that's set up by the
machine driver.  If the CODEC is a GPIO provider then the pattern should
be that the CODEC registers with gpiolib and then the machine driver
uses that GPIO, that way the GPIO can get used for any other purpose and
if a system picks another GPIO for whatever reason then that'll just
work.

This gets more annoying for ACPI systems due to their lack of
standards based enumeration of course, the endemic reliance on board
specific quirks causes breakage all over - it looks like this is just an
example of some ACPI systems using firmware description and some not.
Are the systems that need the hard coding here shipping, for example if
the Windows drivers rely on such hard coding rather than enumeration
from ACPI?  If we need the quirking then the fix isn't just a simple
revert, we need to do something that ensures that the support for all
the different systems plays nicely with each other.

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

Re: [PATCH] Revert "ASoC: amd: acp: Power on/off the speaker enable gpio pin based on DAPM callback."

[Curtis Malainey]

On Tue, Feb 1, 2022 at 10:40 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jan 31, 2022 at 10:39:00AM -0800, Curtis Malainey wrote:
>
> > > I feel instead of reverting this complete patch we can quickly submit a
> > > new patch set with "gpio_spk_en = NONE" for maxim codec case. As codec
> > > driver is anyhow controlling that gpio we don't need to do it from
> > > machine driver. We've already done that here
> > > https://patchwork.kernel.org/project/alsa-devel/patch/20220131203225.1418648-1-vsujithkumar.reddy@amd.com/
>
> > I'm pretty sure the proper solution is to shove this logic into the
> > alc1019 driver like it is in the max98357 driver. The header is
> > already there for gpio which makes me think it was planned but
> > forgotten. Otherwise everyone else who uses this codec and associated
> > pin will have to duplicate this logic in their machine driver.
>
> In general if there's something like a speaker amplifier that's just
> controlled via GPIO I'd expect that to be something that's set up by the
> machine driver.  If the CODEC is a GPIO provider then the pattern should
> be that the CODEC registers with gpiolib and then the machine driver
> uses that GPIO, that way the GPIO can get used for any other purpose and
> if a system picks another GPIO for whatever reason then that'll just
> work.

Just to confirm, by provider, you mean it has on codec gpio it is
exposing to the kernel correct? Interestingly, this appears to be
counter to the max98357a.c driver. It has the SDMODE pin which can put
it into a low power state. The codec driver appears to consume this
pin from FW lookup and toggle it on trigger. I am just curious why we
would not want the codec to handle its own pins? That way the control
logic for the pin is collected into a non-chipset dependent location
that is close to the internal state of the driver.

>
> This gets more annoying for ACPI systems due to their lack of
> standards based enumeration of course, the endemic reliance on board
> specific quirks causes breakage all over - it looks like this is just an
> example of some ACPI systems using firmware description and some not.
> Are the systems that need the hard coding here shipping, for example if
> the Windows drivers rely on such hard coding rather than enumeration
> from ACPI?  If we need the quirking then the fix isn't just a simple
> revert, we need to do something that ensures that the support for all
> the different systems plays nicely with each other.

The board this patch fixes is not shipping, the board it breaks is
planned to ship from my understanding. Eric, feel free to correct me.
We could do a partial revert and remove the _NP fields and both boards
would work (Sujith already sent a patch for this "ASoC: amd: acp: Set
gpio_spkr_en to None for max speaker amplifer in machine driver") but
I think we should still consider a patch to stop hard coding the GPIO
as it should be available via a lookup.

Re: [PATCH] Revert "ASoC: amd: acp: Power on/off the speaker enable gpio pin based on DAPM callback."

[Mark Brown]

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

On Tue, Feb 01, 2022 at 11:07:45AM -0800, Curtis Malainey wrote:
> On Tue, Feb 1, 2022 at 10:40 AM Mark Brown <broonie@kernel.org> wrote:

> > In general if there's something like a speaker amplifier that's just
> > controlled via GPIO I'd expect that to be something that's set up by the
> > machine driver.  If the CODEC is a GPIO provider then the pattern should

> Just to confirm, by provider, you mean it has on codec gpio it is
> exposing to the kernel correct? Interestingly, this appears to be
> counter to the max98357a.c driver. It has the SDMODE pin which can put
> it into a low power state. The codec driver appears to consume this
> pin from FW lookup and toggle it on trigger. I am just curious why we
> would not want the codec to handle its own pins? That way the control
> logic for the pin is collected into a non-chipset dependent location
> that is close to the internal state of the driver.

Bear in mind that I'm just reading some e-mail about a machine driver
here, the most common case for a GPIO for an amplifier is something
where there's a very simple analogue amplifier with just a GPIO to
mute it that's been attached to a CODEC or if it's something that goes
into a device that's otherwise visible to software.  I don't have
immediate visibility of the full set of machines and CODECs here.  Like
I say the state of ACPI firmware description really doesn't help here.

> The board this patch fixes is not shipping, the board it breaks is
> planned to ship from my understanding. Eric, feel free to correct me.
> We could do a partial revert and remove the _NP fields and both boards
> would work (Sujith already sent a patch for this "ASoC: amd: acp: Set
> gpio_spkr_en to None for max speaker amplifer in machine driver") but
> I think we should still consider a patch to stop hard coding the GPIO
> as it should be available via a lookup.

Are there other systems that aren't Chromebooks being covered here, and
if so what state are they in?  In any case what I'd expect to see here
is a series transitioning the existing code to use lookups from the
firmware.

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