Re: [PATCH v5 21/28] clk: Introduce clk_core_has_parent()

[Stephen Boyd <sboyd@kernel.org>]

Quoting Maxime Ripard (2022-05-16 06:25:20)
> We will need to know if a clk_core pointer has a given parent in other
> functions, so let's create a clk_core_has_parent() function that
> clk_has_parent() will call into.
> 
> For good measure, let's add some unit tests as well to make sure it
> works properly.
> 
> Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com> # imx8mp
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> # exynos4210, meson g12b
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/clk/clk.c      | 36 +++++++++++++++++++++---------------
>  drivers/clk/clk_test.c | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 1a217c21be48..7754a5140a6b 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -539,6 +539,26 @@ static bool mux_is_better_rate(unsigned long rate, unsigned long now,
>  static int clk_core_round_rate_nolock(struct clk_core *core,
>                                       struct clk_rate_request *req);
>  
> +static bool clk_core_has_parent(struct clk_core *core, struct clk_core *parent)

const parent please. Sadly core isn't const though because we may fill
the cache in clk_core_get_parent_by_index().

> +{
> +       unsigned int i;
> +
> +       /* Optimize for the case where the parent is already the parent. */
> +       if (core == parent)
> +               return true;
> +
> +       for (i = 0; i < core->num_parents; i++) {
> +               struct clk_core *tmp = clk_core_get_parent_by_index(core, i);
> +               if (!tmp)
> +                       continue;
> +
> +               if (tmp == parent)
> +                       return true;
> +       }
> +
> +       return false;
> +}
> +
>  int clk_mux_determine_rate_flags(struct clk_hw *hw,
>                                  struct clk_rate_request *req,
>                                  unsigned long flags)
> @@ -2590,25 +2610,11 @@ void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent)
>   */
>  bool clk_has_parent(struct clk *clk, struct clk *parent)

Doesn't need to be done in this patch but we should probably mark both
clk and parent as const to indicate we're not going to modify them.

>  {
> -       struct clk_core *core, *parent_core;
> -       int i;
> -
>         /* NULL clocks should be nops, so return success if either is NULL. */
>         if (!clk || !parent)
>                 return true;
>  
> -       core = clk->core;
> -       parent_core = parent->core;
> -
> -       /* Optimize for the case where the parent is already the parent. */
> -       if (core->parent == parent_core)
> -               return true;
> -
> -       for (i = 0; i < core->num_parents; i++)
> -               if (!strcmp(core->parents[i].name, parent_core->name))
> -                       return true;
> -
> -       return false;
> +       return clk_core_has_parent(clk->core, parent->core);
>  }
>  EXPORT_SYMBOL_GPL(clk_has_parent);
>  
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index 4c71c6570021..7e1a231a5a6b 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -473,8 +473,24 @@ clk_test_multiple_parents_mux_get_parent(struct kunit *test)
>         KUNIT_EXPECT_TRUE(test, clk_is_match(parent, ctx->parents_ctx[0].hw.clk));
>  }
>  
> +/*
> + * Test that for a clock with a multiple parents, clk_has_parent()
> + * actually reports all of them as parents.
> + */
> +static void
> +clk_test_multiple_parents_mux_has_parent(struct kunit *test)
> +{
> +       struct clk_multiple_parent_ctx *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +
> +       KUNIT_EXPECT_TRUE(test, clk_has_parent(clk, ctx->parents_ctx[0].hw.clk));
> +       KUNIT_EXPECT_TRUE(test, clk_has_parent(clk, ctx->parents_ctx[1].hw.clk));
> +}
> +
>  static struct kunit_case clk_multiple_parents_mux_test_cases[] = {
>         KUNIT_CASE(clk_test_multiple_parents_mux_get_parent),
> +       KUNIT_CASE(clk_test_multiple_parents_mux_has_parent),
>         {}
>  };
>  
> @@ -884,6 +900,21 @@ clk_test_single_parent_mux_get_parent(struct kunit *test)
>         KUNIT_EXPECT_TRUE(test, clk_is_match(parent, ctx->parent_ctx.hw.clk));
>  }
>  
> +/*
> + * Test that for a clock with a single parent, clk_has_parent() actually
> + * reports it as a parent.
> + */
> +static void
> +clk_test_single_parent_mux_has_parent(struct kunit *test)
> +{
> +       struct clk_single_parent_ctx *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       struct clk *parent = ctx->parent_ctx.hw.clk;

Instead of using hw.clk directly can we use clk_hw_get_clk() and then
clk_put() it later? Eventually we want to remove the clk pointer inside
clk_hw, so every new use means more work later.

> +
> +       KUNIT_EXPECT_TRUE(test, clk_has_parent(clk, parent));
> +}
> +
>  /*
>   * Test that for a clock that can't modify its rate and with a single
>   * parent, if we set disjoints range on the parent and then the child,
> @@ -982,6 +1013,7 @@ clk_test_single_parent_mux_set_range_round_rate_child_smaller(struct kunit *test
>  
>  static struct kunit_case clk_single_parent_mux_test_cases[] = {
>         KUNIT_CASE(clk_test_single_parent_mux_get_parent),
> +       KUNIT_CASE(clk_test_single_parent_mux_has_parent),
>         KUNIT_CASE(clk_test_single_parent_mux_set_range_disjoint_child_last),
>         KUNIT_CASE(clk_test_single_parent_mux_set_range_disjoint_parent_last),
>         KUNIT_CASE(clk_test_single_parent_mux_set_range_round_rate_child_smaller),
> -- 
> 2.36.1
>