Re: [PATCH 13/26] clk: amlogic: s4-pll: naming consistency alignment

[Chuan Liu <chuan.liu@amlogic.com>]

Hi Jerome:

     It's good for me. Thanks!


Reviewed-by: Chuan Liu <chuan.liu@amlogic.com>


On 7/2/2025 11:26 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> Amlogic clock controller drivers are all doing the same thing, more or
> less. Yet, over the years, tiny (and often pointless) differences have
> emerged.
>
> This makes reviews more difficult, allows some errors to slip through and
> make it more difficult to exploit SoC commonalities, leading to code
> duplication.
>
> This change enforce, wherever possible, a consistent and predictable scheme
> when it comes to code organisation and naming, The scheme chosen is what
> was used the most already, to try and minimise the size of the ugly
> resulting diff. Here are some of the rules applied:
> - Aligning clock names, variable names and IDs.
>    - ID cannot change (used in DT)
>    - Variable names w/ SoC name prefixes
>    - Clock names w/o SoC name prefixes, except pclks for historic reasons
> - Composite clock systematic naming : mux: X_sel, div:X_div, gate:X
> - Parent table systematically named with the same name as the clock and
>    a '_parents' suffix
> - Group various tables next to the related clock
> - etc ...
>
> Doing so removes what would otherwise show up as unrelated diff in
> following changes. It will allow to introduce common definitions for
> peripheral clocks, probe helpers, composite clocks, etc ... making further
> review and maintenance easier.
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>   drivers/clk/meson/s4-pll.c | 34 +++++++++++++++++-----------------
>   1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/clk/meson/s4-pll.c b/drivers/clk/meson/s4-pll.c
> index 3d689d2f003e21658016b84918bf1b7b1954c05a..6a266bcafd6257937c1de50cbc5606dcc6f8207b 100644
> --- a/drivers/clk/meson/s4-pll.c
> +++ b/drivers/clk/meson/s4-pll.c
> @@ -281,7 +281,7 @@ static const struct pll_mult_range s4_gp0_pll_mult_range = {
>   /*
>    * Internal gp0 pll emulation configuration parameters
>    */
> -static const struct reg_sequence s4_gp0_init_regs[] = {
> +static const struct reg_sequence s4_gp0_pll_init_regs[] = {
>          { .reg = ANACTRL_GP0PLL_CTRL1,  .def = 0x00000000 },
>          { .reg = ANACTRL_GP0PLL_CTRL2,  .def = 0x00000000 },
>          { .reg = ANACTRL_GP0PLL_CTRL3,  .def = 0x48681c00 },
> @@ -318,8 +318,8 @@ static struct clk_regmap s4_gp0_pll_dco = {
>                          .width   = 1,
>                  },
>                  .range = &s4_gp0_pll_mult_range,
> -               .init_regs = s4_gp0_init_regs,
> -               .init_count = ARRAY_SIZE(s4_gp0_init_regs),
> +               .init_regs = s4_gp0_pll_init_regs,
> +               .init_count = ARRAY_SIZE(s4_gp0_pll_init_regs),
>          },
>          .hw.init = &(struct clk_init_data){
>                  .name = "gp0_pll_dco",
> @@ -353,7 +353,7 @@ static struct clk_regmap s4_gp0_pll = {
>   /*
>    * Internal hifi pll emulation configuration parameters
>    */
> -static const struct reg_sequence s4_hifi_init_regs[] = {
> +static const struct reg_sequence s4_hifi_pll_init_regs[] = {
>          { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00000000 },
>          { .reg = ANACTRL_HIFIPLL_CTRL3, .def = 0x6a285c00 },
>          { .reg = ANACTRL_HIFIPLL_CTRL4, .def = 0x65771290 },
> @@ -394,8 +394,8 @@ static struct clk_regmap s4_hifi_pll_dco = {
>                          .width   = 1,
>                  },
>                  .range = &s4_gp0_pll_mult_range,
> -               .init_regs = s4_hifi_init_regs,
> -               .init_count = ARRAY_SIZE(s4_hifi_init_regs),
> +               .init_regs = s4_hifi_pll_init_regs,
> +               .init_count = ARRAY_SIZE(s4_hifi_pll_init_regs),
>                  .frac_max = 100000,
>                  .flags = CLK_MESON_PLL_ROUND_CLOSEST,
>          },
> @@ -794,11 +794,11 @@ static struct clk_hw *s4_pll_hw_clks[] = {
>          [CLKID_MPLL3]                   = &s4_mpll3.hw,
>   };
>
> -static const struct reg_sequence s4_init_regs[] = {
> +static const struct reg_sequence s4_pll_init_regs[] = {
>          { .reg = ANACTRL_MPLL_CTRL0,    .def = 0x00000543 },
>   };
>
> -static const struct regmap_config clkc_regmap_config = {
> +static const struct regmap_config s4_pll_clkc_regmap_cfg = {
>          .reg_bits       = 32,
>          .val_bits       = 32,
>          .reg_stride     = 4,
> @@ -810,7 +810,7 @@ static struct meson_clk_hw_data s4_pll_clks = {
>          .num = ARRAY_SIZE(s4_pll_hw_clks),
>   };
>
> -static int meson_s4_pll_probe(struct platform_device *pdev)
> +static int s4_pll_clkc_probe(struct platform_device *pdev)
>   {
>          struct device *dev = &pdev->dev;
>          struct regmap *regmap;
> @@ -822,12 +822,12 @@ static int meson_s4_pll_probe(struct platform_device *pdev)
>                  return dev_err_probe(dev, PTR_ERR(base),
>                                       "can't ioremap resource\n");
>
> -       regmap = devm_regmap_init_mmio(dev, base, &clkc_regmap_config);
> +       regmap = devm_regmap_init_mmio(dev, base, &s4_pll_clkc_regmap_cfg);
>          if (IS_ERR(regmap))
>                  return dev_err_probe(dev, PTR_ERR(regmap),
>                                       "can't init regmap mmio region\n");
>
> -       ret = regmap_multi_reg_write(regmap, s4_init_regs, ARRAY_SIZE(s4_init_regs));
> +       ret = regmap_multi_reg_write(regmap, s4_pll_init_regs, ARRAY_SIZE(s4_pll_init_regs));
>          if (ret)
>                  return dev_err_probe(dev, ret,
>                                       "Failed to init registers\n");
> @@ -848,22 +848,22 @@ static int meson_s4_pll_probe(struct platform_device *pdev)
>                                             &s4_pll_clks);
>   }
>
> -static const struct of_device_id clkc_match_table[] = {
> +static const struct of_device_id s4_pll_clkc_match_table[] = {
>          {
>                  .compatible = "amlogic,s4-pll-clkc",
>          },
>          {}
>   };
> -MODULE_DEVICE_TABLE(of, clkc_match_table);
> +MODULE_DEVICE_TABLE(of, s4_pll_clkc_match_table);
>
> -static struct platform_driver s4_driver = {
> -       .probe          = meson_s4_pll_probe,
> +static struct platform_driver s4_pll_clkc_driver = {
> +       .probe          = s4_pll_clkc_probe,
>          .driver         = {
>                  .name   = "s4-pll-clkc",
> -               .of_match_table = clkc_match_table,
> +               .of_match_table = s4_pll_clkc_match_table,
>          },
>   };
> -module_platform_driver(s4_driver);
> +module_platform_driver(s4_pll_clkc_driver);
>
>   MODULE_DESCRIPTION("Amlogic S4 PLL Clock Controller driver");
>   MODULE_AUTHOR("Yu Tu <yu.tu@amlogic.com>");
>
> --
> 2.47.2
>
>
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic