[PATCH 0/4] Aptinate sensors patches

[PATCH 0/4] Aptinate sensors patches

[Laurent Pinchart]

Hi everybody,

Here are three fixes/patches for the MT9P031 and MT9V032 sensor drivers. The
second patch (mt9v032 pixel rate control) requires a control framework
modification (1/4) that has already been reviewed.

Sakari, I've rebased your patch on top of the latest media tree, could you
please review it ?

Laurent Pinchart (3):
  v4l2-ctrls: Add v4l2_ctrl_[gs]_ctrl_int64()
  mt9v032: Export horizontal and vertical blanking as V4L2 controls
  mt9p031: Fix horizontal and vertical blanking configuration

Sakari Ailus (1):
  mt9v032: Provide pixel rate control

 drivers/media/video/mt9p031.c    |   12 ++--
 drivers/media/video/mt9v032.c    |   59 +++++++++++++++-
 drivers/media/video/v4l2-ctrls.c |  135 +++++++++++++++++++++++---------------
 include/media/v4l2-ctrls.h       |   23 +++++++
 4 files changed, 166 insertions(+), 63 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/4] v4l2-ctrls: Add v4l2_ctrl_[gs]_ctrl_int64()

[Laurent Pinchart]

These helper functions get and set a 64-bit control's value from within
a driver. They are similar to v4l2_ctrl_[gs]_ctrl() but operate on
64-bit integer controls instead of 32-bit controls.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/v4l2-ctrls.c |  135 +++++++++++++++++++++++---------------
 include/media/v4l2-ctrls.h       |   23 +++++++
 2 files changed, 105 insertions(+), 53 deletions(-)

diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 9abd9ab..9b9b825 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -1173,76 +1173,53 @@ static int cluster_changed(struct v4l2_ctrl *master)
 	return diff;
 }
 
-/* Validate integer-type control */
-static int validate_new_int(const struct v4l2_ctrl *ctrl, s32 *pval)
+/* Validate a new control */
+static int validate_new(const struct v4l2_ctrl *ctrl,
+			struct v4l2_ext_control *c)
 {
-	s32 val = *pval;
+	size_t len;
 	u32 offset;
+	s32 val;
 
 	switch (ctrl->type) {
 	case V4L2_CTRL_TYPE_INTEGER:
 		/* Round towards the closest legal value */
-		val += ctrl->step / 2;
-		if (val < ctrl->minimum)
-			val = ctrl->minimum;
-		if (val > ctrl->maximum)
-			val = ctrl->maximum;
+		val = c->value + ctrl->step / 2;
+		val = clamp(val, ctrl->minimum, ctrl->maximum);
 		offset = val - ctrl->minimum;
 		offset = ctrl->step * (offset / ctrl->step);
-		val = ctrl->minimum + offset;
-		*pval = val;
+		c->value = ctrl->minimum + offset;
 		return 0;
 
 	case V4L2_CTRL_TYPE_BOOLEAN:
-		*pval = !!val;
+		c->value = !!c->value;
 		return 0;
 
 	case V4L2_CTRL_TYPE_MENU:
 	case V4L2_CTRL_TYPE_INTEGER_MENU:
-		if (val < ctrl->minimum || val > ctrl->maximum)
+		if (c->value < ctrl->minimum || c->value > ctrl->maximum)
 			return -ERANGE;
-		if (ctrl->menu_skip_mask & (1 << val))
+		if (ctrl->menu_skip_mask & (1 << c->value))
 			return -EINVAL;
 		if (ctrl->type == V4L2_CTRL_TYPE_MENU &&
-		    ctrl->qmenu[val][0] == '\0')
+		    ctrl->qmenu[c->value][0] == '\0')
 			return -EINVAL;
 		return 0;
 
 	case V4L2_CTRL_TYPE_BITMASK:
-		*pval &= ctrl->maximum;
+		c->value &= ctrl->maximum;
 		return 0;
 
 	case V4L2_CTRL_TYPE_BUTTON:
 	case V4L2_CTRL_TYPE_CTRL_CLASS:
-		*pval = 0;
+		c->value = 0;
 		return 0;
 
-	default:
-		return -EINVAL;
-	}
-}
-
-/* Validate a new control */
-static int validate_new(const struct v4l2_ctrl *ctrl, struct v4l2_ext_control *c)
-{
-	char *s = c->string;
-	size_t len;
-
-	switch (ctrl->type) {
-	case V4L2_CTRL_TYPE_INTEGER:
-	case V4L2_CTRL_TYPE_BOOLEAN:
-	case V4L2_CTRL_TYPE_MENU:
-	case V4L2_CTRL_TYPE_INTEGER_MENU:
-	case V4L2_CTRL_TYPE_BITMASK:
-	case V4L2_CTRL_TYPE_BUTTON:
-	case V4L2_CTRL_TYPE_CTRL_CLASS:
-		return validate_new_int(ctrl, &c->value);
-
 	case V4L2_CTRL_TYPE_INTEGER64:
 		return 0;
 
 	case V4L2_CTRL_TYPE_STRING:
-		len = strlen(s);
+		len = strlen(c->string);
 		if (len < ctrl->minimum)
 			return -ERANGE;
 		if ((len - ctrl->minimum) % ctrl->step)
@@ -2234,12 +2211,19 @@ int v4l2_subdev_g_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs
 EXPORT_SYMBOL(v4l2_subdev_g_ext_ctrls);
 
 /* Helper function to get a single control */
-static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
+static int get_ctrl(struct v4l2_ctrl *ctrl, struct v4l2_ext_control *c)
 {
 	struct v4l2_ctrl *master = ctrl->cluster[0];
 	int ret = 0;
 	int i;
 
+	/* String controls are not supported. The new_to_user() and
+	 * cur_to_user() calls below would need to be modified not to access
+	 * userspace memory when called from get_ctrl().
+	 */
+	if (ctrl->type == V4L2_CTRL_TYPE_STRING)
+		return -EINVAL;
+
 	if (ctrl->flags & V4L2_CTRL_FLAG_WRITE_ONLY)
 		return -EACCES;
 
@@ -2249,9 +2233,9 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
 		for (i = 0; i < master->ncontrols; i++)
 			cur_to_new(master->cluster[i]);
 		ret = call_op(master, g_volatile_ctrl);
-		*val = ctrl->val;
+		new_to_user(c, ctrl);
 	} else {
-		*val = ctrl->cur.val;
+		cur_to_user(c, ctrl);
 	}
 	v4l2_ctrl_unlock(master);
 	return ret;
@@ -2260,10 +2244,14 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
 int v4l2_g_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *control)
 {
 	struct v4l2_ctrl *ctrl = v4l2_ctrl_find(hdl, control->id);
+	struct v4l2_ext_control c;
+	int ret;
 
 	if (ctrl == NULL || !type_is_int(ctrl))
 		return -EINVAL;
-	return get_ctrl(ctrl, &control->value);
+	ret = get_ctrl(ctrl, &c);
+	control->value = c.value;
+	return ret;
 }
 EXPORT_SYMBOL(v4l2_g_ctrl);
 
@@ -2275,15 +2263,28 @@ EXPORT_SYMBOL(v4l2_subdev_g_ctrl);
 
 s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl)
 {
-	s32 val = 0;
+	struct v4l2_ext_control c;
 
 	/* It's a driver bug if this happens. */
 	WARN_ON(!type_is_int(ctrl));
-	get_ctrl(ctrl, &val);
-	return val;
+	c.value = 0;
+	get_ctrl(ctrl, &c);
+	return c.value;
 }
 EXPORT_SYMBOL(v4l2_ctrl_g_ctrl);
 
