Re: [PATCH v2 2/3] dt-bindings: mtd: microchip-nand: add atmel pmecc

[Rob Herring <robh@kernel.org>]

On Tue, Mar 11, 2025 at 05:58:46PM +0530, Balamanikandan Gunasundar wrote:
> Add bindings for programmable multibit error correction code controller
> (PMECC).
> 
> Signed-off-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
> ---
> 
> Changes in v2:
> - Rename filename to match compatible string
> - Add constraints for sam9x7
> - Droped unused dt labels 
> 
> .../devicetree/bindings/mtd/atmel-nand.txt    | 61 -----------------
>  .../bindings/mtd/microchip,pmecc.yaml         | 67 +++++++++++++++++++
>  2 files changed, 67 insertions(+), 61 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mtd/microchip,pmecc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> index dbbc17a866f2..1934614a9298 100644
> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> @@ -1,64 +1,3 @@
> -* ECC engine (PMECC) bindings:
> -
> -Required properties:
> -- compatible: should be one of the following
> -	"atmel,at91sam9g45-pmecc"
> -	"atmel,sama5d4-pmecc"
> -	"atmel,sama5d2-pmecc"
> -	"microchip,sam9x60-pmecc"
> -	"microchip,sam9x7-pmecc", "atmel,at91sam9g45-pmecc"
> -- reg: should contain 2 register ranges. The first one is pointing to the PMECC
> -       block, and the second one to the PMECC_ERRLOC block.
> -
> -Example:
> -
> -	nfc_io: nfc-io@70000000 {
> -		compatible = "atmel,sama5d3-nfc-io", "syscon";
> -		reg = <0x70000000 0x8000000>;
> -	};
> -
> -	pmecc: ecc-engine@ffffc070 {
> -		compatible = "atmel,at91sam9g45-pmecc";
> -                reg = <0xffffc070 0x490>,
> -                      <0xffffc500 0x100>;
> -	};
> -
> -	ebi: ebi@10000000 {
> -		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>;
> -
> -                nand_controller: nand-controller {
> -			compatible = "atmel,sama5d3-nand-controller";
> -			atmel,nfc-sram = <&nfc_sram>;
> -			atmel,nfc-io = <&nfc_io>;
> -			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.
> -				 */
> -			};
> -		};
> -	};
> -
> ------------------------------------------------------------------------
> -
>  Deprecated bindings (should not be used in new device trees):
>  
>  Required properties:
> diff --git a/Documentation/devicetree/bindings/mtd/microchip,pmecc.yaml b/Documentation/devicetree/bindings/mtd/microchip,pmecc.yaml
> new file mode 100644
> index 000000000000..98260a691a2e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/microchip,pmecc.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mtd/microchip,pmecc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip pmecc controller
> +
> +maintainers:
> +  - Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
> +
> +description: |

Don't need '|' if no formatting to preserve.

> +  Bindings for microchip Programmable Multibit Error Correction Code
> +  Controller (PMECC). pmecc is a programmable BCH encoder/decoder. This
> +  block is passed as the value to the "ecc-engine" property of microchip
> +  nand flash controller node.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - atmel,at91sam9g45-pmecc
> +          - atmel,sama5d2-pmecc
> +          - atmel,sama5d4-pmecc
> +          - microchip,sam9x60-pmecc
> +          - microchip,sam9x7-pmecc

Both alone and with a fallback should not be allowed. Pick one. A 
fallback is only useful if there's a driver/client that only understands 
the fallback and would work with the new implementation. IOW, if 'they 
are the same'.

> +      - items:
> +          - const: microchip,sam9x7-pmecc
> +          - const: atmel,at91sam9g45-pmecc
> +      - items:
> +          - const: microchip,sam9x60-pmecc
> +          - const: atmel,at91sam9g45-pmecc

Combine the last 2 'items' to 1 as the fallback is the same for both.

> +
> +  reg:
> +    items:
> +      - description: Base address and size of PMECC controller registers
> +      - description: Base address and size of PMECC_ERRLOC controller

Drop 'Base address and size of '.

> +
> +  clocks:
> +    description: The clock source for pmecc controller

For a single clock, what else would it be? Drop.

> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: microchip,sam9x7-pmecc
> +    then:
> +      properties:
> +        clocks:
> +          description: The clock source for pmecc controller

What's the purpose of this if/then? It doesn't do anything.

> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    ecc-engine@ffffc070 {
> +        compatible = "microchip,sam9x7-pmecc";
> +        reg = <0xffffe000 0x300>,
> +              <0xffffe600 0x100>;
> +        clocks = <&pmc 2 48>;
> +    };
> -- 
> 2.34.1
>