Re: [PATCH 2/5] dt-bindings: mfd: atmel,at91-usart: convert to json-schema

[<Sergiu.Moga@xxxxxxxxxxxxx>]

On 18.08.2022 11:38, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 17/08/2022 10:55, Sergiu Moga wrote:
>> Convert at91 USART DT Binding for Atmel/Microchip SoCs to
>> json-schema format.
>>
>> Signed-off-by: Sergiu Moga <sergiu.moga@xxxxxxxxxxxxx>
>> ---
>>   .../bindings/mfd/atmel,at91-usart.yaml        | 190 ++++++++++++++++++
>>   .../devicetree/bindings/mfd/atmel-usart.txt   |  98 ---------
>>   2 files changed, 190 insertions(+), 98 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>   delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>> new file mode 100644
>> index 000000000000..cf15d73fa1e8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>> @@ -0,0 +1,190 @@
>> +# 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/mfd/atmel,at91-usart.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
>> +
>> +maintainers:
>> +  - Richard Genoud <richard.genoud@xxxxxxxxx>
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
> This looks quite different than bindings and you commit msg is saying it
> is only a conversion. Mention any changes against original bindings.


Noted. Will make the commit message more descriptive to also include the 
new compatibles.  Perhaps the addition of the GCLK should come under a 
different patch as well.


>> +      - const: atmel,at91rm9200-usart
>> +      - const: atmel,at91sam9260-usart
>> +      - const: microchip,sam9x60-usart
> That's an enum
>
>> +      - items:
>> +          - const: atmel,at91rm9200-dbgu
>> +          - const: atmel,at91rm9200-usart
>> +      - items:
>> +          - const: atmel,at91sam9260-dbgu
>> +          - const: atmel,at91sam9260-usart
>> +      - items:
>> +          - const: microchip,sam9x60-dbgu
>> +          - const: microchip,sam9x60-usart
>> +      - items:
>> +          - const: microchip,sam9x60-usart
>> +          - const: atmel,at91sam9260-usart
> This is not correct - contradicts earlier one.
>

Yes, this was for a DT node we have internally, my bad. You are right, 
it does not really make sense and it should not be the other way around: 
having the DT validate the binding. I will remove this combination in 
the next version.


>> +      - items:
>> +          - const: microchip,sam9x60-dbgu
>> +          - const: microchip,sam9x60-usart
>> +          - const: atmel,at91sam9260-dbgu
>> +          - const: atmel,at91sam9260-usart
> What? You wrote above that microchip,sam9x60-dbgu is compatible only
> with microchip,sam9x60-usart. Now you write it is also compatible with
> other ones?


Yes, this one is intended because the 9x60 IP has new functionalities on 
top of 9260, and some nodes do keep all four as fallback.


>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    contains:
>> +      const: usart
> No, this has to be specific/fixed list.


I wanted to highlight the fact that it must contain either:
clock-names = "usart";
or
clock-names = "usart", "gclk";

What would be the recommended way of doing this then?