+s64 v4l2_ctrl_g_ctrl_int64(struct v4l2_ctrl *ctrl)
+{
+	struct v4l2_ext_control c;
+
+	/* It's a driver bug if this happens. */
+	WARN_ON(ctrl->type != V4L2_CTRL_TYPE_INTEGER64);
+	c.value = 0;
+	get_ctrl(ctrl, &c);
+	return c.value;
+}
+EXPORT_SYMBOL(v4l2_ctrl_g_ctrl_int64);
+
 
 /* Core function that calls try/s_ctrl and ensures that the new value is
    copied to the current value on a set.
@@ -2499,13 +2500,21 @@ int v4l2_subdev_s_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs
 EXPORT_SYMBOL(v4l2_subdev_s_ext_ctrls);
 
 /* Helper function for VIDIOC_S_CTRL compatibility */
-static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
+static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
+		    struct v4l2_ext_control *c)
 {
 	struct v4l2_ctrl *master = ctrl->cluster[0];
 	int ret;
 	int i;
 
-	ret = validate_new_int(ctrl, val);
+	/* String controls are not supported. The user_to_new() and
+	 * cur_to_user() calls below would need to be modified not to access
+	 * userspace memory when called from set_ctrl().
+	 */
+	if (ctrl->type == V4L2_CTRL_TYPE_STRING)
+		return -EINVAL;
+
+	ret = validate_new(ctrl, c);
 	if (ret)
 		return ret;
 
@@ -2520,12 +2529,13 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
 	   manual mode we have to update the current volatile values since
 	   those will become the initial manual values after such a switch. */
 	if (master->is_auto && master->has_volatiles && ctrl == master &&
-	    !is_cur_manual(master) && *val == master->manual_mode_value)
+	    !is_cur_manual(master) && c->value == master->manual_mode_value)
 		update_from_auto_cluster(master);
-	ctrl->val = *val;
-	ctrl->is_new = 1;
+
+	user_to_new(c, ctrl);
 	ret = try_or_set_cluster(fh, master, true);
-	*val = ctrl->cur.val;
+	cur_to_user(c, ctrl);
+
 	v4l2_ctrl_unlock(ctrl);
 	return ret;
 }
@@ -2534,6 +2544,8 @@ int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 					struct v4l2_control *control)
 {
 	struct v4l2_ctrl *ctrl = v4l2_ctrl_find(hdl, control->id);
+	struct v4l2_ext_control c;
+	int ret;
 
 	if (ctrl == NULL || !type_is_int(ctrl))
 		return -EINVAL;
@@ -2541,7 +2553,10 @@ int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 	if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
 		return -EACCES;
 
-	return set_ctrl(fh, ctrl, &control->value);
+	c.value = control->value;
+	ret = set_ctrl(fh, ctrl, &c);
+	control->value = c.value;
+	return ret;
 }
 EXPORT_SYMBOL(v4l2_s_ctrl);
 
@@ -2553,12 +2568,26 @@ EXPORT_SYMBOL(v4l2_subdev_s_ctrl);
 
 int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
 {
+	struct v4l2_ext_control c;
+
 	/* It's a driver bug if this happens. */
 	WARN_ON(!type_is_int(ctrl));
-	return set_ctrl(NULL, ctrl, &val);
+	c.value = val;
+	return set_ctrl(NULL, ctrl, &c);
 }
 EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
 
+int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl, s64 val)
+{
+	struct v4l2_ext_control c;
+
+	/* It's a driver bug if this happens. */
+	WARN_ON(ctrl->type != V4L2_CTRL_TYPE_INTEGER64);
+	c.value64 = val;
+	return set_ctrl(NULL, ctrl, &c);
+}
+EXPORT_SYMBOL(v4l2_ctrl_s_ctrl_int64);
+
 static int v4l2_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
 {
 	struct v4l2_ctrl *ctrl = v4l2_ctrl_find(sev->fh->ctrl_handler, sev->id);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 776605f..7ef6b27 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -511,6 +511,29 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
   */
 int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
 
+/** v4l2_ctrl_g_ctrl_int64() - Helper function to get a 64-bit control's value from within a driver.
+  * @ctrl:	The control.
+  *
+  * This returns the control's value safely by going through the control
+  * framework. This function will lock the control's handler, so it cannot be
+  * used from within the &v4l2_ctrl_ops functions.
+  *
+  * This function is for 64-bit integer type controls only.
+  */
+s64 v4l2_ctrl_g_ctrl_int64(struct v4l2_ctrl *ctrl);
+
+/** v4l2_ctrl_s_ctrl_int64() - Helper function to set a 64-bit control's value from within a driver.
+  * @ctrl:	The control.
+  * @val:	The new value.
+  *
+  * This set the control's new value safely by going through the control
+  * framework. This function will lock the control's handler, so it cannot be
+  * used from within the &v4l2_ctrl_ops functions.
+  *
+  * This function is for 64-bit integer type controls only.
+  */
+int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl, s64 val);
+
 /* Internal helper functions that deal with control events. */
 extern const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops;
 void v4l2_ctrl_replace(struct v4l2_event *old, const struct v4l2_event *new);
-- 
1.7.8.6

[PATCH 2/4] mt9v032: Provide pixel rate control

[Laurent Pinchart]

From: Sakari Ailus <sakari.ailus@iki.fi>

Provide pixel rate control calculated from external clock and horizontal
binning factor.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/mt9v032.c |   27 +++++++++++++++++++++++++--
 1 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/mt9v032.c b/drivers/media/video/mt9v032.c
index 4ba4884..2203a6f 100644
--- a/drivers/media/video/mt9v032.c
+++ b/drivers/media/video/mt9v032.c
@@ -122,6 +122,7 @@ struct mt9v032 {
 	struct v4l2_mbus_framefmt format;
 	struct v4l2_rect crop;
 
+	struct v4l2_ctrl *pixel_rate;
 	struct v4l2_ctrl_handler ctrls;
 
 	struct mutex power_lock;
@@ -187,13 +188,15 @@ mt9v032_update_aec_agc(struct mt9v032 *mt9v032, u16 which, int enable)
 	return 0;
 }
 
+#define EXT_CLK		25000000
+
 static int mt9v032_power_on(struct mt9v032 *mt9v032)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
 	int ret;
 
 	if (mt9v032->pdata->set_clock) {
-		mt9v032->pdata->set_clock(&mt9v032->subdev, 25000000);
+		mt9v032->pdata->set_clock(&mt9v032->subdev, EXT_CLK);
 		udelay(1);
 	}
 
@@ -365,6 +368,17 @@ static int mt9v032_get_format(struct v4l2_subdev *subdev,
 	return 0;
 }
 
+static void mt9v032_configure_pixel_rate(struct mt9v032 *mt9v032,
+					 unsigned int hratio)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
+	int ret;
+
+	ret = v4l2_ctrl_s_ctrl_int64(mt9v032->pixel_rate, EXT_CLK / hratio);
+	if (ret < 0)
+		dev_warn(&client->dev, "failed to set pixel rate (%d)\n", ret);
+}
+
 static int mt9v032_set_format(struct v4l2_subdev *subdev,
 			      struct v4l2_subdev_fh *fh,
 			      struct v4l2_subdev_format *format)
@@ -395,6 +409,8 @@ static int mt9v032_set_format(struct v4l2_subdev *subdev,
 					    format->which);
 	__format->width = __crop->width / hratio;
 	__format->height = __crop->height / vratio;
+	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		mt9v032_configure_pixel_rate(mt9v032, hratio);
 
 	format->format = *__format;
 
@@ -450,6 +466,8 @@ static int mt9v032_set_crop(struct v4l2_subdev *subdev,
 						    crop->which);
 		__format->width = rect.width;
 		__format->height = rect.height;
+		if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+			mt9v032_configure_pixel_rate(mt9v032, 1);
 	}
 
 	*__crop = rect;
@@ -598,6 +616,8 @@ static int mt9v032_registered(struct v4l2_subdev *subdev)
 	dev_info(&client->dev, "MT9V032 detected at address 0x%02x\n",
 			client->addr);
 
+	mt9v032_configure_pixel_rate(mt9v032, 1);
+
 	return ret;
 }
 
@@ -681,7 +701,7 @@ static int mt9v032_probe(struct i2c_client *client,
 	mutex_init(&mt9v032->power_lock);
 	mt9v032->pdata = client->dev.platform_data;
 
-	v4l2_ctrl_handler_init(&mt9v032->ctrls, ARRAY_SIZE(mt9v032_ctrls) + 4);
+	v4l2_ctrl_handler_init(&mt9v032->ctrls, ARRAY_SIZE(mt9v032_ctrls) + 5);
 
 	v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
 			  V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
@@ -695,6 +715,9 @@ static int mt9v032_probe(struct i2c_client *client,
 			  V4L2_CID_EXPOSURE, MT9V032_TOTAL_SHUTTER_WIDTH_MIN,
 			  MT9V032_TOTAL_SHUTTER_WIDTH_MAX, 1,
 			  MT9V032_TOTAL_SHUTTER_WIDTH_DEF);
+	mt9v032->pixel_rate =
+		v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
+				  V4L2_CID_PIXEL_RATE, 0, 0, 1, 0);
 
 	for (i = 0; i < ARRAY_SIZE(mt9v032_ctrls); ++i)
 		v4l2_ctrl_new_custom(&mt9v032->ctrls, &mt9v032_ctrls[i], NULL);
-- 
1.7.8.6

[PATCH 3/4] mt9v032: Export horizontal and vertical blanking as V4L2 controls

[Laurent Pinchart]

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/mt9v032.c |   34 +++++++++++++++++++++++++++++++---
 1 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/mt9v032.c b/drivers/media/video/mt9v032.c
index 2203a6f..df55044 100644
--- a/drivers/media/video/mt9v032.c
+++ b/drivers/media/video/mt9v032.c
@@ -50,10 +50,19 @@
 #define		MT9V032_WINDOW_WIDTH_MAX		752
 #define MT9V032_HORIZONTAL_BLANKING			0x05
 #define		MT9V032_HORIZONTAL_BLANKING_MIN		43
+#define		MT9V032_HORIZONTAL_BLANKING_DEF		94
 #define		MT9V032_HORIZONTAL_BLANKING_MAX		1023
 #define MT9V032_VERTICAL_BLANKING			0x06
 #define		MT9V032_VERTICAL_BLANKING_MIN		4
-#define		MT9V032_VERTICAL_BLANKING_MAX		3000
+#define		MT9V032_VERTICAL_BLANKING_DEF		45
+/* The vertical blanking maximum value is 3000 rows according to the datasheet,
+ * and the sensor is supposed to automatically extend vertical blanking
+ * internally when the exposure time exceeds the total number of lines. However,
+ * experience showed that the vertical blanking is not automatically extended,
+ * and that the vertical blanking registers supports values up to at least 32767
+ * lines.
+ */
+#define		MT9V032_VERTICAL_BLANKING_MAX		32767
 #define MT9V032_CHIP_CONTROL				0x07
 #define		MT9V032_CHIP_CONTROL_MASTER_MODE	(1 << 3)
 #define		MT9V032_CHIP_CONTROL_DOUT_ENABLE	(1 << 7)
@@ -131,6 +140,7 @@ struct mt9v032 {
 	struct mt9v032_platform_data *pdata;
 	u16 chip_control;
 	u16 aec_agc;
+	u16 hblank;
 };
 
 static struct mt9v032 *to_mt9v032(struct v4l2_subdev *sd)
@@ -323,7 +333,7 @@ static int mt9v032_s_stream(struct v4l2_subdev *subdev, int enable)
 		return ret;
 
 	ret = mt9v032_write(client, MT9V032_HORIZONTAL_BLANKING,
-			    max(43, 660 - crop->width));
+			    max_t(s32, mt9v032->hblank, 660 - crop->width));
 	if (ret < 0)
 		return ret;
 
@@ -505,6 +515,15 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
 		return mt9v032_write(client, MT9V032_TOTAL_SHUTTER_WIDTH,
 				     ctrl->val);
 
+	case V4L2_CID_HBLANK:
+		mt9v032->hblank = ctrl->val;
+		return mt9v032_write(client, MT9V032_HORIZONTAL_BLANKING,
+				     ctrl->val);
+
+	case V4L2_CID_VBLANK:
+		return mt9v032_write(client, MT9V032_VERTICAL_BLANKING,
+				     ctrl->val);
+
 	case V4L2_CID_TEST_PATTERN:
 		switch (ctrl->val) {
 		case 0:
@@ -701,7 +720,7 @@ static int mt9v032_probe(struct i2c_client *client,
 	mutex_init(&mt9v032->power_lock);
 	mt9v032->pdata = client->dev.platform_data;
 
-	v4l2_ctrl_handler_init(&mt9v032->ctrls, ARRAY_SIZE(mt9v032_ctrls) + 5);
+	v4l2_ctrl_handler_init(&mt9v032->ctrls, ARRAY_SIZE(mt9v032_ctrls) + 7);
 
 	v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
 			  V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
@@ -715,6 +734,14 @@ static int mt9v032_probe(struct i2c_client *client,
 			  V4L2_CID_EXPOSURE, MT9V032_TOTAL_SHUTTER_WIDTH_MIN,
 			  MT9V032_TOTAL_SHUTTER_WIDTH_MAX, 1,
 			  MT9V032_TOTAL_SHUTTER_WIDTH_DEF);
+	v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
+			  V4L2_CID_HBLANK, MT9V032_HORIZONTAL_BLANKING_MIN,
+			  MT9V032_HORIZONTAL_BLANKING_MAX, 1,
+			  MT9V032_HORIZONTAL_BLANKING_DEF);
+	v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
+			  V4L2_CID_VBLANK, MT9V032_VERTICAL_BLANKING_MIN,
+			  MT9V032_VERTICAL_BLANKING_MAX, 1,
+			  MT9V032_VERTICAL_BLANKING_DEF);
 	mt9v032->pixel_rate =
 		v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
 				  V4L2_CID_PIXEL_RATE, 0, 0, 1, 0);
@@ -740,6 +767,7 @@ static int mt9v032_probe(struct i2c_client *client,
 	mt9v032->format.colorspace = V4L2_COLORSPACE_SRGB;
 
 	mt9v032->aec_agc = MT9V032_AEC_ENABLE | MT9V032_AGC_ENABLE;
+	mt9v032->hblank = MT9V032_HORIZONTAL_BLANKING_DEF;
 
 	v4l2_i2c_subdev_init(&mt9v032->subdev, client, &mt9v032_subdev_ops);
 	mt9v032->subdev.internal_ops = &mt9v032_subdev_internal_ops;
-- 
1.7.8.6

[PATCH 4/4] mt9p031: Fix horizontal and vertical blanking configuration

[Laurent Pinchart]

Compute the horizontal blanking value according to the datasheet. The
value written to the hblank and vblank registers must be equal to the
number of blank columns and rows minus one.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/mt9p031.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c
index 3be537e..2c0f407 100644
--- a/drivers/media/video/mt9p031.c
+++ b/drivers/media/video/mt9p031.c
@@ -55,9 +55,9 @@
 #define		MT9P031_HORIZONTAL_BLANK_MIN		0
 #define		MT9P031_HORIZONTAL_BLANK_MAX		4095
 #define MT9P031_VERTICAL_BLANK				0x06
-#define		MT9P031_VERTICAL_BLANK_MIN		0
-#define		MT9P031_VERTICAL_BLANK_MAX		4095
-#define		MT9P031_VERTICAL_BLANK_DEF		25
+#define		MT9P031_VERTICAL_BLANK_MIN		1
+#define		MT9P031_VERTICAL_BLANK_MAX		4096
+#define		MT9P031_VERTICAL_BLANK_DEF		26
 #define MT9P031_OUTPUT_CONTROL				0x07
 #define		MT9P031_OUTPUT_CONTROL_CEN		2
 #define		MT9P031_OUTPUT_CONTROL_SYN		1
@@ -368,13 +368,13 @@ static int mt9p031_set_params(struct mt9p031 *mt9p031)
 	/* Blanking - use minimum value for horizontal blanking and default
 	 * value for vertical blanking.
 	 */
-	hblank = 346 * ybin + 64 + (80 >> max_t(unsigned int, xbin, 3));
+	hblank = 346 * ybin + 64 + (80 >> min_t(unsigned int, xbin, 3));
 	vblank = MT9P031_VERTICAL_BLANK_DEF;
 
-	ret = mt9p031_write(client, MT9P031_HORIZONTAL_BLANK, hblank);
+	ret = mt9p031_write(client, MT9P031_HORIZONTAL_BLANK, hblank - 1);
 	if (ret < 0)
 		return ret;
-	ret = mt9p031_write(client, MT9P031_VERTICAL_BLANK, vblank);
+	ret = mt9p031_write(client, MT9P031_VERTICAL_BLANK, vblank - 1);
 	if (ret < 0)
 		return ret;
 
-- 
1.7.8.6

[PATCH v2] mt9v032: Export horizontal and vertical blanking as V4L2 controls

[Laurent Pinchart]

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/mt9v032.c |   36 +++++++++++++++++++++++++++++++++---
 1 files changed, 33 insertions(+), 3 deletions(-)

Changes since v1:

- Make sure the total horizontal time will not go below 660 when setting the
  horizontal blanking control
- Restrict the vertical blanking value to 3000 as documented in the datasheet.
  Increasing the exposure time actually extends vertical blanking, as long as
  the user doesn't forget to turn auto-exposure off...

diff --git a/drivers/media/video/mt9v032.c b/drivers/media/video/mt9v032.c
index 2203a6f..e713ad9 100644
--- a/drivers/media/video/mt9v032.c
+++ b/drivers/media/video/mt9v032.c
@@ -50,9 +50,11 @@
 #define		MT9V032_WINDOW_WIDTH_MAX		752
 #define MT9V032_HORIZONTAL_BLANKING			0x05
 #define		MT9V032_HORIZONTAL_BLANKING_MIN		43
+#define		MT9V032_HORIZONTAL_BLANKING_DEF		94
 #define		MT9V032_HORIZONTAL_BLANKING_MAX		1023
 #define MT9V032_VERTICAL_BLANKING			0x06
 #define		MT9V032_VERTICAL_BLANKING_MIN		4
+#define		MT9V032_VERTICAL_BLANKING_DEF		45
 #define		MT9V032_VERTICAL_BLANKING_MAX		3000
 #define MT9V032_CHIP_CONTROL				0x07
 #define		MT9V032_CHIP_CONTROL_MASTER_MODE	(1 << 3)
@@ -129,8 +131,10 @@ struct mt9v032 {
 	int power_count;
 
 	struct mt9v032_platform_data *pdata;
+
 	u16 chip_control;
 	u16 aec_agc;
+	u16 hblank;
 };
 
 static struct mt9v032 *to_mt9v032(struct v4l2_subdev *sd)
@@ -188,6 +192,16 @@ mt9v032_update_aec_agc(struct mt9v032 *mt9v032, u16 which, int enable)
 	return 0;
 }
 
