Re: [PATCH v7 2/2] media: i2c: Add driver for AMS-OSRAM Mira220

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mira220_set_ctrl() calls pm_runtime_put_autosuspend() when pm_runtime_get_if_in_use() returns a negative error code, leading to an unbalanced usage counter decrement.
- [Medium] Bayer pattern format lookup incorrectly inverts the horizontal flip state.
- [Medium] The driver intends to support dynamic frame windowing via the .set_selection API, but a missing implementation and hardcoded format limits completely break this functionality.
--

--- Patch [2]: [PATCH v7 2/2] media: i2c: Add driver for AMS-OSRAM Mira220 ---
commit 77ddbce8591f62cf6c1a56ccec765a03bc96c140
Author: Philippe Baetens <philippebaetens@gmail.com>

media: i2c: Add driver for AMS-OSRAM Mira220

This commit adds a V4L2 subdev driver for the AMS-OSRAM Mira220 image sensor.
The driver implements support for monochrome and RGB variants, dynamically
handling CSI-2 configurations and various image formats.

> diff --git a/drivers/media/i2c/mira220.c b/drivers/media/i2c/mira220.c
> new file mode 100644
> index 0000000000000..5f48648649269
> --- /dev/null
> +++ b/drivers/media/i2c/mira220.c

[ ... ]

> @@ -0,0 +1,2068 @@
> +/* Get bayer order based on flip setting. */
> +static u32 mira220_get_format_code(struct mira220 *mira220, u32 code)
> +{
> +	unsigned int i;
> +
> +	if (mira220_is_mono(mira220)) {
> +		/* for now only support Y10. */
> +		for (i = 0; i < ARRAY_SIZE(mira220_mbus_mono_formats); i++) {
> +			if (mira220_mbus_mono_formats[i] == code)
> +				return mira220_mbus_mono_formats[i];
> +		}
> +
> +		return mira220_mbus_mono_formats[0];
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(mira220_mbus_color_formats); i++) {
> +		if (mira220_mbus_color_formats[i] == code)
> +			break;
> +	}
> +	if (i >= ARRAY_SIZE(mira220_mbus_color_formats))
> +		i = 0;
> +
> +	i = (i & ~3) | (mira220->vflip->val ? 2 : 0) | (mira220->hflip->val ? 0 : 1);

[Severity: Medium]
Is the horizontal flip logic inverted here? 

If hflip->val is 0 (disabled), this evaluates to 1 and adds the offset for the 
horizontally flipped Bayer format. This might cause userspace to receive an 
incorrect Bayer pattern and swap colors during debayering when horizontal flip 
is left at its default value.

> +
> +	return mira220_mbus_color_formats[i];
> +}
> +
> +static int mira220_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct mira220 *mira220 =
> +		container_of(ctrl->handler, struct mira220, ctrl_handler);
> +	struct i2c_client *client = v4l2_get_subdevdata(&mira220->sd);
> +	struct v4l2_mbus_framefmt *format;
> +	struct v4l2_subdev_state *state;
> +	int ret = 0;
> +
> +	state = v4l2_subdev_get_locked_active_state(&mira220->sd);
> +	format = v4l2_subdev_state_get_format(state, 0);

[ ... ]

> +	/* Update the format code to adjust it to the new flip state. */
> +	if (ctrl->id == V4L2_CID_VFLIP || ctrl->id == V4L2_CID_HFLIP)
> +		format->code = mira220_get_format_code(mira220, format->code);
> +
> +	if (!pm_runtime_get_if_in_use(&client->dev))
> +		return 0;

[Severity: High]
Could this check fail to handle negative error codes? 

pm_runtime_get_if_in_use() can return -EINVAL if runtime PM is disabled. If it 
returns a negative value, the ! operator evaluates to false. This allows the 
function to proceed to hardware accesses and eventually call 
pm_runtime_put_autosuspend() without a successful get, leading to an 
unbalanced usage counter.

Should this check for <= 0 instead?

> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_EXPOSURE:
> +		ret = mira220_write_exposure_reg(mira220, ctrl->val);
> +		break;

[ ... ]

> +static const struct v4l2_subdev_video_ops mira220_video_ops = {
> +	.s_stream = v4l2_subdev_s_stream_helper,
> +};
> +
> +static const struct v4l2_subdev_pad_ops mira220_pad_ops = {
> +	.enum_mbus_code = mira220_enum_mbus_code,
> +	.get_fmt = v4l2_subdev_get_fmt,
> +	.set_fmt = mira220_set_pad_format,
> +	.get_selection = mira220_get_selection,

[Severity: Medium]
Should this structure include a .set_selection callback? 

The comments in mira220_update_pad_format() mention that the correct API to 
configure windowing is the .set_selection one, but it appears to be missing 
here. Since mira220_set_framefmt() also forces calculations based on the max 
format dimensions, this omission might leave the crop rectangle immutable and 
lock the sensor to its maximum resolution.

> +	.enum_frame_size = mira220_enum_frame_size,
> +	.enable_streams = mira220_enable_streams,
> +	.disable_streams = mira220_disable_streams,
> +};
> +
> +static const struct v4l2_subdev_ops mira220_subdev_ops = {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-mira220-v7-0-dd5f1de98cd4@ideasonboard.com?part=2