Re: [PATCH v2 05/13] dt-bindings: serial: atmel,at91-usart: convert to json-schema

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Sergiu.Moga@microchip.com, lee@kernel.org, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, Nicolas.Ferre@microchip.com,
	alexandre.belloni@bootlin.com, Claudiu.Beznea@microchip.com,
	richard.genoud@gmail.com, radu_nicolae.pirea@upb.ro,
	gregkh@linuxfoundation.org, broonie@kernel.org,
	mturquette@baylibre.com, sboyd@kernel.org, jirislaby@kernel.org,
	admin@hifiphile.com, Kavyasree.Kotagiri@microchip.com,
	Tudor.Ambarus@microchip.com
Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org,
	linux-serial@vger.kernel.org, linux-clk@vger.kernel.org
Subject: Re: [PATCH v2 05/13] dt-bindings: serial: atmel,at91-usart: convert to json-schema
Date: Thu, 8 Sep 2022 17:10:17 +0200	[thread overview]
Message-ID: <1d3904d9-7fec-2e61-f999-61b89c4becb6@linaro.org> (raw)
In-Reply-To: <fde16d80-28b4-aae6-363f-ad9dcf87a5e1@microchip.com>

On 08/09/2022 17:06, Sergiu.Moga@microchip.com wrote:
> On 08.09.2022 15:29, Krzysztof Kozlowski wrote:

>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +  - clock-names
>>> +  - clocks
>>> +
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        $nodename:
>>> +          pattern: "^serial@[0-9a-f]+$"
>>
>> You should rather check value of atmel,usart-mode, because now you won't
>> properly match device nodes called "foobar". Since usart-mode has only
>> two possible values, this will nicely simplify you if-else.
>>
>>
> 
> 
> I did think of that but the previous binding specifies that 
> atmel,usart-mode is required only for the SPI mode and it is optional 
> for the USART mode. That is why I went for the node's regex since I 
> thought it is something that both nodes would have.

I think it should be explicit - you configure node either to this or
that, so the property should be always present. The node name should not
be responsible for it, even though we want node names to match certain
patterns.

> 
> 
>>> +    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:
>>> +            Maximum number of data the RX and TX FIFOs can store for FIFO
>>> +            capable USARTS.
>>> +          enum: [ 16, 32 ]
>>
>> I did not mention it last time, but I think it should follow generic
>> practice, so define all properties top-level and disallow them for other
>> type. This allows you to simply use additionalProperties:false at the end.
>>
> 
> 
> What would be a good example binding in this case?

The example binding.

https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L212

> 
> 
>>> +
>>> +    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
>>
>> The same - top level and disallow them for uart.
>>
> 
> 
> These values of #size-cells and #address-cells are only meant for the 
> SPI so I guess I would still have to specify their mandatory const 
> values here.

Sure, ok.

> 
> 
>>> +
>>> +        required:
>>> +          - atmel,usart-mode
>>> +          - "#size-cells"
>>> +          - "#address-cells"
>>
>> End else in this branch is what?
>>
> 
> 
> You are right, I will remove the useless if: after else:

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel