[PATCH 00/13] imx274: add cropping and misc improvements

[PATCH 00/13] imx274: add cropping and misc improvements

[Luca Ceresoli]

Hi,

this patchset introduces cropping support for the Sony IMX274 sensor
using the SELECTION API, after several cleanups and general code
improvements.

Patches 1-6 clean up and restructure code in various places and are
pretty much independent from the cropping feature.

Patches 7-11 are further restructuring which are mostly useful to
implement cropping API in a cleaner way.

Patch 12 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.

Patch 13 implements cropping in the set_selection pad operation. On
the v4l2 side there is nothing special, it fit nicely in the existing
infrastructure. The most tricky part was respecting all the device
constraints on the horizontal crop.

Regards,
Luca

Luca Ceresoli (13):
  imx274: document reset delays more clearly
  imx274: fix typo in comment
  imx274: slightly simplify code
  imx274: remove unused data from struct imx274_frmfmt
  imx274: rename and reorder register address definitions
  imx274: remove non-indexed pointers from mode_table
  imx274: initialize format before v4l2 controls
  imx274: consolidate per-mode data in imx274_frmfmt
  imx274: get rid of mode_index
  imx274: actually use IMX274_DEFAULT_MODE
  imx274: simplify imx274_write_table()
  imx274: add helper function to fill a reg_8 table chunk
  imx274: add SELECTION support for cropping

 drivers/media/i2c/imx274.c | 600 ++++++++++++++++++++++++++-------------------
 1 file changed, 349 insertions(+), 251 deletions(-)

-- 
2.7.4

[PATCH 01/13] imx274: document reset delays more clearly

[Luca Ceresoli]

Document the unit to avoid having to look through the code to compute it.
Also clarify that these are min and max values.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 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 daec33f4196a..5e425db9204d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -87,7 +87,7 @@
 #define IMX274_SHR_LIMIT_CONST			(4)
 
 /*
- * Constants for sensor reset delay
+ * Min and max sensor reset delay (microseconds)
  */
 #define IMX274_RESET_DELAY1			(2000)
 #define IMX274_RESET_DELAY2			(2200)
-- 
2.7.4

[PATCH 02/13] imx274: fix typo in comment

[Luca Ceresoli]

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 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 5e425db9204d..dfd04edcdd48 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -971,7 +971,7 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
 	if (!ret) {
 		/*
 		 * exposure time range is decided by frame interval
-		 * need to update it after frame interal changes
+		 * need to update it after frame interval changes
 		 */
 		min = IMX274_MIN_EXPOSURE_TIME;
 		max = fi->interval.numerator * 1000000
-- 
2.7.4

[PATCH 03/13] imx274: slightly simplify code

[Luca Ceresoli]

imx274_s_frame_interval() already has a direct pointer to the v4l2
exposure control, so reuse it to simplify code.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 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 dfd04edcdd48..c5d00ade4d64 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -984,7 +984,7 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
 		}
 
 		/* update exposure time accordingly */
