On 08.09.2022 18:10, Krzysztof Kozlowski wrote: > On 08/09/2022 17:06, Sergiu.Moga@xxxxxxxxxxxxx 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. 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. >> >> >>>> + 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! >> >> >>>> + >>>> + 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 Regards, Sergiu
