[PATCH v2 0/2] Add Cadence I2S-MC controller driver

[PATCH v2 0/2] Add Cadence I2S-MC controller driver

[Xingyu Wu]

The Cadence Multi-channel I2S (I2S-MC) Controller implements a function of
the multi-channel (up to 8-channel) bus. Each stereo channel combines
functions of a transmitter and a receiver, and can switch freely between
them. Each channel has independent gating, clock and interruption control.
It also support some of these channels are used as playback and others can 
also be used as record in the same time.

Four I2S controllers are used on the StarFive JH8100 SoC. Two of the I2S 
controllers use two stereo channels, one of them use four channels, and 
one use eight. It had tested on the fpga with WM8960 and PDM.

Changes since v1:
- Added new compatible for StarFive JH8100 SoC and a special property to
  be got as the max channels number in the bindings.
- Dropped the useless '|' in the bindings.
- Moved the drivers to a new folder named 'cdns' and modified the name
  of functions.

v1: https://lore.kernel.org/all/20231221033223.73201-1-xingyu.wu@starfivetech.com/

Xingyu Wu (2):
  ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller
  ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller

 .../bindings/sound/cdns,i2s-mc.yaml           | 110 +++
 MAINTAINERS                                   |   6 +
 sound/soc/Kconfig                             |   1 +
 sound/soc/Makefile                            |   1 +
 sound/soc/cdns/Kconfig                        |  18 +
 sound/soc/cdns/Makefile                       |   3 +
 sound/soc/cdns/cdns-i2s-mc-pcm.c              | 262 +++++++
 sound/soc/cdns/cdns-i2s-mc.c                  | 724 ++++++++++++++++++
 sound/soc/cdns/cdns-i2s-mc.h                  | 157 ++++
 9 files changed, 1282 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
 create mode 100644 sound/soc/cdns/Kconfig
 create mode 100644 sound/soc/cdns/Makefile
 create mode 100644 sound/soc/cdns/cdns-i2s-mc-pcm.c
 create mode 100644 sound/soc/cdns/cdns-i2s-mc.c
 create mode 100644 sound/soc/cdns/cdns-i2s-mc.h

-- 
2.25.1

[PATCH v2 1/2] ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller

[Xingyu Wu]

Add bindings for the Multi-Channel I2S controller of Cadence.

The Multi-Channel I2S (I2S-MC) implements a function of the
8-channel I2S bus interfasce. Each channel can become receiver
or transmitter. Four I2S instances are used on the StarFive
JH8100 SoC. One instance of them is limited to 2 channels, two
instance are limited to 4 channels, and the other one can use
most 8 channels. Add a unique property about
'starfive,i2s-max-channels' to distinguish each instance.

Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
 .../bindings/sound/cdns,i2s-mc.yaml           | 110 ++++++++++++++++++
 1 file changed, 110 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml

diff --git a/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml b/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
new file mode 100644
index 000000000000..0a1b0122a246
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
@@ -0,0 +1,110 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/cdns,i2s-mc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cadence multi-channel I2S controller
+
+description:
+  The Cadence I2S Controller implements a function of the multi-channel
+  (up to 8-channel) bus. It combines functions of a transmitter and a receiver.
+
+maintainers:
+  - Xingyu Wu <xingyu.wu@starfivetech.com>
+
+properties:
+  compatible:
+    enum:
+      - cdns,i2s-mc
+      - starfive,jh8100-i2s
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    description:
+      The interrupt line number for the I2S controller. Add this
+      parameter if the I2S controller that you are using does not
+      using DMA.
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Bit clock
+      - description: Main ICG clock
+      - description: Inner master clock
+
+  clock-names:
+    items:
+      - const: bclk
+      - const: icg
+      - const: mclk_inner
+
+  resets:
+    maxItems: 1
+
+  dmas:
+    items:
+      - description: TX DMA Channel
+      - description: RX DMA Channel
+    minItems: 1
+
+  dma-names:
+    items:
+      - const: tx
+      - const: rx
+    minItems: 1
+
+  "#sound-dai-cells":
+    const: 0
+
+allOf:
+  - $ref: dai-common.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: starfive,jh8100-i2s
+    then:
+      properties:
+        starfive,i2s-max-channels:
+          description:
+            Number of I2S max stereo channels supported on the StarFive
+            JH8100 SoC.
+          $ref: /schemas/types.yaml#/definitions/uint32
+          enum: [2, 4, 8]
+      required:
+        - starfive,i2s-max-channels
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - resets
+
+oneOf:
+  - required:
+      - dmas
+      - dma-names
+  - required:
+      - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2s@122b0000 {
+      compatible = "cdns,i2s-mc";
+      reg = <0x122b0000 0x1000>;
+      clocks = <&syscrg_ne 133>,
+               <&syscrg_ne 170>,
+               <&syscrg 50>;
+      clock-names = "bclk", "icg",
+                    "mclk_inner";
+      resets = <&syscrg_ne 43>;
+      dmas = <&dma 7>, <&dma 6>;
+      dma-names = "tx", "rx";
+      #sound-dai-cells = <0>;
+    };
-- 
2.25.1

[PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller

[Xingyu Wu]

Add the drivers of Cadence Multi-Channel I2S Controller.

The Cadence I2S Controller implements a function of the multi-channel
(up to 8-channel) bus. Each stereo channel combines functions of a
transmitter and a receiver. Each channel has independent and internal
gating, clock and interruption control. It alos support some of these
channels are used as playback and others can also be used as record
in the same time.

The I2S-MC is used on the StarFive JH8100 SoC and add the compatible
for this.

Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
 MAINTAINERS                      |   6 +
 sound/soc/Kconfig                |   1 +
 sound/soc/Makefile               |   1 +
 sound/soc/cdns/Kconfig           |  18 +
 sound/soc/cdns/Makefile          |   3 +
 sound/soc/cdns/cdns-i2s-mc-pcm.c | 262 +++++++++++
 sound/soc/cdns/cdns-i2s-mc.c     | 724 +++++++++++++++++++++++++++++++
 sound/soc/cdns/cdns-i2s-mc.h     | 157 +++++++
 8 files changed, 1172 insertions(+)
 create mode 100644 sound/soc/cdns/Kconfig
 create mode 100644 sound/soc/cdns/Makefile
 create mode 100644 sound/soc/cdns/cdns-i2s-mc-pcm.c
 create mode 100644 sound/soc/cdns/cdns-i2s-mc.c
 create mode 100644 sound/soc/cdns/cdns-i2s-mc.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 1aabf1c15bb3..640dd70470e8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4560,6 +4560,12 @@ L:	linux-mm@kvack.org
 S:	Maintained
 F:	tools/testing/selftests/cachestat/test_cachestat.c
 
+CADENCE MULTI CHANNEL I2S CONTROLLER DRIVER
+M:	Xingyu Wu <xingyu.wu@starfivetech.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
+F:	sound/soc/cdns/cdns-i2s-mc*
+
 CADENCE MIPI-CSI2 BRIDGES
 M:	Maxime Ripard <mripard@kernel.org>
 L:	linux-media@vger.kernel.org
diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
index 439fa631c342..99133baff518 100644
--- a/sound/soc/Kconfig
+++ b/sound/soc/Kconfig
@@ -83,6 +83,7 @@ source "sound/soc/apple/Kconfig"
 source "sound/soc/atmel/Kconfig"
 source "sound/soc/au1x/Kconfig"
 source "sound/soc/bcm/Kconfig"
+source "sound/soc/cdns/Kconfig"
 source "sound/soc/cirrus/Kconfig"
 source "sound/soc/dwc/Kconfig"
 source "sound/soc/fsl/Kconfig"
diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index 8376fdb217ed..d0d3d8d22eee 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_SND_SOC)	+= amd/
 obj-$(CONFIG_SND_SOC)	+= atmel/
 obj-$(CONFIG_SND_SOC)	+= au1x/
 obj-$(CONFIG_SND_SOC)	+= bcm/
+obj-$(CONFIG_SND_SOC)	+= cdns/
 obj-$(CONFIG_SND_SOC)	+= cirrus/
 obj-$(CONFIG_SND_SOC)	+= dwc/
 obj-$(CONFIG_SND_SOC)	+= fsl/
