Re: [PATCH v2 2/2] media: i2c: Add Sony IMX908 image sensor driver

Hi Lachlan

On Mon, 17 Aug 2026 at 10:21, [email protected]
<[email protected]> wrote:
>
> Dear Dave,
>
> Thanks for your review.
> I was on summer holidays last week so sorry for the delay in replying.
>
> >Hi Lachlan
> >
> >These comments are made without having access to the datasheet or
> >software reference manual (I have requested them), so are based on my
> >experience of other Starvis sensors. Feel free to dismiss them if not
> >applicable to this sensor.
> >
> >And some are personal preferences, so feel free to disagree there :-)
> >
> >On Thu, 6 Aug 2026 at 08:12, Lachlan Michael <[email protected]> wrote:
> >>
> >> The Sony IMX908 is an 8.39 megapixel (3856x2176) CMOS image sensor
> >> with a MIPI CSI-2 output interface, configurable as either 2 or 4
> >> data lanes.
> >>
> >> Add a V4L2 sub-device driver for the sensor. The driver supports
> >> RAW10 and RAW12 output formats, exposure and analogue gain controls,
> >> horizontal and vertical flipping, horizontal and vertical blanking
> >> controls, window cropping and test pattern generation.
> >>
> >> HDR modes and RAW16 output are not currently supported.
> >>
> >> Signed-off-by: Lachlan Michael <[email protected]>
> >> ---
> >> Changes in v2:
> >> - Treat the pixel rate as a fixed sensor property (594 MHz, 8 px/clock),
> >>   read-only.
> >> - Compute HMAX from both the array and MIPI link floors.
> >> - Express HBLANK in pixels with a step of 8; keep HMAX fixed in crop
> >>   mode.
> >> - Drop struct imx908_mode; cache hmax/vmax directly.
> >> - Change the link-frequency table to s64.
> >> - Fix the probe error-unwind ordering.
> >> - Be silent on success (chip ID print is now dev_dbg).
> >> - Drop redundant comments.
> >> - Kconfig: fix a "module will be called" typo.
> >> ---
>
> <snip>

<snip>

