[PATCH] usbtv: fix dependency

[PATCH] usbtv: fix dependency

[Hans Verkuil]

This fixes a dependency problem as found by Randy Dunlap:

https://lkml.org/lkml/2013/6/27/501

Mauro, is there any reason for any V4L2 driver to depend on VIDEO_DEV instead of
just VIDEO_V4L2?

Some drivers depend on VIDEO_DEV, some on VIDEO_V4L2, some on both. It's all
pretty chaotic.

Regards,

	Hans

diff --git a/drivers/media/usb/usbtv/Kconfig b/drivers/media/usb/usbtv/Kconfig
index 8864436..7c5b860 100644
--- a/drivers/media/usb/usbtv/Kconfig
+++ b/drivers/media/usb/usbtv/Kconfig
@@ -1,6 +1,6 @@
 config VIDEO_USBTV
         tristate "USBTV007 video capture support"
-        depends on VIDEO_DEV
+        depends on VIDEO_V4L2
         select VIDEOBUF2_VMALLOC
 
         ---help---

Re: [PATCH] usbtv: fix dependency

[Mauro Carvalho Chehab]

Em Fri, 28 Jun 2013 10:24:15 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> This fixes a dependency problem as found by Randy Dunlap:
> 
> https://lkml.org/lkml/2013/6/27/501
> 
> Mauro, is there any reason for any V4L2 driver to depend on VIDEO_DEV instead of
> just VIDEO_V4L2?
> 
> Some drivers depend on VIDEO_DEV, some on VIDEO_V4L2, some on both. It's all
> pretty chaotic.

It should be noticed that, despite its name, this config is actually a
joint dependency of VIDEO_DEV and I2C that will compile drivers as module
if either I2C or VIDEO_DEV is a module:

	config VIDEO_V4L2
		tristate
		depends on (I2C || I2C=n) && VIDEO_DEV
		default (I2C || I2C=n) && VIDEO_DEV

So, a V4L2 device that doesn't have any I2C device doesn't need to depend
on VIDEO_V4L2. That includes, for example, reversed-engineered webcam
drivers where the sensor code is inside the driver and a few capture-only
device drivers.

It should be noticed, however, that, on several places, the need of adding
a "depends on VIDEO_V4L2" is not needed, as, on some places, the syntax
is:

	if VIDEO_V4L2

	config "driver foo"
	...

	endif

Btw, it could make sense to rename it to something clearer, like
VIDEO_DEV_AND_I2C and define it as:

	config VIDEO_DEV_AND_I2C
		tristate
		depends on I2C && VIDEO_DEV
		default y

Or, even better, to just get rid of it and explicitly add I2C on all
places where it is used.


Regards,
Mauro

> 
> Regards,
> 
> 	Hans
> 
> diff --git a/drivers/media/usb/usbtv/Kconfig b/drivers/media/usb/usbtv/Kconfig
> index 8864436..7c5b860 100644
> --- a/drivers/media/usb/usbtv/Kconfig
> +++ b/drivers/media/usb/usbtv/Kconfig
> @@ -1,6 +1,6 @@
>  config VIDEO_USBTV
>          tristate "USBTV007 video capture support"
> -        depends on VIDEO_DEV
> +        depends on VIDEO_V4L2
>          select VIDEOBUF2_VMALLOC
>  
>          ---help---


-- 

Cheers,
Mauro

Re: [PATCH] usbtv: fix dependency

[Hans Verkuil]

On Fri June 28 2013 13:00:43 Mauro Carvalho Chehab wrote:
> Em Fri, 28 Jun 2013 10:24:15 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > This fixes a dependency problem as found by Randy Dunlap:
> > 
> > https://lkml.org/lkml/2013/6/27/501
> > 
> > Mauro, is there any reason for any V4L2 driver to depend on VIDEO_DEV instead of
> > just VIDEO_V4L2?
> > 
> > Some drivers depend on VIDEO_DEV, some on VIDEO_V4L2, some on both. It's all
> > pretty chaotic.
> 
> It should be noticed that, despite its name, this config is actually a
> joint dependency of VIDEO_DEV and I2C that will compile drivers as module
> if either I2C or VIDEO_DEV is a module:
> 
> 	config VIDEO_V4L2
> 		tristate
> 		depends on (I2C || I2C=n) && VIDEO_DEV
> 		default (I2C || I2C=n) && VIDEO_DEV
> 
> So, a V4L2 device that doesn't have any I2C device doesn't need to depend
> on VIDEO_V4L2. That includes, for example, reversed-engineered webcam
> drivers where the sensor code is inside the driver and a few capture-only
> device drivers.

Yes, it does have to depend on it. That's exactly why usbtv is failing: like
any other v4l2 driver usbtv needs the videodev.ko module. That is dependent
on VIDEO_V4L2. What is happening here is that the dependency of usbtv on
VIDEO_DEV allows it to be built as part of the kernel, but VIDEO_V4L2 is built
as a module due to its I2C dependency with the result that usbtv can't link to
the videodev functions.

