Re: [PATCH v3 2/2] media: i2c: imx678: Add driver for Sony IMX678

[Tarang Raval]

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.

> + * 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.

> +}
> +
> +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.

> +       }
> +
> +       /*
> +        * 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.

> +               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

> +       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.

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/

> +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.

> +               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