Recursive dependency for SAA7134 in 2.6.15-rc7

Recursive dependency for SAA7134 in 2.6.15-rc7

[Jean Delvare]

Hi all,

I gave a try to 2.6.15-rc7 and "make menuconfig" tells me:
Warning! Found recursive dependency: VIDEO_SAA7134_ALSA VIDEO_SAA7134_OSS VIDEO_SAA7134_ALSA

This seems to be the consequence of this patch:
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=7bb9529602f8bb41a92275825b808a42ed33e5be

How do we fix that? menuconfig is indeed really confused by the current
setup. I have the following patch which makes it happier:

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/media/video/saa7134/Kconfig |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- linux-2.6.15-rc7.orig/drivers/media/video/saa7134/Kconfig	2005-12-26 11:04:35.000000000 +0100
+++ linux-2.6.15-rc7/drivers/media/video/saa7134/Kconfig	2005-12-27 11:35:34.000000000 +0100
@@ -14,7 +14,7 @@
 
 config VIDEO_SAA7134_ALSA
 	tristate "Philips SAA7134 DMA audio support"
-	depends on VIDEO_SAA7134 && SOUND && SND && (!VIDEO_SAA7134_OSS || VIDEO_SAA7134_OSS = m)
+	depends on VIDEO_SAA7134 && SND
 	select SND_PCM_OSS
 	---help---
 	  This is a video4linux driver for direct (DMA) audio in
@@ -25,7 +25,7 @@
 
 config VIDEO_SAA7134_OSS
 	tristate "Philips SAA7134 DMA audio support (OSS, DEPRECATED)"
-	depends on VIDEO_SAA7134 && SOUND_PRIME && (!VIDEO_SAA7134_ALSA || VIDEO_SAA7134_ALSA = m)
+	depends on VIDEO_SAA7134 && SOUND_PRIME && VIDEO_SAA7134_ALSA!=y && m
 	---help---
 	  This is a video4linux driver for direct (DMA) audio in
 	  Philips SAA713x based TV cards using OSS

However, I am not entirely happy with it because it introduces an
asymmetry: the OSS variant can no more be built-in, it is now only
available as a module. Not sure this is acceptable.

Another possibility would be to fix Kconfig to properly support this
case. Roman, what do you think?

Yet another possibility would be to not express the dependencies,
explain the mutual exclusion in the help texts, and hope that the user
will read the help texts. I doubt this will work. This seems to be what
was done for the eepro100/e100 case though (well, not even help texts
in this case).

Are there other similar cases in the kernel right now?

It would be nice to fix this before 2.6.15.

Thanks,
-- 
Jean Delvare

Re: Recursive dependency for SAA7134 in 2.6.15-rc7

[Mauro Carvalho Chehab]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

Jean,

Em Ter, 2005-12-27 às 21:53 +0100, Jean Delvare escreveu:
> Hi all,
> 
> I gave a try to 2.6.15-rc7 and "make menuconfig" tells me:
> Warning! Found recursive dependency: VIDEO_SAA7134_ALSA VIDEO_SAA7134_OSS VIDEO_SAA7134_ALSA
> 
> This seems to be the consequence of this patch:
> http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=7bb9529602f8bb41a92275825b808a42ed33e5be
> 
> How do we fix that? menuconfig is indeed really confused by the current
> setup. I have the following patch which makes it happier:

	Please test this patch. It seems that provides the same behavior with a
non-recursive logic.

Cheers, 
Mauro.

[-- Attachment #2: v4l_dvb_saa7134_kconfig_fix.patch --]
[-- Type: text/x-patch, Size: 1100 bytes --]


---

 drivers/media/video/saa7134/Kconfig |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/saa7134/Kconfig b/drivers/media/video/saa7134/Kconfig
index c0f604a..6127c80 100644
--- a/drivers/media/video/saa7134/Kconfig
+++ b/drivers/media/video/saa7134/Kconfig
@@ -14,7 +14,7 @@ config VIDEO_SAA7134
 
 config VIDEO_SAA7134_ALSA
 	tristate "Philips SAA7134 DMA audio support"
-	depends on VIDEO_SAA7134 && SOUND && SND && (!VIDEO_SAA7134_OSS || VIDEO_SAA7134_OSS = m)
+	depends on VIDEO_SAA7134 && SOUND && SND
 	select SND_PCM_OSS
 	---help---
 	  This is a video4linux driver for direct (DMA) audio in
@@ -25,7 +25,7 @@ config VIDEO_SAA7134_ALSA
 
 config VIDEO_SAA7134_OSS
 	tristate "Philips SAA7134 DMA audio support (OSS, DEPRECATED)"
-	depends on VIDEO_SAA7134 && SOUND_PRIME && (!VIDEO_SAA7134_ALSA || VIDEO_SAA7134_ALSA = m)
+	depends on VIDEO_SAA7134 && SOUND_PRIME && (!VIDEO_SAA7134_ALSA || ( VIDEO_SAA7134_ALSA = m && m ) )
 	---help---
 	  This is a video4linux driver for direct (DMA) audio in
 	  Philips SAA713x based TV cards using OSS

Re: Recursive dependency for SAA7134 in 2.6.15-rc7

[Jean Delvare]

Hi Mauro, Linus,

> > I gave a try to 2.6.15-rc7 and "make menuconfig" tells me:
> > Warning! Found recursive dependency: VIDEO_SAA7134_ALSA VIDEO_SAA7134_OSS VIDEO_SAA7134_ALSA
> > 
> > This seems to be the consequence of this patch:
> > http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=7bb9529602f8bb41a92275825b808a42ed33e5be
> > 
> > How do we fix that? menuconfig is indeed really confused by the current
> > setup. I have the following patch which makes it happier:
> 
> Please test this patch. It seems that provides the same behavior with a
> non-recursive logic.

Looks good to me, except for the coding style. Also, depending on both
SND and SOUND doesn't make much sense, as SND itself already depends on
SOUND, so we can simplify the SAA7134_ALSA dependency list.

Linus, can you please apply this patch before releasing 2.6.15?

Thanks.

Fix the cyclic dependency issue between CONFIG_SAA7134_ALSA and
CONFIG_SAA7134_OSS (credits to Mauro Carvalho Chehab.)

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/media/video/saa7134/Kconfig |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- linux-2.6.15-rc7.orig/drivers/media/video/saa7134/Kconfig	2005-12-28 19:13:38.000000000 +0100
+++ linux-2.6.15-rc7/drivers/media/video/saa7134/Kconfig	2005-12-28 20:50:37.000000000 +0100
@@ -14,7 +14,7 @@
 
 config VIDEO_SAA7134_ALSA
 	tristate "Philips SAA7134 DMA audio support"
-	depends on VIDEO_SAA7134 && SOUND && SND && (!VIDEO_SAA7134_OSS || VIDEO_SAA7134_OSS = m)
+	depends on VIDEO_SAA7134 && SND
 	select SND_PCM_OSS
 	---help---
 	  This is a video4linux driver for direct (DMA) audio in
@@ -25,7 +25,7 @@
 
 config VIDEO_SAA7134_OSS
 	tristate "Philips SAA7134 DMA audio support (OSS, DEPRECATED)"
-	depends on VIDEO_SAA7134 && SOUND_PRIME && (!VIDEO_SAA7134_ALSA || VIDEO_SAA7134_ALSA = m)
+	depends on VIDEO_SAA7134 && SOUND_PRIME && (!VIDEO_SAA7134_ALSA || (VIDEO_SAA7134_ALSA=m && m))
 	---help---
 	  This is a video4linux driver for direct (DMA) audio in
 	  Philips SAA713x based TV cards using OSS


-- 
Jean Delvare

Re: Recursive dependency for SAA7134 in 2.6.15-rc7

[Mauro Carvalho Chehab]

Em Qua, 2005-12-28 às 21:02 +0100, Jean Delvare escreveu:
> Hi Mauro, Linus,

> Looks good to me, except for the coding style. Also, depending on both
> SND and SOUND doesn't make much sense, as SND itself already depends on
> SOUND, so we can simplify the SAA7134_ALSA dependency list.
> 
> Linus, can you please apply this patch before releasing 2.6.15?
> 
> Thanks.
> 
> Fix the cyclic dependency issue between CONFIG_SAA7134_ALSA and
> CONFIG_SAA7134_OSS (credits to Mauro Carvalho Chehab.)
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
Acked-by: Mauro Carvalho Chehab <mchehab@brturbo.com.br>

Re: Recursive dependency for SAA7134 in 2.6.15-rc7

[Roman Zippel]

Hi,

On Tuesday 27 December 2005 21:53, Jean Delvare wrote:

> I gave a try to 2.6.15-rc7 and "make menuconfig" tells me:
> Warning! Found recursive dependency: VIDEO_SAA7134_ALSA VIDEO_SAA7134_OSS
> VIDEO_SAA7134_ALSA

Hmm, I really should make this more annoying...

>  config VIDEO_SAA7134_OSS
>  	tristate "Philips SAA7134 DMA audio support (OSS, DEPRECATED)"
> -	depends on VIDEO_SAA7134 && SOUND_PRIME && (!VIDEO_SAA7134_ALSA || 
VIDEO_SAA7134_ALSA = m)
> +	depends on VIDEO_SAA7134 && SOUND_PRIME && 
> VIDEO_SAA7134_ALSA!=y && m

The easiest fix would to just change the last part into "!VIDEO_SAA7134_ALSA".
An alternative would be to use a choice.

bye, Roman

Re: Recursive dependency for SAA7134 in 2.6.15-rc7

[Roman Zippel]

Hi,

On Wednesday 28 December 2005 21:02, Jean Delvare wrote:

> +	depends on VIDEO_SAA7134 && SOUND_PRIME && (!VIDEO_SAA7134_ALSA || 
(VIDEO_SAA7134_ALSA=m && m))

Please do it a little less uglier, simply "!VIDEO_SAA7134_ALSA" is enough.

bye, Roman

Re: Recursive dependency for SAA7134 in 2.6.15-rc7

[Jean Delvare]

Hi Roman,

> On Wednesday 28 December 2005 21:02, Jean Delvare wrote:
> 
> > +	depends on VIDEO_SAA7134 && SOUND_PRIME && (!VIDEO_SAA7134_ALSA || 
> (VIDEO_SAA7134_ALSA=m && m))
> 
> Please do it a little less uglier, simply "!VIDEO_SAA7134_ALSA" is enough.

No, it wouldn't produce the desired effect anymore.

(!VIDEO_SAA7134_ALSA || (VIDEO_SAA7134_ALSA=m && m)) makes it possible
to compile both OSS and ALSA support as modules. Simplifying to
!VIDEO_SAA7134_ALSA would make it impossible.

Thanks,
-- 
Jean Delvare

Re: Recursive dependency for SAA7134 in 2.6.15-rc7

[Roman Zippel]

Hi,

On Thursday 29 December 2005 21:13, Jean Delvare wrote:

> No, it wouldn't produce the desired effect anymore.
>
> (!VIDEO_SAA7134_ALSA || (VIDEO_SAA7134_ALSA=m && m)) makes it possible
> to compile both OSS and ALSA support as modules. Simplifying to
> !VIDEO_SAA7134_ALSA would make it impossible.

Did you try it?

bye, Roman

Re: Recursive dependency for SAA7134 in 2.6.15-rc7

[Jean Delvare]

Hi Roman,

> On Thursday 29 December 2005 21:13, Jean Delvare wrote:
> 
> > No, it wouldn't produce the desired effect anymore.
> >
> > (!VIDEO_SAA7134_ALSA || (VIDEO_SAA7134_ALSA=m && m)) makes it possible
> > to compile both OSS and ALSA support as modules. Simplifying to
> > !VIDEO_SAA7134_ALSA would make it impossible.
> 
> Did you try it?

I didn't back then, just did now, and oh surprise, you are right and I
am wrong... I should have known; after all, *you* wrote kconfig, right?
I checked Documentation/kbuild/kconfig-language.txt and it all makes
sense to me now, although I have to admit the word "counterintuitive"
came to my mind at first. I guess it's just a bit hard to switch to
tristate logic when you use boolean logic all day long.

So, I'm sorry for not verifying it all before I answered to you, and
even more sorry for submitting a less-than-optimal patch for inclusion.
He comes a fix. Linus, feel free to apply it now, or after 2.6.15 is
released, at your option - it's no more a bugfix, only an optimization.

Thanks.

* * * * *

Simplify the VIDEO_SAA7134_OSS Kconfig dependency line a bit. Thanks
to Roman Zippel for the suggestion.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/media/video/saa7134/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-2.6.15-rc7.orig/drivers/media/video/saa7134/Kconfig	2005-12-29 22:01:20.000000000 +0100
+++ linux-2.6.15-rc7/drivers/media/video/saa7134/Kconfig	2005-12-29 22:04:24.000000000 +0100
@@ -25,7 +25,7 @@
 
 config VIDEO_SAA7134_OSS
 	tristate "Philips SAA7134 DMA audio support (OSS, DEPRECATED)"
-	depends on VIDEO_SAA7134 && SOUND_PRIME && (!VIDEO_SAA7134_ALSA || (VIDEO_SAA7134_ALSA=m && m))
+	depends on VIDEO_SAA7134 && SOUND_PRIME && !VIDEO_SAA7134_ALSA
 	---help---
 	  This is a video4linux driver for direct (DMA) audio in
 	  Philips SAA713x based TV cards using OSS


-- 
Jean Delvare

Re: Recursive dependency for SAA7134 in 2.6.15-rc7

[Mauro Carvalho Chehab]

[-- Attachment #1: Type: text/plain, Size: 636 bytes --]

Hi, Jean and Roman,

Em Qui, 2005-12-29 às 22:07 +0100, Jean Delvare escreveu:
> Hi Roman,
> 
> > On Thursday 29 December 2005 21:13, Jean Delvare wrote:
> > 
> > > No, it wouldn't produce the desired effect anymore.
> > 
> > Did you try it?
	Using choice with two tristate config options seems to provide the
better look and feel, while keeping the desired effect. The only
drawback is that Kconfig doesn't accept default values for the "config"
inside a choice. It would be nice if both choice and the ALSA/OSS
options be marked as "m" by default.
	Anyway, IMHO, this is better and clearer than the previous patches.

Cheers, 
Mauro.

[-- Attachment #2: v4l_dvb_tmp_saa7134_choice.patch --]
[-- Type: text/x-patch, Size: 1683 bytes --]

V4L/DVB: change alsa/oss option to choice

Using tristate choice Kconfig option seems to produce a better result.

Signed-off-by: Mauro Carvalho Chehab <mchehab@brturbo.com.br>
---

 drivers/media/video/saa7134/Kconfig |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/media/video/saa7134/Kconfig b/drivers/media/video/saa7134/Kconfig
index 86e1bb3..70f03f5 100644
--- a/drivers/media/video/saa7134/Kconfig
+++ b/drivers/media/video/saa7134/Kconfig
@@ -12,9 +12,14 @@ config VIDEO_SAA7134
 	  To compile this driver as a module, choose M here: the
 	  module will be called saa7134.
 
+choice
+	prompt "Philips SAA7134 DMA audio support"
+	depends on VIDEO_SAA7134 && SOUND
+	default m
+
 config VIDEO_SAA7134_ALSA
-	tristate "Philips SAA7134 DMA audio support"
-	depends on VIDEO_SAA7134 && SND
+	tristate "ALSA audio support"
+	depends on SND
 	select SND_PCM_OSS
 	---help---
 	  This is a video4linux driver for direct (DMA) audio in
@@ -24,8 +29,8 @@ config VIDEO_SAA7134_ALSA
 	  module will be called saa7134-alsa.
 
 config VIDEO_SAA7134_OSS
-	tristate "Philips SAA7134 DMA audio support (OSS, DEPRECATED)"
-	depends on VIDEO_SAA7134 && SOUND_PRIME && (!VIDEO_SAA7134_ALSA || (VIDEO_SAA7134_ALSA=m && m))
+	tristate "OSS audio support (DEPRECATED)"
+	depends on SOUND_PRIME
 	---help---
 	  This is a video4linux driver for direct (DMA) audio in
 	  Philips SAA713x based TV cards using OSS
@@ -34,6 +39,7 @@ config VIDEO_SAA7134_OSS
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called saa7134-oss.
+endchoice
 
 config VIDEO_SAA7134_DVB
 	tristate "DVB/ATSC Support for saa7134 based TV cards"

Re: Recursive dependency for SAA7134 in 2.6.15-rc7

[Jean Delvare]

Hi Mauro,

> > > On Thursday 29 December 2005 21:13, Jean Delvare wrote:
> > > 
> > > > No, it wouldn't produce the desired effect anymore.
> > > 
> > > Did you try it?
>
> 	Using choice with two tristate config options seems to provide the
> better look and feel, while keeping the desired effect. The only
> drawback is that Kconfig doesn't accept default values for the "config"
> inside a choice. It would be nice if both choice and the ALSA/OSS
> options be marked as "m" by default.
> 	Anyway, IMHO, this is better and clearer than the previous patches.

Can you please rebase it on Linus' latest (2.6.15-rc7-git4)? So that I
can give it a try...

Thanks,
-- 
Jean Delvare

Re: Recursive dependency for SAA7134 in 2.6.15-rc7

[Mauro Carvalho Chehab]

[-- Attachment #1: Type: text/plain, Size: 206 bytes --]

Em Sex, 2005-12-30 às 12:06 +0100, Jean Delvare escreveu:
> Hi Mauro,

> Can you please rebase it on Linus' latest (2.6.15-rc7-git4)? So that I
> can give it a try...
	Updated.
> 
> Thanks,
Cheers, 
Mauro.

[-- Attachment #2: v4l_dvb_tmp_saa7134_choice.patch --]
[-- Type: text/x-patch, Size: 1661 bytes --]

V4L/DVB: change alsa/oss option to choice

From:  <>

Using tristate choice Kconfig option seems to produce a
better result.

Signed-off-by: Mauro Carvalho Chehab <mchehab@brturbo.com.br>
---

 drivers/media/video/saa7134/Kconfig |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/media/video/saa7134/Kconfig b/drivers/media/video/saa7134/Kconfig
index 8a5c3e7..70f03f5 100644
--- a/drivers/media/video/saa7134/Kconfig
+++ b/drivers/media/video/saa7134/Kconfig
@@ -12,9 +12,14 @@ config VIDEO_SAA7134
 	  To compile this driver as a module, choose M here: the
 	  module will be called saa7134.
 
+choice
+	prompt "Philips SAA7134 DMA audio support"
+	depends on VIDEO_SAA7134 && SOUND
+	default m
+
 config VIDEO_SAA7134_ALSA
-	tristate "Philips SAA7134 DMA audio support"
-	depends on VIDEO_SAA7134 && SND
+	tristate "ALSA audio support"
+	depends on SND
 	select SND_PCM_OSS
 	---help---
 	  This is a video4linux driver for direct (DMA) audio in
@@ -24,8 +29,8 @@ config VIDEO_SAA7134_ALSA
 	  module will be called saa7134-alsa.
 
 config VIDEO_SAA7134_OSS
-	tristate "Philips SAA7134 DMA audio support (OSS, DEPRECATED)"
-	depends on VIDEO_SAA7134 && SOUND_PRIME && !VIDEO_SAA7134_ALSA
+	tristate "OSS audio support (DEPRECATED)"
+	depends on SOUND_PRIME
 	---help---
 	  This is a video4linux driver for direct (DMA) audio in
 	  Philips SAA713x based TV cards using OSS
@@ -34,6 +39,7 @@ config VIDEO_SAA7134_OSS
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called saa7134-oss.
+endchoice
 
 config VIDEO_SAA7134_DVB
 	tristate "DVB/ATSC Support for saa7134 based TV cards"

Re: Recursive dependency for SAA7134 in 2.6.15-rc7

[Jean Delvare]

Hi Mauro,

Sorry for the delay.

> > Can you please rebase it on Linus' latest (2.6.15-rc7-git4)? So that I
> > can give it a try...
> 	Updated.

I tried it, it works, but I like it less than the current approach.
Being able to select "Philips SAA7134 DMA audio support" as a module
while it doesn't correspond to real code is a bit confusing. Not being
able to unselect it completely is confusing as well. Thinks go even
worse when only ALSA or OSS has been selected in the general sound
menu, as you end up with a complex layout for no reason.

So I believe that "choice" is an interesting Kconfig feature when used
with boolean options, but with modules I am not convinced, especially
when these modules have different dependencies.

So if I were to decide, I would stick to the current code. But I am
obviously not the one to decide here.

Thanks,
-- 
Jean Delvare

Re: Recursive dependency for SAA7134 in 2.6.15-rc7

[Roman Zippel]

Hi,

On Saturday 31 December 2005 19:39, Jean Delvare wrote:

> So I believe that "choice" is an interesting Kconfig feature when used
> with boolean options, but with modules I am not convinced, especially
> when these modules have different dependencies.

Well, I'm always open to suggestions (or even better patches) to improve 
tristate choices. Such interdependent options have to be done via a choice 
group, so they are handled correctly handled by kconfig, otherwise you have 
to live with the current compromise. OTOH how they are mapped to the user 
interface is easily changeable.

bye, Roman

Re: Recursive dependency for SAA7134 in 2.6.15-rc7

[Jean Delvare]

Hi Roman,

> On Saturday 31 December 2005 19:39, Jean Delvare wrote:
> 
> > So I believe that "choice" is an interesting Kconfig feature when used
> > with boolean options, but with modules I am not convinced, especially
> > when these modules have different dependencies.
> 
> Well, I'm always open to suggestions (or even better patches) to improve 
> tristate choices. Such interdependent options have to be done via a choice 
> group, so they are correctly handled by kconfig, otherwise you have 
> to live with the current compromise. OTOH how they are mapped to the user 
> interface is easily changeable.

What I like with the current "compromise" in the SAA1734 case is that,
if the user has only OSS or only ALSA enabled in the sound menu, then
he/she only sees one available module for SAA7134 sound support. It
keeps the configuration interface as simple as it can be. While when
using a choice, the user will still be presented a pre-item/menu, with a
single choice in there. This is what I think is confusing.

If "choice" could fall back to a simple option when the dependencies
are such that only once choice is actually possible, I think it would
improve the situation, in the case of the SAA7134. But I lack time to
propose an actual patch implementing this, and I also did not
investigate to see what it would do for the other use cases of "choice"
in the current kernel tree. Lastly I guess that some people may find it
even more confusing if things were done the way I suggest - it's really
a matter of personal opinion at this point.

Given that there are only a few use cases of "choice" with tri-state
options, it might be just as easy to leave things as is and do with
what we have.

Thanks,
-- 
Jean Delvare