Re: [PATCH] media: none of the drivers should be enabled by default

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Em 01-09-2011 03:39, Guennadi Liakhovetski escreveu:
> Hi Mauro
> 
> On Wed, 31 Aug 2011, Mauro Carvalho Chehab wrote:
> 
> [snip]
> 
>>>> I can't comment on the remote controller drivers as I haven't been involved
>>>> with that.
>>>
>>> Mauro?
>>
>> If RC is disabled, most PC cards don't work (bttv, cx88, ivtv, dvb-usb, ...).
>> Ok, this is due to a lack of proper module support on those drivers, but changing
>> it requires some work on each driver that depends on RC.
> 
> wouldn't a simple "select RC_CORE" in those drivers solve this? E.g., for 
> BT848
> 
> diff --git a/drivers/media/video/bt8xx/Kconfig b/drivers/media/video/bt8xx/Kconfig
> index 7da5c2e..28c087bd 100644
> --- a/drivers/media/video/bt8xx/Kconfig
> +++ b/drivers/media/video/bt8xx/Kconfig
> @@ -4,7 +4,7 @@ config VIDEO_BT848
>  	select I2C_ALGOBIT
>  	select VIDEO_BTCX
>  	select VIDEOBUF_DMA_SG
> -	depends on RC_CORE
> +	select RC_CORE

The usage of select is evil and tricky, as select doesn't validate dependency
of the selected symbol. With the above line, Kernel compilation will break if 
INPUT is not selected, as you forgot to add:

	depends on INPUT

We had exactly this bug reported in the past.

We should really get rid of selects, especially when the selected symbol has
dependencies.

Also, the long term idea is to do things like:

config VIDEO_BT848_INPUT
	depends on RC_CORE

And change the drivers to allow compiling with RC disabled. We did this already
for a couple drivers (saa7134 and em28xx).

One option to not use select would be to do the reverse, as the Kbuild maintainers
pointed us: using the same strategy as we did for tuners.

E. g., using one approach like:

config RC_CORE
	depends on VIDEO_BT848 | VIDEO_CX88 | ...
	default y

The problem with this approach is that people will forget to update the RC_CORE
dependency as new cards were added (we tried things like that in the past).

So, the solution should be, instead:

config MEDIA_TV
	tristate "Enable TV/grabber devices"

config VIDEO_BT848
	depends on MEDIA_TV

config RC_CORE
	depends on MEDIA_TV if MEDIA_TV
	default y if MEDIA_TV

Again, the problem is that a few Webcam devices are supported by the same driver
that also provide TV support. So, we'll need to add the option for the user to
select such on both the MEDIA_TV and the MEDIA_WEBCAM groups.

Hmm... maybe we can do something different, grouping devices by their support bus:

menuconfig MEDIA_PCI

# put here all PCI devices

menuconfig MEDIA_USB

# put here all PCI devices

menuconfig MEDIA_SOC

# put here all SoC devices that aren't connected via PCI or USB

config MEDIA_TV
	depends on MEDIA_PCI | MEDIA_USB | DVB_CORE

config RC_CORE
	depends on MEDIA_TV if MEDIA_TV
	default y if MEDIA_TV

This way, the default for RC_CORE will be N. It will only be activated if a PCI
or USB device is selected, or if the user manually selects it, in order to enable
a RC only device. This also means that the ISA and PARPORT devices will
have their own menu.

>  	select VIDEO_TUNER
>  	select VIDEO_TVEEPROM
>  	select VIDEO_MSP3400 if VIDEO_HELPER_CHIPS_AUTO
> 
> and deselect RC_CORE:
> 
> diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
> index 899f783..259a3e7 100644
> --- a/drivers/media/rc/Kconfig
> +++ b/drivers/media/rc/Kconfig
> @@ -1,7 +1,6 @@
>  menuconfig RC_CORE
>  	tristate "Remote Controller adapters"
>  	depends on INPUT
> -	default INPUT
>  	---help---
>  	  Enable support for Remote Controllers on Linux. This is
>  	  needed in order to support several video capture adapters.
> 
> This way by default you have no RCs, but as soon as you enable one card, 
> like bttv, you get all potentially needed drivers.
> 
>> Also, if RC is selected, the RC decoder protocols need to be enabled by default, 
>> as otherwise several devices will stop working, as, on modern devices, there's
>> no hardware anymore to decode the IR pulses. The RC protocol Kconfig options are 
>> there, in fact, to allow disabling some RC decoding protocol when people are 100% 
>> sure that such software decoder won't be needed on some particular environment.
> 
> Ok, they can be selected. Or even I don't mind them being turned on by 
> default, when RC is on, as long as RC itself is off by default.
> 
>>>> With regards to the tuners: perhaps it is sufficient to default MEDIA_ATTACH
>>>> to 'y'? That should prevent building those tuners that are not needed.
>>>
>>> Sorry, I don't see how this should work. I mean tuners under 
>>> drivers/media/common/tuners/.
>>>
>>>> I wouldn't change anything else here.
>>
>> Tuners are required for all TV and DVB cards. Maybe we can put an explicit Kconfig
>> item for TV devices, and change the config stuff to something like:
>>
>> config MEDIA_NEED_TUNER
>> 	tristate
>>
>> menuconfig MEDIA_TV
>> 	tristate "TV and grabber cards"
>> 	select MEDIA_NEED_TUNER
>> ...
>> menuconfig MEDIA_WEBCAMS
>> 	tristate "Webcameras"
>> ...
>>
>> config DVB_CORE
>> 	tristate "DVB for Linux"
>> 	depends on NET && INET
>> 	select CRC32
>> 	select MEDIA_NEED_TUNER
>>
>>
>> config MEDIA_TUNER_TDA827X
>> 	tristate "Philips TDA827X silicon tuner"
>> 	depends on VIDEO_MEDIA && I2C
>> 	default MEDIA_NEED_TUNER if MEDIA_TUNER_CUSTOMIZE
>> 	help
>> 	  A DVB-T silicon tuner module. Say Y when you want to support this tuner.
>>
>> There's one problem with the above strategy: on a few drivers, the same
>> driver is used for both webcams and TV. I know that em28xx has this problem,
>> as the same driver also supports the non-UVC em27xx-based webcams.
>> I think that the same is true also for usbvision.
>>
>> If we put those devices under the "TV and grabber cards", people that have just a
>> em28xx-based webcam won't find them inside the MEDIA_WEBCAMS menus.
>>
>> Of course, we can workaround it, by creating a "fake" item inside the webcams
>> menu, like:
> 
> Yes, sure, or maybe put it under some "hybrid" menu.

It could be another alternative. Yet, it seems to me that selecting media devices
by their bus is the right thing to do.

One of the problems of the current way is that some TV devices are under the DVB
menu, while others are under V4L menu.

If we sort them by bus type, they'll be better organized and we can also fix this
issue. 

This probably means that we should rework the media tree, to be something like:

/drivers/media
	|
	+-- dvb-core
	+-- rc-core
	+-- v4l-core
	+-- subdevs
	|   |
	|   +-- tuners
	|   +-- frontends
	|   +-- sensors
	|   +-- eeprom
	|   +-- video
	|   L-- audio
	+-- rc-keymaps
	L-- drivers
	    |
	    +-- pci
	    +-- usb
	    +-- isa
	    +-- parport
	    +-- soc
	    L-- common			# for things that are common to more than one driver type

>> menuconfig MEDIA_WEBCAMS
>> 	tristate "Webcameras"
>>
>> config MEDIA_EM27xx
>> 	tristate "em27xx-based webcams"
>>
>>
>> and put some glue magic between MEDIA_EM27xx and em28xx:
>>
>> config VIDEO_EM28XX
>> 	depends on MEDIA_EM27xx if MEDIA_EM27xx
> 
> ehem... why not just
> 
> config MEDIA_EM27xx
> 	tristate "em27xx-based webcams"
> 	select VIDEO_EM28XX

Because select will do the wrong thing, as it won't be checking
any of the dozens of em28xx dependencies.

> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/

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


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux