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

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On Mon, Feb 10, 2025 at 11:28:46PM +0100, Nikola Jelic wrote:
> Signed-off-by: Nikola Jelic <nikola.jelic83@xxxxxxxxx>
> 

Nothing improved here. Respond to previous comments or implement them.

Some things got better in this patch, thank you. Other did not. Maybe
there is misunderstanding about my comments, but you never replied to
them, so I would assume you will be implementing them fully. If
something is unclear, ask responding inline.


> ---
> V2 -> V3: fixed dt_binding_check + yamllint warnings
> V1 -> V2: removed the txt file, fixed review remarks
> ---
>  .../bindings/sound/cml,cmx655d.yaml           | 78 +++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
>  2 files changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/cml,cmx655d.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/cml,cmx655d.yaml b/Documentation/devicetree/bindings/sound/cml,cmx655d.yaml
> new file mode 100644
> index 000000000000..1648f606bf48
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/cml,cmx655d.yaml
> @@ -0,0 +1,78 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/cml,cmx655d.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: CML Micro CMX655D codec
> +
> +maintainers:
> +  - Richard Walton <rwalton@xxxxxxxxxxxx>
> +  - Nikola Jelic <nikola.jelic83@xxxxxxxxx>
> +
> +description:
> +  The CMX655D is an ultra-low power voice codec.
> +
> +allOf:
> +  - $ref: dai-common.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - cml,cmx655d
> +
> +  reg:
> +    description: Local bus address

Drop, why did this appear?

> +    maxItems: 1
> +
> +  "#sound-dai-cells":
> +    description: The first cell indicating the audio interface

Drop, redundant. It wasn't here before and I did not ask for this, so why?

> +    const: 0
> +
> +  reset-gpios:
> +    description: GPIO used for codec reset, negative logic
> +    maxItems: 1
> +
> +  interrupts:
> +    description: Interrupt for the CMX655D IRQ line

Drop

> +    maxItems: 1
> +
> +  interrupt-names:
> +    description: Interrupt name for the CMX655D IRQ line

Nothing improved. Maybe there was some misunderstanding. I asked to look
at other bindings doing this. Where do you see syntax like that for
interrupt-names? What syntax do you see for interrupt-names?

> +    maxItems: 1
> +
> +  cml,classd-oc-reset-attempts:
> +    description: Maximum number of times to reset CMX655 class-D
> +      following a overcurrent event.
> +      >10000 = disable limit.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 10001
> +    default: 5

Still no answer why this is board configurable.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        status = "okay";

Another ignored comment.

> +        audio-codec@54 {
> +            compatible = "cml,cmx655d";
> +            #sound-dai-cells = <0>;
> +            reg = <0x54>;

Another...


Best regards,
Krzysztof