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