[alsa-devel] [PATCH RFC 10/13] ASoC: kirkwood-t5325: add DAPM links between codec and cpu DAI
Russell King - ARM Linux
linux at arm.linux.org.uk
Thu Aug 22 16:16:58 EDT 2013
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.
More information about the linux-arm-kernel
mailing list