Re: [PATCH 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings

Hi,

On Fri, Jul 17, 2020 at 5:02 AM Cheng-Yi Chiang cychiang@chromium.org wrote:

Add devicetree bindings documentation file for sc7180 sound card.

Signed-off-by: Cheng-Yi Chiang cychiang@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@codeaurora.org
• • Cheng-Yi Chiang cychiang@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.

+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.

• 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.

+properties:

• compatible:
• contains:

•  enum:
  

•    - qcom,sc7180-sndcard
  

Just:

properties: compatible: const: qcom,sc7180-sndcard

• 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.

• 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.

...also, again your description doesn't need the "|" at the end. Maybe https://yaml-multiline.info/ will be useful to you?

• 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?

•    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.

...also, shouldn't the mi2s pinctrl be in the i2s nodes, not in the overall sound node?

•    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