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

[Liam Girdwood]

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