| Message ID | 20220331000449.41062-2-povik+lin@cutebit.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Apple Macs machine-level ASoC driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | success | |
| robh/dt-meta-schema | success |
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. > 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. > + 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? > + $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. > + required: > + - reg > + - cpu > + - codec > + additionalProperties: false This entire block is unreadable. > + > +required: > + - compatible > + - model > + > +additionalProperties: false > + > +examples: > + - | > + sound { > + compatible = "apple,j293-macaudio", "apple,macaudio"; > + model = "MacBook Pro J293 integrated audio"; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + dai-link@0 { > + reg = <0>; > + link-name = "Speakers"; > + mclk-fs = <64>; > + > + cpu { > + sound-dai = <&mca 0>, <&mca 1>; > + }; > + codec { > + sound-dai = <&speaker_left_front>, <&speaker_right_front>, > + <&speaker_left_rear>, <&speaker_right_rear>; Align the line. > + }; > + }; > + > + dai-link@1 { > + reg = <1>; > + link-name = "Headphones Jack"; > + mclk-fs = <64>; > + > + cpu { > + sound-dai = <&mca 2>; > + }; > + codec { > + sound-dai = <&jack_codec>; > + }; > + }; > + }; Best regards, Krzysztof
> 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. > >> 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’. > >> + 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. > >> + $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. > > Best regards, > Krzysztof Martin
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
> On 31. 3. 2022, at 10:17, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > 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:" Aha! Thanks. >>>> 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. 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. There are some constraints but technically not in the driver that binds on this binding. I thought no limit is better than an arbitrary one, but if the preference is to have one, I will add it, no problem. > Best regards, > Krzysztof Best, Martin
On 31/03/2022 10:23, Martin Povišer wrote: > >> 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. > > There are some constraints but technically not in the driver that binds > on this binding. I thought no limit is better than an arbitrary one, but > if the preference is to have one, I will add it, no problem. Just to clarify this - bindings are not about the driver, but about the hardware. We model here the hardware and its programming model, not the driver implementation (although of course it's always somehow related). Hardware has some limitations for sure. The question is whether we know them. :) I prefer even arbitrary limit, because then schema will check DTSes for simple mistakes. You can also explain this in commit msg, that maxItems are arbitrary, so whoever in the future wants to change it, will know the background. Best regards, Krzysztof
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> + +definitions: + 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 + "#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). + $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" + required: + - reg + - cpu + - codec + additionalProperties: false + +required: + - compatible + - model + +additionalProperties: false + +examples: + - | + sound { + compatible = "apple,j293-macaudio", "apple,macaudio"; + model = "MacBook Pro J293 integrated audio"; + + #address-cells = <1>; + #size-cells = <0>; + + dai-link@0 { + reg = <0>; + link-name = "Speakers"; + mclk-fs = <64>; + + cpu { + sound-dai = <&mca 0>, <&mca 1>; + }; + codec { + sound-dai = <&speaker_left_front>, <&speaker_right_front>, + <&speaker_left_rear>, <&speaker_right_rear>; + }; + }; + + dai-link@1 { + reg = <1>; + link-name = "Headphones Jack"; + mclk-fs = <64>; + + cpu { + sound-dai = <&mca 2>; + }; + codec { + sound-dai = <&jack_codec>; + }; + }; + };
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