[PATCH 2/4] ASoC: DaVinci: Voice Codec Interface

[PATCH 2/4] ASoC: DaVinci: Voice Codec Interface

[miguel.aguilar]

From: Miguel Aguilar <miguel.aguilar@ridgerun.com>

1) Adds an interface needed by the voice codec CQ0093.
2) Add an option to select internal or external codec in the DM365.

Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com>
---
 sound/soc/davinci/Kconfig        |   26 +++-
 sound/soc/davinci/Makefile       |    2 +
 sound/soc/davinci/davinci-vcif.c |  363 ++++++++++++++++++++++++++++++++++++++
 sound/soc/davinci/davinci-vcif.h |   63 +++++++
 4 files changed, 452 insertions(+), 2 deletions(-)
 create mode 100644 sound/soc/davinci/davinci-vcif.c
 create mode 100644 sound/soc/davinci/davinci-vcif.h

diff --git a/sound/soc/davinci/Kconfig b/sound/soc/davinci/Kconfig
index 047ee39..051a10e 100644
--- a/sound/soc/davinci/Kconfig
+++ b/sound/soc/davinci/Kconfig
@@ -9,18 +9,40 @@ config SND_DAVINCI_SOC
 config SND_DAVINCI_SOC_I2S
 	tristate
 
+config SND_DAVINCI_SOC_VCIF
+	tristate
+
 config SND_DAVINCI_SOC_MCASP
 	tristate
 
 config SND_DAVINCI_SOC_EVM
 	tristate "SoC Audio support for DaVinci DM6446, DM355 or DM365 EVM"
 	depends on SND_DAVINCI_SOC
-	depends on MACH_DAVINCI_EVM || MACH_DAVINCI_DM355_EVM  || MACH_DAVINCI_DM365_EVM
+	depends on MACH_DAVINCI_EVM || MACH_DAVINCI_DM355_EVM || MACH_DAVINCI_DM365_EVM
 	select SND_DAVINCI_SOC_I2S
 	select SND_SOC_TLV320AIC3X
 	help
 	  Say Y if you want to add support for SoC audio on TI
-	  DaVinci DM6446 or DM355 EVM platforms.
+	  DaVinci DM6446, DM355 or DM365 EVM platforms.
+
+choice
+	prompt "DM365 codec select"
+	depends on SND_DAVINCI_SOC_EVM
+	depends on MACH_DAVINCI_DM365_EVM
+	default SND_DM365_EXTERNAL_CODEC
+
+config SND_DM365_INTERNAL_CODEC
+	bool "CQ93VC"
+	select SND_DAVINCI_SOC_VCIF
+	select SND_SOC_CQ0093VC
+	help
+	  Say Y if you want to add support for SoC On-chip voice codec
+
+config SND_DM365_EXTERNAL_CODEC
+	bool "AIC3101"
+	help
+	  Say Y if you want to add support for AIC3101 audio codec
+endchoice
 
 config  SND_DM6467_SOC_EVM
 	tristate "SoC Audio support for DaVinci DM6467 EVM"
diff --git a/sound/soc/davinci/Makefile b/sound/soc/davinci/Makefile
index a6939d7..a93679d 100644
--- a/sound/soc/davinci/Makefile
+++ b/sound/soc/davinci/Makefile
@@ -2,10 +2,12 @@
 snd-soc-davinci-objs := davinci-pcm.o
 snd-soc-davinci-i2s-objs := davinci-i2s.o
 snd-soc-davinci-mcasp-objs:= davinci-mcasp.o
+snd-soc-davinci-vcif-objs:= davinci-vcif.o
 
 obj-$(CONFIG_SND_DAVINCI_SOC) += snd-soc-davinci.o
 obj-$(CONFIG_SND_DAVINCI_SOC_I2S) += snd-soc-davinci-i2s.o
 obj-$(CONFIG_SND_DAVINCI_SOC_MCASP) += snd-soc-davinci-mcasp.o
+obj-$(CONFIG_SND_DAVINCI_SOC_VCIF) += snd-soc-davinci-vcif.o
 
 # DAVINCI Machine Support
 snd-soc-evm-objs := davinci-evm.o
