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 -~----------~----~----~----~------~----~------~--~---
