Re: [PATCH v2 2/2] ASoC: atmel-i2s: add driver for the new Atmel I2S controller

2015-10-05 Thread Mark Brown 
On Tue, Sep 29, 2015 at 04:09:20PM +0200, Cyrille Pitchen wrote:

> + if (pending & ATMEL_I2SC_INT_RXOR) {
> + mask = ATMEL_I2SC_SR_RXOR;
> +
> + for (ch = 0; ch < ATMEL_I2SC_MAX_TDM_CHANNELS; ++ch)
> + if (sr & ATMEL_I2SC_SR_RXORCH(ch)) {
> + mask |= ATMEL_I2SC_SR_RXORCH(ch);
> + dev_err(dev->dev,
> + "RX overrun on channel %d\n", ch);
> + }
> + regmap_write(dev->regmap, ATMEL_I2SC_SCR, mask);
> + }

Coding style - the for loop needs { } for legibility.

> + if (pending & ATMEL_I2SC_INT_TXUR) {
> + mask = ATMEL_I2SC_SR_TXUR;
> +
> + for (ch = 0; ch < ATMEL_I2SC_MAX_TDM_CHANNELS; ++ch)
> + if (sr & ATMEL_I2SC_SR_TXURCH(ch)) {
> + mask |= ATMEL_I2SC_SR_TXURCH(ch);
> + dev_err(dev->dev,
> + "TX underrun on channel %d\n", ch);
> + }
> + regmap_write(dev->regmap, ATMEL_I2SC_SCR, mask);
> +
> + }
> +
> + return IRQ_HANDLED;

This IRQ_HANDLED should be generated only if one of the interrupts we
know about got handled - there was a check to see if any of the unmasked
bits is set earlier on in the function but that's not quite the same
check.

> +
> +static int atmel_i2s_prepare(struct snd_pcm_substream *substream,
> +  struct snd_soc_dai *dai)
> +{
> + struct atmel_i2s_dev *dev = snd_soc_dai_get_drvdata(dai);
> + bool is_playback = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK);
> + unsigned int rhr, sr = 0;
> +
> + if (is_playback) {
> + regmap_read(dev->regmap, ATMEL_I2SC_SR, &sr);
> + if (sr & ATMEL_I2SC_SR_RXRDY) {
> + dev_dbg(dev->dev, "RXRDY is set\n");
> + regmap_read(dev->regmap, ATMEL_I2SC_RHR, &rhr);
> + }
> + }

What's this doing?  It just seems to do two reads and issue a debug
message...

