[alsa-devel] [PATCH 4/7] Alchemy: DB1200 AC97+I2S audio support.
Replaces the sample Alchemy PSC AC97 machine code with a DB1200 machine driver with AC97 and I2S support.
AC97/I2S can be selected at boot time by setting switch S6.7.
Cc: alsa-devel@alsa-project.org Signed-off-by: Manuel Lauss manuel.lauss@gmail.com --- arch/mips/alchemy/devboards/db1200/platform.c | 12 ++ sound/soc/au1x/Kconfig | 10 +- sound/soc/au1x/Makefile | 4 +- sound/soc/au1x/db1200.c | 189 +++++++++++++++++++++++++ sound/soc/au1x/sample-ac97.c | 144 ------------------- 5 files changed, 209 insertions(+), 150 deletions(-) create mode 100644 sound/soc/au1x/db1200.c delete mode 100644 sound/soc/au1x/sample-ac97.c
diff --git a/arch/mips/alchemy/devboards/db1200/platform.c b/arch/mips/alchemy/devboards/db1200/platform.c index cc3f663..b3142e0 100644 --- a/arch/mips/alchemy/devboards/db1200/platform.c +++ b/arch/mips/alchemy/devboards/db1200/platform.c @@ -428,6 +428,7 @@ static int __init db1200_dev_init(void) ARRAY_SIZE(db1200_i2c_devs));
/* SWITCHES: S6.8 I2C/SPI selector (OFF=I2C ON=SPI) + * S6.7 AC97/I2S selector (OFF=AC97 ON=I2S) */
/* NOTE: GPIO215 controls OTG VBUS supply. In SPI mode however @@ -466,6 +467,17 @@ static int __init db1200_dev_init(void) au_writel(pfc, SYS_PINFUNC); au_sync();
+ /* audio is handled in sound/soc/au1x/db1200.c; + * however we need I2C to talk to I2S codec. + */ + if ((bcsr->switches & 3) == BCSR_SWITCHES_DIP_8) { + bcsr->resets |= BCSR_RESETS_PSC1MUX; + printk(KERN_INFO " S6.7 ON : PSC1 mode I2S\n"); + } else { + bcsr->resets &= ~BCSR_RESETS_PSC1MUX; + printk(KERN_INFO " S6.7 OFF: PSC1 mode AC97\n"); + } + return platform_add_devices(db1200_devs, ARRAY_SIZE(db1200_devs)); } device_initcall(db1200_dev_init); diff --git a/sound/soc/au1x/Kconfig b/sound/soc/au1x/Kconfig index 410a893..4b67140 100644 --- a/sound/soc/au1x/Kconfig +++ b/sound/soc/au1x/Kconfig @@ -22,11 +22,13 @@ config SND_SOC_AU1XPSC_AC97 ## ## Boards ## -config SND_SOC_SAMPLE_PSC_AC97 - tristate "Sample Au12x0/Au1550 PSC AC97 sound machine" +config SND_SOC_DB1200 + tristate "DB1200 AC97+I2S audio support" depends on SND_SOC_AU1XPSC select SND_SOC_AU1XPSC_AC97 select SND_SOC_AC97_CODEC + select SND_SOC_AU1XPSC_I2S + select SND_SOC_WM8731 help - This is a sample AC97 sound machine for use in Au12x0/Au1550 - based systems which have audio on PSC1 (e.g. Db1200 demoboard). + Select this option to enable audio (AC97 or I2S) on the + Alchemy/AMD/RMI DB1200 demoboard. diff --git a/sound/soc/au1x/Makefile b/sound/soc/au1x/Makefile index 6c6950b..1687307 100644 --- a/sound/soc/au1x/Makefile +++ b/sound/soc/au1x/Makefile @@ -8,6 +8,6 @@ obj-$(CONFIG_SND_SOC_AU1XPSC_I2S) += snd-soc-au1xpsc-i2s.o obj-$(CONFIG_SND_SOC_AU1XPSC_AC97) += snd-soc-au1xpsc-ac97.o
# Boards -snd-soc-sample-ac97-objs := sample-ac97.o +snd-soc-db1200-objs := db1200.o
-obj-$(CONFIG_SND_SOC_SAMPLE_PSC_AC97) += snd-soc-sample-ac97.o +obj-$(CONFIG_SND_SOC_DB1200) += snd-soc-db1200.o diff --git a/sound/soc/au1x/db1200.c b/sound/soc/au1x/db1200.c new file mode 100644 index 0000000..a2cb02c --- /dev/null +++ b/sound/soc/au1x/db1200.c @@ -0,0 +1,189 @@ +/* + * DB1200 ASoC audio support code. + * + * (c) 2008 Manuel Lauss manuel.lauss@gmail.com + * + */ + +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/timer.h> +#include <linux/interrupt.h> +#include <linux/platform_device.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/soc.h> +#include <sound/soc-dapm.h> +#include <asm/mach-au1x00/au1000.h> +#include <asm/mach-au1x00/au1xxx.h> +#include <asm/mach-au1x00/au1xxx_psc.h> +#include <asm/mach-au1x00/au1xxx_dbdma.h> + +/* it sucks that the ASoC headers are not under include/ */ +#include "../codecs/ac97.h" +#include "../codecs/wm8731.h" +#include "psc.h" + +/*------------------------- AC97 PART ---------------------------*/ + +static int db1200_ac97_init(struct snd_soc_codec *codec) +{ + snd_soc_dapm_sync(codec); + return 0; +} + +static struct snd_soc_dai_link db1200_ac97_dai = { + .name = "AC97", + .stream_name = "AC97 HiFi", + .cpu_dai = &au1xpsc_ac97_dai, + .codec_dai = &ac97_dai, + .init = db1200_ac97_init, +}; + +static struct snd_soc_card db1200_ac97_machine = { + .name = "DB1200 AC97 Audio", + .dai_link = &db1200_ac97_dai, + .num_links = 1, + .platform = &au1xpsc_soc_platform, +}; + +static struct snd_soc_device db1200_ac97_devdata = { + .card = &db1200_ac97_machine, + .codec_dev = &soc_codec_dev_ac97, +}; + +/*------------------------- I2S PART ---------------------------*/ + +static int db1200_i2s_startup(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *codec_dai = rtd->dai->codec_dai; + struct snd_soc_dai *cpu_dai = rtd->dai->cpu_dai; + int ret; + + /* WM8731 has its own 12MHz crystal */ + snd_soc_dai_set_sysclk(codec_dai, WM8731_SYSCLK, + 12000000, SND_SOC_CLOCK_IN); + + /* codec is bitclock and lrclk master */ + ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_LEFT_J | + SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBM_CFM); + if (ret < 0) + goto out; + + ret = snd_soc_dai_set_fmt(cpu_dai, SND_SOC_DAIFMT_LEFT_J | + SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBM_CFM); + if (ret < 0) + goto out; + + ret = 0; +out: + return ret; +} + +static struct snd_soc_ops db1200_i2s_wm8731_ops = { + .startup = db1200_i2s_startup, +}; + +static int db1200_i2s_init(struct snd_soc_codec *codec) +{ + snd_soc_dapm_sync(codec); + return 0; +} + +static struct snd_soc_dai_link db1200_i2s_dai = { + .name = "WM8731", + .stream_name = "WM8731 PCM", + .cpu_dai = &au1xpsc_i2s_dai, + .codec_dai = &wm8731_dai, + .init = db1200_i2s_init, + .ops = &db1200_i2s_wm8731_ops, +}; + +static struct snd_soc_card db1200_i2s_machine = { + .name = "DB1200 I2S Audio", + .dai_link = &db1200_i2s_dai, + .num_links = 1, + .platform = &au1xpsc_soc_platform, +}; + +static struct snd_soc_device db1200_i2s_devdata = { + .card = &db1200_i2s_machine, + .codec_dev = &soc_codec_dev_wm8731, +}; + +/*------------------------- COMMON PART ---------------------------*/ + +static struct resource psc1_res[] = { + [0] = { + .start = CPHYSADDR(PSC1_BASE_ADDR), + .end = CPHYSADDR(PSC1_BASE_ADDR) + 0x000fffff, + .flags = IORESOURCE_MEM, + }, + [1] = { + .start = AU1200_PSC1_INT, + .end = AU1200_PSC1_INT, + .flags = IORESOURCE_IRQ, + }, + [2] = { + .start = DSCR_CMD0_PSC1_TX, + .end = DSCR_CMD0_PSC1_TX, + .flags = IORESOURCE_DMA, + }, + [3] = { + .start = DSCR_CMD0_PSC1_RX, + .end = DSCR_CMD0_PSC1_RX, + .flags = IORESOURCE_DMA, + }, +}; + +static struct platform_device *db1200_asoc_dev; + +static int __init db1200_audio_load(void) +{ + int ret; + + /* DB1200: PSC clock is supplied externally */ + au_writel(PSC_SEL_CLK_SERCLK, PSC1_BASE_ADDR + PSC_SEL_OFFSET); + au_sync(); + + ret = -ENOMEM; + db1200_asoc_dev = platform_device_alloc("soc-audio", -1); + if (!db1200_asoc_dev) + goto out; + + db1200_asoc_dev->resource = + kmemdup(psc1_res, sizeof(struct resource) * + ARRAY_SIZE(psc1_res), GFP_KERNEL); + db1200_asoc_dev->num_resources = ARRAY_SIZE(psc1_res); + db1200_asoc_dev->id = 1; + + /* DB1200 board setup set PSC1MUX to preferred audio device */ + if (bcsr->resets & BCSR_RESETS_PSC1MUX) + platform_set_drvdata(db1200_asoc_dev, &db1200_i2s_devdata); + else + platform_set_drvdata(db1200_asoc_dev, &db1200_ac97_devdata); + + db1200_ac97_devdata.dev = &db1200_asoc_dev->dev; + db1200_i2s_devdata.dev = &db1200_asoc_dev->dev; + ret = platform_device_add(db1200_asoc_dev); + + if (ret) { + platform_device_put(db1200_asoc_dev); + db1200_asoc_dev = NULL; + } +out: + return ret; +} + +static void __exit db1200_audio_unload(void) +{ + platform_device_unregister(db1200_asoc_dev); +} + +module_init(db1200_audio_load); +module_exit(db1200_audio_unload); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("DB1200 ASoC audio support"); +MODULE_AUTHOR("Manuel Lauss"); diff --git a/sound/soc/au1x/sample-ac97.c b/sound/soc/au1x/sample-ac97.c deleted file mode 100644 index 27683eb..0000000 --- a/sound/soc/au1x/sample-ac97.c +++ /dev/null @@ -1,144 +0,0 @@ -/* - * Sample Au12x0/Au1550 PSC AC97 sound machine. - * - * Copyright (c) 2007-2008 Manuel Lauss mano@roarinelk.homelinux.net - * - * This program is free software; you can redistribute it and/or modify - * it under the terms outlined in the file COPYING at the root of this - * source archive. - * - * This is a very generic AC97 sound machine driver for boards which - * have (AC97) audio at PSC1 (e.g. DB1200 demoboards). - */ - -#include <linux/module.h> -#include <linux/moduleparam.h> -#include <linux/timer.h> -#include <linux/interrupt.h> -#include <linux/platform_device.h> -#include <sound/core.h> -#include <sound/pcm.h> -#include <sound/soc.h> -#include <sound/soc-dapm.h> -#include <asm/mach-au1x00/au1000.h> -#include <asm/mach-au1x00/au1xxx_psc.h> -#include <asm/mach-au1x00/au1xxx_dbdma.h> - -#include "../codecs/ac97.h" -#include "psc.h" - -static int au1xpsc_sample_ac97_init(struct snd_soc_codec *codec) -{ - snd_soc_dapm_sync(codec); - return 0; -} - -static struct snd_soc_dai_link au1xpsc_sample_ac97_dai = { - .name = "AC97", - .stream_name = "AC97 HiFi", - .cpu_dai = &au1xpsc_ac97_dai, /* see psc-ac97.c */ - .codec_dai = &ac97_dai, /* see codecs/ac97.c */ - .init = au1xpsc_sample_ac97_init, - .ops = NULL, -}; - -static struct snd_soc_card au1xpsc_sample_ac97_machine = { - .name = "Au1xxx PSC AC97 Audio", - .dai_link = &au1xpsc_sample_ac97_dai, - .num_links = 1, -}; - -static struct snd_soc_device au1xpsc_sample_ac97_devdata = { - .card = &au1xpsc_sample_ac97_machine, - .platform = &au1xpsc_soc_platform, /* see dbdma2.c */ - .codec_dev = &soc_codec_dev_ac97, -}; - -static struct resource au1xpsc_psc1_res[] = { - [0] = { - .start = CPHYSADDR(PSC1_BASE_ADDR), - .end = CPHYSADDR(PSC1_BASE_ADDR) + 0x000fffff, - .flags = IORESOURCE_MEM, - }, - [1] = { -#ifdef CONFIG_SOC_AU1200 - .start = AU1200_PSC1_INT, - .end = AU1200_PSC1_INT, -#elif defined(CONFIG_SOC_AU1550) - .start = AU1550_PSC1_INT, - .end = AU1550_PSC1_INT, -#endif - .flags = IORESOURCE_IRQ, - }, - [2] = { - .start = DSCR_CMD0_PSC1_TX, - .end = DSCR_CMD0_PSC1_TX, - .flags = IORESOURCE_DMA, - }, - [3] = { - .start = DSCR_CMD0_PSC1_RX, - .end = DSCR_CMD0_PSC1_RX, - .flags = IORESOURCE_DMA, - }, -}; - -static struct platform_device *au1xpsc_sample_ac97_dev; - -static int __init au1xpsc_sample_ac97_load(void) -{ - int ret; - -#ifdef CONFIG_SOC_AU1200 - unsigned long io; - - /* modify sys_pinfunc for AC97 on PSC1 */ - io = au_readl(SYS_PINFUNC); - io |= SYS_PINFUNC_P1C; - io &= ~(SYS_PINFUNC_P1A | SYS_PINFUNC_P1B); - au_writel(io, SYS_PINFUNC); - au_sync(); -#endif - - ret = -ENOMEM; - - /* setup PSC clock source for AC97 part: external clock provided - * by codec. The psc-ac97.c driver depends on this setting! - */ - au_writel(PSC_SEL_CLK_SERCLK, PSC1_BASE_ADDR + PSC_SEL_OFFSET); - au_sync(); - - au1xpsc_sample_ac97_dev = platform_device_alloc("soc-audio", -1); - if (!au1xpsc_sample_ac97_dev) - goto out; - - au1xpsc_sample_ac97_dev->resource = - kmemdup(au1xpsc_psc1_res, sizeof(struct resource) * - ARRAY_SIZE(au1xpsc_psc1_res), GFP_KERNEL); - au1xpsc_sample_ac97_dev->num_resources = ARRAY_SIZE(au1xpsc_psc1_res); - au1xpsc_sample_ac97_dev->id = 1; - - platform_set_drvdata(au1xpsc_sample_ac97_dev, - &au1xpsc_sample_ac97_devdata); - au1xpsc_sample_ac97_devdata.dev = &au1xpsc_sample_ac97_dev->dev; - ret = platform_device_add(au1xpsc_sample_ac97_dev); - - if (ret) { - platform_device_put(au1xpsc_sample_ac97_dev); - au1xpsc_sample_ac97_dev = NULL; - } - -out: - return ret; -} - -static void __exit au1xpsc_sample_ac97_exit(void) -{ - platform_device_unregister(au1xpsc_sample_ac97_dev); -} - -module_init(au1xpsc_sample_ac97_load); -module_exit(au1xpsc_sample_ac97_exit); - -MODULE_LICENSE("GPL"); -MODULE_DESCRIPTION("Au1xxx PSC sample AC97 machine"); -MODULE_AUTHOR("Manuel Lauss mano@roarinelk.homelinux.net");
On Sun, Jun 07, 2009 at 08:39:01PM +0200, Manuel Lauss wrote:
Replaces the sample Alchemy PSC AC97 machine code with a DB1200 machine driver with AC97 and I2S support.
Acked-by: Mark Brown broonie@opensource.wolfsonmicro.com
If this is going to go in during the merge window I'm happy for it to go in via the MIPS tree but otherwise I'd much rather it goes via ASoC in case API changes cause merge issues. I don't know what you prefer, Ralf?
We could also split the MIPS and ASoC parts into separate patches if that helps.
+/* it sucks that the ASoC headers are not under include/ */ +#include "../codecs/ac97.h" +#include "../codecs/wm8731.h"
This is because they're internal to ASoC - having them out of include should set off big red warning flags for something outside ASoC is looking at them. If there are things that should be referenced outside ASoC they should be in a separate header in include/sound like the WM9081 platform data.
+static int db1200_ac97_init(struct snd_soc_codec *codec) +{
- snd_soc_dapm_sync(codec);
- return 0;
+}
This could be removed but it does no harm.
+/*------------------------- COMMON PART ---------------------------*/
+static struct resource psc1_res[] = {
- [0] = {
.start = CPHYSADDR(PSC1_BASE_ADDR),.end = CPHYSADDR(PSC1_BASE_ADDR) + 0x000fffff,.flags = IORESOURCE_MEM,
If you conver the I2S driver to use the standard device probing model this could all me moved into the architecture code rather than placed in machine drivers.
Hi Mark!
On Mon, Jun 8, 2009 at 11:25 AM, Mark Brownbroonie@opensource.wolfsonmicro.com wrote:
On Sun, Jun 07, 2009 at 08:39:01PM +0200, Manuel Lauss wrote:
Replaces the sample Alchemy PSC AC97 machine code with a DB1200 machine driver with AC97 and I2S support.
Acked-by: Mark Brown broonie@opensource.wolfsonmicro.com
If this is going to go in during the merge window I'm happy for it to go in via the MIPS tree but otherwise I'd much rather it goes via ASoC in case API changes cause merge issues. I don't know what you prefer, Ralf?
I'd rather have it all go through the MIPS tree; this is the only patch of 7 which touches files outside it, and it depends on another one to apply cleanly.
We could also split the MIPS and ASoC parts into separate patches if that helps.
+/* it sucks that the ASoC headers are not under include/ */ +#include "../codecs/ac97.h" +#include "../codecs/wm8731.h"
This is because they're internal to ASoC - having them out of include should set off big red warning flags for something outside ASoC is looking at them. If there are things that should be referenced outside ASoC they should be in a separate header in include/sound like the WM9081 platform data.
I'd actually love to move this file to the actual board code, however due to the way ASoC is organized this isn't at all possible. This is one of two things I don't like about ASoC: the machine registration is extremely awkward compared to other platform drivers. (the other being that ASoC doesn't support multiple machines [i.e. I could actually run both AC97 and I2S simultaneously one of my boards, as independent sound cards])
+static int db1200_ac97_init(struct snd_soc_codec *codec) +{
- snd_soc_dapm_sync(codec);
- return 0;
+}
This could be removed but it does no harm.
I'll drop it.
+/*------------------------- COMMON PART ---------------------------*/
+static struct resource psc1_res[] = {
- [0] = {
- .start = CPHYSADDR(PSC1_BASE_ADDR),
- .end = CPHYSADDR(PSC1_BASE_ADDR) + 0x000fffff,
- .flags = IORESOURCE_MEM,
If you conver the I2S driver to use the standard device probing model this could all me moved into the architecture code rather than placed in machine drivers.
Again, I'd love to, but can't: the AC97/I2S/DBDMA drivers extract base address and DMA information from the platform device resource structure; however I can't just copy the resource info from the this db1200_sound platform_driver to the soc_audio platform driver because the driver core complains about resource conflicts (two platform_devices sharing the same resources). Unless I missed a flag which needs to be passed to the resource.flags member?
Thanks! Manuel Lauss
On Mon, Jun 08, 2009 at 11:43:41AM +0200, Manuel Lauss wrote:
On Mon, Jun 8, 2009 at 11:25 AM, Mark Brownbroonie@opensource.wolfsonmicro.com wrote:
If this is going to go in during the merge window I'm happy for it to go in via the MIPS tree but otherwise I'd much rather it goes via ASoC in case API changes cause merge issues. I don't know what you prefer, Ralf?
I'd rather have it all go through the MIPS tree; this is the only patch of 7 which touches files outside it, and it depends on another one to apply cleanly.
Well, like I say if it's going via MIPS I'd really prefer it to go in this merge window. If not then I'd expect that splitting out the architecture parts from the machine driver as I suggested ought to deal with the merge issues.
I'd actually love to move this file to the actual board code, however due to the way ASoC is organized this isn't at all possible. This is one of two things I don't like about ASoC: the machine registration is extremely awkward
The main problem long term with trying to move it into the architecture code is that the drivers for anything non-trivial get large enough to qualify as actual drivers - add a jack or two in there, or some more fancy clocking for example. I also catch enough errors in the machine drivers I'm reviewing that I'm a bit nervous about reducing the level of review that they get.
At the minute the APIs are too fluid to make this realistic, anyway - you'd just get constant merge issues.
compared to other platform drivers. (the other being that ASoC doesn't support multiple machines [i.e. I could actually run both AC97 and I2S simultaneously one of my boards, as independent sound cards])
I've got a board sitting on my desk here which has multiple sound systems. Unfortunately the architecture code for it is a bit of a shambles at this point so it's more trouble to work on the platform than it's worth at the minute.
+static struct resource psc1_res[] = {
If you conver the I2S driver to use the standard device probing model this could all me moved into the architecture code rather than placed in machine drivers.
Again, I'd love to, but can't: the AC97/I2S/DBDMA drivers extract base address and DMA information from the platform device resource structure; however I can't just copy the resource info from the this db1200_sound platform_driver to the soc_audio platform driver because the driver core complains about resource conflicts (two platform_devices sharing the same resources). Unless I missed a flag which needs to be passed to the resource.flags member?
If you move the selection of the switch position to the architecture code then it can arrange to register only the device that is in use in the current configuration. If the DMA and DAI drivers both need the same resources they can cooperate with each other - the system will only bring the card on-line once both the DMA and DAI driver are present.
I'd not be too concerned about having the machine driver be able to flip between the two at run time - it's unlikely to be a realistic concern for anything except reference boards and even with those it's relatively unlikely that anyone will be doing that on a regular basis. The wins for general users of the driver should cause an overall reduction in pain.
Hi Mark,
On Mon, Jun 8, 2009 at 12:20 PM, Mark Brownbroonie@opensource.wolfsonmicro.com wrote:
On Mon, Jun 08, 2009 at 11:43:41AM +0200, Manuel Lauss wrote:
On Mon, Jun 8, 2009 at 11:25 AM, Mark Brownbroonie@opensource.wolfsonmicro.com wrote:
If this is going to go in during the merge window I'm happy for it to go in via the MIPS tree but otherwise I'd much rather it goes via ASoC in case API changes cause merge issues. I don't know what you prefer, Ralf?
I'd rather have it all go through the MIPS tree; this is the only patch of 7 which touches files outside it, and it depends on another one to apply cleanly.
Well, like I say if it's going via MIPS I'd really prefer it to go in this merge window. If not then I'd expect that splitting out the architecture parts from the machine driver as I suggested ought to deal with the merge issues.
I'll split it in two: pure ASoC part and pure board part. Agreed?
I'd actually love to move this file to the actual board code, however due to the way ASoC is organized this isn't at all possible. This is one of two things I don't like about ASoC: the machine registration is extremely awkward compared to other platform drivers. (the other being that ASoC doesn't support multiple machines [i.e. I could actually run both AC97 and I2S simultaneously one of my boards, as independent sound cards])
I've got a board sitting on my desk here which has multiple sound systems. Unfortunately the architecture code for it is a bit of a shambles at this point so it's more trouble to work on the platform than it's worth at the minute.
Alchemy's can have up to 4 PSCs with variable base addresses and variable functions. Currently, ASoC can't handle more than 1 AC97 codec (no idea how to pass DAI private data to the ac97 callbacks), and I also don't see how to handle for instance 2 I2S machines with a WM8731 attached to each (i.e. how do I tell ASoC that wm8731 at bus0/0x1b belongs to machine A and wm8731 at bus0/0x1c belongs to machine B?)
This isn't a problem with the DB1200, as AC97 and I2S are connected to the same PSC, but the DB1550 and DB1300 do have independent AC97 and I2S.
+static struct resource psc1_res[] = {
If you conver the I2S driver to use the standard device probing model this could all me moved into the architecture code rather than placed in machine drivers.
Again, I'd love to, but can't: the AC97/I2S/DBDMA drivers extract base address and DMA information from the platform device resource structure; however I can't just copy the resource info from the this db1200_sound platform_driver to the soc_audio platform driver because the driver core complains about resource conflicts (two platform_devices sharing the same resources). Unless I missed a flag which needs to be passed to the resource.flags member?
If you move the selection of the switch position to the architecture code then it can arrange to register only the device that is in use in the current configuration. If the DMA and DAI drivers both need the same resources they can cooperate with each other - the system will only bring the card on-line once both the DMA and DAI driver are present.
I think you misuderstood me. Could you point out an in-kernel machine which already implements what you suggested?
The AC97/I2S dai drivers (psc-ac97/psc-i2s) extract the base address of the PSC they're supposed to drive from the platform_device passed via the probe() callbacks, these in turn are called when a "soc_audio" platform device is called. I need to set either the ac97 or I2S platform data for soc_audio based on the switch setting. I can't register a "db1200_audio" platform device in the board code which in turn registers the "soc_audio" device and have them share the PSC mmio/irq/dma resources.
Manuel Lauss
On Mon, Jun 08, 2009 at 01:25:57PM +0200, Manuel Lauss wrote:
On Mon, Jun 8, 2009 at 12:20 PM, Mark Brownbroonie@opensource.wolfsonmicro.com wrote:
Well, like I say if it's going via MIPS I'd really prefer it to go in this merge window. If not then I'd expect that splitting out the architecture parts from the machine driver as I suggested ought to deal with the merge issues.
I'll split it in two: pure ASoC part and pure board part. Agreed?
Yes, that's fine for me.
how to pass DAI private data to the ac97 callbacks), and I also don't see how to handle for instance 2 I2S machines with a WM8731 attached to each (i.e. how do I tell ASoC that wm8731 at bus0/0x1b belongs to machine A and wm8731 at bus0/0x1c belongs to machine B?)
When multiple cards are supported the struct device for the CODEC will be used to distinguish between instances of the same device - this is why the DAI registration functions are being encouraged to use to provide a struct device, once the multi-card support is in place it will become much more important to have this information.
If you move the selection of the switch position to the architecture code then it can arrange to register only the device that is in use in the current configuration. If the DMA and DAI drivers both need the same resources they can cooperate with each other - the system will only bring the card on-line once both the DMA and DAI driver are present.
I think you misuderstood me. Could you point out an in-kernel machine which already implements what you suggested? The AC97/I2S dai drivers (psc-ac97/psc-i2s) extract the base address of the PSC they're supposed to drive from the platform_device passed via the probe() callbacks, these in turn are called when a "soc_audio" platform device is called.
Sure, that's exactly what I see you doing and what I'm suggesting that you change.
I need to set either the ac97 or I2S platform data for soc_audio based on the switch setting. I can't register a "db1200_audio" platform device in the board code which in turn registers the "soc_audio" device and have them share the PSC mmio/irq/dma resources.
You should convert the DAI drivers to probe as normal platform devices and attach the resources used by the CPU to those devices rather than attaching the data to soc-audio. pxa2xx-ac97 does this, as do the PowerPC drivers and the s3c64xx-i2s driver. The DaVinci drivers currently on the davinci branch of my git for merge after the merge window do this too.
Mark,
You should convert the DAI drivers to probe as normal platform devices and attach the resources used by the CPU to those devices rather than attaching the data to soc-audio. pxa2xx-ac97 does this, as do the PowerPC drivers and the s3c64xx-i2s driver. The DaVinci drivers currently on the davinci branch of my git for merge after the merge window do this too.
I see now what you mean, but this is ugly as sin: I now need to register 2 platform devices in the board code: 1 for the DAI (with resources mmio + irq) and 1 for the DMA engine (with ddma id resources), or register the DMA engine device from within the AC97/I2S drivers.
This is in my opinion even worse than the current scheme, which at least allows me to group all PSC resources into one struct resource which all audio-related drivers can share without too much uglyness.
Manuel Lauss
On Mon, Jun 08, 2009 at 02:21:18PM +0200, Manuel Lauss wrote:
I see now what you mean, but this is ugly as sin: I now need to register 2 platform devices in the board code: 1 for the DAI (with resources mmio + irq) and 1 for the DMA engine (with ddma id resources), or register the DMA engine device from within the AC97/I2S drivers.
This is in my opinion even worse than the current scheme, which at least allows me to group all PSC resources into one struct resource which all audio-related drivers can share without too much uglyness.
If you trigger registration the DMA engine from within the I2S and AC97 devices you can still group everything together like you want to so I don't really see the problem - they're peering at a different device for the data but other than that things are unchanged. At the minute you're loosing a lot of sharing by having this in the individual machine drivers.
Note that one of the possibilities once multiple sound card support is introduced is that DAIs could be shared between multiple cards. This is only going to be at all useful for CPU DAIs which allow separate configuration of the RX and TX paths and will never be the common case but it's something to bear in mind.
Hi Mark,
I see now what you mean, but this is ugly as sin: I now need to register 2 platform devices in the board code: 1 for the DAI (with resources mmio + irq) and 1 for the DMA engine (with ddma id resources), or register the DMA engine device from within the AC97/I2S drivers.
This is in my opinion even worse than the current scheme, which at least allows me to group all PSC resources into one struct resource which all audio-related drivers can share without too much uglyness.
If you trigger registration the DMA engine from within the I2S and AC97 devices you can still group everything together like you want to so I don't really see the problem - they're peering at a different device for the data but other than that things are unchanged. At the minute you're loosing a lot of sharing by having this in the individual machine drivers.
All 3 drivers are now platform_devices -- and now I still have the same problem as before: I can't really share the whole struct resource; i need to allocate a new resource struct, fill in the dma ids and register the dma device with it.
Btw, the davinci-evm in asoc-git also registers mmio and dma from within the machine code.
I can't shake the feeling that there's something wrong with the whole asoc device model (and that asoc was designed with pxa2xx devices in mind which have single audio units with fixed resources that the driver code can hardcode inside it).
Note that one of the possibilities once multiple sound card support is introduced is that DAIs could be shared between multiple cards. This is
I wrote the au1x audio drivers with this in mind: the DAI really describes how to interact with a PSC; I didn't see any point in duplicating the dai description for each possible PSC.
To put an end to this thread: I really don't want to change the machine code or dai drivers at this time, because I have the feeling that it's a few steps backwards. If you don't agree with this then I'll leave the audio parts out of the submission; I don't really care if it goes in or not, I just saw it as a nice sample machine driver for both AC97 and I2S on Alchemy.
Manuel Lauss
On Mon, Jun 08, 2009 at 03:11:39PM +0200, Manuel Lauss wrote:
All 3 drivers are now platform_devices -- and now I still have the same problem as before: I can't really share the whole struct resource; i need to allocate a new resource struct, fill in the dma ids and register the dma device with it.
I don't understand the problem you're seeing here? I don't see any fundamental difference between the before and after pictures in terms of what you're saying. Note that there is no requirement that the DMA driver be a separate device - it can be if you want but if you want to make it part of the DAI driver that works too. As I say, the DAI and DMA drivers can peer into each other's implementations as much as they like. They're split up since on most CPUs the same DMA back end is used by all the DAIs the CPU supports so there's normally some opportunity for code sharing but they're never truly independent of each other.
It's not terribly clear from what you're saying but it *sounds* like for your platform the DMA driver should be essentially a library for the DAI driver, registered by the DAI driver when it probes. Otherwise could you please go into a bit more detail about what you mean when you say you need to "allocate a new resource struct..." and so on - when do you need to allocate the resource struct and why do you need to do this?
In the overwhelming majority of systems I've seen there are no resources associated with the actual machine driver itself - all the resources are for the DMA and DAI.
Btw, the davinci-evm in asoc-git also registers mmio and dma from within the machine code.
Not in the davinci branch I pointed you at, there the drivers have been converted to use the standard device model for this. The old DaVinci code you were looking at predates the ability to register things separately.
I can't shake the feeling that there's something wrong with the whole asoc device model (and that asoc was designed with pxa2xx devices in mind which have single audio units with fixed resources that the driver code can hardcode inside it).
Could you articulate what your concerns are here in more detail?
Previously I've heard a lot of people complaining (with reason) about the fact that it didn't use the standard device model at all, especially for the CODECs, but this is the first time I've ever heard any concerns about the standard device model. The problems you're mentioning with hard coding things are problems which are for most people fixed by using the standard device model and passing the resources for the DAIs up from the architecture code using the standard mechanisms since it makes it trivial to add or move new instances of the same IP block.
Doing this by putting everything into the machine drivers gets a bit tricky once you have multiple DAIs in play on a single card since it gets more error prone to tie the resources to the DAIs.
To put an end to this thread: I really don't want to change the machine code or dai drivers at this time, because I have the feeling that it's a few steps backwards. If you don't agree with this then I'll leave the audio parts out of
It'd be really good to try to understand and address your concerns now before we've been through the transition for more machines - the longer it's left the harder it will be to change. The drivers don't have to change now, I just want to understand what your concerns are.
the submission; I don't really care if it goes in or not, I just saw it as a nice sample machine driver for both AC97 and I2S on Alchemy.
Please submit anyway but at some point someone is going to have to convert the driver - I'm intending to leave as long a transition period as I can but at some point it's going to block other enhancements.
[Removed linux-mips and Ralf from CC]
On Mon, Jun 8, 2009 at 3:45 PM, Mark Brownbroonie@opensource.wolfsonmicro.com wrote:
On Mon, Jun 08, 2009 at 03:11:39PM +0200, Manuel Lauss wrote:
All 3 drivers are now platform_devices -- and now I still have the same problem as before: I can't really share the whole struct resource; i need to allocate a new resource struct, fill in the dma ids and register the dma device with it.
It's not terribly clear from what you're saying but it *sounds* like for your platform the DMA driver should be essentially a library for the DAI driver, registered by the DAI driver when it probes. Otherwise could you please go into a bit more detail about what you mean when you say you need to "allocate a new resource struct..." and so on - when do you need to allocate the resource struct and why do you need to do this?
You are right in this case. It's time for me to reevaluate the architecture of the au1x asoc drivers.
I can't shake the feeling that there's something wrong with the whole asoc device model (and that asoc was designed with pxa2xx devices in mind which have single audio units with fixed resources that the driver code can hardcode inside it).
Could you articulate what your concerns are here in more detail?
What annoys me most is the need to actually have the machine code file in the first place. Or more precisely, the location of it. It should be located where the rest of the board code lives.
The other thing is the "soc_audio" platform device itself. Instead of reg- istering a platform device for soc audio, how about a function call to setup an asoc device. Multiple calls create multiple alsa devices, the argument describes the audio fabric.
Please submit anyway but at some point someone is going to have to convert the driver - I'm intending to leave as long a transition period as I can but at some point it's going to block other enhancements.
I attached an untested patch (an addon to the patch which started this thread) which does this. Please have a look and tell me if this is what suggested initially?
I'll run-test this on real hardware as soon as I get access to it again.
Thank you! Manuel Lauss
On Mon, Jun 08, 2009 at 04:48:23PM +0200, Manuel Lauss wrote:
What annoys me most is the need to actually have the machine code file in the first place. Or more precisely, the location of it. It should be located where the rest of the board code lives.
Like I say, review and API changes are the main factors here at the minute. You will also always get some machines that are substantial enough to warrant "real" drivers too due to having controls plus complex and configurable clocking and routing (few of these make it mainline ATM).
The other thing is the "soc_audio" platform device itself. Instead of reg- istering a platform device for soc audio, how about a function call to setup an asoc device. Multiple calls create multiple alsa devices, the argument describes the audio fabric.
This is waiting for the multiple card support - if you take a look at how the soc-audio device is implemented at the minute you'll see it calls a function snd_soc_register_card() that's currently not exported. Once multi-card support is there the interface to it will be changed a bit and it will be exposed directly to machine drivers and the soc-audio device will go away, though I'll leave it there for a while in order to provide a transition period for machine drivers.
At the minute you can probably create multiple soc-audio devices and it's likely to work in some configurations but not supported.
Please submit anyway but at some point someone is going to have to convert the driver - I'm intending to leave as long a transition period as I can but at some point it's going to block other enhancements.
I attached an untested patch (an addon to the patch which started this thread) which does this. Please have a look and tell me if this is what suggested initially?
Pretty much, though I suspect there's some confusion between the probe functions going on there (there's far too many of them in ASoC at the minute - I intend to streamline things a bit in the future but first I want to get the new models implemented).
I'd expect it's possible to have standard platform devices declared in the CPU definitions for each PSC that the individual boards can just reference (rather than having to copy out the resources, which presumably are fixed for each CPU) but I'm not sure if that's idiomatic for MIPS code or not. Most of ARM works that way.
I'd be tempted to not bother creating the separate device for the DMA driver in au1xpsc_pcm_add() but that's just my personal preference.
Also, you've missed a name for your audio_dev.
Hi Mark,
On Mon, Jun 8, 2009 at 5:24 PM, Mark Brownbroonie@opensource.wolfsonmicro.com wrote:
I attached an untested patch (an addon to the patch which started this thread) which does this. Please have a look and tell me if this is what suggested initially?
Pretty much, though I suspect there's some confusion between the probe functions going on there (there's far too many of them in ASoC at the minute - I intend to streamline things a bit in the future but first I want to get the new models implemented).
I'd expect it's possible to have standard platform devices declared in the CPU definitions for each PSC that the individual boards can just reference (rather than having to copy out the resources, which presumably are fixed for each CPU) but I'm not sure if that's idiomatic for MIPS code or not. Most of ARM works that way.
I'd be tempted to not bother creating the separate device for the DMA driver in au1xpsc_pcm_add() but that's just my personal preference.
This patch was just a quick hack, I'll clean it up and develop it further in the coming days, then submit it to you after some testing.
Also, you've missed a name for your audio_dev.
Nope, it's set in the following hunk depending on switch setting.
Thanks a lot for your patience! Manuel Lauss
participants (2)
-
Manuel Lauss -
Mark Brown