[alsa-devel] [PATCH 2/4] ASoC: DaVinci: Voice Codec Interface
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
On Tue, Sep 22, 2009 at 01:29:20PM -0600, miguel.aguilar@ridgerun.com wrote:
From: Miguel Aguilar miguel.aguilar@ridgerun.com
- 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.
- 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?
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
- 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.
- 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
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:
- 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.
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:
- 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
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
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
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.
participants (3)
-
Mark Brown -
Miguel Aguilar -
miguel.aguilar＠ridgerun.com