[PATCH 0/6] V4L2 core I2C/SPI code cleanup

[PATCH 0/6] V4L2 core I2C/SPI code cleanup

[Ezequiel Garcia]

Hi Mauro, Hans:

While reading v4l2-common.c, I came across a few ifdefs
that could be cleaned-up with some minor reorganization.

Patch 1 is just cleaning the Makefile, removing ifeq/endif
to make it more readable.

Patch 2 merges v4l2-common.ko into videodev.ko, which
I think it's now possible. Let me know if having
these two modules separated serves a purpose
I'm missing.

The rest of the patches split the I2C and SPI helpers,
so they can be conditionally built.

There are a few checkpatch.pl issues triggered here,
all of them belonging to the already existing code.
Let me know if you want me to clean that as well.

The entire series should not affect any functionality,
but just clean-up the code a bit.

Thanks,
Eze

Ezequiel Garcia (6):
  media: v4l2-core: Cleanup Makefile
  media: v4l2-core: Module re-organization
  media: v4l2-core: move spi helpers out of v4l2-common.c
  media: v4l2-core: move i2c helpers out of v4l2-common.c
  media: v4l2-core: introduce a helper to unregister a SPI subdev
  media: v4l2-core: introduce a helper to unregister a I2C subdev

 drivers/media/v4l2-core/Kconfig       |   5 +
 drivers/media/v4l2-core/Makefile      |  15 +-
 drivers/media/v4l2-core/v4l2-common.c | 210 --------------------------
 drivers/media/v4l2-core/v4l2-device.c |  39 +----
 drivers/media/v4l2-core/v4l2-i2c.c    | 167 ++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-spi.c    |  73 +++++++++
 include/media/v4l2-common.h           | 150 +++++++++++++-----
 7 files changed, 366 insertions(+), 293 deletions(-)
 create mode 100644 drivers/media/v4l2-core/v4l2-i2c.c
 create mode 100644 drivers/media/v4l2-core/v4l2-spi.c

-- 
2.22.0

[PATCH 1/6] media: v4l2-core: Cleanup Makefile

[Ezequiel Garcia]

Use the videodev-$(CONFIG_FOO) syntax to simplify the Makefile.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/Makefile | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index 9ee57e1efefe..4d42418e603e 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -8,15 +8,11 @@ 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-clk.o \
 			v4l2-async.o
-ifeq ($(CONFIG_COMPAT),y)
-  videodev-objs += v4l2-compat-ioctl32.o
-endif
-obj-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
-ifeq ($(CONFIG_TRACEPOINTS),y)
-  videodev-objs += v4l2-trace.o
-endif
+videodev-$(CONFIG_COMPAT) += v4l2-compat-ioctl32.o
+videodev-$(CONFIG_TRACEPOINTS) += v4l2-trace.o
 videodev-$(CONFIG_MEDIA_CONTROLLER) += v4l2-mc.o
 
+obj-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
 obj-$(CONFIG_VIDEO_V4L2) += videodev.o
 obj-$(CONFIG_VIDEO_V4L2) += v4l2-common.o
 obj-$(CONFIG_VIDEO_V4L2) += v4l2-dv-timings.o
-- 
2.22.0

[PATCH 2/6] media: v4l2-core: Module re-organization

[Ezequiel Garcia]

videodev.ko and v4l2-common.ko driver are built under
the same conditions. Therefore, it doesn't make much sense
to split them in two different modules.

Splitting v4l2-common to its own driver has done many
years ago:

  commit a9254475bbfbed5f0596d952c6a3c9806e19dd0b
  Author: Mauro Carvalho Chehab <mchehab@infradead.org>
  Date:   Tue Jan 29 18:32:35 2008 -0300

      V4L/DVB (7115): Fix bug #9833: regression when compiling V4L without I2C

Back then, the subsystem organization was different.
However, With the current organization, there is no issue
compiling V4L2 with I2C as y/m/n.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index 4d42418e603e..8e2f52f7800b 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -7,14 +7,13 @@ 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-clk.o \
-			v4l2-async.o
+			v4l2-async.o v4l2-common.o
 videodev-$(CONFIG_COMPAT) += v4l2-compat-ioctl32.o
 videodev-$(CONFIG_TRACEPOINTS) += v4l2-trace.o
 videodev-$(CONFIG_MEDIA_CONTROLLER) += v4l2-mc.o
 
 obj-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
 obj-$(CONFIG_VIDEO_V4L2) += videodev.o
-obj-$(CONFIG_VIDEO_V4L2) += v4l2-common.o
 obj-$(CONFIG_VIDEO_V4L2) += v4l2-dv-timings.o
 
 obj-$(CONFIG_VIDEO_TUNER) += tuner.o
-- 
2.22.0

[PATCH 3/6] media: v4l2-core: move spi helpers out of v4l2-common.c

[Ezequiel Garcia]

Separate the spi helpers to v4l2-spi.c, in order to get rid
of the ifdefery. No functional changes intended, this is
just a cosmetic change to organize the code better.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/Makefile      |  1 +
 drivers/media/v4l2-core/v4l2-common.c | 65 ---------------------------
 drivers/media/v4l2-core/v4l2-spi.c    | 65 +++++++++++++++++++++++++++
 include/media/v4l2-common.h           | 18 +++++++-
 4 files changed, 82 insertions(+), 67 deletions(-)
 create mode 100644 drivers/media/v4l2-core/v4l2-spi.c

diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index 8e2f52f7800b..2deeeac6ee76 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -11,6 +11,7 @@ videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.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
 
 obj-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
 obj-$(CONFIG_VIDEO_V4L2) += videodev.o
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index f8ad1c580a3e..2b87b51f5667 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -41,9 +41,6 @@
 #include <linux/string.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
-#if defined(CONFIG_SPI)
-#include <linux/spi/spi.h>
-#endif
 #include <linux/uaccess.h>
 #include <asm/pgtable.h>
 #include <asm/io.h>
@@ -239,68 +236,6 @@ EXPORT_SYMBOL_GPL(v4l2_i2c_tuner_addrs);
 
 #endif /* defined(CONFIG_I2C) */
 
