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:
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
