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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unbalanced `pm_runtime_put()` in `imx678_set_ctrl()` leads to PM usage count underflow.
- [Medium] Missing dependency on `I2C` in Kconfig for `VIDEO_IMX678`.
- [Medium] I2C error code is shadowed by a chip ID mismatch error during probe.
- [Medium] Sensor is incorrectly left in streaming mode during probe.
- [Low] Unconditional assignment of `dev_pm_ops` structure without `pm_ptr()` wrapper.
--

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

Add a V4L2 subdev driver for the Sony IMX678 image sensor. This driver
supports features like variable framerates, cropping, binning, flipping,
test patterns, and exposure control.

> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
[ ... ]
> +config VIDEO_IMX678
> +	tristate "Sony IMX678 sensor support"
> +	depends on OF_GPIO
> +	select V4L2_CCI_I2C

V4L2_CCI_I2C inherently depends on I2C. Does this symbol also need a
"depends on I2C" constraint to prevent unmet direct dependency warnings
during builds?

> diff --git a/drivers/media/i2c/imx678.c b/drivers/media/i2c/imx678.c
> --- /dev/null
> +++ b/drivers/media/i2c/imx678.c
[ ... ]
> +static int imx678_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> +	/*
> +	 * Applying V4L2 control value only happens
> +	 * when power is up for streaming
> +	 */
> +	if (pm_runtime_get_if_in_use(&client->dev) == 0)
> +		return 0;

Can this cause a PM usage counter underflow? When runtime PM is disabled
(e.g. during system suspend), pm_runtime_get_if_in_use() returns a negative
error code like -EINVAL. Because -EINVAL != 0, the check passes, and
execution continues to the end of the function where pm_runtime_put() is
called without a corresponding increment.

Should this check for <= 0 instead?

[ ... ]
> +static int imx678_identify_model(struct imx678 *imx678)
> +{
[ ... ]
> +	/*
> +	 * 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);

Is it intentional to leave the sensor in streaming mode (0) here? If runtime
PM is disabled or delayed, could this cause the sensor to actively transmit
data continuously after probe?

> +
> +	cci_read(imx678->cci, IMX678_REG_MODULE_ID, &val, &ret);
> +
> +	if (val != IMX678_ID) {
> +		dev_err(&client->dev,
> +			"Chip ID mismatch: %x!=%llx\n", IMX678_ID, val);
> +		return -ENXIO;
> +	}

If the cci_read() fails due to an I2C error, it sets val to 0 and returns an
error code in ret. Because val is 0, the check above will evaluate to true
and return -ENXIO, masking the actual I2C error code.

Should we check if ret is non-zero before validating the chip ID?

[ ... ]
> +static struct i2c_driver imx678_i2c_driver = {
> +	.driver = {
> +		.name = "imx678",
> +		.of_match_table = imx678_of_match,
> +		.pm = &imx678_pm_ops,
> +	},

This isn't a bug, but if CONFIG_PM is disabled, the imx678_pm_ops structure
becomes dead code. Should this use the pm_ptr() wrapper to ensure the pointer
evaluates to NULL and the structure can be optimized away?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-imx678-v2-0-4854ac61d6fb@ideasonboard.com?part=2