| 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:
<PN3P287MB1829E05C58DC154C4F3411ED8BF8A@PN3P287MB1829.INDP287.PROD.OUTLOOK.COM>
Date: Fri, 31 Oct 2025 10:36:01 +0000
From: Tarang Raval <tarang.raval@...iconsignals.io>
To: Svyatoslav Ryhel <clamor95@...il.com>
CC: Jacopo Mondi <jacopo.mondi@...asonboard.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>,
"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,
> On Thu, Oct 30, 2025 at 06:11:42PM +0200, Svyatoslav Ryhel wrote:
> > чт, 30 жовт. 2025 р. о 17:50 Jacopo Mondi <jacopo.mondi@...asonboard.com> пише:
> > >
> > > Hello,
> > > sorry for entering the conversation
> > >
> > > On Thu, Oct 30, 2025 at 05:13:31PM +0200, Svyatoslav Ryhel wrote:
> > > > чт, 30 жовт. 2025 р. о 16:55 Tarang Raval <tarang.raval@...iconsignals.io> пише:
> > > > >
> > > > > 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.
> > > > >
> > > >
> > > > imx219 and imx319 which are recommended references use switch, and it
> > > > seems that media maintainters are particularly picky to code style, I
> > > > have copied it from there.
> > > >
> > >
> > > Personally, whenever doing reviews, receiving a reply that ignores the
> > > merit of the comment and simply refers to the existing code base as an
> > > excuse for not caring, it's what put me off the most.
> > >
> > > Please respect the time reviewers have invested in looking at your
> > > code by at least considering their comment instead of dismissing them.
> > > In this specific case you could have easily said "I like it more this
> > > way and it's consistent with what other drivers do". The same cannot
> > > be said for other comments that you have decided to ignore.
> > >
> >
> > I did not state that I will ignore all the comments. Contrary, I will
> > be applying most/all of them. My point is that Linux kernel
> > documentation, which Sakari Ailus (not an offence or blaming) pointed
> > in v2 clearly pointed to imx219 and imx319 as an valid reference
I believe you can refer to any recently merged camera sensor driver, as it
will likely cover the necessary points.
> I don't think imx219 is outdated, it's actually one of the most
> developed one and is a very good reference. Simply, it's not perfect,
> and we keep finding bugs that you could avoid from the very beginning
> if you take the time to look into comments you receive.
>
> > drivers, while they seem to be outdated. So by using those as
> > reference I wasted both my time and maintainers time. You say "Please
> > respect the time reviewers" but no one respects my time.
>
>
> I'm sorry if you feel disrispected but I think you have been pointed
> to the best reference we have that simply happens not to be perfect ?
>
> And you might ask "if they got a pass for broken code, why didn't I
> get one" ? This is not what's happening though, nobody got a pass,
> simply changing code that is already mainline takes more effort and
> motivation and if we can avoid bugs from the very beginning we should
> do so (otherwise someone will have to fix it later on).
>
> > >
> > > > > > + 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.
> > > > >
> > > >
> > > > imx219 does not specifies this as a define
> > > >
> > >
> > > It doesn't but it should, like 90% of other drivers in mainline do
> > >
> > >
> > > > > > + __v4l2_ctrl_modify_range(sensor->exposure,
> > > > > > + sensor->exposure->minimum,
> > > > > > + max, sensor->exposure->step, max);
> > > > >
> > > > > This may fail; consider adding an error check.
> > > > >
> > > >
> > > > imx219 does not return error here too
> > > >
> > >
> > > so ?
> > >
> > > > > > + 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.
> > > > >
> > > >
> > > > This make code reading cleaner, no?
> > > >
> > > > > > + 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;
> > > > >
> > > >
> > > > cci_write does not state that ret must be initiated.
> > >
I think I should have explained the reasoning behind the change
instead of providing a direct modification. I'll make sure to provide more
context next time.
Best Regards,
Tarang
Powered by blists - more mailing lists