Re: [PATCH] Add Omnivision OV9640 sensor support.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@xxxxxxxxxx?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[Index of Archives]     [Linux Media]     [Older V4L]     [Linux DVB]     [Video Disk Recorder]     [Linux Kernel]     [Asterisk]     [DCCP]     [Netdev]     [X.org]     [Util Linux]     [Xfree86]     [Free Photo Albums]     [Fedora Users]     [Fedora Women]     [ALSA Users]     [ALSA Devel]     [SSH]     [DVB Maintainers]     [Linux USB]     [Yosemite Information]
  Powered by Linux