mbox series

[0/6] ASoC: samsung: fsd: audio support for FSD SoC

Message ID 20221014102151.108539-1-p.rajanbabu@samsung.com
Headers show
Series ASoC: samsung: fsd: audio support for FSD SoC | expand

Message

Padmanabhan Rajanbabu Oct. 14, 2022, 10:21 a.m. UTC
The intention of this patch series is to enable audio support on FSD SoC.

The following features are added in samsung I2S interface:
1. Add TDM support on samsung I2S interface
2. Allow sound card to directly configure prescaler divider instead of
calculating it from frame clock

Also the sound card support for FSD SoC is added, which utilizes samsung
I2S interface as CPU DAI.

This patch is dependent on fsd-pinctrl fixes patch series [1]
[1]: https://lkml.org/lkml/2022/10/13/257

Padmanabhan Rajanbabu (6):
  ASoC: samsung: i2s: TDM Support for CPU DAI driver
  ASoC: samsung: i2s: configure PSR from sound card
  dt-bindings: sound: Add sound card bindings for Tesla FSD
  ASoC: samsung: fsd: Add FSD soundcard driver
  arm64: dts: fsd: Add I2S DAI node for Tesla FSD
  arm64: dts: fsd: Add sound card node for Tesla FSD

 .../bindings/sound/tesla,fsd-card.yaml        | 158 ++++++++
 arch/arm64/boot/dts/tesla/fsd-evb.dts         |  57 +++
 arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi    |  14 +
 arch/arm64/boot/dts/tesla/fsd.dtsi            |  41 ++
 sound/soc/samsung/Kconfig                     |  12 +
 sound/soc/samsung/Makefile                    |   2 +
 sound/soc/samsung/fsd-card.c                  | 349 ++++++++++++++++++
 sound/soc/samsung/i2s-regs.h                  |  17 +
 sound/soc/samsung/i2s.c                       | 120 +++++-
 sound/soc/samsung/i2s.h                       |   1 +
 10 files changed, 766 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
 create mode 100644 sound/soc/samsung/fsd-card.c

Comments

Alim Akhtar Oct. 14, 2022, 1:24 p.m. UTC | #1
>-----Original Message-----
>From: Padmanabhan Rajanbabu [mailto:p.rajanbabu@samsung.com]
>Sent: Friday, October 14, 2022 3:52 PM
>To: lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
>krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
>perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
>alim.akhtar@samsung.com; rcsekar@samsung.com;
>aswani.reddy@samsung.com
>Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
>kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; Padmanabhan
>Rajanbabu <p.rajanbabu@samsung.com>
>Subject: [PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD
>
>Add device tree node for I2S0 and I2S1 CPU DAI instances for Tesla FSD board
>
>Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
>---
> arch/arm64/boot/dts/tesla/fsd-evb.dts      |  8 +++++
> arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi | 14 ++++++++
> arch/arm64/boot/dts/tesla/fsd.dtsi         | 38 ++++++++++++++++++++++
> 3 files changed, 60 insertions(+)
>
>diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts
>b/arch/arm64/boot/dts/tesla/fsd-evb.dts
>index 1db6ddf03f01..c0a4509499ab 100644
>--- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
>+++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
>@@ -41,3 +41,11 @@
> &ufs {
> 	status = "okay";
> };
>+
>+&tdm_0 {
>+	status = "okay";
>+};
>+
>+&tdm_1 {
>+	status = "okay";
>+};
>diff --git a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
>b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
>index e3852c946352..ff6f5d4b16dd 100644
>--- a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
>+++ b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
>@@ -339,6 +339,20 @@
> 		samsung,pin-pud = <FSD_PIN_PULL_UP>;
> 		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> 	};
>+
>+	i2s0_bus: i2s0-bus {
>+		samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2", "gpd1-3",
>"gpd1-4";
>+		samsung,pin-function = <FSD_PIN_FUNC_2>;
>+		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
>+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
>+	};
>+
>+	i2s1_bus: i2s1-bus {
>+		samsung,pins = "gpd2-0", "gpd2-1", "gpd2-2", "gpd2-3",
>"gpd2-4";
>+		samsung,pin-function = <FSD_PIN_FUNC_2>;
>+		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
>+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
>+	};
> };
>
> &pinctrl_pmu {
>diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi
>b/arch/arm64/boot/dts/tesla/fsd.dtsi
>index f35bc5a288c2..5decad45a1b6 100644
>--- a/arch/arm64/boot/dts/tesla/fsd.dtsi
>+++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
>@@ -32,6 +32,8 @@
> 		spi0 = &spi_0;
> 		spi1 = &spi_1;
> 		spi2 = &spi_2;
>+		tdm0 = &tdm_0;
>+		tdm1 = &tdm_1;
> 	};
>
> 	cpus {
>@@ -809,6 +811,42 @@
> 			status = "disabled";
> 		};
>
>+		tdm_0: tdm@140e0000 {
>+			compatible = "samsung,exynos7-i2s";
>+			reg = <0x0 0x140E0000 0x0 0x100>;
Address should be all in small caps
Make sure you have run 'make dtbs_check' 

>+			interrupts = <GIC_SPI 206 IRQ_TYPE_LEVEL_HIGH>;
>+			dmas = <&pdma1 14>, <&pdma1 13>, <&pdma1 12>;
>+			dma-names = "tx", "rx", "tx-sec";
>+			#clock-cells = <1>;
>+			#sound-dai-cells = <1>;
>+			clocks = <&clock_peric PERIC_HCLK_TDM0>,
>+				 <&clock_peric PERIC_HCLK_TDM0>,
>+				 <&clock_peric PERIC_PCLK_TDM0>;
>+			clock-names = "i2s_opclk0", "i2s_opclk1", "iis";
>+			pinctrl-names = "default";
>+			pinctrl-0 = <&i2s0_bus>;
>+			samsung,sec-dai-id = <0>;
>+			status = "disabled";
>+		};
>+
>+		tdm_1: tdm@140f0000 {
>+			compatible = "samsung,exynos7-i2s";
>+			reg = <0x0 0x140F0000 0x0 0x100>;
Same as above

>+			interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>;
>+			dmas = <&pdma1 17>, <&pdma1 16>, <&pdma1 15>;
>+			dma-names = "tx", "rx", "tx-sec";
>+			#clock-cells = <1>;
>+			#sound-dai-cells = <1>;
>+			clocks = <&clock_peric PERIC_HCLK_TDM1>,
>+				 <&clock_peric PERIC_HCLK_TDM1>,
>+				 <&clock_peric PERIC_PCLK_TDM1>;
>+			clock-names = "i2s_opclk0", "i2s_opclk1", "iis";
>+			pinctrl-names = "default";
>+			pinctrl-0 = <&i2s1_bus>;
>+			samsung,sec-dai-id = <1>;
>+			status = "disabled";
>+		};
>+
> 		timer@10040000 {
> 			compatible = "tesla,fsd-mct", "samsung,exynos4210-
>mct";
> 			reg = <0x0 0x10040000 0x0 0x800>;
>--
>2.17.1
Rob Herring (Arm) Oct. 14, 2022, 3:13 p.m. UTC | #2
On Fri, Oct 14, 2022 at 03:51:48PM +0530, Padmanabhan Rajanbabu wrote:
> Add dt-binding reference document to configure the DAI link for Tesla
> FSD sound card driver.

The simple-card or graph-card bindings don't work for you?

> 
> Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> ---
>  .../bindings/sound/tesla,fsd-card.yaml        | 158 ++++++++++++++++++
>  1 file changed, 158 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> new file mode 100644
> index 000000000000..4bd590f4ee27
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> @@ -0,0 +1,158 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2022 Samsung Electronics Co. Ltd.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/tesla,fsd-card.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Tesla FSD ASoC sound card driver
> +
> +maintainers:
> +  - Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> +
> +description: |
> +  This binding describes the node properties and essential DT entries of FSD
> +  SoC sound card node
> +
> +select: false

Never apply this schema. Why?

> +
> +properties:
> +  compatible:
> +    enum:
> +      - tesla,fsd-sndcard
> +
> +  model:
> +    description: Describes the Name of the sound card
> +    $ref: /schemas/types.yaml#/definitions/string
> +
> +  widgets:
> +    description: A list of DAPM widgets in the sound card. Each entry is a pair
> +      of strings, the first being the widget name and the second being the
> +      widget alias
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +
> +  audio-routing:
> +    description: A list of the connections between audio components. Each entry
> +      is a pair of strings, the first being the connection's sink, the second
> +      being the connection's source
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +
> +  dai-tdm-slot-num:
> +    description: Enables TDM mode and specifies the number of TDM slots to be
> +      enabled
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8]

