[PATCH 00/13] ASoC: ti: davinci-i2s: Add features to McBSP driver

[PATCH 00/13] ASoC: ti: davinci-i2s: Add features to McBSP driver

[Bastien Curutchet]

This series aims to add some features to McBSP driver.

Convert bindings from .txt to .yaml.
Add possibility to use an external clock as sample rate generator's
input.
Add handling of new formats (TDM, S24_LE, BP_FC).
Add optional properties in DT:
 - ti,enable-sync-err  : Enable the detection of unexpected frame pulses
 - ti,disable-free-run : Disable the free-running mode where McBSP drives
                         serial clocks during emulation halt
 - ti,drive-dx 	       : Outputs a chosen pattern on DX pin during
                         capture streams.

This has been tested on a platform designed off of the DAVINCI/OMAP-L138
connected to 3 daisy-chained AD7767. An external clock drives the
sample rate generator through the CLKS pin.
The hardware I have only allowed me to test acquisition side of McBSP.
It is connected to a 6 channels TDM and acts as Bit clock provider and
Frame clock consumer.

Bastien Curutchet (13):
  ASoC: dt-bindings: davinci-mcbsp: convert McBSP bindings to yaml
    schema
  ASoC: dt-bindings: davinci-mcbsp: Add new properties
  ASoC: ti: davinci-i2s: Remove the unused clk_input_pin attribute
  ASoC: ti: davinci-i2s: Replace dev_err with dev_err_probe
  ASoC: ti: davinci-i2s: Use external clock to drive sample rate
    generator
  ASoC: ti: davinci-i2s: Delete unnecessary assignment
  ASoC: ti: davinci-i2s: Add TDM support
  ASoC: ti: davinci-i2s: Add handling of BP_FC format
  ASoC: ti: davinci-i2s: Enable unexpected frame pulses detection
  ASoC: ti: davinci-i2s: Make free-running mode optional
  ASoC: ti: davinci-i2s: Add S24_LE to supported formats
  ASoC: dt-bindings: davinic-mcbsp: Add the 'ti,drive-dx' property
  ASoC: ti: davinci-i2s: Opitonally drive DX pin during capture streams

 .../bindings/sound/davinci-mcbsp.txt          |  50 ---
 .../bindings/sound/davinci-mcbsp.yaml         | 119 +++++++
 include/linux/platform_data/davinci_asp.h     |  15 -
 sound/soc/ti/davinci-i2s.c                    | 333 ++++++++++++++----
 4 files changed, 376 insertions(+), 141 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/sound/davinci-mcbsp.txt
 create mode 100644 Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml

-- 
2.43.2

[PATCH 01/13] ASoC: dt-bindings: davinci-mcbsp: convert McBSP bindings to yaml schema

[Bastien Curutchet]

Convert the binding for McBSP controllers for TI SoCs from txt
to YAML schema.

Add properties 'clocks', 'clock-names', 'power-domains' and
'#sound-dai-cells' which were missing from the txt file.
Add '#sound-dai-cells' and 'clocks' in the example which were missing
from the txt file.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 .../bindings/sound/davinci-mcbsp.txt          | 50 ----------
 .../bindings/sound/davinci-mcbsp.yaml         | 96 +++++++++++++++++++
 2 files changed, 96 insertions(+), 50 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/sound/davinci-mcbsp.txt
 create mode 100644 Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml

diff --git a/Documentation/devicetree/bindings/sound/davinci-mcbsp.txt b/Documentation/devicetree/bindings/sound/davinci-mcbsp.txt
deleted file mode 100644
index 3ffc2562fb31..000000000000
--- a/Documentation/devicetree/bindings/sound/davinci-mcbsp.txt
+++ /dev/null
@@ -1,50 +0,0 @@
-Texas Instruments DaVinci McBSP module
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-
-This binding describes the "Multi-channel Buffered Serial Port" (McBSP)
-audio interface found in some TI DaVinci processors like the OMAP-L138 or AM180x.
-
-
-Required properties:
-~~~~~~~~~~~~~~~~~~~~
-- compatible :
-        "ti,da850-mcbsp" : for DA850, AM180x and OPAM-L138 platforms
-
-- reg : physical base address and length of the controller memory mapped
-        region(s).
-- reg-names : Should contain:
-        * "mpu" for the main registers (required).
-        * "dat" for the data FIFO (optional).
-
-- dmas: three element list of DMA controller phandles, DMA request line and
-	TC channel ordered triplets.
-- dma-names: identifier string for each DMA request line in the dmas property.
-	These strings correspond 1:1 with the ordered pairs in dmas. The dma
-	identifiers must be "rx" and "tx".
-
-Optional properties:
-~~~~~~~~~~~~~~~~~~~~
-- interrupts : Interrupt numbers for McBSP
-- interrupt-names : Known interrupt names are "rx" and "tx"
-
-- pinctrl-0: Should specify pin control group used for this controller.
-- pinctrl-names: Should contain only one value - "default", for more details
-        please refer to pinctrl-bindings.txt
-
-Example (AM1808):
-~~~~~~~~~~~~~~~~~
-
-mcbsp0: mcbsp@1d10000 {
-	compatible = "ti,da850-mcbsp";
-	pinctrl-names = "default";
-	pinctrl-0 = <&mcbsp0_pins>;
-
-	reg = 	<0x00110000 0x1000>,
-		<0x00310000 0x1000>;
-	reg-names = "mpu", "dat";
-	interrupts = <97 98>;
-	interrupt-names = "rx", "tx";
-	dmas = <&edma0 3 1
-		&edma0 2 1>;
-	dma-names = "tx", "rx";
-};
diff --git a/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml b/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml
new file mode 100644
index 000000000000..8b0e9b5da08f
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml
@@ -0,0 +1,96 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/davinci-mcbsp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: McBSP Controller for TI SoCs
+
+maintainers:
+  - Bastien Curutchet <bastien.curutchet@bootlin.com>
+
+allOf:
+  - $ref: dai-common.yaml#
+
+properties:
+  compatible:
+    enum:
+      - ti,da850-mcbsp
+
+  reg:
+    minItems: 1
+    items:
+      - description: CFG registers
+      - description: data registers
+
+  reg-names:
+    minItems: 1
+    items:
+      - const: mpu
+      - const: dat
+
+  dmas:
+    items:
+      - description: transmission DMA channel
+      - description: reception DMA channel
+
+  dma-names:
+    items:
+      - const: tx
+      - const: rx
+
+  interrupts:
+    items:
+      - description: RX interrupt
+      - description: TX interrupt
+
+  interrupt-names:
+    items:
+      - const: rx
+      - const: tx
+
+  clocks:
+    items:
+      - description: functional clock
+
+  clock-names:
+    items:
+      - const: fck
+
+  power-domains:
+    description: phandle to the corresponding power-domain
+    maxItems: 1
+
+  "#sound-dai-cells":
+    const: 0
+
+required:
+  - "#sound-dai-cells"
+  - compatible
+  - reg
+  - reg-names
+  - dmas
+  - dma-names
+  - clocks
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    mcbsp0: mcbsp0@1d10000 {
+      #sound-dai-cells = <0>;
+      compatible = "ti,da850-mcbsp";
+      pinctrl-names = "default";
+      pinctrl-0 = <&mcbsp0_pins>;
+
+      reg = <0x111000 0x1000>,
+            <0x311000 0x1000>;
+      reg-names = "mpu", "dat";
+      interrupts = <97>, <98>;
+      interrupt-names = "rx", "tx";
+      dmas = <&edma0 3 1
+              &edma0 2 1>;
+      dma-names = "tx", "rx";
+
+      clocks = <&psc1 14>;
+    };
-- 
2.43.2

Re: [PATCH 01/13] ASoC: dt-bindings: davinci-mcbsp: convert McBSP bindings to yaml schema

[Rob Herring]

On Fri, Mar 15, 2024 at 12:27:33PM +0100, Bastien Curutchet wrote:
> Convert the binding for McBSP controllers for TI SoCs from txt
> to YAML schema.
> 
> Add properties 'clocks', 'clock-names', 'power-domains' and
> '#sound-dai-cells' which were missing from the txt file.
> Add '#sound-dai-cells' and 'clocks' in the example which were missing
> from the txt file.
> 
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
>  .../bindings/sound/davinci-mcbsp.txt          | 50 ----------
>  .../bindings/sound/davinci-mcbsp.yaml         | 96 +++++++++++++++++++
>  2 files changed, 96 insertions(+), 50 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/sound/davinci-mcbsp.txt
>  create mode 100644 Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml

> diff --git a/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml b/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml
> new file mode 100644
> index 000000000000..8b0e9b5da08f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml
> @@ -0,0 +1,96 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/davinci-mcbsp.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: McBSP Controller for TI SoCs
> +
> +maintainers:
> +  - Bastien Curutchet <bastien.curutchet@bootlin.com>
> +
> +allOf:
> +  - $ref: dai-common.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,da850-mcbsp
> +
> +  reg:
> +    minItems: 1
> +    items:
> +      - description: CFG registers
> +      - description: data registers
> +
> +  reg-names:
> +    minItems: 1
> +    items:
> +      - const: mpu
> +      - const: dat
> +
> +  dmas:
> +    items:
> +      - description: transmission DMA channel
> +      - description: reception DMA channel
> +
> +  dma-names:
> +    items:
> +      - const: tx
> +      - const: rx
> +
> +  interrupts:
> +    items:
> +      - description: RX interrupt
> +      - description: TX interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: rx
> +      - const: tx
> +
> +  clocks:
> +    items:
> +      - description: functional clock
> +
> +  clock-names:
> +    items:
> +      - const: fck
> +
> +  power-domains:
> +    description: phandle to the corresponding power-domain

Drop

> +    maxItems: 1
> +
> +  "#sound-dai-cells":
> +    const: 0
> +
> +required:
> +  - "#sound-dai-cells"
> +  - compatible
> +  - reg
> +  - reg-names
> +  - dmas
> +  - dma-names
> +  - clocks
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    mcbsp0: mcbsp0@1d10000 {

Drop unused label.


> +      #sound-dai-cells = <0>;
> +      compatible = "ti,da850-mcbsp";
> +      pinctrl-names = "default";
> +      pinctrl-0 = <&mcbsp0_pins>;
> +
> +      reg = <0x111000 0x1000>,
> +            <0x311000 0x1000>;
> +      reg-names = "mpu", "dat";
> +      interrupts = <97>, <98>;
> +      interrupt-names = "rx", "tx";
> +      dmas = <&edma0 3 1
> +              &edma0 2 1>;

<> around each entry.

Otherwise,

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

> +      dma-names = "tx", "rx";
> +
> +      clocks = <&psc1 14>;
> +    };
> -- 
> 2.43.2
> 

[PATCH 02/13] ASoC: dt-bindings: davinci-mcbsp: Add new properties

[Bastien Curutchet]

Following features are not described in the bindings:
 - The McBSP uses an internal sample rate generator to provide bit clock
   or frame clock. This sample rate generator can be programmed to be
   driven by McBSP's internal clock source or by an external clock source
   (located on CLKS pin).
 - McBSP can be configured in 'free-running' mode so that its serial
   clocks will continue to run during emulation halt.
 - McBSP can generate a SYNCERR when unexpected frame pulses are detected

Add an optional clock item that allows to select an external clock as
sample rate generator's input.

Add a 'ti,disable-free-run' flag to disable the free-running mode. This
mode is selected by default by the driver that's why I add a disabling
flag instead of an enabling one.

Add a 'ti,enable-sync-err' flag to enable SYNCERR generation when
unexpected frame pulses are detected.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 .../devicetree/bindings/sound/davinci-mcbsp.yaml | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml b/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml
index 8b0e9b5da08f..d8d4e7ea6e02 100644
--- a/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml
+++ b/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml
@@ -50,12 +50,16 @@ properties:
       - const: tx
 
   clocks:
+    minItems: 1
     items:
       - description: functional clock
+      - description: external input clock for sample rate generator.
 
   clock-names:
+    minItems: 1
     items:
       - const: fck
+      - const: clks
 
   power-domains:
     description: phandle to the corresponding power-domain
@@ -64,6 +68,18 @@ properties:
   "#sound-dai-cells":
     const: 0
 
