[alsa-devel] Confusing about Playback/Capture, CODEC/CODEC links, and snd_soc_dapm_link_dai_widgets()
Mark, Liam,
recall the Tegra AHUB structure:
+-------------+ +-----+ +--------------+ +-----+ +------+ | FIFO pair 0 |<->| CIF |<->| Cross-bar |<->| CIF |<->| I2S0 |<-> External IO +-------------+ +-----+ | (the "AHUB") | +-----+ +------+ . . . | | . . . +-------------+ +-----+ | | +-----+ +------+ | FIFO pair 3 |<->| CIF |<->| |<->| CIF |<->| I2S4 |<-> External IO +-------------+ +-----+ | | +-----+ +------+ | | | | +-----+ +-------+ | |<->| CIF |<->| SPDIF |<-> External IO | | +-----+ +-------+ | | | | +-----+ +-------+ | |<->| CIF |<->| DAM | | | +-----+ +-------+ +--------------+
(CIF is AHUB XBAR Client InterFace; basically the DAI or DAI link)
I have created a driver for the 4 FIFOs on the left, which exposes 4 (CPU) DAIs.
I have created a driver for the AHUB XBAR, which is an ASoC CODEC, and has a DAI for each of the CIFs.
There's a CODEC/CODEC link between each FIFO DAI and the relevant AHUB CIF DAI.
In order to connect the I2S devices to the AHUB XBAR, I had to make that a CODEC too. This exposes two DAIs; the CIF DAI to connect to the AHUB, and the DAP (Digital Audio Port) DAI to represent the I/O pins on the Tegra package that are connected to the external codec.
I have an issue with the paths that snd_soc_dapm_link_dai_widgets() sets up inside both the AHUB XBAR and the I2S. I'll use the I2S to explain since it's internally much simpler.
The two DAIs in I2S are roughly:
static const struct snd_soc_dai_driver tegra30_i2s_dais[] = { { .playback = { .stream_name = "DAP Playback", }, .capture = { .stream_name = "DAP Capture", }, }, { .playback = { .stream_name = "CIF Playback", }, .capture = { .stream_name = "CIF Capture", }, }, };
For the CIF-side DAI, I instantiated widgets for the AIFs:
static const struct snd_soc_dapm_widget tegra30_i2s_widgets[] = { SND_SOC_DAPM_AIF_IN("CIF RX", "CIF Playback", 0, SND_SOC_NOPM, 0, 0), SND_SOC_DAPM_AIF_OUT("CIF TX", "CIF Capture", 0, SND_SOC_NOPM, 0, 0),
This works fine; snd_soc_dapm_link_dai_widgets() create a route from the DAI widget "DAP Playback" that feeds into the AIF_IN widget "CIF RX".
"works fine" means that the ASoC debugfs files in "asoc/$card/tegra30-i2s.1/dapm/CIF {Playback,RX}" show the expected connections.
The DAPM routes inside the I2S are:
static const struct snd_soc_dapm_route tegra30_i2s_routes[] = { { "DAP TX", NULL, "CIF RX" }, { "CIF TX", NULL, "DAP RX" }, };
This also works fine.
Now, I instantiated two AIF widgets to interface with the DAP-side DAI:
SND_SOC_DAPM_AIF_IN("DAP RX", "DAP Capture", 0, SND_SOC_NOPM, 0, 0), SND_SOC_DAPM_AIF_OUT("DAP TX", "DAP Playback", 0, SND_SOC_NOPM, 0, 0),
However, this causes snd_soc_dapm_link_dai_widgets() to set up some unexpected paths; "DAP Playback" ends up feeding *into* "DAP TX", rather than feeding from/out of it...
# cat CIF\ Playback CIF Playback: Off in 0 out 0 stream CIF Playback inactive out "static" "CIF RX" out "static" "CIF RX" # cat CIF\ RX CIF RX: Off in 0 out 0 stream CIF Playback inactive in "static" "CIF Playback" in "static" "CIF Playback" out "static" "DAP TX" # cat DAP\ TX DAP TX: Off in 0 out 0 stream DAP Playback inactive in "static" "DAP Playback" <<<<<<<<<< in "static" "DAP Playback" <<<<<<<<<< in "static" "CIF RX" # cat DAP\ Playback DAP Playback: Off in 0 out 0 stream DAP Playback inactive out "static" "DAP TX" out "static" "DAP TX"
Now, I can fake this out by swapping the stream names on the DAP-side AIF widgets:
SND_SOC_DAPM_AIF_IN("DAP RX", "DAP Playback", 0, SND_SOC_NOPM, 0, 0), SND_SOC_DAPM_AIF_OUT("DAP TX", "DAP Capture", 0, SND_SOC_NOPM, 0, 0),
Which yields the expected paths in debugfs:
# cat CIF\ Playback CIF Playback: Off in 0 out 0 stream CIF Playback inactive out "static" "CIF RX" out "static" "CIF RX" # cat CIF\ RX CIF RX: Off in 0 out 0 stream CIF Playback inactive in "static" "CIF Playback" in "static" "CIF Playback" out "static" "DAP TX" # cat DAP\ TX DAP TX: Off in 0 out 0 stream DAP Capture inactive in "static" "CIF RX" out "static" "DAP Capture" out "static" "DAP Capture" # cat DAP\ Capture DAP Capture: Off in 0 out 0 stream DAP Capture inactive in "static" "DAP TX" in "static" "DAP TX"
But I'm not sure if that's quite correct. In the DAI definitions, is the .playback sub-structure always meant to represent:
1) Playback from the CPU's perspective, in which case the first set of DAP AIF definitions above would be correct
or:
2) Is it more that Playback==RX_into_codec, Capture==TX_from_codec? This appears to be supported by the fact that the paths get set up correctly with the second set of AIF definitions above.
But if (2) is correct, I wonder why soc_dapm_stream_event()'s first if statement appears to consider the playback_widget of both sides of the DAI to be coupled; wouldn't one side's playback widget be coupled to the other side's capture widget?
As an aside, I'm not sure if it's conceptually correct to talk about playback or capture any more (beyond the initial CPU DAI) with arbitrary CODEC/CODEC links, since who knows what kind of routing/CPU->CPU loopbacks/external CODEC->CODEC loopbacks/... might exist in the CODECs?
Either way though, something is still not working correctly even when the expected paths show up in debugfs. When I start playback on a PCM exposed by the DMA FIFO driver, I see the relevant AHUB XBAR's stream turned on, and a route exists to the relevant AIF_IN widget, but that widget is not considered to have any outward connections by is_connected_output_ep(), which I think is what is causing it and none of the rest of the path to turn on:
(from asoc/$card/asoc/tegra30-ahub-xbar/dapm)
# cat APBIF0\ Playback APBIF0 Playback: On in 1 out 1 stream APBIF0 Playback active out "static" "APBIF0 RX" out "static" "APBIF0 RX" # cat APBIF0\ RX APBIF0 RX: Off in 2 out 0 <<<<< last value shouldn't be 0? stream APBIF0 Playback inactive <<<<< still inactive? in "static" "APBIF0 Playback" in "static" "APBIF0 Playback" out "APBIF0 RX" "I2S0 TX Mux"
even though when I look at all the debugfs files, all the expected paths are there, at least within the AHUB XBAR and I2S drivers, and the external DAI links in the machine driver all probed as expected and bound all the components together.
Probably related, the I2S and WM8903 (DAI) drivers aren't being called by the ASoC core to initialize themselves for playback either; pretty much all that happens is that DMA is started. Is the machine driver responsible for this?
At this point, I'm not sure whether I have a gross mis-understanding of how this is supposed to work, or whether there are simply bits missing from the DAPM code to fully support CODEC/CODEC links, rather than CPU/CODEC links.
Probably slightly related to all of this, but is the following code correct:
int snd_soc_dapm_dai_get_connected_widgets(struct snd_soc_dai *dai, int stream, struct snd_soc_dapm_widget_list **list)
...
if (stream == SNDRV_PCM_STREAM_PLAYBACK) paths = is_connected_output_ep(dai->playback_widget, list); else paths = is_connected_input_ep(dai->playback_widget, list);
I would have expected this to use capture_widget on the final line, but I haven't thought about this in detail, just noticed the lack of symmetry by very brief inspection.
Thanks for any help. Sorry for the long email.
On Thu, May 31, 2012 at 04:49:26PM -0600, Stephen Warren wrote:
I have an issue with the paths that snd_soc_dapm_link_dai_widgets() sets up inside both the AHUB XBAR and the I2S. I'll use the I2S to explain since it's internally much simpler.
You shouldn't be using this at all for new code, you should be using explicit DAPM routes to link to the widgets and specifying NULL stream_name.
- Is it more that Playback==RX_into_codec, Capture==TX_from_codec? This
appears to be supported by the fact that the paths get set up correctly with the second set of AIF definitions above.
Yes.
But if (2) is correct, I wonder why soc_dapm_stream_event()'s first if statement appears to consider the playback_widget of both sides of the DAI to be coupled; wouldn't one side's playback widget be coupled to the other side's capture widget?
The DAIs on CPUs have the opposite sense to DAIs on CODECs. In a traditional CPU<->CODEC link we do connect the two playback widgets directly to each other. The current code isn't correct, it's not normally noticable since most of the time the CPU DAI end of the link is a baseband which doesn't do anything real and is always activated bidirectionally so it really makes no odds which widget we look at.
I'll fix this tomorrow.
As an aside, I'm not sure if it's conceptually correct to talk about playback or capture any more (beyond the initial CPU DAI) with arbitrary CODEC/CODEC links, since who knows what kind of routing/CPU->CPU loopbacks/external CODEC->CODEC loopbacks/... might exist in the CODECs?
We could rename everything to RX and TX but it's a pain and going to be annoying for backporting.
Either way though, something is still not working correctly even when the expected paths show up in debugfs. When I start playback on a PCM exposed by the DMA FIFO driver, I see the relevant AHUB XBAR's stream turned on, and a route exists to the relevant AIF_IN widget, but that
Works for me.
# cat APBIF0\ Playback APBIF0 Playback: On in 1 out 1 stream APBIF0 Playback active out "static" "APBIF0 RX" out "static" "APBIF0 RX" # cat APBIF0\ RX APBIF0 RX: Off in 2 out 0 <<<<< last value shouldn't be 0? stream APBIF0 Playback inactive <<<<< still inactive? in "static" "APBIF0 Playback" in "static" "APBIF0 Playback" out "APBIF0 RX" "I2S0 TX Mux"
So what's the rest of the route look like - what's I2S0 TX Mux look like and so on? For real widgets the number of outputs is a function of the rest of the graph so you need to trace through that, clearly you want to have some outputs but unless they are connected all the way back to an input DAPM will leave everything powered off. This is just like any other DAPM widget in this regard, everything behaves just like an analogue link.
Probably related, the I2S and WM8903 (DAI) drivers aren't being called by the ASoC core to initialize themselves for playback either; pretty much all that happens is that DMA is started. Is the machine driver responsible for this?
No, of course not. This is what soc_dapm_stream_event() is all about. It looks like all that's happening is that since DAPM hasn't found a complete route it didn't power anything (including the inter device links) on.
At this point, I'm not sure whether I have a gross mis-understanding of how this is supposed to work, or whether there are simply bits missing from the DAPM code to fully support CODEC/CODEC links, rather than CPU/CODEC links.
It's really hard to comment in much more detail, you've not shown enough information. What are the DAI links connecting the devices? Is the rest of the DAPM path through the system connected? My best guess is that (possibly due to the linking of the playback widgets) you don't have a full DAPM path.
This is all working just fine in mainline on littlemill.
int snd_soc_dapm_dai_get_connected_widgets(struct snd_soc_dai *dai, int stream, struct snd_soc_dapm_widget_list **list)
...
if (stream == SNDRV_PCM_STREAM_PLAYBACK) paths = is_connected_output_ep(dai->playback_widget, list); else paths = is_connected_input_ep(dai->playback_widget, list);
I would have expected this to use capture_widget on the final line, but I haven't thought about this in detail, just noticed the lack of symmetry by very brief inspection.
Yes, that looks buggy. Don't think there's any mainline users so nobody would notice.
On Fri, 2012-06-01 at 00:37 +0100, Mark Brown wrote:
On Thu, May 31, 2012 at 04:49:26PM -0600, Stephen Warren wrote:
This is all working just fine in mainline on littlemill.
int snd_soc_dapm_dai_get_connected_widgets(struct snd_soc_dai *dai, int stream, struct snd_soc_dapm_widget_list **list)
...
if (stream == SNDRV_PCM_STREAM_PLAYBACK) paths = is_connected_output_ep(dai->playback_widget, list); else paths = is_connected_input_ep(dai->playback_widget, list);
I would have expected this to use capture_widget on the final line, but I haven't thought about this in detail, just noticed the lack of symmetry by very brief inspection.
Yes, that looks buggy. Don't think there's any mainline users so nobody would notice.
Gah, it's a bug - I did have it fixed before the upstreaming but this fix seems to have been lost.
Patch on it's way.
Liam
Hi Liam There seem to be a similar mistake in the implementation of is_connected_output_ep / is_connected_input_ep:
In function is_connected_output_ep: -----------------------------------------------
list_for_each_entry(path, &widget->sinks, list_source) { ... err = dapm_list_add_widget(list, path->sink); ... }
In function is_connected_input_ep: ---------------------------------------------
list_for_each_entry(path, &widget->sources, list_sink) { ... err = dapm_list_add_widget(list, path->sink); }
I would have expected to have for the latter: err = dapm_list_add_widget(list, path->source);
regards Sebastien
-----Original Message----- From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of Liam Girdwood Sent: Friday, June 01, 2012 7:02 PM To: Mark Brown Cc: alsa-devel@alsa-project.org; Stephen Warren Subject: Re: [alsa-devel] Confusing about Playback/Capture, CODEC/CODEC links, and snd_soc_dapm_link_dai_widgets()
On Fri, 2012-06-01 at 00:37 +0100, Mark Brown wrote:
On Thu, May 31, 2012 at 04:49:26PM -0600, Stephen Warren wrote:
This is all working just fine in mainline on littlemill.
int snd_soc_dapm_dai_get_connected_widgets(struct snd_soc_dai *dai, int stream, struct snd_soc_dapm_widget_list **list)
...
if (stream == SNDRV_PCM_STREAM_PLAYBACK) paths = is_connected_output_ep(dai->playback_widget, list); else paths = is_connected_input_ep(dai->playback_widget, list);
I would have expected this to use capture_widget on the final line, but I haven't thought about this in detail, just noticed the lack of symmetry by very brief inspection.
Yes, that looks buggy. Don't think there's any mainline users so nobody would notice.
Gah, it's a bug - I did have it fixed before the upstreaming but this fix seems to have been lost.
Patch on it's way.
Liam
_______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Mon, 2012-06-04 at 15:02 +0200, Sebastien LEDUC wrote:
Hi Liam There seem to be a similar mistake in the implementation of is_connected_output_ep / is_connected_input_ep:
In function is_connected_output_ep:
list_for_each_entry(path, &widget->sinks, list_source) { ... err = dapm_list_add_widget(list, path->sink); ... }
In function is_connected_input_ep:
list_for_each_entry(path, &widget->sources, list_sink) { ... err = dapm_list_add_widget(list, path->sink); }
I would have expected to have for the latter: err = dapm_list_add_widget(list, path->source);
Your right, this patch also got somehow dropped from squashing prior to upstream. I'll send the fix shortly.
Thanks
Liam
On Fri, Jun 01, 2012 at 12:37:09AM +0100, Mark Brown wrote:
On Thu, May 31, 2012 at 04:49:26PM -0600, Stephen Warren wrote:
But if (2) is correct, I wonder why soc_dapm_stream_event()'s first if statement appears to consider the playback_widget of both sides of the DAI to be coupled; wouldn't one side's playback widget be coupled to the other side's capture widget?
The DAIs on CPUs have the opposite sense to DAIs on CODECs. In a traditional CPU<->CODEC link we do connect the two playback widgets directly to each other. The current code isn't correct, it's not normally noticable since most of the time the CPU DAI end of the link is a baseband which doesn't do anything real and is always activated bidirectionally so it really makes no odds which widget we look at.
I'll fix this tomorrow.
Sorry, I misremembered the context here (should've looked at the code not the quote!). The current code is correct, it will only be used in the case where we have a "real CPU" not the CODEC<->CODEC case so it's doing the right thing.
On 05/31/2012 05:37 PM, Mark Brown wrote:
On Thu, May 31, 2012 at 04:49:26PM -0600, Stephen Warren wrote:
[ having problems getting codec/codec DAI links working]
...
It's really hard to comment in much more detail, you've not shown enough information. What are the DAI links connecting the devices? Is the rest of the DAPM path through the system connected? My best guess is that (possibly due to the linking of the playback widgets) you don't have a full DAPM path.
Here's hopefully everything:
I'm playing back audio on CPU DAI named APBIF0. This is connected to AHUB XBAR DAI also named APBIF0. This link appears to be working, since the XBAR's stream node is active:
# cat tegra30-ahub-xbar/dapm/APBIF0\ Receive APBIF0 Receive: On in 1 out 1 stream APBIF0 Receive active out "static" "APBIF0 RX"
Tracing the path through the XBAR, the paths seem set up OK, but nothing else is active:
# cat tegra30-ahub-xbar/dapm/APBIF0\ RX APBIF0 RX: Off in 1 out 0 in "static" "APBIF0 Receive" out "APBIF0 RX" "I2S1 Mux"
# cat tegra30-ahub-xbar/dapm/I2S1\ Mux I2S1 Mux: Off in 1 out 0 in "APBIF0 RX" "APBIF0 RX" out "static" "I2S1 TX"
# cat tegra30-ahub-xbar/dapm/I2S1\ TX I2S1 TX: Off in 1 out 0 in "static" "I2S1 Mux" out "static" "I2S1 Transmit"
# cat tegra30-ahub-xbar/dapm/I2S1\ Transmit I2S1 Transmit: Off in 1 out 0 stream I2S1 Transmit inactive in "static" "I2S1 TX"
There is also a DAI link from XBAR's I2S1 TX/Transmit to the I2S controller's CIF Receive/RX, which appears to have been linked correctly since both DAI names show up in the I2S controller's CIF Receive stream debugfs file (see "in" line):
# cat tegra30-i2s.1/dapm/CIF\ Receive CIF Receive: Off in 0 out 0 stream CIF Receive inactive in "static" "I2S1 Transmit-CIF Receive" out "static" "CIF RX"
(I assume that the "in" line of the AHUB's "APBIF0 Receive" debugfs file way above is only "APBIF0" instead of "APBIF0-APBIF0" since it's a cpu/codec link not a codec/codec link?)
And again, the path through the I2S controller looks complete:
# cat tegra30-i2s.1/dapm/CIF\ RX CIF RX: Off in 0 out 0 in "static" "CIF Receive" out "static" "DAP TX"
# cat tegra30-i2s.1/dapm/DAP\ TX DAP TX: Off in 0 out 0 in "static" "CIF RX" out "static" "DAP Transmit"
# cat tegra30-i2s.1/dapm/DAP\ Transmit DAP Transmit: Off in 0 out 0 stream DAP Transmit inactive in "static" "DAP TX"
Finally, there's a DAI link from the I2S controller's DAP DAI to the WM8903's DAI, which again shows up in the debugfs file:
# cat wm8903.4-001a/dapm/Playback Playback: Off in 0 out 8 stream Playback inactive in "static" "DAP Transmit-Playback" out "static" "AIFRXL" out "static" "AIFRXR" out "static" "AIFRXL" out "static" "AIFRXR"
For completeness, here's the route through the WM8903, although nothing in that codec has changed since before I started reworking the Tegra drivers:
# cd wm8903.4-001a/dapm/
# cat AIFRXL AIFRXL: Off in 0 out 2 stream Left Playback inactive in "static" "Playback" in "static" "Playback" out "Left" "Left Playback Mux"
# cat Left\ Playback\ Mux Left Playback Mux: Off in 0 out 2 in "Left" "AIFRXL" out "static" "DACL"
# cat DACL DACL: Off in 0 out 2 - R18(0x12) bit 3 in "static" "CLK_DSP" in "static" "DACL Sidetone" in "static" "Left Playback Mux" out "DACL Switch" "Left Speaker Mixer" out "DACL Switch" "Left Output Mixer"
# cat Left\ Speaker\ Mixer Left Speaker Mixer: Off in 0 out 2 - R16(0x10) bit 1 in "DACL Switch" "DACL" out "static" "Left Speaker PGA"
# cat Left\ Speaker\ PGA Left Speaker PGA: Off in 0 out 2 - R17(0x11) bit 1 in "static" "Left Speaker Mixer" out "static" "LON" out "static" "LOP"
# cat LON LON: Off in 0 out 1 in "static" "Left Speaker PGA" out "static" "Int Spk"
# amixer cget name='Int Spk Switch' numid=87,iface=MIXER,name='Int Spk Switch' ; type=BOOLEAN,access=rw------,values=1 : values=on
(I do note that on IRC you said soc-pcm might be a better fit for this HW now, but since I figure I'm very close, I'd like to get this working and experiment with it a bit either way)
Thanks so much for the help.
On 05/31/2012 04:49 PM, Stephen Warren wrote:
[I'm having issues with CODEC/CODEC links]
I believe I've tracked down the issue I'm having. Playback isn't working because of a lack of a complete DAPM path from CPU DAI to the final CODEC in the chain. This path does not exist, because some DAIs on some CODECs in the chain are getting DAI widgets created multiple times.
I have the following DAIs/links:
CPU DAI is dev=tegra30-ahub-apbif dai=APBIF0
link (1) from APBIF0 in ^ to APBIF0 in /
CODEC dev=tegra30-ahub-xbar with DAIs APBIF0, I2S1
link (2) from I2S1 in ^ to CIF in /
CODEC dev=tegra30-i2s.1 with DAIs CIF, DAP
link (3) from DAP in ^ to wm8903-hifi in /
CODEC wm8903.4-001a dai=wm8903-hifi
The links in the machine driver are listed in order (1) (3) (2).
The problem is that when link (3) is instantiated, the DAP DAI is probed on its own as just a CPU DAI. This happens in soc_probe_dai_link() in the "probe the cpu_daiI" logic.
Then later when link (2) is instantiated, the entire tegra30-i2s.1 CODEC is probed. This happens in soc_probe_dai_link() in the "probe the CODEC" logic.
Both those two things call snd_soc_dapm_new_dai_widgets() for the I2S CODEC's DAP DAI. Furthermore, the DAPM context in each case is different. For link (2) it's the I2S1 CODEC's DAPM context; see the call to snd_soc_dapm_new_dai_widgets() from soc_probe_dai_link(). For link (3), it's the DAP DAI's own context; see the call to snd_soc_dapm_new_dai_widgets() from snd_soc_probe_codec().
So, there end up being two DAP DAI widgets created for the one DAI.
In turn, I believe that snd_soc_dapm_add_route() ends up being confused by this, either setting up the wrong route, or none.
To solve this:
Initially, I thought that snd_soc_dapm_new_dai_widgets() could just return if the DAI already had widgets, but that doesn't work, since the widgets were created for the wrong DAPM context - the I2S1 DAP DAI's rather than the I2S CODEC's.
Perhaps instead of blindly probing the CPU DAI, soc_probe_dai_link() should check whether that DAI is part of a CODEC, and instead of calling try_module_get() and snd_soc_dapm_new_dai_widgets(), it should just call soc_probe_codec() on the parent CODEC (guarded by whether the CODEC was already probed). Does that sound right? I'm attempting to make that work now, in case it's right.
On Tue, Jun 05, 2012 at 02:24:47PM -0600, Stephen Warren wrote:
Initially, I thought that snd_soc_dapm_new_dai_widgets() could just return if the DAI already had widgets, but that doesn't work, since the widgets were created for the wrong DAPM context - the I2S1 DAP DAI's rather than the I2S CODEC's.
Perhaps instead of blindly probing the CPU DAI, soc_probe_dai_link() should check whether that DAI is part of a CODEC, and instead of calling try_module_get() and snd_soc_dapm_new_dai_widgets(), it should just call soc_probe_codec() on the parent CODEC (guarded by whether the CODEC was already probed). Does that sound right? I'm attempting to make that work now, in case it's right.
No, we should just probe CODECs sensibly before we do any of the DAIs instead of trying to guess what we're doing in the middle of handling the DAI links. Your changes will be making the logic even more complicated here, it should be getting simpler - the reason this blew up for you is that the probe logic is already far too baroque. Given the data we have it'll boil down to similar checks bit it'll be in a clear, comprehensible CODEC probe step rather than intertwined with other stuff. This will also mean that aux_devs make more sense.
On 06/05/2012 02:48 PM, Mark Brown wrote:
On Tue, Jun 05, 2012 at 02:24:47PM -0600, Stephen Warren wrote:
Initially, I thought that snd_soc_dapm_new_dai_widgets() could just return if the DAI already had widgets, but that doesn't work, since the widgets were created for the wrong DAPM context - the I2S1 DAP DAI's rather than the I2S CODEC's.
Perhaps instead of blindly probing the CPU DAI, soc_probe_dai_link() should check whether that DAI is part of a CODEC, and instead of calling try_module_get() and snd_soc_dapm_new_dai_widgets(), it should just call soc_probe_codec() on the parent CODEC (guarded by whether the CODEC was already probed). Does that sound right? I'm attempting to make that work now, in case it's right.
No, we should just probe CODECs sensibly before we do any of the DAIs instead of trying to guess what we're doing in the middle of handling the DAI links. Your changes will be making the logic even more complicated here, it should be getting simpler - the reason this blew up for you is that the probe logic is already far too baroque. Given the data we have it'll boil down to similar checks bit it'll be in a clear, comprehensible CODEC probe step rather than intertwined with other stuff. This will also mean that aux_devs make more sense.
OK, that's actually what my inclination was, but I wasn't sure about changing all the probing in such a radical way, so I didn't mention it.
So, should the logic be something like this early-ish in snd_soc_instantiate_card():
for every dai link: if cpu side is a codec and it isn't probed probe it if codec side isn't probed probe it something similar for aux devs something similar for platforms?
And modify soc_probe_dai_link() to only probe DAIs, never anything else.
Is the following loop still required:
/* early DAI link probe */ for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST; order++) {
I'm not sure what the probe ordering stuff is achieving, and whether it's really intended for just CPU DAIs, just all DAIs, just CODECS, everything...
On Tue, Jun 05, 2012 at 03:17:15PM -0600, Stephen Warren wrote:
So, should the logic be something like this early-ish in snd_soc_instantiate_card():
for every dai link: if cpu side is a codec and it isn't probed probe it if codec side isn't probed probe it something similar for aux devs something similar for platforms?
And modify soc_probe_dai_link() to only probe DAIs, never anything else.
Yeah, something like this.
Is the following loop still required:
/* early DAI link probe */ for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST; order++) {
I'm not sure what the probe ordering stuff is achieving, and whether it's really intended for just CPU DAIs, just all DAIs, just CODECS, everything...
The only user of that is OMAP, I can't remember if they've actually got their users for that upstream or not. IIRC it's there to resolve clock order dependencies but I'm not sure there isn't a neater solution with something like cache only register I/O especially now we've got better infrastructure. Or if we can add something based on having a usable clock API, we can rely on but that doesn't seem too realistic in the medium term.
participants (4)
-
Liam Girdwood -
Mark Brown -
Sebastien LEDUC -
Stephen Warren