| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
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