On Monday 22 December 2008 08:49:34 Trilok Soni wrote: > Hi Hans, > > Sorry for the delay in reply. > > >> Attached the updated ov9640 sensor patch. > > > > Thanks. Here is my review: > > > > 1) Don't use this: static struct ov9640_sensor ov9640; > > > > This allows only one sensor instance. This should be dynamic. Remember > > that it should be possible (once the new v4l2_device/v4l2_subdev > > framework is merged) to reuse this driver in other products as well. So > > it should be possible to use two webcams with this sensor at the same > > time. It's only a small amount of work to make this struct dynamic. > > Ditto for the 'current_value' field of the static struct vcontrol: this > > too is per-instance. > > Hopefully kzallocing in the probe ? Yes. Typically all state information is pulled into one struct which is kzalloc'ed in the probe and freed at the end. > > > 2) Looking at all the YUV and RGB register settings I notice that they > > seem to fall into two parts: all MTX regs and some COM regs are > > identical for either RGB or YUV. It's a good idea to have only two > > arrays for these registers rather than duplicating them for each > > format. You might want to consider setting the remaining COM regs > > directly in a switch (fmt) statement. Try it and see what is more > > readable. > > I will try this. > > > 3) There already exists a standard autoexposure control: > > V4L2_CID_EXPOSURE_AUTO. > > Ok. > > > 4) What does V4L2_CID_FREEZE_AGCAEC do? > > I need to pull out some docs on this, but not looked at them from long > time. I recommend that you read my posting "Re: Extended driver private controls?" on the linux-omap list as well (for some reason it was refused on the v4l list, weird). > > 5) We have standardized the camera control names. A patch for this is > > still pending, but I recommend that in the meantime you use these > > names: > > > > AUTOGAIN: "Gain, Automatic" > > AUTO_WHITE_BALANCE: "White Balance, Automatic" > > HFLIP: "Horizontal Flip" > > VFLIP: "Vertical Flip" > > > > AUTO_EXPOSURE will probably change to "Exposure, Automatic", but this > > is still under discussion. > > Ok. It's been decided that it will be called "Auto Exposure" instead. > > > 6) include/media/ov9640.h: what part of this header needs to be visible > > to other parts of the kernel? A lot seems to be internal to the driver > > and so should be moved to the driver source (or a ov9640_regs.h headers > > next to the driver source). > > I will bring regs inside the source file and remove ov9640.h from > include/media if there is no need of platform data dependency from > board-xxx.c files. Thanks, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
