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 ? > > 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. > 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. > > 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. -- ---Trilok Soni http://triloksoni.wordpress.com http://www.linkedin.com/in/triloksoni -- 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
