RE: [PATCH v1 05/16] dt-bindings: clock: Add StarFive JH8100 System clock and reset generator

[JeeHeng Sia <jeeheng.sia@starfivetech.com>]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Saturday, December 9, 2023 1:53 AM
> To: JeeHeng Sia <jeeheng.sia@starfivetech.com>; kernel@esmil.dk; conor@kernel.org; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; paul.walmsley@sifive.com; palmer@dabbelt.com; aou@eecs.berkeley.edu;
> mturquette@baylibre.com; sboyd@kernel.org; p.zabel@pengutronix.de; emil.renner.berthing@canonical.com; Hal Feng
> <hal.feng@starfivetech.com>; Xingyu Wu <xingyu.wu@starfivetech.com>
> Cc: linux-riscv@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-clk@vger.kernel.org; Leyfoon Tan
> <leyfoon.tan@starfivetech.com>
> 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@starfivetech.com>
> > Reviewed-by: Ley Foon Tan <leyfoon.tan@starfivetech.com>
> > ---
> 
> ...
> 
> > +  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@starfivetech.com>
> > + *
> > + */
> > +
> 
> ...
> 
> > +#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