Convert to driver to use either platform_data or device-tree for configuration
of the device. When using device-tree, the I2S block's configuration is read
from the relevant registers: this reduces the amount of information required in
the device tree.
Signed-off-by: Andrew Jackson <[email protected]>
---
.../devicetree/bindings/sound/designware-i2s.txt | 32 +++
sound/soc/dwc/Kconfig | 1 +
sound/soc/dwc/designware_i2s.c | 238 ++++++++++++++++----
3 files changed, 222 insertions(+), 49 deletions(-)
create mode 100644 Documentation/devicetree/bindings/sound/designware-i2s.txt
diff --git a/Documentation/devicetree/bindings/sound/designware-i2s.txt b/Documentation/devicetree/bindings/sound/designware-i2s.txt
new file mode 100644
index 0000000..cdee591
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/designware-i2s.txt
@@ -0,0 +1,32 @@
+DesignWare I2S controller
+
+Required properties:
+ - compatible : Must be "snps,designware-i2s"
+ - reg : Must contain I2S core's registers location and length
+ - clocks : Pairs of phandle and specifier referencing the controller's clocks.
+ The controller expects two clocks, the clock used for the APB interface and
+ the clock used as the sampling rate reference clock sample.
+ - clock-names : "apb_plck" for the clock to the APB interface, "i2sclk" for the sample
+ rate reference clock.
+ - dmas: Pairs of phandle and specifier for the DMA channels that are used by
+ the core. The core expects one or two dma channels, one for transmit and one for
+ receive.
+ - dma-names : "tx" for the transmit channel, "rx" for the receive channel.
+
+For more details on the 'dma', 'dma-names', 'clock' and 'clock-names' properties
+please check:
+ * resource-names.txt
+ * clock/clock-bindings.txt
+ * dma/dma.txt
+
+Example:
+
+ soc_i2s: i2s@7ff90000 {
+ compatible = "snps,designware-i2s";
+ reg = <0x0 0x7ff90000 0x0 0x1000>;
+ clocks = <&scpi_i2sclk 0>, <&soc_refclk100mhz>;
+ clock-names = "i2sclk", "apb_pclk";
+ #sound-dai-cells = <0>;
+ dmas = <&dma0 5>;
+ dma-names = "tx";
+ };
diff --git a/sound/soc/dwc/Kconfig b/sound/soc/dwc/Kconfig
index e334900..d50e085 100644
--- a/sound/soc/dwc/Kconfig
+++ b/sound/soc/dwc/Kconfig
@@ -1,6 +1,7 @@
config SND_DESIGNWARE_I2S
tristate "Synopsys I2S Device Driver"
depends on CLKDEV_LOOKUP
+ select SND_SOC_GENERIC_DMAENGINE_PCM
help
Say Y or M if you want to add support for I2S driver for
Synopsys desigwnware I2S device. The device supports upto
diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c
index c497ada..083779d 100644
--- a/sound/soc/dwc/designware_i2s.c
+++ b/sound/soc/dwc/designware_i2s.c
@@ -22,6 +22,7 @@
#include <sound/pcm.h>
#include <sound/pcm_params.h>
#include <sound/soc.h>
+#include <sound/dmaengine_pcm.h>
/* common register for all channel */
#define IER 0x000
@@ -54,19 +55,46 @@
#define I2S_COMP_VERSION 0x01F8
#define I2S_COMP_TYPE 0x01FC
+/*
+ * Component parameter register fields - define the I2S block's
+ * configuration.
+ */
+#define COMP1_TX_WORDSIZE_3(r) (((r) & GENMASK(27, 25)) >> 25)
+#define COMP1_TX_WORDSIZE_2(r) (((r) & GENMASK(24, 22)) >> 22)
+#define COMP1_TX_WORDSIZE_1(r) (((r) & GENMASK(21, 19)) >> 19)
+#define COMP1_TX_WORDSIZE_0(r) (((r) & GENMASK(18, 16)) >> 16)
+#define COMP1_TX_CHANNELS(r) (((r) & GENMASK(10, 9)) >> 9)
+#define COMP1_RX_CHANNELS(r) (((r) & GENMASK(8, 7)) >> 7)
+#define COMP1_RX_ENABLED(r) (((r) & BIT(6)) >> 6)
+#define COMP1_TX_ENABLED(r) (((r) & BIT(5)) >> 5)
+#define COMP1_MODE_EN(r) (((r) & BIT(4)) >> 4)
+#define COMP1_FIFO_DEPTH_GLOBAL(r) (((r) & GENMASK(3, 2)) >> 2)
+#define COMP1_APB_DATA_WIDTH(r) (((r) & GENMASK(1, 0)) >> 0)
+
+#define COMP2_RX_WORDSIZE_3(r) (((r) & GENMASK(12, 10)) >> 10)
+#define COMP2_RX_WORDSIZE_2(r) (((r) & GENMASK(9, 7)) >> 7)
+#define COMP2_RX_WORDSIZE_1(r) (((r) & GENMASK(5, 3)) >> 3)
+#define COMP2_RX_WORDSIZE_0(r) (((r) & GENMASK(2, 0)) >> 0)
+
#define MAX_CHANNEL_NUM 8
#define MIN_CHANNEL_NUM 2
+union snd_dma_data {
+ struct i2s_dma_data pd;
+ struct snd_dmaengine_dai_dma_data dt;
+};
+
struct dw_i2s_dev {
void __iomem *i2s_base;
struct clk *clk;
int active;
unsigned int capability;
struct device *dev;
+ bool using_pd;
/* data related to DMA transfers b/w i2s and DMAC */
- struct i2s_dma_data play_dma_data;
- struct i2s_dma_data capture_dma_data;
+ union snd_dma_data play_dma_data;
+ union snd_dma_data capture_dma_data;
struct i2s_clk_config_data config;
int (*i2s_clk_cfg)(struct i2s_clk_config_data *config);
};
@@ -153,7 +181,7 @@ static int dw_i2s_startup(struct snd_pcm_substream *substream,
struct snd_soc_dai *cpu_dai)
{
struct dw_i2s_dev *dev = snd_soc_dai_get_drvdata(cpu_dai);
- struct i2s_dma_data *dma_data = NULL;
+ union snd_dma_data *dma_data = NULL;
if (!(dev->capability & DWC_I2S_RECORD) &&
(substream->stream == SNDRV_PCM_STREAM_CAPTURE))
@@ -227,8 +255,7 @@ static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
i2s_disable_channels(dev, substream->stream);
- /* Iterate over set of channels - independently controlled.
- */
+ /* Iterate over set of channels - independently controlled. */
do {
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
i2s_write_reg(dev->i2s_base, TCR(ch_reg),
@@ -251,13 +278,18 @@ static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
config->sample_rate = params_rate(params);
- if (!dev->i2s_clk_cfg)
- return -EINVAL;
+ if (dev->using_pd) {
+ ret = dev->i2s_clk_cfg(config);
+ if (ret < 0) {
+ dev_err(dev->dev, "runtime audio clk config fail\n");
+ return ret;
+ }
+ } else {
+ u32 bitclk;
- ret = dev->i2s_clk_cfg(config);
- if (ret < 0) {
- dev_err(dev->dev, "runtime audio clk config fail\n");
- return ret;
+ /* TODO: Validate sample rate against permissible set */
+ bitclk = config->sample_rate * config->data_width * 2;
+ clk_set_rate(dev->clk, bitclk);
}
return 0;
@@ -331,6 +363,31 @@ static int dw_i2s_resume(struct snd_soc_dai *dai)
#define dw_i2s_resume NULL
#endif
+/* Maximum resolution of a channel - not uniformly spaced */
+static const u32 fifo_width[] = {
+ 12, 16, 20, 24, 32, 0, 0, 0
+};
+
+/* Width of (DMA) bus */
+static const u32 bus_widths[] = {
+ DMA_SLAVE_BUSWIDTH_1_BYTE,
+ DMA_SLAVE_BUSWIDTH_2_BYTES,
+ DMA_SLAVE_BUSWIDTH_4_BYTES,
+ DMA_SLAVE_BUSWIDTH_UNDEFINED
+};
+
+/* PCM format to support channel resolution */
+static const u32 formats[] = {
+ SNDRV_PCM_FMTBIT_S16_LE,
+ SNDRV_PCM_FMTBIT_S16_LE,
+ SNDRV_PCM_FMTBIT_S24_LE,
+ SNDRV_PCM_FMTBIT_S24_LE,
+ SNDRV_PCM_FMTBIT_S32_LE,
+ 0,
+ 0,
+ 0
+};
+
static int dw_i2s_probe(struct platform_device *pdev)
{
const struct i2s_platform_data *pdata = pdev->dev.platform_data;
@@ -340,11 +397,6 @@ static int dw_i2s_probe(struct platform_device *pdev)
unsigned int cap;
struct snd_soc_dai_driver *dw_i2s_dai;
- if (!pdata) {
- dev_err(&pdev->dev, "Invalid platform data\n");
- return -EINVAL;
- }
-
dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
if (!dev) {
dev_warn(&pdev->dev, "kzalloc fail\n");
@@ -373,47 +425,114 @@ static int dw_i2s_probe(struct platform_device *pdev)
return PTR_ERR(dev->i2s_base);
}
- cap = pdata->cap;
- dev->capability = cap;
- dev->i2s_clk_cfg = pdata->i2s_clk_cfg;
+ if (pdata) {
+ dev->using_pd = true;
+ cap = pdata->cap;
+ dev->capability = cap;
+ dev->i2s_clk_cfg = pdata->i2s_clk_cfg;
+ if (!dev->i2s_clk_cfg) {
+ dev_err(&pdev->dev, "no clock device\n");
+ return -ENODEV;
+ }
- /* Set DMA slaves info */
+ /* Set DMA slaves info */
+
+ dev->play_dma_data.pd.data = pdata->play_dma_data;
+ dev->capture_dma_data.pd.data = pdata->capture_dma_data;
+ dev->play_dma_data.pd.addr = res->start + I2S_TXDMA;
+ dev->capture_dma_data.pd.addr = res->start + I2S_RXDMA;
+ dev->play_dma_data.pd.max_burst = 16;
+ dev->capture_dma_data.pd.max_burst = 16;
+ dev->play_dma_data.pd.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+ dev->capture_dma_data.pd.addr_width =
+ DMA_SLAVE_BUSWIDTH_2_BYTES;
+ dev->play_dma_data.pd.filter = pdata->filter;
+ dev->capture_dma_data.pd.filter = pdata->filter;
+
+ dev->clk = clk_get(&pdev->dev, NULL);
+ if (IS_ERR(dev->clk))
+ return PTR_ERR(dev->clk);
+
+ if (cap & DWC_I2S_PLAY) {
+ dev_dbg(&pdev->dev, " designware: play supported\n");
+ dw_i2s_dai->playback.channels_min = MIN_CHANNEL_NUM;
+ dw_i2s_dai->playback.channels_max = pdata->channel;
+ dw_i2s_dai->playback.formats = pdata->snd_fmts;
+ dw_i2s_dai->playback.rates = pdata->snd_rates;
+ }
- dev->play_dma_data.data = pdata->play_dma_data;
- dev->capture_dma_data.data = pdata->capture_dma_data;
- dev->play_dma_data.addr = res->start + I2S_TXDMA;
- dev->capture_dma_data.addr = res->start + I2S_RXDMA;
- dev->play_dma_data.max_burst = 16;
- dev->capture_dma_data.max_burst = 16;
- dev->play_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
- dev->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
- dev->play_dma_data.filter = pdata->filter;
- dev->capture_dma_data.filter = pdata->filter;
+ if (cap & DWC_I2S_RECORD) {
+ dev_dbg(&pdev->dev, "designware: record supported\n");
+ dw_i2s_dai->capture.channels_min = MIN_CHANNEL_NUM;
+ dw_i2s_dai->capture.channels_max = pdata->channel;
+ dw_i2s_dai->capture.formats = pdata->snd_fmts;
+ dw_i2s_dai->capture.rates = pdata->snd_rates;
+ }
+ } else {
+ /*
+ * Read component parameter registers to extract
+ * the I2S block's configuration.
+ */
+ u32 comp1 = i2s_read_reg(dev->i2s_base, I2S_COMP_PARAM_1);
+ u32 comp2 = i2s_read_reg(dev->i2s_base, I2S_COMP_PARAM_2);
+ u32 bus_width = bus_widths[COMP1_APB_DATA_WIDTH(comp1)];
+ u32 fifo_depth = 1 << (1 + COMP1_FIFO_DEPTH_GLOBAL(comp1));
+ u32 max_size;
+
+ dev->using_pd = false;
+
+ dev->clk = devm_clk_get(&pdev->dev, "i2sclk");
+ if (IS_ERR(dev->clk))
+ return PTR_ERR(dev->clk);
+
+ /*
+ * Code presumes all channels are configured with the same
+ * word size.
+ */
+ if (COMP1_TX_ENABLED(comp1)) {
+ dev_dbg(&pdev->dev, "playback capable\n");
+
+ dev->capability |= DWC_I2S_PLAY;
+ max_size = COMP1_TX_WORDSIZE_0(comp1);
+ dev->play_dma_data.dt.addr = res->start + I2S_TXDMA;
+ dev->play_dma_data.dt.addr_width = bus_width;
+ dev->play_dma_data.dt.chan_name = "TX";
+ dev->play_dma_data.dt.fifo_size = fifo_depth *
+ (fifo_width[max_size]) >> 8;
+ dev->play_dma_data.dt.maxburst = 16;
+ dw_i2s_dai->playback.channels_min = MIN_CHANNEL_NUM;
+ dw_i2s_dai->playback.channels_max =
+ 1 << (COMP1_TX_CHANNELS(comp1) + 1);
+ dw_i2s_dai->playback.formats = formats[max_size];
+ dw_i2s_dai->playback.rates = SNDRV_PCM_RATE_8000_192000;
+ }
+ if (COMP1_RX_ENABLED(comp1)) {
+ dev_dbg(&pdev->dev, "record capable\n");
+
+ dev->capability |= DWC_I2S_RECORD;
+ max_size = COMP2_RX_WORDSIZE_0(comp2);
+ dev->capture_dma_data.dt.addr = res->start + I2S_RXDMA;
+ dev->capture_dma_data.dt.addr_width = bus_width;
+ dev->capture_dma_data.dt.chan_name = "RX";
+ dev->capture_dma_data.dt.fifo_size = fifo_depth *
+ (fifo_width[max_size] >> 8);
+ dev->capture_dma_data.dt.maxburst = 16;
+ dw_i2s_dai->capture.channels_min = MIN_CHANNEL_NUM;
+ dw_i2s_dai->capture.channels_max =
+ 1 << (COMP1_RX_CHANNELS(comp1) + 1);
+ dw_i2s_dai->capture.formats = formats[max_size];
+ dw_i2s_dai->capture.rates = SNDRV_PCM_RATE_8000_192000;
+ }
+ }
- dev->clk = clk_get(&pdev->dev, NULL);
- if (IS_ERR(dev->clk))
- return PTR_ERR(dev->clk);
+ ret = clk_prepare(dev->clk);
+ if (ret < 0)
+ goto err_clk_put;
ret = clk_enable(dev->clk);
if (ret < 0)
goto err_clk_put;
- if (cap & DWC_I2S_PLAY) {
- dev_dbg(&pdev->dev, " designware: play supported\n");
- dw_i2s_dai->playback.channels_min = MIN_CHANNEL_NUM;
- dw_i2s_dai->playback.channels_max = pdata->channel;
- dw_i2s_dai->playback.formats = pdata->snd_fmts;
- dw_i2s_dai->playback.rates = pdata->snd_rates;
- }
-
- if (cap & DWC_I2S_RECORD) {
- dev_dbg(&pdev->dev, "designware: record supported\n");
- dw_i2s_dai->capture.channels_min = MIN_CHANNEL_NUM;
- dw_i2s_dai->capture.channels_max = pdata->channel;
- dw_i2s_dai->capture.formats = pdata->snd_fmts;
- dw_i2s_dai->capture.rates = pdata->snd_rates;
- }
-
dev->dev = &pdev->dev;
dev_set_drvdata(&pdev->dev, dev);
ret = snd_soc_register_component(&pdev->dev, &dw_i2s_component,
@@ -423,6 +542,15 @@ static int dw_i2s_probe(struct platform_device *pdev)
goto err_clk_disable;
}
+ if (!dev->using_pd) {
+ ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Could not register PCM: %d\n", ret);
+ return ret;
+ }
+ }
+
return 0;
err_clk_disable:
@@ -443,12 +571,24 @@ static int dw_i2s_remove(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_OF
+static const struct of_device_id dw_i2s_of_match[] = {
+ { .compatible = "snps,designware-i2s", },
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, dw_i2s_of_match);
+#endif
+
static struct platform_driver dw_i2s_driver = {
.probe = dw_i2s_probe,
.remove = dw_i2s_remove,
.driver = {
.name = "designware-i2s",
.owner = THIS_MODULE,
+#ifdef CONFIG_OF
+ .of_match_table = dw_i2s_of_match,
+#endif
},
};
--
1.7.1
On Wed, Dec 03, 2014 at 04:39:08PM +0000, Andrew Jackson wrote:
> Convert to driver to use either platform_data or device-tree for configuration
> of the device. When using device-tree, the I2S block's configuration is read
> from the relevant registers: this reduces the amount of information required in
> the device tree.
This really needs to be split into two or more patches, there's a whole
bunch of refactoring to support this DT stuff which should be separate
from the DT addition itself. Right now it's hard to tell what each
individual bit of the code is supposed to be doing, the patch is far too
large and doing far too many individual things.
> + if (dev->using_pd) {
> + ret = dev->i2s_clk_cfg(config);
> + if (ret < 0) {
> + dev_err(dev->dev, "runtime audio clk config fail\n");
> + return ret;
> + }
> + } else {
> + u32 bitclk;
Having this whole separate path for using platform data feels icky, we
don't want to have completely separate flows like this. Checking for
the callbacks being there is probably fine but just having totally
separate code paths is a bit icky.
On Wednesday 03 December 2014 16:39:08 Andrew Jackson wrote:
> Convert to driver to use either platform_data or device-tree for configuration
> of the device. When using device-tree, the I2S block's configuration is read
> from the relevant registers: this reduces the amount of information required in
> the device tree.
>
> Signed-off-by: Andrew Jackson <[email protected]>
I don't think we even have to worry about the platform_data case here:
the only platform using this hardware in Linux is arm/mach-spear, and
it defines a device node with a binding that is similar to the one you
document here but that is not implemented in the driver.
So, I think for all practical purposes we can assume that nobody cares
if you make incompatible changes as long as you don't introduce build
regression.
Also, please adapt the arch/arm/boot/dts/spear13*.dts{,i} files
as good as you can. They are broken in other ways too that you don't
have to fix, just make them conform to the binding you add.
Arnd
On Wed, Dec 03, 2014 at 09:13:18PM +0100, Arnd Bergmann wrote:
> I don't think we even have to worry about the platform_data case here:
> the only platform using this hardware in Linux is arm/mach-spear, and
> it defines a device node with a binding that is similar to the one you
> document here but that is not implemented in the driver.
Not all the world uses DT, this is a DesignWare IP so it's likely to get
deployed widely - potentially on PCI cards and things like that.
On Wed, Dec 3, 2014 at 10:09 PM, Andrew Jackson <[email protected]> wrote:
> Convert to driver to use either platform_data or device-tree for configuration
> of the device. When using device-tree, the I2S block's configuration is read
> from the relevant registers: this reduces the amount of information required in
> the device tree.
>
> Signed-off-by: Andrew Jackson <[email protected]>
> ---
> .../devicetree/bindings/sound/designware-i2s.txt | 32 +++
> sound/soc/dwc/Kconfig | 1 +
> sound/soc/dwc/designware_i2s.c | 238 ++++++++++++++++----
> 3 files changed, 222 insertions(+), 49 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/sound/designware-i2s.txt
>
> diff --git a/Documentation/devicetree/bindings/sound/designware-i2s.txt b/Documentation/devicetree/bindings/sound/designware-i2s.txt
> new file mode 100644
> index 0000000..cdee591
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/designware-i2s.txt
> @@ -0,0 +1,32 @@
> +DesignWare I2S controller
> +
> +Required properties:
> + - compatible : Must be "snps,designware-i2s"
> + - reg : Must contain I2S core's registers location and length
> + - clocks : Pairs of phandle and specifier referencing the controller's clocks.
> + The controller expects two clocks, the clock used for the APB interface and
> + the clock used as the sampling rate reference clock sample.
> + - clock-names : "apb_plck" for the clock to the APB interface, "i2sclk" for the sample
> + rate reference clock.
> + - dmas: Pairs of phandle and specifier for the DMA channels that are used by
> + the core. The core expects one or two dma channels, one for transmit and one for
> + receive.
> + - dma-names : "tx" for the transmit channel, "rx" for the receive channel.
> +
> +For more details on the 'dma', 'dma-names', 'clock' and 'clock-names' properties
> +please check:
> + * resource-names.txt
> + * clock/clock-bindings.txt
> + * dma/dma.txt
> +
> +Example:
> +
> + soc_i2s: i2s@7ff90000 {
> + compatible = "snps,designware-i2s";
> + reg = <0x0 0x7ff90000 0x0 0x1000>;
> + clocks = <&scpi_i2sclk 0>, <&soc_refclk100mhz>;
> + clock-names = "i2sclk", "apb_pclk";
> + #sound-dai-cells = <0>;
> + dmas = <&dma0 5>;
> + dma-names = "tx";
> + };
> diff --git a/sound/soc/dwc/Kconfig b/sound/soc/dwc/Kconfig
> index e334900..d50e085 100644
> --- a/sound/soc/dwc/Kconfig
> +++ b/sound/soc/dwc/Kconfig
> @@ -1,6 +1,7 @@
> config SND_DESIGNWARE_I2S
> tristate "Synopsys I2S Device Driver"
> depends on CLKDEV_LOOKUP
> + select SND_SOC_GENERIC_DMAENGINE_PCM
> help
> Say Y or M if you want to add support for I2S driver for
> Synopsys desigwnware I2S device. The device supports upto
> diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c
> index c497ada..083779d 100644
> --- a/sound/soc/dwc/designware_i2s.c
> +++ b/sound/soc/dwc/designware_i2s.c
> @@ -22,6 +22,7 @@
> #include <sound/pcm.h>
> #include <sound/pcm_params.h>
> #include <sound/soc.h>
> +#include <sound/dmaengine_pcm.h>
>
> /* common register for all channel */
> #define IER 0x000
> @@ -54,19 +55,46 @@
> #define I2S_COMP_VERSION 0x01F8
> #define I2S_COMP_TYPE 0x01FC
>
> +/*
> + * Component parameter register fields - define the I2S block's
> + * configuration.
> + */
> +#define COMP1_TX_WORDSIZE_3(r) (((r) & GENMASK(27, 25)) >> 25)
> +#define COMP1_TX_WORDSIZE_2(r) (((r) & GENMASK(24, 22)) >> 22)
> +#define COMP1_TX_WORDSIZE_1(r) (((r) & GENMASK(21, 19)) >> 19)
> +#define COMP1_TX_WORDSIZE_0(r) (((r) & GENMASK(18, 16)) >> 16)
> +#define COMP1_TX_CHANNELS(r) (((r) & GENMASK(10, 9)) >> 9)
> +#define COMP1_RX_CHANNELS(r) (((r) & GENMASK(8, 7)) >> 7)
> +#define COMP1_RX_ENABLED(r) (((r) & BIT(6)) >> 6)
> +#define COMP1_TX_ENABLED(r) (((r) & BIT(5)) >> 5)
> +#define COMP1_MODE_EN(r) (((r) & BIT(4)) >> 4)
> +#define COMP1_FIFO_DEPTH_GLOBAL(r) (((r) & GENMASK(3, 2)) >> 2)
> +#define COMP1_APB_DATA_WIDTH(r) (((r) & GENMASK(1, 0)) >> 0)
> +
> +#define COMP2_RX_WORDSIZE_3(r) (((r) & GENMASK(12, 10)) >> 10)
> +#define COMP2_RX_WORDSIZE_2(r) (((r) & GENMASK(9, 7)) >> 7)
> +#define COMP2_RX_WORDSIZE_1(r) (((r) & GENMASK(5, 3)) >> 3)
> +#define COMP2_RX_WORDSIZE_0(r) (((r) & GENMASK(2, 0)) >> 0)
> +
> #define MAX_CHANNEL_NUM 8
> #define MIN_CHANNEL_NUM 2
>
> +union snd_dma_data {
> + struct i2s_dma_data pd;
> + struct snd_dmaengine_dai_dma_data dt;
> +};
> +
> struct dw_i2s_dev {
> void __iomem *i2s_base;
> struct clk *clk;
> int active;
> unsigned int capability;
> struct device *dev;
> + bool using_pd;
>
> /* data related to DMA transfers b/w i2s and DMAC */
> - struct i2s_dma_data play_dma_data;
> - struct i2s_dma_data capture_dma_data;
> + union snd_dma_data play_dma_data;
> + union snd_dma_data capture_dma_data;
> struct i2s_clk_config_data config;
> int (*i2s_clk_cfg)(struct i2s_clk_config_data *config);
> };
> @@ -153,7 +181,7 @@ static int dw_i2s_startup(struct snd_pcm_substream *substream,
> struct snd_soc_dai *cpu_dai)
> {
> struct dw_i2s_dev *dev = snd_soc_dai_get_drvdata(cpu_dai);
> - struct i2s_dma_data *dma_data = NULL;
> + union snd_dma_data *dma_data = NULL;
>
> if (!(dev->capability & DWC_I2S_RECORD) &&
> (substream->stream == SNDRV_PCM_STREAM_CAPTURE))
> @@ -227,8 +255,7 @@ static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
>
> i2s_disable_channels(dev, substream->stream);
>
> - /* Iterate over set of channels - independently controlled.
> - */
> + /* Iterate over set of channels - independently controlled. */
> do {
> if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> i2s_write_reg(dev->i2s_base, TCR(ch_reg),
> @@ -251,13 +278,18 @@ static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
>
> config->sample_rate = params_rate(params);
>
> - if (!dev->i2s_clk_cfg)
> - return -EINVAL;
> + if (dev->using_pd) {
> + ret = dev->i2s_clk_cfg(config);
> + if (ret < 0) {
> + dev_err(dev->dev, "runtime audio clk config fail\n");
> + return ret;
> + }
> + } else {
> + u32 bitclk;
>
> - ret = dev->i2s_clk_cfg(config);
> - if (ret < 0) {
> - dev_err(dev->dev, "runtime audio clk config fail\n");
> - return ret;
> + /* TODO: Validate sample rate against permissible set */
> + bitclk = config->sample_rate * config->data_width * 2;
> + clk_set_rate(dev->clk, bitclk);
> }
>
> return 0;
> @@ -331,6 +363,31 @@ static int dw_i2s_resume(struct snd_soc_dai *dai)
> #define dw_i2s_resume NULL
> #endif
>
> +/* Maximum resolution of a channel - not uniformly spaced */
> +static const u32 fifo_width[] = {
> + 12, 16, 20, 24, 32, 0, 0, 0
> +};
> +
> +/* Width of (DMA) bus */
> +static const u32 bus_widths[] = {
> + DMA_SLAVE_BUSWIDTH_1_BYTE,
> + DMA_SLAVE_BUSWIDTH_2_BYTES,
> + DMA_SLAVE_BUSWIDTH_4_BYTES,
> + DMA_SLAVE_BUSWIDTH_UNDEFINED
> +};
> +
> +/* PCM format to support channel resolution */
> +static const u32 formats[] = {
> + SNDRV_PCM_FMTBIT_S16_LE,
> + SNDRV_PCM_FMTBIT_S16_LE,
> + SNDRV_PCM_FMTBIT_S24_LE,
> + SNDRV_PCM_FMTBIT_S24_LE,
> + SNDRV_PCM_FMTBIT_S32_LE,
> + 0,
> + 0,
> + 0
> +};
> +
> static int dw_i2s_probe(struct platform_device *pdev)
> {
> const struct i2s_platform_data *pdata = pdev->dev.platform_data;
> @@ -340,11 +397,6 @@ static int dw_i2s_probe(struct platform_device *pdev)
> unsigned int cap;
> struct snd_soc_dai_driver *dw_i2s_dai;
>
> - if (!pdata) {
> - dev_err(&pdev->dev, "Invalid platform data\n");
> - return -EINVAL;
> - }
> -
> dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> if (!dev) {
> dev_warn(&pdev->dev, "kzalloc fail\n");
> @@ -373,47 +425,114 @@ static int dw_i2s_probe(struct platform_device *pdev)
> return PTR_ERR(dev->i2s_base);
> }
>
> - cap = pdata->cap;
> - dev->capability = cap;
> - dev->i2s_clk_cfg = pdata->i2s_clk_cfg;
> + if (pdata) {
> + dev->using_pd = true;
> + cap = pdata->cap;
> + dev->capability = cap;
> + dev->i2s_clk_cfg = pdata->i2s_clk_cfg;
> + if (!dev->i2s_clk_cfg) {
> + dev_err(&pdev->dev, "no clock device\n");
> + return -ENODEV;
> + }
>
> - /* Set DMA slaves info */
> + /* Set DMA slaves info */
> +
> + dev->play_dma_data.pd.data = pdata->play_dma_data;
> + dev->capture_dma_data.pd.data = pdata->capture_dma_data;
> + dev->play_dma_data.pd.addr = res->start + I2S_TXDMA;
> + dev->capture_dma_data.pd.addr = res->start + I2S_RXDMA;
> + dev->play_dma_data.pd.max_burst = 16;
> + dev->capture_dma_data.pd.max_burst = 16;
> + dev->play_dma_data.pd.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> + dev->capture_dma_data.pd.addr_width =
> + DMA_SLAVE_BUSWIDTH_2_BYTES;
> + dev->play_dma_data.pd.filter = pdata->filter;
> + dev->capture_dma_data.pd.filter = pdata->filter;
> +
> + dev->clk = clk_get(&pdev->dev, NULL);
> + if (IS_ERR(dev->clk))
> + return PTR_ERR(dev->clk);
> +
> + if (cap & DWC_I2S_PLAY) {
> + dev_dbg(&pdev->dev, " designware: play supported\n");
> + dw_i2s_dai->playback.channels_min = MIN_CHANNEL_NUM;
> + dw_i2s_dai->playback.channels_max = pdata->channel;
> + dw_i2s_dai->playback.formats = pdata->snd_fmts;
> + dw_i2s_dai->playback.rates = pdata->snd_rates;
> + }
>
> - dev->play_dma_data.data = pdata->play_dma_data;
> - dev->capture_dma_data.data = pdata->capture_dma_data;
> - dev->play_dma_data.addr = res->start + I2S_TXDMA;
> - dev->capture_dma_data.addr = res->start + I2S_RXDMA;
> - dev->play_dma_data.max_burst = 16;
> - dev->capture_dma_data.max_burst = 16;
> - dev->play_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> - dev->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> - dev->play_dma_data.filter = pdata->filter;
> - dev->capture_dma_data.filter = pdata->filter;
> + if (cap & DWC_I2S_RECORD) {
> + dev_dbg(&pdev->dev, "designware: record supported\n");
> + dw_i2s_dai->capture.channels_min = MIN_CHANNEL_NUM;
> + dw_i2s_dai->capture.channels_max = pdata->channel;
> + dw_i2s_dai->capture.formats = pdata->snd_fmts;
> + dw_i2s_dai->capture.rates = pdata->snd_rates;
> + }
> + } else {
> + /*
> + * Read component parameter registers to extract
> + * the I2S block's configuration.
> + */
> + u32 comp1 = i2s_read_reg(dev->i2s_base, I2S_COMP_PARAM_1);
> + u32 comp2 = i2s_read_reg(dev->i2s_base, I2S_COMP_PARAM_2);
> + u32 bus_width = bus_widths[COMP1_APB_DATA_WIDTH(comp1)];
> + u32 fifo_depth = 1 << (1 + COMP1_FIFO_DEPTH_GLOBAL(comp1));
> + u32 max_size;
> +
> + dev->using_pd = false;
> +
> + dev->clk = devm_clk_get(&pdev->dev, "i2sclk");
> + if (IS_ERR(dev->clk))
> + return PTR_ERR(dev->clk);
> +
> + /*
> + * Code presumes all channels are configured with the same
> + * word size.
> + */
> + if (COMP1_TX_ENABLED(comp1)) {
> + dev_dbg(&pdev->dev, "playback capable\n");
> +
> + dev->capability |= DWC_I2S_PLAY;
> + max_size = COMP1_TX_WORDSIZE_0(comp1);
> + dev->play_dma_data.dt.addr = res->start + I2S_TXDMA;
> + dev->play_dma_data.dt.addr_width = bus_width;
> + dev->play_dma_data.dt.chan_name = "TX";
> + dev->play_dma_data.dt.fifo_size = fifo_depth *
> + (fifo_width[max_size]) >> 8;
> + dev->play_dma_data.dt.maxburst = 16;
> + dw_i2s_dai->playback.channels_min = MIN_CHANNEL_NUM;
> + dw_i2s_dai->playback.channels_max =
> + 1 << (COMP1_TX_CHANNELS(comp1) + 1);
> + dw_i2s_dai->playback.formats = formats[max_size];
> + dw_i2s_dai->playback.rates = SNDRV_PCM_RATE_8000_192000;
> + }
> + if (COMP1_RX_ENABLED(comp1)) {
> + dev_dbg(&pdev->dev, "record capable\n");
> +
> + dev->capability |= DWC_I2S_RECORD;
> + max_size = COMP2_RX_WORDSIZE_0(comp2);
> + dev->capture_dma_data.dt.addr = res->start + I2S_RXDMA;
> + dev->capture_dma_data.dt.addr_width = bus_width;
> + dev->capture_dma_data.dt.chan_name = "RX";
> + dev->capture_dma_data.dt.fifo_size = fifo_depth *
> + (fifo_width[max_size] >> 8);
> + dev->capture_dma_data.dt.maxburst = 16;
> + dw_i2s_dai->capture.channels_min = MIN_CHANNEL_NUM;
> + dw_i2s_dai->capture.channels_max =
> + 1 << (COMP1_RX_CHANNELS(comp1) + 1);
> + dw_i2s_dai->capture.formats = formats[max_size];
> + dw_i2s_dai->capture.rates = SNDRV_PCM_RATE_8000_192000;
> + }
> + }
>
> - dev->clk = clk_get(&pdev->dev, NULL);
> - if (IS_ERR(dev->clk))
> - return PTR_ERR(dev->clk);
> + ret = clk_prepare(dev->clk);
> + if (ret < 0)
> + goto err_clk_put;
>
> ret = clk_enable(dev->clk);
> if (ret < 0)
> goto err_clk_put;
>
> - if (cap & DWC_I2S_PLAY) {
> - dev_dbg(&pdev->dev, " designware: play supported\n");
> - dw_i2s_dai->playback.channels_min = MIN_CHANNEL_NUM;
> - dw_i2s_dai->playback.channels_max = pdata->channel;
> - dw_i2s_dai->playback.formats = pdata->snd_fmts;
> - dw_i2s_dai->playback.rates = pdata->snd_rates;
> - }
> -
> - if (cap & DWC_I2S_RECORD) {
> - dev_dbg(&pdev->dev, "designware: record supported\n");
> - dw_i2s_dai->capture.channels_min = MIN_CHANNEL_NUM;
> - dw_i2s_dai->capture.channels_max = pdata->channel;
> - dw_i2s_dai->capture.formats = pdata->snd_fmts;
> - dw_i2s_dai->capture.rates = pdata->snd_rates;
> - }
> -
> dev->dev = &pdev->dev;
> dev_set_drvdata(&pdev->dev, dev);
> ret = snd_soc_register_component(&pdev->dev, &dw_i2s_component,
> @@ -423,6 +542,15 @@ static int dw_i2s_probe(struct platform_device *pdev)
> goto err_clk_disable;
> }
>
> + if (!dev->using_pd) {
> + ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Could not register PCM: %d\n", ret);
> + return ret;
> + }
> + }
> +
> return 0;
>
> err_clk_disable:
> @@ -443,12 +571,24 @@ static int dw_i2s_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id dw_i2s_of_match[] = {
> + { .compatible = "snps,designware-i2s", },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, dw_i2s_of_match);
> +#endif
> +
> static struct platform_driver dw_i2s_driver = {
> .probe = dw_i2s_probe,
> .remove = dw_i2s_remove,
> .driver = {
> .name = "designware-i2s",
> .owner = THIS_MODULE,
> +#ifdef CONFIG_OF
> + .of_match_table = dw_i2s_of_match,
> +#endif
> },
> };
>
> --
> 1.7.1
>
Sorry for the last blank mail.
On Wed, Dec 3, 2014 at 10:09 PM, Andrew Jackson <[email protected]> wrote:
> Convert to driver to use either platform_data or device-tree for configuration
> of the device. When using device-tree, the I2S block's configuration is read
> from the relevant registers: this reduces the amount of information required in
> the device tree.
>
> Signed-off-by: Andrew Jackson <[email protected]>
> ---
> .../devicetree/bindings/sound/designware-i2s.txt | 32 +++
> sound/soc/dwc/Kconfig | 1 +
> sound/soc/dwc/designware_i2s.c | 238 ++++++++++++++++----
> 3 files changed, 222 insertions(+), 49 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/sound/designware-i2s.txt
>
> diff --git a/Documentation/devicetree/bindings/sound/designware-i2s.txt b/Documentation/devicetree/bindings/sound/designware-i2s.txt
> new file mode 100644
> index 0000000..cdee591
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/designware-i2s.txt
> @@ -0,0 +1,32 @@
> +DesignWare I2S controller
> +
> +Required properties:
> + - compatible : Must be "snps,designware-i2s"
> + - reg : Must contain I2S core's registers location and length
> + - clocks : Pairs of phandle and specifier referencing the controller's clocks.
> + The controller expects two clocks, the clock used for the APB interface and
> + the clock used as the sampling rate reference clock sample.
> + - clock-names : "apb_plck" for the clock to the APB interface, "i2sclk" for the sample
> + rate reference clock.
> + - dmas: Pairs of phandle and specifier for the DMA channels that are used by
> + the core. The core expects one or two dma channels, one for transmit and one for
> + receive.
> + - dma-names : "tx" for the transmit channel, "rx" for the receive channel.
> +
> +For more details on the 'dma', 'dma-names', 'clock' and 'clock-names' properties
> +please check:
> + * resource-names.txt
> + * clock/clock-bindings.txt
> + * dma/dma.txt
> +
> +Example:
> +
> + soc_i2s: i2s@7ff90000 {
> + compatible = "snps,designware-i2s";
> + reg = <0x0 0x7ff90000 0x0 0x1000>;
> + clocks = <&scpi_i2sclk 0>, <&soc_refclk100mhz>;
> + clock-names = "i2sclk", "apb_pclk";
> + #sound-dai-cells = <0>;
> + dmas = <&dma0 5>;
> + dma-names = "tx";
> + };
> diff --git a/sound/soc/dwc/Kconfig b/sound/soc/dwc/Kconfig
> index e334900..d50e085 100644
> --- a/sound/soc/dwc/Kconfig
> +++ b/sound/soc/dwc/Kconfig
> @@ -1,6 +1,7 @@
> config SND_DESIGNWARE_I2S
> tristate "Synopsys I2S Device Driver"
> depends on CLKDEV_LOOKUP
> + select SND_SOC_GENERIC_DMAENGINE_PCM
> help
> Say Y or M if you want to add support for I2S driver for
> Synopsys desigwnware I2S device. The device supports upto
> diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c
> index c497ada..083779d 100644
> --- a/sound/soc/dwc/designware_i2s.c
> +++ b/sound/soc/dwc/designware_i2s.c
> @@ -22,6 +22,7 @@
> #include <sound/pcm.h>
> #include <sound/pcm_params.h>
> #include <sound/soc.h>
> +#include <sound/dmaengine_pcm.h>
>
> /* common register for all channel */
> #define IER 0x000
> @@ -54,19 +55,46 @@
> #define I2S_COMP_VERSION 0x01F8
> #define I2S_COMP_TYPE 0x01FC
>
> +/*
> + * Component parameter register fields - define the I2S block's
> + * configuration.
> + */
> +#define COMP1_TX_WORDSIZE_3(r) (((r) & GENMASK(27, 25)) >> 25)
> +#define COMP1_TX_WORDSIZE_2(r) (((r) & GENMASK(24, 22)) >> 22)
> +#define COMP1_TX_WORDSIZE_1(r) (((r) & GENMASK(21, 19)) >> 19)
> +#define COMP1_TX_WORDSIZE_0(r) (((r) & GENMASK(18, 16)) >> 16)
> +#define COMP1_TX_CHANNELS(r) (((r) & GENMASK(10, 9)) >> 9)
> +#define COMP1_RX_CHANNELS(r) (((r) & GENMASK(8, 7)) >> 7)
> +#define COMP1_RX_ENABLED(r) (((r) & BIT(6)) >> 6)
> +#define COMP1_TX_ENABLED(r) (((r) & BIT(5)) >> 5)
> +#define COMP1_MODE_EN(r) (((r) & BIT(4)) >> 4)
> +#define COMP1_FIFO_DEPTH_GLOBAL(r) (((r) & GENMASK(3, 2)) >> 2)
> +#define COMP1_APB_DATA_WIDTH(r) (((r) & GENMASK(1, 0)) >> 0)
> +
> +#define COMP2_RX_WORDSIZE_3(r) (((r) & GENMASK(12, 10)) >> 10)
> +#define COMP2_RX_WORDSIZE_2(r) (((r) & GENMASK(9, 7)) >> 7)
> +#define COMP2_RX_WORDSIZE_1(r) (((r) & GENMASK(5, 3)) >> 3)
> +#define COMP2_RX_WORDSIZE_0(r) (((r) & GENMASK(2, 0)) >> 0)
> +
> #define MAX_CHANNEL_NUM 8
> #define MIN_CHANNEL_NUM 2
>
> +union snd_dma_data {
> + struct i2s_dma_data pd;
> + struct snd_dmaengine_dai_dma_data dt;
> +};
> +
> struct dw_i2s_dev {
> void __iomem *i2s_base;
> struct clk *clk;
> int active;
> unsigned int capability;
> struct device *dev;
> + bool using_pd;
>
> /* data related to DMA transfers b/w i2s and DMAC */
> - struct i2s_dma_data play_dma_data;
> - struct i2s_dma_data capture_dma_data;
> + union snd_dma_data play_dma_data;
> + union snd_dma_data capture_dma_data;
> struct i2s_clk_config_data config;
> int (*i2s_clk_cfg)(struct i2s_clk_config_data *config);
> };
> @@ -153,7 +181,7 @@ static int dw_i2s_startup(struct snd_pcm_substream *substream,
> struct snd_soc_dai *cpu_dai)
> {
> struct dw_i2s_dev *dev = snd_soc_dai_get_drvdata(cpu_dai);
> - struct i2s_dma_data *dma_data = NULL;
> + union snd_dma_data *dma_data = NULL;
>
> if (!(dev->capability & DWC_I2S_RECORD) &&
> (substream->stream == SNDRV_PCM_STREAM_CAPTURE))
> @@ -227,8 +255,7 @@ static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
>
> i2s_disable_channels(dev, substream->stream);
>
> - /* Iterate over set of channels - independently controlled.
> - */
> + /* Iterate over set of channels - independently controlled. */
I dont think so this iteration is required as there are independent
channels available.
I don't have the h/w to test this patch. Can anyone test this ?
> do {
> if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> i2s_write_reg(dev->i2s_base, TCR(ch_reg),
> @@ -251,13 +278,18 @@ static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
>
> config->sample_rate = params_rate(params);
>
> - if (!dev->i2s_clk_cfg)
> - return -EINVAL;
> + if (dev->using_pd) {
> + ret = dev->i2s_clk_cfg(config);
> + if (ret < 0) {
> + dev_err(dev->dev, "runtime audio clk config fail\n");
> + return ret;
> + }
> + } else {
> + u32 bitclk;
>
> - ret = dev->i2s_clk_cfg(config);
> - if (ret < 0) {
> - dev_err(dev->dev, "runtime audio clk config fail\n");
> - return ret;
> + /* TODO: Validate sample rate against permissible set */
> + bitclk = config->sample_rate * config->data_width * 2;
> + clk_set_rate(dev->clk, bitclk);
> }
>
> return 0;
> @@ -331,6 +363,31 @@ static int dw_i2s_resume(struct snd_soc_dai *dai)
> #define dw_i2s_resume NULL
> #endif
>
> +/* Maximum resolution of a channel - not uniformly spaced */
> +static const u32 fifo_width[] = {
> + 12, 16, 20, 24, 32, 0, 0, 0
> +};
> +
> +/* Width of (DMA) bus */
> +static const u32 bus_widths[] = {
> + DMA_SLAVE_BUSWIDTH_1_BYTE,
> + DMA_SLAVE_BUSWIDTH_2_BYTES,
> + DMA_SLAVE_BUSWIDTH_4_BYTES,
> + DMA_SLAVE_BUSWIDTH_UNDEFINED
> +};
> +
> +/* PCM format to support channel resolution */
> +static const u32 formats[] = {
> + SNDRV_PCM_FMTBIT_S16_LE,
> + SNDRV_PCM_FMTBIT_S16_LE,
> + SNDRV_PCM_FMTBIT_S24_LE,
> + SNDRV_PCM_FMTBIT_S24_LE,
> + SNDRV_PCM_FMTBIT_S32_LE,
> + 0,
> + 0,
> + 0
> +};
> +
> static int dw_i2s_probe(struct platform_device *pdev)
> {
> const struct i2s_platform_data *pdata = pdev->dev.platform_data;
> @@ -340,11 +397,6 @@ static int dw_i2s_probe(struct platform_device *pdev)
> unsigned int cap;
> struct snd_soc_dai_driver *dw_i2s_dai;
>
> - if (!pdata) {
> - dev_err(&pdev->dev, "Invalid platform data\n");
> - return -EINVAL;
> - }
> -
This check is required..
> dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> if (!dev) {
> dev_warn(&pdev->dev, "kzalloc fail\n");
> @@ -373,47 +425,114 @@ static int dw_i2s_probe(struct platform_device *pdev)
> return PTR_ERR(dev->i2s_base);
> }
>
> - cap = pdata->cap;
> - dev->capability = cap;
> - dev->i2s_clk_cfg = pdata->i2s_clk_cfg;
> + if (pdata) {
> + dev->using_pd = true;
> + cap = pdata->cap;
> + dev->capability = cap;
We should take take capability from DT, as
if (of_get_property(np, "play", NULL))
cap |= DWC_I2S_PLAY;
if (of_get_property(np, "record", NULL))
cap |= DWC_I2S_RECORD;
> + dev->i2s_clk_cfg = pdata->i2s_clk_cfg;
> + if (!dev->i2s_clk_cfg) {
> + dev_err(&pdev->dev, "no clock device\n");
> + return -ENODEV;
> + }
>
> - /* Set DMA slaves info */
> + /* Set DMA slaves info */
> +
> + dev->play_dma_data.pd.data = pdata->play_dma_data;
As pdata null check is removed it may create issue here !
> + dev->capture_dma_data.pd.data = pdata->capture_dma_data;
> + dev->play_dma_data.pd.addr = res->start + I2S_TXDMA;
> + dev->capture_dma_data.pd.addr = res->start + I2S_RXDMA;
> + dev->play_dma_data.pd.max_burst = 16;
> + dev->capture_dma_data.pd.max_burst = 16;
> + dev->play_dma_data.pd.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> + dev->capture_dma_data.pd.addr_width =
> + DMA_SLAVE_BUSWIDTH_2_BYTES;
> + dev->play_dma_data.pd.filter = pdata->filter;
> + dev->capture_dma_data.pd.filter = pdata->filter;
> +
> + dev->clk = clk_get(&pdev->dev, NULL);
> + if (IS_ERR(dev->clk))
> + return PTR_ERR(dev->clk);
> +
> + if (cap & DWC_I2S_PLAY) {
> + dev_dbg(&pdev->dev, " designware: play supported\n");
> + dw_i2s_dai->playback.channels_min = MIN_CHANNEL_NUM;
> + dw_i2s_dai->playback.channels_max = pdata->channel;
> + dw_i2s_dai->playback.formats = pdata->snd_fmts;
> + dw_i2s_dai->playback.rates = pdata->snd_rates;
> + }
>
> - dev->play_dma_data.data = pdata->play_dma_data;
> - dev->capture_dma_data.data = pdata->capture_dma_data;
> - dev->play_dma_data.addr = res->start + I2S_TXDMA;
> - dev->capture_dma_data.addr = res->start + I2S_RXDMA;
> - dev->play_dma_data.max_burst = 16;
> - dev->capture_dma_data.max_burst = 16;
> - dev->play_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> - dev->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> - dev->play_dma_data.filter = pdata->filter;
> - dev->capture_dma_data.filter = pdata->filter;
> + if (cap & DWC_I2S_RECORD) {
> + dev_dbg(&pdev->dev, "designware: record supported\n");
> + dw_i2s_dai->capture.channels_min = MIN_CHANNEL_NUM;
> + dw_i2s_dai->capture.channels_max = pdata->channel;
> + dw_i2s_dai->capture.formats = pdata->snd_fmts;
> + dw_i2s_dai->capture.rates = pdata->snd_rates;
> + }
> + } else {
> + /*
> + * Read component parameter registers to extract
> + * the I2S block's configuration.
> + */
> + u32 comp1 = i2s_read_reg(dev->i2s_base, I2S_COMP_PARAM_1);
> + u32 comp2 = i2s_read_reg(dev->i2s_base, I2S_COMP_PARAM_2);
> + u32 bus_width = bus_widths[COMP1_APB_DATA_WIDTH(comp1)];
> + u32 fifo_depth = 1 << (1 + COMP1_FIFO_DEPTH_GLOBAL(comp1));
> + u32 max_size;
> +
> + dev->using_pd = false;
> +
> + dev->clk = devm_clk_get(&pdev->dev, "i2sclk");
> + if (IS_ERR(dev->clk))
> + return PTR_ERR(dev->clk);
> +
> + /*
> + * Code presumes all channels are configured with the same
> + * word size.
> + */
But why all channels ? You should configure only those channel which
are required. The number of channel you can pass from either pdata or
DT.
This is also not required as before play start you are going to enable
only those channel which are required and disable others.
Best Regards
Rajeev
> + if (COMP1_TX_ENABLED(comp1)) {
> + dev_dbg(&pdev->dev, "playback capable\n");
> +
> + dev->capability |= DWC_I2S_PLAY;
> + max_size = COMP1_TX_WORDSIZE_0(comp1);
> + dev->play_dma_data.dt.addr = res->start + I2S_TXDMA;
> + dev->play_dma_data.dt.addr_width = bus_width;
> + dev->play_dma_data.dt.chan_name = "TX";
> + dev->play_dma_data.dt.fifo_size = fifo_depth *
> + (fifo_width[max_size]) >> 8;
> + dev->play_dma_data.dt.maxburst = 16;
> + dw_i2s_dai->playback.channels_min = MIN_CHANNEL_NUM;
> + dw_i2s_dai->playback.channels_max =
> + 1 << (COMP1_TX_CHANNELS(comp1) + 1);
> + dw_i2s_dai->playback.formats = formats[max_size];
> + dw_i2s_dai->playback.rates = SNDRV_PCM_RATE_8000_192000;
> + }
> + if (COMP1_RX_ENABLED(comp1)) {
> + dev_dbg(&pdev->dev, "record capable\n");
> +
> + dev->capability |= DWC_I2S_RECORD;
> + max_size = COMP2_RX_WORDSIZE_0(comp2);
> + dev->capture_dma_data.dt.addr = res->start + I2S_RXDMA;
> + dev->capture_dma_data.dt.addr_width = bus_width;
> + dev->capture_dma_data.dt.chan_name = "RX";
> + dev->capture_dma_data.dt.fifo_size = fifo_depth *
> + (fifo_width[max_size] >> 8);
> + dev->capture_dma_data.dt.maxburst = 16;
> + dw_i2s_dai->capture.channels_min = MIN_CHANNEL_NUM;
> + dw_i2s_dai->capture.channels_max =
> + 1 << (COMP1_RX_CHANNELS(comp1) + 1);
> + dw_i2s_dai->capture.formats = formats[max_size];
> + dw_i2s_dai->capture.rates = SNDRV_PCM_RATE_8000_192000;
> + }
> + }
>
> - dev->clk = clk_get(&pdev->dev, NULL);
> - if (IS_ERR(dev->clk))
> - return PTR_ERR(dev->clk);
> + ret = clk_prepare(dev->clk);
> + if (ret < 0)
> + goto err_clk_put;
>
> ret = clk_enable(dev->clk);
> if (ret < 0)
> goto err_clk_put;
>
> - if (cap & DWC_I2S_PLAY) {
> - dev_dbg(&pdev->dev, " designware: play supported\n");
> - dw_i2s_dai->playback.channels_min = MIN_CHANNEL_NUM;
> - dw_i2s_dai->playback.channels_max = pdata->channel;
> - dw_i2s_dai->playback.formats = pdata->snd_fmts;
> - dw_i2s_dai->playback.rates = pdata->snd_rates;
> - }
> -
> - if (cap & DWC_I2S_RECORD) {
> - dev_dbg(&pdev->dev, "designware: record supported\n");
> - dw_i2s_dai->capture.channels_min = MIN_CHANNEL_NUM;
> - dw_i2s_dai->capture.channels_max = pdata->channel;
> - dw_i2s_dai->capture.formats = pdata->snd_fmts;
> - dw_i2s_dai->capture.rates = pdata->snd_rates;
> - }
> -
> dev->dev = &pdev->dev;
> dev_set_drvdata(&pdev->dev, dev);
> ret = snd_soc_register_component(&pdev->dev, &dw_i2s_component,
> @@ -423,6 +542,15 @@ static int dw_i2s_probe(struct platform_device *pdev)
> goto err_clk_disable;
> }
>
> + if (!dev->using_pd) {
> + ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Could not register PCM: %d\n", ret);
> + return ret;
> + }
> + }
> +
> return 0;
>
> err_clk_disable:
> @@ -443,12 +571,24 @@ static int dw_i2s_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id dw_i2s_of_match[] = {
> + { .compatible = "snps,designware-i2s", },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, dw_i2s_of_match);
> +#endif
> +
> static struct platform_driver dw_i2s_driver = {
> .probe = dw_i2s_probe,
> .remove = dw_i2s_remove,
> .driver = {
> .name = "designware-i2s",
> .owner = THIS_MODULE,
> +#ifdef CONFIG_OF
> + .of_match_table = dw_i2s_of_match,
> +#endif
> },
> };
>
> --
> 1.7.1
>
On 12/03/14 18:23, Mark Brown wrote:
> On Wed, Dec 03, 2014 at 04:39:08PM +0000, Andrew Jackson wrote:
>
>> Convert to driver to use either platform_data or device-tree for configuration
>> of the device. When using device-tree, the I2S block's configuration is read
>> from the relevant registers: this reduces the amount of information required in
>> the device tree.
>
> This really needs to be split into two or more patches, there's a whole
> bunch of refactoring to support this DT stuff which should be separate
> from the DT addition itself. Right now it's hard to tell what each
> individual bit of the code is supposed to be doing, the patch is far too
> large and doing far too many individual things.
I will have look at how it might be split. The majority of the new code is in reading and processing the device's configuration: I didn't want to change the platform data handling to do that because some of the comments in the driver suggested that there were ST specific changes to the Designware IP. I wasn't in a position to know whether, if I changed the configuration reading, the driver would still function correctly on the SPEAR platform.
>> + if (dev->using_pd) {
>> + ret = dev->i2s_clk_cfg(config);
>> + if (ret < 0) {
>> + dev_err(dev->dev, "runtime audio clk config fail\n");
>> + return ret;
>> + }
>> + } else {
>> + u32 bitclk;
>
> Having this whole separate path for using platform data feels icky, we
> don't want to have completely separate flows like this. Checking for
> the callbacks being there is probably fine but just having totally
> separate code paths is a bit icky.
I wasn't very happy either but making the test explicit seemed reasonable at the time. I'll change the code to test for the presence of the callback instead.
Andrew
On 12/03/14 20:13, Arnd Bergmann wrote:
> On Wednesday 03 December 2014 16:39:08 Andrew Jackson wrote:
>> Convert to driver to use either platform_data or device-tree for configuration
>> of the device. When using device-tree, the I2S block's configuration is read
>> from the relevant registers: this reduces the amount of information required in
>> the device tree.
>>
>> Signed-off-by: Andrew Jackson <[email protected]>
>
> I don't think we even have to worry about the platform_data case here:
> the only platform using this hardware in Linux is arm/mach-spear, and
> it defines a device node with a binding that is similar to the one you
> document here but that is not implemented in the driver.
>
> So, I think for all practical purposes we can assume that nobody cares
> if you make incompatible changes as long as you don't introduce build
> regression.
>
> Also, please adapt the arch/arm/boot/dts/spear13*.dts{,i} files
> as good as you can. They are broken in other ways too that you don't
> have to fix, just make them conform to the binding you add.
>
Will do. Although I had noticed those DT entries, I couldn't find anything in support of them. I wasn't sure whether someone else was waiting in the wings (so as to speak) with a DT enabled Designware I2S driver.
> Arnd
>
>
On Wednesday 03 December 2014 20:19:05 Mark Brown wrote:
> On Wed, Dec 03, 2014 at 09:13:18PM +0100, Arnd Bergmann wrote:
>
> > I don't think we even have to worry about the platform_data case here:
> > the only platform using this hardware in Linux is arm/mach-spear, and
> > it defines a device node with a binding that is similar to the one you
> > document here but that is not implemented in the driver.
>
> Not all the world uses DT, this is a DesignWare IP so it's likely to get
> deployed widely - potentially on PCI cards and things like that.
It's not really new though, and we are only seeing the second user.
Maybe this one just isn't as common as a lot of the other designware IP?
Arnd
On 12/04/14 07:43, rajeev kumar wrote:
> Sorry for the last blank mail.
>
> On Wed, Dec 3, 2014 at 10:09 PM, Andrew Jackson <[email protected]> wrote:
>> Convert to driver to use either platform_data or device-tree for configuration
>> of the device. When using device-tree, the I2S block's configuration is read
>> from the relevant registers: this reduces the amount of information required in
>> the device tree.
>>
>> Signed-off-by: Andrew Jackson <[email protected]>
>> ---
>> .../devicetree/bindings/sound/designware-i2s.txt | 32 +++
>> sound/soc/dwc/Kconfig | 1 +
>> sound/soc/dwc/designware_i2s.c | 238 ++++++++++++++++----
>> 3 files changed, 222 insertions(+), 49 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/sound/designware-i2s.txt
>>
>> diff --git a/Documentation/devicetree/bindings/sound/designware-i2s.txt b/Documentation/devicetree/bindings/sound/designware-i2s.txt
>> new file mode 100644
>> index 0000000..cdee591
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/designware-i2s.txt
>> @@ -0,0 +1,32 @@
>> +DesignWare I2S controller
>> +
>> +Required properties:
>> + - compatible : Must be "snps,designware-i2s"
>> + - reg : Must contain I2S core's registers location and length
>> + - clocks : Pairs of phandle and specifier referencing the controller's clocks.
>> + The controller expects two clocks, the clock used for the APB interface and
>> + the clock used as the sampling rate reference clock sample.
>> + - clock-names : "apb_plck" for the clock to the APB interface, "i2sclk" for the sample
>> + rate reference clock.
>> + - dmas: Pairs of phandle and specifier for the DMA channels that are used by
>> + the core. The core expects one or two dma channels, one for transmit and one for
>> + receive.
>> + - dma-names : "tx" for the transmit channel, "rx" for the receive channel.
>> +
>> +For more details on the 'dma', 'dma-names', 'clock' and 'clock-names' properties
>> +please check:
>> + * resource-names.txt
>> + * clock/clock-bindings.txt
>> + * dma/dma.txt
>> +
>> +Example:
>> +
>> + soc_i2s: i2s@7ff90000 {
>> + compatible = "snps,designware-i2s";
>> + reg = <0x0 0x7ff90000 0x0 0x1000>;
>> + clocks = <&scpi_i2sclk 0>, <&soc_refclk100mhz>;
>> + clock-names = "i2sclk", "apb_pclk";
>> + #sound-dai-cells = <0>;
>> + dmas = <&dma0 5>;
>> + dma-names = "tx";
>> + };
>> diff --git a/sound/soc/dwc/Kconfig b/sound/soc/dwc/Kconfig
>> index e334900..d50e085 100644
>> --- a/sound/soc/dwc/Kconfig
>> +++ b/sound/soc/dwc/Kconfig
>> @@ -1,6 +1,7 @@
>> config SND_DESIGNWARE_I2S
>> tristate "Synopsys I2S Device Driver"
>> depends on CLKDEV_LOOKUP
>> + select SND_SOC_GENERIC_DMAENGINE_PCM
>> help
>> Say Y or M if you want to add support for I2S driver for
>> Synopsys desigwnware I2S device. The device supports upto
>> diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c
>> index c497ada..083779d 100644
>> --- a/sound/soc/dwc/designware_i2s.c
>> +++ b/sound/soc/dwc/designware_i2s.c
>> @@ -22,6 +22,7 @@
>> #include <sound/pcm.h>
>> #include <sound/pcm_params.h>
>> #include <sound/soc.h>
>> +#include <sound/dmaengine_pcm.h>
>>
>> /* common register for all channel */
>> #define IER 0x000
>> @@ -54,19 +55,46 @@
>> #define I2S_COMP_VERSION 0x01F8
>> #define I2S_COMP_TYPE 0x01FC
>>
>> +/*
>> + * Component parameter register fields - define the I2S block's
>> + * configuration.
>> + */
>> +#define COMP1_TX_WORDSIZE_3(r) (((r) & GENMASK(27, 25)) >> 25)
>> +#define COMP1_TX_WORDSIZE_2(r) (((r) & GENMASK(24, 22)) >> 22)
>> +#define COMP1_TX_WORDSIZE_1(r) (((r) & GENMASK(21, 19)) >> 19)
>> +#define COMP1_TX_WORDSIZE_0(r) (((r) & GENMASK(18, 16)) >> 16)
>> +#define COMP1_TX_CHANNELS(r) (((r) & GENMASK(10, 9)) >> 9)
>> +#define COMP1_RX_CHANNELS(r) (((r) & GENMASK(8, 7)) >> 7)
>> +#define COMP1_RX_ENABLED(r) (((r) & BIT(6)) >> 6)
>> +#define COMP1_TX_ENABLED(r) (((r) & BIT(5)) >> 5)
>> +#define COMP1_MODE_EN(r) (((r) & BIT(4)) >> 4)
>> +#define COMP1_FIFO_DEPTH_GLOBAL(r) (((r) & GENMASK(3, 2)) >> 2)
>> +#define COMP1_APB_DATA_WIDTH(r) (((r) & GENMASK(1, 0)) >> 0)
>> +
>> +#define COMP2_RX_WORDSIZE_3(r) (((r) & GENMASK(12, 10)) >> 10)
>> +#define COMP2_RX_WORDSIZE_2(r) (((r) & GENMASK(9, 7)) >> 7)
>> +#define COMP2_RX_WORDSIZE_1(r) (((r) & GENMASK(5, 3)) >> 3)
>> +#define COMP2_RX_WORDSIZE_0(r) (((r) & GENMASK(2, 0)) >> 0)
>> +
>> #define MAX_CHANNEL_NUM 8
>> #define MIN_CHANNEL_NUM 2
>>
>> +union snd_dma_data {
>> + struct i2s_dma_data pd;
>> + struct snd_dmaengine_dai_dma_data dt;
>> +};
>> +
>> struct dw_i2s_dev {
>> void __iomem *i2s_base;
>> struct clk *clk;
>> int active;
>> unsigned int capability;
>> struct device *dev;
>> + bool using_pd;
>>
>> /* data related to DMA transfers b/w i2s and DMAC */
>> - struct i2s_dma_data play_dma_data;
>> - struct i2s_dma_data capture_dma_data;
>> + union snd_dma_data play_dma_data;
>> + union snd_dma_data capture_dma_data;
>> struct i2s_clk_config_data config;
>> int (*i2s_clk_cfg)(struct i2s_clk_config_data *config);
>> };
>> @@ -153,7 +181,7 @@ static int dw_i2s_startup(struct snd_pcm_substream *substream,
>> struct snd_soc_dai *cpu_dai)
>> {
>> struct dw_i2s_dev *dev = snd_soc_dai_get_drvdata(cpu_dai);
>> - struct i2s_dma_data *dma_data = NULL;
>> + union snd_dma_data *dma_data = NULL;
>>
>> if (!(dev->capability & DWC_I2S_RECORD) &&
>> (substream->stream == SNDRV_PCM_STREAM_CAPTURE))
>> @@ -227,8 +255,7 @@ static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
>>
>> i2s_disable_channels(dev, substream->stream);
>>
>> - /* Iterate over set of channels - independently controlled.
>> - */
>> + /* Iterate over set of channels - independently controlled. */
>
> I dont think so this iteration is required as there are independent
> channels available.
>
See other email.
> I don't have the h/w to test this patch. Can anyone test this ?
>
>> do {
>> if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>> i2s_write_reg(dev->i2s_base, TCR(ch_reg),
>> @@ -251,13 +278,18 @@ static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
>>
>> config->sample_rate = params_rate(params);
>>
>> - if (!dev->i2s_clk_cfg)
>> - return -EINVAL;
>> + if (dev->using_pd) {
>> + ret = dev->i2s_clk_cfg(config);
>> + if (ret < 0) {
>> + dev_err(dev->dev, "runtime audio clk config fail\n");
>> + return ret;
>> + }
>> + } else {
>> + u32 bitclk;
>>
>> - ret = dev->i2s_clk_cfg(config);
>> - if (ret < 0) {
>> - dev_err(dev->dev, "runtime audio clk config fail\n");
>> - return ret;
>> + /* TODO: Validate sample rate against permissible set */
>> + bitclk = config->sample_rate * config->data_width * 2;
>> + clk_set_rate(dev->clk, bitclk);
>> }
>>
>> return 0;
>> @@ -331,6 +363,31 @@ static int dw_i2s_resume(struct snd_soc_dai *dai)
>> #define dw_i2s_resume NULL
>> #endif
>>
>> +/* Maximum resolution of a channel - not uniformly spaced */
>> +static const u32 fifo_width[] = {
>> + 12, 16, 20, 24, 32, 0, 0, 0
>> +};
>> +
>> +/* Width of (DMA) bus */
>> +static const u32 bus_widths[] = {
>> + DMA_SLAVE_BUSWIDTH_1_BYTE,
>> + DMA_SLAVE_BUSWIDTH_2_BYTES,
>> + DMA_SLAVE_BUSWIDTH_4_BYTES,
>> + DMA_SLAVE_BUSWIDTH_UNDEFINED
>> +};
>> +
>> +/* PCM format to support channel resolution */
>> +static const u32 formats[] = {
>> + SNDRV_PCM_FMTBIT_S16_LE,
>> + SNDRV_PCM_FMTBIT_S16_LE,
>> + SNDRV_PCM_FMTBIT_S24_LE,
>> + SNDRV_PCM_FMTBIT_S24_LE,
>> + SNDRV_PCM_FMTBIT_S32_LE,
>> + 0,
>> + 0,
>> + 0
>> +};
>> +
>> static int dw_i2s_probe(struct platform_device *pdev)
>> {
>> const struct i2s_platform_data *pdata = pdev->dev.platform_data;
>> @@ -340,11 +397,6 @@ static int dw_i2s_probe(struct platform_device *pdev)
>> unsigned int cap;
>> struct snd_soc_dai_driver *dw_i2s_dai;
>>
>> - if (!pdata) {
>> - dev_err(&pdev->dev, "Invalid platform data\n");
>> - return -EINVAL;
>> - }
>> -
>
> This check is required..
It is handled below: the absence of platform data (now) implies a device tree implementation.
>
>> dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
>> if (!dev) {
>> dev_warn(&pdev->dev, "kzalloc fail\n");
>> @@ -373,47 +425,114 @@ static int dw_i2s_probe(struct platform_device *pdev)
>> return PTR_ERR(dev->i2s_base);
>> }
>>
>> - cap = pdata->cap;
>> - dev->capability = cap;
>> - dev->i2s_clk_cfg = pdata->i2s_clk_cfg;
>> + if (pdata) {
>> + dev->using_pd = true;
>> + cap = pdata->cap;
>> + dev->capability = cap;
>
> We should take take capability from DT, as
>
> if (of_get_property(np, "play", NULL))
> cap |= DWC_I2S_PLAY;
> if (of_get_property(np, "record", NULL))
> cap |= DWC_I2S_RECORD;
>
>> + dev->i2s_clk_cfg = pdata->i2s_clk_cfg;
>> + if (!dev->i2s_clk_cfg) {
>> + dev_err(&pdev->dev, "no clock device\n");
>> + return -ENODEV;
>> + }
>>
>> - /* Set DMA slaves info */
>> + /* Set DMA slaves info */
>> +
>> + dev->play_dma_data.pd.data = pdata->play_dma_data;
>
> As pdata null check is removed it may create issue here !
No, because we're in the 'if (pdata)' section of code.
>
>> + dev->capture_dma_data.pd.data = pdata->capture_dma_data;
>> + dev->play_dma_data.pd.addr = res->start + I2S_TXDMA;
>> + dev->capture_dma_data.pd.addr = res->start + I2S_RXDMA;
>> + dev->play_dma_data.pd.max_burst = 16;
>> + dev->capture_dma_data.pd.max_burst = 16;
>> + dev->play_dma_data.pd.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
>> + dev->capture_dma_data.pd.addr_width =
>> + DMA_SLAVE_BUSWIDTH_2_BYTES;
>> + dev->play_dma_data.pd.filter = pdata->filter;
>> + dev->capture_dma_data.pd.filter = pdata->filter;
>> +
>> + dev->clk = clk_get(&pdev->dev, NULL);
>> + if (IS_ERR(dev->clk))
>> + return PTR_ERR(dev->clk);
>> +
>> + if (cap & DWC_I2S_PLAY) {
>> + dev_dbg(&pdev->dev, " designware: play supported\n");
>> + dw_i2s_dai->playback.channels_min = MIN_CHANNEL_NUM;
>> + dw_i2s_dai->playback.channels_max = pdata->channel;
>> + dw_i2s_dai->playback.formats = pdata->snd_fmts;
>> + dw_i2s_dai->playback.rates = pdata->snd_rates;
>> + }
>>
>> - dev->play_dma_data.data = pdata->play_dma_data;
>> - dev->capture_dma_data.data = pdata->capture_dma_data;
>> - dev->play_dma_data.addr = res->start + I2S_TXDMA;
>> - dev->capture_dma_data.addr = res->start + I2S_RXDMA;
>> - dev->play_dma_data.max_burst = 16;
>> - dev->capture_dma_data.max_burst = 16;
>> - dev->play_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
>> - dev->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
>> - dev->play_dma_data.filter = pdata->filter;
>> - dev->capture_dma_data.filter = pdata->filter;
>> + if (cap & DWC_I2S_RECORD) {
>> + dev_dbg(&pdev->dev, "designware: record supported\n");
>> + dw_i2s_dai->capture.channels_min = MIN_CHANNEL_NUM;
>> + dw_i2s_dai->capture.channels_max = pdata->channel;
>> + dw_i2s_dai->capture.formats = pdata->snd_fmts;
>> + dw_i2s_dai->capture.rates = pdata->snd_rates;
>> + }
>> + } else {
>> + /*
>> + * Read component parameter registers to extract
>> + * the I2S block's configuration.
>> + */
>> + u32 comp1 = i2s_read_reg(dev->i2s_base, I2S_COMP_PARAM_1);
>> + u32 comp2 = i2s_read_reg(dev->i2s_base, I2S_COMP_PARAM_2);
>> + u32 bus_width = bus_widths[COMP1_APB_DATA_WIDTH(comp1)];
>> + u32 fifo_depth = 1 << (1 + COMP1_FIFO_DEPTH_GLOBAL(comp1));
>> + u32 max_size;
>> +
>> + dev->using_pd = false;
>> +
>> + dev->clk = devm_clk_get(&pdev->dev, "i2sclk");
>> + if (IS_ERR(dev->clk))
>> + return PTR_ERR(dev->clk);
>> +
>> + /*
>> + * Code presumes all channels are configured with the same
>> + * word size.
>> + */
>
> But why all channels ? You should configure only those channel which
> are required. The number of channel you can pass from either pdata or
> DT.
> This is also not required as before play start you are going to enable
> only those channel which are required and disable others.
I will change the comment to say "configured in the IP" i.e. at design time. This is the physical configuration of the IP block.
Andrew
>
> Best Regards
> Rajeev
>
>
>> + if (COMP1_TX_ENABLED(comp1)) {
>> + dev_dbg(&pdev->dev, "playback capable\n");
>> +
>> + dev->capability |= DWC_I2S_PLAY;
>> + max_size = COMP1_TX_WORDSIZE_0(comp1);
>> + dev->play_dma_data.dt.addr = res->start + I2S_TXDMA;
>> + dev->play_dma_data.dt.addr_width = bus_width;
>> + dev->play_dma_data.dt.chan_name = "TX";
>> + dev->play_dma_data.dt.fifo_size = fifo_depth *
>> + (fifo_width[max_size]) >> 8;
>> + dev->play_dma_data.dt.maxburst = 16;
>> + dw_i2s_dai->playback.channels_min = MIN_CHANNEL_NUM;
>> + dw_i2s_dai->playback.channels_max =
>> + 1 << (COMP1_TX_CHANNELS(comp1) + 1);
>> + dw_i2s_dai->playback.formats = formats[max_size];
>> + dw_i2s_dai->playback.rates = SNDRV_PCM_RATE_8000_192000;
>> + }
>> + if (COMP1_RX_ENABLED(comp1)) {
>> + dev_dbg(&pdev->dev, "record capable\n");
>> +
>> + dev->capability |= DWC_I2S_RECORD;
>> + max_size = COMP2_RX_WORDSIZE_0(comp2);
>> + dev->capture_dma_data.dt.addr = res->start + I2S_RXDMA;
>> + dev->capture_dma_data.dt.addr_width = bus_width;
>> + dev->capture_dma_data.dt.chan_name = "RX";
>> + dev->capture_dma_data.dt.fifo_size = fifo_depth *
>> + (fifo_width[max_size] >> 8);
>> + dev->capture_dma_data.dt.maxburst = 16;
>> + dw_i2s_dai->capture.channels_min = MIN_CHANNEL_NUM;
>> + dw_i2s_dai->capture.channels_max =
>> + 1 << (COMP1_RX_CHANNELS(comp1) + 1);
>> + dw_i2s_dai->capture.formats = formats[max_size];
>> + dw_i2s_dai->capture.rates = SNDRV_PCM_RATE_8000_192000;
>> + }
>> + }
>>
>> - dev->clk = clk_get(&pdev->dev, NULL);
>> - if (IS_ERR(dev->clk))
>> - return PTR_ERR(dev->clk);
>> + ret = clk_prepare(dev->clk);
>> + if (ret < 0)
>> + goto err_clk_put;
>>
>> ret = clk_enable(dev->clk);
>> if (ret < 0)
>> goto err_clk_put;
>>
>> - if (cap & DWC_I2S_PLAY) {
>> - dev_dbg(&pdev->dev, " designware: play supported\n");
>> - dw_i2s_dai->playback.channels_min = MIN_CHANNEL_NUM;
>> - dw_i2s_dai->playback.channels_max = pdata->channel;
>> - dw_i2s_dai->playback.formats = pdata->snd_fmts;
>> - dw_i2s_dai->playback.rates = pdata->snd_rates;
>> - }
>> -
>> - if (cap & DWC_I2S_RECORD) {
>> - dev_dbg(&pdev->dev, "designware: record supported\n");
>> - dw_i2s_dai->capture.channels_min = MIN_CHANNEL_NUM;
>> - dw_i2s_dai->capture.channels_max = pdata->channel;
>> - dw_i2s_dai->capture.formats = pdata->snd_fmts;
>> - dw_i2s_dai->capture.rates = pdata->snd_rates;
>> - }
>> -
>> dev->dev = &pdev->dev;
>> dev_set_drvdata(&pdev->dev, dev);
>> ret = snd_soc_register_component(&pdev->dev, &dw_i2s_component,
>> @@ -423,6 +542,15 @@ static int dw_i2s_probe(struct platform_device *pdev)
>> goto err_clk_disable;
>> }
>>
>> + if (!dev->using_pd) {
>> + ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
>> + if (ret) {
>> + dev_err(&pdev->dev,
>> + "Could not register PCM: %d\n", ret);
>> + return ret;
>> + }
>> + }
>> +
>> return 0;
>>
>> err_clk_disable:
>> @@ -443,12 +571,24 @@ static int dw_i2s_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id dw_i2s_of_match[] = {
>> + { .compatible = "snps,designware-i2s", },
>> + {},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, dw_i2s_of_match);
>> +#endif
>> +
>> static struct platform_driver dw_i2s_driver = {
>> .probe = dw_i2s_probe,
>> .remove = dw_i2s_remove,
>> .driver = {
>> .name = "designware-i2s",
>> .owner = THIS_MODULE,
>> +#ifdef CONFIG_OF
>> + .of_match_table = dw_i2s_of_match,
>> +#endif
>> },
>> };
>>
>> --
>> 1.7.1
>>
>
On Thu, Dec 04, 2014 at 10:55:55AM +0100, Arnd Bergmann wrote:
> On Wednesday 03 December 2014 20:19:05 Mark Brown wrote:
> > Not all the world uses DT, this is a DesignWare IP so it's likely to get
> > deployed widely - potentially on PCI cards and things like that.
> It's not really new though, and we are only seeing the second user.
> Maybe this one just isn't as common as a lot of the other designware IP?
It's relatively common with newer companies - it's like UARTs, any
company that's been around for a long time will have a homebrew IP
already but places that don't (or get fed up with their existing IP)
will tend to buy something in.
On Thursday 04 December 2014 11:07:46 Mark Brown wrote:
> On Thu, Dec 04, 2014 at 10:55:55AM +0100, Arnd Bergmann wrote:
> > On Wednesday 03 December 2014 20:19:05 Mark Brown wrote:
>
> > > Not all the world uses DT, this is a DesignWare IP so it's likely to get
> > > deployed widely - potentially on PCI cards and things like that.
>
> > It's not really new though, and we are only seeing the second user.
> > Maybe this one just isn't as common as a lot of the other designware IP?
>
> It's relatively common with newer companies - it's like UARTs, any
> company that's been around for a long time will have a homebrew IP
> already but places that don't (or get fed up with their existing IP)
> will tend to buy something in.
I see, but are any of the newer designs going to use platform_data?
My general feeling was that the only places that still depend on that
are legacy platforms and architectures.
Arnd
On Thu, Dec 04, 2014 at 12:17:15PM +0100, Arnd Bergmann wrote:
> On Thursday 04 December 2014 11:07:46 Mark Brown wrote:
> > It's relatively common with newer companies - it's like UARTs, any
> > company that's been around for a long time will have a homebrew IP
> > already but places that don't (or get fed up with their existing IP)
> > will tend to buy something in.
> I see, but are any of the newer designs going to use platform_data?
> My general feeling was that the only places that still depend on that
> are legacy platforms and architectures.
And x86 which does have things like PCI cards.