> >> +/*
> >> + * Link (consumer) floor: the line period must be long enough for the MIPI
> >> + * burst (width * bpp bits) to drain over num_lanes * link_freq * 2 (DDR).
> >> + */
> >> +static u32 imx908_calc_link_min_hmax(struct imx908 *imx, u32 width, u8 bpp)
> >> +{
> >> +       u64 link_hz = imx908_link_freqs[imx->link_freq_idx];
> >> +       u64 num = (u64)width * bpp * IMX908_XHS_HZ;
> >> +       u64 den = (u64)imx->num_lanes * link_hz * 2; /* DDR */
> >> +
> >> +       /*
> >> +        * den can exceed 32 bits (e.g. 4 lanes * 720 MHz * 2 = 5.76 GHz), so
> >> +        * DIV_ROUND_UP_ULL / do_div would truncate the divisor to u32. Use a
> >> +        * full 64/64 division.
> >> +        */
> >> +       return DIV64_U64_ROUND_UP(num, den);
> >
> >It's unlikely to make that significant a difference, but don't you
> >need to account for the LP to/from HS transitions on the MIPI link?
>
> A more accurate minimum is really drain_time + framing + LP/HS_transition
> and you're right that here I am only calculating the drain_time. It's meant as
> a safe lower bound for the HBLANK range rather than an exact line time.
>
> At 1440 Mbps/lane (720 MHz link) for 4K RAW10 the floor is about 495
> clocks (~6.67 us):
>  - CSI-2 packet header/footer is 48 bits on top of width*bpp, ~0.1%.
>  - the LP/HS transition, from the D-PHY timings, is a few hundred ns per line,
>    ~7% of the floor at 4 lanes (~3% at 2 lanes).
>
> Both terms are strictly positive, so folding them in only ever raises the floor,
> so this is more about accuracy than safety.
>
> Given that, do you suggest I should add in the packet framing and/or the
> LP/HS transition into this floor, or is it ok to leave it as the payload-only
> bound (as-is)?

As long as the calculation gives a value that is expected to work,
then that's fine.

> >> +}
> >> +
> >> +/* HMAX must satisfy both the array (producer) and link (consumer) limits */
> >> +static u16 imx908_calc_min_hmax(struct imx908 *imx, u32 width, u8 bpp)
> >> +{
> >> +       return max(imx908_calc_array_min_hmax(width),
> >> +                  imx908_calc_link_min_hmax(imx, width, bpp));
> >> +}
> >> +
> >> +static u32 imx908_calc_min_vmax(const struct v4l2_rect *crop)
> >> +{
> >> +       /* In window crop mode constrain VMAX (from datasheet) */
> >> +       if (!v4l2_rect_equal(crop, &imx908_active_area))
> >> +               return max_t(u32, crop->height + IMX908_VMAX_CROP_MIN_MARGIN,
> >> +                          IMX908_VMAX_CROP_MIN_LIMIT);
> >> +
> >> +       return IMX908_VMAX_DEFAULT;
> >
> >VMAX values in the Starvis datasheets always seem to be "let's give a
> >nice round frame rate", not "this is the minimum that is valid".
> >If run in window mode at 3856x2176 then VMAX would be 2176+70 = 2246,
> >so I guess not such a big difference, but default is not the same as
> >minimum.
> >
> >(IMX662 runs quite happily with a margin of 40 which gives an extra
> >10% on the frame rate compared to the default. I ought to check
> >against the datasheet for that one though)
>
> You're right — 2250 is the 30 fps operating point, not a stated minimum,
> but the +70 margin is only given for window-crop mode in the datasheet.
>
> There's no documented all-pixel VMAX minimum, so for crop mode I keep the
> datasheet restriction (height + 70, >= 1206) and for all-pixel I leave the
> floor at the 2250 default as a conservative bound rather than inferring one.
>
> I added a comment:
>         /* No datasheet min for all-pixel; use 30 fps default as safe floor */
>         return IMX908_VMAX_DEFAULT;

Any chance you could ask your colleagues?
If height + 70 can be used in all cases then it just simplifies things.

<snip>

> >> +static int imx908_program_window(struct imx908 *imx,
> >> +                                const struct v4l2_rect *crop)
> >> +{
> >> +       bool all_pixel_mode = v4l2_rect_equal(crop, &imx908_active_area);
> >> +       int ret = 0;
> >> +
> >> +       cci_write(imx->cci, IMX908_REG_WINMODE,
> >> +                 all_pixel_mode ? IMX908_WINMODE_ALLPIX : IMX908_WINMODE_CROP,
> >> +                 &ret);
> >
> >This one knocks on to several other checks of the active_area / all-pixel mode.
> >Why switch into the pre-defined all-pixel mode at all? What advantage
> >does it give you over always using window mode?
> >
> >To my mind it just gives the possibility that the crop defined in the
> >structure is incorrect, therefore you aren't getting the pixels you
> >thought you were.
> >(I learned the hard way working on IMX675).
>
> All-pixel mode has variable HMAX; in window-crop mode the datasheet says
> to keep HMAX at the drive-mode value, so collapsing to always-window would
> lose that. The test pattern generator is also all-pixel only. On the crop-accuracy
> concern: the advertised active_area (2176) is the datasheet's active height, distinct
> from the 2180 effective height (the extra 4 lines are the ignored effective-pixel area),
> so all-pixel mode does read out what we advertise.

Not supporting a variable HMAX in window-crop mode rather defeats the
purpose of cropping to reduce the line time and increase frame rate.
Does it really not work? Again can it be checked? Datasheets so often
get copy/pasted from previous versions.

Extending HMAX in all-pixel mode adds the possibility of increasing
the line time and hence allows for longer exposure captures. I haven't
checked the limits for exposure time if HMAX is fixed vs variable, but
I'd be tempted to drop it and make V4L2_CID_HBLANK read only. It can
be revisited at a later date if someone does want extra long
exposures.

The test pattern only working in all-pixel mode is largely irrelevant
as it's only used for test purposes. Trying to gate use of the test
pattern on cropping not being applied starts to get messy.
(I do have the datasheet and app notes for IMX908 now, but none cover
the pattern generator)

Thanks
  Dave