Skip to contentSkip to conversation

video: build fix for drivers/media/video/mt9v022.c

PATCH In review Ingo Molnar 2 participants 6 messages May 3, 2008 – May 5, 2008 Media (V4L/DVB)

lore
Share & tools
lore

Ingo Molnar

author
#0 lore raw

PATCH 1/1 video: build fix for drivers/media/video/mt9v022.c

x86.git testing found the following build bug on latest -git:

CC [M] drivers/media/video/mt9v022.o
drivers/media/video/mt9v022.c: In function 'bus_switch_request':
drivers/media/video/mt9v022.c:199: error: implicit declaration of function 'gpio_is_valid' drivers/media/video/mt9v022.c:201: error: implicit declaration of function 'gpio_request' drivers/media/video/mt9v022.c:207: error: implicit declaration of function 'gpio_direction_output' drivers/media/video/mt9v022.c:211: error: implicit declaration of function 'gpio_free' drivers/media/video/mt9v022.c: In function 'bus_switch_act':
drivers/media/video/mt9v022.c:237: error: implicit declaration of function 'gpio_set_value_cansleep' make[2]: *** [drivers/media/video] Error 2
make[1]: *** [drivers/media] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [drivers] Error 2

with this config:

http://redhat.com/~mingo/misc/config-Sat_May__3_16_08_39_CEST_2008.bad

the bug was that the driver uses GPIO functionality but only includes the GPIO interface definitions for the CONFIG_MT9M001_PCA9536_SWITCH case, which was not set in this config.

