Re: [PATCH v3 2/2] media: i2c: imx678: Add driver for Sony IMX678
From: Jai Luthra
Date: Mon May 25 2026 - 10:08:38 EST
Hi Tarang,
Quoting Tarang Raval (2026-05-22 19:01:01)
> Hi Jai,
>
> I noticed a few issues and also have one question. Could you please help me
> understand that part?
>
> Please check the comments below.
>
> > Add a V4L2 subdev driver for the Sony IMX678 image sensor.
> >
> > IMX678 is a diagonal 8.86 mm (Type 1/1.8) CMOS active pixel type
> > solid-state image sensor with a square pixel array and 8.40 M effective
> > pixels.
> >
> > The following features are supported by the driver:
> > - Monochrome and Color (Bayer filter) variants
> > - Multiple input clock frequencies supported
> > - Multiple link frequencies supported
> > - VBLANK and HBLANK control for variable framerate
> > - Freely configurable crop rectangle through S_SELECTION ioctl
> > - Configurable resolution with 2x2 binning (for the current crop)
> > through S_FMT ioctl
> > - VFLIP and HFLIP control for flipping readout
> > - Test pattern control support
> > - Exposure and gain control
> > - MIPI RAW12 output
> >
> > Following features are not currently supported but may be added later:
> > - Pixel-perfect crop reporting, account for the shift-by-1 when flipping
> > using HFLIP/VFLIP, which maintains the bayer readout order
> > - Increased framerate (lower HMAX/VMAX) when cropping
> > - MIPI RAW10 output mode
> > - Embedded data stream
> >
> > Signed-off-by: Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx>
> > ---
>
> ...
>
> > +#define IMX678_REG_INCK_SEL CCI_REG8(0x3014)
> > +
> > +/* Link Speed */
> > +#define IMX678_REG_DATARATE_SEL CCI_REG8(0x3015)
> > +
> > +/* Lane Count */
> > +#define IMX678_REG_LANEMODE CCI_REG8(0x3040)
> > +
> > +/*
> > + * The internal readout clock runs at 74.25 Hz. In one cycle the AD reads 8
>
> I think it's 74.25 MHz.
Argh, yes.
>
> > + * pixels, thus giving us a rate of 74.25 * 8 = 594 MPix/s
> > + */
> > +#define IMX678_PIXEL_RATE 594000000
> > +#define IMX678_PIX_PER_CLK 8
> > +
> > +/* VMAX - Frame Length in Lines */
> > +#define IMX678_REG_VMAX CCI_REG24_LE(0x3028)
> > +#define IMX678_VMAX_MAX 0xfffff
> > +#define IMX678_VMAX_DEFAULT 2250
>
> ...
>
> > +static const int imx678_tpg_val[] = {
> > + IMX678_TPG_ALL_000,
> > + IMX678_TPG_ALL_000,
> > + IMX678_TPG_ALL_FFF,
> > + IMX678_TPG_ALL_555,
> > + IMX678_TPG_ALL_AAA,
> > + IMX678_TPG_TOG_555_AAA,
> > + IMX678_TPG_TOG_AAA_555,
> > + IMX678_TPG_TOG_000_555,
> > + IMX678_TPG_TOG_555_000,
> > + IMX678_TPG_TOG_000_FFF,
> > + IMX678_TPG_TOG_FFF_000,
> > + IMX678_TPG_H_COLOR_BARS,
> > + IMX678_TPG_V_COLOR_BARS,
> > +};
> > +
> > +/* IMX678 Register List */
> > +/* Common Modes */
>
> You can remove these comments or keep only one of them.
>
> > +static const struct cci_reg_sequence common_regs[] = {
> > + {IMX678_REG_THIN_V_EN, 0x00},
> > + {IMX678_REG_VCMODE, 0x01},
> > + {CCI_REG8(0x306B), 0x00},
> > + {IMX678_REG_GAIN_PGC_FIDMD, 0x01},
> > + {CCI_REG8(0x3460), 0x22},
> > + {CCI_REG8(0x355A), 0x64},
>
> ...
>
> > +static void imx678_set_framing_limits(struct imx678 *imx678,
> > + struct v4l2_subdev_state *state)
> > +{
> > + const struct v4l2_mbus_framefmt *format = imx678_state_format(state);
> > + s64 min_hblank, default_hblank, max_hblank, vblank;
> > + const u32 hmax_4lane = min_hmax_4lane[__ffs(imx678->link_freq_bitmap)];
> > + const u32 lane_scale = imx678->lane_mode == IMX678_LANEMODE_2L ? 2 : 1;
> > + const bool binning = imx678_state_binning(state);
> > + const u8 bpp = binning ? 10 : 12;
> > + u32 hmax, min_hmax;
> > +
> > + imx678->vmax = IMX678_VMAX_DEFAULT;
> > + hmax = hmax_4lane * lane_scale;
> > +
> > + /* HMAX can go lower when using 10bit AD for binning */
> > + min_hmax = (hmax * bpp) / 12;
> > + min_hblank = min_hmax * IMX678_PIX_PER_CLK - format->width;
> > + default_hblank = hmax * IMX678_PIX_PER_CLK - format->width;
> > + max_hblank = IMX678_HMAX_MAX * IMX678_PIX_PER_CLK - format->width;
> > +
> > + __v4l2_ctrl_modify_range(imx678->hblank, min_hblank, max_hblank,
> > + IMX678_PIX_PER_CLK, default_hblank);
> > + __v4l2_ctrl_s_ctrl(imx678->hblank, default_hblank);
> > +
> > + vblank = imx678->vmax - format->height;
> > + __v4l2_ctrl_modify_range(imx678->vblank, vblank,
> > + IMX678_VMAX_MAX - format->height, 2, vblank);
> > + __v4l2_ctrl_s_ctrl(imx678->vblank, IMX678_VMAX_DEFAULT - format->height);
> > +
> > + __v4l2_ctrl_modify_range(imx678->exposure, IMX678_EXPOSURE_MIN,
> > + imx678->vmax - IMX678_SHR_MIN, 1,
> > + IMX678_EXPOSURE_DEFAULT);
>
> This control operation can fail, so please check the error value.
>
> Also, return the error by changing the return type accordingly.
>
Ack.
> > +}
> > +
> > +static int imx678_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct imx678 *imx678 = container_of(ctrl->handler, struct imx678, ctrl_handler);
> > + struct v4l2_subdev_state *state;
> > + struct i2c_client *client = v4l2_get_subdevdata(&imx678->sd);
> > + const struct v4l2_mbus_framefmt *format;
> > + int ret = 0;
> > +
> > + state = v4l2_subdev_get_locked_active_state(&imx678->sd);
> > + format = imx678_state_format(state);
> > +
> > + if (ctrl->id == V4L2_CID_VBLANK) {
> > + u32 current_exposure = imx678->exposure->cur.val;
> > +
> > + imx678->vmax = format->height + ctrl->val;
> > +
> > + current_exposure = clamp_t(u32, current_exposure, IMX678_EXPOSURE_MIN,
> > + imx678->vmax - IMX678_SHR_MIN);
> > + __v4l2_ctrl_modify_range(imx678->exposure, IMX678_EXPOSURE_MIN,
> > + imx678->vmax - IMX678_SHR_MIN, 1,
> > + current_exposure);
>
> Same here, please check the error value.
Ack.
>
> > + }
> > +
> > + /*
> > + * Applying V4L2 control value only happens
> > + * when power is up for streaming
> > + */
> > + if (pm_runtime_get_if_in_use(&client->dev) == 0)
>
> Use pm_runtime_get_if_active.
>
We anyway write all control values everytime in enable_streams(), so I
think it's okay to keep this check a bit strict and skip the writes if the
sensor is RPM_ACTIVE with 0 users. (i.e. streaming has stopped, but device
not suspended yet, which is unlikely given we don't have an autosuspend
timer but suspend immediately here)
Unless of course I misunderstood why you're suggesting it?
> > + return 0;
> > +
> > + switch (ctrl->id) {
> > + case V4L2_CID_VBLANK:
> > + cci_write(imx678->cci, IMX678_REG_VMAX, imx678->vmax, &ret);
> > + fallthrough; /* SHR = VMAX - exposure, so update it */
> > + case V4L2_CID_EXPOSURE: {
> > + u32 shr = imx678->vmax - imx678->exposure->val;
> > +
> > + cci_write(imx678->cci, IMX678_REG_SHR, shr, &ret);
> > + break;
> > + }
> > + case V4L2_CID_ANALOGUE_GAIN:
> > + cci_write(imx678->cci, IMX678_REG_ANALOG_GAIN, ctrl->val, &ret);
> > + break;
> > + case V4L2_CID_HBLANK: {
> > + u32 hmax = (format->width + ctrl->val) / IMX678_PIX_PER_CLK;
> > +
> > + cci_write(imx678->cci, IMX678_REG_HMAX, hmax, &ret);
> > + break;
> > + }
> > + case V4L2_CID_TEST_PATTERN: {
> > + cci_write(imx678->cci, IMX678_REG_TPG_COLORWIDTH,
> > + IMX678_TPG_COLORWIDTH_160PIX, &ret);
> > + cci_write(imx678->cci, IMX678_REG_TPG_PATSEL_DUOUT,
> > + imx678_tpg_val[ctrl->val], &ret);
> > + cci_write(imx678->cci, IMX678_REG_TPG_EN_DUOUT, (ctrl->val) ? 1 : 0,
> > + &ret);
> > + break;
> > + }
> > + case V4L2_CID_HFLIP:
> > + cci_write(imx678->cci, IMX678_REG_WINMODEH, ctrl->val, &ret);
> > + break;
> > + case V4L2_CID_VFLIP:
> > + cci_write(imx678->cci, IMX678_REG_WINMODEV, ctrl->val, &ret);
> > + break;
> > + default:
> > + dev_warn(&client->dev,
> > + "ctrl(id:0x%x,val:0x%x) is not handled\n",
> > + ctrl->id, ctrl->val);
> > + break;
> > + }
> > +
> > + pm_runtime_put(&client->dev);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops imx678_ctrl_ops = {
> > + .s_ctrl = imx678_set_ctrl,
> > +};
>
> ...
>
> > +static int imx678_set_selection(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *sd_state,
> > + struct v4l2_subdev_selection *sel)
> > +{
> > + struct imx678 *imx678 = to_imx678(sd);
> > + struct v4l2_rect *crop;
> > + struct v4l2_rect rect;
> > +
> > + if (sel->target != V4L2_SEL_TGT_CROP || sel->pad != 0)
> > + return -EINVAL;
> > +
> > + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE &&
> > + v4l2_subdev_is_streaming(sd))
> > + return -EBUSY;
> > +
> > + /* Align left, top to 4 */
> > + rect.left = clamp_t(s32, ALIGN(sel->r.left, IMX678_CROP_HST_ALIGN),
> > + imx678_active_area.left,
> > + imx678_active_area.width - IMX678_PIXEL_ARRAY_MIN_WIDTH);
>
> You are ignoring the active_area offset here; please correct it.
>
> In imx296, crop bounds start at (0, 0), so no offset handling is needed there.
>
> You can refer to my patch:
> https://lore.kernel.org/linux-media/20260424092554.26130-4-elgin.perumbilly@xxxxxxxxxxxxxxxxx/#t
>
Ah good catch, will fix.
> > + rect.top = clamp_t(s32, ALIGN(sel->r.top, IMX678_CROP_VST_ALIGN),
> > + imx678_active_area.top,
> > + imx678_active_area.height - IMX678_PIXEL_ARRAY_MIN_HEIGHT);
> > + /* Align width to 16 and height to 4 */
> > + rect.width = clamp_t(u32, ALIGN(sel->r.width, IMX678_CROP_HWIDTH_ALIGN),
> > + IMX678_PIXEL_ARRAY_MIN_WIDTH, imx678_active_area.width);
> > + rect.height = clamp_t(u32, ALIGN(sel->r.height, IMX678_CROP_VWIDTH_ALIGN),
> > + IMX678_PIXEL_ARRAY_MIN_HEIGHT, imx678_active_area.height);
> > +
> > + rect.width = min_t(u32, rect.width, imx678_native_area.width - rect.left);
> > + rect.height = min_t(u32, rect.height, imx678_native_area.height - rect.top);
> > +
> > + crop = v4l2_subdev_state_get_crop(sd_state, sel->pad);
> > +
> > + if (rect.width != crop->width || rect.height != crop->height) {
> > + struct v4l2_mbus_framefmt *format =
> > + v4l2_subdev_state_get_format(sd_state, sel->pad);
> > + format->width = rect.width;
> > + format->height = rect.height;
>
> Why are we not checking here whether binning mode is currently enabled?
>
> Suppose binning mode is enabled, and then userspace changes the crop.
>
> With the below lines:
>
> format->width = rect.width;
> format->height = rect.height;
>
> the format size becomes equal to the crop size, which silently disables binning.
This was intentional.
Let's say I'm streaming in 640x480 binned mode (1280x960 crop) and want to
switch to 3200x1600, I prefer if a single S_SELECTION call with 3200x1600
crop size to do that directly.
On the other hand, if I modify just the (top, left) of my crop without
changing its size (1280x960), I want it to stay in the binned mode and not
snap back the active format to full size.
This is a matter of opinion though, which I hope would become irrelevant
once we move to the new raw sensor model with explicit controls for
binning. I plan to look into it before I post a v4, as Sakari suggested in
his review.
>
> Am I missing something here?
>
> > + }
> > +
> > + *crop = rect;
> > + sel->r = *crop;
> > +
> > + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > + imx678_set_framing_limits(imx678, sd_state);
> > +
> > + return 0;
> > +}
> > +
> > +static int imx678_init_state(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *state)
> > +{
> > + struct imx678 *imx678 = to_imx678(sd);
> > + struct v4l2_subdev_selection sel = {
> > + .which = V4L2_SUBDEV_FORMAT_TRY,
> > + .target = V4L2_SEL_TGT_CROP,
> > + .r = imx678_active_area,
> > + };
> > + struct v4l2_subdev_format fmt = {
> > + .which = V4L2_SUBDEV_FORMAT_TRY,
> > + .pad = 0,
> > + .format = {
> > + .code = imx678_default_mbus_code(imx678),
> > + .width = imx678_active_area.width,
> > + .height = imx678_active_area.height,
> > + },
> > + };
> > +
> > + imx678_set_selection(sd, state, &sel);
> > + imx678_set_pad_format(sd, state, &fmt);
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static int imx678_enable_streams(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *state, u32 pad,
> > + u64 mask)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(sd);
> > + struct imx678 *imx678 = to_imx678(sd);
> > + const struct v4l2_rect *crop = imx678_state_crop(state);
> > + const bool binning = imx678_state_binning(state);
> > + int ret = 0;
>
> You can omit the initialization here.
>
> > +
> > + ret = pm_runtime_resume_and_get(&client->dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = imx678_program_window(imx678, crop, binning);
> > + if (ret) {
> > + dev_err(&client->dev, "%s failed to set mode\n", __func__);
> > + goto err_rpm_put;
> > + }
> > +
> > + ret = __v4l2_ctrl_handler_setup(imx678->sd.ctrl_handler);
> > + if (ret) {
> > + dev_err(&client->dev, "%s failed to apply user values\n", __func__);
> > + goto err_rpm_put;
> > + }
> > +
> > + cci_write(imx678->cci, IMX678_REG_MODE_SELECT, IMX678_MODE_STREAMING, &ret);
> > + usleep_range(IMX678_STREAM_DELAY_US, IMX678_STREAM_DELAY_US +
> > + IMX678_STREAM_DELAY_RANGE_US);
> > + cci_write(imx678->cci, IMX678_REG_XMSTA, 0x00, &ret);
> > +
> > + if (ret) {
> > + dev_err(&client->dev, "%s failed to start streaming\n", __func__);
> > + goto err_rpm_put;
> > + }
> > +
> > + return 0;
> > +
> > +err_rpm_put:
> > + pm_runtime_put(&client->dev);
> > +
> > + return ret;
> > +}
>
> ...
>
> > +static const struct v4l2_subdev_core_ops imx678_core_ops = {
> > + .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> > + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > +};
>
> Drop this
>
> See: https://lore.kernel.org/linux-media/20241029162106.3005800-1-tomm.merciai@xxxxxxxxx/
>
Ack.
> > +static const struct v4l2_subdev_video_ops imx678_video_ops = {
> > + .s_stream = v4l2_subdev_s_stream_helper,
> > +};
> > +
> > +static const struct v4l2_subdev_pad_ops imx678_pad_ops = {
> > + .enum_mbus_code = imx678_enum_mbus_code,
> > + .get_fmt = v4l2_subdev_get_fmt,
> > + .set_fmt = imx678_set_pad_format,
> > + .get_selection = imx678_get_selection,
> > + .set_selection = imx678_set_selection,
> > + .enum_frame_size = imx678_enum_frame_size,
> > + .enable_streams = imx678_enable_streams,
> > + .disable_streams = imx678_disable_streams,
> > +};
>
> ...
>
> > +static int imx678_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct imx678 *imx678;
> > + int ret, i;
> > +
> > + imx678 = devm_kzalloc(&client->dev, sizeof(*imx678), GFP_KERNEL);
> > + if (!imx678)
> > + return -ENOMEM;
> > +
> > + v4l2_i2c_subdev_init(&imx678->sd, client, &imx678_subdev_ops);
> > +
> > + imx678->cci = devm_cci_regmap_init_i2c(client, 16);
> > + if (IS_ERR(imx678->cci))
> > + return dev_err_probe(dev, PTR_ERR(imx678->cci),
> > + "failed to init CCI\n");
> > +
> > + if (imx678_check_hwcfg(dev, imx678))
> > + return -EINVAL;
> > +
> > + imx678->xclk = devm_v4l2_sensor_clk_get(dev, NULL);
> > + if (IS_ERR(imx678->xclk))
> > + return dev_err_probe(dev, PTR_ERR(imx678->xclk),
> > + "failed to get xclk\n");
> > +
> > + imx678->xclk_freq = clk_get_rate(imx678->xclk);
> > +
> > + for (i = 0; i < ARRAY_SIZE(imx678_inck_table); ++i) {
> > + if (imx678_inck_table[i].xclk_hz == imx678->xclk_freq) {
> > + imx678->inck_sel_val = imx678_inck_table[i].inck_sel;
> > + break;
> > + }
> > + }
> > +
> > + if (i == ARRAY_SIZE(imx678_inck_table))
> > + return dev_err_probe(dev, -EINVAL,
> > + "unsupported XCLK rate %u Hz\n",
> > + imx678->xclk_freq);
> > +
> > + ret = imx678_get_regulators(imx678);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to get regulators\n");
> > +
> > + imx678->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > + GPIOD_OUT_HIGH);
> > + if (IS_ERR(imx678->reset_gpio))
> > + return dev_err_probe(dev, PTR_ERR(imx678->reset_gpio),
> > + "failed to get reset GPIO\n");
> > +
> > + ret = imx678_power_on(dev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = imx678_identify_model(imx678);
> > + if (ret)
> > + goto error_power_off;
> > +
> > + pm_runtime_set_active(dev);
> > + pm_runtime_enable(dev);
> > +
> > + ret = imx678_init_controls(imx678);
> > + if (ret)
> > + goto error_pm_runtime;
> > +
> > + imx678->sd.internal_ops = &imx678_internal_ops;
> > + imx678->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > + V4L2_SUBDEV_FL_HAS_EVENTS;
> > + imx678->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > +
> > + imx678->pad.flags = MEDIA_PAD_FL_SOURCE;
> > +
> > + ret = media_entity_pads_init(&imx678->sd.entity, 1, &imx678->pad);
> > + if (ret) {
> > + dev_err(dev, "failed to init entity pads: %d\n", ret);
>
> Use dev_err_probe.
>
> > + goto error_handler_free;
> > + }
> > +
> > + imx678->sd.state_lock = imx678->ctrl_handler.lock;
> > + ret = v4l2_subdev_init_finalize(&imx678->sd);
> > + if (ret < 0) {
> > + dev_err(dev, "subdev init error\n");
>
> Use dev_err_probe.
>
> > + goto error_media_entity;
> > + }
> > +
> > + ret = v4l2_async_register_subdev_sensor(&imx678->sd);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
>
> Use dev_err_probe.
>
Ack.
Thanks,
Jai
> > + goto error_subdev_cleanup;
> > + }
> > +
> > + pm_runtime_idle(dev);
> > +
> > + return 0;
> > +
> > +error_subdev_cleanup:
> > + v4l2_subdev_cleanup(&imx678->sd);
> > +
> > +error_media_entity:
> > + media_entity_cleanup(&imx678->sd.entity);
> > +
> > +error_handler_free:
> > + imx678_free_controls(imx678);
> > +
> > +error_pm_runtime:
> > + pm_runtime_disable(&client->dev);
> > + pm_runtime_set_suspended(&client->dev);
> > +
> > +error_power_off:
> > + imx678_power_off(&client->dev);
> > +
> > + return ret;
> > +}
> > +
> > +static void imx678_remove(struct i2c_client *client)
> > +{
> > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > + struct imx678 *imx678 = to_imx678(sd);
> > +
> > + v4l2_async_unregister_subdev(sd);
> > + v4l2_subdev_cleanup(sd);
> > + media_entity_cleanup(&sd->entity);
> > + imx678_free_controls(imx678);
> > +
> > + pm_runtime_disable(&client->dev);
> > + if (!pm_runtime_status_suspended(&client->dev))
> > + imx678_power_off(&client->dev);
> > + pm_runtime_set_suspended(&client->dev);
> > +}
> > +
> > +static const struct dev_pm_ops imx678_pm_ops = {
> > + SET_RUNTIME_PM_OPS(imx678_power_off, imx678_power_on, NULL)
> > +};
> > +
> > +static const struct of_device_id imx678_of_match[] = {
> > + { .compatible = "sony,imx678" },
> > + { .compatible = "sony,imx678-aamr", .data = &imx678_aamr_info },
> > + { .compatible = "sony,imx678-aaqr", .data = &imx678_aaqr_info },
> > + { /* sentinel */ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, imx678_of_match);
> > +
> > +static struct i2c_driver imx678_i2c_driver = {
> > + .driver = {
> > + .name = "imx678",
> > + .of_match_table = imx678_of_match,
> > + .pm = &imx678_pm_ops,
> > + },
> > + .probe = imx678_probe,
> > + .remove = imx678_remove,
> > +};
> > +
> > +module_i2c_driver(imx678_i2c_driver);
> > +
> > +MODULE_AUTHOR("Will Whang <will@xxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Tetsuya NOMURA <tetsuya.nomura@xxxxxxxxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Sony imx678 sensor driver");
> > +MODULE_LICENSE("GPL");
> >
> > --
> > 2.54.0
>
> Best Regards,
> Tarang