Re: [PATCH] USB: add zr364xx V4L2 driver

Hi Antoine,

A few comments:

> +#include <linux/videodev.h>
Why are you including the old header? Probably you are using some
obsolete stuff. You should instead include videodev2.h, removing the old
style from kernel 2.4 times.

> +static struct usb_device_id device_table[] = {
> 
> 
+/* devices supported by this driver */
> +static struct {
> +	int vendor;
> +	int product;
> +	int method;		/* which init method this camera uses */
> +	const char *name;
> +} devices[] = {
> 
Why do duplicate USB ids? Take a look at the way em28xx-cards do link usb_device_id table with device data.



> +
> +	case VIDIOC_REQBUFS:
> +		DBG("VIDIOC_REQBUFS not supported!");
> +		return -EINVAL;
> +	case VIDIOC_QUERYBUF:
> +		DBG("VIDIOC_QUERYBUF not supported!");
> +		return -EINVAL;
> +	case VIDIOC_QBUF:
> +		DBG("VIDIOC_QBUF not supported!");
> +		return -EINVAL;
> +	case VIDIOC_DQBUF:
> +		DBG("VIDIOC_DQBUF not supported!");
> +		return -EINVAL;
> +
> +	case VIDIOC_ENUMOUTPUT:
> +	case VIDIOC_G_OUTPUT:
> +	case VIDIOC_S_OUTPUT:
> +	case VIDIOC_G_MODULATOR:
> +	case VIDIOC_S_MODULATOR:
> +
> +	case VIDIOC_ENUMAUDIO:
> +	case VIDIOC_G_AUDIO:
> +	case VIDIOC_S_AUDIO:
> +
> +	case VIDIOC_ENUMAUDOUT:
> +	case VIDIOC_G_AUDOUT:
> +	case VIDIOC_S_AUDOUT:
> +
> +	case VIDIOC_ENUMSTD:
> +	case VIDIOC_QUERYSTD:
> +	case VIDIOC_G_STD:
> +	case VIDIOC_S_STD:
> +
> +	case VIDIOC_G_TUNER:
> +	case VIDIOC_S_TUNER:
> +	case VIDIOC_G_FREQUENCY:
> +	case VIDIOC_S_FREQUENCY:
> +
> +	case VIDIOC_OVERLAY:
> +	case VIDIOC_G_FBUF:
> +	case VIDIOC_S_FBUF:
> +
> +	case VIDIOC_G_PARM:
> +	case VIDIOC_S_PARM:
> +		DBG("skip: %d", ioctlnr);
> +		return -EINVAL;

> +
> +	default:
> +		DBG("V4L2 command not supported!: %d", ioctlnr);
> +		return -ENOIOCTLCMD;
> +
> +	}

Instead of those, you should call v4l1_compat to allow your driver to
work with older apps.

The better way would be to use video_ioctl2 handler that will call the
proper backward compatibility functions. As a side effect, it will also
implement a proper set of debug prints for ioctls.

There are some drivers already converted to the newer way: cafe_ccic,
vivi, cx88-video (and some radio drivers). The better would be if you
look at cafe_ccic (since this is also a driver for a webcam).

+static struct video_device zoran_template = {
> +	.owner = THIS_MODULE,
> +	.name = DRIVER_DESC,
> +	.type = VID_TYPE_CAPTURE,
> +	.hardware = VID_HARDWARE_ZR364XX,

diff -uprN -X linux-2.6.20-vanilla/Documentation/dontdiff linux-2.6.20-vanilla/include/linux/videodev.h linux-2.6.20/include/linux/videodev.h
> --- linux-2.6.20-vanilla/include/linux/videodev.h	2007-02-04 19:44:54.000000000 +0100
> +++ linux-2.6.20/include/linux/videodev.h	2007-02-09 10:54:12.000000000 +0100
> @@ -335,6 +335,7 @@ struct video_code
>  #define VID_HARDWARE_SAA7114H   37
>  #define VID_HARDWARE_SN9C102	38
>  #define VID_HARDWARE_ARV	39
> +#define VID_HARDWARE_ZR364XX	40
> 

You don't need to use .hardware. Just remove this and don't touch videodev.h, since those stuff are obsolete.

Also, instead of calling the functions as:
	zoran_foo
You should, instead, call them as 
	zr34xx_ foo
Since zoran driver already use zoran_foo for their internal functions.
This helps avoiding possible future conflicts if, for some reason, a
static symbol should be transformed later into an exported one.

Cheers,
Mauro

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list