+static int
+mt9v032_update_hblank(struct mt9v032 *mt9v032)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
+	struct v4l2_rect *crop = &mt9v032->crop;
+
+	return mt9v032_write(client, MT9V032_HORIZONTAL_BLANKING,
+			     max_t(s32, mt9v032->hblank, 660 - crop->width));
+}
+
 #define EXT_CLK		25000000
 
 static int mt9v032_power_on(struct mt9v032 *mt9v032)
@@ -322,8 +336,7 @@ static int mt9v032_s_stream(struct v4l2_subdev *subdev, int enable)
 	if (ret < 0)
 		return ret;
 
-	ret = mt9v032_write(client, MT9V032_HORIZONTAL_BLANKING,
-			    max(43, 660 - crop->width));
+	ret = mt9v032_update_hblank(mt9v032);
 	if (ret < 0)
 		return ret;
 
@@ -505,6 +518,14 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
 		return mt9v032_write(client, MT9V032_TOTAL_SHUTTER_WIDTH,
 				     ctrl->val);
 
+	case V4L2_CID_HBLANK:
+		mt9v032->hblank = ctrl->val;
+		return mt9v032_update_hblank(mt9v032);
+
+	case V4L2_CID_VBLANK:
+		return mt9v032_write(client, MT9V032_VERTICAL_BLANKING,
+				     ctrl->val);
+
 	case V4L2_CID_TEST_PATTERN:
 		switch (ctrl->val) {
 		case 0:
@@ -701,7 +722,7 @@ static int mt9v032_probe(struct i2c_client *client,
 	mutex_init(&mt9v032->power_lock);
 	mt9v032->pdata = client->dev.platform_data;
 
-	v4l2_ctrl_handler_init(&mt9v032->ctrls, ARRAY_SIZE(mt9v032_ctrls) + 5);
+	v4l2_ctrl_handler_init(&mt9v032->ctrls, ARRAY_SIZE(mt9v032_ctrls) + 7);
 
 	v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
 			  V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
@@ -715,6 +736,14 @@ static int mt9v032_probe(struct i2c_client *client,
 			  V4L2_CID_EXPOSURE, MT9V032_TOTAL_SHUTTER_WIDTH_MIN,
 			  MT9V032_TOTAL_SHUTTER_WIDTH_MAX, 1,
 			  MT9V032_TOTAL_SHUTTER_WIDTH_DEF);
+	v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
+			  V4L2_CID_HBLANK, MT9V032_HORIZONTAL_BLANKING_MIN,
+			  MT9V032_HORIZONTAL_BLANKING_MAX, 1,
+			  MT9V032_HORIZONTAL_BLANKING_DEF);
+	v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
+			  V4L2_CID_VBLANK, MT9V032_VERTICAL_BLANKING_MIN,
+			  MT9V032_VERTICAL_BLANKING_MAX, 1,
+			  MT9V032_VERTICAL_BLANKING_DEF);
 	mt9v032->pixel_rate =
 		v4l2_ctrl_new_std(&mt9v032->ctrls, &mt9v032_ctrl_ops,
 				  V4L2_CID_PIXEL_RATE, 0, 0, 1, 0);
@@ -740,6 +769,7 @@ static int mt9v032_probe(struct i2c_client *client,
 	mt9v032->format.colorspace = V4L2_COLORSPACE_SRGB;
 
 	mt9v032->aec_agc = MT9V032_AEC_ENABLE | MT9V032_AGC_ENABLE;
+	mt9v032->hblank = MT9V032_HORIZONTAL_BLANKING_DEF;
 
 	v4l2_i2c_subdev_init(&mt9v032->subdev, client, &mt9v032_subdev_ops);
 	mt9v032->subdev.internal_ops = &mt9v032_subdev_internal_ops;
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] mt9v032: Export horizontal and vertical blanking as V4L2 controls

[Sakari Ailus]

Hi Laurent,

Thanks for the patch.

On Tue, Jul 24, 2012 at 01:10:42AM +0200, Laurent Pinchart wrote:
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/video/mt9v032.c |   36 +++++++++++++++++++++++++++++++++---
>  1 files changed, 33 insertions(+), 3 deletions(-)
> 
> Changes since v1:
> 
> - Make sure the total horizontal time will not go below 660 when setting the
>   horizontal blanking control
> - Restrict the vertical blanking value to 3000 as documented in the datasheet.
>   Increasing the exposure time actually extends vertical blanking, as long as
>   the user doesn't forget to turn auto-exposure off...

Does binning either horizontally or vertically affect the blanking limits?
If the process is analogue then the answer is typically "yes".

