* Re: [PATCH] dt-bindings : misc : aspeed,cvic interrupt controller : convert the binding document to yaml
2023-02-06 15:33 [PATCH] dt-bindings : misc : aspeed,cvic interrupt controller : convert the binding document to yaml Vijaya Anand
@ 2023-02-06 15:42 ` Krzysztof Kozlowski
2023-02-06 19:49 ` Rob Herring
2023-02-06 19:55 ` Rob Herring
2 siblings, 0 replies; 4+ messages in thread
From: Krzysztof Kozlowski @ 2023-02-06 15:42 UTC (permalink / raw)
To: Vijaya Anand
Cc: Rob Herring, Krzysztof Kozlowski, Joel Stanley, Andrew Jeffery,
Benjamin Herrenschmidt, devicetree, linux-arm-kernel,
linux-aspeed, linux-kernel
Subject: incorrect format. Use style consistent with subsystem (git log).
On 06/02/2023 16:33, Vijaya Anand wrote:
> Convert the binding document for ASPEED AST2400 and AST2500 coprocessor interrupt controller
> from txt to yaml so one could validate dt-entries correctly and any future additions can go
> into yaml format. The options for compatability described according to the example given.
Weird commit msg indentation, wrong wrapping.
https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586
typo - compatibility
> ---
> .../devicetree/bindings/misc/aspeed,cvic.txt | 35 ----------
> .../devicetree/bindings/misc/aspeed,cvic.yaml | 67 +++++++++++++++++++
> 2 files changed, 67 insertions(+), 35 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/misc/aspeed,cvic.txt
> create mode 100644 Documentation/devicetree/bindings/misc/aspeed,cvic.yaml
It is interrupt controller, so move it to interrupt-controller directory.
>
> diff --git a/Documentation/devicetree/bindings/misc/aspeed,cvic.txt b/Documentation/devicetree/bindings/misc/aspeed,cvic.txt
> deleted file mode 100644
> index d62c783d1d5e..000000000000
> --- a/Documentation/devicetree/bindings/misc/aspeed,cvic.txt
> +++ /dev/null
> @@ -1,35 +0,0 @@
> -* ASPEED AST2400 and AST2500 coprocessor interrupt controller
> -
> -This file describes the bindings for the interrupt controller present
> -in the AST2400 and AST2500 BMC SoCs which provides interrupt to the
> -ColdFire coprocessor.
> -
> -It is not a normal interrupt controller and it would be rather
> -inconvenient to create an interrupt tree for it as it somewhat shares
> -some of the same sources as the main ARM interrupt controller but with
> -different numbers.
> -
> -The AST2500 supports a SW generated interrupt
> -
> -Required properties:
> -- reg: address and length of the register for the device.
> -- compatible: "aspeed,cvic" and one of:
> - "aspeed,ast2400-cvic"
> - or
> - "aspeed,ast2500-cvic"
> -
> -- valid-sources: One cell, bitmap of supported sources for the implementation
> -
> -Optional properties;
> -- copro-sw-interrupts: List of interrupt numbers that can be used as
> - SW interrupts from the ARM to the coprocessor.
> - (AST2500 only)
> -
> -Example:
> -
> - cvic: copro-interrupt-controller@1e6c2000 {
> - compatible = "aspeed,ast2500-cvic";
> - valid-sources = <0xffffffff>;
> - copro-sw-interrupts = <1>;
> - reg = <0x1e6c2000 0x80>;
> - };
> diff --git a/Documentation/devicetree/bindings/misc/aspeed,cvic.yaml b/Documentation/devicetree/bindings/misc/aspeed,cvic.yaml
> new file mode 100644
> index 000000000000..bbff0418fa2c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/aspeed,cvic.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/aspeed,cvic.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ASPEED AST2400 and AST2500 coprocessor interrupt controller
> +
> +maintainers:
> + - Benjamin Herrenschmidt <benh@kernel.crashing.org>
> + - Rob Herring <robh@kernel.org>
These should be people having/knowing/responsible for hardware. I doubt
Rob has any interest in Aspeed...
> +
> +description: |
> + This file describes the bindings for the interrupt controller present
Drop "This file describes the bindings for the" but instead rephrase it
and describe the hardware.
> + in the AST2400 and AST2500 BMC SoCs which provides interrupt to the
> + ColdFire coprocessor.
> +
> + It is not a normal interrupt controller and it would be rather
> + inconvenient to create an interrupt tree for it as it somewhat shares
> + some of the same sources as the main ARM interrupt controller but with
> + different numbers.
> +
> + The AST2500 supports a SW generated interruptThe Soft Decision Forward Error Correction (SDFEC) Engine is a Hard IP block
Fix your wrapping and sentences.
> + which provides high-throughput LDPC and Turbo Code implementations.
> + The LDPC decode & encode functionality is capable of covering a range of
> + customer specified Quasi-cyclic (QC) codes. The Turbo decode functionality
> + principally covers codes used by LTE. The FEC Engine offers significant
> + power and area savings versus implementations done in the FPGA fabric.
> +
> +properties:
> +
Drop blank line.
> + compatible:
> + enum:
> + - aspeed,ast2400-cvic
> + - aspeed,ast2500-cvic
> +
> + reg:
> + maxItems: 1
> + description: address and length of the register for the device.
Drop description
> +
> + valid-sources:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: One cell, bitmap of supported sources for the implementation
> +
> + copro-sw-interrupts:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + description: |
Drop | and do fix wrapping.
> + List of interrupt numbers that can be used as
> + SW interrupts from the ARM to the coprocessor.
> + (AST2500 only)
> +
> +required:
> + - compatible
> + - reg
> + - valid-sources
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + cvic: copro-interrupt-controller@1e6c2000
That's not DT coding style. Open DTS and look how this is coded in ARM.
Node name: interrupt-controller
Drop label.
> + {
> + compatible = "aspeed,ast2500-cvic";
> + valid-sources = <0xffffffff>;
> + copro-sw-interrupts = <1>;
> + reg = <0x1e6c2000 0x80>;
reg is second property
> + };
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] dt-bindings : misc : aspeed,cvic interrupt controller : convert the binding document to yaml
2023-02-06 15:33 [PATCH] dt-bindings : misc : aspeed,cvic interrupt controller : convert the binding document to yaml Vijaya Anand
2023-02-06 15:42 ` Krzysztof Kozlowski
@ 2023-02-06 19:49 ` Rob Herring
2023-02-06 19:55 ` Rob Herring
2 siblings, 0 replies; 4+ messages in thread
From: Rob Herring @ 2023-02-06 19:49 UTC (permalink / raw)
To: Vijaya Anand
Cc: linux-kernel, Andrew Jeffery, Benjamin Herrenschmidt,
linux-arm-kernel, linux-aspeed, Joel Stanley, Rob Herring,
devicetree, Krzysztof Kozlowski
On Mon, 06 Feb 2023 21:03:09 +0530, Vijaya Anand wrote:
> Convert the binding document for ASPEED AST2400 and AST2500 coprocessor interrupt controller
> from txt to yaml so one could validate dt-entries correctly and any future additions can go
> into yaml format. The options for compatability described according to the example given.
> ---
> .../devicetree/bindings/misc/aspeed,cvic.txt | 35 ----------
> .../devicetree/bindings/misc/aspeed,cvic.yaml | 67 +++++++++++++++++++
> 2 files changed, 67 insertions(+), 35 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/misc/aspeed,cvic.txt
> create mode 100644 Documentation/devicetree/bindings/misc/aspeed,cvic.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
./Documentation/devicetree/bindings/misc/aspeed,cvic.yaml:23:111: [warning] line too long (123 > 110 characters) (line-length)
dtschema/dtc warnings/errors:
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230206153325.43692-1-sunrockers8@gmail.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] dt-bindings : misc : aspeed,cvic interrupt controller : convert the binding document to yaml
2023-02-06 15:33 [PATCH] dt-bindings : misc : aspeed,cvic interrupt controller : convert the binding document to yaml Vijaya Anand
2023-02-06 15:42 ` Krzysztof Kozlowski
2023-02-06 19:49 ` Rob Herring
@ 2023-02-06 19:55 ` Rob Herring
2 siblings, 0 replies; 4+ messages in thread
From: Rob Herring @ 2023-02-06 19:55 UTC (permalink / raw)
To: Vijaya Anand
Cc: Krzysztof Kozlowski, Joel Stanley, Andrew Jeffery,
Benjamin Herrenschmidt, devicetree, linux-arm-kernel,
linux-aspeed, linux-kernel
On Mon, Feb 6, 2023 at 9:35 AM Vijaya Anand <sunrockers8@gmail.com> wrote:
>
> Convert the binding document for ASPEED AST2400 and AST2500 coprocessor interrupt controller
> from txt to yaml so one could validate dt-entries correctly and any future additions can go
> into yaml format. The options for compatability described according to the example given.
Missing DCO (Signed-off-by). checkpatch.pl will tell this for you.
> ---
> .../devicetree/bindings/misc/aspeed,cvic.txt | 35 ----------
> .../devicetree/bindings/misc/aspeed,cvic.yaml | 67 +++++++++++++++++++
> 2 files changed, 67 insertions(+), 35 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/misc/aspeed,cvic.txt
> create mode 100644 Documentation/devicetree/bindings/misc/aspeed,cvic.yaml
>
> diff --git a/Documentation/devicetree/bindings/misc/aspeed,cvic.txt b/Documentation/devicetree/bindings/misc/aspeed,cvic.txt
> deleted file mode 100644
> index d62c783d1d5e..000000000000
> --- a/Documentation/devicetree/bindings/misc/aspeed,cvic.txt
> +++ /dev/null
> @@ -1,35 +0,0 @@
> -* ASPEED AST2400 and AST2500 coprocessor interrupt controller
> -
> -This file describes the bindings for the interrupt controller present
> -in the AST2400 and AST2500 BMC SoCs which provides interrupt to the
> -ColdFire coprocessor.
> -
> -It is not a normal interrupt controller and it would be rather
> -inconvenient to create an interrupt tree for it as it somewhat shares
> -some of the same sources as the main ARM interrupt controller but with
> -different numbers.
> -
> -The AST2500 supports a SW generated interrupt
> -
> -Required properties:
> -- reg: address and length of the register for the device.
> -- compatible: "aspeed,cvic" and one of:
> - "aspeed,ast2400-cvic"
> - or
> - "aspeed,ast2500-cvic"
> -
> -- valid-sources: One cell, bitmap of supported sources for the implementation
> -
> -Optional properties;
> -- copro-sw-interrupts: List of interrupt numbers that can be used as
> - SW interrupts from the ARM to the coprocessor.
> - (AST2500 only)
> -
> -Example:
> -
> - cvic: copro-interrupt-controller@1e6c2000 {
> - compatible = "aspeed,ast2500-cvic";
> - valid-sources = <0xffffffff>;
> - copro-sw-interrupts = <1>;
> - reg = <0x1e6c2000 0x80>;
> - };
> diff --git a/Documentation/devicetree/bindings/misc/aspeed,cvic.yaml b/Documentation/devicetree/bindings/misc/aspeed,cvic.yaml
> new file mode 100644
> index 000000000000..bbff0418fa2c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/aspeed,cvic.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/aspeed,cvic.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ASPEED AST2400 and AST2500 coprocessor interrupt controller
> +
> +maintainers:
> + - Benjamin Herrenschmidt <benh@kernel.crashing.org>
> + - Rob Herring <robh@kernel.org>
Should be someone that has and cares about Aspeed h/w, not me.
> +
> +description: |
> + This file describes the bindings for the interrupt controller present
> + in the AST2400 and AST2500 BMC SoCs which provides interrupt to the
> + ColdFire coprocessor.
> +
> + It is not a normal interrupt controller and it would be rather
> + inconvenient to create an interrupt tree for it as it somewhat shares
> + some of the same sources as the main ARM interrupt controller but with
> + different numbers.
> +
> + The AST2500 supports a SW generated interruptThe Soft Decision Forward Error Correction (SDFEC) Engine is a Hard IP block
> + which provides high-throughput LDPC and Turbo Code implementations.
> + The LDPC decode & encode functionality is capable of covering a range of
> + customer specified Quasi-cyclic (QC) codes. The Turbo decode functionality
> + principally covers codes used by LTE. The FEC Engine offers significant
> + power and area savings versus implementations done in the FPGA fabric.
> +
> +properties:
> +
> + compatible:
> + enum:
> + - aspeed,ast2400-cvic
> + - aspeed,ast2500-cvic
> +
> + reg:
> + maxItems: 1
> + description: address and length of the register for the device.
Drop generic descriptions.
> +
> + valid-sources:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: One cell, bitmap of supported sources for the implementation
Drop 'one cell'. The type says that.
> +
> + copro-sw-interrupts:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
Is there a range of number of entries and/or values for entries. If
so, add constraints.
> + description: |
Don't need '|' if no formatting to maintain.
> + List of interrupt numbers that can be used as
> + SW interrupts from the ARM to the coprocessor.
> + (AST2500 only)
> +
> +required:
> + - compatible
> + - reg
> + - valid-sources
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + cvic: copro-interrupt-controller@1e6c2000
> + {
> + compatible = "aspeed,ast2500-cvic";
> + valid-sources = <0xffffffff>;
> + copro-sw-interrupts = <1>;
> + reg = <0x1e6c2000 0x80>;
> + };
> --
> 2.37.1 (Apple Git-137.1)
>
^ permalink raw reply [flat|nested] 4+ messages in thread