Re: [PATCH v4 3/5] dt-bindings: clock: Move xlnx,zynqmp-clk to its own schema

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The regex pattern for `mio_clk` in `clock-names` contains a logic error that fails to correctly constrain the index to 00-77.
- [Low] The example in `xlnx,versal-clk.yaml` fails schema validation because it wraps a `versal-clk` clock controller in a `zynqmp-firmware` node.
--

--- Patch [3]: [PATCH v4 3/5] dt-bindings: clock: Move xlnx,zynqmp-clk to its own schema ---
commit 718990212b2e54a25ccb16a23d741b1b1a896e2d
Author: Michal Simek <michal.simek@amd.com>

dt-bindings: clock: Move xlnx,zynqmp-clk to its own schema

The ZynqMP clock controller binding shares only #clock-cells with the
Versal bindings. Move it to a dedicated xlnx,zynqmp-clk.yaml schema.
Also remove "(Optional clock)" from clock description because it is visible
from schema itself.

> diff --git a/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml b/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml
> index b1623c2ab0f69..12d060c39bfc4 100644
> --- a/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml
> +++ b/Documentation/devicetree/bindings/clock/xlnx,versal-clk.yaml
[ ... ]
>  examples:
>    - |
>      firmware {
>        zynqmp_firmware: zynqmp-firmware {
>          compatible = "xlnx,zynqmp-firmware";
>          method = "smc";
>          versal_clk: clock-controller {
>            #clock-cells = <1>;
>            compatible = "xlnx,versal-clk";

[Severity: Low]
Will this example now fail make dt_binding_check validation?

Because xlnx,zynqmp-firmware.yaml is modified in this patch to add an allOf
rule requiring the clock-controller child of zynqmp-firmware to use
compatible = "xlnx,zynqmp-clk", this xlnx,versal-clk node inside a
zynqmp-firmware example seems like it will trigger a schema validation error.

>            clocks = <&pss_ref_clk>, <&video_clk>, <&pss_alt_ref_clk>;
>            clock-names = "pss_ref_clk", "video_clk", "pss_alt_ref_clk";
>          };
>        };
>      };
[ ... ]
> diff --git a/Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.yaml b/Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.yaml
> new file mode 100644
> index 0000000000000..c64550471581c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/xlnx,zynqmp-clk.yaml
[ ... ]
> +  clock-names:
> +    minItems: 5
> +    items:
> +      - const: pss_ref_clk
> +      - const: video_clk
> +      - const: pss_alt_ref_clk
> +      - const: aux_ref_clk
> +      - const: gt_crx_ref_clk
> +      - pattern: "^mio_clk([0-6][0-9]|7[0-7])+.*$"

[Severity: Medium]
Does this pattern correctly constrain the clock index to 00-77?

By combining the + quantifier with .*, this regex allows strings like
"mio_clk100" to validate, since the "10" matches the capturing group and the
remaining "0" matches ".*". Could this be corrected to avoid permitting out of
bounds values?

> +      - pattern: "gem[0-3]+_emio_clk.*$"
> +      - pattern: "swdt[0-1]+_ext_clk.*$"

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783495122.git.michal.simek@amd.com?part=3