On 2023/2/22 17:11, Krzysztof Kozlowski wrote: > On 21/02/2023 15:11, Xingyu Wu wrote: >> Add bindings for the PLL clock generator on the JH7110 RISC-V SoC. > > Just one sentence... not explaining anything around ABI break. Nope. > >> >> Signed-off-by: Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx> >> --- >> .../bindings/clock/starfive,jh7110-pll.yaml | 45 +++++++++++++++++++ >> .../dt-bindings/clock/starfive,jh7110-crg.h | 12 ++--- >> 2 files changed, 52 insertions(+), 5 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml >> >> diff --git a/Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml b/Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml >> new file mode 100644 >> index 000000000000..8fd18e6c2e9b >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/clock/starfive,jh7110-pll.yaml >> @@ -0,0 +1,45 @@ >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/clock/starfive,jh7110-pll.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: StarFive JH7110 PLL Clock Generator >> + >> +maintainers: >> + - Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx> >> + >> +properties: >> + compatible: >> + const: starfive,jh7110-pll >> + >> + clocks: >> + maxItems: 1 >> + - description: Main Oscillator (24 MHz) > > Does not look like you tested the bindings. Please run `make > dt_binding_check` (see > Documentation/devicetree/bindings/writing-schema.rst for instructions). Remove the ' - ' before 'description' and pass the dt_binding_check. > >> + >> + '#clock-cells': >> + const: 1 >> + description: >> + See <dt-bindings/clock/starfive,jh7110-crg.h> for valid indices. >> + >> + starfive,sysreg: >> + $ref: /schemas/types.yaml#/definitions/phandle-array > > That's not how the property is modeled... look at other code. Remove the '-array'. > >> + description: >> + the phandle to System Register Controller syscon node. >> + >> +required: >> + - compatible >> + - clocks >> + - '#clock-cells' >> + - starfive,sysreg >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + pllclk: pll-clock-controller { >> + compatible = "starfive,jh7110-pll"; >> + clocks = <&osc>; >> + #clock-cells = <1>; >> + starfive,sysreg = <&sys_syscon>; >> + }; >> diff --git a/include/dt-bindings/clock/starfive,jh7110-crg.h b/include/dt-bindings/clock/starfive,jh7110-crg.h >> index 5e4f21ca0642..086a6ddcf380 100644 >> --- a/include/dt-bindings/clock/starfive,jh7110-crg.h >> +++ b/include/dt-bindings/clock/starfive,jh7110-crg.h >> @@ -6,6 +6,12 @@ >> #ifndef __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__ >> #define __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__ >> >> +/* PLL clocks */ >> +#define JH7110_CLK_PLL0_OUT 0 >> +#define JH7110_CLK_PLL1_OUT 1 >> +#define JH7110_CLK_PLL2_OUT 2 >> +#define JH7110_PLLCLK_END 3 >> + >> /* SYSCRG clocks */ >> #define JH7110_SYSCLK_CPU_ROOT 0 >> #define JH7110_SYSCLK_CPU_CORE 1 >> @@ -198,11 +204,7 @@ >> #define JH7110_SYSCLK_TDM_TDM_INV 188 >> #define JH7110_SYSCLK_JTAG_CERTIFICATION_TRNG 189 >> >> -#define JH7110_SYSCLK_PLL0_OUT 190 >> -#define JH7110_SYSCLK_PLL1_OUT 191 >> -#define JH7110_SYSCLK_PLL2_OUT 192 >> - > > > NAK. ABI break. You have entire commit msg to explain this and avoid > questions from reviewers. > Discussed with Hal.Feng to delete this part and move it to driver in his next patch series. Best regards, Xingyu Wu
