Re: [PATCH 11/12] dt-bindings: sound: Add Cirrus Logic CS48L31/32/33 codecs

On 09/11/2022 17:53, Richard Fitzgerald wrote:

Codecs in this family have multiple digital and analog audio I/O that support a variety of external hardware connections and configurations.

Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com

.../bindings/sound/cirrus,cs48l32.yaml | 96 +++++++++++++++++++ include/dt-bindings/sound/cs48l32.h | 25 +++++ 2 files changed, 121 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml create mode 100644 include/dt-bindings/sound/cs48l32.h

diff --git a/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml b/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml new file mode 100644 index 000000000000..70fb294c6dc1 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/cirrus,cs48l32.yaml @@ -0,0 +1,96 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/cirrus,cs48l32.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: Cirrus Logic CS48L31/32/33 audio CODECs

+maintainers:

• • patches@opensource.cirrus.com

+description: |

• This describes audio configuration bindings for these codecs.

Don't start with "This". Instead describe the hardware.

• See also the core bindings for the parent MFD driver:
• Documentation/devicetree/bindings/mfd/cirrus,cs48l32.yaml

Same comment as for pinctrl patch.

• and defines for values used in these bindings:
• include/dt-bindings/sound/cs48l32.h
• The properties are all contained in the parent MFD node.

+properties:

Missing compatible. What's the point to organize bindings like that? The schema on its own does nothing - does not match anything.

• '#sound-dai-cells':
• const: 1
• cirrus,in-type:
• description:

•  A list of input type settings for each input. A maximum of 8 cells,
  

•  with four cells per input in the order INnL_1, INnR_1 INnL_2 INnR_2.
  

•  (where _1 and _2 are the alternative mux selections for that INn).
  

•  If the array is shorter than the number of inputs the unspecified
  

•  inputs default to CS48L32_IN_TYPE_DIFF.
  

• $ref: "/schemas/types.yaml#/definitions/uint32-matrix"

Drop quotes.

• minItems: 1
• maxItems: 8
• items:

•  items:
  

•    - description:
  

•        The first cell is INnL_1 input type. One of the CS48L32_IN_TYPE_xxx.
  

•        For non-muxed inputs this sets the type of INnL.
  

What is the "input type"? Referring to constants is not enough, especially that they are not descriptive. Explain here the values.

•        minimum: 0
  

•        maximum: 1
  

•    - description:
  

•        The second cell is INnR_1 input type. One of the CS48L32_IN_TYPE_xxx.
  

•        For non-muxed inputs this sets the type of INnR.
  

•        minimum: 0
  

•        maximum: 1
  

•    - description:
  

•        The third cell is INnL_2 input type. One of the CS48L32_IN_TYPE_xxx.
  

•        For non-muxed inputs this cell must be 0.
  

•        minimum: 0
  

•        maximum: 1
  

•    - description:
  

•        The fourth cell is INnR_2 input type. One of the CS48L32_IN_TYPE_xxx.
  

•        For non-muxed inputs this cell must be 0.
  

•        minimum: 0
  

•        maximum: 1
  

• cirrus,max-channels-clocked:
• description:

•  Maximum number of channels that clocks will be generated for. When using
  

•  multiple data lines, every sample slot can transfer multiple channels
  

•  (one per data line). This pdata sets the maximum number of slots.
  

•  One cell for each ASP, use a value of zero for ASPs that should be
  

•  handled normally.
  

• $ref: /schemas/types.yaml#/definitions/uint32-array
• minItems: 1
• maxItems: 4
• items:

•  default: 0
  

• cirrus,pdm-sup:
• description:

•  Indicates how the MICBIAS pins have been externally connected to DMICs
  

•  on each input. One cell per input (IN1, IN2, ...). One of the
  

•  CS48L32_MICBIAS_xxx values.
  

•  See the INn_PDM_SUP field in the datasheet for a description.
  

No, explain here.

• $ref: /schemas/types.yaml#/definitions/uint32-array
• minItems: 1
• maxItems: 4

+examples:

• • |

•    cs48l32@0 {
  

Node names should be generic. https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetre...

•            compatible = "cirrus,cs48l32";
  

Use 4 spaces for example indentation.

•            cirrus,in-type = <
  

•                    CS48L32_IN_TYPE_DIFF CS48L32_IN_TYPE_DIFF /* IN1[LR]_1 differential */
  

•                    CS48L32_IN_TYPE_SE   CS48L32_IN_TYPE_SE   /* IN1[LR]_2 single-ended */
  

•                    CS48L32_IN_TYPE_DIFF CS48L32_IN_TYPE_DIFF /* IN2[LR]_1 differential */
  

•            >;
  

•            cirrus,max-channels-clocked = <2 0 0>;
  

•    };
  

diff --git a/include/dt-bindings/sound/cs48l32.h b/include/dt-bindings/sound/cs48l32.h new file mode 100644 index 000000000000..0b774da0a6c8 --- /dev/null +++ b/include/dt-bindings/sound/cs48l32.h @@ -0,0 +1,25 @@ +/* SPDX-License-Identifier: GPL-2.0-only */

Dual license.

+/*

• • Device Tree defines for CS48L32 codec.
• • Copyright (C) 2016-2018, 2022 Cirrus Logic, Inc. and

• •           Cirrus Logic International Semiconductor Ltd.
    

• */

+#ifndef DT_BINDINGS_SOUND_CS48L32_H +#define DT_BINDINGS_SOUND_CS48L32_H

+#define CS48L32_IN_TYPE_DIFF 0 +#define CS48L32_IN_TYPE_SE 1

+#define CS48L32_PDM_SUP_VOUT_MIC 0 +#define CS48L32_PDM_SUP_MICBIAS1 1 +#define CS48L32_PDM_SUP_MICBIAS2 2 +#define CS48L32_PDM_SUP_MICBIAS3 3

+#define CS48L32_PDM_FMT_MODE_A_LSB_FIRST 0x0000 +#define CS48L32_PDM_FMT_MODE_B_LSB_FIRST 0x4000 +#define CS48L32_PDM_FMT_MODE_A_MSB_FIRST 0x8000 +#define CS48L32_PDM_FMT_MODE_B_MSB_FIRST 0xc000

Register values do not belong to bindings.

+#endif

Best regards, Krzysztof