[PATCH v2 0/2] ASoC: omap-mcpdm: New McPDM driver

[PATCH v2 0/2] ASoC: omap-mcpdm: New McPDM driver

[Peter Ujfalusi]

Hello Mark, Liam,

I tried to address the comments regarding to version 1 of the new McPDM driver:
http://mailman.alsa-project.org/pipermail/alsa-devel/2011-July/041690.html

The omap hwmod conversion part of the v1 series has been sent as part of the
fixup series for 3.1 to make basic audio work out of box.

This series is aimed for 3.2 due to the scale of the change.
I tried to split patch 2, but I did not managed to find an incremental path from
the old driver to the new one.

I have updated the commit message for patch 2 to explain the reasoning behind
the rewritten driver, so it can be easily looked up.

Notable changes since v1:
- delayed_work from the new driver has been removed, and replaced with a
  DAPM_SUPPLY widget. This widget need to be connected to DAC/ADC of the codec
  by the machine driver in order to have the correct sequence for power off.
The first patch adds private_data pointer for dapm widgets. This private_data
can be used to add widget specific data to any widget. I think this can be handy
for others as well in the future.

- Full duplex audio in legacy mode has been fixed, so we can run arecord | aplay

This series depends on the McPDM fixup patches:
http://mailman.alsa-project.org/pipermail/alsa-devel/2011-August/042401.html

Peter

PS: to make the review easier, I have asked git format-patch to treat the change
as rewrite.

---
Misael Lopez Cruz (1):
  ASoC: omap-mcpdm: Replace legacy driver

Peter Ujfalusi (1):
  ASoC: DAPM: Add private data pointer for DAPM widget

 include/sound/soc-dapm.h    |    1 +
 sound/soc/omap/Makefile     |    2 +-
 sound/soc/omap/mcpdm.c      |  472 -------------------------
 sound/soc/omap/mcpdm.h      |  152 --------
 sound/soc/omap/omap-mcpdm.c |  823 ++++++++++++++++++++++++++++---------------
 sound/soc/omap/omap-mcpdm.h |   97 +++++
 sound/soc/omap/sdp4430.c    |   14 +-
 7 files changed, 656 insertions(+), 905 deletions(-)
 delete mode 100644 sound/soc/omap/mcpdm.c
 delete mode 100644 sound/soc/omap/mcpdm.h
 rewrite sound/soc/omap/omap-mcpdm.c (46%)
 create mode 100644 sound/soc/omap/omap-mcpdm.h

-- 
1.7.6

[PATCH v2 1/2] ASoC: DAPM: Add private data pointer for DAPM widget

[Peter Ujfalusi]

Support for using custom private data assigned
per DAPM widget.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 include/sound/soc-dapm.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index 602024d..84114e7 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -467,6 +467,7 @@ struct snd_soc_dapm_widget {
 	unsigned char force:1;			/* force state */
 	unsigned char ignore_suspend:1;         /* kept enabled over suspend */
 	int subseq;				/* sort within widget type */
+	void *private_data;
 
 	int (*power_check)(struct snd_soc_dapm_widget *w);
 
-- 
1.7.6

Re: [PATCH v2 1/2] ASoC: DAPM: Add private data pointer for DAPM widget

[Mark Brown]

On Fri, Aug 19, 2011 at 10:41:18AM +0300, Peter Ujfalusi wrote:
> Support for using custom private data assigned
> per DAPM widget.

If we're adding this we should probably go the accessor route like we do
with the driver data for the CODECs.

Re: [PATCH v2 1/2] ASoC: DAPM: Add private data pointer for DAPM widget

[Péter Ujfalusi]

On Saturday 20 August 2011 09:02:20 Mark Brown wrote:
> On Fri, Aug 19, 2011 at 10:41:18AM +0300, Peter Ujfalusi wrote:
> > Support for using custom private data assigned
> > per DAPM widget.
> 
> If we're adding this we should probably go the accessor route like we do
> with the driver data for the CODECs.

Something like:

static inline void *snd_soc_widget_get_pdata(struct snd_soc_dapm_widget *w)
{
        return w->private_data;
}

static inline void snd_soc_widget_set_pdata(struct snd_soc_dapm_widget *w,
			void *data)
{
        w->private_data = data;
}

Another option at the moment is that I create global variable within the omap-
mcpdm.c, so I can have access to the mcpdm within the dapm event handler.

-- 
Péter

Re: [PATCH v2 1/2] ASoC: DAPM: Add private data pointer for DAPM widget

[Mark Brown]

On Mon, Aug 22, 2011 at 04:33:20PM +0300, Péter Ujfalusi wrote:
> On Saturday 20 August 2011 09:02:20 Mark Brown wrote:

> > If we're adding this we should probably go the accessor route like we do
> > with the driver data for the CODECs.

> Something like:

> static inline void *snd_soc_widget_get_pdata(struct snd_soc_dapm_widget *w)
> {
>         return w->private_data;
> }

Yeah, something like that.

> Another option at the moment is that I create global variable within the omap-
> mcpdm.c, so I can have access to the mcpdm within the dapm event handler.

That'll probably break in future I'd guess.

[PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Peter Ujfalusi]

From: Misael Lopez Cruz <misael.lopez@ti.com>

Reasons for the replacement:
The current driver for McPDM was developed to support the legacy mode only.
In preparation for the ABE support the current driver stack need the be
replaced.
The new driver is much simpler, easier to extend, and it also fixes some of the
issues with the old stack.

Main changes:
- single file for omap-mcpdm (mcpdm.c/h removed)
- Define names for registers, bits cleaned up, prefixed
- Full-duplex audio operation (arecord | aplay) has been fixed
- Less code

McPDM need to be turned off _after_ the codec's DAC/ADC has been powered down.
The solution for this without extensive change in core:
Use DAPM_SUPPLY to turn off the McPDM interface.
The machine driver connects the "OMAP McPDM Interface" to the codec's DAC/ADC,
so after audio activity we can be sure that the host side McPDM is turned off
at the correct time.

Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
Signed-off-by: Liam Girdwood <lrg@ti.com>
Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/omap/Makefile     |    2 +-
 sound/soc/omap/mcpdm.c      |  472 -------------------------
 sound/soc/omap/mcpdm.h      |  152 --------
 sound/soc/omap/omap-mcpdm.c |  823 ++++++++++++++++++++++++++++---------------
 sound/soc/omap/omap-mcpdm.h |   97 +++++
 sound/soc/omap/sdp4430.c    |   14 +-
 6 files changed, 655 insertions(+), 905 deletions(-)
 delete mode 100644 sound/soc/omap/mcpdm.c
 delete mode 100644 sound/soc/omap/mcpdm.h
 rewrite sound/soc/omap/omap-mcpdm.c (46%)
 create mode 100644 sound/soc/omap/omap-mcpdm.h

diff --git a/sound/soc/omap/Makefile b/sound/soc/omap/Makefile
index 59e2c8d..052fd75 100644
--- a/sound/soc/omap/Makefile
+++ b/sound/soc/omap/Makefile
@@ -1,7 +1,7 @@
 # OMAP Platform Support
 snd-soc-omap-objs := omap-pcm.o
 snd-soc-omap-mcbsp-objs := omap-mcbsp.o
-snd-soc-omap-mcpdm-objs := omap-mcpdm.o mcpdm.o
+snd-soc-omap-mcpdm-objs := omap-mcpdm.o
 snd-soc-omap-hdmi-objs := omap-hdmi.o
 
 obj-$(CONFIG_SND_OMAP_SOC) += snd-soc-omap.o
diff --git a/sound/soc/omap/mcpdm.c b/sound/soc/omap/mcpdm.c
deleted file mode 100644
index 9746a49..0000000
--- a/sound/soc/omap/mcpdm.c
+++ /dev/null
@@ -1,472 +0,0 @@
-/*
- * mcpdm.c  --	McPDM interface driver
- *
- * Author: Jorge Eduardo Candelaria <x0107209@ti.com>
- * Copyright (C) 2009 - Texas Instruments, Inc.
- *
- * 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
- *
- */
-
-#include <linux/module.h>
-#include <linux/init.h>
-#include <linux/device.h>
-#include <linux/platform_device.h>
-#include <linux/wait.h>
-#include <linux/slab.h>
-#include <linux/interrupt.h>
-#include <linux/err.h>
-#include <linux/pm_runtime.h>
-#include <linux/delay.h>
-#include <linux/io.h>
-#include <linux/irq.h>
-
-#include "mcpdm.h"
-
-static struct omap_mcpdm *mcpdm;
-
-static inline void omap_mcpdm_write(u16 reg, u32 val)
-{
-	__raw_writel(val, mcpdm->io_base + reg);
-}
-
-static inline int omap_mcpdm_read(u16 reg)
-{
-	return __raw_readl(mcpdm->io_base + reg);
-}
-
-static void omap_mcpdm_reg_dump(void)
-{
-	dev_dbg(mcpdm->dev, "***********************\n");
-	dev_dbg(mcpdm->dev, "IRQSTATUS_RAW:  0x%04x\n",
-			omap_mcpdm_read(MCPDM_IRQSTATUS_RAW));
-	dev_dbg(mcpdm->dev, "IRQSTATUS:	0x%04x\n",
-			omap_mcpdm_read(MCPDM_IRQSTATUS));
-	dev_dbg(mcpdm->dev, "IRQENABLE_SET:  0x%04x\n",
-			omap_mcpdm_read(MCPDM_IRQENABLE_SET));
-	dev_dbg(mcpdm->dev, "IRQENABLE_CLR:  0x%04x\n",
-			omap_mcpdm_read(MCPDM_IRQENABLE_CLR));
-	dev_dbg(mcpdm->dev, "IRQWAKE_EN: 0x%04x\n",
-			omap_mcpdm_read(MCPDM_IRQWAKE_EN));
-	dev_dbg(mcpdm->dev, "DMAENABLE_SET: 0x%04x\n",
-			omap_mcpdm_read(MCPDM_DMAENABLE_SET));
-	dev_dbg(mcpdm->dev, "DMAENABLE_CLR:  0x%04x\n",
-			omap_mcpdm_read(MCPDM_DMAENABLE_CLR));
-	dev_dbg(mcpdm->dev, "DMAWAKEEN:	0x%04x\n",
-			omap_mcpdm_read(MCPDM_DMAWAKEEN));
-	dev_dbg(mcpdm->dev, "CTRL:	0x%04x\n",
-			omap_mcpdm_read(MCPDM_CTRL));
-	dev_dbg(mcpdm->dev, "DN_DATA:  0x%04x\n",
-			omap_mcpdm_read(MCPDM_DN_DATA));
-	dev_dbg(mcpdm->dev, "UP_DATA: 0x%04x\n",
-			omap_mcpdm_read(MCPDM_UP_DATA));
-	dev_dbg(mcpdm->dev, "FIFO_CTRL_DN: 0x%04x\n",
-			omap_mcpdm_read(MCPDM_FIFO_CTRL_DN));
-	dev_dbg(mcpdm->dev, "FIFO_CTRL_UP:  0x%04x\n",
-			omap_mcpdm_read(MCPDM_FIFO_CTRL_UP));
-	dev_dbg(mcpdm->dev, "DN_OFFSET:	0x%04x\n",
-			omap_mcpdm_read(MCPDM_DN_OFFSET));
-	dev_dbg(mcpdm->dev, "***********************\n");
-}
-
-/*
- * Takes the McPDM module in and out of reset state.
- * Uplink and downlink can be reset individually.
- */
-static void omap_mcpdm_reset_capture(int reset)
-{
-	int ctrl = omap_mcpdm_read(MCPDM_CTRL);
-
-	if (reset)
-		ctrl |= SW_UP_RST;
-	else
-		ctrl &= ~SW_UP_RST;
-
-	omap_mcpdm_write(MCPDM_CTRL, ctrl);
-}
-
-static void omap_mcpdm_reset_playback(int reset)
-{
-	int ctrl = omap_mcpdm_read(MCPDM_CTRL);
-
-	if (reset)
-		ctrl |= SW_DN_RST;
-	else
-		ctrl &= ~SW_DN_RST;
-
-	omap_mcpdm_write(MCPDM_CTRL, ctrl);
-}
-
-/*
- * Enables the transfer through the PDM interface to/from the Phoenix
- * codec by enabling the corresponding UP or DN channels.
- */
-void omap_mcpdm_start(int stream)
-{
-	int ctrl = omap_mcpdm_read(MCPDM_CTRL);
-
-	if (stream)
-		ctrl |= mcpdm->up_channels;
-	else
-		ctrl |= mcpdm->dn_channels;
-
-	omap_mcpdm_write(MCPDM_CTRL, ctrl);
-}
-
-/*
- * Disables the transfer through the PDM interface to/from the Phoenix
- * codec by disabling the corresponding UP or DN channels.
- */
-void omap_mcpdm_stop(int stream)
-{
-	int ctrl = omap_mcpdm_read(MCPDM_CTRL);
-
-	if (stream)
-		ctrl &= ~mcpdm->up_channels;
-	else
-		ctrl &= ~mcpdm->dn_channels;
-
-	omap_mcpdm_write(MCPDM_CTRL, ctrl);
-}
-
-/*
- * Configures McPDM uplink for audio recording.
- * This function should be called before omap_mcpdm_start.
- */
-int omap_mcpdm_capture_open(struct omap_mcpdm_link *uplink)
-{
-	int irq_mask = 0;
-	int ctrl;
-
-	if (!uplink)
-		return -EINVAL;
-
-	mcpdm->uplink = uplink;
-
-	/* Enable irq request generation */
-	irq_mask |= uplink->irq_mask & MCPDM_UPLINK_IRQ_MASK;
-	omap_mcpdm_write(MCPDM_IRQENABLE_SET, irq_mask);
-
-	/* Configure uplink threshold */
-	if (uplink->threshold > UP_THRES_MAX)
-		uplink->threshold = UP_THRES_MAX;
-
-	omap_mcpdm_write(MCPDM_FIFO_CTRL_UP, uplink->threshold);
-
-	/* Configure DMA controller */
-	omap_mcpdm_write(MCPDM_DMAENABLE_SET, DMA_UP_ENABLE);
-
-	/* Set pdm out format */
-	ctrl = omap_mcpdm_read(MCPDM_CTRL);
-	ctrl &= ~PDMOUTFORMAT;
-	ctrl |= uplink->format & PDMOUTFORMAT;
-
-	/* Uplink channels */
-	mcpdm->up_channels = uplink->channels & (PDM_UP_MASK | PDM_STATUS_MASK);
-
-	omap_mcpdm_write(MCPDM_CTRL, ctrl);
-
-	return 0;
-}
-
-/*
- * Configures McPDM downlink for audio playback.
- * This function should be called before omap_mcpdm_start.
- */
-int omap_mcpdm_playback_open(struct omap_mcpdm_link *downlink)
-{
-	int irq_mask = 0;
-	int ctrl;
-
-	if (!downlink)
-		return -EINVAL;
-
-	mcpdm->downlink = downlink;
-
-	/* Enable irq request generation */
-	irq_mask |= downlink->irq_mask & MCPDM_DOWNLINK_IRQ_MASK;
-	omap_mcpdm_write(MCPDM_IRQENABLE_SET, irq_mask);
-
-	/* Configure uplink threshold */
-	if (downlink->threshold > DN_THRES_MAX)
-		downlink->threshold = DN_THRES_MAX;
-
-	omap_mcpdm_write(MCPDM_FIFO_CTRL_DN, downlink->threshold);
-
-	/* Enable DMA request generation */
-	omap_mcpdm_write(MCPDM_DMAENABLE_SET, DMA_DN_ENABLE);
-
-	/* Set pdm out format */
-	ctrl = omap_mcpdm_read(MCPDM_CTRL);
-	ctrl &= ~PDMOUTFORMAT;
-	ctrl |= downlink->format & PDMOUTFORMAT;
-
-	/* Downlink channels */
-	mcpdm->dn_channels = downlink->channels & (PDM_DN_MASK | PDM_CMD_MASK);
-
-	omap_mcpdm_write(MCPDM_CTRL, ctrl);
-
-	return 0;
-}
-
-/*
- * Cleans McPDM uplink configuration.
- * This function should be called when the stream is closed.
- */
-int omap_mcpdm_capture_close(struct omap_mcpdm_link *uplink)
-{
-	int irq_mask = 0;
-
-	if (!uplink)
-		return -EINVAL;
-
-	/* Disable irq request generation */
-	irq_mask |= uplink->irq_mask & MCPDM_UPLINK_IRQ_MASK;
-	omap_mcpdm_write(MCPDM_IRQENABLE_CLR, irq_mask);
-
-	/* Disable DMA request generation */
-	omap_mcpdm_write(MCPDM_DMAENABLE_CLR, DMA_UP_ENABLE);
-
-	/* Clear Downlink channels */
-	mcpdm->up_channels = 0;
-
-	mcpdm->uplink = NULL;
-
-	return 0;
-}
-
-/*
- * Cleans McPDM downlink configuration.
- * This function should be called when the stream is closed.
- */
-int omap_mcpdm_playback_close(struct omap_mcpdm_link *downlink)
-{
-	int irq_mask = 0;
-
-	if (!downlink)
-		return -EINVAL;
-
-	/* Disable irq request generation */
-	irq_mask |= downlink->irq_mask & MCPDM_DOWNLINK_IRQ_MASK;
-	omap_mcpdm_write(MCPDM_IRQENABLE_CLR, irq_mask);
-
-	/* Disable DMA request generation */
-	omap_mcpdm_write(MCPDM_DMAENABLE_CLR, DMA_DN_ENABLE);
-
-	/* clear Downlink channels */
-	mcpdm->dn_channels = 0;
-
-	mcpdm->downlink = NULL;
-
-	return 0;
-}
-
-static irqreturn_t omap_mcpdm_irq_handler(int irq, void *dev_id)
-{
-	struct omap_mcpdm *mcpdm_irq = dev_id;
-	int irq_status;
-
-	irq_status = omap_mcpdm_read(MCPDM_IRQSTATUS);
-
-	/* Acknowledge irq event */
-	omap_mcpdm_write(MCPDM_IRQSTATUS, irq_status);
-
-	if (irq_status & MCPDM_DN_IRQ_FULL) {
-		dev_err(mcpdm_irq->dev, "DN FIFO error %x\n", irq_status);
-		omap_mcpdm_reset_playback(1);
-		omap_mcpdm_playback_open(mcpdm_irq->downlink);
-		omap_mcpdm_reset_playback(0);
-	}
-
-	if (irq_status & MCPDM_DN_IRQ_EMPTY) {
-		dev_err(mcpdm_irq->dev, "DN FIFO error %x\n", irq_status);
-		omap_mcpdm_reset_playback(1);
-		omap_mcpdm_playback_open(mcpdm_irq->downlink);
-		omap_mcpdm_reset_playback(0);
-	}
-
-	if (irq_status & MCPDM_DN_IRQ)
-		dev_dbg(mcpdm_irq->dev, "DN write request\n");
-
-	if (irq_status & MCPDM_UP_IRQ_FULL) {
-		dev_err(mcpdm_irq->dev, "UP FIFO error %x\n", irq_status);
-		omap_mcpdm_reset_capture(1);
-		omap_mcpdm_capture_open(mcpdm_irq->uplink);
-		omap_mcpdm_reset_capture(0);
-	}
-
-	if (irq_status & MCPDM_UP_IRQ_EMPTY) {
-		dev_err(mcpdm_irq->dev, "UP FIFO error %x\n", irq_status);
-		omap_mcpdm_reset_capture(1);
-		omap_mcpdm_capture_open(mcpdm_irq->uplink);
-		omap_mcpdm_reset_capture(0);
-	}
-
-	if (irq_status & MCPDM_UP_IRQ)
-		dev_dbg(mcpdm_irq->dev, "UP write request\n");
-
-	return IRQ_HANDLED;
-}
-
-int omap_mcpdm_request(void)
-{
-	int ret;
-
-	pm_runtime_get_sync(mcpdm->dev);
-
-	spin_lock(&mcpdm->lock);
-
-	if (!mcpdm->free) {
-		dev_err(mcpdm->dev, "McPDM interface is in use\n");
-		spin_unlock(&mcpdm->lock);
-		ret = -EBUSY;
-		goto err;
-	}
-	mcpdm->free = 0;
-
-	spin_unlock(&mcpdm->lock);
-
-	/* Disable lines while request is ongoing */
-	omap_mcpdm_write(MCPDM_CTRL, 0x00);
-
-	ret = request_irq(mcpdm->irq, omap_mcpdm_irq_handler,
-				0, "McPDM", (void *)mcpdm);
-	if (ret) {
-		dev_err(mcpdm->dev, "Request for McPDM IRQ failed\n");
-		goto err;
-	}
-
-	return 0;
-
-err:
-	mcpdm->free = 1;
-	pm_runtime_put_sync(mcpdm->dev);
-	return ret;
-}
-
-void omap_mcpdm_free(void)
-{
-	spin_lock(&mcpdm->lock);
-	if (mcpdm->free) {
-		dev_err(mcpdm->dev, "McPDM interface is already free\n");
-		spin_unlock(&mcpdm->lock);
-		return;
-	}
-	mcpdm->free = 1;
-	spin_unlock(&mcpdm->lock);
-
-	pm_runtime_put_sync(mcpdm->dev);
-
-	free_irq(mcpdm->irq, (void *)mcpdm);
-}
-
-/* Enable/disable DC offset cancelation for the analog
- * headset path (PDM channels 1 and 2).
- */
-int omap_mcpdm_set_offset(int offset1, int offset2)
-{
-	int offset;
-
-	if ((offset1 > DN_OFST_MAX) || (offset2 > DN_OFST_MAX))
-		return -EINVAL;
-
-	offset = (offset1 << DN_OFST_RX1) | (offset2 << DN_OFST_RX2);
-
-	/* offset cancellation for channel 1 */
-	if (offset1)
-		offset |= DN_OFST_RX1_EN;
-	else
-		offset &= ~DN_OFST_RX1_EN;
-
-	/* offset cancellation for channel 2 */
-	if (offset2)
-		offset |= DN_OFST_RX2_EN;
-	else
-		offset &= ~DN_OFST_RX2_EN;
-
-	omap_mcpdm_write(MCPDM_DN_OFFSET, offset);
-
-	return 0;
-}
-
-int __devinit omap_mcpdm_probe(struct platform_device *pdev)
-{
-	struct resource *res;
-	int ret = 0;
-
-	mcpdm = kzalloc(sizeof(struct omap_mcpdm), GFP_KERNEL);
-	if (!mcpdm) {
-		ret = -ENOMEM;
-		goto exit;
-	}
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (res == NULL) {
-		dev_err(&pdev->dev, "no resource\n");
-		goto err_resource;
-	}
-
-	spin_lock_init(&mcpdm->lock);
-	mcpdm->free = 1;
-
-	if (!request_mem_region(res->start, resource_size(res), "McPDM")) {
-		ret = -EBUSY;
-		goto err_resource;
-	}
-
-	mcpdm->io_base = ioremap(res->start, resource_size(res));
-	if (!mcpdm->io_base) {
-		ret = -ENOMEM;
-		goto err_remap;
-	}
-
-	mcpdm->irq = platform_get_irq(pdev, 0);
-
-	mcpdm->dev = &pdev->dev;
-	platform_set_drvdata(pdev, mcpdm);
-
-	pm_runtime_enable(mcpdm->dev);
-
-	return 0;
-
-err_remap:
-	release_mem_region(res->start, resource_size(res));
-err_resource:
-	kfree(mcpdm);
-exit:
-	return ret;
-}
-
-int __devexit omap_mcpdm_remove(struct platform_device *pdev)
-{
-	struct omap_mcpdm *mcpdm_ptr = platform_get_drvdata(pdev);
-	struct resource *res;
-
-	platform_set_drvdata(pdev, NULL);
-
-	pm_runtime_disable(mcpdm_ptr->dev);
-
-	iounmap(mcpdm_ptr->io_base);
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	release_mem_region(res->start, resource_size(res));
-
-	mcpdm_ptr->free = 0;
-	mcpdm_ptr->dev = NULL;
-
-	kfree(mcpdm_ptr);
-
-	return 0;
-}
-
diff --git a/sound/soc/omap/mcpdm.h b/sound/soc/omap/mcpdm.h
deleted file mode 100644
index b055ad1..0000000
--- a/sound/soc/omap/mcpdm.h
+++ /dev/null
@@ -1,152 +0,0 @@
-/*
- * mcpdm.h -- Defines for McPDM driver
- *
- * Author: Jorge Eduardo Candelaria <x0107209@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
- *
- */
-
-/* McPDM registers */
-
-#define MCPDM_REVISION         0x00
-#define MCPDM_SYSCONFIG                0x10
-#define MCPDM_IRQSTATUS_RAW    0x24
-#define MCPDM_IRQSTATUS                0x28
-#define MCPDM_IRQENABLE_SET    0x2C
-#define MCPDM_IRQENABLE_CLR    0x30
-#define MCPDM_IRQWAKE_EN       0x34
-#define MCPDM_DMAENABLE_SET    0x38
-#define MCPDM_DMAENABLE_CLR    0x3C
-#define MCPDM_DMAWAKEEN                0x40
-#define MCPDM_CTRL             0x44
-#define MCPDM_DN_DATA          0x48
-#define MCPDM_UP_DATA          0x4C
-#define MCPDM_FIFO_CTRL_DN     0x50
-#define MCPDM_FIFO_CTRL_UP     0x54
-#define MCPDM_DN_OFFSET                0x58
-
-/*
- * MCPDM_IRQ bit fields
- * IRQSTATUS_RAW, IRQSTATUS, IRQENABLE_SET, IRQENABLE_CLR
- */
-
-#define MCPDM_DN_IRQ                   (1 << 0)
-#define MCPDM_DN_IRQ_EMPTY             (1 << 1)
-#define MCPDM_DN_IRQ_ALMST_EMPTY       (1 << 2)
-#define MCPDM_DN_IRQ_FULL              (1 << 3)
-
-#define MCPDM_UP_IRQ                   (1 << 8)
-#define MCPDM_UP_IRQ_EMPTY             (1 << 9)
-#define MCPDM_UP_IRQ_ALMST_FULL                (1 << 10)
-#define MCPDM_UP_IRQ_FULL              (1 << 11)
-
-#define MCPDM_DOWNLINK_IRQ_MASK                0x00F
-#define MCPDM_UPLINK_IRQ_MASK          0xF00
-
-/*
- * MCPDM_DMAENABLE bit fields
- */
-
-#define DMA_DN_ENABLE          0x1
-#define DMA_UP_ENABLE          0x2
-
-/*
- * MCPDM_CTRL bit fields
- */
-
-#define PDM_UP1_EN             0x0001
-#define PDM_UP2_EN             0x0002
-#define PDM_UP3_EN             0x0004
-#define PDM_DN1_EN             0x0008
-#define PDM_DN2_EN             0x0010
-#define PDM_DN3_EN             0x0020
-#define PDM_DN4_EN             0x0040
-#define PDM_DN5_EN             0x0080
-#define PDMOUTFORMAT           0x0100
-#define CMD_INT                        0x0200
-#define STATUS_INT             0x0400
-#define SW_UP_RST              0x0800
-#define SW_DN_RST              0x1000
-#define PDM_UP_MASK            0x007
-#define PDM_DN_MASK            0x0F8
-#define PDM_CMD_MASK           0x200
-#define PDM_STATUS_MASK                0x400
-
-
-#define PDMOUTFORMAT_LJUST     (0 << 8)
-#define PDMOUTFORMAT_RJUST     (1 << 8)
-
-/*
- * MCPDM_FIFO_CTRL bit fields
- */
-
-#define UP_THRES_MAX           0xF
-#define DN_THRES_MAX           0xF
-
-/*
- * MCPDM_DN_OFFSET bit fields
- */
-
-#define DN_OFST_RX1_EN         0x0001
-#define DN_OFST_RX2_EN         0x0100
-
-#define DN_OFST_RX1            1
-#define DN_OFST_RX2            9
-#define DN_OFST_MAX            0x1F
-
-#define MCPDM_UPLINK           1
-#define MCPDM_DOWNLINK         2
-
-struct omap_mcpdm_link {
-       int irq_mask;
-       int threshold;
-       int format;
-       int channels;
-};
-
-struct omap_mcpdm_platform_data {
-       unsigned long phys_base;
-       u16 irq;
-};
-
-struct omap_mcpdm {
-       struct device *dev;
-       unsigned long phys_base;
-       void __iomem *io_base;
-       u8 free;
-       int irq;
-
-       spinlock_t lock;
-       struct omap_mcpdm_platform_data *pdata;
-       struct omap_mcpdm_link *downlink;
-       struct omap_mcpdm_link *uplink;
-       struct completion irq_completion;
-
-       int dn_channels;
-       int up_channels;
-};
-
-extern void omap_mcpdm_start(int stream);
-extern void omap_mcpdm_stop(int stream);
-extern int omap_mcpdm_capture_open(struct omap_mcpdm_link *uplink);
-extern int omap_mcpdm_playback_open(struct omap_mcpdm_link *downlink);
-extern int omap_mcpdm_capture_close(struct omap_mcpdm_link *uplink);
-extern int omap_mcpdm_playback_close(struct omap_mcpdm_link *downlink);
-extern int omap_mcpdm_request(void);
-extern void omap_mcpdm_free(void);
-extern int omap_mcpdm_set_offset(int offset1, int offset2);
-int __devinit omap_mcpdm_probe(struct platform_device *pdev);
-int __devexit omap_mcpdm_remove(struct platform_device *pdev);
diff --git a/sound/soc/omap/omap-mcpdm.c b/sound/soc/omap/omap-mcpdm.c
dissimilarity index 46%
index 0820b9e..4fd856d 100644
--- a/sound/soc/omap/omap-mcpdm.c
+++ b/sound/soc/omap/omap-mcpdm.c
@@ -1,279 +1,544 @@
-/*
- * omap-mcpdm.c  --  OMAP ALSA SoC DAI driver using McPDM port
- *
- * Copyright (C) 2009 Texas Instruments
- *
- * Author: Misael Lopez Cruz <x0052729@ti.com>
- * Contact: Jorge Eduardo Candelaria <x0107209@ti.com>
- *          Margarita Olaya <magi.olaya@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
- *
- */
-
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/device.h>
-#include <sound/core.h>
-#include <sound/pcm.h>
-#include <sound/pcm_params.h>
-#include <sound/initval.h>
-#include <sound/soc.h>
-
-#include <plat/dma.h>
-#include <plat/mcbsp.h>
-#include "mcpdm.h"
-#include "omap-pcm.h"
-
-struct omap_mcpdm_data {
-	struct omap_mcpdm_link *links;
-	int active;
-};
-
-static struct omap_mcpdm_link omap_mcpdm_links[] = {
-	/* downlink */
-	{
-		.irq_mask = MCPDM_DN_IRQ_EMPTY | MCPDM_DN_IRQ_FULL,
-		.threshold = 2,
-		.format = PDMOUTFORMAT_LJUST,
-	},
-	/* uplink */
-	{
-		.irq_mask = MCPDM_UP_IRQ_EMPTY | MCPDM_UP_IRQ_FULL,
-		.threshold = UP_THRES_MAX - 3,
-		.format = PDMOUTFORMAT_LJUST,
-	},
-};
-
-static struct omap_mcpdm_data mcpdm_data = {
-	.links = omap_mcpdm_links,
-	.active = 0,
-};
-
-/*
- * Stream DMA parameters
- */
-static struct omap_pcm_dma_data omap_mcpdm_dai_dma_params[] = {
-	{
-		.name = "Audio playback",
-		.dma_req = OMAP44XX_DMA_MCPDM_DL,
-		.data_type = OMAP_DMA_DATA_TYPE_S32,
-		.sync_mode = OMAP_DMA_SYNC_PACKET,
-		.packet_size = 16,
-		.port_addr = OMAP44XX_MCPDM_L3_BASE + MCPDM_DN_DATA,
-	},
-	{
-		.name = "Audio capture",
-		.dma_req = OMAP44XX_DMA_MCPDM_UP,
-		.data_type = OMAP_DMA_DATA_TYPE_S32,
-		.sync_mode = OMAP_DMA_SYNC_PACKET,
-		.packet_size = 16,
-		.port_addr = OMAP44XX_MCPDM_L3_BASE + MCPDM_UP_DATA,
-	},
-};
-
-static int omap_mcpdm_dai_startup(struct snd_pcm_substream *substream,
-				  struct snd_soc_dai *dai)
-{
-	int err = 0;
-
-	if (!dai->active)
-		err = omap_mcpdm_request();
-
-	return err;
-}
-
-static void omap_mcpdm_dai_shutdown(struct snd_pcm_substream *substream,
-				    struct snd_soc_dai *dai)
-{
-	if (!dai->active)
-		omap_mcpdm_free();
-}
-
-static int omap_mcpdm_dai_trigger(struct snd_pcm_substream *substream, int cmd,
-				  struct snd_soc_dai *dai)
-{
-	struct omap_mcpdm_data *mcpdm_priv = snd_soc_dai_get_drvdata(dai);
-	int stream = substream->stream;
-	int err = 0;
-
-	switch (cmd) {
-	case SNDRV_PCM_TRIGGER_START:
-	case SNDRV_PCM_TRIGGER_RESUME:
-	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		if (!mcpdm_priv->active++)
-			omap_mcpdm_start(stream);
-		break;
-
-	case SNDRV_PCM_TRIGGER_STOP:
-	case SNDRV_PCM_TRIGGER_SUSPEND:
-	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		if (!--mcpdm_priv->active)
-			omap_mcpdm_stop(stream);
-		break;
-	default:
-		err = -EINVAL;
-	}
-
-	return err;
-}
-
-static int omap_mcpdm_dai_hw_params(struct snd_pcm_substream *substream,
-				    struct snd_pcm_hw_params *params,
-				    struct snd_soc_dai *dai)
-{
-	struct omap_mcpdm_data *mcpdm_priv = snd_soc_dai_get_drvdata(dai);
-	struct omap_mcpdm_link *mcpdm_links = mcpdm_priv->links;
-	struct omap_pcm_dma_data *dma_data;
-	int threshold;
-	int stream = substream->stream;
-	int channels, err, link_mask = 0;
-
-	channels = params_channels(params);
-	switch (channels) {
-	case 4:
-		if (stream == SNDRV_PCM_STREAM_CAPTURE)
-			/* up to 2 channels for capture */
-			return -EINVAL;
-		link_mask |= 1 << 3;
-	case 3:
-		if (stream == SNDRV_PCM_STREAM_CAPTURE)
-			/* up to 2 channels for capture */
-			return -EINVAL;
-		link_mask |= 1 << 2;
-	case 2:
-		link_mask |= 1 << 1;
-	case 1:
-		link_mask |= 1 << 0;
-		break;
-	default:
-		/* unsupported number of channels */
-		return -EINVAL;
-	}
-
-	dma_data = &omap_mcpdm_dai_dma_params[stream];
-	threshold = mcpdm_links[stream].threshold;
-
-	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		mcpdm_links[stream].channels = link_mask << 3;
-		dma_data->packet_size = (DN_THRES_MAX - threshold) * channels;
-
-		err = omap_mcpdm_playback_open(&mcpdm_links[stream]);
-	} else {
-		mcpdm_links[stream].channels = link_mask << 0;
-		dma_data->packet_size = threshold * channels;
-
-		err = omap_mcpdm_capture_open(&mcpdm_links[stream]);
-	}
-
-	snd_soc_dai_set_dma_data(dai, substream, dma_data);
-	return err;
-}
-
-static int omap_mcpdm_dai_hw_free(struct snd_pcm_substream *substream,
-				  struct snd_soc_dai *dai)
-{
-	struct omap_mcpdm_data *mcpdm_priv = snd_soc_dai_get_drvdata(dai);
-	struct omap_mcpdm_link *mcpdm_links = mcpdm_priv->links;
-	int stream = substream->stream;
-	int err;
-
-	if (substream->stream ==  SNDRV_PCM_STREAM_PLAYBACK)
-		err = omap_mcpdm_playback_close(&mcpdm_links[stream]);
-	else
-		err = omap_mcpdm_capture_close(&mcpdm_links[stream]);
-
-	return err;
-}
-
-static struct snd_soc_dai_ops omap_mcpdm_dai_ops = {
-	.startup	= omap_mcpdm_dai_startup,
-	.shutdown	= omap_mcpdm_dai_shutdown,
-	.trigger	= omap_mcpdm_dai_trigger,
-	.hw_params	= omap_mcpdm_dai_hw_params,
-	.hw_free	= omap_mcpdm_dai_hw_free,
-};
-
-#define OMAP_MCPDM_RATES	(SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000)
-#define OMAP_MCPDM_FORMATS	(SNDRV_PCM_FMTBIT_S32_LE)
-
-static int omap_mcpdm_dai_probe(struct snd_soc_dai *dai)
-{
-	snd_soc_dai_set_drvdata(dai, &mcpdm_data);
-	return 0;
-}
-
-static struct snd_soc_dai_driver omap_mcpdm_dai = {
-	.probe = omap_mcpdm_dai_probe,
-	.playback = {
-		.channels_min = 1,
-		.channels_max = 4,
-		.rates = OMAP_MCPDM_RATES,
-		.formats = OMAP_MCPDM_FORMATS,
-	},
-	.capture = {
-		.channels_min = 1,
-		.channels_max = 2,
-		.rates = OMAP_MCPDM_RATES,
-		.formats = OMAP_MCPDM_FORMATS,
-	},
-	.ops = &omap_mcpdm_dai_ops,
-};
-
-static __devinit int asoc_mcpdm_probe(struct platform_device *pdev)
-{
-	int ret;
-
-	ret = omap_mcpdm_probe(pdev);
-	if (ret < 0)
-		return ret;
-	ret = snd_soc_register_dai(&pdev->dev, &omap_mcpdm_dai);
-	if (ret < 0)
-		omap_mcpdm_remove(pdev);
-	return ret;
-}
-
-static int __devexit asoc_mcpdm_remove(struct platform_device *pdev)
-{
-	snd_soc_unregister_dai(&pdev->dev);
-	omap_mcpdm_remove(pdev);
-	return 0;
-}
-
-static struct platform_driver asoc_mcpdm_driver = {
-	.driver = {
-			.name = "omap-mcpdm",
-			.owner = THIS_MODULE,
-	},
-
-	.probe = asoc_mcpdm_probe,
-	.remove = __devexit_p(asoc_mcpdm_remove),
-};
-
-static int __init snd_omap_mcpdm_init(void)
-{
-	return platform_driver_register(&asoc_mcpdm_driver);
-}
-module_init(snd_omap_mcpdm_init);
-
-static void __exit snd_omap_mcpdm_exit(void)
-{
-	platform_driver_unregister(&asoc_mcpdm_driver);
-}
-module_exit(snd_omap_mcpdm_exit);
-
-MODULE_AUTHOR("Misael Lopez Cruz <x0052729@ti.com>");
-MODULE_DESCRIPTION("OMAP PDM SoC Interface");
-MODULE_LICENSE("GPL");
+/*
+ * omap-mcpdm.c  --  OMAP ALSA SoC DAI driver using McPDM port
+ *
+ * Copyright (C) 2009 - 2011 Texas Instruments
+ *
+ * Author: Misael Lopez Cruz <misael.lopez@ti.com>
+ * Contact: Jorge Eduardo Candelaria <x0107209@ti.com>
+ *          Margarita Olaya <magi.olaya@ti.com>
+ *          Peter Ujfalusi <peter.ujfalusi@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
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/slab.h>
+#include <linux/pm_runtime.h>
+
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+
+#include <plat/dma.h>
+#include <plat/omap_hwmod.h>
+#include "omap-mcpdm.h"
+#include "omap-pcm.h"
+
+struct omap_mcpdm {
+	struct device *dev;
+	unsigned long phys_base;
+	void __iomem *io_base;
+	int irq;
+
+	struct mutex mutex;
+
+	/* channel data */
+	u32 dn_channels;
+	u32 up_channels;
+
+	/* McPDM FIFO thresholds */
+	u32 dn_threshold;
+	u32 up_threshold;
+
+	int enabled;
+};
+
+/*
+ * Stream DMA parameters
+ */
+static struct omap_pcm_dma_data omap_mcpdm_dai_dma_params[] = {
+	{
+		.name = "Audio playback",
+		.dma_req = OMAP44XX_DMA_MCPDM_DL,
+		.data_type = OMAP_DMA_DATA_TYPE_S32,
+		.sync_mode = OMAP_DMA_SYNC_PACKET,
+		.port_addr = OMAP44XX_MCPDM_L3_BASE + MCPDM_REG_DN_DATA,
+	},
+	{
+		.name = "Audio capture",
+		.dma_req = OMAP44XX_DMA_MCPDM_UP,
+		.data_type = OMAP_DMA_DATA_TYPE_S32,
+		.sync_mode = OMAP_DMA_SYNC_PACKET,
+		.port_addr = OMAP44XX_MCPDM_L3_BASE + MCPDM_REG_UP_DATA,
+	},
+};
+
+static inline void omap_mcpdm_write(struct omap_mcpdm *mcpdm, u16 reg, u32 val)
+{
+	__raw_writel(val, mcpdm->io_base + reg);
+}
+
+static inline int omap_mcpdm_read(struct omap_mcpdm *mcpdm, u16 reg)
+{
+	return __raw_readl(mcpdm->io_base + reg);
+}
+
+#ifdef DEBUG
+static void omap_mcpdm_reg_dump(struct omap_mcpdm *mcpdm)
+{
+	dev_dbg(mcpdm->dev, "***********************\n");
+	dev_dbg(mcpdm->dev, "IRQSTATUS_RAW:  0x%04x\n",
+			omap_mcpdm_read(mcpdm, MCPDM_REG_IRQSTATUS_RAW));
+	dev_dbg(mcpdm->dev, "IRQSTATUS:  0x%04x\n",
+			omap_mcpdm_read(mcpdm, MCPDM_REG_IRQSTATUS));
+	dev_dbg(mcpdm->dev, "IRQENABLE_SET:  0x%04x\n",
+			omap_mcpdm_read(mcpdm, MCPDM_REG_IRQENABLE_SET));
+	dev_dbg(mcpdm->dev, "IRQENABLE_CLR:  0x%04x\n",
+			omap_mcpdm_read(mcpdm, MCPDM_REG_IRQENABLE_CLR));
+	dev_dbg(mcpdm->dev, "IRQWAKE_EN: 0x%04x\n",
+			omap_mcpdm_read(mcpdm, MCPDM_REG_IRQWAKE_EN));
+	dev_dbg(mcpdm->dev, "DMAENABLE_SET: 0x%04x\n",
+			omap_mcpdm_read(mcpdm, MCPDM_REG_DMAENABLE_SET));
+	dev_dbg(mcpdm->dev, "DMAENABLE_CLR:  0x%04x\n",
+			omap_mcpdm_read(mcpdm, MCPDM_REG_DMAENABLE_CLR));
+	dev_dbg(mcpdm->dev, "DMAWAKEEN:  0x%04x\n",
+			omap_mcpdm_read(mcpdm, MCPDM_REG_DMAWAKEEN));
+	dev_dbg(mcpdm->dev, "CTRL:  0x%04x\n",
+			omap_mcpdm_read(mcpdm, MCPDM_REG_CTRL));
+	dev_dbg(mcpdm->dev, "DN_DATA:  0x%04x\n",
+			omap_mcpdm_read(mcpdm, MCPDM_REG_DN_DATA));
+	dev_dbg(mcpdm->dev, "UP_DATA: 0x%04x\n",
+			omap_mcpdm_read(mcpdm, MCPDM_REG_UP_DATA));
+	dev_dbg(mcpdm->dev, "FIFO_CTRL_DN: 0x%04x\n",
+			omap_mcpdm_read(mcpdm, MCPDM_REG_FIFO_CTRL_DN));
+	dev_dbg(mcpdm->dev, "FIFO_CTRL_UP:  0x%04x\n",
+			omap_mcpdm_read(mcpdm, MCPDM_REG_FIFO_CTRL_UP));
+	dev_dbg(mcpdm->dev, "***********************\n");
+}
+#else
+static void omap_mcpdm_reg_dump(struct omap_mcpdm *mcpdm) {}
+#endif
+
+/*
+ * Enables the transfer through the PDM interface to/from the Phoenix
+ * codec by enabling the corresponding UP or DN channels.
+ */
+static void omap_mcpdm_start(struct omap_mcpdm *mcpdm)
+{
+	u32 ctrl = omap_mcpdm_read(mcpdm, MCPDM_REG_CTRL);
+
+	ctrl |= (MCPDM_SW_DN_RST | MCPDM_SW_UP_RST);
+	omap_mcpdm_write(mcpdm, MCPDM_REG_CTRL, ctrl);
+
+	ctrl |= mcpdm->dn_channels | mcpdm->up_channels;
+	omap_mcpdm_write(mcpdm, MCPDM_REG_CTRL, ctrl);
+
+	ctrl &= ~(MCPDM_SW_DN_RST | MCPDM_SW_UP_RST);
+	omap_mcpdm_write(mcpdm, MCPDM_REG_CTRL, ctrl);
+}
+
+/*
+ * Disables the transfer through the PDM interface to/from the Phoenix
+ * codec by disabling the corresponding UP or DN channels.
+ */
+static void omap_mcpdm_stop(struct omap_mcpdm *mcpdm)
+{
+	u32 ctrl = omap_mcpdm_read(mcpdm, MCPDM_REG_CTRL);
+
+	ctrl |= (MCPDM_SW_DN_RST | MCPDM_SW_UP_RST);
+	omap_mcpdm_write(mcpdm, MCPDM_REG_CTRL, ctrl);
+
+	ctrl &= ~(mcpdm->dn_channels | mcpdm->up_channels);
+	omap_mcpdm_write(mcpdm, MCPDM_REG_CTRL, ctrl);
+
+	ctrl &= ~(MCPDM_SW_DN_RST | MCPDM_SW_UP_RST);
+	omap_mcpdm_write(mcpdm, MCPDM_REG_CTRL, ctrl);
+
+}
+
+/*
+ * Is the physical McPDM interface active.
+ */
+static inline int omap_mcpdm_active(struct omap_mcpdm *mcpdm)
+{
+	return omap_mcpdm_read(mcpdm, MCPDM_REG_CTRL) &
+					(MCPDM_PDM_DN_MASK | MCPDM_PDM_UP_MASK);
+}
+
+/*
+ * Configures McPDM uplink, and downlink for audio.
+ * This function should be called before omap_mcpdm_start.
+ */
+static void omap_mcpdm_open_streams(struct omap_mcpdm *mcpdm)
+{
+	omap_mcpdm_write(mcpdm, MCPDM_REG_IRQENABLE_SET,
+			MCPDM_DN_IRQ_EMPTY | MCPDM_DN_IRQ_FULL |
+			MCPDM_UP_IRQ_EMPTY | MCPDM_UP_IRQ_FULL);
+
+	omap_mcpdm_write(mcpdm, MCPDM_REG_FIFO_CTRL_DN, mcpdm->dn_threshold);
+	omap_mcpdm_write(mcpdm, MCPDM_REG_FIFO_CTRL_UP, mcpdm->up_threshold);
+
+	omap_mcpdm_write(mcpdm, MCPDM_REG_DMAENABLE_SET,
+			MCPDM_DMA_DN_ENABLE | MCPDM_DMA_UP_ENABLE);
+}
+
+/*
+ * Cleans McPDM uplink, and downlink configuration.
+ * This function should be called when the stream is closed.
+ */
+static void omap_mcpdm_close_streams(struct omap_mcpdm *mcpdm)
+{
+	/* Disable irq request generation for downlink */
+	omap_mcpdm_write(mcpdm, MCPDM_REG_IRQENABLE_CLR,
+			MCPDM_DN_IRQ_EMPTY | MCPDM_DN_IRQ_FULL);
+
+	/* Disable DMA request generation for downlink */
+	omap_mcpdm_write(mcpdm, MCPDM_REG_DMAENABLE_CLR, MCPDM_DMA_DN_ENABLE);
+
+	/* Disable irq request generation for uplink */
+	omap_mcpdm_write(mcpdm, MCPDM_REG_IRQENABLE_CLR,
+			MCPDM_UP_IRQ_EMPTY | MCPDM_UP_IRQ_FULL);
+
+	/* Disable DMA request generation for uplink */
+	omap_mcpdm_write(mcpdm, MCPDM_REG_DMAENABLE_CLR, MCPDM_DMA_UP_ENABLE);
+}
+
+static irqreturn_t omap_mcpdm_irq_handler(int irq, void *dev_id)
+{
+	struct omap_mcpdm *mcpdm = dev_id;
+	int irq_status;
+
+	irq_status = omap_mcpdm_read(mcpdm, MCPDM_REG_IRQSTATUS);
+
+	/* Acknowledge irq event */
+	omap_mcpdm_write(mcpdm, MCPDM_REG_IRQSTATUS, irq_status);
+
+	if (irq_status & MCPDM_DN_IRQ_FULL)
+		dev_dbg(mcpdm->dev, "DN (playback) FIFO Full\n");
+
+	if (irq_status & MCPDM_DN_IRQ_EMPTY)
+		dev_dbg(mcpdm->dev, "DN (playback) FIFO Empty\n");
+
+	if (irq_status & MCPDM_DN_IRQ)
+		dev_dbg(mcpdm->dev, "DN (playback) write request\n");
+
+	if (irq_status & MCPDM_UP_IRQ_FULL)
+		dev_dbg(mcpdm->dev, "UP (capture) FIFO Full\n");
+
+	if (irq_status & MCPDM_UP_IRQ_EMPTY)
+		dev_dbg(mcpdm->dev, "UP (capture) FIFO Empty\n");
+
+	if (irq_status & MCPDM_UP_IRQ)
+		dev_dbg(mcpdm->dev, "UP (capture) write request\n");
+
+	return IRQ_HANDLED;
+}
+
+static int omap_mcpdm_dai_startup(struct snd_pcm_substream *substream,
+				  struct snd_soc_dai *dai)
+{
+	struct omap_mcpdm *mcpdm = snd_soc_dai_get_drvdata(dai);
+
+	mutex_lock(&mcpdm->mutex);
+
+	if (!mcpdm->enabled) {
+		pm_runtime_get_sync(mcpdm->dev);
+
+		/* Enable watch dog for ES above ES 1.0 to avoid saturation */
+		if (omap_rev() != OMAP4430_REV_ES1_0) {
+			u32 ctrl = omap_mcpdm_read(mcpdm, MCPDM_REG_CTRL);
+
+			omap_mcpdm_write(mcpdm, MCPDM_REG_CTRL,
+					 ctrl | MCPDM_WD_EN);
+		}
+		omap_mcpdm_open_streams(mcpdm);
+		mcpdm->enabled = 1;
+	}
+
+	mutex_unlock(&mcpdm->mutex);
+
+	return 0;
+}
+
+static int omap_mcpdm_dai_hw_params(struct snd_pcm_substream *substream,
+				    struct snd_pcm_hw_params *params,
+				    struct snd_soc_dai *dai)
+{
+	struct omap_mcpdm *mcpdm = snd_soc_dai_get_drvdata(dai);
+	int stream = substream->stream;
+	struct omap_pcm_dma_data *dma_data;
+	int channels;
+	int link_mask = 0;
+
+	channels = params_channels(params);
+	switch (channels) {
+	case 4:
+		if (stream == SNDRV_PCM_STREAM_CAPTURE)
+			/* up to 2 channels for capture */
+			return -EINVAL;
+		link_mask |= 1 << 3;
+	case 3:
+		if (stream == SNDRV_PCM_STREAM_CAPTURE)
+			/* up to 2 channels for capture */
+			return -EINVAL;
+		link_mask |= 1 << 2;
+	case 2:
+		link_mask |= 1 << 1;
+	case 1:
+		link_mask |= 1 << 0;
+		break;
+	default:
+		/* unsupported number of channels */
+		return -EINVAL;
+	}
+
+	dma_data = &omap_mcpdm_dai_dma_params[stream];
+
+	/* Configure McPDM channels, and DMA packet size */
+	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		mcpdm->dn_channels = link_mask << 3;
+		dma_data->packet_size =
+			(MCPDM_DN_THRES_MAX - mcpdm->dn_threshold) * channels;
+	} else {
+		mcpdm->up_channels = link_mask << 0;
+		dma_data->packet_size = mcpdm->up_threshold * channels;
+	}
+
+	snd_soc_dai_set_dma_data(dai, substream, dma_data);
+
+	return 0;
+}
+
+static int omap_mcpdm_prepare(struct snd_pcm_substream *substream,
+				  struct snd_soc_dai *dai)
+{
+	struct omap_mcpdm *mcpdm = snd_soc_dai_get_drvdata(dai);
+
+	if (!omap_mcpdm_active(mcpdm)) {
+		omap_mcpdm_start(mcpdm);
+		omap_mcpdm_reg_dump(mcpdm);
+	}
+
+	return 0;
+}
+
+static struct snd_soc_dai_ops omap_mcpdm_dai_ops = {
+	.startup	= omap_mcpdm_dai_startup,
+	.hw_params	= omap_mcpdm_dai_hw_params,
+	.prepare	= omap_mcpdm_prepare,
+};
+
+static int omap_mcpdm_probe(struct snd_soc_dai *dai)
+{
+	struct omap_mcpdm *mcpdm = snd_soc_dai_get_drvdata(dai);
+	int ret;
+
+	pm_runtime_enable(mcpdm->dev);
+
+	/* Disable lines while request is ongoing */
+	pm_runtime_get_sync(mcpdm->dev);
+	omap_mcpdm_write(mcpdm, MCPDM_REG_CTRL, 0x00);
+
+	ret = request_irq(mcpdm->irq, omap_mcpdm_irq_handler,
+				0, "McPDM", (void *)mcpdm);
+
+	pm_runtime_put_sync(mcpdm->dev);
+
+	if (ret) {
+		dev_err(mcpdm->dev, "Request for IRQ failed\n");
+		pm_runtime_disable(mcpdm->dev);
+	}
+
+	/* Configure McPDM threshold values */
+	mcpdm->dn_threshold = 2;
+	mcpdm->up_threshold = MCPDM_UP_THRES_MAX - 3;
+	return ret;
+}
+
+static int omap_mcpdm_remove(struct snd_soc_dai *dai)
+{
+	struct omap_mcpdm *mcpdm = snd_soc_dai_get_drvdata(dai);
+
+	free_irq(mcpdm->irq, (void *)mcpdm);
+	pm_runtime_disable(mcpdm->dev);
+
+	return 0;
+}
+
+#define OMAP_MCPDM_RATES	(SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000)
+#define OMAP_MCPDM_FORMATS	SNDRV_PCM_FMTBIT_S32_LE
+
+static struct snd_soc_dai_driver omap_mcpdm_dai = {
+	.probe = omap_mcpdm_probe,
+	.remove = omap_mcpdm_remove,
+	.probe_order = SND_SOC_COMP_ORDER_LATE,
+	.remove_order = SND_SOC_COMP_ORDER_EARLY,
+	.playback = {
+		.channels_min = 1,
+		.channels_max = 4,
+		.rates = OMAP_MCPDM_RATES,
+		.formats = OMAP_MCPDM_FORMATS,
+	},
+	.capture = {
+		.channels_min = 1,
+		.channels_max = 2,
+		.rates = OMAP_MCPDM_RATES,
+		.formats = OMAP_MCPDM_FORMATS,
+	},
+	.ops = &omap_mcpdm_dai_ops,
+};
+
+/*
+ * DAPM event function to ensure, that the host side McPDM interface is turned
+ * off after the codec's DAC.
+ */
+static int omap_mcpdm_interface_event(struct snd_soc_dapm_widget *w,
+		struct snd_kcontrol *kcontrol, int event)
+{
+	struct omap_mcpdm *mcpdm = w->private_data;
+
+	mutex_lock(&mcpdm->mutex);
+	switch (event) {
+	case SND_SOC_DAPM_POST_PMD:
+		if (unlikely(!mcpdm->enabled)) {
+			dev_err(mcpdm->dev, "%s: Not enabled\n", __func__);
+			goto out;
+		}
+		if (omap_mcpdm_active(mcpdm)) {
+			omap_mcpdm_stop(mcpdm);
+			omap_mcpdm_close_streams(mcpdm);
+		}
+
+		if (!omap_mcpdm_active(mcpdm))
+			pm_runtime_put_sync(mcpdm->dev);
+
+		mcpdm->enabled = 0;
+		break;
+	}
+out:
+	mutex_unlock(&mcpdm->mutex);
+	return 0;
+}
+
+/*
+ * DAPM_SUPPLY widget can be hooked up in the machine driver to make sure that
+ * the host side McPDM is not turned off too early.
+ */
+static struct snd_soc_dapm_widget omap_mcpdm_widgets[] = {
+	SND_SOC_DAPM_SUPPLY("OMAP McPDM Interface", SND_SOC_NOPM, 0, 0,
+			    omap_mcpdm_interface_event, SND_SOC_DAPM_POST_PMD),
+};
+
+int omap_mcpdm_add_dapm_widget(struct snd_soc_codec *codec)
+{
+	struct snd_soc_dapm_context *dapm = &codec->dapm;
+
+	return snd_soc_dapm_new_controls(dapm, omap_mcpdm_widgets,
+						ARRAY_SIZE(omap_mcpdm_widgets));
+}
+
+static __devinit int asoc_mcpdm_probe(struct platform_device *pdev)
+{
+	struct omap_mcpdm *mcpdm;
+	struct resource *res;
+	int ret = 0;
+
+	mcpdm = kzalloc(sizeof(struct omap_mcpdm), GFP_KERNEL);
+	if (!mcpdm)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, mcpdm);
+
+	mutex_init(&mcpdm->mutex);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res == NULL) {
+		dev_err(&pdev->dev, "no resource\n");
+		goto err_res;
+	}
+
+	if (!request_mem_region(res->start, resource_size(res), "McPDM")) {
+		ret = -EBUSY;
+		goto err_res;
+	}
+
+	mcpdm->io_base = ioremap(res->start, resource_size(res));
+	if (!mcpdm->io_base) {
+		ret = -ENOMEM;
+		goto err_iomap;
+	}
+
+	mcpdm->irq = platform_get_irq(pdev, 0);
+	if (mcpdm->irq < 0) {
+		ret = mcpdm->irq;
+		goto err_irq;
+	}
+
+	mcpdm->dev = &pdev->dev;
+
+	ret = snd_soc_register_dai(&pdev->dev, &omap_mcpdm_dai);
+	if (!ret) {
+		/* Initialize the private_data within the DAPM_SUPPLY widget */
+		omap_mcpdm_widgets[0].private_data = mcpdm;
+		return 0;
+	}
+
+err_irq:
+	iounmap(mcpdm->io_base);
+err_iomap:
+	release_mem_region(res->start, resource_size(res));
+err_res:
+	kfree(mcpdm);
+	return ret;
+}
+
+static int __devexit asoc_mcpdm_remove(struct platform_device *pdev)
+{
+	struct omap_mcpdm *mcpdm = platform_get_drvdata(pdev);
+	struct resource *res;
+
+	snd_soc_unregister_dai(&pdev->dev);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	iounmap(mcpdm->io_base);
+	release_mem_region(res->start, resource_size(res));
+
+	kfree(mcpdm);
+	return 0;
+}
+
+static struct platform_driver asoc_mcpdm_driver = {
+	.driver = {
+		.name	= "omap-mcpdm",
+		.owner	= THIS_MODULE,
+	},
+
+	.probe	= asoc_mcpdm_probe,
+	.remove	= __devexit_p(asoc_mcpdm_remove),
+};
+
+static int __init snd_omap_mcpdm_init(void)
+{
+	return platform_driver_register(&asoc_mcpdm_driver);
+}
+module_init(snd_omap_mcpdm_init);
+
+static void __exit snd_omap_mcpdm_exit(void)
+{
+	platform_driver_unregister(&asoc_mcpdm_driver);
+}
+module_exit(snd_omap_mcpdm_exit);
+
+MODULE_AUTHOR("Misael Lopez Cruz <misael.lopez@ti.com>");
+MODULE_DESCRIPTION("OMAP PDM SoC Interface");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/omap/omap-mcpdm.h b/sound/soc/omap/omap-mcpdm.h
new file mode 100644
index 0000000..e72524c
--- /dev/null
+++ b/sound/soc/omap/omap-mcpdm.h
@@ -0,0 +1,97 @@
+/*
+ * omap-mcpdm.h
+ *
+ * Copyright (C) 2009 - 2011 Texas Instruments
+ *
+ * Contact: 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
+ *
+ */
+
+#ifndef __OMAP_MCPDM_H__
+#define __OMAP_MCPDM_H__
+
+#define MCPDM_REG_REVISION		0x00
+#define MCPDM_REG_SYSCONFIG		0x10
+#define MCPDM_REG_IRQSTATUS_RAW		0x24
+#define MCPDM_REG_IRQSTATUS		0x28
+#define MCPDM_REG_IRQENABLE_SET		0x2C
+#define MCPDM_REG_IRQENABLE_CLR		0x30
+#define MCPDM_REG_IRQWAKE_EN		0x34
+#define MCPDM_REG_DMAENABLE_SET		0x38
+#define MCPDM_REG_DMAENABLE_CLR		0x3C
+#define MCPDM_REG_DMAWAKEEN		0x40
+#define MCPDM_REG_CTRL			0x44
+#define MCPDM_REG_DN_DATA		0x48
+#define MCPDM_REG_UP_DATA		0x4C
+#define MCPDM_REG_FIFO_CTRL_DN		0x50
+#define MCPDM_REG_FIFO_CTRL_UP		0x54
+#define MCPDM_REG_DN_OFFSET		0x58
+
+/*
+ * MCPDM_IRQ bit fields
+ * IRQSTATUS_RAW, IRQSTATUS, IRQENABLE_SET, IRQENABLE_CLR
+ */
+
+#define MCPDM_DN_IRQ			(1 << 0)
+#define MCPDM_DN_IRQ_EMPTY		(1 << 1)
+#define MCPDM_DN_IRQ_ALMST_EMPTY	(1 << 2)
+#define MCPDM_DN_IRQ_FULL		(1 << 3)
+
+#define MCPDM_UP_IRQ			(1 << 8)
+#define MCPDM_UP_IRQ_EMPTY		(1 << 9)
+#define MCPDM_UP_IRQ_ALMST_FULL		(1 << 10)
+#define MCPDM_UP_IRQ_FULL		(1 << 11)
+
+#define MCPDM_DOWNLINK_IRQ_MASK		0x00F
+#define MCPDM_UPLINK_IRQ_MASK		0xF00
+
+/*
+ * MCPDM_DMAENABLE bit fields
+ */
+
+#define MCPDM_DMA_DN_ENABLE		(1 << 0)
+#define MCPDM_DMA_UP_ENABLE		(1 << 1)
+
+/*
+ * MCPDM_CTRL bit fields
+ */
+
+#define MCPDM_PDM_UPLINK_EN(x)		(1 << (x - 1)) /* ch1 is at bit 0 */
+#define MCPDM_PDM_DOWNLINK_EN(x)	(1 << (x + 2)) /* ch1 is at bit 3 */
+#define MCPDM_PDMOUTFORMAT		(1 << 8)
+#define MCPDM_CMD_INT			(1 << 9)
+#define MCPDM_STATUS_INT		(1 << 10)
+#define MCPDM_SW_UP_RST			(1 << 11)
+#define MCPDM_SW_DN_RST			(1 << 12)
+#define MCPDM_WD_EN			(1 << 14)
+#define MCPDM_PDM_UP_MASK		0x7
+#define MCPDM_PDM_DN_MASK		(0x1f << 3)
+
+
+#define MCPDM_PDMOUTFORMAT_LJUST	(0 << 8)
+#define MCPDM_PDMOUTFORMAT_RJUST	(1 << 8)
+
+/*
+ * MCPDM_FIFO_CTRL bit fields
+ */
+
+#define MCPDM_UP_THRES_MAX		0xF
+#define MCPDM_DN_THRES_MAX		0xF
+
+int omap_mcpdm_add_dapm_widget(struct snd_soc_codec *codec);
+
+#endif	/* End of __OMAP_MCPDM_H__ */
diff --git a/sound/soc/omap/sdp4430.c b/sound/soc/omap/sdp4430.c
index eec77fe..a2b4f60 100644
--- a/sound/soc/omap/sdp4430.c
+++ b/sound/soc/omap/sdp4430.c
@@ -32,7 +32,7 @@
 #include <plat/hardware.h>
 #include <plat/mux.h>
 
