Re: [PATCH][RFC] Add mt9p031 sensor support.
On 25 May 2011 11:43, Laurent Pinchart laurent.pinch...@ideasonboard.com wrote: Hi Javier, On Wednesday 25 May 2011 11:41:42 javier Martin wrote: 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: On Tuesday 24 May 2011 16:30:43 Javier Martin wrote: 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 javier.mar...@vista-silicon.com [snip] diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c new file mode 100644 index 000..04d8812 --- /dev/null +++ b/drivers/media/video/mt9p031.c [snip] +#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). 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][RFC] Add mt9p031 sensor support.
On Fri, 27 May 2011, javier Martin wrote: On 25 May 2011 11:43, Laurent Pinchart laurent.pinch...@ideasonboard.com wrote: Hi Javier, On Wednesday 25 May 2011 11:41:42 javier Martin wrote: 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: On Tuesday 24 May 2011 16:30:43 Javier Martin wrote: 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 javier.mar...@vista-silicon.com [snip] diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c new file mode 100644 index 000..04d8812 --- /dev/null +++ b/drivers/media/video/mt9p031.c [snip] +#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/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][RFC] Add mt9p031 sensor support.
Hi Javier,
Thanks for the patch. Here's a review of the power handling code.
On Tuesday 24 May 2011 16:30:43 Javier Martin wrote:
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 javier.mar...@vista-silicon.com
[snip]
diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c
new file mode 100644
index 000..04d8812
--- /dev/null
+++ b/drivers/media/video/mt9p031.c
[snip]
@@ -0,0 +1,841 @@
+/*
+ * Driver for MT9P031 CMOS Image Sensor from Aptina
+ *
+ * Copyright (C) 2011, Javier Martin javier.mar...@vista-silicon.com
+ *
+ * Copyright (C) 2011, Guennadi Liakhovetski g.liakhovet...@gmx.de
+ *
+ * Based on the MT9V032 driver and Bastian Hecht's code.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/delay.h
+#include linux/device.h
+#include linux/i2c.h
+#include linux/log2.h
+#include linux/pm.h
+#include linux/regulator/consumer.h
+#include linux/slab.h
+#include media/v4l2-subdev.h
+#include linux/videodev2.h
+
+#include media/mt9p031.h
+#include media/v4l2-chip-ident.h
This header is not needed anymore.
+#include media/v4l2-subdev.h
+#include media/v4l2-device.h
+
+#define MT9P031_PIXCLK_FREQ 5400
+
+/* mt9p031 selected register addresses */
+#define MT9P031_CHIP_VERSION 0x00
+#define MT9P031_CHIP_VERSION_VALUE 0x1801
+#define MT9P031_ROW_START0x01
+#define MT9P031_ROW_START_DEF 54
+#define MT9P031_COLUMN_START 0x02
+#define MT9P031_COLUMN_START_DEF16
+#define MT9P031_WINDOW_HEIGHT0x03
+#define MT9P031_WINDOW_WIDTH 0x04
+#define MT9P031_H_BLANKING 0x05
+#define MT9P031_H_BLANKING_VALUE0
+#define MT9P031_V_BLANKING 0x06
+#define MT9P031_V_BLANKING_VALUE25
+#define MT9P031_OUTPUT_CONTROL 0x07
+#define MT9P031_OUTPUT_CONTROL_CEN 2
+#define MT9P031_OUTPUT_CONTROL_SYN 1
+#define MT9P031_SHUTTER_WIDTH_UPPER 0x08
+#define MT9P031_SHUTTER_WIDTH0x09
+#define MT9P031_PIXEL_CLOCK_CONTROL 0x0a
+#define MT9P031_FRAME_RESTART0x0b
+#define MT9P031_SHUTTER_DELAY0x0c
+#define MT9P031_RST 0x0d
+#define MT9P031_RST_ENABLE 1
+#define MT9P031_RST_DISABLE 0
+#define MT9P031_READ_MODE_1 0x1e
+#define MT9P031_READ_MODE_2 0x20
+#define MT9P031_READ_MODE_2_ROW_MIR 0x8000
+#define MT9P031_READ_MODE_2_COL_MIR 0x4000
+#define MT9P031_ROW_ADDRESS_MODE 0x22
+#define MT9P031_COLUMN_ADDRESS_MODE 0x23
+#define MT9P031_GLOBAL_GAIN 0x35
+
+#define MT9P031_WINDOW_HEIGHT_MAX1944
+#define MT9P031_WINDOW_WIDTH_MAX 2592
+#define MT9P031_WINDOW_HEIGHT_MIN2
+#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. You can define *_DEF constants for the
default width and height.
+struct mt9p031 {
+ struct v4l2_subdev subdev;
+ struct media_pad pad;
+ struct v4l2_rect rect; /* Sensor window */
+ struct v4l2_mbus_framefmt format;
+ struct mt9p031_platform_data *pdata;
+ struct mutex power_lock; /* lock to protect power_count */
+ int power_count;
+ u16 xskip;
+ u16 yskip;
+ /* cache register values */
+ u16 output_control;
+ u16 h_blanking;
+ u16 v_blanking;
+ u16 column_address_mode;
+ u16 row_address_mode;
+ u16 column_start;
+ u16 row_start;
+ u16 window_width;
+ u16 window_height;
+ struct regulator *reg_1v8;
+ struct regulator *reg_2v8;
+};
[snip]
+static int restore_registers(struct i2c_client *client)
+{
+ int ret;
+ struct mt9p031 *mt9p031 = to_mt9p031(client);
+
+ /* Disable register update, reconfigure atomically */
+ ret = mt9p031_set_output_control(mt9p031, 0,
+ MT9P031_OUTPUT_CONTROL_SYN);
+ if (ret 0)
+ return ret;
+
+ /* Blanking and start values - default... */
+ ret = reg_write(client, MT9P031_H_BLANKING, mt9p031-h_blanking);
+ if (ret 0)
+ return ret;
+
+ ret = reg_write(client, MT9P031_V_BLANKING,
Re: [PATCH][RFC] Add mt9p031 sensor support.
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 laurent.pinch...@ideasonboard.com wrote: Hi Javier, Thanks for the patch. Here's a review of the power handling code. On Tuesday 24 May 2011 16:30:43 Javier Martin wrote: 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 javier.mar...@vista-silicon.com [snip] diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c new file mode 100644 index 000..04d8812 --- /dev/null +++ b/drivers/media/video/mt9p031.c [snip] +#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? -- 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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][RFC] Add mt9p031 sensor support.
Hi Javier, On Wednesday 25 May 2011 11:41:42 javier Martin wrote: 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: On Tuesday 24 May 2011 16:30:43 Javier Martin wrote: 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 javier.mar...@vista-silicon.com [snip] diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c new file mode 100644 index 000..04d8812 --- /dev/null +++ b/drivers/media/video/mt9p031.c [snip] +#define MT9P031_WINDOW_HEIGHT_MAX1944 +#define MT9P031_WINDOW_WIDTH_MAX 2592 +#define MT9P031_WINDOW_HEIGHT_MIN2 +#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. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
