[PATCH 0/3] various updates for mt9v022 driver

[PATCH 0/3] various updates for mt9v022 driver

[Anatolij Gustschin]

Anatolij Gustschin (3):
  mt9v022: add v4l2 controls for blanking and other register settings
  mt9v022: fix the V4L2_CID_EXPOSURE control
  mt9v022: set y_skip_top field to zero

 drivers/media/i2c/soc_camera/mt9v022.c |  117 ++++++++++++++++++++++++++++----
 1 files changed, 104 insertions(+), 13 deletions(-)

[PATCH 1/3] mt9v022: add v4l2 controls for blanking and other register settings

[Anatolij Gustschin]

Add controls for horizontal and vertical blanking, analog control
and control for undocumented register 32. Also add an error message
for case that the control handler init failed. Since setting the
blanking registers is done by controls now, we should't change these
registers outside of the control function. Use v4l2_ctrl_s_ctrl()
to set them.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
 drivers/media/i2c/soc_camera/mt9v022.c |  105 ++++++++++++++++++++++++++++++--
 1 files changed, 100 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
index 350d0d8..d13c8c4 100644
--- a/drivers/media/i2c/soc_camera/mt9v022.c
+++ b/drivers/media/i2c/soc_camera/mt9v022.c
@@ -50,12 +50,14 @@ MODULE_PARM_DESC(sensor_type, "Sensor type: \"colour\" or \"monochrome\"");
 #define MT9V022_PIXEL_OPERATION_MODE	0x0f
 #define MT9V022_LED_OUT_CONTROL		0x1b
 #define MT9V022_ADC_MODE_CONTROL	0x1c
+#define MT9V022_REG32			0x20
 #define MT9V022_ANALOG_GAIN		0x35
 #define MT9V022_BLACK_LEVEL_CALIB_CTRL	0x47
 #define MT9V022_PIXCLK_FV_LV		0x74
 #define MT9V022_DIGITAL_TEST_PATTERN	0x7f
 #define MT9V022_AEC_AGC_ENABLE		0xAF
 #define MT9V022_MAX_TOTAL_SHUTTER_WIDTH	0xBD
+#define MT9V022_ANALOG_CONTROL		0xC2
 
 /* mt9v024 partial list register addresses changes with respect to mt9v022 */
 #define MT9V024_PIXCLK_FV_LV		0x72
@@ -71,6 +73,13 @@ MODULE_PARM_DESC(sensor_type, "Sensor type: \"colour\" or \"monochrome\"");
 #define MT9V022_COLUMN_SKIP		1
 #define MT9V022_ROW_SKIP		4
 
+#define MT9V022_HORIZONTAL_BLANKING_MIN	43
+#define MT9V022_HORIZONTAL_BLANKING_MAX	1023
+#define MT9V022_HORIZONTAL_BLANKING_DEF	94
+#define MT9V022_VERTICAL_BLANKING_MIN	2
+#define MT9V022_VERTICAL_BLANKING_MAX	3000
+#define MT9V022_VERTICAL_BLANKING_DEF	45
+
 #define is_mt9v024(id) (id == 0x1324)
 
 /* MT9V022 has only one fixed colorspace per pixelcode */
@@ -136,6 +145,8 @@ struct mt9v022 {
 		struct v4l2_ctrl *autogain;
 		struct v4l2_ctrl *gain;
 	};
+	struct v4l2_ctrl *hblank;
+	struct v4l2_ctrl *vblank;
 	struct v4l2_rect rect;	/* Sensor window */
 	const struct mt9v022_datafmt *fmt;
 	const struct mt9v022_datafmt *fmts;
@@ -277,11 +288,10 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 		 * Default 94, Phytec driver says:
 		 * "width + horizontal blank >= 660"
 		 */
-		ret = reg_write(client, MT9V022_HORIZONTAL_BLANKING,
-				rect.width > 660 - 43 ? 43 :
-				660 - rect.width);
+		ret = v4l2_ctrl_s_ctrl(mt9v022->hblank,
+				rect.width > 660 - 43 ? 43 : 660 - rect.width);
 	if (!ret)
-		ret = reg_write(client, MT9V022_VERTICAL_BLANKING, 45);
+		ret = v4l2_ctrl_s_ctrl(mt9v022->vblank, 45);
 	if (!ret)
 		ret = reg_write(client, MT9V022_WINDOW_WIDTH, rect.width);
 	if (!ret)
@@ -476,6 +486,9 @@ static int mt9v022_s_power(struct v4l2_subdev *sd, int on)
 	return soc_camera_set_power(&client->dev, icl, on);
 }
 
+#define V4L2_CID_REG32			(V4L2_CTRL_CLASS_CAMERA | 0x1001)
+#define V4L2_CID_ANALOG_CONTROLS	(V4L2_CTRL_CLASS_CAMERA | 0x1002)
+
 static int mt9v022_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct mt9v022 *mt9v022 = container_of(ctrl->handler,
@@ -504,6 +517,30 @@ static int mt9v022_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 		range = exp->maximum - exp->minimum;
 		exp->val = ((data - 1) * range + 239) / 479 + exp->minimum;
 		return 0;
+	case V4L2_CID_HBLANK:
+		data = reg_read(client, MT9V022_HORIZONTAL_BLANKING);
+		if (data < 0)
+			return -EIO;
+		ctrl->val = data;
+		return 0;
+	case V4L2_CID_VBLANK:
+		data = reg_read(client, MT9V022_VERTICAL_BLANKING);
+		if (data < 0)
+			return -EIO;
+		ctrl->val = data;
+		return 0;
+	case V4L2_CID_REG32:
+		data = reg_read(client, MT9V022_REG32);
+		if (data < 0)
+			return -EIO;
+		ctrl->val = data;
+		return 0;
+	case V4L2_CID_ANALOG_CONTROLS:
+		data = reg_read(client, MT9V022_ANALOG_CONTROL);
+		if (data < 0)
+			return -EIO;
+		ctrl->val = data;
+		return 0;
 	}
 	return -EINVAL;
 }
@@ -585,6 +622,26 @@ static int mt9v022_s_ctrl(struct v4l2_ctrl *ctrl)
 				return -EIO;
 		}
 		return 0;
+	case V4L2_CID_HBLANK:
+		if (reg_write(client, MT9V022_HORIZONTAL_BLANKING,
+				ctrl->val) < 0)
+			return -EIO;
+		return 0;
+	case V4L2_CID_VBLANK:
+		if (reg_write(client, MT9V022_VERTICAL_BLANKING,
+				ctrl->val) < 0)
+			return -EIO;
+		return 0;
+	case V4L2_CID_REG32:
+		if (reg_write(client, MT9V022_REG32,
+				ctrl->val) < 0)
+			return -EIO;
+		return 0;
+	case V4L2_CID_ANALOG_CONTROLS:
+		if (reg_write(client, MT9V022_ANALOG_CONTROL,
+				ctrl->val) < 0)
+			return -EIO;
+		return 0;
 	}
 	return -EINVAL;
 }
@@ -697,6 +754,30 @@ static const struct v4l2_ctrl_ops mt9v022_ctrl_ops = {
 	.s_ctrl = mt9v022_s_ctrl,
 };
 
+static const struct v4l2_ctrl_config mt9v022_ctrls[] = {
+	{
+		.ops		= &mt9v022_ctrl_ops,
+		.id		= V4L2_CID_REG32,
+		.type		= V4L2_CTRL_TYPE_INTEGER,
+		.name		= "reg32 (0x20)",
+		.min		= 0,
+		.max		= 0x0fff,
+		.step		= 1,
+		.def		= 0x01d1,
+		.flags		= V4L2_CTRL_FLAG_VOLATILE,
+	}, {
+		.ops		= &mt9v022_ctrl_ops,
+		.id		= V4L2_CID_ANALOG_CONTROLS,
+		.type		= V4L2_CTRL_TYPE_INTEGER,
+		.name		= "analog controls",
+		.min		= 0,
+		.max		= 0xffff,
+		.step		= 1,
+		.def		= 0x1840,
+		.flags		= V4L2_CTRL_FLAG_VOLATILE,
+	},
+};
+
 static struct v4l2_subdev_core_ops mt9v022_subdev_core_ops = {
 	.g_chip_ident	= mt9v022_g_chip_ident,
 #ifdef CONFIG_VIDEO_ADV_DEBUG
@@ -832,7 +913,7 @@ static int mt9v022_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	v4l2_i2c_subdev_init(&mt9v022->subdev, client, &mt9v022_subdev_ops);
-	v4l2_ctrl_handler_init(&mt9v022->hdl, 6);
+	v4l2_ctrl_handler_init(&mt9v022->hdl, 6 + ARRAY_SIZE(mt9v022_ctrls));
 	v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
 			V4L2_CID_VFLIP, 0, 1, 1, 0);
 	v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
@@ -852,10 +933,24 @@ static int mt9v022_probe(struct i2c_client *client,
 	mt9v022->exposure = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
 			V4L2_CID_EXPOSURE, 1, 255, 1, 255);
 