maximum: 8

> +    default: 2
> +
> +  dai-tdm-slot-width:
> +    description: Specifies the slot width of each TDm slot enabled
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [8, 16, 24]
> +    default: 16

All the above have types defined. You should not be redefining the 
types.

> +
> +  dai-link:
> +    description: Holds the DAI link data between CPU, Codec and Platform
> +    type: object

       additionalProperties: false

> +    properties:
> +      link-name:
> +        description: Specifies the name of the DAI link
> +        $ref: /schemas/types.yaml#/definitions/string
> +
> +      dai-format:
> +        description: Specifies the serial data format of CPU DAI
> +        $ref: /schemas/types.yaml#/definitions/string
> +
> +      tesla,bitclock-master:
> +        description: Specifies the phandle of bitclock master DAI
> +        $ref: /schemas/types.yaml#/definitions/phandle
> +
> +      tesla,frame-master:
> +        description: Specifies the phandle of frameclock master DAI
> +        $ref: /schemas/types.yaml#/definitions/phandle

These are common properties. Drop 'tesla'.

> +
> +      cpu:
> +        description: Holds the CPU DAI node and instance
> +        type: object

           additionalProperties: false

> +        properties:
> +          sound-dai:
> +            description: Specifies the phandle of CPU DAI node
> +            $ref: /schemas/types.yaml#/definitions/phandle-array
> +
> +        required:
> +          - sound-dai
> +
> +      codec:
> +        description: Holds the Codec DAI node and instance
> +        type: object

           additionalProperties: false

> +        properties:
> +          sound-dai:
> +            description: Specifies the phandle of Codec DAI node
> +            $ref: /schemas/types.yaml#/definitions/phandle-array

Already has a type. Need to define how many entries (maxItems: 1 ?).

> +
> +        required:
> +          - sound-dai
> +
> +    required:
> +      - link-name
> +      - dai-format
> +      - tesla,bitclock-master
> +      - tesla,frame-master
> +      - cpu
> +
> +dependencies:
> +  dai-tdm-slot-width: [ 'dai-tdm-slot-num' ]

This should be captured with tdm-slot.txt converted to schema.

> +
> +required:
> +  - compatible
> +  - model
> +  - dai-link
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    sound {
> +        compatible = "tesla,fsd-sndcard";
> +        status = "disabled";

Why have a disabled example? Other than your example won't pass your 
schema.

> +        model = "fsd-i2s";
> +
> +        primary-dai-link-0 {
> +            link-name = "fsd-primary-0";
> +            dai-format = "i2s";
> +            tesla,bitclock-master = <&tdm_0>;
> +            tesla,frame-master = <&tdm_0>;
> +            cpu {
> +                sound-dai = <&tdm_0 0>;
> +            };
> +        };
> +
> +        secondary-dai-link-0 {
> +            link-name = "fsd-secondary-0";
> +            dai-format = "i2s";
> +            tesla,bitclock-master = <&tdm_0>;
> +            tesla,frame-master = <&tdm_0>;
> +            cpu {
> +                sound-dai = <&tdm_0 1>;
> +            };
> +        };
> +
> +        primary-dai-link-1 {
> +            link-name = "fsd-primary-1";
> +            dai-format = "i2s";
> +            tesla,bitclock-master = <&tdm_1>;
> +            tesla,frame-master = <&tdm_1>;
> +            cpu {
> +                sound-dai = <&tdm_1 0>;
> +            };
> +        };
> +
> +        secondary-dai-link-1 {
> +            link-name = "fsd-secondary-1";
> +            dai-format = "i2s";
> +            tesla,bitclock-master = <&tdm_1>;
> +            tesla,frame-master = <&tdm_1>;
> +            cpu {
> +                sound-dai = <&tdm_1 1>;
> +            };
> +        };
> +    };
> -- 
> 2.17.1
> 
>
Krzysztof Kozlowski Oct. 16, 2022, 3:14 p.m. UTC | #3
On 14/10/2022 06:21, Padmanabhan Rajanbabu wrote:
> Add device tree node for I2S0 and I2S1 CPU DAI instances for Tesla
> FSD board
> 
> Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> ---
>  arch/arm64/boot/dts/tesla/fsd-evb.dts      |  8 +++++
>  arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi | 14 ++++++++
>  arch/arm64/boot/dts/tesla/fsd.dtsi         | 38 ++++++++++++++++++++++
>  3 files changed, 60 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> index 1db6ddf03f01..c0a4509499ab 100644
> --- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
> +++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> @@ -41,3 +41,11 @@
>  &ufs {
>  	status = "okay";
>  };
> +
> +&tdm_0 {

Alphabetical order against other label-overrides.

> +	status = "okay";
> +};
> +
> +&tdm_1 {
> +	status = "okay";
> +};
> diff --git a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> index e3852c946352..ff6f5d4b16dd 100644
> --- a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> +++ b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> @@ -339,6 +339,20 @@
>  		samsung,pin-pud = <FSD_PIN_PULL_UP>;
>  		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
>  	};
> +
> +	i2s0_bus: i2s0-bus {

Does not look like you tested the DTS against bindings. Please run `make
dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
for instructions).

> +		samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2", "gpd1-3", "gpd1-4";
> +		samsung,pin-function = <FSD_PIN_FUNC_2>;
> +		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
> +		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> +	};
> +
> +	i2s1_bus: i2s1-bus {
> +		samsung,pins = "gpd2-0", "gpd2-1", "gpd2-2", "gpd2-3", "gpd2-4";
> +		samsung,pin-function = <FSD_PIN_FUNC_2>;
> +		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
> +		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> +	};
>  };
>  
>  &pinctrl_pmu {
> diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi b/arch/arm64/boot/dts/tesla/fsd.dtsi
> index f35bc5a288c2..5decad45a1b6 100644
> --- a/arch/arm64/boot/dts/tesla/fsd.dtsi
> +++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
> @@ -32,6 +32,8 @@
>  		spi0 = &spi_0;
>  		spi1 = &spi_1;
>  		spi2 = &spi_2;
> +		tdm0 = &tdm_0;
> +		tdm1 = &tdm_1;

Why?

>  	};
>  
>  	cpus {
> @@ -809,6 +811,42 @@
>  			status = "disabled";
>  		};
>  
> +		tdm_0: tdm@140e0000 {

Node names should be generic, so this looks like i2s.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +			compatible = "samsung,exynos7-i2s";
> +			reg = <0x0 0x140E0000 0x0 0x100>;
> +			interrupts = <GIC_SPI 206 IRQ_TYPE_LEVEL_HIGH>;
> +			dmas = <&pdma1 14>, <&pdma1 13>, <&pdma1 12>;
> +			dma-names = "tx", "rx", "tx-sec";
> +			#clock-cells = <1>;
> +			#sound-dai-cells = <1>;
> +			clocks = <&clock_peric PERIC_HCLK_TDM0>,
> +				 <&clock_peric PERIC_HCLK_TDM0>,
> +				 <&clock_peric PERIC_PCLK_TDM0>;
> +			clock-names = "i2s_opclk0", "i2s_opclk1", "iis";

Does not look like you tested the DTS against bindings. Please run `make
dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
for instructions).

Best regards,
Krzysztof
Padmanabhan Rajanbabu Oct. 21, 2022, 8:09 a.m. UTC | #4
> -----Original Message-----
> From: Alim Akhtar [mailto:alim.akhtar@samsung.com]
> Sent: 14 October 2022 06:54 PM
> To: 'Padmanabhan Rajanbabu' <p.rajanbabu@samsung.com>;
> lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> rcsekar@samsung.com; aswani.reddy@samsung.com
> Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org
> Subject: RE: [PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD
> 
> 
> 
> >-----Original Message-----
> >From: Padmanabhan Rajanbabu [mailto:p.rajanbabu@samsung.com]
> >Sent: Friday, October 14, 2022 3:52 PM
> >To: lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
> >krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> >perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> >alim.akhtar@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com
> >Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
> >kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org;
> Padmanabhan
> >Rajanbabu <p.rajanbabu@samsung.com>
> >Subject: [PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD
> >
> >Add device tree node for I2S0 and I2S1 CPU DAI instances for Tesla FSD
> >board
> >
> >Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> >---
> > arch/arm64/boot/dts/tesla/fsd-evb.dts      |  8 +++++
> > arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi | 14 ++++++++
> > arch/arm64/boot/dts/tesla/fsd.dtsi         | 38 ++++++++++++++++++++++
> > 3 files changed, 60 insertions(+)
> >
> >diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts
> >b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> >index 1db6ddf03f01..c0a4509499ab 100644
> >--- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
> >+++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> >@@ -41,3 +41,11 @@
> > &ufs {
> > 	status = "okay";
> > };
> >+
> >+&tdm_0 {
> >+	status = "okay";
> >+};
> >+
> >+&tdm_1 {
> >+	status = "okay";
> >+};
> >diff --git a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> >b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> >index e3852c946352..ff6f5d4b16dd 100644
> >--- a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> >+++ b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> >@@ -339,6 +339,20 @@
> > 		samsung,pin-pud = <FSD_PIN_PULL_UP>;
> > 		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> > 	};
> >+
> >+	i2s0_bus: i2s0-bus {
> >+		samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2", "gpd1-3",
> >"gpd1-4";
> >+		samsung,pin-function = <FSD_PIN_FUNC_2>;
> >+		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
> >+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> >+	};
> >+
> >+	i2s1_bus: i2s1-bus {
> >+		samsung,pins = "gpd2-0", "gpd2-1", "gpd2-2", "gpd2-3",
> >"gpd2-4";
> >+		samsung,pin-function = <FSD_PIN_FUNC_2>;
> >+		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
> >+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> >+	};
> > };
> >
> > &pinctrl_pmu {
> >diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi
> >b/arch/arm64/boot/dts/tesla/fsd.dtsi
> >index f35bc5a288c2..5decad45a1b6 100644
> >--- a/arch/arm64/boot/dts/tesla/fsd.dtsi
> >+++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
> >@@ -32,6 +32,8 @@
> > 		spi0 = &spi_0;
> > 		spi1 = &spi_1;
> > 		spi2 = &spi_2;
> >+		tdm0 = &tdm_0;
> >+		tdm1 = &tdm_1;
> > 	};
> >
> > 	cpus {
> >@@ -809,6 +811,42 @@
> > 			status = "disabled";
> > 		};
> >
> >+		tdm_0: tdm@140e0000 {
> >+			compatible = "samsung,exynos7-i2s";
> >+			reg = <0x0 0x140E0000 0x0 0x100>;
> Address should be all in small caps
> Make sure you have run 'make dtbs_check'
Okay
> 
> >+			interrupts = <GIC_SPI 206 IRQ_TYPE_LEVEL_HIGH>;
> >+			dmas = <&pdma1 14>, <&pdma1 13>, <&pdma1 12>;
> >+			dma-names = "tx", "rx", "tx-sec";
> >+			#clock-cells = <1>;
> >+			#sound-dai-cells = <1>;
> >+			clocks = <&clock_peric PERIC_HCLK_TDM0>,
> >+				 <&clock_peric PERIC_HCLK_TDM0>,
> >+				 <&clock_peric PERIC_PCLK_TDM0>;
> >+			clock-names = "i2s_opclk0", "i2s_opclk1", "iis";
> >+			pinctrl-names = "default";
> >+			pinctrl-0 = <&i2s0_bus>;
> >+			samsung,sec-dai-id = <0>;
> >+			status = "disabled";
> >+		};
> >+
> >+		tdm_1: tdm@140f0000 {
> >+			compatible = "samsung,exynos7-i2s";
> >+			reg = <0x0 0x140F0000 0x0 0x100>;
> Same as above
Okay
> 
> >+			interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>;
> >+			dmas = <&pdma1 17>, <&pdma1 16>, <&pdma1 15>;
> >+			dma-names = "tx", "rx", "tx-sec";
> >+			#clock-cells = <1>;
> >+			#sound-dai-cells = <1>;
> >+			clocks = <&clock_peric PERIC_HCLK_TDM1>,
> >+				 <&clock_peric PERIC_HCLK_TDM1>,
> >+				 <&clock_peric PERIC_PCLK_TDM1>;
> >+			clock-names = "i2s_opclk0", "i2s_opclk1", "iis";
> >+			pinctrl-names = "default";
> >+			pinctrl-0 = <&i2s1_bus>;
> >+			samsung,sec-dai-id = <1>;
> >+			status = "disabled";
> >+		};
> >+
> > 		timer@10040000 {
> > 			compatible = "tesla,fsd-mct", "samsung,exynos4210-
> mct";
> > 			reg = <0x0 0x10040000 0x0 0x800>;
> >--
> >2.17.1
>
Padmanabhan Rajanbabu Oct. 21, 2022, 8:44 a.m. UTC | #5
> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: 14 October 2022 08:43 PM
> To: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> Cc: lgirdwood@gmail.com; broonie@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> alim.akhtar@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com; alsa-devel@alsa-project.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-samsung-
> soc@vger.kernel.org
> Subject: Re: [PATCH 3/6] dt-bindings: sound: Add sound card bindings for
> Tesla FSD
> 
> On Fri, Oct 14, 2022 at 03:51:48PM +0530, Padmanabhan Rajanbabu wrote:
> > Add dt-binding reference document to configure the DAI link for Tesla
> > FSD sound card driver.
> 
> The simple-card or graph-card bindings don't work for you?
Thank you for reviewing the patch.

The actual reason for having a custom sound card driver lies in the fact
that the Samsung i2s cpu dai requires configuration of some Samsung
specific properties like rfs, bfs, codec clock direction and root clock
source. We do not have flexibility of configuring the same in simple card
driver (sound/soc/generic/simple-card.c) or audio graph card driver 
(sound/soc/generic/audio-graph-card.c). The binding has been added to
support the custom sound card driver.

I understand from your query that the newly added card has device tree
nodes and properties which are used in simple card as well, but having a
different or new prefixes. I believe to address that, we can restructure
the string names to generic ones. But I would like to understand, to reuse
the simple card or audio graph card bindings, can we add secondary
compatible strings in the simple card dt-binding for the custom sound card
to probe ?
> 
> >
> > Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> > ---
> >  .../bindings/sound/tesla,fsd-card.yaml        | 158 ++++++++++++++++++
> >  1 file changed, 158 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> > b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> > new file mode 100644
> > index 000000000000..4bd590f4ee27
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> > @@ -0,0 +1,158 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) # Copyright
> > +2022 Samsung Electronics Co. Ltd.
> > +%YAML 1.2
> > +---
> > +$id:
> > +https://protect2.fireeye.com/v1/url?k=4ae54403-157e7d1c-4ae4cf4c-
> 000b
> > +abdfecba-9eb398ea304f8ae8&q=1&e=4935bed0-ce62-47dd-8faf-
> 4750b01e22d3&
> >
> +u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fsound%2Ftesla%2Cfsd-
> card.ya
> > +ml%23
> > +$schema:
> > +https://protect2.fireeye.com/v1/url?k=8c72226e-d3e91b71-8c73a921-
> 000b
> > +abdfecba-3ce999f6c052255b&q=1&e=4935bed0-ce62-47dd-8faf-
> 4750b01e22d3&
> > +u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
> > +
> > +title: Tesla FSD ASoC sound card driver
> > +
> > +maintainers:
> > +  - Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> > +
> > +description: |
> > +  This binding describes the node properties and essential DT entries
> > +of FSD
> > +  SoC sound card node
> > +
> > +select: false
> 
> Never apply this schema. Why?
Sorry about it, let me change the select property to true in the next
patchset
> 
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - tesla,fsd-sndcard
> > +
> > +  model:
> > +    description: Describes the Name of the sound card
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +
> > +  widgets:
> > +    description: A list of DAPM widgets in the sound card. Each entry
is a pair
> > +      of strings, the first being the widget name and the second being
the
> > +      widget alias
> > +    $ref: /schemas/types.yaml#/definitions/string-array
> > +
> > +  audio-routing:
> > +    description: A list of the connections between audio components.
Each entry
> > +      is a pair of strings, the first being the connection's sink, the
second
> > +      being the connection's source
> > +    $ref: /schemas/types.yaml#/definitions/string-array
> > +
> > +  dai-tdm-slot-num:
> > +    description: Enables TDM mode and specifies the number of TDM slots
to be
> > +      enabled
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8]
> 
> maximum: 8
Okay
> 
> > +    default: 2
> > +
> > +  dai-tdm-slot-width:
> > +    description: Specifies the slot width of each TDm slot enabled
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [8, 16, 24]
> > +    default: 16
> 
> All the above have types defined. You should not be redefining the types.
Okay
> 
> > +
> > +  dai-link:
> > +    description: Holds the DAI link data between CPU, Codec and
Platform
> > +    type: object
> 
>        additionalProperties: false
okay
> 
> > +    properties:
> > +      link-name:
> > +        description: Specifies the name of the DAI link
> > +        $ref: /schemas/types.yaml#/definitions/string
> > +
> > +      dai-format:
> > +        description: Specifies the serial data format of CPU DAI
> > +        $ref: /schemas/types.yaml#/definitions/string
> > +
> > +      tesla,bitclock-master:
> > +        description: Specifies the phandle of bitclock master DAI
> > +        $ref: /schemas/types.yaml#/definitions/phandle
> > +
> > +      tesla,frame-master:
> > +        description: Specifies the phandle of frameclock master DAI
> > +        $ref: /schemas/types.yaml#/definitions/phandle
> 
> These are common properties. Drop 'tesla'.
Okay
> 
> > +
> > +      cpu:
> > +        description: Holds the CPU DAI node and instance
> > +        type: object
> 
>            additionalProperties: false
Okay
> 
> > +        properties:
> > +          sound-dai:
> > +            description: Specifies the phandle of CPU DAI node
> > +            $ref: /schemas/types.yaml#/definitions/phandle-array
> > +
> > +        required:
> > +          - sound-dai
> > +
> > +      codec:
> > +        description: Holds the Codec DAI node and instance
> > +        type: object
> 
>            additionalProperties: false
Okay
> 
> > +        properties:
> > +          sound-dai:
> > +            description: Specifies the phandle of Codec DAI node
> > +            $ref: /schemas/types.yaml#/definitions/phandle-array
> 
> Already has a type. Need to define how many entries (maxItems: 1 ?).
Okay. I'll update in the upcoming patch set
> 
> > +
> > +        required:
> > +          - sound-dai
> > +
> > +    required:
> > +      - link-name
> > +      - dai-format
> > +      - tesla,bitclock-master
> > +      - tesla,frame-master
> > +      - cpu
> > +
> > +dependencies:
> > +  dai-tdm-slot-width: [ 'dai-tdm-slot-num' ]
> 
> This should be captured with tdm-slot.txt converted to schema.
Okay
> 
> > +
> > +required:
> > +  - compatible
> > +  - model
> > +  - dai-link
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    sound {
> > +        compatible = "tesla,fsd-sndcard";
> > +        status = "disabled";
> 
> Why have a disabled example? Other than your example won't pass your
> schema.
Thanks for noticing, I'll double check and change the example keeping the
status 
as enabled
> 
> > +        model = "fsd-i2s";
> > +
> > +        primary-dai-link-0 {
> > +            link-name = "fsd-primary-0";
> > +            dai-format = "i2s";
> > +            tesla,bitclock-master = <&tdm_0>;
> > +            tesla,frame-master = <&tdm_0>;
> > +            cpu {
> > +                sound-dai = <&tdm_0 0>;
> > +            };
> > +        };
> > +
> > +        secondary-dai-link-0 {
> > +            link-name = "fsd-secondary-0";
> > +            dai-format = "i2s";
> > +            tesla,bitclock-master = <&tdm_0>;
> > +            tesla,frame-master = <&tdm_0>;
> > +            cpu {
> > +                sound-dai = <&tdm_0 1>;
> > +            };
> > +        };
> > +
> > +        primary-dai-link-1 {
> > +            link-name = "fsd-primary-1";
> > +            dai-format = "i2s";
> > +            tesla,bitclock-master = <&tdm_1>;
> > +            tesla,frame-master = <&tdm_1>;
> > +            cpu {
> > +                sound-dai = <&tdm_1 0>;
> > +            };
> > +        };
> > +
> > +        secondary-dai-link-1 {
> > +            link-name = "fsd-secondary-1";
> > +            dai-format = "i2s";
> > +            tesla,bitclock-master = <&tdm_1>;
> > +            tesla,frame-master = <&tdm_1>;
> > +            cpu {
> > +                sound-dai = <&tdm_1 1>;
> > +            };
> > +        };
> > +    };
> > --
> > 2.17.1
> >
> >
Padmanabhan Rajanbabu Oct. 21, 2022, 8:49 a.m. UTC | #6
> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 16 October 2022 08:44 PM
> To: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>;
> lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> alim.akhtar@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com
> Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org
> Subject: Re: [PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD
> 
> On 14/10/2022 06:21, Padmanabhan Rajanbabu wrote:
> > Add device tree node for I2S0 and I2S1 CPU DAI instances for Tesla FSD
> > board
> >
> > Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> > ---
> >  arch/arm64/boot/dts/tesla/fsd-evb.dts      |  8 +++++
> >  arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi | 14 ++++++++
> >  arch/arm64/boot/dts/tesla/fsd.dtsi         | 38 ++++++++++++++++++++++
> >  3 files changed, 60 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts
> > b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> > index 1db6ddf03f01..c0a4509499ab 100644
> > --- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
> > +++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> > @@ -41,3 +41,11 @@
> >  &ufs {
> >  	status = "okay";
> >  };
> > +
> > +&tdm_0 {
> 
> Alphabetical order against other label-overrides.
Okay
> 
> > +	status = "okay";
> > +};
> > +
> > +&tdm_1 {
> > +	status = "okay";
> > +};
> > diff --git a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> > b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> > index e3852c946352..ff6f5d4b16dd 100644
> > --- a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> > +++ b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> > @@ -339,6 +339,20 @@
> >  		samsung,pin-pud = <FSD_PIN_PULL_UP>;
> >  		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> >  	};
> > +
> > +	i2s0_bus: i2s0-bus {
> 
> Does not look like you tested the DTS against bindings. Please run `make
> dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
> for instructions).
I'll double check and run dtbs_check to see if I'm hitting any errors.
> 
> > +		samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2", "gpd1-3",
> "gpd1-4";
> > +		samsung,pin-function = <FSD_PIN_FUNC_2>;
> > +		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
> > +		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> > +	};
> > +
> > +	i2s1_bus: i2s1-bus {
> > +		samsung,pins = "gpd2-0", "gpd2-1", "gpd2-2", "gpd2-3",
> "gpd2-4";
> > +		samsung,pin-function = <FSD_PIN_FUNC_2>;
> > +		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
> > +		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> > +	};
> >  };
> >
> >  &pinctrl_pmu {
> > diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi
> > b/arch/arm64/boot/dts/tesla/fsd.dtsi
> > index f35bc5a288c2..5decad45a1b6 100644
> > --- a/arch/arm64/boot/dts/tesla/fsd.dtsi
> > +++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
> > @@ -32,6 +32,8 @@
> >  		spi0 = &spi_0;
> >  		spi1 = &spi_1;
> >  		spi2 = &spi_2;
> > +		tdm0 = &tdm_0;
> > +		tdm1 = &tdm_1;
> 
> Why?
Sorry, these aliases are not used right now. I'll remove it.

> 
> >  	};
> >
> >  	cpus {
> > @@ -809,6 +811,42 @@
> >  			status = "disabled";
> >  		};
> >
> > +		tdm_0: tdm@140e0000 {
> 
> Node names should be generic, so this looks like i2s.
> https://protect2.fireeye.com/v1/url?k=2cfaa5af-4d874de8-2cfb2ee0-
> 74fe485fff30-cb16acc0c0c574e9&q=1&e=fc8e3b54-a0ef-475e-a4f2-
> 83626a86ac8a&u=https%3A%2F%2Fdevicetree-
> specification.readthedocs.io%2Fen%2Flatest%2Fchapter2-devicetree-
> basics.html%23generic-names-recommendation

Thank you for the link. I could only find audio-controller in the list and
not i2s. so I believe I can use audio-controller node name. Please correct
me otherwise.
> 
> > +			compatible = "samsung,exynos7-i2s";
> > +			reg = <0x0 0x140E0000 0x0 0x100>;
> > +			interrupts = <GIC_SPI 206 IRQ_TYPE_LEVEL_HIGH>;
> > +			dmas = <&pdma1 14>, <&pdma1 13>, <&pdma1 12>;
> > +			dma-names = "tx", "rx", "tx-sec";
> > +			#clock-cells = <1>;
> > +			#sound-dai-cells = <1>;
> > +			clocks = <&clock_peric PERIC_HCLK_TDM0>,
> > +				 <&clock_peric PERIC_HCLK_TDM0>,
> > +				 <&clock_peric PERIC_PCLK_TDM0>;
> > +			clock-names = "i2s_opclk0", "i2s_opclk1", "iis";
> 
> Does not look like you tested the DTS against bindings. Please run `make
> dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
> for instructions).
I'll double check and run dtbs_check to see if I'm hitting any errors.
> 
> Best regards,
> Krzysztof
Thank you for reviewing the patch
Krzysztof Kozlowski Oct. 21, 2022, 1:01 p.m. UTC | #7
On 21/10/2022 04:49, Padmanabhan Rajanbabu wrote:
>>>
>>>  	cpus {
>>> @@ -809,6 +811,42 @@
>>>  			status = "disabled";
>>>  		};
>>>
>>> +		tdm_0: tdm@140e0000 {
>>
>> Node names should be generic, so this looks like i2s.
>> https://protect2.fireeye.com/v1/url?k=2cfaa5af-4d874de8-2cfb2ee0-
>> 74fe485fff30-cb16acc0c0c574e9&q=1&e=fc8e3b54-a0ef-475e-a4f2-
>> 83626a86ac8a&u=https%3A%2F%2Fdevicetree-
>> specification.readthedocs.io%2Fen%2Flatest%2Fchapter2-devicetree-
>> basics.html%23generic-names-recommendation
> 
> Thank you for the link. I could only find audio-controller in the list and
> not i2s. so I believe I can use audio-controller node name. Please correct
> me otherwise.

All I2S controllers use node name "i2s", so if this is I2S, then use "i2s".


Best regards,
Krzysztof
Krzysztof Kozlowski Oct. 22, 2022, 4:48 p.m. UTC | #8
On 21/10/2022 04:44, Padmanabhan Rajanbabu wrote:
>> On Fri, Oct 14, 2022 at 03:51:48PM +0530, Padmanabhan Rajanbabu wrote:
>>> Add dt-binding reference document to configure the DAI link for Tesla
>>> FSD sound card driver.
>>
>> The simple-card or graph-card bindings don't work for you?
> Thank you for reviewing the patch.
> 
> The actual reason for having a custom sound card driver lies in the fact
> that the Samsung i2s cpu dai requires configuration of some Samsung
> specific properties like rfs, bfs, codec clock direction and root clock
> source. We do not have flexibility of configuring the same in simple card
> driver (sound/soc/generic/simple-card.c) or audio graph card driver 
> (sound/soc/generic/audio-graph-card.c). The binding has been added to
> support the custom sound card driver.
> 
> I understand from your query that the newly added card has device tree
> nodes and properties which are used in simple card as well, but having a
> different or new prefixes. I believe to address that, we can restructure
> the string names to generic ones. 

You must use generic, existing properties where applicable.

> But I would like to understand, to reuse
> the simple card or audio graph card bindings, can we add secondary
> compatible strings in the simple card dt-binding for the custom sound card
> to probe ?

If you see other vendor compatibles there, then yes... But there aren't
any, right?

>>
>>>
>>> Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
>>> ---
>>>  .../bindings/sound/tesla,fsd-card.yaml        | 158 ++++++++++++++++++
>>>  1 file changed, 158 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
>>> b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
>>> new file mode 100644
>>> index 000000000000..4bd590f4ee27
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
>>> @@ -0,0 +1,158 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) # Copyright
>>> +2022 Samsung Electronics Co. Ltd.
>>> +%YAML 1.2
>>> +---
>>> +$id:
>>> +https://protect2.fireeye.com/v1/url?k=4ae54403-157e7d1c-4ae4cf4c-
>> 000b
>>> +abdfecba-9eb398ea304f8ae8&q=1&e=4935bed0-ce62-47dd-8faf-
>> 4750b01e22d3&
>>>
>> +u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fsound%2Ftesla%2Cfsd-
>> card.ya
>>> +ml%23
>>> +$schema:
>>> +https://protect2.fireeye.com/v1/url?k=8c72226e-d3e91b71-8c73a921-
>> 000b
>>> +abdfecba-3ce999f6c052255b&q=1&e=4935bed0-ce62-47dd-8faf-
>> 4750b01e22d3&
>>> +u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
>>> +
>>> +title: Tesla FSD ASoC sound card driver
>>> +
>>> +maintainers:
>>> +  - Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
>>> +
>>> +description: |
>>> +  This binding describes the node properties and essential DT entries
>>> +of FSD
>>> +  SoC sound card node
>>> +
>>> +select: false
>>
>> Never apply this schema. Why?
> Sorry about it, let me change the select property to true in the next
> patchset

