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

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

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