No subject

No subject

[Xiubo Li]

Hello,

This patch series is mostly Freescale's SAI SoC Digital Audio Interface driver implementation. And the implementation is only compatible with device tree definition.

This patch series is based on linux-next and has been tested on Vybrid VF610 Tower board using device tree.

Changed in v2:
- Use default settings for the generic dmaengine PCM driver.
- Separate receive and transmit setting in most functions, but some couldn't for the HW limitation.
- Drop some not reduntant code.
- Use devm_snd_soc_register_component() instead of snd_soc_register_component().
- Use devm_snd_soc_register_card() instead of devm_snd_soc_register_card().
- Adjust the code sentences sequence.
- Make the namespacing consistent.
- Rename CONFIG_SND_SOC_FSL_SGTL5000 to CONFIG_SND_SOC_FSL_SGTL5000_VF610.
- Drop some meaningless lines.
- Rename the binding document file.

Added in v1:
- Add SAI SoC Digital Audio Interface driver.
- Add Freescale SAI ALSA SoC Digital Audio Interface node for VF610.
- Enables SAI ALSA SoC DAI device for Vybrid VF610 TOWER board.
- Add device tree bindings for Freescale SAI.
- Revise the bugs about the sgt15000 codec.
- Add SGT15000 based audio machine driver.
- Enable SGT15000 codec based audio driver node for VF610.
- Add device tree bindings for Freescale VF610 sound.

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Xiubo Li]

This adds Freescale SAI ASoC Audio support.
This implementation is only compatible with device tree definition.
Features:
o Supports playback/capture
o Supports 16/20/24 bit PCM
o Supports 8k - 96k sample rates
o Supports slave mode only.

Signed-off-by: Alison Wang <b18965 at freescale.com
Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 sound/soc/fsl/Kconfig   |  16 ++
 sound/soc/fsl/Makefile  |   5 +
 sound/soc/fsl/fsl-sai.c | 472 ++++++++++++++++++++++++++++++++++++++++++++++++
 sound/soc/fsl/fsl-sai.h | 120 ++++++++++++
 4 files changed, 613 insertions(+)
 create mode 100644 sound/soc/fsl/fsl-sai.c
 create mode 100644 sound/soc/fsl/fsl-sai.h

diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig
index b7ab71f..9a8851e 100644
--- a/sound/soc/fsl/Kconfig
+++ b/sound/soc/fsl/Kconfig
@@ -213,3 +213,19 @@ config SND_SOC_IMX_MC13783
 	select SND_SOC_IMX_PCM_DMA
 
 endif # SND_IMX_SOC
+
+menuconfig SND_FSL_SOC
+	tristate "SoC Audio for Freescale FSL CPUs"
+	help
+	  Say Y or M if you want to add support for codecs attached to
+	  the FSL CPUs.
+
+	  This will enable Freeacale SAI, SGT15000 codec.
+
+if SND_FSL_SOC
+
+config SND_SOC_FSL_SAI
+	tristate
+	select SND_SOC_GENERIC_DMAENGINE_PCM
+
+endif # SND_FSL_SOC
diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile
index 8db705b..e5acc03 100644
--- a/sound/soc/fsl/Makefile
+++ b/sound/soc/fsl/Makefile
@@ -56,3 +56,8 @@ obj-$(CONFIG_SND_SOC_IMX_SGTL5000) += snd-soc-imx-sgtl5000.o
 obj-$(CONFIG_SND_SOC_IMX_WM8962) += snd-soc-imx-wm8962.o
 obj-$(CONFIG_SND_SOC_IMX_SPDIF) += snd-soc-imx-spdif.o
 obj-$(CONFIG_SND_SOC_IMX_MC13783) += snd-soc-imx-mc13783.o
+
+# FSL ARM SAI/SGT15000 Platform Support
+snd-soc-fsl-sai-objs := fsl-sai.o
+
+obj-$(CONFIG_SND_SOC_FSL_SAI) += snd-soc-fsl-sai.o
diff --git a/sound/soc/fsl/fsl-sai.c b/sound/soc/fsl/fsl-sai.c
new file mode 100644
index 0000000..bb57e67
--- /dev/null
+++ b/sound/soc/fsl/fsl-sai.c
@@ -0,0 +1,472 @@
+/*
+ * Freescale SAI ALSA SoC Digital Audio Interface driver.
+ *
+ * Copyright 2012-2013 Freescale Semiconductor, Inc.
+ *
+ * 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/clk.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/of_address.h>
+#include <sound/core.h>
+#include <sound/pcm_params.h>
+#include <linux/delay.h>
+#include <linux/dmaengine.h>
+#include <sound/dmaengine_pcm.h>
+
+#include "fsl-sai.h"
+
+static int fsl_sai_set_dai_sysclk_tr(struct snd_soc_dai *cpu_dai,
+		int clk_id, unsigned int freq, int fsl_dir)
+{
+	u32 val_cr2, reg_cr2;
+	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
+
+	if (fsl_dir == FSL_FMT_TRANSMITTER)
+		reg_cr2 = FSL_SAI_TCR2;
+	else
+		reg_cr2 = FSL_SAI_RCR2;
+
+	val_cr2 = readl(sai->base + reg_cr2);
+	switch (clk_id) {
+	case FSL_SAI_CLK_BUS:
+		val_cr2 &= ~FSL_SAI_CR2_MSEL_MASK;
+		val_cr2 |= FSL_SAI_CR2_MSEL_BUS;
+		break;
+	case FSL_SAI_CLK_MAST1:
+		val_cr2 &= ~FSL_SAI_CR2_MSEL_MASK;
+		val_cr2 |= FSL_SAI_CR2_MSEL_MCLK1;
+		break;
+	case FSL_SAI_CLK_MAST2:
+		val_cr2 &= ~FSL_SAI_CR2_MSEL_MASK;
+		val_cr2 |= FSL_SAI_CR2_MSEL_MCLK2;
+		break;
+	case FSL_SAI_CLK_MAST3:
+		val_cr2 &= ~FSL_SAI_CR2_MSEL_MASK;
+		val_cr2 |= FSL_SAI_CR2_MSEL_MCLK3;
+		break;
+	default:
+		return -EINVAL;
+	}
+	writel(val_cr2, sai->base + reg_cr2);
+
+	return 0;
+}
+
+static int fsl_sai_set_dai_sysclk(struct snd_soc_dai *cpu_dai,
+		int clk_id, unsigned int freq, int dir)
+{
+	int ret;
+
+	if (dir == SND_SOC_CLOCK_IN)
+		return 0;
+
+	ret = fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq,
+					FSL_FMT_TRANSMITTER);
+	if (ret) {
+		dev_err(cpu_dai->dev,
+				"Cannot set sai's transmitter sysclk: %d\n",
+				ret);
+		return ret;
+	}
+
+	ret = fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq,
+					FSL_FMT_RECEIVER);
+	if (ret) {
+		dev_err(cpu_dai->dev,
+				"Cannot set sai's receiver sysclk: %d\n",
+				ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int fsl_sai_set_dai_clkdiv(struct snd_soc_dai *cpu_dai,
+		int div_id, int div)
+{
+	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
+	u32 tcr2, rcr2;
+
+	if (div_id == FSL_SAI_TX_DIV) {
+		tcr2 = readl(sai->base + FSL_SAI_TCR2);
+		tcr2 &= ~FSL_SAI_CR2_DIV_MASK;
+		tcr2 |= FSL_SAI_CR2_DIV(div);
+		writel(tcr2, sai->base + FSL_SAI_TCR2);
+
+	} else if (div_id == FSL_SAI_RX_DIV) {
+		rcr2 = readl(sai->base + FSL_SAI_RCR2);
+		rcr2 &= ~FSL_SAI_CR2_DIV_MASK;
+		rcr2 |= FSL_SAI_CR2_DIV(div);
+		writel(rcr2, sai->base + FSL_SAI_RCR2);
+
+	} else
+		return -EINVAL;
+
+	return 0;
+}
+
+static int fsl_sai_set_dai_fmt_tr(struct snd_soc_dai *cpu_dai,
+				unsigned int fmt, int fsl_dir)
+{
+	u32 val_cr2, val_cr3, val_cr4, reg_cr2, reg_cr3, reg_cr4;
+	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
+
+	if (fsl_dir == FSL_FMT_TRANSMITTER) {
+		reg_cr2 = FSL_SAI_TCR2;
+		reg_cr3 = FSL_SAI_TCR3;
+		reg_cr4 = FSL_SAI_TCR4;
+	} else {
+		reg_cr2 = FSL_SAI_RCR2;
+		reg_cr3 = FSL_SAI_RCR3;
+		reg_cr4 = FSL_SAI_RCR4;
+	}
+
+	val_cr2 = readl(sai->base + reg_cr2);
+	val_cr3 = readl(sai->base + reg_cr3);
+	val_cr4 = readl(sai->base + reg_cr4);
+
+	if (sai->fbt == FSL_SAI_FBT_MSB)
+		val_cr4 |= FSL_SAI_CR4_MF;
+	else if (sai->fbt == FSL_SAI_FBT_LSB)
+		val_cr4 &= ~FSL_SAI_CR4_MF;
+
+	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_I2S:
+		val_cr4 |= FSL_SAI_CR4_FSE;
+		val_cr4 |= FSL_SAI_CR4_FSP;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
+	case SND_SOC_DAIFMT_IB_IF:
+		val_cr4 |= FSL_SAI_CR4_FSP;
+		val_cr2 &= ~FSL_SAI_CR2_BCP;
+		break;
+	case SND_SOC_DAIFMT_IB_NF:
+		val_cr4 &= ~FSL_SAI_CR4_FSP;
+		val_cr2 &= ~FSL_SAI_CR2_BCP;
+		break;
+	case SND_SOC_DAIFMT_NB_IF:
+		val_cr4 |= FSL_SAI_CR4_FSP;
+		val_cr2 |= FSL_SAI_CR2_BCP;
+		break;
+	case SND_SOC_DAIFMT_NB_NF:
+		val_cr4 &= ~FSL_SAI_CR4_FSP;
+		val_cr2 |= FSL_SAI_CR2_BCP;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
+	case SND_SOC_DAIFMT_CBS_CFS:
+		val_cr2 |= FSL_SAI_CR2_BCD_MSTR;
+		val_cr4 |= FSL_SAI_CR4_FSD_MSTR;
+		break;
+	case SND_SOC_DAIFMT_CBM_CFM:
+		val_cr2 &= ~FSL_SAI_CR2_BCD_MSTR;
+		val_cr4 &= ~FSL_SAI_CR4_FSD_MSTR;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	val_cr3 |= FSL_SAI_CR3_TRCE;
+
+	if (fsl_dir == FSL_FMT_RECEIVER)
+		val_cr2 |= FSL_SAI_CR2_SYNC;
+
+	writel(val_cr2, sai->base + reg_cr2);
+	writel(val_cr3, sai->base + reg_cr3);
+	writel(val_cr4, sai->base + reg_cr4);
+
+	return 0;
+
+}
+
+static int fsl_sai_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
+{
+	int ret;
+
+	ret = fsl_sai_set_dai_fmt_tr(cpu_dai, fmt, FSL_FMT_TRANSMITTER);
+	if (ret) {
+		dev_err(cpu_dai->dev,
+				"Cannot set sai's transmitter format: %d\n",
+				ret);
+		return ret;
+	}
+
+	ret = fsl_sai_set_dai_fmt_tr(cpu_dai, fmt, FSL_FMT_RECEIVER);
+	if (ret) {
+		dev_err(cpu_dai->dev,
+				"Cannot set sai's receiver format: %d\n",
+				ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int fsl_sai_set_dai_tdm_slot(struct snd_soc_dai *cpu_dai,
+		unsigned int tx_mask, unsigned int rx_mask,
+		int slots, int slot_width)
+{
+	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
+	u32 tcr4, rcr4;
+
+	tcr4 = readl(sai->base + FSL_SAI_TCR4);
+	tcr4 &= ~FSL_SAI_CR4_FRSZ_MASK;
+	tcr4 |= FSL_SAI_CR4_FRSZ(2);
+	writel(tcr4, sai->base + FSL_SAI_TCR4);
+	writel(tx_mask, sai->base + FSL_SAI_TMR);
+
+	rcr4 = readl(sai->base + FSL_SAI_RCR4);
+	rcr4 &= ~FSL_SAI_CR4_FRSZ_MASK;
+	rcr4 |= FSL_SAI_CR4_FRSZ(2);
+	writel(rcr4, sai->base + FSL_SAI_RCR4);
+	writel(rx_mask, sai->base + FSL_SAI_RMR);
+
+	return 0;
+}
+
+static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
+		struct snd_pcm_hw_params *params,
+		struct snd_soc_dai *cpu_dai)
+{
+	u32 val_cr4, val_cr5, reg_cr4, reg_cr5, word_width;
+	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		reg_cr4 = FSL_SAI_TCR4;
+		reg_cr5 = FSL_SAI_TCR5;
+	} else {
+		reg_cr4 = FSL_SAI_RCR4;
+		reg_cr5 = FSL_SAI_RCR5;
+	}
+
+	val_cr4 = readl(sai->base + reg_cr4);
+	val_cr4 &= ~FSL_SAI_CR4_SYWD_MASK;
+
+	val_cr5 = readl(sai->base + reg_cr5);
+	val_cr5 &= ~FSL_SAI_CR5_WNW_MASK;
+	val_cr5 &= ~FSL_SAI_CR5_W0W_MASK;
+	val_cr5 &= ~FSL_SAI_CR5_FBT_MASK;
+
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		word_width = 16;
+		break;
+	case SNDRV_PCM_FORMAT_S20_3LE:
+		word_width = 20;
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		word_width = 24;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	val_cr4 |= FSL_SAI_CR4_SYWD(word_width);
+	val_cr5 |= FSL_SAI_CR5_WNW(word_width);
+	val_cr5 |= FSL_SAI_CR5_W0W(word_width);
+
+	if (sai->fbt == FSL_SAI_FBT_MSB)
+		val_cr5 |= FSL_SAI_CR5_FBT(word_width - 1);
+	else if (sai->fbt == FSL_SAI_FBT_LSB)
+		val_cr5 |= FSL_SAI_CR5_FBT(0);
+
+	writel(val_cr4, sai->base + reg_cr4);
+	writel(val_cr5, sai->base + reg_cr5);
+
+	return 0;
+}
+
+static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
+		struct snd_soc_dai *dai)
+{
+	struct fsl_sai *sai = snd_soc_dai_get_drvdata(dai);
+	unsigned int tcsr, rcsr;
+
+	tcsr = readl(sai->base + FSL_SAI_TCSR);
+	rcsr = readl(sai->base + FSL_SAI_RCSR);
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		tcsr |= FSL_SAI_CSR_FRDE;
+		rcsr &= ~FSL_SAI_CSR_FRDE;
+	} else {
+		rcsr |= FSL_SAI_CSR_FRDE;
+		tcsr &= ~FSL_SAI_CSR_FRDE;
+	}
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		tcsr |= FSL_SAI_CSR_TERE;
+		rcsr |= FSL_SAI_CSR_TERE;
+		writel(rcsr, sai->base + FSL_SAI_RCSR);
+		udelay(10);
+		writel(tcsr, sai->base + FSL_SAI_TCSR);
+		break;
+
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		if (!(dai->playback_active || dai->capture_active)) {
+			tcsr &= ~FSL_SAI_CSR_TERE;
+			rcsr &= ~FSL_SAI_CSR_TERE;
+		}
+		writel(rcsr, sai->base + FSL_SAI_RCSR);
+		udelay(10);
+		writel(tcsr, sai->base + FSL_SAI_TCSR);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct snd_soc_dai_ops fsl_sai_pcm_dai_ops = {
+	.set_sysclk	= fsl_sai_set_dai_sysclk,
+	.set_clkdiv	= fsl_sai_set_dai_clkdiv,
+	.set_fmt	= fsl_sai_set_dai_fmt,
+	.set_tdm_slot	= fsl_sai_set_dai_tdm_slot,
+	.hw_params	= fsl_sai_hw_params,
+	.trigger	= fsl_sai_trigger,
+};
+
+static int fsl_sai_dai_probe(struct snd_soc_dai *dai)
+{
+	int ret;
+	struct fsl_sai *sai = dev_get_drvdata(dai->dev);
+
+	ret = clk_prepare_enable(sai->clk);
+	if (ret)
+		return ret;
+
+	writel(0x0, sai->base + FSL_SAI_RCSR);
+	writel(0x0, sai->base + FSL_SAI_TCSR);
+	writel(sai->dma_params_tx.maxburst, sai->base + FSL_SAI_TCR1);
+	writel(sai->dma_params_rx.maxburst, sai->base + FSL_SAI_RCR1);
+
+	dai->playback_dma_data = &sai->dma_params_tx;
+	dai->capture_dma_data = &sai->dma_params_rx;
+
+	snd_soc_dai_set_drvdata(dai, sai);
+
+	return 0;
+}
+
+int fsl_sai_dai_remove(struct snd_soc_dai *dai)
+{
+	struct fsl_sai *sai = dev_get_drvdata(dai->dev);
+
+	clk_disable_unprepare(sai->clk);
+
+	return 0;
+}
+
+static struct snd_soc_dai_driver fsl_sai_dai = {
+	.probe = fsl_sai_dai_probe,
+	.remove = fsl_sai_dai_remove,
+	.playback = {
+		.channels_min = 1,
+		.channels_max = 2,
+		.rates = SNDRV_PCM_RATE_8000_96000,
+		.formats = FSL_SAI_FORMATS,
+	},
+	.capture = {
+		.channels_min = 1,
+		.channels_max = 2,
+		.rates = SNDRV_PCM_RATE_8000_96000,
+		.formats = FSL_SAI_FORMATS,
+	},
+	.ops = &fsl_sai_pcm_dai_ops,
+};
+
+static const struct snd_soc_component_driver fsl_component = {
+	.name           = "fsl-sai",
+};
+
+static int fsl_sai_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct fsl_sai *sai;
+	int ret = 0;
+
+	sai = devm_kzalloc(&pdev->dev, sizeof(*sai), GFP_KERNEL);
+	if (!sai)
+		return -ENOMEM;
+
+	sai->fbt = FSL_SAI_FBT_MSB;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	sai->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(sai->base))
+		return PTR_ERR(sai->base);
+
+	sai->clk = devm_clk_get(&pdev->dev, "sai");
+	if (IS_ERR(sai->clk)) {
+		dev_err(&pdev->dev, "Cannot get sai's clock: %d\n", ret);
+		return PTR_ERR(sai->clk);
+	}
+
+	sai->dma_params_rx.addr = res->start + FSL_SAI_RDR;
+	sai->dma_params_rx.maxburst = 6;
+
+	sai->dma_params_tx.addr = res->start + FSL_SAI_TDR;
+	sai->dma_params_tx.maxburst = 6;
+
+	platform_set_drvdata(pdev, sai);
+
+	ret = devm_snd_soc_register_component(&pdev->dev, &fsl_component,
+			&fsl_sai_dai, 1);
+	if (ret)
+		return ret;
+
+	ret = snd_dmaengine_pcm_register(&pdev->dev, NULL,
+			SND_DMAENGINE_PCM_FLAG_NO_RESIDUE);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int fsl_sai_remove(struct platform_device *pdev)
+{
+	snd_dmaengine_pcm_unregister(&pdev->dev);
+
+	return 0;
+}
+
+static const struct of_device_id fsl_sai_ids[] = {
+	{ .compatible = "fsl,vf610-sai", },
+	{ /*sentinel*/ }
+};
+
+static struct platform_driver fsl_sai_driver = {
+	.probe = fsl_sai_probe,
+	.remove = fsl_sai_remove,
+
+	.driver = {
+		.name = "fsl-sai",
+		.owner = THIS_MODULE,
+		.of_match_table = fsl_sai_ids,
+	},
+};
+module_platform_driver(fsl_sai_driver);
+
+MODULE_AUTHOR("Xiubo Li, <Li.Xiubo@freescale.com>");
+MODULE_AUTHOR("Alison Wang, <b18965@freescale.com>");
+MODULE_DESCRIPTION("Freescale Soc SAI Interface");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/fsl/fsl-sai.h b/sound/soc/fsl/fsl-sai.h
new file mode 100644
index 0000000..1637679
--- /dev/null
+++ b/sound/soc/fsl/fsl-sai.h
@@ -0,0 +1,120 @@
+/*
+ * Copyright 2012-2013 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __FSL_SAI_H
+#define __FSL_SAI_H
+
+#include <sound/dmaengine_pcm.h>
+
+#define FSL_SAI_FORMATS (SNDRV_PCM_FMTBIT_S16_LE |\
+			 SNDRV_PCM_FMTBIT_S20_3LE |\
+			 SNDRV_PCM_FMTBIT_S24_LE)
+
+/* SAI Transmit/Recieve Control Register */
+#define FSL_SAI_TCSR		0x00
+#define FSL_SAI_RCSR		0x80
+#define FSL_SAI_CSR_TERE	BIT(31)
+#define FSL_SAI_CSR_FWF		BIT(17)
+#define FSL_SAI_CSR_FRIE	BIT(8)
+#define FSL_SAI_CSR_FRDE	BIT(0)
+
+/* SAI Transmit Data/FIFO/MASK Register */
+#define FSL_SAI_TDR		0x20
+#define FSL_SAI_TFR		0x40
+#define FSL_SAI_TMR		0x60
+
+/* SAI Recieve Data/FIFO/MASK Register */
+#define FSL_SAI_RDR		0xa0
+#define FSL_SAI_RFR		0xc0
+#define FSL_SAI_RMR		0xe0
+
+/* SAI Transmit and Recieve Configuration 1 Register */
+#define FSL_SAI_TCR1		0x04
+#define FSL_SAI_RCR1		0x84
+
+/* SAI Transmit and Recieve Configuration 2 Register */
+#define FSL_SAI_TCR2		0x08
+#define FSL_SAI_RCR2		0x88
+#define FSL_SAI_CR2_SYNC	BIT(30)
+#define FSL_SAI_CR2_MSEL_MASK	(0xff << 26)
+#define FSL_SAI_CR2_MSEL_BUS	0
+#define FSL_SAI_CR2_MSEL_MCLK1	BIT(26)
+#define FSL_SAI_CR2_MSEL_MCLK2	BIT(27)
+#define FSL_SAI_CR2_MSEL_MCLK3	(BIT(26) | BIT(27))
+#define FSL_SAI_CR2_BCP		BIT(25)
+#define FSL_SAI_CR2_BCD_MSTR	BIT(24)
+#define FSL_SAI_CR2_DIV(x)	(x)
+#define FSL_SAI_CR2_DIV_MASK	0xff
+
+/* SAI Transmit and Recieve Configuration 3 Register */
+#define FSL_SAI_TCR3		0x0c
+#define FSL_SAI_RCR3		0x8c
+#define FSL_SAI_CR3_TRCE	BIT(16)
+#define FSL_SAI_CR3_WDFL(x)	(x)
+#define FSL_SAI_CR3_WDFL_MASK	0x1f
+
+/* SAI Transmit and Recieve Configuration 4 Register */
+#define FSL_SAI_TCR4		0x10
+#define FSL_SAI_RCR4		0x90
+#define FSL_SAI_CR4_FRSZ(x)	(((x) - 1) << 16)
+#define FSL_SAI_CR4_FRSZ_MASK	(0x1f << 16)
+#define FSL_SAI_CR4_SYWD(x)	(((x) - 1) << 8)
+#define FSL_SAI_CR4_SYWD_MASK	(0x1f << 8)
+#define FSL_SAI_CR4_MF		BIT(4)
+#define FSL_SAI_CR4_FSE		BIT(3)
+#define FSL_SAI_CR4_FSP		BIT(1)
+#define FSL_SAI_CR4_FSD_MSTR	BIT(0)
+
+/* SAI Transmit and Recieve Configuration 5 Register */
+#define FSL_SAI_TCR5		0x14
+#define FSL_SAI_RCR5		0x94
+#define FSL_SAI_CR5_WNW(x)	(((x) - 1) << 24)
+#define FSL_SAI_CR5_WNW_MASK	(0x1f << 24)
+#define FSL_SAI_CR5_W0W(x)	(((x) - 1) << 16)
+#define FSL_SAI_CR5_W0W_MASK	(0x1f << 16)
+#define FSL_SAI_CR5_FBT(x)	((x) << 8)
+#define FSL_SAI_CR5_FBT_MASK	(0x1f << 8)
+
+/* SAI audio dividers */
+#define FSL_SAI_TX_DIV		0
+#define FSL_SAI_RX_DIV		1
+
+/* SAI type */
+#define FSL_SAI_DMA		BIT(0)
+#define FSL_SAI_USE_AC97	BIT(1)
+#define FSL_SAI_NET		BIT(2)
+#define FSL_SAI_TRA_SYN		BIT(3)
+#define FSL_SAI_REC_SYN		BIT(4)
+#define FSL_SAI_USE_I2S_SLAVE	BIT(5)
+
+#define FSL_FMT_TRANSMITTER	0
+#define FSL_FMT_RECEIVER	1
+
+/* SAI clock sources */
+#define FSL_SAI_CLK_BUS		0
+#define FSL_SAI_CLK_MAST1	1
+#define FSL_SAI_CLK_MAST2	2
+#define FSL_SAI_CLK_MAST3	3
+
+enum fsl_sai_fbt {
+	FSL_SAI_FBT_MSB,
+	FSL_SAI_FBT_LSB,
+};
+
+struct fsl_sai {
+	struct clk *clk;
+
+	void __iomem *base;
+
+	enum fsl_sai_fbt fbt;
+
+	struct snd_dmaengine_dai_dma_data dma_params_rx;
+	struct snd_dmaengine_dai_dma_data dma_params_tx;
+};
+
+#endif /* __FSL_SAI_H */
-- 
1.8.4

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Nicolin Chen]

