| 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:
<PN3P287MB35190A4AEE4C8D98142E7B6AFF59A@PN3P287MB3519.INDP287.PROD.OUTLOOK.COM>
Date: Fri, 25 Jul 2025 05:55:23 +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>,
"laurent.pinchart@...asonboard.com" <laurent.pinchart@...asonboard.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>, Ricardo Ribalda <ribalda@...omium.org>, Bryan
O'Donoghue <bryan.odonoghue@...aro.org>, Hans de Goede <hansg@...nel.org>,
André Apitzsch <git@...tzsch.eu>, Benjamin Mugnier
<benjamin.mugnier@...s.st.com>, Matthias Fend <matthias.fend@...end.at>,
Heimir Thor Sverrisson <heimir.sverrisson@...il.com>, Sylvain Petinot
<sylvain.petinot@...s.st.com>, Dongcheng Yan <dongcheng.yan@...el.com>,
Jingjing Xiong <jingjing.xiong@...el.com>, Arnd Bergmann <arnd@...db.de>,
"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 v5 2/2] media: i2c: add ov2735 image sensor driver
> On Thu, Jul 24, 2025 at 04:17:05PM +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)
>
> ...
>
> > MAINTAINERS | 9 +
>
> This should be started as part of patch 1 as in between you will have a
> dangling file, which is not recorded in MAINTAINERS.
>
> ...
>
> > + * Inspired from ov8858, imx219, imx283 camera drivers
>
> Missing period at the end.
>
> ...
>
> > +#include <linux/array_size.h>
>
> + bitops.h
Why??
> > +#include <linux/clk.h>
> > +#include <linux/container_of.h>
> > +#include <linux/delay.h>
> > +#include <linux/device/devres.h>
> > +#include <linux/err.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/property.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/units.h>
> > +#include <linux/types.h>
>
> > +#include <vdso/time64.h>
>
> We do not include vdso in the (regular) drivers. Use linux/time.h.
>
> ...
>
> > +struct ov2735 {
> > + struct device *dev;
>
> Do you need this? Can't it be derived from regmap cci below?
I prefer keeping the dev pointer directly in the struct for simplicity
and better readability. Using regmap_get_device(ov2735->cci) adds an
unnecessary level of indirection, especially since dev is frequently
used for logging, error handling, and regulator/device tree access.
I would prefer to retain it.
> > + struct regmap *cci;
> > + struct v4l2_subdev sd;
> > + struct media_pad pad;
>
> > + struct i2c_client *client;
>
> Do you need this?
>
> > + struct clk *xclk;
> > + struct gpio_desc *reset_gpio;
> > + struct gpio_desc *enable_gpio;
> > + struct regulator_bulk_data supplies[ARRAY_SIZE(ov2735_supply_name)];
> > +
> > + /* V4L2 Controls */
> > + struct v4l2_ctrl_handler handler;
> > + struct v4l2_ctrl *link_freq;
> > + struct v4l2_ctrl *pixel_rate;
> > + struct v4l2_ctrl *hblank;
> > + struct v4l2_ctrl *vblank;
> > + struct v4l2_ctrl *gain;
> > + struct v4l2_ctrl *exposure;
> > + struct v4l2_ctrl *test_pattern;
> > +
> > + u32 link_freq_index;
> > +
> > + u8 current_page;
> > + struct mutex page_lock;
> > +};
>
> ...
>
> > +static int ov2735_page_access(struct ov2735 *ov2735,
> > + u32 reg, void *val, int *err, bool is_read)
> > +{
> > + u8 page = (reg >> CCI_REG_PRIVATE_SHIFT) & 0xff;
>
> ' & 0xff' part is redundant.
>
> > + u32 addr = reg & ~CCI_REG_PRIVATE_MASK;
> > + int ret = 0;
>
> How is this assignment being used?
>
> > + 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, err);
> > + if (ret)
> > + goto err_mutex_unlock;
> > + ov2735->current_page = page;
> > + }
> > +
> > + if (is_read)
> > + ret = cci_read(ov2735->cci, addr, (u64 *)val, err);
> > + else
> > + ret = cci_write(ov2735->cci, addr, *(u64 *)val, err);
>
> Do you really need this castings?
Do you really think this casting is unnecessary?
Please check the definitions of cci_read/write
without this, we can't even build the driver.
> > +
> > +err_mutex_unlock:
> > + mutex_unlock(&ov2735->page_lock);
> > + return ret;
>
> Hmm... Wouldn't be cleanup.h helpful here?
>
> > +}
>
> ...
>
> > +static int ov2735_write(struct ov2735 *ov2735, u32 reg, u64 val, int *err)
> > +{
> > + return ov2735_page_access(ov2735, reg, (void *)&val, err, false);
>
> Why casting?
>
> > +}
>
> ...
>
> > +static int ov2735_set_pll_ctrl(struct ov2735 *ov2735)
> > +{
> > + struct ov2735_pll_parameters *pll_parameters;
> > + u8 pll_ctrl;
> > + u8 pll_outdiv;
> > + int ret = 0;
> > +
> > + pll_parameters = &pll_configs[ov2735->link_freq_index];
> > +
> > + /* BIT[7]: pll_clk_sel, BIT[6:2]: pll_nc, BIT[1:0]: pll_mc */
> > + pll_ctrl = ((pll_parameters->pll_nc << 2) |
> > + (pll_parameters->pll_mc << 0)) & OV2735_REG_PLL_ENABLE;
>
> Logically better to wrap like this (yes, I know that it's slightly longer than 80):
>
> pll_ctrl = ((pll_parameters->pll_nc << 2) | (pll_parameters->pll_mc << 0)) &
> OV2735_REG_PLL_ENABLE;
Will do.
Could you please clarify what you mean by "logically better" in this context?
> > + pll_outdiv = pll_parameters->pll_outdiv;
> > +
> > + ov2735_write(ov2735, OV2735_REG_PLL_CTRL, pll_ctrl, &ret);
>
> > + ov2735_write(ov2735, OV2735_REG_PLL_OUTDIV, pll_outdiv, &ret);
> > +
> > + return ret;
> > +}
>
> ...
>
> > + /* Apply format settings. */
>
> > + /* Apply customized values from user */
>
> Define a single style for one-line comments and use it everywhere consistently.
Are you referring to the period at the end of the comment?
> > + goto error_power_off;
>
> ...
>
> > + devm_pm_runtime_set_active_enabled(ov2735->dev);
> > + devm_pm_runtime_get_noresume(ov2735->dev);
>
> No error checks? What's the point to use devm and what will happen if the first
> fails, for example?
>
> --
> With Best Regards,
> Andy Shevchenko
With all due respect,
I completely understand and appreciate the need for multiple rounds of review.
However, where feasible, it would be helpful to receive style-related and
non-blocking comments earlier in the review process. Iterating on minor issues
in later versions, especially ones that could have been addressed together
earlier, can become a bit frustrating at times. I hope you can understand this
perspective.
Once again, thank you for your time and effort in helping improve the quality
of the driver.
Best Regards,
Hardev
Powered by blists - more mailing lists