The way things are today I do not believe any v4l2 driver should depend on
VIDEO_DEV, instead they should all depend on VIDEO_V4L2. That would make a
lot more sense.

	Hans

> 
> It should be noticed, however, that, on several places, the need of adding
> a "depends on VIDEO_V4L2" is not needed, as, on some places, the syntax
> is:
> 
> 	if VIDEO_V4L2
> 
> 	config "driver foo"
> 	...
> 
> 	endif
> 
> Btw, it could make sense to rename it to something clearer, like
> VIDEO_DEV_AND_I2C and define it as:
> 
> 	config VIDEO_DEV_AND_I2C
> 		tristate
> 		depends on I2C && VIDEO_DEV
> 		default y
> 
> Or, even better, to just get rid of it and explicitly add I2C on all
> places where it is used.
> 
> 
> Regards,
> Mauro
> 
> > 
> > Regards,
> > 
> > 	Hans
> > 
> > diff --git a/drivers/media/usb/usbtv/Kconfig b/drivers/media/usb/usbtv/Kconfig
> > index 8864436..7c5b860 100644
> > --- a/drivers/media/usb/usbtv/Kconfig
> > +++ b/drivers/media/usb/usbtv/Kconfig
> > @@ -1,6 +1,6 @@
> >  config VIDEO_USBTV
> >          tristate "USBTV007 video capture support"
> > -        depends on VIDEO_DEV
> > +        depends on VIDEO_V4L2
> >          select VIDEOBUF2_VMALLOC
> >  
> >          ---help---
> 
> 
> 

Re: [PATCH] usbtv: fix dependency

[Mauro Carvalho Chehab]

Em Fri, 28 Jun 2013 13:18:44 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Fri June 28 2013 13:00:43 Mauro Carvalho Chehab wrote:
> > Em Fri, 28 Jun 2013 10:24:15 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > 
> > > This fixes a dependency problem as found by Randy Dunlap:
> > > 
> > > https://lkml.org/lkml/2013/6/27/501
> > > 
> > > Mauro, is there any reason for any V4L2 driver to depend on VIDEO_DEV instead of
> > > just VIDEO_V4L2?
> > > 
> > > Some drivers depend on VIDEO_DEV, some on VIDEO_V4L2, some on both. It's all
> > > pretty chaotic.
> > 
> > It should be noticed that, despite its name, this config is actually a
> > joint dependency of VIDEO_DEV and I2C that will compile drivers as module
> > if either I2C or VIDEO_DEV is a module:
> > 
> > 	config VIDEO_V4L2
> > 		tristate
> > 		depends on (I2C || I2C=n) && VIDEO_DEV
> > 		default (I2C || I2C=n) && VIDEO_DEV
> > 
> > So, a V4L2 device that doesn't have any I2C device doesn't need to depend
> > on VIDEO_V4L2. That includes, for example, reversed-engineered webcam
> > drivers where the sensor code is inside the driver and a few capture-only
> > device drivers.
> 
> Yes, it does have to depend on it. That's exactly why usbtv is failing: like
> any other v4l2 driver usbtv needs the videodev.ko module. That is dependent
> on VIDEO_V4L2. What is happening here is that the dependency of usbtv on
> VIDEO_DEV allows it to be built as part of the kernel, but VIDEO_V4L2 is built
> as a module due to its I2C dependency with the result that usbtv can't link to
> the videodev functions.
> 
> The way things are today I do not believe any v4l2 driver should depend on
> VIDEO_DEV, instead they should all depend on VIDEO_V4L2. That would make a
> lot more sense.

Hmm...

$ git grep -l i2c drivers/media/v4l2-core/
drivers/media/v4l2-core/tuner-core.c		(not part of videodev.ko module)
drivers/media/v4l2-core/v4l2-async.c
drivers/media/v4l2-core/v4l2-common.c
drivers/media/v4l2-core/v4l2-ctrls.c		(actually, there's just a comment there)
drivers/media/v4l2-core/v4l2-device.c

$ git grep  CONFIG_I2C drivers/media/v4l2-core/
drivers/media/v4l2-core/v4l2-common.c:#if IS_ENABLED(CONFIG_I2C)
drivers/media/v4l2-core/v4l2-common.c:#endif /* defined(CONFIG_I2C) */
drivers/media/v4l2-core/v4l2-device.c:#if IS_ENABLED(CONFIG_I2C)

yes, there are some parts of videodev that are dependent on I2C.

That's basically why all V4L2 drivers should depend on VIDEO_V4L2.

That's said, before the addition of v4l2-async, it was safe to compile
the core without I2C, as the parts of the code that are I2C specific are
protected by a:
	#if defined(CONFIG_I2C)

With its addition, I suspect that we'll still have Kbuild issues, if I2C
is disabled and a driver that doesn't depends on I2C is compiled.

So, 2 patches seem to be needed:

1) a patch that replaces all driver dependencies from CONFIG_DEV to
   CONFIG_V4L2;

2) a patch that fixes the issues with v4l2-async.

