[alsa-devel] ASoC: UDA1380/I2S/SSP patches
Right now the snd-soc-magician driver in ASoC dev is severely bitrotted. I can make it compile and load again, but until now it only outputs silence (or pops and hisses as I fiddle with the uda1380 registers). For now, I'd like feedback on these patches in preparation for the magician update and switch from pxa2xx-ssp to pxa-ssp.
regards Philipp
This patch splits set_dai_fmt into three variants (single interface, dual interface playback only, dual interface capture only) so that data input and output formats can be configured separately for dual interface setups.
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com --- sound/soc/codecs/uda1380.c | 68 +++++++++++++++++++++++++++++++++++++++---- 1 files changed, 61 insertions(+), 7 deletions(-)
diff --git a/sound/soc/codecs/uda1380.c b/sound/soc/codecs/uda1380.c index a957b43..6de37b2 100644 --- a/sound/soc/codecs/uda1380.c +++ b/sound/soc/codecs/uda1380.c @@ -356,7 +356,7 @@ static int uda1380_add_widgets(struct snd_soc_codec *codec) return 0; }
-static int uda1380_set_dai_fmt(struct snd_soc_dai *codec_dai, +static int uda1380_set_dai_fmt_both(struct snd_soc_dai *codec_dai, unsigned int fmt) { struct snd_soc_codec *codec = codec_dai->codec; @@ -366,16 +366,70 @@ static int uda1380_set_dai_fmt(struct snd_soc_dai *codec_dai, iface = uda1380_read_reg_cache(codec, UDA1380_IFACE); iface &= ~(R01_SFORI_MASK | R01_SIM | R01_SFORO_MASK);
- /* FIXME: how to select I2S for DATAO and MSB for DATAI correctly? */ switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { case SND_SOC_DAIFMT_I2S: iface |= R01_SFORI_I2S | R01_SFORO_I2S; break; case SND_SOC_DAIFMT_LSB: - iface |= R01_SFORI_LSB16 | R01_SFORO_I2S; + iface |= R01_SFORI_LSB16 | R01_SFORO_LSB16; break; case SND_SOC_DAIFMT_MSB: - iface |= R01_SFORI_MSB | R01_SFORO_I2S; + iface |= R01_SFORI_MSB | R01_SFORO_MSB; + } + + if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) == SND_SOC_DAIFMT_CBM_CFM) + iface |= R01_SIM; + + uda1380_write(codec, UDA1380_IFACE, iface); + + return 0; +} + +static int uda1380_set_dai_fmt_playback(struct snd_soc_dai *codec_dai, + unsigned int fmt) +{ + struct snd_soc_codec *codec = codec_dai->codec; + int iface; + + /* set up DAI based upon fmt */ + iface = uda1380_read_reg_cache(codec, UDA1380_IFACE); + iface &= ~R01_SFORI_MASK; + + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { + case SND_SOC_DAIFMT_I2S: + iface |= R01_SFORI_I2S; + break; + case SND_SOC_DAIFMT_LSB: + iface |= R01_SFORI_LSB16; + break; + case SND_SOC_DAIFMT_MSB: + iface |= R01_SFORI_MSB; + } + + uda1380_write(codec, UDA1380_IFACE, iface); + + return 0; +} + +static int uda1380_set_dai_fmt_capture(struct snd_soc_dai *codec_dai, + unsigned int fmt) +{ + struct snd_soc_codec *codec = codec_dai->codec; + int iface; + + /* set up DAI based upon fmt */ + iface = uda1380_read_reg_cache(codec, UDA1380_IFACE); + iface &= ~(R01_SIM | R01_SFORO_MASK); + + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { + case SND_SOC_DAIFMT_I2S: + iface |= R01_SFORO_I2S; + break; + case SND_SOC_DAIFMT_LSB: + iface |= R01_SFORO_LSB16; + break; + case SND_SOC_DAIFMT_MSB: + iface |= R01_SFORO_MSB; }
if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) == SND_SOC_DAIFMT_CBM_CFM) @@ -549,7 +603,7 @@ struct snd_soc_dai uda1380_dai[] = { .shutdown = uda1380_pcm_shutdown, .prepare = uda1380_pcm_prepare, .digital_mute = uda1380_mute, - .set_fmt = uda1380_set_dai_fmt, + .set_fmt = uda1380_set_dai_fmt_both, }, }, { /* playback only - dual interface */ @@ -566,7 +620,7 @@ struct snd_soc_dai uda1380_dai[] = { .shutdown = uda1380_pcm_shutdown, .prepare = uda1380_pcm_prepare, .digital_mute = uda1380_mute, - .set_fmt = uda1380_set_dai_fmt, + .set_fmt = uda1380_set_dai_fmt_playback, }, }, { /* capture only - dual interface*/ @@ -582,7 +636,7 @@ struct snd_soc_dai uda1380_dai[] = { .hw_params = uda1380_pcm_hw_params, .shutdown = uda1380_pcm_shutdown, .prepare = uda1380_pcm_prepare, - .set_fmt = uda1380_set_dai_fmt, + .set_fmt = uda1380_set_dai_fmt_capture, }, }, };
On Tue, Feb 03, 2009 at 05:19:40PM +0100, Philipp Zabel wrote:
This patch splits set_dai_fmt into three variants (single interface, dual interface playback only, dual interface capture only) so that data input and output formats can be configured separately for dual interface setups.
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com
Is there any reason not to apply this immediately?
On Tue, Feb 3, 2009 at 5:41 PM, Mark Brown broonie@sirena.org.uk wrote:
On Tue, Feb 03, 2009 at 05:19:40PM +0100, Philipp Zabel wrote:
This patch splits set_dai_fmt into three variants (single interface, dual interface playback only, dual interface capture only) so that data input and output formats can be configured separately for dual interface setups.
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com
Is there any reason not to apply this immediately?
Not that I am aware of. Maybe Vasily can test this patch first. (I can only test the dual interface case.)
regards Philipp
On Tuesday 03 February 2009 18:45:10 pHilipp Zabel wrote:
Not that I am aware of. Maybe Vasily can test this patch first. (I can only test the dual interface case.)
regards Philipp
This patch breaks nothing for me, uda1380_set_dai_fmt_both was used in my configuration.
Regards Vasily
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com --- sound/soc/pxa/pxa-ssp.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c index 73cb6b4..5ee544b 100644 --- a/sound/soc/pxa/pxa-ssp.c +++ b/sound/soc/pxa/pxa-ssp.c @@ -785,7 +785,7 @@ static void pxa_ssp_remove(struct platform_device *pdev, struct snd_soc_dai pxa_ssp_dai[] = { { .name = "pxa2xx-ssp1", - .id = 0, + .id = 1, .probe = pxa_ssp_probe, .remove = pxa_ssp_remove, .suspend = pxa_ssp_suspend, @@ -816,7 +816,7 @@ struct snd_soc_dai pxa_ssp_dai[] = { }, }, { .name = "pxa2xx-ssp2", - .id = 1, + .id = 2, .probe = pxa_ssp_probe, .remove = pxa_ssp_remove, .suspend = pxa_ssp_suspend, @@ -848,7 +848,7 @@ struct snd_soc_dai pxa_ssp_dai[] = { }, { .name = "pxa2xx-ssp3", - .id = 2, + .id = 3, .probe = pxa_ssp_probe, .remove = pxa_ssp_remove, .suspend = pxa_ssp_suspend, @@ -880,7 +880,7 @@ struct snd_soc_dai pxa_ssp_dai[] = { }, { .name = "pxa2xx-ssp4", - .id = 3, + .id = 4, .probe = pxa_ssp_probe, .remove = pxa_ssp_remove, .suspend = pxa_ssp_suspend,
On Tue, Feb 03, 2009 at 05:19:41PM +0100, Philipp Zabel wrote:
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com
Meh, all the display names use the documentation names and the id is used for indexing into arrays (which may well be why you're not getting any audio out...). I'd be more inclined to leave this as-is but if not it needs to adjust all the array usages.
On Tue, Feb 3, 2009 at 5:47 PM, Mark Brown broonie@sirena.org.uk wrote:
On Tue, Feb 03, 2009 at 05:19:41PM +0100, Philipp Zabel wrote:
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com
Meh, all the display names use the documentation names and the id is used for indexing into arrays (which may well be why you're not getting any audio out...). I'd be more inclined to leave this as-is but if not it needs to adjust all the array usages.
Meh, indeed. Jumping between pxa2xx-ssp and pxa-ssp must have confused me. A lot. The only thing wrong with pxa-ssp is that it requests the wrong SSP port, then:
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c index 73cb6b4..7f7c23e 100644 --- a/sound/soc/pxa/pxa-ssp.c +++ b/sound/soc/pxa/pxa-ssp.c @@ -751,7 +751,7 @@ static int pxa_ssp_probe(struct platform_device *pdev, if (!priv) return -ENOMEM;
- priv->dev.ssp = ssp_request(dai->id, "SoC audio"); + priv->dev.ssp = ssp_request(dai->id + 1, "SoC audio"); if (priv->dev.ssp == NULL) { ret = -ENODEV; goto err_priv;
But this isn't right either. Because if we request the correct SSP port here, the subsequent request of the same SSP port in ssp_init (called from ops->startup) fails. How to fix?
regards Philipp
On Tue, Feb 03, 2009 at 06:13:22PM +0100, pHilipp Zabel wrote:
priv->dev.ssp = ssp_request(dai->id, "SoC audio");
priv->dev.ssp = ssp_request(dai->id + 1, "SoC audio"); if (priv->dev.ssp == NULL) { ret = -ENODEV; goto err_priv;
But this isn't right either. Because if we request the correct SSP port here, the subsequent request of the same SSP port in ssp_init (called from ops->startup) fails. How to fix?
Hrm. ssp_init() does a ssp_request() already. The request on probe is just redundant. Eric was muttering about refactoring the API some more but never got round to it.
On Tue, Feb 3, 2009 at 6:35 PM, Mark Brown broonie@sirena.org.uk wrote:
On Tue, Feb 03, 2009 at 06:13:22PM +0100, pHilipp Zabel wrote:
priv->dev.ssp = ssp_request(dai->id, "SoC audio");
priv->dev.ssp = ssp_request(dai->id + 1, "SoC audio"); if (priv->dev.ssp == NULL) { ret = -ENODEV; goto err_priv;But this isn't right either. Because if we request the correct SSP port here, the subsequent request of the same SSP port in ssp_init (called from ops->startup) fails. How to fix?
Hrm. ssp_init() does a ssp_request() already. The request on probe is just redundant. Eric was muttering about refactoring the API some more but never got round to it.
Hm, hm.. Ripping ssp_init/exit's innards out of them (minus the ssp/irq_request parts), like this, seems to work:
--------------------------- sound/soc/pxa/pxa-ssp.c --------------------------- index 1687e18..c56a07c 100644 @@ -21,6 +21,8 @@ #include <linux/clk.h> #include <linux/io.h>
+#include <asm/irq.h> + #include <sound/core.h> #include <sound/pcm.h> #include <sound/initval.h> @@ -221,9 +223,9 @@ static int pxa_ssp_startup(struct snd_pcm_substream *substream, int ret = 0;
if (!cpu_dai->active) { - ret = ssp_init(&priv->dev, cpu_dai->id + 1, SSP_NO_IRQ); - if (ret < 0) - return ret; + priv->dev.port = cpu_dai->id + 1; + priv->dev.irq = NO_IRQ; + clk_enable(priv->dev.ssp->clk); ssp_disable(&priv->dev); } return ret; @@ -238,7 +240,7 @@ static void pxa_ssp_shutdown(struct snd_pcm_substream *substream,
if (!cpu_dai->active) { ssp_disable(&priv->dev); - ssp_exit(&priv->dev); + clk_disable(priv->dev.ssp->clk); } }
On Tue, Feb 03, 2009 at 07:49:43PM +0100, pHilipp Zabel wrote:
Hm, hm.. Ripping ssp_init/exit's innards out of them (minus the ssp/irq_request parts), like this, seems to work:
Yes, that looks good. IIRC that was the direction the SSP API was supposed to be heading in anyway. Please submit properly (with changelog and so on) - I'll test tomorrow with PXA3xx but I'd be surprised if there were any issues.
This should be done during board init via pxa2xx_mfp_config instead.
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com --- sound/soc/pxa/pxa2xx-i2s.c | 36 ------------------------------------ 1 files changed, 0 insertions(+), 36 deletions(-)
diff --git a/sound/soc/pxa/pxa2xx-i2s.c b/sound/soc/pxa/pxa2xx-i2s.c index 517991f..83b59d7 100644 --- a/sound/soc/pxa/pxa2xx-i2s.c +++ b/sound/soc/pxa/pxa2xx-i2s.c @@ -25,20 +25,11 @@
#include <mach/hardware.h> #include <mach/pxa-regs.h> -#include <mach/pxa2xx-gpio.h> #include <mach/audio.h>
#include "pxa2xx-pcm.h" #include "pxa2xx-i2s.h"
-struct pxa2xx_gpio { - u32 sys; - u32 rx; - u32 tx; - u32 clk; - u32 frm; -}; - /* * I2S Controller Register and Bit Definitions */ @@ -106,21 +97,6 @@ static struct pxa2xx_pcm_dma_params pxa2xx_i2s_pcm_stereo_in = { DCMD_BURST32 | DCMD_WIDTH4, };
-static struct pxa2xx_gpio gpio_bus[] = { - { /* I2S SoC Slave */ - .rx = GPIO29_SDATA_IN_I2S_MD, - .tx = GPIO30_SDATA_OUT_I2S_MD, - .clk = GPIO28_BITCLK_IN_I2S_MD, - .frm = GPIO31_SYNC_I2S_MD, - }, - { /* I2S SoC Master */ - .rx = GPIO29_SDATA_IN_I2S_MD, - .tx = GPIO30_SDATA_OUT_I2S_MD, - .clk = GPIO28_BITCLK_OUT_I2S_MD, - .frm = GPIO31_SYNC_I2S_MD, - }, -}; - static int pxa2xx_i2s_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { @@ -181,9 +157,6 @@ static int pxa2xx_i2s_set_dai_sysclk(struct snd_soc_dai *cpu_dai, if (clk_id != PXA2XX_I2S_SYSCLK) return -ENODEV;
- if (pxa_i2s.master && dir == SND_SOC_CLOCK_OUT) - pxa_gpio_mode(gpio_bus[pxa_i2s.master].sys); - return 0; }
@@ -194,10 +167,6 @@ static int pxa2xx_i2s_hw_params(struct snd_pcm_substream *substream, struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_dai *cpu_dai = rtd->dai->cpu_dai;
- pxa_gpio_mode(gpio_bus[pxa_i2s.master].rx); - pxa_gpio_mode(gpio_bus[pxa_i2s.master].tx); - pxa_gpio_mode(gpio_bus[pxa_i2s.master].frm); - pxa_gpio_mode(gpio_bus[pxa_i2s.master].clk); BUG_ON(IS_ERR(clk_i2s)); clk_enable(clk_i2s); pxa_i2s_wait(); @@ -398,11 +367,6 @@ static struct platform_driver pxa2xx_i2s_driver = {
static int __init pxa2xx_i2s_init(void) { - if (cpu_is_pxa27x()) - gpio_bus[1].sys = GPIO113_I2S_SYSCLK_MD; - else - gpio_bus[1].sys = GPIO32_SYSCLK_I2S_MD; - clk_i2s = ERR_PTR(-ENOENT); return platform_driver_register(&pxa2xx_i2s_driver); }
On Tue, Feb 03, 2009 at 05:19:42PM +0100, Philipp Zabel wrote:
This should be done during board init via pxa2xx_mfp_config instead.
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com
It should, but you need to update all the affected boards too.
On Tue, Feb 3, 2009 at 5:32 PM, Mark Brown broonie@sirena.org.uk wrote:
On Tue, Feb 03, 2009 at 05:19:42PM +0100, Philipp Zabel wrote:
This should be done during board init via pxa2xx_mfp_config instead.
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com
It should, but you need to update all the affected boards too.
The affected boards are Corgi, Spitz, Poodle, Magician, AMESOM, H5000, Mainstone. Of those only Spitz, H5000 and Mainstone are missing I2S setup in arch/arm/mach-pxa. I'll try to get those patches into the PXA tree and then resend this one.
My actual problem with the current code is that pxa2xx-i2s sets up GPIO30 as I2S SDATA_OUT while on magician that GPIO is connected to the battery charger.
regards Philipp
On Tue, Feb 03, 2009 at 05:56:24PM +0100, pHilipp Zabel wrote:
The affected boards are Corgi, Spitz, Poodle, Magician, AMESOM, H5000, Mainstone. Of those only Spitz, H5000 and Mainstone are missing I2S setup in arch/arm/mach-pxa. I'll try to get those patches into the PXA tree and then resend this one.
Feel free to merge this as part of the same series/patch. I'm fine with the change, we just need to make sure the machines that need to do the setup are doing it. There's not much likelyhood of conflicts with this so either tree is fine for merging.
snd-soc-pxa-ssp depends on PXA_SSP being enabled. Let SND_PXA_SOC_SSP select it, like SPI_PXA2XX does.
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com --- sound/soc/pxa/Kconfig | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/sound/soc/pxa/Kconfig b/sound/soc/pxa/Kconfig index 5fb9513..8627bf6 100644 --- a/sound/soc/pxa/Kconfig +++ b/sound/soc/pxa/Kconfig @@ -23,6 +23,7 @@ config SND_PXA2XX_SOC_SSP
config SND_PXA_SOC_SSP tristate + select PXA_SSP
config SND_PXA2XX_SOC_CORGI tristate "SoC Audio support for Sharp Zaurus SL-C7x0"
On Tue, Feb 03, 2009 at 05:19:43PM +0100, Philipp Zabel wrote:
snd-soc-pxa-ssp depends on PXA_SSP being enabled. Let SND_PXA_SOC_SSP select it, like SPI_PXA2XX does.
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com
Unfortunately Kconfig ignores selects from things that are themselves selected so this won't do anything.
On Tue, Feb 3, 2009 at 5:35 PM, Mark Brown broonie@sirena.org.uk wrote:
On Tue, Feb 03, 2009 at 05:19:43PM +0100, Philipp Zabel wrote:
snd-soc-pxa-ssp depends on PXA_SSP being enabled. Let SND_PXA_SOC_SSP select it, like SPI_PXA2XX does.
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com
Unfortunately Kconfig ignores selects from things that are themselves selected so this won't do anything.
Ok. I guess I'll just select it from MACH_MAGICIAN then.
regards Philipp
participants (3)
-
Mark Brown -
Philipp Zabel -
Vasily Khoruzhick