[PATCH] ASoC: Tegra: Add support of tegra boards based on ALC5632 codec

[PATCH] ASoC: Tegra: Add support of tegra boards based on ALC5632 codec

[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 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);
-- 
1.7.3.4

Re: [alsa-devel] [PATCH] ASoC: Tegra: Add support of tegra boards basedon ALC5632 codec

[Marc Dietrich]

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-2ukJVAZIZ/Y@public.gmane.org>
> Signed-off-by: Andrey Danin <danindrey-JGs/UdohzUI@public.gmane.org>
> ---
>  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

										^^^ you may replace this with "Tegra machine ASoC 
driver for boards using ALC5332 codec"

> +*
> +* Copyright (C) 2011 The AC100 Kernel Team <ac100-oU9gvf+ajcSyy5GWsZ7R2qxOck334EZe@public.gmane.org>
> +*
> +* Authors:  Leon Romanovsky <leon-2ukJVAZIZ/Y@public.gmane.org>
> +*           Andrey Danin <danindrey-JGs/UdohzUI@public.gmane.org>
> +*           Marc Dietrich <marvin24-Mmb7MZpHnFY@public.gmane.org>
> +*
> +* 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-2ukJVAZIZ/Y@public.gmane.org>");
> +MODULE_DESCRIPTION("Tegra+ALC5632 machine ASoC driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);

Re: [PATCH] ASoC: Tegra: Add support of tegra boards basedon ALC5632 codec

[Leon Romanovsky]

> 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 | Independent Linux Consultant
        www.leon.nu | leon@leon.nu
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Tegra: Add support of tegra boards based on ALC5632 codec

[Mark Brown]

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.

RE: [alsa-devel] [PATCH] ASoC: Tegra: Add support of tegra boards basedon ALC5632 codec

[Stephen Warren]

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)

-- 
nvpublic

Re: [PATCH] ASoC: Tegra: Add support of tegra boards based on ALC5632 codec

[Stephen Warren]

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?

> 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.

-- 
nvpublic

Re: [PATCH] ASoC: Tegra: Add support of tegra boards based on ALC5632 codec

[Mark Brown]

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.

Re: [PATCH] ASoC: Tegra: Add support of tegra boards based on ALC5632 codec

[Stephen Warren]

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]

-- 
nvpublic

Re: [PATCH] ASoC: Tegra: Add support of tegra boards based on ALC5632 codec

[Mark Brown]

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?

Re: [PATCH] ASoC: Tegra: Add support of tegra boards based on ALC5632 codec

[Stephen Warren]

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.

-- 
nvpublic

Re: [alsa-devel] [PATCH] ASoC: Tegra: Add support of tegra boards basedon ALC5632 codec

[Leon Romanovsky]

On Wed, Nov 23, 2011 at 19:37, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 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
>
>



-- 
Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | leon-2ukJVAZIZ/Y@public.gmane.org

Re: [PATCH] ASoC: Tegra: Add support of tegra boards based on ALC5632 codec

[Leon Romanovsky]

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 | Independent Linux Consultant
        www.leon.nu | leon@leon.nu
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Tegra: Add support of tegra boards based on ALC5632 codec

[Stephen Warren]

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.

-- 
nvpublic

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Tegra: Add support of tegra boards based on ALC5632 codec

[Leon Romanovsky]

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 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.
>
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.



-- 
Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | leon@leon.nu