From: Andrew Jackson <[email protected]>
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 <[email protected]>
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 <[email protected]>
---
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,
};
--
1.7.1
From: Andrew Jackson <[email protected]>
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 <[email protected]>
---
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);
--
1.7.1
From: Andrew Jackson <[email protected]>
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 <[email protected]>
---
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);
--
1.7.1
From: Andrew Jackson <[email protected]>
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 <[email protected]>
---
.../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";
+ };
--
1.7.1
From: Andrew Jackson <[email protected]>
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 <[email protected]>
---
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
},
};
--
1.7.1
On Fri, Dec 19, 2014 at 04:18:05PM +0000, Andrew Jackson wrote:
> From: Andrew Jackson <[email protected]>
>
> 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.
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 Fri, Dec 19, 2014 at 04:18:07PM +0000, Andrew Jackson wrote:
> From: Andrew Jackson <[email protected]>
>
> 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.
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: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 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 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().
On Fri, Dec 19, 2014 at 04:18:06PM +0000, Andrew Jackson wrote:
> From: Andrew Jackson <[email protected]>
>
> 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.
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 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 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.