Re: [PATCH RFC v2 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 4 potential issue(s) to consider:
- [Medium] The device tree binding intentionally omits the hardware clock dependency between the top controller and the matrix controller.
- [Low] Missing 'clock-frequency' property in 'fixed-clock' example nodes.
- [Low] The `zte,zx297520v3-matrixclk` compatible string lacks an `if-then` block to explicitly forbid `clocks` and `clock-names`.
- [Low] The node names in the example block do not follow generic naming standards.
--

commit dcd964f6e480b59678a5c8aefa3f700c7eec5d64
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 SoC
clock and reset controllers. It defines the top, matrix, and lsp
controllers, along with their respective clock and reset definitions.

> 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
[ ... ]
> +description: |
> +  The zx297520v3's clock controller consists of 3 controllers, which generate
> +  clocks for internal SoC devices. In addition to clocks it also has reset
> +  controls for most, but not all, devices.
[ ... ]
> +  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.

Does omitting the clock relationship between the top and matrix controllers
hide the physical hardware topology? Device tree bindings should typically
describe the actual hardware connections.

By leaving out the clocks property from the matrixclk node to reference the
topclk outputs, could this force the driver to rely on implicit dependencies
for probe ordering and break proper frequency propagation in the clock
framework?

[ ... ]
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: zte,zx297520v3-lspclk
> +    then:
> +      properties:
> +        clocks:
[ ... ]
> +      required:
> +        - clocks
> +        - clock-names
> +
> +additionalProperties: false

Since clocks and clock-names are defined at the top level of this binding,
would an invalid matrixclk device tree node mistakenly including those
properties silently pass validation?

Because additionalProperties: false only rejects completely undefined
properties, should there be an if-then block for matrixclk that explicitly
sets clocks: false and clock-names: false to prevent this?

[ ... ]
> +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>;
> +    };

Do these fixed-clock examples need the clock-frequency property? The core
schema for fixed clocks mandates this property, which might cause
dt_binding_check to fail if it is missing here.

> +    topclk: topclk@13b000 {
> +        compatible = "zte,zx297520v3-topclk";
> +        reg = <0x0013b000 0x400>;
> +        #clock-cells = <1>;
> +        #reset-cells = <1>;
> +        clocks = <&osc26m>, <&osc32k>;
> +        clock-names = "osc26m", "osc32k";
> +    };
> +
> +    matrixclk: matrixclk@1306000 {
> +        compatible = "zte,zx297520v3-matrixclk";
> +        reg = <0x01306000 0x400>;
> +        #clock-cells = <1>;
> +        #reset-cells = <1>;
> +    };
> +
> +    lspclk: lspclk@1400000 {
> +        compatible = "zte,zx297520v3-lspclk";
> +        reg = <0x01400000 0x100>;

Are these node names deviating from generic naming standards? The devicetree
specification requires node names to describe the generic class of a device
rather than specific block names.

Could these be renamed using the clock-controller@... pattern to comply
with conventions and avoid schema warnings?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-zx29clk-v2-0-29f0edc300f5@gmail.com?part=1