+	mt9v022->hblank = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
+			V4L2_CID_HBLANK, MT9V022_HORIZONTAL_BLANKING_MIN,
+			MT9V022_HORIZONTAL_BLANKING_MAX, 1,
+			MT9V022_HORIZONTAL_BLANKING_DEF);
+
+	mt9v022->vblank = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
+			V4L2_CID_VBLANK, MT9V022_VERTICAL_BLANKING_MIN,
+			MT9V022_VERTICAL_BLANKING_MAX, 1,
+			MT9V022_VERTICAL_BLANKING_DEF);
+
+	v4l2_ctrl_new_custom(&mt9v022->hdl, &mt9v022_ctrls[0], NULL);
+	v4l2_ctrl_new_custom(&mt9v022->hdl, &mt9v022_ctrls[1], NULL);
+
 	mt9v022->subdev.ctrl_handler = &mt9v022->hdl;
 	if (mt9v022->hdl.error) {
 		int err = mt9v022->hdl.error;
 
+		dev_err(&client->dev, "hdl init err %d\n", err);
 		kfree(mt9v022);
 		return err;
 	}
-- 
1.7.1

Re: [PATCH 1/3] mt9v022: add v4l2 controls for blanking and other register settings

[Guennadi Liakhovetski]

Hi Anatolij

On Fri, 24 Aug 2012, Anatolij Gustschin wrote:

> Add controls for horizontal and vertical blanking, analog control
> and control for undocumented register 32.

Sorry, I don't think this is a good idea to export an undocumented 
register as a control. At most I would add a platform parameter for it, if 
really needed. Or do you have to switch it at run-time? If so, you would 
have some idea - at what time to switch it to what value and what effect 
that produces. Then you could define a _logical_ control. If you 
absolutely need to write random values to various registers, you can use 
VIDIOC_DBG_S_REGISTER and VIDIOC_DBG_G_REGISTER.

> Also add an error message
> for case that the control handler init failed. Since setting the
> blanking registers is done by controls now, we should't change these
> registers outside of the control function. Use v4l2_ctrl_s_ctrl()
> to set them.
> 
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> ---
>  drivers/media/i2c/soc_camera/mt9v022.c |  105 ++++++++++++++++++++++++++++++--
>  1 files changed, 100 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
> index 350d0d8..d13c8c4 100644
> --- a/drivers/media/i2c/soc_camera/mt9v022.c
> +++ b/drivers/media/i2c/soc_camera/mt9v022.c
> @@ -50,12 +50,14 @@ MODULE_PARM_DESC(sensor_type, "Sensor type: \"colour\" or \"monochrome\"");
>  #define MT9V022_PIXEL_OPERATION_MODE	0x0f
>  #define MT9V022_LED_OUT_CONTROL		0x1b
>  #define MT9V022_ADC_MODE_CONTROL	0x1c
> +#define MT9V022_REG32			0x20
>  #define MT9V022_ANALOG_GAIN		0x35
>  #define MT9V022_BLACK_LEVEL_CALIB_CTRL	0x47
>  #define MT9V022_PIXCLK_FV_LV		0x74
>  #define MT9V022_DIGITAL_TEST_PATTERN	0x7f
>  #define MT9V022_AEC_AGC_ENABLE		0xAF
>  #define MT9V022_MAX_TOTAL_SHUTTER_WIDTH	0xBD
> +#define MT9V022_ANALOG_CONTROL		0xC2
>  
>  /* mt9v024 partial list register addresses changes with respect to mt9v022 */
>  #define MT9V024_PIXCLK_FV_LV		0x72
> @@ -71,6 +73,13 @@ MODULE_PARM_DESC(sensor_type, "Sensor type: \"colour\" or \"monochrome\"");
>  #define MT9V022_COLUMN_SKIP		1
>  #define MT9V022_ROW_SKIP		4
>  
> +#define MT9V022_HORIZONTAL_BLANKING_MIN	43
> +#define MT9V022_HORIZONTAL_BLANKING_MAX	1023
> +#define MT9V022_HORIZONTAL_BLANKING_DEF	94
> +#define MT9V022_VERTICAL_BLANKING_MIN	2

Interesting, in my datasheet min is 4. Maybe 4 would be a safer bet then.

> +#define MT9V022_VERTICAL_BLANKING_MAX	3000
> +#define MT9V022_VERTICAL_BLANKING_DEF	45
> +
>  #define is_mt9v024(id) (id == 0x1324)
>  
>  /* MT9V022 has only one fixed colorspace per pixelcode */
> @@ -136,6 +145,8 @@ struct mt9v022 {
>  		struct v4l2_ctrl *autogain;
>  		struct v4l2_ctrl *gain;
>  	};
> +	struct v4l2_ctrl *hblank;
> +	struct v4l2_ctrl *vblank;
>  	struct v4l2_rect rect;	/* Sensor window */
>  	const struct mt9v022_datafmt *fmt;
>  	const struct mt9v022_datafmt *fmts;
> @@ -277,11 +288,10 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
>  		 * Default 94, Phytec driver says:
>  		 * "width + horizontal blank >= 660"
>  		 */
> -		ret = reg_write(client, MT9V022_HORIZONTAL_BLANKING,
> -				rect.width > 660 - 43 ? 43 :
> -				660 - rect.width);
> +		ret = v4l2_ctrl_s_ctrl(mt9v022->hblank,
> +				rect.width > 660 - 43 ? 43 : 660 - rect.width);
>  	if (!ret)
> -		ret = reg_write(client, MT9V022_VERTICAL_BLANKING, 45);
> +		ret = v4l2_ctrl_s_ctrl(mt9v022->vblank, 45);
>  	if (!ret)
>  		ret = reg_write(client, MT9V022_WINDOW_WIDTH, rect.width);
>  	if (!ret)
> @@ -476,6 +486,9 @@ static int mt9v022_s_power(struct v4l2_subdev *sd, int on)
>  	return soc_camera_set_power(&client->dev, icl, on);
>  }
>  
> +#define V4L2_CID_REG32			(V4L2_CTRL_CLASS_CAMERA | 0x1001)
> +#define V4L2_CID_ANALOG_CONTROLS	(V4L2_CTRL_CLASS_CAMERA | 0x1002)

Sorry, no again. The MT9V022_ANALOG_CONTROL register contains two fields: 
anti-eclipse and "anti-eclipse reference voltage control," don't think 
they should be set as a single control value. IIUC, controls are supposed 
to control logical parameters of the system. In this case you could 
introduce an "anti-eclipse reference voltage" control with values in the 
range between 0 and 2250mV, setting it to anything below 1900mV would turn 
the enable bit off. Would such a control make sense? Then you might want 
to ask on the ML, whether this control would make sense as a generic one, 
not mt9v022 specific.

> +
>  static int mt9v022_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct mt9v022 *mt9v022 = container_of(ctrl->handler,
> @@ -504,6 +517,30 @@ static int mt9v022_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
>  		range = exp->maximum - exp->minimum;
>  		exp->val = ((data - 1) * range + 239) / 479 + exp->minimum;
>  		return 0;
> +	case V4L2_CID_HBLANK:
> +		data = reg_read(client, MT9V022_HORIZONTAL_BLANKING);
> +		if (data < 0)
> +			return -EIO;
> +		ctrl->val = data;
> +		return 0;
> +	case V4L2_CID_VBLANK:
> +		data = reg_read(client, MT9V022_VERTICAL_BLANKING);
> +		if (data < 0)
> +			return -EIO;
> +		ctrl->val = data;
> +		return 0;
> +	case V4L2_CID_REG32:
> +		data = reg_read(client, MT9V022_REG32);
> +		if (data < 0)
> +			return -EIO;
> +		ctrl->val = data;
> +		return 0;
> +	case V4L2_CID_ANALOG_CONTROLS:
> +		data = reg_read(client, MT9V022_ANALOG_CONTROL);
> +		if (data < 0)
> +			return -EIO;
> +		ctrl->val = data;
> +		return 0;
>  	}
>  	return -EINVAL;
>  }
> @@ -585,6 +622,26 @@ static int mt9v022_s_ctrl(struct v4l2_ctrl *ctrl)
>  				return -EIO;
>  		}
>  		return 0;
> +	case V4L2_CID_HBLANK:
> +		if (reg_write(client, MT9V022_HORIZONTAL_BLANKING,
> +				ctrl->val) < 0)
> +			return -EIO;
> +		return 0;
> +	case V4L2_CID_VBLANK:
> +		if (reg_write(client, MT9V022_VERTICAL_BLANKING,
> +				ctrl->val) < 0)
> +			return -EIO;
> +		return 0;
> +	case V4L2_CID_REG32:
> +		if (reg_write(client, MT9V022_REG32,
> +				ctrl->val) < 0)
> +			return -EIO;
> +		return 0;
> +	case V4L2_CID_ANALOG_CONTROLS:
> +		if (reg_write(client, MT9V022_ANALOG_CONTROL,
> +				ctrl->val) < 0)
> +			return -EIO;
> +		return 0;
>  	}
>  	return -EINVAL;
>  }
> @@ -697,6 +754,30 @@ static const struct v4l2_ctrl_ops mt9v022_ctrl_ops = {
>  	.s_ctrl = mt9v022_s_ctrl,
>  };
>  
> +static const struct v4l2_ctrl_config mt9v022_ctrls[] = {
> +	{
> +		.ops		= &mt9v022_ctrl_ops,
> +		.id		= V4L2_CID_REG32,
> +		.type		= V4L2_CTRL_TYPE_INTEGER,
> +		.name		= "reg32 (0x20)",
> +		.min		= 0,
> +		.max		= 0x0fff,
> +		.step		= 1,
> +		.def		= 0x01d1,
> +		.flags		= V4L2_CTRL_FLAG_VOLATILE,
> +	}, {
> +		.ops		= &mt9v022_ctrl_ops,
> +		.id		= V4L2_CID_ANALOG_CONTROLS,
> +		.type		= V4L2_CTRL_TYPE_INTEGER,
> +		.name		= "analog controls",
> +		.min		= 0,
> +		.max		= 0xffff,
> +		.step		= 1,
> +		.def		= 0x1840,
> +		.flags		= V4L2_CTRL_FLAG_VOLATILE,
> +	},
> +};
> +
>  static struct v4l2_subdev_core_ops mt9v022_subdev_core_ops = {
>  	.g_chip_ident	= mt9v022_g_chip_ident,
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
> @@ -832,7 +913,7 @@ static int mt9v022_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  
>  	v4l2_i2c_subdev_init(&mt9v022->subdev, client, &mt9v022_subdev_ops);
> -	v4l2_ctrl_handler_init(&mt9v022->hdl, 6);
> +	v4l2_ctrl_handler_init(&mt9v022->hdl, 6 + ARRAY_SIZE(mt9v022_ctrls));
>  	v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
>  			V4L2_CID_VFLIP, 0, 1, 1, 0);
>  	v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
> @@ -852,10 +933,24 @@ static int mt9v022_probe(struct i2c_client *client,
>  	mt9v022->exposure = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
>  			V4L2_CID_EXPOSURE, 1, 255, 1, 255);
>  
> +	mt9v022->hblank = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
> +			V4L2_CID_HBLANK, MT9V022_HORIZONTAL_BLANKING_MIN,
> +			MT9V022_HORIZONTAL_BLANKING_MAX, 1,
> +			MT9V022_HORIZONTAL_BLANKING_DEF);
> +
> +	mt9v022->vblank = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
> +			V4L2_CID_VBLANK, MT9V022_VERTICAL_BLANKING_MIN,
> +			MT9V022_VERTICAL_BLANKING_MAX, 1,
> +			MT9V022_VERTICAL_BLANKING_DEF);
> +
> +	v4l2_ctrl_new_custom(&mt9v022->hdl, &mt9v022_ctrls[0], NULL);
> +	v4l2_ctrl_new_custom(&mt9v022->hdl, &mt9v022_ctrls[1], NULL);
> +
>  	mt9v022->subdev.ctrl_handler = &mt9v022->hdl;
>  	if (mt9v022->hdl.error) {
>  		int err = mt9v022->hdl.error;
>  
> +		dev_err(&client->dev, "hdl init err %d\n", err);

That's not very clear IMHO. "hdl" isn't too specific, just "control 
initialisation?"

>  		kfree(mt9v022);
>  		return err;
>  	}
> -- 
> 1.7.1
> 

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

