Re: [PATCH 07/16] clk: tests: Add Kunit testing for of_clk_get_parent_name()

Hi Stephen,

On 11/04/2026 at 18:29:49 -07, Stephen Boyd <[email protected]> wrote:

> Quoting Miquel Raynal (Schneider Electric) (2026-03-27 13:09:29)
>> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
>> index b814b45f1f7e..8a17ad0d185f 100644
>> --- a/drivers/clk/clk_test.c
>> +++ b/drivers/clk/clk_test.c
>> @@ -3651,9 +3651,19 @@ static void clk_parse_clkspec_with_incorrect_index_and_name(struct kunit *test)
>>         KUNIT_EXPECT_TRUE(test, IS_ERR(hw));
>>  }
>>  
>
> Add a comment here to tell us what the test is expecting.

Yep, done.

>> +static void clk_parse_and_get_parent_name(struct kunit *test)
>> +{
>> +       struct clk_parse_clkspec_ctx *ctx = test->priv;
>> +
>> +       KUNIT_EXPECT_STREQ(test,
>> +                          of_clk_get_parent_name(ctx->cons_np, 0),
>> +                          clk_parse_clkspec_1_init_data.name);
>> +}
>
> Reading this test is pretty hard because all the context is in the
> common setup. Maybe the common setup is too broad in this case and
> should be simplified so that tests can show more setup and assert code
> for the things it wants to do. For example, if the name can be passed
> from this test directly it would clarify by moving context into test
> function scope.
>
> 	const char *pname = "expected-name";
>
> 	KUNIT_ASSERT_EQ(test, 0, setup_parent(pname));
>
> 	KUNIT_EXPECT_STREQ(test, pname,
> 			   of_clk_get_parent_name(ctx->cons_np, 0));

While I understand the feeling, provider 1 and 2 are shared with other
test cases which would require restructuring quite a bit the whole test
suite for IMHO little benefit. I can however make more obvious the string
that is actually expected by redefining it in plain sight at the call
site.

>>  static struct kunit_case clk_parse_clkspec_test_cases[] = {
>>         KUNIT_CASE(clk_parse_clkspec_with_correct_index_and_name),
>>         KUNIT_CASE(clk_parse_clkspec_with_incorrect_index_and_name),
>> +       KUNIT_CASE(clk_parse_and_get_parent_name),
>
> Better to call the test something like
> of_clk_get_parent_name_gets_parent_name to indicate what we're
> testing.

Fine by me.

Thanks,
Miquèl