RE: [PATCH v15 05/10] clk: Add Sunplus SP7021 clock driver

[qinjian[覃健]]

> > +               nf = fvco * m_table[m];
> > +               n = nf / F_27M;
> > +               if ((n * F_27M) == nf)
> > +                       break;
> > +       }
> > +       m = m_table[m];
> > +
> > +       if (!m) {
> 
> This can be resolved with a 'return' from a subfunction or a goto.

Sorry, Could you explain more clear?
Did you means like:
	If (!m) {
		ret = -EINVAL;
		goto err_not_found;
	}
	...
	return freq;

err_not_found:
	...
	return ret;
}

> 
> > +               pr_err("%s: %s freq:%lu not found a valid setting\n",
> > +                      __func__, clk_hw_get_name(&clk->hw), freq);
> > +               return -EINVAL;
> > +       }
> > +
> > +       /* save parameters */
> > +       clk->p[SEL_FRA] = 0;
> > +       clk->p[DIVR]    = r;
> > +       clk->p[DIVN]    = n;
> > +       clk->p[DIVM]    = m;
> > +
> > +       return freq;
> > +}




> > +
> > +                               df_quotient  = df / m;
> > +                               df_remainder = ((df % m) * 1000) / m;
> > +
> > +                               if (freq > df_quotient) {
> > +                                       df_quotient  = freq - df_quotient - 1;
> > +                                       df_remainder = 1000 - df_remainder;
> 
> Where does 1000 come from?

1000 is come from "borrow 1" in last operation.

> 
> > +                               } else {
> > +                                       df_quotient = df_quotient - freq;
> > +                               }
> > +




> > +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);
> 
> Please push the lock down. Maybe that means having two conditionals?
> Either way, it is too large because it calls into plla_set_rate() and
> plltv_set_rate() with the lock held.

Sorry, I don't understand "push the lock down. Maybe that means having two conditionals?" what means.
The plla_set_rate() & plltv_set_rate() is only include simple operations & HW reg writes.
I think it should be called with lock held.

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++)
		writel(0xffff0000 | pp[i], clk->reg + (i * 4));
}

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]);
	writel(reg, clk->reg);

	reg  = HWM_FIELD_PREP(MASK_DIVR, clk->p[DIVR]);
	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);
	writel(reg, clk->reg + 8);
}