> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Saturday, December 9, 2023 1:53 AM
> To: JeeHeng Sia <jeeheng.sia@xxxxxxxxxxxxxxxx>; kernel@xxxxxxxx; conor@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; paul.walmsley@xxxxxxxxxx; palmer@xxxxxxxxxxx; aou@xxxxxxxxxxxxxxxxx;
> mturquette@xxxxxxxxxxxx; sboyd@xxxxxxxxxx; p.zabel@xxxxxxxxxxxxxx; emil.renner.berthing@xxxxxxxxxxxxx; Hal Feng
> <hal.feng@xxxxxxxxxxxxxxxx>; Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx>
> Cc: linux-riscv@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx; Leyfoon Tan
> <leyfoon.tan@xxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v1 05/16] dt-bindings: clock: Add StarFive JH8100 System clock and reset generator
>
> On 06/12/2023 12:49, Sia Jee Heng wrote:
> > Add bindings for the System clocks and reset generator
> > (SYSCRG) on JH8100 SoC.
> >
> > Signed-off-by: Sia Jee Heng <jeeheng.sia@xxxxxxxxxxxxxxxx>
> > Reviewed-by: Ley Foon Tan <leyfoon.tan@xxxxxxxxxxxxxxxx>
> > ---
>
> ...
>
> > + clocks:
> > + items:
> > + - description: Main Oscillator (24 MHz)
> > + - description: External I2S Rx BCLK clock
> > + - description: External I2S Rx LRCK clock
> > + - description: External MCLK clock
> > +
> > + clock-names:
> > + items:
> > + - const: clk_osc
> > + - const: clk_i2srx_bclk_ext
> > + - const: clk_i2srx_lrck_ext
> > + - const: clk_mclk_ext
>
> Drop clk_ prefixes everywhere.
Noted.
>
> > +
> > + '#clock-cells':
> > + const: 1
> > + description:
> > + See <dt-bindings/clock/starfive,jh8100-crg.h> for valid indices.
> > +
> > + '#reset-cells':
> > + const: 1
> > + description:
> > + See <dt-bindings/reset/starfive-jh8100-crg.h> for valid indices.
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - clocks
> > + - clock-names
> > + - '#clock-cells'
> > + - '#reset-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/clock/starfive,jh8100-crg.h>
> > +
> > + clock-controller@126d0000 {
> > + compatible = "starfive,jh8100-syscrg";
>
> Use 4 spaces for example indentation.
Noted
>
> > + reg = <0x126d0000 0x10000>;
> > + clocks = <&clk_osc>, <&clk_i2srx_bclk_ext>,
> > + <&clk_i2srx_lrck_ext>, <&clk_mclk_ext>;
> > + clock-names = "clk_osc", "clk_i2srx_bclk_ext",
> > + "clk_i2srx_lrck_ext", "clk_mclk_ext";
> > + #clock-cells = <1>;
> > + #reset-cells = <1>;
> > + };
> > diff --git a/include/dt-bindings/clock/starfive,jh8100-crg.h b/include/dt-bindings/clock/starfive,jh8100-crg.h
> > new file mode 100644
> > index 000000000000..e5bb588ce798
> > --- /dev/null
> > +++ b/include/dt-bindings/clock/starfive,jh8100-crg.h
> > @@ -0,0 +1,123 @@
> > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
>
> How about keeping the same license as binding?
Shouldn't b a problem.
>
> > +/*
> > + * Copyright (C) 2023 StarFive Technology Co., Ltd.
> > + * Sia Jee Heng <jeeheng.sia@xxxxxxxxxxxxxxxx>
> > + *
> > + */
> > +
>
> ...
>
> > +#define SYSCRG_CLK_NNE_ICG_EN 108
> > +
> > +#define SYSCRG_CLK_END 109
>
> Drop from binding header.
Do you mean don’t define the number of clk in the header? I'll have to define
It in the driver then..
>
> > +#endif /* __DT_BINDINGS_CLOCK_STARFIVE_JH8100_H__ */
>
> ...
>
> > + */
> > +#define SYSCRG_RSTN_SYS_SYSCON 0
> > +#define SYSCRG_RSTN_CLK_MOD 1
> > +#define SYSCRG_RSTN_GPU 2
> > +#define SYSCRG_RSTN_GPU_SPU 3
> > +#define SYSCRG_RSTN_GPU_TVSENSOR 4
> > +#define SYSCRG_RSTN_PPU_OP_NORET_GPU_RESET 5
> > +#define SYSCRG_RSTN_NNE 6
> > +#define SYSCRG_RSTN_HD_AUDIO 7
> > +
> > +#define SYSCRG_RESET_NR_RESETS 8
>
> Drop from binding header.
Do you mean don’t define the number of reset in the header? I'll have to define
It in the driver then..
>
> > +
> > +#endif /* __DT_BINDINGS_RESET_STARFIVE_JH8100_H__ */
>
> Best regards,
> Krzysztof