-#include "mcpdm.h"
+#include "omap-mcpdm.h"
 #include "omap-pcm.h"
 #include "../codecs/twl6040.h"
 
@@ -115,6 +115,14 @@ static const struct snd_soc_dapm_route audio_map[] = {
 	/* Aux/FM Stereo In: AFML, AFMR */
 	{"AFML", NULL, "FM Stereo In"},
 	{"AFMR", NULL, "FM Stereo In"},
+
+	/* OMAP4 McPDM interface */
+	{"ADC Left", NULL, "OMAP McPDM Interface"},
+	{"ADC Right", NULL, "OMAP McPDM Interface"},
+	{"HSDAC Left", NULL, "OMAP McPDM Interface"},
+	{"HSDAC Right", NULL, "OMAP McPDM Interface"},
+	{"HFDAC Left", NULL, "OMAP McPDM Interface"},
+	{"HFDAC Right", NULL, "OMAP McPDM Interface"},
 };
 
 static int sdp4430_twl6040_init(struct snd_soc_pcm_runtime *rtd)
@@ -129,6 +137,10 @@ static int sdp4430_twl6040_init(struct snd_soc_pcm_runtime *rtd)
 	if (ret)
 		return ret;
 
+	ret = omap_mcpdm_add_dapm_widget(codec);
+	if (ret)
+		return ret;
+
 	/* Set up SDP4430 specific audio path audio_map */
 	snd_soc_dapm_add_routes(dapm, audio_map, ARRAY_SIZE(audio_map));
 
