Re: [PATCH 07/18] media: v4l: async: Clean up list heads and entries

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sakari,

On Thu, Apr 27, 2023 at 02:52:37PM +0300, Sakari Ailus wrote:
> On Tue, Apr 25, 2023 at 03:49:36AM +0300, Laurent Pinchart wrote:
> > On Thu, Mar 30, 2023 at 02:58:42PM +0300, Sakari Ailus wrote:
> > > The naming of list heads and list entries is confusing as they're named
> > > similarly. Use _head for list head and _list for list entries.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  2 +-
> > >  .../platform/renesas/rcar-vin/rcar-core.c     |  2 +-
> > >  .../platform/renesas/rzg2l-cru/rzg2l-core.c   |  2 +-
> > >  drivers/media/platform/xilinx/xilinx-vipp.c   | 10 +--
> > >  drivers/media/v4l2-core/v4l2-async.c          | 66 +++++++++----------
> > >  .../staging/media/imx/imx-media-dev-common.c  |  2 +-
> > >  drivers/staging/media/tegra-video/vi.c        |  6 +-
> > >  include/media/v4l2-async.h                    | 21 +++---
> > >  8 files changed, 56 insertions(+), 55 deletions(-)
> > 
> > [snip]
> > 
> > > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > > index 0c4cffd081c9..425280b4d387 100644
> > > --- a/include/media/v4l2-async.h
> > > +++ b/include/media/v4l2-async.h
> > > @@ -68,7 +68,7 @@ struct v4l2_async_match {
> > >   * @match:	struct of match type and per-bus type matching data sets
> > >   * @asd_list:	used to add struct v4l2_async_subdev objects to the
> > >   *		master notifier @asd_list
> > 
> > It's now called @asd_head.
> > 
> > > - * @list:	used to link struct v4l2_async_subdev objects, waiting to be
> > > + * @waiting_list: used to link struct v4l2_async_subdev objects, waiting to be
> > >   *		probed, to a notifier->waiting list
> > 
> > It's now called notifier->waiting_head.
> > 
> > Please double-check comments and documentation to catch other
> > occurrences.
> 
> Sure.
> 
> > 
> > >   *
> > >   * When this struct is used as a member in a driver specific struct,
> > > @@ -77,9 +77,10 @@ struct v4l2_async_match {
> > >   */
> > >  struct v4l2_async_subdev {
> > >  	struct v4l2_async_match match;
> > > +
> > >  	/* v4l2-async core private: not to be used by drivers */
> > > -	struct list_head list;
> > >  	struct list_head asd_list;
> > > +	struct list_head waiting_list;
> > >  };
> > >  
> > >  /**
> > > @@ -108,20 +109,20 @@ struct v4l2_async_notifier_operations {
> > >   * @v4l2_dev:	v4l2_device of the root notifier, NULL otherwise
> > >   * @sd:		sub-device that registered the notifier, NULL otherwise
> > >   * @parent:	parent notifier
> > > - * @asd_list:	master list of struct v4l2_async_subdev
> > > - * @waiting:	list of struct v4l2_async_subdev, waiting for their drivers
> > > - * @done:	list of struct v4l2_subdev, already probed
> > > - * @list:	member in a global list of notifiers
> > > + * @asd_head:	master list of struct v4l2_async_subdev
> > > + * @waiting_list: list of struct v4l2_async_subdev, waiting for their drivers
> > > + * @done_head:	list of struct v4l2_subdev, already probed
> > > + * @notifier_list: member in a global list of notifiers
> > >   */
> > >  struct v4l2_async_notifier {
> > >  	const struct v4l2_async_notifier_operations *ops;
> > >  	struct v4l2_device *v4l2_dev;
> > >  	struct v4l2_subdev *sd;
> > >  	struct v4l2_async_notifier *parent;
> > > -	struct list_head asd_list;
> > > -	struct list_head waiting;
> > > -	struct list_head done;
> > > -	struct list_head list;
> > > +	struct list_head asd_head;
> > > +	struct list_head waiting_head;
> > > +	struct list_head done_head;
> > > +	struct list_head notifier_list;
> > 
> > I find the _head suffix to still be confusing. How about the following ?
> > 
> > 	struct {
> > 		struct list_head all;
> > 		struct list_head waiting;
> > 		struct list_head done;
> > 	} asds;
> 
> There are many list heads and entries in v4l2-async related structs and
> before this patch. _head is used for all list heads, _list for list
> entries. I prefer having _head as this way it is trivial to look for all
> instances of that list head, removing the _head part makes this much
> harder.
> 
> How about using _entry for list entries instead?

I like that. I would have used _entry for the list entries, and _list
for the list "heads". I don't like the _head suffix very much, as all of
them are struct list_head instances. I won't nack the series for this
though :-)

> There doesn't seem to be much consistency in the kernel but in the majority
> of cases it is self-evident I guess.

-- 
Regards,

Laurent Pinchart