Re: [PATCH 03/13] dt-bindings: a2b: Analog Devices AD24xx devices

[Krzysztof Kozlowski <krzk@kernel.org>]

On 17/05/2024 14:58, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> Add device tree bindings for the AD24xx series A2B transceiver chips,
> including their functional blocks.
> 
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
>  .../devicetree/bindings/a2b/adi,ad24xx-clk.yaml    |  53 +++++

What is a2b and why clock bindings are not in clock?

>  .../devicetree/bindings/a2b/adi,ad24xx-codec.yaml  |  52 +++++
>  .../devicetree/bindings/a2b/adi,ad24xx-gpio.yaml   |  76 +++++++
>  .../devicetree/bindings/a2b/adi,ad24xx-i2c.yaml    |  55 +++++
>  .../devicetree/bindings/a2b/adi,ad24xx.yaml        | 253 +++++++++++++++++++++

Sorry, all this looks weirdly placed.

>  5 files changed, 489 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/a2b/adi,ad24xx-clk.yaml b/Documentation/devicetree/bindings/a2b/adi,ad24xx-clk.yaml
> new file mode 100644
> index 000000000000..819efaa6a3f9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/a2b/adi,ad24xx-clk.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/a2b/adi,ad24xx-clk.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices Inc. AD24xx clock functional block
> +
> +maintainers:
> +  - Alvin Šipraga <alsi@bang-olufsen.dk>
> +
> +allOf:
> +  - $ref: /schemas/clock/clock.yaml

Drop. There is no single binding doing this, which is usually a hint you
do something not correct.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad2420-clk
> +      - adi,ad2421-clk
> +      - adi,ad2422-clk
> +      - adi,ad2425-clk
> +      - adi,ad2426-clk
> +      - adi,ad2427-clk
> +      - adi,ad2428-clk
> +      - adi,ad2429-clk
> +

This is just incomplete. See other bindings how clock controller is written.

> +required:
> +  - compatible
> +  - clock-output-names
> +
> +unevaluatedProperties: false

additionalProperties: false
> +
> +examples:
> +  - |
> +    a2b {
> +      #address-cells = <1>;
> +      #size-cells = <0>;

Not related, drop entire node.

> +
> +      node@1 {
> +        compatible = "adi,ad2425-node";

Not related, drop entire node.

> +        reg = <1>;
> +        interrupts = <1>;
> +        adi,tdm-mode = <16>;
> +        adi,tdm-slot-size = <32>;
> +
> +        clock {
> +          compatible = "adi,ad2425-clk";
> +          #clock-cells = <1>;
> +          clock-indices = <1>;
> +          clock-output-names = "A2B1 CLKOUT2";
> +        };
> +      };
> +    };
> diff --git a/Documentation/devicetree/bindings/a2b/adi,ad24xx-codec.yaml b/Documentation/devicetree/bindings/a2b/adi,ad24xx-codec.yaml
> new file mode 100644
> index 000000000000..eee12f1c810e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/a2b/adi,ad24xx-codec.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/a2b/adi,ad24xx-codec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices Inc. AD24xx I2S/TDM functional block
> +
> +maintainers:
> +  - Alvin Šipraga <alsi@bang-olufsen.dk>
> +
> +allOf:
> +  - $ref: /schemas/sound/dai-common.yaml#

Why full path? It's the same directory, isn't it?

> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad2403-codec
> +      - adi,ad2410-codec
> +      - adi,ad2425-codec
> +      - adi,ad2428-codec
> +      - adi,ad2429-codec
> +
> +  '#sound-dai-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +  - '#sound-dai-cells'
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    a2b {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      node@2 {
> +        compatible = "adi,ad2428-node";
> +        reg = <2>;
> +        interrupts = <2>;
> +        adi,tdm-mode = <8>;
> +        adi,tdm-slot-size = <32>;

Same comments. Limited review follows.


...


> +
> +required:
> +  - compatible
> +
> +unevaluatedProperties: false

Sorry, but not. No resources, nothing here. Do not create bindings just
to instantiate drivers.


...

> diff --git a/Documentation/devicetree/bindings/a2b/adi,ad24xx.yaml b/Documentation/devicetree/bindings/a2b/adi,ad24xx.yaml
> new file mode 100644
> index 000000000000..dcda15e8032a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/a2b/adi,ad24xx.yaml
> @@ -0,0 +1,253 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/a2b/adi,ad24xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices Inc. AD24xx Automotive Audio Bus A2B Transceiver
> +
> +description: |
> +  AD24xx chips provide A2B bus functionality together with several peripheral

What is A2B?

> +  functions, including GPIO, I2S/TDM, an I2C controller interface, and
> +  programmable clock outputs.
> +
> +maintainers:
> +  - Alvin Šipraga <alsi@bang-olufsen.dk>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad2403
> +      - adi,ad2410
> +      - adi,ad2425
> +      - adi,ad2428
> +      - adi,ad2429
> +

reg: is second property.

> +  reg-names:
> +    items:
> +      - const: base
> +      - const: bus
> +
> +  reg:
> +    items:
> +      - description: Normal I2C address of the chip
> +      - description: Auxiliary BUS_ADDR I2C address of the chip
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  clock-names:
> +    items:
> +      - const: sync

Again misordered. -names always follow main entry. Anyway, clock-names
for just one entry is not really useful.

> +
> +  clocks:
> +    items:
> +      - description: SYNC input pin clock source
> +
> +  vin-supply:
> +    description: Optional regulator for supply voltage to VIN pin
> +
> +  bus-supply:
> +    description: Optional regulator for out-of-band supply voltage to
> +      subodrinate nodes' VIN pins
> +
> +  interrupts: true

??? This must be specific.

> +
> +  interrupt-controller: true
> +
> +  '#interrupt-cells':
> +    const: 1
> +
> +patternProperties:
> +  '^node@[0-9]+$':
> +    type: object
> +    unevaluatedProperties: false

Why? This must be additionalProperties: false, or I miss something...

> +
> +    properties:
> +      compatible:
> +        enum:
> +          - adi,ad2401-node
> +          - adi,ad2402-node
> +          - adi,ad2403-node
> +          - adi,ad2410-node
> +          - adi,ad2420-node
> +          - adi,ad2421-node
> +          - adi,ad2422-node
> +          - adi,ad2425-node
> +          - adi,ad2426-node
> +          - adi,ad2427-node
> +          - adi,ad2428-node
> +          - adi,ad2429-node
> +
> +      reg:
> +        maxItems: 1
> +
> +      interrupts:
> +        maxItems: 1
> +
> +      interrupt-controller: true
> +
> +      '#interrupt-cells':
> +        const: 1
> +
> +      gpio:
> +        $ref: adi,ad24xx-gpio.yaml#
> +
> +      codec:
> +        $ref: adi,ad24xx-codec.yaml#
> +
> +      i2c:
> +        $ref: adi,ad24xx-i2c.yaml#
> +
> +      clock:
> +        $ref: adi,ad24xx-clk.yaml#
> +
> +      adi,tdm-mode:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: TDM mode

Please do not add descriptions which are copies of property name. You
basically said ZERO here. Say something useful...

> +        enum: [2, 4, 8, 12, 16, 20, 24, 32]
> +
> +      adi,tdm-slot-size:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: TDM slot size
> +        enum: [16, 32]
> +
> +      adi,invert-sync:
> +        description: Falling edge of SYNC pin indicates the start of an audio
> +          frame, as opposed to rising edge.
> +        type: boolean
> +
> +      adi,early-sync:
> +        description: The SYNC pin changes one cycle before the MSB of the first
> +          data slot.
> +        type: boolean
> +
> +      adi,alternating-sync:
> +        description: Drive SYNC pin during first half of I2S/TDM data
> +          transmission rather than just pulsing it for once cycle.
> +        type: boolean
> +
> +      adi,rx-on-dtx1:
> +        description: Use the DTX1 pin for I2S/TDM RX in place of the DRX1 pin.
> +        type: boolean
> +
> +      adi,a2b-external-switch-mode-1:
> +        description: Use external switch mode 1 instead of 0 on the assumption
> +          that the downstream node is not using A2B bus power.
> +        type: boolean
> +
> +      adi,drive-strength:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: Configures drive strength low (0) or high (1, default).
> +        enum: [0, 1]
> +        default: 1
> +
> +      adi,invert-interrupt:
> +        description: Invert polarity of IRQ pin, making it active low.
> +        type: boolean
> +
> +      adi,tristate-interrupt:
> +        description: Rather than always actively driving the IRQ pin, only drive
> +          when the interrupt is active and otherwise set to tristate (high-Z).
> +        type: boolean

It looks you put all children properties into parent node... With so
little explanation tricky to judge.

> +
> +    required:
> +      - compatible
> +      - reg
> +      - adi,tdm-mode
> +      - adi,tdm-slot-size
> +
> +    dependencies:
> +      interrupt-controller: [ '#interrupt-cells' ]
> +      '#interrupt-cells': [ interrupt-controller ]
> +
> +required:
> +  - compatible
> +  - reg-names
> +  - reg
> +  - clock-names
> +  - clocks
> +  - '#address-cells'
> +  - '#size-cells'
> +  - interrupts
> +  - interrupt-controller
> +  - '#interrupt-cells'
> +  - node@0
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    sync_clk: sync-clock {

Drop, not related.

> +          compatible = "fixed-clock";
> +          #clock-cells = <0>;
> +          clock-frequency = <48000>;
> +    };
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      a2b@68 {
> +        compatible = "adi,ad2428";
> +        reg-names = "base", "bus";
> +        reg = <0x68>, <0x69>;


Please follow DTS coding style. Do not introduce entire different style
and order of properties. reg-names IS NEVER the second property.



Best regards,
Krzysztof