-- 
1.7.6

Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Lars-Peter Clausen]

On 08/19/2011 09:41 AM, Peter Ujfalusi wrote:
> From: Misael Lopez Cruz <misael.lopez@ti.com>
> 
> Reasons for the replacement:
> The current driver for McPDM was developed to support the legacy mode only.
> In preparation for the ABE support the current driver stack need the be
> replaced.
> The new driver is much simpler, easier to extend, and it also fixes some of the
> issues with the old stack.
> 
> Main changes:
> - single file for omap-mcpdm (mcpdm.c/h removed)
> - Define names for registers, bits cleaned up, prefixed
> - Full-duplex audio operation (arecord | aplay) has been fixed
> - Less code
> 
> McPDM need to be turned off _after_ the codec's DAC/ADC has been powered down.
> The solution for this without extensive change in core:
> Use DAPM_SUPPLY to turn off the McPDM interface.
> The machine driver connects the "OMAP McPDM Interface" to the codec's DAC/ADC,
> so after audio activity we can be sure that the host side McPDM is turned off
> at the correct time.
> 
> Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
> Signed-off-by: Liam Girdwood <lrg@ti.com>
> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  sound/soc/omap/Makefile     |    2 +-
>  sound/soc/omap/mcpdm.c      |  472 -------------------------
>  sound/soc/omap/mcpdm.h      |  152 --------
>  sound/soc/omap/omap-mcpdm.c |  823 ++++++++++++++++++++++++++++---------------
>  sound/soc/omap/omap-mcpdm.h |   97 +++++
>  sound/soc/omap/sdp4430.c    |   14 +-
>  6 files changed, 655 insertions(+), 905 deletions(-)
>  delete mode 100644 sound/soc/omap/mcpdm.c
>  delete mode 100644 sound/soc/omap/mcpdm.h
>  rewrite sound/soc/omap/omap-mcpdm.c (46%)
>  create mode 100644 sound/soc/omap/omap-mcpdm.h
> 
> diff --git a/sound/soc/omap/Makefile b/sound/soc/omap/Makefile
> index 59e2c8d..052fd75 100644
> --- a/sound/soc/omap/Makefile
> +++ b/sound/soc/omap/Makefile
> @@ -1,7 +1,7 @@
>  # OMAP Platform Support
>  snd-soc-omap-objs := omap-pcm.o
>  snd-soc-omap-mcbsp-objs := omap-mcbsp.o
> -snd-soc-omap-mcpdm-objs := omap-mcpdm.o mcpdm.o
> +snd-soc-omap-mcpdm-objs := omap-mcpdm.o
>  snd-soc-omap-hdmi-objs := omap-hdmi.o
>  
>  obj-$(CONFIG_SND_OMAP_SOC) += snd-soc-omap.o
> diff --git a/sound/soc/omap/mcpdm.c b/sound/soc/omap/mcpdm.c
> deleted file mode 100644
> index 9746a49..0000000
> --- a/sound/soc/omap/mcpdm.c
> +++ /dev/null
> @@ -1,472 +0,0 @@
> [...]
> +static int omap_mcpdm_interface_event(struct snd_soc_dapm_widget *w,
> +		struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct omap_mcpdm *mcpdm = w->private_data;

Can't you use
struct omap_mcpdm *mcpdm = snd_soc_codec_get_drvdata(codec)?

This would avoid having to add the private_data field to the widget struct. In
it's current form it will only really work, if there is just one instance of
the driver using the widget. And if that's the case you can use a global
variable directly anyway.

- Lars

Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Lars-Peter Clausen]

