Re: [PATCH RFC 1/4] dt-bindings: clk: zte: Add zx297520v3 clock and reset bindings.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Incomplete Device Tree modeling and schema validation for the matrix clock controller.
- [Low] The 'osc26m' and 'osc32k' fixed-clock example nodes are missing the mandatory 'clock-frequency' property.
- [Low] Non-standard generic node names in schema examples.
--

commit 241b89034c405c46e2b45636517463ab2381b279
Author: Stefan Dösinger <stefandoesinger@gmail.com>

dt-bindings: clk: zte: Add zx297520v3 clock and reset bindings.

This commit introduces device tree bindings for the ZTE zx297520v3 clock
and reset controllers. It defines compatible strings for the top, matrix,
and lsp controllers, along with required properties and standard macros.

> diff --git a/Documentation/devicetree/bindings/clock/zte,zx297520v3-clk.yaml b/Documentation/devicetree/bindings/clock/zte,zx297520v3-clk.yaml
> new file mode 100644
> index 0000000000000..3b7084a18a975
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/zte,zx297520v3-clk.yaml

[ ... ]

> +  The matrix controller consumes clocks generated by PLLs in the top
> +  controller, but there are no controls in the top controller to sever this
> +  link. The interface between these controllers is not expressed in the device
> +  tree, but the matrix controller cannot work without the clock handles
> +  registered by the top controller.

Should this hardware dependency be explicitly modeled in the device tree?
Device tree design principles generally rely on explicitly modeled
connections to handle probe ordering correctly.

[ ... ]

> +properties:
> +  compatible:
> +    enum:
> +      - zte,zx297520v3-topclk
> +      - zte,zx297520v3-matrixclk
> +      - zte,zx297520v3-lspclk
> +
> +  clocks:
> +    minItems: 2
> +    maxItems: 10
> +
> +  clock-names:
> +    minItems: 2
> +    maxItems: 10

[ ... ]

> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: zte,zx297520v3-topclk

[ ... ]

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: zte,zx297520v3-lspclk

Since clocks and clock-names are defined globally in the schema, but there
is no conditional block explicitly forbidding them for the matrix controller
using zte,zx297520v3-matrixclk, will the schema silently permit invalid clock
assignments for the matrix node?

Could an if/then block be added to set clocks to false for the matrix
controller?

[ ... ]

> +examples:
> +  - |
> +    #include <dt-bindings/clock/zte,zx297520v3-clk.h>
> +
> +    osc26m: osc26m {
> +      compatible = "fixed-clock";
> +      clock-output-names = "osc26m";
> +      #clock-cells = <0>;
> +    };
> +
> +    osc32k: osc32k {
> +      compatible = "fixed-clock";
> +      clock-output-names = "osc32k";
> +      #clock-cells = <0>;
> +    };

Are these fixed-clock example nodes missing the mandatory clock-frequency
property? Omitting it might cause dt_binding_check to report schema
validation errors.

> +
> +    topclk: topclk@13b000 {
> +        compatible = "zte,zx297520v3-topclk";

[ ... ]

> +    matrixclk: matrixclk@1306000 {
> +        compatible = "zte,zx297520v3-matrixclk";

[ ... ]

> +    lspclk: lspclk@1400000 {
> +        compatible = "zte,zx297520v3-lspclk";

This isn't a bug, but should these node names reflect the generic class
of the device, like clock-controller@13b000, instead of hardware block
designations like topclk, matrixclk, and lspclk? Standard conventions
require generic node names, and dt_binding_check might warn about this.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-zx29clk-v1-0-e1bacfffe967@gmail.com?part=1