Re: [PATCH v2 07/20] media: imx355: Set binning mode registers programmatically

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The arguments for 'length' and 'value' are swapped in the `imx355_write_reg()` call for `IMX355_REG_LLP`, breaking sensor streaming.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it causes a double-disable of regulators and clocks in the probe error path due to unconditional power off after PM runtime idle.
--

--- Patch [7]: [PATCH v2 07/20] media: imx355: Set binning mode registers programmatically ---
commit 1bc86e7b2e33db0b1ab1611b670669f99ca08232
Author: Dave Stevenson <dave.stevenson@raspberrypi.com>

media: imx355: Set binning mode registers programmatically
    
Compute the binning registers based on the difference between
the mode width/height vs the crop width/height.

> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> index 66e70650e666b..cfc47b5100c46 100644
> --- a/drivers/media/i2c/imx355.c
> +++ b/drivers/media/i2c/imx355.c
[ ... ]
> @@ -1018,6 +981,20 @@ static int imx355_start_streaming(struct imx355 *imx355)
>  	if (ret)
>  		return ret;
>  

[Severity: High]
Are the arguments for value and length swapped in the imx355_write_reg() call
for IMX355_REG_LLP?

Looking at the code just before this addition in imx355_start_streaming():

	/* set line length */
	ret = imx355_write_reg(imx355, IMX355_REG_LLP,
			       imx355->hblank->val + imx355->cur_mode->width,
			       2);

The length parameter receives the computed line length (which can be in the
thousands), while imx355_write_reg() strictly enforces a maximum length of 4.
Will this cause imx355_start_streaming() to immediately fail with -EINVAL and
prevent the sensor from starting?

> +	binning_mode = ((mode->crop.width / mode->width) << 4) |
> +			(mode->crop.height / mode->height);
> +	ret = imx355_write_reg(imx355, IMX355_REG_BINNING_MODE, 1,
> +			       binning_mode == 0x11 ? 0x00 : 0x01);
> +	if (ret)
> +		return ret;
[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
imx355_probe() unconditionally cause a double-disable of regulators and
clocks?

When pm_runtime_idle() is called with a usage count of 0, it can invoke
imx355_power_off() to physically power down the sensor. If
v4l2_async_register_subdev_sensor() subsequently fails, the error path
blindly calls imx355_power_off() a second time:

drivers/media/i2c/imx355.c:imx355_probe() {
    ...
error_power_off:
	imx355_power_off(imx355->dev);
    ...
}

Will this lead to an unbalanced disable, potentially causing issues if the
clocks or regulators are shared and actively used by another subsystem?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-media-imx355-v2-0-1683ec07b897@raspberrypi.com?part=7