-		imx274_set_exposure(imx274, imx274->ctrls.exposure->val);
+		imx274_set_exposure(imx274, ctrl->val);
 
 		dev_dbg(&imx274->client->dev, "set frame interval to %uus\n",
 			fi->interval.numerator * 1000000
-- 
2.7.4

[PATCH 04/13] imx274: remove unused data from struct imx274_frmfmt

[Luca Ceresoli]

struct imx274_frmfmt is instantiated only in the imx274_formats[]
array, where imx274_formats[N].mode always equals N (via enum
imx274_mode).  So .mode carries no information, and unsurprisingly it
is never used.

mbus_code is never used because the 12 bit modes are not implemented.

The colorspace member is also never used, which is normal since the
imx274 sensor can output only one colorspace.

Let's get rid of all of them.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index c5d00ade4d64..9203d377cfe2 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -156,10 +156,7 @@ enum imx274_mode {
  * imx274 format related structure
  */
 struct imx274_frmfmt {
-	u32 mbus_code;
-	enum v4l2_colorspace colorspace;
 	struct v4l2_frmsize_discrete size;
-	enum imx274_mode mode;
 };
 
 /*
@@ -501,12 +498,9 @@ static const struct reg_8 *mode_table[] = {
  * imx274 format related structure
  */
 static const struct imx274_frmfmt imx274_formats[] = {
-	{MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB, {3840, 2160},
-		IMX274_MODE_3840X2160},
-	{MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB, {1920, 1080},
-		IMX274_MODE_1920X1080},
-	{MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB, {1280, 720},
-		IMX274_MODE_1280X720},
+	{ {3840, 2160} },
+	{ {1920, 1080} },
+	{ {1280,  720} },
 };
 
 /*
@@ -890,9 +884,8 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
 	int index;
 
 	dev_dbg(&client->dev,
-		"%s: width = %d height = %d code = %d mbus_code = %d\n",
-		__func__, fmt->width, fmt->height, fmt->code,
-		imx274_formats[imx274->mode_index].mbus_code);
+		"%s: width = %d height = %d code = %d\n",
+		__func__, fmt->width, fmt->height, fmt->code);
 
 	mutex_lock(&imx274->lock);
 
-- 
2.7.4

[PATCH 05/13] imx274: rename and reorder register address definitions

[Luca Ceresoli]

Most registers are defined using the name used in the datasheet.
E.g. the defines for the HMAX register are IMX274_HMAX_REG_*.

Rename the SHR and VMAX register accordingly. Also move them close to
related registers: SHR close to SVR, VMAX close to HMAX.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 9203d377cfe2..62a0d7af0e51 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -107,15 +107,15 @@
 /*
  * IMX274 register definitions
  */
-#define IMX274_FRAME_LENGTH_ADDR_1		0x30FA /* VMAX, MSB */
-#define IMX274_FRAME_LENGTH_ADDR_2		0x30F9 /* VMAX */
-#define IMX274_FRAME_LENGTH_ADDR_3		0x30F8 /* VMAX, LSB */
+#define IMX274_SHR_REG_MSB			0x300D /* SHR */
+#define IMX274_SHR_REG_LSB			0x300C /* SHR */
 #define IMX274_SVR_REG_MSB			0x300F /* SVR */
 #define IMX274_SVR_REG_LSB			0x300E /* SVR */
+#define IMX274_VMAX_REG_1			0x30FA /* VMAX, MSB */
+#define IMX274_VMAX_REG_2			0x30F9 /* VMAX */
+#define IMX274_VMAX_REG_3			0x30F8 /* VMAX, LSB */
 #define IMX274_HMAX_REG_MSB			0x30F7 /* HMAX */
 #define IMX274_HMAX_REG_LSB			0x30F6 /* HMAX */
-#define IMX274_COARSE_TIME_ADDR_MSB		0x300D /* SHR */
-#define IMX274_COARSE_TIME_ADDR_LSB		0x300C /* SHR */
 #define IMX274_ANALOG_GAIN_ADDR_LSB		0x300A /* ANALOG GAIN LSB */
 #define IMX274_ANALOG_GAIN_ADDR_MSB		0x300B /* ANALOG GAIN MSB */
 #define IMX274_DIGITAL_GAIN_REG			0x3012 /* Digital Gain */
@@ -1126,15 +1126,15 @@ static int imx274_get_frame_length(struct stimx274 *priv, u32 *val)
 	svr = (reg_val[1] << IMX274_SHIFT_8_BITS) + reg_val[0];
 
 	/* vmax */
-	err = imx274_read_reg(priv, IMX274_FRAME_LENGTH_ADDR_3, &reg_val[0]);
+	err = imx274_read_reg(priv, IMX274_VMAX_REG_3, &reg_val[0]);
 	if (err)
 		goto fail;
 
-	err = imx274_read_reg(priv, IMX274_FRAME_LENGTH_ADDR_2, &reg_val[1]);
+	err = imx274_read_reg(priv, IMX274_VMAX_REG_2, &reg_val[1]);
 	if (err)
 		goto fail;
 
-	err = imx274_read_reg(priv, IMX274_FRAME_LENGTH_ADDR_1, &reg_val[2]);
+	err = imx274_read_reg(priv, IMX274_VMAX_REG_1, &reg_val[2]);
 	if (err)
 		goto fail;
 
@@ -1293,10 +1293,10 @@ static int imx274_set_gain(struct stimx274 *priv, struct v4l2_ctrl *ctrl)
 static inline void imx274_calculate_coarse_time_regs(struct reg_8 regs[2],
 						     u32 coarse_time)
 {
-	regs->addr = IMX274_COARSE_TIME_ADDR_MSB;
+	regs->addr = IMX274_SHR_REG_MSB;
 	regs->val = (coarse_time >> IMX274_SHIFT_8_BITS)
 			& IMX274_MASK_LSB_8_BITS;
-	(regs + 1)->addr = IMX274_COARSE_TIME_ADDR_LSB;
+	(regs + 1)->addr = IMX274_SHR_REG_LSB;
 	(regs + 1)->val = (coarse_time) & IMX274_MASK_LSB_8_BITS;
 }
 
@@ -1464,13 +1464,13 @@ static int imx274_set_test_pattern(struct stimx274 *priv, int val)
 static inline void imx274_calculate_frame_length_regs(struct reg_8 regs[3],
 						      u32 frame_length)
 {
-	regs->addr = IMX274_FRAME_LENGTH_ADDR_1;
+	regs->addr = IMX274_VMAX_REG_1;
 	regs->val = (frame_length >> IMX274_SHIFT_16_BITS)
 			& IMX274_MASK_LSB_4_BITS;
-	(regs + 1)->addr = IMX274_FRAME_LENGTH_ADDR_2;
+	(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_FRAME_LENGTH_ADDR_3;
+	(regs + 2)->addr = IMX274_VMAX_REG_3;
 	(regs + 2)->val = (frame_length) & IMX274_MASK_LSB_8_BITS;
 }
 
-- 
2.7.4

[PATCH 06/13] imx274: remove non-indexed pointers from mode_table

[Luca Ceresoli]

mode_table[] has 3 members that are accessed based on their index, which
makes worth using an array.

The other members are always accessed with a constant index. This added
indirection gives no improvement and only makes code more verbose.

Remove these pointers from the array and access them directly.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 62a0d7af0e51..63fb94e7da37 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -144,12 +144,6 @@ enum imx274_mode {
 	IMX274_MODE_3840X2160,
 	IMX274_MODE_1920X1080,
 	IMX274_MODE_1280X720,
-
-	IMX274_MODE_START_STREAM_1,
-	IMX274_MODE_START_STREAM_2,
-	IMX274_MODE_START_STREAM_3,
-	IMX274_MODE_START_STREAM_4,
-	IMX274_MODE_STOP_STREAM
 };
 
 /*
@@ -486,12 +480,6 @@ 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_MODE_START_STREAM_1]	= imx274_start_1,
-	[IMX274_MODE_START_STREAM_2]	= imx274_start_2,
-	[IMX274_MODE_START_STREAM_3]	= imx274_start_3,
-	[IMX274_MODE_START_STREAM_4]	= imx274_start_4,
-	[IMX274_MODE_STOP_STREAM]	= imx274_stop,
 };
 
 /*
@@ -731,11 +719,11 @@ static int imx274_mode_regs(struct stimx274 *priv, int mode)
 {
 	int err = 0;
 
-	err = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_1]);
+	err = imx274_write_table(priv, imx274_start_1);
 	if (err)
 		return err;
 
-	err = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_2]);
+	err = imx274_write_table(priv, imx274_start_2);
 	if (err)
 		return err;
 
@@ -760,7 +748,7 @@ static int imx274_start_stream(struct stimx274 *priv)
 	 * give it 1 extra ms for margin
 	 */
 	msleep_range(11);
-	err = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_3]);
+	err = imx274_write_table(priv, imx274_start_3);
 	if (err)
 		return err;
 
@@ -770,7 +758,7 @@ static int imx274_start_stream(struct stimx274 *priv)
 	 * give it 1 extra ms for margin
 	 */
 	msleep_range(8);
-	err = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_4]);
+	err = imx274_write_table(priv, imx274_start_4);
 	if (err)
 		return err;
 
@@ -1081,8 +1069,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 			goto fail;
 	} else {
 		/* stop stream */
-		ret = imx274_write_table(imx274,
-					 mode_table[IMX274_MODE_STOP_STREAM]);
+		ret = imx274_write_table(imx274, imx274_stop);
 		if (ret)
 			goto fail;
 	}
@@ -1779,7 +1766,7 @@ static int imx274_remove(struct i2c_client *client)
 	struct stimx274 *imx274 = to_imx274(sd);
 
 	/* stop stream */
-	imx274_write_table(imx274, mode_table[IMX274_MODE_STOP_STREAM]);
+	imx274_write_table(imx274, imx274_stop);
 
 	v4l2_async_unregister_subdev(sd);
 	v4l2_ctrl_handler_free(&imx274->ctrls.handler);
-- 
2.7.4

[PATCH 07/13] 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>
---
 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 08/13] imx274: consolidate per-mode data in imx274_frmfmt

[Luca Ceresoli]

Data about for 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 becomes 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>
---
 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 09/13] 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 a rather annoying but trivial
