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