[PATCH 1/7] spi: renesas,sh-msiof: Living separately from MSIOF I2S Sound

[PATCH 1/7] spi: renesas,sh-msiof: Living separately from MSIOF I2S Sound

[Kuninori Morimoto]

Renesas MSIOF (Clock-Synchronized Serial Interface with FIFO) can work as
both SPI and I2S. MSIOF-I2S will use Audio Graph Card/Card2 driver which
uses Of-Graph in DT.

MSIOF-SPI/I2S are using same DT compatible properties.
MSIOF-I2S         uses Of-Graph for Audio-Graph-Card/Card2,
MSIOF-SPI doesn't use  Of-Graph.

Ignore MSIOF-I2S case (= Of-Graph) in MSIOF-SPI Doc.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 .../devicetree/bindings/spi/renesas,sh-msiof.yaml    | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml b/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml
index 49649fc3f95a..c491ef5bc78c 100644
--- a/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml
+++ b/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml
@@ -9,6 +9,18 @@ title: Renesas MSIOF SPI controller
 maintainers:
   - Geert Uytterhoeven <geert+renesas@glider.be>
 
+# sharing with MSIOF I2S
+# see
+# ${LINUX}/Documentation/devicetree/bindings/sound/renesas,msiof.yaml
+select:
+  properties:
+    compatible:
+      contains:
+        pattern: "^renesas,.*-msiof$"
+    port: false
+  required:
+    - compatible
+
 allOf:
   - $ref: spi-controller.yaml#
 
-- 
2.43.0

[PATCH 0/7] ASoC: add Renesas MSIOF sound driver

[Kuninori Morimoto]

Hi Mark, Rob

Renesas MSIOF can work as both SPI and I2S.
Current Linux supports MSIOF-SPI. This patch-set adds new MSIOF-I2S.

One concern is that because it is using same HW-IP, we want to share
same compatible for both MSIOF-SPI/I2S case.
MSIOF-I2S (Sound) will use Audio-Graph-Card/Card2 which uses Of-Graph.
So, this patch-set assumes if DT is using Of-Graph, it is MSIOF-I2S,
otherwise, it is MSIOF-SPI (This assumption will works if SPI *never*
use Of-Graph in the future).

[1/7]-[2/7] are for SPI driver not to detect in I2S case.
[3/7]-[6/7] are for I2S driver.
[7/7] is for DT, but it will be re-posted if [1/7]-[6/7] are accepted.

Kuninori Morimoto (7):
  spi: renesas,sh-msiof: Living separately from MSIOF I2S Sound
  spi: sh-msiof: ignore driver probing if it was MSIOF Sound
  ASoC: rsnd: allow to use ADG only
  ASoC: rsnd: enable to use "adg" clock
  ASoC: renesas: add MSIOF sound Documentation
  ASoC: renesas: add MSIOF sound support
  arm64: dts: renesas: sparrow hawk: Add MSIOF Sound support

 .../bindings/sound/renesas,msiof.yaml         | 112 ++++
 .../bindings/spi/renesas,sh-msiof.yaml        |  12 +
 .../dts/renesas/r8a779g3-sparrow-hawk.dts     |  98 +++
 drivers/spi/spi-sh-msiof.c                    |  10 +
 sound/soc/renesas/Kconfig                     |   7 +
 sound/soc/renesas/rcar/Makefile               |   3 +
 sound/soc/renesas/rcar/adg.c                  |  44 +-
 sound/soc/renesas/rcar/core.c                 |   7 +-
 sound/soc/renesas/rcar/msiof.c                | 579 ++++++++++++++++++
 9 files changed, 862 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/renesas,msiof.yaml
 create mode 100644 sound/soc/renesas/rcar/msiof.c

-- 
2.43.0

[PATCH 2/7] spi: sh-msiof: ignore driver probing if it was MSIOF Sound

[Kuninori Morimoto]

Renesas MSIOF (Clock-Synchronized Serial Interface with FIFO) can work as
both SPI and I2S. MSIOF-I2S will use Audio Graph Card/Card2 driver which
Of-Graph in DT.

MSIOF-SPI/I2S are using same DT compatible properties.
MSIOF-I2S         uses Of-Graph for Audio-Graph-Card/Card2,
MSIOF-SPI doesn't use  Of-Graph.

Check "port" node when driver probing

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/spi/spi-sh-msiof.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index 8a98c313548e..51415131eff1 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -20,6 +20,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/sh_dma.h>
@@ -1276,10 +1277,19 @@ static int sh_msiof_spi_probe(struct platform_device *pdev)
 	const struct sh_msiof_chipdata *chipdata;
 	struct sh_msiof_spi_info *info;
 	struct sh_msiof_spi_priv *p;
+	struct device_node *port;
 	unsigned long clksrc;
 	int i;
 	int ret;
 
