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

From: Krzysztof Kozlowski
Date: Mon Nov 14 2022 - 03:45:31 EST

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@xxxxxxxxxxxxxxxxxxxxx>
> ---
> .../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@xxxxxxxxxxxxxxxxxxxxx
> +
> +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-devicetree-basics.html#generic-names-recommendation

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