[PATCH v3 0/7] media: imx274: cleanups, improvements and SELECTION API support

[PATCH v3 0/7] media: imx274: cleanups, improvements and SELECTION API support

[Luca Ceresoli]

Hi,

this patchset introduces cropping support for the Sony IMX274 sensor
using the SELECTION API. 

v3 has a few minor improvements over v2. It also removes the first 6
patches, already applied on the media_tree master branch.

After v2 there has been a short discussion with Sakari Ailus on how
cropping should be configured from userspace [0]. That discussion has
gone stale before I could understand the idea behind the changes
suggested by Sakari, so I'm sending the most up-to-date version of the
old implementation to give it a new spin. I'll be glad to rework my
patch when things are clearer.

Patches 1-5 are overall improvements and restructuring, mostly useful
to implement the SELECTION API in a clean way.

Patch 6 introduces a helper to allow setting many registers computed
at runtime in a straightforward way. This would not have been very
useful before because all long register write sequences came from
const tables, but it's definitely a must for the cropping code where
several register values are computed at runtime.

Patch 7 implements cropping in the set_selection pad operation. On the
v4l2 side there is nothing special. The most tricky part was
respecting all the device constraints on the horizontal crop.

Usage examples:
    
 * Capture the entire 4K area, downscaled to 1080p with 2:1 binning:
   media-ctl -V '"IMX274":0[crop:(0,0)/3840x2160]'
   media-ctl -V '"IMX274":0[fmt:SRGGB8_1X8/1920x1080 field:none]'

 * Capture the central 1080p area (no binning):
   media-ctl -V '"IMX274":0[crop:(960,540)/1920x1080]'
   (this also sets the fmt to 1920x1080)              

Regards,
Luca

[0] https://www.spinics.net/lists/kernel/msg2787725.html


Luca Ceresoli (7):
  media: imx274: initialize format before v4l2 controls
  media: imx274: consolidate per-mode data in imx274_frmfmt
  media: imx274: get rid of mode_index
  media: imx274: actually use IMX274_DEFAULT_MODE
  media: imx274: simplify imx274_write_table()
  media: imx274: add helper function to fill a reg_8 table chunk
  media: imx274: add SELECTION support for cropping

 drivers/media/i2c/imx274.c | 552 +++++++++++++++++++++++++++------------------
 1 file changed, 332 insertions(+), 220 deletions(-)

-- 
2.7.4

[PATCH v3 1/7] media: imx274: initialize format before v4l2 controls

[Luca Ceresoli]

The current probe function calls v4l2_ctrl_handler_setup() before
initializing the format info. This triggers call paths such as:
imx274_probe -> v4l2_ctrl_handler_setup -> imx274_s_ctrl ->
imx274_set_exposure, where priv->mode_index is accessed before being
assigned.

This is wrong but does not trigger a visible bug because priv is
zero-initialized and 0 is the default value for priv->mode_index. But
this would become a crash in follow-up commits when mode_index is
replaced by a pointer that must always be valid.

Fix the bug before it shows up by initializing struct members early.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

---
Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 63fb94e7da37..8a8a11b8d75d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1632,6 +1632,16 @@ static int imx274_probe(struct i2c_client *client,
 
 	mutex_init(&imx274->lock);
 
+	/* initialize format */
+	imx274->mode_index = IMX274_MODE_3840X2160;
+	imx274->format.width = imx274_formats[0].size.width;
+	imx274->format.height = imx274_formats[0].size.height;
+	imx274->format.field = V4L2_FIELD_NONE;
+	imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
+	imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
+	imx274->frame_interval.numerator = 1;
+	imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
+
 	/* initialize regmap */
 	imx274->regmap = devm_regmap_init_i2c(client, &imx274_regmap_config);
 	if (IS_ERR(imx274->regmap)) {
@@ -1720,16 +1730,6 @@ static int imx274_probe(struct i2c_client *client,
 		goto err_ctrls;
 	}
 
-	/* initialize format */
-	imx274->mode_index = IMX274_MODE_3840X2160;
-	imx274->format.width = imx274_formats[0].size.width;
-	imx274->format.height = imx274_formats[0].size.height;
-	imx274->format.field = V4L2_FIELD_NONE;
-	imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
-	imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
-	imx274->frame_interval.numerator = 1;
-	imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
-
 	/* load default control values */
 	ret = imx274_load_default(imx274);
 	if (ret) {
-- 
2.7.4

[PATCH v3 2/7] media: imx274: consolidate per-mode data in imx274_frmfmt

[Luca Ceresoli]

Data about the implemented readout modes is partially stored in
imx274_formats[], the rest is scattered in several arrays. The latter
are then accessed using the mode index, e.g.:

  min_frame_len[priv->mode_index]

Consolidate all these data in imx274_formats[], and store a pointer to
the selected mode (i.e. imx274_formats[priv->mode_index]) in the main
device struct. This way code to use these data becomes more readable,
e.g.:

  priv->mode->min_frame_len

This removes lots of scaffolding code and keeps data about each mode
in a unique place.

Also remove a parameter to imx274_mode_regs() that is now unused.

While this adds the mode pointer to the device struct, it does not
remove the mode_index from it because mode_index is still used in two
dev_dbg() calls.  This will be handled in a follow-up commit.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

---
Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 139 +++++++++++++++++++++------------------------
 1 file changed, 66 insertions(+), 73 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 8a8a11b8d75d..2ec31ae4e60d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -147,10 +147,28 @@ enum imx274_mode {
 };
 
 /*
- * imx274 format related structure
+ * Parameters for each imx274 readout mode.
+ *
+ * These are the values to configure the sensor in one of the
+ * implemented modes.
+ *
+ * @size: recommended recording pixels
+ * @init_regs: registers to initialize the mode
+ * @min_frame_len: Minimum frame length for each mode (see "Frame Rate
+ *                 Adjustment (CSI-2)" in the datasheet)
+ * @min_SHR: Minimum SHR register value (see "Shutter Setting (CSI-2)" in the
+ *           datasheet)
+ * @max_fps: Maximum frames per second
+ * @nocpiop: Number of clocks per internal offset period (see "Integration Time
+ *           in Each Readout Drive Mode (CSI-2)" in the datasheet)
  */
 struct imx274_frmfmt {
 	struct v4l2_frmsize_discrete size;
+	const struct reg_8 *init_regs;
+	int min_frame_len;
+	int min_SHR;
+	int max_fps;
+	int nocpiop;
 };
 
 /*
@@ -476,58 +494,35 @@ static const struct reg_8 imx274_tp_regs[] = {
 	{IMX274_TABLE_END, 0x00}
 };
 
-static const struct reg_8 *mode_table[] = {
-	[IMX274_MODE_3840X2160]		= imx274_mode1_3840x2160_raw10,
-	[IMX274_MODE_1920X1080]		= imx274_mode3_1920x1080_raw10,
-	[IMX274_MODE_1280X720]		= imx274_mode5_1280x720_raw10,
-};
-
-/*
- * imx274 format related structure
- */
+/* nocpiop happens to be the same number for the implemented modes */
 static const struct imx274_frmfmt imx274_formats[] = {
-	{ {3840, 2160} },
-	{ {1920, 1080} },
-	{ {1280,  720} },
-};
-
-/*
- * minimal frame length for each mode
- * refer to datasheet section "Frame Rate Adjustment (CSI-2)"
- */
-static const int min_frame_len[] = {
-	4550, /* mode 1, 4K */
-	2310, /* mode 3, 1080p */
-	2310 /* mode 5, 720p */
-};
-
-/*
- * minimal numbers of SHR register
- * refer to datasheet table "Shutter Setting (CSI-2)"
- */
-static const int min_SHR[] = {
-	12, /* mode 1, 4K */
-	8, /* mode 3, 1080p */
-	8 /* mode 5, 720p */
-};
-
-static const int max_frame_rate[] = {
-	60, /* mode 1 , 4K */
-	120, /* mode 3, 1080p */
-	120 /* mode 5, 720p */
-};
-
-/*
- * Number of clocks per internal offset period
- * a constant based on mode
- * refer to section "Integration Time in Each Readout Drive Mode (CSI-2)"
- * in the datasheet
- * for the implemented 3 modes, it happens to be the same number
- */
-static const int nocpiop[] = {
-	112, /* mode 1 , 4K */
-	112, /* mode 3, 1080p */
-	112 /* mode 5, 720p */
+	{
+		/* mode 1, 4K */
+		.size = {3840, 2160},
+		.init_regs = imx274_mode1_3840x2160_raw10,
+		.min_frame_len = 4550,
+		.min_SHR = 12,
+		.max_fps = 60,
+		.nocpiop = 112,
+	},
+	{
+		/* mode 3, 1080p */
+		.size = {1920, 1080},
+		.init_regs = imx274_mode3_1920x1080_raw10,
+		.min_frame_len = 2310,
+		.min_SHR = 8,
+		.max_fps = 120,
+		.nocpiop = 112,
+	},
+	{
+		/* mode 5, 720p */
+		.size = {1280, 720},
+		.init_regs = imx274_mode5_1280x720_raw10,
+		.min_frame_len = 2310,
+		.min_SHR = 8,
+		.max_fps = 120,
+		.nocpiop = 112,
+	},
 };
 
 /*
@@ -557,6 +552,8 @@ struct imx274_ctrls {
  * @regmap: Pointer to regmap structure
  * @reset_gpio: Pointer to reset gpio
  * @lock: Mutex structure
+ * @mode: Parameters for the selected readout mode
+ *        (points to imx274_formats[mode_index])
  * @mode_index: Resolution mode index
  */
 struct stimx274 {
@@ -569,6 +566,7 @@ struct stimx274 {
 	struct regmap *regmap;
 	struct gpio_desc *reset_gpio;
 	struct mutex lock; /* mutex lock for operations */
+	const struct imx274_frmfmt *mode;
 	u32 mode_index;
 };
 
@@ -704,18 +702,12 @@ static int imx274_write_table(struct stimx274 *priv, const struct reg_8 table[])
 }
 
 /*
- * imx274_mode_regs - Function for set mode registers per mode index
+ * Set mode registers to start stream.
  * @priv: Pointer to device structure
- * @mode: Mode index value
- *
- * This is used to start steam per mode index.
- * mode = 0, start stream for sensor Mode 1: 4K/raw10
- * mode = 1, start stream for sensor Mode 3: 1080p/raw10
- * mode = 2, start stream for sensor Mode 5: 720p/raw10
  *
  * Return: 0 on success, errors otherwise
  */
-static int imx274_mode_regs(struct stimx274 *priv, int mode)
+static int imx274_mode_regs(struct stimx274 *priv)
 {
 	int err = 0;
 
@@ -727,7 +719,7 @@ static int imx274_mode_regs(struct stimx274 *priv, int mode)
 	if (err)
 		return err;
 
-	err = imx274_write_table(priv, mode_table[mode]);
+	err = imx274_write_table(priv, priv->mode->init_regs);
 
 	return err;
 }
@@ -889,6 +881,7 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
 	}
 
 	imx274->mode_index = index;
+	imx274->mode = &imx274_formats[index];
 
 	if (fmt->width > IMX274_MAX_WIDTH)
 		fmt->width = IMX274_MAX_WIDTH;
@@ -1042,7 +1035,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 
 	if (on) {
 		/* load mode registers */
-		ret = imx274_mode_regs(imx274, imx274->mode_index);
+		ret = imx274_mode_regs(imx274);
 		if (ret)
 			goto fail;
 
@@ -1146,14 +1139,14 @@ static int imx274_clamp_coarse_time(struct stimx274 *priv, u32 *val,
 	if (err)
 		return err;
 
-	if (*frame_length < min_frame_len[priv->mode_index])
-		*frame_length = min_frame_len[priv->mode_index];
+	if (*frame_length < priv->mode->min_frame_len)
+		*frame_length =  priv->mode->min_frame_len;
 
 	*val = *frame_length - *val; /* convert to raw shr */
 	if (*val > *frame_length - IMX274_SHR_LIMIT_CONST)
 		*val = *frame_length - IMX274_SHR_LIMIT_CONST;
-	else if (*val < min_SHR[priv->mode_index])
-		*val = min_SHR[priv->mode_index];
+	else if (*val < priv->mode->min_SHR)
+		*val = priv->mode->min_SHR;
 
 	return 0;
 }
@@ -1365,7 +1358,7 @@ static int imx274_set_exposure(struct stimx274 *priv, int val)
 	}
 
 	coarse_time = (IMX274_PIXCLK_CONST1 / IMX274_PIXCLK_CONST2 * val
-			- nocpiop[priv->mode_index]) / hmax;
+			- priv->mode->nocpiop) / hmax;
 
 	/* step 2: convert exposure_time into SHR value */
 
@@ -1375,7 +1368,7 @@ static int imx274_set_exposure(struct stimx274 *priv, int val)
 		goto fail;
 
 	priv->ctrls.exposure->val =
-			(coarse_time * hmax + nocpiop[priv->mode_index])
+			(coarse_time * hmax + priv->mode->nocpiop)
 			/ (IMX274_PIXCLK_CONST1 / IMX274_PIXCLK_CONST2);
 
 	dev_dbg(&priv->client->dev,
@@ -1529,10 +1522,9 @@ static int imx274_set_frame_interval(struct stimx274 *priv,
 				/ frame_interval.numerator);
 
 	/* boundary check */
-	if (req_frame_rate > max_frame_rate[priv->mode_index]) {
+	if (req_frame_rate > priv->mode->max_fps) {
 		frame_interval.numerator = 1;
-		frame_interval.denominator =
-					max_frame_rate[priv->mode_index];
+		frame_interval.denominator = priv->mode->max_fps;
 	} else if (req_frame_rate < IMX274_MIN_FRAME_RATE) {
 		frame_interval.numerator = 1;
 		frame_interval.denominator = IMX274_MIN_FRAME_RATE;
@@ -1634,8 +1626,9 @@ static int imx274_probe(struct i2c_client *client,
 
 	/* initialize format */
 	imx274->mode_index = IMX274_MODE_3840X2160;
-	imx274->format.width = imx274_formats[0].size.width;
-	imx274->format.height = imx274_formats[0].size.height;
+	imx274->mode = &imx274_formats[imx274->mode_index];
+	imx274->format.width = imx274->mode->size.width;
+	imx274->format.height = imx274->mode->size.height;
 	imx274->format.field = V4L2_FIELD_NONE;
 	imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
 	imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
-- 
2.7.4

[PATCH v3 3/7] media: imx274: get rid of mode_index

[Luca Ceresoli]

After restructuring struct imx274_frmfmt, the mode_index field is
still in use only for two dev_dbg() calls in imx274_s_stream(). Let's
remove it and avoid duplicated information.

Replacing the first usage requires some rather annoying but trivial
pointer math. The other one can be removed entirely since it would
print the same value anyway.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

---
Changed v2 -> v3:
 - really fix dev_dbg() format mismatch warning for both 32 and 64 bit

Changed v1 -> v2:
 - add "media: " prefix to commit message
 - fix dev_dbg() format mismatch warning
   ("warning: format ‘%ld’ expects argument of type ‘long int’, but argument 6 has type ‘int’")
 - slightly improve commit message
---
 drivers/media/i2c/imx274.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 2ec31ae4e60d..f075715ffced 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -553,8 +553,6 @@ struct imx274_ctrls {
  * @reset_gpio: Pointer to reset gpio
  * @lock: Mutex structure
  * @mode: Parameters for the selected readout mode
- *        (points to imx274_formats[mode_index])
- * @mode_index: Resolution mode index
  */
 struct stimx274 {
 	struct v4l2_subdev sd;
@@ -567,7 +565,6 @@ struct stimx274 {
 	struct gpio_desc *reset_gpio;
 	struct mutex lock; /* mutex lock for operations */
 	const struct imx274_frmfmt *mode;
-	u32 mode_index;
 };
 
 /*
@@ -880,7 +877,6 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
 		index = 0;
 	}
 
-	imx274->mode_index = index;
 	imx274->mode = &imx274_formats[index];
 
 	if (fmt->width > IMX274_MAX_WIDTH)
@@ -1028,8 +1024,9 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 	struct stimx274 *imx274 = to_imx274(sd);
 	int ret = 0;
 
-	dev_dbg(&imx274->client->dev, "%s : %s, mode index = %d\n", __func__,
-		on ? "Stream Start" : "Stream Stop", imx274->mode_index);
+	dev_dbg(&imx274->client->dev, "%s : %s, mode index = %td\n", __func__,
+		on ? "Stream Start" : "Stream Stop",
+		imx274->mode - &imx274_formats[0]);
 
 	mutex_lock(&imx274->lock);
 
@@ -1068,8 +1065,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 	}
 
 	mutex_unlock(&imx274->lock);
-	dev_dbg(&imx274->client->dev,
-		"%s : Done: mode = %d\n", __func__, imx274->mode_index);
+	dev_dbg(&imx274->client->dev, "%s : Done\n", __func__);
 	return 0;
 
 fail:
@@ -1625,8 +1621,7 @@ static int imx274_probe(struct i2c_client *client,
 	mutex_init(&imx274->lock);
 
 	/* initialize format */
-	imx274->mode_index = IMX274_MODE_3840X2160;
-	imx274->mode = &imx274_formats[imx274->mode_index];
+	imx274->mode = &imx274_formats[IMX274_MODE_3840X2160];
 	imx274->format.width = imx274->mode->size.width;
 	imx274->format.height = imx274->mode->size.height;
 	imx274->format.field = V4L2_FIELD_NONE;
-- 
2.7.4

[PATCH v3 4/7] media: imx274: actually use IMX274_DEFAULT_MODE

[Luca Ceresoli]

IMX274_DEFAULT_MODE is defined but not used. Start using it, so the
default can be more easily changed without digging into the code.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

---
Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index f075715ffced..ceeec97cd330 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1621,7 +1621,7 @@ static int imx274_probe(struct i2c_client *client,
 	mutex_init(&imx274->lock);
 
 	/* initialize format */
-	imx274->mode = &imx274_formats[IMX274_MODE_3840X2160];
+	imx274->mode = &imx274_formats[IMX274_DEFAULT_MODE];
 	imx274->format.width = imx274->mode->size.width;
 	imx274->format.height = imx274->mode->size.height;
 	imx274->format.field = V4L2_FIELD_NONE;
-- 
2.7.4

[PATCH v3 5/7] media: imx274: simplify imx274_write_table()

[Luca Ceresoli]

imx274_write_table() is a mere wrapper (and the only user) to
imx274_regmap_util_write_table_8(). Remove this useless indirection by
merging the two functions into one.

Also get rid of the wait_ms_addr and end_addr parameters since it does
not make any sense to give them any values other than
IMX274_TABLE_WAIT_MS and IMX274_TABLE_END.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

---
Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index ceeec97cd330..48343c2ade83 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -597,20 +597,18 @@ static inline struct stimx274 *to_imx274(struct v4l2_subdev *sd)
 }
 
 /*
- * imx274_regmap_util_write_table_8 - Function for writing register table
- * @regmap: Pointer to device reg map structure
- * @table: Table containing register values
- * @wait_ms_addr: Flag for performing delay
- * @end_addr: Flag for incating end of table
+ * Writing a register table
+ *
+ * @priv: Pointer to device
+ * @table: Table containing register values (with optional delays)
  *
  * This is used to write register table into sensor's reg map.
  *
  * Return: 0 on success, errors otherwise
  */
-static int imx274_regmap_util_write_table_8(struct regmap *regmap,
-					    const struct reg_8 table[],
-					    u16 wait_ms_addr, u16 end_addr)
+static int imx274_write_table(struct stimx274 *priv, const struct reg_8 table[])
 {
+	struct regmap *regmap = priv->regmap;
 	int err = 0;
 	const struct reg_8 *next;
 	u8 val;
@@ -622,8 +620,8 @@ static int imx274_regmap_util_write_table_8(struct regmap *regmap,
 
 	for (next = table;; next++) {
 		if ((next->addr != range_start + range_count) ||
-		    (next->addr == end_addr) ||
-		    (next->addr == wait_ms_addr) ||
+		    (next->addr == IMX274_TABLE_END) ||
+		    (next->addr == IMX274_TABLE_WAIT_MS) ||
 		    (range_count == max_range_vals)) {
 			if (range_count == 1)
 				err = regmap_write(regmap,
@@ -642,10 +640,10 @@ static int imx274_regmap_util_write_table_8(struct regmap *regmap,
 			range_count = 0;
 
 			/* Handle special address values */
-			if (next->addr == end_addr)
+			if (next->addr == IMX274_TABLE_END)
 				break;
 
-			if (next->addr == wait_ms_addr) {
+			if (next->addr == IMX274_TABLE_WAIT_MS) {
 				msleep_range(next->val);
 				continue;
 			}
@@ -692,12 +690,6 @@ static inline int imx274_write_reg(struct stimx274 *priv, u16 addr, u8 val)
 	return err;
 }
 
-static int imx274_write_table(struct stimx274 *priv, const struct reg_8 table[])
-{
-	return imx274_regmap_util_write_table_8(priv->regmap,
-		table, IMX274_TABLE_WAIT_MS, IMX274_TABLE_END);
-}
-
 /*
  * Set mode registers to start stream.
  * @priv: Pointer to device structure
-- 
2.7.4

[PATCH v3 6/7] media: imx274: add helper function to fill a reg_8 table chunk

[Luca Ceresoli]

Tables of struct reg_8 are used to simplify multi-byte register
assignment. However filling these snippets with values computed at
runtime is currently implemented by very similar functions doing the
needed shift & mask manipulation.

Replace all those functions with a unique helper function to fill
reg_8 tables in a simple and clean way.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

---
Changed v2 -> v3:
 - minor reformatting in prepare_reg() documentation

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 90 ++++++++++++++++++++++++++++------------------
 1 file changed, 55 insertions(+), 35 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 48343c2ade83..e5ba19b97083 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -597,6 +597,58 @@ static inline struct stimx274 *to_imx274(struct v4l2_subdev *sd)
 }
 
 /*
+ * Fill consecutive reg_8 items in order to set a multibyte register.
+ *
+ * The sensor has many 2-bytes registers and a 3-byte register. This
+ * simplifies code to set them by filling consecutive entries of a
+ * struct reg_8 table with the data to set a register.
+ *
+ * The number of table entries that is filled is the minimum needed
+ * for the given number of bits (i.e. nbits / 8, rounded up). If nbits
+ * is not a multiple of 8, extra bits in the most significant byte are
+ * zeroed.
+ *
+ * Example:
+ *   Calling prepare_reg(&regs[10], 0x3000, 0xcafe, 12) will set:
+ *   regs[10] = { .addr = 0x3000, .val = 0xfe }
+ *   regs[11] = { .addr = 0x3001, .val = 0x0a }
+ *
+ * @table_base: Pointer to the first reg_8 struct to be filled.  The
+ *              following entries will also be set, make sure they are
+ *              properly allocated!
+ * @addr_lsb: Address of the LSB register.  Other registers must be
+ *            consecutive, least-to-most significant.
+ * @value: Value to be written to the register.
+ * @nbits: Number of bits to write (range: [1..24])
+ */
+static void prepare_reg(struct reg_8 *table_base,
+			u16 addr_lsb,
+			u32 value,
+			size_t nbits)
+{
+	struct reg_8 *cmd = table_base;
+	u16 addr = addr_lsb;
+	size_t bits; /* how many bits at this round */
+
+	WARN_ON(nbits > 24);
+
+	if (nbits > 24)
+		nbits = 24;
+
+	while (nbits > 0) {
+		bits = min_t(size_t, 8, nbits);
+
+		cmd->addr = addr;
+		cmd->val = value & ((1 << bits) - 1);
+
+		nbits -= bits;
+		value >>= 8;
+		addr++;
+		cmd++;
+	}
+}
+
+/*
  * Writing a register table
  *
  * @priv: Pointer to device
@@ -1163,15 +1215,6 @@ static int imx274_set_digital_gain(struct stimx274 *priv, u32 dgain)
 				reg_val & IMX274_MASK_LSB_4_BITS);
 }
 
-static inline void imx274_calculate_gain_regs(struct reg_8 regs[2], u16 gain)
-{
-	regs->addr = IMX274_ANALOG_GAIN_ADDR_MSB;
-	regs->val = (gain >> IMX274_SHIFT_8_BITS) & IMX274_MASK_LSB_3_BITS;
-
-	(regs + 1)->addr = IMX274_ANALOG_GAIN_ADDR_LSB;
-	(regs + 1)->val = (gain) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_gain - Function called when setting gain
  * @priv: Pointer to device structure
@@ -1229,7 +1272,7 @@ static int imx274_set_gain(struct stimx274 *priv, struct v4l2_ctrl *ctrl)
 	if (gain_reg > IMX274_GAIN_REG_MAX)
 		gain_reg = IMX274_GAIN_REG_MAX;
 
-	imx274_calculate_gain_regs(reg_list, (u16)gain_reg);
+	prepare_reg(reg_list, IMX274_ANALOG_GAIN_ADDR_LSB, gain_reg, 11);
 
 	for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
 		err = imx274_write_reg(priv, reg_list[i].addr,
@@ -1258,16 +1301,6 @@ static int imx274_set_gain(struct stimx274 *priv, struct v4l2_ctrl *ctrl)
 	return err;
 }
 
-static inline void imx274_calculate_coarse_time_regs(struct reg_8 regs[2],
-						     u32 coarse_time)
-{
-	regs->addr = IMX274_SHR_REG_MSB;
-	regs->val = (coarse_time >> IMX274_SHIFT_8_BITS)
-			& IMX274_MASK_LSB_8_BITS;
-	(regs + 1)->addr = IMX274_SHR_REG_LSB;
-	(regs + 1)->val = (coarse_time) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_coarse_time - Function called when setting SHR value
  * @priv: Pointer to device structure
@@ -1292,7 +1325,7 @@ static int imx274_set_coarse_time(struct stimx274 *priv, u32 *val)
 		goto fail;
 
 	/* prepare SHR registers */
-	imx274_calculate_coarse_time_regs(reg_list, coarse_time);
+	prepare_reg(reg_list, IMX274_SHR_REG_LSB, coarse_time, 16);
 
 	/* write to SHR registers */
 	for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
@@ -1429,19 +1462,6 @@ static int imx274_set_test_pattern(struct stimx274 *priv, int val)
 	return err;
 }
 
-static inline void imx274_calculate_frame_length_regs(struct reg_8 regs[3],
-						      u32 frame_length)
-{
-	regs->addr = IMX274_VMAX_REG_1;
-	regs->val = (frame_length >> IMX274_SHIFT_16_BITS)
-			& IMX274_MASK_LSB_4_BITS;
-	(regs + 1)->addr = IMX274_VMAX_REG_2;
-	(regs + 1)->val = (frame_length >> IMX274_SHIFT_8_BITS)
-			& IMX274_MASK_LSB_8_BITS;
-	(regs + 2)->addr = IMX274_VMAX_REG_3;
-	(regs + 2)->val = (frame_length) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_frame_length - Function called when setting frame length
  * @priv: Pointer to device structure
@@ -1463,7 +1483,7 @@ static int imx274_set_frame_length(struct stimx274 *priv, u32 val)
 
 	frame_length = (u32)val;
 
-	imx274_calculate_frame_length_regs(reg_list, frame_length);
+	prepare_reg(reg_list, IMX274_VMAX_REG_3, frame_length, 20);
 	for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
 		err = imx274_write_reg(priv, reg_list[i].addr,
 				       reg_list[i].val);
-- 
2.7.4

[PATCH v3 7/7] media: imx274: add SELECTION support for cropping

[Luca Ceresoli]

Currently this driver does not support cropping. The supported modes
are the following, all capturing the entire area:

 - 3840x2160, 1:1 binning (native sensor resolution)
 - 1920x1080, 2:1 binning
 - 1280x720,  3:1 binning

The set_fmt callback chooses among these 3 configurations the one that
matches the requested format.

Adding support to VIDIOC_SUBDEV_G_SELECTION and
VIDIOC_SUBDEV_S_SELECTION involved a complete rewrite of set_fmt,
which now chooses the most appropriate binning based on the ratio
between the previously-set cropping size and the format size being
requested.

Also change the names in enum imx274_mode from being resolution-based
to being binning-based (e.g. from IMX274_MODE_1920X1080 to
IMX274_MODE_BINNING_2_1). Without cropping, the two naming are
equivalent. With cropping, the resolution could be different,
e.g. using 2:1 binning mode to crop 1200x960 and output a 600x480
format. Using binning in the names avoids any misunderstanding.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

---
Changed v2 -> v3:
 - Remove hard-coded HMAX reg setting from all modes, not only the
   first one. HMAX is always computed and set in s_stream now.
 - Reword commit log message.

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 272 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 192 insertions(+), 80 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index e5ba19b97083..f5819ce99e98 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -19,6 +19,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/kernel.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/gpio.h>
@@ -74,7 +75,7 @@
  */
 #define IMX274_MIN_EXPOSURE_TIME		(4 * 260 / 72)
 
-#define IMX274_DEFAULT_MODE			IMX274_MODE_3840X2160
+#define IMX274_DEFAULT_MODE			IMX274_MODE_BINNING_OFF
 #define IMX274_MAX_WIDTH			(3840)
 #define IMX274_MAX_HEIGHT			(2160)
 #define IMX274_MAX_FRAME_RATE			(120)
@@ -111,6 +112,20 @@
 #define IMX274_SHR_REG_LSB			0x300C /* SHR */
 #define IMX274_SVR_REG_MSB			0x300F /* SVR */
 #define IMX274_SVR_REG_LSB			0x300E /* SVR */
+#define IMX274_HTRIM_EN_REG			0x3037
+#define IMX274_HTRIM_START_REG_LSB		0x3038
+#define IMX274_HTRIM_START_REG_MSB		0x3039
+#define IMX274_HTRIM_END_REG_LSB		0x303A
+#define IMX274_HTRIM_END_REG_MSB		0x303B
+#define IMX274_VWIDCUTEN_REG			0x30DD
+#define IMX274_VWIDCUT_REG_LSB			0x30DE
+#define IMX274_VWIDCUT_REG_MSB			0x30DF
+#define IMX274_VWINPOS_REG_LSB			0x30E0
+#define IMX274_VWINPOS_REG_MSB			0x30E1
+#define IMX274_WRITE_VSIZE_REG_LSB		0x3130
+#define IMX274_WRITE_VSIZE_REG_MSB		0x3131
+#define IMX274_Y_OUT_SIZE_REG_LSB		0x3132
+#define IMX274_Y_OUT_SIZE_REG_MSB		0x3133
 #define IMX274_VMAX_REG_1			0x30FA /* VMAX, MSB */
 #define IMX274_VMAX_REG_2			0x30F9 /* VMAX */
 #define IMX274_VMAX_REG_3			0x30F8 /* VMAX, LSB */
@@ -141,9 +156,9 @@ static const struct regmap_config imx274_regmap_config = {
 };
 
 enum imx274_mode {
-	IMX274_MODE_3840X2160,
-	IMX274_MODE_1920X1080,
-	IMX274_MODE_1280X720,
+	IMX274_MODE_BINNING_OFF,
+	IMX274_MODE_BINNING_2_1,
+	IMX274_MODE_BINNING_3_1,
 };
 
 /*
@@ -215,31 +230,14 @@ static const struct reg_8 imx274_mode1_3840x2160_raw10[] = {
 	{0x3004, 0x01},
 	{0x3005, 0x01},
 	{0x3006, 0x00},
-	{0x3007, 0x02},
+	{0x3007, 0xa2},
 
 	{0x3018, 0xA2}, /* output XVS, HVS */
 
 	{0x306B, 0x05},
 	{0x30E2, 0x01},
-	{0x30F6, 0x07}, /* HMAX, 263 */
-	{0x30F7, 0x01}, /* HMAX */
-
-	{0x30dd, 0x01}, /* crop to 2160 */
-	{0x30de, 0x06},
-	{0x30df, 0x00},
-	{0x30e0, 0x12},
-	{0x30e1, 0x00},
-	{0x3037, 0x01}, /* to crop to 3840 */
-	{0x3038, 0x0c},
-	{0x3039, 0x00},
-	{0x303a, 0x0c},
-	{0x303b, 0x0f},
 
 	{0x30EE, 0x01},
-	{0x3130, 0x86},
-	{0x3131, 0x08},
-	{0x3132, 0x7E},
-	{0x3133, 0x08},
 	{0x3342, 0x0A},
 	{0x3343, 0x00},
 	{0x3344, 0x16},
@@ -273,32 +271,14 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
 	{0x3004, 0x02},
 	{0x3005, 0x21},
 	{0x3006, 0x00},
-	{0x3007, 0x11},
+	{0x3007, 0xb1},
 
 	{0x3018, 0xA2}, /* output XVS, HVS */
 
 	{0x306B, 0x05},
 	{0x30E2, 0x02},
 
-	{0x30F6, 0x04}, /* HMAX, 260 */
-	{0x30F7, 0x01}, /* HMAX */
-
-	{0x30dd, 0x01}, /* to crop to 1920x1080 */
-	{0x30de, 0x05},
-	{0x30df, 0x00},
-	{0x30e0, 0x04},
-	{0x30e1, 0x00},
-	{0x3037, 0x01},
-	{0x3038, 0x0c},
-	{0x3039, 0x00},
-	{0x303a, 0x0c},
-	{0x303b, 0x0f},
-
 	{0x30EE, 0x01},
-	{0x3130, 0x4E},
-	{0x3131, 0x04},
-	{0x3132, 0x46},
-	{0x3133, 0x04},
 	{0x3342, 0x0A},
 	{0x3343, 0x00},
 	{0x3344, 0x1A},
@@ -331,31 +311,14 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
 	{0x3004, 0x03},
 	{0x3005, 0x31},
 	{0x3006, 0x00},
-	{0x3007, 0x09},
+	{0x3007, 0xa9},
 
 	{0x3018, 0xA2}, /* output XVS, HVS */
 
 	{0x306B, 0x05},
 	{0x30E2, 0x03},
 
-	{0x30F6, 0x04}, /* HMAX, 260 */
-	{0x30F7, 0x01}, /* HMAX */
-
-	{0x30DD, 0x01},
-	{0x30DE, 0x07},
-	{0x30DF, 0x00},
-	{0x40E0, 0x04},
-	{0x30E1, 0x00},
-	{0x3030, 0xD4},
-	{0x3031, 0x02},
-	{0x3032, 0xD0},
-	{0x3033, 0x02},
-
 	{0x30EE, 0x01},
-	{0x3130, 0xE2},
-	{0x3131, 0x02},
-	{0x3132, 0xDE},
-	{0x3133, 0x02},
 	{0x3342, 0x0A},
 	{0x3343, 0x00},
 	{0x3344, 0x1B},
@@ -561,6 +524,7 @@ struct stimx274 {
 	struct imx274_ctrls ctrls;
 	struct v4l2_mbus_framefmt format;
 	struct v4l2_fract frame_interval;
+	struct v4l2_rect crop;
 	struct regmap *regmap;
 	struct gpio_desc *reset_gpio;
 	struct mutex lock; /* mutex lock for operations */
@@ -901,36 +865,30 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
 {
 	struct v4l2_mbus_framefmt *fmt = &format->format;
 	struct stimx274 *imx274 = to_imx274(sd);
-	struct i2c_client *client = imx274->client;
-	int index;
+	struct device *dev = &imx274->client->dev;
+	unsigned int ratio; /* Binning ratio */
 
-	dev_dbg(&client->dev,
-		"%s: width = %d height = %d code = %d\n",
-		__func__, fmt->width, fmt->height, fmt->code);
+	dev_dbg(dev, "%s: request of  %dx%d (%s)\n",
+		__func__, fmt->width, fmt->height,
+		V4L2_SUBDEV_FORMAT_ACTIVE ? "ACTIVE" : "TRY");
 
 	mutex_lock(&imx274->lock);
 
-	for (index = 0; index < ARRAY_SIZE(imx274_formats); index++) {
-		if (imx274_formats[index].size.width == fmt->width &&
-		    imx274_formats[index].size.height == fmt->height)
+	/* Find ratio (maximize output resolution). Fallback to 1:1. */
+	for (ratio = 3; ratio > 1; ratio--)
+		if (fmt->width <= DIV_ROUND_UP(imx274->crop.width, ratio) &&
+		    fmt->height <= DIV_ROUND_UP(imx274->crop.height, ratio))
 			break;
-	}
-
-	if (index >= ARRAY_SIZE(imx274_formats)) {
-		/* default to first format */
-		index = 0;
-	}
 
-	imx274->mode = &imx274_formats[index];
+	imx274->mode = &imx274_formats[ratio - 1];
 
-	if (fmt->width > IMX274_MAX_WIDTH)
-		fmt->width = IMX274_MAX_WIDTH;
-	if (fmt->height > IMX274_MAX_HEIGHT)
-		fmt->height = IMX274_MAX_HEIGHT;
-	fmt->width = fmt->width & (~IMX274_MASK_LSB_2_BITS);
-	fmt->height = fmt->height & (~IMX274_MASK_LSB_2_BITS);
+	fmt->width = imx274->crop.width / ratio;
+	fmt->height = imx274->crop.height / ratio;
 	fmt->field = V4L2_FIELD_NONE;
 
+	dev_dbg(dev, "%s: adjusted to %dx%d (%d:1 binning)\n",
+		__func__, fmt->width, fmt->height, ratio);
+
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
 		cfg->try_fmt = *fmt;
 	else
@@ -940,6 +898,152 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int imx274_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_pad_config *cfg,
+				struct v4l2_subdev_selection *sel)
+{
+	struct stimx274 *imx274 = to_imx274(sd);
+
+	if (sel->pad != 0)
+		return -EINVAL;
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.left = 0;
+		sel->r.top = 0;
+		sel->r.width = IMX274_MAX_WIDTH;
+		sel->r.height = IMX274_MAX_HEIGHT;
+		return 0;
+	case V4L2_SEL_TGT_CROP:
+		if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+			mutex_lock(&imx274->lock);
+			sel->r = imx274->crop;
+			mutex_unlock(&imx274->lock);
+		} else {
+			sel->r = cfg->try_crop;
+		}
+		return 0;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int imx274_set_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_pad_config *cfg,
+				struct v4l2_subdev_selection *sel)
+{
+	struct stimx274 *imx274 = to_imx274(sd);
+	struct device *dev = &imx274->client->dev;
+	struct v4l2_mbus_framefmt *tgt_fmt;
+	struct v4l2_rect *tgt_crop;
+	struct v4l2_rect new_crop;
+
+	/*
+	 * h_step could be 12 or 24 depending on the binning. But we
+	 * won't know the binning until we choose the mode later in
+	 * imx274_set_fmt(). Thus let's be safe and use the most
+	 * conservative value in all cases.
+	 */
+	const int h_step = 24;
+
+	dev_dbg(dev, "%s: request of  (%d,%d)%dx%d (%s)\n", __func__,
+		sel->r.left, sel->r.top, sel->r.width, sel->r.height,
+		V4L2_SUBDEV_FORMAT_ACTIVE ? "ACTIVE" : "TRY");
+
+	if (sel->target != V4L2_SEL_TGT_CROP || sel->pad != 0)
+		return -EINVAL;
+
+	new_crop.width = rounddown(min_t(u32, sel->r.width, IMX274_MAX_WIDTH),
+				   h_step);
+
+	/* Constraint: HTRIMMING_END - HTRIMMING_START >= 144 */
+	if (new_crop.width < 144)
+		new_crop.width = 144;
+
+	new_crop.left = min_t(u32, roundup(sel->r.left, h_step),
+			      IMX274_MAX_WIDTH - new_crop.width);
+
+	new_crop.height =
+		ALIGN(min_t(u32, sel->r.height, IMX274_MAX_HEIGHT), 2);
+
+	new_crop.top = min_t(u32, ALIGN(sel->r.top, 2),
+			     IMX274_MAX_HEIGHT - new_crop.height);
+
+	dev_dbg(dev, "%s: adjusted to (%d,%d)%dx%d", __func__,
+		new_crop.left, new_crop.top, new_crop.width, new_crop.height);
+
+	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
+		tgt_crop = &cfg->try_crop;
+		tgt_fmt = &cfg->try_fmt;
+	} else {
+		tgt_crop = &imx274->crop;
+		tgt_fmt = &imx274->format;
+	}
+
+	mutex_lock(&imx274->lock);
+
+	if (new_crop.width != tgt_crop->width ||
+	    new_crop.height != tgt_crop->height) {
+		/* crop size changed, reset the output image size */
+		tgt_fmt->width = new_crop.width;
+		tgt_fmt->height = new_crop.height;
+		if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+			imx274->mode = &imx274_formats[IMX274_MODE_BINNING_OFF];
+	}
+
+	*tgt_crop = new_crop;
+
+	mutex_unlock(&imx274->lock);
+
+	sel->r = new_crop;
+
+	return 0;
+}
+
+static int imx274_set_trimming(struct stimx274 *imx274)
+{
+	u32 h_start;
+	u32 h_end;
+	u32 hmax;
+	u32 v_cut;
+	s32 v_pos;
+	u32 write_v_size;
+	u32 y_out_size;
+	struct reg_8 regs[17];
+
+	h_start = imx274->crop.left + 12;
+	h_end = h_start + imx274->crop.width;
+
+	/* Use the minimum allowed value of HMAX */
+	/* Note: except in mode 1, (width / 16 + 23) is always < hmax_min */
+	/* Note: 260 is the minimum HMAX in all implemented modes */
+	hmax = max_t(u32, 260, (imx274->crop.width) / 16 + 23);
+
+	/* invert v_pos if VFLIP */
+	v_pos = imx274->ctrls.vflip->cur.val ?
+		(-imx274->crop.top / 2) : (imx274->crop.top / 2);
+	v_cut = (IMX274_MAX_HEIGHT - imx274->crop.height) / 2;
+	write_v_size = imx274->crop.height + 22;
+	y_out_size   = imx274->crop.height + 14;
+
+	prepare_reg(&regs[0],  IMX274_HMAX_REG_LSB,        hmax,         16);
+	prepare_reg(&regs[2],  IMX274_HTRIM_EN_REG,        1,             1);
+	prepare_reg(&regs[3],  IMX274_HTRIM_START_REG_LSB, h_start,      13);
+	prepare_reg(&regs[5],  IMX274_HTRIM_END_REG_LSB,   h_end,        13);
+
+	prepare_reg(&regs[7],  IMX274_VWIDCUTEN_REG,       1,             1);
+	prepare_reg(&regs[8],  IMX274_VWIDCUT_REG_LSB,     v_cut,        11);
+	prepare_reg(&regs[10], IMX274_VWINPOS_REG_LSB,     v_pos,        12);
+	prepare_reg(&regs[12], IMX274_WRITE_VSIZE_REG_LSB, write_v_size, 13);
+	prepare_reg(&regs[14], IMX274_Y_OUT_SIZE_REG_LSB,  y_out_size,   13);
+
+	regs[16].addr = IMX274_TABLE_END;
+
+	return imx274_write_table(imx274, regs);
+}
+
 /**
  * imx274_g_frame_interval - Get the frame interval
  * @sd: Pointer to V4L2 Sub device structure
@@ -1080,6 +1184,10 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 		if (ret)
 			goto fail;
 
+		ret = imx274_set_trimming(imx274);
+		if (ret)
+			goto fail;
+
 		/*
 		 * update frame rate & expsoure. if the last mode is different,
 		 * HMAX could be changed. As the result, frame rate & exposure
@@ -1589,6 +1697,8 @@ static int imx274_set_frame_interval(struct stimx274 *priv,
 static const struct v4l2_subdev_pad_ops imx274_pad_ops = {
 	.get_fmt = imx274_get_fmt,
 	.set_fmt = imx274_set_fmt,
+	.get_selection = imx274_get_selection,
+	.set_selection = imx274_set_selection,
 };
 
 static const struct v4l2_subdev_video_ops imx274_video_ops = {
@@ -1641,6 +1751,8 @@ static int imx274_probe(struct i2c_client *client,
 	imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
 	imx274->frame_interval.numerator = 1;
 	imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
+	imx274->crop.width = imx274->mode->size.width;
+	imx274->crop.height = imx274->mode->size.height;
 
 	/* initialize regmap */
 	imx274->regmap = devm_regmap_init_i2c(client, &imx274_regmap_config);
-- 
2.7.4