diff --git a/sound/soc/cdns/Kconfig b/sound/soc/cdns/Kconfig
new file mode 100644
index 000000000000..61ef718ebfe7
--- /dev/null
+++ b/sound/soc/cdns/Kconfig
@@ -0,0 +1,18 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config SND_SOC_CADENCE_I2S_MC
+        tristate "Cadence I2S Multi-Channel Controller Device Driver"
+	depends on HAVE_CLK
+        select SND_SOC_GENERIC_DMAENGINE_PCM
+        help
+         Say Y or M if you want to add support for I2S driver for the
+         Cadence Multi-Channel I2S Controller device. The device supports
+         up to a maximum of 8 channels each for play and record.
+
+config SND_SOC_CADENCE_I2S_MC_PCM
+        bool "PCM PIO extension for CDNS I2S MC driver"
+        depends on SND_SOC_CADENCE_I2S_MC
+        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 CDNS I2S-MC devices that
+         don't use DMA controller.
diff --git a/sound/soc/cdns/Makefile b/sound/soc/cdns/Makefile
new file mode 100644
index 000000000000..ca8c040e21df
--- /dev/null
+++ b/sound/soc/cdns/Makefile
@@ -0,0 +1,3 @@
+obj-$(CONFIG_SND_SOC_CADENCE_I2S_MC) += cdns-i2s.o
+cdns-i2s-y := cdns-i2s-mc.o
+cdns-i2s-$(CONFIG_SND_SOC_CADENCE_I2S_MC_PCM) += cdns-i2s-mc-pcm.o
diff --git a/sound/soc/cdns/cdns-i2s-mc-pcm.c b/sound/soc/cdns/cdns-i2s-mc-pcm.c
new file mode 100644
index 000000000000..95ee9ad759a5
--- /dev/null
+++ b/sound/soc/cdns/cdns-i2s-mc-pcm.c
@@ -0,0 +1,262 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Cadence Multi-Channel I2S controller PCM driver
+ *
+ * Copyright (c) 2022-2023 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/io.h>
+#include <linux/rcupdate.h>
+#include <sound/pcm_params.h>
+#include "cdns-i2s-mc.h"
+
+#define PERIOD_BYTES_MIN	4096
+#define BUFFER_BYTES_MAX	(3 * 2 * 8 * PERIOD_BYTES_MIN)
+#define PERIODS_MIN		2
+
+static unsigned int cdns_i2s_pcm_tx(struct cdns_i2s_dev *dev,
+				    struct snd_pcm_runtime *runtime,
+				    unsigned int tx_ptr, bool *period_elapsed,
+				    snd_pcm_format_t format)
+{
+	unsigned int period_pos = tx_ptr % runtime->period_size;
+	const u16 (*p16)[2] = (void *)runtime->dma_area;
+	const u32 (*p32)[2] = (void *)runtime->dma_area;
+	u32 data[2];
+	int i;
+
+	for (i = 0; i < CDNS_I2S_FIFO_DEPTH; i++) {
+		if (format == SNDRV_PCM_FORMAT_S16_LE) {
+			data[0] = p16[tx_ptr][0];
+			data[1] = p16[tx_ptr][1];
+		} else if (format == SNDRV_PCM_FORMAT_S32_LE) {
+			data[0] = p32[tx_ptr][0];
+			data[1] = p32[tx_ptr][1];
+		}
+
+		iowrite32(data[0], dev->base + CDNS_FIFO_MEM);
+		iowrite32(data[1], dev->base + CDNS_FIFO_MEM);
+		period_pos++;
+		if (++tx_ptr >= runtime->buffer_size)
+			tx_ptr = 0;
+	}
+
+	*period_elapsed = period_pos >= runtime->period_size;
+	return tx_ptr;
+}
+
+static unsigned int cdns_i2s_pcm_rx(struct cdns_i2s_dev *dev,
+				    struct snd_pcm_runtime *runtime,
+				    unsigned int rx_ptr, bool *period_elapsed,
+				    snd_pcm_format_t format)
+{
+	unsigned int period_pos = rx_ptr % runtime->period_size;
+	u16 (*p16)[2] = (void *)runtime->dma_area;
+	u32 (*p32)[2] = (void *)runtime->dma_area;
+	u32 data[2];
+	int i;
+
+	for (i = 0; i < CDNS_I2S_FIFO_DEPTH; i++) {
+		data[0] = ioread32(dev->base + CDNS_FIFO_MEM);
+		data[1] = ioread32(dev->base + CDNS_FIFO_MEM);
+		if (format == SNDRV_PCM_FORMAT_S16_LE) {
+			p16[rx_ptr][0] = data[0];
+			p16[rx_ptr][1] = data[1];
+		} else if (format == SNDRV_PCM_FORMAT_S32_LE) {
+			p32[rx_ptr][0] = data[0];
+			p32[rx_ptr][1] = data[1];
+		}
+
+		period_pos++;
+		if (++rx_ptr >= runtime->buffer_size)
+			rx_ptr = 0;
+	}
+
+	*period_elapsed = period_pos >= runtime->period_size;
+	return rx_ptr;
+}
+
+static const struct snd_pcm_hardware cdns_i2s_pcm_hardware = {
+	.info = SNDRV_PCM_INFO_INTERLEAVED |
+		SNDRV_PCM_INFO_MMAP |
+		SNDRV_PCM_INFO_MMAP_VALID |
+		SNDRV_PCM_INFO_BLOCK_TRANSFER |
+		SNDRV_PCM_INFO_PAUSE |
+		SNDRV_PCM_INFO_RESUME,
+	.rates = SNDRV_PCM_RATE_8000 |
+		SNDRV_PCM_RATE_11025 |
+		SNDRV_PCM_RATE_16000 |
+		SNDRV_PCM_RATE_22050 |
+		SNDRV_PCM_RATE_32000 |
+		SNDRV_PCM_RATE_44100 |
+		SNDRV_PCM_RATE_48000,
+	.rate_min = 8000,
+	.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,
+};
+
+static void cdns_i2s_pcm_transfer(struct cdns_i2s_dev *dev, bool push)
+{
+	struct snd_pcm_substream *substream;
+	bool active, period_elapsed;
+
+	rcu_read_lock();
+	if (push)
+		substream = rcu_dereference(dev->tx_substream);
+	else
+		substream = rcu_dereference(dev->rx_substream);
+
+	active = substream && snd_pcm_running(substream);
+	if (active) {
+		unsigned int ptr;
+		unsigned int new_ptr;
+
+		if (push) {
+			ptr = READ_ONCE(dev->tx_ptr);
+			new_ptr = dev->tx_fn(dev, substream->runtime, ptr,
+					     &period_elapsed, dev->format);
+			cmpxchg(&dev->tx_ptr, ptr, new_ptr);
+		} else {
+			ptr = READ_ONCE(dev->rx_ptr);
+			new_ptr = dev->rx_fn(dev, substream->runtime, ptr,
+					     &period_elapsed, dev->format);
+			cmpxchg(&dev->rx_ptr, ptr, new_ptr);
+		}
+
+		if (period_elapsed)
+			snd_pcm_period_elapsed(substream);
+	}
+	rcu_read_unlock();
+}
+
+void cdns_i2s_pcm_push_tx(struct cdns_i2s_dev *dev)
+{
+	cdns_i2s_pcm_transfer(dev, true);
+}
+
+void cdns_i2s_pcm_pop_rx(struct cdns_i2s_dev *dev)
+{
+	cdns_i2s_pcm_transfer(dev, false);
+}
+
+static int cdns_i2s_pcm_open(struct snd_soc_component *component,
+			     struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
+	struct cdns_i2s_dev *dev = snd_soc_dai_get_drvdata(snd_soc_rtd_to_cpu(rtd, 0));
+
+	snd_soc_set_runtime_hwparams(substream, &cdns_i2s_pcm_hardware);
+	snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
+	runtime->private_data = dev;
+
+	return 0;
+}
+
+static int cdns_i2s_pcm_close(struct snd_soc_component *component,
+			      struct snd_pcm_substream *substream)
+{
+	synchronize_rcu();
+	return 0;
+}
+
+static int cdns_i2s_pcm_hw_params(struct snd_soc_component *component,
+				  struct snd_pcm_substream *substream,
+				  struct snd_pcm_hw_params *hw_params)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct cdns_i2s_dev *dev = runtime->private_data;
+
+	dev->format = params_format(hw_params);
+	dev->tx_fn = cdns_i2s_pcm_tx;
+	dev->rx_fn = cdns_i2s_pcm_rx;
+
+	return 0;
+}
+
+static int cdns_i2s_pcm_trigger(struct snd_soc_component *component,
+				struct snd_pcm_substream *substream, int cmd)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct cdns_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:
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+			WRITE_ONCE(dev->tx_ptr, 0);
+			rcu_assign_pointer(dev->tx_substream, substream);
+		} else {
+			WRITE_ONCE(dev->rx_ptr, 0);
+			rcu_assign_pointer(dev->rx_substream, substream);
+		}
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+			rcu_assign_pointer(dev->tx_substream, NULL);
+		else
+			rcu_assign_pointer(dev->rx_substream, NULL);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static snd_pcm_uframes_t cdns_i2s_pcm_pointer(struct snd_soc_component *component,
+					      struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct cdns_i2s_dev *dev = runtime->private_data;
+	snd_pcm_uframes_t pos;
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		pos = READ_ONCE(dev->tx_ptr);
+	else
+		pos = READ_ONCE(dev->rx_ptr);
+
+	return pos < runtime->buffer_size ? pos : 0;
+}
+
+static int cdns_i2s_pcm_new(struct snd_soc_component *component,
+			    struct snd_soc_pcm_runtime *rtd)
+{
+	size_t size = cdns_i2s_pcm_hardware.buffer_bytes_max;
+
+	snd_pcm_set_managed_buffer_all(rtd->pcm,
+				       SNDRV_DMA_TYPE_CONTINUOUS,
+				       NULL, size, size);
+
+	return 0;
+}
+
+static const struct snd_soc_component_driver cdns_i2s_pcm_component = {
+	.open		= cdns_i2s_pcm_open,
+	.close		= cdns_i2s_pcm_close,
+	.hw_params	= cdns_i2s_pcm_hw_params,
+	.trigger	= cdns_i2s_pcm_trigger,
+	.pointer	= cdns_i2s_pcm_pointer,
+	.pcm_construct	= cdns_i2s_pcm_new,
+};
+
+int cdns_i2s_pcm_register(struct platform_device *pdev)
+{
+	return devm_snd_soc_register_component(&pdev->dev,
+					       &cdns_i2s_pcm_component,
+					       NULL, 0);
+}
diff --git a/sound/soc/cdns/cdns-i2s-mc.c b/sound/soc/cdns/cdns-i2s-mc.c
new file mode 100644
index 000000000000..0038f47fd74b
--- /dev/null
+++ b/sound/soc/cdns/cdns-i2s-mc.c
@@ -0,0 +1,724 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Cadence Multi-Channel I2S controller driver
+ *
+ * The Cadence I2S Controller implements a function of the multi-channel
+ * (up to 8-channel) bus. Each stereo channel combines functions of a
+ * transmitter and a receiver. Each channel has independent and internal
+ * gating, clock and interruption control. It alos support some of these
+ * channels are used as playback and others can also be used as record
+ * in the same time.
+ *
+ * Copyright (c) 2023 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+
+#include "cdns-i2s-mc.h"
+
+static void cdns_i2s_set_fifo_mask(struct cdns_i2s_dev *i2s, u32 type)
+{
+	unsigned int val = readl(i2s->base + CDNS_CID_CTRL);
+
+	val &= ~CDNS_I2S_IT_ALL;
+	val |= type;
+	writel(val, i2s->base + CDNS_CID_CTRL);
+}
+
+static inline void cdns_i2s_clear_int(struct cdns_i2s_dev *i2s)
+{
+	writel(0, i2s->base + CDNS_I2S_INTR_STAT);
+}
+
+static int cdns_i2s_reset_mask(struct cdns_i2s_dev *i2s, u32 mask)
+{
+	unsigned int val = readl(i2s->base + CDNS_I2S_CTRL);
+
+	val &= ~mask;
+	writel(val, i2s->base + CDNS_I2S_CTRL);
+
+	/* Wait for the reset bit to done and is set to 1 */
+	return readl_poll_timeout_atomic(i2s->base + CDNS_I2S_CTRL, val,
+					 (val & mask), 0,
+					 CDNS_FIFO_ACK_TIMEOUT_US);
+}
+
+/* Reset for TX and RX control unit  */
+static void cdns_i2s_reset_txrx_unit(struct cdns_i2s_dev *i2s)
+{
+	unsigned int val = readl(i2s->base + CDNS_I2S_CTRL);
+
+	val |= CDNS_I2S_CTRL_TXRX_RST;
+	writel(val, i2s->base + CDNS_I2S_CTRL);
+}
+
+static void cdns_i2s_set_ms_mode(struct cdns_i2s_dev *i2s)
+{
+	unsigned int val = readl(i2s->base + CDNS_I2S_CTRL);
+
+	val &= ~(CDNS_I2S_CTRL_T_MS_MASK | CDNS_I2S_CTRL_R_MS_MASK);
+	val |= (FIELD_PREP(CDNS_I2S_CTRL_T_MS_MASK, i2s->tx_sync_ms_mode) |
+		FIELD_PREP(CDNS_I2S_CTRL_R_MS_MASK, i2s->rx_sync_ms_mode));
+
+	writel(val, i2s->base + CDNS_I2S_CTRL);
+}
+
+/* The threshold of almost empty & full config */
+static void cdns_i2s_set_aempty_afull_th(struct cdns_i2s_dev *i2s,
+					 unsigned int aempty,
+					 unsigned int afull)
+{
+	unsigned int val = aempty | (afull << CDNS_TRFIFO_CTRL_AFULL_SHIFT);
+
+	writel(val, i2s->base + CDNS_TFIFO_CTRL);
+	writel(val, i2s->base + CDNS_RFIFO_CTRL);
+}
+
+static void cdns_i2s_set_channel_strobes(struct cdns_i2s_dev *i2s,
+					 u32 ch, bool strobe)
+{
+	unsigned int val = readl(i2s->base + CDNS_CID_CTRL);
+
+	/* Active Low */
+	if (strobe)
+		val &= ~ch;
+	else
+		val |= ch;
+
+	writel(val, i2s->base + CDNS_CID_CTRL);
+}
+
+/* Enable TX or RX clock */
+static void cdns_i2s_enable_clock(struct cdns_i2s_dev *i2s,
+				  bool is_rx)
+{
+	unsigned int val = readl(i2s->base + CDNS_CID_CTRL);
+	unsigned int mask = (is_rx ? CDNS_CID_CTRL_STROBE_TX :
+			     CDNS_CID_CTRL_STROBE_RX);
+
+	/* Active Low */
+	val &= ~mask;
+	writel(val, i2s->base + CDNS_CID_CTRL);
+}
+
+static void cdns_i2s_set_transmitter_receiver(struct cdns_i2s_dev *i2s,
+					      u32 ch, bool is_transmit)
+{
+	unsigned int val = readl(i2s->base + CDNS_I2S_CTRL);
+
+	/* 1: Transmitter, 0: Receiver */
+	if (is_transmit)
+		val |= (ch << CDNS_I2S_CTRL_TR_CFG_0_SHIFT);
+	else
+		val &= ~(ch << CDNS_I2S_CTRL_TR_CFG_0_SHIFT);
+
+	writel(val, i2s->base + CDNS_I2S_CTRL);
+}
+
+static irqreturn_t cdns_i2s_irq_handler(int irq, void *data)
+{
+	struct cdns_i2s_dev *i2s = data;
+	unsigned int val = readl(i2s->base + CDNS_I2S_INTR_STAT);
+	irqreturn_t ret = IRQ_NONE;
+
+	cdns_i2s_clear_int(i2s);
+
+	if (val & CDNS_I2S_STAT_TX_UNDERRUN)
+		dev_err(i2s->dev, "TX underrun on channel %ld!\n",
+			FIELD_GET(CDNS_I2S_STAT_UNDERR_CODE, val));
+
+	if (val & CDNS_I2S_STAT_RX_OVERRUN)
+		dev_err(i2s->dev, "RX overrun on channel %ld!\n",
+			FIELD_GET(CDNS_I2S_STAT_OVERR_CODE, val));
+
+	/* FIFO is empty when playback start and I2S also need to push the data. */
+	if (val & (CDNS_I2S_STAT_TFIFO_AEMPTY | CDNS_I2S_STAT_TFIFO_EMPTY)) {
+		cdns_i2s_pcm_push_tx(i2s);
+		ret = IRQ_HANDLED;
+	}
+
+	if (val & CDNS_I2S_STAT_RFIFO_AFULL) {
+		cdns_i2s_pcm_pop_rx(i2s);
+		ret = IRQ_HANDLED;
+	}
+
+	return ret;
+}
+
+static void cdns_i2s_enable_channel(struct cdns_i2s_dev *i2s,
+				    u32 ch, bool enable)
+{
+	unsigned int val = readl(i2s->base + CDNS_I2S_CTRL);
+
+	/* Active High */
+	if (enable)
+		val |= ch;
+	else
+		val &= ~ch;
+
+	writel(val, i2s->base + CDNS_I2S_CTRL);
+}
+
+/* Bit masking all interrupt requests */
+static void cdns_i2s_set_all_irq_mask(struct cdns_i2s_dev *i2s, bool mask)
+{
+	unsigned int val = readl(i2s->base + CDNS_CID_CTRL);
+
+	/* Active Low: IRQ are masked */
+	if (mask)
+		val &= ~CDNS_CID_CTRL_INTREQ_MASK;
+	else
+		val |= CDNS_CID_CTRL_INTREQ_MASK;
+
+	writel(val, i2s->base + CDNS_CID_CTRL);
+}
+
+static void cdns_i2s_enable_channel_int(struct cdns_i2s_dev *i2s,
+					u32 ch, bool enable)
+{
+	unsigned int val = readl(i2s->base + CDNS_CID_CTRL);
+
+	/* Active High */
+	if (enable)
+		val |= (ch << CDNS_CID_CTRL_I2S_MASK_0_SHIFT);
+	else
+		val &= ~(ch << CDNS_CID_CTRL_I2S_MASK_0_SHIFT);
+
+	writel(val, i2s->base + CDNS_CID_CTRL);
+}
+
+static void cdns_i2s_channel_start(struct cdns_i2s_dev *i2s,
+				   u32 ch, bool is_transmit)
+{
+	cdns_i2s_set_transmitter_receiver(i2s, ch, is_transmit);
+	cdns_i2s_enable_channel(i2s, ch, true);
+	cdns_i2s_set_channel_strobes(i2s, ch, true);
+	if (i2s->irq >= 0)
+		cdns_i2s_enable_channel_int(i2s, ch, true);
+}
+
+static void cdns_i2s_channel_stop(struct cdns_i2s_dev *i2s, u32 ch)
+{
+	cdns_i2s_enable_channel(i2s, ch, false);
+	if (i2s->irq >= 0)
+		cdns_i2s_enable_channel_int(i2s, ch, false);
+}
+
+static int cdns_i2s_start(struct cdns_i2s_dev *i2s,
+			  struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	unsigned char max_ch = i2s->max_channels;
+	unsigned char i2s_ch;
+	int i;
+
+	/* Each channel is stereo */
+	i2s_ch = runtime->channels / 2;
+	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
+		if ((i2s_ch + i2s->tx_using_channels) > max_ch) {
+			dev_err(i2s->dev,
+				"Max %d channels: using %d for TX, do not support %d for RX\n",
+				max_ch, i2s->tx_using_channels, i2s_ch);
+			return -ENOMEM;
+		}
+
+		i2s->rx_using_channels = i2s_ch;
+		/* Enable channels from 0 to 'max_ch' as tx */
+		for (i = 0; i < i2s_ch; i++)
+			cdns_i2s_channel_start(i2s, CDNS_I2S_CM_0 << i,
+					       CDNS_I2S_TC_RECEIVER);
+
+	} else {
+		if ((i2s_ch + i2s->rx_using_channels) > max_ch) {
+			dev_err(i2s->dev,
+				"Max %d channels: using %d for RX, do not support %d for TX\n",
+				max_ch, i2s->rx_using_channels, i2s_ch);
+			return -ENOMEM;
+		}
+
+		i2s->tx_using_channels = i2s_ch;
+		/* Enable channels from 'max_ch' to 0 as rx */
+		for (i = (max_ch - 1); i > (max_ch - i2s_ch - 1); i--) {
+			if (i < 0)
+				return -EINVAL;
+
+			cdns_i2s_channel_start(i2s, CDNS_I2S_CM_0 << i,
+					       CDNS_I2S_TC_TRANSMITTER);
+		}
+	}
+	cdns_i2s_enable_clock(i2s, substream->stream);
+
+	if (i2s->irq >= 0)
+		cdns_i2s_set_all_irq_mask(i2s, false);
+
+	cdns_i2s_clear_int(i2s);
+
+	return 0;
+}
+
+static int cdns_i2s_stop(struct cdns_i2s_dev *i2s,
+			 struct snd_pcm_substream *substream)
+{
+	unsigned char i2s_ch;
+	int i;
+
+	cdns_i2s_clear_int(i2s);
+
+	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
+		i2s_ch = i2s->rx_using_channels;
+		for (i = 0; i < i2s_ch; i++)
+			cdns_i2s_channel_stop(i2s, (CDNS_I2S_CM_0 << i));
+
+		i2s->rx_using_channels = 0;
+	} else {
+		unsigned char max_ch = i2s->max_channels;
+
+		i2s_ch = i2s->tx_using_channels;
+		for (i = (max_ch - 1); i > (max_ch - i2s_ch - 1); i--) {
+			if (i < 0)
+				return -EINVAL;
+
+			cdns_i2s_channel_stop(i2s, (CDNS_I2S_CM_0 << i));
+		}
+
+		i2s->tx_using_channels = 0;
+	}
+
+	if (i2s->irq >= 0 && !i2s->tx_using_channels && !i2s->rx_using_channels)
+		cdns_i2s_set_all_irq_mask(i2s, true);
+
+	return 0;
+}
+
+static int cdns_i2s_startup(struct snd_pcm_substream *substream,
+			    struct snd_soc_dai *dai)
+{
+	struct cdns_i2s_dev *i2s = snd_soc_dai_get_drvdata(dai);
+	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
+	struct snd_soc_dai_link *dai_link = rtd->dai_link;
+
+	if (i2s->irq < 0)
+		dai_link->trigger_stop = SND_SOC_TRIGGER_ORDER_LDC;
+
+	return 0;
+}
+
+static void cdns_i2s_config(struct cdns_i2s_dev *i2s, int stream)
+{
+	unsigned int val = readl(i2s->base + CDNS_I2S_SRR);
+
+	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		val &= ~(CDNS_I2S_SRR_TRATE_MASK | CDNS_I2S_SRR_TRESOLUTION_MASK);
+		val |= (FIELD_PREP(CDNS_I2S_SRR_TRATE_MASK, i2s->sample_rate_param) |
+			FIELD_PREP(CDNS_I2S_SRR_TRESOLUTION_MASK, (i2s->resolution - 1)));
+	} else {
+		val &= ~(CDNS_I2S_SRR_RRATE_MASK | CDNS_I2S_SRR_RRESOLUTION_MASK);
+		val |= (FIELD_PREP(CDNS_I2S_SRR_RRATE_MASK, i2s->sample_rate_param) |
+			FIELD_PREP(CDNS_I2S_SRR_RRESOLUTION_MASK, (i2s->resolution - 1)));
+	}
+
+	writel(val, i2s->base + CDNS_I2S_SRR);
+}
+
+static int cdns_i2s_hw_params(struct snd_pcm_substream *substream,
+			      struct snd_pcm_hw_params *params,
+			      struct snd_soc_dai *dai)
+{
+	struct cdns_i2s_dev *i2s = snd_soc_dai_get_drvdata(dai);
+	unsigned int sample_rate = params_rate(params);
+	unsigned int channels = params_channels(params);
+	unsigned int fclk_hz = clk_get_rate(i2s->clks[2].clk); /* mclk_inner */
+	unsigned int bclk_rate;
+	int ret;
+	struct snd_dmaengine_dai_dma_data *dma_data;
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		dma_data = &i2s->tx_dma_data;
+	else
+		dma_data = &i2s->rx_dma_data;
+
+	switch (sample_rate) {
+	case 8000:
+	case 11025:
+	case 16000:
+	case 22050:
+	case 32000:
+	case 44100:
+	case 48000:
+		bclk_rate = sample_rate * 64;
+		break;
+	default:
+		dev_err(dai->dev, "%d rate not supported\n", sample_rate);
+		return -EINVAL;
+	}
+
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		dma_data->addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
+		break;
+	case SNDRV_PCM_FORMAT_S32_LE:
+		dma_data->addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+		break;
+	default:
+		dev_err(i2s->dev, "unsupported PCM fmt\n");
+		return -EINVAL;
+	}
+
+	ret = clk_set_rate(i2s->clks[0].clk, bclk_rate); /* bclk */
+	if (ret < 0) {
+		dev_err(i2s->dev, "Can't set i2s bclk: %d\n", ret);
+		return ret;
+	}
+
+	i2s->resolution = params_width(params);
+	i2s->sample_rate_param = fclk_hz / (sample_rate * channels * 32);
+	cdns_i2s_config(i2s, substream->stream);
+
+	if (i2s->irq < 0)
+		snd_soc_dai_set_dma_data(dai, substream, dma_data);
+
+	return 0;
+}
+
+static int cdns_i2s_trigger(struct snd_pcm_substream *substream,
+			    int cmd, struct snd_soc_dai *dai)
+{
+	struct cdns_i2s_dev *i2s = snd_soc_dai_get_drvdata(dai);
+	int ret = 0;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		ret = cdns_i2s_start(i2s, substream);
+		break;
+
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		ret = cdns_i2s_stop(i2s, substream);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static int cdns_i2s_set_fmt(struct snd_soc_dai *cpu_dai,
+			    unsigned int fmt)
+{
+	struct cdns_i2s_dev *i2s = snd_soc_dai_get_drvdata(cpu_dai);
+	int ret = 0;
+
+	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
+	case SND_SOC_DAIFMT_CBM_CFM:
+		i2s->tx_sync_ms_mode = CDNS_I2S_MASTER_MODE;
+		i2s->rx_sync_ms_mode = CDNS_I2S_MASTER_MODE;
+		cdns_i2s_set_ms_mode(i2s);
+		break;
+	case SND_SOC_DAIFMT_CBS_CFS:
+		i2s->tx_sync_ms_mode = CDNS_I2S_SLAVE_MODE;
+		i2s->rx_sync_ms_mode = CDNS_I2S_SLAVE_MODE;
+		cdns_i2s_set_ms_mode(i2s);
+		break;
+	case SND_SOC_DAIFMT_CBM_CFS:
+	case SND_SOC_DAIFMT_CBS_CFM:
+		ret = -EINVAL;
+		break;
+	default:
+		dev_dbg(i2s->dev, "Invalid master/slave format\n");
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+static int cdns_i2s_dai_probe(struct snd_soc_dai *dai)
+{
+	struct cdns_i2s_dev *i2s = snd_soc_dai_get_drvdata(dai);
+	struct snd_dmaengine_dai_dma_data *tx = &i2s->tx_dma_data;
+	struct snd_dmaengine_dai_dma_data *rx = &i2s->rx_dma_data;
+
+	if (i2s->irq >= 0)
+		return 0;
+
+	/* Buswidth will be set by framework */
+	tx->addr_width = DMA_SLAVE_BUSWIDTH_UNDEFINED;
+	tx->addr = i2s->phybase + CDNS_FIFO_MEM;
+	tx->maxburst = 16;
+	tx->fifo_size = 16;
+
+	rx->addr_width = DMA_SLAVE_BUSWIDTH_UNDEFINED;
+	rx->addr = i2s->phybase + CDNS_FIFO_MEM;
+	rx->maxburst = 16;
+	rx->fifo_size = 16;
+
+	snd_soc_dai_init_dma_data(dai, tx, rx);
+
+	return 0;
+}
+
+static const struct snd_soc_component_driver cdns_i2s_component = {
+	.name = "cdns-i2s-mc",
+};
+
+static const struct snd_soc_dai_ops cdns_i2s_dai_ops = {
+	.probe		= cdns_i2s_dai_probe,
+	.startup	= cdns_i2s_startup,
+	.hw_params	= cdns_i2s_hw_params,
+	.trigger	= cdns_i2s_trigger,
+	.set_fmt	= cdns_i2s_set_fmt,
+};
+
+static struct snd_soc_dai_driver cdns_i2s_dai = {
+	.name = "cdns-i2s-mc",
+	.id = 0,
+	.playback = {
+		.stream_name = "Playback",
+		.channels_min = 2,
+		.channels_max = 8,
+		.rates = SNDRV_PCM_RATE_8000_48000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE |
+			   SNDRV_PCM_FMTBIT_S32_LE,
+	},
+	.capture = {
+		.stream_name = "Capture",
+		.channels_min = 2,
+		.channels_max = 8,
+		.rates = SNDRV_PCM_RATE_8000_48000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE |
+			   SNDRV_PCM_FMTBIT_S32_LE,
+	},
+	.ops = &cdns_i2s_dai_ops,
+	.symmetric_rate = 1,
+};
+
+#ifdef CONFIG_PM
+static int cdns_i2s_runtime_suspend(struct device *dev)
+{
+	struct cdns_i2s_dev *i2s = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(i2s->clks[1].clk); /* ICG clock */
+	return 0;
+}
+
+static int cdns_i2s_runtime_resume(struct device *dev)
+{
+	struct cdns_i2s_dev *i2s = dev_get_drvdata(dev);
+
+	return clk_prepare_enable(i2s->clks[1].clk); /* ICG clock */
+}
+#endif
+
+static int cdns_i2s_crg_init(struct cdns_i2s_dev *i2s)
+{
+	struct reset_control *reset = devm_reset_control_get_exclusive(i2s->dev, NULL);
+	int ret;
+
+	if (IS_ERR(reset))
+		return dev_err_probe(i2s->dev, PTR_ERR(reset), "failed to get i2s resets\n");
+
+	i2s->clks[0].id = "bclk";
+	i2s->clks[1].id = "icg";
+	i2s->clks[2].id = "mclk_inner";
+
+	ret = devm_clk_bulk_get(i2s->dev, ARRAY_SIZE(i2s->clks), i2s->clks);
+	if (ret)
+		return dev_err_probe(i2s->dev, ret, "failed to get i2s clocks\n");
+
+	ret = clk_prepare_enable(i2s->clks[1].clk); /* ICG clock */
+	if (ret)
+		return dev_err_probe(i2s->dev, ret, "failed to enable icg clock\n");
+
+	ret = reset_control_deassert(reset);
+	if (ret)
+		goto rst_err;
+
+	return 0;
+
+rst_err:
+	clk_disable_unprepare(i2s->clks[1].clk);
+	return ret;
+}
+
+static int cdns_i2s_init(struct cdns_i2s_dev *i2s)
+{
+	int ret	= cdns_i2s_crg_init(i2s);
+	unsigned int tmp;
+
+	if (ret)
+		return ret;
+
+	/* Software reset i2s controller */
+	ret = cdns_i2s_reset_mask(i2s, CDNS_I2S_CTRL_SFR_RST_MASK);
+	if (ret) {
+		dev_err(i2s->dev, "Failed to reset I2S.\n");
+		return ret;
+	}
+
+	/* reset TX FIFO */
+	ret = cdns_i2s_reset_mask(i2s, CDNS_I2S_CTRL_TFIFO_RST_MASK);
+	if (ret) {
+		dev_err(i2s->dev, "Failed to reset tx fifo.\n");
+		return ret;
+	}
+
+	/* reset RX FIFO */
+	ret = cdns_i2s_reset_mask(i2s, CDNS_I2S_CTRL_RFIFO_RST_MASK);
+	if (ret) {
+		dev_err(i2s->dev, "Failed to reset rx fifo.\n");
+		return ret;
+	}
+
+	/* Default master mode to init */
+	i2s->tx_sync_ms_mode = CDNS_I2S_MASTER_MODE;
+	i2s->rx_sync_ms_mode = CDNS_I2S_MASTER_MODE;
+	cdns_i2s_set_ms_mode(i2s);
+
+	/* Should reset it after setting Master/Slave mode */
+	cdns_i2s_reset_txrx_unit(i2s);
+	cdns_i2s_clear_int(i2s);
+
+	cdns_i2s_set_aempty_afull_th(i2s, (CDNS_I2S_FIFO_DEPTH / 4),
+				     (CDNS_I2S_FIFO_DEPTH / 4 * 3));
+	cdns_i2s_set_fifo_mask(i2s, CDNS_I2S_IT_TFIFO_AEMPTY |
+			       CDNS_I2S_IT_RFIFO_AFULL);
+
+	i2s->rx_using_channels = 0;
+	i2s->tx_using_channels = 0;
+
+	/* starfive,i2s-max-channels is unique property for JH8100 SoC and default max channels 8 */
+	ret = device_property_read_u32(i2s->dev, "starfive,i2s-max-channels", &tmp);
+	if (ret) {
+		i2s->max_channels = CDNS_I2S_CHANNEL_MAX;
+	} else {
+		if (tmp > CDNS_I2S_CHANNEL_MAX) {
+			dev_err(i2s->dev,
+				"The number %d of max channels from DTS is out of range!\n",
+				tmp);
+			return -EINVAL;
+		}
+
+		i2s->max_channels = tmp;
+	}
+
+	return 0;
+}
+
+static int cdns_i2s_probe(struct platform_device *pdev)
+{
+	struct cdns_i2s_dev *i2s;
+	struct resource *res;
+	int ret;
+
+	i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
+	if (!i2s) {
+		ret = -ENOMEM;
+		goto err;
+	}
+	platform_set_drvdata(pdev, i2s);
+
+	i2s->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(i2s->base)) {
+		ret = PTR_ERR(i2s->base);
+		goto err;
+	}
+
+	i2s->dev = &pdev->dev;
+	i2s->phybase = res->start;
+
+	ret = cdns_i2s_init(i2s);
+	if (ret)
+		goto err;
+
+	i2s->irq = platform_get_irq(pdev, 0);
+	if (i2s->irq >= 0) {
+		ret = devm_request_irq(&pdev->dev, i2s->irq, cdns_i2s_irq_handler,
+				       0, pdev->name, i2s);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "request_irq failed\n");
+			goto err;
+		}
+	}
+
+	ret = devm_snd_soc_register_component(&pdev->dev,
+					      &cdns_i2s_component,
+					      &cdns_i2s_dai, 1);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "couldn't register component\n");
+		goto err;
+	}
+
+	if (i2s->irq >= 0)
+		ret = cdns_i2s_pcm_register(pdev);
+	else
+		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;
+	}
+
+	pm_runtime_enable(&pdev->dev);
+	if (pm_runtime_enabled(&pdev->dev))
+		cdns_i2s_runtime_suspend(&pdev->dev);
+
+	dev_dbg(&pdev->dev, "I2S supports %d stereo channels with %s.\n",
+		i2s->max_channels, ((i2s->irq < 0) ? "dma" : "interrupt"));
+
+	return 0;
+
+err:
+	return ret;
+}
+
+static int cdns_i2s_remove(struct platform_device *pdev)
+{
+	pm_runtime_disable(&pdev->dev);
+	if (!pm_runtime_status_suspended(&pdev->dev))
+		cdns_i2s_runtime_suspend(&pdev->dev);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id cdns_i2s_of_match[] = {
+	{ .compatible = "cdns,i2s-mc", },
+	{ .compatible = "starfive,jh8100-i2s", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, cdns_i2s_of_match);
+#endif
+
+static const struct dev_pm_ops cdns_i2s_pm_ops = {
+	SET_RUNTIME_PM_OPS(cdns_i2s_runtime_suspend,
+			   cdns_i2s_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+};
+
+static struct platform_driver cdns_i2s_driver = {
+	.probe   = cdns_i2s_probe,
+	.remove  = cdns_i2s_remove,
+	.driver  = {
+		.name = "cdns-i2s-mc",
+		.of_match_table = of_match_ptr(cdns_i2s_of_match),
+		.pm = &cdns_i2s_pm_ops,
+	},
+};
+
+module_platform_driver(cdns_i2s_driver);
+
+MODULE_AUTHOR("Xingyu Wu <xingyu.wu@starfivetech.com>");
+MODULE_DESCRIPTION("Cadence Multi-Channel I2S controller driver");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/cdns/cdns-i2s-mc.h b/sound/soc/cdns/cdns-i2s-mc.h
new file mode 100644
index 000000000000..1d85332dc4cb
--- /dev/null
+++ b/sound/soc/cdns/cdns-i2s-mc.h
@@ -0,0 +1,157 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Cadence Multi-Channel I2S Controller driver header file
+ *
+ * Copyright (c) 2023 StarFive Technology Co., Ltd.
+ * Author: Xingyu Wu <xingyu.wu@starfivetech.com>
+ */
+
+#ifndef __CDNS_I2S_MC_H
+#define __CDNS_I2S_MC_H
+
+#include <linux/clk.h>
+#include <sound/dmaengine_pcm.h>
+#include <sound/pcm.h>
+
+#define CDNS_I2S_FIFO_DEPTH		128
+#define CDNS_FIFO_ACK_TIMEOUT_US	200
+#define CDNS_I2S_CHANNEL_MAX		8
+
+/* I2S REGS */
+#define CDNS_I2S_CTRL			0x00
+#define CDNS_I2S_INTR_STAT		0x04
+#define CDNS_I2S_SRR			0x08
+#define CDNS_CID_CTRL			0x0c
+#define CDNS_TFIFO_CTRL			0x18
+#define CDNS_RFIFO_CTRL			0x1c
+#define CDNS_FIFO_MEM			0x3c
+
+/*
+ * I2S_CTRL: I2S transceiver control register
+ */
+#define CDNS_I2S_CTRL_TR_CFG_0_SHIFT	8
+#define CDNS_I2S_CTRL_SFR_RST_MASK	BIT(20)
+#define CDNS_I2S_CTRL_T_MS_MASK		BIT(21)
+#define CDNS_I2S_CTRL_R_MS_MASK		BIT(22)
+#define CDNS_I2S_CTRL_TFIFO_RST_MASK	BIT(23)
+#define CDNS_I2S_CTRL_RFIFO_RST_MASK	BIT(24)
+#define CDNS_I2S_CTRL_TXRX_RST		GENMASK(26, 25)
+
+/*
+ * I2S_INTR_STAT: I2S Interrupt status register
+ */
+#define CDNS_I2S_STAT_TX_UNDERRUN	BIT(0)
+#define CDNS_I2S_STAT_UNDERR_CODE	GENMASK(3, 1)
+#define CDNS_I2S_STAT_RX_OVERRUN	BIT(4)
+#define CDNS_I2S_STAT_OVERR_CODE	GENMASK(7, 5)
+#define CDNS_I2S_STAT_TFIFO_EMPTY	BIT(8)
+#define CDNS_I2S_STAT_TFIFO_AEMPTY	BIT(9)
+#define CDNS_I2S_STAT_RFIFO_AFULL	BIT(15)
+
+/*
+ * CID_CTRL: Clock strobes and interrupt masks control register
+ */
+#define CDNS_CID_CTRL_STROBE_TX		BIT(8)
+#define CDNS_CID_CTRL_STROBE_RX		BIT(9)
+#define CDNS_CID_CTRL_INTREQ_MASK	BIT(15)
+#define CDNS_CID_CTRL_I2S_MASK_0_SHIFT	16
+
+/*
+ * I2S_SRR: Sample rate and resolution control register
+ */
+#define CDNS_I2S_SRR_TRATE_MASK		GENMASK(9, 0)
+#define CDNS_I2S_SRR_RRATE_MASK		GENMASK(25, 16)
+#define CDNS_I2S_SRR_TRESOLUTION_MASK	GENMASK(15, 11)
+#define CDNS_I2S_SRR_RRESOLUTION_MASK	GENMASK(31, 27)
+
+/*
+ * TFIFO_CTRL & RFIFO_CTRL: The FIFO thresholds control register
+ * AEMPTY: [15:0]
+ * AFULL: [31:16]
+ */
+#define CDNS_TRFIFO_CTRL_AFULL_SHIFT	16
+
+enum cdns_i2s_channel_mask {
+	CDNS_I2S_CM_0   = BIT(0),
+	CDNS_I2S_CM_1   = BIT(1),
+	CDNS_I2S_CM_2   = BIT(2),
+	CDNS_I2S_CM_3   = BIT(3),
+	CDNS_I2S_CM_4   = BIT(4),
+	CDNS_I2S_CM_5   = BIT(5),
+	CDNS_I2S_CM_6   = BIT(6),
+	CDNS_I2S_CM_7   = BIT(7),
+	CDNS_I2S_CM_ALL = GENMASK(7, 0),
+};
+
+enum i2s_int_type {
+	CDNS_I2S_IT_TFIFO_EMPTY  = BIT(24),
+	CDNS_I2S_IT_TFIFO_AEMPTY = BIT(25),
+	CDNS_I2S_IT_TFIFO_FULL   = BIT(26),
+	CDNS_I2S_IT_TFIFO_AFULL  = BIT(27),
+	CDNS_I2S_IT_RFIFO_EMPTY  = BIT(28),
+	CDNS_I2S_IT_RFIFO_AEMPTY = BIT(29),
+	CDNS_I2S_IT_RFIFO_FULL   = BIT(30),
+	CDNS_I2S_IT_RFIFO_AFULL  = BIT(31),
+	CDNS_I2S_IT_ALL          = GENMASK(31, 24),
+};
+
+enum cdns_i2s_master_slave_mode {
+	CDNS_I2S_SLAVE_MODE = 0,
+	CDNS_I2S_MASTER_MODE = 1,
+};
+
+enum cdns_i2s_transmit_config {
+	CDNS_I2S_TC_RECEIVER = 0,
+	CDNS_I2S_TC_TRANSMITTER = 1,
+};
+
+struct cdns_i2s_dev {
+	struct device *dev;
+	struct clk_bulk_data clks[3];
+	void __iomem *base;
+	resource_size_t	phybase; /* the physical memory */
+	int irq;
+	unsigned int sample_rate_param;
+	unsigned char resolution;
+	unsigned char max_channels /* up to CDNS_I2S_CHANNEL_MAX */;
+	unsigned char tx_using_channels;
+	unsigned char rx_using_channels;
+
+	/*
+	 * Master (value '1') or slave (value '0') configuration bit
+	 * for unit synchronizing all transmitters(receivers) with I2S bus
+	 */
+	bool tx_sync_ms_mode;
+	bool rx_sync_ms_mode;
+	struct snd_dmaengine_dai_dma_data tx_dma_data;
+	struct snd_dmaengine_dai_dma_data rx_dma_data;
+
+#if IS_ENABLED(CONFIG_SND_SOC_CADENCE_I2S_MC_PCM)
+	struct snd_pcm_substream __rcu *tx_substream;
+	struct snd_pcm_substream __rcu *rx_substream;
+	unsigned int (*tx_fn)(struct cdns_i2s_dev *i2s,
+			      struct snd_pcm_runtime *runtime, unsigned int tx_ptr,
+			      bool *period_elapsed, snd_pcm_format_t format);
+	unsigned int (*rx_fn)(struct cdns_i2s_dev *dev,
+			      struct snd_pcm_runtime *runtime, unsigned int rx_ptr,
+			      bool *period_elapsed, snd_pcm_format_t format);
+	snd_pcm_format_t format;
+	unsigned int tx_ptr; /* next frame index in the sample buffer */
+	unsigned int rx_ptr;
+#endif
+};
+
+#if IS_ENABLED(CONFIG_SND_SOC_CADENCE_I2S_MC_PCM)
+void cdns_i2s_pcm_push_tx(struct cdns_i2s_dev *dev);
+void cdns_i2s_pcm_pop_rx(struct cdns_i2s_dev *dev);
+int cdns_i2s_pcm_register(struct platform_device *pdev);
+#else
+static inline void cdns_i2s_pcm_push_tx(struct cdns_i2s_dev *dev) { }
+static inline void cdns_i2s_pcm_pop_rx(struct cdns_i2s_dev *dev) { }
+static inline int cdns_i2s_pcm_register(struct platform_device *pdev)
+{
+	return -EINVAL;
+}
+#endif
+
+#endif /* __CDNS_I2S_MC_H */
-- 
2.25.1

Re: [PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller

[Pierre-Louis Bossart]

> diff --git a/sound/soc/cdns/Kconfig b/sound/soc/cdns/Kconfig
> new file mode 100644
> index 000000000000..61ef718ebfe7
> --- /dev/null
> +++ b/sound/soc/cdns/Kconfig
> @@ -0,0 +1,18 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config SND_SOC_CADENCE_I2S_MC
> +        tristate "Cadence I2S Multi-Channel Controller Device Driver"
> +	depends on HAVE_CLK

indentation is off

> +        select SND_SOC_GENERIC_DMAENGINE_PCM
> +        help
> +         Say Y or M if you want to add support for I2S driver for the
> +         Cadence Multi-Channel I2S Controller device. The device supports
> +         up to a maximum of 8 channels each for play and record.

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Cadence Multi-Channel I2S controller PCM driver
> + *
> + * Copyright (c) 2022-2023 StarFive Technology Co., Ltd.

2024?

> + */
> +
> +#include <linux/io.h>
> +#include <linux/rcupdate.h>
> +#include <sound/pcm_params.h>
> +#include "cdns-i2s-mc.h"
> +
> +#define PERIOD_BYTES_MIN	4096
> +#define BUFFER_BYTES_MAX	(3 * 2 * 8 * PERIOD_BYTES_MIN)

what are those 3 and 2 and 8 factors? a comment or the use of macros
would help.

> +#define PERIODS_MIN		2
> +
> +static unsigned int cdns_i2s_pcm_tx(struct cdns_i2s_dev *dev,
> +				    struct snd_pcm_runtime *runtime,
> +				    unsigned int tx_ptr, bool *period_elapsed,
> +				    snd_pcm_format_t format)
> +{
> +	unsigned int period_pos = tx_ptr % runtime->period_size;

not following what the modulo is for, usually it's modulo the buffer size?

> +	const u16 (*p16)[2] = (void *)runtime->dma_area;
> +	const u32 (*p32)[2] = (void *)runtime->dma_area;
> +	u32 data[2];
> +	int i;
> +
> +	for (i = 0; i < CDNS_I2S_FIFO_DEPTH; i++) {
> +		if (format == SNDRV_PCM_FORMAT_S16_LE) {
> +			data[0] = p16[tx_ptr][0];
> +			data[1] = p16[tx_ptr][1];
> +		} else if (format == SNDRV_PCM_FORMAT_S32_LE) {
> +			data[0] = p32[tx_ptr][0];
> +			data[1] = p32[tx_ptr][1];
> +		}

what about other formats implied by the use of 'else if' ?
> +
> +		iowrite32(data[0], dev->base + CDNS_FIFO_MEM);
> +		iowrite32(data[1], dev->base + CDNS_FIFO_MEM);
> +		period_pos++;
> +		if (++tx_ptr >= runtime->buffer_size)
> +			tx_ptr = 0;
> +	}
> +
> +	*period_elapsed = period_pos >= runtime->period_size;
> +	return tx_ptr;
> +}

> +static void cdns_i2s_pcm_transfer(struct cdns_i2s_dev *dev, bool push)

'push' really means 'tx' so what not use a simpler naming?

> +{
> +	struct snd_pcm_substream *substream;
> +	bool active, period_elapsed;
> +
> +	rcu_read_lock();
> +	if (push)
> +		substream = rcu_dereference(dev->tx_substream);
> +	else
> +		substream = rcu_dereference(dev->rx_substream);
> +
> +	active = substream && snd_pcm_running(substream);
> +	if (active) {
> +		unsigned int ptr;
> +		unsigned int new_ptr;
> +
> +		if (push) {
> +			ptr = READ_ONCE(dev->tx_ptr);
> +			new_ptr = dev->tx_fn(dev, substream->runtime, ptr,
> +					     &period_elapsed, dev->format);
> +			cmpxchg(&dev->tx_ptr, ptr, new_ptr);
> +		} else {
> +			ptr = READ_ONCE(dev->rx_ptr);
> +			new_ptr = dev->rx_fn(dev, substream->runtime, ptr,
> +					     &period_elapsed, dev->format);
> +			cmpxchg(&dev->rx_ptr, ptr, new_ptr);
> +		}
> +
> +		if (period_elapsed)
> +			snd_pcm_period_elapsed(substream);
> +	}
> +	rcu_read_unlock();
> +}
> +
> +void cdns_i2s_pcm_push_tx(struct cdns_i2s_dev *dev)
> +{
> +	cdns_i2s_pcm_transfer(dev, true);
> +}
> +
> +void cdns_i2s_pcm_pop_rx(struct cdns_i2s_dev *dev)
> +{
> +	cdns_i2s_pcm_transfer(dev, false);
> +}
> +
> +static int cdns_i2s_pcm_open(struct snd_soc_component *component,
> +			     struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct cdns_i2s_dev *dev = snd_soc_dai_get_drvdata(snd_soc_rtd_to_cpu(rtd, 0));
> +
> +	snd_soc_set_runtime_hwparams(substream, &cdns_i2s_pcm_hardware);
> +	snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
> +	runtime->private_data = dev;
> +
> +	return 0;
> +}
> +
> +static int cdns_i2s_pcm_close(struct snd_soc_component *component,
> +			      struct snd_pcm_substream *substream)
> +{
> +	synchronize_rcu();
> +	return 0;

runtime->private_data = NULL?

> +}
> +
> +static int cdns_i2s_pcm_hw_params(struct snd_soc_component *component,
> +				  struct snd_pcm_substream *substream,
> +				  struct snd_pcm_hw_params *hw_params)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct cdns_i2s_dev *dev = runtime->private_data;
> +
> +	dev->format = params_format(hw_params);

don't you need to test if the formats are supported?

> +	dev->tx_fn = cdns_i2s_pcm_tx;
> +	dev->rx_fn = cdns_i2s_pcm_rx;
> +
> +	return 0;
> +}

> +static int cdns_i2s_start(struct cdns_i2s_dev *i2s,
> +			  struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	unsigned char max_ch = i2s->max_channels;
> +	unsigned char i2s_ch;
> +	int i;
> +
> +	/* Each channel is stereo */
> +	i2s_ch = runtime->channels / 2;
> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> +		if ((i2s_ch + i2s->tx_using_channels) > max_ch) {
> +			dev_err(i2s->dev,
> +				"Max %d channels: using %d for TX, do not support %d for RX\n",
> +				max_ch, i2s->tx_using_channels, i2s_ch);
> +			return -ENOMEM;

ENOMEM is for memory allocation, that looks more like EINVAL?

> +		}
> +
> +		i2s->rx_using_channels = i2s_ch;
> +		/* Enable channels from 0 to 'max_ch' as tx */
> +		for (i = 0; i < i2s_ch; i++)
> +			cdns_i2s_channel_start(i2s, CDNS_I2S_CM_0 << i,
> +					       CDNS_I2S_TC_RECEIVER);
> +
> +	} else {
> +		if ((i2s_ch + i2s->rx_using_channels) > max_ch) {
> +			dev_err(i2s->dev,
> +				"Max %d channels: using %d for RX, do not support %d for TX\n",
> +				max_ch, i2s->rx_using_channels, i2s_ch);
> +			return -ENOMEM;
> +		}
> +
> +		i2s->tx_using_channels = i2s_ch;
> +		/* Enable channels from 'max_ch' to 0 as rx */
> +		for (i = (max_ch - 1); i > (max_ch - i2s_ch - 1); i--) {
> +			if (i < 0)
> +				return -EINVAL;

that is a test you can probably factor out of the loop before doing
anything that's inconsistent.

> +
> +			cdns_i2s_channel_start(i2s, CDNS_I2S_CM_0 << i,
> +					       CDNS_I2S_TC_TRANSMITTER);
> +		}
> +	}
> +	cdns_i2s_enable_clock(i2s, substream->stream);
> +
> +	if (i2s->irq >= 0)
> +		cdns_i2s_set_all_irq_mask(i2s, false);
> +
> +	cdns_i2s_clear_int(i2s);
> +
> +	return 0;
> +}
> +
> +static int cdns_i2s_stop(struct cdns_i2s_dev *i2s,
> +			 struct snd_pcm_substream *substream)
> +{
> +	unsigned char i2s_ch;
> +	int i;
> +
> +	cdns_i2s_clear_int(i2s);
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> +		i2s_ch = i2s->rx_using_channels;
> +		for (i = 0; i < i2s_ch; i++)
> +			cdns_i2s_channel_stop(i2s, (CDNS_I2S_CM_0 << i));
> +
> +		i2s->rx_using_channels = 0;
> +	} else {
> +		unsigned char max_ch = i2s->max_channels;
> +
> +		i2s_ch = i2s->tx_using_channels;
> +		for (i = (max_ch - 1); i > (max_ch - i2s_ch - 1); i--) {
> +			if (i < 0)
> +				return -EINVAL;

same here, first test if the channel maps are valid, then do the loop?

> +
> +			cdns_i2s_channel_stop(i2s, (CDNS_I2S_CM_0 << i));
> +		}
> +
> +		i2s->tx_using_channels = 0;
> +	}
> +
> +	if (i2s->irq >= 0 && !i2s->tx_using_channels && !i2s->rx_using_channels)
> +		cdns_i2s_set_all_irq_mask(i2s, true);
> +
> +	return 0;
> +}

> +static int cdns_i2s_set_fmt(struct snd_soc_dai *cpu_dai,
> +			    unsigned int fmt)
> +{
> +	struct cdns_i2s_dev *i2s = snd_soc_dai_get_drvdata(cpu_dai);
> +	int ret = 0;
> +
> +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> +	case SND_SOC_DAIFMT_CBM_CFM:
> +		i2s->tx_sync_ms_mode = CDNS_I2S_MASTER_MODE;
> +		i2s->rx_sync_ms_mode = CDNS_I2S_MASTER_MODE;
> +		cdns_i2s_set_ms_mode(i2s);
> +		break;
> +	case SND_SOC_DAIFMT_CBS_CFS:
> +		i2s->tx_sync_ms_mode = CDNS_I2S_SLAVE_MODE;
> +		i2s->rx_sync_ms_mode = CDNS_I2S_SLAVE_MODE;
> +		cdns_i2s_set_ms_mode(i2s);
> +		break;
> +	case SND_SOC_DAIFMT_CBM_CFS:
> +	case SND_SOC_DAIFMT_CBS_CFM:

that's the old stuff, use CBP/CBC macros please.

> +		ret = -EINVAL;
> +		break;
> +	default:
> +		dev_dbg(i2s->dev, "Invalid master/slave format\n");
> +		ret = -EINVAL;
> +		break;
> +	}
> +	return ret;
> +}

