REGRESSION: Re: [GIT] kconfig rc fixes

Message ID 20101104101910.920efbed.randy.dunlap@oracle.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Randy Dunlap Nov. 4, 2010, 5:19 p.m. UTC
  On Thu, 04 Nov 2010 07:10:11 -0400 Mauro Carvalho Chehab wrote:

> All dependencies required by the selected symbols are satisfied. For example,
> the simplest case is likely cafe_ccic, as, currently, there's just one possible
> driver that can be attached dynamically at runtime to cafe_ccic. We have:
> 
> menu "Encoders/decoders and other helper chips"
> 	depends on !VIDEO_HELPER_CHIPS_AUTO
> 
> ...
> config VIDEO_OV7670
>         tristate "OmniVision OV7670 sensor support"
>         depends on I2C && VIDEO_V4L2
> ...
> endmenu
> 
> config VIDEO_CAFE_CCIC
>         tristate "Marvell 88ALP01 (Cafe) CMOS Camera Controller support"
>         depends on PCI && I2C && VIDEO_V4L2
>         select VIDEO_OV7670
> 
> The dependencies needed by ov7670 (I2C and VIDEO_V4L2) are also dependencies of 
> cafe_ccic. So, it shouldn't have any problem for it to work (and it doesn't have,
> really. This is working as-is during the last 4 years).

This warning line:

warning: (VIDEO_CAFE_CCIC && MEDIA_SUPPORT && VIDEO_CAPTURE_DRIVERS && PCI && I2C && VIDEO_V4L2 || VIDEO_VIA_CAMERA && MEDIA_SUPPORT && VIDEO_CAPTURE_DRIVERS && VIDEO_V4L2 && FB_VIA) selects VIDEO_OV7670 which has unmet direct dependencies (MEDIA_SUPPORT && VIDEO_CAPTURE_DRIVERS && !VIDEO_HELPER_CHIPS_AUTO && I2C && VIDEO_V4L2)

is not caused by CAFE_CCIC -- it's caused by VIDEO_VIA_CAMERA, because
VIDEO_HELPER_CHIPS_AUTO=y, so !VIDEO_HELPER_CHIPS_AUTO is false, so
VIDEO_OV7670 should not be available since it depends on
!VIDEO_HELPER_CHIPS_AUTO.

Below is a simple patch that reduces the kconfig warning count in 2.6.37-rc1
from 240 down to only 88.  :)


> It should be noticed that, even if we replace the menu dependencies by an
> If, won't solve. I tried the enclosed patch, to see if it would produce something
> that the new Kconfig behavior accepts. The same errors apply.

That does not change any kconfig symbol logic/truth values.

> It is fine for me if you want/need to change the way Kconfig works, provided
> that it won't break (or produce those annoying warnings) the existing logic, and
> won't open the manual select menu, if the user selects the auto mode.
> Just send us a patch changing it to some other way of doing it.
> 
> Thanks,
> Mauro
> ---

From: Randy Dunlap <randy.dunlap@oracle.com>

Clean up some video driver dependencies.
Reduces the kconfig warning for unmet dependencies from 240
down to only 88 in 2.6.37-rc1.

This makes the drivers in the VIDEO_HELPER_CHIPS_AUTO menu not
be dependent on that symbol.  Just splash a comment there
instead.  config tools will display the comment when
VIDEO_HELPER_CHIPS_AUTO is false.

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 drivers/media/video/Kconfig |    4 ++++
 1 file changed, 4 insertions(+)

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Comments

Mauro Carvalho Chehab Nov. 4, 2010, 6:11 p.m. UTC | #1
Em 04-11-2010 13:19, Randy Dunlap escreveu:
> On Thu, 04 Nov 2010 07:10:11 -0400 Mauro Carvalho Chehab wrote:
> 
>> All dependencies required by the selected symbols are satisfied. For example,
>> the simplest case is likely cafe_ccic, as, currently, there's just one possible
>> driver that can be attached dynamically at runtime to cafe_ccic. We have:
>>
>> menu "Encoders/decoders and other helper chips"
>> 	depends on !VIDEO_HELPER_CHIPS_AUTO
>>
>> ...
>> config VIDEO_OV7670
>>         tristate "OmniVision OV7670 sensor support"
>>         depends on I2C && VIDEO_V4L2
>> ...
>> endmenu
>>
>> config VIDEO_CAFE_CCIC
>>         tristate "Marvell 88ALP01 (Cafe) CMOS Camera Controller support"
>>         depends on PCI && I2C && VIDEO_V4L2
>>         select VIDEO_OV7670
>>
>> The dependencies needed by ov7670 (I2C and VIDEO_V4L2) are also dependencies of 
>> cafe_ccic. So, it shouldn't have any problem for it to work (and it doesn't have,
>> really. This is working as-is during the last 4 years).
> 
> This warning line:
> 
> warning: (VIDEO_CAFE_CCIC && MEDIA_SUPPORT && VIDEO_CAPTURE_DRIVERS && PCI && I2C && VIDEO_V4L2 || VIDEO_VIA_CAMERA && MEDIA_SUPPORT && VIDEO_CAPTURE_DRIVERS && VIDEO_V4L2 && FB_VIA) selects VIDEO_OV7670 which has unmet direct dependencies (MEDIA_SUPPORT && VIDEO_CAPTURE_DRIVERS && !VIDEO_HELPER_CHIPS_AUTO && I2C && VIDEO_V4L2)
> 
> is not caused by CAFE_CCIC -- it's caused by VIDEO_VIA_CAMERA, because
> VIDEO_HELPER_CHIPS_AUTO=y, so !VIDEO_HELPER_CHIPS_AUTO is false, so
> VIDEO_OV7670 should not be available since it depends on
> !VIDEO_HELPER_CHIPS_AUTO.
> 
> Below is a simple patch that reduces the kconfig warning count in 2.6.37-rc1
> from 240 down to only 88.  :)

Yes, but this makes things worse: it will allow compiling drivers that Kernel
will never use, as they won't work without an I2C adapter, and the I2C adapter
is not compiled.

