On Sun, Aug 04, 2013 at 08:22:03PM +0100, Russell King wrote:

Merge these two structures together; nothing other than the I2S and DMA driver makes use of struct kirkwood_dma_data, and it's not like struct kirkwood_dma_data is really just used to convey DMA specific data to the backend; it's more a general shared structure between the two halves.

Applied, thanks.

On Sun, Aug 04, 2013 at 08:23:03PM +0100, Russell King wrote:

Use devm_clk_get() to simplify the error path for the external clock.

I've applied the diff that remains from the existing upstream code (which frees extclk if it's just the internal clock) with a reworded commit message, thanks.

On Sun, Aug 04, 2013 at 08:24:03PM +0100, Russell King wrote:

ASoC automatically creates snd_soc_dapm_dai_in and snd_soc_dapm_dai_out widgets for DAI drivers, and adds them to the list. Later on, ASoC creates automatic routes between these widgets and a widget with a stream name.

Applied, thanks. It's possible to do this a bit more neatly for example by checking the direction of the link but this works and fixes the problem, we can always improve it further later.

On Mon, Aug 05, 2013 at 12:13:07PM +0200, Jean-Francois Moine wrote:

On Sun, 04 Aug 2013 20:27:03 +0100 Russell King rmk+kernel@arm.linux.org.uk wrote:

...

These really should be a single driver because they're fully integrated in hardware. Make them so.

Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk

arch/arm/mach-dove/common.c | 4 ++-- arch/arm/mach-kirkwood/common.c | 24 +++++++++--------------- sound/soc/kirkwood/Kconfig | 5 ----- sound/soc/kirkwood/Makefile | 4 +--- sound/soc/kirkwood/kirkwood-dma.c | 30 +----------------------------- sound/soc/kirkwood/kirkwood-i2s.c | 21 ++++++++++++++++----- sound/soc/kirkwood/kirkwood-openrd.c | 4 ++-- sound/soc/kirkwood/kirkwood-t5325.c | 4 ++-- sound/soc/kirkwood/kirkwood.h | 2 ++ 9 files changed, 35 insertions(+), 63 deletions(-)

[snip]

Glad to see you got my patch, but why did you changed all the names?

You clearly didn't read the covering email to this series. I don't know why I bothered to write it if you don't bother to read it.

Replacing "kirkwood-pcm-audio" by "kirkwood-i2s" should be enough.

Quite simply, i2s is a bus protocol, just like spdif is. It can do both. If we're changing its name, then naming it after just one of the protocols it does is stupid. And PCM is also inappropriate - it does more than PCM because it can also transmit AC3 and MPEG audio over SPDIF to a suitable decoder (and that does work - I've tested it.)

At the end of the day, this is _just_ a DMA engine which outputs I2S and/or SPDIF formatted data of unknown type at a known rate.

On Mon, Aug 05, 2013 at 12:27:33PM +0100, Mark Brown wrote:

On Sun, Aug 04, 2013 at 08:31:04PM +0100, Russell King wrote:

...

Add the DAPM links to connect the codec DAC and ADCs to the cpu DAI I2S inputs/outputs.

...

• { "i2sdi", NULL, "Capture" },
• { "Playback", NULL, "i2sdo" },

This doesn't look good - this is adding DAPM routes which should correspond to the DAI link that's already been configured.

No, you're wrong there:

CPU DAI: Codec DAI dma-playback ---> i2sdo ---> Playback `--> spdifdo -> not connected dma-capture <--- i2sdi <--- Capture

And the intermediate level is needed to determine which outputs from the chip are wired up.

This structure, which is what ASoC normally does, for this SoC is wrong:

CPU DAI: Codec DAI dma-playback ---> Playback dma-capture <--- Capture

because it contains no information on how the connectivity between the codec and DAI is performed.

And don't even say "use dpcm" - if you say that, I want _you_ to write it because dpcm is totally and utterly unusable as it currently stands - as you can see from all the emails I've sent over the weekend.

On Mon, Aug 05, 2013 at 03:40:54PM +0100, Mark Brown wrote:

On Mon, Aug 05, 2013 at 12:33:10PM +0100, Russell King - ARM Linux wrote:

...

On Mon, Aug 05, 2013 at 12:27:33PM +0100, Mark Brown wrote:

...

...

This doesn't look good - this is adding DAPM routes which should correspond to the DAI link that's already been configured.

...

No, you're wrong there:

...

CPU DAI: Codec DAI dma-playback ---> i2sdo ---> Playback `--> spdifdo -> not connected dma-capture <--- i2sdi <--- Capture

...

And the intermediate level is needed to determine which outputs from the chip are wired up.

Right, and that's exactly what the dai_links are doing - they're showing what's hanging off each of the DAIs.

...

And don't even say "use dpcm" - if you say that, I want _you_ to write it because dpcm is totally and utterly unusable as it currently stands - as you can see from all the emails I've sent over the weekend.

I'm going to tell you to work with the framework rather than around it;

Work with the fscking undocumented framework. If you want that, then sodding well document this pile of crap. If not, stop telling people to "work around it".

adding routes that link the CODEC and CPU together in parallel with the links set up for the DAIs is not something that seems like it's going to continue to work sensibly going forwards.

So you're *TOTALLY* missing why I've had to do that. Tell me, how can the CPU DAI detect which of its frigging outputs are connected to the codec without doing the above? Show me how your crappy subsystem is supposed to work. Document the sodding thing.

If you don't do this, don't expect people to understand it. Don't expect people not to find "other solutions" around the utter shite that I've had to deal with ALL FRIGGING WEEKEND.

These patches stay as they are until you get a clue about what's required to drive this hardware and start providing some sensible assistance with your undocumented code.

