diff mbox series

[PATCHv4,9/9] ASoC: dt-bindings: fsl-asoc-card: add compatible for generic codec

Message ID 20240515135411.343333-10-elinor.montmasson@savoirfairelinux.com
State New
Headers show
Series [PATCHv4,1/9] ASoC: fsl-asoc-card: add support for dai links with multiple codecs | expand

Commit Message

Elinor Montmasson May 15, 2024, 1:54 p.m. UTC
Add documentation about new dts bindings following new support
for compatible "fsl,imx-audio-generic".

Some CPU DAI don't require a real audio codec. The new compatible
"fsl,imx-audio-generic" allows using the driver with codec drivers
SPDIF DIT and SPDIF DIR as dummy codecs.
It also allows using not pre-configured audio codecs which
don't require specific control through a codec driver.

The new dts properties give the possibility to set some parameters
about the CPU DAI usually set through the codec configuration.

Signed-off-by: Elinor Montmasson <elinor.montmasson@savoirfairelinux.com>
---
 .../bindings/sound/fsl-asoc-card.yaml         | 96 ++++++++++++++++++-
 1 file changed, 92 insertions(+), 4 deletions(-)

Comments

Mark Brown May 16, 2024, 12:11 p.m. UTC | #1
On Wed, May 15, 2024 at 03:54:11PM +0200, Elinor Montmasson wrote:

> Add documentation about new dts bindings following new support
> for compatible "fsl,imx-audio-generic".

>    audio-codec:
> -    $ref: /schemas/types.yaml#/definitions/phandle
> -    description: The phandle of an audio codec
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: |
> +      The phandle of an audio codec.
> +      If using the "fsl,imx-audio-generic" compatible, give instead a pair of
> +      phandles with the spdif_transmitter first (driver SPDIF DIT) and the
> +      spdif_receiver second (driver SPDIF DIR).
> +    items:
> +      maxItems: 1

This description (and the code) don't feel like they're actually generic
- they're clearly specific to the bidrectional S/PDIF case.  I'd expect
something called -generic to cope with single CODECs as well as double,
and not to have any constraints on what those are.
Elinor Montmasson May 17, 2024, 9:05 a.m. UTC | #2
From: "Mark Brown" <broonie@kernel.org>
Sent: Thursday, 16 May, 2024 14:11:11
> On Wed, May 15, 2024 at 03:54:11PM +0200, Elinor Montmasson wrote:
> 
>> Add documentation about new dts bindings following new support
>> for compatible "fsl,imx-audio-generic".
> 
>>    audio-codec:
>> -    $ref: /schemas/types.yaml#/definitions/phandle
>> -    description: The phandle of an audio codec
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description: |
>> +      The phandle of an audio codec.
>> +      If using the "fsl,imx-audio-generic" compatible, give instead a pair of
>> +      phandles with the spdif_transmitter first (driver SPDIF DIT) and the
>> +      spdif_receiver second (driver SPDIF DIR).
>> +    items:
>> +      maxItems: 1
> 
> This description (and the code) don't feel like they're actually generic
> - they're clearly specific to the bidrectional S/PDIF case.  I'd expect
> something called -generic to cope with single CODECs as well as double,
> and not to have any constraints on what those are.

I proposed, in an reply of the v3 patch series to Krzysztof Kozlowski,
the compatible "fsl,imx-audio-no-codec" instead of "generic".
Krzysztof thought it was too generic, but it would convey more clearly
that it is for cases without codec driver.
Would this other compatible string be more appropriate ?

Best regards,
Elinor Montmasson
Mark Brown May 17, 2024, 11:11 a.m. UTC | #3
On Fri, May 17, 2024 at 05:05:41AM -0400, Elinor Montmasson wrote:
> From: "Mark Brown" <broonie@kernel.org>

> > This description (and the code) don't feel like they're actually generic
> > - they're clearly specific to the bidrectional S/PDIF case.  I'd expect
> > something called -generic to cope with single CODECs as well as double,
> > and not to have any constraints on what those are.

> I proposed, in an reply of the v3 patch series to Krzysztof Kozlowski,
> the compatible "fsl,imx-audio-no-codec" instead of "generic".
> Krzysztof thought it was too generic, but it would convey more clearly
> that it is for cases without codec driver.
> Would this other compatible string be more appropriate ?