diff --git a/sound/soc/davinci/davinci-vcif.c b/sound/soc/davinci/davinci-vcif.c
new file mode 100644
index 0000000..83191f1
--- /dev/null
+++ b/sound/soc/davinci/davinci-vcif.c
@@ -0,0 +1,363 @@
+/*
+ * ALSA SoC Voice Codec Interface for TI DAVINCI processor
+ *
+ * Copyright (C) 2009 Texas Instruments.
+ *
+ * Author: Miguel Aguilar <miguel.aguilar@ridgerun.com>
+ *
+ * Initial code: Hui Geng
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/initval.h>
+#include <sound/soc.h>
+
+#include "davinci-pcm.h"
+#include "davinci-i2s.h"
+#include "davinci-vcif.h"
+
+#define MOD_REG_BIT(val, mask, set) do { \
+	if (set) { \
+		val |= mask; \
+	} else { \
+		val &= ~mask; \
+	} \
+} while (0)
+
+static struct davinci_pcm_dma_params davinci_vcif_pcm_out = {
+	.name = "VCIF PCM Stereo out",
+};
+
+static struct davinci_pcm_dma_params davinci_vcif_pcm_in = {
+	.name = "VCIF PCM Stereo in",
+};
+
+struct davinci_vcif_dev {
+	void __iomem			*base;
+	resource_size_t			pbase;
+	size_t				base_size;
+	int				mode;
+	u32				pcr;
+	struct clk			*clk;
+	struct davinci_pcm_dma_params	*dma_params[2];
+};
+
+static inline void davinci_vcif_write_reg(struct davinci_vcif_dev *dev,
+					   int reg, u32 val)
+{
+	__raw_writel(val, dev->base + reg);
+}
+
+static inline u32 davinci_vcif_read_reg(struct davinci_vcif_dev *dev, int reg)
+{
+	return __raw_readl(dev->base + reg);
+}
+
+static void davinci_vcif_start(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct davinci_vcif_dev *dev = rtd->dai->cpu_dai->private_data;
+	u32 w;
+
+	/* Start the sample generator and enable transmitter/receiver */
+	w = davinci_vcif_read_reg(dev, DAVINCI_VCIF_CTRL_REG);
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		MOD_REG_BIT(w, DAVINCI_VCIF_CTRL_RSTDAC, 1);
+	else
+		MOD_REG_BIT(w, DAVINCI_VCIF_CTRL_RSTADC, 1);
+
+	davinci_vcif_write_reg(dev, DAVINCI_VCIF_CTRL_REG, w);
+}
+
+static void davinci_vcif_stop(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct davinci_vcif_dev *dev = rtd->dai->cpu_dai->private_data;
+	u32 w;
+
+	/* Reset transmitter/receiver and sample rate/frame sync generators */
+	w = davinci_vcif_read_reg(dev, DAVINCI_VCIF_CTRL_REG);
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		MOD_REG_BIT(w, DAVINCI_VCIF_CTRL_RSTDAC, 0);
+	else
+		MOD_REG_BIT(w, DAVINCI_VCIF_CTRL_RSTADC, 0);
+
+	davinci_vcif_write_reg(dev, DAVINCI_VCIF_CTRL_REG, w);
+}
+
+static int davinci_vcif_startup(struct snd_pcm_substream *substream,
+			       struct snd_soc_dai *cpu_dai)
+{
+	struct davinci_vcif_dev *dev = cpu_dai->private_data;
+
+	cpu_dai->dma_data = dev->dma_params[substream->stream];
+	return 0;
+}
+
+static int davinci_vcif_hw_params(struct snd_pcm_substream *substream,
+				 struct snd_pcm_hw_params *params,
+				 struct snd_soc_dai *dai)
+{
+	struct davinci_pcm_dma_params *dma_params = dai->dma_data;
+	struct davinci_vcif_dev *dev = dai->private_data;
+	u32 w;
+
+	/* Restart the codec before setup */
+	davinci_vcif_stop(substream);
+	davinci_vcif_start(substream);
+
+	/* General line settings */
+	davinci_vcif_write_reg(dev,
+			DAVINCI_VCIF_CTRL_REG, DAVINCI_VCIF_CTRL_MASK);
+
+	davinci_vcif_write_reg(dev,
+			DAVINCI_VCIF_INTCLR_REG, DAVINCI_VCIF_INT_MASK);
+
+	davinci_vcif_write_reg(dev,
+			DAVINCI_VCIF_INTEN_REG, DAVINCI_VCIF_INT_MASK);
+
+	w = davinci_vcif_read_reg(dev, DAVINCI_VCIF_CTRL_REG);
+
+	/* Determine xfer data type */
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_U8:
+		dma_params->data_type = 0;
+
+		MOD_REG_BIT(w, DAVINCI_VCIF_CTRL_RD_BITS_8 |
+					DAVINCI_VCIF_CTRL_RD_UNSIGNED |
+					DAVINCI_VCIF_CTRL_WD_BITS_8 |
+					DAVINCI_VCIF_CTRL_WD_UNSIGNED, 1);
+		break;
+	case SNDRV_PCM_FORMAT_S8:
+		dma_params->data_type = 1;
+
+		MOD_REG_BIT(w, DAVINCI_VCIF_CTRL_RD_BITS_8 |
+					DAVINCI_VCIF_CTRL_WD_BITS_8, 1);
+
+		MOD_REG_BIT(w, DAVINCI_VCIF_CTRL_RD_UNSIGNED |
+					DAVINCI_VCIF_CTRL_WD_UNSIGNED, 0);
+		break;
+	case SNDRV_PCM_FORMAT_S16_LE:
+		dma_params->data_type = 2;
+
+		MOD_REG_BIT(w, DAVINCI_VCIF_CTRL_RD_BITS_8 |
+					DAVINCI_VCIF_CTRL_RD_UNSIGNED |
+					DAVINCI_VCIF_CTRL_WD_BITS_8 |
+					DAVINCI_VCIF_CTRL_WD_UNSIGNED, 0);
+		break;
+	default:
+		printk(KERN_WARNING "davinci-vcif: unsupported PCM format");
+		return -EINVAL;
+	}
+
+	dma_params->acnt  = dma_params->data_type;
+
+	davinci_vcif_write_reg(dev, DAVINCI_VCIF_CTRL_REG, w);
+
+	return 0;
+}
+
+static int davinci_vcif_trigger(struct snd_pcm_substream *substream, int cmd,
+				struct snd_soc_dai *dai)
+{
+	int ret = 0;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		davinci_vcif_start(substream);
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		davinci_vcif_stop(substream);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+#define DAVINCI_VCIF_RATES	SNDRV_PCM_RATE_8000_48000
+
+static struct snd_soc_dai_ops davinci_vcif_dai_ops = {
+	.startup 	= davinci_vcif_startup,
+	.trigger	= davinci_vcif_trigger,
+	.hw_params	= davinci_vcif_hw_params,
+};
+
+struct snd_soc_dai davinci_vcif_dai = {
+	.name = "davinci-vcif",
+	.playback = {
+		.channels_min = 1,
+		.channels_max = 2,
+		.rates = DAVINCI_VCIF_RATES,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE,},
+	.capture = {
+		.channels_min = 1,
+		.channels_max = 2,
+		.rates = DAVINCI_VCIF_RATES,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE,},
+	.ops = &davinci_vcif_dai_ops,
+
+};
+EXPORT_SYMBOL_GPL(davinci_vcif_dai);
+
+static int davinci_vcif_probe(struct platform_device *pdev)
+{
+	struct davinci_vcif_dev *dev;
+	struct resource *res, *mem;
+	int ret;
+
+	dev = kzalloc(sizeof(struct davinci_vcif_dev), GFP_KERNEL);
+	if (!dev) {
+		dev_dbg(&pdev->dev, "could not allocate memory for private data\n");
+		return -ENOMEM;
+	}
+
+	dev->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dev->clk)) {
+		dev_dbg(&pdev->dev, "%s: could not get the clock for voice codec\n",
+			pdev->name);
+		ret = -ENODEV;
+		goto fail1;
+	}
+	clk_enable(dev->clk);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "no mem resource\n");
+		ret = -ENODEV;
+		goto fail2;
+	}
+
+	dev->pbase = res->start;
+	dev->base_size = resource_size(res);
+
+	mem = request_mem_region(dev->pbase, dev->base_size, pdev->name);
+	if (!mem) {
+		dev_err(&pdev->dev, "VCIF region already claimed\n");
+		ret = -EBUSY;
+		goto fail2;
+	}
+
+	dev->base = ioremap(dev->pbase, dev->base_size);
+	if (!dev->base) {
+		dev_err(&pdev->dev,"%s: can't ioremap mem resource.\n", pdev->name);
+		ret = -ENOMEM;
+		goto fail3;
+	}
+
+	dev->dma_params[SNDRV_PCM_STREAM_PLAYBACK] = &davinci_vcif_pcm_out;
+	dev->dma_params[SNDRV_PCM_STREAM_PLAYBACK]->dma_addr =
+	    (dma_addr_t)(io_v2p(dev->base) + DAVINCI_VCIF_WFIFO_REG);
+
+	dev->dma_params[SNDRV_PCM_STREAM_CAPTURE] = &davinci_vcif_pcm_in;
+	dev->dma_params[SNDRV_PCM_STREAM_CAPTURE]->dma_addr =
+	    (dma_addr_t)(io_v2p(dev->base) + DAVINCI_VCIF_RFIFO_REG);
+
+	/* first TX, then RX */
+	res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "%s: no DMA resource\n", pdev->name);
+		ret = -ENXIO;
+		goto fail4;
+	}
+	dev->dma_params[SNDRV_PCM_STREAM_PLAYBACK]->channel = res->start;
+
+	res = platform_get_resource(pdev, IORESOURCE_DMA, 1);
+	if (!res) {
+		dev_err(&pdev->dev, "%s: no DMA resource\n", pdev->name);
+		ret = -ENXIO;
+		goto fail4;
+	}
+	dev->dma_params[SNDRV_PCM_STREAM_CAPTURE]->channel = res->start;
+
+	davinci_vcif_dai.private_data = dev;
+	ret = snd_soc_register_dai(&davinci_vcif_dai);
+	if (ret != 0)
+		goto fail4;
+
+	return 0;
+
+fail4:
+	iounmap(dev->base);
+fail3:
+	release_mem_region(dev->pbase, dev->base_size);
+fail2:
+	clk_disable(dev->clk);
+	clk_put(dev->clk);
+	dev->clk = NULL;
+fail1:
+	kfree(dev);
+
+	return ret;
+}
+
+static int davinci_vcif_remove(struct platform_device *pdev)
+{
+	struct davinci_vcif_dev *dev = davinci_vcif_dai.private_data;
+
+	iounmap(dev->base);
+	release_mem_region(dev->pbase, dev->base_size);
+
+	clk_disable(dev->clk);
+	clk_put(dev->clk);
+	dev->clk = NULL;
+
+	kfree(dev);
+
+	snd_soc_unregister_dai(&davinci_vcif_dai);
+
+	return 0;
+}
+
+static struct platform_driver davinci_vcif_driver = {
+	.probe		= davinci_vcif_probe,
+	.remove		= davinci_vcif_remove,
+	.driver		= {
+		.name	= "voice_codec",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init davinci_vcif_init(void)
+{
+	return platform_driver_probe(&davinci_vcif_driver, davinci_vcif_probe);
+}
+module_init(davinci_vcif_init);
+
+static void __exit davinci_vcif_exit(void)
+{
+	platform_driver_unregister(&davinci_vcif_driver);
+}
+module_exit(davinci_vcif_exit);
+
+MODULE_AUTHOR("Miguel Aguilar");
+MODULE_DESCRIPTION("Texas Instruments DaVinci ASoC Voice Codec Interface");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/davinci/davinci-vcif.h b/sound/soc/davinci/davinci-vcif.h
new file mode 100644
index 0000000..30e441d
--- /dev/null
+++ b/sound/soc/davinci/davinci-vcif.h
@@ -0,0 +1,63 @@
+/*
+ * ALSA SoC Voice Codec Interface for TI DAVINCI processor
+ *
+ * Copyright (C) 2009 Texas Instruments.
+ *
+ * Author: Miguel Aguilar <miguel.aguilar@ridgerun.com>
+ *
+ * Initial code: Hui Geng
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef _DAVINCI_VCIF_H
+#define _DAVINCI_VCIF_H
+
+#define DAVINCI_VCIF_PID_REG		0x00
+#define DAVINCI_VCIF_CTRL_REG		0x04
+#define DAVINCI_VCIF_INTEN_REG		0x08
+#define DAVINCI_VCIF_INTSTATUS_REG	0x0c
+#define DAVINCI_VCIF_INTCLR_REG		0x10
+#define DAVINCI_VCIF_EMUL_CTRL_REG	0x14
+#define DAVINCI_VCIF_RFIFO_REG		0x20
+#define DAVINCI_VCIF_WFIFO_REG		0x24
+#define DAVINCI_VCIF_FIFOSTAT_REG	0x28
+#define DAVINCI_VCIF_TST_CTRL_REG	0x2C
+
+#define DAVINCI_VCIF_CTRL_MASK		0x5500
+#define DAVINCI_VCIF_CTRL_RSTADC	(1 << 0)
+#define DAVINCI_VCIF_CTRL_RSTDAC	(1 << 1)
+#define DAVINCI_VCIF_CTRL_RD_BITS_8	(1 << 4)
+#define DAVINCI_VCIF_CTRL_RD_UNSIGNED	(1 << 5)
+#define DAVINCI_VCIF_CTRL_WD_BITS_8	(1 << 6)
+#define DAVINCI_VCIF_CTRL_WD_UNSIGNED	(1 << 7)
+#define DAVINCI_VCIF_CTRL_RFIFOEN	(1 << 8)
+#define DAVINCI_VCIF_CTRL_RFIFOCL	(1 << 9)
+#define DAVINCI_VCIF_CTRL_RFIFOMD_WORD_1	(1 << 10)
+#define DAVINCI_VCIF_CTRL_WFIFOEN	(1 << 12)
+#define DAVINCI_VCIF_CTRL_WFIFOCL	(1 << 13)
+#define DAVINCI_VCIF_CTRL_WFIFOMD_WORD_1	(1 << 14)
+
+#define DAVINCI_VCIF_INT_MASK		0x3F
+#define DAVINCI_VCIF_INT_RDRDY_MASK	(1 << 0)
+#define DAVINCI_VCIF_INT_RERROVF_MASK	(1 << 1)
+#define DAVINCI_VCIF_INT_RERRUDR_MASK	(1 << 2)
+#define DAVINCI_VCIF_INT_WDREQ_MASK	(1 << 3)
+#define DAVINCI_VCIF_INT_WERROVF_MASK	(1 << 4)
+#define DAVINCI_VCIF_INT_WERRUDR_MASK	(1 << 5)
+
+extern struct snd_soc_dai davinci_vcif_dai;
+
+#endif
-- 
1.6.0.4

Re: [PATCH 2/4] ASoC: DaVinci: Voice Codec Interface

[Mark Brown]

On Tue, Sep 22, 2009 at 01:29:20PM -0600, miguel.aguilar@ridgerun.com wrote:
> From: Miguel Aguilar <miguel.aguilar@ridgerun.com>

> 1) Adds an interface needed by the voice codec CQ0093.

It'd be better to mention the specific name of the interface here to
help people find the driver.

> 2) Add an option to select internal or external codec in the DM365.

Can you not do both simultaneously?  This should probably be a separate
patch anyway, the CPU side support is a different issue from using it on
a specific board - one patch to add the driver, one patch to use it on
the EVM.

I'd also expect that changes in the EVM board file would be required?

> +#define MOD_REG_BIT(val, mask, set) do { \
> +	if (set) { \
> +		val |= mask; \
> +	} else { \
> +		val &= ~mask; \
> +	} \
> +} while (0)

Is there not a generic version of this somewhere?  It seems to be used
in quite a few drivers.

> +static int davinci_vcif_hw_params(struct snd_pcm_substream *substream,
> +				 struct snd_pcm_hw_params *params,
> +				 struct snd_soc_dai *dai)
> +{
> +	struct davinci_pcm_dma_params *dma_params = dai->dma_data;
> +	struct davinci_vcif_dev *dev = dai->private_data;
> +	u32 w;
> +
> +	/* Restart the codec before setup */
> +	davinci_vcif_stop(substream);
> +	davinci_vcif_start(substream);

This seems a bit odd - I'd expect to see the start at the end of the
function if you need to do this, otherwise the interface will be running
before you've configured it?

> +	/* Determine xfer data type */
> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_U8:
> +		dma_params->data_type = 0;
> +
> +		MOD_REG_BIT(w, DAVINCI_VCIF_CTRL_RD_BITS_8 |
> +					DAVINCI_VCIF_CTRL_RD_UNSIGNED |
> +					DAVINCI_VCIF_CTRL_WD_BITS_8 |
> +					DAVINCI_VCIF_CTRL_WD_UNSIGNED, 1);
> +		break;

These look like the interface needs to be configured the same way in
both directions?  If that is the case then set symmetric_rates in the
DAI.

> +	.playback = {
> +		.channels_min = 1,
> +		.channels_max = 2,
> +		.rates = DAVINCI_VCIF_RATES,
> +		.formats = SNDRV_PCM_FMTBIT_S16_LE,},

You had options for handling more formats above, shouldn't they be
included here?

> +static struct platform_driver davinci_vcif_driver = {
> +	.probe		= davinci_vcif_probe,
> +	.remove		= davinci_vcif_remove,
> +	.driver		= {
> +		.name	= "voice_codec",
> +		.owner	= THIS_MODULE,
> +	},
> +};

Might "vcif" or "davinci-vcif" be a better name?

Re: [PATCH 2/4] ASoC: DaVinci: Voice Codec Interface

[Miguel Aguilar]

Mark Brown wrote:
> On Tue, Sep 22, 2009 at 01:29:20PM -0600, miguel.aguilar@ridgerun.com wrote:
>> From: Miguel Aguilar <miguel.aguilar@ridgerun.com>
> 
>> 1) Adds an interface needed by the voice codec CQ0093.
> 
> It'd be better to mention the specific name of the interface here to
> help people find the driver.
[MA] ok.
> 
>> 2) Add an option to select internal or external codec in the DM365.
> 
> Can you not do both simultaneously?  This should probably be a separate
> patch anyway, the CPU side support is a different issue from using it on
> a specific board - one patch to add the driver, one patch to use it on
> the EVM.
> 
> I'd also expect that changes in the EVM board file would be required?
[MA] No external and internal codecs can not coexists since they share the same 
DMA channels for TX and RX, so the TI recommendation was choose one codec or the 
other one by the configuration menu.

Actually, This patch series have a separate patch for the driver, vcif, SoC 
specific code, and EVM specific code.
> 
>> +#define MOD_REG_BIT(val, mask, set) do { \
>> +	if (set) { \
>> +		val |= mask; \
>> +	} else { \
>> +		val &= ~mask; \
>> +	} \
>> +} while (0)
> 
> Is there not a generic version of this somewhere?  It seems to be used
> in quite a few drivers.
[MA] I will take a look if this is available some header that I can include.
> 
>> +static int davinci_vcif_hw_params(struct snd_pcm_substream *substream,
>> +				 struct snd_pcm_hw_params *params,
>> +				 struct snd_soc_dai *dai)
>> +{
>> +	struct davinci_pcm_dma_params *dma_params = dai->dma_data;
>> +	struct davinci_vcif_dev *dev = dai->private_data;
>> +	u32 w;
>> +
>> +	/* Restart the codec before setup */
>> +	davinci_vcif_stop(substream);
>> +	davinci_vcif_start(substream);
> 
> This seems a bit odd - I'd expect to see the start at the end of the
> function if you need to do this, otherwise the interface will be running
> before you've configured it?
[MA] The voice codec interface should be restarted before set the hw params. If 
you don't do this you will get underrun and overrun errors due to lack of the 
restarting process. Thats why I do a stop and then a start. I tried to include 
the prepare function however it is called after the hw params function, and that 
doesn't make sense.
> 
>> +	/* Determine xfer data type */
>> +	switch (params_format(params)) {
>> +	case SNDRV_PCM_FORMAT_U8:
>> +		dma_params->data_type = 0;
>> +
>> +		MOD_REG_BIT(w, DAVINCI_VCIF_CTRL_RD_BITS_8 |
>> +					DAVINCI_VCIF_CTRL_RD_UNSIGNED |
>> +					DAVINCI_VCIF_CTRL_WD_BITS_8 |
>> +					DAVINCI_VCIF_CTRL_WD_UNSIGNED, 1);
>> +		break;
> 
> These look like the interface needs to be configured the same way in
> both directions?  If that is the case then set symmetric_rates in the
> DAI.
[MA]I think it should be symmetric, so the TX and RX should be configured in the 
same way.
> 
>> +	.playback = {
>> +		.channels_min = 1,
>> +		.channels_max = 2,
>> +		.rates = DAVINCI_VCIF_RATES,
>> +		.formats = SNDRV_PCM_FMTBIT_S16_LE,},
> 
> You had options for handling more formats above, shouldn't they be
> included here?
[MA] I'll check this.
> 
>> +static struct platform_driver davinci_vcif_driver = {
>> +	.probe		= davinci_vcif_probe,
>> +	.remove		= davinci_vcif_remove,
>> +	.driver		= {
>> +		.name	= "voice_codec",
>> +		.owner	= THIS_MODULE,
>> +	},
>> +};
> 
> Might "vcif" or "davinci-vcif" be a better name?
[MA]To use that name I should change the name of the clock, however that clock 
is actually for the whole voice codec module, both voice codec and voice codec 
interface. The voice codec interface is just a logical separation of the voice 
codec module.

Thanks,

Miguel Aguilar

Re: [PATCH 2/4] ASoC: DaVinci: Voice Codec Interface

[Mark Brown]

On Wed, Sep 23, 2009 at 09:18:42AM -0600, Miguel Aguilar wrote:
> Mark Brown wrote:
> > On Tue, Sep 22, 2009 at 01:29:20PM -0600, miguel.aguilar@ridgerun.com wrote:

> >> 2) Add an option to select internal or external codec in the DM365.

> > Can you not do both simultaneously?  This should probably be a separate

> [MA] No external and internal codecs can not coexists since they share the same 
> DMA channels for TX and RX, so the TI recommendation was choose one codec or the 
> other one by the configuration menu.

That'd stop you using both simultaneously but it shouldn't stop you
having both compiled into the kernel simultaneously.  Would it be
difficult to make the DMA driver do something like return -EBUSY if it
was started opened twice?

> Actually, This patch series have a separate patch for the driver, vcif, SoC 
> specific code, and EVM specific code.

You should move the Kconfig stuff for those into the relevant patches.

> >> +	/* Restart the codec before setup */
> >> +	davinci_vcif_stop(substream);
> >> +	davinci_vcif_start(substream);

> > This seems a bit odd - I'd expect to see the start at the end of the
> > function if you need to do this, otherwise the interface will be running
> > before you've configured it?

> [MA] The voice codec interface should be restarted before set the hw params. If 
> you don't do this you will get underrun and overrun errors due to lack of the 
> restarting process. Thats why I do a stop and then a start. I tried to include 
> the prepare function however it is called after the hw params function, and that 
> doesn't make sense.

What exactly is the start bit doing?  The need to restart doesn't seem
so surprising, what seemed surprising was that you start the device
running again before it's been reconfigured - I'd have expected to see a
stop at the head of the function and then the start at the end after all
the new parameters had been done.

> > These look like the interface needs to be configured the same way in
> > both directions?  If that is the case then set symmetric_rates in the
> > DAI.

> [MA]I think it should be symmetric, so the TX and RX should be configured in the 
> same way.

In that case you should set symmetric_rates in the DAI - this will mean
that the core will tell applications about the need to keep symmetric
rates, meaning that you don't get attempts to configure the one
direction when the other is active.

> > 
> >> +	.driver		= {
> >> +		.name	= "voice_codec",
> >> +		.owner	= THIS_MODULE,
> >> +	},
> >> +};

> > Might "vcif" or "davinci-vcif" be a better name?
> [MA]To use that name I should change the name of the clock, however that clock 
> is actually for the whole voice codec module, both voice codec and voice codec 
> interface. The voice codec interface is just a logical separation of the voice 
> codec module.

The name of the device should have no influence on the name of the clock
- the clock API should be able to 

I suspect that you do need a little MFD here, it sounds like all the
resources need to be allocated to a single device which can then dole
them out to the CODEC and DAI drivers.

Re: [PATCH 2/4] ASoC: DaVinci: Voice Codec Interface

[Miguel Aguilar]

Mark Brown wrote:
> On Wed, Sep 23, 2009 at 09:18:42AM -0600, Miguel Aguilar wrote:
>> Mark Brown wrote:
>>> On Tue, Sep 22, 2009 at 01:29:20PM -0600, miguel.aguilar@ridgerun.com wrote:
> 
>>>> 2) Add an option to select internal or external codec in the DM365.
> 
>>> Can you not do both simultaneously?  This should probably be a separate
> 
>> [MA] No external and internal codecs can not coexists since they share the same 
>> DMA channels for TX and RX, so the TI recommendation was choose one codec or the 
>> other one by the configuration menu.
> 
> That'd stop you using both simultaneously but it shouldn't stop you
> having both compiled into the kernel simultaneously.  Would it be
> difficult to make the DMA driver do something like return -EBUSY if it
> was started opened twice?
[MA] But how I can tell the kernel to use one or the other one in runtime?, and 
Why do we need to have both compiled at the same time? if this is really needed 
What is the best way to that?
> 
>> Actually, This patch series have a separate patch for the driver, vcif, SoC 
>> specific code, and EVM specific code.
> 
> You should move the Kconfig stuff for those into the relevant patches.
[MA] ok I see.
> 
>>>> +	/* Restart the codec before setup */
>>>> +	davinci_vcif_stop(substream);
>>>> +	davinci_vcif_start(substream);
> 
>>> This seems a bit odd - I'd expect to see the start at the end of the
>>> function if you need to do this, otherwise the interface will be running
>>> before you've configured it?
> 
>> [MA] The voice codec interface should be restarted before set the hw params. If 
>> you don't do this you will get underrun and overrun errors due to lack of the 
>> restarting process. Thats why I do a stop and then a start. I tried to include 
>> the prepare function however it is called after the hw params function, and that 
>> doesn't make sense.
> 
> What exactly is the start bit doing?  The need to restart doesn't seem
> so surprising, what seemed surprising was that you start the device
> running again before it's been reconfigured - I'd have expected to see a
> stop at the head of the function and then the start at the end after all
> the new parameters had been done.
[MA] Ok I see your point I'll do it in that way.
> 
>>> These look like the interface needs to be configured the same way in
>>> both directions?  If that is the case then set symmetric_rates in the
>>> DAI.
> 
>> [MA]I think it should be symmetric, so the TX and RX should be configured in the 
>> same way.
> 
> In that case you should set symmetric_rates in the DAI - this will mean
> that the core will tell applications about the need to keep symmetric
> rates, meaning that you don't get attempts to configure the one
> direction when the other is active.
[MA] Ok I'll do that.
> 
>>>> +	.driver		= {
>>>> +		.name	= "voice_codec",
>>>> +		.owner	= THIS_MODULE,
>>>> +	},
>>>> +};
> 
>>> Might "vcif" or "davinci-vcif" be a better name?
>> [MA]To use that name I should change the name of the clock, however that clock 
>> is actually for the whole voice codec module, both voice codec and voice codec 
>> interface. The voice codec interface is just a logical separation of the voice 
>> codec module.
> 
> The name of the device should have no influence on the name of the clock
> - the clock API should be able to 
The problem is that clock name is the same as the device, by using pdev->name, 
most of the drivers does it in this way.
> 
> I suspect that you do need a little MFD here, it sounds like all the
> resources need to be allocated to a single device which can then dole
> them out to the CODEC and DAI drivers.
[MA] I haven't use MFD, can you bring me more details.

