[PATCH 0/4 v7] Add I2S audio support for ARC AXS10x boards

[PATCH 0/4 v7] Add I2S audio support for ARC AXS10x boards

[Jose Abreu]

ARC AXS10x platforms consist of a mainboard with several peripherals.
One of those peripherals is an HDMI output port controlled by the ADV7511
transmitter.

This patch set adds I2S audio for the AXS10x platform.

NOTE:
Although the mainline I2S driver uses ALSA DMA engine, this controller
can be built without DMA support so it was necessary to add this
custom platform driver so that HDMI audio works in AXS boards.

Changes v6 -> v7:
* Discard the use of memcpy
* Report IRQ_HANDLED only when there is an IRQ
* Use interrupts to check if PIO mode is in use
* Unmask interrupts only when in PIO mode
* Remove empty functions

Changes v5 -> v6:
* Use SNDRV_DMA_TYPE_CONTINUOUS

Changes v4 -> v5:
* Resolve undefined references when compiling as module
* Dropped adv7511 audio patches
* Use DMA properties in I2S to check which mode to use: PIO or DMA (as suggested by Lars-Peter Clausen)

Changes v3 -> v4:
* Reintroduced custom PCM driver (see note below)
* Use DT boolean to switch between ALSA DMA engine PCM or custom PCM
* Use fifo depth to program I2S FCR
* Update I2S documentation

Changes v2 -> v3:
* Removed pll_config functions (as suggested by Alexey Brodkin)
* Removed HDMI start at adv7511_core (as suggested by Archit Taneja)
* Use NOP functions for adv7511_audio (as suggested by Archit Taneja)
* Added adv7511_audio_exit() function (as suggested by Archit Taneja)
* Moved adv7511 to its own folder (as suggested by Archit Taneja)
* Separated file rename of adv7511_core (as suggested by Emil Velikov)
* Compile adv7511 as module if ALSA SoC is compiled as module
* Load adv7511 audio only if declared in device tree (as suggested by Laurent Pinchart)
* Dropped custom platform driver, using now ALSA DMA engine
* Dropped IRQ handler for I2S

Changes v1 -> v2:
* DT bindings moved to separate patch (as suggested by Alexey Brodkin)
* Removed defconfigs entries (as suggested by Alexey Brodkin)


Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: linux-snps-arc@lists.infradead.org
Cc: alsa-devel@alsa-project.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Jose Abreu (4):
  ASoC: dwc: Add helper functions to disable/enable irqs
  ASoC: dwc: Do not use devm_clk_get() if using platform data
  ASoC: dwc: Add PIO PCM extension
  ASoC: dwc: Add irq parameter to DOCUMENTATION

 .../devicetree/bindings/sound/designware-i2s.txt   |   4 +
 sound/soc/dwc/Kconfig                              |   9 +
 sound/soc/dwc/Makefile                             |   5 +-
 sound/soc/dwc/designware_i2s.c                     | 229 ++++++++++-----------
 sound/soc/dwc/designware_pcm.c                     | 220 ++++++++++++++++++++
 sound/soc/dwc/local.h                              | 122 +++++++++++
 6 files changed, 467 insertions(+), 122 deletions(-)
 create mode 100644 sound/soc/dwc/designware_pcm.c
 create mode 100644 sound/soc/dwc/local.h

-- 
1.9.1

[PATCH 1/4 v7] ASoC: dwc: Add helper functions to disable/enable irqs

[Jose Abreu]

Helper functions to disable and enable the I2S interrupts were
added. Only the interrupts of the used channels are enabled.

Also, there is no need to enable irqs at dw_i2s_config(), they
are already enabled at startup.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: linux-snps-arc@lists.infradead.org
Cc: alsa-devel@alsa-project.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---

This patch was only introduced in v7.

 sound/soc/dwc/designware_i2s.c | 68 +++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 27 deletions(-)

diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c
index 0db69b7..4c4f0dc 100644
--- a/sound/soc/dwc/designware_i2s.c
+++ b/sound/soc/dwc/designware_i2s.c
@@ -145,26 +145,54 @@ static inline void i2s_clear_irqs(struct dw_i2s_dev *dev, u32 stream)
 	}
 }
 
-static void i2s_start(struct dw_i2s_dev *dev,
-		      struct snd_pcm_substream *substream)
+static inline void i2s_disable_irqs(struct dw_i2s_dev *dev, u32 stream,
+				    int chan_nr)
 {
-	struct i2s_clk_config_data *config = &dev->config;
 	u32 i, irq;
-	i2s_write_reg(dev->i2s_base, IER, 1);
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		for (i = 0; i < (config->chan_nr / 2); i++) {
+	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		for (i = 0; i < (chan_nr / 2); i++) {
+			irq = i2s_read_reg(dev->i2s_base, IMR(i));
+			i2s_write_reg(dev->i2s_base, IMR(i), irq | 0x30);
+		}
+	} else {
+		for (i = 0; i < (chan_nr / 2); i++) {
+			irq = i2s_read_reg(dev->i2s_base, IMR(i));
+			i2s_write_reg(dev->i2s_base, IMR(i), irq | 0x03);
+		}
+	}
+}
+
+static inline void i2s_enable_irqs(struct dw_i2s_dev *dev, u32 stream,
+				   int chan_nr)
+{
+	u32 i, irq;
+
+	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		for (i = 0; i < (chan_nr / 2); i++) {
 			irq = i2s_read_reg(dev->i2s_base, IMR(i));
 			i2s_write_reg(dev->i2s_base, IMR(i), irq & ~0x30);
 		}
-		i2s_write_reg(dev->i2s_base, ITER, 1);
 	} else {
-		for (i = 0; i < (config->chan_nr / 2); i++) {
+		for (i = 0; i < (chan_nr / 2); i++) {
 			irq = i2s_read_reg(dev->i2s_base, IMR(i));
 			i2s_write_reg(dev->i2s_base, IMR(i), irq & ~0x03);
 		}
-		i2s_write_reg(dev->i2s_base, IRER, 1);
 	}
+}
+
+static void i2s_start(struct dw_i2s_dev *dev,
+		      struct snd_pcm_substream *substream)
+{
+	struct i2s_clk_config_data *config = &dev->config;
+
+	i2s_write_reg(dev->i2s_base, IER, 1);
+	i2s_enable_irqs(dev, substream->stream, config->chan_nr);
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		i2s_write_reg(dev->i2s_base, ITER, 1);
+	else
+		i2s_write_reg(dev->i2s_base, IRER, 1);
 
 	i2s_write_reg(dev->i2s_base, CER, 1);
 }
@@ -172,24 +200,14 @@ static void i2s_start(struct dw_i2s_dev *dev,
 static void i2s_stop(struct dw_i2s_dev *dev,
 		struct snd_pcm_substream *substream)
 {
-	u32 i = 0, irq;
 
 	i2s_clear_irqs(dev, substream->stream);
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 		i2s_write_reg(dev->i2s_base, ITER, 0);
-
-		for (i = 0; i < 4; i++) {
-			irq = i2s_read_reg(dev->i2s_base, IMR(i));
-			i2s_write_reg(dev->i2s_base, IMR(i), irq | 0x30);
-		}
-	} else {
+	else
 		i2s_write_reg(dev->i2s_base, IRER, 0);
 
-		for (i = 0; i < 4; i++) {
-			irq = i2s_read_reg(dev->i2s_base, IMR(i));
-			i2s_write_reg(dev->i2s_base, IMR(i), irq | 0x03);
-		}
-	}
+	i2s_disable_irqs(dev, substream->stream, 8);
 
 	if (!dev->active) {
 		i2s_write_reg(dev->i2s_base, CER, 0);
@@ -223,7 +241,7 @@ static int dw_i2s_startup(struct snd_pcm_substream *substream,
 
 static void dw_i2s_config(struct dw_i2s_dev *dev, int stream)
 {
-	u32 ch_reg, irq;
+	u32 ch_reg;
 	struct i2s_clk_config_data *config = &dev->config;
 
 
@@ -235,16 +253,12 @@ static void dw_i2s_config(struct dw_i2s_dev *dev, int stream)
 				      dev->xfer_resolution);
 			i2s_write_reg(dev->i2s_base, TFCR(ch_reg),
 				      dev->fifo_th - 1);
-			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),
 				      dev->xfer_resolution);
 			i2s_write_reg(dev->i2s_base, RFCR(ch_reg),
 				      dev->fifo_th - 1);
-			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);
 		}
 
-- 
1.9.1

Applied "ASoC: dwc: Add helper functions to disable/enable irqs" to the asoc tree

[Mark Brown]

The patch

   ASoC: dwc: Add helper functions to disable/enable irqs

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From df4e7be1832a1ca9fccfb2ed8771a17da6f7fa57 Mon Sep 17 00:00:00 2001
From: Jose Abreu <Jose.Abreu@synopsys.com>
Date: Mon, 23 May 2016 11:02:22 +0100
Subject: [PATCH] ASoC: dwc: Add helper functions to disable/enable irqs

Helper functions to disable and enable the I2S interrupts were
added. Only the interrupts of the used channels are enabled.

Also, there is no need to enable irqs at dw_i2s_config(), they
are already enabled at startup.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: linux-snps-arc@lists.infradead.org
Cc: alsa-devel@alsa-project.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/dwc/designware_i2s.c | 68 +++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 27 deletions(-)

diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c
index 0db69b7e9617..4c4f0dc24f10 100644
--- a/sound/soc/dwc/designware_i2s.c
+++ b/sound/soc/dwc/designware_i2s.c
@@ -145,26 +145,54 @@ static inline void i2s_clear_irqs(struct dw_i2s_dev *dev, u32 stream)
 	}
 }
 
-static void i2s_start(struct dw_i2s_dev *dev,
-		      struct snd_pcm_substream *substream)
+static inline void i2s_disable_irqs(struct dw_i2s_dev *dev, u32 stream,
+				    int chan_nr)
 {
-	struct i2s_clk_config_data *config = &dev->config;
 	u32 i, irq;
-	i2s_write_reg(dev->i2s_base, IER, 1);
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		for (i = 0; i < (config->chan_nr / 2); i++) {
+	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		for (i = 0; i < (chan_nr / 2); i++) {
+			irq = i2s_read_reg(dev->i2s_base, IMR(i));
+			i2s_write_reg(dev->i2s_base, IMR(i), irq | 0x30);
+		}
+	} else {
+		for (i = 0; i < (chan_nr / 2); i++) {
+			irq = i2s_read_reg(dev->i2s_base, IMR(i));
+			i2s_write_reg(dev->i2s_base, IMR(i), irq | 0x03);
+		}
+	}
+}
+
+static inline void i2s_enable_irqs(struct dw_i2s_dev *dev, u32 stream,
+				   int chan_nr)
+{
+	u32 i, irq;
+
+	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		for (i = 0; i < (chan_nr / 2); i++) {
 			irq = i2s_read_reg(dev->i2s_base, IMR(i));
 			i2s_write_reg(dev->i2s_base, IMR(i), irq & ~0x30);
 		}
-		i2s_write_reg(dev->i2s_base, ITER, 1);
 	} else {
-		for (i = 0; i < (config->chan_nr / 2); i++) {
+		for (i = 0; i < (chan_nr / 2); i++) {
 			irq = i2s_read_reg(dev->i2s_base, IMR(i));
 			i2s_write_reg(dev->i2s_base, IMR(i), irq & ~0x03);
 		}
-		i2s_write_reg(dev->i2s_base, IRER, 1);
 	}
+}
+
+static void i2s_start(struct dw_i2s_dev *dev,
+		      struct snd_pcm_substream *substream)
+{
+	struct i2s_clk_config_data *config = &dev->config;
+
+	i2s_write_reg(dev->i2s_base, IER, 1);
+	i2s_enable_irqs(dev, substream->stream, config->chan_nr);
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		i2s_write_reg(dev->i2s_base, ITER, 1);
+	else
+		i2s_write_reg(dev->i2s_base, IRER, 1);
 
 	i2s_write_reg(dev->i2s_base, CER, 1);
 }
@@ -172,24 +200,14 @@ static void i2s_start(struct dw_i2s_dev *dev,
 static void i2s_stop(struct dw_i2s_dev *dev,
 		struct snd_pcm_substream *substream)
 {
-	u32 i = 0, irq;
 
 	i2s_clear_irqs(dev, substream->stream);
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 		i2s_write_reg(dev->i2s_base, ITER, 0);
-
-		for (i = 0; i < 4; i++) {
-			irq = i2s_read_reg(dev->i2s_base, IMR(i));
-			i2s_write_reg(dev->i2s_base, IMR(i), irq | 0x30);
-		}
-	} else {
+	else
 		i2s_write_reg(dev->i2s_base, IRER, 0);
 
-		for (i = 0; i < 4; i++) {
-			irq = i2s_read_reg(dev->i2s_base, IMR(i));
-			i2s_write_reg(dev->i2s_base, IMR(i), irq | 0x03);
-		}
-	}
+	i2s_disable_irqs(dev, substream->stream, 8);
 
 	if (!dev->active) {
 		i2s_write_reg(dev->i2s_base, CER, 0);
@@ -223,7 +241,7 @@ static int dw_i2s_startup(struct snd_pcm_substream *substream,
 
 static void dw_i2s_config(struct dw_i2s_dev *dev, int stream)
 {
-	u32 ch_reg, irq;
+	u32 ch_reg;
 	struct i2s_clk_config_data *config = &dev->config;
 
 
@@ -235,16 +253,12 @@ static void dw_i2s_config(struct dw_i2s_dev *dev, int stream)
 				      dev->xfer_resolution);
 			i2s_write_reg(dev->i2s_base, TFCR(ch_reg),
 				      dev->fifo_th - 1);
-			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),
 				      dev->xfer_resolution);
 			i2s_write_reg(dev->i2s_base, RFCR(ch_reg),
 				      dev->fifo_th - 1);
-			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);
 		}
 
-- 
2.8.1

[PATCH 2/4 v7] ASoC: dwc: Do not use devm_clk_get() if using platform data

[Jose Abreu]

When using platform data the devm_clk_get() function is
called causing a probe failure if the clock is not
declared. As we can pass the clock handler by platform
data call only devm_clk_get() when platform data is not
used.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: linux-snps-arc@lists.infradead.org
Cc: alsa-devel@alsa-project.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---

This patch was only introduced in v7.

 sound/soc/dwc/designware_i2s.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c
index 4c4f0dc..a97be8e 100644
--- a/sound/soc/dwc/designware_i2s.c
+++ b/sound/soc/dwc/designware_i2s.c
@@ -690,15 +690,16 @@ static int dw_i2s_probe(struct platform_device *pdev)
 				dev_err(&pdev->dev, "no clock configure method\n");
 				return -ENODEV;
 			}
-		}
-		dev->clk = devm_clk_get(&pdev->dev, clk_id);
+		} else {
+			dev->clk = devm_clk_get(&pdev->dev, clk_id);
 
-		if (IS_ERR(dev->clk))
-			return PTR_ERR(dev->clk);
+			if (IS_ERR(dev->clk))
+				return PTR_ERR(dev->clk);
 
-		ret = clk_prepare_enable(dev->clk);
-		if (ret < 0)
-			return ret;
+			ret = clk_prepare_enable(dev->clk);
+			if (ret < 0)
+				return ret;
+		}
 	}
 
 	dev_set_drvdata(&pdev->dev, dev);
-- 
1.9.1

Re: [PATCH 2/4 v7] ASoC: dwc: Do not use devm_clk_get() if using platform data

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 640 bytes --]

On Mon, May 23, 2016 at 11:02:23AM +0100, Jose Abreu wrote:

> When using platform data the devm_clk_get() function is
> called causing a probe failure if the clock is not
> declared. As we can pass the clock handler by platform
> data call only devm_clk_get() when platform data is not
> used.

No, this is broken - if the device needs a clock the device needs a
clock and clock names should be static rather than passed in via
platform data (indeed NULL is a perfectly valid clock name).  The system
integation should map in a clock as needed, if it's just a fixed crystal
or something then just register a fixed clock and connect it up.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 3/4 v7] ASoC: dwc: Add PIO PCM extension

[Jose Abreu]

A PCM extension was added to I2S driver so that audio
samples are transferred using PIO mode.

The PCM supports two channels @ 16 or 32 bits with rates
32k, 44.1k and 48k.

Although the mainline I2S driver uses ALSA DMA engine the
I2S controller can be built without DMA support, therefore
this is the reason why this extension was added.

The selection between the use of DMA engine or PIO mode
is detected by declaring or not the interrupt parameters
in the DT and using Kconfig.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: linux-snps-arc@lists.infradead.org
Cc: alsa-devel@alsa-project.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---

Changes v6 -> v7:
* Discard the use of memcpy
* Report IRQ_HANDLED only when there is an IRQ
* Use interrupts to check if PIO mode is in use
* Unmask interrupts only when in PIO mode
* Remove empty functions

Changes v5 -> v6:
* Use SNDRV_DMA_TYPE_CONTINUOUS

Changes v4 -> v5:
* Resolve undefined references when compiling as module
* Use DMA properties in I2S to check which mode to use: PIO or DMA (as suggested by Lars-Peter Clausen)

Changes v3 -> v4:
* Reintroduced custom PCM driver
* Use DT boolean to switch between ALSA DMA engine PCM or custom PCM

Changes v2 -> v3:
* Removed pll_config functions (as suggested by Alexey Brodkin)
* Dropped custom platform driver, using now ALSA DMA engine
* Dropped IRQ handler for I2S

No changes v1 -> v2.

 sound/soc/dwc/Kconfig          |   9 ++
 sound/soc/dwc/Makefile         |   5 +-
 sound/soc/dwc/designware_i2s.c | 148 ++++++++++++---------------
 sound/soc/dwc/designware_pcm.c | 220 +++++++++++++++++++++++++++++++++++++++++
 sound/soc/dwc/local.h          | 122 +++++++++++++++++++++++
 5 files changed, 415 insertions(+), 89 deletions(-)
 create mode 100644 sound/soc/dwc/designware_pcm.c
 create mode 100644 sound/soc/dwc/local.h

diff --git a/sound/soc/dwc/Kconfig b/sound/soc/dwc/Kconfig
index d50e085..c6fd95f 100644
--- a/sound/soc/dwc/Kconfig
+++ b/sound/soc/dwc/Kconfig
@@ -7,4 +7,13 @@ config SND_DESIGNWARE_I2S
 	 Synopsys desigwnware I2S device. The device supports upto
 	 maximum of 8 channels each for play and record.
 
+config SND_DESIGNWARE_PCM
+	bool "PCM PIO extension for I2S driver"
+	depends on SND_DESIGNWARE_I2S
+	help
+	 Say Y or N if you want to add a custom ALSA extension that registers
+	 a PCM and uses PIO to transfer data.
+
+	 This functionality is specially suited for I2S devices that don't have
+	 DMA support.
 
diff --git a/sound/soc/dwc/Makefile b/sound/soc/dwc/Makefile
index 319371f..11ea966 100644
--- a/sound/soc/dwc/Makefile
+++ b/sound/soc/dwc/Makefile
@@ -1,3 +1,4 @@
 # SYNOPSYS Platform Support