computation. The other one can be removed entirely since it prints the
same value anyway.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 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..25907d0817a4 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 %lu\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

Re: [PATCH 09/13] imx274: get rid of mode_index

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3796 bytes --]

Hi Luca,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.16 next-20180413]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Luca-Ceresoli/imx274-add-cropping-and-misc-improvements/20180413-234459
base:   git://linuxtv.org/media_tree.git master
config: i386-randconfig-x017-201814 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:329:0,
                    from include/linux/kernel.h:14,
                    from include/linux/clk.h:16,
                    from drivers/media/i2c/imx274.c:22:
   drivers/media/i2c/imx274.c: In function 'imx274_s_stream':
>> drivers/media/i2c/imx274.c:1027:32: warning: format '%lu' expects argument of type 'long unsigned int', but argument 6 has type 'int' [-Wformat=]
     dev_dbg(&imx274->client->dev, "%s : %s, mode index %lu\n", __func__,
                                   ^
   drivers/media/i2c/imx274.c:1029:3:
      imx274->mode - &imx274_formats[0]);
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:135:39: note: in definition of macro 'dynamic_dev_dbg'
      __dynamic_dev_dbg(&descriptor, dev, fmt, \
                                          ^~~
>> drivers/media/i2c/imx274.c:1027:2: note: in expansion of macro 'dev_dbg'
     dev_dbg(&imx274->client->dev, "%s : %s, mode index %lu\n", __func__,
     ^~~~~~~

vim +1027 drivers/media/i2c/imx274.c

  1011	
  1012	/**
  1013	 * imx274_s_stream - It is used to start/stop the streaming.
  1014	 * @sd: V4L2 Sub device
  1015	 * @on: Flag (True / False)
  1016	 *
  1017	 * This function controls the start or stop of streaming for the
  1018	 * imx274 sensor.
  1019	 *
  1020	 * Return: 0 on success, errors otherwise
  1021	 */
  1022	static int imx274_s_stream(struct v4l2_subdev *sd, int on)
  1023	{
  1024		struct stimx274 *imx274 = to_imx274(sd);
  1025		int ret = 0;
  1026	
> 1027		dev_dbg(&imx274->client->dev, "%s : %s, mode index %lu\n", __func__,
  1028			on ? "Stream Start" : "Stream Stop",
  1029			imx274->mode - &imx274_formats[0]);
  1030	
  1031		mutex_lock(&imx274->lock);
  1032	
  1033		if (on) {
  1034			/* load mode registers */
  1035			ret = imx274_mode_regs(imx274);
  1036			if (ret)
  1037				goto fail;
  1038	
  1039			/*
  1040			 * update frame rate & expsoure. if the last mode is different,
  1041			 * HMAX could be changed. As the result, frame rate & exposure
  1042			 * are changed.
  1043			 * gain is not affected.
  1044			 */
  1045			ret = imx274_set_frame_interval(imx274,
  1046							imx274->frame_interval);
  1047			if (ret)
  1048				goto fail;
  1049	
  1050			/* update exposure time */
  1051			ret = __v4l2_ctrl_s_ctrl(imx274->ctrls.exposure,
  1052						 imx274->ctrls.exposure->val);
  1053			if (ret)
  1054				goto fail;
  1055	
  1056			/* start stream */
  1057			ret = imx274_start_stream(imx274);
  1058			if (ret)
  1059				goto fail;
  1060		} else {
  1061			/* stop stream */
  1062			ret = imx274_write_table(imx274, imx274_stop);
  1063			if (ret)
  1064				goto fail;
  1065		}
  1066	
  1067		mutex_unlock(&imx274->lock);
  1068		dev_dbg(&imx274->client->dev, "%s : Done\n", __func__);
  1069		return 0;
  1070	
  1071	fail:
  1072		mutex_unlock(&imx274->lock);
  1073		dev_err(&imx274->client->dev, "s_stream failed\n");
  1074		return ret;
  1075	}
  1076	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31818 bytes --]

Re: [PATCH 09/13] imx274: get rid of mode_index

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4550 bytes --]