Do you mean create just one device with the whole voice codec instead of use 
vcif and cq0093 separately?

> 


Thanks,

Miguel Aguilar

Re: [PATCH 2/4] ASoC: DaVinci: Voice Codec Interface

[Mark Brown]

On Wed, Sep 23, 2009 at 10:09:57AM -0600, Miguel Aguilar wrote:
> Mark Brown wrote:

>> That'd stop you using both simultaneously but it shouldn't stop you
>> having both compiled into the kernel simultaneously.  Would it be
>> difficult to make the DMA driver do something like return -EBUSY if it
>> was started opened twice?

> [MA] But how I can tell the kernel to use one or the other one in 
> runtime?, and Why do we need to have both compiled at the same time? if 
> this is really needed What is the best way to that?

The kernel would just expose both devices to user space so the choice
would be down to the application layer.  This would give maximum
flexibility to systems, allowing them to chop and change at runtime if
they needed to for some reason (eg, using a lower quality but lower
power voice output for telephony style applications but a higer quality
higher power output for MP3 playback).  It'd also mean that for the EVM
the user would be able to use a single kernel image to evaluate both
outputs which makes life easier for things like distros.

The implementation I suggested above with returning EBUSY if the DMA is
already in use would probably work.

>> The name of the device should have no influence on the name of the clock
>> - the clock API should be able to 