-#if defined(CONFIG_SPI)
-
-/* Load an spi sub-device. */
-
-void v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi,
-		const struct v4l2_subdev_ops *ops)
-{
-	v4l2_subdev_init(sd, ops);
-	sd->flags |= V4L2_SUBDEV_FL_IS_SPI;
-	/* the owner is the same as the spi_device's driver owner */
-	sd->owner = spi->dev.driver->owner;
-	sd->dev = &spi->dev;
-	/* spi_device and v4l2_subdev point to one another */
-	v4l2_set_subdevdata(sd, spi);
-	spi_set_drvdata(spi, sd);
-	/* initialize name */
-	snprintf(sd->name, sizeof(sd->name), "%s %s",
-		spi->dev.driver->name, dev_name(&spi->dev));
-}
-EXPORT_SYMBOL_GPL(v4l2_spi_subdev_init);
-
-struct v4l2_subdev *v4l2_spi_new_subdev(struct v4l2_device *v4l2_dev,
-		struct spi_master *master, struct spi_board_info *info)
-{
-	struct v4l2_subdev *sd = NULL;
-	struct spi_device *spi = NULL;
-
-	BUG_ON(!v4l2_dev);
-
-	if (info->modalias[0])
-		request_module(info->modalias);
-
-	spi = spi_new_device(master, info);
-
-	if (spi == NULL || spi->dev.driver == NULL)
-		goto error;
-
-	if (!try_module_get(spi->dev.driver->owner))
-		goto error;
-
-	sd = spi_get_drvdata(spi);
-
-	/* Register with the v4l2_device which increases the module's
-	   use count as well. */
-	if (v4l2_device_register_subdev(v4l2_dev, sd))
-		sd = NULL;
-
-	/* Decrease the module use count to match the first try_module_get. */
-	module_put(spi->dev.driver->owner);
-
-error:
-	/* If we have a client but no subdev, then something went wrong and
-	   we must unregister the client. */
-	if (!sd)
-		spi_unregister_device(spi);
-
-	return sd;
-}
-EXPORT_SYMBOL_GPL(v4l2_spi_new_subdev);
-
-#endif /* defined(CONFIG_SPI) */
-
 /* Clamp x to be between min and max, aligned to a multiple of 2^align.  min
  * and max don't have to be aligned, but there must be at least one valid
  * value.  E.g., min=17,max=31,align=4 is not allowed as there are no multiples
diff --git a/drivers/media/v4l2-core/v4l2-spi.c b/drivers/media/v4l2-core/v4l2-spi.c
new file mode 100644
index 000000000000..ab5a7eb4205d
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-spi.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * v4l2-spi - SPI helpers for Video4Linux2
+ */
+
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <media/v4l2-common.h>
+#include <media/v4l2-device.h>
+
+void v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi,
+		const struct v4l2_subdev_ops *ops)
+{
+	v4l2_subdev_init(sd, ops);
+	sd->flags |= V4L2_SUBDEV_FL_IS_SPI;
+	/* the owner is the same as the spi_device's driver owner */
+	sd->owner = spi->dev.driver->owner;
+	sd->dev = &spi->dev;
+	/* spi_device and v4l2_subdev point to one another */
+	v4l2_set_subdevdata(sd, spi);
+	spi_set_drvdata(spi, sd);
+	/* initialize name */
+	snprintf(sd->name, sizeof(sd->name), "%s %s",
+		spi->dev.driver->name, dev_name(&spi->dev));
+}
+EXPORT_SYMBOL_GPL(v4l2_spi_subdev_init);
+
+struct v4l2_subdev *v4l2_spi_new_subdev(struct v4l2_device *v4l2_dev,
+		struct spi_master *master, struct spi_board_info *info)
+{
+	struct v4l2_subdev *sd = NULL;
+	struct spi_device *spi = NULL;
+
+	BUG_ON(!v4l2_dev);
+
+	if (info->modalias[0])
+		request_module(info->modalias);
+
+	spi = spi_new_device(master, info);
+
+	if (spi == NULL || spi->dev.driver == NULL)
+		goto error;
+
+	if (!try_module_get(spi->dev.driver->owner))
+		goto error;
+
+	sd = spi_get_drvdata(spi);
+
+	/* Register with the v4l2_device which increases the module's
+	   use count as well. */
+	if (v4l2_device_register_subdev(v4l2_dev, sd))
+		sd = NULL;
+
+	/* Decrease the module use count to match the first try_module_get. */
+	module_put(spi->dev.driver->owner);
+
+error:
+	/* If we have a client but no subdev, then something went wrong and
+	   we must unregister the client. */
+	if (!sd)
+		spi_unregister_device(spi);
+
+	return sd;
+}
+EXPORT_SYMBOL_GPL(v4l2_spi_new_subdev);
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 6b319d0d73ad..a1c5288caa6a 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -216,11 +216,10 @@ const unsigned short *v4l2_i2c_tuner_addrs(enum v4l2_i2c_tuner_type type);
 /* ------------------------------------------------------------------------- */
 
 /* SPI Helper functions */
-#if defined(CONFIG_SPI)
 
 #include <linux/spi/spi.h>
 
-struct spi_device;
+#if defined(CONFIG_SPI)
 
 /**
  *  v4l2_spi_new_subdev - Load an spi module and return an initialized
@@ -246,6 +245,21 @@ struct v4l2_subdev *v4l2_spi_new_subdev(struct v4l2_device *v4l2_dev,
  */
 void v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi,
 		const struct v4l2_subdev_ops *ops);
+
+#else
+
+static inline struct v4l2_subdev *
+v4l2_spi_new_subdev(struct v4l2_device *v4l2_dev,
+		    struct spi_master *master, struct spi_board_info *info)
+{
+	return NULL;
+}
+
+static inline void
+v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi,
+		     const struct v4l2_subdev_ops *ops)
+{}
+
 #endif
 
 /* ------------------------------------------------------------------------- */
-- 
2.22.0

[PATCH 4/6] media: v4l2-core: move i2c helpers out of v4l2-common.c

[Ezequiel Garcia]

Separate the i2c helpers to v4l2-i2c.c, in order to get rid
of the ifdefery. No functional changes intended, this is
just a cosmetic change to organize the code better.

Given I2C is a tristate symbol, a hidden boolean symbol
is introduced, to make the conditional build easier.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/Kconfig       |   5 +
 drivers/media/v4l2-core/Makefile      |   1 +
 drivers/media/v4l2-core/v4l2-common.c | 145 -------------------------
 drivers/media/v4l2-core/v4l2-i2c.c    | 147 ++++++++++++++++++++++++++
 include/media/v4l2-common.h           | 113 +++++++++++++-------
 5 files changed, 229 insertions(+), 182 deletions(-)
 create mode 100644 drivers/media/v4l2-core/v4l2-i2c.c

diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
index 7c5f62f196e5..39e3fb30ba0b 100644
--- a/drivers/media/v4l2-core/Kconfig
+++ b/drivers/media/v4l2-core/Kconfig
@@ -11,6 +11,11 @@ config VIDEO_V4L2
 	select VIDEOBUF2_V4L2 if VIDEOBUF2_CORE
 	default (I2C || I2C=n) && VIDEO_DEV
 
+config VIDEO_V4L2_I2C
+	bool
+	depends on I2C && VIDEO_V4L2
+	default y
+
 config VIDEO_ADV_DEBUG
 	bool "Enable advanced debug functionality on V4L2 drivers"
 	help
diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index 2deeeac6ee76..786bd1ec4d1b 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -12,6 +12,7 @@ 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
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 2b87b51f5667..3f78b17e67bf 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -40,7 +40,6 @@
 #include <linux/mm.h>
 #include <linux/string.h>
 #include <linux/errno.h>
-#include <linux/i2c.h>
 #include <linux/uaccess.h>
 #include <asm/pgtable.h>
 #include <asm/io.h>
@@ -92,150 +91,6 @@ int v4l2_ctrl_query_fill(struct v4l2_queryctrl *qctrl, s32 _min, s32 _max, s32 _
 }
 EXPORT_SYMBOL(v4l2_ctrl_query_fill);
 
-/* I2C Helper functions */
-
-#if IS_ENABLED(CONFIG_I2C)
-
-void v4l2_i2c_subdev_set_name(struct v4l2_subdev *sd, struct i2c_client *client,
-			      const char *devname, const char *postfix)
-{
-	if (!devname)
-		devname = client->dev.driver->name;
-	if (!postfix)
-		postfix = "";
-
-	snprintf(sd->name, sizeof(sd->name), "%s%s %d-%04x", devname, postfix,
-		 i2c_adapter_id(client->adapter), client->addr);
-}
-EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_set_name);
-
-void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
-		const struct v4l2_subdev_ops *ops)
-{
-	v4l2_subdev_init(sd, ops);
-	sd->flags |= V4L2_SUBDEV_FL_IS_I2C;
-	/* the owner is the same as the i2c_client's driver owner */
-	sd->owner = client->dev.driver->owner;
-	sd->dev = &client->dev;
-	/* i2c_client and v4l2_subdev point to one another */
-	v4l2_set_subdevdata(sd, client);
-	i2c_set_clientdata(client, sd);
-	v4l2_i2c_subdev_set_name(sd, client, NULL, NULL);
-}
-EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
-
-/* Load an i2c sub-device. */
-struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
-		struct i2c_adapter *adapter, struct i2c_board_info *info,
-		const unsigned short *probe_addrs)
-{
-	struct v4l2_subdev *sd = NULL;
-	struct i2c_client *client;
-
-	BUG_ON(!v4l2_dev);
-
-	request_module(I2C_MODULE_PREFIX "%s", info->type);
-
-	/* Create the i2c client */
-	if (info->addr == 0 && probe_addrs)
-		client = i2c_new_probed_device(adapter, info, probe_addrs,
-					       NULL);
-	else
-		client = i2c_new_device(adapter, info);
-
-	/* Note: by loading the module first we are certain that c->driver
-	   will be set if the driver was found. If the module was not loaded
-	   first, then the i2c core tries to delay-load the module for us,
-	   and then c->driver is still NULL until the module is finally
-	   loaded. This delay-load mechanism doesn't work if other drivers
-	   want to use the i2c device, so explicitly loading the module
-	   is the best alternative. */
-	if (client == NULL || client->dev.driver == NULL)
-		goto error;
-
-	/* Lock the module so we can safely get the v4l2_subdev pointer */
-	if (!try_module_get(client->dev.driver->owner))
-		goto error;
-	sd = i2c_get_clientdata(client);
-
-	/* Register with the v4l2_device which increases the module's
-	   use count as well. */
-	if (v4l2_device_register_subdev(v4l2_dev, sd))
-		sd = NULL;
-	/* Decrease the module use count to match the first try_module_get. */
-	module_put(client->dev.driver->owner);
-
-error:
-	/* If we have a client but no subdev, then something went wrong and
-	   we must unregister the client. */
-	if (client && sd == NULL)
-		i2c_unregister_device(client);
-	return sd;
-}
-EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev_board);
-
-struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
-		struct i2c_adapter *adapter, const char *client_type,
-		u8 addr, const unsigned short *probe_addrs)
-{
-	struct i2c_board_info info;
-
-	/* Setup the i2c board info with the device type and
-	   the device address. */
-	memset(&info, 0, sizeof(info));
-	strscpy(info.type, client_type, sizeof(info.type));
-	info.addr = addr;
-
-	return v4l2_i2c_new_subdev_board(v4l2_dev, adapter, &info, probe_addrs);
-}
-EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
-
-/* Return i2c client address of v4l2_subdev. */
-unsigned short v4l2_i2c_subdev_addr(struct v4l2_subdev *sd)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-
-	return client ? client->addr : I2C_CLIENT_END;
-}
-EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_addr);
-
-/* Return a list of I2C tuner addresses to probe. Use only if the tuner
-   addresses are unknown. */
-const unsigned short *v4l2_i2c_tuner_addrs(enum v4l2_i2c_tuner_type type)
-{
-	static const unsigned short radio_addrs[] = {
-#if IS_ENABLED(CONFIG_MEDIA_TUNER_TEA5761)
-		0x10,
-#endif
-		0x60,
-		I2C_CLIENT_END
-	};
-	static const unsigned short demod_addrs[] = {
-		0x42, 0x43, 0x4a, 0x4b,
-		I2C_CLIENT_END
-	};
-	static const unsigned short tv_addrs[] = {
-		0x42, 0x43, 0x4a, 0x4b,		/* tda8290 */
-		0x60, 0x61, 0x62, 0x63, 0x64,
-		I2C_CLIENT_END
-	};
-
-	switch (type) {
-	case ADDRS_RADIO:
-		return radio_addrs;
-	case ADDRS_DEMOD:
-		return demod_addrs;
-	case ADDRS_TV:
-		return tv_addrs;
-	case ADDRS_TV_WITH_DEMOD:
-		return tv_addrs + 4;
-	}
-	return NULL;
-}
-EXPORT_SYMBOL_GPL(v4l2_i2c_tuner_addrs);
-
-#endif /* defined(CONFIG_I2C) */
-
 /* Clamp x to be between min and max, aligned to a multiple of 2^align.  min
  * and max don't have to be aligned, but there must be at least one valid
  * value.  E.g., min=17,max=31,align=4 is not allowed as there are no multiples
diff --git a/drivers/media/v4l2-core/v4l2-i2c.c b/drivers/media/v4l2-core/v4l2-i2c.c
new file mode 100644
index 000000000000..f393dd4f1c00
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-i2c.c
@@ -0,0 +1,147 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * v4l2-i2c - I2C helpers for Video4Linux2
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <media/v4l2-common.h>
+#include <media/v4l2-device.h>
+
+void v4l2_i2c_subdev_set_name(struct v4l2_subdev *sd, struct i2c_client *client,
+			      const char *devname, const char *postfix)
+{
+	if (!devname)
+		devname = client->dev.driver->name;
+	if (!postfix)
+		postfix = "";
+
+	snprintf(sd->name, sizeof(sd->name), "%s%s %d-%04x", devname, postfix,
+		 i2c_adapter_id(client->adapter), client->addr);
+}
+EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_set_name);
+
+void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
+		const struct v4l2_subdev_ops *ops)
+{
+	v4l2_subdev_init(sd, ops);
+	sd->flags |= V4L2_SUBDEV_FL_IS_I2C;
+	/* the owner is the same as the i2c_client's driver owner */
+	sd->owner = client->dev.driver->owner;
+	sd->dev = &client->dev;
+	/* i2c_client and v4l2_subdev point to one another */
+	v4l2_set_subdevdata(sd, client);
+	i2c_set_clientdata(client, sd);
+	v4l2_i2c_subdev_set_name(sd, client, NULL, NULL);
+}
+EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
+
+/* Load an i2c sub-device. */
+struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
+		struct i2c_adapter *adapter, struct i2c_board_info *info,
+		const unsigned short *probe_addrs)
+{
+	struct v4l2_subdev *sd = NULL;
+	struct i2c_client *client;
+
+	BUG_ON(!v4l2_dev);
+
+	request_module(I2C_MODULE_PREFIX "%s", info->type);
+
+	/* Create the i2c client */
+	if (info->addr == 0 && probe_addrs)
+		client = i2c_new_probed_device(adapter, info, probe_addrs,
+					       NULL);
+	else
+		client = i2c_new_device(adapter, info);
+
+	/* Note: by loading the module first we are certain that c->driver
+	   will be set if the driver was found. If the module was not loaded
+	   first, then the i2c core tries to delay-load the module for us,
+	   and then c->driver is still NULL until the module is finally
+	   loaded. This delay-load mechanism doesn't work if other drivers
+	   want to use the i2c device, so explicitly loading the module
+	   is the best alternative. */
+	if (client == NULL || client->dev.driver == NULL)
+		goto error;
+
+	/* Lock the module so we can safely get the v4l2_subdev pointer */
+	if (!try_module_get(client->dev.driver->owner))
+		goto error;
+	sd = i2c_get_clientdata(client);
+
+	/* Register with the v4l2_device which increases the module's
+	   use count as well. */
+	if (v4l2_device_register_subdev(v4l2_dev, sd))
+		sd = NULL;
+	/* Decrease the module use count to match the first try_module_get. */
+	module_put(client->dev.driver->owner);
+
+error:
+	/* If we have a client but no subdev, then something went wrong and
+	   we must unregister the client. */
+	if (client && sd == NULL)
+		i2c_unregister_device(client);
+	return sd;
+}
+EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev_board);
+
+struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
+		struct i2c_adapter *adapter, const char *client_type,
+		u8 addr, const unsigned short *probe_addrs)
+{
+	struct i2c_board_info info;
+
+	/* Setup the i2c board info with the device type and
+	   the device address. */
+	memset(&info, 0, sizeof(info));
+	strscpy(info.type, client_type, sizeof(info.type));
+	info.addr = addr;
+
+	return v4l2_i2c_new_subdev_board(v4l2_dev, adapter, &info, probe_addrs);
+}
+EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
+
+/* Return i2c client address of v4l2_subdev. */
+unsigned short v4l2_i2c_subdev_addr(struct v4l2_subdev *sd)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	return client ? client->addr : I2C_CLIENT_END;
+}
+EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_addr);
+
+/* Return a list of I2C tuner addresses to probe. Use only if the tuner
+   addresses are unknown. */
+const unsigned short *v4l2_i2c_tuner_addrs(enum v4l2_i2c_tuner_type type)
+{
+	static const unsigned short radio_addrs[] = {
+#if IS_ENABLED(CONFIG_MEDIA_TUNER_TEA5761)
+		0x10,
+#endif
+		0x60,
+		I2C_CLIENT_END
+	};
+	static const unsigned short demod_addrs[] = {
+		0x42, 0x43, 0x4a, 0x4b,
+		I2C_CLIENT_END
+	};
+	static const unsigned short tv_addrs[] = {
+		0x42, 0x43, 0x4a, 0x4b,		/* tda8290 */
+		0x60, 0x61, 0x62, 0x63, 0x64,
+		I2C_CLIENT_END
+	};
+
+	switch (type) {
+	case ADDRS_RADIO:
+		return radio_addrs;
+	case ADDRS_DEMOD:
+		return demod_addrs;
+	case ADDRS_TV:
+		return tv_addrs;
+	case ADDRS_TV_WITH_DEMOD:
+		return tv_addrs + 4;
+	}
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(v4l2_i2c_tuner_addrs);
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index a1c5288caa6a..8e66edddd37b 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -96,16 +96,45 @@ int v4l2_ctrl_query_fill(struct v4l2_queryctrl *qctrl,
 
 /* ------------------------------------------------------------------------- */
 
-/* I2C Helper functions */
-
-struct i2c_driver;
-struct i2c_adapter;
-struct i2c_client;
-struct i2c_device_id;
 struct v4l2_device;
 struct v4l2_subdev;
 struct v4l2_subdev_ops;
 
+/* I2C Helper functions */
+#include <linux/i2c.h>
+
+/**
+ * enum v4l2_i2c_tuner_type - specifies the range of tuner address that
+ *	should be used when seeking for I2C devices.
+ *
+ * @ADDRS_RADIO:		Radio tuner addresses.
+ *				Represent the following I2C addresses:
+ *				0x10 (if compiled with tea5761 support)
+ *				and 0x60.
+ * @ADDRS_DEMOD:		Demod tuner addresses.
+ *				Represent the following I2C addresses:
+ *				0x42, 0x43, 0x4a and 0x4b.
+ * @ADDRS_TV:			TV tuner addresses.
+ *				Represent the following I2C addresses:
+ *				0x42, 0x43, 0x4a, 0x4b, 0x60, 0x61, 0x62,
+ *				0x63 and 0x64.
+ * @ADDRS_TV_WITH_DEMOD:	TV tuner addresses if demod is present, this
+ *				excludes addresses used by the demodulator
+ *				from the list of candidates.
+ *				Represent the following I2C addresses:
+ *				0x60, 0x61, 0x62, 0x63 and 0x64.
+ *
+ * NOTE: All I2C addresses above use the 7-bit notation.
+ */
+enum v4l2_i2c_tuner_type {
+	ADDRS_RADIO,
+	ADDRS_DEMOD,
+	ADDRS_TV,
+	ADDRS_TV_WITH_DEMOD,
+};
+
+#if defined(CONFIG_VIDEO_V4L2_I2C)
+
 /**
  * v4l2_i2c_new_subdev - Load an i2c module and return an initialized
  *	&struct v4l2_subdev.
@@ -123,8 +152,6 @@ struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
 		struct i2c_adapter *adapter, const char *client_type,
 		u8 addr, const unsigned short *probe_addrs);
 
-struct i2c_board_info;
-
 /**
  * v4l2_i2c_new_subdev_board - Load an i2c module and return an initialized
  *	&struct v4l2_subdev.
@@ -174,35 +201,6 @@ void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
  */
 unsigned short v4l2_i2c_subdev_addr(struct v4l2_subdev *sd);
 
-/**
- * enum v4l2_i2c_tuner_type - specifies the range of tuner address that
- *	should be used when seeking for I2C devices.
- *
- * @ADDRS_RADIO:		Radio tuner addresses.
- *				Represent the following I2C addresses:
- *				0x10 (if compiled with tea5761 support)
- *				and 0x60.
- * @ADDRS_DEMOD:		Demod tuner addresses.
- *				Represent the following I2C addresses:
- *				0x42, 0x43, 0x4a and 0x4b.
- * @ADDRS_TV:			TV tuner addresses.
- *				Represent the following I2C addresses:
- *				0x42, 0x43, 0x4a, 0x4b, 0x60, 0x61, 0x62,
- *				0x63 and 0x64.
- * @ADDRS_TV_WITH_DEMOD:	TV tuner addresses if demod is present, this
- *				excludes addresses used by the demodulator
- *				from the list of candidates.
- *				Represent the following I2C addresses:
- *				0x60, 0x61, 0x62, 0x63 and 0x64.
- *
- * NOTE: All I2C addresses above use the 7-bit notation.
- */
-enum v4l2_i2c_tuner_type {
-	ADDRS_RADIO,
-	ADDRS_DEMOD,
-	ADDRS_TV,
-	ADDRS_TV_WITH_DEMOD,
-};
 /**
  * v4l2_i2c_tuner_addrs - Return a list of I2C tuner addresses to probe.
  *
@@ -213,6 +211,47 @@ enum v4l2_i2c_tuner_type {
  */
 const unsigned short *v4l2_i2c_tuner_addrs(enum v4l2_i2c_tuner_type type);
 
+#else
+
+static inline struct v4l2_subdev *
+v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
+		    struct i2c_adapter *adapter, const char *client_type,
+		    u8 addr, const unsigned short *probe_addrs)
+{
+	return NULL;
+}
+
+static inline struct v4l2_subdev *
+v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
+			  struct i2c_adapter *adapter, struct i2c_board_info *info,
+			  const unsigned short *probe_addrs)
+{
+	return NULL;
+}
+
+static inline void
+v4l2_i2c_subdev_set_name(struct v4l2_subdev *sd, struct i2c_client *client,
+			 const char *devname, const char *postfix)
+{}
+
+static inline void
+v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
+		     const struct v4l2_subdev_ops *ops)
+{}
+
+static inline unsigned short v4l2_i2c_subdev_addr(struct v4l2_subdev *sd)
+{
+	return I2C_CLIENT_END;
+}
+
+static inline const unsigned short *
+v4l2_i2c_tuner_addrs(enum v4l2_i2c_tuner_type type)
+{
+	return NULL;
+}
+
+#endif
+
 /* ------------------------------------------------------------------------- */
 
 /* SPI Helper functions */
-- 
2.22.0

[PATCH 5/6] media: v4l2-core: introduce a helper to unregister a SPI subdev

[Ezequiel Garcia]

Introduce a new video4linux2 SPI helper, to unregister a subdev.
This allows to get rid of some more ifdefs.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-device.c | 14 ++------------
 drivers/media/v4l2-core/v4l2-spi.c    |  8 ++++++++
 include/media/v4l2-common.h           |  9 +++++++++
 3 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index aa277f5bc862..c2811238996f 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -11,9 +11,6 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
-#if defined(CONFIG_SPI)
-#include <linux/spi/spi.h>
-#endif
 #include <linux/videodev2.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ctrls.h>
@@ -124,15 +121,8 @@ void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
 			continue;
 		}
 #endif
-#if defined(CONFIG_SPI)
-		if (sd->flags & V4L2_SUBDEV_FL_IS_SPI) {
-			struct spi_device *spi = v4l2_get_subdevdata(sd);
-
-			if (spi && !spi->dev.of_node && !spi->dev.fwnode)
-				spi_unregister_device(spi);
-			continue;
-		}
-#endif
+		else if (sd->flags & V4L2_SUBDEV_FL_IS_SPI)
+			v4l2_spi_subdev_unregister(sd);
 	}
 	/* Mark as unregistered, thus preventing duplicate unregistrations */
 	v4l2_dev->name[0] = '\0';
diff --git a/drivers/media/v4l2-core/v4l2-spi.c b/drivers/media/v4l2-core/v4l2-spi.c
index ab5a7eb4205d..2a7e82e1412d 100644
--- a/drivers/media/v4l2-core/v4l2-spi.c
+++ b/drivers/media/v4l2-core/v4l2-spi.c
@@ -8,6 +8,14 @@
 #include <media/v4l2-common.h>
 #include <media/v4l2-device.h>
 
+void v4l2_spi_subdev_unregister(struct v4l2_subdev *sd)
+{
+	struct spi_device *spi = v4l2_get_subdevdata(sd);
+
+	if (spi && !spi->dev.of_node && !spi->dev.fwnode)
+		spi_unregister_device(spi);
+}
+
 void v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi,
 		const struct v4l2_subdev_ops *ops)
 {
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 8e66edddd37b..e2878654d043 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -285,6 +285,13 @@ struct v4l2_subdev *v4l2_spi_new_subdev(struct v4l2_device *v4l2_dev,
 void v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi,
 		const struct v4l2_subdev_ops *ops);
 
+/**
+ * v4l2_spi_subdev_unregister - Unregister a v4l2_subdev
+ *
+ * @sd: pointer to &struct v4l2_subdev
+ */
+void v4l2_spi_subdev_unregister(struct v4l2_subdev *sd);
+
 #else
 
 static inline struct v4l2_subdev *
@@ -299,6 +306,8 @@ v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi,
 		     const struct v4l2_subdev_ops *ops)
 {}
 
+static inline void v4l2_spi_subdev_unregister(struct v4l2_subdev *sd)
+{}
 #endif
 
 /* ------------------------------------------------------------------------- */
-- 
2.22.0

[PATCH 5/6] media: v4l2-core: introduce an unregister spi subdev helper

[Ezequiel Garcia]

Introduce a new video4linux2 SPI helper, to unregister a subdev.
This allows to get rid of some more ifdefs.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-device.c | 14 ++------------
 drivers/media/v4l2-core/v4l2-spi.c    |  8 ++++++++
 include/media/v4l2-common.h           |  5 +++++
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index aa277f5bc862..c2811238996f 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -11,9 +11,6 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
-#if defined(CONFIG_SPI)
-#include <linux/spi/spi.h>
-#endif
 #include <linux/videodev2.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ctrls.h>
@@ -124,15 +121,8 @@ void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
 			continue;
 		}
 #endif
-#if defined(CONFIG_SPI)
-		if (sd->flags & V4L2_SUBDEV_FL_IS_SPI) {
-			struct spi_device *spi = v4l2_get_subdevdata(sd);
-
-			if (spi && !spi->dev.of_node && !spi->dev.fwnode)
-				spi_unregister_device(spi);
-			continue;
-		}
-#endif
+		else if (sd->flags & V4L2_SUBDEV_FL_IS_SPI)
+			v4l2_spi_subdev_unregister(sd);
 	}
 	/* Mark as unregistered, thus preventing duplicate unregistrations */
 	v4l2_dev->name[0] = '\0';
diff --git a/drivers/media/v4l2-core/v4l2-spi.c b/drivers/media/v4l2-core/v4l2-spi.c
index 11fd7a64e92f..9ecbb5479afa 100644
--- a/drivers/media/v4l2-core/v4l2-spi.c
+++ b/drivers/media/v4l2-core/v4l2-spi.c
@@ -8,6 +8,14 @@
 #include <media/v4l2-common.h>
 #include <media/v4l2-device.h>
 
+void v4l2_spi_subdev_unregister(struct v4l2_subdev *sd)
+{
+	struct spi_device *spi = v4l2_get_subdevdata(sd);
+
+	if (spi && !spi->dev.of_node && !spi->dev.fwnode)
+		spi_unregister_device(spi);
+}
+
 void v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi,
 		const struct v4l2_subdev_ops *ops)
 {
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index c0dc7e2edb6f..04a748311a7b 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -249,6 +249,11 @@ struct v4l2_subdev *v4l2_spi_new_subdev(struct v4l2_device *v4l2_dev,
  */
 void v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi,
 		const struct v4l2_subdev_ops *ops);
+
+void v4l2_spi_subdev_unregister(struct v4l2_subdev *sd);
+#else
+static inline void v4l2_spi_subdev_unregister(struct v4l2_subdev *sd)
+{}
 #endif
 
 /* ------------------------------------------------------------------------- */
-- 
2.22.0

[PATCH 6/6] media: v4l2-core: introduce a helper to unregister a I2C subdev

[Ezequiel Garcia]

Introduce a new video4linux2 I2C helper, to unregister a subdev.
This allows to get rid of yet another ifdefs.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-device.c | 25 ++-----------------------
 drivers/media/v4l2-core/v4l2-i2c.c    | 20 ++++++++++++++++++++
 include/media/v4l2-common.h           | 10 ++++++++++
 3 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index c2811238996f..63d6b147b21e 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -9,7 +9,6 @@
 #include <linux/types.h>
 #include <linux/ioctl.h>
 #include <linux/module.h>
-#include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/videodev2.h>
 #include <media/v4l2-device.h>
@@ -99,28 +98,8 @@ void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
 	/* Unregister subdevs */
 	list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list) {
 		v4l2_device_unregister_subdev(sd);
-#if IS_ENABLED(CONFIG_I2C)
-		if (sd->flags & V4L2_SUBDEV_FL_IS_I2C) {
-			struct i2c_client *client = v4l2_get_subdevdata(sd);
-
-			/*
-			 * We need to unregister the i2c client
-			 * explicitly. We cannot rely on
-			 * i2c_del_adapter to always unregister
-			 * clients for us, since if the i2c bus is a
-			 * platform bus, then it is never deleted.
-			 *
-			 * Device tree or ACPI based devices must not
-			 * be unregistered as they have not been
-			 * registered by us, and would not be
-			 * re-created by just probing the V4L2 driver.
-			 */
-			if (client &&
-			    !client->dev.of_node && !client->dev.fwnode)
-				i2c_unregister_device(client);
-			continue;
-		}
-#endif
+		if (sd->flags & V4L2_SUBDEV_FL_IS_I2C)
+			v4l2_i2c_subdev_unregister(sd);
 		else if (sd->flags & V4L2_SUBDEV_FL_IS_SPI)
 			v4l2_spi_subdev_unregister(sd);
 	}
