[PATCH v7 00/10] Add strobe duration and hw strobe signal v4l2 ctrl & use it for ov9282

[PATCH v7 00/10] Add strobe duration and hw strobe signal v4l2 ctrl & use it for ov9282

[Richard Leitner]

This series adds two new v4l2 controls:
- V4L2_CID_FLASH_DURATION: "Strobe duration": This control enables
  setting a desired flash/strobe length/duration in µs.
- V4L2_CID_FLASH_HW_STROBE_SIGNAL: "Hardware strobe signal": This
  control enables the hardware strobe output signal of a v4l2 device.

As a first user of these new controls add basic flash/strobe support
for ov9282 sensors using their "hardware strobe output". The duration
calculation is only interpolated from various measurements, as no
documentation was found.

Further flash/strobe-related controls as well as a migration to v4l2-cci
helpers for ov9282 will likely be implemented in future series.

All register addresses/values are based on the OV9281 datasheet v1.53
(january 2019). This series was tested using an ov9281 VisionComponents
camera module.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
Changes in v7:
- Improved v4l2 uAPI documentation (thanks Sakari)
- Link to v6: https://lore.kernel.org/r/20250716-ov9282-flash-strobe-v6-0-934f12aeff33@linux.dev

Changes in v6:
- Fix "Alignment should match open parenthesis" by Media-CI bot in v4l2-flash-led-class.c
- Fix "format string contains non-ascii character (µ)" by Media-CI bot in ov9282.c
- Introduce new V4L2_CID_FLASH_HW_STROBE_SIGNAL control (as suggested by Sakari)
- Implement V4L2_CID_FLASH_HW_STROBE_SIGNAL instead of
  V4L2_CID_FLASH_LED_MODE in ov9282.c (as suggested by Sakari)
- Drop "media: v4l2-flash: fix flash_timeout comment" as this was
  applied (thanks Lee)
- Link to v5: https://lore.kernel.org/r/20250617-ov9282-flash-strobe-v5-0-9762da74d065@linux.dev

Changes in v5:
- Improve try_ctrl for flash_duration by using DIV_ROUND_UP() and abs() (thanks Sakari)
- Drop "leds: flash: Add support for flash/strobe duration" as this was applied upstream
- Add "media: i2c: ov9282: dynamic flash_duration maximum" (thanks Sakari)
- Link to v4: https://lore.kernel.org/r/20250507-ov9282-flash-strobe-v4-0-72b299c1b7c9@linux.dev

Changes in v4:
- Fix FLASH_DURATION implementation in v4l2-flash-led-class.c by adding a
  missing brace and enum entry (thanks Sakari)
- Fix format of multiline comment in ov9282.c (thanks Sakari)
- Add missing NULL check in ov9282.c (thanks Sakari)
- Adapt nr_of_controls_hint for v4l2 handler in ov9282.c (thanks Sakari)
- Add patch for implementing try_ctrl for strobe_duration (thanks Sakari)
- Link to v3: https://lore.kernel.org/r/20250429-ov9282-flash-strobe-v3-0-2105ce179952@linux.dev

Changes in v3:
- create separate patch for leds driver changes (thanks Lee)
- Link to v2: https://lore.kernel.org/r/20250314-ov9282-flash-strobe-v2-0-14d7a281342d@linux.dev

Changes in v2:
- remove not needed controls in struct ov9282 (thanks Dave)
- Fix commit message of 3/3 regarding framerate get/set (thanks Dave)
- Add V4L2_CID_FLASH_STROBE_SOURCE impementation to ov9282
- Add new V4L2_CID_FLASH_DURATION control (as suggested by Laurent)
- Use FLASH_DURATION instead of FLASH_TIMEOUT for ov9282
- Link to v1: https://lore.kernel.org/r/20250303-ov9282-flash-strobe-v1-0-0fd57a1564ba@linux.dev

---
Richard Leitner (10):
      media: v4l: ctrls: add a control for flash/strobe duration
      media: v4l2-flash: add support for flash/strobe duration
      media: v4l: ctrls: add a control for enabling hw strobe signal
      Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
      media: i2c: ov9282: add output enable register definitions
      media: i2c: ov9282: add hardware strobe signal v4l2 control
      media: i2c: ov9282: add strobe_duration v4l2 control
      media: i2c: ov9282: add strobe_source v4l2 control
      media: i2c: ov9282: implement try_ctrl for strobe_duration
      media: i2c: ov9282: dynamic flash_duration maximum

 .../userspace-api/media/v4l/ext-ctrls-flash.rst    |  29 ++++
 drivers/media/i2c/ov9282.c                         | 168 ++++++++++++++++++++-
 drivers/media/v4l2-core/v4l2-ctrls-defs.c          |   3 +
 drivers/media/v4l2-core/v4l2-flash-led-class.c     |  25 +++
 include/uapi/linux/v4l2-controls.h                 |   2 +
 5 files changed, 221 insertions(+), 6 deletions(-)
---
base-commit: d9946fe286439c2aeaa7953b8c316efe5b83d515
change-id: 20250303-ov9282-flash-strobe-ac6bd00c9de6

Best regards,
-- 
Richard Leitner <richard.leitner@linux.dev>

[PATCH v7 01/10] media: v4l: ctrls: add a control for flash/strobe duration

[Richard Leitner]

Add a control V4L2_CID_FLASH_DURATION to set the duration of a
flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
control, as the timeout defines a limit after which the flash is
"forcefully" turned off again.

On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
of the flash/strobe pulse.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
 include/uapi/linux/v4l2-controls.h        | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index 1ea52011247accc51d0261f56eab1cf13c0624a0..f9ed7273a9f3eafe01c31b638e1c8d9fcf5424af 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -1135,6 +1135,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_FLASH_FAULT:		return "Faults";
 	case V4L2_CID_FLASH_CHARGE:		return "Charge";
 	case V4L2_CID_FLASH_READY:		return "Ready to Strobe";
+	case V4L2_CID_FLASH_DURATION:		return "Strobe Duration";
 
 	/* JPEG encoder controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index f836512e9debbc65d62a9fe04069b056be42f7b2..a5b7c382d77118eb7966385c5b22d5a89bc2b272 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1186,6 +1186,7 @@ enum v4l2_flash_strobe_source {
 
 #define V4L2_CID_FLASH_CHARGE			(V4L2_CID_FLASH_CLASS_BASE + 11)
 #define V4L2_CID_FLASH_READY			(V4L2_CID_FLASH_CLASS_BASE + 12)
+#define V4L2_CID_FLASH_DURATION			(V4L2_CID_FLASH_CLASS_BASE + 13)
 
 
 /* JPEG-class control IDs */

-- 
2.47.2

[PATCH v7 02/10] media: v4l2-flash: add support for flash/strobe duration

[Richard Leitner]

Add support for the new V4L2_CID_FLASH_DURATION control to the v4l2
flash led class.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/v4l2-core/v4l2-flash-led-class.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
index 355595a0fefac72c2f6941a30fa430d37dbdccfe..875d56d7190592c1e5ab7acd617b76dcec8792da 100644
--- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
+++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
@@ -29,6 +29,7 @@ enum ctrl_init_data_id {
 	INDICATOR_INTENSITY,
 	FLASH_TIMEOUT,
 	STROBE_SOURCE,
+	FLASH_DURATION,
 	/*
 	 * Only above values are applicable to
 	 * the 'ctrls' array in the struct v4l2_flash.
@@ -298,6 +299,12 @@ static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
 		 * microamperes for flash intensity units.
 		 */
 		return led_set_flash_brightness(fled_cdev, c->val);
+	case V4L2_CID_FLASH_DURATION:
+		/*
+		 * No conversion is needed as LED Flash class also uses
+		 * microseconds for flash duration units.
+		 */
+		return led_set_flash_duration(fled_cdev, c->val);
 	}
 
 	return -EINVAL;
@@ -424,6 +431,14 @@ static void __fill_ctrl_init_data(struct v4l2_flash *v4l2_flash,
 		ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE |
 				  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
 	}
