From: David Gow <davidgow@google.com> To: Stephen Boyd <sboyd@kernel.org> Cc: Michael Turquette <mturquette@baylibre.com>, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org, patches@lists.linux.dev, Brendan Higgins <brendan.higgins@linux.dev>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael J . Wysocki" <rafael@kernel.org>, Richard Weinberger <richard@nod.at>, Anton Ivanov <anton.ivanov@cambridgegreys.com>, Johannes Berg <johannes@sipsolutions.net>, Vincent Whitchurch <vincent.whitchurch@axis.com>, Rob Herring <robh+dt@kernel.org>, Frank Rowand <frowand.list@gmail.com>, Christian Marangi <ansuelsmth@gmail.com>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, devicetree@vger.kernel.org, linux-um@lists.infradead.org, linux-kselftest@vger.kernel.org, kunit-dev@googlegroups.com Subject: Re: [PATCH 4/8] clk: Add test managed clk provider/consumer APIs Date: Sat, 11 Mar 2023 14:32:04 +0800 [thread overview] Message-ID: <CABVgOSk4gEob3rokKF_p2Bcd_Sj3ikUN4R-HPHyTR0Eoo==85g@mail.gmail.com> (raw) In-Reply-To: <77b315f6b89eb256c516ee08b1c17312.sboyd@kernel.org> On Sat, 11 Mar 2023 at 07:21, Stephen Boyd <sboyd@kernel.org> wrote: > > 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. > Personally, I'd rather have this behind CONFIG_CLK_KUNIT_TEST if possible, if only to avoid needlessly building these if someone just wants to test some other subsystem (but needs CONFIG_COMMON_CLK enabled anyway). I doubt it'd be a problem in practice in this case, but we definitely want to keep build (and hence iteration) times down as much as possible, so it's probably good practice to keep all tests behind at least some sort of "test this subsystem" option. > > > > 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(). Yeah: my rule of thumb at the moment is to keep the kunit_ prefix for things which are generic across the whole kernel (and tend to be implemented in lib/kunit), and to use suffixes or infixes (whichever works best) for things which are subsystem-specific. > 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. Sounds good to me. Cheers, -- David
WARNING: multiple messages have this Message-ID (diff)
From: David Gow <davidgow@google.com> To: Stephen Boyd <sboyd@kernel.org> Cc: Michael Turquette <mturquette@baylibre.com>, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org, patches@lists.linux.dev, Brendan Higgins <brendan.higgins@linux.dev>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael J . Wysocki" <rafael@kernel.org>, Richard Weinberger <richard@nod.at>, Anton Ivanov <anton.ivanov@cambridgegreys.com>, Johannes Berg <johannes@sipsolutions.net>, Vincent Whitchurch <vincent.whitchurch@axis.com>, Rob Herring <robh+dt@kernel.org>, Frank Rowand <frowand.list@gmail.com>, Christian Marangi <ansuelsmth@gmail.com>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, devicetree@vger.kernel.org, linux-um@lists.infradead.org, linux-kselftest@vger.kernel.org, kunit-dev@googlegroups.com Subject: Re: [PATCH 4/8] clk: Add test managed clk provider/consumer APIs Date: Sat, 11 Mar 2023 14:32:04 +0800 [thread overview] Message-ID: <CABVgOSk4gEob3rokKF_p2Bcd_Sj3ikUN4R-HPHyTR0Eoo==85g@mail.gmail.com> (raw) In-Reply-To: <77b315f6b89eb256c516ee08b1c17312.sboyd@kernel.org> On Sat, 11 Mar 2023 at 07:21, Stephen Boyd <sboyd@kernel.org> wrote: > > 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. > Personally, I'd rather have this behind CONFIG_CLK_KUNIT_TEST if possible, if only to avoid needlessly building these if someone just wants to test some other subsystem (but needs CONFIG_COMMON_CLK enabled anyway). I doubt it'd be a problem in practice in this case, but we definitely want to keep build (and hence iteration) times down as much as possible, so it's probably good practice to keep all tests behind at least some sort of "test this subsystem" option. > > > > 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(). Yeah: my rule of thumb at the moment is to keep the kunit_ prefix for things which are generic across the whole kernel (and tend to be implemented in lib/kunit), and to use suffixes or infixes (whichever works best) for things which are subsystem-specific. > 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. Sounds good to me. Cheers, -- David _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um
next prev parent reply other threads:[~2023-03-11 6:32 UTC|newest] Thread overview: 120+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-03-02 1:38 [PATCH 0/8] clk: Add kunit tests for fixed rate and parent data Stephen Boyd 2023-03-02 1:38 ` Stephen Boyd 2023-03-02 1:38 ` [PATCH 1/8] dt-bindings: Add linux,kunit binding Stephen Boyd 2023-03-02 1:38 ` Stephen Boyd 2023-03-03 7:14 ` David Gow 2023-03-03 7:14 ` David Gow 2023-03-03 7:49 ` Geert Uytterhoeven 2023-03-03 7:49 ` Geert Uytterhoeven 2023-03-09 23:12 ` Stephen Boyd 2023-03-09 23:12 ` Stephen Boyd 2023-03-10 7:55 ` David Gow 2023-03-10 7:55 ` David Gow 2023-03-02 1:38 ` [PATCH 2/8] of: Enable DTB loading on UML for KUnit tests Stephen Boyd 2023-03-02 1:38 ` Stephen Boyd 2023-03-03 7:15 ` David Gow 2023-03-03 7:15 ` David Gow 2023-03-09 23:19 ` Stephen Boyd 2023-03-09 23:19 ` Stephen Boyd 2023-03-10 8:09 ` David Gow 2023-03-10 8:09 ` David Gow 2023-03-10 23:34 ` Stephen Boyd 2023-03-10 23:34 ` Stephen Boyd 2023-03-11 6:42 ` David Gow 2023-03-11 6:42 ` David Gow 2023-03-13 16:02 ` Frank Rowand 2023-03-13 16:02 ` Frank Rowand 2023-03-14 4:28 ` Frank Rowand 2023-03-14 4:28 ` Frank Rowand 2023-03-15 7:04 ` David Gow 2023-03-15 7:04 ` David Gow 2023-03-15 21:35 ` Frank Rowand 2023-03-15 21:35 ` Frank Rowand 2023-03-16 0:45 ` Frank Rowand 2023-03-16 0:45 ` Frank Rowand 2023-03-16 4:15 ` David Gow 2023-03-16 4:15 ` David Gow 2023-03-21 20:56 ` Stephen Boyd 2023-03-21 20:56 ` Stephen Boyd 2023-03-08 19:46 ` Rob Herring 2023-03-08 19:46 ` Rob Herring 2023-03-02 1:38 ` [PATCH 3/8] kunit: Add test managed platform_device/driver APIs Stephen Boyd 2023-03-02 1:38 ` Stephen Boyd 2023-03-03 7:15 ` David Gow 2023-03-03 7:15 ` David Gow 2023-03-03 14:35 ` Maxime Ripard 2023-03-03 14:35 ` Maxime Ripard 2023-03-09 23:31 ` Stephen Boyd 2023-03-09 23:31 ` Stephen Boyd 2023-03-15 8:27 ` Maxime Ripard 2023-03-15 8:27 ` Maxime Ripard 2023-03-09 23:25 ` Stephen Boyd 2023-03-09 23:25 ` Stephen Boyd 2023-03-10 8:19 ` David Gow 2023-03-10 8:19 ` David Gow 2023-03-02 1:38 ` [PATCH 4/8] clk: Add test managed clk provider/consumer APIs Stephen Boyd 2023-03-02 1:38 ` Stephen Boyd 2023-03-03 7:15 ` David Gow 2023-03-03 7:15 ` David Gow 2023-03-10 23:21 ` Stephen Boyd 2023-03-10 23:21 ` Stephen Boyd 2023-03-11 6:32 ` David Gow [this message] 2023-03-11 6:32 ` David Gow 2023-03-21 14:32 ` Maxime Ripard 2023-03-21 14:32 ` Maxime Ripard 2023-03-02 1:38 ` [PATCH 5/8] dt-bindings: kunit: Add fixed rate clk consumer test Stephen Boyd 2023-03-02 1:38 ` Stephen Boyd 2023-03-02 1:38 ` [PATCH 6/8] clk: Add KUnit tests for clk fixed rate basic type Stephen Boyd 2023-03-02 1:38 ` Stephen Boyd 2023-03-02 1:38 ` [PATCH 7/8] dt-bindings: clk: Add KUnit clk_parent_data test Stephen Boyd 2023-03-02 1:38 ` Stephen Boyd 2023-03-02 1:38 ` [PATCH 8/8] clk: Add KUnit tests for clks registered with struct clk_parent_data Stephen Boyd 2023-03-02 1:38 ` Stephen Boyd 2023-03-02 8:13 ` [PATCH 0/8] clk: Add kunit tests for fixed rate and parent data David Gow 2023-03-02 8:13 ` David Gow 2023-03-02 17:32 ` Rob Herring 2023-03-02 17:32 ` Rob Herring 2023-03-02 19:27 ` Stephen Boyd 2023-03-02 19:27 ` Stephen Boyd 2023-03-02 19:47 ` Geert Uytterhoeven 2023-03-02 19:47 ` Geert Uytterhoeven 2023-03-05 3:32 ` Frank Rowand 2023-03-05 3:32 ` Frank Rowand 2023-03-05 9:26 ` Geert Uytterhoeven 2023-03-05 9:26 ` Geert Uytterhoeven 2023-03-06 5:32 ` Frank Rowand 2023-03-06 5:32 ` Frank Rowand 2023-03-04 15:04 ` Frank Rowand 2023-03-04 15:04 ` Frank Rowand 2023-03-07 21:53 ` Stephen Boyd 2023-03-07 21:53 ` Stephen Boyd 2023-03-04 14:48 ` Frank Rowand 2023-03-04 14:48 ` Frank Rowand 2023-03-02 17:13 ` Rob Herring 2023-03-02 17:13 ` Rob Herring 2023-03-02 19:44 ` Stephen Boyd 2023-03-02 19:44 ` Stephen Boyd 2023-03-02 20:18 ` Rob Herring 2023-03-02 20:18 ` Rob Herring 2023-03-02 23:57 ` Stephen Boyd 2023-03-02 23:57 ` Stephen Boyd 2023-03-04 15:39 ` Frank Rowand 2023-03-04 15:39 ` Frank Rowand 2023-03-06 12:53 ` Rob Herring 2023-03-06 12:53 ` Rob Herring 2023-03-06 15:03 ` Frank Rowand 2023-03-06 15:03 ` Frank Rowand 2023-03-04 15:37 ` Frank Rowand 2023-03-04 15:37 ` Frank Rowand 2023-03-04 15:33 ` Frank Rowand 2023-03-04 15:33 ` Frank Rowand 2023-03-03 14:38 ` Maxime Ripard 2023-03-03 14:38 ` Maxime Ripard 2023-03-07 22:37 ` Stephen Boyd 2023-03-07 22:37 ` Stephen Boyd 2023-03-04 15:50 ` Frank Rowand 2023-03-04 15:50 ` Frank Rowand 2023-03-10 7:48 ` David Gow 2023-03-10 7:48 ` David Gow 2023-03-13 15:30 ` Frank Rowand 2023-03-13 15:30 ` Frank Rowand
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CABVgOSk4gEob3rokKF_p2Bcd_Sj3ikUN4R-HPHyTR0Eoo==85g@mail.gmail.com' \ --to=davidgow@google.com \ --cc=ansuelsmth@gmail.com \ --cc=anton.ivanov@cambridgegreys.com \ --cc=brendan.higgins@linux.dev \ --cc=devicetree@vger.kernel.org \ --cc=frowand.list@gmail.com \ --cc=gregkh@linuxfoundation.org \ --cc=johannes@sipsolutions.net \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=kunit-dev@googlegroups.com \ --cc=linux-clk@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-kselftest@vger.kernel.org \ --cc=linux-um@lists.infradead.org \ --cc=mturquette@baylibre.com \ --cc=patches@lists.linux.dev \ --cc=rafael@kernel.org \ --cc=richard@nod.at \ --cc=robh+dt@kernel.org \ --cc=sboyd@kernel.org \ --cc=vincent.whitchurch@axis.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.