Re: [PATCH v1 1/1] test_printf: Refactor fwnode_pointer() to make it more readable

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 24 Aug 2022 20:05:42 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Converting fwnode_pointer() to use better swnode API allows to
> make code more readable.
> 
> While at it, rename full_name to full_name_third to show exact
> relation in the hierarchy.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/test_printf.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index fe13de1bed5f..6f7f179dd8f4 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -704,31 +704,29 @@ flags(void)
>  
>  static void __init fwnode_pointer(void)
>  {
> -	const struct software_node softnodes[] = {
> -		{ .name = "first", },
> -		{ .name = "second", .parent = &softnodes[0], },
> -		{ .name = "third", .parent = &softnodes[1], },
> -		{ NULL /* Guardian */ }
> -	};
> -	const char * const full_name = "first/second/third";
> +	const struct software_node first = { .name = "first" };
> +	const struct software_node second = { .name = "second", .parent = &first };
> +	const struct software_node third = { .name = "third", .parent = &second };

I personally do not find the above more readable, but honestly, I'm not
attached to this code at all.

> +	const struct software_node *group[] = { &first, &second, &third, NULL };

Could this just be:

	const struct software_node *group[] = {
		&softnodes[0], &softnodes[1], &softnodes[2], NULL };


>  	const char * const full_name_second = "first/second";
> +	const char * const full_name_third = "first/second/third";
>  	const char * const second_name = "second";
>  	const char * const third_name = "third";
>  	int rval;
>  
> -	rval = software_node_register_nodes(softnodes);
> +	rval = software_node_register_node_group(group);
>  	if (rval) {
>  		pr_warn("cannot register softnodes; rval %d\n", rval);
>  		return;
>  	}
>  
> -	test(full_name_second, "%pfw", software_node_fwnode(&softnodes[1]));
> -	test(full_name, "%pfw", software_node_fwnode(&softnodes[2]));
> -	test(full_name, "%pfwf", software_node_fwnode(&softnodes[2]));
> -	test(second_name, "%pfwP", software_node_fwnode(&softnodes[1]));
> -	test(third_name, "%pfwP", software_node_fwnode(&softnodes[2]));
> +	test(full_name_second, "%pfw", software_node_fwnode(&second));
> +	test(full_name_third, "%pfw", software_node_fwnode(&third));
> +	test(full_name_third, "%pfwf", software_node_fwnode(&third));
> +	test(second_name, "%pfwP", software_node_fwnode(&second));
> +	test(third_name, "%pfwP", software_node_fwnode(&third));

Then the above doesn't need to change.

But again, I'm not maintaining this code, so I'm not attached. Just adding
my $0.02 to this (as I'm triaging my inbox and found this email).

-- Steve


>  
> -	software_node_unregister_nodes(softnodes);
> +	software_node_unregister_node_group(group);
>  }
>  
>  static void __init fourcc_pointer(void)