login
Header Space

 
 

Re: [patch, -git] media/video/sound build fix, TEA5761/TEA5767

Previous thread: oops in linear_mergeable_bvec by marco gaddoni on Wednesday, April 30, 2008 - 6:52 am. (2 messages)

Next thread: [PATCH] Fix for memory initialisation debugging framework by Mel Gorman on Wednesday, April 30, 2008 - 7:31 am. (1 message)
To: Mauro Carvalho Chehab <mchehab@...>
Cc: Andrew Morton <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, Mike Galbraith <efault@...>
Date: Wednesday, April 30, 2008 - 7:01 am

x86.git randconfig testing found the following new build error in latest 
-git:

 drivers/built-in.o: In function `v4l2_i2c_drv_attach_legacy':
 tuner-core.c:(.text+0x1a829d): undefined reference to `v4l2_i2c_attach'
 drivers/built-in.o: In function `tuner_command':
 tuner-core.c:(.text+0x1a971c): undefined reference to `v4l_printk_ioctl'

the reason appears to be that the TEA5761/TEA5767 tuner code is 
dependent on (legacy?) V4L infrastructure like v4l2_i2c_attach, by 
virtue of drivers/media/video/tuner-core.c including 
media/v4l2-i2c-drv-legacy.h and ./sound/i2c/other/Makefile doing:

  snd-tea575x-tuner-objs := tea575x-tuner.o

for now i solved this via adding a VIDEO_V4L2_COMMON dependency to 
MEDIA_TUNER - which solves the build problem by excluding these drivers 
- but i suspect there might be a better fix as well that does not 
restrict the selectability of these drivers.

Signed-off-by: Ingo Molnar &lt;mingo@elte.hu&gt;
---
 drivers/media/common/tuners/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/drivers/media/common/tuners/Kconfig
===================================================================
--- linux.orig/drivers/media/common/tuners/Kconfig
+++ linux/drivers/media/common/tuners/Kconfig
@@ -20,7 +20,7 @@ config MEDIA_ATTACH
 config MEDIA_TUNER
 	tristate
 	default DVB_CORE || VIDEO_DEV
-	depends on DVB_CORE || VIDEO_DEV
+	depends on (DVB_CORE || VIDEO_DEV) &amp;&amp; VIDEO_V4L2_COMMON
 	select MEDIA_TUNER_XC2028 if !MEDIA_TUNER_CUSTOMIZE
 	select MEDIA_TUNER_XC5000 if !MEDIA_TUNER_CUSTOMIZE
 	select MEDIA_TUNER_MT20XX if !MEDIA_TUNER_CUSTOMIZE
--
To: Mauro Carvalho Chehab <mchehab@...>
Cc: Andrew Morton <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, Mike Galbraith <efault@...>
Date: Wednesday, April 30, 2008 - 7:13 am

updated fix below - another randconfig also pointed it out that there's 
an I2C dependency. (I cannot possibly do a full analysis of the 
dependencies of this code so i keep fixing up dependencies as they 
occur.)

	Ingo

---------------------&gt;
Subject: media/video/sound build fix, TEA5761/TEA5767
From: Ingo Molnar &lt;mingo@elte.hu&gt;
Date: Wed Apr 30 12:44:02 CEST 2008

x86.git randconfig testing found the following build error:

drivers/built-in.o: In function `v4l2_i2c_drv_attach_legacy':
tuner-core.c:(.text+0x1a829d): undefined reference to `v4l2_i2c_attach'
drivers/built-in.o: In function `tuner_command':
tuner-core.c:(.text+0x1a971c): undefined reference to `v4l_printk_ioctl'

the reason is that the TEA5761/TEA5767 tuner code is dependent on
(legacy?) V4L infrastructure like v4l2_i2c_attach, by virtue of
drivers/media/video/tuner-core.c including media/v4l2-i2c-drv-legacy.h
and ./sound/i2c/other/Makefile doing:

  snd-tea575x-tuner-objs := tea575x-tuner.o

for now i solved this via adding a VIDEO_V4L2_COMMON dependency
to MEDIA_TUNER - but i suspect there might be a better fix as well.

Signed-off-by: Ingo Molnar &lt;mingo@elte.hu&gt;
---
 drivers/media/common/tuners/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/drivers/media/common/tuners/Kconfig
===================================================================
--- linux.orig/drivers/media/common/tuners/Kconfig
+++ linux/drivers/media/common/tuners/Kconfig
@@ -20,7 +20,7 @@ config MEDIA_ATTACH
 config MEDIA_TUNER
 	tristate
 	default DVB_CORE || VIDEO_DEV
-	depends on DVB_CORE || VIDEO_DEV
+	depends on (DVB_CORE || VIDEO_DEV) &amp;&amp; VIDEO_V4L2_COMMON &amp;&amp; I2C
 	select MEDIA_TUNER_XC2028 if !MEDIA_TUNER_CUSTOMIZE
 	select MEDIA_TUNER_XC5000 if !MEDIA_TUNER_CUSTOMIZE
 	select MEDIA_TUNER_MT20XX if !MEDIA_TUNER_CUSTOMIZE
--
To: Ingo Molnar <mingo@...>
Cc: Mauro Carvalho Chehab <mchehab@...>, Andrew Morton <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, Mike Galbraith <efault@...>
Date: Wednesday, April 30, 2008 - 8:04 am

At Wed, 30 Apr 2008 13:13:37 +0200,

I guess this patch is also unnecessary if the reverse selection from
fm801-tea575x is replaced with depend by my patch.  Then
VIDEO_V4L2_COMMON and I2C have to be turned on as the dependency of
VIDEO_V4L1.


thanks,

--
To: Ingo Molnar <mingo@...>
Cc: Mauro Carvalho Chehab <mchehab@...>, Andrew Morton <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, Mike Galbraith <efault@...>
Date: Wednesday, April 30, 2008 - 7:11 am

At Wed, 30 Apr 2008 13:01:15 +0200,

IMO, it's the reverse selection from sound to V4L that makes things
complicated.  I believe it's better to fix it as a normal dependency.

How about the patch below?


Takashi


Fix kconfig dependency mess of fm801-tea575x

FM801-tea575x tuner has a reverse selection to V4L1 and this causes
dependency problem.  The patch simplifies the dependency with a
normal depend on VIDEO_V4L1.

Signed-off-by: Takashi Iwai &lt;tiwai@suse.de&gt;
---

diff --git a/sound/pci/Kconfig b/sound/pci/Kconfig
index 581debf..7e47421 100644
--- a/sound/pci/Kconfig
+++ b/sound/pci/Kconfig
@@ -515,19 +515,16 @@ config SND_FM801
 config SND_FM801_TEA575X_BOOL
 	bool "ForteMedia FM801 + TEA5757 tuner"
 	depends on SND_FM801
+	depends on VIDEO_V4L1=y || VIDEO_V4L1=SND_FM801
 	help
 	  Say Y here to include support for soundcards based on the ForteMedia
 	  FM801 chip with a TEA5757 tuner connected to GPIO1-3 pins (Media
 	  Forte SF256-PCS-02) into the snd-fm801 driver.
 
-	  This will enable support for the old V4L1 API.
-
 config SND_FM801_TEA575X
 	tristate
 	depends on SND_FM801_TEA575X_BOOL
 	default SND_FM801
-	select VIDEO_V4L1
-	select VIDEO_DEV
 
 config SND_HDA_INTEL
 	tristate "Intel HD Audio"
--
To: <tiwai@...>
Cc: <mingo@...>, <mchehab@...>, <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, <efault@...>
Date: Wednesday, April 30, 2008 - 7:17 am

From: Takashi Iwai &lt;tiwai@suse.de&gt;

The question that remains is what does this mean for users?

How does a user, who wants to enable this 'sound' driver,
learn that they must enable the v4l subsystem in order to
do so?

This is typically why select constructs are used, to not bother the
user with such messy details.

But like we see in this case, it can cause serious implementation
problems if not used properly. :(
--
To: David Miller <davem@...>
Cc: <mingo@...>, <mchehab@...>, <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, <efault@...>
Date: Wednesday, April 30, 2008 - 7:54 am

At Wed, 30 Apr 2008 04:17:03 -0700 (PDT),


Indeed.  The problem is that the select doesn't (can't?) resolve the
dependecy in a reverse way.  For example, select VIDEO_V4L1 won't turn
on the dependent items such as VIDEO_DEV, VIDEO_V4L2_COMMON,
VIDEO_ALLOW_V4L1, so and so.

Thus, when another dependency is added to the selected target, it can
easily break the depdency chain like this case...


Takashi
--
To: David Miller <davem@...>
Cc: <tiwai@...>, <mingo@...>, <mchehab@...>, <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, <efault@...>
Date: Wednesday, April 30, 2008 - 7:29 am

[OT to the actual problem]

I have envisioned something like a "requires tag" that
would list what a config symbols needs.

Like in this case it would have been:

	requires V4L

This should in the frontends then if the user selects a symbol
where the 'requires' are not satisfied with a window listing
the menuentries for the symbol that the user needs to
enable to satisfy what the original symbol requires.

This is the only way to do this in a way so the
user is actually aware that enabling a webcam also enables USB.
Or at least this is my best suggestion.

But sorry - I have not implemented it.
And it is likely more complicated than I foresee.

	Sam
--
To: Sam Ravnborg <sam@...>
Cc: David Miller <davem@...>, <tiwai@...>, <mchehab@...>, <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, <efault@...>
Date: Wednesday, April 30, 2008 - 8:18 am

but auto-selecting USB is the obvious thing that most users expect...

I.e. they expect a static hierarchy of 'every component' that exists and 
that can be selected. [yes there are complications like multiple-choice, 
but that's the minority]

Users want to browse the hierarchy and pick their things - and _at most_ 
the tool should warn if there's side side-effects of a selection - users 
will likely not mind that enabling a webcam also enables USB (in the 
overwhelming majority of the cases they couldnt care less about that).

But otherwise it should all be automatic.

And i dont even think there should be any additional Kconfig 
complication. Kconfig should be made _simpler_, not more complex. 
Maintainers list dependencies and that's it. No "select" needed at all! 
The stuff that needs to be selected automatically derives from the 
"depends on" tree that we code.

in fact even dependencies between modules of source code should be 
auto-discovered most of the time.

The symbol dependency tree from an allyesconfig kernel could give a 99% 
accurate map of all the dependencies that exist in the code. Why burden 
humans with that? We've got better things to do than to figure out 
dependencies between source code as the dependencies can be totally 
non-obvious.

Humans might want to _optimize_ the dependencies after the fact and get 
rid of unnecessary dependencies (so that the kernel gets smaller, etc.), 
but there should be no correctness ramifications of it visible to 
developers or users.

So the whole thing is being thought about exactly upside down i believe.

	Ingo
--
To: Ingo Molnar <mingo@...>
Cc: David Miller <davem@...>, <tiwai@...>, <mchehab@...>, <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, <efault@...>
Date: Friday, May 2, 2008 - 11:06 am

Which is exactly what my "require SYMBOL2 would give us.
A webcam would not depends on USB, but would "require USB".
If the decide to include the webcam she will optionally be prompted
so she can decide if it is OK to include USB support too.

Would be nice.

	Sam
--
To: Sam Ravnborg <sam@...>
Cc: Ingo Molnar <mingo@...>, David Miller <davem@...>, Takashi Iwai <tiwai@...>, Andrew Morton <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, <efault@...>
Date: Monday, May 5, 2008 - 4:34 pm

This would be a nice feature. In fact, some SELECT's were wrongly added 
in the past, at V4L/DVB Kconfig's as people understood that this would be 
the behaviour of SELECT.

IMO, I think it is preferred that "SELECT" could act as you've described: 
Check if all dependencies for the selected symbol are satisfied. If not, 
auto-selects or prompt to the users.

The auto-select feature, without prompting could be very helpful to allow 
testing kconfig items, since, instead of running all randomconfig's, 
subsystem maintainers can use scripts that will do something like 
allnoconfig, and then select just the symbols requested by each driver 
(*).


(*) Something like:
 	make select="DRIVER_FOO"
 		and
 	make select="DRIVER_FOO_MODULE"

could just do this: marks NO to everything and Y (or M) to the symbol, and 
just the required dependencies for that symbol to compile.

-- 
Cheers,
Mauro Carvalho Chehab
http://linuxtv.org
mchehab@infradead.org
--
To: Sam Ravnborg <sam@...>
Cc: David Miller <davem@...>, <mingo@...>, <mchehab@...>, <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, <efault@...>
Date: Wednesday, April 30, 2008 - 8:09 am

At Wed, 30 Apr 2008 13:29:59 +0200,


Yeah, in some cases, you can't easily turn on the item depends on
a complex dependency.  Distros have similar problems regarding package
maintenance system.


Takashi
--
To: Takashi Iwai <tiwai@...>
Cc: Mauro Carvalho Chehab <mchehab@...>, Andrew Morton <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, Mike Galbraith <efault@...>
Date: Wednesday, April 30, 2008 - 7:15 am

thanks, i've added it to the mix, instead of my patch.

	Ingo
--
To: Takashi Iwai <tiwai@...>
Cc: Mauro Carvalho Chehab <mchehab@...>, Andrew Morton <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, Mike Galbraith <efault@...>
Date: Wednesday, April 30, 2008 - 7:18 am

it now fails with another randconfig:

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

i've added back my hack to keep things rolling. (Note: i've got some 
media drivers hacks in this tree too so maybe one of them has a 
side-effect)

	Ingo
--
To: Ingo Molnar <mingo@...>
Cc: Mauro Carvalho Chehab <mchehab@...>, Andrew Morton <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, Mike Galbraith <efault@...>
Date: Wednesday, April 30, 2008 - 8:01 am

At Wed, 30 Apr 2008 13:18:03 +0200,


Taking a quick look,

	CONFIG_VIDEO_DEV=y
	CONFIG_VIDEO_V4L2_COMMON=y
	# CONFIG_VIDEO_ALLOW_V4L1 is not set
	CONFIG_VIDEO_V4L1_COMPAT=y

seems strange.  VIDEO_ALLOW_V4L1 should have been selected since
drivers/media/video/Kconfig contains the following:

	config VIDEO_V4L1
		tristate
		depends on VIDEO_DEV &amp;&amp; VIDEO_V4L2_COMMON &amp;&amp; VIDEO_ALLOW_V4L1
		default VIDEO_DEV &amp;&amp; VIDEO_V4L2_COMMON &amp;&amp; VIDEO_ALLOW_V4L1

I guess the remaining reverse-selection caused this.


thanks,

Takashi
--
To: Takashi Iwai <tiwai@...>
Cc: Mauro Carvalho Chehab <mchehab@...>, Andrew Morton <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, Mike Galbraith <efault@...>
Date: Wednesday, April 30, 2008 - 7:21 am

FYI, below are the 7 fixes in this area that i've queued up so far. Let 
me know if i should drop any of them.

	Ingo

-----------------------&gt;
Subject: fix SOC_CAMERA_MT9M001 build bug
From: Ingo Molnar &lt;mingo@elte.hu&gt;
Date: Mon Apr 28 13:51:58 CEST 2008

with this randconfig:

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

  CONFIG_SOC_CAMERA_MT9M001=y
  CONFIG_SOC_CAMERA_MT9V022=y
  # CONFIG_I2C is not set

the build fails:

drivers/built-in.o: In function `reg_read':
mt9m001.c:(.text+0x140a87): undefined reference to `i2c_smbus_read_word_data'
drivers/built-in.o: In function `reg_write':
mt9m001.c:(.text+0x140b6e): undefined reference to `i2c_smbus_write_word_data'
drivers/built-in.o: In function `reg_read':
mt9v022.c:(.text+0x1413a7): undefined reference to `i2c_smbus_read_word_data'
drivers/built-in.o: In function `reg_write':
mt9v022.c:(.text+0x1414ce): undefined reference to `i2c_smbus_write_word_data'
drivers/built-in.o: In function `mt9m001_mod_init':
mt9m001.c:(.init.text+0x158a8): undefined reference to `i2c_register_driver'
drivers/built-in.o: In function `mt9v022_mod_init':
mt9v022.c:(.init.text+0x158b8): undefined reference to `i2c_register_driver'
drivers/built-in.o: In function `mt9m001_mod_exit':
mt9m001.c:(.exit.text+0x1656): undefined reference to `i2c_del_driver'
drivers/built-in.o: In function `mt9v022_mod_exit':
mt9v022.c:(.exit.text+0x1666): undefined reference to `i2c_del_driver'

due to missing I2C dependency.

Signed-off-by: Ingo Molnar &lt;mingo@elte.hu&gt;
---
 drivers/media/video/Kconfig |    2 ++
 1 file changed, 2 insertions(+)

Index: linux/drivers/media/video/Kconfig
===================================================================
--- linux.orig/drivers/media/video/Kconfig
+++ linux/drivers/media/video/Kconfig
@@ -908,6 +908,7 @@ config SOC_CAMERA
 config SOC_CAMERA_MT9M001
 	tristate "mt9m001 support"
 	depends on SOC_CAMERA
+	depends on I2C
 	select GPIO_PCA953X if MT9M001_PCA9536_SW...
