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@xxxxxxxxxxxxxxx>
> > > ---
> > > 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