-obj-$(CONFIG_SND_DESIGNWARE_I2S) += designware_i2s.o
-
+obj-$(CONFIG_SND_DESIGNWARE_I2S) += dwc_i2s.o
+dwc_i2s-y := designware_i2s.o
+dwc_i2s-$(CONFIG_SND_DESIGNWARE_PCM) += designware_pcm.o
diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c
index a97be8e..d4c3811 100644
--- a/sound/soc/dwc/designware_i2s.c
+++ b/sound/soc/dwc/designware_i2s.c
@@ -24,90 +24,7 @@
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
 #include <sound/dmaengine_pcm.h>
-
-/* common register for all channel */
-#define IER		0x000
-#define IRER		0x004
-#define ITER		0x008
-#define CER		0x00C
-#define CCR		0x010
-#define RXFFR		0x014
-#define TXFFR		0x018
-
-/* I2STxRxRegisters for all channels */
-#define LRBR_LTHR(x)	(0x40 * x + 0x020)
-#define RRBR_RTHR(x)	(0x40 * x + 0x024)
-#define RER(x)		(0x40 * x + 0x028)
-#define TER(x)		(0x40 * x + 0x02C)
-#define RCR(x)		(0x40 * x + 0x030)
-#define TCR(x)		(0x40 * x + 0x034)
-#define ISR(x)		(0x40 * x + 0x038)
-#define IMR(x)		(0x40 * x + 0x03C)
-#define ROR(x)		(0x40 * x + 0x040)
-#define TOR(x)		(0x40 * x + 0x044)
-#define RFCR(x)		(0x40 * x + 0x048)
-#define TFCR(x)		(0x40 * x + 0x04C)
-#define RFF(x)		(0x40 * x + 0x050)
-#define TFF(x)		(0x40 * x + 0x054)
-
-/* I2SCOMPRegisters */
-#define I2S_COMP_PARAM_2	0x01F0
-#define I2S_COMP_PARAM_1	0x01F4
-#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)
-
-/* Number of entries in WORDSIZE and DATA_WIDTH parameter registers */
-#define	COMP_MAX_WORDSIZE	(1 << 3)
-#define	COMP_MAX_DATA_WIDTH	(1 << 2)
-
-#define MAX_CHANNEL_NUM		8
-#define MIN_CHANNEL_NUM		2
-
-union dw_i2s_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;
-	unsigned int quirks;
-	unsigned int i2s_reg_comp1;
-	unsigned int i2s_reg_comp2;
-	struct device *dev;
-	u32 ccr;
-	u32 xfer_resolution;
-	u32 fifo_th;
-
-	/* data related to DMA transfers b/w i2s and DMAC */
-	union dw_i2s_snd_dma_data play_dma_data;
-	union dw_i2s_snd_dma_data capture_dma_data;
-	struct i2s_clk_config_data config;
-	int (*i2s_clk_cfg)(struct i2s_clk_config_data *config);
-};
+#include "local.h"
 
 static inline void i2s_write_reg(void __iomem *io_base, int reg, u32 val)
 {
@@ -181,13 +98,49 @@ static inline void i2s_enable_irqs(struct dw_i2s_dev *dev, u32 stream,
 	}
 }
 
+static irqreturn_t i2s_irq_handler(int irq, void *dev_id)
+{
+	struct dw_i2s_dev *dev = dev_id;
+	bool irq_valid = false;
+	u32 isr[4];
+	int i;
+
+	for (i = 0; i < 4; i++)
+		isr[i] = i2s_read_reg(dev->i2s_base, ISR(i));
+
+	i2s_clear_irqs(dev, SNDRV_PCM_STREAM_PLAYBACK);
+	i2s_clear_irqs(dev, SNDRV_PCM_STREAM_CAPTURE);
+
+	for (i = 0; i < 4; i++) {
+		if (isr[i] & 0x33)
+			irq_valid = true;
+
+		/*
+		 * Check if TX fifo is empty. If empty fill FIFO with samples
+		 * NOTE: Only two channels supported
+		 */
+		if ((isr[i] & 0x10) && (i == 0))
+			dw_pcm_push_tx(dev);
+
+		/* Error Handling */
+		if (isr[i] & 0x20)
+			dev_err(dev->dev, "TX overrun (ch_id=%d)\n", i);
+		if (isr[i] & 0x02)
+			dev_err(dev->dev, "RX overrun (ch_id=%d)\n", i);
+	}
+
+	return irq_valid ? IRQ_HANDLED : IRQ_NONE;
+}
+
 static void i2s_start(struct dw_i2s_dev *dev,
 		      struct snd_pcm_substream *substream)
 {
 	struct i2s_clk_config_data *config = &dev->config;
 
 	i2s_write_reg(dev->i2s_base, IER, 1);
-	i2s_enable_irqs(dev, substream->stream, config->chan_nr);
+
+	if (dev->use_pio)
+		i2s_enable_irqs(dev, substream->stream, config->chan_nr);
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 		i2s_write_reg(dev->i2s_base, ITER, 1);
@@ -640,7 +593,7 @@ 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;
+	int ret, irq;
 	struct snd_soc_dai_driver *dw_i2s_dai;
 	const char *clk_id;
 
@@ -665,6 +618,19 @@ static int dw_i2s_probe(struct platform_device *pdev)
 
 	dev->dev = &pdev->dev;
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq >= 0) {
+		dev_dbg(&pdev->dev, "using PIO mode\n");
+		dev->use_pio = true;
+
+		ret = devm_request_irq(&pdev->dev, irq, i2s_irq_handler, 0,
+				pdev->name, dev);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to request irq\n");
+			return ret;
+		}
+	}
+
 	dev->i2s_reg_comp1 = I2S_COMP_PARAM_1;
 	dev->i2s_reg_comp2 = I2S_COMP_PARAM_2;
 	if (pdata) {
@@ -710,14 +676,22 @@ static int dw_i2s_probe(struct platform_device *pdev)
 		goto err_clk_disable;
 	}
 
-	if (!pdata) {
+	if (!pdata && !dev->use_pio) {
 		ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
 		if (ret) {
 			dev_err(&pdev->dev,
 				"Could not register PCM: %d\n", ret);
 			goto err_clk_disable;
 		}
+	} else if (dev->use_pio) {
+		ret = dw_pcm_register(pdev);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"Could not register PIO PCM: %d\n", ret);
+			goto err_clk_disable;
+		}
 	}
+
 	pm_runtime_enable(&pdev->dev);
 	return 0;
 
