[PATCH 00/10] media: i2c: ov9282: fix control range handling

[PATCH 00/10] media: i2c: ov9282: fix control range handling

[Richard Leitner]

This series fixes a set of related issues in the OV9282 control handling
code.

The driver updated exposure- and flash duration-related control ranges
in several places, but some of those updates used an incorrect timebase
or were triggered too late, which could leave the controls clamped
against stale values.

The series:
- checks the return value of exposure range updates,
- introduces various helpers for calculating/converting times,
- refreshes flash-duration limits while the sensor is powered down,
- refreshes dependent ranges on mode changes,
- recomputes flash-duration limits when HBLANK changes,
- removes the no longer needed hblank field of the driver struct,
- harmonizes error printing during probe

This series was tested with an ov9282 sensor on a vision components camera
module attached to an i.MX8MP mainboard.

Any feedback is warmly welcome.

Thanks!

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
Richard Leitner (10):
      media: i2c: ov9282: handle error from exposure range update
      media: i2c: ov9282: fix line time and exposure time calculation
      media: i2c: ov9282: fix flash duration to/from microseconds conversion
      media: i2c: ov9282: update flash_duration range even when powered down
      media: i2c: ov9282: add refresh of missing ranges on a mode change
      media: i2c: ov9282: refresh flash_duration range on an HBLANK write
      media: i2c: ov9282: drop redundant vblank field
      media: i2c: ov9282: harmonize dev_err_probe usage
      media: i2c: ov9282: fix flash duration control range
      media: i2c: ov9282: clamp flash_duration default to its maximum

 drivers/media/i2c/ov9282.c | 253 +++++++++++++++++++++++++++++++--------------
 1 file changed, 174 insertions(+), 79 deletions(-)
---
base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
change-id: 20260909-ov9282-fixes-da986dabc238

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

[PATCH 01/10] media: i2c: ov9282: handle error from exposure range update

[Richard Leitner]

When setting V4L2_CID_VBLANK the V4L2 range for the exposure control is
updated v4l2_ctrl_modify_range(). This range update missed a return value
check, which resulted in ov9282_set_ctrl() not returning on an error from
that function call.

Fix this by checking the return value and returning it on error.

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

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 5d301660a87d8..c10b2e205834e 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -608,6 +608,8 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 					       ov9282->cur_mode->height -
 					       OV9282_EXPOSURE_OFFSET,
 					       1, OV9282_EXPOSURE_DEFAULT);
+		if (ret)
+			return ret;
 		break;
 	}
 

-- 
2.53.0

[PATCH 02/10] media: i2c: ov9282: fix line time and exposure time calculation

[Richard Leitner]

ov9282_exposure_to_us() divided the line length by the pixel rate control,
which is the MIPI rate and not the clock HTS is counted in. The right
clock is PLL2's system clock. With the PLL2 dividers left at their reset
values the chain

SYS_CLK = XVCLK  / pre_div0 / pre_div * loop_div / sys_pre_div / sys_div
        =    24  /    1     /    3    * loop_div /     4       /    2

collapses to SYS_CLK = loop_div MHz.

Fix this by introducing a new static function to calculate the current
line time and use it in ov9282_exposure_to_us().

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

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index c10b2e205834e..3f83a6cf338d8 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -10,10 +10,12 @@
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/math.h>
+#include <linux/math64.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
+#include <linux/time64.h>
 
 #include <media/v4l2-cci.h>
 #include <media/v4l2-ctrls.h>
@@ -472,6 +474,41 @@ static inline struct ov9282 *to_ov9282(struct v4l2_subdev *subdev)
 	return container_of(subdev, struct ov9282, sd);
 }
 
+/**
+ * ov9282_line_time_ns() - Calculate duration of one sensor line.
+ * @ov9282: pointer to ov9282 device
+ *
+ * The line time and therefore OV9282_REG_TIMING_HTS and the strobe frame span
+ * are counted in PLL2's system clock. We assume the PLL2 dividers are at their
+ * reset values, so the formula reduces to SYS_CLK = loop_div MHz.
+ *
+ * Return: line time in nanoseconds.
+ */
+static u32 ov9282_line_time_ns(struct ov9282 *ov9282)
+{
+	u32 hts = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
+	u32 sclk_rate_mhz = ov9282->code == MEDIA_BUS_FMT_Y10_1X10 ?
+		OV9282_PLL_CTRL_0D_RAW10 : OV9282_PLL_CTRL_0D_RAW8;
+
+	/*
+	 * OV9282_REG_TIMING_HTS counts 2-pixel units
+	 */
+	return DIV_ROUND_CLOSEST(hts * (u32)NSEC_PER_USEC, 2 * sclk_rate_mhz);
+}
+
+/**
+ * ov9282_exposure_to_us() - Convert an exposure register value to microseconds
+ * @ov9282: pointer to ov9282 device
+ * @exposure: exposure register value to convert
+ *
+ * Return: microsecond represenation of the given exposure register value.
+ */
+static u32 ov9282_exposure_to_us(struct ov9282 *ov9282, u32 exposure)
+{
+	return div_u64((u64)exposure * ov9282_line_time_ns(ov9282),
+		       NSEC_PER_USEC);
+}
+
 /**
  * ov9282_update_controls() - Update control ranges based on streaming mode
  * @ov9282: pointer to ov9282 device
@@ -510,15 +547,6 @@ 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 / (ov9282->pixel_rate->val / 1000000UL);
-
-	return exposure * trow_us;
-}
-
 /**
  * ov9282_update_exp_gain() - Set updated exposure and gain
  * @ov9282: pointer to ov9282 device

-- 
2.53.0

[PATCH 03/10] media: i2c: ov9282: fix flash duration to/from microseconds conversion

[Richard Leitner]

Currently the flash duration is converted to/from microseconds using a
fixed OV9282_STROBE_SPAN_FACTOR constant. This is inaccurate as it was
found that the "step width of shift and span" (which is not documented
further in the datasheet) scales with the line, so the span is counted
in lines.

Fix the conversion by dropping the constant factor and using the
previously introduced ov9282_line_time_ns() helper instead.

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

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 3f83a6cf338d8..90a0fe542ce4a 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -133,8 +133,6 @@
 #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 */
@@ -509,6 +507,42 @@ static u32 ov9282_exposure_to_us(struct ov9282 *ov9282, u32 exposure)
 		       NSEC_PER_USEC);
 }
 
+/**
+ * ov9282_us_to_flash_duration() - Convert µs to flash duration register value
+ * @ov9282: pointer to ov9282 device
+ * @value: microseconds value to convert
+ *
+ * Calculate "strobe_frame_span" increments from a given value (µs). According
+ * to the datasheet "The step width of shift and span is programmable under
+ * system clock domain.", but this is not documented further. Nonetheless the
+ * step width was found empirically to scale with the line length, so the span
+ * is counted in lines.
+ *
+ * Return: flash duration register value
+ */
+static u32 ov9282_us_to_flash_duration(struct ov9282 *ov9282, u32 value)
+{
+	return div_u64((u64)value * NSEC_PER_USEC, ov9282_line_time_ns(ov9282));
+}
+
+/**
+ * ov9282_flash_duration_to_us() - Convert flash duration register value to µs
+ * @ov9282: pointer to ov9282 device
+ * @value: flash duration register value to convert
+ *
+ * Convert a given "strobe_frame_span" increment value to microseconds. For an
+ * explanation regarding conversion factor see the documentation of
+ * ov9282_us_to_flash_duration. As the calculation there uses an integer
+ * division round up here.
+ *
+ * Return: microseconds
+ */
+static u32 ov9282_flash_duration_to_us(struct ov9282 *ov9282, u32 value)
+{
+	return DIV_ROUND_UP_ULL((u64)value * ov9282_line_time_ns(ov9282),
+				NSEC_PER_USEC);
+}
+
 /**
  * ov9282_update_controls() - Update control ranges based on streaming mode
  * @ov9282: pointer to ov9282 device
@@ -585,34 +619,6 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
 	return ret ? ret : ret_hold;
 }
 
-static u32 ov9282_us_to_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 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)
-{
-	/*
-	 * Calculate back to microseconds from "strobe_frame_span" increments.
-	 * As the calculation in ov9282_us_to_flash_duration uses an integer
-	 * divison round up here.
-	 */
-	u32 frame_width = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
-
-	return DIV_ROUND_UP(value * frame_width, OV9282_STROBE_SPAN_FACTOR);
-}
-
 static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct ov9282 *ov9282 =

-- 
2.53.0

[PATCH 04/10] media: i2c: ov9282: update flash_duration range even when powered down

[Richard Leitner]

The flash_duration range update sat inside ov9282_update_exp_gain(), which
ov9282_set_ctrl() only reaches after the pm_runtime_get_if_in_use() early
return. So with the sensor idle the ceiling kept whatever value it had
when it last streamed. Therefore setting exposure before flash_duration
while the sensor is powered down may clamp flash_duration against an
outdated exposure.

Fix this by moving the range update before the power check.

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

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 90a0fe542ce4a..4c88de1965171 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -645,6 +645,19 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 		if (ret)
 			return ret;
 		break;
+	case V4L2_CID_EXPOSURE:
+		/*
+		 * Ensure the flash duration range is also updated on powered
+		 * down sensors.
+		 */
+		ret = __v4l2_ctrl_modify_range(ov9282->flash_duration, 0,
+					       ov9282_exposure_to_us(ov9282,
+								     ctrl->val),
+					       1,
+					       OV9282_STROBE_FRAME_SPAN_DEFAULT);
+		if (ret)
+			return ret;
+		break;
 	}
 
 	/* Set controls only if sensor is in power on state */

-- 
2.53.0

[PATCH 05/10] media: i2c: ov9282: add refresh of missing ranges on a mode change

[Richard Leitner]

ov9282_update_controls() updates the pixel rate, hblank and vblank, but
currently misses the exposure range and flash_duration. Both are dependent
on the line time and therefore the pixel format.

Refresh both, and commit cur_mode and code in ov9282_set_pad_format()
before the call so the refresh and any nested s_ctrl see the incoming
format.

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

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 4c88de1965171..e64d8343c18e9 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -557,6 +557,8 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
 {
 	u32 hblank_min;
 	s64 pixel_rate;
+	u32 exposure_us;
+	u32 lpfr;
 	int ret;
 
 	ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode->link_freq_idx);
@@ -577,8 +579,22 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
 	if (ret)
 		return ret;
 
-	return __v4l2_ctrl_modify_range(ov9282->vblank_ctrl, mode->vblank_min,
-					mode->vblank_max, 1, mode->vblank);
+	ret = __v4l2_ctrl_modify_range(ov9282->vblank_ctrl, mode->vblank_min,
+				       mode->vblank_max, 1, mode->vblank);
+	if (ret)
+		return ret;
+
+	lpfr = ov9282->vblank_ctrl->val + mode->height;
+	ret = __v4l2_ctrl_modify_range(ov9282->exp_ctrl, OV9282_EXPOSURE_MIN,
+				       lpfr - OV9282_EXPOSURE_OFFSET,
+				       OV9282_EXPOSURE_STEP,
+				       OV9282_EXPOSURE_DEFAULT);
+	if (ret)
+		return ret;
+
+	exposure_us = ov9282_exposure_to_us(ov9282, ov9282->exp_ctrl->val);
+	return __v4l2_ctrl_modify_range(ov9282->flash_duration, 0, exposure_us,
+					1, OV9282_STROBE_FRAME_SPAN_DEFAULT);
 }
 
 /**
@@ -855,10 +871,23 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
 		framefmt = v4l2_subdev_state_get_format(sd_state, fmt->pad);
 		*framefmt = fmt->format;
 	} else {
+		const struct ov9282_mode *old_mode = ov9282->cur_mode;
+		u32 old_code = ov9282->code;
+
+		/*
+		 * Commit before refreshing the ranges. ov9282_update_controls()
+		 * and the nested ov9282_set_ctrl() calls it triggers derive the
+		 * frame length and the line time from cur_mode and code, so
+		 * they have to describe the incoming format, not the outgoing
+		 * one.
+		 */
+		ov9282->cur_mode = mode;
+		ov9282->code = code;
+
 		ret = ov9282_update_controls(ov9282, mode, fmt);
-		if (!ret) {
-			ov9282->cur_mode = mode;
-			ov9282->code = code;
+		if (ret) {
+			ov9282->cur_mode = old_mode;
+			ov9282->code = old_code;
 		}
 	}
 

-- 
2.53.0

[PATCH 06/10] media: i2c: ov9282: refresh flash_duration range on an HBLANK write

[Richard Leitner]

The strobe ceiling is the exposure time in microseconds, so it follows
the line time, and the line length sets the line time. Writing HBLANK moves
it and nothing recomputes the flash_duration ceiling.

Therefore add the missing flash_duration range update when HBLANK is set.

The exposure range is counted in lines, so it is unaffected and does not
need refreshing here.

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

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index e64d8343c18e9..632184e76b9bd 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -674,6 +674,19 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 		if (ret)
 			return ret;
 		break;
+	case V4L2_CID_HBLANK:
+		/*
+		 * HBLANK affects the line time, which then affects the flash
+		 * duration. Therefore recalculate the flash duration range
+		 * here.
+		 */
+		exposure = ov9282_exposure_to_us(ov9282, ov9282->exp_ctrl->val);
+		ret = __v4l2_ctrl_modify_range(ov9282->flash_duration, 0,
+					       exposure, 1,
+					       OV9282_STROBE_FRAME_SPAN_DEFAULT);
+		if (ret)
+			return ret;
+		break;
 	}
 
 	/* Set controls only if sensor is in power on state */

-- 
2.53.0

[PATCH 07/10] media: i2c: ov9282: drop redundant vblank field

[Richard Leitner]

The ov9282 main struct has a dedicated vblank field which was only used
to duplicate the value of the vblank v4l2 control.

Therefore drop this struct field.

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

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 632184e76b9bd..28f8b05b4c09e 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -193,7 +193,6 @@ struct ov9282_mode {
  * @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
  * @code: Mbus code currently selected
@@ -216,7 +215,6 @@ struct ov9282 {
 	};
 	struct v4l2_ctrl *pixel_rate;
 	struct v4l2_ctrl *flash_duration;
-	u32 vblank;
 	bool noncontinuous_clock;
 	const struct ov9282_mode *cur_mode;
 	u32 code;
@@ -646,17 +644,13 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 
 	switch (ctrl->id) {
 	case V4L2_CID_VBLANK:
-		ov9282->vblank = ov9282->vblank_ctrl->val;
-
+		lpfr = ctrl->val + ov9282->cur_mode->height;
 		dev_dbg(ov9282->dev, "Received vblank %u, new lpfr %u",
-			ov9282->vblank,
-			ov9282->vblank + ov9282->cur_mode->height);
+			ctrl->val, lpfr);
 
 		ret = __v4l2_ctrl_modify_range(ov9282->exp_ctrl,
 					       OV9282_EXPOSURE_MIN,
-					       ov9282->vblank +
-					       ov9282->cur_mode->height -
-					       OV9282_EXPOSURE_OFFSET,
+					       lpfr - OV9282_EXPOSURE_OFFSET,
 					       1, OV9282_EXPOSURE_DEFAULT);
 		if (ret)
 			return ret;
@@ -704,7 +698,7 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 		ret = ov9282_update_exp_gain(ov9282, exposure, analog_gain);
 		break;
 	case V4L2_CID_VBLANK:
-		lpfr = ov9282->vblank + ov9282->cur_mode->height;
+		lpfr = ctrl->val + ov9282->cur_mode->height;
 		ret = cci_write(ov9282->regmap, OV9282_REG_LPFR, lpfr, NULL);
 		break;
 	case V4L2_CID_HFLIP:
@@ -1412,7 +1406,6 @@ static int ov9282_probe(struct i2c_client *client)
 	/* Set default mode to first mode */
 	ov9282->cur_mode = &supported_modes[DEFAULT_MODE];
 	ov9282->code = MEDIA_BUS_FMT_Y10_1X10;
-	ov9282->vblank = ov9282->cur_mode->vblank;
 
 	ret = ov9282_init_controls(ov9282);
 	if (ret) {

-- 
2.53.0

[PATCH 08/10] media: i2c: ov9282: harmonize dev_err_probe usage

[Richard Leitner]

Use dev_err_probe() for all error messages during probing. This ensures
there's a common "look-and-feel" in the drivers source code as well as
the system log.

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

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 28f8b05b4c09e..f728709fcf0a6 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -1109,26 +1109,25 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
 	ov9282->reset_gpio = devm_gpiod_get_optional(ov9282->dev, "reset",
 						     GPIOD_OUT_LOW);
 	if (IS_ERR(ov9282->reset_gpio)) {
-		dev_err(ov9282->dev, "failed to get reset gpio %pe",
-			ov9282->reset_gpio);
-		return PTR_ERR(ov9282->reset_gpio);
+		return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->reset_gpio),
+				     "failed to get reset gpio");
 	}
 
 	/* Get sensor input clock */
 	ov9282->inclk = devm_v4l2_sensor_clk_get(ov9282->dev, NULL);
 	if (IS_ERR(ov9282->inclk))
 		return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->inclk),
-				     "could not get inclk\n");
+				     "could not get inclk");
 
 	ret = ov9282_configure_regulators(ov9282);
 	if (ret)
 		return dev_err_probe(ov9282->dev, ret,
-				     "Failed to get power regulators\n");
+				     "Failed to get power regulators");
 
 	rate = clk_get_rate(ov9282->inclk);
 	if (rate != OV9282_INCLK_RATE) {
-		dev_err(ov9282->dev, "inclk frequency mismatch");
-		return -EINVAL;
+		return dev_err_probe(ov9282->dev, -EINVAL,
+				     "inclk frequency mismatch");
 	}
 
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
@@ -1144,16 +1143,15 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
 		bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
 
 	if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV9282_NUM_DATA_LANES) {
-		dev_err(ov9282->dev,
-			"number of CSI2 data lanes %d is not supported",
-			bus_cfg.bus.mipi_csi2.num_data_lanes);
-		ret = -EINVAL;
+		ret = dev_err_probe(ov9282->dev, -EINVAL,
+				    "number of CSI2 data lanes %d is not supported",
+				    bus_cfg.bus.mipi_csi2.num_data_lanes);
 		goto done_endpoint_free;
 	}
 
 	if (!bus_cfg.nr_of_link_frequencies) {
-		dev_err(ov9282->dev, "no link frequencies defined");
-		ret = -EINVAL;
+		ret = dev_err_probe(ov9282->dev, -EINVAL,
+				    "no link frequencies defined");
 		goto done_endpoint_free;
 	}
 
@@ -1382,14 +1380,14 @@ static int ov9282_probe(struct i2c_client *client)
 
 	ret = ov9282_parse_hw_config(ov9282);
 	if (ret) {
-		dev_err(ov9282->dev, "HW configuration is not supported");
-		return ret;
+		return dev_err_probe(ov9282->dev, ret,
+				     "HW configuration is not supported");
 	}
 
 	ov9282->regmap = devm_cci_regmap_init_i2c(client, 16);
 	if (IS_ERR(ov9282->regmap))
 		return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->regmap),
-				     "Failed to init CCI\n");
+				     "Failed to init CCI");
 
 	ret = ov9282_power_on(ov9282->dev);
 	if (ret)
@@ -1399,7 +1397,7 @@ static int ov9282_probe(struct i2c_client *client)
 	/* Check module identity */
 	ret = ov9282_detect(ov9282);
 	if (ret) {
-		dev_err(ov9282->dev, "failed to find sensor: %d", ret);
+		dev_err_probe(ov9282->dev, ret, "failed to find sensor");
 		goto error_power_off;
 	}
 
@@ -1409,7 +1407,7 @@ static int ov9282_probe(struct i2c_client *client)
 
 	ret = ov9282_init_controls(ov9282);
 	if (ret) {
-		dev_err(ov9282->dev, "failed to init controls: %d", ret);
+		dev_err_probe(ov9282->dev, ret, "failed to init controls");
 		goto error_power_off;
 	}
 
@@ -1422,14 +1420,14 @@ static int ov9282_probe(struct i2c_client *client)
 	ov9282->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ret = media_entity_pads_init(&ov9282->sd.entity, 1, &ov9282->pad);
 	if (ret) {
-		dev_err(ov9282->dev, "failed to init entity pads: %d", ret);
+		dev_err_probe(ov9282->dev, ret, "failed to init entity pads");
 		goto error_handler_free;
 	}
 
 	ov9282->sd.state_lock = ov9282->ctrl_handler.lock;
 	ret = v4l2_subdev_init_finalize(&ov9282->sd);
 	if (ret < 0) {
-		dev_err_probe(ov9282->dev, ret, "failed to init subdev\n");
+		dev_err_probe(ov9282->dev, ret, "failed to init subdev");
 		goto error_media_entity;
 	}
 

-- 
2.53.0

[PATCH 09/10] media: i2c: ov9282: fix flash duration control range

[Richard Leitner]

When updating the flash_duration range ensure the ceiling is at least as
long as the exposure time is. This may cause the calculated
flash_duration register value to be rounded up.

This is done by introducing a new
ov9282_update_ctrl_range_flash_duration() function and using them
wherever possible.

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

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index f728709fcf0a6..be38ecfad8c82 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -541,6 +541,29 @@ static u32 ov9282_flash_duration_to_us(struct ov9282 *ov9282, u32 value)
 				NSEC_PER_USEC);
 }
 
+/**
+ * ov9282_update_ctrl_range_flash_duration() - Update flash_duration control range
+ * @ov9282: pointer to ov9282 device
+ *
+ * This may round up the ceiling to the microseconds representation of the
+ * next flash_duration register value to make sure one can illuminate the whole
+ * exposure time long.
+ *
+ * Return: 0 if successful, error code otherwise.
+ */
+static int ov9282_update_ctrl_range_flash_duration(struct ov9282 *ov9282)
+{
+	u32 exposure_us = ov9282_exposure_to_us(ov9282, ov9282->exp_ctrl->val);
+	u32 fd_max = ov9282_us_to_flash_duration(ov9282, exposure_us);
+	u32 fd_max_us = ov9282_flash_duration_to_us(ov9282, fd_max);
+
+	if (fd_max_us < exposure_us)
+		fd_max_us = ov9282_flash_duration_to_us(ov9282, fd_max + 1);
+
+	return __v4l2_ctrl_modify_range(ov9282->flash_duration, 0, fd_max_us,
+					1, OV9282_STROBE_FRAME_SPAN_DEFAULT);
+}
+
 /**
  * ov9282_update_controls() - Update control ranges based on streaming mode
  * @ov9282: pointer to ov9282 device
@@ -555,7 +578,6 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
 {
 	u32 hblank_min;
 	s64 pixel_rate;
-	u32 exposure_us;
 	u32 lpfr;
 	int ret;
 
@@ -590,9 +612,7 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
 	if (ret)
 		return ret;
 
-	exposure_us = ov9282_exposure_to_us(ov9282, ov9282->exp_ctrl->val);
-	return __v4l2_ctrl_modify_range(ov9282->flash_duration, 0, exposure_us,
-					1, OV9282_STROBE_FRAME_SPAN_DEFAULT);
+	return ov9282_update_ctrl_range_flash_duration(ov9282);
 }
 
 /**
@@ -605,11 +625,9 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
  */
 static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
 {
-	u32 exposure_us = ov9282_exposure_to_us(ov9282, exposure);
 	int ret, ret_hold;
 
-	dev_dbg(ov9282->dev, "Set exp %u (~%u us), analog gain %u",
-		exposure, exposure_us, gain);
+	dev_dbg(ov9282->dev, "Set exp %u, analog gain %u", exposure, gain);
 
 	ret = cci_write(ov9282->regmap, OV9282_REG_HOLD, 0x01, NULL);
 	if (ret)
@@ -623,9 +641,7 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
 	if (ret)
 		goto error_release_group_hold;
 
-	ret = __v4l2_ctrl_modify_range(ov9282->flash_duration,
-				       0, exposure_us, 1,
-				       OV9282_STROBE_FRAME_SPAN_DEFAULT);
+	ret = ov9282_update_ctrl_range_flash_duration(ov9282);
 
 error_release_group_hold:
 	ret_hold = cci_write(ov9282->regmap, OV9282_REG_HOLD, 0, NULL);
@@ -660,11 +676,7 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 		 * Ensure the flash duration range is also updated on powered
 		 * down sensors.
 		 */
-		ret = __v4l2_ctrl_modify_range(ov9282->flash_duration, 0,
-					       ov9282_exposure_to_us(ov9282,
-								     ctrl->val),
-					       1,
-					       OV9282_STROBE_FRAME_SPAN_DEFAULT);
+		ret = ov9282_update_ctrl_range_flash_duration(ov9282);
 		if (ret)
 			return ret;
 		break;
@@ -674,10 +686,7 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 		 * duration. Therefore recalculate the flash duration range
 		 * here.
 		 */
-		exposure = ov9282_exposure_to_us(ov9282, ov9282->exp_ctrl->val);
-		ret = __v4l2_ctrl_modify_range(ov9282->flash_duration, 0,
-					       exposure, 1,
-					       OV9282_STROBE_FRAME_SPAN_DEFAULT);
+		ret = ov9282_update_ctrl_range_flash_duration(ov9282);
 		if (ret)
 			return ret;
 		break;

-- 
2.53.0

[PATCH 10/10] media: i2c: ov9282: clamp flash_duration default to its maximum

[Richard Leitner]

Currently the OV9282_STROBE_FRAME_SPAN_DEFAULT is always passed as default
to the flash_duration v4l2 control, where the maximum is dynamically
changed. This may lead to situations where the default is above the
maximum.

Fix this by setting the maximum value as default when
OV9282_STROBE_FRAME_SPAN_DEFAULT is above the maximum value.

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

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index be38ecfad8c82..1c5bed85781ce 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -11,6 +11,7 @@
 #include <linux/i2c.h>
 #include <linux/math.h>
 #include <linux/math64.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
@@ -556,12 +557,15 @@ static int ov9282_update_ctrl_range_flash_duration(struct ov9282 *ov9282)
 	u32 exposure_us = ov9282_exposure_to_us(ov9282, ov9282->exp_ctrl->val);
 	u32 fd_max = ov9282_us_to_flash_duration(ov9282, exposure_us);
 	u32 fd_max_us = ov9282_flash_duration_to_us(ov9282, fd_max);
+	u32 fd_default;
 
 	if (fd_max_us < exposure_us)
 		fd_max_us = ov9282_flash_duration_to_us(ov9282, fd_max + 1);
 
+	fd_default = min_t(u32, OV9282_STROBE_FRAME_SPAN_DEFAULT, fd_max_us);
+
 	return __v4l2_ctrl_modify_range(ov9282->flash_duration, 0, fd_max_us,
-					1, OV9282_STROBE_FRAME_SPAN_DEFAULT);
+					1, fd_default);
 }
 
 /**

-- 
2.53.0

Re: [PATCH 01/10] media: i2c: ov9282: handle error from exposure range update

[Dave Stevenson]

On Mon, 14 Sept 2026 at 20:21, Richard Leitner
<richard.leitner@linux.dev> wrote:
>
> When setting V4L2_CID_VBLANK the V4L2 range for the exposure control is
> updated v4l2_ctrl_modify_range(). This range update missed a return value
> check, which resulted in ov9282_set_ctrl() not returning on an error from
> that function call.
>
> Fix this by checking the return value and returning it on error.
>
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>

AFAICT A failure there would imply a driver bug rather than userspace,
but better to handle it rather than just ignoring it.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/media/i2c/ov9282.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 5d301660a87d8..c10b2e205834e 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -608,6 +608,8 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>                                                ov9282->cur_mode->height -
>                                                OV9282_EXPOSURE_OFFSET,
>                                                1, OV9282_EXPOSURE_DEFAULT);
> +               if (ret)
> +                       return ret;
>                 break;
>         }
>
>
> --
> 2.53.0
>
>

Re: [PATCH 07/10] media: i2c: ov9282: drop redundant vblank field

[Dave Stevenson]

On Mon, 14 Sept 2026 at 20:21, Richard Leitner
<richard.leitner@linux.dev> wrote:
>
> The ov9282 main struct has a dedicated vblank field which was only used
> to duplicate the value of the vblank v4l2 control.
>
> Therefore drop this struct field.
>
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/media/i2c/ov9282.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 632184e76b9bd..28f8b05b4c09e 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -193,7 +193,6 @@ struct ov9282_mode {
>   * @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
>   * @code: Mbus code currently selected
> @@ -216,7 +215,6 @@ struct ov9282 {
>         };
>         struct v4l2_ctrl *pixel_rate;
>         struct v4l2_ctrl *flash_duration;
> -       u32 vblank;
>         bool noncontinuous_clock;
>         const struct ov9282_mode *cur_mode;
>         u32 code;
> @@ -646,17 +644,13 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>
>         switch (ctrl->id) {
>         case V4L2_CID_VBLANK:
> -               ov9282->vblank = ov9282->vblank_ctrl->val;
> -
> +               lpfr = ctrl->val + ov9282->cur_mode->height;
>                 dev_dbg(ov9282->dev, "Received vblank %u, new lpfr %u",
> -                       ov9282->vblank,
> -                       ov9282->vblank + ov9282->cur_mode->height);
> +                       ctrl->val, lpfr);
>
>                 ret = __v4l2_ctrl_modify_range(ov9282->exp_ctrl,
>                                                OV9282_EXPOSURE_MIN,
> -                                              ov9282->vblank +
> -                                              ov9282->cur_mode->height -
> -                                              OV9282_EXPOSURE_OFFSET,
> +                                              lpfr - OV9282_EXPOSURE_OFFSET,
>                                                1, OV9282_EXPOSURE_DEFAULT);
>                 if (ret)
>                         return ret;
> @@ -704,7 +698,7 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>                 ret = ov9282_update_exp_gain(ov9282, exposure, analog_gain);
>                 break;
>         case V4L2_CID_VBLANK:
> -               lpfr = ov9282->vblank + ov9282->cur_mode->height;
> +               lpfr = ctrl->val + ov9282->cur_mode->height;
>                 ret = cci_write(ov9282->regmap, OV9282_REG_LPFR, lpfr, NULL);
>                 break;
>         case V4L2_CID_HFLIP:
> @@ -1412,7 +1406,6 @@ static int ov9282_probe(struct i2c_client *client)
>         /* Set default mode to first mode */
>         ov9282->cur_mode = &supported_modes[DEFAULT_MODE];
>         ov9282->code = MEDIA_BUS_FMT_Y10_1X10;
> -       ov9282->vblank = ov9282->cur_mode->vblank;
>
>         ret = ov9282_init_controls(ov9282);
>         if (ret) {
>
> --
> 2.53.0
>
>

Re: [PATCH 08/10] media: i2c: ov9282: harmonize dev_err_probe usage

[Dave Stevenson]

Hi Richard

On Mon, 14 Sept 2026 at 20:21, Richard Leitner
<richard.leitner@linux.dev> wrote:
>
> Use dev_err_probe() for all error messages during probing. This ensures
> there's a common "look-and-feel" in the drivers source code as well as
> the system log.
>
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
>  drivers/media/i2c/ov9282.c | 38 ++++++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 28f8b05b4c09e..f728709fcf0a6 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -1109,26 +1109,25 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
>         ov9282->reset_gpio = devm_gpiod_get_optional(ov9282->dev, "reset",
>                                                      GPIOD_OUT_LOW);
>         if (IS_ERR(ov9282->reset_gpio)) {
> -               dev_err(ov9282->dev, "failed to get reset gpio %pe",
> -                       ov9282->reset_gpio);
> -               return PTR_ERR(ov9282->reset_gpio);
> +               return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->reset_gpio),
> +                                    "failed to get reset gpio");
>         }
>
>         /* Get sensor input clock */
>         ov9282->inclk = devm_v4l2_sensor_clk_get(ov9282->dev, NULL);
>         if (IS_ERR(ov9282->inclk))
>                 return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->inclk),
> -                                    "could not get inclk\n");
> +                                    "could not get inclk");

My understanding is that dev_err_probe should always have the \n on
the end of the log text.
Admittedly "failed to get reset gpio %pe" above is missing it, but
removing it off all the other instances seems to be the wrong fix.

Otherwise the patch looks fine.

  Dave

>
>         ret = ov9282_configure_regulators(ov9282);
>         if (ret)
>                 return dev_err_probe(ov9282->dev, ret,
> -                                    "Failed to get power regulators\n");
> +                                    "Failed to get power regulators");
>
>         rate = clk_get_rate(ov9282->inclk);
>         if (rate != OV9282_INCLK_RATE) {
> -               dev_err(ov9282->dev, "inclk frequency mismatch");
> -               return -EINVAL;
> +               return dev_err_probe(ov9282->dev, -EINVAL,
> +                                    "inclk frequency mismatch");
>         }
>
>         ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> @@ -1144,16 +1143,15 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
>                 bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
>
>         if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV9282_NUM_DATA_LANES) {
> -               dev_err(ov9282->dev,
> -                       "number of CSI2 data lanes %d is not supported",
> -                       bus_cfg.bus.mipi_csi2.num_data_lanes);
> -               ret = -EINVAL;
> +               ret = dev_err_probe(ov9282->dev, -EINVAL,
> +                                   "number of CSI2 data lanes %d is not supported",
> +                                   bus_cfg.bus.mipi_csi2.num_data_lanes);
>                 goto done_endpoint_free;
>         }
>
>         if (!bus_cfg.nr_of_link_frequencies) {
> -               dev_err(ov9282->dev, "no link frequencies defined");
> -               ret = -EINVAL;
> +               ret = dev_err_probe(ov9282->dev, -EINVAL,
> +                                   "no link frequencies defined");
>                 goto done_endpoint_free;
>         }
>
> @@ -1382,14 +1380,14 @@ static int ov9282_probe(struct i2c_client *client)
>
>         ret = ov9282_parse_hw_config(ov9282);
>         if (ret) {
> -               dev_err(ov9282->dev, "HW configuration is not supported");
> -               return ret;
> +               return dev_err_probe(ov9282->dev, ret,
> +                                    "HW configuration is not supported");
>         }
>
>         ov9282->regmap = devm_cci_regmap_init_i2c(client, 16);
>         if (IS_ERR(ov9282->regmap))
>                 return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->regmap),
> -                                    "Failed to init CCI\n");
> +                                    "Failed to init CCI");
>
>         ret = ov9282_power_on(ov9282->dev);
>         if (ret)
> @@ -1399,7 +1397,7 @@ static int ov9282_probe(struct i2c_client *client)
>         /* Check module identity */
>         ret = ov9282_detect(ov9282);
>         if (ret) {
> -               dev_err(ov9282->dev, "failed to find sensor: %d", ret);
> +               dev_err_probe(ov9282->dev, ret, "failed to find sensor");
>                 goto error_power_off;
>         }
>
> @@ -1409,7 +1407,7 @@ static int ov9282_probe(struct i2c_client *client)
>
>         ret = ov9282_init_controls(ov9282);
>         if (ret) {
> -               dev_err(ov9282->dev, "failed to init controls: %d", ret);
> +               dev_err_probe(ov9282->dev, ret, "failed to init controls");
>                 goto error_power_off;
>         }
>
> @@ -1422,14 +1420,14 @@ static int ov9282_probe(struct i2c_client *client)
>         ov9282->pad.flags = MEDIA_PAD_FL_SOURCE;
>         ret = media_entity_pads_init(&ov9282->sd.entity, 1, &ov9282->pad);
>         if (ret) {
> -               dev_err(ov9282->dev, "failed to init entity pads: %d", ret);
> +               dev_err_probe(ov9282->dev, ret, "failed to init entity pads");
>                 goto error_handler_free;
>         }
>
>         ov9282->sd.state_lock = ov9282->ctrl_handler.lock;
>         ret = v4l2_subdev_init_finalize(&ov9282->sd);
>         if (ret < 0) {
> -               dev_err_probe(ov9282->dev, ret, "failed to init subdev\n");
> +               dev_err_probe(ov9282->dev, ret, "failed to init subdev");
>                 goto error_media_entity;
>         }
>
>
> --
> 2.53.0
>
>

Re: [PATCH 10/10] media: i2c: ov9282: clamp flash_duration default to its maximum

[Dave Stevenson]

"media: i2c: ov9282: clamp flash_duration default to its maximum"
took me a moment to parse as to be the control maximum, not the default maximum.
"clamp flash_duration default to valid range"?

On Mon, 14 Sept 2026 at 20:21, Richard Leitner
<richard.leitner@linux.dev> wrote:
>
> Currently the OV9282_STROBE_FRAME_SPAN_DEFAULT is always passed as default
> to the flash_duration v4l2 control, where the maximum is dynamically
> changed. This may lead to situations where the default is above the
> maximum.
>
> Fix this by setting the maximum value as default when
> OV9282_STROBE_FRAME_SPAN_DEFAULT is above the maximum value.
>
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>