On Mon, Aug 05, 2013 at 09:32:02PM +0100, Russell King - ARM Linux wrote:

which is similar to what's in Liam's driver. However, There is no .dpcm_playback member in mainline ASoC.

OK, that's interesting... presumably there's some other framework changes need upstreaming here, I've really not been paying attention to any out of tree code here.

So, let's summarise this. You're saying that I'm doing it all wrong in my driver creating those widgets and paths. Yet, the widgets and paths are exactly what Liam creates in his driver.

Including the CPU<->CODEC paths? Huh, so it seems. That wasn't required in any of the previous out of tree DPCM systems I've worked on in the past. Something's changed somewhere along the line here...

If those are required they really should be created by the framework in the same way as we do for CODEC<->CODEC links, it's not at all sensible to have to supply them twice and like I say it's just asking for problems.

Not only that, but we have even more duplicated widgets created with this method, even with the hack from this patch set.

snd_soc_dapm_new_dai_widgets: creating playback widget Playback:Playback for dai d81247c0 dapm d8263f10 (playback_widget was (null), new c05ab080) snd_soc_dapm_new_dai_widgets: creating playback widget Playback:Playback for dai d81247c0 dapm d8263c70 (playback_widget was c05ab080, new d8fe2b40)

OK, that's not good.

It seems Liam's commit needs to be reverted to fix that. Whether that's correct or not, I've no idea.

Needs a closer look I think.

Plus, when I try to set things up as Liam has, the result doesn't work, because of the widget naming scheme having no disambiguation.

That's easy enough to sidestep for now.

On Tue, Aug 06, 2013 at 12:30:15AM +0100, Russell King - ARM Linux wrote:

I put it to you that DPCM in mainline is incomplete and nonfunctional as it currently stands. Moreover, it requires either those who know that code to continue to develop it, or someone else who understands the direction that this code is supposed to go picks up to complete it.

I'd be most distressed if it were far off working; it's close enough to the out of tree code I've worked with and I know there were drivers in progress when it was submitted. We've certainly had at least one bug fix from the out of tree users, I'd be surprised if we had anything more substantial than bitrot in the current code.

Add to that that you say that this was being developed with TI and the TI situation happened, it seems unlikely to me that there's any significant chance of movement on that.

TI isn't going to be a big problem here. As you've seen Intel are currently using it for Haswell and they are generally keen on upstreaming, Qualcomm are also relying very heavily on this and the OMAP4s are still out there albeit with a reduced development team. There's a few others including one I expect to get round to when I'm not spending my time writing mails like this but those the the big systems with active use.

So, insisting that DPCM is used for this driver is grossly unfair.

The framework can't cope with simultaneous use of both interfaces unless DPCM is used or the framework is otherwise extended. This is not an unusual situation and given that there is already framework there it would seem to be the most sensible starting point. As is normal any driver local approach is going to need to not get in the way of framework development.

Let's not forget: Liam may be working on this stuff, and may have his own set of patches to sort out DPCM, so having patches from me messing around with the DPCM code apparantly "fixing" problems with it may very well be counter-productive. Again - Liam needs to provide some input on this.

Liam is on vacation this week, he'll be back next week.

On Sat, Aug 10, 2013 at 05:11:23PM +0100, Russell King - ARM Linux wrote:

On Tue, Aug 06, 2013 at 02:32:06PM +0100, Mark Brown wrote:

...

On Tue, Aug 06, 2013 at 12:30:15AM +0100, Russell King - ARM Linux wrote:

...

I put it to you that DPCM in mainline is incomplete and nonfunctional as it currently stands. Moreover, it requires either those who know that code to continue to develop it, or someone else who understands the direction that this code is supposed to go picks up to complete it.

I'd be most distressed if it were far off working; it's close enough to the out of tree code I've worked with and I know there were drivers in progress when it was submitted. We've certainly had at least one bug fix from the out of tree users, I'd be surprised if we had anything more substantial than bitrot in the current code.

Right, so, I've looked at this again today, and I've sort-of got it working.

I'm not so sure about that:

