Re: [PATCH 2/3] dt-bindings: i2c: convert i2c-at91 to json-schema

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

On 09/03/2022 13:07, Sergiu Moga wrote:
> Convert I2C binding for Atmel/Microchip SoCs to Device Tree Schema
> format.
> 
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> ---
>  .../bindings/i2c/atmel,at91sam-i2c.yaml       | 144 ++++++++++++++++++
>  .../devicetree/bindings/i2c/i2c-at91.txt      |  82 ----------
>  2 files changed, 144 insertions(+), 82 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/i2c/atmel,at91sam-i2c.yaml
>  delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-at91.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/atmel,at91sam-i2c.yaml b/Documentation/devicetree/bindings/i2c/atmel,at91sam-i2c.yaml
> new file mode 100644
> index 000000000000..d4aadbbd1a11
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/atmel,at91sam-i2c.yaml
> @@ -0,0 +1,144 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2022 Microchip Technology, Inc. and its subsidiaries
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/atmel,at91sam-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: I2C for Atmel/Microchip platforms
> +
> +maintainers:
> +  - Alexandre Belloni <alexandre.belloni@bootlin.com>
> +
> +allOf:
> +  - $ref: "i2c-controller.yaml"
> +  - if:

allOf with additional if: goes by convention after required:.

> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - atmel,sama5d4-i2c
> +              - atmel,sama5d2-i2c
> +              - microchip,sam9x60-i2c
> +    then:
> +      properties:
> +        i2c-sda-hold-time-ns:
> +          description:
> +            TWD hold time
> +          maxItems: 1
> +
> +properties:
> +  compatible:
> +    enum:
> +      - atmel,at91rm9200-i2c
> +      - atmel,at91sam9261-i2c
> +      - atmel,at91sam9260-i2c
> +      - atmel,at91sam9g20-i2c
> +      - atmel,at91sam9g10-i2c
> +      - atmel,at91sam9x5-i2c
> +      - atmel,sama5d4-i2c
> +      - atmel,sama5d2-i2c
> +      - microchip,sam9x60-i2c
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-frequency:
> +    default: 100000
> +
> +  dmas:
> +    items:
> +      - description: TX DMA Channel Specifier
> +      - description: RX DMA Channel Specifier
> +
> +  dma-names:
> +    items:
> +      - const: tx
> +      - const: rx
> +
> +  atmel,fifo-size:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Maximum number of data the RX and TX FIFOs can store for
> +      FIFO capable I2C controllers.
> +
> +  scl-gpios: true
> +
> +  sda-gpios: true
> +
> +  pinctrl:
> +    description: |
> +      Add extra pinctrl to configure i2c pins to gpio function for i2c
> +      bus recovery, call it "gpio" state

This is a generic property, you do not need it.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - "#address-cells"
> +  - "#size-cells"
> +  - clocks
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/dma/at91.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c0: i2c@fff84000 {
> +        compatible = "atmel,at91sam9g20-i2c";
> +        reg = <0xfff84000 0x100>;
> +        interrupts = <12 IRQ_TYPE_LEVEL_HIGH 6>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        clocks = <&twi0_clk>;
> +        clock-frequency = <400000>;
> +
> +        eeprom@50 {
> +            compatible = "atmel,24c512";
> +            reg = <0x50>;
> +            pagesize = <128>;
> +        };
> +    };
> +
> +    i2c1: i2c@f8034600 {
> +        compatible = "atmel,sama5d2-i2c";
> +        reg = <0xf8034600 0x100>;
> +        interrupts = <19 IRQ_TYPE_LEVEL_HIGH 7>;
> +        dmas = <&dma0
> +            (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1))
> +            AT91_XDMAC_DT_PERID(11)>,
> +               <&dma0
> +            (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1))
> +            AT91_XDMAC_DT_PERID(12)>;
> +        dma-names = "tx", "rx";
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        clocks = <&flx0>;
> +        atmel,fifo-size = <16>;
> +        i2c-sda-hold-time-ns = <336>;
> +        pinctrl-names = "default", "gpio";
> +        pinctrl-0 = <&pinctrl_i2c0>;
> +        pinctrl-1 = <&pinctrl_i2c0_gpio>;
> +        sda-gpios = <&pioA 30 GPIO_ACTIVE_HIGH>;
> +        scl-gpios = <&pioA 31 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> +
> +        eeprom@1a {
> +            compatible = "wm8731";

It seems this is undocumented compatible. Could you use something else,
documented?

Best regards,
Krzysztof