[PATCH v2 00/11] MT9M111/MT9M131

[PATCH v2 00/11] MT9M111/MT9M131

[Michael Grzeschik]

Hey everyone,

here is v2 of the previous (a little messy) patchseries. After i
figured out that the biggest part of the patches were cutted into
unrelated and unneeded pieces here hopefully comes a cleaner patchstack.

The rest of the patches i send last time is living in my git repo for
review, until i figured out that the code is mostly needed for the
softcropping feature of the camera.

But first things first, here are my changes:

Michael Grzeschik (9):
  mt9m111: init chip after read CHIP_VERSION
  mt9m111: register cleanup hex to dec bitoffset
  mt9m111: added new bit offset defines
  mt9m111: changed MIN_DARK_COLS to MT9M131 spec count
  mt9m111: cropcap and s_crop check if type is VIDEO_CAPTURE
  mt9m111: added current colorspace at g_fmt
  mt9m111: added reg_mask function
  mt9m111: rewrite set_pixfmt
  mt9m111: make use of testpattern

Philipp Wiesner (1):
  mt9m111: Added indication that MT9M131 is supported by this driver

Sascha Hauer (1):
  v4l2-mediabus: Add pixelcodes for BGR565 formats

 drivers/media/video/Kconfig   |    5 +-
 drivers/media/video/mt9m111.c |  283 ++++++++++++++++++++++++-----------------
 include/media/v4l2-mediabus.h |    2 +
 3 files changed, 174 insertions(+), 116 deletions(-)

--
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v2 01/11] mt9m111: Added indication that MT9M131 is supported by this driver

[Michael Grzeschik]

From: Philipp Wiesner <p.wiesner@phytec.de>

Added this info to Kconfig and mt9m111.c, some comment cleanup,
replaced 'mt9m11x'-statements by clarifications or driver name.
Driver is fully compatible to mt9m131 which has only additional functions
compared to mt9m111. Those aren't used anyway at the moment.

Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
Changes v1 -> v2

Address comments from Guennadi Liakhovetski:
	* joined strings to one line in probe dev_err

 drivers/media/video/Kconfig   |    5 +++--
 drivers/media/video/mt9m111.c |   36 ++++++++++++++++++++++--------------
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index cdbbbe4..0e8cf24 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -871,10 +871,11 @@ config SOC_CAMERA_MT9M001
 	  and colour models.
 
 config SOC_CAMERA_MT9M111
-	tristate "mt9m111 and mt9m112 support"
+	tristate "mt9m111, mt9m112 and mt9m131 support"
 	depends on SOC_CAMERA && I2C
 	help
-	  This driver supports MT9M111 and MT9M112 cameras from Micron
+	  This driver supports MT9M111, MT9M112 and MT9M131 cameras from
+	  Micron/Aptina
 
 config SOC_CAMERA_MT9T031
 	tristate "mt9t031 support"
diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index d35f536..68f3df6 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -1,5 +1,5 @@
 /*
- * Driver for MT9M111/MT9M112 CMOS Image Sensor from Micron
+ * Driver for MT9M111/MT9M112/MT9M131 CMOS Image Sensor from Micron/Aptina
  *
  * Copyright (C) 2008, Robert Jarzmik <robert.jarzmik@free.fr>
  *
@@ -19,11 +19,14 @@
 #include <media/soc_camera.h>
 
 /*
- * mt9m111 and mt9m112 i2c address is 0x5d or 0x48 (depending on SAddr pin)
+ * MT9M111, MT9M112 and MT9M131:
+ * i2c address is 0x48 or 0x5d (depending on SADDR pin)
  * The platform has to define i2c_board_info and call i2c_register_board_info()
  */
 
-/* mt9m111: Sensor register addresses */
+/*
+ * Sensor core register addresses (0x000..0x0ff)
+ */
 #define MT9M111_CHIP_VERSION		0x000
 #define MT9M111_ROW_START		0x001
 #define MT9M111_COLUMN_START		0x002
@@ -72,8 +75,9 @@
 #define MT9M111_CTXT_CTRL_LED_FLASH_EN	(1 << 2)
 #define MT9M111_CTXT_CTRL_VBLANK_SEL_B	(1 << 1)
 #define MT9M111_CTXT_CTRL_HBLANK_SEL_B	(1 << 0)
+
 /*
- * mt9m111: Colorpipe register addresses (0x100..0x1ff)
+ * Colorpipe register addresses (0x100..0x1ff)
  */
 #define MT9M111_OPER_MODE_CTRL		0x106
 #define MT9M111_OUTPUT_FORMAT_CTRL	0x108
@@ -109,8 +113,9 @@
 #define MT9M111_OUTFMT_SWAP_YCbCr_C_Y	(1 << 1)
 #define MT9M111_OUTFMT_SWAP_RGB_EVEN	(1 << 1)
 #define MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr	(1 << 0)
+
 /*
- * mt9m111: Camera control register addresses (0x200..0x2ff not implemented)
+ * Camera control register addresses (0x200..0x2ff not implemented)
  */
 
 #define reg_read(reg) mt9m111_reg_read(client, MT9M111_##reg)
@@ -160,7 +165,8 @@ enum mt9m111_context {
 
 struct mt9m111 {
 	struct v4l2_subdev subdev;
-	int model;	/* V4L2_IDENT_MT9M11x* codes from v4l2-chip-ident.h */
+	int model;	/* V4L2_IDENT_MT9M111 or V4L2_IDENT_MT9M112 code */
+			/* from v4l2-chip-ident.h */
 	enum mt9m111_context context;
 	struct v4l2_rect rect;
 	const struct mt9m111_datafmt *fmt;
@@ -934,7 +940,7 @@ static int mt9m111_init(struct i2c_client *client)
 	if (!ret)
 		ret = mt9m111_set_autoexposure(client, mt9m111->autoexposure);
 	if (ret)
-		dev_err(&client->dev, "mt9m11x init failed: %d\n", ret);
+		dev_err(&client->dev, "mt9m111 init failed: %d\n", ret);
 	return ret;
 }
 
@@ -970,21 +976,23 @@ static int mt9m111_video_probe(struct soc_camera_device *icd,
 	data = reg_read(CHIP_VERSION);
 
 	switch (data) {
-	case 0x143a: /* MT9M111 */
+	case 0x143a: /* MT9M111 or MT9M131 */
 		mt9m111->model = V4L2_IDENT_MT9M111;
+		dev_info(&client->dev,
+			"Detected a MT9M111/MT9M131 chip ID %x\n", data);
 		break;
 	case 0x148c: /* MT9M112 */
 		mt9m111->model = V4L2_IDENT_MT9M112;
+		dev_info(&client->dev, "Detected a MT9M112 chip ID %x\n", data);
 		break;
 	default:
 		ret = -ENODEV;
 		dev_err(&client->dev,
-			"No MT9M11x chip detected, register read %x\n", data);
+			"No MT9M111/MT9M112/MT9M131 chip detected register read %x\n",
+			data);
 		goto ei2c;
 	}
 
-	dev_info(&client->dev, "Detected a MT9M11x chip ID %x\n", data);
-
 ei2c:
 	return ret;
 }
