Hi Laurent,
On Tue, Apr 25, 2023 at 03:49:36AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> 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?
There doesn't seem to be much consistency in the kernel but in the majority
of cases it is self-evident I guess.
--
Regards,
Sakari Ailus
