Re: [PATCH v5 06/10] clk: Add Sunplus SP7021 clock driver

[Stephen Boyd <sboyd@kernel.org>]

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@cqplus1.com>");
> +MODULE_DESCRIPTION("Sunplus SP7021 Clock Driver");
> +MODULE_LICENSE("GPL v2");

It's not a module, so these macros should be removed.