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

On 08.09.2022 18:10, Krzysztof Kozlowski wrote:

On 08/09/2022 17:06, Sergiu.Moga@microchip.com wrote:

quoted

On 08.09.2022 15:29, Krzysztof Kozlowski wrote:

quoted

quoted

quoted

+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.

No DT of ours has that property atm, since they are all on USART mode by 
default. If I were to make it required. all nodes would fail so I would 
have to add it to each of them.

The node name should not
be responsible for it, even though we want node names to match certain
patterns.

Does checkig for the node's pattern not make it better then? Since it 
imposes an additional check? If it would not have a conventional 
pattern, it would fail through unevaluatedProperies:false at the end, 
since it would have properties that were contained inside a branch that 
the validation of the node would not have gone through since it contains 
a pattern that does not match the conditions of that branch.

quoted

quoted

quoted

+    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

Ah, I understand now. I did not get what you meant by "disallow", I 
guess it's just a "property-name: false".
Thanks!

quoted

quoted

quoted

+
+    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.

quoted

quoted

quoted

+
+        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

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