* [PATCH] usbtv: fix dependency @ 2013-06-28 8:24 Hans Verkuil 2013-06-28 11:00 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 11+ messages in thread From: Hans Verkuil @ 2013-06-28 8:24 UTC (permalink / raw) To: linux-media, Mauro Carvalho Chehab; +Cc: Randy Dunlap 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--- ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] usbtv: fix dependency 2013-06-28 8:24 [PATCH] usbtv: fix dependency Hans Verkuil @ 2013-06-28 11:00 ` Mauro Carvalho Chehab 2013-06-28 11:18 ` Hans Verkuil 0 siblings, 1 reply; 11+ messages in thread From: Mauro Carvalho Chehab @ 2013-06-28 11:00 UTC (permalink / raw) To: Hans Verkuil; +Cc: linux-media, Randy Dunlap 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usbtv: fix dependency 2013-06-28 11:00 ` Mauro Carvalho Chehab @ 2013-06-28 11:18 ` Hans Verkuil 2013-06-28 12:42 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 11+ messages in thread From: Hans Verkuil @ 2013-06-28 11:18 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: linux-media, Randy Dunlap 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--- > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usbtv: fix dependency 2013-06-28 11:18 ` Hans Verkuil @ 2013-06-28 12:42 ` Mauro Carvalho Chehab 2013-06-28 12:59 ` Hans Verkuil 0 siblings, 1 reply; 11+ messages in thread From: Mauro Carvalho Chehab @ 2013-06-28 12:42 UTC (permalink / raw) To: Hans Verkuil; +Cc: linux-media, Randy Dunlap, Guennadi Liakhovetski 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usbtv: fix dependency 2013-06-28 12:42 ` Mauro Carvalho Chehab @ 2013-06-28 12:59 ` Hans Verkuil 2013-06-28 13:24 ` Guennadi Liakhovetski 0 siblings, 1 reply; 11+ messages in thread From: Hans Verkuil @ 2013-06-28 12:59 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: linux-media, Randy Dunlap, Guennadi Liakhovetski 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usbtv: fix dependency 2013-06-28 12:59 ` Hans Verkuil @ 2013-06-28 13:24 ` Guennadi Liakhovetski 2013-06-28 13:55 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 11+ messages in thread From: Guennadi Liakhovetski @ 2013-06-28 13:24 UTC (permalink / raw) To: Hans Verkuil; +Cc: Mauro Carvalho Chehab, linux-media, Randy Dunlap 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/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usbtv: fix dependency 2013-06-28 13:24 ` Guennadi Liakhovetski @ 2013-06-28 13:55 ` Mauro Carvalho Chehab 2013-06-28 14:15 ` Hans Verkuil 2013-06-28 17:54 ` Mauro Carvalho Chehab 0 siblings, 2 replies; 11+ messages in thread From: Mauro Carvalho Chehab @ 2013-06-28 13:55 UTC (permalink / raw) To: Guennadi Liakhovetski, Hans Verkuil; +Cc: linux-media, Randy Dunlap 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usbtv: fix dependency 2013-06-28 13:55 ` Mauro Carvalho Chehab @ 2013-06-28 14:15 ` Hans Verkuil 2013-06-28 15:32 ` Mauro Carvalho Chehab 2013-06-28 17:54 ` Mauro Carvalho Chehab 1 sibling, 1 reply; 11+ messages in thread From: Hans Verkuil @ 2013-06-28 14:15 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: Guennadi Liakhovetski, linux-media, Randy Dunlap 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usbtv: fix dependency 2013-06-28 14:15 ` Hans Verkuil @ 2013-06-28 15:32 ` Mauro Carvalho Chehab 0 siblings, 0 replies; 11+ messages in thread From: Mauro Carvalho Chehab @ 2013-06-28 15:32 UTC (permalink / raw) To: Hans Verkuil; +Cc: Guennadi Liakhovetski, linux-media, Randy Dunlap 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usbtv: fix dependency 2013-06-28 13:55 ` Mauro Carvalho Chehab 2013-06-28 14:15 ` Hans Verkuil @ 2013-06-28 17:54 ` Mauro Carvalho Chehab 2013-07-03 17:26 ` Randy Dunlap 1 sibling, 1 reply; 11+ messages in thread From: Mauro Carvalho Chehab @ 2013-06-28 17:54 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: Hans Verkuil, linux-media, Randy Dunlap 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usbtv: fix dependency 2013-06-28 17:54 ` Mauro Carvalho Chehab @ 2013-07-03 17:26 ` Randy Dunlap 0 siblings, 0 replies; 11+ messages in thread From: Randy Dunlap @ 2013-07-03 17:26 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: Guennadi Liakhovetski, Hans Verkuil, linux-media 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-07-03 17:27 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-06-28 8:24 [PATCH] usbtv: fix dependency Hans Verkuil 2013-06-28 11:00 ` Mauro Carvalho Chehab 2013-06-28 11:18 ` Hans Verkuil 2013-06-28 12:42 ` Mauro Carvalho Chehab 2013-06-28 12:59 ` Hans Verkuil 2013-06-28 13:24 ` Guennadi Liakhovetski 2013-06-28 13:55 ` Mauro Carvalho Chehab 2013-06-28 14:15 ` Hans Verkuil 2013-06-28 15:32 ` Mauro Carvalho Chehab 2013-06-28 17:54 ` Mauro Carvalho Chehab 2013-07-03 17:26 ` Randy Dunlap
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.