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

From: <Sergiu.Moga@microchip.com>
To: <krzysztof.kozlowski@linaro.org>, <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 15:27:46 +0000	[thread overview]
Message-ID: <753d73d0-44b9-9fba-1ed8-53691ecf2ee7@microchip.com> (raw)
In-Reply-To: <1d3904d9-7fec-2e61-f999-61b89c4becb6@linaro.org>

On 08.09.2022 18:10, Krzysztof Kozlowski wrote:
> 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.

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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel