[alsa-devel] [PATCH v3 00/11] ASoC: add imx-sgtl5000 machine driver working with fsl_ssi
I believe I have addressed all the outstanding comments and requests reviewers put on previous versions. So, hopefully this will be the last iteration of the series.
The current sound/for-next branch seems broken for me, and I have not tracking the problem down. But the series works good on SHA below.
3030763 (Merge tag 'asoc-3.4' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound into topic/asoc)
Timur,
I did not pick up you Ack-by tag, because there are some changes since v2 you will need to take another look and have the testing, I guess.
Regards, Shawn
Shawn Guo (11): ASoC: core: missing set_fmt should not be complaint ASoC: fsl: separate SSI and DMA Kconfig options ASoC: imx: merge sound/soc/imx into sound/soc/fsl ASoC: fsl: rename imx-pcm Kconfig options and filename ASoC: fsl: create fsl_utils to accommodate the common functions ASoC: fsl: remove helper fsl_asoc_get_codec_dev_name ASoC: fsl: check property 'compatible' for the machine name ASoC: fsl: make fsl_ssi driver compilable on ARM/IMX ASoC: fsl: remove the fatal error checking on codec-handle ASoC: fsl: let fsl_ssi work with imx pcm and machine drivers ASoC: fsl: add imx-sgtl5000 machine driver
.../bindings/sound/imx-audio-sgtl5000.txt | 24 +++ arch/powerpc/configs/86xx/mpc8610_hpcd_defconfig | 1 + arch/powerpc/configs/mpc85xx_defconfig | 1 + arch/powerpc/configs/mpc85xx_smp_defconfig | 1 + sound/soc/Kconfig | 1 - sound/soc/Makefile | 1 - sound/soc/fsl/Kconfig | 124 ++++++++++++- sound/soc/fsl/Makefile | 29 +++- sound/soc/{imx => fsl}/eukrea-tlv320.c | 2 +- sound/soc/fsl/fsl_ssi.c | 146 ++++++++++++---- sound/soc/fsl/fsl_utils.c | 91 ++++++++++ sound/soc/fsl/fsl_utils.h | 26 +++ sound/soc/{imx => fsl}/imx-audmux.c | 0 sound/soc/{imx => fsl}/imx-audmux.h | 0 .../{imx/imx-pcm-dma-mx2.c => fsl/imx-pcm-dma.c} | 0 sound/soc/{imx => fsl}/imx-pcm-fiq.c | 0 sound/soc/{imx => fsl}/imx-pcm.c | 0 sound/soc/{imx => fsl}/imx-pcm.h | 0 sound/soc/fsl/imx-sgtl5000.c | 177 ++++++++++++++++++ sound/soc/{imx => fsl}/imx-ssi.c | 2 +- sound/soc/{imx => fsl}/imx-ssi.h | 0 sound/soc/fsl/mpc8610_hpcd.c | 168 ++---------------- sound/soc/{imx => fsl}/mx27vis-aic32x4.c | 0 sound/soc/fsl/p1022_ds.c | 190 ++------------------ sound/soc/{imx => fsl}/phycore-ac97.c | 0 sound/soc/{imx => fsl}/wm1133-ev1.c | 0 sound/soc/imx/Kconfig | 79 -------- sound/soc/imx/Makefile | 22 --- sound/soc/soc-core.c | 11 +- 29 files changed, 616 insertions(+), 480 deletions(-)
Not having a DAI link set_fmt operation is perfectly normal and should not be complaint.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/soc-core.c | 11 ++++++----- 1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 7978f6c..98b2635 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1531,14 +1531,14 @@ static void snd_soc_instantiate_card(struct snd_soc_card *card) if (dai_link->dai_fmt) { ret = snd_soc_dai_set_fmt(card->rtd[i].codec_dai, dai_link->dai_fmt); - if (ret != 0) + if (ret != 0 && ret != -ENOTSUPP) dev_warn(card->rtd[i].codec_dai->dev, "Failed to set DAI format: %d\n", ret);
ret = snd_soc_dai_set_fmt(card->rtd[i].cpu_dai, dai_link->dai_fmt); - if (ret != 0) + if (ret != 0 && ret != -ENOTSUPP) dev_warn(card->rtd[i].cpu_dai->dev, "Failed to set DAI format: %d\n", ret); @@ -2971,10 +2971,11 @@ EXPORT_SYMBOL_GPL(snd_soc_codec_set_pll); */ int snd_soc_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) { - if (dai->driver && dai->driver->ops->set_fmt) - return dai->driver->ops->set_fmt(dai, fmt); - else + if (dai->driver == NULL) return -EINVAL; + if (dai->driver->ops->set_fmt == NULL) + return -ENOTSUPP; + return dai->driver->ops->set_fmt(dai, fmt); } EXPORT_SYMBOL_GPL(snd_soc_dai_set_fmt);
On Fri, Mar 09, 2012 at 12:59:40AM +0800, Shawn Guo wrote:
Not having a DAI link set_fmt operation is perfectly normal and should not be complaint.
Applied, thanks. I'll wait for Timur's review on the rest before applying.
The fsl_ssi driver will possibly be shared between Freescale PowerPC and ARM/IMX families, so give it a separate Kconfig option. Then fsl_ssi driver can possibly be selected independently from selecting PowerPC DMA based PCM driver.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/fsl/Kconfig | 15 +++++++++------ sound/soc/fsl/Makefile | 3 ++- 2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig index d754d34..ca693b2 100644 --- a/sound/soc/fsl/Kconfig +++ b/sound/soc/fsl/Kconfig @@ -1,10 +1,11 @@ config SND_MPC52xx_DMA tristate
-# ASoC platform support for the Freescale PowerPC SOCs that have an SSI and -# an Elo DMA controller, such as the MPC8610 and P1022. You will still need to -# select a platform driver and a codec driver. -config SND_SOC_POWERPC_SSI +config SND_SOC_FSL_SSI + tristate + depends on FSL_SOC + +config SND_SOC_POWERPC_DMA tristate depends on FSL_SOC
@@ -12,7 +13,8 @@ config SND_SOC_MPC8610_HPCD tristate "ALSA SoC support for the Freescale MPC8610 HPCD board" # I2C is necessary for the CS4270 driver depends on MPC8610_HPCD && I2C - select SND_SOC_POWERPC_SSI + select SND_SOC_FSL_SSI + select SND_SOC_POWERPC_DMA select SND_SOC_CS4270 select SND_SOC_CS4270_VD33_ERRATA default y if MPC8610_HPCD @@ -23,7 +25,8 @@ config SND_SOC_P1022_DS tristate "ALSA SoC support for the Freescale P1022 DS board" # I2C is necessary for the WM8776 driver depends on P1022_DS && I2C - select SND_SOC_POWERPC_SSI + select SND_SOC_FSL_SSI + select SND_SOC_POWERPC_DMA select SND_SOC_WM8776 default y if P1022_DS help diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile index b4a38c0..95d483f 100644 --- a/sound/soc/fsl/Makefile +++ b/sound/soc/fsl/Makefile @@ -9,7 +9,8 @@ obj-$(CONFIG_SND_SOC_P1022_DS) += snd-soc-p1022-ds.o # Freescale PowerPC SSI/DMA Platform Support snd-soc-fsl-ssi-objs := fsl_ssi.o snd-soc-fsl-dma-objs := fsl_dma.o -obj-$(CONFIG_SND_SOC_POWERPC_SSI) += snd-soc-fsl-ssi.o snd-soc-fsl-dma.o +obj-$(CONFIG_SND_SOC_FSL_SSI) += snd-soc-fsl-ssi.o +obj-$(CONFIG_SND_SOC_POWERPC_DMA) += snd-soc-fsl-dma.o
# MPC5200 Platform Support obj-$(CONFIG_SND_MPC52xx_DMA) += mpc5200_dma.o
Freescale PowerPC and ARM/IMX families share the same SSI IP block. The patch merges sound/soc/imx into sound/soc/fsl, so that the possible code sharing and consolidation can happen.
This is a plain merge, except that menuconfig SND_POWERPC_SOC is added in Kconfig for PowerPC platform as a correspondence to SND_IMX_SOC for IMX platform.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- arch/powerpc/configs/86xx/mpc8610_hpcd_defconfig | 1 + arch/powerpc/configs/mpc85xx_defconfig | 1 + arch/powerpc/configs/mpc85xx_smp_defconfig | 1 + sound/soc/Kconfig | 1 - sound/soc/Makefile | 1 - sound/soc/fsl/Kconfig | 92 ++++++++++++++++++++++ sound/soc/fsl/Makefile | 22 +++++ sound/soc/{imx => fsl}/eukrea-tlv320.c | 2 +- sound/soc/{imx => fsl}/imx-audmux.c | 0 sound/soc/{imx => fsl}/imx-audmux.h | 0 sound/soc/{imx => fsl}/imx-pcm-dma-mx2.c | 0 sound/soc/{imx => fsl}/imx-pcm-fiq.c | 0 sound/soc/{imx => fsl}/imx-pcm.c | 0 sound/soc/{imx => fsl}/imx-pcm.h | 0 sound/soc/{imx => fsl}/imx-ssi.c | 2 +- sound/soc/{imx => fsl}/imx-ssi.h | 0 sound/soc/{imx => fsl}/mx27vis-aic32x4.c | 0 sound/soc/{imx => fsl}/phycore-ac97.c | 0 sound/soc/{imx => fsl}/wm1133-ev1.c | 0 sound/soc/imx/Kconfig | 79 ------------------- sound/soc/imx/Makefile | 22 ----- 21 files changed, 119 insertions(+), 105 deletions(-) rename sound/soc/{imx => fsl}/eukrea-tlv320.c (99%) rename sound/soc/{imx => fsl}/imx-audmux.c (100%) rename sound/soc/{imx => fsl}/imx-audmux.h (100%) rename sound/soc/{imx => fsl}/imx-pcm-dma-mx2.c (100%) rename sound/soc/{imx => fsl}/imx-pcm-fiq.c (100%) rename sound/soc/{imx => fsl}/imx-pcm.c (100%) rename sound/soc/{imx => fsl}/imx-pcm.h (100%) rename sound/soc/{imx => fsl}/imx-ssi.c (99%) rename sound/soc/{imx => fsl}/imx-ssi.h (100%) rename sound/soc/{imx => fsl}/mx27vis-aic32x4.c (100%) rename sound/soc/{imx => fsl}/phycore-ac97.c (100%) rename sound/soc/{imx => fsl}/wm1133-ev1.c (100%) delete mode 100644 sound/soc/imx/Kconfig delete mode 100644 sound/soc/imx/Makefile
diff --git a/arch/powerpc/configs/86xx/mpc8610_hpcd_defconfig b/arch/powerpc/configs/86xx/mpc8610_hpcd_defconfig index 0db9ba0..c09598b 100644 --- a/arch/powerpc/configs/86xx/mpc8610_hpcd_defconfig +++ b/arch/powerpc/configs/86xx/mpc8610_hpcd_defconfig @@ -100,6 +100,7 @@ CONFIG_SND_MIXER_OSS=y CONFIG_SND_PCM_OSS=y # CONFIG_SND_SUPPORT_OLD_API is not set CONFIG_SND_SOC=y +CONFIG_SND_POWERPC_SOC=y CONFIG_RTC_CLASS=y CONFIG_RTC_DRV_CMOS=y CONFIG_EXT2_FS=y diff --git a/arch/powerpc/configs/mpc85xx_defconfig b/arch/powerpc/configs/mpc85xx_defconfig index f37a2ab..44605f8 100644 --- a/arch/powerpc/configs/mpc85xx_defconfig +++ b/arch/powerpc/configs/mpc85xx_defconfig @@ -139,6 +139,7 @@ CONFIG_SND_INTEL8X0=y # CONFIG_SND_PPC is not set # CONFIG_SND_USB is not set CONFIG_SND_SOC=y +CONFIG_SND_POWERPC_SOC=y CONFIG_HID_A4TECH=y CONFIG_HID_APPLE=y CONFIG_HID_BELKIN=y diff --git a/arch/powerpc/configs/mpc85xx_smp_defconfig b/arch/powerpc/configs/mpc85xx_smp_defconfig index abdcd31..98e227e 100644 --- a/arch/powerpc/configs/mpc85xx_smp_defconfig +++ b/arch/powerpc/configs/mpc85xx_smp_defconfig @@ -141,6 +141,7 @@ CONFIG_SND_INTEL8X0=y # CONFIG_SND_PPC is not set # CONFIG_SND_USB is not set CONFIG_SND_SOC=y +CONFIG_SND_POWERPC_SOC=y CONFIG_HID_A4TECH=y CONFIG_HID_APPLE=y CONFIG_HID_BELKIN=y diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig index 91c9855..0f85f6d 100644 --- a/sound/soc/Kconfig +++ b/sound/soc/Kconfig @@ -35,7 +35,6 @@ source "sound/soc/blackfin/Kconfig" source "sound/soc/davinci/Kconfig" source "sound/soc/ep93xx/Kconfig" source "sound/soc/fsl/Kconfig" -source "sound/soc/imx/Kconfig" source "sound/soc/jz4740/Kconfig" source "sound/soc/nuc900/Kconfig" source "sound/soc/omap/Kconfig" diff --git a/sound/soc/Makefile b/sound/soc/Makefile index 2feaf37..363dfd6 100644 --- a/sound/soc/Makefile +++ b/sound/soc/Makefile @@ -12,7 +12,6 @@ obj-$(CONFIG_SND_SOC) += blackfin/ obj-$(CONFIG_SND_SOC) += davinci/ obj-$(CONFIG_SND_SOC) += ep93xx/ obj-$(CONFIG_SND_SOC) += fsl/ -obj-$(CONFIG_SND_SOC) += imx/ obj-$(CONFIG_SND_SOC) += jz4740/ obj-$(CONFIG_SND_SOC) += mid-x86/ obj-$(CONFIG_SND_SOC) += mxs/ diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig index ca693b2..19856a0 100644 --- a/sound/soc/fsl/Kconfig +++ b/sound/soc/fsl/Kconfig @@ -1,3 +1,15 @@ +config SND_SOC_FSL_SSI + tristate + +menuconfig SND_POWERPC_SOC + tristate "SoC Audio for Freescale PowerPC CPUs" + depends on FSL_SOC + help + Say Y or M if you want to add support for codecs attached to + the PowerPC CPUs. + +if SND_POWERPC_SOC + config SND_MPC52xx_DMA tristate
@@ -68,3 +80,83 @@ config SND_MPC52xx_SOC_EFIKA help Say Y if you want to add support for sound on the Efika.
+endif # SND_POWERPC_SOC + +menuconfig SND_IMX_SOC + tristate "SoC Audio for Freescale i.MX CPUs" + depends on ARCH_MXC + help + Say Y or M if you want to add support for codecs attached to + the i.MX CPUs. + +if SND_IMX_SOC + +config SND_SOC_IMX_SSI + tristate + +config SND_SOC_IMX_PCM + tristate + +config SND_MXC_SOC_FIQ + tristate + select FIQ + select SND_SOC_IMX_PCM + +config SND_MXC_SOC_MX2 + tristate + select SND_SOC_DMAENGINE_PCM + select SND_SOC_IMX_PCM + +config SND_SOC_IMX_AUDMUX + tristate + +config SND_MXC_SOC_WM1133_EV1 + tristate "Audio on the the i.MX31ADS with WM1133-EV1 fitted" + depends on MACH_MX31ADS_WM1133_EV1 && EXPERIMENTAL + select SND_SOC_WM8350 + select SND_MXC_SOC_FIQ + select SND_SOC_IMX_AUDMUX + select SND_SOC_IMX_SSI + help + Enable support for audio on the i.MX31ADS with the WM1133-EV1 + PMIC board with WM8835x fitted. + +config SND_SOC_MX27VIS_AIC32X4 + tristate "SoC audio support for Visstrim M10 boards" + depends on MACH_IMX27_VISSTRIM_M10 && I2C + select SND_SOC_TLV320AIC32X4 + select SND_MXC_SOC_MX2 + select SND_SOC_IMX_AUDMUX + select SND_SOC_IMX_SSI + help + Say Y if you want to add support for SoC audio on Visstrim SM10 + board with TLV320AIC32X4 codec. + +config SND_SOC_PHYCORE_AC97 + tristate "SoC Audio support for Phytec phyCORE (and phyCARD) boards" + depends on MACH_PCM043 || MACH_PCA100 + select SND_SOC_AC97_BUS + select SND_SOC_WM9712 + select SND_MXC_SOC_FIQ + select SND_SOC_IMX_AUDMUX + select SND_SOC_IMX_SSI + help + Say Y if you want to add support for SoC audio on Phytec phyCORE + and phyCARD boards in AC97 mode + +config SND_SOC_EUKREA_TLV320 + tristate "Eukrea TLV320" + depends on MACH_EUKREA_MBIMX27_BASEBOARD \ + || MACH_EUKREA_MBIMXSD25_BASEBOARD \ + || MACH_EUKREA_MBIMXSD35_BASEBOARD \ + || MACH_EUKREA_MBIMXSD51_BASEBOARD + depends on I2C + select SND_SOC_TLV320AIC23 + select SND_MXC_SOC_FIQ + select SND_SOC_IMX_AUDMUX + select SND_SOC_IMX_SSI + help + Enable I2S based access to the TLV320AIC23B codec attached + to the SSI interface + +endif # SND_IMX_SOC diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile index 95d483f..36c257f 100644 --- a/sound/soc/fsl/Makefile +++ b/sound/soc/fsl/Makefile @@ -21,3 +21,25 @@ obj-$(CONFIG_SND_SOC_MPC5200_AC97) += mpc5200_psc_ac97.o obj-$(CONFIG_SND_MPC52xx_SOC_PCM030) += pcm030-audio-fabric.o obj-$(CONFIG_SND_MPC52xx_SOC_EFIKA) += efika-audio-fabric.o
+# i.MX Platform Support +snd-soc-imx-ssi-objs := imx-ssi.o +snd-soc-imx-audmux-objs := imx-audmux.o + +obj-$(CONFIG_SND_SOC_IMX_SSI) += snd-soc-imx-ssi.o +obj-$(CONFIG_SND_SOC_IMX_AUDMUX) += snd-soc-imx-audmux.o + +obj-$(CONFIG_SND_SOC_IMX_PCM) += snd-soc-imx-pcm.o +snd-soc-imx-pcm-y := imx-pcm.o +snd-soc-imx-pcm-$(CONFIG_SND_MXC_SOC_FIQ) += imx-pcm-fiq.o +snd-soc-imx-pcm-$(CONFIG_SND_MXC_SOC_MX2) += imx-pcm-dma-mx2.o + +# i.MX Machine Support +snd-soc-eukrea-tlv320-objs := eukrea-tlv320.o +snd-soc-phycore-ac97-objs := phycore-ac97.o +snd-soc-mx27vis-aic32x4-objs := mx27vis-aic32x4.o +snd-soc-wm1133-ev1-objs := wm1133-ev1.o + +obj-$(CONFIG_SND_SOC_EUKREA_TLV320) += snd-soc-eukrea-tlv320.o +obj-$(CONFIG_SND_SOC_PHYCORE_AC97) += snd-soc-phycore-ac97.o +obj-$(CONFIG_SND_SOC_MX27VIS_AIC32X4) += snd-soc-mx27vis-aic32x4.o +obj-$(CONFIG_SND_MXC_SOC_WM1133_EV1) += snd-soc-wm1133-ev1.o diff --git a/sound/soc/imx/eukrea-tlv320.c b/sound/soc/fsl/eukrea-tlv320.c similarity index 99% rename from sound/soc/imx/eukrea-tlv320.c rename to sound/soc/fsl/eukrea-tlv320.c index 7d4475c..efb9ede 100644 --- a/sound/soc/imx/eukrea-tlv320.c +++ b/sound/soc/fsl/eukrea-tlv320.c @@ -7,7 +7,7 @@ * which is Copyright 2009 Simtec Electronics * and on sound/soc/imx/phycore-ac97.c which is * Copyright 2009 Sascha Hauer, Pengutronix s.hauer@pengutronix.de - * + * * This program is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License as published by the * Free Software Foundation; either version 2 of the License, or (at your diff --git a/sound/soc/imx/imx-audmux.c b/sound/soc/fsl/imx-audmux.c similarity index 100% rename from sound/soc/imx/imx-audmux.c rename to sound/soc/fsl/imx-audmux.c diff --git a/sound/soc/imx/imx-audmux.h b/sound/soc/fsl/imx-audmux.h similarity index 100% rename from sound/soc/imx/imx-audmux.h rename to sound/soc/fsl/imx-audmux.h diff --git a/sound/soc/imx/imx-pcm-dma-mx2.c b/sound/soc/fsl/imx-pcm-dma-mx2.c similarity index 100% rename from sound/soc/imx/imx-pcm-dma-mx2.c rename to sound/soc/fsl/imx-pcm-dma-mx2.c diff --git a/sound/soc/imx/imx-pcm-fiq.c b/sound/soc/fsl/imx-pcm-fiq.c similarity index 100% rename from sound/soc/imx/imx-pcm-fiq.c rename to sound/soc/fsl/imx-pcm-fiq.c diff --git a/sound/soc/imx/imx-pcm.c b/sound/soc/fsl/imx-pcm.c similarity index 100% rename from sound/soc/imx/imx-pcm.c rename to sound/soc/fsl/imx-pcm.c diff --git a/sound/soc/imx/imx-pcm.h b/sound/soc/fsl/imx-pcm.h similarity index 100% rename from sound/soc/imx/imx-pcm.h rename to sound/soc/fsl/imx-pcm.h diff --git a/sound/soc/imx/imx-ssi.c b/sound/soc/fsl/imx-ssi.c similarity index 99% rename from sound/soc/imx/imx-ssi.c rename to sound/soc/fsl/imx-ssi.c index 9203cdd..3e0dac5a 100644 --- a/sound/soc/imx/imx-ssi.c +++ b/sound/soc/fsl/imx-ssi.c @@ -28,7 +28,7 @@ * value. When we read the same register two times (and the register still * contains the same value) these status bits are not set. We work * around this by not polling these bits but only wait a fixed delay. - * + * */
#include <linux/clk.h> diff --git a/sound/soc/imx/imx-ssi.h b/sound/soc/fsl/imx-ssi.h similarity index 100% rename from sound/soc/imx/imx-ssi.h rename to sound/soc/fsl/imx-ssi.h diff --git a/sound/soc/imx/mx27vis-aic32x4.c b/sound/soc/fsl/mx27vis-aic32x4.c similarity index 100% rename from sound/soc/imx/mx27vis-aic32x4.c rename to sound/soc/fsl/mx27vis-aic32x4.c diff --git a/sound/soc/imx/phycore-ac97.c b/sound/soc/fsl/phycore-ac97.c similarity index 100% rename from sound/soc/imx/phycore-ac97.c rename to sound/soc/fsl/phycore-ac97.c diff --git a/sound/soc/imx/wm1133-ev1.c b/sound/soc/fsl/wm1133-ev1.c similarity index 100% rename from sound/soc/imx/wm1133-ev1.c rename to sound/soc/fsl/wm1133-ev1.c diff --git a/sound/soc/imx/Kconfig b/sound/soc/imx/Kconfig deleted file mode 100644 index 810acaa..0000000 --- a/sound/soc/imx/Kconfig +++ /dev/null @@ -1,79 +0,0 @@ -menuconfig SND_IMX_SOC - tristate "SoC Audio for Freescale i.MX CPUs" - depends on ARCH_MXC - help - Say Y or M if you want to add support for codecs attached to - the i.MX SSI interface. - - -if SND_IMX_SOC - -config SND_SOC_IMX_SSI - tristate - -config SND_SOC_IMX_PCM - tristate - -config SND_MXC_SOC_FIQ - tristate - select FIQ - select SND_SOC_IMX_PCM - -config SND_MXC_SOC_MX2 - select SND_SOC_DMAENGINE_PCM - tristate - select SND_SOC_IMX_PCM - -config SND_SOC_IMX_AUDMUX - tristate - -config SND_MXC_SOC_WM1133_EV1 - tristate "Audio on the the i.MX31ADS with WM1133-EV1 fitted" - depends on MACH_MX31ADS_WM1133_EV1 && EXPERIMENTAL - select SND_SOC_WM8350 - select SND_MXC_SOC_FIQ - select SND_SOC_IMX_AUDMUX - select SND_SOC_IMX_SSI - help - Enable support for audio on the i.MX31ADS with the WM1133-EV1 - PMIC board with WM8835x fitted. - -config SND_SOC_MX27VIS_AIC32X4 - tristate "SoC audio support for Visstrim M10 boards" - depends on MACH_IMX27_VISSTRIM_M10 && I2C - select SND_SOC_TLV320AIC32X4 - select SND_MXC_SOC_MX2 - select SND_SOC_IMX_AUDMUX - select SND_SOC_IMX_SSI - help - Say Y if you want to add support for SoC audio on Visstrim SM10 - board with TLV320AIC32X4 codec. - -config SND_SOC_PHYCORE_AC97 - tristate "SoC Audio support for Phytec phyCORE (and phyCARD) boards" - depends on MACH_PCM043 || MACH_PCA100 - select SND_SOC_AC97_BUS - select SND_SOC_WM9712 - select SND_MXC_SOC_FIQ - select SND_SOC_IMX_AUDMUX - select SND_SOC_IMX_SSI - help - Say Y if you want to add support for SoC audio on Phytec phyCORE - and phyCARD boards in AC97 mode - -config SND_SOC_EUKREA_TLV320 - tristate "Eukrea TLV320" - depends on MACH_EUKREA_MBIMX27_BASEBOARD \ - || MACH_EUKREA_MBIMXSD25_BASEBOARD \ - || MACH_EUKREA_MBIMXSD35_BASEBOARD \ - || MACH_EUKREA_MBIMXSD51_BASEBOARD - depends on I2C - select SND_SOC_TLV320AIC23 - select SND_MXC_SOC_FIQ - select SND_SOC_IMX_AUDMUX - select SND_SOC_IMX_SSI - help - Enable I2S based access to the TLV320AIC23B codec attached - to the SSI interface - -endif # SND_IMX_SOC diff --git a/sound/soc/imx/Makefile b/sound/soc/imx/Makefile deleted file mode 100644 index f5db3e9..0000000 --- a/sound/soc/imx/Makefile +++ /dev/null @@ -1,22 +0,0 @@ -# i.MX Platform Support -snd-soc-imx-ssi-objs := imx-ssi.o -snd-soc-imx-audmux-objs := imx-audmux.o - -obj-$(CONFIG_SND_SOC_IMX_SSI) += snd-soc-imx-ssi.o -obj-$(CONFIG_SND_SOC_IMX_AUDMUX) += snd-soc-imx-audmux.o - -obj-$(CONFIG_SND_SOC_IMX_PCM) += snd-soc-imx-pcm.o -snd-soc-imx-pcm-y := imx-pcm.o -snd-soc-imx-pcm-$(CONFIG_SND_MXC_SOC_FIQ) += imx-pcm-fiq.o -snd-soc-imx-pcm-$(CONFIG_SND_MXC_SOC_MX2) += imx-pcm-dma-mx2.o - -# i.MX Machine Support -snd-soc-eukrea-tlv320-objs := eukrea-tlv320.o -snd-soc-phycore-ac97-objs := phycore-ac97.o -snd-soc-mx27vis-aic32x4-objs := mx27vis-aic32x4.o -snd-soc-wm1133-ev1-objs := wm1133-ev1.o - -obj-$(CONFIG_SND_SOC_EUKREA_TLV320) += snd-soc-eukrea-tlv320.o -obj-$(CONFIG_SND_SOC_PHYCORE_AC97) += snd-soc-phycore-ac97.o -obj-$(CONFIG_SND_SOC_MX27VIS_AIC32X4) += snd-soc-mx27vis-aic32x4.o -obj-$(CONFIG_SND_MXC_SOC_WM1133_EV1) += snd-soc-wm1133-ev1.o
Rename a couple of imx-pcm Kconfig options and filename to get them well named and less confusing.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/fsl/Kconfig | 12 ++++++------ sound/soc/fsl/Makefile | 4 ++-- sound/soc/fsl/{imx-pcm-dma-mx2.c => imx-pcm-dma.c} | 0 3 files changed, 8 insertions(+), 8 deletions(-) rename sound/soc/fsl/{imx-pcm-dma-mx2.c => imx-pcm-dma.c} (100%)
diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig index 19856a0..e3f7509 100644 --- a/sound/soc/fsl/Kconfig +++ b/sound/soc/fsl/Kconfig @@ -97,12 +97,12 @@ config SND_SOC_IMX_SSI config SND_SOC_IMX_PCM tristate
-config SND_MXC_SOC_FIQ +config SND_SOC_IMX_PCM_FIQ tristate select FIQ select SND_SOC_IMX_PCM
-config SND_MXC_SOC_MX2 +config SND_SOC_IMX_PCM_DMA tristate select SND_SOC_DMAENGINE_PCM select SND_SOC_IMX_PCM @@ -114,7 +114,7 @@ config SND_MXC_SOC_WM1133_EV1 tristate "Audio on the the i.MX31ADS with WM1133-EV1 fitted" depends on MACH_MX31ADS_WM1133_EV1 && EXPERIMENTAL select SND_SOC_WM8350 - select SND_MXC_SOC_FIQ + select SND_SOC_IMX_PCM_FIQ select SND_SOC_IMX_AUDMUX select SND_SOC_IMX_SSI help @@ -125,7 +125,7 @@ config SND_SOC_MX27VIS_AIC32X4 tristate "SoC audio support for Visstrim M10 boards" depends on MACH_IMX27_VISSTRIM_M10 && I2C select SND_SOC_TLV320AIC32X4 - select SND_MXC_SOC_MX2 + select SND_SOC_IMX_PCM_DMA select SND_SOC_IMX_AUDMUX select SND_SOC_IMX_SSI help @@ -137,7 +137,7 @@ config SND_SOC_PHYCORE_AC97 depends on MACH_PCM043 || MACH_PCA100 select SND_SOC_AC97_BUS select SND_SOC_WM9712 - select SND_MXC_SOC_FIQ + select SND_SOC_IMX_PCM_FIQ select SND_SOC_IMX_AUDMUX select SND_SOC_IMX_SSI help @@ -152,7 +152,7 @@ config SND_SOC_EUKREA_TLV320 || MACH_EUKREA_MBIMXSD51_BASEBOARD depends on I2C select SND_SOC_TLV320AIC23 - select SND_MXC_SOC_FIQ + select SND_SOC_IMX_PCM_FIQ select SND_SOC_IMX_AUDMUX select SND_SOC_IMX_SSI help diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile index 36c257f..f031409 100644 --- a/sound/soc/fsl/Makefile +++ b/sound/soc/fsl/Makefile @@ -30,8 +30,8 @@ obj-$(CONFIG_SND_SOC_IMX_AUDMUX) += snd-soc-imx-audmux.o
obj-$(CONFIG_SND_SOC_IMX_PCM) += snd-soc-imx-pcm.o snd-soc-imx-pcm-y := imx-pcm.o -snd-soc-imx-pcm-$(CONFIG_SND_MXC_SOC_FIQ) += imx-pcm-fiq.o -snd-soc-imx-pcm-$(CONFIG_SND_MXC_SOC_MX2) += imx-pcm-dma-mx2.o +snd-soc-imx-pcm-$(CONFIG_SND_SOC_IMX_PCM_FIQ) += imx-pcm-fiq.o +snd-soc-imx-pcm-$(CONFIG_SND_SOC_IMX_PCM_DMA) += imx-pcm-dma.o
# i.MX Machine Support snd-soc-eukrea-tlv320-objs := eukrea-tlv320.o diff --git a/sound/soc/fsl/imx-pcm-dma-mx2.c b/sound/soc/fsl/imx-pcm-dma.c similarity index 100% rename from sound/soc/fsl/imx-pcm-dma-mx2.c rename to sound/soc/fsl/imx-pcm-dma.c
There is some amount of code duplication between mpc8610_hpcd and p1022_ds machine drivers, and the same code will be duplicated again when another new machine driver is added. The patch creates fsl_utils to accommodate the common functions to stop the code duplication.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/fsl/Kconfig | 5 ++ sound/soc/fsl/Makefile | 2 + sound/soc/fsl/fsl_utils.c | 135 ++++++++++++++++++++++++++++++++++++ sound/soc/fsl/fsl_utils.h | 27 +++++++ sound/soc/fsl/mpc8610_hpcd.c | 157 +++--------------------------------------- sound/soc/fsl/p1022_ds.c | 149 +++------------------------------------- 6 files changed, 189 insertions(+), 286 deletions(-) create mode 100644 sound/soc/fsl/fsl_utils.c create mode 100644 sound/soc/fsl/fsl_utils.h
diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig index e3f7509..535ee73 100644 --- a/sound/soc/fsl/Kconfig +++ b/sound/soc/fsl/Kconfig @@ -1,6 +1,9 @@ config SND_SOC_FSL_SSI tristate
+config SND_SOC_FSL_UTILS + tristate + menuconfig SND_POWERPC_SOC tristate "SoC Audio for Freescale PowerPC CPUs" depends on FSL_SOC @@ -26,6 +29,7 @@ config SND_SOC_MPC8610_HPCD # I2C is necessary for the CS4270 driver depends on MPC8610_HPCD && I2C select SND_SOC_FSL_SSI + select SND_SOC_FSL_UTILS select SND_SOC_POWERPC_DMA select SND_SOC_CS4270 select SND_SOC_CS4270_VD33_ERRATA @@ -38,6 +42,7 @@ config SND_SOC_P1022_DS # I2C is necessary for the WM8776 driver depends on P1022_DS && I2C select SND_SOC_FSL_SSI + select SND_SOC_FSL_UTILS select SND_SOC_POWERPC_DMA select SND_SOC_WM8776 default y if P1022_DS diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile index f031409..fbdbed0 100644 --- a/sound/soc/fsl/Makefile +++ b/sound/soc/fsl/Makefile @@ -8,8 +8,10 @@ obj-$(CONFIG_SND_SOC_P1022_DS) += snd-soc-p1022-ds.o
# Freescale PowerPC SSI/DMA Platform Support snd-soc-fsl-ssi-objs := fsl_ssi.o +snd-soc-fsl-utils-objs := fsl_utils.o snd-soc-fsl-dma-objs := fsl_dma.o obj-$(CONFIG_SND_SOC_FSL_SSI) += snd-soc-fsl-ssi.o +obj-$(CONFIG_SND_SOC_FSL_UTILS) += snd-soc-fsl-utils.o obj-$(CONFIG_SND_SOC_POWERPC_DMA) += snd-soc-fsl-dma.o
# MPC5200 Platform Support diff --git a/sound/soc/fsl/fsl_utils.c b/sound/soc/fsl/fsl_utils.c new file mode 100644 index 0000000..4370c28 --- /dev/null +++ b/sound/soc/fsl/fsl_utils.c @@ -0,0 +1,135 @@ +/** + * Freescale ALSA SoC Machine driver utility + * + * Author: Timur Tabi timur@freescale.com + * + * Copyright 2010 Freescale Semiconductor, Inc. + * + * This file is licensed under the terms of the GNU General Public License + * version 2. This program is licensed "as is" without any warranty of any + * kind, whether express or implied. + */ + +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/of_i2c.h> +#include <sound/soc.h> + +#include "fsl_utils.h" + +/** + * fsl_asoc_get_codec_dev_name - determine the dev_name for a codec node + * + * @np: pointer to the I2C device tree node + * @buf: buffer to be filled with the dev_name of the I2C device + * @len: the length of the buffer + * + * This function determines the dev_name for an I2C node. This is the name + * that would be returned by dev_name() if this device_node were part of a + * 'struct device' It's ugly and hackish, but it works. + * + * The dev_name for such devices include the bus number and I2C address. For + * example, "cs4270.0-004f". + */ +int fsl_asoc_get_codec_dev_name(struct device_node *np, char *buf, size_t len) +{ + const u32 *iprop; + u32 addr; + char temp[DAI_NAME_SIZE]; + struct i2c_client *i2c; + + of_modalias_node(np, temp, DAI_NAME_SIZE); + + iprop = of_get_property(np, "reg", NULL); + if (!iprop) + return -EINVAL; + + addr = be32_to_cpup(iprop); + + /* We need the adapter number */ + i2c = of_find_i2c_device_by_node(np); + if (!i2c) { + put_device(&i2c->dev); + return -ENODEV; + } + + snprintf(buf, len, "%s.%u-%04x", temp, i2c->adapter->nr, addr); + put_device(&i2c->dev); + + return 0; +} +EXPORT_SYMBOL(fsl_asoc_get_codec_dev_name); + +/** + * fsl_asoc_get_dma_channel - determine the dma channel for a SSI node + * + * @ssi_np: pointer to the SSI device tree node + * @name: name of the phandle pointing to the dma channel + * @dai: ASoC DAI link pointer to be filled with platform_name + * @dma_channel_id: dma channel id to be returned + * @dma_id: dma id to be returned + * + * This function determines the dma and channel id for given SSI node. It + * also discovers the platform_name for the ASoC DAI link. + */ +int fsl_asoc_get_dma_channel(struct device_node *ssi_np, + const char *name, + struct snd_soc_dai_link *dai, + unsigned int *dma_channel_id, + unsigned int *dma_id) +{ + struct resource res; + struct device_node *dma_channel_np, *dma_np; + const u32 *iprop; + int ret; + + dma_channel_np = of_parse_phandle(ssi_np, name, 0); + if (!dma_channel_np) + return -EINVAL; + + if (!of_device_is_compatible(dma_channel_np, "fsl,ssi-dma-channel")) { + of_node_put(dma_channel_np); + return -EINVAL; + } + + /* Determine the dev_name for the device_node. This code mimics the + * behavior of of_device_make_bus_id(). We need this because ASoC uses + * the dev_name() of the device to match the platform (DMA) device with + * the CPU (SSI) device. It's all ugly and hackish, but it works (for + * now). + * + * dai->platform name should already point to an allocated buffer. + */ + ret = of_address_to_resource(dma_channel_np, 0, &res); + if (ret) { + of_node_put(dma_channel_np); + return ret; + } + snprintf((char *)dai->platform_name, DAI_NAME_SIZE, "%llx.%s", + (unsigned long long) res.start, dma_channel_np->name); + + iprop = of_get_property(dma_channel_np, "cell-index", NULL); + if (!iprop) { + of_node_put(dma_channel_np); + return -EINVAL; + } + *dma_channel_id = be32_to_cpup(iprop); + + dma_np = of_get_parent(dma_channel_np); + iprop = of_get_property(dma_np, "cell-index", NULL); + if (!iprop) { + of_node_put(dma_np); + return -EINVAL; + } + *dma_id = be32_to_cpup(iprop); + + of_node_put(dma_np); + of_node_put(dma_channel_np); + + return 0; +} +EXPORT_SYMBOL(fsl_asoc_get_dma_channel); + +MODULE_AUTHOR("Timur Tabi timur@freescale.com"); +MODULE_DESCRIPTION("Freescale ASoC utility code"); +MODULE_LICENSE("GPL v2"); diff --git a/sound/soc/fsl/fsl_utils.h b/sound/soc/fsl/fsl_utils.h new file mode 100644 index 0000000..44d1436 --- /dev/null +++ b/sound/soc/fsl/fsl_utils.h @@ -0,0 +1,27 @@ +/** + * Freescale ALSA SoC Machine driver utility + * + * Author: Timur Tabi timur@freescale.com + * + * Copyright 2010 Freescale Semiconductor, Inc. + * + * This file is licensed under the terms of the GNU General Public License + * version 2. This program is licensed "as is" without any warranty of any + * kind, whether express or implied. + */ + +#ifndef _FSL_UTILS_H +#define _FSL_UTILS_H + +#define DAI_NAME_SIZE 32 + +struct snd_soc_dai_link; +struct device_node; + +int fsl_asoc_get_codec_dev_name(struct device_node *np, char *buf, size_t len); +int fsl_asoc_get_dma_channel(struct device_node *ssi_np, const char *name, + struct snd_soc_dai_link *dai, + unsigned int *dma_channel_id, + unsigned int *dma_id); + +#endif /* _FSL_UTILS_H */ diff --git a/sound/soc/fsl/mpc8610_hpcd.c b/sound/soc/fsl/mpc8610_hpcd.c index fcf9302..7ead1e5 100644 --- a/sound/soc/fsl/mpc8610_hpcd.c +++ b/sound/soc/fsl/mpc8610_hpcd.c @@ -14,18 +14,16 @@ #include <linux/interrupt.h> #include <linux/of_device.h> #include <linux/slab.h> -#include <linux/of_i2c.h> #include <sound/soc.h> #include <asm/fsl_guts.h>
#include "fsl_dma.h" #include "fsl_ssi.h" +#include "fsl_utils.h"
/* There's only one global utilities register */ static phys_addr_t guts_phys;
-#define DAI_NAME_SIZE 32 - /** * mpc8610_hpcd_data: machine-specific ASoC device data * @@ -181,141 +179,6 @@ static struct snd_soc_ops mpc8610_hpcd_ops = { };
/** - * get_node_by_phandle_name - get a node by its phandle name - * - * This function takes a node, the name of a property in that node, and a - * compatible string. Assuming the property is a phandle to another node, - * it returns that node, (optionally) if that node is compatible. - * - * If the property is not a phandle, or the node it points to is not compatible - * with the specific string, then NULL is returned. - */ -static struct device_node *get_node_by_phandle_name(struct device_node *np, - const char *name, - const char *compatible) -{ - const phandle *ph; - int len; - - ph = of_get_property(np, name, &len); - if (!ph || (len != sizeof(phandle))) - return NULL; - - np = of_find_node_by_phandle(*ph); - if (!np) - return NULL; - - if (compatible && !of_device_is_compatible(np, compatible)) { - of_node_put(np); - return NULL; - } - - return np; -} - -/** - * get_parent_cell_index -- return the cell-index of the parent of a node - * - * Return the value of the cell-index property of the parent of the given - * node. This is used for DMA channel nodes that need to know the DMA ID - * of the controller they are on. - */ -static int get_parent_cell_index(struct device_node *np) -{ - struct device_node *parent = of_get_parent(np); - const u32 *iprop; - - if (!parent) - return -1; - - iprop = of_get_property(parent, "cell-index", NULL); - of_node_put(parent); - - if (!iprop) - return -1; - - return be32_to_cpup(iprop); -} - -/** - * codec_node_dev_name - determine the dev_name for a codec node - * - * This function determines the dev_name for an I2C node. This is the name - * that would be returned by dev_name() if this device_node were part of a - * 'struct device' It's ugly and hackish, but it works. - * - * The dev_name for such devices include the bus number and I2C address. For - * example, "cs4270.0-004f". - */ -static int codec_node_dev_name(struct device_node *np, char *buf, size_t len) -{ - const u32 *iprop; - int addr; - char temp[DAI_NAME_SIZE]; - struct i2c_client *i2c; - - of_modalias_node(np, temp, DAI_NAME_SIZE); - - iprop = of_get_property(np, "reg", NULL); - if (!iprop) - return -EINVAL; - - addr = be32_to_cpup(iprop); - - /* We need the adapter number */ - i2c = of_find_i2c_device_by_node(np); - if (!i2c) - return -ENODEV; - - snprintf(buf, len, "%s.%u-%04x", temp, i2c->adapter->nr, addr); - - return 0; -} - -static int get_dma_channel(struct device_node *ssi_np, - const char *name, - struct snd_soc_dai_link *dai, - unsigned int *dma_channel_id, - unsigned int *dma_id) -{ - struct resource res; - struct device_node *dma_channel_np; - const u32 *iprop; - int ret; - - dma_channel_np = get_node_by_phandle_name(ssi_np, name, - "fsl,ssi-dma-channel"); - if (!dma_channel_np) - return -EINVAL; - - /* Determine the dev_name for the device_node. This code mimics the - * behavior of of_device_make_bus_id(). We need this because ASoC uses - * the dev_name() of the device to match the platform (DMA) device with - * the CPU (SSI) device. It's all ugly and hackish, but it works (for - * now). - * - * dai->platform name should already point to an allocated buffer. - */ - ret = of_address_to_resource(dma_channel_np, 0, &res); - if (ret) - return ret; - snprintf((char *)dai->platform_name, DAI_NAME_SIZE, "%llx.%s", - (unsigned long long) res.start, dma_channel_np->name); - - iprop = of_get_property(dma_channel_np, "cell-index", NULL); - if (!iprop) { - of_node_put(dma_channel_np); - return -EINVAL; - } - - *dma_channel_id = be32_to_cpup(iprop); - *dma_id = get_parent_cell_index(dma_channel_np); - of_node_put(dma_channel_np); - - return 0; -} - -/** * mpc8610_hpcd_probe: platform probe function for the machine driver * * Although this is a machine driver, the SSI node is the "master" node with @@ -353,8 +216,8 @@ static int mpc8610_hpcd_probe(struct platform_device *pdev) machine_data->dai[0].ops = &mpc8610_hpcd_ops;
/* Determine the codec name, it will be used as the codec DAI name */ - ret = codec_node_dev_name(codec_np, machine_data->codec_name, - DAI_NAME_SIZE); + ret = fsl_asoc_get_codec_dev_name(codec_np, machine_data->codec_name, + DAI_NAME_SIZE); if (ret) { dev_err(&pdev->dev, "invalid codec node %s\n", codec_np->full_name); @@ -458,9 +321,10 @@ static int mpc8610_hpcd_probe(struct platform_device *pdev)
/* Find the playback DMA channel to use. */ machine_data->dai[0].platform_name = machine_data->platform_name[0]; - ret = get_dma_channel(np, "fsl,playback-dma", &machine_data->dai[0], - &machine_data->dma_channel_id[0], - &machine_data->dma_id[0]); + ret = fsl_asoc_get_dma_channel(np, "fsl,playback-dma", + &machine_data->dai[0], + &machine_data->dma_channel_id[0], + &machine_data->dma_id[0]); if (ret) { dev_err(&pdev->dev, "missing/invalid playback DMA phandle\n"); goto error; @@ -468,9 +332,10 @@ static int mpc8610_hpcd_probe(struct platform_device *pdev)
/* Find the capture DMA channel to use. */ machine_data->dai[1].platform_name = machine_data->platform_name[1]; - ret = get_dma_channel(np, "fsl,capture-dma", &machine_data->dai[1], - &machine_data->dma_channel_id[1], - &machine_data->dma_id[1]); + ret = fsl_asoc_get_dma_channel(np, "fsl,capture-dma", + &machine_data->dai[1], + &machine_data->dma_channel_id[1], + &machine_data->dma_id[1]); if (ret) { dev_err(&pdev->dev, "missing/invalid capture DMA phandle\n"); goto error; diff --git a/sound/soc/fsl/p1022_ds.c b/sound/soc/fsl/p1022_ds.c index d32ec46..60f7bb8 100644 --- a/sound/soc/fsl/p1022_ds.c +++ b/sound/soc/fsl/p1022_ds.c @@ -14,12 +14,12 @@ #include <linux/interrupt.h> #include <linux/of_device.h> #include <linux/slab.h> -#include <linux/of_i2c.h> #include <sound/soc.h> #include <asm/fsl_guts.h>
#include "fsl_dma.h" #include "fsl_ssi.h" +#include "fsl_utils.h"
/* P1022-specific PMUXCR and DMUXCR bit definitions */
@@ -57,8 +57,6 @@ static inline void guts_set_dmuxcr(struct ccsr_guts_85xx __iomem *guts, /* There's only one global utilities register */ static phys_addr_t guts_phys;
-#define DAI_NAME_SIZE 32 - /** * machine_data: machine-specific ASoC device data * @@ -191,136 +189,6 @@ static struct snd_soc_ops p1022_ds_ops = { };
/** - * get_node_by_phandle_name - get a node by its phandle name - * - * This function takes a node, the name of a property in that node, and a - * compatible string. Assuming the property is a phandle to another node, - * it returns that node, (optionally) if that node is compatible. - * - * If the property is not a phandle, or the node it points to is not compatible - * with the specific string, then NULL is returned. - */ -static struct device_node *get_node_by_phandle_name(struct device_node *np, - const char *name, const char *compatible) -{ - np = of_parse_phandle(np, name, 0); - if (!np) - return NULL; - - if (!of_device_is_compatible(np, compatible)) { - of_node_put(np); - return NULL; - } - - return np; -} - -/** - * get_parent_cell_index -- return the cell-index of the parent of a node - * - * Return the value of the cell-index property of the parent of the given - * node. This is used for DMA channel nodes that need to know the DMA ID - * of the controller they are on. - */ -static int get_parent_cell_index(struct device_node *np) -{ - struct device_node *parent = of_get_parent(np); - const u32 *iprop; - int ret = -1; - - if (!parent) - return -1; - - iprop = of_get_property(parent, "cell-index", NULL); - if (iprop) - ret = be32_to_cpup(iprop); - - of_node_put(parent); - - return ret; -} - -/** - * codec_node_dev_name - determine the dev_name for a codec node - * - * This function determines the dev_name for an I2C node. This is the name - * that would be returned by dev_name() if this device_node were part of a - * 'struct device' It's ugly and hackish, but it works. - * - * The dev_name for such devices include the bus number and I2C address. For - * example, "cs4270-codec.0-004f". - */ -static int codec_node_dev_name(struct device_node *np, char *buf, size_t len) -{ - const u32 *iprop; - int addr; - char temp[DAI_NAME_SIZE]; - struct i2c_client *i2c; - - of_modalias_node(np, temp, DAI_NAME_SIZE); - - iprop = of_get_property(np, "reg", NULL); - if (!iprop) - return -EINVAL; - - addr = be32_to_cpup(iprop); - - /* We need the adapter number */ - i2c = of_find_i2c_device_by_node(np); - if (!i2c) - return -ENODEV; - - snprintf(buf, len, "%s.%u-%04x", temp, i2c->adapter->nr, addr); - - return 0; -} - -static int get_dma_channel(struct device_node *ssi_np, - const char *name, - struct snd_soc_dai_link *dai, - unsigned int *dma_channel_id, - unsigned int *dma_id) -{ - struct resource res; - struct device_node *dma_channel_np; - const u32 *iprop; - int ret; - - dma_channel_np = get_node_by_phandle_name(ssi_np, name, - "fsl,ssi-dma-channel"); - if (!dma_channel_np) - return -EINVAL; - - /* Determine the dev_name for the device_node. This code mimics the - * behavior of of_device_make_bus_id(). We need this because ASoC uses - * the dev_name() of the device to match the platform (DMA) device with - * the CPU (SSI) device. It's all ugly and hackish, but it works (for - * now). - * - * dai->platform name should already point to an allocated buffer. - */ - ret = of_address_to_resource(dma_channel_np, 0, &res); - if (ret) { - of_node_put(dma_channel_np); - return ret; - } - snprintf((char *)dai->platform_name, DAI_NAME_SIZE, "%llx.%s", - (unsigned long long) res.start, dma_channel_np->name); - - iprop = of_get_property(dma_channel_np, "cell-index", NULL); - if (!iprop) { - of_node_put(dma_channel_np); - return -EINVAL; - } - - *dma_channel_id = be32_to_cpup(iprop); - *dma_id = get_parent_cell_index(dma_channel_np); - of_node_put(dma_channel_np); - - return 0; -} - -/** * p1022_ds_probe: platform probe function for the machine driver * * Although this is a machine driver, the SSI node is the "master" node with @@ -358,7 +226,8 @@ static int p1022_ds_probe(struct platform_device *pdev) mdata->dai[0].ops = &p1022_ds_ops;
/* Determine the codec name, it will be used as the codec DAI name */ - ret = codec_node_dev_name(codec_np, mdata->codec_name, DAI_NAME_SIZE); + ret = fsl_asoc_get_codec_dev_name(codec_np, mdata->codec_name, + DAI_NAME_SIZE); if (ret) { dev_err(&pdev->dev, "invalid codec node %s\n", codec_np->full_name); @@ -454,9 +323,9 @@ static int p1022_ds_probe(struct platform_device *pdev)
/* Find the playback DMA channel to use. */ mdata->dai[0].platform_name = mdata->platform_name[0]; - ret = get_dma_channel(np, "fsl,playback-dma", &mdata->dai[0], - &mdata->dma_channel_id[0], - &mdata->dma_id[0]); + ret = fsl_asoc_get_dma_channel(np, "fsl,playback-dma", &mdata->dai[0], + &mdata->dma_channel_id[0], + &mdata->dma_id[0]); if (ret) { dev_err(&pdev->dev, "missing/invalid playback DMA phandle\n"); goto error; @@ -464,9 +333,9 @@ static int p1022_ds_probe(struct platform_device *pdev)
/* Find the capture DMA channel to use. */ mdata->dai[1].platform_name = mdata->platform_name[1]; - ret = get_dma_channel(np, "fsl,capture-dma", &mdata->dai[1], - &mdata->dma_channel_id[1], - &mdata->dma_id[1]); + ret = fsl_asoc_get_dma_channel(np, "fsl,capture-dma", &mdata->dai[1], + &mdata->dma_channel_id[1], + &mdata->dma_id[1]); if (ret) { dev_err(&pdev->dev, "missing/invalid capture DMA phandle\n"); goto error;
The ASoC core now can support matching codec with device node besides name, so we can save helper function fsl_asoc_get_codec_dev_name.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/fsl/fsl_utils.c | 44 ------------------------------------------ sound/soc/fsl/fsl_utils.h | 1 - sound/soc/fsl/mpc8610_hpcd.c | 13 +---------- sound/soc/fsl/p1022_ds.c | 13 +---------- 4 files changed, 4 insertions(+), 67 deletions(-)
diff --git a/sound/soc/fsl/fsl_utils.c b/sound/soc/fsl/fsl_utils.c index 4370c28..b9e42b5 100644 --- a/sound/soc/fsl/fsl_utils.c +++ b/sound/soc/fsl/fsl_utils.c @@ -12,55 +12,11 @@
#include <linux/module.h> #include <linux/of_address.h> -#include <linux/of_i2c.h> #include <sound/soc.h>
#include "fsl_utils.h"
/** - * fsl_asoc_get_codec_dev_name - determine the dev_name for a codec node - * - * @np: pointer to the I2C device tree node - * @buf: buffer to be filled with the dev_name of the I2C device - * @len: the length of the buffer - * - * This function determines the dev_name for an I2C node. This is the name - * that would be returned by dev_name() if this device_node were part of a - * 'struct device' It's ugly and hackish, but it works. - * - * The dev_name for such devices include the bus number and I2C address. For - * example, "cs4270.0-004f". - */ -int fsl_asoc_get_codec_dev_name(struct device_node *np, char *buf, size_t len) -{ - const u32 *iprop; - u32 addr; - char temp[DAI_NAME_SIZE]; - struct i2c_client *i2c; - - of_modalias_node(np, temp, DAI_NAME_SIZE); - - iprop = of_get_property(np, "reg", NULL); - if (!iprop) - return -EINVAL; - - addr = be32_to_cpup(iprop); - - /* We need the adapter number */ - i2c = of_find_i2c_device_by_node(np); - if (!i2c) { - put_device(&i2c->dev); - return -ENODEV; - } - - snprintf(buf, len, "%s.%u-%04x", temp, i2c->adapter->nr, addr); - put_device(&i2c->dev); - - return 0; -} -EXPORT_SYMBOL(fsl_asoc_get_codec_dev_name); - -/** * fsl_asoc_get_dma_channel - determine the dma channel for a SSI node * * @ssi_np: pointer to the SSI device tree node diff --git a/sound/soc/fsl/fsl_utils.h b/sound/soc/fsl/fsl_utils.h index 44d1436..b295112 100644 --- a/sound/soc/fsl/fsl_utils.h +++ b/sound/soc/fsl/fsl_utils.h @@ -18,7 +18,6 @@ struct snd_soc_dai_link; struct device_node;
-int fsl_asoc_get_codec_dev_name(struct device_node *np, char *buf, size_t len); int fsl_asoc_get_dma_channel(struct device_node *ssi_np, const char *name, struct snd_soc_dai_link *dai, unsigned int *dma_channel_id, diff --git a/sound/soc/fsl/mpc8610_hpcd.c b/sound/soc/fsl/mpc8610_hpcd.c index 7ead1e5..354b753 100644 --- a/sound/soc/fsl/mpc8610_hpcd.c +++ b/sound/soc/fsl/mpc8610_hpcd.c @@ -41,7 +41,6 @@ struct mpc8610_hpcd_data { unsigned int dma_id[2]; /* 0 = DMA1, 1 = DMA2, etc */ unsigned int dma_channel_id[2]; /* 0 = ch 0, 1 = ch 1, etc*/ char codec_dai_name[DAI_NAME_SIZE]; - char codec_name[DAI_NAME_SIZE]; char platform_name[2][DAI_NAME_SIZE]; /* One for each DMA channel */ };
@@ -215,16 +214,8 @@ static int mpc8610_hpcd_probe(struct platform_device *pdev) machine_data->dai[0].cpu_dai_name = dev_name(&ssi_pdev->dev); machine_data->dai[0].ops = &mpc8610_hpcd_ops;
- /* Determine the codec name, it will be used as the codec DAI name */ - ret = fsl_asoc_get_codec_dev_name(codec_np, machine_data->codec_name, - DAI_NAME_SIZE); - if (ret) { - dev_err(&pdev->dev, "invalid codec node %s\n", - codec_np->full_name); - ret = -EINVAL; - goto error; - } - machine_data->dai[0].codec_name = machine_data->codec_name; + /* ASoC core can match codec with device node */ + machine_data->dai[0].codec_of_node = codec_np;
/* The DAI name from the codec (snd_soc_dai_driver.name) */ machine_data->dai[0].codec_dai_name = "cs4270-hifi"; diff --git a/sound/soc/fsl/p1022_ds.c b/sound/soc/fsl/p1022_ds.c index 60f7bb8..e87cb41 100644 --- a/sound/soc/fsl/p1022_ds.c +++ b/sound/soc/fsl/p1022_ds.c @@ -73,7 +73,6 @@ struct machine_data { unsigned int ssi_id; /* 0 = SSI1, 1 = SSI2, etc */ unsigned int dma_id[2]; /* 0 = DMA1, 1 = DMA2, etc */ unsigned int dma_channel_id[2]; /* 0 = ch 0, 1 = ch 1, etc*/ - char codec_name[DAI_NAME_SIZE]; char platform_name[2][DAI_NAME_SIZE]; /* One for each DMA channel */ };
@@ -225,16 +224,8 @@ static int p1022_ds_probe(struct platform_device *pdev) mdata->dai[0].cpu_dai_name = dev_name(&ssi_pdev->dev); mdata->dai[0].ops = &p1022_ds_ops;
- /* Determine the codec name, it will be used as the codec DAI name */ - ret = fsl_asoc_get_codec_dev_name(codec_np, mdata->codec_name, - DAI_NAME_SIZE); - if (ret) { - dev_err(&pdev->dev, "invalid codec node %s\n", - codec_np->full_name); - ret = -EINVAL; - goto error; - } - mdata->dai[0].codec_name = mdata->codec_name; + /* ASoC core can match codec with device node */ + mdata->dai[0].codec_of_node = codec_np;
/* We register two DAIs per SSI, one for playback and the other for * capture. We support codecs that have separate DAIs for both playback
Check /compatible rather than /model to determine the machine name. The p1022ds older device trees get a different /model from the new ones, while /compatible is consistent there, so checking /compatible will save the bother of detecting older p1022ds device trees.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/fsl/fsl_ssi.c | 6 +++--- sound/soc/fsl/mpc8610_hpcd.c | 2 +- sound/soc/fsl/p1022_ds.c | 32 +++++--------------------------- 3 files changed, 9 insertions(+), 31 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 3e06696..2eb407f 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -716,12 +716,12 @@ static int __devinit fsl_ssi_probe(struct platform_device *pdev) }
/* Trigger the machine driver's probe function. The platform driver - * name of the machine driver is taken from the /model property of the + * name of the machine driver is taken from /compatible property of the * device tree. We also pass the address of the CPU DAI driver * structure. */ - sprop = of_get_property(of_find_node_by_path("/"), "model", NULL); - /* Sometimes the model name has a "fsl," prefix, so we strip that. */ + sprop = of_get_property(of_find_node_by_path("/"), "compatible", NULL); + /* Sometimes the compatible name has a "fsl," prefix, so we strip it. */ p = strrchr(sprop, ','); if (p) sprop = p + 1; diff --git a/sound/soc/fsl/mpc8610_hpcd.c b/sound/soc/fsl/mpc8610_hpcd.c index 354b753..8fdc430 100644 --- a/sound/soc/fsl/mpc8610_hpcd.c +++ b/sound/soc/fsl/mpc8610_hpcd.c @@ -402,7 +402,7 @@ static struct platform_driver mpc8610_hpcd_driver = { .probe = mpc8610_hpcd_probe, .remove = __devexit_p(mpc8610_hpcd_remove), .driver = { - /* The name must match the 'model' property in the device tree, + /* The name must match 'compatible' property in the device tree, * in lowercase letters. */ .name = "snd-soc-mpc8610hpcd", diff --git a/sound/soc/fsl/p1022_ds.c b/sound/soc/fsl/p1022_ds.c index e87cb41..0f42f31 100644 --- a/sound/soc/fsl/p1022_ds.c +++ b/sound/soc/fsl/p1022_ds.c @@ -403,6 +403,11 @@ static struct platform_driver p1022_ds_driver = { .probe = p1022_ds_probe, .remove = __devexit_p(p1022_ds_remove), .driver = { + /* + * The name must match 'compatible' property in the device tree, + * in lowercase letters. + */ + .name = "snd-soc-p1022ds", .owner = THIS_MODULE, }, }; @@ -416,33 +421,6 @@ static int __init p1022_ds_init(void) { struct device_node *guts_np; struct resource res; - const char *sprop; - - /* - * Check if we're actually running on a P1022DS. Older device trees - * have a model of "fsl,P1022" and newer ones use "fsl,P1022DS", so we - * need to support both. The SSI driver uses that property to link to - * the machine driver, so have to match it. - */ - sprop = of_get_property(of_find_node_by_path("/"), "model", NULL); - if (!sprop) { - pr_err("snd-soc-p1022ds: missing /model node"); - return -ENODEV; - } - - pr_debug("snd-soc-p1022ds: board model name is %s\n", sprop); - - /* - * The name of this board, taken from the device tree. Normally, this is a* - * fixed string, but some P1022DS device trees have a /model property of - * "fsl,P1022", and others have "fsl,P1022DS". - */ - if (strcasecmp(sprop, "fsl,p1022ds") == 0) - p1022_ds_driver.driver.name = "snd-soc-p1022ds"; - else if (strcasecmp(sprop, "fsl,p1022") == 0) - p1022_ds_driver.driver.name = "snd-soc-p1022"; - else - return -ENODEV;
/* Get the physical address of the global utilities registers */ guts_np = of_find_compatible_node(NULL, NULL, "fsl,p1022-guts");
Shawn Guo wrote:
Check /compatible rather than /model to determine the machine name. The p1022ds older device trees get a different /model from the new ones, while /compatible is consistent there, so checking /compatible will save the bother of detecting older p1022ds device trees.
Signed-off-by: Shawn Guo shawn.guo@linaro.org
Acked-by: Timur Tabi timur@freescale.com
On Fri, Mar 09, 2012 at 12:59:46AM +0800, Shawn Guo wrote:
Check /compatible rather than /model to determine the machine name. The p1022ds older device trees get a different /model from the new ones, while /compatible is consistent there, so checking /compatible will save the bother of detecting older p1022ds device trees.
Applied, thanks.
Provide different pair of accessors for accessing SSI registers on PowerPC and ARM/IMX, so that fsl_ssi driver can be built on both architectures.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/fsl/fsl_ssi.c | 67 +++++++++++++++++++++++++++++++--------------- 1 files changed, 45 insertions(+), 22 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 2eb407f..aee2066 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -11,12 +11,16 @@ */
#include <linux/init.h> +#include <linux/io.h> #include <linux/module.h> #include <linux/interrupt.h> #include <linux/device.h> #include <linux/delay.h> #include <linux/slab.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> #include <linux/of_platform.h> +#include <linux/spinlock.h>
#include <sound/core.h> #include <sound/pcm.h> @@ -26,6 +30,27 @@
#include "fsl_ssi.h"
+#ifdef PPC +#define read_ssi(addr) in_be32(addr) +#define write_ssi(val, addr) out_be32(addr, val) +#define write_ssi_mask(addr, clear, set) clrsetbits_be32(addr, clear, set) +#elif defined ARM +#define read_ssi(addr) readl(addr) +#define write_ssi(val, addr) writel(val, addr) +static DEFINE_SPINLOCK(ssi_reg_lock); +static inline void write_ssi_mask(u32 __iomem *addr, u32 clear, u32 set) +{ + u32 val; + unsigned long flags; + + spin_lock_irqsave(&ssi_reg_lock, flags); + val = readl(addr); + val = (val & ~clear) | set; + writel(val, addr); + spin_unlock_irqrestore(&ssi_reg_lock, flags); +} +#endif + /** * FSLSSI_I2S_RATES: sample rates supported by the I2S * @@ -145,7 +170,7 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id) were interrupted for. We mask it with the Interrupt Enable register so that we only check for events that we're interested in. */ - sisr = in_be32(&ssi->sisr) & SIER_FLAGS; + sisr = read_ssi(&ssi->sisr) & SIER_FLAGS;
if (sisr & CCSR_SSI_SISR_RFRC) { ssi_private->stats.rfrc++; @@ -260,7 +285,7 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id)
/* Clear the bits that we set */ if (sisr2) - out_be32(&ssi->sisr, sisr2); + write_ssi(sisr2, &ssi->sisr);
return ret; } @@ -295,7 +320,7 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, * SSI needs to be disabled before updating the registers we set * here. */ - clrbits32(&ssi->scr, CCSR_SSI_SCR_SSIEN); + write_ssi_mask(&ssi->scr, CCSR_SSI_SCR_SSIEN, 0);
/* * Program the SSI into I2S Slave Non-Network Synchronous mode. @@ -303,20 +328,18 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, * * FIXME: Little-endian samples require a different shift dir */ - clrsetbits_be32(&ssi->scr, + write_ssi_mask(&ssi->scr, CCSR_SSI_SCR_I2S_MODE_MASK | CCSR_SSI_SCR_SYN, CCSR_SSI_SCR_TFR_CLK_DIS | CCSR_SSI_SCR_I2S_MODE_SLAVE | (synchronous ? CCSR_SSI_SCR_SYN : 0));
- out_be32(&ssi->stcr, - CCSR_SSI_STCR_TXBIT0 | CCSR_SSI_STCR_TFEN0 | + write_ssi(CCSR_SSI_STCR_TXBIT0 | CCSR_SSI_STCR_TFEN0 | CCSR_SSI_STCR_TFSI | CCSR_SSI_STCR_TEFS | - CCSR_SSI_STCR_TSCKP); + CCSR_SSI_STCR_TSCKP, &ssi->stcr);
- out_be32(&ssi->srcr, - CCSR_SSI_SRCR_RXBIT0 | CCSR_SSI_SRCR_RFEN0 | + write_ssi(CCSR_SSI_SRCR_RXBIT0 | CCSR_SSI_SRCR_RFEN0 | CCSR_SSI_SRCR_RFSI | CCSR_SSI_SRCR_REFS | - CCSR_SSI_SRCR_RSCKP); + CCSR_SSI_SRCR_RSCKP, &ssi->srcr);
/* * The DC and PM bits are only used if the SSI is the clock @@ -324,7 +347,7 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, */
/* Enable the interrupts and DMA requests */ - out_be32(&ssi->sier, SIER_FLAGS); + write_ssi(SIER_FLAGS, &ssi->sier);
/* * Set the watermark for transmit FIFI 0 and receive FIFO 0. We @@ -339,9 +362,9 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, * make this value larger (and maybe we should), but this way * data will be written to memory as soon as it's available. */ - out_be32(&ssi->sfcsr, - CCSR_SSI_SFCSR_TFWM0(ssi_private->fifo_depth - 2) | - CCSR_SSI_SFCSR_RFWM0(ssi_private->fifo_depth - 2)); + write_ssi(CCSR_SSI_SFCSR_TFWM0(ssi_private->fifo_depth - 2) | + CCSR_SSI_SFCSR_RFWM0(ssi_private->fifo_depth - 2), + &ssi->sfcsr);
/* * We keep the SSI disabled because if we enable it, then the @@ -417,7 +440,7 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream *substream, unsigned int sample_size = snd_pcm_format_width(params_format(hw_params)); u32 wl = CCSR_SSI_SxCCR_WL(sample_size); - int enabled = in_be32(&ssi->scr) & CCSR_SSI_SCR_SSIEN; + int enabled = read_ssi(&ssi->scr) & CCSR_SSI_SCR_SSIEN;
/* * If we're in synchronous mode, and the SSI is already enabled, @@ -439,9 +462,9 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream *substream, /* In synchronous mode, the SSI uses STCCR for capture */ if ((substream->stream == SNDRV_PCM_STREAM_PLAYBACK) || ssi_private->cpu_dai_drv.symmetric_rates) - clrsetbits_be32(&ssi->stccr, CCSR_SSI_SxCCR_WL_MASK, wl); + write_ssi_mask(&ssi->stccr, CCSR_SSI_SxCCR_WL_MASK, wl); else - clrsetbits_be32(&ssi->srccr, CCSR_SSI_SxCCR_WL_MASK, wl); + write_ssi_mask(&ssi->srccr, CCSR_SSI_SxCCR_WL_MASK, wl);
return 0; } @@ -466,19 +489,19 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - setbits32(&ssi->scr, + write_ssi_mask(&ssi->scr, 0, CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE); else - setbits32(&ssi->scr, + write_ssi_mask(&ssi->scr, 0, CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_RE); break;
case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - clrbits32(&ssi->scr, CCSR_SSI_SCR_TE); + write_ssi_mask(&ssi->scr, CCSR_SSI_SCR_TE, 0); else - clrbits32(&ssi->scr, CCSR_SSI_SCR_RE); + write_ssi_mask(&ssi->scr, CCSR_SSI_SCR_RE, 0); break;
default: @@ -510,7 +533,7 @@ static void fsl_ssi_shutdown(struct snd_pcm_substream *substream, if (!ssi_private->first_stream) { struct ccsr_ssi __iomem *ssi = ssi_private->ssi;
- clrbits32(&ssi->scr, CCSR_SSI_SCR_SSIEN); + write_ssi_mask(&ssi->scr, CCSR_SSI_SCR_SSIEN, 0); } }
Shawn Guo wrote:
+static DEFINE_SPINLOCK(ssi_reg_lock); +static inline void write_ssi_mask(u32 __iomem *addr, u32 clear, u32 set) +{
- u32 val;
- unsigned long flags;
- spin_lock_irqsave(&ssi_reg_lock, flags);
- val = readl(addr);
- val = (val & ~clear) | set;
- writel(val, addr);
- spin_unlock_irqrestore(&ssi_reg_lock, flags);
+} +#endif
I think this spinlock is the wrong approach. The problem with read-modify-write is on the function level, not the register level.
On Thu, Mar 08, 2012 at 02:13:04PM -0600, Timur Tabi wrote:
Shawn Guo wrote:
+static DEFINE_SPINLOCK(ssi_reg_lock); +static inline void write_ssi_mask(u32 __iomem *addr, u32 clear, u32 set) +{
- u32 val;
- unsigned long flags;
- spin_lock_irqsave(&ssi_reg_lock, flags);
- val = readl(addr);
- val = (val & ~clear) | set;
- writel(val, addr);
- spin_unlock_irqrestore(&ssi_reg_lock, flags);
+} +#endif
I think this spinlock is the wrong approach. The problem with read-modify-write is on the function level, not the register level.
Something like this:
@@ -496,6 +494,10 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_pcm_runtime *rtd = substream->private_data; struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(rtd->cpu_dai); struct ccsr_ssi __iomem *ssi = ssi_private->ssi; + unsigned long flags; + int ret = 0; + + spin_lock_irqsave(&ssi_lock, flags);
switch (cmd) { case SNDRV_PCM_TRIGGER_START: @@ -517,10 +519,12 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd, break;
default: - return -EINVAL; + ret = -EINVAL; }
- return 0; + spin_unlock_irqrestore(&ssi_lock, flags); + + return ret; }
Shawn Guo wrote:
Something like this:
@@ -496,6 +494,10 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_pcm_runtime *rtd = substream->private_data; struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(rtd->cpu_dai); struct ccsr_ssi __iomem *ssi = ssi_private->ssi;
unsigned long flags;int ret = 0;spin_lock_irqsave(&ssi_lock, flags); switch (cmd) { case SNDRV_PCM_TRIGGER_START:@@ -517,10 +519,12 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd, break;
default:
return -EINVAL;
ret = -EINVAL; }
return 0;
spin_unlock_irqrestore(&ssi_lock, flags);return ret;
That's the general idea, but I think it needs more study. I don't consider this to be a bug that needs to be fixed ASAP, since it's never been an actual problem so far. For instance, do we need to protect between hw_params and _trigger?
On Fri, Mar 09, 2012 at 02:09:57AM +0000, Tabi Timur-B04825 wrote:
That's the general idea, but I think it needs more study. I don't consider this to be a bug that needs to be fixed ASAP, since it's never been an actual problem so far. For instance, do we need to protect between hw_params and _trigger?
Ok, I will leave it to you with a big "FIXME" comment then.
Shawn Guo wrote:
On Fri, Mar 09, 2012 at 02:09:57AM +0000, Tabi Timur-B04825 wrote:
That's the general idea, but I think it needs more study. I don't consider this to be a bug that needs to be fixed ASAP, since it's never been an actual problem so far. For instance, do we need to protect between hw_params and _trigger?
Ok, I will leave it to you with a big "FIXME" comment then.
Ok.
On Fri, Mar 09, 2012 at 11:21:31AM +0800, Shawn Guo wrote:
On Fri, Mar 09, 2012 at 02:09:57AM +0000, Tabi Timur-B04825 wrote:
That's the general idea, but I think it needs more study. I don't consider this to be a bug that needs to be fixed ASAP, since it's never been an actual problem so far. For instance, do we need to protect between hw_params and _trigger?
Yes, the capture and playback streams may run in parallel.
Ok, I will leave it to you with a big "FIXME" comment then.
You probably just need a read/modify/write function to do the locking in the style of update_bits().
As Documentation/devicetree/bindings/powerpc/fsl/ssi.txt documented, codec-handle is an optional property, so missing it should not be a fatal error.
More importantly, the imx machine drivers to be added working with fsl_ssi will not have this property, as they use new ASoC machine driver binding mechanism.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/fsl/fsl_ssi.c | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index aee2066..e5903cb 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -645,12 +645,6 @@ static int __devinit fsl_ssi_probe(struct platform_device *pdev) if (!of_device_is_available(np)) return -ENODEV;
- /* Check for a codec-handle property. */ - if (!of_get_property(np, "codec-handle", NULL)) { - dev_err(&pdev->dev, "missing codec-handle property\n"); - return -ENODEV; - } - /* We only support the SSI in "I2S Slave" mode */ sprop = of_get_property(np, "fsl,mode", NULL); if (!sprop || strcmp(sprop, "i2s-slave")) {
Shawn Guo wrote:
As Documentation/devicetree/bindings/powerpc/fsl/ssi.txt documented, codec-handle is an optional property, so missing it should not be a fatal error.]
It's optional in the sense that if your SSI is not connected to anything, then you don't need to specify the codec handle. If the SSI is connected to a codec, then the SSI node needs to point to it.
More importantly, the imx machine drivers to be added working with fsl_ssi will not have this property, as they use new ASoC machine driver binding mechanism.
I want to see that binding documented in Documentation/devicetree/bindings first.
On Thu, Mar 08, 2012 at 02:50:21PM -0600, Timur Tabi wrote:
Shawn Guo wrote:
As Documentation/devicetree/bindings/powerpc/fsl/ssi.txt documented, codec-handle is an optional property, so missing it should not be a fatal error.]
It's optional in the sense that if your SSI is not connected to anything, then you don't need to specify the codec handle. If the SSI is connected to a codec, then the SSI node needs to point to it.
It's true for the binding that mpc8610_hpcd and p1022_ds use. But per request from Mark, we need to use the new binding for imx machine drivers, which will have the codec phandle in "audio complex" node.
More importantly, the imx machine drivers to be added working with fsl_ssi will not have this property, as they use new ASoC machine driver binding mechanism.
I want to see that binding documented in Documentation/devicetree/bindings first.
Documentation/devicetree/bindings/sound/imx-audio-sgtl5000.txt, which takes Documentation/devicetree/bindings/sound/tegra-audio-wm8903.txt as the example.
Shawn Guo wrote:
Documentation/devicetree/bindings/sound/imx-audio-sgtl5000.txt, which takes Documentation/devicetree/bindings/sound/tegra-audio-wm8903.txt as the example.
sound { compatible = "fsl,imx51-babbage-sgtl5000", "fsl,imx-audio-sgtl5000"; model = "imx51-babbage-sgtl5000"; ssi-controller = <&ssi1>; audio-codec = <&sgtl5000>; mux-int-port = <1>; mux-ext-port = <3>; };
How does this handle multiple SSIs, each connected to its own codec?
And where's the binding for the DMA controller?
I honestly don't see how this is an improvement over what I came up with.
On Tue, Mar 13, 2012 at 06:23:50PM -0500, Timur Tabi wrote:
sound { compatible = "fsl,imx51-babbage-sgtl5000", "fsl,imx-audio-sgtl5000"; model = "imx51-babbage-sgtl5000"; ssi-controller = <&ssi1>; audio-codec = <&sgtl5000>; mux-int-port = <1>; mux-ext-port = <3>; };
How does this handle multiple SSIs, each connected to its own codec?
With this card those would just be separate CODECs. This is a binding for a simple card.
And where's the binding for the DMA controller?
The DMA controller is transparently instantiated on Tegra systems, there really isn't a meaningfully separate DMA controller - the I2S controller specifies the signal to use. Currently the card takes care of the ASoC level driver stuff but it really should be done by the I2S controller.
I honestly don't see how this is an improvement over what I came up with.
The biggest improvement is that the SoC binding knows nothing about the card binding at all, cards are completely separate and are free to define any binding they choose using any number of on-SoC and off-SoC devices.
Mark Brown wrote:
With this card those would just be separate CODECs. This is a binding for a simple card.
But that is insufficient for the SSI driver, which is expected to handle multiple SSIs. If you're going to document a binding for the SSI driver, then you need to document how to handle multiple SSIs.
The biggest improvement is that the SoC binding knows nothing about the card binding at all, cards are completely separate and are free to define any binding they choose using any number of on-SoC and off-SoC devices.
Ok, I don't understand that at all.
I also don't understand why you insist on perverting my driver to support two incompatible bindings, especially since I still haven't heard an explanation as to what's wrong with the binding that I defined.
On Wed, Mar 14, 2012 at 02:57:00AM +0000, Tabi Timur-B04825 wrote:
Mark Brown wrote:
With this card those would just be separate CODECs. This is a binding for a simple card.
But that is insufficient for the SSI driver, which is expected to handle multiple SSIs. If you're going to document a binding for the SSI driver, then you need to document how to handle multiple SSIs.
For almost all SoC audio ports this works in exactly the same way it works for multiple instances of any other device - you do a binding that covers a single device and then you bind multiple instances of that device in the system each of which need know nothing about any other instances of the IP.
If your SoC has IP which provides more than one port in a single IP block then the driver for that IP would need to register multiple DAIs from a single binding which is again exactly the same thing as you'd do with any other IP block that behaved similarly.
All of which is exactly the same as for any other device and essentially irrelevant to how we define the bindings for the cards.
The biggest improvement is that the SoC binding knows nothing about the card binding at all, cards are completely separate and are free to define any binding they choose using any number of on-SoC and off-SoC devices.
Ok, I don't understand that at all.
Do you have some questions about the above? What is it that you find unclear?
I also don't understand why you insist on perverting my driver to support two incompatible bindings, especially since I still haven't heard an explanation as to what's wrong with the binding that I defined.
We've been through this *repeatedly*, including in the message you're replying to, and we're still going back to square one every time you decide to start talking about device tree again. This is frustrating in the extreme.
Mark Brown wrote:
All of which is exactly the same as for any other device and essentially irrelevant to how we define the bindings for the cards.
So using imx-audio-sgtl5000.txt as an example, you're saying that if I have two SSIs, I should do this in my device tree:
sound1 { compatible = "fsl,imx51-babbage-sgtl5000", "fsl,imx-audio-sgtl5000"; model = "imx51-babbage-sgtl5000"; ssi-controller = <&ssi1>; audio-codec = <&sgtl5000_1>; mux-int-port = <1>; mux-ext-port = <3>; };
sound2 { compatible = "fsl,imx51-babbage-sgtl5000", "fsl,imx-audio-sgtl5000"; model = "imx51-babbage-sgtl5000"; ssi-controller = <&ssi2>; audio-codec = <&sgtl5000_2>; mux-int-port = <1>; mux-ext-port = <3>; };
The biggest improvement is that the SoC binding knows nothing about the card binding at all, cards are completely separate and are free to define any binding they choose using any number of on-SoC and off-SoC devices.
Ok, I don't understand that at all.
Do you have some questions about the above? What is it that you find unclear?
What's the difference between an "SoC binding" and a "card binding"?
We've been through this *repeatedly*, including in the message you're replying to, and we're still going back to square one every time you decide to start talking about device tree again. This is frustrating in the extreme.
I'm sorry you're frustrated, but so am I. I still don't understand why the binding that I invented for the SSI driver isn't good enough for i.MX. The binding and the code was documented and approved a long time ago. Why can't we keep the same binding on i.MX, to keep the code simpler?
On Wed, Mar 14, 2012 at 06:00:41PM -0500, Timur Tabi wrote:
Mark Brown wrote:
All of which is exactly the same as for any other device and essentially irrelevant to how we define the bindings for the cards.
So using imx-audio-sgtl5000.txt as an example, you're saying that if I have two SSIs, I should do this in my device tree:
sound1 { compatible = "fsl,imx51-babbage-sgtl5000", "fsl,imx-audio-sgtl5000"; model = "imx51-babbage-sgtl5000"; ssi-controller = <&ssi1>; audio-codec = <&sgtl5000_1>; mux-int-port = <1>; mux-ext-port = <3>; };
sound2 { compatible = "fsl,imx51-babbage-sgtl5000", "fsl,imx-audio-sgtl5000"; model = "imx51-babbage-sgtl5000"; ssi-controller = <&ssi2>; audio-codec = <&sgtl5000_2>; mux-int-port = <1>; mux-ext-port = <3>; };
Are you putting an example that has two sgtl5000 codecs connected to SSI1 and SSI2 on a particular board?
I have never seen a real case for such setup. Have you actually seen? I do not even see the need to use two SSI controllers for all imx platforms I have seen.
The real case I have seen is imx28 saif driver, which has to use two saif controllers connected to one sgtl5000 to support full-duplex. I would expect something like below for that machine driver.
sound2 { compatible = "fsl,imx28-evk-sgtl5000", "fsl,mxs-audio-sgtl5000"; model = "imx28-evk-sgtl5000"; saif-controller = <&saif1, &saif2>; };
We can have that controller property be a phandle array in the binding.
The biggest improvement is that the SoC binding knows nothing about the card binding at all, cards are completely separate and are free to define any binding they choose using any number of on-SoC and off-SoC devices.
Ok, I don't understand that at all.
Do you have some questions about the above? What is it that you find unclear?
What's the difference between an "SoC binding" and a "card binding"?
I guess "SoC binding" means the binding for fsl_ssi while "card binding" means the binding for imx-sgtl5000 in this particular case.
We've been through this *repeatedly*, including in the message you're replying to, and we're still going back to square one every time you decide to start talking about device tree again. This is frustrating in the extreme.
I'm sorry you're frustrated, but so am I.
And, so am I :(
I thought you have agreed that we can go for new binding for imx machine drivers per the discussion you and Mark had on the first version of the series. That's why I changed to use the new binding in v2, and you even had given your ack tag on v2. Now you are suddenly against that so strongly. I really do not understand why. Can you share your concern regarding to that? What's the problem with imx using new binding exactly?
I still don't understand why the binding that I invented for the SSI driver isn't good enough for i.MX.
I guess Mark had made it clear that having "codec-handle" encoded in ssi node is not as good as what new binding does, having it encoded in "audio complex" node.
The binding and the code was documented and approved a long time ago. Why can't we keep the same binding on i.MX, to keep the code simpler?
I hardly agree with that. The PowerPC binding will require:
1. fsl_ssi's probe function needs to trigger machine driver's probe 2. machine driver needs to find ssi's node to access codec-handle
which actually makes it complexer than the new binding.
Anyway, either way works, and I have tried both. But unfortunately I still have not made everyone happy. Are we going to make the series miss the v3.4 merge window just because of this argument, which does not make much point to me at all.
Shawn Guo wrote:
Are you putting an example that has two sgtl5000 codecs connected to SSI1 and SSI2 on a particular board?
No, this is just a theoretical example.
I have never seen a real case for such setup. Have you actually seen? I do not even see the need to use two SSI controllers for all imx platforms I have seen.
The PowerPC MPC8610 has two SSIs, and I have a hacked up board that has both connected to two CS4270s. This is how I tested multiple SSI support in my driver.
Since my driver supports any number of SSIs, I expect any changes to it to continue that support.
The real case I have seen is imx28 saif driver, which has to use two saif controllers connected to one sgtl5000 to support full-duplex. I would expect something like below for that machine driver.
sound2 { compatible = "fsl,imx28-evk-sgtl5000", "fsl,mxs-audio-sgtl5000"; model = "imx28-evk-sgtl5000"; saif-controller =<&saif1,&saif2>; };
We can have that controller property be a phandle array in the binding.
I can see how this style of binding would make it easier to support an N-to-M mapping of controllers to codecs. However, this is not a concern for the SSI driver.
I thought you have agreed that we can go for new binding for imx machine drivers per the discussion you and Mark had on the first version of the series. That's why I changed to use the new binding in v2, and you even had given your ack tag on v2. Now you are suddenly against that so strongly. I really do not understand why. Can you share your concern regarding to that? What's the problem with imx using new binding exactly?
I admit I may have been too hasty in giving that ACK, because I did not take the time to study the binding. I figured that the new binding was superior to what I came up with years ago.
Now that I've studied the binding, I no longer believe that. I don't understand what's so wonderful about the new binding that my driver has to support it *and* the old binding. I think it would be easier if i.MX uses the original binding.
I still don't understand why the binding that I invented for the SSI driver isn't good enough for i.MX.
I guess Mark had made it clear that having "codec-handle" encoded in ssi node is not as good as what new binding does, having it encoded in "audio complex" node.
Even if the new binding is "better", I do not see how it's SOOOOO much better that my driver has to support it. The drawback in supporting both is the added complexity.
Why can't we keep the same binding on i.MX, to keep the code simpler?
I hardly agree with that. The PowerPC binding will require:
- fsl_ssi's probe function needs to trigger machine driver's probe
- machine driver needs to find ssi's node to access codec-handle
which actually makes it complexer than the new binding.
But the driver already does this! No one's asking you to add the code to perform these tasks. Just use the existing code.
Anyway, either way works, and I have tried both. But unfortunately I still have not made everyone happy. Are we going to make the series miss the v3.4 merge window just because of this argument, which does not make much point to me at all.
I can respect that concern, but at the same time, I still have to push for what I believe makes the most sense.
On Thu, Mar 15, 2012 at 01:37:06PM +0000, Tabi Timur-B04825 wrote: ...
The PowerPC MPC8610 has two SSIs, and I have a hacked up board that has both connected to two CS4270s. This is how I tested multiple SSI support in my driver.
Since my driver supports any number of SSIs, I expect any changes to it to continue that support.
I think it can still easily get supported with the binding like below.
sound { compatible = "fsl,imx51-babbage-sgtl5000", "fsl,imx-audio-sgtl5000"; model = "imx51-babbage-sgtl5000"; ssi-controller = <&ssi1, &ssi2>; audio-codec = <&sgtl5000_1, &sgtl5000_2>; mux-int-port = <1>; mux-ext-port = <3>; };
The only difference will be both ssi-controller and audio-codec become an array of phandles.
...
I admit I may have been too hasty in giving that ACK, because I did not take the time to study the binding. I figured that the new binding was superior to what I came up with years ago.
Now that I've studied the binding, I no longer believe that. I don't understand what's so wonderful about the new binding that my driver has to support it *and* the old binding. I think it would be easier if i.MX uses the original binding.
I do not think it would be easier for imx. Looking at the imx-sgtl5000 binding, you will find we have audmux port configuration encoded there. How would you suggest to get that fit into the PowerPC binding?
...
Even if the new binding is "better", I do not see how it's SOOOOO much better that my driver has to support it. The drawback in supporting both is the added complexity.
If you insist on that the driver should only support single binding, I would rather change mpc8610_hpcd and p1022_ds to use the new binding.
...
But the driver already does this! No one's asking you to add the code to perform these tasks. Just use the existing code.
Again, what's your suggestion to support audmux configuration with the existing binding?
On Thu, Mar 15, 2012 at 10:21 AM, Shawn Guo shawn.guo@linaro.org wrote:
On Thu, Mar 15, 2012 at 01:37:06PM +0000, Tabi Timur-B04825 wrote: ...
The PowerPC MPC8610 has two SSIs, and I have a hacked up board that has both connected to two CS4270s. This is how I tested multiple SSI support in my driver.
Since my driver supports any number of SSIs, I expect any changes to it to continue that support.
I think it can still easily get supported with the binding like below.
sound { compatible = "fsl,imx51-babbage-sgtl5000", "fsl,imx-audio-sgtl5000"; model = "imx51-babbage-sgtl5000"; ssi-controller = <&ssi1, &ssi2>; audio-codec = <&sgtl5000_1, &sgtl5000_2>; mux-int-port = <1>; mux-ext-port = <3>; };
How would you support binding the imx51 ESAI controller?
The only difference will be both ssi-controller and audio-codec become an array of phandles.
...
I admit I may have been too hasty in giving that ACK, because I did not take the time to study the binding. I figured that the new binding was superior to what I came up with years ago.
Now that I've studied the binding, I no longer believe that. I don't understand what's so wonderful about the new binding that my driver has to support it *and* the old binding. I think it would be easier if i.MX uses the original binding.
I do not think it would be easier for imx. Looking at the imx-sgtl5000 binding, you will find we have audmux port configuration encoded there. How would you suggest to get that fit into the PowerPC binding?
...
Even if the new binding is "better", I do not see how it's SOOOOO much better that my driver has to support it. The drawback in supporting both is the added complexity.
If you insist on that the driver should only support single binding, I would rather change mpc8610_hpcd and p1022_ds to use the new binding.
...
But the driver already does this! No one's asking you to add the code to perform these tasks. Just use the existing code.
Again, what's your suggestion to support audmux configuration with the existing binding?
-- Regards, Shawn _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Thu, Mar 15, 2012 at 11:39 AM, Trent Piepho tpiepho@gmail.com wrote:
On Thu, Mar 15, 2012 at 10:21 AM, Shawn Guo shawn.guo@linaro.org wrote:
On Thu, Mar 15, 2012 at 01:37:06PM +0000, Tabi Timur-B04825 wrote: ...
The PowerPC MPC8610 has two SSIs, and I have a hacked up board that has both connected to two CS4270s. This is how I tested multiple SSI support in my driver.
Since my driver supports any number of SSIs, I expect any changes to it to continue that support.
I think it can still easily get supported with the binding like below.
sound { compatible = "fsl,imx51-babbage-sgtl5000", "fsl,imx-audio-sgtl5000"; model = "imx51-babbage-sgtl5000"; ssi-controller = <&ssi1, &ssi2>; audio-codec = <&sgtl5000_1, &sgtl5000_2>; mux-int-port = <1>; mux-ext-port = <3>; };
How would you support binding the imx51 ESAI controller?
Sorry about previous email, laptop touchpad emitted a spurious click, which caused text window in gmail to become deselected, which caused a subsequent enter to send the email rather than creating a new line.
For esai, would you add something like: esai-controller = <&esai1>; esai-audio-codec = <&sgtl5000_3>;
That seems rather ugly to me.
It seems to me in most device bindings where one piece of hardware is attached to another, than the one upstream has in its binding a phandle to the one downstream. But here we have a third binding with two lists of devices and an implied link between devices in corresponding positions in the lists. Does any other device binding work like that?
On Thu, Mar 15, 2012 at 11:57:52AM -0400, Trent Piepho wrote:
which caused text window in gmail to become deselected, which caused a subsequent enter to send the email rather than creating a new line.
For esai, would you add something like: esai-controller = <&esai1>; esai-audio-codec = <&sgtl5000_3>;
That seems rather ugly to me.
None of these examples seem particularly to reflect realistic audio hardware; they're more describing a board with multiple distinct audio subsystems on it than a single card. Remember that nothing at all is fixed about how the card binding works internally, the card driver is free to pick any binding that makes sense for the systems it supports. Shoehorning every possible card into the same binding is definitely a really bad idea.
It seems to me in most device bindings where one piece of hardware is attached to another, than the one upstream has in its binding a phandle to the one downstream. But here we have a third binding with two lists of devices and an implied link between devices in corresponding positions in the lists. Does any other device binding work like that?
There's no upstream and downstream here, really. But like I say none of this looks like realistic hardware anyway.
*Please* go and read the previous discussions of audio bindings for SoCs, while it's more understandable with people who haven't been involved before its still tiresome to have to go back to square one.
Shawn Guo wrote:
sound { compatible = "fsl,imx51-babbage-sgtl5000", "fsl,imx-audio-sgtl5000"; model = "imx51-babbage-sgtl5000"; ssi-controller = <&ssi1, &ssi2>; audio-codec = <&sgtl5000_1, &sgtl5000_2>; mux-int-port = <1>; mux-ext-port = <3>; };
Based on Mark's other email, I would rather see this:
sound1 { compatible = "fsl,imx51-babbage-sgtl5000", "fsl,imx-audio-sgtl5000"; model = "imx51-babbage-sgtl5000"; ssi-controller = <&ssi1>; audio-codec = <&sgtl5000_1>; mux-int-port = <1>; mux-ext-port = <3>; };
sound2 { compatible = "fsl,imx51-babbage-sgtl5000", "fsl,imx-audio-sgtl5000"; model = "imx51-babbage-sgtl5000"; ssi-controller = <&ssi2>; audio-codec = <&sgtl5000_2>; mux-int-port = <1>; mux-ext-port = <3>; };
(ignoring for the moment that I have no idea what the mux-xxx-port values in sound2 should be)
This makes it clear that ssi1 connects only to sgtl5000_1.
If you're okay with this, then please update the documentation accordingly.
On Thu, Mar 15, 2012 at 11:47:05AM -0500, Timur Tabi wrote:
Shawn Guo wrote:
sound { compatible = "fsl,imx51-babbage-sgtl5000", "fsl,imx-audio-sgtl5000"; model = "imx51-babbage-sgtl5000"; ssi-controller = <&ssi1, &ssi2>; audio-codec = <&sgtl5000_1, &sgtl5000_2>; mux-int-port = <1>; mux-ext-port = <3>; };
Based on Mark's other email, I would rather see this:
sound1 { compatible = "fsl,imx51-babbage-sgtl5000", "fsl,imx-audio-sgtl5000"; model = "imx51-babbage-sgtl5000"; ssi-controller = <&ssi1>; audio-codec = <&sgtl5000_1>; mux-int-port = <1>; mux-ext-port = <3>; };
sound2 { compatible = "fsl,imx51-babbage-sgtl5000", "fsl,imx-audio-sgtl5000"; model = "imx51-babbage-sgtl5000"; ssi-controller = <&ssi2>; audio-codec = <&sgtl5000_2>; mux-int-port = <1>; mux-ext-port = <3>; };
(ignoring for the moment that I have no idea what the mux-xxx-port values in sound2 should be)
This makes it clear that ssi1 connects only to sgtl5000_1.
If you're okay with this, then please update the documentation accordingly.
Hmm, Documentation/devicetree/bindings/sound/imx-audio-sgtl5000.txt is the binding document specifically for imx-sgtl5000 driver rather than for all fsl_ssi based machine drivers. Since I have never seen and doubt will ever see the setup that two SSIs connect to two sgtl5000 codecs on any imx board, I do not see much point to update the documentation for that.
If this is indeed a practical setup for PowerPC, it can be documented in the binding for PowerPC machine driver when there is future PowerPC machine drivers adopting the new binding.
Shawn Guo wrote:
Hmm, Documentation/devicetree/bindings/sound/imx-audio-sgtl5000.txt is the binding document specifically for imx-sgtl5000 driver rather than for all fsl_ssi based machine drivers. Since I have never seen and doubt will ever see the setup that two SSIs connect to two sgtl5000 codecs on any imx board, I do not see much point to update the documentation for that.
If this is indeed a practical setup for PowerPC, it can be documented in the binding for PowerPC machine driver when there is future PowerPC machine drivers adopting the new binding.
Ok. I was going to say that I'd like to see a template that applies to all SOCs with an SSI, but I guess that won't be that useful.
I'm just concerned that if there's no standard way to create bindings for machine drivers, we're going to end up with some kind of mess down the road.
On Fri, Mar 16, 2012 at 01:55:01AM +0000, Tabi Timur-B04825 wrote:
I'm just concerned that if there's no standard way to create bindings for machine drivers, we're going to end up with some kind of mess down the road.
I think that we'll be able to deal with that by adding helpers for common cases (as has already started to happen with some of the nVidia inspired changes like the auto_nc_pins() stuff) - approach the problem by extracting the good practice and putting it into helpers to encourage use. Fortunately machine drivers are by their nature system specific so so if they are messed up they impact a limited subset of systems.
On Wed, Mar 14, 2012 at 06:00:41PM -0500, Timur Tabi wrote:
Mark Brown wrote:
So using imx-audio-sgtl5000.txt as an example, you're saying that if I have two SSIs, I should do this in my device tree:
sound1 { };
sound2 { };
That's a totally sensible option if you've got two unrelated audio subsystems on your board for some reason. If you've got a system where that's not applicable and the two devices are related in some way then you'd define a new sound card binding that reflects that.
The biggest improvement is that the SoC binding knows nothing about the card binding at all, cards are completely separate and are free to define any binding they choose using any number of on-SoC and off-SoC devices.
Ok, I don't understand that at all.
Do you have some questions about the above? What is it that you find unclear?
What's the difference between an "SoC binding" and a "card binding"?
The SoC is the bit of silicon with the CPU and other devices on it like the DMA controller and SSI or whatever ports. The card is the PCB this has been soldered down onto.
We've been through this *repeatedly*, including in the message you're replying to, and we're still going back to square one every time you decide to start talking about device tree again. This is frustrating in the extreme.
I'm sorry you're frustrated, but so am I. I still don't understand why the binding that I invented for the SSI driver isn't good enough for i.MX. The binding and the code was documented and approved a long time ago. Why can't we keep the same binding on i.MX, to keep the code simpler?
The problem with your binding has always been, and continues to be, that it's based on the idea that there's one CODEC per SSI and that the CODEC is a simple appendage of that SSI. There's no real real binding for the machine driver, it's just silently created by a single SSI port. Really there's nothing new with this issue, there's always been this absence of a representation for the machine distinct from the individual devices it is built up from.
This means that when you get systems which have auxiliary devices (like most of the Wolfson reference boards which have a power amplifier for the sub speaker not connected to an SSI port) or that need to set up things like complex accessory detection mechanisms there's no real place in your binding to support those systems. Things like the configuration of the input and output connections which the nVidia WM8903 drivers are able to do over multiple boards are another example.
On Thu, Mar 15, 2012 at 02:27:03PM +0000, Mark Brown wrote:
On Wed, Mar 14, 2012 at 06:00:41PM -0500, Timur Tabi wrote:
...
I'm sorry you're frustrated, but so am I. I still don't understand why the binding that I invented for the SSI driver isn't good enough for i.MX. The binding and the code was documented and approved a long time ago. Why can't we keep the same binding on i.MX, to keep the code simpler?
The problem with your binding has always been, and continues to be, that it's based on the idea that there's one CODEC per SSI and that the CODEC is a simple appendage of that SSI. There's no real real binding for the machine driver, it's just silently created by a single SSI port. Really there's nothing new with this issue, there's always been this absence of a representation for the machine distinct from the individual devices it is built up from.
This means that when you get systems which have auxiliary devices (like most of the Wolfson reference boards which have a power amplifier for the sub speaker not connected to an SSI port) or that need to set up things like complex accessory detection mechanisms there's no real place in your binding to support those systems. Things like the configuration of the input and output connections which the nVidia WM8903 drivers are able to do over multiple boards are another example.
Yes, we will have the exactly same thing for imx-sgtl5000 to be added later. The current imx-sgtl5000 driver just an initial support.
Mark Brown wrote:
On Wed, Mar 14, 2012 at 06:00:41PM -0500, Timur Tabi wrote:
Mark Brown wrote:
So using imx-audio-sgtl5000.txt as an example, you're saying that if I have two SSIs, I should do this in my device tree:
sound1 { };
sound2 { };
That's a totally sensible option if you've got two unrelated audio subsystems on your board for some reason. If you've got a system where that's not applicable and the two devices are related in some way then you'd define a new sound card binding that reflects that.
Well, the two SSIs are related only in that they're the same kind of device. But SSI1 connected to CS4270_1 is completely independent from SSI2 connected to CS4270_2.
So I guess that means that we'd have two soundX {} top-level nodes.
The SoC is the bit of silicon with the CPU and other devices on it like the DMA controller and SSI or whatever ports. The card is the PCB this has been soldered down onto.
Well, I asked about the difference between the soc BINDING and the card BINDING. On PowerPC, at least, there's no distinct binding for either. For example, I2C devices are on the PCB, but they're listed as child nodes of the I2C controller, which is on the SoC.
The problem with your binding has always been, and continues to be, that it's based on the idea that there's one CODEC per SSI and that the CODEC is a simple appendage of that SSI. There's no real real binding for the machine driver, it's just silently created by a single SSI port. Really there's nothing new with this issue, there's always been this absence of a representation for the machine distinct from the individual devices it is built up from.
This means that when you get systems which have auxiliary devices (like most of the Wolfson reference boards which have a power amplifier for the sub speaker not connected to an SSI port) or that need to set up things like complex accessory detection mechanisms there's no real place in your binding to support those systems. Things like the configuration of the input and output connections which the nVidia WM8903 drivers are able to do over multiple boards are another example.
Ok, fair enough. I understand that my binding has no way of specifying board-specific properties, or anything that isn't directly related to the SSI, codec, or DMA controller.
However, I don't like the way this is being represented as a PowerPC vs. ARM issue, because that's just not correct. It's an "old binding" vs "new binding" issue. For example:
+ /* + * In case of imx, the machine driver uses new binding which does + * not require SSI driver to trigger machine driver's probe, but + * the pcm device needs to be registered here. + */ + if (ssi_private->ssi_on_imx) { + ssi_private->imx_pcm_pdev = + platform_device_register_simple("imx-pcm-audio",
This prohibits me from using the new binding on any future PowerPC parts, because it clearly says "iMX" on everything.
On Thu, Mar 15, 2012 at 11:44:16AM -0500, Timur Tabi wrote:
Mark Brown wrote:
The SoC is the bit of silicon with the CPU and other devices on it like the DMA controller and SSI or whatever ports. The card is the PCB this has been soldered down onto.
Well, I asked about the difference between the soc BINDING and the card BINDING. On PowerPC, at least, there's no distinct binding for either. For example, I2C devices are on the PCB, but they're listed as child nodes of the I2C controller, which is on the SoC.
Right, but this is the problem in a nutshell - there's nowhere to put the board binding so the SoC code has to know about and do it.
However, I don't like the way this is being represented as a PowerPC vs. ARM issue, because that's just not correct. It's an "old binding" vs "new binding" issue. For example:
I think that's just creeping in because the old binding is the existing practice on PowerPC.
- /*
* In case of imx, the machine driver uses new binding which does* not require SSI driver to trigger machine driver's probe, but* the pcm device needs to be registered here.*/- if (ssi_private->ssi_on_imx) {
ssi_private->imx_pcm_pdev =platform_device_register_simple("imx-pcm-audio",
This prohibits me from using the new binding on any future PowerPC parts, because it clearly says "iMX" on everything.
So long as it's internal to the code (which the above is) I don't think it really matters, the code can be changed later. We need to do something about DMA controllers in general, and there was some recent discussion on the DT list about a generic binding for them, so leaving the code as-is with nothing in the DT itself seems like a good place to leave things initially.
On Thu, Mar 15, 2012 at 11:44:16AM -0500, Timur Tabi wrote: ...
Ok, fair enough. I understand that my binding has no way of specifying board-specific properties, or anything that isn't directly related to the SSI, codec, or DMA controller.
However, I don't like the way this is being represented as a PowerPC vs. ARM issue, because that's just not correct. It's an "old binding" vs "new binding" issue. For example:
- /*
* In case of imx, the machine driver uses new binding which does* not require SSI driver to trigger machine driver's probe, but* the pcm device needs to be registered here.*/- if (ssi_private->ssi_on_imx) {
This could probably change to check whether property codec-handle is present under SSI node to decide if it runs with "old binding" or "new binding".
ssi_private->imx_pcm_pdev =platform_device_register_simple("imx-pcm-audio",
This is still IMX vs. PowerPC thing rather than "old binding" vs. "new binding" one.
This prohibits me from using the new binding on any future PowerPC parts, because it clearly says "iMX" on everything.
I think we can change it later when the future PowerPC parts come to use the new binding. In any case, I do not see this is an issue that should block the whole series.
Shawn Guo wrote:
This could probably change to check whether property codec-handle is present under SSI node to decide if it runs with "old binding" or "new binding".
I guess that's how we need to do it. The SSI driver should not be looking for /sound node, but that's the most reliable way to know if we have a new or old binding.
ssi_private->imx_pcm_pdev =platform_device_register_simple("imx-pcm-audio",This is still IMX vs. PowerPC thing rather than "old binding" vs. "new binding" one.
What is "imx-pcm-audio"?
This prohibits me from using the new binding on any future PowerPC parts, because it clearly says "iMX" on everything.
I think we can change it later when the future PowerPC parts come to use the new binding. In any case, I do not see this is an issue that should block the whole series.
At least get rid of any unnecessary references to "imx".
On Fri, Mar 16, 2012 at 02:07:42AM +0000, Tabi Timur-B04825 wrote: ...
ssi_private->imx_pcm_pdev =platform_device_register_simple("imx-pcm-audio",This is still IMX vs. PowerPC thing rather than "old binding" vs. "new binding" one.
What is "imx-pcm-audio"?
Something like "fsl-pcm-audio", but different from "fsl-pcm-audio" will be instantiated as a hardware block by device tree, "imx-pcm-audio" is pretty much like a ASoC wrapper for calling dmaengine driver. So there is no hardware block presenting this "device" in device tree, and we need to instantiate it from cpu DAI driver as Mark suggested.
Shawn Guo wrote:
Something like "fsl-pcm-audio", but different from "fsl-pcm-audio" will be instantiated as a hardware block by device tree, "imx-pcm-audio" is pretty much like a ASoC wrapper for calling dmaengine driver. So there is no hardware block presenting this "device" in device tree, and we need to instantiate it from cpu DAI driver as Mark suggested.
Wait, is this the DMA controller? Because on PowerPC, we have device tree nodes for the DMA controller. Why don't you have one on i.MX?
On Fri, Mar 16, 2012 at 03:44:43AM +0000, Tabi Timur-B04825 wrote:
Shawn Guo wrote:
Something like "fsl-pcm-audio", but different from "fsl-pcm-audio" will be instantiated as a hardware block by device tree, "imx-pcm-audio" is pretty much like a ASoC wrapper for calling dmaengine driver. So there is no hardware block presenting this "device" in device tree, and we need to instantiate it from cpu DAI driver as Mark suggested.
Wait, is this the DMA controller? Because on PowerPC, we have device tree nodes for the DMA controller. Why don't you have one on i.MX?
We do have a DMA controller node for IMX.
sdma@83fb0000 { compatible = "fsl,imx51-sdma", "fsl,imx35-sdma"; reg = <0x83fb0000 0x4000>; interrupts = <6>; };
But it's instantiated as a dmaengine device backed by dmaengine driver drivers/dma/imx-dma.c. "imx-pcm-audio" will just be an ASoC dmaengine client using dmaengine API.
Shawn Guo wrote:
But it's instantiated as a dmaengine device backed by dmaengine driver drivers/dma/imx-dma.c. "imx-pcm-audio" will just be an ASoC dmaengine client using dmaengine API.
So you don't have the equivalent of sound/soc/fsl_dma.c? Don't you need to program the DMA controller specifically for audio? There's no way I could use drivers/dma/fsldma.c for audio.
On Fri, Mar 16, 2012 at 04:08:32AM +0000, Tabi Timur-B04825 wrote:
Shawn Guo wrote:
But it's instantiated as a dmaengine device backed by dmaengine driver drivers/dma/imx-dma.c. "imx-pcm-audio" will just be an ASoC dmaengine client using dmaengine API.
So you don't have the equivalent of sound/soc/fsl_dma.c? Don't you need to program the DMA controller specifically for audio? There's no way I could use drivers/dma/fsldma.c for audio.
I need to program the DMA controller for audio. But that's done through generic dmaengine API with audio specific parameters passed in. That's pretty much what all snd_imx_pcm_hw_params (sound/soc/fsl/imx-pcm-dma.c) does there.
Shawn Guo wrote:
I need to program the DMA controller for audio. But that's done through generic dmaengine API with audio specific parameters passed in. That's pretty much what all snd_imx_pcm_hw_params (sound/soc/fsl/imx-pcm-dma.c) does there.
Interesting. I'll have to check that out.
On Fri, Mar 16, 2012 at 02:07:42AM +0000, Tabi Timur-B04825 wrote:
Shawn Guo wrote:
This could probably change to check whether property codec-handle is present under SSI node to decide if it runs with "old binding" or "new binding".
I guess that's how we need to do it. The SSI driver should not be looking for /sound node, but that's the most reliable way to know if we have a new or old binding.
...
This prohibits me from using the new binding on any future PowerPC parts, because it clearly says "iMX" on everything.
I think we can change it later when the future PowerPC parts come to use the new binding. In any case, I do not see this is an issue that should block the whole series.
At least get rid of any unnecessary references to "imx".
So something like this?
/* * If codec-handle property is missing from SSI node, we assume * that the machine driver uses new binding which does not require * SSI driver to trigger machine driver's probe. */ if (!of_get_property(np, "codec-handle", NULL)) { if (ssi_private->ssi_on_imx) { ssi_private->imx_pcm_pdev = platform_device_register_simple("imx-pcm-audio", -1, NULL, 0); if (IS_ERR(ssi_private->imx_pcm_pdev)) { ret = PTR_ERR(ssi_private->imx_pcm_pdev); goto error_dev; } } /* success for new binding case */ return 0; }
It does not reduce any reference to "imx" actually. If you think it's worth another iteration of the series, I will post v5 for it.
Shawn Guo wrote:
So something like this?
/* * If codec-handle property is missing from SSI node, we assume * that the machine driver uses new binding which does not require * SSI driver to trigger machine driver's probe. */ if (!of_get_property(np, "codec-handle", NULL)) { if (ssi_private->ssi_on_imx) { ssi_private->imx_pcm_pdev = platform_device_register_simple("imx-pcm-audio", -1, NULL, 0); if (IS_ERR(ssi_private->imx_pcm_pdev)) { ret = PTR_ERR(ssi_private->imx_pcm_pdev); goto error_dev; } } /* success for new binding case */ return 0; }It does not reduce any reference to "imx" actually. If you think it's worth another iteration of the series, I will post v5 for it.
I had something more like this in mind:
/* * If codec-handle property is missing from SSI node, we assume * that the machine driver uses new binding which does not require * SSI driver to trigger machine driver's probe. */ if (!of_get_property(np, "codec-handle", NULL)) ssi_private->new_binding = true;
if (ssi_private->ssi_on_imx) { ssi_private->imx_pcm_pdev = platform_device_register_simple("imx-pcm-audio", -1, NULL, 0); if (IS_ERR(ssi_private->imx_pcm_pdev)) { ret = PTR_ERR(ssi_private->imx_pcm_pdev); goto error_dev; } } /* success for new binding case */ return 0; }
Well, it's not perfect, but the idea is that we keep track of new vs. old binding separately from imx vs. powerpc. Although I'm thinking there might be a way to general the call to platform_device_register_simple. Maybe we could put "imx-pcm-audio" in the device tree?
On Fri, Mar 16, 2012 at 03:53:39AM +0000, Tabi Timur-B04825 wrote:
Shawn Guo wrote:
So something like this?
/* * If codec-handle property is missing from SSI node, we assume * that the machine driver uses new binding which does not require * SSI driver to trigger machine driver's probe. */ if (!of_get_property(np, "codec-handle", NULL)) { if (ssi_private->ssi_on_imx) { ssi_private->imx_pcm_pdev = platform_device_register_simple("imx-pcm-audio", -1, NULL, 0); if (IS_ERR(ssi_private->imx_pcm_pdev)) { ret = PTR_ERR(ssi_private->imx_pcm_pdev); goto error_dev; } } /* success for new binding case */ return 0; }It does not reduce any reference to "imx" actually. If you think it's worth another iteration of the series, I will post v5 for it.
I had something more like this in mind:
/* * If codec-handle property is missing from SSI node, we assume * that the machine driver uses new binding which does not require * SSI driver to trigger machine driver's probe. */ if (!of_get_property(np, "codec-handle", NULL)) ssi_private->new_binding = true;if (ssi_private->ssi_on_imx) { ssi_private->imx_pcm_pdev = platform_device_register_simple("imx-pcm-audio", -1, NULL, 0); if (IS_ERR(ssi_private->imx_pcm_pdev)) { ret = PTR_ERR(ssi_private->imx_pcm_pdev); goto error_dev; } } /* success for new binding case */ return 0; }
Well, it's not perfect, but the idea is that we keep track of new vs. old binding separately from imx vs. powerpc.
Fine with me.
Although I'm thinking there might be a way to general the call to platform_device_register_simple. Maybe we could put "imx-pcm-audio" in the device tree?
It seems a little bit abuse of device tree to me. My first choice was to do that in imx-sgtl5000 machine driver. But Mark disagrees.
On Fri, Mar 16, 2012 at 03:53:39AM +0000, Tabi Timur-B04825 wrote:
might be a way to general the call to platform_device_register_simple. Maybe we could put "imx-pcm-audio" in the device tree?
We might end up doing that but as this adaption of the DMA controller to the audio stack isn't really hardware so much as an implementation detail of the audio stack it'd be nice to come up with some neater way of doing it, ideally not involving the platform device at all.
I'm starting to think we should have a framework thing that lets DAI drivers say they're a combined DMA/DAI driver for cases like this where there is a generic DMA device that's being shared by lots of different subsystems. It's a common situation and it makes the device registration clearer. Adding Stephen Warren as he was thinking about this stuff in the context of the nVidia drivers.
On Thu, Mar 08, 2012 at 02:50:21PM -0600, Timur Tabi wrote:
Shawn Guo wrote:
More importantly, the imx machine drivers to be added working with fsl_ssi will not have this property, as they use new ASoC machine driver binding mechanism.
I want to see that binding documented in Documentation/devicetree/bindings first.
That binding will generally be per machine driver.
Makes necessary changes on fsl_ssi to let it work with imx pcm and machine drivers.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/fsl/fsl_ssi.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 67 insertions(+), 0 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index e5903cb..c251ee9 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -29,6 +29,7 @@ #include <sound/soc.h>
#include "fsl_ssi.h" +#include "imx-pcm.h"
#ifdef PPC #define read_ssi(addr) in_be32(addr) @@ -119,6 +120,11 @@ struct fsl_ssi_private { struct device_attribute dev_attr; struct platform_device *pdev;
+ int ssi_on_imx; + struct platform_device *imx_pcm_pdev; + struct imx_pcm_dma_params dma_params_tx; + struct imx_pcm_dma_params dma_params_rx; + struct { unsigned int rfrc; unsigned int tfrc; @@ -416,6 +422,12 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, ssi_private->second_stream = substream; }
+ if (ssi_private->ssi_on_imx) + snd_soc_dai_set_dma_data(dai, substream, + (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) ? + &ssi_private->dma_params_tx : + &ssi_private->dma_params_rx); + return 0; }
@@ -709,6 +721,38 @@ static int __devinit fsl_ssi_probe(struct platform_device *pdev) /* Older 8610 DTs didn't have the fifo-depth property */ ssi_private->fifo_depth = 8;
+ if (of_device_is_compatible(pdev->dev.of_node, "fsl,imx1-ssi")) { + u32 dma_events[2]; + ssi_private->ssi_on_imx = 1; + /* + * FIXME: The dma burst size must equal to ssi watermark + * setting on imx. We have burstsize be "fifo_depth - 2" + * here because "fifo_depth - 2" rather than fifo_depth is + * being written into watermark register in fsl_ssi_startup(). + * Is this something needs to be fixed for PowerPC? + */ + ssi_private->dma_params_tx.burstsize = + ssi_private->fifo_depth - 2; + ssi_private->dma_params_rx.burstsize = + ssi_private->fifo_depth - 2; + ssi_private->dma_params_tx.dma_addr = + ssi_private->ssi_phys + offsetof(struct ccsr_ssi, stx0); + ssi_private->dma_params_rx.dma_addr = + ssi_private->ssi_phys + offsetof(struct ccsr_ssi, srx0); + /* + * TODO: This is a temporary solution and should be changed + * to use generic DMA binding later when the helplers get in. + */ + ret = of_property_read_u32_array(pdev->dev.of_node, + "fsl,ssi-dma-events", dma_events, 2); + if (ret) { + dev_err(&pdev->dev, "could not get dma events\n"); + goto error_irq; + } + ssi_private->dma_params_tx.dma = dma_events[0]; + ssi_private->dma_params_rx.dma = dma_events[1]; + } + /* Initialize the the device_attribute structure */ dev_attr = &ssi_private->dev_attr; sysfs_attr_init(&dev_attr->attr); @@ -732,6 +776,24 @@ static int __devinit fsl_ssi_probe(struct platform_device *pdev) goto error_dev; }
+ /* + * In case of imx, the machine driver uses new binding which does + * not require SSI driver to trigger machine driver's probe, but + * the pcm device needs to be registered here. + */ + if (ssi_private->ssi_on_imx) { + ssi_private->imx_pcm_pdev = + platform_device_register_simple("imx-pcm-audio", + -1, NULL, 0); + if (IS_ERR(ssi_private->imx_pcm_pdev)) { + ret = PTR_ERR(ssi_private->imx_pcm_pdev); + goto error_dev; + } else { + /* success for imx */ + return 0; + } + } + /* Trigger the machine driver's probe function. The platform driver * name of the machine driver is taken from /compatible property of the * device tree. We also pass the address of the CPU DAI driver @@ -757,6 +819,8 @@ static int __devinit fsl_ssi_probe(struct platform_device *pdev)
error_dai: snd_soc_unregister_dai(&pdev->dev); + if (ssi_private->ssi_on_imx) + platform_device_unregister(ssi_private->imx_pcm_pdev);
error_dev: dev_set_drvdata(&pdev->dev, NULL); @@ -783,6 +847,8 @@ static int fsl_ssi_remove(struct platform_device *pdev)
platform_device_unregister(ssi_private->pdev); snd_soc_unregister_dai(&pdev->dev); + if (ssi_private->ssi_on_imx) + platform_device_unregister(ssi_private->imx_pcm_pdev); device_remove_file(&pdev->dev, &ssi_private->dev_attr);
free_irq(ssi_private->irq, ssi_private); @@ -796,6 +862,7 @@ static int fsl_ssi_remove(struct platform_device *pdev)
static const struct of_device_id fsl_ssi_ids[] = { { .compatible = "fsl,mpc8610-ssi", }, + { .compatible = "fsl,imx1-ssi", }, {} }; MODULE_DEVICE_TABLE(of, fsl_ssi_ids);
On Fri, Mar 09, 2012 at 12:59:49AM +0800, Shawn Guo wrote:
Makes necessary changes on fsl_ssi to let it work with imx pcm and machine drivers.
Signed-off-by: Shawn Guo shawn.guo@linaro.org
sound/soc/fsl/fsl_ssi.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 67 insertions(+), 0 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index e5903cb..c251ee9 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -29,6 +29,7 @@ #include <sound/soc.h>
#include "fsl_ssi.h" +#include "imx-pcm.h"
#ifdef PPC #define read_ssi(addr) in_be32(addr) @@ -119,6 +120,11 @@ struct fsl_ssi_private { struct device_attribute dev_attr; struct platform_device *pdev;
- int ssi_on_imx;
- struct platform_device *imx_pcm_pdev;
- struct imx_pcm_dma_params dma_params_tx;
- struct imx_pcm_dma_params dma_params_rx;
- struct { unsigned int rfrc; unsigned int tfrc;
@@ -416,6 +422,12 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, ssi_private->second_stream = substream; }
- if (ssi_private->ssi_on_imx)
snd_soc_dai_set_dma_data(dai, substream,(substream->stream == SNDRV_PCM_STREAM_PLAYBACK) ?&ssi_private->dma_params_tx :&ssi_private->dma_params_rx);- return 0;
}
@@ -709,6 +721,38 @@ static int __devinit fsl_ssi_probe(struct platform_device *pdev) /* Older 8610 DTs didn't have the fifo-depth property */ ssi_private->fifo_depth = 8;
- if (of_device_is_compatible(pdev->dev.of_node, "fsl,imx1-ssi")) {
u32 dma_events[2];ssi_private->ssi_on_imx = 1;/** FIXME: The dma burst size must equal to ssi watermark* setting on imx. We have burstsize be "fifo_depth - 2"* here because "fifo_depth - 2" rather than fifo_depth is* being written into watermark register in fsl_ssi_startup().* Is this something needs to be fixed for PowerPC?*/ssi_private->dma_params_tx.burstsize =ssi_private->fifo_depth - 2;ssi_private->dma_params_rx.burstsize =ssi_private->fifo_depth - 2;ssi_private->dma_params_tx.dma_addr =ssi_private->ssi_phys + offsetof(struct ccsr_ssi, stx0);ssi_private->dma_params_rx.dma_addr =ssi_private->ssi_phys + offsetof(struct ccsr_ssi, srx0);/** TODO: This is a temporary solution and should be changed* to use generic DMA binding later when the helplers get in.*/ret = of_property_read_u32_array(pdev->dev.of_node,"fsl,ssi-dma-events", dma_events, 2);if (ret) {dev_err(&pdev->dev, "could not get dma events\n");goto error_irq;}ssi_private->dma_params_tx.dma = dma_events[0];ssi_private->dma_params_rx.dma = dma_events[1];- }
- /* Initialize the the device_attribute structure */ dev_attr = &ssi_private->dev_attr; sysfs_attr_init(&dev_attr->attr);
@@ -732,6 +776,24 @@ static int __devinit fsl_ssi_probe(struct platform_device *pdev) goto error_dev; }
- /*
* In case of imx, the machine driver uses new binding which does* not require SSI driver to trigger machine driver's probe, but* the pcm device needs to be registered here.*/- if (ssi_private->ssi_on_imx) {
ssi_private->imx_pcm_pdev =platform_device_register_simple("imx-pcm-audio",-1, NULL, 0);if (IS_ERR(ssi_private->imx_pcm_pdev)) {ret = PTR_ERR(ssi_private->imx_pcm_pdev);goto error_dev;} else {/* success for imx */return 0;}- }
- /* Trigger the machine driver's probe function. The platform driver
- name of the machine driver is taken from /compatible property of the
- device tree. We also pass the address of the CPU DAI driver
@@ -757,6 +819,8 @@ static int __devinit fsl_ssi_probe(struct platform_device *pdev)
error_dai: snd_soc_unregister_dai(&pdev->dev);
- if (ssi_private->ssi_on_imx)
platform_device_unregister(ssi_private->imx_pcm_pdev);error_dev: dev_set_drvdata(&pdev->dev, NULL); @@ -783,6 +847,8 @@ static int fsl_ssi_remove(struct platform_device *pdev)
platform_device_unregister(ssi_private->pdev); snd_soc_unregister_dai(&pdev->dev);
if (ssi_private->ssi_on_imx)
platform_device_unregister(ssi_private->imx_pcm_pdev);device_remove_file(&pdev->dev, &ssi_private->dev_attr);
free_irq(ssi_private->irq, ssi_private);
@@ -796,6 +862,7 @@ static int fsl_ssi_remove(struct platform_device *pdev)
static const struct of_device_id fsl_ssi_ids[] = { { .compatible = "fsl,mpc8610-ssi", },
- { .compatible = "fsl,imx1-ssi", },
imx1 seems wrong here. The register layout has changed significantly between i.MX1 and i.MX21.
Sascha
On Thu, Mar 08, 2012 at 08:15:26PM +0100, Sascha Hauer wrote: ...
@@ -796,6 +862,7 @@ static int fsl_ssi_remove(struct platform_device *pdev)
static const struct of_device_id fsl_ssi_ids[] = { { .compatible = "fsl,mpc8610-ssi", },
- { .compatible = "fsl,imx1-ssi", },
imx1 seems wrong here. The register layout has changed significantly between i.MX1 and i.MX21.
Yeah, that's right. You indeed know i.MX1 much better than me. Will change to:
.compatible = "fsl,imx21-ssi"
Shawn Guo wrote:
- if (of_device_is_compatible(pdev->dev.of_node, "fsl,imx1-ssi")) {
I wonder if the i.MX code should be protected by an #ifdef. There's no point in compiling this code on PowerPC.
/** FIXME: The dma burst size must equal to ssi watermark* setting on imx. We have burstsize be "fifo_depth - 2"* here because "fifo_depth - 2" rather than fifo_depth is* being written into watermark register in fsl_ssi_startup().* Is this something needs to be fixed for PowerPC?
Setting the watermark to fifo_depth-2 allows the SSI to continue storing data into the FIFO (during capture) in the case the DMA engine isn't fast enough. I don't want the FIFO to be completely full before we start the DMA.
For playback, it's the same thing. I can't be sure the FIFO is completely empty when it's time to DMA more data.
*/ssi_private->dma_params_tx.burstsize =ssi_private->fifo_depth - 2;ssi_private->dma_params_rx.burstsize =ssi_private->fifo_depth - 2;ssi_private->dma_params_tx.dma_addr =ssi_private->ssi_phys + offsetof(struct ccsr_ssi, stx0);ssi_private->dma_params_rx.dma_addr =ssi_private->ssi_phys + offsetof(struct ccsr_ssi, srx0);
So I'm a little confused by this new binding. I went through a lot of work to remove DMA-specific data from the SSI driver, and here you are adding a bunch of it back. What you're doing here -- I'm doing it in the DMA driver where it belongs.
/** TODO: This is a temporary solution and should be changed* to use generic DMA binding later when the helplers get in.*/ret = of_property_read_u32_array(pdev->dev.of_node,"fsl,ssi-dma-events", dma_events, 2);if (ret) {dev_err(&pdev->dev, "could not get dma events\n");goto error_irq;}ssi_private->dma_params_tx.dma = dma_events[0];ssi_private->dma_params_rx.dma = dma_events[1];
of_property_read_u32_array seems overkill for just two integers.
ssi_private->dma_params_tx.dma = be32_to_cpu(prop[0]); ssi_private->dma_params_rx.dma = be32_to_cpu(prop[1]);
On Thu, Mar 08, 2012 at 02:45:29PM -0600, Timur Tabi wrote:
Shawn Guo wrote:
- if (of_device_is_compatible(pdev->dev.of_node, "fsl,imx1-ssi")) {
I wonder if the i.MX code should be protected by an #ifdef. There's no point in compiling this code on PowerPC.
I'm not really fond of #ifdef. The i.MX code is not that much after all.
/** FIXME: The dma burst size must equal to ssi watermark* setting on imx. We have burstsize be "fifo_depth - 2"* here because "fifo_depth - 2" rather than fifo_depth is* being written into watermark register in fsl_ssi_startup().* Is this something needs to be fixed for PowerPC?Setting the watermark to fifo_depth-2 allows the SSI to continue storing data into the FIFO (during capture) in the case the DMA engine isn't fast enough. I don't want the FIFO to be completely full before we start the DMA.
For playback, it's the same thing. I can't be sure the FIFO is completely empty when it's time to DMA more data.
Ok, got you. We used to set fifo_depth on imx-ssi not the value given by hardware manual, but the one already tuned.
So all we need is to encode the hardware value in device tree property "fsl,fifo-depth", and it just works. I will remove this "FIXME".
*/ssi_private->dma_params_tx.burstsize =ssi_private->fifo_depth - 2;ssi_private->dma_params_rx.burstsize =ssi_private->fifo_depth - 2;ssi_private->dma_params_tx.dma_addr =ssi_private->ssi_phys + offsetof(struct ccsr_ssi, stx0);ssi_private->dma_params_rx.dma_addr =ssi_private->ssi_phys + offsetof(struct ccsr_ssi, srx0);So I'm a little confused by this new binding. I went through a lot of work to remove DMA-specific data from the SSI driver, and here you are adding a bunch of it back. What you're doing here -- I'm doing it in the DMA driver where it belongs.
I'm not sure these all belong to DMA driver. These are all configurations/parameters of SSI, which are related to DMA though. I see fsl_dma are handling these, but I do not fully agree with that, because we end up with fsl_dma driver accessing SSI node and "struct ccsr_ssi" which should be SSI private data.
Secondly, this is the way that how imx-ssi and imx-pcm-dma works and interacts. If we want to move these stuff into imx-pcm-dma, we need a good story to convince imx-ssi users that why it's better than what they currently do.
/** TODO: This is a temporary solution and should be changed* to use generic DMA binding later when the helplers get in.*/ret = of_property_read_u32_array(pdev->dev.of_node,"fsl,ssi-dma-events", dma_events, 2);if (ret) {dev_err(&pdev->dev, "could not get dma events\n");goto error_irq;}ssi_private->dma_params_tx.dma = dma_events[0];ssi_private->dma_params_rx.dma = dma_events[1];of_property_read_u32_array seems overkill for just two integers.
Hmm, I do not think so.
Device tree maintainers created helpers of_property_read_u32 and of_property_read_u32_array to ease the user and make the operation less error prone. Looking at the of_property_read_u32 implementation, it's even calling into of_property_read_u32_array. So I do not think it's overkill for reading two integers in any way.
static inline int of_property_read_u32(const struct device_node *np, const char *propname, u32 *out_value) { return of_property_read_u32_array(np, propname, out_value, 1); }
ssi_private->dma_params_tx.dma = be32_to_cpu(prop[0]); ssi_private->dma_params_rx.dma = be32_to_cpu(prop[1]);
Shawn Guo wrote:
Ok, got you. We used to set fifo_depth on imx-ssi not the value given by hardware manual, but the one already tuned.
What do you mean by "already tuned". Tuned by whom?
*/ssi_private->dma_params_tx.burstsize =ssi_private->fifo_depth - 2;ssi_private->dma_params_rx.burstsize =ssi_private->fifo_depth - 2;ssi_private->dma_params_tx.dma_addr =ssi_private->ssi_phys + offsetof(struct ccsr_ssi, stx0);ssi_private->dma_params_rx.dma_addr =ssi_private->ssi_phys + offsetof(struct ccsr_ssi, srx0);So I'm a little confused by this new binding. I went through a lot of work to remove DMA-specific data from the SSI driver, and here you are adding a bunch of it back. What you're doing here -- I'm doing it in the DMA driver where it belongs.
I'm not sure these all belong to DMA driver. These are all configurations/parameters of SSI, which are related to DMA though. I see fsl_dma are handling these, but I do not fully agree with that, because we end up with fsl_dma driver accessing SSI node and "struct ccsr_ssi" which should be SSI private data.
Well, the SSI needs to tell the DMA driver how to program itself. On PowerPC, the DMA's burst size is based on the SSI's FIFO depth. You're effectively doing what I already do, except you do more work in the SSI driver.
I understand that we need to support old and new bindings, but I already created an infrastructure that supports passing SSI information to the DMA driver. It seems that the above code is different only for the sake of being different, not because it's better.
This is not really an i.MX vs. PowerPC issue, which is why I'm not sure this change belong in this patchset.
Secondly, this is the way that how imx-ssi and imx-pcm-dma works and interacts. If we want to move these stuff into imx-pcm-dma, we need a good story to convince imx-ssi users that why it's better than what they currently do.
I don't understand that. You're already rewriting these drivers. Who else needs to be convinced?
On Fri, Mar 09, 2012 at 04:02:40AM +0000, Tabi Timur-B04825 wrote:
Shawn Guo wrote:
Ok, got you. We used to set fifo_depth on imx-ssi not the value given by hardware manual, but the one already tuned.
What do you mean by "already tuned". Tuned by whom?
For example, we have SSI fifo depth 15 on i.MX51 design. Instead of having fifo_depth as 15 and then writing 15 - 2 into burstsize and watermark, we used to have fifo_depth as 13, and then burstsize and watermark can just be fifo_depth.
I agree that fsl_ssi does the right thing, having fifo_depth be the hardware value.
*/ssi_private->dma_params_tx.burstsize =ssi_private->fifo_depth - 2;ssi_private->dma_params_rx.burstsize =ssi_private->fifo_depth - 2;ssi_private->dma_params_tx.dma_addr =ssi_private->ssi_phys + offsetof(struct ccsr_ssi, stx0);ssi_private->dma_params_rx.dma_addr =ssi_private->ssi_phys + offsetof(struct ccsr_ssi, srx0);So I'm a little confused by this new binding. I went through a lot of work to remove DMA-specific data from the SSI driver, and here you are adding a bunch of it back. What you're doing here -- I'm doing it in the DMA driver where it belongs.
I'm not sure these all belong to DMA driver. These are all configurations/parameters of SSI, which are related to DMA though. I see fsl_dma are handling these, but I do not fully agree with that, because we end up with fsl_dma driver accessing SSI node and "struct ccsr_ssi" which should be SSI private data.
Well, the SSI needs to tell the DMA driver how to program itself. On PowerPC, the DMA's burst size is based on the SSI's FIFO depth. You're effectively doing what I already do, except you do more work in the SSI driver.
I understand that we need to support old and new bindings, but I already created an infrastructure that supports passing SSI information to the DMA driver.
I do not see an infrastructure, but that fsl_dma driver accesses SSI private data structure. Instead, snd_soc_dai_set[get]_dma_data looks more like an infrastructure to me.
It seems that the above code is different only for the sake of being different, not because it's better.
This is not really an i.MX vs. PowerPC issue, which is why I'm not sure this change belong in this patchset.
Without this change, fsl_ssi can not work with imx-pcm-dma. I do not understand how it can possibly not belong in this patchset?
Secondly, this is the way that how imx-ssi and imx-pcm-dma works and interacts. If we want to move these stuff into imx-pcm-dma, we need a good story to convince imx-ssi users that why it's better than what they currently do.
I don't understand that. You're already rewriting these drivers. Who else needs to be convinced?
As I told before, even now we have fsl_ssi working for DT based IMX platforms, imx5 and imx6, imx-ssi will still exist for those non-DT IMX, like imx21, imx25, imx27, imx31, imx35. That said, imx-pcm-dma will need to work with both fsl_ssi and imx-ssi. Moving those stuff into imx-pcm-dma will require significant changes on imx-ssi too.
This is the initial imx-sgtl5000 machine driver support. It's a device tree only machine driver working with fsl_ssi driver.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- .../bindings/sound/imx-audio-sgtl5000.txt | 24 +++ sound/soc/fsl/Kconfig | 12 ++ sound/soc/fsl/Makefile | 2 + sound/soc/fsl/imx-sgtl5000.c | 177 ++++++++++++++++++++ 4 files changed, 215 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/sound/imx-audio-sgtl5000.txt create mode 100644 sound/soc/fsl/imx-sgtl5000.c
diff --git a/Documentation/devicetree/bindings/sound/imx-audio-sgtl5000.txt b/Documentation/devicetree/bindings/sound/imx-audio-sgtl5000.txt new file mode 100644 index 0000000..421a374 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/imx-audio-sgtl5000.txt @@ -0,0 +1,24 @@ +Freescale i.MX audio complex with SGTL5000 codec + +Required properties: +- compatible : "fsl,imx-audio-sgtl5000" +- model : The user-visible name of this sound complex +- ssi-controller : The phandle of the i.MX SSI controller +- audio-codec : The phandle of the SGTL5000 audio codec +- mux-int-port : The internal port of the i.MX audio muxer (AUDMUX) +- mux-ext-port : The external port of the i.MX audio muxer + +Note: The AUDMUX port numbering should start at 1, which is consistent with +hardware manual. + +Example: + +sound { + compatible = "fsl,imx51-babbage-sgtl5000", + "fsl,imx-audio-sgtl5000"; + model = "imx51-babbage-sgtl5000"; + ssi-controller = <&ssi1>; + audio-codec = <&sgtl5000>; + mux-int-port = <1>; + mux-ext-port = <3>; +}; diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig index 535ee73..aa14fc9 100644 --- a/sound/soc/fsl/Kconfig +++ b/sound/soc/fsl/Kconfig @@ -164,4 +164,16 @@ config SND_SOC_EUKREA_TLV320 Enable I2S based access to the TLV320AIC23B codec attached to the SSI interface
+config SND_SOC_IMX_SGTL5000 + tristate "SoC Audio support for i.MX boards with sgtl5000" + depends on OF && I2C + select SND_SOC_SGTL5000 + select SND_SOC_IMX_PCM_DMA + select SND_SOC_IMX_AUDMUX + select SND_SOC_FSL_SSI + select SND_SOC_FSL_UTILS + help + Say Y if you want to add support for SoC audio on an i.MX board with + a sgtl5000 codec. + endif # SND_IMX_SOC diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile index fbdbed0..638d385 100644 --- a/sound/soc/fsl/Makefile +++ b/sound/soc/fsl/Makefile @@ -40,8 +40,10 @@ snd-soc-eukrea-tlv320-objs := eukrea-tlv320.o snd-soc-phycore-ac97-objs := phycore-ac97.o snd-soc-mx27vis-aic32x4-objs := mx27vis-aic32x4.o snd-soc-wm1133-ev1-objs := wm1133-ev1.o +snd-soc-imx-sgtl5000-objs := imx-sgtl5000.o
obj-$(CONFIG_SND_SOC_EUKREA_TLV320) += snd-soc-eukrea-tlv320.o obj-$(CONFIG_SND_SOC_PHYCORE_AC97) += snd-soc-phycore-ac97.o obj-$(CONFIG_SND_SOC_MX27VIS_AIC32X4) += snd-soc-mx27vis-aic32x4.o obj-$(CONFIG_SND_MXC_SOC_WM1133_EV1) += snd-soc-wm1133-ev1.o +obj-$(CONFIG_SND_SOC_IMX_SGTL5000) += snd-soc-imx-sgtl5000.o diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c new file mode 100644 index 0000000..3786b61 --- /dev/null +++ b/sound/soc/fsl/imx-sgtl5000.c @@ -0,0 +1,177 @@ +/* + * Copyright 2012 Freescale Semiconductor, Inc. + * Copyright 2012 Linaro Ltd. + * + * The code contained herein is licensed under the GNU General Public + * License. You may obtain a copy of the GNU General Public License + * Version 2 or later at the following locations: + * + * http://www.opensource.org/licenses/gpl-license.html + * http://www.gnu.org/copyleft/gpl.html + */ + +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <sound/soc.h> + +#include "../codecs/sgtl5000.h" +#include "imx-audmux.h" + +#define DAI_NAME_SIZE 32 + +struct imx_sgtl5000_data { + struct snd_soc_dai_link dai; + struct snd_soc_card card; + char codec_dai_name[DAI_NAME_SIZE]; + char platform_name[DAI_NAME_SIZE]; + unsigned int clk_frequency; +}; + +static int imx_sgtl5000_dai_init(struct snd_soc_pcm_runtime *rtd) +{ + struct imx_sgtl5000_data *data = container_of(rtd->card, + struct imx_sgtl5000_data, card); + struct device *dev = rtd->card->dev; + int ret; + + ret = snd_soc_dai_set_sysclk(rtd->codec_dai, SGTL5000_SYSCLK, + data->clk_frequency, SND_SOC_CLOCK_IN); + if (ret) { + dev_err(dev, "could not set codec driver clock params\n"); + return ret; + } + + return 0; +} + +static int __devinit imx_sgtl5000_probe(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct device_node *ssi_np, *codec_np; + struct platform_device *ssi_pdev; + struct imx_sgtl5000_data *data; + int int_port, ext_port; + int ret; + + ret = of_property_read_u32(np, "mux-int-port", &int_port); + if (ret) { + dev_err(&pdev->dev, "mux-int-port missing or invalid\n"); + return ret; + } + ret = of_property_read_u32(np, "mux-ext-port", &ext_port); + if (ret) { + dev_err(&pdev->dev, "mux-ext-port missing or invalid\n"); + return ret; + } + + /* + * The port numbering in the hardware manual starts at 1, while + * the audmux API expects it starts at 0. + */ + int_port--; + ext_port--; + ret = imx_audmux_v2_configure_port(int_port, + IMX_AUDMUX_V2_PTCR_SYN | + IMX_AUDMUX_V2_PTCR_TFSEL(ext_port) | + IMX_AUDMUX_V2_PTCR_TCSEL(ext_port) | + IMX_AUDMUX_V2_PTCR_TFSDIR | + IMX_AUDMUX_V2_PTCR_TCLKDIR, + IMX_AUDMUX_V2_PDCR_RXDSEL(ext_port)); + if (ret) { + dev_err(&pdev->dev, "audmux internal port setup failed\n"); + return ret; + } + imx_audmux_v2_configure_port(ext_port, + IMX_AUDMUX_V2_PTCR_SYN | + IMX_AUDMUX_V2_PTCR_TCSEL(int_port), + IMX_AUDMUX_V2_PDCR_RXDSEL(int_port)); + if (ret) { + dev_err(&pdev->dev, "audmux external port setup failed\n"); + return ret; + } + + ssi_np = of_parse_phandle(pdev->dev.of_node, "ssi-controller", 0); + codec_np = of_parse_phandle(pdev->dev.of_node, "audio-codec", 0); + if (!ssi_np || !codec_np) { + dev_err(&pdev->dev, "phandle missing or invalid\n"); + return -EINVAL; + } + + ssi_pdev = of_find_device_by_node(ssi_np); + if (!ssi_pdev) { + dev_err(&pdev->dev, "failed to find SSI platform device\n"); + return -EINVAL; + } + + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + ret = of_property_read_u32(codec_np, "clock-frequency", + &data->clk_frequency); + if (ret) { + dev_err(&pdev->dev, "clock-frequency missing or invalid\n"); + return ret; + } + + data->dai.name = "HiFi"; + data->dai.stream_name = "HiFi"; + data->dai.codec_dai_name = "sgtl5000"; + data->dai.codec_of_node = codec_np; + data->dai.cpu_dai_name = dev_name(&ssi_pdev->dev); + data->dai.platform_name = "imx-pcm-audio"; + data->dai.init = &imx_sgtl5000_dai_init; + data->dai.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | + SND_SOC_DAIFMT_CBM_CFM; + + data->card.dev = &pdev->dev; + ret = snd_soc_of_parse_card_name(&data->card, "model"); + if (ret) + return ret; + data->card.num_links = 1; + data->card.dai_link = &data->dai; + + ret = snd_soc_register_card(&data->card); + if (ret) { + dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret); + return ret; + } + + platform_set_drvdata(pdev, data); + of_node_put(ssi_np); + of_node_put(codec_np); + + return 0; +} + +static int __devexit imx_sgtl5000_remove(struct platform_device *pdev) +{ + struct imx_sgtl5000_data *data = platform_get_drvdata(pdev); + + snd_soc_unregister_card(&data->card); + + return 0; +} + +static const struct of_device_id imx_sgtl5000_dt_ids[] = { + { .compatible = "fsl,imx-audio-sgtl5000", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, imx_sgtl5000_dt_ids); + +static struct platform_driver imx_sgtl5000_driver = { + .driver = { + .name = "imx-sgtl5000", + .owner = THIS_MODULE, + .of_match_table = imx_sgtl5000_dt_ids, + }, + .probe = imx_sgtl5000_probe, + .remove = __devexit_p(imx_sgtl5000_remove), +}; +module_platform_driver(imx_sgtl5000_driver); + +MODULE_AUTHOR("Shawn Guo shawn.guo@linaro.org"); +MODULE_DESCRIPTION("Freescale i.MX SGTL5000 ASoC machine driver"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:imx-sgtl5000");
Shawn Guo wrote:
I did not pick up you Ack-by tag, because there are some changes since v2 you will need to take another look and have the testing, I guess.
Unfortunately, although it works on the MPC8610, it's broken on the P1022:
# speaker-test -t sine -c 2 -r 44100
speaker-test 1.0.11rc2
Playback device is default Stream parameters are 44100Hz, S16_BE, 2 channels Sine wave rate is 440.0000Hz Rate set to 44100Hz (requested 44100Hz) Buffer size range from 256 to 32768 Period size range from 128 to 16384 Periods = 4 Buffer time size 575 To choose buffer_size = 32768 To choose period_size = 8192 was set period_size = 8192 was set buffer_size = 32768
At this point, nothing else happens, there's no audio, and even Ctrl-C doesn't work.
On Thu, Mar 08, 2012 at 02:05:02PM -0600, Timur Tabi wrote:
Shawn Guo wrote:
I did not pick up you Ack-by tag, because there are some changes since v2 you will need to take another look and have the testing, I guess.
Unfortunately, although it works on the MPC8610, it's broken on the P1022:
# speaker-test -t sine -c 2 -r 44100
speaker-test 1.0.11rc2
Playback device is default Stream parameters are 44100Hz, S16_BE, 2 channels Sine wave rate is 440.0000Hz Rate set to 44100Hz (requested 44100Hz) Buffer size range from 256 to 32768 Period size range from 128 to 16384 Periods = 4 Buffer time size 575 To choose buffer_size = 32768 To choose period_size = 8192 was set period_size = 8192 was set buffer_size = 32768
At this point, nothing else happens, there's no audio, and even Ctrl-C doesn't work.
Are you testing the patch set on top of the SHA below, which I mentioned in the cover letter?
3030763 (Merge tag 'asoc-3.4' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound into topic/asoc)
I see the similar problem on the following SHA with my testing on imx-sgtl5000.
1a8feca (Merge branch 'for-3.4' into asoc-next)
Shawn Guo wrote:
Are you testing the patch set on top of the SHA below, which I mentioned in the cover letter?
3030763 (Merge tag 'asoc-3.4' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound into topic/asoc)
I see the similar problem on the following SHA with my testing on imx-sgtl5000.
1a8feca (Merge branch 'for-3.4' into asoc-next)
It appears the problem is with for-3.4 itself. I'm running git-bisect now to narrow it down.
On Fri, Mar 09, 2012 at 02:11:41AM +0000, Tabi Timur-B04825 wrote:
Shawn Guo wrote:
Are you testing the patch set on top of the SHA below, which I mentioned in the cover letter?
3030763 (Merge tag 'asoc-3.4' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound into topic/asoc)
I see the similar problem on the following SHA with my testing on imx-sgtl5000.
1a8feca (Merge branch 'for-3.4' into asoc-next)
It appears the problem is with for-3.4 itself. I'm running git-bisect now to narrow it down.
My bisect tells that the offending commit is:
96acc35 (ASoC: DAPM: Make sure DAPM widget IO ops hold the component mutex)
But I do not understand why yet.
On 9 March 2012 15:13, Shawn Guo shawn.guo@linaro.org wrote:
On Fri, Mar 09, 2012 at 02:11:41AM +0000, Tabi Timur-B04825 wrote:
Shawn Guo wrote:
Are you testing the patch set on top of the SHA below, which I mentioned in the cover letter?
3030763 (Merge tag 'asoc-3.4' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound into topic/asoc)
I see the similar problem on the following SHA with my testing on imx-sgtl5000.
1a8feca (Merge branch 'for-3.4' into asoc-next)
It appears the problem is with for-3.4 itself. I'm running git-bisect now to narrow it down.
My bisect tells that the offending commit is:
96acc35 (ASoC: DAPM: Make sure DAPM widget IO ops hold the component mutex)
But I do not understand why yet.
============================================= [ INFO: possible recursive locking detected ] 3.3.0-rc4+ #159 Not tainted --------------------------------------------- aplay/395 is trying to acquire lock: (&codec->mutex){+.+...}, at: [<802f7414>] soc_widget_update_bits_locked+0x88/0x 1a0
but task is already holding lock: (&codec->mutex){+.+...}, at: [<802f9df4>] snd_soc_dapm_stream_event+0x38/0xc0
On Fri, Mar 09, 2012 at 03:28:22PM +0800, Shawn Guo wrote:
On 9 March 2012 15:13, Shawn Guo shawn.guo@linaro.org wrote:
On Fri, Mar 09, 2012 at 02:11:41AM +0000, Tabi Timur-B04825 wrote:
Shawn Guo wrote:
Are you testing the patch set on top of the SHA below, which I mentioned in the cover letter?
3030763 (Merge tag 'asoc-3.4' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound into topic/asoc)
Adding Liam who I just noticed isn't on the CCs.
It appears the problem is with for-3.4 itself. I'm running git-bisect now to narrow it down.
My bisect tells that the offending commit is:
96acc35 (ASoC: DAPM: Make sure DAPM widget IO ops hold the component mutex)
But I do not understand why yet.
============================================= [ INFO: possible recursive locking detected ] 3.3.0-rc4+ #159 Not tainted
aplay/395 is trying to acquire lock: (&codec->mutex){+.+...}, at: [<802f7414>] soc_widget_update_bits_locked+0x88/0x 1a0
but task is already holding lock: (&codec->mutex){+.+...}, at: [<802f9df4>] snd_soc_dapm_stream_event+0x38/0xc0
Ick, right. We need to create that separate I/O mutex I think... Perhaps we should punt the locking change until 3.5 too, there's no non-CODEC DAPM in 3.4?
On Fri, Mar 09, 2012 at 03:13:49PM +0800, Shawn Guo wrote:
My bisect tells that the offending commit is:
96acc35 (ASoC: DAPM: Make sure DAPM widget IO ops hold the component mutex)
But I do not understand why yet.
It'll be deadlocking somewhere I expect - turning on the soft lockup detector will probably make it obvious.
participants (7)
-
Mark Brown -
Sascha Hauer -
Shawn Guo -
Tabi Timur-B04825 -
Timur Tabi -
Timur Tabi
-
Trent Piepho