Re: [PATCH v5 2/2] media: i2c: Add driver for AMSOSRMA Mira220

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Sashiko

On Tue, Jul 21, 2026 at 02:36:14PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
> - [High] Dereferencing `mira220->vblank` without checking for NULL in `mira220_init_controls()`.
> - [High] Missing `v4l2_subdev_cleanup()` in the driver's remove function causes an active state memory leak.
> - [Medium] The uninitialized stack variable `id` is evaluated and logged without checking if the I2C read succeeded.
> - [Medium] The active state format code is not updated when `V4L2_CID_HFLIP` or `V4L2_CID_VFLIP` are modified.
> - [Medium] Exposure control maximum limit exceeds the 16-bit hardware register width, leading to silent truncation.
> - [Medium] Hardware exposure register is not updated when `V4L2_CID_EXPOSURE` is clamped due to a `V4L2_CID_VBLANK` change.
> - [Medium] `mira220_power_on()` discards the actual error code when `clk_prepare_enable()` fails.
> - [Medium] Missing runtime PM reference hold during probe exposes the device to concurrent suspend.
> - [Medium] The error path in probe unconditionally powers off the device without checking its runtime PM state.
> - [Medium] V4L2_CID_HBLANK reports an incorrect static value when using a 1-lane MIPI CSI-2 configuration, breaking user-space framerate and exposure calculations.
> - [Low] Typo in user-facing control menu item: 'Vertial Gradient' instead of 'Vertical Gradient'.
> --

Thanks for your findings.

I'll go over them one by one

>
> --- Patch [2]: [PATCH v5 2/2] media: i2c: Add driver for AMSOSRMA Mira220 ---
> commit fbe78b3a27f19e95a842994664c11c10a075c4aa
> Author: Philippe Baetens <philippebaetens@gmail.com>
>
> media: i2c: Add driver for AMSOSRMA Mira220
>
> Add a V4L2 subdev driver for driver for the AMSOSRAM Mira220 image sensor.
>
> diff --git a/drivers/media/i2c/mira220.c b/drivers/media/i2c/mira220.c
> index 0000000000000..861f3230375c2
> --- /dev/null
> +++ b/drivers/media/i2c/mira220.c
> @@ -923,10 +923,10 @@ static const cci_reg_sequence mira220_init_reg_list[] = {
> [ ... ]
> > +static const char *const mira220_test_pattern_menu[] = {
> > +	"Disabled",
> > +	"Vertial Gradient",
> > +};
>
> [Severity: Low]
> Is there a typo in the menu item name? It seems like it should be Vertical
> Gradient instead of Vertial Gradient.
>

Thanks, fixed

> @@ -1023,12 +1023,12 @@ static int mira220_power_on(struct device *dev)
> [ ... ]
> > +	ret = clk_prepare_enable(mira220->xclk);
> > +	if (ret) {
> > +		dev_err(&client->dev, "%s: failed to enable clock\n", __func__);
> > +		goto reg_off;
> > +	}
> > +
> > +	gpiod_set_value_cansleep(mira220->reset_gpio, 0);
> > +
> > +	return 0;
> > +
> > +reg_off:
> > +	return regulator_bulk_disable(MIRA220_NUM_SUPPLIES, mira220->supplies);
> > +}
>
> [Severity: Medium]
> Will this discard the actual error code when clk_prepare_enable() fails?
>
> The function returns the result of regulator_bulk_disable(), which typically
> returns 0 on success.
>
> This could cause the caller to falsely assume the device was successfully
> powered on, potentially leading to confusing I2C read failures later in the
> probe process.

Ack, I can call regulator_bulk_disable() ignoring its return value and
return the error code from clk_prepare_enable().

>
> @@ -1113,5 +1113,5 @@ static int mira220_write_stop_streaming_regs(struct mira220 *mira220)
> [ ... ]
> > +static u32 mira220_calc_exposure(struct mira220 *mira220, u32 height, u32 vblank)
> > +{
> > +	return (height + vblank) -
> > +	       (int)(MIRA220_GLOB_NUM_CLK_CYCLES / mira220->row_length);
> > +}
>
> [Severity: Medium]
> Could this calculation cause the exposure limit to exceed the 16-bit register
> capacity?
>
> With vblank up to 65535, max_exposure can reach roughly 66929. When the value
> is written to the hardware register:
>
> ret = cci_write(mira220->regmap, MIRA220_EXP_TIME_REG, exposure, NULL);
>
> it might be silently truncated to 16 bits (e.g., 66929 becomes 1393), resulting
> in an incorrectly brief exposure time.

Good finding! With a max vblank of 0xffff we can certainly overflow
the exposure register. I will cap the max exposure value to 0xffff

>
> @@ -1155,10 +1155,10 @@ static int mira220_set_ctrl(struct v4l2_ctrl *ctrl)
> [ ... ]
> > +	if (ctrl->id == V4L2_CID_VBLANK) {
> > +		int exposure_max, exposure_def;
> > +
> > +		/* Update max exposure while meeting expected vblanking */
> > +		exposure_max = mira220_calc_exposure(mira220, format->height,
> > +						     ctrl->val);
> > +		exposure_def = min(exposure_max, MIRA220_DEFAULT_EXPOSURE);
> > +		ret = __v4l2_ctrl_modify_range(mira220->exposure,
> > +					       mira220->exposure->minimum,
> > +					       exposure_max,
> > +					       mira220->exposure->step,
> > +					       exposure_def);
> > +		if (ret)
> > +			return ret;
> > +	}
>
> [Severity: Medium]
> If the current exposure exceeds the new maximum here, the framework clamps it,
> but does this code miss updating the hardware exposure register?
>
> Since exposure and vblank are not clustered, mira220_set_ctrl() is not
> triggered for exposure, and only the vblank hardware register is updated.
>
> This could leave the hardware exposure register out-of-sync with the clamped
> value.

Am I mistaken or __v4l2_ctrl_modify_range() calls set_ctrl() if
ctrl->val is updated ?

>
> @@ -1177,10 +1177,10 @@ static int mira220_set_ctrl(struct v4l2_ctrl *ctrl)
> [ ... ]
> > +	case V4L2_CID_HFLIP:
> > +		ret = cci_write(mira220->regmap, MIRA220_HFLIP_REG,
> > +				mira220->hflip->val, NULL);
> > +		break;
> > +	case V4L2_CID_VFLIP:
> > +		ret = cci_write(mira220->regmap, MIRA220_VFLIP_REG,
> > +				mira220->vflip->val, NULL);
> > +		break;
>
> [Severity: Medium]
> Should the active state format code be updated when the layout-modifying
> controls change the underlying format?
>
> Modifying these controls alters the Bayer format. Without updating the active
> state state->pads[0].format.code, subsequent calls to get the format could
> return the stale format code without the flip applied.
>

yes it should (note that here it should be done and it's harmless
because we grab the flip controls during streaming).

