| Message ID | E1V641A-0004lv-DM@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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)
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(-)