Unable to handle kernel NULL pointer dereference at virtual address 00000008 pgd = d2450000 [00000008] *pgd=12285831, *pte=00000000, *ppte=00000000 Internal error: Oops: 17 [#1] PREEMPT ARM Modules linked in: fuse bnep rfcomm bluetooth ext2 ext3 jbd snd_soc_spdif_tx m25p80 orion_wdt mtd snd_soc_kirkwood snd_soc_kirkwood_spdif CPU: 0 PID: 2514 Comm: vlc Not tainted 3.10.0+ #652 task: d8102800 ti: d38fa000 task.ti: d38fa000 PC is at snd_pcm_info+0xc8/0xd8 LR is at 0x30232065 pc : [<c030f724>] lr : [<30232065>] psr: a00f0013 sp : d38fbea8 ip : d8c2ead0 fp : c05de6d8 r10: c05de7d0 r9 : fffffdfd r8 : 00000000 r7 : d8c268a8 r6 : d8c26800 r5 : d8c26c00 r4 : d8c2ea00 r3 : 00000000 r2 : d8c2ea00 r1 : 00000001 r0 : d8c26c00 Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user Control: 10c5387d Table: 12450019 DAC: 00000015 Process vlc (pid: 2514, stack limit = 0xd38fa248) Stack: (0xd38fbea8 to 0xd38fc000) bea0: c0af7144 d8c2ea00 d8c26c00 ab5032b8 00000001 c030f768 bec0: 00000000 d8c20000 ab5032b8 c030a67c 0000001b d116b840 d8380330 c1205531 bee0: 0000001b d116b840 d8263fc0 d8c20000 ab5032b8 c03056b0 00000001 c05e6e80 bf00: c05e6e88 c05be828 00020120 00000000 d38fa000 600f0013 00000001 0000001b bf20: d38fa000 00020000 ab5032b8 c0088fec 00000001 00000000 d38fa000 00000000 bf40: 600f0013 ca17c380 0000001b ab5032b8 d8380330 0000001b d38fa000 00020000 bf60: ab5032b8 c00e50bc c00edecc 00020000 ab5032b8 00000001 ca17c380 ab5032b8 bf80: c1205531 c00e5394 ab50366c 00000001 00000000 000120b0 ab50366c 00000036 bfa0: c000e5a8 c000e3e0 00000000 000120b0 0000001b c1205531 ab5032b8 a91a3e10 bfc0: 00000000 000120b0 ab50366c 00000036 ab503454 00000001 00000000 ab5032b8 bfe0: b69a00f4 ab5032a4 b6933109 b6e0d07c a00f0010 0000001b aaaaaaaa aaaaaaaa [<c030f724>] (snd_pcm_info+0xc8/0xd8) from [<c030f768>] (snd_pcm_info_user+0x34/0x9c) [<c030f768>] (snd_pcm_info_user+0x34/0x9c) from [<c030a67c>] (snd_pcm_control_ioctl+0x274/0x280) [<c030a67c>] (snd_pcm_control_ioctl+0x274/0x280) from [<c03056b0>] (snd_ctl_ioctl+0xc0/0x55c) [<c03056b0>] (snd_ctl_ioctl+0xc0/0x55c) from [<c00e50bc>] (do_vfs_ioctl+0x80/0x31c) [<c00e50bc>] (do_vfs_ioctl+0x80/0x31c) from [<c00e5394>] (SyS_ioctl+0x3c/0x60) [<c00e5394>] (SyS_ioctl+0x3c/0x60) from [<c000e3e0>] (ret_fast_syscall+0x0/0x48) Code: e1a00005 e59530dc e3a01001 e1a02004 (e5933008)

That is caused by:

/* AB: FIXME!!! This is definitely nonsense */ if (runtime) { info->sync = runtime->sync; substream->ops->ioctl(substream, SNDRV_PCM_IOCTL1_INFO, info); }

where substream->ops is NULL.

So, we're back to square one and DPCM not being correctly functional.

On Mon, Aug 12, 2013 at 08:40:15AM +0100, Liam Girdwood wrote:

Russell, I'm just back from holiday now and have done a quick re test on Haswell. I can see the correct DAPM path and DPCM state as expected for Haswell, but let me get through my Inbox today and I'll see where we have some differences tomorrow.

Liam, welcome back.

Looking at this last night, I notice this:

* Creates a new internal PCM instance with no userspace device or procfs * entries. This is used by ASoC Back End PCMs in order to create a PCM that * will only be used internally by kernel drivers. i.e. it cannot be opened * by userspace. It provides existing ASoC components drivers with a substream * and access to any private data. * * The pcm operators have to be set afterwards to the new instance * via snd_pcm_set_ops().

Note the final paragraph. So, soc-pcm.c does this:

/* create the PCM */ if (rtd->dai_link->no_pcm) { snprintf(new_name, sizeof(new_name), "(%s)", rtd->dai_link->stream_name);

ret = snd_pcm_new_internal(rtd->card->snd_card, new_name, num, playback, capture, &pcm); } else { ... if (rtd->dai_link->no_pcm) { if (playback) pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd; if (capture) pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd; goto out; } ... if (playback) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &rtd->ops);

if (capture) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &rtd->ops); ... out: dev_info(rtd->card->dev, " %s <-> %s mapping ok\n", codec_dai->name, cpu_dai->name); return ret;

Hence, soc-pcm.c never sets the ops for the 'internal' stream, which, because it uses the dummy DAI, has both a playback and a capture stream.

The above is the only place where the ops are set on the ALSA stream(s) by ASoC, which means there's no way that the internal streams can ever have a non-NULL ops pointer, and suggests a possible reason why I get this oops.

On Tue, Aug 13, 2013 at 03:59:12PM +0100, Liam Girdwood wrote:

On Mon, 2013-08-12 at 09:28 +0100, Russell King - ARM Linux wrote:

...

On Mon, Aug 12, 2013 at 08:40:15AM +0100, Liam Girdwood wrote:

...

Russell, I'm just back from holiday now and have done a quick re test on Haswell. I can see the correct DAPM path and DPCM state as expected for Haswell, but let me get through my Inbox today and I'll see where we have some differences tomorrow.

Liam, welcome back.

Looking at this last night, I notice this:

• Creates a new internal PCM instance with no userspace device or procfs
• entries. This is used by ASoC Back End PCMs in order to create a PCM that
• will only be used internally by kernel drivers. i.e. it cannot be opened
• by userspace. It provides existing ASoC components drivers with a substream
• and access to any private data.
• The pcm operators have to be set afterwards to the new instance
• via snd_pcm_set_ops().

Note the final paragraph. So, soc-pcm.c does this:

    /* create the PCM */
    if (rtd->dai_link->no_pcm) {
            snprintf(new_name, sizeof(new_name), "(%s)",
                    rtd->dai_link->stream_name);

            ret = snd_pcm_new_internal(rtd->card->snd_card, new_name, num,
                            playback, capture, &pcm);
    } else {

... if (rtd->dai_link->no_pcm) { if (playback) pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd; if (capture) pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd; goto out; } ... if (playback) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &rtd->ops);

    if (capture)
            snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &rtd->ops);