diff --git a/drivers/media/v4l2-core/v4l2-i2c.c b/drivers/media/v4l2-core/v4l2-i2c.c
index f393dd4f1c00..3d7a3081ec0b 100644
--- a/drivers/media/v4l2-core/v4l2-i2c.c
+++ b/drivers/media/v4l2-core/v4l2-i2c.c
@@ -8,6 +8,26 @@
 #include <media/v4l2-common.h>
 #include <media/v4l2-device.h>
 
+void v4l2_i2c_subdev_unregister(struct v4l2_subdev *sd)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	/*
+	 * We need to unregister the i2c client
+	 * explicitly. We cannot rely on
+	 * i2c_del_adapter to always unregister
+	 * clients for us, since if the i2c bus is a
+	 * platform bus, then it is never deleted.
+	 *
+	 * Device tree or ACPI based devices must not
+	 * be unregistered as they have not been
+	 * registered by us, and would not be
+	 * re-created by just probing the V4L2 driver.
+	 */
+	 if (client && !client->dev.of_node && !client->dev.fwnode)
+		i2c_unregister_device(client);
+}
+
 void v4l2_i2c_subdev_set_name(struct v4l2_subdev *sd, struct i2c_client *client,
 			      const char *devname, const char *postfix)
 {
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index e2878654d043..c070d8ae11e5 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -211,6 +211,13 @@ unsigned short v4l2_i2c_subdev_addr(struct v4l2_subdev *sd);
  */
 const unsigned short *v4l2_i2c_tuner_addrs(enum v4l2_i2c_tuner_type type);
 
+/**
+ * v4l2_i2c_subdev_unregister - Unregister a v4l2_subdev
+ *
+ * @sd: pointer to &struct v4l2_subdev
+ */
+void v4l2_i2c_subdev_unregister(struct v4l2_subdev *sd);
+
 #else
 
 static inline struct v4l2_subdev *
@@ -250,6 +257,9 @@ v4l2_i2c_tuner_addrs(enum v4l2_i2c_tuner_type type)
 	return NULL;
 }
 
+static inline void v4l2_i2c_subdev_unregister(struct v4l2_subdev *sd)
+{}
+
 #endif
 
 /* ------------------------------------------------------------------------- */
-- 
2.22.0

[PATCH 6/6] media: v4l2-core: introduce unregister subdev i2c helper

[Ezequiel Garcia]

Introduce a new video4linux2 I2C helper, to unregister a subdev.
This allows to get rid of yet another ifdefs.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-device.c | 25 ++-----------------------
 drivers/media/v4l2-core/v4l2-i2c.c    | 20 ++++++++++++++++++++
 include/media/v4l2-common.h           |  4 ++++
 3 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index c2811238996f..63d6b147b21e 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -9,7 +9,6 @@
 #include <linux/types.h>
 #include <linux/ioctl.h>
 #include <linux/module.h>
-#include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/videodev2.h>
 #include <media/v4l2-device.h>
@@ -99,28 +98,8 @@ void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
 	/* Unregister subdevs */
 	list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list) {
 		v4l2_device_unregister_subdev(sd);
-#if IS_ENABLED(CONFIG_I2C)
-		if (sd->flags & V4L2_SUBDEV_FL_IS_I2C) {
-			struct i2c_client *client = v4l2_get_subdevdata(sd);
-
-			/*
-			 * We need to unregister the i2c client
-			 * explicitly. We cannot rely on
-			 * i2c_del_adapter to always unregister
-			 * clients for us, since if the i2c bus is a
-			 * platform bus, then it is never deleted.
-			 *
-			 * Device tree or ACPI based devices must not
-			 * be unregistered as they have not been
-			 * registered by us, and would not be
-			 * re-created by just probing the V4L2 driver.
-			 */
-			if (client &&
-			    !client->dev.of_node && !client->dev.fwnode)
-				i2c_unregister_device(client);
-			continue;
-		}
-#endif
+		if (sd->flags & V4L2_SUBDEV_FL_IS_I2C)
+			v4l2_i2c_subdev_unregister(sd);
 		else if (sd->flags & V4L2_SUBDEV_FL_IS_SPI)
 			v4l2_spi_subdev_unregister(sd);
 	}
diff --git a/drivers/media/v4l2-core/v4l2-i2c.c b/drivers/media/v4l2-core/v4l2-i2c.c
index f9853cc823df..b2cc8d727a60 100644
--- a/drivers/media/v4l2-core/v4l2-i2c.c
+++ b/drivers/media/v4l2-core/v4l2-i2c.c
@@ -8,6 +8,26 @@
 #include <media/v4l2-common.h>
 #include <media/v4l2-device.h>
 
+void v4l2_i2c_subdev_unregister(struct v4l2_subdev *sd)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	/*
+	 * We need to unregister the i2c client
+	 * explicitly. We cannot rely on
+	 * i2c_del_adapter to always unregister
+	 * clients for us, since if the i2c bus is a
+	 * platform bus, then it is never deleted.
+	 *
+	 * Device tree or ACPI based devices must not
+	 * be unregistered as they have not been
+	 * registered by us, and would not be
+	 * re-created by just probing the V4L2 driver.
+	 */
+	 if (client && !client->dev.of_node && !client->dev.fwnode)
+		i2c_unregister_device(client);
+}
+
 void v4l2_i2c_subdev_set_name(struct v4l2_subdev *sd, struct i2c_client *client,
 			      const char *devname, const char *postfix)
 {
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 04a748311a7b..c38d55d16230 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -214,6 +214,10 @@ enum v4l2_i2c_tuner_type {
  */
 const unsigned short *v4l2_i2c_tuner_addrs(enum v4l2_i2c_tuner_type type);
 
+void v4l2_i2c_subdev_unregister(struct v4l2_subdev *sd);
+#else
+static inline void v4l2_i2c_subdev_unregister(struct v4l2_subdev *sd)
+{}
 #endif
 
 /* ------------------------------------------------------------------------- */
-- 
2.22.0

Re: [PATCH 0/6] V4L2 core I2C/SPI code cleanup

[Hans Verkuil]

On 7/15/19 11:06 PM, Ezequiel Garcia wrote:
> Hi Mauro, Hans:
> 
> While reading v4l2-common.c, I came across a few ifdefs
> that could be cleaned-up with some minor reorganization.
> 
> Patch 1 is just cleaning the Makefile, removing ifeq/endif
> to make it more readable.
> 
> Patch 2 merges v4l2-common.ko into videodev.ko, which
> I think it's now possible. Let me know if having
> these two modules separated serves a purpose
> I'm missing.
> 
> The rest of the patches split the I2C and SPI helpers,
> so they can be conditionally built.
> 
> There are a few checkpatch.pl issues triggered here,
> all of them belonging to the already existing code.
> Let me know if you want me to clean that as well.
> 
> The entire series should not affect any functionality,
> but just clean-up the code a bit.

This series looks good, but since you are looking in this
area anyway, can you post a follow-up patch that replaces the
BUG_ON in v4l2-i2c.c and -spi.c with a WARN_ON?

Thanks!

	Hans

> 
> Thanks,
> Eze
> 
> Ezequiel Garcia (6):
>   media: v4l2-core: Cleanup Makefile
>   media: v4l2-core: Module re-organization
>   media: v4l2-core: move spi helpers out of v4l2-common.c
>   media: v4l2-core: move i2c helpers out of v4l2-common.c
>   media: v4l2-core: introduce a helper to unregister a SPI subdev
>   media: v4l2-core: introduce a helper to unregister a I2C subdev
> 
>  drivers/media/v4l2-core/Kconfig       |   5 +
>  drivers/media/v4l2-core/Makefile      |  15 +-
>  drivers/media/v4l2-core/v4l2-common.c | 210 --------------------------
>  drivers/media/v4l2-core/v4l2-device.c |  39 +----
>  drivers/media/v4l2-core/v4l2-i2c.c    | 167 ++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-spi.c    |  73 +++++++++
>  include/media/v4l2-common.h           | 150 +++++++++++++-----
>  7 files changed, 366 insertions(+), 293 deletions(-)
>  create mode 100644 drivers/media/v4l2-core/v4l2-i2c.c
>  create mode 100644 drivers/media/v4l2-core/v4l2-spi.c
> 

Re: [PATCH 2/6] media: v4l2-core: Module re-organization

[Mauro Carvalho Chehab]

Em Mon, 15 Jul 2019 18:06:43 -0300
Ezequiel Garcia <ezequiel@collabora.com> escreveu:

> videodev.ko and v4l2-common.ko driver are built under
> the same conditions. Therefore, it doesn't make much sense
> to split them in two different modules.
> 
> Splitting v4l2-common to its own driver has done many
> years ago:
> 
>   commit a9254475bbfbed5f0596d952c6a3c9806e19dd0b
>   Author: Mauro Carvalho Chehab <mchehab@infradead.org>
>   Date:   Tue Jan 29 18:32:35 2008 -0300
> 
>       V4L/DVB (7115): Fix bug #9833: regression when compiling V4L without I2C
> 
> Back then, the subsystem organization was different.
> However, With the current organization, there is no issue
> compiling V4L2 with I2C as y/m/n.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/media/v4l2-core/Makefile | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> index 4d42418e603e..8e2f52f7800b 100644
> --- a/drivers/media/v4l2-core/Makefile
> +++ b/drivers/media/v4l2-core/Makefile
> @@ -7,14 +7,13 @@ 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-clk.o \
> -			v4l2-async.o
> +			v4l2-async.o v4l2-common.o
>  videodev-$(CONFIG_COMPAT) += v4l2-compat-ioctl32.o
>  videodev-$(CONFIG_TRACEPOINTS) += v4l2-trace.o
>  videodev-$(CONFIG_MEDIA_CONTROLLER) += v4l2-mc.o
>  
>  obj-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
>  obj-$(CONFIG_VIDEO_V4L2) += videodev.o
> -obj-$(CONFIG_VIDEO_V4L2) += v4l2-common.o
>  obj-$(CONFIG_VIDEO_V4L2) += v4l2-dv-timings.o
>  
>  obj-$(CONFIG_VIDEO_TUNER) += tuner.o


Huh? This patch sounds incomplete... Where are you removing the
MODULE_foo macros) from v4l2-common.c?


Thanks,
Mauro

Re: [PATCH 2/6] media: v4l2-core: Module re-organization

[Ezequiel Garcia]

On Thu, 2019-07-25 at 13:31 -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 15 Jul 2019 18:06:43 -0300
> Ezequiel Garcia <ezequiel@collabora.com> escreveu:
> 
> > videodev.ko and v4l2-common.ko driver are built under
> > the same conditions. Therefore, it doesn't make much sense
> > to split them in two different modules.
> > 
> > Splitting v4l2-common to its own driver has done many
> > years ago:
> > 
> >   commit a9254475bbfbed5f0596d952c6a3c9806e19dd0b
> >   Author: Mauro Carvalho Chehab <mchehab@infradead.org>
> >   Date:   Tue Jan 29 18:32:35 2008 -0300
> > 
> >       V4L/DVB (7115): Fix bug #9833: regression when compiling V4L without I2C
> > 
> > Back then, the subsystem organization was different.
> > However, With the current organization, there is no issue
> > compiling V4L2 with I2C as y/m/n.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/media/v4l2-core/Makefile | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> > index 4d42418e603e..8e2f52f7800b 100644
> > --- a/drivers/media/v4l2-core/Makefile
> > +++ b/drivers/media/v4l2-core/Makefile
> > @@ -7,14 +7,13 @@ 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-clk.o \
> > -			v4l2-async.o
> > +			v4l2-async.o v4l2-common.o
> >  videodev-$(CONFIG_COMPAT) += v4l2-compat-ioctl32.o
> >  videodev-$(CONFIG_TRACEPOINTS) += v4l2-trace.o
> >  videodev-$(CONFIG_MEDIA_CONTROLLER) += v4l2-mc.o
> >  
> >  obj-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
> >  obj-$(CONFIG_VIDEO_V4L2) += videodev.o
> > -obj-$(CONFIG_VIDEO_V4L2) += v4l2-common.o
> >  obj-$(CONFIG_VIDEO_V4L2) += v4l2-dv-timings.o
> >  
> >  obj-$(CONFIG_VIDEO_TUNER) += tuner.o
> 
> Huh? This patch sounds incomplete... Where are you removing the
> MODULE_foo macros) from v4l2-common.c?
> 

If you are refering to:

MODULE_AUTHOR("Bill Dirks, Justin Schoeman, Gerd Knorr");
MODULE_DESCRIPTION("misc helper functions for v4l2 device drivers");
MODULE_LICENSE("GPL");

I decided to keep them for documentation purposes,
but more importantly because they were already there
before commit a9254475bbfbed5f0596d952c6a3c9806e19dd0b
(when v4l2-common.c was not a module, afaics).

Do you think we should remove them?

Thanks,
Ezequiel

Re: [PATCH 2/6] media: v4l2-core: Module re-organization

[Mauro Carvalho Chehab]

Em Thu, 25 Jul 2019 13:41:34 -0300
Ezequiel Garcia <ezequiel@collabora.com> escreveu:

