Re: [PATCH] clk: fix clock global name usage.

[Jerome Brunet]

On Fri, 2019-05-24 at 07:33 -0700, Stephen Boyd wrote:
> Do you set the index to 0 in this clk's parent_data? We purposefully set
> the index to -1 in clk_core_populate_parent_map() so that the fw_name
> can be NULL but the index can be something >= 0 and then we'll use that
> to lookup the clk from DT. We need to support that combination.
> 
>         fw_name   |   index |  DT lookup?
>         ----------+---------+------------
>         NULL      |    >= 0 |     Y
>         NULL      |    -1   |     N
>         non-NULL  |    -1   |     ?
>         non-NULL  |    >= 0 |     Y
> 
> Maybe we should support the ? case, because right now it will fail to do
> the DT lookup when the index is -1.

Hi Stephen,

We are trying to migrate all meson clocks to the new parent structure.
There is a little quirk which forces us to continue to use legacy names
for a couple of clocks.

We heavily use static data which init everything to 0.
Here is an example:

static struct clk_regmap g12a_aoclk_cts_rtc_oscin = {
[...]
 	.hw.init = &(struct clk_init_data){
 		.name = "g12a_ao_cts_rtc_oscin",
 		.ops = &clk_regmap_mux_ops,
-		.parent_names = (const char *[]){ "g12a_ao_32k_by_oscin",
-						  IN_PREFIX "ext_32k-0" },
+		.parent_data = (const struct clk_parent_data []) {
+			{ .name = "g12a_ao_32k_by_oscin" },
+			{ .fw_name = "ext-32k-0", },
+		},
 		.num_parents = 2,
 		.flags = CLK_SET_RATE_PARENT,
 	},
};

With this, instead of taking name = "g12a_ao_32k_by_oscin" for entry #0
it takes DT names at index 0 which is not what we intended.

If I understand correctly we should put
+			{ .name = "g12a_ao_32k_by_oscin", index = -1, },

And would be alright ?

While I understand it, it is not very obvious or nice to look at.
Plus it is a bit weird that this -1 is required for .name and not .hw.

Do you think we could come up with a priority order which makes the first
example work ?

Something like:

if (hw) {
	/* use pointer */
} else if (name) {
	/* use legacy global names */
} else if (fw_name) {
	/* use DT names */
} else if (index >= 0) 
	/* use DT index */
} else {
	return -EINVAL;
}

The last 2 clause could be removed if we make index an unsigned.

Cheers
Jerome

> 
> So this patch instead?