Re: [PATCH 1/3] dt-bindings: crypto: Convert Atmel AES to yaml

[<Tudor.Ambarus@microchip.com>]

Hi, Krzysztof,

On 2/7/22 17:56, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 07/02/2022 04:24, Tudor Ambarus wrote:
>> Convert Atmel AES documentation to yaml format. With the conversion the
>> clock and clock-names properties are made mandatory. The driver returns
>> -EINVAL if "aes_clk" is not found, reflect that in the bindings and make
>> the clock and clock-names properties mandatory. Update the example to
>> better describe how one should define the dt node.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  .../devicetree/bindings/crypto/atmel,aes.yaml | 65 +++++++++++++++++++
>>  .../bindings/crypto/atmel-crypto.txt          | 20 ------
>>  2 files changed, 65 insertions(+), 20 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/crypto/atmel,aes.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/crypto/atmel,aes.yaml b/Documentation/devicetree/bindings/crypto/atmel,aes.yaml
>> new file mode 100644
>> index 000000000000..f77ec04dbabe
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/crypto/atmel,aes.yaml
>> @@ -0,0 +1,65 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/crypto/atmel,aes.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Atmel Advanced Encryption Standard (AES) HW cryptographic accelerator
>> +
>> +maintainers:
>> +  - Tudor Ambarus <tudor.ambarus@microchip.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: atmel,at91sam9g46-aes
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    const: aes_clk
>> +
>> +  dmas:
>> +    items:
>> +      - description: TX DMA Channel
>> +      - description: RX DMA Channel
>> +
>> +  dma-names:
>> +    items:
>> +      - const: tx
>> +      - const: rx
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - clock-names
>> +  - dmas
>> +  - dma-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/clock/at91.h>
>> +    #include <dt-bindings/dma/at91.h>
> 
> One empty line for readability.

Ok.

> 
>> +    aes: aes@f8038000 {
> 
> Generic node name, so "crypto".

Hm, I'm not convinced why, would you please give more details about this
requirement? This IP is capable of doing just AES operations, I find it
generic enough. We use the "aes" name on all our SoCs that have a version
of this IP, that would be quite a change. So I would prefer to keep the
"aes" name if possible.

> 
>> +      compatible = "atmel,at91sam9g46-aes";
>> +      reg = <0xe1810000 0x100>;
>> +      interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
>> +      clocks = <&pmc PMC_TYPE_PERIPHERAL 27>;
>> +      clock-names = "aes_clk";
>> +      dmas = <&dma0 AT91_XDMAC_DT_PERID(1)>,
>> +             <&dma0 AT91_XDMAC_DT_PERID(2)>;
>> +      dma-names = "tx", "rx";
>> +      status= "okay";
>> +    };
> 
> Drop the status property.

Ok. Thanks for reviewing.
ta

> 
> Best regards,
> Krzysztof