Message-ID: <a5d8543b27adc6adf8bec2f3548f13b0@gaisler.com>
Date: Mon, 24 Nov 2025 10:37:40 +0100
From: Arun Muthusamy <arun.muthusamy@...sler.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 mkl@...gutronix.de, mailhol@...nel.org, 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

Hi Krzysztof,

Thank you for your thorough review and insightful questions. I’d like to 
clarify a few points regarding the DT binding and get your guidance.

Node name vs. compatible matching:
SPARC systems do not use DTS files; the device tree is generated by the 
PROM. On LEON (SPARC32), AMBA Plug & Play information creates the DT 
properties, and drivers historically match devices based on node names.
For DTS-based systems such as NOEL, this patch series adds 
compatible-string matching. To reflect this, I updated the $nodename 
pattern to support LEON-style node names:
properties:
   $nodename:
     pattern: "^(GAISLER_GRCAN|01_03d|GAISLER_GRHCAN|01_034)$"
I’d appreciate any suggestions on the preferred way to describe this 
dual matching approach: node name for PROM-based LEON, compatible string 
for DTS-based NOEL.


Freq and Clocks:
The driver needs to support both LEON and NOEL platforms:
LEON: relies on the freq property
NOEL: uses a standard clocks binding
Because of this dual approach, the freq property is no longer required 
in the DTS binding itself.
It is only relevant for LEON/PROM-based systems and is handled 
internally by the driver


Systemid:
The driver now reads systemid directly from /ambapp0, so the property no 
longer needs to be defined in the DTS. The previous documentation was 
outdated and should have been updated after commit:
1e93ed26acf0 ("can: grcan: grcan_probe(): fix broken system id check for 
errata workaround needs")


Thanks,

-- 
BR,

Arun Muthusamy
Software Engineer
Frontgrade Gaisler
T : +46 (0) 700 558 528
arun.muthusamy@...sler.com

Frontgrade Gaisler AB, Kungsgatan 12, SE-411 19 GÖTEBORG, Sweden.
+46 (0) 31 775 8650, www.gaisler.com

On 18.11.2025 12:01, Krzysztof Kozlowski wrote:
> 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

Powered by blists - more mailing lists