From: Ezequiel Garcia <ezequiel@collabora.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: linux-media@vger.kernel.org, ezequiel.garcia@collabora.com
Subject: Re: [PATCH v2 4/4] v4l: async, fwnode: Improve module organisation
Date: Sun, 28 Mar 2021 10:27:35 -0300 [thread overview]
Message-ID: <d0736505a4814aa2ba19c3c2cd9480d44b0781ef.camel@collabora.com> (raw)
In-Reply-To: <20210323192742.GA3@paasikivi.fi.intel.com>
On Tue, 2021-03-23 at 21:27 +0200, Sakari Ailus wrote:
> Hi Ezequiel,
>
> On Tue, Mar 23, 2021 at 02:46:04PM -0300, Ezequiel Garcia wrote:
> > On Fri, 2021-03-12 at 14:56 +0200, Sakari Ailus wrote:
> > > The V4L2 async framework is generally used with the V4L2 fwnode, which
> > > also depends on the former. There is only one exception, the CAFE_CCIC
> > > driver, which uses V4L2 async but does not need V4L2 fwnode.
> > >
> > > At the same time there is a vast number of systems that need videodev
> > > module, but have no use for v4l2-async that's now part of videodev.
> > >
> > > In order to improve, build v4l2-async and v4l2-fwnode as a single module
> > > called v4l2-async (the v4l2-async.c file is renamed as v4l2-async-core.c).
> > > Also the menu item V4L2_FWNODE is renamed as V4L2_ASYNC.
> > >
> >
> > Seems this is not what this patch is doing: the symbol is not renamed,
> > and now we have two modules v4l2-fwnode.ko and v4l2-async.ko, unless
> > I'm confused.
>
> Oh, well, I changed what the patch does but forgot to update this. :-I
>
> Basically this now splits v4l2-async out of videodev.
>
> >
> > I personally was more fond of having just one v4l2-async.ko module,
> > but I'm not sure if you found any obstacles.
> >
> > > This also moves the initialisation of the debufs entries for async subdevs
> > > to loading of the v4l2-async module. The directory is named as
> > > "v4l2-async".
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > drivers/media/i2c/Kconfig | 8 ++++++++
> > > drivers/media/v4l2-core/Kconfig | 4 ++++
> > > drivers/media/v4l2-core/Makefile | 11 +++++++++--
> > > drivers/media/v4l2-core/v4l2-async.c | 23 +++++++++++++++++++++--
> > > drivers/media/v4l2-core/v4l2-dev.c | 5 -----
> > > 5 files changed, 42 insertions(+), 9 deletions(-)
> > >
> > [..]
> > > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> > > index e4cd589b99a5..eb5ebb5461fd 100644
> > > --- a/drivers/media/v4l2-core/Makefile
> > > +++ b/drivers/media/v4l2-core/Makefile
> > > @@ -7,15 +7,22 @@ tuner-objs := tuner-core.o
> > >
> > > videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> > > v4l2-event.o v4l2-ctrls.o v4l2-subdev.o \
> > > - v4l2-async.o v4l2-common.o
> > > + v4l2-common.o
> > > videodev-$(CONFIG_COMPAT) += v4l2-compat-ioctl32.o
> > > videodev-$(CONFIG_TRACEPOINTS) += v4l2-trace.o
> > > videodev-$(CONFIG_MEDIA_CONTROLLER) += v4l2-mc.o
> > > videodev-$(CONFIG_SPI) += v4l2-spi.o
> > > videodev-$(CONFIG_VIDEO_V4L2_I2C) += v4l2-i2c.o
> > >
> > > -obj-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
> > > obj-$(CONFIG_VIDEO_V4L2) += videodev.o
> > > +ifneq ($(findstring y,$(CONFIG_V4L2_ASYNC)$(CONFIG_V4L2_FWNODE)),)
> >
> > Hm, to me this ifdefery is an indication that we are trying
> > to do something fishy.
>
> What's happening here is that v4l2-fwnode depends on v4l2-async, but for
> drivers to avoid selecting both, v4l2-async is built if only
> CONFIG_V4L2_FWNODE is set.
>
> So this perhaps isn't too pretty but it saves hassle in Kconfig files.
>
How about:
v4l2-async-objs := v4l2-async-core.o
v4l2-async-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
obj-$(CONFIG_V4L2_ASYNC) += v4l2-async.o
config V4L2_FWNODE
bool
select V4L2_ASYNC
config V4L2_ASYNC
tristate
This avoids the ifdefs and expresses the same behavior
you stated above:
* v4l2-async is now out of videodev
* v4l2-fwnode depends on v4l2-async (through select in this case,
given they are helper modules).
* v4l2-async is built if CONFIG_V4L2_FWNODE is set.
And v4l2-async is a module or built-in, depending on the
media subsystem (MEDIA_SUPPORT) being module or built-in,
which I think makes sense.
Thanks,
Ezequiel
next prev parent reply other threads:[~2021-03-28 13:28 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-12 12:56 [PATCH v2 0/4] Knead V4L2 async and fwnode frameworks a little Sakari Ailus
2021-03-12 12:56 ` [PATCH v2 1/4] v4l: fwnode: Rename and make static V4L2 async notifier helper Sakari Ailus
2021-03-23 16:18 ` Ezequiel Garcia
2021-03-12 12:56 ` [PATCH v2 2/4] v4l: fwnode: Rename v4l2_async_register_subdev_sensor_common Sakari Ailus
2021-03-23 16:56 ` Ezequiel Garcia
2021-03-12 12:56 ` [PATCH v2 3/4] v4l: async: Wrap long lines, remove '(' at the end of lines Sakari Ailus
2021-03-22 11:19 ` [PATCH v2.1 1/1] " Sakari Ailus
2021-03-12 12:56 ` [PATCH v2 4/4] v4l: async, fwnode: Improve module organisation Sakari Ailus
2021-03-12 15:02 ` Niklas Söderlund
2021-03-12 15:31 ` Sakari Ailus
2021-03-23 17:46 ` Ezequiel Garcia
2021-03-23 19:27 ` Sakari Ailus
2021-03-28 13:27 ` Ezequiel Garcia [this message]
2021-04-30 7:03 ` Sakari Ailus
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d0736505a4814aa2ba19c3c2cd9480d44b0781ef.camel@collabora.com \
--to=ezequiel@collabora.com \
--cc=ezequiel.garcia@collabora.com \
--cc=linux-media@vger.kernel.org \
--cc=sakari.ailus@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).