| 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: <aG_EM2SaL1dkueKW@smile.fi.intel.com>
Date: Thu, 10 Jul 2025 16:46:27 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>
Cc: sakari.ailus@...ux.intel.com, krzk+dt@...nel.org,
kieran.bingham@...asonboard.com, dave.stevenson@...pberrypi.com,
pratap.nirujogi@....com, laurent.pinchart@...asonboard.com,
tarang.raval@...iconsignals.io,
Himanshu Bhavani <himanshu.bhavani@...iconsignals.io>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Hans Verkuil <hverkuil@...all.nl>,
Ricardo Ribalda <ribalda@...omium.org>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
André Apitzsch <git@...tzsch.eu>,
Arnd Bergmann <arnd@...db.de>,
Matthias Fend <matthias.fend@...end.at>,
Sylvain Petinot <sylvain.petinot@...s.st.com>,
Dongcheng Yan <dongcheng.yan@...el.com>,
Benjamin Mugnier <benjamin.mugnier@...s.st.com>,
Heimir Thor Sverrisson <heimir.sverrisson@...il.com>,
Jingjing Xiong <jingjing.xiong@...el.com>,
Hans de Goede <hansg@...nel.org>, Hao Yao <hao.yao@...el.com>,
linux-media@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] media: i2c: add ov2735 image sensor driver
On Thu, Jul 10, 2025 at 06:40:59PM +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.
>
> The following features are supported:
> - Manual exposure an gain control support
> - vblank/hblank control support
> - Test pattern support control
> - Supported resolution: 1920 x 1080 @ 30fps (SGRBG10)
...
> +static int ov2735_cci_access(struct ov2735 *ov2735,
> + u32 reg, void *val, int *err, bool is_read)
> +{
> + u8 page = (reg >> CCI_REG_PRIVATE_SHIFT) & 0xff;
> + u32 addr = reg & ~CCI_REG_PRIVATE_MASK;
> + int ret = 0;
> +
> + if (err && *err)
> + return *err;
> +
> + mutex_lock(&ov2735->page_lock);
> +
> + /* Perform page access before read/write */
> + if (ov2735->current_page != page) {
> + ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, page, &ret);
> + if (ret)
> + goto err_mutex_unlock;
> + ov2735->current_page = page;
> + }
> +
> + if (is_read)
> + cci_read(ov2735->cci, addr, (u64 *)val, &ret);
> + else
> + cci_write(ov2735->cci, addr, *(u64 *)val, &ret);
> +
> + if (!ret && err)
ret here is never non-0. I believe the check is unneeded and this should be
inside the below label. Otherwise, what's the point?
> + *err = ret;
> +
> +err_mutex_unlock:
> + mutex_unlock(&ov2735->page_lock);
> + return ret;
> +}
Have you researched on how hard is to implement a page access framework as
being suggested in previous review?
...
> +static int ov2735_enable_test_pattern(struct ov2735 *ov2735, u32 pattern)
> +{
> + int ret = 0;
> + u64 val;
> +
> + ov2735_cci_read(ov2735, OV2735_REG_TEST_PATTERN, &val, &ret);
In this case is better to have the NULL form with no assignment done.
Use a common sense.
> + if (ret)
> + return ret;
> +
> + switch (pattern) {
> + case 0:
> + val &= ~OV2735_TEST_PATTERN_ENABLE;
> + break;
> + case 1:
> + val |= OV2735_TEST_PATTERN_ENABLE;
> + break;
> + }
> +
> + return ov2735_cci_write(ov2735, OV2735_REG_TEST_PATTERN, val, NULL);
> +}
...
> +static int ov2735_enable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state, u32 pad,
> + u64 streams_mask)
> +{
> + 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;
> +
> + 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;
> + ov2735_cci_multi_reg_write(ov2735, reg_list->regvals, reg_list->num_regs, &ret);
NULL-variant is better here. Revisit all these again, please.
> + 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 */
> + ov2735_cci_write(ov2735, OV2735_REG_STREAM_CTRL, OV2735_STREAM_ON, &ret);
> + if (ret)
> + goto err_rpm_put;
> +
> + return 0;
> +
> +err_rpm_put:
> + pm_runtime_put(ov2735->dev);
> + return ret;
> +}
...
> +static int ov2735_power_on(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct ov2735 *ov2735 = to_ov2735(sd);
> + unsigned long delay_us;
> + int ret;
> +
> + gpiod_set_value_cansleep(ov2735->enable_gpio, 0);
> + fsleep(3000);
Can you add a comment with a datasheet reference to explain the value used
here?
> + ret = regulator_bulk_enable(ARRAY_SIZE(ov2735_supply_name),
> + ov2735->supplies);
> + if (ret) {
> + dev_err(ov2735->dev, "failed to enable regulators\n");
> + return ret;
> + }
> +
> + gpiod_set_value_cansleep(ov2735->enable_gpio, 1);
> + fsleep(3000);
Ditto.
> + gpiod_set_value_cansleep(ov2735->reset_gpio, 0);
> + fsleep(3000);
Ditto.
> + ret = clk_prepare_enable(ov2735->xclk);
> + if (ret) {
> + dev_err(ov2735->dev, "failed to enable clock\n");
> + goto err_regulator_off;
> + }
> +
> + /* 8192 cycles prior to first SCCB transaction */
> + delay_us = DIV_ROUND_UP(8192, OV2735_XCLK_FREQ / 1000 / 1000);
Shouldn't these / 1000 / 1000 be simply / HZ_PER_MHZ?
Or if you want to keep it precise to the physics, use MEGA from units.h.
> + fsleep(delay_us);
> +
> + return 0;
> +
> +err_regulator_off:
> + regulator_bulk_disable(ARRAY_SIZE(ov2735_supply_name), ov2735->supplies);
> + return ret;
> +}
...
> +static int ov2735_power_off(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct ov2735 *ov2735 = to_ov2735(sd);
> +
> + gpiod_set_value_cansleep(ov2735->enable_gpio, 1);
Power off enables the chip?! Perhaps you missed polarity in DT?
> + clk_disable_unprepare(ov2735->xclk);
> + gpiod_set_value_cansleep(ov2735->reset_gpio, 0);
In the similar way.
> + regulator_bulk_disable(ARRAY_SIZE(ov2735_supply_name), ov2735->supplies);
> +
> + return 0;
> +}
...
> +static int ov2735_parse_endpoint(struct ov2735 *ov2735)
> +{
> + struct fwnode_handle *fwnode;
> + struct v4l2_fwnode_endpoint bus_cfg = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY,
> + };
> + struct fwnode_handle *ep;
> + int ret;
> +
> + fwnode = dev_fwnode(ov2735->dev);
> + ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> + if (!ep) {
> + dev_err(ov2735->dev, "Failed to get next endpoint\n");
> + return -ENXIO;
Please, be consistent, use dev_err_probe().
> + }
> +
> + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> + fwnode_handle_put(ep);
> + if (ret)
> + return ret;
> +
> + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> + dev_err_probe(ov2735->dev, -EINVAL, "only 2 data lanes are supported\n");
ret = dev_err_probe(...);
Otherwise it's 0.
> + goto error_out;
> + }
> +
> + return 0;
> +
> +error_out:
> + v4l2_fwnode_endpoint_free(&bus_cfg);
> +
> + return ret;
> +};
...
> + ov2735->cci = devm_cci_regmap_init_i2c(client, 8);
> + if (IS_ERR(ov2735->cci)) {
> + ret = PTR_ERR(ov2735->cci);
> + return dev_err_probe(ov2735->dev, ret, "failed to initialize CCI\n");
Why not in a single expression?
> + }
...
> + /* Set Current page to 0 */
> + ov2735->current_page = 0;
> + mutex_init(&ov2735->page_lock);
ret = devm_mutex_init(...);
i.o.w. do not mix devm chain with non-devm allocations.
...
> + ov2735->xclk = devm_v4l2_sensor_clk_get(ov2735->dev, NULL);
> + if (IS_ERR(ov2735->xclk)) {
> + return dev_err_probe(ov2735->dev, PTR_ERR(ov2735->xclk),
> + "failed to get xclk\n");
> + }
You can drop {}.
> + xclk_freq = clk_get_rate(ov2735->xclk);
> + if (xclk_freq != OV2735_XCLK_FREQ)
> + return dev_err_probe(ov2735->dev, -EINVAL,
> + "xclk frequency not supported: %d Hz\n",
> + xclk_freq);
You see, the code is full of inconsistencies, please take your time (no need to
hurry with a new version, you have at least two months for this series to be
reviewed and applied if okay).
...
> + ret = ov2735_get_regulators(ov2735);
> + if (ret)
> + return dev_err_probe(ov2735->dev, ret, "failed to get regulators\n");
> +
> + ret = ov2735_parse_endpoint(ov2735);
> + if (ret) {
> + dev_err(ov2735->dev, "failed to parse endpoint configuration\n");
> + return ret;
return dev_error_probe(...);
> + }
Again, read above.
...
> + ret = v4l2_async_register_subdev_sensor(&ov2735->sd);
> + if (ret < 0) {
Why ' < 0'? What is the meaning of the positive return?
Same Q to all these inconsistencies (note, in some cases it's needed
but I doubt here and in many other places it's the case).
> + dev_err_probe(ov2735->dev, ret,
> + "failed to register ov2735 sub-device\n");
> + goto error_subdev_cleanup;
> + }
...
> +static void ov2735_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct ov2735 *ov2735 = to_ov2735(sd);
> +
> + v4l2_async_unregister_subdev(sd);
> + v4l2_subdev_cleanup(&ov2735->sd);
> + media_entity_cleanup(&sd->entity);
> + v4l2_ctrl_handler_free(ov2735->sd.ctrl_handler);
> + pm_runtime_disable(ov2735->dev);
Why not devm variant being used in probe?
> + if (!pm_runtime_status_suspended(ov2735->dev))
> + ov2735_power_off(ov2735->dev);
> + pm_runtime_set_suspended(ov2735->dev);
So, you probably want SMART_SUSPEND or something like that when enabling
runtime PM in probe. These lines looks like duplication of what PM runtime may
do for you.
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists