[PATCH 00/14] SPDIF support

[PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

This is my latest set of patches to get SPDIF working on the Dove Cubox
platform.  As per the last time, I've left out the "card" part of this,
because that is only useful for non-DT setups, which aren't supported in
mainline.

The first seven patches have already been applied by Mark, and are
included here for completeness by anyone who wishes to test this series.

There is very little change between this and the previous set of patches.

The HACK patch is gone, as I've found out through much digging and
experimentation with DAPM (inspite of the complete lack of assistance
from ASoC developers) how to use the stream widgets without the widget
overwriting bug getting in the way.

The CPU DAI widgets are attached to the CPU DAI context, and their
access to the driver private data updated accordingly.  Why attaching
them to the CPU DAI context previously didn't work, I'm not entirely
sure, but that certainly previously failed in testing (it may have been
due to the duplicated DAI routes and/or the lack of output pin on the
SPDIF tranceiver.)

I've added three patches: one to add IEC958 channel status setting,
another to prefer the external clock over the internal clock, and
removal of the IEC958 subframe support (see the commit message for
that patch why this can not be supported.)

This is a non-DPCM driver, and is the absolute minimum solution which
can provide correct hardware behaviour while still being conformant
with single zImage kernels and not causing any known regressions.

The same conditions apply as per my previous posting - the DAI link
needs to be setup and the associated DAPM routes to tell the CPU DAI
which outputs are in use, like this:

DAI link:
                .name = "S/PDIF1",
                .stream_name = "IEC958 Playback",
                .platform_name = "mvebu-audio.1",
                .cpu_dai_name = "mvebu-audio.1",
                .codec_dai_name = "dit-hifi",
                .codec_name = "spdif-dit",

static const struct snd_soc_dapm_route routes[] = {
        { "Playback", NULL, "spdifdo" },
};

When DPCM is eventually implemented, the DAPM route remains, the DAI link
becomes two, as previously illustrated in other discussions.

Full diffstat for this series:

 arch/arm/mach-dove/common.c          |    4 +-
 arch/arm/mach-kirkwood/common.c      |   24 ++---
 sound/soc/codecs/spdif_transciever.c |   15 +++-
 sound/soc/kirkwood/Kconfig           |    5 -
 sound/soc/kirkwood/Makefile          |    4 +-
 sound/soc/kirkwood/kirkwood-dma.c    |  114 ++++--------------
 sound/soc/kirkwood/kirkwood-i2s.c    |  213 +++++++++++++++++++++++++++++-----
 sound/soc/kirkwood/kirkwood-openrd.c |   12 ++-
 sound/soc/kirkwood/kirkwood-t5325.c  |    8 +-
 sound/soc/kirkwood/kirkwood.h        |   45 +++++++-
 sound/soc/soc-dapm.c                 |    2 +-
 11 files changed, 295 insertions(+), 151 deletions(-)

Diffstat for the patches remaining for mainline:

 sound/soc/kirkwood/kirkwood-dma.c    |    4 +-
 sound/soc/kirkwood/kirkwood-i2s.c    |  170 +++++++++++++++++++++++++++++++---
 sound/soc/kirkwood/kirkwood-openrd.c |    8 ++
 sound/soc/kirkwood/kirkwood-t5325.c  |    4 +
 sound/soc/kirkwood/kirkwood.h        |   34 +++++++
 5 files changed, 205 insertions(+), 15 deletions(-)

[PATCH 01/14] ASoC: kirkwood: merge struct kirkwood_dma_priv with struct kirkwood_dma_data

[Russell King]

Merge these two structures together; nothing other than the I2S and
DMA driver makes use of struct kirkwood_dma_data, and it's not like
struct kirkwood_dma_data is really just used to convey DMA specific
data to the backend; it's more a general shared structure between the
two halves.

This will later allow kirkwood-dma.c and kirkwood-i2s.c to be merged
together.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 sound/soc/kirkwood/kirkwood-dma.c |   76 +++++++++++--------------------------
 sound/soc/kirkwood/kirkwood.h     |    2 +
 2 files changed, 24 insertions(+), 54 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c
index d3d4bdc..112c262 100644
--- a/sound/soc/kirkwood/kirkwood-dma.c
+++ b/sound/soc/kirkwood/kirkwood-dma.c
@@ -33,11 +33,11 @@
 	 SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE | \
 	 SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE)
 
-struct kirkwood_dma_priv {
-	struct snd_pcm_substream *play_stream;
-	struct snd_pcm_substream *rec_stream;
-	struct kirkwood_dma_data *data;
-};
+static struct kirkwood_dma_data *kirkwood_priv(struct snd_pcm_substream *subs)
+{
+	struct snd_soc_pcm_runtime *soc_runtime = subs->private_data;
+	return snd_soc_dai_get_drvdata(soc_runtime->cpu_dai);
+}
 
 static struct snd_pcm_hardware kirkwood_dma_snd_hw = {
 	.info = (SNDRV_PCM_INFO_INTERLEAVED |
@@ -63,8 +63,7 @@ static u64 kirkwood_dma_dmamask = DMA_BIT_MASK(32);
 
 static irqreturn_t kirkwood_dma_irq(int irq, void *dev_id)
 {
-	struct kirkwood_dma_priv *prdata = dev_id;
-	struct kirkwood_dma_data *priv = prdata->data;
+	struct kirkwood_dma_data *priv = dev_id;
 	unsigned long mask, status, cause;
 
 	mask = readl(priv->io + KIRKWOOD_INT_MASK);
@@ -89,10 +88,10 @@ static irqreturn_t kirkwood_dma_irq(int irq, void *dev_id)
 	writel(status, priv->io + KIRKWOOD_INT_CAUSE);
 
 	if (status & KIRKWOOD_INT_CAUSE_PLAY_BYTES)
-		snd_pcm_period_elapsed(prdata->play_stream);
+		snd_pcm_period_elapsed(priv->substream_play);
 
 	if (status & KIRKWOOD_INT_CAUSE_REC_BYTES)
-		snd_pcm_period_elapsed(prdata->rec_stream);
+		snd_pcm_period_elapsed(priv->substream_rec);
 
 	return IRQ_HANDLED;
 }
@@ -126,15 +125,10 @@ static int kirkwood_dma_open(struct snd_pcm_substream *substream)
 {
 	int err;
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	struct snd_soc_pcm_runtime *soc_runtime = substream->private_data;
-	struct snd_soc_platform *platform = soc_runtime->platform;
-	struct snd_soc_dai *cpu_dai = soc_runtime->cpu_dai;
-	struct kirkwood_dma_data *priv;
-	struct kirkwood_dma_priv *prdata = snd_soc_platform_get_drvdata(platform);
+	struct kirkwood_dma_data *priv = kirkwood_priv(substream);
 	const struct mbus_dram_target_info *dram;
 	unsigned long addr;
 
-	priv = snd_soc_dai_get_dma_data(cpu_dai, substream);
 	snd_soc_set_runtime_hwparams(substream, &kirkwood_dma_snd_hw);
 
 	/* Ensure that all constraints linked to dma burst are fulfilled */
@@ -157,21 +151,11 @@ static int kirkwood_dma_open(struct snd_pcm_substream *substream)
 	if (err < 0)
 		return err;
 
-	if (prdata == NULL) {
-		prdata = kzalloc(sizeof(struct kirkwood_dma_priv), GFP_KERNEL);
-		if (prdata == NULL)
-			return -ENOMEM;
-
-		prdata->data = priv;
-
+	if (!priv->substream_play && !priv->substream_rec) {
 		err = request_irq(priv->irq, kirkwood_dma_irq, IRQF_SHARED,
-				  "kirkwood-i2s", prdata);
-		if (err) {
-			kfree(prdata);
+				  "kirkwood-i2s", priv);
+		if (err)
 			return -EBUSY;
-		}
-
-		snd_soc_platform_set_drvdata(platform, prdata);
 
 		/*
 		 * Enable Error interrupts. We're only ack'ing them but
@@ -183,11 +167,11 @@ static int kirkwood_dma_open(struct snd_pcm_substream *substream)
 	dram = mv_mbus_dram_info();
 	addr = substream->dma_buffer.addr;
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		prdata->play_stream = substream;
+		priv->substream_play = substream;
 		kirkwood_dma_conf_mbus_windows(priv->io,
 			KIRKWOOD_PLAYBACK_WIN, addr, dram);
 	} else {
-		prdata->rec_stream = substream;
+		priv->substream_rec = substream;
 		kirkwood_dma_conf_mbus_windows(priv->io,
 			KIRKWOOD_RECORD_WIN, addr, dram);
 	}
@@ -197,27 +181,19 @@ static int kirkwood_dma_open(struct snd_pcm_substream *substream)
 
 static int kirkwood_dma_close(struct snd_pcm_substream *substream)
 {
-	struct snd_soc_pcm_runtime *soc_runtime = substream->private_data;
-	struct snd_soc_dai *cpu_dai = soc_runtime->cpu_dai;
-	struct snd_soc_platform *platform = soc_runtime->platform;
-	struct kirkwood_dma_priv *prdata = snd_soc_platform_get_drvdata(platform);
-	struct kirkwood_dma_data *priv;
-
-	priv = snd_soc_dai_get_dma_data(cpu_dai, substream);
+	struct kirkwood_dma_data *priv = kirkwood_priv(substream);
 
-	if (!prdata || !priv)
+	if (!priv)
 		return 0;
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-		prdata->play_stream = NULL;
+		priv->substream_play = NULL;
 	else
-		prdata->rec_stream = NULL;
+		priv->substream_rec = NULL;
 
-	if (!prdata->play_stream && !prdata->rec_stream) {
+	if (!priv->substream_play && !priv->substream_rec) {
 		writel(0, priv->io + KIRKWOOD_ERR_MASK);
-		free_irq(priv->irq, prdata);
-		kfree(prdata);
-		snd_soc_platform_set_drvdata(platform, NULL);
+		free_irq(priv->irq, priv);
 	}
 
 	return 0;
@@ -243,13 +219,9 @@ static int kirkwood_dma_hw_free(struct snd_pcm_substream *substream)
 static int kirkwood_dma_prepare(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	struct snd_soc_pcm_runtime *soc_runtime = substream->private_data;
-	struct snd_soc_dai *cpu_dai = soc_runtime->cpu_dai;
-	struct kirkwood_dma_data *priv;
+	struct kirkwood_dma_data *priv = kirkwood_priv(substream);
 	unsigned long size, count;
 
-	priv = snd_soc_dai_get_dma_data(cpu_dai, substream);
-
 	/* compute buffer size in term of "words" as requested in specs */
 	size = frames_to_bytes(runtime, runtime->buffer_size);
 	size = (size>>2)-1;
@@ -272,13 +244,9 @@ static int kirkwood_dma_prepare(struct snd_pcm_substream *substream)
 static snd_pcm_uframes_t kirkwood_dma_pointer(struct snd_pcm_substream
 						*substream)
 {
-	struct snd_soc_pcm_runtime *soc_runtime = substream->private_data;
-	struct snd_soc_dai *cpu_dai = soc_runtime->cpu_dai;
-	struct kirkwood_dma_data *priv;
+	struct kirkwood_dma_data *priv = kirkwood_priv(substream);
 	snd_pcm_uframes_t count;
 
-	priv = snd_soc_dai_get_dma_data(cpu_dai, substream);
-
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 		count = bytes_to_frames(substream->runtime,
 			readl(priv->io + KIRKWOOD_PLAY_BYTE_COUNT));
diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h
index 4d92637..10a3aaa 100644
--- a/sound/soc/kirkwood/kirkwood.h
+++ b/sound/soc/kirkwood/kirkwood.h
@@ -129,6 +129,8 @@ struct kirkwood_dma_data {
 	struct clk *extclk;
 	uint32_t ctl_play;
 	uint32_t ctl_rec;
+	struct snd_pcm_substream *substream_play;
+	struct snd_pcm_substream *substream_rec;
 	int irq;
 	int burst;
 };
-- 
1.7.4.4

[PATCH 02/14] ASoC: kirkwood: use devm_clk_get() for the external clock

[Russell King]

Use devm_clk_get() to simplify the error path for the external clock.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 sound/soc/kirkwood/kirkwood-i2s.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 4c9dad3..ee27981 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -498,10 +498,10 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 	if (err < 0)
 		return err;
 
-	priv->extclk = clk_get(&pdev->dev, "extclk");
+	priv->extclk = devm_clk_get(&pdev->dev, "extclk");
 	if (!IS_ERR(priv->extclk)) {
 		if (priv->extclk == priv->clk) {
-			clk_put(priv->extclk);
+			devm_clk_put(&pdev->dev, priv->extclk);
 			priv->extclk = ERR_PTR(-EINVAL);
 		} else {
 			dev_info(&pdev->dev, "found external clock\n");
@@ -529,10 +529,8 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 		return 0;
 	dev_err(&pdev->dev, "snd_soc_register_component failed\n");
 
-	if (!IS_ERR(priv->extclk)) {
+	if (!IS_ERR(priv->extclk))
 		clk_disable_unprepare(priv->extclk);
-		clk_put(priv->extclk);
-	}
 	clk_disable_unprepare(priv->clk);
 
 	return err;
@@ -544,10 +542,8 @@ static int kirkwood_i2s_dev_remove(struct platform_device *pdev)
 
 	snd_soc_unregister_component(&pdev->dev);
 
-	if (!IS_ERR(priv->extclk)) {
+	if (!IS_ERR(priv->extclk))
 		clk_disable_unprepare(priv->extclk);
-		clk_put(priv->extclk);
-	}
 	clk_disable_unprepare(priv->clk);
 
 	return 0;
-- 
1.7.4.4

[PATCH 03/14] ASoC: avoid duplicated DAI routes

[Russell King]

ASoC automatically creates snd_soc_dapm_dai_in and snd_soc_dapm_dai_out
widgets for DAI drivers, and adds them to the list.  Later on, ASoC
creates automatic routes between these widgets and a widget with a
stream name.

We look for a snd_soc_dapm_dai_in or snd_soc_dapm_dai_out widget, and
use this to obtain the DAI structure.  We then scan all widgets for
any with a stream name refering to either the capture or the playback
stream, and create routes.

If you have both a snd_soc_dapm_dai_in and a snd_soc_dapm_dai_out
referring to the same DAI structure, this ends up creating one set of
routes for the DAI for the snd_soc_dapm_dai_in widget, and a duplicated
set of routes for the snd_soc_dapm_dai_out widget.

Fix this by checking that the stream name for the widget matches the
DAI widget name.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 sound/soc/soc-dapm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index c7051c4..9f67976 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -3439,7 +3439,7 @@ int snd_soc_dapm_link_dai_widgets(struct snd_soc_card *card)
 				break;
 			}
 
-			if (!w->sname)
+			if (!w->sname || !strstr(w->sname, dai_w->name))
 				continue;
 
 			if (dai->driver->playback.stream_name &&
-- 
1.7.4.4

[PATCH 04/14] ASoC: kirkwood: provide KIRKWOOD_PLAYCTL_ENABLE_MASK

[Russell King]

Provide a helper macro which includes the sum of all enable bits in
the playback control register.  This simplifies the code a little.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 sound/soc/kirkwood/kirkwood-i2s.c |   10 ++++------
 sound/soc/kirkwood/kirkwood.h     |    5 ++++-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index ee27981..f39e57c 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -199,8 +199,7 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
 			ctl_play |= KIRKWOOD_PLAYCTL_MONO_OFF;
 
 		priv->ctl_play &= ~(KIRKWOOD_PLAYCTL_MONO_MASK |
-				    KIRKWOOD_PLAYCTL_I2S_EN |
-				    KIRKWOOD_PLAYCTL_SPDIF_EN |
+				    KIRKWOOD_PLAYCTL_ENABLE_MASK |
 				    KIRKWOOD_PLAYCTL_SIZE_MASK);
 		priv->ctl_play |= ctl_play;
 	} else {
@@ -244,8 +243,7 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_TRIGGER_START:
 		/* configure */
 		ctl = priv->ctl_play;
-		value = ctl & ~(KIRKWOOD_PLAYCTL_I2S_EN |
-				KIRKWOOD_PLAYCTL_SPDIF_EN);
+		value = ctl & ~KIRKWOOD_PLAYCTL_ENABLE_MASK;
 		writel(value, priv->io + KIRKWOOD_PLAYCTL);
 
 		/* enable interrupts */
@@ -267,7 +265,7 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
 		writel(value, priv->io + KIRKWOOD_INT_MASK);
 
 		/* disable all playbacks */
-		ctl &= ~(KIRKWOOD_PLAYCTL_I2S_EN | KIRKWOOD_PLAYCTL_SPDIF_EN);
+		ctl &= ~KIRKWOOD_PLAYCTL_ENABLE_MASK;
 		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
 		break;
 
@@ -387,7 +385,7 @@ static int kirkwood_i2s_probe(struct snd_soc_dai *dai)
 
 	/* disable playback/record */
 	value = readl(priv->io + KIRKWOOD_PLAYCTL);
-	value &= ~(KIRKWOOD_PLAYCTL_I2S_EN|KIRKWOOD_PLAYCTL_SPDIF_EN);
+	value &= ~KIRKWOOD_PLAYCTL_ENABLE_MASK;
 	writel(value, priv->io + KIRKWOOD_PLAYCTL);
 
 	value = readl(priv->io + KIRKWOOD_RECCTL);
diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h
index 10a3aaa..9a50607 100644
--- a/sound/soc/kirkwood/kirkwood.h
+++ b/sound/soc/kirkwood/kirkwood.h
@@ -54,7 +54,7 @@
 #define KIRKWOOD_PLAYCTL_MONO_OFF		(0<<5)
 #define KIRKWOOD_PLAYCTL_I2S_MUTE		(1<<7)
 #define KIRKWOOD_PLAYCTL_SPDIF_EN		(1<<4)
-#define KIRKWOOD_PLAYCTL_I2S_EN		(1<<3)
+#define KIRKWOOD_PLAYCTL_I2S_EN			(1<<3)
 #define KIRKWOOD_PLAYCTL_SIZE_MASK		(7<<0)
 #define KIRKWOOD_PLAYCTL_SIZE_16		(7<<0)
 #define KIRKWOOD_PLAYCTL_SIZE_16_C		(3<<0)
@@ -62,6 +62,9 @@
 #define KIRKWOOD_PLAYCTL_SIZE_24		(1<<0)
 #define KIRKWOOD_PLAYCTL_SIZE_32		(0<<0)
 
+#define KIRKWOOD_PLAYCTL_ENABLE_MASK		(KIRKWOOD_PLAYCTL_SPDIF_EN | \
+						 KIRKWOOD_PLAYCTL_I2S_EN)
+
 #define KIRKWOOD_PLAY_BUF_ADDR			0x1104
 #define KIRKWOOD_PLAY_BUF_SIZE			0x1108
 #define KIRKWOOD_PLAY_BYTE_COUNT		0x110C
-- 
1.7.4.4

[PATCH 05/14] ASoC: kirkwood: combine kirkwood-i2s and kirkwood-dma drivers

[Russell King]

These really should be a single driver because they're fully integrated
in hardware.  Make them so.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/mach-dove/common.c          |    4 ++--
 arch/arm/mach-kirkwood/common.c      |   24 +++++++++---------------
 sound/soc/kirkwood/Kconfig           |    5 -----
 sound/soc/kirkwood/Makefile          |    4 +---
 sound/soc/kirkwood/kirkwood-dma.c    |   30 +-----------------------------
 sound/soc/kirkwood/kirkwood-i2s.c    |   21 ++++++++++++++++-----
 sound/soc/kirkwood/kirkwood-openrd.c |    4 ++--
 sound/soc/kirkwood/kirkwood-t5325.c  |    4 ++--
 sound/soc/kirkwood/kirkwood.h        |    2 ++
 9 files changed, 35 insertions(+), 63 deletions(-)

diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
index e2b5da0..68a2d6a 100644
--- a/arch/arm/mach-dove/common.c
+++ b/arch/arm/mach-dove/common.c
@@ -109,8 +109,8 @@ static void __init dove_clk_init(void)
 	orion_clkdev_add(NULL, "sdhci-dove.1", sdio1);
 	orion_clkdev_add(NULL, "orion_nand", nand);
 	orion_clkdev_add(NULL, "cafe1000-ccic.0", camera);
-	orion_clkdev_add(NULL, "kirkwood-i2s.0", i2s0);
-	orion_clkdev_add(NULL, "kirkwood-i2s.1", i2s1);
+	orion_clkdev_add(NULL, "mvebu-audio.0", i2s0);
+	orion_clkdev_add(NULL, "mvebu-audio.1", i2s1);
 	orion_clkdev_add(NULL, "mv_crypto", crypto);
 	orion_clkdev_add(NULL, "dove-ac97", ac97);
 	orion_clkdev_add(NULL, "dove-pdma", pdma);
diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
index f389228..d24d39d 100644
--- a/arch/arm/mach-kirkwood/common.c
+++ b/arch/arm/mach-kirkwood/common.c
@@ -263,7 +263,7 @@ void __init kirkwood_clk_init(void)
 	orion_clkdev_add(NULL, MV_XOR_NAME ".1", xor1);
 	orion_clkdev_add("0", "pcie", pex0);
 	orion_clkdev_add("1", "pcie", pex1);
-	orion_clkdev_add(NULL, "kirkwood-i2s", audio);
+	orion_clkdev_add(NULL, "mvebu-audio", audio);
 	orion_clkdev_add(NULL, MV64XXX_I2C_CTLR_NAME ".0", runit);
 	orion_clkdev_add(NULL, MV64XXX_I2C_CTLR_NAME ".1", runit);
 
@@ -559,7 +559,7 @@ void __init kirkwood_timer_init(void)
 /*****************************************************************************
  * Audio
  ****************************************************************************/
-static struct resource kirkwood_i2s_resources[] = {
+static struct resource kirkwood_audio_resources[] = {
 	[0] = {
 		.start  = AUDIO_PHYS_BASE,
 		.end    = AUDIO_PHYS_BASE + SZ_16K - 1,
@@ -572,29 +572,23 @@ static struct resource kirkwood_i2s_resources[] = {
 	},
 };
 
-static struct kirkwood_asoc_platform_data kirkwood_i2s_data = {
+static struct kirkwood_asoc_platform_data kirkwood_audio_data = {
 	.burst       = 128,
 };
 
-static struct platform_device kirkwood_i2s_device = {
-	.name		= "kirkwood-i2s",
+static struct platform_device kirkwood_audio_device = {
+	.name		= "mvebu-audio",
 	.id		= -1,
-	.num_resources	= ARRAY_SIZE(kirkwood_i2s_resources),
-	.resource	= kirkwood_i2s_resources,
+	.num_resources	= ARRAY_SIZE(kirkwood_audio_resources),
+	.resource	= kirkwood_audio_resources,
 	.dev		= {
-		.platform_data	= &kirkwood_i2s_data,
+		.platform_data	= &kirkwood_audio_data,
 	},
 };
 
-static struct platform_device kirkwood_pcm_device = {
-	.name		= "kirkwood-pcm-audio",
-	.id		= -1,
-};
-
 void __init kirkwood_audio_init(void)
 {
-	platform_device_register(&kirkwood_i2s_device);
-	platform_device_register(&kirkwood_pcm_device);
+	platform_device_register(&kirkwood_audio_device);
 }
 
 /*****************************************************************************
diff --git a/sound/soc/kirkwood/Kconfig b/sound/soc/kirkwood/Kconfig
index c62d715..b6917a7 100644
--- a/sound/soc/kirkwood/Kconfig
+++ b/sound/soc/kirkwood/Kconfig
@@ -6,14 +6,10 @@ config SND_KIRKWOOD_SOC
 	  the Kirkwood I2S interface. You will also need to select the
 	  audio interfaces to support below.
 
-config SND_KIRKWOOD_SOC_I2S
-	tristate
-
 config SND_KIRKWOOD_SOC_OPENRD
 	tristate "SoC Audio support for Kirkwood Openrd Client"
 	depends on SND_KIRKWOOD_SOC && (MACH_OPENRD_CLIENT || MACH_OPENRD_ULTIMATE)
 	depends on I2C
-	select SND_KIRKWOOD_SOC_I2S
 	select SND_SOC_CS42L51
 	help
 	  Say Y if you want to add support for SoC audio on
@@ -22,7 +18,6 @@ config SND_KIRKWOOD_SOC_OPENRD
 config SND_KIRKWOOD_SOC_T5325
 	tristate "SoC Audio support for HP t5325"
 	depends on SND_KIRKWOOD_SOC && MACH_T5325 && I2C
-	select SND_KIRKWOOD_SOC_I2S
 	select SND_SOC_ALC5623
 	help
 	  Say Y if you want to add support for SoC audio on
diff --git a/sound/soc/kirkwood/Makefile b/sound/soc/kirkwood/Makefile
index 3e62ae9..9e78138 100644
--- a/sound/soc/kirkwood/Makefile
+++ b/sound/soc/kirkwood/Makefile
@@ -1,8 +1,6 @@
-snd-soc-kirkwood-objs := kirkwood-dma.o
-snd-soc-kirkwood-i2s-objs := kirkwood-i2s.o
+snd-soc-kirkwood-objs := kirkwood-dma.o kirkwood-i2s.o
 
 obj-$(CONFIG_SND_KIRKWOOD_SOC) += snd-soc-kirkwood.o
-obj-$(CONFIG_SND_KIRKWOOD_SOC_I2S) += snd-soc-kirkwood-i2s.o
 
 snd-soc-openrd-objs := kirkwood-openrd.o
 snd-soc-t5325-objs := kirkwood-t5325.o
diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c
index 112c262..2091d55 100644
--- a/sound/soc/kirkwood/kirkwood-dma.c
+++ b/sound/soc/kirkwood/kirkwood-dma.c
@@ -334,36 +334,8 @@ static void kirkwood_dma_free_dma_buffers(struct snd_pcm *pcm)
 	}
 }
 
-static struct snd_soc_platform_driver kirkwood_soc_platform = {
+struct snd_soc_platform_driver kirkwood_soc_platform = {
 	.ops		= &kirkwood_dma_ops,
 	.pcm_new	= kirkwood_dma_new,
 	.pcm_free	= kirkwood_dma_free_dma_buffers,
 };
-
-static int kirkwood_soc_platform_probe(struct platform_device *pdev)
-{
-	return snd_soc_register_platform(&pdev->dev, &kirkwood_soc_platform);
-}
-
-static int kirkwood_soc_platform_remove(struct platform_device *pdev)
-{
-	snd_soc_unregister_platform(&pdev->dev);
-	return 0;
-}
-
-static struct platform_driver kirkwood_pcm_driver = {
-	.driver = {
-			.name = "kirkwood-pcm-audio",
-			.owner = THIS_MODULE,
-	},
-
-	.probe = kirkwood_soc_platform_probe,
-	.remove = kirkwood_soc_platform_remove,
-};
-
-module_platform_driver(kirkwood_pcm_driver);
-
-MODULE_AUTHOR("Arnaud Patard <arnaud.patard@rtp-net.org>");
-MODULE_DESCRIPTION("Marvell Kirkwood Audio DMA module");
-MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:kirkwood-pcm-audio");
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index f39e57c..84dd9b0 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -24,7 +24,7 @@
 #include <linux/platform_data/asoc-kirkwood.h>
 #include "kirkwood.h"
 
-#define DRV_NAME	"kirkwood-i2s"
+#define DRV_NAME	"mvebu-audio"
 
 #define KIRKWOOD_I2S_RATES \
 	(SNDRV_PCM_RATE_44100 | \
@@ -523,10 +523,20 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 
 	err = snd_soc_register_component(&pdev->dev, &kirkwood_i2s_component,
 					 soc_dai, 1);
-	if (!err)
-		return 0;
-	dev_err(&pdev->dev, "snd_soc_register_component failed\n");
+	if (err) {
+		dev_err(&pdev->dev, "snd_soc_register_component failed\n");
+		goto err_component;
+	}
 
+	err = snd_soc_register_platform(&pdev->dev, &kirkwood_soc_platform);
+	if (err) {
+		dev_err(&pdev->dev, "snd_soc_register_platform failed\n");
+		goto err_platform;
+	}
+	return 0;
+ err_platform:
+	snd_soc_unregister_component(&pdev->dev);
+ err_component:
 	if (!IS_ERR(priv->extclk))
 		clk_disable_unprepare(priv->extclk);
 	clk_disable_unprepare(priv->clk);
@@ -538,6 +548,7 @@ static int kirkwood_i2s_dev_remove(struct platform_device *pdev)
 {
 	struct kirkwood_dma_data *priv = dev_get_drvdata(&pdev->dev);
 
+	snd_soc_unregister_platform(&pdev->dev);
 	snd_soc_unregister_component(&pdev->dev);
 
 	if (!IS_ERR(priv->extclk))
@@ -562,4 +573,4 @@ module_platform_driver(kirkwood_i2s_driver);
 MODULE_AUTHOR("Arnaud Patard, <arnaud.patard@rtp-net.org>");
 MODULE_DESCRIPTION("Kirkwood I2S SoC Interface");
 MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:kirkwood-i2s");
+MODULE_ALIAS("platform:mvebu-audio");
diff --git a/sound/soc/kirkwood/kirkwood-openrd.c b/sound/soc/kirkwood/kirkwood-openrd.c
index b979c71..a25dfcf 100644
--- a/sound/soc/kirkwood/kirkwood-openrd.c
+++ b/sound/soc/kirkwood/kirkwood-openrd.c
@@ -54,8 +54,8 @@ static struct snd_soc_dai_link openrd_client_dai[] = {
 {
 	.name = "CS42L51",
 	.stream_name = "CS42L51 HiFi",
-	.cpu_dai_name = "kirkwood-i2s",
-	.platform_name = "kirkwood-pcm-audio",
+	.cpu_dai_name = "mvebu-audio",
+	.platform_name = "mvebu-audio",
 	.codec_dai_name = "cs42l51-hifi",
 	.codec_name = "cs42l51-codec.0-004a",
 	.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBS_CFS,
diff --git a/sound/soc/kirkwood/kirkwood-t5325.c b/sound/soc/kirkwood/kirkwood-t5325.c
index 1d0ed6f..82a8c5f 100644
--- a/sound/soc/kirkwood/kirkwood-t5325.c
+++ b/sound/soc/kirkwood/kirkwood-t5325.c
@@ -70,8 +70,8 @@ static struct snd_soc_dai_link t5325_dai[] = {
 {
 	.name = "ALC5621",
 	.stream_name = "ALC5621 HiFi",
-	.cpu_dai_name = "kirkwood-i2s",
-	.platform_name = "kirkwood-pcm-audio",
+	.cpu_dai_name = "mvebu-audio",
+	.platform_name = "mvebu-audio",
 	.codec_dai_name = "alc5621-hifi",
 	.codec_name = "alc562x-codec.0-001a",
 	.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBS_CFS,
diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h
index 9a50607..1d13dee 100644
--- a/sound/soc/kirkwood/kirkwood.h
+++ b/sound/soc/kirkwood/kirkwood.h
@@ -138,4 +138,6 @@ struct kirkwood_dma_data {
 	int burst;
 };
 
+extern struct snd_soc_platform_driver kirkwood_soc_platform;
+
 #endif
-- 
1.7.4.4

[PATCH 06/14] ASoC: kirkwood: move calculation of max buffer size to kirkwood.h

[Russell King]

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 sound/soc/kirkwood/kirkwood-dma.c |    2 +-
 sound/soc/kirkwood/kirkwood.h     |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c
index 2091d55..ee31016 100644
--- a/sound/soc/kirkwood/kirkwood-dma.c
+++ b/sound/soc/kirkwood/kirkwood-dma.c
@@ -51,7 +51,7 @@ static struct snd_pcm_hardware kirkwood_dma_snd_hw = {
 	.rate_max		= 384000,
 	.channels_min		= 1,
 	.channels_max		= 8,
-	.buffer_bytes_max	= KIRKWOOD_SND_MAX_PERIOD_BYTES * KIRKWOOD_SND_MAX_PERIODS,
+	.buffer_bytes_max	= KIRKWOOD_SND_MAX_BUFFER_BYTES,
 	.period_bytes_min	= KIRKWOOD_SND_MIN_PERIOD_BYTES,
 	.period_bytes_max	= KIRKWOOD_SND_MAX_PERIOD_BYTES,
 	.periods_min		= KIRKWOOD_SND_MIN_PERIODS,
diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h
index 1d13dee..f8e1ccc 100644
--- a/sound/soc/kirkwood/kirkwood.h
+++ b/sound/soc/kirkwood/kirkwood.h
@@ -125,6 +125,8 @@
 #define KIRKWOOD_SND_MAX_PERIODS		16
 #define KIRKWOOD_SND_MIN_PERIOD_BYTES		0x4000
 #define KIRKWOOD_SND_MAX_PERIOD_BYTES		0x4000
+#define KIRKWOOD_SND_MAX_BUFFER_BYTES		(KIRKWOOD_SND_MAX_PERIOD_BYTES \
+						 * KIRKWOOD_SND_MAX_PERIODS)
 
 struct kirkwood_dma_data {
 	void __iomem *io;
-- 
1.7.4.4

[PATCH 07/14] ASoC: spdif_transceiver: add output pin widget

[Russell King]

Codecs without any outputs now remain powered down, which means any
paths to these codecs also remain powered down.

Add an always-enabled output pin widget to the spdif transceiver codec.
This enables DAPM to correctly identify that the spdif transceiver is
in use when playback is enabled, which will then allow DAPM to power up
any links from the CPU DAI to the SPDIF transceiver.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 sound/soc/codecs/spdif_transciever.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/sound/soc/codecs/spdif_transciever.c b/sound/soc/codecs/spdif_transciever.c
index 112a49d..8df8d79 100644
--- a/sound/soc/codecs/spdif_transciever.c
+++ b/sound/soc/codecs/spdif_transciever.c
@@ -27,7 +27,20 @@
 #define STUB_FORMATS	SNDRV_PCM_FMTBIT_S16_LE
 
 
-static struct snd_soc_codec_driver soc_codec_spdif_dit;
+static const struct snd_soc_dapm_widget dit_widgets[] = {
+	SND_SOC_DAPM_OUTPUT("spdif-out"),
+};
+
+static const const struct snd_soc_dapm_route dit_routes[] = {
+	{ "spdif-out", NULL, "Playback" },
+};
+
+static struct snd_soc_codec_driver soc_codec_spdif_dit = {
+	.dapm_widgets = dit_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(dit_widgets),
+	.dapm_routes = dit_routes,
+	.num_dapm_routes = ARRAY_SIZE(dit_routes),
+};
 
 static struct snd_soc_dai_driver dit_stub_dai = {
 	.name		= "dit-hifi",
-- 
1.7.4.4

[PATCH 08/14] ASoC: kirkwood: prefer external clock over internal clock

[Russell King]

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 sound/soc/kirkwood/kirkwood-i2s.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 84dd9b0..8e10369 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -104,20 +104,20 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai,
 {
 	uint32_t clks_ctrl;
 
-	if (rate == 44100 || rate == 48000 || rate == 96000) {
-		/* use internal dco for supported rates */
-		dev_dbg(dai->dev, "%s: dco set rate = %lu\n",
-			__func__, rate);
-		kirkwood_set_dco(priv->io, rate);
-
-		clks_ctrl = KIRKWOOD_MCLK_SOURCE_DCO;
-	} else if (!IS_ERR(priv->extclk)) {
+	if (!IS_ERR(priv->extclk)) {
 		/* use optional external clk for other rates */
 		dev_dbg(dai->dev, "%s: extclk set rate = %lu -> %lu\n",
 			__func__, rate, 256 * rate);
 		clk_set_rate(priv->extclk, 256 * rate);
 
 		clks_ctrl = KIRKWOOD_MCLK_SOURCE_EXTCLK;
+	} else if (rate == 44100 || rate == 48000 || rate == 96000) {
+		/* use internal dco for supported rates */
+		dev_dbg(dai->dev, "%s: dco set rate = %lu\n",
+			__func__, rate);
+		kirkwood_set_dco(priv->io, rate);
+
+		clks_ctrl = KIRKWOOD_MCLK_SOURCE_DCO;
 	}
 	writel(clks_ctrl, priv->io + KIRKWOOD_CLOCKS_CTRL);
 }
-- 
1.7.4.4

[PATCH 08/14] ASoC: kirkwood: prefer external clock over internal clock

[Jean-Francois Moine]

On Sat, 31 Aug 2013 13:42:36 +0100
Russell King <rmk+kernel@arm.linux.org.uk> wrote:

> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  sound/soc/kirkwood/kirkwood-i2s.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
> index 84dd9b0..8e10369 100644
> --- a/sound/soc/kirkwood/kirkwood-i2s.c
> +++ b/sound/soc/kirkwood/kirkwood-i2s.c
> @@ -104,20 +104,20 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai,
>  {
>  	uint32_t clks_ctrl;
>  
> -	if (rate == 44100 || rate == 48000 || rate == 96000) {
> -		/* use internal dco for supported rates */
> -		dev_dbg(dai->dev, "%s: dco set rate = %lu\n",
> -			__func__, rate);
> -		kirkwood_set_dco(priv->io, rate);
> -
> -		clks_ctrl = KIRKWOOD_MCLK_SOURCE_DCO;
> -	} else if (!IS_ERR(priv->extclk)) {
> +	if (!IS_ERR(priv->extclk)) {
>  		/* use optional external clk for other rates */
>  		dev_dbg(dai->dev, "%s: extclk set rate = %lu -> %lu\n",
>  			__func__, rate, 256 * rate);
>  		clk_set_rate(priv->extclk, 256 * rate);
>  
>  		clks_ctrl = KIRKWOOD_MCLK_SOURCE_EXTCLK;
> +	} else if (rate == 44100 || rate == 48000 || rate == 96000) {

The rate is always good, and having this test raises a compilation warning
(clks_ctrl may be not initialized).


> +		/* use internal dco for supported rates */
> +		dev_dbg(dai->dev, "%s: dco set rate = %lu\n",
> +			__func__, rate);
> +		kirkwood_set_dco(priv->io, rate);
> +
> +		clks_ctrl = KIRKWOOD_MCLK_SOURCE_DCO;
>  	}
>  	writel(clks_ctrl, priv->io + KIRKWOOD_CLOCKS_CTRL);
>  }



-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH 08/14] ASoC: kirkwood: prefer external clock over internal clock

[Mark Brown]

On Sun, Sep 01, 2013 at 06:41:53PM +0200, Jean-Francois Moine wrote:
> Russell King <rmk+kernel@arm.linux.org.uk> wrote:

> > +	} else if (rate == 44100 || rate == 48000 || rate == 96000) {

> The rate is always good, and having this test raises a compilation warning
> (clks_ctrl may be not initialized).

That should be fixed - it might be worth keeping the test and adding an
else with an error return for robustness.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130902/1e9ebb66/attachment-0001.sig>

[PATCH 08/14] ASoC: kirkwood: prefer external clock over internal clock

[Russell King - ARM Linux]

On Mon, Sep 02, 2013 at 12:01:05PM +0100, Mark Brown wrote:
> On Sun, Sep 01, 2013 at 06:41:53PM +0200, Jean-Francois Moine wrote:
> > Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> 
> > > +	} else if (rate == 44100 || rate == 48000 || rate == 96000) {
> 
> > The rate is always good, and having this test raises a compilation warning
> > (clks_ctrl may be not initialized).
> 
> That should be fixed - it might be worth keeping the test and adding an
> else with an error return for robustness.

I believe you already have a fix merged.  I don't have that fix in my tree.

[PATCH 09/14] ASoC: kirkwood-dma: remove IEC958_SUBFRAME formats

[Russell King]

The Audio block does not support IEC958 subframes as formatted by
ALSA: they're very close, but not close enough.  The formats differ
by:

3  2   2   2   1   1
1  8   4   0   6   2   8   4   0
PCUVDDDDDDDDDDDDDDDD....AAAATTTT - IEC958 subframe
PCUV0000........DDDDDDDDDDDDDDDD - Audio block format

Where P = parity, C = channel status, U = user data, V = validity,
D = sample data, A = aux, T = preamble.  As can be seen, the
position of the sample is in a different position, and the audio
block does not have the aux or preamble bits.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 sound/soc/kirkwood/kirkwood-dma.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c
index ee31016..3c8fc75 100644
--- a/sound/soc/kirkwood/kirkwood-dma.c
+++ b/sound/soc/kirkwood/kirkwood-dma.c
@@ -29,9 +29,7 @@
 #define KIRKWOOD_FORMATS \
 	(SNDRV_PCM_FMTBIT_S16_LE | \
 	 SNDRV_PCM_FMTBIT_S24_LE | \
-	 SNDRV_PCM_FMTBIT_S32_LE | \
-	 SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE | \
-	 SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE)
+	 SNDRV_PCM_FMTBIT_S32_LE)
 
 static struct kirkwood_dma_data *kirkwood_priv(struct snd_pcm_substream *subs)
 {
-- 
1.7.4.4

[PATCH 09/14] ASoC: kirkwood-dma: remove IEC958_SUBFRAME formats

[Mark Brown]

On Sat, Aug 31, 2013 at 01:43:36PM +0100, Russell King wrote:
> The Audio block does not support IEC958 subframes as formatted by
> ALSA: they're very close, but not close enough.  The formats differ

Applied, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130902/e9924e23/attachment.sig>

[PATCH 10/14] ASoC: kirkwood: add DAPM widgets for input and output routing

[Russell King]

Add DAPM widgets for the audio unit inputs and outputs.  The SPDIF
output route must only be used if the hardware actually supports it.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 sound/soc/kirkwood/kirkwood-i2s.c |   35 ++++++++++++++++++++++++++++++++++-
 sound/soc/kirkwood/kirkwood.h     |    1 +
 2 files changed, 35 insertions(+), 1 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 8e10369..8a6ff1a 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -242,7 +242,7 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 		/* configure */
-		ctl = priv->ctl_play;
+		ctl = priv->ctl_play & priv->ctl_play_mask;
 		value = ctl & ~KIRKWOOD_PLAYCTL_ENABLE_MASK;
 		writel(value, priv->io + KIRKWOOD_PLAYCTL);
 
@@ -360,12 +360,38 @@ static int kirkwood_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 	return 0;
 }
 
+static int kirkwood_i2s_play_i2s(struct snd_soc_dapm_widget *w,
+	struct snd_kcontrol *ctl, int event)
+{
+	/* CPU DAI is not available, so use driver data from device */
+	struct kirkwood_dma_data *priv = dev_get_drvdata(w->dapm->dev);
+
+	if (SND_SOC_DAPM_EVENT_ON(event))
+		priv->ctl_play_mask |= KIRKWOOD_PLAYCTL_I2S_EN;
+	else
+		priv->ctl_play_mask &= ~KIRKWOOD_PLAYCTL_I2S_EN;
+
+	return 0;
+}
+
+static const struct snd_soc_dapm_widget widgets[] = {
+	/* These widget names come from the names from the functional spec */
+	SND_SOC_DAPM_AIF_OUT_E("i2sdo", "dma-tx",
+		0, SND_SOC_NOPM, 0, 0, kirkwood_i2s_play_i2s,
+		SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
+	SND_SOC_DAPM_AIF_IN("i2sdi", "dma-rx",
+		0, SND_SOC_NOPM, 0, 0),
+};
+
 static int kirkwood_i2s_probe(struct snd_soc_dai *dai)
 {
 	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai);
 	unsigned long value;
 	unsigned int reg_data;
 
+	/* It appears that these can't be attached to the CPU DAI */
+	snd_soc_dapm_new_controls(&dai->dapm, widgets, ARRAY_SIZE(widgets));
+
 	/* put system in a "safe" state : */
 	/* disable audio interrupts */
 	writel(0xffffffff, priv->io + KIRKWOOD_INT_CAUSE);
@@ -383,6 +409,8 @@ static int kirkwood_i2s_probe(struct snd_soc_dai *dai)
 	reg_data |= 0x111D18;
 	writel(reg_data, priv->io + 0x1200);
 
+	priv->ctl_play_mask = ~KIRKWOOD_PLAYCTL_ENABLE_MASK;
+
 	/* disable playback/record */
 	value = readl(priv->io + KIRKWOOD_PLAYCTL);
 	value &= ~KIRKWOOD_PLAYCTL_ENABLE_MASK;
@@ -413,12 +441,14 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai = {
 	.probe = kirkwood_i2s_probe,
 	.remove = kirkwood_i2s_remove,
 	.playback = {
+		.stream_name = "dma-tx",
 		.channels_min = 1,
 		.channels_max = 2,
 		.rates = KIRKWOOD_I2S_RATES,
 		.formats = KIRKWOOD_I2S_FORMATS,
 	},
 	.capture = {
+		.stream_name = "dma-rx",
 		.channels_min = 1,
 		.channels_max = 2,
 		.rates = KIRKWOOD_I2S_RATES,
@@ -431,6 +461,7 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk = {
 	.probe = kirkwood_i2s_probe,
 	.remove = kirkwood_i2s_remove,
 	.playback = {
+		.stream_name = "dma-tx",
 		.channels_min = 1,
 		.channels_max = 2,
 		.rates = SNDRV_PCM_RATE_8000_192000 |
@@ -439,6 +470,7 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk = {
 		.formats = KIRKWOOD_I2S_FORMATS,
 	},
 	.capture = {
+		.stream_name = "dma-rx",
 		.channels_min = 1,
 		.channels_max = 2,
 		.rates = SNDRV_PCM_RATE_8000_192000 |
@@ -534,6 +566,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 		goto err_platform;
 	}
 	return 0;
+
  err_platform:
 	snd_soc_unregister_component(&pdev->dev);
  err_component:
diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h
index f8e1ccc..3b70876 100644
--- a/sound/soc/kirkwood/kirkwood.h
+++ b/sound/soc/kirkwood/kirkwood.h
@@ -132,6 +132,7 @@ struct kirkwood_dma_data {
 	void __iomem *io;
 	struct clk *clk;
 	struct clk *extclk;
+	uint32_t ctl_play_mask;
 	uint32_t ctl_play;
 	uint32_t ctl_rec;
 	struct snd_pcm_substream *substream_play;
-- 
1.7.4.4

[PATCH 11/14] ASoC: kirkwood-openrd: add DAPM links between codec and cpu DAI

[Russell King]

Add the DAPM links to connect the codec DAC and ADCs to the cpu DAI
I2S inputs/outputs.  This is required to restore audio on this
platform as kirkwood-i2s must know which output streams to enable.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 sound/soc/kirkwood/kirkwood-openrd.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-openrd.c b/sound/soc/kirkwood/kirkwood-openrd.c
index a25dfcf..258fcce 100644
--- a/sound/soc/kirkwood/kirkwood-openrd.c
+++ b/sound/soc/kirkwood/kirkwood-openrd.c
@@ -50,6 +50,7 @@ static struct snd_soc_ops openrd_client_ops = {
 };
 
 
+/* This will need to be updated when DPCM support works. */
 static struct snd_soc_dai_link openrd_client_dai[] = {
 {
 	.name = "CS42L51",
@@ -63,12 +64,19 @@ static struct snd_soc_dai_link openrd_client_dai[] = {
 },
 };
 
+static const struct snd_soc_dapm_route routes[] = {
+	/* Connect the codec streams to the I2S connections */
+	{ "Playback", NULL, "i2sdo" },
+	{ "i2sdi", NULL, "Capture" },
+};
 
 static struct snd_soc_card openrd_client = {
 	.name = "OpenRD Client",
 	.owner = THIS_MODULE,
 	.dai_link = openrd_client_dai,
 	.num_links = ARRAY_SIZE(openrd_client_dai),
+	.dapm_routes = routes,
+	.num_dapm_routes = ARRAY_SIZE(routes),
 };
 
 static int openrd_probe(struct platform_device *pdev)
-- 
1.7.4.4

[PATCH 12/14] ASoC: kirkwood-t5325: add DAPM links between codec and cpu DAI

[Russell King]

Add the DAPM links to connect the codec DAC and ADCs to the cpu DAI
I2S inputs/outputs.  This is required to restore audio on this
platform as kirkwood-i2s must know which output streams to enable.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 sound/soc/kirkwood/kirkwood-t5325.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-t5325.c b/sound/soc/kirkwood/kirkwood-t5325.c
index 82a8c5f..5f5ae08 100644
--- a/sound/soc/kirkwood/kirkwood-t5325.c
+++ b/sound/soc/kirkwood/kirkwood-t5325.c
@@ -52,6 +52,9 @@ static const struct snd_soc_dapm_route t5325_route[] = {
 
 	{ "MIC1",		NULL,	"Mic Jack" },
 	{ "MIC2",		NULL,	"Mic Jack" },
+
+	{ "i2sdi",		NULL,	"Capture" },
+	{ "Playback",		NULL,	"i2sdo" },
 };
 
 static int t5325_dai_init(struct snd_soc_pcm_runtime *rtd)
@@ -66,6 +69,7 @@ static int t5325_dai_init(struct snd_soc_pcm_runtime *rtd)
 	return 0;
 }
 
+/* This will need to be updated when DPCM support works. */
 static struct snd_soc_dai_link t5325_dai[] = {
 {
 	.name = "ALC5621",
-- 
1.7.4.4

[PATCH 13/14] ASoC: kirkwood: add SPDIF output support

[Russell King]

Add support for SPDIF output.  This is enabled via a widget being
connected to an output.  When this widget is not connected, SPDIF
output will remain disabled.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 sound/soc/kirkwood/kirkwood-i2s.c |   26 +++++++++++++++++++++++---
 1 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 8a6ff1a..63df507 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -159,7 +159,8 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_FORMAT_S16_LE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_16;
 		ctl_play = KIRKWOOD_PLAYCTL_SIZE_16_C |
-			   KIRKWOOD_PLAYCTL_I2S_EN;
+			   KIRKWOOD_PLAYCTL_I2S_EN |
+			   KIRKWOOD_PLAYCTL_SPDIF_EN;
 		ctl_rec = KIRKWOOD_RECCTL_SIZE_16_C |
 			  KIRKWOOD_RECCTL_I2S_EN;
 		break;
@@ -169,7 +170,8 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_FORMAT_S20_3LE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_20;
 		ctl_play = KIRKWOOD_PLAYCTL_SIZE_20 |
-			   KIRKWOOD_PLAYCTL_I2S_EN;
+			   KIRKWOOD_PLAYCTL_I2S_EN |
+			   KIRKWOOD_PLAYCTL_SPDIF_EN;
 		ctl_rec = KIRKWOOD_RECCTL_SIZE_20 |
 			  KIRKWOOD_RECCTL_I2S_EN;
 		break;
@@ -177,7 +179,8 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_FORMAT_S24_LE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_24;
 		ctl_play = KIRKWOOD_PLAYCTL_SIZE_24 |
-			   KIRKWOOD_PLAYCTL_I2S_EN;
+			   KIRKWOOD_PLAYCTL_I2S_EN |
+			   KIRKWOOD_PLAYCTL_SPDIF_EN;
 		ctl_rec = KIRKWOOD_RECCTL_SIZE_24 |
 			  KIRKWOOD_RECCTL_I2S_EN;
 		break;
@@ -374,11 +377,28 @@ static int kirkwood_i2s_play_i2s(struct snd_soc_dapm_widget *w,
 	return 0;
 }
 
+static int kirkwood_i2s_play_spdif(struct snd_soc_dapm_widget *w,
+	struct snd_kcontrol *ctl, int event)
+{
+	/* CPU DAI is not available, so use driver data from device */
+	struct kirkwood_dma_data *priv = dev_get_drvdata(w->dapm->dev);
+
+	if (SND_SOC_DAPM_EVENT_ON(event))
+		priv->ctl_play_mask |= KIRKWOOD_PLAYCTL_SPDIF_EN;
+	else
+		priv->ctl_play_mask &= ~KIRKWOOD_PLAYCTL_SPDIF_EN;
+
+	return 0;
+}
+
 static const struct snd_soc_dapm_widget widgets[] = {
 	/* These widget names come from the names from the functional spec */
 	SND_SOC_DAPM_AIF_OUT_E("i2sdo", "dma-tx",
 		0, SND_SOC_NOPM, 0, 0, kirkwood_i2s_play_i2s,
 		SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
+	SND_SOC_DAPM_AIF_OUT_E("spdifdo", "dma-tx",
+		0, SND_SOC_NOPM, 0, 0, kirkwood_i2s_play_spdif,
+		SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
 	SND_SOC_DAPM_AIF_IN("i2sdi", "dma-rx",
 		0, SND_SOC_NOPM, 0, 0),
 };
-- 
1.7.4.4

[PATCH 13/14] ASoC: kirkwood: add SPDIF output support

[Mark Brown]

On Sat, Aug 31, 2013 at 01:47:37PM +0100, Russell King wrote:
> Add support for SPDIF output.  This is enabled via a widget being
> connected to an output.  When this widget is not connected, SPDIF
> output will remain disabled.

This is still not a good approach to adding a new digital audio
interface since it does not create a DAI but instead adds it like an
analogue link (but hooked inside the CPU using the DPCM DAPM hooks).
While this may work right now but will not be robust as the framework is
developed and can cause problems in system integration.

A new DAI should be being added for the S/PDIF interface, this should
fully utilise DPCM but an either/or approach would be OK as a stepping
stone.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130903/887b131c/attachment.sig>

[PATCH 13/14] ASoC: kirkwood: add SPDIF output support

[Russell King - ARM Linux]

On Tue, Sep 03, 2013 at 12:17:20PM +0100, Mark Brown wrote:
> On Sat, Aug 31, 2013 at 01:47:37PM +0100, Russell King wrote:
> > Add support for SPDIF output.  This is enabled via a widget being
> > connected to an output.  When this widget is not connected, SPDIF
> > output will remain disabled.
> 
> This is still not a good approach to adding a new digital audio
> interface since it does not create a DAI but instead adds it like an
> analogue link (but hooked inside the CPU using the DPCM DAPM hooks).

This is the way Liam's Haswell DPCM driver works.

> While this may work right now but will not be robust as the framework is
> developed and can cause problems in system integration.
> 
> A new DAI should be being added for the S/PDIF interface, this should
> fully utilise DPCM but an either/or approach would be OK as a stepping
> stone.

A new CPU DAI will be a front end DAI.  So what you're saying here is
that your earlier statement where you clearly said "one front end and
two back ends" was wrong.

Sorry, I'm not playing your game anymore.  I'm not interested in trying
to work with you anymore, because its totally impossible.  You're being
as obstructive as you have been from the very start, and as demonstrated
last night when you said "there is no mixer" in Liam's DPCM driver, you
don't know what you're talking about half the time.  Or, you're
intentionally making false statements.

There is no point persuing this any further; I'm not wasting any more
time on playing your stupid games.

[PATCH 13/14] ASoC: kirkwood: add SPDIF output support

[Mark Brown]

On Tue, Sep 03, 2013 at 12:38:32PM +0100, Russell King - ARM Linux wrote:
> On Tue, Sep 03, 2013 at 12:17:20PM +0100, Mark Brown wrote:

> > This is still not a good approach to adding a new digital audio
> > interface since it does not create a DAI but instead adds it like an
> > analogue link (but hooked inside the CPU using the DPCM DAPM hooks).

> This is the way Liam's Haswell DPCM driver works.

Liam's driver also adds DAIs.

> > A new DAI should be being added for the S/PDIF interface, this should
> > fully utilise DPCM but an either/or approach would be OK as a stepping
> > stone.

> A new CPU DAI will be a front end DAI.  So what you're saying here is
> that your earlier statement where you clearly said "one front end and
> two back ends" was wrong.

I have not yet replied to the last e-mail you sent last night but it
appears from that that and from the above that you have front end and
back end confused.  A front end DAI should be associated with DMA, a
back end DAI should represent an external digital audio interface for
the SoC.  This means that the I2S and S/PDIF interfaces should both have
back end DAIs representing them and the DMA should be represented by a
front end.

> Sorry, I'm not playing your game anymore.  I'm not interested in trying
> to work with you anymore, because its totally impossible.  You're being
> as obstructive as you have been from the very start, and as demonstrated
> last night when you said "there is no mixer" in Liam's DPCM driver, you
> don't know what you're talking about half the time.  Or, you're
> intentionally making false statements.

I did not say that - I said:

| This is not correct, there is no mixer in the link between the back end
| and the CODEC.

The mixer in Haswell is in the DSP block which sits between the front
and back end DAIs, external devices like the CODEC are connected to the
back ends.  I fear that your confusion between front and back end may
have mislead you here.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130903/1e873cb2/attachment.sig>

[PATCH 13/14] ASoC: kirkwood: add SPDIF output support

[Russell King - ARM Linux]

On Tue, Sep 03, 2013 at 12:59:38PM +0100, Mark Brown wrote:
> The mixer in Haswell is in the DSP block which sits between the front
> and back end DAIs, external devices like the CODEC are connected to the
> back ends.  I fear that your confusion between front and back end may
> have mislead you here.

Here's what Liam says about his driver:

"It will show a simple example of how to connect about 5 FE pcms to
2 BE DAIs".  So this tells us how the driver is structured, and here
is the confirmation of that:

Here's the declarations, with non-relevant information removed:

static struct snd_soc_dai_driver hsw_dais[] = {
        {
                .name  = "System Pin", 
                .playback = {
                        .stream_name = "System Playback",
        },
        {
                /* PCM and compressed */
                .name  = "Offload0 Pin",
                .playback = {
                        .stream_name = "Offload0 Playback",
                },
        },
        {
                /* PCM and compressed */
                .name  = "Offload1 Pin",
                .playback = {
                        .stream_name = "Offload1 Playback",
                },
        },
        {
                .name  = "Loopback Pin",  
                .capture = {
                        .stream_name = "Loopback Capture",
                },
        },
        {
                .name  = "Capture Pin",
                .capture = {
                        .stream_name = "Analog Capture",
                },
        },
};

So, this creates five front end DAIs.  This layer also creates some
widgets:

static const struct snd_soc_dapm_widget widgets[] = {
        /* Backend DAIs  */
        SND_SOC_DAPM_AIF_IN("SSP0 CODEC IN", NULL, 0, SND_SOC_NOPM, 0, 0),
        SND_SOC_DAPM_AIF_OUT("SSP0 CODEC OUT", NULL, 0, SND_SOC_NOPM, 0, 0),
        SND_SOC_DAPM_AIF_IN("SSP1 BT IN", NULL, 0, SND_SOC_NOPM, 0, 0),
        SND_SOC_DAPM_AIF_OUT("SSP1 BT OUT", NULL, 0, SND_SOC_NOPM, 0, 0),
                        
        /* Global Playback Mixer */
        SND_SOC_DAPM_MIXER("Playback VMixer", SND_SOC_NOPM, 0, 0, NULL, 0),
};

and links these to the streams above:

static const struct snd_soc_dapm_route graph[] = {
        /* Playback Mixer */
        {"Playback VMixer", NULL, "System Playback"},
        {"Playback VMixer", NULL, "Offload0 Playback"}, 
        {"Playback VMixer", NULL, "Offload1 Playback"},
                        
        {"SSP0 CODEC OUT", NULL, "Playback VMixer"},
        {"Loopback Capture", NULL, "Playback VMixer"},
                  
        {"Analog Capture", NULL, "SSP0 CODEC IN"},
};

So, what the above ends up with is this:

[s]System Playback ---+                    +-> [aif]SSP0 CODEC OUT
                      |                    |
[s]Offload0 Playback -+-> Playback VMixer -+
                      |                    |
[s]Offload1 Playback -+                    +-> [s]Loopback Capture

[aif]SSP0 CODEC IN -> [s]Analog Capture

The [s] and [aif] annotations label these up as stream widgets which
are created automatically by ASoC, and [aif] widgets from the CPU DAI
layer.

The card layer sets up these links (again, minimised to omit unnecessary
information).  First, let's look at the definition of snd_soc_dai_link,
so we know how to identify front end and back end DAIs:

struct snd_soc_dai_link {
        /* Do not create a PCM for this DAI link (Backend link) */
        unsigned int no_pcm:1;

        /* This DAI link can route to other DAI links at runtime (Frontend)*/
        unsigned int dynamic:1;
};

So, anything with .no_pcm = 1 is a backend, and anything with
.dynamic = 1 is a frontend.  This is backed up by the code:

        /* ASoC PCM operations */
        if (rtd->dai_link->dynamic) {
                rtd->ops.open           = dpcm_fe_dai_open;
                rtd->ops.hw_params      = dpcm_fe_dai_hw_params;
                rtd->ops.prepare        = dpcm_fe_dai_prepare;
                rtd->ops.trigger        = dpcm_fe_dai_trigger;
                rtd->ops.hw_free        = dpcm_fe_dai_hw_free;
                rtd->ops.close          = dpcm_fe_dai_close;
                rtd->ops.pointer        = soc_pcm_pointer;
                rtd->ops.ioctl          = soc_pcm_ioctl;
        } else {
                rtd->ops.open           = soc_pcm_open;
                rtd->ops.hw_params      = soc_pcm_hw_params;
                rtd->ops.prepare        = soc_pcm_prepare;
                rtd->ops.trigger        = soc_pcm_trigger;
                rtd->ops.hw_free        = soc_pcm_hw_free;
                rtd->ops.close          = soc_pcm_close;
                rtd->ops.pointer        = soc_pcm_pointer;
                rtd->ops.ioctl          = soc_pcm_ioctl;
        }

.dynamic can't be a backend, because why would we assign it front-end
DAI operations if it were set true.  So, it's quite clear that
.dynamic marks a frontend DAI.

static struct snd_soc_dai_link haswell_dais[] = {
        /* Front End DAI links */
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ note that comment
        {
                .name = "System",  
                .stream_name = "System Playback",  
                .cpu_dai_name = "System Pin",  
                .platform_name = "hsw-pcm-audio",
                .dynamic = 1,
                .codec_name = "snd-soc-dummy",
                .codec_dai_name = "snd-soc-dummy-dai",
                .dpcm_playback = 1,
        },
Clearly, "System Playback" is a front end stream - not only does this has
.dynamic = 1, but also it binds it to the dummy codec.
        {
                .name = "Offload0",
                .stream_name = "Offload0 Playback",
                .cpu_dai_name = "Offload0 Pin",
                .platform_name = "hsw-pcm-audio",
                .dynamic = 1,
                .codec_name = "snd-soc-dummy",
                .codec_dai_name = "snd-soc-dummy-dai",
                .dpcm_playback = 1,
        },
Again, another front end stream.
        {
                .name = "Offload1",
                .stream_name = "Offload1 Playback",
                .cpu_dai_name = "Offload1 Pin",  
                .platform_name = "hsw-pcm-audio",
                .dynamic = 1,
                .codec_name = "snd-soc-dummy",
                .codec_dai_name = "snd-soc-dummy-dai",
                .dpcm_playback = 1,
        },
And again.
        { 
                .name = "Loopback",
                .stream_name = "Loopback",
                .cpu_dai_name = "Loopback Pin",
                .platform_name = "hsw-pcm-audio",
                .dynamic = 1,
                .codec_name = "snd-soc-dummy",
                .codec_dai_name = "snd-soc-dummy-dai",
                .dpcm_capture = 1,
        },
And again.
        { 
                .name = "Capture",
                .stream_name = "Capture",
                .cpu_dai_name = "Capture Pin",
                .platform_name = "hsw-pcm-audio",
                .dynamic = 1,
                .codec_name = "snd-soc-dummy",
                .codec_dai_name = "snd-soc-dummy-dai",
                .trigger = {SND_SOC_DPCM_TRIGGER_POST, SND_SOC_DPCM_TRIGGER_POS$
                .dpcm_capture = 1,
        },
And again.

So... let's mark what's a front-end DAI stream:

[FE:s]System Playback ---+                    +-> [aif]SSP0 CODEC OUT
                         |                    |
[FE:s]Offload0 Playback -+-> Playback VMixer -+
                         |                    |
[FE:s]Offload1 Playback -+                    +-> [FE:s]Loopback Capture

[aif]SSP0 CODEC IN -> [FE:s]Analog Capture

Now for the backends.  The card layer creates these two DAI links:
        /* Back End DAI links */
        { 
                /* SSP0 - Codec */ 
                .name = "Codec",
                .cpu_dai_name = "snd-soc-dummy-dai",
                .platform_name = "snd-soc-dummy",
                .no_pcm = 1,
                .codec_name = "rt5640.0-001c",
                .codec_dai_name = "rt5640-aif1",
                .dpcm_playback = 1,
                .dpcm_capture = 1,
        },
        {
                /* SSP1 - BT */
                .name = "SSP1-Codec",
                .cpu_dai_name = "snd-soc-dummy-dai",
                .platform_name = "snd-soc-dummy",
                .no_pcm = 1,
                .codec_name = "snd-soc-dummy",
                .codec_dai_name = "snd-soc-dummy-dai",
	}

We shall ignore the second, because that's just a dummy.  The first isn't.

As it nas .no_pcm = 1, and the dummy dai for the CPU Dai, this is clearly
a backend.  So, "rt5640-aif1" is the backend DAI which is the _codec_
DAI.

The RT5640 Codec driver creates this:

struct snd_soc_dai_driver rt5640_dai[] = {  
        {
                .name = "rt5640-aif1",
                .id = RT5640_AIF1,
                .playback = {
                        .stream_name = "AIF1 Playback",
                },
                .capture = {
                        .stream_name = "AIF1 Capture",
                },
...

which creates two streams called "AIF1 Playback" and "AIF1 Capture".
These are the codec streams.  Let's call them "[BE:s]AIF1 Playback" and
"[BE:s]AIF1 Capture" so we can clearly understand that these are backend
DAI streams.

What does the card layer do to connect these two together?

static const struct snd_soc_dapm_route hsw_map[] = {
        {"Headphones", NULL, "HPOR"},
        {"Headphones", NULL, "HPOL"},
        {"IN2P", NULL, "Mic"},
                
        /* CODEC BE connections */
        {"SSP0 CODEC IN", NULL, "AIF1 Capture"},
        {"AIF1 Playback", NULL, "SSP0 CODEC OUT"},
};

Note the last two lines.  So, let's add them to this diagram:

[FE:s]System Playback ---+                    +-> [aif]SSP0 CODEC OUT -> [BE:s]AIF1 Playback
                         |                    |
[FE:s]Offload0 Playback -+-> Playback VMixer -+
                         |                    |
[FE:s]Offload1 Playback -+                    +-> [FE:s]Loopback Capture

[BE:s]AIF1 Capture -> [aif]SSP0 CODEC IN -> [FE:s]Analog Capture

And here we have the complete structure.  The DAPM routes and DAPM widgets
sit between the front end DAI streams, and the back end DAI streams - and
the back end DAI streams belong to the codec.

What am I doing in my driver?

Firstly, here's the front end driver:

static struct snd_soc_dai_driver kirkwood_i2s_dai = {
        .playback = {
                .stream_name = "dma-tx",
        },
        .capture = {
                .stream_name = "dma-rx",
        },
};

So here we have two streams, one called "dma-tx" and the other called
"dma-rx".  These are equivalent to "System Playback" and "Analog
Capture" from Liam's driver.

static const struct snd_soc_dapm_widget widgets[] = {
        /* These widget names come from the names from the functional spec */
        SND_SOC_DAPM_AIF_OUT_E("i2sdo", "dma-tx",
                0, SND_SOC_NOPM, 0, 0, kirkwood_i2s_play_i2s,
                SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
        SND_SOC_DAPM_AIF_OUT_E("spdifdo", "dma-tx",
                0, SND_SOC_NOPM, 0, 0, kirkwood_i2s_play_spdif,
                SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
        SND_SOC_DAPM_AIF_IN("i2sdi", "dma-rx",
                0, SND_SOC_NOPM, 0, 0),
};

Onto these two streams, I attach some widgets:

             +-> [aif]i2sdo
[FE:s]dma-tx-+
             +-> [aif]spdifdo

[aif]i2sdi-->[FE:s]dma-rx

The DAI links are setup:

static struct snd_soc_dai_link kirkwood_spdif_dai1[] = {
        {
                .name = "S/PDIF1",
                .stream_name = "Audio Playback",
                .platform_name = "mvebu-audio.1",
                .cpu_dai_name = "mvebu-audio.1",
                .codec_name = "snd-soc-dummy",
                .codec_dai_name = "snd-soc-dummy-dai",
                .dynamic = 1,
        }, {
                .name = "Codec",
                .stream_name = "IEC958 Playback",
                .cpu_dai_name = "snd-soc-dummy-dai",
                .platform_name = "snd-soc-dummy",
                .no_pcm = 1,
                .codec_dai_name = "dit-hifi",
                .codec_name = "spdif-dit",
        },
};

The first is the frontend - note the .dynamic = 1 in there and the
dummy codec.  The second is the backend, which is the spdif transmitter.
The SPDIF transmitter gives us this:

static struct snd_soc_dai_driver dit_stub_dai = {
        .name           = "dit-hifi",
        .playback       = {
                .stream_name    = "spdif-Playback",

So, it has a stream called "spdif-Playback" (you know full well why I had
to add the "spdif-" prefix here, so I won't explain that.)  This gives us
a backend _codec_ stream of "spdif-Playback" which I'll mark up as
"[BE]spdif-Playback".  This is no different from Liam's RT5640 Codec
driver, and is equivalent to the "AIF1 Playback" there.

How do I connect these together?

static const struct snd_soc_dapm_route routes[] = {
        { "spdif-Playback", NULL, "spdifdo" },
};

So, we end up with this (I'm ignoring the capture side here):

             +-> [aif]i2sdo -> (not connected at present)
[FE:s]dma-tx-+
             +-> [aif]spdifdo -> [BE:s]spdif-Playback

Now, let's go and compare that back to the structure of Liam's original
driver:

[FE:s]System Playback ---+                    +-> [aif]SSP0 CODEC OUT -> [BE:s]AIF1 Playback
                         |                    |
[FE:s]Offload0 Playback -+-> Playback VMixer -+
                         |                    |
[FE:s]Offload1 Playback -+                    +-> [FE:s]Loopback Capture

[BE:s]AIF1 Capture -> [aif]SSP0 CODEC IN -> [FE:s]Analog Capture

and we can see that both of these are doing the same thing.  Front end
CPU DAI streams are connected through widgets to backend _codec_ streams.

So, what I've been trying to find out from you is: you're saying that my
driver is somehow incorrect, and needs to create more front-end DAIs.
I'm saying that it is conformant with Liam's example.

The hardware as a whole - the entire platform - is structured like this:

                                       I2S enable
                                            v
                                   +-> I2S formatter -----.
                                   |                      +-> HDMI chip
<system memory> -> dma -> tx fifo -+                    +-'
                    ^              +-> SPDIF formatter -+
                    |                       ^           +-> TOSlink out
               dma enable              SPDIF enable

where "dma enable" is provided internally in the hardware as the logical
OR of I2S enable and SPDIF enable.  There is no software control of that.

Comparing that with the structure I'm creating, the "dma-tx" stream
represents the DMA and TX fifo.  The "i2sdo" AIF widget represents the
I2S formatted output.  The "spdifdo" AIF widget represents the SPDIF
formatted output.  The "spdif-Playback" backend stream represents the
TOSlink output, and the HDMI chip is currently not represented as it
appears ASoC doesn't need to know about it with properly formatted SPDIF.

Now, if you still disagree that my approach is compliant with Liam's,
then please describe _and_ more importantly draw diagrams as I have done
above - and as I've done in the past for you - to illustrate what you
believe to be a _correct_ solution to this.

[PATCH 13/14] ASoC: kirkwood: add SPDIF output support

[Mark Brown]

On Tue, Sep 03, 2013 at 02:34:03PM +0100, Russell King - ARM Linux wrote:
> On Tue, Sep 03, 2013 at 12:59:38PM +0100, Mark Brown wrote:

> > The mixer in Haswell is in the DSP block which sits between the front
> > and back end DAIs, external devices like the CODEC are connected to the
> > back ends.  I fear that your confusion between front and back end may
> > have mislead you here.

> Here's what Liam says about his driver:

> "It will show a simple example of how to connect about 5 FE pcms to
> 2 BE DAIs".  So this tells us how the driver is structured, and here
> is the confirmation of that:

A couple of high level things here.  

One is that, as you have been told on many occasions, you need to be
aware that the driver Liam has been working on which you are using as a
reference here is out of tree and that it is therefore both possible and
likely that issues will be identified during the review on the way into
mainline.  The need to manually add DAPM routes for DAI links is one
example of this.  This means that you cannot take the out of tree code
as gospel, as it has not yet been reviewed issues may be identified
which are also present in the out of tree code.

The other is that it would have been really helpful if earlier on rather
than simply stating that you believe there are no problems in the code
you had said that this was based on you being unable to see any
differences between what these patches are doing and what the out of
tree driver is doing.

> As it nas .no_pcm = 1, and the dummy dai for the CPU Dai, this is clearly
> a backend.  So, "rt5640-aif1" is the backend DAI which is the _codec_
> DAI.

Note that both SoC and CPU DAIs can be specified since the back end link
is being set up using a dai_link.  In the example you are referring to
the SoC side is stubbed out using a dummy since AIUI all the back end
setup is done by the DSP rather than by the CPU and the issue with
manual route creation for DAI links had not yet been identified.

> The DAI links are setup:
> 
> static struct snd_soc_dai_link kirkwood_spdif_dai1[] = {
>         {
>                 .name = "S/PDIF1",
>                 .stream_name = "Audio Playback",
>                 .platform_name = "mvebu-audio.1",
>                 .cpu_dai_name = "mvebu-audio.1",
>                 .codec_name = "snd-soc-dummy",
>                 .codec_dai_name = "snd-soc-dummy-dai",
>                 .dynamic = 1,
>         }, {
>                 .name = "Codec",
>                 .stream_name = "IEC958 Playback",
>                 .cpu_dai_name = "snd-soc-dummy-dai",
>                 .platform_name = "snd-soc-dummy",
>                 .no_pcm = 1,
>                 .codec_dai_name = "dit-hifi",
>                 .codec_name = "spdif-dit",
>         },
> };

> The first is the frontend - note the .dynamic = 1 in there and the
> dummy codec.  The second is the backend, which is the spdif transmitter.

The back end should not be using a dummy DAI for the CPU DAI, it should
be using a back end DAI provided by the SoC driver as previously and
repeatedly requested.  As previously discussed this will avoid the
boards having to specify the connections between the SoC and the
external devices twice.  It will also ensure that the board is shielded
from the implementation details of the SoC driver with a consistent
interface being presented to them.

> So, what I've been trying to find out from you is: you're saying that my
> driver is somehow incorrect, and needs to create more front-end DAIs.

No, I have not been saying that more more front end DAIs are needed.  To
repeat what I said in the e-mail to which you are replying:

| the SoC.  This means that the I2S and S/PDIF interfaces should both have
| back end DAIs representing them and the DMA should be represented by a
| front end.

That's one front end DAI for the DMA, one back end DAI for the I2S and
one back end DAI for the S/PDIF making a total of one front end DAI and
two back end DAIs.  As the code already has a front end DAI this means
that back end DAIs need to be added.

> I'm saying that it is conformant with Liam's example.

> Now, if you still disagree that my approach is compliant with Liam's,
> then please describe _and_ more importantly draw diagrams as I have done
> above - and as I've done in the past for you - to illustrate what you
> believe to be a _correct_ solution to this.

Again, you are using out of tree code as a reference here and so you
need to be aware that issues which are identified in review may also be
present in that out of tree code.  If the review comments and out of
tree code disagree it is very important that you focus on the review
comments and if you are saying something based on out of tree code it is
very important that you highlight that.

You need to create the back ends and connect them in line with the AIF
DAPM widgets, the diagrams are essentially the same as those you drew.
Until the framework does this automatically you will need to manually
add DAPM routes between the SoC and the CODEC, though they should be
clearly marked so that they can readily be removed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130904/11c34659/attachment.sig>

[PATCH 14/14] ASoC: kirkwood: add IEC958 channel status support

[Russell King]

Add support for userspace setting the IEC958 channel status data
for the SPDIF interface.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 sound/soc/kirkwood/kirkwood-i2s.c |   93 +++++++++++++++++++++++++++++++++++++
 sound/soc/kirkwood/kirkwood.h     |   33 +++++++++++++
 2 files changed, 126 insertions(+), 0 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 63df507..11f51e0 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -18,6 +18,7 @@
 #include <linux/mbus.h>
 #include <linux/delay.h>
 #include <linux/clk.h>
+#include <sound/asoundef.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
@@ -29,11 +30,94 @@
 #define KIRKWOOD_I2S_RATES \
 	(SNDRV_PCM_RATE_44100 | \
 	 SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000)
+
 #define KIRKWOOD_I2S_FORMATS \
 	(SNDRV_PCM_FMTBIT_S16_LE | \
 	 SNDRV_PCM_FMTBIT_S24_LE | \
 	 SNDRV_PCM_FMTBIT_S32_LE)
 
+static int kirkwood_iec958_info(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_info *uinfo)
+{
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_IEC958;
+	uinfo->count = 1;
+
+	return 0;
+}
+
+static int kirkwood_iec958_mask_get(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	memset(ucontrol->value.iec958.status, 0,
+	       sizeof(ucontrol->value.iec958.status));
+	memset(ucontrol->value.iec958.status, 0xff, 4);
+	return 0;
+}
+
+static int kirkwood_iec958_dflt_get(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_dai *cpu_dai = snd_kcontrol_chip(kcontrol);
+	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(cpu_dai);
+	u32 reg;
+
+	reg = readl_relaxed(priv->io + KIRKWOOD_SPDIF_STATUS0_L);
+	ucontrol->value.iec958.status[3] = reg >> 24;
+	ucontrol->value.iec958.status[2] = reg >> 16;
+	ucontrol->value.iec958.status[1] = reg >>  8;
+	ucontrol->value.iec958.status[0] = reg;
+
+	return 0;
+}
+
+static int kirkwood_iec958_dflt_put(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_dai *cpu_dai = snd_kcontrol_chip(kcontrol);
+	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(cpu_dai);
+	u32 val, nval;
+
+	val  = readl_relaxed(priv->io + KIRKWOOD_SPDIF_PLAYCTL);
+	nval = val & ~(KIRKWOOD_SPDIF_NON_PCM | KIRKWOOD_SPDIF_REG_VALIDITY);
+	if (ucontrol->value.iec958.status[0] & IEC958_AES0_NONAUDIO)
+		nval |= KIRKWOOD_SPDIF_NON_PCM | KIRKWOOD_SPDIF_REG_VALIDITY;
+	writel_relaxed(nval, priv->io + KIRKWOOD_SPDIF_PLAYCTL);
+
+	val = readl_relaxed(priv->io + KIRKWOOD_SPDIF_STATUS0_L);
+	nval = ucontrol->value.iec958.status[3] << 24 |
+	       ucontrol->value.iec958.status[2] << 16 |
+	       ucontrol->value.iec958.status[1] << 8 |
+	       ucontrol->value.iec958.status[0];
+	writel_relaxed(nval, priv->io + KIRKWOOD_SPDIF_STATUS0_L);
+	writel_relaxed(nval, priv->io + KIRKWOOD_SPDIF_STATUS0_R);
+
+	return nval != val;
+}
+
+static const struct snd_kcontrol_new kirkwood_i2s_iec958_controls[] = {
+	{
+		.iface	= SNDRV_CTL_ELEM_IFACE_MIXER,
+		.name	= SNDRV_CTL_NAME_IEC958("", PLAYBACK, CON_MASK),
+		.access	= SNDRV_CTL_ELEM_ACCESS_READ,
+		.info	= kirkwood_iec958_info,
+		.get	= kirkwood_iec958_mask_get,
+	}, {
+		.iface	= SNDRV_CTL_ELEM_IFACE_MIXER,
+		.name	= SNDRV_CTL_NAME_IEC958("", PLAYBACK, PRO_MASK),
+		.access	= SNDRV_CTL_ELEM_ACCESS_READ,
+		.info	= kirkwood_iec958_info,
+		.get	= kirkwood_iec958_mask_get,
+	}, {
+		.iface	= SNDRV_CTL_ELEM_IFACE_PCM,
+		.name	= SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT),
+		.access	= SNDRV_CTL_ELEM_ACCESS_READWRITE |
+			  SNDRV_CTL_ELEM_ACCESS_VOLATILE,
+		.info	= kirkwood_iec958_info,
+		.get	= kirkwood_iec958_dflt_get,
+		.put	= kirkwood_iec958_dflt_put,
+	},
+};
+
 static int kirkwood_i2s_set_fmt(struct snd_soc_dai *cpu_dai,
 		unsigned int fmt)
 {
@@ -408,6 +492,15 @@ static int kirkwood_i2s_probe(struct snd_soc_dai *dai)
 	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai);
 	unsigned long value;
 	unsigned int reg_data;
+	int ret;
+
+	ret = snd_soc_add_dai_controls(dai, kirkwood_i2s_iec958_controls,
+				ARRAY_SIZE(kirkwood_i2s_iec958_controls));
+	if (ret) {
+		dev_err(dai->dev,
+			"unable to add soc card controls: %d\n", ret);
+		return ret;
+	}
 
 	/* It appears that these can't be attached to the CPU DAI */
 	snd_soc_dapm_new_controls(&dai->dapm, widgets, ARRAY_SIZE(widgets));
diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h
index 3b70876..17b48a6 100644
--- a/sound/soc/kirkwood/kirkwood.h
+++ b/sound/soc/kirkwood/kirkwood.h
@@ -105,6 +105,39 @@
 #define KIRKWOOD_PLAY_BYTE_INT_COUNT		0x1314
 #define KIRKWOOD_BYTE_INT_COUNT_MASK		0xffffff
 
+#define KIRKWOOD_SPDIF_PLAYCTL			0x2204
+#define KIRKWOOD_SPDIF_NON_PCM			(1<<17)
+#define KIRKWOOD_SPDIF_REG_VALIDITY		(1<<16)
+#define KIRKWOOD_SPDIF_FORCE_PARERR		(1<<4)
+#define KIRKWOOD_SPDIF_MEM_USER_EN		(1<<2)
+#define KIRKWOOD_SPDIF_MEM_VALIDITY_EN		(1<<1)
+#define KIRKWOOD_SPDIF_BLOCK_START_MODE		(1<<0)
+
+#define KIRKWOOD_SPDIF_STATUS0_L		0x2280
+#define KIRKWOOD_SPDIF_STATUS1_L		0x2284
+#define KIRKWOOD_SPDIF_STATUS2_L		0x2288
+#define KIRKWOOD_SPDIF_STATUS3_L		0x228c
+#define KIRKWOOD_SPDIF_STATUS4_L		0x2290
+#define KIRKWOOD_SPDIF_STATUS5_L		0x2294
+#define KIRKWOOD_SPDIF_STATUS0_R		0x22a0
+#define KIRKWOOD_SPDIF_STATUS1_R		0x22a4
+#define KIRKWOOD_SPDIF_STATUS2_R		0x22a8
+#define KIRKWOOD_SPDIF_STATUS3_R		0x22ac
+#define KIRKWOOD_SPDIF_STATUS4_R		0x22b0
+#define KIRKWOOD_SPDIF_STATUS5_R		0x22b4
+#define KIRKWOOD_SPDIF_USER0_L			0x22c0
+#define KIRKWOOD_SPDIF_USER1_L			0x22c4
+#define KIRKWOOD_SPDIF_USER2_L			0x22c8
+#define KIRKWOOD_SPDIF_USER3_L			0x22cc
+#define KIRKWOOD_SPDIF_USER4_L			0x22d0
+#define KIRKWOOD_SPDIF_USER5_L			0x22d4
+#define KIRKWOOD_SPDIF_USER0_R			0x22e0
+#define KIRKWOOD_SPDIF_USER1_R			0x22e4
+#define KIRKWOOD_SPDIF_USER2_R			0x22e8
+#define KIRKWOOD_SPDIF_USER3_R			0x22ec
+#define KIRKWOOD_SPDIF_USER4_R			0x22f0
+#define KIRKWOOD_SPDIF_USER5_R			0x22f4
+
 #define KIRKWOOD_I2S_PLAYCTL			0x2508
 #define KIRKWOOD_I2S_RECCTL			0x2408
 #define KIRKWOOD_I2S_CTL_JUST_MASK		(0xf<<26)
-- 
1.7.4.4

[alsa-devel] [PATCH 00/14] SPDIF support

[Lars-Peter Clausen]

On 08/31/2013 02:34 PM, Russell King - ARM Linux wrote:
[...]
> The same conditions apply as per my previous posting - the DAI link
> needs to be setup and the associated DAPM routes to tell the CPU DAI
> which outputs are in use, like this:
> 
> DAI link:
>                 .name = "S/PDIF1",
>                 .stream_name = "IEC958 Playback",
>                 .platform_name = "mvebu-audio.1",
>                 .cpu_dai_name = "mvebu-audio.1",
>                 .codec_dai_name = "dit-hifi",
>                 .codec_name = "spdif-dit",
> 
> static const struct snd_soc_dapm_route routes[] = {
>         { "Playback", NULL, "spdifdo" },
> };

This is still not exactly the right way to implement this though. Add a
second DAI to your CPU driver, like this:

static struct snd_soc_dai_driver kirkwood_mvebu_dais[] = {
	{
		.id = 0,
		.probe = kirkwood_i2s_probe,
    		.playback = {
			.stream_name = "I2S Capture",
			...
		},
		.capture = {
			.stream_name = "I2S Playback",
			...
		},
		.ops = &kirkwood_i2s_dai_ops,
	}, {
		.id = 1,
		.probe = kirkwood_i2s_probe,
    		.playback = {
			.stream_name = "SPDIF Capture",
			...
		},
		.capture = {
			.stream_name = "SPDIF Playback",
			...
		},
		.ops = &kirkwood_i2s_dai_ops,
	},
};

Then connect the SPDIF AIF widgets to the SPDIF streams and the I2S AIF
widgets to the I2S streams. In your machine driver setup the link config to
use DAI 0 if the board uses I2S and DAI 1 if the board uses SPDIF. The ASoC
framework will then create the necessary routes all by itself. Of course
this won't work on a platform where both the SPDIF and I2S controller are
used at the same time, but neither does your current solution.

- Lars

[alsa-devel] [PATCH 00/14] SPDIF support

[Mark Brown]

On Sat, Aug 31, 2013 at 05:28:26PM +0200, Lars-Peter Clausen wrote:

> Then connect the SPDIF AIF widgets to the SPDIF streams and the I2S AIF
> widgets to the I2S streams. In your machine driver setup the link config to
> use DAI 0 if the board uses I2S and DAI 1 if the board uses SPDIF. The ASoC
> framework will then create the necessary routes all by itself. Of course
> this won't work on a platform where both the SPDIF and I2S controller are
> used at the same time, but neither does your current solution.

This is the either/or approach I've been suggesting.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130831/f644636a/attachment-0001.sig>

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Sat, Aug 31, 2013 at 06:28:53PM +0100, Mark Brown wrote:
> On Sat, Aug 31, 2013 at 05:28:26PM +0200, Lars-Peter Clausen wrote:
> 
> > Then connect the SPDIF AIF widgets to the SPDIF streams and the I2S AIF
> > widgets to the I2S streams. In your machine driver setup the link config to
> > use DAI 0 if the board uses I2S and DAI 1 if the board uses SPDIF. The ASoC
> > framework will then create the necessary routes all by itself. Of course
> > this won't work on a platform where both the SPDIF and I2S controller are
> > used at the same time, but neither does your current solution.
> 
> This is the either/or approach I've been suggesting.

Ah, here we go again.  This is precisely why I can't work with you.
Nothing I say seems to stick in your mind.  I've been telling you for the
last four weeks that it doesn't work - and I've shown you the oopses and
warnings I get from trying it.  Your response to date: nothing of any
real use.

If you're not going to provide any useful responses on bug reports, it is
totally unreasonable that you even suggest that _that_ is how it should
be done when I've already proved that it's impossible at present.

[alsa-devel] [PATCH 00/14] SPDIF support

[Lars-Peter Clausen]

On 08/31/2013 09:19 PM, Russell King - ARM Linux wrote:
> On Sat, Aug 31, 2013 at 06:28:53PM +0100, Mark Brown wrote:
>> On Sat, Aug 31, 2013 at 05:28:26PM +0200, Lars-Peter Clausen wrote:
>>
>>> Then connect the SPDIF AIF widgets to the SPDIF streams and the I2S AIF
>>> widgets to the I2S streams. In your machine driver setup the link config to
>>> use DAI 0 if the board uses I2S and DAI 1 if the board uses SPDIF. The ASoC
>>> framework will then create the necessary routes all by itself. Of course
>>> this won't work on a platform where both the SPDIF and I2S controller are
>>> used at the same time, but neither does your current solution.
>>
>> This is the either/or approach I've been suggesting.
> 
> Ah, here we go again.  This is precisely why I can't work with you.
> Nothing I say seems to stick in your mind.  I've been telling you for the
> last four weeks that it doesn't work - and I've shown you the oopses and
> warnings I get from trying it.  Your response to date: nothing of any
> real use.
> 
> If you're not going to provide any useful responses on bug reports, it is
> totally unreasonable that you even suggest that _that_ is how it should
> be done when I've already proved that it's impossible at present.

Russell, please stop being such a bitch. You seem to be the only one who has
any problems working with Mark, which maybe should make you wonder if the
problem is not on your side. Insulting and screaming at people all the time
won't exactly motivate them to help you with your problems. Please try to be
constructive, it will makes things much easier for everyone.

Thanks,
- Lars

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Sat, Aug 31, 2013 at 10:46:35PM +0200, Lars-Peter Clausen wrote:
> On 08/31/2013 09:19 PM, Russell King - ARM Linux wrote:
> > On Sat, Aug 31, 2013 at 06:28:53PM +0100, Mark Brown wrote:
> >> On Sat, Aug 31, 2013 at 05:28:26PM +0200, Lars-Peter Clausen wrote:
> >>
> >>> Then connect the SPDIF AIF widgets to the SPDIF streams and the I2S AIF
> >>> widgets to the I2S streams. In your machine driver setup the link config to
> >>> use DAI 0 if the board uses I2S and DAI 1 if the board uses SPDIF. The ASoC
> >>> framework will then create the necessary routes all by itself. Of course
> >>> this won't work on a platform where both the SPDIF and I2S controller are
> >>> used at the same time, but neither does your current solution.
> >>
> >> This is the either/or approach I've been suggesting.
> > 
> > Ah, here we go again.  This is precisely why I can't work with you.
> > Nothing I say seems to stick in your mind.  I've been telling you for the
> > last four weeks that it doesn't work - and I've shown you the oopses and
> > warnings I get from trying it.  Your response to date: nothing of any
> > real use.
> > 
> > If you're not going to provide any useful responses on bug reports, it is
> > totally unreasonable that you even suggest that _that_ is how it should
> > be done when I've already proved that it's impossible at present.
> 
> Russell, please stop being such a bitch. You seem to be the only one who has
> any problems working with Mark, which maybe should make you wonder if the
> problem is not on your side. Insulting and screaming at people all the time
> won't exactly motivate them to help you with your problems. Please try to be
> constructive, it will makes things much easier for everyone.

Consider that Mark's been saying the same old stuff for the last four
weeks, and I've been showing that it doesn't work, and the only thing
that Mark says is the same old same old - maybe in the hope that if he
says it enough times the bugs will mysteriously vanish.  Unfortunately,
that isn't happening.

It's also not constructive or helpful, and it's extremely frustrating.
I have very little patience left with ASoC at this point, having spent
a month trying to get this stuff working.

I'm not the only one: I've heard other people complain about the _exact_
same issue with ASoC: ASoC is difficult to work with, and many people
just seem to give up with it - or keep their stuff in their own trees
locally.  I can very well see why that happens.

As for "This is the either/or approach I've been suggesting."  No, that's
another false statement from Mark.  What Mark has been suggesting all
along is to use DPCM - and that's something which I tried for ages to
get working, and it just doesn't work (as evidenced by the oopses and
warnings that get spat out of the kernel.)

Mark suggested it _before_ he suggested DPCM, and I tried that approach.
The problem is that it doesn't fit the hardware.  They aren't two
separate streams - they're a single stream with two output paths, and
there's restrictions in the hardware to do with how they're controlled.

There are two choices in how the hardware is used: either one output
only is used, or if both outputs are used, they must be used "as one" -
both outputs must be simultaneously enabled and disabled.  As far as
I know, that's not possible with two DAIs.

And here's the patch I tried.

See - I've been down this route before.  I've tried it, and I know
what it's problems are.  I'm not making up _anything_ here - I really
have tried all these "suggestions" and I'm just going round in circles
because Mark isn't listening to what I've been reporting back.

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index cc733d1..8ef4241 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -35,9 +35,7 @@
 #define KIRKWOOD_I2S_FORMATS \
 	(SNDRV_PCM_FMTBIT_S16_LE | \
 	 SNDRV_PCM_FMTBIT_S24_LE | \
-	 SNDRV_PCM_FMTBIT_S32_LE | \
-	 SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE | \
-	 SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE)
+	 SNDRV_PCM_FMTBIT_S32_LE)
 
 static void kirkwood_i2s_dump_spdif(struct device *dev,
 	struct kirkwood_dma_data *priv)
@@ -258,10 +256,22 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
 				 struct snd_soc_dai *dai)
 {
 	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai);
-	uint32_t ctl_play, ctl_rec;
+	uint32_t ctl_play, ctl_rec, ctl_play_mask, ctl_rec_mask;
 	unsigned int i2s_reg;
 	unsigned long i2s_value;
 
+	/*
+	 * Select the playback/record enable masks according to which
+	 * DAI is being used.
+	 */
+	if (dai->id == 0) {
+		ctl_play_mask = KIRKWOOD_PLAYCTL_I2S_EN;
+		ctl_rec_mask = KIRKWOOD_RECCTL_I2S_EN;
+	} else {
+		ctl_play_mask = KIRKWOOD_PLAYCTL_SPDIF_EN;
+		ctl_rec_mask = KIRKWOOD_RECCTL_SPDIF_EN;
+	}
+
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		i2s_reg = KIRKWOOD_I2S_PLAYCTL;
 	} else {
@@ -279,48 +289,30 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
 	 */
 	switch (params_format(params)) {
 	case SNDRV_PCM_FORMAT_S16_LE:
+	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE:
+	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_16;
-		ctl_play = KIRKWOOD_PLAYCTL_SIZE_16_C |
-			   KIRKWOOD_PLAYCTL_I2S_EN |
-			   KIRKWOOD_PLAYCTL_SPDIF_EN;
-		ctl_rec = KIRKWOOD_RECCTL_SIZE_16_C |
-			  KIRKWOOD_RECCTL_I2S_EN;
+		ctl_play = KIRKWOOD_PLAYCTL_SIZE_16_C | ctl_play_mask;
+		ctl_rec = KIRKWOOD_RECCTL_SIZE_16_C | ctl_rec_mask;
 		break;
 	/*
 	 * doesn't work... S20_3LE != kirkwood 20bit format ?
 	 *
 	case SNDRV_PCM_FORMAT_S20_3LE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_20;
-		ctl_play = KIRKWOOD_PLAYCTL_SIZE_20 |
-			   KIRKWOOD_PLAYCTL_I2S_EN |
-			   KIRKWOOD_PLAYCTL_SPDIF_EN;
-		ctl_rec = KIRKWOOD_RECCTL_SIZE_20 |
-			  KIRKWOOD_RECCTL_I2S_EN;
+		ctl_play = KIRKWOOD_PLAYCTL_SIZE_20 | ctl_play_mask;
+		ctl_rec = KIRKWOOD_RECCTL_SIZE_20 | ctl_rec_mask;
 		break;
 	*/
 	case SNDRV_PCM_FORMAT_S24_LE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_24;
-		ctl_play = KIRKWOOD_PLAYCTL_SIZE_24 |
-			   KIRKWOOD_PLAYCTL_I2S_EN |
-			   KIRKWOOD_PLAYCTL_SPDIF_EN;
-		ctl_rec = KIRKWOOD_RECCTL_SIZE_24 |
-			  KIRKWOOD_RECCTL_I2S_EN;
+		ctl_play = KIRKWOOD_PLAYCTL_SIZE_24 | ctl_play_mask;
+		ctl_rec = KIRKWOOD_RECCTL_SIZE_24 | ctl_rec_mask;
 		break;
 	case SNDRV_PCM_FORMAT_S32_LE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_32;
-		ctl_play = KIRKWOOD_PLAYCTL_SIZE_32 |
-			   KIRKWOOD_PLAYCTL_I2S_EN;
-		ctl_rec = KIRKWOOD_RECCTL_SIZE_32 |
-			  KIRKWOOD_RECCTL_I2S_EN;
-		break;
-	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE:
-	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE:
-		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
-			return -EINVAL;
-		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_16;
-		ctl_play = KIRKWOOD_PLAYCTL_SIZE_16_C |
-			   KIRKWOOD_PLAYCTL_SPDIF_EN;
-		ctl_rec = 0;
+		ctl_play = KIRKWOOD_PLAYCTL_SIZE_32 | ctl_play_mask;
+		ctl_rec = KIRKWOOD_RECCTL_SIZE_32 | ctl_rec_mask;
 		break;
 	default:
 		return -EINVAL;
@@ -333,12 +325,12 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
 			ctl_play |= KIRKWOOD_PLAYCTL_MONO_OFF;
 
 		priv->ctl_play &= ~(KIRKWOOD_PLAYCTL_MONO_MASK |
-				    KIRKWOOD_PLAYCTL_I2S_EN |
-				    KIRKWOOD_PLAYCTL_SPDIF_EN |
+				    ctl_play_mask |
 				    KIRKWOOD_PLAYCTL_SIZE_MASK);
 		priv->ctl_play |= ctl_play;
 	} else {
-		priv->ctl_rec &= ~KIRKWOOD_RECCTL_SIZE_MASK;
+		priv->ctl_rec &= ~(KIRKWOOD_RECCTL_SIZE_MASK |
+				   ctl_rec_mask);
 		priv->ctl_rec |= ctl_rec;
 	}
 
@@ -351,7 +343,13 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
 				int cmd, struct snd_soc_dai *dai)
 {
 	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai);
-	uint32_t ctl, value;
+	uint32_t ctl, value, mute_mask;
+
+	if (dai->id == 0) {
+		mute_mask = KIRKWOOD_PLAYCTL_I2S_MUTE;
+	} else {
+		mute_mask = KIRKWOOD_PLAYCTL_SPDIF_MUTE;
+	}
 
 	ctl = readl(priv->io + KIRKWOOD_PLAYCTL);
 	if (ctl & KIRKWOOD_PLAYCTL_PAUSE) {
@@ -396,7 +394,7 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
 
 	case SNDRV_PCM_TRIGGER_STOP:
 		/* stop audio, disable interrupts */
-		ctl |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE;
+		ctl |= KIRKWOOD_PLAYCTL_PAUSE | mute_mask;
 		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
 
 		value = readl(priv->io + KIRKWOOD_INT_MASK);
@@ -410,13 +408,13 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
 
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
-		ctl |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE;
+		ctl |= KIRKWOOD_PLAYCTL_PAUSE | mute_mask;
 		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
 		break;
 
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		ctl &= ~(KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE);
+		ctl &= ~(KIRKWOOD_PLAYCTL_PAUSE | mute_mask);
 		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
 		break;
 
@@ -499,56 +497,6 @@ static int kirkwood_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 	return 0;
 }
 
-static int kirkwood_i2s_probe(struct snd_soc_dai *dai)
-{
-	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai);
-	unsigned long value;
-	unsigned int reg_data;
-	int ret;
-
-	ret = snd_soc_add_dai_controls(dai, kirkwood_i2s_iec958_controls,
-				ARRAY_SIZE(kirkwood_i2s_iec958_controls));
-	if (ret) {
-		dev_err(dai->dev, "unable to add soc card controls\n");
-		return ret;
-	}
-	/* put system in a "safe" state : */
-	/* disable audio interrupts */
-	writel(0xffffffff, priv->io + KIRKWOOD_INT_CAUSE);
-	writel(0, priv->io + KIRKWOOD_INT_MASK);
-
-	reg_data = readl(priv->io + 0x120c);
-	reg_data = readl(priv->io + 0x1200);
-	reg_data &= (~(0x333FF8));
-	reg_data |= 0x111D18;
-	writel(reg_data, priv->io + 0x1200);
-
-	msleep(500);
-
-	reg_data = readl(priv->io + 0x1200);
-	reg_data &= (~(0x333FF8));
-	reg_data |= 0x111D18;
-	msleep(500);
-	writel(reg_data, priv->io + 0x1200);
-
-	/* disable playback/record */
-	value = readl(priv->io + KIRKWOOD_PLAYCTL);
-	value &= ~(KIRKWOOD_PLAYCTL_I2S_EN|KIRKWOOD_PLAYCTL_SPDIF_EN);
-	writel(value, priv->io + KIRKWOOD_PLAYCTL);
-
-	value = readl(priv->io + KIRKWOOD_RECCTL);
-	value &= ~(KIRKWOOD_RECCTL_I2S_EN | KIRKWOOD_RECCTL_SPDIF_EN);
-	writel(value, priv->io + KIRKWOOD_RECCTL);
-
-	return 0;
-
-}
-
-static int kirkwood_i2s_remove(struct snd_soc_dai *dai)
-{
-	return 0;
-}
-
 static const struct snd_soc_dai_ops kirkwood_i2s_dai_ops = {
 	.startup	= kirkwood_i2s_startup,
 	.trigger	= kirkwood_i2s_trigger,
@@ -556,54 +504,57 @@ static const struct snd_soc_dai_ops kirkwood_i2s_dai_ops = {
 	.set_fmt        = kirkwood_i2s_set_fmt,
 };
 
+static int kirkwood_spdif_probe(struct snd_soc_dai *dai)
+{
+	int ret = snd_soc_add_dai_controls(dai, kirkwood_i2s_iec958_controls,
+				ARRAY_SIZE(kirkwood_i2s_iec958_controls));
+	if (ret)
+		dev_err(dai->dev, "unable to add soc card controls\n");
 
-static struct snd_soc_dai_driver kirkwood_i2s_dai = {
-	.probe = kirkwood_i2s_probe,
-	.remove = kirkwood_i2s_remove,
-	.playback = {
-		.channels_min = 1,
-		.channels_max = 2,
-		.rates = KIRKWOOD_I2S_RATES,
-		.formats = KIRKWOOD_I2S_FORMATS,
-	},
-	.capture = {
-		.channels_min = 1,
-		.channels_max = 2,
-		.rates = KIRKWOOD_I2S_RATES,
-		.formats = KIRKWOOD_I2S_FORMATS,
-	},
-	.ops = &kirkwood_i2s_dai_ops,
-};
+	return ret;
+}
 
-static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk = {
-	.probe = kirkwood_i2s_probe,
-	.remove = kirkwood_i2s_remove,
-	.playback = {
-		.channels_min = 1,
-		.channels_max = 2,
-		.rates = SNDRV_PCM_RATE_8000_192000 |
-			 SNDRV_PCM_RATE_CONTINUOUS |
-			 SNDRV_PCM_RATE_KNOT,
-		.formats = KIRKWOOD_I2S_FORMATS,
-	},
-	.capture = {
-		.channels_min = 1,
-		.channels_max = 2,
-		.rates = SNDRV_PCM_RATE_8000_192000 |
-			 SNDRV_PCM_RATE_CONTINUOUS |
-			 SNDRV_PCM_RATE_KNOT,
-		.formats = KIRKWOOD_I2S_FORMATS,
+static struct snd_soc_dai_driver kirkwood_dais[KIRKWOOD_NUM_DAIS] = {
+	{
+		.name = "kirkwood-i2s.%d",
+		.ops = &kirkwood_i2s_dai_ops,
+		.playback = {
+			.channels_min = 1,
+			.channels_max = 2,
+			.rates = KIRKWOOD_I2S_RATES,
+			.formats = KIRKWOOD_I2S_FORMATS,
+		},
+		.capture = {
+			.channels_min = 1,
+			.channels_max = 2,
+			.rates = KIRKWOOD_I2S_RATES,
+			.formats = KIRKWOOD_I2S_FORMATS,
+		},
+		.symmetric_rates = 1,
+	}, {
+		.name = "kirkwood-spdif.%d",
+		.probe = kirkwood_spdif_probe,
+		.ops = &kirkwood_i2s_dai_ops,
+		.playback = {
+			.channels_min = 1,
+			.channels_max = 2,
+			.rates = KIRKWOOD_I2S_RATES,
+			.formats = SNDRV_PCM_FMTBIT_S16_LE |
+				   SNDRV_PCM_FMTBIT_S24_LE |
+				   SNDRV_PCM_FMTBIT_S32_LE |
+				   SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE |
+				   SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE,
+		},
 	},
-	.ops = &kirkwood_i2s_dai_ops,
 };
 
 static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 {
 	struct kirkwood_asoc_platform_data *data = pdev->dev.platform_data;
-	struct snd_soc_dai_driver *soc_dai = &kirkwood_i2s_dai;
 	struct kirkwood_dma_data *priv;
 	struct resource *mem;
-	int err;
+	int i, err;
+	u32 val;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
@@ -612,6 +563,24 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 	}
 	dev_set_drvdata(&pdev->dev, priv);
 
+	memcpy(priv->dai_driver, kirkwood_dais, sizeof(priv->dai_driver));
+
+	/* format the DAI names according to the platform device ID */
+	for (i = 0; i < KIRKWOOD_NUM_DAIS; i++) {
+		const char *fmt = priv->dai_driver[i].name;
+		char *name, *p;
+
+		if (pdev->id < 0) {
+			name = kstrdup(fmt, GFP_KERNEL);
+			p = strchr(name, '.');
+			if (p)
+				*p = '\0';
+		} else {
+			name = kasprintf(GFP_KERNEL, fmt, pdev->id);
+		}
+		priv->dai_driver[i].name = name;
+	}
+
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!mem) {
 		dev_err(&pdev->dev, "platform_get_resource failed\n");
@@ -651,9 +620,23 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 			clk_put(priv->extclk);
 			priv->extclk = ERR_PTR(-EINVAL);
 		} else {
+			int i;
+
 			dev_info(&pdev->dev, "found external clock\n");
 			clk_prepare_enable(priv->extclk);
-			soc_dai = &kirkwood_i2s_dai_extclk;
+
+			for (i = 0; i < KIRKWOOD_NUM_DAIS; i++) {
+				if (priv->dai_driver[i].playback.rates)
+					priv->dai_driver[i].playback.rates =
+						SNDRV_PCM_RATE_8000_192000 |
+						SNDRV_PCM_RATE_CONTINUOUS |
+						SNDRV_PCM_RATE_KNOT;
+				if (priv->dai_driver[i].capture.rates)
+					priv->dai_driver[i].capture.rates =
+						SNDRV_PCM_RATE_8000_192000 |
+						SNDRV_PCM_RATE_CONTINUOUS |
+						SNDRV_PCM_RATE_KNOT;
+			}
 		}
 	}
 
@@ -673,7 +656,35 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 	writel(KIRKWOOD_MCLK_SOURCE_DCO, 
 	       priv->io+KIRKWOOD_CLOCKS_CTRL);
 
-	err = snd_soc_register_dai(&pdev->dev, soc_dai);
+	/* put system in a "safe" state : disable audio interrupts */
+	writel(0xffffffff, priv->io + KIRKWOOD_INT_CAUSE);
+	writel(0, priv->io + KIRKWOOD_INT_MASK);
+
+	val = readl(priv->io + 0x120c);
+	val = readl(priv->io + 0x1200);
+	val &= (~(0x333FF8));
+	val |= 0x111D18;
+	writel(val, priv->io + 0x1200);
+
+	msleep(500);
+
+	val = readl(priv->io + 0x1200);
+	val &= (~(0x333FF8));
+	val |= 0x111D18;
+	msleep(500);
+	writel(val, priv->io + 0x1200);
+
+	/* disable playback/record */
+	val = readl(priv->io + KIRKWOOD_PLAYCTL);
+	val &= ~(KIRKWOOD_PLAYCTL_I2S_EN|KIRKWOOD_PLAYCTL_SPDIF_EN);
+	writel(val, priv->io + KIRKWOOD_PLAYCTL);
+
+	val = readl(priv->io + KIRKWOOD_RECCTL);
+	val &= ~(KIRKWOOD_RECCTL_I2S_EN | KIRKWOOD_RECCTL_SPDIF_EN);
+	writel(val, priv->io + KIRKWOOD_RECCTL);
+
+	err = snd_soc_register_dais(&pdev->dev, priv->dai_driver,
+				    KIRKWOOD_NUM_DAIS);
 	if (!err)
 		return 0;
 	dev_err(&pdev->dev, "snd_soc_register_dai failed\n");
diff --git a/sound/soc/kirkwood/kirkwood-spdif.c b/sound/soc/kirkwood/kirkwood-spdif.c
index 2c459c1..62e5b62 100644
--- a/sound/soc/kirkwood/kirkwood-spdif.c
+++ b/sound/soc/kirkwood/kirkwood-spdif.c
@@ -61,7 +61,7 @@ static struct snd_soc_dai_link kirkwood_spdif_dai0[] = {
 		.name = "S/PDIF0",
 		.stream_name = "S/PDIF0 PCM Playback",
 		.platform_name = "kirkwood-pcm-audio.0",
-		.cpu_dai_name = "kirkwood-i2s.0",
+		.cpu_dai_name = "kirkwood-spdif.0",
 		.codec_dai_name = "dit-hifi",
 		.codec_name = "spdif-dit",
 		.ops = &kirkwood_spdif_ops,
@@ -73,7 +73,7 @@ static struct snd_soc_dai_link kirkwood_spdif_dai1[] = {
 		.name = "S/PDIF1",
 		.stream_name = "IEC958 Playback",
 		.platform_name = "kirkwood-pcm-audio.1",
-		.cpu_dai_name = "kirkwood-i2s.1",
+		.cpu_dai_name = "kirkwood-spdif.1",
 		.codec_dai_name = "dit-hifi",
 		.codec_name = "spdif-dit",
 		.ops = &kirkwood_spdif_ops,
diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h
index b7cf25c..c7ca27e 100644
--- a/sound/soc/kirkwood/kirkwood.h
+++ b/sound/soc/kirkwood/kirkwood.h
@@ -157,12 +157,15 @@
 #define KIRKWOOD_SND_MAX_PERIOD_BYTES		0x800000
 #define KIRKWOOD_SND_MAX_BUFFER_BYTES		0x100000
 
+#define KIRKWOOD_NUM_DAIS 2
+
 struct kirkwood_dma_data {
 	void __iomem *io;
 	struct clk *clk;
 	struct clk *extclk;
 	uint32_t ctl_play;
 	uint32_t ctl_rec;
+	struct snd_soc_dai_driver dai_driver[KIRKWOOD_NUM_DAIS];
 	struct snd_pcm_substream *substream_play;
 	struct snd_pcm_substream *substream_rec;
 	int irq;

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Sat, Aug 31, 2013 at 10:05:18PM +0100, Russell King - ARM Linux wrote:
> On Sat, Aug 31, 2013 at 10:46:35PM +0200, Lars-Peter Clausen wrote:
> > On 08/31/2013 09:19 PM, Russell King - ARM Linux wrote:
> > > On Sat, Aug 31, 2013 at 06:28:53PM +0100, Mark Brown wrote:
> > >> On Sat, Aug 31, 2013 at 05:28:26PM +0200, Lars-Peter Clausen wrote:
> > >>
> > >>> Then connect the SPDIF AIF widgets to the SPDIF streams and the I2S AIF
> > >>> widgets to the I2S streams. In your machine driver setup the link config to
> > >>> use DAI 0 if the board uses I2S and DAI 1 if the board uses SPDIF. The ASoC
> > >>> framework will then create the necessary routes all by itself. Of course
> > >>> this won't work on a platform where both the SPDIF and I2S controller are
> > >>> used at the same time, but neither does your current solution.
> > >>
> > >> This is the either/or approach I've been suggesting.
> > > 
> > > Ah, here we go again.  This is precisely why I can't work with you.
> > > Nothing I say seems to stick in your mind.  I've been telling you for the
> > > last four weeks that it doesn't work - and I've shown you the oopses and
> > > warnings I get from trying it.  Your response to date: nothing of any
> > > real use.
> > > 
> > > If you're not going to provide any useful responses on bug reports, it is
> > > totally unreasonable that you even suggest that _that_ is how it should
> > > be done when I've already proved that it's impossible at present.
> > 
> > Russell, please stop being such a bitch. You seem to be the only one who has
> > any problems working with Mark, which maybe should make you wonder if the
> > problem is not on your side. Insulting and screaming at people all the time
> > won't exactly motivate them to help you with your problems. Please try to be
> > constructive, it will makes things much easier for everyone.
> 
> Consider that Mark's been saying the same old stuff for the last four
> weeks, and I've been showing that it doesn't work, and the only thing
> that Mark says is the same old same old - maybe in the hope that if he
> says it enough times the bugs will mysteriously vanish.  Unfortunately,
> that isn't happening.
> 
> It's also not constructive or helpful, and it's extremely frustrating.
> I have very little patience left with ASoC at this point, having spent
> a month trying to get this stuff working.
> 
> I'm not the only one: I've heard other people complain about the _exact_
> same issue with ASoC: ASoC is difficult to work with, and many people
> just seem to give up with it - or keep their stuff in their own trees
> locally.  I can very well see why that happens.
> 
> As for "This is the either/or approach I've been suggesting."  No, that's
> another false statement from Mark.  What Mark has been suggesting all
> along is to use DPCM - and that's something which I tried for ages to
> get working, and it just doesn't work (as evidenced by the oopses and
> warnings that get spat out of the kernel.)
> 
> Mark suggested it _before_ he suggested DPCM, and I tried that approach.
> The problem is that it doesn't fit the hardware.  They aren't two
> separate streams - they're a single stream with two output paths, and
> there's restrictions in the hardware to do with how they're controlled.
> 
> There are two choices in how the hardware is used: either one output
> only is used, or if both outputs are used, they must be used "as one" -
> both outputs must be simultaneously enabled and disabled.  As far as
> I know, that's not possible with two DAIs.
> 
> And here's the patch I tried.
> 
> See - I've been down this route before.  I've tried it, and I know
> what it's problems are.  I'm not making up _anything_ here - I really
> have tried all these "suggestions" and I'm just going round in circles
> because Mark isn't listening to what I've been reporting back.

And this is how I ended up going down the DPCM route - this is from
private mails, so I'm only including those bits which I authored.
This is from an email exchange back in May (yes, this has been going
on since May!)

> > > So, I have this Armada 510 on the Cubox, which is wired up to use mainly
> > > the SPDIF output from the device.
> >
> > > The structure of the device (if you look at the page I point out in the
> > > PDF below) is:
> >
> > > Tx DMA --> Tx FIFO ----> I2S formatter ----> I2S output
> > >                     `--> SPDIF formatter --> SPDIF output
> >
> > > There are separate mute and enable bits for the I2S and SPDIF.  The mute
> > > bits can be set and cleared independently at any time (but it's
> > > recommended
> > > to set the mute bits before pausing the entire block).  However, the
> > > enable bits must be set and cleared simultaneously if both SPDIF and I2S
> > > output are required - setting one then the other is not permitted by
> > > the spec.
> >
> > > Now, Mark describes below about using two front ends and a single back
> > > end to describe this.  So, I've created two DAIs in kirkwood-i2s.c,
> > > one for I2S and one for SPDIF.  I've since come to the conclusion that
> > > this is wrong.  I'm now wondering how all this front end/back end stuff
> > > hangs together, and so forth.

(Liam asks why)

> It's wrong because there's no way to tie the two DAIs together and tell
> ASoC that the second DAI must not be started if the first one is already
> running.
>
> You can a card module come along, bind to the SPDIF one, then have the
> SPDIF output be used (so the transmission starts), then the new card
> module binds separately to the I2S one, at which point that automatically
> starts up - which then violates the conditions layed down that "both
> shall be started together or just one".
>
> There is no way in the CPU DAI backend to tell ALSA "these two DAIs are
> inherently linked and must be either started together or only one."

(Liam includes his relevant parts of a driver he is working on using
DPCM.)

So, it seems that you and Mark disagree with Liam.  That's just great.

I'm getting really tired of being told "you should do it this way" by
various people where their way is different.  I'm just going round and
round in circles with this.

[alsa-devel] [PATCH 00/14] SPDIF support

[Mark Brown]

On Sat, Aug 31, 2013 at 10:05:18PM +0100, Russell King - ARM Linux wrote:

> I'm not the only one: I've heard other people complain about the _exact_
> same issue with ASoC: ASoC is difficult to work with, and many people
> just seem to give up with it - or keep their stuff in their own trees
> locally.  I can very well see why that happens.

We do appear to have a fairly good success rate with systems working
with mainline; I can only think of one driver that didn't make it in and
that was one where we had a bit of an issue getting the code to visually
resemble Linux code so I don't feel too bad about it.  I am aware of
some people who didn't work upstream, we've generally had some useful
discussions with them once we've found each other.

> As for "This is the either/or approach I've been suggesting."  No, that's
> another false statement from Mark.  What Mark has been suggesting all
> along is to use DPCM - and that's something which I tried for ages to
> get working, and it just doesn't work (as evidenced by the oopses and
> warnings that get spat out of the kernel.)

While it is correct that I have been saying the final result should use
DPCM (since this is what the hardware looks like) you will recall that I
did recently suggest either/or as a stepping stone towards a full
implementation, allowing systems with only S/PDIF to be supported while
the other issues are still being worked on.

> There are two choices in how the hardware is used: either one output
> only is used, or if both outputs are used, they must be used "as one" -
> both outputs must be simultaneously enabled and disabled.  As far as
> I know, that's not possible with two DAIs.

That is correct, this is why I call it an either/or approach - a system
would be able to use either I2S or S/PDIF but not both.  For systems
with both I2S and S/PDIF connected one or the other would have to be
chosen by the machine driver.

> And here's the patch I tried.

Ah, I'm not sure that I have seen this before (it's certainly not been
submitted).  Just looking at the diff this all seems fine for an
either/or approach though...

> index 2c459c1..62e5b62 100644
> --- a/sound/soc/kirkwood/kirkwood-spdif.c
> +++ b/sound/soc/kirkwood/kirkwood-spdif.c
> @@ -61,7 +61,7 @@ static struct snd_soc_dai_link kirkwood_spdif_dai0[] = {
>  		.name = "S/PDIF0",
>  		.stream_name = "S/PDIF0 PCM Playback",
>  		.platform_name = "kirkwood-pcm-audio.0",
> -		.cpu_dai_name = "kirkwood-i2s.0",
> +		.cpu_dai_name = "kirkwood-spdif.0",
>  		.codec_dai_name = "dit-hifi",
>  		.codec_name = "spdif-dit",
>  		.ops = &kirkwood_spdif_ops,
> @@ -73,7 +73,7 @@ static struct snd_soc_dai_link kirkwood_spdif_dai1[] = {
>  		.name = "S/PDIF1",
>  		.stream_name = "IEC958 Playback",
>  		.platform_name = "kirkwood-pcm-audio.1",
> -		.cpu_dai_name = "kirkwood-i2s.1",
> +		.cpu_dai_name = "kirkwood-spdif.1",
>  		.codec_dai_name = "dit-hifi",
>  		.codec_name = "spdif-dit",
>  		.ops = &kirkwood_spdif_ops,

...this file does not appear to be in mainline and some of the rest of
the context suggested this was based off something old.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130901/2ecf5768/attachment.sig>

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Sun, Sep 01, 2013 at 01:19:28PM +0100, Mark Brown wrote:
> On Sat, Aug 31, 2013 at 10:05:18PM +0100, Russell King - ARM Linux wrote:
> 
> > I'm not the only one: I've heard other people complain about the _exact_
> > same issue with ASoC: ASoC is difficult to work with, and many people
> > just seem to give up with it - or keep their stuff in their own trees
> > locally.  I can very well see why that happens.
> 
> We do appear to have a fairly good success rate with systems working
> with mainline; I can only think of one driver that didn't make it in and
> that was one where we had a bit of an issue getting the code to visually
> resemble Linux code so I don't feel too bad about it.  I am aware of
> some people who didn't work upstream, we've generally had some useful
> discussions with them once we've found each other.

I wonder whether you include my ASoC sa11x0 assabet driver in that,
which I gave up trying to get into mainline, because it didn't fit
ASoC with its requirement to have the SSP transmit DMA running in
order to capture, and didn't use the latest soc-dmaengine stuff.
That driver remains in my tree, and is continually forward-ported,
and built in my nightly ARM builds.

> > As for "This is the either/or approach I've been suggesting."  No, that's
> > another false statement from Mark.  What Mark has been suggesting all
> > along is to use DPCM - and that's something which I tried for ages to
> > get working, and it just doesn't work (as evidenced by the oopses and
> > warnings that get spat out of the kernel.)
> 
> While it is correct that I have been saying the final result should use
> DPCM (since this is what the hardware looks like) you will recall that I
> did recently suggest either/or as a stepping stone towards a full
> implementation, allowing systems with only S/PDIF to be supported while
> the other issues are still being worked on.

Yes, and when I asked for details how you'd like that done, you
conveniently decided that you would not reply to that.

> > There are two choices in how the hardware is used: either one output
> > only is used, or if both outputs are used, they must be used "as one" -
> > both outputs must be simultaneously enabled and disabled.  As far as
> > I know, that's not possible with two DAIs.
> 
> That is correct, this is why I call it an either/or approach - a system
> would be able to use either I2S or S/PDIF but not both.  For systems
> with both I2S and S/PDIF connected one or the other would have to be
> chosen by the machine driver.
> 
> > And here's the patch I tried.
> 
> Ah, I'm not sure that I have seen this before (it's certainly not been
> submitted).  Just looking at the diff this all seems fine for an
> either/or approach though...

You wouldn't have seen this exact patch before, because it's a delta
between _this_ series and the one which I just built using the patch
from back in May 2013.  However, I believe you have seen the May 2013
patch at some point along the way.

> > index 2c459c1..62e5b62 100644
> > --- a/sound/soc/kirkwood/kirkwood-spdif.c
> > +++ b/sound/soc/kirkwood/kirkwood-spdif.c
> > @@ -61,7 +61,7 @@ static struct snd_soc_dai_link kirkwood_spdif_dai0[] = {
> >  		.name = "S/PDIF0",
> >  		.stream_name = "S/PDIF0 PCM Playback",
> >  		.platform_name = "kirkwood-pcm-audio.0",
> > -		.cpu_dai_name = "kirkwood-i2s.0",
> > +		.cpu_dai_name = "kirkwood-spdif.0",
> >  		.codec_dai_name = "dit-hifi",
> >  		.codec_name = "spdif-dit",
> >  		.ops = &kirkwood_spdif_ops,
> > @@ -73,7 +73,7 @@ static struct snd_soc_dai_link kirkwood_spdif_dai1[] = {
> >  		.name = "S/PDIF1",
> >  		.stream_name = "IEC958 Playback",
> >  		.platform_name = "kirkwood-pcm-audio.1",
> > -		.cpu_dai_name = "kirkwood-i2s.1",
> > +		.cpu_dai_name = "kirkwood-spdif.1",
> >  		.codec_dai_name = "dit-hifi",
> >  		.codec_name = "spdif-dit",
> >  		.ops = &kirkwood_spdif_ops,
> 
> ...this file does not appear to be in mainline and some of the rest of
> the context suggested this was based off something old.

Correct - and I have no intention of submitting this file, because it
presupposes mainline non-DT support for Dove.  Non-DT support for Dove
is being removed from mainline at present.  The mainline SIS5531 clock
driver which provides the external clock for this on the Cubox does
not support non-DT.

The non-DT support I run has full support for everything on Dove, which
means I can do much more in-depth testing than just running madplay or
something similar to check that there's audio out - which is about the
limit of what can be done with DT-only setups at the moment, such as
the DPCM bug triggered by vlc.

This is where those who are using mainline kernels with DT on Dove
platforms (like Jean-Francois and Sebastian) need a working solution to
this in a form which they can come up with a representative DT solution.
This is not a one-person effort - there's multiple efforts working on
this, and it's all inter-linked.

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Sun, Sep 01, 2013 at 01:34:33PM +0100, Russell King - ARM Linux wrote:
> The non-DT support I run has full support for everything on Dove, which
> means I can do much more in-depth testing than just running madplay or
> something similar to check that there's audio out - which is about the
> limit of what can be done with DT-only setups at the moment, such as
> the DPCM bug triggered by vlc.

Sorry, that was a bit unclear: I meant that I can do testing with vlc
on my setup, which I don't believe to be possible yet on DT setups.

[alsa-devel] [PATCH 00/14] SPDIF support

[Mark Brown]

On Sun, Sep 01, 2013 at 01:34:33PM +0100, Russell King - ARM Linux wrote:
> On Sun, Sep 01, 2013 at 01:19:28PM +0100, Mark Brown wrote:

> > We do appear to have a fairly good success rate with systems working
> > with mainline; I can only think of one driver that didn't make it in and
> > that was one where we had a bit of an issue getting the code to visually
> > resemble Linux code so I don't feel too bad about it.  I am aware of
> > some people who didn't work upstream, we've generally had some useful
> > discussions with them once we've found each other.

> I wonder whether you include my ASoC sa11x0 assabet driver in that,
> which I gave up trying to get into mainline, because it didn't fit
> ASoC with its requirement to have the SSP transmit DMA running in
> order to capture, and didn't use the latest soc-dmaengine stuff.
> That driver remains in my tree, and is continually forward-ported,
> and built in my nightly ARM builds.

To be honest not really, you've only mentioned it by providing links to
the git repository (it's never been posted) and there has been no real
discussion of the issues or other interest in getting the code
integrated.  For the capture DMA requirement you mention I'd expect a
driver local fix should be OK so long as it's well abstracted - it's not
likely to be a common issue.  

The approach I think I have suggested before is to get playback only
support merged then worry about dealing with the fun stuff needed for
capture support, that still seems like the most obvious way forwards if
there is a desire to get the code merged.

> > While it is correct that I have been saying the final result should use
> > DPCM (since this is what the hardware looks like) you will recall that I
> > did recently suggest either/or as a stepping stone towards a full
> > implementation, allowing systems with only S/PDIF to be supported while
> > the other issues are still being worked on.

> Yes, and when I asked for details how you'd like that done, you
> conveniently decided that you would not reply to that.

Indeed.  This was due to the way you asked:

   http://thread.gmane.org/gmane.linux.ports.arm.kernel/257542/focus=111909

which didn't indicate a great deal of interest in pursing this,
suggesting that it wasn't worth the effort going into more detail.
Something more like "that sounds like a useful idea, I've looked but I'm
not quite sure how to implement it" would have done so.

> This is where those who are using mainline kernels with DT on Dove
> platforms (like Jean-Francois and Sebastian) need a working solution to
> this in a form which they can come up with a representative DT solution.
> This is not a one-person effort - there's multiple efforts working on
> this, and it's all inter-linked.

Yes, and this is one of the reasons for suggesting getting either/or
support merged - it will help things like the binding definition
progress (as well as being useful for any users with a S/PDIF only
system).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130902/51e92dd1/attachment.sig>

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Mon, Sep 02, 2013 at 03:06:32PM +0100, Mark Brown wrote:
> On Sun, Sep 01, 2013 at 01:34:33PM +0100, Russell King - ARM Linux wrote:
> > This is where those who are using mainline kernels with DT on Dove
> > platforms (like Jean-Francois and Sebastian) need a working solution to
> > this in a form which they can come up with a representative DT solution.
> > This is not a one-person effort - there's multiple efforts working on
> > this, and it's all inter-linked.
> 
> Yes, and this is one of the reasons for suggesting getting either/or
> support merged - it will help things like the binding definition
> progress (as well as being useful for any users with a S/PDIF only
> system).

Sorry, but I believe the exact opposite:

1. The DAI link binding created for a dual-DAI driver is completely
   different from the DAI link binding for a DPCM driver.  The dual
   DAI link binding will have to be completely rewritten when the
   driver is converted to DPCM.

2. When the driver is converted to DPCM, it must use DPCM for
   everything, otherwise it has no way to know which of SPDIF or I2S to
   enable.  The only way I know to work around that is to add additional
   routes to link up the AIF widgets, and that's the solution you're all
   telling me is not acceptable, as per the patch set at the start of
   this thread.

[alsa-devel] [PATCH 00/14] SPDIF support

[Mark Brown]

On Mon, Sep 02, 2013 at 03:16:31PM +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 02, 2013 at 03:06:32PM +0100, Mark Brown wrote:

> > Yes, and this is one of the reasons for suggesting getting either/or
> > support merged - it will help things like the binding definition
> > progress (as well as being useful for any users with a S/PDIF only
> > system).

> Sorry, but I believe the exact opposite:

> 1. The DAI link binding created for a dual-DAI driver is completely
>    different from the DAI link binding for a DPCM driver.  The dual
>    DAI link binding will have to be completely rewritten when the
>    driver is converted to DPCM.

That seems like overstating the difficulties.  The updates for any new
S/PDIF drivers will be pretty much the same as those for the existing
I2S drivers and should just be mechanical changes, nothing too taxing.

> 2. When the driver is converted to DPCM, it must use DPCM for
>    everything, otherwise it has no way to know which of SPDIF or I2S to
>    enable.  The only way I know to work around that is to add additional
>    routes to link up the AIF widgets, and that's the solution you're all
>    telling me is not acceptable, as per the patch set at the start of
>    this thread.

What we have been telling you is that if there is a DAI link present
(there should be one for each physical DAI link) then this should be
enough information for the framework to know that the two DAIs are
linked and if any routes are needed in DAPM these should be added
automatically in the same way that we add links for CODEC<->CODEC links
at present.

It's been said to you off-list that having the links manually added
but marking them for removal when the framework figures out how to do
that should be OK.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130902/8a631a0f/attachment.sig>

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Mon, Sep 02, 2013 at 05:27:29PM +0100, Mark Brown wrote:
> On Mon, Sep 02, 2013 at 03:16:31PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Sep 02, 2013 at 03:06:32PM +0100, Mark Brown wrote:
> 
> > > Yes, and this is one of the reasons for suggesting getting either/or
> > > support merged - it will help things like the binding definition
> > > progress (as well as being useful for any users with a S/PDIF only
> > > system).
> 
> > Sorry, but I believe the exact opposite:
> 
> > 1. The DAI link binding created for a dual-DAI driver is completely
> >    different from the DAI link binding for a DPCM driver.  The dual
> >    DAI link binding will have to be completely rewritten when the
> >    driver is converted to DPCM.
> 
> That seems like overstating the difficulties.  The updates for any new
> S/PDIF drivers will be pretty much the same as those for the existing
> I2S drivers and should just be mechanical changes, nothing too taxing.

Sorry, you need to explain more.

Here's the dual-DAI version:

                .name = "S/PDIF1",
                .stream_name = "IEC958 Playback",
                .platform_name = "mvebu-audio.1",
                .cpu_dai_name = "kirkwood-spdif.1",
                .codec_dai_name = "dit-hifi",
                .codec_name = "spdif-dit",

Here's the DPCM version:

        {
                .name = "S/PDIF1",
                .stream_name = "Audio Playback",
                .platform_name = "mvebu-audio.1",
                .cpu_dai_name = "mvebu-audio.1",
                .codec_name = "snd-soc-dummy",
                .codec_dai_name = "snd-soc-dummy-dai",
                .dynamic = 1,
        }, {
                .name = "Codec",
                .stream_name = "IEC958 Playback",
                .cpu_dai_name = "snd-soc-dummy-dai",
                .platform_name = "snd-soc-dummy",
                .no_pcm = 1,
                .codec_dai_name = "dit-hifi",
                .codec_name = "spdif-dit",
        },

The above are two completely different beasts.  Please explain how the DT
representation for those DAI links can be the same.

> > 2. When the driver is converted to DPCM, it must use DPCM for
> >    everything, otherwise it has no way to know which of SPDIF or I2S to
> >    enable.  The only way I know to work around that is to add additional
> >    routes to link up the AIF widgets, and that's the solution you're all
> >    telling me is not acceptable, as per the patch set at the start of
> >    this thread.
> 
> What we have been telling you is that if there is a DAI link present
> (there should be one for each physical DAI link) then this should be
> enough information for the framework to know that the two DAIs are
> linked and if any routes are needed in DAPM these should be added
> automatically in the same way that we add links for CODEC<->CODEC links
> at present.
> 
> It's been said to you off-list that having the links manually added
> but marking them for removal when the framework figures out how to do
> that should be OK.

Sorry, I just don't get this.  What you seem to be telling me here is to
forget DPCM, and just go with the dual DAI solution.

If I have separate CPU DAI links, then I can't do simultaneous operation,
because both DAI links are entirely separate entities which are activated
entirely separately.  The only way I know how to handle this is with the
patch set I've already posted.

We've been through this before: this is also not how Liam's example DPCM
driver works - please go back and look at those diagrams I drew you of
how Liam's driver is setup.  I've also said to you that from what I can
see, the routes are still required for DPCM - those routes are in Liam's
DPCM driver as well.

[alsa-devel] [PATCH 00/14] SPDIF support

[Mark Brown]

On Mon, Sep 02, 2013 at 05:59:33PM +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 02, 2013 at 05:27:29PM +0100, Mark Brown wrote:

> > That seems like overstating the difficulties.  The updates for any new
> > S/PDIF drivers will be pretty much the same as those for the existing
> > I2S drivers and should just be mechanical changes, nothing too taxing.

> Sorry, you need to explain more.

> Here's the dual-DAI version:

>                 .name = "S/PDIF1",
>                 .stream_name = "IEC958 Playback",
>                 .platform_name = "mvebu-audio.1",
>                 .cpu_dai_name = "kirkwood-spdif.1",
>                 .codec_dai_name = "dit-hifi",
>                 .codec_name = "spdif-dit",

> Here's the DPCM version:
> 
>         {
>                 .name = "S/PDIF1",
>                 .stream_name = "Audio Playback",
>                 .platform_name = "mvebu-audio.1",
>                 .cpu_dai_name = "mvebu-audio.1",
>                 .codec_name = "snd-soc-dummy",
>                 .codec_dai_name = "snd-soc-dummy-dai",
>                 .dynamic = 1,
>         }, {
>                 .name = "Codec",
>                 .stream_name = "IEC958 Playback",
>                 .cpu_dai_name = "snd-soc-dummy-dai",
>                 .platform_name = "snd-soc-dummy",
>                 .no_pcm = 1,
>                 .codec_dai_name = "dit-hifi",
>                 .codec_name = "spdif-dit",
>         },

> The above are two completely different beasts.  Please explain how the DT
> representation for those DAI links can be the same.

If the binding is representing the differences between those two in the
DT then the DT binding is clearly not good since it is exposing Linux
implementation details - all the binding needs to say is that there's
something connected to the S/PDIF output of the audio controller.

> > > 2. When the driver is converted to DPCM, it must use DPCM for
> > >    everything, otherwise it has no way to know which of SPDIF or I2S to
> > >    enable.  The only way I know to work around that is to add additional
> > >    routes to link up the AIF widgets, and that's the solution you're all
> > >    telling me is not acceptable, as per the patch set at the start of
> > >    this thread.
> > 
> > What we have been telling you is that if there is a DAI link present
> > (there should be one for each physical DAI link) then this should be
> > enough information for the framework to know that the two DAIs are
> > linked and if any routes are needed in DAPM these should be added
> > automatically in the same way that we add links for CODEC<->CODEC links
> > at present.

> > It's been said to you off-list that having the links manually added
> > but marking them for removal when the framework figures out how to do
> > that should be OK.

> Sorry, I just don't get this.  What you seem to be telling me here is to
> forget DPCM, and just go with the dual DAI solution.

No, it's not clear to me why you would think that.  If we are talking
about adding DAPM routes matching DAI links then we are talking about
DPCM.  To reiterate the dual DAI implementation would merely be a
 stepping stone to a DPCM based implementation.

> If I have separate CPU DAI links, then I can't do simultaneous operation,
> because both DAI links are entirely separate entities which are activated
> entirely separately.  The only way I know how to handle this is with the
> patch set I've already posted.

As you are aware DPCM supports multiple DAIs and a good DPCM
implementation for this hardware will have one front end DAI for the DMA
and two back end DAIs, one for the S/PDIF interface and one for the I2S
interface.

> We've been through this before: this is also not how Liam's example DPCM
> driver works - please go back and look at those diagrams I drew you of
> how Liam's driver is setup.  I've also said to you that from what I can
> see, the routes are still required for DPCM - those routes are in Liam's
> DPCM driver as well.

This is why in the above message I reminded you of the suggestion that
until the framework does automatically figure out that those routes
should be there the routes are manually added in the driver clearly
marked so they can easily be removed when the framework does the right
thing here.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130902/d125bfda/attachment-0001.sig>

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Mon, Sep 02, 2013 at 09:44:21PM +0100, Mark Brown wrote:
> On Mon, Sep 02, 2013 at 05:59:33PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Sep 02, 2013 at 05:27:29PM +0100, Mark Brown wrote:
> 
> > > That seems like overstating the difficulties.  The updates for any new
> > > S/PDIF drivers will be pretty much the same as those for the existing
> > > I2S drivers and should just be mechanical changes, nothing too taxing.
> 
> > Sorry, you need to explain more.
> 
> > Here's the dual-DAI version:
> 
> >                 .name = "S/PDIF1",
> >                 .stream_name = "IEC958 Playback",
> >                 .platform_name = "mvebu-audio.1",
> >                 .cpu_dai_name = "kirkwood-spdif.1",
> >                 .codec_dai_name = "dit-hifi",
> >                 .codec_name = "spdif-dit",
> 
> > Here's the DPCM version:
> > 
> >         {
> >                 .name = "S/PDIF1",
> >                 .stream_name = "Audio Playback",
> >                 .platform_name = "mvebu-audio.1",
> >                 .cpu_dai_name = "mvebu-audio.1",
> >                 .codec_name = "snd-soc-dummy",
> >                 .codec_dai_name = "snd-soc-dummy-dai",
> >                 .dynamic = 1,
> >         }, {
> >                 .name = "Codec",
> >                 .stream_name = "IEC958 Playback",
> >                 .cpu_dai_name = "snd-soc-dummy-dai",
> >                 .platform_name = "snd-soc-dummy",
> >                 .no_pcm = 1,
> >                 .codec_dai_name = "dit-hifi",
> >                 .codec_name = "spdif-dit",
> >         },
> 
> > The above are two completely different beasts.  Please explain how the DT
> > representation for those DAI links can be the same.
> 
> If the binding is representing the differences between those two in the
> DT then the DT binding is clearly not good since it is exposing Linux
> implementation details - all the binding needs to say is that there's
> something connected to the S/PDIF output of the audio controller.

Can you explain how the kernel would know the difference between a DPCM
and a non-DPCM setup from just the information in the DT please?

> > > > 2. When the driver is converted to DPCM, it must use DPCM for
> > > >    everything, otherwise it has no way to know which of SPDIF or I2S to
> > > >    enable.  The only way I know to work around that is to add additional
> > > >    routes to link up the AIF widgets, and that's the solution you're all
> > > >    telling me is not acceptable, as per the patch set at the start of
> > > >    this thread.
> > > 
> > > What we have been telling you is that if there is a DAI link present
> > > (there should be one for each physical DAI link) then this should be
> > > enough information for the framework to know that the two DAIs are
> > > linked and if any routes are needed in DAPM these should be added
> > > automatically in the same way that we add links for CODEC<->CODEC links
> > > at present.
> 
> > > It's been said to you off-list that having the links manually added
> > > but marking them for removal when the framework figures out how to do
> > > that should be OK.
> 
> > Sorry, I just don't get this.  What you seem to be telling me here is to
> > forget DPCM, and just go with the dual DAI solution.
> 
> No, it's not clear to me why you would think that.  If we are talking
> about adding DAPM routes matching DAI links then we are talking about
> DPCM.  To reiterate the dual DAI implementation would merely be a
>  stepping stone to a DPCM based implementation.

Your reply to my question about DPCM seems to be talking about the
dual-DAI solution.  Maybe you need to be more specific about what
"two DAIs" you are referring to.

> > If I have separate CPU DAI links, then I can't do simultaneous operation,
> > because both DAI links are entirely separate entities which are activated
> > entirely separately.  The only way I know how to handle this is with the
> > patch set I've already posted.
> 
> As you are aware DPCM supports multiple DAIs and a good DPCM
> implementation for this hardware will have one front end DAI for the DMA
> and two back end DAIs, one for the S/PDIF interface and one for the I2S
> interface.

Isn't that what this patch series creates - the front end DAI is the CPU
DAI, and the backend DAIs are provided by the snd-soc-dummy-dai.  I
refer you to this in Liam's driver:

        /* Back End DAI links */
        {
                /* SSP0 - Codec */
                .name = "Codec",
                .be_id = 0,
                .cpu_dai_name = "snd-soc-dummy-dai",
                .platform_name = "snd-soc-dummy",
                .no_pcm = 1,
                .codec_name = "...0-001c",
                .codec_dai_name = "...-aif1",
                .ignore_suspend = 1,
                .ignore_pmdown_time = 1,
                .be_hw_params_fixup = ...,
                .ops = &haswell_ops,
                .dpcm_playback = 1,
                .dpcm_capture = 1,
        },
        {
                /* SSP1 - BT */
                .name = "SSP1-Codec",
                .be_id = 1,
                .cpu_dai_name = "snd-soc-dummy-dai",
                .platform_name = "snd-soc-dummy",
                .no_pcm = 1,
                .codec_name = "snd-soc-dummy",
                .codec_dai_name = "snd-soc-dummy-dai",
                .ignore_suspend = 1,
                .ignore_pmdown_time = 1,
        },

Notice that for the DPCM BE, there is no CPU-layer involvement here.

The difference here is that at the moment, with this patch series plus
the DPCM add-on patch, I am only creating one BE, but that BE is being
created in exactly the same way as the above is doing.

> > We've been through this before: this is also not how Liam's example DPCM
> > driver works - please go back and look at those diagrams I drew you of
> > how Liam's driver is setup.  I've also said to you that from what I can
> > see, the routes are still required for DPCM - those routes are in Liam's
> > DPCM driver as well.
> 
> This is why in the above message I reminded you of the suggestion that
> until the framework does automatically figure out that those routes
> should be there the routes are manually added in the driver clearly
> marked so they can easily be removed when the framework does the right
> thing here.

I'm not sure how the framework could figure out these things
automatically.  If you go back to the diagram which I drew you for
Liam's driver, it's not a simple CPU-AIF to Codec linkage - there's
a mixer in the middle of it as well.  There's also feedback from that
mixer (which is on the output side) to an input stream to the CPU DAI
side.

Liam also suggested that there could be DAPM routing which can control
how the FE and BEs are linked together.

So, I still don't see that there is anything wrong with my patch series
plus DPCM-enabling patch, apart from the issues which I've reported.
Maybe you could review it and provide specific comments against the
patches highlighting the code which you have issues with.

As for providing ALSA with a set of PCM operations, I'm not sure what
they should be for a DPCM backend.

[alsa-devel] [PATCH 00/14] SPDIF support

[Mark Brown]

On Mon, Sep 02, 2013 at 10:18:30PM +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 02, 2013 at 09:44:21PM +0100, Mark Brown wrote:

> > If the binding is representing the differences between those two in the
> > DT then the DT binding is clearly not good since it is exposing Linux
> > implementation details - all the binding needs to say is that there's
> > something connected to the S/PDIF output of the audio controller.

> Can you explain how the kernel would know the difference between a DPCM
> and a non-DPCM setup from just the information in the DT please?

In the short term from the machine driver of course - linking things
together is what it's there for.  In the longer term I'd expect that the
compatible string could be moved over to a generic machine driver which
can understand the way we're representing the device internally, or
perhaps we'll change that internal representation and it'd do something
totally different anyway, but no need to worry about that for now.

> > > > What we have been telling you is that if there is a DAI link present
> > > > (there should be one for each physical DAI link) then this should be
> > > > enough information for the framework to know that the two DAIs are
> > > > linked and if any routes are needed in DAPM these should be added
> > > > automatically in the same way that we add links for CODEC<->CODEC links
> > > > at present.

> Your reply to my question about DPCM seems to be talking about the
> dual-DAI solution.  Maybe you need to be more specific about what
> "two DAIs" you are referring to.

The two DAIs that are linked by a DAI link are the two DAIs at either
end of the link, for example the I2S DAI on the SoC and the I2S DAI on
the CODEC.

> > As you are aware DPCM supports multiple DAIs and a good DPCM
> > implementation for this hardware will have one front end DAI for the DMA
> > and two back end DAIs, one for the S/PDIF interface and one for the I2S
> > interface.

> Isn't that what this patch series creates - the front end DAI is the CPU
> DAI, and the backend DAIs are provided by the snd-soc-dummy-dai.  I

Not in this patch series as far as I can see, there appear to be no
references to the dummy DAI in it.  From a comment later on in your mail
I suspect you're thinking of the result of adding some additional
changes to the series, please squash those into the existing patches
appropriately and resubmit.

> Notice that for the DPCM BE, there is no CPU-layer involvement here.

> The difference here is that at the moment, with this patch series plus
> the DPCM add-on patch, I am only creating one BE, but that BE is being
> created in exactly the same way as the above is doing.

You should have one back end DAI per physical DAI.

> > This is why in the above message I reminded you of the suggestion that
> > until the framework does automatically figure out that those routes
> > should be there the routes are manually added in the driver clearly
> > marked so they can easily be removed when the framework does the right
> > thing here.

> I'm not sure how the framework could figure out these things
> automatically.  If you go back to the diagram which I drew you for
> Liam's driver, it's not a simple CPU-AIF to Codec linkage - there's
> a mixer in the middle of it as well.  There's also feedback from that

This is not correct, there is no mixer in the link between the back end
and the CODEC.

> mixer (which is on the output side) to an input stream to the CPU DAI
> side.

> Liam also suggested that there could be DAPM routing which can control
> how the FE and BEs are linked together.

This is correct, it is in fact required for operation - you are of
course well aware that front ends and back ends are not connected using
DAI links but are instead connected using normal DAPM links.  This
should all be very simple for Kirkwood.

> So, I still don't see that there is anything wrong with my patch series
> plus DPCM-enabling patch, apart from the issues which I've reported.
> Maybe you could review it and provide specific comments against the
> patches highlighting the code which you have issues with.

I think the most serious issues with the current series were already
covered by Lars-Peter and the discussion in these subtreads, no need to
repeat things.

> As for providing ALSA with a set of PCM operations, I'm not sure what
> they should be for a DPCM backend.

Unless there is a need to actually take some action you can, naturally,
provide stubs.  You should provide the minimal set of stubs required for
operation in your testing.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130902/25592c19/attachment.sig>

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Mon, Sep 02, 2013 at 11:35:00PM +0100, Mark Brown wrote:
> Not in this patch series as far as I can see, there appear to be no
> references to the dummy DAI in it.  From a comment later on in your mail
> I suspect you're thinking of the result of adding some additional
> changes to the series, please squash those into the existing patches
> appropriately and resubmit.

I'm thinking of making _no_ changes to this series at present, because
I fail to see anything wrong with it.

As for "no references to the dummy DAI in it" - I did mention the
additional patch which is _not_ part of this series which contains
the changes to enable DPCM.  Here's an extract which I posted just
two messages before this one of the resulting DAI link structure:

Here's the DPCM version:                                                        
                                                                                
        {                                                                       
                .name = "S/PDIF1",                                              
                .stream_name = "Audio Playback",                                
                .platform_name = "mvebu-audio.1",                               
                .cpu_dai_name = "mvebu-audio.1",                                
                .codec_name = "snd-soc-dummy",                                  
                .codec_dai_name = "snd-soc-dummy-dai",                          
                .dynamic = 1,                                                   
        }, {                                                                    
                .name = "Codec",                                                
                .stream_name = "IEC958 Playback",                               
                .cpu_dai_name = "snd-soc-dummy-dai",                            
                .platform_name = "snd-soc-dummy",                               
                .no_pcm = 1,                                                    
                .codec_dai_name = "dit-hifi",                                   
                .codec_name = "spdif-dit",                                      
        },                                                                      

Please see the second entry.  This is the backend DAI setup.  This refers
to the dummy DAI for the CPU, and has 'no_pcm' set - both of which I
believe are required to indicate that this is representing a backend DAI.

As I've said before, the reason I haven't included this file is that it
is unclear that it is useful for non-DT setups.  As there is no non-DT
Cubox support in the kernel and never will be, I don't see any point
submitting it as part of this series.

Secondly, I don't see the point of it being part of this series, because
if we merge the changes for DPCM support, the thing falls apart - I'm
not going to list the problems yet again.

> > Notice that for the DPCM BE, there is no CPU-layer involvement here.
> 
> > The difference here is that at the moment, with this patch series plus
> > the DPCM add-on patch, I am only creating one BE, but that BE is being
> > created in exactly the same way as the above is doing.
> 
> You should have one back end DAI per physical DAI.

What do you mean "physical DAI"?  Do you mean "CPU DAI" which would be a
front end DAI?

Liam told me that it was possible to have multiple backends for a single
front end.  You've also told me on IRC:

16:29 < broonie> I have a pretty good idea of how I'd expect to see the
 hardware represented - two BEs for the DAIs connected to a FE for the DMA.

That isn't two FEs.  You explicitly state there "two BEs" for a single FE.

> > > This is why in the above message I reminded you of the suggestion that
> > > until the framework does automatically figure out that those routes
> > > should be there the routes are manually added in the driver clearly
> > > marked so they can easily be removed when the framework does the right
> > > thing here.
> 
> > I'm not sure how the framework could figure out these things
> > automatically.  If you go back to the diagram which I drew you for
> > Liam's driver, it's not a simple CPU-AIF to Codec linkage - there's
> > a mixer in the middle of it as well.  There's also feedback from that
> 
> This is not correct, there is no mixer in the link between the back end
> and the CODEC.

Let me re-post the structure of the widgets linking the codecs streams and
CPU streams in Liam's driver then.

CPU DAI drivers:    DAI stream names:
--------------------------------------
System Pin          System Playback
Offload0 Pin        Offload0 Playback
Offload1 Pin        Offload1 Playback
Loopback Pin        Loopback Capture
Capture Pin         Analog Capture

DAPM routes:
[s]System Playback   --------v           .----> [aif]SSP0 CODEC OUT
[s]Offload0 Playback ---> [w]Playback VMixer
[s]Offload1 Playback --------^           `----> [s]Loopback Capture
 
[aif]SSP0 CODEC IN ---> [s]Analog Capture

where [s] is a stream widget, [w] is a DAPM widget, and [aif] is an
AIF widget.

Looks like there's a mixer in there to me.  It may not be a bit of
hardware which actually does mixing or anything (I don't know what it
does) but that appears to be DAPM structure which Liam creates between
the CPU streams and the codecs.

> > mixer (which is on the output side) to an input stream to the CPU DAI
> > side.
> 
> > Liam also suggested that there could be DAPM routing which can control
> > how the FE and BEs are linked together.
> 
> This is correct, it is in fact required for operation - you are of
> course well aware that front ends and back ends are not connected using
> DAI links but are instead connected using normal DAPM links.  This
> should all be very simple for Kirkwood.

And these are the DAPM routes which you're telling me to put a comment
against because they will need to be removed.  I can't see why they'd
ever need to be removed, and I can't see how ASoC could possibly
know the structure between the CPU and codec in DPCM solutions.

> > So, I still don't see that there is anything wrong with my patch series
> > plus DPCM-enabling patch, apart from the issues which I've reported.
> > Maybe you could review it and provide specific comments against the
> > patches highlighting the code which you have issues with.
> 
> I think the most serious issues with the current series were already
> covered by Lars-Peter and the discussion in these subtreads, no need to
> repeat things.

Sorry, I thought Lars-Peter's comments were about getting something that
would be acceptable to go in _before_ DPCM was in a working state.

So, I'll re-state again.  I see nothing wrong with my patch series plus
the DPCM-enabling patch.  I see this as a complete and proper DPCM
solution with no flaws in the driver code what so ever.  I don't see
Lars-Peter's comments being relevant to the DPCM solution, but only to
a stop-gap solution.

> > As for providing ALSA with a set of PCM operations, I'm not sure what
> > they should be for a DPCM backend.
> 
> Unless there is a need to actually take some action you can, naturally,
> provide stubs.  You should provide the minimal set of stubs required for
> operation in your testing.

That needs to go in the ASoC code though - I don't think I have access
to the ALSA PCM which has been created for the backend DAI(s).  Remember,
the backend DAIs are "owned" by the dummy DAI driver, not the CPU driver,
so the CPU driver doesn't get to see any operations for that.

[alsa-devel] [PATCH 00/14] SPDIF support

[Mark Brown]

On Tue, Sep 03, 2013 at 12:00:11AM +0100, Russell King - ARM Linux wrote:

> > You should have one back end DAI per physical DAI.

> What do you mean "physical DAI"?  Do you mean "CPU DAI" which would be a
> front end DAI?

The same question got asked and answered in another subthread but for
the record I'm speaking here about the back end DAIs - there should be
one for S/PDIF and one for I2S.  With DPCM there should also be a front
end DAI for the DMA.  I've cut a bunch of other discussion which was
duplicated in that subthread.

> > Unless there is a need to actually take some action you can, naturally,
> > provide stubs.  You should provide the minimal set of stubs required for
> > operation in your testing.

> That needs to go in the ASoC code though - I don't think I have access
> to the ALSA PCM which has been created for the backend DAI(s).  Remember,
> the backend DAIs are "owned" by the dummy DAI driver, not the CPU driver,
> so the CPU driver doesn't get to see any operations for that.

If you need to define operations for the dummy driver then modify the
dummy driver.  However as indicated in the other thread you should not
be using the dummy DAI driver for the CPU side back end.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130904/82e6cc89/attachment.sig>

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Sat, Aug 31, 2013 at 10:46:35PM +0200, Lars-Peter Clausen wrote:
> On 08/31/2013 09:19 PM, Russell King - ARM Linux wrote:
> > On Sat, Aug 31, 2013 at 06:28:53PM +0100, Mark Brown wrote:
> >> On Sat, Aug 31, 2013 at 05:28:26PM +0200, Lars-Peter Clausen wrote:
> >>
> >>> Then connect the SPDIF AIF widgets to the SPDIF streams and the I2S AIF
> >>> widgets to the I2S streams. In your machine driver setup the link config to
> >>> use DAI 0 if the board uses I2S and DAI 1 if the board uses SPDIF. The ASoC
> >>> framework will then create the necessary routes all by itself. Of course
> >>> this won't work on a platform where both the SPDIF and I2S controller are
> >>> used at the same time, but neither does your current solution.
> >>
> >> This is the either/or approach I've been suggesting.
> > 
> > Ah, here we go again.  This is precisely why I can't work with you.
> > Nothing I say seems to stick in your mind.  I've been telling you for the
> > last four weeks that it doesn't work - and I've shown you the oopses and
> > warnings I get from trying it.  Your response to date: nothing of any
> > real use.
> > 
> > If you're not going to provide any useful responses on bug reports, it is
> > totally unreasonable that you even suggest that _that_ is how it should
> > be done when I've already proved that it's impossible at present.
> 
> Russell, please stop being such a bitch. You seem to be the only one who has
> any problems working with Mark, which maybe should make you wonder if the
> problem is not on your side. Insulting and screaming at people all the time
> won't exactly motivate them to help you with your problems. Please try to be
> constructive, it will makes things much easier for everyone.

Here's what was said on August 4th, which is also where I'm being
encouraged to persue the DPCM route.  Since IRC is technically public
forum, there is no problem posting this, and this was all discussed
openly in a channel where others were present.

This discussion happened after the previous set of patches were posted.

16:29 < broonie> I have a pretty good idea of how I'd expect to see the hardware represented - two BEs for the DAIs connected to a FE for the DMA.
16:29 < broonie> Probably just hard wired, or perhaps with a soft switch between the two.
16:31 < rmk> so I have Liam's (example) driver.  It doesn't help me understand this DPCM stuff - as I said in my emails over the weekend.
16:31 < broonie> Right, I've never actually looked at that.
16:31 < broonie> Since he's not posted it ye.t
16:32 < rmk> right, so what you're basically saying is "stop working around ASoC, and use a subsystem which no one except Liam understands"
16:32 < rmk> ... which not even _you_ understand yet
16:33 < broonie> I'd expect people like Peter to have a good handle on it too.
16:34 < broonie> Do things not hang together if ou create the links like he has in haswell_dais?
16:35 < broonie> With the dummy CODEC on the FE and the dummy CPU on the BE?
16:35 < rmk> well, if I'm reading it correctly, it has multiple frontends connected to two backends and I can't work out how the frontends are connected backends
16:36 < broonie> Yes, that's what it should be doing.
16:37 < rmk> the problem I have is the same old problem: how do I know which backend(s) are active from the frontend's perspective?
16:37 < broonie> They're hooked in via the Playback VMixer AFAICT (I can't run this stuff obviously).
16:38 < broonie> I'd expect the BE to do the caring but I know you need to enable them simultaneously.
16:38 < broonie> So I'd have the BEs set flags prior to being powered
16:38 < broonie> Then check in the FE.
16:39 < broonie> Or to a first approximation you can probably get away with just always enabling both if the pinmux is set up to output them.
16:39 < broonie> and/or the DAI links are present.
16:41 < rmk> so, I need to put two DAI drivers in the CPU level, a DAI link to setup the frontends, DAPM links to connect the front end streams to the back ends yes?
16:42 < broonie> Yes.
16:44 < rmk> and I can register the front end DAI driver separately from the two backends?
16:44 < rmk> via two snd_soc_register_component
16:45 < broonie> That should be possible, yes.
16:48 < rmk> right, I'll give that a go sometime this week, and I'll add some kind of documentation on this if I get it to work, because its really not fair not to have anything on this stuff.
16:49 < rmk> oh, another question
16:49 < rmk> .dynamic in the dai link structure, that's for backends only, right?
16:50 < broonie> Think so, yes.
16:52 < rmk> hmm
16:52 < rmk>         if (rtd->dai_link->dynamic) {
16:52 < rmk>                 rtd->ops.open           = dpcm_fe_dai_open;
16:52 < rmk> probably needed for frontends
17:53 < rmk> no, this can't work, all the DAI links have to be registered in one place and one place only.
17:53 < rmk> that's at the soc_card level
17:53 < broonie> Yes, currently it involves cut'n'paste in the cards which is sucky.
17:57 < rmk> well, having been through the (Liam's example) stuff, what I've currently got in kirkwood-i2s is correct and to what Liam's doing
17:58 < rmk> Liam has this:

(table of DAI drivers and stream names omitted)

17:58 < rmk> He then sets up a set of widgets and routing like this:

(diagram of widgets and routes removed)

17:58 < rmk> where [s] are streams, [w] are non-aif widgets, and [aif] are aif widgets
18:00 < rmk> what's different is the DAI links, and the backend codec streams are attached with DAPM routes to those aif widgets
18:12 < rmk> and I already have the DAPM routes.
18:13 < rmk> so literally the only change I've made so far is changing the DAI links
19:10 < rmk> broonie: well, with that change, I see no widgets being powered up

(output from /sys/kernel/debug/asoc/... cut)

19:12 < broonie> OK.
19:13 < broonie> Looking at that we don't seem to be kicking *any* of the DAIs to power up.
19:16 < broonie> Are all the relevant trigger functions getting called?
19:20 * rmk hits another error on reloading the modules...
19:21 < rmk> ------------[ cut here ]------------
19:21 < rmk> WARNING: at /home/rmk/git/linux-cubox/fs/proc/generic.c:356 proc_register+0xac/0
19:21 < rmk> x128()
19:21 < rmk> proc_dir_entry 'asound/oss' already registered
19:21 < rmk> but...
19:21 < rmk> kirkwood_i2s_play_trigger: 101b ffffffe7
19:21 < rmk> yes it does get called but because the widgets aren't powered up neither enable bit is set
19:31 < broonie> OK, I was checking to see how much it was missing doing.
19:37 < broonie> The be_clients list on the front end must be empty...
19:38 < rmk> err...
19:38 < broonie> Which in turn comes from dpcm_add_paths() not noticing the links.
19:43 < rmk> well, I have no_pcm set in the backend dai_link
19:43 < broonie> Probably easiest to debug with prints.
19:43 < rmk> there's quite a few errors in the logs too
19:43 < rmk> snd-soc-dummy snd-soc-dummy: ASoC: Failed to create platform debugfs directory
19:47 < broonie> That's probably getting added twice somehow then.
19:49 < rmk> The DAI widgets are being overwritten... again
19:49 < rmk> snd_soc_dapm_new_dai_widgets: creating playback widget Playback:Playback for dai d81247c0 dapm d8263f10 (playback_widget was   (null), new c05ab080)
19:50 < rmk> snd_soc_dapm_new_dai_widgets: creating playback widget Playback:Playback for dai d81247c0 dapm d8263c70 (playback_widget was c05ab080, new d8fe2b40)
19:51 < rmk> that's output by this in snd_soc_dapm_new_dai_widgets:
19:51 < rmk>                 w = snd_soc_dapm_new_control(dapm, &template);
19:51 < rmk> printk("%s: creating playback widget %s:%s for dai %p dapm %p (playback_widget was %p, new %p)\n", __func__, template.name, template.sname, dai, dapm, dai->playback_widget, w);
19:51 < rmk>                 if (!w) {
19:51 < rmk>                         dev_err(dapm->dev, "ASoC: Failed to create %s widget\n",
20:04 * rmk tries commenting out Liam's addition and uncommenting the one in my HACK patch
20:09 < rmk> nope, that doesn't fix it either
20:19 < rmk> broonie: looks like this also sends pulseaudio into a hissy fit
20:20 < broonie> Not surprising, pulseaudio is probably the best testsuite we have for DMA related stuff.
20:38 < rmk> not so helpful when it hammers on the mixer opening and closing it continuously, flooding the kernel log
20:42 < rmk>  S!PDIF1: ASoC: found 0 audio playback paths
20:42 < rmk>  S!PDIF1: ASoC: S/PDIF1 no valid playback route
20:42 < rmk>  S!PDIF1: ASoC: found 0 new BE paths
20:42 < rmk>  S!PDIF1: ASoC: open FE S/PDIF1
20:42 < rmk>  S!PDIF1: ASoC: hw_params FE S/PDIF1 rate 44100 chan 2 fmt 2
20:42 < rmk>  S!PDIF1: ASoC: prepare FE S/PDIF1
20:42 < rmk>  S!PDIF1: ASoC: no backend DAIs enabled for S/PDIF1
20:42 < rmk>  S!PDIF1: ASoC: hw_free FE S/PDIF1
20:42 < rmk>  S!PDIF1: ASoC: hw_params FE S/PDIF1 rate 44100 chan 2 fmt 2
20:42 < rmk>  S!PDIF1: ASoC: prepare FE S/PDIF1
20:42 < rmk> ...
20:58 < rmk> right, the problem is this - look carefully at this:
20:58 < rmk> snd-soc-dummy/dapm/Playback:Playback: Off  in 0 out 0
20:58 < rmk> snd-soc-dummy/dapm/Playback: stream Playback inactive
20:58 < rmk> snd-soc-dummy/dapm/Playback: in  "static" "spdifdo"
20:58 < rmk> spdif-dit/dapm/Playback:Playback: Off  in 0 out 1
20:58 < rmk> spdif-dit/dapm/Playback: stream Playback inactive
20:58 < rmk> spdif-dit/dapm/Playback: out "static" "spdif-out"
20:59 < rmk> how do I tell asoc to connect to the spdif-dit "playback" widget rather than the dummy "playback" widget which has the same name?
21:00 < broonie> Ugh. simplest thing for now is going to be to rename one of them I expect.
21:00 < broonie> That's not nice or a good long term fix but it should get you over the hurdle.
21:00 < rmk> so... the answer to my question from yesterday is that yes, we're going to have to give all these things unique names
21:01 < broonie> If they're in the same DAPM context it should be fine.
21:01 < rmk> are they with this?
21:01 < rmk> aren't they in separate contexts?
21:02 < broonie> You're linking spdif-dit to something outside spdif-dit though.
21:02 < rmk> ones in the dummy codec's dapm context, the other in the spdif-dit dapm context
21:02 < broonie> I mean if the source and destination are in the same context.
21:02 < broonie> That's the case that we do the deupe for.
21:05 < rmk> well, it still looks to me like there's a fair number of problems with this stuff which need sorting
21:05 < rmk> not only this but the multiple widget creation problem
21:06 < broonie> It's not perfect, no - I'd certainly expect the CPU side stuff to need fixups since it's never been used in anger.
21:06 < broonie> In mainline.
21:07 < rmk> well, someone needs to come up with a fix for these duplicated widgets, and that's not me.
21:07 < rmk> because... at the moment... asoc is completely useless for this hardware

The reason for my last couple of lines there is not that I'm being
difficult - I really don't know what the correct fix for the duplicated
widget problem is, and I still don't know.  What I do know is that I've
been able to work around it for _this_ patch set and not require the
hack to get it working in this form.

As far as I'm aware, the only person who knows what the answer to that
problem is Liam.

Notice the first two lines: this is the solution which Liam suggested,
which is the DPCM solution, and that's the solution I've been working
towards.  Mark there confirms that it's his expected solution.

Here's my patch which converts the Cubox SPDIF-only support to DPCM, with
all the side effects of the warnings from procfs and the oops from ALSA,
and shuts up a very annoying warning which fills the kernel log at a
rediculous rate, preventing other kernel messages from being seen.  You
can issues discussed above being solved in this patch, namely the
duplicated "Playback" widget.

As the single-frontend single-backend DPCM arrangement triggers warnings
and a kernel oops, there's not much point trying to move forward to a
multi-backend arrangement.

diff --git a/sound/soc/codecs/spdif_transciever.c b/sound/soc/codecs/spdif_transciever.c
index 553708d..fde0150 100644
--- a/sound/soc/codecs/spdif_transciever.c
+++ b/sound/soc/codecs/spdif_transciever.c
@@ -34,7 +34,7 @@ static const struct snd_soc_dapm_widget dit_widgets[] = {
 };
 
 static const const struct snd_soc_dapm_route dit_routes[] = {
-	{ "spdif-out", NULL, "Playback" },
+	{ "spdif-out", NULL, "spdif-Playback" },
 };
 
 static struct snd_soc_codec_driver soc_codec_spdif_dit = {
@@ -47,7 +47,7 @@ static struct snd_soc_codec_driver soc_codec_spdif_dit = {
 static struct snd_soc_dai_driver dit_stub_dai = {
 	.name		= "dit-hifi",
 	.playback 	= {
-		.stream_name	= "Playback",
+		.stream_name	= "spdif-Playback",
 		.channels_min	= 1,
 		.channels_max	= 384,
 		.rates		= STUB_RATES,
diff --git a/sound/soc/kirkwood/kirkwood-spdif.c b/sound/soc/kirkwood/kirkwood-spdif.c
index e5257ec..d1ee8f7 100644
--- a/sound/soc/kirkwood/kirkwood-spdif.c
+++ b/sound/soc/kirkwood/kirkwood-spdif.c
@@ -21,7 +21,7 @@ static struct snd_soc_ops kirkwood_spdif_ops = {
 #include <sound/soc.h>
 
 static const struct snd_soc_dapm_route routes[] = {
-	{ "Playback", NULL, "spdifdo" },
+	{ "spdif-Playback", NULL, "spdifdo" },
 };
 
 static struct snd_soc_dai_link kirkwood_spdif_dai0[] = {
@@ -38,9 +38,18 @@ static struct snd_soc_dai_link kirkwood_spdif_dai0[] = {
 static struct snd_soc_dai_link kirkwood_spdif_dai1[] = {
 	{
 		.name = "S/PDIF1",
-		.stream_name = "IEC958 Playback",
+		.stream_name = "Audio Playback",
 		.platform_name = "mvebu-audio.1",
 		.cpu_dai_name = "mvebu-audio.1",
+		.codec_name = "snd-soc-dummy",
+		.codec_dai_name = "snd-soc-dummy-dai",
+		.dynamic = 1,
+	}, {
+		.name = "Codec",
+		.stream_name = "IEC958 Playback",
+		.cpu_dai_name = "snd-soc-dummy-dai",
+		.platform_name = "snd-soc-dummy",
+		.no_pcm = 1,
 		.codec_dai_name = "dit-hifi",
 		.codec_name = "spdif-dit",
 	},
@@ -70,7 +79,7 @@ static int kirkwood_spdif_probe(struct platform_device *pdev)
 		card->dai_link = kirkwood_spdif_dai0;
 	else
 		card->dai_link = kirkwood_spdif_dai1;
-	card->num_links = 1;
+	card->num_links = 2;
 	card->dapm_routes = routes;
 	card->num_dapm_routes = ARRAY_SIZE(routes);
 	card->dev = &pdev->dev;
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index ccb6be4..381df28 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1634,8 +1634,8 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
 
 	/* there is no point preparing this FE if there are no BEs */
 	if (list_empty(&fe->dpcm[stream].be_clients)) {
-		dev_err(fe->dev, "ASoC: no backend DAIs enabled for %s\n",
-				fe->dai_link->name);
+//		dev_err(fe->dev, "ASoC: no backend DAIs enabled for %s\n",
+//				fe->dai_link->name);
 		ret = -EINVAL;
 		goto out;
 	}

[alsa-devel] [PATCH 00/14] SPDIF support

[Lars-Peter Clausen]

On 09/01/2013 08:42 AM, Russell King - ARM Linux wrote:
> [...]
> 21:07 < rmk> well, someone needs to come up with a fix for these duplicated widgets, and that's not me.
> 21:07 < rmk> because... at the moment... asoc is completely useless for this hardware
> 
> The reason for my last couple of lines there is not that I'm being
> difficult - I really don't know what the correct fix for the duplicated

Well then say it that way. "I don't quite understand how this works yet and
am unable to fix this myself can you help me figure it out". But maybe it is
a cultural thing, who knows.

Lets try to wrap up the situation:

* The hardware has one audio stream, but two DAIs, one for SPDIF one for
I2S. The same audio stream is sent to both DAIs at the same time (It is
possible though to disable one or both of the DAIs).

* This is something new and not supported by classical ASoC.

* DPCM has support for this, but DPCM is still new, unstable,
under-documented and apparently has a couple of bugs.

* With non-DPCM ASoC it is possible to have two DAIs if they are not used at
the same time (which is what I recommend you implement first, before trying
to get DPCM running).

I still don't know if you actually need to feature of being able to output
the same audio signal to both DAIs, do you have such a board? But even then
I still recommend to first get the non-DPCM either/or approach implemented
and once that's working try to get DPCM running. Which probably involves
fixing some of the DPCM issues in the core. As I said sending the same audio
streams to two DAIs is something new and if there was no DPCM yet you'd need
to add support for sending the same stream to multiple DAIs. So either way
you'd have to get your hands dirty. And I'm sure people are willing to help
you figure out the parts you don't understand yet if you ask _nicely_. I
mean I don't come to you either if I have a new ARM SoC that's not supported
yet and demand that you implement support for it and exclaim that the ARM
port sucks because it doesn't support that SoC yet.

- Lars

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Sun, Sep 01, 2013 at 09:42:29AM +0200, Lars-Peter Clausen wrote:
> Lets try to wrap up the situation:
> 
> * The hardware has one audio stream, but two DAIs, one for SPDIF one for
> I2S. The same audio stream is sent to both DAIs at the same time (It is
> possible though to disable one or both of the DAIs).

More or less.  To be more clear: audio DMA commences when either one or
both outputs are enabled, and stops when both are disabled.

This is why either only one can be enabled, or if both are to be enabled,
both enable bits must be set in one register write, and when disabled,
both bits must be cleared in one register write.

So, lets say for argument sake that you wanted to go from disabled to a
single output, then to dual output, back to single output, and finally
back to disabled.  You would need this sequence:

- enable single output
...
- disable single output
- wait for audio unit to indicate not busy
- enable both outputs
...
- disable both outputs
- wait for audio unit to indicate not busy
- enable single output
...
- disable single output
- wait for audio unit to indicate not busy

> * This is something new and not supported by classical ASoC.
> 
> * DPCM has support for this, but DPCM is still new, unstable,
> under-documented and apparently has a couple of bugs.
> 
> * With non-DPCM ASoC it is possible to have two DAIs if they are not used at
> the same time (which is what I recommend you implement first, before trying
> to get DPCM running).

If you'd look at my other responses, you'll see that this is what I tried
back in May, and I was unhappy about that solution because:

1. there is no guarantee that they couldn't be used at the same time.
2. this results in two entirely separate "CPU DAI"s, each with their
   own independent sample rate/format settings, which if they happen
   to be used together will result in fighting over the same register(s).

Moreover, this results in a completely different set of changes to the
driver which are in an opposing direction to the DPCM approach.

> I still don't know if you actually need to feature of being able to output
> the same audio signal to both DAIs, do you have such a board?

This board has the SPDIF connected to a TOSlink and a HDMI transmitter.
It also has the I2S connected only to the HDMI transmitter, though it's
common at the moment to only use the SPDIF output to drive them both.

Having recently changed the TV connected to the HDMI, I find that where
audio used to work at 48kHz and 44.1kHz with the old TV, it no longer
works on anything but 44.1kHz.  The old TV converted everything to
analogue internally in its HDMI receiver before passing it for further
processing.  The new TV is a modern full HD model, so keeps everything in
the digital domain.  I have yet to work out why the TV is muting itself
with 48kHz audio.  However, audio continues to work via the TOSlink output
at both sample rates.

What I'm saying there is that we may need to have another codec in there
so the HDMI transmitter has access to parameters like the sample rate,
or we may have to switch it to using I2S for PCM audio and back to SPDIF
for compressed audio (I'm hoping not because I think that's going to be
an extremely hard problem to solve.)

This is a brand new problem which I've only discovered during last week.
As I have no SPDIF or HDMI analysers, I don't have an answer for this
at the moment, and the only way I can solve it is by verifying the
existing setup (which I believe is correct to the HDMI v1.3 spec) and
experimenting, which will take some time.

However, that's not a reason to hold up these patches - these patches
do work, and allow audio to be used on this platform in at least some
configurations.

> But even then
> I still recommend to first get the non-DPCM either/or approach implemented
> and once that's working try to get DPCM running. Which probably involves
> fixing some of the DPCM issues in the core. As I said sending the same audio
> streams to two DAIs is something new and if there was no DPCM yet you'd need
> to add support for sending the same stream to multiple DAIs. So either way
> you'd have to get your hands dirty.

Could you comment on the patch which creates the two front-end DAIs which
I sent in a different sub-thread - the one which I indicated was from back
in May time.  It isn't quite suitable for submission because the base it
applies to has changed since then, but it should be sufficient to give an
idea of the solution I was trying to implement there.

> And I'm sure people are willing to help
> you figure out the parts you don't understand yet if you ask _nicely_.

Can you then please explain why when I ask for help understanding DAPM
in a "nice" way, the response I get is just "it's just a graph walk"
and no further technical details?

I explained DAPM as I understood it to someone who knows nothing about it
a fortnight ago, and this is how I described DAPM with only a basic
understanding of it, most of that gathered by having been through some of
the code with printk()s to work out some of the problems I was seeing:

| DAPM is a set of "widgets" representing various components of an
| audio system.  The widgets are linked together by a graph.  Each
| widget lives in a context - cpu, platform or codec.  Some bindings
| only happen within a context, others cross contexts (so linking the
| CPU audio stream to the codec for example)

I didn't want to go into the complexities of which widgets are activated
when a stream starts playing, or the special cases with the various
different types of widget that affect the walking and activation of the
graph.

Notice how much more information there - though it wasn't _that_ coherent
(rather than using "linked" it should've been "bound").  The point is,
I've described that there are widgets, there's a binding of widgets
together, the widgets are associated with a context, and finally that
there are restrictions on what bindings can happen.

I've probably missed out quite a bit too without really knowing it -
because I don't know it yet either.  Yes, I do know about the
Documentation/sound/alsa/soc/dapm.txt document.

> I mean I don't come to you either if I have a new ARM SoC that's not
> supported yet and demand that you implement support for it and exclaim
> that the ARM port sucks because it doesn't support that SoC yet.

If you come to me and ask about something in ARM, then you will most
likely get something more than a few words explaining a big chunk of
code - a bit like the oops report I disected last night and provided
full reasoning of the conclusion that I came to (SDRAM failure /
hardware fault on bit 8 of the SDRAM data bus.)

[alsa-devel] [PATCH 00/14] SPDIF support

[Lars-Peter Clausen]

On 09/01/2013 10:51 AM, Russell King - ARM Linux wrote:
> On Sun, Sep 01, 2013 at 09:42:29AM +0200, Lars-Peter Clausen wrote:
>> Lets try to wrap up the situation:
>>
>> * The hardware has one audio stream, but two DAIs, one for SPDIF one for
>> I2S. The same audio stream is sent to both DAIs at the same time (It is
>> possible though to disable one or both of the DAIs).
> 
> More or less.  To be more clear: audio DMA commences when either one or
> both outputs are enabled, and stops when both are disabled.
> 
> This is why either only one can be enabled, or if both are to be enabled,
> both enable bits must be set in one register write, and when disabled,
> both bits must be cleared in one register write.
> 
> So, lets say for argument sake that you wanted to go from disabled to a
> single output, then to dual output, back to single output, and finally
> back to disabled.  You would need this sequence:
> 
> - enable single output
> ...
> - disable single output
> - wait for audio unit to indicate not busy
> - enable both outputs
> ...
> - disable both outputs
> - wait for audio unit to indicate not busy
> - enable single output
> ...
> - disable single output
> - wait for audio unit to indicate not busy
> 
>> * This is something new and not supported by classical ASoC.
>>
>> * DPCM has support for this, but DPCM is still new, unstable,
>> under-documented and apparently has a couple of bugs.
>>
>> * With non-DPCM ASoC it is possible to have two DAIs if they are not used at
>> the same time (which is what I recommend you implement first, before trying
>> to get DPCM running).
> 
> If you'd look at my other responses, you'll see that this is what I tried
> back in May, and I was unhappy about that solution because:
> 
> 1. there is no guarantee that they couldn't be used at the same time.
> 2. this results in two entirely separate "CPU DAI"s, each with their
>    own independent sample rate/format settings, which if they happen
>    to be used together will result in fighting over the same register(s).

I know, but you can make it policy that only one of them may be used at a
time. Furthermore you can add a check to the startup() callback to return an
error, if the other DAI is active.

>
> Moreover, this results in a completely different set of changes to the
> driver which are in an opposing direction to the DPCM approach.
> 

I think the patch is actually going to be maybe a 100 lines or so and it
gives you something to work with and unlike your current approach is not
trying to work around the framework. Then you can add the other patches
adding the SPDIF controls on top of it. Once that's done you can concentrate
on trying to get DPCM running.

>> I still don't know if you actually need to feature of being able to output
>> the same audio signal to both DAIs, do you have such a board?
> 
> This board has the SPDIF connected to a TOSlink and a HDMI transmitter.
> It also has the I2S connected only to the HDMI transmitter, though it's
> common at the moment to only use the SPDIF output to drive them both.

Ok, so things will work fine with the either/or approach for now.

[...]
>> But even then
>> I still recommend to first get the non-DPCM either/or approach implemented
>> and once that's working try to get DPCM running. Which probably involves
>> fixing some of the DPCM issues in the core. As I said sending the same audio
>> streams to two DAIs is something new and if there was no DPCM yet you'd need
>> to add support for sending the same stream to multiple DAIs. So either way
>> you'd have to get your hands dirty.
> 
> Could you comment on the patch which creates the two front-end DAIs which
> I sent in a different sub-thread - the one which I indicated was from back
> in May time.  It isn't quite suitable for submission because the base it
> applies to has changed since then, but it should be sufficient to give an
> idea of the solution I was trying to implement there.

The patch looks OK, except for maybe the way the names for the DAIs are
created. That probably something that's better handled in the core in a
generic way.

- Lars

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Sun, Sep 01, 2013 at 12:08:37PM +0200, Lars-Peter Clausen wrote:
> On 09/01/2013 10:51 AM, Russell King - ARM Linux wrote:
> > If you'd look at my other responses, you'll see that this is what I tried
> > back in May, and I was unhappy about that solution because:
> > 
> > 1. there is no guarantee that they couldn't be used at the same time.
> > 2. this results in two entirely separate "CPU DAI"s, each with their
> >    own independent sample rate/format settings, which if they happen
> >    to be used together will result in fighting over the same register(s).
> 
> I know, but you can make it policy that only one of them may be used at a
> time. Furthermore you can add a check to the startup() callback to return an
> error, if the other DAI is active.

Okay, but can you confirm a few things:

1. It is my understanding that the startup() callback will be called when
   the ALSA control device is opened (/dev/snd/controlC*).

2. With two CPU DAIs, won't this cause there to be two ALSA PCM and ALSA
   control devices if they are both bound?  IOW, /dev/snd/controlC0 and
   /dev/snd/controlC1 ?  It is my understanding that one ALSA PCM
   instance is registered for each CPU DAI which is "linked up" via a
   DAI link.

If that is so, then it's not going to behave well with pulseaudio should
both end up being bound - with that running, pulseaudio will try to open
every /dev/snd/controlC* file that it finds, and any errors that it
finds, it just retries as fast as it possibly can.

> > Moreover, this results in a completely different set of changes to the
> > driver which are in an opposing direction to the DPCM approach.
> > 
> 
> I think the patch is actually going to be maybe a 100 lines or so and it
> gives you something to work with and unlike your current approach is not
> trying to work around the framework. Then you can add the other patches
> adding the SPDIF controls on top of it. Once that's done you can concentrate
> on trying to get DPCM running.

Yes, which will mean effectively reverting the diff below - it is my
understanding that this set plus the split in the DAI links is the full
DPCM configuration required from the driver layers of ASoC.

However, I'm much less certain that the result of applying this delta
is correct for existing platforms - particularly the CPU DAI names in
their DAI link structures.  As I don't have any of those platforms,
that's something I can't test.

In my opinion, this is a large delta to put in and then have to take
back out again when the problems with DPCM get resolved.  Linus'
objections over pointless churn which have been levelled at the ARM
community come to mind.

 sound/soc/kirkwood/kirkwood-i2s.c    |  318 ++++++++++++++++------------------
 sound/soc/kirkwood/kirkwood-openrd.c |   10 +-
 sound/soc/kirkwood/kirkwood-t5325.c  |    6 +-
 sound/soc/kirkwood/kirkwood.h        |    5 +-
 4 files changed, 157 insertions(+), 182 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 11f51e0..6a441cc 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -211,19 +211,44 @@ static int kirkwood_i2s_startup(struct snd_pcm_substream *substream,
 {
 	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai);
 
+	if (priv->active_dai && priv->active_dai != dai)
+		return -EBUSY;
+	priv->active_dai = dai;
+
 	snd_soc_dai_set_dma_data(dai, substream, priv);
 	return 0;
 }
 
+static void kirkwood_i2s_shutdown(struct snd_pcm_substream *substream,
+				  struct snd_soc_dai *dai)
+{
+	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai);
+
+	if (priv->active_dai == dai)
+		priv->active_dai = NULL;
+}
+
 static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
 				 struct snd_pcm_hw_params *params,
 				 struct snd_soc_dai *dai)
 {
 	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai);
-	uint32_t ctl_play, ctl_rec;
+	uint32_t ctl_play, ctl_rec, ctl_play_mask, ctl_rec_mask;
 	unsigned int i2s_reg;
 	unsigned long i2s_value;
 
+	/*
+	 * Select the playback/record enable masks according to which
+	 * DAI is being used.
+	 */
+	if (dai->id == 0) {
+		ctl_play_mask = KIRKWOOD_PLAYCTL_I2S_EN;
+		ctl_rec_mask = KIRKWOOD_RECCTL_I2S_EN;
+	} else {
+		ctl_play_mask = KIRKWOOD_PLAYCTL_SPDIF_EN;
+		ctl_rec_mask = KIRKWOOD_RECCTL_SPDIF_EN;
+	}
+
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		i2s_reg = KIRKWOOD_I2S_PLAYCTL;
 	} else {
@@ -242,38 +267,27 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
 	switch (params_format(params)) {
 	case SNDRV_PCM_FORMAT_S16_LE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_16;
-		ctl_play = KIRKWOOD_PLAYCTL_SIZE_16_C |
-			   KIRKWOOD_PLAYCTL_I2S_EN |
-			   KIRKWOOD_PLAYCTL_SPDIF_EN;
-		ctl_rec = KIRKWOOD_RECCTL_SIZE_16_C |
-			  KIRKWOOD_RECCTL_I2S_EN;
+		ctl_play = KIRKWOOD_PLAYCTL_SIZE_16_C | ctl_play_mask;
+		ctl_rec = KIRKWOOD_RECCTL_SIZE_16_C | ctl_rec_mask;
 		break;
 	/*
 	 * doesn't work... S20_3LE != kirkwood 20bit format ?
 	 *
 	case SNDRV_PCM_FORMAT_S20_3LE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_20;
-		ctl_play = KIRKWOOD_PLAYCTL_SIZE_20 |
-			   KIRKWOOD_PLAYCTL_I2S_EN |
-			   KIRKWOOD_PLAYCTL_SPDIF_EN;
-		ctl_rec = KIRKWOOD_RECCTL_SIZE_20 |
-			  KIRKWOOD_RECCTL_I2S_EN;
+		ctl_play = KIRKWOOD_PLAYCTL_SIZE_20 | ctl_play_mask;
+		ctl_rec = KIRKWOOD_RECCTL_SIZE_20 | ctl_rec_mask;
 		break;
 	*/
 	case SNDRV_PCM_FORMAT_S24_LE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_24;
-		ctl_play = KIRKWOOD_PLAYCTL_SIZE_24 |
-			   KIRKWOOD_PLAYCTL_I2S_EN |
-			   KIRKWOOD_PLAYCTL_SPDIF_EN;
-		ctl_rec = KIRKWOOD_RECCTL_SIZE_24 |
-			  KIRKWOOD_RECCTL_I2S_EN;
+		ctl_play = KIRKWOOD_PLAYCTL_SIZE_24 | ctl_play_mask;
+		ctl_rec = KIRKWOOD_RECCTL_SIZE_24 | ctl_rec_mask;
 		break;
 	case SNDRV_PCM_FORMAT_S32_LE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_32;
-		ctl_play = KIRKWOOD_PLAYCTL_SIZE_32 |
-			   KIRKWOOD_PLAYCTL_I2S_EN;
-		ctl_rec = KIRKWOOD_RECCTL_SIZE_32 |
-			  KIRKWOOD_RECCTL_I2S_EN;
+		ctl_play = KIRKWOOD_PLAYCTL_SIZE_32 | ctl_play_mask;
+		ctl_rec = KIRKWOOD_RECCTL_SIZE_32 | ctl_rec_mask;
 		break;
 	default:
 		return -EINVAL;
@@ -286,11 +300,12 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
 			ctl_play |= KIRKWOOD_PLAYCTL_MONO_OFF;
 
 		priv->ctl_play &= ~(KIRKWOOD_PLAYCTL_MONO_MASK |
-				    KIRKWOOD_PLAYCTL_ENABLE_MASK |
+				    ctl_play_mask |
 				    KIRKWOOD_PLAYCTL_SIZE_MASK);
 		priv->ctl_play |= ctl_play;
 	} else {
-		priv->ctl_rec &= ~KIRKWOOD_RECCTL_SIZE_MASK;
+		priv->ctl_rec &= ~(KIRKWOOD_RECCTL_SIZE_MASK |
+				   ctl_rec_mask);
 		priv->ctl_rec |= ctl_rec;
 	}
 
@@ -303,7 +318,13 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
 				int cmd, struct snd_soc_dai *dai)
 {
 	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai);
-	uint32_t ctl, value;
+	uint32_t ctl, value, mute_mask;
+
+	if (dai->id == 0) {
+		mute_mask = KIRKWOOD_PLAYCTL_I2S_MUTE;
+	} else {
+		mute_mask = KIRKWOOD_PLAYCTL_SPDIF_MUTE;
+	}
 
 	ctl = readl(priv->io + KIRKWOOD_PLAYCTL);
 	if (ctl & KIRKWOOD_PLAYCTL_PAUSE) {
@@ -329,7 +350,7 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 		/* configure */
-		ctl = priv->ctl_play & priv->ctl_play_mask;
+		ctl = priv->ctl_play;
 		value = ctl & ~KIRKWOOD_PLAYCTL_ENABLE_MASK;
 		writel(value, priv->io + KIRKWOOD_PLAYCTL);
 
@@ -344,7 +365,7 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
 
 	case SNDRV_PCM_TRIGGER_STOP:
 		/* stop audio, disable interrupts */
-		ctl |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE;
+		ctl |= KIRKWOOD_PLAYCTL_PAUSE | mute_mask;
 		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
 
 		value = readl(priv->io + KIRKWOOD_INT_MASK);
@@ -358,13 +379,13 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
 
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
-		ctl |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE;
+		ctl |= KIRKWOOD_PLAYCTL_PAUSE | mute_mask;
 		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
 		break;
 
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		ctl &= ~(KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE);
+		ctl &= ~(KIRKWOOD_PLAYCTL_PAUSE | mute_mask);
 		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
 		break;
 
@@ -447,151 +468,56 @@ static int kirkwood_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 	return 0;
 }
 
-static int kirkwood_i2s_play_i2s(struct snd_soc_dapm_widget *w,
-	struct snd_kcontrol *ctl, int event)
-{
-	/* CPU DAI is not available, so use driver data from device */
-	struct kirkwood_dma_data *priv = dev_get_drvdata(w->dapm->dev);
-
-	if (SND_SOC_DAPM_EVENT_ON(event))
-		priv->ctl_play_mask |= KIRKWOOD_PLAYCTL_I2S_EN;
-	else
-		priv->ctl_play_mask &= ~KIRKWOOD_PLAYCTL_I2S_EN;
-
-	return 0;
-}
-
-static int kirkwood_i2s_play_spdif(struct snd_soc_dapm_widget *w,
-	struct snd_kcontrol *ctl, int event)
-{
-	/* CPU DAI is not available, so use driver data from device */
-	struct kirkwood_dma_data *priv = dev_get_drvdata(w->dapm->dev);
-
-	if (SND_SOC_DAPM_EVENT_ON(event))
-		priv->ctl_play_mask |= KIRKWOOD_PLAYCTL_SPDIF_EN;
-	else
-		priv->ctl_play_mask &= ~KIRKWOOD_PLAYCTL_SPDIF_EN;
-
-	return 0;
-}
-
-static const struct snd_soc_dapm_widget widgets[] = {
-	/* These widget names come from the names from the functional spec */
-	SND_SOC_DAPM_AIF_OUT_E("i2sdo", "dma-tx",
-		0, SND_SOC_NOPM, 0, 0, kirkwood_i2s_play_i2s,
-		SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
-	SND_SOC_DAPM_AIF_OUT_E("spdifdo", "dma-tx",
-		0, SND_SOC_NOPM, 0, 0, kirkwood_i2s_play_spdif,
-		SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
-	SND_SOC_DAPM_AIF_IN("i2sdi", "dma-rx",
-		0, SND_SOC_NOPM, 0, 0),
-};
-
-static int kirkwood_i2s_probe(struct snd_soc_dai *dai)
-{
-	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai);
-	unsigned long value;
-	unsigned int reg_data;
-	int ret;
-
-	ret = snd_soc_add_dai_controls(dai, kirkwood_i2s_iec958_controls,
-				ARRAY_SIZE(kirkwood_i2s_iec958_controls));
-	if (ret) {
-		dev_err(dai->dev,
-			"unable to add soc card controls: %d\n", ret);
-		return ret;
-	}
-
-	/* It appears that these can't be attached to the CPU DAI */
-	snd_soc_dapm_new_controls(&dai->dapm, widgets, ARRAY_SIZE(widgets));
-
-	/* put system in a "safe" state : */
-	/* disable audio interrupts */
-	writel(0xffffffff, priv->io + KIRKWOOD_INT_CAUSE);
-	writel(0, priv->io + KIRKWOOD_INT_MASK);
-
-	reg_data = readl(priv->io + 0x1200);
-	reg_data &= (~(0x333FF8));
-	reg_data |= 0x111D18;
-	writel(reg_data, priv->io + 0x1200);
-
-	msleep(500);
-
-	reg_data = readl(priv->io + 0x1200);
-	reg_data &= (~(0x333FF8));
-	reg_data |= 0x111D18;
-	writel(reg_data, priv->io + 0x1200);
-
-	priv->ctl_play_mask = ~KIRKWOOD_PLAYCTL_ENABLE_MASK;
-
-	/* disable playback/record */
-	value = readl(priv->io + KIRKWOOD_PLAYCTL);
-	value &= ~KIRKWOOD_PLAYCTL_ENABLE_MASK;
-	writel(value, priv->io + KIRKWOOD_PLAYCTL);
-
-	value = readl(priv->io + KIRKWOOD_RECCTL);
-	value &= ~(KIRKWOOD_RECCTL_I2S_EN | KIRKWOOD_RECCTL_SPDIF_EN);
-	writel(value, priv->io + KIRKWOOD_RECCTL);
-
-	return 0;
-
-}
-
-static int kirkwood_i2s_remove(struct snd_soc_dai *dai)
-{
-	return 0;
-}
-
 static const struct snd_soc_dai_ops kirkwood_i2s_dai_ops = {
 	.startup	= kirkwood_i2s_startup,
+	.shutdown	= kirkwood_i2s_shutdown,
 	.trigger	= kirkwood_i2s_trigger,
 	.hw_params      = kirkwood_i2s_hw_params,
 	.set_fmt        = kirkwood_i2s_set_fmt,
 };
 
+static int kirkwood_spdif_probe(struct snd_soc_dai *dai)
+{
+	int ret = snd_soc_add_dai_controls(dai, kirkwood_i2s_iec958_controls,
+				ARRAY_SIZE(kirkwood_i2s_iec958_controls));
+	if (ret)
+		dev_err(dai->dev, "unable to add soc card controls\n");
 
-static struct snd_soc_dai_driver kirkwood_i2s_dai = {
-	.probe = kirkwood_i2s_probe,
-	.remove = kirkwood_i2s_remove,
-	.playback = {
-		.stream_name = "dma-tx",
-		.channels_min = 1,
-		.channels_max = 2,
-		.rates = KIRKWOOD_I2S_RATES,
-		.formats = KIRKWOOD_I2S_FORMATS,
-	},
-	.capture = {
-		.stream_name = "dma-rx",
-		.channels_min = 1,
-		.channels_max = 2,
-		.rates = KIRKWOOD_I2S_RATES,
-		.formats = KIRKWOOD_I2S_FORMATS,
-	},
-	.ops = &kirkwood_i2s_dai_ops,
-};
+	return ret;
+}
 
-static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk = {
-	.probe = kirkwood_i2s_probe,
-	.remove = kirkwood_i2s_remove,
-	.playback = {
-		.stream_name = "dma-tx",
-		.channels_min = 1,
-		.channels_max = 2,
-		.rates = SNDRV_PCM_RATE_8000_192000 |
-			 SNDRV_PCM_RATE_CONTINUOUS |
-			 SNDRV_PCM_RATE_KNOT,
-		.formats = KIRKWOOD_I2S_FORMATS,
-	},
-	.capture = {
-		.stream_name = "dma-rx",
-		.channels_min = 1,
-		.channels_max = 2,
-		.rates = SNDRV_PCM_RATE_8000_192000 |
-			 SNDRV_PCM_RATE_CONTINUOUS |
-			 SNDRV_PCM_RATE_KNOT,
-		.formats = KIRKWOOD_I2S_FORMATS,
+static struct snd_soc_dai_driver kirkwood_dais[KIRKWOOD_NUM_DAIS] = {
+	{
+		.name = "kirkwood-i2s.%d",
+		.ops = &kirkwood_i2s_dai_ops,
+		.playback = {
+			.channels_min = 1,
+			.channels_max = 2,
+			.rates = KIRKWOOD_I2S_RATES,
+			.formats = KIRKWOOD_I2S_FORMATS,
+		},
+		.capture = {
+			.channels_min = 1,
+			.channels_max = 2,
+			.rates = KIRKWOOD_I2S_RATES,
+			.formats = KIRKWOOD_I2S_FORMATS,
+		},
+		.symmetric_rates = 1,
+	}, {
+		.name = "kirkwood-spdif.%d",
+		.probe = kirkwood_spdif_probe,
+		.ops = &kirkwood_i2s_dai_ops,
+		.playback = {
+			.channels_min = 1,
+			.channels_max = 2,
+			.rates = KIRKWOOD_I2S_RATES,
+			.formats = SNDRV_PCM_FMTBIT_S16_LE |
+				   SNDRV_PCM_FMTBIT_S24_LE |
+				   SNDRV_PCM_FMTBIT_S32_LE |
+				   SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE |
+				   SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE,
+		},
 	},
-	.ops = &kirkwood_i2s_dai_ops,
 };
 
 static const struct snd_soc_component_driver kirkwood_i2s_component = {
@@ -601,10 +527,10 @@ static const struct snd_soc_component_driver kirkwood_i2s_component = {
 static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 {
 	struct kirkwood_asoc_platform_data *data = pdev->dev.platform_data;
-	struct snd_soc_dai_driver *soc_dai = &kirkwood_i2s_dai;
 	struct kirkwood_dma_data *priv;
 	struct resource *mem;
-	int err;
+	int i, err;
+	u32 val;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
@@ -613,6 +539,24 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 	}
 	dev_set_drvdata(&pdev->dev, priv);
 
+	memcpy(priv->dai_driver, kirkwood_dais, sizeof(priv->dai_driver));
+
+	/* format the DAI names according to the platform device ID */
+	for (i = 0; i < KIRKWOOD_NUM_DAIS; i++) {
+		const char *fmt = priv->dai_driver[i].name;
+		char *name, *p;
+
+		if (pdev->id < 0) {
+			name = kstrdup(fmt, GFP_KERNEL);
+			p = strchr(name, '.');
+			if (p)
+				*p = '\0';
+		} else {
+			name = kasprintf(GFP_KERNEL, fmt, pdev->id);
+		}
+		priv->dai_driver[i].name = name;
+	}
+
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	priv->io = devm_ioremap_resource(&pdev->dev, mem);
 	if (IS_ERR(priv->io))
@@ -647,9 +591,23 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 			devm_clk_put(&pdev->dev, priv->extclk);
 			priv->extclk = ERR_PTR(-EINVAL);
 		} else {
+			int i;
+
 			dev_info(&pdev->dev, "found external clock\n");
 			clk_prepare_enable(priv->extclk);
-			soc_dai = &kirkwood_i2s_dai_extclk;
+
+			for (i = 0; i < KIRKWOOD_NUM_DAIS; i++) {
+				if (priv->dai_driver[i].playback.rates)
+					priv->dai_driver[i].playback.rates =
+						SNDRV_PCM_RATE_8000_192000 |
+						SNDRV_PCM_RATE_CONTINUOUS |
+						SNDRV_PCM_RATE_KNOT;
+				if (priv->dai_driver[i].capture.rates)
+					priv->dai_driver[i].capture.rates =
+						SNDRV_PCM_RATE_8000_192000 |
+						SNDRV_PCM_RATE_CONTINUOUS |
+						SNDRV_PCM_RATE_KNOT;
+			}
 		}
 	}
 
@@ -666,8 +624,35 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 		priv->ctl_rec |= KIRKWOOD_RECCTL_BURST_128;
 	}
 
+	/* put system in a "safe" state : disable audio interrupts */
+	writel(0xffffffff, priv->io + KIRKWOOD_INT_CAUSE);
+	writel(0, priv->io + KIRKWOOD_INT_MASK);
+
+	val = readl(priv->io + 0x120c);
+	val = readl(priv->io + 0x1200);
+	val &= (~(0x333FF8));
+	val |= 0x111D18;
+	writel(val, priv->io + 0x1200);
+
+	msleep(500);
+
+	val = readl(priv->io + 0x1200);
+	val &= (~(0x333FF8));
+	val |= 0x111D18;
+	msleep(500);
+	writel(val, priv->io + 0x1200);
+
+	/* disable playback/record */
+	val = readl(priv->io + KIRKWOOD_PLAYCTL);
+	val &= ~(KIRKWOOD_PLAYCTL_I2S_EN|KIRKWOOD_PLAYCTL_SPDIF_EN);
+	writel(val, priv->io + KIRKWOOD_PLAYCTL);
+
+	val = readl(priv->io + KIRKWOOD_RECCTL);
+	val &= ~(KIRKWOOD_RECCTL_I2S_EN | KIRKWOOD_RECCTL_SPDIF_EN);
+	writel(val, priv->io + KIRKWOOD_RECCTL);
+
 	err = snd_soc_register_component(&pdev->dev, &kirkwood_i2s_component,
-					 soc_dai, 1);
+					 priv->dai_driver, KIRKWOOD_NUM_DAIS);
 	if (err) {
 		dev_err(&pdev->dev, "snd_soc_register_component failed\n");
 		goto err_component;
@@ -679,7 +664,6 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 		goto err_platform;
 	}
 	return 0;
-
  err_platform:
 	snd_soc_unregister_component(&pdev->dev);
  err_component:
diff --git a/sound/soc/kirkwood/kirkwood-openrd.c b/sound/soc/kirkwood/kirkwood-openrd.c
index 258fcce..c92f42a 100644
--- a/sound/soc/kirkwood/kirkwood-openrd.c
+++ b/sound/soc/kirkwood/kirkwood-openrd.c
@@ -50,12 +50,11 @@ static struct snd_soc_ops openrd_client_ops = {
 };
 
 
-/* This will need to be updated when DPCM support works. */
 static struct snd_soc_dai_link openrd_client_dai[] = {
 {
 	.name = "CS42L51",
 	.stream_name = "CS42L51 HiFi",
-	.cpu_dai_name = "mvebu-audio",
+	.cpu_dai_name = "kirkwood-i2s.0",
 	.platform_name = "mvebu-audio",
 	.codec_dai_name = "cs42l51-hifi",
 	.codec_name = "cs42l51-codec.0-004a",
@@ -64,19 +63,12 @@ static struct snd_soc_dai_link openrd_client_dai[] = {
 },
 };
 
-static const struct snd_soc_dapm_route routes[] = {
-	/* Connect the codec streams to the I2S connections */
-	{ "Playback", NULL, "i2sdo" },
-	{ "i2sdi", NULL, "Capture" },
-};
 
 static struct snd_soc_card openrd_client = {
 	.name = "OpenRD Client",
 	.owner = THIS_MODULE,
 	.dai_link = openrd_client_dai,
 	.num_links = ARRAY_SIZE(openrd_client_dai),
-	.dapm_routes = routes,
-	.num_dapm_routes = ARRAY_SIZE(routes),
 };
 
 static int openrd_probe(struct platform_device *pdev)
diff --git a/sound/soc/kirkwood/kirkwood-t5325.c b/sound/soc/kirkwood/kirkwood-t5325.c
index 5f5ae08..02911f7 100644
--- a/sound/soc/kirkwood/kirkwood-t5325.c
+++ b/sound/soc/kirkwood/kirkwood-t5325.c
@@ -52,9 +52,6 @@ static const struct snd_soc_dapm_route t5325_route[] = {
 
 	{ "MIC1",		NULL,	"Mic Jack" },
 	{ "MIC2",		NULL,	"Mic Jack" },
-
-	{ "i2sdi",		NULL,	"Capture" },
-	{ "Playback",		NULL,	"i2sdo" },
 };
 
 static int t5325_dai_init(struct snd_soc_pcm_runtime *rtd)
@@ -69,12 +66,11 @@ static int t5325_dai_init(struct snd_soc_pcm_runtime *rtd)
 	return 0;
 }
 
-/* This will need to be updated when DPCM support works. */
 static struct snd_soc_dai_link t5325_dai[] = {
 {
 	.name = "ALC5621",
 	.stream_name = "ALC5621 HiFi",
-	.cpu_dai_name = "mvebu-audio",
+	.cpu_dai_name = "kirkwood-i2s.0",
 	.platform_name = "mvebu-audio",
 	.codec_dai_name = "alc5621-hifi",
 	.codec_name = "alc562x-codec.0-001a",
diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h
index 17b48a6..595e0fe 100644
--- a/sound/soc/kirkwood/kirkwood.h
+++ b/sound/soc/kirkwood/kirkwood.h
@@ -161,13 +161,16 @@
 #define KIRKWOOD_SND_MAX_BUFFER_BYTES		(KIRKWOOD_SND_MAX_PERIOD_BYTES \
 						 * KIRKWOOD_SND_MAX_PERIODS)
 
+#define KIRKWOOD_NUM_DAIS 2
+
 struct kirkwood_dma_data {
 	void __iomem *io;
 	struct clk *clk;
 	struct clk *extclk;
-	uint32_t ctl_play_mask;
 	uint32_t ctl_play;
 	uint32_t ctl_rec;
+	struct snd_soc_dai *active_dai;
+	struct snd_soc_dai_driver dai_driver[KIRKWOOD_NUM_DAIS];
 	struct snd_pcm_substream *substream_play;
 	struct snd_pcm_substream *substream_rec;
 	int irq;

[alsa-devel] [PATCH 00/14] SPDIF support

[Lars-Peter Clausen]

On 09/01/2013 02:04 PM, Russell King - ARM Linux wrote:
> On Sun, Sep 01, 2013 at 12:08:37PM +0200, Lars-Peter Clausen wrote:
>> On 09/01/2013 10:51 AM, Russell King - ARM Linux wrote:
>>> If you'd look at my other responses, you'll see that this is what I tried
>>> back in May, and I was unhappy about that solution because:
>>>
>>> 1. there is no guarantee that they couldn't be used at the same time.
>>> 2. this results in two entirely separate "CPU DAI"s, each with their
>>>    own independent sample rate/format settings, which if they happen
>>>    to be used together will result in fighting over the same register(s).
>>
>> I know, but you can make it policy that only one of them may be used at a
>> time. Furthermore you can add a check to the startup() callback to return an
>> error, if the other DAI is active.
> 
> Okay, but can you confirm a few things:
> 
> 1. It is my understanding that the startup() callback will be called when
>    the ALSA control device is opened (/dev/snd/controlC*).

Yes.

> 
> 2. With two CPU DAIs, won't this cause there to be two ALSA PCM and ALSA
>    control devices if they are both bound?  IOW, /dev/snd/controlC0 and
>    /dev/snd/controlC1 ?  It is my understanding that one ALSA PCM
>    instance is registered for each CPU DAI which is "linked up" via a
>    DAI link.
> 

Yes, but only if you create a link for both the I2S and and the SPDIF DAI.
And as I understood you you don't necessarily need to do that and it is more
of a nice to have.

> If that is so, then it's not going to behave well with pulseaudio should
> both end up being bound - with that running, pulseaudio will try to open
> every /dev/snd/controlC* file that it finds, and any errors that it
> finds, it just retries as fast as it possibly can.
> 
>>> Moreover, this results in a completely different set of changes to the
>>> driver which are in an opposing direction to the DPCM approach.
>>>
>>
>> I think the patch is actually going to be maybe a 100 lines or so and it
>> gives you something to work with and unlike your current approach is not
>> trying to work around the framework. Then you can add the other patches
>> adding the SPDIF controls on top of it. Once that's done you can concentrate
>> on trying to get DPCM running.
> 
> Yes, which will mean effectively reverting the diff below - it is my
> understanding that this set plus the split in the DAI links is the full
> DPCM configuration required from the driver layers of ASoC.
> 
> However, I'm much less certain that the result of applying this delta
> is correct for existing platforms - particularly the CPU DAI names in
> their DAI link structures.  As I don't have any of those platforms,
> that's something I can't test.

Instead of that custom name generation you implemented in your patch the
name of the DAIs should always be the same and not depend on the platform
device id. Then in the link config use both cpu_name and cpu_dai_name.

E.g.

{
	...
	.cpu_name = "mvebu-audio.0"
	.cpu_dai_name = "mvebu-audio-spdif",
	...
}

cpu_name is the dev_name() of the device and cpu_dai_name the name of the DAI.

And well for devicetree phandles should be used.

> 
> In my opinion, this is a large delta to put in and then have to take
> back out again when the problems with DPCM get resolved.  Linus'
> objections over pointless churn which have been levelled at the ARM
> community come to mind.

I wouldn't worry about that, the patch below is against your working branch,
the patch should be a lot smaller against the ASoC tree.

- Lars

> 
>  sound/soc/kirkwood/kirkwood-i2s.c    |  318 ++++++++++++++++------------------
>  sound/soc/kirkwood/kirkwood-openrd.c |   10 +-
>  sound/soc/kirkwood/kirkwood-t5325.c  |    6 +-
>  sound/soc/kirkwood/kirkwood.h        |    5 +-
>  4 files changed, 157 insertions(+), 182 deletions(-)
> 
> diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
> index 11f51e0..6a441cc 100644
> --- a/sound/soc/kirkwood/kirkwood-i2s.c
> +++ b/sound/soc/kirkwood/kirkwood-i2s.c
> @@ -211,19 +211,44 @@ static int kirkwood_i2s_startup(struct snd_pcm_substream *substream,
>  {
>  	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai);
>  
> +	if (priv->active_dai && priv->active_dai != dai)
> +		return -EBUSY;
> +	priv->active_dai = dai;
> +
>  	snd_soc_dai_set_dma_data(dai, substream, priv);
>  	return 0;
>  }
>  
> +static void kirkwood_i2s_shutdown(struct snd_pcm_substream *substream,
> +				  struct snd_soc_dai *dai)
> +{
> +	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai);
> +
> +	if (priv->active_dai == dai)
> +		priv->active_dai = NULL;
> +}
> +
>  static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
>  				 struct snd_pcm_hw_params *params,
>  				 struct snd_soc_dai *dai)
>  {
>  	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai);
> -	uint32_t ctl_play, ctl_rec;
> +	uint32_t ctl_play, ctl_rec, ctl_play_mask, ctl_rec_mask;
>  	unsigned int i2s_reg;
>  	unsigned long i2s_value;
>  
> +	/*
> +	 * Select the playback/record enable masks according to which
> +	 * DAI is being used.
> +	 */
> +	if (dai->id == 0) {
> +		ctl_play_mask = KIRKWOOD_PLAYCTL_I2S_EN;
> +		ctl_rec_mask = KIRKWOOD_RECCTL_I2S_EN;
> +	} else {
> +		ctl_play_mask = KIRKWOOD_PLAYCTL_SPDIF_EN;
> +		ctl_rec_mask = KIRKWOOD_RECCTL_SPDIF_EN;
> +	}
> +
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>  		i2s_reg = KIRKWOOD_I2S_PLAYCTL;
>  	} else {
> @@ -242,38 +267,27 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
>  	switch (params_format(params)) {
>  	case SNDRV_PCM_FORMAT_S16_LE:
>  		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_16;
> -		ctl_play = KIRKWOOD_PLAYCTL_SIZE_16_C |
> -			   KIRKWOOD_PLAYCTL_I2S_EN |
> -			   KIRKWOOD_PLAYCTL_SPDIF_EN;
> -		ctl_rec = KIRKWOOD_RECCTL_SIZE_16_C |
> -			  KIRKWOOD_RECCTL_I2S_EN;
> +		ctl_play = KIRKWOOD_PLAYCTL_SIZE_16_C | ctl_play_mask;
> +		ctl_rec = KIRKWOOD_RECCTL_SIZE_16_C | ctl_rec_mask;
>  		break;
>  	/*
>  	 * doesn't work... S20_3LE != kirkwood 20bit format ?
>  	 *
>  	case SNDRV_PCM_FORMAT_S20_3LE:
>  		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_20;
> -		ctl_play = KIRKWOOD_PLAYCTL_SIZE_20 |
> -			   KIRKWOOD_PLAYCTL_I2S_EN |
> -			   KIRKWOOD_PLAYCTL_SPDIF_EN;
> -		ctl_rec = KIRKWOOD_RECCTL_SIZE_20 |
> -			  KIRKWOOD_RECCTL_I2S_EN;
> +		ctl_play = KIRKWOOD_PLAYCTL_SIZE_20 | ctl_play_mask;
> +		ctl_rec = KIRKWOOD_RECCTL_SIZE_20 | ctl_rec_mask;
>  		break;
>  	*/
>  	case SNDRV_PCM_FORMAT_S24_LE:
>  		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_24;
> -		ctl_play = KIRKWOOD_PLAYCTL_SIZE_24 |
> -			   KIRKWOOD_PLAYCTL_I2S_EN |
> -			   KIRKWOOD_PLAYCTL_SPDIF_EN;
> -		ctl_rec = KIRKWOOD_RECCTL_SIZE_24 |
> -			  KIRKWOOD_RECCTL_I2S_EN;
> +		ctl_play = KIRKWOOD_PLAYCTL_SIZE_24 | ctl_play_mask;
> +		ctl_rec = KIRKWOOD_RECCTL_SIZE_24 | ctl_rec_mask;
>  		break;
>  	case SNDRV_PCM_FORMAT_S32_LE:
>  		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_32;
> -		ctl_play = KIRKWOOD_PLAYCTL_SIZE_32 |
> -			   KIRKWOOD_PLAYCTL_I2S_EN;
> -		ctl_rec = KIRKWOOD_RECCTL_SIZE_32 |
> -			  KIRKWOOD_RECCTL_I2S_EN;
> +		ctl_play = KIRKWOOD_PLAYCTL_SIZE_32 | ctl_play_mask;
> +		ctl_rec = KIRKWOOD_RECCTL_SIZE_32 | ctl_rec_mask;
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -286,11 +300,12 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
>  			ctl_play |= KIRKWOOD_PLAYCTL_MONO_OFF;
>  
>  		priv->ctl_play &= ~(KIRKWOOD_PLAYCTL_MONO_MASK |
> -				    KIRKWOOD_PLAYCTL_ENABLE_MASK |
> +				    ctl_play_mask |
>  				    KIRKWOOD_PLAYCTL_SIZE_MASK);
>  		priv->ctl_play |= ctl_play;
>  	} else {
> -		priv->ctl_rec &= ~KIRKWOOD_RECCTL_SIZE_MASK;
> +		priv->ctl_rec &= ~(KIRKWOOD_RECCTL_SIZE_MASK |
> +				   ctl_rec_mask);
>  		priv->ctl_rec |= ctl_rec;
>  	}
>  
> @@ -303,7 +318,13 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
>  				int cmd, struct snd_soc_dai *dai)
>  {
>  	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai);
> -	uint32_t ctl, value;
> +	uint32_t ctl, value, mute_mask;
> +
> +	if (dai->id == 0) {
> +		mute_mask = KIRKWOOD_PLAYCTL_I2S_MUTE;
> +	} else {
> +		mute_mask = KIRKWOOD_PLAYCTL_SPDIF_MUTE;
> +	}
>  
>  	ctl = readl(priv->io + KIRKWOOD_PLAYCTL);
>  	if (ctl & KIRKWOOD_PLAYCTL_PAUSE) {
> @@ -329,7 +350,7 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
>  		/* configure */
> -		ctl = priv->ctl_play & priv->ctl_play_mask;
> +		ctl = priv->ctl_play;
>  		value = ctl & ~KIRKWOOD_PLAYCTL_ENABLE_MASK;
>  		writel(value, priv->io + KIRKWOOD_PLAYCTL);
>  
> @@ -344,7 +365,7 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
>  
>  	case SNDRV_PCM_TRIGGER_STOP:
>  		/* stop audio, disable interrupts */
> -		ctl |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE;
> +		ctl |= KIRKWOOD_PLAYCTL_PAUSE | mute_mask;
>  		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
>  
>  		value = readl(priv->io + KIRKWOOD_INT_MASK);
> @@ -358,13 +379,13 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
>  
>  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>  	case SNDRV_PCM_TRIGGER_SUSPEND:
> -		ctl |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE;
> +		ctl |= KIRKWOOD_PLAYCTL_PAUSE | mute_mask;
>  		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
>  		break;
>  
>  	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> -		ctl &= ~(KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE);
> +		ctl &= ~(KIRKWOOD_PLAYCTL_PAUSE | mute_mask);
>  		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
>  		break;
>  
> @@ -447,151 +468,56 @@ static int kirkwood_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
>  	return 0;
>  }
>  
> -static int kirkwood_i2s_play_i2s(struct snd_soc_dapm_widget *w,
> -	struct snd_kcontrol *ctl, int event)
> -{
> -	/* CPU DAI is not available, so use driver data from device */
> -	struct kirkwood_dma_data *priv = dev_get_drvdata(w->dapm->dev);
> -
> -	if (SND_SOC_DAPM_EVENT_ON(event))
> -		priv->ctl_play_mask |= KIRKWOOD_PLAYCTL_I2S_EN;
> -	else
> -		priv->ctl_play_mask &= ~KIRKWOOD_PLAYCTL_I2S_EN;
> -
> -	return 0;
> -}
> -
> -static int kirkwood_i2s_play_spdif(struct snd_soc_dapm_widget *w,
> -	struct snd_kcontrol *ctl, int event)
> -{
> -	/* CPU DAI is not available, so use driver data from device */
> -	struct kirkwood_dma_data *priv = dev_get_drvdata(w->dapm->dev);
> -
> -	if (SND_SOC_DAPM_EVENT_ON(event))
> -		priv->ctl_play_mask |= KIRKWOOD_PLAYCTL_SPDIF_EN;
> -	else
> -		priv->ctl_play_mask &= ~KIRKWOOD_PLAYCTL_SPDIF_EN;
> -
> -	return 0;
> -}
> -
> -static const struct snd_soc_dapm_widget widgets[] = {
> -	/* These widget names come from the names from the functional spec */
> -	SND_SOC_DAPM_AIF_OUT_E("i2sdo", "dma-tx",
> -		0, SND_SOC_NOPM, 0, 0, kirkwood_i2s_play_i2s,
> -		SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
> -	SND_SOC_DAPM_AIF_OUT_E("spdifdo", "dma-tx",
> -		0, SND_SOC_NOPM, 0, 0, kirkwood_i2s_play_spdif,
> -		SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
> -	SND_SOC_DAPM_AIF_IN("i2sdi", "dma-rx",
> -		0, SND_SOC_NOPM, 0, 0),
> -};
> -
> -static int kirkwood_i2s_probe(struct snd_soc_dai *dai)
> -{
> -	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai);
> -	unsigned long value;
> -	unsigned int reg_data;
> -	int ret;
> -
> -	ret = snd_soc_add_dai_controls(dai, kirkwood_i2s_iec958_controls,
> -				ARRAY_SIZE(kirkwood_i2s_iec958_controls));
> -	if (ret) {
> -		dev_err(dai->dev,
> -			"unable to add soc card controls: %d\n", ret);
> -		return ret;
> -	}
> -
> -	/* It appears that these can't be attached to the CPU DAI */
> -	snd_soc_dapm_new_controls(&dai->dapm, widgets, ARRAY_SIZE(widgets));
> -
> -	/* put system in a "safe" state : */
> -	/* disable audio interrupts */
> -	writel(0xffffffff, priv->io + KIRKWOOD_INT_CAUSE);
> -	writel(0, priv->io + KIRKWOOD_INT_MASK);
> -
> -	reg_data = readl(priv->io + 0x1200);
> -	reg_data &= (~(0x333FF8));
> -	reg_data |= 0x111D18;
> -	writel(reg_data, priv->io + 0x1200);
> -
> -	msleep(500);
> -
> -	reg_data = readl(priv->io + 0x1200);
> -	reg_data &= (~(0x333FF8));
> -	reg_data |= 0x111D18;
> -	writel(reg_data, priv->io + 0x1200);
> -
> -	priv->ctl_play_mask = ~KIRKWOOD_PLAYCTL_ENABLE_MASK;
> -
> -	/* disable playback/record */
> -	value = readl(priv->io + KIRKWOOD_PLAYCTL);
> -	value &= ~KIRKWOOD_PLAYCTL_ENABLE_MASK;
> -	writel(value, priv->io + KIRKWOOD_PLAYCTL);
> -
> -	value = readl(priv->io + KIRKWOOD_RECCTL);
> -	value &= ~(KIRKWOOD_RECCTL_I2S_EN | KIRKWOOD_RECCTL_SPDIF_EN);
> -	writel(value, priv->io + KIRKWOOD_RECCTL);
> -
> -	return 0;
> -
> -}
> -
> -static int kirkwood_i2s_remove(struct snd_soc_dai *dai)
> -{
> -	return 0;
> -}
> -
>  static const struct snd_soc_dai_ops kirkwood_i2s_dai_ops = {
>  	.startup	= kirkwood_i2s_startup,
> +	.shutdown	= kirkwood_i2s_shutdown,
>  	.trigger	= kirkwood_i2s_trigger,
>  	.hw_params      = kirkwood_i2s_hw_params,
>  	.set_fmt        = kirkwood_i2s_set_fmt,
>  };
>  
> +static int kirkwood_spdif_probe(struct snd_soc_dai *dai)
> +{
> +	int ret = snd_soc_add_dai_controls(dai, kirkwood_i2s_iec958_controls,
> +				ARRAY_SIZE(kirkwood_i2s_iec958_controls));
> +	if (ret)
> +		dev_err(dai->dev, "unable to add soc card controls\n");
>  
> -static struct snd_soc_dai_driver kirkwood_i2s_dai = {
> -	.probe = kirkwood_i2s_probe,
> -	.remove = kirkwood_i2s_remove,
> -	.playback = {
> -		.stream_name = "dma-tx",
> -		.channels_min = 1,
> -		.channels_max = 2,
> -		.rates = KIRKWOOD_I2S_RATES,
> -		.formats = KIRKWOOD_I2S_FORMATS,
> -	},
> -	.capture = {
> -		.stream_name = "dma-rx",
> -		.channels_min = 1,
> -		.channels_max = 2,
> -		.rates = KIRKWOOD_I2S_RATES,
> -		.formats = KIRKWOOD_I2S_FORMATS,
> -	},
> -	.ops = &kirkwood_i2s_dai_ops,
> -};
> +	return ret;
> +}
>  
> -static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk = {
> -	.probe = kirkwood_i2s_probe,
> -	.remove = kirkwood_i2s_remove,
> -	.playback = {
> -		.stream_name = "dma-tx",
> -		.channels_min = 1,
> -		.channels_max = 2,
> -		.rates = SNDRV_PCM_RATE_8000_192000 |
> -			 SNDRV_PCM_RATE_CONTINUOUS |
> -			 SNDRV_PCM_RATE_KNOT,
> -		.formats = KIRKWOOD_I2S_FORMATS,
> -	},
> -	.capture = {
> -		.stream_name = "dma-rx",
> -		.channels_min = 1,
> -		.channels_max = 2,
> -		.rates = SNDRV_PCM_RATE_8000_192000 |
> -			 SNDRV_PCM_RATE_CONTINUOUS |
> -			 SNDRV_PCM_RATE_KNOT,
> -		.formats = KIRKWOOD_I2S_FORMATS,
> +static struct snd_soc_dai_driver kirkwood_dais[KIRKWOOD_NUM_DAIS] = {
> +	{
> +		.name = "kirkwood-i2s.%d",
> +		.ops = &kirkwood_i2s_dai_ops,
> +		.playback = {
> +			.channels_min = 1,
> +			.channels_max = 2,
> +			.rates = KIRKWOOD_I2S_RATES,
> +			.formats = KIRKWOOD_I2S_FORMATS,
> +		},
> +		.capture = {
> +			.channels_min = 1,
> +			.channels_max = 2,
> +			.rates = KIRKWOOD_I2S_RATES,
> +			.formats = KIRKWOOD_I2S_FORMATS,
> +		},
> +		.symmetric_rates = 1,
> +	}, {
> +		.name = "kirkwood-spdif.%d",
> +		.probe = kirkwood_spdif_probe,
> +		.ops = &kirkwood_i2s_dai_ops,
> +		.playback = {
> +			.channels_min = 1,
> +			.channels_max = 2,
> +			.rates = KIRKWOOD_I2S_RATES,
> +			.formats = SNDRV_PCM_FMTBIT_S16_LE |
> +				   SNDRV_PCM_FMTBIT_S24_LE |
> +				   SNDRV_PCM_FMTBIT_S32_LE |
> +				   SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE |
> +				   SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE,
> +		},
>  	},
> -	.ops = &kirkwood_i2s_dai_ops,
>  };
>  
>  static const struct snd_soc_component_driver kirkwood_i2s_component = {
> @@ -601,10 +527,10 @@ static const struct snd_soc_component_driver kirkwood_i2s_component = {
>  static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
>  {
>  	struct kirkwood_asoc_platform_data *data = pdev->dev.platform_data;
> -	struct snd_soc_dai_driver *soc_dai = &kirkwood_i2s_dai;
>  	struct kirkwood_dma_data *priv;
>  	struct resource *mem;
> -	int err;
> +	int i, err;
> +	u32 val;
>  
>  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv) {
> @@ -613,6 +539,24 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
>  	}
>  	dev_set_drvdata(&pdev->dev, priv);
>  
> +	memcpy(priv->dai_driver, kirkwood_dais, sizeof(priv->dai_driver));
> +
> +	/* format the DAI names according to the platform device ID */
> +	for (i = 0; i < KIRKWOOD_NUM_DAIS; i++) {
> +		const char *fmt = priv->dai_driver[i].name;
> +		char *name, *p;
> +
> +		if (pdev->id < 0) {
> +			name = kstrdup(fmt, GFP_KERNEL);
> +			p = strchr(name, '.');
> +			if (p)
> +				*p = '\0';
> +		} else {
> +			name = kasprintf(GFP_KERNEL, fmt, pdev->id);
> +		}
> +		priv->dai_driver[i].name = name;
> +	}
> +
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	priv->io = devm_ioremap_resource(&pdev->dev, mem);
>  	if (IS_ERR(priv->io))
> @@ -647,9 +591,23 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
>  			devm_clk_put(&pdev->dev, priv->extclk);
>  			priv->extclk = ERR_PTR(-EINVAL);
>  		} else {
> +			int i;
> +
>  			dev_info(&pdev->dev, "found external clock\n");
>  			clk_prepare_enable(priv->extclk);
> -			soc_dai = &kirkwood_i2s_dai_extclk;
> +
> +			for (i = 0; i < KIRKWOOD_NUM_DAIS; i++) {
> +				if (priv->dai_driver[i].playback.rates)
> +					priv->dai_driver[i].playback.rates =
> +						SNDRV_PCM_RATE_8000_192000 |
> +						SNDRV_PCM_RATE_CONTINUOUS |
> +						SNDRV_PCM_RATE_KNOT;
> +				if (priv->dai_driver[i].capture.rates)
> +					priv->dai_driver[i].capture.rates =
> +						SNDRV_PCM_RATE_8000_192000 |
> +						SNDRV_PCM_RATE_CONTINUOUS |
> +						SNDRV_PCM_RATE_KNOT;
> +			}
>  		}
>  	}
>  
> @@ -666,8 +624,35 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
>  		priv->ctl_rec |= KIRKWOOD_RECCTL_BURST_128;
>  	}
>  
> +	/* put system in a "safe" state : disable audio interrupts */
> +	writel(0xffffffff, priv->io + KIRKWOOD_INT_CAUSE);
> +	writel(0, priv->io + KIRKWOOD_INT_MASK);
> +
> +	val = readl(priv->io + 0x120c);
> +	val = readl(priv->io + 0x1200);
> +	val &= (~(0x333FF8));
> +	val |= 0x111D18;
> +	writel(val, priv->io + 0x1200);
> +
> +	msleep(500);
> +
> +	val = readl(priv->io + 0x1200);
> +	val &= (~(0x333FF8));
> +	val |= 0x111D18;
> +	msleep(500);
> +	writel(val, priv->io + 0x1200);
> +
> +	/* disable playback/record */
> +	val = readl(priv->io + KIRKWOOD_PLAYCTL);
> +	val &= ~(KIRKWOOD_PLAYCTL_I2S_EN|KIRKWOOD_PLAYCTL_SPDIF_EN);
> +	writel(val, priv->io + KIRKWOOD_PLAYCTL);
> +
> +	val = readl(priv->io + KIRKWOOD_RECCTL);
> +	val &= ~(KIRKWOOD_RECCTL_I2S_EN | KIRKWOOD_RECCTL_SPDIF_EN);
> +	writel(val, priv->io + KIRKWOOD_RECCTL);
> +
>  	err = snd_soc_register_component(&pdev->dev, &kirkwood_i2s_component,
> -					 soc_dai, 1);
> +					 priv->dai_driver, KIRKWOOD_NUM_DAIS);
>  	if (err) {
>  		dev_err(&pdev->dev, "snd_soc_register_component failed\n");
>  		goto err_component;
> @@ -679,7 +664,6 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
>  		goto err_platform;
>  	}
>  	return 0;
> -
>   err_platform:
>  	snd_soc_unregister_component(&pdev->dev);
>   err_component:
> diff --git a/sound/soc/kirkwood/kirkwood-openrd.c b/sound/soc/kirkwood/kirkwood-openrd.c
> index 258fcce..c92f42a 100644
> --- a/sound/soc/kirkwood/kirkwood-openrd.c
> +++ b/sound/soc/kirkwood/kirkwood-openrd.c
> @@ -50,12 +50,11 @@ static struct snd_soc_ops openrd_client_ops = {
>  };
>  
>  
> -/* This will need to be updated when DPCM support works. */
>  static struct snd_soc_dai_link openrd_client_dai[] = {
>  {
>  	.name = "CS42L51",
>  	.stream_name = "CS42L51 HiFi",
> -	.cpu_dai_name = "mvebu-audio",
> +	.cpu_dai_name = "kirkwood-i2s.0",
>  	.platform_name = "mvebu-audio",
>  	.codec_dai_name = "cs42l51-hifi",
>  	.codec_name = "cs42l51-codec.0-004a",
> @@ -64,19 +63,12 @@ static struct snd_soc_dai_link openrd_client_dai[] = {
>  },
>  };
>  
> -static const struct snd_soc_dapm_route routes[] = {
> -	/* Connect the codec streams to the I2S connections */
> -	{ "Playback", NULL, "i2sdo" },
> -	{ "i2sdi", NULL, "Capture" },
> -};
>  
>  static struct snd_soc_card openrd_client = {
>  	.name = "OpenRD Client",
>  	.owner = THIS_MODULE,
>  	.dai_link = openrd_client_dai,
>  	.num_links = ARRAY_SIZE(openrd_client_dai),
> -	.dapm_routes = routes,
> -	.num_dapm_routes = ARRAY_SIZE(routes),
>  };
>  
>  static int openrd_probe(struct platform_device *pdev)
> diff --git a/sound/soc/kirkwood/kirkwood-t5325.c b/sound/soc/kirkwood/kirkwood-t5325.c
> index 5f5ae08..02911f7 100644
> --- a/sound/soc/kirkwood/kirkwood-t5325.c
> +++ b/sound/soc/kirkwood/kirkwood-t5325.c
> @@ -52,9 +52,6 @@ static const struct snd_soc_dapm_route t5325_route[] = {
>  
>  	{ "MIC1",		NULL,	"Mic Jack" },
>  	{ "MIC2",		NULL,	"Mic Jack" },
> -
> -	{ "i2sdi",		NULL,	"Capture" },
> -	{ "Playback",		NULL,	"i2sdo" },
>  };
>  
>  static int t5325_dai_init(struct snd_soc_pcm_runtime *rtd)
> @@ -69,12 +66,11 @@ static int t5325_dai_init(struct snd_soc_pcm_runtime *rtd)
>  	return 0;
>  }
>  
> -/* This will need to be updated when DPCM support works. */
>  static struct snd_soc_dai_link t5325_dai[] = {
>  {
>  	.name = "ALC5621",
>  	.stream_name = "ALC5621 HiFi",
> -	.cpu_dai_name = "mvebu-audio",
> +	.cpu_dai_name = "kirkwood-i2s.0",
>  	.platform_name = "mvebu-audio",
>  	.codec_dai_name = "alc5621-hifi",
>  	.codec_name = "alc562x-codec.0-001a",
> diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h
> index 17b48a6..595e0fe 100644
> --- a/sound/soc/kirkwood/kirkwood.h
> +++ b/sound/soc/kirkwood/kirkwood.h
> @@ -161,13 +161,16 @@
>  #define KIRKWOOD_SND_MAX_BUFFER_BYTES		(KIRKWOOD_SND_MAX_PERIOD_BYTES \
>  						 * KIRKWOOD_SND_MAX_PERIODS)
>  
> +#define KIRKWOOD_NUM_DAIS 2
> +
>  struct kirkwood_dma_data {
>  	void __iomem *io;
>  	struct clk *clk;
>  	struct clk *extclk;
> -	uint32_t ctl_play_mask;
>  	uint32_t ctl_play;
>  	uint32_t ctl_rec;
> +	struct snd_soc_dai *active_dai;
> +	struct snd_soc_dai_driver dai_driver[KIRKWOOD_NUM_DAIS];
>  	struct snd_pcm_substream *substream_play;
>  	struct snd_pcm_substream *substream_rec;
>  	int irq;
> 

[alsa-devel] [PATCH 00/14] SPDIF support

[Mark Brown]

On Sun, Sep 01, 2013 at 09:51:21AM +0100, Russell King - ARM Linux wrote:
> On Sun, Sep 01, 2013 at 09:42:29AM +0200, Lars-Peter Clausen wrote:

> > * With non-DPCM ASoC it is possible to have two DAIs if they are not used at
> > the same time (which is what I recommend you implement first, before trying
> > to get DPCM running).

> If you'd look at my other responses, you'll see that this is what I tried
> back in May, and I was unhappy about that solution because:

> 1. there is no guarantee that they couldn't be used at the same time.
> 2. this results in two entirely separate "CPU DAI"s, each with their
>    own independent sample rate/format settings, which if they happen
>    to be used together will result in fighting over the same register(s).

These aren't issues for either/or, the whole point here is that the
machine driver will be limited to only connecting one of the DAIs so
there will be no way for userspace to interact with the unused DAI.  As
Lars-Peter says you can add explicit checks for this in the code if you
are concerned about system integrators getting this wrong.

> Moreover, this results in a completely different set of changes to the
> driver which are in an opposing direction to the DPCM approach.

Like Lars-Peter says it really, really shouldn't be - what moving to
DPCM should be doing with this code is mostly moving the code around a
bit to pull the bits that are shared into a front end DAI.  The most
substantial change should be handling the enables but that shouldn't
take much code at all, your curent patch does it in 35 lines and I'd not
expect it to be much different in a DPCM world.

> > And I'm sure people are willing to help
> > you figure out the parts you don't understand yet if you ask _nicely_.

> Can you then please explain why when I ask for help understanding DAPM
> in a "nice" way, the response I get is just "it's just a graph walk"
> and no further technical details?

Ask more detailed questions and engage in a discussion; the reason you
keep on getting the same responses is that you tend to repeat the same
requests a lot.  Something like "I understand the big picture but I am
trying to do X and need to know Y because Z" (with all of X, Y and Z
being important) is usually a good approach.

The public interface for DAPM is that the widgets get created with
sensible types and hooked up together then powered up as needed, if
something needs to know specifics of that process like exactly when a
given register will be written that's a worrying implementation detail.

> | DAPM is a set of "widgets" representing various components of an
> | audio system.  The widgets are linked together by a graph.  Each
> | widget lives in a context - cpu, platform or codec.  Some bindings
> | only happen within a context, others cross contexts (so linking the
> | CPU audio stream to the codec for example)

This last statement is not the case; you can see the route connecting
code in snd_soc_dapm_add_route() - there is no case in which it will
only try to match within a single context.

As I have said to you without more information it is hard to tell what
problems you are seeing when you are trying this.  It could be a case of
trying to create routes before the widgets are added, or it could be a
case of trying to create inter device links with widgets with globally
duplicated names (though that would give wrong routes rather than no
routes so I suspect sequencing).

> > I mean I don't come to you either if I have a new ARM SoC that's not
> > supported yet and demand that you implement support for it and exclaim
> > that the ARM port sucks because it doesn't support that SoC yet.

> If you come to me and ask about something in ARM, then you will most
> likely get something more than a few words explaining a big chunk of
> code - a bit like the oops report I disected last night and provided
> full reasoning of the conclusion that I came to (SDRAM failure /
> hardware fault on bit 8 of the SDRAM data bus.)

The bit about the way in which support is requested is important here -
it really can make a really big difference how one asks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130901/a6759ac6/attachment.sig>

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Sun, Sep 01, 2013 at 12:51:16PM +0100, Mark Brown wrote:
> Like Lars-Peter says it really, really shouldn't be - what moving to
> DPCM should be doing with this code is mostly moving the code around a
> bit to pull the bits that are shared into a front end DAI.  The most
> substantial change should be handling the enables but that shouldn't
> take much code at all, your curent patch does it in 35 lines and I'd not
> expect it to be much different in a DPCM world.

The delta between the DPCM version and the dual-DAI version is over 300
lines of change - the methods employed by these two methods are completely
different.

Maybe you could look at the patch and suggest where I'm going wrong?

> On Sun, Sep 01, 2013 at 09:51:21AM +0100, Russell King - ARM Linux wrote:
> > Can you then please explain why when I ask for help understanding DAPM
> > in a "nice" way, the response I get is just "it's just a graph walk"
> > and no further technical details?
> 
> Ask more detailed questions and engage in a discussion; the reason you
> keep on getting the same responses is that you tend to repeat the same
> requests a lot.  Something like "I understand the big picture but I am
> trying to do X and need to know Y because Z" (with all of X, Y and Z
> being important) is usually a good approach.

When you don't even understand the "big picture", it's hard to know
where to start.  That's why starting off with a simple question is
the right thing to do - and you expect to get a simple but informative
response, so that helps you to frame more specific questions later.

If you start from a position of knowing very little, it's exceedingly
difficult to ask specific questions.

> The public interface for DAPM is that the widgets get created with
> sensible types and hooked up together then powered up as needed, if
> something needs to know specifics of that process like exactly when a
> given register will be written that's a worrying implementation detail.
> 
> > | DAPM is a set of "widgets" representing various components of an
> > | audio system.  The widgets are linked together by a graph.  Each
> > | widget lives in a context - cpu, platform or codec.  Some bindings
> > | only happen within a context, others cross contexts (so linking the
> > | CPU audio stream to the codec for example)
> 
> This last statement is not the case; you can see the route connecting
> code in snd_soc_dapm_add_route() - there is no case in which it will
> only try to match within a single context.
> 
> As I have said to you without more information it is hard to tell what
> problems you are seeing when you are trying this.  It could be a case of
> trying to create routes before the widgets are added, or it could be a
> case of trying to create inter device links with widgets with globally
> duplicated names (though that would give wrong routes rather than no
> routes so I suspect sequencing).

Right, so, I have a widget with a stream name, and the stream name matches
a CPU DAI stream.

If I register this widget against the platform DAPM context, then there is
no connection between this widget and the CPU DAI streams.  (Bear in mind
that at the time I tried this, I had disabled the creation of the stream
widgets that were overwriting the platform stream widgets - because you
were not providing any useful information to work around that problem.)

If I register this widget against the CPU DAI DAPM context, the stream
name is matched to the CPU DAI streams, and a connection is made between
the stream widgets and my widget.

This is what I mean by "some bindings only happen within a context".

[alsa-devel] [PATCH 00/14] SPDIF support

[Mark Brown]

On Sun, Sep 01, 2013 at 01:15:18PM +0100, Russell King - ARM Linux wrote:
> On Sun, Sep 01, 2013 at 12:51:16PM +0100, Mark Brown wrote:
> > Like Lars-Peter says it really, really shouldn't be - what moving to
> > DPCM should be doing with this code is mostly moving the code around a
> > bit to pull the bits that are shared into a front end DAI.  The most
> > substantial change should be handling the enables but that shouldn't
> > take much code at all, your curent patch does it in 35 lines and I'd not
> > expect it to be much different in a DPCM world.

> The delta between the DPCM version and the dual-DAI version is over 300
> lines of change - the methods employed by these two methods are completely
> different.

> Maybe you could look at the patch and suggest where I'm going wrong?

The diff doesn't look that worrying to me - a large chunk of it is about
moving code around to register multiple DAIs which is something that the
final DPCM code is going to want to do anyway.  A lot of the changes in
the hw_params() function would probably stay the same too, though the
function would move to the front end DAI I expect.

> > Ask more detailed questions and engage in a discussion; the reason you
> > keep on getting the same responses is that you tend to repeat the same
> > requests a lot.  Something like "I understand the big picture but I am
> > trying to do X and need to know Y because Z" (with all of X, Y and Z
> > being important) is usually a good approach.

> When you don't even understand the "big picture", it's hard to know
> where to start.  That's why starting off with a simple question is
> the right thing to do - and you expect to get a simple but informative
> response, so that helps you to frame more specific questions later.

> If you start from a position of knowing very little, it's exceedingly
> difficult to ask specific questions.

So say that - explain that you find it hard to relate the answer to what
you're trying to accomplish, doing things like providing more detail on
the problem you're trying to solve and highlighting anything that you've
noticed is unusual to try to help people understand what might be
missing from their answers.

> Right, so, I have a widget with a stream name, and the stream name matches
> a CPU DAI stream.

Ah, this is stream names not regular routes - new code should just use a
normal DAPM route to connect to the AIF widgets.  Stream names are being
phased out now that we can just use DAPM routes (which mean that we
don't need to go through every single widget doing string checks every
time we start or stop a stream).

> If I register this widget against the platform DAPM context, then there is
> no connection between this widget and the CPU DAI streams.  (Bear in mind
> that at the time I tried this, I had disabled the creation of the stream
> widgets that were overwriting the platform stream widgets - because you
> were not providing any useful information to work around that problem.)

> If I register this widget against the CPU DAI DAPM context, the stream
> name is matched to the CPU DAI streams, and a connection is made between
> the stream widgets and my widget.

> This is what I mean by "some bindings only happen within a context".

This is happening because you're doing things based on the stream name -
stream names aren't really DAPM routes, they're magic things which only
work on the same DAPM context.  Previously we used to go through and do
the string compare to look up widgets outside of DAPM at runtime which
on a per device basis (though that made little difference since the only
devices supported were CODECs which are always a single device and
context), the current implementation replicates this functionality
exactly using DAPM routes and the long term plan is that no stream names
will be specified in widgets.

If you omit the stream name and add the routes via a routing table you
should at least get some error messages.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130901/6ff3aedc/attachment.sig>

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Sat, Aug 31, 2013 at 05:28:26PM +0200, Lars-Peter Clausen wrote:
> On 08/31/2013 02:34 PM, Russell King - ARM Linux wrote:
> [...]
> > The same conditions apply as per my previous posting - the DAI link
> > needs to be setup and the associated DAPM routes to tell the CPU DAI
> > which outputs are in use, like this:
> > 
> > DAI link:
> >                 .name = "S/PDIF1",
> >                 .stream_name = "IEC958 Playback",
> >                 .platform_name = "mvebu-audio.1",
> >                 .cpu_dai_name = "mvebu-audio.1",
> >                 .codec_dai_name = "dit-hifi",
> >                 .codec_name = "spdif-dit",
> > 
> > static const struct snd_soc_dapm_route routes[] = {
> >         { "Playback", NULL, "spdifdo" },
> > };
> 
> This is still not exactly the right way to implement this though. Add a
> second DAI to your CPU driver, like this:

What you're suggesting is the DPCM solution.

That would be fine _if_ it works, which it doesn't.  Not only does it
cause the kernel to spit out various warnings (caused by the creation
of multiple procfs files with the same name) but it also causes a kernel
oops when VLC tries to use it (due to NULL ops in the ALSA PCM.)

As I said in my cover to this patch series: this is the minimum set of
changes which provide a working solution.

I would like to have the DPCM solution, but at present that's far from
possible.

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Sat, Aug 31, 2013 at 08:14:14PM +0100, Russell King - ARM Linux wrote:
> On Sat, Aug 31, 2013 at 05:28:26PM +0200, Lars-Peter Clausen wrote:
> > On 08/31/2013 02:34 PM, Russell King - ARM Linux wrote:
> > [...]
> > > The same conditions apply as per my previous posting - the DAI link
> > > needs to be setup and the associated DAPM routes to tell the CPU DAI
> > > which outputs are in use, like this:
> > > 
> > > DAI link:
> > >                 .name = "S/PDIF1",
> > >                 .stream_name = "IEC958 Playback",
> > >                 .platform_name = "mvebu-audio.1",
> > >                 .cpu_dai_name = "mvebu-audio.1",
> > >                 .codec_dai_name = "dit-hifi",
> > >                 .codec_name = "spdif-dit",
> > > 
> > > static const struct snd_soc_dapm_route routes[] = {
> > >         { "Playback", NULL, "spdifdo" },
> > > };
> > 
> > This is still not exactly the right way to implement this though. Add a
> > second DAI to your CPU driver, like this:
> 
> What you're suggesting is the DPCM solution.
> 
> That would be fine _if_ it works, which it doesn't.  Not only does it
> cause the kernel to spit out various warnings (caused by the creation
> of multiple procfs files with the same name) but it also causes a kernel
> oops when VLC tries to use it (due to NULL ops in the ALSA PCM.)

Here's the warning I get - I've left the syslog stamp in these as
evidence for how long this has been known - and I reported these to
Mark when I found them:

Aug 10 15:17:18 cubox kernel: WARNING: at /home/rmk/git/linux-cubox/fs/proc/generic.c:356 proc_register+0xac/0x128()
Aug 10 15:17:18 cubox kernel: proc_dir_entry 'asound/oss' already registered
Aug 10 15:17:18 cubox kernel: Modules linked in: snd_soc_spdif_tx m25p80 orion_wdt mtd snd_soc_kirkwood snd_soc_kirkwood_spdif
Aug 10 15:17:18 cubox kernel: CPU: 0 PID: 388 Comm: kworker/u2:2 Not tainted 3.10.0+ #645
Aug 10 15:17:18 cubox kernel: Workqueue: deferwq deferred_probe_work_func
Aug 10 15:17:18 cubox kernel: [<c0013d7c>] (unwind_backtrace+0x0/0xf8) from [<c0011bec>] (show_stack+0x10/0x14)
Aug 10 15:17:18 cubox kernel: [<c0011bec>] (show_stack+0x10/0x14) from [<c0048b80>] (warn_slowpath_common+0x4c/0x68)
Aug 10 15:17:18 cubox kernel: [<c0048b80>] (warn_slowpath_common+0x4c/0x68) from [<c0048c30>] (warn_slowpath_fmt+0x30/0x40)
Aug 10 15:17:18 cubox kernel: [<c0048c30>] (warn_slowpath_fmt+0x30/0x40) from [<c0124804>] (proc_register+0xac/0x128)
Aug 10 15:17:18 cubox kernel: [<c0124804>] (proc_register+0xac/0x128) from [<c0124910>] (proc_create_data+0x90/0xac)
Aug 10 15:17:18 cubox kernel: [<c0124910>] (proc_create_data+0x90/0xac) from [<c0302b48>] (snd_info_register+0x54/0xf0)
Aug 10 15:17:18 cubox kernel: [<c0302b48>] (snd_info_register+0x54/0xf0) from [<c03190e8>] (snd_pcm_oss_register_minor+0xcc/0x1cc)
Aug 10 15:17:18 cubox kernel: [<c03190e8>] (snd_pcm_oss_register_minor+0xcc/0x1cc) from [<c030bdec>] (snd_pcm_dev_register+0x1ac/0x290)
Aug 10 15:17:18 cubox kernel: [<c030bdec>] (snd_pcm_dev_register+0x1ac/0x290) from [<c03069c8>] (snd_device_register_all+0x40/0x80)
Aug 10 15:17:18 cubox kernel: [<c03069c8>] (snd_device_register_all+0x40/0x80) from [<c030192c>] (snd_card_register+0x24/0x228)
Aug 10 15:17:18 cubox kernel: [<c030192c>] (snd_card_register+0x24/0x228) from [<c03252a4>] (snd_soc_instantiate_card+0x7b4/0x868)
Aug 10 15:17:18 cubox kernel: [<c03252a4>] (snd_soc_instantiate_card+0x7b4/0x868) from [<c03255c4>] (snd_soc_register_card+0x26c/0x324)
Aug 10 15:17:18 cubox kernel: [<c03255c4>] (snd_soc_register_card+0x26c/0x324) from [<bf00c0b4>] (kirkwood_spdif_probe+0x88/0xd8 [snd_soc_kirkwood_spdif])
Aug 10 15:17:18 cubox kernel: [<bf00c0b4>] (kirkwood_spdif_probe+0x88/0xd8 [snd_soc_kirkwood_spdif]) from [<c0259468>] (platform_drv_probe+0x18/0x1c)
Aug 10 15:17:18 cubox kernel: [<c0259468>] (platform_drv_probe+0x18/0x1c) from [<c0258144>] (really_probe+0x74/0x1f4)
Aug 10 15:17:18 cubox kernel: [<c0258144>] (really_probe+0x74/0x1f4) from [<c02583d8>] (driver_probe_device+0x30/0x48)
Aug 10 15:17:18 cubox kernel: [<c02583d8>] (driver_probe_device+0x30/0x48) from [<c0256a48>] (bus_for_each_drv+0x60/0x8c)
Aug 10 15:17:18 cubox kernel: [<c0256a48>] (bus_for_each_drv+0x60/0x8c) from [<c0258368>] (device_attach+0x80/0xa4)
Aug 10 15:17:18 cubox kernel: [<c0258368>] (device_attach+0x80/0xa4) from [<c02577a8>] (bus_probe_device+0x88/0xac)
Aug 10 15:17:18 cubox kernel: [<c02577a8>] (bus_probe_device+0x88/0xac) from [<c0257c34>] (deferred_probe_work_func+0x6c/0x9c)
Aug 10 15:17:18 cubox kernel: [<c0257c34>] (deferred_probe_work_func+0x6c/0x9c) from [<c0060a74>] (process_one_work+0x190/0x3f4)
Aug 10 15:17:18 cubox kernel: [<c0060a74>] (process_one_work+0x190/0x3f4) from [<c0062544>] (worker_thread+0xf4/0x318)
Aug 10 15:17:18 cubox kernel: [<c0062544>] (worker_thread+0xf4/0x318) from [<c006800c>] (kthread+0xa8/0xb4)
Aug 10 15:17:18 cubox kernel: [<c006800c>] (kthread+0xa8/0xb4) from [<c000e4a8>] (ret_from_fork+0x14/0x2c)
Aug 10 15:17:18 cubox kernel: ---[ end trace 174d2956b4f53cd5 ]---
Aug 10 15:17:18 cubox kernel: ------------[ cut here ]------------
Aug 10 15:17:18 cubox kernel: WARNING: at /home/rmk/git/linux-cubox/fs/proc/generic.c:356 proc_register+0xac/0x128()
Aug 10 15:17:18 cubox kernel: proc_dir_entry 'asound/oss' already registered
Aug 10 15:17:18 cubox kernel: Modules linked in: snd_soc_spdif_tx m25p80 orion_wdt mtd snd_soc_kirkwood snd_soc_kirkwood_spdif
Aug 10 15:17:18 cubox kernel: CPU: 0 PID: 388 Comm: kworker/u2:2 Tainted: G   W    3.10.0+ #645
Aug 10 15:17:18 cubox kernel: Workqueue: deferwq deferred_probe_work_func
Aug 10 15:17:18 cubox kernel: [<c0013d7c>] (unwind_backtrace+0x0/0xf8) from [<c0011bec>] (show_stack+0x10/0x14)
Aug 10 15:17:18 cubox kernel: [<c0011bec>] (show_stack+0x10/0x14) from [<c0048b80>] (warn_slowpath_common+0x4c/0x68)
Aug 10 15:17:18 cubox kernel: [<c0048b80>] (warn_slowpath_common+0x4c/0x68) from [<c0048c30>] (warn_slowpath_fmt+0x30/0x40)
Aug 10 15:17:18 cubox kernel: [<c0048c30>] (warn_slowpath_fmt+0x30/0x40) from [<c0124804>] (proc_register+0xac/0x128)
Aug 10 15:17:18 cubox kernel: [<c0124804>] (proc_register+0xac/0x128) from [<c0124910>] (proc_create_data+0x90/0xac)
Aug 10 15:17:18 cubox kernel: [<c0124910>] (proc_create_data+0x90/0xac) from [<c0302b48>] (snd_info_register+0x54/0xf0)
Aug 10 15:17:18 cubox kernel: [<c0302b48>] (snd_info_register+0x54/0xf0) from [<c03190e8>] (snd_pcm_oss_register_minor+0xcc/0x1cc)
Aug 10 15:17:18 cubox kernel: [<c03190e8>] (snd_pcm_oss_register_minor+0xcc/0x1cc) from [<c030bdec>] (snd_pcm_dev_register+0x1ac/0x290)
Aug 10 15:17:18 cubox kernel: [<c030bdec>] (snd_pcm_dev_register+0x1ac/0x290) from [<c03069c8>] (snd_device_register_all+0x40/0x80)
Aug 10 15:17:18 cubox kernel: [<c03069c8>] (snd_device_register_all+0x40/0x80) from [<c030192c>] (snd_card_register+0x24/0x228)
Aug 10 15:17:18 cubox kernel: [<c030192c>] (snd_card_register+0x24/0x228) from [<c03252a4>] (snd_soc_instantiate_card+0x7b4/0x868)
Aug 10 15:17:18 cubox kernel: [<c03252a4>] (snd_soc_instantiate_card+0x7b4/0x868) from [<c03255c4>] (snd_soc_register_card+0x26c/0x324)
Aug 10 15:17:18 cubox kernel: [<c03255c4>] (snd_soc_register_card+0x26c/0x324) from [<bf00c0b4>] (kirkwood_spdif_probe+0x88/0xd8 [snd_soc_kirkwood_spdif])
Aug 10 15:17:18 cubox kernel: [<bf00c0b4>] (kirkwood_spdif_probe+0x88/0xd8 [snd_soc_kirkwood_spdif]) from [<c0259468>] (platform_drv_probe+0x18/0x1c)
Aug 10 15:17:18 cubox kernel: [<c0259468>] (platform_drv_probe+0x18/0x1c) from [<c0258144>] (really_probe+0x74/0x1f4)
Aug 10 15:17:18 cubox kernel: [<c0258144>] (really_probe+0x74/0x1f4) from [<c02583d8>] (driver_probe_device+0x30/0x48)
Aug 10 15:17:18 cubox kernel: [<c02583d8>] (driver_probe_device+0x30/0x48) from [<c0256a48>] (bus_for_each_drv+0x60/0x8c)
Aug 10 15:17:18 cubox kernel: [<c0256a48>] (bus_for_each_drv+0x60/0x8c) from [<c0258368>] (device_attach+0x80/0xa4)
Aug 10 15:17:18 cubox kernel: [<c0258368>] (device_attach+0x80/0xa4) from [<c02577a8>] (bus_probe_device+0x88/0xac)
Aug 10 15:17:18 cubox kernel: [<c02577a8>] (bus_probe_device+0x88/0xac) from [<c0257c34>] (deferred_probe_work_func+0x6c/0x9c)
Aug 10 15:17:18 cubox kernel: [<c0257c34>] (deferred_probe_work_func+0x6c/0x9c) from [<c0060a74>] (process_one_work+0x190/0x3f4)
Aug 10 15:17:18 cubox kernel: [<c0060a74>] (process_one_work+0x190/0x3f4) from [<c0062544>] (worker_thread+0xf4/0x318)
Aug 10 15:17:18 cubox kernel: [<c0062544>] (worker_thread+0xf4/0x318) from [<c006800c>] (kthread+0xa8/0xb4)
Aug 10 15:17:18 cubox kernel: [<c006800c>] (kthread+0xa8/0xb4) from [<c000e4a8>] (ret_from_fork+0x14/0x2c)

I also get a load of these:

Aug 10 16:09:27 cubox kernel: S!PDIF1: ASoC: no backend DAIs enabled for S/PDIF1

from time to time as there's no way to tell ASoC in a DPCM confirmation
that although the CPU DAI has capture capability, the capture side is
not connected to any codec.

And here's the oops:

Aug 10 22:06:17 cubox kernel: Unable to handle kernel NULL pointer dereference at virtual address 00000008
Aug 10 22:06:17 cubox kernel: pgd = d2450000
Aug 10 22:06:17 cubox kernel: [00000008] *pgd=12285831, *pte=00000000, *ppte=00000000
Aug 10 22:06:17 cubox kernel: Internal error: Oops: 17 [#1] PREEMPT ARM
Aug 10 22:06:17 cubox kernel: Modules linked in: fuse bnep rfcomm bluetooth ext2 ext3 jbd snd_soc_spdif_tx m25p80 orion_wdt mtd snd_soc_kirkwood snd_soc_kirkwood_spdif
Aug 10 22:06:17 cubox kernel: CPU: 0 PID: 2514 Comm: vlc Not tainted 3.10.0+ #652
Aug 10 22:06:17 cubox kernel: task: d8102800 ti: d38fa000 task.ti: d38fa000
Aug 10 22:06:17 cubox kernel: PC is at snd_pcm_info+0xc8/0xd8
Aug 10 22:06:17 cubox kernel: LR is at 0x30232065
Aug 10 22:06:17 cubox kernel: pc : [<c030f724>]    lr : [<30232065>]    psr: a00f0013
Aug 10 22:06:17 cubox kernel: sp : d38fbea8  ip : d8c2ead0  fp : c05de6d8
Aug 10 22:06:17 cubox kernel: r10: c05de7d0  r9 : fffffdfd  r8 : 00000000
Aug 10 22:06:17 cubox kernel: r7 : d8c268a8  r6 : d8c26800  r5 : d8c26c00  r4 : d8c2ea00
Aug 10 22:06:17 cubox kernel: r3 : 00000000  r2 : d8c2ea00  r1 : 00000001  r0 : d8c26c00
Aug 10 22:06:17 cubox kernel: Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
Aug 10 22:06:17 cubox kernel: Control: 10c5387d  Table: 12450019  DAC: 00000015
Aug 10 22:06:17 cubox kernel: Process vlc (pid: 2514, stack limit = 0xd38fa248)
Aug 10 22:06:17 cubox kernel: Stack: (0xd38fbea8 to 0xd38fc000)
Aug 10 22:06:17 cubox kernel: bea0:                   c0af7144 d8c2ea00 d8c26c00 ab5032b8 00000001 c030f768
Aug 10 22:06:17 cubox kernel: bec0: 00000000 d8c20000 ab5032b8 c030a67c 0000001b d116b840 d8380330 c1205531
Aug 10 22:06:17 cubox kernel: bee0: 0000001b d116b840 d8263fc0 d8c20000 ab5032b8 c03056b0 00000001 c05e6e80
Aug 10 22:06:17 cubox kernel: bf00: c05e6e88 c05be828 00020120 00000000 d38fa000 600f0013 00000001 0000001b
Aug 10 22:06:17 cubox kernel: bf20: d38fa000 00020000 ab5032b8 c0088fec 00000001 00000000 d38fa000 00000000
Aug 10 22:06:17 cubox kernel: bf40: 600f0013 ca17c380 0000001b ab5032b8 d8380330 0000001b d38fa000 00020000
Aug 10 22:06:17 cubox kernel: bf60: ab5032b8 c00e50bc c00edecc 00020000 ab5032b8 00000001 ca17c380 ab5032b8
Aug 10 22:06:17 cubox kernel: bf80: c1205531 c00e5394 ab50366c 00000001 00000000 000120b0 ab50366c 00000036
Aug 10 22:06:17 cubox kernel: bfa0: c000e5a8 c000e3e0 00000000 000120b0 0000001b c1205531 ab5032b8 a91a3e10
Aug 10 22:06:17 cubox kernel: bfc0: 00000000 000120b0 ab50366c 00000036 ab503454 00000001 00000000 ab5032b8
Aug 10 22:06:17 cubox kernel: [<c030f724>] (snd_pcm_info+0xc8/0xd8) from [<c030f768>] (snd_pcm_info_user+0x34/0x9c)
Aug 10 22:06:17 cubox kernel: [<c030f768>] (snd_pcm_info_user+0x34/0x9c) from [<c030a67c>] (snd_pcm_control_ioctl+0x274/0x280)
Aug 10 22:06:17 cubox kernel: [<c030a67c>] (snd_pcm_control_ioctl+0x274/0x280) from [<c03056b0>] (snd_ctl_ioctl+0xc0/0x55c)
Aug 10 22:06:17 cubox kernel: [<c03056b0>] (snd_ctl_ioctl+0xc0/0x55c) from [<c00e50bc>] (do_vfs_ioctl+0x80/0x31c)
Aug 10 22:06:17 cubox kernel: [<c00e50bc>] (do_vfs_ioctl+0x80/0x31c) from [<c00e5394>] (SyS_ioctl+0x3c/0x60)
Aug 10 22:06:17 cubox kernel: [<c00e5394>] (SyS_ioctl+0x3c/0x60) from [<c000e3e0>] (ret_fast_syscall+0x0/0x48)
Aug 10 22:06:17 cubox kernel: Code: e1a00005 e59530dc e3a01001 e1a02004 (e5933008)

This is caused because substream->ops is NULL.  Why that's the case,
I don't know, but I believe the PCM which is trying to be operated on
is the one registered against the backend (by snd_pcm_new_internal).

This is why I'm soo frustrated with Mark: Mark just churns the same
old useless statements out without _listening_ to anything I've said
or providing anything useful to help with the problems I find.

[alsa-devel] [PATCH 00/14] SPDIF support

[Mark Brown]

On Sat, Aug 31, 2013 at 08:34:31PM +0100, Russell King - ARM Linux wrote:

> Here's the warning I get - I've left the syslog stamp in these as
> evidence for how long this has been known - and I reported these to
> Mark when I found them:

> Aug 10 15:17:18 cubox kernel: WARNING: at /home/rmk/git/linux-cubox/fs/proc/generic.c:356 proc_register+0xac/0x128()
> Aug 10 15:17:18 cubox kernel: proc_dir_entry 'asound/oss' already registered

You will recall that when you reported this I told you that OSS
emulation is not really supported with ASoC and that I would expect a
large proportion of drivers to fail when used with it.  There was no
remaining interest in OSS, the use cases that did exist were all desktop
based but those all seem to have gone away too now.

To clarify what this means is that if you want to use to use the
in-kernel OSS emulation you are pretty much on your own.

> I also get a load of these:

> Aug 10 16:09:27 cubox kernel: S!PDIF1: ASoC: no backend DAIs enabled for S/PDIF1

> from time to time as there's no way to tell ASoC in a DPCM confirmation
> that although the CPU DAI has capture capability, the capture side is
> not connected to any codec.

Yup, as I've said that's an issue.  There should be no need to tell the
framework anything extra, it should be able to figure it out for itself
based on information it already has - I've suggested just going through
and looking to see if there are any DAIs that could possibly be
connected before printing the warning, only checking if the user is
actually trying to do a capture should also do the trick.

> And here's the oops:

> Aug 10 22:06:17 cubox kernel: [<c030f724>] (snd_pcm_info+0xc8/0xd8) from [<c030f768>] (snd_pcm_info_user+0x34/0x9c)
> Aug 10 22:06:17 cubox kernel: [<c030f768>] (snd_pcm_info_user+0x34/0x9c) from [<c030a67c>] (snd_pcm_control_ioctl+0x274/0x280)

> This is caused because substream->ops is NULL.  Why that's the case,
> I don't know, but I believe the PCM which is trying to be operated on
> is the one registered against the backend (by snd_pcm_new_internal).

Liam did provide you with a workaround for this one along with a
suggested proper fix.  Have you had any success in using this
workaround, or with implementing the proper fix for that matter?  I
don't recall seeing any feedback on the results but perhaps I missed it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130902/63141e20/attachment.sig>

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Mon, Sep 02, 2013 at 03:47:13PM +0100, Mark Brown wrote:
> On Sat, Aug 31, 2013 at 08:34:31PM +0100, Russell King - ARM Linux wrote:
> > And here's the oops:
> 
> > Aug 10 22:06:17 cubox kernel: [<c030f724>] (snd_pcm_info+0xc8/0xd8) from [<c030f768>] (snd_pcm_info_user+0x34/0x9c)
> > Aug 10 22:06:17 cubox kernel: [<c030f768>] (snd_pcm_info_user+0x34/0x9c) from [<c030a67c>] (snd_pcm_control_ioctl+0x274/0x280)
> 
> > This is caused because substream->ops is NULL.  Why that's the case,
> > I don't know, but I believe the PCM which is trying to be operated on
> > is the one registered against the backend (by snd_pcm_new_internal).
> 
> Liam did provide you with a workaround for this one along with a
> suggested proper fix.  Have you had any success in using this
> workaround, or with implementing the proper fix for that matter?  I
> don't recall seeing any feedback on the results but perhaps I missed it.

I am not aware of any such patch.

[alsa-devel] [PATCH 00/14] SPDIF support

[Russell King - ARM Linux]

On Mon, Sep 02, 2013 at 03:52:07PM +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 02, 2013 at 03:47:13PM +0100, Mark Brown wrote:
> > On Sat, Aug 31, 2013 at 08:34:31PM +0100, Russell King - ARM Linux wrote:
> > > And here's the oops:
> > 
> > > Aug 10 22:06:17 cubox kernel: [<c030f724>] (snd_pcm_info+0xc8/0xd8) from [<c030f768>] (snd_pcm_info_user+0x34/0x9c)
> > > Aug 10 22:06:17 cubox kernel: [<c030f768>] (snd_pcm_info_user+0x34/0x9c) from [<c030a67c>] (snd_pcm_control_ioctl+0x274/0x280)
> > 
> > > This is caused because substream->ops is NULL.  Why that's the case,
> > > I don't know, but I believe the PCM which is trying to be operated on
> > > is the one registered against the backend (by snd_pcm_new_internal).
> > 
> > Liam did provide you with a workaround for this one along with a
> > suggested proper fix.  Have you had any success in using this
> > workaround, or with implementing the proper fix for that matter?  I
> > don't recall seeing any feedback on the results but perhaps I missed it.
> 
> I am not aware of any such patch.

That was a little hasty - I should say: I have not received any such patch
and I have nothing which suggests that a solution to this problem was even
outlined.  (I have checked my mailbox for anything from Liam, of which
there are a very small number of such mails.)

[alsa-devel] [PATCH 00/14] SPDIF support

[Mark Brown]

On Mon, Sep 02, 2013 at 03:57:22PM +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 02, 2013 at 03:52:07PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Sep 02, 2013 at 03:47:13PM +0100, Mark Brown wrote:

> > > > This is caused because substream->ops is NULL.  Why that's the case,
> > > > I don't know, but I believe the PCM which is trying to be operated on
> > > > is the one registered against the backend (by snd_pcm_new_internal).

> > > Liam did provide you with a workaround for this one along with a
> > > suggested proper fix.  Have you had any success in using this
> > > workaround, or with implementing the proper fix for that matter?  I
> > > don't recall seeing any feedback on the results but perhaps I missed it.

> > I am not aware of any such patch.

> That was a little hasty - I should say: I have not received any such patch
> and I have nothing which suggests that a solution to this problem was even
> outlined.  (I have checked my mailbox for anything from Liam, of which
> there are a very small number of such mails.)

I was thinking of this:

   http://article.gmane.org/gmane.linux.alsa.devel/111647

where Liam suggests setting some ops as a workaround and that the
framework should just check for them before dereferencing otherwise.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130902/996d10ea/attachment-0001.sig>

[alsa-devel] [PATCH 00/14] SPDIF support

[Lars-Peter Clausen]

On 08/31/2013 09:14 PM, Russell King - ARM Linux wrote:
> On Sat, Aug 31, 2013 at 05:28:26PM +0200, Lars-Peter Clausen wrote:
>> On 08/31/2013 02:34 PM, Russell King - ARM Linux wrote:
>> [...]
>>> The same conditions apply as per my previous posting - the DAI link
>>> needs to be setup and the associated DAPM routes to tell the CPU DAI
>>> which outputs are in use, like this:
>>>
>>> DAI link:
>>>                 .name = "S/PDIF1",
>>>                 .stream_name = "IEC958 Playback",
>>>                 .platform_name = "mvebu-audio.1",
>>>                 .cpu_dai_name = "mvebu-audio.1",
>>>                 .codec_dai_name = "dit-hifi",
>>>                 .codec_name = "spdif-dit",
>>>
>>> static const struct snd_soc_dapm_route routes[] = {
>>>         { "Playback", NULL, "spdifdo" },
>>> };
>>
>> This is still not exactly the right way to implement this though. Add a
>> second DAI to your CPU driver, like this:
> 
> What you're suggesting is the DPCM solution.

No it is not using any DPCM. You only need to use DPCM if you want to use
both DAIs at the same time. But maybe you want to do that?

- Lars