| Message ID | 201308052319.26720.linux@rainbow-software.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 08/05/2013 11:19 PM, Ondrej Zary wrote: > Hello, > the in-kernel stkwebcam driver (by Jaime Velasco Juan and Nicolas VIVIEN) > supports only two webcam types (USB IDs 0x174f:0xa311 and 0x05e1:0x0501). > There are many other Syntek webcam types that are not supported by this > driver (such as 0x174f:0x6a31 in Asus F5RL laptop). > > There is an out-of-tree GPL driver called stk11xx (by Martin Roos and also > Nicolas VIVIEN) at http://sourceforge.net/projects/syntekdriver/ which > supports more webcams. It can be even compiled for the latest kernels using > the patch below and seems to work somehow (slow and buggy but better than > nothing) with the Asus F5RL. I took a quick look and there are a number of issues with this driver: 1) It conflicts usb-id wise with the new stk1160 driver (which supports usb-id 05e1:0408) so support for that usb-id, and any code only used for that id will need to be removed 2) "seems to work somehow (slow and buggy)" is not really the quality we aim for with in kernel drivers. We definitely will want to remove any usb-ids, and any code only used for those ids, where there is overlap with the existing stkwebcam driver, to avoid regressions 3) It does in kernel bayer decoding, this is not acceptable, it needs to be modified to produce buffers with raw bayer data (libv4l will take care of the bater decoding in userspace). 4) It is not using any of the new kernel infrastructure we have been adding over time, like the control-framework, videobuf2, etc. It would be best to convert this to a gspca sub driver (of which there are many already, which can serve as examples), so that it will use all the existing framework code. As a minimum issues 1-3 needs to be addressed before this can be merged. An alternative / better approach might be to simply only lift the code for your camera, and add a new gspca driver supporting only your camera. Either way since non of the v4l developers have a laptop which such a camera, you will need to do most of the work yourself, as we cannot test. So congratulations, you've just become a v4l kernel developer :) Regards, Hans > > Is there any possibility that this driver could be merged into the kernel? > The code could probably be simplified a lot and integrated into gspca. > > > diff -urp syntekdriver-code-107-trunk-orig/driver/stk11xx.h syntekdriver-code-107-trunk//driver/stk11xx.h > --- syntekdriver-code-107-trunk-orig/driver/stk11xx.h 2012-03-10 10:03:12.000000000 +0100 > +++ syntekdriver-code-107-trunk//driver/stk11xx.h 2013-08-05 22:50:00.000000000 +0200 > @@ -33,6 +33,7 @@ > > #ifndef STK11XX_H > #define STK11XX_H > +#include <media/v4l2-device.h> > > #define DRIVER_NAME "stk11xx" /**< Name of this driver */ > #define DRIVER_VERSION "v3.0.0" /**< Version of this driver */ > @@ -316,6 +317,7 @@ struct stk11xx_video { > * @struct usb_stk11xx > */ > struct usb_stk11xx { > + struct v4l2_device v4l2_dev; > struct video_device *vdev; /**< Pointer on a V4L2 video device */ > struct usb_device *udev; /**< Pointer on a USB device */ > struct usb_interface *interface; /**< Pointer on a USB interface */ > diff -urp syntekdriver-code-107-trunk-orig/driver/stk11xx-v4l.c syntekdriver-code-107-trunk//driver/stk11xx-v4l.c > --- syntekdriver-code-107-trunk-orig/driver/stk11xx-v4l.c 2012-03-10 09:54:57.000000000 +0100 > +++ syntekdriver-code-107-trunk//driver/stk11xx-v4l.c 2013-08-05 22:51:12.000000000 +0200 > @@ -1498,9 +1498,17 @@ int v4l_stk11xx_register_video_device(st > { > int err; > > + err = v4l2_device_register(&dev->interface->dev, &dev->v4l2_dev); > + if (err < 0) { > + STK_ERROR("couldn't register v4l2_device\n"); > + kfree(dev); > + return err; > + } > + > strcpy(dev->vdev->name, DRIVER_DESC); > > - dev->vdev->parent = &dev->interface->dev; > +// dev->vdev->parent = &dev->interface->dev; > + dev->vdev->v4l2_dev = &dev->v4l2_dev; > dev->vdev->fops = &v4l_stk11xx_fops; > dev->vdev->release = video_device_release; > dev->vdev->minor = -1; > @@ -1533,6 +1541,7 @@ int v4l_stk11xx_unregister_video_device( > > video_set_drvdata(dev->vdev, NULL); > video_unregister_device(dev->vdev); > + v4l2_device_unregister(&dev->v4l2_dev); > > return 0; > } > > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 06 August 2013, Hans de Goede wrote: > Hi, > > On 08/05/2013 11:19 PM, Ondrej Zary wrote: > > Hello, > > the in-kernel stkwebcam driver (by Jaime Velasco Juan and Nicolas VIVIEN) > > supports only two webcam types (USB IDs 0x174f:0xa311 and 0x05e1:0x0501). > > There are many other Syntek webcam types that are not supported by this > > driver (such as 0x174f:0x6a31 in Asus F5RL laptop). > > > > There is an out-of-tree GPL driver called stk11xx (by Martin Roos and > > also Nicolas VIVIEN) at http://sourceforge.net/projects/syntekdriver/ > > which supports more webcams. It can be even compiled for the latest > > kernels using the patch below and seems to work somehow (slow and buggy > > but better than nothing) with the Asus F5RL. > > I took a quick look and there are a number of issues with this driver: > > 1) It conflicts usb-id wise with the new stk1160 driver (which supports > usb-id 05e1:0408) so support for that usb-id, and any code only used for > that id will need to be removed > > 2) "seems to work somehow (slow and buggy)" is not really the quality > we aim for with in kernel drivers. We definitely will want to remove > any usb-ids, and any code only used for those ids, where there is overlap > with the existing stkwebcam driver, to avoid regressions > > 3) It does in kernel bayer decoding, this is not acceptable, it needs to > be modified to produce buffers with raw bayer data (libv4l will take care > of the bater decoding in userspace). > > 4) It is not using any of the new kernel infrastructure we have been adding > over time, like the control-framework, videobuf2, etc. It would be best > to convert this to a gspca sub driver (of which there are many already, > which can serve as examples), so that it will use all the existing > framework code. Yes, this would be the best way - only extract the HW-dependent parts. > As a minimum issues 1-3 needs to be addressed before this can be merged. An > alternative / better approach might be to simply only lift the code for > your camera, and add a new gspca driver supporting only your camera. > > Either way since non of the v4l developers have a laptop which such a > camera, you will need to do most of the work yourself, as we cannot test. > > So congratulations, you've just become a v4l kernel developer :) Unfortunately the laptop isn't mine. I'll have it only for a while but will try to do something. > Regards, > > Hans > > > Is there any possibility that this driver could be merged into the > > kernel? The code could probably be simplified a lot and integrated into > > gspca. > > > > > > diff -urp syntekdriver-code-107-trunk-orig/driver/stk11xx.h > > syntekdriver-code-107-trunk//driver/stk11xx.h --- > > syntekdriver-code-107-trunk-orig/driver/stk11xx.h 2012-03-10 > > 10:03:12.000000000 +0100 +++ > > syntekdriver-code-107-trunk//driver/stk11xx.h 2013-08-05 > > 22:50:00.000000000 +0200 @@ -33,6 +33,7 @@ > > > > #ifndef STK11XX_H > > #define STK11XX_H > > +#include <media/v4l2-device.h> > > > > #define DRIVER_NAME "stk11xx" /**< Name of this driver */ > > #define DRIVER_VERSION "v3.0.0" /**< Version of this driver */ > > @@ -316,6 +317,7 @@ struct stk11xx_video { > > * @struct usb_stk11xx > > */ > > struct usb_stk11xx { > > + struct v4l2_device v4l2_dev; > > struct video_device *vdev; /**< Pointer on a V4L2 video device */ > > struct usb_device *udev; /**< Pointer on a USB device */ > > struct usb_interface *interface; /**< Pointer on a USB interface */ > > diff -urp syntekdriver-code-107-trunk-orig/driver/stk11xx-v4l.c > > syntekdriver-code-107-trunk//driver/stk11xx-v4l.c --- > > syntekdriver-code-107-trunk-orig/driver/stk11xx-v4l.c 2012-03-10 > > 09:54:57.000000000 +0100 +++ > > syntekdriver-code-107-trunk//driver/stk11xx-v4l.c 2013-08-05 > > 22:51:12.000000000 +0200 @@ -1498,9 +1498,17 @@ int > > v4l_stk11xx_register_video_device(st > > { > > int err; > > > > + err = v4l2_device_register(&dev->interface->dev, &dev->v4l2_dev); > > + if (err < 0) { > > + STK_ERROR("couldn't register v4l2_device\n"); > > + kfree(dev); > > + return err; > > + } > > + > > strcpy(dev->vdev->name, DRIVER_DESC); > > > > - dev->vdev->parent = &dev->interface->dev; > > +// dev->vdev->parent = &dev->interface->dev; > > + dev->vdev->v4l2_dev = &dev->v4l2_dev; > > dev->vdev->fops = &v4l_stk11xx_fops; > > dev->vdev->release = video_device_release; > > dev->vdev->minor = -1; > > @@ -1533,6 +1541,7 @@ int v4l_stk11xx_unregister_video_device( > > > > video_set_drvdata(dev->vdev, NULL); > > video_unregister_device(dev->vdev); > > + v4l2_device_unregister(&dev->v4l2_dev); > > > > return 0; > > }
diff -urp syntekdriver-code-107-trunk-orig/driver/stk11xx.h syntekdriver-code-107-trunk//driver/stk11xx.h --- syntekdriver-code-107-trunk-orig/driver/stk11xx.h 2012-03-10 10:03:12.000000000 +0100 +++ syntekdriver-code-107-trunk//driver/stk11xx.h 2013-08-05 22:50:00.000000000 +0200 @@ -33,6 +33,7 @@ #ifndef STK11XX_H #define STK11XX_H +#include <media/v4l2-device.h> #define DRIVER_NAME "stk11xx" /**< Name of this driver */ #define DRIVER_VERSION "v3.0.0" /**< Version of this driver */ @@ -316,6 +317,7 @@ struct stk11xx_video { * @struct usb_stk11xx */ struct usb_stk11xx { + struct v4l2_device v4l2_dev; struct video_device *vdev; /**< Pointer on a V4L2 video device */ struct usb_device *udev; /**< Pointer on a USB device */ struct usb_interface *interface; /**< Pointer on a USB interface */ diff -urp syntekdriver-code-107-trunk-orig/driver/stk11xx-v4l.c syntekdriver-code-107-trunk//driver/stk11xx-v4l.c --- syntekdriver-code-107-trunk-orig/driver/stk11xx-v4l.c 2012-03-10 09:54:57.000000000 +0100 +++ syntekdriver-code-107-trunk//driver/stk11xx-v4l.c 2013-08-05 22:51:12.000000000 +0200 @@ -1498,9 +1498,17 @@ int v4l_stk11xx_register_video_device(st { int err; + err = v4l2_device_register(&dev->interface->dev, &dev->v4l2_dev); + if (err < 0) { + STK_ERROR("couldn't register v4l2_device\n"); + kfree(dev); + return err; + } + strcpy(dev->vdev->name, DRIVER_DESC); - dev->vdev->parent = &dev->interface->dev; +// dev->vdev->parent = &dev->interface->dev; + dev->vdev->v4l2_dev = &dev->v4l2_dev; dev->vdev->fops = &v4l_stk11xx_fops; dev->vdev->release = video_device_release; dev->vdev->minor = -1; @@ -1533,6 +1541,7 @@ int v4l_stk11xx_unregister_video_device( video_set_drvdata(dev->vdev, NULL); video_unregister_device(dev->vdev); + v4l2_device_unregister(&dev->v4l2_dev); return 0; }