Re: [PATCH 4/8] clk: Add test managed clk provider/consumer APIs

[David Gow]

On Thu, 2 Mar 2023 at 09:38, Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
>
> Unit tests are more ergonomic and simpler to understand if they don't
> have to hoist a bunch of code into the test harness init and exit
> functions. Add some test managed wrappers for the clk APIs so that clk
> unit tests can write more code in the actual test and less code in the
> harness.
>
> Only add APIs that are used for now. More wrappers can be added in the
> future as necessary.
>
> Cc: Brendan Higgins <brendan.higgins@xxxxxxxxx>
> Cc: David Gow <davidgow@xxxxxxxxxx>
> Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxx>
> ---

Looks good, modulo bikeshedding below.

>  drivers/clk/Makefile    |   5 +
>  drivers/clk/clk-kunit.c | 204 ++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk-kunit.h |  28 ++++++
>  3 files changed, 237 insertions(+)
>  create mode 100644 drivers/clk/clk-kunit.c
>  create mode 100644 drivers/clk/clk-kunit.h
>
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index e3ca0d058a25..7efce649b0d3 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -17,6 +17,11 @@ ifeq ($(CONFIG_OF), y)
>  obj-$(CONFIG_COMMON_CLK)       += clk-conf.o
>  endif
>
> +# KUnit specific helpers
> +ifeq ($(CONFIG_COMMON_CLK), y)
> +obj-$(CONFIG_KUNIT)            += clk-kunit.o

Do we want to compile these in whenever KUnit is enabled, or only when
we're building clk tests specifically? I suspect this would be served
better by being under a CLK_KUNIT config option, which all of the
tests then depend on. (Whether that's the existing
CONFIG_CLK_KUNIT_TEST, and all of the clk tests live under the same
config option, or a separate parent option would be up to you).

Equally, this could be a bit interesting if CONFIG_KUNIT=m. Given
CONFIG_COMMON_CLK=y, this would end up as a clk-kunit module, no?

> +endif
> +
>  # hardware specific clock types
>  # please keep this section sorted lexicographically by file path name
>  obj-$(CONFIG_COMMON_CLK_APPLE_NCO)     += clk-apple-nco.o
> diff --git a/drivers/clk/clk-kunit.c b/drivers/clk/clk-kunit.c
> new file mode 100644
> index 000000000000..78d85b3a7a4a
> --- /dev/null
> +++ b/drivers/clk/clk-kunit.c
> @@ -0,0 +1,204 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit helpers for clk tests
> + */
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +
> +#include <kunit/resource.h>
> +
> +#include "clk-kunit.h"
> +
> +static void kunit_clk_disable_unprepare(struct kunit_resource *res)

We need to decide on the naming scheme of these, and in particular if
they should be kunit_clk or clk_kunit (or something else).

I'd lean to clk_kunit, if only to match DRM's KUnit helpers being
drm_kunit_helper better, and so that these are more tightly bound to
the subsystem being tested.
(i.e., so I don't have to scroll through every subsystem's helpers
when autocompleting kunit_).


> +{
> +       struct clk *clk = res->data;
> +
> +       clk_disable_unprepare(clk);
> +}
> +
> +/**
> + * kunit_clk_prepare_enable() - Test managed clk_prepare_enable()
> + * @test: The test context
> + * @clk: clk to prepare and enable
> + *
> + * Returns: 0 on success, or negative errno on failure.
> + */
> +int kunit_clk_prepare_enable(struct kunit *test, struct clk *clk)
> +{
> +       if (!kunit_alloc_resource(test, NULL, kunit_clk_disable_unprepare,
> +                                 GFP_KERNEL, clk))
> +               return -EINVAL;
> +
> +       return clk_prepare_enable(clk);
> +}
> +EXPORT_SYMBOL_GPL(kunit_clk_prepare_enable);
> +
> +static void kunit_clk_put(struct kunit_resource *res)
> +{
> +       struct clk *clk = res->data;
> +
> +       clk_put(clk);
> +}
> +
> +/**
> + * kunit_clk_get() - Test managed clk_get()
> + * @test: The test context
> + * @dev: device for clock "consumer"
> + * @id: clock consumer ID
> + *
> + * Returns: new clk consumer or ERR_PTR on failure
> + */
> +struct clk *
> +kunit_clk_get(struct kunit *test, struct device *dev, const char *con_id)
> +{
> +       struct clk *clk;
> +
> +       clk = clk_get(dev, con_id);
> +       if (IS_ERR(clk))
> +               return clk;
> +
> +       if (!kunit_alloc_resource(test, NULL, kunit_clk_put, GFP_KERNEL, clk)) {
> +               clk_put(clk);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       return clk;
> +}
> +EXPORT_SYMBOL_GPL(kunit_clk_get);
> +
> +/**
> + * kunit_of_clk_get() - Test managed of_clk_get()
> + * @test: The test context
> + * @np: device_node for clock "consumer"
> + * @index: index in 'clocks' property of @np
> + *
> + * Returns: new clk consumer or ERR_PTR on failure
> + */
> +struct clk *
> +kunit_of_clk_get(struct kunit *test, struct device_node *np, int index)
> +{
> +       struct clk *clk;
> +
> +       clk = of_clk_get(np, index);
> +       if (IS_ERR(clk))
> +               return clk;
> +
> +       if (!kunit_alloc_resource(test, NULL, kunit_clk_put, GFP_KERNEL, clk)) {
> +               clk_put(clk);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       return clk;
> +}
> +EXPORT_SYMBOL_GPL(kunit_of_clk_get);
> +
> +/**
> + * kunit_clk_hw_get_clk() - Test managed clk_hw_get_clk()
> + * @test: The test context
> + * @hw: clk_hw associated with the clk being consumed
> + * @con_id: connection ID string on device
> + *
> + * Returns: new clk consumer or ERR_PTR on failure
> + */
> +struct clk *
> +kunit_clk_hw_get_clk(struct kunit *test, struct clk_hw *hw, const char *con_id)
> +{
> +       struct clk *clk;
> +
> +       clk = clk_hw_get_clk(hw, con_id);
> +       if (IS_ERR(clk))
> +               return clk;
> +
> +       if (!kunit_alloc_resource(test, NULL, kunit_clk_put, GFP_KERNEL, clk)) {
> +               clk_put(clk);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       return clk;
> +}
> +EXPORT_SYMBOL_GPL(kunit_clk_hw_get_clk);
> +
> +/**
> + * kunit_clk_hw_get_clk_prepared_enabled() - Test managed clk_hw_get_clk() + clk_prepare_enable()
> + * @test: The test context
> + * @hw: clk_hw associated with the clk being consumed
> + * @con_id: connection ID string on device
> + *
> + * Returns: new clk consumer that is prepared and enabled or ERR_PTR on failure
> + */
> +struct clk *
> +kunit_clk_hw_get_clk_prepared_enabled(struct kunit *test, struct clk_hw *hw,
> +                                     const char *con_id)
> +{
> +       int ret;
> +       struct clk *clk;
> +
> +       clk = kunit_clk_hw_get_clk(test, hw, con_id);
> +       if (IS_ERR(clk))
> +               return clk;
> +
> +       ret = kunit_clk_prepare_enable(test, clk);
> +       if (ret)
> +               return ERR_PTR(ret);
> +
> +       return clk;
> +}
> +EXPORT_SYMBOL_GPL(kunit_clk_hw_get_clk_prepared_enabled);
> +
> +static void kunit_clk_hw_unregister(struct kunit_resource *res)
> +{
> +       struct clk_hw *hw = res->data;
> +
> +       clk_hw_unregister(hw);
> +}
> +
> +/**
> + * kunit_clk_hw_register() - Test managed clk_hw_register()
> + * @test: The test context
> + * @dev: device that is registering this clock
> + * @hw: link to hardware-specific clock data
> + *
> + * Returns: 0 on success or a negative errno value on failure
> + */
> +int kunit_clk_hw_register(struct kunit *test, struct device *dev, struct clk_hw *hw)
> +{
> +       int ret;
> +
> +       ret = clk_hw_register(dev, hw);
> +       if (ret)
> +               return ret;
> +
> +       if (!kunit_alloc_resource(test, NULL, kunit_clk_hw_unregister, GFP_KERNEL, hw)) {
> +               clk_hw_unregister(hw);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +/**
> + * kunit_of_clk_hw_register() - Test managed of_clk_hw_register()
> + * @test: The test context
> + * @node: device_node of device that is registering this clock
> + * @hw: link to hardware-specific clock data
> + *
> + * Returns: 0 on success or a negative errno value on failure
> + */
> +int kunit_of_clk_hw_register(struct kunit *test, struct device_node *node, struct clk_hw *hw)
> +{
> +       int ret;
> +
> +       ret = of_clk_hw_register(node, hw);
> +       if (ret)
> +               return ret;
> +
> +       if (!kunit_alloc_resource(test, NULL, kunit_clk_hw_unregister, GFP_KERNEL, hw)) {
> +               clk_hw_unregister(hw);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> diff --git a/drivers/clk/clk-kunit.h b/drivers/clk/clk-kunit.h
> new file mode 100644
> index 000000000000..153597d69269
> --- /dev/null
> +++ b/drivers/clk/clk-kunit.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _CLK_KUNIT_H
> +#define _CLK_KUNIT_H
> +
> +struct clk;
> +struct clk_hw;
> +struct device;
> +struct device_node;
> +struct kunit;
> +
> +struct clk *
> +kunit_clk_get(struct kunit *test, struct device *dev, const char *con_id);
> +struct clk *
> +kunit_of_clk_get(struct kunit *test, struct device_node *np, int index);
> +
> +struct clk *
> +kunit_clk_hw_get_clk(struct kunit *test, struct clk_hw *hw, const char *con_id);
> +struct clk *
> +kunit_clk_hw_get_clk_prepared_enabled(struct kunit *test, struct clk_hw *hw,
> +                                     const char *con_id);
> +
> +int kunit_clk_prepare_enable(struct kunit *test, struct clk *clk);
> +
> +int kunit_clk_hw_register(struct kunit *test, struct device *dev, struct clk_hw *hw);
> +int kunit_of_clk_hw_register(struct kunit *test, struct device_node *node,
> +                            struct clk_hw *hw);
> +
> +#endif
> --
> https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
> https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature