lkml.org 
[lkml]   [2022]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v1 1/1] test_printf: Refactor fwnode_pointer() to make it more readable
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@linux.intel.com> 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


\
 
 \ /
  Last update: 2022-09-29 19:07    [W:0.073 / U:0.724 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site