Hi Trilok, On Sunday 07 December 2008 10:40:51 Trilok Soni wrote: > Hi Hans, > > On Mon, Dec 1, 2008 at 6:21 PM, Trilok Soni <soni.trilok@xxxxxxxxx> wrote: > > Hi Hans, > > > >> I reviewed this sensor driver and it's fine except for one thing: > >> setting the default registers from outside the driver. This is a > >> really bad idea. I2C drivers should be self-contained. I've made > >> the same comment in the tvp514x driver review which I'm copying > >> below (with some small edits): > > > > I knew that you are going to comment on that, and I agree on those > > points. I will pull in that register initialization to the driver. > > 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. 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. 3) There already exists a standard autoexposure control: V4L2_CID_EXPOSURE_AUTO. 4) What does V4L2_CID_FREEZE_AGCAEC do? 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. 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). Regards, 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
