[PATCH 05/26] clk: amlogic: c3-peripherals: naming consistency alignment
Chuan Liu
chuan.liu at amlogic.com
Thu Jul 3 01:31:12 PDT 2025
On 7/3/2025 3:48 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Thu 03 Jul 2025 at 10:51, Chuan Liu <chuan.liu at 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 at 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
More information about the linux-amlogic
mailing list