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

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

On 7/3/2025 5:02 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
>>>>> -#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.
>>
> No they are not consistent at all when it come to this
>
> Controller prefixing the pclks:
> * axg-ao
> * axg
> * g12-ao
> * g12
> * gxbb
> * s4-periphs
>
> Controllers not prefixing the pclks
> * gxbb-ao
> * a1-periphs
> * c3-periphs
> * meson8b
>
> I do not want to invent new names to avoid the names clashes if the
> prefixes are dropped. I tried that way and it was a mess.
>
> As noted in the description, clock names will not be prefixed with SoC
> name, *except* for the pclks for the historic reason explained above.


Understood, I'm no further questions. Thanks!


Reviewed-by: Chuan Liu <chuan.liu@amlogic.com>


>>> 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 is not a matter of preference.
>
>>
>>> * 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
> --
> Jerome