Re: [PATCH v2 2/3] media: i2c: add imx576 image sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Calculation of pixel_rate overflows on 32-bit platforms, causing wildly incorrect pixel rate control.
- [High] Sensor is permanently left in register HOLD mode if an intermediate control write fails.
- [Low] Unconditional assignment of dev_pm_ops produces dead code when CONFIG_PM is disabled.
--

commit 511969ab2e888f822293c83a79b2a96d1becf3a2
Author: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>

media: i2c: add imx576 image sensor driver

Add a v4l2 subdevice driver for the Sony imx576 sensor.
The Sony IMX576 image sensor with an active array size of 5760 x 4312.
The following features are supported: manual exposure and gain control,
vblank/hblank control, and 2880 x 2156 30fps (SRGGB10) resolution.

> diff --git a/drivers/media/i2c/imx576.c b/drivers/media/i2c/imx576.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/media/i2c/imx576.c
[ ... ]
> +static int imx576_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> +	switch (ctrl->id) {
> +	case V4L2_CID_VBLANK: {
> +		u64 vmax = fmt->height + ctrl->val;
> +
> +		cci_write(imx576->regmap, IMX576_REG_VTS, vmax, &ret);
> +		break;
> +	}
> +	case V4L2_CID_EXPOSURE:
> +		cci_write(imx576->regmap, IMX576_REG_EXPOSURE, ctrl->val, &ret);
> +		break;
> +	case V4L2_CID_ANALOGUE_GAIN:
> +		cci_write(imx576->regmap, IMX576_REG_ANALOG_GAIN,
> +			  ctrl->val, &ret);
> +		break;
> +	default:
> +		dev_err(imx576->dev, "Invalid control %d\n", ctrl->id);
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	cci_write(imx576->regmap, IMX576_REG_HOLD, 0, &ret);

[Severity: High]
Does this leave the sensor permanently in register HOLD mode if an
intermediate control write fails?

Looking at cci_write(), it immediately returns without performing any I2C
write if the error pointer (ret) already contains a non-zero error code. If an
intermediate control write in the switch block fails, ret will hold an error
code like -EIO.

The final cci_write() will see this error and instantly skip clearing the
HOLD register, leaving the camera hardware in a stuck state where no further
register updates take effect.

[ ... ]
> +static int imx576_init_controls(struct imx576 *imx576)
> +{
[ ... ]
> +	imx576->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx576_ctrl_ops,
> +					   V4L2_CID_VBLANK, vblank_def,
> +					   IMX576_VBLANK_MAX, 1,
> +					   vblank_def);
> +
> +	/* pixel_rate = link_frequency * 2 * nr_of_lanes / bits_per_sample */
> +	pixel_rate = div_u64(IMX576_LINK_FREQ_600MHZ * 2 * IMX576_NUM_DATA_LANES, 10);

[Severity: High]
Does this calculation overflow on 32-bit platforms before the result gets
passed to div_u64()?

IMX576_LINK_FREQ_600MHZ is defined as (600 * HZ_PER_MHZ), and since
HZ_PER_MHZ is an unsigned long, the entire multiplication is evaluated as a
32-bit unsigned long on 32-bit architectures.

The result of 600,000,000 * 8 is 4,800,000,000, which exceeds the maximum
32-bit unsigned integer value and overflows to 505,032,704 before div_u64()
implicitly casts it to u64.

Could this be explicitly cast to u64 before the multiplication to ensure the
correct pixel rate is calculated?

[ ... ]
> +static struct i2c_driver imx576_driver = {
> +	.driver = {
> +		.name = "imx576",
> +		.pm = &imx576_pm_ops,

[Severity: Low]
Will this unconditional assignment produce dead code when CONFIG_PM is
disabled?

Assigning the dev_pm_ops structure directly prevents the compiler from
eliminating the imx576_pm_ops structure when power management support
is disabled.

Should this be wrapped with pm_ptr() to allow the compiler to discard the
unused structure?

> +		.of_match_table = imx576_of_match,
> +	},
> +	.probe = imx576_probe,
> +	.remove = imx576_remove,
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619125439.55311-1-himanshu.bhavani@siliconsignals.io?part=2