It's not directly related to this patch, but the effect of the driver just
exposing one sub-device really shows better now. Besides lacking the way to
specify binning as in the V4L2 subdev API (compose selection target), the
user also can't use the crop bounds selection target to get the size of the
pixel array.

We could either accept this for the time being and fix it later on of fix it
now.

I prefer fixing it right now but admit that this patch isn't breaking
anything, it rather is missing quite relevant functionality to control the
sensor in a generic way.

Kind regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [PATCH 0/4] Aptinate sensors patches

[Sakari Ailus]

On Mon, Jul 23, 2012 at 08:34:58PM +0200, Laurent Pinchart wrote:
> Hi everybody,
> 
> Here are three fixes/patches for the MT9P031 and MT9V032 sensor drivers. The
> second patch (mt9v032 pixel rate control) requires a control framework
> modification (1/4) that has already been reviewed.
> 
> Sakari, I've rebased your patch on top of the latest media tree, could you
> please review it ?

For patches 1, 2 and 4:

Acked-by: Sakari Ailus <sakari.ailus@iki.fi>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [PATCH v2] mt9v032: Export horizontal and vertical blanking as V4L2 controls

[Laurent Pinchart]

Hi Sakari,

On Thursday 26 July 2012 23:54:01 Sakari Ailus wrote:
> On Tue, Jul 24, 2012 at 01:10:42AM +0200, Laurent Pinchart wrote:
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/media/video/mt9v032.c |   36 +++++++++++++++++++++++++++++++++---
> >  1 files changed, 33 insertions(+), 3 deletions(-)
> > 
> > Changes since v1:
> > 
> > - Make sure the total horizontal time will not go below 660 when setting
> >   the horizontal blanking control
> > 
> > - Restrict the vertical blanking value to 3000 as documented in the
> >   datasheet. Increasing the exposure time actually extends vertical
> >   blanking, as long as the user doesn't forget to turn auto-exposure
> >   off...
> 
> Does binning either horizontally or vertically affect the blanking limits?
> If the process is analogue then the answer is typically "yes".

The datasheet doesn't specify whether binning and blanking can influence each 
other.

> It's not directly related to this patch, but the effect of the driver just
> exposing one sub-device really shows better now. Besides lacking the way to
> specify binning as in the V4L2 subdev API (compose selection target), the
> user also can't use the crop bounds selection target to get the size of the
> pixel array.
> 
> We could either accept this for the time being and fix it later on of fix it
> now.
> 
> I prefer fixing it right now but admit that this patch isn't breaking
> anything, it rather is missing quite relevant functionality to control the
> sensor in a generic way.

I'd rather apply this patch first, as it doesn't break anything :-) Fixing the 
problem will require discussions, and that will take time.

Binning/skipping is a pretty common feature in sensors. Exposing two sub-
devices like the SMIA++ driver does is one way to fix the problem, but do we 
really want to do that for the vast majority of sensors ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] mt9v032: Export horizontal and vertical blanking as V4L2 controls

[Sakari Ailus]

Hi Laurent,

On Fri, Jul 27, 2012 at 01:02:04AM +0200, Laurent Pinchart wrote:
> On Thursday 26 July 2012 23:54:01 Sakari Ailus wrote:
> > On Tue, Jul 24, 2012 at 01:10:42AM +0200, Laurent Pinchart wrote:
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > >  drivers/media/video/mt9v032.c |   36 +++++++++++++++++++++++++++++++++---
> > >  1 files changed, 33 insertions(+), 3 deletions(-)
> > > 
> > > Changes since v1:
> > > 
> > > - Make sure the total horizontal time will not go below 660 when setting
> > >   the horizontal blanking control
> > > 
> > > - Restrict the vertical blanking value to 3000 as documented in the
> > >   datasheet. Increasing the exposure time actually extends vertical
> > >   blanking, as long as the user doesn't forget to turn auto-exposure
> > >   off...
> > 
> > Does binning either horizontally or vertically affect the blanking limits?
> > If the process is analogue then the answer is typically "yes".
> 
> The datasheet doesn't specify whether binning and blanking can influence each 
> other.

Vertical binning is often analogue since digital binning would require as
much temporary memory as the row holds pixels. This means the hardware
already does binning before a/d conversion and there's only need to actually
read half the number of rows, hence the effect on frame length.

> > It's not directly related to this patch, but the effect of the driver just
> > exposing one sub-device really shows better now. Besides lacking the way to
> > specify binning as in the V4L2 subdev API (compose selection target), the
> > user also can't use the crop bounds selection target to get the size of the
> > pixel array.
> > 
> > We could either accept this for the time being and fix it later on of fix it
> > now.
> > 
> > I prefer fixing it right now but admit that this patch isn't breaking
> > anything, it rather is missing quite relevant functionality to control the
> > sensor in a generic way.
> 
> I'd rather apply this patch first, as it doesn't break anything :-) Fixing the 
> problem will require discussions, and that will take time.

How so? There's nothing special in this sensor as far as I can see.

> Binning/skipping is a pretty common feature in sensors. Exposing two sub-
> devices like the SMIA++ driver does is one way to fix the problem, but do we 
> really want to do that for the vast majority of sensors ?

If we want to expose the sensor's functionality using the V4L2 subdev API
and not a sensor specific API, the answer is "yes". The bottom line is that
we have just one API to configure scaling and cropping and that API
(selections) is the same independently of where the operation is being
performed; whether it's the sensor or the ISP.

Kind regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [PATCH v2] mt9v032: Export horizontal and vertical blanking as V4L2 controls

[Laurent Pinchart]

Hi Sakari,

On Saturday 28 July 2012 00:27:23 Sakari Ailus wrote:
> On Fri, Jul 27, 2012 at 01:02:04AM +0200, Laurent Pinchart wrote:
> > On Thursday 26 July 2012 23:54:01 Sakari Ailus wrote:
> > > On Tue, Jul 24, 2012 at 01:10:42AM +0200, Laurent Pinchart wrote:
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > > 
> > > >  drivers/media/video/mt9v032.c |   36  ++++++++++++++++++++++++++++---
> > > >  1 files changed, 33 insertions(+), 3 deletions(-)
> > > > 
> > > > Changes since v1:
> > > > 
> > > > - Make sure the total horizontal time will not go below 660 when
> > > >   setting the horizontal blanking control
> > > > 
> > > > - Restrict the vertical blanking value to 3000 as documented in the
> > > >   datasheet. Increasing the exposure time actually extends vertical
> > > >   blanking, as long as the user doesn't forget to turn auto-exposure
> > > >   off...
> > > 
> > > Does binning either horizontally or vertically affect the blanking
> > > limits? If the process is analogue then the answer is typically "yes".
> > 
> > The datasheet doesn't specify whether binning and blanking can influence
> > each other.
> 
> Vertical binning is often analogue since digital binning would require as
> much temporary memory as the row holds pixels. This means the hardware
> already does binning before a/d conversion and there's only need to actually
> read half the number of rows, hence the effect on frame length.

That will affect the frame length, but why would it affect vertical blanking ?

> > > It's not directly related to this patch, but the effect of the driver
> > > just exposing one sub-device really shows better now. Besides lacking
> > > the way to specify binning as in the V4L2 subdev API (compose selection
> > > target), the user also can't use the crop bounds selection target to get
> > > the size of the pixel array.
> > > 
> > > We could either accept this for the time being and fix it later on of
> > > fix it now.
> > > 
> > > I prefer fixing it right now but admit that this patch isn't breaking
> > > anything, it rather is missing quite relevant functionality to control
> > > the sensor in a generic way.
> > 
> > I'd rather apply this patch first, as it doesn't break anything :-) Fixing
> > the problem will require discussions, and that will take time.
> 
> How so? There's nothing special in this sensor as far as I can see.
> 
> > Binning/skipping is a pretty common feature in sensors. Exposing two sub-
> > devices like the SMIA++ driver does is one way to fix the problem, but do
> > we really want to do that for the vast majority of sensors ?
> 
> If we want to expose the sensor's functionality using the V4L2 subdev API
> and not a sensor specific API, the answer is "yes". The bottom line is that
> we have just one API to configure scaling and cropping and that API
> (selections) is the same independently of where the operation is being
> performed; whether it's the sensor or the ISP.

If we want to expose two subdevices for every sensor we will need to get both 
kernelspace (ISP drivers) and userspace ready for this. I'm not against the 
idea, but we need to plan it properly.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] mt9v032: Export horizontal and vertical blanking as V4L2 controls

[Sakari Ailus]

Hi Laurent,

Laurent Pinchart wrote:
> On Saturday 28 July 2012 00:27:23 Sakari Ailus wrote:
>> On Fri, Jul 27, 2012 at 01:02:04AM +0200, Laurent Pinchart wrote:
>>> On Thursday 26 July 2012 23:54:01 Sakari Ailus wrote:
>>>> On Tue, Jul 24, 2012 at 01:10:42AM +0200, Laurent Pinchart wrote:
>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>> ---
>>>>>
>>>>>   drivers/media/video/mt9v032.c |   36  ++++++++++++++++++++++++++++---
>>>>>   1 files changed, 33 insertions(+), 3 deletions(-)
>>>>>
>>>>> Changes since v1:
>>>>>
>>>>> - Make sure the total horizontal time will not go below 660 when
>>>>>    setting the horizontal blanking control
>>>>>
>>>>> - Restrict the vertical blanking value to 3000 as documented in the
>>>>>    datasheet. Increasing the exposure time actually extends vertical
>>>>>    blanking, as long as the user doesn't forget to turn auto-exposure
>>>>>    off...
>>>>
>>>> Does binning either horizontally or vertically affect the blanking
>>>> limits? If the process is analogue then the answer is typically "yes".
>>>
>>> The datasheet doesn't specify whether binning and blanking can influence
>>> each other.
>>
>> Vertical binning is often analogue since digital binning would require as
>> much temporary memory as the row holds pixels. This means the hardware
>> already does binning before a/d conversion and there's only need to actually
>> read half the number of rows, hence the effect on frame length.
>
> That will affect the frame length, but why would it affect vertical blanking ?

Frame length == image height + vertical blanking.

The SMIA++ driver (at least) associates the blanking controls to the 
pixel array subdev. They might be more naturally placed to the source 
(either binner or scaler) but the width and height (to calculate the 
frame and line length) are related to the dimensions of the pixel array 
crop rectangle.

So when the binning configuration changes, that changes the limits for 
blanking and thus possibly also blanking itself.

>>>> It's not directly related to this patch, but the effect of the driver
>>>> just exposing one sub-device really shows better now. Besides lacking
>>>> the way to specify binning as in the V4L2 subdev API (compose selection
>>>> target), the user also can't use the crop bounds selection target to get
>>>> the size of the pixel array.
>>>>
>>>> We could either accept this for the time being and fix it later on of
>>>> fix it now.
>>>>
>>>> I prefer fixing it right now but admit that this patch isn't breaking
>>>> anything, it rather is missing quite relevant functionality to control
>>>> the sensor in a generic way.
>>>
>>> I'd rather apply this patch first, as it doesn't break anything :-) Fixing
>>> the problem will require discussions, and that will take time.
>>
>> How so? There's nothing special in this sensor as far as I can see.
>>
>>> Binning/skipping is a pretty common feature in sensors. Exposing two sub-
>>> devices like the SMIA++ driver does is one way to fix the problem, but do
>>> we really want to do that for the vast majority of sensors ?
>>
>> If we want to expose the sensor's functionality using the V4L2 subdev API
>> and not a sensor specific API, the answer is "yes". The bottom line is that
>> we have just one API to configure scaling and cropping and that API
>> (selections) is the same independently of where the operation is being
>> performed; whether it's the sensor or the ISP.
>
> If we want to expose two subdevices for every sensor we will need to get both
> kernelspace (ISP drivers) and userspace ready for this. I'm not against the
> idea, but we need to plan it properly.

I fully agree that this has to be planned properly; e.g. libv4l support for
controlling low level devices required by this is completely missing right
now.

Cheers,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH v2] mt9v032: Export horizontal and vertical blanking as V4L2 controls

[Laurent Pinchart]

Hi Sakari,

