RE: [PATCH 3/6] dt-bindings: sound: Add sound card bindings for Tesla FSD

["Padmanabhan Rajanbabu" <p.rajanbabu@samsung.com>]

> -----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
> >
> >