+  ti,disable-free-run:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Disable free-running mode. If not present, serial clocks continue to run
+      during emulation halt.
+
+  ti,enable-sync-err:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Enable synchronisation error detections when an unexpected frame pulse is
+      received. If not present, unexpected frame pulses are ignored.
+
 required:
   - "#sound-dai-cells"
   - compatible
-- 
2.43.2

Re: [PATCH 02/13] ASoC: dt-bindings: davinci-mcbsp: Add new properties

[Rob Herring]

On Fri, 15 Mar 2024 12:27:34 +0100, Bastien Curutchet wrote:
> Following features are not described in the bindings:
>  - The McBSP uses an internal sample rate generator to provide bit clock
>    or frame clock. This sample rate generator can be programmed to be
>    driven by McBSP's internal clock source or by an external clock source
>    (located on CLKS pin).
>  - McBSP can be configured in 'free-running' mode so that its serial
>    clocks will continue to run during emulation halt.
>  - McBSP can generate a SYNCERR when unexpected frame pulses are detected
> 
> Add an optional clock item that allows to select an external clock as
> sample rate generator's input.
> 
> Add a 'ti,disable-free-run' flag to disable the free-running mode. This
> mode is selected by default by the driver that's why I add a disabling
> flag instead of an enabling one.
> 
> Add a 'ti,enable-sync-err' flag to enable SYNCERR generation when
> unexpected frame pulses are detected.
> 
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
>  .../devicetree/bindings/sound/davinci-mcbsp.yaml | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 

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

[PATCH 03/13] ASoC: ti: davinci-i2s: Remove the unused clk_input_pin attribute

[Bastien Curutchet]

The clk_input_pin attribute of davinci_mcbsp_dev struct is not set since
commit 257ade78b601 ("ASoC: davinci-i2s: Convert to use edma-pcm").

Remove the attribute.
Keep the behaviour of the MCBSP_CLKR case as MCBSP_CLKR == 0.
I can't test the BC_FP format so I added back the initial comment that
was removed by commit ec6375533748 ("ASoC: DaVinci: Added selection of
clk input pin for McBSP"). This was the last dependency to
linux/platform_data/davinci_asp.h so it is not included anymore.

Remove the enum mcbsp_clk_input_pin from davinci_asp.h as it is not used
anywhere else.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 include/linux/platform_data/davinci_asp.h | 15 --------------
 sound/soc/ti/davinci-i2s.c                | 24 ++++-------------------
 2 files changed, 4 insertions(+), 35 deletions(-)

diff --git a/include/linux/platform_data/davinci_asp.h b/include/linux/platform_data/davinci_asp.h
index c8645b2ed3c0..b9c8520b4bd3 100644
--- a/include/linux/platform_data/davinci_asp.h
+++ b/include/linux/platform_data/davinci_asp.h
@@ -25,16 +25,6 @@ struct davinci_mcasp_pdata {
 	unsigned sram_size_capture;
 	struct gen_pool *sram_pool;
 
-	/*
-	 * If McBSP peripheral gets the clock from an external pin,
-	 * there are three chooses, that are MCBSP_CLKX, MCBSP_CLKR
-	 * and MCBSP_CLKS.
-	 * Depending on different hardware connections it is possible
-	 * to use this setting to change the behaviour of McBSP
-	 * driver.
-	 */
-	int clk_input_pin;
-
 	/*
 	 * This flag works when both clock and FS are outputs for the cpu
 	 * and makes clock more accurate (FS is not symmetrical and the
@@ -91,11 +81,6 @@ enum {
 	MCASP_VERSION_OMAP,	/* OMAP4/5 */
 };
 
-enum mcbsp_clk_input_pin {
-	MCBSP_CLKR = 0,		/* as in DM365 */
-	MCBSP_CLKS,
-};
-
 #define INACTIVE_MODE	0
 #define TX_MODE		1
 #define RX_MODE		2
diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c
index 07c8b2259208..5c906641640e 100644
--- a/sound/soc/ti/davinci-i2s.c
+++ b/sound/soc/ti/davinci-i2s.c
@@ -19,7 +19,6 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/clk.h>
-#include <linux/platform_data/davinci_asp.h>
 
 #include <sound/core.h>
 #include <sound/pcm.h>
@@ -159,7 +158,6 @@ struct davinci_mcbsp_dev {
 
 	unsigned int fmt;
 	int clk_div;
-	int clk_input_pin;
 	bool i2s_accurate_sck;
 };
 
@@ -239,26 +237,12 @@ static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 			DAVINCI_MCBSP_PCR_CLKRM;
 		break;
 	case SND_SOC_DAIFMT_BC_FP:
-		pcr = DAVINCI_MCBSP_PCR_FSRM | DAVINCI_MCBSP_PCR_FSXM;
 		/*
-		 * Selection of the clock input pin that is the
-		 * input for the Sample Rate Generator.
-		 * McBSP FSR and FSX are driven by the Sample Rate
-		 * Generator.
+		 * McBSP CLKR pin is the input for the Sample Rate Generator.
+		 * McBSP FSR and FSX are driven by the Sample Rate Generator.
 		 */
-		switch (dev->clk_input_pin) {
-		case MCBSP_CLKS:
-			pcr |= DAVINCI_MCBSP_PCR_CLKXM |
-				DAVINCI_MCBSP_PCR_CLKRM;
-			break;
-		case MCBSP_CLKR:
-			pcr |= DAVINCI_MCBSP_PCR_SCLKME;
-			break;
-		default:
-			dev_err(dev->dev, "bad clk_input_pin\n");
-			return -EINVAL;
-		}
-
+		pcr = DAVINCI_MCBSP_PCR_FSRM | DAVINCI_MCBSP_PCR_FSXM;
+		pcr |= DAVINCI_MCBSP_PCR_SCLKME;
 		break;
 	case SND_SOC_DAIFMT_BC_FC:
 		/* codec is master */
-- 
2.43.2

[PATCH 04/13] ASoC: ti: davinci-i2s: Replace dev_err with dev_err_probe

[Bastien Curutchet]

There are dev_err() in the probe() function.

Replace them with dev_err_probe()

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 sound/soc/ti/davinci-i2s.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c
index 5c906641640e..4cb3ef62db20 100644
--- a/sound/soc/ti/davinci-i2s.c
+++ b/sound/soc/ti/davinci-i2s.c
@@ -644,8 +644,7 @@ static int davinci_i2s_probe(struct platform_device *pdev)
 			 "\"mpu\" mem resource not found, using index 0\n");
 		mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 		if (!mem) {
-			dev_err(&pdev->dev, "no mem resource?\n");
-			return -ENODEV;
+			return dev_err_probe(&pdev->dev, -ENODEV, "no mem resource?\n");
 		}
 	}
 
@@ -672,8 +671,7 @@ static int davinci_i2s_probe(struct platform_device *pdev)
 	} else if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) {
 		dma_data->filter_data = "tx";
 	} else {
-		dev_err(&pdev->dev, "Missing DMA tx resource\n");
-		return -ENODEV;
+		return dev_err_probe(&pdev->dev, -ENODEV, "Missing DMA tx resource\n");
 	}
 
 	dma_data = &dev->dma_data[SNDRV_PCM_STREAM_CAPTURE];
@@ -687,8 +685,7 @@ static int davinci_i2s_probe(struct platform_device *pdev)
 	} else if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) {
 		dma_data->filter_data = "rx";
 	} else {
-		dev_err(&pdev->dev, "Missing DMA rx resource\n");
-		return -ENODEV;
+		return dev_err_probe(&pdev->dev, -ENODEV, "Missing DMA rx resource\n");
 	}
 
 	dev->clk = clk_get(&pdev->dev, NULL);
@@ -708,7 +705,7 @@ static int davinci_i2s_probe(struct platform_device *pdev)
 
 	ret = edma_pcm_platform_register(&pdev->dev);
 	if (ret) {
-		dev_err(&pdev->dev, "register PCM failed: %d\n", ret);
+		dev_err_probe(&pdev->dev, ret, "register PCM failed\n");
 		goto err_unregister_component;
 	}
 
-- 
2.43.2

Re: [PATCH 04/13] ASoC: ti: davinci-i2s: Replace dev_err with dev_err_probe

[Mark Brown]

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

On Fri, Mar 15, 2024 at 12:27:36PM +0100, Bastien Curutchet wrote:

> -			dev_err(&pdev->dev, "no mem resource?\n");
> -			return -ENODEV;
> +			return dev_err_probe(&pdev->dev, -ENODEV, "no mem resource?\n");
>  		}

dev_err_probe() with a fixed error code doesn't seem to make much sense,
the whole point is to handle deferral but for a straight lookup like
this that can't happen.

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

Re: [PATCH 04/13] ASoC: ti: davinci-i2s: Replace dev_err with dev_err_probe

[Herve Codina]

Hi Mark,

On Fri, 15 Mar 2024 14:07:13 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Fri, Mar 15, 2024 at 12:27:36PM +0100, Bastien Curutchet wrote:
> 
> > -			dev_err(&pdev->dev, "no mem resource?\n");
> > -			return -ENODEV;
> > +			return dev_err_probe(&pdev->dev, -ENODEV, "no mem resource?\n");
> >  		}  
> 
> dev_err_probe() with a fixed error code doesn't seem to make much sense,
> the whole point is to handle deferral but for a straight lookup like
> this that can't happen.

The error code is uniformly formatted and the error path is more compact.
  https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L4963

IMHO, to benefit of these feature, it makes sense to use it even with a fixed
error code.

Best regards,
Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 04/13] ASoC: ti: davinci-i2s: Replace dev_err with dev_err_probe

[Mark Brown]

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

On Fri, Mar 15, 2024 at 03:23:32PM +0100, Herve Codina wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > dev_err_probe() with a fixed error code doesn't seem to make much sense,
> > the whole point is to handle deferral but for a straight lookup like
> > this that can't happen.

> The error code is uniformly formatted and the error path is more compact.
>   https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L4963

> IMHO, to benefit of these feature, it makes sense to use it even with a fixed
> error code.

I'm not convinced TBH, the fixed error code smells pretty bad.

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

Re: [PATCH 04/13] ASoC: ti: davinci-i2s: Replace dev_err with dev_err_probe

[Bastien Curutchet]

Hi Mark,

On 3/15/24 15:40, Mark Brown wrote:
> On Fri, Mar 15, 2024 at 03:23:32PM +0100, Herve Codina wrote:
>> Mark Brown <broonie@kernel.org> wrote:
> 
>>> dev_err_probe() with a fixed error code doesn't seem to make much sense,
>>> the whole point is to handle deferral but for a straight lookup like
>>> this that can't happen.
> 
>> The error code is uniformly formatted and the error path is more compact.
>>    https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L4963
> 
>> IMHO, to benefit of these feature, it makes sense to use it even with a fixed
>> error code.
> 
> I'm not convinced TBH, the fixed error code smells pretty bad.

Ok. I'll keep the dev_err() for the fixed errors then, and use the 
dev_err_probe() for the others, would that be ok ?

Best regards,
Bastien

Re: [PATCH 04/13] ASoC: ti: davinci-i2s: Replace dev_err with dev_err_probe

[Mark Brown]

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

On Mon, Mar 18, 2024 at 08:40:24AM +0100, Bastien Curutchet wrote:
> On 3/15/24 15:40, Mark Brown wrote:

> > I'm not convinced TBH, the fixed error code smells pretty bad.

> Ok. I'll keep the dev_err() for the fixed errors then, and use the
> dev_err_probe() for the others, would that be ok ?

Yes, when we can get a deferral it's the right thing.

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

[PATCH 05/13] ASoC: ti: davinci-i2s: Use external clock to drive sample rate generator

[Bastien Curutchet]

McBSP's internal sample rate generator can be programed to be driven by
its internal clock or by an external clock source located on CLKS pin.
The external clock source case is not handled by the driver.

Handle an optional clock related to this external clock source. If
present, the driver uses the clock located on CLKS pin as input for the
sample rate generator. Thus, the external clock rate is used to compute
divisors. If this optional clock is not present, the sample rate
generator is driven by the McBSP's functional clock.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 sound/soc/ti/davinci-i2s.c | 65 ++++++++++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 16 deletions(-)

diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c
index 4cb3ef62db20..8ee22d79b0f4 100644
--- a/sound/soc/ti/davinci-i2s.c
+++ b/sound/soc/ti/davinci-i2s.c
@@ -134,6 +134,7 @@ struct davinci_mcbsp_dev {
 	int				mode;
 	u32				pcr;
 	struct clk			*clk;
+	struct clk			*ext_clk;
 	/*
 	 * Combining both channels into 1 element will at least double the
 	 * amount of time between servicing the dma channel, increase
@@ -364,7 +365,8 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
 	struct davinci_mcbsp_dev *dev = snd_soc_dai_get_drvdata(dai);
 	struct snd_interval *i = NULL;
 	int mcbsp_word_length, master;
-	unsigned int rcr, xcr, srgr, clk_div, freq, framesize;
+	unsigned int rcr, xcr, clk_div, freq, framesize;
+	unsigned int srgr = 0;
 	u32 spcr;
 	snd_pcm_format_t fmt;
 	unsigned element_cnt = 1;
@@ -385,9 +387,13 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
 
 	switch (master) {
 	case SND_SOC_DAIFMT_BP_FP:
-		freq = clk_get_rate(dev->clk);
-		srgr = DAVINCI_MCBSP_SRGR_FSGM |
-		       DAVINCI_MCBSP_SRGR_CLKSM;
+		if (dev->ext_clk) {
+			freq = clk_get_rate(dev->ext_clk);
+		} else {
+			freq = clk_get_rate(dev->clk);
+			srgr = DAVINCI_MCBSP_SRGR_CLKSM;
+		}
+		srgr |= DAVINCI_MCBSP_SRGR_FSGM;
 		srgr |= DAVINCI_MCBSP_SRGR_FWID(mcbsp_word_length *
 						8 - 1);
 		if (dev->i2s_accurate_sck) {
@@ -688,12 +694,36 @@ static int davinci_i2s_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, -ENODEV, "Missing DMA rx resource\n");
 	}
 
-	dev->clk = clk_get(&pdev->dev, NULL);
+	/*
+	 * The optional is there for backward compatibility.
+	 * If 'fck' is not present, the clk_get(dev, NULL) that follows may find something
+	 */
+	dev->clk = devm_clk_get_optional(&pdev->dev, "fck");
 	if (IS_ERR(dev->clk))
-		return -ENODEV;
-	ret = clk_enable(dev->clk);
+		return dev_err_probe(&pdev->dev, PTR_ERR(dev->clk), "Invalid functional clock\n");
+	if (!dev->clk) {
+		dev->clk = devm_clk_get(&pdev->dev, NULL);
+		if (IS_ERR(dev->clk))
+			return dev_err_probe(&pdev->dev, PTR_ERR(dev->clk),
+					     "Missing functional clock\n");
+	}
+
+	dev->ext_clk = devm_clk_get_optional(&pdev->dev, "clks");
+	if (IS_ERR(dev->ext_clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(dev->ext_clk), "Invalid external clock\n");
+
+	ret = clk_prepare_enable(dev->clk);
 	if (ret)
-		goto err_put_clk;
+		return ret;
+
+	if (dev->ext_clk) {
+		dev_dbg(&pdev->dev, "External clock used for sample rate generator\n");
+		ret = clk_prepare_enable(dev->ext_clk);
+		if (ret) {
+			dev_err_probe(&pdev->dev, ret, "Failed to enable external clock\n");
+			goto err_disable_clk;
+		}
+	}
 
 	dev->dev = &pdev->dev;
 	dev_set_drvdata(&pdev->dev, dev);
@@ -701,7 +731,7 @@ static int davinci_i2s_probe(struct platform_device *pdev)
 	ret = snd_soc_register_component(&pdev->dev, &davinci_i2s_component,
 					 &davinci_i2s_dai, 1);
 	if (ret != 0)
-		goto err_release_clk;
+		goto err_disable_ext_clk;
 
 	ret = edma_pcm_platform_register(&pdev->dev);
 	if (ret) {
@@ -713,10 +743,12 @@ static int davinci_i2s_probe(struct platform_device *pdev)
 
 err_unregister_component:
 	snd_soc_unregister_component(&pdev->dev);
-err_release_clk:
-	clk_disable(dev->clk);
-err_put_clk:
-	clk_put(dev->clk);
+err_disable_ext_clk:
+	if (dev->ext_clk)
+		clk_disable_unprepare(dev->ext_clk);
+err_disable_clk:
+	clk_disable_unprepare(dev->clk);
+
 	return ret;
 }
 
@@ -726,9 +758,10 @@ static void davinci_i2s_remove(struct platform_device *pdev)
 
 	snd_soc_unregister_component(&pdev->dev);
 
-	clk_disable(dev->clk);
-	clk_put(dev->clk);
-	dev->clk = NULL;
+	clk_disable_unprepare(dev->clk);
+
+	if (dev->ext_clk)
+		clk_disable_unprepare(dev->ext_clk);
 }
 
 static const struct of_device_id davinci_i2s_match[] __maybe_unused = {
-- 
2.43.2

[PATCH 06/13] ASoC: ti: davinci-i2s: Delete unnecessary assignment

[Bastien Curutchet]

In davinci_i2s_hw_params(), mcbsp_word_length is set twice to
asp_word_length[fmt].

Remove second unnecessary assignment.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 sound/soc/ti/davinci-i2s.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c
index 8ee22d79b0f4..f4514d4dff07 100644
--- a/sound/soc/ti/davinci-i2s.c
+++ b/sound/soc/ti/davinci-i2s.c
@@ -479,7 +479,6 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
 			return -EINVAL;
 		}
 	}