On Monday 13 August 2012 17:18:20 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> > On Saturday 28 July 2012 00:27:23 Sakari Ailus wrote:
> >> On Fri, Jul 27, 2012 at 01:02:04AM +0200, Laurent Pinchart wrote:
> >>> On Thursday 26 July 2012 23:54:01 Sakari Ailus wrote:
> >>>> On Tue, Jul 24, 2012 at 01:10:42AM +0200, Laurent Pinchart wrote:
> >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>> ---
> >>>>> 
> >>>>>   drivers/media/video/mt9v032.c |   36 
> >>>>>   ++++++++++++++++++++++++++++---
> >>>>>   1 files changed, 33 insertions(+), 3 deletions(-)
> >>>>> 
> >>>>> Changes since v1:
> >>>>> 
> >>>>> - Make sure the total horizontal time will not go below 660 when
> >>>>>    setting the horizontal blanking control
> >>>>> 
> >>>>> - Restrict the vertical blanking value to 3000 as documented in the
> >>>>>    datasheet. Increasing the exposure time actually extends vertical
> >>>>>    blanking, as long as the user doesn't forget to turn auto-exposure
> >>>>>    off...
> >>>> 
> >>>> Does binning either horizontally or vertically affect the blanking
> >>>> limits? If the process is analogue then the answer is typically "yes".
> >>> 
> >>> The datasheet doesn't specify whether binning and blanking can influence
> >>> each other.
> >> 
> >> Vertical binning is often analogue since digital binning would require as
> >> much temporary memory as the row holds pixels. This means the hardware
> >> already does binning before a/d conversion and there's only need to
> >> actually read half the number of rows, hence the effect on frame length.
> > 
> > That will affect the frame length, but why would it affect vertical
> > blanking ?
>
> Frame length == image height + vertical blanking.
> 
> The SMIA++ driver (at least) associates the blanking controls to the
> pixel array subdev. They might be more naturally placed to the source
> (either binner or scaler) but the width and height (to calculate the
> frame and line length) are related to the dimensions of the pixel array
> crop rectangle.
> 
> So when the binning configuration changes, that changes the limits for
> blanking and thus possibly also blanking itself.

Do the blanking controls expose blanking after binning or before binning ? In 
the later case I don't see how binning would influence them.

> >>>> It's not directly related to this patch, but the effect of the driver
> >>>> just exposing one sub-device really shows better now. Besides lacking
> >>>> the way to specify binning as in the V4L2 subdev API (compose selection
> >>>> target), the user also can't use the crop bounds selection target to
> >>>> get the size of the pixel array.
> >>>> 
> >>>> We could either accept this for the time being and fix it later on of
> >>>> fix it now.
> >>>> 
> >>>> I prefer fixing it right now but admit that this patch isn't breaking
> >>>> anything, it rather is missing quite relevant functionality to control
> >>>> the sensor in a generic way.
> >>> 
> >>> I'd rather apply this patch first, as it doesn't break anything :-)
> >>> Fixing the problem will require discussions, and that will take time.
> >> 
> >> How so? There's nothing special in this sensor as far as I can see.
> >> 
> >>> Binning/skipping is a pretty common feature in sensors. Exposing two
> >>> sub-devices like the SMIA++ driver does is one way to fix the problem,
> >>> but do we really want to do that for the vast majority of sensors ?
> >> 
> >> If we want to expose the sensor's functionality using the V4L2 subdev API
> >> and not a sensor specific API, the answer is "yes". The bottom line is
> >> that we have just one API to configure scaling and cropping and that API
> >> (selections) is the same independently of where the operation is being
> >> performed; whether it's the sensor or the ISP.
> > 
> > If we want to expose two subdevices for every sensor we will need to get
> > both kernelspace (ISP drivers) and userspace ready for this. I'm not
> > against the idea, but we need to plan it properly.
> 
> I fully agree that this has to be planned properly; e.g. libv4l support for
> controlling low level devices required by this is completely missing right
> now.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] mt9v032: Export horizontal and vertical blanking as V4L2 controls

[Sakari Ailus]

Laurent Pinchart wrote:
> Hi Sakari,
>
> On Monday 13 August 2012 17:18:20 Sakari Ailus wrote:
>> Laurent Pinchart wrote:
>>> On Saturday 28 July 2012 00:27:23 Sakari Ailus wrote:
>>>> On Fri, Jul 27, 2012 at 01:02:04AM +0200, Laurent Pinchart wrote:
>>>>> On Thursday 26 July 2012 23:54:01 Sakari Ailus wrote:
>>>>>> On Tue, Jul 24, 2012 at 01:10:42AM +0200, Laurent Pinchart wrote:
>>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>>>> ---
>>>>>>>
>>>>>>>    drivers/media/video/mt9v032.c |   36
>>>>>>>    ++++++++++++++++++++++++++++---
>>>>>>>    1 files changed, 33 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> Changes since v1:
>>>>>>>
>>>>>>> - Make sure the total horizontal time will not go below 660 when
>>>>>>>     setting the horizontal blanking control
>>>>>>>
>>>>>>> - Restrict the vertical blanking value to 3000 as documented in the
>>>>>>>     datasheet. Increasing the exposure time actually extends vertical
>>>>>>>     blanking, as long as the user doesn't forget to turn auto-exposure
>>>>>>>     off...
>>>>>>
>>>>>> Does binning either horizontally or vertically affect the blanking
>>>>>> limits? If the process is analogue then the answer is typically "yes".
>>>>>
>>>>> The datasheet doesn't specify whether binning and blanking can influence
>>>>> each other.
>>>>
>>>> Vertical binning is often analogue since digital binning would require as
>>>> much temporary memory as the row holds pixels. This means the hardware
>>>> already does binning before a/d conversion and there's only need to
>>>> actually read half the number of rows, hence the effect on frame length.
>>>
>>> That will affect the frame length, but why would it affect vertical
>>> blanking ?
>>
>> Frame length == image height + vertical blanking.
>>
>> The SMIA++ driver (at least) associates the blanking controls to the
>> pixel array subdev. They might be more naturally placed to the source
>> (either binner or scaler) but the width and height (to calculate the
>> frame and line length) are related to the dimensions of the pixel array
>> crop rectangle.
>>
>> So when the binning configuration changes, that changes the limits for
>> blanking and thus possibly also blanking itself.
>
> Do the blanking controls expose blanking after binning or before binning ? In
> the later case I don't see how binning would influence them.

Some sensors control the blanking in pixel array directly whereas some, 
like the SMIA++, control the frame length in the source (scaler or 
binner) source instead.

So it is up to the sensor hardware --- I think it's still better to keep 
all the controls in a single subdev. Otherwise it'd be quite difficult 
for the user to figure out how to calculate the frame rate.

Kind regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi