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

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

Hi Sakari.

On 8/21/25 12:18, Sakari Ailus wrote:
> 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.
> 

Right, I've tested a mocked version before sending v4, and it worked
expectedly with no issues, the check for a return value has been removed.

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

You are right here, bulk version of regulator API falls back to a dummy
regulator usage, I've tested that it works totally fine, and therefore I'll
change to it in the next version of the driver.

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

-- 
Best wishes,
Vladimir