[alsa-devel] [PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ
CONFIG_FIQ is only needed when CONFIG_SND_MXC_SOC_FIQ is selected to build imx-pcm-fiq.c, so let SND_MXC_SOC_FIQ select FIQ.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/imx/Kconfig | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/sound/soc/imx/Kconfig b/sound/soc/imx/Kconfig index 91b66ef..17810d9 100644 --- a/sound/soc/imx/Kconfig +++ b/sound/soc/imx/Kconfig @@ -1,7 +1,6 @@ menuconfig SND_IMX_SOC tristate "SoC Audio for Freescale i.MX CPUs" depends on ARCH_MXC - select FIQ select SND_SOC_AC97_BUS help Say Y or M if you want to add support for codecs attached to @@ -12,6 +11,7 @@ if SND_IMX_SOC
config SND_MXC_SOC_FIQ tristate + select FIQ
config SND_MXC_SOC_MX2 tristate
SND_SOC_AC97_BUS is selected to enable the AC97 support in soc-core. Rather than selecting the option under SND_IMX_SOC, it's better to leave the selection to individual machine driver which knows if AC97 support is needed or not.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/imx/Kconfig | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/sound/soc/imx/Kconfig b/sound/soc/imx/Kconfig index 17810d9..bca5d34 100644 --- a/sound/soc/imx/Kconfig +++ b/sound/soc/imx/Kconfig @@ -1,7 +1,6 @@ menuconfig SND_IMX_SOC tristate "SoC Audio for Freescale i.MX CPUs" depends on ARCH_MXC - select SND_SOC_AC97_BUS help Say Y or M if you want to add support for codecs attached to the i.MX SSI interface. @@ -37,6 +36,7 @@ config SND_SOC_MX27VIS_AIC32X4 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 help
It's not necessary for imx-pcm-dma-mx2 to access imx_ssi.dma_params for burstsize initialization. Instead, it can just be done in imx-ssi probe function once.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/imx/imx-pcm-dma-mx2.c | 5 ----- sound/soc/imx/imx-ssi.c | 2 +- 2 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/sound/soc/imx/imx-pcm-dma-mx2.c b/sound/soc/imx/imx-pcm-dma-mx2.c index f974e61..1737141 100644 --- a/sound/soc/imx/imx-pcm-dma-mx2.c +++ b/sound/soc/imx/imx-pcm-dma-mx2.c @@ -271,11 +271,6 @@ static struct snd_soc_platform_driver imx_soc_platform_mx2 = {
static int __devinit imx_soc_platform_probe(struct platform_device *pdev) { - struct imx_ssi *ssi = platform_get_drvdata(pdev); - - ssi->dma_params_tx.burstsize = 6; - ssi->dma_params_rx.burstsize = 4; - return snd_soc_register_platform(&pdev->dev, &imx_soc_platform_mx2); }
diff --git a/sound/soc/imx/imx-ssi.c b/sound/soc/imx/imx-ssi.c index dbf43f5..25c6231 100644 --- a/sound/soc/imx/imx-ssi.c +++ b/sound/soc/imx/imx-ssi.c @@ -668,7 +668,7 @@ static int imx_ssi_probe(struct platform_device *pdev) ssi->dma_params_rx.dma_addr = res->start + SSI_SRX0; ssi->dma_params_tx.dma_addr = res->start + SSI_STX0;
- ssi->dma_params_tx.burstsize = 4; + ssi->dma_params_tx.burstsize = 6; ssi->dma_params_rx.burstsize = 4;
res = platform_get_resource_byname(pdev, IORESOURCE_DMA, "tx0");
Currently the imx-ssi.c[h] accommodates the imx-pcm common bits which are shared between imx-pcm-dma-mx2 and imx-pcm-fiq drivers. It assumes that imx-pcm-dma-mx2 and imx-pcm-fiq will always be used together with imx-ssi driver. However this becomes untrue when we see that driver sound/soc/fsl/fsl_ssi could possibly work with imx-pcm-dma-mx2 too.
The patch moves the imx-pcm common bits from imx-ssi.c[h] into new files imx-pcm.c[h], and let imx-pcm-dma-mx2 and imx-pcm-fiq drivers build it in, so that imx-pcm-dma-mx2 can work with no dependency on imx-ssi driver.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/imx/Kconfig | 5 ++ sound/soc/imx/Makefile | 10 ++-- sound/soc/imx/imx-pcm-dma-mx2.c | 2 +- sound/soc/imx/imx-pcm.c | 105 +++++++++++++++++++++++++++++++++++++++ sound/soc/imx/imx-pcm.h | 32 ++++++++++++ sound/soc/imx/imx-ssi.c | 88 -------------------------------- sound/soc/imx/imx-ssi.h | 16 +------ 7 files changed, 149 insertions(+), 109 deletions(-) create mode 100644 sound/soc/imx/imx-pcm.c create mode 100644 sound/soc/imx/imx-pcm.h
diff --git a/sound/soc/imx/Kconfig b/sound/soc/imx/Kconfig index bca5d34..863a6b4 100644 --- a/sound/soc/imx/Kconfig +++ b/sound/soc/imx/Kconfig @@ -8,12 +8,17 @@ menuconfig SND_IMX_SOC
if SND_IMX_SOC
+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_IMX_PCM
config SND_MXC_SOC_WM1133_EV1 tristate "Audio on the the i.MX31ADS with WM1133-EV1 fitted" diff --git a/sound/soc/imx/Makefile b/sound/soc/imx/Makefile index d6d609b..303bc05 100644 --- a/sound/soc/imx/Makefile +++ b/sound/soc/imx/Makefile @@ -1,11 +1,11 @@ # i.MX Platform Support snd-soc-imx-objs := imx-ssi.o -snd-soc-imx-fiq-objs := imx-pcm-fiq.o -snd-soc-imx-mx2-objs := imx-pcm-dma-mx2.o - obj-$(CONFIG_SND_IMX_SOC) += snd-soc-imx.o -obj-$(CONFIG_SND_MXC_SOC_FIQ) += snd-soc-imx-fiq.o -obj-$(CONFIG_SND_MXC_SOC_MX2) += snd-soc-imx-mx2.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 diff --git a/sound/soc/imx/imx-pcm-dma-mx2.c b/sound/soc/imx/imx-pcm-dma-mx2.c index 1737141..e2d7287 100644 --- a/sound/soc/imx/imx-pcm-dma-mx2.c +++ b/sound/soc/imx/imx-pcm-dma-mx2.c @@ -30,7 +30,7 @@
#include <mach/dma.h>
-#include "imx-ssi.h" +#include "imx-pcm.h"
struct imx_pcm_runtime_data { int period_bytes; diff --git a/sound/soc/imx/imx-pcm.c b/sound/soc/imx/imx-pcm.c new file mode 100644 index 0000000..93dc360 --- /dev/null +++ b/sound/soc/imx/imx-pcm.c @@ -0,0 +1,105 @@ +/* + * Copyright 2009 Sascha Hauer s.hauer@pengutronix.de + * + * This code is based on code copyrighted by Freescale, + * Liam Girdwood, Javier Martin and probably others. + * + * 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 + * option) any later version. + */ + +#include <linux/dma-mapping.h> +#include <linux/module.h> +#include <sound/pcm.h> +#include <sound/soc.h> +#include "imx-pcm.h" + +int snd_imx_pcm_mmap(struct snd_pcm_substream *substream, + struct vm_area_struct *vma) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + int ret; + + ret = dma_mmap_writecombine(substream->pcm->card->dev, vma, + runtime->dma_area, runtime->dma_addr, runtime->dma_bytes); + + pr_debug("%s: ret: %d %p 0x%08x 0x%08x\n", __func__, ret, + runtime->dma_area, + runtime->dma_addr, + runtime->dma_bytes); + return ret; +} +EXPORT_SYMBOL_GPL(snd_imx_pcm_mmap); + +static int imx_pcm_preallocate_dma_buffer(struct snd_pcm *pcm, int stream) +{ + struct snd_pcm_substream *substream = pcm->streams[stream].substream; + struct snd_dma_buffer *buf = &substream->dma_buffer; + size_t size = IMX_SSI_DMABUF_SIZE; + + buf->dev.type = SNDRV_DMA_TYPE_DEV; + buf->dev.dev = pcm->card->dev; + buf->private_data = NULL; + buf->area = dma_alloc_writecombine(pcm->card->dev, size, + &buf->addr, GFP_KERNEL); + if (!buf->area) + return -ENOMEM; + buf->bytes = size; + + return 0; +} + +static u64 imx_pcm_dmamask = DMA_BIT_MASK(32); + +int imx_pcm_new(struct snd_soc_pcm_runtime *rtd) +{ + struct snd_card *card = rtd->card->snd_card; + struct snd_pcm *pcm = rtd->pcm; + int ret = 0; + + if (!card->dev->dma_mask) + card->dev->dma_mask = &imx_pcm_dmamask; + if (!card->dev->coherent_dma_mask) + card->dev->coherent_dma_mask = DMA_BIT_MASK(32); + if (pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream) { + ret = imx_pcm_preallocate_dma_buffer(pcm, + SNDRV_PCM_STREAM_PLAYBACK); + if (ret) + goto out; + } + + if (pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream) { + ret = imx_pcm_preallocate_dma_buffer(pcm, + SNDRV_PCM_STREAM_CAPTURE); + if (ret) + goto out; + } + +out: + return ret; +} +EXPORT_SYMBOL_GPL(imx_pcm_new); + +void imx_pcm_free(struct snd_pcm *pcm) +{ + struct snd_pcm_substream *substream; + struct snd_dma_buffer *buf; + int stream; + + for (stream = 0; stream < 2; stream++) { + substream = pcm->streams[stream].substream; + if (!substream) + continue; + + buf = &substream->dma_buffer; + if (!buf->area) + continue; + + dma_free_writecombine(pcm->card->dev, buf->bytes, + buf->area, buf->addr); + buf->area = NULL; + } +} +EXPORT_SYMBOL_GPL(imx_pcm_free); diff --git a/sound/soc/imx/imx-pcm.h b/sound/soc/imx/imx-pcm.h new file mode 100644 index 0000000..b5f5c3a --- /dev/null +++ b/sound/soc/imx/imx-pcm.h @@ -0,0 +1,32 @@ +/* + * Copyright 2009 Sascha Hauer s.hauer@pengutronix.de + * + * This code is based on code copyrighted by Freescale, + * Liam Girdwood, Javier Martin and probably others. + * + * 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 + * option) any later version. + */ + +#ifndef _IMX_PCM_H +#define _IMX_PCM_H + +/* + * Do not change this as the FIQ handler depends on this size + */ +#define IMX_SSI_DMABUF_SIZE (64 * 1024) + +struct imx_pcm_dma_params { + int dma; + unsigned long dma_addr; + int burstsize; +}; + +int snd_imx_pcm_mmap(struct snd_pcm_substream *substream, + struct vm_area_struct *vma); +int imx_pcm_new(struct snd_soc_pcm_runtime *rtd); +void imx_pcm_free(struct snd_pcm *pcm); + +#endif /* _IMX_PCM_H */ diff --git a/sound/soc/imx/imx-ssi.c b/sound/soc/imx/imx-ssi.c index 25c6231..9203cdd 100644 --- a/sound/soc/imx/imx-ssi.c +++ b/sound/soc/imx/imx-ssi.c @@ -363,94 +363,6 @@ static const struct snd_soc_dai_ops imx_ssi_pcm_dai_ops = { .trigger = imx_ssi_trigger, };
-int snd_imx_pcm_mmap(struct snd_pcm_substream *substream, - struct vm_area_struct *vma) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - int ret; - - ret = dma_mmap_writecombine(substream->pcm->card->dev, vma, - runtime->dma_area, runtime->dma_addr, runtime->dma_bytes); - - pr_debug("%s: ret: %d %p 0x%08x 0x%08x\n", __func__, ret, - runtime->dma_area, - runtime->dma_addr, - runtime->dma_bytes); - return ret; -} -EXPORT_SYMBOL_GPL(snd_imx_pcm_mmap); - -static int imx_pcm_preallocate_dma_buffer(struct snd_pcm *pcm, int stream) -{ - struct snd_pcm_substream *substream = pcm->streams[stream].substream; - struct snd_dma_buffer *buf = &substream->dma_buffer; - size_t size = IMX_SSI_DMABUF_SIZE; - - buf->dev.type = SNDRV_DMA_TYPE_DEV; - buf->dev.dev = pcm->card->dev; - buf->private_data = NULL; - buf->area = dma_alloc_writecombine(pcm->card->dev, size, - &buf->addr, GFP_KERNEL); - if (!buf->area) - return -ENOMEM; - buf->bytes = size; - - return 0; -} - -static u64 imx_pcm_dmamask = DMA_BIT_MASK(32); - -int imx_pcm_new(struct snd_soc_pcm_runtime *rtd) -{ - struct snd_card *card = rtd->card->snd_card; - struct snd_pcm *pcm = rtd->pcm; - int ret = 0; - - if (!card->dev->dma_mask) - card->dev->dma_mask = &imx_pcm_dmamask; - if (!card->dev->coherent_dma_mask) - card->dev->coherent_dma_mask = DMA_BIT_MASK(32); - if (pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream) { - ret = imx_pcm_preallocate_dma_buffer(pcm, - SNDRV_PCM_STREAM_PLAYBACK); - if (ret) - goto out; - } - - if (pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream) { - ret = imx_pcm_preallocate_dma_buffer(pcm, - SNDRV_PCM_STREAM_CAPTURE); - if (ret) - goto out; - } - -out: - return ret; -} -EXPORT_SYMBOL_GPL(imx_pcm_new); - -void imx_pcm_free(struct snd_pcm *pcm) -{ - struct snd_pcm_substream *substream; - struct snd_dma_buffer *buf; - int stream; - - for (stream = 0; stream < 2; stream++) { - substream = pcm->streams[stream].substream; - if (!substream) - continue; - - buf = &substream->dma_buffer; - if (!buf->area) - continue; - - dma_free_writecombine(pcm->card->dev, buf->bytes, - buf->area, buf->addr); - buf->area = NULL; - } -} -EXPORT_SYMBOL_GPL(imx_pcm_free); - static int imx_ssi_dai_probe(struct snd_soc_dai *dai) { struct imx_ssi *ssi = dev_get_drvdata(dai->dev); diff --git a/sound/soc/imx/imx-ssi.h b/sound/soc/imx/imx-ssi.h index 1072dfb..5744e86 100644 --- a/sound/soc/imx/imx-ssi.h +++ b/sound/soc/imx/imx-ssi.h @@ -187,12 +187,7 @@
#include <linux/dmaengine.h> #include <mach/dma.h> - -struct imx_pcm_dma_params { - int dma; - unsigned long dma_addr; - int burstsize; -}; +#include "imx-pcm.h"
struct imx_ssi { struct platform_device *ac97_dev; @@ -218,13 +213,4 @@ struct imx_ssi { struct platform_device *soc_platform_pdev_fiq; };
-int snd_imx_pcm_mmap(struct snd_pcm_substream *substream, struct vm_area_struct *vma); -int imx_pcm_new(struct snd_soc_pcm_runtime *rtd); -void imx_pcm_free(struct snd_pcm *pcm); - -/* - * Do not change this as the FIQ handler depends on this size - */ -#define IMX_SSI_DMABUF_SIZE (64 * 1024) - #endif /* _IMX_SSI_H */
This is the second step towards reusing fsl_ssi driver on device tree based IMX platforms. The series merges sound/soc/imx into sound/soc/fsl and makes fsl_ssi driver compilable on ARM/IMX.
Compile-tested for PowerPC.
Shawn Guo (4): ASoC: imx: add an explicit Kconfig option for imx-ssi driver ASoC: fsl: separate SSI and DMA Kconfig options ASoC: imx: merge sound/soc/imx into sound/soc/fsl ASoC: fsl: make fsl_ssi driver compilable on ARM/IMX
sound/soc/Makefile | 1 - sound/soc/fsl/Kconfig | 99 ++++++++++++++++++++++++++++-- sound/soc/fsl/Makefile | 22 ++++++- sound/soc/{imx => fsl}/eukrea-tlv320.c | 2 +- sound/soc/fsl/fsl_ssi.c | 75 ++++++++++++++--------- 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 | 64 ------------------- sound/soc/imx/Makefile | 19 ------ 17 files changed, 162 insertions(+), 123 deletions(-)
Currently ASoC:imx uses menuconfig option SND_IMX_SOC selects imx-ssi driver, and it works because all the machine driver covered by the menuconfig need to build imx-ssi driver in. However, it will not work any more if we have a imx based machine driver going into the menuconfig while working with fsl_ssi driver (sound/soc/fsl/fsl_ssi.c) rather than imx-ssi one.
The patch adds an explicit Kconfig option SND_SOC_IMX_SSI for imx-ssi driver, so that it can be selected independently from the menuconfig option SND_IMX_SOC.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/imx/Kconfig | 7 +++++++ sound/soc/imx/Makefile | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/sound/soc/imx/Kconfig b/sound/soc/imx/Kconfig index 863a6b4..b9de8a8 100644 --- a/sound/soc/imx/Kconfig +++ b/sound/soc/imx/Kconfig @@ -8,6 +8,9 @@ menuconfig SND_IMX_SOC
if SND_IMX_SOC
+config SND_SOC_IMX_SSI + tristate + config SND_SOC_IMX_PCM tristate
@@ -25,6 +28,7 @@ config SND_MXC_SOC_WM1133_EV1 depends on MACH_MX31ADS_WM1133_EV1 && EXPERIMENTAL select SND_SOC_WM8350 select SND_MXC_SOC_FIQ + select SND_SOC_IMX_SSI help Enable support for audio on the i.MX31ADS with the WM1133-EV1 PMIC board with WM8835x fitted. @@ -34,6 +38,7 @@ config SND_SOC_MX27VIS_AIC32X4 depends on MACH_IMX27_VISSTRIM_M10 && I2C select SND_SOC_TLV320AIC32X4 select SND_MXC_SOC_MX2 + select SND_SOC_IMX_SSI help Say Y if you want to add support for SoC audio on Visstrim SM10 board with TLV320AIC32X4 codec. @@ -44,6 +49,7 @@ config SND_SOC_PHYCORE_AC97 select SND_SOC_AC97_BUS select SND_SOC_WM9712 select SND_MXC_SOC_FIQ + 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 @@ -57,6 +63,7 @@ config SND_SOC_EUKREA_TLV320 depends on I2C select SND_SOC_TLV320AIC23 select SND_MXC_SOC_FIQ + select SND_SOC_IMX_SSI help Enable I2S based access to the TLV320AIC23B codec attached to the SSI interface diff --git a/sound/soc/imx/Makefile b/sound/soc/imx/Makefile index 303bc05..981c9da 100644 --- a/sound/soc/imx/Makefile +++ b/sound/soc/imx/Makefile @@ -1,6 +1,6 @@ # i.MX Platform Support -snd-soc-imx-objs := imx-ssi.o -obj-$(CONFIG_SND_IMX_SOC) += snd-soc-imx.o +snd-soc-imx-ssi-objs := imx-ssi.o +obj-$(CONFIG_SND_SOC_IMX_SSI) += snd-soc-imx-ssi.o
obj-$(CONFIG_SND_SOC_IMX_PCM) += snd-soc-imx-pcm.o snd-soc-imx-pcm-y := imx-pcm.o
Shawn Guo wrote:
Signed-off-by: Shawn Guo shawn.guo@linaro.org
Since when do you work for Linaro?
sound/soc/imx/Kconfig | 7 +++++++ sound/soc/imx/Makefile | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/sound/soc/imx/Kconfig b/sound/soc/imx/Kconfig index 863a6b4..b9de8a8 100644 --- a/sound/soc/imx/Kconfig +++ b/sound/soc/imx/Kconfig @@ -8,6 +8,9 @@ menuconfig SND_IMX_SOC
if SND_IMX_SOC
+config SND_SOC_IMX_SSI
- tristate
I was hoping to see sound/soc/imx disappear completely, and all IMX support move into sound/soc/fsl. After all, i.MX is a Freescale name.
Anyway, I'm really glad to see this patchset, but I won't be able to devote much time to studying it until next week, at the earliest.
On Thu, Feb 23, 2012 at 09:40:00AM -0600, Timur Tabi wrote:
Shawn Guo wrote:
Signed-off-by: Shawn Guo shawn.guo@linaro.org
Since when do you work for Linaro?
Since one year ago, as Freescale assignee to Linaro. So I'm still employed by Freescale.
sound/soc/imx/Kconfig | 7 +++++++ sound/soc/imx/Makefile | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/sound/soc/imx/Kconfig b/sound/soc/imx/Kconfig index 863a6b4..b9de8a8 100644 --- a/sound/soc/imx/Kconfig +++ b/sound/soc/imx/Kconfig @@ -8,6 +8,9 @@ menuconfig SND_IMX_SOC
if SND_IMX_SOC
+config SND_SOC_IMX_SSI
- tristate
I was hoping to see sound/soc/imx disappear completely, and all IMX support move into sound/soc/fsl. After all, i.MX is a Freescale name.
That's what I'm doing. But to be clear, imx-ssi driver will still exist for all those non-DT i.MX platforms. We are supporting DT for imx5 and imx6, but have not seen a plan to migrate all those old i.MX platforms to DT.
Shawn Guo wrote:
That's what I'm doing. But to be clear, imx-ssi driver will still exist for all those non-DT i.MX platforms. We are supporting DT for imx5 and imx6, but have not seen a plan to migrate all those old i.MX platforms to DT.
On PowerPC, we handle this by creating cuImages. These are kernel images that have a DTB built-in. Perhaps something like that is needed for ARM?
On Fri, Feb 24, 2012 at 02:59:22AM +0000, Tabi Timur-B04825 wrote:
Shawn Guo wrote:
That's what I'm doing. But to be clear, imx-ssi driver will still exist for all those non-DT i.MX platforms. We are supporting DT for imx5 and imx6, but have not seen a plan to migrate all those old i.MX platforms to DT.
On PowerPC, we handle this by creating cuImages. These are kernel images that have a DTB built-in. Perhaps something like that is needed for ARM?
We have the similar support on ARM (dtb can be appended to zImage). It resolves the problem that boot loader does not support DT. However what I'm saying is that we do not have a plan to add DT support for those old i.MX SoCs like imx1, imx2, imx3 in kernel. That said, the kernel running on those platforms are not aware of DT.
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 --- sound/soc/Kconfig | 1 - sound/soc/Makefile | 1 - sound/soc/fsl/Kconfig | 84 ++++++++++++++++++++++++++++++ sound/soc/fsl/Makefile | 19 +++++++ sound/soc/{imx => fsl}/eukrea-tlv320.c | 2 +- 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 | 71 ------------------------- sound/soc/imx/Makefile | 19 ------- 16 files changed, 105 insertions(+), 94 deletions(-) rename sound/soc/{imx => fsl}/eukrea-tlv320.c (99%) 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/sound/soc/Kconfig b/sound/soc/Kconfig index 35e662d..2b95e6b 100644 --- a/sound/soc/Kconfig +++ b/sound/soc/Kconfig @@ -32,7 +32,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 9ea8ac8..d95a119 100644 --- a/sound/soc/Makefile +++ b/sound/soc/Makefile @@ -9,7 +9,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..a62dc28 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,75 @@ 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_IMX_PCM + +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_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_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_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_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..65087b1 100644 --- a/sound/soc/fsl/Makefile +++ b/sound/soc/fsl/Makefile @@ -21,3 +21,22 @@ 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 +obj-$(CONFIG_SND_SOC_IMX_SSI) += snd-soc-imx-ssi.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 1c1fdd1..c33a303 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-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 b9de8a8..0000000 --- a/sound/soc/imx/Kconfig +++ /dev/null @@ -1,71 +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 - tristate - select SND_SOC_IMX_PCM - -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_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_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_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_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 981c9da..0000000 --- a/sound/soc/imx/Makefile +++ /dev/null @@ -1,19 +0,0 @@ -# i.MX Platform Support -snd-soc-imx-ssi-objs := imx-ssi.o -obj-$(CONFIG_SND_SOC_IMX_SSI) += snd-soc-imx-ssi.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
Change PowerPC dependent IO accessors to architecture independent ones, so that fsl_ssi driver can be built on both PowerPC and ARM/IMX.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/fsl/fsl_ssi.c | 75 ++++++++++++++++++++++++++++------------------ 1 files changed, 46 insertions(+), 29 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 3e06696..32fc84b 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -11,11 +11,14 @@ */
#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 <sound/core.h> @@ -145,7 +148,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 = readl(&ssi->sisr) & SIER_FLAGS;
if (sisr & CCSR_SSI_SISR_RFRC) { ssi_private->stats.rfrc++; @@ -260,7 +263,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); + writel(sisr2, &ssi->sisr);
return ret; } @@ -280,6 +283,7 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(rtd->cpu_dai); int synchronous = ssi_private->cpu_dai_drv.symmetric_rates; + u32 val;
/* * If this is the first stream opened, then request the IRQ @@ -295,7 +299,9 @@ 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); + val = readl(&ssi->scr); + val &= ~CCSR_SSI_SCR_SSIEN; + writel(val, &ssi->scr);
/* * Program the SSI into I2S Slave Non-Network Synchronous mode. @@ -303,20 +309,19 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, * * FIXME: Little-endian samples require a different shift dir */ - clrsetbits_be32(&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)); + val = readl(&ssi->scr); + val &= ~(CCSR_SSI_SCR_I2S_MODE_MASK | CCSR_SSI_SCR_SYN); + val |= CCSR_SSI_SCR_TFR_CLK_DIS | CCSR_SSI_SCR_I2S_MODE_SLAVE | + (synchronous ? CCSR_SSI_SCR_SYN : 0); + writel(val, &ssi->scr);
- out_be32(&ssi->stcr, - CCSR_SSI_STCR_TXBIT0 | CCSR_SSI_STCR_TFEN0 | + writel(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 | + writel(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 +329,7 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream, */
/* Enable the interrupts and DMA requests */ - out_be32(&ssi->sier, SIER_FLAGS); + writel(SIER_FLAGS, &ssi->sier);
/* * Set the watermark for transmit FIFI 0 and receive FIFO 0. We @@ -339,9 +344,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)); + writel(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 +422,8 @@ 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 = readl(&ssi->scr) & CCSR_SSI_SCR_SSIEN; + u32 val;
/* * If we're in synchronous mode, and the SSI is already enabled, @@ -438,10 +444,17 @@ 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); - else - clrsetbits_be32(&ssi->srccr, CCSR_SSI_SxCCR_WL_MASK, wl); + ssi_private->cpu_dai_drv.symmetric_rates) { + val = readl(&ssi->stccr); + val &= CCSR_SSI_SxCCR_WL_MASK; + val |= wl; + writel(val, &ssi->stccr); + } else { + val = readl(&ssi->srccr); + val &= CCSR_SSI_SxCCR_WL_MASK; + val |= wl; + writel(val, &ssi->srccr); + }
return 0; } @@ -461,29 +474,30 @@ 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; + u32 val;
+ val = readl(&ssi->scr); switch (cmd) { case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - setbits32(&ssi->scr, - CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE); + val |= CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE; else - setbits32(&ssi->scr, - CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_RE); + val |= 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); + val &= ~CCSR_SSI_SCR_TE; else - clrbits32(&ssi->scr, CCSR_SSI_SCR_RE); + val &= ~CCSR_SSI_SCR_RE; break;
default: return -EINVAL; } + writel(val, &ssi->scr);
return 0; } @@ -498,6 +512,7 @@ static void fsl_ssi_shutdown(struct snd_pcm_substream *substream, { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(rtd->cpu_dai); + u32 val;
if (ssi_private->first_stream == substream) ssi_private->first_stream = ssi_private->second_stream; @@ -510,7 +525,9 @@ 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); + val = readl(&ssi->scr); + val &= ~CCSR_SSI_SCR_SSIEN; + writel(val, &ssi->scr); } }
This is the third series towards reusing fsl_ssi driver on device tree based IMX platforms. It makes a few cleanups on existing PowerPC code to ease the later addition of an IMX machine driver that works on fsl_ssi.
Shawn Guo (6): ASoC: fsl: correct get_dma_channel parameter name ASoC: fsl: align mpc8610_hpcd with p1022_ds on getting codec node ASoC: Remove unnecessary -codec from cs4270 driver name ASoC: fsl: create fsl_utils to accommodate the common functions ASoC: fsl: use platform_device_id table to match p1022_ds device ASoC: fsl: check property 'compatible' for the machine name
sound/soc/codecs/cs4270.c | 2 +- sound/soc/fsl/Kconfig | 5 + sound/soc/fsl/Makefile | 2 + sound/soc/fsl/fsl_ssi.c | 6 +- sound/soc/fsl/fsl_utils.c | 153 ++++++++++++++++++++++++++++++++++ sound/soc/fsl/fsl_utils.h | 29 +++++++ sound/soc/fsl/mpc8610_hpcd.c | 167 +++---------------------------------- sound/soc/fsl/p1022_ds.c | 185 ++++------------------------------------- sound/soc/pxa/raumfeld.c | 2 +- 9 files changed, 226 insertions(+), 325 deletions(-)
On Fri, Feb 24, 2012 at 10:09:35PM +0800, Shawn Guo wrote:
This is the third series towards reusing fsl_ssi driver on device tree based IMX platforms. It makes a few cleanups on existing PowerPC code to ease the later addition of an IMX machine driver that works on fsl_ssi.
Please don't post new threads as followups to old threads, it makes things harder to follow in threaded mail readers.
On Fri, Feb 24, 2012 at 02:02:01PM +0000, Mark Brown wrote:
On Fri, Feb 24, 2012 at 10:09:35PM +0800, Shawn Guo wrote:
This is the third series towards reusing fsl_ssi driver on device tree based IMX platforms. It makes a few cleanups on existing PowerPC code to ease the later addition of an IMX machine driver that works on fsl_ssi.
Please don't post new threads as followups to old threads, it makes things harder to follow in threaded mail readers.
I did that because they are a big series, though some of the patches can be standalone.
On Fri, Feb 24, 2012 at 10:23:45PM +0800, Shawn Guo wrote:
On Fri, Feb 24, 2012 at 02:02:01PM +0000, Mark Brown wrote:
Please don't post new threads as followups to old threads, it makes things harder to follow in threaded mail readers.
I did that because they are a big series, though some of the patches can be standalone.
That doesn't seem to be related to my point? I'm asking you to not reply to an old version of your patch series in order to post a new version of it, just post the new version.
On Fri, Feb 24, 2012 at 02:17:06PM +0000, Mark Brown wrote:
On Fri, Feb 24, 2012 at 10:23:45PM +0800, Shawn Guo wrote:
On Fri, Feb 24, 2012 at 02:02:01PM +0000, Mark Brown wrote:
Please don't post new threads as followups to old threads, it makes things harder to follow in threaded mail readers.
I did that because they are a big series, though some of the patches can be standalone.
That doesn't seem to be related to my point? I'm asking you to not reply to an old version of your patch series in order to post a new version of it, just post the new version.
This is the first version of this series.
Shawn Guo wrote:
That doesn't seem to be related to my point? I'm asking you to not reply to an old version of your patch series in order to post a new version of it, just post the new version.
This is the first version of this series.
I think there's a communication breakdown. Shawn, take a look at this:
http://mailman.alsa-project.org/pipermail/alsa-devel/2012-February/thread.ht...
Search for "[alsa-devel] [PATCH 0/6] ASoC: a few cleanups on sound/soc/fsl".
You'll see that this email is indented, because it was posted to the mailing list AS A REPLY to "[alsa-devel] [PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ". That's because it has this line in the message header:
In-Reply-To: 1329979644-31046-1-git-send-email-shawn.guo@linaro.org
This is what Mark is talking about. He wants new patch sets to be posted NOT as replies to other patch sets.
On Fri, Feb 24, 2012 at 05:14:12PM -0600, Timur Tabi wrote:
Shawn Guo wrote:
That doesn't seem to be related to my point? I'm asking you to not reply to an old version of your patch series in order to post a new version of it, just post the new version.
This is the first version of this series.
I think there's a communication breakdown. Shawn, take a look at this:
http://mailman.alsa-project.org/pipermail/alsa-devel/2012-February/thread.ht...
Search for "[alsa-devel] [PATCH 0/6] ASoC: a few cleanups on sound/soc/fsl".
You'll see that this email is indented, because it was posted to the mailing list AS A REPLY to "[alsa-devel] [PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ". That's because it has this line in the message header:
In-Reply-To: 1329979644-31046-1-git-send-email-shawn.guo@linaro.org
This is what Mark is talking about. He wants new patch sets to be posted NOT as replies to other patch sets.
Yes, I did something wrong. But my point is the following patches should belong to one big series. They can not be applied separately.
[PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ [PATCH 2/4] ASoC: imx: move SND_SOC_AC97_BUS selection down to machine driver [PATCH 3/4] ASoC: imx: initialize dma_params burstsize just in imx-ssi [PATCH 4/4] ASoC: imx: separate imx-pcm bits from imx-ssi driver [PATCH 0/4] ASoC: merge imx into fsl [PATCH 0/6] ASoC: a few cleanups on sound/soc/fsl
Shawn Guo wrote:
Yes, I did something wrong. But my point is the following patches should belong to one big series. They can not be applied separately.
[PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ [PATCH 2/4] ASoC: imx: move SND_SOC_AC97_BUS selection down to machine driver [PATCH 3/4] ASoC: imx: initialize dma_params burstsize just in imx-ssi [PATCH 4/4] ASoC: imx: separate imx-pcm bits from imx-ssi driver [PATCH 0/4] ASoC: merge imx into fsl [PATCH 0/6] ASoC: a few cleanups on sound/soc/fsl
That's not what Mark is talking about. You somehow posted patch 0/6 as a reply to the *previous* patchset, so the two patchsets were merged into one email thread.
On Sat, Feb 25, 2012 at 01:39:05AM +0000, Tabi Timur-B04825 wrote:
Shawn Guo wrote:
Yes, I did something wrong. But my point is the following patches should belong to one big series. They can not be applied separately.
[PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ [PATCH 2/4] ASoC: imx: move SND_SOC_AC97_BUS selection down to machine driver [PATCH 3/4] ASoC: imx: initialize dma_params burstsize just in imx-ssi [PATCH 4/4] ASoC: imx: separate imx-pcm bits from imx-ssi driver [PATCH 0/4] ASoC: merge imx into fsl [PATCH 0/6] ASoC: a few cleanups on sound/soc/fsl
That's not what Mark is talking about. You somehow posted patch 0/6 as a reply to the *previous* patchset, so the two patchsets were merged into one email thread.
Sorry, you and Mark are not listening to Shawn. Even worse, you're telling Shawn that what he's trying to explain to you is not the point when it is actually the whole point.
The "[PATCH 0/6] ASoC: a few cleanups on sound/soc/fsl" was posted as a follow-up to "[PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ".
So, the "*previous* patchset" _was_ "[PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ" and not some other random patch set. That is a fundamental fact, which can't be argued. Anyone can verify it, and it can be verified by checking the email headers.
On Sat, Feb 25, 2012 at 10:17:58AM +0000, Russell King - ARM Linux wrote:
On Sat, Feb 25, 2012 at 01:39:05AM +0000, Tabi Timur-B04825 wrote:
That's not what Mark is talking about. You somehow posted patch 0/6 as a reply to the *previous* patchset, so the two patchsets were merged into one email thread.
Sorry, you and Mark are not listening to Shawn. Even worse, you're telling Shawn that what he's trying to explain to you is not the point when it is actually the whole point.
The "[PATCH 0/6] ASoC: a few cleanups on sound/soc/fsl" was posted as a follow-up to "[PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ".
So, the "*previous* patchset" _was_ "[PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ" and not some other random patch set. That is a fundamental fact, which can't be argued. Anyone can verify it, and it can be verified by checking the email headers.
No, I get that it's a new version of the same patch set rather than a totally unrelated patch set that was followed up to - I'd still rather it weren't posted like that as the threading ends up being less helpful than is desirable.
On Sat, Feb 25, 2012 at 11:44:15AM +0000, Mark Brown wrote:
On Sat, Feb 25, 2012 at 10:17:58AM +0000, Russell King - ARM Linux wrote:
On Sat, Feb 25, 2012 at 01:39:05AM +0000, Tabi Timur-B04825 wrote:
That's not what Mark is talking about. You somehow posted patch 0/6 as a reply to the *previous* patchset, so the two patchsets were merged into one email thread.
Sorry, you and Mark are not listening to Shawn. Even worse, you're telling Shawn that what he's trying to explain to you is not the point when it is actually the whole point.
The "[PATCH 0/6] ASoC: a few cleanups on sound/soc/fsl" was posted as a follow-up to "[PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ".
So, the "*previous* patchset" _was_ "[PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ" and not some other random patch set. That is a fundamental fact, which can't be argued. Anyone can verify it, and it can be verified by checking the email headers.
No, I get that it's a new version of the same patch set rather than a totally unrelated patch set that was followed up to - I'd still rather it weren't posted like that as the threading ends up being less helpful than is desirable.
Take a look at the diffstat, and you'll see that there's no way that it's a "new version of the same patch set". Tell me, how can the set of six patches be a new version of the previous set of four patches when the previous set of four patches moves a bunch of files and the following set of six patches does not?
And if you bother to read Shawn's reply to your question, you'll see that the whole reason he grouped the two together is because they follow on from each other.
Here's the diffstats from his emails from the interlinked thread:
[PATCH 1/4] ASoC: imx: add an explicit Kconfig option for imx-ssi driver sound/soc/imx/Kconfig | 7 +++++++ sound/soc/imx/Makefile | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-)
[PATCH 2/4] ASoC: fsl: separate SSI and DMA Kconfig options sound/soc/fsl/Kconfig | 15 +++++++++------ sound/soc/fsl/Makefile | 3 ++- 2 files changed, 11 insertions(+), 7 deletions(-)
[PATCH 3/4] ASoC: imx: merge sound/soc/imx into sound/soc/fsl sound/soc/Kconfig | 1 - sound/soc/Makefile | 1 - sound/soc/fsl/Kconfig | 84 ++++++++++++++++++++++++++++++ sound/soc/fsl/Makefile | 19 +++++++ sound/soc/{imx => fsl}/eukrea-tlv320.c | 2 +- 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}/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 | 71 ------------------------- sound/soc/imx/Makefile | 19 ------- 16 files changed, 105 insertions(+), 94 deletions(-)
[PATCH 4/4] ASoC: fsl: make fsl_ssi driver compilable on ARM/IMX sound/soc/fsl/fsl_ssi.c | 75 ++++++++++++++++++++++++++++------------------
[PATCH 1/6] ASoC: fsl: correct get_dma_channel parameter name sound/soc/fsl/mpc8610_hpcd.c | 4 ++-- sound/soc/fsl/p1022_ds.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
[PATCH 2/6] ASoC: fsl: align mpc8610_hpcd with p1022_ds on getting codec node sound/soc/fsl/mpc8610_hpcd.c | 8 ++------ 1 files changed, 2 insertions(+), 6 deletions(-)
[PATCH 3/6] ASoC: Remove unnecessary -codec from cs4270 driver name sound/soc/codecs/cs4270.c | 2 +- sound/soc/fsl/mpc8610_hpcd.c | 4 ++-- sound/soc/pxa/raumfeld.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)
[PATCH 4/6] ASoC: fsl: create fsl_utils to accommodate the common functions sound/soc/fsl/Kconfig | 5 ++ sound/soc/fsl/Makefile | 2 + sound/soc/fsl/fsl_utils.c | 153 ++++++++++++++++++++++++++++++++++++++++ sound/soc/fsl/fsl_utils.h | 29 ++++++++ sound/soc/fsl/mpc8610_hpcd.c | 157 +++--------------------------------------- sound/soc/fsl/p1022_ds.c | 149 +++------------------------------------- 6 files changed, 209 insertions(+), 286 deletions(-)
[PATCH 5/6] ASoC: fsl: use platform_device_id table to match p1022_ds device sound/soc/fsl/p1022_ds.c | 36 +++++++++--------------------------- 1 files changed, 9 insertions(+), 27 deletions(-)
[PATCH 6/6] ASoC: fsl: check property 'compatible' for the machine name sound/soc/fsl/fsl_ssi.c | 6 +++--- sound/soc/fsl/mpc8610_hpcd.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)
See - not the 6 patch patchset is IN NO WAY a new version of the 4 patch patchset like you're trying to say it is.
So my point that you're not listening stands.
On Sat, Feb 25, 2012 at 12:12:17PM +0000, Russell King - ARM Linux wrote:
Take a look at the diffstat, and you'll see that there's no way that it's a "new version of the same patch set". Tell me, how can the set of six
Yeah, sorry - I misremembered the specific reason they were related when I replied to you.
And if you bother to read Shawn's reply to your question, you'll see that the whole reason he grouped the two together is because they follow on from each other.
No, I understand that the patch sets are related, like I say I had misremembered the relation when replying to you.
So my point that you're not listening stands.
I am, what I'm asking is that people don't send new patch sets in reply to old ones even if they are related as while the thought is good in that case the software tends not to do a great job with it. Either just mentioning things in the cover letter or resending the previous stuff as part of a new, combined series tends to work better.
On Sat, Feb 25, 2012 at 11:44:15AM +0000, Mark Brown wrote:
On Sat, Feb 25, 2012 at 10:17:58AM +0000, Russell King - ARM Linux wrote:
On Sat, Feb 25, 2012 at 01:39:05AM +0000, Tabi Timur-B04825 wrote:
That's not what Mark is talking about. You somehow posted patch 0/6 as a reply to the *previous* patchset, so the two patchsets were merged into one email thread.
Sorry, you and Mark are not listening to Shawn. Even worse, you're telling Shawn that what he's trying to explain to you is not the point when it is actually the whole point.
The "[PATCH 0/6] ASoC: a few cleanups on sound/soc/fsl" was posted as a follow-up to "[PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ".
So, the "*previous* patchset" _was_ "[PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ" and not some other random patch set. That is a fundamental fact, which can't be argued. Anyone can verify it, and it can be verified by checking the email headers.
No, I get that it's a new version of the same patch set
I know it's confusing. But "[PATCH 0/6] ASoC: a few cleanups on sound/soc/fsl" is not a new version of the series "[PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ". Instead, it's a follow-up series which depends on previously posted patches.
Anyway, I will stop doing that to confuse people.
Regards, Shawn
rather than a totally unrelated patch set that was followed up to - I'd still rather it weren't posted like that as the threading ends up being less helpful than is desirable.
On Sat, Feb 25, 2012 at 07:21:51AM +0800, Shawn Guo wrote:
On Fri, Feb 24, 2012 at 02:17:06PM +0000, Mark Brown wrote:
On Fri, Feb 24, 2012 at 10:23:45PM +0800, Shawn Guo wrote:
On Fri, Feb 24, 2012 at 02:02:01PM +0000, Mark Brown wrote:
Please don't post new threads as followups to old threads, it makes things harder to follow in threaded mail readers.
I did that because they are a big series, though some of the patches can be standalone.
That doesn't seem to be related to my point? I'm asking you to not reply to an old version of your patch series in order to post a new version of it, just post the new version.
This is the first version of this series.
Your covering message for this series of six patches:
From: Shawn Guo shawn.guo@linaro.org To: alsa-devel@alsa-project.org Subject: [PATCH 0/6] ASoC: a few cleanups on sound/soc/fsl Date: Fri, 24 Feb 2012 22:09:35 +0800 Message-Id: 1330092582-21180-1-git-send-email-shawn.guo@linaro.org X-Mailer: git-send-email 1.7.5.4 In-Reply-To: 1329979644-31046-1-git-send-email-shawn.guo@linaro.org ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ References: 1329979644-31046-1-git-send-email-shawn.guo@linaro.org ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Your patch 1 from a previous series:
From: Shawn Guo shawn.guo@linaro.org To: alsa-devel@alsa-project.org Subject: [PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ Date: Thu, 23 Feb 2012 14:47:21 +0800 Message-Id: 1329979644-31046-1-git-send-email-shawn.guo@linaro.org ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Notice the same message ids. That means your covering letter with subject "[PATCH 0/6] ASoC: a few cleanups on sound/soc/fsl" is a reply to your "[PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ" message.
Consequently anyone who reads list mail by threads gets your new series of six patches attached to your previous series of four patches. That can be extremely annoying, especially when it ends up being buried in peoples mailboxes as a direct result of threaded sorting - and it's a recipe for your patches to be ignored.
On Fri, Feb 24, 2012 at 11:23:29PM +0000, Russell King - ARM Linux wrote:
On Sat, Feb 25, 2012 at 07:21:51AM +0800, Shawn Guo wrote:
On Fri, Feb 24, 2012 at 02:17:06PM +0000, Mark Brown wrote:
On Fri, Feb 24, 2012 at 10:23:45PM +0800, Shawn Guo wrote:
On Fri, Feb 24, 2012 at 02:02:01PM +0000, Mark Brown wrote:
Please don't post new threads as followups to old threads, it makes things harder to follow in threaded mail readers.
I did that because they are a big series, though some of the patches can be standalone.
That doesn't seem to be related to my point? I'm asking you to not reply to an old version of your patch series in order to post a new version of it, just post the new version.
This is the first version of this series.
Your covering message for this series of six patches:
From: Shawn Guo shawn.guo@linaro.org To: alsa-devel@alsa-project.org Subject: [PATCH 0/6] ASoC: a few cleanups on sound/soc/fsl Date: Fri, 24 Feb 2012 22:09:35 +0800 Message-Id: 1330092582-21180-1-git-send-email-shawn.guo@linaro.org X-Mailer: git-send-email 1.7.5.4 In-Reply-To: 1329979644-31046-1-git-send-email-shawn.guo@linaro.org ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ References: 1329979644-31046-1-git-send-email-shawn.guo@linaro.org ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Your patch 1 from a previous series:
From: Shawn Guo shawn.guo@linaro.org To: alsa-devel@alsa-project.org Subject: [PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ Date: Thu, 23 Feb 2012 14:47:21 +0800 Message-Id: 1329979644-31046-1-git-send-email-shawn.guo@linaro.org ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Notice the same message ids. That means your covering letter with subject "[PATCH 0/6] ASoC: a few cleanups on sound/soc/fsl" is a reply to your "[PATCH 1/4] ASoC: imx: let SND_MXC_SOC_FIQ select FIQ" message.
Consequently anyone who reads list mail by threads gets your new series of six patches attached to your previous series of four patches. That can be extremely annoying, especially when it ends up being buried in peoples mailboxes as a direct result of threaded sorting - and it's a recipe for your patches to be ignored.
Sorry about that. But they are not separate patch series but belong to one big series. It seems it's a bad idea to post one big series in such a way. Will stop doing it.
The second parameter of function get_dma_channel is actually a property name rather than a compatible string, so rename it for less confusing.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/fsl/mpc8610_hpcd.c | 4 ++-- sound/soc/fsl/p1022_ds.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/fsl/mpc8610_hpcd.c b/sound/soc/fsl/mpc8610_hpcd.c index 0ea4a5a..15e06e9d 100644 --- a/sound/soc/fsl/mpc8610_hpcd.c +++ b/sound/soc/fsl/mpc8610_hpcd.c @@ -273,7 +273,7 @@ static int codec_node_dev_name(struct device_node *np, char *buf, size_t len) }
static int get_dma_channel(struct device_node *ssi_np, - const char *compatible, + const char *name, struct snd_soc_dai_link *dai, unsigned int *dma_channel_id, unsigned int *dma_id) @@ -283,7 +283,7 @@ static int get_dma_channel(struct device_node *ssi_np, const u32 *iprop; int ret;
- dma_channel_np = get_node_by_phandle_name(ssi_np, compatible, + dma_channel_np = get_node_by_phandle_name(ssi_np, name, "fsl,ssi-dma-channel"); if (!dma_channel_np) return -EINVAL; diff --git a/sound/soc/fsl/p1022_ds.c b/sound/soc/fsl/p1022_ds.c index a5d4e80..d32ec46 100644 --- a/sound/soc/fsl/p1022_ds.c +++ b/sound/soc/fsl/p1022_ds.c @@ -276,7 +276,7 @@ static int codec_node_dev_name(struct device_node *np, char *buf, size_t len) }
static int get_dma_channel(struct device_node *ssi_np, - const char *compatible, + const char *name, struct snd_soc_dai_link *dai, unsigned int *dma_channel_id, unsigned int *dma_id) @@ -286,7 +286,7 @@ static int get_dma_channel(struct device_node *ssi_np, const u32 *iprop; int ret;
- dma_channel_np = get_node_by_phandle_name(ssi_np, compatible, + dma_channel_np = get_node_by_phandle_name(ssi_np, name, "fsl,ssi-dma-channel"); if (!dma_channel_np) return -EINVAL;
Shawn Guo wrote:
The second parameter of function get_dma_channel is actually a property name rather than a compatible string, so rename it for less confusing.
Signed-off-by: Shawn Guo shawn.guo@linaro.org
Acked-by: Timur Tabi timur@freescale.com
On Fri, Feb 24, 2012 at 10:09:36PM +0800, Shawn Guo wrote:
The second parameter of function get_dma_channel is actually a property name rather than a compatible string, so rename it for less confusing.
Applied, thanks.
Align mpc8610_hpcd with p1022_ds on getting codec node by just calling of_parse_phandle. The bonus point of doing that is we can save exporting get_node_by_phandle_name() when we consolidate the common bits between mpc8610_hpcd and p1022_ds into a module, which can be shared by more machine drivers added later.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/fsl/mpc8610_hpcd.c | 8 ++------ 1 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/sound/soc/fsl/mpc8610_hpcd.c b/sound/soc/fsl/mpc8610_hpcd.c index 15e06e9d..93256b3 100644 --- a/sound/soc/fsl/mpc8610_hpcd.c +++ b/sound/soc/fsl/mpc8610_hpcd.c @@ -336,12 +336,8 @@ static int mpc8610_hpcd_probe(struct platform_device *pdev) const char *sprop; const u32 *iprop;
- /* We are only interested in SSIs with a codec phandle in them, - * so let's make sure this SSI has one. The MPC8610 HPCD only - * knows about the CS4270 codec, so reject anything else. - */ - codec_np = get_node_by_phandle_name(np, "codec-handle", - "cirrus,cs4270"); + /* Find the codec node for this SSI. */ + codec_np = of_parse_phandle(np, "codec-handle", 0); if (!codec_np) { dev_err(dev, "invalid codec node\n"); return -EINVAL;
Shawn Guo wrote:
Align mpc8610_hpcd with p1022_ds on getting codec node by just calling of_parse_phandle. The bonus point of doing that is we can save exporting get_node_by_phandle_name() when we consolidate the common bits between mpc8610_hpcd and p1022_ds into a module, which can be shared by more machine drivers added later.
Signed-off-by: Shawn Guo shawn.guo@linaro.org
Acked-by: Timur Tabi timur@freescale.com
On Fri, Feb 24, 2012 at 10:09:37PM +0800, Shawn Guo wrote:
Align mpc8610_hpcd with p1022_ds on getting codec node by just calling of_parse_phandle. The bonus point of doing that is we can save exporting get_node_by_phandle_name() when we consolidate the common bits between mpc8610_hpcd and p1022_ds into a module, which can be shared by more machine drivers added later.
Applied, thanks.
Similar to what commit 1e3ad57 (ASoC: Remove redundant -codec from WM8776 driver name) does for wm8776 driver, this patch does the same thing for cs4270 driver.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/codecs/cs4270.c | 2 +- sound/soc/fsl/mpc8610_hpcd.c | 4 ++-- sound/soc/pxa/raumfeld.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c index 6baccd2..1d672f5 100644 --- a/sound/soc/codecs/cs4270.c +++ b/sound/soc/codecs/cs4270.c @@ -715,7 +715,7 @@ MODULE_DEVICE_TABLE(i2c, cs4270_id); */ static struct i2c_driver cs4270_i2c_driver = { .driver = { - .name = "cs4270-codec", + .name = "cs4270", .owner = THIS_MODULE, }, .id_table = cs4270_id, diff --git a/sound/soc/fsl/mpc8610_hpcd.c b/sound/soc/fsl/mpc8610_hpcd.c index 93256b3..fcf9302 100644 --- a/sound/soc/fsl/mpc8610_hpcd.c +++ b/sound/soc/fsl/mpc8610_hpcd.c @@ -245,7 +245,7 @@ static int get_parent_cell_index(struct device_node *np) * '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". + * example, "cs4270.0-004f". */ static int codec_node_dev_name(struct device_node *np, char *buf, size_t len) { @@ -267,7 +267,7 @@ static int codec_node_dev_name(struct device_node *np, char *buf, size_t len) if (!i2c) return -ENODEV;
- snprintf(buf, len, "%s-codec.%u-%04x", temp, i2c->adapter->nr, addr); + snprintf(buf, len, "%s.%u-%04x", temp, i2c->adapter->nr, addr);
return 0; } diff --git a/sound/soc/pxa/raumfeld.c b/sound/soc/pxa/raumfeld.c index ba15451..0837065 100644 --- a/sound/soc/pxa/raumfeld.c +++ b/sound/soc/pxa/raumfeld.c @@ -232,7 +232,7 @@ static struct snd_soc_ops raumfeld_ak4104_ops = { .cpu_dai_name = "pxa-ssp-dai.0", \ .platform_name = "pxa-pcm-audio", \ .codec_dai_name = "cs4270-hifi", \ - .codec_name = "cs4270-codec.0-0048", \ + .codec_name = "cs4270.0-0048", \ .ops = &raumfeld_cs4270_ops, \ }
Shawn Guo wrote:
Similar to what commit 1e3ad57 (ASoC: Remove redundant -codec from WM8776 driver name) does for wm8776 driver, this patch does the same thing for cs4270 driver.
Signed-off-by: Shawn Guo shawn.guo@linaro.org
Acked-by: Timur Tabi timur@freescale.com
On Fri, Feb 24, 2012 at 10:09:38PM +0800, Shawn Guo wrote:
Similar to what commit 1e3ad57 (ASoC: Remove redundant -codec from WM8776 driver name) does for wm8776 driver, this patch does the same thing for cs4270 driver.
Applied, thanks.
There are 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 | 153 ++++++++++++++++++++++++++++++++++++++++ sound/soc/fsl/fsl_utils.h | 29 ++++++++ sound/soc/fsl/mpc8610_hpcd.c | 157 +++--------------------------------------- sound/soc/fsl/p1022_ds.c | 149 +++------------------------------------- 6 files changed, 209 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 a62dc28..337c3d1 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 65087b1..46752af 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..90722aa --- /dev/null +++ b/sound/soc/fsl/fsl_utils.c @@ -0,0 +1,153 @@ +/** + * 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_i2c.h> +#include <sound/soc.h> + +#include "fsl_utils.h" + +/** + * 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; +} + +/** + * fsl_asoc_get_codec_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". + */ +int fsl_asoc_get_codec_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; +} +EXPORT_SYMBOL_GPL(fsl_asoc_get_codec_dev_name); + +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; + 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; +} +EXPORT_SYMBOL_GPL(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..c928a15 --- /dev/null +++ b/sound/soc/fsl/fsl_utils.h @@ -0,0 +1,29 @@ +/** + * 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; + +extern int fsl_asoc_get_codec_dev_name(struct device_node *np, + char *buf, size_t len); +extern 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;
Shawn Guo wrote:
There are 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
What repository is this for? When I apply it to Mark's for-next branch, I get this:
Applying: ASoC: fsl: create fsl_utils to accommodate the common functions error: patch failed: sound/soc/fsl/Kconfig:1 error: sound/soc/fsl/Kconfig: patch does not apply error: patch failed: sound/soc/fsl/Makefile:8 error: sound/soc/fsl/Makefile: patch does not apply Patch failed at 0004 ASoC: fsl: create fsl_utils to accommodate the common functions When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort".
Shawn Guo wrote:
There are some amount of code duplication between mpc8610_hpcd and
"There is"
diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile index 65087b1..46752af 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
I think it would be easier to do this:
obj-$(CONFIG_SND_SOC_FSL_SSI) += snd-soc-fsl-ssi.o snd-soc-fsl-utils.o obj-$(CONFIG_SND_SOC_POWERPC_DMA) += snd-soc-fsl-dma.o snd-soc-fsl-utils.o
Then you don't need to change the Kconfig.
+static struct device_node *get_node_by_phandle_name(struct device_node *np,
- const char *name, const char *compatible)
...
+static int get_parent_cell_index(struct device_node *np)
I think you should merge these two functions into their callers. There's not much point in keeping them separate now. That will also allow you to add dev_err() messages when returning error codes.
+/**
- fsl_asoc_get_codec_dev_name - determine the dev_name for a codec node
Can you add kerneldoc descriptions of the parameters?
* @np: pointer to the I2C device tree 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".
- */
+int fsl_asoc_get_codec_dev_name(struct device_node *np, char *buf, size_t len) +{
- const u32 *iprop;
- int addr;
'addr' should be a u32, actually. I'm not sure why I made it a signed integer.
+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)+{
Can you add kerneldoc comments to this function?
+MODULE_AUTHOR("Timur Tabi timur@freescale.com"); +MODULE_DESCRIPTION("Freescale ASoC utility code"); +MODULE_LICENSE("GPL v2");
Is this really a module?
diff --git a/sound/soc/fsl/fsl_utils.h b/sound/soc/fsl/fsl_utils.h new file mode 100644 index 0000000..c928a15 --- /dev/null +++ b/sound/soc/fsl/fsl_utils.h @@ -0,0 +1,29 @@ +/**
- 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;
+extern int fsl_asoc_get_codec_dev_name(struct device_node *np,
char *buf, size_t len);+extern 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);
No 'extern' for function prototypes, please.
On Mon, Feb 27, 2012 at 03:36:48PM -0600, Timur Tabi wrote:
Shawn Guo wrote:
There are some amount of code duplication between mpc8610_hpcd and
"There is"
Ok.
diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile index 65087b1..46752af 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
I think it would be easier to do this:
obj-$(CONFIG_SND_SOC_FSL_SSI) += snd-soc-fsl-ssi.o snd-soc-fsl-utils.o obj-$(CONFIG_SND_SOC_POWERPC_DMA) += snd-soc-fsl-dma.o snd-soc-fsl-utils.o
Since fsl_utils.c is created to accommodate the common functions used by mpc8610_hpcd.c and p1022_ds.c, I guess you actually meant something like the following?
# MPC8610 HPCD Machine Support snd-soc-mpc8610-hpcd-objs := mpc8610_hpcd.o fsl_utils.o obj-$(CONFIG_SND_SOC_MPC8610_HPCD) += snd-soc-mpc8610-hpcd.o
# P1022 DS Machine Support snd-soc-p1022-ds-objs := p1022_ds.o fsl_utils.o obj-$(CONFIG_SND_SOC_P1022_DS) += snd-soc-p1022-ds.o
I actually started with doing so, but later changed to make fsl_utils as a module. It seems that mpc8610_hpcd and p1022_ds will not be built in a single kernel image, but on ARM/IMX, it's very likely that multiple machine drivers will be built in a single image, as the ARM community is moving to support single image for multiple SoCs. In that case, fsl_utils can not be linked like that but has to be a module.
Then you don't need to change the Kconfig.
+static struct device_node *get_node_by_phandle_name(struct device_node *np,
- const char *name, const char *compatible)
...
+static int get_parent_cell_index(struct device_node *np)
I think you should merge these two functions into their callers. There's not much point in keeping them separate now. That will also allow you to add dev_err() messages when returning error codes.
Ok.
+/**
- fsl_asoc_get_codec_dev_name - determine the dev_name for a codec node
Can you add kerneldoc descriptions of the parameters?
Ok.
- @np: pointer to the I2C device tree 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".
- */
+int fsl_asoc_get_codec_dev_name(struct device_node *np, char *buf, size_t len) +{
- const u32 *iprop;
- int addr;
'addr' should be a u32, actually. I'm not sure why I made it a signed integer.
Ok.
+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)+{
Can you add kerneldoc comments to this function?
Ok.
+MODULE_AUTHOR("Timur Tabi timur@freescale.com"); +MODULE_DESCRIPTION("Freescale ASoC utility code"); +MODULE_LICENSE("GPL v2");
Is this really a module?
As I explained above, yes, it has to be a module.
diff --git a/sound/soc/fsl/fsl_utils.h b/sound/soc/fsl/fsl_utils.h new file mode 100644 index 0000000..c928a15 --- /dev/null +++ b/sound/soc/fsl/fsl_utils.h @@ -0,0 +1,29 @@ +/**
- 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;
+extern int fsl_asoc_get_codec_dev_name(struct device_node *np,
char *buf, size_t len);+extern 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);No 'extern' for function prototypes, please.
Ok.
Shawn Guo wrote:
Since fsl_utils.c is created to accommodate the common functions used by mpc8610_hpcd.c and p1022_ds.c, I guess you actually meant something like the following?
# MPC8610 HPCD Machine Support snd-soc-mpc8610-hpcd-objs := mpc8610_hpcd.o fsl_utils.o obj-$(CONFIG_SND_SOC_MPC8610_HPCD) += snd-soc-mpc8610-hpcd.o
# P1022 DS Machine Support snd-soc-p1022-ds-objs := p1022_ds.o fsl_utils.o obj-$(CONFIG_SND_SOC_P1022_DS) += snd-soc-p1022-ds.o
Yes. :-)
I actually started with doing so, but later changed to make fsl_utils as a module. It seems that mpc8610_hpcd and p1022_ds will not be built in a single kernel image, but on ARM/IMX, it's very likely that multiple machine drivers will be built in a single image, as the ARM community is moving to support single image for multiple SoCs. In that case, fsl_utils can not be linked like that but has to be a module.
Ah yes, good point.
Shawn Guo wrote:
+EXPORT_SYMBOL_GPL(fsl_asoc_get_codec_dev_name);
Please use EXPORT_SYMBOL() instead of EXPORT_SYMBOL_GPL(). Freescale does not want to impose license restrictions on its customers.
Also, I think there's a bug in this function that I never got around to fixing. I think you need to call put_device() somewhere. of_find_i2c_device_by_node() calls bus_find_device(), which calls get_device(), which increments the reference count.
Instead of run-time detecting driver name from device tree model property for binding the correct platform device created in fsl_ssi probe function, we can simply use platform_device_id table to match the p1022_ds device.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/fsl/p1022_ds.c | 36 +++++++++--------------------------- 1 files changed, 9 insertions(+), 27 deletions(-)
diff --git a/sound/soc/fsl/p1022_ds.c b/sound/soc/fsl/p1022_ds.c index 60f7bb8..a6f4a8a 100644 --- a/sound/soc/fsl/p1022_ds.c +++ b/sound/soc/fsl/p1022_ds.c @@ -408,10 +408,19 @@ static int __devexit p1022_ds_remove(struct platform_device *pdev) return 0; }
+static struct platform_device_id p1022_ds_ids[] = { + { .name = "snd-soc-p1022ds", }, + { .name = "snd-soc-p1022", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(platform, p1022_ds_ids); + static struct platform_driver p1022_ds_driver = { .probe = p1022_ds_probe, .remove = __devexit_p(p1022_ds_remove), + .id_table = p1022_ds_ids, .driver = { + .name = "p1022_ds", .owner = THIS_MODULE, }, }; @@ -425,33 +434,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 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.*/
Please retain this paragraph somewhere. It's important to explain why we're checking for both strings.
Shawn Guo wrote:
- /*
* 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.*/
Please retain this paragraph somewhere. It's important to explain why we're checking for both strings.
On Mon, Feb 27, 2012 at 03:39:17PM -0600, Timur Tabi wrote:
Shawn Guo wrote:
- /*
* 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.*/Please retain this paragraph somewhere. It's important to explain why we're checking for both strings.
Ok, it will be something like the following.
/* * 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. */ static struct platform_device_id p1022_ds_ids[] = { { .name = "snd-soc-p1022ds", }, { .name = "snd-soc-p1022", }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(platform, p1022_ds_ids);
The fsl_ssi probe function detects the client machine name using model property of the device tree. But it will not work for the ARM client machines which are to be added. The model property is given as a descriptive string on all ARM device tree (arch/arm/boot/*.dts). Instead, the compatible property gives the machine name.
It seems the fsl_ssi PowerPC users, machine mpc8610_hpcd and p1022ds, basically have the same thing in model and compatible properties. Detecting machine name using compatible property becomes a solution working for both PowerPC and ARM users of fsl_ssi.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/fsl/fsl_ssi.c | 6 +++--- sound/soc/fsl/mpc8610_hpcd.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c index 32fc84b..ff570a6 100644 --- a/sound/soc/fsl/fsl_ssi.c +++ b/sound/soc/fsl/fsl_ssi.c @@ -733,12 +733,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 7ead1e5..4195182 100644 --- a/sound/soc/fsl/mpc8610_hpcd.c +++ b/sound/soc/fsl/mpc8610_hpcd.c @@ -411,7 +411,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",
On Fri, Feb 24, 2012 at 10:09:41PM +0800, Shawn Guo wrote:
The fsl_ssi probe function detects the client machine name using model property of the device tree. But it will not work for the ARM client machines which are to be added. The model property is given as a descriptive string on all ARM device tree (arch/arm/boot/*.dts). Instead, the compatible property gives the machine name.
The ARM machines really shouldn't be using the bindings that PowerPC is using for the machine as they conflate the machine driver with the SSI binding. The idiom for new systems is that machine drivers should be first class devices in the device tree referencing the SSI and CODEC.
The current bindings should be preserved for existing systems of course.
Mark Brown wrote:
The ARM machines really shouldn't be using the bindings that PowerPC is using for the machine as they conflate the machine driver with the SSI binding. The idiom for new systems is that machine drivers should be first class devices in the device tree referencing the SSI and CODEC.
Where is this documented?
I don't remember participating in the discussion for this new binding.
The current bindings should be preserved for existing systems of course.
There's no reason we can't update the PowerPC device trees.
On Fri, Feb 24, 2012 at 10:30:05AM -0600, Timur Tabi wrote:
Mark Brown wrote:
The ARM machines really shouldn't be using the bindings that PowerPC is using for the machine as they conflate the machine driver with the SSI binding. The idiom for new systems is that machine drivers should be first class devices in the device tree referencing the SSI and CODEC.
Where is this documented?
Documentation/devicetree/bindings/sound/tegra*, I guess.
On Fri, Feb 24, 2012 at 10:30:05AM -0600, Timur Tabi wrote:
Mark Brown wrote:
The ARM machines really shouldn't be using the bindings that PowerPC is using for the machine as they conflate the machine driver with the SSI binding. The idiom for new systems is that machine drivers should be first class devices in the device tree referencing the SSI and CODEC.
Where is this documented?
I don't remember participating in the discussion for this new binding.
It's not a specific binding but rather a general idea of how these bindings should be done - I don't know how one would go about documenting general ideas like that, I have asked people to do that but I don't think there's a mechanism. There's a couple of such bindings in mainline at present (the nVidia ones).
I do recall you being involved in some of the discussions, though it's possible I have these confused with other device tree threads.
The current bindings should be preserved for existing systems of course.
There's no reason we can't update the PowerPC device trees.
We could, yes, but it would make people's existing device trees buggy which would be sad. It'd be nicer to keep the old binding around and support it for people who are already using it.
On Fri, Feb 24, 2012 at 02:12:07PM +0000, Mark Brown wrote:
On Fri, Feb 24, 2012 at 10:09:41PM +0800, Shawn Guo wrote:
The fsl_ssi probe function detects the client machine name using model property of the device tree. But it will not work for the ARM client machines which are to be added. The model property is given as a descriptive string on all ARM device tree (arch/arm/boot/*.dts). Instead, the compatible property gives the machine name.
The ARM machines really shouldn't be using the bindings that PowerPC is using for the machine as they conflate the machine driver with the SSI binding. The idiom for new systems is that machine drivers should be first class devices in the device tree referencing the SSI and CODEC.
The current bindings should be preserved for existing systems of course.
Per discussion [1], the whole point we move to fsl_ssi is that we can reuse the DT support already in fsl_ssi. If it's something we should not use for IMX, I fail to see why we should make this move.
Regards, Shawn
[1] http://article.gmane.org/gmane.linux.drivers.devicetree/6637
On Sat, Feb 25, 2012 at 09:28:47AM +0800, Shawn Guo wrote:
Per discussion [1], the whole point we move to fsl_ssi is that we can reuse the DT support already in fsl_ssi. If it's something we should not use for IMX, I fail to see why we should make this move.
I was assuming that the idea was to share the code for device control as the same IP block was being deployed in the various Freescale SoCs - if the only thing they have in common is the device tree support it would be a bit strange to merge them.
Note that it's purely the bit where the machine driver is instantiated from the SSI (which is just one or two of several properties) that I'm talking about here, the bindings for the SSI itself look good.
On Sat, Feb 25, 2012 at 11:42:28AM +0000, Mark Brown wrote:
On Sat, Feb 25, 2012 at 09:28:47AM +0800, Shawn Guo wrote:
Per discussion [1], the whole point we move to fsl_ssi is that we can reuse the DT support already in fsl_ssi. If it's something we should not use for IMX, I fail to see why we should make this move.
I was assuming that the idea was to share the code for device control as the same IP block was being deployed in the various Freescale SoCs
- if the only thing they have in common is the device tree support it
would be a bit strange to merge them.
Here is the thing.
For historical reason, there are two drivers for the same SSI block found on PowerPC and IMX, fsl_ssi and imx-ssi. The fsl_ssi driver is used on PowerPC with DT support, while imx-ssi is used on IMX with no DT support. Along with the work of supporting DT for the modern IMX SoCs, I was planning to add DT support into imx-ssi. But I was told by Timur that instead of doing that, I should reuse fsl_ssi for those DT aware IMX platforms. From his POV, one big reason of why imx-ssi was created is that ARM/IMX did not support DT before while fsl_ssi was born to support DT only. Since ARM/IMX starts supporting DT, fsl_ssi should be reused there.
I'm personally fine with either reusing fsl_ssi or adding DT support into imx-ssi. I do not see any problem with reusing fsl_ssi so far, but I can quickly turn around to adding new binding for imx-ssi if you are strong on this position.
Note that it's purely the bit where the machine driver is instantiated from the SSI (which is just one or two of several properties) that I'm talking about here, the bindings for the SSI itself look good.
Hmm, there is no SSI binding property contributing to instantiating the machine driver. It looks /model property for the device name used to instantiate the machine driver, which I personally think is fine.
On Sat, Feb 25, 2012 at 09:09:53PM +0800, Shawn Guo wrote:
DT aware IMX platforms. From his POV, one big reason of why imx-ssi was created is that ARM/IMX did not support DT before while fsl_ssi was born to support DT only. Since ARM/IMX starts supporting DT, fsl_ssi should be reused there.
No, that's not really a major reason - the big issue was more the lack of any support from Freescale for the i.MX in mainline at that time and the lack of any information on the similarities or differences between the IP implementations in the two processor families, there's quite a few quirks in the SSI register interface and it wasn't clear what was shared and what wasn't. It was more important to get things working in mainline, the hope was that someone would come along later and merge if that was possible (which you're now doing!).
I'm personally fine with either reusing fsl_ssi or adding DT support into imx-ssi. I do not see any problem with reusing fsl_ssi so far, but I can quickly turn around to adding new binding for imx-ssi if you are strong on this position.
Nobody's suggesting that they don't get merged if they really are the same IP and can use the same code. The device tree bindings are (and should be) a really small proportion of the code.
Note that it's purely the bit where the machine driver is instantiated from the SSI (which is just one or two of several properties) that I'm talking about here, the bindings for the SSI itself look good.
Hmm, there is no SSI binding property contributing to instantiating the machine driver. It looks /model property for the device name used to instantiate the machine driver, which I personally think is fine.
The CODEC binding for the machine driver is part of the SSI binding, and a mandatory one at that. It looks like the code should be refactored to register the SSI unconditionally and then optionally also register a machine driver if there's a CODEC property present. That way existing systems should continue to work fine with their current bindings and new ones can use the modern binding style.
Just using the plain machine name can get a bit inconvenient when you get reference designs cloned which share the audio but differ in other ways - the nVidia WM8903 driver is a good example of this, there's a large number of boards that are definitely distinct boards but are very similar from an audio point of view. You'd have to add all the machines to the machine driver compatible list.
Mark Brown wrote:
Nobody's suggesting that they don't get merged if they really are the same IP and can use the same code. The device tree bindings are (and should be) a really small proportion of the code.
My understanding is that they really are the same exact IP.
The CODEC binding for the machine driver is part of the SSI binding, and a mandatory one at that. It looks like the code should be refactored to register the SSI unconditionally
Ok.
and then optionally also register a machine driver if there's a CODEC property present.
Ah, that's not a bad idea. I like that.
Shawn Guo wrote:
It seems the fsl_ssi PowerPC users, machine mpc8610_hpcd and p1022ds, basically have the same thing in model and compatible properties. Detecting machine name using compatible property becomes a solution working for both PowerPC and ARM users of fsl_ssi.
Do you actually have an 8610 or P1022 to test this? I'm guessing the answer is no.
On Fri, Feb 24, 2012 at 10:32:50AM -0600, Timur Tabi wrote:
Shawn Guo wrote:
It seems the fsl_ssi PowerPC users, machine mpc8610_hpcd and p1022ds, basically have the same thing in model and compatible properties. Detecting machine name using compatible property becomes a solution working for both PowerPC and ARM users of fsl_ssi.
Do you actually have an 8610 or P1022 to test this? I'm guessing the answer is no.
No, I do not. I only compile-tested it. I need your favor here.
Shawn Guo wrote:
Do you actually have an 8610 or P1022 to test this? I'm guessing the answer is no.
No, I do not. I only compile-tested it. I need your favor here.
No problem. I will get to it as soon as I can.
Shawn Guo wrote:
The fsl_ssi probe function detects the client machine name using model property of the device tree. But it will not work for the ARM client machines which are to be added. The model property is given as a descriptive string on all ARM device tree (arch/arm/boot/*.dts). Instead, the compatible property gives the machine name.
If you apply this change to the P1022DS as well, then you can eliminate the need to check for "P1022" and "P1022DS", since the compatible string in the P1022DS DTS has always said, "fsl,P1022DS".
On Mon, Feb 27, 2012 at 03:54:01PM -0600, Timur Tabi wrote:
Shawn Guo wrote:
The fsl_ssi probe function detects the client machine name using model property of the device tree. But it will not work for the ARM client machines which are to be added. The model property is given as a descriptive string on all ARM device tree (arch/arm/boot/*.dts). Instead, the compatible property gives the machine name.
If you apply this change to the P1022DS as well, then you can eliminate the need to check for "P1022" and "P1022DS", since the compatible string in the P1022DS DTS has always said, "fsl,P1022DS".
Since I was asked by Mark to use new binding for new machine driver, I would drop this patch and keep PowerPC machine drivers working with /model property.
Shawn Guo wrote:
Since I was asked by Mark to use new binding for new machine driver, I would drop this patch and keep PowerPC machine drivers working with /model property.
I think Mark is only concerned with not breaking compatibility with existing device trees. Both the P1022 and MPC8610 device trees have had this compatible property.
On Tue, Feb 28, 2012 at 02:12:53AM +0000, Tabi Timur-B04825 wrote:
Shawn Guo wrote:
Since I was asked by Mark to use new binding for new machine driver, I would drop this patch and keep PowerPC machine drivers working with /model property.
I think Mark is only concerned with not breaking compatibility with existing device trees. Both the P1022 and MPC8610 device trees have had this compatible property.
Are you saying that both older and new P1022 device trees have compatible = "fsl,P1022DS", so that changing to check /compatible could save the check on /model of P1022 device tree? Otherwise, the check between "fsl,P1022" and "fsl,P1022DS" can not be saved.
Shawn Guo wrote:
Are you saying that both older and new P1022 device trees have compatible = "fsl,P1022DS", so that changing to check /compatible could save the check on /model of P1022 device tree?
Yes, that's what I'm saying.
You can verify for yourself by looking at the git history of the device trees.
On Tue, Feb 28, 2012 at 03:42:57AM +0000, Tabi Timur-B04825 wrote:
Shawn Guo wrote:
Are you saying that both older and new P1022 device trees have compatible = "fsl,P1022DS", so that changing to check /compatible could save the check on /model of P1022 device tree?
Yes, that's what I'm saying.
Ok, I will drop "[PATCH 5/6] ASoC: fsl: use platform_device_id table to match p1022_ds device" and move to this then.
Shawn Guo wrote:
This is the third series towards reusing fsl_ssi driver on device tree based IMX platforms. It makes a few cleanups on existing PowerPC code to ease the later addition of an IMX machine driver that works on fsl_ssi.
These patches don't apply 100% cleanly, and there are some issues that need to be resolved, but I'll review each one and ACK those that I think are okay. It would probably be better if none were applied until the whole patchset is okay, though.
participants (5)
-
Mark Brown -
Russell King - ARM Linux -
Shawn Guo -
Tabi Timur-B04825 -
Timur Tabi