[patch] video: build fix for drivers/media/video/mt9v022.c

[patch] video: build fix for drivers/media/video/mt9v022.c

[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.

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 <mingo@elte.hu>
---
 drivers/media/video/mt9v022.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Index: linux/drivers/media/video/mt9v022.c
===================================================================
--- linux.orig/drivers/media/video/mt9v022.c
+++ 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() */

Re: [patch] video: build fix for drivers/media/video/mt9v022.c

[Guennadi Liakhovetski]

On Sat, 3 May 2008, Ingo Molnar wrote:

> 
> 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.

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

Re: [patch] video: build fix for drivers/media/video/mt9v022.c

[Ingo Molnar]

* Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

> > 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.

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

Re: [patch] video: build fix for drivers/media/video/mt9v022.c

[Guennadi Liakhovetski]

On Sat, 3 May 2008, Ingo Molnar wrote:

> * Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 
> > > 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.
> 
> 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.

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

Re: [patch] video: build fix for drivers/media/video/mt9v022.c

[Ingo Molnar]

* Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

> 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

Index: linux/drivers/media/video/mt9m001.c
===================================================================
--- linux.orig/drivers/media/video/mt9m001.c
+++ 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() */

Re: [patch] video: build fix for drivers/media/video/mt9v022.c

[Guennadi Liakhovetski]

On Sat, 3 May 2008, Ingo Molnar wrote:

> 
> 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 <mingo@elte.hu>

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