Re: Syntek webcams and out-of-tree driver

[Ondrej Zary <linux@rainbow-software.org>]

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



-- 
Ondrej Zary