Hi Luca,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.17-rc1 next-20180418]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Luca-Ceresoli/imx274-add-cropping-and-misc-improvements/20180413-234459
base:   git://linuxtv.org/media_tree.git master
config: i386-randconfig-h1-04190052 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from include/linux/dynamic_debug.h:6:0,
                    from include/linux/printk.h:329,
                    from include/linux/kernel.h:14,
                    from include/linux/clk.h:16,
                    from drivers/media//i2c/imx274.c:22:
   drivers/media//i2c/imx274.c: In function 'imx274_s_stream':
>> include/linux/jump_label.h:407:59: warning: format '%lu' expects argument of type 'long unsigned int', but argument 6 has type 'int' [-Wformat=]
     else if (__builtin_types_compatible_p(typeof(*x), struct static_key_false)) \
                                                              ^
   include/linux/dynamic_debug.h:103:2: note: in expansion of macro 'static_branch_unlikely'
     static_branch_unlikely(&descriptor.key.dd_key_false)
     ^
   include/linux/dynamic_debug.h:134:6: note: in expansion of macro 'DYNAMIC_DEBUG_BRANCH'
     if (DYNAMIC_DEBUG_BRANCH(descriptor))   \
         ^
   include/linux/device.h:1364:2: note: in expansion of macro 'dynamic_dev_dbg'
     dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
     ^
   drivers/media//i2c/imx274.c:1027:2: note: in expansion of macro 'dev_dbg'
     dev_dbg(&imx274->client->dev, "%s : %s, mode index %lu\n", __func__,
     ^
--
   In file included from include/linux/dynamic_debug.h:6:0,
                    from include/linux/printk.h:329,
                    from include/linux/kernel.h:14,
                    from include/linux/clk.h:16,
                    from drivers/media/i2c/imx274.c:22:
   drivers/media/i2c/imx274.c: In function 'imx274_s_stream':
>> include/linux/jump_label.h:407:59: warning: format '%lu' expects argument of type 'long unsigned int', but argument 6 has type 'int' [-Wformat=]
     else if (__builtin_types_compatible_p(typeof(*x), struct static_key_false)) \
                                                              ^
   include/linux/dynamic_debug.h:103:2: note: in expansion of macro 'static_branch_unlikely'
     static_branch_unlikely(&descriptor.key.dd_key_false)
     ^
   include/linux/dynamic_debug.h:134:6: note: in expansion of macro 'DYNAMIC_DEBUG_BRANCH'
     if (DYNAMIC_DEBUG_BRANCH(descriptor))   \
         ^
   include/linux/device.h:1364:2: note: in expansion of macro 'dynamic_dev_dbg'
     dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
     ^
   drivers/media/i2c/imx274.c:1027:2: note: in expansion of macro 'dev_dbg'
     dev_dbg(&imx274->client->dev, "%s : %s, mode index %lu\n", __func__,
     ^

vim +407 include/linux/jump_label.h

11276d53 Peter Zijlstra 2015-07-24  401  
11276d53 Peter Zijlstra 2015-07-24  402  #define static_branch_unlikely(x)						\
11276d53 Peter Zijlstra 2015-07-24  403  ({										\
11276d53 Peter Zijlstra 2015-07-24  404  	bool branch;								\
11276d53 Peter Zijlstra 2015-07-24  405  	if (__builtin_types_compatible_p(typeof(*x), struct static_key_true))	\
11276d53 Peter Zijlstra 2015-07-24  406  		branch = arch_static_branch_jump(&(x)->key, false);		\
11276d53 Peter Zijlstra 2015-07-24 @407  	else if (__builtin_types_compatible_p(typeof(*x), struct static_key_false)) \
11276d53 Peter Zijlstra 2015-07-24  408  		branch = arch_static_branch(&(x)->key, false);			\
11276d53 Peter Zijlstra 2015-07-24  409  	else									\
11276d53 Peter Zijlstra 2015-07-24  410  		branch = ____wrong_branch_error();				\
81dcf89f Peter Zijlstra 2018-01-18  411  	unlikely(branch);							\
11276d53 Peter Zijlstra 2015-07-24  412  })
11276d53 Peter Zijlstra 2015-07-24  413  

:::::: The code at line 407 was first introduced by commit
:::::: 11276d5306b8e5b438a36bbff855fe792d7eaa61 locking/static_keys: Add a new static_key interface

:::::: TO: Peter Zijlstra <peterz@infradead.org>
:::::: CC: Ingo Molnar <mingo@kernel.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31846 bytes --]

[PATCH 10/13] 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>
---
 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 25907d0817a4..39d548ce30c4 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 11/13] 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>
---
 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 39d548ce30c4..ebe37a087a02 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 12/13] 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>
---
 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 ebe37a087a02..0b3183afdd7b 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 13/13] 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.

Note that the names in enum imx274_mode change from being
resolution-based to being binning-based. 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 anny
misunderstanding.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 266 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 192 insertions(+), 74 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 0b3183afdd7b..db843e0ce057 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,7 +271,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
 	{0x3004, 0x02},
 	{0x3005, 0x21},
 	{0x3006, 0x00},
-	{0x3007, 0x11},
+	{0x3007, 0xb1},
 
 	{0x3018, 0xA2}, /* output XVS, HVS */
 
@@ -283,22 +281,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
 	{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,7 +314,7 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
 	{0x3004, 0x03},
 	{0x3005, 0x31},
 	{0x3006, 0x00},
-	{0x3007, 0x09},
+	{0x3007, 0xa9},
 
 	{0x3018, 0xA2}, /* output XVS, HVS */
 
@@ -341,21 +324,7 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
 	{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 +530,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 +871,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 +904,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 +1190,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 +1703,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 +1757,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