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