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

[Jerome Brunet <jbrunet@baylibre.com>]

>>>>
>>>> -#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.

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