+
+	/* Init FLASH_DURATION ctrl data */
+	if (has_flash_op(fled_cdev, duration_set)) {
+		ctrl_init_data[FLASH_DURATION].cid = V4L2_CID_FLASH_DURATION;
+		ctrl_cfg = &ctrl_init_data[FLASH_DURATION].config;
+		__lfs_to_v4l2_ctrl_config(&fled_cdev->duration, ctrl_cfg);
+		ctrl_cfg->id = V4L2_CID_FLASH_DURATION;
+	}
 }
 
 static int v4l2_flash_init_controls(struct v4l2_flash *v4l2_flash,
@@ -543,6 +558,16 @@ static int __sync_device_with_v4l2_controls(struct v4l2_flash *v4l2_flash)
 			return ret;
 	}
 
+	if (ctrls[FLASH_DURATION]) {
+		if (WARN_ON_ONCE(!fled_cdev))
+			return -EINVAL;
+
+		ret = led_set_flash_duration(fled_cdev,
+					     ctrls[FLASH_DURATION]->val);
+		if (ret < 0)
+			return ret;
+	}
+
 	/*
 	 * For some hardware arrangements setting strobe source may affect
 	 * torch mode. Synchronize strobe source setting only if not in torch

-- 
2.47.2

[PATCH v7 03/10] media: v4l: ctrls: add a control for enabling hw strobe signal

[Richard Leitner]

Add a control V4L2_CID_FLASH_HW_STROBE_SIGNAL to en- or disable the
strobe output of v4l2 devices (most likely sensors).

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/v4l2-core/v4l2-ctrls-defs.c | 2 ++
 include/uapi/linux/v4l2-controls.h        | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index f9ed7273a9f3eafe01c31b638e1c8d9fcf5424af..e13214ca362b9bdd2302118963008b04fcad8d4c 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -1136,6 +1136,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_FLASH_CHARGE:		return "Charge";
 	case V4L2_CID_FLASH_READY:		return "Ready to Strobe";
 	case V4L2_CID_FLASH_DURATION:		return "Strobe Duration";
+	case V4L2_CID_FLASH_HW_STROBE_SIGNAL:	return "Hardware Strobe Signal";
 
 	/* JPEG encoder controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1282,6 +1283,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_FLASH_STROBE_STATUS:
 	case V4L2_CID_FLASH_CHARGE:
 	case V4L2_CID_FLASH_READY:
+	case V4L2_CID_FLASH_HW_STROBE_SIGNAL:
 	case V4L2_CID_MPEG_VIDEO_DECODER_MPEG4_DEBLOCK_FILTER:
 	case V4L2_CID_MPEG_VIDEO_DECODER_SLICE_INTERFACE:
 	case V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE:
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index a5b7c382d77118eb7966385c5b22d5a89bc2b272..2a09815939a6d9abbe2babb3429e2227970275e1 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1187,6 +1187,7 @@ enum v4l2_flash_strobe_source {
 #define V4L2_CID_FLASH_CHARGE			(V4L2_CID_FLASH_CLASS_BASE + 11)
 #define V4L2_CID_FLASH_READY			(V4L2_CID_FLASH_CLASS_BASE + 12)
 #define V4L2_CID_FLASH_DURATION			(V4L2_CID_FLASH_CLASS_BASE + 13)
+#define V4L2_CID_FLASH_HW_STROBE_SIGNAL		(V4L2_CID_FLASH_CLASS_BASE + 14)
 
 
 /* JPEG-class control IDs */

-- 
2.47.2

[PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}

[Richard Leitner]

Add the new strobe duration and hardware strobe signal control to v4l
uAPI documentation. Additionally add labels for cross-referencing v4l
controls.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 .../userspace-api/media/v4l/ext-ctrls-flash.rst    | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
index d22c5efb806a183a3ad67ec3e6550b002a51659a..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
@@ -57,6 +57,8 @@ Flash Control IDs
 ``V4L2_CID_FLASH_CLASS (class)``
     The FLASH class descriptor.
 
+.. _v4l2-cid-flash-led-mode:
+
 ``V4L2_CID_FLASH_LED_MODE (menu)``
     Defines the mode of the flash LED, the high-power white LED attached
     to the flash controller. Setting this control may not be possible in
@@ -80,6 +82,8 @@ Flash Control IDs
 
 
 
+.. _v4l2-cid-flash-strobe-source:
+
 ``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
     Defines the source of the flash LED strobe.
 
@@ -186,3 +190,28 @@ Flash Control IDs
     charged before strobing. LED flashes often require a cooldown period
     after strobe during which another strobe will not be possible. This
     is a read-only control.
+
+.. _v4l2-cid-flash-duration:
+
+``V4L2_CID_FLASH_DURATION (integer)``
+    Duration of the flash strobe pulse generated by the strobe source,
+    typically a camera sensor. This method of controlling flash LED strobe
+    duration has three prerequisites: the strobe source's
+    :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
+    enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
+    must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
+    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
+    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
+    if possible.
+
+.. _v4l2-cid-flash-hw-strobe-signal:
+
+``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``
+    Enables the output of a hardware strobe signal from the strobe source,
+    typically a camera sensor. To control a flash LED driver connected to this
+    hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
+    must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
+    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
+    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
+    supports it, the length of the strobe signal can be configured by
+    adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.

-- 
2.47.2

[PATCH v7 05/10] media: i2c: ov9282: add output enable register definitions

[Richard Leitner]

Add #define's for the output enable registers (0x3004, 0x3005, 0x3006),
also known as SC_CTRL_04, SC_CTRL_05, SC_CTRL_04. Use those register
definitions instead of the raw values in the `common_regs` struct.

All values are based on the OV9281 datasheet v1.53 (january 2019).

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/i2c/ov9282.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index c882a021cf18852237bf9b9524d3de0c5b48cbcb..f42e0d439753e74d14e3a3592029e48f49234927 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -37,6 +37,29 @@
 #define OV9282_REG_ID		0x300a
 #define OV9282_ID		0x9281
 
+/* Output enable registers */
+#define OV9282_REG_OUTPUT_ENABLE4	0x3004
+#define OV9282_OUTPUT_ENABLE4_GPIO2	BIT(1)
+#define OV9282_OUTPUT_ENABLE4_D9	BIT(0)
+
+#define OV9282_REG_OUTPUT_ENABLE5	0x3005
+#define OV9282_OUTPUT_ENABLE5_D8	BIT(7)
+#define OV9282_OUTPUT_ENABLE5_D7	BIT(6)
+#define OV9282_OUTPUT_ENABLE5_D6	BIT(5)
+#define OV9282_OUTPUT_ENABLE5_D5	BIT(4)
+#define OV9282_OUTPUT_ENABLE5_D4	BIT(3)
+#define OV9282_OUTPUT_ENABLE5_D3	BIT(2)
+#define OV9282_OUTPUT_ENABLE5_D2	BIT(1)
+#define OV9282_OUTPUT_ENABLE5_D1	BIT(0)
+
+#define OV9282_REG_OUTPUT_ENABLE6	0x3006
+#define OV9282_OUTPUT_ENABLE6_D0	BIT(7)
+#define OV9282_OUTPUT_ENABLE6_PCLK	BIT(6)
+#define OV9282_OUTPUT_ENABLE6_HREF	BIT(5)
+#define OV9282_OUTPUT_ENABLE6_STROBE	BIT(3)
+#define OV9282_OUTPUT_ENABLE6_ILPWM	BIT(2)
+#define OV9282_OUTPUT_ENABLE6_VSYNC	BIT(1)
+
 /* Exposure control */
 #define OV9282_REG_EXPOSURE	0x3500
 #define OV9282_EXPOSURE_MIN	1