On 08/19/2011 10:38 AM, Lars-Peter Clausen wrote:
> On 08/19/2011 09:41 AM, Peter Ujfalusi wrote:
>> From: Misael Lopez Cruz <misael.lopez@ti.com>
>>
>> Reasons for the replacement:
>> The current driver for McPDM was developed to support the legacy mode only.
>> In preparation for the ABE support the current driver stack need the be
>> replaced.
>> The new driver is much simpler, easier to extend, and it also fixes some of the
>> issues with the old stack.
>>
>> Main changes:
>> - single file for omap-mcpdm (mcpdm.c/h removed)
>> - Define names for registers, bits cleaned up, prefixed
>> - Full-duplex audio operation (arecord | aplay) has been fixed
>> - Less code
>>
>> McPDM need to be turned off _after_ the codec's DAC/ADC has been powered down.
>> The solution for this without extensive change in core:
>> Use DAPM_SUPPLY to turn off the McPDM interface.
>> The machine driver connects the "OMAP McPDM Interface" to the codec's DAC/ADC,
>> so after audio activity we can be sure that the host side McPDM is turned off
>> at the correct time.
>>
>> Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
>> Signed-off-by: Liam Girdwood <lrg@ti.com>
>> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  sound/soc/omap/Makefile     |    2 +-
>>  sound/soc/omap/mcpdm.c      |  472 -------------------------
>>  sound/soc/omap/mcpdm.h      |  152 --------
>>  sound/soc/omap/omap-mcpdm.c |  823 ++++++++++++++++++++++++++++---------------
>>  sound/soc/omap/omap-mcpdm.h |   97 +++++
>>  sound/soc/omap/sdp4430.c    |   14 +-
>>  6 files changed, 655 insertions(+), 905 deletions(-)
>>  delete mode 100644 sound/soc/omap/mcpdm.c
>>  delete mode 100644 sound/soc/omap/mcpdm.h
>>  rewrite sound/soc/omap/omap-mcpdm.c (46%)
>>  create mode 100644 sound/soc/omap/omap-mcpdm.h
>>
>> diff --git a/sound/soc/omap/Makefile b/sound/soc/omap/Makefile
>> index 59e2c8d..052fd75 100644
>> --- a/sound/soc/omap/Makefile
>> +++ b/sound/soc/omap/Makefile
>> @@ -1,7 +1,7 @@
>>  # OMAP Platform Support
>>  snd-soc-omap-objs := omap-pcm.o
>>  snd-soc-omap-mcbsp-objs := omap-mcbsp.o
>> -snd-soc-omap-mcpdm-objs := omap-mcpdm.o mcpdm.o
>> +snd-soc-omap-mcpdm-objs := omap-mcpdm.o
>>  snd-soc-omap-hdmi-objs := omap-hdmi.o
>>  
>>  obj-$(CONFIG_SND_OMAP_SOC) += snd-soc-omap.o
>> diff --git a/sound/soc/omap/mcpdm.c b/sound/soc/omap/mcpdm.c
>> deleted file mode 100644
>> index 9746a49..0000000
>> --- a/sound/soc/omap/mcpdm.c
>> +++ /dev/null
>> @@ -1,472 +0,0 @@
>> [...]
>> +static int omap_mcpdm_interface_event(struct snd_soc_dapm_widget *w,
>> +		struct snd_kcontrol *kcontrol, int event)
>> +{
>> +	struct omap_mcpdm *mcpdm = w->private_data;
> 
> Can't you use
> struct omap_mcpdm *mcpdm = snd_soc_codec_get_drvdata(codec)?

sorry:
struct omap_mcpdm *mcpdm =  snd_soc_platform_get_drvdata(w->platform);


> 
> This would avoid having to add the private_data field to the widget struct. In
> it's current form it will only really work, if there is just one instance of
> the driver using the widget. And if that's the case you can use a global
> variable directly anyway.
> 
> - Lars
> 
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Liam Girdwood]