Worse than that: if you go into all V4L bridge drivers, that implements the I2C
adapters and disable them, the I2C ancillary adapters will still be compiled
(as they won't return to 'n'), but they will never ever be used...

So, no, this is not a solution.

What we need is to prompt the menu only if the user wants to do some manual configuration.
Otherwise, just use the selects done by the drivers that implement the I2C bus adapters,
and have some code to use those selected I2C devices.

Cheers,
Mauro.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Arnaud Lacombe Nov. 4, 2010, 6:32 p.m. UTC | #2
Hi,

On Thu, Nov 4, 2010 at 2:11 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> [...]
> Yes, but this makes things worse: it will allow compiling drivers that Kernel
> will never use, as they won't work without an I2C adapter, and the I2C adapter
> is not compiled.
>
> Worse than that: if you go into all V4L bridge drivers, that implements the I2C
> adapters and disable them, the I2C ancillary adapters will still be compiled
> (as they won't return to 'n'), but they will never ever be used...
>
> So, no, this is not a solution.
>
> What we need is to prompt the menu only if the user wants to do some manual configuration.
> Otherwise, just use the selects done by the drivers that implement the I2C bus adapters,
> and have some code to use those selected I2C devices.
>
These is an easy solution: doing as
`Documentation/kbuild/kconfig-language.txt' say it should be done:

config MODULES
        bool "modules ?"
        default y

config AUTO
        bool "AUTO"

config IVTV
        tristate "IVTV"
        select WM42 if AUTO

menu "TV"
        depends on !AUTO

config WM42_USER
        tristate "WM42"
        select WM42

endmenu

config WM42
        tristate
        default n

 - Arnaud
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Mauro Carvalho Chehab Nov. 4, 2010, 6:51 p.m. UTC | #3
Em 04-11-2010 14:32, Arnaud Lacombe escreveu:
> Hi,
> 
> On Thu, Nov 4, 2010 at 2:11 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> [...]
>> Yes, but this makes things worse: it will allow compiling drivers that Kernel
>> will never use, as they won't work without an I2C adapter, and the I2C adapter
>> is not compiled.
>>
>> Worse than that: if you go into all V4L bridge drivers, that implements the I2C
>> adapters and disable them, the I2C ancillary adapters will still be compiled
>> (as they won't return to 'n'), but they will never ever be used...
>>
>> So, no, this is not a solution.
>>
>> What we need is to prompt the menu only if the user wants to do some manual configuration.
>> Otherwise, just use the selects done by the drivers that implement the I2C bus adapters,
>> and have some code to use those selected I2C devices.
>>
> These is an easy solution: doing as
> `Documentation/kbuild/kconfig-language.txt' say it should be done:
> 
> config MODULES
>         bool "modules ?"
>         default y
> 
> config AUTO
>         bool "AUTO"
> 
> config IVTV
>         tristate "IVTV"
>         select WM42 if AUTO
> 
> menu "TV"
>         depends on !AUTO
> 
> config WM42_USER
>         tristate "WM42"
>         select WM42
> 
> endmenu
> 
> config WM42
>         tristate
>         default n
> 
>  - Arnaud

This may work, but it means that every single I2C/frontend/tuner will require two
entries for each driver. This means to create and manage around 100+ new symbols.
The drivers/media Kconfig files are complex enough as-is, without adding those 100+
new artificial symbols. We should work to make things simple and improve users experience,
and not to create artificial complexity that will make Kconfig almost unreadable.

I still think that the easiest way to solve this is to add some logic that will
hide the menu if a condition doesn't happen. Something like:
	menu FOO
		prompt if BAR

or
	menu FOO
		show if BAR

Mauro.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Arnaud Lacombe Nov. 6, 2010, 9:30 p.m. UTC | #4
Hi,

This should do the job.

A.

Arnaud Lacombe (5):
  kconfig: add an option to determine a menu's visibility
  kconfig: regen parser
  Revert "i2c: Fix Kconfig dependencies"
  media/video: convert Kconfig to use the menu's `visible' keyword
  i2c/algos: convert Kconfig to use the menu's `visible' keyword

 drivers/i2c/Kconfig                  |    3 +-
 drivers/i2c/algos/Kconfig            |   14 +-
 drivers/media/video/Kconfig          |    2 +-
 scripts/kconfig/expr.h               |    1 +
 scripts/kconfig/lkc.h                |    1 +
 scripts/kconfig/menu.c               |   11 +
 scripts/kconfig/zconf.gperf          |    1 +
 scripts/kconfig/zconf.hash.c_shipped |  122 ++++----
 scripts/kconfig/zconf.tab.c_shipped  |  570 +++++++++++++++++----------------
 scripts/kconfig/zconf.y              |   21 +-
 10 files changed, 393 insertions(+), 353 deletions(-)
  
Mauro Carvalho Chehab Nov. 6, 2010, 10:28 p.m. UTC | #5
Em 06-11-2010 17:30, Arnaud Lacombe escreveu:
> Hi,
> 
> This should do the job.
> 

Thank you, Arnaud! Good job!

I'm currently at the airport preparing to take an international flight
to return back home from LPC. I'll test your patch series tomorrow or more likely
during the beginning of the next week. There are probably few more drivers/media
Kconfig files that need to use "visible if" option to remove all warnings, 
but it will be a trivial fix.

Thanks!
Mauro

> A.
> 
> Arnaud Lacombe (5):
>   kconfig: add an option to determine a menu's visibility
>   kconfig: regen parser
>   Revert "i2c: Fix Kconfig dependencies"
>   media/video: convert Kconfig to use the menu's `visible' keyword
>   i2c/algos: convert Kconfig to use the menu's `visible' keyword
> 
>  drivers/i2c/Kconfig                  |    3 +-
>  drivers/i2c/algos/Kconfig            |   14 +-
>  drivers/media/video/Kconfig          |    2 +-
>  scripts/kconfig/expr.h               |    1 +
>  scripts/kconfig/lkc.h                |    1 +
>  scripts/kconfig/menu.c               |   11 +
>  scripts/kconfig/zconf.gperf          |    1 +
>  scripts/kconfig/zconf.hash.c_shipped |  122 ++++----
>  scripts/kconfig/zconf.tab.c_shipped  |  570 +++++++++++++++++----------------
>  scripts/kconfig/zconf.y              |   21 +-
>  10 files changed, 393 insertions(+), 353 deletions(-)
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Patch

--- linux-2.6.37-rc1-git3.orig/drivers/media/video/Kconfig
+++ linux-2.6.37-rc1-git3/drivers/media/video/Kconfig
@@ -112,6 +112,10 @@  config VIDEO_IR_I2C
 #
 
 menu "Encoders/decoders and other helper chips"
+
+comment "Only change these kconfig Helper/Auto settings if you are sure"
+	depends on !VIDEO_HELPER_CHIPS_AUTO
+comment "that you know what you are doing"
 	depends on !VIDEO_HELPER_CHIPS_AUTO
 
 comment "Audio decoders"