Re: [PATCH 1/3] mt9v022: add v4l2 controls for blanking and other register settings

[Detlev Zundel]

Hello Guennadi,

> Hi Anatolij
>
> On Fri, 24 Aug 2012, Anatolij Gustschin wrote:
>
>> Add controls for horizontal and vertical blanking, analog control
>> and control for undocumented register 32.
>
> Sorry, I don't think this is a good idea to export an undocumented 
> register as a control.

Why exactly is that?  Even though it is not documented, we need to
fiddle with it to make our application work at all.  So we tend to
believe that other users of the chip will want to use it also.

Furthermore I don't see that we fundamentally reject patches for other
parts in the Linux kernel where people found out things not in the
official datasheets.

But I agree that better documentation would be valuable - we are trying
to find out more information on the original 

> At most I would add a platform parameter for it, if really needed. Or
> do you have to switch it at run-time? If so, you would have some idea
> - at what time to switch it to what value and what effect that
> produces. Then you could define a _logical_ control. If you absolutely
> need to write random values to various registers, you can use
> VIDIOC_DBG_S_REGISTER and VIDIOC_DBG_G_REGISTER.

I made a mistake of using them once in an application before I noticed
the comments immediately above in the header file[1]:

  /*
   *      A D V A N C E D   D E B U G G I N G
   *
   *      NOTE: EXPERIMENTAL API, NEVER RELY ON THIS IN APPLICATIONS!
   *      FOR DEBUGGING, TESTING AND INTERNAL USE ONLY!
   */
  
  /* VIDIOC_DBG_G_REGISTER and VIDIOC_DBG_S_REGISTER */
  
At that time I was burned by the API changing (the undocumented register
does not go away) and thus this time we want to follow the advice in the
header file.  So I really hope that you do not suggest that we use this
in an application or do you?

Best wishes
  Detlev

