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

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

On 7/3/2025 3:48 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> 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.


Yes, in fact most clock configurations are consistent across our SoCs. Over
the years, we've been continuously working to make our driver code more 
'common'
and efficient.


>
> 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:


I still prefer adding SoC prefixes to variable names but not to clock names.
clocks with the same name generally have similar functions across 
different chips.


> * It would have changed more clock names and I prefer to minimize those
> changes


Your recent patch series has already made significant changes, and this is
relatively a minor adjustment😉


> * 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