Re: [PATCH] ASoC: dt-bindings: cirrus,cs4271: Convert to dtschema

[Krzysztof Kozlowski <krzk@kernel.org>]

On 26/08/2024 11:53, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> Convert the Cirrus Logic CS4271 audio CODEC bindings to DT schema.
> 
> Cc: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> Link: https://lore.kernel.org/all/20240715-ep93xx-v11-0-4e924efda795@maquefel.me

I think Link goes above your SoB. Your Sob must be the last tag from you.

> ---
> This is complementary patch to ep93xx DT conversion series.

...

> +
> +title: Cirrus Logic CS4271 audio CODEC
> +
> +maintainers:
> +  - Alexander Sverdlin <alexander.sverdlin@gmail.com>
> +  - Nikita Shubin <nikita.shubin@maquefel.me>
> +
> +description:
> +  The CS4271 is a stereo audio codec. This driver supports both the I2C
> +  and the SPI bus.

Drop references to driver. You can say that device supports
configuration over I2C and SPI.

> +
> +allOf:
> +  - $ref: dai-common.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    const: cirrus,cs4271
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-cpha: true
> +
> +  spi-cpol: true
> +
> +  '#sound-dai-cells':
> +    const: 0

None of these three were present in original binding.

> +
> +  reset-gpio:

If it bases on cs4270, look there - there is no "gpio", but "gpios".
Mention changes in commit msg.


> +    description:
> +      This pin will be deasserted before communication to the codec starts.
> +    maxItems: 1
> +
> +  va-supply:
> +    description: Analog power supply.
> +
> +  vd-supply:
> +    description: Digital power supply.
> +
> +  vl-supply:
> +    description: Serial Control Port power supply.
> +
> +  port:
> +    $ref: audio-graph-port.yaml#
> +    unevaluatedProperties: false

This wasn't present in original binding and nothing in commit msg
explained changes from pure conversion. Explain them in commit msg.

> +
> +  cirrus,amuteb-eq-bmutec:
> +    description:
> +      When given, the Codec's AMUTEB=BMUTEC flag is enabled.
> +    type: boolean
> +
> +  cirrus,enable-soft-reset:
> +    description: |
> +      The CS4271 requires its LRCLK and MCLK to be stable before its RESET
> +      line is de-asserted. That also means that clocks cannot be changed
> +      without putting the chip back into hardware reset, which also requires
> +      a complete re-initialization of all registers.
> +
> +      One (undocumented) workaround is to assert and de-assert the PDN bit
> +      in the MODE2 register. This workaround can be enabled with this DT
> +      property.
> +
> +      Note that this is not needed in case the clocks are stable
> +      throughout the entire runtime of the codec.
> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        cs4271: codec@0 {

Drop unused label.

> +            compatible = "cirrus,cs4271";
> +            reg = <0>;
> +            reset-gpio = <&gpio0 1 0>;

Use defines for flags.

Also, make the example complete. You  miss many properties.

> +        };
> +    };
> +
> +...
> \ No newline at end of file

This needs to be always fixed before posting.



Best regards,
Krzysztof