Hi Xiubo,

On Fri, Nov 01, 2013 at 03:04:48PM +0800, Xiubo Li wrote:
> This adds Freescale SAI ASoC Audio support.
> This implementation is only compatible with device tree definition.
> Features:
> o Supports playback/capture
> o Supports 16/20/24 bit PCM
> o Supports 8k - 96k sample rates
> o Supports slave mode only.
> 

Just for curiosity, I found there're quite a bit code actually support
I2S master mode such as set_sysclk() and register configuration to FMT
SND_SOC_DAIFMT_CBS_CFS.

Is that really "supports slave mode only"? Or just you haven't make
it properly work?

> diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig
> index b7ab71f..9a8851e 100644
> --- a/sound/soc/fsl/Kconfig
> +++ b/sound/soc/fsl/Kconfig
> @@ -213,3 +213,19 @@ config SND_SOC_IMX_MC13783
>  	select SND_SOC_IMX_PCM_DMA
>  
>  endif # SND_IMX_SOC
> +
> +menuconfig SND_FSL_SOC
> +	tristate "SoC Audio for Freescale FSL CPUs"
> +	help
> +	  Say Y or M if you want to add support for codecs attached to
> +	  the FSL CPUs.
> +
> +	  This will enable Freeacale SAI, SGT15000 codec.
> +
> +if SND_FSL_SOC

Why check this here? SAI should be a regular IP module which can be used by
other SoC as well. We would never know if next i.MX SoC won't contain SAI.

> +
> +config SND_SOC_FSL_SAI
> +	tristate
> +	select SND_SOC_GENERIC_DMAENGINE_PCM

I think you should keep SAI beside SSI/SPDIF directly.

> +
> +endif # SND_FSL_SOC
> diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile
> index 8db705b..e5acc03 100644
> --- a/sound/soc/fsl/Makefile
> +++ b/sound/soc/fsl/Makefile
> @@ -56,3 +56,8 @@ obj-$(CONFIG_SND_SOC_IMX_SGTL5000) += snd-soc-imx-sgtl5000.o
>  obj-$(CONFIG_SND_SOC_IMX_WM8962) += snd-soc-imx-wm8962.o
>  obj-$(CONFIG_SND_SOC_IMX_SPDIF) += snd-soc-imx-spdif.o
>  obj-$(CONFIG_SND_SOC_IMX_MC13783) += snd-soc-imx-mc13783.o
> +
> +# FSL ARM SAI/SGT15000 Platform Support

Should be SGTL5000, not SGT1. And SAI should not be used with SGTL5000 only,
it's a bit confusing to mention SGTL5000 here.

> +snd-soc-fsl-sai-objs := fsl-sai.o

And I think it should be better to put it along with fsl-ssi.o and fsl-spdif.o

> +
> +obj-$(CONFIG_SND_SOC_FSL_SAI) += snd-soc-fsl-sai.o

Ditto

> diff --git a/sound/soc/fsl/fsl-sai.c b/sound/soc/fsl/fsl-sai.c
> new file mode 100644
> index 0000000..bb57e67
> --- /dev/null
> +++ b/sound/soc/fsl/fsl-sai.c
> @@ -0,0 +1,472 @@
> +/*
> + * Freescale SAI ALSA SoC Digital Audio Interface driver.
> + *
> + * Copyright 2012-2013 Freescale Semiconductor, Inc.
> + *
> + * 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.

There're too many double space inside. Could you pls revise it?

> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/of_address.h>
> +#include <sound/core.h>
> +#include <sound/pcm_params.h>
> +#include <linux/delay.h>
> +#include <linux/dmaengine.h>
> +#include <sound/dmaengine_pcm.h>

I think it's better to keep <sound/xxx.h> together. And basically
we can keep header files ordered by alphabet.

> +
> +#include "fsl-sai.h"
> +
> +static int fsl_sai_set_dai_sysclk_tr(struct snd_soc_dai *cpu_dai,
> +		int clk_id, unsigned int freq, int fsl_dir)
> +{
> +	u32 val_cr2, reg_cr2;
> +	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
> +
> +	if (fsl_dir == FSL_FMT_TRANSMITTER)
> +		reg_cr2 = FSL_SAI_TCR2;
> +	else
> +		reg_cr2 = FSL_SAI_RCR2;
> +
> +	val_cr2 = readl(sai->base + reg_cr2);
> +	switch (clk_id) {
> +	case FSL_SAI_CLK_BUS:
> +		val_cr2 &= ~FSL_SAI_CR2_MSEL_MASK;
> +		val_cr2 |= FSL_SAI_CR2_MSEL_BUS;
> +		break;
> +	case FSL_SAI_CLK_MAST1:
> +		val_cr2 &= ~FSL_SAI_CR2_MSEL_MASK;
> +		val_cr2 |= FSL_SAI_CR2_MSEL_MCLK1;
> +		break;
> +	case FSL_SAI_CLK_MAST2:
> +		val_cr2 &= ~FSL_SAI_CR2_MSEL_MASK;
> +		val_cr2 |= FSL_SAI_CR2_MSEL_MCLK2;
> +		break;
> +	case FSL_SAI_CLK_MAST3:
> +		val_cr2 &= ~FSL_SAI_CR2_MSEL_MASK;
> +		val_cr2 |= FSL_SAI_CR2_MSEL_MCLK3;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	writel(val_cr2, sai->base + reg_cr2);
> +
> +	return 0;
> +}
> +
> +static int fsl_sai_set_dai_sysclk(struct snd_soc_dai *cpu_dai,
> +		int clk_id, unsigned int freq, int dir)
> +{
> +	int ret;
> +
> +	if (dir == SND_SOC_CLOCK_IN)
> +		return 0;
> +
> +	ret = fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq,
> +					FSL_FMT_TRANSMITTER);
> +	if (ret) {
> +		dev_err(cpu_dai->dev,
> +				"Cannot set sai's transmitter sysclk: %d\n",
> +				ret);
> +		return ret;
> +	}
> +
> +	ret = fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq,
> +					FSL_FMT_RECEIVER);
> +	if (ret) {
> +		dev_err(cpu_dai->dev,
> +				"Cannot set sai's receiver sysclk: %d\n",
> +				ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fsl_sai_set_dai_clkdiv(struct snd_soc_dai *cpu_dai,
> +		int div_id, int div)
> +{
> +	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
> +	u32 tcr2, rcr2;
> +
> +	if (div_id == FSL_SAI_TX_DIV) {
> +		tcr2 = readl(sai->base + FSL_SAI_TCR2);
> +		tcr2 &= ~FSL_SAI_CR2_DIV_MASK;
> +		tcr2 |= FSL_SAI_CR2_DIV(div);
> +		writel(tcr2, sai->base + FSL_SAI_TCR2);
> +
> +	} else if (div_id == FSL_SAI_RX_DIV) {
> +		rcr2 = readl(sai->base + FSL_SAI_RCR2);
> +		rcr2 &= ~FSL_SAI_CR2_DIV_MASK;
> +		rcr2 |= FSL_SAI_CR2_DIV(div);
> +		writel(rcr2, sai->base + FSL_SAI_RCR2);
> +
> +	} else
> +		return -EINVAL;

It would look better if using switch-case. And the last 'else'
could be 'default:'.

> +
> +	return 0;
> +}
> +
> +static int fsl_sai_set_dai_fmt_tr(struct snd_soc_dai *cpu_dai,
> +				unsigned int fmt, int fsl_dir)
> +{
> +	u32 val_cr2, val_cr3, val_cr4, reg_cr2, reg_cr3, reg_cr4;
> +	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
> +
> +	if (fsl_dir == FSL_FMT_TRANSMITTER) {
> +		reg_cr2 = FSL_SAI_TCR2;
> +		reg_cr3 = FSL_SAI_TCR3;
> +		reg_cr4 = FSL_SAI_TCR4;
> +	} else {
> +		reg_cr2 = FSL_SAI_RCR2;
> +		reg_cr3 = FSL_SAI_RCR3;
> +		reg_cr4 = FSL_SAI_RCR4;
> +	}
> +
> +	val_cr2 = readl(sai->base + reg_cr2);
> +	val_cr3 = readl(sai->base + reg_cr3);
> +	val_cr4 = readl(sai->base + reg_cr4);
> +
> +	if (sai->fbt == FSL_SAI_FBT_MSB)
> +		val_cr4 |= FSL_SAI_CR4_MF;
> +	else if (sai->fbt == FSL_SAI_FBT_LSB)
> +		val_cr4 &= ~FSL_SAI_CR4_MF;
> +
> +	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +	case SND_SOC_DAIFMT_I2S:
> +		val_cr4 |= FSL_SAI_CR4_FSE;
> +		val_cr4 |= FSL_SAI_CR4_FSP;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> +	case SND_SOC_DAIFMT_IB_IF:
> +		val_cr4 |= FSL_SAI_CR4_FSP;
> +		val_cr2 &= ~FSL_SAI_CR2_BCP;
> +		break;
> +	case SND_SOC_DAIFMT_IB_NF:
> +		val_cr4 &= ~FSL_SAI_CR4_FSP;
> +		val_cr2 &= ~FSL_SAI_CR2_BCP;
> +		break;
> +	case SND_SOC_DAIFMT_NB_IF:
> +		val_cr4 |= FSL_SAI_CR4_FSP;
> +		val_cr2 |= FSL_SAI_CR2_BCP;
> +		break;
> +	case SND_SOC_DAIFMT_NB_NF:
> +		val_cr4 &= ~FSL_SAI_CR4_FSP;
> +		val_cr2 |= FSL_SAI_CR2_BCP;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> +	case SND_SOC_DAIFMT_CBS_CFS:
> +		val_cr2 |= FSL_SAI_CR2_BCD_MSTR;
> +		val_cr4 |= FSL_SAI_CR4_FSD_MSTR;
> +		break;
> +	case SND_SOC_DAIFMT_CBM_CFM:
> +		val_cr2 &= ~FSL_SAI_CR2_BCD_MSTR;
> +		val_cr4 &= ~FSL_SAI_CR4_FSD_MSTR;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	val_cr3 |= FSL_SAI_CR3_TRCE;
> +
> +	if (fsl_dir == FSL_FMT_RECEIVER)
> +		val_cr2 |= FSL_SAI_CR2_SYNC;
> +
> +	writel(val_cr2, sai->base + reg_cr2);
> +	writel(val_cr3, sai->base + reg_cr3);
> +	writel(val_cr4, sai->base + reg_cr4);
> +
> +	return 0;

> +

Pls drop this extra line.

> +}
> +
> +static int fsl_sai_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
> +{
> +	int ret;
> +
> +	ret = fsl_sai_set_dai_fmt_tr(cpu_dai, fmt, FSL_FMT_TRANSMITTER);
> +	if (ret) {
> +		dev_err(cpu_dai->dev,
> +				"Cannot set sai's transmitter format: %d\n",
> +				ret);
> +		return ret;
> +	}
> +
> +	ret = fsl_sai_set_dai_fmt_tr(cpu_dai, fmt, FSL_FMT_RECEIVER);
> +	if (ret) {
> +		dev_err(cpu_dai->dev,
> +				"Cannot set sai's receiver format: %d\n",
> +				ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fsl_sai_set_dai_tdm_slot(struct snd_soc_dai *cpu_dai,
> +		unsigned int tx_mask, unsigned int rx_mask,
> +		int slots, int slot_width)
> +{
> +	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
> +	u32 tcr4, rcr4;
> +
> +	tcr4 = readl(sai->base + FSL_SAI_TCR4);
> +	tcr4 &= ~FSL_SAI_CR4_FRSZ_MASK;
> +	tcr4 |= FSL_SAI_CR4_FRSZ(2);
> +	writel(tcr4, sai->base + FSL_SAI_TCR4);
> +	writel(tx_mask, sai->base + FSL_SAI_TMR);
> +
> +	rcr4 = readl(sai->base + FSL_SAI_RCR4);
> +	rcr4 &= ~FSL_SAI_CR4_FRSZ_MASK;
> +	rcr4 |= FSL_SAI_CR4_FRSZ(2);
> +	writel(rcr4, sai->base + FSL_SAI_RCR4);
> +	writel(rx_mask, sai->base + FSL_SAI_RMR);
> +
> +	return 0;
> +}
> +
> +static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
> +		struct snd_pcm_hw_params *params,
> +		struct snd_soc_dai *cpu_dai)
> +{
> +	u32 val_cr4, val_cr5, reg_cr4, reg_cr5, word_width;
> +	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		reg_cr4 = FSL_SAI_TCR4;
> +		reg_cr5 = FSL_SAI_TCR5;
> +	} else {
> +		reg_cr4 = FSL_SAI_RCR4;
> +		reg_cr5 = FSL_SAI_RCR5;
> +	}
> +
> +	val_cr4 = readl(sai->base + reg_cr4);
> +	val_cr4 &= ~FSL_SAI_CR4_SYWD_MASK;
> +
> +	val_cr5 = readl(sai->base + reg_cr5);
> +	val_cr5 &= ~FSL_SAI_CR5_WNW_MASK;
> +	val_cr5 &= ~FSL_SAI_CR5_W0W_MASK;
> +	val_cr5 &= ~FSL_SAI_CR5_FBT_MASK;
> +
> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_S16_LE:
> +		word_width = 16;
> +		break;
> +	case SNDRV_PCM_FORMAT_S20_3LE:
> +		word_width = 20;
> +		break;
> +	case SNDRV_PCM_FORMAT_S24_LE:
> +		word_width = 24;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	val_cr4 |= FSL_SAI_CR4_SYWD(word_width);
> +	val_cr5 |= FSL_SAI_CR5_WNW(word_width);
> +	val_cr5 |= FSL_SAI_CR5_W0W(word_width);
> +
> +	if (sai->fbt == FSL_SAI_FBT_MSB)
> +		val_cr5 |= FSL_SAI_CR5_FBT(word_width - 1);
> +	else if (sai->fbt == FSL_SAI_FBT_LSB)
> +		val_cr5 |= FSL_SAI_CR5_FBT(0);
> +
> +	writel(val_cr4, sai->base + reg_cr4);
> +	writel(val_cr5, sai->base + reg_cr5);
> +
> +	return 0;
> +}
> +
> +static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
> +		struct snd_soc_dai *dai)
> +{
> +	struct fsl_sai *sai = snd_soc_dai_get_drvdata(dai);
> +	unsigned int tcsr, rcsr;
> +
> +	tcsr = readl(sai->base + FSL_SAI_TCSR);
> +	rcsr = readl(sai->base + FSL_SAI_RCSR);
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		tcsr |= FSL_SAI_CSR_FRDE;
> +		rcsr &= ~FSL_SAI_CSR_FRDE;
> +	} else {
> +		rcsr |= FSL_SAI_CSR_FRDE;
> +		tcsr &= ~FSL_SAI_CSR_FRDE;
> +	}
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		tcsr |= FSL_SAI_CSR_TERE;
> +		rcsr |= FSL_SAI_CSR_TERE;
> +		writel(rcsr, sai->base + FSL_SAI_RCSR);
> +		udelay(10);

Does SAI really needs this udelay() here? Required by IP's operation flow?
If so, I think it's better to add comments here to explain it.

> +		writel(tcsr, sai->base + FSL_SAI_TCSR);
> +		break;
> +
> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		if (!(dai->playback_active || dai->capture_active)) {
> +			tcsr &= ~FSL_SAI_CSR_TERE;
> +			rcsr &= ~FSL_SAI_CSR_TERE;
> +		}
> +		writel(rcsr, sai->base + FSL_SAI_RCSR);
> +		udelay(10);
> +		writel(tcsr, sai->base + FSL_SAI_TCSR);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct snd_soc_dai_ops fsl_sai_pcm_dai_ops = {
> +	.set_sysclk	= fsl_sai_set_dai_sysclk,
> +	.set_clkdiv	= fsl_sai_set_dai_clkdiv,
> +	.set_fmt	= fsl_sai_set_dai_fmt,
> +	.set_tdm_slot	= fsl_sai_set_dai_tdm_slot,
> +	.hw_params	= fsl_sai_hw_params,
> +	.trigger	= fsl_sai_trigger,
> +};
> +
> +static int fsl_sai_dai_probe(struct snd_soc_dai *dai)
> +{
> +	int ret;
> +	struct fsl_sai *sai = dev_get_drvdata(dai->dev);
> +
> +	ret = clk_prepare_enable(sai->clk);
> +	if (ret)
> +		return ret;
> +
> +	writel(0x0, sai->base + FSL_SAI_RCSR);
> +	writel(0x0, sai->base + FSL_SAI_TCSR);
> +	writel(sai->dma_params_tx.maxburst, sai->base + FSL_SAI_TCR1);
> +	writel(sai->dma_params_rx.maxburst, sai->base + FSL_SAI_RCR1);
> +
> +	dai->playback_dma_data = &sai->dma_params_tx;
> +	dai->capture_dma_data = &sai->dma_params_rx;
> +
> +	snd_soc_dai_set_drvdata(dai, sai);
> +
> +	return 0;
> +}
> +
> +int fsl_sai_dai_remove(struct snd_soc_dai *dai)

static

> +{
> +	struct fsl_sai *sai = dev_get_drvdata(dai->dev);
> +
> +	clk_disable_unprepare(sai->clk);
> +
> +	return 0;
> +}
> +
> +static struct snd_soc_dai_driver fsl_sai_dai = {
> +	.probe = fsl_sai_dai_probe,
> +	.remove = fsl_sai_dai_remove,
> +	.playback = {
> +		.channels_min = 1,
> +		.channels_max = 2,
> +		.rates = SNDRV_PCM_RATE_8000_96000,
> +		.formats = FSL_SAI_FORMATS,
> +	},
> +	.capture = {
> +		.channels_min = 1,
> +		.channels_max = 2,
> +		.rates = SNDRV_PCM_RATE_8000_96000,
> +		.formats = FSL_SAI_FORMATS,
> +	},
> +	.ops = &fsl_sai_pcm_dai_ops,
> +};
> +
> +static const struct snd_soc_component_driver fsl_component = {
> +	.name           = "fsl-sai",
> +};
> +
> +static int fsl_sai_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct fsl_sai *sai;
> +	int ret = 0;
> +
> +	sai = devm_kzalloc(&pdev->dev, sizeof(*sai), GFP_KERNEL);
> +	if (!sai)
> +		return -ENOMEM;
> +
> +	sai->fbt = FSL_SAI_FBT_MSB;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	sai->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(sai->base))
> +		return PTR_ERR(sai->base);
> +
> +	sai->clk = devm_clk_get(&pdev->dev, "sai");
> +	if (IS_ERR(sai->clk)) {
> +		dev_err(&pdev->dev, "Cannot get sai's clock: %d\n", ret);
> +		return PTR_ERR(sai->clk);
> +	}
> +
> +	sai->dma_params_rx.addr = res->start + FSL_SAI_RDR;
> +	sai->dma_params_rx.maxburst = 6;
> +
> +	sai->dma_params_tx.addr = res->start + FSL_SAI_TDR;
> +	sai->dma_params_tx.maxburst = 6;
> +
> +	platform_set_drvdata(pdev, sai);
> +
> +	ret = devm_snd_soc_register_component(&pdev->dev, &fsl_component,
> +			&fsl_sai_dai, 1);
> +	if (ret)
> +		return ret;
> +
> +	ret = snd_dmaengine_pcm_register(&pdev->dev, NULL,
> +			SND_DMAENGINE_PCM_FLAG_NO_RESIDUE);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int fsl_sai_remove(struct platform_device *pdev)
> +{
> +	snd_dmaengine_pcm_unregister(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id fsl_sai_ids[] = {
> +	{ .compatible = "fsl,vf610-sai", },

> +	{ /*sentinel*/ }

