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 18:05:47 +0200	[thread overview]
Message-ID: <17d142eb-80c1-0aa5-c4fa-a8e4ccd97f08@linaro.org> (raw)
In-Reply-To: <753d73d0-44b9-9fba-1ed8-53691ecf2ee7@microchip.com>

On 08/09/2022 17:27, Sergiu.Moga@microchip.com wrote:
> 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.

Which is a problem because...?

Have in mind that bindings can be changed. ABI here won't be broken.

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

Not really, because if it is "foobar" your schema would not be applied
correctly.

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

Not for properties which are for example missing...

Best regards,
Krzysztof

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