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

[Andy Shevchenko]

On Thu, Sep 29, 2022 at 12:06:32PM -0400, Steven Rostedt wrote:
> On Wed, 24 Aug 2022 20:05:42 +0300
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:

Thank you for review, my answers below.

...

> > +	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 };

It could, but the issue is that it will loose the self-explanatory naming
scheme. It's much easier to see what we test and what we expect in the below
calls...

> >  	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));

...here.

> Then the above doesn't need to change.

And that's why I want to change them.

> 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).

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

-- 
With Best Regards,
Andy Shevchenko