On Thu, Oct 28, 2021 at 01:17:10PM +0300, Tomi Valkeinen wrote:
> On 28/10/2021 12:03, Jacopo Mondi wrote:
> > On Thu, Oct 28, 2021 at 11:32:12AM +0300, Tomi Valkeinen wrote:
> >> On 17/10/2021 21:24, Jacopo Mondi wrote:
> >>> Add a for_each_active_route() macro to replace the repeated pattern
> >>> of iterating on the active routes of a routing table.
> >>>
> >>> Replace the existing occurrences of such pattern in the codebase.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> >>> ---
> >>> drivers/media/i2c/ds90ub913.c | 8 ++------
> >>> drivers/media/i2c/ds90ub953.c | 7 ++-----
> >>> drivers/media/i2c/ds90ub960.c | 8 ++------
> >>> drivers/media/i2c/max9286.c | 10 ++--------
> >>> drivers/media/platform/ti-vpe/cal-video.c | 9 ++-------
> >>> drivers/media/v4l2-core/v4l2-subdev.c | 18 ++++++++++++++++++
> >>> include/media/v4l2-subdev.h | 11 +++++++++++
> >>> 7 files changed, 39 insertions(+), 32 deletions(-)
> >>>
> >>
> >> ...
> >>
> >>> +struct v4l2_subdev_route *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;
> >>> +}
> >>
> >> Also, this must be exported. I'll add that. And probably better to have a
> >
> > Does it ? I would rather have it in the header, as this is only
> > meant to be called by the for_each_active_route() macro, and not by
> > other users. However it seemed to be rather long to be defined as a
> > static inline function in the header, so I opted to move it to the c
> > file.
>
> Yes, static inline is an option. The function is a bit long-ish, though,
> as you mention.
>
> > To be honest, it's not clear to me what happens if a module calls the
> > for_each_active_route() macro that calls this non-exported function,
> > so you're probably correct.
>
> The module cannot be loaded if it refers to a non-exported symbol.
>
> > However exporting the symbol makes it available globally, but I guess
>
> Yes, thus the prefix is a good thing =).
>
> > that's not a big deal if it's clearly documented that drivers shall
> > not call this directly (or maybe we want it to be available globally,
> > why not...)
>
> I'll see how long helper functions similar macros have as inline in
> other parts of the kernel. Maybe static inline is fine.
>
> But if not, we'll just need to document the helper function. I don't see
> why we should say it shouldn't be called directly, though. But if that
> is the case, we can prefix it with __.
The __ prefix is exactly what I was going to propose.
--
Regards,
Laurent Pinchart