I think this could be left in blank without comments inside.
And if you really want to add it, pls add like: /* sentinel */
						  ^        ^
> +};
> +
> +static struct platform_driver fsl_sai_driver = {
> +	.probe = fsl_sai_probe,
> +	.remove = fsl_sai_remove,
> +
> +	.driver = {
> +		.name = "fsl-sai",
> +		.owner = THIS_MODULE,
> +		.of_match_table = fsl_sai_ids,
> +	},
> +};
> +module_platform_driver(fsl_sai_driver);
> +
> +MODULE_AUTHOR("Xiubo Li, <Li.Xiubo@freescale.com>");
> +MODULE_AUTHOR("Alison Wang, <b18965@freescale.com>");
> +MODULE_DESCRIPTION("Freescale Soc SAI Interface");
> +MODULE_LICENSE("GPL");

Should be better if added:
MODULE_ALIAS("platform:fsl-sai");

This would support module auto-load feature in some Linux-OS.

Best regards,
Nicolin Chen

> diff --git a/sound/soc/fsl/fsl-sai.h b/sound/soc/fsl/fsl-sai.h
> new file mode 100644
> index 0000000..1637679
> --- /dev/null
> +++ b/sound/soc/fsl/fsl-sai.h
> @@ -0,0 +1,120 @@
> +/*
> + * Copyright 2012-2013 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __FSL_SAI_H
> +#define __FSL_SAI_H
> +
> +#include <sound/dmaengine_pcm.h>
> +
> +#define FSL_SAI_FORMATS (SNDRV_PCM_FMTBIT_S16_LE |\
> +			 SNDRV_PCM_FMTBIT_S20_3LE |\
> +			 SNDRV_PCM_FMTBIT_S24_LE)
> +
> +/* SAI Transmit/Recieve Control Register */
> +#define FSL_SAI_TCSR		0x00
> +#define FSL_SAI_RCSR		0x80
> +#define FSL_SAI_CSR_TERE	BIT(31)
> +#define FSL_SAI_CSR_FWF		BIT(17)
> +#define FSL_SAI_CSR_FRIE	BIT(8)
> +#define FSL_SAI_CSR_FRDE	BIT(0)
> +
> +/* SAI Transmit Data/FIFO/MASK Register */
> +#define FSL_SAI_TDR		0x20
> +#define FSL_SAI_TFR		0x40
> +#define FSL_SAI_TMR		0x60
> +
> +/* SAI Recieve Data/FIFO/MASK Register */
> +#define FSL_SAI_RDR		0xa0
> +#define FSL_SAI_RFR		0xc0
> +#define FSL_SAI_RMR		0xe0
> +
> +/* SAI Transmit and Recieve Configuration 1 Register */
> +#define FSL_SAI_TCR1		0x04
> +#define FSL_SAI_RCR1		0x84
> +
> +/* SAI Transmit and Recieve Configuration 2 Register */
> +#define FSL_SAI_TCR2		0x08
> +#define FSL_SAI_RCR2		0x88
> +#define FSL_SAI_CR2_SYNC	BIT(30)
> +#define FSL_SAI_CR2_MSEL_MASK	(0xff << 26)
> +#define FSL_SAI_CR2_MSEL_BUS	0
> +#define FSL_SAI_CR2_MSEL_MCLK1	BIT(26)
> +#define FSL_SAI_CR2_MSEL_MCLK2	BIT(27)
> +#define FSL_SAI_CR2_MSEL_MCLK3	(BIT(26) | BIT(27))
> +#define FSL_SAI_CR2_BCP		BIT(25)
> +#define FSL_SAI_CR2_BCD_MSTR	BIT(24)
> +#define FSL_SAI_CR2_DIV(x)	(x)
> +#define FSL_SAI_CR2_DIV_MASK	0xff
> +
> +/* SAI Transmit and Recieve Configuration 3 Register */
> +#define FSL_SAI_TCR3		0x0c
> +#define FSL_SAI_RCR3		0x8c
> +#define FSL_SAI_CR3_TRCE	BIT(16)
> +#define FSL_SAI_CR3_WDFL(x)	(x)
> +#define FSL_SAI_CR3_WDFL_MASK	0x1f
> +
> +/* SAI Transmit and Recieve Configuration 4 Register */
> +#define FSL_SAI_TCR4		0x10
> +#define FSL_SAI_RCR4		0x90
> +#define FSL_SAI_CR4_FRSZ(x)	(((x) - 1) << 16)
> +#define FSL_SAI_CR4_FRSZ_MASK	(0x1f << 16)
> +#define FSL_SAI_CR4_SYWD(x)	(((x) - 1) << 8)
> +#define FSL_SAI_CR4_SYWD_MASK	(0x1f << 8)
> +#define FSL_SAI_CR4_MF		BIT(4)
> +#define FSL_SAI_CR4_FSE		BIT(3)
> +#define FSL_SAI_CR4_FSP		BIT(1)
> +#define FSL_SAI_CR4_FSD_MSTR	BIT(0)
> +
> +/* SAI Transmit and Recieve Configuration 5 Register */
> +#define FSL_SAI_TCR5		0x14
> +#define FSL_SAI_RCR5		0x94
> +#define FSL_SAI_CR5_WNW(x)	(((x) - 1) << 24)
> +#define FSL_SAI_CR5_WNW_MASK	(0x1f << 24)
> +#define FSL_SAI_CR5_W0W(x)	(((x) - 1) << 16)
> +#define FSL_SAI_CR5_W0W_MASK	(0x1f << 16)
> +#define FSL_SAI_CR5_FBT(x)	((x) << 8)
> +#define FSL_SAI_CR5_FBT_MASK	(0x1f << 8)
> +
> +/* SAI audio dividers */
> +#define FSL_SAI_TX_DIV		0
> +#define FSL_SAI_RX_DIV		1
> +
> +/* SAI type */
> +#define FSL_SAI_DMA		BIT(0)
> +#define FSL_SAI_USE_AC97	BIT(1)
> +#define FSL_SAI_NET		BIT(2)
> +#define FSL_SAI_TRA_SYN		BIT(3)
> +#define FSL_SAI_REC_SYN		BIT(4)
> +#define FSL_SAI_USE_I2S_SLAVE	BIT(5)
> +
> +#define FSL_FMT_TRANSMITTER	0
> +#define FSL_FMT_RECEIVER	1
> +
> +/* SAI clock sources */
> +#define FSL_SAI_CLK_BUS		0
> +#define FSL_SAI_CLK_MAST1	1
> +#define FSL_SAI_CLK_MAST2	2
> +#define FSL_SAI_CLK_MAST3	3
> +
> +enum fsl_sai_fbt {
> +	FSL_SAI_FBT_MSB,
> +	FSL_SAI_FBT_LSB,
> +};
> +
> +struct fsl_sai {
> +	struct clk *clk;
> +
> +	void __iomem *base;
> +
> +	enum fsl_sai_fbt fbt;
> +
> +	struct snd_dmaengine_dai_dma_data dma_params_rx;
> +	struct snd_dmaengine_dai_dma_data dma_params_tx;
> +};
> +
> +#endif /* __FSL_SAI_H */
> -- 
> 1.8.4
> 

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Li Xiubo]

> > This adds Freescale SAI ASoC Audio support.
> > This implementation is only compatible with device tree definition.
> > Features:
> > o Supports playback/capture
> > o Supports 16/20/24 bit PCM
> > o Supports 8k - 96k sample rates
> > o Supports slave mode only.
> >
> 
> Just for curiosity, I found there're quite a bit code actually support
> I2S master mode such as set_sysclk() and register configuration to FMT
> SND_SOC_DAIFMT_CBS_CFS.
> 
> Is that really "supports slave mode only"? Or just you haven't make it
> properly work?
> 

Sorry, this comments is not very consistent with the driver implementation.
On VF610 there only supports slave mode.

So, I will revise this.


> > diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig index
> > b7ab71f..9a8851e 100644
> > --- a/sound/soc/fsl/Kconfig
> > +++ b/sound/soc/fsl/Kconfig
> > @@ -213,3 +213,19 @@ config SND_SOC_IMX_MC13783
> >  	select SND_SOC_IMX_PCM_DMA
> >
> >  endif # SND_IMX_SOC
> > +
> > +menuconfig SND_FSL_SOC
> > +	tristate "SoC Audio for Freescale FSL CPUs"
> > +	help
> > +	  Say Y or M if you want to add support for codecs attached to
> > +	  the FSL CPUs.
> > +
> > +	  This will enable Freeacale SAI, SGT15000 codec.
> > +
> > +if SND_FSL_SOC
> 
> Why check this here? SAI should be a regular IP module which can be used
> by other SoC as well. We would never know if next i.MX SoC won't contain
> SAI.
> 
> > +
> > +config SND_SOC_FSL_SAI
> > +	tristate
> > +	select SND_SOC_GENERIC_DMAENGINE_PCM
> 
> I think you should keep SAI beside SSI/SPDIF directly.
> 

That's right.

> > +
> > +endif # SND_FSL_SOC
> > diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile index
> > 8db705b..e5acc03 100644
> > --- a/sound/soc/fsl/Makefile
> > +++ b/sound/soc/fsl/Makefile
> > @@ -56,3 +56,8 @@ obj-$(CONFIG_SND_SOC_IMX_SGTL5000) +=
> > snd-soc-imx-sgtl5000.o
> >  obj-$(CONFIG_SND_SOC_IMX_WM8962) += snd-soc-imx-wm8962.o
> >  obj-$(CONFIG_SND_SOC_IMX_SPDIF) += snd-soc-imx-spdif.o
> >  obj-$(CONFIG_SND_SOC_IMX_MC13783) += snd-soc-imx-mc13783.o
> > +
> > +# FSL ARM SAI/SGT15000 Platform Support
> 
> Should be SGTL5000, not SGT1. And SAI should not be used with SGTL5000
> only, it's a bit confusing to mention SGTL5000 here.
> 

Yes, this will be revised then.

> > +snd-soc-fsl-sai-objs := fsl-sai.o
> 
> And I think it should be better to put it along with fsl-ssi.o and fsl-
> spdif.o
> 

But fsl-ssi.o and fsl-spdif.o is based PowrePC platform? Which we can see from the comments.
While fsl-sai.o is base ARM platform.
Despite whether there is any different between ARM and PPC or not, the comments won't be correct.