> +#ifdef CONFIG_PM

Do we need this or just rely on __unused?

> +static int cdns_i2s_runtime_suspend(struct device *dev)
> +{
> +	struct cdns_i2s_dev *i2s = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(i2s->clks[1].clk); /* ICG clock */
> +	return 0;
> +}
> +
> +static int cdns_i2s_runtime_resume(struct device *dev)
> +{
> +	struct cdns_i2s_dev *i2s = dev_get_drvdata(dev);
> +
> +	return clk_prepare_enable(i2s->clks[1].clk); /* ICG clock */
> +}
> +#endif

> +static int cdns_i2s_probe(struct platform_device *pdev)
> +{
> +	struct cdns_i2s_dev *i2s;
> +	struct resource *res;
> +	int ret;
> +
> +	i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
> +	if (!i2s) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +	platform_set_drvdata(pdev, i2s);
> +
> +	i2s->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(i2s->base)) {
> +		ret = PTR_ERR(i2s->base);
> +		goto err;
> +	}
> +
> +	i2s->dev = &pdev->dev;
> +	i2s->phybase = res->start;
> +
> +	ret = cdns_i2s_init(i2s);
> +	if (ret)
> +		goto err;
> +
> +	i2s->irq = platform_get_irq(pdev, 0);
> +	if (i2s->irq >= 0) {
> +		ret = devm_request_irq(&pdev->dev, i2s->irq, cdns_i2s_irq_handler,
> +				       0, pdev->name, i2s);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "request_irq failed\n");
> +			goto err;
> +		}
> +	}
> +
> +	ret = devm_snd_soc_register_component(&pdev->dev,
> +					      &cdns_i2s_component,
> +					      &cdns_i2s_dai, 1);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "couldn't register component\n");
> +		goto err;
> +	}
> +
> +	if (i2s->irq >= 0)
> +		ret = cdns_i2s_pcm_register(pdev);
> +	else
> +		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;
> +	}
> +
> +	pm_runtime_enable(&pdev->dev);
> +	if (pm_runtime_enabled(&pdev->dev))
> +		cdns_i2s_runtime_suspend(&pdev->dev);

