Re: [PATCH 05/26] clk: amlogic: c3-peripherals: naming consistency alignment

[Jerome Brunet <jbrunet@baylibre.com>]

On Thu 03 Jul 2025 at 10:51, Chuan Liu <chuan.liu@amlogic.com> wrote:

> 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/c3-peripherals.c | 1261 ++++++++++++++++++------------------
>>   1 file changed, 630 insertions(+), 631 deletions(-)
>>
>> diff --git a/drivers/clk/meson/c3-peripherals.c b/drivers/clk/meson/c3-peripherals.c
>> index a25e7d5dc6691d2d4a852e3d3da2b36f251cc141..a09cb1435ab108b2dcc209c6557bcd1988c4ba1a 100644
>> --- a/drivers/clk/meson/c3-peripherals.c
>> +++ b/drivers/clk/meson/c3-peripherals.c
>> @@ -48,7 +48,7 @@
>>   #define SPIFC_CLK_CTRL                         0x1a0
>>   #define NNA_CLK_CTRL                           0x220
>>
>> -static struct clk_regmap rtc_xtal_clkin = {
>> +static struct clk_regmap c3_rtc_xtal_clkin = {
>>          .data = &(struct clk_regmap_gate_data) {
>>                  .offset = RTC_BY_OSCIN_CTRL0,
>>                  .bit_idx = 31,
>> @@ -63,12 +63,12 @@ static struct clk_regmap rtc_xtal_clkin = {
>>          },
>>   };
>>
>> -static const struct meson_clk_dualdiv_param rtc_32k_div_table[] = {
>> +static const struct meson_clk_dualdiv_param c3_rtc_32k_div_table[] = {
>>          { 733, 732, 8, 11, 1 },
>>          { /* sentinel */ }
>>   };
>>
>> -static struct clk_regmap rtc_32k_div = {
>> +static struct clk_regmap c3_rtc_32k_div = {
>>          .data = &(struct meson_clk_dualdiv_data) {
>>                  .n1 = {
>>                          .reg_off = RTC_BY_OSCIN_CTRL0,
>> @@ -95,39 +95,39 @@ static struct clk_regmap rtc_32k_div = {
>>                          .shift   = 28,
>>                          .width   = 1,
>>                  },
>> -               .table = rtc_32k_div_table,
>> +               .table = c3_rtc_32k_div_table,
>>          },
>>          .hw.init = &(struct clk_init_data) {
>>                  .name = "rtc_32k_div",
>>                  .ops = &meson_clk_dualdiv_ops,
>>                  .parent_hws = (const struct clk_hw *[]) {
>> -                       &rtc_xtal_clkin.hw
>> +                       &c3_rtc_xtal_clkin.hw
>>                  },
>>                  .num_parents = 1,
>>          },
>>   };
>>
>> -static const struct clk_parent_data rtc_32k_mux_parent_data[] = {
>> -       { .hw = &rtc_32k_div.hw },
>> -       { .hw = &rtc_xtal_clkin.hw }
>> +static const struct clk_parent_data c3_rtc_32k_parents[] = {
>> +       { .hw = &c3_rtc_32k_div.hw },
>> +       { .hw = &c3_rtc_xtal_clkin.hw }
>>   };
>>
>> -static struct clk_regmap rtc_32k_mux = {
>> +static struct clk_regmap c3_rtc_32k_sel = {
>>          .data = &(struct clk_regmap_mux_data) {
>>                  .offset = RTC_BY_OSCIN_CTRL1,
>>                  .mask = 0x1,
>>                  .shift = 24,
>>          },
>>          .hw.init = &(struct clk_init_data) {
>> -               .name = "rtc_32k_mux",
>> +               .name = "rtc_32k_sel",
>>                  .ops = &clk_regmap_mux_ops,
>> -               .parent_data = rtc_32k_mux_parent_data,
>> -               .num_parents = ARRAY_SIZE(rtc_32k_mux_parent_data),
>> +               .parent_data = c3_rtc_32k_parents,
>> +               .num_parents = ARRAY_SIZE(c3_rtc_32k_parents),
>>                  .flags = CLK_SET_RATE_PARENT,
>>          },
>>   };
>>
>> -static struct clk_regmap rtc_32k = {
>> +static struct clk_regmap c3_rtc_32k = {
>>          .data = &(struct clk_regmap_gate_data) {
>>                  .offset = RTC_BY_OSCIN_CTRL0,
>>                  .bit_idx = 30,
>> @@ -136,20 +136,20 @@ static struct clk_regmap rtc_32k = {
>>                  .name = "rtc_32k",
>>                  .ops = &clk_regmap_gate_ops,
>>                  .parent_hws = (const struct clk_hw *[]) {
>> -                       &rtc_32k_mux.hw
>> +                       &c3_rtc_32k_sel.hw
>>                  },
>>                  .num_parents = 1,
>>                  .flags = CLK_SET_RATE_PARENT,
>>          },
>>   };
>>
>> -static const struct clk_parent_data rtc_clk_mux_parent_data[] = {
>> +static const struct clk_parent_data c3_rtc_clk_parents[] = {
>>          { .fw_name = "oscin" },
>> -       { .hw = &rtc_32k.hw },
>> +       { .hw = &c3_rtc_32k.hw },
>>          { .fw_name = "pad_osc" }
>>   };
>>
>> -static struct clk_regmap rtc_clk = {
>> +static struct clk_regmap c3_rtc_clk = {
>>          .data = &(struct clk_regmap_mux_data) {
>>                  .offset = RTC_CTRL,
>>                  .mask = 0x3,
>> @@ -158,62 +158,62 @@ static struct clk_regmap rtc_clk = {
>>          .hw.init = &(struct clk_init_data) {
>>                  .name = "rtc_clk",
>>                  .ops = &clk_regmap_mux_ops,
>> -               .parent_data = rtc_clk_mux_parent_data,
>> -               .num_parents = ARRAY_SIZE(rtc_clk_mux_parent_data),
>> +               .parent_data = c3_rtc_clk_parents,
>> +               .num_parents = ARRAY_SIZE(c3_rtc_clk_parents),
>>                  .flags = CLK_SET_RATE_PARENT,
>>          },
>>   };
>>
>> -#define C3_CLK_GATE(_name, _reg, _bit, _fw_name, _ops, _flags)         \
>> -struct clk_regmap _name = {                                            \
>> +#define C3_PCLK(_name, _reg, _bit, _fw_name, _ops, _flags)             \
>> +struct clk_regmap c3_##_name = {                                       \
>>          .data = &(struct clk_regmap_gate_data){                         \
>>                  .offset = (_reg),                                       \
>>                  .bit_idx = (_bit),                                      \
>>          },                                                              \
>>          .hw.init = &(struct clk_init_data) {                            \
>> -               .name = #_name,                                         \
>> +               .name = "c3_" #_name,                                   \
>
>
> Prefixing variable names with 'SoC' is understandable (to avoid duplicate
> definitions and facilitate variable searching), but is it necessary to add
> 'SoC' prefixes to clock names?

This is part of the description but I'll ellaborate.

Some controllers do so, some do not. This is a typical pointless
difference that make code sharing difficult and lead to the duplication
I'm addressing now.

Both with and without are fine but picking one a sticking to it helps a
lot. I would have preferred to drop the prefix from the pclk clock
names, same as the other clock, but:
* It would have changed more clock names and I prefer to minimize those
changes
* It would have caused several name clashes with other clocks.

so prefix it is for the peripheral clock.

In the end, what matters is consistency.

>
>
>>                  .ops = _ops,                                            \
>>                  .parent_data = &(const struct clk_parent_data) {        \
>> -                       .fw_name = #_fw_name,                           \
>> +                       .fw_name = (_fw_name),                          \
>>                  },                                                      \
>>                  .num_parents = 1,                                       \
>>                  .flags = (_flags),                                      \
>>          },                                                              \
>>   }
>
>
> [...]

-- 
Jerome