| Message ID | 20201224010907.263125-6-djrscally@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally <djrscally@gmail.com> wrote: > > To maintain consistency with software_node_unregister_nodes(), reverse > the order in which the software_node_unregister_node_group() function > unregisters nodes. ... > - * Unregister multiple software nodes at once. > + * Unregister multiple software nodes at once. The array will be unwound in > + * reverse order (i.e. last entry first) and thus if any member of the array > + * has its .parent member set then they should appear later in the array such > + * that they are unregistered first. I'm, as being not a native speaker, a bit confused by this comment. The idea is that children are unregistered first. Can you try to make it more clear maybe? > */
On 24/12/2020 12:13, Andy Shevchenko wrote: > On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally <djrscally@gmail.com> wrote: >> >> To maintain consistency with software_node_unregister_nodes(), reverse >> the order in which the software_node_unregister_node_group() function >> unregisters nodes. > > ... > >> - * Unregister multiple software nodes at once. >> + * Unregister multiple software nodes at once. The array will be unwound in >> + * reverse order (i.e. last entry first) and thus if any member of the array >> + * has its .parent member set then they should appear later in the array such >> + * that they are unregistered first. > > I'm, as being not a native speaker, a bit confused by this comment. > The idea is that children are unregistered first. Can you try to make > it more clear maybe? Sure, how about: The array will be unwound in reverse order (i.e. last entry first). If any member of the array is a child of another member then the child must appear later in the array than their parent, so that they are unregistered first. ?
On 24/12/2020 14:12, Andy Shevchenko wrote: > On Thu, Dec 24, 2020 at 4:00 PM Daniel Scally <djrscally@gmail.com> wrote: >> On 24/12/2020 12:13, Andy Shevchenko wrote: >>> On Thu, Dec 24, 2020 at 3:12 AM Daniel Scally <djrscally@gmail.com> wrote: > ... > >>>> + * Unregister multiple software nodes at once. The array will be unwound in >>>> + * reverse order (i.e. last entry first) and thus if any member of the array >>>> + * has its .parent member set then they should appear later in the array such >>>> + * that they are unregistered first. >>> I'm, as being not a native speaker, a bit confused by this comment. >>> The idea is that children are unregistered first. Can you try to make >>> it more clear maybe? >> Sure, how about: >> >> The array will be unwound in reverse order (i.e. last entry first). If >> any member of the array is a child of another member then the child must > children ? Yes, you are right of course. > >> appear later in the array than their parent, so that they are >> unregistered first. > I think with the above change it will be better, yes. > Ok, done.
From: Daniel Scally > Sent: 24 December 2020 14:14 ... > >> The array will be unwound in reverse order (i.e. last entry first). If > >> any member of the array is a child of another member then the child must > > children ? > > Yes, you are right of course. The second 'child' is a back-reference to 'any member' so is singular so 'child' is correct. 'the child' could be replaced by 'it' You could have: If any members of the array are children of another member then the children must appear later in the list. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, Dec 24, 2020 at 06:36:10PM +0000, David Laight wrote: > From: Daniel Scally > > Sent: 24 December 2020 14:14 > ... > > >> The array will be unwound in reverse order (i.e. last entry first). If > > >> any member of the array is a child of another member then the child must > > > children ? > > > > Yes, you are right of course. > > The second 'child' is a back-reference to 'any member' so is singular > so 'child' is correct. > 'the child' could be replaced by 'it' > > You could have: > If any members of the array are children of another member then the > children must appear later in the list. Works for me! Dan, can you consider David's proposal?
Hi Daniel, On Thu, Dec 24, 2020 at 01:08:58AM +0000, Daniel Scally wrote: > To maintain consistency with software_node_unregister_nodes(), reverse > the order in which the software_node_unregister_node_group() function > unregisters nodes. > > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v3 > - Fixed the dereference of the terminating NULL entry > - Comment cleanup > > drivers/base/swnode.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c > index ade49173ff8d..2d07eb04c6c8 100644 > --- a/drivers/base/swnode.c > +++ b/drivers/base/swnode.c > @@ -779,16 +779,22 @@ EXPORT_SYMBOL_GPL(software_node_register_node_group); > * software_node_unregister_node_group - Unregister a group of software nodes > * @node_group: NULL terminated array of software node pointers to be unregistered > * > - * Unregister multiple software nodes at once. > + * Unregister multiple software nodes at once. The array will be unwound in > + * reverse order (i.e. last entry first) and thus if any member of the array > + * has its .parent member set then they should appear later in the array such > + * that they are unregistered first. > */ > void software_node_unregister_node_group(const struct software_node **node_group) With this line wrapped, Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> > { > - unsigned int i; > + unsigned int i = 0; > > if (!node_group) > return; > > - for (i = 0; node_group[i]; i++) > + while (node_group[i]) > + i++; > + > + while (i--) > software_node_unregister(node_group[i]); > } > EXPORT_SYMBOL_GPL(software_node_unregister_node_group);
On Mon, Dec 28, 2020 at 06:34:10PM +0200, Sakari Ailus wrote: > On Thu, Dec 24, 2020 at 01:08:58AM +0000, Daniel Scally wrote: ... > > void software_node_unregister_node_group(const struct software_node **node_group) > > With this line wrapped, Why? It's only one character behind 80 and wrapping it will decrease readability. Moreover, documentation has explicit exceptions for such cases.
On 28/12/2020 10:15, Andy Shevchenko wrote: > On Thu, Dec 24, 2020 at 06:36:10PM +0000, David Laight wrote: >> From: Daniel Scally >>> Sent: 24 December 2020 14:14 >> ... >>>>> The array will be unwound in reverse order (i.e. last entry first). If >>>>> any member of the array is a child of another member then the child must >>>> children ? >>> >>> Yes, you are right of course. >> >> The second 'child' is a back-reference to 'any member' so is singular >> so 'child' is correct. >> 'the child' could be replaced by 'it' >> >> You could have: >> If any members of the array are children of another member then the >> children must appear later in the list. > > Works for me! > Dan, can you consider David's proposal? Yep - done, thanks David
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index ade49173ff8d..2d07eb04c6c8 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -779,16 +779,22 @@ EXPORT_SYMBOL_GPL(software_node_register_node_group); * software_node_unregister_node_group - Unregister a group of software nodes * @node_group: NULL terminated array of software node pointers to be unregistered * - * Unregister multiple software nodes at once. + * Unregister multiple software nodes at once. The array will be unwound in + * reverse order (i.e. last entry first) and thus if any member of the array + * has its .parent member set then they should appear later in the array such + * that they are unregistered first. */ void software_node_unregister_node_group(const struct software_node **node_group) { - unsigned int i; + unsigned int i = 0; if (!node_group) return; - for (i = 0; node_group[i]; i++) + while (node_group[i]) + i++; + + while (i--) software_node_unregister(node_group[i]); } EXPORT_SYMBOL_GPL(software_node_unregister_node_group);