[PATCH 0/10] Kirkwood ASoC drivers fixes and improvements

[PATCH 0/10] Kirkwood ASoC drivers fixes and improvements

[Russell King - ARM Linux]

The first five patches of this patch series fixes a number of problems
with the Kirkwood audio driver, as seen on Dove platforms.  The biggest
issue is that they can cause the kernel to lock up solidly, requiring
a reset to recover.

Other issues included in the first six patches are bugs found by the
cubox folk.

The other five patches convert the I2S driver to use the devm_* APIs,
improve the handling of the record and playback control registers so
we're not reading and writing them as often, lift restrictions in the
DMA driver which aren't necessary to impose, and add support for
external clocks so that sample rates other than 44.1, 48 and 96kHz
can be supported.  Some of these changes prepare the driver to support
the SPDIF output including passthrough mode; however, that support is
not included in this patch set.

These patches probably need checking out on Kirkwood before they go
into mainline, but given the severity of the kernel lockup, I suggest
that once tested the first five patches go into -rc.

 sound/soc/kirkwood/kirkwood-dma.c |   19 ++-
 sound/soc/kirkwood/kirkwood-i2s.c |  291 ++++++++++++++++++++++---------------
 sound/soc/kirkwood/kirkwood.h     |   11 +-
 3 files changed, 197 insertions(+), 124 deletions(-)

 ASoC: kirkwood-dma: remove channel restrictions
 ASoC: kirkwood-i2s: add support for external clock rates
 ASoC: kirkwood-dma: remove restriction on sample rates
 ASoC: kirkwood-i2s: better handling of play/record control registers
 ASoC: kirkwood-i2s: use devm_* APIs
 ASoC: kirkwood-i2s: more pause-mode fixes
 ASoC: kirkwood-i2s: fix DMA underruns
 ASoC: kirkwood-i2s: fix DCO lock detection
 ASoC: kirkwood-dma: don't ignore other irq causes on error
 ASoC: kirkwood-dma: fix use of virt_to_phys()

[PATCH 01/10] ASoC: kirkwood-dma: fix use of virt_to_phys()

[Russell King]

This is part of a patch found in Rabeeh Khoury's git tree for the
cubox.

You can not use virt_to_phys() on the address returned from
dma_alloc_coherent(); it may not be part of the kernel direct-mapped
memory.  Fix this to use the DMA address instead.

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

diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c
index b9f1659..afe1930 100644
--- a/sound/soc/kirkwood/kirkwood-dma.c
+++ b/sound/soc/kirkwood/kirkwood-dma.c
@@ -178,7 +178,7 @@ static int kirkwood_dma_open(struct snd_pcm_substream *substream)
 	}
 
 	dram = mv_mbus_dram_info();
-	addr = virt_to_phys(substream->dma_buffer.area);
+	addr = substream->dma_buffer.addr;
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		prdata->play_stream = substream;
 		kirkwood_dma_conf_mbus_windows(priv->io,
-- 
1.7.4.4

[PATCH 02/10] ASoC: kirkwood-dma: don't ignore other irq causes on error

[Russell King]

Ignoring the real cause of the interrupt is not a good idea; this
behaviour has been observed to bring Dove platforms to silently
lockup.  Instead, on error fall through to the normal interrupt
processing.

This is especially important on Dove platforms as errors are
handled separately, and allows us to clear down the real cause of
the interrupt.

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

diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c
index afe1930..2ba0814 100644
--- a/sound/soc/kirkwood/kirkwood-dma.c
+++ b/sound/soc/kirkwood/kirkwood-dma.c
@@ -71,7 +71,6 @@ static irqreturn_t kirkwood_dma_irq(int irq, void *dev_id)
 		printk(KERN_WARNING "%s: got err interrupt 0x%lx\n",
 				__func__, cause);
 		writel(cause, priv->io + KIRKWOOD_ERR_CAUSE);
-		return IRQ_HANDLED;
 	}
 
 	/* we've enabled only bytes interrupts ... */
-- 
1.7.4.4

[PATCH 03/10] ASoC: kirkwood-i2s: fix DCO lock detection

[Russell King]

This is part of a patch found in Rabeeh Khoury's git tree for the
cubox, which is further attributed to Sebastian Hesselbrath.

Rather than masking the KIRKWOOD_DCO_SPCR_STATUS register contents
against the registers virtual address, let's actually use the bit
definition for the locked status, as required in the documentation.

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

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 542538d..485af80 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -95,7 +95,7 @@ static inline void kirkwood_set_dco(void __iomem *io, unsigned long rate)
 	do {
 		cpu_relax();
 		value = readl(io + KIRKWOOD_DCO_SPCR_STATUS);
-		value &= KIRKWOOD_DCO_SPCR_STATUS;
+		value &= KIRKWOOD_DCO_SPCR_STATUS_DCO_LOCK;
 	} while (value == 0);
 }
 
-- 
1.7.4.4

[PATCH 04/10] ASoC: kirkwood-i2s: fix DMA underruns

[Russell King]

Stress testing the driver with multiple start/stop events causes
kirkwood-dma to report underrun errors (which used to cause the kernel
to lock up solidly).  This is because kirkwood-i2s is not respecting
the restrictions imposed on clearing the 'pause' bit.  Follow what the
spec says; the busy bit must be read as being clear twice before the
pause bit can be released.  This solves the underruns.

However, it has been noticed that the busy bit occasionally does not
clear itself, hence the waiting is bounded to 5ms maximum to avoid a
new reason for the kernel to lockup.

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

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 485af80..826306d 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -180,67 +180,76 @@ 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);
-	unsigned long value;
-
-	/*
-	 * specs says KIRKWOOD_PLAYCTL must be read 2 times before
-	 * changing it. So read 1 time here and 1 later.
-	 */
-	value = readl(priv->io + KIRKWOOD_PLAYCTL);
+	uint32_t ctl, value;
+
+	ctl = readl(priv->io + KIRKWOOD_PLAYCTL);
+	if (ctl & KIRKWOOD_PLAYCTL_PAUSE) {
+		unsigned timeout = 5000;
+		/*
+		 * The Armada510 spec says that if we enter pause mode, the
+		 * busy bit must be read back as clear _twice_.  Make sure
+		 * we respect that otherwise we get DMA underruns.
+		 */
+		do {
+			value = ctl;
+			ctl = readl(priv->io + KIRKWOOD_PLAYCTL);
+			if (!((ctl | value) & KIRKWOOD_PLAYCTL_PLAY_BUSY))
+				break;
+			udelay(1);
+		} while (timeout--);
+
+		if ((ctl | value) & KIRKWOOD_PLAYCTL_PLAY_BUSY)
+			dev_notice(dai->dev, "timed out waiting for busy to deassert: %08x\n",
+				   ctl);
+	}
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 		/* stop audio, enable interrupts */
-		value = readl(priv->io + KIRKWOOD_PLAYCTL);
-		value |= KIRKWOOD_PLAYCTL_PAUSE;
-		writel(value, priv->io + KIRKWOOD_PLAYCTL);
+		ctl |= KIRKWOOD_PLAYCTL_PAUSE;
+		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
 
 		value = readl(priv->io + KIRKWOOD_INT_MASK);
 		value |= KIRKWOOD_INT_CAUSE_PLAY_BYTES;
 		writel(value, priv->io + KIRKWOOD_INT_MASK);
 
 		/* configure audio & enable i2s playback */
-		value = readl(priv->io + KIRKWOOD_PLAYCTL);
-		value &= ~KIRKWOOD_PLAYCTL_BURST_MASK;
-		value &= ~(KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE
+		ctl &= ~KIRKWOOD_PLAYCTL_BURST_MASK;
+		ctl &= ~(KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE
 				| KIRKWOOD_PLAYCTL_SPDIF_EN);
 
 		if (priv->burst == 32)
-			value |= KIRKWOOD_PLAYCTL_BURST_32;
+			ctl |= KIRKWOOD_PLAYCTL_BURST_32;
 		else
-			value |= KIRKWOOD_PLAYCTL_BURST_128;
-		value |= KIRKWOOD_PLAYCTL_I2S_EN;
-		writel(value, priv->io + KIRKWOOD_PLAYCTL);
+			ctl |= KIRKWOOD_PLAYCTL_BURST_128;
+		ctl |= KIRKWOOD_PLAYCTL_I2S_EN;
+		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
 		break;
 
 	case SNDRV_PCM_TRIGGER_STOP:
 		/* stop audio, disable interrupts */
-		value = readl(priv->io + KIRKWOOD_PLAYCTL);
-		value |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE;
-		writel(value, priv->io + KIRKWOOD_PLAYCTL);
+		ctl |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE;
+		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
 
 		value = readl(priv->io + KIRKWOOD_INT_MASK);
 		value &= ~KIRKWOOD_INT_CAUSE_PLAY_BYTES;
 		writel(value, priv->io + KIRKWOOD_INT_MASK);
 
 		/* disable all playbacks */
-		value = readl(priv->io + KIRKWOOD_PLAYCTL);
-		value &= ~(KIRKWOOD_PLAYCTL_I2S_EN | KIRKWOOD_PLAYCTL_SPDIF_EN);
-		writel(value, priv->io + KIRKWOOD_PLAYCTL);
+		ctl &= ~(KIRKWOOD_PLAYCTL_I2S_EN | KIRKWOOD_PLAYCTL_SPDIF_EN);
+		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
 		break;
 
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
-		value = readl(priv->io + KIRKWOOD_PLAYCTL);
-		value |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE;
-		writel(value, priv->io + KIRKWOOD_PLAYCTL);
+		ctl |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE;
+		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
 		break;
 
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		value = readl(priv->io + KIRKWOOD_PLAYCTL);
-		value &= ~(KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE);
-		writel(value, priv->io + KIRKWOOD_PLAYCTL);
+		ctl &= ~(KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE);
+		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
 		break;
 
 	default:
-- 
1.7.4.4

[PATCH 05/10] ASoC: kirkwood-i2s: more pause-mode fixes

[Russell King]

Don't even momentarily set the pause status when starting the channel;
if we do, we should check the busy bit to ensure that we comply with
the spec.  In any case, it isn't necessary; we will not active on a
START event so there is no need to pause the DMA.

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

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 826306d..1d5db48 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -205,10 +205,6 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
-		/* stop audio, enable interrupts */
-		ctl |= KIRKWOOD_PLAYCTL_PAUSE;
-		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
-
 		value = readl(priv->io + KIRKWOOD_INT_MASK);
 		value |= KIRKWOOD_INT_CAUSE_PLAY_BYTES;
 		writel(value, priv->io + KIRKWOOD_INT_MASK);
@@ -269,11 +265,6 @@ static int kirkwood_i2s_rec_trigger(struct snd_pcm_substream *substream,
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
-		/* stop audio, enable interrupts */
-		value = readl(priv->io + KIRKWOOD_RECCTL);
-		value |= KIRKWOOD_RECCTL_PAUSE;
-		writel(value, priv->io + KIRKWOOD_RECCTL);
-
 		value = readl(priv->io + KIRKWOOD_INT_MASK);
 		value |= KIRKWOOD_INT_CAUSE_REC_BYTES;
 		writel(value, priv->io + KIRKWOOD_INT_MASK);
-- 
1.7.4.4

[PATCH 06/10] ASoC: kirkwood-i2s: use devm_* APIs

[Russell King]

Simplify the cleanup paths in the driver by using the devm_* APIs,
ensuring that all error paths are correctly checked.

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

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 1d5db48..f059f40 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -406,57 +406,47 @@ static __devinit int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 	struct kirkwood_dma_data *priv;
 	int err;
 
-	priv = kzalloc(sizeof(struct kirkwood_dma_data), GFP_KERNEL);
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
 		dev_err(&pdev->dev, "allocation failed\n");
-		err = -ENOMEM;
-		goto error;
+		return -ENOMEM;
 	}
 	dev_set_drvdata(&pdev->dev, priv);
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!mem) {
 		dev_err(&pdev->dev, "platform_get_resource failed\n");
-		err = -ENXIO;
-		goto err_alloc;
+		return -ENXIO;
 	}
 
-	priv->mem = request_mem_region(mem->start, SZ_16K, DRV_NAME);
-	if (!priv->mem) {
-		dev_err(&pdev->dev, "request_mem_region failed\n");
-		err = -EBUSY;
-		goto err_alloc;
-	}
-
-	priv->io = ioremap(priv->mem->start, SZ_16K);
+	priv->io = devm_request_and_ioremap(&pdev->dev, mem);
 	if (!priv->io) {
-		dev_err(&pdev->dev, "ioremap failed\n");
-		err = -ENOMEM;
-		goto err_iomem;
+		dev_err(&pdev->dev, "devm_request_and_ioremap failed\n");
+		return -ENOMEM;
 	}
 
 	priv->irq = platform_get_irq(pdev, 0);
 	if (priv->irq <= 0) {
 		dev_err(&pdev->dev, "platform_get_irq failed\n");
-		err = -ENXIO;
-		goto err_ioremap;
+		return -ENXIO;
 	}
 
 	if (!data) {
 		dev_err(&pdev->dev, "no platform data ?!\n");
-		err = -EINVAL;
-		goto err_ioremap;
+		return -EINVAL;
 	}
 
 	priv->burst = data->burst;
 
-	priv->clk = clk_get(&pdev->dev, NULL);
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		dev_err(&pdev->dev, "no clock\n");
-		err = PTR_ERR(priv->clk);
-		goto err_ioremap;
+		return PTR_ERR(priv->clk);
 	}
-	clk_prepare_enable(priv->clk);
+
+	err = clk_prepare_enable(priv->clk);
+	if (err < 0)
+		return err;
 
 	err = snd_soc_register_dai(&pdev->dev, &kirkwood_i2s_dai);
 	if (!err)
@@ -464,15 +454,7 @@ static __devinit int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 	dev_err(&pdev->dev, "snd_soc_register_dai failed\n");
 
 	clk_disable_unprepare(priv->clk);
-	clk_put(priv->clk);
-
-err_ioremap:
-	iounmap(priv->io);
-err_iomem:
-	release_mem_region(priv->mem->start, SZ_16K);
-err_alloc:
-	kfree(priv);
-error:
+
 	return err;
 }
 
@@ -483,11 +465,6 @@ static __devexit int kirkwood_i2s_dev_remove(struct platform_device *pdev)
 	snd_soc_unregister_dai(&pdev->dev);
 
 	clk_disable_unprepare(priv->clk);
-	clk_put(priv->clk);
-
-	iounmap(priv->io);
-	release_mem_region(priv->mem->start, SZ_16K);
-	kfree(priv);
 
 	return 0;
 }
diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h
index f9084d8..7d92b9e 100644
--- a/sound/soc/kirkwood/kirkwood.h
+++ b/sound/soc/kirkwood/kirkwood.h
@@ -119,7 +119,6 @@
 #define KIRKWOOD_SND_MAX_PERIOD_BYTES		0x4000
 
 struct kirkwood_dma_data {
-	struct resource *mem;
 	void __iomem *io;
 	int irq;
 	int burst;
-- 
1.7.4.4

[PATCH 07/10] ASoC: kirkwood-i2s: better handling of play/record control registers

[Russell King]

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

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index f059f40..823ef1e 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -113,15 +113,14 @@ 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);
-	unsigned int i2s_reg, reg;
-	unsigned long i2s_value, value;
+	uint32_t ctl_play, ctl_rec;
+	unsigned int i2s_reg;
+	unsigned long i2s_value;
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		i2s_reg = KIRKWOOD_I2S_PLAYCTL;
-		reg = KIRKWOOD_PLAYCTL;
 	} else {
 		i2s_reg = KIRKWOOD_I2S_RECCTL;
-		reg = KIRKWOOD_RECCTL;
 	}
 
 	/* set dco conf */
@@ -130,9 +129,6 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
 	i2s_value = readl(priv->io+i2s_reg);
 	i2s_value &= ~KIRKWOOD_I2S_CTL_SIZE_MASK;
 
-	value = readl(priv->io+reg);
-	value &= ~KIRKWOOD_PLAYCTL_SIZE_MASK;
-
 	/*
 	 * Size settings in play/rec i2s control regs and play/rec control
 	 * regs must be the same.
@@ -140,38 +136,57 @@ 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;
-		value |= KIRKWOOD_PLAYCTL_SIZE_16_C;
+		ctl_play = KIRKWOOD_PLAYCTL_SIZE_16_C |
+			   KIRKWOOD_PLAYCTL_I2S_EN;
+		ctl_rec = KIRKWOOD_RECCTL_SIZE_16_C |
+			  KIRKWOOD_RECCTL_I2S_EN;
 		break;
 	/*
 	 * doesn't work... S20_3LE != kirkwood 20bit format ?
 	 *
 	case SNDRV_PCM_FORMAT_S20_3LE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_20;
-		value |= KIRKWOOD_PLAYCTL_SIZE_20;
+		ctl_play = KIRKWOOD_PLAYCTL_SIZE_20 |
+			   KIRKWOOD_PLAYCTL_I2S_EN;
+		ctl_rec = KIRKWOOD_RECCTL_SIZE_20 |
+			  KIRKWOOD_RECCTL_I2S_EN;
 		break;
 	*/
 	case SNDRV_PCM_FORMAT_S24_LE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_24;
-		value |= KIRKWOOD_PLAYCTL_SIZE_24;
+		ctl_play = KIRKWOOD_PLAYCTL_SIZE_24 |
+			   KIRKWOOD_PLAYCTL_I2S_EN;
+		ctl_rec = KIRKWOOD_RECCTL_SIZE_24 |
+			  KIRKWOOD_RECCTL_I2S_EN;
 		break;
 	case SNDRV_PCM_FORMAT_S32_LE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_32;
-		value |= KIRKWOOD_PLAYCTL_SIZE_32;
+		ctl_play = KIRKWOOD_PLAYCTL_SIZE_32 |
+			   KIRKWOOD_PLAYCTL_I2S_EN;
+		ctl_rec = KIRKWOOD_RECCTL_SIZE_32 |
+			  KIRKWOOD_RECCTL_I2S_EN;
 		break;
 	default:
 		return -EINVAL;
 	}
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		value &= ~KIRKWOOD_PLAYCTL_MONO_MASK;
 		if (params_channels(params) == 1)
-			value |= KIRKWOOD_PLAYCTL_MONO_BOTH;
+			ctl_play |= KIRKWOOD_PLAYCTL_MONO_BOTH;
 		else
-			value |= KIRKWOOD_PLAYCTL_MONO_OFF;
+			ctl_play |= KIRKWOOD_PLAYCTL_MONO_OFF;
+
+		priv->ctl_play &= ~(KIRKWOOD_PLAYCTL_MONO_MASK |
+				    KIRKWOOD_PLAYCTL_I2S_EN |
+				    KIRKWOOD_PLAYCTL_SPDIF_EN |
+				    KIRKWOOD_PLAYCTL_SIZE_MASK);
+		priv->ctl_play |= ctl_play;
+	} else {
+		priv->ctl_rec &= ~KIRKWOOD_RECCTL_SIZE_MASK;
+		priv->ctl_rec |= ctl_rec;
 	}
 
 	writel(i2s_value, priv->io+i2s_reg);
-	writel(value, priv->io+reg);
 
 	return 0;
 }
@@ -205,20 +220,18 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream,
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
+		/* configure */
+		ctl = priv->ctl_play;
+		value = ctl & ~(KIRKWOOD_PLAYCTL_I2S_EN |
+				KIRKWOOD_PLAYCTL_SPDIF_EN);
+		writel(value, priv->io + KIRKWOOD_PLAYCTL);
+
+		/* enable interrupts */
 		value = readl(priv->io + KIRKWOOD_INT_MASK);
 		value |= KIRKWOOD_INT_CAUSE_PLAY_BYTES;
 		writel(value, priv->io + KIRKWOOD_INT_MASK);
 
-		/* configure audio & enable i2s playback */
-		ctl &= ~KIRKWOOD_PLAYCTL_BURST_MASK;
-		ctl &= ~(KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE
-				| KIRKWOOD_PLAYCTL_SPDIF_EN);
-
-		if (priv->burst == 32)
-			ctl |= KIRKWOOD_PLAYCTL_BURST_32;
-		else
-			ctl |= KIRKWOOD_PLAYCTL_BURST_128;
-		ctl |= KIRKWOOD_PLAYCTL_I2S_EN;
+		/* enable playback */
 		writel(ctl, priv->io + KIRKWOOD_PLAYCTL);
 		break;
 
@@ -259,30 +272,24 @@ static int kirkwood_i2s_rec_trigger(struct snd_pcm_substream *substream,
 				int cmd, struct snd_soc_dai *dai)
 {
 	struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai);
-	unsigned long value;
+	uint32_t ctl, value;
 
 	value = readl(priv->io + KIRKWOOD_RECCTL);
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
+		/* configure */
+		ctl = priv->ctl_rec;
+		value = ctl & ~KIRKWOOD_RECCTL_I2S_EN;
+		writel(value, priv->io + KIRKWOOD_RECCTL);
+
+		/* enable interrupts */
 		value = readl(priv->io + KIRKWOOD_INT_MASK);
 		value |= KIRKWOOD_INT_CAUSE_REC_BYTES;
 		writel(value, priv->io + KIRKWOOD_INT_MASK);
 
-		/* configure audio & enable i2s record */
-		value = readl(priv->io + KIRKWOOD_RECCTL);
-		value &= ~KIRKWOOD_RECCTL_BURST_MASK;
-		value &= ~KIRKWOOD_RECCTL_MONO;
-		value &= ~(KIRKWOOD_RECCTL_PAUSE | KIRKWOOD_RECCTL_MUTE
-			| KIRKWOOD_RECCTL_SPDIF_EN);
-
-		if (priv->burst == 32)
-			value |= KIRKWOOD_RECCTL_BURST_32;
-		else
-			value |= KIRKWOOD_RECCTL_BURST_128;
-		value |= KIRKWOOD_RECCTL_I2S_EN;
-
-		writel(value, priv->io + KIRKWOOD_RECCTL);
+		/* enable record */
+		writel(ctl, priv->io + KIRKWOOD_RECCTL);
 		break;
 
 	case SNDRV_PCM_TRIGGER_STOP:
@@ -448,6 +455,31 @@ static __devinit int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 	if (err < 0)
 		return err;
 
+	priv->extclk = clk_get(&pdev->dev, "extclk");
+	if (!IS_ERR(priv->extclk)) {
+		if (priv->extclk == priv->clk) {
+			clk_put(priv->extclk);
+			priv->extclk = ERR_PTR(-EINVAL);
+		} else {
+			dev_info(&pdev->dev, "found external clock\n");
+			clk_prepare_enable(priv->extclk);
+			soc_dai = &kirkwood_i2s_dai_extclk;
+		}
+	}
+
+	/* Some sensible defaults - this reflects the powerup values */
+	priv->ctl_play = KIRKWOOD_PLAYCTL_SIZE_24;
+	priv->ctl_rec = KIRKWOOD_RECCTL_SIZE_24;
+
+	/* Select the burst size */
+	if (data->burst == 32) {
+		priv->ctl_play |= KIRKWOOD_PLAYCTL_BURST_32;
+		priv->ctl_rec |= KIRKWOOD_RECCTL_BURST_32;
+	} else {
+		priv->ctl_play |= KIRKWOOD_PLAYCTL_BURST_128;
+		priv->ctl_rec |= KIRKWOOD_RECCTL_BURST_128;
+	}
+
 	err = snd_soc_register_dai(&pdev->dev, &kirkwood_i2s_dai);
 	if (!err)
 		return 0;
diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h
index 7d92b9e..6e3b14a 100644
--- a/sound/soc/kirkwood/kirkwood.h
+++ b/sound/soc/kirkwood/kirkwood.h
@@ -120,6 +120,8 @@
 
 struct kirkwood_dma_data {
 	void __iomem *io;
+	uint32_t ctl_play;
+	uint32_t ctl_rec;
 	int irq;
 	int burst;
 	struct clk *clk;
-- 
1.7.4.4

[PATCH 08/10] ASoC: kirkwood-dma: remove restriction on sample rates

[Russell King]

This is part of a patch found in Rabeeh Khoury's git tree for the
cubox.

The kirkwood DMA hardware for ASoC does not impose any restrictions
on the sample rates available, so it's silly to impose an artificial
set in the DMA code.  The restrictions come from the availble clocks
to the I2S module, which are already handled in the I2S part of the
driver.

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

diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c
index 2ba0814..73c90c6 100644
--- a/sound/soc/kirkwood/kirkwood-dma.c
+++ b/sound/soc/kirkwood/kirkwood-dma.c
@@ -22,8 +22,10 @@
 #include "kirkwood.h"
 
 #define KIRKWOOD_RATES \
-	(SNDRV_PCM_RATE_44100 | \
-	 SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000)
+	(SNDRV_PCM_RATE_8000_192000 |		\
+	 SNDRV_PCM_RATE_CONTINUOUS |		\
+	 SNDRV_PCM_RATE_KNOT)
+
 #define KIRKWOOD_FORMATS \
 	(SNDRV_PCM_FMTBIT_S16_LE | \
 	 SNDRV_PCM_FMTBIT_S24_LE | \
@@ -43,8 +45,8 @@ static struct snd_pcm_hardware kirkwood_dma_snd_hw = {
 		 SNDRV_PCM_INFO_PAUSE),
 	.formats		= KIRKWOOD_FORMATS,
 	.rates			= KIRKWOOD_RATES,
-	.rate_min		= 44100,
-	.rate_max		= 96000,
+	.rate_min		= 8000,
+	.rate_max		= 384000,
 	.channels_min		= 1,
 	.channels_max		= 2,
 	.buffer_bytes_max	= KIRKWOOD_SND_MAX_PERIOD_BYTES * KIRKWOOD_SND_MAX_PERIODS,
-- 
1.7.4.4

[PATCH 09/10] ASoC: kirkwood-i2s: add support for external clock rates

[Russell King]

This is part of a patch found in Rabeeh Khoury's git tree for the
cubox, and cleaned up by me.

Some platforms provide an external clock which can be used to allow
other sample rates to be selected.  Provide support for this.

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

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 823ef1e..d3629d5 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -99,6 +99,29 @@ static inline void kirkwood_set_dco(void __iomem *io, unsigned long rate)
 	} while (value == 0);
 }
 
+static void kirkwood_set_rate(struct snd_soc_dai *dai,
+	struct kirkwood_dma_data *priv, unsigned long rate)
+{
+	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)) {
+		/* 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;
+	}
+	writel(clks_ctrl, priv->io + KIRKWOOD_CLOCKS_CTRL);
+}
+
 static int kirkwood_i2s_startup(struct snd_pcm_substream *substream,
 		struct snd_soc_dai *dai)
 {
@@ -123,8 +146,7 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
 		i2s_reg = KIRKWOOD_I2S_RECCTL;
 	}
 
-	/* set dco conf */
-	kirkwood_set_dco(priv->io, params_rate(params));
+	kirkwood_set_rate(dai, priv, params_rate(params));
 
 	i2s_value = readl(priv->io+i2s_reg);
 	i2s_value &= ~KIRKWOOD_I2S_CTL_SIZE_MASK;
@@ -396,21 +418,45 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai = {
 		.channels_min = 1,
 		.channels_max = 2,
 		.rates = KIRKWOOD_I2S_RATES,
-		.formats = KIRKWOOD_I2S_FORMATS,},
+		.formats = KIRKWOOD_I2S_FORMATS,
+	},
 	.capture = {
 		.channels_min = 1,
 		.channels_max = 2,
 		.rates = KIRKWOOD_I2S_RATES,
-		.formats = KIRKWOOD_I2S_FORMATS,},
+		.formats = KIRKWOOD_I2S_FORMATS,
+	},
+	.ops = &kirkwood_i2s_dai_ops,
+};
+
+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,
+	},
 	.ops = &kirkwood_i2s_dai_ops,
 };
 
 static __devinit int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 {
-	struct resource *mem;
-	struct kirkwood_asoc_platform_data *data =
-		pdev->dev.platform_data;
+	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;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -480,11 +526,15 @@ static __devinit int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 		priv->ctl_rec |= KIRKWOOD_RECCTL_BURST_128;
 	}
 
-	err = snd_soc_register_dai(&pdev->dev, &kirkwood_i2s_dai);
+	err = snd_soc_register_dai(&pdev->dev, soc_dai);
 	if (!err)
 		return 0;
 	dev_err(&pdev->dev, "snd_soc_register_dai failed\n");
 
+	if (!IS_ERR(priv->extclk)) {
+		clk_disable_unprepare(priv->extclk);
+		clk_put(priv->extclk);
+	}
 	clk_disable_unprepare(priv->clk);
 
 	return err;
@@ -496,6 +546,10 @@ static __devexit int kirkwood_i2s_dev_remove(struct platform_device *pdev)
 
 	snd_soc_unregister_dai(&pdev->dev);
 
+	if (!IS_ERR(priv->extclk)) {
+		clk_disable_unprepare(priv->extclk);
+		clk_put(priv->extclk);
+	}
 	clk_disable_unprepare(priv->clk);
 
 	return 0;
diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h
index 6e3b14a..4d92637 100644
--- a/sound/soc/kirkwood/kirkwood.h
+++ b/sound/soc/kirkwood/kirkwood.h
@@ -77,6 +77,11 @@
 #define KIRKWOOD_DCO_SPCR_STATUS		0x120c
 #define KIRKWOOD_DCO_SPCR_STATUS_DCO_LOCK	(1<<16)
 
+#define KIRKWOOD_CLOCKS_CTRL			0x1230
+#define KIRKWOOD_MCLK_SOURCE_MASK		(3<<0)
+#define KIRKWOOD_MCLK_SOURCE_DCO		(0<<0)
+#define KIRKWOOD_MCLK_SOURCE_EXTCLK		(3<<0)
+
 #define KIRKWOOD_ERR_CAUSE			0x1300
 #define KIRKWOOD_ERR_MASK			0x1304
 
@@ -120,11 +125,12 @@
 
 struct kirkwood_dma_data {
 	void __iomem *io;
+	struct clk *clk;
+	struct clk *extclk;
 	uint32_t ctl_play;
 	uint32_t ctl_rec;
 	int irq;
 	int burst;
-	struct clk *clk;
 };
 
 #endif
-- 
1.7.4.4

Re: [PATCH 0/10] Kirkwood ASoC drivers fixes and improvements

[Russell King - ARM Linux]

Argh.  Just noticed the driver author is _not_ listed in MAINTAINTERS.
I guess the driver author doesn't care about receiving patches for his
driver then...  If Arnaud does care then that needs to be fixed.  I will
not be spamming everyone again with this patch set just because someone
can't be bothered to ensure that they have appropriate MAINTAINERS entries.
I'm sure they can be dug out of the alsa-devel archives instead.

On Tue, Nov 20, 2012 at 12:17:26PM +0000, Russell King - ARM Linux wrote:
> The first five patches of this patch series fixes a number of problems
> with the Kirkwood audio driver, as seen on Dove platforms.  The biggest
> issue is that they can cause the kernel to lock up solidly, requiring
> a reset to recover.
> 
> Other issues included in the first six patches are bugs found by the
> cubox folk.
> 
> The other five patches convert the I2S driver to use the devm_* APIs,
> improve the handling of the record and playback control registers so
> we're not reading and writing them as often, lift restrictions in the
> DMA driver which aren't necessary to impose, and add support for
> external clocks so that sample rates other than 44.1, 48 and 96kHz
> can be supported.  Some of these changes prepare the driver to support
> the SPDIF output including passthrough mode; however, that support is
> not included in this patch set.
> 
> These patches probably need checking out on Kirkwood before they go
> into mainline, but given the severity of the kernel lockup, I suggest
> that once tested the first five patches go into -rc.
> 
>  sound/soc/kirkwood/kirkwood-dma.c |   19 ++-
>  sound/soc/kirkwood/kirkwood-i2s.c |  291 ++++++++++++++++++++++---------------
>  sound/soc/kirkwood/kirkwood.h     |   11 +-
>  3 files changed, 197 insertions(+), 124 deletions(-)
> 
>  ASoC: kirkwood-dma: remove channel restrictions
>  ASoC: kirkwood-i2s: add support for external clock rates
>  ASoC: kirkwood-dma: remove restriction on sample rates
>  ASoC: kirkwood-i2s: better handling of play/record control registers
>  ASoC: kirkwood-i2s: use devm_* APIs
>  ASoC: kirkwood-i2s: more pause-mode fixes
>  ASoC: kirkwood-i2s: fix DMA underruns
>  ASoC: kirkwood-i2s: fix DCO lock detection
>  ASoC: kirkwood-dma: don't ignore other irq causes on error
>  ASoC: kirkwood-dma: fix use of virt_to_phys()
> 

Re: [PATCH 0/10] Kirkwood ASoC drivers fixes and improvements

[Russell King - ARM Linux]

All,

I'm opening this to a wider audience in the hope of getting a solution.

There are classes of hardware out there where the DMA support is tightly
integrated along side the rest of the I2S interface.

One such example of this is the hardware found on Marvell Kirkwood
platforms and their derivatives.   Not only are the registers interleaved
between the I2S interface part and the DMA part, and the DMA can only be
used for I2S, but the current Kirkwood ASoC implementation in
sound/soc/kirkwood shares a common data structure created by the "cpu dai"
to provide register access and other data.  This data structure is
"struct kirkwood_dma_data".

This means that in software, as things stand today, kirkwood-dma.c has
intimate knowledge of kirkwood-i2s.c despite being two separate platform
drivers.  This also means that the split between kirkwood-dma.c and
kirkwood-i2s.c is entirely artificial, brought about by the insistance
of ASoC that these things shall be separate.

How does ASoC insist that these be separate?  It only provides the
necessary callbacks for DMA related stuff to what it calls the "platform"
driver.  The interface part is called a "cpu dai" and one of those must
always be provided.  Each of these must be a separate platform device,
because they're each named drivers, and the name comes from the
device/driver.

This is fine in the pre-DT world, where we have board files in arch/arm
which can create whatever platform devices are needed to bring ASoC up,
but we're moving to a DT-only solution - that means no board files.
DT is a hardware description; it does not describe whatever random ideas
some software implementation has come up with; a DT description of audio
on Kirkwood will not make any distinction between the artificial
"platform" part and the "cpu dai" part.

Instead, a DT description will just declare there to be one I2S device
with its relevant resources.

Such a description is _inherently_ incompatible with ASoC as long as
ASoC insists that there is this artificial distinction.

What Mark is telling me is that he requires yet more board files to
spring up under sound/soc/ which create these artificial platform
devices.  Not only does that go against the direction which we're heading
on ARM (at Linus' insistance) to get rid of board files, but it perpetuates
this silly idea that every audio interface should be split up whether
there's a distinction there or not.

It also makes the handling of -EPROBE_DEFER yet more complex; using this
driver on the Dove Cubox platform with built as a set of modules revealed
that where a platform device was being registered by code in
sound/soc/kirkwood, and would be immediately unregistered on -EPROBE_DEFER
never to be re-registered.

It is my belief that Mark's position is not the right solution for where
things are heading.

We need to have solutions which do not require artificial breakup of
drivers; we need solutions where hardware can be described by DT and
that DT description be used by the kernel with the minimum of code.
What we don't need are yet more board files appearing in some other
random part of the kernel tree.

Your thoughts please.

[PATCH 0/10] Kirkwood ASoC drivers fixes and improvements

[Russell King - ARM Linux]

All,

I'm opening this to a wider audience in the hope of getting a solution.

There are classes of hardware out there where the DMA support is tightly
integrated along side the rest of the I2S interface.

One such example of this is the hardware found on Marvell Kirkwood
platforms and their derivatives.   Not only are the registers interleaved
between the I2S interface part and the DMA part, and the DMA can only be
used for I2S, but the current Kirkwood ASoC implementation in
sound/soc/kirkwood shares a common data structure created by the "cpu dai"
to provide register access and other data.  This data structure is
"struct kirkwood_dma_data".

This means that in software, as things stand today, kirkwood-dma.c has
intimate knowledge of kirkwood-i2s.c despite being two separate platform
drivers.  This also means that the split between kirkwood-dma.c and
kirkwood-i2s.c is entirely artificial, brought about by the insistance
of ASoC that these things shall be separate.

How does ASoC insist that these be separate?  It only provides the
necessary callbacks for DMA related stuff to what it calls the "platform"
driver.  The interface part is called a "cpu dai" and one of those must
always be provided.  Each of these must be a separate platform device,
because they're each named drivers, and the name comes from the
device/driver.

This is fine in the pre-DT world, where we have board files in arch/arm
which can create whatever platform devices are needed to bring ASoC up,
but we're moving to a DT-only solution - that means no board files.
DT is a hardware description; it does not describe whatever random ideas
some software implementation has come up with; a DT description of audio
on Kirkwood will not make any distinction between the artificial
"platform" part and the "cpu dai" part.

Instead, a DT description will just declare there to be one I2S device
with its relevant resources.

Such a description is _inherently_ incompatible with ASoC as long as
ASoC insists that there is this artificial distinction.

What Mark is telling me is that he requires yet more board files to
spring up under sound/soc/ which create these artificial platform
devices.  Not only does that go against the direction which we're heading
on ARM (at Linus' insistance) to get rid of board files, but it perpetuates
this silly idea that every audio interface should be split up whether
there's a distinction there or not.

It also makes the handling of -EPROBE_DEFER yet more complex; using this
driver on the Dove Cubox platform with built as a set of modules revealed
that where a platform device was being registered by code in
sound/soc/kirkwood, and would be immediately unregistered on -EPROBE_DEFER
never to be re-registered.

It is my belief that Mark's position is not the right solution for where
things are heading.

We need to have solutions which do not require artificial breakup of
drivers; we need solutions where hardware can be described by DT and
that DT description be used by the kernel with the minimum of code.
What we don't need are yet more board files appearing in some other
random part of the kernel tree.

Your thoughts please.

Re: [PATCH 0/10] Kirkwood ASoC drivers fixes and improvements

[Mark Brown]

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

On Thu, Nov 22, 2012 at 02:06:14PM +0000, Russell King - ARM Linux wrote:

> Instead, a DT description will just declare there to be one I2S device
> with its relevant resources.

Yes, this is exactly the sort of thing all the existing platforms
using DT are doing in their ASoC drivers - there's normally some
grouping or sharing at the hardware level that doesn't map exactly onto
Linux models.  We're already at the point you're trying to get to here.

> Such a description is _inherently_ incompatible with ASoC as long as
> ASoC insists that there is this artificial distinction.

> What Mark is telling me is that he requires yet more board files to
> spring up under sound/soc/ which create these artificial platform
> devices.  Not only does that go against the direction which we're heading

What I said was that you should just register everything from the DT
nodes that describe the hardware and not create dummy devices in the DT
for the Linux internals.  The drivers instantiated from DT should take
care of mapping the hardware into Linux, instantiating everything they
need to from code.  The approach the existing drivers in mainline are
taking is to register multiple ASoC functions from a single device model
device which seems like the logical approach to doing the mapping to me.

Some of the remarks you've made on IRC and code you've pointed me at
suggest that you have formed the impression that there needs to be a 1:1
mapping between device model devices and ASoC function drivers.  This is
not the case.  A driver for a device model device can register as many
different ASoC functions of whatever type it sees fit from a single
device model device.

> on ARM (at Linus' insistance) to get rid of board files, but it perpetuates
> this silly idea that every audio interface should be split up whether
> there's a distinction there or not.

I'm not entirely convinced that modelling different areas of
functionality with different ops structures (which is essentially what
is happening here) is a massive design flaw.

> We need to have solutions which do not require artificial breakup of
> drivers; we need solutions where hardware can be described by DT and
> that DT description be used by the kernel with the minimum of code.
> What we don't need are yet more board files appearing in some other
> random part of the kernel tree.

This is the current situation, none of the existing DT using platforms
have had any need to do that.  Nothing about Kirkwood audio seems to be
at all unusual here.

We *do* have board files for the linkage between the various components
since (as discussed previously ad nauseum) a lot of embedded audio
hardware is interesting enough to warrant having a driver itself.  There
has been some work on generic drivers for simpler systems which should
be useful and can be built on, though we do need the problems with the
clock framework availability to be addressed (both getting it available
on all platforms and ensuring that the platforms with custom frameworks
move to the genric one).  These drivers are separate to the issue you
are discussing.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[PATCH 0/10] Kirkwood ASoC drivers fixes and improvements

[Mark Brown]

On Thu, Nov 22, 2012 at 02:06:14PM +0000, Russell King - ARM Linux wrote:

> Instead, a DT description will just declare there to be one I2S device
> with its relevant resources.

Yes, this is exactly the sort of thing all the existing platforms
using DT are doing in their ASoC drivers - there's normally some
grouping or sharing at the hardware level that doesn't map exactly onto
Linux models.  We're already at the point you're trying to get to here.

> Such a description is _inherently_ incompatible with ASoC as long as
> ASoC insists that there is this artificial distinction.

> What Mark is telling me is that he requires yet more board files to
> spring up under sound/soc/ which create these artificial platform
> devices.  Not only does that go against the direction which we're heading

What I said was that you should just register everything from the DT
nodes that describe the hardware and not create dummy devices in the DT
for the Linux internals.  The drivers instantiated from DT should take
care of mapping the hardware into Linux, instantiating everything they
need to from code.  The approach the existing drivers in mainline are
taking is to register multiple ASoC functions from a single device model
device which seems like the logical approach to doing the mapping to me.

Some of the remarks you've made on IRC and code you've pointed me at
suggest that you have formed the impression that there needs to be a 1:1
mapping between device model devices and ASoC function drivers.  This is
not the case.  A driver for a device model device can register as many
different ASoC functions of whatever type it sees fit from a single
device model device.

> on ARM (at Linus' insistance) to get rid of board files, but it perpetuates
> this silly idea that every audio interface should be split up whether
> there's a distinction there or not.

I'm not entirely convinced that modelling different areas of
functionality with different ops structures (which is essentially what
is happening here) is a massive design flaw.

> We need to have solutions which do not require artificial breakup of
> drivers; we need solutions where hardware can be described by DT and
> that DT description be used by the kernel with the minimum of code.
> What we don't need are yet more board files appearing in some other
> random part of the kernel tree.

This is the current situation, none of the existing DT using platforms
have had any need to do that.  Nothing about Kirkwood audio seems to be
at all unusual here.

We *do* have board files for the linkage between the various components
since (as discussed previously ad nauseum) a lot of embedded audio
hardware is interesting enough to warrant having a driver itself.  There
has been some work on generic drivers for simpler systems which should
be useful and can be built on, though we do need the problems with the
clock framework availability to be addressed (both getting it available
on all platforms and ensuring that the platforms with custom frameworks
move to the genric one).  These drivers are separate to the issue you
are discussing.
-------------- 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/20121123/c35b8c7f/attachment.sig>

Re: [PATCH 0/10] Kirkwood ASoC drivers fixes and improvements

[Mark Brown]

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

On Thu, Nov 22, 2012 at 04:29:22PM +0100, Arnaud Patard wrote:
> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> > Argh.  Just noticed the driver author is _not_ listed in MAINTAINTERS.

> hm. right. sorry. I tend to always forget this. Is this still really
> usefull given that there's get_maintainer.pl ?

Yes, get_maintainer.pl is far too prone to problems like false positives
and other issues when it guesses based on git - real information helps
it a lot.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[PATCH 10/10] ASoC: kirkwood-dma: remove channel restrictions

[Russell King]

This is part of a patch found in Rabeeh Khoury's git tree for the
cubox.

With SPDIF passthrough, we are not restricted to just two channels of
audio; we can support however many channels the non-audio stream can
itself support.  In any case, kirkwood-dma is not involved in the
format selection.  So yet rid of this restriction.

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

diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c
index 73c90c6..58d5a96 100644
--- a/sound/soc/kirkwood/kirkwood-dma.c
+++ b/sound/soc/kirkwood/kirkwood-dma.c
@@ -29,7 +29,9 @@
 #define KIRKWOOD_FORMATS \
 	(SNDRV_PCM_FMTBIT_S16_LE | \
 	 SNDRV_PCM_FMTBIT_S24_LE | \
-	 SNDRV_PCM_FMTBIT_S32_LE)
+	 SNDRV_PCM_FMTBIT_S32_LE | \
+	 SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE | \
+	 SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE)
 
 struct kirkwood_dma_priv {
 	struct snd_pcm_substream *play_stream;
@@ -48,7 +50,7 @@ static struct snd_pcm_hardware kirkwood_dma_snd_hw = {
 	.rate_min		= 8000,
 	.rate_max		= 384000,
 	.channels_min		= 1,
-	.channels_max		= 2,
+	.channels_max		= 8,
 	.buffer_bytes_max	= KIRKWOOD_SND_MAX_PERIOD_BYTES * KIRKWOOD_SND_MAX_PERIODS,
 	.period_bytes_min	= KIRKWOOD_SND_MIN_PERIOD_BYTES,
 	.period_bytes_max	= KIRKWOOD_SND_MAX_PERIOD_BYTES,
-- 
1.7.4.4

Re: [PATCH 0/10] Kirkwood ASoC drivers fixes and improvements

[Mark Brown]

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

On Tue, Nov 20, 2012 at 12:17:26PM +0000, Russell King - ARM Linux wrote:
> The first five patches of this patch series fixes a number of problems
> with the Kirkwood audio driver, as seen on Dove platforms.  The biggest
> issue is that they can cause the kernel to lock up solidly, requiring
> a reset to recover.

Applied all, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

Re: [PATCH 0/10] Kirkwood ASoC drivers fixes and improvements

[Russell King - ARM Linux]

On Wed, Nov 21, 2012 at 10:40:45AM +0900, Mark Brown wrote:
> On Tue, Nov 20, 2012 at 12:17:26PM +0000, Russell King - ARM Linux wrote:
> > The first five patches of this patch series fixes a number of problems
> > with the Kirkwood audio driver, as seen on Dove platforms.  The biggest
> > issue is that they can cause the kernel to lock up solidly, requiring
> > a reset to recover.
> 
> Applied all, thanks.

I've been discussing this with Sebastian Hesselbarth and he's agreed
to have his sign-off on a bunch of these patches... I guess not now.

Re: [PATCH 0/10] Kirkwood ASoC drivers fixes and improvements

[Mark Brown]

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

On Wed, Nov 21, 2012 at 09:41:42AM +0000, Russell King - ARM Linux wrote:

> I've been discussing this with Sebastian Hesselbarth and he's agreed
> to have his sign-off on a bunch of these patches... I guess not now.

It's on a separate branch which I've not produced a signed tag for so I
can rebase and add if you/he send me something.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

Re: [PATCH 0/10] Kirkwood ASoC drivers fixes and improvements

[Takashi Iwai]

At Wed, 21 Nov 2012 18:47:21 +0900,
Mark Brown wrote:
> 
> On Wed, Nov 21, 2012 at 09:41:42AM +0000, Russell King - ARM Linux wrote:
> 
> > I've been discussing this with Sebastian Hesselbarth and he's agreed
> > to have his sign-off on a bunch of these patches... I guess not now.
> 
> It's on a separate branch which I've not produced a signed tag for so I
> can rebase and add if you/he send me something.

Hm, isn't a branch/tag I already pulled in this morning?


Takashi

Re: [PATCH 0/10] Kirkwood ASoC drivers fixes and improvements

[Mark Brown]

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

On Wed, Nov 21, 2012 at 10:59:49AM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > It's on a separate branch which I've not produced a signed tag for so I
> > can rebase and add if you/he send me something.

> Hm, isn't a branch/tag I already pulled in this morning?

Gah, right - sorry.  Half of it is in what you pulled, half isn't.  The
extra stuff I can still fix up.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

Re: [PATCH 0/10] Kirkwood ASoC drivers fixes and improvements

[Russell King - ARM Linux]

On Wed, Nov 21, 2012 at 06:47:21PM +0900, Mark Brown wrote:
> On Wed, Nov 21, 2012 at 09:41:42AM +0000, Russell King - ARM Linux wrote:
> 
> > I've been discussing this with Sebastian Hesselbarth and he's agreed
> > to have his sign-off on a bunch of these patches... I guess not now.
> 
> It's on a separate branch which I've not produced a signed tag for so I
> can rebase and add if you/he send me something.

That's a good idea; I did say that the patches should be tested on
Kirkwood first.  Also, Sebastian has been doing his own independent
testing of them last night... unsuccessfully from what I read on IRC,
but that may not be due to the patches themselves but the amount of
churn going on with the Dove architecture having broken something else
in the clk stuff.

So you might get some tested-by's at some point, or you might be asked
to hold off on them.

Also remember that Arnaud hasn't seen these patches (unless they got
through the alsa mailing list moderation and he's looked them up in
the archives.)