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

[Stephen Boyd <sboyd@kernel.org>]

Quoting David Gow (2023-03-02 23:15:35)
> On Thu, 2 Mar 2023 at 09:38, Stephen Boyd <sboyd@kernel.org> 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@linux.dev>
> > Cc: David Gow <davidgow@google.com>
> > Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> > ---
> 
> Looks good, modulo bikeshedding below.

Cool!

> >
> > 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).

I was thinking of building it in with whatever mode CONFIG_KUNIT is
built as. If this is a module because CONFIG_KUNIT=m, then unit tests
would depend on that, and this would be a module as well. modprobe would
know that some unit test module depends on symbols provided by
clk-kunit.ko and thus load clk-kunit.ko first.

> 
> 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?

Yes, that is the intent.

> 
> > +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_).

Ok, got it. I was trying to match kunit_kzalloc() style. It makes it
easy to slap the 'kunit_' prefix on existing auto-completed function
names like kzalloc() or clk_prepare_enable().

I wasn't aware of drm_kunit_helper. That's a mouthful! We don't call it
slab_kunit_helper_kzalloc(). Maybe to satisfy all conditions it should
be:

	clk_prepare_enable_kunit()

so that kunit_ autocomplete doesn't have a big scroll list, and clk
subsystem autocompletes, and we know it is kunit specific.