Quoting Stephen Boyd (2019-08-01 10:12:09)
> Calls to clk_core_get() will return ERR_PTR(-EINVAL) if we've started
> migrating a clk driver to use the DT based style of specifying parents
> but we haven't made any DT updates yet. This happens when we pass a
> non-NULL value as the 'name' argument of of_parse_clkspec(). That
> function returns -EINVAL in such a situation, instead of -ENOENT like we
> expected. The return value comes back up to clk_core_fill_parent_index()
> which proceeds to skip calling clk_core_lookup() because the error
> pointer isn't equal to -ENOENT, it's -EINVAL.
>
> Furthermore, we'll blindly overwrite the error pointer returned by
> clk_core_get() with NULL when there isn't a legacy .name member
> specified in the parent map. This isn't too bad right now because we
> don't really care to differentiate NULL from an error, but in the future
> we should only try to do a legacy lookup if we know we might find
> something so that DT lookups that fail don't try to lookup based on
> strings when there isn't any string to match, hiding the error.
>
> Fix both these problems so that clk provider drivers can use the new
> style of parent mapping without having to also update their DT at the
> same time. This patch is based on an earlier patch from Taniya Das which
> checked for -EINVAL in addition to -ENOENT return values.
>
> Fixes: 601b6e93304a ("clk: Allow parents to be specified via clkspec index")
> Cc: Taniya Das <t...@codeaurora.org>
> Cc: Jerome Brunet <jbru...@baylibre.com>
> Cc: Chen-Yu Tsai <w...@csie.org>
> Reported-by: Taniya Das <t...@codeaurora.org>
> Signed-off-by: Stephen Boyd <sb...@kernel.org>
> ---
> drivers/clk/clk.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index c0990703ce54..6587a70c271c 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -355,8 +355,9 @@ static struct clk_core *clk_core_lookup(const char *name)
> * };
> *
> * Returns: -ENOENT when the provider can't be found or the clk doesn't
> - * exist in the provider. -EINVAL when the name can't be found. NULL when the
> - * provider knows about the clk but it isn't provided on this system.
> + * exist in the provider or the name can't be found in the DT node or
> + * in a clkdev lookup. NULL when the provider knows about the clk but it
> + * isn't provided on this system.
> * A valid clk_core pointer when the clk can be found in the provider.
> */
> static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index)
> @@ -374,9 +375,9 @@ static struct clk_core *clk_core_get(struct clk_core
> *core, u8 p_index)
> /*
> * If the DT search above couldn't find the provider or the provider
> * didn't know about this clk, fallback to looking up via clkdev based
> - * clk_lookups
> + * clk_lookups.
> */
> - if (PTR_ERR(hw) == -ENOENT && name)
> + if (IS_ERR(hw) && name)
> hw = clk_find_hw(dev_id, name);
I thought about this some more. I think we need to call
of_parse_clkspec() in clk_core_get() and only fallback to looking up in
clkdev if we can't parse the DT clock specifier. Otherwise, we'll have a
situation where the DT parsing may fail to find the clock because it
hasn't been registered yet, so it returns -EPROBE_DEFER, but then we'll
overwrite that error value with -ENOENT because clk_find_hw() will be
called.
I'll resend this with a better approach that should still fix the
original problem while making it possible for this scenario to return
errors from the clk provider lookup.
>
> if (IS_ERR(hw))
> @@ -401,7 +402,7 @@ static void clk_core_fill_parent_index(struct clk_core
> *core, u8 index)
> parent = ERR_PTR(-EPROBE_DEFER);
> } else {
> parent = clk_core_get(core, index);
> - if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT)
> + if (IS_ERR(parent) && entry->name)
> parent = clk_core_lookup(entry->name);
> }
