Sounds good. That leaves some style thingsan the error handling in sn9c20x_create_sysfs_files().
I'll get on the style stuff if you commit the patches we already have. (Rebasing massive amounts of small changes fails sometimes and it would be a shame to waste time on this boring stuff.) GWater On 10 Jun., 23:00, Brian Johnson <[email protected]> wrote: > 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 -~----------~----~----~----~------~----~------~--~---