-	mcbsp_word_length = asp_word_length[fmt];
 
 	switch (master) {
 	case SND_SOC_DAIFMT_BP_FP:
-- 
2.43.2

[PATCH 07/13] ASoC: ti: davinci-i2s: Add TDM support

[Bastien Curutchet]

TDM is not supported by the McBSP driver. The McBSP datasheet does not
name explicitly TDM as a supported format but it is possible to configure
the McBSP to do TDM if all slots are used by McBSP.

Add TDM support. It uses single-phase frame. Slot width is used to
compute the McBSP's word length.

Implement the set_tdm_slot() hook of snd_soc_dai_ops struct. It only
supports TDM if all slots are used by McBSP.

The snd_soc_dai_driver's capture.channels_max is updated from 2 to 128.
128 is the maximum frame length when using single-phase frame.
playback.channels_max has not been updated as I could not test TDM on
playbacks with my hardware.

This was tested on platform designed off of DAVINCI/OMAP_L138 with BP_FC
format so this is only supported format for TDM yet. A check is done in
davinci_i2s_set_dai_fmt() to prevent TDM to be used with other formats

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 sound/soc/ti/davinci-i2s.c | 81 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 77 insertions(+), 4 deletions(-)

diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c
index f4514d4dff07..4adaed010700 100644
--- a/sound/soc/ti/davinci-i2s.c
+++ b/sound/soc/ti/davinci-i2s.c
@@ -160,6 +160,9 @@ struct davinci_mcbsp_dev {
 	unsigned int fmt;
 	int clk_div;
 	bool i2s_accurate_sck;
+
+	int tdm_slots;
+	int slot_width;
 };
 
 static inline void davinci_mcbsp_write_reg(struct davinci_mcbsp_dev *dev,
@@ -213,6 +216,57 @@ static void davinci_mcbsp_stop(struct davinci_mcbsp_dev *dev, int playback)
 	toggle_clock(dev, playback);
 }
 
+static int davinci_i2s_tdm_word_length(int tdm_slot_width)
+{
+	switch (tdm_slot_width) {
+	case 8:
+		return DAVINCI_MCBSP_WORD_8;
+	case 12:
+		return DAVINCI_MCBSP_WORD_12;
+	case 16:
+		return DAVINCI_MCBSP_WORD_16;
+	case 20:
+		return DAVINCI_MCBSP_WORD_20;
+	case 24:
+		return DAVINCI_MCBSP_WORD_24;
+	case 32:
+		return DAVINCI_MCBSP_WORD_32;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int davinci_i2s_set_tdm_slot(struct snd_soc_dai *cpu_dai,
+				    unsigned int tx_mask,
+				    unsigned int rx_mask,
+				    int slots, int slot_width)
+{
+	struct davinci_mcbsp_dev *dev = snd_soc_dai_get_drvdata(cpu_dai);
+
+	dev_dbg(dev->dev, "%s - slots %d, slot_width %d\n", __func__, slots, slot_width);
+
+	if (slots > 128 || !slots) {
+		dev_err(dev->dev, "Invalid number of slots\n");
+		return -EINVAL;
+	}
+
+	if (rx_mask != (1 << slots) - 1) {
+		dev_err(dev->dev, "Invalid RX mask (0x%08x) : all slots must be used by McBSP\n",
+			rx_mask);
+		return -EINVAL;
+	}
+
+	if (davinci_i2s_tdm_word_length(slot_width) < 0) {
+		dev_err(dev->dev, "%s: Unsupported slot_width %d\n", __func__, slot_width);
+		return -EINVAL;
+	}
+
+	dev->tdm_slots = slots;
+	dev->slot_width = slot_width;
+
+	return 0;
+}
+
 #define DEFAULT_BITPERSAMPLE	16
 
 static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,
@@ -228,6 +282,13 @@ static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 		DAVINCI_MCBSP_SRGR_FWID(DEFAULT_BITPERSAMPLE - 1);
 
 	dev->fmt = fmt;
+
+	if ((dev->tdm_slots || dev->slot_width) &&
+	    ((fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) != SND_SOC_DAIFMT_BP_FC)) {
+		dev_err(dev->dev, "TDM is only supported for BP_FC format\n");
+		return -EINVAL;
+	}
+
 	/* set master/slave audio interface */
 	switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
 	case SND_SOC_DAIFMT_BP_FP:
@@ -383,7 +444,13 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
 
 	master = dev->fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK;
 	fmt = params_format(params);
-	mcbsp_word_length = asp_word_length[fmt];
+	if (dev->slot_width)
+		mcbsp_word_length = davinci_i2s_tdm_word_length(dev->slot_width);
+	else
+		mcbsp_word_length = asp_word_length[fmt];
+
+	if (mcbsp_word_length < 0)
+		return mcbsp_word_length;
 
 	switch (master) {
 	case SND_SOC_DAIFMT_BP_FP:
@@ -483,8 +550,13 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
 	switch (master) {
 	case SND_SOC_DAIFMT_BP_FP:
 	case SND_SOC_DAIFMT_BP_FC:
-		rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(0);
-		xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(0);
+		if (dev->tdm_slots > 0) {
+			rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(dev->tdm_slots - 1);
+			xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(dev->tdm_slots - 1);
+		} else {
+			rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(0);
+			xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(0);
+		}
 		break;
 	case SND_SOC_DAIFMT_BC_FC:
 	case SND_SOC_DAIFMT_BC_FP:
@@ -609,6 +681,7 @@ static const struct snd_soc_dai_ops davinci_i2s_dai_ops = {
 	.hw_params	= davinci_i2s_hw_params,
 	.set_fmt	= davinci_i2s_set_dai_fmt,
 	.set_clkdiv	= davinci_i2s_dai_set_clkdiv,
+	.set_tdm_slot   = davinci_i2s_set_tdm_slot,
 
 };
 
@@ -621,7 +694,7 @@ static struct snd_soc_dai_driver davinci_i2s_dai = {
 	},
 	.capture = {
 		.channels_min = 2,
-		.channels_max = 2,
+		.channels_max = 128,
 		.rates = DAVINCI_I2S_RATES,
 		.formats = DAVINCI_I2S_FORMATS,
 	},
-- 
2.43.2

Re: [PATCH 07/13] ASoC: ti: davinci-i2s: Add TDM support

[Péter Ujfalusi]

On 15/03/2024 13:27, Bastien Curutchet wrote:
> TDM is not supported by the McBSP driver. The McBSP datasheet does not
> name explicitly TDM as a supported format but it is possible to configure
> the McBSP to do TDM if all slots are used by McBSP.
> 
> Add TDM support. It uses single-phase frame. Slot width is used to
> compute the McBSP's word length.
> 
> Implement the set_tdm_slot() hook of snd_soc_dai_ops struct. It only
> supports TDM if all slots are used by McBSP.
> 
> The snd_soc_dai_driver's capture.channels_max is updated from 2 to 128.
> 128 is the maximum frame length when using single-phase frame.
> playback.channels_max has not been updated as I could not test TDM on
> playbacks with my hardware.
> 
> This was tested on platform designed off of DAVINCI/OMAP_L138 with BP_FC
> format so this is only supported format for TDM yet. A check is done in
> davinci_i2s_set_dai_fmt() to prevent TDM to be used with other formats
> 
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
>  sound/soc/ti/davinci-i2s.c | 81 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 77 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c
> index f4514d4dff07..4adaed010700 100644
> --- a/sound/soc/ti/davinci-i2s.c
> +++ b/sound/soc/ti/davinci-i2s.c
> @@ -160,6 +160,9 @@ struct davinci_mcbsp_dev {
>  	unsigned int fmt;
>  	int clk_div;
>  	bool i2s_accurate_sck;
> +
> +	int tdm_slots;
> +	int slot_width;
>  };
>  
>  static inline void davinci_mcbsp_write_reg(struct davinci_mcbsp_dev *dev,
> @@ -213,6 +216,57 @@ static void davinci_mcbsp_stop(struct davinci_mcbsp_dev *dev, int playback)
>  	toggle_clock(dev, playback);
>  }
>  
> +static int davinci_i2s_tdm_word_length(int tdm_slot_width)
> +{
> +	switch (tdm_slot_width) {
> +	case 8:
> +		return DAVINCI_MCBSP_WORD_8;
> +	case 12:
> +		return DAVINCI_MCBSP_WORD_12;
> +	case 16:
> +		return DAVINCI_MCBSP_WORD_16;
> +	case 20:
> +		return DAVINCI_MCBSP_WORD_20;
> +	case 24:
> +		return DAVINCI_MCBSP_WORD_24;
> +	case 32:
> +		return DAVINCI_MCBSP_WORD_32;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int davinci_i2s_set_tdm_slot(struct snd_soc_dai *cpu_dai,
> +				    unsigned int tx_mask,
> +				    unsigned int rx_mask,
> +				    int slots, int slot_width)
> +{
> +	struct davinci_mcbsp_dev *dev = snd_soc_dai_get_drvdata(cpu_dai);
> +
> +	dev_dbg(dev->dev, "%s - slots %d, slot_width %d\n", __func__, slots, slot_width);

The __func__ can be ommited, it is better to leave it for dynamic
debugging by adding "dyndbg=+pmf" module parameter if needed.

> +
> +	if (slots > 128 || !slots) {
> +		dev_err(dev->dev, "Invalid number of slots\n");
> +		return -EINVAL;
> +	}
> +
> +	if (rx_mask != (1 << slots) - 1) {
> +		dev_err(dev->dev, "Invalid RX mask (0x%08x) : all slots must be used by McBSP\n",
> +			rx_mask);
> +		return -EINVAL;

This is only a restriction for RX?

> +	}
> +
> +	if (davinci_i2s_tdm_word_length(slot_width) < 0) {
> +		dev_err(dev->dev, "%s: Unsupported slot_width %d\n", __func__, slot_width);
> +		return -EINVAL;
> +	}
> +
> +	dev->tdm_slots = slots;
> +	dev->slot_width = slot_width;
> +
> +	return 0;
> +}
> +
>  #define DEFAULT_BITPERSAMPLE	16
>  
>  static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,
> @@ -228,6 +282,13 @@ static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,
>  		DAVINCI_MCBSP_SRGR_FWID(DEFAULT_BITPERSAMPLE - 1);
>  
>  	dev->fmt = fmt;
> +
> +	if ((dev->tdm_slots || dev->slot_width) &&
> +	    ((fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) != SND_SOC_DAIFMT_BP_FC)) {
> +		dev_err(dev->dev, "TDM is only supported for BP_FC format\n");
> +		return -EINVAL;

I think this is not a valid statement, Fsync can be generated internally
or coming from external source in TDM mode also.

> +	}
> +
>  	/* set master/slave audio interface */
>  	switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
>  	case SND_SOC_DAIFMT_BP_FP:
> @@ -383,7 +444,13 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
>  
>  	master = dev->fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK;
>  	fmt = params_format(params);
> -	mcbsp_word_length = asp_word_length[fmt];
> +	if (dev->slot_width)
> +		mcbsp_word_length = davinci_i2s_tdm_word_length(dev->slot_width);
> +	else
> +		mcbsp_word_length = asp_word_length[fmt];
> +
> +	if (mcbsp_word_length < 0)
> +		return mcbsp_word_length;
>  
>  	switch (master) {
>  	case SND_SOC_DAIFMT_BP_FP:
> @@ -483,8 +550,13 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
>  	switch (master) {
>  	case SND_SOC_DAIFMT_BP_FP:
>  	case SND_SOC_DAIFMT_BP_FC:
> -		rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(0);
> -		xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(0);
> +		if (dev->tdm_slots > 0) {
> +			rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(dev->tdm_slots - 1);
> +			xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(dev->tdm_slots - 1);
> +		} else {
> +			rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(0);
> +			xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(0);
> +		}
>  		break;
>  	case SND_SOC_DAIFMT_BC_FC:
>  	case SND_SOC_DAIFMT_BC_FP:
> @@ -609,6 +681,7 @@ static const struct snd_soc_dai_ops davinci_i2s_dai_ops = {
>  	.hw_params	= davinci_i2s_hw_params,
>  	.set_fmt	= davinci_i2s_set_dai_fmt,
>  	.set_clkdiv	= davinci_i2s_dai_set_clkdiv,
> +	.set_tdm_slot   = davinci_i2s_set_tdm_slot,
>  
>  };
>  
> @@ -621,7 +694,7 @@ static struct snd_soc_dai_driver davinci_i2s_dai = {
>  	},
>  	.capture = {
>  		.channels_min = 2,
> -		.channels_max = 2,
> +		.channels_max = 128,
>  		.rates = DAVINCI_I2S_RATES,
>  		.formats = DAVINCI_I2S_FORMATS,
>  	},

-- 
Péter

Re: [PATCH 07/13] ASoC: ti: davinci-i2s: Add TDM support

[Bastien Curutchet]

Hi Péter,

>> +static int davinci_i2s_set_tdm_slot(struct snd_soc_dai *cpu_dai,
>> +				    unsigned int tx_mask,
>> +				    unsigned int rx_mask,
>> +				    int slots, int slot_width)
>> +{
>> +	struct davinci_mcbsp_dev *dev = snd_soc_dai_get_drvdata(cpu_dai);
>> +
>> +	dev_dbg(dev->dev, "%s - slots %d, slot_width %d\n", __func__, slots, slot_width);
> 
> The __func__ can be ommited, it is better to leave it for dynamic
> debugging by adding "dyndbg=+pmf" module parameter if needed.
> 

True, I'll remove the __func__.

>> +
>> +	if (slots > 128 || !slots) {
>> +		dev_err(dev->dev, "Invalid number of slots\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (rx_mask != (1 << slots) - 1) {
>> +		dev_err(dev->dev, "Invalid RX mask (0x%08x) : all slots must be used by McBSP\n",
>> +			rx_mask);
>> +		return -EINVAL;
> 
> This is only a restriction for RX?
> 

Nope you're right, I'll add the same for tx_mask.

>> +	}
>> +
>> +	if (davinci_i2s_tdm_word_length(slot_width) < 0) {
>> +		dev_err(dev->dev, "%s: Unsupported slot_width %d\n", __func__, slot_width);
>> +		return -EINVAL;
>> +	}
>> +
>> +	dev->tdm_slots = slots;
>> +	dev->slot_width = slot_width;
>> +
>> +	return 0;
>> +}
>> +
>>   #define DEFAULT_BITPERSAMPLE	16
>>   
>>   static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,
>> @@ -228,6 +282,13 @@ static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,
>>   		DAVINCI_MCBSP_SRGR_FWID(DEFAULT_BITPERSAMPLE - 1);
>>   
>>   	dev->fmt = fmt;
>> +
>> +	if ((dev->tdm_slots || dev->slot_width) &&
>> +	    ((fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) != SND_SOC_DAIFMT_BP_FC)) {
>> +		dev_err(dev->dev, "TDM is only supported for BP_FC format\n");
>> +		return -EINVAL;
> 
> I think this is not a valid statement, Fsync can be generated internally
> or coming from external source in TDM mode also.
> 

My hardware allow me to only test BP_FC so I wished to put some
'barriers' in front of untested things.


Best regards,
Bastien

Re: [PATCH 07/13] ASoC: ti: davinci-i2s: Add TDM support

[Péter Ujfalusi]

HI Bastien,

On 20/03/2024 09:31, Bastien Curutchet wrote:
>>> +    if ((dev->tdm_slots || dev->slot_width) &&
>>> +        ((fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) !=
>>> SND_SOC_DAIFMT_BP_FC)) {
>>> +        dev_err(dev->dev, "TDM is only supported for BP_FC format\n");
>>> +        return -EINVAL;
>>
>> I think this is not a valid statement, Fsync can be generated internally
>> or coming from external source in TDM mode also.
>>
> 
> My hardware allow me to only test BP_FC so I wished to put some
> 'barriers' in front of untested things.

I don't see restrictions on the other changes affecting this to be lifted.
I would allow TDM for full clock provider mode also.

> 
> 
> Best regards,
> Bastien

-- 
Péter

[PATCH 08/13] ASoC: ti: davinci-i2s: Add handling of BP_FC format

[Bastien Curutchet]

McBSP is able to drive bit clock and consume frame clock but BP_FC
format is not handled by McBSP driver.

Add BP_FC format support.
When BP_FC is selected:
  - CLKX and CLKR are configured as outputs
  - The sample rate generator is configured to be able to provide bit
    clock.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 sound/soc/ti/davinci-i2s.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c
index 4adaed010700..7cdd86f47ead 100644
--- a/sound/soc/ti/davinci-i2s.c
+++ b/sound/soc/ti/davinci-i2s.c
@@ -306,6 +306,12 @@ static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 		pcr = DAVINCI_MCBSP_PCR_FSRM | DAVINCI_MCBSP_PCR_FSXM;
 		pcr |= DAVINCI_MCBSP_PCR_SCLKME;
 		break;
+	case SND_SOC_DAIFMT_BP_FC:
+		/* cpu is bitclock provider */
+		pcr = DAVINCI_MCBSP_PCR_CLKXM |
+			DAVINCI_MCBSP_PCR_CLKRM;
+		break;
+
 	case SND_SOC_DAIFMT_BC_FC:
 		/* codec is master */
 		pcr = 0;
@@ -491,6 +497,23 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
 		clk_div &= 0xFF;
 		srgr |= clk_div;
 		break;
+	case SND_SOC_DAIFMT_BP_FC:
+		if (dev->ext_clk) {
+			freq = clk_get_rate(dev->ext_clk);
+		} else {
+			freq = clk_get_rate(dev->clk);
+			srgr = DAVINCI_MCBSP_SRGR_CLKSM;
+		}
+		if (dev->tdm_slots && dev->slot_width) {
+			clk_div = freq / (params->rate_num * params->rate_den)
+				 / (dev->tdm_slots * dev->slot_width) - 1;
+		} else {
+			clk_div = freq / (mcbsp_word_length * 16) /
+				  params->rate_num * params->rate_den;
+		}
+		clk_div &= 0xFF;
+		srgr |= clk_div;
+		break;
 	case SND_SOC_DAIFMT_BC_FC:
 		/* Clock and frame sync given from external sources */
 		i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS);
-- 
2.43.2

[PATCH 09/13] ASoC: ti: davinci-i2s: Enable unexpected frame pulses detection

[Bastien Curutchet]

McBSP can generate an SYNCERR when unexpected frame pulses are
detected. The driver always disables this feature and ignore the
unexpected frame pulses.

Enable the generation of SYNCERR by the McBSP according to the
'ti,enable-sync-err' device-tree property.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 sound/soc/ti/davinci-i2s.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c
index 7cdd86f47ead..2c19e45b6b54 100644
--- a/sound/soc/ti/davinci-i2s.c
+++ b/sound/soc/ti/davinci-i2s.c
@@ -163,6 +163,8 @@ struct davinci_mcbsp_dev {
 
 	int tdm_slots;
 	int slot_width;
+
+	bool sync_err;
 };
 
 static inline void davinci_mcbsp_write_reg(struct davinci_mcbsp_dev *dev,
@@ -432,8 +434,10 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
 	struct davinci_mcbsp_dev *dev = snd_soc_dai_get_drvdata(dai);
 	struct snd_interval *i = NULL;
 	int mcbsp_word_length, master;
-	unsigned int rcr, xcr, clk_div, freq, framesize;
+	unsigned int clk_div, freq, framesize;
 	unsigned int srgr = 0;
+	unsigned int rcr = 0;
+	unsigned int xcr = 0;
 	u32 spcr;
 	snd_pcm_format_t fmt;
 	unsigned element_cnt = 1;
@@ -530,8 +534,10 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
 	}
 	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SRGR_REG, srgr);
 
-	rcr = DAVINCI_MCBSP_RCR_RFIG;
-	xcr = DAVINCI_MCBSP_XCR_XFIG;
+	if (!dev->sync_err) {
+		rcr |= DAVINCI_MCBSP_RCR_RFIG;
+		xcr |= DAVINCI_MCBSP_XCR_XFIG;
+	}
 	if (dev->mode == MOD_DSP_B) {
 		rcr |= DAVINCI_MCBSP_RCR_RDATDLY(0);
 		xcr |= DAVINCI_MCBSP_XCR_XDATDLY(0);
@@ -760,6 +766,8 @@ static int davinci_i2s_probe(struct platform_device *pdev)
 
 	dev->base = io_base;
 
+	dev->sync_err = of_property_read_bool(pdev->dev.of_node, "ti,enable-sync-err");
+
 	/* setup DMA, first TX, then RX */
 	dma_data = &dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK];
 	dma_data->addr = (dma_addr_t)(mem->start + DAVINCI_MCBSP_DXR_REG);
-- 
2.43.2

Re: [PATCH 09/13] ASoC: ti: davinci-i2s: Enable unexpected frame pulses detection

[Mark Brown]

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

On Fri, Mar 15, 2024 at 12:27:41PM +0100, Bastien Curutchet wrote:

> McBSP can generate an SYNCERR when unexpected frame pulses are
> detected. The driver always disables this feature and ignore the
> unexpected frame pulses.

What does "unexpected" mean?

> Enable the generation of SYNCERR by the McBSP according to the
> 'ti,enable-sync-err' device-tree property.

Why would this be optional, and how is this reported - I'm not seeing
any interrupt handling updates?

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

Re: [PATCH 09/13] ASoC: ti: davinci-i2s: Enable unexpected frame pulses detection

[Bastien Curutchet]

Hi Mark,

On 3/15/24 15:09, Mark Brown wrote:
> On Fri, Mar 15, 2024 at 12:27:41PM +0100, Bastien Curutchet wrote:
> 
>> McBSP can generate an SYNCERR when unexpected frame pulses are
>> detected. The driver always disables this feature and ignore the
>> unexpected frame pulses.
> 
> What does "unexpected" mean?

Unexpected frame sync pulse is defined in datasheet as a sync pulse that 
occurs <N> bit clocks earlier than the last transmitted bit of the 
previous frame. The <N> can be configured through registers.

> 
>> Enable the generation of SYNCERR by the McBSP according to the
>> 'ti,enable-sync-err' device-tree property.
> 
> Why would this be optional, and how is this reported - I'm not seeing
> any interrupt handling updates?

It is possible to deliberately ignore them and that is what is done 
today in the driver.
This is reported as a status bit in a register. An interrupt can indeed 
be generated from this but I'm not using it (now at least).
I use the fact that McBSP automatically drops previous element and 
starts a new reception when an unexpected frame pulse occurs.


Best regards,
Bastien

Re: [PATCH 09/13] ASoC: ti: davinci-i2s: Enable unexpected frame pulses detection

[Mark Brown]

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

On Fri, Mar 15, 2024 at 03:45:24PM +0100, Bastien Curutchet wrote:
> On 3/15/24 15:09, Mark Brown wrote:
> > On Fri, Mar 15, 2024 at 12:27:41PM +0100, Bastien Curutchet wrote:

> > > McBSP can generate an SYNCERR when unexpected frame pulses are
> > > detected. The driver always disables this feature and ignore the
> > > unexpected frame pulses.

> > What does "unexpected" mean?

> Unexpected frame sync pulse is defined in datasheet as a sync pulse that
> occurs <N> bit clocks earlier than the last transmitted bit of the previous
> frame. The <N> can be configured through registers.

> > > Enable the generation of SYNCERR by the McBSP according to the
> > > 'ti,enable-sync-err' device-tree property.

> > Why would this be optional, and how is this reported - I'm not seeing
> > any interrupt handling updates?

> It is possible to deliberately ignore them and that is what is done today in
> the driver.
> This is reported as a status bit in a register. An interrupt can indeed be
> generated from this but I'm not using it (now at least).
> I use the fact that McBSP automatically drops previous element and starts a
> new reception when an unexpected frame pulse occurs.

That sounds like a very standard behaviour for incorrect clocking.  I
don't think this needs configuration at all, just enable this mode.

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

[PATCH 10/13] ASoC: ti: davinci-i2s: Make free-running mode optional

[Bastien Curutchet]

McBSP has free-running mode where serial clocks continue to run during
emulation halts. This mode is always enabled by the driver.

Disable this free-running mode according to the 'ti,disable-free-run'
device-tree property.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 sound/soc/ti/davinci-i2s.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c
index 2c19e45b6b54..67307e75f2a8 100644
--- a/sound/soc/ti/davinci-i2s.c
+++ b/sound/soc/ti/davinci-i2s.c
@@ -165,6 +165,7 @@ struct davinci_mcbsp_dev {
 	int slot_width;
 
 	bool sync_err;
+	bool free_run;
 };
 
 static inline void davinci_mcbsp_write_reg(struct davinci_mcbsp_dev *dev,
@@ -444,11 +445,13 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
 
 	/* general line settings */
 	spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
+	if (dev->free_run)
+		spcr |= DAVINCI_MCBSP_SPCR_FREE;
 	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
-		spcr |= DAVINCI_MCBSP_SPCR_RINTM(3) | DAVINCI_MCBSP_SPCR_FREE;
+		spcr |= DAVINCI_MCBSP_SPCR_RINTM(3);
 		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
 	} else {
-		spcr |= DAVINCI_MCBSP_SPCR_XINTM(3) | DAVINCI_MCBSP_SPCR_FREE;
+		spcr |= DAVINCI_MCBSP_SPCR_XINTM(3);
 		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
 	}
 
@@ -766,6 +769,7 @@ static int davinci_i2s_probe(struct platform_device *pdev)
 
 	dev->base = io_base;
 
+	dev->free_run = !of_property_read_bool(pdev->dev.of_node, "ti,disable-free-run");
 	dev->sync_err = of_property_read_bool(pdev->dev.of_node, "ti,enable-sync-err");
 
 	/* setup DMA, first TX, then RX */
-- 
2.43.2

Re: [PATCH 10/13] ASoC: ti: davinci-i2s: Make free-running mode optional

[Mark Brown]

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

On Fri, Mar 15, 2024 at 12:27:42PM +0100, Bastien Curutchet wrote:
> McBSP has free-running mode where serial clocks continue to run during
> emulation halts. This mode is always enabled by the driver.
> 
> Disable this free-running mode according to the 'ti,disable-free-run'
> device-tree property.

This sounds like SND_SOC_DAIFMT_CONT rather than a DT property.

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

[PATCH 11/13] ASoC: ti: davinci-i2s: Add S24_LE to supported formats

[Bastien Curutchet]

S24_LE is supported by McBSP but not by the driver.

Add S24_LE to driver's supported formats. Using it enables the sign
extension in DRR (Data Receive Register). The other formats are kept
with the zero extension in DRR.

Remove data_type table as it is no longer used.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 sound/soc/ti/davinci-i2s.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c
index 67307e75f2a8..13e349e7a6ec 100644
--- a/sound/soc/ti/davinci-i2s.c
+++ b/sound/soc/ti/davinci-i2s.c
@@ -61,6 +61,9 @@
 
 #define DAVINCI_MCBSP_SPCR_RRST		(1 << 0)
 #define DAVINCI_MCBSP_SPCR_RINTM(v)	((v) << 4)
+#define DAVINCI_MCBSP_SPCR_RJUST(v)	((v) << 13)
+#define DAVINCI_MCBSP_SPCR_RJUST_Z_LE	DAVINCI_MCBSP_SPCR_RJUST(0)
+#define DAVINCI_MCBSP_SPCR_RJUST_S_LE	DAVINCI_MCBSP_SPCR_RJUST(1)
 #define DAVINCI_MCBSP_SPCR_XRST		(1 << 16)
 #define DAVINCI_MCBSP_SPCR_XINTM(v)	((v) << 20)
 #define DAVINCI_MCBSP_SPCR_GRST		(1 << 22)
@@ -107,15 +110,10 @@ enum {
 	DAVINCI_MCBSP_WORD_32,
 };
 
-static const unsigned char data_type[SNDRV_PCM_FORMAT_S32_LE + 1] = {
-	[SNDRV_PCM_FORMAT_S8]		= 1,
-	[SNDRV_PCM_FORMAT_S16_LE]	= 2,
-	[SNDRV_PCM_FORMAT_S32_LE]	= 4,
-};
-
 static const unsigned char asp_word_length[SNDRV_PCM_FORMAT_S32_LE + 1] = {
 	[SNDRV_PCM_FORMAT_S8]		= DAVINCI_MCBSP_WORD_8,
 	[SNDRV_PCM_FORMAT_S16_LE]	= DAVINCI_MCBSP_WORD_16,
+	[SNDRV_PCM_FORMAT_S24_LE]	= DAVINCI_MCBSP_WORD_24,
 	[SNDRV_PCM_FORMAT_S32_LE]	= DAVINCI_MCBSP_WORD_32,
 };
 
@@ -443,8 +441,23 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
 	snd_pcm_format_t fmt;
 	unsigned element_cnt = 1;
 
-	/* general line settings */
 	spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
+
+	/* Determine xfer data type */
+	fmt = params_format(params);
+	switch (fmt) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+	case SNDRV_PCM_FORMAT_S32_LE:
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		spcr |= DAVINCI_MCBSP_SPCR_RJUST_S_LE;
+		break;
+	default:
+		dev_warn(dev->dev, "davinci-i2s: unsupported PCM format\n");
+		return -EINVAL;
+	}
+
+	/* general line settings */
 	if (dev->free_run)
 		spcr |= DAVINCI_MCBSP_SPCR_FREE;
 	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
@@ -548,12 +561,6 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
 		rcr |= DAVINCI_MCBSP_RCR_RDATDLY(1);
 		xcr |= DAVINCI_MCBSP_XCR_XDATDLY(1);
 	}
-	/* Determine xfer data type */
-	fmt = params_format(params);
-	if ((fmt > SNDRV_PCM_FORMAT_S32_LE) || !data_type[fmt]) {
-		printk(KERN_WARNING "davinci-i2s: unsupported PCM format\n");
-		return -EINVAL;
-	}
 
 	if (params_channels(params) == 2) {
 		element_cnt = 2;
@@ -692,6 +699,7 @@ static void davinci_i2s_shutdown(struct snd_pcm_substream *substream,
 
 #define DAVINCI_I2S_RATES	SNDRV_PCM_RATE_8000_96000
 #define DAVINCI_I2S_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | \
+				 SNDRV_PCM_FMTBIT_S24_LE | \
 				 SNDRV_PCM_FMTBIT_S32_LE)
 
 static int davinci_i2s_dai_probe(struct snd_soc_dai *dai)
-- 
2.43.2

[PATCH 12/13] ASoC: dt-bindings: davinic-mcbsp: Add the 'ti,drive-dx' property

[Bastien Curutchet]

McBSP is able to handle capture and playback stream.
The McBSP's DX pins that outputs serial data during playback streams can
be used to output a chosen pattern during capture streams. For instance,
this can be useful to drive an active-low signal during capture streams
(by choosing <0> as pattern)

Add a 'ti,drive-dx' property that can be used to repeatedly output a
chosen pattern on DX pin while capture stream is ON.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml b/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml
index d8d4e7ea6e02..f4d1fc6bcd61 100644
--- a/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml
+++ b/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml
@@ -80,6 +80,13 @@ properties:
       Enable synchronisation error detections when an unexpected frame pulse is
       received. If not present, unexpected frame pulses are ignored.
 
+  ti,drive-dx:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      If the property is present, McBSP will repeatedly output the selected
+      value on DX pin during capture streams. For instance, if set to 0, this
+      can be used to drive an active-low signal.
+
 required:
   - "#sound-dai-cells"
   - compatible
-- 
2.43.2

Re: [PATCH 12/13] ASoC: dt-bindings: davinic-mcbsp: Add the 'ti,drive-dx' property

[Rob Herring]

On Fri, 15 Mar 2024 12:27:44 +0100, Bastien Curutchet wrote:
> McBSP is able to handle capture and playback stream.
> The McBSP's DX pins that outputs serial data during playback streams can
> be used to output a chosen pattern during capture streams. For instance,
> this can be useful to drive an active-low signal during capture streams
> (by choosing <0> as pattern)
> 
> Add a 'ti,drive-dx' property that can be used to repeatedly output a
> chosen pattern on DX pin while capture stream is ON.
> 
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
>  Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

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

Re: [PATCH 12/13] ASoC: dt-bindings: davinic-mcbsp: Add the 'ti,drive-dx' property

[Péter Ujfalusi]

On 15/03/2024 13:27, Bastien Curutchet wrote:
> McBSP is able to handle capture and playback stream.
> The McBSP's DX pins that outputs serial data during playback streams can
> be used to output a chosen pattern during capture streams. For instance,
> this can be useful to drive an active-low signal during capture streams
> (by choosing <0> as pattern)

or configure the MCBSPx.DX pin as GPO and use it as a GPIO?

Quite novel use of the hardware, no doubt about it. If you don't have
DMA servicing the TX, it will just re-transmit the word from from the
DXR register when the transmitter is pulled out of reset.

Interesting, but I'm not sure if this belongs to DT.

> Add a 'ti,drive-dx' property that can be used to repeatedly output a
> chosen pattern on DX pin while capture stream is ON.
> 
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
>  Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml b/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml
> index d8d4e7ea6e02..f4d1fc6bcd61 100644
> --- a/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml
> +++ b/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml
> @@ -80,6 +80,13 @@ properties:
>        Enable synchronisation error detections when an unexpected frame pulse is
>        received. If not present, unexpected frame pulses are ignored.
>  
> +  ti,drive-dx:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      If the property is present, McBSP will repeatedly output the selected
> +      value on DX pin during capture streams. For instance, if set to 0, this
> +      can be used to drive an active-low signal.
> +
>  required:
>    - "#sound-dai-cells"
>    - compatible

-- 
Péter

Re: [PATCH 12/13] ASoC: dt-bindings: davinic-mcbsp: Add the 'ti,drive-dx' property

[Bastien Curutchet]

Hi Péter,


On 3/19/24 19:02, Péter Ujfalusi wrote:
> 
> 
> On 15/03/2024 13:27, Bastien Curutchet wrote:
>> McBSP is able to handle capture and playback stream.
>> The McBSP's DX pins that outputs serial data during playback streams can
>> be used to output a chosen pattern during capture streams. For instance,
>> this can be useful to drive an active-low signal during capture streams
>> (by choosing <0> as pattern)
> 
> or configure the MCBSPx.DX pin as GPO and use it as a GPIO?

In my use case, DX pin is connected to the ADC chip select pin so I want
the DX pin to toggle the closest possible to capture's start. That's
why I introduced this feature over configuring the pin as GPO.

> 
> Quite novel use of the hardware, no doubt about it. If you don't have
> DMA servicing the TX, it will just re-transmit the word from from the
> DXR register when the transmitter is pulled out of reset.
> 
> Interesting, but I'm not sure if this belongs to DT.
> 
>> Add a 'ti,drive-dx' property that can be used to repeatedly output a
>> chosen pattern on DX pin while capture stream is ON.
>>
>> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
>> ---
>>   Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml b/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml
>> index d8d4e7ea6e02..f4d1fc6bcd61 100644
>> --- a/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml
>> +++ b/Documentation/devicetree/bindings/sound/davinci-mcbsp.yaml
>> @@ -80,6 +80,13 @@ properties:
>>         Enable synchronisation error detections when an unexpected frame pulse is
>>         received. If not present, unexpected frame pulses are ignored.
>>   
>> +  ti,drive-dx:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      If the property is present, McBSP will repeatedly output the selected
>> +      value on DX pin during capture streams. For instance, if set to 0, this
>> +      can be used to drive an active-low signal.
>> +
>>   required:
>>     - "#sound-dai-cells"
>>     - compatible
> 

Best regards,
Bastien

[PATCH 13/13] ASoC: ti: davinci-i2s: Opitonally drive DX pin during capture streams

[Bastien Curutchet]

The McBSP's DX pin that outputs serial data during playback streams can
be used during capture streams to repeatedly output a chosen pattern.
For instance, this can be useful to drive an active-low signal during
captures (by choosing <0> as output pattern).

Enable this behaviour when the device-tree property 'ti,drive-dx' is
present. DX pin is driven with the provided pattern every time a
capture stream is launched.

This property is not compatible with classic playback stream so
davinci_i2s_trigger() returns an error if a playback stream is started
while 'ti,drive-dx' flag is present.

This has been tested on a board designed of a DAVINCI/OMAP-L138 where
the DX pin is linked to the chip select pin of the converters of the
capture side.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 sound/soc/ti/davinci-i2s.c | 74 ++++++++++++++++++++++++++++++++------
 1 file changed, 63 insertions(+), 11 deletions(-)

diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c
index 13e349e7a6ec..e289a84bdd6a 100644
--- a/sound/soc/ti/davinci-i2s.c
+++ b/sound/soc/ti/davinci-i2s.c
@@ -101,6 +101,9 @@
 #define DAVINCI_MCBSP_PCR_FSRM		(1 << 10)
 #define DAVINCI_MCBSP_PCR_FSXM		(1 << 11)
 
+#define PLAYBACK_CLOCK			1
+#define CAPTURE_CLOCK			0
+
 enum {
 	DAVINCI_MCBSP_WORD_8 = 0,
 	DAVINCI_MCBSP_WORD_12,
@@ -164,6 +167,8 @@ struct davinci_mcbsp_dev {
 
 	bool sync_err;
 	bool free_run;
+	bool drive_dx;
+	u32 dx_val;
 };
 
 static inline void davinci_mcbsp_write_reg(struct davinci_mcbsp_dev *dev,
@@ -187,6 +192,19 @@ static void toggle_clock(struct davinci_mcbsp_dev *dev, int playback)
 	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_PCR_REG, dev->pcr);
 }
 
+static int davinci_drive_dx(struct davinci_mcbsp_dev *dev)
+{
+	unsigned int spcr;
+
+	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_DXR_REG, dev->dx_val);
+
+	spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
+	spcr |= DAVINCI_MCBSP_SPCR_XRST;
+	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
+
+	return 0;
+}
+
 static void davinci_mcbsp_start(struct davinci_mcbsp_dev *dev,
 		struct snd_pcm_substream *substream)
 {
@@ -194,6 +212,9 @@ static void davinci_mcbsp_start(struct davinci_mcbsp_dev *dev,
 	u32 spcr;
 	u32 mask = playback ? DAVINCI_MCBSP_SPCR_XRST : DAVINCI_MCBSP_SPCR_RRST;
 
+	if (!playback && dev->drive_dx)
+		davinci_drive_dx(dev);
+
 	/* Enable transmitter or receiver */
 	spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
 	spcr |= mask;
@@ -212,9 +233,17 @@ static void davinci_mcbsp_stop(struct davinci_mcbsp_dev *dev, int playback)
 	/* Reset transmitter/receiver and sample rate/frame sync generators */
 	spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
 	spcr &= ~(DAVINCI_MCBSP_SPCR_GRST | DAVINCI_MCBSP_SPCR_FRST);
-	spcr &= playback ? ~DAVINCI_MCBSP_SPCR_XRST : ~DAVINCI_MCBSP_SPCR_RRST;
-	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
-	toggle_clock(dev, playback);
+
+	if (!playback) {
+		spcr &= ~DAVINCI_MCBSP_SPCR_RRST;
+		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
+		toggle_clock(dev, CAPTURE_CLOCK);
+	}
+	if (playback || dev->drive_dx) {
+		spcr &= ~DAVINCI_MCBSP_SPCR_XRST;
+		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
+		toggle_clock(dev, PLAYBACK_CLOCK);
+	}
 }
 
 static int davinci_i2s_tdm_word_length(int tdm_slot_width)
@@ -408,6 +437,10 @@ static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 	}
 	if (inv_fs == true)
 		pcr ^= (DAVINCI_MCBSP_PCR_FSXP | DAVINCI_MCBSP_PCR_FSRP);
+
+	if (dev->drive_dx)
+		pcr |= DAVINCI_MCBSP_PCR_CLKRP;
+
 	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SRGR_REG, srgr);
 	dev->pcr = pcr;
 	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_PCR_REG, pcr);
@@ -562,6 +595,9 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
 		xcr |= DAVINCI_MCBSP_XCR_XDATDLY(1);
 	}
 
+	if (dev->drive_dx)
+		xcr |= DAVINCI_MCBSP_XCR_XDATDLY(2);
+
 	if (params_channels(params) == 2) {
 		element_cnt = 2;
 		if (double_fmt[fmt] && dev->enable_channel_combine) {
@@ -611,9 +647,9 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
 	xcr |= DAVINCI_MCBSP_XCR_XWDLEN1(mcbsp_word_length) |
 		DAVINCI_MCBSP_XCR_XWDLEN2(mcbsp_word_length);
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK || dev->drive_dx)
 		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_XCR_REG, xcr);
-	else
+	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
 		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_RCR_REG, rcr);
 
 	pr_debug("%s - %d  srgr=%X\n", __func__, __LINE__, srgr);
@@ -628,16 +664,21 @@ static int davinci_i2s_prepare(struct snd_pcm_substream *substream,
 	struct davinci_mcbsp_dev *dev = snd_soc_dai_get_drvdata(dai);
 	int playback = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK);
 	u32 spcr;
-	u32 mask = playback ? DAVINCI_MCBSP_SPCR_XRST : DAVINCI_MCBSP_SPCR_RRST;
 
 	davinci_mcbsp_stop(dev, playback);
 
 	spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
-	if (spcr & mask) {
+	if (spcr & DAVINCI_MCBSP_SPCR_XRST) {
 		/* start off disabled */
 		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG,
-					spcr & ~mask);
-		toggle_clock(dev, playback);
+					spcr & ~DAVINCI_MCBSP_SPCR_XRST);
+		toggle_clock(dev, PLAYBACK_CLOCK);
+	}
+	if (spcr & DAVINCI_MCBSP_SPCR_RRST) {
+		/* start off disabled */
+		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG,
+					spcr & ~DAVINCI_MCBSP_SPCR_RRST);
+		toggle_clock(dev, CAPTURE_CLOCK);
 	}
 	if (dev->pcr & (DAVINCI_MCBSP_PCR_FSXM | DAVINCI_MCBSP_PCR_FSRM |
 			DAVINCI_MCBSP_PCR_CLKXM | DAVINCI_MCBSP_PCR_CLKRM)) {
@@ -646,7 +687,7 @@ static int davinci_i2s_prepare(struct snd_pcm_substream *substream,
 		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
 	}
 
-	if (playback) {
+	if (playback || dev->drive_dx) {
 		/* Enable the transmitter */
 		spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
 		spcr |= DAVINCI_MCBSP_SPCR_XRST;
@@ -659,7 +700,7 @@ static int davinci_i2s_prepare(struct snd_pcm_substream *substream,
 		spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
 		spcr &= ~DAVINCI_MCBSP_SPCR_XRST;
 		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
-		toggle_clock(dev, playback);
+		toggle_clock(dev, PLAYBACK_CLOCK);
 	}
 
 	return 0;
@@ -672,6 +713,11 @@ static int davinci_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 	int ret = 0;
 	int playback = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK);
 
+	if (playback && dev->drive_dx) {
+		dev_err(dev->dev, "Playback is not allowed when drive-cs flag is set\n");
+		return -EINVAL;
+	}
+
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
@@ -779,6 +825,12 @@ static int davinci_i2s_probe(struct platform_device *pdev)
 
 	dev->free_run = !of_property_read_bool(pdev->dev.of_node, "ti,disable-free-run");
 	dev->sync_err = of_property_read_bool(pdev->dev.of_node, "ti,enable-sync-err");
+	dev->drive_dx = false;
+	ret = of_property_read_u32(pdev->dev.of_node, "ti,drive-dx", &dev->dx_val);
+	if (ret && ret != -EINVAL)
+		return ret;
+	if (!ret)
+		dev->drive_dx = true;
 
 	/* setup DMA, first TX, then RX */
 	dma_data = &dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK];
-- 
2.43.2

Re: [PATCH 13/13] ASoC: ti: davinci-i2s: Opitonally drive DX pin during capture streams

[Péter Ujfalusi]

On 15/03/2024 13:27, Bastien Curutchet wrote:
> The McBSP's DX pin that outputs serial data during playback streams can
> be used during capture streams to repeatedly output a chosen pattern.
> For instance, this can be useful to drive an active-low signal during
> captures (by choosing <0> as output pattern).

Are there really any other use of this than to pull down or up the DX
pin (0 or 0xffff)?

If you just use the pin as GPIO then you don't need to change anything
in the driver, The playback would not erach the pin, so no need to block it.

> Enable this behaviour when the device-tree property 'ti,drive-dx' is
> present. DX pin is driven with the provided pattern every time a
> capture stream is launched.

It is an interesting use of the hardware... You are controlling an
external device (light an LED when capture is on)?

> This property is not compatible with classic playback stream so
> davinci_i2s_trigger() returns an error if a playback stream is started
> while 'ti,drive-dx' flag is present.

Propbaly add the .startup() callback and block the playback right there?

> 
> This has been tested on a board designed of a DAVINCI/OMAP-L138 where
> the DX pin is linked to the chip select pin of the converters of the
> capture side.

Isn't the DX will be pulled down as soon as the McBSP is enabled?
Can you just re-configure the PUPD_SEL for the pin group to make the pin
to be pulled the other way?

> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
>  sound/soc/ti/davinci-i2s.c | 74 ++++++++++++++++++++++++++++++++------
>  1 file changed, 63 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c
> index 13e349e7a6ec..e289a84bdd6a 100644
> --- a/sound/soc/ti/davinci-i2s.c
> +++ b/sound/soc/ti/davinci-i2s.c
> @@ -101,6 +101,9 @@
>  #define DAVINCI_MCBSP_PCR_FSRM		(1 << 10)
>  #define DAVINCI_MCBSP_PCR_FSXM		(1 << 11)
>  
> +#define PLAYBACK_CLOCK			1
> +#define CAPTURE_CLOCK			0
> +
>  enum {
>  	DAVINCI_MCBSP_WORD_8 = 0,
>  	DAVINCI_MCBSP_WORD_12,
> @@ -164,6 +167,8 @@ struct davinci_mcbsp_dev {
>  
>  	bool sync_err;
>  	bool free_run;
> +	bool drive_dx;
> +	u32 dx_val;
>  };
>  
>  static inline void davinci_mcbsp_write_reg(struct davinci_mcbsp_dev *dev,
> @@ -187,6 +192,19 @@ static void toggle_clock(struct davinci_mcbsp_dev *dev, int playback)
>  	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_PCR_REG, dev->pcr);
>  }
>  
> +static int davinci_drive_dx(struct davinci_mcbsp_dev *dev)
> +{
> +	unsigned int spcr;
> +
> +	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_DXR_REG, dev->dx_val);
> +
> +	spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
> +	spcr |= DAVINCI_MCBSP_SPCR_XRST;
> +	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);



> +
> +	return 0;
> +}
> +
>  static void davinci_mcbsp_start(struct davinci_mcbsp_dev *dev,
>  		struct snd_pcm_substream *substream)
>  {
> @@ -194,6 +212,9 @@ static void davinci_mcbsp_start(struct davinci_mcbsp_dev *dev,
>  	u32 spcr;
>  	u32 mask = playback ? DAVINCI_MCBSP_SPCR_XRST : DAVINCI_MCBSP_SPCR_RRST;
>  
> +	if (!playback && dev->drive_dx)
> +		davinci_drive_dx(dev);
> +
>  	/* Enable transmitter or receiver */
>  	spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
>  	spcr |= mask;

if (dev->drive_dx) {
	spcr |= DAVINCI_MCBSP_SPCR_XRST;
	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_DXR_REG, dev->dx_val);
}

and no need for the davinci_drive_dx() function, plus it makes it
symmetric of what you do on stop()

> @@ -212,9 +233,17 @@ static void davinci_mcbsp_stop(struct davinci_mcbsp_dev *dev, int playback)
>  	/* Reset transmitter/receiver and sample rate/frame sync generators */
>  	spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
>  	spcr &= ~(DAVINCI_MCBSP_SPCR_GRST | DAVINCI_MCBSP_SPCR_FRST);
> -	spcr &= playback ? ~DAVINCI_MCBSP_SPCR_XRST : ~DAVINCI_MCBSP_SPCR_RRST;
> -	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
> -	toggle_clock(dev, playback);
> +
> +	if (!playback) {
> +		spcr &= ~DAVINCI_MCBSP_SPCR_RRST;
> +		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
> +		toggle_clock(dev, CAPTURE_CLOCK);
> +	}
> +	if (playback || dev->drive_dx) {
> +		spcr &= ~DAVINCI_MCBSP_SPCR_XRST;
> +		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
> +		toggle_clock(dev, PLAYBACK_CLOCK);
> +	}
>  }
>  
>  static int davinci_i2s_tdm_word_length(int tdm_slot_width)
> @@ -408,6 +437,10 @@ static int davinci_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai,
>  	}
>  	if (inv_fs == true)
>  		pcr ^= (DAVINCI_MCBSP_PCR_FSXP | DAVINCI_MCBSP_PCR_FSRP);
> +
> +	if (dev->drive_dx)
> +		pcr |= DAVINCI_MCBSP_PCR_CLKRP;
> +
>  	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SRGR_REG, srgr);
>  	dev->pcr = pcr;
>  	davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_PCR_REG, pcr);
> @@ -562,6 +595,9 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
>  		xcr |= DAVINCI_MCBSP_XCR_XDATDLY(1);
>  	}
>  
> +	if (dev->drive_dx)
> +		xcr |= DAVINCI_MCBSP_XCR_XDATDLY(2);
> +
>  	if (params_channels(params) == 2) {
>  		element_cnt = 2;
>  		if (double_fmt[fmt] && dev->enable_channel_combine) {
> @@ -611,9 +647,9 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream,
>  	xcr |= DAVINCI_MCBSP_XCR_XWDLEN1(mcbsp_word_length) |
>  		DAVINCI_MCBSP_XCR_XWDLEN2(mcbsp_word_length);
>  
> -	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK || dev->drive_dx)
>  		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_XCR_REG, xcr);
> -	else
> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
>  		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_RCR_REG, rcr);
>  
>  	pr_debug("%s - %d  srgr=%X\n", __func__, __LINE__, srgr);
> @@ -628,16 +664,21 @@ static int davinci_i2s_prepare(struct snd_pcm_substream *substream,
>  	struct davinci_mcbsp_dev *dev = snd_soc_dai_get_drvdata(dai);
>  	int playback = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK);
>  	u32 spcr;
> -	u32 mask = playback ? DAVINCI_MCBSP_SPCR_XRST : DAVINCI_MCBSP_SPCR_RRST;
>  
>  	davinci_mcbsp_stop(dev, playback);
>  
>  	spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
> -	if (spcr & mask) {
> +	if (spcr & DAVINCI_MCBSP_SPCR_XRST) {
>  		/* start off disabled */
>  		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG,
> -					spcr & ~mask);
> -		toggle_clock(dev, playback);
> +					spcr & ~DAVINCI_MCBSP_SPCR_XRST);
> +		toggle_clock(dev, PLAYBACK_CLOCK);
> +	}
> +	if (spcr & DAVINCI_MCBSP_SPCR_RRST) {
> +		/* start off disabled */
> +		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG,
> +					spcr & ~DAVINCI_MCBSP_SPCR_RRST);
> +		toggle_clock(dev, CAPTURE_CLOCK);
>  	}
>  	if (dev->pcr & (DAVINCI_MCBSP_PCR_FSXM | DAVINCI_MCBSP_PCR_FSRM |
>  			DAVINCI_MCBSP_PCR_CLKXM | DAVINCI_MCBSP_PCR_CLKRM)) {
> @@ -646,7 +687,7 @@ static int davinci_i2s_prepare(struct snd_pcm_substream *substream,
>  		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
>  	}
>  
> -	if (playback) {
> +	if (playback || dev->drive_dx) {
>  		/* Enable the transmitter */
>  		spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
>  		spcr |= DAVINCI_MCBSP_SPCR_XRST;
> @@ -659,7 +700,7 @@ static int davinci_i2s_prepare(struct snd_pcm_substream *substream,
>  		spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG);
>  		spcr &= ~DAVINCI_MCBSP_SPCR_XRST;
>  		davinci_mcbsp_write_reg(dev, DAVINCI_MCBSP_SPCR_REG, spcr);
> -		toggle_clock(dev, playback);
> +		toggle_clock(dev, PLAYBACK_CLOCK);
>  	}
>  
>  	return 0;
> @@ -672,6 +713,11 @@ static int davinci_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
>  	int ret = 0;
>  	int playback = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK);
>  
> +	if (playback && dev->drive_dx) {
> +		dev_err(dev->dev, "Playback is not allowed when drive-cs flag is set\n");
> +		return -EINVAL;
> +	}
> +
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
>  	case SNDRV_PCM_TRIGGER_RESUME:
> @@ -779,6 +825,12 @@ static int davinci_i2s_probe(struct platform_device *pdev)
>  
>  	dev->free_run = !of_property_read_bool(pdev->dev.of_node, "ti,disable-free-run");
>  	dev->sync_err = of_property_read_bool(pdev->dev.of_node, "ti,enable-sync-err");
> +	dev->drive_dx = false;

no need to initialize it to 0, dev is allocated with devm_kzalloc()

> +	ret = of_property_read_u32(pdev->dev.of_node, "ti,drive-dx", &dev->dx_val);
> +	if (ret && ret != -EINVAL)
> +		return ret;
> +	if (!ret)
> +		dev->drive_dx = true;

if (!ret)
	dev->drive_dx = true;
else if (ret != -EINVAL)
	return ret;

>  
>  	/* setup DMA, first TX, then RX */
>  	dma_data = &dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK];