No.  There is very clearly a CODEC here, it physically exists, we can
point at it on the board and it has a software representation.  Your
code is also very specific to the two CODEC case.
Rob Herring (Arm) May 20, 2024, 6:31 p.m. UTC | #4
On Wed, May 15, 2024 at 03:54:11PM +0200, Elinor Montmasson wrote:
> Add documentation about new dts bindings following new support
> for compatible "fsl,imx-audio-generic".
> 
> Some CPU DAI don't require a real audio codec. The new compatible
> "fsl,imx-audio-generic" allows using the driver with codec drivers
> SPDIF DIT and SPDIF DIR as dummy codecs.
> It also allows using not pre-configured audio codecs which
> don't require specific control through a codec driver.
> 
> The new dts properties give the possibility to set some parameters
> about the CPU DAI usually set through the codec configuration.
> 
> Signed-off-by: Elinor Montmasson <elinor.montmasson@savoirfairelinux.com>
> ---
>  .../bindings/sound/fsl-asoc-card.yaml         | 96 ++++++++++++++++++-
>  1 file changed, 92 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml b/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml
> index 9922664d5ccc..332d8bf96e06 100644
> --- a/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml
> +++ b/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml
> @@ -23,6 +23,16 @@ description:
>    and PCM DAI formats. However, it'll be also possible to support those non
>    AC'97/I2S/PCM type sound cards, such as S/PDIF audio and HDMI audio, as
>    long as the driver has been properly upgraded.
> +  To use CPU DAIs that do not require a codec such as an S/PDIF controller,
> +  or to use a DAI to output or capture raw I2S/TDM data, you can
> +  use the compatible "fsl,imx-audio-generic".
> +
> +definitions:
> +  imx-audio-generic-dependency:
> +    properties:
> +      compatible:
> +        contains:
> +          const: fsl,imx-audio-generic
>  
>  maintainers:
>    - Shengjiu Wang <shengjiu.wang@nxp.com>
> @@ -81,6 +91,7 @@ properties:
>                - fsl,imx-audio-wm8960
>                - fsl,imx-audio-wm8962
>                - fsl,imx-audio-wm8958
> +              - fsl,imx-audio-generic
>  
>    model:
>      $ref: /schemas/types.yaml#/definitions/string
> @@ -93,8 +104,14 @@ properties:
>        need to add ASRC support via DPCM.
>  
>    audio-codec:
> -    $ref: /schemas/types.yaml#/definitions/phandle
> -    description: The phandle of an audio codec
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: |
> +      The phandle of an audio codec.
> +      If using the "fsl,imx-audio-generic" compatible, give instead a pair of
> +      phandles with the spdif_transmitter first (driver SPDIF DIT) and the
> +      spdif_receiver second (driver SPDIF DIR).

       minItems: 1
       maxItems: 2

> +    items:
> +      maxItems: 1
>  
>    audio-cpu:
>      $ref: /schemas/types.yaml#/definitions/phandle
> @@ -150,8 +167,8 @@ properties:
>      description: dai-link uses bit clock inversion.
>  
>    mclk-id:
> -    $ref: /schemas/types.yaml#/definitions/uint32
> -    description: main clock id, specific for each card configuration.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description: Main clock id for each codec, specific for each card configuration.

       minItems: 1
       maxItems: 2
>  
>    mux-int-port:
>      $ref: /schemas/types.yaml#/definitions/uint32
> @@ -167,10 +184,68 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/phandle
>      description: The phandle of an CPU DAI controller
>  
> +  # Properties relevant only with "fsl,imx-audio-generic" compatible
> +  dai-tdm-slot-width:
> +    description: See tdm-slot.txt.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  dai-tdm-slot-num:
> +    description: See tdm-slot.txt.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  clocks:
> +    description: |
> +      The CPU DAI system clock, used to retrieve
> +      the CPU DAI system clock frequency with the generic codec.
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: cpu_sysclk
> +
> +  cpu-system-clock-direction-out:
> +    description: |
> +      Specifies cpu system clock direction as 'out' on initialization.
> +      If not set, direction is 'in'.
> +    $ref: /schemas/types.yaml#/definitions/flag
> +
> +dependencies:
> +  dai-tdm-slot-width:
> +    $ref: "#/definitions/imx-audio-generic-dependency"
> +  dai-tdm-slot-num:
> +    $ref: "#/definitions/imx-audio-generic-dependency"
> +  clocks:
> +    $ref: "#/definitions/imx-audio-generic-dependency"
> +  cpu-system-clock-direction-out:
> +    $ref: "#/definitions/imx-audio-generic-dependency"

This works, but is an unusual pattern...

