Re: [PATCH v7 02/15] dt-bindings: net: can: gaisler,grcan: Convert to DT schema

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The `gaisler,grhcan` compatible string is missing from the schema.
- [Medium] Incomplete schema validation for required clock properties.
- [Low] Unused header inclusion in the device tree example.
--

commit e39872eda9f93cf639be07e78cd2690d6ce06614
Author: Arun Muthusamy <arun.muthusamy@gaisler.com>

dt-bindings: net: can: gaisler,grcan: Convert to DT schema

This commit converts the device tree bindings for Gaisler GRCAN, GRHCAN,
and GRCANFD CAN controllers from a text format to YAML format. It removes
the stale systemid property, makes freq optional, adds clocks, and provides
an example.

> 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 0000000000000..8fea97c7319ad
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/gaisler,grcan.yaml
> @@ -0,0 +1,62 @@
> +# 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#

[ ... ]

> +properties:
> +  compatible:
> +    enum:
> +      - gaisler,grcan
> +      - gaisler,grcanfd

The commit message, title, and description explicitly state that the binding
supports the GRHCAN CAN controller, and the original text binding also
documented support for it. 

Does this enum unintentionally omit "gaisler,grhcan"? Without it, device
trees specifying the GRHCAN hardware variant might fail schema validation.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  freq:
> +    $ref: /schemas/types.yaml#/definitions/uint32

[ ... ]

> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +

The original text binding required the freq property for the CAN controller
to operate. The new schema makes freq optional and introduces clocks, but
neither is required here. 

Should the schema enforce that at least one clock source is provided using an
anyOf constraint, to prevent device trees lacking clock definitions from
silently passing validation?

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

This isn't a bug, but the example includes irq.h while the interrupts
property is assigned a raw integer <6>. Is this include necessary here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508070121.6918-1-arun.muthusamy@gaisler.com?part=2