Re: [PATCH 2/2] media: i2c: Add a driver for Sony IMX708

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect check of `pm_runtime_get_if_in_use()` return value in `imx708_set_ctrl()` leads to unbalanced runtime PM puts and hardware access while powered off.
- [Medium] Incorrect power-off sequence in `imx708_power_off()`: regulators are disabled before the input clock.
- [Medium] Software state (`imx708->long_exp_shift`) is updated before hardware confirms the write, leading to state mismatch on I2C failure.
--

--- Patch [2]: [PATCH 2/2] media: i2c: Add a driver for Sony IMX708 ---
commit 34adccd1a70befee0909b84ab4c6054b6de7c8c3
Author: Jai Luthra <jai.luthra@ideasonboard.com>

media: i2c: Add a driver for Sony IMX708

This commit introduces a new V4L2 I2C driver for the Sony IMX708 11.9
Megapixel CMOS sensor. It adds basic support for capturing the full active
area and includes controls for exposure, analogue gain, test patterns, and
variable frame rates using VBLANK.

> diff --git a/drivers/media/i2c/imx708.c b/drivers/media/i2c/imx708.c
> new file mode 100644
> index 0000000000000..052474befc5e4
> --- /dev/null
> +++ b/drivers/media/i2c/imx708.c
[ ... ]
> +static int imx708_set_frame_length(struct imx708 *imx708, unsigned int val)
> +{
> +	int ret = 0;
> +
> +	imx708->long_exp_shift = 0;
> +
> +	while (val > IMX708_FRAME_LENGTH_MAX) {
> +		imx708->long_exp_shift++;
> +		val >>= 1;
> +	}
> +
> +	cci_write(imx708->cci, IMX708_REG_FRAME_LENGTH, val, &ret);

[Severity: Medium]
Does this leave the software state out of sync if the I2C transaction fails?

The driver mutates imx708->long_exp_shift before issuing the cci_write()
commands. If cci_write() returns an error, the V4L2 framework rolls back
the VBLANK control to its previous value, but the driver retains the
mutated long_exp_shift. Subsequent exposure calculations would then use
this corrupted shift value.

> +	cci_write(imx708->cci, IMX708_LONG_EXP_SHIFT_REG,
> +		  imx708->long_exp_shift, &ret);
> +
> +	return ret;
> +}
> +
> +static int imx708_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> +	/*
> +	 * Only apply control values when device is powered on (RPM ACTIVE)
> +	 * and streaming (usage count != 0)
> +	 */
> +	if (!pm_runtime_get_if_in_use(&client->dev))
> +		return 0;

[Severity: High]
Can this cause unbalanced runtime PM puts and hardware access while powered
off?

The pm_runtime_get_if_in_use() function returns -EINVAL if runtime PM is
disabled. In C, !(-EINVAL) evaluates to false, which causes the function to
bypass the early return. It then proceeds to access hardware registers and
calls pm_runtime_put_autosuspend(), decrementing a usage count that was
never incremented.

> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_ANALOGUE_GAIN:
> +		cci_write(imx708->cci, IMX708_REG_ANALOG_GAIN, ctrl->val, &ret);
[ ... ]
> +static int imx708_power_off(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx708 *imx708 = to_imx708(sd);
> +
> +	gpiod_set_value_cansleep(imx708->reset_gpio, 1);
> +	regulator_bulk_disable(ARRAY_SIZE(imx708_supply_name),
> +			       imx708->supplies);
> +	clk_disable_unprepare(imx708->inclk);

[Severity: Medium]
Should the clock be disabled before the regulators are turned off?

During device power down, regulators vana and vdig are turned off while the
input clock inclk is still running.

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-imx708-v1-0-1f36f54e14d4@ideasonboard.com?part=2