> > +
> > +obj-$(CONFIG_SND_SOC_FSL_SAI) += snd-soc-fsl-sai.o
> 
> Ditto
> 
> > diff --git a/sound/soc/fsl/fsl-sai.c b/sound/soc/fsl/fsl-sai.c new
> > file mode 100644 index 0000000..bb57e67
> > --- /dev/null
> > +++ b/sound/soc/fsl/fsl-sai.c
> > @@ -0,0 +1,472 @@
> > +/*
> > + * Freescale SAI ALSA SoC Digital Audio Interface driver.
> > + *
> > + * Copyright 2012-2013 Freescale Semiconductor, Inc.
> > + *
> > + * 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.
> 
> There're too many double space inside. Could you pls revise it?
> 

Yes, see the next version.

> > + *
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/of_address.h>
> > +#include <sound/core.h>
> > +#include <sound/pcm_params.h>
> > +#include <linux/delay.h>
> > +#include <linux/dmaengine.h>
> > +#include <sound/dmaengine_pcm.h>
> 
> I think it's better to keep <sound/xxx.h> together. And basically we can
> keep header files ordered by alphabet.
> 

Okey.

> > +
> > +#include "fsl-sai.h"
> > +
> > +static int fsl_sai_set_dai_sysclk_tr(struct snd_soc_dai *cpu_dai,
> > +		int clk_id, unsigned int freq, int fsl_dir) {
> > +	u32 val_cr2, reg_cr2;
> > +	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
> > +
> > +	if (fsl_dir == FSL_FMT_TRANSMITTER)
> > +		reg_cr2 = FSL_SAI_TCR2;
> > +	else
> > +		reg_cr2 = FSL_SAI_RCR2;
> > +
> > +	val_cr2 = readl(sai->base + reg_cr2);
> > +	switch (clk_id) {
> > +	case FSL_SAI_CLK_BUS:
> > +		val_cr2 &= ~FSL_SAI_CR2_MSEL_MASK;
> > +		val_cr2 |= FSL_SAI_CR2_MSEL_BUS;
> > +		break;
> > +	case FSL_SAI_CLK_MAST1:
> > +		val_cr2 &= ~FSL_SAI_CR2_MSEL_MASK;
> > +		val_cr2 |= FSL_SAI_CR2_MSEL_MCLK1;
> > +		break;
> > +	case FSL_SAI_CLK_MAST2:
> > +		val_cr2 &= ~FSL_SAI_CR2_MSEL_MASK;
> > +		val_cr2 |= FSL_SAI_CR2_MSEL_MCLK2;
> > +		break;
> > +	case FSL_SAI_CLK_MAST3:
> > +		val_cr2 &= ~FSL_SAI_CR2_MSEL_MASK;
> > +		val_cr2 |= FSL_SAI_CR2_MSEL_MCLK3;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	writel(val_cr2, sai->base + reg_cr2);
> > +
> > +	return 0;
> > +}
> > +
> > +static int fsl_sai_set_dai_sysclk(struct snd_soc_dai *cpu_dai,
> > +		int clk_id, unsigned int freq, int dir) {
> > +	int ret;
> > +
> > +	if (dir == SND_SOC_CLOCK_IN)
> > +		return 0;
> > +
> > +	ret = fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq,
> > +					FSL_FMT_TRANSMITTER);
> > +	if (ret) {
> > +		dev_err(cpu_dai->dev,
> > +				"Cannot set sai's transmitter sysclk: %d\n",
> > +				ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq,
> > +					FSL_FMT_RECEIVER);
> > +	if (ret) {
> > +		dev_err(cpu_dai->dev,
> > +				"Cannot set sai's receiver sysclk: %d\n",
> > +				ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int fsl_sai_set_dai_clkdiv(struct snd_soc_dai *cpu_dai,
> > +		int div_id, int div)
> > +{
> > +	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
> > +	u32 tcr2, rcr2;
> > +
> > +	if (div_id == FSL_SAI_TX_DIV) {
> > +		tcr2 = readl(sai->base + FSL_SAI_TCR2);
> > +		tcr2 &= ~FSL_SAI_CR2_DIV_MASK;
> > +		tcr2 |= FSL_SAI_CR2_DIV(div);
> > +		writel(tcr2, sai->base + FSL_SAI_TCR2);
> > +
> > +	} else if (div_id == FSL_SAI_RX_DIV) {
> > +		rcr2 = readl(sai->base + FSL_SAI_RCR2);
> > +		rcr2 &= ~FSL_SAI_CR2_DIV_MASK;
> > +		rcr2 |= FSL_SAI_CR2_DIV(div);
> > +		writel(rcr2, sai->base + FSL_SAI_RCR2);
> > +
> > +	} else
> > +		return -EINVAL;
> 
> It would look better if using switch-case. And the last 'else'
> could be 'default:'.
> 

I'll think it over.

> > +
> > +	return 0;
> > +}
> > +
> > +static int fsl_sai_set_dai_fmt_tr(struct snd_soc_dai *cpu_dai,
> > +				unsigned int fmt, int fsl_dir)
> > +{
> > +	u32 val_cr2, val_cr3, val_cr4, reg_cr2, reg_cr3, reg_cr4;
> > +	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
> > +
> > +	if (fsl_dir == FSL_FMT_TRANSMITTER) {
> > +		reg_cr2 = FSL_SAI_TCR2;
> > +		reg_cr3 = FSL_SAI_TCR3;
> > +		reg_cr4 = FSL_SAI_TCR4;
> > +	} else {
> > +		reg_cr2 = FSL_SAI_RCR2;
> > +		reg_cr3 = FSL_SAI_RCR3;
> > +		reg_cr4 = FSL_SAI_RCR4;
> > +	}
> > +
> > +	val_cr2 = readl(sai->base + reg_cr2);
> > +	val_cr3 = readl(sai->base + reg_cr3);
> > +	val_cr4 = readl(sai->base + reg_cr4);
> > +
> > +	if (sai->fbt == FSL_SAI_FBT_MSB)
> > +		val_cr4 |= FSL_SAI_CR4_MF;
> > +	else if (sai->fbt == FSL_SAI_FBT_LSB)
> > +		val_cr4 &= ~FSL_SAI_CR4_MF;
> > +
> > +	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> > +	case SND_SOC_DAIFMT_I2S:
> > +		val_cr4 |= FSL_SAI_CR4_FSE;
> > +		val_cr4 |= FSL_SAI_CR4_FSP;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> > +	case SND_SOC_DAIFMT_IB_IF:
> > +		val_cr4 |= FSL_SAI_CR4_FSP;
> > +		val_cr2 &= ~FSL_SAI_CR2_BCP;
> > +		break;
> > +	case SND_SOC_DAIFMT_IB_NF:
> > +		val_cr4 &= ~FSL_SAI_CR4_FSP;
> > +		val_cr2 &= ~FSL_SAI_CR2_BCP;
> > +		break;
> > +	case SND_SOC_DAIFMT_NB_IF:
> > +		val_cr4 |= FSL_SAI_CR4_FSP;
> > +		val_cr2 |= FSL_SAI_CR2_BCP;
> > +		break;
> > +	case SND_SOC_DAIFMT_NB_NF:
> > +		val_cr4 &= ~FSL_SAI_CR4_FSP;
> > +		val_cr2 |= FSL_SAI_CR2_BCP;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > +	case SND_SOC_DAIFMT_CBS_CFS:
> > +		val_cr2 |= FSL_SAI_CR2_BCD_MSTR;
> > +		val_cr4 |= FSL_SAI_CR4_FSD_MSTR;
> > +		break;
> > +	case SND_SOC_DAIFMT_CBM_CFM:
> > +		val_cr2 &= ~FSL_SAI_CR2_BCD_MSTR;
> > +		val_cr4 &= ~FSL_SAI_CR4_FSD_MSTR;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	val_cr3 |= FSL_SAI_CR3_TRCE;
> > +
> > +	if (fsl_dir == FSL_FMT_RECEIVER)
> > +		val_cr2 |= FSL_SAI_CR2_SYNC;
> > +
> > +	writel(val_cr2, sai->base + reg_cr2);
> > +	writel(val_cr3, sai->base + reg_cr3);
> > +	writel(val_cr4, sai->base + reg_cr4);
> > +
> > +	return 0;
> 
> > +
> 
> Pls drop this extra line.
>

See the next version.

> 
> > +}
> > +
> > +static int fsl_sai_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned
> > +int fmt) {
> > +	int ret;
> > +
> > +	ret = fsl_sai_set_dai_fmt_tr(cpu_dai, fmt, FSL_FMT_TRANSMITTER);
> > +	if (ret) {
> > +		dev_err(cpu_dai->dev,
> > +				"Cannot set sai's transmitter format: %d\n",
> > +				ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = fsl_sai_set_dai_fmt_tr(cpu_dai, fmt, FSL_FMT_RECEIVER);
> > +	if (ret) {
> > +		dev_err(cpu_dai->dev,
> > +				"Cannot set sai's receiver format: %d\n",
> > +				ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int fsl_sai_set_dai_tdm_slot(struct snd_soc_dai *cpu_dai,
> > +		unsigned int tx_mask, unsigned int rx_mask,
> > +		int slots, int slot_width)
> > +{
> > +	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
> > +	u32 tcr4, rcr4;
> > +
> > +	tcr4 = readl(sai->base + FSL_SAI_TCR4);
> > +	tcr4 &= ~FSL_SAI_CR4_FRSZ_MASK;
> > +	tcr4 |= FSL_SAI_CR4_FRSZ(2);
> > +	writel(tcr4, sai->base + FSL_SAI_TCR4);
> > +	writel(tx_mask, sai->base + FSL_SAI_TMR);
> > +
> > +	rcr4 = readl(sai->base + FSL_SAI_RCR4);
> > +	rcr4 &= ~FSL_SAI_CR4_FRSZ_MASK;
> > +	rcr4 |= FSL_SAI_CR4_FRSZ(2);
> > +	writel(rcr4, sai->base + FSL_SAI_RCR4);
> > +	writel(rx_mask, sai->base + FSL_SAI_RMR);
> > +
> > +	return 0;
> > +}
> > +
> > +static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
> > +		struct snd_pcm_hw_params *params,
> > +		struct snd_soc_dai *cpu_dai)
> > +{
> > +	u32 val_cr4, val_cr5, reg_cr4, reg_cr5, word_width;
> > +	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
> > +
> > +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> > +		reg_cr4 = FSL_SAI_TCR4;
> > +		reg_cr5 = FSL_SAI_TCR5;
> > +	} else {
> > +		reg_cr4 = FSL_SAI_RCR4;
> > +		reg_cr5 = FSL_SAI_RCR5;
> > +	}
> > +
> > +	val_cr4 = readl(sai->base + reg_cr4);
> > +	val_cr4 &= ~FSL_SAI_CR4_SYWD_MASK;
> > +
> > +	val_cr5 = readl(sai->base + reg_cr5);
> > +	val_cr5 &= ~FSL_SAI_CR5_WNW_MASK;
> > +	val_cr5 &= ~FSL_SAI_CR5_W0W_MASK;
> > +	val_cr5 &= ~FSL_SAI_CR5_FBT_MASK;
> > +
> > +	switch (params_format(params)) {
> > +	case SNDRV_PCM_FORMAT_S16_LE:
> > +		word_width = 16;
> > +		break;
> > +	case SNDRV_PCM_FORMAT_S20_3LE:
> > +		word_width = 20;
> > +		break;
> > +	case SNDRV_PCM_FORMAT_S24_LE:
> > +		word_width = 24;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	val_cr4 |= FSL_SAI_CR4_SYWD(word_width);
> > +	val_cr5 |= FSL_SAI_CR5_WNW(word_width);
> > +	val_cr5 |= FSL_SAI_CR5_W0W(word_width);
> > +
> > +	if (sai->fbt == FSL_SAI_FBT_MSB)
> > +		val_cr5 |= FSL_SAI_CR5_FBT(word_width - 1);
> > +	else if (sai->fbt == FSL_SAI_FBT_LSB)
> > +		val_cr5 |= FSL_SAI_CR5_FBT(0);
> > +
> > +	writel(val_cr4, sai->base + reg_cr4);
> > +	writel(val_cr5, sai->base + reg_cr5);
> > +
> > +	return 0;
> > +}
> > +
> > +static int fsl_sai_trigger(struct snd_pcm_substream *substream, int
> cmd,
> > +		struct snd_soc_dai *dai)
> > +{
> > +	struct fsl_sai *sai = snd_soc_dai_get_drvdata(dai);
> > +	unsigned int tcsr, rcsr;
> > +
> > +	tcsr = readl(sai->base + FSL_SAI_TCSR);
> > +	rcsr = readl(sai->base + FSL_SAI_RCSR);
> > +
> > +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> > +		tcsr |= FSL_SAI_CSR_FRDE;
> > +		rcsr &= ~FSL_SAI_CSR_FRDE;
> > +	} else {
> > +		rcsr |= FSL_SAI_CSR_FRDE;
> > +		tcsr &= ~FSL_SAI_CSR_FRDE;
> > +	}
> > +
> > +	switch (cmd) {
> > +	case SNDRV_PCM_TRIGGER_START:
> > +	case SNDRV_PCM_TRIGGER_RESUME:
> > +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> > +		tcsr |= FSL_SAI_CSR_TERE;
> > +		rcsr |= FSL_SAI_CSR_TERE;
> > +		writel(rcsr, sai->base + FSL_SAI_RCSR);
> > +		udelay(10);
> 
> Does SAI really needs this udelay() here? Required by IP's operation flow?
> If so, I think it's better to add comments here to explain it.
> 
+++++++++++++++++
Synchronous mode
The SAI transmitter and receiver can be configured to operate with synchronous bit clock
and frame sync.

1,
If the transmitter bit clock and frame sync are to be used by both the transmitter and
receiver:
	...
* It is recommended that the transmitter is the last enabled and the first disabled.

2,
If the receiver bit clock and frame sync are to be used by both the transmitter and
receiver:
	...
* It is recommended that the receiver is the last enabled and the first disabled.
------------------

So I think the delay is needed, and I still tunning it.



> > +		writel(tcsr, sai->base + FSL_SAI_TCSR);
> > +		break;
> > +
> > +	case SNDRV_PCM_TRIGGER_STOP:
> > +	case SNDRV_PCM_TRIGGER_SUSPEND:
> > +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> > +		if (!(dai->playback_active || dai->capture_active)) {
> > +			tcsr &= ~FSL_SAI_CSR_TERE;
> > +			rcsr &= ~FSL_SAI_CSR_TERE;
> > +		}
> > +		writel(rcsr, sai->base + FSL_SAI_RCSR);
> > +		udelay(10);
> > +		writel(tcsr, sai->base + FSL_SAI_TCSR);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct snd_soc_dai_ops fsl_sai_pcm_dai_ops = {
> > +	.set_sysclk	= fsl_sai_set_dai_sysclk,
> > +	.set_clkdiv	= fsl_sai_set_dai_clkdiv,
> > +	.set_fmt	= fsl_sai_set_dai_fmt,
> > +	.set_tdm_slot	= fsl_sai_set_dai_tdm_slot,
> > +	.hw_params	= fsl_sai_hw_params,
> > +	.trigger	= fsl_sai_trigger,
> > +};
> > +
> > +static int fsl_sai_dai_probe(struct snd_soc_dai *dai) {
> > +	int ret;
> > +	struct fsl_sai *sai = dev_get_drvdata(dai->dev);
> > +
> > +	ret = clk_prepare_enable(sai->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	writel(0x0, sai->base + FSL_SAI_RCSR);
> > +	writel(0x0, sai->base + FSL_SAI_TCSR);
> > +	writel(sai->dma_params_tx.maxburst, sai->base + FSL_SAI_TCR1);
> > +	writel(sai->dma_params_rx.maxburst, sai->base + FSL_SAI_RCR1);
> > +
> > +	dai->playback_dma_data = &sai->dma_params_tx;
> > +	dai->capture_dma_data = &sai->dma_params_rx;
> > +
> > +	snd_soc_dai_set_drvdata(dai, sai);
> > +
> > +	return 0;
> > +}
> > +
> > +int fsl_sai_dai_remove(struct snd_soc_dai *dai)
> 
> static
> 

Yes.

> > +{
> > +	struct fsl_sai *sai = dev_get_drvdata(dai->dev);
> > +
> > +	clk_disable_unprepare(sai->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct snd_soc_dai_driver fsl_sai_dai = {
> > +	.probe = fsl_sai_dai_probe,
> > +	.remove = fsl_sai_dai_remove,
> > +	.playback = {
> > +		.channels_min = 1,
> > +		.channels_max = 2,
> > +		.rates = SNDRV_PCM_RATE_8000_96000,
> > +		.formats = FSL_SAI_FORMATS,
> > +	},
> > +	.capture = {
> > +		.channels_min = 1,
> > +		.channels_max = 2,
> > +		.rates = SNDRV_PCM_RATE_8000_96000,
> > +		.formats = FSL_SAI_FORMATS,
> > +	},
> > +	.ops = &fsl_sai_pcm_dai_ops,
> > +};
> > +
> > +static const struct snd_soc_component_driver fsl_component = {
> > +	.name           = "fsl-sai",
> > +};
> > +
> > +static int fsl_sai_probe(struct platform_device *pdev) {
> > +	struct resource *res;
> > +	struct fsl_sai *sai;
> > +	int ret = 0;
> > +
> > +	sai = devm_kzalloc(&pdev->dev, sizeof(*sai), GFP_KERNEL);
> > +	if (!sai)
> > +		return -ENOMEM;
> > +
> > +	sai->fbt = FSL_SAI_FBT_MSB;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	sai->base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(sai->base))
> > +		return PTR_ERR(sai->base);
> > +
> > +	sai->clk = devm_clk_get(&pdev->dev, "sai");
> > +	if (IS_ERR(sai->clk)) {
> > +		dev_err(&pdev->dev, "Cannot get sai's clock: %d\n", ret);
> > +		return PTR_ERR(sai->clk);
> > +	}
> > +
> > +	sai->dma_params_rx.addr = res->start + FSL_SAI_RDR;
> > +	sai->dma_params_rx.maxburst = 6;
> > +
> > +	sai->dma_params_tx.addr = res->start + FSL_SAI_TDR;
> > +	sai->dma_params_tx.maxburst = 6;
> > +
> > +	platform_set_drvdata(pdev, sai);
> > +
> > +	ret = devm_snd_soc_register_component(&pdev->dev, &fsl_component,
> > +			&fsl_sai_dai, 1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = snd_dmaengine_pcm_register(&pdev->dev, NULL,
> > +			SND_DMAENGINE_PCM_FLAG_NO_RESIDUE);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static int fsl_sai_remove(struct platform_device *pdev) {
> > +	snd_dmaengine_pcm_unregister(&pdev->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id fsl_sai_ids[] = {
> > +	{ .compatible = "fsl,vf610-sai", },
> 
> > +	{ /*sentinel*/ }
> 
> I think this could be left in blank without comments inside.
> And if you really want to add it, pls add like: /* sentinel */
> 						  ^        ^

Okey.

> > +};
> > +
> > +static struct platform_driver fsl_sai_driver = {
> > +	.probe = fsl_sai_probe,
> > +	.remove = fsl_sai_remove,
> > +
> > +	.driver = {
> > +		.name = "fsl-sai",
> > +		.owner = THIS_MODULE,
> > +		.of_match_table = fsl_sai_ids,
> > +	},
> > +};
> > +module_platform_driver(fsl_sai_driver);
> > +
> > +MODULE_AUTHOR("Xiubo Li, <Li.Xiubo@freescale.com>");
> > +MODULE_AUTHOR("Alison Wang, <b18965@freescale.com>");
> > +MODULE_DESCRIPTION("Freescale Soc SAI Interface");
> > +MODULE_LICENSE("GPL");
> 
> Should be better if added:
> MODULE_ALIAS("platform:fsl-sai");
> 
> This would support module auto-load feature in some Linux-OS.
> 

I will think it over.

BRS,
Xiubo

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Nicolin Chen]

On Mon, Nov 04, 2013 at 11:45:10AM +0800, Xiubo Li-B47053 wrote:
> > > +snd-soc-fsl-sai-objs := fsl-sai.o
> > 
> > And I think it should be better to put it along with fsl-ssi.o and fsl-
> > spdif.o
> > 
> 
> But fsl-ssi.o and fsl-spdif.o is based PowrePC platform? Which we can see from the comments.

No. fsl-ssi.c is compatible to both ARM and PPC. And fsl-spdif.c is currently
used on ARM only. So please just put along with them.

> > > +	case SNDRV_PCM_TRIGGER_START:
> > > +	case SNDRV_PCM_TRIGGER_RESUME:
> > > +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> > > +		tcsr |= FSL_SAI_CSR_TERE;
> > > +		rcsr |= FSL_SAI_CSR_TERE;
> > > +		writel(rcsr, sai->base + FSL_SAI_RCSR);
> > > +		udelay(10);
> > 
> > Does SAI really needs this udelay() here? Required by IP's operation flow?
> > If so, I think it's better to add comments here to explain it.
> > 
> +++++++++++++++++
> Synchronous mode
> The SAI transmitter and receiver can be configured to operate with synchronous bit clock
> and frame sync.
> 
> 1,
> If the transmitter bit clock and frame sync are to be used by both the transmitter and
> receiver:
> 	...
> * It is recommended that the transmitter is the last enabled and the first disabled.
> 
> 2,
> If the receiver bit clock and frame sync are to be used by both the transmitter and
> receiver:
> 	...
> * It is recommended that the receiver is the last enabled and the first disabled.
> ------------------
> 
> So I think the delay is needed, and I still tunning it.
> 

The udelay just doesn't make sense to what you are talking about.

Does SAI really need 10us delay between two register-updating?

We basically use udelay only if the IP hardware actually needs it: some
IP needs time to boot itself up after doing software reset for example.
But it doesn't look reasonable to me by using udelay to make sure "the 
last enabled".

And from the 'Synchronous mode' you just provided, there're another issue:

+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		tcsr |= FSL_SAI_CSR_TERE;
+		rcsr |= FSL_SAI_CSR_TERE;
+		writel(rcsr, sai->base + FSL_SAI_RCSR);
+		udelay(10);
+		writel(tcsr, sai->base + FSL_SAI_TCSR);
+		break;
+
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		if (!(dai->playback_active || dai->capture_active)) {
+			tcsr &= ~FSL_SAI_CSR_TERE;
+			rcsr &= ~FSL_SAI_CSR_TERE;
+		}
+		writel(rcsr, sai->base + FSL_SAI_RCSR);
+		udelay(10);
+		writel(tcsr, sai->base + FSL_SAI_TCSR);
+		break;

ISSUE 1: You might make sure transmitter is the last enabled.
	 However, it's not the first disabled. Is this okay?

ISSUE 2: There are two cases listed in 'Synchronous mode'.
	 However, your driver doesn't take care of them.
	 The SAI's synchronous mode looks like more flexible
	 than SSI's. The driver needs to be more sophisticated
	 so that it can handle multiple cases when TX/RX clocks
	 are controlled by either TX or RX, and surely, the
	 asynchronous mode as well.


And there's another personal tip: I think you can first try to focus on
this SAI driver and pend the others. There might be two many things you
need to refine if you are doing them at the same time.

Best regards,
Nicolin Chen

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Li Xiubo]

 
> The udelay just doesn't make sense to what you are talking about.
> 
> Does SAI really need 10us delay between two register-updating?
> 

No, this is not must be.

> We basically use udelay only if the IP hardware actually needs it: some
> IP needs time to boot itself up after doing software reset for example.
> But it doesn't look reasonable to me by using udelay to make sure "the
> last enabled".
> 
> And from the 'Synchronous mode' you just provided, there're another issue:
> 
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		tcsr |= FSL_SAI_CSR_TERE;
> +		rcsr |= FSL_SAI_CSR_TERE;
> +		writel(rcsr, sai->base + FSL_SAI_RCSR);
> +		udelay(10);
> +		writel(tcsr, sai->base + FSL_SAI_TCSR);
> +		break;
> +
> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		if (!(dai->playback_active || dai->capture_active)) {
> +			tcsr &= ~FSL_SAI_CSR_TERE;
> +			rcsr &= ~FSL_SAI_CSR_TERE;
> +		}
> +		writel(rcsr, sai->base + FSL_SAI_RCSR);
> +		udelay(10);
> +		writel(tcsr, sai->base + FSL_SAI_TCSR);
> +		break;
> 
> ISSUE 1: You might make sure transmitter is the last enabled.
> 	 However, it's not the first disabled. Is this okay?
> 

Yes, this is just programming mistake. I'll revise it.

In this case the transmitter should be the last enabled and the first disabled.


> ISSUE 2: There are two cases listed in 'Synchronous mode'.
> 	 However, your driver doesn't take care of them.
> 	 The SAI's synchronous mode looks like more flexible
> 	 than SSI's. The driver needs to be more sophisticated
> 	 so that it can handle multiple cases when TX/RX clocks
> 	 are controlled by either TX or RX, and surely, the
> 	 asynchronous mode as well.
> 

Because in Vybrid the transmitter bit clock and frame sync are to be used by both the transmitter and receiver, and only this case can be used here, so now I only handle this case.

> 
> And there's another personal tip: I think you can first try to focus on
> this SAI driver and pend the others. There might be two many things you
> need to refine if you are doing them at the same time.
> 

I'll implement them later if needed.


--
Best Regards,
Xiubo

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Nicolin Chen]

On Wed, Nov 20, 2013 at 11:37:45AM +0800, Xiubo Li-B47053 wrote:
>  
> > The udelay just doesn't make sense to what you are talking about.
> > 
> > Does SAI really need 10us delay between two register-updating?
> > 
> 
> No, this is not must be.

Then you should explain in your comments why you really put it here or just
drop it if it's just a mistake.

> > We basically use udelay only if the IP hardware actually needs it: some
> > IP needs time to boot itself up after doing software reset for example.
> > But it doesn't look reasonable to me by using udelay to make sure "the
> > last enabled".
> > 
> > And from the 'Synchronous mode' you just provided, there're another issue:
> > 
> > +	case SNDRV_PCM_TRIGGER_START:
> > +	case SNDRV_PCM_TRIGGER_RESUME:
> > +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> > +		tcsr |= FSL_SAI_CSR_TERE;
> > +		rcsr |= FSL_SAI_CSR_TERE;
> > +		writel(rcsr, sai->base + FSL_SAI_RCSR);
> > +		udelay(10);
> > +		writel(tcsr, sai->base + FSL_SAI_TCSR);
> > +		break;
> > +
> > +	case SNDRV_PCM_TRIGGER_STOP:
> > +	case SNDRV_PCM_TRIGGER_SUSPEND:
> > +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> > +		if (!(dai->playback_active || dai->capture_active)) {
> > +			tcsr &= ~FSL_SAI_CSR_TERE;
> > +			rcsr &= ~FSL_SAI_CSR_TERE;
> > +		}
> > +		writel(rcsr, sai->base + FSL_SAI_RCSR);
> > +		udelay(10);
> > +		writel(tcsr, sai->base + FSL_SAI_TCSR);
> > +		break;
> > 
> > ISSUE 1: You might make sure transmitter is the last enabled.
> > 	 However, it's not the first disabled. Is this okay?
> > 
> 
> Yes, this is just programming mistake. I'll revise it.
> 
> In this case the transmitter should be the last enabled and the first disabled.
> 
> 
> > ISSUE 2: There are two cases listed in 'Synchronous mode'.
> > 	 However, your driver doesn't take care of them.
> > 	 The SAI's synchronous mode looks like more flexible
> > 	 than SSI's. The driver needs to be more sophisticated
> > 	 so that it can handle multiple cases when TX/RX clocks
> > 	 are controlled by either TX or RX, and surely, the
> > 	 asynchronous mode as well.
> > 
> 
> Because in Vybrid the transmitter bit clock and frame sync are to be used by
> both the transmitter and receiver, and only this case can be used here, so
> now I only handle this case.

It's fairly okay if adding explicit comments to indicate that currently the
driver only supports its Synchronous mode with clocks controlled by TX only.

Best,
Nicolin Chen

> > 
> > And there's another personal tip: I think you can first try to focus on
> > this SAI driver and pend the others. There might be two many things you
> > need to refine if you are doing them at the same time.
> > 
> 
> I'll implement them later if needed.
> 
> 
> --
> Best Regards,
> Xiubo
> 

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Li Xiubo]

> > > The udelay just doesn't make sense to what you are talking about.
> > >
> > > Does SAI really need 10us delay between two register-updating?
> > >
> >
> > No, this is not must be.
> 
> Then you should explain in your comments why you really put it here or
> just drop it if it's just a mistake.
> 

The udelay will be removed then.


> > > ISSUE 2: There are two cases listed in 'Synchronous mode'.
> > > 	 However, your driver doesn't take care of them.
> > > 	 The SAI's synchronous mode looks like more flexible
> > > 	 than SSI's. The driver needs to be more sophisticated
> > > 	 so that it can handle multiple cases when TX/RX clocks
> > > 	 are controlled by either TX or RX, and surely, the
> > > 	 asynchronous mode as well.
> > >
> >
> > Because in Vybrid the transmitter bit clock and frame sync are to be
> > used by both the transmitter and receiver, and only this case can be
> > used here, so now I only handle this case.
> 
> It's fairly okay if adding explicit comments to indicate that currently
> the driver only supports its Synchronous mode with clocks controlled by
> TX only.
> 

Just think, on other platforms maybe only the Rx's clock is available.
Thus I think there should be one DT property to control this, and then the SAI driver can be more flexible.

Or could you give me some more practical ideas ?

--
Best Regards,
Xiubo

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Timur Tabi]

Li Xiubo wrote:
> But fsl-ssi.o and fsl-spdif.o is based PowrePC platform? Which we can see from the comments.

fsl_ssi was originally PPC-only, but it now supports PPC and ARM.  You 
can see that from the git history.

If there are any comments that say PPC but are not PPC-specific, that 
should be fixed.

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Li Xiubo]

> > But fsl-ssi.o and fsl-spdif.o is based PowrePC platform? Which we can
> see from the comments.
> 
> fsl_ssi was originally PPC-only, but it now supports PPC and ARM.  You
> can see that from the git history.
> 
> If there are any comments that say PPC but are not PPC-specific, that
> should be fixed.

Yes, find it.

The comments is in "sound/soc/fsl/Makefile" :
+++++++++++
"# Freescale PowerPC SSI/DMA Platform Support"
-----------

But fsl-spdif.o is also under it.
And this is also support ARM and PowerPC platforms at the same time ?
If so, the comments should be modified to :
+++++++++++
"# Freescale PowerPC and ARM SSI/DMA Platform Support"
-----------



Best Regards,
Xiubo

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Timur Tabi]

Li Xiubo wrote:
>> If there are any comments that say PPC but are not PPC-specific, that
>> >should be fixed.
> Yes, find it.
>
> The comments is in "sound/soc/fsl/Makefile" :
> +++++++++++
> "# Freescale PowerPC SSI/DMA Platform Support"
> -----------
>
> But fsl-spdif.o is also under it.
> And this is also support ARM and PowerPC platforms at the same time ?
> If so, the comments should be modified to :
> +++++++++++
> "# Freescale PowerPC and ARM SSI/DMA Platform Support"
> -----------

Yes, this should be changed.

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Li Xiubo]

> >> If there are any comments that say PPC but are not PPC-specific, that
> >> >should be fixed.
> > Yes, find it.
> >
> > The comments is in "sound/soc/fsl/Makefile" :
> > +++++++++++
> > "# Freescale PowerPC SSI/DMA Platform Support"
> > -----------
> >
> > But fsl-spdif.o is also under it.
> > And this is also support ARM and PowerPC platforms at the same time ?
> > If so, the comments should be modified to :
> > +++++++++++
> > "# Freescale PowerPC and ARM SSI/DMA Platform Support"
> > -----------
> 
> Yes, this should be changed.
> 

How about :
+++++++++
"# Freescale PowerPC and ARM SSI/DMA/SAI/SPDIF Platform Support"	
---------
 ?

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Shawn Guo]

On Wed, Nov 06, 2013 at 03:53:24AM +0000, Li Xiubo wrote:
> > >> If there are any comments that say PPC but are not PPC-specific, that
> > >> >should be fixed.
> > > Yes, find it.
> > >
> > > The comments is in "sound/soc/fsl/Makefile" :
> > > +++++++++++
> > > "# Freescale PowerPC SSI/DMA Platform Support"
> > > -----------
> > >
> > > But fsl-spdif.o is also under it.
> > > And this is also support ARM and PowerPC platforms at the same time ?
> > > If so, the comments should be modified to :
> > > +++++++++++
> > > "# Freescale PowerPC and ARM SSI/DMA Platform Support"
> > > -----------
> > 
> > Yes, this should be changed.
> > 
> 
> How about :
> +++++++++
> "# Freescale PowerPC and ARM SSI/DMA/SAI/SPDIF Platform Support"	
> ---------
>  ?

Or we can just drop 'PowerPC and ARM'?

Shawn

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Li Xiubo]

> > > >> If there are any comments that say PPC but are not PPC-specific,
> that
> > > >> >should be fixed.
> > > > Yes, find it.
> > > >
> > > > The comments is in "sound/soc/fsl/Makefile" :
> > > > +++++++++++
> > > > "# Freescale PowerPC SSI/DMA Platform Support"
> > > > -----------
> > > >
> > > > But fsl-spdif.o is also under it.
> > > > And this is also support ARM and PowerPC platforms at the same
> time ?
> > > > If so, the comments should be modified to :
> > > > +++++++++++
> > > > "# Freescale PowerPC and ARM SSI/DMA Platform Support"
> > > > -----------
> > >
> > > Yes, this should be changed.
> > >
> >
> > How about :
> > +++++++++
> > "# Freescale PowerPC and ARM SSI/DMA/SAI/SPDIF Platform Support"
> > ---------
> >  ?
> 
> Or we can just drop 'PowerPC and ARM'?
> 

Yes, this will be better.


Best Regards,
Xiubo

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Mark Brown]

On Fri, Nov 01, 2013 at 03:04:48PM +0800, Xiubo Li wrote:

> +static int fsl_sai_set_dai_clkdiv(struct snd_soc_dai *cpu_dai,
> +		int div_id, int div)
> +{
> +	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
> +	u32 tcr2, rcr2;
> +
> +	if (div_id == FSL_SAI_TX_DIV) {
> +		tcr2 = readl(sai->base + FSL_SAI_TCR2);
> +		tcr2 &= ~FSL_SAI_CR2_DIV_MASK;
> +		tcr2 |= FSL_SAI_CR2_DIV(div);
> +		writel(tcr2, sai->base + FSL_SAI_TCR2);

What is this divider and why does the user have to set it manually?

> +	} else
> +		return -EINVAL;
> +

Coding style?

> +static int fsl_sai_dai_probe(struct snd_soc_dai *dai)
> +{
> +	int ret;
> +	struct fsl_sai *sai = dev_get_drvdata(dai->dev);
> +
> +	ret = clk_prepare_enable(sai->clk);
> +	if (ret)
> +		return ret;

It'd be nicer to only enable the clock while the device is in active
use.

> +	ret = snd_dmaengine_pcm_register(&pdev->dev, NULL,
> +			SND_DMAENGINE_PCM_FLAG_NO_RESIDUE);
> +	if (ret)
> +		return ret;

We should have a devm_ version of this.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131101/6f21a6bb/attachment.sig>

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Li Xiubo]

> > +static int fsl_sai_set_dai_clkdiv(struct snd_soc_dai *cpu_dai,
> > +		int div_id, int div)
> > +{
> > +	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
> > +	u32 tcr2, rcr2;
> > +
> > +	if (div_id == FSL_SAI_TX_DIV) {
> > +		tcr2 = readl(sai->base + FSL_SAI_TCR2);
> > +		tcr2 &= ~FSL_SAI_CR2_DIV_MASK;
> > +		tcr2 |= FSL_SAI_CR2_DIV(div);
> > +		writel(tcr2, sai->base + FSL_SAI_TCR2);
> 
> What is this divider and why does the user have to set it manually?
> 

This is the bit clock divider. I'll add some comments or rename them to be more readable.
>From the IP spec:
++++++
Bit Clock Divide
Divides down the audio master clock to generate the bit clock when configured for an internal bit clock.
------

>From the ASoC subsystem comments we can see that:
++++++
Configures the clock dividers. This is used to derive the best DAI bit and
frame clocks from the system or master clock. It's best to set the DAI bit
and frame clocks as low as possible to save system power.
------


> > +static int fsl_sai_dai_probe(struct snd_soc_dai *dai) {
> > +	int ret;
> > +	struct fsl_sai *sai = dev_get_drvdata(dai->dev);
> > +
> > +	ret = clk_prepare_enable(sai->clk);
> > +	if (ret)
> > +		return ret;
> 
> It'd be nicer to only enable the clock while the device is in active use.
> 

While if the module clock is not enabled here, the followed registers cannot read/write in the same function.
And this _probe function is the _dai_probe not the driver's module _probe.

If the clk_prepare_enable(sai->clk) is not here, where should it be will be nicer ?
One of the following functions ?
        .set_sysclk     = fsl_sai_set_dai_sysclk,
        .set_clkdiv     = fsl_sai_set_dai_clkdiv,
        .set_fmt        = fsl_sai_set_dai_fmt,
        .set_tdm_slot   = fsl_sai_set_dai_tdm_slot,
        .hw_params      = fsl_sai_hw_params,
        .trigger        = fsl_sai_trigger,


> > +	ret = snd_dmaengine_pcm_register(&pdev->dev, NULL,
> > +			SND_DMAENGINE_PCM_FLAG_NO_RESIDUE);
> > +	if (ret)
> > +		return ret;
> 
> We should have a devm_ version of this.

Sorry, is there one patch for adding the devm_ version of snd_dmaengine_pcm_register() already ?
In the -next and other topics branches I could not find it.



Best Regards,
Xiubo

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Mark Brown]

On Mon, Nov 04, 2013 at 07:35:12AM +0000, Li Xiubo wrote:

> From the ASoC subsystem comments we can see that:
> ++++++
> Configures the clock dividers. This is used to derive the best DAI bit and
> frame clocks from the system or master clock. It's best to set the DAI bit
> and frame clocks as low as possible to save system power.
> ------

You should never use this unless you have to, there is no point in every
single machine driver using your driver having to duplicate the same
calculations.

> 
> > > +static int fsl_sai_dai_probe(struct snd_soc_dai *dai) {
> > > +	int ret;
> > > +	struct fsl_sai *sai = dev_get_drvdata(dai->dev);
> > > +
> > > +	ret = clk_prepare_enable(sai->clk);
> > > +	if (ret)
> > > +		return ret;

> > It'd be nicer to only enable the clock while the device is in active use.

> While if the module clock is not enabled here, the followed registers cannot read/write in the same function.
> And this _probe function is the _dai_probe not the driver's module _probe.

So you can enable the clock when you explicitly need to write to the
registers...

> If the clk_prepare_enable(sai->clk) is not here, where should it be will be nicer ?
> One of the following functions ?
>         .set_sysclk     = fsl_sai_set_dai_sysclk,
>         .set_clkdiv     = fsl_sai_set_dai_clkdiv,
>         .set_fmt        = fsl_sai_set_dai_fmt,
>         .set_tdm_slot   = fsl_sai_set_dai_tdm_slot,
>         .hw_params      = fsl_sai_hw_params,
>         .trigger        = fsl_sai_trigger,

It could be in any or all of them except trigger (where the core should
hold a runtime PM reference anyway).  You can always take a reference
for the duration of the function if you're concerned it may be called
when the referent isn't otherwise held.

> > > +	ret = snd_dmaengine_pcm_register(&pdev->dev, NULL,
> > > +			SND_DMAENGINE_PCM_FLAG_NO_RESIDUE);
> > > +	if (ret)
> > > +		return ret;

> > We should have a devm_ version of this.

> Sorry, is there one patch for adding the devm_ version of snd_dmaengine_pcm_register() already ?
> In the -next and other topics branches I could not find it.

No, there isn't one but there should be one.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131104/ae3affc4/attachment.sig>

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Li Xiubo]

> > From the ASoC subsystem comments we can see that:
> > ++++++
> > Configures the clock dividers. This is used to derive the best DAI bit
> > and frame clocks from the system or master clock. It's best to set the
> > DAI bit and frame clocks as low as possible to save system power.
> > ------
> 
> You should never use this unless you have to, there is no point in every
> single machine driver using your driver having to duplicate the same
> calculations.
> 

Okey, I'll think it over, if not have to, I will revise this.

> >
> > > > +static int fsl_sai_dai_probe(struct snd_soc_dai *dai) {
> > > > +	int ret;
> > > > +	struct fsl_sai *sai = dev_get_drvdata(dai->dev);
> > > > +
> > > > +	ret = clk_prepare_enable(sai->clk);
> > > > +	if (ret)
> > > > +		return ret;
> 
> > > It'd be nicer to only enable the clock while the device is in active
> use.
> 
> > While if the module clock is not enabled here, the followed registers
> cannot read/write in the same function.
> > And this _probe function is the _dai_probe not the driver's module
> _probe.
> 
> So you can enable the clock when you explicitly need to write to the
> registers...
> 
> > If the clk_prepare_enable(sai->clk) is not here, where should it be
> will be nicer ?
> > One of the following functions ?
> >         .set_sysclk     = fsl_sai_set_dai_sysclk,
> >         .set_clkdiv     = fsl_sai_set_dai_clkdiv,
> >         .set_fmt        = fsl_sai_set_dai_fmt,
> >         .set_tdm_slot   = fsl_sai_set_dai_tdm_slot,
> >         .hw_params      = fsl_sai_hw_params,
> >         .trigger        = fsl_sai_trigger,
> 
> It could be in any or all of them except trigger (where the core should
> hold a runtime PM reference anyway).  You can always take a reference for
> the duration of the function if you're concerned it may be called when
> the referent isn't otherwise held.
> 

While in this _sai_dai_probe function just followed the clock enable sentence, there are some register writing operations:
The PATCH:
+++++++++++++++++++++++
+static int fsl_sai_dai_probe(struct snd_soc_dai *dai) {
+	int ret;
+	struct fsl_sai *sai = dev_get_drvdata(dai->dev);
+
+	ret = clk_prepare_enable(sai->clk);                     =====> clock enable here
+	if (ret)
+		return ret;
+
+	writel(0x0, sai->base + FSL_SAI_RCSR);			  ======>registers writing here.
+	writel(0x0, sai->base + FSL_SAI_TCSR);			  ======> and here
+	writel(sai->dma_params_tx.maxburst, sai->base + FSL_SAI_TCR1); =======>and here
+	writel(sai->dma_params_rx.maxburst, sai->base + FSL_SAI_RCR1); =======>and here
+
+	dai->playback_dma_data = &sai->dma_params_tx;
+	dai->capture_dma_data = &sai->dma_params_rx;
+
+	snd_soc_dai_set_drvdata(dai, sai);
+
+	return 0;
+}
-------------------------

As your opinions, should I move the four register writing operations to .set_sysclk/set_clkdiv/... functions too ?
Or just add a clk_disable_unprepare() after them here, and then add clk_prepare_enable in one of .set_sysclk/set_clkdiv/...?

And the first two of this four registers must be initialize as early as possible, and if move them to one of the .set_sysclk/set_clkdiv/... functions,
How can I very ensure which one is the first to be called ?
Won't the calling sequence be changed in the feature ?

>From the debug logs, we can see that:
1, _sai_probe
This is called when the machine brings up and has one SAI device.

2, _sai_dai_probe
3, .set_sysclk
4, .set_fmt
Are called in order when the machine has Audio driver and is enabled, and also while the machine brings up.

The above four steps only be called one time in order.

When aplay/arecord is runs the following will be called in order:
5, .set_tdm_slot
6, .hw_param
7, .trigger -->begain 
8, .trigger --> end

The 2,3,4 are always called almost the same time, and they are all have register read/write operations.
Now the clk_prepare_enable() is in step 2, and it won't be any different moving to step 3 or 4.

So, only could move it to step 5 or 6, if so every time the aplay/arecord runs, clk_prepare_enable() will be
called, and there has no chance to call clk_disable_unprepare().

Now from the code we can see that I have add clk_prepare_enable() in _sai_dai_probe() and clk_disable_unprepare() in _sai_dai_remove().
Isn't this okey ?



> > > > +	ret = snd_dmaengine_pcm_register(&pdev->dev, NULL,
> > > > +			SND_DMAENGINE_PCM_FLAG_NO_RESIDUE);
> > > > +	if (ret)
> > > > +		return ret;
> 
> > > We should have a devm_ version of this.
> 
> > Sorry, is there one patch for adding the devm_ version of
> snd_dmaengine_pcm_register() already ?
> > In the -next and other topics branches I could not find it.
> 
> No, there isn't one but there should be one.
>
And if it has existed then I will use it.




BRs,
Xiubo

[PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Mark Brown]

On Tue, Nov 05, 2013 at 03:21:49AM +0000, Li Xiubo wrote:

> As your opinions, should I move the four register writing operations to .set_sysclk/set_clkdiv/... functions too ?
> Or just add a clk_disable_unprepare() after them here, and then add clk_prepare_enable in one of .set_sysclk/set_clkdiv/...?

The latter.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131106/4797fbb6/attachment.sig>

[PATCHv2 2/8] ARM: dts: Add Freescale SAI ALSA SoC Digital Audio Interface node for VF610.

[Xiubo Li]

This patch add the SAI's edma mux Tx and Rx support.

Signed-off-by: Jingchang Lu <b35083@freescale.com>
Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 arch/arm/boot/dts/vf610.dtsi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/vf610.dtsi b/arch/arm/boot/dts/vf610.dtsi
index 18e3a4c..391f180 100644
--- a/arch/arm/boot/dts/vf610.dtsi
+++ b/arch/arm/boot/dts/vf610.dtsi
@@ -151,9 +151,11 @@
 			sai2: sai at 40031000 {
 				compatible = "fsl,vf610-sai";
 				reg = <0x40031000 0x1000>;
-				interrupts = <0 86 0x04>;
 				clocks = <&clks VF610_CLK_SAI2>;
 				clock-names = "sai";
+				dma-names = "tx", "rx";
+				dmas = <&edma0 0 VF610_EDMA_MUXID0_SAI2_TX>,
+					<&edma0 0 VF610_EDMA_MUXID0_SAI2_RX>;
 				status = "disabled";
 			};
 
-- 
1.8.4

[PATCHv2 3/8] ARM: dts: Enables SAI ALSA SoC DAI device for Vybrid VF610 TOWER board.

[Xiubo Li]

This patch add and enable SAI device.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 arch/arm/boot/dts/vf610-twr.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-twr.dts
index 1a58678..e4106dd 100644
--- a/arch/arm/boot/dts/vf610-twr.dts
+++ b/arch/arm/boot/dts/vf610-twr.dts
@@ -57,6 +57,12 @@
 	status = "okay";
 };
 
+&sai2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_sai2_1>;
+	status = "okay";
+};
+
 &uart1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_uart1_1>;
-- 
1.8.4

[PATCHv2 3/8] ARM: dts: Enables SAI ALSA SoC DAI device for Vybrid VF610 TOWER board.

[Bill Pringlemeir]

On  1 Nov 2013, Li.Xiubo at freescale.com wrote:

> This patch add and enable SAI device.
>
> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> ---
> arch/arm/boot/dts/vf610-twr.dts | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-twr.dts
> index 1a58678..e4106dd 100644
> --- a/arch/arm/boot/dts/vf610-twr.dts
> +++ b/arch/arm/boot/dts/vf610-twr.dts
>>> -57,6 +57,12 @@
> 	status = "okay";
> };

Should we put a comment here that a TWR-AUDIO-SGTL board is needed?  Or
is this better in a menuconfig?  I guess TWR-AUDIO-SGTL is needed as
this codec is not on the VF610-TWR main board?

> +&sai2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_sai2_1>;
> +	status = "okay";
> +};
> +

[PATCHv2 3/8] ARM: dts: Enables SAI ALSA SoC DAI device for Vybrid VF610 TOWER board.

[Li Xiubo]

> > This patch add and enable SAI device.
> >
> > arch/arm/boot/dts/vf610-twr.dts | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/vf610-twr.dts
> > b/arch/arm/boot/dts/vf610-twr.dts index 1a58678..e4106dd 100644
> > --- a/arch/arm/boot/dts/vf610-twr.dts
> > +++ b/arch/arm/boot/dts/vf610-twr.dts
> >>> -57,6 +57,12 @@
> > 	status = "okay";
> > };
> 
> Should we put a comment here that a TWR-AUDIO-SGTL board is needed?  Or
> is this better in a menuconfig?  I guess TWR-AUDIO-SGTL is needed as this
> codec is not on the VF610-TWR main board?
> 

Yes, the SGTL5000 codec is not on the VF610-TWR main board, and needs an extra TWR-AUDIO-SGTL board.

IMO, puting this comment in the menuconfig is much better.


--
Best Regards,
Xiubo

[PATCHv2 3/8] ARM: dts: Enables SAI ALSA SoC DAI device for Vybrid VF610 TOWER board.

[Bill Pringlemeir]

[sorry, I should have copied alsa-devel at alsa-project.org previously].

On 19 Nov 2013, Li.Xiubo at freescale.com wrote:

>>> This patch add and enable SAI device.
>>>
>>> arch/arm/boot/dts/vf610-twr.dts | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/vf610-twr.dts
>>> b/arch/arm/boot/dts/vf610-twr.dts index 1a58678..e4106dd 100644
>>> --- a/arch/arm/boot/dts/vf610-twr.dts
>>> +++ b/arch/arm/boot/dts/vf610-twr.dts
>>>>> -57,6 +57,12 @@
>>> 	status = "okay";
>>> };
>>
>> Should we put a comment here that a TWR-AUDIO-SGTL board is needed?  Or
>> is this better in a menuconfig?  I guess TWR-AUDIO-SGTL is needed as this
>> codec is not on the VF610-TWR main board?
>>
>
> Yes, the SGTL5000 codec is not on the VF610-TWR main board, and needs an extra
> 	TWR-AUDIO-SGTL board.
>
> IMO, puting this comment in the menuconfig is much better.

I agree.  I guess the patch set 6 makes more sense.  Also, you might
consider adding a printk in the 'fsl_sgtl5000_probe()'.  I guess that
devm_snd_soc_register_card() will fail if the board is not present?  As
the board is not 'standard' people may come asking why the audio fails
on the 'VF610-TWR'.

+	ret = devm_snd_soc_register_card(&pdev->dev, &fsl_sgt1500_card);
+	if (ret) {
+		dev_err(&pdev->dev, "register soc sound card failed :%d\n"
+                               "TWR-AUDIO-SGTL board required.\n",
+				ret);
+		return ret;
+	}

Especially, you can make one kernel that handles both cases w/wo
TWR-AUDIO-SGTL board attached.

Thanks for your work.
Bill Pringlemeir.

[PATCHv2 3/8] ARM: dts: Enables SAI ALSA SoC DAI device for Vybrid VF610 TOWER board.

[Li Xiubo]

