On Thu, Jul 24, 2025 at 04:17:05PM +0530, Hardevsinh Palaniya wrote:

quoted

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)

...

quoted

 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.

...

quoted

+ * Inspired from ov8858, imx219, imx283 camera drivers

Missing period at the end.

...

quoted

+#include <linux/array_size.h>

+ bitops.h

Why??
 

quoted

+#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>

quoted

+#include <vdso/time64.h>

We do not include vdso in the (regular) drivers. Use linux/time.h.

...

quoted

+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. 

quoted

+     struct regmap *cci;
+     struct v4l2_subdev sd;
+     struct media_pad pad;

quoted

+     struct i2c_client *client;

Do you need this?

quoted

+     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;
+};

...

quoted

+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.

quoted

+     u32 addr = reg & ~CCI_REG_PRIVATE_MASK;
+     int ret = 0;

How is this assignment being used?

quoted

+     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.

quoted

+
+err_mutex_unlock:
+     mutex_unlock(&ov2735->page_lock);
+     return ret;

Hmm... Wouldn't be cleanup.h helpful here?

quoted

+}

...

quoted

+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?

quoted

+}

...

quoted

+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?

quoted

+     pll_outdiv = pll_parameters->pll_outdiv;
+
+     ov2735_write(ov2735, OV2735_REG_PLL_CTRL, pll_ctrl, &ret);

quoted

+     ov2735_write(ov2735, OV2735_REG_PLL_OUTDIV, pll_outdiv, &ret);
+
+     return ret;
+}

...

quoted

+     /* Apply format settings. */

quoted

+     /* 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?
 

quoted

+             goto error_power_off;

...

quoted

+     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