Messages in this thread | | | Date | Thu, 29 Sep 2022 20:04:04 +0300 | From | Andy Shevchenko <> | Subject | Re: [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
| |