[PATCH 0/4] Omnivision OV4689 cropping and binning

[PATCH 0/4] Omnivision OV4689 cropping and binning

[Mikhail Rudenko]

This small series consists of patches 17-20 from [1], which were
requested to merged separately later. No significant changes have been
made since and the R-b's are kept.

Patch 1 implements configurable analogue crop rectangle via
.set_selection callback. Patches 2 and 3 are refactorings and are not
supposed to introduce any functional change. Patch 4 enables 2x2
binning option.

[1] https://lore.kernel.org/all/20240402164552.19171-1-mike.rudenko@gmail.com/

Mikhail Rudenko (4):
  media: i2c: ov4689: Configurable analogue crop
  media: i2c: ov4689: Eliminate struct ov4689_mode
  media: i2c: ov4689: Refactor ov4689_s_stream
  media: i2c: ov4689: Implement 2x2 binning

 drivers/media/i2c/ov4689.c | 483 +++++++++++++++++++++++++------------
 1 file changed, 328 insertions(+), 155 deletions(-)

--
2.45.2

[PATCH 1/4] media: i2c: ov4689: Configurable analogue crop

[Mikhail Rudenko]

Implement configurable analogue crop via .set_selection call.
ov4689_init_cfg is modified to initialize default subdev selection.
Offsets are aligned to 2 to preserve Bayer order, selection width is
aligned to 4 and height to 2 to meet hardware requirements.

Experimentally discovered values of the cropping-related registers and
vfifo_read_start for various output sizes are used. Default BLC anchor
positions are used for the default analogue crop, scaling down
proportionally for the smaller crop sizes.

When analogue crop is adjusted, several consequential actions take
place: the output format is reset, exposure/vblank/hblank control
ranges and default values are adjusted accordingly. Additionally,
ov4689_set_ctrl utilizes pad crop instead of cur_mode width and
height for HTS and VTS calculation. Also, ov4689_enum_frame_sizes is
modified to report crop size as available frame size.

Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/ov4689.c | 276 ++++++++++++++++++++++++++++---------
 1 file changed, 212 insertions(+), 64 deletions(-)

diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c
index 1c3a449f9354..933d714aa26c 100644
--- a/drivers/media/i2c/ov4689.c
+++ b/drivers/media/i2c/ov4689.c
@@ -45,8 +45,12 @@
 #define OV4689_REG_V_CROP_START		CCI_REG16(0x3802)
 #define OV4689_REG_H_CROP_END		CCI_REG16(0x3804)
 #define OV4689_REG_V_CROP_END		CCI_REG16(0x3806)
+
 #define OV4689_REG_H_OUTPUT_SIZE	CCI_REG16(0x3808)
+#define OV4689_H_OUTPUT_SIZE_DEFAULT	2688
+
 #define OV4689_REG_V_OUTPUT_SIZE	CCI_REG16(0x380a)
+#define OV4689_V_OUTPUT_SIZE_DEFAULT	1520
 
 #define OV4689_REG_HTS			CCI_REG16(0x380c)
 #define OV4689_HTS_DIVIDER		4
@@ -96,6 +100,19 @@
 #define OV4689_DUMMY_ROWS		8	/* 8 dummy rows on each side */
 #define OV4689_DUMMY_COLUMNS		16	/* 16 dummy columns on each side */
 
+/*
+ * These values are not hardware limits, but rather the minimums that
+ * the driver has been tested to.
+ */
+#define OV4689_H_CROP_MIN		128
+#define OV4689_V_CROP_MIN		128
+
+/*
+ * Minimum working vertical blanking value. Found experimentally at
+ * minimum HTS values.
+ */
+#define OV4689_VBLANK_MIN		31
+
 static const char *const ov4689_supply_names[] = {
 	"avdd", /* Analog power */
 	"dovdd", /* Digital I/O power */
@@ -134,7 +151,7 @@ struct ov4689 {
 	u32 clock_rate;
 
 	struct v4l2_ctrl_handler ctrl_handler;
-	struct v4l2_ctrl *exposure;
+	struct v4l2_ctrl *exposure, *hblank, *vblank;
 
 	const struct ov4689_mode *cur_mode;
 };
@@ -320,24 +337,27 @@ static const struct ov4689_gain_range ov4689_gain_ranges[] = {
 	},
 };
 
-static void ov4689_fill_fmt(const struct ov4689_mode *mode,
-			    struct v4l2_mbus_framefmt *fmt)
-{
-	fmt->code = MEDIA_BUS_FMT_SBGGR10_1X10;
-	fmt->width = mode->width;
-	fmt->height = mode->height;
-	fmt->field = V4L2_FIELD_NONE;
-}
-
 static int ov4689_set_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_state *sd_state,
 			  struct v4l2_subdev_format *fmt)
 {
-	struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
-	struct ov4689 *ov4689 = to_ov4689(sd);
+	struct v4l2_mbus_framefmt *format;
+	struct v4l2_rect *crop;
+
+	crop = v4l2_subdev_state_get_crop(sd_state, fmt->pad);
+	format = v4l2_subdev_state_get_format(sd_state, fmt->pad);
+
+	format->width = crop->width;
+	format->height = crop->height;
 
-	/* only one mode supported for now */
-	ov4689_fill_fmt(ov4689->cur_mode, mbus_fmt);
+	format->code = MEDIA_BUS_FMT_SBGGR10_1X10;
+	format->field = V4L2_FIELD_NONE;
+	format->colorspace = V4L2_COLORSPACE_RAW;
+	format->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+	format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	format->xfer_func = V4L2_XFER_FUNC_NONE;
+
+	fmt->format = *format;
 
 	return 0;
 }
@@ -357,16 +377,20 @@ static int ov4689_enum_frame_sizes(struct v4l2_subdev *sd,
 				   struct v4l2_subdev_state *sd_state,
 				   struct v4l2_subdev_frame_size_enum *fse)
 {
-	if (fse->index >= ARRAY_SIZE(supported_modes))
+	const struct v4l2_rect *crop;
+
+	if (fse->index >= 1)
 		return -EINVAL;
 
 	if (fse->code != MEDIA_BUS_FMT_SBGGR10_1X10)
 		return -EINVAL;
 
-	fse->min_width = supported_modes[fse->index].width;
-	fse->max_width = supported_modes[fse->index].width;
-	fse->max_height = supported_modes[fse->index].height;
-	fse->min_height = supported_modes[fse->index].height;
+	crop = v4l2_subdev_state_get_crop(sd_state, 0);
+
+	fse->min_width = crop->width;
+	fse->max_width = crop->width;
+	fse->max_height = crop->height;
+	fse->min_height = crop->height;
 
 	return 0;
 }
