Re: [PATCH v5 2/2] media: i2c: add ov2735 image sensor driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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