Re: [PATCH 1/5] ASoC: DMIC: Adding the OMAP DMIC driver

Felipe Balbi
2010-12-28T11:14:29Z

Hi,

On Mon, Dec 27, 2010 at 10:17:02PM -0600, David Lambert wrote:
>This patch adds support for the OMAP4 digital microphone DAI.
>
>This DAI can support support recording in 2, 4, or 6 channels
>
>When provided with a 19.2Mhz functional clock, can encode at 96Khz or
>192Khz (all
>channels must have the same sample rate).
>
>Details of the hardware interface can be found in the OMAP4 TRM in Section 23.7
>
>Signed-off-by: David Lambert <dlambert@ti.com>
>---
> arch/arm/plat-omap/include/plat/dmic.h | 83 +++++
> sound/soc/omap/omap-dmic.c | 596 ++++++++++++++++++++++++++++++++
> sound/soc/omap/omap-dmic.h | 38 ++
> 3 files changed, 717 insertions(+), 0 deletions(-)
> create mode 100644 arch/arm/plat-omap/include/plat/dmic.h
> create mode 100644 sound/soc/omap/omap-dmic.c
> create mode 100644 sound/soc/omap/omap-dmic.h
>
>diff --git a/arch/arm/plat-omap/include/plat/dmic.h b/arch/arm/plat-omap/include/plat/dmic.h
>new file mode 100644
>index 0000000..8a988bf
>--- /dev/null
>+++ b/arch/arm/plat-omap/include/plat/dmic.h
>@@ -0,0 +1,83 @@
>+/*
>+ * omap-dmic.h -- OMAP Digital Microphone Controller
>+ *
>+ * Author: Liam Girdwood <lrg@slimlogic.co.uk>
>+ * David Lambert <dlambert@ti.com>
>+ * Misael Lopez Cruz <misael.lopez@ti.com>
>+ *
>+ * 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.
>+ */
>+
>+#ifndef __ASM_ARCH_OMAP_DMIC_H
>+#define __ASM_ARCH_OMAP_DMIC_H
>+
>+#define OMAP44XX_DMIC_L3_BASE 0x4902e000
>+
>+#define OMAP_DMIC_REVISION 0x00
>+#define OMAP_DMIC_SYSCONFIG 0x10
>+#define OMAP_DMIC_IRQSTATUS_RAW 0x24
>+#define OMAP_DMIC_IRQSTATUS 0x28
>+#define OMAP_DMIC_IRQENABLE_SET 0x2C
>+#define OMAP_DMIC_IRQENABLE_CLR 0x30
>+#define OMAP_DMIC_IRQWAKE_EN 0x34
>+#define OMAP_DMIC_DMAENABLE_SET 0x38
>+#define OMAP_DMIC_DMAENABLE_CLR 0x3C
>+#define OMAP_DMIC_DMAWAKEEN 0x40
>+#define OMAP_DMIC_CTRL 0x44
>+#define OMAP_DMIC_DATA 0x48
>+#define OMAP_DMIC_FIFO_CTRL 0x4C
>+#define OMAP_DMIC_FIFO_DMIC1R_DATA 0x50
>+#define OMAP_DMIC_FIFO_DMIC1L_DATA 0x54
>+#define OMAP_DMIC_FIFO_DMIC2R_DATA 0x58
>+#define OMAP_DMIC_FIFO_DMIC2L_DATA 0x5C
>+#define OMAP_DMIC_FIFO_DMIC3R_DATA 0x60
>+#define OMAP_DMIC_FIFO_DMIC3L_DATA 0x64
>+
>+/*
>+ * DMIC_IRQ bit fields
>+ * IRQSTATUS_RAW, IRQSTATUS, IRQENABLE_SET, IRQENABLE_CLR
>+ */
>+
>+#define OMAP_DMIC_IRQ (1 << 0)
>+#define OMAP_DMIC_IRQ_FULL (1 << 1)
>+#define OMAP_DMIC_IRQ_ALMST_EMPTY (1 << 2)
>+#define OMAP_DMIC_IRQ_EMPTY (1 << 3)
>+#define OMAP_DMIC_IRQ_MASK 0x07
>+
>+/*
>+ * DMIC_DMAENABLE bit fields
>+ */
>+
>+#define OMAP_DMIC_DMA_ENABLE 0x1
>+
>+/*
>+ * DMIC_CTRL bit fields
>+ */
>+
>+#define OMAP_DMIC_UP1_ENABLE 0x0001
>+#define OMAP_DMIC_UP2_ENABLE 0x0002
>+#define OMAP_DMIC_UP3_ENABLE 0x0004
>+#define OMAP_DMIC_FORMAT 0x0008
>+#define OMAP_DMIC_POLAR1 0x0010
>+#define OMAP_DMIC_POLAR2 0x0020
>+#define OMAP_DMIC_POLAR3 0x0040
>+#define OMAP_DMIC_POLAR_MASK 0x0070
>+#define OMAP_DMIC_CLK_DIV_SHIFT 7
>+#define OMAP_DMIC_CLK_DIV_MASK 0x0380
>+#define OMAP_DMIC_RESET 0x0400
>+
>+#define OMAP_DMIC_ENABLE_MASK 0x007
>+
>+#define OMAP_DMICOUTFORMAT_LJUST (0 << 3)
>+#define OMAP_DMICOUTFORMAT_RJUST (1 << 3)
>+
>+/*
>+ * DMIC_FIFO_CTRL bit fields
>+ */
>+
>+#define OMAP_DMIC_THRES_MAX 0xF
>+
>+

