| 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:
<PN3P287MB351968C7B57C3C97D799D1E1FF4EA@PN3P287MB3519.INDP287.PROD.OUTLOOK.COM>
Date: Tue, 8 Jul 2025 07:04:48 +0000
From: Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
CC: "sakari.ailus@...ux.intel.com" <sakari.ailus@...ux.intel.com>, Himanshu
Bhavani <himanshu.bhavani@...iconsignals.io>, Mauro Carvalho Chehab
<mchehab@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Hans Verkuil
<hverkuil@...all.nl>, André Apitzsch <git@...tzsch.eu>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>, Hans de Goede
<hansg@...nel.org>, Tarang Raval <tarang.raval@...iconsignals.io>, Jingjing
Xiong <jingjing.xiong@...el.com>, Dongcheng Yan <dongcheng.yan@...el.com>,
Sylvain Petinot <sylvain.petinot@...s.st.com>, Benjamin Mugnier
<benjamin.mugnier@...s.st.com>, Matthias Fend <matthias.fend@...end.at>, Arnd
Bergmann <arnd@...db.de>, 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 2/2] media: i2c: add ov2735 image sensor driver
Hi Andy,
Thanks for the review.
I have corrected the code, and all of your comments have now been addressed.
However, I have a question about one of the comments. Please see below.
> On Mon, Jul 07, 2025 at 08:31:06PM +0530, Hardevsinh Palaniya wrote:
> > Add a v4l2 subdevice driver for the Omnivision OV2735 sensor.
> >
> > The Omnivision OV2735 is a 1/2.7-Inch CMOS image sensor with an
> > active array size of 1920 x 1080.
> >
> > - Manual exposure an gain control support
> > - vblank/hblank control support
> > - Supported resolution: 1920 x 1080 @ 30fps (SGRBG10)
>
> ...
>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/units.h>
>
> More stuff is in use than just these headers provide.
> E.g.,
>
> + array_size.h
> + container_of.h
> + gpio/consumer.h
> + types.h
>
> And so on... Also in some cases the forward declarations are enough to have.
>
> .,,
>
> > +#define OV2735_LINK_FREQ_420MHZ 420000000
>
> HZ_PER_MHZ ?
>
> ...
>
> > +#define OV2735_PIXEL_RATE 168000000
>
> What's the unit?
>
> ...
>
> > +static const s64 link_freq_menu_items[] = {
> > + OV2735_LINK_FREQ_420MHZ
>
> Keep the trailing comma like you have done in other cases.
>
> > +};
>
> ...
>
> > +static int ov2735_enable_test_pattern(struct ov2735 *ov2735, u32 pattern)
> > +{
> > + int ret;
> > + u64 val;
> > +
> > + ret = cci_read(ov2735->cci, OV2735_REG_TEST_PATTERN, &val, NULL);
> > + if (ret)
> > + return ret;
> > +
> > + switch (pattern) {
> > + case 0:
> > + val &= ~OV2735_TEST_PATTERN_ENABLE;
> > + break;
> > + case 1:
> > + val |= OV2735_TEST_PATTERN_ENABLE;
> > + break;
> > + }
>
> > + ret = cci_write(ov2735->cci, OV2735_REG_TEST_PATTERN, val, NULL);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
>
> Is this the required style? Because these 5 LoCs is just a simple
>
> return cci_write(ov2735->cci, OV2735_REG_TEST_PATTERN, val, NULL);
>
> > +}
>
> ...
>
> > +static int ov2735_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct ov2735 *ov2735 = container_of(ctrl->handler, struct ov2735,
> > + handler);
> > + const struct ov2735_mode *mode;
> > + struct v4l2_mbus_framefmt *fmt;
> > + struct v4l2_subdev_state *state;
>
> > + int vts;
>
> Can be negative?
>
> > + int ret = 0;
>
> How is this assignment useful?
>
> > + state = v4l2_subdev_get_locked_active_state(&ov2735->sd);
> > + fmt = v4l2_subdev_state_get_format(state, 0);
> > +
> > + mode = v4l2_find_nearest_size(supported_modes,
> > + ARRAY_SIZE(supported_modes),
> > + width, height,
> > + fmt->width, fmt->height);
> > +
> > + if (ctrl->id == V4L2_CID_VBLANK) {
> > + /* Honour the VBLANK limits when setting exposure. */
> > + s64 max = mode->height + ctrl->val - 4;
> > +
> > + ret = __v4l2_ctrl_modify_range(ov2735->exposure,
> > + ov2735->exposure->minimum, max,
> > + ov2735->exposure->step,
> > + ov2735->exposure->default_value);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + /*
> > + * Applying V4L2 control value only happens
> > + * when power is up for streaming
>
> Multi-line comments shouldn't neglect punctuation.
>
> > + */
> > + if (pm_runtime_get_if_in_use(ov2735->dev) == 0)
> > + return 0;
> > +
> > + ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, 0x01, NULL);
> > +
> > + switch (ctrl->id) {
> > + case V4L2_CID_EXPOSURE:
> > + ret |= cci_write(ov2735->cci, OV2735_REG_LONG_EXPOSURE, ctrl->val, NULL);
> > + break;
> > + case V4L2_CID_ANALOGUE_GAIN:
> > + ret |= cci_write(ov2735->cci, OV2735_REG_ANALOG_GAIN, ctrl->val, NULL);
> > + break;
> > + case V4L2_CID_HBLANK:
> > + ret |= cci_write(ov2735->cci, OV2735_REG_HBLANK, ctrl->val, NULL);
> > + break;
> > + case V4L2_CID_VBLANK:
> > + vts = ctrl->val + mode->height;
> > + ret |= cci_write(ov2735->cci, OV2735_REG_FRAME_EXP_SEPERATE_EN,
> > + OV2735_FRAME_EXP_SEPERATE_EN, NULL);
> > + ret |= cci_write(ov2735->cci, OV2735_REG_FRAME_LENGTH, vts, NULL);
> > + break;
> > + case V4L2_CID_TEST_PATTERN:
> > + ret = ov2735_enable_test_pattern(ov2735, ctrl->val);
> > + break;
> > + default:
> > + dev_err(ov2735->dev, "ctrl(id:0x%x, val:0x%x) is not handled\n",
> > + ctrl->id, ctrl->val);
> > + break;
> > + }
> > + ret |= cci_write(ov2735->cci, OV2735_REG_FRAME_SYNC, 0x01, NULL);
> > +
> > + pm_runtime_put(ov2735->dev);
> > +
> > + return ret;
> > +}
>
> ...
>
> > +static int ov2735_init_controls(struct ov2735 *ov2735)
> > +{
> > + struct v4l2_ctrl_handler *ctrl_hdlr;
> > + struct v4l2_fwnode_device_properties props;
> > + const struct ov2735_mode *mode = &supported_modes[0];
> > + u64 hblank_def, vblank_def, exp_max;
> > + int ret;
> > +
> > + ctrl_hdlr = &ov2735->handler;
> > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 7);
> > + if (ret)
> > + return ret;
> > +
> > + ov2735->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops, V4L2_CID_PIXEL_RATE,
> > + 0, OV2735_PIXEL_RATE, 1, OV2735_PIXEL_RATE);
>
> Besides it's too long, it has trailing space.
>
> > +
> > + ov2735->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &ov2735_ctrl_ops,
> > + V4L2_CID_LINK_FREQ,
> > + 0, 0, link_freq_menu_items);
> > + if (ov2735->link_freq)
> > + ov2735->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > + hblank_def = mode->hts_def - mode->width;
> > + ov2735->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops, V4L2_CID_HBLANK,
> > + hblank_def, hblank_def, 1, hblank_def);
> > +
> > + vblank_def = mode->vts_def - mode->height;
> > + ov2735->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops,
> > + V4L2_CID_VBLANK, vblank_def,
> > + OV2735_VTS_MAX - mode->height,
> > + 1, vblank_def);
>
> It's weird indentation.
>
> > +
> > + exp_max = mode->vts_def - 4;
> > + ov2735->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops,
> > + V4L2_CID_EXPOSURE, OV2735_EXPOSURE_MIN,
> > + exp_max, OV2735_EXPOSURE_STEP, mode->exp_def);
> > +
> > + ov2735->gain = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops,
> > + V4L2_CID_ANALOGUE_GAIN, ANALOG_GAIN_MIN,
> > + ANALOG_GAIN_MAX, ANALOG_GAIN_STEP,
> > + ANALOG_GAIN_DEFAULT);
>
> Ditto.
>
> > + ov2735->test_pattern = v4l2_ctrl_new_std_menu_items(ctrl_hdlr,
> > + &ov2735_ctrl_ops, V4L2_CID_TEST_PATTERN,
> > + ARRAY_SIZE(ov2735_test_pattern_menu) - 1,
> > + 0, 0, ov2735_test_pattern_menu);
>
> Ditto.
>
> > + if (ctrl_hdlr->error) {
> > + ret = ctrl_hdlr->error;
> > + dev_err(ov2735->dev, "control init failed (%d)\n", ret);
> > + goto error;
> > + }
> > +
> > + ret = v4l2_fwnode_device_parse(ov2735->dev, &props);
> > + if (ret)
> > + goto error;
> > +
> > + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov2735_ctrl_ops,
> > + &props);
> > + if (ret)
> > + goto error;
> > +
> > + ov2735->sd.ctrl_handler = ctrl_hdlr;
> > +
> > + return 0;
> > +error:
>
> Usual way of naming labels is to explain what is going to happen when goto.
> Moreover it's even inconsistent, the below code use err prefix, but better
> naming.
>
> Here the
>
> err_handler_free:
>
> for example is better.
>
> > + v4l2_ctrl_handler_free(ctrl_hdlr);
> > +
> > + return ret;
> > +}
>
> ...
>
> > +static int ov2735_enable_streams(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *state, u32 pad,
> > + u64 streams_mask)
>
> Indentation issue.
>
> > +{
> > + struct ov2735 *ov2735 = to_ov2735(sd);
> > + const struct ov2735_mode *mode;
> > + const struct ov2735_reglist *reg_list;
> > + const struct v4l2_mbus_framefmt *fmt;
> > + int ret = 0;
>
> Needless assignment.
>
> > + fmt = v4l2_subdev_state_get_format(state, 0);
> > + mode = v4l2_find_nearest_size(supported_modes,
> > + ARRAY_SIZE(supported_modes),
> > + width, height,
> > + fmt->width, fmt->height);
> > +
> > + ret = pm_runtime_resume_and_get(ov2735->dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + reg_list = &mode->reg_list;
> > + ret = cci_multi_reg_write(ov2735->cci, reg_list->regvals, reg_list->num_regs, NULL);
> > + if (ret) {
> > + dev_err(ov2735->dev, "%s failed to send mfg header\n", __func__);
> > + goto err_rpm_put;
> > + }
> > +
> > + /* Apply customized values from user */
> > + ret = __v4l2_ctrl_handler_setup(ov2735->sd.ctrl_handler);
> > + if (ret)
> > + goto err_rpm_put;
> > +
> > + /* set stream on register */
> > + ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, 0x01, NULL);
> > + ret |= cci_write(ov2735->cci, OV2735_REG_STREAM_CTRL, OV2735_STREAM_ON, NULL);
> > + if (ret)
> > + goto err_rpm_put;
> > +
> > + return 0;
> > +
> > +err_rpm_put:
> > + pm_runtime_put(ov2735->dev);
> > + return ret;
> > +}
>
> ...
>
> > +static int ov2735_disable_streams(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *state, u32 pad,
> > + u64 streams_mask)
> > +{
> > + struct ov2735 *ov2735 = to_ov2735(sd);
> > + int ret = 0;
> > +
> > + /* set stream off register */
> > + ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, 0x01, NULL);
> > + ret |= cci_write(ov2735->cci, OV2735_REG_STREAM_CTRL, OV2735_STREAM_OFF, NULL);
>
> Why not using the ret parameter? Same for other similar cases above and beyond.
I am not sure what you want to suggest here.
Do I need to check ret like this?
ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, 0x01, NULL);
if (ret) {
// error message
}
ret = cci_write(ov2735->cci, OV2735_REG_STREAM_CTRL, OV2735_STREAM_OFF, NULL);
if (ret) {
// error message
}
Best Regards,
Hardev
Powered by blists - more mailing lists