[RFC PATCH v1 01/14] clk: thead: Refactor TH1520 clock driver to share common code
Michal Wilczynski
m.wilczynski at samsung.com
Wed Dec 4 05:54:02 PST 2024
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 at 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 at kernel.org>
>> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
>> + * Authors: Yangtao Li <frank.li at 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 at kernel.org>
>> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
>> + * Authors: Yangtao Li <frank.li at 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ł
>
More information about the linux-riscv
mailing list