[PATCH v2 2/5] clk: samsung: Add support to register rate_table for PLL3xxx

[Doug Anderson]

Vikas / Yadwinder,

On Wed, May 29, 2013 at 6:37 AM, Vikas Sajjan <vikas.sajjan at linaro.org> wrote:
> From: Yadwinder Singh Brar <yadi.brar at samsung.com>
>
> This patch defines a common rate_table which will contain recommended p, m, s,
> k values for supported rates that needs to be changed for changing
> corresponding PLL's rate.
>
> Signed-off-by: Yadwinder Singh Brar <yadi.brar at samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos4.c    |    8 ++++----
>  drivers/clk/samsung/clk-exynos5250.c |   14 +++++++-------
>  drivers/clk/samsung/clk-pll.c        |   14 ++++++++++++--
>  drivers/clk/samsung/clk-pll.h        |   33 +++++++++++++++++++++++++++++++--
>  4 files changed, 54 insertions(+), 15 deletions(-)

I also reviewed this in our gerrit
<https://gerrit.chromium.org/gerrit/#/c/56742/>, but I'll summarize
here for the list...

>  struct clk * __init samsung_clk_register_pll35xx(const char *name,
> -                       const char *pname, const void __iomem *base)
> +                       const char *pname, const void __iomem *base,
> +                       const struct samsung_pll_rate_table *rate_table,
> +                       const unsigned int rate_count)

Feels like you should document here that rate_table needs to be sorted
and the sort order.

> +struct samsung_pll_rate_table {
> +       unsigned  int rate;

nit: extra space before "int" should be removed.

Also: you can include rate here if you need a convenient place to
store it (which sadly means that this structure can't be const).
...but I do like Tomasz's idea of actually calculating it.  You can't
know it at compile time since the parent rate comes from the device
tree.

compatible = "samsung,clock-xxti";
clock-frequency = <24000000>;

> +       unsigned int pdiv;
> +       unsigned int mdiv;
> +       unsigned int sdiv;
> +       unsigned int kdiv;

I think kdiv is signed.

-Doug