public inbox for linux-media@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls
@ 2023-02-15 22:29 Laurent Pinchart
  2023-02-15 22:29 ` [PATCH v3 01/15] media: dt-bindings: media: i2c: Add mono version to IMX290 bindings Laurent Pinchart
                   ` (16 more replies)
  0 siblings, 17 replies; 20+ messages in thread
From: Laurent Pinchart @ 2023-02-15 22:29 UTC (permalink / raw)
  To: linux-media
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Alexander Stein, Rob Herring, Krzysztof Kozlowski, devicetree

Hello,

This patch series combines the "[PATCH v2 0/2] Add support for mono
version of Sony IMX290 sensor" ([1]) and "[PATCH v2 00/13] imx290: Minor
fixes, support for alternate INCK, and more ctrls" ([2]) previously
submitted by Dave into a single series.

As promised in my review of v2 of both series, I have tested the changes
with my IMX327 camera sensor, connected to the i.MX8MP ISP, with
libcamera. I haven't noticed any regression (but, full disclaimer, I
haven't tested all the newly features). I think we're thus good to go.

This version handles all review comments from v2, resulting in the
following changes:

- Deprecate the sony,imx290 compatible
- Update the DT example to use the new sony,imx290lqr compatible
- Drop unneeded pointer cast
- Don't move imx290_of_match table
- Fix typos

The code has also been rebased on top of the latest media master branch,
with rebase conflicts and rebase-induced compilation breakages fixed.

The patches are available from

git://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git next/media/sensors/imx290

[1] https://lore.kernel.org/linux-media/20230203191644.947643-1-dave.stevenson@raspberrypi.com
[2] https://lore.kernel.org/linux-media/20230203191811.947697-1-dave.stevenson@raspberrypi.com

Dave Stevenson (15):
  media: dt-bindings: media: i2c: Add mono version to IMX290 bindings
  media: i2c: imx290: Add support for the mono sensor variant
  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

 .../bindings/media/i2c/sony,imx290.yaml       |  24 +-
 drivers/media/i2c/imx290.c                    | 537 ++++++++++++++----
 2 files changed, 442 insertions(+), 119 deletions(-)


base-commit: 83e0f265aa8d0e37cc8e15d318b64da0ec03ff41
-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v3 01/15] media: dt-bindings: media: i2c: Add mono version to IMX290 bindings
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
@ 2023-02-15 22:29 ` Laurent Pinchart
  2023-02-15 22:29 ` [PATCH v3 02/15] media: i2c: imx290: Add support for the mono sensor variant Laurent Pinchart
                   ` (15 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2023-02-15 22:29 UTC (permalink / raw)
  To: linux-media
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Alexander Stein, Rob Herring, Krzysztof Kozlowski, devicetree

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

The IMX290 module is available as either monochrome or colour and
the variant is not detectable at runtime.

Add a new compatible string for the monochrome version, based on the
full device name IMX290LLR. For consistency, add a new compatible string
for the colour version based on the IMX290LQR full device name, and
deprecate the current ambiguous compatible string.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v2:

- Deprecate the sony,imx290 compatible
- Update the example to use the new sony,imx290lqr compatible.
---
 .../bindings/media/i2c/sony,imx290.yaml       | 24 +++++++++++++------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
index 21377daae026..cafb6e1a7150 100644
--- a/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
@@ -12,15 +12,25 @@ maintainers:
 
 description: |-
   The Sony IMX290 is a 1/2.8-Inch CMOS Solid-state image sensor with Square
-  Pixel for Color Cameras. It is programmable through I2C and 4-wire
-  interfaces. The sensor output is available via CMOS logic parallel SDR
-  output, Low voltage LVDS DDR output and CSI-2 serial data output. The CSI-2
-  bus is the default. No bindings have been defined for the other busses.
+  Pixel, available in either mono or colour variants. It is programmable
+  through I2C and 4-wire interfaces.
+
+  The sensor output is available via CMOS logic parallel SDR output, Low voltage
+  LVDS DDR output and CSI-2 serial data output. The CSI-2 bus is the default.
+  No bindings have been defined for the other busses.
+
+  imx290lqr is the full model identifier for the colour variant. "sony,imx290"
+  is treated the same as this as it was the original compatible string.
+  imx290llr is the mono version of the sensor.
 
 properties:
   compatible:
-    enum:
-      - sony,imx290
+    oneOf:
+      - enum:
+          - sony,imx290lqr # Colour
+          - sony,imx290llr # Monochrome
+      - const: sony,imx290
+        deprecated: true
 
   reg:
     maxItems: 1
@@ -101,7 +111,7 @@ examples:
         #size-cells = <0>;
 
         imx290: camera-sensor@1a {
-            compatible = "sony,imx290";
+            compatible = "sony,imx290lqr";
             reg = <0x1a>;
 
             pinctrl-names = "default";
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 02/15] media: i2c: imx290: Add support for the mono sensor variant
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
  2023-02-15 22:29 ` [PATCH v3 01/15] media: dt-bindings: media: i2c: Add mono version to IMX290 bindings Laurent Pinchart
@ 2023-02-15 22:29 ` Laurent Pinchart
  2023-02-15 22:29 ` [PATCH v3 03/15] media: i2c: imx290: Match kernel coding style on whitespace Laurent Pinchart
                   ` (14 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2023-02-15 22:29 UTC (permalink / raw)
  To: linux-media
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Alexander Stein, Rob Herring, Krzysztof Kozlowski, devicetree

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

The IMX290 module is available as either mono or colour (Bayer).

Update the driver so that it can advertise the correct mono
formats instead of the colour ones.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v2:

- Fix conflict causing compilation breakage
- Drop unneeded pointer cast
- Don't move imx290_of_match table
---
 drivers/media/i2c/imx290.c | 75 +++++++++++++++++++++++++++++++-------
 1 file changed, 61 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 49d6c8bdec41..febb8aeb9252 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -13,6 +13,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
@@ -157,6 +158,21 @@
 
 #define IMX290_NUM_SUPPLIES				3
 
+enum imx290_colour_variant {
+	IMX290_VARIANT_COLOUR,
+	IMX290_VARIANT_MONO,
+	IMX290_VARIANT_MAX
+};
+
+enum imx290_model {
+	IMX290_MODEL_IMX290LQR,
+	IMX290_MODEL_IMX290LLR,
+};
+
+struct imx290_model_info {
+	enum imx290_colour_variant colour_variant;
+};
+
 struct imx290_regval {
 	u32 reg;
 	u32 val;
@@ -177,6 +193,7 @@ struct imx290 {
 	struct clk *xclk;
 	struct regmap *regmap;
 	u8 nlanes;
+	const struct imx290_model_info *model;
 
 	struct v4l2_subdev sd;
 	struct media_pad pad;
@@ -414,7 +431,7 @@ static inline int imx290_modes_num(const struct imx290 *imx290)
 }
 
 struct imx290_format_info {
-	u32 code;
+	u32 code[IMX290_VARIANT_MAX];
 	u8 bpp;
 	const struct imx290_regval *regs;
 	unsigned int num_regs;
@@ -422,26 +439,33 @@ struct imx290_format_info {
 
 static const struct imx290_format_info imx290_formats[] = {
 	{
-		.code = MEDIA_BUS_FMT_SRGGB10_1X10,
+		.code = {
+			[IMX290_VARIANT_COLOUR] = MEDIA_BUS_FMT_SRGGB10_1X10,
+			[IMX290_VARIANT_MONO] = MEDIA_BUS_FMT_Y10_1X10
+		},
 		.bpp = 10,
 		.regs = imx290_10bit_settings,
 		.num_regs = ARRAY_SIZE(imx290_10bit_settings),
 	}, {
-		.code = MEDIA_BUS_FMT_SRGGB12_1X12,
+		.code = {
+			[IMX290_VARIANT_COLOUR] = MEDIA_BUS_FMT_SRGGB12_1X12,
+			[IMX290_VARIANT_MONO] = MEDIA_BUS_FMT_Y12_1X12
+		},
 		.bpp = 12,
 		.regs = imx290_12bit_settings,
 		.num_regs = ARRAY_SIZE(imx290_12bit_settings),
 	}
 };
 
-static const struct imx290_format_info *imx290_format_info(u32 code)
+static const struct imx290_format_info *
+imx290_format_info(const struct imx290 *imx290, u32 code)
 {
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(imx290_formats); ++i) {
 		const struct imx290_format_info *info = &imx290_formats[i];
 
-		if (info->code == code)
+		if (info->code[imx290->model->colour_variant] == code)
 			return info;
 	}
 
@@ -536,7 +560,7 @@ static int imx290_set_black_level(struct imx290 *imx290,
 				  const struct v4l2_mbus_framefmt *format,
 				  unsigned int black_level, int *err)
 {
-	unsigned int bpp = imx290_format_info(format->code)->bpp;
+	unsigned int bpp = imx290_format_info(imx290, format->code)->bpp;
 
 	return imx290_write(imx290, IMX290_BLKLEVEL,
 			    black_level >> (16 - bpp), err);
@@ -548,7 +572,7 @@ static int imx290_setup_format(struct imx290 *imx290,
 	const struct imx290_format_info *info;
 	int ret;
 
-	info = imx290_format_info(format->code);
+	info = imx290_format_info(imx290, format->code);
 
 	ret = imx290_set_register_array(imx290, info->regs, info->num_regs);
 	if (ret < 0) {
@@ -649,7 +673,7 @@ static void imx290_ctrl_update(struct imx290 *imx290,
 
 	/* 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);
+	do_div(pixel_rate, imx290_format_info(imx290, format->code)->bpp);
 
 	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
 	__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate, pixel_rate);
@@ -844,10 +868,12 @@ static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
 {
+	const struct imx290 *imx290 = to_imx290(sd);
+
 	if (code->index >= ARRAY_SIZE(imx290_formats))
 		return -EINVAL;
 
-	code->code = imx290_formats[code->index].code;
+	code->code = imx290_formats[code->index].code[imx290->model->colour_variant];
 
 	return 0;
 }
@@ -859,7 +885,7 @@ static int imx290_enum_frame_size(struct v4l2_subdev *sd,
 	const struct imx290 *imx290 = to_imx290(sd);
 	const struct imx290_mode *imx290_modes = imx290_modes_ptr(imx290);
 
-	if (!imx290_format_info(fse->code))
+	if (!imx290_format_info(imx290, fse->code))
 		return -EINVAL;
 
 	if (fse->index >= imx290_modes_num(imx290))
@@ -888,8 +914,8 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 	fmt->format.width = mode->width;
 	fmt->format.height = mode->height;
 
-	if (!imx290_format_info(fmt->format.code))
-		fmt->format.code = imx290_formats[0].code;
+	if (!imx290_format_info(imx290, fmt->format.code))
+		fmt->format.code = imx290_formats[0].code[imx290->model->colour_variant];
 
 	fmt->format.field = V4L2_FIELD_NONE;
 
@@ -1177,6 +1203,15 @@ static s64 imx290_check_link_freqs(const struct imx290 *imx290,
 	return 0;
 }
 
+static const struct imx290_model_info imx290_models[] = {
+	[IMX290_MODEL_IMX290LQR] = {
+		.colour_variant = IMX290_VARIANT_COLOUR,
+	},
+	[IMX290_MODEL_IMX290LLR] = {
+		.colour_variant = IMX290_VARIANT_MONO,
+	},
+};
+
 static int imx290_parse_dt(struct imx290 *imx290)
 {
 	/* Only CSI2 is supported for now: */
@@ -1187,6 +1222,8 @@ static int imx290_parse_dt(struct imx290 *imx290)
 	int ret;
 	s64 fq;
 
+	imx290->model = of_device_get_match_data(imx290->dev);
+
 	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(imx290->dev), NULL);
 	if (!endpoint) {
 		dev_err(imx290->dev, "Endpoint node not found\n");
@@ -1352,8 +1389,18 @@ static void imx290_remove(struct i2c_client *client)
 }
 
 static const struct of_device_id imx290_of_match[] = {
-	{ .compatible = "sony,imx290" },
-	{ /* sentinel */ }
+	{
+		/* Deprecated - synonym for "sony,imx290lqr" */
+		.compatible = "sony,imx290",
+		.data = &imx290_models[IMX290_MODEL_IMX290LQR],
+	}, {
+		.compatible = "sony,imx290lqr",
+		.data = &imx290_models[IMX290_MODEL_IMX290LQR],
+	}, {
+		.compatible = "sony,imx290llr",
+		.data = &imx290_models[IMX290_MODEL_IMX290LLR],
+	},
+	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, imx290_of_match);
 
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 03/15] media: i2c: imx290: Match kernel coding style on whitespace
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
  2023-02-15 22:29 ` [PATCH v3 01/15] media: dt-bindings: media: i2c: Add mono version to IMX290 bindings Laurent Pinchart
  2023-02-15 22:29 ` [PATCH v3 02/15] media: i2c: imx290: Add support for the mono sensor variant Laurent Pinchart
@ 2023-02-15 22:29 ` Laurent Pinchart
  2023-02-15 22:29 ` [PATCH v3 04/15] media: i2c: imx290: Set the colorspace fields in the format Laurent Pinchart
                   ` (13 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2023-02-15 22:29 UTC (permalink / raw)
  To: linux-media
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Alexander Stein

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

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>
Signed-off-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 febb8aeb9252..a8acc785d995 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;
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 04/15] media: i2c: imx290: Set the colorspace fields in the format
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
                   ` (2 preceding siblings ...)
  2023-02-15 22:29 ` [PATCH v3 03/15] media: i2c: imx290: Match kernel coding style on whitespace Laurent Pinchart