-- 
Péter

Re: [PATCH 13/13] ASoC: ti: davinci-i2s: Opitonally drive DX pin during capture streams

[Bastien Curutchet]

Hi Péter,

On 3/19/24 19:29, Péter Ujfalusi wrote:
> 
> 
> On 15/03/2024 13:27, Bastien Curutchet wrote:
>> The McBSP's DX pin that outputs serial data during playback streams can
>> be used during capture streams to repeatedly output a chosen pattern.
>> For instance, this can be useful to drive an active-low signal during
>> captures (by choosing <0> as output pattern).
> 
> Are there really any other use of this than to pull down or up the DX
> pin (0 or 0xffff)
I don't know, indeed today I can only think about these two patterns.
I tried to do something in a 'generic' way so it can evolve if needed.

> If you just use the pin as GPIO then you don't need to change anything
> in the driver, The playback would not erach the pin, so no need to block it.
> 
>> Enable this behaviour when the device-tree property 'ti,drive-dx' is
>> present. DX pin is driven with the provided pattern every time a
>> capture stream is launched.
> 
> It is an interesting use of the hardware... You are controlling an
> external device (light an LED when capture is on)?

Yes I control the chip select pin of the ADC that is sending data to DR 
pin, that's why I need the DX pin to be synchronized with capture
streams.

