Re: [PATCH 2/2] media: i2c: og0va1b: Add OmniVision OG0VA1B camera sensor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The driver calculates V4L2_CID_HBLANK as a physically impossible negative value and underreports V4L2_CID_PIXEL_RATE.
- [Medium] Missing `pm_runtime_mark_last_busy()` defeats the configured autosuspend delay.
- [Medium] The power-off sequence incorrectly disables the clock before asserting reset.
--

commit fdebdbb98b4ed425fa89fd8e9a84a3dd03b711db
Author: Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>

media: i2c: og0va1b: Add OmniVision OG0VA1B camera sensor

Add V4L2 sub device driver for OmniVision OG0VA1B image sensor.
OmniVision OG0VA1B is an image sensor, which produces frames in 10-bit
raw output format (Y10) over a 1-lane MIPI CSI-2 interface and supports
the 640x480 (VGA) resolution.

> diff --git a/drivers/media/i2c/og0va1b.c b/drivers/media/i2c/og0va1b.c
> new file mode 100644
> index 0000000000000..f0505b7ba7f32
> --- /dev/null
> +++ b/drivers/media/i2c/og0va1b.c
[ ... ]
> +static const struct og0va1b_mode og0va1b_modes[] = {
> +	{
> +		.width = 640,
> +		.height = 480,
> +		.hts = 376,
> +		.vts = 2096,

[Severity: High]
Does this code underreport the pixel rate and result in a physically
impossible negative value for the horizontal blanking?

Looking at og0va1b_modes, hts is 376 and width is 640. Later in
og0va1b_init_controls(), hblank_val is calculated as hts - width, which
equals -264:

og0va1b_init_controls() {
	...
	int hblank_val = mode->hts - mode->width;
	...
	og0va1b->hblank =
		v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_HBLANK,
				  hblank_val, hblank_val, 1, hblank_val);
}

Could the 376 value represent clock cycles rather than pixels, where the
sensor outputs 2 pixels per clock? If so, should hts be defined in pixels
(752) and the V4L2_CID_PIXEL_RATE be set to 96 MHz (for 10-bit raw over a
480 MHz link) to comply with V4L2 standards?

[ ... ]
> +static int og0va1b_power_off(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct og0va1b *og0va1b = sd_to_og0va1b(sd);
> +
> +	clk_disable_unprepare(og0va1b->inclk);
> +	gpiod_set_value_cansleep(og0va1b->reset_gpio, 1);

[Severity: Medium]
Does this sequence leave the sensor in a dangling state?

The power-off sequence disables the input clock before asserting the hardware
reset GPIO. To ensure the sensor's internal state machine enters reset cleanly,
should the reset line be asserted while the clock is still running?

[ ... ]
> +static int og0va1b_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> +	switch (ctrl->id) {
> +	case V4L2_CID_EXPOSURE:
[ ... ]
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	pm_runtime_put_autosuspend(og0va1b->sd.dev);
> +	return ret;
> +}

[Severity: Medium]
Will this defeat the configured autosuspend delay?

The driver explicitly configures an autosuspend delay of 1000ms in probe(),
but calls pm_runtime_put_autosuspend() here without first updating the
last_busy timestamp via pm_runtime_mark_last_busy().

Without the updated timestamp, the runtime PM core might evaluate the
autosuspend timer as already expired, causing the device to power off
synchronously.

[ ... ]
> +static int og0va1b_disable_streams(struct v4l2_subdev *sd,
> +				   struct v4l2_subdev_state *state,
> +				   u32 pad, u64 streams_mask)
> +{
> +	struct og0va1b *og0va1b = sd_to_og0va1b(sd);
> +	int ret;
> +
> +	ret = cci_write(og0va1b->regmap, OG0VA1B_REG_STREAMING, 0, NULL);
> +	if (ret)
> +		dev_warn(og0va1b->sd.dev,
> +			 "failed to stop streaming: %d\n", ret);
> +
> +	usleep_range(5000, 6000);
> +	pm_runtime_put_autosuspend(og0va1b->sd.dev);
> +
> +	return 0;
> +}

[Severity: Medium]
Similarly, does the missing pm_runtime_mark_last_busy() call before
pm_runtime_put_autosuspend() here cause immediate device suspend, defeating
the autosuspend timer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-og0va1b-v1-0-dda71bb83009@oss.qualcomm.com?part=2