Re: [PATCH 02/10] media: i2c: ov9282: fix line time and exposure time calculation
From: Richard Leitner
Date: Mon Sep 28 2026 - 07:34:22 EST
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@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.
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@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
> > > >
> > > >