With regards to the last one, I can see 3 ways to fix it:
	1) don't add v4l2-async at videodev.ko if I2C is not selected;
	2) protect the I2C specific parts of v4l2-async with
		#if defined(CONFIG_I2C)
	3) put v4l2-async on a separate module.

(or some combination of the above)

As only very few drivers use v4l2-async, as this is more focused to
fix troubles with OT, I think that the better would be to do (3) and
to add an specific Kconfig entry for it.

Regards,
Mauro.

Re: [PATCH] usbtv: fix dependency

[Hans Verkuil]

On Fri June 28 2013 14:42:46 Mauro Carvalho Chehab wrote:
> Em Fri, 28 Jun 2013 13:18:44 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > On Fri June 28 2013 13:00:43 Mauro Carvalho Chehab wrote:
> > > Em Fri, 28 Jun 2013 10:24:15 +0200
> > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > > 
> > > > This fixes a dependency problem as found by Randy Dunlap:
> > > > 
> > > > https://lkml.org/lkml/2013/6/27/501
> > > > 
> > > > Mauro, is there any reason for any V4L2 driver to depend on VIDEO_DEV instead of
> > > > just VIDEO_V4L2?
> > > > 
> > > > Some drivers depend on VIDEO_DEV, some on VIDEO_V4L2, some on both. It's all
> > > > pretty chaotic.
> > > 
> > > It should be noticed that, despite its name, this config is actually a
> > > joint dependency of VIDEO_DEV and I2C that will compile drivers as module
> > > if either I2C or VIDEO_DEV is a module:
> > > 
> > > 	config VIDEO_V4L2
> > > 		tristate
> > > 		depends on (I2C || I2C=n) && VIDEO_DEV
> > > 		default (I2C || I2C=n) && VIDEO_DEV
> > > 
> > > So, a V4L2 device that doesn't have any I2C device doesn't need to depend
> > > on VIDEO_V4L2. That includes, for example, reversed-engineered webcam
> > > drivers where the sensor code is inside the driver and a few capture-only
> > > device drivers.
> > 
> > Yes, it does have to depend on it. That's exactly why usbtv is failing: like
> > any other v4l2 driver usbtv needs the videodev.ko module. That is dependent
> > on VIDEO_V4L2. What is happening here is that the dependency of usbtv on
> > VIDEO_DEV allows it to be built as part of the kernel, but VIDEO_V4L2 is built
> > as a module due to its I2C dependency with the result that usbtv can't link to
> > the videodev functions.
> > 
> > The way things are today I do not believe any v4l2 driver should depend on
> > VIDEO_DEV, instead they should all depend on VIDEO_V4L2. That would make a
> > lot more sense.
> 
> Hmm...
> 
> $ git grep -l i2c drivers/media/v4l2-core/
> drivers/media/v4l2-core/tuner-core.c		(not part of videodev.ko module)
> drivers/media/v4l2-core/v4l2-async.c
> drivers/media/v4l2-core/v4l2-common.c
> drivers/media/v4l2-core/v4l2-ctrls.c		(actually, there's just a comment there)
> drivers/media/v4l2-core/v4l2-device.c
> 
> $ git grep  CONFIG_I2C drivers/media/v4l2-core/
> drivers/media/v4l2-core/v4l2-common.c:#if IS_ENABLED(CONFIG_I2C)
> drivers/media/v4l2-core/v4l2-common.c:#endif /* defined(CONFIG_I2C) */
> drivers/media/v4l2-core/v4l2-device.c:#if IS_ENABLED(CONFIG_I2C)
> 
> yes, there are some parts of videodev that are dependent on I2C.
> 
> That's basically why all V4L2 drivers should depend on VIDEO_V4L2.
> 
> That's said, before the addition of v4l2-async, it was safe to compile
> the core without I2C, as the parts of the code that are I2C specific are
> protected by a:
> 	#if defined(CONFIG_I2C)
> 
> With its addition, I suspect that we'll still have Kbuild issues, if I2C
> is disabled and a driver that doesn't depends on I2C is compiled.
> 
> So, 2 patches seem to be needed:
> 
> 1) a patch that replaces all driver dependencies from CONFIG_DEV to
>    CONFIG_V4L2;
> 
> 2) a patch that fixes the issues with v4l2-async.
> 
> With regards to the last one, I can see 3 ways to fix it:
> 	1) don't add v4l2-async at videodev.ko if I2C is not selected;
> 	2) protect the I2C specific parts of v4l2-async with
> 		#if defined(CONFIG_I2C)
> 	3) put v4l2-async on a separate module.
> 
> (or some combination of the above)
> 
> As only very few drivers use v4l2-async, as this is more focused to
> fix troubles with OT, I think that the better would be to do (3) and
> to add an specific Kconfig entry for it.

No, #2 is the right choice here. That's necessary anyway since it is a valid
use-case that I2C is disabled and you use it for platform devices only.

Guennadi, can you look at this? The only thing that is probably needed is
that match_i2c returns false if CONFIG_I2C is undefined.

