Message ID | 20201102023212.594137-1-CTLIN0@nuvoton.com |
---|---|
State | Superseded |
Headers | show |
Series | ASoC: nau8315: add codec driver | expand |
>>> +static int nau8315_enpin_event(struct snd_soc_dapm_widget *w, >>> + struct snd_kcontrol *kcontrol, int event) >>> +{ >>> + struct snd_soc_component *component = >>> + snd_soc_dapm_to_component(w->dapm); >>> + struct nau8315_priv *nau8315 = >>> + snd_soc_component_get_drvdata(component); >>> + >> >> [1] >> >>> + if (event & SND_SOC_DAPM_POST_PMU) >>> + nau8315->enpin_switch = 1; >>> + else if (event & SND_SOC_DAPM_POST_PMD) >>> + nau8315->enpin_switch = 0; >> >> And even if this variable was useful, for symmetry should it be >> PRE_PMU/POST_PMD? >> > Yes, I agree with your opinion. > > From the above description, I suppose you might want to point whether > the dapm widget of EN_PIN is redundant, right? That's a reason the > physical hardware pin is set to high/low in trigger function point of > snd_soc_dai_ops, it always occurred after dapm event. > If yes, from my current platform of verification, even if the dapm > widget of EN_PIN is removed, the result of sound output is same yet. > Maybe the first version, I should submit a simpler version. The model from the Max98357a seems to come from 128f825aeab79 (' ASoC: max98357a: move control of SD_MODE to DAPM') It doesn't seem like this additional EN_PIN is useful at the codec level but may help with machine integration. I still don't get the POST_PMU/POST_PMD. This was changed in 04a646ff5acaa by Tzung-Bi Shih @ Google, wondering if there is an explanation? Not pushing back, just trying to get what makes sense for amplifiers.
On Wed, Nov 4, 2020 at 10:51 PM Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: > The model from the Max98357a seems to come from 128f825aeab79 (' > ASoC: max98357a: move control of SD_MODE to DAPM') > > It doesn't seem like this additional EN_PIN is useful at the codec level > but may help with machine integration. We have a platform that max98357a shares the I2S with another codec. The software control here is for separating them. See the commit message of 128f825aeab79 ("ASoC: max98357a: move control of SD_MODE to DAPM") for more details. > I still don't get the POST_PMU/POST_PMD. This was changed in > 04a646ff5acaa by Tzung-Bi Shih @ Google, wondering if there is an > explanation? There was a report for max98357a pop noise on rk3399-gru-kevin due to commit 128f825aeab79. Commit 04a646ff5acaa ("ASoC: max98357a: move control of SD_MODE back to DAI ops") fixes it. See https://patchwork.kernel.org/project/alsa-devel/patch/20200721114232.2812254-1-tzungbi@google.com/#23502085. There is no reason for the asymmetric POST_PMU/POST_PMD here. You should find that the sdmode_switch doesn't take effect immediately in the DAPM event. I guess I was trying something (e.g. turning on/off when the stream is opening) but failed. And I probably forgot to recover the DAPM events back to symmetric (to avoid confusing people). If nau8315 doesn't share I2S with other components for now, it could be better to not introduce the software mute control.
On Thu, Nov 05, 2020 at 10:20:33AM +0800, Tzung-Bi Shih wrote: > If nau8315 doesn't share I2S with other components for now, it could > be better to not introduce the software mute control. The mute callback is there because there's some controllers that don't start up cleanly and end up outputing glitches (for example due to not being able to flush their FIFOs) - keeping the CODEC muted until after the I2S is running covers those glitches.
On 11/5/20 9:02 AM, Mark Brown wrote: > On Thu, Nov 05, 2020 at 10:20:33AM +0800, Tzung-Bi Shih wrote: > >> If nau8315 doesn't share I2S with other components for now, it could >> be better to not introduce the software mute control. > > The mute callback is there because there's some controllers that don't > start up cleanly and end up outputing glitches (for example due to not > being able to flush their FIFOs) - keeping the CODEC muted until after > the I2S is running covers those glitches. Thanks for explaining those dependencies. The code looks good to me: Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
On 2020/11/6 上午 12:21, Pierre-Louis Bossart wrote: > > > On 11/5/20 9:02 AM, Mark Brown wrote: >> On Thu, Nov 05, 2020 at 10:20:33AM +0800, Tzung-Bi Shih wrote: >> >>> If nau8315 doesn't share I2S with other components for now, it could >>> be better to not introduce the software mute control. >> >> The mute callback is there because there's some controllers that don't >> start up cleanly and end up outputing glitches (for example due to not >> being able to flush their FIFOs) - keeping the CODEC muted until after >> the I2S is running covers those glitches. > > Thanks for explaining those dependencies. The code looks good to me: > > Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Thanks for your comments. However, I also agree the opinion from Tzung-Bi Shih @ Google. May I confirm whether I should keep this patch, and remove dapm widget of EN_PIN next patch?
On Fri, Nov 06, 2020 at 11:31:36AM +0800, CTLIN0 wrote: > Thanks for your comments. However, I also agree the opinion from > Tzung-Bi Shih @ Google. > May I confirm whether I should keep this patch, and remove dapm widget > of EN_PIN next patch? Please send incremental patches on top of this one with any changes.
On 11/6/20 6:20 AM, Mark Brown wrote: > On Fri, Nov 06, 2020 at 11:31:36AM +0800, CTLIN0 wrote: > >> Thanks for your comments. However, I also agree the opinion from >> Tzung-Bi Shih @ Google. >> May I confirm whether I should keep this patch, and remove dapm widget >> of EN_PIN next patch? > > Please send incremental patches on top of this one with any changes. EN_PIN is not strictly required but as discussed it provides additional flexibility for machine drivers, so probably better to keep it?
diff --git a/Documentation/devicetree/bindings/sound/nau8315.txt b/Documentation/devicetree/bindings/sound/nau8315.txt new file mode 100644 index 000000000000..6eaec46f384c --- /dev/null +++ b/Documentation/devicetree/bindings/sound/nau8315.txt @@ -0,0 +1,18 @@ +Nuvoton NAU8315 Mono Class-D Amplifier + +Required properties: +- compatible : "nuvoton,nau8315" + +Optional properties: +- enable-gpios : GPIO specifier for the chip's device enable input(EN) pin. + If this option is not specified then driver does not manage + the pin state (e.g. chip is always on). + +Example: + +#include <dt-bindings/gpio/gpio.h> + +nau8315 { + compatible = "nuvoton,nau8315"; + enable-gpios = <&gpio1 5 GPIO_ACTIVE_HIGH>; +}; diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 5791b7056af6..2b6eceb3573c 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -130,6 +130,7 @@ config SND_SOC_ALL_CODECS imply SND_SOC_MT6358 imply SND_SOC_MT6359 imply SND_SOC_MT6660 + imply SND_SOC_NAU8315 imply SND_SOC_NAU8540 imply SND_SOC_NAU8810 imply SND_SOC_NAU8822 @@ -1760,6 +1761,10 @@ config SND_SOC_MT6660 Select N if you don't have MT6660 on board. Select M to build this as module. +config SND_SOC_NAU8315 + tristate "Nuvoton Technology Corporation NAU8315 CODEC" + depends on GPIOLIB + config SND_SOC_NAU8540 tristate "Nuvoton Technology Corporation NAU85L40 CODEC" depends on I2C diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index 11ce98c25d6c..144d9256d4d5 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -129,6 +129,7 @@ snd-soc-mt6351-objs := mt6351.o snd-soc-mt6358-objs := mt6358.o snd-soc-mt6359-objs := mt6359.o snd-soc-mt6660-objs := mt6660.o +snd-soc-nau8315-objs := nau8315.o snd-soc-nau8540-objs := nau8540.o snd-soc-nau8810-objs := nau8810.o snd-soc-nau8822-objs := nau8822.o @@ -438,6 +439,7 @@ obj-$(CONFIG_SND_SOC_MT6351) += snd-soc-mt6351.o obj-$(CONFIG_SND_SOC_MT6358) += snd-soc-mt6358.o obj-$(CONFIG_SND_SOC_MT6359) += snd-soc-mt6359.o obj-$(CONFIG_SND_SOC_MT6660) += snd-soc-mt6660.o +obj-$(CONFIG_SND_SOC_NAU8315) += snd-soc-nau8315.o obj-$(CONFIG_SND_SOC_NAU8540) += snd-soc-nau8540.o obj-$(CONFIG_SND_SOC_NAU8810) += snd-soc-nau8810.o obj-$(CONFIG_SND_SOC_NAU8822) += snd-soc-nau8822.o diff --git a/sound/soc/codecs/nau8315.c b/sound/soc/codecs/nau8315.c new file mode 100644 index 000000000000..e6bc5c0a5036 --- /dev/null +++ b/sound/soc/codecs/nau8315.c @@ -0,0 +1,166 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// nau8315.c -- NAU8315 ALSA SoC Audio Amplifier Driver +// +// Copyright 2020 Nuvoton Technology Crop. +// +// Author: David Lin <ctlin0@nuvoton.com> +// +// Based on MAX98357A.c + +#include <linux/acpi.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/gpio.h> +#include <linux/gpio/consumer.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <sound/pcm.h> +#include <sound/soc.h> +#include <sound/soc-dai.h> +#include <sound/soc-dapm.h> + +struct nau8315_priv { + struct gpio_desc *enable; + int enpin_switch; +}; + +static int nau8315_daiops_trigger(struct snd_pcm_substream *substream, + int cmd, struct snd_soc_dai *dai) +{ + struct snd_soc_component *component = dai->component; + struct nau8315_priv *nau8315 = + snd_soc_component_get_drvdata(component); + + if (!nau8315->enable) + return 0; + + switch (cmd) { + case SNDRV_PCM_TRIGGER_START: + case SNDRV_PCM_TRIGGER_RESUME: + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: + if (nau8315->enpin_switch) { + gpiod_set_value(nau8315->enable, 1); + dev_dbg(component->dev, "set enable to 1"); + } + break; + case SNDRV_PCM_TRIGGER_STOP: + case SNDRV_PCM_TRIGGER_SUSPEND: + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: + gpiod_set_value(nau8315->enable, 0); + dev_dbg(component->dev, "set enable to 0"); + break; + } + + return 0; +} + +static int nau8315_enpin_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *kcontrol, int event) +{ + struct snd_soc_component *component = + snd_soc_dapm_to_component(w->dapm); + struct nau8315_priv *nau8315 = + snd_soc_component_get_drvdata(component); + + if (event & SND_SOC_DAPM_POST_PMU) + nau8315->enpin_switch = 1; + else if (event & SND_SOC_DAPM_POST_PMD) + nau8315->enpin_switch = 0; + + return 0; +} + +static const struct snd_soc_dapm_widget nau8315_dapm_widgets[] = { + SND_SOC_DAPM_OUTPUT("Speaker"), + SND_SOC_DAPM_OUT_DRV_E("EN_Pin", SND_SOC_NOPM, 0, 0, NULL, 0, + nau8315_enpin_event, + SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD), +}; + +static const struct snd_soc_dapm_route nau8315_dapm_routes[] = { + {"EN_Pin", NULL, "HiFi Playback"}, + {"Speaker", NULL, "EN_Pin"}, +}; + +static const struct snd_soc_component_driver nau8315_component_driver = { + .dapm_widgets = nau8315_dapm_widgets, + .num_dapm_widgets = ARRAY_SIZE(nau8315_dapm_widgets), + .dapm_routes = nau8315_dapm_routes, + .num_dapm_routes = ARRAY_SIZE(nau8315_dapm_routes), + .idle_bias_on = 1, + .use_pmdown_time = 1, + .endianness = 1, + .non_legacy_dai_naming = 1, +}; + +static const struct snd_soc_dai_ops nau8315_dai_ops = { + .trigger = nau8315_daiops_trigger, +}; + +#define NAU8315_RATES SNDRV_PCM_RATE_8000_96000 +#define NAU8315_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_3LE) + +static struct snd_soc_dai_driver nau8315_dai_driver = { + .name = "nau8315-hifi", + .playback = { + .stream_name = "HiFi Playback", + .formats = NAU8315_FORMATS, + .rates = NAU8315_RATES, + .channels_min = 1, + .channels_max = 2, + }, + .ops = &nau8315_dai_ops, +}; + +static int nau8315_platform_probe(struct platform_device *pdev) +{ + struct nau8315_priv *nau8315; + + nau8315 = devm_kzalloc(&pdev->dev, sizeof(*nau8315), GFP_KERNEL); + if (!nau8315) + return -ENOMEM; + + nau8315->enable = devm_gpiod_get_optional(&pdev->dev, + "enable", GPIOD_OUT_LOW); + if (IS_ERR(nau8315->enable)) + return PTR_ERR(nau8315->enable); + + dev_set_drvdata(&pdev->dev, nau8315); + + return devm_snd_soc_register_component(&pdev->dev, + &nau8315_component_driver, + &nau8315_dai_driver, 1); +} + +#ifdef CONFIG_OF +static const struct of_device_id nau8315_device_id[] = { + { .compatible = "nuvoton,nau8315" }, + {} +}; +MODULE_DEVICE_TABLE(of, nau8315_device_id); +#endif + +#ifdef CONFIG_ACPI +static const struct acpi_device_id nau8315_acpi_match[] = { + { "NVTN2010", 0 }, + {}, +}; +MODULE_DEVICE_TABLE(acpi, nau8315_acpi_match); +#endif + +static struct platform_driver nau8315_platform_driver = { + .driver = { + .name = "nau8315", + .of_match_table = of_match_ptr(nau8315_device_id), + .acpi_match_table = ACPI_PTR(nau8315_acpi_match), + }, + .probe = nau8315_platform_probe, +}; +module_platform_driver(nau8315_platform_driver); + +MODULE_DESCRIPTION("ASoC NAU8315 Mono Class-D Amplifier Driver"); +MODULE_AUTHOR("David Lin <ctlin0@nuvoton.com>"); +MODULE_LICENSE("GPL v2");
Add codec driver for Nuvoton NAU8315. Signed-off-by: David Lin <CTLIN0@nuvoton.com> --- .../devicetree/bindings/sound/nau8315.txt | 18 ++ sound/soc/codecs/Kconfig | 5 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/nau8315.c | 166 ++++++++++++++++++ 4 files changed, 191 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/nau8315.txt create mode 100644 sound/soc/codecs/nau8315.c