Re: [PATCH v15 08/19] media: subdev: Add for_each_active_route() macro

From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: Dafna Hirschfeld <dafna@fastmail.com>,
	linux-media@vger.kernel.org,
	Jacopo Mondi <jacopo+renesas@jmondi.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	niklas.soderlund+renesas@ragnatech.se,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Kishon Vijay Abraham <kishon@ti.com>,
	satish.nagireddy@getcruise.com, Tomasz Figa <tfiga@chromium.org>
Subject: Re: [PATCH v15 08/19] media: subdev: Add for_each_active_route() macro
Date: Thu, 13 Oct 2022 07:41:38 +0000	[thread overview]
Message-ID: <Y0fBMnu9QXplW+FW@paasikivi.fi.intel.com> (raw)
In-Reply-To: <c8a132f8-0f42-a720-72d1-06c29a90c5fc@ideasonboard.com>

On Wed, Oct 12, 2022 at 09:15:31AM +0300, Tomi Valkeinen wrote:
> On 09/10/2022 08:38, Dafna Hirschfeld wrote:
> > On 03.10.2022 15:18, Tomi Valkeinen wrote:
> > > From: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > 
> > > Add a for_each_active_route() macro to replace the repeated pattern
> > > of iterating on the active routes of a routing table.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > > ---
> > > .clang-format                         |  1 +
> > > drivers/media/v4l2-core/v4l2-subdev.c | 20 ++++++++++++++++++++
> > > include/media/v4l2-subdev.h           | 13 +++++++++++++
> > > 3 files changed, 34 insertions(+)
> > > 
> > > diff --git a/.clang-format b/.clang-format
> > > index 1247d54f9e49..31f39ae78f7b 100644
> > > --- a/.clang-format
> > > +++ b/.clang-format
> > > @@ -190,6 +190,7 @@ ForEachMacros:
> > >   - 'for_each_active_dev_scope'
> > >   - 'for_each_active_drhd_unit'
> > >   - 'for_each_active_iommu'
> > > +  - 'for_each_active_route'
> > >   - 'for_each_aggr_pgid'
> > >   - 'for_each_available_child_of_node'
> > >   - 'for_each_bench'
> > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
> > > b/drivers/media/v4l2-core/v4l2-subdev.c
> > > index 3ae4f39a50e4..1049c07d2e49 100644
> > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > @@ -1212,6 +1212,26 @@ int v4l2_subdev_set_routing(struct
> > > v4l2_subdev *sd,
> > > }
> > > EXPORT_SYMBOL_GPL(v4l2_subdev_set_routing);
> > > 
> > > +struct v4l2_subdev_route *
> > > +__v4l2_subdev_next_active_route(const struct v4l2_subdev_krouting
> > > *routing,
> > > +                struct v4l2_subdev_route *route)
> > > +{
> > > +    if (route)
> > > +        ++route;
> > > +    else
> > > +        route = &routing->routes[0];
> > > +
> > > +    for (; route < routing->routes + routing->num_routes; ++route) {
> > > +        if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
> > > +            continue;
> > > +
> > > +        return route;
> > > +    }
> > > +
> > > +    return NULL;
> > > +}
> > > +EXPORT_SYMBOL_GPL(__v4l2_subdev_next_active_route);
> > > +
> > > #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
> > > 
> > > #endif /* CONFIG_MEDIA_CONTROLLER */
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index 7962e6572bda..89e58208e330 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -1435,6 +1435,19 @@ int v4l2_subdev_set_routing(struct
> > > v4l2_subdev *sd,
> > >                 struct v4l2_subdev_state *state,
> > >                 const struct v4l2_subdev_krouting *routing);
> > > 
> > > +struct v4l2_subdev_route *
> > > +__v4l2_subdev_next_active_route(const struct v4l2_subdev_krouting
> > > *routing,
> > > +                struct v4l2_subdev_route *route);
> > > +
> > > +/**
> > > + * for_each_active_route - iterate on all active routes of a
> > > routing table
> > > + * @routing: The routing table
> > > + * @route: The route iterator
> > > + */
> > > +#define for_each_active_route(routing, route) \
> > > +    for ((route) = NULL;                  \
> > > +         ((route) = __v4l2_subdev_next_active_route((routing),
> > > (route)));)
> > Hi, shouldn't it be something like:
> >      for ((route) = NULL; (route) ; (route) =
> > __v4l2_subdev_next_active_route((routing), (route)))
> 
> What you suggest would never do anything: you initialize route to NULL, and
> then check if the route is !NULL.
> 
> I also find the current version a bit "interesting", but afaics, it works
> correctly.

It would look better IMO written differently, yes. But this likely results
in fewer instructions so I'm fine with it. ;)

-- 
Sakari Ailus