| 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: <d7c44d80-f8b6-48ff-a41a-c340bee4e5af@intel.com>
Date: Tue, 8 Jul 2025 15:41:00 +0800
From: "Yan, Dongcheng" <dongcheng.yan@...el.com>
To: Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>,
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>,
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 Hardevsinh,
On 7/8/2025 3:04 PM, Hardevsinh Palaniya wrote:
> 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
> }
>
cci_write/read has a param named ret, for example:
cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, 0x01, &ret);
cci_write(ov2735->cci, OV2735_REG_STREAM_CTRL, OV2735_STREAM_OFF, &ret);
then check ret here or return ret.
Thanks,
Dongcheng> Best Regards,
> Hardev
Powered by blists - more mailing lists