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

[Chuan Liu]

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


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