[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

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
>
>

[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

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 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

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

[Dave Stevenson]

Hi Richard

On Mon, 14 Sept 2026 at 20:21, Richard Leitner
<richard.leitner@linux.dev> wrote:
>
> 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.

It really shouldn't be. PIXEL_RATE is the pixel array and LINK_FREQ is
the MIPI rate. Many drivers do conflate the two.

However I did notice a couple of weeks back that pixel_rate is
incorrect for 8bit readout on ov9282 [1]. (It's correct for 10bit
readout).

<quote> Actually I see the problem. In the hardware the pixel rate for 8 bit
is scaled by the change of PLL2 multiplier from 0x50 to 0x60 = x1.2.
The pixel rate control is scaled from /10 to /8 or x1.25. 1.2/1.25 =
0.96, so my 96% of speed would be spot on. The use of link frequency
in computing pixel rate is totally bogus as they are on independent
PLLs </quote>

If you correct OV9282_PIXEL_RATE_8BIT to being 192000000 (instead of
the current 200000000), does that solve your problem without
recomputing things?

  Dave

[1] https://lore.kernel.org/linux-media/CAPY8ntBaSHjztuSLTOtv9KYvEbmcbinSMJ78ZDg+0sZg-1fz3Q@mail.gmail.com/
The original reporter did come back to me and acknowledge he was using
the downstream Rockchip driver which is doing the wrong thing.

> 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
>
>

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

[Richard Leitner]

Hi Dave,

thanks for your feedback!

On Wed, Sep 23, 2026 at 03:16:27PM +0100, Dave Stevenson wrote:
> Hi Richard
> 
> On Mon, 14 Sept 2026 at 20:21, Richard Leitner
> <richard.leitner@linux.dev> wrote:
> >
> > 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.
> 
> It really shouldn't be. PIXEL_RATE is the pixel array and LINK_FREQ is
> the MIPI rate. Many drivers do conflate the two.
> 
> However I did notice a couple of weeks back that pixel_rate is
> incorrect for 8bit readout on ov9282 [1]. (It's correct for 10bit
> readout).
> 
> <quote> Actually I see the problem. In the hardware the pixel rate for 8 bit
> is scaled by the change of PLL2 multiplier from 0x50 to 0x60 = x1.2.
> The pixel rate control is scaled from /10 to /8 or x1.25. 1.2/1.25 =
> 0.96, so my 96% of speed would be spot on. The use of link frequency
> in computing pixel rate is totally bogus as they are on independent
> PLLs </quote>

So I guess I should also get rid of the pixel rate pre-processor
calculation which depends on the link frequency?

Is it OK with you to just hard-code them?

#define OV9282_PIXEL_RATE_10BIT   160000000
#define OV9282_PIXEL_RATE_8BIT    192000000

AFAICT it should be possible to calculate the PLL1 pix clk from the input
clock (inclk or XVCLK in the datasheet) provided in the dts. But tbh I
would prefer to not add this to this series. If it's desired and possible
from your point of view, I can tackle this in a future series.

> 
> If you correct OV9282_PIXEL_RATE_8BIT to being 192000000 (instead of
> the current 200000000), does that solve your problem without
> recomputing things?

Thanks for the explanation above. I guess you're right, the link freq/mipi
rate should not be in there. I've just ran a quick test and it should be
fine with that change.

As i would like to keep the ov9282_line_time_ns() function, what's your
take on the following implementation? Would that be OK for a v2?

static u32 ov9282_line_time_ns(struct ov9282 *ov9282)
{
	u64 hts = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
	return div_u64(hts * NSEC_PER_SEC, ov9282->pixel_rate->val);
}

thanks!

regards;rl

> 
>   Dave
> 
> [1] https://lore.kernel.org/linux-media/CAPY8ntBaSHjztuSLTOtv9KYvEbmcbinSMJ78ZDg+0sZg-1fz3Q@mail.gmail.com/
> The original reporter did come back to me and acknowledge he was using
> the downstream Rockchip driver which is doing the wrong thing.
> 
> > 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
> >
> >

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

[Dave Stevenson]

Hi Richard

On Mon, 28 Sept 2026 at 09:56, Richard Leitner
<richard.leitner@linux.dev> wrote:
>
> Hi Dave,
>
> thanks for your feedback!
>
> On Wed, Sep 23, 2026 at 03:16:27PM +0100, Dave Stevenson wrote:
> > Hi Richard
> >
> > On Mon, 14 Sept 2026 at 20:21, Richard Leitner
> > <richard.leitner@linux.dev> wrote:
> > >
> > > 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.
> >
> > It really shouldn't be. PIXEL_RATE is the pixel array and LINK_FREQ is
> > the MIPI rate. Many drivers do conflate the two.
> >
> > However I did notice a couple of weeks back that pixel_rate is
> > incorrect for 8bit readout on ov9282 [1]. (It's correct for 10bit
> > readout).
> >
> > <quote> Actually I see the problem. In the hardware the pixel rate for 8 bit
> > is scaled by the change of PLL2 multiplier from 0x50 to 0x60 = x1.2.
> > The pixel rate control is scaled from /10 to /8 or x1.25. 1.2/1.25 =
> > 0.96, so my 96% of speed would be spot on. The use of link frequency
> > in computing pixel rate is totally bogus as they are on independent
> > PLLs </quote>
>
> So I guess I should also get rid of the pixel rate pre-processor
> calculation which depends on the link frequency?
>
> Is it OK with you to just hard-code them?
>
> #define OV9282_PIXEL_RATE_10BIT   160000000
> #define OV9282_PIXEL_RATE_8BIT    192000000
>
> AFAICT it should be possible to calculate the PLL1 pix clk from the input
> clock (inclk or XVCLK in the datasheet) provided in the dts. But tbh I
> would prefer to not add this to this series. If it's desired and possible
> from your point of view, I can tackle this in a future series.

I'm happy for them to be hard coded, possibly with a comment that the
pixel rate is derived from the PLL2 clock tree. For 10bit readout, the
whole PLL config is exactly the same as the sample PLL configuration
given in the datasheet (table 2-10 for me with datasheet v1.53). 8bit
readout changes the PLL2 multiplier.

The registers initially looked pretty close to the CCS standard and so
possible to use the ccs-pll helpers, except the dividers support /1.5
and /2.5 in places so I don't think it can be used as-is (PLL1 prediv
is set to /1.5 in the current setup).

Whilst feasible, if no one has had a use case needing an alternate
input clock rate in 5 years since the ov9282 driver was merged (Aug
2021), then there's little point in jumping through hoops to
accommodate a theoretical future user who may never surface. I'd save
yourself the effort.

> >
> > If you correct OV9282_PIXEL_RATE_8BIT to being 192000000 (instead of
> > the current 200000000), does that solve your problem without
> > recomputing things?
>
> Thanks for the explanation above. I guess you're right, the link freq/mipi
> rate should not be in there. I've just ran a quick test and it should be
> fine with that change.

Great. If just correcting the pixel rate solves the problem, then it's
better not to complicate the calculations in other places
unnecessarily.

> As i would like to keep the ov9282_line_time_ns() function, what's your
> take on the following implementation? Would that be OK for a v2?
>
> static u32 ov9282_line_time_ns(struct ov9282 *ov9282)
> {
>         u64 hts = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
>         return div_u64(hts * NSEC_PER_SEC, ov9282->pixel_rate->val);
> }

I have no issue with having a helper for the flash calculations.

  Dave

> thanks!
>
> regards;rl
>
> >
> >   Dave
> >
> > [1] https://lore.kernel.org/linux-media/CAPY8ntBaSHjztuSLTOtv9KYvEbmcbinSMJ78ZDg+0sZg-1fz3Q@mail.gmail.com/
> > The original reporter did come back to me and acknowledge he was using
> > the downstream Rockchip driver which is doing the wrong thing.
> >
> > > 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
> > >
> > >

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

[Richard Leitner]

On Mon, Sep 28, 2026 at 12:02:18PM +0100, Dave Stevenson wrote:
> Hi Richard
> 
> On Mon, 28 Sept 2026 at 09:56, Richard Leitner
> <richard.leitner@linux.dev> wrote:
> >
> > Hi Dave,
> >
> > thanks for your feedback!
> >
> > On Wed, Sep 23, 2026 at 03:16:27PM +0100, Dave Stevenson wrote:
> > > Hi Richard
> > >
> > > On Mon, 14 Sept 2026 at 20:21, Richard Leitner
> > > <richard.leitner@linux.dev> wrote:
> > > >
> > > > 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.
> > >
> > > It really shouldn't be. PIXEL_RATE is the pixel array and LINK_FREQ is
> > > the MIPI rate. Many drivers do conflate the two.
> > >
> > > However I did notice a couple of weeks back that pixel_rate is
> > > incorrect for 8bit readout on ov9282 [1]. (It's correct for 10bit
> > > readout).
> > >
> > > <quote> Actually I see the problem. In the hardware the pixel rate for 8 bit
> > > is scaled by the change of PLL2 multiplier from 0x50 to 0x60 = x1.2.
> > > The pixel rate control is scaled from /10 to /8 or x1.25. 1.2/1.25 =
> > > 0.96, so my 96% of speed would be spot on. The use of link frequency
> > > in computing pixel rate is totally bogus as they are on independent
> > > PLLs </quote>
> >
> > So I guess I should also get rid of the pixel rate pre-processor
> > calculation which depends on the link frequency?
> >
> > Is it OK with you to just hard-code them?
> >
> > #define OV9282_PIXEL_RATE_10BIT   160000000
> > #define OV9282_PIXEL_RATE_8BIT    192000000
> >
> > AFAICT it should be possible to calculate the PLL1 pix clk from the input
> > clock (inclk or XVCLK in the datasheet) provided in the dts. But tbh I
> > would prefer to not add this to this series. If it's desired and possible
> > from your point of view, I can tackle this in a future series.
> 
> I'm happy for them to be hard coded, possibly with a comment that the
> pixel rate is derived from the PLL2 clock tree. For 10bit readout, the
> whole PLL config is exactly the same as the sample PLL configuration
> given in the datasheet (table 2-10 for me with datasheet v1.53). 8bit
> readout changes the PLL2 multiplier.
> 
> The registers initially looked pretty close to the CCS standard and so
> possible to use the ccs-pll helpers, except the dividers support /1.5
> and /2.5 in places so I don't think it can be used as-is (PLL1 prediv
> is set to /1.5 in the current setup).
> 
> Whilst feasible, if no one has had a use case needing an alternate
> input clock rate in 5 years since the ov9282 driver was merged (Aug
> 2021), then there's little point in jumping through hoops to
> accommodate a theoretical future user who may never surface. I'd save
> yourself the effort.