@@ -213,9 +236,9 @@ static const struct ov9282_reg common_regs[] = {
 	{0x0302, 0x32},
 	{0x030e, 0x02},
 	{0x3001, 0x00},
-	{0x3004, 0x00},
-	{0x3005, 0x00},
-	{0x3006, 0x04},
+	{OV9282_REG_OUTPUT_ENABLE4, 0x00},
+	{OV9282_REG_OUTPUT_ENABLE5, 0x00},
+	{OV9282_REG_OUTPUT_ENABLE6, OV9282_OUTPUT_ENABLE6_ILPWM},
 	{0x3011, 0x0a},
 	{0x3013, 0x18},
 	{0x301c, 0xf0},

-- 
2.47.2

[PATCH v7 06/10] media: i2c: ov9282: add hardware strobe signal v4l2 control

[Richard Leitner]

Add V4L2_CID_FLASH_HW_STROBE_SIGNAL enable/disable support using the
"strobe output enable" feature of the sensor.

All values are based on the OV9281 datasheet v1.53 (january 2019) and
tested using an ov9281 VisionComponents module.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/i2c/ov9282.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index f42e0d439753e74d14e3a3592029e48f49234927..ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -670,6 +670,23 @@ static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value)
 				current_val);
 }
 
+static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool enable)
+{
+	u32 current_val;
+	int ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
+				  &current_val);
+	if (ret)
+		return ret;
+
+	if (enable)
+		current_val |= OV9282_OUTPUT_ENABLE6_STROBE;
+	else
+		current_val &= ~OV9282_OUTPUT_ENABLE6_STROBE;
+
+	return ov9282_write_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
+				current_val);
+}
+
 /**
  * ov9282_set_ctrl() - Set subdevice control
  * @ctrl: pointer to v4l2_ctrl structure
@@ -736,6 +753,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 		ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2,
 				       (ctrl->val + ov9282->cur_mode->width) >> 1);
 		break;
+	case V4L2_CID_FLASH_HW_STROBE_SIGNAL:
+		ret = ov9282_set_ctrl_flash_hw_strobe_signal(ov9282, ctrl->val);
+		break;
 	default:
 		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
 		ret = -EINVAL;
@@ -1326,7 +1346,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	u32 lpfr;
 	int ret;
 
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
 	if (ret)
 		return ret;
 
@@ -1391,6 +1411,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 						OV9282_TIMING_HTS_MAX - mode->width,
 						1, hblank_min);
 
+	/* Flash/Strobe controls */
+	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
+
 	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
 	if (!ret) {
 		/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */

-- 
2.47.2

[PATCH v7 07/10] media: i2c: ov9282: add strobe_duration v4l2 control

[Richard Leitner]

Add V4L2_CID_FLASH_DURATION support using the "strobe_frame_span"
feature of the sensor. This is implemented by transforming the given µs
value by an interpolated formula to a "span step width" value and
writing it to register PWM_CTRL_25, PWM_CTRL_26, PWM_CTRL_27,
PWM_CTRL_28 (0x3925, 0x3926, 0x3927, 0x3928).

The maximum control value is set to the period of the current default
framerate.

All register values are based on the OV9281 datasheet v1.53 (jan 2019)
and tested using an ov9281 VisionComponents module.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/i2c/ov9282.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb..c405e3411daf37cf98d5af3535702f8321394af5 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -97,6 +97,10 @@
 #define OV9282_REG_MIPI_CTRL00	0x4800
 #define OV9282_GATED_CLOCK	BIT(5)
 
+/* Flash/Strobe control registers */
+#define OV9282_REG_FLASH_DURATION	0x3925
+#define OV9282_FLASH_DURATION_DEFAULT	0x0000001a
+
 /* Input clock rate */
 #define OV9282_INCLK_RATE	24000000
 
@@ -687,6 +691,25 @@ static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool en
 				current_val);
 }
 
+static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value)
+{
+	/*
+	 * Calculate "strobe_frame_span" increments from a given value (µs).
+	 * This is quite tricky as "The step width of shift and span is
+	 * programmable under system clock domain.", but it's not documented
+	 * how to program this step width (at least in the datasheet available
+	 * to the author at time of writing).
+	 * The formula below is interpolated from different modes/framerates
+	 * and should work quite well for most settings.
+	 */
+	u32 val = value * 192 / (ov9282->cur_mode->width + ov9282->hblank_ctrl->val);
+
+	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION, 1, (val >> 24) & 0xff);
+	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 1, 1, (val >> 16) & 0xff);
+	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 2, 1, (val >> 8) & 0xff);
+	return ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 3, 1, val & 0xff);
+}
+
 /**
  * ov9282_set_ctrl() - Set subdevice control
  * @ctrl: pointer to v4l2_ctrl structure
@@ -756,6 +779,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_FLASH_HW_STROBE_SIGNAL:
 		ret = ov9282_set_ctrl_flash_hw_strobe_signal(ov9282, ctrl->val);
 		break;
+	case V4L2_CID_FLASH_DURATION:
+		ret = ov9282_set_ctrl_flash_duration(ov9282, ctrl->val);
+		break;
 	default:
 		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
 		ret = -EINVAL;
@@ -1346,7 +1372,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	u32 lpfr;
 	int ret;
 
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12);
 	if (ret)
 		return ret;
 
@@ -1414,6 +1440,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	/* Flash/Strobe controls */
 	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
 
+	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
+			  0, 13900, 1, 8);
+
 	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
 	if (!ret) {
 		/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */

-- 
2.47.2

[PATCH v7 08/10] media: i2c: ov9282: add strobe_source v4l2 control

[Richard Leitner]

Add read-only V4L2_CID_FLASH_STROBE_SOURCE control. Its value is fixed
to V4L2_FLASH_STROBE_SOURCE_EXTERNAL as the camera sensor triggers the
strobe based on its register settings.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/i2c/ov9282.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index c405e3411daf37cf98d5af3535702f8321394af5..9efc82a1929a76c6fb245e7dbfb5276a133d1c5d 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -1368,11 +1368,12 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	struct v4l2_ctrl_handler *ctrl_hdlr = &ov9282->ctrl_handler;
 	const struct ov9282_mode *mode = ov9282->cur_mode;
 	struct v4l2_fwnode_device_properties props;
+	struct v4l2_ctrl *ctrl;
 	u32 hblank_min;
 	u32 lpfr;
 	int ret;
 
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12);
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 13);
 	if (ret)
 		return ret;
 
@@ -1443,6 +1444,14 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
 			  0, 13900, 1, 8);
 
+	ctrl = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
+				      V4L2_CID_FLASH_STROBE_SOURCE,
+				      V4L2_FLASH_STROBE_SOURCE_EXTERNAL,
+				      ~(1 << V4L2_FLASH_STROBE_SOURCE_EXTERNAL),
+				      V4L2_FLASH_STROBE_SOURCE_EXTERNAL);
+	if (ctrl)
+		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
 	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
 	if (!ret) {
 		/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */

-- 
2.47.2

[PATCH v7 09/10] media: i2c: ov9282: implement try_ctrl for strobe_duration

[Richard Leitner]

As the granularity of the hardware supported values is lower than the
control value, implement a try_ctrl() function for
V4L2_CID_FLASH_DURATION. This function calculates the nearest possible
µs strobe duration for the given value and returns it back to the
caller.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/i2c/ov9282.c | 54 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 9efc82a1929a76c6fb245e7dbfb5276a133d1c5d..b104ae77f00e9e7777342e48b7bf3caa6d297f69 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -128,6 +128,8 @@
 #define OV9282_REG_MIN		0x00
 #define OV9282_REG_MAX		0xfffff
 
+#define OV9282_STROBE_SPAN_FACTOR	192
+
 static const char * const ov9282_supply_names[] = {
 	"avdd",		/* Analog power */
 	"dovdd",	/* Digital I/O power */
@@ -691,7 +693,7 @@ static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool en
 				current_val);
 }
 
