| Message ID | 20181210073240.32278-5-weiyi.lu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Mediatek MT8183 clock and scpsys support | expand |
Quoting Weiyi Lu (2018-12-09 23:32:31) > diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c > index f0ff5f535c7e..81400601f107 100644 > --- a/drivers/clk/mediatek/clk-pll.c > +++ b/drivers/clk/mediatek/clk-pll.c > @@ -69,11 +71,13 @@ static unsigned long __mtk_pll_recalc_rate(struct mtk_clk_pll *pll, u32 fin, > { > int pcwbits = pll->data->pcwbits; > int pcwfbits; > + int ibits; > u64 vco; > u8 c = 0; > > /* The fractional part of the PLL divider. */ > - pcwfbits = pcwbits > INTEGER_BITS ? pcwbits - INTEGER_BITS : 0; > + ibits = pll->data->pcwibits ? pll->data->pcwibits : INTEGER_BITS; > + pcwfbits = pcwbits > ibits ? pcwbits - ibits : 0; This is practically unreadable. It should be changed to an if statement. > > vco = (u64)fin * pcw; > > @@ -167,9 +171,10 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw, > static void mtk_pll_calc_values(struct mtk_clk_pll *pll, u32 *pcw, u32 *postdiv, > u32 freq, u32 fin) > { > - unsigned long fmin = 1000 * MHZ; > + unsigned long fmin = pll->data->fmin ? pll->data->fmin : (1000 * MHZ); > const struct mtk_pll_div_table *div_table = pll->data->div_table; > u64 _pcw; > + int ibits; > u32 val; > > if (freq > pll->data->fmax) > @@ -193,7 +198,8 @@ static void mtk_pll_calc_values(struct mtk_clk_pll *pll, u32 *pcw, u32 *postdiv, > } > > /* _pcw = freq * postdiv / fin * 2^pcwfbits */ > - _pcw = ((u64)freq << val) << (pll->data->pcwbits - INTEGER_BITS); > + ibits = pll->data->pcwibits ? pll->data->pcwibits : INTEGER_BITS; > + _pcw = ((u64)freq << val) << (pll->data->pcwbits - ibits); Similar comment. Readability is low here.
On Fri, 2018-12-14 at 14:02 -0800, Stephen Boyd wrote: > Quoting Weiyi Lu (2018-12-09 23:32:31) > > diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c > > index f0ff5f535c7e..81400601f107 100644 > > --- a/drivers/clk/mediatek/clk-pll.c > > +++ b/drivers/clk/mediatek/clk-pll.c > > @@ -69,11 +71,13 @@ static unsigned long __mtk_pll_recalc_rate(struct mtk_clk_pll *pll, u32 fin, > > { > > int pcwbits = pll->data->pcwbits; > > int pcwfbits; > > + int ibits; > > u64 vco; > > u8 c = 0; > > > > /* The fractional part of the PLL divider. */ > > - pcwfbits = pcwbits > INTEGER_BITS ? pcwbits - INTEGER_BITS : 0; > > + ibits = pll->data->pcwibits ? pll->data->pcwibits : INTEGER_BITS; > > + pcwfbits = pcwbits > ibits ? pcwbits - ibits : 0; > > This is practically unreadable. It should be changed to an if statement. > OK, will be fixed in next version. > > > > vco = (u64)fin * pcw; > > > > @@ -167,9 +171,10 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw, > > static void mtk_pll_calc_values(struct mtk_clk_pll *pll, u32 *pcw, u32 *postdiv, > > u32 freq, u32 fin) > > { > > - unsigned long fmin = 1000 * MHZ; > > + unsigned long fmin = pll->data->fmin ? pll->data->fmin : (1000 * MHZ); > > const struct mtk_pll_div_table *div_table = pll->data->div_table; > > u64 _pcw; > > + int ibits; > > u32 val; > > > > if (freq > pll->data->fmax) > > @@ -193,7 +198,8 @@ static void mtk_pll_calc_values(struct mtk_clk_pll *pll, u32 *pcw, u32 *postdiv, > > } > > > > /* _pcw = freq * postdiv / fin * 2^pcwfbits */ > > - _pcw = ((u64)freq << val) << (pll->data->pcwbits - INTEGER_BITS); > > + ibits = pll->data->pcwibits ? pll->data->pcwibits : INTEGER_BITS; > > + _pcw = ((u64)freq << val) << (pll->data->pcwbits - ibits); > > Similar comment. Readability is low here. I thought these two lines here are clean enough. Just simple conditional assignment and shift operation. I'd like to not to change it. >
diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h index f83c2bbb677e..11b5517903d0 100644 --- a/drivers/clk/mediatek/clk-mtk.h +++ b/drivers/clk/mediatek/clk-mtk.h @@ -214,8 +214,10 @@ struct mtk_pll_data { unsigned int flags; const struct clk_ops *ops; u32 rst_bar_mask; + unsigned long fmin; unsigned long fmax; int pcwbits; + int pcwibits; uint32_t pcw_reg; int pcw_shift; const struct mtk_pll_div_table *div_table; diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c index f0ff5f535c7e..81400601f107 100644 --- a/drivers/clk/mediatek/clk-pll.c +++ b/drivers/clk/mediatek/clk-pll.c @@ -32,6 +32,8 @@ #define AUDPLL_TUNER_EN BIT(31) #define POSTDIV_MASK 0x7 + +/* default 7 bits integer, can be overridden with pcwibits. */ #define INTEGER_BITS 7 /* @@ -69,11 +71,13 @@ static unsigned long __mtk_pll_recalc_rate(struct mtk_clk_pll *pll, u32 fin, { int pcwbits = pll->data->pcwbits; int pcwfbits; + int ibits; u64 vco; u8 c = 0; /* The fractional part of the PLL divider. */ - pcwfbits = pcwbits > INTEGER_BITS ? pcwbits - INTEGER_BITS : 0; + ibits = pll->data->pcwibits ? pll->data->pcwibits : INTEGER_BITS; + pcwfbits = pcwbits > ibits ? pcwbits - ibits : 0; vco = (u64)fin * pcw; @@ -167,9 +171,10 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw, static void mtk_pll_calc_values(struct mtk_clk_pll *pll, u32 *pcw, u32 *postdiv, u32 freq, u32 fin) { - unsigned long fmin = 1000 * MHZ; + unsigned long fmin = pll->data->fmin ? pll->data->fmin : (1000 * MHZ); const struct mtk_pll_div_table *div_table = pll->data->div_table; u64 _pcw; + int ibits; u32 val; if (freq > pll->data->fmax) @@ -193,7 +198,8 @@ static void mtk_pll_calc_values(struct mtk_clk_pll *pll, u32 *pcw, u32 *postdiv, } /* _pcw = freq * postdiv / fin * 2^pcwfbits */ - _pcw = ((u64)freq << val) << (pll->data->pcwbits - INTEGER_BITS); + ibits = pll->data->pcwibits ? pll->data->pcwibits : INTEGER_BITS; + _pcw = ((u64)freq << val) << (pll->data->pcwbits - ibits); do_div(_pcw, fin); *pcw = (u32)_pcw;