that sequence looks suspicious.... Why would you suspend immediately
during the probe? You're probably missing all the autosuspend stuff?

> +
> +	dev_dbg(&pdev->dev, "I2S supports %d stereo channels with %s.\n",
> +		i2s->max_channels, ((i2s->irq < 0) ? "dma" : "interrupt"));
> +
> +	return 0;
> +
> +err:
> +	return ret;
> +}
> +
> +static int cdns_i2s_remove(struct platform_device *pdev)
> +{
> +	pm_runtime_disable(&pdev->dev);
> +	if (!pm_runtime_status_suspended(&pdev->dev))
> +		cdns_i2s_runtime_suspend(&pdev->dev);

... and this one too. Once you've disabled pm_runtime, checking the
status is irrelevant...

> +
> +	return 0;
> +}
> +

Re: [PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller

[Mark Brown]

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

On Wed, Mar 20, 2024 at 10:00:24AM -0500, Pierre-Louis Bossart wrote:

> > +	for (i = 0; i < CDNS_I2S_FIFO_DEPTH; i++) {
> > +		if (format == SNDRV_PCM_FORMAT_S16_LE) {
> > +			data[0] = p16[tx_ptr][0];
> > +			data[1] = p16[tx_ptr][1];
> > +		} else if (format == SNDRV_PCM_FORMAT_S32_LE) {
> > +			data[0] = p32[tx_ptr][0];
> > +			data[1] = p32[tx_ptr][1];
> > +		}

> what about other formats implied by the use of 'else if' ?

In general things like this should be written as switch statements.

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

Re: [PATCH v2 1/2] ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller

[Krzysztof Kozlowski]

On 20/03/2024 10:02, Xingyu Wu wrote:
> Add bindings for the Multi-Channel I2S controller of Cadence.
> 
> The Multi-Channel I2S (I2S-MC) implements a function of the
> 8-channel I2S bus interfasce. Each channel can become receiver
> or transmitter. Four I2S instances are used on the StarFive
> JH8100 SoC. One instance of them is limited to 2 channels, two
> instance are limited to 4 channels, and the other one can use
> most 8 channels. Add a unique property about
> 'starfive,i2s-max-channels' to distinguish each instance.
> 
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> ---
>  .../bindings/sound/cdns,i2s-mc.yaml           | 110 ++++++++++++++++++
>  1 file changed, 110 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml b/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> new file mode 100644
> index 000000000000..0a1b0122a246
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> @@ -0,0 +1,110 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/cdns,i2s-mc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cadence multi-channel I2S controller
> +
> +description:
> +  The Cadence I2S Controller implements a function of the multi-channel
> +  (up to 8-channel) bus. It combines functions of a transmitter and a receiver.
> +
> +maintainers:
> +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - cdns,i2s-mc

Why did this appear? Who asked for this? Usually these blocks are not
usable on their own.

> +      - starfive,jh8100-i2s
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    description:
> +      The interrupt line number for the I2S controller. Add this
> +      parameter if the I2S controller that you are using does not
> +      using DMA.

That's still wrong. You already got comment on this. Either you have
interrupt or not. You do not add interrupts, based on your choice or not
of having DMA. Drop the comment.

> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Bit clock
> +      - description: Main ICG clock
> +      - description: Inner master clock
> +
> +  clock-names:
> +    items:
> +      - const: bclk
> +      - const: icg
> +      - const: mclk_inner
> +
> +  resets:
> +    maxItems: 1
> +
> +  dmas:
> +    items:
> +      - description: TX DMA Channel
> +      - description: RX DMA Channel
> +    minItems: 1
> +
> +  dma-names:
> +    items:
> +      - const: tx
> +      - const: rx
> +    minItems: 1
> +
> +  "#sound-dai-cells":
> +    const: 0
> +
> +allOf:
> +  - $ref: dai-common.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: starfive,jh8100-i2s
> +    then:
> +      properties:
> +        starfive,i2s-max-channels:
> +          description:
> +            Number of I2S max stereo channels supported on the StarFive
> +            JH8100 SoC.
> +          $ref: /schemas/types.yaml#/definitions/uint32

Properties must be defined in top-level. You can disallow the property
for other variants, but I claim you won't have here other variants.

https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L212


> +          enum: [2, 4, 8]
> +      required:
> +        - starfive,i2s-max-channels
> +
> +required:

required goes after properties.

> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - resets
> +
> +oneOf:
> +  - required:
> +      - dmas
> +      - dma-names
> +  - required:
> +      - interrupts

This won't work. Provide both interrupts and dmas, and then test your DTS.

> +
> +unevaluatedProperties: false
> +

Best regards,
Krzysztof

回复: [PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller

[Xingyu Wu]

> > diff --git a/sound/soc/cdns/Kconfig b/sound/soc/cdns/Kconfig new file
> > mode 100644 index 000000000000..61ef718ebfe7
> > --- /dev/null
> > +++ b/sound/soc/cdns/Kconfig
> > @@ -0,0 +1,18 @@
> > +# SPDX-License-Identifier: GPL-2.0-only config SND_SOC_CADENCE_I2S_MC
> > +        tristate "Cadence I2S Multi-Channel Controller Device Driver"
> > +	depends on HAVE_CLK
> 
> indentation is off

Will fix. Thanks.

> 
> > +        select SND_SOC_GENERIC_DMAENGINE_PCM
> > +        help
> > +         Say Y or M if you want to add support for I2S driver for the
> > +         Cadence Multi-Channel I2S Controller device. The device supports
> > +         up to a maximum of 8 channels each for play and record.
> 
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Cadence Multi-Channel I2S controller PCM driver
> > + *
> > + * Copyright (c) 2022-2023 StarFive Technology Co., Ltd.
> 
> 2024?

Will fix.

> 
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/rcupdate.h>
> > +#include <sound/pcm_params.h>
> > +#include "cdns-i2s-mc.h"
> > +
> > +#define PERIOD_BYTES_MIN	4096
> > +#define BUFFER_BYTES_MAX	(3 * 2 * 8 * PERIOD_BYTES_MIN)
> 
> what are those 3 and 2 and 8 factors? a comment or the use of macros would help.

Will fix.

> 
> > +#define PERIODS_MIN		2
> > +
> > +static unsigned int cdns_i2s_pcm_tx(struct cdns_i2s_dev *dev,
> > +				    struct snd_pcm_runtime *runtime,
> > +				    unsigned int tx_ptr, bool *period_elapsed,
> > +				    snd_pcm_format_t format)
> > +{
> > +	unsigned int period_pos = tx_ptr % runtime->period_size;
> 
> not following what the modulo is for, usually it's modulo the buffer size?

This is to see if the new data is divisible by period_size and to determine whether
it is enough for a period_size in the later loop.

> 
> > +	const u16 (*p16)[2] = (void *)runtime->dma_area;
> > +	const u32 (*p32)[2] = (void *)runtime->dma_area;
> > +	u32 data[2];
> > +	int i;
> > +
> > +	for (i = 0; i < CDNS_I2S_FIFO_DEPTH; i++) {
> > +		if (format == SNDRV_PCM_FORMAT_S16_LE) {
> > +			data[0] = p16[tx_ptr][0];
> > +			data[1] = p16[tx_ptr][1];
> > +		} else if (format == SNDRV_PCM_FORMAT_S32_LE) {
> > +			data[0] = p32[tx_ptr][0];
> > +			data[1] = p32[tx_ptr][1];
> > +		}
> 
> what about other formats implied by the use of 'else if' ?

I think I just support S16_LE and S32_LE in the snd_soc_dai_driver struct,
and ALSA device will filter out other formats for me, so I didn't add them.
Do I still have to add the other case?
	
> > +
> > +		iowrite32(data[0], dev->base + CDNS_FIFO_MEM);
> > +		iowrite32(data[1], dev->base + CDNS_FIFO_MEM);
> > +		period_pos++;
> > +		if (++tx_ptr >= runtime->buffer_size)
> > +			tx_ptr = 0;
> > +	}
> > +
> > +	*period_elapsed = period_pos >= runtime->period_size;
> > +	return tx_ptr;
> > +}
> 
> > +static void cdns_i2s_pcm_transfer(struct cdns_i2s_dev *dev, bool
> > +push)
> 
> 'push' really means 'tx' so what not use a simpler naming?

Will fix.

> 
> > +{
> > +	struct snd_pcm_substream *substream;
> > +	bool active, period_elapsed;
> > +
> > +	rcu_read_lock();
> > +	if (push)
> > +		substream = rcu_dereference(dev->tx_substream);
> > +	else
> > +		substream = rcu_dereference(dev->rx_substream);
> > +
> > +	active = substream && snd_pcm_running(substream);
> > +	if (active) {
> > +		unsigned int ptr;
> > +		unsigned int new_ptr;
> > +
> > +		if (push) {
> > +			ptr = READ_ONCE(dev->tx_ptr);
> > +			new_ptr = dev->tx_fn(dev, substream->runtime, ptr,
> > +					     &period_elapsed, dev->format);
> > +			cmpxchg(&dev->tx_ptr, ptr, new_ptr);
> > +		} else {
> > +			ptr = READ_ONCE(dev->rx_ptr);
> > +			new_ptr = dev->rx_fn(dev, substream->runtime, ptr,
> > +					     &period_elapsed, dev->format);
> > +			cmpxchg(&dev->rx_ptr, ptr, new_ptr);
> > +		}
> > +
> > +		if (period_elapsed)
> > +			snd_pcm_period_elapsed(substream);
> > +	}
> > +	rcu_read_unlock();
> > +}
> > +
> > +void cdns_i2s_pcm_push_tx(struct cdns_i2s_dev *dev) {
> > +	cdns_i2s_pcm_transfer(dev, true);
> > +}
> > +
> > +void cdns_i2s_pcm_pop_rx(struct cdns_i2s_dev *dev) {
> > +	cdns_i2s_pcm_transfer(dev, false);
> > +}
> > +
> > +static int cdns_i2s_pcm_open(struct snd_soc_component *component,
> > +			     struct snd_pcm_substream *substream) {
> > +	struct snd_pcm_runtime *runtime = substream->runtime;
> > +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> > +	struct cdns_i2s_dev *dev =
> > +snd_soc_dai_get_drvdata(snd_soc_rtd_to_cpu(rtd, 0));
> > +
> > +	snd_soc_set_runtime_hwparams(substream, &cdns_i2s_pcm_hardware);
> > +	snd_pcm_hw_constraint_integer(runtime,
> SNDRV_PCM_HW_PARAM_PERIODS);
> > +	runtime->private_data = dev;
> > +
> > +	return 0;
> > +}
> > +
> > +static int cdns_i2s_pcm_close(struct snd_soc_component *component,
> > +			      struct snd_pcm_substream *substream) {
> > +	synchronize_rcu();
> > +	return 0;
> 
> runtime->private_data = NULL?

Will add.

> 
> > +}
> > +
> > +static int cdns_i2s_pcm_hw_params(struct snd_soc_component *component,
> > +				  struct snd_pcm_substream *substream,
> > +				  struct snd_pcm_hw_params *hw_params) {
> > +	struct snd_pcm_runtime *runtime = substream->runtime;
> > +	struct cdns_i2s_dev *dev = runtime->private_data;
> > +
> > +	dev->format = params_format(hw_params);
> 
> don't you need to test if the formats are supported?

Will add the test.

> 
> > +	dev->tx_fn = cdns_i2s_pcm_tx;
> > +	dev->rx_fn = cdns_i2s_pcm_rx;
> > +
> > +	return 0;
> > +}
> 
> > +static int cdns_i2s_start(struct cdns_i2s_dev *i2s,
> > +			  struct snd_pcm_substream *substream) {
> > +	struct snd_pcm_runtime *runtime = substream->runtime;
> > +	unsigned char max_ch = i2s->max_channels;
> > +	unsigned char i2s_ch;
> > +	int i;
> > +
> > +	/* Each channel is stereo */
> > +	i2s_ch = runtime->channels / 2;
> > +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> > +		if ((i2s_ch + i2s->tx_using_channels) > max_ch) {
> > +			dev_err(i2s->dev,
> > +				"Max %d channels: using %d for TX, do not support %d for RX\n",
> > +				max_ch, i2s->tx_using_channels, i2s_ch);
> > +			return -ENOMEM;
> 
> ENOMEM is for memory allocation, that looks more like EINVAL?

Will fix.

> 
> > +		}
> > +
> > +		i2s->rx_using_channels = i2s_ch;
> > +		/* Enable channels from 0 to 'max_ch' as tx */
> > +		for (i = 0; i < i2s_ch; i++)
> > +			cdns_i2s_channel_start(i2s, CDNS_I2S_CM_0 << i,
> > +					       CDNS_I2S_TC_RECEIVER);
> > +
> > +	} else {
> > +		if ((i2s_ch + i2s->rx_using_channels) > max_ch) {
> > +			dev_err(i2s->dev,
> > +				"Max %d channels: using %d for RX, do not support %d for TX\n",
> > +				max_ch, i2s->rx_using_channels, i2s_ch);
> > +			return -ENOMEM;
> > +		}
> > +
> > +		i2s->tx_using_channels = i2s_ch;
> > +		/* Enable channels from 'max_ch' to 0 as rx */
> > +		for (i = (max_ch - 1); i > (max_ch - i2s_ch - 1); i--) {
> > +			if (i < 0)
> > +				return -EINVAL;
> 
> that is a test you can probably factor out of the loop before doing anything that's
> inconsistent.

OK, I will move it out of the loop. Thanks.

> 
> > +
> > +			cdns_i2s_channel_start(i2s, CDNS_I2S_CM_0 << i,
> > +					       CDNS_I2S_TC_TRANSMITTER);
> > +		}
> > +	}
> > +	cdns_i2s_enable_clock(i2s, substream->stream);
> > +
> > +	if (i2s->irq >= 0)
> > +		cdns_i2s_set_all_irq_mask(i2s, false);
> > +
> > +	cdns_i2s_clear_int(i2s);
> > +
> > +	return 0;
> > +}
> > +
> > +static int cdns_i2s_stop(struct cdns_i2s_dev *i2s,
> > +			 struct snd_pcm_substream *substream) {
> > +	unsigned char i2s_ch;
> > +	int i;
> > +
> > +	cdns_i2s_clear_int(i2s);
> > +
> > +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> > +		i2s_ch = i2s->rx_using_channels;
> > +		for (i = 0; i < i2s_ch; i++)
> > +			cdns_i2s_channel_stop(i2s, (CDNS_I2S_CM_0 << i));
> > +
> > +		i2s->rx_using_channels = 0;
> > +	} else {
> > +		unsigned char max_ch = i2s->max_channels;
> > +
> > +		i2s_ch = i2s->tx_using_channels;
> > +		for (i = (max_ch - 1); i > (max_ch - i2s_ch - 1); i--) {
> > +			if (i < 0)
> > +				return -EINVAL;
> 
> same here, first test if the channel maps are valid, then do the loop?

Will fix.

> 
> > +
> > +			cdns_i2s_channel_stop(i2s, (CDNS_I2S_CM_0 << i));
> > +		}
> > +
> > +		i2s->tx_using_channels = 0;
> > +	}
> > +
> > +	if (i2s->irq >= 0 && !i2s->tx_using_channels && !i2s->rx_using_channels)
> > +		cdns_i2s_set_all_irq_mask(i2s, true);
> > +
> > +	return 0;
> > +}
> 
> > +static int cdns_i2s_set_fmt(struct snd_soc_dai *cpu_dai,
> > +			    unsigned int fmt)
> > +{
> > +	struct cdns_i2s_dev *i2s = snd_soc_dai_get_drvdata(cpu_dai);
> > +	int ret = 0;
> > +
> > +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > +	case SND_SOC_DAIFMT_CBM_CFM:
> > +		i2s->tx_sync_ms_mode = CDNS_I2S_MASTER_MODE;
> > +		i2s->rx_sync_ms_mode = CDNS_I2S_MASTER_MODE;
> > +		cdns_i2s_set_ms_mode(i2s);
> > +		break;
> > +	case SND_SOC_DAIFMT_CBS_CFS:
> > +		i2s->tx_sync_ms_mode = CDNS_I2S_SLAVE_MODE;
> > +		i2s->rx_sync_ms_mode = CDNS_I2S_SLAVE_MODE;
> > +		cdns_i2s_set_ms_mode(i2s);
> > +		break;
> > +	case SND_SOC_DAIFMT_CBM_CFS:
> > +	case SND_SOC_DAIFMT_CBS_CFM:
> 
> that's the old stuff, use CBP/CBC macros please.

Will fix.

> 
> > +		ret = -EINVAL;
> > +		break;
> > +	default:
> > +		dev_dbg(i2s->dev, "Invalid master/slave format\n");
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +	return ret;
> > +}
> 
> > +#ifdef CONFIG_PM
> 
> Do we need this or just rely on __unused?

I think both are OK.

> 
> > +static int cdns_i2s_runtime_suspend(struct device *dev) {
> > +	struct cdns_i2s_dev *i2s = dev_get_drvdata(dev);
> > +
> > +	clk_disable_unprepare(i2s->clks[1].clk); /* ICG clock */
> > +	return 0;
> > +}
> > +
> > +static int cdns_i2s_runtime_resume(struct device *dev) {
> > +	struct cdns_i2s_dev *i2s = dev_get_drvdata(dev);
> > +
> > +	return clk_prepare_enable(i2s->clks[1].clk); /* ICG clock */ }
> > +#endif
> 
> > +static int cdns_i2s_probe(struct platform_device *pdev) {
> > +	struct cdns_i2s_dev *i2s;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
> > +	if (!i2s) {
> > +		ret = -ENOMEM;
> > +		goto err;
> > +	}
> > +	platform_set_drvdata(pdev, i2s);
> > +
> > +	i2s->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > +	if (IS_ERR(i2s->base)) {
> > +		ret = PTR_ERR(i2s->base);
> > +		goto err;
> > +	}
> > +
> > +	i2s->dev = &pdev->dev;
> > +	i2s->phybase = res->start;
> > +
> > +	ret = cdns_i2s_init(i2s);
> > +	if (ret)
> > +		goto err;
> > +
> > +	i2s->irq = platform_get_irq(pdev, 0);
> > +	if (i2s->irq >= 0) {
> > +		ret = devm_request_irq(&pdev->dev, i2s->irq, cdns_i2s_irq_handler,
> > +				       0, pdev->name, i2s);
> > +		if (ret < 0) {
> > +			dev_err(&pdev->dev, "request_irq failed\n");
> > +			goto err;
> > +		}
> > +	}
> > +
> > +	ret = devm_snd_soc_register_component(&pdev->dev,
> > +					      &cdns_i2s_component,
> > +					      &cdns_i2s_dai, 1);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "couldn't register component\n");
> > +		goto err;
> > +	}
> > +
> > +	if (i2s->irq >= 0)
> > +		ret = cdns_i2s_pcm_register(pdev);
> > +	else
> > +		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;
> > +	}
> > +
> > +	pm_runtime_enable(&pdev->dev);
> > +	if (pm_runtime_enabled(&pdev->dev))
> > +		cdns_i2s_runtime_suspend(&pdev->dev);
> 
> that sequence looks suspicious.... Why would you suspend immediately during the
> probe? You're probably missing all the autosuspend stuff?

Since I have enabled clocks before, and the device is in the suspend state after
pm_runtime_enable(), I need to disable clocks in cdns_i2s_runtime_suspend()
to match the suspend state.

> 
> > +
> > +	dev_dbg(&pdev->dev, "I2S supports %d stereo channels with %s.\n",
> > +		i2s->max_channels, ((i2s->irq < 0) ? "dma" : "interrupt"));
> > +
> > +	return 0;
> > +
> > +err:
> > +	return ret;
> > +}
> > +
> > +static int cdns_i2s_remove(struct platform_device *pdev) {
> > +	pm_runtime_disable(&pdev->dev);
> > +	if (!pm_runtime_status_suspended(&pdev->dev))
> > +		cdns_i2s_runtime_suspend(&pdev->dev);
> 
> ... and this one too. Once you've disabled pm_runtime, checking the status is
> irrelevant...

I think the clocks need to be always enabled after probe if disable pm_runtime,
and should be disabled when remove. This will do that.

> 
> > +
> > +	return 0;
> > +}
> > +

Best regards,
Xingyu Wu

回复: [PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller

[Xingyu Wu]

> 
> On Wed, Mar 20, 2024 at 10:00:24AM -0500, Pierre-Louis Bossart wrote:
> 
> > > +	for (i = 0; i < CDNS_I2S_FIFO_DEPTH; i++) {
> > > +		if (format == SNDRV_PCM_FORMAT_S16_LE) {
> > > +			data[0] = p16[tx_ptr][0];
> > > +			data[1] = p16[tx_ptr][1];
> > > +		} else if (format == SNDRV_PCM_FORMAT_S32_LE) {
> > > +			data[0] = p32[tx_ptr][0];
> > > +			data[1] = p32[tx_ptr][1];
> > > +		}
> 
> > what about other formats implied by the use of 'else if' ?
> 
> In general things like this should be written as switch statements.

OK, I will fix it in next version of this patch.

Thanks,
Xingyu Wu

回复: [PATCH v2 1/2] ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller

[Xingyu Wu]

> 
> On 20/03/2024 10:02, Xingyu Wu wrote:
> > Add bindings for the Multi-Channel I2S controller of Cadence.
> >
> > The Multi-Channel I2S (I2S-MC) implements a function of the 8-channel
> > I2S bus interfasce. Each channel can become receiver or transmitter.
> > Four I2S instances are used on the StarFive
> > JH8100 SoC. One instance of them is limited to 2 channels, two
> > instance are limited to 4 channels, and the other one can use most 8
> > channels. Add a unique property about 'starfive,i2s-max-channels' to
> > distinguish each instance.
> >
> > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> > ---
> >  .../bindings/sound/cdns,i2s-mc.yaml           | 110 ++++++++++++++++++
> >  1 file changed, 110 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> > b/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> > new file mode 100644
> > index 000000000000..0a1b0122a246
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/cdns,i2s-mc.yaml
> > @@ -0,0 +1,110 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/cdns,i2s-mc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Cadence multi-channel I2S controller
> > +
> > +description:
> > +  The Cadence I2S Controller implements a function of the
> > +multi-channel
> > +  (up to 8-channel) bus. It combines functions of a transmitter and a receiver.
> > +
> > +maintainers:
> > +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - cdns,i2s-mc
> 
> Why did this appear? Who asked for this? Usually these blocks are not usable on their
> own.

I wonder if I should keep the original IP compatible. Do I not need it?

> 
> > +      - starfive,jh8100-i2s
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    description:
> > +      The interrupt line number for the I2S controller. Add this
> > +      parameter if the I2S controller that you are using does not
> > +      using DMA.
> 
> That's still wrong. You already got comment on this. Either you have interrupt or not.
> You do not add interrupts, based on your choice or not of having DMA. Drop the
> comment.

Do I keep this property and drop this description?

> 
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: Bit clock
> > +      - description: Main ICG clock
> > +      - description: Inner master clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: bclk
> > +      - const: icg
> > +      - const: mclk_inner
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  dmas:
> > +    items:
> > +      - description: TX DMA Channel
> > +      - description: RX DMA Channel
> > +    minItems: 1
> > +
> > +  dma-names:
> > +    items:
> > +      - const: tx
> > +      - const: rx
> > +    minItems: 1
> > +
> > +  "#sound-dai-cells":
> > +    const: 0
> > +
> > +allOf:
> > +  - $ref: dai-common.yaml#
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: starfive,jh8100-i2s
> > +    then:
> > +      properties:
> > +        starfive,i2s-max-channels:
> > +          description:
> > +            Number of I2S max stereo channels supported on the StarFive
> > +            JH8100 SoC.
> > +          $ref: /schemas/types.yaml#/definitions/uint32
> 
> Properties must be defined in top-level. You can disallow the property for other
> variants, but I claim you won't have here other variants.
> 
> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/e
> xample-schema.yaml#L212
> 

Will fix.

> 
> > +          enum: [2, 4, 8]
> > +      required:
> > +        - starfive,i2s-max-channels
> > +
> > +required:
> 
> required goes after properties.

Will fix.

> 
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - resets
> > +
> > +oneOf:
> > +  - required:
> > +      - dmas
> > +      - dma-names
> > +  - required:
> > +      - interrupts
> 
> This won't work. Provide both interrupts and dmas, and then test your DTS.

I provided both properties in the DTS and test by dtbs_check. Then it printed that:
'More than one condition true in one of shema: ...'

> 
> > +
> > +unevaluatedProperties: false
> > +
> 
> Best regards,
> Krzysztof


Best regards,
Xingyu Wu

Re: 回复: [PATCH v2 1/2] ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller

[Krzysztof Kozlowski]

On 26/03/2024 07:29, Xingyu Wu wrote:
>>
>> On 20/03/2024 10:02, Xingyu Wu wrote:
>>> Add bindings for the Multi-Channel I2S controller of Cadence.

Your email app responds very weird. You bypassed all possible filters
and it is simply not visible that you answer to me. You Reply to
everyone, not to me with Cc to others. There is no standard header whom
do you quote, e.g. "On 26/03/2024 07:29, Xingyu Wu wrote:"

Please use some decent email system, otherwise I will miss all replies
from you.

>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - cdns,i2s-mc
>>
>> Why did this appear? Who asked for this? Usually these blocks are not usable on their
>> own.
> 
> I wonder if I should keep the original IP compatible. Do I not need it?

As I said, it is not usable on its own, so unless you have other
arguments then no. But my point was that no one asked for this.

> 
>>
>>> +      - starfive,jh8100-i2s
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    description:
>>> +      The interrupt line number for the I2S controller. Add this
>>> +      parameter if the I2S controller that you are using does not
>>> +      using DMA.
>>
>> That's still wrong. You already got comment on this. Either you have interrupt or not.
>> You do not add interrupts, based on your choice or not of having DMA. Drop the
>> comment.
> 
> Do I keep this property and drop this description?

Drop description. Keep property, if your hardware has interrupts.

...

>>
>>> +  - compatible
>>> +  - reg
>>> +  - clocks
>>> +  - clock-names
>>> +  - resets
>>> +
>>> +oneOf:
>>> +  - required:
>>> +      - dmas
>>> +      - dma-names
>>> +  - required:
>>> +      - interrupts
>>
>> This won't work. Provide both interrupts and dmas, and then test your DTS.
> 
> I provided both properties in the DTS and test by dtbs_check. Then it printed that:
> 'More than one condition true in one of shema: ...'

Exactly. Having both properties is a correct DTS. Interrupts do not
disappear just because you decide to describe DMA. It is OS choice what
to use if both are provided.



Best regards,
Krzysztof

RE: 回复: [PATCH v2 1/2] ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller

[Xingyu Wu]

On 26/03/2024 14:46, Krzysztof Kozlowski wrote:
> 
> On 26/03/2024 07:29, Xingyu Wu wrote:
> >>
> >> On 20/03/2024 10:02, Xingyu Wu wrote:
> >>> Add bindings for the Multi-Channel I2S controller of Cadence.
> 
> Your email app responds very weird. You bypassed all possible filters and it is
> simply not visible that you answer to me. You Reply to everyone, not to me with
> Cc to others. There is no standard header whom do you quote, e.g. "On
> 26/03/2024 07:29, Xingyu Wu wrote:"
> 
> Please use some decent email system, otherwise I will miss all replies from you.

Thank you for reminding me. I will correct it.

> 
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - cdns,i2s-mc
> >>
> >> Why did this appear? Who asked for this? Usually these blocks are not
> >> usable on their own.
> >
> > I wonder if I should keep the original IP compatible. Do I not need it?
> 
> As I said, it is not usable on its own, so unless you have other arguments then no.
> But my point was that no one asked for this.

I want to keep the original IP compatible which can distinguish from the JH8100 SoC.
Can I write it like this:
compatible:
   enum:
          - starfive,jh8100-i2s
   const: cdns,i2s-mc

and I write this in the DTS:
compatible = "starfive,jh8100-i2s", "cdns,i2s-mc";

> 
> >
> >>
> >>> +      - starfive,jh8100-i2s
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  interrupts:
> >>> +    description:
> >>> +      The interrupt line number for the I2S controller. Add this
> >>> +      parameter if the I2S controller that you are using does not
> >>> +      using DMA.
> >>
> >> That's still wrong. You already got comment on this. Either you have interrupt
> or not.
> >> You do not add interrupts, based on your choice or not of having DMA.
> >> Drop the comment.
> >
> > Do I keep this property and drop this description?
> 
> Drop description. Keep property, if your hardware has interrupts.
> 

Will drop.

> ...
> 
> >>
> >>> +  - compatible
> >>> +  - reg
> >>> +  - clocks
> >>> +  - clock-names
> >>> +  - resets
> >>> +
> >>> +oneOf:
> >>> +  - required:
> >>> +      - dmas
> >>> +      - dma-names
> >>> +  - required:
> >>> +      - interrupts
> >>
> >> This won't work. Provide both interrupts and dmas, and then test your DTS.
> >
> > I provided both properties in the DTS and test by dtbs_check. Then it printed
> that:
> > 'More than one condition true in one of shema: ...'
> 
> Exactly. Having both properties is a correct DTS. Interrupts do not disappear just
> because you decide to describe DMA. It is OS choice what to use if both are
> provided.
> 

But this I2S can only use either DMA or interrupts.

Can I use the config (like SND_SOC_CADENCE_I2S_MC_PCM)  to choose DMA or
interrupt if having both them in DTS?

Best regards,
Xingyu Wu

Re: 回复: [PATCH v2 1/2] ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller

[Krzysztof Kozlowski]

On 26/03/2024 14:43, Xingyu Wu wrote:
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - cdns,i2s-mc
>>>>
>>>> Why did this appear? Who asked for this? Usually these blocks are not
>>>> usable on their own.
>>>
>>> I wonder if I should keep the original IP compatible. Do I not need it?
>>
>> As I said, it is not usable on its own, so unless you have other arguments then no.
>> But my point was that no one asked for this.
> 
> I want to keep the original IP compatible which can distinguish from the JH8100 SoC.
> Can I write it like this:
> compatible:
>    enum:
>           - starfive,jh8100-i2s
>    const: cdns,i2s-mc
> 
> and I write this in the DTS:
> compatible = "starfive,jh8100-i2s", "cdns,i2s-mc";

Can you provide any rationale for this? I asked "unless you have other
arguments", so where are the arguments?

Nothing was explained in patch changelog. Nothing was provided in this
email thread.

> 
>>
>>>
>>>>
>>>>> +      - starfive,jh8100-i2s
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  interrupts:
>>>>> +    description:
>>>>> +      The interrupt line number for the I2S controller. Add this
>>>>> +      parameter if the I2S controller that you are using does not
>>>>> +      using DMA.
>>>>
>>>> That's still wrong. You already got comment on this. Either you have interrupt
>> or not.
>>>> You do not add interrupts, based on your choice or not of having DMA.
>>>> Drop the comment.
>>>
>>> Do I keep this property and drop this description?
>>
>> Drop description. Keep property, if your hardware has interrupts.
>>
> 
> Will drop.
> 
>> ...
>>
>>>>
>>>>> +  - compatible
>>>>> +  - reg
>>>>> +  - clocks
>>>>> +  - clock-names
>>>>> +  - resets
>>>>> +
>>>>> +oneOf:
>>>>> +  - required:
>>>>> +      - dmas
>>>>> +      - dma-names
>>>>> +  - required:
>>>>> +      - interrupts
>>>>
>>>> This won't work. Provide both interrupts and dmas, and then test your DTS.
>>>
>>> I provided both properties in the DTS and test by dtbs_check. Then it printed
>> that:
>>> 'More than one condition true in one of shema: ...'
>>
>> Exactly. Having both properties is a correct DTS. Interrupts do not disappear just
>> because you decide to describe DMA. It is OS choice what to use if both are
>> provided.
>>
> 
> But this I2S can only use either DMA or interrupts.

Just like many other components. DTS should reflect hardware. Hardware
has interrupts and DMA, right?

> 
> Can I use the config (like SND_SOC_CADENCE_I2S_MC_PCM)  to choose DMA or
> interrupt if having both them in DTS?

Don't know, I tend to focus here on bindings.

Best regards,
Krzysztof

RE: 回复: [PATCH v2 1/2] ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller

[Xingyu Wu]

On 27/03/2024 13:12, Krzysztof Kozlowski wrote:

> On 26/03/2024 14:43, Xingyu Wu wrote:
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    enum:
> >>>>> +      - cdns,i2s-mc
> >>>>
> >>>> Why did this appear? Who asked for this? Usually these blocks are
> >>>> not usable on their own.
> >>>
> >>> I wonder if I should keep the original IP compatible. Do I not need it?
> >>
> >> As I said, it is not usable on its own, so unless you have other arguments then
> no.
> >> But my point was that no one asked for this.
> >
> > I want to keep the original IP compatible which can distinguish from the JH8100
> SoC.
> > Can I write it like this:
> > compatible:
> >    enum:
> >           - starfive,jh8100-i2s
> >    const: cdns,i2s-mc
> >
> > and I write this in the DTS:
> > compatible = "starfive,jh8100-i2s", "cdns,i2s-mc";
> 
> Can you provide any rationale for this? I asked "unless you have other
> arguments", so where are the arguments?
> 
> Nothing was explained in patch changelog. Nothing was provided in this email
> thread.

I don't know if I understood what mark said[1]. Is it to keep the original IP and
add other compatible?

[1] https://lore.kernel.org/all/27155281-573c-493d-96fe-1f28ebb0ce5e@sirena.org.uk/

Or should I only keep the compatible 'starfive,jh8110-i2s' and change the
bindings name to same to this compatible?

> 
> >
> >>
> >>>
> >>>>
> >>>>> +      - starfive,jh8100-i2s
> >>>>> +
> >>>>> +  reg:
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  interrupts:
> >>>>> +    description:
> >>>>> +      The interrupt line number for the I2S controller. Add this
> >>>>> +      parameter if the I2S controller that you are using does not
> >>>>> +      using DMA.
> >>>>
> >>>> That's still wrong. You already got comment on this. Either you
> >>>> have interrupt
> >> or not.
> >>>> You do not add interrupts, based on your choice or not of having DMA.
> >>>> Drop the comment.
> >>>
> >>> Do I keep this property and drop this description?
> >>
> >> Drop description. Keep property, if your hardware has interrupts.
> >>
> >
> > Will drop.
> >
> >> ...
> >>
> >>>>
> >>>>> +  - compatible
> >>>>> +  - reg
> >>>>> +  - clocks
> >>>>> +  - clock-names
> >>>>> +  - resets
> >>>>> +
> >>>>> +oneOf:
> >>>>> +  - required:
> >>>>> +      - dmas
> >>>>> +      - dma-names
> >>>>> +  - required:
> >>>>> +      - interrupts
> >>>>
> >>>> This won't work. Provide both interrupts and dmas, and then test your DTS.
> >>>
> >>> I provided both properties in the DTS and test by dtbs_check. Then
> >>> it printed
> >> that:
> >>> 'More than one condition true in one of shema: ...'
> >>
> >> Exactly. Having both properties is a correct DTS. Interrupts do not
> >> disappear just because you decide to describe DMA. It is OS choice
> >> what to use if both are provided.
> >>
> >
> > But this I2S can only use either DMA or interrupts.
> 
> Just like many other components. DTS should reflect hardware. Hardware has
> interrupts and DMA, right?

Yes. The hardware can use interrupts and provide the handshake interface of
DMA to DMA controller. In software, we can choose only one between them.
Do I keep them both in the bindings and provide the selection in the driver?

> 
> >
> > Can I use the config (like SND_SOC_CADENCE_I2S_MC_PCM)  to choose DMA
> > or interrupt if having both them in DTS?
> 
> Don't know, I tend to focus here on bindings.
> 

Best regards,
Xingyu Wu

Re: 回复: [PATCH v2 1/2] ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller

[Krzysztof Kozlowski]

On 29/03/2024 04:56, Xingyu Wu wrote:
>>> I want to keep the original IP compatible which can distinguish from the JH8100
>> SoC.
>>> Can I write it like this:
>>> compatible:
>>>    enum:
>>>           - starfive,jh8100-i2s
>>>    const: cdns,i2s-mc
>>>
>>> and I write this in the DTS:
>>> compatible = "starfive,jh8100-i2s", "cdns,i2s-mc";
>>
>> Can you provide any rationale for this? I asked "unless you have other
>> arguments", so where are the arguments?
>>
>> Nothing was explained in patch changelog. Nothing was provided in this email
>> thread.
> 
> I don't know if I understood what mark said[1]. Is it to keep the original IP and
> add other compatible?
> 
> [1] https://lore.kernel.org/all/27155281-573c-493d-96fe-1f28ebb0ce5e@sirena.org.uk/
> 

I stated and I keep my statement that such block is usually not usable
on its own and always needs some sort of quirks or SoC-specific
implementation. At least this is what I saw in other similar cases, but
not exactly I2S.

Therefore I think fallback is not usable here, thus please use only
starfive compatible. Drop the fallback. It could be added in the future
if I am proven wrong. If you think that fallback is usable alone, please
bring some real life case.

> Or should I only keep the compatible 'starfive,jh8110-i2s' and change the
> bindings name to same to this compatible?

Filename could be cdns,i2s-mc.yaml, assuming that's the name of original
IP block.

...

>>>>
>>>
>>> But this I2S can only use either DMA or interrupts.
>>
>> Just like many other components. DTS should reflect hardware. Hardware has
>> interrupts and DMA, right?
> 
> Yes. The hardware can use interrupts and provide the handshake interface of
> DMA to DMA controller. In software, we can choose only one between them.
> Do I keep them both in the bindings and provide the selection in the driver?

Yes.

Best regards,
Krzysztof

Re: 回复: [PATCH v2 1/2] ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller

[Mark Brown]

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

On Fri, Mar 29, 2024 at 12:42:22PM +0100, Krzysztof Kozlowski wrote:

> I stated and I keep my statement that such block is usually not usable
> on its own and always needs some sort of quirks or SoC-specific
> implementation. At least this is what I saw in other similar cases, but
> not exactly I2S.

I wouldn't be so pessimistic, especially not for I2S - a good portion of
quirks there are extra features rather than things needed for basic
operation, a lot of things that might in the past have been quirks for
basic operation are these days hidden behind the DT bindings.

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

Re: 回复: [PATCH v2 1/2] ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller

[Krzysztof Kozlowski]

On 29/03/2024 14:36, Mark Brown wrote:
> On Fri, Mar 29, 2024 at 12:42:22PM +0100, Krzysztof Kozlowski wrote:
> 
>> I stated and I keep my statement that such block is usually not usable
>> on its own and always needs some sort of quirks or SoC-specific
>> implementation. At least this is what I saw in other similar cases, but
>> not exactly I2S.
> 
> I wouldn't be so pessimistic, especially not for I2S - a good portion of
> quirks there are extra features rather than things needed for basic
> operation, a lot of things that might in the past have been quirks for
> basic operation are these days hidden behind the DT bindings.

OK, I trust your judgement, so cdns as fallback seems okay, but I don't
think it warrants cdns as used alone. Not particularly because cdns is
different, but because we expect specific SoC compatible always.

Thus if cdns,i2s-mc stays, then:

items:
  - enum:
      - starfive,jh8100-i2s
  - cdns,i2s-mc

Best regards,
Krzysztof

RE: 回复: [PATCH v2 1/2] ASoC: dt-bindings: Add bindings for Cadence I2S-MC controller

[Xingyu Wu]

On 30/03/2024 0:02, Krzysztof Kozlowski wrote:
> 
> On 29/03/2024 14:36, Mark Brown wrote:
> > On Fri, Mar 29, 2024 at 12:42:22PM +0100, Krzysztof Kozlowski wrote:
> >
> >> I stated and I keep my statement that such block is usually not
> >> usable on its own and always needs some sort of quirks or
> >> SoC-specific implementation. At least this is what I saw in other
> >> similar cases, but not exactly I2S.
> >
> > I wouldn't be so pessimistic, especially not for I2S - a good portion
> > of quirks there are extra features rather than things needed for basic
> > operation, a lot of things that might in the past have been quirks for
> > basic operation are these days hidden behind the DT bindings.
> 
> OK, I trust your judgement, so cdns as fallback seems okay, but I don't think it
> warrants cdns as used alone. Not particularly because cdns is different, but
> because we expect specific SoC compatible always.
> 
> Thus if cdns,i2s-mc stays, then:
> 
> items:
>   - enum:
>       - starfive,jh8100-i2s
>   - cdns,i2s-mc
> 

OK, thanks Krzysztof and Mark. I will modify it in next patch.

Best regards,
Xingyu Wu

Re: 回复: [PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller

[Pierre-Louis Bossart]

>>
>>> +#define PERIODS_MIN		2
>>> +
>>> +static unsigned int cdns_i2s_pcm_tx(struct cdns_i2s_dev *dev,
>>> +				    struct snd_pcm_runtime *runtime,
>>> +				    unsigned int tx_ptr, bool *period_elapsed,
>>> +				    snd_pcm_format_t format)
>>> +{
>>> +	unsigned int period_pos = tx_ptr % runtime->period_size;
>>
>> not following what the modulo is for, usually it's modulo the buffer size?
> 
> This is to see if the new data is divisible by period_size and to determine whether
> it is enough for a period_size in the later loop.

That didn't answer to my question, the position is usually between
0..buffer_size.1.

Doing increments on a modulo value then comparisons as done below seems
rather questionable.
	
>>> +
>>> +		iowrite32(data[0], dev->base + CDNS_FIFO_MEM);
>>> +		iowrite32(data[1], dev->base + CDNS_FIFO_MEM);
>>> +		period_pos++;
>>> +		if (++tx_ptr >= runtime->buffer_size)
>>> +			tx_ptr = 0;
>>> +	}
>>> +
>>> +	*period_elapsed = period_pos >= runtime->period_size;
>>> +	return tx_ptr;
>>> +}

>>> +	pm_runtime_enable(&pdev->dev);
>>> +	if (pm_runtime_enabled(&pdev->dev))
>>> +		cdns_i2s_runtime_suspend(&pdev->dev);
>>
>> that sequence looks suspicious.... Why would you suspend immediately during the
>> probe? You're probably missing all the autosuspend stuff?
> 
> Since I have enabled clocks before, and the device is in the suspend state after
> pm_runtime_enable(), I need to disable clocks in cdns_i2s_runtime_suspend()
> to match the suspend state.

That is very odd on two counts
a) if you haven't enabled the clocks, why do you need to disbale them?
b) if you do a pm_runtime_enable(), then the branch if
(pm_runtime_enabled) is always true.


> 
>>
>>> +
>>> +	dev_dbg(&pdev->dev, "I2S supports %d stereo channels with %s.\n",
>>> +		i2s->max_channels, ((i2s->irq < 0) ? "dma" : "interrupt"));
>>> +
>>> +	return 0;
>>> +
>>> +err:
>>> +	return ret;
>>> +}
>>> +
>>> +static int cdns_i2s_remove(struct platform_device *pdev) {
>>> +	pm_runtime_disable(&pdev->dev);
>>> +	if (!pm_runtime_status_suspended(&pdev->dev))
>>> +		cdns_i2s_runtime_suspend(&pdev->dev);
>>
>> ... and this one too. Once you've disabled pm_runtime, checking the status is
>> irrelevant...
> 
> I think the clocks need to be always enabled after probe if disable pm_runtime,
> and should be disabled when remove. This will do that.

if you are disabling pm_runtime, then the pm_runtime state becames invalid.
When pm_runtime_disable() is added in remove operations, it's mainly to
prevent the device from suspending.

RE: 回复: [PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller

[Xingyu Wu]

On 02/04/2024 21:57, Pierre-Louis Bossart wrote:
> 
> 
> >>
> >>> +#define PERIODS_MIN		2
> >>> +
> >>> +static unsigned int cdns_i2s_pcm_tx(struct cdns_i2s_dev *dev,
> >>> +				    struct snd_pcm_runtime *runtime,
> >>> +				    unsigned int tx_ptr, bool *period_elapsed,
> >>> +				    snd_pcm_format_t format)
> >>> +{
> >>> +	unsigned int period_pos = tx_ptr % runtime->period_size;
> >>
> >> not following what the modulo is for, usually it's modulo the buffer size?
> >
> > This is to see if the new data is divisible by period_size and to
> > determine whether it is enough for a period_size in the later loop.
> 
> That didn't answer to my question, the position is usually between
> 0..buffer_size.1.

Yes, this position will be used later in the cdns_i2s_pcm_pointer().
But this cdns_i2s_pcm_tx() is called by I2S hardware interrupt which
would be frequently called several times each period. The period_pos
is used to determine whether there is enough a period_size to call
snd_pcm_period_elapsed().

> 
> Doing increments on a modulo value then comparisons as done below seems
> rather questionable.
> 
> >>> +
> >>> +		iowrite32(data[0], dev->base + CDNS_FIFO_MEM);
> >>> +		iowrite32(data[1], dev->base + CDNS_FIFO_MEM);
> >>> +		period_pos++;
> >>> +		if (++tx_ptr >= runtime->buffer_size)
> >>> +			tx_ptr = 0;
> >>> +	}
> >>> +
> >>> +	*period_elapsed = period_pos >= runtime->period_size;
> >>> +	return tx_ptr;
> >>> +}
> 
> >>> +	pm_runtime_enable(&pdev->dev);
> >>> +	if (pm_runtime_enabled(&pdev->dev))
> >>> +		cdns_i2s_runtime_suspend(&pdev->dev);
> >>
> >> that sequence looks suspicious.... Why would you suspend immediately
> >> during the probe? You're probably missing all the autosuspend stuff?
> >
> > Since I have enabled clocks before, and the device is in the suspend
> > state after pm_runtime_enable(), I need to disable clocks in
> > cdns_i2s_runtime_suspend() to match the suspend state.
> 
> That is very odd on two counts
> a) if you haven't enabled the clocks, why do you need to disbale them?
> b) if you do a pm_runtime_enable(), then the branch if
> (pm_runtime_enabled) is always true.
> 