+	/* Check whether MSIOF is used as I2S mode or SPI mode by checking "port" node */
+	port = of_graph_get_next_port(pdev->dev.of_node, NULL);
+	if (port) {
+		/* It was MSIOF-I2S */
+		of_node_put(port);
+		return -ENODEV;
+	}
+
 	chipdata = of_device_get_match_data(&pdev->dev);
 	if (chipdata) {
 		info = sh_msiof_spi_parse_dt(&pdev->dev);
-- 
2.43.0

[PATCH 3/7] ASoC: rsnd: allow to use ADG only

[Kuninori Morimoto]

Audio clock generator (= ADG) can be used standalone, but current driver
will be error in such use case. Makes it as not error.
And, current driver registers it as fixed rate clock, but actual clkout
was handled when SSI start works. Setup clkout setting when it was probed.
Otherwise it can't be used ADG only.

Because of this fixup, current rsnd_adg_get_clkout() function name will be
strange. Rename get -> init.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/renesas/rcar/adg.c  | 28 ++++++++++++++++------------
 sound/soc/renesas/rcar/core.c |  7 ++++++-
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/sound/soc/renesas/rcar/adg.c b/sound/soc/renesas/rcar/adg.c
index 191f212d338c..db980e4642b8 100644
--- a/sound/soc/renesas/rcar/adg.c
+++ b/sound/soc/renesas/rcar/adg.c
@@ -377,16 +377,9 @@ int rsnd_adg_ssi_clk_try_start(struct rsnd_mod *ssi_mod, unsigned int rate)
 int rsnd_adg_clk_control(struct rsnd_priv *priv, int enable)
 {
 	struct rsnd_adg *adg = rsnd_priv_to_adg(priv);
-	struct rsnd_mod *adg_mod = rsnd_mod_get(adg);
 	struct clk *clk;
 	int ret = 0, i;
 
-	if (enable) {
-		rsnd_mod_bset(adg_mod, BRGCKR, 0x80770000, adg->ckr);
-		rsnd_mod_write(adg_mod, BRRA,  adg->brga);
-		rsnd_mod_write(adg_mod, BRRB,  adg->brgb);
-	}
-
 	for_each_rsnd_clkin(clk, adg, i) {
 		if (enable) {
 			ret = clk_prepare_enable(clk);
@@ -504,13 +497,14 @@ static void rsnd_adg_unregister_clkout(struct rsnd_priv *priv)
 		clk_unregister_fixed_rate(clk);
 }
 
-static int rsnd_adg_get_clkout(struct rsnd_priv *priv)
+static int rsnd_adg_init_clkout(struct rsnd_priv *priv)
 {
 	struct rsnd_adg *adg = priv->adg;
 	struct clk *clk;
 	struct device *dev = rsnd_priv_to_dev(priv);
 	struct device_node *np = dev->of_node;
 	struct property *prop;
+	struct rsnd_mod *adg_mod = rsnd_mod_get(adg);
 	u32 ckr, brgx, brga, brgb;
 	u32 req_rate[ADG_HZ_SIZE] = {};
 	uint32_t count = 0;
@@ -537,7 +531,7 @@ static int rsnd_adg_get_clkout(struct rsnd_priv *priv)
 	 */
 	prop = of_find_property(np, "clock-frequency", NULL);
 	if (!prop)
-		goto rsnd_adg_get_clkout_end;
+		goto rsnd_adg_init_clkout_end;
 
 	req_size = prop->length / sizeof(u32);
 	if (req_size > ADG_HZ_SIZE) {
@@ -633,7 +627,7 @@ static int rsnd_adg_get_clkout(struct rsnd_priv *priv)
 
 	if (!(adg->brg_rate[ADG_HZ_48]  && req_Hz[ADG_HZ_48]) &&
 	    !(adg->brg_rate[ADG_HZ_441] && req_Hz[ADG_HZ_441]))
-		goto rsnd_adg_get_clkout_end;
+		goto rsnd_adg_init_clkout_end;
 
 	if (approximate)
 		dev_info(dev, "It uses CLK_I as approximate rate");
@@ -682,11 +676,21 @@ static int rsnd_adg_get_clkout(struct rsnd_priv *priv)
 				    &adg->onecell);
 	}
 
-rsnd_adg_get_clkout_end:
+rsnd_adg_init_clkout_end:
 	adg->ckr = ckr;
 	adg->brga = brga;
 	adg->brgb = brgb;
 
+	/*
+	 * setup default clkout
+	 */
+	if (0 == (req_rate[0] % 8000))
+		ckr = 0x80000000; /* use BRGB output */
+
+	rsnd_mod_bset(adg_mod, BRGCKR, 0x80770000, adg->ckr | ckr);
+	rsnd_mod_write(adg_mod, BRRA,  adg->brga);
+	rsnd_mod_write(adg_mod, BRRB,  adg->brgb);
+
 	return 0;
 
 err:
@@ -764,7 +768,7 @@ int rsnd_adg_probe(struct rsnd_priv *priv)
 	if (ret)
 		return ret;
 
-	ret = rsnd_adg_get_clkout(priv);
+	ret = rsnd_adg_init_clkout(priv);
 	if (ret)
 		return ret;
 
diff --git a/sound/soc/renesas/rcar/core.c b/sound/soc/renesas/rcar/core.c
index 30afc942d381..4f4ed24cb361 100644
--- a/sound/soc/renesas/rcar/core.c
+++ b/sound/soc/renesas/rcar/core.c
@@ -1482,8 +1482,13 @@ static int rsnd_dai_probe(struct rsnd_priv *priv)
 	int dai_i;
 
 	nr = rsnd_dai_of_node(priv, &is_graph);
+
+	/*
+	 * There is a case that it is used only for ADG (Sound Clock).
+	 * No DAI is not error
+	 */
 	if (!nr)
-		return -EINVAL;
+		return 0;
 
 	rdrv = devm_kcalloc(dev, nr, sizeof(*rdrv), GFP_KERNEL);
 	rdai = devm_kcalloc(dev, nr, sizeof(*rdai), GFP_KERNEL);
-- 
2.43.0

[PATCH 4/7] ASoC: rsnd: enable to use "adg" clock

[Kuninori Morimoto]

ADG needs its MSTP to use it, and it was handled as "clk_i" before.
R-Car Gen2/Gen3 are using it, but Gen4 doesn't have it.
"clk_i" is not intuitive for ADG MSTP.
Let's enable to use "adg" clock. It can keep compatible with R-Car
Gen2/Gen3 and Gen4.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/renesas/rcar/adg.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/sound/soc/renesas/rcar/adg.c b/sound/soc/renesas/rcar/adg.c
index db980e4642b8..5f132b95a85c 100644
--- a/sound/soc/renesas/rcar/adg.c
+++ b/sound/soc/renesas/rcar/adg.c
@@ -30,6 +30,7 @@ static struct rsnd_mod_ops adg_ops = {
 #define ADG_HZ_SIZE	2
 
 struct rsnd_adg {
+	struct clk *adg;
 	struct clk *clkin[CLKINMAX];
 	struct clk *clkout[CLKOUTMAX];
 	struct clk *null_clk;
@@ -380,6 +381,13 @@ int rsnd_adg_clk_control(struct rsnd_priv *priv, int enable)
 	struct clk *clk;
 	int ret = 0, i;
 
+	/* enable adg */
+	if (enable) {
+		ret = clk_prepare_enable(adg->adg);
+		if (ret < 0)
+			return ret;
+	}
+
 	for_each_rsnd_clkin(clk, adg, i) {
 		if (enable) {
 			ret = clk_prepare_enable(clk);
@@ -408,6 +416,10 @@ int rsnd_adg_clk_control(struct rsnd_priv *priv, int enable)
 	if (ret < 0)
 		rsnd_adg_clk_disable(priv);
 
+	/* disable adg */
+	if (!enable)
+		clk_disable_unprepare(adg->adg);
+
 	return ret;
 }
 
@@ -464,6 +476,16 @@ static int rsnd_adg_get_clkin(struct rsnd_priv *priv)
 		clkin_size = ARRAY_SIZE(clkin_name_gen4);
 	}
 
+	/*
+	 * get adg
+	 * No "adg" is not error
+	 */
+	clk = devm_clk_get(dev, "adg");
+	if (IS_ERR_OR_NULL(clk))
+		clk = rsnd_adg_null_clk_get(priv);
+	adg->adg = clk;
+
+	/* get clkin */
 	for (i = 0; i < clkin_size; i++) {
 		clk = devm_clk_get(dev, clkin_name[i]);
 
-- 
2.43.0

[PATCH 5/7] ASoC: renesas: add MSIOF sound Documentation

[Kuninori Morimoto]

Renesas MSIOF (Clock-Synchronized Serial Interface with FIFO) can work as
both SPI and I2S. MSIOF-I2S will use Audio Graph Card/Card2 driver which
uses Of-Graph in DT.

MSIOF-SPI/I2S are using same DT compatible properties.
MSIOF-I2S         uses Of-Graph for Audio-Graph-Card/Card2,
MSIOF-SPI doesn't use  Of-Graph.

Adds MSIOF-I2S documentation for Sound.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 .../bindings/sound/renesas,msiof.yaml         | 112 ++++++++++++++++++
 1 file changed, 112 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/renesas,msiof.yaml

diff --git a/Documentation/devicetree/bindings/sound/renesas,msiof.yaml b/Documentation/devicetree/bindings/sound/renesas,msiof.yaml
new file mode 100644
index 000000000000..5173e80698fb
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/renesas,msiof.yaml
@@ -0,0 +1,112 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/renesas,msiof.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas MSIOF I2S controller
+
+maintainers:
+  - Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
+
+# sharing with MSIOF SPI
+# see
+# ${LINUX}/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml
+select:
+  properties:
+    compatible:
+      contains:
+        pattern: "renesas,.*-msiof"
+  required:
+    - compatible
+    - port
+
+properties:
+  compatible:
+    items:
+      - const: renesas,msiof-r8a779g0   # R-Car V4H
+      - const: renesas,rcar-gen4-msiof  # generic R-Car Gen4
+
+  reg:
+    minItems: 1
+    maxItems: 2
+    oneOf:
+      - items:
+          - description: CPU and DMA engine registers
+      - items:
+          - description: CPU registers
+          - description: DMA engine registers
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  dmas:
+    minItems: 2
+    maxItems: 4
+
+  dma-names:
+    minItems: 2
+    maxItems: 4
+    items:
+      enum: [ tx, rx ]
+
+  port:
+    $ref: audio-graph-port.yaml#/definitions/port-base
+    unevaluatedProperties: false
+    patternProperties:
+      "^endpoint(@[0-9a-f]+)?":
+        $ref: audio-graph-port.yaml#/definitions/endpoint-base
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - power-domains
+  - port
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/r8a779g0-cpg-mssr.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/power/r8a779g0-sysc.h>
+
+    dummy-codec {
+      compatible = "test-codec";
+
+      port {
+        codec_ep: endpoint {
+          remote-endpoint = <&msiof1_snd_ep>;
+        };
+      };
+    };
+
+    msiof1: serial@e6ea0000 {
+      compatible = "renesas,msiof-r8a779g0",
+                   "renesas,rcar-gen4-msiof";
+      reg = <0 0xe6ea0000 0 0x0064>;
+      interrupts = <GIC_SPI 240 IRQ_TYPE_LEVEL_HIGH>;
+      clocks = <&cpg CPG_MOD 619>;
+      dmas = <&dmac0 0x43>, <&dmac0 0x42>,
+             <&dmac1 0x43>, <&dmac1 0x42>;
+      dma-names = "tx", "rx", "tx", "rx";
+      power-domains = <&sysc R8A779G0_PD_ALWAYS_ON>;
+      resets = <&cpg 619>;
+
+      port {
+        msiof1_snd_ep: endpoint {
+          remote-endpoint = <&codec_ep>;
+        };
+      };
+    };
-- 
2.43.0

[PATCH 6/7] ASoC: renesas: add MSIOF sound support

[Kuninori Morimoto]

Renesas MSIOF (Clock-Synchronized Serial Interface with FIFO) can work as
both SPI and I2S. Adds MSIOF-I2S driver.

MSIOF-SPI/I2S are using same DT compatible properties.
MSIOF-I2S         uses Of-Graph for Audio-Graph-Card/Card2,
MSIOF-SPI doesn't use  Of-Graph.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/renesas/Kconfig       |   7 +
 sound/soc/renesas/rcar/Makefile |   3 +
 sound/soc/renesas/rcar/msiof.c  | 579 ++++++++++++++++++++++++++++++++
 3 files changed, 589 insertions(+)
 create mode 100644 sound/soc/renesas/rcar/msiof.c

diff --git a/sound/soc/renesas/Kconfig b/sound/soc/renesas/Kconfig
index cb01fb36355f..dabf02a955ca 100644
--- a/sound/soc/renesas/Kconfig
+++ b/sound/soc/renesas/Kconfig
@@ -46,6 +46,13 @@ config SND_SOC_RCAR
 	help
 	  This option enables R-Car SRU/SCU/SSIU/SSI sound support
 
+config SND_SOC_MSIOF
+	tristate "R-Car series MSIOF support"
+	depends on OF
+	select SND_DMAENGINE_PCM
+	help
+	  This option enables R-Car MSIOF sound support
+
 config SND_SOC_RZ
 	tristate "RZ/G2L series SSIF-2 support"
 	depends on ARCH_RZG2L || COMPILE_TEST
diff --git a/sound/soc/renesas/rcar/Makefile b/sound/soc/renesas/rcar/Makefile
index 45eb875a912a..3a2c875595bd 100644
--- a/sound/soc/renesas/rcar/Makefile
+++ b/sound/soc/renesas/rcar/Makefile
@@ -1,3 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 snd-soc-rcar-y		:= core.o gen.o dma.o adg.o ssi.o ssiu.o src.o ctu.o mix.o dvc.o cmd.o debugfs.o
 obj-$(CONFIG_SND_SOC_RCAR)	+= snd-soc-rcar.o
+
+snd-soc-msiof-y			:= msiof.o
+obj-$(CONFIG_SND_SOC_MSIOF)	+= snd-soc-msiof.o
diff --git a/sound/soc/renesas/rcar/msiof.c b/sound/soc/renesas/rcar/msiof.c
new file mode 100644
index 000000000000..de8de3468402
--- /dev/null
+++ b/sound/soc/renesas/rcar/msiof.c
@@ -0,0 +1,579 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Renesas R-Car MSIOF (Clock-Synchronized Serial Interface with FIFO) I2S driver
+//
+// Copyright (C) 2025 Renesas Solutions Corp.
+// Author: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
+//
+
+/*
+ * [NOTE]
+ *
+ * This driver doesn't support Clock/Frame Provider Mode
+ *
+ * Basically MSIOF is created for SPI, but we can use it as I2S (Sound). Because of it, when we use
+ * it as I2S (Sound) with Provider Mode, we need to send dummy TX data even though it is used for
+ * RX. Because SPI HW needs TX Clock/Frame output for RX purpose also. It makes driver code complex.
+ *
+ * And when we use MSIOF (Sound) as Provider Mode, the clock source is [MSO clock] (= 133.33MHz)
+ * SoC internal clock. It is not for 48kHz/44.1kHz base clock. Thus the output/input will not be
+ * accurate sound.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_dma.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <sound/dmaengine_pcm.h>
+#include <sound/soc.h>
+
+/* register */
+#define SITMDR1		0x00
+#define SITMDR2		0x04
+#define SITMDR3		0x08
+#define SIRMDR1		0x10
+#define SIRMDR2		0x14
+#define SIRMDR3		0x18
+#define SITSCR		0x20
+#define SICTR		0x28
+#define SIFCTR		0x30
+#define SISTR		0x40
+#define SIIER		0x44
+#define SITFDR		0x50
+#define SIRFDR		0x60
+
+/* SITMDR1/ SIRMDR1 */
+#define TRMD		(1 << 31)		/* Transfer Mode (1 = Master mode) */
+#define PCON		(1 << 30)		/* Transfer Signal Connection */
+#define SYNCMD_SPI	(2 << 28)		/* Level mode/SPI */
+#define SYNCMD_LR	(3 << 28)		/* L/R mode */
+#define SYNCAC		(1 << 25)		/* Sync Polarity (Active-low) */
+#define DTDL_1		(1 << 20)		/* 1-clock-cycle delay */
+#define TXSTP		(1 <<  0)		/* Transmission/Reception Stop on FIFO */
+
+/* SITMDR2 and SIRMDR2 */
+#define BITLEN1(x)	(((x) - 1) << 24)	/* Data Size (8-32 bits) */
+#define WDLEN1(x)	(((x) - 1) << 16)	/* Word Count (1-64/256 (SH, A1))) */
+#define GRP		(1 << 30)		/* Group count */
+
+/* SITSCR */
+#define SITSCR_V(p, d)	((p << 8) + d)
+
+/* SICTR */
+#define TEDG		(1 << 27)		/* Transmit Timing (1 = falling edge) */
+#define REDG		(1 << 26)		/* Receive  Timing (1 = rising  edge) */
+#define TSCKE		(1 << 15)		/* Transmit Serial Clock Output Enable */
+#define TFSE		(1 << 14)		/* Transmit Frame Sync Signal Output Enable */
+#define TXE		(1 <<  9)		/* Transmit Enable */
+#define RXE		(1 <<  8)		/* Receive Enable */
+#define TXRST		(1 <<  1)		/* Transmit Reset */
+#define RXRST		(1 <<  0)		/* Receive Reset */
+
+/* SISTR */
+#define TFEMP		(1 << 29)		/* Transmit FIFO Empty */
+#define TDREQ		(1 << 28)		/* Transmit Data Transfer Request */
+#define TEOF		(1 << 23)		/* Frame Transmission End */
+#define TFSERR		(1 << 21)		/* Transmit Frame Synchronization Error */
+#define TFOVF		(1 << 20)		/* Transmit FIFO Overflow */
+#define TFUDF		(1 << 19)		/* Transmit FIFO Underflow */
+#define RFFUL		(1 << 13)		/* Receive FIFO Full */
+#define RDREQ		(1 << 12)		/* Receive Data Transfer Request */
+#define REOF		(1 <<  7)		/* Frame Reception End */
+#define RFSERR		(1 <<  5)		/* Receive Frame Synchronization Error */
+#define RFUDF		(1 <<  4)		/* Receive FIFO Underflow */
+#define RFOVF		(1 <<  3)		/* Receive FIFO Overflow */
+#define SISTR_ERR_TX	(TFSERR | TFOVF | TFUDF)
+#define SISTR_ERR_RX	(RFSERR | RFOVF | RFUDF)
+#define SISTR_ERR	(SISTR_ERR_TX | SISTR_ERR_RX)
+
+/* SIIER */
+#define TDMAE		(1 << 31)		/* Transmit Data DMA Transfer Req. Enable */
+#define TDREQE		(1 << 28)		/* Transmit Data Transfer Request Enable */
+#define RDMAE		(1 << 15)		/* Receive Data DMA Transfer Req. Enable */
+#define RDREQE		(1 << 12)		/* Receive Data Transfer Request Enable */
+
+/*
+ * The data on memory in 24bit case is located at <rigth> side
+ *	[  xxxxxx]
+ *	[  xxxxxx]
+ *	[  xxxxxx]
+ *
+ * HW assuming signal in 24bit case is located at <left> side
+ *	---+        +--------+
+ *	   +--------+        +--------+...
+ *	   [xxxxx  ][xxxxx  ][xxxxx  ]
+ *
+ * When we use 24bit data, it will be transfered via 32bit width via DMA,
+ * and MSIOF/DMA doesn't support data shift, we can't use 24bit data correctly.
+ * There is no such issue on 16/32bit data case.
+ */
+#define MSIOF_RATES	SNDRV_PCM_RATE_8000_192000
+#define MSIOF_FMTS	(SNDRV_PCM_FMTBIT_S16_LE |\
+			 SNDRV_PCM_FMTBIT_S32_LE)
+
+struct msiof_priv {
+	struct device *dev;
+	struct snd_pcm_substream *substream[SNDRV_PCM_STREAM_LAST + 1];
+	spinlock_t lock;
+	void __iomem *base;
+	resource_size_t phy_addr;
+
+	/* for error */
+	int err_syc[SNDRV_PCM_STREAM_LAST + 1];
+	int err_ovf[SNDRV_PCM_STREAM_LAST + 1];
+	int err_udf[SNDRV_PCM_STREAM_LAST + 1];
+
+	/* bit field */
+	u32 flags;
+#define MSIOF_FLAGS_NEED_DELAY		(1 << 0)
+};
+#define msiof_flag_has(priv, flag)	(priv->flags &  flag)
+#define msiof_flag_set(priv, flag)	(priv->flags |= flag)
+
+#define msiof_is_play(substream)	(substream)->stream == SNDRV_PCM_STREAM_PLAYBACK
+#define msiof_read(priv, reg)		ioread32((priv)->base + reg)
+#define msiof_write(priv, reg, val)	iowrite32(val, (priv)->base + reg)
+#define msiof_status_clear(priv)	msiof_write(priv, SISTR, SISTR_ERR)
+
+static void msiof_update(struct msiof_priv *priv, u32 reg, u32 mask, u32 val)
+{
+	u32 old = msiof_read(priv, reg);
+	u32 new = (old & ~mask) | (val & mask);
+
+	if (old != new)
+		msiof_write(priv, reg, new);
+}
+
+static void msiof_update_and_wait(struct msiof_priv *priv, u32 reg, u32 mask, u32 val, u32 expect)
+{
+	u32 data;
+	int ret;
+
+	msiof_update(priv, reg, mask, val);
+
+	ret = readl_poll_timeout_atomic(priv->base + reg, data,
+					(data & mask) == expect, 1, 128);
+	if (ret)
+		dev_warn(priv->dev, "write timeout [0x%02x] 0x%08x / 0x%08x\n",
+			 reg, data, expect);
+}
+
+static int msiof_hw_start(struct snd_soc_component *component,
+			  struct snd_pcm_substream *substream, int cmd)
+{
+	struct msiof_priv *priv = snd_soc_component_get_drvdata(component);
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	int is_play = msiof_is_play(substream);
+	int width = snd_pcm_format_width(runtime->format);
+	u32 val;
+
+	/*
+	 * see
+	 *	Datasheet 109.3.6 [Transmit and Receive Procedures]
+	 *
+	 *	TX: Fig 109.14	- Fig 109.23
+	 *	RX: Fig 109.15
+	 */
+
+	/* reset errors */
+	priv->err_syc[substream->stream] =
+	priv->err_ovf[substream->stream] =
+	priv->err_udf[substream->stream] = 0;
+
+	/* SITMDRx */
+	if (is_play) {
+		val = PCON | SYNCMD_LR | SYNCAC | TXSTP;
+		if (msiof_flag_has(priv, MSIOF_FLAGS_NEED_DELAY))
+			val |= DTDL_1;
+
+		msiof_write(priv, SITMDR1, val);
+
+		val = BITLEN1(width);
+		msiof_write(priv, SITMDR2, val | GRP);
+		msiof_write(priv, SITMDR3, val);
+
+	}
+	/* SIRMDRx */
+	else {
+		val = SYNCMD_LR | SYNCAC;
+		if (msiof_flag_has(priv, MSIOF_FLAGS_NEED_DELAY))
+			val |= DTDL_1;
+
+		msiof_write(priv, SIRMDR1, val);
+
+		val = BITLEN1(width);
+		msiof_write(priv, SIRMDR2, val | GRP);
+		msiof_write(priv, SIRMDR3, val);
+	}
+
+	/* SIIER */
+	if (is_play)
+		val = TDREQE | TDMAE | SISTR_ERR_TX;
+	else
+		val = RDREQE | RDMAE | SISTR_ERR_RX;
+	msiof_update(priv, SIIER, val, val);
+
+	/* SICTR */
+	if (is_play)
+		val = TXE | TEDG;
+	else
+		val = RXE | REDG;
+	msiof_update_and_wait(priv, SICTR, val, val, val);
+
+	msiof_status_clear(priv);
+
+	/* Start DMAC */
+	snd_dmaengine_pcm_trigger(substream, cmd);
+
+	return 0;
+}
+
+static int msiof_hw_stop(struct snd_soc_component *component,
+			 struct snd_pcm_substream *substream, int cmd)
+{
+	struct msiof_priv *priv = snd_soc_component_get_drvdata(component);
+	struct device *dev = component->dev;
+	int is_play = msiof_is_play(substream);
+	u32 val;
+
+	/* SIIER */
+	if (is_play)
+		val = TDREQE | TDMAE | SISTR_ERR_TX;
+	else
+		val = RDREQE | RDMAE | SISTR_ERR_RX;
+	msiof_update(priv, SIIER, val, 0);
+
+	/* Stop DMAC */
+	snd_dmaengine_pcm_trigger(substream, cmd);
+
+	/* SICTR */
+	if (is_play)
+		val = TXE;
+	else
+		val = RXE;
+	msiof_update_and_wait(priv, SICTR, val, 0, 0);
+
+	/* indicate error status if exist */
+	if (priv->err_syc[substream->stream] ||
+	    priv->err_ovf[substream->stream] ||
+	    priv->err_udf[substream->stream])
+		dev_warn(dev, "FSERR(%s) = %d, FOVF = %d, FUDF = %d\n",
+			 snd_pcm_direction_name(substream->stream),
+			 priv->err_syc[substream->stream],
+			 priv->err_ovf[substream->stream],
+			 priv->err_udf[substream->stream]);
+
+	return 0;
+}
+
+static int msiof_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
+{
+	struct msiof_priv *priv = snd_soc_dai_get_drvdata(dai);
+
+	switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
+	/* it supports Clock/Frame consumer mode only */
+	case SND_SOC_DAIFMT_BC_FC:
+		break;
+	/* others are error */
+	default:
+		return -EINVAL;
+	}
+
+	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
+	/* it supports NB_NF only */
+	case SND_SOC_DAIFMT_NB_NF:
+	default:
+		break;
+	/* others are error */
+	case SND_SOC_DAIFMT_NB_IF:
+	case SND_SOC_DAIFMT_IB_NF:
+	case SND_SOC_DAIFMT_IB_IF:
+		return -EINVAL;
+	}
+
+	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_I2S:
+		msiof_flag_set(priv, MSIOF_FLAGS_NEED_DELAY);
+		break;
+	case SND_SOC_DAIFMT_LEFT_J:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/*
+ * Select below from Sound Card, not auto
+ *	SND_SOC_DAIFMT_CBC_CFC
+ *	SND_SOC_DAIFMT_CBP_CFP
+ */
+static const u64 msiof_dai_formats = SND_SOC_POSSIBLE_DAIFMT_I2S	|
+				     SND_SOC_POSSIBLE_DAIFMT_LEFT_J	|
+				     SND_SOC_POSSIBLE_DAIFMT_NB_NF;
+
+static const struct snd_soc_dai_ops msiof_dai_ops = {
+	.set_fmt			= msiof_dai_set_fmt,
+	.auto_selectable_formats	= &msiof_dai_formats,
+	.num_auto_selectable_formats	= 1,
+};
+
+static struct snd_soc_dai_driver msiof_dai_driver = {
+	.name = "msiof-dai",
+	.playback = {
+		.rates		= MSIOF_RATES,
+		.formats	= MSIOF_FMTS,
+		.channels_min	= 2,
+		.channels_max	= 2,
+	},
+	.capture = {
+		.rates		= MSIOF_RATES,
+		.formats	= MSIOF_FMTS,
+		.channels_min	= 2,
+		.channels_max	= 2,
+	},
+	.ops = &msiof_dai_ops,
+};
+
+static struct snd_pcm_hardware msiof_pcm_hardware = {
+	.info =	SNDRV_PCM_INFO_INTERLEAVED	|
+		SNDRV_PCM_INFO_MMAP		|
+		SNDRV_PCM_INFO_MMAP_VALID,
+	.buffer_bytes_max	= 64 * 1024,
+	.period_bytes_min	= 32,
+	.period_bytes_max	= 8192,
+	.periods_min		= 1,
+	.periods_max		= 32,
+	.fifo_size		= 64,
+};
+
+static int msiof_open(struct snd_soc_component *component,
+		      struct snd_pcm_substream *substream)
+{
+	struct device *dev = component->dev;
+	struct dma_chan *chan;
+	static const char *dma_names[] = {"rx", "tx"};
+	int is_play = msiof_is_play(substream);
+	int ret;
+
+	chan = of_dma_request_slave_channel(dev->of_node, dma_names[is_play]);
+	if (IS_ERR_OR_NULL(chan))
+		return PTR_ERR(chan);
+
+	ret = snd_dmaengine_pcm_open(substream, chan);
+	if (ret < 0)
+		goto open_err_dma;
+
+	snd_soc_set_runtime_hwparams(substream, &msiof_pcm_hardware);
+
+	ret = snd_pcm_hw_constraint_integer(substream->runtime, SNDRV_PCM_HW_PARAM_PERIODS);
+
+open_err_dma:
+	if (ret < 0)
+		dma_release_channel(chan);
+
+	return ret;
+}
+
+static int msiof_close(struct snd_soc_component *component,
+		       struct snd_pcm_substream *substream)
+{
+	return snd_dmaengine_pcm_close_release_chan(substream);
+}
+
+static snd_pcm_uframes_t msiof_pointer(struct snd_soc_component *component,
+				       struct snd_pcm_substream *substream)
+{
+	return snd_dmaengine_pcm_pointer(substream);
+}
+
+#define PREALLOC_BUFFER		(32 * 1024)
+#define PREALLOC_BUFFER_MAX	(32 * 1024)
+static int msiof_new(struct snd_soc_component *component,
+		     struct snd_soc_pcm_runtime *rtd)
+{
+	snd_pcm_set_managed_buffer_all(rtd->pcm, SNDRV_DMA_TYPE_DEV,
+				       rtd->card->snd_card->dev,
+				       PREALLOC_BUFFER, PREALLOC_BUFFER_MAX);
+	return 0;
+}
+
+static int msiof_trigger(struct snd_soc_component *component,
+			 struct snd_pcm_substream *substream, int cmd)
+{
+	struct device *dev = component->dev;
+	struct msiof_priv *priv = dev_get_drvdata(dev);
+	unsigned long flags;
+	int ret = -EINVAL;
+
+	spin_lock_irqsave(&priv->lock, flags);
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+		priv->substream[substream->stream] = substream;
+		fallthrough;
+	case SNDRV_PCM_TRIGGER_RESUME:
+		ret = msiof_hw_start(component, substream, cmd);
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+		priv->substream[substream->stream] = NULL;
+		fallthrough;
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+		ret = msiof_hw_stop(component, substream, cmd);
+		break;
+	}
+
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return ret;
+}
+
+static int msiof_hw_params(struct snd_soc_component *component,
+			   struct snd_pcm_substream *substream,
+			   struct snd_pcm_hw_params *params)
+{
+	struct msiof_priv *priv = dev_get_drvdata(component->dev);
+	struct dma_chan *chan = snd_dmaengine_pcm_get_chan(substream);
+	struct dma_slave_config cfg = {};
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&priv->lock, flags);
+
+	ret = snd_hwparams_to_dma_slave_config(substream, params, &cfg);
+	if (ret < 0)
+		goto hw_params_out;
+
+	cfg.dst_addr = priv->phy_addr + SITFDR;
+	cfg.src_addr = priv->phy_addr + SIRFDR;
+
+	ret = dmaengine_slave_config(chan, &cfg);
+hw_params_out:
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return ret;
+}
+
+static const struct snd_soc_component_driver msiof_component_driver = {
+	.name		= "msiof",
+	.open		= msiof_open,
+	.close		= msiof_close,
+	.pointer	= msiof_pointer,
+	.pcm_construct	= msiof_new,
+	.trigger	= msiof_trigger,
+	.hw_params	= msiof_hw_params,
+};
+
+static irqreturn_t msiof_interrupt(int irq, void *data)
+{
+	struct msiof_priv *priv = data;
+	struct snd_pcm_substream *substream;
+	u32 sistr;
+
+	spin_lock(&priv->lock);
+
+	sistr = msiof_read(priv, SISTR);
+	msiof_status_clear(priv);
+
+	spin_unlock(&priv->lock);
+
+	/* overflow/underflow error */
+	substream = priv->substream[SNDRV_PCM_STREAM_PLAYBACK];
+	if (substream && (sistr & SISTR_ERR_TX)) {
+		// snd_pcm_stop_xrun(substream);
+		if (sistr & TFSERR)
+			priv->err_syc[SNDRV_PCM_STREAM_PLAYBACK]++;
+		if (sistr & TFOVF)
+			priv->err_ovf[SNDRV_PCM_STREAM_PLAYBACK]++;
+		if (sistr & TFUDF)
+			priv->err_udf[SNDRV_PCM_STREAM_PLAYBACK]++;
+	}
+
+	substream = priv->substream[SNDRV_PCM_STREAM_CAPTURE];
+	if (substream && (sistr & SISTR_ERR_RX)) {
+		// snd_pcm_stop_xrun(substream);
+		if (sistr & RFSERR)
+			priv->err_syc[SNDRV_PCM_STREAM_CAPTURE]++;
+		if (sistr & RFOVF)
+			priv->err_ovf[SNDRV_PCM_STREAM_CAPTURE]++;
+		if (sistr & RFUDF)
+			priv->err_udf[SNDRV_PCM_STREAM_CAPTURE]++;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int msiof_probe(struct platform_device *pdev)
+{
+	struct msiof_priv *priv;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct device_node *port;
+	int irq, ret;
+
+	/* Check MSIOF as Sound mode or SPI mode */
+	port = of_graph_get_next_port(dev->of_node, NULL);
+	if (!port)
+		return -ENODEV;
+	of_node_put(port);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0)
+		return -ENODEV;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENODEV;
+
+	priv->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	ret = devm_request_irq(dev, irq, msiof_interrupt, 0, dev_name(dev), priv);
+	if (ret)
+		return ret;
+
+	priv->dev	= dev;
+	priv->phy_addr	= res->start;
+
+	spin_lock_init(&priv->lock);
+	platform_set_drvdata(pdev, priv);
+
+	devm_pm_runtime_enable(dev);
+
+	ret = devm_snd_soc_register_component(dev, &msiof_component_driver,
+					      &msiof_dai_driver, 1);
+	if (ret)
+		return ret;
+
+	dev_info(dev, "probed\n");
+
+	return ret;
+}
+
+static const struct of_device_id msiof_of_match[] = {
+	{ .compatible = "renesas,rcar-gen4-msiof", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, msiof_of_match);
+
+static struct platform_driver msiof_driver = {
+	.driver 	= {
+		.name	= "msiof-pcm-audio",
+		.of_match_table = msiof_of_match,
+	},
+	.probe		= msiof_probe,
+};
+module_platform_driver(msiof_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Renesas R-Car MSIOF I2S audio driver");
+MODULE_AUTHOR("Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>");
+MODULE_ALIAS("platform:msiof-pcm-audio");
-- 
2.43.0

[PATCH 7/7] arm64: dts: renesas: sparrow hawk: Add MSIOF Sound support

[Kuninori Morimoto]

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 .../dts/renesas/r8a779g3-sparrow-hawk.dts     | 98 +++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a779g3-sparrow-hawk.dts b/arch/arm64/boot/dts/renesas/r8a779g3-sparrow-hawk.dts
index b54d45115a85..0a4a9e1f85a5 100644
--- a/arch/arm64/boot/dts/renesas/r8a779g3-sparrow-hawk.dts
+++ b/arch/arm64/boot/dts/renesas/r8a779g3-sparrow-hawk.dts
@@ -4,6 +4,29 @@
  *
  * Copyright (C) 2025 Marek Vasut <marek.vasut+renesas@mailbox.org>
  */
+/*
+ * DA7212 Codec settings
+ *
+ * for Playback
+ *	> amixer set "Headphone" 40%
+ *	> amixer set "Headphone" on
+ *	> amixer set "Mixout Left DAC Left"  on
+ *	> amixer set "Mixout Right DAC Right" on
+ *
+ * for Capture (Aux/Mic)
+ *	> amixer set "Aux" on
+ *	> amixer set "Aux" 80%
+ *	> amixer set "Mixin PGA" on
+ *	> amixer set "Mixin PGA" 50%
+ *	> amixer set "ADC" on
+ *	> amixer set "ADC" 80%
+ *	> amixer set "Mixin Left Aux Left" on
+ *	> amixer set "Mixin Right Aux Right" on
+ *	> amixer set "Mic 1" on
+ *	> amixer set "Mic 1" 80%
+ *	> amixer set "Mixin Left Mic 1" on
+ *	> amixer set "Mixin Right Mic 1" on
+ */
 
 /dts-v1/;
 #include <dt-bindings/gpio/gpio.h>
@@ -150,6 +173,12 @@ vcc_sdhi: regulator-vcc-sdhi {
 		gpios-states = <1>;
 		states = <3300000 0>, <1800000 1>;
 	};
+
+	/* Page 30 / Audio_Codec */
+	sound_card: sound {
+		compatible = "audio-graph-card2";
+		links = <&msiof1_snd>;
+	};
 };
 
 /* Page 22 / Ether_AVB0 */
@@ -341,6 +370,29 @@ i2c0_mux1: i2c@1 {
 			reg = <1>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+
+			/* Page 30 / Audio_Codec */
+			codec@1a {
+				compatible = "dlg,da7212";
+
+				#sound-dai-cells = <0>;
+				reg = <0x1a>;
+
+				clocks = <&rcar_sound>;
+				clock-names = "mclk";
+
+				VDDA-supply   = <&reg_1p8v>;
+				VDDMIC-supply = <&reg_3p3v>;
+				VDDIO-supply  = <&reg_3p3v>;
+
+				port {
+					da7212_endpoint: endpoint {
+						bitclock-master;
+						frame-master;
+						remote-endpoint = <&msiof1_snd_endpoint>;
+					};
+				};
+			};
 		};
 
 		i2c0_mux2: i2c@2 {
@@ -603,6 +655,52 @@ sd_uhs_pins: sd-uhs {
 		function = "mmc";
 		power-source = <1800>;
 	};
+
+	/* Page 30 / Audio_Codec */
+	msiof1_pins: sound {
+		groups = "msiof1_clk", "msiof1_sync", "msiof1_txd", "msiof1_rxd";
+		function = "msiof1";
+	};
+
+	/* Page 30 / Audio_Codec */
+	sound_clk_pins: sound-clk {
+		groups = "audio_clkin", "audio_clkout";
+		function = "audio_clk";
+	};
+};
+
+&audio_clkin {
+	clock-frequency = <24576000>;
+};
+
+/* Page 30 / Audio_Codec */
+&rcar_sound {
+	pinctrl-0 = <&sound_clk_pins>;
+	pinctrl-names = "default";
+
+	/* It is used for ADG output as DA7212_MCLK */
+
+	/* audio_clkout */
+	clock-frequency = <12288000>; /* 48 kHz groups */
+
+	status = "okay";
+};
+
+&msiof1 {
+	pinctrl-0 = <&msiof1_pins>;
+	pinctrl-names = "default";
+
+	status = "okay";
+
+	/* ignore DT warning */
+	/delete-property/#address-cells;
+	/delete-property/#size-cells;
+
+	msiof1_snd: port {
+		msiof1_snd_endpoint: endpoint {
+			remote-endpoint = <&da7212_endpoint>;
+		};
+	};
 };
 
 /* Page 31 / FAN */
-- 
2.43.0

Re: [PATCH 1/7] spi: renesas,sh-msiof: Living separately from MSIOF I2S Sound

[Krzysztof Kozlowski]

On 18/03/2025 03:06, Kuninori Morimoto wrote:
> Renesas MSIOF (Clock-Synchronized Serial Interface with FIFO) can work as
> both SPI and I2S. MSIOF-I2S will use Audio Graph Card/Card2 driver which
> uses Of-Graph in DT.
> 
> MSIOF-SPI/I2S are using same DT compatible properties.
> MSIOF-I2S         uses Of-Graph for Audio-Graph-Card/Card2,
> MSIOF-SPI doesn't use  Of-Graph.
> 
> Ignore MSIOF-I2S case (= Of-Graph) in MSIOF-SPI Doc.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

Missing dt-bindings.

> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  .../devicetree/bindings/spi/renesas,sh-msiof.yaml    | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml b/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml
> index 49649fc3f95a..c491ef5bc78c 100644
> --- a/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml
> +++ b/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml
> @@ -9,6 +9,18 @@ title: Renesas MSIOF SPI controller
>  maintainers:
>    - Geert Uytterhoeven <geert+renesas@glider.be>
>  
> +# sharing with MSIOF I2S

That's not how you create common schemas. You need to separate schema
which is then referenced with $ref with all users.

> +# see
> +# ${LINUX}/Documentation/devicetree/bindings/sound/renesas,msiof.yaml

In anycase drop ${linux}, so the path is verifiable.... which leads to
problem: I do not see that file in next.

> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        pattern: "^renesas,.*-msiof$"

You don't get custom selects. This replaces existing select which breaks
the binding.


Best regards,
Krzysztof

Re: [PATCH 5/7] ASoC: renesas: add MSIOF sound Documentation

[Krzysztof Kozlowski]

On 09/04/2025 03:05, Kuninori Morimoto wrote:
> Renesas MSIOF (Clock-Synchronized Serial Interface with FIFO) can work as
> both SPI and I2S. MSIOF-I2S will use Audio Graph Card/Card2 driver which
> uses Of-Graph in DT.
> 

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

A nit, subject: drop second/last, redundant "Documentation". The
"dt-bindings" prefix is already stating that these are documentation.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18


> MSIOF-SPI/I2S are using same DT compatible properties.
> MSIOF-I2S         uses Of-Graph for Audio-Graph-Card/Card2,
> MSIOF-SPI doesn't use  Of-Graph.
> 
> Adds MSIOF-I2S documentation for Sound.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  .../bindings/sound/renesas,msiof.yaml         | 112 ++++++++++++++++++
>  1 file changed, 112 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/renesas,msiof.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/renesas,msiof.yaml b/Documentation/devicetree/bindings/sound/renesas,msiof.yaml
> new file mode 100644
> index 000000000000..5173e80698fb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/renesas,msiof.yaml
> @@ -0,0 +1,112 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/renesas,msiof.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas MSIOF I2S controller
> +
> +maintainers:
> +  - Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> +
> +# sharing with MSIOF SPI
> +# see
> +# ${LINUX}/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        pattern: "renesas,.*-msiof"
> +  required:
> +    - compatible
> +    - port

Drop entire select.

> +
> +properties:
> +  compatible:
> +    items:
> +      - const: renesas,msiof-r8a779g0   # R-Car V4H


Use expected format of all soc compatibles. It has been always: SoC-module.

> +      - const: renesas,rcar-gen4-msiof  # generic R-Car Gen4

If you have duplicated compatibles then:
1. It rarely makes sense because you claim that two different devices
are using the same compatible. Different device, different compatible.
2. Or if this is really same device, then only one schema.

> +
> +  reg:
> +    minItems: 1
> +    maxItems: 2

Drop these two.

> +    oneOf:

Why is this flexible?

> +      - items:
> +          - description: CPU and DMA engine registers
> +      - items:
> +          - description: CPU registers
> +          - description: DMA engine registers
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  dmas:
> +    minItems: 2
> +    maxItems: 4

Why flexible?

> +
> +  dma-names:
> +    minItems: 2
> +    maxItems: 4
> +    items:
> +      enum: [ tx, rx ]

How would that work? tx rx tx rx? And then driver requests 'tx' (by
name) and what is supposed to be returned?

> +
> +  port:
> +    $ref: audio-graph-port.yaml#/definitions/port-base
> +    unevaluatedProperties: false
> +    patternProperties:
> +      "^endpoint(@[0-9a-f]+)?":
> +        $ref: audio-graph-port.yaml#/definitions/endpoint-base
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - power-domains
> +  - port
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/r8a779g0-cpg-mssr.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/power/r8a779g0-sysc.h>
> +
> +    dummy-codec {
> +      compatible = "test-codec";
> +
> +      port {
> +        codec_ep: endpoint {
> +          remote-endpoint = <&msiof1_snd_ep>;
> +        };
> +      };
> +    };

Drop, not related to the binding.

> +
> +    msiof1: serial@e6ea0000 {

serial means UART controller. You need name matching the class of the
device.
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +      compatible = "renesas,msiof-r8a779g0",
> +                   "renesas,rcar-gen4-msiof";
> +      reg = <0 0xe6ea0000 0 0x0064>;
> +      interrupts = <GIC_SPI 240 IRQ_TYPE_LEVEL_HIGH>;
> +      clocks = <&cpg CPG_MOD 619>;
> +      dmas = <&dmac0 0x43>, <&dmac0 0x42>,
> +             <&dmac1 0x43>, <&dmac1 0x42>;
> +      dma-names = "tx", "rx", "tx", "rx";

So test it now - get DMA by name 'tx'. What do you get?

Also schema should fail here.

> +      power-domains = <&sysc R8A779G0_PD_ALWAYS_ON>;
> +      resets = <&cpg 619>;
> +
> +      port {
> +        msiof1_snd_ep: endpoint {
> +          remote-endpoint = <&codec_ep>;
> +        };
> +      };
> +    };


Best regards,
Krzysztof

Re: [PATCH 6/7] ASoC: renesas: add MSIOF sound support

[Krzysztof Kozlowski]

On 09/04/2025 03:05, Kuninori Morimoto wrote:
> +	priv->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	ret = devm_request_irq(dev, irq, msiof_interrupt, 0, dev_name(dev), priv);
> +	if (ret)
> +		return ret;
> +
> +	priv->dev	= dev;
> +	priv->phy_addr	= res->start;
> +
> +	spin_lock_init(&priv->lock);
> +	platform_set_drvdata(pdev, priv);
> +
> +	devm_pm_runtime_enable(dev);
> +
> +	ret = devm_snd_soc_register_component(dev, &msiof_component_driver,
> +					      &msiof_dai_driver, 1);
> +	if (ret)
> +		return ret;
> +
> +	dev_info(dev, "probed\n");

Drop. Driver should be silent on success and simple success messages are
useless. Core already gives you information that probe succeeded.

> +
> +	return ret;
> +}
> +
> +static const struct of_device_id msiof_of_match[] = {
> +	{ .compatible = "renesas,rcar-gen4-msiof", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, msiof_of_match);
> +
> +static struct platform_driver msiof_driver = {
> +	.driver 	= {
> +		.name	= "msiof-pcm-audio",
> +		.of_match_table = msiof_of_match,
> +	},
> +	.probe		= msiof_probe,
> +};
> +module_platform_driver(msiof_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Renesas R-Car MSIOF I2S audio driver");
> +MODULE_AUTHOR("Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>");
> +MODULE_ALIAS("platform:msiof-pcm-audio");

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.




Best regards,
Krzysztof

Re: [PATCH 5/7] ASoC: renesas: add MSIOF sound Documentation

[Geert Uytterhoeven]

Hi Krzysztof,

On Wed, 9 Apr 2025 at 08:37, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 09/04/2025 03:05, Kuninori Morimoto wrote:
> > Renesas MSIOF (Clock-Synchronized Serial Interface with FIFO) can work as
> > both SPI and I2S. MSIOF-I2S will use Audio Graph Card/Card2 driver which
> > uses Of-Graph in DT.
>
> > MSIOF-SPI/I2S are using same DT compatible properties.
> > MSIOF-I2S         uses Of-Graph for Audio-Graph-Card/Card2,
> > MSIOF-SPI doesn't use  Of-Graph.
> >
> > Adds MSIOF-I2S documentation for Sound.
> >
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > ---
> >  .../bindings/sound/renesas,msiof.yaml         | 112 ++++++++++++++++++
> >  1 file changed, 112 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/sound/renesas,msiof.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/renesas,msiof.yaml b/Documentation/devicetree/bindings/sound/renesas,msiof.yaml
> > new file mode 100644
> > index 000000000000..5173e80698fb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/renesas,msiof.yaml
> > @@ -0,0 +1,112 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/renesas,msiof.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Renesas MSIOF I2S controller
> > +
> > +maintainers:
> > +  - Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > +
> > +# sharing with MSIOF SPI
> > +# see
> > +# ${LINUX}/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml

http://devicetree.org/schemas/spi/renesas,sh-msiof.yaml

> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        pattern: "renesas,.*-msiof"
> > +  required:
> > +    - compatible
> > +    - port
>
> Drop entire select.

This is needed to avoid matching when using the device in SPI mode.

> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: renesas,msiof-r8a779g0   # R-Car V4H
>
> Use expected format of all soc compatibles. It has been always: SoC-module.

This is a pre-existing compatible value, so it cannot be changed.

> > +      - const: renesas,rcar-gen4-msiof  # generic R-Car Gen4
>
> If you have duplicated compatibles then:
> 1. It rarely makes sense because you claim that two different devices
> are using the same compatible. Different device, different compatible.
> 2. Or if this is really same device, then only one schema.

This the same device, but it can be used in two (actually more)
different modes: SPI and I2S.  Hence it has two separate DT binding
documents.  If this needs to be merged (the result is gonna be ugly):
where to fit it in the DT binding doc hierarchy?

> > +  reg:
> > +    minItems: 1
> > +    maxItems: 2
>
> Drop these two.
>
> > +    oneOf:
>
> Why is this flexible?

I am not sure where this is coming from (an old SH part?).
The SPI bindings have the same construct.  As this binding supports
R-Car Gen4 only, a single reg should be fine.

>
> > +      - items:
> > +          - description: CPU and DMA engine registers
> > +      - items:
> > +          - description: CPU registers
> > +          - description: DMA engine registers

> > +  dmas:
> > +    minItems: 2
> > +    maxItems: 4
>
> Why flexible?
>
> > +
> > +  dma-names:
> > +    minItems: 2
> > +    maxItems: 4
> > +    items:
> > +      enum: [ tx, rx ]
>
> How would that work? tx rx tx rx? And then driver requests 'tx' (by
> name) and what is supposed to be returned?

The module may be connected to one or more DMA controllers (see below).

> > +
> > +    msiof1: serial@e6ea0000 {
>
> serial means UART controller. You need name matching the class of the
> device.
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

What is the recommend generic node name for a flexible serial device
that can operate as (a.o.) either SPI or I2S controller?

> > +      compatible = "renesas,msiof-r8a779g0",
> > +                   "renesas,rcar-gen4-msiof";
> > +      reg = <0 0xe6ea0000 0 0x0064>;
> > +      interrupts = <GIC_SPI 240 IRQ_TYPE_LEVEL_HIGH>;
> > +      clocks = <&cpg CPG_MOD 619>;
> > +      dmas = <&dmac0 0x43>, <&dmac0 0x42>,
> > +             <&dmac1 0x43>, <&dmac1 0x42>;
> > +      dma-names = "tx", "rx", "tx", "rx";
>
> So test it now - get DMA by name 'tx'. What do you get?

A handle to either <&dmac0 0x43> or <&dmac1 0x43>; which one is
random. It's been working like that for ages.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/7] spi: sh-msiof: ignore driver probing if it was MSIOF Sound

[Geert Uytterhoeven]

Hi Morimoto-san,

On Wed, 9 Apr 2025 at 03:05, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> Renesas MSIOF (Clock-Synchronized Serial Interface with FIFO) can work as
> both SPI and I2S. MSIOF-I2S will use Audio Graph Card/Card2 driver which
> Of-Graph in DT.
>
> MSIOF-SPI/I2S are using same DT compatible properties.
> MSIOF-I2S         uses Of-Graph for Audio-Graph-Card/Card2,
> MSIOF-SPI doesn't use  Of-Graph.
>
> Check "port" node when driver probing
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Still, some comment below

> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
> @@ -20,6 +20,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_graph.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/sh_dma.h>
> @@ -1276,10 +1277,19 @@ static int sh_msiof_spi_probe(struct platform_device *pdev)
>         const struct sh_msiof_chipdata *chipdata;
>         struct sh_msiof_spi_info *info;
>         struct sh_msiof_spi_priv *p;
> +       struct device_node *port;

If you would add "__free(device_node)", you could drop the of_node_put()
below.

>         unsigned long clksrc;
>         int i;
>         int ret;
>
> +       /* Check whether MSIOF is used as I2S mode or SPI mode by checking "port" node */
> +       port = of_graph_get_next_port(pdev->dev.of_node, NULL);

This is actually checking for both "ports" and "port".  If you know the
subnode is called "port", you could simplify to of_get_child_by_name().

> +       if (port) {
> +               /* It was MSIOF-I2S */
> +               of_node_put(port);
> +               return -ENODEV;
> +       }
> +
>         chipdata = of_device_get_match_data(&pdev->dev);
>         if (chipdata) {
>                 info = sh_msiof_spi_parse_dt(&pdev->dev);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 6/7] ASoC: renesas: add MSIOF sound support

[Geert Uytterhoeven]

Hi Morimoto-san,

On Wed, 9 Apr 2025 at 03:05, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> Renesas MSIOF (Clock-Synchronized Serial Interface with FIFO) can work as
> both SPI and I2S. Adds MSIOF-I2S driver.
>
> MSIOF-SPI/I2S are using same DT compatible properties.
> MSIOF-I2S         uses Of-Graph for Audio-Graph-Card/Card2,
> MSIOF-SPI doesn't use  Of-Graph.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Thanks for your patch!

> --- a/sound/soc/renesas/Kconfig
> +++ b/sound/soc/renesas/Kconfig
> @@ -46,6 +46,13 @@ config SND_SOC_RCAR
>         help
>           This option enables R-Car SRU/SCU/SSIU/SSI sound support
>
> +config SND_SOC_MSIOF
> +       tristate "R-Car series MSIOF support"
> +       depends on OF

depends on ARCH_RENESAS || COMPILE_TEST

> +       select SND_DMAENGINE_PCM
> +       help
> +         This option enables R-Car MSIOF sound support
> +
>  config SND_SOC_RZ
>         tristate "RZ/G2L series SSIF-2 support"
>         depends on ARCH_RZG2L || COMPILE_TEST
> diff --git a/sound/soc/renesas/rcar/Makefile b/sound/soc/renesas/rcar/Makefile
> index 45eb875a912a..3a2c875595bd 100644
> --- a/sound/soc/renesas/rcar/Makefile
> +++ b/sound/soc/renesas/rcar/Makefile
> @@ -1,3 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  snd-soc-rcar-y         := core.o gen.o dma.o adg.o ssi.o ssiu.o src.o ctu.o mix.o dvc.o cmd.o debugfs.o
>  obj-$(CONFIG_SND_SOC_RCAR)     += snd-soc-rcar.o
> +
> +snd-soc-msiof-y                        := msiof.o
> +obj-$(CONFIG_SND_SOC_MSIOF)    += snd-soc-msiof.o
> diff --git a/sound/soc/renesas/rcar/msiof.c b/sound/soc/renesas/rcar/msiof.c
> new file mode 100644
> index 000000000000..de8de3468402
> --- /dev/null
> +++ b/sound/soc/renesas/rcar/msiof.c
> @@ -0,0 +1,579 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Renesas R-Car MSIOF (Clock-Synchronized Serial Interface with FIFO) I2S driver
> +//
> +// Copyright (C) 2025 Renesas Solutions Corp.
> +// Author: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> +//
> +
> +/*
> + * [NOTE]
> + *
> + * This driver doesn't support Clock/Frame Provider Mode
> + *
> + * Basically MSIOF is created for SPI, but we can use it as I2S (Sound). Because of it, when we use
> + * it as I2S (Sound) with Provider Mode, we need to send dummy TX data even though it is used for
> + * RX. Because SPI HW needs TX Clock/Frame output for RX purpose also. It makes driver code complex.
> + *
> + * And when we use MSIOF (Sound) as Provider Mode, the clock source is [MSO clock] (= 133.33MHz)
> + * SoC internal clock. It is not for 48kHz/44.1kHz base clock. Thus the output/input will not be
> + * accurate sound.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_dma.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <sound/dmaengine_pcm.h>
> +#include <sound/soc.h>
> +
> +/* register */
> +#define SITMDR1                0x00
> +#define SITMDR2                0x04
> +#define SITMDR3                0x08
> +#define SIRMDR1                0x10
> +#define SIRMDR2                0x14
> +#define SIRMDR3                0x18
> +#define SITSCR         0x20
> +#define SICTR          0x28
> +#define SIFCTR         0x30
> +#define SISTR          0x40
> +#define SIIER          0x44
> +#define SITFDR         0x50
> +#define SIRFDR         0x60

[...]

Perhaps the register definitions can be shared with spi-sh-msiof.c,
by extracting them into a shared header file?

Note that I have already converted drivers/spi/spi-sh-msiof.c locally
to use FIELD_PREP() (which requires changes to some macros), so we
may want to implement the sharing later.

> +static int msiof_hw_start(struct snd_soc_component *component,
> +                         struct snd_pcm_substream *substream, int cmd)
> +{
> +       struct msiof_priv *priv = snd_soc_component_get_drvdata(component);
> +       struct snd_pcm_runtime *runtime = substream->runtime;
> +       int is_play = msiof_is_play(substream);
> +       int width = snd_pcm_format_width(runtime->format);
> +       u32 val;
> +
> +       /*
> +        * see
> +        *      Datasheet 109.3.6 [Transmit and Receive Procedures]
> +        *
> +        *      TX: Fig 109.14  - Fig 109.23
> +        *      RX: Fig 109.15
> +        */
> +
> +       /* reset errors */
> +       priv->err_syc[substream->stream] =
> +       priv->err_ovf[substream->stream] =
> +       priv->err_udf[substream->stream] = 0;
> +
> +       /* SITMDRx */
> +       if (is_play) {
> +               val = PCON | SYNCMD_LR | SYNCAC | TXSTP;
> +               if (msiof_flag_has(priv, MSIOF_FLAGS_NEED_DELAY))
> +                       val |= DTDL_1;
> +
> +               msiof_write(priv, SITMDR1, val);
> +
> +               val = BITLEN1(width);
> +               msiof_write(priv, SITMDR2, val | GRP);
> +               msiof_write(priv, SITMDR3, val);
> +

Don't you have to initialize SITMDR[123] unconditionally, as reception
requires transmitting dummy data on R-Car (cfr. SPI_CONTROLLER_MUST_TX)?

> +       }
> +       /* SIRMDRx */
> +       else {
> +               val = SYNCMD_LR | SYNCAC;
> +               if (msiof_flag_has(priv, MSIOF_FLAGS_NEED_DELAY))
> +                       val |= DTDL_1;
> +
> +               msiof_write(priv, SIRMDR1, val);
> +
> +               val = BITLEN1(width);
> +               msiof_write(priv, SIRMDR2, val | GRP);
> +               msiof_write(priv, SIRMDR3, val);
> +       }
> +
> +       /* SIIER */
> +       if (is_play)
> +               val = TDREQE | TDMAE | SISTR_ERR_TX;
> +       else
> +               val = RDREQE | RDMAE | SISTR_ERR_RX;

Likewise for transmit control flags.

> +       msiof_update(priv, SIIER, val, val);
> +
> +       /* SICTR */
> +       if (is_play)
> +               val = TXE | TEDG;
> +       else
> +               val = RXE | REDG;

Likewise,

> +       msiof_update_and_wait(priv, SICTR, val, val, val);
> +
> +       msiof_status_clear(priv);
> +
> +       /* Start DMAC */
> +       snd_dmaengine_pcm_trigger(substream, cmd);
> +
> +       return 0;
> +}
> +
> +static int msiof_hw_stop(struct snd_soc_component *component,
> +                        struct snd_pcm_substream *substream, int cmd)
> +{
> +       struct msiof_priv *priv = snd_soc_component_get_drvdata(component);
> +       struct device *dev = component->dev;
> +       int is_play = msiof_is_play(substream);
> +       u32 val;
> +
> +       /* SIIER */
> +       if (is_play)
> +               val = TDREQE | TDMAE | SISTR_ERR_TX;
> +       else
> +               val = RDREQE | RDMAE | SISTR_ERR_RX;

Likewise.

> +       msiof_update(priv, SIIER, val, 0);
> +
> +       /* Stop DMAC */
> +       snd_dmaengine_pcm_trigger(substream, cmd);
> +
> +       /* SICTR */
> +       if (is_play)
> +               val = TXE;
> +       else
> +               val = RXE;

Likewise.

> +       msiof_update_and_wait(priv, SICTR, val, 0, 0);
> +
> +       /* indicate error status if exist */
> +       if (priv->err_syc[substream->stream] ||
> +           priv->err_ovf[substream->stream] ||
> +           priv->err_udf[substream->stream])
> +               dev_warn(dev, "FSERR(%s) = %d, FOVF = %d, FUDF = %d\n",
> +                        snd_pcm_direction_name(substream->stream),
> +                        priv->err_syc[substream->stream],
> +                        priv->err_ovf[substream->stream],
> +                        priv->err_udf[substream->stream]);
> +
> +       return 0;
> +}

> +static int msiof_probe(struct platform_device *pdev)
> +{
> +       struct msiof_priv *priv;
> +       struct device *dev = &pdev->dev;
> +       struct resource *res;
> +       struct device_node *port;
> +       int irq, ret;
> +
> +       /* Check MSIOF as Sound mode or SPI mode */
> +       port = of_graph_get_next_port(dev->of_node, NULL);
> +       if (!port)
> +               return -ENODEV;
> +       of_node_put(port);

Just wondering: don't you need to use port? Or is that handled
elsewhere, in common sound code?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 5/7] ASoC: renesas: add MSIOF sound Documentation

[Krzysztof Kozlowski]

On Wed, Apr 09, 2025 at 08:37:03AM GMT, Krzysztof Kozlowski wrote:
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: renesas,msiof-r8a779g0   # R-Car V4H
> 
> 
> Use expected format of all soc compatibles. It has been always: SoC-module.

... unless this is an existing compatible, but then it should be in one
schema, not two.

Best regards,
Krzysztof

Re: [PATCH 5/7] ASoC: renesas: add MSIOF sound Documentation

[Krzysztof Kozlowski]

On Wed, Apr 09, 2025 at 09:01:22AM GMT, Geert Uytterhoeven wrote:
> > > +select:
> > > +  properties:
> > > +    compatible:
> > > +      contains:
> > > +        pattern: "renesas,.*-msiof"
> > > +  required:
> > > +    - compatible
> > > +    - port
> >
> > Drop entire select.
> 
> This is needed to avoid matching when using the device in SPI mode.

Which you need to avoid, so drop the select. One device, one schema.

> 
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - const: renesas,msiof-r8a779g0   # R-Car V4H
> >
> > Use expected format of all soc compatibles. It has been always: SoC-module.
> 
> This is a pre-existing compatible value, so it cannot be changed.
> 
> > > +      - const: renesas,rcar-gen4-msiof  # generic R-Car Gen4
> >
> > If you have duplicated compatibles then:
> > 1. It rarely makes sense because you claim that two different devices
> > are using the same compatible. Different device, different compatible.
> > 2. Or if this is really same device, then only one schema.
> 
> This the same device, but it can be used in two (actually more)
> different modes: SPI and I2S.  Hence it has two separate DT binding
> documents.  If this needs to be merged (the result is gonna be ugly):

... then next time don't post incomplete bindings. I know we do not have
time machine, but any mess is on contributors who posted some limited
scope/view of the hardware entirely ignoring the rest of interfaces.

> where to fit it in the DT binding doc hierarchy?

Does not matter, whatever fits better in overal picture/purpose of this
device.

> 
> > > +  reg:
> > > +    minItems: 1
> > > +    maxItems: 2
> >
> > Drop these two.
> >
> > > +    oneOf:
> >
> > Why is this flexible?
> 
> I am not sure where this is coming from (an old SH part?).
> The SPI bindings have the same construct.  As this binding supports
> R-Car Gen4 only, a single reg should be fine.
> 
> >
> > > +      - items:
> > > +          - description: CPU and DMA engine registers
> > > +      - items:
> > > +          - description: CPU registers
> > > +          - description: DMA engine registers
> 
> > > +  dmas:
> > > +    minItems: 2
> > > +    maxItems: 4
> >
> > Why flexible?
> >
> > > +
> > > +  dma-names:
> > > +    minItems: 2
> > > +    maxItems: 4
> > > +    items:
> > > +      enum: [ tx, rx ]
> >
> > How would that work? tx rx tx rx? And then driver requests 'tx' (by
> > name) and what is supposed to be returned?
> 
> The module may be connected to one or more DMA controllers (see below).

Yes, but how the implementation would work?

Anyway, this needs to be strictly ordered, not random rx rx tx tx or rx
rx rx rx.

> 
> > > +
> > > +    msiof1: serial@e6ea0000 {
> >
> > serial means UART controller. You need name matching the class of the
> > device.
> > Node names should be generic. See also an explanation and list of
> > examples (not exhaustive) in DT specification:
> > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> What is the recommend generic node name for a flexible serial device
> that can operate as (a.o.) either SPI or I2S controller?

i2s
or even not so generic msiof, but definitely not serial because that is
reserved for UART.

> 
> > > +      compatible = "renesas,msiof-r8a779g0",
> > > +                   "renesas,rcar-gen4-msiof";
> > > +      reg = <0 0xe6ea0000 0 0x0064>;
> > > +      interrupts = <GIC_SPI 240 IRQ_TYPE_LEVEL_HIGH>;
> > > +      clocks = <&cpg CPG_MOD 619>;
> > > +      dmas = <&dmac0 0x43>, <&dmac0 0x42>,
> > > +             <&dmac1 0x43>, <&dmac1 0x42>;
> > > +      dma-names = "tx", "rx", "tx", "rx";
> >
> > So test it now - get DMA by name 'tx'. What do you get?
> 
> A handle to either <&dmac0 0x43> or <&dmac1 0x43>; which one is
> random. It's been working like that for ages.

Interesting. And is this expected behavior? Driver does not care which
RX and which TX it gets? Like RX from dmac0 and TX from dmac1?

Best regards,
Krzysztof

Re: [PATCH 5/7] ASoC: renesas: add MSIOF sound Documentation

[Geert Uytterhoeven]

Hi Krzysztof,

On Wed, 9 Apr 2025 at 09:52, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Wed, Apr 09, 2025 at 09:01:22AM GMT, Geert Uytterhoeven wrote:
> > > > +select:
> > > > +  properties:
> > > > +    compatible:
> > > > +      contains:
> > > > +        pattern: "renesas,.*-msiof"
> > > > +  required:
> > > > +    - compatible
> > > > +    - port
> > >
> > > Drop entire select.
> >
> > This is needed to avoid matching when using the device in SPI mode.
>
> Which you need to avoid, so drop the select. One device, one schema.

OK... (to be read as "dot dot dot', really! ;-)

> > > > +properties:
> > > > +  compatible:
> > > > +    items:
> > > > +      - const: renesas,msiof-r8a779g0   # R-Car V4H
> > >
> > > Use expected format of all soc compatibles. It has been always: SoC-module.
> >
> > This is a pre-existing compatible value, so it cannot be changed.
> >
> > > > +      - const: renesas,rcar-gen4-msiof  # generic R-Car Gen4
> > >
> > > If you have duplicated compatibles then:
> > > 1. It rarely makes sense because you claim that two different devices
> > > are using the same compatible. Different device, different compatible.
> > > 2. Or if this is really same device, then only one schema.
> >
> > This the same device, but it can be used in two (actually more)
> > different modes: SPI and I2S.  Hence it has two separate DT binding
> > documents.  If this needs to be merged (the result is gonna be ugly):
>
> ... then next time don't post incomplete bindings. I know we do not have

:-)

> time machine, but any mess is on contributors who posted some limited
> scope/view of the hardware entirely ignoring the rest of interfaces.

This is the first time someone implemented I2S using MSIOF on a system
intended to run Linux.  Note that MSIOF is not even limited to SPI and
I2S.  It can be used as a generic synchronous serial interface, too. So
far no one did under Linux, so it is not reflected yet in the bindings.
MSIOF is also used to provide a clock signal to a PMIC on some older
R-Car boards.  As that PMIC has no upstream Linux driver, no one ever
implemented support for this mode in Linux.  So I guess I should be
pro-active, and add #clock-cells to the unified MSIOF DT bindings, too?

Note that there are other devices to consider, too. E.g. SCIF can
not only be used as a UART, but also as a USART, SPI, or even I2C
controller... (currently Linux with DT supports the UART personality only,
but drivers/spi/spi-sh-sci.c does exist for SH).

> > where to fit it in the DT binding doc hierarchy?
>
> Does not matter, whatever fits better in overal picture/purpose of this
> device.

OK, hence the existing SPI bindings....

> > > > +  dmas:
> > > > +    minItems: 2
> > > > +    maxItems: 4
> > >
> > > Why flexible?
> > >
> > > > +
> > > > +  dma-names:
> > > > +    minItems: 2
> > > > +    maxItems: 4
> > > > +    items:
> > > > +      enum: [ tx, rx ]
> > >
> > > How would that work? tx rx tx rx? And then driver requests 'tx' (by
> > > name) and what is supposed to be returned?
> >
> > The module may be connected to one or more DMA controllers (see below).
>
> Yes, but how the implementation would work?
>
> Anyway, this needs to be strictly ordered, not random rx rx tx tx or rx
> rx rx rx.

Why?

> > > > +
> > > > +    msiof1: serial@e6ea0000 {
> > >
> > > serial means UART controller. You need name matching the class of the
> > > device.
> > > Node names should be generic. See also an explanation and list of
> > > examples (not exhaustive) in DT specification:
> > > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> >
> > What is the recommend generic node name for a flexible serial device
> > that can operate as (a.o.) either SPI or I2S controller?
>
> i2s
> or even not so generic msiof, but definitely not serial because that is
> reserved for UART.

The MSIOF device node lives in the SoC-specific .dtsi file.  Its use
case is not known in that file, and specified only in the board
.dts file.

> > > > +      compatible = "renesas,msiof-r8a779g0",
> > > > +                   "renesas,rcar-gen4-msiof";
> > > > +      reg = <0 0xe6ea0000 0 0x0064>;
> > > > +      interrupts = <GIC_SPI 240 IRQ_TYPE_LEVEL_HIGH>;
> > > > +      clocks = <&cpg CPG_MOD 619>;
> > > > +      dmas = <&dmac0 0x43>, <&dmac0 0x42>,
> > > > +             <&dmac1 0x43>, <&dmac1 0x42>;
> > > > +      dma-names = "tx", "rx", "tx", "rx";
> > >
> > > So test it now - get DMA by name 'tx'. What do you get?
> >
> > A handle to either <&dmac0 0x43> or <&dmac1 0x43>; which one is
> > random. It's been working like that for ages.
>
> Interesting. And is this expected behavior? Driver does not care which
> RX and which TX it gets? Like RX from dmac0 and TX from dmac1?

Exactly.
This use case was one of the requirements when DMA support was DTified.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 5/7] ASoC: renesas: add MSIOF sound Documentation

[Krzysztof Kozlowski]

On 09/04/2025 10:09, Geert Uytterhoeven wrote:
>>>>
>>>> If you have duplicated compatibles then:
>>>> 1. It rarely makes sense because you claim that two different devices
>>>> are using the same compatible. Different device, different compatible.
>>>> 2. Or if this is really same device, then only one schema.
>>>
>>> This the same device, but it can be used in two (actually more)
>>> different modes: SPI and I2S.  Hence it has two separate DT binding
>>> documents.  If this needs to be merged (the result is gonna be ugly):
>>
>> ... then next time don't post incomplete bindings. I know we do not have
> 
> :-)
> 
>> time machine, but any mess is on contributors who posted some limited
>> scope/view of the hardware entirely ignoring the rest of interfaces.
> 
> This is the first time someone implemented I2S using MSIOF on a system
> intended to run Linux.  Note that MSIOF is not even limited to SPI and
> I2S.  It can be used as a generic synchronous serial interface, too. So

So like a serial engine for UART/I2C/SPI? I think all or most of new
SoCs since few years switched to these.

> far no one did under Linux, so it is not reflected yet in the bindings.
> MSIOF is also used to provide a clock signal to a PMIC on some older
> R-Car boards.  As that PMIC has no upstream Linux driver, no one ever
> implemented support for this mode in Linux.  So I guess I should be
> pro-active, and add #clock-cells to the unified MSIOF DT bindings, too?

Yes, probably. Although missing cells is easy to change but missing
protocol, like this patchset here, is quite more challenging.


> 
> Note that there are other devices to consider, too. E.g. SCIF can
> not only be used as a UART, but also as a USART, SPI, or even I2C
> controller... (currently Linux with DT supports the UART personality only,
> but drivers/spi/spi-sh-sci.c does exist for SH).

Just like all serial engines for all other SoCs and there are no
problems with them... Why is this somehow different?

> 
>>> where to fit it in the DT binding doc hierarchy?
>>
>> Does not matter, whatever fits better in overal picture/purpose of this
>> device.
> 
> OK, hence the existing SPI bindings....
> 
>>>>> +  dmas:
>>>>> +    minItems: 2
>>>>> +    maxItems: 4
>>>>
>>>> Why flexible?
>>>>
>>>>> +
>>>>> +  dma-names:
>>>>> +    minItems: 2
>>>>> +    maxItems: 4
>>>>> +    items:
>>>>> +      enum: [ tx, rx ]
>>>>
>>>> How would that work? tx rx tx rx? And then driver requests 'tx' (by
>>>> name) and what is supposed to be returned?
>>>
>>> The module may be connected to one or more DMA controllers (see below).
>>
>> Yes, but how the implementation would work?
>>
>> Anyway, this needs to be strictly ordered, not random rx rx tx tx or rx
>> rx rx rx.
> 
> Why?

Because that's the standard DT rule, so unless, you come with a need to
bypass the rule, standard applies. Why? Because implementations can use
one of two ABIs - name or index - and binding should allow it. The names
are for cases where entries are optional in the middle, so you cannot
use index. Only for that. You cannot use that exception and make a
standard case "now I want flexibility everywhere". No. Flexibility is
only for special cases.

> 
>>>>> +
>>>>> +    msiof1: serial@e6ea0000 {
>>>>
>>>> serial means UART controller. You need name matching the class of the
>>>> device.
>>>> Node names should be generic. See also an explanation and list of
>>>> examples (not exhaustive) in DT specification:
>>>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>>>
>>> What is the recommend generic node name for a flexible serial device
>>> that can operate as (a.o.) either SPI or I2S controller?
>>
>> i2s
>> or even not so generic msiof, but definitely not serial because that is
>> reserved for UART.
> 
> The MSIOF device node lives in the SoC-specific .dtsi file.  Its use
> case is not known in that file, and specified only in the board
> .dts file.

sure, so call it serial-engine. or msiof.

Not serial. Why? well, I said twice - it is reserved by dtschema for serial.


Best regards,
Krzysztof

Re: [PATCH 1/7] spi: renesas,sh-msiof: Living separately from MSIOF I2S Sound

[Kuninori Morimoto]

Hi

> > Renesas MSIOF (Clock-Synchronized Serial Interface with FIFO) can work as
> > both SPI and I2S. MSIOF-I2S will use Audio Graph Card/Card2 driver which
> > uses Of-Graph in DT.
> > 
> > MSIOF-SPI/I2S are using same DT compatible properties.
> > MSIOF-I2S         uses Of-Graph for Audio-Graph-Card/Card2,
> > MSIOF-SPI doesn't use  Of-Graph.
> > 
> > Ignore MSIOF-I2S case (= Of-Graph) in MSIOF-SPI Doc.
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.

Yeah, I did :)

git log --oneline --no-merges Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml 

1f48cbd6f00f spi: renesas,sh-msiof: Add r8a779h0 support
a0dcd1ff9629 spi: renesas,sh-msiof: Miscellaneous improvements
aa69dc65e3b3 spi: renesas,sh-msiof: Add r8a779g0 support
f4d381038700 spi: renesas,sh-msiof: Fix 'unevaluatedProperties' warnings
b076fdd02133 spi: renesas,sh-msiof: R-Car V3U is R-Car Gen4
e1e62f05d5d9 spi: renesas,sh-msiof: Add generic Gen4 and r8a779f0 support
5a674d9dc9a0 dt-bindings: Fix array constraints on scalar properties
6be69293196c spi: renesas,sh-msiof: Add r8a779a0 support
6fdc6e23a7d1 dt-bindings: Add missing 'unevaluatedProperties'
aef161f4f1b8 spi: renesas,sh-msiof: Add r8a77961 support
b4f7f5f54705 spi: renesas,sh-msiof: Add r8a774e1 support
6383b118efaf spi: renesas,sh-msiof: Add r8a7742 support
fba5618451d2 dt-bindings: Fix incorrect 'reg' property sizes
3d21a4609335 dt-bindings: Remove cases of 'allOf' containing a '$ref'
97f41c68b83e dt-bindings: spi: sh-msiof: Add r8a774b1 support
9c3c41761f45 dt-bindings: spi: sh-msiof: Convert bindings to json-schema

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 6/7] ASoC: renesas: add MSIOF sound support

[Kuninori Morimoto]

Hi Krzysztof, Mark, Iwai-san

> > +	dev_info(dev, "probed\n");
> 
> Drop. Driver should be silent on success and simple success messages are
> useless. Core already gives you information that probe succeeded.

What does "Core" mean here ??

> > +MODULE_ALIAS("platform:msiof-pcm-audio");
> 
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong (e.g. misses either
> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
> for incomplete ID table.

Iwai-san, Mark, do you have any comment about this ?
Almost all ALSA drivers are using it

	> git grep MODULE_ALIAS sound | wc -l
	249

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 2/7] spi: sh-msiof: ignore driver probing if it was MSIOF Sound

[Kuninori Morimoto]

Hi Geert

Thank you for your review

> > Renesas MSIOF (Clock-Synchronized Serial Interface with FIFO) can work as
> > both SPI and I2S. MSIOF-I2S will use Audio Graph Card/Card2 driver which
> > Of-Graph in DT.
> >
> > MSIOF-SPI/I2S are using same DT compatible properties.
> > MSIOF-I2S         uses Of-Graph for Audio-Graph-Card/Card2,
> > MSIOF-SPI doesn't use  Of-Graph.
> >
> > Check "port" node when driver probing
> >
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks

> > @@ -1276,10 +1277,19 @@ static int sh_msiof_spi_probe(struct platform_device *pdev)
> >         const struct sh_msiof_chipdata *chipdata;
> >         struct sh_msiof_spi_info *info;
> >         struct sh_msiof_spi_priv *p;
> > +       struct device_node *port;
> 
> If you would add "__free(device_node)", you could drop the of_node_put()
> below.

Yes, indeed. will use it in v2

> > +       /* Check whether MSIOF is used as I2S mode or SPI mode by checking "port" node */
> > +       port = of_graph_get_next_port(pdev->dev.of_node, NULL);
> 
> This is actually checking for both "ports" and "port".  If you know the
> subnode is called "port", you could simplify to of_get_child_by_name().

Current dt-bindings Doc is caring only "port" for now (because it will be
more complicated if care both...), but sound might/can use "ports".

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 6/7] ASoC: renesas: add MSIOF sound support

[Kuninori Morimoto]

Hi Geert

Thank you for your review

> > +config SND_SOC_MSIOF
> > +       tristate "R-Car series MSIOF support"
> > +       depends on OF
> 
> depends on ARCH_RENESAS || COMPILE_TEST

Ah, yes indeed. Will add in v2

> Perhaps the register definitions can be shared with spi-sh-msiof.c,
> by extracting them into a shared header file?
> 
> Note that I have already converted drivers/spi/spi-sh-msiof.c locally
> to use FIELD_PREP() (which requires changes to some macros), so we
> may want to implement the sharing later.

Yes 100% I can agree about this !
I'm happy to sharing it, but it will be next step

> > +       /* SITMDRx */
> > +       if (is_play) {
> > +               val = PCON | SYNCMD_LR | SYNCAC | TXSTP;
> > +               if (msiof_flag_has(priv, MSIOF_FLAGS_NEED_DELAY))
> > +                       val |= DTDL_1;
> > +
> > +               msiof_write(priv, SITMDR1, val);
> > +
> > +               val = BITLEN1(width);
> > +               msiof_write(priv, SITMDR2, val | GRP);
> > +               msiof_write(priv, SITMDR3, val);
> > +
> 
> Don't you have to initialize SITMDR[123] unconditionally, as reception
> requires transmitting dummy data on R-Car (cfr. SPI_CONTROLLER_MUST_TX)?

Good catch, but I added 1 restriction for MSIOF-I2S mode.
I have explained it on top of this driver. The restriction is
"MSIOF-I2S doesn't work as Clock/Frame Provider Mode".
So, dummy transmit for RX is not needed/assumed.
I think it is one of big-diff between MSIOF-SPI ?

/*
 * [NOTE]
 *
 * This driver doesn't support Clock/Frame Provider Mode
 *
 * Basically MSIOF is created for SPI, but we can use it as I2S (Sound). Because of it, when we use
 * it as I2S (Sound) with Provider Mode, we need to send dummy TX data even though it is used for
 * RX. Because SPI HW needs TX Clock/Frame output for RX purpose also. It makes driver code complex.
 *
 * And when we use MSIOF (Sound) as Provider Mode, the clock source is [MSO clock] (= 133.33MHz)
 * SoC internal clock. It is not for 48kHz/44.1kHz base clock. Thus the output/input will not be
 * accurate sound.
 */

> > +static int msiof_probe(struct platform_device *pdev)
> > +{
> > +       struct msiof_priv *priv;
> > +       struct device *dev = &pdev->dev;
> > +       struct resource *res;
> > +       struct device_node *port;
> > +       int irq, ret;
> > +
> > +       /* Check MSIOF as Sound mode or SPI mode */
> > +       port = of_graph_get_next_port(dev->of_node, NULL);
> > +       if (!port)
> > +               return -ENODEV;
> > +       of_node_put(port);
> 
> Just wondering: don't you need to use port? Or is that handled
> elsewhere, in common sound code?

"ports/port" will be handled by "Sound Card" driver
(= Audio Graph Card/Card2), not MSIOF driver.

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 5/7] ASoC: renesas: add MSIOF sound Documentation

[Kuninori Morimoto]

Hi Krzysztof

> Yes, probably. Although missing cells is easy to change but missing
> protocol, like this patchset here, is quite more challenging.

MSIOF-I2S is using MSIOF-SPI's property, these are compatible.
The diff is I2S is using Of-Graph, SPI is not. Not so challenging ?

> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - const: renesas,msiof-r8a779g0   # R-Car V4H
> > 
> > 
> > Use expected format of all soc compatibles. It has been always: SoC-module.
> 
> ... unless this is an existing compatible, but then it should be in one
> schema, not two.

If I merged MSIOF-I2S/SPI into one, Of-Graph part will be option ?

>> where to fit it in the DT binding doc hierarchy?
>
> Does not matter, whatever fits better in overal picture/purpose of this
> device.

Can I put info like
linux/Documentation/devicetree/bindings/sound/renesas,msiof.txt

--- renesas,msiof.txt ---
MSIOF-I2S and MSIOF-SPI are using same DT schema.
See
	linux/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml 
-------------------------

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 6/7] ASoC: renesas: add MSIOF sound support

[Kuninori Morimoto]

Hi Geert, again

> > > +config SND_SOC_MSIOF
> > > +       tristate "R-Car series MSIOF support"
> > > +       depends on OF
> > 
> > depends on ARCH_RENESAS || COMPILE_TEST
> 
> Ah, yes indeed. Will add in v2

Renesas category Sound drivers are under below menu.
So, it is not needed on each drivers.

menu "SoC Audio support for Renesas SoCs"
	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 6/7] ASoC: renesas: add MSIOF sound support

[Krzysztof Kozlowski]

On 10/04/2025 01:25, Kuninori Morimoto wrote:
> 
> Hi Krzysztof, Mark, Iwai-san
> 
>>> +	dev_info(dev, "probed\n");
>>
>> Drop. Driver should be silent on success and simple success messages are
>> useless. Core already gives you information that probe succeeded.
> 
> What does "Core" mean here ??

Core of Linux.

> 
>>> +MODULE_ALIAS("platform:msiof-pcm-audio");
>>
>> You should not need MODULE_ALIAS() in normal cases. If you need it,
>> usually it means your device ID table is wrong (e.g. misses either
>> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
>> for incomplete ID table.
> 
> Iwai-san, Mark, do you have any comment about this ?
> Almost all ALSA drivers are using it
> 
> 	> git grep MODULE_ALIAS sound | wc -l
> 	249


What do you need it for? You already have ID table.

Just because drivers need it, is not a justification that you need. If
other drivers have poor code, it's okay to do the same?


Best regards,
Krzysztof

Re: [PATCH 5/7] ASoC: renesas: add MSIOF sound Documentation

[Krzysztof Kozlowski]

On 10/04/2025 02:49, Kuninori Morimoto wrote:
> 
> Hi Krzysztof
> 
>> Yes, probably. Although missing cells is easy to change but missing
>> protocol, like this patchset here, is quite more challenging.
> 
> MSIOF-I2S is using MSIOF-SPI's property, these are compatible.
> The diff is I2S is using Of-Graph, SPI is not. Not so challenging ?

Not challenging? Then I don't see the point why we are discussing and
just implement the feedback.

> 
>>>> +properties:
>>>> +  compatible:
>>>> +    items:
>>>> +      - const: renesas,msiof-r8a779g0   # R-Car V4H
>>>
>>>
>>> Use expected format of all soc compatibles. It has been always: SoC-module.
>>
>> ... unless this is an existing compatible, but then it should be in one
>> schema, not two.
> 
> If I merged MSIOF-I2S/SPI into one, Of-Graph part will be option ?
> 
>>> where to fit it in the DT binding doc hierarchy?
>>
>> Does not matter, whatever fits better in overal picture/purpose of this
>> device.
> 
> Can I put info like
> linux/Documentation/devicetree/bindings/sound/renesas,msiof.txt

No. TXT bindings are not accepted. There is no benefit in this.

Best regards,
Krzysztof

Re: [PATCH 1/7] spi: renesas,sh-msiof: Living separately from MSIOF I2S Sound

[Krzysztof Kozlowski]

On 10/04/2025 01:19, Kuninori Morimoto wrote:
> 
> Hi
> 
>>> Renesas MSIOF (Clock-Synchronized Serial Interface with FIFO) can work as
>>> both SPI and I2S. MSIOF-I2S will use Audio Graph Card/Card2 driver which
>>> uses Of-Graph in DT.
>>>
>>> MSIOF-SPI/I2S are using same DT compatible properties.
>>> MSIOF-I2S         uses Of-Graph for Audio-Graph-Card/Card2,
>>> MSIOF-SPI doesn't use  Of-Graph.
>>>
>>> Ignore MSIOF-I2S case (= Of-Graph) in MSIOF-SPI Doc.
>>
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching.
> 
> Yeah, I did :)
> 
> git log --oneline --no-merges Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml 

So run it on the directory. Apparently many contributors decided to
ignore Mark's rule about naming and if people ignore Mark's rule, Mark
was removing dt-bindings prefix, thus you have what you have below:

> 
> 1f48cbd6f00f spi: renesas,sh-msiof: Add r8a779h0 support
> a0dcd1ff9629 spi: renesas,sh-msiof: Miscellaneous improvements
> aa69dc65e3b3 spi: renesas,sh-msiof: Add r8a779g0 support
> f4d381038700 spi: renesas,sh-msiof: Fix 'unevaluatedProperties' warnings
> b076fdd02133 spi: renesas,sh-msiof: R-Car V3U is R-Car Gen4
> e1e62f05d5d9 spi: renesas,sh-msiof: Add generic Gen4 and r8a779f0 support
> 5a674d9dc9a0 dt-bindings: Fix array constraints on scalar properties
> 6be69293196c spi: renesas,sh-msiof: Add r8a779a0 support
> 6fdc6e23a7d1 dt-bindings: Add missing 'unevaluatedProperties'
> aef161f4f1b8 spi: renesas,sh-msiof: Add r8a77961 support
> b4f7f5f54705 spi: renesas,sh-msiof: Add r8a774e1 support
> 6383b118efaf spi: renesas,sh-msiof: Add r8a7742 support

Prefixes are explained in the docs I linked to.

Best regards,
Krzysztof

Re: [PATCH 6/7] ASoC: renesas: add MSIOF sound support

[Kuninori Morimoto]

Hi Krzysztof, Mark, Takashi-san

> >>> +MODULE_ALIAS("platform:msiof-pcm-audio");
> >>
> >> You should not need MODULE_ALIAS() in normal cases. If you need it,
> >> usually it means your device ID table is wrong (e.g. misses either
> >> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
> >> for incomplete ID table.
> > 
> > Iwai-san, Mark, do you have any comment about this ?
> > Almost all ALSA drivers are using it
> > 
> > 	> git grep MODULE_ALIAS sound | wc -l
> > 	249
> 
> 
> What do you need it for? You already have ID table.
> 
> Just because drivers need it, is not a justification that you need. If
> other drivers have poor code, it's okay to do the same?

In my understanding, it is needed for userspace (and the macro is for it ?)
but I'm not familiar with userspace.
Mark, Takashi-san ?

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 6/7] ASoC: renesas: add MSIOF sound support

[Krzysztof Kozlowski]

On 10/04/2025 08:29, Kuninori Morimoto wrote:
> 
> Hi Krzysztof, Mark, Takashi-san
> 
>>>>> +MODULE_ALIAS("platform:msiof-pcm-audio");
>>>>
>>>> You should not need MODULE_ALIAS() in normal cases. If you need it,
>>>> usually it means your device ID table is wrong (e.g. misses either
>>>> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
>>>> for incomplete ID table.
>>>
>>> Iwai-san, Mark, do you have any comment about this ?
>>> Almost all ALSA drivers are using it
>>>
>>> 	> git grep MODULE_ALIAS sound | wc -l
>>> 	249
>>
>>
>> What do you need it for? You already have ID table.
>>
>> Just because drivers need it, is not a justification that you need. If
>> other drivers have poor code, it's okay to do the same?
> 
> In my understanding, it is needed for userspace (and the macro is for it ?)
> but I'm not familiar with userspace.

It creates alias and you already have the alias from OF ID table, so
which userspace depends on this alias? Using other drivers as an example
is not helping, because for several of them such alias is necessary
since they do not have ID table. But you have. So again - my comment
stays valid or please bring the explanation which component relies on
this alias and cannot be switched to aliases coming from OF or platform
tables.

I am kind of repeating here myself, but I gave you the answer that you
do not need it.

Best regards,
Krzysztof

Re: [PATCH 5/7] ASoC: renesas: add MSIOF sound Documentation

[Kuninori Morimoto]

Hi Krzysztof

> >>> where to fit it in the DT binding doc hierarchy?
> >>
> >> Does not matter, whatever fits better in overal picture/purpose of this
> >> device.
> > 
> > Can I put info like
> > linux/Documentation/devicetree/bindings/sound/renesas,msiof.txt
> 
> No. TXT bindings are not accepted. There is no benefit in this.

So, no MSIOF I2S info is allowed under
linux/Documentation/devicetree/bindings/sound/ ??

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 6/7] ASoC: renesas: add MSIOF sound support

[Geert Uytterhoeven]

Hi Morimoto-san,

On Thu, 10 Apr 2025 at 04:37, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> > > > +config SND_SOC_MSIOF
> > > > +       tristate "R-Car series MSIOF support"
> > > > +       depends on OF
> > >
> > > depends on ARCH_RENESAS || COMPILE_TEST
> >
> > Ah, yes indeed. Will add in v2
>
> Renesas category Sound drivers are under below menu.
> So, it is not needed on each drivers.
>
> menu "SoC Audio support for Renesas SoCs"
>         depends on SUPERH || ARCH_RENESAS || COMPILE_TEST

Thanks, I should have checked that...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 6/7] ASoC: renesas: add MSIOF sound support

[Geert Uytterhoeven]

Hi Morimoto-san,

On Thu, 10 Apr 2025 at 01:45, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> > > +       /* SITMDRx */
> > > +       if (is_play) {
> > > +               val = PCON | SYNCMD_LR | SYNCAC | TXSTP;
> > > +               if (msiof_flag_has(priv, MSIOF_FLAGS_NEED_DELAY))
> > > +                       val |= DTDL_1;
> > > +
> > > +               msiof_write(priv, SITMDR1, val);
> > > +
> > > +               val = BITLEN1(width);
> > > +               msiof_write(priv, SITMDR2, val | GRP);
> > > +               msiof_write(priv, SITMDR3, val);
> > > +
> >
> > Don't you have to initialize SITMDR[123] unconditionally, as reception
> > requires transmitting dummy data on R-Car (cfr. SPI_CONTROLLER_MUST_TX)?
>
> Good catch, but I added 1 restriction for MSIOF-I2S mode.
> I have explained it on top of this driver. The restriction is
> "MSIOF-I2S doesn't work as Clock/Frame Provider Mode".
> So, dummy transmit for RX is not needed/assumed.
> I think it is one of big-diff between MSIOF-SPI ?

IC.  Being just a mortal sound-noob, I didn't know what "Clock/Frame
Provider Mode" means ;-) Oh, now I understand. I had missed
completely that you are running MSIOF in slave mode. So everything
should be fine.

And

    /* SITSCR */
    #define SITSCR_V(p, d) ((p << 8) + d)

is unused, and can be removed.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 6/7] ASoC: renesas: add MSIOF sound support

[Kuninori Morimoto]

Hi Geert

> > > > > +config SND_SOC_MSIOF
> > > > > +       tristate "R-Car series MSIOF support"
> > > > > +       depends on OF
> > > >
> > > > depends on ARCH_RENESAS || COMPILE_TEST
> > >
> > > Ah, yes indeed. Will add in v2
> >
> > Renesas category Sound drivers are under below menu.
> > So, it is not needed on each drivers.
> >
> > menu "SoC Audio support for Renesas SoCs"
> >         depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
> 
> Thanks, I should have checked that...

No problem.
Thank you for your review anyway

Best regards
---
Kuninori Morimoto

Re: [PATCH 6/7] ASoC: renesas: add MSIOF sound support

[Kuninori Morimoto]

Hi Geert

> > > Don't you have to initialize SITMDR[123] unconditionally, as reception
> > > requires transmitting dummy data on R-Car (cfr. SPI_CONTROLLER_MUST_TX)?
> >
> > Good catch, but I added 1 restriction for MSIOF-I2S mode.
> > I have explained it on top of this driver. The restriction is
> > "MSIOF-I2S doesn't work as Clock/Frame Provider Mode".
> > So, dummy transmit for RX is not needed/assumed.
> > I think it is one of big-diff between MSIOF-SPI ?
> 
> IC.  Being just a mortal sound-noob, I didn't know what "Clock/Frame
> Provider Mode" means ;-) Oh, now I understand. I had missed
> completely that you are running MSIOF in slave mode. So everything
> should be fine.

ASoC is now using Provider/Consumer instead of Master/Slave.
Veteran engineers are not familiar with it :)

>     /* SITSCR */
>     #define SITSCR_V(p, d) ((p << 8) + d)
> 
> is unused, and can be removed.

OK, Thanks.
Will remove in v2

Thank you for your help !!

Best regards
---
Kuninori Morimoto