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

2020-07-21 Thread Cheng-yi Chiang 
On Fri, Jul 17, 2020 at 11:03 PM Doug Anderson  wrote:
>
> Hi,
>

Thanks for the review!

> On Fri, Jul 17, 2020 at 5:02 AM Cheng-Yi Chiang  wrote:
> >
> > Add devicetree bindings documentation file for sc7180 sound card.
> >
> > Signed-off-by: Cheng-Yi Chiang 
> > ---
> >  .../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 ..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 
> > +  - Cheng-Yi Chiang 
> > +
> > +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  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

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

2020-07-17 Thread Doug Anderson 
Hi,

On Fri, Jul 17, 2020 at 5:02 AM Cheng-Yi Chiang  wrote:
>
> Add devicetree bindings documentation file for sc7180 sound card.
>
> Signed-off-by: Cheng-Yi Chiang 
> ---
>  .../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 ..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 
> +  - Cheng-Yi Chiang 
> +
> +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  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 = <_mi2s_active _mi2s_dout_active
> + _mi2s_ws_active _mi2s_active
> + _mi2s_dout_active _mi2s_ws_active
> + _mi2s_din_active _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";
> +

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

2020-07-17 Thread Cheng-Yi Chiang 
Add devicetree bindings documentation file for sc7180 sound card.

Signed-off-by: Cheng-Yi Chiang 
---
 .../bindings/sound/qcom,sc7180.yaml   | 123 ++
 1 file changed, 123 insertions(+)
 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 ..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 
+  - Cheng-Yi Chiang 
+
+description: |
+  This binding describes the SC7180 sound card, which uses LPASS for audio.
+
+definitions:
+
+  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
+
+properties:
+  compatible:
+contains:
+  enum:
+- 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.
+
+  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.
+
+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 {
+compatible = "qcom,sc7180-sndcard";
+model = "sc7180-snd-card";
+
+pinctrl-names = "default";
+pinctrl-0 = <_mi2s_active _mi2s_dout_active
+ _mi2s_ws_active _mi2s_active
+ _mi2s_dout_active _mi2s_ws_active
+ _mi2s_din_active _mi2s_mclk_active>;
+
+audio-routing =
+"Headphone Jack", "HPOL",
+"Headphone Jack", "HPOR";
+
+dai-link-0 {
+link-name = "MultiMedia0";
+cpu {
+sound-dai = <_cpu 0>;
+};
+
+codec {
+sound-dai = < 0>;
+};
+};
+
+dai-link-1 {
+link-name = "MultiMedia1";
+unidirectional = <0>;
+cpu {
+sound-dai = <_cpu 1>;
+};
+
+codec {
+sound-dai = <>;
+};
+};
+};
-- 
2.28.0.rc0.105.gf9edc3c819-goog