[PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls

[PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls

[Dave Stevenson]

Hi All

This is a small patch set that fixes a number of issues, adds in support
for the alternate input clock frequency option, and expands the control support
for flips and VBLANK/HBLANK.

My source tree has the 2 patches I've just sent for mono support first, but I
believe the two series should apply in either order (true for v1, possibly not
for v2).
There's tree with both patchsets applied at [1]. 

  Dave

Changes since v1:
2/11: Hardcoded colourspace values.
4/11: Missing blank line added.
6/11: s/repitition/repetition/
8/11: Set exposure range on changing mode.
8/11: Alter the initial exposure to be max.
8/11: Split out allowing VMAX to drop to 750 in 720p mode into a new patch.
10/11: Switch to an enum.
10/11: Compute IMX290_EXTCK_FREQ.
10/11: Move EXCK_FREQ and IMX290_INCKSEL7 settings to 
New patch: Add error code to logging in imx290_start_streaming.
Collected R-B tags.

[1] https://github.com/6by9/linux/tree/upstream_imx290

Dave Stevenson (13):
  media: i2c: imx290: Match kernel coding style on whitespace
  media: i2c: imx290: Set the colorspace fields in the format
  media: i2c: imx290: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks
  media: i2c: imx290: Fix the pixel rate at 148.5Mpix/s
  media: i2c: imx290: Support 60fps in 2 lane operation
  media: i2c: imx290: Use CSI timings as per datasheet
  media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write
  media: i2c: imx290: Convert V4L2_CID_VBLANK to read/write
  media: i2c: imx290: VMAX is mode dependent
  media: i2c: imx290: Remove duplicated write to IMX290_CTRL_07
  media: i2c: imx290: Add support for 74.25MHz external clock
  media: i2c: imx290: Add support for H & V Flips
  media: i2c: imx290: Add the error code to logs in start_streaming

 drivers/media/i2c/imx290.c | 464 +++++++++++++++++++++++++++++--------
 1 file changed, 365 insertions(+), 99 deletions(-)

-- 
2.34.1

[PATCH v2 01/13] media: i2c: imx290: Match kernel coding style on whitespace

[Dave Stevenson]

Fix up a couple of coding style issues regarding missing blank
lines after declarations, double blank lines, and incorrect
indentation.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx290.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 9ce839541926..22d6194678bc 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -106,7 +106,6 @@
 
 #define IMX290_VMAX_DEFAULT				1125
 
-
 /*
  * The IMX290 pixel array is organized as follows:
  *
@@ -350,6 +349,7 @@ static const s64 imx290_link_freq_2lanes[] = {
 	[FREQ_INDEX_1080P] = 445500000,
 	[FREQ_INDEX_720P] = 297000000,
 };
+
 static const s64 imx290_link_freq_4lanes[] = {
 	[FREQ_INDEX_1080P] = 222750000,
 	[FREQ_INDEX_720P] = 148500000,
@@ -485,7 +485,7 @@ static int __always_unused imx290_read(struct imx290 *imx290, u32 addr, u32 *val
 			      data, (addr >> IMX290_REG_SIZE_SHIFT) & 3);
 	if (ret < 0) {
 		dev_err(imx290->dev, "%u-bit read from 0x%04x failed: %d\n",
-			 ((addr >> IMX290_REG_SIZE_SHIFT) & 3) * 8,
+			((addr >> IMX290_REG_SIZE_SHIFT) & 3) * 8,
 			 addr & IMX290_REG_ADDR_MASK, ret);
 		return ret;
 	}
@@ -506,7 +506,7 @@ static int imx290_write(struct imx290 *imx290, u32 addr, u32 value, int *err)
 			       data, (addr >> IMX290_REG_SIZE_SHIFT) & 3);
 	if (ret < 0) {
 		dev_err(imx290->dev, "%u-bit write to 0x%04x failed: %d\n",
-			 ((addr >> IMX290_REG_SIZE_SHIFT) & 3) * 8,
+			((addr >> IMX290_REG_SIZE_SHIFT) & 3) * 8,
 			 addr & IMX290_REG_ADDR_MASK, ret);
 		if (err)
 			*err = ret;
@@ -772,8 +772,7 @@ static int imx290_start_streaming(struct imx290 *imx290,
 
 	/* Set init register settings */
 	ret = imx290_set_register_array(imx290, imx290_global_init_settings,
-					ARRAY_SIZE(
-						imx290_global_init_settings));
+					ARRAY_SIZE(imx290_global_init_settings));
 	if (ret < 0) {
 		dev_err(imx290->dev, "Could not set init registers\n");
 		return ret;
-- 
2.34.1

[PATCH v2 02/13] media: i2c: imx290: Set the colorspace fields in the format

[Dave Stevenson]

The colorspace fields were left untouched in imx290_set_fmt
which lead to a v4l2-compliance failure.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx290.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 22d6194678bc..827c0804792f 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -917,6 +917,10 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 		fmt->format.code = imx290_formats[0].code[imx290->model->colour_variant];
 
 	fmt->format.field = V4L2_FIELD_NONE;
+	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
+	fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_601;
+	fmt->format.quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	fmt->format.xfer_func = V4L2_XFER_FUNC_NONE;
 
 	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
 
-- 
2.34.1

Re: [PATCH v2 02/13] media: i2c: imx290: Set the colorspace fields in the format

[Laurent Pinchart]

Hi Dave,

Thank you for the patch.

On Fri, Feb 03, 2023 at 07:18:00PM +0000, Dave Stevenson wrote:
> The colorspace fields were left untouched in imx290_set_fmt
> which lead to a v4l2-compliance failure.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/imx290.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 22d6194678bc..827c0804792f 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -917,6 +917,10 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
>  		fmt->format.code = imx290_formats[0].code[imx290->model->colour_variant];
>  
>  	fmt->format.field = V4L2_FIELD_NONE;
> +	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
> +	fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_601;
> +	fmt->format.quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	fmt->format.xfer_func = V4L2_XFER_FUNC_NONE;
>  
>  	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
>  

-- 
Regards,

Laurent Pinchart

[PATCH v2 03/13] media: i2c: imx290: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks

[Dave Stevenson]

Any V4L2 subdevice that implements controls and declares
V4L2_SUBDEV_FL_HAS_DEVNODE should also declare V4L2_SUBDEV_FL_HAS_EVENTS
and implement subscribe_event and unsubscribe_event hooks.

This driver didn't and would therefore fail v4l2-compliance
testing.

Add the relevant hooks.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 827c0804792f..22faa9b54810 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -20,6 +20,7 @@
 #include <media/media-entity.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
@@ -993,6 +994,11 @@ static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
 	return 0;
 }
 
+static const struct v4l2_subdev_core_ops imx290_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
 static const struct v4l2_subdev_video_ops imx290_video_ops = {
 	.s_stream = imx290_set_stream,
 };
@@ -1007,6 +1013,7 @@ static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
 };
 
 static const struct v4l2_subdev_ops imx290_subdev_ops = {
+	.core = &imx290_core_ops,
 	.video = &imx290_video_ops,
 	.pad = &imx290_pad_ops,
 };
@@ -1025,7 +1032,8 @@ static int imx290_subdev_init(struct imx290 *imx290)
 	imx290->current_mode = &imx290_modes_ptr(imx290)[0];
 
 	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
-	imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
+			    V4L2_SUBDEV_FL_HAS_EVENTS;
 	imx290->sd.dev = imx290->dev;
 	imx290->sd.entity.ops = &imx290_subdev_entity_ops;
 	imx290->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
-- 
2.34.1

[PATCH v2 04/13] media: i2c: imx290: Fix the pixel rate at 148.5Mpix/s

[Dave Stevenson]

The datasheet lists the link frequency changes between
1080p and 720p modes. This is correct that the link frequency
changes as measured on an oscilloscope.

Link frequency is not necessarily the same as pixel rate.

The datasheet gives standard configurations for 1080p and 720p
modes at a number of frame rates.
Looking at the 1080p mode it gives:
HMAX = 0x898 = 2200
VMAX = 0x465 = 1125
2200 * 1125 * 60fps = 148.5MPix/s

Looking at the 720p mode it gives:
HMAX = 0xce4 = 3300
VMAX = 0x2ee = 750
3300 * 750 * 60fps = 148.5Mpix/s

This driver currently scales the pixel rate proportionally to the
link frequency, however the above shows that this is not the
correct thing to do, and currently all frame rate and exposure
calculations give incorrect results.

Correctly report the pixel rate as being 148.5MPix/s under any
mode.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx290.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 22faa9b54810..e83c458c7cf8 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -107,6 +107,8 @@
 
 #define IMX290_VMAX_DEFAULT				1125
 
+#define IMX290_PIXEL_RATE				148500000
+
 /*
  * The IMX290 pixel array is organized as follows:
  *
@@ -205,7 +207,6 @@ struct imx290 {
 
 	struct v4l2_ctrl_handler ctrls;
 	struct v4l2_ctrl *link_freq;
-	struct v4l2_ctrl *pixel_rate;
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *vblank;
 };
@@ -669,15 +670,8 @@ static void imx290_ctrl_update(struct imx290 *imx290,
 {
 	unsigned int hblank = mode->hmax - mode->width;
 	unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
-	s64 link_freq = imx290_link_freqs_ptr(imx290)[mode->link_freq_index];
-	u64 pixel_rate;
-
-	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
-	pixel_rate = link_freq * 2 * imx290->nlanes;
-	do_div(pixel_rate, imx290_format_info(format->code)->bpp);
 
 	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
-	__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate, pixel_rate);
 
 	__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
 	__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
@@ -727,9 +721,9 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 	if (imx290->link_freq)
 		imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
-	imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
-					       V4L2_CID_PIXEL_RATE,
-					       1, INT_MAX, 1, 1);
+	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, V4L2_CID_PIXEL_RATE,
+			  IMX290_PIXEL_RATE, IMX290_PIXEL_RATE, 1,
+			  IMX290_PIXEL_RATE);
 
 	v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
 				     V4L2_CID_TEST_PATTERN,
-- 
2.34.1

[PATCH v2 05/13] media: i2c: imx290: Support 60fps in 2 lane operation

[Dave Stevenson]

Commit "97589ad61c73 media: i2c: imx290: Add support for 2 data lanes"
added support for running in two lane mode (instead of 4), but
without changing the link frequency that resulted in a max of 30fps.

Commit "98e0500eadb7 media: i2c: imx290: Add configurable link frequency
and pixel rate" then doubled the link frequency when in 2 lane mode,
but didn't undo the correction for running at only 30fps, just extending
horizontal blanking instead.

Remove the 30fps limit on 2 lane by correcting the register config
in accordance with the datasheet for 60fps operation over 2 lanes.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index e83c458c7cf8..a5ea59353a4d 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -382,7 +382,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
 	{
 		.width = 1920,
 		.height = 1080,
-		.hmax = 4400,
+		.hmax = 2200,
 		.link_freq_index = FREQ_INDEX_1080P,
 		.data = imx290_1080p_settings,
 		.data_size = ARRAY_SIZE(imx290_1080p_settings),
@@ -390,7 +390,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
 	{
 		.width = 1280,
 		.height = 720,
-		.hmax = 6600,
+		.hmax = 3300,
 		.link_freq_index = FREQ_INDEX_720P,
 		.data = imx290_720p_settings,
 		.data_size = ARRAY_SIZE(imx290_720p_settings),
@@ -539,21 +539,10 @@ static int imx290_set_register_array(struct imx290 *imx290,
 static int imx290_set_data_lanes(struct imx290 *imx290)
 {
 	int ret = 0;
-	u32 frsel;
-
-	switch (imx290->nlanes) {
-	case 2:
-	default:
-		frsel = 0x02;
-		break;
-	case 4:
-		frsel = 0x01;
-		break;
-	}
 
 	imx290_write(imx290, IMX290_PHY_LANE_NUM, imx290->nlanes - 1, &ret);
 	imx290_write(imx290, IMX290_CSI_LANE_MODE, imx290->nlanes - 1, &ret);
-	imx290_write(imx290, IMX290_FR_FDG_SEL, frsel, &ret);
+	imx290_write(imx290, IMX290_FR_FDG_SEL, 0x01, &ret);
 
 	return ret;
 }
-- 
2.34.1

[PATCH v2 06/13] media: i2c: imx290: Use CSI timings as per datasheet

[Dave Stevenson]

Commit "98e0500eadb7 media: i2c: imx290: Add configurable link frequency
and pixel rate" added support for the increased link frequencies
on 2 data lanes, but didn't update the CSI timing registers in
accordance with the datasheet.

Use the specified settings.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx290.c | 126 +++++++++++++++++++++++++++++++------
 1 file changed, 106 insertions(+), 20 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index a5ea59353a4d..2abe4cdab819 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -190,6 +190,18 @@ struct imx290_mode {
 	u32 data_size;
 };
 
+struct imx290_csi_cfg {
+	u16 repetition;
+	u16 tclkpost;
+	u16 thszero;
+	u16 thsprepare;
+	u16 tclktrail;
+	u16 thstrail;
+	u16 tclkzero;
+	u16 tclkprepare;
+	u16 tlpx;
+};
+
 struct imx290 {
 	struct device *dev;
 	struct clk *xclk;
@@ -289,16 +301,6 @@ static const struct imx290_regval imx290_1080p_settings[] = {
 	{ IMX290_INCKSEL4, 0x01 },
 	{ IMX290_INCKSEL5, 0x1a },
 	{ IMX290_INCKSEL6, 0x1a },
-	/* data rate settings */
-	{ IMX290_REPETITION, 0x10 },
-	{ IMX290_TCLKPOST, 87 },
-	{ IMX290_THSZERO, 55 },
-	{ IMX290_THSPREPARE, 31 },
-	{ IMX290_TCLKTRAIL, 31 },
-	{ IMX290_THSTRAIL, 31 },
-	{ IMX290_TCLKZERO, 119 },
-	{ IMX290_TCLKPREPARE, 31 },
-	{ IMX290_TLPX, 23 },
 };
 
 static const struct imx290_regval imx290_720p_settings[] = {
@@ -314,16 +316,6 @@ static const struct imx290_regval imx290_720p_settings[] = {
 	{ IMX290_INCKSEL4, 0x01 },
 	{ IMX290_INCKSEL5, 0x1a },
 	{ IMX290_INCKSEL6, 0x1a },
-	/* data rate settings */
-	{ IMX290_REPETITION, 0x10 },
-	{ IMX290_TCLKPOST, 79 },
-	{ IMX290_THSZERO, 47 },
-	{ IMX290_THSPREPARE, 23 },
-	{ IMX290_TCLKTRAIL, 23 },
-	{ IMX290_THSTRAIL, 23 },
-	{ IMX290_TCLKZERO, 87 },
-	{ IMX290_TCLKPREPARE, 23 },
-	{ IMX290_TLPX, 23 },
 };
 
 static const struct imx290_regval imx290_10bit_settings[] = {
@@ -344,6 +336,58 @@ static const struct imx290_regval imx290_12bit_settings[] = {
 	{ IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW12 },
 };
 
+static const struct imx290_csi_cfg imx290_csi_222_75mhz = {
+	/* 222.25MHz or 445.5Mbit/s per lane */
+	.repetition = 0x10,
+	.tclkpost = 87,
+	.thszero = 55,
+	.thsprepare = 31,
+	.tclktrail = 31,
+	.thstrail = 31,
+	.tclkzero = 119,
+	.tclkprepare = 31,
+	.tlpx = 23,
+};
+
+static const struct imx290_csi_cfg imx290_csi_445_5mhz = {
+	/* 445.5MHz or 891Mbit/s per lane */
+	.repetition = 0x00,
+	.tclkpost = 119,
+	.thszero = 103,
+	.thsprepare = 71,
+	.tclktrail = 55,
+	.thstrail = 63,
+	.tclkzero = 255,
+	.tclkprepare = 63,
+	.tlpx = 55,
+};
+
+static const struct imx290_csi_cfg imx290_csi_148_5mhz = {
+	/* 148.5MHz or 297Mbit/s per lane */
+	.repetition = 0x10,
+	.tclkpost = 79,
+	.thszero = 47,
+	.thsprepare = 23,
+	.tclktrail = 23,
+	.thstrail = 23,
+	.tclkzero = 87,
+	.tclkprepare = 23,
+	.tlpx = 23,
+};
+
+static const struct imx290_csi_cfg imx290_csi_297mhz = {
+	/* 297MHz or 594Mbit/s per lane */
+	.repetition = 0x00,
+	.tclkpost = 103,
+	.thszero = 87,
+	.thsprepare = 47,
+	.tclktrail = 39,
+	.thstrail = 47,
+	.tclkzero = 191,
+	.tclkprepare = 47,
+	.tlpx = 39,
+};
+
 /* supported link frequencies */
 #define FREQ_INDEX_1080P	0
 #define FREQ_INDEX_720P		1
@@ -557,6 +601,42 @@ static int imx290_set_black_level(struct imx290 *imx290,
 			    black_level >> (16 - bpp), err);
 }
 
+static int imx290_set_csi_config(struct imx290 *imx290)
+{
+	const s64 *link_freqs = imx290_link_freqs_ptr(imx290);
+	const struct imx290_csi_cfg *csi_cfg;
+	int ret = 0;
+
+	switch (link_freqs[imx290->current_mode->link_freq_index]) {
+	case 445500000:
+		csi_cfg = &imx290_csi_445_5mhz;
+		break;
+	case 297000000:
+		csi_cfg = &imx290_csi_297mhz;
+		break;
+	case 222750000:
+		csi_cfg = &imx290_csi_222_75mhz;
+		break;
+	case 148500000:
+		csi_cfg = &imx290_csi_148_5mhz;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	imx290_write(imx290, IMX290_REPETITION, csi_cfg->repetition, &ret);
+	imx290_write(imx290, IMX290_TCLKPOST, csi_cfg->tclkpost, &ret);
+	imx290_write(imx290, IMX290_THSZERO, csi_cfg->thszero, &ret);
+	imx290_write(imx290, IMX290_THSPREPARE, csi_cfg->thsprepare, &ret);
+	imx290_write(imx290, IMX290_TCLKTRAIL, csi_cfg->tclktrail, &ret);
+	imx290_write(imx290, IMX290_THSTRAIL, csi_cfg->thstrail, &ret);
+	imx290_write(imx290, IMX290_TCLKZERO, csi_cfg->tclkzero, &ret);
+	imx290_write(imx290, IMX290_TCLKPREPARE, csi_cfg->tclkprepare, &ret);
+	imx290_write(imx290, IMX290_TLPX, csi_cfg->tlpx, &ret);
+
+	return ret;
+}
+
 static int imx290_setup_format(struct imx290 *imx290,
 			       const struct v4l2_mbus_framefmt *format)
 {
@@ -769,6 +849,12 @@ static int imx290_start_streaming(struct imx290 *imx290,
 		return ret;
 	}
 
+	ret = imx290_set_csi_config(imx290);
+	if (ret < 0) {
+		dev_err(imx290->dev, "Could not set csi cfg\n");
+		return ret;
+	}
+
 	/* Apply the register values related to current frame format */
 	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
 	ret = imx290_setup_format(imx290, format);
-- 
2.34.1

Re: [PATCH v2 06/13] media: i2c: imx290: Use CSI timings as per datasheet

[Alexander Stein]

Hi Dave,

thanks for the updated patch.

Am Freitag, 3. Februar 2023, 20:18:04 CET schrieb Dave Stevenson:
> Commit "98e0500eadb7 media: i2c: imx290: Add configurable link frequency
> and pixel rate" added support for the increased link frequencies
> on 2 data lanes, but didn't update the CSI timing registers in
> accordance with the datasheet.
> 
> Use the specified settings.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/i2c/imx290.c | 126 +++++++++++++++++++++++++++++++------
>  1 file changed, 106 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index a5ea59353a4d..2abe4cdab819 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -190,6 +190,18 @@ struct imx290_mode {
>  	u32 data_size;
>  };
> 
> +struct imx290_csi_cfg {
> +	u16 repetition;
> +	u16 tclkpost;
> +	u16 thszero;
> +	u16 thsprepare;
> +	u16 tclktrail;
> +	u16 thstrail;
> +	u16 tclkzero;
> +	u16 tclkprepare;
> +	u16 tlpx;
> +};
> +
>  struct imx290 {
>  	struct device *dev;
>  	struct clk *xclk;
> @@ -289,16 +301,6 @@ static const struct imx290_regval
> imx290_1080p_settings[] = { { IMX290_INCKSEL4, 0x01 },
>  	{ IMX290_INCKSEL5, 0x1a },
>  	{ IMX290_INCKSEL6, 0x1a },
> -	/* data rate settings */
> -	{ IMX290_REPETITION, 0x10 },
> -	{ IMX290_TCLKPOST, 87 },
> -	{ IMX290_THSZERO, 55 },
> -	{ IMX290_THSPREPARE, 31 },
> -	{ IMX290_TCLKTRAIL, 31 },
> -	{ IMX290_THSTRAIL, 31 },
> -	{ IMX290_TCLKZERO, 119 },
> -	{ IMX290_TCLKPREPARE, 31 },
> -	{ IMX290_TLPX, 23 },
>  };
> 
>  static const struct imx290_regval imx290_720p_settings[] = {
> @@ -314,16 +316,6 @@ static const struct imx290_regval
> imx290_720p_settings[] = { { IMX290_INCKSEL4, 0x01 },
>  	{ IMX290_INCKSEL5, 0x1a },
>  	{ IMX290_INCKSEL6, 0x1a },
> -	/* data rate settings */
> -	{ IMX290_REPETITION, 0x10 },
> -	{ IMX290_TCLKPOST, 79 },
> -	{ IMX290_THSZERO, 47 },
> -	{ IMX290_THSPREPARE, 23 },
> -	{ IMX290_TCLKTRAIL, 23 },
> -	{ IMX290_THSTRAIL, 23 },
> -	{ IMX290_TCLKZERO, 87 },
> -	{ IMX290_TCLKPREPARE, 23 },
> -	{ IMX290_TLPX, 23 },
>  };
> 
>  static const struct imx290_regval imx290_10bit_settings[] = {
> @@ -344,6 +336,58 @@ static const struct imx290_regval
> imx290_12bit_settings[] = { { IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW12 },
>  };
> 
> +static const struct imx290_csi_cfg imx290_csi_222_75mhz = {
> +	/* 222.25MHz or 445.5Mbit/s per lane */

s/222.25/222.75/ ;-)

Despite that:
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Best regards,
Alexander

> +	.repetition = 0x10,
> +	.tclkpost = 87,
> +	.thszero = 55,
> +	.thsprepare = 31,
> +	.tclktrail = 31,
> +	.thstrail = 31,
> +	.tclkzero = 119,
> +	.tclkprepare = 31,
> +	.tlpx = 23,
> +};
> +
> +static const struct imx290_csi_cfg imx290_csi_445_5mhz = {
> +	/* 445.5MHz or 891Mbit/s per lane */
> +	.repetition = 0x00,
> +	.tclkpost = 119,
> +	.thszero = 103,
> +	.thsprepare = 71,
> +	.tclktrail = 55,
> +	.thstrail = 63,
> +	.tclkzero = 255,
> +	.tclkprepare = 63,
> +	.tlpx = 55,
> +};
> +
> +static const struct imx290_csi_cfg imx290_csi_148_5mhz = {
> +	/* 148.5MHz or 297Mbit/s per lane */
> +	.repetition = 0x10,
> +	.tclkpost = 79,
> +	.thszero = 47,
> +	.thsprepare = 23,
> +	.tclktrail = 23,
> +	.thstrail = 23,
> +	.tclkzero = 87,
> +	.tclkprepare = 23,
> +	.tlpx = 23,
> +};
> +
> +static const struct imx290_csi_cfg imx290_csi_297mhz = {
> +	/* 297MHz or 594Mbit/s per lane */
> +	.repetition = 0x00,
> +	.tclkpost = 103,
> +	.thszero = 87,
> +	.thsprepare = 47,
> +	.tclktrail = 39,
> +	.thstrail = 47,
> +	.tclkzero = 191,
> +	.tclkprepare = 47,
> +	.tlpx = 39,
> +};
> +
>  /* supported link frequencies */
>  #define FREQ_INDEX_1080P	0
>  #define FREQ_INDEX_720P		1
> @@ -557,6 +601,42 @@ static int imx290_set_black_level(struct imx290
> *imx290, black_level >> (16 - bpp), err);
>  }
> 
> +static int imx290_set_csi_config(struct imx290 *imx290)
> +{
> +	const s64 *link_freqs = imx290_link_freqs_ptr(imx290);
> +	const struct imx290_csi_cfg *csi_cfg;
> +	int ret = 0;
> +
> +	switch (link_freqs[imx290->current_mode->link_freq_index]) {
> +	case 445500000:
> +		csi_cfg = &imx290_csi_445_5mhz;
> +		break;
> +	case 297000000:
> +		csi_cfg = &imx290_csi_297mhz;
> +		break;
> +	case 222750000:
> +		csi_cfg = &imx290_csi_222_75mhz;
> +		break;
> +	case 148500000:
> +		csi_cfg = &imx290_csi_148_5mhz;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	imx290_write(imx290, IMX290_REPETITION, csi_cfg->repetition, &ret);
> +	imx290_write(imx290, IMX290_TCLKPOST, csi_cfg->tclkpost, &ret);
> +	imx290_write(imx290, IMX290_THSZERO, csi_cfg->thszero, &ret);
> +	imx290_write(imx290, IMX290_THSPREPARE, csi_cfg->thsprepare, &ret);
> +	imx290_write(imx290, IMX290_TCLKTRAIL, csi_cfg->tclktrail, &ret);
> +	imx290_write(imx290, IMX290_THSTRAIL, csi_cfg->thstrail, &ret);
> +	imx290_write(imx290, IMX290_TCLKZERO, csi_cfg->tclkzero, &ret);
> +	imx290_write(imx290, IMX290_TCLKPREPARE, csi_cfg->tclkprepare, 
&ret);
> +	imx290_write(imx290, IMX290_TLPX, csi_cfg->tlpx, &ret);
> +
> +	return ret;
> +}
> +
>  static int imx290_setup_format(struct imx290 *imx290,
>  			       const struct v4l2_mbus_framefmt *format)
>  {
> @@ -769,6 +849,12 @@ static int imx290_start_streaming(struct imx290
> *imx290, return ret;
>  	}
> 
> +	ret = imx290_set_csi_config(imx290);
> +	if (ret < 0) {
> +		dev_err(imx290->dev, "Could not set csi cfg\n");
> +		return ret;
> +	}
> +
>  	/* Apply the register values related to current frame format */
>  	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
>  	ret = imx290_setup_format(imx290, format);

[PATCH v2 07/13] media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write

[Dave Stevenson]

The driver exposed V4L2_CID_HBLANK as a read only control to allow
for exposure calculations and determination of the frame rate.

Convert to a read/write control so that the frame rate can be
controlled.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx290.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 2abe4cdab819..74eafed15613 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -47,6 +47,7 @@
 #define IMX290_GAIN					IMX290_REG_8BIT(0x3014)
 #define IMX290_VMAX					IMX290_REG_24BIT(0x3018)
 #define IMX290_HMAX					IMX290_REG_16BIT(0x301c)
+#define IMX290_HMAX_MAX					0xffff
 #define IMX290_SHS1					IMX290_REG_24BIT(0x3020)
 #define IMX290_WINWV_OB					IMX290_REG_8BIT(0x303a)
 #define IMX290_WINPV					IMX290_REG_16BIT(0x303c)
@@ -183,7 +184,7 @@ struct imx290_regval {
 struct imx290_mode {
 	u32 width;
 	u32 height;
-	u32 hmax;
+	u32 hmax_min;
 	u8 link_freq_index;
 
 	const struct imx290_regval *data;
@@ -426,7 +427,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
 	{
 		.width = 1920,
 		.height = 1080,
-		.hmax = 2200,
+		.hmax_min = 2200,
 		.link_freq_index = FREQ_INDEX_1080P,
 		.data = imx290_1080p_settings,
 		.data_size = ARRAY_SIZE(imx290_1080p_settings),
@@ -434,7 +435,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
 	{
 		.width = 1280,
 		.height = 720,
-		.hmax = 3300,
+		.hmax_min = 3300,
 		.link_freq_index = FREQ_INDEX_720P,
 		.data = imx290_720p_settings,
 		.data_size = ARRAY_SIZE(imx290_720p_settings),
@@ -445,7 +446,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
 	{
 		.width = 1920,
 		.height = 1080,
-		.hmax = 2200,
+		.hmax_min = 2200,
 		.link_freq_index = FREQ_INDEX_1080P,
 		.data = imx290_1080p_settings,
 		.data_size = ARRAY_SIZE(imx290_1080p_settings),
@@ -453,7 +454,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
 	{
 		.width = 1280,
 		.height = 720,
-		.hmax = 3300,
+		.hmax_min = 3300,
 		.link_freq_index = FREQ_INDEX_720P,
 		.data = imx290_720p_settings,
 		.data_size = ARRAY_SIZE(imx290_720p_settings),
@@ -707,6 +708,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 		}
 		break;
 
+	case V4L2_CID_HBLANK:
+		ret = imx290_write(imx290, IMX290_HMAX,
+				   ctrl->val + imx290->current_mode->width,
+				   NULL);
+		break;
+
 	default:
 		ret = -EINVAL;
 		break;
@@ -737,12 +744,14 @@ static void imx290_ctrl_update(struct imx290 *imx290,
 			       const struct v4l2_mbus_framefmt *format,
 			       const struct imx290_mode *mode)
 {
-	unsigned int hblank = mode->hmax - mode->width;
+	unsigned int hblank_min = mode->hmax_min - mode->width;
+	unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
 	unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
 
 	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
 
-	__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
+	__v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max, 1,
+				 hblank_min);
 	__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
 }
 
@@ -799,10 +808,11 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 				     ARRAY_SIZE(imx290_test_pattern_menu) - 1,
 				     0, 0, imx290_test_pattern_menu);
 
+	/*
+	 * Actual range will be set from imx290_ctrl_update later in the probe.
+	 */
 	imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
 					   V4L2_CID_HBLANK, 1, 1, 1, 1);
-	if (imx290->hblank)
-		imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
 	imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
 					   V4L2_CID_VBLANK, 1, 1, 1, 1);
@@ -871,11 +881,6 @@ static int imx290_start_streaming(struct imx290 *imx290,
 		return ret;
 	}
 
-	ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax,
-			   NULL);
-	if (ret)
-		return ret;
-
 	/* Apply customized values from user */
 	ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
 	if (ret) {
-- 
2.34.1

Re: [PATCH v2 07/13] media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write

[Alexander Stein]

Hi Dave,

Am Freitag, 3. Februar 2023, 20:18:05 CET schrieb Dave Stevenson:
> The driver exposed V4L2_CID_HBLANK as a read only control to allow
> for exposure calculations and determination of the frame rate.
> 
> Convert to a read/write control so that the frame rate can be
> controlled.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> ---
>  drivers/media/i2c/imx290.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 2abe4cdab819..74eafed15613 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -47,6 +47,7 @@
>  #define IMX290_GAIN					
IMX290_REG_8BIT(0x3014)
>  #define IMX290_VMAX					
IMX290_REG_24BIT(0x3018)
>  #define IMX290_HMAX					
IMX290_REG_16BIT(0x301c)
> +#define IMX290_HMAX_MAX					0xffff
>  #define IMX290_SHS1					
IMX290_REG_24BIT(0x3020)
>  #define IMX290_WINWV_OB					
IMX290_REG_8BIT(0x303a)
>  #define IMX290_WINPV					
IMX290_REG_16BIT(0x303c)
> @@ -183,7 +184,7 @@ struct imx290_regval {
>  struct imx290_mode {
>  	u32 width;
>  	u32 height;
> -	u32 hmax;
> +	u32 hmax_min;
>  	u8 link_freq_index;
> 
>  	const struct imx290_regval *data;
> @@ -426,7 +427,7 @@ static const struct imx290_mode imx290_modes_2lanes[] =
> { {
>  		.width = 1920,
>  		.height = 1080,
> -		.hmax = 2200,
> +		.hmax_min = 2200,
>  		.link_freq_index = FREQ_INDEX_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> @@ -434,7 +435,7 @@ static const struct imx290_mode imx290_modes_2lanes[] =
> { {
>  		.width = 1280,
>  		.height = 720,
> -		.hmax = 3300,
> +		.hmax_min = 3300,
>  		.link_freq_index = FREQ_INDEX_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> @@ -445,7 +446,7 @@ static const struct imx290_mode imx290_modes_4lanes[] =
> { {
>  		.width = 1920,
>  		.height = 1080,
> -		.hmax = 2200,
> +		.hmax_min = 2200,
>  		.link_freq_index = FREQ_INDEX_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> @@ -453,7 +454,7 @@ static const struct imx290_mode imx290_modes_4lanes[] =
> { {
>  		.width = 1280,
>  		.height = 720,
> -		.hmax = 3300,
> +		.hmax_min = 3300,
>  		.link_freq_index = FREQ_INDEX_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> @@ -707,6 +708,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
>  		}
>  		break;
> 
> +	case V4L2_CID_HBLANK:
> +		ret = imx290_write(imx290, IMX290_HMAX,
> +				   ctrl->val + imx290->current_mode-
>width,
> +				   NULL);
> +		break;
> +
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -737,12 +744,14 @@ static void imx290_ctrl_update(struct imx290 *imx290,
>  			       const struct v4l2_mbus_framefmt *format,
>  			       const struct imx290_mode *mode)
>  {
> -	unsigned int hblank = mode->hmax - mode->width;
> +	unsigned int hblank_min = mode->hmax_min - mode->width;
> +	unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
>  	unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
> 
>  	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> 
> -	__v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, 1, hblank);
> +	__v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max, 1,
> +				 hblank_min);
>  	__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
>  }
> 
> @@ -799,10 +808,11 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>  				     
ARRAY_SIZE(imx290_test_pattern_menu) - 1,
>  				     0, 0, imx290_test_pattern_menu);
> 
> +	/*
> +	 * Actual range will be set from imx290_ctrl_update later in the 
probe.
> +	 */
>  	imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
>  					   V4L2_CID_HBLANK, 1, 1, 1, 
1);
> -	if (imx290->hblank)
> -		imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> 
>  	imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
>  					   V4L2_CID_VBLANK, 1, 1, 1, 
1);
> @@ -871,11 +881,6 @@ static int imx290_start_streaming(struct imx290
> *imx290, return ret;
>  	}
> 
> -	ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax,
> -			   NULL);
> -	if (ret)
> -		return ret;
> -
>  	/* Apply customized values from user */
>  	ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
>  	if (ret) {

[PATCH v2 08/13] media: i2c: imx290: Convert V4L2_CID_VBLANK to read/write

[Dave Stevenson]

The driver exposed V4L2_CID_VBLANK as a read only control to allow
for exposure calculations and determination of the frame rate.

Convert to a read/write control so that the frame rate can be
controlled.
V4L2_CID_VBLANK also sets the limits for the exposure control,
therefore exposure ranges have to be updated when vblank changes
(either via s_ctrl, or via changing mode).

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx290.c | 58 +++++++++++++++++++++++++++++++-------
 1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 74eafed15613..403bd7de875e 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -46,6 +46,7 @@
 #define IMX290_BLKLEVEL					IMX290_REG_16BIT(0x300a)
 #define IMX290_GAIN					IMX290_REG_8BIT(0x3014)
 #define IMX290_VMAX					IMX290_REG_24BIT(0x3018)
+#define IMX290_VMAX_MAX					0x3ffff
 #define IMX290_HMAX					IMX290_REG_16BIT(0x301c)
 #define IMX290_HMAX_MAX					0xffff
 #define IMX290_SHS1					IMX290_REG_24BIT(0x3020)
@@ -106,6 +107,9 @@
 #define IMX290_PGCTRL_THRU				BIT(1)
 #define IMX290_PGCTRL_MODE(n)				((n) << 4)
 
+/* Number of lines by which exposure must be less than VMAX) */
+#define IMX290_EXPOSURE_OFFSET				2
+
 #define IMX290_VMAX_DEFAULT				1125
 
 #define IMX290_PIXEL_RATE				148500000
@@ -222,6 +226,7 @@ struct imx290 {
 	struct v4l2_ctrl *link_freq;
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *vblank;
+	struct v4l2_ctrl *exposure;
 };
 
 static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
@@ -235,7 +240,6 @@ static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
 
 static const struct imx290_regval imx290_global_init_settings[] = {
 	{ IMX290_CTRL_07, IMX290_WINMODE_1080P },
-	{ IMX290_VMAX, IMX290_VMAX_DEFAULT },
 	{ IMX290_EXTCK_FREQ, 0x2520 },
 	{ IMX290_WINWV_OB, 12 },
 	{ IMX290_WINPH, 0 },
@@ -659,6 +663,16 @@ static int imx290_setup_format(struct imx290 *imx290,
 /* ----------------------------------------------------------------------------
  * Controls
  */
+static void imx290_exposure_update(struct imx290 *imx290,
+				   const struct imx290_mode *mode)
+{
+	unsigned int exposure_max;
+
+	exposure_max = imx290->vblank->val + mode->height -
+		       IMX290_EXPOSURE_OFFSET;
+	__v4l2_ctrl_modify_range(imx290->exposure, 1, exposure_max, 1,
+				 exposure_max);
+}
 
 static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 {
@@ -666,7 +680,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 					     struct imx290, ctrls);
 	const struct v4l2_mbus_framefmt *format;
 	struct v4l2_subdev_state *state;
-	int ret = 0;
+	int ret = 0, vmax;
 
 	/*
 	 * Return immediately for controls that don't need to be applied to the
@@ -675,6 +689,11 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 	if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
 		return 0;
 
+	if (ctrl->id == V4L2_CID_VBLANK) {
+		/* Changing vblank changes the allowed range for exposure. */
+		imx290_exposure_update(imx290, imx290->current_mode);
+	}
+
 	/* V4L2 controls values will be applied only when power is already up */
 	if (!pm_runtime_get_if_in_use(imx290->dev))
 		return 0;
@@ -687,9 +706,23 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 		ret = imx290_write(imx290, IMX290_GAIN, ctrl->val, NULL);
 		break;
 
+	case V4L2_CID_VBLANK:
+		ret = imx290_write(imx290, IMX290_VMAX,
+				   ctrl->val + imx290->current_mode->height,
+				   NULL);
+		/*
+		 * Due to the way that exposure is programmed in this sensor in
+		 * relation to VMAX, we have to reprogramme it whenever VMAX is
+		 * changed.
+		 * Update ctrl so that the V4L2_CID_EXPOSURE case can refer to
+		 * it.
+		 */
+		ctrl = imx290->exposure;
+		fallthrough;
 	case V4L2_CID_EXPOSURE:
+		vmax = imx290->vblank->val + imx290->current_mode->height;
 		ret = imx290_write(imx290, IMX290_SHS1,
-				   IMX290_VMAX_DEFAULT - ctrl->val - 1, NULL);
+				   vmax - ctrl->val - 1, NULL);
 		break;
 
 	case V4L2_CID_TEST_PATTERN:
@@ -746,13 +779,15 @@ static void imx290_ctrl_update(struct imx290 *imx290,
 {
 	unsigned int hblank_min = mode->hmax_min - mode->width;
 	unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
-	unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height;
+	unsigned int vblank_min = IMX290_VMAX_DEFAULT - mode->height;
+	unsigned int vblank_max = IMX290_VMAX_MAX - mode->height;
 
 	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
 
 	__v4l2_ctrl_modify_range(imx290->hblank, hblank_min, hblank_max, 1,
 				 hblank_min);
-	__v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, 1, vblank);
+	__v4l2_ctrl_modify_range(imx290->vblank, vblank_min, vblank_max, 1,
+				 vblank_min);
 }
 
 static int imx290_ctrl_init(struct imx290 *imx290)
@@ -782,9 +817,13 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
 			  V4L2_CID_ANALOGUE_GAIN, 0, 100, 1, 0);
 
-	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
-			  V4L2_CID_EXPOSURE, 1, IMX290_VMAX_DEFAULT - 2, 1,
-			  IMX290_VMAX_DEFAULT - 2);
+	/*
+	 * Correct range will be determined through imx290_ctrl_update setting
+	 * V4L2_CID_VBLANK.
+	 */
+	imx290->exposure = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+					     V4L2_CID_EXPOSURE, 1, 65535, 1,
+					     65535);
 
 	/*
 	 * Set the link frequency, pixel rate, horizontal blanking and vertical
@@ -816,8 +855,6 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 
 	imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
 					   V4L2_CID_VBLANK, 1, 1, 1, 1);
-	if (imx290->vblank)
-		imx290->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
 	v4l2_ctrl_new_fwnode_properties(&imx290->ctrls, &imx290_ctrl_ops,
 					&props);
@@ -1003,6 +1040,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 		imx290->current_mode = mode;
 
 		imx290_ctrl_update(imx290, &fmt->format, mode);
+		imx290_exposure_update(imx290, mode);
 	}
 
 	*format = fmt->format;
-- 
2.34.1

[PATCH v2 09/13] media: i2c: imx290: VMAX is mode dependent

[Dave Stevenson]

The default VMAX for 60fps in 720p mode is 750 according to
the datasheet, however the driver always left it at 1125
thereby stopping 60fps being achieved.

Make VMAX (and therefore V4L2_CID_VBLANK) mode dependent so
that 720p60 can be achieved.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx290.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 403bd7de875e..6235021a8d24 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -110,8 +110,6 @@
 /* Number of lines by which exposure must be less than VMAX) */
 #define IMX290_EXPOSURE_OFFSET				2
 
-#define IMX290_VMAX_DEFAULT				1125
-
 #define IMX290_PIXEL_RATE				148500000
 
 /*
@@ -189,6 +187,7 @@ struct imx290_mode {
 	u32 width;
 	u32 height;
 	u32 hmax_min;
+	u32 vmax_min;
 	u8 link_freq_index;
 
 	const struct imx290_regval *data;
@@ -432,6 +431,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
 		.width = 1920,
 		.height = 1080,
 		.hmax_min = 2200,
+		.vmax_min = 1125,
 		.link_freq_index = FREQ_INDEX_1080P,
 		.data = imx290_1080p_settings,
 		.data_size = ARRAY_SIZE(imx290_1080p_settings),
@@ -440,6 +440,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
 		.width = 1280,
 		.height = 720,
 		.hmax_min = 3300,
+		.vmax_min = 750,
 		.link_freq_index = FREQ_INDEX_720P,
 		.data = imx290_720p_settings,
 		.data_size = ARRAY_SIZE(imx290_720p_settings),
@@ -451,6 +452,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
 		.width = 1920,
 		.height = 1080,
 		.hmax_min = 2200,
+		.vmax_min = 1125,
 		.link_freq_index = FREQ_INDEX_1080P,
 		.data = imx290_1080p_settings,
 		.data_size = ARRAY_SIZE(imx290_1080p_settings),
@@ -459,6 +461,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
 		.width = 1280,
 		.height = 720,
 		.hmax_min = 3300,
+		.vmax_min = 750,
 		.link_freq_index = FREQ_INDEX_720P,
 		.data = imx290_720p_settings,
 		.data_size = ARRAY_SIZE(imx290_720p_settings),
@@ -779,7 +782,7 @@ static void imx290_ctrl_update(struct imx290 *imx290,
 {
 	unsigned int hblank_min = mode->hmax_min - mode->width;
 	unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
-	unsigned int vblank_min = IMX290_VMAX_DEFAULT - mode->height;
+	unsigned int vblank_min = mode->vmax_min - mode->height;
 	unsigned int vblank_max = IMX290_VMAX_MAX - mode->height;
 
 	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
-- 
2.34.1

Re: [PATCH v2 09/13] media: i2c: imx290: VMAX is mode dependent

[Laurent Pinchart]

Hi Dave,

Thank you for the patch.

On Fri, Feb 03, 2023 at 07:18:07PM +0000, Dave Stevenson wrote:
> The default VMAX for 60fps in 720p mode is 750 according to
> the datasheet, however the driver always left it at 1125
> thereby stopping 60fps being achieved.
> 
> Make VMAX (and therefore V4L2_CID_VBLANK) mode dependent so
> that 720p60 can be achieved.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/imx290.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 403bd7de875e..6235021a8d24 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -110,8 +110,6 @@
>  /* Number of lines by which exposure must be less than VMAX) */
>  #define IMX290_EXPOSURE_OFFSET				2
>  
> -#define IMX290_VMAX_DEFAULT				1125
> -
>  #define IMX290_PIXEL_RATE				148500000
>  
>  /*
> @@ -189,6 +187,7 @@ struct imx290_mode {
>  	u32 width;
>  	u32 height;
>  	u32 hmax_min;
> +	u32 vmax_min;
>  	u8 link_freq_index;
>  
>  	const struct imx290_regval *data;
> @@ -432,6 +431,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
>  		.width = 1920,
>  		.height = 1080,
>  		.hmax_min = 2200,
> +		.vmax_min = 1125,
>  		.link_freq_index = FREQ_INDEX_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> @@ -440,6 +440,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
>  		.width = 1280,
>  		.height = 720,
>  		.hmax_min = 3300,
> +		.vmax_min = 750,
>  		.link_freq_index = FREQ_INDEX_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> @@ -451,6 +452,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
>  		.width = 1920,
>  		.height = 1080,
>  		.hmax_min = 2200,
> +		.vmax_min = 1125,
>  		.link_freq_index = FREQ_INDEX_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> @@ -459,6 +461,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
>  		.width = 1280,
>  		.height = 720,
>  		.hmax_min = 3300,
> +		.vmax_min = 750,
>  		.link_freq_index = FREQ_INDEX_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> @@ -779,7 +782,7 @@ static void imx290_ctrl_update(struct imx290 *imx290,
>  {
>  	unsigned int hblank_min = mode->hmax_min - mode->width;
>  	unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
> -	unsigned int vblank_min = IMX290_VMAX_DEFAULT - mode->height;
> +	unsigned int vblank_min = mode->vmax_min - mode->height;
>  	unsigned int vblank_max = IMX290_VMAX_MAX - mode->height;
>  
>  	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);

-- 
Regards,

Laurent Pinchart

[PATCH v2 10/13] media: i2c: imx290: Remove duplicated write to IMX290_CTRL_07

[Dave Stevenson]

IMX290_CTRL_07 was written from both imx290_global_init_settings
and imx290_1080p_settings and imx290_720p_settings.

Remove it from imx290_global_init_settings as the setting varies
based on the mode.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/imx290.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 6235021a8d24..a74930e86a6c 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -238,7 +238,6 @@ static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
  */
 
 static const struct imx290_regval imx290_global_init_settings[] = {
-	{ IMX290_CTRL_07, IMX290_WINMODE_1080P },
 	{ IMX290_EXTCK_FREQ, 0x2520 },
 	{ IMX290_WINWV_OB, 12 },
 	{ IMX290_WINPH, 0 },
-- 
2.34.1

[PATCH v2 11/13] media: i2c: imx290: Add support for 74.25MHz external clock

[Dave Stevenson]

The sensor supports either a 37.125 or 74.25MHz external, but
the driver only supported 37.125MHz.

Add the relevant register configuration for either clock
frequency option.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx290.c | 132 ++++++++++++++++++++++++++++++++-----
 1 file changed, 116 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index a74930e86a6c..045d27b4d31b 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -102,6 +102,7 @@
 #define IMX290_TCLKPREPARE				IMX290_REG_16BIT(0x3452)
 #define IMX290_TLPX					IMX290_REG_16BIT(0x3454)
 #define IMX290_X_OUT_SIZE				IMX290_REG_16BIT(0x3472)
+#define IMX290_INCKSEL7					IMX290_REG_8BIT(0x3480)
 
 #define IMX290_PGCTRL_REGEN				BIT(0)
 #define IMX290_PGCTRL_THRU				BIT(1)
@@ -178,11 +179,29 @@ struct imx290_model_info {
 	enum imx290_colour_variant colour_variant;
 };
 
+enum imx290_clk_freq {
+	IMX290_CLK_37_125,
+	IMX290_CLK_74_25,
+	IMX290_NUM_CLK
+};
+
 struct imx290_regval {
 	u32 reg;
 	u32 val;
 };
 
+/*
+ * Clock configuration for registers INCKSEL1 to INCKSEL6.
+ */
+struct imx290_clk_cfg {
+	u8 incksel1;
+	u8 incksel2;
+	u8 incksel3;
+	u8 incksel4;
+	u8 incksel5;
+	u8 incksel6;
+};
+
 struct imx290_mode {
 	u32 width;
 	u32 height;
@@ -192,6 +211,8 @@ struct imx290_mode {
 
 	const struct imx290_regval *data;
 	u32 data_size;
+
+	const struct imx290_clk_cfg *clk_cfg;
 };
 
 struct imx290_csi_cfg {
@@ -210,6 +231,7 @@ struct imx290 {
 	struct device *dev;
 	struct clk *xclk;
 	struct regmap *regmap;
+	enum imx290_clk_freq xclk_idx;
 	u8 nlanes;
 	const struct imx290_model_info *model;
 
@@ -238,7 +260,6 @@ static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
  */
 
 static const struct imx290_regval imx290_global_init_settings[] = {
-	{ IMX290_EXTCK_FREQ, 0x2520 },
 	{ IMX290_WINWV_OB, 12 },
 	{ IMX290_WINPH, 0 },
 	{ IMX290_WINPV, 0 },
@@ -288,7 +309,18 @@ static const struct imx290_regval imx290_global_init_settings[] = {
 	{ IMX290_REG_8BIT(0x33b0), 0x50 },
 	{ IMX290_REG_8BIT(0x33b2), 0x1a },
 	{ IMX290_REG_8BIT(0x33b3), 0x04 },
-	{ IMX290_REG_8BIT(0x3480), 0x49 },
+};
+
+#define IMX290_NUM_CLK_REGS	2
+static const struct imx290_regval xclk_regs[][IMX290_NUM_CLK_REGS] = {
+	[IMX290_CLK_37_125] = {
+		{ IMX290_EXTCK_FREQ, (37125 * 256) / 1000 },
+		{ IMX290_INCKSEL7, 0x49 },
+	},
+	[IMX290_CLK_74_25] = {
+		{ IMX290_EXTCK_FREQ, (74250 * 256) / 1000 },
+		{ IMX290_INCKSEL7, 0x92 },
+	},
 };
 
 static const struct imx290_regval imx290_1080p_settings[] = {
@@ -298,12 +330,6 @@ static const struct imx290_regval imx290_1080p_settings[] = {
 	{ IMX290_OPB_SIZE_V, 10 },
 	{ IMX290_X_OUT_SIZE, 1920 },
 	{ IMX290_Y_OUT_SIZE, 1080 },
-	{ IMX290_INCKSEL1, 0x18 },
-	{ IMX290_INCKSEL2, 0x03 },
-	{ IMX290_INCKSEL3, 0x20 },
-	{ IMX290_INCKSEL4, 0x01 },
-	{ IMX290_INCKSEL5, 0x1a },
-	{ IMX290_INCKSEL6, 0x1a },
 };
 
 static const struct imx290_regval imx290_720p_settings[] = {
@@ -313,12 +339,6 @@ static const struct imx290_regval imx290_720p_settings[] = {
 	{ IMX290_OPB_SIZE_V, 4 },
 	{ IMX290_X_OUT_SIZE, 1280 },
 	{ IMX290_Y_OUT_SIZE, 720 },
-	{ IMX290_INCKSEL1, 0x20 },
-	{ IMX290_INCKSEL2, 0x00 },
-	{ IMX290_INCKSEL3, 0x20 },
-	{ IMX290_INCKSEL4, 0x01 },
-	{ IMX290_INCKSEL5, 0x1a },
-	{ IMX290_INCKSEL6, 0x1a },
 };
 
 static const struct imx290_regval imx290_10bit_settings[] = {
@@ -424,6 +444,48 @@ static inline int imx290_link_freqs_num(const struct imx290 *imx290)
 		return ARRAY_SIZE(imx290_link_freq_4lanes);
 }
 
+static const struct imx290_clk_cfg imx290_1080p_clock_config[] = {
+	[IMX290_CLK_37_125] = {
+		/* 37.125MHz clock config */
+		.incksel1 = 0x18,
+		.incksel2 = 0x03,
+		.incksel3 = 0x20,
+		.incksel4 = 0x01,
+		.incksel5 = 0x1a,
+		.incksel6 = 0x1a,
+	},
+	[IMX290_CLK_74_25] = {
+		/* 74.25MHz clock config */
+		.incksel1 = 0x0c,
+		.incksel2 = 0x03,
+		.incksel3 = 0x10,
+		.incksel4 = 0x01,
+		.incksel5 = 0x1b,
+		.incksel6 = 0x1b,
+	},
+};
+
+static const struct imx290_clk_cfg imx290_720p_clock_config[] = {
+	[IMX290_CLK_37_125] = {
+		/* 37.125MHz clock config */
+		.incksel1 = 0x20,
+		.incksel2 = 0x00,
+		.incksel3 = 0x20,
+		.incksel4 = 0x01,
+		.incksel5 = 0x1a,
+		.incksel6 = 0x1a,
+	},
+	[IMX290_CLK_74_25] = {
+		/* 74.25MHz clock config */
+		.incksel1 = 0x10,
+		.incksel2 = 0x00,
+		.incksel3 = 0x10,
+		.incksel4 = 0x01,
+		.incksel5 = 0x1b,
+		.incksel6 = 0x1b,
+	},
+};
+
 /* Mode configs */
 static const struct imx290_mode imx290_modes_2lanes[] = {
 	{
@@ -434,6 +496,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
 		.link_freq_index = FREQ_INDEX_1080P,
 		.data = imx290_1080p_settings,
 		.data_size = ARRAY_SIZE(imx290_1080p_settings),
+		.clk_cfg = imx290_1080p_clock_config,
 	},
 	{
 		.width = 1280,
@@ -443,6 +506,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
 		.link_freq_index = FREQ_INDEX_720P,
 		.data = imx290_720p_settings,
 		.data_size = ARRAY_SIZE(imx290_720p_settings),
+		.clk_cfg = imx290_720p_clock_config,
 	},
 };
 
@@ -455,6 +519,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
 		.link_freq_index = FREQ_INDEX_1080P,
 		.data = imx290_1080p_settings,
 		.data_size = ARRAY_SIZE(imx290_1080p_settings),
+		.clk_cfg = imx290_1080p_clock_config,
 	},
 	{
 		.width = 1280,
@@ -464,6 +529,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
 		.link_freq_index = FREQ_INDEX_720P,
 		.data = imx290_720p_settings,
 		.data_size = ARRAY_SIZE(imx290_720p_settings),
+		.clk_cfg = imx290_720p_clock_config,
 	},
 };
 
@@ -587,6 +653,26 @@ static int imx290_set_register_array(struct imx290 *imx290,
 	return 0;
 }
 
+static int imx290_set_clock(struct imx290 *imx290)
+{
+	const struct imx290_mode *mode = imx290->current_mode;
+	enum imx290_clk_freq clk_idx = imx290->xclk_idx;
+	const struct imx290_clk_cfg *clk_cfg = &mode->clk_cfg[clk_idx];
+	int ret;
+
+	ret = imx290_set_register_array(imx290, xclk_regs[clk_idx],
+					IMX290_NUM_CLK_REGS);
+
+	imx290_write(imx290, IMX290_INCKSEL1, clk_cfg->incksel1, &ret);
+	imx290_write(imx290, IMX290_INCKSEL2, clk_cfg->incksel2, &ret);
+	imx290_write(imx290, IMX290_INCKSEL3, clk_cfg->incksel3, &ret);
+	imx290_write(imx290, IMX290_INCKSEL4, clk_cfg->incksel4, &ret);
+	imx290_write(imx290, IMX290_INCKSEL5, clk_cfg->incksel5, &ret);
+	imx290_write(imx290, IMX290_INCKSEL6, clk_cfg->incksel6, &ret);
+
+	return ret;
+}
+
 static int imx290_set_data_lanes(struct imx290 *imx290)
 {
 	int ret = 0;
@@ -891,6 +977,13 @@ static int imx290_start_streaming(struct imx290 *imx290,
 		return ret;
 	}
 
+	/* Set clock parameters based on mode and xclk */
+	ret = imx290_set_clock(imx290);
+	if (ret < 0) {
+		dev_err(imx290->dev, "Could not set clocks\n");
+		return ret;
+	}
+
 	/* Set data lane count */
 	ret = imx290_set_data_lanes(imx290);
 	if (ret < 0) {
@@ -1290,8 +1383,15 @@ static int imx290_init_clk(struct imx290 *imx290)
 		return ret;
 	}
 
-	/* external clock must be 37.125 MHz */
-	if (xclk_freq != 37125000) {
+	/* external clock must be 37.125 MHz or 74.25MHz */
+	switch (xclk_freq) {
+	case 37125000:
+		imx290->xclk_idx = IMX290_CLK_37_125;
+		break;
+	case 74250000:
+		imx290->xclk_idx = IMX290_CLK_74_25;
+		break;
+	default:
 		dev_err(imx290->dev, "External clock frequency %u is not supported\n",
 			xclk_freq);
 		return -EINVAL;
-- 
2.34.1

Re: [PATCH v2 11/13] media: i2c: imx290: Add support for 74.25MHz external clock

[Alexander Stein]

Hi Dave,

thanks for the update.

Am Freitag, 3. Februar 2023, 20:18:09 CET schrieb Dave Stevenson:
> The sensor supports either a 37.125 or 74.25MHz external, but
> the driver only supported 37.125MHz.
> 
> Add the relevant register configuration for either clock
> frequency option.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> ---
>  drivers/media/i2c/imx290.c | 132 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 116 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index a74930e86a6c..045d27b4d31b 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -102,6 +102,7 @@
>  #define IMX290_TCLKPREPARE				
IMX290_REG_16BIT(0x3452)
>  #define IMX290_TLPX					
IMX290_REG_16BIT(0x3454)
>  #define IMX290_X_OUT_SIZE				
IMX290_REG_16BIT(0x3472)
> +#define IMX290_INCKSEL7					
IMX290_REG_8BIT(0x3480)
> 
>  #define IMX290_PGCTRL_REGEN				BIT(0)
>  #define IMX290_PGCTRL_THRU				BIT(1)
> @@ -178,11 +179,29 @@ struct imx290_model_info {
>  	enum imx290_colour_variant colour_variant;
>  };
> 
> +enum imx290_clk_freq {
> +	IMX290_CLK_37_125,
> +	IMX290_CLK_74_25,
> +	IMX290_NUM_CLK
> +};
> +
>  struct imx290_regval {
>  	u32 reg;
>  	u32 val;
>  };
> 
> +/*
> + * Clock configuration for registers INCKSEL1 to INCKSEL6.
> + */
> +struct imx290_clk_cfg {
> +	u8 incksel1;
> +	u8 incksel2;
> +	u8 incksel3;
> +	u8 incksel4;
> +	u8 incksel5;
> +	u8 incksel6;
> +};
> +
>  struct imx290_mode {
>  	u32 width;
>  	u32 height;
> @@ -192,6 +211,8 @@ struct imx290_mode {
> 
>  	const struct imx290_regval *data;
>  	u32 data_size;
> +
> +	const struct imx290_clk_cfg *clk_cfg;
>  };
> 
>  struct imx290_csi_cfg {
> @@ -210,6 +231,7 @@ struct imx290 {
>  	struct device *dev;
>  	struct clk *xclk;
>  	struct regmap *regmap;
> +	enum imx290_clk_freq xclk_idx;
>  	u8 nlanes;
>  	const struct imx290_model_info *model;
> 
> @@ -238,7 +260,6 @@ static inline struct imx290 *to_imx290(struct
> v4l2_subdev *_sd) */
> 
>  static const struct imx290_regval imx290_global_init_settings[] = {
> -	{ IMX290_EXTCK_FREQ, 0x2520 },
>  	{ IMX290_WINWV_OB, 12 },
>  	{ IMX290_WINPH, 0 },
>  	{ IMX290_WINPV, 0 },
> @@ -288,7 +309,18 @@ static const struct imx290_regval
> imx290_global_init_settings[] = { { IMX290_REG_8BIT(0x33b0), 0x50 },
>  	{ IMX290_REG_8BIT(0x33b2), 0x1a },
>  	{ IMX290_REG_8BIT(0x33b3), 0x04 },
> -	{ IMX290_REG_8BIT(0x3480), 0x49 },
> +};
> +
> +#define IMX290_NUM_CLK_REGS	2
> +static const struct imx290_regval xclk_regs[][IMX290_NUM_CLK_REGS] = {
> +	[IMX290_CLK_37_125] = {
> +		{ IMX290_EXTCK_FREQ, (37125 * 256) / 1000 },
> +		{ IMX290_INCKSEL7, 0x49 },
> +	},
> +	[IMX290_CLK_74_25] = {
> +		{ IMX290_EXTCK_FREQ, (74250 * 256) / 1000 },
> +		{ IMX290_INCKSEL7, 0x92 },
> +	},
>  };
> 
>  static const struct imx290_regval imx290_1080p_settings[] = {
> @@ -298,12 +330,6 @@ static const struct imx290_regval
> imx290_1080p_settings[] = { { IMX290_OPB_SIZE_V, 10 },
>  	{ IMX290_X_OUT_SIZE, 1920 },
>  	{ IMX290_Y_OUT_SIZE, 1080 },
> -	{ IMX290_INCKSEL1, 0x18 },
> -	{ IMX290_INCKSEL2, 0x03 },
> -	{ IMX290_INCKSEL3, 0x20 },
> -	{ IMX290_INCKSEL4, 0x01 },
> -	{ IMX290_INCKSEL5, 0x1a },
> -	{ IMX290_INCKSEL6, 0x1a },
>  };
> 
>  static const struct imx290_regval imx290_720p_settings[] = {
> @@ -313,12 +339,6 @@ static const struct imx290_regval
> imx290_720p_settings[] = { { IMX290_OPB_SIZE_V, 4 },
>  	{ IMX290_X_OUT_SIZE, 1280 },
>  	{ IMX290_Y_OUT_SIZE, 720 },
> -	{ IMX290_INCKSEL1, 0x20 },
> -	{ IMX290_INCKSEL2, 0x00 },
> -	{ IMX290_INCKSEL3, 0x20 },
> -	{ IMX290_INCKSEL4, 0x01 },
> -	{ IMX290_INCKSEL5, 0x1a },
> -	{ IMX290_INCKSEL6, 0x1a },
>  };
> 
>  static const struct imx290_regval imx290_10bit_settings[] = {
> @@ -424,6 +444,48 @@ static inline int imx290_link_freqs_num(const struct
> imx290 *imx290) return ARRAY_SIZE(imx290_link_freq_4lanes);
>  }
> 
> +static const struct imx290_clk_cfg imx290_1080p_clock_config[] = {
> +	[IMX290_CLK_37_125] = {
> +		/* 37.125MHz clock config */
> +		.incksel1 = 0x18,
> +		.incksel2 = 0x03,
> +		.incksel3 = 0x20,
> +		.incksel4 = 0x01,
> +		.incksel5 = 0x1a,
> +		.incksel6 = 0x1a,
> +	},
> +	[IMX290_CLK_74_25] = {
> +		/* 74.25MHz clock config */
> +		.incksel1 = 0x0c,
> +		.incksel2 = 0x03,
> +		.incksel3 = 0x10,
> +		.incksel4 = 0x01,
> +		.incksel5 = 0x1b,
> +		.incksel6 = 0x1b,
> +	},
> +};
> +
> +static const struct imx290_clk_cfg imx290_720p_clock_config[] = {
> +	[IMX290_CLK_37_125] = {
> +		/* 37.125MHz clock config */
> +		.incksel1 = 0x20,
> +		.incksel2 = 0x00,
> +		.incksel3 = 0x20,
> +		.incksel4 = 0x01,
> +		.incksel5 = 0x1a,
> +		.incksel6 = 0x1a,
> +	},
> +	[IMX290_CLK_74_25] = {
> +		/* 74.25MHz clock config */
> +		.incksel1 = 0x10,
> +		.incksel2 = 0x00,
> +		.incksel3 = 0x10,
> +		.incksel4 = 0x01,
> +		.incksel5 = 0x1b,
> +		.incksel6 = 0x1b,
> +	},
> +};
> +
>  /* Mode configs */
>  static const struct imx290_mode imx290_modes_2lanes[] = {
>  	{
> @@ -434,6 +496,7 @@ static const struct imx290_mode imx290_modes_2lanes[] =
> { .link_freq_index = FREQ_INDEX_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> +		.clk_cfg = imx290_1080p_clock_config,
>  	},
>  	{
>  		.width = 1280,
> @@ -443,6 +506,7 @@ static const struct imx290_mode imx290_modes_2lanes[] =
> { .link_freq_index = FREQ_INDEX_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> +		.clk_cfg = imx290_720p_clock_config,
>  	},
>  };
> 
> @@ -455,6 +519,7 @@ static const struct imx290_mode imx290_modes_4lanes[] =
> { .link_freq_index = FREQ_INDEX_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> +		.clk_cfg = imx290_1080p_clock_config,
>  	},
>  	{
>  		.width = 1280,
> @@ -464,6 +529,7 @@ static const struct imx290_mode imx290_modes_4lanes[] =
> { .link_freq_index = FREQ_INDEX_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> +		.clk_cfg = imx290_720p_clock_config,
>  	},
>  };
> 
> @@ -587,6 +653,26 @@ static int imx290_set_register_array(struct imx290
> *imx290, return 0;
>  }
> 
> +static int imx290_set_clock(struct imx290 *imx290)
> +{
> +	const struct imx290_mode *mode = imx290->current_mode;
> +	enum imx290_clk_freq clk_idx = imx290->xclk_idx;
> +	const struct imx290_clk_cfg *clk_cfg = &mode->clk_cfg[clk_idx];
> +	int ret;
> +
> +	ret = imx290_set_register_array(imx290, xclk_regs[clk_idx],
> +					IMX290_NUM_CLK_REGS);
> +
> +	imx290_write(imx290, IMX290_INCKSEL1, clk_cfg->incksel1, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL2, clk_cfg->incksel2, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL3, clk_cfg->incksel3, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL4, clk_cfg->incksel4, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL5, clk_cfg->incksel5, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL6, clk_cfg->incksel6, &ret);
> +
> +	return ret;
> +}
> +
>  static int imx290_set_data_lanes(struct imx290 *imx290)
>  {
>  	int ret = 0;
> @@ -891,6 +977,13 @@ static int imx290_start_streaming(struct imx290
> *imx290, return ret;
>  	}
> 
> +	/* Set clock parameters based on mode and xclk */
> +	ret = imx290_set_clock(imx290);
> +	if (ret < 0) {
> +		dev_err(imx290->dev, "Could not set clocks\n");
> +		return ret;
> +	}
> +
>  	/* Set data lane count */
>  	ret = imx290_set_data_lanes(imx290);
>  	if (ret < 0) {
> @@ -1290,8 +1383,15 @@ static int imx290_init_clk(struct imx290 *imx290)
>  		return ret;
>  	}
> 
> -	/* external clock must be 37.125 MHz */
> -	if (xclk_freq != 37125000) {
> +	/* external clock must be 37.125 MHz or 74.25MHz */
> +	switch (xclk_freq) {
> +	case 37125000:
> +		imx290->xclk_idx = IMX290_CLK_37_125;
> +		break;
> +	case 74250000:
> +		imx290->xclk_idx = IMX290_CLK_74_25;
> +		break;
> +	default:
>  		dev_err(imx290->dev, "External clock frequency %u is not 
supported\n",
>  			xclk_freq);
>  		return -EINVAL;

Re: [PATCH v2 11/13] media: i2c: imx290: Add support for 74.25MHz external clock

[Laurent Pinchart]

Hi Dave,

Thank you for the patch.

On Fri, Feb 03, 2023 at 07:18:09PM +0000, Dave Stevenson wrote:
> The sensor supports either a 37.125 or 74.25MHz external, but
> the driver only supported 37.125MHz.
> 
> Add the relevant register configuration for either clock
> frequency option.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/imx290.c | 132 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 116 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index a74930e86a6c..045d27b4d31b 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -102,6 +102,7 @@
>  #define IMX290_TCLKPREPARE				IMX290_REG_16BIT(0x3452)
>  #define IMX290_TLPX					IMX290_REG_16BIT(0x3454)
>  #define IMX290_X_OUT_SIZE				IMX290_REG_16BIT(0x3472)
> +#define IMX290_INCKSEL7					IMX290_REG_8BIT(0x3480)
>  
>  #define IMX290_PGCTRL_REGEN				BIT(0)
>  #define IMX290_PGCTRL_THRU				BIT(1)
> @@ -178,11 +179,29 @@ struct imx290_model_info {
>  	enum imx290_colour_variant colour_variant;
>  };
>  
> +enum imx290_clk_freq {
> +	IMX290_CLK_37_125,
> +	IMX290_CLK_74_25,
> +	IMX290_NUM_CLK
> +};
> +
>  struct imx290_regval {
>  	u32 reg;
>  	u32 val;
>  };
>  
> +/*
> + * Clock configuration for registers INCKSEL1 to INCKSEL6.
> + */
> +struct imx290_clk_cfg {
> +	u8 incksel1;
> +	u8 incksel2;
> +	u8 incksel3;
> +	u8 incksel4;
> +	u8 incksel5;
> +	u8 incksel6;
> +};
> +
>  struct imx290_mode {
>  	u32 width;
>  	u32 height;
> @@ -192,6 +211,8 @@ struct imx290_mode {
>  
>  	const struct imx290_regval *data;
>  	u32 data_size;
> +
> +	const struct imx290_clk_cfg *clk_cfg;
>  };
>  
>  struct imx290_csi_cfg {
> @@ -210,6 +231,7 @@ struct imx290 {
>  	struct device *dev;
>  	struct clk *xclk;
>  	struct regmap *regmap;
> +	enum imx290_clk_freq xclk_idx;
>  	u8 nlanes;
>  	const struct imx290_model_info *model;
>  
> @@ -238,7 +260,6 @@ static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
>   */
>  
>  static const struct imx290_regval imx290_global_init_settings[] = {
> -	{ IMX290_EXTCK_FREQ, 0x2520 },
>  	{ IMX290_WINWV_OB, 12 },
>  	{ IMX290_WINPH, 0 },
>  	{ IMX290_WINPV, 0 },
> @@ -288,7 +309,18 @@ static const struct imx290_regval imx290_global_init_settings[] = {
>  	{ IMX290_REG_8BIT(0x33b0), 0x50 },
>  	{ IMX290_REG_8BIT(0x33b2), 0x1a },
>  	{ IMX290_REG_8BIT(0x33b3), 0x04 },
> -	{ IMX290_REG_8BIT(0x3480), 0x49 },
> +};
> +
> +#define IMX290_NUM_CLK_REGS	2
> +static const struct imx290_regval xclk_regs[][IMX290_NUM_CLK_REGS] = {
> +	[IMX290_CLK_37_125] = {
> +		{ IMX290_EXTCK_FREQ, (37125 * 256) / 1000 },
> +		{ IMX290_INCKSEL7, 0x49 },
> +	},
> +	[IMX290_CLK_74_25] = {
> +		{ IMX290_EXTCK_FREQ, (74250 * 256) / 1000 },
> +		{ IMX290_INCKSEL7, 0x92 },
> +	},
>  };
>  
>  static const struct imx290_regval imx290_1080p_settings[] = {
> @@ -298,12 +330,6 @@ static const struct imx290_regval imx290_1080p_settings[] = {
>  	{ IMX290_OPB_SIZE_V, 10 },
>  	{ IMX290_X_OUT_SIZE, 1920 },
>  	{ IMX290_Y_OUT_SIZE, 1080 },
> -	{ IMX290_INCKSEL1, 0x18 },
> -	{ IMX290_INCKSEL2, 0x03 },
> -	{ IMX290_INCKSEL3, 0x20 },
> -	{ IMX290_INCKSEL4, 0x01 },
> -	{ IMX290_INCKSEL5, 0x1a },
> -	{ IMX290_INCKSEL6, 0x1a },
>  };
>  
>  static const struct imx290_regval imx290_720p_settings[] = {
> @@ -313,12 +339,6 @@ static const struct imx290_regval imx290_720p_settings[] = {
>  	{ IMX290_OPB_SIZE_V, 4 },
>  	{ IMX290_X_OUT_SIZE, 1280 },
>  	{ IMX290_Y_OUT_SIZE, 720 },
> -	{ IMX290_INCKSEL1, 0x20 },
> -	{ IMX290_INCKSEL2, 0x00 },
> -	{ IMX290_INCKSEL3, 0x20 },
> -	{ IMX290_INCKSEL4, 0x01 },
> -	{ IMX290_INCKSEL5, 0x1a },
> -	{ IMX290_INCKSEL6, 0x1a },
>  };
>  
>  static const struct imx290_regval imx290_10bit_settings[] = {
> @@ -424,6 +444,48 @@ static inline int imx290_link_freqs_num(const struct imx290 *imx290)
>  		return ARRAY_SIZE(imx290_link_freq_4lanes);
>  }
>  
> +static const struct imx290_clk_cfg imx290_1080p_clock_config[] = {
> +	[IMX290_CLK_37_125] = {
> +		/* 37.125MHz clock config */
> +		.incksel1 = 0x18,
> +		.incksel2 = 0x03,
> +		.incksel3 = 0x20,
> +		.incksel4 = 0x01,
> +		.incksel5 = 0x1a,
> +		.incksel6 = 0x1a,
> +	},
> +	[IMX290_CLK_74_25] = {
> +		/* 74.25MHz clock config */
> +		.incksel1 = 0x0c,
> +		.incksel2 = 0x03,
> +		.incksel3 = 0x10,
> +		.incksel4 = 0x01,
> +		.incksel5 = 0x1b,
> +		.incksel6 = 0x1b,
> +	},
> +};
> +
> +static const struct imx290_clk_cfg imx290_720p_clock_config[] = {
> +	[IMX290_CLK_37_125] = {
> +		/* 37.125MHz clock config */
> +		.incksel1 = 0x20,
> +		.incksel2 = 0x00,
> +		.incksel3 = 0x20,
> +		.incksel4 = 0x01,
> +		.incksel5 = 0x1a,
> +		.incksel6 = 0x1a,
> +	},
> +	[IMX290_CLK_74_25] = {
> +		/* 74.25MHz clock config */
> +		.incksel1 = 0x10,
> +		.incksel2 = 0x00,
> +		.incksel3 = 0x10,
> +		.incksel4 = 0x01,
> +		.incksel5 = 0x1b,
> +		.incksel6 = 0x1b,
> +	},
> +};
> +
>  /* Mode configs */
>  static const struct imx290_mode imx290_modes_2lanes[] = {
>  	{
> @@ -434,6 +496,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
>  		.link_freq_index = FREQ_INDEX_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> +		.clk_cfg = imx290_1080p_clock_config,
>  	},
>  	{
>  		.width = 1280,
> @@ -443,6 +506,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
>  		.link_freq_index = FREQ_INDEX_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> +		.clk_cfg = imx290_720p_clock_config,
>  	},
>  };
>  
> @@ -455,6 +519,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
>  		.link_freq_index = FREQ_INDEX_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> +		.clk_cfg = imx290_1080p_clock_config,
>  	},
>  	{
>  		.width = 1280,
> @@ -464,6 +529,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
>  		.link_freq_index = FREQ_INDEX_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> +		.clk_cfg = imx290_720p_clock_config,
>  	},
>  };
>  
> @@ -587,6 +653,26 @@ static int imx290_set_register_array(struct imx290 *imx290,
>  	return 0;
>  }
>  
> +static int imx290_set_clock(struct imx290 *imx290)
> +{
> +	const struct imx290_mode *mode = imx290->current_mode;
> +	enum imx290_clk_freq clk_idx = imx290->xclk_idx;
> +	const struct imx290_clk_cfg *clk_cfg = &mode->clk_cfg[clk_idx];
> +	int ret;
> +
> +	ret = imx290_set_register_array(imx290, xclk_regs[clk_idx],
> +					IMX290_NUM_CLK_REGS);
> +
> +	imx290_write(imx290, IMX290_INCKSEL1, clk_cfg->incksel1, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL2, clk_cfg->incksel2, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL3, clk_cfg->incksel3, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL4, clk_cfg->incksel4, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL5, clk_cfg->incksel5, &ret);
> +	imx290_write(imx290, IMX290_INCKSEL6, clk_cfg->incksel6, &ret);
> +
> +	return ret;
> +}
> +
>  static int imx290_set_data_lanes(struct imx290 *imx290)
>  {
>  	int ret = 0;
> @@ -891,6 +977,13 @@ static int imx290_start_streaming(struct imx290 *imx290,
>  		return ret;
>  	}
>  
> +	/* Set clock parameters based on mode and xclk */
> +	ret = imx290_set_clock(imx290);
> +	if (ret < 0) {
> +		dev_err(imx290->dev, "Could not set clocks\n");
> +		return ret;
> +	}
> +
>  	/* Set data lane count */
>  	ret = imx290_set_data_lanes(imx290);
>  	if (ret < 0) {
> @@ -1290,8 +1383,15 @@ static int imx290_init_clk(struct imx290 *imx290)
>  		return ret;
>  	}
>  
> -	/* external clock must be 37.125 MHz */
> -	if (xclk_freq != 37125000) {
> +	/* external clock must be 37.125 MHz or 74.25MHz */
> +	switch (xclk_freq) {
> +	case 37125000:
> +		imx290->xclk_idx = IMX290_CLK_37_125;
> +		break;
> +	case 74250000:
> +		imx290->xclk_idx = IMX290_CLK_74_25;
> +		break;
> +	default:
>  		dev_err(imx290->dev, "External clock frequency %u is not supported\n",
>  			xclk_freq);
>  		return -EINVAL;

-- 
Regards,

Laurent Pinchart

[PATCH v2 12/13] media: i2c: imx290: Add support for H & V Flips

[Dave Stevenson]

The sensor supports H & V flips, so add the relevant hooks for
V4L2_CID_HFLIP and V4L2_CID_VFLIP to configure them.

Note that the Bayer order is maintained as the readout area
shifts by 1 pixel in the appropriate direction (note the
comment about the top margin being 8 pixels whilst the bottom
margin is 9). The V4L2_SEL_TGT_CROP region is therefore
adjusted appropriately.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx290.c | 51 ++++++++++++++++++++++++++++++++++----
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 045d27b4d31b..7167eb1edb9b 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -208,6 +208,7 @@ struct imx290_mode {
 	u32 hmax_min;
 	u32 vmax_min;
 	u8 link_freq_index;
+	u8 ctrl_07;
 
 	const struct imx290_regval *data;
 	u32 data_size;
@@ -248,6 +249,10 @@ struct imx290 {
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *vblank;
 	struct v4l2_ctrl *exposure;
+	struct {
+		struct v4l2_ctrl *hflip;
+		struct v4l2_ctrl *vflip;
+	};
 };
 
 static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
@@ -325,7 +330,6 @@ static const struct imx290_regval xclk_regs[][IMX290_NUM_CLK_REGS] = {
 
 static const struct imx290_regval imx290_1080p_settings[] = {
 	/* mode settings */
-	{ IMX290_CTRL_07, IMX290_WINMODE_1080P },
 	{ IMX290_WINWV_OB, 12 },
 	{ IMX290_OPB_SIZE_V, 10 },
 	{ IMX290_X_OUT_SIZE, 1920 },
@@ -334,7 +338,6 @@ static const struct imx290_regval imx290_1080p_settings[] = {
 
 static const struct imx290_regval imx290_720p_settings[] = {
 	/* mode settings */
-	{ IMX290_CTRL_07, IMX290_WINMODE_720P },
 	{ IMX290_WINWV_OB, 6 },
 	{ IMX290_OPB_SIZE_V, 4 },
 	{ IMX290_X_OUT_SIZE, 1280 },
@@ -494,6 +497,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
 		.hmax_min = 2200,
 		.vmax_min = 1125,
 		.link_freq_index = FREQ_INDEX_1080P,
+		.ctrl_07 = IMX290_WINMODE_1080P,
 		.data = imx290_1080p_settings,
 		.data_size = ARRAY_SIZE(imx290_1080p_settings),
 		.clk_cfg = imx290_1080p_clock_config,
@@ -504,6 +508,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
 		.hmax_min = 3300,
 		.vmax_min = 750,
 		.link_freq_index = FREQ_INDEX_720P,
+		.ctrl_07 = IMX290_WINMODE_720P,
 		.data = imx290_720p_settings,
 		.data_size = ARRAY_SIZE(imx290_720p_settings),
 		.clk_cfg = imx290_720p_clock_config,
@@ -517,6 +522,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
 		.hmax_min = 2200,
 		.vmax_min = 1125,
 		.link_freq_index = FREQ_INDEX_1080P,
+		.ctrl_07 = IMX290_WINMODE_1080P,
 		.data = imx290_1080p_settings,
 		.data_size = ARRAY_SIZE(imx290_1080p_settings),
 		.clk_cfg = imx290_1080p_clock_config,
@@ -527,6 +533,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
 		.hmax_min = 3300,
 		.vmax_min = 750,
 		.link_freq_index = FREQ_INDEX_720P,
+		.ctrl_07 = IMX290_WINMODE_720P,
 		.data = imx290_720p_settings,
 		.data_size = ARRAY_SIZE(imx290_720p_settings),
 		.clk_cfg = imx290_720p_clock_config,
@@ -835,6 +842,20 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 				   NULL);
 		break;
 
+	case V4L2_CID_HFLIP:
+	case V4L2_CID_VFLIP:
+	{
+		u32 reg;
+
+		reg = imx290->current_mode->ctrl_07;
+		if (imx290->hflip->val)
+			reg |= IMX290_HREVERSE;
+		if (imx290->vflip->val)
+			reg |= IMX290_VREVERSE;
+		ret = imx290_write(imx290, IMX290_CTRL_07, reg, NULL);
+		break;
+	}
+
 	default:
 		ret = -EINVAL;
 		break;
@@ -887,7 +908,7 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 	if (ret < 0)
 		return ret;
 
-	v4l2_ctrl_handler_init(&imx290->ctrls, 9);
+	v4l2_ctrl_handler_init(&imx290->ctrls, 11);
 
 	/*
 	 * The sensor has an analog gain and a digital gain, both controlled
@@ -944,6 +965,12 @@ static int imx290_ctrl_init(struct imx290 *imx290)
 	imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
 					   V4L2_CID_VBLANK, 1, 1, 1, 1);
 
+	imx290->hflip = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+					  V4L2_CID_HFLIP, 0, 1, 1, 0);
+	imx290->vflip = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
+					  V4L2_CID_VFLIP, 0, 1, 1, 0);
+	v4l2_ctrl_cluster(2, &imx290->hflip);
+
 	v4l2_ctrl_new_fwnode_properties(&imx290->ctrls, &imx290_ctrl_ops,
 					&props);
 
@@ -1065,6 +1092,13 @@ static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
 		pm_runtime_put_autosuspend(imx290->dev);
 	}
 
+	/*
+	 * vflip and hflip should not be changed during streaming as the sensor
+	 * will produce an invalid frame.
+	 */
+	__v4l2_ctrl_grab(imx290->vflip, enable);
+	__v4l2_ctrl_grab(imx290->hflip, enable);
+
 unlock:
 	v4l2_subdev_unlock_state(state);
 	return ret;
@@ -1147,16 +1181,23 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
 				struct v4l2_subdev_state *sd_state,
 				struct v4l2_subdev_selection *sel)
 {
+	struct imx290 *imx290 = to_imx290(sd);
 	struct v4l2_mbus_framefmt *format;
 
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP: {
 		format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
 
+		/*
+		 * The sensor moves the readout by 1 pixel based on flips to
+		 * keep the Bayer order the same.
+		 */
 		sel->r.top = IMX920_PIXEL_ARRAY_MARGIN_TOP
-			   + (IMX290_PIXEL_ARRAY_RECORDING_HEIGHT - format->height) / 2;
+			   + (IMX290_PIXEL_ARRAY_RECORDING_HEIGHT - format->height) / 2
+			   + imx290->vflip->val;
 		sel->r.left = IMX920_PIXEL_ARRAY_MARGIN_LEFT
-			    + (IMX290_PIXEL_ARRAY_RECORDING_WIDTH - format->width) / 2;
+			    + (IMX290_PIXEL_ARRAY_RECORDING_WIDTH - format->width) / 2
+			    + imx290->hflip->val;
 		sel->r.width = format->width;
 		sel->r.height = format->height;
 
-- 
2.34.1

Re: [PATCH v2 12/13] media: i2c: imx290: Add support for H & V Flips

[Laurent Pinchart]

Hi Dave,

Thank you for the patch.

On Fri, Feb 03, 2023 at 07:18:10PM +0000, Dave Stevenson wrote:
> The sensor supports H & V flips, so add the relevant hooks for
> V4L2_CID_HFLIP and V4L2_CID_VFLIP to configure them.
> 
> Note that the Bayer order is maintained as the readout area
> shifts by 1 pixel in the appropriate direction (note the
> comment about the top margin being 8 pixels whilst the bottom
> margin is 9). The V4L2_SEL_TGT_CROP region is therefore
> adjusted appropriately.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/imx290.c | 51 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 045d27b4d31b..7167eb1edb9b 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -208,6 +208,7 @@ struct imx290_mode {
>  	u32 hmax_min;
>  	u32 vmax_min;
>  	u8 link_freq_index;
> +	u8 ctrl_07;
>  
>  	const struct imx290_regval *data;
>  	u32 data_size;
> @@ -248,6 +249,10 @@ struct imx290 {
>  	struct v4l2_ctrl *hblank;
>  	struct v4l2_ctrl *vblank;
>  	struct v4l2_ctrl *exposure;
> +	struct {
> +		struct v4l2_ctrl *hflip;
> +		struct v4l2_ctrl *vflip;
> +	};
>  };
>  
>  static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
> @@ -325,7 +330,6 @@ static const struct imx290_regval xclk_regs[][IMX290_NUM_CLK_REGS] = {
>  
>  static const struct imx290_regval imx290_1080p_settings[] = {
>  	/* mode settings */
> -	{ IMX290_CTRL_07, IMX290_WINMODE_1080P },
>  	{ IMX290_WINWV_OB, 12 },
>  	{ IMX290_OPB_SIZE_V, 10 },
>  	{ IMX290_X_OUT_SIZE, 1920 },
> @@ -334,7 +338,6 @@ static const struct imx290_regval imx290_1080p_settings[] = {
>  
>  static const struct imx290_regval imx290_720p_settings[] = {
>  	/* mode settings */
> -	{ IMX290_CTRL_07, IMX290_WINMODE_720P },
>  	{ IMX290_WINWV_OB, 6 },
>  	{ IMX290_OPB_SIZE_V, 4 },
>  	{ IMX290_X_OUT_SIZE, 1280 },
> @@ -494,6 +497,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
>  		.hmax_min = 2200,
>  		.vmax_min = 1125,
>  		.link_freq_index = FREQ_INDEX_1080P,
> +		.ctrl_07 = IMX290_WINMODE_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
>  		.clk_cfg = imx290_1080p_clock_config,
> @@ -504,6 +508,7 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
>  		.hmax_min = 3300,
>  		.vmax_min = 750,
>  		.link_freq_index = FREQ_INDEX_720P,
> +		.ctrl_07 = IMX290_WINMODE_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
>  		.clk_cfg = imx290_720p_clock_config,
> @@ -517,6 +522,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
>  		.hmax_min = 2200,
>  		.vmax_min = 1125,
>  		.link_freq_index = FREQ_INDEX_1080P,
> +		.ctrl_07 = IMX290_WINMODE_1080P,
>  		.data = imx290_1080p_settings,
>  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
>  		.clk_cfg = imx290_1080p_clock_config,
> @@ -527,6 +533,7 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
>  		.hmax_min = 3300,
>  		.vmax_min = 750,
>  		.link_freq_index = FREQ_INDEX_720P,
> +		.ctrl_07 = IMX290_WINMODE_720P,
>  		.data = imx290_720p_settings,
>  		.data_size = ARRAY_SIZE(imx290_720p_settings),
>  		.clk_cfg = imx290_720p_clock_config,
> @@ -835,6 +842,20 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
>  				   NULL);
>  		break;
>  
> +	case V4L2_CID_HFLIP:
> +	case V4L2_CID_VFLIP:
> +	{
> +		u32 reg;
> +
> +		reg = imx290->current_mode->ctrl_07;
> +		if (imx290->hflip->val)
> +			reg |= IMX290_HREVERSE;
> +		if (imx290->vflip->val)
> +			reg |= IMX290_VREVERSE;
> +		ret = imx290_write(imx290, IMX290_CTRL_07, reg, NULL);
> +		break;
> +	}
> +
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -887,7 +908,7 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>  	if (ret < 0)
>  		return ret;
>  
> -	v4l2_ctrl_handler_init(&imx290->ctrls, 9);
> +	v4l2_ctrl_handler_init(&imx290->ctrls, 11);
>  
>  	/*
>  	 * The sensor has an analog gain and a digital gain, both controlled
> @@ -944,6 +965,12 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>  	imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
>  					   V4L2_CID_VBLANK, 1, 1, 1, 1);
>  
> +	imx290->hflip = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> +					  V4L2_CID_HFLIP, 0, 1, 1, 0);
> +	imx290->vflip = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> +					  V4L2_CID_VFLIP, 0, 1, 1, 0);
> +	v4l2_ctrl_cluster(2, &imx290->hflip);
> +
>  	v4l2_ctrl_new_fwnode_properties(&imx290->ctrls, &imx290_ctrl_ops,
>  					&props);
>  
> @@ -1065,6 +1092,13 @@ static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
>  		pm_runtime_put_autosuspend(imx290->dev);
>  	}
>  
> +	/*
> +	 * vflip and hflip should not be changed during streaming as the sensor
> +	 * will produce an invalid frame.
> +	 */
> +	__v4l2_ctrl_grab(imx290->vflip, enable);
> +	__v4l2_ctrl_grab(imx290->hflip, enable);
> +
>  unlock:
>  	v4l2_subdev_unlock_state(state);
>  	return ret;
> @@ -1147,16 +1181,23 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
>  				struct v4l2_subdev_state *sd_state,
>  				struct v4l2_subdev_selection *sel)
>  {
> +	struct imx290 *imx290 = to_imx290(sd);
>  	struct v4l2_mbus_framefmt *format;
>  
>  	switch (sel->target) {
>  	case V4L2_SEL_TGT_CROP: {
>  		format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
>  
> +		/*
> +		 * The sensor moves the readout by 1 pixel based on flips to
> +		 * keep the Bayer order the same.
> +		 */
>  		sel->r.top = IMX920_PIXEL_ARRAY_MARGIN_TOP
> -			   + (IMX290_PIXEL_ARRAY_RECORDING_HEIGHT - format->height) / 2;
> +			   + (IMX290_PIXEL_ARRAY_RECORDING_HEIGHT - format->height) / 2
> +			   + imx290->vflip->val;
>  		sel->r.left = IMX920_PIXEL_ARRAY_MARGIN_LEFT
> -			    + (IMX290_PIXEL_ARRAY_RECORDING_WIDTH - format->width) / 2;
> +			    + (IMX290_PIXEL_ARRAY_RECORDING_WIDTH - format->width) / 2
> +			    + imx290->hflip->val;
>  		sel->r.width = format->width;
>  		sel->r.height = format->height;
>  

-- 
Regards,

Laurent Pinchart

[PATCH v2 13/13] media: i2c: imx290: Add the error code to logs in start_streaming

[Dave Stevenson]

imx290_start_streaming logs what failed, but not the error
code from that function. Add it into the log message.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx290.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 7167eb1edb9b..f635f1213477 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -1007,20 +1007,20 @@ static int imx290_start_streaming(struct imx290 *imx290,
 	/* Set clock parameters based on mode and xclk */
 	ret = imx290_set_clock(imx290);
 	if (ret < 0) {
-		dev_err(imx290->dev, "Could not set clocks\n");
+		dev_err(imx290->dev, "Could not set clocks - %d\n", ret);
 		return ret;
 	}
 
 	/* Set data lane count */
 	ret = imx290_set_data_lanes(imx290);
 	if (ret < 0) {
-		dev_err(imx290->dev, "Could not set data lanes\n");
+		dev_err(imx290->dev, "Could not set data lanes - %d\n", ret);
 		return ret;
 	}
 
 	ret = imx290_set_csi_config(imx290);
 	if (ret < 0) {
-		dev_err(imx290->dev, "Could not set csi cfg\n");
+		dev_err(imx290->dev, "Could not set csi cfg - %d\n", ret);
 		return ret;
 	}
 
@@ -1028,7 +1028,7 @@ static int imx290_start_streaming(struct imx290 *imx290,
 	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
 	ret = imx290_setup_format(imx290, format);
 	if (ret < 0) {
-		dev_err(imx290->dev, "Could not set frame format\n");
+		dev_err(imx290->dev, "Could not set frame format - %d\n", ret);
 		return ret;
 	}
 
@@ -1036,14 +1036,14 @@ static int imx290_start_streaming(struct imx290 *imx290,
 	ret = imx290_set_register_array(imx290, imx290->current_mode->data,
 					imx290->current_mode->data_size);
 	if (ret < 0) {
-		dev_err(imx290->dev, "Could not set current mode\n");
+		dev_err(imx290->dev, "Could not set current mode - %d\n", ret);
 		return ret;
 	}
 
 	/* Apply customized values from user */
 	ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
 	if (ret) {
-		dev_err(imx290->dev, "Could not sync v4l2 controls\n");
+		dev_err(imx290->dev, "Could not sync v4l2 controls - %d\n", ret);
 		return ret;
 	}
 
-- 
2.34.1

Re: [PATCH v2 13/13] media: i2c: imx290: Add the error code to logs in start_streaming

[Alexander Stein]

Hi Dave,

thanks for the update.

Am Freitag, 3. Februar 2023, 20:18:11 CET schrieb Dave Stevenson:
> imx290_start_streaming logs what failed, but not the error
> code from that function. Add it into the log message.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> ---
>  drivers/media/i2c/imx290.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 7167eb1edb9b..f635f1213477 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -1007,20 +1007,20 @@ static int imx290_start_streaming(struct imx290
> *imx290, /* Set clock parameters based on mode and xclk */
>  	ret = imx290_set_clock(imx290);
>  	if (ret < 0) {
> -		dev_err(imx290->dev, "Could not set clocks\n");
> +		dev_err(imx290->dev, "Could not set clocks - %d\n", ret);
>  		return ret;
>  	}
> 
>  	/* Set data lane count */
>  	ret = imx290_set_data_lanes(imx290);
>  	if (ret < 0) {
> -		dev_err(imx290->dev, "Could not set data lanes\n");
> +		dev_err(imx290->dev, "Could not set data lanes - %d\n", 
ret);
>  		return ret;
>  	}
> 
>  	ret = imx290_set_csi_config(imx290);
>  	if (ret < 0) {
> -		dev_err(imx290->dev, "Could not set csi cfg\n");
> +		dev_err(imx290->dev, "Could not set csi cfg - %d\n", ret);
>  		return ret;
>  	}
> 
> @@ -1028,7 +1028,7 @@ static int imx290_start_streaming(struct imx290
> *imx290, format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
>  	ret = imx290_setup_format(imx290, format);
>  	if (ret < 0) {
> -		dev_err(imx290->dev, "Could not set frame format\n");
> +		dev_err(imx290->dev, "Could not set frame format - %d\n", 
ret);
>  		return ret;
>  	}
> 
> @@ -1036,14 +1036,14 @@ static int imx290_start_streaming(struct imx290
> *imx290, ret = imx290_set_register_array(imx290,
> imx290->current_mode->data, imx290->current_mode->data_size);
>  	if (ret < 0) {
> -		dev_err(imx290->dev, "Could not set current mode\n");
> +		dev_err(imx290->dev, "Could not set current mode - %d\n", 
ret);
>  		return ret;
>  	}
> 
>  	/* Apply customized values from user */
>  	ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
>  	if (ret) {
> -		dev_err(imx290->dev, "Could not sync v4l2 controls\n");
> +		dev_err(imx290->dev, "Could not sync v4l2 controls - 
%d\n", ret);
>  		return ret;
>  	}

Re: [PATCH v2 13/13] media: i2c: imx290: Add the error code to logs in start_streaming

[Laurent Pinchart]

Hi Dave,

Thank you for the patch.

On Fri, Feb 03, 2023 at 07:18:11PM +0000, Dave Stevenson wrote:
> imx290_start_streaming logs what failed, but not the error
> code from that function. Add it into the log message.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/imx290.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 7167eb1edb9b..f635f1213477 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -1007,20 +1007,20 @@ static int imx290_start_streaming(struct imx290 *imx290,
>  	/* Set clock parameters based on mode and xclk */
>  	ret = imx290_set_clock(imx290);
>  	if (ret < 0) {
> -		dev_err(imx290->dev, "Could not set clocks\n");
> +		dev_err(imx290->dev, "Could not set clocks - %d\n", ret);
>  		return ret;
>  	}
>  
>  	/* Set data lane count */
>  	ret = imx290_set_data_lanes(imx290);
>  	if (ret < 0) {
> -		dev_err(imx290->dev, "Could not set data lanes\n");
> +		dev_err(imx290->dev, "Could not set data lanes - %d\n", ret);
>  		return ret;
>  	}
>  
>  	ret = imx290_set_csi_config(imx290);
>  	if (ret < 0) {
> -		dev_err(imx290->dev, "Could not set csi cfg\n");
> +		dev_err(imx290->dev, "Could not set csi cfg - %d\n", ret);
>  		return ret;
>  	}
>  
> @@ -1028,7 +1028,7 @@ static int imx290_start_streaming(struct imx290 *imx290,
>  	format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0);
>  	ret = imx290_setup_format(imx290, format);
>  	if (ret < 0) {
> -		dev_err(imx290->dev, "Could not set frame format\n");
> +		dev_err(imx290->dev, "Could not set frame format - %d\n", ret);
>  		return ret;
>  	}
>  
> @@ -1036,14 +1036,14 @@ static int imx290_start_streaming(struct imx290 *imx290,
>  	ret = imx290_set_register_array(imx290, imx290->current_mode->data,
>  					imx290->current_mode->data_size);
>  	if (ret < 0) {
> -		dev_err(imx290->dev, "Could not set current mode\n");
> +		dev_err(imx290->dev, "Could not set current mode - %d\n", ret);
>  		return ret;
>  	}
>  
>  	/* Apply customized values from user */
>  	ret = __v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
>  	if (ret) {
> -		dev_err(imx290->dev, "Could not sync v4l2 controls\n");
> +		dev_err(imx290->dev, "Could not sync v4l2 controls - %d\n", ret);
>  		return ret;
>  	}
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls

[Laurent Pinchart]

Hi Dave,

On Fri, Feb 03, 2023 at 07:17:58PM +0000, Dave Stevenson wrote:
> Hi All
> 
> This is a small patch set that fixes a number of issues, adds in support
> for the alternate input clock frequency option, and expands the control support
> for flips and VBLANK/HBLANK.
> 
> My source tree has the 2 patches I've just sent for mono support first, but I
> believe the two series should apply in either order (true for v1, possibly not
> for v2).
> There's tree with both patchsets applied at [1]. 

This is a great series, thank you for upstreaming it.

Patch 04/13 conflicts with the latest media tree, but the conflict is
easy to fix. I've taken the whole series in my tree to test it when I'll
get access to hardware at the end of this week, I can then post a v3
with the review comments addressed. As for the other imx290 series
you've submitted, feel free to beat me to it if you want to.

> Changes since v1:
> 2/11: Hardcoded colourspace values.
> 4/11: Missing blank line added.
> 6/11: s/repitition/repetition/
> 8/11: Set exposure range on changing mode.
> 8/11: Alter the initial exposure to be max.
> 8/11: Split out allowing VMAX to drop to 750 in 720p mode into a new patch.
> 10/11: Switch to an enum.
> 10/11: Compute IMX290_EXTCK_FREQ.
> 10/11: Move EXCK_FREQ and IMX290_INCKSEL7 settings to 
> New patch: Add error code to logging in imx290_start_streaming.
> Collected R-B tags.
> 
> [1] https://github.com/6by9/linux/tree/upstream_imx290
> 
> Dave Stevenson (13):
>   media: i2c: imx290: Match kernel coding style on whitespace
>   media: i2c: imx290: Set the colorspace fields in the format
>   media: i2c: imx290: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks
>   media: i2c: imx290: Fix the pixel rate at 148.5Mpix/s
>   media: i2c: imx290: Support 60fps in 2 lane operation
>   media: i2c: imx290: Use CSI timings as per datasheet
>   media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write
>   media: i2c: imx290: Convert V4L2_CID_VBLANK to read/write
>   media: i2c: imx290: VMAX is mode dependent
>   media: i2c: imx290: Remove duplicated write to IMX290_CTRL_07
>   media: i2c: imx290: Add support for 74.25MHz external clock
>   media: i2c: imx290: Add support for H & V Flips
>   media: i2c: imx290: Add the error code to logs in start_streaming
> 
>  drivers/media/i2c/imx290.c | 464 +++++++++++++++++++++++++++++--------
>  1 file changed, 365 insertions(+), 99 deletions(-)

-- 
Regards,

Laurent Pinchart