Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-staging
Hans Verkuil wrote: On Friday 20 November 2009 22:06:02 Devin Heitmueller wrote: On Fri, Nov 20, 2009 at 3:38 PM, Hans Verkuil hverk...@xs4all.nl wrote: Hi Mauro, Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-staging for the following: - Enable staging drivers by default when building v4l-dvb - go7007: Add struct v4l2_device. - s2250: Change module structure - s2250: subdev conversion - go7007: subdev conversion I have to admit that I am not sure that enabling staging drivers by default is a good idea. Staging drivers can be highly unstable, and can potentially damage hardware. I can totally imagine less experienced users with one of these devices building the current code and then being confused why their hardware is detected but is totally broken, or worse becomes damaged. If there are drivers in the staging tree that are so unreliable that they can break the hardware, then those should be explicitly disabled, rather than disabling all drivers in the staging tree. Or perhaps do not belong there at all, or belong under the CONFIG_STAGING_BROKEN option. A driver like the go7007 is under active development, and it does work. It only needs more cleanup before it can be moved to drivers/media/video, so there was no reason that it should be disabled. Mauro, what are the risks of always compiling the tm6000 and cx25821 drivers? Let me know if you think that either one or both should be disabled for now and I'll make a patch for it. Even on upstream, drivers in staging are not compiled by default. To enable them, you need to answer yes to two questions. If the driver is OK, it shouldn't be in staging. The drivers that are in staging have issues that may be just coding style, can be the usage of wrong API's, or can be something serious. Also, since the criteria for a driver to be in staging are weak, they may not be prepared to be used widely, since they is likely doing some evil things, like duplicating existing code or creating non-accepted API's. In the go7007 case, among other things, the driver duplicates existing code at the saa7115 driver still uses the already deprecated DECODER ioctl's, and creates its own API (see go7007.h). So, I'm against of enabling any staging drivers to be compiled by default. Of course, in the case of auto-compilation tests tools like what you have, it is valuable if you add a patch there that enables their compilations inside the tool, but such patch shouldn't be committed at the development tree, simply because staging drivers aren't ready yet. The developers and testers of the staging drivers should manually enable it, after being warned about the risks of using them. By not compiling you run the very high risk of bit rot: code getting seriously out-of-sync with the rest of the tree. Possibly requiring a lot of work later. If the driver is being maintained, this risk is low, since the driver maintainer will take care of it. It helps if your automatic build scripts could point that the driver compilation broke for some kernels, but, as the driver is being prepared for upstream submission, the developer should already being using the latest -rc kernel for its development. If they aren't maintained, they'll be removed from staging tree on a few kernel cycles, so patches fixing the compilation for those unmaintained drivers just means that someone lost his time fixing a dead driver. Cheers, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-staging
Hans Verkuil wrote: On Friday 20 November 2009 22:06:02 Devin Heitmueller wrote: On Fri, Nov 20, 2009 at 3:38 PM, Hans Verkuil hverk...@xs4all.nl wrote: Hi Mauro, Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-staging for the following: - Enable staging drivers by default when building v4l-dvb - go7007: Add struct v4l2_device. - s2250: Change module structure - s2250: subdev conversion - go7007: subdev conversion I have to admit that I am not sure that enabling staging drivers by default is a good idea. Staging drivers can be highly unstable, and can potentially damage hardware. I can totally imagine less experienced users with one of these devices building the current code and then being confused why their hardware is detected but is totally broken, or worse becomes damaged. If there are drivers in the staging tree that are so unreliable that they can break the hardware, then those should be explicitly disabled, rather than disabling all drivers in the staging tree. Or perhaps do not belong there at all, or belong under the CONFIG_STAGING_BROKEN option. A driver like the go7007 is under active development, and it does work. It only needs more cleanup before it can be moved to drivers/media/video, so there was no reason that it should be disabled. Mauro, what are the risks of always compiling the tm6000 and cx25821 drivers? Let me know if you think that either one or both should be disabled for now and I'll make a patch for it. Even on upstream, drivers in staging are not compiled by default. To enable them, you need to answer yes to two questions. If the driver is OK, it shouldn't be in staging. The drivers that are in staging have issues that may be just coding style, can be the usage of wrong API's, or can be something serious. Also, since the criteria for a driver to be in staging are weak, they may not be prepared to be used widely, since they is likely doing some evil things, like duplicating existing code or creating non-accepted API's. In the go7007 case, among other things, the driver duplicates existing code at the saa7115 driver still uses the already deprecated DECODER ioctl's, and creates its own API (see go7007.h). So, I'm against of enabling any staging drivers to be compiled by default. Of course, in the case of auto-compilation tests tools like what you have, it is valuable if you add a patch there that enables their compilations inside the tool, but such patch shouldn't be committed at the development tree, simply because staging drivers aren't ready yet. The developers and testers of the staging drivers should manually enable it, after being warned about the risks of using them. By not compiling you run the very high risk of bit rot: code getting seriously out-of-sync with the rest of the tree. Possibly requiring a lot of work later. If the driver is being maintained, this risk is low, since the driver maintainer will take care of it. It helps if your automatic build scripts could point that the driver compilation broke for some kernels, but, as the driver is being prepared for upstream submission, the developer should already being using the latest -rc kernel for its development. If they aren't maintained, they'll be removed from staging tree on a few kernel cycles, so patches fixing the compilation for those unmaintained drivers just means that someone lost his time fixing a dead driver. I've reverted that patch and respun my v4l-dvb-staging tree. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-staging
Hans Verkuil wrote: Hans Verkuil wrote: On Friday 20 November 2009 22:06:02 Devin Heitmueller wrote: On Fri, Nov 20, 2009 at 3:38 PM, Hans Verkuil hverk...@xs4all.nl wrote: Hi Mauro, Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-staging for the following: - Enable staging drivers by default when building v4l-dvb - go7007: Add struct v4l2_device. - s2250: Change module structure - s2250: subdev conversion - go7007: subdev conversion I have to admit that I am not sure that enabling staging drivers by default is a good idea. Staging drivers can be highly unstable, and can potentially damage hardware. I can totally imagine less experienced users with one of these devices building the current code and then being confused why their hardware is detected but is totally broken, or worse becomes damaged. If there are drivers in the staging tree that are so unreliable that they can break the hardware, then those should be explicitly disabled, rather than disabling all drivers in the staging tree. Or perhaps do not belong there at all, or belong under the CONFIG_STAGING_BROKEN option. A driver like the go7007 is under active development, and it does work. It only needs more cleanup before it can be moved to drivers/media/video, so there was no reason that it should be disabled. Mauro, what are the risks of always compiling the tm6000 and cx25821 drivers? Let me know if you think that either one or both should be disabled for now and I'll make a patch for it. Even on upstream, drivers in staging are not compiled by default. To enable them, you need to answer yes to two questions. If the driver is OK, it shouldn't be in staging. The drivers that are in staging have issues that may be just coding style, can be the usage of wrong API's, or can be something serious. Also, since the criteria for a driver to be in staging are weak, they may not be prepared to be used widely, since they is likely doing some evil things, like duplicating existing code or creating non-accepted API's. In the go7007 case, among other things, the driver duplicates existing code at the saa7115 driver still uses the already deprecated DECODER ioctl's, and creates its own API (see go7007.h). So, I'm against of enabling any staging drivers to be compiled by default. Of course, in the case of auto-compilation tests tools like what you have, it is valuable if you add a patch there that enables their compilations inside the tool, but such patch shouldn't be committed at the development tree, simply because staging drivers aren't ready yet. The developers and testers of the staging drivers should manually enable it, after being warned about the risks of using them. By not compiling you run the very high risk of bit rot: code getting seriously out-of-sync with the rest of the tree. Possibly requiring a lot of work later. If the driver is being maintained, this risk is low, since the driver maintainer will take care of it. It helps if your automatic build scripts could point that the driver compilation broke for some kernels, but, as the driver is being prepared for upstream submission, the developer should already being using the latest -rc kernel for its development. If they aren't maintained, they'll be removed from staging tree on a few kernel cycles, so patches fixing the compilation for those unmaintained drivers just means that someone lost his time fixing a dead driver. I've reverted that patch and respun my v4l-dvb-staging tree. Thanks. I should be pulling from all pending requests today, after finishing the pending stuff at patchwork. Cheers, Mauro. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-staging
On Fri, Nov 20, 2009 at 4:45 PM, Hans Verkuil hverk...@xs4all.nl wrote: If there are drivers in the staging tree that are so unreliable that they can break the hardware, then those should be explicitly disabled, rather than disabling all drivers in the staging tree. Or perhaps do not belong there at all, or belong under the CONFIG_STAGING_BROKEN option. A driver like the go7007 is under active development, and it does work. It only needs more cleanup before it can be moved to drivers/media/video, so there was no reason that it should be disabled. Mauro, what are the risks of always compiling the tm6000 and cx25821 drivers? Let me know if you think that either one or both should be disabled for now and I'll make a patch for it. I'm certainly much more worried about the tm6000 stuff than the cx25821, primarily because the cx25821 is pretty rare and the tm6000 is used by several fairly popular consumer products, but is completely broken. I agree that we should be periodically ensuring the modules still compile, but I think that by default they should need to be explicitly enabled by a developer who knows what he/she is doing and understands the ramifications/risks. By not compiling you run the very high risk of bit rot: code getting seriously out-of-sync with the rest of the tree. Possibly requiring a lot of work later. Our tree is primarily for developers, not for end-users. So we need to see any code breakages as early as possible. Sure, in a perfect world that would be true. In reality though, I'm confident that a *huge* percentage of people who compile the v4l-dvb code have absolutely no idea what the hell they are doing. They are end-users who just want to see their device work because the changes haven't made it into their distro yet. I can certainly appreciate the concerns about the bit-rot issue. I am just worried that perhaps we set the bar too low in terms of what got into staging if it's going to be compiled by default. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
