Re: [PATCH v2 09/14] ASoC: dt-bindings: qcom,wsa881x: extend description to analog mode

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On Thu, Dec 12, 2024 at 12:47:22AM +0000, Alexey Klimov wrote:
> WSA881X also supports analog mode when device is configured via i2c
> only. Document it, add properties, new compatibles and example.
> 
> While at this, also adjust quotes.
> 
> Cc: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> Signed-off-by: Alexey Klimov <alexey.klimov@xxxxxxxxxx>
> ---
>  .../bindings/sound/qcom,wsa881x.yaml          | 75 +++++++++++++++++--
>  1 file changed, 67 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/qcom,wsa881x.yaml b/Documentation/devicetree/bindings/sound/qcom,wsa881x.yaml
> index ac03672ebf6d..e482d9dc0de2 100644
> --- a/Documentation/devicetree/bindings/sound/qcom,wsa881x.yaml
> +++ b/Documentation/devicetree/bindings/sound/qcom,wsa881x.yaml
> @@ -12,15 +12,17 @@ maintainers:
>  description: |
>    WSA8810 is a class-D smart speaker amplifier and WSA8815
>    is a high-output power class-D smart speaker amplifier.
> -  Their primary operating mode uses a SoundWire digital audio
> -  interface. This binding is for SoundWire interface.
> -
> -allOf:
> -  - $ref: dai-common.yaml#
> +  This family of amplifiers support two operating modes:
> +  SoundWire digital audio interface which is a primary mode
> +  and analog mode when device is configured via i2c only.
> +  This binding describes both modes.
>  
>  properties:
>    compatible:
> -    const: sdw10217201000
> +    enum:
> +      - qcom,wsa8810
> +      - qcom,wsa8815

You implement only one compatible, so does it mean they are compatible?
If so, make them compatible.

> +      - sdw10217201000
>  
>    reg:
>      maxItems: 1
> @@ -35,17 +37,60 @@ properties:
>    '#sound-dai-cells':
>      const: 0
>  
> +  clocks:
> +    maxItems: 1
> +
> +  mclk-gpios:
> +    description: GPIO spec for mclk

Do not repeat property name as description. Say something useful. "GPIO
spec for" is redundant, it cannot be anything else, so basically your
description saod "mclk" which is the same as in property name.

Usually clocks are not GPIOs, so description could explain that.

> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg
>    - powerdown-gpios
> -  - "#thermal-sensor-cells"
> -  - "#sound-dai-cells"
> +  - '#thermal-sensor-cells'
> +  - '#sound-dai-cells'
> +
> +allOf:
> +  - $ref: dai-common.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,wsa8810
> +            const: qcom,wsa8815

That's not a valid syntax. Either enum or const.

This was never tested.

> +    then:
> +      properties:
> +        reg:
> +          description:
> +            In case of analog mode this should be I2C address of the digital
> +            part of the device. The I2C address of analog part of an amplifier
> +            is expected to be located at the fixed offset.
> +          maxItems: 1
> +          items:
> +            minimum: 0x0e
> +            maximum: 0x0f
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,wsa8810
> +            const: qcom,wsa8815

Why are you repeating the if?

> +    then:
> +      required:
> +        - clocks
> +        - mclk-gpios
>  
>  unevaluatedProperties: false
>  
>  examples:
>    - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/sound/qcom,q6afe.h>
> +
>      soundwire@c2d0000 {
>          #address-cells = <2>;
>          #size-cells = <0>;
> @@ -68,4 +113,18 @@ examples:
>          };
>      };
>  
> +    i2c0 {

i2c

Best regards,
Krzysztof