linux-kernel - Re: [PATCH 02/10] dt-bindings: net: can: grcan: Convert GRCAN CAN controllers binding from txt to YAML

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c80ff180-b7f1-4f39-b39d-2953ef75a7ad@kernel.org>
Date: Tue, 18 Nov 2025 12:01:23 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Arun Muthusamy <arun.muthusamy@...sler.com>, robh@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org, mkl@...gutronix.de,
 mailhol@...nel.org
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-can@...r.kernel.org
Subject: Re: [PATCH 02/10] dt-bindings: net: can: grcan: Convert GRCAN CAN
 controllers binding from txt to YAML

On 18/11/2025 10:21, Arun Muthusamy wrote:
> Migrate device tree bindings for Gaisler GRCAN, GRHCAN
> and GRCANFD CAN controllers from a text format to YAML format.
>     - Add properties such as `compatible`, `reg`, `interrupts`

Odd indentation. Please write readable commit msgs.

Also:
1. Why? You need to explain why you are changing binding during conversion.
2. Reg was already there, so I don't understand why you need to add it.


>     and `clocks` for the CAN controllers.
>     - Removal of the old `grcan.txt` file as its contents have
>     been fully migrated to the YAML file.

Drop, that's not relevant.

>     - YAML file includes examples of device tree bindings for
>     the CAN controllers

Drop, not relevant. Please look at git history how commits are written.

> 
> Signed-off-by: Arun Muthusamy <arun.muthusamy@...sler.com>
> ---
>  .../bindings/net/can/gaisler,grcan.yaml       | 85 +++++++++++++++++++
>  .../devicetree/bindings/net/can/grcan.txt     | 28 ------
>  2 files changed, 85 insertions(+), 28 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/can/gaisler,grcan.yaml
>  delete mode 100644 Documentation/devicetree/bindings/net/can/grcan.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/can/gaisler,grcan.yaml b/Documentation/devicetree/bindings/net/can/gaisler,grcan.yaml
> new file mode 100644
> index 000000000000..521bdd89f130
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/gaisler,grcan.yaml
> @@ -0,0 +1,85 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/can/gaisler,grcan.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title:
> +  Aeroflex Gaisler GRCAN, GRHCAN and GRCANFD CAN controllers.
> +
> +description: |
> +  GRCAN, GRCANFD, GRHCAN controllers are available in the GRLIB VHDL IP core
> +  library.
> +
> +  For further information look in the documentation for the GRLIB IP library:
> +  https://download.gaisler.com/products/GRLIB/doc/grip.pdf
> +
> +maintainers:
> +  - Arun Muthusamy <arun.muthusamy@...sler.com>
> +  - Andreas Larsson <andreas@...sler.com>
> +
> +allOf:
> +  - $ref: can-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - gaisler,grcan
> +      - gaisler,grcanfd

Blank line

> +  name:
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      Fallback on node name matching for systems that don't provide compatible.
> +    enum:
> +      - GAISLER_GRCAN
> +      - 01_03d
> +      - GAISLER_GRHCAN
> +      - "01_034"

This does not really work. Are you really defining here "name" property?

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  freq:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Frequency of the external oscillator clock in Hz (the frequency of the
> +      amba bus in the ordinary case).
> +      This property should be used by systems that utilize the common clock
> +      framework is not supported.

Missing systemid. Your commit msg must explain any changes done to the
binding during conversion.

> +
> +unevaluatedProperties: false

This goes after required block.

> +
> +required:

compatible as well

> +  - reg
> +  - interrupts

Where is freq? It was required in the old binding. Again, you need to
explain the changes.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    can@...00000 {
> +        compatible = "gaisler,grcanfd";
> +        clocks = <&sysclock>;
> +        reg = <0xff400000 0x400>;
> +        interrupt-parent = <&plic0>;
> +        interrupts = <6>;
> +    };

One example is enough

> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    can@...00000 {
> +        compatible = "gaisler,grcan";
> +        clocks = <&sysclock>;
> +        reg = <0xff400000 0x400>;
> +        interrupt-parent = <&plic0>;
> +        interrupts = <6>;
> +    };
> +  - |
> +    GAISLER_GRCAN@...40000 {

Especially no such examples. Please read DTS coding style.

> +        reg = <0xff840000 0x400>;
> +        freq = <50000000>;
> +        interrupts = <16>;
> +    };
> diff --git a/Documentation/devicetree/bindings/net/can/grcan.txt b/Documentation/devicetree/bindings/net/can/grcan.txt
> deleted file mode 100644
> index 34ef3498f887..000000000000
> --- a/Documentation/devicetree/bindings/net/can/grcan.txt
> +++ /dev/null
> @@ -1,28 +0,0 @@
Best regards,
Krzysztof