[alsa-devel] [PATCH RFC 00/13] Adding SPDIF support to kirkwood-i2s
This is only a RFC at present due to the need to test on Kirkwood and fix at least one bug in ASoC core code. I can't say that this stuff is fully tested, because I don't have any Kirkwood platforms, but I do have one Dove platform, which as I've said in the past doesn't run mainline. I don't run DT on it, so these patches also don't contain anything DT related. However, this patch set is aimed at sorting out the SPDIF on Dove and Kirkwood, which the Dove-based Cubox uses. The first patch in the series I've already sent, and I think Mark may have already picked up. The second is my implementation of what I think Mark committed to use the devm_* APIs for the external clock. Patches 3 and 4 fix problems which I've found with the ASoC core code, which prevent these patches from working correctly. Patch 3 I've already discussed with Mark over the weekend, and I think that's pretty much settled, but patch 4 is a definite hack. Patch 4 addresses an issue where we end up registering the CPU DAIs stream widgets twice - once against the codec DAPM context and again against the platform DAPM context. When these are registered, pointers to these widgets are saved into the CPU DAI structure, so one set of these widgets gets overwritten by the other set. The set which wins (normally the ones in the CPU DAPM context) ends up being the set which are made active. However, the platform ones remain on the DAI list, and these can end up being bound to by routes. ASoC people need to fix this problem as a pre-requisit to the following patches working. Patches 5 and 6 are just cleanups - I decided it was easier to do my own version of merging the I2S and DMA files rather than using Jean-F's patch (sorry, I just wanted something that worked...) Patch 7 is based on some changes in Rabeeh's kernel - I will have a following patch at some point to adjust these parameters to something a little saner. This just gets mainline closer to the tree which I can test. Patch 8 adds the I2S widgets to kirkwood-i2s and uses them. With this patch applied, kirkwood-i2s will be silent unless at least one of the output widgets is "powered up" - in the case of this patch, that's the "i2sdo" widget - naming following what's in the functional spec. Patch 9 and 10 add the DAPM routes to connect the I2S inputs/outputs to the codec streams, restoring audio output on these platforms. As I don't have these platforms, this should be tested by someone who has. Patch 11, again, discussed with Mark, this adds an always enabled output pin for the SPDIF transceiver, so that the output is powered up by DAPM. Without this, the SPDIF output stream goes into "active" mode during playback, but is never powered up. This results in any connected source widgets also not being powered up. Patch 12 adds the SPDIF output widget to kirkwood-i2s, called "spdifdo", again a name derived from the functional spec. The actual pin is called "AU_SPDIFDO". Patch 13 adds the IEC958 channel status data to patch 12, allowing the channel status to be set by applications using an appropriate ALSA library configuration file via the standard methods. What I haven't included is the board file (like the openrd and t5325 ones already present) for SPDIF support on Dove, as that's based on the pre-DT setups, and I'm not sure anyone in mainline is interested in that. So the above provides the components necessary, hopefully someone with ASoC & DT knowledge can bind these components together via appropriate DT bindings. For reference, the DAI link looks like this: .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", and the required route: static const struct snd_soc_dapm_route routes[] = { { "Playback", NULL, "spdifdo" }, };
Merge these two structures together; nothing other than the I2S and DMA driver makes use of struct kirkwood_dma_data, and it's not like struct kirkwood_dma_data is really just used to convey DMA specific data to the backend; it's more a general shared structure between the two halves. This will later allow kirkwood-dma.c and kirkwood-i2s.c to be merged together. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- sound/soc/kirkwood/kirkwood-dma.c | 76 +++++++++++-------------------------- sound/soc/kirkwood/kirkwood.h | 2 + 2 files changed, 24 insertions(+), 54 deletions(-) diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c index d3d4bdc..112c262 100644 --- a/sound/soc/kirkwood/kirkwood-dma.c +++ b/sound/soc/kirkwood/kirkwood-dma.c @@ -33,11 +33,11 @@ SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE | \ SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE) -struct kirkwood_dma_priv { - struct snd_pcm_substream *play_stream; - struct snd_pcm_substream *rec_stream; - struct kirkwood_dma_data *data; -}; +static struct kirkwood_dma_data *kirkwood_priv(struct snd_pcm_substream *subs) +{ + struct snd_soc_pcm_runtime *soc_runtime = subs->private_data; + return snd_soc_dai_get_drvdata(soc_runtime->cpu_dai); +} static struct snd_pcm_hardware kirkwood_dma_snd_hw = { .info = (SNDRV_PCM_INFO_INTERLEAVED | @@ -63,8 +63,7 @@ static u64 kirkwood_dma_dmamask = DMA_BIT_MASK(32); static irqreturn_t kirkwood_dma_irq(int irq, void *dev_id) { - struct kirkwood_dma_priv *prdata = dev_id; - struct kirkwood_dma_data *priv = prdata->data; + struct kirkwood_dma_data *priv = dev_id; unsigned long mask, status, cause; mask = readl(priv->io + KIRKWOOD_INT_MASK); @@ -89,10 +88,10 @@ static irqreturn_t kirkwood_dma_irq(int irq, void *dev_id) writel(status, priv->io + KIRKWOOD_INT_CAUSE); if (status & KIRKWOOD_INT_CAUSE_PLAY_BYTES) - snd_pcm_period_elapsed(prdata->play_stream); + snd_pcm_period_elapsed(priv->substream_play); if (status & KIRKWOOD_INT_CAUSE_REC_BYTES) - snd_pcm_period_elapsed(prdata->rec_stream); + snd_pcm_period_elapsed(priv->substream_rec); return IRQ_HANDLED; } @@ -126,15 +125,10 @@ static int kirkwood_dma_open(struct snd_pcm_substream *substream) { int err; struct snd_pcm_runtime *runtime = substream->runtime; - struct snd_soc_pcm_runtime *soc_runtime = substream->private_data; - struct snd_soc_platform *platform = soc_runtime->platform; - struct snd_soc_dai *cpu_dai = soc_runtime->cpu_dai; - struct kirkwood_dma_data *priv; - struct kirkwood_dma_priv *prdata = snd_soc_platform_get_drvdata(platform); + struct kirkwood_dma_data *priv = kirkwood_priv(substream); const struct mbus_dram_target_info *dram; unsigned long addr; - priv = snd_soc_dai_get_dma_data(cpu_dai, substream); snd_soc_set_runtime_hwparams(substream, &kirkwood_dma_snd_hw); /* Ensure that all constraints linked to dma burst are fulfilled */ @@ -157,21 +151,11 @@ static int kirkwood_dma_open(struct snd_pcm_substream *substream) if (err < 0) return err; - if (prdata == NULL) { - prdata = kzalloc(sizeof(struct kirkwood_dma_priv), GFP_KERNEL); - if (prdata == NULL) - return -ENOMEM; - - prdata->data = priv; - + if (!priv->substream_play && !priv->substream_rec) { err = request_irq(priv->irq, kirkwood_dma_irq, IRQF_SHARED, - "kirkwood-i2s", prdata); - if (err) { - kfree(prdata); + "kirkwood-i2s", priv); + if (err) return -EBUSY; - } - - snd_soc_platform_set_drvdata(platform, prdata); /* * Enable Error interrupts. We're only ack'ing them but @@ -183,11 +167,11 @@ static int kirkwood_dma_open(struct snd_pcm_substream *substream) dram = mv_mbus_dram_info(); addr = substream->dma_buffer.addr; if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { - prdata->play_stream = substream; + priv->substream_play = substream; kirkwood_dma_conf_mbus_windows(priv->io, KIRKWOOD_PLAYBACK_WIN, addr, dram); } else { - prdata->rec_stream = substream; + priv->substream_rec = substream; kirkwood_dma_conf_mbus_windows(priv->io, KIRKWOOD_RECORD_WIN, addr, dram); } @@ -197,27 +181,19 @@ static int kirkwood_dma_open(struct snd_pcm_substream *substream) static int kirkwood_dma_close(struct snd_pcm_substream *substream) { - struct snd_soc_pcm_runtime *soc_runtime = substream->private_data; - struct snd_soc_dai *cpu_dai = soc_runtime->cpu_dai; - struct snd_soc_platform *platform = soc_runtime->platform; - struct kirkwood_dma_priv *prdata = snd_soc_platform_get_drvdata(platform); - struct kirkwood_dma_data *priv; - - priv = snd_soc_dai_get_dma_data(cpu_dai, substream); + struct kirkwood_dma_data *priv = kirkwood_priv(substream); - if (!prdata || !priv) + if (!priv) return 0; if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - prdata->play_stream = NULL; + priv->substream_play = NULL; else - prdata->rec_stream = NULL; + priv->substream_rec = NULL; - if (!prdata->play_stream && !prdata->rec_stream) { + if (!priv->substream_play && !priv->substream_rec) { writel(0, priv->io + KIRKWOOD_ERR_MASK); - free_irq(priv->irq, prdata); - kfree(prdata); - snd_soc_platform_set_drvdata(platform, NULL); + free_irq(priv->irq, priv); } return 0; @@ -243,13 +219,9 @@ static int kirkwood_dma_hw_free(struct snd_pcm_substream *substream) static int kirkwood_dma_prepare(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; - struct snd_soc_pcm_runtime *soc_runtime = substream->private_data; - struct snd_soc_dai *cpu_dai = soc_runtime->cpu_dai; - struct kirkwood_dma_data *priv; + struct kirkwood_dma_data *priv = kirkwood_priv(substream); unsigned long size, count; - priv = snd_soc_dai_get_dma_data(cpu_dai, substream); - /* compute buffer size in term of "words" as requested in specs */ size = frames_to_bytes(runtime, runtime->buffer_size); size = (size>>2)-1; @@ -272,13 +244,9 @@ static int kirkwood_dma_prepare(struct snd_pcm_substream *substream) static snd_pcm_uframes_t kirkwood_dma_pointer(struct snd_pcm_substream *substream) { - struct snd_soc_pcm_runtime *soc_runtime = substream->private_data; - struct snd_soc_dai *cpu_dai = soc_runtime->cpu_dai; - struct kirkwood_dma_data *priv; + struct kirkwood_dma_data *priv = kirkwood_priv(substream); snd_pcm_uframes_t count; - priv = snd_soc_dai_get_dma_data(cpu_dai, substream); - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) count = bytes_to_frames(substream->runtime, readl(priv->io + KIRKWOOD_PLAY_BYTE_COUNT)); diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h index 4d92637..10a3aaa 100644 --- a/sound/soc/kirkwood/kirkwood.h +++ b/sound/soc/kirkwood/kirkwood.h @@ -129,6 +129,8 @@ struct kirkwood_dma_data { struct clk *extclk; uint32_t ctl_play; uint32_t ctl_rec; + struct snd_pcm_substream *substream_play; + struct snd_pcm_substream *substream_rec; int irq; int burst; }; -- 1.7.4.4
On Sun, Aug 04, 2013 at 08:22:03PM +0100, Russell King wrote:
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.
Applied, thanks.
Use devm_clk_get() to simplify the error path for the external clock. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- sound/soc/kirkwood/kirkwood-i2s.c | 12 ++++-------- 1 files changed, 4 insertions(+), 8 deletions(-) diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index 4c9dad3..ee27981 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -498,10 +498,10 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) if (err < 0) return err; - priv->extclk = clk_get(&pdev->dev, "extclk"); + priv->extclk = devm_clk_get(&pdev->dev, "extclk"); if (!IS_ERR(priv->extclk)) { if (priv->extclk == priv->clk) { - clk_put(priv->extclk); + devm_clk_put(&pdev->dev, priv->extclk); priv->extclk = ERR_PTR(-EINVAL); } else { dev_info(&pdev->dev, "found external clock\n"); @@ -529,10 +529,8 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) return 0; dev_err(&pdev->dev, "snd_soc_register_component failed\n"); - if (!IS_ERR(priv->extclk)) { + if (!IS_ERR(priv->extclk)) clk_disable_unprepare(priv->extclk); - clk_put(priv->extclk); - } clk_disable_unprepare(priv->clk); return err; @@ -544,10 +542,8 @@ static int kirkwood_i2s_dev_remove(struct platform_device *pdev) snd_soc_unregister_component(&pdev->dev); - if (!IS_ERR(priv->extclk)) { + if (!IS_ERR(priv->extclk)) clk_disable_unprepare(priv->extclk); - clk_put(priv->extclk); - } clk_disable_unprepare(priv->clk); return 0; -- 1.7.4.4
On Sun, Aug 04, 2013 at 08:23:03PM +0100, Russell King wrote:
Use devm_clk_get() to simplify the error path for the external clock.
I've applied the diff that remains from the existing upstream code (which frees extclk if it's just the internal clock) with a reworded commit message, thanks.
ASoC automatically creates snd_soc_dapm_dai_in and snd_soc_dapm_dai_out widgets for DAI drivers, and adds them to the list. Later on, ASoC creates automatic routes between these widgets and a widget with a stream name. We look for a snd_soc_dapm_dai_in or snd_soc_dapm_dai_out widget, and use this to obtain the DAI structure. We then scan all widgets for any with a stream name refering to either the capture or the playback stream, and create routes. If you have both a snd_soc_dapm_dai_in and a snd_soc_dapm_dai_out referring to the same DAI structure, this ends up creating one set of routes for the DAI for the snd_soc_dapm_dai_in widget, and a duplicated set of routes for the snd_soc_dapm_dai_out widget. Fix this by checking that the stream name for the widget matches the DAI widget name. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- sound/soc/soc-dapm.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index c7051c4..9f67976 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -3439,7 +3439,7 @@ int snd_soc_dapm_link_dai_widgets(struct snd_soc_card *card) break; } - if (!w->sname) + if (!w->sname || !strstr(w->sname, dai_w->name)) continue; if (dai->driver->playback.stream_name && -- 1.7.4.4
On Sun, Aug 04, 2013 at 08:24:03PM +0100, Russell King wrote:
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.
Applied, thanks. It's possible to do this a bit more neatly for example by checking the direction of the link but this works and fixes the problem, we can always improve it further later.
The CPU DAI widgets are created twice by the core when the CPU DAI and the platform DAI are registered against the same device structure since commit be09ad90e1 (ASoC: core: Add platform DAI widget mapping). This wouldn't be a problem except that these widgets are stored in the DAI structure, and the platform ones are created first, and the CPU ones later. We end up with two sets of widgets: the platform ones appear in debugfs, but the CPU ones do not, and these two sets of widgets with the same name are added to the card's DAI list. This makes it difficult to ensure that the right set of widgets (which will be activated by playback) are correctly linked to other widgets by the DAPM routes. It's also very confusing because the widgets you can view via debugfs may not be the ones ASoC is actually using. This needs fixing properly - this patch is just a hack around the problem which gets Kirkwood with SPDIF working. It is likely this patch breaks other implementations. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- sound/soc/soc-core.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index d56bbea..48e883e 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1362,7 +1362,7 @@ static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order) return -ENODEV; list_add(&cpu_dai->dapm.list, &card->dapm_list); - snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); +// snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); } if (cpu_dai->driver->probe) { -- 1.7.4.4
Provide a helper macro which includes the sum of all enable bits in the playback control register. This simplifies the code a little. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- sound/soc/kirkwood/kirkwood-i2s.c | 10 ++++------ sound/soc/kirkwood/kirkwood.h | 5 ++++- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index ee27981..f39e57c 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -199,8 +199,7 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream, ctl_play |= KIRKWOOD_PLAYCTL_MONO_OFF; priv->ctl_play &= ~(KIRKWOOD_PLAYCTL_MONO_MASK | - KIRKWOOD_PLAYCTL_I2S_EN | - KIRKWOOD_PLAYCTL_SPDIF_EN | + KIRKWOOD_PLAYCTL_ENABLE_MASK | KIRKWOOD_PLAYCTL_SIZE_MASK); priv->ctl_play |= ctl_play; } else { @@ -244,8 +243,7 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream, case SNDRV_PCM_TRIGGER_START: /* configure */ ctl = priv->ctl_play; - value = ctl & ~(KIRKWOOD_PLAYCTL_I2S_EN | - KIRKWOOD_PLAYCTL_SPDIF_EN); + value = ctl & ~KIRKWOOD_PLAYCTL_ENABLE_MASK; writel(value, priv->io + KIRKWOOD_PLAYCTL); /* enable interrupts */ @@ -267,7 +265,7 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream, writel(value, priv->io + KIRKWOOD_INT_MASK); /* disable all playbacks */ - ctl &= ~(KIRKWOOD_PLAYCTL_I2S_EN | KIRKWOOD_PLAYCTL_SPDIF_EN); + ctl &= ~KIRKWOOD_PLAYCTL_ENABLE_MASK; writel(ctl, priv->io + KIRKWOOD_PLAYCTL); break; @@ -387,7 +385,7 @@ static int kirkwood_i2s_probe(struct snd_soc_dai *dai) /* disable playback/record */ value = readl(priv->io + KIRKWOOD_PLAYCTL); - value &= ~(KIRKWOOD_PLAYCTL_I2S_EN|KIRKWOOD_PLAYCTL_SPDIF_EN); + value &= ~KIRKWOOD_PLAYCTL_ENABLE_MASK; writel(value, priv->io + KIRKWOOD_PLAYCTL); value = readl(priv->io + KIRKWOOD_RECCTL); diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h index 10a3aaa..9a50607 100644 --- a/sound/soc/kirkwood/kirkwood.h +++ b/sound/soc/kirkwood/kirkwood.h @@ -54,7 +54,7 @@ #define KIRKWOOD_PLAYCTL_MONO_OFF (0<<5) #define KIRKWOOD_PLAYCTL_I2S_MUTE (1<<7) #define KIRKWOOD_PLAYCTL_SPDIF_EN (1<<4) -#define KIRKWOOD_PLAYCTL_I2S_EN (1<<3) +#define KIRKWOOD_PLAYCTL_I2S_EN (1<<3) #define KIRKWOOD_PLAYCTL_SIZE_MASK (7<<0) #define KIRKWOOD_PLAYCTL_SIZE_16 (7<<0) #define KIRKWOOD_PLAYCTL_SIZE_16_C (3<<0) @@ -62,6 +62,9 @@ #define KIRKWOOD_PLAYCTL_SIZE_24 (1<<0) #define KIRKWOOD_PLAYCTL_SIZE_32 (0<<0) +#define KIRKWOOD_PLAYCTL_ENABLE_MASK (KIRKWOOD_PLAYCTL_SPDIF_EN | \ + KIRKWOOD_PLAYCTL_I2S_EN) + #define KIRKWOOD_PLAY_BUF_ADDR 0x1104 #define KIRKWOOD_PLAY_BUF_SIZE 0x1108 #define KIRKWOOD_PLAY_BYTE_COUNT 0x110C -- 1.7.4.4
On Sun, Aug 04, 2013 at 08:26:03PM +0100, Russell King wrote:
Provide a helper macro which includes the sum of all enable bits in the playback control register. This simplifies the code a little.
Applied, thanks.
These really should be a single driver because they're fully integrated in hardware. Make them so. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- arch/arm/mach-dove/common.c | 4 ++-- arch/arm/mach-kirkwood/common.c | 24 +++++++++--------------- sound/soc/kirkwood/Kconfig | 5 ----- sound/soc/kirkwood/Makefile | 4 +--- sound/soc/kirkwood/kirkwood-dma.c | 30 +----------------------------- sound/soc/kirkwood/kirkwood-i2s.c | 21 ++++++++++++++++----- sound/soc/kirkwood/kirkwood-openrd.c | 4 ++-- sound/soc/kirkwood/kirkwood-t5325.c | 4 ++-- sound/soc/kirkwood/kirkwood.h | 2 ++ 9 files changed, 35 insertions(+), 63 deletions(-) diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c index e2b5da0..68a2d6a 100644 --- a/arch/arm/mach-dove/common.c +++ b/arch/arm/mach-dove/common.c @@ -109,8 +109,8 @@ static void __init dove_clk_init(void) orion_clkdev_add(NULL, "sdhci-dove.1", sdio1); orion_clkdev_add(NULL, "orion_nand", nand); orion_clkdev_add(NULL, "cafe1000-ccic.0", camera); - orion_clkdev_add(NULL, "kirkwood-i2s.0", i2s0); - orion_clkdev_add(NULL, "kirkwood-i2s.1", i2s1); + orion_clkdev_add(NULL, "mvebu-audio.0", i2s0); + orion_clkdev_add(NULL, "mvebu-audio.1", i2s1); orion_clkdev_add(NULL, "mv_crypto", crypto); orion_clkdev_add(NULL, "dove-ac97", ac97); orion_clkdev_add(NULL, "dove-pdma", pdma); diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c index f389228..d24d39d 100644 --- a/arch/arm/mach-kirkwood/common.c +++ b/arch/arm/mach-kirkwood/common.c @@ -263,7 +263,7 @@ void __init kirkwood_clk_init(void) orion_clkdev_add(NULL, MV_XOR_NAME ".1", xor1); orion_clkdev_add("0", "pcie", pex0); orion_clkdev_add("1", "pcie", pex1); - orion_clkdev_add(NULL, "kirkwood-i2s", audio); + orion_clkdev_add(NULL, "mvebu-audio", audio); orion_clkdev_add(NULL, MV64XXX_I2C_CTLR_NAME ".0", runit); orion_clkdev_add(NULL, MV64XXX_I2C_CTLR_NAME ".1", runit); @@ -559,7 +559,7 @@ void __init kirkwood_timer_init(void) /***************************************************************************** * Audio ****************************************************************************/ -static struct resource kirkwood_i2s_resources[] = { +static struct resource kirkwood_audio_resources[] = { [0] = { .start = AUDIO_PHYS_BASE, .end = AUDIO_PHYS_BASE + SZ_16K - 1, @@ -572,29 +572,23 @@ static struct resource kirkwood_i2s_resources[] = { }, }; -static struct kirkwood_asoc_platform_data kirkwood_i2s_data = { +static struct kirkwood_asoc_platform_data kirkwood_audio_data = { .burst = 128, }; -static struct platform_device kirkwood_i2s_device = { - .name = "kirkwood-i2s", +static struct platform_device kirkwood_audio_device = { + .name = "mvebu-audio", .id = -1, - .num_resources = ARRAY_SIZE(kirkwood_i2s_resources), - .resource = kirkwood_i2s_resources, + .num_resources = ARRAY_SIZE(kirkwood_audio_resources), + .resource = kirkwood_audio_resources, .dev = { - .platform_data = &kirkwood_i2s_data, + .platform_data = &kirkwood_audio_data, }, }; -static struct platform_device kirkwood_pcm_device = { - .name = "kirkwood-pcm-audio", - .id = -1, -}; - void __init kirkwood_audio_init(void) { - platform_device_register(&kirkwood_i2s_device); - platform_device_register(&kirkwood_pcm_device); + platform_device_register(&kirkwood_audio_device); } /***************************************************************************** diff --git a/sound/soc/kirkwood/Kconfig b/sound/soc/kirkwood/Kconfig index c62d715..b6917a7 100644 --- a/sound/soc/kirkwood/Kconfig +++ b/sound/soc/kirkwood/Kconfig @@ -6,14 +6,10 @@ config SND_KIRKWOOD_SOC the Kirkwood I2S interface. You will also need to select the audio interfaces to support below. -config SND_KIRKWOOD_SOC_I2S - tristate - config SND_KIRKWOOD_SOC_OPENRD tristate "SoC Audio support for Kirkwood Openrd Client" depends on SND_KIRKWOOD_SOC && (MACH_OPENRD_CLIENT || MACH_OPENRD_ULTIMATE) depends on I2C - select SND_KIRKWOOD_SOC_I2S select SND_SOC_CS42L51 help Say Y if you want to add support for SoC audio on @@ -22,7 +18,6 @@ config SND_KIRKWOOD_SOC_OPENRD config SND_KIRKWOOD_SOC_T5325 tristate "SoC Audio support for HP t5325" depends on SND_KIRKWOOD_SOC && MACH_T5325 && I2C - select SND_KIRKWOOD_SOC_I2S select SND_SOC_ALC5623 help Say Y if you want to add support for SoC audio on diff --git a/sound/soc/kirkwood/Makefile b/sound/soc/kirkwood/Makefile index 3e62ae9..9e78138 100644 --- a/sound/soc/kirkwood/Makefile +++ b/sound/soc/kirkwood/Makefile @@ -1,8 +1,6 @@ -snd-soc-kirkwood-objs := kirkwood-dma.o -snd-soc-kirkwood-i2s-objs := kirkwood-i2s.o +snd-soc-kirkwood-objs := kirkwood-dma.o kirkwood-i2s.o obj-$(CONFIG_SND_KIRKWOOD_SOC) += snd-soc-kirkwood.o -obj-$(CONFIG_SND_KIRKWOOD_SOC_I2S) += snd-soc-kirkwood-i2s.o snd-soc-openrd-objs := kirkwood-openrd.o snd-soc-t5325-objs := kirkwood-t5325.o diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c index 112c262..2091d55 100644 --- a/sound/soc/kirkwood/kirkwood-dma.c +++ b/sound/soc/kirkwood/kirkwood-dma.c @@ -334,36 +334,8 @@ static void kirkwood_dma_free_dma_buffers(struct snd_pcm *pcm) } } -static struct snd_soc_platform_driver kirkwood_soc_platform = { +struct snd_soc_platform_driver kirkwood_soc_platform = { .ops = &kirkwood_dma_ops, .pcm_new = kirkwood_dma_new, .pcm_free = kirkwood_dma_free_dma_buffers, }; - -static int kirkwood_soc_platform_probe(struct platform_device *pdev) -{ - return snd_soc_register_platform(&pdev->dev, &kirkwood_soc_platform); -} - -static int kirkwood_soc_platform_remove(struct platform_device *pdev) -{ - snd_soc_unregister_platform(&pdev->dev); - return 0; -} - -static struct platform_driver kirkwood_pcm_driver = { - .driver = { - .name = "kirkwood-pcm-audio", - .owner = THIS_MODULE, - }, - - .probe = kirkwood_soc_platform_probe, - .remove = kirkwood_soc_platform_remove, -}; - -module_platform_driver(kirkwood_pcm_driver); - -MODULE_AUTHOR("Arnaud Patard <arnaud.patard@rtp-net.org>"); -MODULE_DESCRIPTION("Marvell Kirkwood Audio DMA module"); -MODULE_LICENSE("GPL"); -MODULE_ALIAS("platform:kirkwood-pcm-audio"); diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index f39e57c..84dd9b0 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -24,7 +24,7 @@ #include <linux/platform_data/asoc-kirkwood.h> #include "kirkwood.h" -#define DRV_NAME "kirkwood-i2s" +#define DRV_NAME "mvebu-audio" #define KIRKWOOD_I2S_RATES \ (SNDRV_PCM_RATE_44100 | \ @@ -523,10 +523,20 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) err = snd_soc_register_component(&pdev->dev, &kirkwood_i2s_component, soc_dai, 1); - if (!err) - return 0; - dev_err(&pdev->dev, "snd_soc_register_component failed\n"); + if (err) { + dev_err(&pdev->dev, "snd_soc_register_component failed\n"); + goto err_component; + } + err = snd_soc_register_platform(&pdev->dev, &kirkwood_soc_platform); + if (err) { + dev_err(&pdev->dev, "snd_soc_register_platform failed\n"); + goto err_platform; + } + return 0; + err_platform: + snd_soc_unregister_component(&pdev->dev); + err_component: if (!IS_ERR(priv->extclk)) clk_disable_unprepare(priv->extclk); clk_disable_unprepare(priv->clk); @@ -538,6 +548,7 @@ static int kirkwood_i2s_dev_remove(struct platform_device *pdev) { struct kirkwood_dma_data *priv = dev_get_drvdata(&pdev->dev); + snd_soc_unregister_platform(&pdev->dev); snd_soc_unregister_component(&pdev->dev); if (!IS_ERR(priv->extclk)) @@ -562,4 +573,4 @@ module_platform_driver(kirkwood_i2s_driver); MODULE_AUTHOR("Arnaud Patard, <arnaud.patard@rtp-net.org>"); MODULE_DESCRIPTION("Kirkwood I2S SoC Interface"); MODULE_LICENSE("GPL"); -MODULE_ALIAS("platform:kirkwood-i2s"); +MODULE_ALIAS("platform:mvebu-audio"); diff --git a/sound/soc/kirkwood/kirkwood-openrd.c b/sound/soc/kirkwood/kirkwood-openrd.c index b979c71..a25dfcf 100644 --- a/sound/soc/kirkwood/kirkwood-openrd.c +++ b/sound/soc/kirkwood/kirkwood-openrd.c @@ -54,8 +54,8 @@ static struct snd_soc_dai_link openrd_client_dai[] = { { .name = "CS42L51", .stream_name = "CS42L51 HiFi", - .cpu_dai_name = "kirkwood-i2s", - .platform_name = "kirkwood-pcm-audio", + .cpu_dai_name = "mvebu-audio", + .platform_name = "mvebu-audio", .codec_dai_name = "cs42l51-hifi", .codec_name = "cs42l51-codec.0-004a", .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBS_CFS, diff --git a/sound/soc/kirkwood/kirkwood-t5325.c b/sound/soc/kirkwood/kirkwood-t5325.c index 1d0ed6f..82a8c5f 100644 --- a/sound/soc/kirkwood/kirkwood-t5325.c +++ b/sound/soc/kirkwood/kirkwood-t5325.c @@ -70,8 +70,8 @@ static struct snd_soc_dai_link t5325_dai[] = { { .name = "ALC5621", .stream_name = "ALC5621 HiFi", - .cpu_dai_name = "kirkwood-i2s", - .platform_name = "kirkwood-pcm-audio", + .cpu_dai_name = "mvebu-audio", + .platform_name = "mvebu-audio", .codec_dai_name = "alc5621-hifi", .codec_name = "alc562x-codec.0-001a", .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBS_CFS, diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h index 9a50607..1d13dee 100644 --- a/sound/soc/kirkwood/kirkwood.h +++ b/sound/soc/kirkwood/kirkwood.h @@ -138,4 +138,6 @@ struct kirkwood_dma_data { int burst; }; +extern struct snd_soc_platform_driver kirkwood_soc_platform; + #endif -- 1.7.4.4
On Sun, 04 Aug 2013 20:27:03 +0100 Russell King <rmk+kernel@arm.linux.org.uk> wrote:
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(-) [snip]
Glad to see you got my patch, but why did you changed all the names? Replacing "kirkwood-pcm-audio" by "kirkwood-i2s" should be enough. -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
On Mon, Aug 05, 2013 at 12:13:07PM +0200, Jean-Francois Moine wrote:
On Sun, 04 Aug 2013 20:27:03 +0100 Russell King <rmk+kernel@arm.linux.org.uk> wrote:
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(-) [snip]
Glad to see you got my patch, but why did you changed all the names?
You clearly didn't read the covering email to this series. I don't know why I bothered to write it if you don't bother to read it.
Replacing "kirkwood-pcm-audio" by "kirkwood-i2s" should be enough.
Quite simply, i2s is a bus protocol, just like spdif is. It can do both. If we're changing its name, then naming it after just one of the protocols it does is stupid. And PCM is also inappropriate - it does more than PCM because it can also transmit AC3 and MPEG audio over SPDIF to a suitable decoder (and that does work - I've tested it.) At the end of the day, this is _just_ a DMA engine which outputs I2S and/or SPDIF formatted data of unknown type at a known rate.
On Sun, Aug 04, 2013 at 08:27:03PM +0100, Russell King wrote:
These really should be a single driver because they're fully integrated in hardware. Make them so.
Applied, thanks.
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- sound/soc/kirkwood/kirkwood-dma.c | 2 +- sound/soc/kirkwood/kirkwood.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletions(-) diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c index 2091d55..ee31016 100644 --- a/sound/soc/kirkwood/kirkwood-dma.c +++ b/sound/soc/kirkwood/kirkwood-dma.c @@ -51,7 +51,7 @@ static struct snd_pcm_hardware kirkwood_dma_snd_hw = { .rate_max = 384000, .channels_min = 1, .channels_max = 8, - .buffer_bytes_max = KIRKWOOD_SND_MAX_PERIOD_BYTES * KIRKWOOD_SND_MAX_PERIODS, + .buffer_bytes_max = KIRKWOOD_SND_MAX_BUFFER_BYTES, .period_bytes_min = KIRKWOOD_SND_MIN_PERIOD_BYTES, .period_bytes_max = KIRKWOOD_SND_MAX_PERIOD_BYTES, .periods_min = KIRKWOOD_SND_MIN_PERIODS, diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h index 1d13dee..f8e1ccc 100644 --- a/sound/soc/kirkwood/kirkwood.h +++ b/sound/soc/kirkwood/kirkwood.h @@ -125,6 +125,8 @@ #define KIRKWOOD_SND_MAX_PERIODS 16 #define KIRKWOOD_SND_MIN_PERIOD_BYTES 0x4000 #define KIRKWOOD_SND_MAX_PERIOD_BYTES 0x4000 +#define KIRKWOOD_SND_MAX_BUFFER_BYTES (KIRKWOOD_SND_MAX_PERIOD_BYTES \ + * KIRKWOOD_SND_MAX_PERIODS) struct kirkwood_dma_data { void __iomem *io; -- 1.7.4.4
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 | 36 +++++++++++++++++++++++++++++++++++- sound/soc/kirkwood/kirkwood.h | 1 + 2 files changed, 36 insertions(+), 1 deletions(-) diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index 84dd9b0..3d039c3 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,39 @@ 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) +{ + /* This works because the platform and cpu dai are the same dev */ + struct kirkwood_dma_data *priv = snd_soc_platform_get_drvdata(w->platform); + + 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->platform->dapm, + widgets, ARRAY_SIZE(widgets)); + /* put system in a "safe" state : */ /* disable audio interrupts */ writel(0xffffffff, priv->io + KIRKWOOD_INT_CAUSE); @@ -383,6 +410,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 +442,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 +462,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 +471,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 +567,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) goto err_platform; } return 0; + err_platform: snd_soc_unregister_component(&pdev->dev); err_component: diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h index f8e1ccc..3b70876 100644 --- a/sound/soc/kirkwood/kirkwood.h +++ b/sound/soc/kirkwood/kirkwood.h @@ -132,6 +132,7 @@ struct kirkwood_dma_data { void __iomem *io; struct clk *clk; struct clk *extclk; + uint32_t ctl_play_mask; uint32_t ctl_play; uint32_t ctl_rec; struct snd_pcm_substream *substream_play; -- 1.7.4.4
Add the DAPM links to connect the codec DAC and ADCs to the cpu DAI I2S inputs/outputs. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- sound/soc/kirkwood/kirkwood-openrd.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/sound/soc/kirkwood/kirkwood-openrd.c b/sound/soc/kirkwood/kirkwood-openrd.c index a25dfcf..37740f4 100644 --- a/sound/soc/kirkwood/kirkwood-openrd.c +++ b/sound/soc/kirkwood/kirkwood-openrd.c @@ -63,12 +63,19 @@ static struct snd_soc_dai_link openrd_client_dai[] = { }, }; +static const struct snd_soc_dapm_route routes[] = { + /* Connect the codec streams to the I2S connections */ + { "Playback", NULL, "i2sdo" }, + { "i2sdi", NULL, "Capture" }, +}; static struct snd_soc_card openrd_client = { .name = "OpenRD Client", .owner = THIS_MODULE, .dai_link = openrd_client_dai, .num_links = ARRAY_SIZE(openrd_client_dai), + .dapm_routes = routes, + .num_dapm_routes = ARRAY_SIZE(routes), }; static int openrd_probe(struct platform_device *pdev) -- 1.7.4.4
Add the DAPM links to connect the codec DAC and ADCs to the cpu DAI I2S inputs/outputs. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- sound/soc/kirkwood/kirkwood-t5325.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/sound/soc/kirkwood/kirkwood-t5325.c b/sound/soc/kirkwood/kirkwood-t5325.c index 82a8c5f..1647779 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) -- 1.7.4.4
On Sun, Aug 04, 2013 at 08:31:04PM +0100, Russell King wrote:
Add the DAPM links to connect the codec DAC and ADCs to the cpu DAI I2S inputs/outputs.
+ { "i2sdi", NULL, "Capture" }, + { "Playback", NULL, "i2sdo" },
This doesn't look good - this is adding DAPM routes which should correspond to the DAI link that's already been configured. This isn't something we should have to add to the machine driver, if the machine driver already set up a dai_link then these things should already be taken care of one way or another. Either links like this get added or the DAIs get kicked at runtime as part of the link startup.
On Mon, Aug 05, 2013 at 12:27:33PM +0100, Mark Brown wrote:
On Sun, Aug 04, 2013 at 08:31:04PM +0100, Russell King wrote:
Add the DAPM links to connect the codec DAC and ADCs to the cpu DAI I2S inputs/outputs.
+ { "i2sdi", NULL, "Capture" }, + { "Playback", NULL, "i2sdo" },
This doesn't look good - this is adding DAPM routes which should correspond to the DAI link that's already been configured.
No, you're wrong there: CPU DAI: Codec DAI dma-playback ---> i2sdo ---> Playback `--> spdifdo -> not connected dma-capture <--- i2sdi <--- Capture And the intermediate level is needed to determine which outputs from the chip are wired up. This structure, which is what ASoC normally does, for this SoC is wrong: CPU DAI: Codec DAI dma-playback ---> Playback dma-capture <--- Capture because it contains no information on how the connectivity between the codec and DAI is performed. And don't even say "use dpcm" - if you say that, I want _you_ to write it because dpcm is totally and utterly unusable as it currently stands - as you can see from all the emails I've sent over the weekend.
On Mon, Aug 05, 2013 at 12:33:10PM +0100, Russell King - ARM Linux wrote:
On Mon, Aug 05, 2013 at 12:27:33PM +0100, Mark Brown wrote:
This doesn't look good - this is adding DAPM routes which should correspond to the DAI link that's already been configured.
No, you're wrong there:
CPU DAI: Codec DAI dma-playback ---> i2sdo ---> Playback `--> spdifdo -> not connected dma-capture <--- i2sdi <--- Capture
And the intermediate level is needed to determine which outputs from the chip are wired up.
Right, and that's exactly what the dai_links are doing - they're showing what's hanging off each of the DAIs.
And don't even say "use dpcm" - if you say that, I want _you_ to write it because dpcm is totally and utterly unusable as it currently stands - as you can see from all the emails I've sent over the weekend.
I'm going to tell you to work with the framework rather than around it; adding routes that link the CODEC and CPU together in parallel with the links set up for the DAIs is not something that seems like it's going to continue to work sensibly going forwards. If this were done at a framework level that should be fine but having it as a special case in some specific boards isn't good. Since I don't have Kirkwood hardware or any particular use for it I've no real intention of working on the hardware directly, sorry.
On Mon, Aug 05, 2013 at 03:40:54PM +0100, Mark Brown wrote:
On Mon, Aug 05, 2013 at 12:33:10PM +0100, Russell King - ARM Linux wrote:
On Mon, Aug 05, 2013 at 12:27:33PM +0100, Mark Brown wrote:
This doesn't look good - this is adding DAPM routes which should correspond to the DAI link that's already been configured.
No, you're wrong there:
CPU DAI: Codec DAI dma-playback ---> i2sdo ---> Playback `--> spdifdo -> not connected dma-capture <--- i2sdi <--- Capture
And the intermediate level is needed to determine which outputs from the chip are wired up.
Right, and that's exactly what the dai_links are doing - they're showing what's hanging off each of the DAIs.
And don't even say "use dpcm" - if you say that, I want _you_ to write it because dpcm is totally and utterly unusable as it currently stands - as you can see from all the emails I've sent over the weekend.
I'm going to tell you to work with the framework rather than around it;
Work with the fscking undocumented framework. If you want that, then sodding well document this pile of crap. If not, stop telling people to "work around it".
adding routes that link the CODEC and CPU together in parallel with the links set up for the DAIs is not something that seems like it's going to continue to work sensibly going forwards.
So you're *TOTALLY* missing why I've had to do that. Tell me, how can the CPU DAI detect which of its frigging outputs are connected to the codec without doing the above? Show me how your crappy subsystem is supposed to work. Document the sodding thing. If you don't do this, don't expect people to understand it. Don't expect people not to find "other solutions" around the utter shite that I've had to deal with ALL FRIGGING WEEKEND. These patches stay as they are until you get a clue about what's required to drive this hardware and start providing some sensible assistance with your undocumented code.
On Mon, Aug 05, 2013 at 03:56:44PM +0100, Russell King - ARM Linux wrote:
On Mon, Aug 05, 2013 at 03:40:54PM +0100, Mark Brown wrote:
On Mon, Aug 05, 2013 at 12:33:10PM +0100, Russell King - ARM Linux wrote:
On Mon, Aug 05, 2013 at 12:27:33PM +0100, Mark Brown wrote:
This doesn't look good - this is adding DAPM routes which should correspond to the DAI link that's already been configured.
No, you're wrong there:
CPU DAI: Codec DAI dma-playback ---> i2sdo ---> Playback `--> spdifdo -> not connected dma-capture <--- i2sdi <--- Capture
Well, having followed your suggestions and re-analysed Liam's driver, it basically does not work, because none of the widgets get powered up. Liam's DAPM driver does this: ==== Frontend ==== 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 Routing: [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 ==== Platform ==== Routing: [aif]SSP0 CODEC OUT ---> [s]AIF1 Playback ... ==== Codec ==== DAI drivers: DAI stream names rt5640-aif1 AIF1 Playback --> AIF1RX --> DAI1 RX Mux --> IF1 DAC -->... AIF1 Capture ... And what I have already in kirkwood-i2s is: CPU DAI: [s]dma-playback ---> [aif]i2sdo `--> [aif]spdifdo [s]dma-capture <--- [aif]i2sdi So that follows exactly what Liam's driver does. So no changes are required there. I've changed the DAI links to this in kirkwood-spdif.c: static struct snd_soc_dai_link kirkwood_spdif_dai1[] = { { .name = "S/PDIF1", .stream_name = "IEC958 Playback", .platform_name = "mvebu-audio.1", .cpu_dai_name = "mvebu-audio.1", .dynamic = 1, .codec_name = "snd-soc-dummy", .codec_dai_name = "snd-soc-dummy-dai", }, { .name = "Codec", .cpu_dai_name = "snd-soc-dummy-dai", .platform_name = "snd-soc-dummy", .no_pcm = 1, .codec_dai_name = "dit-hifi", .codec_name = "spdif-dit", .ops = &kirkwood_spdif_ops, }, }; which is similar to what's in Liam's driver. However, There is no .dpcm_playback member in mainline ASoC. However, when trying to play something: bash# grep . /sys/kernel/debug/asoc/Kirkwood\ SPDIF/*/dapm/* | \ sed 's,/sys/kernel/debug/asoc/Kirkwood SPDIF/,,' mvebu-audio.1/dapm/bias_level:Off mvebu-audio.1/dapm/i2sdi:i2sdi: Off in 0 out 0 mvebu-audio.1/dapm/i2sdi: stream dma-rx inactive mvebu-audio.1/dapm/i2sdo:i2sdo: Off in 0 out 0 mvebu-audio.1/dapm/i2sdo: stream dma-tx inactive mvebu-audio.1/dapm/spdifdo:spdifdo: Off in 0 out 0 mvebu-audio.1/dapm/spdifdo: stream dma-tx inactive mvebu-audio.1/dapm/spdifdo: out "static" "Playback" snd-soc-dummy/dapm/Capture:Capture: Off in 0 out 0 snd-soc-dummy/dapm/Capture: stream Capture inactive snd-soc-dummy/dapm/Playback:Playback: Off in 0 out 0 snd-soc-dummy/dapm/Playback: stream Playback inactive snd-soc-dummy/dapm/Playback: in "static" "spdifdo" snd-soc-dummy/dapm/bias_level:Standby spdif-dit/dapm/Playback:Playback: Off in 0 out 1 spdif-dit/dapm/Playback: stream Playback inactive spdif-dit/dapm/Playback: out "static" "spdif-out" spdif-dit/dapm/bias_level:Standby spdif-dit/dapm/spdif-out:spdif-out: Off in 0 out 1 spdif-dit/dapm/spdif-out: in "static" "Playback" Nothing gets powered up. This is hardly surprising, because of how ASoC connects the widgets. So, let's summarise this. You're saying that I'm doing it all wrong in my driver creating those widgets and paths. Yet, the widgets and paths are exactly what Liam creates in his driver. Not only that, but we have even more duplicated widgets created with this method, even with the hack from this patch set. snd_soc_dapm_new_dai_widgets: creating playback widget Playback:Playback for dai d81247c0 dapm d8263f10 (playback_widget was (null), new c05ab080) snd_soc_dapm_new_dai_widgets: creating playback widget Playback:Playback for dai d81247c0 dapm d8263c70 (playback_widget was c05ab080, new d8fe2b40) It seems Liam's commit needs to be reverted to fix that. Whether that's correct or not, I've no idea. Plus, when I try to set things up as Liam has, the result doesn't work, because of the widget naming scheme having no disambiguation.
On Mon, Aug 05, 2013 at 09:32:02PM +0100, Russell King - ARM Linux wrote:
which is similar to what's in Liam's driver. However, There is no .dpcm_playback member in mainline ASoC.
OK, that's interesting... presumably there's some other framework changes need upstreaming here, I've really not been paying attention to any out of tree code here.
So, let's summarise this. You're saying that I'm doing it all wrong in my driver creating those widgets and paths. Yet, the widgets and paths are exactly what Liam creates in his driver.
Including the CPU<->CODEC paths? Huh, so it seems. That wasn't required in any of the previous out of tree DPCM systems I've worked on in the past. Something's changed somewhere along the line here... If those are required they really should be created by the framework in the same way as we do for CODEC<->CODEC links, it's not at all sensible to have to supply them twice and like I say it's just asking for problems.
Not only that, but we have even more duplicated widgets created with this method, even with the hack from this patch set.
snd_soc_dapm_new_dai_widgets: creating playback widget Playback:Playback for dai d81247c0 dapm d8263f10 (playback_widget was (null), new c05ab080) snd_soc_dapm_new_dai_widgets: creating playback widget Playback:Playback for dai d81247c0 dapm d8263c70 (playback_widget was c05ab080, new d8fe2b40)
OK, that's not good.
It seems Liam's commit needs to be reverted to fix that. Whether that's correct or not, I've no idea.
Needs a closer look I think.
Plus, when I try to set things up as Liam has, the result doesn't work, because of the widget naming scheme having no disambiguation.
That's easy enough to sidestep for now.
On Mon, Aug 05, 2013 at 11:06:39PM +0100, Mark Brown wrote:
On Mon, Aug 05, 2013 at 09:32:02PM +0100, Russell King - ARM Linux wrote:
which is similar to what's in Liam's driver. However, There is no .dpcm_playback member in mainline ASoC.
OK, that's interesting... presumably there's some other framework changes need upstreaming here, I've really not been paying attention to any out of tree code here.
So, let's summarise this. You're saying that I'm doing it all wrong in my driver creating those widgets and paths. Yet, the widgets and paths are exactly what Liam creates in his driver.
Including the CPU<->CODEC paths? Huh, so it seems. That wasn't required in any of the previous out of tree DPCM systems I've worked on in the past. Something's changed somewhere along the line here...
If those are required they really should be created by the framework in the same way as we do for CODEC<->CODEC links, it's not at all sensible to have to supply them twice and like I say it's just asking for problems.
Not only that, but we have even more duplicated widgets created with this method, even with the hack from this patch set.
snd_soc_dapm_new_dai_widgets: creating playback widget Playback:Playback for dai d81247c0 dapm d8263f10 (playback_widget was (null), new c05ab080) snd_soc_dapm_new_dai_widgets: creating playback widget Playback:Playback for dai d81247c0 dapm d8263c70 (playback_widget was c05ab080, new d8fe2b40)
OK, that's not good.
It seems Liam's commit needs to be reverted to fix that. Whether that's correct or not, I've no idea.
Needs a closer look I think.
Plus, when I try to set things up as Liam has, the result doesn't work, because of the widget naming scheme having no disambiguation.
That's easy enough to sidestep for now.
... which does almost nothing to fix the problem. S!PDIF1: ASoC: found 0 audio playback paths S!PDIF1: ASoC: S/PDIF1 no valid playback route S!PDIF1: ASoC: found 0 new BE paths S!PDIF1: ASoC: open FE S/PDIF1 S!PDIF1: ASoC: hw_params FE S/PDIF1 rate 44100 chan 2 fmt 2 S!PDIF1: ASoC: prepare FE S/PDIF1 S!PDIF1: ASoC: no backend DAIs enabled for S/PDIF1 S!PDIF1: ASoC: hw_free FE S/PDIF1 S!PDIF1: ASoC: hw_params FE S/PDIF1 rate 44100 chan 2 fmt 2 Here's what the DAPM widgets look like - this is only those which ASoC places into debugfs which is not all of them: mvebu-audio.1/dapm/bias_level:Off mvebu-audio.1/dapm/i2sdi:i2sdi: Off in 0 out 0 mvebu-audio.1/dapm/i2sdi: stream dma-rx inactive mvebu-audio.1/dapm/i2sdo:i2sdo: Off in 0 out 0 mvebu-audio.1/dapm/i2sdo: stream dma-tx inactive mvebu-audio.1/dapm/spdifdo:spdifdo: Off in 0 out 1 mvebu-audio.1/dapm/spdifdo: stream dma-tx inactive mvebu-audio.1/dapm/spdifdo: out "static" "spdif-Playback" snd-soc-dummy/dapm/Capture:Capture: Off in 0 out 0 snd-soc-dummy/dapm/Capture: stream Capture inactive snd-soc-dummy/dapm/Playback:Playback: Off in 0 out 0 snd-soc-dummy/dapm/Playback: stream Playback inactive snd-soc-dummy/dapm/bias_level:Standby spdif-dit/dapm/bias_level:Standby spdif-dit/dapm/spdif-Playback:spdif-Playback: Off in 0 out 1 spdif-dit/dapm/spdif-Playback: stream spdif-Playback inactive spdif-dit/dapm/spdif-Playback: in "static" "spdifdo" spdif-dit/dapm/spdif-Playback: out "static" "spdif-out" spdif-dit/dapm/spdif-out:spdif-out: Off in 0 out 1 spdif-dit/dapm/spdif-out: in "static" "spdif-Playback" And below is the diff against my original working build. Much of the diff is debugging for investigations into DAPM to discover various things about it which I was unable to find out by asking. I put it to you that DPCM in mainline is incomplete and nonfunctional as it currently stands. Moreover, it requires either those who know that code to continue to develop it, or someone else who understands the direction that this code is supposed to go picks up to complete it. Add to that that you say that this was being developed with TI and the TI situation happened, it seems unlikely to me that there's any significant chance of movement on that. So, insisting that DPCM is used for this driver is grossly unfair. What I have implemented so far in these patches is not _that_ far from apparantly what is required for a DPCM solution, if only it would work. Therefore, there is no reason for you to refuse this patch based on your original assertion. The main thing is that it _works_ and, as far as is currently known, it should not cause a regression for existing mainline kernel users. Hopefully the t5325 folk will be able to test on their platform. If DPCM moves forward to a state where it becomes usable, or if Liam can provide some input on what's wrong and how it can be addressed, we can then look at converting the driver over to use it. Given everything I've discovered about DPCM thus far should be a trivial matter of something like the diff below (without the debug hacks) - provided of course that the structure in Liam's driver was correct. Let's not forget: Liam may be working on this stuff, and may have his own set of patches to sort out DPCM, so having patches from me messing around with the DPCM code apparantly "fixing" problems with it may very well be counter-productive. Again - Liam needs to provide some input on this. 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 @@ -36,7 +36,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 = { @@ -49,7 +49,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-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index 05d977a..977e461 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -372,7 +372,7 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream, dev_notice(dai->dev, "timed out waiting for busy to deassert: %08x\n", ctl); } - +printk("%s: %02x %02x\n", __func__, priv->ctl_play, priv->ctl_play_mask); switch (cmd) { case SNDRV_PCM_TRIGGER_START: /* configure */ diff --git a/sound/soc/kirkwood/kirkwood-spdif.c b/sound/soc/kirkwood/kirkwood-spdif.c index e5257ec..13f2fcd 100644 --- a/sound/soc/kirkwood/kirkwood-spdif.c +++ b/sound/soc/kirkwood/kirkwood-spdif.c @@ -57,7 +57,7 @@ static struct snd_soc_ops kirkwood_spdif_ops = { }; static const struct snd_soc_dapm_route routes[] = { - { "Playback", NULL, "spdifdo" }, + { "spdif-Playback", NULL, "spdifdo" }, }; static struct snd_soc_dai_link kirkwood_spdif_dai0[] = { @@ -78,6 +78,14 @@ static struct snd_soc_dai_link kirkwood_spdif_dai1[] = { .stream_name = "IEC958 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", + .cpu_dai_name = "snd-soc-dummy-dai", + .platform_name = "snd-soc-dummy", + .no_pcm = 1, .codec_dai_name = "dit-hifi", .codec_name = "spdif-dit", .ops = &kirkwood_spdif_ops, @@ -108,7 +116,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-core.c b/sound/soc/soc-core.c index 48e883e..c312ad6 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1163,7 +1163,7 @@ static int soc_probe_platform(struct snd_soc_card *card, if (dai->dev != platform->dev) continue; - snd_soc_dapm_new_dai_widgets(&platform->dapm, dai); +// snd_soc_dapm_new_dai_widgets(&platform->dapm, dai); } platform->dapm.idle_bias_off = 1; @@ -1362,7 +1362,7 @@ static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order) return -ENODEV; list_add(&cpu_dai->dapm.list, &card->dapm_list); -// snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); + snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); } if (cpu_dai->driver->probe) { diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 9f67976..a3ba010 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -22,7 +22,7 @@ * device reopen. * */ - +#define DEBUG #include <linux/module.h> #include <linux/moduleparam.h> #include <linux/init.h> @@ -305,6 +305,7 @@ static int snd_soc_dapm_set_bias_level(struct snd_soc_dapm_context *dapm, int ret = 0; trace_snd_soc_bias_level_start(card, level); + printk("%s(%p,%u)\n", __func__, card, level); if (card && card->set_bias_level) ret = card->set_bias_level(card, dapm, level); @@ -1110,6 +1111,7 @@ EXPORT_SYMBOL_GPL(dapm_clock_event); static int dapm_widget_power_check(struct snd_soc_dapm_widget *w) { +printk("%s(%p): %s %u %u %u\n", __func__, w, w->name, w->power_checked, w->new_power, w->force); if (w->power_checked) return w->new_power; @@ -1119,6 +1121,7 @@ static int dapm_widget_power_check(struct snd_soc_dapm_widget *w) w->new_power = w->power_check(w); w->power_checked = true; +printk("%s: %s %u %u %u\n", __func__, w->name, w->power_checked, w->new_power, w->force); return w->new_power; } @@ -1135,6 +1138,7 @@ static int dapm_generic_check_power(struct snd_soc_dapm_widget *w) dapm_clear_walk_input(w->dapm, &w->sources); out = is_connected_output_ep(w, NULL); dapm_clear_walk_output(w->dapm, &w->sinks); +printk("%s(%p): %u %u\n", __func__, w, out, in); return out != 0 && in != 0; } @@ -1163,6 +1167,7 @@ static int dapm_dac_check_power(struct snd_soc_dapm_widget *w) if (w->active) { out = is_connected_output_ep(w, NULL); + printk("%s(%p): %u\n", __func__, w, out); dapm_clear_walk_output(w->dapm, &w->sinks); return out != 0; } else { @@ -1576,6 +1581,7 @@ static void dapm_widget_set_power(struct snd_soc_dapm_widget *w, bool power, return; trace_snd_soc_dapm_widget_power(w, power); + printk("%s(%p,%u): %s\n", __func__, w, power, w->name); /* If we changed our power state perhaps our neigbours changed * also. @@ -1652,6 +1658,7 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event) enum snd_soc_bias_level bias; trace_snd_soc_dapm_start(card); + printk("%s(%p,%u)\n", __func__, dapm, event); list_for_each_entry(d, &card->dapm_list, list) { if (d->idle_bias_off) @@ -1669,6 +1676,7 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event) * iterate. */ list_for_each_entry(w, &card->dapm_dirty, dirty) { +printk("%s: dirty %s power %u active %u\n", __func__, w->name, w->power, w->active); dapm_power_one_widget(w, &up_list, &down_list); } @@ -1682,7 +1690,7 @@ static int dapm_power_widgets(struct snd_soc_dapm_context *dapm, int event) list_del_init(&w->dirty); break; } - +printk("%s: widget %s power %u active %u\n", __func__, w->name, w->power, w->active); if (w->power) { d = w->dapm; @@ -2256,7 +2264,7 @@ static int snd_soc_dapm_add_route(struct snd_soc_dapm_context *dapm, route->sink); return -ENODEV; } - +printk("%s: adding a route from %s to %s (%p->%p)\n", __func__, wsource->name, wsink->name, wsource, wsink); path = kzalloc(sizeof(struct snd_soc_dapm_path), GFP_KERNEL); if (!path) return -ENOMEM; @@ -2562,6 +2570,7 @@ int snd_soc_dapm_new_widgets(struct snd_soc_dapm_context *dapm) { struct snd_soc_dapm_widget *w; unsigned int val; +printk("%s(%p)\n", __func__, dapm); mutex_lock_nested(&dapm->card->dapm_mutex, SND_SOC_DAPM_CLASS_INIT); @@ -3060,6 +3069,7 @@ snd_soc_dapm_new_control(struct snd_soc_dapm_context *dapm, if ((w = dapm_cnew_widget(widget)) == NULL) return NULL; +printk("%s(%p, %p): %s => %p\n", __func__, dapm, widget, widget->name, w); switch (w->id) { case snd_soc_dapm_regulator_supply: @@ -3184,6 +3194,7 @@ int snd_soc_dapm_new_controls(struct snd_soc_dapm_context *dapm, struct snd_soc_dapm_widget *w; int i; int ret = 0; +printk("%s(%p)\n", __func__, dapm); mutex_lock_nested(&dapm->card->dapm_mutex, SND_SOC_DAPM_CLASS_INIT); for (i = 0; i < num; i++) { @@ -3326,6 +3337,8 @@ int snd_soc_dapm_new_pcm(struct snd_soc_card *card, return -ENOMEM; snprintf(link_name, len, "%s-%s", source->name, sink->name); + printk("%s: linking %s to %s via %s\n", __func__, source->name, sink->name, link_name); + memset(&template, 0, sizeof(template)); template.reg = SND_SOC_NOPM; template.id = snd_soc_dapm_dai_link; @@ -3376,6 +3389,8 @@ int snd_soc_dapm_new_dai_widgets(struct snd_soc_dapm_context *dapm, template.name); w = snd_soc_dapm_new_control(dapm, &template); +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); if (!w) { dev_err(dapm->dev, "ASoC: Failed to create %s widget\n", dai->driver->playback.stream_name); @@ -3394,6 +3409,8 @@ int snd_soc_dapm_new_dai_widgets(struct snd_soc_dapm_context *dapm, template.name); w = snd_soc_dapm_new_control(dapm, &template); +printk("%s: creating capture widget %s:%s for dai %p dapm %p (capture_widget was %p, new %p)\n", + __func__, template.name, template.sname, dai, dapm, dai->capture_widget, w); if (!w) { dev_err(dapm->dev, "ASoC: Failed to create %s widget\n", dai->driver->capture.stream_name); @@ -3423,7 +3440,7 @@ int snd_soc_dapm_link_dai_widgets(struct snd_soc_card *card) default: continue; } - +printk("%s: found dai widget %s:%s (%p)\n", __func__, dai_w->name, dai_w->sname, dai_w); dai = dai_w->priv; /* ...find all widgets with the same stream and link them */ @@ -3441,6 +3458,7 @@ int snd_soc_dapm_link_dai_widgets(struct snd_soc_card *card) if (!w->sname || !strstr(w->sname, dai_w->name)) continue; +printk("%s: found widget %s:%s (%p)\n", __func__, w->name, w->sname, w); if (dai->driver->playback.stream_name && strstr(w->sname, @@ -3484,7 +3502,7 @@ static void soc_dapm_stream_event(struct snd_soc_pcm_runtime *rtd, int stream, w_cpu = cpu_dai->capture_widget; w_codec = codec_dai->capture_widget; } - +printk("%s: w_cpu %p w_codec %p event %u\n", __func__, w_cpu, w_codec, event); if (w_cpu) { dapm_mark_dirty(w_cpu, "stream event"); diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index ccb6be4..b1c98b5 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -15,7 +15,7 @@ * option) any later version. * */ - +#define DEBUG #include <linux/kernel.h> #include <linux/init.h> #include <linux/delay.h>
On Tue, Aug 06, 2013 at 12:30:15AM +0100, Russell King - ARM Linux wrote:
I put it to you that DPCM in mainline is incomplete and nonfunctional as it currently stands. Moreover, it requires either those who know that code to continue to develop it, or someone else who understands the direction that this code is supposed to go picks up to complete it.
I'd be most distressed if it were far off working; it's close enough to the out of tree code I've worked with and I know there were drivers in progress when it was submitted. We've certainly had at least one bug fix from the out of tree users, I'd be surprised if we had anything more substantial than bitrot in the current code.
Add to that that you say that this was being developed with TI and the TI situation happened, it seems unlikely to me that there's any significant chance of movement on that.
TI isn't going to be a big problem here. As you've seen Intel are currently using it for Haswell and they are generally keen on upstreaming, Qualcomm are also relying very heavily on this and the OMAP4s are still out there albeit with a reduced development team. There's a few others including one I expect to get round to when I'm not spending my time writing mails like this but those the the big systems with active use.
So, insisting that DPCM is used for this driver is grossly unfair.
The framework can't cope with simultaneous use of both interfaces unless DPCM is used or the framework is otherwise extended. This is not an unusual situation and given that there is already framework there it would seem to be the most sensible starting point. As is normal any driver local approach is going to need to not get in the way of framework development.
Let's not forget: Liam may be working on this stuff, and may have his own set of patches to sort out DPCM, so having patches from me messing around with the DPCM code apparantly "fixing" problems with it may very well be counter-productive. Again - Liam needs to provide some input on this.
Liam is on vacation this week, he'll be back next week.
On Tue, Aug 06, 2013 at 02:32:06PM +0100, Mark Brown wrote:
On Tue, Aug 06, 2013 at 12:30:15AM +0100, Russell King - ARM Linux wrote:
I put it to you that DPCM in mainline is incomplete and nonfunctional as it currently stands. Moreover, it requires either those who know that code to continue to develop it, or someone else who understands the direction that this code is supposed to go picks up to complete it.
I'd be most distressed if it were far off working; it's close enough to the out of tree code I've worked with and I know there were drivers in progress when it was submitted. We've certainly had at least one bug fix from the out of tree users, I'd be surprised if we had anything more substantial than bitrot in the current code.
Right, so, I've looked at this again today, and I've sort-of got it working. It would appear that the problem is that the AIF widgets are not finding their stream - the implicit routes are not being created. It _appears_ that the strean name in widgets are only ever connected to the streams within the same DAPM context. Hence, to get this stuff right, you _have_ to know about the DAPM internals, and you _have_ to know that it's more than just "a graph walk". Documentation would help there... Anyway, with that fixed, the widgets get powered up - great, finally some forward progress. Not quite, because it still doesn't work right. ASoC still ends up filling my disk with lots of: S!PDIF1: ASoC: no backend DAIs enabled for S/PDIF1 messages. Throwing a WARN_ON(1) just after that message reveals: [<c032d334>] (dpcm_fe_dai_prepare+0xe0/0xf0) from [<c030d324>] (snd_pcm_do_prepare+0x14/0x2c) [<c030d324>] (snd_pcm_do_prepare+0x14/0x2c) from [<c030cedc>] (snd_pcm_action_single+0x38/0x78) [<c030cedc>] (snd_pcm_action_single+0x38/0x78) from [<c030e8e0>] (snd_pcm_action_nonatomic+0x60/0x78) [<c030e8e0>] (snd_pcm_action_nonatomic+0x60/0x78) from [<c0311040>] (snd_pcm_common_ioctl1+0x2b4/0x5bc) [<c0311040>] (snd_pcm_common_ioctl1+0x2b4/0x5bc) from [<c0311398>] (snd_pcm_capture_ioctl1+0x50/0x308) [<c0311398>] (snd_pcm_capture_ioctl1+0x50/0x308) from [<c00e50bc>] (do_vfs_ioctl+0x80/0x31c) [<c00e50bc>] (do_vfs_ioctl+0x80/0x31c) from [<c00e5394>] (SyS_ioctl+0x3c/0x60) [<c00e5394>] (SyS_ioctl+0x3c/0x60) from [<c000e3e0>] (ret_fast_syscall+0x0/0x48) So, it's because the capture isn't wired up. The capture isn't wired up on this platform, so how do I tell ASoC not to bother with the capture side with DPCM? Creating a link for the capture side to the dummy codec is wrong, because that allows capture to be brought up when in fact there is no capture wired up on the board (and means that we'll end up trying to use unconnected inputs.) I think the easiest "solution" to that is to just comment out the damned warning in the core code for the moment, until a proper solution can be thought up. There is also this which seems to be a core ALSA problem - I get two of these on every boot: WARNING: at /home/rmk/git/linux-cubox/fs/proc/generic.c:356 proc_register+0xac/0x128() proc_dir_entry 'asound/oss' already registered Modules linked in: snd_soc_spdif_tx m25p80 mtd orion_wdt snd_soc_kirkwood snd_soc_kirkwood_spdif CPU: 0 PID: 388 Comm: kworker/u2:2 Not tainted 3.10.0+ #649 Workqueue: deferwq deferred_probe_work_func [<c0013d7c>] (unwind_backtrace+0x0/0xf8) from [<c0011bec>] (show_stack+0x10/0x14) [<c0011bec>] (show_stack+0x10/0x14) from [<c0048b80>] (warn_slowpath_common+0x4c/0x68) [<c0048b80>] (warn_slowpath_common+0x4c/0x68) from [<c0048c30>] (warn_slowpath_fmt+0x30/0x40) [<c0048c30>] (warn_slowpath_fmt+0x30/0x40) from [<c0124804>] (proc_register+0xac/0x128) [<c0124804>] (proc_register+0xac/0x128) from [<c0124910>] (proc_create_data+0x90/0xac) [<c0124910>] (proc_create_data+0x90/0xac) from [<c0302b48>] (snd_info_register+0x54/0xf0) [<c0302b48>] (snd_info_register+0x54/0xf0) from [<c03190e8>] (snd_pcm_oss_register_minor+0xcc/0x1cc) [<c03190e8>] (snd_pcm_oss_register_minor+0xcc/0x1cc) from [<c030bdec>] (snd_pcm_dev_register+0x1ac/0x290) [<c030bdec>] (snd_pcm_dev_register+0x1ac/0x290) from [<c03069c8>] (snd_device_register_all+0x40/0x80) [<c03069c8>] (snd_device_register_all+0x40/0x80) from [<c030192c>] (snd_card_register+0x24/0x228) [<c030192c>] (snd_card_register+0x24/0x228) from [<c03252a4>] (snd_soc_instantiate_card+0x7b4/0x868) [<c03252a4>] (snd_soc_instantiate_card+0x7b4/0x868) from [<c03255c4>] (snd_soc_register_card+0x26c/0x324) [<c03255c4>] (snd_soc_register_card+0x26c/0x324) from [<bf00c0b4>] (kirkwood_spdif_probe+0x88/0xd8 [snd_soc_kirkwood_spdif]) [<bf00c0b4>] (kirkwood_spdif_probe+0x88/0xd8 [snd_soc_kirkwood_spdif]) from [<c0259468>] (platform_drv_probe+0x18/0x1c) [<c0259468>] (platform_drv_probe+0x18/0x1c) from [<c0258144>] (really_probe+0x74/0x1f4) [<c0258144>] (really_probe+0x74/0x1f4) from [<c02583d8>] (driver_probe_device+0x30/0x48) [<c02583d8>] (driver_probe_device+0x30/0x48) from [<c0256a48>] (bus_for_each_drv+0x60/0x8c) [<c0256a48>] (bus_for_each_drv+0x60/0x8c) from [<c0258368>] (device_attach+0x80/0xa4) [<c0258368>] (device_attach+0x80/0xa4) from [<c02577a8>] (bus_probe_device+0x88/0xac) [<c02577a8>] (bus_probe_device+0x88/0xac) from [<c0257c34>] (deferred_probe_work_func+0x6c/0x9c) [<c0257c34>] (deferred_probe_work_func+0x6c/0x9c) from [<c0060a74>] (process_one_work+0x190/0x3f4) [<c0060a74>] (process_one_work+0x190/0x3f4) from [<c0062544>] (worker_thread+0xf4/0x318) [<c0062544>] (worker_thread+0xf4/0x318) from [<c006800c>] (kthread+0xa8/0xb4) [<c006800c>] (kthread+0xa8/0xb4) from [<c000e4a8>] (ret_from_fork+0x14/0x2c) I suspect if I put another codec into the mix, I'll get four of them (one pair for each backend stream.) And /proc/asound ends up looking like this: total 0 lrwxrwxrwx 1 root root 5 Aug 10 16:14 SPDIF -> card0 dr-xr-xr-x 4 root root 0 Aug 10 16:14 card0 -r--r--r-- 1 root root 0 Aug 10 16:14 cards -r--r--r-- 1 root root 0 Aug 10 16:14 devices -rw-r--r-- 1 root root 0 Aug 10 16:14 oss -rw-r--r-- 1 root root 0 Aug 10 16:14 oss -rw-r--r-- 1 root root 0 Aug 10 16:14 oss -r--r--r-- 1 root root 0 Aug 10 16:14 pcm -r--r--r-- 1 root root 0 Aug 10 16:14 timers -r--r--r-- 1 root root 0 Aug 10 16:14 version Yes, three 'oss'es, and /proc/asound/pcm looks like this: 00-00: IEC958 Playback (*) : : playback 1 : capture 1 00-01: ((null)) : : playback 1 : capture 1 which doesn't look very healthy. That ((null)) seems to be down to the lack of a .stream_name for the backend DAI link - but then Liam doesn't have that either. Giving it a stream name fixes the /proc/asound/pcm output: 00-00: Audio Playback (*) : : playback 1 : capture 1 00-01: (IEC958 Playback) : : playback 1 : capture 1 but doesn't stop those warnings (not really surprising, because it isn't the card level which is being complained about). Probably the easiest way to stop that appearing is to just disable OSS compatibility. That's something which should be documented until it is fixed - that DPCM is currently incompatible with OSS. So, there are two issues which still need resolving: 1. The registration of Codec widgets from the platform introduced by Liam in be09ad90e17b79fdb0d513a31e814ff4d42e3dff ASoC: core: Add platform DAI widget mapping 2. How to deal with disconnected front end streams which have no backend provided by their connected codec (in this case, front end can do capture and playback, back end can only do playback.) Both I think are for Liam to solve. The last issue which doesn't block this provided it is documented is: 3. The core ALSA developers need to comment on the multiple /proc/asound/oss creation problem. Here's the hacky patch against my patch set which gets DPCM "working": 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 @@ -36,7 +36,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 = { @@ -49,7 +49,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-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index 05d977a..66fc76c 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -500,8 +500,11 @@ static int kirkwood_i2s_trigger(struct snd_pcm_substream *substream, int cmd, static int kirkwood_i2s_play_i2s(struct snd_soc_dapm_widget *w, struct snd_kcontrol *ctl, int event) { - /* This works because the platform and cpu dai are the same dev */ - struct kirkwood_dma_data *priv = snd_soc_platform_get_drvdata(w->platform); + /* + * The CPU DAI is not available via the widget, so pick + * out private data from the 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; @@ -514,8 +517,11 @@ static int kirkwood_i2s_play_i2s(struct snd_soc_dapm_widget *w, static int kirkwood_i2s_play_spdif(struct snd_soc_dapm_widget *w, struct snd_kcontrol *ctl, int event) { - /* This works because the platform and cpu dai are the same dev */ - struct kirkwood_dma_data *priv = snd_soc_platform_get_drvdata(w->platform); + /* + * The CPU DAI is not available via the widget, so pick + * out private data from the 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; @@ -553,8 +559,7 @@ static int kirkwood_i2s_probe(struct snd_soc_dai *dai) } /* It appears that these can't be attached to the CPU DAI */ - snd_soc_dapm_new_controls(&dai->platform->dapm, - widgets, ARRAY_SIZE(widgets)); + snd_soc_dapm_new_controls(&dai->dapm, widgets, ARRAY_SIZE(widgets)); /* put system in a "safe" state : */ /* disable audio interrupts */ 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 @@ -57,7 +57,7 @@ static struct snd_soc_ops kirkwood_spdif_ops = { }; static const struct snd_soc_dapm_route routes[] = { - { "Playback", NULL, "spdifdo" }, + { "spdif-Playback", NULL, "spdifdo" }, }; static struct snd_soc_dai_link kirkwood_spdif_dai0[] = { @@ -75,9 +75,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", .ops = &kirkwood_spdif_ops, @@ -108,7 +117,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-core.c b/sound/soc/soc-core.c index 48e883e..9176815 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1158,6 +1158,7 @@ static int soc_probe_platform(struct snd_soc_card *card, snd_soc_dapm_new_controls(&platform->dapm, driver->dapm_widgets, driver->num_dapm_widgets); +#if 0 /* This breaks DAPM on Kirkwood */ /* Create DAPM widgets for each DAI stream */ list_for_each_entry(dai, &dai_list, list) { if (dai->dev != platform->dev) @@ -1165,6 +1166,7 @@ static int soc_probe_platform(struct snd_soc_card *card, snd_soc_dapm_new_dai_widgets(&platform->dapm, dai); } +#endif platform->dapm.idle_bias_off = 1; @@ -1362,7 +1364,7 @@ static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order) return -ENODEV; list_add(&cpu_dai->dapm.list, &card->dapm_list); -// snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); + snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); } if (cpu_dai->driver->probe) { 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 Sat, Aug 10, 2013 at 05:11:23PM +0100, Russell King - ARM Linux wrote:
On Tue, Aug 06, 2013 at 02:32:06PM +0100, Mark Brown wrote:
On Tue, Aug 06, 2013 at 12:30:15AM +0100, Russell King - ARM Linux wrote:
I put it to you that DPCM in mainline is incomplete and nonfunctional as it currently stands. Moreover, it requires either those who know that code to continue to develop it, or someone else who understands the direction that this code is supposed to go picks up to complete it.
I'd be most distressed if it were far off working; it's close enough to the out of tree code I've worked with and I know there were drivers in progress when it was submitted. We've certainly had at least one bug fix from the out of tree users, I'd be surprised if we had anything more substantial than bitrot in the current code.
Right, so, I've looked at this again today, and I've sort-of got it working.
I'm not so sure about that: Unable to handle kernel NULL pointer dereference at virtual address 00000008 pgd = d2450000 [00000008] *pgd=12285831, *pte=00000000, *ppte=00000000 Internal error: Oops: 17 [#1] PREEMPT ARM 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 CPU: 0 PID: 2514 Comm: vlc Not tainted 3.10.0+ #652 task: d8102800 ti: d38fa000 task.ti: d38fa000 PC is at snd_pcm_info+0xc8/0xd8 LR is at 0x30232065 pc : [<c030f724>] lr : [<30232065>] psr: a00f0013 sp : d38fbea8 ip : d8c2ead0 fp : c05de6d8 r10: c05de7d0 r9 : fffffdfd r8 : 00000000 r7 : d8c268a8 r6 : d8c26800 r5 : d8c26c00 r4 : d8c2ea00 r3 : 00000000 r2 : d8c2ea00 r1 : 00000001 r0 : d8c26c00 Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user Control: 10c5387d Table: 12450019 DAC: 00000015 Process vlc (pid: 2514, stack limit = 0xd38fa248) Stack: (0xd38fbea8 to 0xd38fc000) bea0: c0af7144 d8c2ea00 d8c26c00 ab5032b8 00000001 c030f768 bec0: 00000000 d8c20000 ab5032b8 c030a67c 0000001b d116b840 d8380330 c1205531 bee0: 0000001b d116b840 d8263fc0 d8c20000 ab5032b8 c03056b0 00000001 c05e6e80 bf00: c05e6e88 c05be828 00020120 00000000 d38fa000 600f0013 00000001 0000001b bf20: d38fa000 00020000 ab5032b8 c0088fec 00000001 00000000 d38fa000 00000000 bf40: 600f0013 ca17c380 0000001b ab5032b8 d8380330 0000001b d38fa000 00020000 bf60: ab5032b8 c00e50bc c00edecc 00020000 ab5032b8 00000001 ca17c380 ab5032b8 bf80: c1205531 c00e5394 ab50366c 00000001 00000000 000120b0 ab50366c 00000036 bfa0: c000e5a8 c000e3e0 00000000 000120b0 0000001b c1205531 ab5032b8 a91a3e10 bfc0: 00000000 000120b0 ab50366c 00000036 ab503454 00000001 00000000 ab5032b8 bfe0: b69a00f4 ab5032a4 b6933109 b6e0d07c a00f0010 0000001b aaaaaaaa aaaaaaaa [<c030f724>] (snd_pcm_info+0xc8/0xd8) from [<c030f768>] (snd_pcm_info_user+0x34/0x9c) [<c030f768>] (snd_pcm_info_user+0x34/0x9c) from [<c030a67c>] (snd_pcm_control_ioctl+0x274/0x280) [<c030a67c>] (snd_pcm_control_ioctl+0x274/0x280) from [<c03056b0>] (snd_ctl_ioctl+0xc0/0x55c) [<c03056b0>] (snd_ctl_ioctl+0xc0/0x55c) from [<c00e50bc>] (do_vfs_ioctl+0x80/0x31c) [<c00e50bc>] (do_vfs_ioctl+0x80/0x31c) from [<c00e5394>] (SyS_ioctl+0x3c/0x60) [<c00e5394>] (SyS_ioctl+0x3c/0x60) from [<c000e3e0>] (ret_fast_syscall+0x0/0x48) Code: e1a00005 e59530dc e3a01001 e1a02004 (e5933008) That is caused by: /* AB: FIXME!!! This is definitely nonsense */ if (runtime) { info->sync = runtime->sync; substream->ops->ioctl(substream, SNDRV_PCM_IOCTL1_INFO, info); } where substream->ops is NULL. So, we're back to square one and DPCM not being correctly functional.
On Sat, 2013-08-10 at 22:13 +0100, Russell King - ARM Linux wrote:
On Sat, Aug 10, 2013 at 05:11:23PM +0100, Russell King - ARM Linux wrote:
On Tue, Aug 06, 2013 at 02:32:06PM +0100, Mark Brown wrote:
On Tue, Aug 06, 2013 at 12:30:15AM +0100, Russell King - ARM Linux wrote:
I put it to you that DPCM in mainline is incomplete and nonfunctional as it currently stands. Moreover, it requires either those who know that code to continue to develop it, or someone else who understands the direction that this code is supposed to go picks up to complete it.
I'd be most distressed if it were far off working; it's close enough to the out of tree code I've worked with and I know there were drivers in progress when it was submitted. We've certainly had at least one bug fix from the out of tree users, I'd be surprised if we had anything more substantial than bitrot in the current code.
Right, so, I've looked at this again today, and I've sort-of got it working.
I'm not so sure about that:
Russell, I'm just back from holiday now and have done a quick re test on Haswell. I can see the correct DAPM path and DPCM state as expected for Haswell, but let me get through my Inbox today and I'll see where we have some differences tomorrow. Liam
On Mon, Aug 12, 2013 at 08:40:15AM +0100, Liam Girdwood wrote:
Russell, I'm just back from holiday now and have done a quick re test on Haswell. I can see the correct DAPM path and DPCM state as expected for Haswell, but let me get through my Inbox today and I'll see where we have some differences tomorrow.
Liam, welcome back. Looking at this last night, I notice this: * Creates a new internal PCM instance with no userspace device or procfs * entries. This is used by ASoC Back End PCMs in order to create a PCM that * will only be used internally by kernel drivers. i.e. it cannot be opened * by userspace. It provides existing ASoC components drivers with a substream * and access to any private data. * * The pcm operators have to be set afterwards to the new instance * via snd_pcm_set_ops(). Note the final paragraph. So, soc-pcm.c does this: /* create the PCM */ if (rtd->dai_link->no_pcm) { snprintf(new_name, sizeof(new_name), "(%s)", rtd->dai_link->stream_name); ret = snd_pcm_new_internal(rtd->card->snd_card, new_name, num, playback, capture, &pcm); } else { ... if (rtd->dai_link->no_pcm) { if (playback) pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd; if (capture) pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd; goto out; } ... if (playback) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &rtd->ops); if (capture) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &rtd->ops); ... out: dev_info(rtd->card->dev, " %s <-> %s mapping ok\n", codec_dai->name, cpu_dai->name); return ret; Hence, soc-pcm.c never sets the ops for the 'internal' stream, which, because it uses the dummy DAI, has both a playback and a capture stream. The above is the only place where the ops are set on the ALSA stream(s) by ASoC, which means there's no way that the internal streams can ever have a non-NULL ops pointer, and suggests a possible reason why I get this oops.
On Mon, 2013-08-12 at 09:28 +0100, Russell King - ARM Linux wrote:
On Mon, Aug 12, 2013 at 08:40:15AM +0100, Liam Girdwood wrote:
Russell, I'm just back from holiday now and have done a quick re test on Haswell. I can see the correct DAPM path and DPCM state as expected for Haswell, but let me get through my Inbox today and I'll see where we have some differences tomorrow.
Liam, welcome back.
Looking at this last night, I notice this:
* Creates a new internal PCM instance with no userspace device or procfs * entries. This is used by ASoC Back End PCMs in order to create a PCM that * will only be used internally by kernel drivers. i.e. it cannot be opened * by userspace. It provides existing ASoC components drivers with a substream * and access to any private data. * * The pcm operators have to be set afterwards to the new instance * via snd_pcm_set_ops().
Note the final paragraph. So, soc-pcm.c does this:
/* create the PCM */ if (rtd->dai_link->no_pcm) { snprintf(new_name, sizeof(new_name), "(%s)", rtd->dai_link->stream_name);
ret = snd_pcm_new_internal(rtd->card->snd_card, new_name, num, playback, capture, &pcm); } else { ... if (rtd->dai_link->no_pcm) { if (playback) pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd; if (capture) pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd; goto out; } ... if (playback) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &rtd->ops);
if (capture) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &rtd->ops); ... out: dev_info(rtd->card->dev, " %s <-> %s mapping ok\n", codec_dai->name, cpu_dai->name); return ret;
Hence, soc-pcm.c never sets the ops for the 'internal' stream, which, because it uses the dummy DAI, has both a playback and a capture stream.
The above is the only place where the ops are set on the ALSA stream(s) by ASoC, which means there's no way that the internal streams can ever have a non-NULL ops pointer, and suggests a possible reason why I get this oops.
Ah, Ok. The OMAP4 ABE driver always had ops so never hit this (probably true for Qualcomm driver too). I think we need a check at some point so that we don't dereference this if no ops are set. Iiuc, there should be no harm in setting the ops (since ASoC checks for NULL) for either playback or capture as a workaround in the short term until the fix is upstream. Liam
On Tue, Aug 13, 2013 at 03:59:12PM +0100, Liam Girdwood wrote:
On Mon, 2013-08-12 at 09:28 +0100, Russell King - ARM Linux wrote:
On Mon, Aug 12, 2013 at 08:40:15AM +0100, Liam Girdwood wrote:
Russell, I'm just back from holiday now and have done a quick re test on Haswell. I can see the correct DAPM path and DPCM state as expected for Haswell, but let me get through my Inbox today and I'll see where we have some differences tomorrow.
Liam, welcome back.
Looking at this last night, I notice this:
* Creates a new internal PCM instance with no userspace device or procfs * entries. This is used by ASoC Back End PCMs in order to create a PCM that * will only be used internally by kernel drivers. i.e. it cannot be opened * by userspace. It provides existing ASoC components drivers with a substream * and access to any private data. * * The pcm operators have to be set afterwards to the new instance * via snd_pcm_set_ops().
Note the final paragraph. So, soc-pcm.c does this:
/* create the PCM */ if (rtd->dai_link->no_pcm) { snprintf(new_name, sizeof(new_name), "(%s)", rtd->dai_link->stream_name);
ret = snd_pcm_new_internal(rtd->card->snd_card, new_name, num, playback, capture, &pcm); } else { ... if (rtd->dai_link->no_pcm) { if (playback) pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd; if (capture) pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd; goto out; } ... if (playback) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &rtd->ops);
if (capture) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &rtd->ops); ... out: dev_info(rtd->card->dev, " %s <-> %s mapping ok\n", codec_dai->name, cpu_dai->name); return ret;
Hence, soc-pcm.c never sets the ops for the 'internal' stream, which, because it uses the dummy DAI, has both a playback and a capture stream.
The above is the only place where the ops are set on the ALSA stream(s) by ASoC, which means there's no way that the internal streams can ever have a non-NULL ops pointer, and suggests a possible reason why I get this oops.
Ah, Ok. The OMAP4 ABE driver always had ops so never hit this (probably true for Qualcomm driver too). I think we need a check at some point so that we don't dereference this if no ops are set.
Iiuc, there should be no harm in setting the ops (since ASoC checks for NULL) for either playback or capture as a workaround in the short term until the fix is upstream.
So where do we stand with all this stuff? As far as I'm concerned, there's nothing more _I_ can do with DPCM to make it work. DPCM seems to be broken. Mark is adamant that the Kirkwood-i2s with SPDIF support _will_ use DPCM and refuses to take patches which don't. So, until DPCM gets fixed, I can't proceed with getting SPDIF for kirkwood into mainline. Either that, or I need you to ack the patches and tell Mark to accept them because they're actually correct and its just the core DPCM code which won't allow them to work (which is what I suspect the real situation to be.)
On Tue, Aug 20, 2013 at 11:25:55AM +0100, Russell King - ARM Linux wrote:
Mark is adamant that the Kirkwood-i2s with SPDIF support _will_ use DPCM and refuses to take patches which don't.
No, that's for simultaneous use of I2S and S/PDIF. Using one or the other shouldn't be a problem, that's just a single DAI and a single PCM which has always been supported.
So, until DPCM gets fixed, I can't proceed with getting SPDIF for kirkwood into mainline.
I would suggest getting the either/or support merged first.
On Tue, Aug 20, 2013 at 12:44:21PM +0100, Mark Brown wrote:
On Tue, Aug 20, 2013 at 11:25:55AM +0100, Russell King - ARM Linux wrote:
Mark is adamant that the Kirkwood-i2s with SPDIF support _will_ use DPCM and refuses to take patches which don't.
No, that's for simultaneous use of I2S and S/PDIF. Using one or the other shouldn't be a problem, that's just a single DAI and a single PCM which has always been supported.
So, until DPCM gets fixed, I can't proceed with getting SPDIF for kirkwood into mainline.
I would suggest getting the either/or support merged first.
Sorry, I don't know how to do that.
On Tue, Aug 20, 2013 at 12:49:49PM +0100, Russell King - ARM Linux wrote:
On Tue, Aug 20, 2013 at 12:44:21PM +0100, Mark Brown wrote:
On Tue, Aug 20, 2013 at 11:25:55AM +0100, Russell King - ARM Linux wrote:
Mark is adamant that the Kirkwood-i2s with SPDIF support _will_ use DPCM and refuses to take patches which don't.
No, that's for simultaneous use of I2S and S/PDIF. Using one or the other shouldn't be a problem, that's just a single DAI and a single PCM which has always been supported.
So, until DPCM gets fixed, I can't proceed with getting SPDIF for kirkwood into mainline.
I would suggest getting the either/or support merged first.
Sorry, I don't know how to do that.
Let's be a little more clear about that: I don't know how to do that because that's the approach taken by _these_ very patches which you've rejected for "abusing the ASoC core". That's why I'm asking Liam directly to effectively overrule you, because I believe your position to be wrong on these patches, and based on an incorrect understanding about DPCM - as I've already evidenced earlier in this thread by illustrating that the DAPM setup I create in the CPU DAI part of these patches is exactly the same as Liam creates in his Haswell driver. Moreover, I believe these patches to be _almost_ correct to what Liam suggests is required, so there's really no reason not to take them. (The only thing difference is that the AIF widgets should be registered against the CPU DAI DAPM context when DPCM becomes usable.) Since you continue to refuse to take the patches, but haven't given any further reasons why, and I've shown your original objections to be provably false, you leave me no other options but to try and bypass you, especially when you have plainly stated that you don't care about Kirkwood stuff.
On Tue, Aug 20, 2013 at 02:31:43PM +0100, Russell King - ARM Linux wrote:
Let's be a little more clear about that: I don't know how to do that because that's the approach taken by _these_ very patches which you've rejected for "abusing the ASoC core". That's why I'm asking Liam
The patches I can recall seeing recently have all had some workarounds in the core which would need to be resolved differently, though it's possible I missed that being done in some version in your mails as there have generally also been a lot of modifications adding debug statements in the core. If you've got code you think is in a good state to submit then please do send it as a normal patch series, the last one I've got here has "ASoC: HACK: avoid creating duplicated widgets" as part of it for example. Just to be clear when I say either/or I'm talking about a DAI driver that can either run in S/PDIF mode or run in I2S mode in a given machine but not support both being hooked up in the same machine. Obviously this isn't the end goal but it might be a useful intermediate step if you find you are are blocked.
Since you continue to refuse to take the patches, but haven't given any further reasons why, and I've shown your original objections to be provably false, you leave me no other options but to try and bypass
To reiterate please submit patches if you believe everything is OK now and then let's go through and review them.
you, especially when you have plainly stated that you don't care about Kirkwood stuff.
I think you'll find that anything I've said along those lines has been in relation to your strongly worded requests for me to make changes for you.
On Tue, Aug 20, 2013 at 07:50:19PM +0100, Mark Brown wrote:
On Tue, Aug 20, 2013 at 02:31:43PM +0100, Russell King - ARM Linux wrote:
Let's be a little more clear about that: I don't know how to do that because that's the approach taken by _these_ very patches which you've rejected for "abusing the ASoC core". That's why I'm asking Liam
The patches I can recall seeing recently have all had some workarounds in the core which would need to be resolved differently, though it's possible I missed that being done in some version in your mails as there have generally also been a lot of modifications adding debug statements in the core.
The "workarounds in the core" are because there's bugs in the core that I have no idea how to solve. You are allegedly the maintainer for the core code, and so you should understand that code, so you are best placed to say how the core code should be fixed. I'm willing to do the patch generation to fix them but *you* need to give some guidance here - something that you seem incapable to do. At the moment, the only fix I can see being workable is to comment out the broken bit in the core code. If the problem is that you don't understand the issue, then you could try replying with some questions about it. If the problem is that you don't understand the code, well... there's not much point in continuing this discussion until you've had time to study and understand that code.
If you've got code you think is in a good state to submit then please do send it as a normal patch series, the last one I've got here has "ASoC: HACK: avoid creating duplicated widgets" as part of it for example.
That patch still hasn't gone away, and is still required, because there has been no guidance or comments about the problem. Let's explain it yet again... You have said "there is no problem registering the platform and the CPU dai from the same device structure". Let's assume that's a fact and see what happens in the core code: static int soc_probe_platform(struct snd_soc_card *card, struct snd_soc_platform *platform) { /* Create DAPM widgets for each DAI stream */ list_for_each_entry(dai, &dai_list, list) { if (dai->dev != platform->dev) continue; snd_soc_dapm_new_dai_widgets(&platform->dapm, dai); } } static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order) { if (!cpu_dai->probed && cpu_dai->driver->probe_order == order) { if (!cpu_dai->codec) { cpu_dai->dapm.card = card; if (!try_module_get(cpu_dai->dev->driver->owner)) return -ENODEV; list_add(&cpu_dai->dapm.list, &card->dapm_list); snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); } Now, the CPU DAI is added to the dai_list (it has to be there to be found so the DAI link can bind it, and so soc_probe_link_dais() can be called.) Think about what happens with the above code if platform->dev is the same as the device used for the CPU DAI (dai->dev) - which can happen when the platform and CPU DAI are registered from the same platform_device, which you claim is legal with ASoC. Now, look at snd_soc_dapm_new_dai_widgets(): int snd_soc_dapm_new_dai_widgets(struct snd_soc_dapm_context *dapm, struct snd_soc_dai *dai) { if (dai->driver->playback.stream_name) { ... dai->playback_widget = w; } if (dai->driver->capture.stream_name) { ... dai->capture_widget = w; } What happens if the widgets which are bound to are the first set that are created, but they're overwritten when the second set get created? (And that _does_ happen.) The second set are the ones activated when the audio device is opened, not the first set. Now, there's nothing new in the above, I've already explained all the above to you several times. I've had nothing of any help what so ever back from you on this. I've asked you how to solve this. I've had absolutely nothing back. So what am I supposed to do here? Stuff doesn't work with the core code how it is, so I took the only solution _you_ left me by your silence, which is to hack the core code. At this point, you leave me with no other conclusion but to assume that the reason you are being so unhelpful is that you don't understand this code, and that you don't know what the right solution is. I dare you to tell me I'm wrong: and the only thing that will convince me that I'm wrong is if you tell me how you'd like the above issue to be solved.
Just to be clear when I say either/or I'm talking about a DAI driver that can either run in S/PDIF mode or run in I2S mode in a given machine but not support both being hooked up in the same machine. Obviously this isn't the end goal but it might be a useful intermediate step if you find you are are blocked.
Well, it doesn't take much to realise that the CPU DAI needs to know whether the _single_ attached codec is SPDIF or I2S. How it does that is via which AIFs are bound - and as DPCM doesn't work at the moment, you can only do that by having _one_ DAI link specified, and DAPM routes between the AIFs and the codecs streams. If you know a better way, you could try saying what that is, because at the moment what I've presented in these patches is the best that I can do with the limited knowledge of ASoC. I'm not going to litter the driver with #ifdef's to work around this when there's a perfectly good way to work around it at runtime - which I've proven can work if the core code issue I point out above gets fixed.
Since you continue to refuse to take the patches, but haven't given any further reasons why, and I've shown your original objections to be provably false, you leave me no other options but to try and bypass
To reiterate please submit patches if you believe everything is OK now and then let's go through and review them.
You mean like your poor review attempt on the first round where you completely ignored the issue with the core code (the HACK patch)? You mean like your poor review stating that the DAPM infrastructure I added in the CPU level code was all wrong, but it turns out that it's exactly the same as Liam's DPCM setup. Why should I submit a new round of patches which haven't changed in any meaningful way (which I've already stated) for another review by someone who seems not to know what they're talking about? Can't you go back and look at the existing set again and provide a better quality of review, maybe providing some suggestions on the core issue which I keep pointing out? This is precisely why I've called for Liam to look at them instead; I feel that I will get a much better quality of review from Liam, and maybe even some helpful suggestions about how to solve some of the remaining issues - something which has been totally lacking in every response from you.
you, especially when you have plainly stated that you don't care about Kirkwood stuff.
I think you'll find that anything I've said along those lines has been in relation to your strongly worded requests for me to make changes for you.
No, my strongly worded emails are directed at you not to ask you to make changes for me, but because you are being obstructive, uncooperative, and unhelpful. Apart from "use DPCM" and "DAPM is just a graph walk" I've had nothing of any real use what so ever back from you. Much of my effort has been based around my own interpretations and debugging of the ASoC code inspite of you, which may or may not be correct. However, one thing I do know is that I've pointed out the bug above multiple times to you and there's still no movement on it - which just confirms my conclusion that you are just being plain obstructive.
On Tue, 2013-08-20 at 21:18 +0100, Russell King - ARM Linux wrote:
On Tue, Aug 20, 2013 at 07:50:19PM +0100, Mark Brown wrote:
On Tue, Aug 20, 2013 at 02:31:43PM +0100, Russell King - ARM Linux wrote:
Let's be a little more clear about that: I don't know how to do that because that's the approach taken by _these_ very patches which you've rejected for "abusing the ASoC core". That's why I'm asking Liam
The patches I can recall seeing recently have all had some workarounds in the core which would need to be resolved differently, though it's possible I missed that being done in some version in your mails as there have generally also been a lot of modifications adding debug statements in the core.
The "workarounds in the core" are because there's bugs in the core that I have no idea how to solve. You are allegedly the maintainer for the core code, and so you should understand that code, so you are best placed to say how the core code should be fixed. I'm willing to do the patch generation to fix them but *you* need to give some guidance here - something that you seem incapable to do. At the moment, the only fix I can see being workable is to comment out the broken bit in the core code.
I'll fix this issue as I've replied privately, but you know it's not appropriate to just comment stuff out in core code (especially if you don't fully understand it). I'm sure you would complain loudly to me if I tried to do a similar HACK in the ARM core.
If the problem is that you don't understand the issue, then you could try replying with some questions about it.
If the problem is that you don't understand the code, well... there's not much point in continuing this discussion until you've had time to study and understand that code.
If you've got code you think is in a good state to submit then please do send it as a normal patch series, the last one I've got here has "ASoC: HACK: avoid creating duplicated widgets" as part of it for example.
That patch still hasn't gone away, and is still required, because there has been no guidance or comments about the problem. Let's explain it yet again...
You have said "there is no problem registering the platform and the CPU dai from the same device structure". Let's assume that's a fact and see what happens in the core code:
static int soc_probe_platform(struct snd_soc_card *card, struct snd_soc_platform *platform) { /* Create DAPM widgets for each DAI stream */ list_for_each_entry(dai, &dai_list, list) { if (dai->dev != platform->dev) continue;
snd_soc_dapm_new_dai_widgets(&platform->dapm, dai); } }
static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order) { if (!cpu_dai->probed && cpu_dai->driver->probe_order == order) { if (!cpu_dai->codec) { cpu_dai->dapm.card = card; if (!try_module_get(cpu_dai->dev->driver->owner)) return -ENODEV;
list_add(&cpu_dai->dapm.list, &card->dapm_list); snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); }
Now, the CPU DAI is added to the dai_list (it has to be there to be found so the DAI link can bind it, and so soc_probe_link_dais() can be called.)
Think about what happens with the above code if platform->dev is the same as the device used for the CPU DAI (dai->dev) - which can happen when the platform and CPU DAI are registered from the same platform_device, which you claim is legal with ASoC.
Now, look at snd_soc_dapm_new_dai_widgets():
int snd_soc_dapm_new_dai_widgets(struct snd_soc_dapm_context *dapm, struct snd_soc_dai *dai) { if (dai->driver->playback.stream_name) { ... dai->playback_widget = w; } if (dai->driver->capture.stream_name) { ... dai->capture_widget = w; }
What happens if the widgets which are bound to are the first set that are created, but they're overwritten when the second set get created? (And that _does_ happen.) The second set are the ones activated when the audio device is opened, not the first set.
Now, there's nothing new in the above, I've already explained all the above to you several times. I've had nothing of any help what so ever back from you on this. I've asked you how to solve this. I've had absolutely nothing back. So what am I supposed to do here? Stuff doesn't work with the core code how it is, so I took the only solution _you_ left me by your silence, which is to hack the core code.
It does seem that your configuration is different to the configurations that work well on Haswell, OMAP4 and Qualcomm and that's probably why you are the only person reporting this atm. I also think the tight coupling between the I2S and SPDIF HW made your problem far more complex and therefore more difficult (for me at least) to follow when the signal to noise ratio of this and related threads started to deteriorate. Both Mark and are are happy to fix things, but please remember that we can't just jump and schedule this work in as top priority, we have to prioritise work on severity and impact alongside that of our employers and customers. I'm sure if things were the other way around (e.g the problem was in the ARM core) then Mark would have to wait for you to respond and fix the issue in your time frame. I'm also certain Mark would not start making the conversation personal either. As I've said, I'll do a proper fix for patch 4 and CC you on the submission. The rest of the series looked ok and then I'm sure Mark will take it. Liam
On Thu, Aug 22, 2013 at 08:22:50PM +0100, Liam Girdwood wrote:
On Tue, 2013-08-20 at 21:18 +0100, Russell King - ARM Linux wrote:
On Tue, Aug 20, 2013 at 07:50:19PM +0100, Mark Brown wrote:
On Tue, Aug 20, 2013 at 02:31:43PM +0100, Russell King - ARM Linux wrote:
Let's be a little more clear about that: I don't know how to do that because that's the approach taken by _these_ very patches which you've rejected for "abusing the ASoC core". That's why I'm asking Liam
The patches I can recall seeing recently have all had some workarounds in the core which would need to be resolved differently, though it's possible I missed that being done in some version in your mails as there have generally also been a lot of modifications adding debug statements in the core.
The "workarounds in the core" are because there's bugs in the core that I have no idea how to solve. You are allegedly the maintainer for the core code, and so you should understand that code, so you are best placed to say how the core code should be fixed. I'm willing to do the patch generation to fix them but *you* need to give some guidance here - something that you seem incapable to do. At the moment, the only fix I can see being workable is to comment out the broken bit in the core code.
I'll fix this issue as I've replied privately, but you know it's not appropriate to just comment stuff out in core code (especially if you don't fully understand it). I'm sure you would complain loudly to me if I tried to do a similar HACK in the ARM core.
Hang on, tell me exactly *where* I've asked for the hack to be merged. The answer is: I haven't. What I've been asking for all along is how this should be solved, and getting nowhere - whether I ask in a reasonable and calm manner or have to take it to extremes like I have done now.
If the problem is that you don't understand the issue, then you could try replying with some questions about it.
If the problem is that you don't understand the code, well... there's not much point in continuing this discussion until you've had time to study and understand that code.
If you've got code you think is in a good state to submit then please do send it as a normal patch series, the last one I've got here has "ASoC: HACK: avoid creating duplicated widgets" as part of it for example.
That patch still hasn't gone away, and is still required, because there has been no guidance or comments about the problem. Let's explain it yet again...
You have said "there is no problem registering the platform and the CPU dai from the same device structure". Let's assume that's a fact and see what happens in the core code:
static int soc_probe_platform(struct snd_soc_card *card, struct snd_soc_platform *platform) { /* Create DAPM widgets for each DAI stream */ list_for_each_entry(dai, &dai_list, list) { if (dai->dev != platform->dev) continue;
snd_soc_dapm_new_dai_widgets(&platform->dapm, dai); } }
static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order) { if (!cpu_dai->probed && cpu_dai->driver->probe_order == order) { if (!cpu_dai->codec) { cpu_dai->dapm.card = card; if (!try_module_get(cpu_dai->dev->driver->owner)) return -ENODEV;
list_add(&cpu_dai->dapm.list, &card->dapm_list); snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); }
Now, the CPU DAI is added to the dai_list (it has to be there to be found so the DAI link can bind it, and so soc_probe_link_dais() can be called.)
Think about what happens with the above code if platform->dev is the same as the device used for the CPU DAI (dai->dev) - which can happen when the platform and CPU DAI are registered from the same platform_device, which you claim is legal with ASoC.
Now, look at snd_soc_dapm_new_dai_widgets():
int snd_soc_dapm_new_dai_widgets(struct snd_soc_dapm_context *dapm, struct snd_soc_dai *dai) { if (dai->driver->playback.stream_name) { ... dai->playback_widget = w; } if (dai->driver->capture.stream_name) { ... dai->capture_widget = w; }
What happens if the widgets which are bound to are the first set that are created, but they're overwritten when the second set get created? (And that _does_ happen.) The second set are the ones activated when the audio device is opened, not the first set.
Now, there's nothing new in the above, I've already explained all the above to you several times. I've had nothing of any help what so ever back from you on this. I've asked you how to solve this. I've had absolutely nothing back. So what am I supposed to do here? Stuff doesn't work with the core code how it is, so I took the only solution _you_ left me by your silence, which is to hack the core code.
It does seem that your configuration is different to the configurations that work well on Haswell, OMAP4 and Qualcomm and that's probably why you are the only person reporting this atm. I also think the tight coupling between the I2S and SPDIF HW made your problem far more complex and therefore more difficult (for me at least) to follow when the signal to noise ratio of this and related threads started to deteriorate.
Erm, you have completely the wrong end of the stick here. This has nothing to do with I2S and SPDIF at all. It's about having the _same_ struct device assocated with both the platform/DMA backend, registered by snd_soc_register_platform() and the CPU DAI registered via snd_soc_register_component(). SPDIF or I2S doesn't come into this bug.
On Thu, 2013-08-22 at 21:16 +0100, Russell King - ARM Linux wrote:
On Thu, Aug 22, 2013 at 08:22:50PM +0100, Liam Girdwood wrote:
On Tue, 2013-08-20 at 21:18 +0100, Russell King - ARM Linux wrote:
On Tue, Aug 20, 2013 at 07:50:19PM +0100, Mark Brown wrote:
On Tue, Aug 20, 2013 at 02:31:43PM +0100, Russell King - ARM Linux wrote:
Let's be a little more clear about that: I don't know how to do that because that's the approach taken by _these_ very patches which you've rejected for "abusing the ASoC core". That's why I'm asking Liam
The patches I can recall seeing recently have all had some workarounds in the core which would need to be resolved differently, though it's possible I missed that being done in some version in your mails as there have generally also been a lot of modifications adding debug statements in the core.
The "workarounds in the core" are because there's bugs in the core that I have no idea how to solve. You are allegedly the maintainer for the core code, and so you should understand that code, so you are best placed to say how the core code should be fixed. I'm willing to do the patch generation to fix them but *you* need to give some guidance here - something that you seem incapable to do. At the moment, the only fix I can see being workable is to comment out the broken bit in the core code.
I'll fix this issue as I've replied privately, but you know it's not appropriate to just comment stuff out in core code (especially if you don't fully understand it). I'm sure you would complain loudly to me if I tried to do a similar HACK in the ARM core.
Hang on, tell me exactly *where* I've asked for the hack to be merged. The answer is: I haven't. What I've been asking for all along is how this should be solved, and getting nowhere - whether I ask in a reasonable and calm manner or have to take it to extremes like I have done now.
You asked me privately to Ack the series so you could carry it in your own tree for upstreaming. Sorry if I misunderstood this request, but it seemed straightforward.
If the problem is that you don't understand the issue, then you could try replying with some questions about it.
If the problem is that you don't understand the code, well... there's not much point in continuing this discussion until you've had time to study and understand that code.
If you've got code you think is in a good state to submit then please do send it as a normal patch series, the last one I've got here has "ASoC: HACK: avoid creating duplicated widgets" as part of it for example.
That patch still hasn't gone away, and is still required, because there has been no guidance or comments about the problem. Let's explain it yet again...
You have said "there is no problem registering the platform and the CPU dai from the same device structure". Let's assume that's a fact and see what happens in the core code:
static int soc_probe_platform(struct snd_soc_card *card, struct snd_soc_platform *platform) { /* Create DAPM widgets for each DAI stream */ list_for_each_entry(dai, &dai_list, list) { if (dai->dev != platform->dev) continue;
snd_soc_dapm_new_dai_widgets(&platform->dapm, dai); } }
static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order) { if (!cpu_dai->probed && cpu_dai->driver->probe_order == order) { if (!cpu_dai->codec) { cpu_dai->dapm.card = card; if (!try_module_get(cpu_dai->dev->driver->owner)) return -ENODEV;
list_add(&cpu_dai->dapm.list, &card->dapm_list); snd_soc_dapm_new_dai_widgets(&cpu_dai->dapm, cpu_dai); }
Now, the CPU DAI is added to the dai_list (it has to be there to be found so the DAI link can bind it, and so soc_probe_link_dais() can be called.)
Think about what happens with the above code if platform->dev is the same as the device used for the CPU DAI (dai->dev) - which can happen when the platform and CPU DAI are registered from the same platform_device, which you claim is legal with ASoC.
Now, look at snd_soc_dapm_new_dai_widgets():
int snd_soc_dapm_new_dai_widgets(struct snd_soc_dapm_context *dapm, struct snd_soc_dai *dai) { if (dai->driver->playback.stream_name) { ... dai->playback_widget = w; } if (dai->driver->capture.stream_name) { ... dai->capture_widget = w; }
What happens if the widgets which are bound to are the first set that are created, but they're overwritten when the second set get created? (And that _does_ happen.) The second set are the ones activated when the audio device is opened, not the first set.
Now, there's nothing new in the above, I've already explained all the above to you several times. I've had nothing of any help what so ever back from you on this. I've asked you how to solve this. I've had absolutely nothing back. So what am I supposed to do here? Stuff doesn't work with the core code how it is, so I took the only solution _you_ left me by your silence, which is to hack the core code.
It does seem that your configuration is different to the configurations that work well on Haswell, OMAP4 and Qualcomm and that's probably why you are the only person reporting this atm. I also think the tight coupling between the I2S and SPDIF HW made your problem far more complex and therefore more difficult (for me at least) to follow when the signal to noise ratio of this and related threads started to deteriorate.
Erm, you have completely the wrong end of the stick here. This has nothing to do with I2S and SPDIF at all.
It's about having the _same_ struct device assocated with both the platform/DMA backend, registered by snd_soc_register_platform() and the CPU DAI registered via snd_soc_register_component(). SPDIF or I2S doesn't come into this bug.
I was trying to highlight that your HW is the only one with the shared struct device and this iiuc was due to the tight coupling between I2s and SPDIF. Liam
On Fri, Aug 23, 2013 at 01:13:14PM +0100, Liam Girdwood wrote:
On Thu, 2013-08-22 at 21:16 +0100, Russell King - ARM Linux wrote:
On Thu, Aug 22, 2013 at 08:22:50PM +0100, Liam Girdwood wrote:
I'll fix this issue as I've replied privately, but you know it's not appropriate to just comment stuff out in core code (especially if you don't fully understand it). I'm sure you would complain loudly to me if I tried to do a similar HACK in the ARM core.
Hang on, tell me exactly *where* I've asked for the hack to be merged. The answer is: I haven't. What I've been asking for all along is how this should be solved, and getting nowhere - whether I ask in a reasonable and calm manner or have to take it to extremes like I have done now.
You asked me privately to Ack the series so you could carry it in your own tree for upstreaming. Sorry if I misunderstood this request, but it seemed straightforward.
What I'm after are acks for those patches which are acceptable - which I believe is the entire series with the exception of the HACK patch.
Erm, you have completely the wrong end of the stick here. This has nothing to do with I2S and SPDIF at all.
It's about having the _same_ struct device assocated with both the platform/DMA backend, registered by snd_soc_register_platform() and the CPU DAI registered via snd_soc_register_component(). SPDIF or I2S doesn't come into this bug.
I was trying to highlight that your HW is the only one with the shared struct device and this iiuc was due to the tight coupling between I2s and SPDIF.
Not at all - the tight coupling of the DMA and the CPU DAI backend is merely because there is no actual separation of those two items: the DMA engine is tightly coupled with the audio interface itself. They both share the same register space, and they both make use of the same iomapping created by kirkwood_i2s_dev_probe() in the current tree. The split of the DMA backend from the CPU DAI backend is something which early ASoC forced, but that has "mostly" been fixed with (as far as I'm currently aware through testing) the problem I've been trying to get addressed for the last month. Moreover, the split is undesirable when switching to device tree, and as all Marvell support is moving to device tree, having the DMA and CPU DAI backends combined (as Mark has told me several times is fully supported in ASoC) will be required. Bear in mind that ARM is no longer accepting new non-DT support for Marvell stuff, so this is something which must be fixed if any new Marvell platform is to be supported.
On Fri, Aug 23, 2013 at 01:58:03PM +0100, Russell King - ARM Linux wrote:
On Fri, Aug 23, 2013 at 01:13:14PM +0100, Liam Girdwood wrote:
You asked me privately to Ack the series so you could carry it in your own tree for upstreaming. Sorry if I misunderstood this request, but it seemed straightforward.
What I'm after are acks for those patches which are acceptable - which I believe is the entire series with the exception of the HACK patch.
If there no dependency then why is that patch included as part of this series, especially so early on? Obviously we shouldn't cause problems for existing machines in mainline so I stopped applying patches which seemed like they might depend on that one. I had at the time been expecting a revised version of the series to follow in due course with a better fix as at the time the hack was sent you'd only just determined what the problem was.
The split of the DMA backend from the CPU DAI backend is something which early ASoC forced, but that has "mostly" been fixed with (as far as I'm currently aware through testing) the problem I've been
Essentially all the dmaengine based platforms in mainline use a shared device for DMA and DAI; I'm fairly sure someone would have mentioned if there were problems. As you have been repeatedly told the Kirkwood drivers are the first drivers submitted to mainline which use DPCM and therefore it is not surprising that there are a few issues which need to be worked through, there were a few revisions to the framework which went in as a result of review during the mainline merge. The problem you are seeing here is due to this being the first platform with a *shared* device to use DPCM.
On Fri, Aug 23, 2013 at 05:58:00PM +0100, Mark Brown wrote:
On Fri, Aug 23, 2013 at 01:58:03PM +0100, Russell King - ARM Linux wrote:
The split of the DMA backend from the CPU DAI backend is something which early ASoC forced, but that has "mostly" been fixed with (as far as I'm currently aware through testing) the problem I've been
Essentially all the dmaengine based platforms in mainline use a shared device for DMA and DAI; I'm fairly sure someone would have mentioned if there were problems.
As you have been repeatedly told the Kirkwood drivers are the first drivers submitted to mainline which use DPCM and therefore it is not surprising that there are a few issues which need to be worked through, there were a few revisions to the framework which went in as a result of review during the mainline merge. The problem you are seeing here is due to this being the first platform with a *shared* device to use DPCM.
So that's why it fails _without_ any DPCM stuff? That's why the codecs fail to have their set_bias stuff called? Look Mark, frankly, shut your fucking mouth up about DPCM. This bug has nothing what so ever to do with DPCM, and the more times you try and state that doesn't change that *FACT*. And it is a FACT. You've had this problem described by IRC, including extracts from the ASoC code indicating where things go wrong. I've shown you the debug I've used. I've shown you the result of that debug. You've had descriptions of this problem via email too. Yet you refuse to acknowledge that there could possibly be a problem here. All the time that you insist that there isn't a problem against factual evidence, you're just making yourself look totally incompetent and obstructive - not only that but you're making yourself look like a total idiot. Your comments above are just PLAIN WRONG. I'm sick to death with you. You are not a fit person to hold the maintainership role for ASoC in my opinion, and you are long past having any ability to change my opinion on that anymore, given your obtuseness against this bug. I live in hope that one day you'll recognise your mistake here and appologise to me - but frankly I think that's something that you would find absolutely impossible to do.
On Fri, Aug 23, 2013 at 06:45:05PM +0100, Russell King - ARM Linux wrote:
Look Mark, frankly, shut your fucking mouth up about DPCM. This bug has nothing what so ever to do with DPCM, and the more times you try and state that doesn't change that *FACT*. And it is a FACT.
You must know that this and the rest of your e-mail are not an appropriate or constructive way to interact with people. I now regret that have not confronted you on this more openly and directly before but unfortunately I had hoped that stepping back would help with deescalation. I know I have needed to take a step back on several occasions myself in order to be calmer in my response.
So that's why it fails _without_ any DPCM stuff? That's why the codecs fail to have their set_bias stuff called?
So, it is true that the widgets get double created in all cases where the same struct device is used for both platform and DAI. However if DPCM is not used nothing really notices since the SoC side widgets don't do anything useful. The relevance of DPCM is that it tries to use the widgets and therefore causes practical problems in normal use, otherwise it's just a memory leak on init. As you know the issue with the DAPMless CODECs was unrelated to this, was due to poor test coverage of such devices and has been resolved by making DAPM mandatory for CODECs to eliminate the possibility of similar regressions caused by the need for special casing.
You've had this problem described by IRC, including extracts from the ASoC code indicating where things go wrong. I've shown you the debug I've used. I've shown you the result of that debug. You've had descriptions of this problem via email too. Yet you refuse to acknowledge that there could possibly be a problem here.
I am fairly sure that I have agreed that there is a bug and at the very least not said that everything is fine. Just for the record there clearly is a bug with double creation of DAI widgets when the same struct device is used to register both a DAI and a platform. You will also recall that Liam said he would provide a patch when he has time. As I have told you before your generally confrontational and demanding approach is not a good one for getting help from people, it doesn't help push your problems up the priority list or generate positive attention. Especially with the more extreme examples it can actively work against engagement; at an unconscious level it makes things less fun and at a conscious level not only are breaks for calm needed but I know that I at least have no desire to encourage anyone to believe that this is a good way of getting results.
On 23 August 2013 19:45, Russell King - ARM Linux <linux@arm.linux.org.uk>wrote:
On Fri, Aug 23, 2013 at 05:58:00PM +0100, Mark Brown wrote:
On Fri, Aug 23, 2013 at 01:58:03PM +0100, Russell King - ARM Linux wrote:
The split of the DMA backend from the CPU DAI backend is something which early ASoC forced, but that has "mostly" been fixed with (as far as I'm currently aware through testing) the problem I've been
Essentially all the dmaengine based platforms in mainline use a shared device for DMA and DAI; I'm fairly sure someone would have mentioned if there were problems.
As you have been repeatedly told the Kirkwood drivers are the first drivers submitted to mainline which use DPCM and therefore it is not surprising that there are a few issues which need to be worked through, there were a few revisions to the framework which went in as a result of review during the mainline merge. The problem you are seeing here is due to this being the first platform with a *shared* device to use DPCM.
So that's why it fails _without_ any DPCM stuff? That's why the codecs fail to have their set_bias stuff called?
Look Mark, frankly, shut your fucking mouth up about DPCM.
I was about to answer your other emails and work on the fix tonight but was very disappointed in you when I read this statement. There is really no need to be abusive to anybody on the list and this sort of abuse is very unprofessional in front of the community and your customers. I'm sorry, but my motivation to fix the issue tonight has gone. My free time is short this week as I'm travelling, so I wont be able to look at this again until Monday now. This bug has
nothing what so ever to do with DPCM, and the more times you try and state that doesn't change that *FACT*. And it is a FACT.
You've had this problem described by IRC, including extracts from the ASoC code indicating where things go wrong. I've shown you the debug I've used. I've shown you the result of that debug. You've had descriptions of this problem via email too. Yet you refuse to acknowledge that there could possibly be a problem here.
All the time that you insist that there isn't a problem against factual evidence, you're just making yourself look totally incompetent and obstructive - not only that but you're making yourself look like a total idiot.
I think you are doing a lot worse to your reputation with abusive statements. Please remember that your customers do read this list.
I'm sick to death with you.
You are not a fit person to hold the maintainership role for ASoC in my opinion, and you are long past having any ability to change my opinion on that anymore, given your obtuseness against this bug.
I completely disagree with you here. Do you not see how statements like this will just end up making you look like someone who is very difficult to work with when faced with a technical disagreement ? I had acknowledged to you that this had not been tested for your configuration before you sent this email and that I would fix the issue. Liam
On Thu, Aug 29, 2013 at 11:12:35PM +0200, Liam Girdwood wrote:
Do you not see how statements like this will just end up making you look like someone who is very difficult to work with when faced with a technical disagreement ? I had acknowledged to you that this had not been tested for your configuration before you sent this email and that I would fix the issue.
Sorry, you're making the assumption that there's a _technical_ disagreement here. That's not how I view any of this; I believe that what's been going on has been a stream of misinformation and obstruction which is a totally different issue. While it may be true that the bug affects DPCM _too_, it is not DPCM which is the problem. That's pretty obvious when you consider that the patch series which I posted (which is non-DPCM) trips over the bug as well. Is it reasonable to continually claim that a bug is due to DPCM when the code being used which provokes it doesn't make any use of DPCM. It's that kind of misinformation that I can't stand, and I will make it plainly obvious when there's blatent misinformation being banded out. As a result of this, I can never trust anything Mark says; that's something which I established about a fortnight ago when this same old wrong story was trotted out for the I don't know how many time. Of course people are going to get frustrated and annoyed if that's the kind of behaviour that's being presented: when the facts show that something is wrong, repeating it many times doesn't make it any more right. All that it does is waste time. I note that Mark has finally agreed earlier this week for the *first* time (in reply to the email you quoted) that the bug doesn't necessarily have anything to do with DPCM - which is at last some progress. To have taken four weeks to get to that point is pretty direbolical. What about the oops bug in ALSA PCM that DPCM causes... am I going to have to spend another month trying to get that bug recognised? (That has been ignored too.) If this is what it takes, then I'm seriously going to consider rewriting this as a plain ALSA driver and ditch ASoC; ASoC seems to be too buggy at the moment and it takes too long just to get bugs recognised. It's not the actual fixing that I'm particularly worried about: it's a display that the maintainer correctly recognises the bug, and has the capability to fix it - and if they don't have time, they can communicate how they'd like it to be fixed or pass it to someone who does. Every time I've asked about how to fix the widget overwriting bug, the replies (if any) have avoided the question or given the same old misinformation. You only need to go back and look at the initial patch submission to see that; the patch containing my hacked workaround has never had any kind of response. Is it acceptable to think "the submitter doesn't expect that to be merged so I'll ignore it"? The fact that any kind of hack appears in a patch series means that there's a problem which needs to be solved, and the submitter may not know how it should be fixed. That's definitely the case with this patch set: more so since I'd already asked the question about how it should be solved and already got nowhere. All in all, there's been: 1. The combining of the CPU and platform code(s) under one device is absolutely no problem for ASoC, there's multiple drivers which do this already. While this is a true statement, it creates the bug at the heart of this thread - and the bug _occuring_ has nothing to do with DPCM. The bug is caused solely by the combining of the two components. 2. Stream names in DAIs don't have to be globally unique. This is false if you want to try and bind to specific streams - even more so when you can end up with multiple codecs with DPCM, where all their output streams are called "Playback". It may be that the stream names are not supposed to be used in DAPM routes, but that is not documented, and to date no one has made such a statement. 3. The infamous flippant "DAPM is just a graph walk" statement - it's more complicated than that because DAPM widgets have types which affect how the graph is walked, and the activation in multiple points in the graph make it more difficult to understand. As for the DAPM contexts which affect how the graph gets built, and in some cases whether some links even get created. No, it's more than "just a graph walk" - while true at a very basic level, it provides nothing of any use to anyone trying to understand this code - the response may as well have been "It's written in C". (In the course of this, I've given an overview of it to a small number of people, and it takes a lot more than six words to even begin to describe what it is.) 4. By adding the AIFs and routes, I'm bypassing/abusing the ASoC DAI core. If this were to be the case, why does your DPCM driver set them up. Therefore, my conclusion is that Mark was wrong about this too. Yes, it may be that for the time being it's not that appealing, because as DPCM doesn't work in mainline, I'm unable to separate the DAI link between the CPU and the Codec. That doesn't make the creation of the routes any more wrong when they're the exact same routes which need to be created for DPCM - especially when they're relied upon for there to be any audio output what so ever from the driver. 5. Mark's "suggestion" that I can get this driver to work without this hack... I still don't see how, and Mark hasn't effectively explained how either - and I'm left wondering whether Mark even read the patch to see how it worked, and to see how and why it uses the AIF widgets, which brings into question the "review" which was done. While the simple solution would be to add some ifdefs, that is not acceptable when you consider that this driver is already part of a multi-platform kernel - hard-coding it to one kind of output would definitely cause regressions. Also consider that this patch series was already my best effort at getting this going without using DPCM and without adding ifdefs without causing regressions for other Kirkwood platforms. If those issues mean that I'm difficult to work with... well... I suspect anyone in my position receiving those kinds of responses would also end up getting extremely frustrated, as I have.
On Fri, Aug 30, 2013 at 12:27:05PM +0100, Russell King - ARM Linux wrote:
1. The combining of the CPU and platform code(s) under one device is absolutely no problem for ASoC, there's multiple drivers which do this already. While this is a true statement, it creates the bug at the heart of this thread - and the bug _occuring_ has nothing to do with DPCM. The bug is caused solely by the combining of the two components.
Right, having gone back and looked at this in-depth again, and tried some experiments, I no longer need this problem fixed for the non-DPCM code to work, but I do still need to do this:
4. By adding the AIFs and routes, I'm bypassing/abusing the ASoC DAI core. If this were to be the case, why does your DPCM driver set them up. Therefore, my conclusion is that Mark was wrong about this too. Yes, it may be that for the time being it's not that appealing, because as DPCM doesn't work in mainline, I'm unable to separate the DAI link between the CPU and the Codec. That doesn't make the creation of the routes any more wrong when they're the exact same routes which need to be created for DPCM - especially when they're relied upon for there to be any audio output what so ever from the driver.
so I can detect which output paths to enable. I will be posting updated patches shortly, which will just be those which Mark hasn't taken yet.
On Sat, Aug 10, 2013 at 05:11:23PM +0100, Russell King - ARM Linux wrote:
It would appear that the problem is that the AIF widgets are not finding their stream - the implicit routes are not being created. It _appears_ that the strean name in widgets are only ever connected to the streams within the same DAPM context.
That shouldn't be the case in general - DAPM binds first to a widget in the local context for disambiguation and then falls back to a search of the gobal context. However it is possible that you've got an ordering issue during registration.
So, it's because the capture isn't wired up. The capture isn't wired up on this platform, so how do I tell ASoC not to bother with the capture side with DPCM? Creating a link for the capture side to the dummy codec is wrong, because that allows capture to be brought up when in fact there is no capture wired up on the board (and means that we'll end up trying to use unconnected inputs.)
What I'd expect to happen is that the wiring for the SoC internal bits can be there since it physically is there but we don't generate anything that userspace can see unless we also provide a back end it can be wired up to. We probably need to fill that bit in though since I don't think anyone ever tried to use this stuff on a unidirectional system before.
but doesn't stop those warnings (not really surprising, because it isn't the card level which is being complained about). Probably the easiest way to stop that appearing is to just disable OSS compatibility.
That's something which should be documented until it is fixed - that DPCM is currently incompatible with OSS.
That's not likely to ever be supported even if anyone cared, very few ASoC drivers will work with OSS due to the very poor mapping between OSS parameter configuration and ALSA parameter configuration.
So, there are two issues which still need resolving: 1. The registration of Codec widgets from the platform introduced by Liam in be09ad90e17b79fdb0d513a31e814ff4d42e3dff ASoC: core: Add platform DAI widget mapping
Well, that patch isn't visibly doing anything with CODECs - it's acting on the CPU and platform, not on the CODECs. I think what's happening there is that if you've got the same device for the DAI and platform then the widgets will be created twice, once by the DAI and once by the platform. The current code is fine if they're separate devices but will fail if they are the same device. We just need to make the DAPM context per device I think, possibly as part of moving over to components (based on the work that Morimoto-san started) though we could do it without that.
2. How to deal with disconnected front end streams which have no backend provided by their connected codec (in this case, front end can do capture and playback, back end can only do playback.)
Like I say I think we should just not be complaining if there's no capture back end available. Probably just checking if there's any at all is good enough for realistic uses.
Codecs without any outputs now remain powered down, which means any paths to these codecs also remain powered down. Add an always-enabled output pin widget to the spdif transceiver codec. This enables DAPM to correctly identify that the spdif transceiver is in use when playback is enabled, which will then allow DAPM to power up any links from the CPU DAI to the SPDIF transceiver. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- sound/soc/codecs/spdif_transciever.c | 15 ++++++++++++++- 1 files changed, 14 insertions(+), 1 deletions(-) diff --git a/sound/soc/codecs/spdif_transciever.c b/sound/soc/codecs/spdif_transciever.c index 112a49d..8df8d79 100644 --- a/sound/soc/codecs/spdif_transciever.c +++ b/sound/soc/codecs/spdif_transciever.c @@ -27,7 +27,20 @@ #define STUB_FORMATS SNDRV_PCM_FMTBIT_S16_LE -static struct snd_soc_codec_driver soc_codec_spdif_dit; +static const struct snd_soc_dapm_widget dit_widgets[] = { + SND_SOC_DAPM_OUTPUT("spdif-out"), +}; + +static const const struct snd_soc_dapm_route dit_routes[] = { + { "spdif-out", NULL, "Playback" }, +}; + +static struct snd_soc_codec_driver soc_codec_spdif_dit = { + .dapm_widgets = dit_widgets, + .num_dapm_widgets = ARRAY_SIZE(dit_widgets), + .dapm_routes = dit_routes, + .num_dapm_routes = ARRAY_SIZE(dit_routes), +}; static struct snd_soc_dai_driver dit_stub_dai = { .name = "dit-hifi", -- 1.7.4.4
On Sun, Aug 04, 2013 at 08:32:04PM +0100, Russell King wrote:
Codecs without any outputs now remain powered down, which means any paths to these codecs also remain powered down.
Applied, thanks.
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 3d039c3..236b0b2 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) +{ + /* This works because the platform and cpu dai are the same dev */ + struct kirkwood_dma_data *priv = snd_soc_platform_get_drvdata(w->platform); + + if (SND_SOC_DAPM_EVENT_ON(event)) + priv->ctl_play_mask |= KIRKWOOD_PLAYCTL_SPDIF_EN; + else + priv->ctl_play_mask &= ~KIRKWOOD_PLAYCTL_SPDIF_EN; + + return 0; +} + static const struct snd_soc_dapm_widget widgets[] = { /* These widget names come from the names from the functional spec */ SND_SOC_DAPM_AIF_OUT_E("i2sdo", "dma-tx", 0, SND_SOC_NOPM, 0, 0, kirkwood_i2s_play_i2s, SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_AIF_OUT_E("spdifdo", "dma-tx", + 0, SND_SOC_NOPM, 0, 0, kirkwood_i2s_play_spdif, + SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_AIF_IN("i2sdi", "dma-rx", 0, SND_SOC_NOPM, 0, 0), }; -- 1.7.4.4
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 236b0b2..c6599eb 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->platform->dapm, diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h index 3b70876..17b48a6 100644 --- a/sound/soc/kirkwood/kirkwood.h +++ b/sound/soc/kirkwood/kirkwood.h @@ -105,6 +105,39 @@ #define KIRKWOOD_PLAY_BYTE_INT_COUNT 0x1314 #define KIRKWOOD_BYTE_INT_COUNT_MASK 0xffffff +#define KIRKWOOD_SPDIF_PLAYCTL 0x2204 +#define KIRKWOOD_SPDIF_NON_PCM (1<<17) +#define KIRKWOOD_SPDIF_REG_VALIDITY (1<<16) +#define KIRKWOOD_SPDIF_FORCE_PARERR (1<<4) +#define KIRKWOOD_SPDIF_MEM_USER_EN (1<<2) +#define KIRKWOOD_SPDIF_MEM_VALIDITY_EN (1<<1) +#define KIRKWOOD_SPDIF_BLOCK_START_MODE (1<<0) + +#define KIRKWOOD_SPDIF_STATUS0_L 0x2280 +#define KIRKWOOD_SPDIF_STATUS1_L 0x2284 +#define KIRKWOOD_SPDIF_STATUS2_L 0x2288 +#define KIRKWOOD_SPDIF_STATUS3_L 0x228c +#define KIRKWOOD_SPDIF_STATUS4_L 0x2290 +#define KIRKWOOD_SPDIF_STATUS5_L 0x2294 +#define KIRKWOOD_SPDIF_STATUS0_R 0x22a0 +#define KIRKWOOD_SPDIF_STATUS1_R 0x22a4 +#define KIRKWOOD_SPDIF_STATUS2_R 0x22a8 +#define KIRKWOOD_SPDIF_STATUS3_R 0x22ac +#define KIRKWOOD_SPDIF_STATUS4_R 0x22b0 +#define KIRKWOOD_SPDIF_STATUS5_R 0x22b4 +#define KIRKWOOD_SPDIF_USER0_L 0x22c0 +#define KIRKWOOD_SPDIF_USER1_L 0x22c4 +#define KIRKWOOD_SPDIF_USER2_L 0x22c8 +#define KIRKWOOD_SPDIF_USER3_L 0x22cc +#define KIRKWOOD_SPDIF_USER4_L 0x22d0 +#define KIRKWOOD_SPDIF_USER5_L 0x22d4 +#define KIRKWOOD_SPDIF_USER0_R 0x22e0 +#define KIRKWOOD_SPDIF_USER1_R 0x22e4 +#define KIRKWOOD_SPDIF_USER2_R 0x22e8 +#define KIRKWOOD_SPDIF_USER3_R 0x22ec +#define KIRKWOOD_SPDIF_USER4_R 0x22f0 +#define KIRKWOOD_SPDIF_USER5_R 0x22f4 + #define KIRKWOOD_I2S_PLAYCTL 0x2508 #define KIRKWOOD_I2S_RECCTL 0x2408 #define KIRKWOOD_I2S_CTL_JUST_MASK (0xf<<26) -- 1.7.4.4
On 08/04/2013 09:21 PM, Russell King - ARM Linux wrote:
This is only a RFC at present due to the need to test on Kirkwood and fix at least one bug in ASoC core code.
I can't say that this stuff is fully tested, because I don't have any Kirkwood platforms, but I do have one Dove platform, which as I've said in the past doesn't run mainline. I don't run DT on it, so these patches also don't contain anything DT related.
Added Thomas Petazzoni to Cc, IIRC he has access to a T5325. I'll add DT support as I already did for Jean-Francois' patches. But as currently DT maintainers are very busy, I suggest to have a separate patch set for those and take these in earlier.
However, this patch set is aimed at sorting out the SPDIF on Dove and Kirkwood, which the Dove-based Cubox uses.
[...]
What I haven't included is the board file (like the openrd and t5325 ones already present) for SPDIF support on Dove, as that's based on the pre-DT setups, and I'm not sure anyone in mainline is interested in that. So the above provides the components necessary, hopefully someone with ASoC & DT knowledge can bind these components together via appropriate DT bindings. For reference, the DAI link looks like this:
.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",
Not a big deal for DT. I suggest to have a "audio-codecs" property that link CPU DAI to codec(s). One thing, I have noticed is that currently you need to supply both codec_name (or DT node) _and_ codec_dai_name. For that it would be helpful, if ASoC supplies helpers to get the DAI by index and use phandle with args here. As an example, the nodes could look like: spdif: spdif-transmitter { compatible = "linux,spdif-dit"; }; i2s1: audio-controller@b0000 { compatible = "marvell,dove-i2s"; reg = <0xb0000 0x2345>; audio-codecs = <&spdif 0>; }; ASoC then should resolve DT node and index to codec_dai and codec_dai_name. (Or have child nodes for each dai? Will also look through other drivers first). Also, the compatibles for mvebu-audio driver should be marvell,dove-i2s, marvell,kirkwood-i2s, and marvell,armada-370-i2s (if supported). With those, we can distinguish the controller features easily.
and the required route:
static const struct snd_soc_dapm_route routes[] = { { "Playback", NULL, "spdifdo" }, };
I am not sure about how to represent DAPM in DT but I guess there are already drivers that make use of it. Sebastian
Sebastian, Russell, On Sun, 04 Aug 2013 23:45:30 +0200, Sebastian Hesselbarth wrote:
On 08/04/2013 09:21 PM, Russell King - ARM Linux wrote:
This is only a RFC at present due to the need to test on Kirkwood and fix at least one bug in ASoC core code.
I can't say that this stuff is fully tested, because I don't have any Kirkwood platforms, but I do have one Dove platform, which as I've said in the past doesn't run mainline. I don't run DT on it, so these patches also don't contain anything DT related.
Added Thomas Petazzoni to Cc, IIRC he has access to a T5325.
Yes, I have access to a Kirkwood t5325, which I'm currently trying to move to DT. Note that I also received Armada 370 audio hardware last week, so I'll be working on this as well in the future, this hardware has both in/out jack plugs as well as SPDIF ones. So, yes, please Cc me on Kirkwood audio related developments. Thanks! Thomas Petazzoni -- Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
On Mon, Aug 05, 2013 at 10:43:35AM +0200, Thomas Petazzoni wrote:
Sebastian, Russell,
On Sun, 04 Aug 2013 23:45:30 +0200, Sebastian Hesselbarth wrote:
On 08/04/2013 09:21 PM, Russell King - ARM Linux wrote:
This is only a RFC at present due to the need to test on Kirkwood and fix at least one bug in ASoC core code.
I can't say that this stuff is fully tested, because I don't have any Kirkwood platforms, but I do have one Dove platform, which as I've said in the past doesn't run mainline. I don't run DT on it, so these patches also don't contain anything DT related.
Added Thomas Petazzoni to Cc, IIRC he has access to a T5325.
Yes, I have access to a Kirkwood t5325, which I'm currently trying to move to DT. Note that I also received Armada 370 audio hardware last week, so I'll be working on this as well in the future, this hardware has both in/out jack plugs as well as SPDIF ones.
So, yes, please Cc me on Kirkwood audio related developments.
Can you pick the patches off the mailing list(s)? Re-sending them is just going to be noise for everyone.
Dear Russell King - ARM Linux, On Mon, 5 Aug 2013 09:53:34 +0100, Russell King - ARM Linux wrote:
Yes, I have access to a Kirkwood t5325, which I'm currently trying to move to DT. Note that I also received Armada 370 audio hardware last week, so I'll be working on this as well in the future, this hardware has both in/out jack plugs as well as SPDIF ones.
So, yes, please Cc me on Kirkwood audio related developments.
Can you pick the patches off the mailing list(s)? Re-sending them is just going to be noise for everyone.
Yes, sure. I was just talking about future postings of this patch set. For the current version, no need to resend just to Cc me. Thanks! Thomas -- Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
On Sun, Aug 04, 2013 at 11:45:30PM +0200, Sebastian Hesselbarth wrote:
On 08/04/2013 09:21 PM, Russell King - ARM Linux wrote:
.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",
Not a big deal for DT. I suggest to have a "audio-codecs" property that link CPU DAI to codec(s). One thing, I have noticed is that currently you need to supply both codec_name (or DT node) _and_ codec_dai_name. For that it would be helpful, if ASoC supplies helpers to get the DAI by index and use phandle with args here.
This should follow the same pattern as the other board bindings.
i2s1: audio-controller@b0000 { compatible = "marvell,dove-i2s"; reg = <0xb0000 0x2345>; audio-codecs = <&spdif 0>; };
No, things are glued together using the machine driver - again, look at how all the other systems work. The wiring on the board can get interesting enough to need a binding of its own, for very simple bindings that should just be pointing to a generic driver.
On 08/05/13 13:59, Mark Brown wrote:
On Sun, Aug 04, 2013 at 11:45:30PM +0200, Sebastian Hesselbarth wrote:
On 08/04/2013 09:21 PM, Russell King - ARM Linux wrote:
.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",
Not a big deal for DT. I suggest to have a "audio-codecs" property that link CPU DAI to codec(s). One thing, I have noticed is that currently you need to supply both codec_name (or DT node) _and_ codec_dai_name. For that it would be helpful, if ASoC supplies helpers to get the DAI by index and use phandle with args here.
This should follow the same pattern as the other board bindings.
Mark, looking at e.g. nvidia,tegra-audio-*.txt bindings, I guess you are proposing to have a new binding for every SoC/codec/board combination? Also, you should have seen "ASoC: fsl: Add S/PDIF machine driver", which is - again - proposing a new binding for fsl with spdif "codec". You really want me to go the same way or is there any "other board binding" I should follow?
i2s1: audio-controller@b0000 { compatible = "marvell,dove-i2s"; reg = <0xb0000 0x2345>; audio-codecs = <&spdif 0>; };
No, things are glued together using the machine driver - again, look at how all the other systems work. The wiring on the board can get interesting enough to need a binding of its own, for very simple bindings that should just be pointing to a generic driver.
And that is what will be true for almost all codecs we hook up on Marvell SoCs. The audio controller is very simple with only mute control available. We need to connect either i2s or spdif or both to any codec and the codecs output. The codecs is determined by phandle and the output (codec_dai_name) could be determined by the phandle's arg, e.g. 0 for the first codec_dai provided. That will be one machine driver for possibly all Marvell boards, not what e.g. tegra is doing with one machine driver for each board they can find. I understand that the exact wiring on each board may get complicated, but I don't see why a (well thought) binding should not be able to describe it? Sebastian
On Mon, Aug 05, 2013 at 03:06:16PM +0200, Sebastian Hesselbarth wrote:
looking at e.g. nvidia,tegra-audio-*.txt bindings, I guess you are proposing to have a new binding for every SoC/codec/board combination? Also, you should have seen "ASoC: fsl: Add S/PDIF machine driver", which is - again - proposing a new binding for fsl with spdif "codec".
You really want me to go the same way or is there any "other board binding" I should follow?
Yes, go that way. There is the simple-card driver which should be able to grow to consume most of these things for simple stereo CODECs, having the common clock API more widely available so we can create a hard requirement that clocks be described using the clock API would help a lot with that.
That will be one machine driver for possibly all Marvell boards, not what e.g. tegra is doing with one machine driver for each board they can find. I understand that the exact wiring on each board may get complicated, but I don't see why a (well thought) binding should not be able to describe it?
The clocking arrangements for a smartphone are not that trivial and we need to be able to cope with things like having more than two devices on the bus.
On 08/05/13 16:07, Mark Brown wrote:
On Mon, Aug 05, 2013 at 03:06:16PM +0200, Sebastian Hesselbarth wrote:
looking at e.g. nvidia,tegra-audio-*.txt bindings, I guess you are proposing to have a new binding for every SoC/codec/board combination? Also, you should have seen "ASoC: fsl: Add S/PDIF machine driver", which is - again - proposing a new binding for fsl with spdif "codec".
You really want me to go the same way or is there any "other board binding" I should follow?
Yes, go that way. There is the simple-card driver which should be able to grow to consume most of these things for simple stereo CODECs, having the common clock API more widely available so we can create a hard requirement that clocks be described using the clock API would help a lot with that.
That will be one machine driver for possibly all Marvell boards, not what e.g. tegra is doing with one machine driver for each board they can find. I understand that the exact wiring on each board may get complicated, but I don't see why a (well thought) binding should not be able to describe it?
The clocking arrangements for a smartphone are not that trivial and we need to be able to cope with things like having more than two devices on the bus.
Mark, I cannot follow you on the clocking arrangements. The binding proposal was for audio controller <-> CODEC connections. For clocks there are common properties ("clocks", "clock-names", "clock-frequency") to pass them to drivers - for "sound cards" in general there are not. I am not referring to ASoC or ALSA here, but describing device inter-connection. And that can be done in a generic way to allow all those SoC/codec/board specific DT bindings to become very obsolete. The device nodes in question can still contain clocks properties of course, but you do not need a special compatible for e.g. "nvidia,tegra-audio-rt5640-beaver". So, having a look at the node in question: sound { compatible = "nvidia,tegra-audio-rt5640-beaver", "nvidia,tegra-audio-rt5640"; nvidia,model = "NVIDIA Tegra Beaver"; nvidia,audio-routing = "Headphones", "HPOR", "Headphones", "HPOL"; nvidia,i2s-controller = <&tegra_i2s1>; nvidia,audio-codec = <&rt5640>; nvidia,hp-det-gpios = <&gpio TEGRA_GPIO(W, 2) GPIO_ACTIVE_HIGH>; clocks = <&tegra_car TEGRA30_CLK_PLL_A>, <&tegra_car TEGRA30_CLK_PLL_A_OUT0>, <&tegra_car TEGRA30_CLK_EXTERN1>; clock-names = "pll_a", "pll_a_out0", "mclk"; }; all those "nvidia," prefixed properties are not nvidia-specific at all. Also, all those "nvidia," properties would have fit very well into the i2s controller node - there may be more complex SoCs requiring an extra "sound card" node, Tegra is not. Even those foo-gpios properties can be generalized and moved to ASoC; have a look at MMC drivers using one common "cd-gpios" property for very different drivers. What is so special with Tegra and this gpio that it needs a vendor-specific property? I was asking for generalizing those exact properties to generic ones and them move support for them to the ASoC/ALSA framework. With all that DT binding re-review coming up, I doubt that the tegra binding will be accepted again anyway.
i2s1: audio-controller@b0000 { compatible = "marvell,dove-i2s"; reg = <0xb0000 0x2345>; audio-codecs = <&spdif 0>; }; No, things are glued together using the machine driver - again, look at how all the other systems work. The wiring on the board can get interesting enough to need a binding of its own, for very simple bindings that should just be pointing to a generic driver.
I learned to never match "device nodes" with "drivers" as there is simply no relation between them. So, back to my original node proposal, can you tell me what is so different, except that I am not using "marvell,audio-codec(s)" but "audio-codecs"; and hook the one available codec output by using phandle/args instead of a new compatible string? Sebastian (Note: The above i2s1 node will contains "clocks" for sure, but that is not related to sound-specific properties.)
On Mon, Aug 05, 2013 at 05:04:35PM +0200, Sebastian Hesselbarth wrote:
I cannot follow you on the clocking arrangements. The binding proposal was for audio controller <-> CODEC connections. For clocks there are common properties ("clocks", "clock-names", "clock-frequency") to pass them to drivers - for "sound cards" in general there are not.
The clocking arrangements are an example of why the boards can get interesting enough to describe for themselves.
So, having a look at the node in question:
sound { compatible = "nvidia,tegra-audio-rt5640-beaver", "nvidia,tegra-audio-rt5640"; nvidia,model = "NVIDIA Tegra Beaver";
nvidia,audio-routing = "Headphones", "HPOR", "Headphones", "HPOL";
nvidia,i2s-controller = <&tegra_i2s1>; nvidia,audio-codec = <&rt5640>;
nvidia,hp-det-gpios = <&gpio TEGRA_GPIO(W, 2) GPIO_ACTIVE_HIGH>;
clocks = <&tegra_car TEGRA30_CLK_PLL_A>, <&tegra_car TEGRA30_CLK_PLL_A_OUT0>, <&tegra_car TEGRA30_CLK_EXTERN1>; clock-names = "pll_a", "pll_a_out0", "mclk"; };
all those "nvidia," prefixed properties are not nvidia-specific at all.
This is all because you have to add a prefix for DT.
Also, all those "nvidia," properties would have fit very well into the i2s controller node
No, almost none of them have any place there. For example, the audio routing contains only connections to the CODEC so the I2S controller isn't in play, the headphone detection is connected to the AP but isn't connected to the I2S port.
- there may be more complex SoCs requiring an extra "sound card" node, Tegra is not.
This is nothing to do with the SoC and all to do with the rest of the board - Tegra systems will frequently have these issues since Tegra is frequently deployed in smartphones which tend to push the limits on these things. The smartphone use case will typically have a separate digital clock domain for the baseband, there will usually also be a bluetooth SCO link slaved to one of the other digital domains. Start drilling down into the details and you'll usually find a bit more complexity and usually a bunch of device specific constraints too.
Even those foo-gpios properties can be generalized and moved to ASoC; have a look at MMC drivers using one common "cd-gpios" property for very different drivers. What is so special with Tegra and this gpio that it needs a vendor-specific property?
This is mostly just a function of the DT convention requiring a vendor prefix be shoved on everything.
I was asking for generalizing those exact properties to generic ones and them move support for them to the ASoC/ALSA framework. With all that DT binding re-review coming up, I doubt that the tegra binding will be accepted again anyway.
Sure it would.
i2s1: audio-controller@b0000 { compatible = "marvell,dove-i2s"; reg = <0xb0000 0x2345>; audio-codecs = <&spdif 0>; };
No, things are glued together using the machine driver - again, look at how all the other systems work. The wiring on the board can get interesting enough to need a binding of its own, for very simple bindings that should just be pointing to a generic driver.
I learned to never match "device nodes" with "drivers" as there is simply no relation between them.
That's clearly a thinko for device node.
So, back to my original node proposal, can you tell me what is so different, except that I am not using "marvell,audio-codec(s)" but "audio-codecs"; and hook the one available codec output by using phandle/args instead of a new compatible string?
From my point of view I'd rather not be shoving vendor prefixes on all the properties, this is coming from DT convention requiring vendor prefixes on bindings.
On 08/05/2013 06:59 PM, Mark Brown wrote:
On Mon, Aug 05, 2013 at 05:04:35PM +0200, Sebastian Hesselbarth wrote:
I cannot follow you on the clocking arrangements. The binding proposal was for audio controller <-> CODEC connections. For clocks there are common properties ("clocks", "clock-names", "clock-frequency") to pass them to drivers - for "sound cards" in general there are not.
The clocking arrangements are an example of why the boards can get interesting enough to describe for themselves.
I understand there are complex arrangements, I still don't think that you need a global super-node to describe them. Anyway, I am not voting against a super-node.
So, having a look at the node in question:
sound { compatible = "nvidia,tegra-audio-rt5640-beaver", "nvidia,tegra-audio-rt5640"; nvidia,model = "NVIDIA Tegra Beaver";
nvidia,audio-routing = "Headphones", "HPOR", "Headphones", "HPOL";
nvidia,i2s-controller = <&tegra_i2s1>; nvidia,audio-codec = <&rt5640>;
nvidia,hp-det-gpios = <&gpio TEGRA_GPIO(W, 2) GPIO_ACTIVE_HIGH>;
clocks = <&tegra_car TEGRA30_CLK_PLL_A>, <&tegra_car TEGRA30_CLK_PLL_A_OUT0>, <&tegra_car TEGRA30_CLK_EXTERN1>; clock-names = "pll_a", "pll_a_out0", "mclk"; };
all those "nvidia," prefixed properties are not nvidia-specific at all.
This is all because you have to add a prefix for DT.
Right, like we have on all the other non-standard properties like "pinctrl-0", "bla-gpios", "clocks", ... come on, just make them generic enough and you can omit the vendor prefix.
Also, all those "nvidia," properties would have fit very well into the i2s controller node
No, almost none of them have any place there. For example, the audio routing contains only connections to the CODEC so the I2S controller isn't in play, the headphone detection is connected to the AP but isn't connected to the I2S port.
From a quick look in tegra30_hub.h, I can see configuration registers i2s formatting. I assume that i2s controller on Tegra can therefore directly connected to a I2S codec, can't it? Then, with an existing i2s node and an existing codec node - why is there no place to link both? I can think of use cases that are hard to describe in a link-to-link way, but none of them really requires a super-node nor special board-specific compatible strings. With that we can just have the root DT node be compatible with Beaver above and register all the old platform_device way. [...]
I learned to never match "device nodes" with "drivers" as there is simply no relation between them.
That's clearly a thinko for device node.
I assume with "thinko" you mean "thought wrong" - IMHO the above statement is very true. If it wouldn't, why not just have a node for kirkwood-dma and kirkwood-i2s instead of merging the drivers? You think that will also be accepted by DT maintainers?
So, back to my original node proposal, can you tell me what is so different, except that I am not using "marvell,audio-codec(s)" but "audio-codecs"; and hook the one available codec output by using phandle/args instead of a new compatible string?
From my point of view I'd rather not be shoving vendor prefixes on all the properties, this is coming from DT convention requiring vendor prefixes on bindings.
I understand that those vendor prefixes are part of the helper functions of ASoC. But no other subsystem has a similar approach but though of properties generic enough to help drivers find what they need to know. Either ASoC is mis-interpreting the vendor-prefix requirement - or all other subsystems are. Sebastian
On Mon, Aug 05, 2013 at 08:14:47PM +0200, Sebastian Hesselbarth wrote:
On 08/05/2013 06:59 PM, Mark Brown wrote:
The clocking arrangements are an example of why the boards can get interesting enough to describe for themselves.
I understand there are complex arrangements, I still don't think that you need a global super-node to describe them. Anyway, I am not voting against a super-node.
Please look back through the list archives, we've been round this loop repeatedly.
Also, all those "nvidia," properties would have fit very well into the i2s controller node
No, almost none of them have any place there. For example, the audio routing contains only connections to the CODEC so the I2S controller isn't in play, the headphone detection is connected to the AP but isn't connected to the I2S port.
From a quick look in tegra30_hub.h, I can see configuration registers i2s formatting. I assume that i2s controller on Tegra can therefore directly connected to a I2S codec, can't it? Then, with an existing i2s node and an existing codec node - why is there no place to link both?
At a minimum we'd need to figure out which end of the link to put it on and what to do about multi-drop links or links which share some or all of the clocks but have split data lines.
I can think of use cases that are hard to describe in a link-to-link way, but none of them really requires a super-node nor special board-specific compatible strings. With that we can just have the root DT node be compatible with Beaver above and register all the old platform_device way.
It's just plain good practice to have some way to figure out which board we're running on.
[...]
I learned to never match "device nodes" with "drivers" as there is simply no relation between them.
That's clearly a thinko for device node.
I assume with "thinko" you mean "thought wrong" - IMHO the above statement is very true. If it wouldn't, why not just have a node for kirkwood-dma and kirkwood-i2s instead of merging the drivers? You think that will also be accepted by DT maintainers?
We don't have a node for the DMA driver because it's just a software wrapper for the DMA controller. We do have a node for the board because it's an actual physical thing that we can point at, the board driver is representing the audio subsystem schematic.
From my point of view I'd rather not be shoving vendor prefixes on all the properties, this is coming from DT convention requiring vendor prefixes on bindings.
I understand that those vendor prefixes are part of the helper functions of ASoC. But no other subsystem has a similar approach but
No? I can't parse what you're saying here at all.
though of properties generic enough to help drivers find what they need to know. Either ASoC is mis-interpreting the vendor-prefix requirement - or all other subsystems are.
This isn't me, this is people like Stephen (who's one of the DT guys...).
On 08/05/2013 09:04 AM, Sebastian Hesselbarth wrote: ...
So, having a look at the node in question:
sound { compatible = "nvidia,tegra-audio-rt5640-beaver", "nvidia,tegra-audio-rt5640"; nvidia,model = "NVIDIA Tegra Beaver";
nvidia,audio-routing = "Headphones", "HPOR", "Headphones", "HPOL";
nvidia,i2s-controller = <&tegra_i2s1>; nvidia,audio-codec = <&rt5640>;
nvidia,hp-det-gpios = <&gpio TEGRA_GPIO(W, 2) GPIO_ACTIVE_HIGH>;
clocks = <&tegra_car TEGRA30_CLK_PLL_A>, <&tegra_car TEGRA30_CLK_PLL_A_OUT0>, <&tegra_car TEGRA30_CLK_EXTERN1>; clock-names = "pll_a", "pll_a_out0", "mclk"; };
all those "nvidia," prefixed properties are not nvidia-specific at all.
Indeed. I do still have a desire to do a more encompassing generic binding that could use generic property names. However, I'm pulled in many directions and never manage to get around to that:-( Months or even years ago I did get as far as posting a binding with my ideas, but it needs a lot more work to correctly separate out what can be generic and what still needs to be SoC-/board-specific, how to represent it all in DT, and how to code stuff up into useful utility code that ASoC machine drivers will find useful, since custom machine drivers will almost certainly be required to do the last pieces of integration of SoC-specific control, even with a relatively complete generic DT representation (unless you start putting Forth into DT for the last bits!)
Also, all those "nvidia," properties would have fit very well into the i2s controller node - there may be more complex SoCs requiring an extra "sound card" node, Tegra is not.
That's certainly not true. It only looks like that because the DTs we currently have upstream expose only a subset of the whole functionality of the system. It's certainly not possible to move all the properties into e.g. the I2S controller node in general, because the audio system as a whole spans far more devices than just a single I2S controller.
Even those foo-gpios properties can be generalized and moved to ASoC; have a look at MMC drivers using one common "cd-gpios" property for very different drivers. What is so special with Tegra and this gpio that it needs a vendor-specific property?
I was asking for generalizing those exact properties to generic ones and them move support for them to the ASoC/ALSA framework. With all that DT binding re-review coming up, I doubt that the tegra binding will be accepted again anyway.
I don't agree. It's not *generic*, but I don't see any requirement to only accept completely generic bindings. If there was such a requirement, we'd probably never get any more bindings for embedded-style audio HW... The only thing that might change is removing some of the prefixes from the property names.
On 08/05/2013 03:06 PM, Sebastian Hesselbarth wrote:
On 08/05/13 13:59, Mark Brown wrote:
On Sun, Aug 04, 2013 at 11:45:30PM +0200, Sebastian Hesselbarth wrote:
On 08/04/2013 09:21 PM, Russell King - ARM Linux wrote:
.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",
Not a big deal for DT. I suggest to have a "audio-codecs" property that link CPU DAI to codec(s). One thing, I have noticed is that currently you need to supply both codec_name (or DT node) _and_ codec_dai_name. For that it would be helpful, if ASoC supplies helpers to get the DAI by index and use phandle with args here.
This should follow the same pattern as the other board bindings.
Mark,
looking at e.g. nvidia,tegra-audio-*.txt bindings, I guess you are proposing to have a new binding for every SoC/codec/board combination? Also, you should have seen "ASoC: fsl: Add S/PDIF machine driver", which is - again - proposing a new binding for fsl with spdif "codec".
You really want me to go the same way or is there any "other board binding" I should follow?
In my opinion having different bindings for each board is not a good idea. Having a common set of property names will allow us to factor out the parsing to the ASoC core instead of doing this by hand for each board driver.
i2s1: audio-controller@b0000 { compatible = "marvell,dove-i2s"; reg = <0xb0000 0x2345>; audio-codecs = <&spdif 0>; };
No, things are glued together using the machine driver - again, look at how all the other systems work. The wiring on the board can get interesting enough to need a binding of its own, for very simple bindings that should just be pointing to a generic driver.
And that is what will be true for almost all codecs we hook up on Marvell SoCs. The audio controller is very simple with only mute control available. We need to connect either i2s or spdif or both to any codec and the codecs output. The codecs is determined by phandle and the output (codec_dai_name) could be determined by the phandle's arg, e.g. 0 for the first codec_dai provided.
Kuninori Morimoto started working on a xlate callback for CODECs and DAIs some time ago[1]. If nobody else is going to pick this up I'm probably going to continue his work on this. - Lars [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2013-February/059788.ht...
On Mon, Aug 05, 2013 at 04:10:47PM +0200, Lars-Peter Clausen wrote:
In my opinion having different bindings for each board is not a good idea. Having a common set of property names will allow us to factor out the parsing to the ASoC core instead of doing this by hand for each board driver.
Yeah, that's the idea - right now there's blanks that need filling in by the machine drivers so we do have them but once those get dealt with the compatible strings can just be moved over into a generic binding.
Hi Lars
Kuninori Morimoto started working on a xlate callback for CODECs and DAIs some time ago[1]. If nobody else is going to pick this up I'm probably going to continue his work on this.
Thank you for your offer about above. I'm sorry for my late work for xlate callback. but, unfortunately, I busy now, so, I can't work for it now. I'm very happy if someone/you can work for it. Best regards --- Kuninori Morimoto
Hi
Kuninori Morimoto started working on a xlate callback for CODECs and DAIs some time ago[1]. If nobody else is going to pick this up I'm probably going to continue his work on this.
Thank you for your offer about above. I'm sorry for my late work for xlate callback. but, unfortunately, I busy now, so, I can't work for it now. I'm very happy if someone/you can work for it.
My understanding for this ASoC DT support milestone is that 1) merge codec/component 2) add .of_xlate_dai_name on it 3) add DT support But, do you think 1) is possible ? Becouse, now, too many functions are based on/using "codec" struct snd_soc_codec / struct snd_soc_codec_driver. In reality, merging these is super difficult IMO. But adding .of_xlate_dai_name on each codec/component is not difficult, and, I can do it immediately. what is your opinion ? Best regards --- Kuninori Morimoto
On 08/30/2013 09:20 AM, Kuninori Morimoto wrote:
Hi
Kuninori Morimoto started working on a xlate callback for CODECs and DAIs some time ago[1]. If nobody else is going to pick this up I'm probably going to continue his work on this.
Thank you for your offer about above. I'm sorry for my late work for xlate callback. but, unfortunately, I busy now, so, I can't work for it now. I'm very happy if someone/you can work for it.
My understanding for this ASoC DT support milestone is that
1) merge codec/component 2) add .of_xlate_dai_name on it 3) add DT support
But, do you think 1) is possible ? Becouse, now, too many functions are based on/using "codec" struct snd_soc_codec / struct snd_soc_codec_driver. In reality, merging these is super difficult IMO.
But adding .of_xlate_dai_name on each codec/component is not difficult, and, I can do it immediately.
what is your opinion ?
The idea is to use the component struct as a base class for the codec and platform struct. So drivers will keep seeing what they see now and don't need to be updated. Lots of the fields that can be shared between component, platform and codec are only used by the core. - Lars
On Fri, Aug 30, 2013 at 10:26:07AM +0200, Lars-Peter Clausen wrote:
On 08/30/2013 09:20 AM, Kuninori Morimoto wrote:
My understanding for this ASoC DT support milestone is that
1) merge codec/component 2) add .of_xlate_dai_name on it 3) add DT support
But, do you think 1) is possible ? Becouse, now, too many functions are based on/using "codec" struct snd_soc_codec / struct snd_soc_codec_driver. In reality, merging these is super difficult IMO.
The idea is to use the component struct as a base class for the codec and platform struct. So drivers will keep seeing what they see now and don't need to be updated. Lots of the fields that can be shared between component, platform and codec are only used by the core.
Indeed, and doing things that can be done with a direct search and replace is not so bad. Splitting things up makes life easier too. I'd been planning to start looking at the merge of CODEC and component myself during the next release cycle.
On Mon, Aug 05, 2013 at 03:06:16PM +0200, Sebastian Hesselbarth wrote:
And that is what will be true for almost all codecs we hook up on Marvell SoCs. The audio controller is very simple with only mute control available. We need to connect either i2s or spdif or both to any codec and the codecs output. The codecs is determined by phandle and the output (codec_dai_name) could be determined by the phandle's arg, e.g. 0 for the first codec_dai provided.
Sebastian, I suggest just not fucking bothering with this. Mark has no intention to listen to what we need to make SPDIF and I2S work on this hardware, so persuing getting this stuff into mainline is just a total and utter waste of time. Maybe when Mark stops being an arsehole and starts trying to understand what people need from ASoC, and he starts documenting this stuff, it might be possible to resurect this, but quite frankly with his attitude towards people trying to use his code, I think we're probably far better off dumping ASoC and going in at the ALSA level instead. I've had enough of ASoC crap.
participants (11)
-
Jean-Francois Moine -
Kuninori Morimoto -
Lars-Peter Clausen -
Liam Girdwood -
Liam Girdwood -
Mark Brown -
Russell King -
Russell King - ARM Linux -
Sebastian Hesselbarth -
Stephen Warren -
Thomas Petazzoni