On 19/08/11 09:49, Lars-Peter Clausen wrote:
> On 08/19/2011 10:38 AM, Lars-Peter Clausen wrote:
>> On 08/19/2011 09:41 AM, Peter Ujfalusi wrote:
>>> From: Misael Lopez Cruz <misael.lopez@ti.com>
>>>
>>> Reasons for the replacement:
>>> The current driver for McPDM was developed to support the legacy mode only.
>>> In preparation for the ABE support the current driver stack need the be
>>> replaced.
>>> The new driver is much simpler, easier to extend, and it also fixes some of the
>>> issues with the old stack.
>>>
>>> Main changes:
>>> - single file for omap-mcpdm (mcpdm.c/h removed)
>>> - Define names for registers, bits cleaned up, prefixed
>>> - Full-duplex audio operation (arecord | aplay) has been fixed
>>> - Less code
>>>
>>> McPDM need to be turned off _after_ the codec's DAC/ADC has been powered down.
>>> The solution for this without extensive change in core:
>>> Use DAPM_SUPPLY to turn off the McPDM interface.
>>> The machine driver connects the "OMAP McPDM Interface" to the codec's DAC/ADC,
>>> so after audio activity we can be sure that the host side McPDM is turned off
>>> at the correct time.
>>>
>>> Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
>>> Signed-off-by: Liam Girdwood <lrg@ti.com>
>>> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>> ---
>>>  sound/soc/omap/Makefile     |    2 +-
>>>  sound/soc/omap/mcpdm.c      |  472 -------------------------
>>>  sound/soc/omap/mcpdm.h      |  152 --------
>>>  sound/soc/omap/omap-mcpdm.c |  823 ++++++++++++++++++++++++++++---------------
>>>  sound/soc/omap/omap-mcpdm.h |   97 +++++
>>>  sound/soc/omap/sdp4430.c    |   14 +-
>>>  6 files changed, 655 insertions(+), 905 deletions(-)
>>>  delete mode 100644 sound/soc/omap/mcpdm.c
>>>  delete mode 100644 sound/soc/omap/mcpdm.h
>>>  rewrite sound/soc/omap/omap-mcpdm.c (46%)
>>>  create mode 100644 sound/soc/omap/omap-mcpdm.h
>>>
>>> diff --git a/sound/soc/omap/Makefile b/sound/soc/omap/Makefile
>>> index 59e2c8d..052fd75 100644
>>> --- a/sound/soc/omap/Makefile
>>> +++ b/sound/soc/omap/Makefile
>>> @@ -1,7 +1,7 @@
>>>  # OMAP Platform Support
>>>  snd-soc-omap-objs := omap-pcm.o
>>>  snd-soc-omap-mcbsp-objs := omap-mcbsp.o
>>> -snd-soc-omap-mcpdm-objs := omap-mcpdm.o mcpdm.o
>>> +snd-soc-omap-mcpdm-objs := omap-mcpdm.o
>>>  snd-soc-omap-hdmi-objs := omap-hdmi.o
>>>  
>>>  obj-$(CONFIG_SND_OMAP_SOC) += snd-soc-omap.o
>>> diff --git a/sound/soc/omap/mcpdm.c b/sound/soc/omap/mcpdm.c
>>> deleted file mode 100644
>>> index 9746a49..0000000
>>> --- a/sound/soc/omap/mcpdm.c
>>> +++ /dev/null
>>> @@ -1,472 +0,0 @@
>>> [...]
>>> +static int omap_mcpdm_interface_event(struct snd_soc_dapm_widget *w,
>>> +		struct snd_kcontrol *kcontrol, int event)
>>> +{
>>> +	struct omap_mcpdm *mcpdm = w->private_data;
>>
>> Can't you use
>> struct omap_mcpdm *mcpdm = snd_soc_codec_get_drvdata(codec)?
> 
> sorry:
> struct omap_mcpdm *mcpdm =  snd_soc_platform_get_drvdata(w->platform);
> 
> 
>>
>> This would avoid having to add the private_data field to the widget struct. In
>> it's current form it will only really work, if there is just one instance of
>> the driver using the widget. And if that's the case you can use a global
>> variable directly anyway.

This wont work as the McPDM DAI is neither a codec or a platform driver. It's best to give the widgets some private data since we may have other uses for it too.

Thanks

Liam

Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Lars-Peter Clausen]

