Re: [PATCH v2 06/13] media: subdev: Add for_each_active_route() macro

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello,

On Thu, Oct 28, 2021 at 01:18:44PM +0300, Laurent Pinchart wrote:
> 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.
> >

Yeah, dumb me, the macro will just expand and the symbol won't be
available.

> > > 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.
>

Ack to the __ prefix and export the symbol from the .c file if it's
deemed too long to live in the header!

Thanks
   j

> --
> Regards,
>
> Laurent Pinchart



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux