Re: [PATCH v1 1/2] dt-bindings: clock: sprd: Add SC9832E clock controller

[Krzysztof Kozlowski <krzk@kernel.org>]

On 20/01/2026 15:44, Nadi Ke wrote:
> +maintainers:
> +  - Orson Zhai <orsonzhai@gmail.com>
> +  - Baolin Wang <baolin.wang7@gmail.com>
> +  - Chunyan Zhang <zhang.lyra@gmail.com>
> +  - Nadi Ke <kanadenady@gmail.com>
> +
> +properties:
> +  "#clock-cells":
> +    const: 1
> +
> +  compatible:

Compatible is always first.

> +    enum:
> +      - sprd,sc9832e-ap-clk
> +      - sprd,sc9832e-aon-clk
> +      - sprd,sc9832e-apahb-gate
> +      - sprd,sc9832e-pmu-gate
> +      - sprd,sc9832e-aonapb-gate
> +      - sprd,sc9832e-apapb-gate
> +      - sprd,sc9832e-pll
> +      - sprd,sc9832e-mpll
> +      - sprd,sc9832e-dpll
> +      - sprd,sc9832e-rpll
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 4
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      The input parent clock(s) phandle for this clock, only list fixed
> +      clocks which are declared in devicetree.
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 4
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - '#clock-cells'

Why reg is not always required?

> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: sprd,sc9832e-ap-clk
> +    then:
> +      required:
> +        - reg
> +      properties:
> +        clocks:
> +          minItems: 2
> +          maxItems: 2
> +        clock-names:
> +          items:
> +            - const: ext-26m
> +            - const: ext-1m
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: sprd,sc9832e-pmu-gate
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 1

Drop

> +          maxItems: 1
> +        clock-names:
> +          items:
> +            - const: ext-26m

And the rest? 3-4 clocks? All other devices?

This binding is really poor and messy. Please write constrained binding.

> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: sprd,sc9832e-aon-clk
> +    then:
> +      required:
> +        - reg
> +
> +description: |

Completely messed placement.

> +  Unisoc SC9832E Clock Control Unit.
> +  Other SC9832E clock nodes should be the child of a syscon node with
> +  compatible: "sprd,sc9832e-glbregs", "syscon", "simple-mfd".

Not relevant. Other schema should define that.

> +  The 'reg' property is also required if there is a sub-range of registers.
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    /* AP Clock Controller */
> +    ap_clk: clock-controller@21500000 {
> +      compatible = "sprd,sc9832e-ap-clk";
> +      reg = <0x21500000 0x1000>;
> +      clocks = <&ext_26m>, <&ext_1m>;
> +      clock-names = "ext-26m", "ext-1m";
> +      #clock-cells = <1>;
> +    };
> +
> +  - |
> +    /* PMU Gate Controller inside Syscon */
> +    syscon@402b0000 {

Drop entire node, not relevant here.

> +      compatible = "sprd,sc9832e-glbregs", "syscon", "simple-mfd";
> +      reg = <0x402b0000 0x4000>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +      ranges = <0 0x402b0000 0x4000>;
> +
> +      pmu_gate: pmu-gate@0 {
> +        compatible = "sprd,sc9832e-pmu-gate";
> +        reg = <0x0 0x1000>;
> +        clocks = <&ext_26m>;
> +        clock-names = "ext-26m";
> +        #clock-cells = <1>;
> +      };
> +    };
> +
> +...
> diff --git a/include/dt-bindings/clock/sprd,sc9832e-clk.h b/include/dt-bindings/clock/sprd,sc9832e-clk.h
> new file mode 100644
> index 000000000..6eff6a82b
> --- /dev/null
> +++ b/include/dt-bindings/clock/sprd,sc9832e-clk.h
> @@ -0,0 +1,187 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * Unisoc SC9832E platform clocks
> + *
> + * Copyright (C) 2015 Spreadtrum, Inc.
> + */
> +
> +#ifndef _DT_BINDINGS_CLK_SC9832E_H_
> +#define _DT_BINDINGS_CLK_SC9832E_H_
> +
> +#define CLK_ISPPLL_GATE		0
> +#define CLK_MPLL_GATE		1
> +#define CLK_DPLL_GATE		2
> +#define CLK_LPLL_GATE		3
> +#define CLK_GPLL_GATE		4
> +#define CLK_PMU_GATE_NUM	(CLK_GPLL_GATE + 1)

Drop, not ABI.

> +
> +#define CLK_TWPLL		0
> +#define CLK_TWPLL_768M		1
> +#define CLK_TWPLL_384M		2
> +#define CLK_TWPLL_192M		3
> +#define CLK_TWPLL_96M		4
> +#define CLK_TWPLL_48M		5
> +#define CLK_TWPLL_24M		6
> +#define CLK_TWPLL_12M		7
> +#define CLK_TWPLL_512M		8
> +#define CLK_TWPLL_256M		9
> +#define CLK_TWPLL_128M		10
> +#define CLK_TWPLL_64M		11
> +#define CLK_TWPLL_307M2		12
> +#define CLK_TWPLL_219M4		13
> +#define CLK_TWPLL_170M6		14
> +#define CLK_TWPLL_153M6		15
> +#define CLK_TWPLL_76M8		16
> +#define CLK_TWPLL_51M2		17
> +#define CLK_TWPLL_38M4		18
> +#define CLK_TWPLL_19M2		19
> +#define CLK_LPLL		20
> +#define CLK_LPLL_409M6		21
> +#define CLK_LPLL_245M76		22
> +#define CLK_GPLL		23
> +#define CLK_ISPPLL		24
> +#define CLK_ISPPLL_468M		25
> +#define CLK_PLL_NUM		(CLK_ISPPLL_468M + 1)

Same here and everywhere else.


Best regards,
Krzysztof