>> This property is not compatible with classic playback stream so
>> davinci_i2s_trigger() returns an error if a playback stream is started
>> while 'ti,drive-dx' flag is present.
> 
> Propbaly add the .startup() callback and block the playback right there?
> 

Ok, TBH my mastery of the sound subsystem is not high enough to have an
opinion of where this should go so I'll trust you on this.

>>
>> This has been tested on a board designed of a DAVINCI/OMAP-L138 where
>> the DX pin is linked to the chip select pin of the converters of the
>> capture side.
> 
> Isn't the DX will be pulled down as soon as the McBSP is enabled?
> Can you just re-configure the PUPD_SEL for the pin group to make the pin
> to be pulled the other way?
> 

Well, the acquisition chain in my use case is a bit convoluted. The DX
pin's main purpose is to drive ADC chip select but it is also connected
to other components and all this needs synchronization upon captures.


I'll integrate your feedback about the code in next iteration, thank
you.


Best regards,
Bastien

Re: [PATCH 13/13] ASoC: ti: davinci-i2s: Opitonally drive DX pin during capture streams

[Péter Ujfalusi]

Hi Bastien,

On 20/03/2024 10:52, Bastien Curutchet wrote:
> Hi Péter,
> 
> On 3/19/24 19:29, Péter Ujfalusi wrote:
>>
>>
>> On 15/03/2024 13:27, Bastien Curutchet wrote:
>>> The McBSP's DX pin that outputs serial data during playback streams can
>>> be used during capture streams to repeatedly output a chosen pattern.
>>> For instance, this can be useful to drive an active-low signal during
>>> captures (by choosing <0> as output pattern).
>>
>> Are there really any other use of this than to pull down or up the DX
>> pin (0 or 0xffff)
>
> I don't know, indeed today I can only think about these two patterns.
> I tried to do something in a 'generic' way so it can evolve if needed.

I think the definition of the 'ti,drive-dx' is somehow odd. It allows
you to set it to 0x1234 and the DX pin will show 0x1234 when you capture
32bit. If you capture 16bit then it will transmit 0x12 (or 0x34?), no?
If you have 4 channel capture then I won't speculate what will be on the
DX pin ;)

Would not be better to say that the DX pin will be driven low or high
during capture _and_ disable the playback support?

> 
>> If you just use the pin as GPIO then you don't need to change anything
>> in the driver, The playback would not erach the pin, so no need to
>> block it.
>>
>>> Enable this behaviour when the device-tree property 'ti,drive-dx' is
>>> present. DX pin is driven with the provided pattern every time a
>>> capture stream is launched.
>>
>> It is an interesting use of the hardware... You are controlling an
>> external device (light an LED when capture is on)?
> 
> Yes I control the chip select pin of the ADC that is sending data to DR
> pin, that's why I need the DX pin to be synchronized with capture
> streams.

I see. Still a a novel use of a feature ;)

> 
>>> This property is not compatible with classic playback stream so
>>> davinci_i2s_trigger() returns an error if a playback stream is started
>>> while 'ti,drive-dx' flag is present.
>>
>> Propbaly add the .startup() callback and block the playback right there?
>>
> 
> Ok, TBH my mastery of the sound subsystem is not high enough to have an
> opinion of where this should go so I'll trust you on this.

