Skip to contentSkip to conversation

V4L2 core I2C/SPI code cleanup

PATCH Superseded Ezequiel Garcia 3 participants 14 messages Jul 15, 2019 – Jul 25, 2019 Media (V4L/DVB)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover V4L2 core I2C/SPI code cleanup

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
Signature · 1 line
2.22.0
#1 lore raw

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

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

---
 drivers/media/v4l2-core/Makefile | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)
drivers/media/v4l2-core/Makefile +3 -7
index 9ee57e1efefe..4d42418e603e 100644
@@ -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
Signature · 1 line
2.22.0
#2 lore raw

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

In reply to 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 <[email protected]>
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.

---
 drivers/media/v4l2-core/Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/media/v4l2-core/Makefile +1 -2
index 4d42418e603e..8e2f52f7800b 100644
@@ -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
Signature · 1 line
2.22.0
#3 lore raw

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

Em Mon, 15 Jul 2019 18:06:43 -0300 Ezequiel Garcia <[email protected]> escreveu:

Quoted 32 lines

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 <[email protected]>
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.

---
 drivers/media/v4l2-core/Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/media/v4l2-core/Makefile +1 -1
index 4d42418e603e..8e2f52f7800b 100644
@@ -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

drivers/media/v4l2-core/Makefile tuner-objs := tuner-core.o · line 19

Quoted 10 lines
drivers/media/v4l2-core/Makefile +0 -1
 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

#4 lore raw

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

Quoted 44 lines · Mauro Carvalho Chehab

Em Mon, 15 Jul 2019 18:06:43 -0300 Ezequiel Garcia <[email protected]> escreveu:

Quoted 41 lines

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 <[email protected]>
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.

---
 drivers/media/v4l2-core/Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/media/v4l2-core/Makefile +1 -2
index 4d42418e603e..8e2f52f7800b 100644
@@ -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
 
Quoted 5 lines

drivers/media/v4l2-core/Makefile tuner-objs := tuner-core.o · line 19

Quoted 1 line
drivers/media/v4l2-core/Makefile +0 -0
 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

#5 lore raw

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

Em Thu, 25 Jul 2019 13:41:34 -0300 Ezequiel Garcia <[email protected]> escreveu:

Quoted 57 lines · Mauro Carvalho Chehab
Quoted 44 lines

Em Mon, 15 Jul 2019 18:06:43 -0300 Ezequiel Garcia <[email protected]> escreveu:

Quoted 41 lines

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 <[email protected]>
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.

---
 drivers/media/v4l2-core/Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/media/v4l2-core/Makefile +1 -2
index 4d42418e603e..8e2f52f7800b 100644
@@ -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
 
Quoted 5 lines

drivers/media/v4l2-core/Makefile tuner-objs := tuner-core.o · line 19

Quoted 1 line
drivers/media/v4l2-core/Makefile +0 -0
 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");

Quoted 6 lines

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 <[email protected]>"); 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

#6 lore raw

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

Quoted 79 lines · Mauro Carvalho Chehab

Em Thu, 25 Jul 2019 13:41:34 -0300 Ezequiel Garcia <[email protected]> escreveu:

Quoted 57 lines · Mauro Carvalho Chehab
Quoted 44 lines

Em Mon, 15 Jul 2019 18:06:43 -0300 Ezequiel Garcia <[email protected]> escreveu:

Quoted 41 lines

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 <[email protected]>
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.

---
 drivers/media/v4l2-core/Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/media/v4l2-core/Makefile +1 -2
index 4d42418e603e..8e2f52f7800b 100644
@@ -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
 
Quoted 5 lines

drivers/media/v4l2-core/Makefile tuner-objs := tuner-core.o · line 19

Quoted 1 line
drivers/media/v4l2-core/Makefile +0 -0
 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");

Quoted 6 lines

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 <[email protected]>"); 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);

Quoted 4 lines

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

#7 lore raw

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

In reply to 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.

---
 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
drivers/media/v4l2-core/Makefile +1 -0
index 8e2f52f7800b..2deeeac6ee76 100644
@@ -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
drivers/media/v4l2-core/v4l2-common.c +0 -65

77 lines not on this page · raw message

drivers/media/v4l2-core/v4l2-spi.c new file +65 -0

65 lines not on this page · raw message

include/media/v4l2-common.h +16 -2
index 6b319d0d73ad..a1c5288caa6a 100644
@@ -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
 
 /* ------------------------------------------------------------------------- */
Signature · 1 line
2.22.0
#8 lore raw

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

In reply to 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.

---
 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
drivers/media/v4l2-core/Kconfig +5 -0
index 7c5f62f196e5..39e3fb30ba0b 100644
@@ -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
drivers/media/v4l2-core/Makefile +1 -0
index 2deeeac6ee76..786bd1ec4d1b 100644
@@ -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
drivers/media/v4l2-core/v4l2-common.c +0 -145

157 lines not on this page · raw message

drivers/media/v4l2-core/v4l2-i2c.c new file +147 -0

147 lines not on this page · raw message

include/media/v4l2-common.h +76 -37

141 lines not on this page · raw message

Signature · 1 line
2.22.0
#9 lore raw

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

In reply to Ezequiel Garcia

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

---
 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(-)
drivers/media/v4l2-core/v4l2-device.c +2 -12
index aa277f5bc862..c2811238996f 100644
@@ -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';
drivers/media/v4l2-core/v4l2-spi.c +8 -0
index ab5a7eb4205d..2a7e82e1412d 100644
@@ -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)
 {
include/media/v4l2-common.h +9 -0
index 8e66edddd37b..e2878654d043 100644
@@ -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
 
 /* ------------------------------------------------------------------------- */
Signature · 1 line
2.22.0
#10 lore raw

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

In reply to Ezequiel Garcia

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

---
 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(-)
drivers/media/v4l2-core/v4l2-device.c +2 -12
index aa277f5bc862..c2811238996f 100644
@@ -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';
drivers/media/v4l2-core/v4l2-spi.c +8 -0
index 11fd7a64e92f..9ecbb5479afa 100644
@@ -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)
 {
include/media/v4l2-common.h +5 -0
index c0dc7e2edb6f..04a748311a7b 100644
@@ -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
 
 /* ------------------------------------------------------------------------- */
Signature · 1 line
2.22.0
#11 lore raw

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

In reply to Ezequiel Garcia

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

---
 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(-)
drivers/media/v4l2-core/v4l2-device.c +2 -23
index c2811238996f..63d6b147b21e 100644
@@ -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);
 	}
drivers/media/v4l2-core/v4l2-i2c.c +20 -0
index f393dd4f1c00..3d7a3081ec0b 100644
@@ -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)
 {
include/media/v4l2-common.h +10 -0
index e2878654d043..c070d8ae11e5 100644
@@ -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
 
 /* ------------------------------------------------------------------------- */
Signature · 1 line
2.22.0
#12 lore raw

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

In reply to Ezequiel Garcia

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

---
 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(-)
drivers/media/v4l2-core/v4l2-device.c +2 -23
index c2811238996f..63d6b147b21e 100644
@@ -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);
 	}
drivers/media/v4l2-core/v4l2-i2c.c +20 -0
index f9853cc823df..b2cc8d727a60 100644
@@ -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)
 {
include/media/v4l2-common.h +4 -0
index 04a748311a7b..c38d55d16230 100644
@@ -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
 
 /* ------------------------------------------------------------------------- */
Signature · 1 line
2.22.0
#13 lore raw
In reply to Ezequiel Garcia
Quoted 16 lines · 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.

Quoted 6 lines

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

Quoted 23 lines

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