To: Ingo Molnar <mingo@...>
Cc: Takashi Iwai <tiwai@...>, <video4linux-list@...>, Mike Galbraith <efault@...>, <linux-kernel@...>, Mauro Carvalho Chehab <mchehab@...>, <linux-dvb-maintainer@...>, Andrew Morton <akpm@...>
Date: Wednesday, April 30, 2008 - 8:36 am

Ingo,

It should actually be as follows:



[PATCH] cx23885: fix kbuild dependencies

Thanks to Ingo Molnar for finding this.

Signed-off-by: Michael Krufky &lt;mkrufky@linuxtv.org&gt;

--- linux.orig/drivers/media/video/cx23885/Kconfig
+++ linux/drivers/media/video/cx23885/Kconfig
@@ -9,6 +9,8 @@ config VIDEO_CX23885
 	select VIDEO_IR
 	select VIDEOBUF_DVB
 	select VIDEO_CX25840
+	select VIDEO_CX2341X
+	select DVB_DIB7000P if !DVB_FE_CUSTOMISE
 	select MEDIA_TUNER_MT2131 if !DVB_FE_CUSTOMISE
 	select DVB_S5H1409 if !DVB_FE_CUSTOMISE
 	select DVB_LGDT330X if !DVB_FE_CUSTOMISE
--
To: Ingo Molnar <mingo@...>
Cc: Mauro Carvalho Chehab <mchehab@...>, Andrew Morton <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, Mike Galbraith <efault@...>
Date: Wednesday, April 30, 2008 - 7:58 am

At Wed, 30 Apr 2008 13:21:38 +0200,

This one shouldn't be needed with my patch.


Takashi
--
To: Takashi Iwai <tiwai@...>
Cc: Mauro Carvalho Chehab <mchehab@...>, Andrew Morton <akpm@...>, <linux-dvb-maintainer@...>, <video4linux-list@...>, <linux-kernel@...>, Mike Galbraith <efault@...>
Date: Wednesday, April 30, 2008 - 8:05 am

ok, removed it, we'll see.

I also have the one below, of similar pattern. (so it's 8 fixlets, 
otherwise it's a vanilla kernel in this area)

	Ingo

---------------&gt;
Subject: video: STKWEBCAM fixes
From: Ingo Molnar &lt;mingo@elte.hu&gt;
Date: Tue Apr 29 15:11:29 CEST 2008

x86.git randconfig testing found these build bugs in -git:

 drivers/media/video/stk-webcam.c: In function 'stk_create_sysfs_files':
 drivers/media/video/stk-webcam.c:340: error: implicit declaration of function 'video_device_create_file'
 drivers/media/video/stk-webcam.c: In function 'stk_remove_sysfs_files':
 [...]
 drivers/media/video/stk-webcam.c: In function 'v4l_stk_mmap':
 drivers/media/video/stk-webcam.c:846: error: 'VM_WRITE' undeclared (first use in this function)
 [...]

config at:

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

the reason was a missing include file, and a dependency on a deprecated
(but still in use) VIDEO_V4L1_COMPAT API.

Signed-off-by: Ingo Molnar &lt;mingo@elte.hu&gt;
---
 drivers/media/video/Kconfig      |    1 +
 drivers/media/video/stk-webcam.c |    1 +
 2 files changed, 2 insertions(+)

Index: linux/drivers/media/video/Kconfig
===================================================================
--- linux.orig/drivers/media/video/Kconfig
+++ linux/drivers/media/video/Kconfig
@@ -883,6 +883,7 @@ config USB_ZR364XX
 config USB_STKWEBCAM
 	tristate "USB Syntek DC1125 Camera support"
 	depends on VIDEO_V4L2 &amp;&amp; EXPERIMENTAL
+	select VIDEO_V4L1_COMPAT
 	---help---
 	  Say Y here if you want to use this type of camera.
 	  Supported devices are typically found in some Asus laptops,
Index: linux/drivers/media/video/stk-webcam.c
===================================================================
--- linux.orig/drivers/media/video/stk-webcam.c
+++ linux/drivers/media/video/stk-webcam.c
@@ -28,6 +28,7 @@
 #include &lt;linux/errno.h&gt;
 #include &lt;linux/slab.h&gt;
 #include &lt;linux/kref.h&gt;
+#include &lt;linux/mm.h&gt...
Previous thread: oops in linear_mergeable_bvec by marco gaddoni on Wednesday, April 30, 2008 - 6:52 am. (2 messages)

Next thread: [PATCH] Fix for memory initialisation debugging framework by Mel Gorman on Wednesday, April 30, 2008 - 7:31 am. (1 message)
speck-geostationary