Quoting Qin Jian (2021-12-02 23:34:23) > diff --git a/drivers/clk/clk-sp7021.c b/drivers/clk/clk-sp7021.c > new file mode 100644 > index 000000000..040578e82 > --- /dev/null > +++ b/drivers/clk/clk-sp7021.c > @@ -0,0 +1,738 @@ > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +/* > + * Copyright (C) Sunplus Technology Co., Ltd. > + * All rights reserved. > + */ > +//#define DEBUG > +#include <linux/module.h> It's not a module, don't include this. > +#include <linux/clk.h> Is this include used? > +#include <linux/clk-provider.h> > +#include <linux/clkdev.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/bitfield.h> > +#include <linux/slab.h> > +#include <linux/io.h> > +#include <linux/err.h> > +#include <linux/delay.h> Is this include used? > +#include <dt-bindings/clock/sp-sp7021.h> > + > +#ifndef clk_readl > +#define clk_readl readl > +#define clk_writel writel > +#endif Please remove these and use readl/writel directly. > + > +#define REG(i) (pll_regs + (i) * 4) > + > +#define PLLA_CTL REG(7) > +#define PLLE_CTL REG(12) > +#define PLLF_CTL REG(13) > +#define PLLTV_CTL REG(14) > +#define PLLSYS_CTL REG(26) > + > +#define EXTCLK "extclk" > + > +/* speical div_width values for PLLTV/PLLA */ > +#define DIV_TV 33 > +#define DIV_A 34 > + > +/* PLLTV parameters */ > +enum { > + SEL_FRA, > + SDM_MOD, > + PH_SEL, > + NFRA, > + DIVR, > + DIVN, > + DIVM, > + P_MAX > +}; > + > +#define MASK_SEL_FRA GENMASK(1, 1) > +#define MASK_SDM_MOD GENMASK(2, 2) > +#define MASK_PH_SEL GENMASK(4, 4) > +#define MASK_NFRA GENMASK(12, 6) > +#define MASK_DIVR GENMASK(8, 7) > +#define MASK_DIVN GENMASK(7, 0) > +#define MASK_DIVM GENMASK(14, 8) > + > +/* HIWORD_MASK FIELD_PREP */ > +#define HWM_FIELD_PREP(mask, value) \ > +({\ > + u32 m = mask; \ > + (m << 16) | FIELD_PREP(m, value); \ Please tab out the \ to a single column that's the same for all. > +}) > + > +struct sp_pll { > + struct clk_hw hw; > + void __iomem *reg; > + spinlock_t *lock; /* lock for reg */ > + int div_shift; > + int div_width; > + int pd_bit; /* power down bit idx */ > + int bp_bit; /* bypass bit idx */ > + unsigned long brate; /* base rate, TODO: replace brate with muldiv */ > + u32 p[P_MAX]; /* for hold PLLTV/PLLA parameters */ > +}; > + > +#define to_sp_pll(_hw) container_of(_hw, struct sp_pll, hw) > + > +#define clk_regs (moon_regs + 0x000) /* G0 ~ CLKEN */ > +#define pll_regs (moon_regs + 0x200) /* G4 ~ PLL */ > +static void __iomem *moon_regs; > + > +#define P_EXTCLK BIT(16) > +static const char * const parents[] = { Can we use clk_parent_data instead? > + "pllsys", > + "extclk", > +}; > + > +static const u32 gates[] = { > + CLK_SYSTEM, > + CLK_RTC, > + CLK_IOCTL, > + CLK_IOP, > + CLK_OTPRX, > + CLK_NOC, > + CLK_BR, > + CLK_RBUS_L00, > + CLK_SPIFL, > + CLK_SDCTRL0, > + CLK_PERI0 | P_EXTCLK, > + CLK_A926, > + CLK_UMCTL2, > + CLK_PERI1 | P_EXTCLK, > + > + CLK_DDR_PHY0, > + CLK_ACHIP, > + CLK_STC0, > + CLK_STC_AV0, > + CLK_STC_AV1, > + CLK_STC_AV2, > + CLK_UA0 | P_EXTCLK, > + CLK_UA1 | P_EXTCLK, > + CLK_UA2 | P_EXTCLK, > + CLK_UA3 | P_EXTCLK, > + CLK_UA4 | P_EXTCLK, > + CLK_HWUA | P_EXTCLK, > + CLK_DDC0, > + CLK_UADMA | P_EXTCLK, > + > + CLK_CBDMA0, > + CLK_CBDMA1, > + CLK_SPI_COMBO_0, > + CLK_SPI_COMBO_1, > + CLK_SPI_COMBO_2, > + CLK_SPI_COMBO_3, > + CLK_AUD, > + CLK_USBC0, > + CLK_USBC1, > + CLK_UPHY0, > + CLK_UPHY1, > + > + CLK_I2CM0, > + CLK_I2CM1, > + CLK_I2CM2, > + CLK_I2CM3, > + CLK_PMC, > + CLK_CARD_CTL0, > + CLK_CARD_CTL1, > + > + CLK_CARD_CTL4, > + CLK_BCH, > + CLK_DDFCH, > + CLK_CSIIW0, > + CLK_CSIIW1, > + CLK_MIPICSI0, > + CLK_MIPICSI1, > + > + CLK_HDMI_TX, > + CLK_VPOST, > + > + CLK_TGEN, > + CLK_DMIX, > + CLK_TCON, > + CLK_INTERRUPT, > + > + CLK_RGST, > + CLK_GPIO, > + CLK_RBUS_TOP, > + > + CLK_MAILBOX, > + CLK_SPIND, > + CLK_I2C2CBUS, > + CLK_SEC, > + CLK_GPOST0, > + CLK_DVE, > + > + CLK_OSD0, > + CLK_DISP_PWM, > + CLK_UADBG, > + CLK_DUMMY_MASTER, > + CLK_FIO_CTL, > + CLK_FPGA, > + CLK_L2SW, > + CLK_ICM, > + CLK_AXI_GLOBAL, > +}; > + > +static struct clk *clks[CLK_MAX]; > +static struct clk_onecell_data clk_data; > + > +static DEFINE_SPINLOCK(plla_lock); > +static DEFINE_SPINLOCK(plle_lock); > +static DEFINE_SPINLOCK(pllf_lock); > +static DEFINE_SPINLOCK(pllsys_lock); > +static DEFINE_SPINLOCK(plltv_lock); > + > +#define _M 1000000UL > +#define F_27M (27 * _M) > + > +/******************************************** PLL_TV *******************************************/ > + > +//#define PLLTV_STEP_DIR (?) /* Unit: HZ */ > + > +/* TODO: set proper FVCO range */ > +#define FVCO_MIN (100 * _M) > +#define FVCO_MAX (200 * _M) > + > +#define F_MIN (FVCO_MIN / 8) > +#define F_MAX (FVCO_MAX) > + > +static long plltv_integer_div(struct sp_pll *clk, unsigned long freq) > +{ > + /* valid m values: 27M must be divisible by m, 0 means end */ > + static const u32 m_table[] = { > + 1, 2, 3, 4, 5, 6, 8, 9, 10, 12, 15, 16, 18, 20, 24, 25, 27, 30, 32, 0 > + }; > + u32 m, n, r; > +#ifdef PLLTV_STEP_DIR > + u32 step = (PLLTV_STEP_DIR > 0) ? PLLTV_STEP_DIR : -PLLTV_STEP_DIR; > + int calc_times = 1000000 / step; > +#endif > + unsigned long fvco, nf; > + > + /* check freq */ > + if (freq < F_MIN) { > + pr_warn("%s: %s freq:%lu < F_MIN:%lu, round up\n", > + __func__, clk_hw_get_name(&clk->hw), freq, F_MIN); > + freq = F_MIN; > + } else if (freq > F_MAX) { > + pr_warn("%s: %s freq:%lu > F_MAX:%lu, round down\n", > + __func__, clk_hw_get_name(&clk->hw), freq, F_MAX); > + freq = F_MAX; > + } > + > +#ifdef PLLTV_STEP_DIR > + if ((freq % step) != 0) > + freq += step - (freq % step) + ((PLLTV_STEP_DIR > 0) ? 0 : PLLTV_STEP_DIR); > +#endif > + > +#ifdef PLLTV_STEP_DIR > +CALC: > + if (!calc_times) { > + pr_err("%s: %s freq:%lu out of recalc times\n", > + __func__, clk_hw_get_name(&clk->hw), freq); > + return -ETIMEOUT; > + } > +#endif > + > + /* DIVR 0~3 */ > + for (r = 0; r <= 3; r++) { > + fvco = freq << r; > + if (fvco <= FVCO_MAX) > + break; > + } > + > + /* DIVM */ > + for (m = 0; m_table[m]; m++) { > + nf = fvco * m_table[m]; > + n = nf / F_27M; > + if ((n * F_27M) == nf) > + break; > + } > + m = m_table[m]; > + > + if (!m) { > +#ifdef PLLTV_STEP_DIR > + freq += PLLTV_STEP_DIR; > + calc_times--; > + goto CALC; > +#else Please remove the debugging code. > + pr_err("%s: %s freq:%lu not found a valid setting\n", > + __func__, clk_hw_get_name(&clk->hw), freq); > + return -EINVAL; > +#endif > + } > + > + /* save parameters */ > + clk->p[SEL_FRA] = 0; > + clk->p[DIVR] = r; > + clk->p[DIVN] = n; > + clk->p[DIVM] = m; > + > + return freq; > +} > + > +/* parameters for PLLTV fractional divider */ > +static const u32 pt[][5] = { > + /* conventional fractional */ > + { > + 1, // factor > + 5, // 5 * p0 (nint) > + 1, // 1 * p0 > + F_27M, // F_27M / p0 > + 1, // p0 / p2 > + }, > + /* phase rotation */ > + { > + 10, // factor > + 54, // 5.4 * p0 (nint) > + 2, // 0.2 * p0 > + F_27M / 10, // F_27M / p0 > + 5, // p0 / p2 > + }, > +}; > + > +static const u32 mods[] = { 91, 55 }; /* SDM_MOD mod values */ > + > +static long plltv_fractional_div(struct sp_pll *clk, unsigned long freq) > +{ > + u32 m, r; > + u32 nint, nfra; > + u32 diff_min_quotient = 210000000, diff_min_remainder = 0; > + u32 diff_min_sign = 0; > + unsigned long fvco, nf, f, fout = 0; > + int sdm, ph; > + > + /* check freq */ > + if (freq < F_MIN) { > + pr_warn("%s: %s freq:%lu < F_MIN:%lu, round up\n", > + __func__, clk_hw_get_name(&clk->hw), freq, F_MIN); > + freq = F_MIN; > + } else if (freq > F_MAX) { > + pr_warn("%s: %s freq:%lu > F_MAX:%lu, round down\n", > + __func__, clk_hw_get_name(&clk->hw), freq, F_MAX); > + freq = F_MAX; > + } > + > + /* DIVR 0~3 */ > + for (r = 0; r <= 3; r++) { > + fvco = freq << r; > + if (fvco <= FVCO_MAX) > + break; > + } > + f = F_27M >> r; > + > + /* PH_SEL 1/0 */ > + for (ph = 1; ph >= 0; ph--) { > + const u32 *pp = pt[ph]; > + u32 ms = 1; > + > + /* SDM_MOD 0/1 */ > + for (sdm = 0; sdm <= 1; sdm++) { > + u32 mod = mods[sdm]; > + > + /* DIVM 1~32 */ > + for (m = ms; m <= 32; m++) { > + u32 diff_freq; > + u32 diff_freq_quotient = 0, diff_freq_remainder = 0; > + u32 diff_freq_sign = 0; /* 0:Positive number, 1:Negative number */ > + > + nf = fvco * m; > + nint = nf / pp[3]; > + > + if (nint < pp[1]) > + continue; > + if (nint > pp[1]) > + break; > + > + nfra = (((nf % pp[3]) * mod * pp[4]) + (F_27M / 2)) / F_27M; > + if (nfra) > + diff_freq = (f * (nint + pp[2]) / pp[0]) - > + (f * (mod - nfra) / mod / pp[4]); > + else > + diff_freq = (f * (nint) / pp[0]); > + > + diff_freq_quotient = diff_freq / m; > + diff_freq_remainder = ((diff_freq % m) * 1000) / m; > + > + if (freq > diff_freq_quotient) { > + diff_freq_quotient = freq - diff_freq_quotient - 1; > + diff_freq_remainder = 1000 - diff_freq_remainder; > + diff_freq_sign = 1; > + } else { > + diff_freq_quotient = diff_freq_quotient - freq; > + diff_freq_sign = 0; > + } > + > + if (diff_min_quotient > diff_freq_quotient || > + (diff_min_quotient == diff_freq_quotient && > + diff_min_remainder > diff_freq_remainder)) { > + /* found a closer freq, save parameters */ > + clk->p[SEL_FRA] = 1; > + clk->p[SDM_MOD] = sdm; > + clk->p[PH_SEL] = ph; > + clk->p[NFRA] = nfra; > + clk->p[DIVR] = r; > + clk->p[DIVM] = m; > + > + fout = diff_freq / m; > + diff_min_quotient = diff_freq_quotient; > + diff_min_remainder = diff_freq_remainder; > + diff_min_sign = diff_freq_sign; > + } > + } > + } > + } > + > + if (!fout) { > + pr_err("%s: %s freq:%lu not found a valid setting\n", > + __func__, clk_hw_get_name(&clk->hw), freq); > + return -EINVAL; > + } > + > + return fout; > +} > + > +static long plltv_div(struct sp_pll *clk, unsigned long freq) > +{ > + if (freq % 100) > + return plltv_fractional_div(clk, freq); > + else > + return plltv_integer_div(clk, freq); > +} > + > +static void plltv_set_rate(struct sp_pll *clk) > +{ > + u32 reg; > + > + reg = HWM_FIELD_PREP(MASK_SEL_FRA, clk->p[SEL_FRA]); > + reg |= HWM_FIELD_PREP(MASK_SDM_MOD, clk->p[SDM_MOD]); > + reg |= HWM_FIELD_PREP(MASK_PH_SEL, clk->p[PH_SEL]); > + reg |= HWM_FIELD_PREP(MASK_NFRA, clk->p[NFRA]); > + clk_writel(reg, clk->reg); > + > + reg = HWM_FIELD_PREP(MASK_DIVR, clk->p[DIVR]); > + clk_writel(reg, clk->reg + 4); > + > + reg = HWM_FIELD_PREP(MASK_DIVN, clk->p[DIVN] - 1); > + reg |= HWM_FIELD_PREP(MASK_DIVM, clk->p[DIVM] - 1); > + clk_writel(reg, clk->reg + 8); > +} > + > +/******************************************** PLL_A ********************************************/ > + > +/* from Q628_PLLs_REG_setting.xlsx */ > +struct { > + u32 rate; > + u32 regs[5]; > +} pa[] = { Can this be const? and static? > + { > + .rate = 135475200, > + .regs = { > + 0x4801, > + 0x02df, > + 0x248f, > + 0x0211, > + 0x33e9 > + } > + }, > + { > + .rate = 147456000, > + .regs = { > + 0x4801, > + 0x1adf, > + 0x2490, > + 0x0349, > + 0x33e9 > + } > + }, > + { > + .rate = 196608000, > + .regs = { > + 0x4801, > + 0x42ef, > + 0x2495, > + 0x01c6, > + 0x33e9 > + } > + }, > +}; > + > +static void plla_set_rate(struct sp_pll *clk) > +{ > + const u32 *pp = pa[clk->p[0]].regs; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(pa->regs); i++) > + clk_writel(0xffff0000 | pp[i], clk->reg + (i * 4)); > +} > + > +static long plla_round_rate(struct sp_pll *clk, unsigned long rate) > +{ > + int i = ARRAY_SIZE(pa); > + > + while (--i) { > + if (rate >= pa[i].rate) > + break; > + } > + clk->p[0] = i; > + return pa[i].rate; > +} > + > +/******************************************* SP_PLL ********************************************/ > + > +static long sp_pll_calc_div(struct sp_pll *clk, unsigned long rate) > +{ > + u32 fbdiv; > + u32 max = 1 << clk->div_width; > + > + fbdiv = DIV_ROUND_CLOSEST(rate, clk->brate); > + if (fbdiv > max) > + fbdiv = max; > + return fbdiv; > +} > + > +static long sp_pll_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *prate) > +{ > + struct sp_pll *clk = to_sp_pll(hw); > + long ret; > + > + if (rate == *prate) > + ret = *prate; /* bypass */ > + else if (clk->div_width == DIV_A) { > + ret = plla_round_rate(clk, rate); > + } else if (clk->div_width == DIV_TV) { > + ret = plltv_div(clk, rate); > + if (ret < 0) > + ret = *prate; > + } else { > + ret = sp_pll_calc_div(clk, rate) * clk->brate; > + } > + > + return ret; > +} > + > +static unsigned long sp_pll_recalc_rate(struct clk_hw *hw, > + unsigned long prate) > +{ > + struct sp_pll *clk = to_sp_pll(hw); > + u32 reg = clk_readl(clk->reg); > + unsigned long ret; > + > + if (reg & BIT(clk->bp_bit)) { > + ret = prate; /* bypass */ > + } else if (clk->div_width == DIV_A) { > + ret = pa[clk->p[0]].rate; > + } else if (clk->div_width == DIV_TV) { > + u32 m, r, reg2; > + > + r = FIELD_GET(MASK_DIVR, clk_readl(clk->reg + 4)); > + reg2 = clk_readl(clk->reg + 8); > + m = FIELD_GET(MASK_DIVM, reg2) + 1; > + > + if (reg & BIT(1)) { /* SEL_FRA */ > + /* fractional divider */ > + u32 sdm = FIELD_GET(MASK_SDM_MOD, reg); > + u32 ph = FIELD_GET(MASK_PH_SEL, reg); > + u32 nfra = FIELD_GET(MASK_NFRA, reg); > + const u32 *pp = pt[ph]; > + > + ret = prate >> r; > + ret = (ret * (pp[1] + pp[2]) / pp[0]) - > + (ret * (mods[sdm] - nfra) / mods[sdm] / pp[4]); > + ret /= m; > + } else { > + /* integer divider */ > + u32 n = FIELD_GET(MASK_DIVN, reg2) + 1; > + > + ret = (prate / m * n) >> r; > + } > + } else { > + u32 fbdiv = (reg >> clk->div_shift) & ((1 << clk->div_width) - 1); > + > + ret = clk->brate * fbdiv; > + } > + > + return ret; > +} > + > +static int sp_pll_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long prate) > +{ > + struct sp_pll *clk = to_sp_pll(hw); > + unsigned long flags; > + u32 reg; > + > + spin_lock_irqsave(clk->lock, flags); > + > + reg = BIT(clk->bp_bit + 16); /* HIWORD_MASK */ > + > + if (rate == prate) > + reg |= BIT(clk->bp_bit); /* bypass */ > + else if (clk->div_width == DIV_A) > + plla_set_rate(clk); > + else if (clk->div_width == DIV_TV) > + plltv_set_rate(clk); > + else if (clk->div_width) { > + u32 fbdiv = sp_pll_calc_div(clk, rate); > + u32 mask = GENMASK(clk->div_shift + clk->div_width - 1, clk->div_shift); > + > + reg |= (mask << 16) | (((fbdiv - 1) << clk->div_shift) & mask); > + } > + > + clk_writel(reg, clk->reg); > + > + spin_unlock_irqrestore(clk->lock, flags); > + > + return 0; > +} > + > +static int sp_pll_enable(struct clk_hw *hw) > +{ > + struct sp_pll *clk = to_sp_pll(hw); > + unsigned long flags; > + > + spin_lock_irqsave(clk->lock, flags); > + clk_writel(BIT(clk->pd_bit + 16) | BIT(clk->pd_bit), clk->reg); /* power up */ > + spin_unlock_irqrestore(clk->lock, flags); > + > + return 0; > +} > + > +static void sp_pll_disable(struct clk_hw *hw) > +{ > + struct sp_pll *clk = to_sp_pll(hw); > + unsigned long flags; > + > + spin_lock_irqsave(clk->lock, flags); > + clk_writel(BIT(clk->pd_bit + 16), clk->reg); /* power down */ > + spin_unlock_irqrestore(clk->lock, flags); > +} > + > +static int sp_pll_is_enabled(struct clk_hw *hw) > +{ > + struct sp_pll *clk = to_sp_pll(hw); > + > + return clk_readl(clk->reg) & BIT(clk->pd_bit); > +} > + > +static const struct clk_ops sp_pll_ops = { > + .enable = sp_pll_enable, > + .disable = sp_pll_disable, > + .is_enabled = sp_pll_is_enabled, > + .round_rate = sp_pll_round_rate, > + .recalc_rate = sp_pll_recalc_rate, > + .set_rate = sp_pll_set_rate > +}; > + > +static const struct clk_ops sp_pll_sub_ops = { > + .enable = sp_pll_enable, > + .disable = sp_pll_disable, > + .is_enabled = sp_pll_is_enabled, > + .recalc_rate = sp_pll_recalc_rate, > +}; > + > +struct clk *clk_register_sp_pll(const char *name, const char *parent, > + void __iomem *reg, int pd_bit, int bp_bit, > + unsigned long brate, int shift, int width, > + spinlock_t *lock) > +{ > + struct sp_pll *pll; > + struct clk *clk; > + struct clk_init_data initd = { > + .name = name, > + .parent_names = &parent, > + .ops = (bp_bit >= 0) ? &sp_pll_ops : &sp_pll_sub_ops, > + .num_parents = 1, > + .flags = CLK_IGNORE_UNUSED Why? Preferably this flag is removed and either CLK_IS_CRITICAL is used, with a comment indicating what is critical, or the clk is removed entirely and driver probe just turns the clk on and forgets about it. > + }; > + > + pll = kzalloc(sizeof(*pll), GFP_KERNEL); > + if (!pll) > + return ERR_PTR(-ENOMEM); > + > + if (reg == PLLSYS_CTL) > + initd.flags |= CLK_IS_CRITICAL; Please comment why it is critical. > + > + pll->hw.init = &initd; > + pll->reg = reg; > + pll->pd_bit = pd_bit; > + pll->bp_bit = bp_bit; > + pll->brate = brate; > + pll->div_shift = shift; > + pll->div_width = width; > + pll->lock = lock; > + > + clk = clk_register(NULL, &pll->hw); Please use clk_hw_register > + if (WARN_ON(IS_ERR(clk))) { > + kfree(pll); > + } else { > + pr_info("%-20s%lu\n", name, clk_get_rate(clk)); > + clk_register_clkdev(clk, NULL, name); > + } > + > + return clk; > +} > + > +static void __init sp_clk_setup(struct device_node *np) > +{ > + int i, j; > + > + moon_regs = of_iomap(np, 0); Why moon_ prefix? > + if (WARN_ON(!moon_regs)) > + return; /* -ENXIO */ > + > + /* PLL_A */ > + clks[PLL_A] = clk_register_sp_pll("plla", EXTCLK, > + PLLA_CTL, 11, 12, 27000000, 0, DIV_A, &plla_lock); > + > + /* PLL_E */ > + clks[PLL_E] = clk_register_sp_pll("plle", EXTCLK, > + PLLE_CTL, 6, 2, 50000000, 0, 0, &plle_lock); > + clks[PLL_E_2P5] = clk_register_sp_pll("plle_2p5", "plle", > + PLLE_CTL, 13, -1, 2500000, 0, 0, &plle_lock); > + clks[PLL_E_25] = clk_register_sp_pll("plle_25", "plle", > + PLLE_CTL, 12, -1, 25000000, 0, 0, &plle_lock); > + clks[PLL_E_112P5] = clk_register_sp_pll("plle_112p5", "plle", > + PLLE_CTL, 11, -1, 112500000, 0, 0, &plle_lock); > + > + /* PLL_F */ > + clks[PLL_F] = clk_register_sp_pll("pllf", EXTCLK, > + PLLF_CTL, 0, 10, 13500000, 1, 4, &pllf_lock); > + > + /* PLL_TV */ > + clks[PLL_TV] = clk_register_sp_pll("plltv", EXTCLK, > + PLLTV_CTL, 0, 15, 27000000, 0, DIV_TV, &plltv_lock); > + clks[PLL_TV_A] = clk_register_divider(NULL, "plltv_a", "plltv", 0, > + PLLTV_CTL + 4, 5, 1, > + CLK_DIVIDER_POWER_OF_TWO, &plltv_lock); > + clk_register_clkdev(clks[PLL_TV_A], NULL, "plltv_a"); > + > + /* PLL_SYS */ > + clks[PLL_SYS] = clk_register_sp_pll("pllsys", EXTCLK, > + PLLSYS_CTL, 10, 9, 13500000, 0, 4, &pllsys_lock); > + > + /* gates */ > + for (i = 0; i < ARRAY_SIZE(gates); i++) { > + char s[10]; > + > + j = gates[i] & 0xffff; > + sprintf(s, "clken%02x", j); > + clks[j] = clk_register_gate(NULL, s, parents[gates[i] >> 16], CLK_IGNORE_UNUSED, Please use hw based registration. > + clk_regs + (j >> 4) * 4, j & 0x0f, > + CLK_GATE_HIWORD_MASK, NULL); > + } > + > + clk_data.clks = clks; > + clk_data.clk_num = ARRAY_SIZE(clks); > + of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data); Please use a hw based provider instead of a clk one. > +} > + > +CLK_OF_DECLARE(sp_clkc, "sunplus,sp7021-clkc", sp_clk_setup); Why can't this be a platform driver? > + > +MODULE_AUTHOR("Qin Jian <qinjian@xxxxxxxxxxx>"); > +MODULE_DESCRIPTION("Sunplus SP7021 Clock Driver"); > +MODULE_LICENSE("GPL v2"); It's not a module, so these macros should be removed.