[PATCH 05/26] clk: amlogic: c3-peripherals: naming consistency alignment
Jerome Brunet
jbrunet at baylibre.com
Thu Jul 3 00:48:37 PDT 2025
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.
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
More information about the linux-amlogic
mailing list