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

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

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