Re: [PATCH v5 14/28] clk: Fix clk_get_parent() documentation

[Stephen Boyd <sboyd@kernel.org>]

Quoting Maxime Ripard (2022-05-16 06:25:13)
> The clk_get_parent() documentation in the header states that it will
> return a valid pointer, or an error pointer on failure.
> 
> However, the documentation in the source file, and the code itself, will
> return also return NULL if there isn't any parent for that clock. Let's

s/return//

> mention it.
> 
> An orphan clock should return NULL too, so let's add a test for it.
> 
> 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_test.c | 17 +++++++++++++++++
>  include/linux/clk.h    |  5 +++--
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index 9aa5b946f324..c52098e463d3 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -532,6 +532,22 @@ clk_orphan_transparent_multiple_parent_mux_test_exit(struct kunit *test)
>         clk_hw_unregister(&ctx->parents_ctx[1].hw);
>  }
>  
> +/*
> + * Test that, for a mux whose current parent hasn't been registered yet,

Mention "orphan" here somehow.

> + * clk_get_parent() will return NULL.
> + */
> +static void
> +clk_test_orphan_transparent_multiple_parent_mux_get_parent(struct kunit *test)
> +{
> +       struct clk_multiple_parent_ctx *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       struct clk *parent;
> +
> +       parent = clk_get_parent(clk);
> +       KUNIT_EXPECT_PTR_EQ(test, parent, NULL);

Please put clk_get_parent() into the expectation so that the print on
error is more verbose.

> +}
> +
>  /*
>   * Test that, for a mux whose current parent hasn't been registered yet,
>   * calling clk_set_parent() to a valid parent will properly update the
> @@ -678,6 +694,7 @@ clk_test_orphan_transparent_multiple_parent_mux_set_range_set_parent_get_rate(st
>  }
>  
>  static struct kunit_case clk_orphan_transparent_multiple_parent_mux_test_cases[] = {
> +       KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_get_parent),
>         KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent),
>         KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_get_rate),
>         KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_range_modified),

Please split the test from the documentation update.

> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 1507d5147898..39710b8453fa 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -755,8 +755,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent);
>   * clk_get_parent - get the parent clock source for this clock
>   * @clk: clock source
>   *
> - * Returns struct clk corresponding to parent clock source, or
> - * valid IS_ERR() condition containing errno.
> + * Returns struct clk corresponding to parent clock source, a NULL
> + * pointer if it doesn't have a parent, or a valid IS_ERR() condition
> + * containing errno.

I'd rather not update this. A return value of NULL is a 'struct clk
corresponding to parent clock source' already, and we don't want to
document CCF implementation details in clk.h because there are other
implementations of the API.