dt-bindings: edac: Add Altera SOCFPGA SDRAM EDAC binding

[PATCH v3] dt-bindings: edac: Add Altera SOCFPGA SDRAM EDAC binding

Posted by Alessandro Zanni 3 months ago

Convert the device tree bindings for the Altera PCIe MSI controller

Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.com>
---

Notes:
    v3: moved yaml file from arm/altera to the edac folder, removed items keys, added general node names

 .../arm/altera/socfpga-sdram-edac.txt         | 15 -------
 .../bindings/edac/altr,sdram-edac.yaml        | 44 +++++++++++++++++++
 2 files changed, 44 insertions(+), 15 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
 create mode 100644 Documentation/devicetree/bindings/edac/altr,sdram-edac.yaml

diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
deleted file mode 100644
index f5ad0ff69fae..000000000000
--- a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
+++ /dev/null
@@ -1,15 +0,0 @@
-Altera SOCFPGA SDRAM Error Detection & Correction [EDAC]
-The EDAC accesses a range of registers in the SDRAM controller.
-
-Required properties:
-- compatible : should contain "altr,sdram-edac" or "altr,sdram-edac-a10"
-- altr,sdr-syscon : phandle of the sdr module
-- interrupts : Should contain the SDRAM ECC IRQ in the
-	appropriate format for the IRQ controller.
-
-Example:
-	sdramedac {
-		compatible = "altr,sdram-edac";
-		altr,sdr-syscon = <&sdr>;
-		interrupts = <0 39 4>;
-	};
diff --git a/Documentation/devicetree/bindings/edac/altr,sdram-edac.yaml b/Documentation/devicetree/bindings/edac/altr,sdram-edac.yaml
new file mode 100644
index 000000000000..31bcee4274fa
--- /dev/null
+++ b/Documentation/devicetree/bindings/edac/altr,sdram-edac.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/edac/altr,sdram-edac.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Altera SOCFPGA SDRAM Error Detection & Correction [EDAC]
+
+maintainers:
+  - Dinh Nguyen <dinguyen@kernel.org>
+
+description:
+  The EDAC accesses a range of registers in the SDRAM controller.
+
+properties:
+  compatible:
+    enum:
+      - altr,sdram-edac
+      - altr,sdram-edac-a10
+
+  altr,sdr-syscon:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: 
+      Phandle of the sdr module
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - altr,sdr-syscon
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    memory-controller {
+      compatible = "altr,sdram-edac";
+      altr,sdr-syscon = <&sdr>;
+      interrupts = <0 39 4>;
+    };
+
+...
-- 
2.43.0

Re: [PATCH v3] dt-bindings: edac: Add Altera SOCFPGA SDRAM EDAC binding

Posted by Krzysztof Kozlowski 2 months, 4 weeks ago

On 01/08/2024 18:10, Alessandro Zanni wrote:
> Convert the device tree bindings for the Altera PCIe MSI controller
> 
> Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.com>
> ---
> 

Subject: you did not add binding in this patch. Binding was already
there. You converted one to DT schema.

> Notes:
>     v3: moved yaml file from arm/altera to the edac folder, removed items keys, added general node names
> 

...

> +
> +examples:
> +  - |
> +    memory-controller {
> +      compatible = "altr,sdram-edac";
> +      altr,sdr-syscon = <&sdr>;
> +      interrupts = <0 39 4>;

No improvements here.

<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>

Best regards,
Krzysztof

Re: [PATCH v3] dt-bindings: edac: Add Altera SOCFPGA SDRAM EDAC binding

Posted by Alessandro Zanni 2 months, 4 weeks ago

Thank you for the review.

This patch aims to be a conversion from txt file
"bindings/arm/altera/socfpga-sdram-edac.txt" to this
"bindings/edac/altr,sdram-edac.yaml" file.
That's why I also deleted the txt file in the patch.

Maybe I should have put a different title to make it clearer.

This is the original example in the txt file:
sdramedac {
    compatible = "altr,sdram-edac";
    altr,sdr-syscon = <&sdr>;
    interrupts = <0 39 4>;

and this is my conversion:
> +  - |
> +    memory-controller {
> +      compatible = "altr,sdram-edac";
> +      altr,sdr-syscon = <&sdr>;
> +      interrupts = <0 39 4>;

Sorry for that but, to me, it's not clear what I should convert in
this example to complete the yaml file.

Best regards,
Alessandro


Il giorno ven 2 ago 2024 alle ore 08:54 Krzysztof Kozlowski
<krzk@kernel.org> ha scritto:
>
> On 01/08/2024 18:10, Alessandro Zanni wrote:
> > Convert the device tree bindings for the Altera PCIe MSI controller
> >
> > Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.com>
> > ---
> >
>
> Subject: you did not add binding in this patch. Binding was already
> there. You converted one to DT schema.
>
>
> > Notes:
> >     v3: moved yaml file from arm/altera to the edac folder, removed items keys, added general node names
> >
>
> ...
>
> > +
> > +examples:
> > +  - |
> > +    memory-controller {
> > +      compatible = "altr,sdram-edac";
> > +      altr,sdr-syscon = <&sdr>;
> > +      interrupts = <0 39 4>;
>
> No improvements here.
>
> <form letter>
> This is a friendly reminder during the review process.
>
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
>
> Thank you.
> </form letter>
>
>
> Best regards,
> Krzysztof
>

Re: [PATCH v3] dt-bindings: edac: Add Altera SOCFPGA SDRAM EDAC binding

Posted by Krzysztof Kozlowski 2 months, 3 weeks ago

On 02/08/2024 16:32, Alessandro Zanni wrote:
> Thank you for the review.
> 
> This patch aims to be a conversion from txt file
> "bindings/arm/altera/socfpga-sdram-edac.txt" to this
> "bindings/edac/altr,sdram-edac.yaml" file.
> That's why I also deleted the txt file in the patch.
> 
> Maybe I should have put a different title to make it clearer.
> 
> This is the original example in the txt file:
> sdramedac {
>     compatible = "altr,sdram-edac";
>     altr,sdr-syscon = <&sdr>;
>     interrupts = <0 39 4>;
> 
> and this is my conversion:
>> +  - |
>> +    memory-controller {
>> +      compatible = "altr,sdram-edac";
>> +      altr,sdr-syscon = <&sdr>;
>> +      interrupts = <0 39 4>;
> 
> Sorry for that but, to me, it's not clear what I should convert in
> this example to complete the yaml file.
> 

Don't top-post. I have no clue what you are referring to.

Best regards,
Krzysztof