Re: [PATCH 12/26] clk: amlogic: s4-peripherals: naming consistency alignment

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

Hi Jerome:


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-peripherals.c | 746 ++++++++++++++++++-------------------
>   1 file changed, 370 insertions(+), 376 deletions(-)
>
> diff --git a/drivers/clk/meson/s4-peripherals.c b/drivers/clk/meson/s4-peripherals.c
> index c9400cf54c84c3dc7c63d0636933951b0cac230c..9bcd35f12836de5e318fd1ad9c9ae15a2bfc3dd7 100644
> --- a/drivers/clk/meson/s4-peripherals.c
> +++ b/drivers/clk/meson/s4-peripherals.c


[...]


> @@ -1320,7 +1320,7 @@ static struct clk_regmap s4_ts_clk_gate = {
>    * mux because it does top-to-bottom updates the each clock tree and
>    * switches to the "inactive" one when CLK_SET_RATE_GATE is set.
>    */
> -static const struct clk_parent_data s4_mali_0_1_parent_data[] = {
> +static const struct clk_parent_data s4_mali_parents[] = {
>          { .fw_name = "xtal", },
>          { .fw_name = "gp0_pll", },
>          { .fw_name = "hifi_pll", },
> @@ -1340,8 +1340,8 @@ static struct clk_regmap s4_mali_0_sel = {
>          .hw.init = &(struct clk_init_data){
>                  .name = "mali_0_sel",
>                  .ops = &clk_regmap_mux_ops,
> -               .parent_data = s4_mali_0_1_parent_data,
> -               .num_parents = ARRAY_SIZE(s4_mali_0_1_parent_data),
> +               .parent_data = s4_mali_parents,
> +               .num_parents = ARRAY_SIZE(s4_mali_parents),
>                  /*
>                   * Don't request the parent to change the rate because
>                   * all GPU frequencies can be derived from the fclk_*
> @@ -1394,8 +1394,8 @@ static struct clk_regmap s4_mali_1_sel = {
>          .hw.init = &(struct clk_init_data){
>                  .name = "mali_1_sel",
>                  .ops = &clk_regmap_mux_ops,
> -               .parent_data = s4_mali_0_1_parent_data,
> -               .num_parents = ARRAY_SIZE(s4_mali_0_1_parent_data),
> +               .parent_data = s4_mali_parents,
> +               .num_parents = ARRAY_SIZE(s4_mali_parents),
>                  .flags = 0,
>          },
>   };
> @@ -1433,28 +1433,26 @@ static struct clk_regmap s4_mali_1 = {
>          },
>   };
>
> -static const struct clk_hw *s4_mali_parent_hws[] = {
> -       &s4_mali_0.hw,
> -       &s4_mali_1.hw
> -};
> -
> -static struct clk_regmap s4_mali_mux = {
> +static struct clk_regmap s4_mali_sel = {
>          .data = &(struct clk_regmap_mux_data){
>                  .offset = CLKCTRL_MALI_CLK_CTRL,
>                  .mask = 1,
>                  .shift = 31,
>          },
>          .hw.init = &(struct clk_init_data){
> -               .name = "mali",
> +               .name = "mali_sel",
>                  .ops = &clk_regmap_mux_ops,
> -               .parent_hws = s4_mali_parent_hws,
> +               .parent_hws = (const struct clk_hw *[]) {
> +                       &s4_mali_0.hw,
> +                       &s4_mali_1.hw,
> +               },
>                  .num_parents = 2,
>                  .flags = CLK_SET_RATE_PARENT,
>          },
>   };
>
>   /* VDEC clocks */
> -static const struct clk_parent_data s4_dec_parent_data[] = {
> +static const struct clk_parent_data s4_dec_parents[] = {
>          { .fw_name = "fclk_div2p5", },
>          { .fw_name = "fclk_div3", },
>          { .fw_name = "fclk_div4", },
> @@ -1465,7 +1463,7 @@ static const struct clk_parent_data s4_dec_parent_data[] = {
>          { .fw_name = "xtal", }
>   };
>
> -static struct clk_regmap s4_vdec_p0_mux = {
> +static struct clk_regmap s4_vdec_p0_sel = {


Since both vdec_clk and mali_clk are 'no glitch clock', should we also unify
the naming from 's4_vdec_p0'/'s4_vdec_p1' to 's4_vdec_0'/'s4_vdec_1'?


>          .data = &(struct clk_regmap_mux_data){
>                  .offset = CLKCTRL_VDEC_CLK_CTRL,
>                  .mask = 0x7,
> @@ -1473,10 +1471,10 @@ static struct clk_regmap s4_vdec_p0_mux = {
>                  .flags = CLK_MUX_ROUND_CLOSEST,
>          },
>          .hw.init = &(struct clk_init_data) {
> -               .name = "vdec_p0_mux",
> +               .name = "vdec_p0_sel",
>                  .ops = &clk_regmap_mux_ops,
> -               .parent_data = s4_dec_parent_data,
> -               .num_parents = ARRAY_SIZE(s4_dec_parent_data),
> +               .parent_data = s4_dec_parents,
> +               .num_parents = ARRAY_SIZE(s4_dec_parents),
>                  .flags = 0,
>          },
>   };
> @@ -1492,7 +1490,7 @@ static struct clk_regmap s4_vdec_p0_div = {
>                  .name = "vdec_p0_div",
>                  .ops = &clk_regmap_divider_ops,
>                  .parent_hws = (const struct clk_hw *[]) {
> -                       &s4_vdec_p0_mux.hw
> +                       &s4_vdec_p0_sel.hw
>                  },
>                  .num_parents = 1,
>                  .flags = CLK_SET_RATE_PARENT,
> @@ -1515,7 +1513,7 @@ static struct clk_regmap s4_vdec_p0 = {
>          },
>   };
>
> -static struct clk_regmap s4_vdec_p1_mux = {
> +static struct clk_regmap s4_vdec_p1_sel = {
>          .data = &(struct clk_regmap_mux_data){
>                  .offset = CLKCTRL_VDEC3_CLK_CTRL,
>                  .mask = 0x7,
> @@ -1523,10 +1521,10 @@ static struct clk_regmap s4_vdec_p1_mux = {
>                  .flags = CLK_MUX_ROUND_CLOSEST,
>          },
>          .hw.init = &(struct clk_init_data) {
> -               .name = "vdec_p1_mux",
> +               .name = "vdec_p1_sel",
>                  .ops = &clk_regmap_mux_ops,
> -               .parent_data = s4_dec_parent_data,
> -               .num_parents = ARRAY_SIZE(s4_dec_parent_data),
> +               .parent_data = s4_dec_parents,
> +               .num_parents = ARRAY_SIZE(s4_dec_parents),
>                  .flags = 0,
>          },
>   };
> @@ -1542,7 +1540,7 @@ static struct clk_regmap s4_vdec_p1_div = {
>                  .name = "vdec_p1_div",
>                  .ops = &clk_regmap_divider_ops,
>                  .parent_hws = (const struct clk_hw *[]) {
> -                       &s4_vdec_p1_mux.hw
> +                       &s4_vdec_p1_sel.hw
>                  },
>                  .num_parents = 1,
>                  .flags = CLK_SET_RATE_PARENT,
> @@ -1565,27 +1563,25 @@ static struct clk_regmap s4_vdec_p1 = {
>          },
>   };
>
> -static const struct clk_hw *s4_vdec_mux_parent_hws[] = {
> -       &s4_vdec_p0.hw,
> -       &s4_vdec_p1.hw
> -};
> -
> -static struct clk_regmap s4_vdec_mux = {
> +static struct clk_regmap s4_vdec_sel = {
>          .data = &(struct clk_regmap_mux_data){
>                  .offset = CLKCTRL_VDEC3_CLK_CTRL,
>                  .mask = 0x1,
>                  .shift = 15,
>          },
>          .hw.init = &(struct clk_init_data) {
> -               .name = "vdec_mux",
> +               .name = "vdec_sel",
>                  .ops = &clk_regmap_mux_ops,
> -               .parent_hws = s4_vdec_mux_parent_hws,
> -               .num_parents = ARRAY_SIZE(s4_vdec_mux_parent_hws),
> +               .parent_hws = (const struct clk_hw *[]) {
> +                       &s4_vdec_p0.hw,
> +                       &s4_vdec_p1.hw,
> +               },
> +               .num_parents = 2,
>                  .flags = CLK_SET_RATE_PARENT,
>          },
>   };
>
> -static struct clk_regmap s4_hevcf_p0_mux = {
> +static struct clk_regmap s4_hevcf_p0_sel = {


+static struct clk_regmap s4_hevcf_0_sel
+static struct clk_regmap s4_hevcf_0_div
.
.
.


>          .data = &(struct clk_regmap_mux_data){
>                  .offset = CLKCTRL_VDEC2_CLK_CTRL,
>                  .mask = 0x7,
> @@ -1593,10 +1589,10 @@ static struct clk_regmap s4_hevcf_p0_mux = {
>                  .flags = CLK_MUX_ROUND_CLOSEST,
>          },
>          .hw.init = &(struct clk_init_data) {
> -               .name = "hevcf_p0_mux",
> +               .name = "hevcf_p0_sel",
>                  .ops = &clk_regmap_mux_ops,
> -               .parent_data = s4_dec_parent_data,
> -               .num_parents = ARRAY_SIZE(s4_dec_parent_data),
> +               .parent_data = s4_dec_parents,
> +               .num_parents = ARRAY_SIZE(s4_dec_parents),
>                  .flags = 0,
>          },
>   };
> @@ -1612,7 +1608,7 @@ static struct clk_regmap s4_hevcf_p0_div = {
>                  .name = "hevcf_p0_div",
>                  .ops = &clk_regmap_divider_ops,
>                  .parent_hws = (const struct clk_hw *[]) {
> -                       &s4_hevcf_p0_mux.hw
> +                       &s4_hevcf_p0_sel.hw
>                  },
>                  .num_parents = 1,
>                  .flags = CLK_SET_RATE_PARENT,
> @@ -1625,7 +1621,7 @@ static struct clk_regmap s4_hevcf_p0 = {
>                  .bit_idx = 8,
>          },
>          .hw.init = &(struct clk_init_data){
> -               .name = "hevcf_p0_gate",
> +               .name = "hevcf_p0",
>                  .ops = &clk_regmap_gate_ops,
>                  .parent_hws = (const struct clk_hw *[]) {
>                          &s4_hevcf_p0_div.hw
> @@ -1635,7 +1631,7 @@ static struct clk_regmap s4_hevcf_p0 = {
>          },
>   };
>
> -static struct clk_regmap s4_hevcf_p1_mux = {
> +static struct clk_regmap s4_hevcf_p1_sel = {
>          .data = &(struct clk_regmap_mux_data){
>                  .offset = CLKCTRL_VDEC4_CLK_CTRL,
>                  .mask = 0x7,
> @@ -1643,10 +1639,10 @@ static struct clk_regmap s4_hevcf_p1_mux = {
>                  .flags = CLK_MUX_ROUND_CLOSEST,
>          },
>          .hw.init = &(struct clk_init_data) {
> -               .name = "hevcf_p1_mux",
> +               .name = "hevcf_p1_sel",
>                  .ops = &clk_regmap_mux_ops,
> -               .parent_data = s4_dec_parent_data,
> -               .num_parents = ARRAY_SIZE(s4_dec_parent_data),
> +               .parent_data = s4_dec_parents,
> +               .num_parents = ARRAY_SIZE(s4_dec_parents),
>                  .flags = 0,
>          },
>   };
> @@ -1662,7 +1658,7 @@ static struct clk_regmap s4_hevcf_p1_div = {
>                  .name = "hevcf_p1_div",
>                  .ops = &clk_regmap_divider_ops,
>                  .parent_hws = (const struct clk_hw *[]) {
> -                       &s4_hevcf_p1_mux.hw
> +                       &s4_hevcf_p1_sel.hw
>                  },
>                  .num_parents = 1,
>                  .flags = CLK_SET_RATE_PARENT,
> @@ -1685,28 +1681,26 @@ static struct clk_regmap s4_hevcf_p1 = {
>          },
>   };
>
> -static const struct clk_hw *s4_hevcf_mux_parent_hws[] = {
> -       &s4_hevcf_p0.hw,
> -       &s4_hevcf_p1.hw
> -};
> -
> -static struct clk_regmap s4_hevcf_mux = {
> +static struct clk_regmap s4_hevcf_sel = {
>          .data = &(struct clk_regmap_mux_data){
>                  .offset = CLKCTRL_VDEC4_CLK_CTRL,
>                  .mask = 0x1,
>                  .shift = 15,
>          },
>          .hw.init = &(struct clk_init_data) {
> -               .name = "hevcf",
> +               .name = "hevcf_sel",
>                  .ops = &clk_regmap_mux_ops,
> -               .parent_hws = s4_hevcf_mux_parent_hws,
> -               .num_parents = ARRAY_SIZE(s4_hevcf_mux_parent_hws),
> +               .parent_hws = (const struct clk_hw *[]) {
> +                       &s4_hevcf_p0.hw,
> +                       &s4_hevcf_p1.hw,
> +               },
> +               .num_parents = 2,
>                  .flags = CLK_SET_RATE_PARENT,
>          },
>   };
>
>   /* VPU Clock */
> -static const struct clk_parent_data s4_vpu_parent_data[] = {
> +static const struct clk_parent_data s4_vpu_parents[] = {
>          { .fw_name = "fclk_div3", },
>          { .fw_name = "fclk_div4", },
>          { .fw_name = "fclk_div5", },
> @@ -1726,8 +1720,8 @@ static struct clk_regmap s4_vpu_0_sel = {
>          .hw.init = &(struct clk_init_data){
>                  .name = "vpu_0_sel",
>                  .ops = &clk_regmap_mux_ops,
> -               .parent_data = s4_vpu_parent_data,
> -               .num_parents = ARRAY_SIZE(s4_vpu_parent_data),
> +               .parent_data = s4_vpu_parents,
> +               .num_parents = ARRAY_SIZE(s4_vpu_parents),
>                  .flags = 0,
>          },
>   };
> @@ -1770,8 +1764,8 @@ static struct clk_regmap s4_vpu_1_sel = {
>          .hw.init = &(struct clk_init_data){
>                  .name = "vpu_1_sel",
>                  .ops = &clk_regmap_mux_ops,
> -               .parent_data = s4_vpu_parent_data,
> -               .num_parents = ARRAY_SIZE(s4_vpu_parent_data),
> +               .parent_data = s4_vpu_parents,
> +               .num_parents = ARRAY_SIZE(s4_vpu_parents),
>                  .flags = 0,
>          },
>   };
> @@ -1823,24 +1817,24 @@ static struct clk_regmap s4_vpu = {
>          },
>   };
>
> -static const struct clk_parent_data vpu_clkb_tmp_parent_data[] = {
> +static const struct clk_parent_data vpu_clkb_tmp_parents[] = {
>          { .hw = &s4_vpu.hw },
>          { .fw_name = "fclk_div4", },
>          { .fw_name = "fclk_div5", },
>          { .fw_name = "fclk_div7", }
>   };
>
> -static struct clk_regmap s4_vpu_clkb_tmp_mux = {
> +static struct clk_regmap s4_vpu_clkb_tmp_sel = {
>          .data = &(struct clk_regmap_mux_data){
>                  .offset = CLKCTRL_VPU_CLKB_CTRL,
>                  .mask = 0x3,
>                  .shift = 20,
>          },
>          .hw.init = &(struct clk_init_data) {
> -               .name = "vpu_clkb_tmp_mux",
> +               .name = "vpu_clkb_tmp_sel",
>                  .ops = &clk_regmap_mux_ops,
> -               .parent_data = vpu_clkb_tmp_parent_data,
> -               .num_parents = ARRAY_SIZE(vpu_clkb_tmp_parent_data),
> +               .parent_data = vpu_clkb_tmp_parents,
> +               .num_parents = ARRAY_SIZE(vpu_clkb_tmp_parents),
>                  .flags = CLK_SET_RATE_PARENT,
>          },
>   };
> @@ -1855,7 +1849,7 @@ static struct clk_regmap s4_vpu_clkb_tmp_div = {
>                  .name = "vpu_clkb_tmp_div",
>                  .ops = &clk_regmap_divider_ops,
>                  .parent_hws = (const struct clk_hw *[]) {
> -                       &s4_vpu_clkb_tmp_mux.hw
> +                       &s4_vpu_clkb_tmp_sel.hw
>                  },
>                  .num_parents = 1,
>                  .flags = CLK_SET_RATE_PARENT,
> @@ -1911,7 +1905,7 @@ static struct clk_regmap s4_vpu_clkb = {
>          },
>   };
>
> -static const struct clk_parent_data s4_vpu_clkc_parent_data[] = {
> +static const struct clk_parent_data s4_vpu_clkc_parents[] = {
>          { .fw_name = "fclk_div4", },
>          { .fw_name = "fclk_div3", },
>          { .fw_name = "fclk_div5", },
> @@ -1922,17 +1916,17 @@ static const struct clk_parent_data s4_vpu_clkc_parent_data[] = {
>          { .fw_name = "gp0_pll", },
>   };
>
> -static struct clk_regmap s4_vpu_clkc_p0_mux  = {
> +static struct clk_regmap s4_vpu_clkc_p0_sel  = {


+static struct clk_regmap s4_vpu_clkc_0_sel
+static struct clk_regmap s4_vpu_clkc_0_div
.
.
.


>          .data = &(struct clk_regmap_mux_data){
>                  .offset = CLKCTRL_VPU_CLKC_CTRL,
>                  .mask = 0x7,
>                  .shift = 9,
>          },
>          .hw.init = &(struct clk_init_data) {
> -               .name = "vpu_clkc_p0_mux",
> +               .name = "vpu_clkc_p0_sel",
>                  .ops = &clk_regmap_mux_ops,
> -               .parent_data = s4_vpu_clkc_parent_data,
> -               .num_parents = ARRAY_SIZE(s4_vpu_clkc_parent_data),
> +               .parent_data = s4_vpu_clkc_parents,
> +               .num_parents = ARRAY_SIZE(s4_vpu_clkc_parents),
>                  .flags = 0,
>          },
>   };
> @@ -1947,7 +1941,7 @@ static struct clk_regmap s4_vpu_clkc_p0_div = {
>                  .name = "vpu_clkc_p0_div",
>                  .ops = &clk_regmap_divider_ops,
>                  .parent_hws = (const struct clk_hw *[]) {
> -                       &s4_vpu_clkc_p0_mux.hw
> +                       &s4_vpu_clkc_p0_sel.hw
>                  },
>                  .num_parents = 1,
>                  .flags = CLK_SET_RATE_PARENT,
> @@ -1970,17 +1964,17 @@ static struct clk_regmap s4_vpu_clkc_p0 = {
>          },
>   };
>
> -static struct clk_regmap s4_vpu_clkc_p1_mux = {
> +static struct clk_regmap s4_vpu_clkc_p1_sel = {
>          .data = &(struct clk_regmap_mux_data){
>                  .offset = CLKCTRL_VPU_CLKC_CTRL,
>                  .mask = 0x7,
>                  .shift = 25,
>          },
>          .hw.init = &(struct clk_init_data) {
> -               .name = "vpu_clkc_p1_mux",
> +               .name = "vpu_clkc_p1_sel",
>                  .ops = &clk_regmap_mux_ops,
> -               .parent_data = s4_vpu_clkc_parent_data,
> -               .num_parents = ARRAY_SIZE(s4_vpu_clkc_parent_data),
> +               .parent_data = s4_vpu_clkc_parents,
> +               .num_parents = ARRAY_SIZE(s4_vpu_clkc_parents),
>                  .flags = 0,
>          },
>   };
> @@ -1995,7 +1989,7 @@ static struct clk_regmap s4_vpu_clkc_p1_div = {
>                  .name = "vpu_clkc_p1_div",
>                  .ops = &clk_regmap_divider_ops,
>                  .parent_hws = (const struct clk_hw *[]) {
> -                       &s4_vpu_clkc_p1_mux.hw
> +                       &s4_vpu_clkc_p1_sel.hw
>                  },
>                  .num_parents = 1,
>                  .flags = CLK_SET_RATE_PARENT,
> @@ -2018,28 +2012,26 @@ static struct clk_regmap s4_vpu_clkc_p1 = {
>          },
>   };
>
> -static const struct clk_hw *s4_vpu_mux_parent_hws[] = {
> -       &s4_vpu_clkc_p0.hw,
> -       &s4_vpu_clkc_p1.hw
> -};
> -
> -static struct clk_regmap s4_vpu_clkc_mux = {
> +static struct clk_regmap s4_vpu_clkc_sel = {
>          .data = &(struct clk_regmap_mux_data){
>                  .offset = CLKCTRL_VPU_CLKC_CTRL,
>                  .mask = 0x1,
>                  .shift = 31,
>          },
>          .hw.init = &(struct clk_init_data) {
> -               .name = "vpu_clkc_mux",
> +               .name = "vpu_clkc_sel",
>                  .ops = &clk_regmap_mux_ops,
> -               .parent_hws = s4_vpu_mux_parent_hws,
> -               .num_parents = ARRAY_SIZE(s4_vpu_mux_parent_hws),
> +               .parent_hws = (const struct clk_hw *[]) {
> +                       &s4_vpu_clkc_p0.hw,
> +                       &s4_vpu_clkc_p1.hw,
> +               },
> +               .num_parents = 2,
>                  .flags = CLK_SET_RATE_PARENT,
>          },
>   };


[...]


>   MODULE_DESCRIPTION("Amlogic S4 Peripherals 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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic