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