On Fri, 27 May 2011, javier Martin wrote:

On 25 May 2011 11:43, Laurent Pinchart
[off-list ref] wrote:

quoted

Hi Javier,

On Wednesday 25 May 2011 11:41:42 javier Martin wrote:

quoted

Hi,
thank you for the review, I agree with you on all the suggested
changes except on this one:

On 25 May 2011 10:05, Laurent Pinchart wrote:

quoted

On Tuesday 24 May 2011 16:30:43 Javier Martin wrote:

quoted

This RFC includes a power management implementation that causes
the sensor to show images with horizontal artifacts (usually
monochrome lines that appear on the image randomly).

Signed-off-by: Javier Martin <redacted>

[snip]

quoted

diff --git a/drivers/media/video/mt9p031.c

b/drivers/media/video/mt9p031.c new file mode 100644
index 0000000..04d8812

--- /dev/null
+++ b/drivers/media/video/mt9p031.c

[snip]

quoted

+#define MT9P031_WINDOW_HEIGHT_MAX ? ? ? ? ? ?1944
+#define MT9P031_WINDOW_WIDTH_MAX ? ? ? ? ? ? 2592
+#define MT9P031_WINDOW_HEIGHT_MIN ? ? ? ? ? ?2
+#define MT9P031_WINDOW_WIDTH_MIN ? ? ? ? ? ? 18

Can you move those 4 constants right below MT9P031_WINDOW_HEIGHT and
MT9P031_WINDOW_WIDTH ? The max values are not correct, according to the
datasheet they should be 2005 and 2751.

In figure 4, it says active image size is 2592 x 1944
Why should I include active boundary and dark pixels?

Users might want to get the dark pixels for black level compensation purpose.
As the chip allows for that, it should be supported. The default should of
course be the active area of 2592 x 1944 pixels.

OK, that sounds reasonable. However, that would include black pixels
that are located at the beginning of the array (0,0) to (16, 54),
which means that users would have to specify a crop value of (15,54)
to eliminate those initial black level pixels. Which seems quite
unnatural to me.
Another option could be setting (16,54) as default values and allowing
to introduce negative cropping values. Is this possible?
And finally, the most sensible idea IMHO could be not letting the user
to see pixels from (0,0) to (15,54) (setting 15,54 as minimum and
default )and, for black level compensation, ending pixels could be
used (2608,1998) to (2751, 2003).

No, you set your crop bounds to (0,0)-... but your default rectangle to 
(15,54)-... and that's also what you set if noone issues an S_CROP.

Thanks
Guennadi

What do you think?


-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/