> The problem is that clock name is the same as the device, by using 
> pdev->name, most of the drivers does it in this way.

The DaVinci API doesn't need that - you should be able to use a fixed
string (or even a null string) for the clock name and have the clock API
figure things out based on the struct device.  Look at how the ASP is
set up for an example.

>> I suspect that you do need a little MFD here, it sounds like all the
>> resources need to be allocated to a single device which can then dole
>> them out to the CODEC and DAI drivers.

> [MA] I haven't use MFD, can you bring me more details.

> Do you mean create just one device with the whole voice codec instead of 
> use vcif and cq0093 separately?

Sort of - see drivers/mfd for examples.  You have a device driver that
is instantiated by the platform and has all the resources allocated to
it.  It then manages allocating the resources to subdevices that do the
actual work and ensuring that the subdevices don't step on each other.

>
>>
>
>
> Thanks,
>
> Miguel Aguilar

Handling two audio codec in the same kernel

[Miguel Aguilar]

Hi Mark,

I have been working on developing the voice codec driver for the davinci 
platforms, in particularly for the DM365 EVM. This board has one AIC codec and 
one voice codec.

Some time ago I sent the initial patch for adding support for the Voice codec. 
You suggested me that I should register in the case of DM365 EVM both codecs in 
the same kernel (AIC and Voice Codec):

