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

[Sergiu.Moga]

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