Re: [RESEND PATCH v5 2/2] media: i2c: add os02g10 image sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Setting digital gain to its advertised maximum of 2048 causes a wrap-around to 0, resulting in a black image.
- [Medium] Incorrect error checking for pm_runtime_get_if_active() in os02g10_set_ctrl() leads to unbalanced pm_runtime_put() and hardware access while powered off.
- [Low] Missing HBLANK control implementation despite claims in the commit message.
--

commit 1c1498e9a9139b064778f0a3a923b9cecbc3aa62
Author: Elgin Perumbilly <elgin.perumbilly@siliconsignals.io>

media: i2c: add os02g10 image sensor driver

This commit adds a new V4L2 subdevice driver for the Omnivision os02g10
CMOS image sensor. It supports 1920x1080 resolution at 30fps and includes
manual exposure, gain, blanking, flip, and test pattern controls.

>     The following features are supported:
>     - Manual exposure an gain control support
>     - vblank/hblank control support

[Severity: Low]
Is the HBLANK control missing from this driver?

The commit message advertises support for it, but V4L2_CID_HBLANK is never
instantiated in os02g10_init_controls().

> diff --git a/drivers/media/i2c/os02g10.c b/drivers/media/i2c/os02g10.c
> new file mode 100644
> index 0000000000000..61de45c54d188
> --- /dev/null
> +++ b/drivers/media/i2c/os02g10.c
[ ... ]
> +static int os02g10_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct os02g10 *os02g10 = container_of_const(ctrl->handler,
> +						     struct os02g10, handler);
> +	struct v4l2_subdev_state *state;
> +	struct v4l2_mbus_framefmt *fmt;
> +	int ret = 0;
> +
> +	state = v4l2_subdev_get_locked_active_state(&os02g10->sd);
> +	fmt = v4l2_subdev_state_get_format(state, 0);
> +
> +	if (ctrl->id == V4L2_CID_VBLANK) {
> +		/* Honour the VBLANK limits when setting exposure */
> +		s64 max = fmt->height + ctrl->val - OS02G10_EXPOSURE_MARGIN;
> +
> +		ret = __v4l2_ctrl_modify_range(os02g10->exposure,
> +					       os02g10->exposure->minimum, max,
> +					       os02g10->exposure->step,
> +					       os02g10->exposure->default_value);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (pm_runtime_get_if_active(os02g10->dev) == 0)
> +		return 0;

[Severity: Medium]
Does this check properly handle negative error codes from
pm_runtime_get_if_active()? 

If runtime PM is disabled, this function returns a negative error code
(such as -EINVAL), which bypasses this check. This allows execution to
proceed to the hardware accesses and subsequently call pm_runtime_put() at
the end of the function, which will underflow the usage counter since it
was never successfully incremented.

> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_EXPOSURE:
> +		cci_write(os02g10->cci, OS02G10_REG_LONG_EXPOSURE,
> +			  ctrl->val, &ret);
> +		break;
> +	case V4L2_CID_ANALOGUE_GAIN:
> +		cci_write(os02g10->cci, OS02G10_REG_ANALOG_GAIN,
> +			  ctrl->val, &ret);
> +		break;
> +	case V4L2_CID_DIGITAL_GAIN:
> +		cci_write(os02g10->cci, OS02G10_REG_DIGITAL_GAIN_L,
> +			  (ctrl->val & 0xff), &ret);
> +		cci_write(os02g10->cci, OS02G10_REG_DIGITAL_GAIN_H,
> +			  ((ctrl->val >> 8) & 0x7), &ret);

[Severity: High]
Does this bitwise mask handle the maximum digital gain value of 2048
correctly? 

When the user sets the digital gain to OS02G10_DIGITAL_GAIN_MAX (2048 or
0x0800), shifting right by 8 yields 0x08. Masking this with 0x7 evaluates
to 0. This writes 0x00 to both the low and high registers, unexpectedly
setting the gain to 0 instead of the maximum.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908114235.86568-1-elgin.perumbilly@siliconsignals.io?part=2