>> [MA] But how I can tell the kernel to use one or the other one in 
>> runtime?, and Why do we need to have both compiled at the same time? if 
>> this is really needed What is the best way to that?
> 
> The kernel would just expose both devices to user space so the choice
> would be down to the application layer.  This would give maximum
> flexibility to systems, allowing them to chop and change at runtime if
> they needed to for some reason (eg, using a lower quality but lower
> power voice output for telephony style applications but a higer quality
> higher power output for MP3 playback).  It'd also mean that for the EVM
> the user would be able to use a single kernel image to evaluate both
> outputs which makes life easier for things like distros.
> 

There was another problem because both codecs share the same DMA channels, 
however this it was solved by following your suggestion:

> The implementation I suggested above with returning EBUSY if the DMA is
> already in use would probably work.
> 

At this point the both drivers AIC and Voice Codec are implemented, and also the 
logic to handle the dma channels properly.

I have some questions:

* How is the proper way to register both codecs in the same kernel?
* If I have registered in the same kernel, How I can choose the codec that will 
be used in the alsamixer menu?

At this point I have added the following structures to the 
sound/soc/davinci/davinci-evm.c file:

static struct snd_soc_dai_link dm365_evm_dai[] = {
	{
		.name = "TLV320AIC3X",
		.stream_name = "AIC3X",
		.cpu_dai = &davinci_i2s_dai,
		.codec_dai = &aic3x_dai,
		.init = evm_aic3x_init,
		.ops = &evm_ops,
	},
	{
		.name = "Voice Codec - CQ93VC",
		.stream_name = "CQ93",
		.cpu_dai = &davinci_vcif_dai,
		.codec_dai = &cq93vc_dai,
	},
};

/* davinci dm365 evm audio machine driver external or internal */
static struct snd_soc_card dm365_snd_soc_card_evm = {
	.name = "DaVinci DM365 EVM",
	.platform = &davinci_soc_platform,
	.dai_link = dm365_evm_dai,
	.num_links = ARRAY_SIZE(dm365_evm_dai),
};

/* evm audio subsystem */
static struct snd_soc_device dm365_evm_snd_devdata = {
	.card = &dm365_snd_soc_card_evm,
	.codec_dev = &soc_codec_dev_aic3x,  // Only codec_dev at time?
	.codec_data = &aic3x_setup,
};

This is the output:
...
 > Advanced Linux Sound Architecture Driver Version 1.0.21.
 > No device for DAI tlv320aic3x
 > No device for DAI davinci-i2s
 > asoc: tlv320aic3x <-> davinci-i2s mapping ok
 > asoc: CQ93VC <-> davinci-vcif mapping ok
 > ALSA device list:
 >   #0: DaVinci DM365 EVM (tlv320aic3x)
 > ...


root@dm365-evm:/apps# arecord -D 'plughw:0,0'| aplay -D 'plughw:0,0' &
Recording WAVE 'stdin' : Unsigned 8 bit, Rate 8000 Hz, Mono
Playing WAVE 'stdin' : Unsigned 8 bit, Rate 8000 Hz, Mono

root@dm365-evm:/apps# arecord -D 'plughw:0,1'| aplay -D 'plughw:0,1'
davinci_pcm: Failed to get dma channels
asoc: can't open platform davinci-audio
arecord: main:608: audio open error: Device or resource busy
davinci_pcm: Failed to get dma channels
asoc: can't open platform davinci-audio
aplay: main:608: audio open error: Device or resource busy

root@dm365-evm:/apps# arecord -D 'plughw:0,1'| aplay -D 'plughw:0,1' &
Recording WAVE 'stdin' : Unsigned 8 bit, Rate 8000 Hz, Mono
Playing WAVE 'stdin' : Unsigned 8 bit, Rate 8000 Hz, Mono

root@dm365-evm:/apps# arecord -D 'plughw:0,0'| aplay -D 'plughw:0,0'
davinci_pcm: Failed to get dma channels
asoc: can't open platform davinci-audio
davinci_pcm: Failed to get dma channels
arecord: main:60asoc: can't open platform davinci-audio
8: audio open error: Device or resource busy
aplay: main:608: audio open error: Device or resource busy

You can do different combinations as well -
root@dm365-evm:/apps# arecord -D 'plughw:0,0'| aplay -D 'plughw:0,1'
Recording WAVE 'stdin' : Unsigned 8 bit, Rate 8000 Hz, Mono
Playing WAVE 'stdin' : Unsigned 8 bit, Rate 8000 Hz, Mono

root@dm365-evm:/apps# arecord -D 'plughw:0,1'| aplay -D 'plughw:0,0'
Recording WAVE 'stdin' : Unsigned 8 bit, ate 8000 Hz, Mono
Playing WAVE 'stdin' : Unsigned 8 bit, Rate 8000 Hz, Mono

The DMA logic works fine as you see in the examples above. However, only the AIC 
works recording/playback, the voice codec doesn't work. If I switch the default 
codec to Voice Codec it works.

Thanks,
Miguel Aguilar

Re: Handling two audio codec in the same kernel

[Mark Brown]

On Tue, Nov 24, 2009 at 10:37:41AM -0600, Miguel Aguilar wrote:

> * How is the proper way to register both codecs in the same kernel?

At the minute you can't really.  However, if the CODEC for the voice
interface is mostly empty (which I seem to recall was the case?) then
you can skip registering the CODEC and only register the DAI.

> * If I have registered in the same kernel, How I can choose the codec 
> that will be used in the alsamixer menu?

The application layer will do this based on which DAI it chooses to play
audio down - each will appear separately to them, as you're doing below.

> The DMA logic works fine as you see in the examples above. However, only 
> the AIC works recording/playback, the voice codec doesn't work. If I 
> switch the default codec to Voice Codec it works.

Initially I'd suggest testing with hw rather than plughw to make sure
the test is isolated to the drivers.

RE: Handling two audio codec in the same kernel

[Narnakaje, Snehaprabha]

> -----Original Message-----
> From: Mark Brown [mailto:broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org]
> Sent: Wednesday, November 25, 2009 6:21 AM
> To: Miguel Aguilar
> Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org;
> clark.becker-9uBrGCPFOa1Wk0Htik3J/w@public.gmane.org; santiago.nunez-9uBrGCPFOa1Wk0Htik3J/w@public.gmane.org;
> diego.dompe-9uBrGCPFOa1Wk0Htik3J/w@public.gmane.org; alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org; Narnakaje,
> Snehaprabha; todd.fischer-9uBrGCPFOa1Wk0Htik3J/w@public.gmane.org
> Subject: Re: Handling two audio codec in the same kernel
> 
> On Tue, Nov 24, 2009 at 10:37:41AM -0600, Miguel Aguilar wrote:
> 
> > * How is the proper way to register both codecs in the same kernel?
> 
> At the minute you can't really.  However, if the CODEC for the voice
> interface is mostly empty (which I seem to recall was the case?) then
> you can skip registering the CODEC and only register the DAI.

Yes, voice codec does not call the snd_soc_register_codec handler. But it does call the snd_soc_register_dai handler. However we have the snd_soc_codec_device structure defined for the voice codec. This structure initializes the probe, remove, suspend and resume handlers for the voice codec. This probe function initializes the voice codec peripheral, powers it on and adds the controls for amixer. However the probe function does not get invoked since we have configured AIC3101 codec as the default codec for .codec_dev in the snd_soc_device structure. Some how, we have to invoke the voice codec probe function. Not sure, if the evm_init() in sound/soc/davinci/davinci-evm.c is the right place to call soc_codec_dev_cq93vc.probe(). It seems to work if I call the .probe() in evm_init().

It looks like we also need to comment out snd_soc_init_card() in the voice codec probe function, since the card would already have registered by the AIC3101 codec.

Question related to amixer - should we be seeing controls for both the codecs? How do we change the gain settings specific to a codec?

Thanks
Sneha

> 
> > * If I have registered in the same kernel, How I can choose the codec
> > that will be used in the alsamixer menu?
> 
> The application layer will do this based on which DAI it chooses to play
> audio down - each will appear separately to them, as you're doing below.
> 
> > The DMA logic works fine as you see in the examples above. However, only
> > the AIC works recording/playback, the voice codec doesn't work. If I
> > switch the default codec to Voice Codec it works.
> 
> Initially I'd suggest testing with hw rather than plughw to make sure
> the test is isolated to the drivers.