Message-ID: <508a5ee6c6b365e8d9cdefd5a9eec769.sboyd@kernel.org>
Date: Wed, 26 Feb 2025 17:01:05 -0800
From: Stephen Boyd <sboyd@...nel.org>
To: Jerome Brunet <jbrunet@...libre.com>, Kevin Hilman <khilman@...libre.com>, Martin Blumenstingl <martin.blumenstingl@...glemail.com>, Michael Turquette <mturquette@...libre.com>, Neil Armstrong <neil.armstrong@...aro.org>
Cc: linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org, linux-amlogic@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org, Jerome Brunet <jbrunet@...libre.com>
Subject: Re: [PATCH v3 1/4] clk: add a clk_hw helpers to get the clock device or device_node

Quoting Jerome Brunet (2025-01-20 09:15:30)
> Add helpers to get the device or device_node associated with clk_hw.
> 
> This can be used by clock drivers to access various device related
> functionality such as devres, dev_ prints, etc ...
> 
> Add test for these new helpers in clk-test.
> 
> Signed-off-by: Jerome Brunet <jbrunet@...libre.com>
> ---

Thanks for adding tests!

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 9b45fa005030f56e1478b9742715ebcde898133f..9818f87c1c56ab9a3782c2fd55d3f602041769c3 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -365,6 +365,39 @@ const char *clk_hw_get_name(const struct clk_hw *hw)
>  }
>  EXPORT_SYMBOL_GPL(clk_hw_get_name);
>  
> +/**
> + * clk_hw_get_dev - get device from an hardware clock.

Please add () to indicate function.

> + * @hw: the clk_hw pointer to get the struct device from
> + *
> + * This is a helper to get the struct device associated with a hardware
> + * clock. Some clocks, such as the ones registered from an early clock
> + * controller, may not be associated with any struct device.

Maybe write out that an 'early clock controller' is one that registers
clks with CLK_OF_DECLARE() or otherwise didn't pass a device pointer
while registering the clk.

> + *
> + * Return: the struct device associated with the clock, or NULL if there
> + * is none.
> + */
> +struct device *clk_hw_get_dev(const struct clk_hw *hw)
> +{
> +       return hw->core->dev;

Maybe we should increment the device refcount and require callers to
put_device(). Now's our chance to make the change!

> +}
> +EXPORT_SYMBOL_GPL(clk_hw_get_dev);
> +
> +/**
> + * clk_hw_get_of_node - get device_node from an hardware clock.

'from a hardware' and remove the period.

> + * @hw: the clk_hw pointer to get the struct device_node from
> + *
> + * This is a helper to get the struct device_node associated with an
> + * hardware clock.
> + *
> + * Return: the struct device_node associated with the clock, or NULL
> + * if there is none.
> + */

Can you put the kernel-doc in the header prototype? I want to move all
the comments there so we can include the header in the rst doc file with
the header 'clk provider API' or something like that.

> +struct device_node *clk_hw_get_of_node(const struct clk_hw *hw)
> +{
> +       return hw->core->of_node;

Maybe we should increment the of_node refcount and require callers to
of_node_put(). Now's our chance to make the change!

> +}
> +EXPORT_SYMBOL_GPL(clk_hw_get_of_node);
> +
>  struct clk_hw *__clk_get_hw(struct clk *clk)
>  {
>         return !clk ? NULL : clk->core->hw;
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index f08feeaa3750bc86859294650de298762dea690a..4dcdde283598b7f940c653ebc0d5a5f4c27637a2 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -3077,6 +3084,80 @@ static struct kunit_suite clk_register_clk_parent_data_device_suite = {
>         .test_cases = clk_register_clk_parent_data_device_test_cases,
>  };
>  
> +static void clk_register_dummy_device_driver(struct kunit *test)
> +{
> +       static const struct of_device_id match_table[] = {
> +               { .compatible = "test,clk-dummy-device" },
> +               { }
> +       };
> +
> +       clk_register_of_device_driver(test, match_table);
> +}
> +
> +/*
> + * Test that a clk registered with a struct device can provide back the
> + * struct device it was registered with.
> + */
> +static void clk_hw_get_dev_test(struct kunit *test)

The name of the test can tell us what it expects:

	clk_hw_get_dev_with_dev_gets_dev()
	clk_hw_get_dev_null_dev_gets_null()
	clk_hw_get_dev_with_node_gets_null() # this one uses of_clk_hw_register()
	clk_hw_get_of_node_with_dev_gets_node() # this one uses clk_hw_register()
	clk_hw_get_of_node_with_node_gets_node()
	clk_hw_get_of_node_null_node_gets_null()

I put some more test names. If we use gen params we can have spaces in
the name.

> +{
> +       struct clk_register_device_ctx *ctx;
> +
> +       ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +       test->priv = ctx;
> +
> +       clk_register_dummy_device_driver(test);
> +       ctx->hw.init = CLK_HW_INIT_NO_PARENT("test_get_dev",
> +                                            &clk_dummy_rate_ops, 0);
> +
> +       KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, ctx->dev, &ctx->hw));

Please put a newline between the assertions and the expectation. That
makes it easier to see at a glance what's expected by the test.

> +       KUNIT_EXPECT_PTR_EQ(test, ctx->dev, clk_hw_get_dev(&ctx->hw));
> +}
> +
> +/*
> + * Test that a clk registered with a struct device_node can provide back the
> + * struct device_node it was registered with.
> + */
> +static void clk_hw_get_of_node_test(struct kunit *test)
> +{
> +       struct device_node *np;
> +       struct clk_hw *hw;
> +
> +       hw = kunit_kzalloc(test, sizeof(*hw), GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
> +
> +       np = of_find_compatible_node(NULL, NULL, "test,clk-dummy-device");
> +       hw->init = CLK_HW_INIT_NO_PARENT("test_get_of_node",
> +                                        &clk_dummy_rate_ops, 0);
> +       of_node_put_kunit(test, np);
> +
> +       KUNIT_ASSERT_EQ(test, 0, of_clk_hw_register_kunit(test, np, hw));

The stuff before the expectation should likely go to the init function.
Or it can use the genparams stuff so we can set some struct members to
indicate if the pointer should be NULL or not and then twist through the
code a couple times.

> +       KUNIT_EXPECT_PTR_EQ(test, np, clk_hw_get_of_node(hw));
> +}
> +
> +static struct kunit_case clk_hw_test_cases[] = {
> +       KUNIT_CASE(clk_hw_get_dev_test),
> +       KUNIT_CASE(clk_hw_get_of_node_test),

Please add tests for the absence of the pointers.

> +       {}
> +};
> +
> +static int clk_hw_test_init(struct kunit *test)
> +{
> +       KUNIT_ASSERT_EQ(test, 0,
> +                       of_overlay_apply_kunit(test, kunit_clk_dummy_device));
> +
> +       return 0;
> +}
> +
> +/*
> + * Test suite to verify the sanity clk_hw helper functions.

Test suite to verify clk_hw_get_dev() and clk_hw_get_of_node().

> + */
> +static struct kunit_suite clk_hw_test_suite = {

A better name is clk_hw_get_dev_of_node_suite

> +       .name = "clk_hw_test_suite",

Same, clk_hw_get_dev_of_node_suite.

> +       .init = clk_hw_test_init,
> +       .test_cases = clk_hw_test_cases,
> +};
> +
>  struct clk_assigned_rates_context {
>         struct clk_dummy_context clk0;
>         struct clk_dummy_context clk1;
> diff --git a/drivers/clk/kunit_clk_dummy_device.dtso b/drivers/clk/kunit_clk_dummy_device.dtso
> new file mode 100644
> index 0000000000000000000000000000000000000000..5cc89aa11264428b09e47fd29c5f9ecfb8c32fdd
> --- /dev/null
> +++ b/drivers/clk/kunit_clk_dummy_device.dtso

Ideally the name of the file gives a clue to the name of the test suite that
uses it. Perhaps kunit_clk_hw_get_dev_of_node.dtso is better.

> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/dts-v1/;
> +/plugin/;
> +
> +&{/} {
> +       kunit-clock-controller {
> +               compatible = "test,clk-dummy-device";

Maybe "test,clk-hw-get-dev-node".

> +               #clock-cells = <0>;
> +       };
> +};

Powered by blists - more mailing lists