... out: dev_info(rtd->card->dev, " %s <-> %s mapping ok\n", codec_dai->name, cpu_dai->name); return ret;

Hence, soc-pcm.c never sets the ops for the 'internal' stream, which, because it uses the dummy DAI, has both a playback and a capture stream.

The above is the only place where the ops are set on the ALSA stream(s) by ASoC, which means there's no way that the internal streams can ever have a non-NULL ops pointer, and suggests a possible reason why I get this oops.

Ah, Ok. The OMAP4 ABE driver always had ops so never hit this (probably true for Qualcomm driver too). I think we need a check at some point so that we don't dereference this if no ops are set.

Iiuc, there should be no harm in setting the ops (since ASoC checks for NULL) for either playback or capture as a workaround in the short term until the fix is upstream.

So where do we stand with all this stuff? As far as I'm concerned, there's nothing more _I_ can do with DPCM to make it work. DPCM seems to be broken.

Mark is adamant that the Kirkwood-i2s with SPDIF support _will_ use DPCM and refuses to take patches which don't.

So, until DPCM gets fixed, I can't proceed with getting SPDIF for kirkwood into mainline.

Either that, or I need you to ack the patches and tell Mark to accept them because they're actually correct and its just the core DPCM code which won't allow them to work (which is what I suspect the real situation to be.)

On Tue, Aug 20, 2013 at 12:44:21PM +0100, Mark Brown wrote:

On Tue, Aug 20, 2013 at 11:25:55AM +0100, Russell King - ARM Linux wrote:

...

Mark is adamant that the Kirkwood-i2s with SPDIF support _will_ use DPCM and refuses to take patches which don't.

No, that's for simultaneous use of I2S and S/PDIF. Using one or the other shouldn't be a problem, that's just a single DAI and a single PCM which has always been supported.

...

So, until DPCM gets fixed, I can't proceed with getting SPDIF for kirkwood into mainline.

I would suggest getting the either/or support merged first.

Sorry, I don't know how to do that.

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.

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.

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.

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, 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.

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

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

On Fri, Aug 23, 2013 at 01:58:03PM +0100, Russell King - ARM Linux wrote:

On Fri, Aug 23, 2013 at 01:13:14PM +0100, Liam Girdwood wrote:

...

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.

What I'm after are acks for those patches which are acceptable - which I believe is the entire series with the exception of the HACK patch.

If there no dependency then why is that patch included as part of this series, especially so early on? Obviously we shouldn't cause problems for existing machines in mainline so I stopped applying patches which seemed like they might depend on that one. I had at the time been expecting a revised version of the series to follow in due course with a better fix as at the time the hack was sent you'd only just determined what the problem was.

The split of the DMA backend from the CPU DAI backend is something which early ASoC forced, but that has "mostly" been fixed with (as far as I'm currently aware through testing) the problem I've been

Essentially all the dmaengine based platforms in mainline use a shared device for DMA and DAI; I'm fairly sure someone would have mentioned if there were problems.

As you have been repeatedly told the Kirkwood drivers are the first drivers submitted to mainline which use DPCM and therefore it is not surprising that there are a few issues which need to be worked through, there were a few revisions to the framework which went in as a result of review during the mainline merge. The problem you are seeing here is due to this being the first platform with a *shared* device to use DPCM.

On Fri, Aug 23, 2013 at 06:45:05PM +0100, Russell King - ARM Linux wrote:

Look Mark, frankly, shut your fucking mouth up about DPCM. This bug has nothing what so ever to do with DPCM, and the more times you try and state that doesn't change that *FACT*. And it is a FACT.

You must know that this and the rest of your e-mail are not an appropriate or constructive way to interact with people.

I now regret that have not confronted you on this more openly and directly before but unfortunately I had hoped that stepping back would help with deescalation. I know I have needed to take a step back on several occasions myself in order to be calmer in my response.

So that's why it fails _without_ any DPCM stuff? That's why the codecs fail to have their set_bias stuff called?

So, it is true that the widgets get double created in all cases where the same struct device is used for both platform and DAI. However if DPCM is not used nothing really notices since the SoC side widgets don't do anything useful. The relevance of DPCM is that it tries to use the widgets and therefore causes practical problems in normal use, otherwise it's just a memory leak on init.

As you know the issue with the DAPMless CODECs was unrelated to this, was due to poor test coverage of such devices and has been resolved by making DAPM mandatory for CODECs to eliminate the possibility of similar regressions caused by the need for special casing.

You've had this problem described by IRC, including extracts from the ASoC code indicating where things go wrong. I've shown you the debug I've used. I've shown you the result of that debug. You've had descriptions of this problem via email too. Yet you refuse to acknowledge that there could possibly be a problem here.

I am fairly sure that I have agreed that there is a bug and at the very least not said that everything is fine. Just for the record there clearly is a bug with double creation of DAI widgets when the same struct device is used to register both a DAI and a platform. You will also recall that Liam said he would provide a patch when he has time.

As I have told you before your generally confrontational and demanding approach is not a good one for getting help from people, it doesn't help push your problems up the priority list or generate positive attention. Especially with the more extreme examples it can actively work against engagement; at an unconscious level it makes things less fun and at a conscious level not only are breaks for calm needed but I know that I at least have no desire to encourage anyone to believe that this is a good way of getting results.

On Thu, Aug 29, 2013 at 11:12:35PM +0200, Liam Girdwood wrote:

Do you not see how statements like this will just end up making you look like someone who is very difficult to work with when faced with a technical disagreement ? I had acknowledged to you that this had not been tested for your configuration before you sent this email and that I would fix the issue.

Sorry, you're making the assumption that there's a _technical_ disagreement here. That's not how I view any of this; I believe that what's been going on has been a stream of misinformation and obstruction which is a totally different issue.

While it may be true that the bug affects DPCM _too_, it is not DPCM which is the problem. That's pretty obvious when you consider that the patch series which I posted (which is non-DPCM) trips over the bug as well.

Is it reasonable to continually claim that a bug is due to DPCM when the code being used which provokes it doesn't make any use of DPCM.

It's that kind of misinformation that I can't stand, and I will make it plainly obvious when there's blatent misinformation being banded out.

As a result of this, I can never trust anything Mark says; that's something which I established about a fortnight ago when this same old wrong story was trotted out for the I don't know how many time. Of course people are going to get frustrated and annoyed if that's the kind of behaviour that's being presented: when the facts show that something is wrong, repeating it many times doesn't make it any more right. All that it does is waste time.

I note that Mark has finally agreed earlier this week for the *first* time (in reply to the email you quoted) that the bug doesn't necessarily have anything to do with DPCM - which is at last some progress. To have taken four weeks to get to that point is pretty direbolical.

What about the oops bug in ALSA PCM that DPCM causes... am I going to have to spend another month trying to get that bug recognised? (That has been ignored too.)

If this is what it takes, then I'm seriously going to consider rewriting this as a plain ALSA driver and ditch ASoC; ASoC seems to be too buggy at the moment and it takes too long just to get bugs recognised.

It's not the actual fixing that I'm particularly worried about: it's a display that the maintainer correctly recognises the bug, and has the capability to fix it - and if they don't have time, they can communicate how they'd like it to be fixed or pass it to someone who does. Every time I've asked about how to fix the widget overwriting bug, the replies (if any) have avoided the question or given the same old misinformation.

You only need to go back and look at the initial patch submission to see that; the patch containing my hacked workaround has never had any kind of response. Is it acceptable to think "the submitter doesn't expect that to be merged so I'll ignore it"? The fact that any kind of hack appears in a patch series means that there's a problem which needs to be solved, and the submitter may not know how it should be fixed.

That's definitely the case with this patch set: more so since I'd already asked the question about how it should be solved and already got nowhere.

All in all, there's been:

1. The combining of the CPU and platform code(s) under one device is absolutely no problem for ASoC, there's multiple drivers which do this already. While this is a true statement, it creates the bug at the heart of this thread - and the bug _occuring_ has nothing to do with DPCM. The bug is caused solely by the combining of the two components.

2. Stream names in DAIs don't have to be globally unique. This is false if you want to try and bind to specific streams - even more so when you can end up with multiple codecs with DPCM, where all their output streams are called "Playback".

It may be that the stream names are not supposed to be used in DAPM routes, but that is not documented, and to date no one has made such a statement.

3. The infamous flippant "DAPM is just a graph walk" statement - it's more complicated than that because DAPM widgets have types which affect how the graph is walked, and the activation in multiple points in the graph make it more difficult to understand. As for the DAPM contexts which affect how the graph gets built, and in some cases whether some links even get created. No, it's more than "just a graph walk" - while true at a very basic level, it provides nothing of any use to anyone trying to understand this code - the response may as well have been "It's written in C".

(In the course of this, I've given an overview of it to a small number of people, and it takes a lot more than six words to even begin to describe what it is.)

4. By adding the AIFs and routes, I'm bypassing/abusing the ASoC DAI core. If this were to be the case, why does your DPCM driver set them up. Therefore, my conclusion is that Mark was wrong about this too. Yes, it may be that for the time being it's not that appealing, because as DPCM doesn't work in mainline, I'm unable to separate the DAI link between the CPU and the Codec. That doesn't make the creation of the routes any more wrong when they're the exact same routes which need to be created for DPCM - especially when they're relied upon for there to be any audio output what so ever from the driver.

5. Mark's "suggestion" that I can get this driver to work without this hack... I still don't see how, and Mark hasn't effectively explained how either - and I'm left wondering whether Mark even read the patch to see how it worked, and to see how and why it uses the AIF widgets, which brings into question the "review" which was done.

While the simple solution would be to add some ifdefs, that is not acceptable when you consider that this driver is already part of a multi-platform kernel - hard-coding it to one kind of output would definitely cause regressions. Also consider that this patch series was already my best effort at getting this going without using DPCM and without adding ifdefs without causing regressions for other Kirkwood platforms.

If those issues mean that I'm difficult to work with... well... I suspect anyone in my position receiving those kinds of responses would also end up getting extremely frustrated, as I have.

On Sat, Aug 10, 2013 at 05:11:23PM +0100, Russell King - ARM Linux wrote:

It would appear that the problem is that the AIF widgets are not finding their stream - the implicit routes are not being created. It _appears_ that the strean name in widgets are only ever connected to the streams within the same DAPM context.

That shouldn't be the case in general - DAPM binds first to a widget in the local context for disambiguation and then falls back to a search of the gobal context. However it is possible that you've got an ordering issue during registration.

So, it's because the capture isn't wired up. The capture isn't wired up on this platform, so how do I tell ASoC not to bother with the capture side with DPCM? Creating a link for the capture side to the dummy codec is wrong, because that allows capture to be brought up when in fact there is no capture wired up on the board (and means that we'll end up trying to use unconnected inputs.)

What I'd expect to happen is that the wiring for the SoC internal bits can be there since it physically is there but we don't generate anything that userspace can see unless we also provide a back end it can be wired up to. We probably need to fill that bit in though since I don't think anyone ever tried to use this stuff on a unidirectional system before.

but doesn't stop those warnings (not really surprising, because it isn't the card level which is being complained about). Probably the easiest way to stop that appearing is to just disable OSS compatibility.

That's something which should be documented until it is fixed - that DPCM is currently incompatible with OSS.

That's not likely to ever be supported even if anyone cared, very few ASoC drivers will work with OSS due to the very poor mapping between OSS parameter configuration and ALSA parameter configuration.

So, there are two issues which still need resolving:

1. The registration of Codec widgets from the platform introduced by Liam in be09ad90e17b79fdb0d513a31e814ff4d42e3dff ASoC: core: Add platform DAI widget mapping

Well, that patch isn't visibly doing anything with CODECs - it's acting on the CPU and platform, not on the CODECs. I think what's happening there is that if you've got the same device for the DAI and platform then the widgets will be created twice, once by the DAI and once by the platform. The current code is fine if they're separate devices but will fail if they are the same device.

We just need to make the DAPM context per device I think, possibly as part of moving over to components (based on the work that Morimoto-san started) though we could do it without that.

2. How to deal with disconnected front end streams which have no backend provided by their connected codec (in this case, front end can do capture and playback, back end can only do playback.)

Like I say I think we should just not be complaining if there's no capture back end available. Probably just checking if there's any at all is good enough for realistic uses.

On Sun, Aug 04, 2013 at 08:32:04PM +0100, Russell King wrote:

Codecs without any outputs now remain powered down, which means any paths to these codecs also remain powered down.

Applied, thanks.

Sebastian, Russell,

On Sun, 04 Aug 2013 23:45:30 +0200, Sebastian Hesselbarth wrote:

On 08/04/2013 09:21 PM, Russell King - ARM Linux wrote:

...

This is only a RFC at present due to the need to test on Kirkwood and fix at least one bug in ASoC core code.

I can't say that this stuff is fully tested, because I don't have any Kirkwood platforms, but I do have one Dove platform, which as I've said in the past doesn't run mainline. I don't run DT on it, so these patches also don't contain anything DT related.

Added Thomas Petazzoni to Cc, IIRC he has access to a T5325.

Yes, I have access to a Kirkwood t5325, which I'm currently trying to move to DT. Note that I also received Armada 370 audio hardware last week, so I'll be working on this as well in the future, this hardware has both in/out jack plugs as well as SPDIF ones.

So, yes, please Cc me on Kirkwood audio related developments.

Thanks!

Thomas Petazzoni

Dear Russell King - ARM Linux,

On Mon, 5 Aug 2013 09:53:34 +0100, Russell King - ARM Linux wrote:

...

Yes, I have access to a Kirkwood t5325, which I'm currently trying to move to DT. Note that I also received Armada 370 audio hardware last week, so I'll be working on this as well in the future, this hardware has both in/out jack plugs as well as SPDIF ones.

So, yes, please Cc me on Kirkwood audio related developments.

Can you pick the patches off the mailing list(s)? Re-sending them is just going to be noise for everyone.

Yes, sure. I was just talking about future postings of this patch set. For the current version, no need to resend just to Cc me.

Thanks!

Thomas

On 08/05/13 13:59, Mark Brown wrote:

On Sun, Aug 04, 2013 at 11:45:30PM +0200, Sebastian Hesselbarth wrote:

...

On 08/04/2013 09:21 PM, Russell King - ARM Linux wrote:

...

...

             .name = "S/PDIF1",
             .stream_name = "IEC958 Playback",
             .platform_name = "mvebu-audio.1",
             .cpu_dai_name = "mvebu-audio.1",
             .codec_dai_name = "dit-hifi",
             .codec_name = "spdif-dit",

...

Not a big deal for DT. I suggest to have a "audio-codecs" property that link CPU DAI to codec(s). One thing, I have noticed is that currently you need to supply both codec_name (or DT node) _and_ codec_dai_name. For that it would be helpful, if ASoC supplies helpers to get the DAI by index and use phandle with args here.

This should follow the same pattern as the other board bindings.

Mark,

looking at e.g. nvidia,tegra-audio-*.txt bindings, I guess you are proposing to have a new binding for every SoC/codec/board combination? Also, you should have seen "ASoC: fsl: Add S/PDIF machine driver", which is - again - proposing a new binding for fsl with spdif "codec".

You really want me to go the same way or is there any "other board binding" I should follow?

...

i2s1: audio-controller@b0000 { compatible = "marvell,dove-i2s"; reg = <0xb0000 0x2345>; audio-codecs = <&spdif 0>; };

No, things are glued together using the machine driver - again, look at how all the other systems work. The wiring on the board can get interesting enough to need a binding of its own, for very simple bindings that should just be pointing to a generic driver.

And that is what will be true for almost all codecs we hook up on Marvell SoCs. The audio controller is very simple with only mute control available. We need to connect either i2s or spdif or both to any codec and the codecs output. The codecs is determined by phandle and the output (codec_dai_name) could be determined by the phandle's arg, e.g. 0 for the first codec_dai provided.

That will be one machine driver for possibly all Marvell boards, not what e.g. tegra is doing with one machine driver for each board they can find. I understand that the exact wiring on each board may get complicated, but I don't see why a (well thought) binding should not be able to describe it?

Sebastian

On 08/05/13 16:07, Mark Brown wrote:

On Mon, Aug 05, 2013 at 03:06:16PM +0200, Sebastian Hesselbarth wrote:

...

looking at e.g. nvidia,tegra-audio-*.txt bindings, I guess you are proposing to have a new binding for every SoC/codec/board combination? Also, you should have seen "ASoC: fsl: Add S/PDIF machine driver", which is - again - proposing a new binding for fsl with spdif "codec".

...

You really want me to go the same way or is there any "other board binding" I should follow?

Yes, go that way. There is the simple-card driver which should be able to grow to consume most of these things for simple stereo CODECs, having the common clock API more widely available so we can create a hard requirement that clocks be described using the clock API would help a lot with that.

...

That will be one machine driver for possibly all Marvell boards, not what e.g. tegra is doing with one machine driver for each board they can find. I understand that the exact wiring on each board may get complicated, but I don't see why a (well thought) binding should not be able to describe it?

The clocking arrangements for a smartphone are not that trivial and we need to be able to cope with things like having more than two devices on the bus.

Mark,

I cannot follow you on the clocking arrangements. The binding proposal was for audio controller <-> CODEC connections. For clocks there are common properties ("clocks", "clock-names", "clock-frequency") to pass them to drivers - for "sound cards" in general there are not.

I am not referring to ASoC or ALSA here, but describing device inter-connection. And that can be done in a generic way to allow all those SoC/codec/board specific DT bindings to become very obsolete. The device nodes in question can still contain clocks properties of course, but you do not need a special compatible for e.g. "nvidia,tegra-audio-rt5640-beaver".

So, having a look at the node in question:

sound { compatible = "nvidia,tegra-audio-rt5640-beaver", "nvidia,tegra-audio-rt5640"; nvidia,model = "NVIDIA Tegra Beaver";

nvidia,audio-routing = "Headphones", "HPOR", "Headphones", "HPOL";

nvidia,i2s-controller = <&tegra_i2s1>; nvidia,audio-codec = <&rt5640>;

nvidia,hp-det-gpios = <&gpio TEGRA_GPIO(W, 2) GPIO_ACTIVE_HIGH>;

clocks = <&tegra_car TEGRA30_CLK_PLL_A>, <&tegra_car TEGRA30_CLK_PLL_A_OUT0>, <&tegra_car TEGRA30_CLK_EXTERN1>; clock-names = "pll_a", "pll_a_out0", "mclk"; };

all those "nvidia," prefixed properties are not nvidia-specific at all. Also, all those "nvidia," properties would have fit very well into the i2s controller node - there may be more complex SoCs requiring an extra "sound card" node, Tegra is not.

Even those foo-gpios properties can be generalized and moved to ASoC; have a look at MMC drivers using one common "cd-gpios" property for very different drivers. What is so special with Tegra and this gpio that it needs a vendor-specific property?

I was asking for generalizing those exact properties to generic ones and them move support for them to the ASoC/ALSA framework. With all that DT binding re-review coming up, I doubt that the tegra binding will be accepted again anyway.

...

i2s1: audio-controller@b0000 { compatible = "marvell,dove-i2s"; reg = <0xb0000 0x2345>; audio-codecs = <&spdif 0>; };

No, things are glued together using the machine driver - again, look at how all the other systems work. The wiring on the board can get interesting enough to need a binding of its own, for very simple bindings that should just be pointing to a generic driver.

I learned to never match "device nodes" with "drivers" as there is simply no relation between them.

So, back to my original node proposal, can you tell me what is so different, except that I am not using "marvell,audio-codec(s)" but "audio-codecs"; and hook the one available codec output by using phandle/args instead of a new compatible string?

Sebastian

(Note: The above i2s1 node will contains "clocks" for sure, but that is not related to sound-specific properties.)

On 08/05/2013 06:59 PM, Mark Brown wrote:

On Mon, Aug 05, 2013 at 05:04:35PM +0200, Sebastian Hesselbarth wrote:

...

I cannot follow you on the clocking arrangements. The binding proposal was for audio controller <-> CODEC connections. For clocks there are common properties ("clocks", "clock-names", "clock-frequency") to pass them to drivers - for "sound cards" in general there are not.

The clocking arrangements are an example of why the boards can get interesting enough to describe for themselves.

I understand there are complex arrangements, I still don't think that you need a global super-node to describe them. Anyway, I am not voting against a super-node.

...

So, having a look at the node in question:

...

sound { compatible = "nvidia,tegra-audio-rt5640-beaver", "nvidia,tegra-audio-rt5640"; nvidia,model = "NVIDIA Tegra Beaver";

nvidia,audio-routing = "Headphones", "HPOR", "Headphones", "HPOL";

nvidia,i2s-controller = <&tegra_i2s1>; nvidia,audio-codec = <&rt5640>;

nvidia,hp-det-gpios = <&gpio TEGRA_GPIO(W, 2) GPIO_ACTIVE_HIGH>;

clocks = <&tegra_car TEGRA30_CLK_PLL_A>, <&tegra_car TEGRA30_CLK_PLL_A_OUT0>, <&tegra_car TEGRA30_CLK_EXTERN1>; clock-names = "pll_a", "pll_a_out0", "mclk"; };

all those "nvidia," prefixed properties are not nvidia-specific at all.

This is all because you have to add a prefix for DT.

Right, like we have on all the other non-standard properties like "pinctrl-0", "bla-gpios", "clocks", ... come on, just make them generic enough and you can omit the vendor prefix.

...

Also, all those "nvidia," properties would have fit very well into the i2s controller node

No, almost none of them have any place there. For example, the audio routing contains only connections to the CODEC so the I2S controller isn't in play, the headphone detection is connected to the AP but isn't connected to the I2S port.

From a quick look in tegra30_hub.h, I can see configuration registers i2s formatting. I assume that i2s controller on Tegra can therefore directly connected to a I2S codec, can't it? Then, with an existing i2s node and an existing codec node - why is there no place to link both?

I can think of use cases that are hard to describe in a link-to-link way, but none of them really requires a super-node nor special board-specific compatible strings. With that we can just have the root DT node be compatible with Beaver above and register all the old platform_device way.

[...]

...

I learned to never match "device nodes" with "drivers" as there is simply no relation between them.

That's clearly a thinko for device node.

I assume with "thinko" you mean "thought wrong" - IMHO the above statement is very true. If it wouldn't, why not just have a node for kirkwood-dma and kirkwood-i2s instead of merging the drivers? You think that will also be accepted by DT maintainers?

...

So, back to my original node proposal, can you tell me what is so different, except that I am not using "marvell,audio-codec(s)" but "audio-codecs"; and hook the one available codec output by using phandle/args instead of a new compatible string?

From my point of view I'd rather not be shoving vendor prefixes on all the properties, this is coming from DT convention requiring vendor prefixes on bindings.

I understand that those vendor prefixes are part of the helper functions of ASoC. But no other subsystem has a similar approach but though of properties generic enough to help drivers find what they need to know. Either ASoC is mis-interpreting the vendor-prefix requirement - or all other subsystems are.

Sebastian

On 08/05/2013 09:04 AM, Sebastian Hesselbarth wrote: ...

So, having a look at the node in question:

sound { compatible = "nvidia,tegra-audio-rt5640-beaver", "nvidia,tegra-audio-rt5640"; nvidia,model = "NVIDIA Tegra Beaver";

nvidia,audio-routing = "Headphones", "HPOR", "Headphones", "HPOL";

nvidia,i2s-controller = <&tegra_i2s1>; nvidia,audio-codec = <&rt5640>;

nvidia,hp-det-gpios = <&gpio TEGRA_GPIO(W, 2) GPIO_ACTIVE_HIGH>;

clocks = <&tegra_car TEGRA30_CLK_PLL_A>, <&tegra_car TEGRA30_CLK_PLL_A_OUT0>, <&tegra_car TEGRA30_CLK_EXTERN1>; clock-names = "pll_a", "pll_a_out0", "mclk"; };

all those "nvidia," prefixed properties are not nvidia-specific at all.

Indeed. I do still have a desire to do a more encompassing generic binding that could use generic property names. However, I'm pulled in many directions and never manage to get around to that:-( Months or even years ago I did get as far as posting a binding with my ideas, but it needs a lot more work to correctly separate out what can be generic and what still needs to be SoC-/board-specific, how to represent it all in DT, and how to code stuff up into useful utility code that ASoC machine drivers will find useful, since custom machine drivers will almost certainly be required to do the last pieces of integration of SoC-specific control, even with a relatively complete generic DT representation (unless you start putting Forth into DT for the last bits!)

Also, all those "nvidia," properties would have fit very well into the i2s controller node - there may be more complex SoCs requiring an extra "sound card" node, Tegra is not.

That's certainly not true. It only looks like that because the DTs we currently have upstream expose only a subset of the whole functionality of the system. It's certainly not possible to move all the properties into e.g. the I2S controller node in general, because the audio system as a whole spans far more devices than just a single I2S controller.

Even those foo-gpios properties can be generalized and moved to ASoC; have a look at MMC drivers using one common "cd-gpios" property for very different drivers. What is so special with Tegra and this gpio that it needs a vendor-specific property?

I was asking for generalizing those exact properties to generic ones and them move support for them to the ASoC/ALSA framework. With all that DT binding re-review coming up, I doubt that the tegra binding will be accepted again anyway.

I don't agree. It's not *generic*, but I don't see any requirement to only accept completely generic bindings. If there was such a requirement, we'd probably never get any more bindings for embedded-style audio HW... The only thing that might change is removing some of the prefixes from the property names.

On Mon, Aug 05, 2013 at 04:10:47PM +0200, Lars-Peter Clausen wrote:

In my opinion having different bindings for each board is not a good idea. Having a common set of property names will allow us to factor out the parsing to the ASoC core instead of doing this by hand for each board driver.

Yeah, that's the idea - right now there's blanks that need filling in by the machine drivers so we do have them but once those get dealt with the compatible strings can just be moved over into a generic binding.

Hi

...

Kuninori Morimoto started working on a xlate callback for CODECs and DAIs some time ago[1]. If nobody else is going to pick this up I'm probably going to continue his work on this.

Thank you for your offer about above. I'm sorry for my late work for xlate callback. but, unfortunately, I busy now, so, I can't work for it now. I'm very happy if someone/you can work for it.

My understanding for this ASoC DT support milestone is that

1) merge codec/component 2) add .of_xlate_dai_name on it 3) add DT support

But, do you think 1) is possible ? Becouse, now, too many functions are based on/using "codec" struct snd_soc_codec / struct snd_soc_codec_driver. In reality, merging these is super difficult IMO.

But adding .of_xlate_dai_name on each codec/component is not difficult, and, I can do it immediately.

what is your opinion ?

Best regards --- Kuninori Morimoto

On Fri, Aug 30, 2013 at 10:26:07AM +0200, Lars-Peter Clausen wrote:

On 08/30/2013 09:20 AM, Kuninori Morimoto wrote:

...

My understanding for this ASoC DT support milestone is that

...

1. merge codec/component
2. add .of_xlate_dai_name on it
3. add DT support

...

But, do you think 1) is possible ? Becouse, now, too many functions are based on/using "codec" struct snd_soc_codec / struct snd_soc_codec_driver. In reality, merging these is super difficult IMO.

The idea is to use the component struct as a base class for the codec and platform struct. So drivers will keep seeing what they see now and don't need to be updated. Lots of the fields that can be shared between component, platform and codec are only used by the core.

Indeed, and doing things that can be done with a direct search and replace is not so bad. Splitting things up makes life easier too. I'd been planning to start looking at the merge of CODEC and component myself during the next release cycle.