Re: [PATCH v2 2/2] media: i2c: Add Sony IMX908 image sensor driver
From: Lachlan.Michael@xxxxxxxx
Date: Mon Aug 17 2026 - 05:43:29 EST
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 <lachlan.michael@xxxxxxxx> 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 <lachlan.michael@xxxxxxxx>
>> ---
>> 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>
>> + { CCI_REG8(0x46a6), 0xcf },
>> + { CCI_REG8(0x46e2), 0xf3 },
>> +};
>> +
>> +static const u32 tuning_regs[] = {
>
>Rename to bit_depth_regs? That is what they are programmed based on.
>Almost all registers affect the tuning.
Ok.
>> + CCI_REG8(0x3d78),
>> + CCI_REG8(0x3d79),
>> + CCI_REG8(0x3d80),
>> + CCI_REG8(0x3d81),
>> + CCI_REG8(0x3d88),
>> + CCI_REG8(0x3d89),
>> + CCI_REG8(0x3d90),
>> + CCI_REG8(0x3d91),
>> +};
>> +
<snip>
>> +enum {
>> + IMX908_LINK_FREQ_297MHZ,
>> + IMX908_LINK_FREQ_360MHZ,
>> + IMX908_LINK_FREQ_445MHZ,
>> + IMX908_LINK_FREQ_594MHZ,
>> + IMX908_LINK_FREQ_720MHZ,
>> + IMX908_LINK_FREQ_891MHZ,
>> + IMX908_LINK_FREQ_1039MHZ,
>> + IMX908_LINK_FREQ_1188MHZ,
>
>If you reversed the order in this enum then it would also be the
>register value. If you want to be explicit, then assign a value to
>each as well
> IMX908_LINK_FREQ_1188MHZ = 0x00,
> IMX908_LINK_FREQ_1039MHZ = 0x01,
> IMX908_LINK_FREQ_891MHZ = 0x02,
> etc
>That would make imx908_datarate_sel[] and
>imx908_link_freq_to_datarate_sel() redundant.
>The link frequency control is read only, so it makes no difference to
>userspace what order they're in.
Oh, nice suggestion :-)
Fixed for next version.
>> +};
>> +
>> +static const s64 imx908_link_freqs[] = {
>> + [IMX908_LINK_FREQ_297MHZ] = 297000000LL,
>> + [IMX908_LINK_FREQ_360MHZ] = 360000000LL,
>> + [IMX908_LINK_FREQ_445MHZ] = 445000000LL,
>> + [IMX908_LINK_FREQ_594MHZ] = 594000000LL,
>> + [IMX908_LINK_FREQ_720MHZ] = 720000000LL,
>> + [IMX908_LINK_FREQ_891MHZ] = 891000000LL,
>> + [IMX908_LINK_FREQ_1039MHZ] = 1039500000LL,
>> + [IMX908_LINK_FREQ_1188MHZ] = 1188000000LL,
>> +};
>> +
>> +/* DDR lane_rate (Mbps/lane) = 2 x link_freq (Hz) / 1e6 */
>> +static const u8 imx908_datarate_sel[] = {
>> + [IMX908_LINK_FREQ_297MHZ] = 0x07, /* 594 Mbps */
>> + [IMX908_LINK_FREQ_360MHZ] = 0x06, /* 720 Mbps */
>> + [IMX908_LINK_FREQ_445MHZ] = 0x05, /* 891 Mbps */
>> + [IMX908_LINK_FREQ_594MHZ] = 0x04, /* 1188 Mbps */
>> + [IMX908_LINK_FREQ_720MHZ] = 0x03, /* 1440 Mbps */
>> + [IMX908_LINK_FREQ_891MHZ] = 0x02, /* 1782 Mbps */
>> + [IMX908_LINK_FREQ_1039MHZ] = 0x01, /* 2079 Mbps */
>> + [IMX908_LINK_FREQ_1188MHZ] = 0x00, /* 2376 Mbps */
>> +};
>> +
>> +/* Allowed clock values in Hz */
>> +static const u32 imx908_inck_table[] = {
>> + 74250000,
>> + 37125000,
>> + 72000000,
>> + 27000000,
>> + 24000000,
>> +};
>> +
>> +struct imx908 {
>> + struct v4l2_subdev sd;
>> + struct media_pad pad;
>> + struct device *dev;
>> +
>> + struct regmap *cci;
>> +
>> + struct clk *xclk;
>> + struct gpio_desc *reset_gpio;
>> + struct regulator_bulk_data supplies[ARRAY_SIZE(imx908_supply_names)];
>> +
>> + u8 inck_sel;
>> +
>> + u8 num_lanes;
>> + unsigned long link_freq_bitmap;
>> + unsigned int link_freq_idx;
>> +
>> + /* Cached current sensor timing */
>> + u16 hmax; /* clocks per line */
>> + u32 vmax; /* lines per frame */
>> +
>> + struct {
>> + struct v4l2_ctrl_handler handler;
>> +
>> + struct v4l2_ctrl *pixel_rate; /* fixed, read-only */
>> + struct v4l2_ctrl *exposure;
>> + struct v4l2_ctrl *vblank;
>> + struct v4l2_ctrl *hblank;
>> + struct v4l2_ctrl *test_pattern;
>> + } ctrls;
>> +};
>> +
>> +static inline struct imx908 *to_imx908(struct v4l2_subdev *_sd)
>> +{
>> + return container_of(_sd, struct imx908, sd);
>> +}
>> +
>> +/* ----------------------- Basic Helper Functions --------------------------- */
>> +
>> +static int imx908_link_freq_to_datarate_sel(struct imx908 *imx,
>> + u64 link_freq_hz, u8 *sel)
>> +{
>> + for (unsigned int i = 0; i < ARRAY_SIZE(imx908_link_freqs); i++) {
>> + if (imx908_link_freqs[i] == link_freq_hz) {
>> + *sel = imx908_datarate_sel[i];
>> + return 0;
>> + }
>> + }
>> + return -EINVAL;
>> +}
>> +
>> +static bool imx908_mbus_code_supported(struct imx908 *imx, u32 code)
>> +{
>> + for (unsigned int i = 0; i < ARRAY_SIZE(imx908_mbus_codes); i++) {
>> + if (imx908_mbus_codes[i] == code)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static u16 imx908_calc_hmax(u32 width, u32 hblank)
>> +{
>> + /* Fixed pixel rate makes this a divide-by-8 */
>> + u32 hmax = DIV_ROUND_UP(width + hblank, IMX908_PIX_PER_CLK);
>> +
>> + return min_t(u32, hmax, IMX908_HMAX_MAX);
>> +}
>> +
>> +static u32 imx908_calc_vmax(u32 height, u32 vblank)
>> +{
>> + return min_t(u32, height + vblank, IMX908_VMAX_MAX);
>> +}
>> +
>> +/*
>> + * Array (producer) floor: with the fixed pixel rate the column ADC emits
>> + * IMX908_PIX_PER_CLK pixels per internal clock, so a line cannot be shorter
>> + * than ceil(width / 8) internal clocks.
>> + */
>> +static u32 imx908_calc_array_min_hmax(u32 width)
>> +{
>> + return DIV_ROUND_UP(width, IMX908_PIX_PER_CLK);
>> +}
>> +
>> +/*
>> + * 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)?
>> +}
>> +
>> +/* 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;
>> +}
>> +
>> +static u32 imx908_calc_min_vblank(const struct v4l2_rect *crop)
>> +{
>> + u32 min_vmax = imx908_calc_min_vmax(crop);
>> +
>> + if (min_vmax <= crop->height)
>> + return 0;
>> +
>> + return min_vmax - crop->height;
>> +}
>> +
>> +static u32 imx908_calc_max_vblank(u32 height)
>> +{
>> + return IMX908_VMAX_MAX - height;
>> +}
>> +
>> +static int imx908_set_exposure_lines(struct imx908 *imx, u32 exposure_lines)
>> +{
>> + u32 max_lines = imx->vmax - IMX908_MIN_SHR0;
>> +
>> + exposure_lines = clamp_t(u32, exposure_lines, 1, max_lines);
>
>Isn't this clamp already enforced by the control handler limits?
For a plain exposure set it's redundant. But it guards the combined
VBLANK+EXPOSURE set: exposure is validated against the old (larger) VMAX,
so if VBLANK's s_ctrl shrinks VMAX first, SHR0 = VMAX - exposure underflows.
The clamp absorbs that; I added a comment.
>> +
>> + u32 shr0 = imx->vmax - exposure_lines;
>> +
>> + return cci_write(imx->cci, IMX908_REG_SHR0, shr0, NULL);
>> +}
>> +
<snip>
>> +static void imx908_update_vblank_limits(struct imx908 *imx,
>> + const struct v4l2_rect *crop)
>> +{
>> + if (!imx->ctrls.vblank)
>> + return;
>
>Is there actually a path that calls this function before the control
>has been created? I can't immediately see one, but I do know they can
>hide.
You're right, there's no such path. init_controls() sets the initial VBLANK
range inline and never calls update_vblank_limits(); the updater only runs
via update_framing_limits() (init_state/set_fmt/set_selection), all after the
controls exist.
>> +
>> + u32 min_vblank = imx908_calc_min_vblank(crop);
>> + u32 max_vblank = imx908_calc_max_vblank(crop->height);
>> +
>> + __v4l2_ctrl_modify_range(imx->ctrls.vblank, min_vblank, max_vblank, 1,
>> + imx->ctrls.vblank->default_value);
>> +}
>> +
<snip>
>> +
>> +/* -------------------- Mode Building ----------------------- */
>> +static void imx908_mode_build(struct imx908 *imx,
>> + const struct v4l2_mbus_framefmt *fmt,
>> + const struct v4l2_rect *crop)
>> +{
>> + u8 bpp = imx908_bits_per_pixel(fmt->code);
>> +
>> + /* --- H timing (HMAX) --- */
>> + if (v4l2_rect_equal(crop, &imx908_active_area)) {
>> + u16 min_hmax = imx908_calc_min_hmax(imx, fmt->width, bpp);
>> + u32 min_hblank = imx908_hmax_to_hblank(min_hmax, fmt->width);
>> + u32 max_hblank = imx908_hmax_to_hblank(IMX908_HMAX_MAX,
>> + fmt->width);
>> + u32 hblank = clamp_t(u32, imx->ctrls.hblank->val, min_hblank,
>> + max_hblank);
>
>Again, isn't this clamp enforced by the control handler?
This is the set_fmt/set_selection path, not a control write, so the handler's
range check doesn't apply. The hblank range is width-dependent, so on a
width change the stored hblank->val is still validated against the old width
and can be out of range for the new one. We clamp it into the recomputed
range before deriving HMAX (same reason the vblank clamp just below it is
needed).
>> + imx->hmax = imx908_calc_hmax(fmt->width, hblank);
>> + } else if (!imx->hmax) {
>> + /* First crop before any all-pixel config: seed from default */
>> + u32 hblank = imx908_hmax_to_hblank(IMX908_HMAX_DEFAULT,
>> + imx908_active_area.width);
>> +
>> + imx->hmax = imx908_calc_hmax(fmt->width, hblank);
>> + }
>> + /* else: crop with HMAX already fixed by the drive mode - keep it */
>> +
>> + /* --- V timing (VMAX) --- */
>> + u32 min_vblank = imx908_calc_min_vblank(crop);
>
>General query over defining variables in the middle of functions as
>you'll get some reviewers pushing back on it[0].
>In this case you're only using the values once each, so there's little
>need for them at all.
Ok, I've folded them into the clamp (also HMAX) for the next version.
>> + u32 max_vblank = imx908_calc_max_vblank(fmt->height);
>> + u32 vblank = clamp_t(u32, imx->ctrls.vblank->val,
>> + min_vblank, max_vblank);
>> +
>> + imx->vmax = imx908_calc_vmax(fmt->height, vblank);
>> +}
>> +
>> +static void imx908_update_framing_limits(struct imx908 *imx,
>> + struct v4l2_subdev_state *state)
>> +{
>> + const struct v4l2_mbus_framefmt *fmt;
>> + const struct v4l2_rect *crop;
>> +
>> + fmt = v4l2_subdev_state_get_format(state, IMX908_SOURCE_PAD);
>> + crop = v4l2_subdev_state_get_crop(state, IMX908_SOURCE_PAD);
>> + u8 bpp = imx908_bits_per_pixel(fmt->code);
>> +
>> + /* All-pixel: HBLANK drives HMAX. Crop: HMAX fixed, HBLANK derived. */
>> + if (v4l2_rect_equal(crop, &imx908_active_area))
>> + imx908_update_hblank_limits(imx, fmt->width, bpp);
>> + else
>> + imx908_update_hblank_crop(imx, crop->width);
>> +
>> + imx908_update_vblank_limits(imx, crop);
>> + imx908_mode_build(imx, fmt, crop);
>> +
>> + __v4l2_ctrl_modify_range(imx->ctrls.exposure,
>> + IMX908_EXPOSURE_MIN,
>> + imx->vmax - IMX908_MIN_SHR0,
>> + IMX908_EXPOSURE_STEP,
>> + imx->ctrls.exposure->default_value);
>> +}
>> +
>> +/* ----------------------- HW Programming ---------------------- */
>> +
>> +static int imx908_mode_apply(struct imx908 *imx,
>> + const struct v4l2_mbus_framefmt *fmt)
>> +{
>> + int ret = 0;
>> + u8 adbit, mdbit;
>> +
>> + switch (fmt->code) {
>> + case MEDIA_BUS_FMT_SRGGB12_1X12:
>> + adbit = IMX908_ADBIT_12BIT;
>> + mdbit = IMX908_MDBIT_RAW12;
>> + break;
>> +
>> + case MEDIA_BUS_FMT_SRGGB10_1X10:
>> + default:
>> + adbit = IMX908_ADBIT_10BIT;
>> + mdbit = IMX908_MDBIT_RAW10;
>> + break;
>> + }
>> +
>> + cci_write(imx->cci, IMX908_REG_XMASTER, IMX908_CONTROLLER_MODE, &ret);
>> + cci_write(imx->cci, IMX908_REG_ADBIT, adbit, &ret);
>> + cci_write(imx->cci, IMX908_REG_MDBIT, mdbit, &ret);
>> +
>> + /* Tuning registers that need to be changed for RAW10/10-bit */
>> + u8 value = adbit == IMX908_ADBIT_10BIT &&
>> + mdbit == IMX908_MDBIT_RAW10 ? 0x0c : 0x05;
>
>It's impossible for only one of those conditionals to be true as
>they're set above.
>Set "value" from the switch/case above to put it all in one place.
Ok, done for the next version.
>> +
>> + for (unsigned int i = 0; i < ARRAY_SIZE(tuning_regs); ++i)
>> + cci_write(imx->cci, tuning_regs[i], value, &ret);
>> +
>> + cci_write(imx->cci, IMX908_REG_INCK_SEL, imx->inck_sel, &ret);
>> + if (ret) {
>> + dev_err(imx->dev, "INCK register write failed: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + u8 datarate_sel;
>> + u64 link_freq = imx908_link_freqs[imx->link_freq_idx];
>> +
>> + ret = imx908_link_freq_to_datarate_sel(imx, link_freq, &datarate_sel);
>
>Is it possible for this call to fail? Probe has filtered the link
>frequencies using v4l2_link_freq_to_bitmap(), so the value has to
>match one of the entries.
>(Then again I've proposed removing imx908_link_freq_to_datarate_sel() above).
Fixed per your previous suggestion.
>> + cci_write(imx->cci, IMX908_REG_DATARATE_SEL, datarate_sel, &ret);
>> + cci_write(imx->cci, IMX908_REG_LANEMODE, imx->num_lanes - 1, &ret);
>> +
>> + /* Recommended black level offset is 50 in 10-bit, 200 for others */
>> + u16 blklevel = (mdbit == IMX908_MDBIT_RAW10) ? 50 : 200;
>
>Move this assignment into the switch/case too.
Done.
>> +
>> + cci_write(imx->cci, IMX908_REG_BLKLEVEL, blklevel, &ret);
>> +
>> + return ret;
>> +}
>> +
>> +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.
>> +
>> + if (!all_pixel_mode) {
>> + cci_write(imx->cci, IMX908_REG_PIX_HST, crop->left, &ret);
>> + cci_write(imx->cci, IMX908_REG_PIX_HWIDTH, crop->width, &ret);
>> + cci_write(imx->cci, IMX908_REG_PIX_VST, crop->top, &ret);
>> + cci_write(imx->cci, IMX908_REG_PIX_VWIDTH, crop->height, &ret);
>> + }
<snip>
>> +static int imx908_set_selection(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state,
>> + struct v4l2_subdev_selection *sel)
>> +{
>> + struct v4l2_mbus_framefmt *format;
>> + struct v4l2_rect *crop;
>> + struct v4l2_rect *r = &sel->r;
>> + struct imx908 *imx = to_imx908(sd);
>
>One for the maintainers.
>Sakari asked for set_selection to NOT be implemented on imx678 as it
>complicates the common raw sensor model conversion [1], and that was
>then echoed for imx662 [2] (although that is now hopefully going to be
>an extension of imx678).
>Is implementing it acceptable here or not?
IMX908 only does analog window crop, no binning, so I think it's ok in light of
the discussions you raised, that is analog-crop-vs-binning distinction Laurent
and Sakari drew on the os02g10 thread.
(sorry my company mailer mangles links so deleted here)
I will wait for official comments on this.
>> +
>> + if (sel->target != V4L2_SEL_TGT_CROP)
>> + return -EINVAL;
>> +
>> + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE &&
>> + v4l2_subdev_is_streaming(sd))
>> + return -EBUSY;
>> +
>> + crop = v4l2_subdev_state_get_crop(sd_state, sel->pad);
>> + format = v4l2_subdev_state_get_format(sd_state, sel->pad);
>> +
>> + /*
>> + * Round the crop rectangle size down to the hardware alignment
>> + * constraints, then clamp it to the supported size range.
>> + */
>> + r->width = clamp_t(s32, ALIGN_DOWN(r->width, IMX908_CROP_ALIGN_HWIDTH),
>> + IMX908_CROP_MIN_WIDTH, imx908_active_area.width);
>> + r->height = clamp_t(s32, ALIGN_DOWN(r->height, IMX908_CROP_ALIGN_VWIDTH),
>> + IMX908_CROP_MIN_HEIGHT, imx908_active_area.height);
>> +
>> + /*
>> + * Round and clamp the crop position similarly, with the maximum value
>> + * chosen so that the crop rectangle remains fully inside the active
>> + * pixel array.
>> + */
>> + r->left = clamp_t(s32, ALIGN_DOWN(r->left, IMX908_CROP_ALIGN_HSTART), 0,
>> + imx908_active_area.width - r->width);
>> + r->top = clamp_t(s32, ALIGN_DOWN(r->top, IMX908_CROP_ALIGN_VSTART), 0,
>> + imx908_active_area.height - r->height);
>> +
>> + *crop = *r;
>> +
>> + /* IMX908 has no binning, so the output size matches the crop 1:1 */
>> + format->width = crop->width;
>> + format->height = crop->height;
>> +
>> + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>> + imx908_update_framing_limits(imx, sd_state);
>> +
>> + return 0;
>> +}
<snip>
> +
>> +/* ----------------------- Power management ---------------------- */
>> +
>> +static int imx908_power_on(struct imx908 *imx)
>> +{
>> + int ret;
>> +
>> + ret = regulator_bulk_enable(ARRAY_SIZE(imx908_supply_names),
>> + imx->supplies);
>> + if (ret) {
>> + dev_err(imx->dev, "failed to enable regulators\n");
>> + return ret;
>> + }
>> + msleep(200); /* IMX908 power ok after 200ms */
>
>Using fsleep() avoids the debate over which delay function is
>required. Ditto in various other places.
Ok, switched to fsleep() here and for the other delays in the driver.
>> +
>> + if (imx->reset_gpio) {
>> + gpiod_set_value_cansleep(imx->reset_gpio, 1); /* XCLR low */
>> + udelay(1); /* >= 500ns T_low */
>> + gpiod_set_value_cansleep(imx->reset_gpio, 0); /* Sensor start */
>> + }
>> +
>> + ret = clk_prepare_enable(imx->xclk);
>> + if (ret) {
>> + dev_err(imx->dev, "failed to enable xclk: %d\n", ret);
>> + goto err_reset;
>> + }
>> +
>> + /* T_1 >=20us delay before initial SDA/SCL */
>> + usleep_range(20, 25);
>> +
>> + return 0;
>> +
>> +err_reset:
>> + gpiod_set_value_cansleep(imx->reset_gpio, 1); /* assert reset */
>> + regulator_bulk_disable(ARRAY_SIZE(imx908_supply_names), imx->supplies);
>> + return ret;
> +}
>> +
>> +static void imx908_power_off(struct imx908 *imx)
>> +{
>> + gpiod_set_value_cansleep(imx->reset_gpio, 1);
>> + clk_disable_unprepare(imx->xclk);
>
>This may be correct for imx908, but most commonly the power off
>sequence is the reverse of power on. This has swapped the order of
>clock and reset.
Fixed in next version.
>> + regulator_bulk_disable(ARRAY_SIZE(imx908_supply_names), imx->supplies);
>> +}
>> +
<snip>
>> +static int imx908_probe(struct i2c_client *client)
>> +{
>> + struct imx908 *imx;
>> + int ret;
>> +
>> + imx = devm_kzalloc(&client->dev, sizeof(*imx), GFP_KERNEL);
>> + if (!imx)
>> + return -ENOMEM;
>> + imx->dev = &client->dev;
>> +
>> + v4l2_i2c_subdev_init(&imx->sd, client, &imx908_subdev_ops);
>> + imx->sd.internal_ops = &imx908_internal_ops;
>> +
>> + imx->cci = devm_cci_regmap_init_i2c(client, 16);
>> + if (IS_ERR(imx->cci))
>> + return dev_err_probe(&client->dev, PTR_ERR(imx->cci),
>> + "CCI regmap init failed\n");
>> +
>> + imx->xclk = devm_clk_get(imx->dev, NULL);
>> + if (IS_ERR(imx->xclk))
>> + return dev_err_probe(imx->dev, PTR_ERR(imx->xclk),
>> + "failed to get clock\n");
>> +
>> + ret = imx908_get_inck_sel(imx, clk_get_rate(imx->xclk));
>> + if (ret)
>> + return ret;
>> +
>> + imx->reset_gpio = devm_gpiod_get_optional(imx->dev, "reset",
>> + GPIOD_OUT_HIGH);
>> +
>> + if (IS_ERR(imx->reset_gpio))
>> + return dev_err_probe(imx->dev, PTR_ERR(imx->reset_gpio),
>> + "failed to get reset gpio\n");
>> +
>> + ret = imx908_get_regulators(imx);
>> + if (ret)
>> + return dev_err_probe(&client->dev, ret,
>> + "failed to get regulators\n");
>> +
>> + ret = imx908_parse_fwnode(imx);
>> + if (ret)
>> + return dev_err_probe(&client->dev, ret,
>> + "device tree parse failed\n");
>> +
>> + ret = imx908_power_on(imx);
>> + if (ret)
>> + return dev_err_probe(&client->dev, ret, "power-on failed\n");
>> +
>> + ret = imx908_identify_model(imx);
>> + if (ret) {
>> + dev_err(imx->dev, "failed to identify model: %d\n", ret);
>> + goto err_power_off;
>> + }
>> +
>> + pm_runtime_set_active(imx->dev);
>> + pm_runtime_enable(imx->dev);
>> +
>> + ret = imx908_init_controls(imx);
>> + if (ret)
>> + goto err_pm_disable;
>> +
>> + imx->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>> + imx->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>> + imx->pad.flags = MEDIA_PAD_FL_SOURCE;
>> + ret = media_entity_pads_init(&imx->sd.entity, 1, &imx->pad);
>> + if (ret)
>> + goto err_hdl;
>> +
>> + /* Share the ctrl handler lock so s_ctrl can access the locked state */
>> + imx->sd.state_lock = imx->ctrls.handler.lock;
>> +
>> + ret = v4l2_subdev_init_finalize(&imx->sd);
>> + if (ret)
>> + goto err_entity;
>> +
>> + pm_runtime_set_autosuspend_delay(imx->dev, 1000);
>> + pm_runtime_use_autosuspend(imx->dev);
>
>Ideally this should be cancelled with
>pm_runtime_dont_use_autosuspend() on error or remove.
Ok, I think I'll move to devm_pm_runtime_enable() so the manual
pm_runtime_disable and the dont_use_autosuspend cleanup both go away.
>> + pm_runtime_mark_last_busy(imx->dev);
>> +
>> + ret = v4l2_async_register_subdev_sensor(&imx->sd);
>> + if (ret)
>> + goto err_subdev;
>
>I think this has left the sensor powered up. Don't we want a
> pm_runtime_idle(imx->dev);
>here?
Ok. I've reworked the probe PM. Probe now takes a
pm_runtime_get_noresume() ref after set_active and balances it with
pm_runtime_put_autosuspend() after registration, so the sensor
autosuspends instead of staying on.
Thanks,
Lachlan