Re: [RFC PATCH v1 01/14] clk: thead: Refactor TH1520 clock driver to share common code

[Michal Wilczynski <m.wilczynski@samsung.com>]

On 12/3/24 20:56, Stephen Boyd wrote:
> Quoting Michal Wilczynski (2024-12-03 05:41:24)
>> diff --git a/drivers/clk/thead/Makefile b/drivers/clk/thead/Makefile
>> index 7ee0bec1f251..d7cf88390b69 100644
>> --- a/drivers/clk/thead/Makefile
>> +++ b/drivers/clk/thead/Makefile
>> @@ -1,2 +1,2 @@
>>  # SPDX-License-Identifier: GPL-2.0
>> -obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520-ap.o
>> +obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520.o clk-th1520-ap.o
> 
> Can the -ap driver be extended instead? Or are the clks in a different
> IO region?

The Video Output (VO) clocks reside in a different address space as
defined in the T-HEAD manual 4.4.1 [1]. Therefore, creating a separate
driver made sense to maintain clarity and adhere to the existing
convention of having one driver per subsystem, similar to the
AP-specific driver.

[1] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf
> 
>> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
>> index 17e32ae08720..a6015805b859 100644
>> --- a/drivers/clk/thead/clk-th1520-ap.c
>> +++ b/drivers/clk/thead/clk-th1520-ap.c
>> @@ -5,297 +5,9 @@
>>   *  Authors: Yangtao Li <frank.li@vivo.com>
>>   */
>>  
>> -#include <dt-bindings/clock/thead,th1520-clk-ap.h>
> 
> Presumably this should stay here.
> 
>> -#include <linux/bitfield.h>
>> -#include <linux/clk-provider.h>
>> -#include <linux/device.h>
>> -#include <linux/module.h>
>> -#include <linux/platform_device.h>
>> -#include <linux/regmap.h>
> 
> These should all stay here as well.
> 
>> -
>> -#define TH1520_PLL_POSTDIV2    GENMASK(26, 24)
>> -#define TH1520_PLL_POSTDIV1    GENMASK(22, 20)
>> -#define TH1520_PLL_FBDIV       GENMASK(19, 8)
>> -#define TH1520_PLL_REFDIV      GENMASK(5, 0)
>> -#define TH1520_PLL_BYPASS      BIT(30)
>> -#define TH1520_PLL_DSMPD       BIT(24)
>> -#define TH1520_PLL_FRAC                GENMASK(23, 0)
>> -#define TH1520_PLL_FRAC_BITS    24
>> -
>> -struct ccu_internal {
>> -       u8      shift;
>> -       u8      width;
>> -};
>> -
>> -struct ccu_div_internal {
>> -       u8      shift;
>> -       u8      width;
>> -       u32     flags;
>> -};
>> -
>> -struct ccu_common {
>> -       int             clkid;
>> -       struct regmap   *map;
>> -       u16             cfg0;
>> -       u16             cfg1;
>> -       struct clk_hw   hw;
>> -};
>> -
>> -struct ccu_mux {
>> -       struct ccu_internal     mux;
>> -       struct ccu_common       common;
>> -};
>> -
>> -struct ccu_gate {
>> -       u32                     enable;
>> -       struct ccu_common       common;
>> -};
>> -
>> -struct ccu_div {
>> -       u32                     enable;
>> -       struct ccu_div_internal div;
>> -       struct ccu_internal     mux;
>> -       struct ccu_common       common;
>> -};
>> -
>> -struct ccu_pll {
>> -       struct ccu_common       common;
>> -};
>> -
>> -#define TH_CCU_ARG(_shift, _width)                                     \
>> -       {                                                               \
>> -               .shift  = _shift,                                       \
>> -               .width  = _width,                                       \
>> -       }
>> -
>> -#define TH_CCU_DIV_FLAGS(_shift, _width, _flags)                       \
>> -       {                                                               \
>> -               .shift  = _shift,                                       \
>> -               .width  = _width,                                       \
>> -               .flags  = _flags,                                       \
>> -       }
>> -
>> -#define CCU_GATE(_clkid, _struct, _name, _parent, _reg, _gate, _flags) \
>> -       struct ccu_gate _struct = {                                     \
>> -               .enable = _gate,                                        \
>> -               .common = {                                             \
>> -                       .clkid          = _clkid,                       \
>> -                       .cfg0           = _reg,                         \
>> -                       .hw.init        = CLK_HW_INIT_PARENTS_DATA(     \
>> -                                               _name,                  \
>> -                                               _parent,                \
>> -                                               &clk_gate_ops,          \
>> -                                               _flags),                \
>> -               }                                                       \
>> -       }
>> -
>> -static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
>> -{
>> -       return container_of(hw, struct ccu_common, hw);
>> -}
>> -
>> -static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
>> -{
>> -       struct ccu_common *common = hw_to_ccu_common(hw);
>> -
>> -       return container_of(common, struct ccu_mux, common);
>> -}
>> -
>> -static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
>> -{
>> -       struct ccu_common *common = hw_to_ccu_common(hw);
>> +#include "clk-th1520.h"
>>  
>> -       return container_of(common, struct ccu_pll, common);
>> -}
>> -
>> -static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
>> -{
>> -       struct ccu_common *common = hw_to_ccu_common(hw);
>> -
>> -       return container_of(common, struct ccu_div, common);
>> -}
>> -
>> -static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
>> -{
>> -       struct ccu_common *common = hw_to_ccu_common(hw);
>> -
>> -       return container_of(common, struct ccu_gate, common);
>> -}
>> -
>> -static u8 ccu_get_parent_helper(struct ccu_common *common,
>> -                               struct ccu_internal *mux)
>> -{
>> -       unsigned int val;
>> -       u8 parent;
>> -
>> -       regmap_read(common->map, common->cfg0, &val);
>> -       parent = val >> mux->shift;
>> -       parent &= GENMASK(mux->width - 1, 0);
>> -
>> -       return parent;
>> -}
>> -
>> -static int ccu_set_parent_helper(struct ccu_common *common,
>> -                                struct ccu_internal *mux,
>> -                                u8 index)
>> -{
>> -       return regmap_update_bits(common->map, common->cfg0,
>> -                       GENMASK(mux->width - 1, 0) << mux->shift,
>> -                       index << mux->shift);
>> -}
>> -
>> -static void ccu_disable_helper(struct ccu_common *common, u32 gate)
>> -{
>> -       if (!gate)
>> -               return;
>> -       regmap_update_bits(common->map, common->cfg0,
>> -                          gate, ~gate);
>> -}
>> -
>> -static int ccu_enable_helper(struct ccu_common *common, u32 gate)
>> -{
>> -       unsigned int val;
>> -       int ret;
>> -
>> -       if (!gate)
>> -               return 0;
>> -
>> -       ret = regmap_update_bits(common->map, common->cfg0, gate, gate);
>> -       regmap_read(common->map, common->cfg0, &val);
>> -       return ret;
>> -}
>> -
>> -static int ccu_is_enabled_helper(struct ccu_common *common, u32 gate)
>> -{
>> -       unsigned int val;
>> -
>> -       if (!gate)
>> -               return true;
>> -
>> -       regmap_read(common->map, common->cfg0, &val);
>> -       return val & gate;
>> -}
>> -
>> -static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
>> -                                        unsigned long parent_rate)
>> -{
>> -       struct ccu_div *cd = hw_to_ccu_div(hw);
>> -       unsigned long rate;
>> -       unsigned int val;
>> -
>> -       regmap_read(cd->common.map, cd->common.cfg0, &val);
>> -       val = val >> cd->div.shift;
>> -       val &= GENMASK(cd->div.width - 1, 0);
>> -       rate = divider_recalc_rate(hw, parent_rate, val, NULL,
>> -                                  cd->div.flags, cd->div.width);
>> -
>> -       return rate;
>> -}
>> -
>> -static u8 ccu_div_get_parent(struct clk_hw *hw)
>> -{
>> -       struct ccu_div *cd = hw_to_ccu_div(hw);
>> -
>> -       return ccu_get_parent_helper(&cd->common, &cd->mux);
>> -}
>> -
>> -static int ccu_div_set_parent(struct clk_hw *hw, u8 index)
>> -{
>> -       struct ccu_div *cd = hw_to_ccu_div(hw);
>> -
>> -       return ccu_set_parent_helper(&cd->common, &cd->mux, index);
>> -}
>> -
>> -static void ccu_div_disable(struct clk_hw *hw)
>> -{
>> -       struct ccu_div *cd = hw_to_ccu_div(hw);
>> -
>> -       ccu_disable_helper(&cd->common, cd->enable);
>> -}
>> -
>> -static int ccu_div_enable(struct clk_hw *hw)
>> -{
>> -       struct ccu_div *cd = hw_to_ccu_div(hw);
>> -
>> -       return ccu_enable_helper(&cd->common, cd->enable);
>> -}
>> -
>> -static int ccu_div_is_enabled(struct clk_hw *hw)
>> -{
>> -       struct ccu_div *cd = hw_to_ccu_div(hw);
>> -
>> -       return ccu_is_enabled_helper(&cd->common, cd->enable);
>> -}
>> -
>> -static const struct clk_ops ccu_div_ops = {
>> -       .disable        = ccu_div_disable,
>> -       .enable         = ccu_div_enable,
>> -       .is_enabled     = ccu_div_is_enabled,
>> -       .get_parent     = ccu_div_get_parent,
>> -       .set_parent     = ccu_div_set_parent,
>> -       .recalc_rate    = ccu_div_recalc_rate,
>> -       .determine_rate = clk_hw_determine_rate_no_reparent,
>> -};
>> -
>> -static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
>> -                                               unsigned long parent_rate)
>> -{
>> -       struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> -       unsigned long div, mul, frac;
>> -       unsigned int cfg0, cfg1;
>> -       u64 rate = parent_rate;
>> -
>> -       regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
>> -       regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
>> -
>> -       mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
>> -       div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
>> -       if (!(cfg1 & TH1520_PLL_DSMPD)) {
>> -               mul <<= TH1520_PLL_FRAC_BITS;
>> -               frac = FIELD_GET(TH1520_PLL_FRAC, cfg1);
>> -               mul += frac;
>> -               div <<= TH1520_PLL_FRAC_BITS;
>> -       }
>> -       rate = parent_rate * mul;
>> -       rate = rate / div;
>> -       return rate;
>> -}
>> -
>> -static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
>> -                                                   unsigned long parent_rate)
>> -{
>> -       struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> -       unsigned long div, rate = parent_rate;
>> -       unsigned int cfg0, cfg1;
>> -
>> -       regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
>> -       regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
>> -
>> -       if (cfg1 & TH1520_PLL_BYPASS)
>> -               return rate;
>> -
>> -       div = FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *
>> -             FIELD_GET(TH1520_PLL_POSTDIV2, cfg0);
>> -
>> -       rate = rate / div;
>> -
>> -       return rate;
>> -}
>> -
>> -static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
>> -                                        unsigned long parent_rate)
>> -{
>> -       unsigned long rate = parent_rate;
>> -
>> -       rate = th1520_pll_vco_recalc_rate(hw, rate);
>> -       rate = th1520_pll_postdiv_recalc_rate(hw, rate);
>> -
>> -       return rate;
>> -}
>> -
>> -static const struct clk_ops clk_pll_ops = {
>> -       .recalc_rate    = ccu_pll_recalc_rate,
>> -};
>> +#define NR_CLKS        (CLK_UART_SCLK + 1)
>>  
>>  static const struct clk_parent_data osc_24m_clk[] = {
>>         { .index = 0 }
>> @@ -956,15 +668,6 @@ static struct ccu_common *th1520_gate_clks[] = {
>>         &sram3_clk.common,
>>  };
>>  
>> -#define NR_CLKS        (CLK_UART_SCLK + 1)
> 
> Why did this move?
> 
>> -
>> -static const struct regmap_config th1520_clk_regmap_config = {
>> -       .reg_bits = 32,
>> -       .val_bits = 32,
>> -       .reg_stride = 4,
>> -       .fast_io = true,
>> -};
>> -
>>  static int th1520_clk_probe(struct platform_device *pdev)
>>  {
>>         struct device *dev = &pdev->dev;
>> diff --git a/drivers/clk/thead/clk-th1520.c b/drivers/clk/thead/clk-th1520.c
>> new file mode 100644
>> index 000000000000..e2bfe56de9af
>> --- /dev/null
>> +++ b/drivers/clk/thead/clk-th1520.c
>> @@ -0,0 +1,188 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
>> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
>> + *  Authors: Yangtao Li <frank.li@vivo.com>
>> + */
>> +
>> +#include "clk-th1520.h"
> 
> Explicitly include linux headers here, don't just put them all into a
> header file. It helps us easily see what C files are using different
> parts of the kernel infrastructure.
> 
>> +
>> +static u8 ccu_get_parent_helper(struct ccu_common *common,
>> +                               struct ccu_internal *mux)
>> +{
>> +       unsigned int val;
>> +       u8 parent;
>> +
>> +       regmap_read(common->map, common->cfg0, &val);
>> +       parent = val >> mux->shift;
>> +       parent &= GENMASK(mux->width - 1, 0);
>> +
>> +       return parent;
>> +}
>> +
>> +static int ccu_set_parent_helper(struct ccu_common *common,
>> +                                struct ccu_internal *mux, u8 index)
>> +{
>> +       return regmap_update_bits(common->map, common->cfg0,
>> +                                 GENMASK(mux->width - 1, 0) << mux->shift,
>> +                                 index << mux->shift);
>> +}
>> +
>> +static void ccu_disable_helper(struct ccu_common *common, u32 gate)
>> +{
>> +       if (!gate)
>> +               return;
>> +       regmap_update_bits(common->map, common->cfg0, gate, ~gate);
>> +}
>> +
>> +static int ccu_enable_helper(struct ccu_common *common, u32 gate)
>> +{
>> +       unsigned int val;
>> +       int ret;
>> +
>> +       if (!gate)
>> +               return 0;
>> +
>> +       ret = regmap_update_bits(common->map, common->cfg0, gate, gate);
>> +       regmap_read(common->map, common->cfg0, &val);
>> +       return ret;
>> +}
>> +
>> +static int ccu_is_enabled_helper(struct ccu_common *common, u32 gate)
>> +{
>> +       unsigned int val;
>> +
>> +       if (!gate)
>> +               return true;
>> +
>> +       regmap_read(common->map, common->cfg0, &val);
>> +       return val & gate;
>> +}
>> +
>> +static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
>> +                                        unsigned long parent_rate)
>> +{
>> +       struct ccu_div *cd = hw_to_ccu_div(hw);
>> +       unsigned long rate;
>> +       unsigned int val;
>> +
>> +       regmap_read(cd->common.map, cd->common.cfg0, &val);
>> +       val = val >> cd->div.shift;
>> +       val &= GENMASK(cd->div.width - 1, 0);
>> +       rate = divider_recalc_rate(hw, parent_rate, val, NULL, cd->div.flags,
>> +                                  cd->div.width);
>> +
>> +       return rate;
>> +}
>> +
>> +static u8 ccu_div_get_parent(struct clk_hw *hw)
>> +{
>> +       struct ccu_div *cd = hw_to_ccu_div(hw);
>> +
>> +       return ccu_get_parent_helper(&cd->common, &cd->mux);
>> +}
>> +
>> +static int ccu_div_set_parent(struct clk_hw *hw, u8 index)
>> +{
>> +       struct ccu_div *cd = hw_to_ccu_div(hw);
>> +
>> +       return ccu_set_parent_helper(&cd->common, &cd->mux, index);
>> +}
>> +
>> +static void ccu_div_disable(struct clk_hw *hw)
>> +{
>> +       struct ccu_div *cd = hw_to_ccu_div(hw);
>> +
>> +       ccu_disable_helper(&cd->common, cd->enable);
>> +}
>> +
>> +static int ccu_div_enable(struct clk_hw *hw)
>> +{
>> +       struct ccu_div *cd = hw_to_ccu_div(hw);
>> +
>> +       return ccu_enable_helper(&cd->common, cd->enable);
>> +}
>> +
>> +static int ccu_div_is_enabled(struct clk_hw *hw)
>> +{
>> +       struct ccu_div *cd = hw_to_ccu_div(hw);
>> +
>> +       return ccu_is_enabled_helper(&cd->common, cd->enable);
>> +}
>> +
>> +const struct clk_ops ccu_div_ops = {
>> +       .disable = ccu_div_disable,
>> +       .enable = ccu_div_enable,
>> +       .is_enabled = ccu_div_is_enabled,
>> +       .get_parent = ccu_div_get_parent,
>> +       .set_parent = ccu_div_set_parent,
>> +       .recalc_rate = ccu_div_recalc_rate,
>> +       .determine_rate = clk_hw_determine_rate_no_reparent,
>> +};
>> +
>> +static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
>> +                                               unsigned long parent_rate)
>> +{
>> +       struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> +       unsigned long div, mul, frac;
>> +       unsigned int cfg0, cfg1;
>> +       u64 rate = parent_rate;
>> +
>> +       regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
>> +       regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
>> +
>> +       mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
>> +       div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
>> +       if (!(cfg1 & TH1520_PLL_DSMPD)) {
>> +               mul <<= TH1520_PLL_FRAC_BITS;
>> +               frac = FIELD_GET(TH1520_PLL_FRAC, cfg1);
>> +               mul += frac;
>> +               div <<= TH1520_PLL_FRAC_BITS;
>> +       }
>> +       rate = parent_rate * mul;
>> +       rate = rate / div;
>> +       return rate;
>> +}
>> +
>> +static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
>> +                                                   unsigned long parent_rate)
>> +{
>> +       struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> +       unsigned long div, rate = parent_rate;
>> +       unsigned int cfg0, cfg1;
>> +
>> +       regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
>> +       regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
>> +
>> +       if (cfg1 & TH1520_PLL_BYPASS)
>> +               return rate;
>> +
>> +       div = FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *
>> +             FIELD_GET(TH1520_PLL_POSTDIV2, cfg0);
>> +
>> +       rate = rate / div;
>> +
>> +       return rate;
>> +}
>> +
>> +static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
>> +                                        unsigned long parent_rate)
>> +{
>> +       unsigned long rate = parent_rate;
>> +
>> +       rate = th1520_pll_vco_recalc_rate(hw, rate);
>> +       rate = th1520_pll_postdiv_recalc_rate(hw, rate);
>> +
>> +       return rate;
>> +}
>> +
>> +const struct clk_ops clk_pll_ops = {
>> +       .recalc_rate = ccu_pll_recalc_rate,
>> +};
>> +
>> +const struct regmap_config th1520_clk_regmap_config = {
> 
> I don't get why this is moved.
> 
>> +       .reg_bits = 32,
>> +       .val_bits = 32,
>> +       .reg_stride = 4,
>> +       .fast_io = true,
>> +};
>> diff --git a/drivers/clk/thead/clk-th1520.h b/drivers/clk/thead/clk-th1520.h
>> new file mode 100644
>> index 000000000000..285d41e65008
>> --- /dev/null
>> +++ b/drivers/clk/thead/clk-th1520.h
>> @@ -0,0 +1,134 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
>> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
>> + *  Authors: Yangtao Li <frank.li@vivo.com>
>> + *
>> + * clk-th1520.h - Common definitions for T-HEAD TH1520 Clock Drivers
>> + */
>> +
>> +#ifndef CLK_TH1520_H
>> +#define CLK_TH1520_H
>> +
>> +#include <dt-bindings/clock/thead,th1520-clk-ap.h>
> 
> dt-bindings comes after linux includes, but this shouldn't be here
> anyway.
> 
>> +#include <linux/bitfield.h>
>> +#include <linux/clk-provider.h>
> 
> Seems we have to have this one for clk_hw.
> 
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
> 
> I don't see these includes used here, so remove them.
> 
>> +#include <linux/regmap.h>
> 
> Forward declare regmap and drop the include
> 
> struct regmap;
> 
>> +
>> +#define TH1520_PLL_POSTDIV2    GENMASK(26, 24)
>> +#define TH1520_PLL_POSTDIV1    GENMASK(22, 20)
>> +#define TH1520_PLL_FBDIV       GENMASK(19, 8)
>> +#define TH1520_PLL_REFDIV      GENMASK(5, 0)
>> +#define TH1520_PLL_BYPASS      BIT(30)
>> +#define TH1520_PLL_DSMPD       BIT(24)
>> +#define TH1520_PLL_FRAC                GENMASK(23, 0)
>> +#define TH1520_PLL_FRAC_BITS    24
> 
> Are these going to be used in multiple drivers?
> 
>> +
>> +struct ccu_internal {
>> +       u8      shift;
>> +       u8      width;
>> +};
>> +
>> +struct ccu_div_internal {
>> +       u8      shift;
>> +       u8      width;
>> +       u32     flags;
>> +};
>> +
>> +struct ccu_common {
>> +       int             clkid;
>> +       struct regmap   *map;
>> +       u16             cfg0;
>> +       u16             cfg1;
>> +       struct clk_hw   hw;
>> +};
>> +
>> +struct ccu_mux {
>> +       struct ccu_internal     mux;
>> +       struct ccu_common       common;
>> +};
>> +
>> +struct ccu_gate {
>> +       u32                     enable;
>> +       struct ccu_common       common;
>> +};
>> +
>> +struct ccu_div {
>> +       u32                     enable;
>> +       struct ccu_div_internal div;
>> +       struct ccu_internal     mux;
>> +       struct ccu_common       common;
>> +};
>> +
>> +struct ccu_pll {
>> +       struct ccu_common       common;
>> +};
>> +
>> +#define TH_CCU_ARG(_shift, _width)                                     \
>> +       {                                                               \
>> +               .shift  = _shift,                                       \
>> +               .width  = _width,                                       \
>> +       }
>> +
>> +#define TH_CCU_DIV_FLAGS(_shift, _width, _flags)                       \
>> +       {                                                               \
>> +               .shift  = _shift,                                       \
>> +               .width  = _width,                                       \
>> +               .flags  = _flags,                                       \
>> +       }
>> +
>> +#define CCU_GATE(_clkid, _struct, _name, _parent, _reg, _gate, _flags) \
>> +       struct ccu_gate _struct = {                                     \
>> +               .enable = _gate,                                        \
>> +               .common = {                                             \
>> +                       .clkid          = _clkid,                       \
>> +                       .cfg0           = _reg,                         \
>> +                       .hw.init        = CLK_HW_INIT_PARENTS_DATA(     \
>> +                                               _name,                  \
>> +                                               _parent,                \
>> +                                               &clk_gate_ops,          \
>> +                                               _flags),                \
>> +               }                                                       \
>> +       }
>> +
>> +static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
>> +{
>> +       return container_of(hw, struct ccu_common, hw);
>> +}
>> +
>> +static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
>> +{
>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>> +
>> +       return container_of(common, struct ccu_mux, common);
>> +}
>> +
>> +static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
>> +{
>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>> +
>> +       return container_of(common, struct ccu_pll, common);
>> +}
>> +
>> +static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
>> +{
>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>> +
>> +       return container_of(common, struct ccu_div, common);
>> +}
>> +
>> +static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw)
>> +{
>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>> +
>> +       return container_of(common, struct ccu_gate, common);
>> +}
>> +
>> +extern const struct clk_ops ccu_div_ops;
>> +extern const struct clk_ops clk_pll_ops;
>> +extern const struct regmap_config th1520_clk_regmap_config;
> 
> Why is the regmap config exported?

The regmap_config is exported to allow reuse across multiple drivers.
Initially, I passed the clock VOSYS address space using the reg property
and created the regmap from it, enabling other drivers to utilize the
same configuration. Later, I switched to a regmap-based syscon approach
but haven’t moved the regmap_config back to the AP driver.

Based on Krzysztof's feedback, using the reg property to pass
memory-mapped registers is preferred. If needed, I can create a separate
regmap_config for the VO subsystem instead of reusing the existing one.

I will also address the other points you mentioned in your review.

Thank you for your feedback.
Michał

>