-static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value)
+static u32 ov9282_us_to_flash_duration(struct ov9282 *ov9282, u32 value)
 {
 	/*
 	 * Calculate "strobe_frame_span" increments from a given value (µs).
@@ -702,7 +704,27 @@ static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value)
 	 * The formula below is interpolated from different modes/framerates
 	 * and should work quite well for most settings.
 	 */
-	u32 val = value * 192 / (ov9282->cur_mode->width + ov9282->hblank_ctrl->val);
+	u32 frame_width = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
+
+	return value * OV9282_STROBE_SPAN_FACTOR / frame_width;
+}
+
+static u32 ov9282_flash_duration_to_us(struct ov9282 *ov9282, u32 value)
+{
+	/*
+	 * As the calculation in ov9282_us_to_flash_duration uses an integer
+	 * divison calculate in ns here to get more precision. Then check if
+	 * we need to compensate that divison by incrementing the µs result.
+	 */
+	u32 frame_width = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
+	u64 ns = value * 1000 * frame_width / OV9282_STROBE_SPAN_FACTOR;
+
+	return DIV_ROUND_UP(ns, 1000);
+}
+
+static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value)
+{
+	u32 val = ov9282_us_to_flash_duration(ov9282, value);
 
 	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION, 1, (val >> 24) & 0xff);
 	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 1, 1, (val >> 16) & 0xff);
@@ -792,9 +814,37 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 	return ret;
 }
 
+static int ov9282_try_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct ov9282 *ov9282 =
+		container_of(ctrl->handler, struct ov9282, ctrl_handler);
+
+	if (ctrl->id == V4L2_CID_FLASH_DURATION) {
+		u32 fd = ov9282_us_to_flash_duration(ov9282, ctrl->val);
+		u32 us = ctrl->val;
+
+		/* get nearest strobe_duration value */
+		u32 us0 = ov9282_flash_duration_to_us(ov9282, fd);
+		u32 us1 = ov9282_flash_duration_to_us(ov9282, (fd + 1));
+
+		if (abs(us1 - us) < abs(us - us0))
+			ctrl->val = us1;
+		else
+			ctrl->val = us0;
+
+		if (us != ctrl->val) {
+			dev_dbg(ov9282->dev, "using next valid strobe_duration %u instead of %u\n",
+				ctrl->val, us);
+		}
+	}
+
+	return 0;
+}
+
 /* V4l2 subdevice control ops*/
 static const struct v4l2_ctrl_ops ov9282_ctrl_ops = {
 	.s_ctrl = ov9282_set_ctrl,
+	.try_ctrl = ov9282_try_ctrl,
 };
 
 /**

-- 
2.47.2

[PATCH v7 10/10] media: i2c: ov9282: dynamic flash_duration maximum

[Richard Leitner]

This patch sets the current exposure time as maximum for the
flash_duration control. As Flash/Strobes which are longer than the
exposure time have no effect.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/i2c/ov9282.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index b104ae77f00e9e7777342e48b7bf3caa6d297f69..3253d9f271cb3caef6d85837ebec4f5beb466a4d 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -198,6 +198,7 @@ struct ov9282_mode {
  * @exp_ctrl: Pointer to exposure control
  * @again_ctrl: Pointer to analog gain control
  * @pixel_rate: Pointer to pixel rate control
+ * @flash_duration: Pointer to flash duration control
  * @vblank: Vertical blanking in lines
  * @noncontinuous_clock: Selection of CSI2 noncontinuous clock mode
  * @cur_mode: Pointer to current selected sensor mode
@@ -220,6 +221,7 @@ struct ov9282 {
 		struct v4l2_ctrl *again_ctrl;
 	};
 	struct v4l2_ctrl *pixel_rate;
+	struct v4l2_ctrl *flash_duration;
 	u32 vblank;
 	bool noncontinuous_clock;
 	const struct ov9282_mode *cur_mode;
@@ -611,6 +613,15 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
 					mode->vblank_max, 1, mode->vblank);
 }
 
+static u32 ov9282_exposure_to_us(struct ov9282 *ov9282, u32 exposure)
+{
+	/* calculate exposure time in µs */
+	u32 frame_width = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
+	u32 trow_us = (frame_width * 1000000UL) / ov9282->pixel_rate->val;
+
+	return exposure * trow_us;
+}
+
 /**
  * ov9282_update_exp_gain() - Set updated exposure and gain
  * @ov9282: pointer to ov9282 device
@@ -622,9 +633,10 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
 static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
 {
 	int ret;
+	u32 exposure_us = ov9282_exposure_to_us(ov9282, exposure);
 
-	dev_dbg(ov9282->dev, "Set exp %u, analog gain %u",
-		exposure, gain);
+	dev_dbg(ov9282->dev, "Set exp %u (~%u us), analog gain %u",
+		exposure, exposure_us, gain);
 
 	ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1);
 	if (ret)
@@ -635,6 +647,12 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
 		goto error_release_group_hold;
 
 	ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain);
+	if (ret)
+		goto error_release_group_hold;
+
+	ret = __v4l2_ctrl_modify_range(ov9282->flash_duration,
+				       0, exposure_us, 1,
+				       OV9282_FLASH_DURATION_DEFAULT);
 
 error_release_group_hold:
 	ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0);
@@ -1420,6 +1438,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	struct v4l2_fwnode_device_properties props;
 	struct v4l2_ctrl *ctrl;
 	u32 hblank_min;
+	u32 exposure_us;
 	u32 lpfr;
 	int ret;
 
@@ -1491,8 +1510,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	/* Flash/Strobe controls */
 	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
 
-	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
-			  0, 13900, 1, 8);
+	exposure_us = ov9282_exposure_to_us(ov9282, OV9282_EXPOSURE_DEFAULT);
+	ov9282->flash_duration = v4l2_ctrl_new_std(ctrl_hdlr,
+						   &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
+						   0, exposure_us,
+						   1, OV9282_FLASH_DURATION_DEFAULT);
 
 	ctrl = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
 				      V4L2_CID_FLASH_STROBE_SOURCE,

-- 
2.47.2

Re: [PATCH v7 07/10] media: i2c: ov9282: add strobe_duration v4l2 control

[Sakari Ailus]

Hi Richard,

On Mon, Sep 01, 2025 at 05:05:12PM +0200, Richard Leitner wrote:
> Add V4L2_CID_FLASH_DURATION support using the "strobe_frame_span"
> feature of the sensor. This is implemented by transforming the given µs
> value by an interpolated formula to a "span step width" value and
> writing it to register PWM_CTRL_25, PWM_CTRL_26, PWM_CTRL_27,
> PWM_CTRL_28 (0x3925, 0x3926, 0x3927, 0x3928).
> 
> The maximum control value is set to the period of the current default
> framerate.
> 
> All register values are based on the OV9281 datasheet v1.53 (jan 2019)
> and tested using an ov9281 VisionComponents module.
> 
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
>  drivers/media/i2c/ov9282.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb..c405e3411daf37cf98d5af3535702f8321394af5 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -97,6 +97,10 @@
>  #define OV9282_REG_MIPI_CTRL00	0x4800
>  #define OV9282_GATED_CLOCK	BIT(5)
>  
> +/* Flash/Strobe control registers */
> +#define OV9282_REG_FLASH_DURATION	0x3925
> +#define OV9282_FLASH_DURATION_DEFAULT	0x0000001a
> +
>  /* Input clock rate */
>  #define OV9282_INCLK_RATE	24000000
>  
> @@ -687,6 +691,25 @@ static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool en
>  				current_val);
>  }
>  
> +static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value)
> +{
> +	/*
> +	 * Calculate "strobe_frame_span" increments from a given value (µs).
> +	 * This is quite tricky as "The step width of shift and span is
> +	 * programmable under system clock domain.", but it's not documented
> +	 * how to program this step width (at least in the datasheet available
> +	 * to the author at time of writing).
> +	 * The formula below is interpolated from different modes/framerates
> +	 * and should work quite well for most settings.
> +	 */
> +	u32 val = value * 192 / (ov9282->cur_mode->width + ov9282->hblank_ctrl->val);
> +
> +	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION, 1, (val >> 24) & 0xff);
> +	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 1, 1, (val >> 16) & 0xff);
> +	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 2, 1, (val >> 8) & 0xff);
> +	return ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 3, 1, val & 0xff);

