[alsa-devel] [PATCH] ASoC: Tegra: Add support of tegra boards based on ALC5632 codec
At this stage only Toshiba AC100/Dynabook supported.
Signed-off-by: Leon Romanovsky leon@leon.nu Signed-off-by: Andrey Danin danindrey@mail.ru --- sound/soc/tegra/Kconfig | 9 ++ sound/soc/tegra/Makefile | 2 + sound/soc/tegra/tegra_alc5632.c | 298 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 309 insertions(+), 0 deletions(-) create mode 100644 sound/soc/tegra/tegra_alc5632.c
diff --git a/sound/soc/tegra/Kconfig b/sound/soc/tegra/Kconfig index c6af1fd..f8fcda3 100644 --- a/sound/soc/tegra/Kconfig +++ b/sound/soc/tegra/Kconfig @@ -47,3 +47,12 @@ config SND_SOC_TEGRA_TRIMSLICE help Say Y or M here if you want to add support for SoC audio on the TrimSlice platform. + +config SND_SOC_TEGRA_ALC5632 + tristate "SoC Audio support for Tegra boards using an ALC5632 codec" + depends on SND_SOC_TEGRA && I2C + select SND_SOC_TEGRA_I2S + select SND_SOC_ALC5632 + help + Say Y or M here if you want to add support for SoC audio on the + Toshiba AC100 netbook. diff --git a/sound/soc/tegra/Makefile b/sound/soc/tegra/Makefile index 4d943b3..8e584b8 100644 --- a/sound/soc/tegra/Makefile +++ b/sound/soc/tegra/Makefile @@ -14,6 +14,8 @@ obj-$(CONFIG_SND_SOC_TEGRA_SPDIF) += snd-soc-tegra-spdif.o # Tegra machine Support snd-soc-tegra-wm8903-objs := tegra_wm8903.o snd-soc-tegra-trimslice-objs := trimslice.o +snd-soc-tegra-alc5632-objs := tegra_alc5632.o
obj-$(CONFIG_SND_SOC_TEGRA_WM8903) += snd-soc-tegra-wm8903.o obj-$(CONFIG_SND_SOC_TEGRA_TRIMSLICE) += snd-soc-tegra-trimslice.o +obj-$(CONFIG_SND_SOC_TEGRA_ALC5632) += snd-soc-tegra-alc5632.o diff --git a/sound/soc/tegra/tegra_alc5632.c b/sound/soc/tegra/tegra_alc5632.c new file mode 100644 index 0000000..b8828c1 --- /dev/null +++ b/sound/soc/tegra/tegra_alc5632.c @@ -0,0 +1,298 @@ +/* +* tegra_alc5632.c -- Toshiba AC100(PAZ00) machine ASoC driver +* +* Copyright (C) 2011 The AC100 Kernel Team ac100@lists.lauchpad.net +* +* Authors: Leon Romanovsky leon@leon.nu +* Andrey Danin danindrey@mail.ru +* Marc Dietrich marvin24@gmx.de +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +*/ + +#include <asm/mach-types.h> + +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/gpio.h> + +#include <sound/core.h> +#include <sound/jack.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> + +#include "../codecs/alc5632.h" + +#include "tegra_das.h" +#include "tegra_i2s.h" +#include "tegra_pcm.h" +#include "tegra_asoc_utils.h" + +#define DRV_NAME "tegra-alc5632" + +struct tegra_alc5632 { + struct tegra_asoc_utils_data util_data; +}; + +static int tegra_alc5632_asoc_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *codec_dai = rtd->codec_dai; + struct snd_soc_dai *cpu_dai = rtd->cpu_dai; + struct snd_soc_codec *codec = rtd->codec; + struct snd_soc_card *card = codec->card; + struct tegra_alc5632 *alc5632 = snd_soc_card_get_drvdata(card); + int srate, mclk; + int err; + + srate = params_rate(params); + switch (srate) { + case 64000: + case 88200: + case 96000: + mclk = 128 * srate; + break; + default: + mclk = 512 * srate; + break; + } + /* FIXME: Codec only requires >= 3MHz if OSR==0 */ + while (mclk < 6000000) + mclk *= 2; + + err = tegra_asoc_utils_set_rate(&alc5632->util_data, srate, mclk); + if (err < 0) { + dev_err(card->dev, "Can't configure clocks\n"); + return err; + } + + err = snd_soc_dai_set_fmt(codec_dai, + SND_SOC_DAIFMT_I2S | + SND_SOC_DAIFMT_NB_NF | + SND_SOC_DAIFMT_CBS_CFS); + if (err < 0) { + dev_err(card->dev, "codec_dai fmt not set\n"); + return err; + } + + err = snd_soc_dai_set_fmt(cpu_dai, + SND_SOC_DAIFMT_I2S | + SND_SOC_DAIFMT_NB_NF | + SND_SOC_DAIFMT_CBS_CFS); + if (err < 0) { + dev_err(card->dev, "cpu_dai fmt not set\n"); + return err; + } + + err = snd_soc_dai_set_sysclk(codec_dai, 0, mclk, + SND_SOC_CLOCK_IN); + if (err < 0) { + dev_err(card->dev, "codec_dai clock not set\n"); + return err; + } + + return 0; +} + +static struct snd_soc_ops tegra_alc5632_asoc_ops = { + .hw_params = tegra_alc5632_asoc_hw_params, +}; + +static struct snd_soc_jack tegra_alc5632_hs_jack; + +static struct snd_soc_jack_pin tegra_alc5632_hs_jack_pins[] = { + { + .pin = "Headset Mic", + .mask = SND_JACK_MICROPHONE, + }, + { + .pin = "Headset Stereophone", + .mask = SND_JACK_HEADPHONE, + }, +}; + +static struct snd_soc_jack_gpio tegra_alc5632_hs_jack_gpios[] = { + { + .name = "Headset Detect", + .report = SND_JACK_HEADSET, + .debounce_time = 150, + } +}; + +static const struct snd_soc_dapm_widget tegra_alc5632_dapm_widgets[] = { + SND_SOC_DAPM_SPK("Int Spk", NULL), + SND_SOC_DAPM_MIC("Int Mic", NULL), + SND_SOC_DAPM_HP("Headset Stereophone", NULL), + SND_SOC_DAPM_MIC("Headset Mic", NULL), +}; + +static const struct snd_soc_dapm_route tegra_alc5632_audio_map[] = { + /* Internal Mic */ + {"MIC2", NULL, "Mic Bias 2"}, + {"Mic Bias 2", NULL, "Int Mic"}, + + /* Internal Speaker */ + {"Int Spk", NULL, "SPKOUT"}, + {"Int Spk", NULL, "SPKOUTN"}, + + /* Headset Mic */ + {"MIC1", NULL, "Mic Bias 1"}, + {"Mic Bias 1", NULL, "Headset Mic"}, + + /* Headset Stereophone */ + {"Headset Stereophone", NULL, "HPR"}, + {"Headset Stereophone", NULL, "HPL"}, +}; + +static const struct snd_kcontrol_new tegra_alc5632_controls[] = { + SOC_DAPM_PIN_SWITCH("Int Spk"), +}; + +static int tegra_alc5632_asoc_init(struct snd_soc_pcm_runtime *rtd) +{ + struct snd_soc_codec *codec = rtd->codec; + struct snd_soc_dapm_context *dapm = &codec->dapm; + int ret; + + ret = snd_soc_add_controls(codec, tegra_alc5632_controls, + ARRAY_SIZE(tegra_alc5632_controls)); + if (ret < 0) + return ret; + + snd_soc_dapm_new_controls(dapm, tegra_alc5632_dapm_widgets, + ARRAY_SIZE(tegra_alc5632_dapm_widgets)); + + snd_soc_dapm_add_routes(dapm, tegra_alc5632_audio_map, + ARRAY_SIZE(tegra_alc5632_audio_map)); + + snd_soc_jack_new(codec, "Headset Jack", SND_JACK_HEADSET, + &tegra_alc5632_hs_jack); + snd_soc_jack_add_pins(&tegra_alc5632_hs_jack, + ARRAY_SIZE(tegra_alc5632_hs_jack_pins), + tegra_alc5632_hs_jack_pins); + snd_soc_jack_add_gpios(&tegra_alc5632_hs_jack, + ARRAY_SIZE(tegra_alc5632_hs_jack_gpios), + tegra_alc5632_hs_jack_gpios); + + snd_soc_dapm_nc_pin(dapm, "AUXOUT"); + snd_soc_dapm_nc_pin(dapm, "LINEINL"); + snd_soc_dapm_nc_pin(dapm, "LINEINR"); + snd_soc_dapm_nc_pin(dapm, "PHONEP"); + snd_soc_dapm_nc_pin(dapm, "PHONEN"); + snd_soc_dapm_nc_pin(dapm, "MIC2"); + + snd_soc_dapm_sync(dapm); + + return 0; +} + +static struct snd_soc_dai_link tegra_alc5632_dai = { + .name = "ALC5632", + .stream_name = "ALC5632 PCM", + .codec_name = "alc5632.0-001e", + .platform_name = "tegra-pcm-audio", + .cpu_dai_name = "tegra-i2s.0", + .codec_dai_name = "alc5632-hifi", + .init = tegra_alc5632_asoc_init, + .ops = &tegra_alc5632_asoc_ops, +}; + +static struct snd_soc_card snd_soc_tegra_alc5632 = { + .name = "tegra-alc5632", + .dai_link = &tegra_alc5632_dai, + .num_links = 1, +}; + +static __devinit int tegra_alc5632_probe(struct platform_device *pdev) +{ + struct snd_soc_card *card = &snd_soc_tegra_alc5632; + struct tegra_alc5632 *alc5632; + int ret; + + if (!machine_is_paz00()) { + dev_err(&pdev->dev, "Not running on Toshiba AC100!\n"); + return -ENODEV; + } + + alc5632 = kzalloc(sizeof(struct tegra_alc5632), GFP_KERNEL); + if (!alc5632) { + dev_err(&pdev->dev, "Can't allocate tegra_alc5632\n"); + return -ENOMEM; + } + + ret = tegra_asoc_utils_init(&alc5632->util_data, &pdev->dev); + if (ret) + goto err_free_tegra_alc5632; + + card->dev = &pdev->dev; + platform_set_drvdata(pdev, card); + snd_soc_card_set_drvdata(card, alc5632); + + ret = snd_soc_register_card(card); + if (ret) { + dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", + ret); + goto err_clear_drvdata; + } + + return 0; + +err_clear_drvdata: + snd_soc_card_set_drvdata(card, NULL); + platform_set_drvdata(pdev, NULL); + card->dev = NULL; + tegra_asoc_utils_fini(&alc5632->util_data); +err_free_tegra_alc5632: + kfree(alc5632); + return ret; +} + +static int __devexit tegra_alc5632_remove(struct platform_device *pdev) +{ + struct snd_soc_card *card = platform_get_drvdata(pdev); + struct tegra_alc5632 *alc5632 = snd_soc_card_get_drvdata(card); + + snd_soc_unregister_card(card); + + snd_soc_card_set_drvdata(card, NULL); + platform_set_drvdata(pdev, NULL); + card->dev = NULL; + + tegra_asoc_utils_fini(&alc5632->util_data); + + kfree(alc5632); + + return 0; +} + +static struct platform_driver tegra_alc5632_driver = { + .driver = { + .name = DRV_NAME, + .owner = THIS_MODULE, + .pm = &snd_soc_pm_ops, + }, + .probe = tegra_alc5632_probe, + .remove = __devexit_p(tegra_alc5632_remove), +}; + +static int __init tegra_alc5632_init(void) +{ + return platform_driver_register(&tegra_alc5632_driver); +} +module_init(tegra_alc5632_init); + +static void __exit tegra_alc5632_exit(void) +{ + platform_driver_unregister(&tegra_alc5632_driver); +} +module_exit(tegra_alc5632_exit); + +MODULE_AUTHOR("Leon Romanovsky leon@leon.nu"); +MODULE_DESCRIPTION("Tegra+ALC5632 machine ASoC driver"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:" DRV_NAME);
added Cc: linux-tegra, Stephen Warren, Olof Johansson
Hi Leon,
I still don't see how the hp gpio detect comes in. Can you explain this?
Marc
Am Montag, 21. November 2011, 22:08:08 schrieb Leon Romanovsky:
At this stage only Toshiba AC100/Dynabook supported.
Signed-off-by: Leon Romanovsky leon@leon.nu Signed-off-by: Andrey Danin danindrey@mail.ru
sound/soc/tegra/Kconfig | 9 ++ sound/soc/tegra/Makefile | 2 + sound/soc/tegra/tegra_alc5632.c | 298 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 309 insertions(+), 0 deletions(-) create mode 100644 sound/soc/tegra/tegra_alc5632.c
diff --git a/sound/soc/tegra/Kconfig b/sound/soc/tegra/Kconfig index c6af1fd..f8fcda3 100644 --- a/sound/soc/tegra/Kconfig +++ b/sound/soc/tegra/Kconfig @@ -47,3 +47,12 @@ config SND_SOC_TEGRA_TRIMSLICE help Say Y or M here if you want to add support for SoC audio on the TrimSlice platform.
+config SND_SOC_TEGRA_ALC5632
- tristate "SoC Audio support for Tegra boards using an ALC5632 codec"
- depends on SND_SOC_TEGRA && I2C
- select SND_SOC_TEGRA_I2S
- select SND_SOC_ALC5632
- help
Say Y or M here if you want to add support for SoC audio on theToshiba AC100 netbook.diff --git a/sound/soc/tegra/Makefile b/sound/soc/tegra/Makefile index 4d943b3..8e584b8 100644 --- a/sound/soc/tegra/Makefile +++ b/sound/soc/tegra/Makefile @@ -14,6 +14,8 @@ obj-$(CONFIG_SND_SOC_TEGRA_SPDIF) += snd-soc-tegra-spdif.o # Tegra machine Support snd-soc-tegra-wm8903-objs := tegra_wm8903.o snd-soc-tegra-trimslice-objs := trimslice.o +snd-soc-tegra-alc5632-objs := tegra_alc5632.o
obj-$(CONFIG_SND_SOC_TEGRA_WM8903) += snd-soc-tegra-wm8903.o obj-$(CONFIG_SND_SOC_TEGRA_TRIMSLICE) += snd-soc-tegra-trimslice.o +obj-$(CONFIG_SND_SOC_TEGRA_ALC5632) += snd-soc-tegra-alc5632.o diff --git a/sound/soc/tegra/tegra_alc5632.c b/sound/soc/tegra/tegra_alc5632.c new file mode 100644 index 0000000..b8828c1 --- /dev/null +++ b/sound/soc/tegra/tegra_alc5632.c @@ -0,0 +1,298 @@ +/* +* tegra_alc5632.c -- Toshiba AC100(PAZ00) machine ASoC driver
^^^ you may replace this with "Tegra machine ASoC driver for boards using ALC5332 codec"
+* +* Copyright (C) 2011 The AC100 Kernel Team ac100@lists.lauchpad.net +* +* Authors: Leon Romanovsky leon@leon.nu +* Andrey Danin danindrey@mail.ru +* Marc Dietrich marvin24@gmx.de +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +*/
+#include <asm/mach-types.h>
+#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/gpio.h>
+#include <sound/core.h> +#include <sound/jack.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h>
+#include "../codecs/alc5632.h"
+#include "tegra_das.h" +#include "tegra_i2s.h" +#include "tegra_pcm.h" +#include "tegra_asoc_utils.h"
+#define DRV_NAME "tegra-alc5632"
+struct tegra_alc5632 {
- struct tegra_asoc_utils_data util_data;
+};
+static int tegra_alc5632_asoc_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)+{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct snd_soc_dai *codec_dai = rtd->codec_dai;
- struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
- struct snd_soc_codec *codec = rtd->codec;
- struct snd_soc_card *card = codec->card;
- struct tegra_alc5632 *alc5632 = snd_soc_card_get_drvdata(card);
- int srate, mclk;
- int err;
- srate = params_rate(params);
- switch (srate) {
- case 64000:
- case 88200:
- case 96000:
mclk = 128 * srate;break;- default:
mclk = 512 * srate;break;- }
- /* FIXME: Codec only requires >= 3MHz if OSR==0 */
- while (mclk < 6000000)
mclk *= 2;- err = tegra_asoc_utils_set_rate(&alc5632->util_data, srate, mclk);
- if (err < 0) {
dev_err(card->dev, "Can't configure clocks\n");return err;- }
- err = snd_soc_dai_set_fmt(codec_dai,
SND_SOC_DAIFMT_I2S |SND_SOC_DAIFMT_NB_NF |SND_SOC_DAIFMT_CBS_CFS);- if (err < 0) {
dev_err(card->dev, "codec_dai fmt not set\n");return err;- }
- err = snd_soc_dai_set_fmt(cpu_dai,
SND_SOC_DAIFMT_I2S |SND_SOC_DAIFMT_NB_NF |SND_SOC_DAIFMT_CBS_CFS);- if (err < 0) {
dev_err(card->dev, "cpu_dai fmt not set\n");return err;- }
- err = snd_soc_dai_set_sysclk(codec_dai, 0, mclk,
SND_SOC_CLOCK_IN);- if (err < 0) {
dev_err(card->dev, "codec_dai clock not set\n");return err;- }
- return 0;
+}
+static struct snd_soc_ops tegra_alc5632_asoc_ops = {
- .hw_params = tegra_alc5632_asoc_hw_params,
+};
+static struct snd_soc_jack tegra_alc5632_hs_jack;
+static struct snd_soc_jack_pin tegra_alc5632_hs_jack_pins[] = {
- {
.pin = "Headset Mic",.mask = SND_JACK_MICROPHONE,- },
- {
.pin = "Headset Stereophone",.mask = SND_JACK_HEADPHONE,- },
+};
+static struct snd_soc_jack_gpio tegra_alc5632_hs_jack_gpios[] = {
- {
.name = "Headset Detect",.report = SND_JACK_HEADSET,.debounce_time = 150,- }
+};
+static const struct snd_soc_dapm_widget tegra_alc5632_dapm_widgets[] = {
- SND_SOC_DAPM_SPK("Int Spk", NULL),
- SND_SOC_DAPM_MIC("Int Mic", NULL),
- SND_SOC_DAPM_HP("Headset Stereophone", NULL),
- SND_SOC_DAPM_MIC("Headset Mic", NULL),
+};
+static const struct snd_soc_dapm_route tegra_alc5632_audio_map[] = {
- /* Internal Mic */
- {"MIC2", NULL, "Mic Bias 2"},
- {"Mic Bias 2", NULL, "Int Mic"},
- /* Internal Speaker */
- {"Int Spk", NULL, "SPKOUT"},
- {"Int Spk", NULL, "SPKOUTN"},
- /* Headset Mic */
- {"MIC1", NULL, "Mic Bias 1"},
- {"Mic Bias 1", NULL, "Headset Mic"},
- /* Headset Stereophone */
- {"Headset Stereophone", NULL, "HPR"},
- {"Headset Stereophone", NULL, "HPL"},
+};
+static const struct snd_kcontrol_new tegra_alc5632_controls[] = {
- SOC_DAPM_PIN_SWITCH("Int Spk"),
+};
+static int tegra_alc5632_asoc_init(struct snd_soc_pcm_runtime *rtd) +{
- struct snd_soc_codec *codec = rtd->codec;
- struct snd_soc_dapm_context *dapm = &codec->dapm;
- int ret;
- ret = snd_soc_add_controls(codec, tegra_alc5632_controls,
ARRAY_SIZE(tegra_alc5632_controls));- if (ret < 0)
return ret;- snd_soc_dapm_new_controls(dapm, tegra_alc5632_dapm_widgets,
ARRAY_SIZE(tegra_alc5632_dapm_widgets));- snd_soc_dapm_add_routes(dapm, tegra_alc5632_audio_map,
ARRAY_SIZE(tegra_alc5632_audio_map));- snd_soc_jack_new(codec, "Headset Jack", SND_JACK_HEADSET,
&tegra_alc5632_hs_jack);- snd_soc_jack_add_pins(&tegra_alc5632_hs_jack,
ARRAY_SIZE(tegra_alc5632_hs_jack_pins),tegra_alc5632_hs_jack_pins);- snd_soc_jack_add_gpios(&tegra_alc5632_hs_jack,
ARRAY_SIZE(tegra_alc5632_hs_jack_gpios),tegra_alc5632_hs_jack_gpios);- snd_soc_dapm_nc_pin(dapm, "AUXOUT");
- snd_soc_dapm_nc_pin(dapm, "LINEINL");
- snd_soc_dapm_nc_pin(dapm, "LINEINR");
- snd_soc_dapm_nc_pin(dapm, "PHONEP");
- snd_soc_dapm_nc_pin(dapm, "PHONEN");
- snd_soc_dapm_nc_pin(dapm, "MIC2");
- snd_soc_dapm_sync(dapm);
- return 0;
+}
+static struct snd_soc_dai_link tegra_alc5632_dai = {
- .name = "ALC5632",
- .stream_name = "ALC5632 PCM",
- .codec_name = "alc5632.0-001e",
- .platform_name = "tegra-pcm-audio",
- .cpu_dai_name = "tegra-i2s.0",
- .codec_dai_name = "alc5632-hifi",
- .init = tegra_alc5632_asoc_init,
- .ops = &tegra_alc5632_asoc_ops,
+};
+static struct snd_soc_card snd_soc_tegra_alc5632 = {
- .name = "tegra-alc5632",
- .dai_link = &tegra_alc5632_dai,
- .num_links = 1,
+};
+static __devinit int tegra_alc5632_probe(struct platform_device *pdev) +{
- struct snd_soc_card *card = &snd_soc_tegra_alc5632;
- struct tegra_alc5632 *alc5632;
- int ret;
- if (!machine_is_paz00()) {
dev_err(&pdev->dev, "Not running on Toshiba AC100!\n");return -ENODEV;- }
- alc5632 = kzalloc(sizeof(struct tegra_alc5632), GFP_KERNEL);
- if (!alc5632) {
dev_err(&pdev->dev, "Can't allocate tegra_alc5632\n");return -ENOMEM;- }
- ret = tegra_asoc_utils_init(&alc5632->util_data, &pdev->dev);
- if (ret)
goto err_free_tegra_alc5632;- card->dev = &pdev->dev;
- platform_set_drvdata(pdev, card);
- snd_soc_card_set_drvdata(card, alc5632);
- ret = snd_soc_register_card(card);
- if (ret) {
dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n",ret);goto err_clear_drvdata;- }
- return 0;
+err_clear_drvdata:
- snd_soc_card_set_drvdata(card, NULL);
- platform_set_drvdata(pdev, NULL);
- card->dev = NULL;
- tegra_asoc_utils_fini(&alc5632->util_data);
+err_free_tegra_alc5632:
- kfree(alc5632);
- return ret;
+}
+static int __devexit tegra_alc5632_remove(struct platform_device *pdev) +{
- struct snd_soc_card *card = platform_get_drvdata(pdev);
- struct tegra_alc5632 *alc5632 = snd_soc_card_get_drvdata(card);
- snd_soc_unregister_card(card);
- snd_soc_card_set_drvdata(card, NULL);
- platform_set_drvdata(pdev, NULL);
- card->dev = NULL;
- tegra_asoc_utils_fini(&alc5632->util_data);
- kfree(alc5632);
- return 0;
+}
+static struct platform_driver tegra_alc5632_driver = {
- .driver = {
.name = DRV_NAME,.owner = THIS_MODULE,.pm = &snd_soc_pm_ops,- },
- .probe = tegra_alc5632_probe,
- .remove = __devexit_p(tegra_alc5632_remove),
+};
+static int __init tegra_alc5632_init(void) +{
- return platform_driver_register(&tegra_alc5632_driver);
+} +module_init(tegra_alc5632_init);
+static void __exit tegra_alc5632_exit(void) +{
- platform_driver_unregister(&tegra_alc5632_driver);
+} +module_exit(tegra_alc5632_exit);
+MODULE_AUTHOR("Leon Romanovsky leon@leon.nu"); +MODULE_DESCRIPTION("Tegra+ALC5632 machine ASoC driver"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:" DRV_NAME);
I still don't see how the hp gpio detect comes in. Can you explain this?
The detection itself is done via soc-jack interface.
Leon Romanovsky wrote at Wednesday, November 23, 2011 1:05 AM:
Marc Dietrich wrote:
I still don't see how the hp gpio detect comes in. Can you explain this?
The detection itself is done via soc-jack interface.
I think Marc wanted to know how the driver knows which GPIO to use, and where it sets up use of that GPIO. The Tegra+WM8903 machine driver does the following, which I think is missing from this driver:
tegra_wm8903_hp_jack_gpio.gpio = pdata->gpio_hp_det;
(I hope to review your patch myself later today)
On Wed, Nov 23, 2011 at 19:37, Stephen Warren swarren@nvidia.com wrote:
Leon Romanovsky wrote at Wednesday, November 23, 2011 1:05 AM:
Marc Dietrich wrote:
I still don't see how the hp gpio detect comes in. Can you explain this?
The detection itself is done via soc-jack interface.
I think Marc wanted to know how the driver knows which GPIO to use, and where it sets up use of that GPIO. The Tegra+WM8903 machine driver does the following, which I think is missing from this driver:
I removed it from our code before patch, because it didn't work as expected. There is no reason to push the non-working parts. Our main reason of this merge is to receive guidance from community on "correct" way of implementing.
tegra_wm8903_hp_jack_gpio.gpio = pdata->gpio_hp_det;
(I hope to review your patch myself later today)
-- nvpublic
On Mon, Nov 21, 2011 at 10:08:08PM +0200, Leon Romanovsky wrote:
Mostly OK, a few things below but they're very minor.
- err = snd_soc_dai_set_fmt(cpu_dai,
SND_SOC_DAIFMT_I2S |SND_SOC_DAIFMT_NB_NF |SND_SOC_DAIFMT_CBS_CFS);- if (err < 0) {
dev_err(card->dev, "cpu_dai fmt not set\n");return err;- }
- err = snd_soc_dai_set_sysclk(codec_dai, 0, mclk,
SND_SOC_CLOCK_IN);- if (err < 0) {
dev_err(card->dev, "codec_dai clock not set\n");return err;- }
These should use the .dai_fmt member in the dai_link struct.
- ret = snd_soc_add_controls(codec, tegra_alc5632_controls,
ARRAY_SIZE(tegra_alc5632_controls));- if (ret < 0)
return ret;- snd_soc_dapm_new_controls(dapm, tegra_alc5632_dapm_widgets,
ARRAY_SIZE(tegra_alc5632_dapm_widgets));- snd_soc_dapm_add_routes(dapm, tegra_alc5632_audio_map,
ARRAY_SIZE(tegra_alc5632_audio_map));
These should use the controls, dapm_widgets and dapm_routes members of the card structure.
- snd_soc_dapm_sync(dapm);
No need for this, the core will sync for you.
- if (!machine_is_paz00()) {
dev_err(&pdev->dev, "Not running on Toshiba AC100!\n");return -ENODEV;- }
Should be no need to check for htis as you're using an explicit platform device - if the device is registered you should be OK.
+static int __init tegra_alc5632_init(void) +{
- return platform_driver_register(&tegra_alc5632_driver);
+} +module_init(tegra_alc5632_init);
+static void __exit tegra_alc5632_exit(void) +{
- platform_driver_unregister(&tegra_alc5632_driver);
+} +module_exit(tegra_alc5632_exit);
Use platform_module_driver for this.
Leon Romanovsky wrote at Monday, November 21, 2011 1:08 PM:
At this stage only Toshiba AC100/Dynabook supported.
...
diff --git a/sound/soc/tegra/Kconfig b/sound/soc/tegra/Kconfig
...
+config SND_SOC_TEGRA_ALC5632
- tristate "SoC Audio support for Tegra boards using an ALC5632 codec"
- depends on SND_SOC_TEGRA && I2C
- select SND_SOC_TEGRA_I2S
- select SND_SOC_ALC5632
- help
Say Y or M here if you want to add support for SoC audio on theToshiba AC100 netbook.
If this is going to be named tegra_acl5632 rather than something board- specific such as tegra_paz00, then perhaps you should do Kconfig the same way as the tegra_wm8903 driver; have a MACH_HAS_SND_SOC_TEGRA_ALC5632 variable that boards need to select, and make this depend on that?
diff --git a/sound/soc/tegra/tegra_alc5632.c b/sound/soc/tegra/tegra_alc5632.c
...
+static int tegra_alc5632_asoc_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)
...
- srate = params_rate(params);
- switch (srate) {
- case 64000:
- case 88200:
- case 96000:
mclk = 128 * srate;break;- default:
mclk = 512 * srate;break;- }
- /* FIXME: Codec only requires >= 3MHz if OSR==0 */
- while (mclk < 6000000)
mclk *= 2;
That is identical to the code in tegra_wm8903. Are you sure it's correct for the ALC5632? At the very least, I think the comment is wrong; OSR is a WM8903 register field, which I doubt exists on the ALC5632.
- err = snd_soc_dai_set_fmt(cpu_dai,
SND_SOC_DAIFMT_I2S |SND_SOC_DAIFMT_NB_NF |SND_SOC_DAIFMT_CBS_CFS);- if (err < 0) {
dev_err(card->dev, "cpu_dai fmt not set\n");return err;- }
- err = snd_soc_dai_set_sysclk(codec_dai, 0, mclk,
SND_SOC_CLOCK_IN);- if (err < 0) {
dev_err(card->dev, "codec_dai clock not set\n");return err;- }
Mark Brown wrote here:
These should use the .dai_fmt member in the dai_link struct.
Mark, I guess you mean that we should remove those two snd_soc_dai_set_fmt calls, and just set .dai_fmt to a constant value? If so, that should be applied to the other two Tegra machine drivers too.
That said, while these values are constant right now, I think they won't always be; I think we'll need to switch between DSP mode for mono and I2S mode for stereo here; at least I /think/ that's what I remember our downstream drivers doing...
+static struct snd_soc_jack_gpio tegra_alc5632_hs_jack_gpios[] = {
- {
.name = "Headset Detect",.report = SND_JACK_HEADSET,.debounce_time = 150,- }
+};
You probably don't want to make this an array, since that'll make the code that fills in tegra_alc5632_hs_jack_gpio [0].gpio have to hard-code the "0" array index; better to make this just:
static struct snd_soc_jack_gpio tegra_alc5632_hs_jack_gpio = { .name = "Headset Detect", .report = SND_JACK_HEADSET, .debounce_time = 150, };
And then you can assign tegra_alc5632_hs_jack_gpio.gpio.
+static const struct snd_soc_dapm_route tegra_alc5632_audio_map[] = {
- /* Internal Mic */
- {"MIC2", NULL, "Mic Bias 2"},
- {"Mic Bias 2", NULL, "Int Mic"},
Yay, a separate mic bias for each microphone:-)
+static int tegra_alc5632_asoc_init(struct snd_soc_pcm_runtime *rtd)
...
- snd_soc_jack_new(codec, "Headset Jack", SND_JACK_HEADSET,
&tegra_alc5632_hs_jack);- snd_soc_jack_add_pins(&tegra_alc5632_hs_jack,
ARRAY_SIZE(tegra_alc5632_hs_jack_pins),tegra_alc5632_hs_jack_pins);- snd_soc_jack_add_gpios(&tegra_alc5632_hs_jack,
ARRAY_SIZE(tegra_alc5632_hs_jack_gpios),tegra_alc5632_hs_jack_gpios);
Do you need to call something in the ALC5632 codec driver here to start mic detection? The tegra_wm8903 driver calls wm8903_mic_detect() here.
- snd_soc_dapm_nc_pin(dapm, "AUXOUT");
- snd_soc_dapm_nc_pin(dapm, "LINEINL");
- snd_soc_dapm_nc_pin(dapm, "LINEINR");
- snd_soc_dapm_nc_pin(dapm, "PHONEP");
- snd_soc_dapm_nc_pin(dapm, "PHONEN");
- snd_soc_dapm_nc_pin(dapm, "MIC2");
----------==========----------==========----------==========----------========== You may be able to remove that if you set snd_soc_tegra_alc5632.fully_routed. The patch for that was just merged into Mark's ASoC tree.
+static __devinit int tegra_alc5632_probe(struct platform_device *pdev)
...
- alc5632 = kzalloc(sizeof(struct tegra_alc5632), GFP_KERNEL);
If you use devm_kzalloc there, it'll remove the need to manually free it; see the very latest tegra_wm8903 driver.
+err_clear_drvdata:
- snd_soc_card_set_drvdata(card, NULL);
- platform_set_drvdata(pdev, NULL);
- card->dev = NULL;
There's no need for those last 3 lines.
- tegra_asoc_utils_fini(&alc5632->util_data);
+err_free_tegra_alc5632:
- kfree(alc5632);
That kfree is what you could get rid of using devm_kzalloc.
+static int __devexit tegra_alc5632_remove(struct platform_device *pdev)
...
- snd_soc_card_set_drvdata(card, NULL);
- platform_set_drvdata(pdev, NULL);
- card->dev = NULL;
Same here; no need for those last 3 lines.
- tegra_asoc_utils_fini(&alc5632->util_data);
- kfree(alc5632);
That's another thing you could remove if using devm_kzalloc.
...
+MODULE_LICENSE("GPL");
The comment at the top would imply "GPL v2", since no "or later" is mentioned there. Yes, the existing Tegra code is probably wrong here too.
On Wed, Nov 23, 2011 at 01:50:39PM -0800, Stephen Warren wrote:
Leon Romanovsky wrote at Monday, November 21, 2011 1:08 PM:
- switch (srate) {
- case 64000:
- case 88200:
- case 96000:
mclk = 128 * srate;break;- default:
mclk = 512 * srate;break;- }
- /* FIXME: Codec only requires >= 3MHz if OSR==0 */
- while (mclk < 6000000)
mclk *= 2;
That is identical to the code in tegra_wm8903. Are you sure it's correct for the ALC5632? At the very least, I think the comment is wrong; OSR is a WM8903 register field, which I doubt exists on the ALC5632.
I wouldn't be surprised if it were actually - the requirements for CODECs are generally much of a muchness, there's good sensible DSP and electrical engineering requirmenents for them.
Mark Brown wrote here:
These should use the .dai_fmt member in the dai_link struct.
Mark, I guess you mean that we should remove those two snd_soc_dai_set_fmt calls, and just set .dai_fmt to a constant value? If so, that should be applied to the other two Tegra machine drivers too.
Yes.
That said, while these values are constant right now, I think they won't always be; I think we'll need to switch between DSP mode for mono and I2S mode for stereo here; at least I /think/ that's what I remember our downstream drivers doing...
That would be a surprising restriction for your hardware to have. From the CODEC point of view it really doesn't care at all, all the interface formats are interchangable.
+static struct snd_soc_jack_gpio tegra_alc5632_hs_jack_gpios[] = {
- {
...
Do you need to call something in the ALC5632 codec driver here to start mic detection? The tegra_wm8903 driver calls wm8903_mic_detect() here.
It's not using the CODEC for detection, it's using a simple GPIO for tip detect.
+MODULE_LICENSE("GPL");
The comment at the top would imply "GPL v2", since no "or later" is mentioned there. Yes, the existing Tegra code is probably wrong here too.
GPL v2 is the standard interpretation for GPL within the kernel given that that is the license of the kernel itself. I'd only bother saying anything different if the license were different but it won't do any harm.
Mark Brown wrote at Wednesday, November 23, 2011 2:59 PM:
On Wed, Nov 23, 2011 at 01:50:39PM -0800, Stephen Warren wrote:
Leon Romanovsky wrote at Monday, November 21, 2011 1:08 PM:
...
Mark Brown wrote here:
These should use the .dai_fmt member in the dai_link struct.
Mark, I guess you mean that we should remove those two snd_soc_dai_set_fmt calls, and just set .dai_fmt to a constant value? If so, that should be applied to the other two Tegra machine drivers too.
Yes.
That said, while these values are constant right now, I think they won't always be; I think we'll need to switch between DSP mode for mono and I2S mode for stereo here; at least I /think/ that's what I remember our downstream drivers doing...
That would be a surprising restriction for your hardware to have. From the CODEC point of view it really doesn't care at all, all the interface formats are interchangable.
I believe Tegra20 HW "hard-codes" the channel count to 2 in I2S mode, And hence always pulls 2 channels from the FIFO for each frame. So, I2S can't do mono. This appears to be fixed in Tegra30. Tegra can be surprising:-)
+MODULE_LICENSE("GPL");
The comment at the top would imply "GPL v2", since no "or later" is mentioned there. Yes, the existing Tegra code is probably wrong here too.
GPL v2 is the standard interpretation for GPL within the kernel given that that is the license of the kernel itself. I'd only bother saying anything different if the license were different but it won't do any harm.
I mentioned this, because of the following in include/linux/module.h
* The following license idents are currently accepted as indicating free * software modules * * "GPL" [GNU Public License v2 or later] * "GPL v2" [GNU Public License v2]
On Wed, Nov 23, 2011 at 02:30:12PM -0800, Stephen Warren wrote:
Mark Brown wrote at Wednesday, November 23, 2011 2:59 PM:
That said, while these values are constant right now, I think they won't always be; I think we'll need to switch between DSP mode for mono and I2S mode for stereo here; at least I /think/ that's what I remember our downstream drivers doing...
That would be a surprising restriction for your hardware to have. From the CODEC point of view it really doesn't care at all, all the interface formats are interchangable.
I believe Tegra20 HW "hard-codes" the channel count to 2 in I2S mode, And hence always pulls 2 channels from the FIFO for each frame. So, I2S can't do mono. This appears to be fixed in Tegra30. Tegra can be surprising:-)
If it's got that restriction why not run it in PCM mode all the time?
Mark Brown wrote at Wednesday, November 23, 2011 3:46 PM:
On Wed, Nov 23, 2011 at 02:30:12PM -0800, Stephen Warren wrote:
Mark Brown wrote at Wednesday, November 23, 2011 2:59 PM:
That said, while these values are constant right now, I think they won't always be; I think we'll need to switch between DSP mode for mono and I2S mode for stereo here; at least I /think/ that's what I remember our downstream drivers doing...
That would be a surprising restriction for your hardware to have. From the CODEC point of view it really doesn't care at all, all the interface formats are interchangable.
I believe Tegra20 HW "hard-codes" the channel count to 2 in I2S mode, And hence always pulls 2 channels from the FIFO for each frame. So, I2S can't do mono. This appears to be fixed in Tegra30. Tegra can be surprising:-)
If it's got that restriction why not run it in PCM mode all the time?
That's a good idea. The internal feedback is that should work.
Leon, I'd suggest just hard-coding the current values into .dai_fmt as Mark suggested. I'll take an action to investigate switching to DSP/PCM mode, mono support, and switching the machine drivers (and I2S driver) to it.
On Wed, Nov 23, 2011 at 23:50, Stephen Warren swarren@nvidia.com wrote:
Leon Romanovsky wrote at Monday, November 21, 2011 1:08 PM:
At this stage only Toshiba AC100/Dynabook supported.
...
diff --git a/sound/soc/tegra/Kconfig b/sound/soc/tegra/Kconfig
...
+config SND_SOC_TEGRA_ALC5632
- tristate "SoC Audio support for Tegra boards using an ALC5632 codec"
- depends on SND_SOC_TEGRA && I2C
- select SND_SOC_TEGRA_I2S
- select SND_SOC_ALC5632
- help
- Say Y or M here if you want to add support for SoC audio on the
- Toshiba AC100 netbook.
If this is going to be named tegra_acl5632 rather than something board- specific such as tegra_paz00, then perhaps you should do Kconfig the same way as the tegra_wm8903 driver; have a MACH_HAS_SND_SOC_TEGRA_ALC5632 variable that boards need to select, and make this depend on that?
I saw in the Kconfing MACH_HAS_SND_SOC_TEGRA_WM8903, but don't know if it is necessary, because no one use it. Do I need to do the same in our case too?
diff --git a/sound/soc/tegra/tegra_alc5632.c b/sound/soc/tegra/tegra_alc5632.c
...
+static int tegra_alc5632_asoc_hw_params(struct snd_pcm_substream *substream,
- struct snd_pcm_hw_params *params)
...
- srate = params_rate(params);
- switch (srate) {
- case 64000:
- case 88200:
- case 96000:
- mclk = 128 * srate;
- break;
- default:
- mclk = 512 * srate;
- break;
- }
- /* FIXME: Codec only requires >= 3MHz if OSR==0 */
- while (mclk < 6000000)
- mclk *= 2;
That is identical to the code in tegra_wm8903. Are you sure it's correct for the ALC5632? At the very least, I think the comment is wrong; OSR is a WM8903 register field, which I doubt exists on the ALC5632.
The code started from the harmony board file, there are possibilities it is not necessary.
+static struct snd_soc_jack_gpio tegra_alc5632_hs_jack_gpios[] = {
- {
- .name = "Headset Detect",
- .report = SND_JACK_HEADSET,
- .debounce_time = 150,
- }
+};
You probably don't want to make this an array, since that'll make the code that fills in tegra_alc5632_hs_jack_gpio [0].gpio have to hard-code the "0" array index; better to make this just:
This is something, I never understand. There are plenty implementations which use SND_JACK_HEADSET and plenty which prefer to separate it. What is the guideline?
+static const struct snd_soc_dapm_route tegra_alc5632_audio_map[] = {
- /* Internal Mic */
- {"MIC2", NULL, "Mic Bias 2"},
- {"Mic Bias 2", NULL, "Int Mic"},
Yay, a separate mic bias for each microphone:-)
Is it wrong? Our codec support two microphones, and two different biases.
Leon Romanovsky wrote at Sunday, November 27, 2011 1:48 AM:
On Wed, Nov 23, 2011 at 23:50, Stephen Warren swarren@nvidia.com wrote:
Leon Romanovsky wrote at Monday, November 21, 2011 1:08 PM:
At this stage only Toshiba AC100/Dynabook supported.
...
diff --git a/sound/soc/tegra/Kconfig b/sound/soc/tegra/Kconfig
...
+config SND_SOC_TEGRA_ALC5632
- tristate "SoC Audio support for Tegra boards using an ALC5632 codec"
- depends on SND_SOC_TEGRA && I2C
- select SND_SOC_TEGRA_I2S
- select SND_SOC_ALC5632
- help
- Say Y or M here if you want to add support for SoC audio on the
- Toshiba AC100 netbook.
If this is going to be named tegra_acl5632 rather than something board- specific such as tegra_paz00, then perhaps you should do Kconfig the same way as the tegra_wm8903 driver; have a MACH_HAS_SND_SOC_TEGRA_ALC5632 variable that boards need to select, and make this depend on that?
I saw in the Kconfing MACH_HAS_SND_SOC_TEGRA_WM8903, but don't know if it is necessary, because no one use it. Do I need to do the same in our case too?
Well, I suppose we can always add this later if we need.
Thinking about this more though, with the moved to device-tree, having such a variable isn't that useful, since the arch/arm/mach-tegra side won't have individual machine Kconfig options that'll allow selecting this driver.
So, it's fine the way it is.
diff --git a/sound/soc/tegra/tegra_alc5632.c b/sound/soc/tegra/tegra_alc5632.c
...
+static int tegra_alc5632_asoc_hw_params(struct snd_pcm_substream *substream,
- struct snd_pcm_hw_params *params)
...
- srate = params_rate(params);
- switch (srate) {
- case 64000:
- case 88200:
- case 96000:
- mclk = 128 * srate;
- break;
- default:
- mclk = 512 * srate;
- break;
- }
- /* FIXME: Codec only requires >= 3MHz if OSR==0 */
- while (mclk < 6000000)
- mclk *= 2;
That is identical to the code in tegra_wm8903. Are you sure it's correct for the ALC5632? At the very least, I think the comment is wrong; OSR is a WM8903 register field, which I doubt exists on the ALC5632.
The code started from the harmony board file, there are possibilities it is not necessary.
Mark pointed out that this might be correct anyway. Still, I'd ask that you check the codec's datasheet to be sure.
+static struct snd_soc_jack_gpio tegra_alc5632_hs_jack_gpios[] = {
- {
- .name = "Headset Detect",
- .report = SND_JACK_HEADSET,
- .debounce_time = 150,
- }
+};
You probably don't want to make this an array, since that'll make the code that fills in tegra_alc5632_hs_jack_gpio [0].gpio have to hard-code the "0" array index; better to make this just:
This is something, I never understand. There are plenty implementations which use SND_JACK_HEADSET and plenty which prefer to separate it. What is the guideline?
I don't quite understand your response. I was simply pointing out that The code above would be simpler as:
static struct snd_soc_jack_gpio tegra_alc5632_hs_jack_gpio = { .name = "Headset Detect", .report = SND_JACK_HEADSET, .debounce_time = 150, };
This was review feedback from Mark on other Tegra ASoC machine drivers.
It's plausible there are machines which don't format it that way. Still, when adding new ones, we may as well follow best practices if possible.
+static const struct snd_soc_dapm_route tegra_alc5632_audio_map[] = {
- /* Internal Mic */
- {"MIC2", NULL, "Mic Bias 2"},
- {"Mic Bias 2", NULL, "Int Mic"},
Yay, a separate mic bias for each microphone:-)
Is it wrong? Our codec support two microphones, and two different biases.
My comment was re: the design of the Harmony board, which is difficult to fully support in HW. It's good that there are two mic bias pins. I see no issue with your code.
On Mon, Nov 28, 2011 at 18:52, Stephen Warren swarren@nvidia.com wrote:
Leon Romanovsky wrote at Sunday, November 27, 2011 1:48 AM:
On Wed, Nov 23, 2011 at 23:50, Stephen Warren swarren@nvidia.com
wrote:
Leon Romanovsky wrote at Monday, November 21, 2011 1:08 PM:
At this stage only Toshiba AC100/Dynabook supported.
...
diff --git a/sound/soc/tegra/Kconfig b/sound/soc/tegra/Kconfig
...
+config SND_SOC_TEGRA_ALC5632
tristate "SoC Audio support for Tegra boards using an ALC5632codec"
depends on SND_SOC_TEGRA && I2Cselect SND_SOC_TEGRA_I2Sselect SND_SOC_ALC5632helpSay Y or M here if you want to add support for SoC audio onthe
Toshiba AC100 netbook.If this is going to be named tegra_acl5632 rather than something board- specific such as tegra_paz00, then perhaps you should do Kconfig the same way as the tegra_wm8903 driver; have a
MACH_HAS_SND_SOC_TEGRA_ALC5632
variable that boards need to select, and make this depend on that?
I saw in the Kconfing MACH_HAS_SND_SOC_TEGRA_WM8903, but don't know if it is necessary, because no one use it. Do I need to do the same in our case too?
Well, I suppose we can always add this later if we need.
Thinking about this more though, with the moved to device-tree, having such a variable isn't that useful, since the arch/arm/mach-tegra side won't have individual machine Kconfig options that'll allow selecting this driver.
So, it's fine the way it is.
diff --git a/sound/soc/tegra/tegra_alc5632.c
b/sound/soc/tegra/tegra_alc5632.c
...
+static int tegra_alc5632_asoc_hw_params(struct snd_pcm_substream
*substream,
struct snd_pcm_hw_params*params)
...
srate = params_rate(params);switch (srate) {case 64000:case 88200:case 96000:mclk = 128 * srate;break;default:mclk = 512 * srate;break;}/* FIXME: Codec only requires >= 3MHz if OSR==0 */while (mclk < 6000000)mclk *= 2;That is identical to the code in tegra_wm8903. Are you sure it's
correct
for the ALC5632? At the very least, I think the comment is wrong; OSR
is
a WM8903 register field, which I doubt exists on the ALC5632.
The code started from the harmony board file, there are possibilities it is not necessary.
Mark pointed out that this might be correct anyway. Still, I'd ask that you check the codec's datasheet to be sure.
Sure
+static struct snd_soc_jack_gpio tegra_alc5632_hs_jack_gpios[] = {
{.name = "Headset Detect",.report = SND_JACK_HEADSET,.debounce_time = 150,}+};
You probably don't want to make this an array, since that'll make the code that fills in tegra_alc5632_hs_jack_gpio [0].gpio have to
hard-code
the "0" array index; better to make this just:
This is something, I never understand. There are plenty implementations which use SND_JACK_HEADSET and plenty which prefer to separate it. What is the guideline?
I don't quite understand your response. I was simply pointing out that The code above would be simpler as:
static struct snd_soc_jack_gpio tegra_alc5632_hs_jack_gpio = { .name = "Headset Detect", .report = SND_JACK_HEADSET, .debounce_time = 150, };
Thanks, I previously missed the difference: static struct snd_soc_jack_gpio tegra_alc5632_hs_jack_gpios[] -----> static struct snd_soc_jack_gpio tegra_alc5632_hs_jack_gpio
This was review feedback from Mark on other Tegra ASoC machine drivers.
It's plausible there are machines which don't format it that way. Still, when adding new ones, we may as well follow best practices if possible.
+static const struct snd_soc_dapm_route tegra_alc5632_audio_map[] = {
/* Internal Mic */{"MIC2", NULL, "Mic Bias 2"},{"Mic Bias 2", NULL, "Int Mic"},Yay, a separate mic bias for each microphone:-)
Is it wrong? Our codec support two microphones, and two different biases.
My comment was re: the design of the Harmony board, which is difficult to fully support in HW. It's good that there are two mic bias pins. I see no issue with your code.
-- nvpublic
Thanks for the review, I'll post updated patch in the next few days.
participants (4)
-
Leon Romanovsky -
Marc Dietrich -
Mark Brown -
Stephen Warren