Re: [RFC 3/9] clk: qcom: rcg: Add macros to collapse definition

[Dmitry Baryshkov]

On Tue, 26 Jul 2022 at 17:23, Abel Vesa <abel.vesa@xxxxxxxxxx> wrote:
>
> Add macros for a visually more compact rcg clocks definition,
> one for each type of rcg2 ops struct. These are only the ones
> used by gcc-sdm845 driver. More will be added later on.
>
> Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx>
> ---
>  drivers/clk/qcom/clk-rcg.h | 40 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
> index 012e745794fd..e856d472a14e 100644
> --- a/drivers/clk/qcom/clk-rcg.h
> +++ b/drivers/clk/qcom/clk-rcg.h
> @@ -180,6 +180,46 @@ struct clk_rcg_dfs_data {
>         struct clk_init_data *init;
>  };
>
> +#define __DEFINE_QCOM_CC_CLK_RCG2(_name, _cmd_rcgr, _mnd_width,                \
> +                               _hid_width, _parent_map, _freq_tbl,     \
> +                               _parent_data, _ops, _flags)             \
> +       static struct clk_init_data _name##_init = {                    \
> +               .name = #_name,                                         \
> +               .parent_data = _parent_data,                            \

I must admit, I do not see beauty here. I'd prefer to be able to use
either parent_data or parent_hws.

> +               .num_parents = ARRAY_SIZE(_parent_data),                \
> +               .ops = _ops,                                            \
> +       };                                                              \
> +                                                                       \
> +       static struct clk_rcg2 _name = {                                \
> +               .cmd_rcgr = _cmd_rcgr,                                  \
> +               .mnd_width = _mnd_width,                                \
> +               .hid_width = _hid_width,                                \
> +               .parent_map = _parent_map,                              \
> +               .freq_tbl = _freq_tbl,                                  \
> +               .clkr.hw.init = &_name##_init,                          \
> +       }
> +
> +#define DEFINE_QCOM_CC_CLK_RCG2(_name, _cmd_rcgr, _mnd_width,          \
> +                               _hid_width, _parent_map, _freq_tbl,     \
> +                               _parent_data)                           \
> +       __DEFINE_QCOM_CC_CLK_RCG2(_name, _cmd_rcgr, _mnd_width,         \
> +                               _hid_width, _parent_map, _freq_tbl,     \
> +                               _parent_data, &clk_rcg2_ops, 0)
> +
> +#define DEFINE_QCOM_CC_CLK_RCG2_SHARED(_name, _cmd_rcgr, _mnd_width,           \
> +                               _hid_width, _parent_map, _freq_tbl,     \
> +                               _parent_data)                           \
> +       __DEFINE_QCOM_CC_CLK_RCG2(_name, _cmd_rcgr, _mnd_width,         \
> +                               _hid_width, _parent_map, _freq_tbl,     \
> +                               _parent_data, &clk_rcg2_shared_ops, 0)
> +
> +#define DEFINE_QCOM_CC_CLK_RCG2_FLOOR(_name, _cmd_rcgr, _mnd_width,            \
> +                               _hid_width, _parent_map, _freq_tbl,     \
> +                               _parent_data)                           \
> +       __DEFINE_QCOM_CC_CLK_RCG2(_name, _cmd_rcgr, _mnd_width,         \
> +                               _hid_width, _parent_map, _freq_tbl,     \
> +                               _parent_data, &clk_rcg2_floor_ops, 0)

Too many variants. I'd suggest making the default one (&clk_rcg2_ops)
and the extensible (with variable ops).

> +
>  #define DEFINE_RCG_DFS(r) \
>         { .rcg = &r, .init = &r##_init }
>
> --
> 2.34.3
>


-- 
With best wishes
Dmitry