[alsa-devel] [PATCH v3 0/5] ASoC: dwc: Add device tree support to designware I2S
From: Andrew Jackson Andrew.Jackson@arm.com
This patch set extends the DesignWare I2S driver to provide device tree support and fixes a couple of small faults.
Changes v2->v3 + Drop applied patch + Flush FIFOs in prepare rather than hw_params [Lars-Peter Clausen]
Changes v1->v2 + Drop negative use count patch [Mark Brown] + Remove unnecessary debug print messages [Lars-Peter Clausen] + Rewrite iteration as for loop rather than do...while [Mark Brown] + Reorder patches to send fixes first [Mark Brown] + Simplify device tree code [Mark Brown] + Split device tree patch in two [Mark Brown] + Expand explanatory comment on channel configuration [Rajeev Kumar]
Arnd: I've not forgotten about updating the spear entries and will submit as a separate patch.
Andrew Jackson (5): ASoC: dwc: Ensure FIFOs are flushed to prevent channel swap ASoC: dwc: Iterate over all channels ASoC: dwc: Reorder code in preparation for DT support ASoC: dwc: Add devicetree support for Designware I2S ASoC: dwc: Add documentation for I2S DT
.../devicetree/bindings/sound/designware-i2s.txt | 32 +++ sound/soc/dwc/Kconfig | 1 + sound/soc/dwc/designware_i2s.c | 293 +++++++++++++++----- 3 files changed, 258 insertions(+), 68 deletions(-) create mode 100644 Documentation/devicetree/bindings/sound/designware-i2s.txt
From: Andrew Jackson Andrew.Jackson@arm.com
Flush the FIFOs when the stream is prepared for use. This avoids an inadvertent swapping of the left/right channels if the FIFOs are not empty at startup.
Signed-off-by: Andrew Jackson Andrew.Jackson@arm.com --- sound/soc/dwc/designware_i2s.c | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c index f81e747..5c13303 100644 --- a/sound/soc/dwc/designware_i2s.c +++ b/sound/soc/dwc/designware_i2s.c @@ -263,6 +263,19 @@ static void dw_i2s_shutdown(struct snd_pcm_substream *substream, snd_soc_dai_set_dma_data(dai, substream, NULL); }
+static int dw_i2s_prepare(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct dw_i2s_dev *dev = snd_soc_dai_get_drvdata(dai); + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + i2s_write_reg(dev->i2s_base, TXFFR, 1); + else + i2s_write_reg(dev->i2s_base, RXFFR, 1); + + return 0; +} + static int dw_i2s_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) { @@ -294,6 +307,7 @@ static struct snd_soc_dai_ops dw_i2s_dai_ops = { .startup = dw_i2s_startup, .shutdown = dw_i2s_shutdown, .hw_params = dw_i2s_hw_params, + .prepare = dw_i2s_prepare, .trigger = dw_i2s_trigger, };
On Fri, Dec 19, 2014 at 04:18:05PM +0000, Andrew Jackson wrote:
From: Andrew Jackson Andrew.Jackson@arm.com
Flush the FIFOs when the stream is prepared for use. This avoids an inadvertent swapping of the left/right channels if the FIFOs are not empty at startup.
Applied, thanks.
From: Andrew Jackson Andrew.Jackson@arm.com
The Designware core can be configured with up to four stereo channels. Each stereo channel is individually configured so, when the driver's hw_params call is made, each requested stereo channel has to be programmed.
Signed-off-by: Andrew Jackson Andrew.Jackson@arm.com --- sound/soc/dwc/designware_i2s.c | 35 ++++++++++++++++------------------- 1 files changed, 16 insertions(+), 19 deletions(-)
diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c index 5c13303..2ba1e2e 100644 --- a/sound/soc/dwc/designware_i2s.c +++ b/sound/soc/dwc/designware_i2s.c @@ -209,16 +209,9 @@ static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
switch (config->chan_nr) { case EIGHT_CHANNEL_SUPPORT: - ch_reg = 3; - break; case SIX_CHANNEL_SUPPORT: - ch_reg = 2; - break; case FOUR_CHANNEL_SUPPORT: - ch_reg = 1; - break; case TWO_CHANNEL_SUPPORT: - ch_reg = 0; break; default: dev_err(dev->dev, "channel not supported\n"); @@ -227,18 +220,22 @@ static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
i2s_disable_channels(dev, substream->stream);
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { - i2s_write_reg(dev->i2s_base, TCR(ch_reg), xfer_resolution); - i2s_write_reg(dev->i2s_base, TFCR(ch_reg), 0x02); - irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg)); - i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x30); - i2s_write_reg(dev->i2s_base, TER(ch_reg), 1); - } else { - i2s_write_reg(dev->i2s_base, RCR(ch_reg), xfer_resolution); - i2s_write_reg(dev->i2s_base, RFCR(ch_reg), 0x07); - irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg)); - i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x03); - i2s_write_reg(dev->i2s_base, RER(ch_reg), 1); + for (ch_reg = 0; ch_reg < (config->chan_nr / 2); ch_reg++) { + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + i2s_write_reg(dev->i2s_base, TCR(ch_reg), + xfer_resolution); + i2s_write_reg(dev->i2s_base, TFCR(ch_reg), 0x02); + irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg)); + i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x30); + i2s_write_reg(dev->i2s_base, TER(ch_reg), 1); + } else { + i2s_write_reg(dev->i2s_base, RCR(ch_reg), + xfer_resolution); + i2s_write_reg(dev->i2s_base, RFCR(ch_reg), 0x07); + irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg)); + i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x03); + i2s_write_reg(dev->i2s_base, RER(ch_reg), 1); + } }
i2s_write_reg(dev->i2s_base, CCR, ccr);
On Fri, Dec 19, 2014 at 04:18:06PM +0000, Andrew Jackson wrote:
The Designware core can be configured with up to four stereo channels. Each stereo channel is individually configured so, when the driver's hw_params call is made, each requested stereo channel has to be programmed.
This is quite unclear to someone who doesn't know the hardware, is this a bug fix or a new feature? It looks like it's a fix...
On 12/22/14 13:53, Mark Brown wrote:
On Fri, Dec 19, 2014 at 04:18:06PM +0000, Andrew Jackson wrote:
The Designware core can be configured with up to four stereo channels. Each stereo channel is individually configured so, when the driver's hw_params call is made, each requested stereo channel has to be programmed.
This is quite unclear to someone who doesn't know the hardware, is this a bug fix or a new feature? It looks like it's a fix...
It is a fix. In the Designware core, each stereo channel is configured individually. So, when hw_params is called to configure N channels, N/2 stereo channels need to be configured in the core. The existing code doesn't do this and will only configure the highest numbered channel.
Andrew
On Fri, Dec 19, 2014 at 04:18:06PM +0000, Andrew Jackson wrote:
From: Andrew Jackson Andrew.Jackson@arm.com
The Designware core can be configured with up to four stereo channels. Each stereo channel is individually configured so, when the driver's hw_params call is made, each requested stereo channel has to be programmed.
Applied, thanks.
From: Andrew Jackson Andrew.Jackson@arm.com
Move code that configures the DAI and DMA into a separate function. This reduces the size of the dw_i2s_probe function and will make it easier to add support for device tree to the driver.
Signed-off-by: Andrew Jackson Andrew.Jackson@arm.com --- sound/soc/dwc/designware_i2s.c | 73 +++++++++++++++++++++------------------ 1 files changed, 39 insertions(+), 34 deletions(-)
diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c index 2ba1e2e..06d3a34 100644 --- a/sound/soc/dwc/designware_i2s.c +++ b/sound/soc/dwc/designware_i2s.c @@ -335,13 +335,47 @@ static int dw_i2s_resume(struct snd_soc_dai *dai) #define dw_i2s_resume NULL #endif
+static void dw_configure_dai_by_pd(struct dw_i2s_dev *dev, + struct snd_soc_dai_driver *dw_i2s_dai, + struct resource *res, + const struct i2s_platform_data *pdata) +{ + /* Set DMA slaves info */ + + 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 (pdata->cap & DWC_I2S_PLAY) { + dev_dbg(dev->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 (pdata->cap & DWC_I2S_RECORD) { + dev_dbg(dev->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; + } +} + static int dw_i2s_probe(struct platform_device *pdev) { const struct i2s_platform_data *pdata = pdev->dev.platform_data; struct dw_i2s_dev *dev; struct resource *res; int ret; - unsigned int cap; struct snd_soc_dai_driver *dw_i2s_dai;
if (!pdata) { @@ -368,23 +402,11 @@ static int dw_i2s_probe(struct platform_device *pdev) if (IS_ERR(dev->i2s_base)) return PTR_ERR(dev->i2s_base);
- cap = pdata->cap; - dev->capability = cap; - dev->i2s_clk_cfg = pdata->i2s_clk_cfg; - - /* Set DMA slaves info */ - - 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; + dev->dev = &pdev->dev; + dw_configure_dai_by_pd(dev, dw_i2s_dai, res, pdata);
+ dev->capability = pdata->cap; + dev->i2s_clk_cfg = pdata->i2s_clk_cfg; dev->clk = clk_get(&pdev->dev, NULL); if (IS_ERR(dev->clk)) return PTR_ERR(dev->clk); @@ -393,23 +415,6 @@ static int dw_i2s_probe(struct platform_device *pdev) 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, dw_i2s_dai, 1);
On Fri, Dec 19, 2014 at 04:18:07PM +0000, Andrew Jackson wrote:
From: Andrew Jackson Andrew.Jackson@arm.com
Move code that configures the DAI and DMA into a separate function. This reduces the size of the dw_i2s_probe function and will make it easier to add support for device tree to the driver.
Applied, thanks.
From: Andrew Jackson Andrew.Jackson@arm.com
Allow the driver to be configured through a device tree rather than platform data. When using device-tree, read the I2S block's configuration from the relevant registers: this reduces the amount of information required in the device tree.
Signed-off-by: Andrew Jackson Andrew.Jackson@arm.com --- sound/soc/dwc/Kconfig | 1 + sound/soc/dwc/designware_i2s.c | 199 ++++++++++++++++++++++++++++++++++------ 2 files changed, 171 insertions(+), 29 deletions(-)
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 06d3a34..7905c52 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,9 +55,35 @@ #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; @@ -65,8 +92,8 @@ struct dw_i2s_dev { struct device *dev;
/* 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 +180,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)) @@ -242,13 +269,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->i2s_clk_cfg) { + 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; @@ -335,6 +367,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 void dw_configure_dai_by_pd(struct dw_i2s_dev *dev, struct snd_soc_dai_driver *dw_i2s_dai, struct resource *res, @@ -342,16 +399,16 @@ static void dw_configure_dai_by_pd(struct dw_i2s_dev *dev, { /* Set DMA slaves info */
- 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; + 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;
if (pdata->cap & DWC_I2S_PLAY) { dev_dbg(dev->dev, " designware: play supported\n"); @@ -370,6 +427,59 @@ static void dw_configure_dai_by_pd(struct dw_i2s_dev *dev, } }
+static void dw_configure_dai_by_dt(struct dw_i2s_dev *dev, + struct snd_soc_dai_driver *dw_i2s_dai, + struct resource *res) +{ + /* + * 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; + + /* + * All channels' hardware configuration assumed to be the same. + */ + if (COMP1_TX_ENABLED(comp1)) { + dev_dbg(dev->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(dev->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; + } +} + static int dw_i2s_probe(struct platform_device *pdev) { const struct i2s_platform_data *pdata = pdev->dev.platform_data; @@ -378,11 +488,6 @@ static int dw_i2s_probe(struct platform_device *pdev) int ret; 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"); @@ -403,13 +508,28 @@ static int dw_i2s_probe(struct platform_device *pdev) return PTR_ERR(dev->i2s_base);
dev->dev = &pdev->dev; - dw_configure_dai_by_pd(dev, dw_i2s_dai, res, pdata); + if (pdata) { + dw_configure_dai_by_pd(dev, dw_i2s_dai, res, pdata); + + dev->capability = pdata->cap; + dev->i2s_clk_cfg = pdata->i2s_clk_cfg; + if (!dev->i2s_clk_cfg) { + dev_err(&pdev->dev, "no clock configure method\n"); + return -ENODEV; + }
- dev->capability = pdata->cap; - dev->i2s_clk_cfg = pdata->i2s_clk_cfg; - dev->clk = clk_get(&pdev->dev, NULL); + dev->clk = clk_get(&pdev->dev, NULL); + } else { + dw_configure_dai_by_dt(dev, dw_i2s_dai, res); + + dev->clk = devm_clk_get(&pdev->dev, "i2sclk"); + } if (IS_ERR(dev->clk)) - return PTR_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) @@ -423,6 +543,15 @@ static int dw_i2s_probe(struct platform_device *pdev) goto err_clk_disable; }
+ if (!pdata) { + 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,11 +572,23 @@ 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", +#ifdef CONFIG_OF + .of_match_table = dw_i2s_of_match, +#endif }, };
On Fri, Dec 19, 2014 at 04:18:08PM +0000, Andrew Jackson wrote:
+union snd_dma_data {
- struct i2s_dma_data pd;
- struct snd_dmaengine_dai_dma_data dt;
+};
This is a driver local union with a very generic name, it seems likely that this will collide in future causing build breaks
- 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);
This is ignoring errors in clk_set_rate().
+/* 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
+};
- u32 bus_width = bus_widths[COMP1_APB_DATA_WIDTH(comp1)];
- u32 fifo_depth = 1 << (1 + COMP1_FIFO_DEPTH_GLOBAL(comp1));
- u32 max_size;
I'd feel a lot more comfortable if there were bounds checking on these array indexes, especially since the arrays aren't explicitly sized and instead just have the number of elements that is (hopefully) safe with no comments or anything. As things stand this is all using really fraigle idioms, this could easily be broken if someone is updating the driver for new IP features or even just cleaning up the code.
- dev->i2s_clk_cfg = pdata->i2s_clk_cfg;
- dev->clk = clk_get(&pdev->dev, NULL);
dev->clk = clk_get(&pdev->dev, NULL);- } else {
dw_configure_dai_by_dt(dev, dw_i2s_dai, res);dev->clk = devm_clk_get(&pdev->dev, "i2sclk");- }
This changes from clk_get() to devm_clk_get() but I'm not seeing anything that removes clk_put() calls.
+#ifdef CONFIG_OF
.of_match_table = dw_i2s_of_match,+#endif
of_match_ptr().
On 12/22/14 14:10, Mark Brown wrote:
On Fri, Dec 19, 2014 at 04:18:08PM +0000, Andrew Jackson wrote:
+union snd_dma_data {
- struct i2s_dma_data pd;
- struct snd_dmaengine_dai_dma_data dt;
+};
This is a driver local union with a very generic name, it seems likely that this will collide in future causing build breaks
I'll change it.
- 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);This is ignoring errors in clk_set_rate().
+/* 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
+};
- u32 bus_width = bus_widths[COMP1_APB_DATA_WIDTH(comp1)];
- u32 fifo_depth = 1 << (1 + COMP1_FIFO_DEPTH_GLOBAL(comp1));
- u32 max_size;
I'd feel a lot more comfortable if there were bounds checking on these array indexes, especially since the arrays aren't explicitly sized and instead just have the number of elements that is (hopefully) safe with no comments or anything. As things stand this is all using really fraigle idioms, this could easily be broken if someone is updating the driver for new IP features or even just cleaning up the code.
I will add robustness.
- dev->i2s_clk_cfg = pdata->i2s_clk_cfg;
- dev->clk = clk_get(&pdev->dev, NULL);
dev->clk = clk_get(&pdev->dev, NULL);- } else {
dw_configure_dai_by_dt(dev, dw_i2s_dai, res);dev->clk = devm_clk_get(&pdev->dev, "i2sclk");- }
This changes from clk_get() to devm_clk_get() but I'm not seeing anything that removes clk_put() calls.
+#ifdef CONFIG_OF
.of_match_table = dw_i2s_of_match,+#endif
of_match_ptr().
Thanks for all the comments.
Andrew
On Fri, Dec 19, 2014 at 04:18:08PM +0000, Andrew Jackson wrote:
if (IS_ERR(dev->clk))
return PTR_ERR(dev->clk);
return PTR_ERR(dev->clk);- ret = clk_prepare(dev->clk);
- if (ret < 0)
goto err_clk_put;
This isn't adding device tree support, it's adding use of clk_prepare() which is a separate change and is normally better done by converting to use clk_prepare_enable().
From: Andrew Jackson Andrew.Jackson@arm.com
Add documentation for Designware I2S hardware block. The block requires two clocks (one for audio sampling, the other for APB) and DMA channels for receive and transmit.
Signed-off-by: Andrew Jackson Andrew.Jackson@arm.com --- .../devicetree/bindings/sound/designware-i2s.txt | 32 ++++++++++++++++++++ 1 files changed, 32 insertions(+), 0 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"; + };
On Fri, Dec 19, 2014 at 04:18:09PM +0000, Andrew Jackson wrote:
Add documentation for Designware I2S hardware block. The block requires two clocks (one for audio sampling, the other for APB) and DMA channels for receive and transmit.
You should generally include the binding before the code to parse it, both because the binding is required in order to tell if the code is doing the right thing and also because people will often not even look at code with a missing binding.
- 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.
This is a name based lookup of clocks but the code doesn't use apb_pclk at all; it needs to or the binding needs to say that apb_pclk must be the first listed clock (which would not be good).
- 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";- };
This omits a lot of configurability that is in platform data and replaces it by reading back the parameters from the hardware. If this is a viable approach to that configuration you should do this for both platform data and device tree rather than only device tree. The point with keeping platform data is that it's not good to make the device DT only, improving the usability of platform data in a way that happens to also make the DT case easier is totally fine. If we can determine how the IP is configured from the hardware that's both less work and more robust no matter how the device is instantiated.
On 12/22/14 14:26, Mark Brown wrote:
On Fri, Dec 19, 2014 at 04:18:09PM +0000, Andrew Jackson wrote:
Add documentation for Designware I2S hardware block. The block requires two clocks (one for audio sampling, the other for APB) and DMA channels for receive and transmit.
You should generally include the binding before the code to parse it, both because the binding is required in order to tell if the code is doing the right thing and also because people will often not even look at code with a missing binding.
Fair enough: I'll reorder the (remaining) patches.
- 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.
This is a name based lookup of clocks but the code doesn't use apb_pclk at all; it needs to or the binding needs to say that apb_pclk must be the first listed clock (which would not be good).
I can remove apb_pclk: I was modelling the device tree entry on various PLxxx examples (c.f. amba-pl011) which also reference an AMBA clock but don't use it. (The effect being to document what clock the block is driven by.)
- 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";- };
This omits a lot of configurability that is in platform data and replaces it by reading back the parameters from the hardware. If this is a viable approach to that configuration you should do this for both platform data and device tree rather than only device tree. The point with keeping platform data is that it's not good to make the device DT only, improving the usability of platform data in a way that happens to also make the DT case easier is totally fine. If we can determine how the IP is configured from the hardware that's both less work and more robust no matter how the device is instantiated.
I agree. I didn't do it like this originally because it wasn't clear whether or not the original driver catered for some custom IP and I wanted to ensure that I didn't break the existing driver. I'm happy to switch both platform data and device tree to reading their parameters from the hardware.
Andrew
On Mon, Dec 22, 2014 at 03:51:38PM +0000, Andrew Jackson wrote:
On 12/22/14 14:26, Mark Brown wrote:
This is a name based lookup of clocks but the code doesn't use apb_pclk at all; it needs to or the binding needs to say that apb_pclk must be the first listed clock (which would not be good).
I can remove apb_pclk: I was modelling the device tree entry on various PLxxx examples (c.f. amba-pl011) which also reference an AMBA clock but don't use it. (The effect being to document what clock the block is driven by.)
With the AMBA devices the AMBA core manages the bus clock for the drivers IIRC.
participants (2)
-
Andrew Jackson -
Mark Brown