> >>> This patch add and enable SAI device.
> >>>
> >>> arch/arm/boot/dts/vf610-twr.dts | 6 ++++++
> >>> 1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/arch/arm/boot/dts/vf610-twr.dts
> >>> b/arch/arm/boot/dts/vf610-twr.dts index 1a58678..e4106dd 100644
> >>> --- a/arch/arm/boot/dts/vf610-twr.dts
> >>> +++ b/arch/arm/boot/dts/vf610-twr.dts
> >>>>> -57,6 +57,12 @@
> >>> 	status = "okay";
> >>> };
> >>
> >> Should we put a comment here that a TWR-AUDIO-SGTL board is needed?
> >> Or is this better in a menuconfig?  I guess TWR-AUDIO-SGTL is needed
> >> as this codec is not on the VF610-TWR main board?
> >>
> >
> > Yes, the SGTL5000 codec is not on the VF610-TWR main board, and needs
> an extra
> > 	TWR-AUDIO-SGTL board.
> >
> > IMO, puting this comment in the menuconfig is much better.
> 
> I agree.  I guess the patch set 6 makes more sense.  Also, you might
> consider adding a printk in the 'fsl_sgtl5000_probe()'.  I guess that
> devm_snd_soc_register_card() will fail if the board is not present?  
>

Yes, you are right.
The logs:
+++++++++++
vf610-sgtl5000 sound.3: ASoC: CODEC (null) not registered
vf610-sgtl5000 sound.3: register soc sound card failed :-517
-----------


> +	ret = devm_snd_soc_register_card(&pdev->dev, &fsl_sgt1500_card);
> +	if (ret) {
> +		dev_err(&pdev->dev, "register soc sound card failed :%d\n"
> +                               "TWR-AUDIO-SGTL board required.\n",
> +				ret);
> +		return ret;
> +	}
> 
> Especially, you can make one kernel that handles both cases w/wo TWR-
> AUDIO-SGTL board attached.
> 

Yes, it looks nicer.

Well, as one log about the CODEC already exists? Should the "TWR-AUDIO-SGTL board required.\n" log is still needed here ?


--
Best Regards,
Xiubo

[PATCHv2 3/8] ARM: dts: Enables SAI ALSA SoC DAI device for Vybrid VF610 TOWER board.

[Bill Pringlemeir]

On 20 Nov 2013, Li.Xiubo at freescale.com wrote:

>> I agree.  I guess the patch set 6 makes more sense.  Also, you might
>> consider adding a printk in the 'fsl_sgtl5000_probe()'.  I guess that
>> devm_snd_soc_register_card() will fail if the board is not present?  

> Yes, you are right.
> The logs:
> +++++++++++
> vf610-sgtl5000 sound.3: ASoC: CODEC (null) not registered
> vf610-sgtl5000 sound.3: register soc sound card failed :-517
> -----------

>> +	ret = devm_snd_soc_register_card(&pdev->dev, &fsl_sgt1500_card);
>> +	if (ret) {
>> + dev_err(&pdev->dev, "register soc sound card failed :%d\n"
>> +                               "TWR-AUDIO-SGTL board required.\n",
>> +				ret);
>> +		return ret;
>> +	}

>> Especially, you can make one kernel that handles both cases w/wo TWR-
>> AUDIO-SGTL board attached.

> Yes, it looks nicer.

> Well, as one log about the CODEC already exists? Should the "TWR-AUDIO-SGTL
> 	board required.\n" log is still needed here ?

I think that people will understand,

> vf610-sgtl5000 sound.3: ASoC: CODEC (null) not registered
> vf610-sgtl5000 sound.3: TWR-AUDIO-SGTL board required.

As oppose to the typical 'dev_err()' of "register soc sound card failed
:XXX".  This message looks like something abnormal went wrong.  At least
'dev_err()' maybe the wrong level?

With just the error code, people will have to look through code to
diagnose this and may just think that something is wrong with the
driver.  It will be a very common case that a user will not have this
board.  I just think the message could be more explicit to avoid
confusion.  I am not sure if the numeric '-517' will always return in
this case; if so, the message could be conditional.

That is just a suggestion.  I know when I booted the TimeSys version of
Linux and before I looked at the schematic, I didn't know why the codec
failed to register and I thought I had a u-boot issue and/or the code
was not working for my board.

Fwiw,
Bill Pringlemeir.

PS: For those not familiar with the tower.  The VF610-TWR has riser
cards at the sides and different boards can be connected.  The
TWR-AUDIO-SGTL is another board that needs to be plugged in.

[PATCHv2 3/8] ARM: dts: Enables SAI ALSA SoC DAI device for Vybrid VF610 TOWER board.

[Li Xiubo]

> >> +	ret = devm_snd_soc_register_card(&pdev->dev, &fsl_sgt1500_card);
> >> +	if (ret) {
> >> + dev_err(&pdev->dev, "register soc sound card failed :%d\n"
> >> +                               "TWR-AUDIO-SGTL board required.\n",
> >> +				ret);
> >> +		return ret;
> >> +	}
> 
> >> Especially, you can make one kernel that handles both cases w/wo TWR-
> >> AUDIO-SGTL board attached.
> 
> > Yes, it looks nicer.
> 
> > Well, as one log about the CODEC already exists? Should the "TWR-AUDIO-
> SGTL
> > 	board required.\n" log is still needed here ?
> 
> I think that people will understand,
> 
> > vf610-sgtl5000 sound.3: ASoC: CODEC (null) not registered
> > vf610-sgtl5000 sound.3: TWR-AUDIO-SGTL board required.
> 
> As oppose to the typical 'dev_err()' of "register soc sound card
> failed :XXX".  This message looks like something abnormal went wrong.  At
> least 'dev_err()' maybe the wrong level?
> 
> With just the error code, people will have to look through code to
> diagnose this and may just think that something is wrong with the driver.
> It will be a very common case that a user will not have this board.  I
> just think the message could be more explicit to avoid confusion.
>

Yes, I agree.
Well, maybe it not very explicitly points out the actual failling message. And for the tower, the TWR-AUDIO-SGTL board maybe the most likely error case.

>From the ASoC subsystem, we can see that there are more than 2 situations that will return the numeric '-517', and before that there will be one explicit failing message printing out as we have seen.

How about putting this comment in the menuconfig ?

>
> I am not sure if the numeric '-517' will always return in this case; if so,
> the message could be conditional.
> 

The following logs are the situation:
1, Plug in the TWR-AUDIO-SGTL sub-board.
2, Enable the SGTL5000 driver.
3, Disable the CPU DAI driver, here it's the SAI driver.
++++++++++++
sgtl5000 0-000a: sgtl5000 revision 0x11
vf610-sgtl5000 sound.3: ASoC: CPU DAI (null) not registered
vf610-sgtl5000 sound.3: register soc sound card failed :-517
------------
 
And the failed numeric is '-517' too.



> That is just a suggestion.  I know when I booted the TimeSys version of
> Linux and before I looked at the schematic, I didn't know why the codec
> failed to register and I thought I had a u-boot issue and/or the code was
> not working for my board.
> 
> Fwiw,
> Bill Pringlemeir.
> 
> PS: For those not familiar with the tower.  The VF610-TWR has riser cards
> at the sides and different boards can be connected.  The TWR-AUDIO-SGTL
> is another board that needs to be plugged in.

[PATCHv2 3/8] ARM: dts: Enables SAI ALSA SoC DAI device for Vybrid VF610 TOWER board.

[Bill Pringlemeir]

On 22 Nov 2013, Li.Xiubo at freescale.com wrote:

>> I think that people will understand,

>>> vf610-sgtl5000 sound.3: ASoC: CODEC (null) not registered
>>> vf610-sgtl5000 sound.3: TWR-AUDIO-SGTL board required.

>> As oppose to the typical 'dev_err()' of "register soc sound card
>> failed :XXX".  This message looks like something abnormal went wrong.  At
>> least 'dev_err()' maybe the wrong level?

>> With just the error code, people will have to look through code to
>> diagnose this and may just think that something is wrong with the driver.
>> It will be a very common case that a user will not have this board.  I
>> just think the message could be more explicit to avoid confusion.

> Yes, I agree.  Well, maybe it not very explicitly points out the
> actual failling message. And for the tower, the TWR-AUDIO-SGTL board
> maybe the most likely error case.

> From the ASoC subsystem, we can see that there are more than 2
> situations that will return the numeric '-517', and before that there
> will be one explicit failing message printing out as we have seen.

> How about putting this comment in the menuconfig ?

> The following logs are the situation:
> 1, Plug in the TWR-AUDIO-SGTL sub-board.
> 2, Enable the SGTL5000 driver.
> 3, Disable the CPU DAI driver, here it's the SAI driver.
> ++++++++++++
> sgtl5000 0-000a: sgtl5000 revision 0x11
> vf610-sgtl5000 sound.3: ASoC: CPU DAI (null) not registered
> vf610-sgtl5000 sound.3: register soc sound card failed :-517
> ------------

> And the failed numeric is '-517' too.

Won't that be prevented by,

+config SND_SOC_FSL_SGTL5000_VF610
+	tristate "SoC Audio support for FSL boards with sgtl5000"
+	depends on OF && I2C
+	select SND_SOC_FSL_SAI

Or do you have to manually edit the '.config' to get this?  Maybe you
could also get this message by messing with the vf610-twr.dts 'sound'
stanza.  Maybe these people are a little more use to being bitten?

>> That is just a suggestion.  I know when I booted the TimeSys version of
>> Linux and before I looked at the schematic, I didn't know why the codec
>> failed to register and I thought I had a u-boot issue and/or the code was
>> not working for my board.

>> PS: For those not familiar with the tower.  The VF610-TWR has riser cards
>> at the sides and different boards can be connected.  The TWR-AUDIO-SGTL
>> is another board that needs to be plugged in.

I look for other places that a board has an optional daughter board.  For
the most part, the menuconfig had an option to include the driver and
then platform initialization code 'probed' the device and provide a
message.

For instance, the 3ds_debugboard.c in mxc_expio_init() has a message,

 pr_info("3-Stack Debug board not detected\n");

I don't know if there is an Alsa standard mechanism for handling this or
something in the device tree infrastructure.  However, I am pretty sure
that we could say the board is not there if the i2c device doesn't
respond with it's id.  But maybe this is hidden by the ASOC
infrastructure?

It will be quite common for people to run a 'imx_v6_v7_defconfig' with
the 'vf610-twr.dts'.  These people won't read a menu config; in fact the
binary maybe delivered to them.

I read the error as 'EPROBE_DEFER'.  We have this in the SGTL5000 codec
and soc_core.c.  The sgtl5000_i2c_probe() will never be called if the
board is not present.  So I think the error is from soc_bind_dai_link(),

	if (!rtd->codec) {
		dev_err(card->dev, "ASoC: CODEC %s not registered\n",
			dai_link->codec_name);
		return -EPROBE_DEFER;
	}

I guess the issue is the order of the sub-system initialization and the
soc-core doesn't want to make any assumptions about this.  I am not sure
if there is some way an SOC machine file can look to see if a codec is
not populated in it's probe.  Or if it is expected that the machine file
will be probed multiple times when '-EPROBE_DEFER' is returned.  This
maybe for either the DAI or the codec and is abnormal for the machine.

In any case, an "TWR-AUDIO-SGTL board required.\n" would be fairly
obvious to anyone reading the logs.  They should be able to see if they
have this board or not.  If they have it, then the error -517 is from
something else.  I agree that always printing this out is not the most
elegant, but I think it is the more pragmatic than not having it.

Fwiw,
Bill Pringlemeir.

[PATCHv2 3/8] ARM: dts: Enables SAI ALSA SoC DAI device for Vybrid VF610 TOWER board.

[Li Xiubo]

> > The following logs are the situation:
> > 1, Plug in the TWR-AUDIO-SGTL sub-board.
> > 2, Enable the SGTL5000 driver.
> > 3, Disable the CPU DAI driver, here it's the SAI driver.
> > ++++++++++++
> > sgtl5000 0-000a: sgtl5000 revision 0x11
> > vf610-sgtl5000 sound.3: ASoC: CPU DAI (null) not registered
> > vf610-sgtl5000 sound.3: register soc sound card failed :-517
> > ------------
> 
> > And the failed numeric is '-517' too.
> 
> Won't that be prevented by,
> 
> +config SND_SOC_FSL_SGTL5000_VF610
> +	tristate "SoC Audio support for FSL boards with sgtl5000"
> +	depends on OF && I2C
> +	select SND_SOC_FSL_SAI
> 
> Or do you have to manually edit the '.config' to get this?

Yes, it is.


> >> That is just a suggestion.  I know when I booted the TimeSys version of
> >> Linux and before I looked at the schematic, I didn't know why the codec
> >> failed to register and I thought I had a u-boot issue and/or the code
> was
> >> not working for my board.
> 
> >> PS: For those not familiar with the tower.  The VF610-TWR has riser
> cards
> >> at the sides and different boards can be connected.  The TWR-AUDIO-SGTL
> >> is another board that needs to be plugged in.
> 
> I look for other places that a board has an optional daughter board.  For
> the most part, the menuconfig had an option to include the driver and
> then platform initialization code 'probed' the device and provide a
> message.
> 
> For instance, the 3ds_debugboard.c in mxc_expio_init() has a message,
> 
>  pr_info("3-Stack Debug board not detected\n");
> 
> I don't know if there is an Alsa standard mechanism for handling this or
> something in the device tree infrastructure.  However, I am pretty sure
> that we could say the board is not there if the i2c device doesn't
> respond with it's id.  But maybe this is hidden by the ASOC
> infrastructure?
> 
> It will be quite common for people to run a 'imx_v6_v7_defconfig' with
> the 'vf610-twr.dts'.  These people won't read a menu config; in fact the
> binary maybe delivered to them.
> 
> I read the error as 'EPROBE_DEFER'.  We have this in the SGTL5000 codec
> and soc_core.c.  The sgtl5000_i2c_probe() will never be called if the
> board is not present.  So I think the error is from soc_bind_dai_link(),
> 
> 	if (!rtd->codec) {
> 		dev_err(card->dev, "ASoC: CODEC %s not registered\n",
> 			dai_link->codec_name);
> 		return -EPROBE_DEFER;
> 	}
> 
> I guess the issue is the order of the sub-system initialization and the
> soc-core doesn't want to make any assumptions about this.  I am not sure
> if there is some way an SOC machine file can look to see if a codec is
> not populated in it's probe.  Or if it is expected that the machine file
> will be probed multiple times when '-EPROBE_DEFER' is returned.  This
> maybe for either the DAI or the codec and is abnormal for the machine.
> 
> In any case, an "TWR-AUDIO-SGTL board required.\n" would be fairly
> obvious to anyone reading the logs.  They should be able to see if they
> have this board or not.  If they have it, then the error -517 is from
> something else.  I agree that always printing this out is not the most
> elegant, but I think it is the more pragmatic than not having it.
> 

Okey, I will think it over and revise this.

--
Best Regards,

[PATCHv2 4/8] Documentation: Add device tree bindings for Freescale SAI.

[Xiubo Li]

This adds the Document for Freescale SAI driver under
Documentation/devicetree/bindings/sound/.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 .../devicetree/bindings/sound/fsl-sai.txt          | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/fsl-sai.txt

diff --git a/Documentation/devicetree/bindings/sound/fsl-sai.txt b/Documentation/devicetree/bindings/sound/fsl-sai.txt
new file mode 100644
index 0000000..267afbd
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/fsl-sai.txt
@@ -0,0 +1,32 @@
+Freescale Synchronous Audio Interface (SAI).
+
+The SAI is based on I2S module that used communicating with audio codecs,
+which provides a synchronous audio interface that supports fullduplex
+serial interfaces with frame synchronization such as I2S, AC97, TDM, and
+codec/DSP interfaces.
+
+
+Required properties:
+- compatible: Compatible list, contains "fsl,vf610-sai".
+- reg: Offset and length of the register set for the device.
+- clocks: Must contain an entry for each entry in clock-names.
+- clock-names : Must include the "sai" entry.
+- dmas : Generic dma devicetree binding as described in
+  Documentation/devicetree/bindings/dma/dma.txt.
+- dma-names : Two dmas have to be defined, "tx" and "rx".
+- pinctrl-names: Must contain a "default" entry.
+- pinctrl-NNN: One property must exist for each entry in pinctrl-names.
+  See ../pinctrl/pinctrl-bindings.txt for details of the property values.
+
+Example:
+sai2: sai at 40031000 {
+	      compatible = "fsl,vf610-sai";
+	      reg = <0x40031000 0x1000>;
+	      pinctrl-names = "default";
+	      pinctrl-0 = <&pinctrl_sai2_1>;
+	      clocks = <&clks VF610_CLK_SAI2>;
+	      clock-names = "sai";
+	      dma-names = "tx", "rx";
+	      dmas = <&edma0 0 VF610_EDMA_MUXID0_SAI2_TX>,
+		   <&edma0 0 VF610_EDMA_MUXID0_SAI2_RX>;
+};
-- 
1.8.4

[PATCHv2 5/8] ASoC: SGTL5000: Enhance the SGTL5000 codec driver about regulator.

[Xiubo Li]

On VF610 series there are no regulators used, and now whether the
CONFIG_REGULATOR mirco is enabled or not, for the VF610 audio
patch series, the board cannot be probe successfully.
And this patch will solve this issue.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 sound/soc/codecs/sgtl5000.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 1f4093f..c2f6d86 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -61,6 +61,7 @@ static const struct reg_default sgtl5000_reg_defaults[] = {
 	{ SGTL5000_DAP_AVC_DECAY,		0x0050 },
 };
 
+#ifdef CONFIG_REGULATOR
 /* regulator supplies for sgtl5000, VDDD is an optional external supply */
 enum sgtl5000_regulator_supplies {
 	VDDA,
@@ -93,6 +94,9 @@ static struct regulator_init_data ldo_init_data = {
 	.num_consumer_supplies = 1,
 	.consumer_supplies = &ldo_consumer[0],
 };
+#else
+#define SGTL5000_SUPPLY_NUM 0
+#endif
 
 /*
  * sgtl5000 internal ldo regulator,
@@ -112,7 +116,9 @@ struct sgtl5000_priv {
 	int master;	/* i2s master or not */
 	int fmt;	/* i2s data format */
 	struct regulator_bulk_data supplies[SGTL5000_SUPPLY_NUM];
+#ifdef CONFIG_REGULATOR
 	struct ldo_regulator *ldo;
+#endif
 	struct regmap *regmap;
 	struct clk *mclk;
 };
@@ -879,6 +885,7 @@ static int ldo_regulator_remove(struct snd_soc_codec *codec)
 	return 0;
 }
 #else
+#ifndef CONFIG_SND_SOC_FSL_SGTL5000_VF610
 static int ldo_regulator_register(struct snd_soc_codec *codec,
 				struct regulator_init_data *init_data,
 				int voltage)
@@ -886,6 +893,7 @@ static int ldo_regulator_register(struct snd_soc_codec *codec,
 	dev_err(codec->dev, "this setup needs regulator support in the kernel\n");
 	return -EINVAL;
 }
+#endif
 
 static int ldo_regulator_remove(struct snd_soc_codec *codec)
 {
@@ -1137,6 +1145,7 @@ static int sgtl5000_resume(struct snd_soc_codec *codec)
 #define sgtl5000_resume  NULL
 #endif	/* CONFIG_SUSPEND */
 
+#ifdef CONFIG_REGULATOR
 /*
  * sgtl5000 has 3 internal power supplies:
  * 1. VAG, normally set to vdda/2
@@ -1373,6 +1382,7 @@ err_regulator_free:
 	return ret;
 
 }
+#endif
 
 static int sgtl5000_probe(struct snd_soc_codec *codec)
 {
@@ -1387,6 +1397,7 @@ static int sgtl5000_probe(struct snd_soc_codec *codec)
 		return ret;
 	}
 
+#ifdef CONFIG_REGULATOR
 	ret = sgtl5000_enable_regulators(codec);
 	if (ret)
 		return ret;
@@ -1395,6 +1406,7 @@ static int sgtl5000_probe(struct snd_soc_codec *codec)
 	ret = sgtl5000_set_power_regs(codec);
 	if (ret)
 		goto err;
+#endif
 
 	/* enable small pop, introduce 400ms delay in turning off */
 	snd_soc_update_bits(codec, SGTL5000_CHIP_REF_CTRL,
-- 
1.8.4

[PATCHv2 5/8] ASoC: SGTL5000: Enhance the SGTL5000 codec driver about regulator.

[Nicolin Chen]

On Fri, Nov 01, 2013 at 03:04:52PM +0800, Xiubo Li wrote:
> On VF610 series there are no regulators used, and now whether the
> CONFIG_REGULATOR mirco is enabled or not, for the VF610 audio

micro? or macro?

> patch series, the board cannot be probe successfully.
> And this patch will solve this issue.
> 

I finally got your idea about what this patch does. But it seems to
be more likely a work around. Maybe I here can't think it through
comprehensively, but I think you can try to add a fixed regulator to
sgtl5000 in the devicetree, rather than disabling common functions
even if not breaking others.

> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> ---
>  sound/soc/codecs/sgtl5000.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
> index 1f4093f..c2f6d86 100644
> --- a/sound/soc/codecs/sgtl5000.c
> +++ b/sound/soc/codecs/sgtl5000.c
> @@ -61,6 +61,7 @@ static const struct reg_default sgtl5000_reg_defaults[] = {
>  	{ SGTL5000_DAP_AVC_DECAY,		0x0050 },
>  };
>  
> +#ifdef CONFIG_REGULATOR
>  /* regulator supplies for sgtl5000, VDDD is an optional external supply */
>  enum sgtl5000_regulator_supplies {
>  	VDDA,
> @@ -93,6 +94,9 @@ static struct regulator_init_data ldo_init_data = {
>  	.num_consumer_supplies = 1,
>  	.consumer_supplies = &ldo_consumer[0],
>  };
> +#else
> +#define SGTL5000_SUPPLY_NUM 0
> +#endif
>  
>  /*
>   * sgtl5000 internal ldo regulator,
> @@ -112,7 +116,9 @@ struct sgtl5000_priv {
>  	int master;	/* i2s master or not */
>  	int fmt;	/* i2s data format */
>  	struct regulator_bulk_data supplies[SGTL5000_SUPPLY_NUM];
> +#ifdef CONFIG_REGULATOR
>  	struct ldo_regulator *ldo;
> +#endif
>  	struct regmap *regmap;
>  	struct clk *mclk;
>  };
> @@ -879,6 +885,7 @@ static int ldo_regulator_remove(struct snd_soc_codec *codec)
>  	return 0;
>  }
>  #else
> +#ifndef CONFIG_SND_SOC_FSL_SGTL5000_VF610

Checking a platform or SoC specific define here is just a bit....

Could you pls find a better solution? Like I said at first, use fixed regulator
or figure out why your System would hang with current sgtl5000 driver. If it
really has a critical bug in its regulator-related code, I think you can then
make a greater contribution by fixing the bug rather than bypassing it.

Best regards,
Nicolin Chen

>  static int ldo_regulator_register(struct snd_soc_codec *codec,
>  				struct regulator_init_data *init_data,
>  				int voltage)
> @@ -886,6 +893,7 @@ static int ldo_regulator_register(struct snd_soc_codec *codec,
>  	dev_err(codec->dev, "this setup needs regulator support in the kernel\n");
>  	return -EINVAL;
>  }
> +#endif
>  
>  static int ldo_regulator_remove(struct snd_soc_codec *codec)
>  {
> @@ -1137,6 +1145,7 @@ static int sgtl5000_resume(struct snd_soc_codec *codec)
>  #define sgtl5000_resume  NULL
>  #endif	/* CONFIG_SUSPEND */
>  
> +#ifdef CONFIG_REGULATOR
>  /*
>   * sgtl5000 has 3 internal power supplies:
>   * 1. VAG, normally set to vdda/2
> @@ -1373,6 +1382,7 @@ err_regulator_free:
>  	return ret;
>  
>  }
> +#endif
>  
>  static int sgtl5000_probe(struct snd_soc_codec *codec)
>  {
> @@ -1387,6 +1397,7 @@ static int sgtl5000_probe(struct snd_soc_codec *codec)
>  		return ret;
>  	}
>  
> +#ifdef CONFIG_REGULATOR
>  	ret = sgtl5000_enable_regulators(codec);
>  	if (ret)
>  		return ret;
> @@ -1395,6 +1406,7 @@ static int sgtl5000_probe(struct snd_soc_codec *codec)
>  	ret = sgtl5000_set_power_regs(codec);
>  	if (ret)
>  		goto err;
> +#endif
>  
>  	/* enable small pop, introduce 400ms delay in turning off */
>  	snd_soc_update_bits(codec, SGTL5000_CHIP_REF_CTRL,
> -- 
> 1.8.4
> 

[PATCHv2 5/8] ASoC: SGTL5000: Enhance the SGTL5000 codec driver about regulator.

[Mark Brown]

On Fri, Nov 01, 2013 at 03:04:52PM +0800, Xiubo Li wrote:
> On VF610 series there are no regulators used, and now whether the
> CONFIG_REGULATOR mirco is enabled or not, for the VF610 audio
> patch series, the board cannot be probe successfully.
> And this patch will solve this issue.

I don't understand what this is for at all, you're just saying there is
a problem you're trying to solve but you don't explain anything about
what the problem is or how your changes address it.

> +#ifndef CONFIG_SND_SOC_FSL_SGTL5000_VF610
>  static int ldo_regulator_register(struct snd_soc_codec *codec,

This is definitely broken, it won't work with multi-platform kernels,
and I don't understand what this is supposed to do - what is the reason
for making this change?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131101/00662d36/attachment.sig>

[PATCHv2 5/8] ASoC: SGTL5000: Enhance the SGTL5000 codec driver about regulator.

[Li Xiubo]

> > On VF610 series there are no regulators used, and now whether the
> > CONFIG_REGULATOR mirco is enabled or not, for the VF610 audio patch
> > series, the board cannot be probe successfully.
> > And this patch will solve this issue.
> 
> I don't understand what this is for at all, you're just saying there is a
> problem you're trying to solve but you don't explain anything about what
> the problem is or how your changes address it.
> 

Sorry for confusing.

The SGTL5000 is based on regulators and when it is disabled, there will be an error returns directly while the SGTL5000 codec is probing.
But on VF610 boards there has no regulators or needn't configure them, so the CONFIG_REGULATOR is disabled, and then the error is returned causing
to the SGTL5000 probe broken. 

Has I make it clear?

> > +#ifndef CONFIG_SND_SOC_FSL_SGTL5000_VF610
> >  static int ldo_regulator_register(struct snd_soc_codec *codec,
> 
> This is definitely broken, it won't work with multi-platform kernels, and
> I don't understand what this is supposed to do - what is the reason for
> making this change?
>

Yes, it is.

How about adding one node properties like : "regulator-exist", which will be controlled by platform data ?

[PATCHv2 5/8] ASoC: SGTL5000: Enhance the SGTL5000 codec driver about regulator.

[Mark Brown]

On Wed, Nov 06, 2013 at 08:59:53AM +0000, Li Xiubo wrote:

Please fix your mailer to word wrap within paragraphs.

> The SGTL5000 is based on regulators and when it is disabled, there
> will be an error returns directly while the SGTL5000 codec is probing.

What makes you say this?  That's not how the regulator API works.  Have
you actually seen any problems?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131106/3e04c897/attachment.sig>

[PATCHv2 5/8] ASoC: SGTL5000: Enhance the SGTL5000 codec driver about regulator.

[Li Xiubo]

> > The SGTL5000 is based on regulators and when it is disabled, there
> > will be an error returns directly while the SGTL5000 codec is probing.
> 
> What makes you say this?  
>

>From the code:
File path: "sound/soc/codecs/sgtl5000.c"
==================
#ifdef CONFIG_REGULATOR
.....
#else
static int ldo_regulator_register(struct snd_soc_codec *codec,
                                struct regulator_init_data *init_data,
                                int voltage)
{
        dev_err(codec->dev, "this setup needs regulator support in the kernel\n");
        return -EINVAL;
}

static int ldo_regulator_remove(struct snd_soc_codec *codec)
{
        return 0;
}
#endif
-------------------

sgtl5000_probe() -->
sgtl5000_enable_regulators() -->
sgtl5000_replace_vddd_with_ldo() -->
ldo_regulator_register().


> That's not how the regulator API works.  Have
> you actually seen any problems?

There are some regulator APIs implemented by SGTL5000 driver, not the regulator subsystem APIs.

[PATCHv2 5/8] ASoC: SGTL5000: Enhance the SGTL5000 codec driver about regulator.

[Mark Brown]

On Thu, Nov 07, 2013 at 03:01:02AM +0000, Li Xiubo wrote:
> > > The SGTL5000 is based on regulators and when it is disabled, there
> > > will be an error returns directly while the SGTL5000 codec is probing.

> > What makes you say this?  

> static int ldo_regulator_register(struct snd_soc_codec *codec,
>                                 struct regulator_init_data *init_data,
>                                 int voltage)

If the regulator is not used in the system then why is the driver
getting as far as trying to register it?  Surely this is a system
configuration error?  This all sounds like some problem with either the
system integration or the driver which is causing it to try to register
the regulator needlessly - you should be fixing that problem, not adding
ifdefs.  I'm still unclear on exactly what the issue is so it's hard to
say exactly what the best way forwards is.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131107/fd5aef54/attachment-0001.sig>

[PATCHv2 6/8] ASoC: fsl: add SGTL5000 based audio machine driver.

[Xiubo Li]

This is the SGTL5000 codec based audio driver supported with both
playback and capture dai link implemention.

This implementation is only compatible with device tree definition.

Signed-off-by: Alison Wang <b18965 at freescale.com
Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>

Conflicts:
	sound/soc/fsl/Makefile
---
 sound/soc/fsl/Kconfig              |  10 ++
 sound/soc/fsl/Makefile             |   2 +
 sound/soc/fsl/fsl-sgtl5000-vf610.c | 208 +++++++++++++++++++++++++++++++++++++
 3 files changed, 220 insertions(+)
 create mode 100644 sound/soc/fsl/fsl-sgtl5000-vf610.c

diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig
index 9a8851e..1b835ba 100644
--- a/sound/soc/fsl/Kconfig
+++ b/sound/soc/fsl/Kconfig
@@ -228,4 +228,14 @@ config SND_SOC_FSL_SAI
 	tristate
 	select SND_SOC_GENERIC_DMAENGINE_PCM
 
+config SND_SOC_FSL_SGTL5000_VF610
+	tristate "SoC Audio support for FSL boards with sgtl5000"
+	depends on OF && I2C
+	select SND_SOC_FSL_SAI
+	select SND_SOC_FSL_PCM
+	select SND_SOC_SGTL5000
+	help
+	  Say Y if you want to add support for SoC audio on an FSL board with
+	  a sgtl5000 codec.
+
 endif # SND_FSL_SOC
diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile
index e5acc03..26fc551 100644
--- a/sound/soc/fsl/Makefile
+++ b/sound/soc/fsl/Makefile
@@ -59,5 +59,7 @@ obj-$(CONFIG_SND_SOC_IMX_MC13783) += snd-soc-imx-mc13783.o
 
 # FSL ARM SAI/SGT15000 Platform Support
 snd-soc-fsl-sai-objs := fsl-sai.o
+snd-soc-fsl-sgtl5000-vf610-objs := fsl-sgtl5000-vf610.o
 
 obj-$(CONFIG_SND_SOC_FSL_SAI) += snd-soc-fsl-sai.o
+obj-$(CONFIG_SND_SOC_FSL_SGTL5000_VF610) += snd-soc-fsl-sgtl5000-vf610.o
diff --git a/sound/soc/fsl/fsl-sgtl5000-vf610.c b/sound/soc/fsl/fsl-sgtl5000-vf610.c
new file mode 100644
index 0000000..f535b42
--- /dev/null
+++ b/sound/soc/fsl/fsl-sgtl5000-vf610.c
@@ -0,0 +1,208 @@
+/*
+ * Freeacale ALSA SoC Audio using SGT1500 as codec.
+ *
+ * Copyright 2012-2013 Freescale Semiconductor, Inc.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/i2c.h>
+#include <linux/clk.h>
+
+#include "../codecs/sgtl5000.h"
+#include "fsl-sai.h"
+
+static unsigned int sysclk_rate;
+
+static int fsl_sgtl5000_dai_init(struct snd_soc_pcm_runtime *rtd)
+{
+	int ret;
+	struct device *dev = rtd->card->dev;
+
+	ret = snd_soc_dai_set_sysclk(rtd->codec_dai, SGTL5000_SYSCLK,
+				     sysclk_rate, SND_SOC_CLOCK_IN);
+	if (ret) {
+		dev_err(dev, "could not set codec driver clock params :%d\n",
+				ret);
+		return ret;
+	}
+
+	ret = snd_soc_dai_set_sysclk(rtd->cpu_dai, FSL_SAI_CLK_BUS,
+				     sysclk_rate, SND_SOC_CLOCK_OUT);
+	if (ret) {
+		dev_err(dev, "could not set cpu dai driver clock params :%d\n",
+				ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int sgtl5000_params(struct snd_pcm_substream *substream,
+		struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	unsigned int channels = params_channels(params);
+
+	/* TODO: The SAI driver should figure this out for us */
+	switch (channels) {
+	case 2:
+		snd_soc_dai_set_tdm_slot(cpu_dai, 0xfffffffc, 0xfffffffc, 2, 0);
+		break;
+	case 1:
+		snd_soc_dai_set_tdm_slot(cpu_dai, 0xfffffffe, 0xfffffffe, 1, 0);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static struct snd_soc_ops fsl_sgtl5000_hifi_ops = {
+	.hw_params = sgtl5000_params,
+};
+
+static struct snd_soc_dai_link fsl_sgtl5000_dai = {
+	.name = "HiFi",
+	.stream_name = "HiFi",
+	.codec_dai_name = "sgtl5000",
+	.init = &fsl_sgtl5000_dai_init,
+	.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
+		SND_SOC_DAIFMT_CBM_CFM,
+	.ops = &fsl_sgtl5000_hifi_ops,
+};
+
+static const struct snd_soc_dapm_widget fsl_sgtl5000_dapm_widgets[] = {
+	SND_SOC_DAPM_MIC("Mic Jack", NULL),
+	SND_SOC_DAPM_LINE("Line In Jack", NULL),
+	SND_SOC_DAPM_HP("Headphone Jack", NULL),
+	SND_SOC_DAPM_SPK("Line Out Jack", NULL),
+	SND_SOC_DAPM_SPK("Ext Spk", NULL),
+};
+
+static struct snd_soc_card fsl_sgt1500_card = {
+	.owner = THIS_MODULE,
+	.num_links = 1,
+	.dai_link = &fsl_sgtl5000_dai,
+	.dapm_widgets = fsl_sgtl5000_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(fsl_sgtl5000_dapm_widgets),
+};
+
+static int fsl_sgtl5000_parse_dt(struct platform_device *pdev)
+{
+	int ret;
+	struct device_node *sai_np, *codec_np;
+	struct clk *codec_clk;
+	struct i2c_client *codec_dev;
+	struct device_node *np = pdev->dev.of_node;
+
+	ret = snd_soc_of_parse_card_name(&fsl_sgt1500_card, "model");
+	if (ret)
+		return ret;
+
+	ret = snd_soc_of_parse_audio_routing(&fsl_sgt1500_card,
+			"audio-routing");
+	if (ret)
+		return ret;
+
+	sai_np = of_parse_phandle(np, "saif-controller", 0);
+	if (!sai_np) {
+		dev_err(&pdev->dev, "\"saif-controller\" phandle missing or "
+				"invalid\n");
+		return -EINVAL;
+	}
+	fsl_sgtl5000_dai.cpu_of_node = sai_np;
+	fsl_sgtl5000_dai.platform_of_node = sai_np;
+
+	codec_np = of_parse_phandle(np, "audio-codec", 0);
+	if (!codec_np) {
+		dev_err(&pdev->dev, "\"audio-codec\" phandle missing or "
+				"invalid\n");
+		ret = -EINVAL;
+		goto sai_np_fail;
+	}
+	fsl_sgtl5000_dai.codec_of_node = codec_np;
+
+	codec_dev = of_find_i2c_device_by_node(codec_np);
+	if (!codec_dev) {
+		dev_err(&pdev->dev, "failed to find codec platform device\n");
+		ret = PTR_ERR(codec_dev);
+		goto codec_np_fail;
+	}
+
+	codec_clk = devm_clk_get(&codec_dev->dev, NULL);
+	if (IS_ERR(codec_clk)) {
+		dev_err(&pdev->dev, "failed to get codec clock\n");
+		ret = PTR_ERR(codec_clk);
+		goto codec_np_fail;
+	}
+
+	sysclk_rate = clk_get_rate(codec_clk);
+
+codec_np_fail:
+	of_node_put(codec_np);
+sai_np_fail:
+	of_node_put(sai_np);
+
+	return ret;
+}
+
+static int fsl_sgtl5000_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	fsl_sgt1500_card.dev = &pdev->dev;
+
+	ret = fsl_sgtl5000_parse_dt(pdev);
+	if (ret) {
+		dev_err(&pdev->dev,
+				"parse sgtl5000 device tree failed :%d\n",
+				ret);
+		return ret;
+	}
+
+	ret = devm_snd_soc_register_card(&pdev->dev, &fsl_sgt1500_card);
+	if (ret) {
+		dev_err(&pdev->dev, "register soc sound card failed :%d\n",
+				ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int fsl_sgtl5000_remove(struct platform_device *pdev)
+{
+	snd_soc_unregister_card(&fsl_sgt1500_card);
+
+	return 0;
+}
+
+static const struct of_device_id fsl_sgtl5000_dt_ids[] = {
+	{ .compatible = "fsl,vf610-sgtl5000", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, fsl_sgtl5000_dt_ids);
+
+static struct platform_driver fsl_sgtl5000_driver = {
+	.driver = {
+		.name = "fsl-sgtl5000",
+		.owner = THIS_MODULE,
+		.of_match_table = fsl_sgtl5000_dt_ids,
+	},
+	.probe = fsl_sgtl5000_probe,
+	.remove = fsl_sgtl5000_remove,
+};
+module_platform_driver(fsl_sgtl5000_driver);
+
+MODULE_AUTHOR("Xiubo Li <Li.Xiubo@freescale.com>");
+MODULE_DESCRIPTION("Freescale SGTL5000 ASoC driver");
+MODULE_LICENSE("GPL");
-- 
1.8.4

[PATCHv2 6/8] ASoC: fsl: add SGTL5000 based audio machine driver.

[Oskar Schirmer]

Not that it would improve functionality, but:

On Fri, Nov 01, 2013 at 15:04:53 +0800, Xiubo Li wrote:
[...]
> diff --git a/sound/soc/fsl/fsl-sgtl5000-vf610.c b/sound/soc/fsl/fsl-sgtl5000-vf610.c
> new file mode 100644
> index 0000000..f535b42
> --- /dev/null
> +++ b/sound/soc/fsl/fsl-sgtl5000-vf610.c
> @@ -0,0 +1,208 @@
> +/*
> + * Freeacale ALSA SoC Audio using SGT1500 as codec.
          ^                             ^   ^
     "Freescale"                    "SGTL5000"

regards,
  Oskar

[PATCHv2 6/8] ASoC: fsl: add SGTL5000 based audio machine driver.

[Li Xiubo]

=
> [...]
> > diff --git a/sound/soc/fsl/fsl-sgtl5000-vf610.c b/sound/soc/fsl/fsl-
> sgtl5000-vf610.c
> > new file mode 100644
> > index 0000000..f535b42
> > --- /dev/null
> > +++ b/sound/soc/fsl/fsl-sgtl5000-vf610.c
> > @@ -0,0 +1,208 @@
> > +/*
> > + * Freeacale ALSA SoC Audio using SGT1500 as codec.
>           ^                             ^   ^
>      "Freescale"                    "SGTL5000"
> 
Yes. See the next version.

[PATCHv2 6/8] ASoC: fsl: add SGTL5000 based audio machine driver.

[Nicolin Chen]

Hi Xiubo,

On Fri, Nov 01, 2013 at 03:04:53PM +0800, Xiubo Li wrote:
> This is the SGTL5000 codec based audio driver supported with both
> playback and capture dai link implemention.
> 
> This implementation is only compatible with device tree definition.
> 
> Signed-off-by: Alison Wang <b18965@freescale.com
> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> 
> Conflicts:
> 	sound/soc/fsl/Makefile
> ---
>  sound/soc/fsl/Kconfig              |  10 ++
>  sound/soc/fsl/Makefile             |   2 +

>  sound/soc/fsl/fsl-sgtl5000-vf610.c | 208 +++++++++++++++++++++++++++++++++++++

I just doubt if this file naming is appropriate. Even if we might not have
rigor rule for the file names, according to existing ones, they are all in
a same pattern: [SoC name]-[codec name].c

"imx-sgtl5000.c" for example

I think it would make user less confused about what this file exactly is if
this machine driver also follow the pattern: vf610-sgtl5000.c


@Shawn

What do you think about the file name?

>  3 files changed, 220 insertions(+)
>  create mode 100644 sound/soc/fsl/fsl-sgtl5000-vf610.c
> 
> diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig
> index 9a8851e..1b835ba 100644
> --- a/sound/soc/fsl/Kconfig
> +++ b/sound/soc/fsl/Kconfig
> @@ -228,4 +228,14 @@ config SND_SOC_FSL_SAI
>  	tristate
>  	select SND_SOC_GENERIC_DMAENGINE_PCM
>  
> +config SND_SOC_FSL_SGTL5000_VF610

Same problem with the this define.

> +	tristate "SoC Audio support for FSL boards with sgtl5000"

And 'FSL' here confuses me a lot. Because those boards based on i.MX series
also could be called FSL boards.

> +	depends on OF && I2C
> +	select SND_SOC_FSL_SAI
> +	select SND_SOC_FSL_PCM
> +	select SND_SOC_SGTL5000
> +	help
> +	  Say Y if you want to add support for SoC audio on an FSL board with
> +	  a sgtl5000 codec.
> +
>  endif # SND_FSL_SOC
> diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile
> index e5acc03..26fc551 100644
> --- a/sound/soc/fsl/Makefile
> +++ b/sound/soc/fsl/Makefile
> @@ -59,5 +59,7 @@ obj-$(CONFIG_SND_SOC_IMX_MC13783) += snd-soc-imx-mc13783.o
>  
>  # FSL ARM SAI/SGT15000 Platform Support
>  snd-soc-fsl-sai-objs := fsl-sai.o
> +snd-soc-fsl-sgtl5000-vf610-objs := fsl-sgtl5000-vf610.o
>  
>  obj-$(CONFIG_SND_SOC_FSL_SAI) += snd-soc-fsl-sai.o
> +obj-$(CONFIG_SND_SOC_FSL_SGTL5000_VF610) += snd-soc-fsl-sgtl5000-vf610.o
> diff --git a/sound/soc/fsl/fsl-sgtl5000-vf610.c b/sound/soc/fsl/fsl-sgtl5000-vf610.c
> new file mode 100644
> index 0000000..f535b42
> --- /dev/null
> +++ b/sound/soc/fsl/fsl-sgtl5000-vf610.c
> @@ -0,0 +1,208 @@
> +/*
> + * Freeacale ALSA SoC Audio using SGT1500 as codec.
> + *
> + * Copyright 2012-2013 Freescale Semiconductor, Inc.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/i2c.h>
> +#include <linux/clk.h>
> +
> +#include "../codecs/sgtl5000.h"
> +#include "fsl-sai.h"
> +
> +static unsigned int sysclk_rate;
> +
> +static int fsl_sgtl5000_dai_init(struct snd_soc_pcm_runtime *rtd)

Naming issue here again.

At least from my point of view, if you actually merged imx-sgtl5000 with
vf610-sgtl5000 and made it also compatible to other freescale SoCs, you
could then fairly call it fsl_sgtl5000_xxxx.

Well, I might be a little picky here because it's a static function and
won't conflict others. Just the name here doesn't look so explicit to me.

Please reconsider about this whole file's naming.

Best regards,
Nicolin Chen

> +{
> +	int ret;
> +	struct device *dev = rtd->card->dev;
> +
> +	ret = snd_soc_dai_set_sysclk(rtd->codec_dai, SGTL5000_SYSCLK,
> +				     sysclk_rate, SND_SOC_CLOCK_IN);
> +	if (ret) {
> +		dev_err(dev, "could not set codec driver clock params :%d\n",
> +				ret);
> +		return ret;
> +	}
> +
> +	ret = snd_soc_dai_set_sysclk(rtd->cpu_dai, FSL_SAI_CLK_BUS,
> +				     sysclk_rate, SND_SOC_CLOCK_OUT);
> +	if (ret) {
> +		dev_err(dev, "could not set cpu dai driver clock params :%d\n",
> +				ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sgtl5000_params(struct snd_pcm_substream *substream,
> +		struct snd_pcm_hw_params *params)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +	unsigned int channels = params_channels(params);
> +
> +	/* TODO: The SAI driver should figure this out for us */
> +	switch (channels) {
> +	case 2:
> +		snd_soc_dai_set_tdm_slot(cpu_dai, 0xfffffffc, 0xfffffffc, 2, 0);
> +		break;
> +	case 1:
> +		snd_soc_dai_set_tdm_slot(cpu_dai, 0xfffffffe, 0xfffffffe, 1, 0);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct snd_soc_ops fsl_sgtl5000_hifi_ops = {
> +	.hw_params = sgtl5000_params,
> +};
> +
> +static struct snd_soc_dai_link fsl_sgtl5000_dai = {
> +	.name = "HiFi",
> +	.stream_name = "HiFi",
> +	.codec_dai_name = "sgtl5000",
> +	.init = &fsl_sgtl5000_dai_init,
> +	.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
> +		SND_SOC_DAIFMT_CBM_CFM,
> +	.ops = &fsl_sgtl5000_hifi_ops,
> +};
> +
> +static const struct snd_soc_dapm_widget fsl_sgtl5000_dapm_widgets[] = {
> +	SND_SOC_DAPM_MIC("Mic Jack", NULL),
> +	SND_SOC_DAPM_LINE("Line In Jack", NULL),
> +	SND_SOC_DAPM_HP("Headphone Jack", NULL),
> +	SND_SOC_DAPM_SPK("Line Out Jack", NULL),
> +	SND_SOC_DAPM_SPK("Ext Spk", NULL),
> +};
> +
> +static struct snd_soc_card fsl_sgt1500_card = {
> +	.owner = THIS_MODULE,
> +	.num_links = 1,
> +	.dai_link = &fsl_sgtl5000_dai,
> +	.dapm_widgets = fsl_sgtl5000_dapm_widgets,
> +	.num_dapm_widgets = ARRAY_SIZE(fsl_sgtl5000_dapm_widgets),
> +};
> +
> +static int fsl_sgtl5000_parse_dt(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device_node *sai_np, *codec_np;
> +	struct clk *codec_clk;
> +	struct i2c_client *codec_dev;
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	ret = snd_soc_of_parse_card_name(&fsl_sgt1500_card, "model");
> +	if (ret)
> +		return ret;
> +
> +	ret = snd_soc_of_parse_audio_routing(&fsl_sgt1500_card,
> +			"audio-routing");
> +	if (ret)
> +		return ret;
> +
> +	sai_np = of_parse_phandle(np, "saif-controller", 0);
> +	if (!sai_np) {
> +		dev_err(&pdev->dev, "\"saif-controller\" phandle missing or "
> +				"invalid\n");
> +		return -EINVAL;
> +	}
> +	fsl_sgtl5000_dai.cpu_of_node = sai_np;
> +	fsl_sgtl5000_dai.platform_of_node = sai_np;
> +
> +	codec_np = of_parse_phandle(np, "audio-codec", 0);
> +	if (!codec_np) {
> +		dev_err(&pdev->dev, "\"audio-codec\" phandle missing or "
> +				"invalid\n");
> +		ret = -EINVAL;
> +		goto sai_np_fail;
> +	}
> +	fsl_sgtl5000_dai.codec_of_node = codec_np;
> +
> +	codec_dev = of_find_i2c_device_by_node(codec_np);
> +	if (!codec_dev) {
> +		dev_err(&pdev->dev, "failed to find codec platform device\n");
> +		ret = PTR_ERR(codec_dev);
> +		goto codec_np_fail;
> +	}
> +
> +	codec_clk = devm_clk_get(&codec_dev->dev, NULL);
> +	if (IS_ERR(codec_clk)) {
> +		dev_err(&pdev->dev, "failed to get codec clock\n");
> +		ret = PTR_ERR(codec_clk);
> +		goto codec_np_fail;
> +	}
> +
> +	sysclk_rate = clk_get_rate(codec_clk);
> +
> +codec_np_fail:
> +	of_node_put(codec_np);
> +sai_np_fail:
> +	of_node_put(sai_np);
> +
> +	return ret;
> +}
> +
> +static int fsl_sgtl5000_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	fsl_sgt1500_card.dev = &pdev->dev;
> +
> +	ret = fsl_sgtl5000_parse_dt(pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +				"parse sgtl5000 device tree failed :%d\n",
> +				ret);
> +		return ret;
> +	}
> +
> +	ret = devm_snd_soc_register_card(&pdev->dev, &fsl_sgt1500_card);
> +	if (ret) {
> +		dev_err(&pdev->dev, "register soc sound card failed :%d\n",
> +				ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fsl_sgtl5000_remove(struct platform_device *pdev)
> +{
> +	snd_soc_unregister_card(&fsl_sgt1500_card);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id fsl_sgtl5000_dt_ids[] = {
> +	{ .compatible = "fsl,vf610-sgtl5000", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, fsl_sgtl5000_dt_ids);
> +
> +static struct platform_driver fsl_sgtl5000_driver = {
> +	.driver = {
> +		.name = "fsl-sgtl5000",
> +		.owner = THIS_MODULE,
> +		.of_match_table = fsl_sgtl5000_dt_ids,
> +	},
> +	.probe = fsl_sgtl5000_probe,
> +	.remove = fsl_sgtl5000_remove,
> +};
> +module_platform_driver(fsl_sgtl5000_driver);
> +
> +MODULE_AUTHOR("Xiubo Li <Li.Xiubo@freescale.com>");
> +MODULE_DESCRIPTION("Freescale SGTL5000 ASoC driver");
> +MODULE_LICENSE("GPL");
> -- 
> 1.8.4
> 

[PATCHv2 6/8] ASoC: fsl: add SGTL5000 based audio machine driver.

[Shawn Guo]

On Fri, Nov 01, 2013 at 06:28:05PM +0800, Nicolin Chen wrote:
> >  sound/soc/fsl/fsl-sgtl5000-vf610.c | 208 +++++++++++++++++++++++++++++++++++++
> 
> I just doubt if this file naming is appropriate. Even if we might not have
> rigor rule for the file names, according to existing ones, they are all in
> a same pattern: [SoC name]-[codec name].c
> 
> "imx-sgtl5000.c" for example
> 
> I think it would make user less confused about what this file exactly is if
> this machine driver also follow the pattern: vf610-sgtl5000.c
> 
> 
> @Shawn
> 
> What do you think about the file name?

Yeah, it would be better to name the file following the existing the
pattern.

Shawn

[PATCHv2 6/8] ASoC: fsl: add SGTL5000 based audio machine driver.

[Li Xiubo]

> > >  sound/soc/fsl/fsl-sgtl5000-vf610.c | 208
> > > +++++++++++++++++++++++++++++++++++++
> >
> > I just doubt if this file naming is appropriate. Even if we might not
> > have rigor rule for the file names, according to existing ones, they
> > are all in a same pattern: [SoC name]-[codec name].c
> >
> > "imx-sgtl5000.c" for example
> >
> > I think it would make user less confused about what this file exactly
> > is if this machine driver also follow the pattern: vf610-sgtl5000.c
> >
> >
> > @Shawn
> >
> > What do you think about the file name?
> 
> Yeah, it would be better to name the file following the existing the
> pattern.
> 

Please see the next version.

Best Regards,
Xiubo

[PATCHv2 6/8] ASoC: fsl: add SGTL5000 based audio machine driver.

[Li Xiubo]

Hi Nicolin,


> > This is the SGTL5000 codec based audio driver supported with both
> > playback and capture dai link implemention.
> >
> > This implementation is only compatible with device tree definition.
> >
> > Signed-off-by: Alison Wang <b18965@freescale.com
> > Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> >
> > Conflicts:
> > 	sound/soc/fsl/Makefile
> > ---
> >  sound/soc/fsl/Kconfig              |  10 ++
> >  sound/soc/fsl/Makefile             |   2 +
> 
> >  sound/soc/fsl/fsl-sgtl5000-vf610.c | 208
> > +++++++++++++++++++++++++++++++++++++
> 
> I just doubt if this file naming is appropriate. Even if we might not
> have rigor rule for the file names, according to existing ones, they are
> all in a same pattern: [SoC name]-[codec name].c
> 
> "imx-sgtl5000.c" for example
> 
> I think it would make user less confused about what this file exactly is
> if this machine driver also follow the pattern: vf610-sgtl5000.c
> 

Yes, it looks nicer.

> 
> @Shawn
> 
> What do you think about the file name?
> 
> >  3 files changed, 220 insertions(+)
> >  create mode 100644 sound/soc/fsl/fsl-sgtl5000-vf610.c
> >
> > diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig index
> > 9a8851e..1b835ba 100644
> > --- a/sound/soc/fsl/Kconfig
> > +++ b/sound/soc/fsl/Kconfig
> > @@ -228,4 +228,14 @@ config SND_SOC_FSL_SAI
> >  	tristate
> >  	select SND_SOC_GENERIC_DMAENGINE_PCM
> >
> > +config SND_SOC_FSL_SGTL5000_VF610
> 
> Same problem with the this define.
> 
> > +	tristate "SoC Audio support for FSL boards with sgtl5000"
> 
> And 'FSL' here confuses me a lot. Because those boards based on i.MX
> series also could be called FSL boards.
> 

Yes, this should be VF610.

> > +	depends on OF && I2C
> > +	select SND_SOC_FSL_SAI
> > +	select SND_SOC_FSL_PCM
> > +	select SND_SOC_SGTL5000
> > +	help
> > +	  Say Y if you want to add support for SoC audio on an FSL board
> with
> > +	  a sgtl5000 codec.
> > +
> >  endif # SND_FSL_SOC
> > diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile index
> > e5acc03..26fc551 100644
> > --- a/sound/soc/fsl/Makefile
> > +++ b/sound/soc/fsl/Makefile
> > @@ -59,5 +59,7 @@ obj-$(CONFIG_SND_SOC_IMX_MC13783) +=
> > snd-soc-imx-mc13783.o
> >
> >  # FSL ARM SAI/SGT15000 Platform Support  snd-soc-fsl-sai-objs :=
> > fsl-sai.o
> > +snd-soc-fsl-sgtl5000-vf610-objs := fsl-sgtl5000-vf610.o
> >
> >  obj-$(CONFIG_SND_SOC_FSL_SAI) += snd-soc-fsl-sai.o
> > +obj-$(CONFIG_SND_SOC_FSL_SGTL5000_VF610) +=
> > +snd-soc-fsl-sgtl5000-vf610.o
> > diff --git a/sound/soc/fsl/fsl-sgtl5000-vf610.c
> > b/sound/soc/fsl/fsl-sgtl5000-vf610.c
> > new file mode 100644
> > index 0000000..f535b42
> > --- /dev/null
> > +++ b/sound/soc/fsl/fsl-sgtl5000-vf610.c
> > @@ -0,0 +1,208 @@
> > +/*
> > + * Freeacale ALSA SoC Audio using SGT1500 as codec.
> > + *
> > + * Copyright 2012-2013 Freescale Semiconductor, Inc.
> > + *
> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/i2c.h>
> > +#include <linux/clk.h>
> > +
> > +#include "../codecs/sgtl5000.h"
> > +#include "fsl-sai.h"
> > +
> > +static unsigned int sysclk_rate;
> > +
> > +static int fsl_sgtl5000_dai_init(struct snd_soc_pcm_runtime *rtd)
> 
> Naming issue here again.
> 
> At least from my point of view, if you actually merged imx-sgtl5000 with
> vf610-sgtl5000 and made it also compatible to other freescale SoCs, you
> could then fairly call it fsl_sgtl5000_xxxx.
> 
> Well, I might be a little picky here because it's a static function and
> won't conflict others. Just the name here doesn't look so explicit to me.
> 
> Please reconsider about this whole file's naming.
> 

Yes, I still not very sure the names of the functions and files, from your replies,
I have got many information about the rules and others, I'll think it over and do some
research, Please see the next version.



Best regards,
Xiubo

[PATCHv2 6/8] ASoC: fsl: add SGTL5000 based audio machine driver.

[Mark Brown]

On Fri, Nov 01, 2013 at 03:04:53PM +0800, Xiubo Li wrote:

> Conflicts:
> 	sound/soc/fsl/Makefile

Ahem.

> +	/* TODO: The SAI driver should figure this out for us */
> +	switch (channels) {
> +	case 2:
> +		snd_soc_dai_set_tdm_slot(cpu_dai, 0xfffffffc, 0xfffffffc, 2, 0);
> +		break;
> +	case 1:
> +		snd_soc_dai_set_tdm_slot(cpu_dai, 0xfffffffe, 0xfffffffe, 1, 0);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

Yes, it should - this code should probably just be copied straight into
the SAI driver.  If we need to support other configurations we can do
that later.

> +static int fsl_sgtl5000_remove(struct platform_device *pdev)
> +{
> +	snd_soc_unregister_card(&fsl_sgt1500_card);
> +
> +	return 0;
> +}

You're using snd_soc_unregister_card() so you don't need to do this.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131101/9f61e6ef/attachment.sig>

[PATCHv2 6/8] ASoC: fsl: add SGTL5000 based audio machine driver.

[Li Xiubo]

> > Conflicts:
> > 	sound/soc/fsl/Makefile
> 
> Ahem.
> 

This will be removed.

> > +static int fsl_sgtl5000_remove(struct platform_device *pdev) {
> > +	snd_soc_unregister_card(&fsl_sgt1500_card);
> > +
> > +	return 0;
> > +}
> 
> You're using snd_soc_unregister_card() so you don't need to do this.
>

See the next version.

[PATCHv2 6/8] ASoC: fsl: add SGTL5000 based audio machine driver.

[Li Xiubo]

> > +	/* TODO: The SAI driver should figure this out for us */
> > +	switch (channels) {
> > +	case 2:
> > +		snd_soc_dai_set_tdm_slot(cpu_dai, 0xfffffffc, 0xfffffffc, 2,
> 0);
> > +		break;
> > +	case 1:
> > +		snd_soc_dai_set_tdm_slot(cpu_dai, 0xfffffffe, 0xfffffffe, 1,
> 0);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> 
> Yes, it should - this code should probably just be copied straight into
> the SAI driver.  If we need to support other configurations we can do
> that later.
> 

Well, yes.
I have considered carefully about this, It maybe nicer to move this to SAI driver's .hw_params.



--
Best Regards,
Xiubo

[PATCHv2 7/8] ARM: dts: Enable SGTL5000 codec based audio driver node for VF610.

[Xiubo Li]

This patch add and enable SGTL5000 codec support, and also specified
the corresponding SAI node.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
Signed-off-by: Alison Wang <b18965@freescale.com
---
 arch/arm/boot/dts/vf610-twr.dts | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-twr.dts
index e4106dd..a2d9214 100644
--- a/arch/arm/boot/dts/vf610-twr.dts
+++ b/arch/arm/boot/dts/vf610-twr.dts
@@ -34,6 +34,19 @@
 		};
 	};
 
+	sound {
+		compatible = "fsl,vf610-sgtl5000";
+		model = "vf610-sgtl5000";
+		saif-controller = <&sai2>;
+		audio-codec = <&codec>;
+		audio-routing =
+			"MIC_IN", "Mic Jack",
+			"Mic Jack", "Mic Bias",
+			"LINE_IN", "Line In Jack",
+			"Headphone Jack", "HP_OUT",
+			"Ext Spk", "LINE_OUT";
+	};
+
 };
 
 &fec0 {
@@ -55,6 +68,12 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_i2c0_1>;
 	status = "okay";
+
+	codec: sgtl5000 at 0a {
+		compatible = "fsl,sgtl5000";
+		reg = <0x0a>;
+		clocks = <&clks VF610_CLK_SAI2>;
+	};
 };
 
 &sai2 {
-- 
1.8.4

[PATCHv2 8/8] Documentation: Add device tree bindings for Freescale VF610 sound.

[Xiubo Li]

This adds the Document for Freescale VF610 sound driver under
Documentation/devicetree/bindings/sound/.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 .../bindings/sound/fsl_audio_sgt15000_vf610.txt    | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/fsl_audio_sgt15000_vf610.txt

diff --git a/Documentation/devicetree/bindings/sound/fsl_audio_sgt15000_vf610.txt b/Documentation/devicetree/bindings/sound/fsl_audio_sgt15000_vf610.txt
new file mode 100644
index 0000000..76ff838
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/fsl_audio_sgt15000_vf610.txt
@@ -0,0 +1,46 @@
+Freescale VF610 audio complex with SGTL5000 codec
+
+Required properties:
+- compatible: "fsl,vf610-sgtl5000"
+- model: The user-visible name of this sound complex.
+- saif-controllers: The phandle list of the SAI controller.
+- audio-codec: The phandle of the SGTL5000 audio codec.
+- audio-routing : A list of the connections between audio components.
+  Each entry is a pair of strings, the first being the connection's sink,
+  the second being the connection's source. Valid names could be power
+  supplies, SGTL5000 pins, and the jacks on the board:
+
+  -- Power supplies:
+     * Mic Bias
+
+  -- Board connectors:
+     * Mic Jack
+     * Line In Jack
+     * Headphone Jack
+     * Line Out Jack
+     * Ext Spk
+
+Example:
+
+sound {
+	compatible = "fsl,vf610-sgtl5000";
+	model = "vf610-sgtl5000";
+	saif-controller = <&sai2>;
+	audio-codec = <&codec>;
+	audio-routing =
+		"MIC_IN", "Mic Jack",
+		"Mic Jack", "Mic Bias",
+		"LINE_IN", "Line In Jack",
+		"Headphone Jack", "HP_OUT",
+		"Ext Spk", "LINE_OUT";
+};
+
+&i2c0 {
+	...
+
+	codec: sgtl5000 at 0a {
+	       compatible = "fsl,sgtl5000";
+	       reg = <0x0a>;
+	       clocks = <&clks VF610_CLK_SAI2>;
+       };
+};
-- 
1.8.4

[PATCHv1 0/8] ALSA: Add SAI driver and enable SGT15000 codec

[Li Xiubo]

Sorry, forgot the Subject!

Sent again.


> 
> Hello,
> 
> This patch series is mostly Freescale's SAI SoC Digital Audio Interface
> driver implementation. And the implementation is only compatible with
> device tree definition.
> 
> This patch series is based on linux-next and has been tested on Vybrid
> VF610 Tower board using device tree.
> 
> Changed in v2:
> - Use default settings for the generic dmaengine PCM driver.
> - Separate receive and transmit setting in most functions, but some
> couldn't for the HW limitation.
> - Drop some not reduntant code.
> - Use devm_snd_soc_register_component() instead of
> snd_soc_register_component().
> - Use devm_snd_soc_register_card() instead of
> devm_snd_soc_register_card().
> - Adjust the code sentences sequence.
> - Make the namespacing consistent.
> - Rename CONFIG_SND_SOC_FSL_SGTL5000 to CONFIG_SND_SOC_FSL_SGTL5000_VF610.
> - Drop some meaningless lines.
> - Rename the binding document file.
> 
> Added in v1:
> - Add SAI SoC Digital Audio Interface driver.
> - Add Freescale SAI ALSA SoC Digital Audio Interface node for VF610.
> - Enables SAI ALSA SoC DAI device for Vybrid VF610 TOWER board.
> - Add device tree bindings for Freescale SAI.
> - Revise the bugs about the sgt15000 codec.
> - Add SGT15000 based audio machine driver.
> - Enable SGT15000 codec based audio driver node for VF610.
> - Add device tree bindings for Freescale VF610 sound.
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html