* [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls
@ 2023-02-03 19:17 Dave Stevenson
2023-02-03 19:17 ` [PATCH v2 01/13] media: i2c: imx290: Match kernel coding style on whitespace Dave Stevenson
` (13 more replies)
0 siblings, 14 replies; 24+ messages in thread
From: Dave Stevenson @ 2023-02-03 19:17 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Alexander Stein
Cc: 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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 01/13] media: i2c: imx290: Match kernel coding style on whitespace
2023-02-03 19:17 [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Dave Stevenson
@ 2023-02-03 19:17 ` Dave Stevenson
2023-02-03 19:18 ` [PATCH v2 02/13] media: i2c: imx290: Set the colorspace fields in the format Dave Stevenson
` (12 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Dave Stevenson @ 2023-02-03 19:17 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Alexander Stein
Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 02/13] media: i2c: imx290: Set the colorspace fields in the format
2023-02-03 19:17 [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Dave Stevenson
2023-02-03 19:17 ` [PATCH v2 01/13] media: i2c: imx290: Match kernel coding style on whitespace Dave Stevenson
@ 2023-02-03 19:18 ` Dave Stevenson
2023-02-07 1:34 ` Laurent Pinchart
2023-02-03 19:18 ` [PATCH v2 03/13] media: i2c: imx290: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks Dave Stevenson
` (11 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: Dave Stevenson @ 2023-02-03 19:18 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Alexander Stein
Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 03/13] media: i2c: imx290: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks
2023-02-03 19:17 [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Dave Stevenson
2023-02-03 19:17 ` [PATCH v2 01/13] media: i2c: imx290: Match kernel coding style on whitespace Dave Stevenson
2023-02-03 19:18 ` [PATCH v2 02/13] media: i2c: imx290: Set the colorspace fields in the format Dave Stevenson
@ 2023-02-03 19:18 ` Dave Stevenson
2023-02-03 19:18 ` [PATCH v2 04/13] media: i2c: imx290: Fix the pixel rate at 148.5Mpix/s Dave Stevenson
` (10 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Dave Stevenson @ 2023-02-03 19:18 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Alexander Stein
Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 04/13] media: i2c: imx290: Fix the pixel rate at 148.5Mpix/s
2023-02-03 19:17 [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Dave Stevenson
` (2 preceding siblings ...)
2023-02-03 19:18 ` [PATCH v2 03/13] media: i2c: imx290: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks Dave Stevenson
@ 2023-02-03 19:18 ` Dave Stevenson
2023-02-03 19:18 ` [PATCH v2 05/13] media: i2c: imx290: Support 60fps in 2 lane operation Dave Stevenson
` (9 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Dave Stevenson @ 2023-02-03 19:18 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Alexander Stein
Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 05/13] media: i2c: imx290: Support 60fps in 2 lane operation
2023-02-03 19:17 [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Dave Stevenson
` (3 preceding siblings ...)
2023-02-03 19:18 ` [PATCH v2 04/13] media: i2c: imx290: Fix the pixel rate at 148.5Mpix/s Dave Stevenson
@ 2023-02-03 19:18 ` Dave Stevenson
2023-02-03 19:18 ` [PATCH v2 06/13] media: i2c: imx290: Use CSI timings as per datasheet Dave Stevenson
` (8 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Dave Stevenson @ 2023-02-03 19:18 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Alexander Stein
Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 06/13] media: i2c: imx290: Use CSI timings as per datasheet
2023-02-03 19:17 [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Dave Stevenson
` (4 preceding siblings ...)
2023-02-03 19:18 ` [PATCH v2 05/13] media: i2c: imx290: Support 60fps in 2 lane operation Dave Stevenson
@ 2023-02-03 19:18 ` Dave Stevenson
2023-02-06 7:48 ` Alexander Stein
2023-02-03 19:18 ` [PATCH v2 07/13] media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write Dave Stevenson
` (7 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: Dave Stevenson @ 2023-02-03 19:18 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Alexander Stein
Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 07/13] media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write
2023-02-03 19:17 [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Dave Stevenson
` (5 preceding siblings ...)
2023-02-03 19:18 ` [PATCH v2 06/13] media: i2c: imx290: Use CSI timings as per datasheet Dave Stevenson
@ 2023-02-03 19:18 ` Dave Stevenson
2023-02-06 7:59 ` Alexander Stein
2023-02-03 19:18 ` [PATCH v2 08/13] media: i2c: imx290: Convert V4L2_CID_VBLANK " Dave Stevenson
` (6 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: Dave Stevenson @ 2023-02-03 19:18 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Alexander Stein
Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 08/13] media: i2c: imx290: Convert V4L2_CID_VBLANK to read/write
2023-02-03 19:17 [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Dave Stevenson
` (6 preceding siblings ...)
2023-02-03 19:18 ` [PATCH v2 07/13] media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write Dave Stevenson
@ 2023-02-03 19:18 ` Dave Stevenson
2023-02-03 19:18 ` [PATCH v2 09/13] media: i2c: imx290: VMAX is mode dependent Dave Stevenson
` (5 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Dave Stevenson @ 2023-02-03 19:18 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Alexander Stein
Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 09/13] media: i2c: imx290: VMAX is mode dependent
2023-02-03 19:17 [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Dave Stevenson
` (7 preceding siblings ...)
2023-02-03 19:18 ` [PATCH v2 08/13] media: i2c: imx290: Convert V4L2_CID_VBLANK " Dave Stevenson
@ 2023-02-03 19:18 ` Dave Stevenson
2023-02-07 1:46 ` Laurent Pinchart
2023-02-03 19:18 ` [PATCH v2 10/13] media: i2c: imx290: Remove duplicated write to IMX290_CTRL_07 Dave Stevenson
` (4 subsequent siblings)
13 siblings, 1 reply; 24+ messages in thread
From: Dave Stevenson @ 2023-02-03 19:18 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Alexander Stein
Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 10/13] media: i2c: imx290: Remove duplicated write to IMX290_CTRL_07
2023-02-03 19:17 [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Dave Stevenson
` (8 preceding siblings ...)
2023-02-03 19:18 ` [PATCH v2 09/13] media: i2c: imx290: VMAX is mode dependent Dave Stevenson
@ 2023-02-03 19:18 ` Dave Stevenson
2023-02-03 19:18 ` [PATCH v2 11/13] media: i2c: imx290: Add support for 74.25MHz external clock Dave Stevenson
` (3 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Dave Stevenson @ 2023-02-03 19:18 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Alexander Stein
Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 11/13] media: i2c: imx290: Add support for 74.25MHz external clock
2023-02-03 19:17 [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Dave Stevenson
` (9 preceding siblings ...)
2023-02-03 19:18 ` [PATCH v2 10/13] media: i2c: imx290: Remove duplicated write to IMX290_CTRL_07 Dave Stevenson
@ 2023-02-03 19:18 ` Dave Stevenson
2023-02-06 8:24 ` Alexander Stein
2023-02-07 1:50 ` Laurent Pinchart
2023-02-03 19:18 ` [PATCH v2 12/13] media: i2c: imx290: Add support for H & V Flips Dave Stevenson
` (2 subsequent siblings)
13 siblings, 2 replies; 24+ messages in thread
From: Dave Stevenson @ 2023-02-03 19:18 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Alexander Stein
Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 12/13] media: i2c: imx290: Add support for H & V Flips
2023-02-03 19:17 [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Dave Stevenson
` (10 preceding siblings ...)
2023-02-03 19:18 ` [PATCH v2 11/13] media: i2c: imx290: Add support for 74.25MHz external clock Dave Stevenson
@ 2023-02-03 19:18 ` Dave Stevenson
2023-02-07 1:52 ` Laurent Pinchart
2023-02-03 19:18 ` [PATCH v2 13/13] media: i2c: imx290: Add the error code to logs in start_streaming Dave Stevenson
2023-02-07 1:55 ` [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Laurent Pinchart
13 siblings, 1 reply; 24+ messages in thread
From: Dave Stevenson @ 2023-02-03 19:18 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Alexander Stein
Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 13/13] media: i2c: imx290: Add the error code to logs in start_streaming
2023-02-03 19:17 [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Dave Stevenson
` (11 preceding siblings ...)
2023-02-03 19:18 ` [PATCH v2 12/13] media: i2c: imx290: Add support for H & V Flips Dave Stevenson
@ 2023-02-03 19:18 ` Dave Stevenson
2023-02-06 8:27 ` Alexander Stein
2023-02-07 1:53 ` Laurent Pinchart
2023-02-07 1:55 ` [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Laurent Pinchart
13 siblings, 2 replies; 24+ messages in thread
From: Dave Stevenson @ 2023-02-03 19:18 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Alexander Stein
Cc: 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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 06/13] media: i2c: imx290: Use CSI timings as per datasheet
2023-02-03 19:18 ` [PATCH v2 06/13] media: i2c: imx290: Use CSI timings as per datasheet Dave Stevenson
@ 2023-02-06 7:48 ` Alexander Stein
0 siblings, 0 replies; 24+ messages in thread
From: Alexander Stein @ 2023-02-06 7:48 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Dave Stevenson
Cc: Dave Stevenson
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);
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 07/13] media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write
2023-02-03 19:18 ` [PATCH v2 07/13] media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write Dave Stevenson
@ 2023-02-06 7:59 ` Alexander Stein
0 siblings, 0 replies; 24+ messages in thread
From: Alexander Stein @ 2023-02-06 7:59 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Dave Stevenson
Cc: Dave Stevenson
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) {
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 11/13] media: i2c: imx290: Add support for 74.25MHz external clock
2023-02-03 19:18 ` [PATCH v2 11/13] media: i2c: imx290: Add support for 74.25MHz external clock Dave Stevenson
@ 2023-02-06 8:24 ` Alexander Stein
2023-02-07 1:50 ` Laurent Pinchart
1 sibling, 0 replies; 24+ messages in thread
From: Alexander Stein @ 2023-02-06 8:24 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Dave Stevenson
Cc: Dave Stevenson
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;
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 13/13] media: i2c: imx290: Add the error code to logs in start_streaming
2023-02-03 19:18 ` [PATCH v2 13/13] media: i2c: imx290: Add the error code to logs in start_streaming Dave Stevenson
@ 2023-02-06 8:27 ` Alexander Stein
2023-02-07 1:53 ` Laurent Pinchart
1 sibling, 0 replies; 24+ messages in thread
From: Alexander Stein @ 2023-02-06 8:27 UTC (permalink / raw)
To: Manivannan Sadhasivam, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Dave Stevenson
Cc: Dave Stevenson
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;
> }
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 02/13] media: i2c: imx290: Set the colorspace fields in the format
2023-02-03 19:18 ` [PATCH v2 02/13] media: i2c: imx290: Set the colorspace fields in the format Dave Stevenson
@ 2023-02-07 1:34 ` Laurent Pinchart
0 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-02-07 1:34 UTC (permalink / raw)
To: Dave Stevenson
Cc: Manivannan Sadhasivam, Mauro Carvalho Chehab, linux-media,
Alexander Stein
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 09/13] media: i2c: imx290: VMAX is mode dependent
2023-02-03 19:18 ` [PATCH v2 09/13] media: i2c: imx290: VMAX is mode dependent Dave Stevenson
@ 2023-02-07 1:46 ` Laurent Pinchart
0 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-02-07 1:46 UTC (permalink / raw)
To: Dave Stevenson
Cc: Manivannan Sadhasivam, Mauro Carvalho Chehab, linux-media,
Alexander Stein
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 11/13] media: i2c: imx290: Add support for 74.25MHz external clock
2023-02-03 19:18 ` [PATCH v2 11/13] media: i2c: imx290: Add support for 74.25MHz external clock Dave Stevenson
2023-02-06 8:24 ` Alexander Stein
@ 2023-02-07 1:50 ` Laurent Pinchart
1 sibling, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-02-07 1:50 UTC (permalink / raw)
To: Dave Stevenson
Cc: Manivannan Sadhasivam, Mauro Carvalho Chehab, linux-media,
Alexander Stein
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 12/13] media: i2c: imx290: Add support for H & V Flips
2023-02-03 19:18 ` [PATCH v2 12/13] media: i2c: imx290: Add support for H & V Flips Dave Stevenson
@ 2023-02-07 1:52 ` Laurent Pinchart
0 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-02-07 1:52 UTC (permalink / raw)
To: Dave Stevenson
Cc: Manivannan Sadhasivam, Mauro Carvalho Chehab, linux-media,
Alexander Stein
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 13/13] media: i2c: imx290: Add the error code to logs in start_streaming
2023-02-03 19:18 ` [PATCH v2 13/13] media: i2c: imx290: Add the error code to logs in start_streaming Dave Stevenson
2023-02-06 8:27 ` Alexander Stein
@ 2023-02-07 1:53 ` Laurent Pinchart
1 sibling, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-02-07 1:53 UTC (permalink / raw)
To: Dave Stevenson
Cc: Manivannan Sadhasivam, Mauro Carvalho Chehab, linux-media,
Alexander Stein
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls
2023-02-03 19:17 [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Dave Stevenson
` (12 preceding siblings ...)
2023-02-03 19:18 ` [PATCH v2 13/13] media: i2c: imx290: Add the error code to logs in start_streaming Dave Stevenson
@ 2023-02-07 1:55 ` Laurent Pinchart
13 siblings, 0 replies; 24+ messages in thread
From: Laurent Pinchart @ 2023-02-07 1:55 UTC (permalink / raw)
To: Dave Stevenson
Cc: Manivannan Sadhasivam, Mauro Carvalho Chehab, linux-media,
Alexander Stein
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
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2023-02-07 1:55 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-03 19:17 [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Dave Stevenson
2023-02-03 19:17 ` [PATCH v2 01/13] media: i2c: imx290: Match kernel coding style on whitespace Dave Stevenson
2023-02-03 19:18 ` [PATCH v2 02/13] media: i2c: imx290: Set the colorspace fields in the format Dave Stevenson
2023-02-07 1:34 ` Laurent Pinchart
2023-02-03 19:18 ` [PATCH v2 03/13] media: i2c: imx290: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks Dave Stevenson
2023-02-03 19:18 ` [PATCH v2 04/13] media: i2c: imx290: Fix the pixel rate at 148.5Mpix/s Dave Stevenson
2023-02-03 19:18 ` [PATCH v2 05/13] media: i2c: imx290: Support 60fps in 2 lane operation Dave Stevenson
2023-02-03 19:18 ` [PATCH v2 06/13] media: i2c: imx290: Use CSI timings as per datasheet Dave Stevenson
2023-02-06 7:48 ` Alexander Stein
2023-02-03 19:18 ` [PATCH v2 07/13] media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write Dave Stevenson
2023-02-06 7:59 ` Alexander Stein
2023-02-03 19:18 ` [PATCH v2 08/13] media: i2c: imx290: Convert V4L2_CID_VBLANK " Dave Stevenson
2023-02-03 19:18 ` [PATCH v2 09/13] media: i2c: imx290: VMAX is mode dependent Dave Stevenson
2023-02-07 1:46 ` Laurent Pinchart
2023-02-03 19:18 ` [PATCH v2 10/13] media: i2c: imx290: Remove duplicated write to IMX290_CTRL_07 Dave Stevenson
2023-02-03 19:18 ` [PATCH v2 11/13] media: i2c: imx290: Add support for 74.25MHz external clock Dave Stevenson
2023-02-06 8:24 ` Alexander Stein
2023-02-07 1:50 ` Laurent Pinchart
2023-02-03 19:18 ` [PATCH v2 12/13] media: i2c: imx290: Add support for H & V Flips Dave Stevenson
2023-02-07 1:52 ` Laurent Pinchart
2023-02-03 19:18 ` [PATCH v2 13/13] media: i2c: imx290: Add the error code to logs in start_streaming Dave Stevenson
2023-02-06 8:27 ` Alexander Stein
2023-02-07 1:53 ` Laurent Pinchart
2023-02-07 1:55 ` [PATCH v2 00/13] imx290: Minor fixes, support for alternate INCK, and more ctrls Laurent Pinchart
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.