Re: [PATCH v5 2/2] media: i2c: imx678: Add driver for Sony IMX678

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Sensor is left in STREAMING mode after model identification, leading to power drain and potential hardware state issues.
- [Low] Incompatible pointer type warning when passing `const u64 *` to `v4l2_ctrl_new_int_menu()` and `v4l2_link_freq_to_bitmap()` which expect `const s64 *`.
--

commit faaa0977c251c6b8135131c5875cd627b67a4017
Author: Jai Luthra <jai.luthra@ideasonboard.com>
Subject: media: i2c: imx678: Add driver for Sony IMX678

This commit adds a V4L2 subdev driver for the Sony IMX678 image sensor. It
provides support for MIPI RAW12 output, various clock and link frequencies,
along with basic V4L2 controls.

> diff --git a/drivers/media/i2c/imx678.c b/drivers/media/i2c/imx678.c
> new file mode 100644
> index 0000000000000..83626c3eb744b
> --- /dev/null
> +++ b/drivers/media/i2c/imx678.c

[ ... ]

> +static const u64 link_freqs[] = {
> +	[IMX678_LINK_FREQ_297MHZ]  = 297000000,
> +	[IMX678_LINK_FREQ_360MHZ]  = 360000000,
> +	[IMX678_LINK_FREQ_445MHZ]  = 445500000,
> +	[IMX678_LINK_FREQ_594MHZ]  = 594000000,
> +	[IMX678_LINK_FREQ_720MHZ]  = 720000000,
> +	[IMX678_LINK_FREQ_891MHZ]  = 891000000,
> +	[IMX678_LINK_FREQ_1039MHZ] = 1039500000,
> +	[IMX678_LINK_FREQ_1188MHZ] = 1188000000,
> +};

[ ... ]

> +static int imx678_init_controls(struct imx678 *imx678)
> +{

[ ... ]

> +	/* LINK_FREQ is also read only */
> +	link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &imx678_ctrl_ops,
> +					   V4L2_CID_LINK_FREQ,
> +					   ARRAY_SIZE(link_freqs) - 1,
> +					   __ffs(imx678->link_freq_bitmap),
> +					   link_freqs);

[Severity: Low]
Will this cause a build failure due to incompatible pointer types?

The array link_freqs is defined as u64, but v4l2_ctrl_new_int_menu() expects
a const s64 pointer for its qmenu_int parameter. A similar issue exists below 
where link_freqs is passed to v4l2_link_freq_to_bitmap() in 
imx678_check_hwcfg(), which also expects an s64 array.

Should link_freqs be defined as s64 instead?

[ ... ]

> +static int imx678_identify_model(struct imx678 *imx678)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx678->sd);
> +	const struct imx678_model_info *info;
> +	enum imx678_type detected;
> +	int ret = 0;
> +	u64 val = 0;
> +
> +	info = device_get_match_data(&client->dev);
> +
> +	/*
> +	 * This sensor's ID registers become accessible 80ms after coming out
> +	 * of STANDBY mode.
> +	 */
> +	cci_write(imx678->cci, IMX678_REG_MODE_SELECT, 0, &ret);
> +	fsleep(IMX678_MODULE_ID_DELAY);
> +
> +	cci_read(imx678->cci, IMX678_REG_MODULE_ID, &val, &ret);

[Severity: Medium]
Is the sensor left in STREAMING mode after model identification completes?

The code writes 0 to IMX678_REG_MODE_SELECT to wake the sensor to read the
ID registers, but it doesn't appear to write 1 back to return it to STANDBY
mode before exiting the imx678_identify_model() function.

Could this leave the sensor streaming indefinitely on the bus if PM is
disabled, or cause configuration issues later when s_stream(1) is called
since the driver will be writing configuration while actively streaming?

> +
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"I2C transaction failed ret = %d\n", ret);
> +		return ret;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-imx678-v5-0-0523dbed0dad@ideasonboard.com?part=2