@@ -388,20 +412,14 @@ static int ov4689_get_selection(struct v4l2_subdev *sd,
 				struct v4l2_subdev_state *state,
 				struct v4l2_subdev_selection *sel)
 {
-	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
-		return -EINVAL;
-
 	switch (sel->target) {
-	case V4L2_SEL_TGT_CROP_BOUNDS:
-		sel->r.top = 0;
-		sel->r.left = 0;
-		sel->r.width = OV4689_PIXEL_ARRAY_WIDTH;
-		sel->r.height = OV4689_PIXEL_ARRAY_HEIGHT;
-		return 0;
 	case V4L2_SEL_TGT_CROP:
+		sel->r = *v4l2_subdev_state_get_crop(state, sel->pad);
+		return 0;
+	case V4L2_SEL_TGT_CROP_BOUNDS:
 	case V4L2_SEL_TGT_CROP_DEFAULT:
-		sel->r.top = OV4689_DUMMY_ROWS;
 		sel->r.left = OV4689_DUMMY_COLUMNS;
+		sel->r.top = OV4689_DUMMY_ROWS;
 		sel->r.width =
 			OV4689_PIXEL_ARRAY_WIDTH - 2 * OV4689_DUMMY_COLUMNS;
 		sel->r.height =
@@ -412,37 +430,141 @@ static int ov4689_get_selection(struct v4l2_subdev *sd,
 	return -EINVAL;
 }
 
-static int ov4689_setup_timings(struct ov4689 *ov4689)
+/*
+ * Minimum working HTS value for given output width (found
+ * experimentally).
+ */
+static unsigned int ov4689_hts_min(unsigned int width)
+{
+	return max_t(unsigned int, 3156, 224 + width * 19 / 16);
+}
+
+static void ov4689_update_ctrl_ranges(struct ov4689 *ov4689,
+				      struct v4l2_rect *crop)
+{
+	struct v4l2_ctrl *exposure = ov4689->exposure;
+	struct v4l2_ctrl *vblank = ov4689->vblank;
+	struct v4l2_ctrl *hblank = ov4689->hblank;
+	s64 def_val, min_val, max_val;
+
+	min_val = ov4689_hts_min(crop->width) - crop->width;
+	max_val = OV4689_HTS_MAX - crop->width;
+	def_val = clamp_t(s64, hblank->default_value, min_val, max_val);
+	__v4l2_ctrl_modify_range(hblank, min_val, max_val, hblank->step,
+				 def_val);
+
+	min_val = OV4689_VBLANK_MIN;
+	max_val = OV4689_HTS_MAX - crop->width;
+	def_val = clamp_t(s64, vblank->default_value, min_val, max_val);
+	__v4l2_ctrl_modify_range(vblank, min_val, max_val, vblank->step,
+				 def_val);
+
+	min_val = exposure->minimum;
+	max_val = crop->height + vblank->val - 4;
+	def_val = clamp_t(s64, exposure->default_value, min_val, max_val);
+	__v4l2_ctrl_modify_range(exposure, min_val, max_val, exposure->step,
+				 def_val);
+}
+
+static int ov4689_set_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_selection *sel)
 {
-	const struct ov4689_mode *mode = ov4689->cur_mode;
+	struct ov4689 *ov4689 = to_ov4689(sd);
+	struct v4l2_mbus_framefmt *format;
+	struct v4l2_rect *crop;
+	struct v4l2_rect rect;
+
+	if (sel->target != V4L2_SEL_TGT_CROP)
+		return -EINVAL;
+
+	rect.left = clamp(ALIGN(sel->r.left, 2), OV4689_DUMMY_COLUMNS,
+			  OV4689_PIXEL_ARRAY_WIDTH);
+	rect.top = clamp(ALIGN(sel->r.top, 2), OV4689_DUMMY_ROWS,
+			 OV4689_PIXEL_ARRAY_HEIGHT);
+
+	rect.width = clamp_t(unsigned int, ALIGN(sel->r.width, 4),
+			     OV4689_H_CROP_MIN, OV4689_PIXEL_ARRAY_WIDTH -
+			     2 * OV4689_DUMMY_COLUMNS);
+	rect.height = clamp_t(unsigned int, ALIGN(sel->r.height, 2),
+			      OV4689_V_CROP_MIN, OV4689_PIXEL_ARRAY_HEIGHT -
+			      2 * OV4689_DUMMY_ROWS);
+
+	crop = v4l2_subdev_state_get_crop(state, sel->pad);
+
+	if (rect.width != crop->width || rect.height != crop->height) {
+		/*
+		 * Reset the output image size if the crop rectangle size has
+		 * been modified.
+		 */
+		format = v4l2_subdev_state_get_format(state, sel->pad);
+		format->width = rect.width;
+		format->height = rect.height;
+
+		if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+			ov4689_update_ctrl_ranges(ov4689, &rect);
+	}
+
+	*crop = rect;
+	sel->r = rect;
+
+	return 0;
+}
+
+static int ov4689_setup_timings(struct ov4689 *ov4689,
+				struct v4l2_subdev_state *state)
+{
+	const struct v4l2_mbus_framefmt *format;
 	struct regmap *rm = ov4689->regmap;
+	const struct v4l2_rect *crop;
 	int ret = 0;
 
-	cci_write(rm, OV4689_REG_H_CROP_START, 8, &ret);
-	cci_write(rm, OV4689_REG_V_CROP_START, 8, &ret);
-	cci_write(rm, OV4689_REG_H_CROP_END, 2711, &ret);
-	cci_write(rm, OV4689_REG_V_CROP_END, 1531, &ret);
+	format = v4l2_subdev_state_get_format(state, 0);
+	crop = v4l2_subdev_state_get_crop(state, 0);
+
+	cci_write(rm, OV4689_REG_H_CROP_START, crop->left, &ret);
+	cci_write(rm, OV4689_REG_V_CROP_START, crop->top, &ret);
+	cci_write(rm, OV4689_REG_H_CROP_END, crop->left + crop->width + 1, &ret);
+	cci_write(rm, OV4689_REG_V_CROP_END, crop->top + crop->height + 1, &ret);
 
-	cci_write(rm, OV4689_REG_H_OUTPUT_SIZE, mode->width, &ret);
-	cci_write(rm, OV4689_REG_V_OUTPUT_SIZE, mode->height, &ret);
+	cci_write(rm, OV4689_REG_H_OUTPUT_SIZE, format->width, &ret);
+	cci_write(rm, OV4689_REG_V_OUTPUT_SIZE, format->height, &ret);
 
-	cci_write(rm, OV4689_REG_H_WIN_OFF, 8, &ret);
-	cci_write(rm, OV4689_REG_V_WIN_OFF, 4, &ret);
+	cci_write(rm, OV4689_REG_H_WIN_OFF, 0, &ret);
+	cci_write(rm, OV4689_REG_V_WIN_OFF, 0, &ret);
 
-	cci_write(rm, OV4689_REG_VFIFO_CTRL_01, 167, &ret);
+	/*
+	 * Maximum working value of vfifo_read_start for given output
+	 * width (found experimentally).
+	 */
+	cci_write(rm, OV4689_REG_VFIFO_CTRL_01, format->width / 16 - 1, &ret);
 
 	return ret;
 }
 
-static int ov4689_setup_blc_anchors(struct ov4689 *ov4689)
+/*
+ * Setup black level compensation anchors. For the default frame width
+ * default anchors positions are used. For smaller crop sizes they are
+ * scaled accordingly.
+ */
+static int ov4689_setup_blc_anchors(struct ov4689 *ov4689,
+				    struct v4l2_subdev_state *state)
 {
+	unsigned int width_def = OV4689_H_OUTPUT_SIZE_DEFAULT;
 	struct regmap *rm = ov4689->regmap;
+	const struct v4l2_rect *crop;
 	int ret = 0;
 
-	cci_write(rm, OV4689_REG_ANCHOR_LEFT_START, 16, &ret);
-	cci_write(rm, OV4689_REG_ANCHOR_LEFT_END, 1999, &ret);
-	cci_write(rm, OV4689_REG_ANCHOR_RIGHT_START, 2400, &ret);
-	cci_write(rm, OV4689_REG_ANCHOR_RIGHT_END, 2415, &ret);
+	crop = v4l2_subdev_state_get_crop(state, 0);
+
+	cci_write(rm, OV4689_REG_ANCHOR_LEFT_START,
+		  OV4689_ANCHOR_LEFT_START_DEF * crop->width / width_def, &ret);
+	cci_write(rm, OV4689_REG_ANCHOR_LEFT_END,
+		  OV4689_ANCHOR_LEFT_END_DEF * crop->width / width_def, &ret);
+	cci_write(rm, OV4689_REG_ANCHOR_RIGHT_START,
+		  OV4689_ANCHOR_RIGHT_START_DEF * crop->width / width_def, &ret);
+	cci_write(rm, OV4689_REG_ANCHOR_RIGHT_END,
+		  OV4689_ANCHOR_RIGHT_END_DEF * crop->width / width_def, &ret);
 
 	return ret;
 }
@@ -470,13 +592,13 @@ static int ov4689_s_stream(struct v4l2_subdev *sd, int on)
 			goto unlock_and_return;
 		}
 