[1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=include/linux/videodev2.h

-- 
I'm sorry that I long ago coined the term "objects" for this topic
because it gets many people to focus on the lesser idea.
                      -- Alan Kay on "Object" Oriented Programming

Re: [PATCH 1/3] mt9v022: add v4l2 controls for blanking and other register settings

[Guennadi Liakhovetski]

Hi Detlev

On Fri, 24 Aug 2012, Detlev Zundel wrote:

> Hello Guennadi,
> 
> > Hi Anatolij
> >
> > On Fri, 24 Aug 2012, Anatolij Gustschin wrote:
> >
> >> Add controls for horizontal and vertical blanking, analog control
> >> and control for undocumented register 32.
> >
> > Sorry, I don't think this is a good idea to export an undocumented 
> > register as a control.
> 
> Why exactly is that?  Even though it is not documented, we need to
> fiddle with it to make our application work at all.  So we tend to
> believe that other users of the chip will want to use it also.

Below I asked to provide details about how you have to change this 
register value: toggle dynamically at run-time or just set once at 
initialisation? Even if toggle: are this certain moments, related to 
standard camera activities (e.g., starting and stopping streaming, 
changing geometry etc.) or you have to set this absolutely asynchronously 
at moments of time, that only your application knows about?

> Furthermore I don't see that we fundamentally reject patches for other
> parts in the Linux kernel where people found out things not in the
> official datasheets.

The problem is not, that this register is undocumented, the problem rather 
is, that IMHO exporting an API to user-space, setting an undocumented 
register to arbitrary values is, hm, at least pretty dubious.

> But I agree that better documentation would be valuable - we are trying
> to find out more information on the original 

That would be great, thanks!

> > At most I would add a platform parameter for it, if really needed. Or
> > do you have to switch it at run-time? If so, you would have some idea
> > - at what time to switch it to what value and what effect that
> > produces. Then you could define a _logical_ control. If you absolutely
> > need to write random values to various registers, you can use
> > VIDIOC_DBG_S_REGISTER and VIDIOC_DBG_G_REGISTER.
> 
> I made a mistake of using them once in an application before I noticed
> the comments immediately above in the header file[1]:
> 
>   /*
>    *      A D V A N C E D   D E B U G G I N G
>    *
>    *      NOTE: EXPERIMENTAL API, NEVER RELY ON THIS IN APPLICATIONS!
>    *      FOR DEBUGGING, TESTING AND INTERNAL USE ONLY!
>    */
>   
>   /* VIDIOC_DBG_G_REGISTER and VIDIOC_DBG_S_REGISTER */
>   
> At that time I was burned by the API changing (the undocumented register
> does not go away) and thus this time we want to follow the advice in the
> header file.  So I really hope that you do not suggest that we use this
> in an application or do you?

Not in a production application of course, no. That's why I asked to 
explain how and why this register has to be changed.

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

Re: [PATCH 1/3] mt9v022: add v4l2 controls for blanking and other register settings

[Detlev Zundel]

Hi Guennadi,

> Hi Detlev
>
> On Fri, 24 Aug 2012, Detlev Zundel wrote:
>
>> Hello Guennadi,
>> 
>> > Hi Anatolij
>> >
>> > On Fri, 24 Aug 2012, Anatolij Gustschin wrote:
>> >
>> >> Add controls for horizontal and vertical blanking, analog control
>> >> and control for undocumented register 32.
>> >
>> > Sorry, I don't think this is a good idea to export an undocumented 
>> > register as a control.
>> 
>> Why exactly is that?  Even though it is not documented, we need to
>> fiddle with it to make our application work at all.  So we tend to
>> believe that other users of the chip will want to use it also.
>
> Below I asked to provide details about how you have to change this 
> register value: toggle dynamically at run-time or just set once at 
> initialisation? Even if toggle: are this certain moments, related to 
> standard camera activities (e.g., starting and stopping streaming, 
> changing geometry etc.) or you have to set this absolutely asynchronously 
> at moments of time, that only your application knows about?

Anatolij can answer those detail questions, all I know is that without
fiddling with the register we do not receive valid pictures at all.

>> Furthermore I don't see that we fundamentally reject patches for other
>> parts in the Linux kernel where people found out things not in the
>> official datasheets.
>
> The problem is not, that this register is undocumented, the problem rather 
> is, that IMHO exporting an API to user-space, setting an undocumented 
> register to arbitrary values is, hm, at least pretty dubious.

As I wrote above, without fiddling with the register, we do _not_
receive correct pictures at all.  So this is not dubious but shown by
experiment to be needed (at least in our setup).

Best wishes
  Detlev

-- 
A language that doesn't affect the way you think about programming, is
not worth knowing.             -- Alan Perlis, Epigrams on Programming

Re: [PATCH 1/3] mt9v022: add v4l2 controls for blanking and other register settings

[Anatolij Gustschin]

Hi Guennadi,

On Fri, 24 Aug 2012 15:35:59 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
...
> Below I asked to provide details about how you have to change this 
> register value: toggle dynamically at run-time or just set once at 
> initialisation? Even if toggle: are this certain moments, related to 
> standard camera activities (e.g., starting and stopping streaming, 
> changing geometry etc.) or you have to set this absolutely asynchronously 
> at moments of time, that only your application knows about?

Every time the sensor is reset, it resets this register. Without setting
the register after sensor reset to the needed value I only get garbage data
from the sensor. Since the possibility to reset the sensor is provided on
the hardware and also used, the register has to be set after each sensor
reset. Only the instance controlling the reset gpio pin "knows" the time,
when the register should be initialized again, so it is asynchronously and
not related to the standard camera activities. But since the stuff is _not_
documented, I can only speculate. Maybe it can be set to different values
to achieve different things, currently I do not know.

Thanks,

Anatolij

Re: [PATCH 1/3] mt9v022: add v4l2 controls for blanking and other register settings

[Guennadi Liakhovetski]

On Fri, 24 Aug 2012, Anatolij Gustschin wrote:

> Hi Guennadi,
> 
> On Fri, 24 Aug 2012 15:35:59 +0200 (CEST)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> ...
> > Below I asked to provide details about how you have to change this 
> > register value: toggle dynamically at run-time or just set once at 
> > initialisation? Even if toggle: are this certain moments, related to 
> > standard camera activities (e.g., starting and stopping streaming, 
> > changing geometry etc.) or you have to set this absolutely asynchronously 
> > at moments of time, that only your application knows about?
> 
> Every time the sensor is reset, it resets this register. Without setting
> the register after sensor reset to the needed value I only get garbage data
> from the sensor. Since the possibility to reset the sensor is provided on
> the hardware and also used, the register has to be set after each sensor
> reset. Only the instance controlling the reset gpio pin "knows" the time,
> when the register should be initialized again, so it is asynchronously and
> not related to the standard camera activities. But since the stuff is _not_
> documented, I can only speculate. Maybe it can be set to different values
> to achieve different things, currently I do not know.

How about adding that register write (if required by platform data) to 
mt9v022_s_power() in the "on" case? This is called (on soc-camera hosts at 
least) on each first open(), would this suffice?

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

Re: [PATCH 1/3] mt9v022: add v4l2 controls for blanking and other register settings

[Anatolij Gustschin]

Hi Guennadi,

On Fri, 24 Aug 2012 23:23:37 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
...
> > Every time the sensor is reset, it resets this register. Without setting
> > the register after sensor reset to the needed value I only get garbage data
> > from the sensor. Since the possibility to reset the sensor is provided on
> > the hardware and also used, the register has to be set after each sensor
> > reset. Only the instance controlling the reset gpio pin "knows" the time,
> > when the register should be initialized again, so it is asynchronously and
> > not related to the standard camera activities. But since the stuff is _not_
> > documented, I can only speculate. Maybe it can be set to different values
> > to achieve different things, currently I do not know.
> 
> How about adding that register write (if required by platform data) to 
> mt9v022_s_power() in the "on" case? This is called (on soc-camera hosts at 
> least) on each first open(), would this suffice?

This would suffice. But now I found some more info for this register.
Rev3. errata mentions that the bit 9 of the register should be set when
in snapshot mode (in my case this is the only mode that we can use).
Additionally the errata recommends to set bit 2 when the high dynamic
range mode is used. Now I'm not sure how to realise these settings.

The bit 9 should be set/unset when configuring the master/snapshot
mode in mt9v022_s_stream(), I think. 

For setting bit 2 we could add V4L2_CID_WIDE_DYNAMIC_RANGE control
which primarily configures the HiDy mode in register 0x0f and
additionally sets/unsets bit 2 in register 0x20. But setting this
bit 2 seems to be needed also in linear mode when manual exposure
control is used. With the recommended register value 0x03D1 in linear
mode the image quality is really bad when manual exposure mode is
used, independent of the configured exposure time. Using 0x03D5
in linear mode however gives good image quality here. So setting
bit 2 should be independent of HiDy control. The errata states "the
register setting recommendations are based on the characterization of
the image sensor only and that camera module makers should test these
recommendations on their module and evaluate the overall performance".
These settings should be configurable independently of each other,
I think.

Thanks,
Anatolij

Re: [PATCH 1/3] mt9v022: add v4l2 controls for blanking and other register settings

[Guennadi Liakhovetski]

Hi Anatolij

On Tue, 28 Aug 2012, Anatolij Gustschin wrote:

> Hi Guennadi,
> 
> On Fri, 24 Aug 2012 23:23:37 +0200 (CEST)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> ...
> > > Every time the sensor is reset, it resets this register. Without setting
> > > the register after sensor reset to the needed value I only get garbage data
> > > from the sensor. Since the possibility to reset the sensor is provided on
> > > the hardware and also used, the register has to be set after each sensor
> > > reset. Only the instance controlling the reset gpio pin "knows" the time,
> > > when the register should be initialized again, so it is asynchronously and
> > > not related to the standard camera activities. But since the stuff is _not_
> > > documented, I can only speculate. Maybe it can be set to different values
> > > to achieve different things, currently I do not know.
> > 
> > How about adding that register write (if required by platform data) to 
> > mt9v022_s_power() in the "on" case? This is called (on soc-camera hosts at 
> > least) on each first open(), would this suffice?
> 
> This would suffice. But now I found some more info for this register.
> Rev3. errata

Yes, I've found that one too. But I don't understand the table they 
present there: have only default values of various registers changed, or 
have actually new features been added in Rev.3? If the latter we'd have to 
find out which revision we're running on.

> mentions that the bit 9 of the register should be set when
> in snapshot mode (in my case this is the only mode that we can use).

Currently the snapshot mode is used in the mt9v022 driver to stop 
streaming. This means you've got more local changes in your driver to 
enable capture in the snapshot mode?

> Additionally the errata recommends to set bit 2 when the high dynamic
> range mode is used.

The HiDy mode is also not used so far in the driver.

> Now I'm not sure how to realise these settings.
> 
> The bit 9 should be set/unset when configuring the master/snapshot
> mode in mt9v022_s_stream(), I think.

As mentioned above, currently the snapshot mode is used by the driver to 
stop continuous data read-out by the sensor, so, unless we begin 
supporting capture in the snapshot mode, setting any further configuration 
parameters seems pretty useless to me.

> For setting bit 2 we could add V4L2_CID_WIDE_DYNAMIC_RANGE control
> which primarily configures the HiDy mode in register 0x0f and
> additionally sets/unsets bit 2 in register 0x20. But setting this
> bit 2 seems to be needed also in linear mode when manual exposure
> control is used. With the recommended register value 0x03D1 in linear
> mode the image quality is really bad when manual exposure mode is
> used, independent of the configured exposure time. Using 0x03D5
> in linear mode however gives good image quality here.

Doesn't this switch on the HiDy mode? What happens if you also set 0x0f:6 
to 1?

> So setting
> bit 2 should be independent of HiDy control. The errata states "the
> register setting recommendations are based on the characterization of
> the image sensor only and that camera module makers should test these
> recommendations on their module and evaluate the overall performance".
> These settings should be configurable independently of each other,
> I think.

Maybe we need some noise (PFN) related control?

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

Re: [PATCH 1/3] mt9v022: add v4l2 controls for blanking and other register settings

[Anatolij Gustschin]

Hi Guennadi,

On Tue, 11 Sep 2012 10:24:23 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

> Hi Anatolij
> 
> On Tue, 28 Aug 2012, Anatolij Gustschin wrote:
> 
> > Hi Guennadi,
> > 
> > On Fri, 24 Aug 2012 23:23:37 +0200 (CEST)
> > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > ...
> > > > Every time the sensor is reset, it resets this register. Without setting
> > > > the register after sensor reset to the needed value I only get garbage data
> > > > from the sensor. Since the possibility to reset the sensor is provided on
> > > > the hardware and also used, the register has to be set after each sensor
> > > > reset. Only the instance controlling the reset gpio pin "knows" the time,
> > > > when the register should be initialized again, so it is asynchronously and
> > > > not related to the standard camera activities. But since the stuff is _not_
> > > > documented, I can only speculate. Maybe it can be set to different values
> > > > to achieve different things, currently I do not know.
> > > 
> > > How about adding that register write (if required by platform data) to 
> > > mt9v022_s_power() in the "on" case? This is called (on soc-camera hosts at 
> > > least) on each first open(), would this suffice?
> > 
> > This would suffice. But now I found some more info for this register.
> > Rev3. errata
> 
> Yes, I've found that one too. But I don't understand the table they 
> present there: have only default values of various registers changed, or 
> have actually new features been added in Rev.3? If the latter we'd have to 
> find out which revision we're running on.

Some issues have been fixed in Rev3 so that these register settings can
work. I've found the technical note describing the snapshot mode and it
says that register 0x20 bit 2 and bit 9 must be set in the snapshot mode.
This applies to mt9v022 rev3 and mt9v024, so it should be configured
dependent on revision, yes. I'll remove register 0x20 setting control
entirely and provide separate patch configuring required settings in
snapshot mode.

> > mentions that the bit 9 of the register should be set when
> > in snapshot mode (in my case this is the only mode that we can use).
> 
> Currently the snapshot mode is used in the mt9v022 driver to stop 
> streaming. This means you've got more local changes in your driver to 
> enable capture in the snapshot mode?

Yes, in my case the capture driver sets this mode in its start_streaming
function. I'll submit the driver, too.

> > Additionally the errata recommends to set bit 2 when the high dynamic
> > range mode is used.
> 
> The HiDy mode is also not used so far in the driver.

Yes. I think support for it could be added as V4L2_CID_WIDE_DYNAMIC_RANGE
control.

> > Now I'm not sure how to realise these settings.
> > 
> > The bit 9 should be set/unset when configuring the master/snapshot
> > mode in mt9v022_s_stream(), I think.
> 
> As mentioned above, currently the snapshot mode is used by the driver to 
> stop continuous data read-out by the sensor, so, unless we begin 
> supporting capture in the snapshot mode, setting any further configuration 
> parameters seems pretty useless to me.

We need the snapshot mode, our camera doesn't work in normal mode,
only external triggering is supported on it. I'll add required
configuration by a separate patch.

> > For setting bit 2 we could add V4L2_CID_WIDE_DYNAMIC_RANGE control
> > which primarily configures the HiDy mode in register 0x0f and
> > additionally sets/unsets bit 2 in register 0x20. But setting this
> > bit 2 seems to be needed also in linear mode when manual exposure
> > control is used. With the recommended register value 0x03D1 in linear
> > mode the image quality is really bad when manual exposure mode is
> > used, independent of the configured exposure time. Using 0x03D5
> > in linear mode however gives good image quality here.
> 
> Doesn't this switch on the HiDy mode? What happens if you also set 0x0f:6 
> to 1?

No, it doesn't. It is supposed to lower pixel-wise FPN. If I configure
HiDy mode by setting 0x0f:6 to 1, I get better image quality, the loss of
detail in very bright or dark areas of a picture is reduced, as expected.

> 
> > So setting
> > bit 2 should be independent of HiDy control. The errata states "the
> > register setting recommendations are based on the characterization of
> > the image sensor only and that camera module makers should test these
> > recommendations on their module and evaluate the overall performance".
> > These settings should be configurable independently of each other,
> > I think.
> 
> Maybe we need some noise (PFN) related control?

Maybe.

Thanks,
Anatolij

Re: [PATCH 1/3] mt9v022: add v4l2 controls for blanking and other register settings

[Anatolij Gustschin]

Hi Guennadi,

On Fri, 24 Aug 2012 13:08:52 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
...
> > +#define MT9V022_HORIZONTAL_BLANKING_MIN	43
> > +#define MT9V022_HORIZONTAL_BLANKING_MAX	1023
> > +#define MT9V022_HORIZONTAL_BLANKING_DEF	94
> > +#define MT9V022_VERTICAL_BLANKING_MIN	2
> 
> Interesting, in my datasheet min is 4. Maybe 4 would be a safer bet then.

The legal range in the datasheet here is 2-3000. The datasheet
states that the minimal value must be 4 only if "show dark rows"
control bit is set (it is unset by default).

...
> > +#define V4L2_CID_REG32			(V4L2_CTRL_CLASS_CAMERA | 0x1001)
> > +#define V4L2_CID_ANALOG_CONTROLS	(V4L2_CTRL_CLASS_CAMERA | 0x1002)
> 
> Sorry, no again. The MT9V022_ANALOG_CONTROL register contains two fields: 
> anti-eclipse and "anti-eclipse reference voltage control," don't think 
> they should be set as a single control value. IIUC, controls are supposed 
> to control logical parameters of the system. In this case you could 
> introduce an "anti-eclipse reference voltage" control with values in the 
> range between 0 and 2250mV, setting it to anything below 1900mV would turn 
> the enable bit off. Would such a control make sense? Then you might want 
> to ask on the ML, whether this control would make sense as a generic one, 
> not mt9v022 specific.

It probably makes sense since other sensors also have anti-eclipse control
registers.

...
> >  	if (mt9v022->hdl.error) {
> >  		int err = mt9v022->hdl.error;
> >  
> > +		dev_err(&client->dev, "hdl init err %d\n", err);
> 
> That's not very clear IMHO. "hdl" isn't too specific, just "control 
> initialisation?"

Ok, I'll fix it.

Thanks,

Anatolij

[PATCH v2 1/3] mt9v022: add v4l2 controls for blanking

[Anatolij Gustschin]

Add controls for horizontal and vertical blanking. Also add an error
message for case that the control handler init failed. Since setting
the blanking registers is done by controls now, we shouldn't change
these registers outside of the control function. Use v4l2_ctrl_s_ctrl()
to set them.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
Changes since first version:
 - drop analog and reg32 setting controls
 - use more descriptive error message for handler init error
 - revise commit log
 - rebase on staging/for_v3.7 branch

 drivers/media/i2c/soc_camera/mt9v022.c |   49 +++++++++++++++++++++++++++++--
 1 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
index 350d0d8..3cb23c6 100644
--- a/drivers/media/i2c/soc_camera/mt9v022.c
+++ b/drivers/media/i2c/soc_camera/mt9v022.c
@@ -71,6 +71,13 @@ MODULE_PARM_DESC(sensor_type, "Sensor type: \"colour\" or \"monochrome\"");
 #define MT9V022_COLUMN_SKIP		1
 #define MT9V022_ROW_SKIP		4
 
+#define MT9V022_HORIZONTAL_BLANKING_MIN	43
+#define MT9V022_HORIZONTAL_BLANKING_MAX	1023
+#define MT9V022_HORIZONTAL_BLANKING_DEF	94
+#define MT9V022_VERTICAL_BLANKING_MIN	2
+#define MT9V022_VERTICAL_BLANKING_MAX	3000
+#define MT9V022_VERTICAL_BLANKING_DEF	45
+
 #define is_mt9v024(id) (id == 0x1324)
 
 /* MT9V022 has only one fixed colorspace per pixelcode */
@@ -136,6 +143,8 @@ struct mt9v022 {
 		struct v4l2_ctrl *autogain;
 		struct v4l2_ctrl *gain;
 	};
+	struct v4l2_ctrl *hblank;
+	struct v4l2_ctrl *vblank;
 	struct v4l2_rect rect;	/* Sensor window */
 	const struct mt9v022_datafmt *fmt;
 	const struct mt9v022_datafmt *fmts;
@@ -277,11 +286,10 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 		 * Default 94, Phytec driver says:
 		 * "width + horizontal blank >= 660"
 		 */
-		ret = reg_write(client, MT9V022_HORIZONTAL_BLANKING,
-				rect.width > 660 - 43 ? 43 :
-				660 - rect.width);
+		ret = v4l2_ctrl_s_ctrl(mt9v022->hblank,
+				rect.width > 660 - 43 ? 43 : 660 - rect.width);
 	if (!ret)
-		ret = reg_write(client, MT9V022_VERTICAL_BLANKING, 45);
+		ret = v4l2_ctrl_s_ctrl(mt9v022->vblank, 45);
 	if (!ret)
 		ret = reg_write(client, MT9V022_WINDOW_WIDTH, rect.width);
 	if (!ret)
@@ -504,6 +512,18 @@ static int mt9v022_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 		range = exp->maximum - exp->minimum;
 		exp->val = ((data - 1) * range + 239) / 479 + exp->minimum;
 		return 0;
+	case V4L2_CID_HBLANK:
+		data = reg_read(client, MT9V022_HORIZONTAL_BLANKING);
+		if (data < 0)
+			return -EIO;
+		ctrl->val = data;
+		return 0;
+	case V4L2_CID_VBLANK:
+		data = reg_read(client, MT9V022_VERTICAL_BLANKING);
+		if (data < 0)
+			return -EIO;
+		ctrl->val = data;
+		return 0;
 	}
 	return -EINVAL;
 }
@@ -585,6 +605,16 @@ static int mt9v022_s_ctrl(struct v4l2_ctrl *ctrl)
 				return -EIO;
 		}
 		return 0;
+	case V4L2_CID_HBLANK:
+		if (reg_write(client, MT9V022_HORIZONTAL_BLANKING,
+				ctrl->val) < 0)
+			return -EIO;
+		return 0;
+	case V4L2_CID_VBLANK:
+		if (reg_write(client, MT9V022_VERTICAL_BLANKING,
+				ctrl->val) < 0)
+			return -EIO;
+		return 0;
 	}
 	return -EINVAL;
 }
@@ -852,10 +882,21 @@ static int mt9v022_probe(struct i2c_client *client,
 	mt9v022->exposure = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
 			V4L2_CID_EXPOSURE, 1, 255, 1, 255);
 
+	mt9v022->hblank = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
+			V4L2_CID_HBLANK, MT9V022_HORIZONTAL_BLANKING_MIN,
+			MT9V022_HORIZONTAL_BLANKING_MAX, 1,
+			MT9V022_HORIZONTAL_BLANKING_DEF);
+
+	mt9v022->vblank = v4l2_ctrl_new_std(&mt9v022->hdl, &mt9v022_ctrl_ops,
+			V4L2_CID_VBLANK, MT9V022_VERTICAL_BLANKING_MIN,
+			MT9V022_VERTICAL_BLANKING_MAX, 1,
+			MT9V022_VERTICAL_BLANKING_DEF);
+
 	mt9v022->subdev.ctrl_handler = &mt9v022->hdl;
 	if (mt9v022->hdl.error) {
 		int err = mt9v022->hdl.error;
 
+		dev_err(&client->dev, "control initialisation err %d\n", err);
 		kfree(mt9v022);
 		return err;
 	}
-- 
1.7.1

Re: [PATCH v2 1/3] mt9v022: add v4l2 controls for blanking

[Anatolij Gustschin]

Hi Guennadi,

On Fri, 28 Sep 2012 00:03:45 +0200
Anatolij Gustschin <agust@denx.de> wrote:

> Add controls for horizontal and vertical blanking. Also add an error
> message for case that the control handler init failed. Since setting
> the blanking registers is done by controls now, we shouldn't change
> these registers outside of the control function. Use v4l2_ctrl_s_ctrl()
> to set them.
> 
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> ---
> Changes since first version:
>  - drop analog and reg32 setting controls
>  - use more descriptive error message for handler init error
>  - revise commit log
>  - rebase on staging/for_v3.7 branch
> 
>  drivers/media/i2c/soc_camera/mt9v022.c |   49 +++++++++++++++++++++++++++++--
>  1 files changed, 45 insertions(+), 4 deletions(-)

Can these mt9v022 patches be queued for mainlining, please?

Thanks,
Anatolij

Re: [PATCH v2 1/3] mt9v022: add v4l2 controls for blanking

[Guennadi Liakhovetski]

On Sat, 6 Oct 2012, Anatolij Gustschin wrote:

> Hi Guennadi,
> 
> On Fri, 28 Sep 2012 00:03:45 +0200
> Anatolij Gustschin <agust@denx.de> wrote:
> 
> > Add controls for horizontal and vertical blanking. Also add an error
> > message for case that the control handler init failed. Since setting
> > the blanking registers is done by controls now, we shouldn't change
> > these registers outside of the control function. Use v4l2_ctrl_s_ctrl()
> > to set them.
> > 
> > Signed-off-by: Anatolij Gustschin <agust@denx.de>
> > ---
> > Changes since first version:
> >  - drop analog and reg32 setting controls
> >  - use more descriptive error message for handler init error
> >  - revise commit log
> >  - rebase on staging/for_v3.7 branch
> > 
> >  drivers/media/i2c/soc_camera/mt9v022.c |   49 +++++++++++++++++++++++++++++--
> >  1 files changed, 45 insertions(+), 4 deletions(-)
> 
> Can these mt9v022 patches be queued for mainlining, please?

Sure, so far I'm collecting patches to be queued for 3.8.

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

[PATCH 2/3] mt9v022: fix the V4L2_CID_EXPOSURE control

[Anatolij Gustschin]

Since the MT9V022_TOTAL_SHUTTER_WIDTH register is controlled in manual
mode by V4L2_CID_EXPOSURE control, it shouldn't be written directly in
mt9v022_s_crop(). In manual mode this register should be set to the
V4L2_CID_EXPOSURE control value. Changing this register directly and
outside of the actual control function means that the register value
is not in sync with the corresponding control value. Thus, the following
problem is observed:

    - setting this control initially succeeds
    - VIDIOC_S_CROP ioctl() overwrites the MT9V022_TOTAL_SHUTTER_WIDTH
      register
    - setting this control to the same value again doesn't
      result in setting the register since the control value
      was previously cached and doesn't differ

Fix it by always setting the register to the controlled value, when
in manual mode.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
 drivers/media/i2c/soc_camera/mt9v022.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
index d13c8c4..d26c071 100644
--- a/drivers/media/i2c/soc_camera/mt9v022.c
+++ b/drivers/media/i2c/soc_camera/mt9v022.c
@@ -274,9 +274,9 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 		if (ret & 1) /* Autoexposure */
 			ret = reg_write(client, mt9v022->reg->max_total_shutter_width,
 					rect.height + mt9v022->y_skip_top + 43);
-		else
-			ret = reg_write(client, MT9V022_TOTAL_SHUTTER_WIDTH,
-					rect.height + mt9v022->y_skip_top + 43);
+		else /* Set to the manually controlled value */
+			ret = v4l2_ctrl_s_ctrl(mt9v022->exposure,
+					       mt9v022->exposure->val);
 	}
 	/* Setup frame format: defaults apart from width and height */
 	if (!ret)
-- 
1.7.1

Re: [PATCH 2/3] mt9v022: fix the V4L2_CID_EXPOSURE control

[Guennadi Liakhovetski]

On Fri, 24 Aug 2012, Anatolij Gustschin wrote:

> Since the MT9V022_TOTAL_SHUTTER_WIDTH register is controlled in manual
> mode by V4L2_CID_EXPOSURE control, it shouldn't be written directly in
> mt9v022_s_crop(). In manual mode this register should be set to the
> V4L2_CID_EXPOSURE control value. Changing this register directly and
> outside of the actual control function means that the register value
> is not in sync with the corresponding control value. Thus, the following
> problem is observed:
> 
>     - setting this control initially succeeds
>     - VIDIOC_S_CROP ioctl() overwrites the MT9V022_TOTAL_SHUTTER_WIDTH
>       register
>     - setting this control to the same value again doesn't
>       result in setting the register since the control value
>       was previously cached and doesn't differ
> 
> Fix it by always setting the register to the controlled value, when
> in manual mode.
> 
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> ---
>  drivers/media/i2c/soc_camera/mt9v022.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
> index d13c8c4..d26c071 100644
> --- a/drivers/media/i2c/soc_camera/mt9v022.c
> +++ b/drivers/media/i2c/soc_camera/mt9v022.c
> @@ -274,9 +274,9 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
>  		if (ret & 1) /* Autoexposure */
>  			ret = reg_write(client, mt9v022->reg->max_total_shutter_width,
>  					rect.height + mt9v022->y_skip_top + 43);
> -		else
> -			ret = reg_write(client, MT9V022_TOTAL_SHUTTER_WIDTH,
> -					rect.height + mt9v022->y_skip_top + 43);
> +		else /* Set to the manually controlled value */
> +			ret = v4l2_ctrl_s_ctrl(mt9v022->exposure,
> +					       mt9v022->exposure->val);

But why do we have to write it here at all then? Autoexposure can be off 
only if the user has set exposure manually, using V4L2_CID_EXPOSURE_AUTO. 
In this case MT9V022_TOTAL_SHUTTER_WIDTH already contains the correct 
value. Why do we have to set it again? Maybe just adding a comment, 
explaining the above, would suffice?

>  	}
>  	/* Setup frame format: defaults apart from width and height */
>  	if (!ret)
> -- 
> 1.7.1

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

Re: [PATCH 2/3] mt9v022: fix the V4L2_CID_EXPOSURE control

[Anatolij Gustschin]

Hi Guennadi,

On Fri, 24 Aug 2012 13:22:18 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
...
> > --- a/drivers/media/i2c/soc_camera/mt9v022.c
> > +++ b/drivers/media/i2c/soc_camera/mt9v022.c
> > @@ -274,9 +274,9 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
> >  		if (ret & 1) /* Autoexposure */
> >  			ret = reg_write(client, mt9v022->reg->max_total_shutter_width,
> >  					rect.height + mt9v022->y_skip_top + 43);
> > -		else
> > -			ret = reg_write(client, MT9V022_TOTAL_SHUTTER_WIDTH,
> > -					rect.height + mt9v022->y_skip_top + 43);
> > +		else /* Set to the manually controlled value */
> > +			ret = v4l2_ctrl_s_ctrl(mt9v022->exposure,
> > +					       mt9v022->exposure->val);
> 
> But why do we have to write it here at all then? Autoexposure can be off 
> only if the user has set exposure manually, using V4L2_CID_EXPOSURE_AUTO. 
> In this case MT9V022_TOTAL_SHUTTER_WIDTH already contains the correct 
> value. Why do we have to set it again? Maybe just adding a comment, 
> explaining the above, would suffice?

Actually we do not have to write it here, yes. Should I remove the shutter
register setting here entirely? And add a comment explaining, why?

Thanks,

Anatolij

Re: [PATCH 2/3] mt9v022: fix the V4L2_CID_EXPOSURE control

[Guennadi Liakhovetski]

On Fri, 24 Aug 2012, Anatolij Gustschin wrote:

> Hi Guennadi,
> 
> On Fri, 24 Aug 2012 13:22:18 +0200 (CEST)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> ...
> > > --- a/drivers/media/i2c/soc_camera/mt9v022.c
> > > +++ b/drivers/media/i2c/soc_camera/mt9v022.c
> > > @@ -274,9 +274,9 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
> > >  		if (ret & 1) /* Autoexposure */
> > >  			ret = reg_write(client, mt9v022->reg->max_total_shutter_width,
> > >  					rect.height + mt9v022->y_skip_top + 43);
> > > -		else
> > > -			ret = reg_write(client, MT9V022_TOTAL_SHUTTER_WIDTH,
> > > -					rect.height + mt9v022->y_skip_top + 43);
> > > +		else /* Set to the manually controlled value */
> > > +			ret = v4l2_ctrl_s_ctrl(mt9v022->exposure,
> > > +					       mt9v022->exposure->val);
> > 
> > But why do we have to write it here at all then? Autoexposure can be off 
> > only if the user has set exposure manually, using V4L2_CID_EXPOSURE_AUTO. 
> > In this case MT9V022_TOTAL_SHUTTER_WIDTH already contains the correct 
> > value. Why do we have to set it again? Maybe just adding a comment, 
> > explaining the above, would suffice?
> 
> Actually we do not have to write it here, yes. Should I remove the shutter
> register setting here entirely? And add a comment explaining, why?

Remove it from the "else" clause, yes, please. And a comment would be 
good!

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

Re: [PATCH 2/3] mt9v022: fix the V4L2_CID_EXPOSURE control

[Anatolij Gustschin]

