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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] VBLANK and EXPOSURE controls are not clustered, leading to out-of-sync hardware registers when exposure is clamped.
- [High] Unconditional destructive reset of the VBLANK control in `mira220_set_pad_format()`.
- [Medium] Incorrect runtime PM initialization causes immediate synchronous suspend and race conditions.
- [Medium] Missing stabilization delay after de-asserting the device reset GPIO.
--

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

This commit introduces a new V4L2 subdevice driver for the AMS-OSRAM Mira220
image sensor. It implements support for mono and RGB variants with resolutions
up to 1600x1400 at 90fps via a MIPI CSI-2 interface.

> diff --git a/drivers/media/i2c/mira220.c b/drivers/media/i2c/mira220.c
> new file mode 100644
> index 0000000000000..40462adc64eb4
> --- /dev/null
> +++ b/drivers/media/i2c/mira220.c
[ ... ]
> +static int mira220_power_on(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct mira220 *mira220 = to_mira220(sd);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(mira220_supplies),
> +				    mira220->supplies);
> +	if (ret) {
> +		dev_err(&client->dev, "%s: failed to enable regulators\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	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;

[Severity: Medium]
Could the lack of a stabilization delay here cause unreliable probe failures?

The hardware is taken out of reset and mira220_identify_module() immediately
begins I2C OTP reads, but most image sensors require a documented
stabilization period before their I2C interfaces can reliably acknowledge
traffic.

[ ... ]
> +static int mira220_set_pad_format(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_state *state,
> +				  struct v4l2_subdev_format *fmt)
> +{
> +	struct mira220 *mira220 = to_mira220(sd);
> +	struct v4l2_rect *crop;
> +	u32 min_vblank;
> +	int ret;
> +
> +	mira220_update_pad_format(mira220, &fmt->format, fmt->format.code);
> +	*v4l2_subdev_state_get_format(state, 0) = fmt->format;
> +
> +	crop = v4l2_subdev_state_get_crop(state, 0);
> +	crop->width = fmt->format.width;
> +	crop->height = fmt->format.height;
> +	crop->left = MIRA220_PIXEL_ARRAY_LEFT;
> +	crop->top = MIRA220_PIXEL_ARRAY_TOP;
> +
> +	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> +		return 0;
> +
> +	/* Update vblank based on new mode. */
> +	min_vblank = mira220_calc_min_vblank(mira220);
> +	ret = __v4l2_ctrl_modify_range(mira220->vblank, min_vblank,
> +				       MIRA220_MAX_VBLANK, 1, min_vblank);
> +	if (ret)
> +		return ret;
> +
> +	return __v4l2_ctrl_s_ctrl(mira220->vblank, min_vblank);
> +}

[Severity: High]
Does this code unintentionally discard the user's explicitly requested frame
rate?

Calling __v4l2_ctrl_s_ctrl() unconditionally forces the VBLANK control to
min_vblank whenever the format is negotiated or set, which overwrites any
custom settings previously established by the user.

[ ... ]
> +static int mira220_init_controls(struct mira220 *mira220)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&mira220->sd);
> +	struct v4l2_ctrl_handler *ctrl_hdlr;
> +	struct v4l2_fwnode_device_properties props;
> +	struct v4l2_ctrl *ctrl;
> +	u32 max_exposure = 0;
> +	u32 min_vblank;
> +	u32 hblank_val;
> +	int ret;
> +
> +	ctrl_hdlr = &mira220->ctrl_handler;
> +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
> +	if (ret)
> +		return ret;
> +
> +	/* By default, PIXEL_RATE is read only */
> +	v4l2_ctrl_new_std(ctrl_hdlr, &mira220_ctrl_ops, V4L2_CID_PIXEL_RATE,
> +			  MIRA220_PIXEL_RATE, MIRA220_PIXEL_RATE, 1,
> +			  MIRA220_PIXEL_RATE);
> +
> +	min_vblank = mira220_calc_min_vblank(mira220);
> +	mira220->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &mira220_ctrl_ops,
> +					    V4L2_CID_VBLANK,
> +					    min_vblank, MIRA220_MAX_VBLANK, 1,
> +					    min_vblank);
> +
> +	ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr, NULL, V4L2_CID_LINK_FREQ,
> +				      0, 0, &mira220_link_freqs[0]);
> +	if (ctrl)
> +		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	/*
> +	 * Scale hblank according to the number of enabled data lanes to match
> +	 * row_length.
> +	 */
> +	hblank_val = MIRA220_LLP_1600x1400_304 * (2 / mira220->lanes)
> +		   - MIRA220_PIXEL_ARRAY_WIDTH;
> +	ctrl = v4l2_ctrl_new_std(ctrl_hdlr, NULL, V4L2_CID_HBLANK, hblank_val,
> +				 hblank_val, 1, hblank_val);
> +	if (ctrl)
> +		ctrl->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,
> +					     min_vblank);
> +
> +	mira220->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &mira220_ctrl_ops,
> +					      V4L2_CID_EXPOSURE,
> +					      MIRA220_EXPOSURE_MIN,
> +					      max_exposure, 1,
> +					      MIRA220_DEFAULT_EXPOSURE);

[Severity: High]
Should VBLANK and EXPOSURE be registered as a control cluster using
v4l2_ctrl_cluster()?

When VBLANK is modified in mira220_set_ctrl(), it updates the exposure bounds.
If the current exposure exceeds the new maximum, the V4L2 core clamps the
software value. Without a cluster, the s_ctrl handler for EXPOSURE is not
invoked, meaning the clamped value is never written to hardware.

[ ... ]
> +static int mira220_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct mira220 *mira220;
> +	int ret;
[ ... ]
> +	ret = v4l2_async_register_subdev_sensor(&mira220->sd);
> +	if (ret < 0) {
> +		dev_err_probe(dev, ret,
> +			      "failed to register sensor sub-device\n");
> +		goto error_subdev_cleanup;
> +	}
> +
> +	pm_runtime_idle(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	return 0;

[Severity: Medium]
Is it intentional to call pm_runtime_idle() before configuring autosuspend?

Because the usage count is 0 and autosuspend is not yet active, this triggers
an immediate synchronous suspend, defeating the 1000ms delay and causing an
unnecessary power cycle during the probe path.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-mira220-v10-0-8825794e6095@ideasonboard.com?part=2