Message-ID:
 <PN3P287MB182950EC8691183FBFC4EC098BFBA@PN3P287MB1829.INDP287.PROD.OUTLOOK.COM>
Date: Thu, 30 Oct 2025 14:55:00 +0000
From: Tarang Raval <tarang.raval@...iconsignals.io>
To: Svyatoslav Ryhel <clamor95@...il.com>, Mauro Carvalho Chehab
	<mchehab@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Sakari Ailus
	<sakari.ailus@...ux.intel.com>, Hans Verkuil <hverkuil@...all.nl>, Hans de
 Goede <hansg@...nel.org>, André Apitzsch <git@...tzsch.eu>,
	Sylvain Petinot <sylvain.petinot@...s.st.com>, Benjamin Mugnier
	<benjamin.mugnier@...s.st.com>, Dongcheng Yan <dongcheng.yan@...el.com>,
	Heimir Thor Sverrisson <heimir.sverrisson@...il.com>
CC: "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor
 driver

Hi Svyatoslav,

> Add a v4l2 sub-device driver for the Sony IMX111 image sensor. This is a
> camera sensor using the i2c bus for control and the csi-2 bus for data.
> 
> The following features are supported:
> - manual exposure, digital and analog gain control support
> - pixel rate/link freq control support
> - supported resolution up to 3280x2464 for single shot capture
> - supported resolution up to 1920x1080 @ 30fps for video
> - supported bayer order output SGBRG10 and SGBRG8
> 
> Camera module seems to be partially compatible with Nokia SMIA but it
> lacks a few registers required for clock calculations and has different
> vendor-specific per-mode configurations which makes it incompatible with
> existing CCS driver.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@...il.com>

---

> +static int imx111_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +   struct imx111 *sensor = ctrl_to_imx111(ctrl);
> +   struct device *dev = regmap_get_device(sensor->regmap);
> +   s64 max;
> +   int ret = 0;
> +
> +   /* Propagate change of current control to all related controls */
> +   switch (ctrl->id) {

Do we need the switch statement, since only one case is present? 
You can use an 'if' instead. 

> +   case V4L2_CID_VBLANK:
> +         /* Update max exposure while meeting expected vblanking */
> +         max = sensor->cur_mode->height + ctrl->val - 5;

You can define a macro for the value 5 to improve readability.
Also, make this change in the init_control function. 

> +         __v4l2_ctrl_modify_range(sensor->exposure,
> +                            sensor->exposure->minimum,
> +                            max, sensor->exposure->step, max);

This may fail; consider adding an error check.

> +         break;
> +   }
> +
> +   /*
> +    * Applying V4L2 control value only happens
> +    * when power is up for streaming
> +    */
> +   if (!pm_runtime_get_if_in_use(dev))
> +         return 0;
> +
> +   switch (ctrl->id) {
> +   case V4L2_CID_ANALOGUE_GAIN:
> +         cci_write(sensor->regmap, IMX111_REG_ANALOG_GAIN, ctrl->val, &ret);
> +         break;
> +   case V4L2_CID_DIGITAL_GAIN:
> +         ret = imx111_update_digital_gain(sensor, ctrl->val);
> +         break;
> +   case V4L2_CID_EXPOSURE:
> +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> +                     IMX111_GROUP_WRITE_ON, &ret);
> +         cci_write(sensor->regmap, IMX111_INTEGRATION_TIME, ctrl->val, &ret);
> +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> +                     0, &ret);
> +         break;
> +   case V4L2_CID_HBLANK:
> +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> +                     IMX111_GROUP_WRITE_ON, &ret);
> +         dev_err(dev, "writing 0x%x to HTL\n", sensor->cur_mode->width + ctrl->val);
> +         cci_write(sensor->regmap, IMX111_HORIZONTAL_TOTAL_LENGTH,
> +                 sensor->cur_mode->width + ctrl->val, &ret);
> +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> +                     0, &ret);
> +         break;
> +   case V4L2_CID_VBLANK:
> +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> +                     IMX111_GROUP_WRITE_ON, &ret);
> +         dev_err(dev, "writing 0x%x to VTL\n", sensor->cur_mode->height + ctrl->val);
> +         cci_write(sensor->regmap, IMX111_VERTICAL_TOTAL_LENGTH,
> +                 sensor->cur_mode->height + ctrl->val, &ret);
> +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> +                     0, &ret);
> +         break;
> +   case V4L2_CID_HFLIP:
> +   case V4L2_CID_VFLIP:
> +         cci_write(sensor->regmap, IMX111_IMAGE_ORIENTATION,
> +                 sensor->hflip->val | sensor->vflip->val << 1, &ret);
> +         break;
> +   case V4L2_CID_TEST_PATTERN:
> +         cci_write(sensor->regmap, IMX111_TEST_PATTERN, ctrl->val, &ret);
> +         break;
> +   case V4L2_CID_TEST_PATTERN_RED:
> +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> +                     IMX111_GROUP_WRITE_ON, &ret);
> +         cci_write(sensor->regmap, IMX111_SOLID_COLOR_RED, ctrl->val, &ret);
> +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> +                     0, &ret);
> +         break;
> +   case V4L2_CID_TEST_PATTERN_GREENR:
> +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> +                     IMX111_GROUP_WRITE_ON, &ret);
> +         cci_write(sensor->regmap, IMX111_SOLID_COLOR_GR, ctrl->val, &ret);
> +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> +                     0, &ret);
> +         break;
> +   case V4L2_CID_TEST_PATTERN_BLUE:
> +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> +                     IMX111_GROUP_WRITE_ON, &ret);
> +         cci_write(sensor->regmap, IMX111_SOLID_COLOR_BLUE, ctrl->val, &ret);
> +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> +                     0, &ret);
> +         break;
> +   case V4L2_CID_TEST_PATTERN_GREENB:
> +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> +                     IMX111_GROUP_WRITE_ON, &ret);
> +         cci_write(sensor->regmap, IMX111_SOLID_COLOR_GB, ctrl->val, &ret);
> +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> +                     0, &ret);
> +         break;
> +   default:
> +         ret = -EINVAL;
> +   }
> +
> +   pm_runtime_put(dev);
> +
> +   return ret;
> +}

---

> +static int imx111_init_controls(struct imx111 *sensor)
> +{
> +   const struct v4l2_ctrl_ops *ops = &imx111_ctrl_ops;
> +   struct device *dev = regmap_get_device(sensor->regmap);
> +   const struct imx111_mode *mode = sensor->cur_mode;
> +   struct v4l2_fwnode_device_properties props;
> +   struct v4l2_subdev *sd = &sensor->sd;

No need for a new variable; there is only one user in the function.

> +   struct v4l2_ctrl_handler *hdl = &sensor->hdl;
> +   s64 pixel_rate_min, pixel_rate_max;
> +   int i, ret;
> +
> +   ret = v4l2_fwnode_device_parse(dev, &props);
> +   if (ret < 0)
> +         return ret;
> +
> +   ret = v4l2_ctrl_handler_init(hdl, 13);

Now there are 15 controls. 

No need for explicit error checking; you can omit the error check if you'd like.

> +   if (ret)
> +         return ret;
> +
> +   pixel_rate_min = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW10);
> +   pixel_rate_max = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW8);
> +   sensor->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE,
> +                                  pixel_rate_min, pixel_rate_max,
> +                                  1, div_u64(sensor->pixel_clk_raw,
> +                                  2 * sensor->data_depth));
> +
> +   sensor->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ,
> +                                    0, 0, &sensor->default_link_freq);
> +   if (sensor->link_freq)
> +         sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +   v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> +                 IMX111_ANA_GAIN_MIN, IMX111_ANA_GAIN_MAX,
> +                 IMX111_ANA_GAIN_STEP, IMX111_ANA_GAIN_DEFAULT);
> +
> +   v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN,
> +                 IMX111_DGTL_GAIN_MIN, IMX111_DGTL_GAIN_MAX,
> +                 IMX111_DGTL_GAIN_STEP, IMX111_DGTL_GAIN_DEFAULT);
> +
> +   sensor->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
> +   if (sensor->hflip)
> +         sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> +   sensor->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
> +   if (sensor->vflip)
> +         sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> +   sensor->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, IMX111_VBLANK_MIN,
> +                              IMX111_VTL_MAX - mode->height, 1,
> +                              mode->vtl_def - mode->height);
> +   sensor->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, IMX111_HBLANK_MIN,
> +                              IMX111_HTL_MAX - mode->width, 1,
> +                              mode->htl_def - mode->width);
> +
> +   /*
> +    * The maximum coarse integration time is the frame length in lines
> +    * minus five.
> +    */
> +   sensor->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> +                                IMX111_INTEGRATION_TIME_MIN,
> +                                IMX111_PIXEL_ARRAY_HEIGHT - 5,
> +                                IMX111_INTEGRATION_TIME_STEP,
> +                                IMX111_PIXEL_ARRAY_HEIGHT - 5);
> +
> +   v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
> +
> +   v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
> +                          ARRAY_SIZE(test_pattern_menu) - 1, 0, 0,
> +                          test_pattern_menu);
> +   for (i = 0; i < 4; i++) {
> +         /*
> +          * The assumption is that
> +          * V4L2_CID_TEST_PATTERN_GREENR == V4L2_CID_TEST_PATTERN_RED + 1
> +          * V4L2_CID_TEST_PATTERN_BLUE   == V4L2_CID_TEST_PATTERN_RED + 2
> +          * V4L2_CID_TEST_PATTERN_GREENB == V4L2_CID_TEST_PATTERN_RED + 3
> +          */
> +         v4l2_ctrl_new_std(hdl, ops, V4L2_CID_TEST_PATTERN_RED + i,
> +                       IMX111_TESTP_COLOUR_MIN, IMX111_TESTP_COLOUR_MAX,
> +                       IMX111_TESTP_COLOUR_STEP, IMX111_TESTP_COLOUR_MAX);
> +         /* The "Solid color" pattern is white by default */
> +   }
> +
> +   if (hdl->error)
> +         return hdl->error;
> +
> +   sd->ctrl_handler = hdl;
> +
> +   return 0;
> +};

---

> +static int imx111_initialize(struct imx111 *sensor)
> +{
> +   struct device *dev = regmap_get_device(sensor->regmap);
> +   int ret;

ret = 0;

> +
> +   /* Configure the PLL. */
> +   cci_write(sensor->regmap, IMX111_PRE_PLL_CLK_DIVIDER_PLL1,
> +           sensor->pll->pre_div, &ret);
> +   cci_write(sensor->regmap, IMX111_PLL_MULTIPLIER_PLL1, sensor->pll->mult, &ret);
> +   cci_write(sensor->regmap, IMX111_POST_DIVIDER, IMX111_POST_DIVIDER_DIV1, &ret);
> +   cci_write(sensor->regmap, IMX111_PLL_SETTLING_TIME,
> +           to_settle_delay(sensor->pll->extclk_rate), &ret);
> +
> +   ret = cci_multi_reg_write(sensor->regmap, imx111_global_init,
> +                       ARRAY_SIZE(imx111_global_init), NULL);

You are overwriting the previous errors.

please use ret |= 

> +   if (ret < 0) {
> +         dev_err(dev, "Failed to initialize the sensor\n");
> +         return ret;
> +   }
> +
> +   return 0;
> +}

---

> +static int imx111_set_format(struct v4l2_subdev *sd,
> +                    struct v4l2_subdev_state *state,
> +                    struct v4l2_subdev_format *format)
> +{
> +   struct imx111 *sensor = sd_to_imx111(sd);
> +   struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
> +   struct v4l2_mbus_framefmt *fmt;
> +   const struct imx111_mode *mode;
> +
> +   mode = v4l2_find_nearest_size(imx111_modes, ARRAY_SIZE(imx111_modes),
> +                           width, height,
> +                           mbus_fmt->width, mbus_fmt->height);
> +
> +   fmt = v4l2_subdev_state_get_format(state, format->pad);
> +
> +   fmt->code = imx111_get_format_code(sensor, mbus_fmt->code, false);
> +   fmt->width = mode->width;
> +   fmt->height = mode->height;
> +   fmt->colorspace = V4L2_COLORSPACE_RAW;
> +
> +   *mbus_fmt = *fmt;
> +
> +   if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> +         sensor->cur_mode = mode;
> +         sensor->data_depth = imx111_get_format_bpp(fmt);
> +         __v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate,
> +                            div_u64(sensor->pixel_clk_raw, 2 * sensor->data_depth));
> +
> +         __v4l2_ctrl_modify_range(sensor->vblank, IMX111_VBLANK_MIN,
> +                            IMX111_VTL_MAX - mode->height, 1,
> +                            mode->vtl_def - mode->height);
> +         __v4l2_ctrl_s_ctrl(sensor->vblank, mode->vtl_def - mode->height);
> +
> +         __v4l2_ctrl_modify_range(sensor->hblank, IMX111_HBLANK_MIN,
> +                            IMX111_HTL_MAX - mode->width, 1,
> +                            mode->htl_def - mode->width);
> +         __v4l2_ctrl_s_ctrl(sensor->hblank, mode->htl_def - mode->width);