The quick fix seems to be to include linux/gpio.h unconditionally. (this seems like a small cleanup as well as it removes and #ifdef is more robust than an inclusion of asm/gpio.h) Not tested too much yet, so please have another look in any case.

Signed-off-by Ingo Molnar <[email protected]>
---
 drivers/media/video/mt9v022.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/media/video/mt9v022.c +1 -4
Index: linux/drivers/media/video/mt9v022.c
===================================================================
@@ -13,15 +13,12 @@
 #include <linux/i2c.h>
 #include <linux/delay.h>
 #include <linux/log2.h>
+#include <linux/gpio.h>
 
 #include <media/v4l2-common.h>
 #include <media/v4l2-chip-ident.h>
 #include <media/soc_camera.h>
 
-#ifdef CONFIG_MT9M001_PCA9536_SWITCH
-#include <asm/gpio.h>
-#endif
-
 /* mt9v022 i2c address 0x48, 0x4c, 0x58, 0x5c
  * The platform has to define i2c_board_info
  * and call i2c_register_board_info() */
#1 lore raw
Quoted 16 lines · Ingo Molnar

x86.git testing found the following build bug on latest -git:

CC [M] drivers/media/video/mt9v022.o
drivers/media/video/mt9v022.c: In function 'bus_switch_request':
drivers/media/video/mt9v022.c:199: error: implicit declaration of function 'gpio_is_valid' drivers/media/video/mt9v022.c:201: error: implicit declaration of function 'gpio_request' drivers/media/video/mt9v022.c:207: error: implicit declaration of function 'gpio_direction_output' drivers/media/video/mt9v022.c:211: error: implicit declaration of function 'gpio_free' drivers/media/video/mt9v022.c: In function 'bus_switch_act':
drivers/media/video/mt9v022.c:237: error: implicit declaration of function 'gpio_set_value_cansleep' make[2]: *** [drivers/media/video] Error 2
make[1]: *** [drivers/media] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [drivers] Error 2

Quoted 7 lines

with this config:

   http://redhat.com/~mingo/misc/config-Sat_May__3_16_08_39_CEST_2008.bad

the bug was that the driver uses GPIO functionality but only includes the GPIO interface definitions for the CONFIG_MT9M001_PCA9536_SWITCH case, which was not set in this config.

Ok, once again a good catch and a wrong fix, sorry:-) The bug is that not CONFIG_MT9M001_PCA9536_SWITCH but CONFIG_MT9V022_PCA9536_SWITCH shall be checked for, of course. Copy-paste:-( I'll prepare a correct patch and submit it. As for the "cleanup" side - don't know. Would it be better to unconditionally include it? It won't hurt of course, looks better, but is unneeded when the GPIO is not used. And, although grep reports most drivers including asm/gpio.h, including linux/gpio.h seems indeed better.

Thanks
Guennadi
---
Guennadi Liakhovetski

author
#2 lore raw
Quoted 9 lines · Guennadi Liakhovetski
Quoted 8 lines

with this config:

   http://redhat.com/~mingo/misc/config-Sat_May__3_16_08_39_CEST_2008.bad

the bug was that the driver uses GPIO functionality but only includes the GPIO interface definitions for the CONFIG_MT9M001_PCA9536_SWITCH case, which was not set in this config.

Quoted 8 lines

Ok, once again a good catch and a wrong fix, sorry :-) The bug is that not CONFIG_MT9M001_PCA9536_SWITCH but CONFIG_MT9V022_PCA9536_SWITCH shall be checked for, of course. Copy-paste:-( I'll prepare a correct patch and submit it. As for the "cleanup" side - don't know. Would it be better to unconditionally include it? It won't hurt of course, looks better, but is unneeded when the GPIO is not used. And, although grep reports most drivers including asm/gpio.h, including linux/gpio.h seems indeed better.

No. If you look at the core kernel you wont see it sprinkled with things like:

#ifdef CONFIG_LOCKDEP
#include <linux/lockdep.h>
#endif

why? Because we have put all such things into the include file themselves and do not want to sprinkle .c files with ugly #ifdefs. This is a basic cleanliness issue.

Furthermore, if anyone _else_ copy & pastes your driver, this bug wont hit him anymore.

so please keep that #include <linux/gpio.h>.

Ingo

#3 lore raw
Quoted 23 lines · Ingo Molnar
Quoted 9 lines · Guennadi Liakhovetski
Quoted 8 lines

with this config:

   http://redhat.com/~mingo/misc/config-Sat_May__3_16_08_39_CEST_2008.bad

the bug was that the driver uses GPIO functionality but only includes the GPIO interface definitions for the CONFIG_MT9M001_PCA9536_SWITCH case, which was not set in this config.

Quoted 8 lines

Ok, once again a good catch and a wrong fix, sorry :-) The bug is that not CONFIG_MT9M001_PCA9536_SWITCH but CONFIG_MT9V022_PCA9536_SWITCH shall be checked for, of course. Copy-paste:-( I'll prepare a correct patch and submit it. As for the "cleanup" side - don't know. Would it be better to unconditionally include it? It won't hurt of course, looks better, but is unneeded when the GPIO is not used. And, although grep reports most drivers including asm/gpio.h, including linux/gpio.h seems indeed better.

No. If you look at the core kernel you wont see it sprinkled with things like:

Quoted 7 lines

#ifdef CONFIG_LOCKDEP
#include <linux/lockdep.h>
#endif

why? Because we have put all such things into the include file themselves and do not want to sprinkle .c files with ugly #ifdefs. This is a basic cleanliness issue.

Ok, right, sorry, the patch was indeed correct, just the reason for the compilation breakage was not quite what you suggested. So, I'll apply your putch, but please fix mt9m001.c in the same patch too - it also includes gpio.h conditionally. Although, it does test the correct macro.

Actually, this CONFIG option might disappear completely in the future, if we decide to switch to a module parameter instead. So that one kernel can be used on systems with and without the GPIO.

Thanks
Guennadi
---
Guennadi Liakhovetski

author
#4 lore raw
Quoted 9 lines · Guennadi Liakhovetski

Ok, right, sorry, the patch was indeed correct, just the reason for the compilation breakage was not quite what you suggested. So, I'll apply your putch, but please fix mt9m001.c in the same patch too - it also includes gpio.h conditionally. Although, it does test the correct macro.

Actually, this CONFIG option might disappear completely in the future, if we decide to switch to a module parameter instead. So that one kernel can be used on systems with and without the GPIO.

ok - untested patch for mt9m001.c attached.

Ingo

drivers/media/video/mt9m001.c +1 -4
Index: linux/drivers/media/video/mt9m001.c
===================================================================
@@ -12,15 +12,12 @@
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/log2.h>
+#include <linux/gpio.h>
 
 #include <media/v4l2-common.h>
 #include <media/v4l2-chip-ident.h>
 #include <media/soc_camera.h>
 
-#ifdef CONFIG_MT9M001_PCA9536_SWITCH
-#include <asm/gpio.h>
-#endif
-
 /* mt9m001 i2c address 0x5d
  * The platform has to define i2c_board_info
  * and call i2c_register_board_info() */
#5 lore raw
In reply to Ingo Molnar
Quoted 24 lines · Ingo Molnar

x86.git testing found the following build bug on latest -git:

CC [M] drivers/media/video/mt9v022.o
drivers/media/video/mt9v022.c: In function 'bus_switch_request':
drivers/media/video/mt9v022.c:199: error: implicit declaration of function 'gpio_is_valid' drivers/media/video/mt9v022.c:201: error: implicit declaration of function 'gpio_request' drivers/media/video/mt9v022.c:207: error: implicit declaration of function 'gpio_direction_output' drivers/media/video/mt9v022.c:211: error: implicit declaration of function 'gpio_free' drivers/media/video/mt9v022.c: In function 'bus_switch_act':
drivers/media/video/mt9v022.c:237: error: implicit declaration of function 'gpio_set_value_cansleep' make[2]: *** [drivers/media/video] Error 2
make[1]: *** [drivers/media] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [drivers] Error 2

with this config:

   http://redhat.com/~mingo/misc/config-Sat_May__3_16_08_39_CEST_2008.bad

the bug was that the driver uses GPIO functionality but only includes the GPIO interface definitions for the CONFIG_MT9M001_PCA9536_SWITCH case, which was not set in this config.

Quoted 6 lines

The quick fix seems to be to include linux/gpio.h unconditionally. (this seems like a small cleanup as well as it removes and #ifdef is more robust than an inclusion of asm/gpio.h) Not tested too much yet, so please have another look in any case.

Signed-off-by Ingo Molnar <[email protected]>

Mauro, please pull this fix, extended with a similar fix for mt9m001 from another email from Ingo, along with a copy-paste error in a comment in mt9v022, from

http://linuxtv.org/hg/~gliakhovetski/v4l-dvb

Thanks
Guennadi
---
Guennadi Liakhovetski