I have added

	/* 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);

Before

	if (pm_runtime_get_if_in_use(&client->dev) <= 0)
		return 0;

And verified the format is updated when a flip is applied

$ v4l2-ctl -d /dev/v4l-subdev6 --get-subdev-fmt pad=0
ioctl: VIDIOC_SUBDEV_G_FMT (pad=0)
	Mediabus Code     : 0x3011 (MEDIA_BUS_FMT_SGRBG12_1X12)

$ v4l2-ctl -d /dev/v4l-subdev6 -c 'vertical_flip'=1

$ v4l2-ctl -d /dev/v4l-subdev6 --get-subdev-fmt pad=0
ioctl: VIDIOC_SUBDEV_G_FMT (pad=0)
	Mediabus Code     : 0x3008 (MEDIA_BUS_FMT_SBGGR12_1X12)


> @@ -1486,8 +1486,8 @@ static int mira220_identify_module(struct mira220 *mira220)
> [ ... ]
> > +	mira220_otp_read(mira220, 0x3a, 0, &id, &ret);
> > +	if (id < 1 || id > 2) {
> > +		dev_err(&client->dev,
> > +			"Read OTP 0x3a, id must be 1 or 2, but got: 0x%02x\n", id);
> > +		ret = -EINVAL;
> > +		goto err_power_off;
> > +	}
>
> [Severity: Medium]
> Does this evaluate the uninitialized id variable before validating the
> accumulated ret status?
>
> mira220_otp_read() is designed to skip execution if ret is already non-zero.
> If any previous OTP read failed, or if reading 0x3a itself fails, id is left
> uninitialized.
>
> This block could potentially log the uninitialized value before checking the
> error code in ret.

it can happen. I'll check for ret before accessing id.

>
> @@ -1837,12 +1837,12 @@ static int mira220_init_controls(struct mira220 *mira220)
> [ ... ]
> > +	hblank = v4l2_ctrl_new_std(ctrl_hdlr, NULL, V4L2_CID_HBLANK,
> > +				   MIRA220_HBLANK_1600x1400_304,
> > +				   MIRA220_HBLANK_1600x1400_304, 1,
> > +				   MIRA220_HBLANK_1600x1400_304);
>
> [Severity: Medium]
> Will this hardcoded hblank value report an incorrect static value when using a
> 1-lane MIPI CSI-2 configuration?
>
> It assumes a 2-lane layout where row_length is 304. If the device parses a
> 1-lane configuration, row_length doubles to 608, which would effectively
> double the line time, meaning hblank should be dynamically set to 4480.
>
> Because the control remains at 1440, user space might be misled into
> calculating a falsely high frame rate.
>

No, I don't think so.

doubling the row_length value programmed to the hardware maintains the
calculation stable with a single data lanes.

I've run some tests

$ cam -c2 -C100
18.330450 (8.46 fps) cam0-stream0 seq: 000041 bytesused: 1920000
18.448761 (8.45 fps) cam0-stream0 seq: 000042 bytesused: 1920000
18.567066 (8.45 fps) cam0-stream0 seq: 000043 bytesused: 1920000
18.685575 (8.44 fps) cam0-stream0 seq: 000044 bytesused: 1920000
18.803881 (8.45 fps) cam0-stream0 seq: 000045 bytesused: 1920000
18.922129 (8.46 fps) cam0-stream0 seq: 000046 bytesused: 1920000
19.040449 (8.45 fps) cam0-stream0 seq: 000047 bytesused: 1920000
19.158755 (8.45 fps) cam0-stream0 seq: 000048 bytesused: 1920000
19.277181 (8.44 fps) cam0-stream0 seq: 000049 bytesused: 1920000
19.395720 (8.44 fps) cam0-stream0 seq: 000050 bytesused: 1920000
19.513876 (8.46 fps) cam0-stream0 seq: 000051 bytesused: 1920000
19.632150 (8.45 fps) cam0-stream0 seq: 000052 bytesused: 1920000
19.750510 (8.45 fps) cam0-stream0 seq: 000053 bytesused: 1920000
19.869041 (8.44 fps) cam0-stream0 seq: 000054 bytesused: 1920000
19.987234 (8.46 fps) cam0-stream0 seq: 000055 bytesused: 1920000
20.105549 (8.45 fps) cam0-stream0 seq: 000056 bytesused: 1920000
20.223992 (8.44 fps) cam0-stream0 seq: 000057 bytesused: 1920000
20.342276 (8.45 fps) cam0-stream0 seq: 000058 bytesused: 1920000
20.460710 (8.44 fps) cam0-stream0 seq: 000059 bytesused: 1920000

The detected frame rate is ~8.45

Using the values reported in controls for calculating the frame rate

 v4l2-ctl -d /dev/v4l-subdev6 -l
            vertical_blanking 0x009e0901 (int)    : min=17 max=65535 step=1 default=17 value=13569 flags=0x00001000
            horizontal_blanking 0x009e0902 (int)    : min=1440 max=1440 step=1 default=1440 value=1440 flags=read-only, 0x00001000
            pixel_rate 0x009f0902 (int64)  : min=384000000 max=384000000 step=1 default=384000000 value=384000000 flags=read-only, 0x00001000

I get

        pixel_rate = 384000000
        llp = 1600 + 1440 = 3040
        fll = 1400 + 13569 = 14969
        384000000 / (3040 * 14969) = 8.438492182

Which matches the measured value

> > +	if (hblank)
> > +		hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > +	/* Max exposure is determined by vblank + vsize and Tglob. */
> > +	max_exposure = mira220_calc_exposure(mira220,
> > +					     MIRA220_PIXEL_ARRAY_HEIGHT,
> > +					     mira220->vblank->val);
>
> [Severity: High]
> Can mira220->vblank be NULL here?
>
> The v4l2_ctrl_new_std() call above could fail and return NULL if memory
> allocation fails.
>
> Dereferencing mira220->vblank->val immediately without verifying if the
> creation succeeded could cause a kernel panic during device probe.
>

I feel like this really is a corner case ?
I can bail out on !vblank though

> @@ -1936,6 +1936,6 @@ static int mira220_probe(struct i2c_client *client)
> [ ... ]
> > +	/* Enable runtime PM and turn off the device */
> > +	pm_runtime_set_autosuspend_delay(dev, 1000);
> > +	pm_runtime_use_autosuspend(dev);
> > +	pm_runtime_set_active(dev);
> > +	pm_runtime_enable(dev);
> > +
> > +	ret = mira220_identify_module(mira220);
>
> [Severity: Medium]
> Is it possible for the device to be concurrently suspended while probe
> continues?
>
> The device is marked active with a usage count of 0 after pm_runtime_enable().
> If the PM subsystem or userspace triggers a suspend (e.g. by setting
> autosuspend_delay_ms=0), the device could be powered down while probe executes
> mira220_identify_module().
>
> This could lead to I2C timeouts or bus crashes. Taking a pm_runtime reference
> hold could prevent this.
>

By calling pm_runtime_get_noresume() (as the device is powered up
already) ?


> @@ -1985,5 +1985,5 @@ static int mira220_probe(struct i2c_client *client)
> [ ... ]
> > +error_power_off:
> > +	pm_runtime_disable(dev);
> > +	mira220_power_off(dev);
> > +	pm_runtime_set_suspended(dev);
> > +	return ret;
> > +}
>
> [Severity: Medium]
> Should this error path check the runtime PM state before unconditionally
> powering off the device?
>
> The PM core might have already suspended the device due to the zero usage count.
>
> Calling mira220_power_off() unconditionally could redundantly disable clocks
> and regulators that are already disabled, potentially causing double-disable
> warnings.
>

Only in the case some suspend went on during probe. Even if unlikely,
guarding the call with

	if (!pm_runtime_status_suspended(&client->dev))
		mira220_power_off(&client->dev);

costs nothing

> @@ -1992,7 +1992,7 @@ static void mira220_remove(struct i2c_client *client)
> [ ... ]
> > +static void mira220_remove(struct i2c_client *client)
> > +{
> > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +	struct mira220 *mira220 = to_mira220(sd);
> > +
> > +	v4l2_async_unregister_subdev(sd);
> > +	media_entity_cleanup(&sd->entity);
> > +
> > +	v4l2_ctrl_handler_free(mira220->sd.ctrl_handler);
>
> [Severity: High]
> Does this unbind path miss a call to v4l2_subdev_cleanup()?
>
> The driver initializes the subdev active state via v4l2_subdev_init_finalize()
> during probe, but fails to clean it up in mira220_remove().
>
> This could permanently leak the memory allocated for the active state and pads
> every time the driver is unbound.

Ack, I should call v4l2_subdev_cleanup()

Thanks
  j
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260721-mira220-v5-0-533b0ad1065e@ideasonboard.com?part=2