All the above V4L2 operations need to check for errors.

> +   }
> +
> +   return 0;
> +}

---

> +static int imx111_identify_module(struct imx111 *sensor)
> +{
> +   struct device *dev = regmap_get_device(sensor->regmap);
> +   u64 value, revision, manufacturer;
> +   int ret;
> +
> +   ret = cci_read(sensor->regmap, IMX111_PRODUCT_ID, &value, NULL);
> +   if (ret)
> +         return ret;
> +
> +   if (value != IMX111_CHIP_ID) {
> +         dev_err(dev, "chip id mismatch: %x!=%04llx", IMX111_CHIP_ID, value);
> +         return -ENXIO;
> +   }
> +
> +   cci_read(sensor->regmap, IMX111_REVISION, &revision, NULL);
> +   cci_read(sensor->regmap, IMX111_MANUFACTURER_ID, &manufacturer, NULL);

Instead of NULL, pass ret for the error code, and return ret at the end.

> +
> +   dev_dbg(dev, "module IMX%03llx rev. %llu manufacturer %llu\n",
> +         value, revision, manufacturer);
> +
> +   return 0;
> +}
> +
> +static int imx111_clk_init(struct imx111 *sensor)
> +{
> +   struct device *dev = regmap_get_device(sensor->regmap);
> +   u32 ndata_lanes = sensor->bus_cfg.bus.mipi_csi2.num_data_lanes;
> +   u64 extclk_rate, system_clk;
> +   unsigned int i;
> +
> +   extclk_rate = clk_get_rate(sensor->extclk);
> +   if (!extclk_rate)
> +         return dev_err_probe(dev, -EINVAL, "EXTCLK rate unknown\n");
> +
> +   for (i = 0; i < ARRAY_SIZE(imx111_pll); i++) {
> +         if (clk_get_rate(sensor->extclk) == imx111_pll[i].extclk_rate) {
> +               sensor->pll = &imx111_pll[i];
> +               break;
> +         }
> +   }
> +   if (!sensor->pll)
> +         return dev_err_probe(dev, -EINVAL, "Unsupported EXTCLK rate %llu\n", extclk_rate);

Max line length should be 80 columns. This applies everywhere the line 
length exceeds 80 characters.

> +
> +   system_clk = div_u64(extclk_rate, sensor->pll->pre_div) * sensor->pll->mult;
> +
> +   /*
> +    * Pixel clock or Logic clock is used for internal image processing is
> +    * generated by dividing into 1/10 or 1/8 frequency according to the
> +    * word length of the CSI2 interface. This clock is designating the pixel
> +    * rate and used as the base of integration time, frame rate etc.
> +    */
> +   sensor->pixel_clk_raw = system_clk * ndata_lanes;
> +
> +   /*
> +    * The CSI-2 bus is clocked for 16-bit per pixel, transmitted in DDR over n lanes
> +    * for RAW10 default format.
> +    */
> +   sensor->default_link_freq = div_u64(sensor->pixel_clk_raw * 8,
> +                               2 * IMX111_DATA_DEPTH_RAW10);
> +
> +   if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
> +       sensor->bus_cfg.link_frequencies[0] != sensor->default_link_freq)
> +         return dev_err_probe(dev, -EINVAL,
> +                          "Unsupported DT link-frequencies, expected %llu\n",
> +                          sensor->default_link_freq);
> +
> +   return 0;
> +}

---

> +static const struct of_device_id imx111_of_match[] = {
> +   { .compatible = "sony,imx111" },
> +   { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx111_of_match);
> +
> +static struct i2c_driver imx111_i2c_driver = {
> +   .driver = {
> +         .name = "imx111",
> +         .of_match_table = imx111_of_match,
> +         .pm = &imx111_pm_ops,
> +   },
> +   .probe = imx111_probe,
> +   .remove = imx111_remove,
> +};
> +module_i2c_driver(imx111_i2c_driver);
> +
> +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@...il.com>");
> +MODULE_DESCRIPTION("Sony IMX111 CMOS Image Sensor driver");
> +MODULE_LICENSE("GPL");

Best Regards,
Tarang

Powered by blists - more mailing lists