@@ -1034,13 +1042,13 @@ static int mt9m111_probe(struct i2c_client *client,
 	int ret;
 
 	if (!icd) {
-		dev_err(&client->dev, "MT9M11x: missing soc-camera data!\n");
+		dev_err(&client->dev, "mt9m111: soc-camera data missing!\n");
 		return -EINVAL;
 	}
 
 	icl = to_soc_camera_link(icd);
 	if (!icl) {
-		dev_err(&client->dev, "MT9M11x driver needs platform data\n");
+		dev_err(&client->dev, "mt9m111: driver needs platform data\n");
 		return -EINVAL;
 	}
 
@@ -1116,6 +1124,6 @@ static void __exit mt9m111_mod_exit(void)
 module_init(mt9m111_mod_init);
 module_exit(mt9m111_mod_exit);
 
-MODULE_DESCRIPTION("Micron MT9M111/MT9M112 Camera driver");
+MODULE_DESCRIPTION("Micron/Aptina MT9M111/MT9M112/MT9M131 Camera driver");
 MODULE_AUTHOR("Robert Jarzmik");
 MODULE_LICENSE("GPL");
-- 
1.7.1

[PATCH 02/11] mt9m111: init chip after read CHIP_VERSION

[Michael Grzeschik]

Moved mt9m111_init after the chip version detection passage: I
don't like the idea of writing on a device we haven't identified
yet.

Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/media/video/mt9m111.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index 68f3df6..e7618da 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -969,10 +969,6 @@ static int mt9m111_video_probe(struct soc_camera_device *icd,
 	mt9m111->swap_rgb_even_odd = 1;
 	mt9m111->swap_rgb_red_blue = 1;
 
-	ret = mt9m111_init(client);
-	if (ret)
-		goto ei2c;
-
 	data = reg_read(CHIP_VERSION);
 
 	switch (data) {
@@ -993,6 +989,8 @@ static int mt9m111_video_probe(struct soc_camera_device *icd,
 		goto ei2c;
 	}
 
+	ret = mt9m111_init(client);
+
 ei2c:
 	return ret;
 }
-- 
1.7.1

[PATCH 03/11] mt9m111: register cleanup hex to dec bitoffset

[Michael Grzeschik]

Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/media/video/mt9m111.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index e7618da..8c076e5 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -100,14 +100,14 @@
 #define MT9M111_OUTFMT_BYPASS_IFP	(1 << 10)
 #define MT9M111_OUTFMT_INV_PIX_CLOCK	(1 << 9)
 #define MT9M111_OUTFMT_RGB		(1 << 8)
-#define MT9M111_OUTFMT_RGB565		(0x0 << 6)
-#define MT9M111_OUTFMT_RGB555		(0x1 << 6)
-#define MT9M111_OUTFMT_RGB444x		(0x2 << 6)
-#define MT9M111_OUTFMT_RGBx444		(0x3 << 6)
-#define MT9M111_OUTFMT_TST_RAMP_OFF	(0x0 << 4)
-#define MT9M111_OUTFMT_TST_RAMP_COL	(0x1 << 4)
-#define MT9M111_OUTFMT_TST_RAMP_ROW	(0x2 << 4)
-#define MT9M111_OUTFMT_TST_RAMP_FRAME	(0x3 << 4)
+#define MT9M111_OUTFMT_RGB565		(0 << 6)
+#define MT9M111_OUTFMT_RGB555		(1 << 6)
+#define MT9M111_OUTFMT_RGB444x		(2 << 6)
+#define MT9M111_OUTFMT_RGBx444		(3 << 6)
+#define MT9M111_OUTFMT_TST_RAMP_OFF	(0 << 4)
+#define MT9M111_OUTFMT_TST_RAMP_COL	(1 << 4)
+#define MT9M111_OUTFMT_TST_RAMP_ROW	(2 << 4)
+#define MT9M111_OUTFMT_TST_RAMP_FRAME	(3 << 4)
 #define MT9M111_OUTFMT_SHIFT_3_UP	(1 << 3)
 #define MT9M111_OUTFMT_AVG_CHROMA	(1 << 2)
 #define MT9M111_OUTFMT_SWAP_YCbCr_C_Y	(1 << 1)
-- 
1.7.1

[PATCH 04/11] mt9m111: added new bit offset defines

[Michael Grzeschik]

Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/media/video/mt9m111.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index 8c076e5..1b21522 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -63,6 +63,12 @@
 #define MT9M111_RESET_RESTART_FRAME	(1 << 1)
 #define MT9M111_RESET_RESET_MODE	(1 << 0)
 
+#define MT9M111_RM_FULL_POWER_RD	(0 << 10)
+#define MT9M111_RM_LOW_POWER_RD		(1 << 10)
+#define MT9M111_RM_COL_SKIP_4X		(1 << 5)
+#define MT9M111_RM_ROW_SKIP_4X		(1 << 4)
+#define MT9M111_RM_COL_SKIP_2X		(1 << 3)
+#define MT9M111_RM_ROW_SKIP_2X		(1 << 2)
 #define MT9M111_RMB_MIRROR_COLS		(1 << 1)
 #define MT9M111_RMB_MIRROR_ROWS		(1 << 0)
 #define MT9M111_CTXT_CTRL_RESTART	(1 << 15)
-- 
1.7.1

[PATCH 05/11] mt9m111: changed MIN_DARK_COLS to MT9M131 spec count

[Michael Grzeschik]

Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/media/video/mt9m111.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index 1b21522..944e0cb 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -130,7 +130,7 @@
 #define reg_clear(reg, val) mt9m111_reg_clear(client, MT9M111_##reg, (val))
 
 #define MT9M111_MIN_DARK_ROWS	8
-#define MT9M111_MIN_DARK_COLS	24
+#define MT9M111_MIN_DARK_COLS	26
 #define MT9M111_MAX_HEIGHT	1024
 #define MT9M111_MAX_WIDTH	1280
 
-- 
1.7.1

[PATCH 06/11] mt9m111: cropcap and s_crop check if type is VIDEO_CAPTURE

[Michael Grzeschik]

Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---

This one is a merge of:

[PATCH 09/20] mt9m111: cropcap check if type is CAPTURE
[PATCH 13/20] mt9m111: s_crop check for VIDEO_CAPTURE type


 drivers/media/video/mt9m111.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index 944e0cb..89c3f89 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -453,6 +453,9 @@ static int mt9m111_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 	dev_dbg(&client->dev, "%s left=%d, top=%d, width=%d, height=%d\n",
 		__func__, rect.left, rect.top, rect.width, rect.height);
 
+	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
 	ret = mt9m111_make_rect(client, &rect);
 	if (!ret)
 		mt9m111->rect = rect;
@@ -472,12 +475,14 @@ static int mt9m111_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 
 static int mt9m111_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
 {
+	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
 	a->bounds.left			= MT9M111_MIN_DARK_COLS;
 	a->bounds.top			= MT9M111_MIN_DARK_ROWS;
 	a->bounds.width			= MT9M111_MAX_WIDTH;
 	a->bounds.height		= MT9M111_MAX_HEIGHT;
 	a->defrect			= a->bounds;
-	a->type				= V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	a->pixelaspect.numerator	= 1;
 	a->pixelaspect.denominator	= 1;
 
-- 
1.7.1

[PATCH 07/11] mt9m111: added current colorspace at g_fmt

[Michael Grzeschik]

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/media/video/mt9m111.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index 89c3f89..48c63bc 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -498,6 +498,7 @@ static int mt9m111_g_fmt(struct v4l2_subdev *sd,
 	mf->width	= mt9m111->rect.width;
 	mf->height	= mt9m111->rect.height;
 	mf->code	= mt9m111->fmt->code;
+	mf->colorspace	= mt9m111->fmt->colorspace;
 	mf->field	= V4L2_FIELD_NONE;
 
 	return 0;
-- 
1.7.1

[PATCH 08/11] mt9m111: added reg_mask function

[Michael Grzeschik]

reg_mask is basically the same as clearing & setting registers,
but it is more convenient and faster (saves one rw cycle).

Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/media/video/mt9m111.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index 48c63bc..e865938 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -128,6 +128,8 @@
 #define reg_write(reg, val) mt9m111_reg_write(client, MT9M111_##reg, (val))
 #define reg_set(reg, val) mt9m111_reg_set(client, MT9M111_##reg, (val))
 #define reg_clear(reg, val) mt9m111_reg_clear(client, MT9M111_##reg, (val))
+#define reg_mask(reg, val, mask) mt9m111_reg_mask(client, MT9M111_##reg, \
+		(val), (mask))
 
 #define MT9M111_MIN_DARK_ROWS	8
 #define MT9M111_MIN_DARK_COLS	26
@@ -257,6 +259,15 @@ static int mt9m111_reg_clear(struct i2c_client *client, const u16 reg,
 	return mt9m111_reg_write(client, reg, ret & ~data);
 }
 
+static int mt9m111_reg_mask(struct i2c_client *client, const u16 reg,
+			    const u16 data, const u16 mask)
+{
+	int ret;
+
+	ret = mt9m111_reg_read(client, reg);
+	return mt9m111_reg_write(client, reg, (ret & ~mask) | data);
+}
+
 static int mt9m111_set_context(struct i2c_client *client,
 			       enum mt9m111_context ctxt)
 {
-- 
1.7.1

[PATCH 09/11] v4l2-mediabus: Add pixelcodes for BGR565 formats

[Michael Grzeschik]

From: Sascha Hauer <s.hauer@pengutronix.de>

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 include/media/v4l2-mediabus.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 0dbe02a..d0b7340 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h
@@ -32,6 +32,8 @@ enum v4l2_mbus_pixelcode {
 	V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE,
 	V4L2_MBUS_FMT_RGB565_2X8_LE,
 	V4L2_MBUS_FMT_RGB565_2X8_BE,
+	V4L2_MBUS_FMT_BGR565_2X8_LE,
+	V4L2_MBUS_FMT_BGR565_2X8_BE,
 	V4L2_MBUS_FMT_SBGGR8_1X8,
 	V4L2_MBUS_FMT_SBGGR10_1X10,
 	V4L2_MBUS_FMT_GREY8_1X8,
-- 
1.7.1

[PATCH v2 10/11] mt9m111: rewrite set_pixfmt

[Michael Grzeschik]

added more supported BE colour formats
and also support BGR565 swapped pixel formats

removed pixfmt helper functions and option flags
setting the configuration register directly in set_pixfmt

Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
Changes v1 -> v2
	* removed unrelated OPMODE handling in this function

 drivers/media/video/mt9m111.c |  143 ++++++++++++++++-------------------------
 1 files changed, 56 insertions(+), 87 deletions(-)

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index e865938..25b2317 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -101,7 +101,8 @@
 
 #define MT9M111_OPMODE_AUTOEXPO_EN	(1 << 14)
 #define MT9M111_OPMODE_AUTOWHITEBAL_EN	(1 << 1)
-
+#define MT9M111_OUTFMT_FLIP_BAYER_COL  (1 << 9)
+#define MT9M111_OUTFMT_FLIP_BAYER_ROW  (1 << 8)
 #define MT9M111_OUTFMT_PROCESSED_BAYER	(1 << 14)
 #define MT9M111_OUTFMT_BYPASS_IFP	(1 << 10)
 #define MT9M111_OUTFMT_INV_PIX_CLOCK	(1 << 9)
@@ -119,6 +120,7 @@
 #define MT9M111_OUTFMT_SWAP_YCbCr_C_Y	(1 << 1)
 #define MT9M111_OUTFMT_SWAP_RGB_EVEN	(1 << 1)
 #define MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr	(1 << 0)
+#define MT9M111_OUTFMT_SWAP_RGB_R_B	(1 << 0)
 
 /*
  * Camera control register addresses (0x200..0x2ff not implemented)
@@ -161,7 +163,11 @@ static const struct mt9m111_datafmt mt9m111_colour_fmts[] = {
 	{V4L2_MBUS_FMT_YUYV8_2X8_BE, V4L2_COLORSPACE_JPEG},
 	{V4L2_MBUS_FMT_YVYU8_2X8_BE, V4L2_COLORSPACE_JPEG},
 	{V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE, V4L2_COLORSPACE_SRGB},
+	{V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE, V4L2_COLORSPACE_SRGB},
 	{V4L2_MBUS_FMT_RGB565_2X8_LE, V4L2_COLORSPACE_SRGB},
+	{V4L2_MBUS_FMT_RGB565_2X8_BE, V4L2_COLORSPACE_SRGB},
+	{V4L2_MBUS_FMT_BGR565_2X8_LE, V4L2_COLORSPACE_SRGB},
+	{V4L2_MBUS_FMT_BGR565_2X8_BE, V4L2_COLORSPACE_SRGB},
 	{V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_COLORSPACE_SRGB},
 	{V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE, V4L2_COLORSPACE_SRGB},
 };
@@ -184,10 +190,6 @@ struct mt9m111 {
 	unsigned int powered:1;
 	unsigned int hflip:1;
 	unsigned int vflip:1;
-	unsigned int swap_rgb_even_odd:1;
-	unsigned int swap_rgb_red_blue:1;
-	unsigned int swap_yuv_y_chromas:1;
-	unsigned int swap_yuv_cb_cr:1;
 	unsigned int autowhitebalance:1;
 };
 
@@ -329,68 +331,6 @@ static int mt9m111_setup_rect(struct i2c_client *client,
 	return ret;
 }
 
-static int mt9m111_setup_pixfmt(struct i2c_client *client, u16 outfmt)
-{
-	int ret;
-
-	ret = reg_write(OUTPUT_FORMAT_CTRL2_A, outfmt);
-	if (!ret)
-		ret = reg_write(OUTPUT_FORMAT_CTRL2_B, outfmt);
-	return ret;
-}
-
-static int mt9m111_setfmt_bayer8(struct i2c_client *client)
-{
-	return mt9m111_setup_pixfmt(client, MT9M111_OUTFMT_PROCESSED_BAYER |
-				    MT9M111_OUTFMT_RGB);
-}
-
-static int mt9m111_setfmt_bayer10(struct i2c_client *client)
-{
-	return mt9m111_setup_pixfmt(client, MT9M111_OUTFMT_BYPASS_IFP);
-}
-
-static int mt9m111_setfmt_rgb565(struct i2c_client *client)
-{
-	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int val = 0;
-
-	if (mt9m111->swap_rgb_red_blue)
-		val |= MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
-	if (mt9m111->swap_rgb_even_odd)
-		val |= MT9M111_OUTFMT_SWAP_RGB_EVEN;
-	val |= MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB565;
-
-	return mt9m111_setup_pixfmt(client, val);
-}
-
-static int mt9m111_setfmt_rgb555(struct i2c_client *client)
-{
-	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int val = 0;
-
-	if (mt9m111->swap_rgb_red_blue)
-		val |= MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
-	if (mt9m111->swap_rgb_even_odd)
-		val |= MT9M111_OUTFMT_SWAP_RGB_EVEN;
-	val |= MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB555;
-
-	return mt9m111_setup_pixfmt(client, val);
-}
-
-static int mt9m111_setfmt_yuv(struct i2c_client *client)
-{
-	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int val = 0;
-
-	if (mt9m111->swap_yuv_cb_cr)
-		val |= MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
-	if (mt9m111->swap_yuv_y_chromas)
-		val |= MT9M111_OUTFMT_SWAP_YCbCr_C_Y;
-
-	return mt9m111_setup_pixfmt(client, val);
-}
-
 static int mt9m111_enable(struct i2c_client *client)
 {
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
@@ -518,41 +458,54 @@ static int mt9m111_g_fmt(struct v4l2_subdev *sd,
 static int mt9m111_set_pixfmt(struct i2c_client *client,
 			      enum v4l2_mbus_pixelcode code)
 {
-	struct mt9m111 *mt9m111 = to_mt9m111(client);
+	u16 data_outfmt1 = 0, data_outfmt2 = 0, mask_outfmt1, mask_outfmt2;
 	int ret;
 
 	switch (code) {
 	case V4L2_MBUS_FMT_SBGGR8_1X8:
-		ret = mt9m111_setfmt_bayer8(client);
+		data_outfmt1 = MT9M111_OUTFMT_FLIP_BAYER_ROW;
+		data_outfmt2 = MT9M111_OUTFMT_PROCESSED_BAYER |
+			MT9M111_OUTFMT_RGB;
 		break;
 	case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE:
-		ret = mt9m111_setfmt_bayer10(client);
+		data_outfmt2 = MT9M111_OUTFMT_BYPASS_IFP | MT9M111_OUTFMT_RGB;
 		break;
 	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE:
-		ret = mt9m111_setfmt_rgb555(client);
+		data_outfmt2 = MT9M111_OUTFMT_SWAP_RGB_EVEN |
+			MT9M111_OUTFMT_RGB |
+			MT9M111_OUTFMT_RGB555;
+		break;
+	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE:
+		data_outfmt2 = MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB555;
 		break;
 	case V4L2_MBUS_FMT_RGB565_2X8_LE:
-		ret = mt9m111_setfmt_rgb565(client);
+		data_outfmt2 = MT9M111_OUTFMT_SWAP_RGB_EVEN |
+			MT9M111_OUTFMT_RGB |
+			MT9M111_OUTFMT_RGB565;
+		break;
+	case V4L2_MBUS_FMT_RGB565_2X8_BE:
+		data_outfmt2 = MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB565;
+		break;
+	case V4L2_MBUS_FMT_BGR565_2X8_LE:
+		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr |
+			MT9M111_OUTFMT_SWAP_RGB_EVEN |
+			MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB565;
+		break;
+	case V4L2_MBUS_FMT_BGR565_2X8_BE:
+		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr |
+			MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB565;
 		break;
 	case V4L2_MBUS_FMT_YUYV8_2X8_BE:
-		mt9m111->swap_yuv_y_chromas = 0;
-		mt9m111->swap_yuv_cb_cr = 0;
-		ret = mt9m111_setfmt_yuv(client);
 		break;
 	case V4L2_MBUS_FMT_YVYU8_2X8_BE:
-		mt9m111->swap_yuv_y_chromas = 0;
-		mt9m111->swap_yuv_cb_cr = 1;
-		ret = mt9m111_setfmt_yuv(client);
+		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
 		break;
 	case V4L2_MBUS_FMT_YUYV8_2X8_LE:
-		mt9m111->swap_yuv_y_chromas = 1;
-		mt9m111->swap_yuv_cb_cr = 0;
-		ret = mt9m111_setfmt_yuv(client);
+		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_C_Y;
 		break;
 	case V4L2_MBUS_FMT_YVYU8_2X8_LE:
-		mt9m111->swap_yuv_y_chromas = 1;
-		mt9m111->swap_yuv_cb_cr = 1;
-		ret = mt9m111_setfmt_yuv(client);
+		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_C_Y |
+			MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
 		break;
 	default:
 		dev_err(&client->dev, "Pixel format not handled : %x\n",
@@ -560,6 +513,25 @@ static int mt9m111_set_pixfmt(struct i2c_client *client,
 		ret = -EINVAL;
 	}
 
+	mask_outfmt1 = MT9M111_OUTFMT_FLIP_BAYER_COL |
+		MT9M111_OUTFMT_FLIP_BAYER_ROW;
+
+	mask_outfmt2 = MT9M111_OUTFMT_PROCESSED_BAYER |
+		MT9M111_OUTFMT_BYPASS_IFP | MT9M111_OUTFMT_RGB |
+		MT9M111_OUTFMT_RGB565 | MT9M111_OUTFMT_RGB555 |
+		MT9M111_OUTFMT_RGB444x | MT9M111_OUTFMT_RGBx444 |
+		MT9M111_OUTFMT_SWAP_YCbCr_C_Y | MT9M111_OUTFMT_SWAP_RGB_EVEN |
+		MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr | MT9M111_OUTFMT_SWAP_RGB_R_B;
+
+	ret = reg_mask(OUTPUT_FORMAT_CTRL, data_outfmt1, mask_outfmt1);
+
+	if (!ret)
+		ret = reg_mask(OUTPUT_FORMAT_CTRL2_A, data_outfmt2,
+			mask_outfmt2);
+	if (!ret)
+		ret = reg_mask(OUTPUT_FORMAT_CTRL2_B, data_outfmt2,
+			mask_outfmt2);
+
 	return ret;
 }
 
@@ -989,9 +961,6 @@ static int mt9m111_video_probe(struct soc_camera_device *icd,
 	mt9m111->autoexposure = 1;
 	mt9m111->autowhitebalance = 1;
 
-	mt9m111->swap_rgb_even_odd = 1;
-	mt9m111->swap_rgb_red_blue = 1;
-
 	data = reg_read(CHIP_VERSION);
 
 	switch (data) {
-- 
1.7.1

[PATCH v2 11/11] mt9m111: make use of testpattern

[Michael Grzeschik]

Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
Changes v1 -> v2
	* removed ifdef DEBUG

 drivers/media/video/mt9m111.c |   57 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 57 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index 25b2317..e1b2d33 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -87,6 +87,7 @@
  */
 #define MT9M111_OPER_MODE_CTRL		0x106
 #define MT9M111_OUTPUT_FORMAT_CTRL	0x108
+#define MT9M111_TEST_PATTERN_GEN	0x148
 #define MT9M111_REDUCER_XZOOM_B		0x1a0
 #define MT9M111_REDUCER_XSIZE_B		0x1a1
 #define MT9M111_REDUCER_YZOOM_B		0x1a3
@@ -103,6 +104,15 @@
 #define MT9M111_OPMODE_AUTOWHITEBAL_EN	(1 << 1)
 #define MT9M111_OUTFMT_FLIP_BAYER_COL  (1 << 9)
 #define MT9M111_OUTFMT_FLIP_BAYER_ROW  (1 << 8)
+#define MT9M111_TST_PATT_OFF		(0 << 0)
+#define MT9M111_TST_PATT_1		(1 << 0)
+#define MT9M111_TST_PATT_2		(2 << 0)
+#define MT9M111_TST_PATT_3		(3 << 0)
+#define MT9M111_TST_PATT_4		(4 << 0)
+#define MT9M111_TST_PATT_5		(5 << 0)
+#define MT9M111_TST_PATT_6		(6 << 0)
+#define MT9M111_TST_PATT_COLORBARS	(7 << 0)
+#define MT9M111_TST_PATT_FORCE_WB_GAIN_1 (1 << 7)
 #define MT9M111_OUTFMT_PROCESSED_BAYER	(1 << 14)
 #define MT9M111_OUTFMT_BYPASS_IFP	(1 << 10)
 #define MT9M111_OUTFMT_INV_PIX_CLOCK	(1 << 9)
@@ -138,6 +148,11 @@
 #define MT9M111_MAX_HEIGHT	1024
 #define MT9M111_MAX_WIDTH	1280
 
+static int testpattern;
+module_param(testpattern, int, S_IRUGO);
+MODULE_PARM_DESC(testpattern, "Test pattern: a number from 1 to 10, 0 for "
+		"normal usage");
+
 /* MT9M111 has only one fixed colorspace per pixelcode */
 struct mt9m111_datafmt {
 	enum v4l2_mbus_pixelcode	code;
@@ -459,6 +474,7 @@ static int mt9m111_set_pixfmt(struct i2c_client *client,
 			      enum v4l2_mbus_pixelcode code)
 {
 	u16 data_outfmt1 = 0, data_outfmt2 = 0, mask_outfmt1, mask_outfmt2;
+	u16 pattern = 0;
 	int ret;
 
 	switch (code) {
@@ -525,6 +541,47 @@ static int mt9m111_set_pixfmt(struct i2c_client *client,
 
 	ret = reg_mask(OUTPUT_FORMAT_CTRL, data_outfmt1, mask_outfmt1);
 
+	switch (testpattern) {
+	case 1:
+		pattern = MT9M111_TST_PATT_1 | MT9M111_TST_PATT_FORCE_WB_GAIN_1;
+		break;
+	case 2:
+		pattern = MT9M111_TST_PATT_2 | MT9M111_TST_PATT_FORCE_WB_GAIN_1;
+		break;
+	case 3:
+		pattern = MT9M111_TST_PATT_3 | MT9M111_TST_PATT_FORCE_WB_GAIN_1;
+		break;
+	case 4:
+		pattern = MT9M111_TST_PATT_4 | MT9M111_TST_PATT_FORCE_WB_GAIN_1;
+		break;
+	case 5:
+		pattern = MT9M111_TST_PATT_5 | MT9M111_TST_PATT_FORCE_WB_GAIN_1;
+		break;
+	case 6:
+		pattern = MT9M111_TST_PATT_6 | MT9M111_TST_PATT_FORCE_WB_GAIN_1;
+		break;
+	case 7:
+		pattern = MT9M111_TST_PATT_COLORBARS |
+			MT9M111_TST_PATT_FORCE_WB_GAIN_1;
+		break;
+	case 8:
+		data_outfmt2 |= MT9M111_OUTFMT_TST_RAMP_COL;
+		break;
+	case 9:
+		data_outfmt2 |= MT9M111_OUTFMT_TST_RAMP_ROW;
+		break;
+	case 10:
+		data_outfmt2 |= MT9M111_OUTFMT_TST_RAMP_FRAME;
+		break;
+	}
+
+	dev_dbg(&client->dev, "%s: using testpattern %d\n", __func__,
+			testpattern);
+
+	if (!ret)
+		ret = mt9m111_reg_set(client,
+				MT9M111_TEST_PATTERN_GEN, pattern);
+
 	if (!ret)
 		ret = reg_mask(OUTPUT_FORMAT_CTRL2_A, data_outfmt2,
 			mask_outfmt2);
-- 
1.7.1

Re: [PATCH v2 00/11] MT9M111/MT9M131

[Michael Grzeschik]

Hi Robert, Guennadi,

after the messed up previous patchseries, this v2 series is left
without any feedback. Hopefully not forgotten. :-)

Btw: The first two patches are already applied.

Thanks,
Michael

On Tue, Aug 03, 2010 at 12:57:38PM +0200, Michael Grzeschik wrote:
> Hey everyone,
> 
> here is v2 of the previous (a little messy) patchseries. After i
> figured out that the biggest part of the patches were cutted into
> unrelated and unneeded pieces here hopefully comes a cleaner patchstack.
> 
> The rest of the patches i send last time is living in my git repo for
> review, until i figured out that the code is mostly needed for the
> softcropping feature of the camera.
> 
> But first things first, here are my changes:
> 
> Michael Grzeschik (9):
>   mt9m111: init chip after read CHIP_VERSION
>   mt9m111: register cleanup hex to dec bitoffset
>   mt9m111: added new bit offset defines
>   mt9m111: changed MIN_DARK_COLS to MT9M131 spec count
>   mt9m111: cropcap and s_crop check if type is VIDEO_CAPTURE
>   mt9m111: added current colorspace at g_fmt
>   mt9m111: added reg_mask function
>   mt9m111: rewrite set_pixfmt
>   mt9m111: make use of testpattern
> 
> Philipp Wiesner (1):
>   mt9m111: Added indication that MT9M131 is supported by this driver
> 
> Sascha Hauer (1):
>   v4l2-mediabus: Add pixelcodes for BGR565 formats
> 
>  drivers/media/video/Kconfig   |    5 +-
>  drivers/media/video/mt9m111.c |  283 ++++++++++++++++++++++++-----------------
>  include/media/v4l2-mediabus.h |    2 +
>  3 files changed, 174 insertions(+), 116 deletions(-)
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 00/11] MT9M111/MT9M131

[Guennadi Liakhovetski]

On Tue, 17 Aug 2010, Michael Grzeschik wrote:

> Hi Robert, Guennadi,
> 
> after the messed up previous patchseries, this v2 series is left
> without any feedback. Hopefully not forgotten. :-)

No, it is not forgotten, please, give me some more time.

Thanks
Guennadi

> 
> Btw: The first two patches are already applied.
> 
> Thanks,
> Michael
> 
> On Tue, Aug 03, 2010 at 12:57:38PM +0200, Michael Grzeschik wrote:
> > Hey everyone,
> > 
> > here is v2 of the previous (a little messy) patchseries. After i
> > figured out that the biggest part of the patches were cutted into
> > unrelated and unneeded pieces here hopefully comes a cleaner patchstack.
> > 
> > The rest of the patches i send last time is living in my git repo for
> > review, until i figured out that the code is mostly needed for the
> > softcropping feature of the camera.
> > 
> > But first things first, here are my changes:
> > 
> > Michael Grzeschik (9):
> >   mt9m111: init chip after read CHIP_VERSION
> >   mt9m111: register cleanup hex to dec bitoffset
> >   mt9m111: added new bit offset defines
> >   mt9m111: changed MIN_DARK_COLS to MT9M131 spec count
> >   mt9m111: cropcap and s_crop check if type is VIDEO_CAPTURE
> >   mt9m111: added current colorspace at g_fmt
> >   mt9m111: added reg_mask function
> >   mt9m111: rewrite set_pixfmt
> >   mt9m111: make use of testpattern
> > 
> > Philipp Wiesner (1):
> >   mt9m111: Added indication that MT9M131 is supported by this driver
> > 
> > Sascha Hauer (1):
> >   v4l2-mediabus: Add pixelcodes for BGR565 formats
> > 
> >  drivers/media/video/Kconfig   |    5 +-
> >  drivers/media/video/mt9m111.c |  283 ++++++++++++++++++++++++-----------------
> >  include/media/v4l2-mediabus.h |    2 +
> >  3 files changed, 174 insertions(+), 116 deletions(-)
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

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

Re: [PATCH v2 00/11] MT9M111/MT9M131

[Robert Jarzmik]

Michael Grzeschik <mgr@pengutronix.de> writes:

> Hi Robert, Guennadi,
>
> after the messed up previous patchseries, this v2 series is left
> without any feedback. Hopefully not forgotten. :-)
No, not forgotten.

I need a week, but Guennadi can superseed me anytime if he is the first to fire
:)

Cheers.

--
Robert

Re: [PATCH v2 00/11] MT9M111/MT9M131

[Guennadi Liakhovetski]

Hi Robert

On Wed, 18 Aug 2010, Robert Jarzmik wrote:

> Michael Grzeschik <mgr@pengutronix.de> writes:
> 
> > Hi Robert, Guennadi,
> >
> > after the messed up previous patchseries, this v2 series is left
> > without any feedback. Hopefully not forgotten. :-)
> No, not forgotten.
> 
> I need a week, but Guennadi can superseed me anytime if he is the first to fire
> :)

Well, I looked through remaining patches 3-11, they look ok, but of course 
I cannot test patch 10, which is the most intrusive one. Patch 5 is also 
changing behaviour, but since indeed 26 black columns are specified in the 
datasheet (also for mt9m111), it should be ok. So, Robert, I'll wait for 
your review and test results, if you don't mind. We still have a bit of 
time until the 2.6.37 merge window, so, should be ok.

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

Re: [PATCH v2 10/11] mt9m111: rewrite set_pixfmt

[Guennadi Liakhovetski]

Robert, I'll need your ack / tested by on this one too. It actually 
changes behaviour, for example, it sets MT9M111_OUTFMT_FLIP_BAYER_ROW in 
the OUTPUT_FORMAT_CTRL register for the V4L2_MBUS_FMT_SBGGR8_1X8 8 bit 
Bayer format. Maybe other things too - please have a look.

Thanks
Guennadi

On Tue, 3 Aug 2010, Michael Grzeschik wrote:

> added more supported BE colour formats
> and also support BGR565 swapped pixel formats
> 
> removed pixfmt helper functions and option flags
> setting the configuration register directly in set_pixfmt
> 
> Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> Changes v1 -> v2
> 	* removed unrelated OPMODE handling in this function
> 
>  drivers/media/video/mt9m111.c |  143 ++++++++++++++++-------------------------
>  1 files changed, 56 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
> index e865938..25b2317 100644
> --- a/drivers/media/video/mt9m111.c
> +++ b/drivers/media/video/mt9m111.c
> @@ -101,7 +101,8 @@
>  
>  #define MT9M111_OPMODE_AUTOEXPO_EN	(1 << 14)
>  #define MT9M111_OPMODE_AUTOWHITEBAL_EN	(1 << 1)
> -
> +#define MT9M111_OUTFMT_FLIP_BAYER_COL  (1 << 9)
> +#define MT9M111_OUTFMT_FLIP_BAYER_ROW  (1 << 8)
>  #define MT9M111_OUTFMT_PROCESSED_BAYER	(1 << 14)
>  #define MT9M111_OUTFMT_BYPASS_IFP	(1 << 10)
>  #define MT9M111_OUTFMT_INV_PIX_CLOCK	(1 << 9)
> @@ -119,6 +120,7 @@
>  #define MT9M111_OUTFMT_SWAP_YCbCr_C_Y	(1 << 1)
>  #define MT9M111_OUTFMT_SWAP_RGB_EVEN	(1 << 1)
>  #define MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr	(1 << 0)
> +#define MT9M111_OUTFMT_SWAP_RGB_R_B	(1 << 0)
>  
>  /*
>   * Camera control register addresses (0x200..0x2ff not implemented)
> @@ -161,7 +163,11 @@ static const struct mt9m111_datafmt mt9m111_colour_fmts[] = {
>  	{V4L2_MBUS_FMT_YUYV8_2X8_BE, V4L2_COLORSPACE_JPEG},
>  	{V4L2_MBUS_FMT_YVYU8_2X8_BE, V4L2_COLORSPACE_JPEG},
>  	{V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE, V4L2_COLORSPACE_SRGB},
> +	{V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE, V4L2_COLORSPACE_SRGB},
>  	{V4L2_MBUS_FMT_RGB565_2X8_LE, V4L2_COLORSPACE_SRGB},
> +	{V4L2_MBUS_FMT_RGB565_2X8_BE, V4L2_COLORSPACE_SRGB},
> +	{V4L2_MBUS_FMT_BGR565_2X8_LE, V4L2_COLORSPACE_SRGB},
> +	{V4L2_MBUS_FMT_BGR565_2X8_BE, V4L2_COLORSPACE_SRGB},
>  	{V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_COLORSPACE_SRGB},
>  	{V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE, V4L2_COLORSPACE_SRGB},
>  };
> @@ -184,10 +190,6 @@ struct mt9m111 {
>  	unsigned int powered:1;
>  	unsigned int hflip:1;
>  	unsigned int vflip:1;
> -	unsigned int swap_rgb_even_odd:1;
> -	unsigned int swap_rgb_red_blue:1;
> -	unsigned int swap_yuv_y_chromas:1;
> -	unsigned int swap_yuv_cb_cr:1;
>  	unsigned int autowhitebalance:1;
>  };
>  
> @@ -329,68 +331,6 @@ static int mt9m111_setup_rect(struct i2c_client *client,
>  	return ret;
>  }
>  
> -static int mt9m111_setup_pixfmt(struct i2c_client *client, u16 outfmt)
> -{
> -	int ret;
> -
> -	ret = reg_write(OUTPUT_FORMAT_CTRL2_A, outfmt);
> -	if (!ret)
> -		ret = reg_write(OUTPUT_FORMAT_CTRL2_B, outfmt);
> -	return ret;
> -}
> -
> -static int mt9m111_setfmt_bayer8(struct i2c_client *client)
> -{
> -	return mt9m111_setup_pixfmt(client, MT9M111_OUTFMT_PROCESSED_BAYER |
> -				    MT9M111_OUTFMT_RGB);
> -}
> -
> -static int mt9m111_setfmt_bayer10(struct i2c_client *client)
> -{
> -	return mt9m111_setup_pixfmt(client, MT9M111_OUTFMT_BYPASS_IFP);
> -}
> -
> -static int mt9m111_setfmt_rgb565(struct i2c_client *client)
> -{
> -	struct mt9m111 *mt9m111 = to_mt9m111(client);
> -	int val = 0;
> -
> -	if (mt9m111->swap_rgb_red_blue)
> -		val |= MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
> -	if (mt9m111->swap_rgb_even_odd)
> -		val |= MT9M111_OUTFMT_SWAP_RGB_EVEN;
> -	val |= MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB565;
> -
> -	return mt9m111_setup_pixfmt(client, val);
> -}
> -
> -static int mt9m111_setfmt_rgb555(struct i2c_client *client)
> -{
> -	struct mt9m111 *mt9m111 = to_mt9m111(client);
> -	int val = 0;
> -
> -	if (mt9m111->swap_rgb_red_blue)
> -		val |= MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
> -	if (mt9m111->swap_rgb_even_odd)
> -		val |= MT9M111_OUTFMT_SWAP_RGB_EVEN;
> -	val |= MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB555;
> -
> -	return mt9m111_setup_pixfmt(client, val);
> -}
> -
> -static int mt9m111_setfmt_yuv(struct i2c_client *client)
> -{
> -	struct mt9m111 *mt9m111 = to_mt9m111(client);
> -	int val = 0;
> -
> -	if (mt9m111->swap_yuv_cb_cr)
> -		val |= MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
> -	if (mt9m111->swap_yuv_y_chromas)
> -		val |= MT9M111_OUTFMT_SWAP_YCbCr_C_Y;
> -
> -	return mt9m111_setup_pixfmt(client, val);
> -}
> -
>  static int mt9m111_enable(struct i2c_client *client)
>  {
>  	struct mt9m111 *mt9m111 = to_mt9m111(client);
> @@ -518,41 +458,54 @@ static int mt9m111_g_fmt(struct v4l2_subdev *sd,
>  static int mt9m111_set_pixfmt(struct i2c_client *client,
>  			      enum v4l2_mbus_pixelcode code)
>  {
> -	struct mt9m111 *mt9m111 = to_mt9m111(client);
> +	u16 data_outfmt1 = 0, data_outfmt2 = 0, mask_outfmt1, mask_outfmt2;
>  	int ret;
>  
>  	switch (code) {
>  	case V4L2_MBUS_FMT_SBGGR8_1X8:
> -		ret = mt9m111_setfmt_bayer8(client);
> +		data_outfmt1 = MT9M111_OUTFMT_FLIP_BAYER_ROW;
> +		data_outfmt2 = MT9M111_OUTFMT_PROCESSED_BAYER |
> +			MT9M111_OUTFMT_RGB;
>  		break;
>  	case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE:
> -		ret = mt9m111_setfmt_bayer10(client);
> +		data_outfmt2 = MT9M111_OUTFMT_BYPASS_IFP | MT9M111_OUTFMT_RGB;
>  		break;
>  	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE:
> -		ret = mt9m111_setfmt_rgb555(client);
> +		data_outfmt2 = MT9M111_OUTFMT_SWAP_RGB_EVEN |
> +			MT9M111_OUTFMT_RGB |
> +			MT9M111_OUTFMT_RGB555;
> +		break;
> +	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE:
> +		data_outfmt2 = MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB555;
>  		break;
>  	case V4L2_MBUS_FMT_RGB565_2X8_LE:
> -		ret = mt9m111_setfmt_rgb565(client);
> +		data_outfmt2 = MT9M111_OUTFMT_SWAP_RGB_EVEN |
> +			MT9M111_OUTFMT_RGB |
> +			MT9M111_OUTFMT_RGB565;
> +		break;
> +	case V4L2_MBUS_FMT_RGB565_2X8_BE:
> +		data_outfmt2 = MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB565;
> +		break;
> +	case V4L2_MBUS_FMT_BGR565_2X8_LE:
> +		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr |
> +			MT9M111_OUTFMT_SWAP_RGB_EVEN |
> +			MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB565;
> +		break;
> +	case V4L2_MBUS_FMT_BGR565_2X8_BE:
> +		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr |
> +			MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB565;
>  		break;
>  	case V4L2_MBUS_FMT_YUYV8_2X8_BE:
> -		mt9m111->swap_yuv_y_chromas = 0;
> -		mt9m111->swap_yuv_cb_cr = 0;
> -		ret = mt9m111_setfmt_yuv(client);
>  		break;
>  	case V4L2_MBUS_FMT_YVYU8_2X8_BE:
> -		mt9m111->swap_yuv_y_chromas = 0;
> -		mt9m111->swap_yuv_cb_cr = 1;
> -		ret = mt9m111_setfmt_yuv(client);
> +		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
>  		break;
>  	case V4L2_MBUS_FMT_YUYV8_2X8_LE:
> -		mt9m111->swap_yuv_y_chromas = 1;
> -		mt9m111->swap_yuv_cb_cr = 0;
> -		ret = mt9m111_setfmt_yuv(client);
> +		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_C_Y;
>  		break;
>  	case V4L2_MBUS_FMT_YVYU8_2X8_LE:
> -		mt9m111->swap_yuv_y_chromas = 1;
> -		mt9m111->swap_yuv_cb_cr = 1;
> -		ret = mt9m111_setfmt_yuv(client);
> +		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_C_Y |
> +			MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
>  		break;
>  	default:
>  		dev_err(&client->dev, "Pixel format not handled : %x\n",
> @@ -560,6 +513,25 @@ static int mt9m111_set_pixfmt(struct i2c_client *client,
>  		ret = -EINVAL;
>  	}
>  
> +	mask_outfmt1 = MT9M111_OUTFMT_FLIP_BAYER_COL |
> +		MT9M111_OUTFMT_FLIP_BAYER_ROW;
> +
> +	mask_outfmt2 = MT9M111_OUTFMT_PROCESSED_BAYER |
> +		MT9M111_OUTFMT_BYPASS_IFP | MT9M111_OUTFMT_RGB |
> +		MT9M111_OUTFMT_RGB565 | MT9M111_OUTFMT_RGB555 |
> +		MT9M111_OUTFMT_RGB444x | MT9M111_OUTFMT_RGBx444 |
> +		MT9M111_OUTFMT_SWAP_YCbCr_C_Y | MT9M111_OUTFMT_SWAP_RGB_EVEN |
> +		MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr | MT9M111_OUTFMT_SWAP_RGB_R_B;
> +
> +	ret = reg_mask(OUTPUT_FORMAT_CTRL, data_outfmt1, mask_outfmt1);
> +
> +	if (!ret)
> +		ret = reg_mask(OUTPUT_FORMAT_CTRL2_A, data_outfmt2,
> +			mask_outfmt2);
> +	if (!ret)
> +		ret = reg_mask(OUTPUT_FORMAT_CTRL2_B, data_outfmt2,
> +			mask_outfmt2);
> +
>  	return ret;
>  }
>  
> @@ -989,9 +961,6 @@ static int mt9m111_video_probe(struct soc_camera_device *icd,
>  	mt9m111->autoexposure = 1;
>  	mt9m111->autowhitebalance = 1;
>  
> -	mt9m111->swap_rgb_even_odd = 1;
> -	mt9m111->swap_rgb_red_blue = 1;
> -
>  	data = reg_read(CHIP_VERSION);
>  
>  	switch (data) {
> -- 
> 1.7.1
> 
> 

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

Re: [PATCH 04/11] mt9m111: added new bit offset defines

[Guennadi Liakhovetski]

On Tue, 3 Aug 2010, Michael Grzeschik wrote:

> Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

I don't see these being used in any of your patches...

Thanks
Guennadi

> ---
>  drivers/media/video/mt9m111.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
> index 8c076e5..1b21522 100644
> --- a/drivers/media/video/mt9m111.c
> +++ b/drivers/media/video/mt9m111.c
> @@ -63,6 +63,12 @@
>  #define MT9M111_RESET_RESTART_FRAME	(1 << 1)
>  #define MT9M111_RESET_RESET_MODE	(1 << 0)
>  
> +#define MT9M111_RM_FULL_POWER_RD	(0 << 10)
> +#define MT9M111_RM_LOW_POWER_RD		(1 << 10)
> +#define MT9M111_RM_COL_SKIP_4X		(1 << 5)
> +#define MT9M111_RM_ROW_SKIP_4X		(1 << 4)
> +#define MT9M111_RM_COL_SKIP_2X		(1 << 3)
> +#define MT9M111_RM_ROW_SKIP_2X		(1 << 2)
>  #define MT9M111_RMB_MIRROR_COLS		(1 << 1)
>  #define MT9M111_RMB_MIRROR_ROWS		(1 << 0)
>  #define MT9M111_CTXT_CTRL_RESTART	(1 << 15)
> -- 
> 1.7.1
> 
> 

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

Re: [PATCH 04/11] mt9m111: added new bit offset defines

[Michael Grzeschik]

On Fri, Aug 27, 2010 at 05:11:18PM +0200, Guennadi Liakhovetski wrote:
> On Tue, 3 Aug 2010, Michael Grzeschik wrote:
> 
> > Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> I don't see these being used in any of your patches...
Yes, these are not used. They are a left over from the previous patchstack.
But they are checked against the datasheet and are correct.
Is it a problem to take them anyway?

Thanks,
Michael

> > ---
> >  drivers/media/video/mt9m111.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
> > index 8c076e5..1b21522 100644
> > --- a/drivers/media/video/mt9m111.c
> > +++ b/drivers/media/video/mt9m111.c
> > @@ -63,6 +63,12 @@
> >  #define MT9M111_RESET_RESTART_FRAME	(1 << 1)
> >  #define MT9M111_RESET_RESET_MODE	(1 << 0)
> >  
> > +#define MT9M111_RM_FULL_POWER_RD	(0 << 10)
> > +#define MT9M111_RM_LOW_POWER_RD		(1 << 10)
> > +#define MT9M111_RM_COL_SKIP_4X		(1 << 5)
> > +#define MT9M111_RM_ROW_SKIP_4X		(1 << 4)
> > +#define MT9M111_RM_COL_SKIP_2X		(1 << 3)
> > +#define MT9M111_RM_ROW_SKIP_2X		(1 << 2)
> >  #define MT9M111_RMB_MIRROR_COLS		(1 << 1)
> >  #define MT9M111_RMB_MIRROR_ROWS		(1 << 0)
> >  #define MT9M111_CTXT_CTRL_RESTART	(1 << 15)
> > -- 
> > 1.7.1
> > 
> > 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 04/11] mt9m111: added new bit offset defines

[Guennadi Liakhovetski]

On Fri, 27 Aug 2010, Michael Grzeschik wrote:

> On Fri, Aug 27, 2010 at 05:11:18PM +0200, Guennadi Liakhovetski wrote:
> > On Tue, 3 Aug 2010, Michael Grzeschik wrote:
> > 
> > > Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
> > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > 
> > I don't see these being used in any of your patches...
> Yes, these are not used. They are a left over from the previous patchstack.
> But they are checked against the datasheet and are correct.
> Is it a problem to take them anyway?

It is not a problem, it is unneeded. You do not want to add all registers 
and all their fields to every driver, do you? There are some drivers in 
the kernel, that define more registers, than are used. Of course, say, if 
you use bits 0, 1, 2, and 4 of a register, you might as well define bit 3 
- especially, if they are logically related. But this patch adds a whole 
family of parameters, none of which is used, so, I personally would avoid 
that.

Thanks
Guennadi

> 
> Thanks,
> Michael
> 
> > > ---
> > >  drivers/media/video/mt9m111.c |    6 ++++++
> > >  1 files changed, 6 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
> > > index 8c076e5..1b21522 100644
> > > --- a/drivers/media/video/mt9m111.c
> > > +++ b/drivers/media/video/mt9m111.c
> > > @@ -63,6 +63,12 @@
> > >  #define MT9M111_RESET_RESTART_FRAME	(1 << 1)
> > >  #define MT9M111_RESET_RESET_MODE	(1 << 0)
> > >  
> > > +#define MT9M111_RM_FULL_POWER_RD	(0 << 10)
> > > +#define MT9M111_RM_LOW_POWER_RD		(1 << 10)
> > > +#define MT9M111_RM_COL_SKIP_4X		(1 << 5)
> > > +#define MT9M111_RM_ROW_SKIP_4X		(1 << 4)
> > > +#define MT9M111_RM_COL_SKIP_2X		(1 << 3)
> > > +#define MT9M111_RM_ROW_SKIP_2X		(1 << 2)
> > >  #define MT9M111_RMB_MIRROR_COLS		(1 << 1)
> > >  #define MT9M111_RMB_MIRROR_ROWS		(1 << 0)
> > >  #define MT9M111_CTXT_CTRL_RESTART	(1 << 15)
> > > -- 
> > > 1.7.1
> > > 
> > > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

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

Re: [PATCH 04/11] mt9m111: added new bit offset defines

[Michael Grzeschik]

On Fri, Aug 27, 2010 at 06:30:28PM +0200, Guennadi Liakhovetski wrote:
> On Fri, 27 Aug 2010, Michael Grzeschik wrote:
> 
> > On Fri, Aug 27, 2010 at 05:11:18PM +0200, Guennadi Liakhovetski wrote:
> > > On Tue, 3 Aug 2010, Michael Grzeschik wrote:
> > > 
> > > > Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
> > > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > > 
> > > I don't see these being used in any of your patches...
> > Yes, these are not used. They are a left over from the previous patchstack.
> > But they are checked against the datasheet and are correct.
> > Is it a problem to take them anyway?
> 
> It is not a problem, it is unneeded. You do not want to add all registers 
> and all their fields to every driver, do you? There are some drivers in 
> the kernel, that define more registers, than are used. Of course, say, if 
> you use bits 0, 1, 2, and 4 of a register, you might as well define bit 3 
> - especially, if they are logically related. But this patch adds a whole 
> family of parameters, none of which is used, so, I personally would avoid 
> that.

Ok, no big deal. Personally i don't have a problem with additional
inexpensive registers and fields. As they often can be a good hint to
some functionality of a chip before you begin to scroll through the,
sometimes not so easy to find, datasheets. But that is probably a pure
matter of taste.

Regards,
Michael

> > 
> > > > ---
> > > >  drivers/media/video/mt9m111.c |    6 ++++++
> > > >  1 files changed, 6 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
> > > > index 8c076e5..1b21522 100644
> > > > --- a/drivers/media/video/mt9m111.c
> > > > +++ b/drivers/media/video/mt9m111.c
> > > > @@ -63,6 +63,12 @@
> > > >  #define MT9M111_RESET_RESTART_FRAME	(1 << 1)
> > > >  #define MT9M111_RESET_RESET_MODE	(1 << 0)
> > > >  
> > > > +#define MT9M111_RM_FULL_POWER_RD	(0 << 10)
> > > > +#define MT9M111_RM_LOW_POWER_RD		(1 << 10)
> > > > +#define MT9M111_RM_COL_SKIP_4X		(1 << 5)
> > > > +#define MT9M111_RM_ROW_SKIP_4X		(1 << 4)
> > > > +#define MT9M111_RM_COL_SKIP_2X		(1 << 3)
> > > > +#define MT9M111_RM_ROW_SKIP_2X		(1 << 2)
> > > >  #define MT9M111_RMB_MIRROR_COLS		(1 << 1)
> > > >  #define MT9M111_RMB_MIRROR_ROWS		(1 << 0)
> > > >  #define MT9M111_CTXT_CTRL_RESTART	(1 << 15)
> > > > -- 
> > > > 1.7.1
> > > > 
> > > > 
> > 
> > -- 
> > Pengutronix e.K.                           |                             |
> > Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> > 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 11/11] mt9m111: make use of testpattern

[Robert Jarzmik]

Michael Grzeschik <m.grzeschik@pengutronix.de> writes:

> Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

I would require a small change here.

I am using the testpattern for non regression tests. This change implies that
the test pattern can only be set up by module parameters, and blocks the usage
through V4L2 debug, registers, see below:
        memset(&set_reg, 0, sizeof(set_reg));
        set_reg.match.type = V4L2_CHIP_MATCH_I2C_ADDR;
        set_reg.match.addr = 0x5d;
        set_reg.reg = 0x148;
        set_reg.val = test_pattern;
        set_reg.size = 1;
        if (test_pattern != -1)
                if (-1 == xioctl (fd, VIDIOC_DBG_S_REGISTER, &set_reg)) {
                        fprintf (stderr, "%s could set test pattern %x\n",
                                 dev_name, test_pattern);
                        exit (EXIT_FAILURE);
                }

But, the idea is not bad. Therefore, I'd like you to change:
> +	dev_dbg(&client->dev, "%s: using testpattern %d\n", __func__,
> +			testpattern);
> +
> +	if (!ret)
> +		ret = mt9m111_reg_set(client,
> +				MT9M111_TEST_PATTERN_GEN, pattern);
into
> +	dev_dbg(&client->dev, "%s: using testpattern %d\n", __func__,
> +			testpattern);
> +
> +	if (!ret && pattern)
> +		ret = mt9m111_reg_set(client,
> +				MT9M111_TEST_PATTERN_GEN, pattern);
> +

This way, the V4L2 debug registers usage is still allowed, and your module
parameter works too.

Cheers.

--
Robert

Re: [PATCH v2 11/11] mt9m111: make use of testpattern

[Guennadi Liakhovetski]

On Sun, 29 Aug 2010, Robert Jarzmik wrote:

> Michael Grzeschik <m.grzeschik@pengutronix.de> writes:
> 
> > Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> I would require a small change here.
> 
> I am using the testpattern for non regression tests. This change implies that
> the test pattern can only be set up by module parameters, and blocks the usage
> through V4L2 debug, registers, see below:
>         memset(&set_reg, 0, sizeof(set_reg));
>         set_reg.match.type = V4L2_CHIP_MATCH_I2C_ADDR;
>         set_reg.match.addr = 0x5d;
>         set_reg.reg = 0x148;
>         set_reg.val = test_pattern;
>         set_reg.size = 1;
>         if (test_pattern != -1)
>                 if (-1 == xioctl (fd, VIDIOC_DBG_S_REGISTER, &set_reg)) {
>                         fprintf (stderr, "%s could set test pattern %x\n",
>                                  dev_name, test_pattern);
>                         exit (EXIT_FAILURE);
>                 }
> 
> But, the idea is not bad. Therefore, I'd like you to change:
> > +	dev_dbg(&client->dev, "%s: using testpattern %d\n", __func__,
> > +			testpattern);
> > +
> > +	if (!ret)
> > +		ret = mt9m111_reg_set(client,
> > +				MT9M111_TEST_PATTERN_GEN, pattern);
> into
> > +	dev_dbg(&client->dev, "%s: using testpattern %d\n", __func__,
> > +			testpattern);
> > +
> > +	if (!ret && pattern)
> > +		ret = mt9m111_reg_set(client,
> > +				MT9M111_TEST_PATTERN_GEN, pattern);
> > +
> 
> This way, the V4L2 debug registers usage is still allowed, and your module
> parameter works too.

Yes, but this has another disadvantage - if you do not use s_register / 
g_register, maybe you just have CONFIG_VIDEO_ADV_DEBUG off, then, once you 
load the module with the testpattern parameter, you cannot switch using 
testpatterns off again (without a reboot or a power cycle). With the 
original version you can load the driver with the parameter set, then 
unload it, load it without the parameter and testpattern would be cleared. 
In general, I think, using direct register access is discouraged, 
especially if there's a way to set the same functionality using driver's 
supported interfaces. Hm, if I'm not mistaken, it has once been mentioned, 
that these test-patterns can be nicely implemented using the S_INPUT 
ioctl(). Am I right? How about that? But we'd need a confirmation for 
that, I'm not 100% sure.

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

Re: [PATCH v2 10/11] mt9m111: rewrite set_pixfmt

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> Robert, I'll need your ack / tested by on this one too. It actually 
> changes behaviour, for example, it sets MT9M111_OUTFMT_FLIP_BAYER_ROW in 
> the OUTPUT_FORMAT_CTRL register for the V4L2_MBUS_FMT_SBGGR8_1X8 8 bit 
> Bayer format. Maybe other things too - please have a look.

For the YUV and RGB formats, tested and acked.
For the bayer, I don't use it. With row switch, that gives back:
byte offset: 0 1 2 3
             B G B G
             G R G R

Without the switch:
byte offset: 0 1 2 3
             G R G R
             B G B G

I would have expected the second version (ie. without the switch, ie. the
original version of mt9m111 driver) to be correct, but I might be wrong. Maybe
Michael can enlighten me here.

-- 
Robert

Re: [PATCH v2 00/11] MT9M111/MT9M131

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> On Tue, 17 Aug 2010, Michael Grzeschik wrote:
>
>> Hi Robert, Guennadi,
>> 
>> after the messed up previous patchseries, this v2 series is left
>> without any feedback. Hopefully not forgotten. :-)
>
> No, it is not forgotten, please, give me some more time.

Hi,

As you may have noticed, I'm very busy and my reviews are very slow lately. I'm
sorry for that, as my work pumps out every bit of energy I have.

Except for patch 11 where I'd like a little amendment, I have tested the full
serie, and my feeling is that things work at least as well as before, and
probably even better.

Therefore, as I won't have much time ahead, please find my ack, for the full serie:
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

-- 
Robert

Re: [PATCH v2 10/11] mt9m111: rewrite set_pixfmt

[Michael Grzeschik]

Hi Robert and Guennadi

On Sun, Aug 29, 2010 at 09:17:00PM +0200, Robert Jarzmik wrote:
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > Robert, I'll need your ack / tested by on this one too. It actually 
> > changes behaviour, for example, it sets MT9M111_OUTFMT_FLIP_BAYER_ROW in 
> > the OUTPUT_FORMAT_CTRL register for the V4L2_MBUS_FMT_SBGGR8_1X8 8 bit 
> > Bayer format. Maybe other things too - please have a look.
> 
> For the YUV and RGB formats, tested and acked.
> For the bayer, I don't use it. With row switch, that gives back:
> byte offset: 0 1 2 3
>              B G B G
>              G R G R
> 
> Without the switch:
> byte offset: 0 1 2 3
>              G R G R
>              B G B G
> 
> I would have expected the second version (ie. without the switch, ie. the
> original version of mt9m111 driver) to be correct, but I might be wrong. Maybe
> Michael can enlighten me here.
Yes this seems odd, i normaly expect the first line to be BGBG.
I will search for the cause and reply a little later, perhaps end of
the week, since i am also short on time at this moment.

Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 00/11] MT9M111/MT9M131

[Michael Grzeschik]

Hi,

On Sun, Aug 29, 2010 at 09:20:06PM +0200, Robert Jarzmik wrote:
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > On Tue, 17 Aug 2010, Michael Grzeschik wrote:
> >
> >> Hi Robert, Guennadi,
> >> 
> >> after the messed up previous patchseries, this v2 series is left
> >> without any feedback. Hopefully not forgotten. :-)
> >
> > No, it is not forgotten, please, give me some more time.

> As you may have noticed, I'm very busy and my reviews are very slow lately. I'm
> sorry for that, as my work pumps out every bit of energy I have.
Perhaps not so bad, but other priorities at the moment here.

> Except for patch 11 where I'd like a little amendment, I have tested the full
> serie, and my feeling is that things work at least as well as before, and
> probably even better.
>
> Therefore, as I won't have much time ahead, please find my ack, for the full serie:
> Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
Anyway, thanks for your time and work so far.

Greetings,
Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 10/11] mt9m111: rewrite set_pixfmt

[Guennadi Liakhovetski]

On Tue, 31 Aug 2010, Michael Grzeschik wrote:

> Hi Robert and Guennadi
> 
> On Sun, Aug 29, 2010 at 09:17:00PM +0200, Robert Jarzmik wrote:
> > Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> > 
> > > Robert, I'll need your ack / tested by on this one too. It actually 
> > > changes behaviour, for example, it sets MT9M111_OUTFMT_FLIP_BAYER_ROW in 
> > > the OUTPUT_FORMAT_CTRL register for the V4L2_MBUS_FMT_SBGGR8_1X8 8 bit 
> > > Bayer format. Maybe other things too - please have a look.
> > 
> > For the YUV and RGB formats, tested and acked.
> > For the bayer, I don't use it. With row switch, that gives back:
> > byte offset: 0 1 2 3
> >              B G B G
> >              G R G R
> > 
> > Without the switch:
> > byte offset: 0 1 2 3
> >              G R G R
> >              B G B G
> > 
> > I would have expected the second version (ie. without the switch, ie. the
> > original version of mt9m111 driver) to be correct, but I might be wrong. Maybe
> > Michael can enlighten me here.
> Yes this seems odd, i normaly expect the first line to be BGBG.
> I will search for the cause and reply a little later, perhaps end of
> the week, since i am also short on time at this moment.

Ok, _if_ you have to redo this patch, maybe you could also merge

[PATCH 04/11] mt9m111: added new bit offset defines
[PATCH 08/11] mt9m111: added reg_mask function

into it, otherwise their purpose is unclear.

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

Re: [PATCH v2 11/11] mt9m111: make use of testpattern

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> Yes, but this has another disadvantage - if you do not use s_register / 
> g_register, maybe you just have CONFIG_VIDEO_ADV_DEBUG off, then, once you 
> load the module with the testpattern parameter, you cannot switch using 
> testpatterns off again (without a reboot or a power cycle). With the 
> original version you can load the driver with the parameter set, then 
> unload it, load it without the parameter and testpattern would be cleared. 
> In general, I think, using direct register access is discouraged, 
> especially if there's a way to set the same functionality using driver's 
> supported interfaces.

I agree. If there is a way without debug registers, let's use it.

> Hm, if I'm not mistaken, it has once been mentioned, that these test-patterns
> can be nicely implemented using the S_INPUT ioctl(). Am I right? How about
> that? But we'd need a confirmation for that, I'm not 100% sure.
I can't remember that. But if there is a standard ioctl (as seems to show
videodev2.h), and that its use could mean "camera's input is a testpattern" or
"camera input is the normal optical flow", then we should use it.
If not, the old way with debug registers is the only alternative I see without
having to unload/reload the module (if it's a module and not statically embedded
in the kernel).

Cheers.

--
Robert

Re: [PATCH v2 10/11] mt9m111: rewrite set_pixfmt

[Guennadi Liakhovetski]

Michael, any insight?

Thanks
Guennadi

On Sat, 4 Sep 2010, Guennadi Liakhovetski wrote:

> On Tue, 31 Aug 2010, Michael Grzeschik wrote:
> 
> > Hi Robert and Guennadi
> > 
> > On Sun, Aug 29, 2010 at 09:17:00PM +0200, Robert Jarzmik wrote:
> > > Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> > > 
> > > > Robert, I'll need your ack / tested by on this one too. It actually 
> > > > changes behaviour, for example, it sets MT9M111_OUTFMT_FLIP_BAYER_ROW in 
> > > > the OUTPUT_FORMAT_CTRL register for the V4L2_MBUS_FMT_SBGGR8_1X8 8 bit 
> > > > Bayer format. Maybe other things too - please have a look.
> > > 
> > > For the YUV and RGB formats, tested and acked.
> > > For the bayer, I don't use it. With row switch, that gives back:
> > > byte offset: 0 1 2 3
> > >              B G B G
> > >              G R G R
> > > 
> > > Without the switch:
> > > byte offset: 0 1 2 3
> > >              G R G R
> > >              B G B G
> > > 
> > > I would have expected the second version (ie. without the switch, ie. the
> > > original version of mt9m111 driver) to be correct, but I might be wrong. Maybe
> > > Michael can enlighten me here.
> > Yes this seems odd, i normaly expect the first line to be BGBG.
> > I will search for the cause and reply a little later, perhaps end of
> > the week, since i am also short on time at this moment.
> 
> Ok, _if_ you have to redo this patch, maybe you could also merge
> 
> [PATCH 04/11] mt9m111: added new bit offset defines
> [PATCH 08/11] mt9m111: added reg_mask function
> 
> into it, otherwise their purpose is unclear.
> 
> Thanks
> Guennadi
> ---
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

Re: [PATCH v2 10/11] mt9m111: rewrite set_pixfmt

[Michael Grzeschik]

On Sat, Oct 02, 2010 at 10:03:55AM +0200, Guennadi Liakhovetski wrote:
> Michael, any insight?

long time ago...

> > > > For the YUV and RGB formats, tested and acked.
> > > > For the bayer, I don't use it. With row switch, that gives back:
> > > > byte offset: 0 1 2 3
> > > >              B G B G
> > > >              G R G R
> > > > 
> > > > Without the switch:
> > > > byte offset: 0 1 2 3
> > > >              G R G R
> > > >              B G B G
> > > > 
> > > > I would have expected the second version (ie. without the switch, ie. the
> > > > original version of mt9m111 driver) to be correct, but I might be wrong. Maybe
> > > > Michael can enlighten me here.
> > > Yes this seems odd, i normaly expect the first line to be BGBG.
> > > I will search for the cause and reply a little later, perhaps end of
> > > the week, since i am also short on time at this moment.

I have reviewed the Datasheet of the Camera and found Roberts previously
described behaviour as correct. So the Bayercode seems functional in
that patch.

> > Ok, _if_ you have to redo this patch, maybe you could also merge
> > 
> > [PATCH 04/11] mt9m111: added new bit offset defines
> > [PATCH 08/11] mt9m111: added reg_mask function
> > 
> > into it, otherwise their purpose is unclear.

I will send a squashed Version of the three patches in some minutes.

Cheers,
Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v3] mt9m111: rewrite set_pixfmt

[Michael Grzeschik]

added new bit offset defines,
more supported BE colour formats
and also support BGR565 swapped pixel formats

removed pixfmt helper functions and option flags
setting the configuration register directly in set_pixfmt

added reg_mask function

reg_mask is basically the same as clearing & setting registers,
but it is more convenient and faster (saves one rw cycle).

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
---
Changes v1 -> v2
        * removed unrelated OPMODE handling in this function

Changes v2 -> v3
        * squashed: "[PATCH 04/11] mt9m111: added new bit offset defines"
	* squashed: "[PATCH 08/11] mt9m111: added reg_mask function"

 drivers/media/video/mt9m111.c |  176 +++++++++++++++++++----------------------
 1 files changed, 81 insertions(+), 95 deletions(-)

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index 3eeda19..9da30c0 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -63,6 +63,12 @@
 #define MT9M111_RESET_RESTART_FRAME	(1 << 1)
 #define MT9M111_RESET_RESET_MODE	(1 << 0)
 
+#define MT9M111_RM_FULL_POWER_RD	(0 << 10)
+#define MT9M111_RM_LOW_POWER_RD		(1 << 10)
+#define MT9M111_RM_COL_SKIP_4X		(1 << 5)
+#define MT9M111_RM_ROW_SKIP_4X		(1 << 4)
+#define MT9M111_RM_COL_SKIP_2X		(1 << 3)
+#define MT9M111_RM_ROW_SKIP_2X		(1 << 2)
 #define MT9M111_RMB_MIRROR_COLS		(1 << 1)
 #define MT9M111_RMB_MIRROR_ROWS		(1 << 0)
 #define MT9M111_CTXT_CTRL_RESTART	(1 << 15)
@@ -95,7 +101,8 @@
 
 #define MT9M111_OPMODE_AUTOEXPO_EN	(1 << 14)
 #define MT9M111_OPMODE_AUTOWHITEBAL_EN	(1 << 1)
-
+#define MT9M111_OUTFMT_FLIP_BAYER_COL  (1 << 9)
+#define MT9M111_OUTFMT_FLIP_BAYER_ROW  (1 << 8)
 #define MT9M111_OUTFMT_PROCESSED_BAYER	(1 << 14)
 #define MT9M111_OUTFMT_BYPASS_IFP	(1 << 10)
 #define MT9M111_OUTFMT_INV_PIX_CLOCK	(1 << 9)
@@ -113,6 +120,7 @@
 #define MT9M111_OUTFMT_SWAP_YCbCr_C_Y	(1 << 1)
 #define MT9M111_OUTFMT_SWAP_RGB_EVEN	(1 << 1)
 #define MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr	(1 << 0)
+#define MT9M111_OUTFMT_SWAP_RGB_R_B	(1 << 0)
 
 /*
  * Camera control register addresses (0x200..0x2ff not implemented)
@@ -122,6 +130,8 @@
 #define reg_write(reg, val) mt9m111_reg_write(client, MT9M111_##reg, (val))
 #define reg_set(reg, val) mt9m111_reg_set(client, MT9M111_##reg, (val))
 #define reg_clear(reg, val) mt9m111_reg_clear(client, MT9M111_##reg, (val))
+#define reg_mask(reg, val, mask) mt9m111_reg_mask(client, MT9M111_##reg, \
+		(val), (mask))
 
 #define MT9M111_MIN_DARK_ROWS	8
 #define MT9M111_MIN_DARK_COLS	26
@@ -148,12 +158,16 @@ static const struct mt9m111_datafmt *mt9m111_find_datafmt(
 }
 
 static const struct mt9m111_datafmt mt9m111_colour_fmts[] = {
-	{V4L2_MBUS_FMT_YUYV8_2X8, V4L2_COLORSPACE_JPEG},
-	{V4L2_MBUS_FMT_YVYU8_2X8, V4L2_COLORSPACE_JPEG},
-	{V4L2_MBUS_FMT_UYVY8_2X8, V4L2_COLORSPACE_JPEG},
-	{V4L2_MBUS_FMT_VYUY8_2X8, V4L2_COLORSPACE_JPEG},
+	{V4L2_MBUS_FMT_YUYV8_2X8_LE, V4L2_COLORSPACE_JPEG},
+	{V4L2_MBUS_FMT_YVYU8_2X8_LE, V4L2_COLORSPACE_JPEG},
+	{V4L2_MBUS_FMT_YUYV8_2X8_BE, V4L2_COLORSPACE_JPEG},
+	{V4L2_MBUS_FMT_YVYU8_2X8_BE, V4L2_COLORSPACE_JPEG},
 	{V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE, V4L2_COLORSPACE_SRGB},
+	{V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE, V4L2_COLORSPACE_SRGB},
 	{V4L2_MBUS_FMT_RGB565_2X8_LE, V4L2_COLORSPACE_SRGB},
+	{V4L2_MBUS_FMT_RGB565_2X8_BE, V4L2_COLORSPACE_SRGB},
+	{V4L2_MBUS_FMT_BGR565_2X8_LE, V4L2_COLORSPACE_SRGB},
+	{V4L2_MBUS_FMT_BGR565_2X8_BE, V4L2_COLORSPACE_SRGB},
 	{V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_COLORSPACE_SRGB},
 	{V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE, V4L2_COLORSPACE_SRGB},
 };
@@ -176,10 +190,6 @@ struct mt9m111 {
 	unsigned int powered:1;
 	unsigned int hflip:1;
 	unsigned int vflip:1;
-	unsigned int swap_rgb_even_odd:1;
-	unsigned int swap_rgb_red_blue:1;
-	unsigned int swap_yuv_y_chromas:1;
-	unsigned int swap_yuv_cb_cr:1;
 	unsigned int autowhitebalance:1;
 };
 
@@ -251,6 +261,15 @@ static int mt9m111_reg_clear(struct i2c_client *client, const u16 reg,
 	return mt9m111_reg_write(client, reg, ret & ~data);
 }
 
+static int mt9m111_reg_mask(struct i2c_client *client, const u16 reg,
+			    const u16 data, const u16 mask)
+{
+	int ret;
+
+	ret = mt9m111_reg_read(client, reg);
+	return mt9m111_reg_write(client, reg, (ret & ~mask) | data);
+}
+
 static int mt9m111_set_context(struct i2c_client *client,
 			       enum mt9m111_context ctxt)
 {
@@ -312,68 +331,6 @@ static int mt9m111_setup_rect(struct i2c_client *client,
 	return ret;
 }
 
-static int mt9m111_setup_pixfmt(struct i2c_client *client, u16 outfmt)
-{
-	int ret;
-
-	ret = reg_write(OUTPUT_FORMAT_CTRL2_A, outfmt);
-	if (!ret)
-		ret = reg_write(OUTPUT_FORMAT_CTRL2_B, outfmt);
-	return ret;
-}
-
-static int mt9m111_setfmt_bayer8(struct i2c_client *client)
-{
-	return mt9m111_setup_pixfmt(client, MT9M111_OUTFMT_PROCESSED_BAYER |
-				    MT9M111_OUTFMT_RGB);
-}
-
-static int mt9m111_setfmt_bayer10(struct i2c_client *client)
-{
-	return mt9m111_setup_pixfmt(client, MT9M111_OUTFMT_BYPASS_IFP);
-}
-
-static int mt9m111_setfmt_rgb565(struct i2c_client *client)
-{
-	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int val = 0;
-
-	if (mt9m111->swap_rgb_red_blue)
-		val |= MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
-	if (mt9m111->swap_rgb_even_odd)
-		val |= MT9M111_OUTFMT_SWAP_RGB_EVEN;
-	val |= MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB565;
-
-	return mt9m111_setup_pixfmt(client, val);
-}
-
-static int mt9m111_setfmt_rgb555(struct i2c_client *client)
-{
-	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int val = 0;
-
-	if (mt9m111->swap_rgb_red_blue)
-		val |= MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
-	if (mt9m111->swap_rgb_even_odd)
-		val |= MT9M111_OUTFMT_SWAP_RGB_EVEN;
-	val |= MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB555;
-
-	return mt9m111_setup_pixfmt(client, val);
-}
-
-static int mt9m111_setfmt_yuv(struct i2c_client *client)
-{
-	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int val = 0;
-
-	if (mt9m111->swap_yuv_cb_cr)
-		val |= MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
-	if (mt9m111->swap_yuv_y_chromas)
-		val |= MT9M111_OUTFMT_SWAP_YCbCr_C_Y;
-
-	return mt9m111_setup_pixfmt(client, val);
-}
-
 static int mt9m111_enable(struct i2c_client *client)
 {
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
@@ -501,41 +458,54 @@ static int mt9m111_g_fmt(struct v4l2_subdev *sd,
 static int mt9m111_set_pixfmt(struct i2c_client *client,
 			      enum v4l2_mbus_pixelcode code)
 {
-	struct mt9m111 *mt9m111 = to_mt9m111(client);
+	u16 data_outfmt1 = 0, data_outfmt2 = 0, mask_outfmt1, mask_outfmt2;
 	int ret;
 
 	switch (code) {
 	case V4L2_MBUS_FMT_SBGGR8_1X8:
-		ret = mt9m111_setfmt_bayer8(client);
+		data_outfmt1 = MT9M111_OUTFMT_FLIP_BAYER_ROW;
+		data_outfmt2 = MT9M111_OUTFMT_PROCESSED_BAYER |
+			MT9M111_OUTFMT_RGB;
 		break;
 	case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE:
-		ret = mt9m111_setfmt_bayer10(client);
+		data_outfmt2 = MT9M111_OUTFMT_BYPASS_IFP | MT9M111_OUTFMT_RGB;
 		break;
 	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE:
-		ret = mt9m111_setfmt_rgb555(client);
+		data_outfmt2 = MT9M111_OUTFMT_SWAP_RGB_EVEN |
+			MT9M111_OUTFMT_RGB |
+			MT9M111_OUTFMT_RGB555;
+		break;
+	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE:
+		data_outfmt2 = MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB555;
 		break;
 	case V4L2_MBUS_FMT_RGB565_2X8_LE:
-		ret = mt9m111_setfmt_rgb565(client);
+		data_outfmt2 = MT9M111_OUTFMT_SWAP_RGB_EVEN |
+			MT9M111_OUTFMT_RGB |
+			MT9M111_OUTFMT_RGB565;
+		break;
+	case V4L2_MBUS_FMT_RGB565_2X8_BE:
+		data_outfmt2 = MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB565;
 		break;
-	case V4L2_MBUS_FMT_UYVY8_2X8:
-		mt9m111->swap_yuv_y_chromas = 0;
-		mt9m111->swap_yuv_cb_cr = 0;
-		ret = mt9m111_setfmt_yuv(client);
+	case V4L2_MBUS_FMT_BGR565_2X8_LE:
+		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr |
+			MT9M111_OUTFMT_SWAP_RGB_EVEN |
+			MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB565;
 		break;
-	case V4L2_MBUS_FMT_VYUY8_2X8:
-		mt9m111->swap_yuv_y_chromas = 0;
-		mt9m111->swap_yuv_cb_cr = 1;
-		ret = mt9m111_setfmt_yuv(client);
+	case V4L2_MBUS_FMT_BGR565_2X8_BE:
+		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr |
+			MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB565;
 		break;
-	case V4L2_MBUS_FMT_YUYV8_2X8:
-		mt9m111->swap_yuv_y_chromas = 1;
-		mt9m111->swap_yuv_cb_cr = 0;
-		ret = mt9m111_setfmt_yuv(client);
+	case V4L2_MBUS_FMT_YUYV8_2X8_BE:
 		break;
-	case V4L2_MBUS_FMT_YVYU8_2X8:
-		mt9m111->swap_yuv_y_chromas = 1;
-		mt9m111->swap_yuv_cb_cr = 1;
-		ret = mt9m111_setfmt_yuv(client);
+	case V4L2_MBUS_FMT_YVYU8_2X8_BE:
+		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
+		break;
+	case V4L2_MBUS_FMT_YUYV8_2X8_LE:
+		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_C_Y;
+		break;
+	case V4L2_MBUS_FMT_YVYU8_2X8_LE:
+		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_C_Y |
+			MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
 		break;
 	default:
 		dev_err(&client->dev, "Pixel format not handled : %x\n",
@@ -543,6 +513,25 @@ static int mt9m111_set_pixfmt(struct i2c_client *client,
 		ret = -EINVAL;
 	}
 
+	mask_outfmt1 = MT9M111_OUTFMT_FLIP_BAYER_COL |
+		MT9M111_OUTFMT_FLIP_BAYER_ROW;
+
+	mask_outfmt2 = MT9M111_OUTFMT_PROCESSED_BAYER |
+		MT9M111_OUTFMT_BYPASS_IFP | MT9M111_OUTFMT_RGB |
+		MT9M111_OUTFMT_RGB565 | MT9M111_OUTFMT_RGB555 |
+		MT9M111_OUTFMT_RGB444x | MT9M111_OUTFMT_RGBx444 |
+		MT9M111_OUTFMT_SWAP_YCbCr_C_Y | MT9M111_OUTFMT_SWAP_RGB_EVEN |
+		MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr | MT9M111_OUTFMT_SWAP_RGB_R_B;
+
+	ret = reg_mask(OUTPUT_FORMAT_CTRL, data_outfmt1, mask_outfmt1);
+
+	if (!ret)
+		ret = reg_mask(OUTPUT_FORMAT_CTRL2_A, data_outfmt2,
+			mask_outfmt2);
+	if (!ret)
+		ret = reg_mask(OUTPUT_FORMAT_CTRL2_B, data_outfmt2,
+			mask_outfmt2);
+
 	return ret;
 }
 
@@ -972,9 +961,6 @@ static int mt9m111_video_probe(struct soc_camera_device *icd,
 	mt9m111->autoexposure = 1;
 	mt9m111->autowhitebalance = 1;
 
-	mt9m111->swap_rgb_even_odd = 1;
-	mt9m111->swap_rgb_red_blue = 1;
-
 	data = reg_read(CHIP_VERSION);
 
 	switch (data) {
-- 
1.7.0

Re: [PATCH v3] mt9m111: rewrite set_pixfmt

[Guennadi Liakhovetski]

On Mon, 25 Oct 2010, Michael Grzeschik wrote:

> added new bit offset defines,
> more supported BE colour formats
> and also support BGR565 swapped pixel formats
> 
> removed pixfmt helper functions and option flags
> setting the configuration register directly in set_pixfmt
> 
> added reg_mask function
> 
> reg_mask is basically the same as clearing & setting registers,
> but it is more convenient and faster (saves one rw cycle).
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Philipp Wiesner <p.wiesner@phytec.de>
> ---
> Changes v1 -> v2
>         * removed unrelated OPMODE handling in this function
> 
> Changes v2 -> v3
>         * squashed: "[PATCH 04/11] mt9m111: added new bit offset defines"
> 	* squashed: "[PATCH 08/11] mt9m111: added reg_mask function"
> 
>  drivers/media/video/mt9m111.c |  176 +++++++++++++++++++----------------------
>  1 files changed, 81 insertions(+), 95 deletions(-)
> 
> diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
> index 3eeda19..9da30c0 100644
> --- a/drivers/media/video/mt9m111.c
> +++ b/drivers/media/video/mt9m111.c
> @@ -63,6 +63,12 @@
>  #define MT9M111_RESET_RESTART_FRAME	(1 << 1)
>  #define MT9M111_RESET_RESET_MODE	(1 << 0)
>  
> +#define MT9M111_RM_FULL_POWER_RD	(0 << 10)
> +#define MT9M111_RM_LOW_POWER_RD		(1 << 10)
> +#define MT9M111_RM_COL_SKIP_4X		(1 << 5)
> +#define MT9M111_RM_ROW_SKIP_4X		(1 << 4)
> +#define MT9M111_RM_COL_SKIP_2X		(1 << 3)
> +#define MT9M111_RM_ROW_SKIP_2X		(1 << 2)
>  #define MT9M111_RMB_MIRROR_COLS		(1 << 1)
>  #define MT9M111_RMB_MIRROR_ROWS		(1 << 0)
>  #define MT9M111_CTXT_CTRL_RESTART	(1 << 15)
> @@ -95,7 +101,8 @@
>  
>  #define MT9M111_OPMODE_AUTOEXPO_EN	(1 << 14)
>  #define MT9M111_OPMODE_AUTOWHITEBAL_EN	(1 << 1)
> -
> +#define MT9M111_OUTFMT_FLIP_BAYER_COL  (1 << 9)
> +#define MT9M111_OUTFMT_FLIP_BAYER_ROW  (1 << 8)
>  #define MT9M111_OUTFMT_PROCESSED_BAYER	(1 << 14)
>  #define MT9M111_OUTFMT_BYPASS_IFP	(1 << 10)
>  #define MT9M111_OUTFMT_INV_PIX_CLOCK	(1 << 9)
> @@ -113,6 +120,7 @@
>  #define MT9M111_OUTFMT_SWAP_YCbCr_C_Y	(1 << 1)
>  #define MT9M111_OUTFMT_SWAP_RGB_EVEN	(1 << 1)
>  #define MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr	(1 << 0)
> +#define MT9M111_OUTFMT_SWAP_RGB_R_B	(1 << 0)
>  
>  /*
>   * Camera control register addresses (0x200..0x2ff not implemented)
> @@ -122,6 +130,8 @@
>  #define reg_write(reg, val) mt9m111_reg_write(client, MT9M111_##reg, (val))
>  #define reg_set(reg, val) mt9m111_reg_set(client, MT9M111_##reg, (val))
>  #define reg_clear(reg, val) mt9m111_reg_clear(client, MT9M111_##reg, (val))
> +#define reg_mask(reg, val, mask) mt9m111_reg_mask(client, MT9M111_##reg, \
> +		(val), (mask))
>  
>  #define MT9M111_MIN_DARK_ROWS	8
>  #define MT9M111_MIN_DARK_COLS	26
> @@ -148,12 +158,16 @@ static const struct mt9m111_datafmt *mt9m111_find_datafmt(
>  }
>  
>  static const struct mt9m111_datafmt mt9m111_colour_fmts[] = {
> -	{V4L2_MBUS_FMT_YUYV8_2X8, V4L2_COLORSPACE_JPEG},
> -	{V4L2_MBUS_FMT_YVYU8_2X8, V4L2_COLORSPACE_JPEG},
> -	{V4L2_MBUS_FMT_UYVY8_2X8, V4L2_COLORSPACE_JPEG},
> -	{V4L2_MBUS_FMT_VYUY8_2X8, V4L2_COLORSPACE_JPEG},
> +	{V4L2_MBUS_FMT_YUYV8_2X8_LE, V4L2_COLORSPACE_JPEG},
> +	{V4L2_MBUS_FMT_YVYU8_2X8_LE, V4L2_COLORSPACE_JPEG},
> +	{V4L2_MBUS_FMT_YUYV8_2X8_BE, V4L2_COLORSPACE_JPEG},
> +	{V4L2_MBUS_FMT_YVYU8_2X8_BE, V4L2_COLORSPACE_JPEG},

This looks to me like you're breaking the driver. Please, develop against 
a recent v4l tree, or, in fact, against next. With this in mind, I don't 
think we still have a realistic chance to get this in for 2.6.37. In fact, 
it is _already_ too late, so, no way, sorry.

>  	{V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE, V4L2_COLORSPACE_SRGB},
> +	{V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE, V4L2_COLORSPACE_SRGB},
>  	{V4L2_MBUS_FMT_RGB565_2X8_LE, V4L2_COLORSPACE_SRGB},
> +	{V4L2_MBUS_FMT_RGB565_2X8_BE, V4L2_COLORSPACE_SRGB},
> +	{V4L2_MBUS_FMT_BGR565_2X8_LE, V4L2_COLORSPACE_SRGB},
> +	{V4L2_MBUS_FMT_BGR565_2X8_BE, V4L2_COLORSPACE_SRGB},
>  	{V4L2_MBUS_FMT_SBGGR8_1X8, V4L2_COLORSPACE_SRGB},
>  	{V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE, V4L2_COLORSPACE_SRGB},
>  };
> @@ -176,10 +190,6 @@ struct mt9m111 {
>  	unsigned int powered:1;
>  	unsigned int hflip:1;
>  	unsigned int vflip:1;
> -	unsigned int swap_rgb_even_odd:1;
> -	unsigned int swap_rgb_red_blue:1;
> -	unsigned int swap_yuv_y_chromas:1;
> -	unsigned int swap_yuv_cb_cr:1;
>  	unsigned int autowhitebalance:1;
>  };
>  
> @@ -251,6 +261,15 @@ static int mt9m111_reg_clear(struct i2c_client *client, const u16 reg,
>  	return mt9m111_reg_write(client, reg, ret & ~data);
>  }
>  
> +static int mt9m111_reg_mask(struct i2c_client *client, const u16 reg,
> +			    const u16 data, const u16 mask)
> +{
> +	int ret;
> +
> +	ret = mt9m111_reg_read(client, reg);
> +	return mt9m111_reg_write(client, reg, (ret & ~mask) | data);

Ok, I feel ashamed, that I have accepted this driver in this form... It is 
full of such buggy error handling instances, and this adds just one 
more... So, I would very appreciate if you could clean them up - before 
this patch, and handle this error properly too, otherwise I might do this 
myself some time... And, just noticed - "static int lastpage" from 
reg_page_map_set() must be moved into struct mt9m111, if this driver shall 
be able to handle more than one sensor simultaneously, at least in 
principle...

Thanks
Guennadi

> +}
> +
>  static int mt9m111_set_context(struct i2c_client *client,
>  			       enum mt9m111_context ctxt)
>  {
> @@ -312,68 +331,6 @@ static int mt9m111_setup_rect(struct i2c_client *client,
>  	return ret;
>  }
>  
> -static int mt9m111_setup_pixfmt(struct i2c_client *client, u16 outfmt)
> -{
> -	int ret;
> -
> -	ret = reg_write(OUTPUT_FORMAT_CTRL2_A, outfmt);
> -	if (!ret)
> -		ret = reg_write(OUTPUT_FORMAT_CTRL2_B, outfmt);
> -	return ret;
> -}
> -
> -static int mt9m111_setfmt_bayer8(struct i2c_client *client)
> -{
> -	return mt9m111_setup_pixfmt(client, MT9M111_OUTFMT_PROCESSED_BAYER |
> -				    MT9M111_OUTFMT_RGB);
> -}
> -
> -static int mt9m111_setfmt_bayer10(struct i2c_client *client)
> -{
> -	return mt9m111_setup_pixfmt(client, MT9M111_OUTFMT_BYPASS_IFP);
> -}
> -
> -static int mt9m111_setfmt_rgb565(struct i2c_client *client)
> -{
> -	struct mt9m111 *mt9m111 = to_mt9m111(client);
> -	int val = 0;
> -
> -	if (mt9m111->swap_rgb_red_blue)
> -		val |= MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
> -	if (mt9m111->swap_rgb_even_odd)
> -		val |= MT9M111_OUTFMT_SWAP_RGB_EVEN;
> -	val |= MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB565;
> -
> -	return mt9m111_setup_pixfmt(client, val);
> -}
> -
> -static int mt9m111_setfmt_rgb555(struct i2c_client *client)
> -{
> -	struct mt9m111 *mt9m111 = to_mt9m111(client);
> -	int val = 0;
> -
> -	if (mt9m111->swap_rgb_red_blue)
> -		val |= MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
> -	if (mt9m111->swap_rgb_even_odd)
> -		val |= MT9M111_OUTFMT_SWAP_RGB_EVEN;
> -	val |= MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB555;
> -
> -	return mt9m111_setup_pixfmt(client, val);
> -}
> -
> -static int mt9m111_setfmt_yuv(struct i2c_client *client)
> -{
> -	struct mt9m111 *mt9m111 = to_mt9m111(client);
> -	int val = 0;
> -
> -	if (mt9m111->swap_yuv_cb_cr)
> -		val |= MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
> -	if (mt9m111->swap_yuv_y_chromas)
> -		val |= MT9M111_OUTFMT_SWAP_YCbCr_C_Y;
> -
> -	return mt9m111_setup_pixfmt(client, val);
> -}
> -
>  static int mt9m111_enable(struct i2c_client *client)
>  {
>  	struct mt9m111 *mt9m111 = to_mt9m111(client);
> @@ -501,41 +458,54 @@ static int mt9m111_g_fmt(struct v4l2_subdev *sd,
>  static int mt9m111_set_pixfmt(struct i2c_client *client,
>  			      enum v4l2_mbus_pixelcode code)
>  {
> -	struct mt9m111 *mt9m111 = to_mt9m111(client);
> +	u16 data_outfmt1 = 0, data_outfmt2 = 0, mask_outfmt1, mask_outfmt2;
>  	int ret;
>  
>  	switch (code) {
>  	case V4L2_MBUS_FMT_SBGGR8_1X8:
> -		ret = mt9m111_setfmt_bayer8(client);
> +		data_outfmt1 = MT9M111_OUTFMT_FLIP_BAYER_ROW;
> +		data_outfmt2 = MT9M111_OUTFMT_PROCESSED_BAYER |
> +			MT9M111_OUTFMT_RGB;
>  		break;
>  	case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE:
> -		ret = mt9m111_setfmt_bayer10(client);
> +		data_outfmt2 = MT9M111_OUTFMT_BYPASS_IFP | MT9M111_OUTFMT_RGB;
>  		break;
>  	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE:
> -		ret = mt9m111_setfmt_rgb555(client);
> +		data_outfmt2 = MT9M111_OUTFMT_SWAP_RGB_EVEN |
> +			MT9M111_OUTFMT_RGB |
> +			MT9M111_OUTFMT_RGB555;
> +		break;
> +	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE:
> +		data_outfmt2 = MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB555;
>  		break;
>  	case V4L2_MBUS_FMT_RGB565_2X8_LE:
> -		ret = mt9m111_setfmt_rgb565(client);
> +		data_outfmt2 = MT9M111_OUTFMT_SWAP_RGB_EVEN |
> +			MT9M111_OUTFMT_RGB |
> +			MT9M111_OUTFMT_RGB565;
> +		break;
> +	case V4L2_MBUS_FMT_RGB565_2X8_BE:
> +		data_outfmt2 = MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB565;
>  		break;
> -	case V4L2_MBUS_FMT_UYVY8_2X8:
> -		mt9m111->swap_yuv_y_chromas = 0;
> -		mt9m111->swap_yuv_cb_cr = 0;
> -		ret = mt9m111_setfmt_yuv(client);
> +	case V4L2_MBUS_FMT_BGR565_2X8_LE:
> +		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr |
> +			MT9M111_OUTFMT_SWAP_RGB_EVEN |
> +			MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB565;
>  		break;
> -	case V4L2_MBUS_FMT_VYUY8_2X8:
> -		mt9m111->swap_yuv_y_chromas = 0;
> -		mt9m111->swap_yuv_cb_cr = 1;
> -		ret = mt9m111_setfmt_yuv(client);
> +	case V4L2_MBUS_FMT_BGR565_2X8_BE:
> +		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr |
> +			MT9M111_OUTFMT_RGB | MT9M111_OUTFMT_RGB565;
>  		break;
> -	case V4L2_MBUS_FMT_YUYV8_2X8:
> -		mt9m111->swap_yuv_y_chromas = 1;
> -		mt9m111->swap_yuv_cb_cr = 0;
> -		ret = mt9m111_setfmt_yuv(client);
> +	case V4L2_MBUS_FMT_YUYV8_2X8_BE:
>  		break;
> -	case V4L2_MBUS_FMT_YVYU8_2X8:
> -		mt9m111->swap_yuv_y_chromas = 1;
> -		mt9m111->swap_yuv_cb_cr = 1;
> -		ret = mt9m111_setfmt_yuv(client);
> +	case V4L2_MBUS_FMT_YVYU8_2X8_BE:
> +		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
> +		break;
> +	case V4L2_MBUS_FMT_YUYV8_2X8_LE:
> +		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_C_Y;
> +		break;
> +	case V4L2_MBUS_FMT_YVYU8_2X8_LE:
> +		data_outfmt2 = MT9M111_OUTFMT_SWAP_YCbCr_C_Y |
> +			MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr;
>  		break;
>  	default:
>  		dev_err(&client->dev, "Pixel format not handled : %x\n",
> @@ -543,6 +513,25 @@ static int mt9m111_set_pixfmt(struct i2c_client *client,
>  		ret = -EINVAL;
>  	}
>  
> +	mask_outfmt1 = MT9M111_OUTFMT_FLIP_BAYER_COL |
> +		MT9M111_OUTFMT_FLIP_BAYER_ROW;
> +
> +	mask_outfmt2 = MT9M111_OUTFMT_PROCESSED_BAYER |
> +		MT9M111_OUTFMT_BYPASS_IFP | MT9M111_OUTFMT_RGB |
> +		MT9M111_OUTFMT_RGB565 | MT9M111_OUTFMT_RGB555 |
> +		MT9M111_OUTFMT_RGB444x | MT9M111_OUTFMT_RGBx444 |
> +		MT9M111_OUTFMT_SWAP_YCbCr_C_Y | MT9M111_OUTFMT_SWAP_RGB_EVEN |
> +		MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr | MT9M111_OUTFMT_SWAP_RGB_R_B;
> +
> +	ret = reg_mask(OUTPUT_FORMAT_CTRL, data_outfmt1, mask_outfmt1);
> +
> +	if (!ret)
> +		ret = reg_mask(OUTPUT_FORMAT_CTRL2_A, data_outfmt2,
> +			mask_outfmt2);
> +	if (!ret)
> +		ret = reg_mask(OUTPUT_FORMAT_CTRL2_B, data_outfmt2,
> +			mask_outfmt2);
> +
>  	return ret;
>  }
>  
> @@ -972,9 +961,6 @@ static int mt9m111_video_probe(struct soc_camera_device *icd,
>  	mt9m111->autoexposure = 1;
>  	mt9m111->autowhitebalance = 1;
>  
> -	mt9m111->swap_rgb_even_odd = 1;
> -	mt9m111->swap_rgb_red_blue = 1;
> -
>  	data = reg_read(CHIP_VERSION);
>  
>  	switch (data) {
> -- 
> 1.7.0
> 
> 

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