On 08/19/2011 02:38 PM, Liam Girdwood wrote:
> On 19/08/11 09:49, Lars-Peter Clausen wrote:
>> On 08/19/2011 10:38 AM, Lars-Peter Clausen wrote:
>>> On 08/19/2011 09:41 AM, Peter Ujfalusi wrote:
>>>> From: Misael Lopez Cruz <misael.lopez@ti.com>
>>>>
>>>> Reasons for the replacement:
>>>> The current driver for McPDM was developed to support the legacy mode only.
>>>> In preparation for the ABE support the current driver stack need the be
>>>> replaced.
>>>> The new driver is much simpler, easier to extend, and it also fixes some of the
>>>> issues with the old stack.
>>>>
>>>> Main changes:
>>>> - single file for omap-mcpdm (mcpdm.c/h removed)
>>>> - Define names for registers, bits cleaned up, prefixed
>>>> - Full-duplex audio operation (arecord | aplay) has been fixed
>>>> - Less code
>>>>
>>>> McPDM need to be turned off _after_ the codec's DAC/ADC has been powered down.
>>>> The solution for this without extensive change in core:
>>>> Use DAPM_SUPPLY to turn off the McPDM interface.
>>>> The machine driver connects the "OMAP McPDM Interface" to the codec's DAC/ADC,
>>>> so after audio activity we can be sure that the host side McPDM is turned off
>>>> at the correct time.
>>>>
>>>> Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
>>>> Signed-off-by: Liam Girdwood <lrg@ti.com>
>>>> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
>>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>>> ---
>>>>  sound/soc/omap/Makefile     |    2 +-
>>>>  sound/soc/omap/mcpdm.c      |  472 -------------------------
>>>>  sound/soc/omap/mcpdm.h      |  152 --------
>>>>  sound/soc/omap/omap-mcpdm.c |  823 ++++++++++++++++++++++++++++---------------
>>>>  sound/soc/omap/omap-mcpdm.h |   97 +++++
>>>>  sound/soc/omap/sdp4430.c    |   14 +-
>>>>  6 files changed, 655 insertions(+), 905 deletions(-)
>>>>  delete mode 100644 sound/soc/omap/mcpdm.c
>>>>  delete mode 100644 sound/soc/omap/mcpdm.h
>>>>  rewrite sound/soc/omap/omap-mcpdm.c (46%)
>>>>  create mode 100644 sound/soc/omap/omap-mcpdm.h
>>>>
>>>> diff --git a/sound/soc/omap/Makefile b/sound/soc/omap/Makefile
>>>> index 59e2c8d..052fd75 100644
>>>> --- a/sound/soc/omap/Makefile
>>>> +++ b/sound/soc/omap/Makefile
>>>> @@ -1,7 +1,7 @@
>>>>  # OMAP Platform Support
>>>>  snd-soc-omap-objs := omap-pcm.o
>>>>  snd-soc-omap-mcbsp-objs := omap-mcbsp.o
>>>> -snd-soc-omap-mcpdm-objs := omap-mcpdm.o mcpdm.o
>>>> +snd-soc-omap-mcpdm-objs := omap-mcpdm.o
>>>>  snd-soc-omap-hdmi-objs := omap-hdmi.o
>>>>  
>>>>  obj-$(CONFIG_SND_OMAP_SOC) += snd-soc-omap.o
>>>> diff --git a/sound/soc/omap/mcpdm.c b/sound/soc/omap/mcpdm.c
>>>> deleted file mode 100644
>>>> index 9746a49..0000000
>>>> --- a/sound/soc/omap/mcpdm.c
>>>> +++ /dev/null
>>>> @@ -1,472 +0,0 @@
>>>> [...]
>>>> +static int omap_mcpdm_interface_event(struct snd_soc_dapm_widget *w,
>>>> +		struct snd_kcontrol *kcontrol, int event)
>>>> +{
>>>> +	struct omap_mcpdm *mcpdm = w->private_data;
>>>
>>> Can't you use
>>> struct omap_mcpdm *mcpdm = snd_soc_codec_get_drvdata(codec)?
>>
>> sorry:
>> struct omap_mcpdm *mcpdm =  snd_soc_platform_get_drvdata(w->platform);
>>
>>
>>>
>>> This would avoid having to add the private_data field to the widget struct. In
>>> it's current form it will only really work, if there is just one instance of
>>> the driver using the widget. And if that's the case you can use a global
>>> variable directly anyway.
> 
> This wont work as the McPDM DAI is neither a codec or a platform driver. It's best to give the widgets some private data since we may have other uses for it too.
> 
oops, sorry. I somehow though this was a platform driver, don't know why. But
still the way you use private_data in this driver, you use it to cleverly hide
a global variable.

The whole thing is sort of a hack.  I mean, if it turns out that we want DAPM
on the SoC DAI side, we should probably give it is own DAPM context, instead of
using the cards context for this.

- Lars

Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Liam Girdwood]

On 19/08/11 14:13, Lars-Peter Clausen wrote:
> On 08/19/2011 02:38 PM, Liam Girdwood wrote:
>> On 19/08/11 09:49, Lars-Peter Clausen wrote:
>>> On 08/19/2011 10:38 AM, Lars-Peter Clausen wrote:
>>>> On 08/19/2011 09:41 AM, Peter Ujfalusi wrote:
>>>>> From: Misael Lopez Cruz <misael.lopez@ti.com>
>>>>>
>>>>> Reasons for the replacement:
>>>>> The current driver for McPDM was developed to support the legacy mode only.
>>>>> In preparation for the ABE support the current driver stack need the be
>>>>> replaced.
>>>>> The new driver is much simpler, easier to extend, and it also fixes some of the
>>>>> issues with the old stack.
>>>>>
>>>>> Main changes:
>>>>> - single file for omap-mcpdm (mcpdm.c/h removed)
>>>>> - Define names for registers, bits cleaned up, prefixed
>>>>> - Full-duplex audio operation (arecord | aplay) has been fixed
>>>>> - Less code
>>>>>
>>>>> McPDM need to be turned off _after_ the codec's DAC/ADC has been powered down.
>>>>> The solution for this without extensive change in core:
>>>>> Use DAPM_SUPPLY to turn off the McPDM interface.
>>>>> The machine driver connects the "OMAP McPDM Interface" to the codec's DAC/ADC,
>>>>> so after audio activity we can be sure that the host side McPDM is turned off
>>>>> at the correct time.
>>>>>
>>>>> Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
>>>>> Signed-off-by: Liam Girdwood <lrg@ti.com>
>>>>> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
>>>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>>>> ---
>>>>>  sound/soc/omap/Makefile     |    2 +-
>>>>>  sound/soc/omap/mcpdm.c      |  472 -------------------------
>>>>>  sound/soc/omap/mcpdm.h      |  152 --------
>>>>>  sound/soc/omap/omap-mcpdm.c |  823 ++++++++++++++++++++++++++++---------------
>>>>>  sound/soc/omap/omap-mcpdm.h |   97 +++++
>>>>>  sound/soc/omap/sdp4430.c    |   14 +-
>>>>>  6 files changed, 655 insertions(+), 905 deletions(-)
>>>>>  delete mode 100644 sound/soc/omap/mcpdm.c
>>>>>  delete mode 100644 sound/soc/omap/mcpdm.h
>>>>>  rewrite sound/soc/omap/omap-mcpdm.c (46%)
>>>>>  create mode 100644 sound/soc/omap/omap-mcpdm.h
>>>>>
>>>>> diff --git a/sound/soc/omap/Makefile b/sound/soc/omap/Makefile
>>>>> index 59e2c8d..052fd75 100644
>>>>> --- a/sound/soc/omap/Makefile
>>>>> +++ b/sound/soc/omap/Makefile
>>>>> @@ -1,7 +1,7 @@
>>>>>  # OMAP Platform Support
>>>>>  snd-soc-omap-objs := omap-pcm.o
>>>>>  snd-soc-omap-mcbsp-objs := omap-mcbsp.o
>>>>> -snd-soc-omap-mcpdm-objs := omap-mcpdm.o mcpdm.o
>>>>> +snd-soc-omap-mcpdm-objs := omap-mcpdm.o
>>>>>  snd-soc-omap-hdmi-objs := omap-hdmi.o
>>>>>  
>>>>>  obj-$(CONFIG_SND_OMAP_SOC) += snd-soc-omap.o
>>>>> diff --git a/sound/soc/omap/mcpdm.c b/sound/soc/omap/mcpdm.c
>>>>> deleted file mode 100644
>>>>> index 9746a49..0000000
>>>>> --- a/sound/soc/omap/mcpdm.c
>>>>> +++ /dev/null
>>>>> @@ -1,472 +0,0 @@
>>>>> [...]
>>>>> +static int omap_mcpdm_interface_event(struct snd_soc_dapm_widget *w,
>>>>> +		struct snd_kcontrol *kcontrol, int event)
>>>>> +{
>>>>> +	struct omap_mcpdm *mcpdm = w->private_data;
>>>>
>>>> Can't you use
>>>> struct omap_mcpdm *mcpdm = snd_soc_codec_get_drvdata(codec)?
>>>
>>> sorry:
>>> struct omap_mcpdm *mcpdm =  snd_soc_platform_get_drvdata(w->platform);
>>>
>>>
>>>>
>>>> This would avoid having to add the private_data field to the widget struct. In
>>>> it's current form it will only really work, if there is just one instance of
>>>> the driver using the widget. And if that's the case you can use a global
>>>> variable directly anyway.
>>
>> This wont work as the McPDM DAI is neither a codec or a platform driver. It's best to give the widgets some private data since we may have other uses for it too.
>>
> oops, sorry. I somehow though this was a platform driver, don't know why. But
> still the way you use private_data in this driver, you use it to cleverly hide
> a global variable.
> 
> The whole thing is sort of a hack.  I mean, if it turns out that we want DAPM
> on the SoC DAI side, we should probably give it is own DAPM context, instead of
> using the cards context for this.
> 

A SoC DAI can be represented within DAPM already with the AIF widget, so adding DAI context should be fine.

Liam

> - Lars

Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Péter Ujfalusi]

