I tried to fix the UDIA problem but that seems impossible: dev_*() requires "struct device *". In most functions this is available via &(dev->udev->dev). That's ugly but usable. Unfortunatly there are many functions that need info and debugging output which don't have this struct available at all. I tried very hard to get devices from usb_interfaces, v4l_devices, usb_devices but in the end I often had to fall back to using printk() directly. And that's not comfortable and not really an option.
My advice: Use dev_*() but redefine the macro like UDIA_*(). Thoughts, other ideas? GWater If anyone wants my halfdone patch... Say so. > Jun., 08:53, Daniel Ribeiro <[email protected]> wrote: > Em Sáb, 2009-06-06 às 01:35 -0400, Brian Johnson escreveu: > > > sn9c20x: 184k > > omnivision: 43k > > micron: 30k > > hv7131r: 4.4k > > > I could reduce the big one my about another 30k by splitting out the > > sysfs and debugfs as well which would break down as > > > sn9c20x: 154k > > sysfs: 20k > > debugfs: 10k > > omnivision: 43k > > micron: 30k > > hv7131r: 4.4k > > Thats a start. :) > > I have some comments that could help reduce review ping-pong, just small > stuff that I already know are not well seen on the kernel community... > > static const int RX[] > static const int RY[] > static const int GX[] > static const int GY[] > static const int BX[] > static const int BY[] > > Namespace pollution. Names like these are not good, because they may > clash with other stuff on the kernel, use a more meaningful name, and > keep it lower-case (upper case is generally for macro). > Also, it may be a good idea to move these into a header file. > > __u8, __u16, __u32 > > Just use u8, u16, u32... > > 545 ret = usb_sn9c20x_control_write(dev, 0x1006, led, 2); > 546 > 547 return ret; > > Just "return usb_sn9c20x...." No need for the ret variable. Also applies > to a lot of other functions. > > UDIA_INFO("Using yuv420 output format\n"); > > This is a show stopper. People dont like these. Use dev_err|warn|dbg() > or pr_debug(). > > int sn9c20x_create_sysfs_files(struct video_device *vdev) > > This function lacks error handling.. > > Other than these minor nitpicks the driver looks good. I lack the v4l > subsystem knowledge to comment on more technical stuff. > > Don't forget to run scripts/checkpatch.pl on the patches, and follow the > CodingStyle. > > Mauro is always at #v4l on irc.freenode.net, nick "mchehab", it may be a > good idea to come in and present the project/driver on IRC. > > It's already too late for .30, but I think you have chances to get it in > for .31 if you send it for review ASAP. > > Good Luck! > > -- > Daniel Ribeiro > > signature.asc > < 1 KBAnzeigenHerunterladen --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