one blank line only. BTW, are these used anywwhere outside the dmic.c
driver ? If not, it's better to move the definitions there.

>+#endif
>diff --git a/sound/soc/omap/omap-dmic.c b/sound/soc/omap/omap-dmic.c
>new file mode 100644
>index 0000000..6ba05bd
>--- /dev/null
>+++ b/sound/soc/omap/omap-dmic.c
>@@ -0,0 +1,596 @@
>+/*
>+ * omap-dmic.c -- OMAP ASoC DMIC DAI driver
>+ *
>+ * Copyright (C) 2010 Texas Instruments
>+ *
>+ * Author: Liam Girdwood <lrg@slimlogic.co.uk>
>+ * David Lambert <dlambert@ti.com>
>+ * Misael Lopez Cruz <misael.lopez@ti.com>
>+ *
>+ * 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.
>+ *
>+ * 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., 51 Franklin St, Fifth Floor, Boston, MA
>+ * 02110-1301 USA
>+ *
>+ */
>+
>+#undef DEBUG
>+
>+#include <linux/init.h>
>+#include <linux/module.h>
>+#include <linux/platform_device.h>
>+#include <linux/wait.h>
>+#include <linux/interrupt.h>
>+#include <linux/err.h>
>+#include <linux/clk.h>
>+#include <linux/delay.h>
>+#include <linux/io.h>
>+#include <linux/irq.h>
>+#include <linux/slab.h>
>+#include <linux/pm_runtime.h>
>+
>+#include <plat/control.h>
>+#include <plat/dma.h>
>+#include <plat/dmic.h>
>+#include <plat/omap_hwmod.h>

I'm not sure drivers should be including these (besides dmic.h which you
added). plat/control.h doesn't even exist anymore and hwmod is supposed
to live under mach-omap*/ only.

>+#include <sound/core.h>
>+#include <sound/pcm.h>
>+#include <sound/pcm_params.h>
>+#include <sound/initval.h>
>+#include <sound/soc.h>
>+
>+#include "omap-dmic.h"
>+#include "omap-pcm.h"

These little guys here look rather small to deserve being split to
separate files. Also, it looks like they're only used by this driver
(??) so it's better to move the definitions to this C source file
instead.

>+static struct omap_dmic_link omap_dmic_link = {

const ?

>+ .irq_mask = OMAP_DMIC_IRQ_EMPTY | OMAP_DMIC_IRQ_FULL,
>+ .threshold = 2,
>+ .format = OMAP_DMICOUTFORMAT_LJUST,
>+ .polar = OMAP_DMIC_POLAR1 | OMAP_DMIC_POLAR2
>+ | OMAP_DMIC_POLAR3,
>+};
>+
>+/*
>+ * Stream DMA parameters
>+ */
>+static struct omap_pcm_dma_data omap_dmic_dai_dma_params = {

const ?

>+ .name = "DMIC capture",
>+ .data_type = OMAP_DMA_DATA_TYPE_S32,
>+ .sync_mode = OMAP_DMA_SYNC_PACKET,
>+ .packet_size = 2,
>+ .port_addr = OMAP44XX_DMIC_L3_BASE + OMAP_DMIC_DATA,
>+};
>+
>+

one blank line only.

>+#ifdef DEBUG
>+static void omap_dmic_reg_dump(struct omap_dmic *dmic)
>+{
>+ dev_dbg(dmic->dev, "***********************\n");
>+ dev_dbg(dmic->dev, "OMAP_DMIC_IRQSTATUS_RAW: 0x%04x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_IRQSTATUS_RAW));
>+ dev_dbg(dmic->dev, "OMAP_DMIC_IRQSTATUS: 0x%04x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_IRQSTATUS));
>+ dev_dbg(dmic->dev, "OMAP_DMIC_IRQENABLE_SET: 0x%04x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_IRQENABLE_SET));
>+ dev_dbg(dmic->dev, "OMAP_DMIC_IRQENABLE_CLR: 0x%04x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_IRQENABLE_CLR));
>+ dev_dbg(dmic->dev, "OMAP_DMIC_IRQWAKE_EN: 0x%04x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_IRQWAKE_EN));
>+ dev_dbg(dmic->dev, "OMAP_DMIC_DMAENABLE_SET: 0x%04x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_DMAENABLE_SET));
>+ dev_dbg(dmic->dev, "OMAP_DMIC_DMAENABLE_CLR: 0x%04x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_DMAENABLE_CLR));
>+ dev_dbg(dmic->dev, "OMAP_DMIC_DMAWAKEEN: 0x%04x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_DMAWAKEEN));
>+ dev_dbg(dmic->dev, "OMAP_DMIC_CTRL: 0x%04x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_CTRL));
>+ dev_dbg(dmic->dev, "OMAP_DMIC_DATA: 0x%04x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_DATA));
>+ dev_dbg(dmic->dev, "OMAP_DMIC_FIFO_CTRL: 0x%04x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_FIFO_CTRL));
>+ dev_dbg(dmic->dev, "OMAP_DMIC_FIFO_DMIC1R_DATA: 0x%08x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_FIFO_DMIC1R_DATA));
>+ dev_dbg(dmic->dev, "OMAP_DMIC_FIFO_DMIC1L_DATA: 0x%08x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_FIFO_DMIC1L_DATA));
>+ dev_dbg(dmic->dev, "OMAP_DMIC_FIFO_DMIC2R_DATA: 0x%08x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_FIFO_DMIC2R_DATA));
>+ dev_dbg(dmic->dev, "OMAP_DMIC_FIFO_DMIC2L_DATA: 0x%08x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_FIFO_DMIC2L_DATA));
>+ dev_dbg(dmic->dev, "OMAP_DMIC_FIFO_DMIC3R_DATA: 0x%08x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_FIFO_DMIC3R_DATA));
>+ dev_dbg(dmic->dev, "OMAP_DMIC_FIFO_DMIC3L_DATA: 0x%08x\n",
>+ omap_dmic_read(dmic, OMAP_DMIC_FIFO_DMIC3L_DATA));
>+ dev_dbg(dmic->dev, "***********************\n");
>+}
>+#else
>+static void omap_dmic_reg_dump(struct omap_dmic *dmic) {}
>+#endif

Would be better to make a debugfs layer ??

>+static irqreturn_t omap_dmic_irq_handler(int irq, void *dev_id)
>+{
>+ struct omap_dmic *dmic = dev_id;
>+ u32 irq_status;
>+
>+ irq_status = omap_dmic_read(dmic, OMAP_DMIC_IRQSTATUS);
>+
>+ /* Acknowledge irq event */
>+ omap_dmic_write(dmic, OMAP_DMIC_IRQSTATUS, irq_status);
>+ if (irq_status & OMAP_DMIC_IRQ_FULL)
>+ dev_dbg(dmic->dev, "DMIC FIFO error %x\n", irq_status);
>+
>+ if (irq_status & OMAP_DMIC_IRQ_EMPTY)
>+ dev_dbg(dmic->dev, "DMIC FIFO error %x\n", irq_status);
>+
>+ if (irq_status & OMAP_DMIC_IRQ)
>+ dev_dbg(dmic->dev, "DMIC write request\n");

no locking needed ??

>+static int omap_dmic_dai_startup(struct snd_pcm_substream *substream,
>+ struct snd_soc_dai *dai)
>+{
>+ struct omap_dmic *dmic = snd_soc_dai_get_drvdata(dai);
>+
>+ if (!dmic->active++)
>+ pm_runtime_get_sync(dmic->dev);
>+
>+ return 0;
>+}
>+
>+static void omap_dmic_dai_shutdown(struct snd_pcm_substream *substream,
>+ struct snd_soc_dai *dai)
>+{
>+ struct omap_dmic *dmic = snd_soc_dai_get_drvdata(dai);
>+
>+ if (!--dmic->active)
>+ pm_runtime_put_sync(dmic->dev);

I could be wrong but I believe pm_runtime implementation on OMAP has
its own refcounting, so you could drop the need for dmic->active.

>+static struct snd_soc_dai_ops omap_dmic_dai_ops = {

should this be const ?

>+static struct snd_soc_dai_driver omap_dmic_dai = {

this too ??

>+static __devinit int asoc_dmic_probe(struct platform_device *pdev)
>+{
>+ struct omap_dmic *dmic;
>+ struct resource *res;
>+ int ret;
>+
>+ dmic = kzalloc(sizeof(struct omap_dmic), GFP_KERNEL);
>+ if (!dmic)
>+ return -ENOMEM;
>+
>+ platform_set_drvdata(pdev, dmic);
>+ dmic->dev = &pdev->dev;
>+ dmic->link = &omap_dmic_link;
>+ dmic->sysclk = OMAP_DMIC_SYSCLK_SYNC_MUX_CLKS;
>+
>+ spin_lock_init(&dmic->lock);
>+
>+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>+ if (!res) {
>+ dev_err(dmic->dev, "invalid memory resource\n");
>+ ret = -ENODEV;
>+ goto err_res;
>+ }
>+
>+ dmic->io_base = ioremap(res->start, resource_size(res));
>+ if (!dmic->io_base) {
>+ ret = -ENOMEM;
>+ goto err_res;
>+ }
>+
>+ dmic->irq = platform_get_irq(pdev, 0);
>+ if (dmic->irq < 0) {
>+ ret = dmic->irq;
>+ goto err_irq;
>+ }
>+
>+ res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
>+ if (!res) {
>+ dev_err(dmic->dev, "invalid dma resource\n");
>+ ret = -ENODEV;
>+ goto err_irq;
>+ }
>+ omap_dmic_dai_dma_params.dma_req = res->start;
>+
>+ pm_runtime_enable(dmic->dev);
>+
>+ /* Disable lines while request is ongoing */
>+ omap_dmic_write(dmic, OMAP_DMIC_CTRL, 0x00);
>+
>+ ret = request_threaded_irq(dmic->irq, NULL, omap_dmic_irq_handler,
>+ IRQF_ONESHOT, "DMIC", (void *)dmic);

Does this need to be threaded ? Doesn't look like. Also, you don't need
the (void *) cast.

>+static int __devexit asoc_dmic_remove(struct platform_device *pdev)
>+{
>+ struct omap_dmic *dmic = platform_get_drvdata(pdev);
>+
>+ snd_soc_unregister_dai(&pdev->dev);
>+ free_irq(dmic->irq, dmic);
>+ iounmap(dmic->io_base);
>+ kfree(dmic);

pm_runtime_disable() ??

--
balbi