No, just drop it. Look at other bindings or at example-schema

>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - tesla,fsd-sndcard
>>> +
>>> +  model:
>>> +    description: Describes the Name of the sound card
>>> +    $ref: /schemas/types.yaml#/definitions/string
>>> +
>>> +  widgets:
>>> +    description: A list of DAPM widgets in the sound card. Each entry
> is a pair
>>> +      of strings, the first being the widget name and the second being
> the
>>> +      widget alias
>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>> +
>>> +  audio-routing:
>>> +    description: A list of the connections between audio components.
> Each entry
>>> +      is a pair of strings, the first being the connection's sink, the
> second
>>> +      being the connection's source
>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>> +
>>> +  dai-tdm-slot-num:
>>> +    description: Enables TDM mode and specifies the number of TDM slots
> to be
>>> +      enabled
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8]
>>
>> maximum: 8
> Okay
>>
>>> +    default: 2
>>> +
>>> +  dai-tdm-slot-width:
>>> +    description: Specifies the slot width of each TDm slot enabled
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [8, 16, 24]
>>> +    default: 16
>>
>> All the above have types defined. You should not be redefining the types.
> Okay
>>
>>> +
>>> +  dai-link:
>>> +    description: Holds the DAI link data between CPU, Codec and
> Platform
>>> +    type: object
>>
>>        additionalProperties: false
> okay
>>
>>> +    properties:
>>> +      link-name:
>>> +        description: Specifies the name of the DAI link
>>> +        $ref: /schemas/types.yaml#/definitions/string
>>> +
>>> +      dai-format:
>>> +        description: Specifies the serial data format of CPU DAI
>>> +        $ref: /schemas/types.yaml#/definitions/string
>>> +
>>> +      tesla,bitclock-master:
>>> +        description: Specifies the phandle of bitclock master DAI
>>> +        $ref: /schemas/types.yaml#/definitions/phandle
>>> +
>>> +      tesla,frame-master:
>>> +        description: Specifies the phandle of frameclock master DAI
>>> +        $ref: /schemas/types.yaml#/definitions/phandle
>>
>> These are common properties. Drop 'tesla'.
> Okay
>>
>>> +
>>> +      cpu:
>>> +        description: Holds the CPU DAI node and instance
>>> +        type: object
>>
>>            additionalProperties: false
> Okay
>>
>>> +        properties:
>>> +          sound-dai:
>>> +            description: Specifies the phandle of CPU DAI node
>>> +            $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +
>>> +        required:
>>> +          - sound-dai
>>> +
>>> +      codec:
>>> +        description: Holds the Codec DAI node and instance
>>> +        type: object
>>
>>            additionalProperties: false
> Okay
>>
>>> +        properties:
>>> +          sound-dai:
>>> +            description: Specifies the phandle of Codec DAI node
>>> +            $ref: /schemas/types.yaml#/definitions/phandle-array
>>
>> Already has a type. Need to define how many entries (maxItems: 1 ?).
> Okay. I'll update in the upcoming patch set
>>
>>> +
>>> +        required:
>>> +          - sound-dai
>>> +
>>> +    required:
>>> +      - link-name
>>> +      - dai-format
>>> +      - tesla,bitclock-master
>>> +      - tesla,frame-master
>>> +      - cpu
>>> +
>>> +dependencies:
>>> +  dai-tdm-slot-width: [ 'dai-tdm-slot-num' ]
>>
>> This should be captured with tdm-slot.txt converted to schema.
> Okay
>>
>>> +
>>> +required:
>>> +  - compatible
>>> +  - model
>>> +  - dai-link
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    sound {
>>> +        compatible = "tesla,fsd-sndcard";
>>> +        status = "disabled";
>>
>> Why have a disabled example? Other than your example won't pass your
>> schema.
> Thanks for noticing, I'll double check and change the example keeping the
> status 
> as enabled

No, just drop. Start with example-schema instead.

Best regards,
Krzysztof
Padmanabhan Rajanbabu Nov. 8, 2022, 5:26 a.m. UTC | #9
> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 21 October 2022 06:32 PM
> To: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>;
> lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> alim.akhtar@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com
> Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org
> Subject: Re: [PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD
> 
> On 21/10/2022 04:49, Padmanabhan Rajanbabu wrote:
> >>>
> >>>  	cpus {
> >>> @@ -809,6 +811,42 @@
> >>>  			status = "disabled";
> >>>  		};
> >>>
> >>> +		tdm_0: tdm@140e0000 {
> >>
> >> Node names should be generic, so this looks like i2s.
> >> https://protect2.fireeye.com/v1/url?k=2cfaa5af-4d874de8-2cfb2ee0-
> >> 74fe485fff30-cb16acc0c0c574e9&q=1&e=fc8e3b54-a0ef-475e-a4f2-
> >> 83626a86ac8a&u=https%3A%2F%2Fdevicetree-
> >> specification.readthedocs.io%2Fen%2Flatest%2Fchapter2-devicetree-
> >> basics.html%23generic-names-recommendation
> >
> > Thank you for the link. I could only find audio-controller in the list
> > and not i2s. so I believe I can use audio-controller node name. Please
> > correct me otherwise.
> 
> All I2S controllers use node name "i2s", so if this is I2S, then use "i2s".

Okay, I'll ensure the next version will use "i2s" node name

> 
> 
> Best regards,
> Krzysztof
Padmanabhan Rajanbabu Nov. 8, 2022, 5:33 a.m. UTC | #10
> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 22 October 2022 10:19 PM
> To: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>; 'Rob Herring'
> <robh@kernel.org>
> Cc: lgirdwood@gmail.com; broonie@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> alim.akhtar@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com; alsa-devel@alsa-project.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-samsung-
> soc@vger.kernel.org
> Subject: Re: [PATCH 3/6] dt-bindings: sound: Add sound card bindings for
> Tesla FSD
> 
> On 21/10/2022 04:44, Padmanabhan Rajanbabu wrote:
> >> On Fri, Oct 14, 2022 at 03:51:48PM +0530, Padmanabhan Rajanbabu wrote:
> >>> Add dt-binding reference document to configure the DAI link for
> >>> Tesla FSD sound card driver.
> >>
> >> The simple-card or graph-card bindings don't work for you?
> > Thank you for reviewing the patch.
> >
> > The actual reason for having a custom sound card driver lies in the
> > fact that the Samsung i2s cpu dai requires configuration of some
> > Samsung specific properties like rfs, bfs, codec clock direction and
> > root clock source. We do not have flexibility of configuring the same
> > in simple card driver (sound/soc/generic/simple-card.c) or audio graph
> > card driver (sound/soc/generic/audio-graph-card.c). The binding has
> > been added to support the custom sound card driver.
> >
> > I understand from your query that the newly added card has device tree
> > nodes and properties which are used in simple card as well, but having
> > a different or new prefixes. I believe to address that, we can
> > restructure the string names to generic ones.
> 
> You must use generic, existing properties where applicable.

Okay

> 
> > But I would like to understand, to reuse the simple card or audio
> > graph card bindings, can we add secondary compatible strings in the
> > simple card dt-binding for the custom sound card to probe ?
> 
> If you see other vendor compatibles there, then yes... But there aren't any,
> right?

Yes you are right, we don't see other vendor compatibles. But, am I allowed
to add such secondary compatibles so that we can extend the simple card
and its utilities to a large extent?

If no, then I believe we will need a separate binding to extend the generic
properties.
> 
> >>
> >>>
> >>> Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> >>> ---
> >>>  .../bindings/sound/tesla,fsd-card.yaml        | 158 ++++++++++++++++++
> >>>  1 file changed, 158 insertions(+)
> >>>  create mode 100644
> >>> Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> >>>
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> >>> b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> >>> new file mode 100644
> >>> index 000000000000..4bd590f4ee27
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> >>> @@ -0,0 +1,158 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) #
> >>> +Copyright
> >>> +2022 Samsung Electronics Co. Ltd.
> >>> +%YAML 1.2
> >>> +---
> >>> +$id:
> >>> +https://protect2.fireeye.com/v1/url?k=4ae54403-157e7d1c-4ae4cf4c-
> >> 000b
> >>> +abdfecba-9eb398ea304f8ae8&q=1&e=4935bed0-ce62-47dd-8faf-
> >> 4750b01e22d3&
> >>>
> >>
> +u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fsound%2Ftesla%2Cfsd-
> >> card.ya
> >>> +ml%23
> >>> +$schema:
> >>> +https://protect2.fireeye.com/v1/url?k=8c72226e-d3e91b71-8c73a921-
> >> 000b
> >>> +abdfecba-3ce999f6c052255b&q=1&e=4935bed0-ce62-47dd-8faf-
> >> 4750b01e22d3&
> >>> +u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
> >>> +
> >>> +title: Tesla FSD ASoC sound card driver
> >>> +
> >>> +maintainers:
> >>> +  - Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> >>> +
> >>> +description: |
> >>> +  This binding describes the node properties and essential DT
> >>> +entries of FSD
> >>> +  SoC sound card node
> >>> +
> >>> +select: false
> >>
> >> Never apply this schema. Why?
> > Sorry about it, let me change the select property to true in the next
> > patchset
> 
> No, just drop it. Look at other bindings or at example-schema
> 
> >>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - tesla,fsd-sndcard
> >>> +
> >>> +  model:
> >>> +    description: Describes the Name of the sound card
> >>> +    $ref: /schemas/types.yaml#/definitions/string
> >>> +
> >>> +  widgets:
> >>> +    description: A list of DAPM widgets in the sound card. Each
> >>> + entry
> > is a pair
> >>> +      of strings, the first being the widget name and the second
> >>> + being
> > the
> >>> +      widget alias
> >>> +    $ref: /schemas/types.yaml#/definitions/string-array
> >>> +
> >>> +  audio-routing:
> >>> +    description: A list of the connections between audio components.
> > Each entry
> >>> +      is a pair of strings, the first being the connection's sink,
> >>> + the
> > second
> >>> +      being the connection's source
> >>> +    $ref: /schemas/types.yaml#/definitions/string-array
> >>> +
> >>> +  dai-tdm-slot-num:
> >>> +    description: Enables TDM mode and specifies the number of TDM
> >>> + slots
> > to be
> >>> +      enabled
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8]
> >>
> >> maximum: 8
> > Okay
> >>
> >>> +    default: 2
> >>> +
> >>> +  dai-tdm-slot-width:
> >>> +    description: Specifies the slot width of each TDm slot enabled
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    enum: [8, 16, 24]
> >>> +    default: 16
> >>
> >> All the above have types defined. You should not be redefining the types.
> > Okay
> >>
> >>> +
> >>> +  dai-link:
> >>> +    description: Holds the DAI link data between CPU, Codec and
> > Platform
> >>> +    type: object
> >>
> >>        additionalProperties: false
> > okay
> >>
> >>> +    properties:
> >>> +      link-name:
> >>> +        description: Specifies the name of the DAI link
> >>> +        $ref: /schemas/types.yaml#/definitions/string
> >>> +
> >>> +      dai-format:
> >>> +        description: Specifies the serial data format of CPU DAI
> >>> +        $ref: /schemas/types.yaml#/definitions/string
> >>> +
> >>> +      tesla,bitclock-master:
> >>> +        description: Specifies the phandle of bitclock master DAI
> >>> +        $ref: /schemas/types.yaml#/definitions/phandle
> >>> +
> >>> +      tesla,frame-master:
> >>> +        description: Specifies the phandle of frameclock master DAI
> >>> +        $ref: /schemas/types.yaml#/definitions/phandle
> >>
> >> These are common properties. Drop 'tesla'.
> > Okay
> >>
> >>> +
> >>> +      cpu:
> >>> +        description: Holds the CPU DAI node and instance
> >>> +        type: object
> >>
> >>            additionalProperties: false
> > Okay
> >>
> >>> +        properties:
> >>> +          sound-dai:
> >>> +            description: Specifies the phandle of CPU DAI node
> >>> +            $ref: /schemas/types.yaml#/definitions/phandle-array
> >>> +
> >>> +        required:
> >>> +          - sound-dai
> >>> +
> >>> +      codec:
> >>> +        description: Holds the Codec DAI node and instance
> >>> +        type: object
> >>
> >>            additionalProperties: false
> > Okay
> >>
> >>> +        properties:
> >>> +          sound-dai:
> >>> +            description: Specifies the phandle of Codec DAI node
> >>> +            $ref: /schemas/types.yaml#/definitions/phandle-array
> >>
> >> Already has a type. Need to define how many entries (maxItems: 1 ?).
> > Okay. I'll update in the upcoming patch set
> >>
> >>> +
> >>> +        required:
> >>> +          - sound-dai
> >>> +
> >>> +    required:
> >>> +      - link-name
> >>> +      - dai-format
> >>> +      - tesla,bitclock-master
> >>> +      - tesla,frame-master
> >>> +      - cpu
> >>> +
> >>> +dependencies:
> >>> +  dai-tdm-slot-width: [ 'dai-tdm-slot-num' ]
> >>
> >> This should be captured with tdm-slot.txt converted to schema.
> > Okay
> >>
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - model
> >>> +  - dai-link
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +    sound {
> >>> +        compatible = "tesla,fsd-sndcard";
> >>> +        status = "disabled";
> >>
> >> Why have a disabled example? Other than your example won't pass your
> >> schema.
> > Thanks for noticing, I'll double check and change the example keeping
> > the status as enabled
> 
> No, just drop. Start with example-schema instead.
> 
> Best regards,
> Krzysztof
Krzysztof Kozlowski Nov. 8, 2022, 10:36 a.m. UTC | #11
On 08/11/2022 06:33, Padmanabhan Rajanbabu wrote:
>>>
>>> The actual reason for having a custom sound card driver lies in the
>>> fact that the Samsung i2s cpu dai requires configuration of some
>>> Samsung specific properties like rfs, bfs, codec clock direction and
>>> root clock source. We do not have flexibility of configuring the same
>>> in simple card driver (sound/soc/generic/simple-card.c) or audio graph
>>> card driver (sound/soc/generic/audio-graph-card.c). The binding has
>>> been added to support the custom sound card driver.
>>>
>>> I understand from your query that the newly added card has device tree
>>> nodes and properties which are used in simple card as well, but having
>>> a different or new prefixes. I believe to address that, we can
>>> restructure the string names to generic ones.
>>
>> You must use generic, existing properties where applicable.
> 
> Okay
> 
>>
>>> But I would like to understand, to reuse the simple card or audio
>>> graph card bindings, can we add secondary compatible strings in the
>>> simple card dt-binding for the custom sound card to probe ?
>>
>> If you see other vendor compatibles there, then yes... But there aren't any,
>> right?
> 
> Yes you are right, we don't see other vendor compatibles. But, am I allowed
> to add such secondary compatibles so that we can extend the simple card
> and its utilities to a large extent?
> 
> If no, then I believe we will need a separate binding to extend the generic
> properties.

If you have any custom properties, then yes. But you don't have.


Best regards,
Krzysztof