Re: [PATCH 1/2] ASoC: dt-bindings: Add cmx655 codec

[Krzysztof Kozlowski <krzk@kernel.org>]

On 03/02/2025 18:01, Nikola Jelic wrote:
> +
> +title: CML Micro CMX655D codec
> +
> +maintainers:
> +  - Richard Walton <rwalton@cmlmicro.com>
> +  - Nikola Jelic <nikola.jelic83@gmail.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  The CMX655D is an ultra-low power voice codec.
> +
> +allOf:
> +  - $ref: dai-common.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - cml,cmx655d
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#sound-dai-cells":
> +    const: 0
> +
> +  reset-gpios:
> +    description: GPIO used for codec reset, negative logic
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-names:
> +    maxItems: 1

Describe the names or drop. Look how other bindings do it - there is
never syntax like that alone.

> +
> +  pinctrl-names:
> +    maxItems: 1

Drop property, not needed here.

> +
> +  pinctr-0:
> +    maxItems: 1

Drop property, not needed and not correct.

> +

> +  cmx655,classd-oc-reset-attempts:
> +    description: Maximum number of times to reset CMX655 class-D
> +      following a overcurrent event.
> +      Default = 5, >10000 = disable limit.

Missing constraints, min/max. Drop also redundant part - schema already
says default 5.

But more important, I am not sure why this is board-configurable.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 5
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        status = "okay";

Drop

> +        codec: cmx655 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


audio-codec, codec etc.

> +            reg = <0x54>;
> +            #sound-dai-cells = <0>;
> +            compatible = "cml,cmx655d";

Follow DTS coding style. compatible must be first.

> +            reset-gpios = <&gpio 24 1>;

Use defines for standard flags.

> +            interrupt-parent = <&gpio>;
> +            interrupts = <25 0x2>;

Use defines for standard flags.

> +            interrupt-names = "irq";

Drop

> +            pinctrl-names = "default";
> +            pinctrl-0 = <&ev6550DHAT_pins>;



Best regards,
Krzysztof