[alsa-devel] [PATCH 00/14] SPDIF support
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(-)
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; };
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;
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 &&
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
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
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;
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",
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); }
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;}
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.
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.
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) {
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.
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;
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)
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",
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), };
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.
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.
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.
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.
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.
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)
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
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.
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.
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
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;
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.
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",@@ -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",
...this file does not appear to be in mainline and some of the rest of the context suggested this was based off something old.
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",@@ -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",...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.
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.
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).
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.
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:
- 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.
- 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.
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:
- 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.
- 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.
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.
- 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.
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?
- 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.
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.
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.
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.
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; }
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
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.)
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:
- there is no guarantee that they couldn't be used at the same time.
- 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
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:
- there is no guarantee that they couldn't be used at the same time.
- 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;
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:
- there is no guarantee that they couldn't be used at the same time.
- 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:
- It is my understanding that the startup() callback will be called when the ALSA control device is opened (/dev/snd/controlC*).
Yes.
- 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;case SNDRV_PCM_FORMAT_S20_3LE: i2s_value |= KIRKWOOD_I2S_CTL_SIZE_20;
- doesn't work... S20_3LE != kirkwood 20bit format ?
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;
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;
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;@@ -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_rec &= ~KIRKWOOD_RECCTL_SIZE_MASK;
priv->ctl_rec &= ~(KIRKWOOD_RECCTL_SIZE_MASK |ctl_rec_mask);@@ -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;@@ -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);@@ -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);@@ -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;
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:
- there is no guarantee that they couldn't be used at the same time.
- 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.
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".
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.
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.
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.
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.
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.
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.)
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.
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
participants (5)
-
Jean-Francois Moine -
Lars-Peter Clausen -
Mark Brown -
Russell King -
Russell King - ARM Linux