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]

Date:   Thu, 8 Sep 2022 17:10:17 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Sergiu.Moga@...rochip.com, 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 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. The node name should not
be responsible for it, even though we want node names to match certain
patterns.

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

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