Mauro Carvalho Chehab wrote:>>> I don't like to create a video-buf.h header. This will make non-pci>>> devices dependent on PCI, or will require some additional logic for>>> checking kernel Kconfig symbols. I also expect that other newer videobuf>>> methods to be created. So, this header will just generate undesirable>>> mess.>>> >> This would not create dependencies of non-pci devices on pci -- a>> simple header inclusion is optimized by the c compiler -- only needed>> methods are considered and are actually included in the build.>> >> Wrong. This has nothing to do with c optimizer. Try this:>> Create this as header.h:>> struct foo {> struct some_pci_struct foo;> };>> Create this as main.c:>> #include "header.h">> int main (void)> {> return 0;> }>> You will got the following error:>> /tmp/header.h:2: error: field ‘foo’ has incomplete type>> If we use your approach, a non-pci videobuf driver will work only at the> architectures where you have a PCI bus (since the pci structs won't> exist on that architecture).> OK, I wasn't thinking clearly when I had written that at first... We would have to use some #ifdef logic within video-buf.h in order to avoid this, but that's ugly, and more trouble than it's worth.
You've proven your point.> >>> What we might do is to rename the generic abstract method to another>>> name. This will generate compilation errors, making easier for people to>>> realize what happened.>>> >> If we rename it to video-buf.h, it would cover the issue that I have inmind.>>>> > It is much more clear to include videobuf-vmalloc or videobuf-dma-sg,> depending on the proper videobuf module that will be needed by a driver.>> Creating a video-buf.h that just includes the two will be unused by the> drivers. So, what's the sense of creating a header file at the kernel> that aren't used inside kernel?>
It would just be nice to have errors when a module calls a function defined in the old header, rather than the new one.
This is a concern, but a minor one, as this is only an issue during the transitional kernels. If we've fixed all instances, then there should be nothing to worry about.
In general, I just don't like modules building against the wrong headers and not reporting any errors... I guess we'll just have to live with it this way for now.
> >>> I'm not sure if this is valuable enough, since I don't know any other>>> DMA S/G driver using videobuf being developed for their inclusion at>>> Kernel.>>> >> Maybe an out of tree driver uses it?>> >> Although there's no intention to make life harder for out-of-tree> drivers, they shouldn't be considered on changes made at kernel> internals. The proper place for a kernel driver is in-kernel, not> outside.>> Anyway, a compilation with a driver including video-buf.h currently will> generate an error, indicating that something has changed at v4l> infrastructure. By creating a header like you're proposing won't fix> this.> >> Maybe there is an in-tree driver that still might have old methodsbeing used but we didnt hit those bugs yet due to incomplete testingmethods?>> >> The only in-kernel drivers that use videobuf infrastructure are:> cx88, saa7134, bttv, saa7146 and, now, cx23885. >> After the patch, all of they are already converted.>> grep videobuf_queue_pci_init *.c> bttv-driver.c: videobuf_queue_pci_init(&fh->cap, &bttv_video_qops,> bttv-driver.c: videobuf_queue_pci_init(&fh->vbi, &bttv_vbi_qops,> cx23885-dvb.c: videobuf_queue_pci_init(&port->dvb.dvbq, &dvb_qops,> dev->pci, &port->slock,> cx88-blackbird.c: videobuf_queue_pci_init(&fh->mpegq,> &blackbird_qops,> cx88-dvb.c: videobuf_queue_pci_init(&dev->dvb.dvbq, &dvb_qops,> cx88-video.c: videobuf_queue_pci_init(&fh->vidq, &cx8800_video_qops,> cx88-video.c: videobuf_queue_pci_init(&fh->vbiq, &cx8800_vbi_qops,> saa7134-dvb.c: videobuf_queue_pci_init(&dev->dvb.dvbq,> &saa7134_ts_qops,> saa7134-empress.c: videobuf_queue_pci_init(&dev->empress_tsq,> &saa7134_ts_qops,> saa7134-video.c: videobuf_queue_pci_init(&fh->cap, &video_qops,> saa7134-video.c: videobuf_queue_pci_init(&fh->vbi,> &saa7134_vbi_qops,> saa7146_vbi.c: videobuf_queue_pci_init(&fh->vbi_q, &vbi_qops,> saa7146_video.c: videobuf_queue_pci_init(&fh->video_q,> &video_qops,> videobuf-dma-sg.c:void videobuf_queue_pci_init(struct videobuf_queue* q,>> There's no other driver using the abstract videobuf_queue_init.> OK, we should be fine, then.> A final point for your consideration:>> videobuf_queue_init is the only function that had its meaning changed> without changing its parameters (currently, it is just an abstract> method. In the past, this were the function that were used to initialize> a videobuf queue). >> The changes you're proposing won't solve the target you've addressed in> the first place, since it will still compile without warning, if you> forget to rename it to videobuf_queue_pci_init.>> The proper change is simply doing something like:>> s/videobuf_queue_init/videobuf_queue_abstract_init/>> After this, on all places where videobuf stuff is used without> conversion, an error will be generated.> This wouldn't be a bad idea.> After my considerations, if you still think this is important, I'll> accept a patch renaming the old videobuf_queue_init to a new name.>
I think that you've proven that it is NOT important... although, it still might be a good idea. I'm not sure -- I was only making a suggestion.
Cheers,
Mike
_______________________________________________linux-dvb mailing listlinux-dvb@xxxxxxxxxxxxxxx://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