@ 2023-02-15 22:29 ` Laurent Pinchart
  2023-02-15 22:29 ` [PATCH v3 05/15] media: i2c: imx290: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks Laurent Pinchart
                   ` (12 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2023-02-15 22:29 UTC (permalink / raw)
  To: linux-media
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Alexander Stein

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

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>
Signed-off-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 a8acc785d995..d68752aea3cf 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 related	[flat|nested] 20+ messages in thread

* [PATCH v3 05/15] media: i2c: imx290: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
                   ` (3 preceding siblings ...)
  2023-02-15 22:29 ` [PATCH v3 04/15] media: i2c: imx290: Set the colorspace fields in the format Laurent Pinchart
@ 2023-02-15 22:29 ` Laurent Pinchart
  2023-02-15 22:29 ` [PATCH v3 06/15] media: i2c: imx290: Fix the pixel rate at 148.5Mpix/s Laurent Pinchart
                   ` (11 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2023-02-15 22:29 UTC (permalink / raw)
  To: linux-media
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Alexander Stein

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

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>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 d68752aea3cf..0a77391ece9b 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;
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 06/15] media: i2c: imx290: Fix the pixel rate at 148.5Mpix/s
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
                   ` (4 preceding siblings ...)
  2023-02-15 22:29 ` [PATCH v3 05/15] media: i2c: imx290: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks Laurent Pinchart
@ 2023-02-15 22:29 ` Laurent Pinchart
  2023-02-15 22:29 ` [PATCH v3 07/15] media: i2c: imx290: Support 60fps in 2 lane operation Laurent Pinchart
                   ` (10 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2023-02-15 22:29 UTC (permalink / raw)
  To: linux-media
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Alexander Stein

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

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>
Signed-off-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 0a77391ece9b..c0b37afd2fbe 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(imx290, 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,
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 07/15] media: i2c: imx290: Support 60fps in 2 lane operation
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
                   ` (5 preceding siblings ...)
  2023-02-15 22:29 ` [PATCH v3 06/15] media: i2c: imx290: Fix the pixel rate at 148.5Mpix/s Laurent Pinchart
@ 2023-02-15 22:29 ` Laurent Pinchart
  2023-02-15 22:29 ` [PATCH v3 08/15] media: i2c: imx290: Use CSI timings as per datasheet Laurent Pinchart
                   ` (9 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2023-02-15 22:29 UTC (permalink / raw)
  To: linux-media
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Alexander Stein

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

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>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v2:

- Fix rebase conflict
---
 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 c0b37afd2fbe..b83918ac4cc3 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;
 }
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 08/15] media: i2c: imx290: Use CSI timings as per datasheet
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
                   ` (6 preceding siblings ...)
  2023-02-15 22:29 ` [PATCH v3 07/15] media: i2c: imx290: Support 60fps in 2 lane operation Laurent Pinchart
@ 2023-02-15 22:29 ` Laurent Pinchart
  2023-02-15 22:29 ` [PATCH v3 09/15] media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write Laurent Pinchart
                   ` (8 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2023-02-15 22:29 UTC (permalink / raw)
  To: linux-media
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Alexander Stein

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

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>
Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v2:

- Fix typo in comment
---
 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 b83918ac4cc3..26194c2b04c4 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.75MHz 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);
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 09/15] media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
                   ` (7 preceding siblings ...)
  2023-02-15 22:29 ` [PATCH v3 08/15] media: i2c: imx290: Use CSI timings as per datasheet Laurent Pinchart
@ 2023-02-15 22:29 ` Laurent Pinchart
  2023-02-15 22:29 ` [PATCH v3 10/15] media: i2c: imx290: Convert V4L2_CID_VBLANK " Laurent Pinchart
                   ` (7 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2023-02-15 22:29 UTC (permalink / raw)
  To: linux-media
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Alexander Stein

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

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>
Signed-off-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 26194c2b04c4..f82fb05b6695 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) {
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 10/15] media: i2c: imx290: Convert V4L2_CID_VBLANK to read/write
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
                   ` (8 preceding siblings ...)
  2023-02-15 22:29 ` [PATCH v3 09/15] media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write Laurent Pinchart
@ 2023-02-15 22:29 ` Laurent Pinchart
  2023-02-17  8:26   ` Alexander Stein
  2023-02-15 22:29 ` [PATCH v3 11/15] media: i2c: imx290: VMAX is mode dependent Laurent Pinchart
                   ` (6 subsequent siblings)
  16 siblings, 1 reply; 20+ messages in thread
From: Laurent Pinchart @ 2023-02-15 22:29 UTC (permalink / raw)
  To: linux-media
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Alexander Stein

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

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>
Signed-off-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 f82fb05b6695..1ae01cd43efb 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;
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 11/15] media: i2c: imx290: VMAX is mode dependent
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
                   ` (9 preceding siblings ...)
  2023-02-15 22:29 ` [PATCH v3 10/15] media: i2c: imx290: Convert V4L2_CID_VBLANK " Laurent Pinchart
@ 2023-02-15 22:29 ` Laurent Pinchart
  2023-02-15 22:30 ` [PATCH v3 12/15] media: i2c: imx290: Remove duplicated write to IMX290_CTRL_07 Laurent Pinchart
                   ` (5 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2023-02-15 22:29 UTC (permalink / raw)
  To: linux-media
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Alexander Stein

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

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>
Signed-off-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 1ae01cd43efb..36d6fe12336d 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 related	[flat|nested] 20+ messages in thread

* [PATCH v3 12/15] media: i2c: imx290: Remove duplicated write to IMX290_CTRL_07
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
                   ` (10 preceding siblings ...)
  2023-02-15 22:29 ` [PATCH v3 11/15] media: i2c: imx290: VMAX is mode dependent Laurent Pinchart
@ 2023-02-15 22:30 ` Laurent Pinchart
  2023-02-15 22:30 ` [PATCH v3 13/15] media: i2c: imx290: Add support for 74.25MHz external clock Laurent Pinchart
                   ` (4 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2023-02-15 22:30 UTC (permalink / raw)
  To: linux-media
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Alexander Stein

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

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>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 36d6fe12336d..2d41b5403933 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 },
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v3 13/15] media: i2c: imx290: Add support for 74.25MHz external clock
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
                   ` (11 preceding siblings ...)
  2023-02-15 22:30 ` [PATCH v3 12/15] media: i2c: imx290: Remove duplicated write to IMX290_CTRL_07 Laurent Pinchart
@ 2023-02-15 22:30 ` Laurent Pinchart
  2023-02-15 22:30 ` [PATCH v3 14/15] media: i2c: imx290: Add support for H & V Flips Laurent Pinchart
                   ` (3 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2023-02-15 22:30 UTC (permalink / raw)
  To: linux-media
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Alexander Stein

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-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 2d41b5403933..14a6ec4e398d 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 related	[flat|nested] 20+ messages in thread

* [PATCH v3 14/15] media: i2c: imx290: Add support for H & V Flips
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
                   ` (12 preceding siblings ...)
  2023-02-15 22:30 ` [PATCH v3 13/15] media: i2c: imx290: Add support for 74.25MHz external clock Laurent Pinchart
@ 2023-02-15 22:30 ` Laurent Pinchart
  2023-02-15 22:30 ` [PATCH v3 15/15] media: i2c: imx290: Add the error code to logs in start_streaming Laurent Pinchart
                   ` (2 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2023-02-15 22:30 UTC (permalink / raw)
  To: linux-media
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Alexander Stein

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

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>
Signed-off-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 14a6ec4e398d..ec380bb7533f 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 related	[flat|nested] 20+ messages in thread

* [PATCH v3 15/15] media: i2c: imx290: Add the error code to logs in start_streaming
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
                   ` (13 preceding siblings ...)
  2023-02-15 22:30 ` [PATCH v3 14/15] media: i2c: imx290: Add support for H & V Flips Laurent Pinchart
@ 2023-02-15 22:30 ` Laurent Pinchart
  2023-02-17  9:08 ` [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Alexander Stein
  2023-03-09  8:16 ` Alexander Stein
  16 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2023-02-15 22:30 UTC (permalink / raw)
  To: linux-media
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Alexander Stein

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-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 ec380bb7533f..1911fa9bc2b3 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 related	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 10/15] media: i2c: imx290: Convert V4L2_CID_VBLANK to read/write
  2023-02-15 22:29 ` [PATCH v3 10/15] media: i2c: imx290: Convert V4L2_CID_VBLANK " Laurent Pinchart
@ 2023-02-17  8:26   ` Alexander Stein
  0 siblings, 0 replies; 20+ messages in thread
From: Alexander Stein @ 2023-02-17  8:26 UTC (permalink / raw)
  To: linux-media, Laurent Pinchart
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus

Hi Laurent,

thanks for putting these series together.

Am Mittwoch, 15. Februar 2023, 23:29:58 CET schrieb Laurent Pinchart:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> 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>
> Signed-off-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 f82fb05b6695..1ae01cd43efb 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) */

I guess the ')' is a leftover. Despite that:
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Best regards,
Alexander

> +#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;


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
                   ` (14 preceding siblings ...)
  2023-02-15 22:30 ` [PATCH v3 15/15] media: i2c: imx290: Add the error code to logs in start_streaming Laurent Pinchart
@ 2023-02-17  9:08 ` Alexander Stein
  2023-03-09  8:16 ` Alexander Stein
  16 siblings, 0 replies; 20+ messages in thread
From: Alexander Stein @ 2023-02-17  9:08 UTC (permalink / raw)
  To: linux-media, Laurent Pinchart
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus, Rob Herring,
	Krzysztof Kozlowski, devicetree

Hi Laurent,

thanks for the wrap-up.
I could test this series on my TQMa8MPxL (i.MX8MP) using ISP and libcamera as 
well. I think as well that we're good to go.

Best regards,
Alexander

Am Mittwoch, 15. Februar 2023, 23:29:48 CET schrieb Laurent Pinchart:
> Hello,
> 
> This patch series combines the "[PATCH v2 0/2] Add support for mono
> version of Sony IMX290 sensor" ([1]) and "[PATCH v2 00/13] imx290: Minor
> fixes, support for alternate INCK, and more ctrls" ([2]) previously
> submitted by Dave into a single series.
> 
> As promised in my review of v2 of both series, I have tested the changes
> with my IMX327 camera sensor, connected to the i.MX8MP ISP, with
> libcamera. I haven't noticed any regression (but, full disclaimer, I
> haven't tested all the newly features). I think we're thus good to go.
> 
> This version handles all review comments from v2, resulting in the
> following changes:
> 
> - Deprecate the sony,imx290 compatible
> - Update the DT example to use the new sony,imx290lqr compatible
> - Drop unneeded pointer cast
> - Don't move imx290_of_match table
> - Fix typos
> 
> The code has also been rebased on top of the latest media master branch,
> with rebase conflicts and rebase-induced compilation breakages fixed.
> 
> The patches are available from
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git
> next/media/sensors/imx290
> 
> [1]
> https://lore.kernel.org/linux-media/20230203191644.947643-1-dave.stevenson@
> raspberrypi.com [2]
> https://lore.kernel.org/linux-media/20230203191811.947697-1-dave.stevenson@
> raspberrypi.com
> 
> Dave Stevenson (15):
>   media: dt-bindings: media: i2c: Add mono version to IMX290 bindings
>   media: i2c: imx290: Add support for the mono sensor variant
>   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
> 
>  .../bindings/media/i2c/sony,imx290.yaml       |  24 +-
>  drivers/media/i2c/imx290.c                    | 537 ++++++++++++++----
>  2 files changed, 442 insertions(+), 119 deletions(-)
> 
> 
> base-commit: 83e0f265aa8d0e37cc8e15d318b64da0ec03ff41


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls
  2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
                   ` (15 preceding siblings ...)
  2023-02-17  9:08 ` [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Alexander Stein
@ 2023-03-09  8:16 ` Alexander Stein
  2023-03-12 12:56   ` Laurent Pinchart
  16 siblings, 1 reply; 20+ messages in thread
From: Alexander Stein @ 2023-03-09  8:16 UTC (permalink / raw)
  To: linux-media, Laurent Pinchart
  Cc: Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus, Rob Herring,
	Krzysztof Kozlowski, devicetree

Hi everyone,

Am Mittwoch, 15. Februar 2023, 23:29:48 CET schrieb Laurent Pinchart:
> Hello,
> 
> This patch series combines the "[PATCH v2 0/2] Add support for mono
> version of Sony IMX290 sensor" ([1]) and "[PATCH v2 00/13] imx290: Minor
> fixes, support for alternate INCK, and more ctrls" ([2]) previously
> submitted by Dave into a single series.
> 
> As promised in my review of v2 of both series, I have tested the changes
> with my IMX327 camera sensor, connected to the i.MX8MP ISP, with
> libcamera. I haven't noticed any regression (but, full disclaimer, I
> haven't tested all the newly features). I think we're thus good to go.

What is the status of this series? Will it be picked up?

Best regards,
Alexander

> 
> This version handles all review comments from v2, resulting in the
> following changes:
> 
> - Deprecate the sony,imx290 compatible
> - Update the DT example to use the new sony,imx290lqr compatible
> - Drop unneeded pointer cast
> - Don't move imx290_of_match table
> - Fix typos
> 
> The code has also been rebased on top of the latest media master branch,
> with rebase conflicts and rebase-induced compilation breakages fixed.
> 
> The patches are available from
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git
> next/media/sensors/imx290
> 
> [1]
> https://lore.kernel.org/linux-media/20230203191644.947643-1-dave.stevenson@
> raspberrypi.com [2]
> https://lore.kernel.org/linux-media/20230203191811.947697-1-dave.stevenson@
> raspberrypi.com
> 
> Dave Stevenson (15):
>   media: dt-bindings: media: i2c: Add mono version to IMX290 bindings
>   media: i2c: imx290: Add support for the mono sensor variant
>   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
> 
>  .../bindings/media/i2c/sony,imx290.yaml       |  24 +-
>  drivers/media/i2c/imx290.c                    | 537 ++++++++++++++----
>  2 files changed, 442 insertions(+), 119 deletions(-)
> 
> 
> base-commit: 83e0f265aa8d0e37cc8e15d318b64da0ec03ff41


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls
  2023-03-09  8:16 ` Alexander Stein
@ 2023-03-12 12:56   ` Laurent Pinchart
  0 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2023-03-12 12:56 UTC (permalink / raw)
  To: Alexander Stein
  Cc: linux-media, Dave Stevenson, Manivannan Sadhasivam, Sakari Ailus,
	Rob Herring, Krzysztof Kozlowski, devicetree

Hi Alexander,

On Thu, Mar 09, 2023 at 09:16:47AM +0100, Alexander Stein wrote:
> Am Mittwoch, 15. Februar 2023, 23:29:48 CET schrieb Laurent Pinchart:
> > Hello,
> > 
> > This patch series combines the "[PATCH v2 0/2] Add support for mono
> > version of Sony IMX290 sensor" ([1]) and "[PATCH v2 00/13] imx290: Minor
> > fixes, support for alternate INCK, and more ctrls" ([2]) previously
> > submitted by Dave into a single series.
> > 
> > As promised in my review of v2 of both series, I have tested the changes
> > with my IMX327 camera sensor, connected to the i.MX8MP ISP, with
> > libcamera. I haven't noticed any regression (but, full disclaimer, I
> > haven't tested all the newly features). I think we're thus good to go.
> 
> What is the status of this series? Will it be picked up?

I expect Sakari to pick it up for v6.4. I have pushed the whole series
with acks to the next/media/sensors/imx290 branch on
git://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git.

> > This version handles all review comments from v2, resulting in the
> > following changes:
> > 
> > - Deprecate the sony,imx290 compatible
> > - Update the DT example to use the new sony,imx290lqr compatible
> > - Drop unneeded pointer cast
> > - Don't move imx290_of_match table
> > - Fix typos
> > 
> > The code has also been rebased on top of the latest media master branch,
> > with rebase conflicts and rebase-induced compilation breakages fixed.
> > 
> > The patches are available from
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git
> > next/media/sensors/imx290
> > 
> > [1] https://lore.kernel.org/linux-media/20230203191644.947643-1-dave.stevenson@raspberrypi.com
> > [2] https://lore.kernel.org/linux-media/20230203191811.947697-1-dave.stevenson@raspberrypi.com
> > 
> > Dave Stevenson (15):
> >   media: dt-bindings: media: i2c: Add mono version to IMX290 bindings
> >   media: i2c: imx290: Add support for the mono sensor variant
> >   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
> > 
> >  .../bindings/media/i2c/sony,imx290.yaml       |  24 +-
> >  drivers/media/i2c/imx290.c                    | 537 ++++++++++++++----
> >  2 files changed, 442 insertions(+), 119 deletions(-)
> > 
> > 
> > base-commit: 83e0f265aa8d0e37cc8e15d318b64da0ec03ff41

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2023-03-12 12:56 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-15 22:29 [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Laurent Pinchart
2023-02-15 22:29 ` [PATCH v3 01/15] media: dt-bindings: media: i2c: Add mono version to IMX290 bindings Laurent Pinchart
2023-02-15 22:29 ` [PATCH v3 02/15] media: i2c: imx290: Add support for the mono sensor variant Laurent Pinchart
2023-02-15 22:29 ` [PATCH v3 03/15] media: i2c: imx290: Match kernel coding style on whitespace Laurent Pinchart
2023-02-15 22:29 ` [PATCH v3 04/15] media: i2c: imx290: Set the colorspace fields in the format Laurent Pinchart
2023-02-15 22:29 ` [PATCH v3 05/15] media: i2c: imx290: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks Laurent Pinchart
2023-02-15 22:29 ` [PATCH v3 06/15] media: i2c: imx290: Fix the pixel rate at 148.5Mpix/s Laurent Pinchart
2023-02-15 22:29 ` [PATCH v3 07/15] media: i2c: imx290: Support 60fps in 2 lane operation Laurent Pinchart
2023-02-15 22:29 ` [PATCH v3 08/15] media: i2c: imx290: Use CSI timings as per datasheet Laurent Pinchart
2023-02-15 22:29 ` [PATCH v3 09/15] media: i2c: imx290: Convert V4L2_CID_HBLANK to read/write Laurent Pinchart
2023-02-15 22:29 ` [PATCH v3 10/15] media: i2c: imx290: Convert V4L2_CID_VBLANK " Laurent Pinchart
2023-02-17  8:26   ` Alexander Stein
2023-02-15 22:29 ` [PATCH v3 11/15] media: i2c: imx290: VMAX is mode dependent Laurent Pinchart
2023-02-15 22:30 ` [PATCH v3 12/15] media: i2c: imx290: Remove duplicated write to IMX290_CTRL_07 Laurent Pinchart
2023-02-15 22:30 ` [PATCH v3 13/15] media: i2c: imx290: Add support for 74.25MHz external clock Laurent Pinchart
2023-02-15 22:30 ` [PATCH v3 14/15] media: i2c: imx290: Add support for H & V Flips Laurent Pinchart
2023-02-15 22:30 ` [PATCH v3 15/15] media: i2c: imx290: Add the error code to logs in start_streaming Laurent Pinchart
2023-02-17  9:08 ` [PATCH v3 00/15] media: i2c: imx290: Mono support, minor fixes, alternate INCK, and more controls Alexander Stein
2023-03-09  8:16 ` Alexander Stein
2023-03-12 12:56   ` Laurent Pinchart

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox