Re: [PATCH v3 2/2] media: i2c: Add OmniVision OV6211 image sensor driver

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Vladimir,

On Wed, Aug 20, 2025 at 01:57:52AM +0300, Vladimir Zapolskiy wrote:
> Hi Sakari,
> 
> thank you so much for review. Please find a few comments below.
> 
> On 8/18/25 15:08, Sakari Ailus wrote:
> > Hi Vladimir,
> > 
> > Thanks for the update.
> > 
> > On Wed, Aug 13, 2025 at 12:30:24AM +0300, Vladimir Zapolskiy wrote:
> > > OmniVision OV6211 is a monochrome image sensor, which produces frames in
> > > 8/10-bit raw output format and supports 400x400, 200x200 and 100x100
> > > output image resolution modes.
> > > 
> > > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> 
> <snip>
> 
> > > +static int ov6211_set_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > +	struct ov6211 *ov6211 = container_of(ctrl->handler, struct ov6211,
> > > +					     ctrl_handler);
> > > +	int ret;
> > > +
> > > +	/* V4L2 controls values will be applied only when power is already up */
> > > +	if (!pm_runtime_get_if_in_use(ov6211->dev))
> > 
> > I think this should be pm_runtime_get_if_active() as no writes will now
> > take place even if the sensor is powered on.
> 
> Ack.
> 
> > > +		return 0;
> > > +
> > > +	switch (ctrl->id) {
> > > +	case V4L2_CID_ANALOGUE_GAIN:
> > > +		ret = cci_write(ov6211->regmap, OV6211_REG_ANALOGUE_GAIN,
> > > +				ctrl->val, NULL);
> > > +		break;
> > > +	case V4L2_CID_EXPOSURE:
> > > +		ret = cci_write(ov6211->regmap, OV6211_REG_EXPOSURE,
> > > +				ctrl->val << 4, NULL);
> > > +		break;
> > > +	default:
> > > +		ret = -EINVAL;
> > > +		break;
> > > +	}
> > > +
> > > +	pm_runtime_put(ov6211->dev);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static const struct v4l2_ctrl_ops ov6211_ctrl_ops = {
> > > +	.s_ctrl = ov6211_set_ctrl,
> > > +};
> > > +
> > > +static int ov6211_init_controls(struct ov6211 *ov6211)
> > > +{
> > > +	struct v4l2_ctrl_handler *ctrl_hdlr = &ov6211->ctrl_handler;
> > > +	const struct ov6211_mode *mode = &supported_modes[0];
> > > +	struct v4l2_fwnode_device_properties props;
> > > +	s64 exposure_max, pixel_rate, h_blank;
> > > +	struct v4l2_ctrl *ctrl;
> > > +	int ret;
> > > +
> > > +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
> > > +	if (ret)
> > > +		return ret;
> > 
> > This check can be omitted. Up to you.
> 
> This check I would prefer to keep, otherwise I have a feeling that under
> ENOMEM the following code would make an attempt to insert the controls
> data over an NULL pointer...
> 
> I'll mock it to check it explicitly though, but I'm inclined to keep it.

The control framework will handle that internally.

> 
> <snip>
> 
> > > +
> > > +	ret = cci_write(ov6211->regmap, OV6211_REG_MODE_SELECT,
> > > +			OV6211_MODE_STREAMING, NULL);
> > > +	if (ret) {
> > > +		dev_err(ov6211->dev, "failed to start streaming: %d\n", ret);
> > > +		goto error;
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +error:
> > > +	pm_runtime_mark_last_busy(ov6211->dev);
> > 
> > Please omit direct calls to pm_runtime_mark_last_busy(); this is now called
> > via the runtime PM put autosuspend etc. functions.
> > 
> 
> Ack.
> 
> <snip>
> 
> > > +	if (bus_cfg.nr_of_link_frequencies != 1 ||
> > > +	    bus_cfg.link_frequencies[0] != link_freq_menu_items[0]) {
> > 
> > Could you use v4l2_link_freq_to_bitmap()? I think it'd simplify the
> > function and possibly error handling, too.
> > 
> 
> Ack, let it be so.
> 
> > > +		dev_err(ov6211->dev, "Unsupported MIPI CSI2 link frequency\n");
> > > +		ret = -EINVAL;
> > > +		goto error;
> > > +	}
> > > +
> > > +error:
> > > +	v4l2_fwnode_endpoint_free(&bus_cfg);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int ov6211_power_on(struct device *dev)
> > > +{
> > > +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > +	struct ov6211 *ov6211 = to_ov6211(sd);
> > > +	int ret;
> > > +
> > > +	if (ov6211->avdd) {
> > > +		ret = regulator_enable(ov6211->avdd);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	if (ov6211->dovdd) {
> > > +		ret = regulator_enable(ov6211->dovdd);
> > > +		if (ret)
> > > +			goto avdd_disable;
> > > +	}
> > > +
> > > +	if (ov6211->dvdd) {
> > > +		ret = regulator_enable(ov6211->dvdd);
> > > +		if (ret)
> > > +			goto dovdd_disable;
> > > +	}
> > 
> > Can you use the regulator bulk functions? Or if the order is required, add
> > a comment about it? Otherwise someone will provide a "fix" soon afterwards.
> > :-)
> 
> I've already responded to a similar review comment from Krzysztof on v1,
> here the rationale is to follow a deliberately selected model of having
> some of regulators as optional. Moreover the "digital core" DVDD regulator
> is truly optional, there is a working sensor configuration to omit it.
> 
> Unfortunately so far there is no bulk regulator helper, which deals properly
> with optional regulators, so I would prefer to keep it registered this way.

Won't the regulator framework provide you a dummy regulator if a given
regulator isn't assigned to the device in DT?

E.g. the CCS driver unconditionally requests regulators and fails on error.
It still works on ACPI platforms without any regulators (as in terms of the
regulator framework).

> > > +	gpiod_set_value_cansleep(ov6211->reset_gpio, 0);
> > > +	usleep_range(10 * USEC_PER_MSEC, 15 * USEC_PER_MSEC);
> > > +
> > > +	ret = clk_prepare_enable(ov6211->xvclk);
> > 
> > Is the clock really supposed to be enabled here, and not e.g. before
> > lifting reset?
> > 
> 
> The so called "gated" mode of XVCLK clock says that is should be enabled
> after releasing XSHUTDOWN pin, so I don't see a mistake here.
> 
> Please note, it's basically a common part with other OmniVision sensors,
> for instance you may get a reference from OG01A1B product specification etc.
> 
> Thank you for review. Shortly I'll send another new but very similar
> image sensor driver acknowledging your given comments.

-- 
Regards,

Sakari Ailus