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 <[EMAIL PROTECTED]>
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