> +static int atmel_i2s_sama5d2_mck_init(struct atmel_i2s_dev *dev,
> +   struct device_node *np)
> +{
> + #define SFR_I2SCLKSEL 0x90U
> + struct regmap *sfr;
> + int id;
> +
> + id = of_alias_get_id(np, "i2s");
> + if (id < 0) {
> + dev_err(dev->dev, "failed to get alias ID\n");
> + return id;
> + }
> + if (id > 1) {
> + dev_err(dev->dev, "invalid I2S controller ID: %d\n", id);
> + return -EINVAL;
> + }

This didn't appear in the DT binding and looks pretty funky - what's
going on here?

> + sfr = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> + if (IS_ERR(sfr)) {
> + dev_err(dev->dev, "failed to get SFR syscon\n");
> + return PTR_ERR(sfr);
> + }

This didn't appear in the binding either.

> + /* Get hardware capabilities. */
> + match = of_match_node(atmel_i2s_dt_ids, np);
> + dev->caps = match ? match->data : NULL;

Please don't abuse the ternery operator like this, just write a normal
if statement :/


signature.asc
Description: Digital signature

[PATCH v2 2/2] ASoC: atmel-i2s: add driver for the new Atmel I2S controller

2015-09-29 Thread Cyrille Pitchen 
This patch adds support for the Atmel I2S controller embedded into
sama5d2x SoCs.

Signed-off-by: Cyrille Pitchen 
---
 sound/soc/atmel/Kconfig |  10 +
 sound/soc/atmel/Makefile|   2 +
 sound/soc/atmel/atmel-i2s.c | 760 
 3 files changed, 772 insertions(+)
 create mode 100644 sound/soc/atmel/atmel-i2s.c

diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig
index 1489cd461aec..c026c207a386 100644
--- a/sound/soc/atmel/Kconfig
+++ b/sound/soc/atmel/Kconfig
@@ -30,6 +30,16 @@ config SND_ATMEL_SOC_SSC
default y if SND_ATMEL_SOC_SSC_DMA=y || SND_ATMEL_SOC_SSC_PDC=y
default m if SND_ATMEL_SOC_SSC_DMA=m || SND_ATMEL_SOC_SSC_PDC=m
 
+config SND_ATMEL_SOC_I2S
+   tristate "SoC Audio support for the Atmel I2S module"
+   select SND_SOC_GENERIC_DMAENGINE_PCM
+   select REGMAP_MMIO
+   depends on OF && (ARCH_AT91 || COMPILE_TEST)
+   help
+ Say Y or M if you want to add support for codecs attached to
+ the Atmel I2S interface. You will also need
+ to select the audio interfaces to support below.
+
 config SND_AT91_SOC_SAM9G20_WM8731
tristate "SoC Audio support for WM8731-based At91sam9g20 evaluation 
board"
depends on ARCH_AT91 || COMPILE_TEST
diff --git a/sound/soc/atmel/Makefile b/sound/soc/atmel/Makefile
index b327e5cc8de3..4d960464aecb 100644
--- a/sound/soc/atmel/Makefile
+++ b/sound/soc/atmel/Makefile
@@ -2,10 +2,12 @@
 snd-soc-atmel-pcm-pdc-objs := atmel-pcm-pdc.o
 snd-soc-atmel-pcm-dma-objs := atmel-pcm-dma.o
 snd-soc-atmel_ssc_dai-objs := atmel_ssc_dai.o
+snd-soc-atmel-i2s-objs := atmel-i2s.o
 
 obj-$(CONFIG_SND_ATMEL_SOC_PDC) += snd-soc-atmel-pcm-pdc.o
 obj-$(CONFIG_SND_ATMEL_SOC_DMA) += snd-soc-atmel-pcm-dma.o
 obj-$(CONFIG_SND_ATMEL_SOC_SSC) += snd-soc-atmel_ssc_dai.o
+obj-$(CONFIG_SND_ATMEL_SOC_I2S) += snd-soc-atmel-i2s.o
 
 # AT91 Machine Support
 snd-soc-sam9g20-wm8731-objs := sam9g20_wm8731.o
diff --git a/sound/soc/atmel/atmel-i2s.c b/sound/soc/atmel/atmel-i2s.c
new file mode 100644
index ..9e15acabbaab
--- /dev/null
+++ b/sound/soc/atmel/atmel-i2s.c
@@ -0,0 +1,760 @@
+/*
+ * Driver for Atmel I2S controller
+ *
+ * Copyright (C) 2015 Atmel Corporation
+ *
+ * Author: Cyrille Pitchen 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ */
+
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define ATMEL_I2SC_MAX_TDM_CHANNELS8
+
+/*
+ *  I2S Controller Register map 
+ */
+#define ATMEL_I2SC_CR  0x  /* Control Register */
+#define ATMEL_I2SC_MR  0x0004  /* Mode Register */
+#define ATMEL_I2SC_SR  0x0008  /* Status Register */
+#define ATMEL_I2SC_SCR 0x000c  /* Status Clear Register */
+#define ATMEL_I2SC_SSR 0x0010  /* Status Set Register */
+#define ATMEL_I2SC_IER 0x0014  /* Interrupt Enable Register */
+#define ATMEL_I2SC_IDR 0x0018  /* Interrupt Disable Register */
+#define ATMEL_I2SC_IMR 0x001c  /* Interrupt Mask Register */
+#define ATMEL_I2SC_RHR 0x0020  /* Receiver Holding Register */
+#define ATMEL_I2SC_THR 0x0024  /* Transmitter Holding Register */
+#define ATMEL_I2SC_VERSION 0x0028  /* Version Register */
+
+
+/*
+ *  Control Register (Write-only) 
+ */
+#define ATMEL_I2SC_CR_RXEN BIT(0)  /* Receiver Enable */
+#define ATMEL_I2SC_CR_RXDISBIT(1)  /* Receiver Disable */
+#define ATMEL_I2SC_CR_CKEN BIT(2)  /* Clock Enable */
+#define ATMEL_I2SC_CR_CKDISBIT(3)  /* Clock Disable */
+#define ATMEL_I2SC_CR_TXEN BIT(4)  /* Transmitter Enable */
+#define ATMEL_I2SC_CR_TXDISBIT(5)  /* Transmitter Disable */
+#define ATMEL_I2SC_CR_SWRSTBIT(7)  /* Software Reset */
+
+
+/*
+ *  Mode Register (Read/Write) 
+ */
+#define ATMEL_I2SC_MR_MODE_MASKGENMASK(0, 0)
+#define ATMEL_I2SC_MR_MODE_SLAVE   (0 << 0)
+#define ATMEL_I2SC_MR_MODE_MASTER  (1 << 0)
+
+#define ATMEL_I2SC_MR_DATALENGTH_MASK  GENMASK(4, 2)
+#define ATMEL_I2SC_MR_DATALENGTH_32_BITS   (0 << 2)
+#define ATMEL_I2SC_MR_DATALENGTH_24_BITS   (1 << 2)
+#define ATMEL_I2SC_MR_DATALENGTH_20_BITS   (2 << 2)
+#define ATMEL_I2SC_MR_DATALENGTH_18_BITS   (3 << 2)
+#define ATMEL_I2SC_MR_DATALENGTH_16_BITS   (4 << 2)
+#define ATMEL_I2SC_MR_DATALENGTH_16_BITS_COMPACT   (5 << 2)
+