> On Thu, 2019-07-25 at 13:31 -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 15 Jul 2019 18:06:43 -0300
> > Ezequiel Garcia <ezequiel@collabora.com> escreveu:
> >   
> > > videodev.ko and v4l2-common.ko driver are built under
> > > the same conditions. Therefore, it doesn't make much sense
> > > to split them in two different modules.
> > > 
> > > Splitting v4l2-common to its own driver has done many
> > > years ago:
> > > 
> > >   commit a9254475bbfbed5f0596d952c6a3c9806e19dd0b
> > >   Author: Mauro Carvalho Chehab <mchehab@infradead.org>
> > >   Date:   Tue Jan 29 18:32:35 2008 -0300
> > > 
> > >       V4L/DVB (7115): Fix bug #9833: regression when compiling V4L without I2C
> > > 
> > > Back then, the subsystem organization was different.
> > > However, With the current organization, there is no issue
> > > compiling V4L2 with I2C as y/m/n.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > ---
> > >  drivers/media/v4l2-core/Makefile | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> > > index 4d42418e603e..8e2f52f7800b 100644
> > > --- a/drivers/media/v4l2-core/Makefile
> > > +++ b/drivers/media/v4l2-core/Makefile
> > > @@ -7,14 +7,13 @@ 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-clk.o \
> > > -			v4l2-async.o
> > > +			v4l2-async.o v4l2-common.o
> > >  videodev-$(CONFIG_COMPAT) += v4l2-compat-ioctl32.o
> > >  videodev-$(CONFIG_TRACEPOINTS) += v4l2-trace.o
> > >  videodev-$(CONFIG_MEDIA_CONTROLLER) += v4l2-mc.o
> > >  
> > >  obj-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
> > >  obj-$(CONFIG_VIDEO_V4L2) += videodev.o
> > > -obj-$(CONFIG_VIDEO_V4L2) += v4l2-common.o
> > >  obj-$(CONFIG_VIDEO_V4L2) += v4l2-dv-timings.o
> > >  
> > >  obj-$(CONFIG_VIDEO_TUNER) += tuner.o  
> > 
> > Huh? This patch sounds incomplete... Where are you removing the
> > MODULE_foo macros) from v4l2-common.c?
> >   
> 
> If you are refering to:
> 
> MODULE_AUTHOR("Bill Dirks, Justin Schoeman, Gerd Knorr");
> MODULE_DESCRIPTION("misc helper functions for v4l2 device drivers");
> MODULE_LICENSE("GPL");
> 
> I decided to keep them for documentation purposes,
> but more importantly because they were already there
> before commit a9254475bbfbed5f0596d952c6a3c9806e19dd0b
> (when v4l2-common.c was not a module, afaics).
> 
> Do you think we should remove them?

Well, it needs to be handled somehow. Please notice, however, that
there are some differences between v4l2-common and v4l2-dev:

drivers/media/v4l2-core/v4l2-common.c:MODULE_AUTHOR("Bill Dirks, Justin Schoeman, Gerd Knorr");
drivers/media/v4l2-core/v4l2-common.c:MODULE_DESCRIPTION("misc helper functions for v4l2 device drivers");
drivers/media/v4l2-core/v4l2-common.c:MODULE_LICENSE("GPL");
drivers/media/v4l2-core/v4l2-common.c:	request_module(I2C_MODULE_PREFIX "%s", info->type);
drivers/media/v4l2-core/v4l2-dev.c:MODULE_AUTHOR("Alan Cox, Mauro Carvalho Chehab <mchehab@kernel.org>");
drivers/media/v4l2-core/v4l2-dev.c:MODULE_DESCRIPTION("Device registrar for Video4Linux drivers v2");
drivers/media/v4l2-core/v4l2-dev.c:MODULE_LICENSE("GPL");
drivers/media/v4l2-core/v4l2-dev.c:MODULE_ALIAS_CHARDEV_MAJOR(VIDEO_MAJOR);

So, you can't simply remove. You'll probably need to touch
both files, do some cleanup and keep some things under the
licensing comment at the top of the file (if not there yet).

Thanks,
Mauro

Re: [PATCH 2/6] media: v4l2-core: Module re-organization

[Ezequiel Garcia]

On Thu, 2019-07-25 at 14:03 -0300, Mauro Carvalho Chehab wrote:
> Em Thu, 25 Jul 2019 13:41:34 -0300
> Ezequiel Garcia <ezequiel@collabora.com> escreveu:
> 
> > On Thu, 2019-07-25 at 13:31 -0300, Mauro Carvalho Chehab wrote:
> > > Em Mon, 15 Jul 2019 18:06:43 -0300
> > > Ezequiel Garcia <ezequiel@collabora.com> escreveu:
> > >   
> > > > videodev.ko and v4l2-common.ko driver are built under
> > > > the same conditions. Therefore, it doesn't make much sense
> > > > to split them in two different modules.
> > > > 
> > > > Splitting v4l2-common to its own driver has done many
> > > > years ago:
> > > > 
> > > >   commit a9254475bbfbed5f0596d952c6a3c9806e19dd0b
> > > >   Author: Mauro Carvalho Chehab <mchehab@infradead.org>
> > > >   Date:   Tue Jan 29 18:32:35 2008 -0300
> > > > 
> > > >       V4L/DVB (7115): Fix bug #9833: regression when compiling V4L without I2C
> > > > 
> > > > Back then, the subsystem organization was different.
> > > > However, With the current organization, there is no issue
> > > > compiling V4L2 with I2C as y/m/n.
> > > > 
> > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > > ---
> > > >  drivers/media/v4l2-core/Makefile | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> > > > index 4d42418e603e..8e2f52f7800b 100644
> > > > --- a/drivers/media/v4l2-core/Makefile
> > > > +++ b/drivers/media/v4l2-core/Makefile
> > > > @@ -7,14 +7,13 @@ 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-clk.o \
> > > > -			v4l2-async.o
> > > > +			v4l2-async.o v4l2-common.o
> > > >  videodev-$(CONFIG_COMPAT) += v4l2-compat-ioctl32.o
> > > >  videodev-$(CONFIG_TRACEPOINTS) += v4l2-trace.o
> > > >  videodev-$(CONFIG_MEDIA_CONTROLLER) += v4l2-mc.o
> > > >  
> > > >  obj-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
> > > >  obj-$(CONFIG_VIDEO_V4L2) += videodev.o
> > > > -obj-$(CONFIG_VIDEO_V4L2) += v4l2-common.o
> > > >  obj-$(CONFIG_VIDEO_V4L2) += v4l2-dv-timings.o
> > > >  
> > > >  obj-$(CONFIG_VIDEO_TUNER) += tuner.o  
> > > 
> > > Huh? This patch sounds incomplete... Where are you removing the
> > > MODULE_foo macros) from v4l2-common.c?
> > >   
> > 
> > If you are refering to:
> > 
> > MODULE_AUTHOR("Bill Dirks, Justin Schoeman, Gerd Knorr");
> > MODULE_DESCRIPTION("misc helper functions for v4l2 device drivers");
> > MODULE_LICENSE("GPL");
> > 
> > I decided to keep them for documentation purposes,
> > but more importantly because they were already there
> > before commit a9254475bbfbed5f0596d952c6a3c9806e19dd0b
> > (when v4l2-common.c was not a module, afaics).
> > 
> > Do you think we should remove them?
> 
> Well, it needs to be handled somehow. Please notice, however, that
> there are some differences between v4l2-common and v4l2-dev:
> 
> drivers/media/v4l2-core/v4l2-common.c:MODULE_AUTHOR("Bill Dirks, Justin Schoeman, Gerd Knorr");
> drivers/media/v4l2-core/v4l2-common.c:MODULE_DESCRIPTION("misc helper functions for v4l2 device drivers");
> drivers/media/v4l2-core/v4l2-common.c:MODULE_LICENSE("GPL");
> drivers/media/v4l2-core/v4l2-common.c:	request_module(I2C_MODULE_PREFIX "%s", info->type);
> drivers/media/v4l2-core/v4l2-dev.c:MODULE_AUTHOR("Alan Cox, Mauro Carvalho Chehab <mchehab@kernel.org>");
> drivers/media/v4l2-core/v4l2-dev.c:MODULE_DESCRIPTION("Device registrar for Video4Linux drivers v2");
> drivers/media/v4l2-core/v4l2-dev.c:MODULE_LICENSE("GPL");
> drivers/media/v4l2-core/v4l2-dev.c:MODULE_ALIAS_CHARDEV_MAJOR(VIDEO_MAJOR);
> 
> So, you can't simply remove. You'll probably need to touch
> both files, do some cleanup and keep some things under the
> licensing comment at the top of the file (if not there yet).
> 

OK, I'll rebase and submit a new version trying to tackle this.

Thanks,
Ezequiel