[PATCH] ASoC/multi-component: fix au1x platform

[PATCH] ASoC/multi-component: fix au1x platform

[Manuel Lauss]

[-- Attachment #1: Type: text/plain, Size: 1102 bytes --]

Hi Liam,

Attached is a patch which fixes the Alchemy (au1x) audio platform
in your "topic/multi-component" branch.

Tested on my Db1200.

One minor issue remains: with AC97, the following warning is generated,
courtesy of the generic ac97 codec:
asoc: ac97-hifi <-> au1xpsc-ac97.1 mapping ok
kobject (8fdc59b0): tried to init an initialized object, something is
seriously wrong.
Call Trace:
[<804959c4>] dump_stack+0x8/0x34
[<802a30e4>] kobject_init+0x50/0xcc
[<802ec27c>] device_initialize+0x2c/0x70
[<802ecb64>] device_register+0x14/0x28
[<8039e058>] snd_soc_instantiate_cards+0xa00/0xb10
[<8039e27c>] soc_probe+0x114/0x154
[<802ef0fc>] driver_probe_device+0xe4/0x1a0
[<802ee404>] bus_for_each_drv+0x60/0xb0
[<802ef370>] device_attach+0x74/0xa8
[<802ee1e8>] bus_probe_device+0x30/0x54
[<802ec9a0>] device_add+0x384/0x534
[<802f09a4>] platform_device_add+0x15c/0x1c8
[<805ccb14>] db1200_audio_load+0x70/0x9c
[<801004fc>] do_one_initcall+0xfc/0x1e0
[<805ba32c>] kernel_init+0xc8/0x168
[<80105904>] kernel_thread_helper+0x10/0x18

ALSA device list:
  #0: DB1200_AC97


Best regards,
      Manuel Lauss

[-- Attachment #2: 1000-ASoC-multi-component-fix-au1x-platform.patch --]
[-- Type: application/octet-stream, Size: 5613 bytes --]

From ef970bf6f84cee8eed8e4fa38f45fd5872d5b47b Mon Sep 17 00:00:00 2001
From: Manuel Lauss <manuel.lauss@googlemail.com>
Date: Thu, 19 Aug 2010 15:13:04 +0200
Subject: [PATCH] ASoC: multi-component: fix au1x platform

Fixes the au1x audio platform in Liam Girdwood's topic/multi-component branch.

Signed-off-by: Manuel Lauss <manuel.lauss@googlemail.com>
---
 arch/mips/alchemy/devboards/db1200/platform.c |   10 ++++++++--
 sound/soc/au1x/db1200.c                       |   18 +++++++++---------
 sound/soc/au1x/dbdma2.c                       |    7 +++++--
 sound/soc/au1x/psc-ac97.c                     |    7 +++++++
 sound/soc/au1x/psc-i2s.c                      |    9 ++++++++-
 5 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/arch/mips/alchemy/devboards/db1200/platform.c b/arch/mips/alchemy/devboards/db1200/platform.c
index 3fa34c3..f985373 100644
--- a/arch/mips/alchemy/devboards/db1200/platform.c
+++ b/arch/mips/alchemy/devboards/db1200/platform.c
@@ -429,8 +429,14 @@ static struct platform_device db1200_audio_dev = {
 	.resource	= au1200_psc1_res,
 };
 
+static struct platform_device db1200_ac97_codec = {
+	.name		= "ac97-codec",
+	.id		= 1,	/* on PSC1 */
+};
+
 static struct platform_device *db1200_devs[] __initdata = {
 	NULL,		/* PSC0, selected by S6.8 */
+	&db1200_ac97_codec,
 	&db1200_ide_dev,
 	&db1200_eth_dev,
 	&db1200_rtc_dev,
@@ -494,11 +500,11 @@ static int __init db1200_dev_init(void)
 	sw &= BCSR_SWITCHES_DIP_8 | BCSR_SWITCHES_DIP_7;
 	if (sw == BCSR_SWITCHES_DIP_8) {
 		bcsr_mod(BCSR_RESETS, 0, BCSR_RESETS_PSC1MUX);
-		db1200_audio_dev.name = "au1xpsc_i2s";
+		db1200_audio_dev.name = "au1xpsc-i2s";
 		printk(KERN_INFO " S6.7 ON : PSC1 mode I2S\n");
 	} else {
 		bcsr_mod(BCSR_RESETS, BCSR_RESETS_PSC1MUX, 0);
-		db1200_audio_dev.name = "au1xpsc_ac97";
+		db1200_audio_dev.name = "au1xpsc-ac97";
 		printk(KERN_INFO " S6.7 OFF: PSC1 mode AC97\n");
 	}
 
diff --git a/sound/soc/au1x/db1200.c b/sound/soc/au1x/db1200.c
index 8780c90..2065d03 100644
--- a/sound/soc/au1x/db1200.c
+++ b/sound/soc/au1x/db1200.c
@@ -26,11 +26,11 @@
 
 static struct snd_soc_dai_link db1200_ac97_dai = {
 	.name		= "AC97",
-	.stream_name	= "AC97 HiFi",
-	.cpu_dai_name	= "au1xpsc-ac97",
+	.stream_name	= "AC97 Playback",
+	.cpu_dai_name	= "au1xpsc-ac97.1",
 	.codec_dai_name	= "ac97-hifi",
-	.platform_name	=  "au1xpsc-pcm-audio",
-	.codec_name	= "ac97-codec",
+	.platform_name	= "au1xpsc-pcm-audio.1",
+	.codec_name	= "ac97-codec.1",
 };
 
 static struct snd_soc_card db1200_ac97_machine = {
@@ -74,11 +74,11 @@ static struct snd_soc_ops db1200_i2s_wm8731_ops = {
 
 static struct snd_soc_dai_link db1200_i2s_dai = {
 	.name		= "WM8731",
-	.stream_name	= "WM8731 PCM",
-	.cpu_dai_name	= "au1xpsc",
-	.codec_dai_name	= "wm8731-hifi"
-	.platform_name	= "au1xpsc-pcm-audio",
-	.codec_name	= "wm8731-codec.0-001a",
+	.stream_name	= "WM8731",
+	.cpu_dai_name	= "au1xpsc-i2s.1",
+	.codec_dai_name	= "wm8731-hifi",
+	.platform_name	= "au1xpsc-pcm-audio.1",
+	.codec_name	= "wm8731-codec.0-001b",
 	.ops		= &db1200_i2s_wm8731_ops,
 };
 
diff --git a/sound/soc/au1x/dbdma2.c b/sound/soc/au1x/dbdma2.c
index 00fdb9c..b6d3db8 100644
--- a/sound/soc/au1x/dbdma2.c
+++ b/sound/soc/au1x/dbdma2.c
@@ -344,7 +344,6 @@ struct snd_soc_platform_driver au1xpsc_soc_platform = {
 	.pcm_new	= au1xpsc_pcm_new,
 	.pcm_free	= au1xpsc_pcm_free_dma_buffers,
 };
-EXPORT_SYMBOL_GPL(au1xpsc_soc_platform);
 
 static int __devinit au1xpsc_pcm_drvprobe(struct platform_device *pdev)
 {
@@ -460,7 +459,11 @@ struct platform_device *au1xpsc_pcm_add(struct platform_device *pdev)
 	res[1].start = res[1].end = id[1];
 	res[0].flags = res[1].flags = IORESOURCE_DMA;
 
-	pd = platform_device_alloc("au1xpsc-pcm", -1);
+	/* match the device id with the PSC instance that is registering
+	 * us.  May be useful when ASoC one day can handle multiple independent
+	 * "cards".
+	 */
+	pd = platform_device_alloc("au1xpsc-pcm-audio", pdev->id);
 	if (!pd)
 		goto out;
 
diff --git a/sound/soc/au1x/psc-ac97.c b/sound/soc/au1x/psc-ac97.c
index 6a9516c..8f6c4f4 100644
--- a/sound/soc/au1x/psc-ac97.c
+++ b/sound/soc/au1x/psc-ac97.c
@@ -387,6 +387,13 @@ static int __devinit au1xpsc_ac97_drvprobe(struct platform_device *pdev)
 	au_writel(PSC_SEL_PS_AC97MODE | sel, PSC_SEL(wd));
 	au_sync();
 
+	/* name the DAI after this devices' instance ("au1xpsc-ac97.PSCINDEX").
+	 * XXX: Useful if ASoC ever gains the ability to handle multiple
+	 * independent "cards" (don't forget to dynamically allocate
+	 * the snd_soc_dai_driver structure then)
+	 */
+	au1xpsc_ac97_dai.name = kobject_name(&pdev->dev.kobj);
+
 	ret = snd_soc_register_dai(&pdev->dev, &au1xpsc_ac97_dai);
 	if (ret)
 		goto out1;
diff --git a/sound/soc/au1x/psc-i2s.c b/sound/soc/au1x/psc-i2s.c
index 94e560a..2e7e599 100644
--- a/sound/soc/au1x/psc-i2s.c
+++ b/sound/soc/au1x/psc-i2s.c
@@ -337,6 +337,13 @@ static int __devinit au1xpsc_i2s_drvprobe(struct platform_device *pdev)
 	 * time out.
 	 */
 
+	/* name the DAI after this devices' instance ("au1xpsc-i2s.PSCINDEX").
+	 * XXX: Useful if ASoC ever gains the ability to handle multiple
+	 * independent "cards" (don't forget to dynamically allocate
+	 * the snd_soc_dai_driver structure then)
+	 */
+	au1xpsc_i2s_dai.name = kobject_name(&pdev->dev.kobj);
+
 	ret = snd_soc_register_dai(&pdev->dev, &au1xpsc_i2s_dai);
 	if (ret)
 		goto out1;
@@ -427,7 +434,7 @@ static struct dev_pm_ops au1xpsci2s_pmops = {
 
 static struct platform_driver au1xpsc_i2s_driver = {
 	.driver		= {
-		.name	= "au1xpsc",
+		.name	= "au1xpsc-i2s",
 		.owner	= THIS_MODULE,
 		.pm	= AU1XPSCI2S_PMOPS,
 	},
-- 
1.7.2


[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC/multi-component: fix au1x platform

[Mark Brown]

On Thu, Aug 19, 2010 at 03:21:35PM +0200, Manuel Lauss wrote:

> Attached is a patch which fixes the Alchemy (au1x) audio platform
> in your "topic/multi-component" branch.

I would provide more detailed review but you've attached your patch as a
base64 encoded attachment rather than including it inline so my MUA
hasn't quoted it.

As far as your comments about multiple cards go this should now be
supported, though the fact that you're modifying the static data for the
DAI means that the aux1x drivers probably don't support this or only
support it in a limited fashion.

Your kobject_name() stuff looks pretty fishy - is there some reason for
not using dev_name()?

Re: [PATCH] ASoC/multi-component: fix au1x platform

[Manuel Lauss]

Hi Mark,

On Thu, Aug 19, 2010 at 4:06 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Aug 19, 2010 at 03:21:35PM +0200, Manuel Lauss wrote:
>
>> Attached is a patch which fixes the Alchemy (au1x) audio platform
>> in your "topic/multi-component" branch.
>
> I would provide more detailed review but you've attached your patch as a
> base64 encoded attachment rather than including it inline so my MUA
> hasn't quoted it.
>
> As far as your comments about multiple cards go this should now be
> supported, though the fact that you're modifying the static data for the
> DAI means that the aux1x drivers probably don't support this or only
> support it in a limited fashion.

The hardware does support it with the issue you mentioned fixed, but the
global "soc_ac97_ops" needs to go away first.  The alchemy line can
support up to 4 independent ac97 "cards" with certain models.


> Your kobject_name() stuff looks pretty fishy - is there some reason for
> not using dev_name()?

no reason, I'll change it.

Thanks,
      Manuel Lauss

Re: [PATCH] ASoC/multi-component: fix au1x platform

[Mark Brown]

On Thu, Aug 19, 2010 at 04:30:10PM +0200, Manuel Lauss wrote:
> On Thu, Aug 19, 2010 at 4:06 PM, Mark Brown

> > As far as your comments about multiple cards go this should now be
> > supported, though the fact that you're modifying the static data for the
> > DAI means that the aux1x drivers probably don't support this or only
> > support it in a limited fashion.

> The hardware does support it with the issue you mentioned fixed, but the
> global "soc_ac97_ops" needs to go away first.  The alchemy line can
> support up to 4 independent ac97 "cards" with certain models.

So this comment only applies to the AC97 DAIs, not the I2S ones?
Actually, even for AC97 I'd expect things to work fine off the top of my
head - while you are forced to use the same ops for everything the ops
is just a vtable which I'd expect to be the same for all DAIs?

Re: [PATCH] ASoC/multi-component: fix au1x platform

[Manuel Lauss]

On Thu, Aug 19, 2010 at 4:35 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Aug 19, 2010 at 04:30:10PM +0200, Manuel Lauss wrote:
>> On Thu, Aug 19, 2010 at 4:06 PM, Mark Brown
>
>> > As far as your comments about multiple cards go this should now be
>> > supported, though the fact that you're modifying the static data for the
>> > DAI means that the aux1x drivers probably don't support this or only
>> > support it in a limited fashion.
>
>> The hardware does support it with the issue you mentioned fixed, but the
>> global "soc_ac97_ops" needs to go away first.  The alchemy line can
>> support up to 4 independent ac97 "cards" with certain models.
>
> So this comment only applies to the AC97 DAIs, not the I2S ones?
> Actually, even for AC97 I'd expect things to work fine off the top of my
> head - while you are forced to use the same ops for everything the ops
> is just a vtable which I'd expect to be the same for all DAIs?

True, but I don't see how one can pass instance data to
the ac97_read/write/whatever functions.

Manuel

Re: [PATCH] ASoC/multi-component: fix au1x platform

[Mark Brown]

On Thu, Aug 19, 2010 at 04:39:17PM +0200, Manuel Lauss wrote:

> True, but I don't see how one can pass instance data to
> the ac97_read/write/whatever functions.

The struct snd_ac97 * passed in to them has a pointer to the struct
snd_card * for the card which should (via some further indirection)
allow you to get back to the ASoC specific structures.

Re: [PATCH] ASoC/multi-component: fix au1x platform

[Manuel Lauss]

On Thu, Aug 19, 2010 at 4:48 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Aug 19, 2010 at 04:39:17PM +0200, Manuel Lauss wrote:
>
>> True, but I don't see how one can pass instance data to
>> the ac97_read/write/whatever functions.
>
> The struct snd_ac97 * passed in to them has a pointer to the struct
> snd_card * for the card which should (via some further indirection)
> allow you to get back to the ASoC specific structures.

Are there an in-tree examples I could look at?  (and for example on how
to pass per-instance platform data around?  It's not immediately obvious
to me how to get data allocated in a platform_driver.probe() callback to
the various dai_ops and pcm_ops).

Thanks!
       Manuel Lauss

Re: [PATCH] ASoC/multi-component: fix au1x platform

[Manuel Lauss]

On Thu, Aug 19, 2010 at 4:58 PM, Manuel Lauss
<manuel.lauss@googlemail.com> wrote:
> On Thu, Aug 19, 2010 at 4:48 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
>> On Thu, Aug 19, 2010 at 04:39:17PM +0200, Manuel Lauss wrote:
>>
>>> True, but I don't see how one can pass instance data to
>>> the ac97_read/write/whatever functions.
>>
>> The struct snd_ac97 * passed in to them has a pointer to the struct
>> snd_card * for the card which should (via some further indirection)
>> allow you to get back to the ASoC specific structures.
>

>                                                                  (and for example on how
> to pass per-instance platform data around?  It's not immediately obvious
> to me how to get data allocated in a platform_driver.probe() callback to
> the various dai_ops and pcm_ops).

Nevermind, figured it out by accident.

Manuel Lauss

Re: [PATCH] ASoC/multi-component: fix au1x platform

[Mark Brown]

On Thu, Aug 19, 2010 at 04:58:55PM +0200, Manuel Lauss wrote:
> On Thu, Aug 19, 2010 at 4:48 PM, Mark Brown

Please remember to keep people CCed on Linux mailing list postings.

> > The struct snd_ac97 * passed in to them has a pointer to the struct
> > snd_card * for the card which should (via some further indirection)
> > allow you to get back to the ASoC specific structures.

> Are there an in-tree examples I could look at?  (and for example on how
> to pass per-instance platform data around?  It's not immediately obvious
> to me how to get data allocated in a platform_driver.probe() callback to
> the various dai_ops and pcm_ops).

This is the only embedded device I've ever seen where anyone wanted to
use multiple AC97 controllers so not directly.  If you look at the SoC
core you can see how the various ALSA structures are converted into ASoC
specific ones, though.

Re: [PATCH] ASoC/multi-component: fix au1x platform

[Manuel Lauss]

On Thu, Aug 19, 2010 at 5:52 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Aug 19, 2010 at 04:58:55PM +0200, Manuel Lauss wrote:
>> On Thu, Aug 19, 2010 at 4:48 PM, Mark Brown
>
> Please remember to keep people CCed on Linux mailing list postings.
>
>> > The struct snd_ac97 * passed in to them has a pointer to the struct
>> > snd_card * for the card which should (via some further indirection)
>> > allow you to get back to the ASoC specific structures.
>
>> Are there an in-tree examples I could look at?  (and for example on how
>> to pass per-instance platform data around?  It's not immediately obvious
>> to me how to get data allocated in a platform_driver.probe() callback to
>> the various dai_ops and pcm_ops).
>
> This is the only embedded device I've ever seen where anyone wanted to
> use multiple AC97 controllers so not directly.  If you look at the SoC
> core you can see how the various ALSA structures are converted into ASoC
> specific ones, though.

There are up to four PSCs, each capable of both AC97 and I2S operation,
with separate RX and TX DMA channels. I consider each PSC a separate "card".

I figured out how to access platform data in dai_ops,  however I see absolutely
no way to get at it from the ac97_ops.


Manuel

Re: [PATCH] ASoC/multi-component: fix au1x platform

[Mark Brown]

On Thu, Aug 19, 2010 at 05:59:24PM +0200, Manuel Lauss wrote:
> On Thu, Aug 19, 2010 at 5:52 PM, Mark Brown

> > This is the only embedded device I've ever seen where anyone wanted to
> > use multiple AC97 controllers so not directly. ?If you look at the SoC
> > core you can see how the various ALSA structures are converted into ASoC
> > specific ones, though.

> I figured out how to access platform data in dai_ops,  however I see absolutely
> no way to get at it from the ac97_ops.

The snd_card has private data...

Re: [PATCH] ASoC/multi-component: fix au1x platform

[Manuel Lauss]

On Thu, Aug 19, 2010 at 6:06 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Aug 19, 2010 at 05:59:24PM +0200, Manuel Lauss wrote:
>> On Thu, Aug 19, 2010 at 5:52 PM, Mark Brown
>
>> > This is the only embedded device I've ever seen where anyone wanted to
>> > use multiple AC97 controllers so not directly. ?If you look at the SoC
>> > core you can see how the various ALSA structures are converted into ASoC
>> > specific ones, though.
>
>> I figured out how to access platform data in dai_ops,  however I see absolutely
>> no way to get at it from the ac97_ops.
>
> The snd_card has private data...

Thanks.  However there's another problem:  the ac97 codec accesses the
snd_ac97_ops() before the AC97 controller has been set up, which, naturally,
results in an oops.  I'll keep the current crutch for ac97 in place for now.

AC97 SoC Audio Codec 0.6
CPU 0 Unable to handle kernel paging request at virtual address
00000050, epc == 803a42dc, ra == 80396ad0
Call Trace:
[<803a42dc>] au1xpsc_ac97_cold_reset+0x1c/0x14c
[<80396ad0>] snd_ac97_mixer+0x1dc/0x198c
[<803a2ea4>] ac97_soc_probe+0xa8/0xd4
[<8039db20>] snd_soc_instantiate_cards+0x4c8/0xb10
[<8039e27c>] soc_probe+0x114/0x154
[<802ef0fc>] driver_probe_device+0xe4/0x1a0
[<802ee404>] bus_for_each_drv+0x60/0xb0
[<802ef370>] device_attach+0x74/0xa8
[<802ee1e8>] bus_probe_device+0x30/0x54
[<802ec9a0>] device_add+0x384/0x534
[<802f09a4>] platform_device_add+0x15c/0x1c8
[<805ccb04>] db1200_audio_load+0x70/0x9c
[<801004fc>] do_one_initcall+0xfc/0x1e0
[<805ba32c>] kernel_init+0xc8/0x168
[<80105904>] kernel_thread_helper+0x10/0x18


Manuel