> +
>  required:
>    - compatible
>    - model
>  
> +allOf:
> +  - if:
> +      $ref: "#/definitions/imx-audio-generic-dependency"
> +    then:
> +      properties:
> +        audio-codec:
> +          items:
> +            - description: SPDIF DIT phandle
> +            - description: SPDIF DIR phandle
> +        mclk-id:
> +          maxItems: 1
> +          items:
> +            minItems: 1
> +            maxItems: 2
> +    else:
> +      properties:
> +        audio-codec:
> +          maxItems: 1
> +        mclk-id:
> +          maxItems: 1
> +          items:
> +            maxItems: 1


You can handle the dependency like this instead:

           dai-tdm-slot-width: false
           dai-tdm-slot-num: false


And then you don't need the definitions.

> +
>  unevaluatedProperties: false
>  
>  examples:
> @@ -195,3 +270,16 @@ examples:
>               "AIN2L", "Line In Jack",
>               "AIN2R", "Line In Jack";
>      };
> +
> +  - |
> +    #include <dt-bindings/clock/imx8mn-clock.h>
> +    sound-spdif-asrc {
> +      compatible = "fsl,imx-audio-generic";
> +      model = "spdif-asrc-audio";
> +      audio-cpu = <&spdif>;
> +      audio-asrc = <&easrc>;
> +      audio-codec = <&spdifdit>, <&spdifdir>;
> +      clocks = <&clk IMX8MN_CLK_SAI5_ROOT>;
> +      clock-names = "cpu_sysclk";
> +      cpu-system-clock-direction-out;
> +    };
> -- 
> 2.34.1
>
Elinor Montmasson May 31, 2024, 12:48 p.m. UTC | #5
From: "Mark Brown" <broonie@kernel.org>
Sent: Friday, 17 May, 2024 13:11:43
> On Fri, May 17, 2024 at 05:05:41AM -0400, Elinor Montmasson wrote:
>> From: "Mark Brown" <broonie@kernel.org>
> 
>> > This description (and the code) don't feel like they're actually generic
>> > - they're clearly specific to the bidrectional S/PDIF case.  I'd expect
>> > something called -generic to cope with single CODECs as well as double,
>> > and not to have any constraints on what those are.
> 
>> I proposed, in an reply of the v3 patch series to Krzysztof Kozlowski,
>> the compatible "fsl,imx-audio-no-codec" instead of "generic".
>> Krzysztof thought it was too generic, but it would convey more clearly
>> that it is for cases without codec driver.
>> Would this other compatible string be more appropriate ?
> 
> No.  There is very clearly a CODEC here, it physically exists, we can
> point at it on the board and it has a software representation.  Your
> code is also very specific to the two CODEC case.

Then maybe it's not be a good idea to make this compatible generic
for this contribution.
The original intention is to bring support for the S/PDIF,
so maybe the contribution should focus on this use case?
In that case, would changing the compatible for "fsl,imx-audio-spdif-card"
be acceptable?
"fsl,imx-audio-spdif" is already used for the `imx-spdif.c`
which does not use the ASRC.
Mark Brown May 31, 2024, 1:14 p.m. UTC | #6
On Fri, May 31, 2024 at 08:48:04AM -0400, Elinor Montmasson wrote:

> Then maybe it's not be a good idea to make this compatible generic
> for this contribution.
> The original intention is to bring support for the S/PDIF,
> so maybe the contribution should focus on this use case?
> In that case, would changing the compatible for "fsl,imx-audio-spdif-card"
> be acceptable?
> "fsl,imx-audio-spdif" is already used for the `imx-spdif.c`
> which does not use the ASRC.

Why not just use the existing compatible - why would someone not want to
be able to use the ASRC if it's available in their system?
Elinor Montmasson May 31, 2024, 2:48 p.m. UTC | #7
From: "Mark Brown" <broonie@kernel.org>
Sent: Friday, 31 May, 2024 15:14:13

> On Fri, May 31, 2024 at 08:48:04AM -0400, Elinor Montmasson wrote:
> 
>> Then maybe it's not be a good idea to make this compatible generic
>> for this contribution.
>> The original intention is to bring support for the S/PDIF,
>> so maybe the contribution should focus on this use case?
>> In that case, would changing the compatible for "fsl,imx-audio-spdif-card"
>> be acceptable?
>> "fsl,imx-audio-spdif" is already used for the `imx-spdif.c`
>> which does not use the ASRC.
> 
> Why not just use the existing compatible - why would someone not want to
> be able to use the ASRC if it's available in their system?

That's true but it will be a problem if both `fsl-asoc-card.c` and
`imx-spdif.c` drivers have the same compatible, and they don't
have the same DT properties.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml b/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml
index 9922664d5ccc..332d8bf96e06 100644
--- a/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml
+++ b/Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml
@@ -23,6 +23,16 @@  description:
   and PCM DAI formats. However, it'll be also possible to support those non
   AC'97/I2S/PCM type sound cards, such as S/PDIF audio and HDMI audio, as
   long as the driver has been properly upgraded.
+  To use CPU DAIs that do not require a codec such as an S/PDIF controller,
+  or to use a DAI to output or capture raw I2S/TDM data, you can
+  use the compatible "fsl,imx-audio-generic".
+
+definitions:
+  imx-audio-generic-dependency:
+    properties:
+      compatible:
+        contains:
+          const: fsl,imx-audio-generic
 
 maintainers:
   - Shengjiu Wang <shengjiu.wang@nxp.com>
@@ -81,6 +91,7 @@  properties:
               - fsl,imx-audio-wm8960
               - fsl,imx-audio-wm8962
               - fsl,imx-audio-wm8958
+              - fsl,imx-audio-generic
 
   model:
     $ref: /schemas/types.yaml#/definitions/string
@@ -93,8 +104,14 @@  properties:
       need to add ASRC support via DPCM.
 
   audio-codec:
-    $ref: /schemas/types.yaml#/definitions/phandle
-    description: The phandle of an audio codec
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: |
+      The phandle of an audio codec.
+      If using the "fsl,imx-audio-generic" compatible, give instead a pair of
+      phandles with the spdif_transmitter first (driver SPDIF DIT) and the
+      spdif_receiver second (driver SPDIF DIR).
+    items:
+      maxItems: 1
 
   audio-cpu:
     $ref: /schemas/types.yaml#/definitions/phandle
@@ -150,8 +167,8 @@  properties:
     description: dai-link uses bit clock inversion.
 
   mclk-id:
-    $ref: /schemas/types.yaml#/definitions/uint32
-    description: main clock id, specific for each card configuration.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description: Main clock id for each codec, specific for each card configuration.
 
   mux-int-port:
     $ref: /schemas/types.yaml#/definitions/uint32
@@ -167,10 +184,68 @@  properties:
     $ref: /schemas/types.yaml#/definitions/phandle
     description: The phandle of an CPU DAI controller
 
+  # Properties relevant only with "fsl,imx-audio-generic" compatible
+  dai-tdm-slot-width:
+    description: See tdm-slot.txt.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  dai-tdm-slot-num:
+    description: See tdm-slot.txt.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  clocks:
+    description: |
+      The CPU DAI system clock, used to retrieve
+      the CPU DAI system clock frequency with the generic codec.
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: cpu_sysclk
+
+  cpu-system-clock-direction-out:
+    description: |
+      Specifies cpu system clock direction as 'out' on initialization.
+      If not set, direction is 'in'.
+    $ref: /schemas/types.yaml#/definitions/flag
+
+dependencies:
+  dai-tdm-slot-width:
+    $ref: "#/definitions/imx-audio-generic-dependency"
+  dai-tdm-slot-num:
+    $ref: "#/definitions/imx-audio-generic-dependency"
+  clocks:
+    $ref: "#/definitions/imx-audio-generic-dependency"
+  cpu-system-clock-direction-out:
+    $ref: "#/definitions/imx-audio-generic-dependency"
+
 required:
   - compatible
   - model
 
+allOf:
+  - if:
+      $ref: "#/definitions/imx-audio-generic-dependency"
+    then:
+      properties:
+        audio-codec:
+          items:
+            - description: SPDIF DIT phandle
+            - description: SPDIF DIR phandle
+        mclk-id:
+          maxItems: 1
+          items:
+            minItems: 1
+            maxItems: 2
+    else:
+      properties:
+        audio-codec:
+          maxItems: 1
+        mclk-id:
+          maxItems: 1
+          items:
+            maxItems: 1
+
 unevaluatedProperties: false
 
 examples:
@@ -195,3 +270,16 @@  examples:
              "AIN2L", "Line In Jack",
              "AIN2R", "Line In Jack";
     };
+
+  - |
+    #include <dt-bindings/clock/imx8mn-clock.h>
+    sound-spdif-asrc {
+      compatible = "fsl,imx-audio-generic";
+      model = "spdif-asrc-audio";
+      audio-cpu = <&spdif>;
+      audio-asrc = <&easrc>;
+      audio-codec = <&spdifdit>, <&spdifdir>;
+      clocks = <&clk IMX8MN_CLK_SAI5_ROOT>;
+      clock-names = "cpu_sysclk";
+      cpu-system-clock-direction-out;
+    };