Hi Hans,
Many thanks for working on this.
On Fri, Jan 20, 2023 at 12:45:19PM +0100, Hans de Goede wrote:
> Currently the videodev.ko code may be builtin while e.g. v4l2-fwnode.ko
> is build as a module.
>
> This makes it hard to add code depending on other subsystems spanning
> both videodev.ko and v4l2-fwnode.ko. Specifically this block adding code
> depending on the LED subsystem.
>
> This is made even harder because CONFIG_V4L2_FWNODE is selected,
> not depended on so it itself cannot depend on another subsystem without
> editing all the Kconfig symbols selecting it to also list the dependency
> and there are many of such symbols.
>
> Adding a "select LED_CLASS if NEW_LEDS" to CONFIG_V4L2_FWNODE leads
> to Kconfig erroring out with "error: recursive dependency detected!".
>
> To fix this dependency mess, change the V4L2_FWNODE and V4L2_ASYNC
> (which V4L2_FWNODE selects) Kconfig symbols from tristate to bools and
> link their code into videodev.ko instead of making them separate modules.
>
> This will allow using IS_REACHABLE(LED_CLASS) for the new LED integration
> code without needing to worry that it expands to 0 in some places and
> 1 in other places because some of the code being builtin vs modular.
>
> On x86_64 this leads to the following size changes for videodev.ko
>
> [hans@shalem linux]$ size drivers/media/v4l2-core/videodev.ko
>
> Before:
> text data bss dec hex filename
> 218206 14395 2448 235049 39629 drivers/media/v4l2-core/videodev.ko
> After:
> text data bss dec hex filename
> 243213 17615 2456 263284 40474 drivers/media/v4l2-core/videodev.ko
>
> So (as expected) there is some increase in size here, but it
> really is not that much.
>
> And the uncompressed no-debuginfo .ko file disk-usage actually shrinks
> by 17 KiB (comparing the slightly larger videodev.ko against the
> 3 original modules) and loading time will also be better.
>
> Acked-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> Changes in v5:
> - Add a new v4l2-dev-priv.h for the async debugfs prototypes and add
> static inline wrappers there when CONFIG_V4L2_ASYNC is not enabled
>
> Changes in v4:
> - New patch in v4 of this patch-set
> ---
> drivers/media/v4l2-core/Kconfig | 4 ++--
> drivers/media/v4l2-core/Makefile | 4 ++--
> drivers/media/v4l2-core/v4l2-async.c | 17 ++++-------------
> drivers/media/v4l2-core/v4l2-dev-priv.h | 19 +++++++++++++++++++
> drivers/media/v4l2-core/v4l2-dev.c | 8 ++++++++
> drivers/media/v4l2-core/v4l2-fwnode.c | 6 ------
> 6 files changed, 35 insertions(+), 23 deletions(-)
> create mode 100644 drivers/media/v4l2-core/v4l2-dev-priv.h
>
> diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
> index 348559bc2468..73574d946010 100644
> --- a/drivers/media/v4l2-core/Kconfig
> +++ b/drivers/media/v4l2-core/Kconfig
> @@ -68,11 +68,11 @@ config V4L2_FLASH_LED_CLASS
> When in doubt, say N.
>
> config V4L2_FWNODE
> - tristate
> + bool
> select V4L2_ASYNC
>
> config V4L2_ASYNC
> - tristate
> + bool
>
> # Used by drivers that need Videobuf modules
> config VIDEOBUF_GEN
> diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> index 41d91bd10cf2..8c5a1ab8d939 100644
> --- a/drivers/media/v4l2-core/Makefile
> +++ b/drivers/media/v4l2-core/Makefile
> @@ -15,7 +15,9 @@ videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
>
> # Please keep it alphabetically sorted by Kconfig name
> # (e. g. LC_ALL=C sort Makefile)
> +videodev-$(CONFIG_V4L2_ASYNC) += v4l2-async.o
> videodev-$(CONFIG_COMPAT) += v4l2-compat-ioctl32.o
> +videodev-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
> videodev-$(CONFIG_MEDIA_CONTROLLER) += v4l2-mc.o
> videodev-$(CONFIG_SPI) += v4l2-spi.o
> videodev-$(CONFIG_TRACEPOINTS) += v4l2-trace.o
> @@ -24,9 +26,7 @@ videodev-$(CONFIG_VIDEO_V4L2_I2C) += v4l2-i2c.o
> # Please keep it alphabetically sorted by Kconfig name
> # (e. g. LC_ALL=C sort Makefile)
>
> -obj-$(CONFIG_V4L2_ASYNC) += v4l2-async.o
> obj-$(CONFIG_V4L2_FLASH_LED_CLASS) += v4l2-flash-led-class.o
> -obj-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
> obj-$(CONFIG_V4L2_H264) += v4l2-h264.o
> obj-$(CONFIG_V4L2_JPEG_HELPER) += v4l2-jpeg.o
> obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 2f1b718a9189..024d6b82b50a 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -11,7 +11,6 @@
> #include <linux/i2c.h>
> #include <linux/list.h>
> #include <linux/mm.h>
> -#include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> @@ -24,6 +23,8 @@
> #include <media/v4l2-fwnode.h>
> #include <media/v4l2-subdev.h>
>
> +#include "v4l2-dev-priv.h"
> +
> static int v4l2_async_nf_call_bound(struct v4l2_async_notifier *n,
> struct v4l2_subdev *subdev,
> struct v4l2_async_subdev *asd)
> @@ -900,25 +901,15 @@ DEFINE_SHOW_ATTRIBUTE(pending_subdevs);
>
> static struct dentry *v4l2_async_debugfs_dir;
>
> -static int __init v4l2_async_init(void)
> +void __init v4l2_async_debugfs_init(void)
> {
> v4l2_async_debugfs_dir = debugfs_create_dir("v4l2-async", NULL);
> debugfs_create_file("pending_async_subdevices", 0444,
> v4l2_async_debugfs_dir, NULL,
> &pending_subdevs_fops);
> -
> - return 0;
> }
>
> -static void __exit v4l2_async_exit(void)
> +void __exit v4l2_async_debugfs_exit(void)
> {
> debugfs_remove_recursive(v4l2_async_debugfs_dir);
> }
> -
> -subsys_initcall(v4l2_async_init);
> -module_exit(v4l2_async_exit);
> -
> -MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@xxxxxx>");
> -MODULE_AUTHOR("Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>");
> -MODULE_AUTHOR("Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>");
> -MODULE_LICENSE("GPL");
> diff --git a/drivers/media/v4l2-core/v4l2-dev-priv.h b/drivers/media/v4l2-core/v4l2-dev-priv.h
> new file mode 100644
> index 000000000000..b5b1ee78be20
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-dev-priv.h
Could we call this v4l2-async-debugfs.h? I don't necessarily expect more
material here.
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Video capture interface for Linux version 2 private header.
> + *
> + * Copyright (C) 2023 Hans de Goede <hdegoede@xxxxxxxxxx>
> + */
> +
> +#ifndef _V4L2_DEV_PRIV_H_
> +#define _V4L2_DEV_PRIV_H_
> +
> +#if IS_ENABLED(CONFIG_V4L2_ASYNC)
> +void v4l2_async_debugfs_init(void);
> +void v4l2_async_debugfs_exit(void);
> +#else
> +static inline void v4l2_async_debugfs_init(void) {}
> +static inline void v4l2_async_debugfs_exit(void) {}
> +#endif
> +
> +#endif
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 397d553177fa..10ba2e4196a6 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -31,6 +31,8 @@
> #include <media/v4l2-ioctl.h>
> #include <media/v4l2-event.h>
>
> +#include "v4l2-dev-priv.h"
> +
> #define VIDEO_NUM_DEVICES 256
> #define VIDEO_NAME "video4linux"
>
> @@ -1190,6 +1192,7 @@ static int __init videodev_init(void)
> return -EIO;
> }
>
> + v4l2_async_debugfs_init();
> return 0;
> }
>
> @@ -1197,6 +1200,7 @@ static void __exit videodev_exit(void)
> {
> dev_t dev = MKDEV(VIDEO_MAJOR, 0);
>
> + v4l2_async_debugfs_exit();
> class_unregister(&video_class);
> unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
> }
> @@ -1205,6 +1209,10 @@ subsys_initcall(videodev_init);
> module_exit(videodev_exit)
>
> MODULE_AUTHOR("Alan Cox, Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>, Bill Dirks, Justin Schoeman, Gerd Knorr");
> +MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@xxxxxx>");
> +MODULE_AUTHOR("Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>");
> MODULE_DESCRIPTION("Video4Linux2 core driver");
> MODULE_LICENSE("GPL");
> MODULE_ALIAS_CHARDEV_MAJOR(VIDEO_MAJOR);
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 3d9533c1b202..c8a2264262bc 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -17,7 +17,6 @@
> #include <linux/acpi.h>
> #include <linux/kernel.h>
> #include <linux/mm.h>
> -#include <linux/module.h>
> #include <linux/of.h>
> #include <linux/property.h>
> #include <linux/slab.h>
> @@ -1328,8 +1327,3 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
> return ret;
> }
> EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor);
> -
> -MODULE_LICENSE("GPL");
> -MODULE_AUTHOR("Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>");
> -MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>");
> -MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@xxxxxx>");
> --
> 2.39.0
>
--
Sakari Ailus
