| Message ID | 20220129000837.6207-1-cujomalainey@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "ASoC: amd: acp: Power on/off the speaker enable gpio pin based on DAPM callback." | expand |
> > -- > > 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.
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.
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.
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.
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.
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,
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(-)