-		ret = ov4689_setup_timings(ov4689);
+		ret = ov4689_setup_timings(ov4689, sd_state);
 		if (ret) {
 			pm_runtime_put(dev);
 			goto unlock_and_return;
 		}
 
-		ret = ov4689_setup_blc_anchors(ov4689);
+		ret = ov4689_setup_blc_anchors(ov4689, sd_state);
 		if (ret) {
 			pm_runtime_put(dev);
 			goto unlock_and_return;
@@ -568,10 +690,25 @@ static int __maybe_unused ov4689_power_off(struct device *dev)
 static int ov4689_init_state(struct v4l2_subdev *sd,
 			     struct v4l2_subdev_state *sd_state)
 {
-	struct v4l2_mbus_framefmt *fmt =
-		v4l2_subdev_state_get_format(sd_state, 0);
+	u32 width_def = OV4689_H_OUTPUT_SIZE_DEFAULT;
+	u32 height_def = OV4689_V_OUTPUT_SIZE_DEFAULT;
+
+	struct v4l2_subdev_selection sel  = {
+		.target = V4L2_SEL_TGT_CROP,
+		.r.left = OV4689_DUMMY_COLUMNS,
+		.r.top = OV4689_DUMMY_ROWS,
+		.r.width = width_def,
+		.r.height = height_def,
+	};
+	struct v4l2_subdev_format format = {
+		.format = {
+			.width = width_def,
+			.height = height_def,
+		},
+	};
 
-	ov4689_fill_fmt(&supported_modes[OV4689_MODE_2688_1520], fmt);
+	ov4689_set_selection(sd, sd_state, &sel);
+	ov4689_set_fmt(sd, sd_state, &format);
 
 	return 0;
 }
@@ -590,6 +727,7 @@ static const struct v4l2_subdev_pad_ops ov4689_pad_ops = {
 	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = ov4689_set_fmt,
 	.get_selection = ov4689_get_selection,
+	.set_selection = ov4689_set_selection,
 };
 
 static const struct v4l2_subdev_internal_ops ov4689_internal_ops = {
@@ -635,20 +773,28 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl)
 	struct ov4689 *ov4689 =
 		container_of(ctrl->handler, struct ov4689, ctrl_handler);
 	struct regmap *regmap = ov4689->regmap;
+	struct v4l2_subdev_state *sd_state;
 	struct device *dev = ov4689->dev;
+	struct v4l2_rect *crop;
+	s64 max_expo, def_expo;
 	int sensor_gain = 0;
-	s64 max_expo;
 	int ret = 0;
 
+	sd_state = v4l2_subdev_get_locked_active_state(&ov4689->subdev);
+	crop = v4l2_subdev_state_get_crop(sd_state, 0);
+
 	/* Propagate change of current control to all related controls */
 	switch (ctrl->id) {
 	case V4L2_CID_VBLANK:
 		/* Update max exposure while meeting expected vblanking */
-		max_expo = ov4689->cur_mode->height + ctrl->val - 4;
-		__v4l2_ctrl_modify_range(ov4689->exposure,
-					 ov4689->exposure->minimum, max_expo,
-					 ov4689->exposure->step,
-					 ov4689->exposure->default_value);
+		max_expo = crop->height + ctrl->val - 4;
+		def_expo = clamp_t(s64, ov4689->exposure->default_value,
+				   ov4689->exposure->minimum, max_expo);
+
+		ret = __v4l2_ctrl_modify_range(ov4689->exposure,
+					       ov4689->exposure->minimum,
+					       max_expo, ov4689->exposure->step,
+					       def_expo);
 		break;
 	}
 
@@ -666,14 +812,14 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	case V4L2_CID_VBLANK:
 		cci_write(regmap, OV4689_REG_VTS,
-			  ctrl->val + ov4689->cur_mode->height, &ret);
+			  ctrl->val + crop->height, &ret);
 		break;
 	case V4L2_CID_TEST_PATTERN:
 		ret = ov4689_enable_test_pattern(ov4689, ctrl->val);
 		break;
 	case V4L2_CID_HBLANK:
 		cci_write(regmap, OV4689_REG_HTS,
-			  (ctrl->val + ov4689->cur_mode->width) /
+			  (ctrl->val + crop->width) /
 			  OV4689_HTS_DIVIDER, &ret);
 		break;
 	case V4L2_CID_VFLIP:
@@ -739,14 +885,16 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689)
 
 	hblank_def = mode->hts_def - mode->width;
 	hblank_min = mode->hts_min - mode->width;
-	v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_HBLANK,
-			  hblank_min, OV4689_HTS_MAX - mode->width,
-			  OV4689_HTS_DIVIDER, hblank_def);
+	ov4689->hblank = v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops,
+					   V4L2_CID_HBLANK, hblank_min,
+					   OV4689_HTS_MAX - mode->width,
+					   OV4689_HTS_DIVIDER, hblank_def);
 
 	vblank_def = mode->vts_def - mode->height;
-	v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_VBLANK,
-			  vblank_def, OV4689_VTS_MAX - mode->height, 1,
-			  vblank_def);
+	ov4689->vblank = v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops,
+					   V4L2_CID_VBLANK, OV4689_VBLANK_MIN,
+					   OV4689_VTS_MAX - mode->height, 1,
+					   vblank_def);
 
 	exposure_max = mode->vts_def - 4;
 	ov4689->exposure =
-- 
2.45.2

[PATCH 2/4] media: i2c: ov4689: Eliminate struct ov4689_mode

[Mikhail Rudenko]

With the output frame size now controlled by selection rather than
cur_mode, this commit relocates pixel rate and default VTS to
defines. Consequently, it removes struct ov4689_mode and the cur_mode
field from struct ov4689.

Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>
---
 drivers/media/i2c/ov4689.c | 70 +++++++++-----------------------------
 1 file changed, 17 insertions(+), 53 deletions(-)

diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c
index 933d714aa26c..affc6e3d5467 100644
--- a/drivers/media/i2c/ov4689.c
+++ b/drivers/media/i2c/ov4689.c
@@ -57,6 +57,8 @@
 #define OV4689_HTS_MAX			0x7fff
 
 #define OV4689_REG_VTS			CCI_REG16(0x380e)
+/* Default VTS corresponds to 30 fps at default crop and minimal HTS */
+#define OV4689_VTS_DEF			4683
 #define OV4689_VTS_MAX			0x7fff
 
 #define OV4689_REG_H_WIN_OFF		CCI_REG16(0x3810)
@@ -94,6 +96,7 @@
 
 #define OV4689_LANES			4
 #define OV4689_XVCLK_FREQ		24000000
+#define OV4689_PIXEL_RATE		480000000
 
 #define OV4689_PIXEL_ARRAY_WIDTH	2720
 #define OV4689_PIXEL_ARRAY_HEIGHT	1536
@@ -119,24 +122,6 @@ static const char *const ov4689_supply_names[] = {
 	"dvdd", /* Digital core power */
 };
 
-enum ov4689_mode_id {
-	OV4689_MODE_2688_1520 = 0,
-	OV4689_NUM_MODES,
-};
-
-struct ov4689_mode {
-	enum ov4689_mode_id id;
-	u32 width;
-	u32 height;
-	u32 hts_def;
-	u32 hts_min;
-	u32 vts_def;
-	u32 exp_def;
-	u32 pixel_rate;
-	const struct cci_reg_sequence *reg_list;
-	unsigned int num_regs;
-};
-
 struct ov4689 {
 	struct device *dev;
 	struct regmap *regmap;
@@ -152,8 +137,6 @@ struct ov4689 {
 
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_ctrl *exposure, *hblank, *vblank;
-
-	const struct ov4689_mode *cur_mode;
 };
 
 #define to_ov4689(sd) container_of(sd, struct ov4689, subdev)
@@ -172,7 +155,7 @@ struct ov4689_gain_range {
  * max_framerate 90fps
  * mipi_datarate per lane 1008Mbps
  */
-static const struct cci_reg_sequence ov4689_2688x1520_regs[] = {
+static const struct cci_reg_sequence ov4689_common_regs[] = {
 	/* System control*/
 	{ CCI_REG8(0x0103), 0x01 }, /* SC_CTRL0103 software_reset = 1 */
 	{ CCI_REG8(0x3000), 0x20 }, /* SC_CMMN_PAD_OEN0 FSIN_output_enable = 1 */
@@ -273,21 +256,6 @@ static const struct cci_reg_sequence ov4689_2688x1520_regs[] = {
 	{ CCI_REG8(0x5503), 0x0f }, /* OTP_DPC_END_L otp_end_address[7:0] = 0x0f */
 };
 
-static const struct ov4689_mode supported_modes[] = {
-	{
-		.id = OV4689_MODE_2688_1520,
-		.width = 2688,
-		.height = 1520,
-		.exp_def = 1536,
-		.hts_def = 10296,
-		.hts_min = 3432,
-		.vts_def = 1554,
-		.pixel_rate = 480000000,
-		.reg_list = ov4689_2688x1520_regs,
-		.num_regs = ARRAY_SIZE(ov4689_2688x1520_regs),
-	},
-};
-
 static const u64 link_freq_menu_items[] = { 504000000 };
 
 static const char *const ov4689_test_pattern_menu[] = {
@@ -584,8 +552,8 @@ static int ov4689_s_stream(struct v4l2_subdev *sd, int on)
 			goto unlock_and_return;
 
 		ret = cci_multi_reg_write(ov4689->regmap,
-					  ov4689->cur_mode->reg_list,
-					  ov4689->cur_mode->num_regs,
+					  ov4689_common_regs,
+					  ARRAY_SIZE(ov4689_common_regs),
 					  NULL);
 		if (ret) {
 			pm_runtime_put(dev);
@@ -863,14 +831,12 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689)
 	struct i2c_client *client = v4l2_get_subdevdata(&ov4689->subdev);
 	struct v4l2_fwnode_device_properties props;
 	struct v4l2_ctrl_handler *handler;
-	const struct ov4689_mode *mode;
 	s64 exposure_max, vblank_def;
-	s64 hblank_def, hblank_min;
 	struct v4l2_ctrl *ctrl;
+	s64 hblank_def;
 	int ret;
 
 	handler = &ov4689->ctrl_handler;
-	mode = ov4689->cur_mode;
 	ret = v4l2_ctrl_handler_init(handler, 15);
 	if (ret)
 		return ret;
@@ -881,26 +847,26 @@ static int ov4689_initialize_controls(struct ov4689 *ov4689)
 		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
 	v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, 0,
-			  mode->pixel_rate, 1, mode->pixel_rate);
+			  OV4689_PIXEL_RATE, 1, OV4689_PIXEL_RATE);
 
-	hblank_def = mode->hts_def - mode->width;
-	hblank_min = mode->hts_min - mode->width;
+	hblank_def = ov4689_hts_min(OV4689_H_OUTPUT_SIZE_DEFAULT) -
+		     OV4689_H_OUTPUT_SIZE_DEFAULT;
 	ov4689->hblank = v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops,
-					   V4L2_CID_HBLANK, hblank_min,
-					   OV4689_HTS_MAX - mode->width,
+					   V4L2_CID_HBLANK, hblank_def,
+					   OV4689_HTS_MAX - OV4689_H_OUTPUT_SIZE_DEFAULT,
 					   OV4689_HTS_DIVIDER, hblank_def);
 
-	vblank_def = mode->vts_def - mode->height;
+	vblank_def = OV4689_VTS_DEF - OV4689_V_OUTPUT_SIZE_DEFAULT;
 	ov4689->vblank = v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops,
 					   V4L2_CID_VBLANK, OV4689_VBLANK_MIN,
-					   OV4689_VTS_MAX - mode->height, 1,
-					   vblank_def);
+					   OV4689_VTS_MAX - OV4689_V_OUTPUT_SIZE_DEFAULT,
+					   1, vblank_def);
 