With or without that changed:
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/media/i2c/ov9282.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index be38ecfad8c82..1c5bed85781ce 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -11,6 +11,7 @@
>  #include <linux/i2c.h>
>  #include <linux/math.h>
>  #include <linux/math64.h>
> +#include <linux/minmax.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
> @@ -556,12 +557,15 @@ static int ov9282_update_ctrl_range_flash_duration(struct ov9282 *ov9282)
>         u32 exposure_us = ov9282_exposure_to_us(ov9282, ov9282->exp_ctrl->val);
>         u32 fd_max = ov9282_us_to_flash_duration(ov9282, exposure_us);
>         u32 fd_max_us = ov9282_flash_duration_to_us(ov9282, fd_max);
> +       u32 fd_default;
>
>         if (fd_max_us < exposure_us)
>                 fd_max_us = ov9282_flash_duration_to_us(ov9282, fd_max + 1);
>
> +       fd_default = min_t(u32, OV9282_STROBE_FRAME_SPAN_DEFAULT, fd_max_us);
> +
>         return __v4l2_ctrl_modify_range(ov9282->flash_duration, 0, fd_max_us,
> -                                       1, OV9282_STROBE_FRAME_SPAN_DEFAULT);
> +                                       1, fd_default);
>  }
>
>  /**
>
> --
> 2.53.0
>
>

Re: [PATCH 10/10] media: i2c: ov9282: clamp flash_duration default to its maximum

[Richard Leitner]

Hi Dave,

thanks for your feedback!

On Tue, Sep 15, 2026 at 04:09:13PM +0100, Dave Stevenson wrote:
> "media: i2c: ov9282: clamp flash_duration default to its maximum"
> took me a moment to parse as to be the control maximum, not the default maximum.
> "clamp flash_duration default to valid range"?

Sure. That makes sense. I will adapt the patch title accordingly in v2.

regards;rl

> 
> On Mon, 14 Sept 2026 at 20:21, Richard Leitner
> <richard.leitner@linux.dev> wrote:
> >
> > Currently the OV9282_STROBE_FRAME_SPAN_DEFAULT is always passed as default
> > to the flash_duration v4l2 control, where the maximum is dynamically
> > changed. This may lead to situations where the default is above the
> > maximum.
> >
> > Fix this by setting the maximum value as default when
> > OV9282_STROBE_FRAME_SPAN_DEFAULT is above the maximum value.
> >
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> 
> With or without that changed:
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> > ---
> >  drivers/media/i2c/ov9282.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index be38ecfad8c82..1c5bed85781ce 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/i2c.h>
> >  #include <linux/math.h>
> >  #include <linux/math64.h>
> > +#include <linux/minmax.h>
> >  #include <linux/module.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/regmap.h>
> > @@ -556,12 +557,15 @@ static int ov9282_update_ctrl_range_flash_duration(struct ov9282 *ov9282)
> >         u32 exposure_us = ov9282_exposure_to_us(ov9282, ov9282->exp_ctrl->val);
> >         u32 fd_max = ov9282_us_to_flash_duration(ov9282, exposure_us);
> >         u32 fd_max_us = ov9282_flash_duration_to_us(ov9282, fd_max);
> > +       u32 fd_default;
> >
> >         if (fd_max_us < exposure_us)
> >                 fd_max_us = ov9282_flash_duration_to_us(ov9282, fd_max + 1);
> >
> > +       fd_default = min_t(u32, OV9282_STROBE_FRAME_SPAN_DEFAULT, fd_max_us);
> > +
> >         return __v4l2_ctrl_modify_range(ov9282->flash_duration, 0, fd_max_us,
> > -                                       1, OV9282_STROBE_FRAME_SPAN_DEFAULT);
> > +                                       1, fd_default);
> >  }
> >
> >  /**
> >
> > --
> > 2.53.0
> >
> >

Re: [PATCH 08/10] media: i2c: ov9282: harmonize dev_err_probe usage

[Richard Leitner]

Hi Dave,

thanks for the review!

On Tue, Sep 15, 2026 at 03:55:01PM +0100, Dave Stevenson wrote:
> Hi Richard
> 
> On Mon, 14 Sept 2026 at 20:21, Richard Leitner
> <richard.leitner@linux.dev> wrote:
> >
> > Use dev_err_probe() for all error messages during probing. This ensures
> > there's a common "look-and-feel" in the drivers source code as well as
> > the system log.
> >
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> >  drivers/media/i2c/ov9282.c | 38 ++++++++++++++++++--------------------
> >  1 file changed, 18 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 28f8b05b4c09e..f728709fcf0a6 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -1109,26 +1109,25 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
> >         ov9282->reset_gpio = devm_gpiod_get_optional(ov9282->dev, "reset",
> >                                                      GPIOD_OUT_LOW);
> >         if (IS_ERR(ov9282->reset_gpio)) {
> > -               dev_err(ov9282->dev, "failed to get reset gpio %pe",
> > -                       ov9282->reset_gpio);
> > -               return PTR_ERR(ov9282->reset_gpio);
> > +               return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->reset_gpio),
> > +                                    "failed to get reset gpio");
> >         }
> >
> >         /* Get sensor input clock */
> >         ov9282->inclk = devm_v4l2_sensor_clk_get(ov9282->dev, NULL);
> >         if (IS_ERR(ov9282->inclk))
> >                 return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->inclk),
> > -                                    "could not get inclk\n");
> > +                                    "could not get inclk");
> 
> My understanding is that dev_err_probe should always have the \n on
> the end of the log text.
> Admittedly "failed to get reset gpio %pe" above is missing it, but
> removing it off all the other instances seems to be the wrong fix.

Thanks for the pointer. I wasn't aware of this convention. Also the
documentation of dev_err_probe mentions nothing. Nonetheless when
grepping through the kernel source I totally agree. "\n" is definitely
preferred.

I will adapt this for v2.

regards;rl

> 
> Otherwise the patch looks fine.
> 
>   Dave
> 
> >
> >         ret = ov9282_configure_regulators(ov9282);
> >         if (ret)
> >                 return dev_err_probe(ov9282->dev, ret,
> > -                                    "Failed to get power regulators\n");
> > +                                    "Failed to get power regulators");
> >
> >         rate = clk_get_rate(ov9282->inclk);
> >         if (rate != OV9282_INCLK_RATE) {
> > -               dev_err(ov9282->dev, "inclk frequency mismatch");
> > -               return -EINVAL;
> > +               return dev_err_probe(ov9282->dev, -EINVAL,
> > +                                    "inclk frequency mismatch");
> >         }
> >
> >         ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > @@ -1144,16 +1143,15 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
> >                 bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
> >
> >         if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV9282_NUM_DATA_LANES) {
> > -               dev_err(ov9282->dev,
> > -                       "number of CSI2 data lanes %d is not supported",
> > -                       bus_cfg.bus.mipi_csi2.num_data_lanes);
> > -               ret = -EINVAL;
> > +               ret = dev_err_probe(ov9282->dev, -EINVAL,
> > +                                   "number of CSI2 data lanes %d is not supported",
> > +                                   bus_cfg.bus.mipi_csi2.num_data_lanes);
> >                 goto done_endpoint_free;
> >         }
> >
> >         if (!bus_cfg.nr_of_link_frequencies) {
> > -               dev_err(ov9282->dev, "no link frequencies defined");
> > -               ret = -EINVAL;
> > +               ret = dev_err_probe(ov9282->dev, -EINVAL,
> > +                                   "no link frequencies defined");
> >                 goto done_endpoint_free;
> >         }
> >
> > @@ -1382,14 +1380,14 @@ static int ov9282_probe(struct i2c_client *client)
> >
> >         ret = ov9282_parse_hw_config(ov9282);
> >         if (ret) {
> > -               dev_err(ov9282->dev, "HW configuration is not supported");
> > -               return ret;
> > +               return dev_err_probe(ov9282->dev, ret,
> > +                                    "HW configuration is not supported");
> >         }
> >
> >         ov9282->regmap = devm_cci_regmap_init_i2c(client, 16);
> >         if (IS_ERR(ov9282->regmap))
> >                 return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->regmap),
> > -                                    "Failed to init CCI\n");
> > +                                    "Failed to init CCI");
> >
> >         ret = ov9282_power_on(ov9282->dev);
> >         if (ret)
> > @@ -1399,7 +1397,7 @@ static int ov9282_probe(struct i2c_client *client)
> >         /* Check module identity */
> >         ret = ov9282_detect(ov9282);
> >         if (ret) {
> > -               dev_err(ov9282->dev, "failed to find sensor: %d", ret);
> > +               dev_err_probe(ov9282->dev, ret, "failed to find sensor");
> >                 goto error_power_off;
> >         }
> >
> > @@ -1409,7 +1407,7 @@ static int ov9282_probe(struct i2c_client *client)
> >
> >         ret = ov9282_init_controls(ov9282);
> >         if (ret) {
> > -               dev_err(ov9282->dev, "failed to init controls: %d", ret);
> > +               dev_err_probe(ov9282->dev, ret, "failed to init controls");
> >                 goto error_power_off;
> >         }
> >
> > @@ -1422,14 +1420,14 @@ static int ov9282_probe(struct i2c_client *client)
> >         ov9282->pad.flags = MEDIA_PAD_FL_SOURCE;
> >         ret = media_entity_pads_init(&ov9282->sd.entity, 1, &ov9282->pad);
> >         if (ret) {
> > -               dev_err(ov9282->dev, "failed to init entity pads: %d", ret);
> > +               dev_err_probe(ov9282->dev, ret, "failed to init entity pads");
> >                 goto error_handler_free;
> >         }
> >
> >         ov9282->sd.state_lock = ov9282->ctrl_handler.lock;
> >         ret = v4l2_subdev_init_finalize(&ov9282->sd);
> >         if (ret < 0) {
> > -               dev_err_probe(ov9282->dev, ret, "failed to init subdev\n");
> > +               dev_err_probe(ov9282->dev, ret, "failed to init subdev");
> >                 goto error_media_entity;
> >         }
> >
> >
> > --
> > 2.53.0
> >
> >

Re: [PATCH 04/10] media: i2c: ov9282: update flash_duration range even when powered down

[Dave Stevenson]

Hi Richard

On Mon, 14 Sept 2026 at 20:21, Richard Leitner
<richard.leitner@linux.dev> wrote:
>
> The flash_duration range update sat inside ov9282_update_exp_gain(), which
> ov9282_set_ctrl() only reaches after the pm_runtime_get_if_in_use() early
> return. So with the sensor idle the ceiling kept whatever value it had
> when it last streamed. Therefore setting exposure before flash_duration
> while the sensor is powered down may clamp flash_duration against an
> outdated exposure.
>
> Fix this by moving the range update before the power check.
>
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
>  drivers/media/i2c/ov9282.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 90a0fe542ce4a..4c88de1965171 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -645,6 +645,19 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
>                 if (ret)
>                         return ret;
>                 break;
> +       case V4L2_CID_EXPOSURE:
> +               /*
> +                * Ensure the flash duration range is also updated on powered
> +                * down sensors.
> +                */
> +               ret = __v4l2_ctrl_modify_range(ov9282->flash_duration, 0,
> +                                              ov9282_exposure_to_us(ov9282,
> +                                                                    ctrl->val),
> +                                              1,
> +                                              OV9282_STROBE_FRAME_SPAN_DEFAULT);
> +               if (ret)
> +                       return ret;
> +               break;

This hasn't removed the __v4l2_ctrl_modify_range call from
ov9282_update_exp_gain(), so the range gets updated twice if the
sensor is powered on.

  Dave

>         }
>
>         /* Set controls only if sensor is in power on state */
>
> --
> 2.53.0
>
>

Re: [PATCH 02/10] media: i2c: ov9282: fix line time and exposure time calculation

[Bryan O'Donoghue]

On 14/09/2026 20:20, Richard Leitner wrote:
>   The right
> clock is PLL2's system clock. With the PLL2 dividers left at their reset
> values the chain

I think this statement could use some more justification for the commit log.

PLL2 is stated in the data-sheet ?

Either way

Fixes:
Cc: stable@vger.kernel.org

---
bod

Re: [PATCH 03/10] media: i2c: ov9282: fix flash duration to/from microseconds conversion

[Bryan O'Donoghue]

On 14/09/2026 20:21, Richard Leitner wrote:
> Fix the conversion by dropping the constant factor and using the
> previously introduced ov9282_line_time_ns() helper instead.
> 
> Signed-off-by: Richard Leitner<richard.leitner@linux.dev>

Fixes:
Cc: stable@vger.kernel.org

---
bod

Re: [PATCH 02/10] media: i2c: ov9282: fix line time and exposure time calculation

[Richard Leitner]

Hi Bryan,

thanks for your feedback!

On Thu, Sep 17, 2026 at 11:51:46AM +0100, Bryan O'Donoghue wrote:
> On 14/09/2026 20:20, Richard Leitner wrote:
> >   The right
> > clock is PLL2's system clock. With the PLL2 dividers left at their reset
> > values the chain
> 
> I think this statement could use some more justification for the commit log.

Which part do you mean? That "PLL2's system clock" is the right clock?
Or the "dividers left at their reset values"?
Or both? :-)

> 
> PLL2 is stated in the data-sheet ?

Yes, PLL2 and its clock dividers are described in the datasheet (similar
to the formular shown in the commit message).

The fact that "PLL2's system clock" is the right for this, i.e. proof that
OV9282_REG_TIMING_HTS is counted in increments of it is according to my
research not available in the datasheet.

> 
> Either way
> 
> Fixes:
> Cc: stable@vger.kernel.org

As this whole series basically fixes things, would it be acceptable to add
the Cc for all patches?

> 
> ---
> bod

regards;rl