The bitwise and operation is redundant.

Could you do this in a single write?

Also error handling is (largely) missing.

> +}
> +
>  /**
>   * ov9282_set_ctrl() - Set subdevice control
>   * @ctrl: pointer to v4l2_ctrl structure
> @@ -756,6 +779,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>  	case V4L2_CID_FLASH_HW_STROBE_SIGNAL:
>  		ret = ov9282_set_ctrl_flash_hw_strobe_signal(ov9282, ctrl->val);
>  		break;
> +	case V4L2_CID_FLASH_DURATION:
> +		ret = ov9282_set_ctrl_flash_duration(ov9282, ctrl->val);
> +		break;
>  	default:
>  		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
>  		ret = -EINVAL;
> @@ -1346,7 +1372,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  	u32 lpfr;
>  	int ret;
>  
> -	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12);
>  	if (ret)
>  		return ret;
>  
> @@ -1414,6 +1440,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  	/* Flash/Strobe controls */
>  	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
>  
> +	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> +			  0, 13900, 1, 8);
> +
>  	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
>  	if (!ret) {
>  		/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v7 06/10] media: i2c: ov9282: add hardware strobe signal v4l2 control

[Sakari Ailus]

Hi Richard,

On Mon, Sep 01, 2025 at 05:05:11PM +0200, Richard Leitner wrote:
> Add V4L2_CID_FLASH_HW_STROBE_SIGNAL enable/disable support using the
> "strobe output enable" feature of the sensor.
> 
> All values are based on the OV9281 datasheet v1.53 (january 2019) and
> tested using an ov9281 VisionComponents module.
> 
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
>  drivers/media/i2c/ov9282.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index f42e0d439753e74d14e3a3592029e48f49234927..ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -670,6 +670,23 @@ static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value)
>  				current_val);
>  }
>  
> +static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool enable)
> +{
> +	u32 current_val;
> +	int ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
> +				  &current_val);
> +	if (ret)
> +		return ret;

Please don't do assignments in variable declaration if that involves error
handling.

> +
> +	if (enable)
> +		current_val |= OV9282_OUTPUT_ENABLE6_STROBE;
> +	else
> +		current_val &= ~OV9282_OUTPUT_ENABLE6_STROBE;
> +
> +	return ov9282_write_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
> +				current_val);
> +}
> +
>  /**
>   * ov9282_set_ctrl() - Set subdevice control
>   * @ctrl: pointer to v4l2_ctrl structure
> @@ -736,6 +753,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>  		ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2,
>  				       (ctrl->val + ov9282->cur_mode->width) >> 1);
>  		break;
> +	case V4L2_CID_FLASH_HW_STROBE_SIGNAL:
> +		ret = ov9282_set_ctrl_flash_hw_strobe_signal(ov9282, ctrl->val);
> +		break;
>  	default:
>  		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
>  		ret = -EINVAL;
> @@ -1326,7 +1346,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  	u32 lpfr;
>  	int ret;
>  
> -	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
> +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
>  	if (ret)
>  		return ret;
>  
> @@ -1391,6 +1411,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  						OV9282_TIMING_HTS_MAX - mode->width,
>  						1, hblank_min);
>  
> +	/* Flash/Strobe controls */
> +	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);

This seems rather long.

> +
>  	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
>  	if (!ret) {
>  		/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v7 09/10] media: i2c: ov9282: implement try_ctrl for strobe_duration

[Sakari Ailus]

Hi Richard,

On Mon, Sep 01, 2025 at 05:05:14PM +0200, Richard Leitner wrote:
> As the granularity of the hardware supported values is lower than the
> control value, implement a try_ctrl() function for
> V4L2_CID_FLASH_DURATION. This function calculates the nearest possible
> µs strobe duration for the given value and returns it back to the
> caller.
> 
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
>  drivers/media/i2c/ov9282.c | 54 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 9efc82a1929a76c6fb245e7dbfb5276a133d1c5d..b104ae77f00e9e7777342e48b7bf3caa6d297f69 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -128,6 +128,8 @@
>  #define OV9282_REG_MIN		0x00
>  #define OV9282_REG_MAX		0xfffff
>  
> +#define OV9282_STROBE_SPAN_FACTOR	192
> +
>  static const char * const ov9282_supply_names[] = {
>  	"avdd",		/* Analog power */
>  	"dovdd",	/* Digital I/O power */
> @@ -691,7 +693,7 @@ static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool en
>  				current_val);
>  }
>  
> -static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value)
> +static u32 ov9282_us_to_flash_duration(struct ov9282 *ov9282, u32 value)
>  {
>  	/*
>  	 * Calculate "strobe_frame_span" increments from a given value (µs).
> @@ -702,7 +704,27 @@ static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value)
>  	 * The formula below is interpolated from different modes/framerates
>  	 * and should work quite well for most settings.
>  	 */
> -	u32 val = value * 192 / (ov9282->cur_mode->width + ov9282->hblank_ctrl->val);
> +	u32 frame_width = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
> +
> +	return value * OV9282_STROBE_SPAN_FACTOR / frame_width;
> +}
> +
> +static u32 ov9282_flash_duration_to_us(struct ov9282 *ov9282, u32 value)
> +{
> +	/*
> +	 * As the calculation in ov9282_us_to_flash_duration uses an integer
> +	 * divison calculate in ns here to get more precision. Then check if
> +	 * we need to compensate that divison by incrementing the µs result.
> +	 */
> +	u32 frame_width = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
> +	u64 ns = value * 1000 * frame_width / OV9282_STROBE_SPAN_FACTOR;
> +
> +	return DIV_ROUND_UP(ns, 1000);
> +}
> +
> +static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value)
> +{
> +	u32 val = ov9282_us_to_flash_duration(ov9282, value);
>  
>  	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION, 1, (val >> 24) & 0xff);
>  	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 1, 1, (val >> 16) & 0xff);
> @@ -792,9 +814,37 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>  	return ret;
>  }
>  
> +static int ov9282_try_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct ov9282 *ov9282 =
> +		container_of(ctrl->handler, struct ov9282, ctrl_handler);

container_of_const(), please.

> +
> +	if (ctrl->id == V4L2_CID_FLASH_DURATION) {
> +		u32 fd = ov9282_us_to_flash_duration(ov9282, ctrl->val);
> +		u32 us = ctrl->val;

You could use us as argument to ov9282_us_to_flash_duration() if you switch
the order.

> +
> +		/* get nearest strobe_duration value */
> +		u32 us0 = ov9282_flash_duration_to_us(ov9282, fd);
> +		u32 us1 = ov9282_flash_duration_to_us(ov9282, (fd + 1));

Redundant parentheses.

> +
> +		if (abs(us1 - us) < abs(us - us0))
> +			ctrl->val = us1;
> +		else
> +			ctrl->val = us0;
> +
> +		if (us != ctrl->val) {
> +			dev_dbg(ov9282->dev, "using next valid strobe_duration %u instead of %u\n",
> +				ctrl->val, us);
> +		}

Redundant braces.

> +	}
> +
> +	return 0;
> +}
> +
>  /* V4l2 subdevice control ops*/
>  static const struct v4l2_ctrl_ops ov9282_ctrl_ops = {
>  	.s_ctrl = ov9282_set_ctrl,
> +	.try_ctrl = ov9282_try_ctrl,
>  };
>  
>  /**
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v7 10/10] media: i2c: ov9282: dynamic flash_duration maximum

[Sakari Ailus]

Hi Richard,

On Mon, Sep 01, 2025 at 05:05:15PM +0200, Richard Leitner wrote:
> This patch sets the current exposure time as maximum for the
> flash_duration control. As Flash/Strobes which are longer than the
> exposure time have no effect.
> 
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
>  drivers/media/i2c/ov9282.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index b104ae77f00e9e7777342e48b7bf3caa6d297f69..3253d9f271cb3caef6d85837ebec4f5beb466a4d 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -198,6 +198,7 @@ struct ov9282_mode {
>   * @exp_ctrl: Pointer to exposure control
>   * @again_ctrl: Pointer to analog gain control
>   * @pixel_rate: Pointer to pixel rate control
> + * @flash_duration: Pointer to flash duration control
>   * @vblank: Vertical blanking in lines
>   * @noncontinuous_clock: Selection of CSI2 noncontinuous clock mode
>   * @cur_mode: Pointer to current selected sensor mode
> @@ -220,6 +221,7 @@ struct ov9282 {
>  		struct v4l2_ctrl *again_ctrl;
>  	};
>  	struct v4l2_ctrl *pixel_rate;
> +	struct v4l2_ctrl *flash_duration;
>  	u32 vblank;
>  	bool noncontinuous_clock;
>  	const struct ov9282_mode *cur_mode;
> @@ -611,6 +613,15 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
>  					mode->vblank_max, 1, mode->vblank);
>  }
>  
> +static u32 ov9282_exposure_to_us(struct ov9282 *ov9282, u32 exposure)
> +{
> +	/* calculate exposure time in µs */
> +	u32 frame_width = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
> +	u32 trow_us = (frame_width * 1000000UL) / ov9282->pixel_rate->val;