diff --git a/sound/soc/dwc/designware_pcm.c b/sound/soc/dwc/designware_pcm.c
new file mode 100644
index 0000000..d995b50
--- /dev/null
+++ b/sound/soc/dwc/designware_pcm.c
@@ -0,0 +1,220 @@
+/*
+ * ALSA SoC Synopsys PIO PCM for I2S driver
+ *
+ * sound/soc/dwc/designware_pcm.c
+ *
+ * Copyright (C) 2016 Synopsys
+ * Jose Abreu <joabreu@synopsys.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/io.h>
+#include <linux/rcupdate.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include "local.h"
+
+#define BUFFER_BYTES_MAX	(3 * 2 * 8 * PERIOD_BYTES_MIN)
+#define PERIOD_BYTES_MIN	4096
+#define PERIODS_MIN		2
+
+#define dw_pcm_tx_fn(sample_bits) \
+static unsigned int dw_pcm_tx_##sample_bits(struct dw_i2s_dev *dev, \
+		struct snd_pcm_runtime *runtime, unsigned int tx_ptr, \
+		bool *period_elapsed) \
+{ \
+	const u##sample_bits (*p)[2] = (void *)runtime->dma_area; \
+	unsigned int period_pos = tx_ptr % runtime->period_size; \
+	int i; \
+\
+	for (i = 0; i < dev->fifo_th; i++) { \
+		iowrite32(p[tx_ptr][0], dev->i2s_base + LRBR_LTHR(0)); \
+		iowrite32(p[tx_ptr][1], dev->i2s_base + RRBR_RTHR(0)); \
+		period_pos++; \
+		if (++tx_ptr >= runtime->buffer_size) \
+			tx_ptr = 0; \
+	} \
+	*period_elapsed = period_pos >= runtime->period_size; \
+	return tx_ptr; \
+}
+
+dw_pcm_tx_fn(16);
+dw_pcm_tx_fn(32);
+
+#undef dw_pcm_tx_fn
+
+static const struct snd_pcm_hardware dw_pcm_hardware = {
+	.info = SNDRV_PCM_INFO_INTERLEAVED |
+		SNDRV_PCM_INFO_MMAP |
+		SNDRV_PCM_INFO_MMAP_VALID |
+		SNDRV_PCM_INFO_BLOCK_TRANSFER,
+	.rates = SNDRV_PCM_RATE_32000 |
+		SNDRV_PCM_RATE_44100 |
+		SNDRV_PCM_RATE_48000,
+	.rate_min = 32000,
+	.rate_max = 48000,
+	.formats = SNDRV_PCM_FMTBIT_S16_LE |
+		SNDRV_PCM_FMTBIT_S32_LE,
+	.channels_min = 2,
+	.channels_max = 2,
+	.buffer_bytes_max = BUFFER_BYTES_MAX,
+	.period_bytes_min = PERIOD_BYTES_MIN,
+	.period_bytes_max = BUFFER_BYTES_MAX / PERIODS_MIN,
+	.periods_min = PERIODS_MIN,
+	.periods_max = BUFFER_BYTES_MAX / PERIOD_BYTES_MIN,
+	.fifo_size = 16,
+};
+
+void dw_pcm_push_tx(struct dw_i2s_dev *dev)
+{
+	struct snd_pcm_substream *tx_substream;
+	bool tx_active, period_elapsed;
+
+	rcu_read_lock();
+	tx_substream = rcu_dereference(dev->tx_substream);
+	tx_active = tx_substream && snd_pcm_running(tx_substream);
+	if (tx_active) {
+		unsigned int tx_ptr = ACCESS_ONCE(dev->tx_ptr);
+		unsigned int new_tx_ptr = dev->tx_fn(dev, tx_substream->runtime,
+				tx_ptr, &period_elapsed);
+		cmpxchg(&dev->tx_ptr, tx_ptr, new_tx_ptr);
+
+		if (period_elapsed)
+			snd_pcm_period_elapsed(tx_substream);
+	}
+	rcu_read_unlock();
+}
+
+static int dw_pcm_open(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct dw_i2s_dev *dev = snd_soc_dai_get_drvdata(rtd->cpu_dai);
+
+	snd_soc_set_runtime_hwparams(substream, &dw_pcm_hardware);
+	snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
+	runtime->private_data = dev;
+
+	return 0;
+}
+
+static int dw_pcm_close(struct snd_pcm_substream *substream)
+{
+	synchronize_rcu();
+	return 0;
+}
+
+static int dw_pcm_hw_params(struct snd_pcm_substream *substream,
+		struct snd_pcm_hw_params *hw_params)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct dw_i2s_dev *dev = runtime->private_data;
+	int ret;
+
+	switch (params_channels(hw_params)) {
+	case 2:
+		break;
+	default:
+		dev_err(dev->dev, "invalid channels number\n");
+		return -EINVAL;
+	}
+
+	switch (params_format(hw_params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		dev->tx_fn = dw_pcm_tx_16;
+		break;
+	case SNDRV_PCM_FORMAT_S32_LE:
+		dev->tx_fn = dw_pcm_tx_32;
+		break;
+	default:
+		dev_err(dev->dev, "invalid format\n");
+		return -EINVAL;
+	}
+
+	if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK) {
+		dev_err(dev->dev, "only playback is available\n");
+		return -EINVAL;
+	}
+
+	ret = snd_pcm_lib_malloc_pages(substream,
+			params_buffer_bytes(hw_params));
+	return (ret < 0) ? ret : 0;
+}
+
+static int dw_pcm_hw_free(struct snd_pcm_substream *substream)
+{
+	return snd_pcm_lib_free_pages(substream);
+}
+
+static int dw_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct dw_i2s_dev *dev = runtime->private_data;
+	int ret = 0;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		ACCESS_ONCE(dev->tx_ptr) = 0;
+		rcu_assign_pointer(dev->tx_substream, substream);
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		rcu_assign_pointer(dev->tx_substream, NULL);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static snd_pcm_uframes_t dw_pcm_pointer(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct dw_i2s_dev *dev = runtime->private_data;
+	snd_pcm_uframes_t pos = ACCESS_ONCE(dev->tx_ptr);
+
+	return pos < runtime->buffer_size ? pos : 0;
+}
+
+static int dw_pcm_new(struct snd_soc_pcm_runtime *rtd)
+{
+	size_t size = dw_pcm_hardware.buffer_bytes_max;
+
+	return snd_pcm_lib_preallocate_pages_for_all(rtd->pcm,
+			SNDRV_DMA_TYPE_CONTINUOUS,
+			snd_dma_continuous_data(GFP_KERNEL), size, size);
+}
+
+static void dw_pcm_free(struct snd_pcm *pcm)
+{
+	snd_pcm_lib_preallocate_free_for_all(pcm);
+}
+
+static const struct snd_pcm_ops dw_pcm_ops = {
+	.open = dw_pcm_open,
+	.close = dw_pcm_close,
+	.ioctl = snd_pcm_lib_ioctl,
+	.hw_params = dw_pcm_hw_params,
+	.hw_free = dw_pcm_hw_free,
+	.trigger = dw_pcm_trigger,
+	.pointer = dw_pcm_pointer,
+};
+
+static const struct snd_soc_platform_driver dw_pcm_platform = {
+	.pcm_new = dw_pcm_new,
+	.pcm_free = dw_pcm_free,
+	.ops = &dw_pcm_ops,
+};
+
+int dw_pcm_register(struct platform_device *pdev)
+{
+	return devm_snd_soc_register_platform(&pdev->dev, &dw_pcm_platform);
+}
diff --git a/sound/soc/dwc/local.h b/sound/soc/dwc/local.h
new file mode 100644
index 0000000..6a8edc7
--- /dev/null
+++ b/sound/soc/dwc/local.h
@@ -0,0 +1,122 @@
+/*
+ * Copyright (ST) 2012 Rajeev Kumar (rajeevkumar.linux@gmail.com)
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __DESIGNWARE_LOCAL_H
+#define __DESIGNWARE_LOCAL_H
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/types.h>
+#include <sound/dmaengine_pcm.h>
+#include <sound/pcm.h>
+#include <sound/designware_i2s.h>
+
+/* common register for all channel */
+#define IER		0x000
+#define IRER		0x004
+#define ITER		0x008
+#define CER		0x00C
+#define CCR		0x010
+#define RXFFR		0x014
+#define TXFFR		0x018
+
+/* I2STxRxRegisters for all channels */
+#define LRBR_LTHR(x)	(0x40 * x + 0x020)
+#define RRBR_RTHR(x)	(0x40 * x + 0x024)
+#define RER(x)		(0x40 * x + 0x028)
+#define TER(x)		(0x40 * x + 0x02C)
+#define RCR(x)		(0x40 * x + 0x030)
+#define TCR(x)		(0x40 * x + 0x034)
+#define ISR(x)		(0x40 * x + 0x038)
+#define IMR(x)		(0x40 * x + 0x03C)
+#define ROR(x)		(0x40 * x + 0x040)
+#define TOR(x)		(0x40 * x + 0x044)
+#define RFCR(x)		(0x40 * x + 0x048)
+#define TFCR(x)		(0x40 * x + 0x04C)
+#define RFF(x)		(0x40 * x + 0x050)
+#define TFF(x)		(0x40 * x + 0x054)
+
+/* I2SCOMPRegisters */
+#define I2S_COMP_PARAM_2	0x01F0
+#define I2S_COMP_PARAM_1	0x01F4
+#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)
+
+/* Number of entries in WORDSIZE and DATA_WIDTH parameter registers */
+#define	COMP_MAX_WORDSIZE	(1 << 3)
+#define	COMP_MAX_DATA_WIDTH	(1 << 2)
+
+#define MAX_CHANNEL_NUM		8
+#define MIN_CHANNEL_NUM		2
+
+union dw_i2s_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;
+	unsigned int quirks;
+	unsigned int i2s_reg_comp1;
+	unsigned int i2s_reg_comp2;
+	struct device *dev;
+	u32 ccr;
+	u32 xfer_resolution;
+	u32 fifo_th;
+
+	/* data related to DMA transfers b/w i2s and DMAC */
+	union dw_i2s_snd_dma_data play_dma_data;
+	union dw_i2s_snd_dma_data capture_dma_data;
+	struct i2s_clk_config_data config;
+	int (*i2s_clk_cfg)(struct i2s_clk_config_data *config);
+
+	/* data related to PIO transfers (TX) */
+	bool use_pio;
+	struct snd_pcm_substream __rcu *tx_substream;
+	unsigned int (*tx_fn)(struct dw_i2s_dev *dev,
+			struct snd_pcm_runtime *runtime, unsigned int tx_ptr,
+			bool *period_elapsed);
+	unsigned int tx_ptr;
+};
+
+#if IS_ENABLED(CONFIG_SND_DESIGNWARE_PCM)
+void dw_pcm_push_tx(struct dw_i2s_dev *dev);
+int dw_pcm_register(struct platform_device *pdev);
+#else
+void dw_pcm_push_tx(struct dw_i2s_dev *dev) { }
+int dw_pcm_register(struct platform_device *pdev)
+{
+	return -EINVAL;
+}
+#endif
+
+#endif
-- 
1.9.1

Re: [PATCH 3/4 v7] ASoC: dwc: Add PIO PCM extension

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1984 bytes --]

On Mon, May 23, 2016 at 11:02:24AM +0100, Jose Abreu wrote:

> +config SND_DESIGNWARE_PCM
> +	bool "PCM PIO extension for I2S driver"

Why can't this be built as a module?

> +
> +	return irq_valid ? IRQ_HANDLED : IRQ_NONE;

Please write a normal if statement, the ternery operator doesn't help
legibility.

>  static void i2s_start(struct dw_i2s_dev *dev,
>  		      struct snd_pcm_substream *substream)
>  {
>  	struct i2s_clk_config_data *config = &dev->config;
>  
>  	i2s_write_reg(dev->i2s_base, IER, 1);
> -	i2s_enable_irqs(dev, substream->stream, config->chan_nr);
> +
> +	if (dev->use_pio)
> +		i2s_enable_irqs(dev, substream->stream, config->chan_nr);
>  
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>  		i2s_write_reg(dev->i2s_base, ITER, 1);

That seems wrong, or at least something that should be separate?
Previously we needed interrupts for DMA operation but now we enable
interrupts only if we don't use DMA.  It feels like we want to make the
change for DMA separately if only to make it clear for bisection, are we
100% sure that masking the interrupt won't also mask the DMA request
signals?

> +	irq = platform_get_irq(pdev, 0);
> +	if (irq >= 0) {
> +		dev_dbg(&pdev->dev, "using PIO mode\n");
> +		dev->use_pio = true;
> +
> +		ret = devm_request_irq(&pdev->dev, irq, i2s_irq_handler, 0,
> +				pdev->name, dev);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "failed to request irq\n");
> +			return ret;
> +		}
> +	}

