Re: [PATCH v2 2/6] dt-bindings: sound: cirrus,cs42l43: Add initial DT binding

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 30/05/2023 14:21, Charles Keepax wrote:
> The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface
> (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed
> for portable applications. It provides a high dynamic range, stereo
> DAC for headphone output, two integrated Class D amplifiers for
> loudspeakers, and two ADCs for wired headset microphone input or
> stereo line input. PDM inputs are provided for digital microphones.
> 
> Add a YAML DT binding document for this device.
> 
> Signed-off-by: Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>

Thank you for your patch. There is something to discuss/improve.


> +  clocks:
> +    items:
> +      - description: Synchronous audio clock provided on mclk_in.
> +
> +  clock-names:
> +    const: mclk
> +
> +  cirrus,bias-low:
> +    type: boolean
> +    description:
> +      Select a 1.8V headset micbias rather than 2.8V.
> +
> +  cirrus,bias-sense-ua:

"ua" looks like microamp. If so, microamp instead:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Current at which the headset micbias sense clamp will engage, 0 to
> +      disable.
> +    enum: [ 0, 14, 23, 41, 50, 60, 68, 86, 95 ]
> +    default: 0
> +
> +  cirrus,bias-ramp-ms:
> +    description:
> +      Time in milliseconds the hardware allows for the headset micbias to
> +      ramp up.
> +    enum: [ 10, 40, 90, 170 ]
> +    default: 170
> +
> +  cirrus,detect-us:
> +    description:
> +      Time in microseconds the type detection will run for. Long values will
> +      cause more audible effects, but give more accurate detection.
> +    enum: [ 20, 100, 1000, 10000, 50000, 75000, 100000, 200000 ]
> +    default: 10000
> +
> +  cirrus,button-automute:
> +    type: boolean
> +    description:
> +      Enable the hardware automuting of decimator 1 when a headset button is
> +      pressed.
> +
> +  cirrus,buttons-ohms:
> +    description:
> +      Impedance in Ohms for each headset button, these should be listed in
> +      ascending order.
> +    minItems: 1
> +    maxItems: 6
> +
> +  cirrus,tip-debounce-ms:
> +    description:
> +      Software debounce on tip sense triggering in milliseconds.
> +    default: 0
> +
> +  cirrus,tip-invert:
> +    type: boolean
> +    description:
> +      Indicates tip detect polarity, inverted implies open-circuit whilst the
> +      jack is inserted.
> +
> +  cirrus,tip-disable-pullup:
> +    type: boolean
> +    description:
> +      Indicates if the internal pullup on the tip detect should be disabled.
> +
> +  cirrus,tip-fall-db-ms:
> +    description:
> +      Time in milliseconds a falling edge on the tip detect should be hardware
> +      debounced for. Note the falling edge is considered after the invert.
> +    enum: [ 0, 125, 250, 500, 750, 1000, 1250, 1500 ]
> +    default: 500
> +
> +  cirrus,tip-rise-db-ms:
> +    description:
> +      Time in milliseconds a rising edge on the tip detect should be hardware
> +      debounced for. Note the rising edge is considered after the invert.
> +    enum: [ 0, 125, 250, 500, 750, 1000, 1250, 1500 ]
> +    default: 500
> +
> +  cirrus,use-ring-sense:
> +    type: boolean
> +    description:
> +      Indicates if the ring sense should be used.
> +
> +  cirrus,ring-invert:
> +    type: boolean
> +    description:
> +      Indicates ring detect polarity, inverted implies open-circuit whilst the
> +      jack is inserted.
> +
> +  cirrus,ring-disable-pullup:
> +    type: boolean
> +    description:
> +      Indicates if the internal pullup on the ring detect should be disabled.
> +
> +  cirrus,ring-fall-db-ms:
> +    description:
> +      Time in milliseconds a falling edge on the ring detect should be hardware
> +      debounced for. Note the falling edge is considered after the invert.
> +    enum: [ 0, 125, 250, 500, 750, 1000, 1250, 1500 ]
> +    default: 500
> +
> +  cirrus,ring-rise-db-ms:
> +    description:
> +      Time in milliseconds a rising edge on the ring detect should be hardware
> +      debounced for. Note the rising edge is considered after the invert.
> +    enum: [ 0, 125, 250, 500, 750, 1000, 1250, 1500 ]
> +    default: 500
> +
> +  pinctrl:
> +    type: object
> +
> +    allOf:

Drop allOf, just "$ref: ......"

> +      - $ref: /schemas/pinctrl/pinctrl.yaml#
> +
> +    additionalProperties: false

Also drop blank lines between these three above.

> +
> +    properties:
> +      gpio-controller: true
> +
> +      '#gpio-cells':
> +        const: 2
> +
> +      gpio-ranges:
> +        items:
> +          - description: A phandle to the CODEC pinctrl node
> +            minimum: 0
> +          - const: 0
> +          - const: 0
> +          - const: 3
> +
> +    patternProperties:
> +      "-state$":

Use consistent quotes, either " or ' everywhere

> +        oneOf:
> +          - $ref: "#/$defs/cirrus-cs42l43-state"
> +          - patternProperties:
> +              "-pins$":
> +                $ref: "#/$defs/cirrus-cs42l43-state"
> +            additionalProperties: false
> +
> +  spi:
> +    type: object
> +
> +    allOf:
> +      - $ref: /schemas/spi/spi-controller.yaml#
> +
> +    unevaluatedProperties: false

Same comments here.

> +
> +$defs:
> +  cirrus-cs42l43-state:
> +    type: object
> +



Best regards,
Krzysztof