[alsa-devel] [PATCH RFC 10/13] ASoC: kirkwood-t5325: add DAPM links between codec and cpu DAI

[Liam Girdwood]

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