Re: [PATCH 1/2 v8] ASoC: dwc: Add PIO PCM extension

2016-06-07 Thread Mark Brown 
On Mon, May 30, 2016 at 10:53:15AM +0100, Jose Abreu wrote:

A few small things but this looks basically fine.

> + if (isr[i] & 0x33)
> + irq_valid = true;

This checks for a mask of 0x33 but...

> +
> + /*
> +  * Check if TX fifo is empty. If empty fill FIFO with samples
> +  * NOTE: Only two channels supported
> +  */
> + if ((isr[i] & 0x10) && (i == 0) && dev->use_pio)
> + 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);

...only 0x32 seem to actually be handled, and then only sometimes.  It's
going to be clearer and more robust to just set the handled flag to true
when handling, that way we don't have to match up different numerical
masks.  Some defines would help legibility too.

> + ret = snd_pcm_lib_malloc_pages(substream,
> + params_buffer_bytes(hw_params));
> + return (ret < 0) ? ret : 0;

Write normal if statements please, it helps legibility.


signature.asc
Description: PGP signature

Re: [PATCH 1/2 v8] ASoC: dwc: Add PIO PCM extension

2016-06-07 Thread Mark Brown 
On Mon, May 30, 2016 at 10:53:15AM +0100, Jose Abreu wrote:

A few small things but this looks basically fine.

> + if (isr[i] & 0x33)
> + irq_valid = true;

This checks for a mask of 0x33 but...

> +
> + /*
> +  * Check if TX fifo is empty. If empty fill FIFO with samples
> +  * NOTE: Only two channels supported
> +  */
> + if ((isr[i] & 0x10) && (i == 0) && dev->use_pio)
> + 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);

...only 0x32 seem to actually be handled, and then only sometimes.  It's
going to be clearer and more robust to just set the handled flag to true
when handling, that way we don't have to match up different numerical
masks.  Some defines would help legibility too.

> + ret = snd_pcm_lib_malloc_pages(substream,
> + params_buffer_bytes(hw_params));
> + return (ret < 0) ? ret : 0;

Write normal if statements please, it helps legibility.


signature.asc
Description: PGP signature

[PATCH 1/2 v8] ASoC: dwc: Add PIO PCM extension

2016-05-30 Thread 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.

Signed-off-by: Jose Abreu 
Cc: Carlos Palminha 
Cc: Mark Brown 
Cc: Liam Girdwood 
Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Cc: Rob Herring 
Cc: Alexey Brodkin 
Cc: linux-snps-...@lists.infradead.org
Cc: alsa-de...@alsa-project.org
Cc: linux-kernel@vger.kernel.org
---

Changes v7 -> v8:
* Build PIO PCM as module
* Always unmask interrupts even when in DMA mode
* Fallback to PIO mode only if ALSA DMA engine probe fails

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 |   1 +
 sound/soc/dwc/designware_i2s.c | 152 
 sound/soc/dwc/designware_pcm.c | 222 +
 sound/soc/dwc/local.h  | 122 ++
 5 files changed, 418 insertions(+), 88 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..c297efe 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
+   tristate "PCM PIO extension for I2S driver"
+   depends on SND_DESIGNWARE_I2S
+   help
+Say Y, M 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..1b48bccc 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_PCM) += designware_pcm.o
 
diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c
index 4c4f0dc..a238fa0 100644
--- a/sound/soc/dwc/designware_i2s.c
+++ b/sound/soc/dwc/designware_i2s.c
@@ -24,90 +24,7 @@
 #include 
 #include 
 #include 
-
-/* common register for all channel */
-#define IER0x000
-#define IRER   0x004
-#define ITER   0x008
-#define CER0x00C
-#define CCR0x010
-#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.
- */
-#defineCOMP1_TX_WORDSIZE_3(r)  (((r) & GENMASK(27, 25)) >> 25)
-#defineCOMP1_TX_WORDSIZE_2(r)  (((r) & GENMASK(24, 22)) >> 22)
-#defineCOMP1_TX_WORDSIZE_1(r)  (((r) & GENMASK(21, 19)) >> 19)
-#defineCOMP1_TX_WORDSIZE_0(r)  (((r) & GENMASK(18, 16)) >> 16)
-#defineCOMP1_TX_CHANNELS(r)(((r) & GENMASK(10, 9)) >> 9)
-#defineCOMP1_RX_CHANNELS(r)(((r) & GENMASK(8, 7)) >> 7)
-#defineCOMP1_RX_ENABLED(r) (((r) & BIT(6))

[PATCH 1/2 v8] ASoC: dwc: Add PIO PCM extension

2016-05-30 Thread 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.

Signed-off-by: Jose Abreu 
Cc: Carlos Palminha 
Cc: Mark Brown 
Cc: Liam Girdwood 
Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Cc: Rob Herring 
Cc: Alexey Brodkin 
Cc: linux-snps-...@lists.infradead.org
Cc: alsa-de...@alsa-project.org
Cc: linux-kernel@vger.kernel.org
---

Changes v7 -> v8:
* Build PIO PCM as module
* Always unmask interrupts even when in DMA mode
* Fallback to PIO mode only if ALSA DMA engine probe fails

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 |   1 +
 sound/soc/dwc/designware_i2s.c | 152 
 sound/soc/dwc/designware_pcm.c | 222 +
 sound/soc/dwc/local.h  | 122 ++
 5 files changed, 418 insertions(+), 88 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..c297efe 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
+   tristate "PCM PIO extension for I2S driver"
+   depends on SND_DESIGNWARE_I2S
+   help
+Say Y, M 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..1b48bccc 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_PCM) += designware_pcm.o
 
diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c
index 4c4f0dc..a238fa0 100644
--- a/sound/soc/dwc/designware_i2s.c
+++ b/sound/soc/dwc/designware_i2s.c
@@ -24,90 +24,7 @@
 #include 
 #include 
 #include 
-
-/* common register for all channel */
-#define IER0x000
-#define IRER   0x004
-#define ITER   0x008
-#define CER0x00C
-#define CCR0x010
-#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.
- */
-#defineCOMP1_TX_WORDSIZE_3(r)  (((r) & GENMASK(27, 25)) >> 25)
-#defineCOMP1_TX_WORDSIZE_2(r)  (((r) & GENMASK(24, 22)) >> 22)
-#defineCOMP1_TX_WORDSIZE_1(r)  (((r) & GENMASK(21, 19)) >> 19)
-#defineCOMP1_TX_WORDSIZE_0(r)  (((r) & GENMASK(18, 16)) >> 16)
-#defineCOMP1_TX_CHANNELS(r)(((r) & GENMASK(10, 9)) >> 9)
-#defineCOMP1_RX_CHANNELS(r)(((r) & GENMASK(8, 7)) >> 7)
-#defineCOMP1_RX_ENABLED(r) (((r) & BIT(6)) >> 6)
-#defineCOMP1_TX_ENABLED(r) (((r) & BIT(5)) >> 5)
-#defineCOMP1_MODE_EN(r)(((r) & BIT(4)) >> 4)
-#define