I prefer to keep this part of videodev, at least for now: I think there will
be a fairly quick uptake of this API internally, certainly for subdevs. Note
BTW that even x86 kernels come with CONFIG_OF enabled these days.

Regards,

	Hans

Re: [PATCH] usbtv: fix dependency

[Guennadi Liakhovetski]

Hi Hans, Mauro

On Fri, 28 Jun 2013, Hans Verkuil wrote:

> On Fri June 28 2013 14:42:46 Mauro Carvalho Chehab wrote:
> > Em Fri, 28 Jun 2013 13:18:44 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > 
> > > On Fri June 28 2013 13:00:43 Mauro Carvalho Chehab wrote:
> > > > Em Fri, 28 Jun 2013 10:24:15 +0200
> > > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > > > 
> > > > > This fixes a dependency problem as found by Randy Dunlap:
> > > > > 
> > > > > https://lkml.org/lkml/2013/6/27/501
> > > > > 
> > > > > Mauro, is there any reason for any V4L2 driver to depend on VIDEO_DEV instead of
> > > > > just VIDEO_V4L2?
> > > > > 
> > > > > Some drivers depend on VIDEO_DEV, some on VIDEO_V4L2, some on both. It's all
> > > > > pretty chaotic.
> > > > 
> > > > It should be noticed that, despite its name, this config is actually a
> > > > joint dependency of VIDEO_DEV and I2C that will compile drivers as module
> > > > if either I2C or VIDEO_DEV is a module:
> > > > 
> > > > 	config VIDEO_V4L2
> > > > 		tristate
> > > > 		depends on (I2C || I2C=n) && VIDEO_DEV
> > > > 		default (I2C || I2C=n) && VIDEO_DEV
> > > > 
> > > > So, a V4L2 device that doesn't have any I2C device doesn't need to depend
> > > > on VIDEO_V4L2. That includes, for example, reversed-engineered webcam
> > > > drivers where the sensor code is inside the driver and a few capture-only
> > > > device drivers.
> > > 
> > > Yes, it does have to depend on it. That's exactly why usbtv is failing: like
> > > any other v4l2 driver usbtv needs the videodev.ko module. That is dependent
> > > on VIDEO_V4L2. What is happening here is that the dependency of usbtv on
> > > VIDEO_DEV allows it to be built as part of the kernel, but VIDEO_V4L2 is built
> > > as a module due to its I2C dependency with the result that usbtv can't link to
> > > the videodev functions.
> > > 
> > > The way things are today I do not believe any v4l2 driver should depend on
> > > VIDEO_DEV, instead they should all depend on VIDEO_V4L2. That would make a
> > > lot more sense.
> > 
> > Hmm...
> > 
> > $ git grep -l i2c drivers/media/v4l2-core/
> > drivers/media/v4l2-core/tuner-core.c		(not part of videodev.ko module)
> > drivers/media/v4l2-core/v4l2-async.c
> > drivers/media/v4l2-core/v4l2-common.c
> > drivers/media/v4l2-core/v4l2-ctrls.c		(actually, there's just a comment there)
> > drivers/media/v4l2-core/v4l2-device.c
> > 
> > $ git grep  CONFIG_I2C drivers/media/v4l2-core/
> > drivers/media/v4l2-core/v4l2-common.c:#if IS_ENABLED(CONFIG_I2C)
> > drivers/media/v4l2-core/v4l2-common.c:#endif /* defined(CONFIG_I2C) */
> > drivers/media/v4l2-core/v4l2-device.c:#if IS_ENABLED(CONFIG_I2C)
> > 
> > yes, there are some parts of videodev that are dependent on I2C.
> > 
> > That's basically why all V4L2 drivers should depend on VIDEO_V4L2.
> > 
> > That's said, before the addition of v4l2-async, it was safe to compile
> > the core without I2C, as the parts of the code that are I2C specific are
> > protected by a:
> > 	#if defined(CONFIG_I2C)
> > 
> > With its addition, I suspect that we'll still have Kbuild issues, if I2C
> > is disabled and a driver that doesn't depends on I2C is compiled.
> > 
> > So, 2 patches seem to be needed:
> > 
> > 1) a patch that replaces all driver dependencies from CONFIG_DEV to
> >    CONFIG_V4L2;
> > 
> > 2) a patch that fixes the issues with v4l2-async.
> > 
> > With regards to the last one, I can see 3 ways to fix it:
> > 	1) don't add v4l2-async at videodev.ko if I2C is not selected;
> > 	2) protect the I2C specific parts of v4l2-async with
> > 		#if defined(CONFIG_I2C)
> > 	3) put v4l2-async on a separate module.
> > 
> > (or some combination of the above)
> > 
> > As only very few drivers use v4l2-async, as this is more focused to
> > fix troubles with OT, I think that the better would be to do (3) and
> > to add an specific Kconfig entry for it.
> 
> No, #2 is the right choice here. That's necessary anyway since it is a valid
> use-case that I2C is disabled and you use it for platform devices only.
> 
> Guennadi, can you look at this? The only thing that is probably needed is
> that match_i2c returns false if CONFIG_I2C is undefined.
> 
> I prefer to keep this part of videodev, at least for now: I think there will
> be a fairly quick uptake of this API internally, certainly for subdevs. Note
> BTW that even x86 kernels come with CONFIG_OF enabled these days.

