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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] media: i2c: imx681: missing clock rate validation
- [Medium] media: i2c: imx681: flip controls silently ignored during streaming
--

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

media: i2c: Add Sony IMX681 sensor driver
    
Add a V4L2 sub-device driver for the Sony IMX681, a 10-bit Bayer sensor
on a two-lane CSI-2 bus. It offers a single 3844x2640 mode at 30 fps and
controls for exposure, analogue and digital gain, vertical blanking,
both flips and the sensor's test patterns.

> diff --git a/drivers/media/i2c/imx681.c b/drivers/media/i2c/imx681.c
> new file mode 100644
> index 0000000000000..3be96965c53ec
> --- /dev/null
> +++ b/drivers/media/i2c/imx681.c
> @@ -0,0 +1,1003 @@

[ ... ]

> +static int imx681_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct imx681 *imx681 = container_of(ctrl->handler, struct imx681,
> +					     ctrl_handler);

[ ... ]

> +	switch (ctrl->id) {

[ ... ]

> +	case V4L2_CID_HFLIP:
> +	case V4L2_CID_VFLIP:
> +		/* Read from imx681_start_streaming(); the layout changes. */
> +		break;

[Severity: Medium]
Will this silently ignore flip control changes while the sensor is streaming?

When userspace modifies V4L2_CID_HFLIP or V4L2_CID_VFLIP via VIDIOC_S_CTRL
during streaming, returning 0 here without updating the hardware tells the
V4L2 core the change succeeded. This can cause the V4L2 control state to
diverge from the actual hardware state, breaking pipeline format validation
because the reported Bayer format no longer matches the sensor's actual
output.

Should the driver call v4l2_ctrl_grab(..., true) when streaming starts to
prevent these modifications?

[ ... ]

> +static int imx681_probe(struct i2c_client *client)
> +{
> +	struct imx681 *imx681;
> +	int ret;
> +
> +	imx681 = devm_kzalloc(&client->dev, sizeof(*imx681), GFP_KERNEL);
> +	if (!imx681)
> +		return -ENOMEM;

[ ... ]

> +	/* Initialise CCI regmap for 16-bit register addresses */
> +	imx681->cci = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(imx681->cci))
> +		return dev_err_probe(imx681->dev, PTR_ERR(imx681->cci),
> +				     "failed to init CCI\n");
> +
> +	/* Get clock (optional - INT3472 provides it on Surface devices) */
> +	imx681->xclk = devm_clk_get_optional(imx681->dev, NULL);

[Severity: Medium]
Does this clock need rate validation?

The sensor's PLL and CSI-2 link timings appear to be configured assuming an
exact 19.2 MHz input clock. If a different clock rate is provided (e.g.,
24 MHz from a device tree or ACPI configuration), this probe will silently
succeed, but the sensor might output garbage or fail to link.

Should we check the rate using clk_get_rate() here?

> +	if (IS_ERR(imx681->xclk))
> +		return dev_err_probe(imx681->dev, PTR_ERR(imx681->xclk),
> +				     "failed to get clock\n");

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