Quoting David Gow (2023-03-02 23:15:35)
> On Thu, 2 Mar 2023 at 09:38, Stephen Boyd <sb...@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.higg...@linux.dev>
> > Cc: David Gow <david...@google.com>
> > Signed-off-by: Stephen Boyd <sb...@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.
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
