Re: [PATCH v4 5/5] clk/exynos5260: add clock file for exynos5260

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Pankaj,

On 07.03.2014 14:56, Pankaj Dubey wrote:
+static void __init exynos5260_clk_top_init(struct device_node *np)
+{
+       struct exynos5260_cmu_info cmu;
+       struct samsung_clk_provider *ctx;
+
+       memset(&cmu, 0, sizeof(cmu));
+
+       cmu.pll_clks = top_pll_clks;
+       cmu.nr_pll_clks =  ARRAY_SIZE(top_pll_clks);
+       cmu.mux_clks = top_mux_clks;
+       cmu.nr_mux_clks = ARRAY_SIZE(top_mux_clks);
+       cmu.div_clks = top_div_clks;
+       cmu.nr_div_clks = ARRAY_SIZE(top_div_clks);
+       cmu.gate_clks = top_gate_clks;
+       cmu.nr_gate_clks = ARRAY_SIZE(top_gate_clks);
+       cmu.nr_clk_ids = TOP_NR_CLK;
+       cmu.clk_regs = top_clk_regs;
+       cmu.nr_clk_regs = ARRAY_SIZE(top_clk_regs);
+
+       ctx = exynos5260_cmu_register_one(np, &cmu);
+
+       samsung_clk_of_register_fixed_ext(ctx, fixed_rate_ext_clks,
+                       ARRAY_SIZE(fixed_rate_ext_clks),
+                       ext_clk_match);
+}
+
+CLK_OF_DECLARE(exynos5260_clk_top, "samsung,exynos5260-clock-top",
+               exynos5260_clk_top_init);

Well with this approach we end up adding 14 such
exynosxxx_clk_xxx_init functions all of which has similar lines of
code. As I know there are many upcoming Exynos SoC which will also
have similar multiple clock controllers (in some of them there are
upto 25 clock domains, and in that case we will end up writing 25 such
init functions) so I have following suggestion where we can have one
more structure which will hold all static data and match_table to
match compatibility string and return CMU_TYPE which can be mapped to
get proper clock_data which can be used in single clock_init function.
Following is some sample code which I have implemented and tested on
one of Exynos SoC. Please let me know your opinion about this.

Yes, this looks better indeed, however there is still a room for improvement. Please see my comments below.


=============================

static struct exynosxxxx_clock_data exynosxxxx_clk_data[] __initdata = {

Instead of making this an array, particular elements could be separate structures. This would simplify the code below.

         {
                 .cmu_type = CMU_TYPE_TOP,
                 .mux_clocks = top_mux_clks,
                 .div_clocks = top_div_clks,
                 .pll_clocks = top_pll_clks,
                 .clk_regs = top_clk_regs,
                 .nr_mux_clocks = ARRAY_SIZE(top_mux_clks),
                 .nr_div_clocks = ARRAY_SIZE(top_div_clks),
                 .nr_pll_clocks = ARRAY_SIZE(top_pll_clks),
                 .nr_clk_regs = ARRAY_SIZE(top_clk_regs),
                 .nr_clks  = TOP_NR_CLK,
         }, {
                 .cmu_type = CMU_TYPE_EGL,
                 .mux_clocks = egl_mux_clks,
                 .div_clocks = egl_div_clks,
                 .pll_clocks = egl_pll_clks,
                  .clk_regs = egl_clk_regs,
                 .nr_mux_clocks = ARRAY_SIZE(egl_mux_clks),
                 .nr_div_clocks = ARRAY_SIZE(egl_div_clks),
                 .nr_pll_clocks = ARRAY_SIZE(egl_pll_clks),
                 .nr_clk_regs = ARRAY_SIZE(egl_clk_regs),
                 .nr_clks  = EGL_NR_CLK,
         }, {
        /* Add similar elements here */
};

static struct of_device_id cmu_subtype_match_table[] = {
         {
                 .compatible = "exynosxxxx-cmu-top",
                 .data   = (void *)CMU_TYPE_TOP,

Here the data would be just a pointer to respective clock data struct defined above.

         }, {
                 .compatible = "exynosxxx-cmu-peris",
                 .data   = (void *)CMU_TYPE_PERIS,
         }, {
        /* Add similar elements here */
};

void __init exynosxxx_clk_init(struct device_node *np)
{
          [snip]

         match = of_match_node(cmu_subtype_match_table, np);

         if (!match)
                 panic("%s: cmu type (%s) is not supported.\n", __func__,
                                 np->name);

This can't happen, because this function won't be called for any node with compatible string not declared using CLK_OF_DECLARE().


         reg_base = of_iomap(np, 0);
         if (!reg_base)
                 panic("%s: failed to map registers\n", __func__);

         cmu_type = (unsigned long) match->data;

         for (; i < CMU_TYPE_ALL; i++) {
                 clk_data = &exynosxxxx_clk_data[i];
                 if (cmu_type == clk_data->cmu_type)
                         break;
         }

Now clk_data could be taken directly from match->data, without the need to iterate over an array.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux