Re: Syntek webcams and out-of-tree driver

[Hans de Goede <hdegoede@redhat.com>]

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;
>   }
>
>
>