a) It must enable clocks first to read and write registers when I2S probe.
Then it is done to probe, the clocks are still enabled and the state of pm
is suspend. So it need to be disabled to match the state and will resume
and be enabled by ALSA.

b) Because CONFIG_PM would be disabled and pm_runtime_enabled()
return false , then it is no need to disable clock and I2S still can work.

> 
> >
> >>
> >>> +
> >>> +	dev_dbg(&pdev->dev, "I2S supports %d stereo channels with %s.\n",
> >>> +		i2s->max_channels, ((i2s->irq < 0) ? "dma" : "interrupt"));
> >>> +
> >>> +	return 0;
> >>> +
> >>> +err:
> >>> +	return ret;
> >>> +}
> >>> +
> >>> +static int cdns_i2s_remove(struct platform_device *pdev) {
> >>> +	pm_runtime_disable(&pdev->dev);
> >>> +	if (!pm_runtime_status_suspended(&pdev->dev))
> >>> +		cdns_i2s_runtime_suspend(&pdev->dev);
> >>
> >> ... and this one too. Once you've disabled pm_runtime, checking the
> >> status is irrelevant...
> >
> > I think the clocks need to be always enabled after probe if disable
> > pm_runtime, and should be disabled when remove. This will do that.
> 
> if you are disabling pm_runtime, then the pm_runtime state becames invalid.
> When pm_runtime_disable() is added in remove operations, it's mainly to
> prevent the device from suspending.

Should I use the pm_runtime_enabled() before the pm_runtime_disable()?

Best regards,
Xingyu Wu

Re: 回复: [PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller

[Pierre-Louis Bossart]

On 4/16/24 02:23, Xingyu Wu wrote:
> On 02/04/2024 21:57, Pierre-Louis Bossart wrote:
>>
>>
>>>>
>>>>> +#define PERIODS_MIN		2
>>>>> +
>>>>> +static unsigned int cdns_i2s_pcm_tx(struct cdns_i2s_dev *dev,
>>>>> +				    struct snd_pcm_runtime *runtime,
>>>>> +				    unsigned int tx_ptr, bool *period_elapsed,
>>>>> +				    snd_pcm_format_t format)
>>>>> +{
>>>>> +	unsigned int period_pos = tx_ptr % runtime->period_size;
>>>>
>>>> not following what the modulo is for, usually it's modulo the buffer size?
>>>
>>> This is to see if the new data is divisible by period_size and to
>>> determine whether it is enough for a period_size in the later loop.
>>
>> That didn't answer to my question, the position is usually between
>> 0..buffer_size.1.
> 
> Yes, this position will be used later in the cdns_i2s_pcm_pointer().
> But this cdns_i2s_pcm_tx() is called by I2S hardware interrupt which
> would be frequently called several times each period. The period_pos
> is used to determine whether there is enough a period_size to call
> snd_pcm_period_elapsed().
> 
>>
>> Doing increments on a modulo value then comparisons as done below seems
>> rather questionable.
>>
>>>>> +
>>>>> +		iowrite32(data[0], dev->base + CDNS_FIFO_MEM);
>>>>> +		iowrite32(data[1], dev->base + CDNS_FIFO_MEM);
>>>>> +		period_pos++;
>>>>> +		if (++tx_ptr >= runtime->buffer_size)
>>>>> +			tx_ptr = 0;
>>>>> +	}
>>>>> +
>>>>> +	*period_elapsed = period_pos >= runtime->period_size;
>>>>> +	return tx_ptr;
>>>>> +}
>>
>>>>> +	pm_runtime_enable(&pdev->dev);
>>>>> +	if (pm_runtime_enabled(&pdev->dev))
>>>>> +		cdns_i2s_runtime_suspend(&pdev->dev);
>>>>
>>>> that sequence looks suspicious.... Why would you suspend immediately
>>>> during the probe? You're probably missing all the autosuspend stuff?
>>>
>>> Since I have enabled clocks before, and the device is in the suspend
>>> state after pm_runtime_enable(), I need to disable clocks in
>>> cdns_i2s_runtime_suspend() to match the suspend state.
>>
>> That is very odd on two counts
>> a) if you haven't enabled the clocks, why do you need to disbale them?
>> b) if you do a pm_runtime_enable(), then the branch if
>> (pm_runtime_enabled) is always true.
>>
> 
> a) It must enable clocks first to read and write registers when I2S probe.
> Then it is done to probe, the clocks are still enabled and the state of pm
> is suspend. So it need to be disabled to match the state and will resume
> and be enabled by ALSA.

I think you are missing a pm_runtime_set_active() to reconcile the pm
state with the hardware state. The premise of pm_runtime is that on
probe your device is active and later on it will suspend. Having
pm_runtime_enabled with a suspended device without the framework
involved to trigger the transition to suspend is asking for trouble.

> b) Because CONFIG_PM would be disabled and pm_runtime_enabled()
> return false , then it is no need to disable clock and I2S still can work.

Again you are trying to make things more complicated than they need to
be. Don't try to actively manage and query states, let the framework do
it for you.

Try to probe and bring the device to an active state. Then use
pm_runtime_mark_last_busy(), use pm_runtime_enable and let autosuspend
do the work for you. If pm_runtime is not enabled the suspend will not
happen.

Also keep in mind that pm_runtime_enabled() will return false if the
user mucks with the power state in sysfs, it's not only a case of
CONFIG_PM being selected or not.
> 
>>
>>>
>>>>
>>>>> +
>>>>> +	dev_dbg(&pdev->dev, "I2S supports %d stereo channels with %s.\n",
>>>>> +		i2s->max_channels, ((i2s->irq < 0) ? "dma" : "interrupt"));
>>>>> +
>>>>> +	return 0;
>>>>> +
>>>>> +err:
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static int cdns_i2s_remove(struct platform_device *pdev) {
>>>>> +	pm_runtime_disable(&pdev->dev);
>>>>> +	if (!pm_runtime_status_suspended(&pdev->dev))
>>>>> +		cdns_i2s_runtime_suspend(&pdev->dev);
>>>>
>>>> ... and this one too. Once you've disabled pm_runtime, checking the
>>>> status is irrelevant...
>>>
>>> I think the clocks need to be always enabled after probe if disable
>>> pm_runtime, and should be disabled when remove. This will do that.
>>
>> if you are disabling pm_runtime, then the pm_runtime state becames invalid.
>> When pm_runtime_disable() is added in remove operations, it's mainly to
>> prevent the device from suspending.
> 
> Should I use the pm_runtime_enabled() before the pm_runtime_disable()?

It doesn't matter, the problem is the second part where you try to check
the status of pm_runtime *after* disabling it.

RE: 回复: [PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller

[Xingyu Wu]

On 16/04/2024 21:51, Pierre-Louis Bossart wrote:
> 
> 
> On 4/16/24 02:23, Xingyu Wu wrote:
> > On 02/04/2024 21:57, Pierre-Louis Bossart wrote:
> >>
> >>
> >>>>
> >>>>> +#define PERIODS_MIN		2
> >>>>> +
> >>>>> +static unsigned int cdns_i2s_pcm_tx(struct cdns_i2s_dev *dev,
> >>>>> +				    struct snd_pcm_runtime *runtime,
> >>>>> +				    unsigned int tx_ptr, bool
> *period_elapsed,
> >>>>> +				    snd_pcm_format_t format)
> >>>>> +{
> >>>>> +	unsigned int period_pos = tx_ptr % runtime->period_size;
> >>>>
> >>>> not following what the modulo is for, usually it's modulo the buffer size?
> >>>
> >>> This is to see if the new data is divisible by period_size and to
> >>> determine whether it is enough for a period_size in the later loop.
> >>
> >> That didn't answer to my question, the position is usually between
> >> 0..buffer_size.1.
> >
> > Yes, this position will be used later in the cdns_i2s_pcm_pointer().
> > But this cdns_i2s_pcm_tx() is called by I2S hardware interrupt which
> > would be frequently called several times each period. The period_pos
> > is used to determine whether there is enough a period_size to call
> > snd_pcm_period_elapsed().
> >
> >>
> >> Doing increments on a modulo value then comparisons as done below
> >> seems rather questionable.
> >>
> >>>>> +
> >>>>> +		iowrite32(data[0], dev->base + CDNS_FIFO_MEM);
> >>>>> +		iowrite32(data[1], dev->base + CDNS_FIFO_MEM);
> >>>>> +		period_pos++;
> >>>>> +		if (++tx_ptr >= runtime->buffer_size)
> >>>>> +			tx_ptr = 0;
> >>>>> +	}
> >>>>> +
> >>>>> +	*period_elapsed = period_pos >= runtime->period_size;
> >>>>> +	return tx_ptr;
> >>>>> +}
> >>
> >>>>> +	pm_runtime_enable(&pdev->dev);
> >>>>> +	if (pm_runtime_enabled(&pdev->dev))
> >>>>> +		cdns_i2s_runtime_suspend(&pdev->dev);
> >>>>
> >>>> that sequence looks suspicious.... Why would you suspend
> >>>> immediately during the probe? You're probably missing all the autosuspend
> stuff?
> >>>
> >>> Since I have enabled clocks before, and the device is in the suspend
> >>> state after pm_runtime_enable(), I need to disable clocks in
> >>> cdns_i2s_runtime_suspend() to match the suspend state.
> >>
> >> That is very odd on two counts
> >> a) if you haven't enabled the clocks, why do you need to disbale them?
> >> b) if you do a pm_runtime_enable(), then the branch if
> >> (pm_runtime_enabled) is always true.
> >>
> >
> > a) It must enable clocks first to read and write registers when I2S probe.
> > Then it is done to probe, the clocks are still enabled and the state
> > of pm is suspend. So it need to be disabled to match the state and
> > will resume and be enabled by ALSA.
> 
> I think you are missing a pm_runtime_set_active() to reconcile the pm state with
> the hardware state. The premise of pm_runtime is that on probe your device is
> active and later on it will suspend. Having pm_runtime_enabled with a
> suspended device without the framework involved to trigger the transition to
> suspend is asking for trouble.

Great, It is better to use pm_runtime_set_active(). I will modify it in next patch.

> 
> > b) Because CONFIG_PM would be disabled and pm_runtime_enabled() return
> > false , then it is no need to disable clock and I2S still can work.
> 
> Again you are trying to make things more complicated than they need to be.
> Don't try to actively manage and query states, let the framework do it for you.
> 
> Try to probe and bring the device to an active state. Then use
> pm_runtime_mark_last_busy(), use pm_runtime_enable and let autosuspend
> do the work for you. If pm_runtime is not enabled the suspend will not happen.
> 
> Also keep in mind that pm_runtime_enabled() will return false if the user mucks
> with the power state in sysfs, it's not only a case of CONFIG_PM being selected
> or not.

Noted. Thanks.

> >
> >>
> >>>
> >>>>
> >>>>> +
> >>>>> +	dev_dbg(&pdev->dev, "I2S supports %d stereo channels
> with %s.\n",
> >>>>> +		i2s->max_channels, ((i2s->irq < 0) ? "dma" : "interrupt"));
> >>>>> +
> >>>>> +	return 0;
> >>>>> +
> >>>>> +err:
> >>>>> +	return ret;
> >>>>> +}
> >>>>> +
> >>>>> +static int cdns_i2s_remove(struct platform_device *pdev) {
> >>>>> +	pm_runtime_disable(&pdev->dev);
> >>>>> +	if (!pm_runtime_status_suspended(&pdev->dev))
> >>>>> +		cdns_i2s_runtime_suspend(&pdev->dev);
> >>>>
> >>>> ... and this one too. Once you've disabled pm_runtime, checking the
> >>>> status is irrelevant...
> >>>
> >>> I think the clocks need to be always enabled after probe if disable
> >>> pm_runtime, and should be disabled when remove. This will do that.
> >>
> >> if you are disabling pm_runtime, then the pm_runtime state becames invalid.
> >> When pm_runtime_disable() is added in remove operations, it's mainly
> >> to prevent the device from suspending.
> >
> > Should I use the pm_runtime_enabled() before the pm_runtime_disable()?
> 
> It doesn't matter, the problem is the second part where you try to check the
> status of pm_runtime *after* disabling it.
> 

Will fix.

Thanks,
Xingyu Wu