> 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.
Of course :-)
> Some minor comments below:
>
> > +5. Driver installation
> > +======================
>
> <snip>
>
> I don't think you need this for the in-kernel documentation :)
"Driver installation" gives informations for configuring the kernel too.
It is appropriate, although the title could be something more descriptive..
> > +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 :)
I read the code better doing this way. It is my coding style when instructions
above are split into different parts, but I will change it if this little pattern can
not be accepted :-)
> > +/*--------------------------------------------------------------------------
> > + 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.
Parsing of the data is a very simple routine. IMHO it is better using
a single file. If the actual procfs interface can't be included into Linux 2.6,
I will remove it at all (with my disappointment though). I suppose
removing it means that I also need to get rid of the same code for the
inclusion into Linux 2.4 ?
> > +/* 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.
'ovcamchip' is the i2c-kernel-driver for the i2c-adapter included in the
w9968cf v4l1 driver, Typical I2C communication mechanism is used, no
ioctls and kernel-userspace boundary crossing. Have a look at the
ovcamchip _v2.25_ source code for more informations..
Thanks for your suggestions, but can you tell me what I _must_
remove/modify (at least) before including the driver into both Linux2.4
and Linux 2.6?
-Luca
-------------------------------------------------------
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