It would be more elegant to only create PCM for the capture in this
case, but I would not bother with it.
Stopping user right at startup time is second better.

>>>
>>> This has been tested on a board designed of a DAVINCI/OMAP-L138 where
>>> the DX pin is linked to the chip select pin of the converters of the
>>> capture side.
>>
>> Isn't the DX will be pulled down as soon as the McBSP is enabled?
>> Can you just re-configure the PUPD_SEL for the pin group to make the pin
>> to be pulled the other way?
>>
> 
> Well, the acquisition chain in my use case is a bit convoluted. The DX
> pin's main purpose is to drive ADC chip select but it is also connected
> to other components and all this needs synchronization upon captures.

OK, thanks for the explanation.

-- 
Péter

Re: [PATCH 13/13] ASoC: ti: davinci-i2s: Opitonally drive DX pin during capture streams

[Péter Ujfalusi]

On 20/03/2024 17:42, Péter Ujfalusi wrote:
>>> On 15/03/2024 13:27, Bastien Curutchet wrote:
>>>> The McBSP's DX pin that outputs serial data during playback streams can
>>>> be used during capture streams to repeatedly output a chosen pattern.
>>>> For instance, this can be useful to drive an active-low signal during
>>>> captures (by choosing <0> as output pattern).
>>>
>>> Are there really any other use of this than to pull down or up the DX
>>> pin (0 or 0xffff)
>>
>> I don't know, indeed today I can only think about these two patterns.
>> I tried to do something in a 'generic' way so it can evolve if needed.
> 
> I think the definition of the 'ti,drive-dx' is somehow odd. It allows
> you to set it to 0x1234 and the DX pin will show 0x1234 when you capture
> 32bit. If you capture 16bit then it will transmit 0x12 (or 0x34?), no?
> If you have 4 channel capture then I won't speculate what will be on the
> DX pin ;)
> 
> Would not be better to say that the DX pin will be driven low or high
> during capture _and_ disable the playback support?

After some thinking, it might be still better to use the DX pin as GPIO
and either have a custom machine driver which would handle it (set low
when a capture trigger happens) or connect it in DAPM as a supply, bias
or something and ASoC would handle it automagically.

I think that would be cleaner in many ways. What do you think?

-- 
Péter

Re: [PATCH 13/13] ASoC: ti: davinci-i2s: Opitonally drive DX pin during capture streams

[Bastien Curutchet]

Hi Péter,

On 3/20/24 21:30, Péter Ujfalusi wrote:
> 
> 
> On 20/03/2024 17:42, Péter Ujfalusi wrote:
>>>> On 15/03/2024 13:27, Bastien Curutchet wrote:
>>>>> The McBSP's DX pin that outputs serial data during playback streams can
>>>>> be used during capture streams to repeatedly output a chosen pattern.
>>>>> For instance, this can be useful to drive an active-low signal during
>>>>> captures (by choosing <0> as output pattern).
>>>>
>>>> Are there really any other use of this than to pull down or up the DX
>>>> pin (0 or 0xffff)
>>>
>>> I don't know, indeed today I can only think about these two patterns.
>>> I tried to do something in a 'generic' way so it can evolve if needed.
>>
>> I think the definition of the 'ti,drive-dx' is somehow odd. It allows
>> you to set it to 0x1234 and the DX pin will show 0x1234 when you capture
>> 32bit. If you capture 16bit then it will transmit 0x12 (or 0x34?), no?
>> If you have 4 channel capture then I won't speculate what will be on the
>> DX pin ;)
>>
>> Would not be better to say that the DX pin will be driven low or high
>> during capture _and_ disable the playback support?
> 
> After some thinking, it might be still better to use the DX pin as GPIO
> and either have a custom machine driver which would handle it (set low
> when a capture trigger happens) or connect it in DAPM as a supply, bias
> or something and ASoC would handle it automagically.
> 
> I think that would be cleaner in many ways. What do you think?
> 
I agree, that would be cleaner. I ran a few tests to see if that would
work on my hardware. It doesn't ... So I looked back to the schematics
and found two reasons :
  * the DX pin needs to be in sync with the clock.
  * the DX pin needs to be in a high-impedance state between two frames
    so a pull-up can drive it back up. Actually, the DX pin is also
    linked to the FSR pin so it provides the frame clock to the capture
    stream.

Bast regards,
Bastien

Re: [PATCH 13/13] ASoC: ti: davinci-i2s: Opitonally drive DX pin during capture streams

[Péter Ujfalusi]

Hi Bastien,

On 3/21/24 17:14, Bastien Curutchet wrote:
>>> I think the definition of the 'ti,drive-dx' is somehow odd. It allows
>>> you to set it to 0x1234 and the DX pin will show 0x1234 when you capture
>>> 32bit. If you capture 16bit then it will transmit 0x12 (or 0x34?), no?
>>> If you have 4 channel capture then I won't speculate what will be on the
>>> DX pin ;)
>>>
>>> Would not be better to say that the DX pin will be driven low or high
>>> during capture _and_ disable the playback support?
>>
>> After some thinking, it might be still better to use the DX pin as GPIO
>> and either have a custom machine driver which would handle it (set low
>> when a capture trigger happens) or connect it in DAPM as a supply, bias
>> or something and ASoC would handle it automagically.
>>
>> I think that would be cleaner in many ways. What do you think?
>>
> I agree, that would be cleaner. I ran a few tests to see if that would
> work on my hardware. It doesn't ... So I looked back to the schematics
> and found two reasons :
>  * the DX pin needs to be in sync with the clock.

I'm not sure what this means, sync with which clock?

>  * the DX pin needs to be in a high-impedance state between two frames
>    so a pull-up can drive it back up. Actually, the DX pin is also
>    linked to the FSR pin so it provides the frame clock to the capture
>    stream.

Hrm, you are using the DX pin as FSR for the capture? Why not McBSP.FSR pin?

Looking back to the patch, one thing stood out: you are setting the
XDATDLY to 2.
You have some sort of T1 framing on the bus? The pullup will make the DX
line high in for the framing bit, right?
Or you simulate another FSR line with T1 framing DX?

The 'ti,drive-dx' sounds like a bad property for sure, you have T1
framing and driving the DX to certain level.
It is like DSP_A (1 bit delay) playing constant 0x2 ?

Can you use aplay /dev/zero and a DT property to select T1 framing for
the playback? Or that would be too coarse for timing the start of
playback and capture?

-- 
Péter

Re: [PATCH 13/13] ASoC: ti: davinci-i2s: Opitonally drive DX pin during capture streams

[Bastien Curutchet]

Hi Péter,

On 3/21/24 19:31, Péter Ujfalusi wrote:
> Hi Bastien,
> 
> On 3/21/24 17:14, Bastien Curutchet wrote:
>>>> I think the definition of the 'ti,drive-dx' is somehow odd. It allows
>>>> you to set it to 0x1234 and the DX pin will show 0x1234 when you capture
>>>> 32bit. If you capture 16bit then it will transmit 0x12 (or 0x34?), no?
>>>> If you have 4 channel capture then I won't speculate what will be on the
>>>> DX pin ;)
>>>>
>>>> Would not be better to say that the DX pin will be driven low or high
>>>> during capture _and_ disable the playback support?
>>>
>>> After some thinking, it might be still better to use the DX pin as GPIO
>>> and either have a custom machine driver which would handle it (set low
>>> when a capture trigger happens) or connect it in DAPM as a supply, bias
>>> or something and ASoC would handle it automagically.
>>>
>>> I think that would be cleaner in many ways. What do you think?
>>>
>> I agree, that would be cleaner. I ran a few tests to see if that would
>> work on my hardware. It doesn't ... So I looked back to the schematics
>> and found two reasons :
>>   * the DX pin needs to be in sync with the clock.
> 
> I'm not sure what this means, sync with which clock?
> 

Sorry, that was not very clear, I meant sync with the bit block that is
output on McBSP.CLKR pin.

>>   * the DX pin needs to be in a high-impedance state between two frames
>>     so a pull-up can drive it back up. Actually, the DX pin is also
>>     linked to the FSR pin so it provides the frame clock to the capture
>>     stream.
> 
> Hrm, you are using the DX pin as FSR for the capture? Why not McBSP.FSR pin >

The McBSP.FSR pin is used for the capture but is driven by the McBSP.DX
pin. Both pins are linked together.

> Looking back to the patch, one thing stood out: you are setting the
> XDATDLY to 2.
> You have some sort of T1 framing on the bus? The pullup will make the DX
> line high in for the framing bit, right? > Or you simulate another FSR line with T1 framing DX?
> 

Yes the goal is to simulate an FSR.

> The 'ti,drive-dx' sounds like a bad property for sure, you have T1
> framing and driving the DX to certain level.
> It is like DSP_A (1 bit delay) playing constant 0x2 ?
> 
> Can you use aplay /dev/zero and a DT property to select T1 framing for
> the playback? Or that would be too coarse for timing the start of
> playback and capture?
> 

That's a good idea, thank you. I'll try this and come back to you.


Best regards,
Bastien

Re: [PATCH 13/13] ASoC: ti: davinci-i2s: Opitonally drive DX pin during capture streams

[Bastien Curutchet]

Hi Péter,

>> Can you use aplay /dev/zero and a DT property to select T1 framing for
>> the playback? Or that would be too coarse for timing the start of
>> playback and capture?
>>
> 
> That's a good idea, thank you. I'll try this and come back to you.
> 
>

I tried it and it works fine, thank you.

We still need to run some performance tests before fully adopting it but
anyway I'll send a new iteration of the patch series that drops the
drive-dx part and just keeps a DT property to select T1 framing.

Best regards,
Bastien