Re: [PATCH v3 2/2] clk: amlogic: Add A9 peripherals clock controller driver

[Jerome Brunet <jbrunet@baylibre.com>]

On mar. 16 juin 2026 at 14:12, Jian Hu <jian.hu@amlogic.com> wrote:


>>>
>>> If you think splitting it further into separate helper macros would improve
>>> readability.
>> One clock per macro please. Hidding 2 declaration is recipe for
>> disaster. For ex, here the first one is static, the 2nd is not
>
>
> I'll split it into separate helper macros so that each macro expands to a
> single clock definition.
>
> They are defined as follows: (Excluding struct clk_regmap)
>
> #define A9_VCLK_GATE(_name, _reg, _bit,  _parent)        \
>         .data = &(struct clk_regmap_gate_data){          \
>                 .offset = _reg,      \
>                 .bit_idx = _bit,       \
>         },       \
>         .hw.init = &(struct clk_init_data) {           \
>                 .name = #_name "_en",      \
>                 .ops = &clk_regmap_gate_ops,           \
>                 .parent_hws = (const struct clk_hw *[]) { _parent },    \
>                 .num_parents = 1,      \
>                 .flags = CLK_SET_RATE_PARENT,      \
>         },
>
> #define A9_VCLK_DIV(_name, _reg, _div)       \
>
>     ....
>
> static struct clk_regmap a9_vclk_div2_en = {
>         A9_VCLK_GATE(vclk_div2, VID_CLK_CTRL, 1, &a9_vclk.hw),
> };
>
>
> static struct clk_regmap a9_vclk_div2 = {
>         A9_VCLK_DIV(vclk_div2, VID_CLK_CTRL, 2),
> };
>
> My understanding is that you would prefer helper macros to cover only the
> repeated initializer fields,
> while keeping the actual clock declarations explicit.

I do not have a definitive preference over this but I do want things to be
consistent, at least within the driver, globaly whenever possible.

Look at the other macros you have already defined in your driver and do
the same thing, including the way you declare the variable. Apart from
this, it seems fine.

>
> If that's not what you had in mind, please let me know.
>>> I can do that as well.
>>>

-- 
Jerome