Ok so I have simply removed the printing from init and exit routines. I:ve also removed most of the image control module parameters whcih cuts size down some as well as taking out the same ones from teh sysfs since you really should be setting them with v4l controls instead. Currently the params and sysfs entries look like the following.
module params: jpeg bulk bandwidth hflip vlfip min_buffers max_buffers v4l_debug sysfs: release information videostatus hlfip vflip v4l_debug replaces the old log_level and is a boolean value that will enable ioctl debugging output from teh v4l subsystem. I left vlfip in since some cameras by default preoduce upside down output, hflip was left in for symmetry. On Wed, Jun 10, 2009 at 12:05 PM, GWater<[email protected]> wrote: > > I looked over at a few other drivers. > > Both gspca and sn9c1xx have fewer module paramters, none of the > related to image settings. I think we can remove the debugging > messages in the init. And maybe remove most of these parameters, too. > The only ones providing real functionality are jpeg, bulk, bandwidth > and max/min_buffers. Maybe we should cut it down to them and show > error messages when we actually act use their values for something. > > GWater > > PS: sn9c1xx seems to have its own debugging facility. but the code > looks strange anyway. > > On 10 Jun., 02:34, Brian Johnson <[email protected]> wrote: >> Daniel, >> I've changed most of the UDIA_* to dev_* however currently there are >> some messages being printed from the drivers init and exit functions >> which do not have the dev structure available. Should i replace those >> with just a printk or remove them completely? >> >> Also the kernel doesn't have a usleep function as far as i can tell. >> >> 2009/6/9 Daniel Ribeiro <[email protected]>: >> >> > Em Seg, 2009-06-08 às 01:19 -0700, GWater escreveu: >> >> The device structure is not available in many functions in sn9c20x- >> >> queue.c. Then there are the init functions in sn9c20x-ub.c and several >> >> functions in sn9c20x-sysfs.c (the problem can be fixed by getting the >> >> device from the *priv thingy. However I don't think this is really >> >> worth the effort and I think there will be performance loss. >> >> > There is no performance loss on dereferencing a pointer. A printk costs >> > more than 1000 (just guessing) pointer dereferences. >> >> > All sysfs functions take a struct device * as the first argument. Only >> > thing wrong in there is that you are naming it "class". But what is the >> > point here? There is no UDIA_* on -sysfs.c >> >> > On -usb.c just use udev->dev. >> >> > On -queue.c i see that you have a single sn9c20x_video_queue for each >> > each struct usb_sn9c20x. You should consider not using a struct >> > sn9c20x_video_queue at all, and just merge your child fields on the >> > parent struct. If its not an option for you, then you can get your >> > struct usb_sn9c20x using the container_of macro. eg: >> >> > struct usb_sn9c20x *dev = container_of(queue, struct usb_sn9c20x, queue) >> >> >> On 8 Jun., 08:15, Brian Johnson <[email protected]> wrote: >> >> The other issue is of course that our current macros >> >> besides just printing the message will check the log level allowing >> >> the driver to dynamically adjust which messages get displayed. >> >> > You shouldn't invent your own log level facility, the kernel already has >> > one and filtering is done by standard sys(k)logd. >> > Use dev_[info|warn|err|dbg]. dev_dbg needs #define DEBUG to actually be >> > on the code. >> >> > Some other minor nitpicks.... >> >> > Dont: }, >> > { >> > use }, { instead. >> >> > Regarding comments format, dont: >> >> > 300 /*some hardcoded values are present >> > 301 *like those for maximal/minimal exposure >> > 302 *and exposure steps*/ >> >> > /* single line (dont forget the spaces after start and before end) */ >> >> > /* >> > * Multi-line comment >> > * line 2 >> > * line 3 >> > */ >> >> > 577 if (ret < 0) >> > 578 return ret; >> > 579 else >> > 580 return 0; >> >> > No need for "else" after "return". >> >> > 613 udelay(delay); >> >> > Urgh! Do you really want to block all the kernel while you wait for >> > USB/I2C I/O? This is a MEGA performance killer. Use usleep() instead. >> >> > -- >> > Daniel Ribeiro > > > --~--~---------~--~----~------------~-------~--~----~ Lets make microdia webcams plug'n play, (currently plug'n pray) To post to this group, send email to [email protected] Visit us online https://groups.google.com/group/microdia -~----------~----~----~----~------~----~------~--~---
