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.
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.
At this point, you leave me with no other conclusion but to assume that the reason you are being so unhelpful is that you don't understand this code, and that you don't know what the right solution is. I dare you to tell me I'm wrong: and the only thing that will convince me that I'm wrong is if you tell me how you'd like the above issue to be solved.
Just to be clear when I say either/or I'm talking about a DAI driver that can either run in S/PDIF mode or run in I2S mode in a given machine but not support both being hooked up in the same machine. Obviously this isn't the end goal but it might be a useful intermediate step if you find you are are blocked.
Well, it doesn't take much to realise that the CPU DAI needs to know whether the _single_ attached codec is SPDIF or I2S. How it does that is via which AIFs are bound - and as DPCM doesn't work at the moment, you can only do that by having _one_ DAI link specified, and DAPM routes between the AIFs and the codecs streams.
If you know a better way, you could try saying what that is, because at the moment what I've presented in these patches is the best that I can do with the limited knowledge of ASoC.
I'm not going to litter the driver with #ifdef's to work around this when there's a perfectly good way to work around it at runtime - which I've proven can work if the core code issue I point out above gets fixed.
Since you continue to refuse to take the patches, but haven't given any further reasons why, and I've shown your original objections to be provably false, you leave me no other options but to try and bypass
To reiterate please submit patches if you believe everything is OK now and then let's go through and review them.
You mean like your poor review attempt on the first round where you completely ignored the issue with the core code (the HACK patch)?
You mean like your poor review stating that the DAPM infrastructure I added in the CPU level code was all wrong, but it turns out that it's exactly the same as Liam's DPCM setup.
Why should I submit a new round of patches which haven't changed in any meaningful way (which I've already stated) for another review by someone who seems not to know what they're talking about? Can't you go back and look at the existing set again and provide a better quality of review, maybe providing some suggestions on the core issue which I keep pointing out?
This is precisely why I've called for Liam to look at them instead; I feel that I will get a much better quality of review from Liam, and maybe even some helpful suggestions about how to solve some of the remaining issues - something which has been totally lacking in every response from you.
you, especially when you have plainly stated that you don't care about Kirkwood stuff.
I think you'll find that anything I've said along those lines has been in relation to your strongly worded requests for me to make changes for you.
No, my strongly worded emails are directed at you not to ask you to make changes for me, but because you are being obstructive, uncooperative, and unhelpful. Apart from "use DPCM" and "DAPM is just a graph walk" I've had nothing of any real use what so ever back from you. Much of my effort has been based around my own interpretations and debugging of the ASoC code inspite of you, which may or may not be correct.
However, one thing I do know is that I've pointed out the bug above multiple times to you and there's still no movement on it - which just confirms my conclusion that you are just being plain obstructive.