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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <753d73d0-44b9-9fba-1ed8-53691ecf2ee7@microchip.com>
Date:   Thu, 8 Sep 2022 15:27:46 +0000
From:   <Sergiu.Moga@...rochip.com>
To:     <krzysztof.kozlowski@...aro.org>, <lee@...nel.org>,
        <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <Nicolas.Ferre@...rochip.com>, <alexandre.belloni@...tlin.com>,
        <Claudiu.Beznea@...rochip.com>, <richard.genoud@...il.com>,
        <radu_nicolae.pirea@....ro>, <gregkh@...uxfoundation.org>,
        <broonie@...nel.org>, <mturquette@...libre.com>,
        <sboyd@...nel.org>, <jirislaby@...nel.org>, <admin@...iphile.com>,
        <Kavyasree.Kotagiri@...rochip.com>, <Tudor.Ambarus@...rochip.com>
CC:     <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux-spi@...r.kernel.org>,
        <linux-serial@...r.kernel.org>, <linux-clk@...r.kernel.org>
Subject: 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@...rochip.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