On Friday 19 August 2011 10:49:08 Lars-Peter Clausen wrote:
> sorry:
> struct omap_mcpdm *mcpdm =  snd_soc_platform_get_drvdata(w->platform);
> 
> > This would avoid having to add the private_data field to the widget
> > struct. In it's current form it will only really work, if there is just
> > one instance of the driver using the widget. And if that's the case you
> > can use a global variable directly anyway.

The other option was to use global variable to get the *mcpdm for the widget.
Generally I try to avoid global variables as much as I can, since the use of 
them looks really hackish.
There could be some way to reach the CPU dai via pointers/lookups from the 
widget, but it does looked really ugly, and I was only half way there.

-- 
Péter

Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Mark Brown]

On Mon, Aug 22, 2011 at 02:34:23PM +0300, Péter Ujfalusi wrote:
> On Friday 19 August 2011 10:49:08 Lars-Peter Clausen wrote:
> > sorry:
> > struct omap_mcpdm *mcpdm =  snd_soc_platform_get_drvdata(w->platform);

> > > This would avoid having to add the private_data field to the widget
> > > struct. In it's current form it will only really work, if there is just
> > > one instance of the driver using the widget. And if that's the case you
> > > can use a global variable directly anyway.

> The other option was to use global variable to get the *mcpdm for the widget.
> Generally I try to avoid global variables as much as I can, since the use of 
> them looks really hackish.
> There could be some way to reach the CPU dai via pointers/lookups from the 
> widget, but it does looked really ugly, and I was only half way there.

The widget will have a DAPM context associated with it, should we
perhaps add this facility to the DAPM context where it'd then work for
everything?  If we're getting fancy the context could look up the CODEC
context rather than use a local one if it's for a CODEC.

Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Lars-Peter Clausen]

On 08/22/2011 01:34 PM, Péter Ujfalusi wrote:
> On Friday 19 August 2011 10:49:08 Lars-Peter Clausen wrote:
>> sorry:
>> struct omap_mcpdm *mcpdm =  snd_soc_platform_get_drvdata(w->platform);
>>
>>> This would avoid having to add the private_data field to the widget
>>> struct. In it's current form it will only really work, if there is just
>>> one instance of the driver using the widget. And if that's the case you
>>> can use a global variable directly anyway.
> 
> The other option was to use global variable to get the *mcpdm for the widget.
> Generally I try to avoid global variables as much as I can, since the use of 
> them looks really hackish.

omap_mcpdm_widgets is a global variable. You assign to it in asoc_mcpdm_probe
and read from it in omap_mcpdm_add_dapm_widget. The fact that you hide your
*mcpdm in a void pointer doesn't make it less hackish.

Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Péter Ujfalusi]

On Monday 22 August 2011 15:39:15 Lars-Peter Clausen wrote:
> omap_mcpdm_widgets is a global variable.

Yeah, as most of the snd_soc_dapm_widget.

> You assign to it in asoc_mcpdm_probe

Since at compile time I don't have the pointer for the mcpdm (it is allocated 
earlier in the same function), I need to assign it somewhere.

> and read from it in omap_mcpdm_add_dapm_widget.

I don't see any reference to the omap_mcpdm_widgets in there.

> The fact that you hide your *mcpdm in a void pointer doesn't make it less
> hackish.

Well, from that point of view most of the kernel is hackish. We tend to have 
void pointers for various things, like platform_data, device_data, 
driver_data, private_data, etc.
You see, the point here is that this private_data for the widget can be used 
for others as well, if needed. It would make no sense to put "struct 
omap_mcpdm *mcpdm", just because I have this requirement first, does it? 

For sure, I could have chosen to create one global pointer for this event 
handler:

static struct omap_mcpdm *mcpdm_global;

Use the mcpdm_global within omap_mcpdm_interface_event function, and assign it 
at asoc_mcpdm_probe time.

Would that look better? IMHO it is not.

As Mark suggested, we should have accessor for this, if we are going to have 
such a thing.

The reason for this (as I described in the commit message):
OMAP McPDM has the requirement (along with the twl6040), that we need to stop 
the cpu dai _after_ the DAC/ADC has been stopped on the codec side.
Now, I do not want to hard wire the twl6040, nor the omap-mcpdm with each 
other.
The omap-mcpdm implements the event callback, and the machine driver is 
responsible to create the DAPM route to make sure we follow the correct 
sequence.
I could as well implemented these things in the machine driver (+global 
pointer for mcpdm), but that can lead duplicated code in the future (new 
machine driver, etc).

-- 
Péter

Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Lars-Peter Clausen]

On 08/23/2011 08:49 AM, Péter Ujfalusi wrote:
> On Monday 22 August 2011 15:39:15 Lars-Peter Clausen wrote:
>> omap_mcpdm_widgets is a global variable.
> 
> Yeah, as most of the snd_soc_dapm_widget.
> 

The point is, you use it to pass runtime specific data around, while the others
are constant compile time data, which are used as a template.

>> You assign to it in asoc_mcpdm_probe
> 
> Since at compile time I don't have the pointer for the mcpdm (it is allocated 
> earlier in the same function), I need to assign it somewhere.
> 
>> and read from it in omap_mcpdm_add_dapm_widget.
> 
> I don't see any reference to the omap_mcpdm_widgets in there.

+	return snd_soc_dapm_new_controls(dapm, omap_mcpdm_widgets,
+					ARRAY_SIZE(omap_mcpdm_widgets));


> 
>> The fact that you hide your *mcpdm in a void pointer doesn't make it less
>> hackish.
> 
> Well, from that point of view most of the kernel is hackish. We tend to have 
> void pointers for various things, like platform_data, device_data, 
> driver_data, private_data, etc.

I'm not arguing against such constructs. I'm arguing against your usage of them.

Let me give you an example which is analogous to yours:

static struct platform_device foo;

static void bar_probe(struct platform_device *pdev)
{
	foo.dev.platform_data = ...;
}

void bar_some_global_func(void)
{
	platform_device_add(&foo);
}

You'll rarely see this in driver code.

If that doesn't convince you, ask yourself what would happen if you had two
instances of the mcpdm driver.

> You see, the point here is that this private_data for the widget can be used 
> for others as well, if needed. It would make no sense to put "struct 
> omap_mcpdm *mcpdm", just because I have this requirement first, does it? 
> 
> For sure, I could have chosen to create one global pointer for this event 
> handler:
> 
> static struct omap_mcpdm *mcpdm_global;
> 
> Use the mcpdm_global within omap_mcpdm_interface_event function, and assign it 
> at asoc_mcpdm_probe time.
> 
> Would that look better? IMHO it is not.
> 

Your current solution might look better on the surface, but it is deep ugly on
the inside. You've hidden your mcpdm_global in a construct that is normally
present in a ASoC driver. You've just slightly changed it in subtitle way,
apparently so subtitle you don't even see it yourself.

Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Péter Ujfalusi]

Hi,

On Tuesday 23 August 2011 12:14:58 Lars-Peter Clausen wrote:
> If that doesn't convince you, ask yourself what would happen if you had two
> instances of the mcpdm driver.

I see what you mean.
I guess I was blinded by the fact that we only have one instance of the McPDM 
driver.
Yeah, you are right this was really bad design.
I have reworked it for v3.

Thanks,
Péter

Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Paul Menzel]

[-- Attachment #1.1: Type: text/plain, Size: 371 bytes --]

Am Freitag, den 19.08.2011, 10:41 +0300 schrieb Peter Ujfalusi:
> From: Misael Lopez Cruz <misael.lopez@ti.com>
> 
> Reasons for the replacement:
> The current driver for McPDM was developed to support the legacy mode only.
> In preparation for the ABE support the current driver stack need the be
> replaced.

s/need the/needs to/

[…]


Thanks,

Paul

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Mark Brown]

On Fri, Aug 19, 2011 at 10:41:19AM +0300, Peter Ujfalusi wrote:

> +/*
> + * DAPM event function to ensure, that the host side McPDM interface is turned
> + * off after the codec's DAC.
> + */
> +static int omap_mcpdm_interface_event(struct snd_soc_dapm_widget *w,
> +		struct snd_kcontrol *kcontrol, int event)

So, the issue here is that the CODEC you're using on your system lacks
DAC mute support and doesn't handle the end of the input stream
gracefully?  This doesn't sound like a McPDM specific issue at all, and
I'm slightly surprised we don't run into it more often.

Currently the sequence we use on stream teardown is:

  1. Mute.
  2. Stop stream.
  3. Wait for the DAPM teardown time.
  4. Power down.

but it seems like what your CODEC actually wants is:

  1. Power down.
  2. Stop stream.

which isn't at all unresonable and if the CODEC is actually able to
support that mode of operation well then it'll be lower power.  This
seems like something we should be supporting in the core as I would
expect other devices will find it useful, PDM class D speaker drivers
being the most obvious example.

I do think it'd be helpful to split this code out as a separate patch
as it's the controversial bit...

Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Péter Ujfalusi]

Hi Mark,

On Saturday 20 August 2011 09:01:14 Mark Brown wrote:
> So, the issue here is that the CODEC you're using on your system lacks
> DAC mute support and doesn't handle the end of the input stream
> gracefully?  This doesn't sound like a McPDM specific issue at all, and
> I'm slightly surprised we don't run into it more often.
> 
> Currently the sequence we use on stream teardown is:
> 
>   1. Mute.
>   2. Stop stream.
>   3. Wait for the DAPM teardown time.
>   4. Power down.
> 
> but it seems like what your CODEC actually wants is:
> 
>   1. Power down.
>   2. Stop stream.

Something like that, yes.

> which isn't at all unresonable and if the CODEC is actually able to
> support that mode of operation well then it'll be lower power.  This
> seems like something we should be supporting in the core as I would
> expect other devices will find it useful, PDM class D speaker drivers
> being the most obvious example.

That's good, if we have other users with similar requirements towards the 
sequencing.
One thing, which might need special care in the down sequence:

1. Stop platform (DMA)
2. Power down (mostly codec side)
3. Stop cpu dai

I'm not sure about this, but we should not have running DMA after 
trigger:stop?

> I do think it'd be helpful to split this code out as a separate patch
> as it's the controversial bit...

It is not that easy.
There's no incremental way from the old driver to the new one.
What I can try however is to write an intermediate driver, which does not have 
the delayed sequencing. I know that this is a bit problematic, and it is not 
going to work as good as the driver in this series, but probably it will give 
the needed separation of the sequencing part.
This going to take some time, since I need to - kind of - write a new driver, 
which is in half way between the two versions ;)

--
Péter

Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Mark Brown]

On Tue, Aug 23, 2011 at 10:52:03AM +0300, Péter Ujfalusi wrote:

> 1. Stop platform (DMA)
> 2. Power down (mostly codec side)
> 3. Stop cpu dai

> I'm not sure about this, but we should not have running DMA after 
> trigger:stop?

I'd expect that to stop the DMA.  I need to think about this properly
but I think the main trick is keeping the CPU DAI active for longer than
we would normally.

Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Péter Ujfalusi]

Hi Mark,

On Tuesday 23 August 2011 09:52:03 Ujfalusi, Peter wrote:
> > I do think it'd be helpful to split this code out as a separate patch
> > as it's the controversial bit...
> 
> It is not that easy.
> There's no incremental way from the old driver to the new one.
> What I can try however is to write an intermediate driver, which does not
> have the delayed sequencing. I know that this is a bit problematic, and it
> is not going to work as good as the driver in this series, but probably it
> will give the needed separation of the sequencing part.
> This going to take some time, since I need to - kind of - write a new
> driver, which is in half way between the two versions ;)

I have looked at the possibility of separating the controversial part from the 
rest of the change, but it does not look feasible.
I would need to write a driver to fill the gap (well a hack driver for the 
time of one commit), and even in that way the patch which adds the 
'controversial' bit is going to contain quite a bit of code, since I need to 
rework other parts of the driver.
I think the improved commit message contains enough information on the change.

Thanks,
Péter