>> +
>> +  clocks:
>> +    minItems: 1
>> +    maxItems: 2
> Not really - define the items. One item could be optional, though.
>
>> +
>> +  dmas:
>> +    items:
>> +      - description: TX DMA Channel
>> +      - description: RX DMA Channel
>> +
>> +  dma-names:
>> +    items:
>> +      - const: tx
>> +      - const: rx
>> +
>> +  atmel,usart-mode:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: |
> No need for |
>
>> +      Must be either 1 for SPI or 0 for USART.
> Mention the header.
>
>> +    enum: [ 0, 1 ]
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clock-names
>> +  - clocks
>> +
>> +if:
> Put it under allOf.
>
>> +  properties:
>> +    $nodename:
>> +      pattern: "^serial@[0-9a-f]+$"
>> +then:
>> +  allOf:
>> +    - $ref: /schemas/serial/serial.yaml#
>> +    - $ref: /schemas/serial/rs485.yaml#
>> +
>> +  properties:
>> +    atmel,use-dma-rx:
>> +      type: boolean
>> +      description: use of PDC or DMA for receiving data
>> +
>> +    atmel,use-dma-tx:
>> +      type: boolean
>> +      description: use of PDC or DMA for transmitting data
>> +
>> +    atmel,fifo-size:
>> +      $ref: /schemas/types.yaml#/definitions/uint32
>> +      description: |
> No need for |
>
>> +        Maximum number of data the RX and TX FIFOs can store for FIFO
>> +        capable USARTS.
>> +      enum: [ 16, 32 ]
>> +
>> +else:
>> +  if:
>> +    properties:
>> +      $nodename:
>> +        pattern: "^spi@[0-9a-f]+$"
>> +  then:
>> +    allOf:
>> +      - $ref: /schemas/spi/spi-controller.yaml#
>> +
>> +    properties:
>> +      atmel,usart-mode:
>> +        const: 1
>> +
>> +      "#size-cells":
>> +        const: 0
>> +
>> +      "#address-cells":
>> +        const: 1
>> +
>> +    required:
>> +      - atmel,usart-mode
>> +      - "#size-cells"
>> +      - "#address-cells"
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    #include <dt-bindings/mfd/at91-usart.h>
>> +    #include <dt-bindings/dma/at91.h>
>> +
>> +    /* use PDC */
>> +    usart0: serial@fff8c000 {
>> +        compatible = "atmel,at91sam9260-usart";
>> +        reg = <0xfff8c000 0x4000>;
>> +        interrupts = <7>;
>> +        clocks = <&usart0_clk>;
>> +        clock-names = "usart";
>> +        atmel,use-dma-rx;
>> +        atmel,use-dma-tx;
>> +        rts-gpios = <&pioD 15 GPIO_ACTIVE_LOW>;
>> +        cts-gpios = <&pioD 16 GPIO_ACTIVE_LOW>;
>> +        dtr-gpios = <&pioD 17 GPIO_ACTIVE_LOW>;
>> +        dsr-gpios = <&pioD 18 GPIO_ACTIVE_LOW>;
>> +        dcd-gpios = <&pioD 20 GPIO_ACTIVE_LOW>;
>> +        rng-gpios = <&pioD 19 GPIO_ACTIVE_LOW>;
>> +    };
>> +
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    #include <dt-bindings/mfd/at91-usart.h>
>> +    #include <dt-bindings/dma/at91.h>
>> +
>> +    /* use DMA */
>> +    usart1: serial@f001c000 {
>> +        compatible = "atmel,at91sam9260-usart";
>> +        reg = <0xf001c000 0x100>;
>> +        interrupts = <12 IRQ_TYPE_LEVEL_HIGH 5>;
>> +        clocks = <&usart0_clk>;
>> +        clock-names = "usart";
>> +        atmel,use-dma-rx;
>> +        atmel,use-dma-tx;
>> +        dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(3)>,
>> +               <&dma0 2 (AT91_DMA_CFG_PER_ID(4) | AT91_DMA_CFG_FIFOCFG_ASAP)>;
>> +        dma-names = "tx", "rx";
>> +        atmel,fifo-size = <32>;
>> +    };
>> +
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    #include <dt-bindings/mfd/at91-usart.h>
>> +    #include <dt-bindings/dma/at91.h>
>> +
>> +    /* SPI mode */
>> +    spi0: spi@f001c000 {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +        compatible = "atmel,at91sam9260-usart";
>> +        atmel,usart-mode = <AT91_USART_MODE_SPI>;
>> +        reg = <0xf001c000 0x100>;
> compatible, then reg then the reset of properties
>
>> +        interrupts = <12 IRQ_TYPE_LEVEL_HIGH 5>;
>> +        clocks = <&usart0_clk>;
>> +        clock-names = "usart";
>> +        dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(3)>,
>> +               <&dma0 2 (AT91_DMA_CFG_PER_ID(4) | AT91_DMA_CFG_FIFOCFG_ASAP)>;
>> +        dma-names = "tx", "rx";
>> +        cs-gpios = <&pioB 3 GPIO_ACTIVE_HIGH>;
>> +    };
> Best regards,
> Krzysztof


Regards,
     Sergiu