[PATCH 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
Cheng-yi Chiang
cychiang at chromium.org
Tue Jul 21 07:29:55 EDT 2020
On Fri, Jul 17, 2020 at 11:03 PM Doug Anderson <dianders at chromium.org> wrote:
>
> Hi,
>
Thanks for the review!
> On Fri, Jul 17, 2020 at 5:02 AM Cheng-Yi Chiang <cychiang at chromium.org> wrote:
> >
> > Add devicetree bindings documentation file for sc7180 sound card.
> >
> > Signed-off-by: Cheng-Yi Chiang <cychiang at chromium.org>
> > ---
> > .../bindings/sound/qcom,sc7180.yaml | 123 ++++++++++++++++++
> > 1 file changed, 123 insertions(+)
>
> A bit of a mechanical review since my audio knowledge is not strong.
>
>
> > create mode 100644 Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
> > new file mode 100644
> > index 000000000000..d60d2880d991
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
> > @@ -0,0 +1,123 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/qcom,sc7180.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm Technologies Inc. SC7180 ASoC sound card driver
> > +
> > +maintainers:
> > + - Rohit kumar <rohitkr at codeaurora.org>
> > + - Cheng-Yi Chiang <cychiang at chromium.org>
> > +
> > +description: |
> > + This binding describes the SC7180 sound card, which uses LPASS for audio.
>
> nit: you don't need the pipe at the end of the "description" line.
> That means that newlines are important and you don't need it.
>
>
Thanks for the explanation. Fixed in v2.
> > +definitions:
>
> I haven't yet seen much yaml using definitions like this. It feels
> like overkill for some of these properties, especially ones that are
> only ever used once in the "properties:" section and are/or are really
> simple.
>
>
ACK. In v2 I only kept dai definition and removed others.
> > + link-name:
> > + description: Indicates dai-link name and PCM stream name.
> > + $ref: /schemas/types.yaml#/definitions/string
> > + maxItems: 1
> > +
> > + dai:
> > + type: object
> > + properties:
> > + sound-dai:
> > + maxItems: 1
> > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > + description: phandle array of the codec or CPU DAI
> > +
> > + required:
> > + - sound-dai
> > +
> > + unidirectional:
> > + description: Specify direction of unidirectional dai link.
> > + 0 for playback only. 1 for capture only.
> > + $ref: /schemas/types.yaml#/definitions/uint32
>
> So if the property isn't there then it's _not_ unidirectional and if
> it is there then this specifies the direction, right? I almost wonder
> if this should just be two boolean properties, like:
>
> playback-only;
> capture-only;
>
> ...but I guess I'd leave it to Rob and/or Mark to say what they liked
> better. In any case if you keep it how you have it then you should
> use yaml to force it to be either 0 or 1 if present.
>
>
ACK
Use playback-only and capture-only in v2 instead.
> > +
> > +properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,sc7180-sndcard
>
> Just:
>
> properties:
> compatible:
> const: qcom,sc7180-sndcard
>
>
Fixed in v2.
>
> > + audio-routing:
> > + $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> > + 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.
>
> You don't need the "|-" after the "description:". That says newlines
> are important but strip the newline from the end.
>
Fixed in v2.
>
> > + model:
> > + $ref: /schemas/types.yaml#/definitions/string
> > + description: User specified audio sound card name
> > +
> > +patternProperties:
> > + "^dai-link-[0-9]+$":
> > + description: |
> > + Each subnode represents a dai link. Subnodes of each dai links would be
> > + cpu/codec dais.
>
> From looking at "simple-card.yaml", I'm gonna guess that instead of
> encoding the link number in the name of the node that you should
> actually use a unit address and a reg in the subnodes.
Thanks for the explanation. Fixed in v2.
I think this naming is better, although there is no usage in the
machine driver for the reg.
>
> ...also, again your description doesn't need the "|" at the end.
> Maybe <https://yaml-multiline.info/> will be useful to you?
>
>
Thanks for the explanation and the pointer!
> > + type: object
> > +
> > + properties:
> > + link-name:
> > + $ref: "#/definitions/link-name"
> > +
> > + unidirectional:
> > + $ref: "#/definitions/unidirectional"
> > +
> > + cpu:
> > + $ref: "#/definitions/dai"
> > +
> > + codec:
> > + $ref: "#/definitions/dai"
> > +
> > + required:
> > + - link-name
> > + - cpu
> > + - codec
> > +
> > + additionalProperties: false
> > +
> > +examples:
> > +
> > + - |
> > + snd {
>
> Can you use the full node name "sound" here?
>
>
Fixed in v2.
> > + compatible = "qcom,sc7180-sndcard";
> > + model = "sc7180-snd-card";
> > +
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&sec_mi2s_active &sec_mi2s_dout_active
> > + &sec_mi2s_ws_active &pri_mi2s_active
> > + &pri_mi2s_dout_active &pri_mi2s_ws_active
> > + &pri_mi2s_din_active &pri_mi2s_mclk_active>;
>
> I think pinctrl is usually not in the dt examples.
>
Fixed in v2.
> ...also, shouldn't the mi2s pinctrl be in the i2s nodes, not in the
> overall sound node?
Yes. Thanks for pointing this out. Fixed in dts file in chromium.
>
>
> > + audio-routing =
> > + "Headphone Jack", "HPOL",
> > + "Headphone Jack", "HPOR";
> > +
> > + dai-link-0 {
> > + link-name = "MultiMedia0";
> > + cpu {
> > + sound-dai = <&lpass_cpu 0>;
> > + };
> > +
> > + codec {
> > + sound-dai = <&alc5682 0>;
> > + };
> > + };
> > +
> > + dai-link-1 {
> > + link-name = "MultiMedia1";
> > + unidirectional = <0>;
> > + cpu {
> > + sound-dai = <&lpass_cpu 1>;
> > + };
> > +
> > + codec {
> > + sound-dai = <&max98357a>;
> > + };
> > + };
> > + };
> > --
> > 2.28.0.rc0.105.gf9edc3c819-goog
> >
More information about the linux-arm-kernel
mailing list