Redundant parentheses.

> +
> +	return exposure * trow_us;
> +}
> +
>  /**
>   * ov9282_update_exp_gain() - Set updated exposure and gain
>   * @ov9282: pointer to ov9282 device
> @@ -622,9 +633,10 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
>  static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
>  {
>  	int ret;
> +	u32 exposure_us = ov9282_exposure_to_us(ov9282, exposure);
>  
> -	dev_dbg(ov9282->dev, "Set exp %u, analog gain %u",
> -		exposure, gain);
> +	dev_dbg(ov9282->dev, "Set exp %u (~%u us), analog gain %u",
> +		exposure, exposure_us, gain);
>  
>  	ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1);
>  	if (ret)
> @@ -635,6 +647,12 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
>  		goto error_release_group_hold;
>  
>  	ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain);
> +	if (ret)
> +		goto error_release_group_hold;
> +
> +	ret = __v4l2_ctrl_modify_range(ov9282->flash_duration,
> +				       0, exposure_us, 1,
> +				       OV9282_FLASH_DURATION_DEFAULT);
>  
>  error_release_group_hold:
>  	ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0);
> @@ -1420,6 +1438,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  	struct v4l2_fwnode_device_properties props;
>  	struct v4l2_ctrl *ctrl;
>  	u32 hblank_min;
> +	u32 exposure_us;
>  	u32 lpfr;
>  	int ret;
>  
> @@ -1491,8 +1510,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
>  	/* Flash/Strobe controls */
>  	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
>  
> -	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> -			  0, 13900, 1, 8);
> +	exposure_us = ov9282_exposure_to_us(ov9282, OV9282_EXPOSURE_DEFAULT);
> +	ov9282->flash_duration = v4l2_ctrl_new_std(ctrl_hdlr,
> +						   &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> +						   0, exposure_us,
> +						   1, OV9282_FLASH_DURATION_DEFAULT);

Wrap this differently, please, e.g. after '='.

>  
>  	ctrl = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
>  				      V4L2_CID_FLASH_STROBE_SOURCE,
> 

To me the set looks good but I wouldn't mind about having a bit more
review.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v7 07/10] media: i2c: ov9282: add strobe_duration v4l2 control

[Richard Leitner]

Hi Sakari,

thanks again for the review :)

On Mon, Sep 01, 2025 at 11:55:37PM +0300, Sakari Ailus wrote:
> Hi Richard,
> 
> On Mon, Sep 01, 2025 at 05:05:12PM +0200, Richard Leitner wrote:
> > Add V4L2_CID_FLASH_DURATION support using the "strobe_frame_span"
> > feature of the sensor. This is implemented by transforming the given µs
> > value by an interpolated formula to a "span step width" value and
> > writing it to register PWM_CTRL_25, PWM_CTRL_26, PWM_CTRL_27,
> > PWM_CTRL_28 (0x3925, 0x3926, 0x3927, 0x3928).
> > 
> > The maximum control value is set to the period of the current default
> > framerate.
> > 
> > All register values are based on the OV9281 datasheet v1.53 (jan 2019)
> > and tested using an ov9281 VisionComponents module.
> > 
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> >  drivers/media/i2c/ov9282.c | 31 ++++++++++++++++++++++++++++++-
> >  1 file changed, 30 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb..c405e3411daf37cf98d5af3535702f8321394af5 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -97,6 +97,10 @@
> >  #define OV9282_REG_MIPI_CTRL00	0x4800
> >  #define OV9282_GATED_CLOCK	BIT(5)
> >  
> > +/* Flash/Strobe control registers */
> > +#define OV9282_REG_FLASH_DURATION	0x3925
> > +#define OV9282_FLASH_DURATION_DEFAULT	0x0000001a
> > +
> >  /* Input clock rate */
> >  #define OV9282_INCLK_RATE	24000000
> >  
> > @@ -687,6 +691,25 @@ static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool en
> >  				current_val);
> >  }
> >  
> > +static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value)
> > +{
> > +	/*
> > +	 * Calculate "strobe_frame_span" increments from a given value (µs).
> > +	 * This is quite tricky as "The step width of shift and span is
> > +	 * programmable under system clock domain.", but it's not documented
> > +	 * how to program this step width (at least in the datasheet available
> > +	 * to the author at time of writing).
> > +	 * The formula below is interpolated from different modes/framerates
> > +	 * and should work quite well for most settings.
> > +	 */
> > +	u32 val = value * 192 / (ov9282->cur_mode->width + ov9282->hblank_ctrl->val);
> > +
> > +	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION, 1, (val >> 24) & 0xff);
> > +	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 1, 1, (val >> 16) & 0xff);
> > +	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 2, 1, (val >> 8) & 0xff);
> > +	return ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 3, 1, val & 0xff);
> 
> The bitwise and operation is redundant.

True. Thanks for the catch!

> 
> Could you do this in a single write?

I've implemented this in single byte writes due to some "special
behaviour" of the vision components ov9281 modules. On those modules
single byte interactions seem broken in some cases. Maybe Laurent knows
more about this and the current state, as he was/is in contact with VC.

See also: https://lore.kernel.org/all/918ce2ca-55ff-aff8-ea6c-0c17f566d59d@online.de/

Nonetheless, thanks for the pointer. I haven't documented this
accordingly. I will try to reproduce the issue again and either change
this to a single write or add a describing comment.

> 
> Also error handling is (largely) missing.

Good catch. Thanks.

> 
> > +}
> > +
> >  /**
> >   * ov9282_set_ctrl() - Set subdevice control
> >   * @ctrl: pointer to v4l2_ctrl structure
> > @@ -756,6 +779,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >  	case V4L2_CID_FLASH_HW_STROBE_SIGNAL:
> >  		ret = ov9282_set_ctrl_flash_hw_strobe_signal(ov9282, ctrl->val);
> >  		break;
> > +	case V4L2_CID_FLASH_DURATION:
> > +		ret = ov9282_set_ctrl_flash_duration(ov9282, ctrl->val);
> > +		break;
> >  	default:
> >  		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> >  		ret = -EINVAL;
> > @@ -1346,7 +1372,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >  	u32 lpfr;
> >  	int ret;
> >  
> > -	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> > +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 12);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -1414,6 +1440,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >  	/* Flash/Strobe controls */
> >  	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> >  
> > +	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > +			  0, 13900, 1, 8);
> > +
> >  	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> >  	if (!ret) {
> >  		/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
> > 
> 
> -- 
> Regards,
> 
> Sakari Ailus

regards;rl

Re: [PATCH v7 06/10] media: i2c: ov9282: add hardware strobe signal v4l2 control

[Richard Leitner]

Hi Sakari,

On Mon, Sep 01, 2025 at 11:57:15PM +0300, Sakari Ailus wrote:
> Hi Richard,
> 
> On Mon, Sep 01, 2025 at 05:05:11PM +0200, Richard Leitner wrote:
> > Add V4L2_CID_FLASH_HW_STROBE_SIGNAL enable/disable support using the
> > "strobe output enable" feature of the sensor.
> > 
> > All values are based on the OV9281 datasheet v1.53 (january 2019) and
> > tested using an ov9281 VisionComponents module.
> > 
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> >  drivers/media/i2c/ov9282.c | 25 ++++++++++++++++++++++++-
> >  1 file changed, 24 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index f42e0d439753e74d14e3a3592029e48f49234927..ff0f69f0dc3a2d0518806b9ea65c1b520b5c55fb 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -670,6 +670,23 @@ static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value)
> >  				current_val);
> >  }
> >  
> > +static int ov9282_set_ctrl_flash_hw_strobe_signal(struct ov9282 *ov9282, bool enable)
> > +{
> > +	u32 current_val;
> > +	int ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
> > +				  &current_val);
> > +	if (ret)
> > +		return ret;
> 
> Please don't do assignments in variable declaration if that involves error
> handling.

Sure. Will fix that!

> 
> > +
> > +	if (enable)
> > +		current_val |= OV9282_OUTPUT_ENABLE6_STROBE;
> > +	else
> > +		current_val &= ~OV9282_OUTPUT_ENABLE6_STROBE;
> > +
> > +	return ov9282_write_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
> > +				current_val);
> > +}
> > +
> >  /**
> >   * ov9282_set_ctrl() - Set subdevice control
> >   * @ctrl: pointer to v4l2_ctrl structure
> > @@ -736,6 +753,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >  		ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2,
> >  				       (ctrl->val + ov9282->cur_mode->width) >> 1);
> >  		break;
> > +	case V4L2_CID_FLASH_HW_STROBE_SIGNAL:
> > +		ret = ov9282_set_ctrl_flash_hw_strobe_signal(ov9282, ctrl->val);
> > +		break;
> >  	default:
> >  		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> >  		ret = -EINVAL;
> > @@ -1326,7 +1346,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >  	u32 lpfr;
> >  	int ret;
> >  
> > -	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
> > +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -1391,6 +1411,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >  						OV9282_TIMING_HTS_MAX - mode->width,
> >  						1, hblank_min);
> >  
> > +	/* Flash/Strobe controls */
> > +	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> 
> This seems rather long.

It's exactly 100 chars wide, so from a policy point of view it should be
fine ;-). But I'm also fine with breaking it to 80 if you prefer?

> 
> > +
> >  	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> >  	if (!ret) {
> >  		/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
> > 
> 
> -- 
> Regards,
> 
> Sakari Ailus

thanks!

regards;rl

Re: [PATCH v7 10/10] media: i2c: ov9282: dynamic flash_duration maximum

[Richard Leitner]

Hi Sakari,

On Tue, Sep 02, 2025 at 12:16:51AM +0300, Sakari Ailus wrote:
> Hi Richard,
> 
> On Mon, Sep 01, 2025 at 05:05:15PM +0200, Richard Leitner wrote:
> > This patch sets the current exposure time as maximum for the
> > flash_duration control. As Flash/Strobes which are longer than the
> > exposure time have no effect.
> > 
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> >  drivers/media/i2c/ov9282.c | 30 ++++++++++++++++++++++++++----
> >  1 file changed, 26 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index b104ae77f00e9e7777342e48b7bf3caa6d297f69..3253d9f271cb3caef6d85837ebec4f5beb466a4d 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -198,6 +198,7 @@ struct ov9282_mode {
> >   * @exp_ctrl: Pointer to exposure control
> >   * @again_ctrl: Pointer to analog gain control
> >   * @pixel_rate: Pointer to pixel rate control
> > + * @flash_duration: Pointer to flash duration control
> >   * @vblank: Vertical blanking in lines
> >   * @noncontinuous_clock: Selection of CSI2 noncontinuous clock mode
> >   * @cur_mode: Pointer to current selected sensor mode
> > @@ -220,6 +221,7 @@ struct ov9282 {
> >  		struct v4l2_ctrl *again_ctrl;
> >  	};
> >  	struct v4l2_ctrl *pixel_rate;
> > +	struct v4l2_ctrl *flash_duration;
> >  	u32 vblank;
> >  	bool noncontinuous_clock;
> >  	const struct ov9282_mode *cur_mode;
> > @@ -611,6 +613,15 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
> >  					mode->vblank_max, 1, mode->vblank);
> >  }
> >  
> > +static u32 ov9282_exposure_to_us(struct ov9282 *ov9282, u32 exposure)
> > +{
> > +	/* calculate exposure time in µs */
> > +	u32 frame_width = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
> > +	u32 trow_us = (frame_width * 1000000UL) / ov9282->pixel_rate->val;
> 
> Redundant parentheses.

True. Will fix this. Thanks for the catch.

> 
> > +
> > +	return exposure * trow_us;
> > +}
> > +
> >  /**
> >   * ov9282_update_exp_gain() - Set updated exposure and gain
> >   * @ov9282: pointer to ov9282 device
> > @@ -622,9 +633,10 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
> >  static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
> >  {
> >  	int ret;
> > +	u32 exposure_us = ov9282_exposure_to_us(ov9282, exposure);
> >  
> > -	dev_dbg(ov9282->dev, "Set exp %u, analog gain %u",
> > -		exposure, gain);
> > +	dev_dbg(ov9282->dev, "Set exp %u (~%u us), analog gain %u",
> > +		exposure, exposure_us, gain);
> >  
> >  	ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1);
> >  	if (ret)
> > @@ -635,6 +647,12 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
> >  		goto error_release_group_hold;
> >  
> >  	ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain);
> > +	if (ret)
> > +		goto error_release_group_hold;
> > +
> > +	ret = __v4l2_ctrl_modify_range(ov9282->flash_duration,
> > +				       0, exposure_us, 1,
> > +				       OV9282_FLASH_DURATION_DEFAULT);
> >  
> >  error_release_group_hold:
> >  	ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0);
> > @@ -1420,6 +1438,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >  	struct v4l2_fwnode_device_properties props;
> >  	struct v4l2_ctrl *ctrl;
> >  	u32 hblank_min;
> > +	u32 exposure_us;
> >  	u32 lpfr;
> >  	int ret;
> >  
> > @@ -1491,8 +1510,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >  	/* Flash/Strobe controls */
> >  	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> >  
> > -	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > -			  0, 13900, 1, 8);
> > +	exposure_us = ov9282_exposure_to_us(ov9282, OV9282_EXPOSURE_DEFAULT);
> > +	ov9282->flash_duration = v4l2_ctrl_new_std(ctrl_hdlr,
> > +						   &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > +						   0, exposure_us,
> > +						   1, OV9282_FLASH_DURATION_DEFAULT);
> 
> Wrap this differently, please, e.g. after '='.

This is wrapped the same way as all other v4l2_ctrl_new_X() calls in
ov9282_init_controls(). Therefore I've chosen to do it this way here
too.

So if I'm going to change this one, IMHO all others should be changed
too (exp_ctrl, again_ctrl, vblank_ctrl, pixel_rate, link_freq_ctrl,
hblank_ctrl). Is this intended?

If so I'm wondering if this would be a suiteable approach?

ov9282->flash_duration =
	v4l2_ctrl_new_std(ctrl_hdlr,
			   &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
			   0, exposure_us,
			   1, OV9282_FLASH_DURATION_DEFAULT);

It is fine for checkpatch, but introduces a newline for every ctrl and
tbh I'm not sure if it improves readability?

> 
> >  
> >  	ctrl = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
> >  				      V4L2_CID_FLASH_STROBE_SOURCE,
> > 
> 
> To me the set looks good but I wouldn't mind about having a bit more
> review.

Thanks for your continuous feedback! It improved the series a lot!

Is there anyhthing I can assists/help?

> 
> -- 
> Kind regards,
> 
> Sakari Ailus

regards;rl