This also seems wrong.  We're forcing PIO if an interrupt is provided
rather than based on DMA being configured which means that if the
interrupt is wired up and happens to be described in DT we'll get worse
performance.  People should be able to just describe the system without
worrying about this, and we might find some other use for the interrupts
in future.  Indeed right now it would probably be reasonable to use the
error interrupts all the time if they're available.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 3/4 v7] ASoC: dwc: Add PIO PCM extension

[Jose Abreu]

Hi Mark,

Thanks for your comments.

On 24-05-2016 17:41, Mark Brown wrote:
> On Mon, May 23, 2016 at 11:02:24AM +0100, Jose Abreu wrote:
>
>> +config SND_DESIGNWARE_PCM
>> +	bool "PCM PIO extension for I2S driver"
> Why can't this be built as a module?

I can change but my intention was to make this PCM a kind of extension to the
driver instead of adding a new module to the system.

>
>> +
>> +	return irq_valid ? IRQ_HANDLED : IRQ_NONE;
> Please write a normal if statement, the ternery operator doesn't help
> legibility.

Ok.

>
>>  static void i2s_start(struct dw_i2s_dev *dev,
>>  		      struct snd_pcm_substream *substream)
>>  {
>>  	struct i2s_clk_config_data *config = &dev->config;
>>  
>>  	i2s_write_reg(dev->i2s_base, IER, 1);
>> -	i2s_enable_irqs(dev, substream->stream, config->chan_nr);
>> +
>> +	if (dev->use_pio)
>> +		i2s_enable_irqs(dev, substream->stream, config->chan_nr);
>>  
>>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>  		i2s_write_reg(dev->i2s_base, ITER, 1);
> That seems wrong, or at least something that should be separate?
> Previously we needed interrupts for DMA operation but now we enable
> interrupts only if we don't use DMA.  It feels like we want to make the
> change for DMA separately if only to make it clear for bisection, are we
> 100% sure that masking the interrupt won't also mask the DMA request
> signals?

Indeed I thought about this and the interrupts must also be enabled when in DMA
mode. Although there is no interrupt handler in the original driver (without
this patches) in some setups the interrupt line may be connected to the DMA
controller. I will drop this change and always enable interrupts. Please note
that I don't have a setup with DMA support so I can only test using the PIO mode.

>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq >= 0) {
>> +		dev_dbg(&pdev->dev, "using PIO mode\n");
>> +		dev->use_pio = true;
>> +
>> +		ret = devm_request_irq(&pdev->dev, irq, i2s_irq_handler, 0,
>> +				pdev->name, dev);
>> +		if (ret < 0) {
>> +			dev_err(&pdev->dev, "failed to request irq\n");
>> +			return ret;
>> +		}
>> +	}
> This also seems wrong.  We're forcing PIO if an interrupt is provided
> rather than based on DMA being configured which means that if the
> interrupt is wired up and happens to be described in DT we'll get worse
> performance.  People should be able to just describe the system without
> worrying about this, and we might find some other use for the interrupts
> in future.  Indeed right now it would probably be reasonable to use the
> error interrupts all the time if they're available.

How should I then determine which mode to use?
    - Check if DMA parameters are declared in DT, or
    - Check if snd_dmaengine_pcm_register() fails, or
    - Assume PIO mode will be used when compiling with PIO PCM, or
    - Something else ?

Best regards,
Jose Miguel Abreu

Re: [PATCH 3/4 v7] ASoC: dwc: Add PIO PCM extension

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2027 bytes --]

On Tue, May 24, 2016 at 06:07:14PM +0100, Jose Abreu wrote:
> On 24-05-2016 17:41, Mark Brown wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> >>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> >>  		i2s_write_reg(dev->i2s_base, ITER, 1);

> > That seems wrong, or at least something that should be separate?
> > Previously we needed interrupts for DMA operation but now we enable
> > interrupts only if we don't use DMA.  It feels like we want to make the
> > change for DMA separately if only to make it clear for bisection, are we
> > 100% sure that masking the interrupt won't also mask the DMA request
> > signals?

> Indeed I thought about this and the interrupts must also be enabled when in DMA
> mode. Although there is no interrupt handler in the original driver (without
> this patches) in some setups the interrupt line may be connected to the DMA
> controller. I will drop this change and always enable interrupts. Please note
> that I don't have a setup with DMA support so I can only test using the PIO mode.

Presumably you can talk to your hardware colleagues and get them to make
you a FPGA with a DMA IP available?

> > This also seems wrong.  We're forcing PIO if an interrupt is provided
> > rather than based on DMA being configured which means that if the
> > interrupt is wired up and happens to be described in DT we'll get worse

> How should I then determine which mode to use?
>     - Check if DMA parameters are declared in DT, or
>     - Check if snd_dmaengine_pcm_register() fails, or
>     - Assume PIO mode will be used when compiling with PIO PCM, or
>     - Something else ?

