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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> > > ---
> > > .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
