First off, nice job. Thanks for sending this, but can you please send
me a patch for 2.6? I need that before I can add this to the 2.4 kernel
tree.
Some minor comments below:
> +5. Driver installation
> +======================
<snip>
I don't think you need this for the in-kernel documentation :)
> +static int w9968cf_allocate_memory(struct w9968cf_device* cam)
> +{
> + const unsigned long bufsize = w9968cf_get_max_bufsize(cam);
> + const u16 p_size = wMaxPacketSize[cam->altsetting-1];
> + void* buff = NULL;
> + u8 i;
> +
> + /* NOTE: Deallocation is done elsewhere in case of error */
> +
> + /* Allocate memory for the isochronous transfer buffers */
> + for (i = 0; i < W9968CF_URBS; i++) {
> + if (!(cam->transfer_buffer[i] =
> + kmalloc(W9968CF_ISO_PACKETS*p_size, GFP_KERNEL)))
> + {
Oops, put that '{' back where it belongs on the line above :)
> +/*--------------------------------------------------------------------------
> + Write to /proc/video/w9968cf/dev<minor#>
> + --------------------------------------------------------------------------*/
> +static int w9968cf_proc_write_dev(struct file* file, const char* buffer,
> + unsigned long count, void *data)
Can't you just split this up into a bunch of individual files like you
will have to do for 2.6? It makes the "parsing" of the data a lot
easier. And will make users moving from 2.4 to 2.6 more comfortable.
> + case VIDIOCSWIN: /* set capture area */
> + {
> + struct video_window win;
> + int err = 0;
> +
> + if (copy_from_user(&win, arg, sizeof(win)))
> + return -EFAULT;
> +
> + DBG(6, "VIDIOCSWIN called: clipcount=%d, flags=%d, "
> + "x=%d, y=%d, %dx%d", win.clipcount, win.flags,
> + win.x, win.y, win.width, win.height)
> +
> + if (win.clipcount != 0 || win.flags != 0)
> + return -EINVAL;
> +
> + if ((err = w9968cf_adjust_window_size(cam, (u16*)&win.width,
> + (u16*)&win.height)))
> + {
Oops, that dangling '{' again. You have that in a few more places in
this file too.
> +/* API */
> +struct ovcamchip_control {
> + __u32 id;
> + __s32 value;
> +};
> +
> +struct ovcamchip_window {
> + int x;
> + int y;
> + int width;
> + int height;
> + int format;
> + int quarter; /* Scale width and height down 2x */
> +
> + /* This stuff will be removed eventually */
> + int clockdiv; /* Clock divisor setting */
> +};
The ovcamchip_window structure has to be defined with "__" type
variables like you did on the ovcamchip_control structure if you are
going to cross the kernel-userspace boundry.
Any reason why you are using ioctls here and not just using the procfs
(and for 2.6, sysfs?)
ioctls will not be portable to other platforms unless you provide the
32-64 bit thunking layer, so no one with x86-64 boxes will be able to
use your driver.
thanks,
greg k-h
-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
