Re: [PATCH v5 2/3] media: i2c: Add Sony IMX681 sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] media: i2c: imx681: non-atomic control updates due to missing cluster
--

commit acaa5eee3740a4ac3a0c239fabfd5d066ae26848
Author: Andre Gilerson <andre.gilerson@gmail.com>

media: i2c: Add Sony IMX681 sensor driver

This commit adds support for the Sony IMX681 sensor found on Intel Lunar Lake 
devices like the Surface Pro 11. It configures the sensor using 
reverse-engineered I2C sequences and exposes controls via the V4L2 API.

> diff --git a/drivers/media/i2c/imx681.c b/drivers/media/i2c/imx681.c
> new file mode 100644
> index 0000000000000..825d8c80a29b8
> --- /dev/null
> +++ b/drivers/media/i2c/imx681.c
[ ... ]
> +static int imx681_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> +	switch (ctrl->id) {
> +	case V4L2_CID_VBLANK:
> +		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x01, &ret);
> +		cci_write(imx681->cci, IMX681_REG_FRAME_LENGTH,
> +			  IMX681_HEIGHT + ctrl->val, &ret);
> +		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x00, &ret);
> +		dev_dbg(imx681->dev, "set frame_length: %d, ret=%d\n",
> +			IMX681_HEIGHT + ctrl->val, ret);
> +		break;
> +
> +	case V4L2_CID_EXPOSURE:
> +		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x01, &ret);
> +		cci_write(imx681->cci, IMX681_REG_EXPOSURE, ctrl->val, &ret);
> +		cci_write(imx681->cci, IMX681_REG_GROUP_HOLD, 0x00, &ret);

[Severity: Medium]
Does this break hardware atomicity during control updates?

The group hold is toggled independently for every control modification. If a
frame readout boundary occurs between these separate I2C transactions, could
the sensor latch a partial state, causing image tearing or brightness glitches?

[ ... ]
> +static int imx681_init_controls(struct imx681 *imx681)
> +{
[ ... ]
> +	/* Exposure */
> +	imx681->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx681_ctrl_ops,
> +					     V4L2_CID_EXPOSURE,
> +					     IMX681_EXPOSURE_MIN,
> +					     IMX681_EXPOSURE_MAX, 1,
> +					     IMX681_EXPOSURE_DEFAULT);
> +
> +	/* Analog gain */
> +	v4l2_ctrl_new_std(ctrl_hdlr, &imx681_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> +			  IMX681_ANA_GAIN_MIN, IMX681_ANA_GAIN_MAX, 1,
> +			  IMX681_ANA_GAIN_DEFAULT);
> +
> +	/* Digital gain */
> +	v4l2_ctrl_new_std(ctrl_hdlr, &imx681_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
> +			  IMX681_DIG_GAIN_MIN, IMX681_DIG_GAIN_MAX, 1,
> +			  IMX681_DIG_GAIN_DEFAULT);

[Severity: Medium]
Are these related controls missing a call to v4l2_ctrl_cluster()?

Because vblank, exposure, and the gains are instantiated but never clustered,
the V4L2 core will invoke imx681_set_ctrl() sequentially for each individual
control rather than passing them as a single atomic update.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921192450.21811-1-lsa.uz@pm.me?part=2