On Fri, 24 Aug 2012 16:32:57 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
...
> > > But why do we have to write it here at all then? Autoexposure can be off 
> > > only if the user has set exposure manually, using V4L2_CID_EXPOSURE_AUTO. 
> > > In this case MT9V022_TOTAL_SHUTTER_WIDTH already contains the correct 
> > > value. Why do we have to set it again? Maybe just adding a comment, 
> > > explaining the above, would suffice?
> > 
> > Actually we do not have to write it here, yes. Should I remove the shutter
> > register setting here entirely? And add a comment explaining, why?
> 
> Remove it from the "else" clause, yes, please. And a comment would be 
> good!

Ok, I'll resubmit a reworked patch.

Thanks,
Anatolij

[PATCH v2 2/3] mt9v022: fix the V4L2_CID_EXPOSURE control

[Anatolij Gustschin]

Since the MT9V022_TOTAL_SHUTTER_WIDTH register is controlled in manual
mode by V4L2_CID_EXPOSURE control, it shouldn't be written directly in
mt9v022_s_crop(). In manual mode this register should be set to the
V4L2_CID_EXPOSURE control value. Changing this register directly and
outside of the actual control function means that the register value
is not in sync with the corresponding control value. Thus, the following
problem is observed:

    - setting this control initially succeeds
    - VIDIOC_S_CROP ioctl() overwrites the MT9V022_TOTAL_SHUTTER_WIDTH
      register
    - setting this control to the same value again doesn't
      result in setting the register since the control value
      was previously cached and doesn't differ

Remove MT9V022_TOTAL_SHUTTER_WIDTH register setting in mt9v022_s_crop()
and add a comment explaining why it is not needed in manual mode.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
Changes since first version:
 - remove setting total shutter width register in mt9v022_s_crop()
   if in manual exposure mode and add a comment explaining why it is
   not needed
 - revise commit log
 - rebase on staging/for_v3.7 branch

 drivers/media/i2c/soc_camera/mt9v022.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
index 3cb23c6..e0f4cb4 100644
--- a/drivers/media/i2c/soc_camera/mt9v022.c
+++ b/drivers/media/i2c/soc_camera/mt9v022.c
@@ -272,9 +272,14 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 		if (ret & 1) /* Autoexposure */
 			ret = reg_write(client, mt9v022->reg->max_total_shutter_width,
 					rect.height + mt9v022->y_skip_top + 43);
-		else
-			ret = reg_write(client, MT9V022_TOTAL_SHUTTER_WIDTH,
-					rect.height + mt9v022->y_skip_top + 43);
+		/*
+		 * If autoexposure is off, there is no need to set
+		 * MT9V022_TOTAL_SHUTTER_WIDTH here. Autoexposure can be off
+		 * only if the user has set exposure manually, using the
+		 * V4L2_CID_EXPOSURE_AUTO with the value V4L2_EXPOSURE_MANUAL.
+		 * In this case the register MT9V022_TOTAL_SHUTTER_WIDTH
+		 * already contains the correct value.
+		 */
 	}
 	/* Setup frame format: defaults apart from width and height */
 	if (!ret)
-- 
1.7.1

[PATCH 3/3] mt9v022: set y_skip_top field to zero

[Anatolij Gustschin]

Set "y_skip_top" to zero and remove comment as I do not see this
line corruption on two different mt9v022 setups. The first read-out
line is perfectly fine.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
 drivers/media/i2c/soc_camera/mt9v022.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
index d26c071..e41d738 100644
--- a/drivers/media/i2c/soc_camera/mt9v022.c
+++ b/drivers/media/i2c/soc_camera/mt9v022.c
@@ -960,11 +960,7 @@ static int mt9v022_probe(struct i2c_client *client,
 
 	mt9v022->chip_control = MT9V022_CHIP_CONTROL_DEFAULT;
 
-	/*
-	 * MT9V022 _really_ corrupts the first read out line.
-	 * TODO: verify on i.MX31
-	 */
-	mt9v022->y_skip_top	= 1;
+	mt9v022->y_skip_top	= 0;
 	mt9v022->rect.left	= MT9V022_COLUMN_SKIP;
 	mt9v022->rect.top	= MT9V022_ROW_SKIP;
 	mt9v022->rect.width	= MT9V022_MAX_WIDTH;
-- 
1.7.1

Re: [PATCH 3/3] mt9v022: set y_skip_top field to zero

[Guennadi Liakhovetski]

On Fri, 24 Aug 2012, Anatolij Gustschin wrote:

> Set "y_skip_top" to zero and remove comment as I do not see this
> line corruption on two different mt9v022 setups. The first read-out
> line is perfectly fine.

On what systems have you checked this?

Thanks
Guennadi

> 
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> ---
>  drivers/media/i2c/soc_camera/mt9v022.c |    6 +-----
>  1 files changed, 1 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
> index d26c071..e41d738 100644
> --- a/drivers/media/i2c/soc_camera/mt9v022.c
> +++ b/drivers/media/i2c/soc_camera/mt9v022.c
> @@ -960,11 +960,7 @@ static int mt9v022_probe(struct i2c_client *client,
>  
>  	mt9v022->chip_control = MT9V022_CHIP_CONTROL_DEFAULT;
>  
> -	/*
> -	 * MT9V022 _really_ corrupts the first read out line.
> -	 * TODO: verify on i.MX31
> -	 */
> -	mt9v022->y_skip_top	= 1;
> +	mt9v022->y_skip_top	= 0;
>  	mt9v022->rect.left	= MT9V022_COLUMN_SKIP;
>  	mt9v022->rect.top	= MT9V022_ROW_SKIP;
>  	mt9v022->rect.width	= MT9V022_MAX_WIDTH;
> -- 
> 1.7.1
> 

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

Re: [PATCH 3/3] mt9v022: set y_skip_top field to zero

[Anatolij Gustschin]

On Fri, 24 Aug 2012 13:23:22 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

> On Fri, 24 Aug 2012, Anatolij Gustschin wrote:
> 
> > Set "y_skip_top" to zero and remove comment as I do not see this
> > line corruption on two different mt9v022 setups. The first read-out
> > line is perfectly fine.
> 
> On what systems have you checked this?

On camera systems from ifm, both using mt9v022.

Anatolij

Re: [PATCH 3/3] mt9v022: set y_skip_top field to zero

[Guennadi Liakhovetski]

On Fri, 24 Aug 2012, Anatolij Gustschin wrote:

> On Fri, 24 Aug 2012 13:23:22 +0200 (CEST)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 
> > On Fri, 24 Aug 2012, Anatolij Gustschin wrote:
> > 
> > > Set "y_skip_top" to zero and remove comment as I do not see this
> > > line corruption on two different mt9v022 setups. The first read-out
> > > line is perfectly fine.
> > 
> > On what systems have you checked this?
> 
> On camera systems from ifm, both using mt9v022.

Ok, I agree, this was a hack in the beginning, and, probably, there was a 
reason for the problem, that we've seen, that we didn't find a proper 
solution to, but I wouldn't like to punish those systems now. The 
y_skip_top field is only taken into account by the pxa driver, and there 
is only one pxa270 system, using mt9v022: pcm990-baseboard.c. Could you, 
please, add platform data to mt9v022 with only one parameter to initialise 
y_skip_top, use 0 as default and set it to 1 on pcm990-baseboard.c?

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

Re: [PATCH 3/3] mt9v022: set y_skip_top field to zero

[Anatolij Gustschin]

Hi Guennadi,

On Tue, 11 Sep 2012 10:55:31 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
...
> > > On what systems have you checked this?
> > 
> > On camera systems from ifm, both using mt9v022.
> 
> Ok, I agree, this was a hack in the beginning, and, probably, there was a 
> reason for the problem, that we've seen, that we didn't find a proper 
> solution to, but I wouldn't like to punish those systems now. The 
> y_skip_top field is only taken into account by the pxa driver, and there 
> is only one pxa270 system, using mt9v022: pcm990-baseboard.c. Could you, 
> please, add platform data to mt9v022 with only one parameter to initialise 
> y_skip_top, use 0 as default and set it to 1 on pcm990-baseboard.c?

Yes, I've reworked this patch as suggested and will resubmit.

Thanks,
Anatolij

[PATCH v2 3/3] mt9v022: set y_skip_top field to zero as default

[Anatolij Gustschin]

Set "y_skip_top" to zero and revise comment as I do not see this line
corruption on two different mt9v022 setups. The first read-out line
is perfectly fine. Add mt9v022 platform data configuring y_skip_top
for platforms that have issues with the first read-out line. Set
y_skip_top to 1 for pcm990 board.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
Changes since first version:
 - add platform data to mt9v022 with only one parameter to initialise 
   y_skip_top, use 0 as default and set it to 1 on pcm990-baseboard.c
 - revise commit log
 - rebase on staging/for_v3.7 branch

 arch/arm/mach-pxa/pcm990-baseboard.c   |    6 ++++++
 drivers/media/i2c/soc_camera/mt9v022.c |    8 +++++---
 include/media/mt9v022.h                |   16 ++++++++++++++++
 3 files changed, 27 insertions(+), 3 deletions(-)
 create mode 100644 include/media/mt9v022.h

diff --git a/arch/arm/mach-pxa/pcm990-baseboard.c b/arch/arm/mach-pxa/pcm990-baseboard.c
index cb723e8..e2973f2 100644
--- a/arch/arm/mach-pxa/pcm990-baseboard.c
+++ b/arch/arm/mach-pxa/pcm990-baseboard.c
@@ -26,6 +26,7 @@
 #include <linux/i2c/pxa-i2c.h>
 #include <linux/pwm_backlight.h>
 
+#include <media/mt9v022.h>
 #include <media/soc_camera.h>
 
 #include <mach/camera.h>
@@ -468,6 +469,10 @@ static struct i2c_board_info __initdata pcm990_i2c_devices[] = {
 	},
 };
 
+static struct mt9v022_platform_data mt9v022_pdata = {
+	.y_skip_top = 1,
+};
+
 static struct i2c_board_info pcm990_camera_i2c[] = {
 	{
 		I2C_BOARD_INFO("mt9v022", 0x48),
@@ -480,6 +485,7 @@ static struct soc_camera_link iclink[] = {
 	{
 		.bus_id			= 0, /* Must match with the camera ID */
 		.board_info		= &pcm990_camera_i2c[0],
+		.priv			= &mt9v022_pdata,
 		.i2c_adapter_id		= 0,
 		.query_bus_param	= pcm990_camera_query_bus_param,
 		.set_bus_param		= pcm990_camera_set_bus_param,
diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
index e0f4cb4..8feaddc 100644
--- a/drivers/media/i2c/soc_camera/mt9v022.c
+++ b/drivers/media/i2c/soc_camera/mt9v022.c
@@ -15,6 +15,7 @@
 #include <linux/log2.h>
 #include <linux/module.h>
 
+#include <media/mt9v022.h>
 #include <media/soc_camera.h>
 #include <media/soc_mediabus.h>
 #include <media/v4l2-subdev.h>
@@ -849,6 +850,7 @@ static int mt9v022_probe(struct i2c_client *client,
 	struct mt9v022 *mt9v022;
 	struct soc_camera_link *icl = soc_camera_i2c_to_link(client);
 	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+	struct mt9v022_platform_data *pdata = icl->priv;
 	int ret;
 
 	if (!icl) {
@@ -912,10 +914,10 @@ static int mt9v022_probe(struct i2c_client *client,
 	mt9v022->chip_control = MT9V022_CHIP_CONTROL_DEFAULT;
 
 	/*
-	 * MT9V022 _really_ corrupts the first read out line.
-	 * TODO: verify on i.MX31
+	 * On some platforms the first read out line is corrupted.
+	 * Workaround it by skipping if indicated by platform data.
 	 */
-	mt9v022->y_skip_top	= 1;
+	mt9v022->y_skip_top	= pdata ? pdata->y_skip_top : 0;
 	mt9v022->rect.left	= MT9V022_COLUMN_SKIP;
 	mt9v022->rect.top	= MT9V022_ROW_SKIP;
 	mt9v022->rect.width	= MT9V022_MAX_WIDTH;
diff --git a/include/media/mt9v022.h b/include/media/mt9v022.h
new file mode 100644
index 0000000..4056180
--- /dev/null
+++ b/include/media/mt9v022.h
@@ -0,0 +1,16 @@
+/*
+ * mt9v022 sensor
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __MT9V022_H__
+#define __MT9V022_H__
+
+struct mt9v022_platform_data {
+	unsigned short y_skip_top;	/* Lines to skip at the top */
+};
+
+#endif
-- 
1.7.1

Re: [PATCH v2 3/3] mt9v022: set y_skip_top field to zero as default

[Eric Miao]

On Fri, Sep 28, 2012 at 8:21 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> Hi Eric
>
> On Fri, 28 Sep 2012, Guennadi Liakhovetski wrote:
>
>> Hi Anatolij
>>
>> I can take this patch, but we need an ack from a PXA / ARM maintainer.
>
> Could we have your ack, please?

Yes, this looks completely good to me. Sorry for the delay.

Acked-by: Eric Miao <eric.y.miao@gmail.com>

>
> Thanks
> Guennadi
>
>> On Fri, 28 Sep 2012, Anatolij Gustschin wrote:
>>
>> > Set "y_skip_top" to zero and revise comment as I do not see this line
>> > corruption on two different mt9v022 setups. The first read-out line
>> > is perfectly fine. Add mt9v022 platform data configuring y_skip_top
>> > for platforms that have issues with the first read-out line. Set
>> > y_skip_top to 1 for pcm990 board.
>> >
>> > Signed-off-by: Anatolij Gustschin <agust@denx.de>
>> > ---
>> > Changes since first version:
>> >  - add platform data to mt9v022 with only one parameter to initialise
>> >    y_skip_top, use 0 as default and set it to 1 on pcm990-baseboard.c
>> >  - revise commit log
>> >  - rebase on staging/for_v3.7 branch
>> >
>> >  arch/arm/mach-pxa/pcm990-baseboard.c   |    6 ++++++
>> >  drivers/media/i2c/soc_camera/mt9v022.c |    8 +++++---
>> >  include/media/mt9v022.h                |   16 ++++++++++++++++
>> >  3 files changed, 27 insertions(+), 3 deletions(-)
>> >  create mode 100644 include/media/mt9v022.h
>> >
>> > diff --git a/arch/arm/mach-pxa/pcm990-baseboard.c b/arch/arm/mach-pxa/pcm990-baseboard.c
>> > index cb723e8..e2973f2 100644
>> > --- a/arch/arm/mach-pxa/pcm990-baseboard.c
>> > +++ b/arch/arm/mach-pxa/pcm990-baseboard.c
>> > @@ -26,6 +26,7 @@
>> >  #include <linux/i2c/pxa-i2c.h>
>> >  #include <linux/pwm_backlight.h>
>> >
>> > +#include <media/mt9v022.h>
>> >  #include <media/soc_camera.h>
>> >
>> >  #include <mach/camera.h>
>> > @@ -468,6 +469,10 @@ static struct i2c_board_info __initdata pcm990_i2c_devices[] = {
>> >     },
>> >  };
>> >
>> > +static struct mt9v022_platform_data mt9v022_pdata = {
>> > +   .y_skip_top = 1,
>> > +};
>> > +
>> >  static struct i2c_board_info pcm990_camera_i2c[] = {
>> >     {
>> >             I2C_BOARD_INFO("mt9v022", 0x48),
>> > @@ -480,6 +485,7 @@ static struct soc_camera_link iclink[] = {
>> >     {
>> >             .bus_id                 = 0, /* Must match with the camera ID */
>> >             .board_info             = &pcm990_camera_i2c[0],
>> > +           .priv                   = &mt9v022_pdata,
>> >             .i2c_adapter_id         = 0,
>> >             .query_bus_param        = pcm990_camera_query_bus_param,
>> >             .set_bus_param          = pcm990_camera_set_bus_param,
>> > diff --git a/drivers/media/i2c/soc_camera/mt9v022.c b/drivers/media/i2c/soc_camera/mt9v022.c
>> > index e0f4cb4..8feaddc 100644
>> > --- a/drivers/media/i2c/soc_camera/mt9v022.c
>> > +++ b/drivers/media/i2c/soc_camera/mt9v022.c
>> > @@ -15,6 +15,7 @@
>> >  #include <linux/log2.h>
>> >  #include <linux/module.h>
>> >
>> > +#include <media/mt9v022.h>
>> >  #include <media/soc_camera.h>
>> >  #include <media/soc_mediabus.h>
>> >  #include <media/v4l2-subdev.h>
>> > @@ -849,6 +850,7 @@ static int mt9v022_probe(struct i2c_client *client,
>> >     struct mt9v022 *mt9v022;
>> >     struct soc_camera_link *icl = soc_camera_i2c_to_link(client);
>> >     struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
>> > +   struct mt9v022_platform_data *pdata = icl->priv;
>> >     int ret;
>> >
>> >     if (!icl) {
>> > @@ -912,10 +914,10 @@ static int mt9v022_probe(struct i2c_client *client,
>> >     mt9v022->chip_control = MT9V022_CHIP_CONTROL_DEFAULT;
>> >
>> >     /*
>> > -    * MT9V022 _really_ corrupts the first read out line.
>> > -    * TODO: verify on i.MX31
>> > +    * On some platforms the first read out line is corrupted.
>> > +    * Workaround it by skipping if indicated by platform data.
>> >      */
>> > -   mt9v022->y_skip_top     = 1;
>> > +   mt9v022->y_skip_top     = pdata ? pdata->y_skip_top : 0;
>> >     mt9v022->rect.left      = MT9V022_COLUMN_SKIP;
>> >     mt9v022->rect.top       = MT9V022_ROW_SKIP;
>> >     mt9v022->rect.width     = MT9V022_MAX_WIDTH;
>> > diff --git a/include/media/mt9v022.h b/include/media/mt9v022.h
>> > new file mode 100644
>> > index 0000000..4056180
>> > --- /dev/null
>> > +++ b/include/media/mt9v022.h
>> > @@ -0,0 +1,16 @@
>> > +/*
>> > + * mt9v022 sensor
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License version 2 as
>> > + * published by the Free Software Foundation.
>> > + */
>> > +
>> > +#ifndef __MT9V022_H__
>> > +#define __MT9V022_H__
>> > +
>> > +struct mt9v022_platform_data {
>> > +   unsigned short y_skip_top;      /* Lines to skip at the top */
>> > +};
>> > +
>> > +#endif
>> > --
>> > 1.7.1
>> >
>>
>> ---
>> Guennadi Liakhovetski, Ph.D.
>> Freelance Open-Source Software Developer
>> http://www.open-technology.de/
>>
>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/