On Tue, Dec 28, 2010 at 5:14 AM, Felipe Balbi <balbi@xxxxxx> wrote:
> 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@xxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx>
>> + * David Lambert <dlambert@xxxxxx>
>> + * Misael Lopez Cruz <misael.lopez@xxxxxx>
>> + *
>> + * 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.
>
They were originally in the omap-dmic.h header, but it was suggested
that we move
them to a platform header so that the driver could be more
architecture independent
and we could put the platform specific details here. I'm OK putting
them just about
anywhere, as long as we have consensus.
>> +#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@xxxxxxxxxxxxxxx>
>> + * David Lambert <dlambert@xxxxxx>
>> + * Misael Lopez Cruz <misael.lopez@xxxxxx>
>> + *
>> + * 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.
Agreed, see comment above about the location of the macros for the
registers. If we agree it belongs in the platform header, we can collapse
the content of omap-dmic.h in to the C source file. If we don't think they
belong in a platform header, they can come back here and then it won't
be quite so little.
>
>> +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 ??
I'll look in to what it would take to do this. Could this be a
feature to add later?
>
>> +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.
This was a included for a future use case where the driver would be
accessed by the
Audio Back End. In this case, there are multiple DAI's associated to
this driver and
we need to keep track of the number of active DAI's and only cal
pm_runtime_put_sync()
when there are no others running.
>
>> +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.
This was originally not a threaded IRQ handler. But in an internal
review, it was
suggested that this was the current trend in drivers and I should
follow that course.
I'm open to having it non-threaded.
>
>> +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
>
--
David Lambert
OMAP™ Multimedia
214-567-5692
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