This patch

http://git.linuxtv.org/gliakhovetski/v4l-dvb.git/commitdiff/a92d0222c693db29a5d00eaedcdebf748789c38e

has been pushed 3 days ago:

https://patchwork.linuxtv.org/patch/19090/

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

Re: [PATCH] usbtv: fix dependency

[Mauro Carvalho Chehab]

Em Fri, 28 Jun 2013 15:24:20 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> escreveu:

> Hi Hans, Mauro
> 
> On Fri, 28 Jun 2013, Hans Verkuil wrote:
> 
> > On Fri June 28 2013 14:42:46 Mauro Carvalho Chehab wrote:
> > > Em Fri, 28 Jun 2013 13:18:44 +0200
> > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > > 
> > > > On Fri June 28 2013 13:00:43 Mauro Carvalho Chehab wrote:
> > > > > Em Fri, 28 Jun 2013 10:24:15 +0200
> > > > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > > > > 
> > > > > > This fixes a dependency problem as found by Randy Dunlap:
> > > > > > 
> > > > > > https://lkml.org/lkml/2013/6/27/501
> > > > > > 
> > > > > > Mauro, is there any reason for any V4L2 driver to depend on VIDEO_DEV instead of
> > > > > > just VIDEO_V4L2?
> > > > > > 
> > > > > > Some drivers depend on VIDEO_DEV, some on VIDEO_V4L2, some on both. It's all
> > > > > > pretty chaotic.
> > > > > 
> > > > > It should be noticed that, despite its name, this config is actually a
> > > > > joint dependency of VIDEO_DEV and I2C that will compile drivers as module
> > > > > if either I2C or VIDEO_DEV is a module:
> > > > > 
> > > > > 	config VIDEO_V4L2
> > > > > 		tristate
> > > > > 		depends on (I2C || I2C=n) && VIDEO_DEV
> > > > > 		default (I2C || I2C=n) && VIDEO_DEV
> > > > > 
> > > > > So, a V4L2 device that doesn't have any I2C device doesn't need to depend
> > > > > on VIDEO_V4L2. That includes, for example, reversed-engineered webcam
> > > > > drivers where the sensor code is inside the driver and a few capture-only
> > > > > device drivers.
> > > > 
> > > > Yes, it does have to depend on it. That's exactly why usbtv is failing: like
> > > > any other v4l2 driver usbtv needs the videodev.ko module. That is dependent
> > > > on VIDEO_V4L2. What is happening here is that the dependency of usbtv on
> > > > VIDEO_DEV allows it to be built as part of the kernel, but VIDEO_V4L2 is built
> > > > as a module due to its I2C dependency with the result that usbtv can't link to
> > > > the videodev functions.
> > > > 
> > > > The way things are today I do not believe any v4l2 driver should depend on
> > > > VIDEO_DEV, instead they should all depend on VIDEO_V4L2. That would make a
> > > > lot more sense.
> > > 
> > > Hmm...
> > > 
> > > $ git grep -l i2c drivers/media/v4l2-core/
> > > drivers/media/v4l2-core/tuner-core.c		(not part of videodev.ko module)
> > > drivers/media/v4l2-core/v4l2-async.c
> > > drivers/media/v4l2-core/v4l2-common.c
> > > drivers/media/v4l2-core/v4l2-ctrls.c		(actually, there's just a comment there)
> > > drivers/media/v4l2-core/v4l2-device.c
> > > 
> > > $ git grep  CONFIG_I2C drivers/media/v4l2-core/
> > > drivers/media/v4l2-core/v4l2-common.c:#if IS_ENABLED(CONFIG_I2C)
> > > drivers/media/v4l2-core/v4l2-common.c:#endif /* defined(CONFIG_I2C) */
> > > drivers/media/v4l2-core/v4l2-device.c:#if IS_ENABLED(CONFIG_I2C)
> > > 
> > > yes, there are some parts of videodev that are dependent on I2C.
> > > 
> > > That's basically why all V4L2 drivers should depend on VIDEO_V4L2.
> > > 
> > > That's said, before the addition of v4l2-async, it was safe to compile
> > > the core without I2C, as the parts of the code that are I2C specific are
> > > protected by a:
> > > 	#if defined(CONFIG_I2C)
> > > 
> > > With its addition, I suspect that we'll still have Kbuild issues, if I2C
> > > is disabled and a driver that doesn't depends on I2C is compiled.
> > > 
> > > So, 2 patches seem to be needed:
> > > 
> > > 1) a patch that replaces all driver dependencies from CONFIG_DEV to
> > >    CONFIG_V4L2;
> > > 
> > > 2) a patch that fixes the issues with v4l2-async.
> > > 
> > > With regards to the last one, I can see 3 ways to fix it:
> > > 	1) don't add v4l2-async at videodev.ko if I2C is not selected;
> > > 	2) protect the I2C specific parts of v4l2-async with
> > > 		#if defined(CONFIG_I2C)
> > > 	3) put v4l2-async on a separate module.
> > > 
> > > (or some combination of the above)
> > > 
> > > As only very few drivers use v4l2-async, as this is more focused to
> > > fix troubles with OT, I think that the better would be to do (3) and
> > > to add an specific Kconfig entry for it.
> > 
> > No, #2 is the right choice here. That's necessary anyway since it is a valid
> > use-case that I2C is disabled and you use it for platform devices only.

True, if are there any case where a platform kernel would be compiled without
I2C. I can't figure out any of such cases where such a weird config would
make sense.

> > 
> > Guennadi, can you look at this? The only thing that is probably needed is
> > that match_i2c returns false if CONFIG_I2C is undefined.
> > 
> > I prefer to keep this part of videodev, at least for now: I think there will
> > be a fairly quick uptake of this API internally, certainly for subdevs. Note
> > BTW that even x86 kernels come with CONFIG_OF enabled these days.

Fedora 18 (and even Fedora 19 beta) Kernels don't enable it for x86. It
is enabled there only for arm:
	config-armv7-generic:CONFIG_OF=y
	config-armv7-generic:CONFIG_OF_DEVICE=y

I don't doubt that this would be enabled on x86 there some day, but
this is not the current status, and I fail to see why one would enable
it on x86, except if some specific distro have some specific issues
at their boot loaders that would be easier to solve if OF is enabled
on all of their kernels.

>From my side, I don't see much sense of overbloating the videodev core
with platform-specific code.

> 
> This patch
> 
> http://git.linuxtv.org/gliakhovetski/v4l-dvb.git/commitdiff/a92d0222c693db29a5d00eaedcdebf748789c38e
> 
> has been pushed 3 days ago:
> 
> https://patchwork.linuxtv.org/patch/19090/

As, according with:
	https://lwn.net/Articles/556034/rss

-rc7 is likely the last one before 3.10, that means that the media merge
window for 3.11 is closed already (as we close it one week before, in order
to give more time for reviewing the patches better at -next).

So, please split the fix patches from it on a separate pull request.

Regards,
Mauro

Re: [PATCH] usbtv: fix dependency

[Hans Verkuil]

On Fri June 28 2013 15:55:15 Mauro Carvalho Chehab wrote:
> Em Fri, 28 Jun 2013 15:24:20 +0200 (CEST)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> escreveu:
> 
> > Hi Hans, Mauro
> > 
> > On Fri, 28 Jun 2013, Hans Verkuil wrote:
> > 
> > > On Fri June 28 2013 14:42:46 Mauro Carvalho Chehab wrote:
> > > > Em Fri, 28 Jun 2013 13:18:44 +0200
> > > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > > > 
> > > > > On Fri June 28 2013 13:00:43 Mauro Carvalho Chehab wrote:
> > > > > > Em Fri, 28 Jun 2013 10:24:15 +0200
> > > > > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > > > > > 
> > > > > > > This fixes a dependency problem as found by Randy Dunlap:
> > > > > > > 
> > > > > > > https://lkml.org/lkml/2013/6/27/501
> > > > > > > 
> > > > > > > Mauro, is there any reason for any V4L2 driver to depend on VIDEO_DEV instead of
> > > > > > > just VIDEO_V4L2?
> > > > > > > 
> > > > > > > Some drivers depend on VIDEO_DEV, some on VIDEO_V4L2, some on both. It's all
> > > > > > > pretty chaotic.
> > > > > > 
> > > > > > It should be noticed that, despite its name, this config is actually a
> > > > > > joint dependency of VIDEO_DEV and I2C that will compile drivers as module
> > > > > > if either I2C or VIDEO_DEV is a module:
> > > > > > 
> > > > > > 	config VIDEO_V4L2
> > > > > > 		tristate
> > > > > > 		depends on (I2C || I2C=n) && VIDEO_DEV
> > > > > > 		default (I2C || I2C=n) && VIDEO_DEV
> > > > > > 
> > > > > > So, a V4L2 device that doesn't have any I2C device doesn't need to depend
> > > > > > on VIDEO_V4L2. That includes, for example, reversed-engineered webcam
> > > > > > drivers where the sensor code is inside the driver and a few capture-only
> > > > > > device drivers.
> > > > > 
> > > > > Yes, it does have to depend on it. That's exactly why usbtv is failing: like
> > > > > any other v4l2 driver usbtv needs the videodev.ko module. That is dependent
> > > > > on VIDEO_V4L2. What is happening here is that the dependency of usbtv on
> > > > > VIDEO_DEV allows it to be built as part of the kernel, but VIDEO_V4L2 is built
> > > > > as a module due to its I2C dependency with the result that usbtv can't link to
> > > > > the videodev functions.
> > > > > 
> > > > > The way things are today I do not believe any v4l2 driver should depend on
> > > > > VIDEO_DEV, instead they should all depend on VIDEO_V4L2. That would make a
> > > > > lot more sense.
> > > > 
> > > > Hmm...
> > > > 
> > > > $ git grep -l i2c drivers/media/v4l2-core/
> > > > drivers/media/v4l2-core/tuner-core.c		(not part of videodev.ko module)
> > > > drivers/media/v4l2-core/v4l2-async.c
> > > > drivers/media/v4l2-core/v4l2-common.c
> > > > drivers/media/v4l2-core/v4l2-ctrls.c		(actually, there's just a comment there)
> > > > drivers/media/v4l2-core/v4l2-device.c
> > > > 
> > > > $ git grep  CONFIG_I2C drivers/media/v4l2-core/
> > > > drivers/media/v4l2-core/v4l2-common.c:#if IS_ENABLED(CONFIG_I2C)
> > > > drivers/media/v4l2-core/v4l2-common.c:#endif /* defined(CONFIG_I2C) */
> > > > drivers/media/v4l2-core/v4l2-device.c:#if IS_ENABLED(CONFIG_I2C)
> > > > 
> > > > yes, there are some parts of videodev that are dependent on I2C.
> > > > 
> > > > That's basically why all V4L2 drivers should depend on VIDEO_V4L2.
> > > > 
> > > > That's said, before the addition of v4l2-async, it was safe to compile
> > > > the core without I2C, as the parts of the code that are I2C specific are
> > > > protected by a:
> > > > 	#if defined(CONFIG_I2C)
> > > > 
> > > > With its addition, I suspect that we'll still have Kbuild issues, if I2C
> > > > is disabled and a driver that doesn't depends on I2C is compiled.
> > > > 
> > > > So, 2 patches seem to be needed:
> > > > 
> > > > 1) a patch that replaces all driver dependencies from CONFIG_DEV to
> > > >    CONFIG_V4L2;
> > > > 
> > > > 2) a patch that fixes the issues with v4l2-async.
> > > > 
> > > > With regards to the last one, I can see 3 ways to fix it:
> > > > 	1) don't add v4l2-async at videodev.ko if I2C is not selected;
> > > > 	2) protect the I2C specific parts of v4l2-async with
> > > > 		#if defined(CONFIG_I2C)
> > > > 	3) put v4l2-async on a separate module.
> > > > 
> > > > (or some combination of the above)
> > > > 
> > > > As only very few drivers use v4l2-async, as this is more focused to
> > > > fix troubles with OT, I think that the better would be to do (3) and
> > > > to add an specific Kconfig entry for it.
> > > 
> > > No, #2 is the right choice here. That's necessary anyway since it is a valid
> > > use-case that I2C is disabled and you use it for platform devices only.
> 
> True, if are there any case where a platform kernel would be compiled without
> I2C. I can't figure out any of such cases where such a weird config would
> make sense.
> 
> > > 
> > > Guennadi, can you look at this? The only thing that is probably needed is
> > > that match_i2c returns false if CONFIG_I2C is undefined.
> > > 
> > > I prefer to keep this part of videodev, at least for now: I think there will
> > > be a fairly quick uptake of this API internally, certainly for subdevs. Note
> > > BTW that even x86 kernels come with CONFIG_OF enabled these days.
> 
> Fedora 18 (and even Fedora 19 beta) Kernels don't enable it for x86. It
> is enabled there only for arm:
> 	config-armv7-generic:CONFIG_OF=y
> 	config-armv7-generic:CONFIG_OF_DEVICE=y
> 
> I don't doubt that this would be enabled on x86 there some day, but
> this is not the current status, and I fail to see why one would enable
> it on x86, except if some specific distro have some specific issues
> at their boot loaders that would be easier to solve if OF is enabled
> on all of their kernels.

Debian sid turns it on, I think it is for some intel-based embedded systems.

> From my side, I don't see much sense of overbloating the videodev core
> with platform-specific code.

About half of this source is necessary anyway for subdevs that need to
be async-aware. Note that v4l2-async is by itself unrelated to CONFIG_OF.
It can be used by any driver that wants to asynchronously load i2c drivers.

> 
> > 
> > This patch
> > 
> > http://git.linuxtv.org/gliakhovetski/v4l-dvb.git/commitdiff/a92d0222c693db29a5d00eaedcdebf748789c38e
> > 
> > has been pushed 3 days ago:
> > 
> > https://patchwork.linuxtv.org/patch/19090/
> 
> As, according with:
> 	https://lwn.net/Articles/556034/rss
> 
> -rc7 is likely the last one before 3.10, that means that the media merge
> window for 3.11 is closed already (as we close it one week before, in order
> to give more time for reviewing the patches better at -next).

To make it easier for us submaintainers (and others as well, for that matter),
can you post a message next time when you close the media merge window?

I realized that because of this I need to split up my pull request as well :-(

Regards,

	Hans

> So, please split the fix patches from it on a separate pull request.

Re: [PATCH] usbtv: fix dependency

[Mauro Carvalho Chehab]

Em Fri, 28 Jun 2013 16:15:29 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> > > > I prefer to keep this part of videodev, at least for now: I think there will
> > > > be a fairly quick uptake of this API internally, certainly for subdevs. Note
> > > > BTW that even x86 kernels come with CONFIG_OF enabled these days.
> > 
> > Fedora 18 (and even Fedora 19 beta) Kernels don't enable it for x86. It
> > is enabled there only for arm:
> > 	config-armv7-generic:CONFIG_OF=y
> > 	config-armv7-generic:CONFIG_OF_DEVICE=y
> > 
> > I don't doubt that this would be enabled on x86 there some day, but
> > this is not the current status, and I fail to see why one would enable
> > it on x86, except if some specific distro have some specific issues
> > at their boot loaders that would be easier to solve if OF is enabled
> > on all of their kernels.
> 
> Debian sid turns it on,

Weird. 

> I think it is for some intel-based embedded systems.

Ok, then, it could make sense, if they want to use the same Kernel for both
embedded and non-embedded systems.

> > From my side, I don't see much sense of overbloating the videodev core
> > with platform-specific code.
> 
> About half of this source is necessary anyway for subdevs that need to
> be async-aware. Note that v4l2-async is by itself unrelated to CONFIG_OF.
> It can be used by any driver that wants to asynchronously load i2c drivers.

Making a V4L2 driver to initialize synchronously is already too complex,
and it takes some time until the code there becomes stable. I don't think
a typical driver should be using the async API, as this will just add
unneeded complexity to an already-complex-enough task.

So, yes, in thesis, this API could be used by non-OF driver, but
in practice, what's the gain of doing it? 

Very few hardware have more than one I2C buses, so, the access to the 
subdevs will be serialized anyway.

For a serialized I2C bus, the async API doesn't bring any benefit, and
may actually hurt the drivers, as the driver/hardware may have hard-to-track
bugs if initialized on certain specific infrequent init sequence.

The tm6000 chips, for example, have this issue: there are some parts 
there that, if you change the order where the registers are written, the
hardware crashes - it is believed to be a bug at its internal 8051
firmware.

Even the few hardware that have more than one I2C buses, where a parallel
init could make the hardware to initialize faster, it may have troubles
with parallel initialization, due to I2C gateways, I2C clock registers,
firmwares, etc. Allowing async init there could be a real nightmare,
any may require it to be serialized, anyway.

So, I see no benefit on having this code to be mandatory for a non-OF
hardware. In contrary, it would just increase the code size and make
debugging harder for nothing.

Cheers,
Mauro

Re: [PATCH] usbtv: fix dependency

[Mauro Carvalho Chehab]

Em Fri, 28 Jun 2013 10:55:15 -0300
Mauro Carvalho Chehab <mchehab@redhat.com> escreveu:

> > This patch
> > 
> > http://git.linuxtv.org/gliakhovetski/v4l-dvb.git/commitdiff/a92d0222c693db29a5d00eaedcdebf748789c38e
> > 
> > has been pushed 3 days ago:
> > 
> > https://patchwork.linuxtv.org/patch/19090/
> 
> As, according with:
> 	https://lwn.net/Articles/556034/rss
> 
> -rc7 is likely the last one before 3.10, that means that the media merge
> window for 3.11 is closed already (as we close it one week before, in order
> to give more time for reviewing the patches better at -next).
> 
> So, please split the fix patches from it on a separate pull request.

Hmm... just took a look at the actual pull request... from the description,
it seems to contain just fixes/documentation, so, I'll be handling it
in a few.

Regards,
Mauro
-- 

Cheers,
Mauro

Re: [PATCH] usbtv: fix dependency

[Randy Dunlap]

On 06/28/13 10:54, Mauro Carvalho Chehab wrote:
> Em Fri, 28 Jun 2013 10:55:15 -0300
> Mauro Carvalho Chehab <mchehab@redhat.com> escreveu:
> 
>>> This patch
>>>
>>> http://git.linuxtv.org/gliakhovetski/v4l-dvb.git/commitdiff/a92d0222c693db29a5d00eaedcdebf748789c38e
>>>
>>> has been pushed 3 days ago:
>>>
>>> https://patchwork.linuxtv.org/patch/19090/
>>
>> As, according with:
>> 	https://lwn.net/Articles/556034/rss
>>
>> -rc7 is likely the last one before 3.10, that means that the media merge
>> window for 3.11 is closed already (as we close it one week before, in order
>> to give more time for reviewing the patches better at -next).
>>
>> So, please split the fix patches from it on a separate pull request.
> 
> Hmm... just took a look at the actual pull request... from the description,
> it seems to contain just fixes/documentation, so, I'll be handling it
> in a few.

Has this patch been merged yet?  If so, where?

This build failure is still occurring in linux-next-20130703.


thanks,

-- 
~Randy