Re: [PATCH 1/3] dt-bindings: mtd: atmel-nand: convert txt to yaml

[<Balamanikandan.Gunasundar@microchip.com>]

Hi Conor,

On 20/03/24 10:05 pm, Conor Dooley wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 
> ForwardedMessage.eml
> 
> Subject:
> Re: [PATCH 1/3] dt-bindings: mtd: atmel-nand: convert txt to yaml
> From:
> Conor Dooley <conor@kernel.org>
> Date:
> 20/03/24, 10:05 pm
> 
> To:
> Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
> CC:
> Miquel Raynal <miquel.raynal@bootlin.com>, Richard Weinberger 
> <richard@nod.at>, Vignesh Raghavendra <vigneshr@ti.com>, Rob Herring 
> <robh@kernel.org>, Krzysztof Kozlowski 
> <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
> Nicolas Ferre <nicolas.ferre@microchip.com>, Alexandre Belloni 
> <alexandre.belloni@bootlin.com>, Claudiu Beznea 
> <claudiu.beznea@tuxon.dev>, linux-mtd@lists.infradead.org, 
> devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, 
> linux-kernel@vger.kernel.org
> 
> 
> On Wed, Mar 20, 2024 at 11:22:07AM +0530, Balamanikandan Gunasundar wrote:
>> Convert text to yaml for atmel nand controller
>>
>> Signed-off-by: Balamanikandan Gunasundar<balamanikandan.gunasundar@microchip.com>
>> ---
>>   .../devicetree/bindings/mtd/atmel-nand.txt         |  50 -------
>>   .../devicetree/bindings/mtd/atmel-nand.yaml        | 166 +++++++++++++++++++++
>>   MAINTAINERS                                        |   2 +-
>>   3 files changed, 167 insertions(+), 51 deletions(-)
>> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.yaml b/Documentation/devicetree/bindings/mtd/atmel-nand.yaml
>> new file mode 100644
>> index 000000000000..a5482d292293
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.yaml
> Filename matching a compatible please.
> 
>> @@ -0,0 +1,166 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id:http://devicetree.org/schemas/mtd/atmel-nand.yaml#
>> +$schema:http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Atmel NAND flash controller
>> +
>> +maintainers:
>> +  - Balamanikandan Gunasundar<balamanikandan.gunasundar@microchip.com>
>> +
>> +description: |
>> +  The NAND flash controller node should be defined under the EBI bus (see
>> +  Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt|yaml).
> text|yaml?
> 
>> +  One or several NAND devices can be defined under this NAND controller.
>> +  The NAND controller might be connected to an ECC engine.
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - items:
>> +          - enum:
> This is just an enum, drop the items and oneof.
> 
>> +              - atmel,at91rm9200-nand-controller
>> +              - atmel,at91sam9260-nand-controller
>> +              - atmel,at91sam9261-nand-controller
>> +              - atmel,at91sam9g45-nand-controller
>> +              - atmel,sama5d3-nand-controller
>> +              - microchip,sam9x60-nand-controller
>> +
>> +  ranges:
>> +    description: empty ranges property to forward EBI ranges definitions.
>> +
>> +  ecc-engine:
>> +    description:
>> +      phandle to the PMECC block. Only meaningful if the SoC embeds a PMECC
>> +      engine.
>> +
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - atmel,at91rm9200-nand-controller
>> +              - atmel,at91sam9260-nand-controller
>> +              - atmel,at91sam9261-nand-controller
>> +              - atmel,at91sam9g45-nand-controller
>> +              - atmel,sama5d3-nand-controller
>> +              - microchip,sam9x60-nand-controller
>> +    then:
>> +      properties:
>> +        "#address-cells":
>> +          const: 2
>> +
>> +        "#size-cells":
>> +          const: 1
> Why is this in an if? Isn't this all of the devices in the binding?
> 

The default nand-controller.yaml defines this as const values. 
(#address-cell : 1 and #size-cells : 1). I am trying to override this 
const value. May be I should think about better approach ?

>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: atmel,sama5d3-nand-controller
>> +    then:
>> +      properties:
>> +        atmel,nfc-io:
>> +          $ref: /schemas/types.yaml#/definitions/phandle
>> +          description: phandle to the NFC IO block.
>> +
>> +        atmel,nfc-sram:
>> +          $ref: /schemas/types.yaml#/definitions/phandle
>> +          description: phandle to the NFC SRAM block
> Please define the properties at the top level and use if statements to
> constrain them.
> 
>> +
>> +required:
>> +  - compatible
>> +  - ranges
>> +  - "#address-cells"
>> +  - "#size-cells"
>> +
>> +patternProperties:
>> +  "^nand@[a-f0-9]$":
>> +    type: object
>> +    $ref: nand-chip.yaml#
>> +    description:
>> +      NAND chip bindings. All generic properties described in
>> +      Documentation/devicetree/bindings/mtd/{common,nand}.txt also apply to
>> +      the NAND device node, and NAND partitions should be defined under the
>> +      NAND node as described in
>> +      Documentation/devicetree/bindings/mtd/partition.txt.
> These files do not exist.
> 

Apologies for copying the content from the text file. I will correct this.

>> +    properties:
>> +      reg:
>> +        minItems: 1
>> +        description:
>> +          describes the CS lines assigned to the NAND device. If the NAND device
>> +          exposes multiple CS lines (multi-dies chips), your reg property will
>> +          contain X tuples of 3 entries.
> The "if" here is not accurate. Your binding mandates there being 3
> entries.
> 
>> +         reg = <0x3 0x0 0x800000>;
>> +          1st entry - the CS line this NAND chip is connected to
>> +          2nd entry - the base offset of the memory region assigned to this
>> +                      device (always 0)
>> +          3rd entry - the memory region size (always 0x800000)
>> +
>> +      rb-gpios:
>> +        description:
>> +          the GPIO(s) used to check the Ready/Busy status of the NAND.
>> +
>> +      cs-gpios:
>> +        description:
>> +          the GPIO(s) used to control the CS line.
>> +
>> +      det-gpios:
>> +        description:
>> +          the GPIO used to detect if a Smartmedia Card is present.
>> +
>> +      "atmel,rb":
>> +        description:
>> +          an integer identifying the native Ready/Busy pin. Only meaningful
>> +          on sama5 SoCs.
> Then please constrain it to sama5 SoCs only 🙂
> 
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    nfc_io: nfc-io@70000000 {
>> +            compatible = "atmel,sama5d3-nfc-io", "syscon";
>> +            reg = <0x70000000 0x8000000>;
>> +    };
> What's this got to do with the binding?
> 
>> +    pmecc: ecc-engine@ffffc070 {
>> +            compatible = "atmel,at91sam9g45-pmecc";
>> +            reg = <0xffffc070 0x490>,
>> +                  <0xffffc500 0x100>;
>> +    };
>> +
>> +    ebi: ebi@10000000 {
> Drop the unused label.
> 
> Same applies here, read the coding style about how to write dts nodes
> please.
> 

Yes. I should fix the alignment. I will send a v2 shortly

Thanks,
Bala

> Thanks,
> Conor.
> 
>> +            compatible = "atmel,sama5d3-ebi";
>> +            #address-cells = <2>;
>> +            #size-cells = <1>;
>> +            atmel,smc = <&hsmc>;
>> +            reg = <0x10000000 0x10000000
>> +                   0x40000000 0x30000000>;
>> +            ranges = <0x0 0x0 0x10000000 0x10000000
>> +                      0x1 0x0 0x40000000 0x10000000
>> +                      0x2 0x0 0x50000000 0x10000000
>> +                      0x3 0x0 0x60000000 0x10000000>;
>> +            clocks = <&mck>;
>> +
>> +            nandflash_controller: nandflash-controller {
>> +                    compatible = "atmel,sama5d3-nand-controller";
>> +                    ecc-engine = <&pmecc>;
>> +                    #address-cells = <2>;
>> +                    #size-cells = <1>;
>> +                    ranges;
>> +
>> +                    nand@3 {
>> +                            reg = <0x3 0x0 0x800000>;
>> +                            atmel,rb = <0>;
>> +
>> +                            /*
>> +                             * Put generic NAND/MTD properties and
>> +                             * subnodes here.
>> +                             */
>> +                    };
>> +             };
>> +    };
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index b6582bd3eb2c..3f2a6756223f 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -14503,7 +14503,7 @@ MICROCHIP NAND DRIVER
>>   M:	Balamanikandan Gunasundar<balamanikandan.gunasundar@microchip.com>
>>   L:	linux-mtd@lists.infradead.org
>>   S:	Supported
>> -F:	Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> +F:	Documentation/devicetree/bindings/mtd/atmel-*.yaml
>>   F:	drivers/mtd/nand/raw/atmel/*
>>   
>>   MICROCHIP OTPC DRIVER
>>
>> -- 
>> 2.25.1
>>

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