You could either unconditionally register the PIO driver and only
actually start using it if the driver is instantiated or you could check
to see if the registration function works (handling deferred probe - if
the DMA driver just didn't load yet you should wait for it).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 3/4 v7] ASoC: dwc: Add PIO PCM extension

[Jose Abreu]

Hi Mark,


On 24-05-2016 18:51, Mark Brown wrote:
> On Tue, May 24, 2016 at 06:07:14PM +0100, Jose Abreu wrote:
>> On 24-05-2016 17:41, Mark Brown wrote:
> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns.  Doing this makes your messages much
> easier to read and reply to.
>
>>>>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>>>  		i2s_write_reg(dev->i2s_base, ITER, 1);
>>> That seems wrong, or at least something that should be separate?
>>> Previously we needed interrupts for DMA operation but now we enable
>>> interrupts only if we don't use DMA.  It feels like we want to make the
>>> change for DMA separately if only to make it clear for bisection, are we
>>> 100% sure that masking the interrupt won't also mask the DMA request
>>> signals?
>> Indeed I thought about this and the interrupts must also be enabled when in DMA
>> mode. Although there is no interrupt handler in the original driver (without
>> this patches) in some setups the interrupt line may be connected to the DMA
>> controller. I will drop this change and always enable interrupts. Please note
>> that I don't have a setup with DMA support so I can only test using the PIO mode.
> Presumably you can talk to your hardware colleagues and get them to make
> you a FPGA with a DMA IP available?

Its already in the todo list.

>
>>> This also seems wrong.  We're forcing PIO if an interrupt is provided
>>> rather than based on DMA being configured which means that if the
>>> interrupt is wired up and happens to be described in DT we'll get worse
>> How should I then determine which mode to use?
>>     - Check if DMA parameters are declared in DT, or
>>     - Check if snd_dmaengine_pcm_register() fails, or
>>     - Assume PIO mode will be used when compiling with PIO PCM, or
>>     - Something else ?
> You could either unconditionally register the PIO driver and only
> actually start using it if the driver is instantiated or you could check
> to see if the registration function works (handling deferred probe - if
> the DMA driver just didn't load yet you should wait for it).

I think I will take the second option. Something like this:
"
    ret = snd_dmaengine_pcm_register(...)
    if (ret == -EPROBE_DEFER)
        return ret;
    else
        pio_register(...);
"?


Best regards,
Jose Miguel Abreu

Re: [PATCH 3/4 v7] ASoC: dwc: Add PIO PCM extension

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 380 bytes --]

On Wed, May 25, 2016 at 11:11:47AM +0100, Jose Abreu wrote:

> I think I will take the second option. Something like this:
> "
>     ret = snd_dmaengine_pcm_register(...)
>     if (ret == -EPROBE_DEFER)
>         return ret;
>     else
>         pio_register(...);
> "?

Sure.  You should print a diagnostic if you fail to get the DMA, for any
real system it's going to be a bug.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 3/4 v7] ASoC: dwc: Add PIO PCM extension

[Jose Abreu]

Hi Mark,


On 25-05-2016 11:18, Mark Brown wrote:
> On Wed, May 25, 2016 at 11:11:47AM +0100, Jose Abreu wrote:
>
>> I think I will take the second option. Something like this:
>> "
>>     ret = snd_dmaengine_pcm_register(...)
>>     if (ret == -EPROBE_DEFER)
>>         return ret;
>>     else
>>         pio_register(...);
>> "?
> Sure.  You should print a diagnostic if you fail to get the DMA, for any
> real system it's going to be a bug.

Ok, will do that. I noticed the last I2S patch that you merged
("ASoC: dwc: Add helper functions to disable/enable irqs") is not
in for-next yet. Should I base my work on 'topic/dwc' branch?

Best regards,
Jose Miguel Abreu

Re: [PATCH 3/4 v7] ASoC: dwc: Add PIO PCM extension

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

On Wed, May 25, 2016 at 11:49:12AM +0100, Jose Abreu wrote:

> Ok, will do that. I noticed the last I2S patch that you merged
> ("ASoC: dwc: Add helper functions to disable/enable irqs") is not
> in for-next yet. Should I base my work on 'topic/dwc' branch?

We are in the merge window.  No new non-bugfix patches will be merged
until the merge window ends, most likely sometime this weekend.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 4/4 v7] ASoC: dwc: Add irq parameter to DOCUMENTATION

[Jose Abreu]

A parameter description for the interruptions of the
I2S controller was added. This interrupt parameter
should only be set when I2S does not have DMA support.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: linux-snps-arc@lists.infradead.org
Cc: alsa-devel@alsa-project.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---

Changes v6 -> v7:
* interrupts is now optional property

No changes v5 -> v6.

Changes v4 -> v5:
* interrupts is now required property
* Drop 'snps-use-dmaengine' property

This patch was only introduced in v4.

 Documentation/devicetree/bindings/sound/designware-i2s.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/designware-i2s.txt b/Documentation/devicetree/bindings/sound/designware-i2s.txt
index 7bb5424..6a536d5 100644
--- a/Documentation/devicetree/bindings/sound/designware-i2s.txt
+++ b/Documentation/devicetree/bindings/sound/designware-i2s.txt
@@ -12,6 +12,10 @@ Required properties:
    one for receive.
  - dma-names : "tx" for the transmit channel, "rx" for the receive channel.
 
+Optional properties:
+ - interrupts: The interrupt line number for the I2S controller. Add this
+   parameter if the I2S controller that you are using does not support DMA.
+
 For more details on the 'dma', 'dma-names', 'clock' and 'clock-names'
 properties please check:
 	* resource-names.txt
-- 
1.9.1

Re: [PATCH 4/4 v7] ASoC: dwc: Add irq parameter to DOCUMENTATION

[Rob Herring]

On Mon, May 23, 2016 at 11:02:25AM +0100, Jose Abreu wrote:
> A parameter description for the interruptions of the
> I2S controller was added. This interrupt parameter
> should only be set when I2S does not have DMA support.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: linux-snps-arc@lists.infradead.org
> Cc: alsa-devel@alsa-project.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> 
> Changes v6 -> v7:
> * interrupts is now optional property
> 
> No changes v5 -> v6.
> 
> Changes v4 -> v5:
> * interrupts is now required property
> * Drop 'snps-use-dmaengine' property
> 
> This patch was only introduced in v4.
> 
>  Documentation/devicetree/bindings/sound/designware-i2s.txt | 4 ++++
>  1 file changed, 4 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Applied "ASoC: dwc: Add irq parameter to DOCUMENTATION" to the asoc tree

[Mark Brown]

The patch

   ASoC: dwc: Add irq parameter to DOCUMENTATION

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 5aa1418defb0e88fb7f70c5b112d531cfcede851 Mon Sep 17 00:00:00 2001
From: Jose Abreu <Jose.Abreu@synopsys.com>
Date: Thu, 9 Jun 2016 12:47:06 +0100
Subject: [PATCH] ASoC: dwc: Add irq parameter to DOCUMENTATION

A parameter description for the interruptions of the
I2S controller was added.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Acked-by: Rob Herring <robh@kernel.org>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Alexey Brodkin <abrodkin@synopsys.com>
Cc: linux-snps-arc@lists.infradead.org
Cc: alsa-devel@alsa-project.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 Documentation/devicetree/bindings/sound/designware-i2s.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/designware-i2s.txt b/Documentation/devicetree/bindings/sound/designware-i2s.txt
index 7bb54247f8e8..6a536d570e29 100644
--- a/Documentation/devicetree/bindings/sound/designware-i2s.txt
+++ b/Documentation/devicetree/bindings/sound/designware-i2s.txt
@@ -12,6 +12,10 @@ Required properties:
    one for receive.
  - dma-names : "tx" for the transmit channel, "rx" for the receive channel.
 
+Optional properties:
+ - interrupts: The interrupt line number for the I2S controller. Add this
+   parameter if the I2S controller that you are using does not support DMA.
+
 For more details on the 'dma', 'dma-names', 'clock' and 'clock-names'
 properties please check:
 	* resource-names.txt
-- 
2.8.1