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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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