Re: [PATCH v3 1/4] dt-bindings: crypto: rockchip: Add RK356x/RK3588 crypto engine binding

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The schema fails to use conditional `if-then` blocks to enforce variant-specific resource constraints for resets, allowing invalid configurations to silently pass validation.
- [Medium] The `rockchip,rk3588-crypto` compatible string incorrectly declares `rockchip,rk3568-crypto` as a fallback despite having incompatible hardware resource requirements.
--

--- Patch [1]: [PATCH v3 1/4] dt-bindings: crypto: rockchip: Add RK356x/RK3588 crypto engine binding ---
commit e7f36faa96ab3720550555cbd2cbc8b8549dc8d0
Author: Dawid Olesinski <[email protected]>

dt-bindings: crypto: rockchip: Add RK356x/RK3588 crypto engine binding

This commit adds a YAML device tree binding for the Rockchip V2 cryptographic
hardware accelerator present on the RK3568 and RK3588 SoCs. It defines
clocks, resets, and interrupts, noting that RK3588 accesses these via the
ARM SCMI firmware interface while RK3568 uses direct CRU access.

> diff --git a/Documentation/devicetree/bindings/crypto/rockchip,rk3588-crypto.yaml b/Documentation/devicetree/bindings/crypto/rockchip,rk3588-crypto.yaml
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/crypto/rockchip,rk3588-crypto.yaml
> @@ -0,0 +1,83 @@
[ ... ]
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: rockchip,rk3568-crypto
> +      - items:
> +          - enum:
> +              - rockchip,rk3588-crypto
> +          - const: rockchip,rk3568-crypto

[Severity: Medium]
Is it safe to declare rockchip,rk3568-crypto as a fallback for
rockchip,rk3588-crypto here?

Since the older RK3568 hardware requires 3 resets and the RK3588 provides
only 1 (via SCMI), wouldn't an older OS probing the RK3588 device using
this fallback string fail to probe due to missing resources?

> +
> +  reg:
> +    maxItems: 1
[ ... ]
> +  clock-names:
> +    items:
> +      - const: core
> +      - const: aclk
> +      - const: hclk
> +
> +  resets:
> +    minItems: 1
> +    maxItems: 3
> +
> +  reset-names:
> +    minItems: 1
> +    items:
> +      - const: core
> +      - const: aclk
> +      - const: hclk

[Severity: Medium]
Should there be an allOf with an if-then conditional block to enforce the
exact number of resets based on the compatible string?

With minItems set to 1 globally, an incomplete device tree node for RK3568
with only one reset would silently pass dt_binding_check validation, even
though the hardware requires all three.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1