-	exposure_max = mode->vts_def - 4;
+	exposure_max = OV4689_VTS_DEF - 4;
 	ov4689->exposure =
 		v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_EXPOSURE,
 				  OV4689_EXPOSURE_MIN, exposure_max,
-				  OV4689_EXPOSURE_STEP, mode->exp_def);
+				  OV4689_EXPOSURE_STEP, exposure_max);
 
 	v4l2_ctrl_new_std(handler, &ov4689_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
 			  ov4689_gain_ranges[0].logical_min,
@@ -1055,8 +1021,6 @@ static int ov4689_probe(struct i2c_client *client)
 
 	ov4689->dev = dev;
 
-	ov4689->cur_mode = &supported_modes[OV4689_MODE_2688_1520];
-
 	ov4689->xvclk = devm_clk_get_optional(dev, NULL);
 	if (IS_ERR(ov4689->xvclk))
 		return dev_err_probe(dev, PTR_ERR(ov4689->xvclk),
-- 
2.45.2

[PATCH 3/4] media: i2c: ov4689: Refactor ov4689_s_stream

[Mikhail Rudenko]

Split ov4689_s_stream into ov4689_stream_on and ov4689_stream_off
functions. Also remove repetitive pm_runtime_put calls and call
pm_runtime_put once at the end of the ov4689_stream_on function if any
error occurred.

Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ov4689.c | 100 ++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 47 deletions(-)

diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c
index affc6e3d5467..c4c7c462672a 100644
--- a/drivers/media/i2c/ov4689.c
+++ b/drivers/media/i2c/ov4689.c
@@ -537,61 +537,67 @@ static int ov4689_setup_blc_anchors(struct ov4689 *ov4689,
 	return ret;
 }
 
+static int ov4689_stream_on(struct ov4689 *ov4689,
+			    struct v4l2_subdev_state *sd_state)
+{
+	int ret;
+
+	ret = pm_runtime_resume_and_get(ov4689->dev);
+	if (ret < 0)
+		return ret;
+
+	ret = cci_multi_reg_write(ov4689->regmap, ov4689_common_regs,
+				  ARRAY_SIZE(ov4689_common_regs), NULL);
+	if (ret)
+		goto error;
+
+	ret = ov4689_setup_timings(ov4689, sd_state);
+	if (ret)
+		goto error;
+
+	ret = ov4689_setup_blc_anchors(ov4689, sd_state);
+	if (ret)
+		goto error;
+
+	ret = __v4l2_ctrl_handler_setup(&ov4689->ctrl_handler);
+	if (ret)
+		goto error;
+
+	ret = cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE,
+			OV4689_MODE_STREAMING, NULL);
+	if (ret)
+		goto error;
+
+	return 0;
+
+error:
+	pm_runtime_put(ov4689->dev);
+	return ret;
+}
+
+static int ov4689_stream_off(struct ov4689 *ov4689)
+{
+	cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, OV4689_MODE_SW_STANDBY,
+		  NULL);
+	pm_runtime_mark_last_busy(ov4689->dev);
+	pm_runtime_put_autosuspend(ov4689->dev);
+
+	return 0;
+}
+
 static int ov4689_s_stream(struct v4l2_subdev *sd, int on)
 {
 	struct ov4689 *ov4689 = to_ov4689(sd);
 	struct v4l2_subdev_state *sd_state;
-	struct device *dev = ov4689->dev;
-	int ret = 0;
+	int ret;
 
 	sd_state = v4l2_subdev_lock_and_get_active_state(&ov4689->subdev);
 
-	if (on) {
-		ret = pm_runtime_resume_and_get(dev);
-		if (ret < 0)
-			goto unlock_and_return;
-
-		ret = cci_multi_reg_write(ov4689->regmap,
-					  ov4689_common_regs,
-					  ARRAY_SIZE(ov4689_common_regs),
-					  NULL);
-		if (ret) {
-			pm_runtime_put(dev);
-			goto unlock_and_return;
-		}
-
-		ret = ov4689_setup_timings(ov4689, sd_state);
-		if (ret) {
-			pm_runtime_put(dev);
-			goto unlock_and_return;
-		}
-
-		ret = ov4689_setup_blc_anchors(ov4689, sd_state);
-		if (ret) {
-			pm_runtime_put(dev);
-			goto unlock_and_return;
-		}
-
-		ret = __v4l2_ctrl_handler_setup(&ov4689->ctrl_handler);
-		if (ret) {
-			pm_runtime_put(dev);
-			goto unlock_and_return;
-		}
-
-		ret = cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE,
-				OV4689_MODE_STREAMING, NULL);
-		if (ret) {
-			pm_runtime_put(dev);
-			goto unlock_and_return;
-		}
-	} else {
-		cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE,
-			  OV4689_MODE_SW_STANDBY, NULL);
-		pm_runtime_mark_last_busy(dev);
-		pm_runtime_put_autosuspend(dev);
-	}
+	if (on)
+		ret = ov4689_stream_on(ov4689, sd_state);
+	else
+		ret = ov4689_stream_off(ov4689);
 
-unlock_and_return:
 	v4l2_subdev_unlock_state(sd_state);
 
 	return ret;
-- 
2.45.2

[PATCH 4/4] media: i2c: ov4689: Implement 2x2 binning

[Mikhail Rudenko]

Implement 2x2 binning support. Compute best binning mode (none or 2x2)
from pad crop and pad format in ov4689_set_fmt. Use output frame size
instead of analogue crop to compute control ranges and BLC anchors.

Also move ov4689_hts_min and ov4689_update_ctrl_ranges, since they are
now also called from ov4689_set_fmt. Update frame timings to
accommodate the requirements of binning mode and avoid visual
artefacts. Additionally, report 2x2 binned mode in addition to
non-binned one in ov4689_enum_frame_sizes.

Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>
---
 drivers/media/i2c/ov4689.c | 179 ++++++++++++++++++++++++-------------
 1 file changed, 117 insertions(+), 62 deletions(-)

diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c
index c4c7c462672a..1499fbe88b76 100644
--- a/drivers/media/i2c/ov4689.c
+++ b/drivers/media/i2c/ov4689.c
@@ -114,7 +114,7 @@
  * Minimum working vertical blanking value. Found experimentally at
  * minimum HTS values.
  */
-#define OV4689_VBLANK_MIN		31
+#define OV4689_VBLANK_MIN		35
 
 static const char *const ov4689_supply_names[] = {
 	"avdd", /* Analog power */
@@ -256,6 +256,18 @@ static const struct cci_reg_sequence ov4689_common_regs[] = {
 	{ CCI_REG8(0x5503), 0x0f }, /* OTP_DPC_END_L otp_end_address[7:0] = 0x0f */
 };
 
+static const struct cci_reg_sequence ov4689_2x2_binning_regs[] = {
+	{ CCI_REG8(0x3632), 0x05 }, /* ADC */
+	{ CCI_REG8(0x376b), 0x40 }, /* Sensor control */
+	{ CCI_REG8(0x3814), 0x03 }, /* H_INC_ODD */
+	{ CCI_REG8(0x3821), 0x07 }, /* TIMING_FORMAT_2 hor_binning = 1*/
+	{ CCI_REG8(0x382a), 0x03 }, /* V_INC_ODD */
+	{ CCI_REG8(0x3830), 0x08 }, /* BLC_NUM_OPTION blc_use_num_2 = 1 */
+	{ CCI_REG8(0x3836), 0x02 }, /* TIMING_REG_36 r_zline_use_num_2 = 1 */
+	{ CCI_REG8(0x4001), 0x50 }, /* BLC DEBUG MODE */
+	{ CCI_REG8(0x4502), 0x44 }, /* ADC synch control*/
+};
+
 static const u64 link_freq_menu_items[] = { 504000000 };
 
 static const char *const ov4689_test_pattern_menu[] = {
@@ -305,18 +317,85 @@ static const struct ov4689_gain_range ov4689_gain_ranges[] = {
 	},
 };
 
+/*
+ * For now, only 2x2 binning implemented in this driver.
+ */
+static void ov4689_best_binning(struct ov4689 *ov4689,
+				const struct v4l2_mbus_framefmt *format,
+				const struct v4l2_rect *crop,
+				unsigned int *binning)
+{
+	const struct v4l2_area candidates[] = {
+		{ crop->width, crop->height },
+		{ crop->width / 2, crop->height / 2 },
+	};
+
+	const struct v4l2_area *best;
+	int index;
+
+	best = v4l2_find_nearest_size(candidates, ARRAY_SIZE(candidates), width,
+				      height, format->width, format->height);
+	index = best - candidates;
+	*binning = index + 1;
+
+	dev_dbg(ov4689->dev,
+		"best_binning: crop=%dx%d format=%dx%d binning=%d\n",
+		crop->width, crop->height, format->width, format->height,
+		*binning);
+}
+
+/*
+ * Minimum working HTS value for given output width (found
+ * experimentally).
+ */
+static unsigned int ov4689_hts_min(unsigned int width)
+{
+	return max_t(unsigned int, 3156, 224 + width * 19 / 16);
+}
+
+static void ov4689_update_ctrl_ranges(struct ov4689 *ov4689, unsigned int width,
+				      unsigned int height)
+{
+	struct v4l2_ctrl *exposure = ov4689->exposure;
+	struct v4l2_ctrl *vblank = ov4689->vblank;
+	struct v4l2_ctrl *hblank = ov4689->hblank;
+	s64 def_val, min_val, max_val;
+
+	min_val = ov4689_hts_min(width) - width;
+	max_val = OV4689_HTS_MAX - width;
+	def_val = clamp_t(s64, hblank->default_value, min_val, max_val);
+	__v4l2_ctrl_modify_range(hblank, min_val, max_val, hblank->step,
+				 def_val);
+
+	min_val = OV4689_VBLANK_MIN;
+	max_val = OV4689_HTS_MAX - width;
+	def_val = clamp_t(s64, vblank->default_value, min_val, max_val);
+	__v4l2_ctrl_modify_range(vblank, min_val, max_val, vblank->step,
+				 def_val);
+
+	min_val = exposure->minimum;
+	max_val = height + vblank->val - 4;
+	def_val = clamp_t(s64, exposure->default_value, min_val, max_val);
+	__v4l2_ctrl_modify_range(exposure, min_val, max_val, exposure->step,
+				 def_val);
+}
+
 static int ov4689_set_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_state *sd_state,
 			  struct v4l2_subdev_format *fmt)
 {
+	struct ov4689 *ov4689 = to_ov4689(sd);
 	struct v4l2_mbus_framefmt *format;
 	struct v4l2_rect *crop;
+	unsigned int binning;
 
 	crop = v4l2_subdev_state_get_crop(sd_state, fmt->pad);
 	format = v4l2_subdev_state_get_format(sd_state, fmt->pad);
 
-	format->width = crop->width;
-	format->height = crop->height;
+	ov4689_best_binning(ov4689, &fmt->format, crop, &binning);
+
+	format->width = crop->width / binning;
+	format->height = crop->height / binning;
 
 	format->code = MEDIA_BUS_FMT_SBGGR10_1X10;
 	format->field = V4L2_FIELD_NONE;
@@ -327,6 +406,9 @@ static int ov4689_set_fmt(struct v4l2_subdev *sd,
 
 	fmt->format = *format;
 
+	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		ov4689_update_ctrl_ranges(ov4689, format->width, format->height);
+
 	return 0;
 }
 
@@ -346,8 +428,9 @@ static int ov4689_enum_frame_sizes(struct v4l2_subdev *sd,
 				   struct v4l2_subdev_frame_size_enum *fse)
 {
 	const struct v4l2_rect *crop;
+	int binning;
 
-	if (fse->index >= 1)
+	if (fse->index >= 2)
 		return -EINVAL;
 
 	if (fse->code != MEDIA_BUS_FMT_SBGGR10_1X10)
@@ -355,10 +438,11 @@ static int ov4689_enum_frame_sizes(struct v4l2_subdev *sd,
 
 	crop = v4l2_subdev_state_get_crop(sd_state, 0);
 
-	fse->min_width = crop->width;
-	fse->max_width = crop->width;
-	fse->max_height = crop->height;
-	fse->min_height = crop->height;
+	binning = fse->index + 1;
+	fse->min_width = crop->width / binning;
+	fse->max_width = crop->width / binning;
+	fse->max_height = crop->height / binning;
+	fse->min_height = crop->height / binning;
 
 	return 0;
 }
@@ -398,42 +482,6 @@ static int ov4689_get_selection(struct v4l2_subdev *sd,
 	return -EINVAL;
 }
 
-/*
- * Minimum working HTS value for given output width (found
- * experimentally).
- */
-static unsigned int ov4689_hts_min(unsigned int width)
-{
-	return max_t(unsigned int, 3156, 224 + width * 19 / 16);
-}
-
-static void ov4689_update_ctrl_ranges(struct ov4689 *ov4689,
-				      struct v4l2_rect *crop)
-{
-	struct v4l2_ctrl *exposure = ov4689->exposure;
-	struct v4l2_ctrl *vblank = ov4689->vblank;
-	struct v4l2_ctrl *hblank = ov4689->hblank;
-	s64 def_val, min_val, max_val;
-
-	min_val = ov4689_hts_min(crop->width) - crop->width;
-	max_val = OV4689_HTS_MAX - crop->width;
-	def_val = clamp_t(s64, hblank->default_value, min_val, max_val);
-	__v4l2_ctrl_modify_range(hblank, min_val, max_val, hblank->step,
-				 def_val);
-
-	min_val = OV4689_VBLANK_MIN;
-	max_val = OV4689_HTS_MAX - crop->width;
-	def_val = clamp_t(s64, vblank->default_value, min_val, max_val);
-	__v4l2_ctrl_modify_range(vblank, min_val, max_val, vblank->step,
-				 def_val);
-
-	min_val = exposure->minimum;
-	max_val = crop->height + vblank->val - 4;
-	def_val = clamp_t(s64, exposure->default_value, min_val, max_val);
-	__v4l2_ctrl_modify_range(exposure, min_val, max_val, exposure->step,
-				 def_val);
-}
-
 static int ov4689_set_selection(struct v4l2_subdev *sd,
 				struct v4l2_subdev_state *state,
 				struct v4l2_subdev_selection *sel)
@@ -470,7 +518,8 @@ static int ov4689_set_selection(struct v4l2_subdev *sd,
 		format->height = rect.height;
 
 		if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-			ov4689_update_ctrl_ranges(ov4689, &rect);
+			ov4689_update_ctrl_ranges(ov4689, rect.width,
+						  rect.height);
 	}
 
 	*crop = rect;
@@ -485,21 +534,25 @@ static int ov4689_setup_timings(struct ov4689 *ov4689,
 	const struct v4l2_mbus_framefmt *format;
 	struct regmap *rm = ov4689->regmap;
 	const struct v4l2_rect *crop;
+	const int v_offset = 2;
+	unsigned int binning;
 	int ret = 0;
 
 	format = v4l2_subdev_state_get_format(state, 0);
 	crop = v4l2_subdev_state_get_crop(state, 0);
 
+	ov4689_best_binning(ov4689, format, crop, &binning);
+
 	cci_write(rm, OV4689_REG_H_CROP_START, crop->left, &ret);
-	cci_write(rm, OV4689_REG_V_CROP_START, crop->top, &ret);
-	cci_write(rm, OV4689_REG_H_CROP_END, crop->left + crop->width + 1, &ret);
-	cci_write(rm, OV4689_REG_V_CROP_END, crop->top + crop->height + 1, &ret);
+	cci_write(rm, OV4689_REG_V_CROP_START, crop->top - v_offset, &ret);
+	cci_write(rm, OV4689_REG_H_CROP_END, crop->left + crop->width + 3, &ret);
+	cci_write(rm, OV4689_REG_V_CROP_END, crop->top + crop->height + 7, &ret);
 
 	cci_write(rm, OV4689_REG_H_OUTPUT_SIZE, format->width, &ret);
 	cci_write(rm, OV4689_REG_V_OUTPUT_SIZE, format->height, &ret);
 
 	cci_write(rm, OV4689_REG_H_WIN_OFF, 0, &ret);
-	cci_write(rm, OV4689_REG_V_WIN_OFF, 0, &ret);
+	cci_write(rm, OV4689_REG_V_WIN_OFF, v_offset, &ret);
 
 	/*
 	 * Maximum working value of vfifo_read_start for given output
@@ -507,6 +560,10 @@ static int ov4689_setup_timings(struct ov4689 *ov4689,
 	 */
 	cci_write(rm, OV4689_REG_VFIFO_CTRL_01, format->width / 16 - 1, &ret);
 
+	if (binning == 2)
+		cci_multi_reg_write(ov4689->regmap, ov4689_2x2_binning_regs,
+				    ARRAY_SIZE(ov4689_2x2_binning_regs),
+				    &ret);
 	return ret;
 }
 
@@ -519,20 +576,20 @@ static int ov4689_setup_blc_anchors(struct ov4689 *ov4689,
 				    struct v4l2_subdev_state *state)
 {
 	unsigned int width_def = OV4689_H_OUTPUT_SIZE_DEFAULT;
+	const struct v4l2_mbus_framefmt *format;
 	struct regmap *rm = ov4689->regmap;
-	const struct v4l2_rect *crop;
 	int ret = 0;
 
-	crop = v4l2_subdev_state_get_crop(state, 0);
+	format = v4l2_subdev_state_get_format(state, 0);
 
 	cci_write(rm, OV4689_REG_ANCHOR_LEFT_START,
-		  OV4689_ANCHOR_LEFT_START_DEF * crop->width / width_def, &ret);
+		  OV4689_ANCHOR_LEFT_START_DEF * format->width / width_def, &ret);
 	cci_write(rm, OV4689_REG_ANCHOR_LEFT_END,
-		  OV4689_ANCHOR_LEFT_END_DEF * crop->width / width_def, &ret);
+		  OV4689_ANCHOR_LEFT_END_DEF * format->width / width_def, &ret);
 	cci_write(rm, OV4689_REG_ANCHOR_RIGHT_START,
-		  OV4689_ANCHOR_RIGHT_START_DEF * crop->width / width_def, &ret);
+		  OV4689_ANCHOR_RIGHT_START_DEF * format->width / width_def, &ret);
 	cci_write(rm, OV4689_REG_ANCHOR_RIGHT_END,
-		  OV4689_ANCHOR_RIGHT_END_DEF * crop->width / width_def, &ret);
+		  OV4689_ANCHOR_RIGHT_END_DEF * format->width / width_def, &ret);
 
 	return ret;
 }
@@ -749,19 +806,19 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl)
 	struct regmap *regmap = ov4689->regmap;
 	struct v4l2_subdev_state *sd_state;
 	struct device *dev = ov4689->dev;
-	struct v4l2_rect *crop;
+	struct v4l2_mbus_framefmt *fmt;
 	s64 max_expo, def_expo;
 	int sensor_gain = 0;
 	int ret = 0;
 
 	sd_state = v4l2_subdev_get_locked_active_state(&ov4689->subdev);
-	crop = v4l2_subdev_state_get_crop(sd_state, 0);
+	fmt = v4l2_subdev_state_get_format(sd_state, 0);
 
 	/* Propagate change of current control to all related controls */
 	switch (ctrl->id) {
 	case V4L2_CID_VBLANK:
 		/* Update max exposure while meeting expected vblanking */
-		max_expo = crop->height + ctrl->val - 4;
+		max_expo = fmt->height + ctrl->val - 4;
 		def_expo = clamp_t(s64, ov4689->exposure->default_value,
 				   ov4689->exposure->minimum, max_expo);
 
@@ -785,16 +842,14 @@ static int ov4689_set_ctrl(struct v4l2_ctrl *ctrl)
 		cci_write(regmap, OV4689_REG_GAIN, sensor_gain, &ret);
 		break;
 	case V4L2_CID_VBLANK:
-		cci_write(regmap, OV4689_REG_VTS,
-			  ctrl->val + crop->height, &ret);
+		cci_write(regmap, OV4689_REG_VTS, ctrl->val + fmt->height, &ret);
 		break;
 	case V4L2_CID_TEST_PATTERN:
 		ret = ov4689_enable_test_pattern(ov4689, ctrl->val);
 		break;
 	case V4L2_CID_HBLANK:
 		cci_write(regmap, OV4689_REG_HTS,
-			  (ctrl->val + crop->width) /
-			  OV4689_HTS_DIVIDER, &ret);
+			  (ctrl->val + fmt->width) / OV4689_HTS_DIVIDER, &ret);
 		break;
 	case V4L2_CID_VFLIP:
 		cci_update_bits(regmap, OV4689_REG_TIMING_FORMAT1,
-- 
2.45.2

[PATCH 4/4] media: i2c: ov4689: Implement 2x2 binning

[Changhuang Liang]

Hi, Mikhail

Thanks for your patch.

>
> Implement 2x2 binning support. Compute best binning mode (none or 2x2)
> from pad crop and pad format in ov4689_set_fmt. Use output frame size
> instead of analogue crop to compute control ranges and BLC anchors.
>
> Also move ov4689_hts_min and ov4689_update_ctrl_ranges, since they are
> now also called from ov4689_set_fmt. Update frame timings to
> accommodate the requirements of binning mode and avoid visual
> artefacts. Additionally, report 2x2 binned mode in addition to
> non-binned one in ov4689_enum_frame_sizes.
>
> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>

Tested-by: Changhuang Liang <changhuang.liang@starfivetech.com>

> ---
>  drivers/media/i2c/ov4689.c | 179 ++++++++++++++++++++++++-------------
>  1 file changed, 117 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c
> index c4c7c462672a..1499fbe88b76 100644
> --- a/drivers/media/i2c/ov4689.c
> +++ b/drivers/media/i2c/ov4689.c
> @@ -114,7 +114,7 @@
>   * Minimum working vertical blanking value. Found experimentally at
>   * minimum HTS values.
>   */

Regards,
Changhuang

Re: [PATCH 4/4] media: i2c: ov4689: Implement 2x2 binning

[Mikhail Rudenko]

Hi, Changhuang

On 2024-08-13 at 05:44 -07, Changhuang Liang <changhuang.liang@starfivetech.com> wrote:

> Hi, Mikhail
>
> Thanks for your patch.
>
>>
>> Implement 2x2 binning support. Compute best binning mode (none or 2x2)
>> from pad crop and pad format in ov4689_set_fmt. Use output frame size
>> instead of analogue crop to compute control ranges and BLC anchors.
>>
>> Also move ov4689_hts_min and ov4689_update_ctrl_ranges, since they are
>> now also called from ov4689_set_fmt. Update frame timings to
>> accommodate the requirements of binning mode and avoid visual
>> artefacts. Additionally, report 2x2 binned mode in addition to
>> non-binned one in ov4689_enum_frame_sizes.
>>
>> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>
>
> Tested-by: Changhuang Liang <changhuang.liang@starfivetech.com>
>

Thanks for testing! It's good to see that this code has now been
independently verified as it contains a few bits that were obtained
through reverse engineering/trial and error that I was never entirely
sure of. (Of course I did lots of testing to ensure that all possible
crop+binning modes work as expected, but when datasheet lacks required
information, one can never be fully sure.)

>> ---
>>  drivers/media/i2c/ov4689.c | 179 ++++++++++++++++++++++++-------------
>>  1 file changed, 117 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c
>> index c4c7c462672a..1499fbe88b76 100644
>> --- a/drivers/media/i2c/ov4689.c
>> +++ b/drivers/media/i2c/ov4689.c
>> @@ -114,7 +114,7 @@
>>   * Minimum working vertical blanking value. Found experimentally at
>>   * minimum HTS values.
>>   */
>
> Regards,
> Changhuang


--
Best regards,
Mikhail Rudenko

Re: [PATCH 0/4] Omnivision OV4689 cropping and binning

[Mikhail Rudenko]

On 2024-06-16 at 23:24 +03, Mikhail Rudenko <mike.rudenko@gmail.com> wrote:

> This small series consists of patches 17-20 from [1], which were
> requested to merged separately later. No significant changes have been
> made since and the R-b's are kept.
>
> Patch 1 implements configurable analogue crop rectangle via
> .set_selection callback. Patches 2 and 3 are refactorings and are not
> supposed to introduce any functional change. Patch 4 enables 2x2
> binning option.
>
> [1] https://lore.kernel.org/all/20240402164552.19171-1-mike.rudenko@gmail.com/
>
> Mikhail Rudenko (4):
>   media: i2c: ov4689: Configurable analogue crop
>   media: i2c: ov4689: Eliminate struct ov4689_mode
>   media: i2c: ov4689: Refactor ov4689_s_stream
>   media: i2c: ov4689: Implement 2x2 binning
>
>  drivers/media/i2c/ov4689.c | 483 +++++++++++++++++++++++++------------
>  1 file changed, 328 insertions(+), 155 deletions(-)

Gentle ping.

Sakari, in [1] you said the these four patches should be postponed until
internal API refactoring is complete. Is it complete now? Could you
point out the places where new API should be used?

[1] https://lore.kernel.org/all/ZhzEdXz-R2I6nZXf@kekkonen.localdomain/

--
Best regards,
Mikhail Rudenko