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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect check of `pm_runtime_get_if_in_use()` return value leads to an unbalanced decrement of the runtime PM usage counter when runtime PM is disabled.
- [Medium] Claimed dynamic frame windowing is unreachable dead code due to fixed formats and a missing `.set_selection` pad operation.
--

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

media: i2c: Add driver for AMS-OSRAM Mira220

Add a V4L2 subdev driver for driver for the AMS-OSRAM Mira220 image sensor.
The driver implements support for mono and RGB 12, 10 and 8 bits formats.

> diff --git a/drivers/media/i2c/mira220.c b/drivers/media/i2c/mira220.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/media/i2c/mira220.c
[ ... ]
> +static int mira220_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> +	/* 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]
Can this lead to an unbalanced decrement of the runtime PM usage counter?

When pm_runtime_get_if_in_use() is called while the device's runtime PM is
disabled, it returns -EINVAL. Because -EINVAL is non-zero, the check here
evaluates to false and the function proceeds to the switch statement.

This causes the function to unconditionally call pm_runtime_put_autosuspend()
at the end, decrementing the usage counter and potentially causing an
underflow. Would it be safer to explicitly check for positive values, such as
if (pm_runtime_get_if_in_use(&client->dev) <= 0)?

> +
> +	switch (ctrl->id) {
[ ... ]
> +	}
> +
> +	pm_runtime_put_autosuspend(&client->dev);
> +
> +	return ret;
> +}
[ ... ]
> +static int mira220_set_framefmt(struct mira220 *mira220,
> +				struct v4l2_subdev_state *state)
> +{
[ ... ]
> +	/*
> +	 * Program the ROI area, centered in the sensor's pixel array.
> +	 *
> +	 * TODO: Use the crop rectangle sizes once the driver is ported to the
> +	 * new RAW camera sensor model.
> +	 */
> +	top = (MIRA220_PIXEL_ARRAY_HEIGHT - format->height) / 2;
> +	left = (MIRA220_PIXEL_ARRAY_WIDTH - format->width) / 2;
> +
> +	cci_write(mira220->regmap, MIRA220_VSTART_REG, top, &ret);

[Severity: Medium]
Is this dynamic windowing logic currently unreachable?

The patch description mentions that the driver dynamically programs frame
windowing. However, looking at mira220_update_pad_format(), the format width
and height are hardcoded to MIRA220_PIXEL_ARRAY_WIDTH and
MIRA220_PIXEL_ARRAY_HEIGHT.

Since mira220_pad_ops below doesn't implement a .set_selection callback,
userspace cannot configure the crop rectangle. Does this mean the top and
left offsets calculated here will always evaluate to zero, making this
calculation dead code for now?

> +	cci_write(mira220->regmap, MIRA220_HSTART_REG,
> +		  left & MIRA220_HSTART_MASK, &ret);
[ ... ]
> +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,
> +	.enum_frame_size = mira220_enum_frame_size,
> +	.enable_streams = mira220_enable_streams,
> +	.disable_streams = mira220_disable_streams,
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-mira220-v11-0-1295fe17021d@ideasonboard.com?part=2