Re: [PATCH net-next 20/37] dt-bindings: can: renesas,rcar-canfd: Restrict resets in top-level

[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 binding never makes `resets`/`reset-names` required for
  RZ/G3L (`renesas,r9a08g046-canfd`), although the driver treats it…
- [Low] The commit message says the patch disallows "reset-names" for the
  two R-Car variants (renesas,rcar-gen3-canfd and renesas,rcar-…

--

dt-bindings: can: renesas,rcar-canfd: Restrict resets in top-level

This commit adds top-level minItems: 1 and maxItems: 2 limits to the
resets property. It drops the per-variant maxItems: 2 lines, which the new
top-level limit makes redundant. It also adds reset-names: false to the
R-Car gen3/gen4 conditional block.

Link: https://patch.msgid.link/20261001060825.53335-2-krzysztof.kozlowski@oss.qualcomm.com

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

[ ... ]

> @@ -232,6 +233,8 @@ allOf:
>          resets:
>            maxItems: 1
>  
> +        reset-names: false
> +

[Severity: Low]
The commit message says:

  Since top-level part says "reset-names" cannot accept one item, disallow
  it for the two variants which do not use it in practice.

Weren't these two variants already blocked from using reset-names before
this patch? Further down in allOf there is an existing block for the same
two compatibles:

  - if:
      properties:
        compatible:
          contains:
            enum:
              - renesas,rcar-gen3-canfd
              - renesas,rcar-gen4-canfd
    then:
      required:
        - resets
      properties:
        reset-names: false

The patch doesn't touch that block. As a result, the added
reset-names: false copies a constraint that already exists, and validation
results don't change.

Could the added line be dropped, or one of the two copies removed and the
commit message reworded? As written, the message describes a behaviour
change that this patch doesn't make.

>    - if:
>        properties:
>          compatible:
> @@ -247,7 +250,6 @@ allOf:
>  
>          resets:
>            minItems: 2
> -          maxItems: 2
>  

[Severity: Medium]
This patch reworks the per-variant reset constraints. Should
renesas,r9a08g046-canfd (RZ/G3L) also be added to the allOf block that
requires resets and reset-names? That block currently lists only:

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

The RZ/G3L block only sets resets: minItems: 2. That constraint applies
only when resets is present.

The driver handles RZ/G3L exactly like RZ/G3S. In
drivers/net/can/rcar/rcar_canfd.c:

  { .compatible = "renesas,r9a08g046-canfd", .data = &r9a08g045_hw_info },

rcar_canfd_probe() looks up both resets by name as optional:

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

Consider an r9a08g046 node with no resets, or with resets but no
reset-names. It passes dtbs_check, and probe gets NULL handles. Then, in
rcar_canfd_global_init(), the calls

  err = reset_control_reset(gpriv->rstc1);
  ...
  err = reset_control_reset(gpriv->rstc2);

do nothing, and rcar_canfd_reset_controller() runs on a block that may
still be held in reset.

This gap came from the earlier commit in this series,
"dt-bindings: can: renesas,rcar-canfd: Document RZ/G3L SoC", not from this
patch. It is still present at the end of the series.

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