| 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: <aIKi1BkNzNvsf5Tr@smile.fi.intel.com>
Date: Fri, 25 Jul 2025 00:17:08 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>
Cc: sakari.ailus@...ux.intel.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,
devicetree@...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
> +#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?
> + 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?
> +
> +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;
> + 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.
> + 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
Powered by blists - more mailing lists