Re: [PATCH v3 09/20] media: imx355: pixel_rate never changes, so don't recompute

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Double power-off on the probe error path leads to unbalanced clock and regulator disables.
--

--- Patch [9]: [PATCH v3 09/20] media: imx355: pixel_rate never changes, so don't recompute ---
commit 7b1b03fa06b5d964ffe6862ea61125935053b34e
Author: Dave Stevenson <dave.stevenson@raspberrypi.com>

media: imx355: pixel_rate never changes, so don't recompute

This commit removes the recalculation of the pixel rate in
imx355_set_pad_format, as it remains constant. It also removes the stored
pointer to the pixel_rate control in the imx355 structure since it is no
longer needed.

> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> index 2880e03b3f08a..2a97e076bff76 100644
> --- a/drivers/media/i2c/imx355.c
> +++ b/drivers/media/i2c/imx355.c
[ ... ]
> @@ -1180,9 +1175,8 @@ static int imx355_init_controls(struct imx355 *imx355)
>  	pixel_rate = IMX355_LINK_FREQ_DEFAULT * 2 * 4;
>  	do_div(pixel_rate, 10);
>  	/* By default, PIXEL_RATE is read only */
> -	imx355->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx355_ctrl_ops,
> -					       V4L2_CID_PIXEL_RATE, pixel_rate,
> -					       pixel_rate, 1, pixel_rate);
> +	v4l2_ctrl_new_std(ctrl_hdlr, &imx355_ctrl_ops, V4L2_CID_PIXEL_RATE,
> +			  pixel_rate, pixel_rate, 1, pixel_rate);
>  
>  	/* Initialize vblank/hblank/exposure parameters based on current mode */
>  	mode = imx355->cur_mode;

[Severity: High]
This is a pre-existing issue, but while reviewing this file I noticed a
potential double power-off bug in the probe error path.

If v4l2_async_register_subdev_sensor() fails in imx355_probe(), the code
jumps to the error handler:

drivers/media/i2c/imx355.c:imx355_probe() {
    ...
    pm_runtime_idle(imx355->dev);
    pm_runtime_set_autosuspend_delay(imx355->dev, 1000);
    pm_runtime_use_autosuspend(imx355->dev);

    ret = v4l2_async_register_subdev_sensor(&imx355->sd);
    if (ret < 0)
        goto error_media_entity_runtime_pm;
    ...
error_media_entity_runtime_pm:
    ...
error_power_off:
    imx355_power_off(imx355->dev);
    ...
}

Since pm_runtime_idle() synchronously suspends the device when the usage
count is zero (which calls imx355_power_off()), won't falling through to
error_power_off and calling imx355_power_off() again lead to unbalanced
clock and regulator disables?

If regulators or clocks are shared with other devices, could this extra
disable drop the use count to zero and inadvertently power off resources
that other active devices are relying on?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-media-imx355-v3-0-9df386a623d7@raspberrypi.com?part=9