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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 31 Mar 2022 10:17:46 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Martin Povišer <povik@...ebit.org>
Cc:     Martin Povišer <povik+lin@...ebit.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Mark Kettenis <kettenis@...nbsd.org>,
        Hector Martin <marcan@...can.st>,
        Sven Peter <sven@...npeter.dev>
Subject: Re: [RFC PATCH 1/5] dt-bindings: sound: Add Apple Macs sound system

On 31/03/2022 08:57, Martin Povišer wrote:
> 
>> On 31. 3. 2022, at 8:43, Krzysztof Kozlowski <krzysztof.kozlowski@...aro.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@...ebit.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@...ebit.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