Re: [PATCH net-next 16/37] dt-bindings: can: renesas,rcar-canfd: Document RZ/G3L SoC

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch adds the RZ/G3L compatible (renesas,r9a08g046-canfd)
  but does not add it to the existing allOf block in…
- [Low] The new RZ/G3L allOf block in
  Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml…

--

dt-bindings: can: renesas,rcar-canfd: Document RZ/G3L SoC

This commit adds the renesas,r9a08g046-canfd compatible for the RZ/G3L SoC
to the R-Car CAN FD binding. It adds a new allOf block for RZ/G3L that
covers 11 interrupts, 2 resets and channels 0-2. It also adds RZ/G3L next to
RZ/G3S in the block that disallows renesas,no-can-fd and renesas,fd-only.

Link: https://patch.msgid.link/20261001075742.21961-2-biju.das.jz@bp.renesas.com

> diff --git a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> index bbaaef8f4282e..cef20f0403715 100644
> --- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
> +++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml

[ ... ]

> @@ -292,6 +293,25 @@ allOf:
>        patternProperties:
>          "^channel[6-7]$": false
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: renesas,r9a08g046-canfd
> +    then:
> +      properties:
> +        interrupts:
> +          minItems: 11
> +
> +        interrupt-names:
> +          minItems: 11

[Severity: Low]
Should interrupts and interrupt-names also have maxItems: 11 here?

The top-level interrupts and interrupt-names lists have 20 entries. With
only a lower bound in this block, the schema accepts anywhere from 11 to 20
entries for RZ/G3L. That means ch3_*, ch4_* and ch5_* interrupts pass
validation, even though this same block sets "^channel[3-7]$": false. The
commit message also says the part has 11 interrupts (2 global plus 3 for
each of channels 0-2).

The other variants use exact counts:

  RZ/G3S and RZ/G2L: maxItems: 8, combined with the top-level minItems: 8
  RZ/G3E: minItems: 20, combined with the implied top-level maximum of 20

Since 11 falls between those two bounds, RZ/G3L seems to need both a
minimum and a maximum. The upper bound is still missing at the end of the
series.

resets had the same problem at this commit, since the top level was
resets: true. That part is fixed later in the series by "dt-bindings: can:
renesas,rcar-canfd: Restrict resets in top-level", which changes the top
level to minItems: 1, maxItems: 2.

> +
> +        resets:
> +          minItems: 2

[Severity: Medium]
Is renesas,r9a08g046-canfd missing from the existing allOf block that makes
resets and reset-names required? That block still reads:

  - if:
      properties:
        compatible:
          contains:
            enum:
              - renesas,r9a08g045-canfd
              - renesas,r9a09g047-canfd
              - renesas,rzg2l-canfd
    then:
      required:
        - resets
        - reset-names

The minItems: 2 here only applies when resets is present, so it doesn't
make the property required. An RZ/G3L node with no resets and no
reset-names would still pass dtbs_check. Every other variant either
requires resets or forbids them (RZ/T2H).

The driver won't catch the omission either. rcar_canfd_probe() gets both
resets as optional:

  gpriv->rstc1 = devm_reset_control_get_optional_exclusive(dev, "rstp_n");
  ...
  gpriv->rstc2 = devm_reset_control_get_optional_exclusive(dev, "rstc_n");

These calls return NULL when the resets are absent. rcar_canfd_global_init()
then calls reset_control_reset() on those NULL handles, which does nothing.
Probe succeeds, but the controller is never reset or released from reset.

This patch does add r9a08g046 next to r9a08g045 in the no-can-fd/fd-only
block. Later in the series, neither "can: rcar_canfd: Add support for
Renesas RZ/G3L" nor "dt-bindings: can: renesas,rcar-canfd: Restrict resets
in top-level" adds it to the required-resets enum.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009134323.64064-2-mkl%40pengutronix.de