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@xxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/