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

[Pankaj Dubey <pankaj.dubey@samsung.com>]

Hi Tomasz,


On Fri, Mar 7, 2014 at 11:12 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> 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().

Agreed.

>
>
>>
>>          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.

Good point. Let me change as per your suggestion and test. And then we
will go for this change in next new version of patch. Also please
review rest patch also and let us know if anything still can be
improved.

Thanks,
Pankaj Dubey

>
> Best regards,
> Tomasz
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel