Re: [RFC PATCH 1/5] dt-bindings: sound: Add Apple Macs sound system

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

On 31/03/2022 08:57, Martin Povišer wrote:
> 
>> On 31. 3. 2022, at 8:43, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 31/03/2022 02:04, Martin Povišer wrote:
>>> Add binding for Apple Silicon Macs' machine-level sound system.
>>>
>>> Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
>>> ---
>>> .../bindings/sound/apple,macaudio.yaml        | 103 ++++++++++++++++++
>>> 1 file changed, 103 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/sound/apple,macaudio.yaml
>>>
>>
>> Commit title does not match subsystem.
> 
> Tell more please. I don’t see it.

git log --oneline  -- Documentation/devicetree/bindings/sound/


Mark expects "ASoC: dt-bindings:"

> 
>>
>>> diff --git a/Documentation/devicetree/bindings/sound/apple,macaudio.yaml b/Documentation/devicetree/bindings/sound/apple,macaudio.yaml
>>> new file mode 100644
>>> index 000000000000..a6380e4bdd1a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/sound/apple,macaudio.yaml
>>> @@ -0,0 +1,103 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/sound/apple,macaudio.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Apple Silicon Macs integrated sound system
>>> +
>>> +maintainers:
>>> +  - Martin Povišer <povik+lin@cutebit.org>
>>> +
>>
>> Add description.
>>
>>> +definitions:
>>
>> This does not make code more readable.
> 
> Are you sure? It prevents duplication later on for ‘codec' and ‘cpu’.

That's true, but duplication is small, unless you think this will be
extended. I guess it is a trade-off, but so far for few lines and just
two users of such definition, I would prefer to duplicate. I don't have
strong opinion, though.

> 
>>
>>> +  dai:
>>> +    type: object
>>> +    properties:
>>> +      sound-dai: true
>>> +    required:
>>> +      - sound-dai
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - apple,j274-macaudio
>>> +          - apple,j293-macaudio
>>> +          - apple,j314-macaudio
>>> +      - const: apple,macaudio
>>
>> Open example-schema.yaml and look at formatting plus general coding
>> style. You miss line breaks making it unreadable.
>>
>>> +  "#address-cells":
>>> +    const: 1
>>> +  "#size-cells":
>>> +    const: 0
>>> +  model:
>>> +    description: |
>>> +      Model name to use when the sound system is presented to users as a sound card.
>>> +    $ref: /schemas/types.yaml#/definitions/string
>>> +
>>> +patternProperties:
>>> +  "^dai-link(@[0-9a-f]+)?$":
>>> +    description: |
>>> +      A DAI link comprising of CPU and CODEC DAI specifiers and supplemental properties.
>>> +    type: object
>>> +    properties:
>>> +      reg:
>>> +        maxItems: 1
>>> +      mclk-fs:
>>> +        description: |
>>> +          Forced MCLK/samplerate factor (optional).
>>
>> Optional is obvious from !required.
>>
>> Description is different than existing field in simple card. Is this the
>> same field or not?
> 
> It is the same. I didn’t want to copy the simple card text because this is optionally BSD,
> simple card wasn’t.

OK

> 
>>
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +      link-name:
>>> +        description: Name for the DAI link to present to users.
>>> +        $ref: /schemas/types.yaml#/definitions/string
>>> +      cpu:
>>> +        $ref: "#/definitions/dai"
>>> +      codec:
>>> +        $ref: "#/definitions/dai"
>>
>> missing maxItems for DAI phandles.
> 
> Well there’s not a maximum.

There should be some maximum of supported codecs. Hardware might have
such constraints. If really unsure, choose some reasonable (small)
amount. It could be later raised, if needed.


Best regards,
Krzysztof