[PATCH 1/5] mt9m111: set inital return values to zero

[PATCH 1/5] mt9m111: set inital return values to zero

[Michael Grzeschik]

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

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index ebebed9..f10dcf0 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -190,7 +190,7 @@ static struct mt9m111 *to_mt9m111(const struct i2c_client *client)
 
 static int reg_page_map_set(struct i2c_client *client, const u16 reg)
 {
-	int ret;
+	int ret = 0;
 	u16 page;
 	static int lastpage = -1;	/* PageMap cache value */
 
@@ -208,7 +208,7 @@ static int reg_page_map_set(struct i2c_client *client, const u16 reg)
 
 static int mt9m111_reg_read(struct i2c_client *client, const u16 reg)
 {
-	int ret;
+	int ret = 0;
 
 	ret = reg_page_map_set(client, reg);
 	if (!ret)
@@ -221,7 +221,7 @@ static int mt9m111_reg_read(struct i2c_client *client, const u16 reg)
 static int mt9m111_reg_write(struct i2c_client *client, const u16 reg,
 			     const u16 data)
 {
-	int ret;
+	int ret = 0;
 
 	ret = reg_page_map_set(client, reg);
 	if (!ret)
@@ -234,7 +234,7 @@ static int mt9m111_reg_write(struct i2c_client *client, const u16 reg,
 static int mt9m111_reg_set(struct i2c_client *client, const u16 reg,
 			   const u16 data)
 {
-	int ret;
+	int ret = 0;
 
 	ret = mt9m111_reg_read(client, reg);
 	if (ret >= 0)
@@ -245,7 +245,7 @@ static int mt9m111_reg_set(struct i2c_client *client, const u16 reg,
 static int mt9m111_reg_clear(struct i2c_client *client, const u16 reg,
 			     const u16 data)
 {
-	int ret;
+	int ret = 0;
 
 	ret = mt9m111_reg_read(client, reg);
 	return mt9m111_reg_write(client, reg, ret & ~data);
@@ -387,7 +387,7 @@ static int mt9m111_setfmt_yuv(struct i2c_client *client)
 static int mt9m111_enable(struct i2c_client *client)
 {
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int ret;
+	int ret = 0;
 
 	ret = reg_set(RESET, MT9M111_RESET_CHIP_ENABLE);
 	if (!ret)
@@ -397,7 +397,7 @@ static int mt9m111_enable(struct i2c_client *client)
 
 static int mt9m111_reset(struct i2c_client *client)
 {
-	int ret;
+	int ret = 0;
 
 	ret = reg_set(RESET, MT9M111_RESET_RESET_MODE);
 	if (!ret)
@@ -452,7 +452,7 @@ static int mt9m111_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 	struct v4l2_rect rect = a->c;
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int ret;
+	int ret = 0;
 
 	dev_dbg(&client->dev, "%s left=%d, top=%d, width=%d, height=%d\n",
 		__func__, rect.left, rect.top, rect.width, rect.height);
@@ -568,7 +568,7 @@ static int mt9m111_s_fmt(struct v4l2_subdev *sd,
 		.width	= mf->width,
 		.height	= mf->height,
 	};
-	int ret;
+	int ret = 0;
 
 	fmt = mt9m111_find_datafmt(mf->code, mt9m111_colour_fmts,
 				   ARRAY_SIZE(mt9m111_colour_fmts));
@@ -741,7 +741,7 @@ static struct soc_camera_ops mt9m111_ops = {
 static int mt9m111_set_flip(struct i2c_client *client, int flip, int mask)
 {
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int ret;
+	int ret = 0;
 
 	if (mt9m111->context == HIGHPOWER) {
 		if (flip)
@@ -791,7 +791,7 @@ static int mt9m111_set_global_gain(struct i2c_client *client, int gain)
 static int mt9m111_set_autoexposure(struct i2c_client *client, int on)
 {
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int ret;
+	int ret = 0;
 
 	if (on)
 		ret = reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
@@ -807,7 +807,7 @@ static int mt9m111_set_autoexposure(struct i2c_client *client, int on)
 static int mt9m111_set_autowhitebalance(struct i2c_client *client, int on)
 {
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int ret;
+	int ret = 0;
 
 	if (on)
 		ret = reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
@@ -868,7 +868,7 @@ static int mt9m111_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
 	const struct v4l2_queryctrl *qctrl;
-	int ret;
+	int ret = 0;
 
 	qctrl = soc_camera_find_qctrl(&mt9m111_ops, ctrl->id);
 	if (!qctrl)
@@ -945,7 +945,7 @@ static int mt9m111_resume(struct soc_camera_device *icd)
 static int mt9m111_init(struct i2c_client *client)
 {
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int ret;
+	int ret = 0;
 
 	mt9m111->context = HIGHPOWER;
 	ret = mt9m111_enable(client);
@@ -969,7 +969,7 @@ static int mt9m111_video_probe(struct soc_camera_device *icd,
 {
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
 	s32 data;
-	int ret;
+	int ret = 0;
 
 	/*
 	 * We must have a parent by now. And it cannot be a wrong one.
@@ -1053,7 +1053,7 @@ static int mt9m111_probe(struct i2c_client *client,
 	struct soc_camera_device *icd = client->dev.platform_data;
 	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
 	struct soc_camera_link *icl;
-	int ret;
+	int ret = 0;
 
 	if (!icd) {
 		dev_err(&client->dev, "mt9m111: soc-camera data missing!\n");
-- 
1.7.5.4

[PATCH 2/5] mt9m111: fix missing return value check mt9m111_reg_clear

[Michael Grzeschik]

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

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index f10dcf0..e08b46c 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -248,7 +248,9 @@ static int mt9m111_reg_clear(struct i2c_client *client, const u16 reg,
 	int ret = 0;
 
 	ret = mt9m111_reg_read(client, reg);
-	return mt9m111_reg_write(client, reg, ret & ~data);
+	if (ret >= 0)
+		ret = mt9m111_reg_write(client, reg, ret & ~data);
+	return ret;
 }
 
 static int mt9m111_set_context(struct i2c_client *client,
-- 
1.7.5.4

Re: [PATCH 2/5] mt9m111: fix missing return value check mt9m111_reg_clear

[Guennadi Liakhovetski]

On Tue, 12 Jul 2011, Michael Grzeschik wrote:

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Applied, thanks

> ---
>  drivers/media/video/mt9m111.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
> index f10dcf0..e08b46c 100644
> --- a/drivers/media/video/mt9m111.c
> +++ b/drivers/media/video/mt9m111.c
> @@ -248,7 +248,9 @@ static int mt9m111_reg_clear(struct i2c_client *client, const u16 reg,
>  	int ret = 0;
>  
>  	ret = mt9m111_reg_read(client, reg);
> -	return mt9m111_reg_write(client, reg, ret & ~data);
> +	if (ret >= 0)
> +		ret = mt9m111_reg_write(client, reg, ret & ~data);
> +	return ret;
>  }
>  
>  static int mt9m111_set_context(struct i2c_client *client,
> -- 
> 1.7.5.4
> 

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

[PATCH 3/5] mt9m111: move lastpage to struct mt9m111 for multi instances

[Michael Grzeschik]

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

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index e08b46c..8ad99a9 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -170,6 +170,7 @@ struct mt9m111 {
 	enum mt9m111_context context;
 	struct v4l2_rect rect;
 	const struct mt9m111_datafmt *fmt;
+	int lastpage;
 	unsigned int gain;
 	unsigned char autoexposure;
 	unsigned char datawidth;
@@ -192,17 +193,17 @@ static int reg_page_map_set(struct i2c_client *client, const u16 reg)
 {
 	int ret = 0;
 	u16 page;
-	static int lastpage = -1;	/* PageMap cache value */
+	struct mt9m111 *mt9m111 = to_mt9m111(client);
 
 	page = (reg >> 8);
-	if (page == lastpage)
+	if (page == mt9m111->lastpage)
 		return 0;
 	if (page > 2)
 		return -EINVAL;
 
 	ret = i2c_smbus_write_word_data(client, MT9M111_PAGE_MAP, swab16(page));
 	if (!ret)
-		lastpage = page;
+		mt9m111->lastpage = page;
 	return ret;
 }
 
-- 
1.7.5.4

Re: [PATCH 3/5] mt9m111: move lastpage to struct mt9m111 for multi instances

[Laurent Pinchart]

Hi Michael,

On Tuesday 12 July 2011 17:39:04 Michael Grzeschik wrote:
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/media/video/mt9m111.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
> index e08b46c..8ad99a9 100644
> --- a/drivers/media/video/mt9m111.c
> +++ b/drivers/media/video/mt9m111.c
> @@ -170,6 +170,7 @@ struct mt9m111 {
>  	enum mt9m111_context context;
>  	struct v4l2_rect rect;
>  	const struct mt9m111_datafmt *fmt;
> +	int lastpage;
>  	unsigned int gain;
>  	unsigned char autoexposure;
>  	unsigned char datawidth;
> @@ -192,17 +193,17 @@ static int reg_page_map_set(struct i2c_client
> *client, const u16 reg) {
>  	int ret = 0;
>  	u16 page;
> -	static int lastpage = -1;	/* PageMap cache value */

You're loosing lastpage initialization to -1.

> +	struct mt9m111 *mt9m111 = to_mt9m111(client);
> 
>  	page = (reg >> 8);
> -	if (page == lastpage)
> +	if (page == mt9m111->lastpage)
>  		return 0;
>  	if (page > 2)
>  		return -EINVAL;
> 
>  	ret = i2c_smbus_write_word_data(client, MT9M111_PAGE_MAP, swab16(page));
>  	if (!ret)
> -		lastpage = page;
> +		mt9m111->lastpage = page;
>  	return ret;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/5] mt9m111: move lastpage to struct mt9m111 for multi instances

[Guennadi Liakhovetski]

On Thu, 14 Jul 2011, Laurent Pinchart wrote:

> Hi Michael,
> 
> On Tuesday 12 July 2011 17:39:04 Michael Grzeschik wrote:
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> >  drivers/media/video/mt9m111.c |    7 ++++---
> >  1 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
> > index e08b46c..8ad99a9 100644
> > --- a/drivers/media/video/mt9m111.c
> > +++ b/drivers/media/video/mt9m111.c
> > @@ -170,6 +170,7 @@ struct mt9m111 {
> >  	enum mt9m111_context context;
> >  	struct v4l2_rect rect;
> >  	const struct mt9m111_datafmt *fmt;
> > +	int lastpage;
> >  	unsigned int gain;
> >  	unsigned char autoexposure;
> >  	unsigned char datawidth;
> > @@ -192,17 +193,17 @@ static int reg_page_map_set(struct i2c_client
> > *client, const u16 reg) {
> >  	int ret = 0;
> >  	u16 page;
> > -	static int lastpage = -1;	/* PageMap cache value */
> 
> You're loosing lastpage initialization to -1.

Seconded. A fixed version of this patch will ve welcome for 3.2.

Thanks
Guennadi

> 
> > +	struct mt9m111 *mt9m111 = to_mt9m111(client);
> > 
> >  	page = (reg >> 8);
> > -	if (page == lastpage)
> > +	if (page == mt9m111->lastpage)
> >  		return 0;
> >  	if (page > 2)
> >  		return -EINVAL;
> > 
> >  	ret = i2c_smbus_write_word_data(client, MT9M111_PAGE_MAP, swab16(page));
> >  	if (!ret)
> > -		lastpage = page;
> > +		mt9m111->lastpage = page;
> >  	return ret;
> >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

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

[PATCH v2] mt9m111: move lastpage to struct mt9m111 for multi instances

[Michael Grzeschik]

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
v1 -> v2: added initial value -1 for lastpage

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

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index a357aa8..07af26e 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -184,6 +184,7 @@ struct mt9m111 {
 	struct mutex power_lock; /* lock to protect power_count */
 	int power_count;
 	const struct mt9m111_datafmt *fmt;
+	int lastpage;	/* PageMap cache value */
 	unsigned int gain;
 	unsigned char autoexposure;
 	unsigned char datawidth;
@@ -202,17 +203,17 @@ static int reg_page_map_set(struct i2c_client *client, const u16 reg)
 {
 	int ret;
 	u16 page;
-	static int lastpage = -1;	/* PageMap cache value */
+	struct mt9m111 *mt9m111 = to_mt9m111(client);
 
 	page = (reg >> 8);
-	if (page == lastpage)
+	if (page == mt9m111->lastpage)
 		return 0;
 	if (page > 2)
 		return -EINVAL;
 
 	ret = i2c_smbus_write_word_data(client, MT9M111_PAGE_MAP, swab16(page));
 	if (!ret)
-		lastpage = page;
+		mt9m111->lastpage = page;
 	return ret;
 }
 
@@ -932,6 +933,8 @@ static int mt9m111_video_probe(struct soc_camera_device *icd,
 	BUG_ON(!icd->parent ||
 	       to_soc_camera_host(icd->parent)->nr != icd->iface);
 
+	mt9m111->lastpage = -1;
+
 	mt9m111->autoexposure = 1;
 	mt9m111->autowhitebalance = 1;
 
-- 
1.7.5.4

Re: [PATCH v2] mt9m111: move lastpage to struct mt9m111 for multi instances

[Guennadi Liakhovetski]

Hi Michael

Looks good now, thanks. Unfortunately, we've already missed the 3.1 merge 
window, unless Linus decides to release one more 3.0-rcX kernel. But 
still, I think, this can qualify as a fix, so, it should be ok even after 
-rc1.

Thanks
Guennadi

On Tue, 19 Jul 2011, Michael Grzeschik wrote:

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> v1 -> v2: added initial value -1 for lastpage
> 
>  drivers/media/video/mt9m111.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
> index a357aa8..07af26e 100644
> --- a/drivers/media/video/mt9m111.c
> +++ b/drivers/media/video/mt9m111.c
> @@ -184,6 +184,7 @@ struct mt9m111 {
>  	struct mutex power_lock; /* lock to protect power_count */
>  	int power_count;
>  	const struct mt9m111_datafmt *fmt;
> +	int lastpage;	/* PageMap cache value */
>  	unsigned int gain;
>  	unsigned char autoexposure;
>  	unsigned char datawidth;
> @@ -202,17 +203,17 @@ static int reg_page_map_set(struct i2c_client *client, const u16 reg)
>  {
>  	int ret;
>  	u16 page;
> -	static int lastpage = -1;	/* PageMap cache value */
> +	struct mt9m111 *mt9m111 = to_mt9m111(client);
>  
>  	page = (reg >> 8);
> -	if (page == lastpage)
> +	if (page == mt9m111->lastpage)
>  		return 0;
>  	if (page > 2)
>  		return -EINVAL;
>  
>  	ret = i2c_smbus_write_word_data(client, MT9M111_PAGE_MAP, swab16(page));
>  	if (!ret)
> -		lastpage = page;
> +		mt9m111->lastpage = page;
>  	return ret;
>  }
>  
> @@ -932,6 +933,8 @@ static int mt9m111_video_probe(struct soc_camera_device *icd,
>  	BUG_ON(!icd->parent ||
>  	       to_soc_camera_host(icd->parent)->nr != icd->iface);
>  
> +	mt9m111->lastpage = -1;
> +
>  	mt9m111->autoexposure = 1;
>  	mt9m111->autowhitebalance = 1;
>  
> -- 
> 1.7.5.4
> 

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

[PATCH v4 4/5] 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"
Changes v3 -> v4
	* removed wrong formats V4L2_MBUS_FMT_{YVYU8,YUYV8}_2X8_BE
	* fixed some return value handling

 drivers/media/video/mt9m111.c |  175 ++++++++++++++++++-----------------------
 1 files changed, 77 insertions(+), 98 deletions(-)

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index 8ad99a9..7eb2e4a 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
@@ -153,7 +163,11 @@ static const struct mt9m111_datafmt mt9m111_colour_fmts[] = {
 	{V4L2_MBUS_FMT_UYVY8_2X8, V4L2_COLORSPACE_JPEG},
 	{V4L2_MBUS_FMT_VYUY8_2X8, 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},
 };
@@ -177,10 +191,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;
 };
 
@@ -254,6 +264,17 @@ static int mt9m111_reg_clear(struct i2c_client *client, const u16 reg,
 	return ret;
 }
 
+static int mt9m111_reg_mask(struct i2c_client *client, const u16 reg,
+			    const u16 data, const u16 mask)
+{
+	int ret = 0;
+
+	ret = mt9m111_reg_read(client, reg);
+	if (ret >= 0)
+		ret = mt9m111_reg_write(client, reg, (ret & ~mask) | data);
+	return ret;
+}
+
 static int mt9m111_set_context(struct i2c_client *client,
 			       enum mt9m111_context ctxt)
 {
@@ -315,78 +336,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;
-	u16 mask = MT9M111_OUTFMT_PROCESSED_BAYER | MT9M111_OUTFMT_RGB |
-		MT9M111_OUTFMT_BYPASS_IFP | MT9M111_OUTFMT_SWAP_RGB_EVEN |
-		MT9M111_OUTFMT_RGB565 | MT9M111_OUTFMT_RGB555 |
-		MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr |
-		MT9M111_OUTFMT_SWAP_YCbCr_C_Y;
-
-	ret = reg_read(OUTPUT_FORMAT_CTRL2_A);
-	if (ret >= 0)
-		ret = reg_write(OUTPUT_FORMAT_CTRL2_A, (ret & ~mask) | outfmt);
-	if (!ret)
-		ret = reg_read(OUTPUT_FORMAT_CTRL2_B);
-	if (ret >= 0)
-		ret = reg_write(OUTPUT_FORMAT_CTRL2_B, (ret & ~mask) | 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);
@@ -514,41 +463,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);
-	int ret;
+	u16 data_outfmt1 = 0, data_outfmt2 = 0, mask_outfmt1, mask_outfmt2;
+	int ret = 0;
 
 	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_UYVY8_2X8:
-		mt9m111->swap_yuv_y_chromas = 0;
-		mt9m111->swap_yuv_cb_cr = 0;
-		ret = mt9m111_setfmt_yuv(client);
 		break;
 	case V4L2_MBUS_FMT_VYUY8_2X8:
-		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:
-		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:
-		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",
@@ -556,6 +518,26 @@ 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;
+
+	if (!ret)
+		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;
 }
 
@@ -985,9 +967,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.5.4

Re: [PATCH v4 4/5] mt9m111: rewrite set_pixfmt

[Guennadi Liakhovetski]

On Tue, 12 Jul 2011, 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>

Applied after pretty heavy modifications. (1) forward-ported to the 
current tree, (2) removed Bayer swapping, as discussed earlier, (3) 
removed double bitfield definitions. Please, check out

http://git.linuxtv.org/gliakhovetski/v4l-dvb.git?a=shortlog;h=refs/heads/for-3.1

and see, if I haven't inadvertently broken anything.

Thanks
Guennadi

> ---
> 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"
> Changes v3 -> v4
> 	* removed wrong formats V4L2_MBUS_FMT_{YVYU8,YUYV8}_2X8_BE
> 	* fixed some return value handling
> 
>  drivers/media/video/mt9m111.c |  175 ++++++++++++++++++-----------------------
>  1 files changed, 77 insertions(+), 98 deletions(-)
> 
> diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
> index 8ad99a9..7eb2e4a 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
> @@ -153,7 +163,11 @@ static const struct mt9m111_datafmt mt9m111_colour_fmts[] = {
>  	{V4L2_MBUS_FMT_UYVY8_2X8, V4L2_COLORSPACE_JPEG},
>  	{V4L2_MBUS_FMT_VYUY8_2X8, 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},
>  };
> @@ -177,10 +191,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;
>  };
>  
> @@ -254,6 +264,17 @@ static int mt9m111_reg_clear(struct i2c_client *client, const u16 reg,
>  	return ret;
>  }
>  
> +static int mt9m111_reg_mask(struct i2c_client *client, const u16 reg,
> +			    const u16 data, const u16 mask)
> +{
> +	int ret = 0;
> +
> +	ret = mt9m111_reg_read(client, reg);
> +	if (ret >= 0)
> +		ret = mt9m111_reg_write(client, reg, (ret & ~mask) | data);
> +	return ret;
> +}
> +
>  static int mt9m111_set_context(struct i2c_client *client,
>  			       enum mt9m111_context ctxt)
>  {
> @@ -315,78 +336,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;
> -	u16 mask = MT9M111_OUTFMT_PROCESSED_BAYER | MT9M111_OUTFMT_RGB |
> -		MT9M111_OUTFMT_BYPASS_IFP | MT9M111_OUTFMT_SWAP_RGB_EVEN |
> -		MT9M111_OUTFMT_RGB565 | MT9M111_OUTFMT_RGB555 |
> -		MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr |
> -		MT9M111_OUTFMT_SWAP_YCbCr_C_Y;
> -
> -	ret = reg_read(OUTPUT_FORMAT_CTRL2_A);
> -	if (ret >= 0)
> -		ret = reg_write(OUTPUT_FORMAT_CTRL2_A, (ret & ~mask) | outfmt);
> -	if (!ret)
> -		ret = reg_read(OUTPUT_FORMAT_CTRL2_B);
> -	if (ret >= 0)
> -		ret = reg_write(OUTPUT_FORMAT_CTRL2_B, (ret & ~mask) | 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);
> @@ -514,41 +463,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);
> -	int ret;
> +	u16 data_outfmt1 = 0, data_outfmt2 = 0, mask_outfmt1, mask_outfmt2;
> +	int ret = 0;
>  
>  	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_UYVY8_2X8:
> -		mt9m111->swap_yuv_y_chromas = 0;
> -		mt9m111->swap_yuv_cb_cr = 0;
> -		ret = mt9m111_setfmt_yuv(client);
>  		break;
>  	case V4L2_MBUS_FMT_VYUY8_2X8:
> -		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:
> -		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:
> -		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",
> @@ -556,6 +518,26 @@ 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;
> +
> +	if (!ret)
> +		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;
>  }
>  
> @@ -985,9 +967,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.5.4
> 

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

Re: [PATCH v4 4/5] mt9m111: rewrite set_pixfmt

[Michael Grzeschik]

Hi Guennadi,

On Sun, Jul 17, 2011 at 07:09:42PM +0200, Guennadi Liakhovetski wrote:
> On Tue, 12 Jul 2011, 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>
> 
> Applied after pretty heavy modifications. (1) forward-ported to the 
> current tree, (2) removed Bayer swapping, as discussed earlier, (3) 
> removed double bitfield definitions. Please, check out
> 
> http://git.linuxtv.org/gliakhovetski/v4l-dvb.git?a=shortlog;h=refs/heads/for-3.1
> 
> and see, if I haven't inadvertently broken anything.

I double checked all modifications and also tested your patch with some
formats. I also prefer the idea to handle the configuration of
data_outfmt1 in a separete patch. So everything looks right to me.

Thanks,
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 5/5] 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 7eb2e4a..a3463d9 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;
@@ -464,6 +479,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 = 0;
 
 	switch (code) {
@@ -531,6 +547,47 @@ static int mt9m111_set_pixfmt(struct i2c_client *client,
 	if (!ret)
 		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.5.4

Re: [PATCH 5/5] mt9m111: make use of testpattern

[Guennadi Liakhovetski]

On Tue, 12 Jul 2011, Michael Grzeschik wrote:

> 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 7eb2e4a..a3463d9 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");
> +

I already replied, that I do not like using a module parameter for this.

Thanks
Guennadi

>  /* MT9M111 has only one fixed colorspace per pixelcode */
>  struct mt9m111_datafmt {
>  	enum v4l2_mbus_pixelcode	code;
> @@ -464,6 +479,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 = 0;
>  
>  	switch (code) {
> @@ -531,6 +547,47 @@ static int mt9m111_set_pixfmt(struct i2c_client *client,
>  	if (!ret)
>  		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.5.4
> 

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

Re: [PATCH 1/5] mt9m111: set inital return values to zero

[Laurent Pinchart]

Hi Michael,

There's no need to set initial return values to zero if they're assigned to in 
all code paths.

[snip]

> *client) static int mt9m111_enable(struct i2c_client *client)
>  {
>  	struct mt9m111 *mt9m111 = to_mt9m111(client);
> -	int ret;
> +	int ret = 0;
> 
>  	ret = reg_set(RESET, MT9M111_RESET_CHIP_ENABLE);
>  	if (!ret)

This is a clear example, ret will never be used uninitialized. Initializing it 
to 0 would be a waste of resources (although in this case it will probably be 
optimized out by the compiler).

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/5] mt9m111: set inital return values to zero

[Guennadi Liakhovetski]

On Thu, 14 Jul 2011, Laurent Pinchart wrote:

> Hi Michael,
> 
> There's no need to set initial return values to zero if they're assigned to in 
> all code paths.
> 
> [snip]
> 
> > *client) static int mt9m111_enable(struct i2c_client *client)
> >  {
> >  	struct mt9m111 *mt9m111 = to_mt9m111(client);
> > -	int ret;
> > +	int ret = 0;
> > 
> >  	ret = reg_set(RESET, MT9M111_RESET_CHIP_ENABLE);
> >  	if (!ret)
> 
> This is a clear example, ret will never be used uninitialized. Initializing it 
> to 0 would be a waste of resources (although in this case it will probably be 
> optimized out by the compiler).

Seconded. When I wrote:

> > +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...

I didn't mean to init all return codes to 0. I meant, before using a 
result of a reg_read(), you have to check it for error. I.e.,

+	ret = mt9m111_reg_read(client, reg);
+	if (ret >= 0)
+		ret = mt9m111_reg_write(client, reg, (ret & ~mask) | data);
+	return ret;

In principle, after the updated version of your patch "mt9m111: rewrite 
set_pixfmt" all errors, returned by reg_read(), reg_write() and reg_mask() 
are checked, even if some of them I would do a bit differently. E.g., I 
would propagate the error code instead of replacing it with -EIO, etc. But 
in principle all error cases are handled, so, we can live with that for 
now. I'm dropping this patch.

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