Re: [PATCH v7 10/10] media: i2c: ov9282: dynamic flash_duration maximum

[Sakari Ailus]

Hi Richard,

On Wed, Sep 03, 2025 at 09:13:35AM +0200, Richard Leitner wrote:
> > > @@ -1491,8 +1510,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > >  	/* Flash/Strobe controls */
> > >  	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> > >  
> > > -	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > -			  0, 13900, 1, 8);
> > > +	exposure_us = ov9282_exposure_to_us(ov9282, OV9282_EXPOSURE_DEFAULT);
> > > +	ov9282->flash_duration = v4l2_ctrl_new_std(ctrl_hdlr,
> > > +						   &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > +						   0, exposure_us,
> > > +						   1, OV9282_FLASH_DURATION_DEFAULT);
> > 
> > Wrap this differently, please, e.g. after '='.
> 
> This is wrapped the same way as all other v4l2_ctrl_new_X() calls in
> ov9282_init_controls(). Therefore I've chosen to do it this way here
> too.
> 
> So if I'm going to change this one, IMHO all others should be changed
> too (exp_ctrl, again_ctrl, vblank_ctrl, pixel_rate, link_freq_ctrl,
> hblank_ctrl). Is this intended?
> 
> If so I'm wondering if this would be a suiteable approach?
> 
> ov9282->flash_duration =
> 	v4l2_ctrl_new_std(ctrl_hdlr,
> 			   &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> 			   0, exposure_us,
> 			   1, OV9282_FLASH_DURATION_DEFAULT);
> 
> It is fine for checkpatch, but introduces a newline for every ctrl and
> tbh I'm not sure if it improves readability?

I don't think it's worse at least. You can also rewrap the rest of the
lines:

	ov9282->flash_duration =
		v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
				  V4L2_CID_FLASH_DURATION, 0, exposure_us, 1,
				  OV9282_FLASH_DURATION_DEFAULT);

> > >  	ctrl = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
> > >  				      V4L2_CID_FLASH_STROBE_SOURCE,
> > > 
> > 
> > To me the set looks good but I wouldn't mind about having a bit more
> > review.
> 
> Thanks for your continuous feedback! It improved the series a lot!
> 
> Is there anyhthing I can assists/help?

I asked Laurent if he could check this out, it'd be nice to get these to
6.18.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v7 10/10] media: i2c: ov9282: dynamic flash_duration maximum

[Richard Leitner]

Hi Sakari,

On Wed, Sep 03, 2025 at 10:48:48AM +0300, Sakari Ailus wrote:
> Hi Richard,
> 
> On Wed, Sep 03, 2025 at 09:13:35AM +0200, Richard Leitner wrote:
> > > > @@ -1491,8 +1510,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > >  	/* Flash/Strobe controls */
> > > >  	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> > > >  
> > > > -	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > > -			  0, 13900, 1, 8);
> > > > +	exposure_us = ov9282_exposure_to_us(ov9282, OV9282_EXPOSURE_DEFAULT);
> > > > +	ov9282->flash_duration = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > +						   &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > > +						   0, exposure_us,
> > > > +						   1, OV9282_FLASH_DURATION_DEFAULT);
> > > 
> > > Wrap this differently, please, e.g. after '='.
> > 
> > This is wrapped the same way as all other v4l2_ctrl_new_X() calls in
> > ov9282_init_controls(). Therefore I've chosen to do it this way here
> > too.
> > 
> > So if I'm going to change this one, IMHO all others should be changed
> > too (exp_ctrl, again_ctrl, vblank_ctrl, pixel_rate, link_freq_ctrl,
> > hblank_ctrl). Is this intended?
> > 
> > If so I'm wondering if this would be a suiteable approach?
> > 
> > ov9282->flash_duration =
> > 	v4l2_ctrl_new_std(ctrl_hdlr,
> > 			   &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > 			   0, exposure_us,
> > 			   1, OV9282_FLASH_DURATION_DEFAULT);
> > 
> > It is fine for checkpatch, but introduces a newline for every ctrl and
> > tbh I'm not sure if it improves readability?
> 
> I don't think it's worse at least. You can also rewrap the rest of the
> lines:
> 
> 	ov9282->flash_duration =
> 		v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
> 				  V4L2_CID_FLASH_DURATION, 0, exposure_us, 1,
> 				  OV9282_FLASH_DURATION_DEFAULT);
> 

Ok. Fine with me ;)

So I will adapt this patch and add a new patch which changes the wrapping
for all remaining v4l2_ctrl_new_X() calls in ov9282_init_controls() to the
series and send a v8? Or should I wait for feedback from Laurent for v8?

> > > >  	ctrl = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
> > > >  				      V4L2_CID_FLASH_STROBE_SOURCE,
> > > > 
> > > 
> > > To me the set looks good but I wouldn't mind about having a bit more
> > > review.
> > 
> > Thanks for your continuous feedback! It improved the series a lot!
> > 
> > Is there anyhthing I can assists/help?
> 
> I asked Laurent if he could check this out, it'd be nice to get these to
> 6.18.

Nice. Thanks! Yeah, that would be nice, indeed :)

> 
> -- 
> Kind regards,
> 
> Sakari Ailus

regards;rl

Re: [PATCH v7 10/10] media: i2c: ov9282: dynamic flash_duration maximum

[Sakari Ailus]

Hi Richard,

On Wed, Sep 03, 2025 at 10:24:46AM +0200, Richard Leitner wrote:
> Hi Sakari,
> 
> On Wed, Sep 03, 2025 at 10:48:48AM +0300, Sakari Ailus wrote:
> > Hi Richard,
> > 
> > On Wed, Sep 03, 2025 at 09:13:35AM +0200, Richard Leitner wrote:
> > > > > @@ -1491,8 +1510,11 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> > > > >  	/* Flash/Strobe controls */
> > > > >  	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_HW_STROBE_SIGNAL, 0, 1, 1, 0);
> > > > >  
> > > > > -	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > > > -			  0, 13900, 1, 8);
> > > > > +	exposure_us = ov9282_exposure_to_us(ov9282, OV9282_EXPOSURE_DEFAULT);
> > > > > +	ov9282->flash_duration = v4l2_ctrl_new_std(ctrl_hdlr,
> > > > > +						   &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > > > +						   0, exposure_us,
> > > > > +						   1, OV9282_FLASH_DURATION_DEFAULT);
> > > > 
> > > > Wrap this differently, please, e.g. after '='.
> > > 
> > > This is wrapped the same way as all other v4l2_ctrl_new_X() calls in
> > > ov9282_init_controls(). Therefore I've chosen to do it this way here
> > > too.
> > > 
> > > So if I'm going to change this one, IMHO all others should be changed
> > > too (exp_ctrl, again_ctrl, vblank_ctrl, pixel_rate, link_freq_ctrl,
> > > hblank_ctrl). Is this intended?
> > > 
> > > If so I'm wondering if this would be a suiteable approach?
> > > 
> > > ov9282->flash_duration =
> > > 	v4l2_ctrl_new_std(ctrl_hdlr,
> > > 			   &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
> > > 			   0, exposure_us,
> > > 			   1, OV9282_FLASH_DURATION_DEFAULT);
> > > 
> > > It is fine for checkpatch, but introduces a newline for every ctrl and
> > > tbh I'm not sure if it improves readability?
> > 
> > I don't think it's worse at least. You can also rewrap the rest of the
> > lines:
> > 
> > 	ov9282->flash_duration =
> > 		v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops,
> > 				  V4L2_CID_FLASH_DURATION, 0, exposure_us, 1,
> > 				  OV9282_FLASH_DURATION_DEFAULT);
> > 
> 
> Ok. Fine with me ;)
> 
> So I will adapt this patch and add a new patch which changes the wrapping
> for all remaining v4l2_ctrl_new_X() calls in ov9282_init_controls() to the
> series and send a v8? Or should I wait for feedback from Laurent for v8?

Let's wait for Laurent to review this first. The changes I asked for are
minor.

-- 
Sakari Ailus