On Wed, Sep 24, 2003 at 04:05:40AM +0200, Luca Risolia wrote:
> > > +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..
The sections on uncompressing the release and building it might be good
to remove too.
> > > +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 :-)
Yes, it needs to be changed to the proper kernel coding style as
outlined in Documentation/CodingStyle please. Remember, you aren't
going to be the only one looking at this code now :)
> > > +/*--------------------------------------------------------------------------
> > > + 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 ?
The procfs stuff for the v4l layer is gone in 2.6, replaced with the
sysfs interface. Since sysfs enforces the "one value, one file" rule,
you will have to split this up into a bunch of different sysfs files.
So, because of that, why not do the same thing for your proc interface
in 2.4 so that users will have the same interface for both kernel trees.
> > > +/* 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..
Hm, then what's the OVCAMCHIP_CMD_S_MODE ioctl definition for? Why not
just export some functions that your add-on module can call? That would
make things a lot easier I bet.
Oh, and the EXPORT_NO_SYMBOLS; line should be removed, it's obsolete
now.
> 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?
Well, fix the above things, and port it to 2.6. I think the 2.6 port
will shake out the proc issues. Send that patch to us and we can work
from there.
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