That's of course fine with me ;-) Thanks for the quick response.

Then I'll add a comment and will likely send v2 later today.

regards;rl

> 
> > >
> > > If you correct OV9282_PIXEL_RATE_8BIT to being 192000000 (instead of
> > > the current 200000000), does that solve your problem without
> > > recomputing things?
> >
> > Thanks for the explanation above. I guess you're right, the link freq/mipi
> > rate should not be in there. I've just ran a quick test and it should be
> > fine with that change.
> 
> Great. If just correcting the pixel rate solves the problem, then it's
> better not to complicate the calculations in other places
> unnecessarily.
> 
> > As i would like to keep the ov9282_line_time_ns() function, what's your
> > take on the following implementation? Would that be OK for a v2?
> >
> > static u32 ov9282_line_time_ns(struct ov9282 *ov9282)
> > {
> >         u64 hts = ov9282->cur_mode->width + ov9282->hblank_ctrl->val;
> >         return div_u64(hts * NSEC_PER_SEC, ov9282->pixel_rate->val);
> > }
> 
> I have no issue with having a helper for the flash calculations.
> 
>   Dave
> 
> > thanks!
> >
> > regards;rl
> >
> > >
> > >   Dave
> > >
> > > [1] https://lore.kernel.org/linux-media/CAPY8ntBaSHjztuSLTOtv9KYvEbmcbinSMJ78ZDg+0sZg-1fz3Q@mail.gmail.com/
> > > The original reporter did come back to me and acknowledge he was using
> > > the downstream Rockchip driver which is doing the wrong thing.
> > >
> > > > 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

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 03/10] media: i2c: ov9282: fix flash duration to/from microseconds conversion

[Richard Leitner]

Hi Bryan,

thanks for your feedback!

On Thu, Sep 17, 2026 at 10:53:09AM +0000, Bryan O'Donoghue wrote:
> 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

Will address this in v2.

Thanks & regards;rl

> 
> ---
> bod
> 

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

[Dave Stevenson]

Hi Richard

On Mon, 14 Sept 2026 at 20:21, Richard Leitner
<richard.leitner@linux.dev> wrote:
>
> 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
> -

I'd been scratching my head over this one of where this magic number
had come from previously. I've now just clocked that it's the
(corrected) 8bit pixel rate.
Using the updated version of ov9282_line_time_ns we've discussed in
2/10, this should therefore give the correct numbers.

I'll hold off on giving an R-b until I can see it in-situ with the
other updates, but it looks like it should be correct.

  Dave

>  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
>
>

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

[Richard Leitner]

On Mon, Sep 28, 2026 at 12:07:13PM +0100, Dave Stevenson wrote:
> Hi Richard
> 
> On Mon, 14 Sept 2026 at 20:21, Richard Leitner
> <richard.leitner@linux.dev> wrote:
> >
> > 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
> > -
> 
> I'd been scratching my head over this one of where this magic number
> had come from previously. I've now just clocked that it's the
> (corrected) 8bit pixel rate.
> Using the updated version of ov9282_line_time_ns we've discussed in
> 2/10, this should therefore give the correct numbers.

This was literally introduced by me as I had no clue how to correctly
calculate those values back then. The value was basically the result of my
scope measurements for my specific use case. It was the best
approximation I could find back then ;-)

https://lore.kernel.org/lkml/20251209-ov9282-flash-strobe-v10-0-0117cab82e2d@linux.dev/

> 
> I'll hold off on giving an R-b until I can see it in-situ with the
> other updates, but it looks like it should be correct.

Sure. Makes sense!

thanks!

regards;rl

> 
>   Dave
> 
> >  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

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 04/10] media: i2c: ov9282: update flash_duration range even when powered down

[Richard Leitner]

Hi Dave,
thanks for your feedback!

On Thu, Sep 17, 2026 at 11:38:22AM +0100, Dave Stevenson wrote:
> 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.

I will take a look at it and adapt it for v2 accordingly.

regards;rl

> 
>   Dave
> 
> >         }
> >
> >         /* 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

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

[Dave Stevenson]

Hi Richard

On Mon, 14 Sept 2026 at 20:21, Richard Leitner
<richard.leitner@linux.dev> wrote:
>
> 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.

The exposure control is dependent on vblank / frame height, not the line time.

> 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);

I would have expected that this should be handled by the first clause
in ov9282_set_ctrl where if vblank is changed it updates exp_ctrl.
That's how many drivers handle it.

Looking closer they tend to set an explicit vblank on mode change
though, and it is that which triggers the set_ctrl.
There has been previous debate as to whether changing mode should
reset blanking to give a defined frame rate. Memory says that Sakari
was in favour of doing that, but I can't find the thread.
Sashiko also flagged on a previous patchset of mine that if the
previous mode happened to have had vblank adjusted to be the same as
the new default value that the new mode selects, then the control
handler framework won't call set_ctrl(VBLANK), and so the exposure
range (depending on mode height and VBLANK) won't get updated.

All a little messy, so I can be persuaded that there are enough holes
in the current driver that the easiest solution is just to update the
control ranges here if you say that set_ctrl(VBLANK) doesn't get
called in this case.

 Dave

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

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

[Richard Leitner]

On Mon, Sep 28, 2026 at 02:15:05PM +0100, Dave Stevenson wrote:
> Hi Richard
> 
> On Mon, 14 Sept 2026 at 20:21, Richard Leitner
> <richard.leitner@linux.dev> wrote:
> >
> > 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.
> 
> The exposure control is dependent on vblank / frame height, not the line time.

Sure. You're right. Thanks for the catch!

> 
> > 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);
> 
> I would have expected that this should be handled by the first clause
> in ov9282_set_ctrl where if vblank is changed it updates exp_ctrl.
> That's how many drivers handle it.
> 
> Looking closer they tend to set an explicit vblank on mode change
> though, and it is that which triggers the set_ctrl.
> There has been previous debate as to whether changing mode should
> reset blanking to give a defined frame rate. Memory says that Sakari
> was in favour of doing that, but I can't find the thread.
> Sashiko also flagged on a previous patchset of mine that if the
> previous mode happened to have had vblank adjusted to be the same as
> the new default value that the new mode selects, then the control
> handler framework won't call set_ctrl(VBLANK), and so the exposure
> range (depending on mode height and VBLANK) won't get updated.

Thanks for the explanation!

If resetting vblank and hblank on a mode change is the "preferred" or
"usual" implementation, then I'm of course fine with that. Either in
this series or in a future one ;-)

> 
> All a little messy, so I can be persuaded that there are enough holes
> in the current driver that the easiest solution is just to update the
> control ranges here if you say that set_ctrl(VBLANK) doesn't get
> called in this case.

Regarding the vblank set_ctrl on mode change: I will do another testing
round and either add a comment on the behaviour or remove that
modify_range call.

thanks!

regards;rl

> 
>  Dave
> 
> > +       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

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

[Dave Stevenson]

Hi Richard

On Mon, 14 Sept 2026 at 20:21, Richard Leitner
<richard.leitner@linux.dev> wrote:
>
> 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.
> +                */

Except for the comment, this clause is identical to that for
V4L2_CID_EXPOSURE that you added in patch 4. No need for the
duplication.

  Dave

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

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

[Richard Leitner]

Hi Dave,

thanks for your feedback!

On Wed, Sep 23, 2026 at 04:09:25PM +0100, Dave Stevenson wrote:
> Hi Richard
> 
> On Mon, 14 Sept 2026 at 20:21, Richard Leitner
> <richard.leitner@linux.dev> wrote:
> >
> > 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.
> > +                */
> 
> Except for the comment, this clause is identical to that for
> V4L2_CID_EXPOSURE that you added in patch 4. No need for the
> duplication.

That's true. I will merge the two switch arms to get rid of the
duplicated code. Thanks!

regards;rl

> 
>   Dave
> 
> > +               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

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
>
>

[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

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 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
> >
> >

[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

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

[Dave Stevenson]

Hi Richard

On Mon, 14 Sept 2026 at 20:21, Richard Leitner
<richard.leitner@linux.dev> wrote:
>
> 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);

Reading the driver for how it handles flash duration, it all gets a
bit convoluted. I see part of it comes from the units being usecs
whilst natively it is in lines, but we've got this slightly odd calc
and adding 1, and rounding in try_ctrl to get closest to the absolute
value.
Seeing as this is the max flash_duration, is it actually limited by
the exposure, or by the frame duration? The difference between those
is a minimum of 25 lines (OV9282_EXPOSURE_OFFSET), which I think gives
you up to another 9usecs to play with. That covers any of this
rounding stuff. You can afford to always round that one down and never
clip the range below the exposure time.

There is reference in the docs to a different behaviour if "the
vertical blanking period is long", but with a bit to set to give
stable behaviour (0x3017 bit 1).

You've far more experience with how this sensor handles the strobe
outputs though.

  Dave

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

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

[Richard Leitner]

Hi Dave,

thanks for your feedback! Greatly appreciated!

On Mon, Sep 28, 2026 at 02:47:36PM +0100, Dave Stevenson wrote:
> Hi Richard
> 
> On Mon, 14 Sept 2026 at 20:21, Richard Leitner
> <richard.leitner@linux.dev> wrote:
> >
> > 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);
> 
> Reading the driver for how it handles flash duration, it all gets a
> bit convoluted. I see part of it comes from the units being usecs
> whilst natively it is in lines, but we've got this slightly odd calc
> and adding 1, and rounding in try_ctrl to get closest to the absolute
> value.

That's true. I'm also not happy with this back-and-forth conversion between
u/n-secs and lines. From a V4l2 API user perspective I'd love to have
all "timing related" controls in the same unit (likely usecs). I was
already playing around with that approach, but as this would break the
current exporsure property I have not sent those patches...

If you have any "mainline-acceptable" idea on how to do that I would
definitely love to hear it ;-)

> Seeing as this is the max flash_duration, is it actually limited by
> the exposure, or by the frame duration? The difference between those
> is a minimum of 25 lines (OV9282_EXPOSURE_OFFSET), which I think gives
> you up to another 9usecs to play with. That covers any of this
> rounding stuff. You can afford to always round that one down and never
> clip the range below the exposure time.

From a hardware perspective this isn't limited at all AFAICT. But from a
"use-case" perspective a flash_duration (with a flash offset=0, which is
the hard-coded default case in this driver currently) longer than the
exposure time makes no sense, as it does not have any effect on the image.

With this patch I want to make sure to be able to illuminate the frame
during the complete exposure time, but keep the maximum as short as
possible. So I was comparing/aiming this at the exposure time, not the
frame duration.

As the frame duration is OV9282_EXPOSURE_OFFSET longer than the exposure
time: Would a "turned-on flash" during those 25 lines have any impact on
the frames brightness in your opinion?
I haven't explicitely tested/measured this with hardware, but I guess
this does not have any impact?

Or do you mean when calculating against the frame duration the +/-1
stuff can be dropped and the code would be easier to read?

Sorry If misunderstood your feedback somehow.

> 
> There is reference in the docs to a different behaviour if "the
> vertical blanking period is long", but with a bit to set to give
> stable behaviour (0x3017 bit 1).

I've read that paragraph in the datasheet, yes. But as this was not
affecting my use-case I haven't included this in this series or my
downstream kernel. Do you think it's worth adding support for this? Do
you know of any reports complaining about this power saving feature?

thanks!

regards;rl

> 
> You've far more experience with how this sensor handles the strobe
> outputs though.
> 
>   Dave
> 
> > +
> > +       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
> >
> >

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

[Dave Stevenson]

On Mon, 28 Sept 2026 at 15:23, Richard Leitner
<richard.leitner@linux.dev> wrote:
>
> Hi Dave,
>
> thanks for your feedback! Greatly appreciated!
>
> On Mon, Sep 28, 2026 at 02:47:36PM +0100, Dave Stevenson wrote:
> > Hi Richard
> >
> > On Mon, 14 Sept 2026 at 20:21, Richard Leitner
> > <richard.leitner@linux.dev> wrote:
> > >
> > > 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);
> >
> > Reading the driver for how it handles flash duration, it all gets a
> > bit convoluted. I see part of it comes from the units being usecs
> > whilst natively it is in lines, but we've got this slightly odd calc
> > and adding 1, and rounding in try_ctrl to get closest to the absolute
> > value.
>
> That's true. I'm also not happy with this back-and-forth conversion between
> u/n-secs and lines. From a V4l2 API user perspective I'd love to have
> all "timing related" controls in the same unit (likely usecs). I was
> already playing around with that approach, but as this would break the
> current exporsure property I have not sent those patches...
>
> If you have any "mainline-acceptable" idea on how to do that I would
> definitely love to hear it ;-)

Sadly I think that there is a need for some of the back and forth in
conversions :-(

> > Seeing as this is the max flash_duration, is it actually limited by
> > the exposure, or by the frame duration? The difference between those
> > is a minimum of 25 lines (OV9282_EXPOSURE_OFFSET), which I think gives
> > you up to another 9usecs to play with. That covers any of this
> > rounding stuff. You can afford to always round that one down and never
> > clip the range below the exposure time.
>
> From a hardware perspective this isn't limited at all AFAICT. But from a
> "use-case" perspective a flash_duration (with a flash offset=0, which is
> the hard-coded default case in this driver currently) longer than the
> exposure time makes no sense, as it does not have any effect on the image.
>
> With this patch I want to make sure to be able to illuminate the frame
> during the complete exposure time, but keep the maximum as short as
> possible. So I was comparing/aiming this at the exposure time, not the
> frame duration.
>
> As the frame duration is OV9282_EXPOSURE_OFFSET longer than the exposure
> time: Would a "turned-on flash" during those 25 lines have any impact on
> the frames brightness in your opinion?
> I haven't explicitely tested/measured this with hardware, but I guess
> this does not have any impact?

No, as this is a global shutter sensor I wouldn't expect the flash to
impact the image if a connected flash was on for more lines than the
exposure.

> Or do you mean when calculating against the frame duration the +/-1
> stuff can be dropped and the code would be easier to read?

Yes, I'm suggesting the driver handles the range of values that the
hardware can support correctly. Whether that helps for your image
quality requirements is a different question.
The driver can pass the buck with at least some of the rounding issues
to userspace, which potentially has use-case specific knowledge. And
it simplifies the driver in the process.

> Sorry If misunderstood your feedback somehow.
>
> >
> > There is reference in the docs to a different behaviour if "the
> > vertical blanking period is long", but with a bit to set to give
> > stable behaviour (0x3017 bit 1).
>
> I've read that paragraph in the datasheet, yes. But as this was not
> affecting my use-case I haven't included this in this series or my
> downstream kernel. Do you think it's worth adding support for this? Do
> you know of any reports complaining about this power saving feature?

It only matters if you allow flash duration to be significantly
greater than the exposure time, which I was effectively proposing.

  Dave

> thanks!
>
> regards;rl
>
> >
> > You've far more experience with how this sensor handles the strobe
> > outputs though.
> >
> >   Dave
> >
> > > +
> > > +       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 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
> >
> >