Re: [PATCH v3 4/6] clk: add lpc18xx ccu clk driver

[Michael Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

Quoting Michael Turquette (2015-05-27 20:44:24)
> Hi Joachim,
> 
> Quoting Joachim Eastwood (2015-05-18 15:35:57)
> <snip>
> > +static void lpc18xx_ccu_register_branch_clks(void __iomem *reg_base,
> > +                                            int base_clk_id,
> > +                                            const char *parent)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(clk_branches); i++) {
> > +               if (clk_branches[i].base_id != base_clk_id)
> > +                       continue;
> > +
> > +               lpc18xx_ccu_register_branch_gate_div(&clk_branches[i], reg_base,
> > +                                                    parent);
> > +
> > +               if (clk_branches[i].flags & CCU_BRANCH_IS_BUS)
> > +                       parent = clk_branches[i].name;
> > +       }
> > +}
> > +
> > +static void __init lpc18xx_ccu_init(struct device_node *np)
> > +{
> > +       struct lpc18xx_branch_clk_data *clk_data;
> > +       int num_base_ids, *base_ids;
> > +       void __iomem *reg_base;
> > +       const char *parent;
> > +       int base_clk_id;
> > +       int i;
> > +
> > +       reg_base = of_iomap(np, 0);
> > +       if (!reg_base) {
> > +               pr_warn("%s: failed to map address range\n", __func__);
> > +               return;
> > +       }
> > +
> > +       clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
> > +       if (!clk_data)
> > +               return;
> > +
> > +       num_base_ids = of_clk_get_parent_count(np);
> > +
> > +       base_ids = kcalloc(num_base_ids, sizeof(int), GFP_KERNEL);
> > +       if (!base_ids) {
> > +               kfree(clk_data);
> > +               return;
> > +       }
> > +
> > +       clk_data->base_ids = base_ids;
> > +       clk_data->num_base_ids = num_base_ids;
> > +
> > +       for (i = 0; i < num_base_ids; i++) {
> > +               struct clk *clk = of_clk_get(np, i);
> > +               if (IS_ERR(clk)) {
> > +                       pr_warn("%s: failed to get clock at idx %d\n",
> > +                               __func__, i);
> > +                       continue;
> > +               }
> > +
> > +               parent = __clk_get_name(clk);
> > +               base_clk_id = of_clk_get_index(np, i);
> > +
> > +               clk_data->base_ids[i] = base_clk_id;
> > +               lpc18xx_ccu_register_branch_clks(reg_base, base_clk_id,
> > +                                                parent);
> 
> Thanks for sending V3. This driver is getting close!
> 
> So the main thing I don't understand is why you do not encode the CGU
> clock parent string names into this CCU driver. If I understand your
> approach correctly, you do the following in lpc18xx_ccu_init:

Ugh, I just saw that you mentioned this approach in your cover letter.
No need for you to answer all of my questions then.

Please make the change to encode the parent string names directly. V4
should be good to go after that.

Thanks,
Mike

> 
> 1) count the number of parent clocks (ostensibly CGU clocks)
> 2) iterate through all of those CGU clocks, extracting a base_id value
> (loop #1)
> 3) using this base_id as a key you walk through an array in the CCU
> driver trying to find a matching base_id value
> (loop #2)
> 4) after finding the corresponding CCU clocks you get the parent clock
> with of_clk_get
> 5) using of_clk_get you fetch its name with __clk_get_name
> 6) you pass this parent name into a fairly typical looking registration
> function
> 
> Assuming I got all of that right, I hope we can simplify it
> considerably.
> 
> You already have an array of CCU clock information in this driver,
> clk_branches[]. Why not encode the parent string name here? This would
> involve adding a "parent_name" member to struct lpc18xx_clk_branch.
> 
> Doing the above, your O(n^2)-ish registration function becomes O(n):
> 
> 1) iterate through the array of the CCU clocks (clk_branchs[])
> 2) register them
> 3) profit
> 
> I'm starting to think any reference to base_id is sign that things are
> wrong in your driver. I am unconvinced that you need to "share" this
> base_id across CGU and CCU drivers in the way you do. If I'm wrong
> please help me to understand.
> 
> As a wild thought, if you do not want to encode parent string names into
> this driver, have you tried to use the clock-names property in the CCU
> blob? You do not need clock-output-names in the CGU blob either. But
> this is just an idea. It is far for straightforward for you t encode the
> parent names in your clk_branches[] array.
> 
> Thanks,
> Mike
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html