Re: [PATCH] media: i2c: ov5647: fix power cleanup on remove

[Dave Stevenson]

Hi Guangshuo

On Tue, 15 Sept 2026 at 10:05, Guangshuo Li <lgs201920130244@xxxxxxxxx> wrote:
>
> ov5647_remove() disables runtime PM without powering off the sensor if
> it is still runtime active or updating the runtime PM state to
> suspended.
>
> pm_runtime_disable() prevents further runtime PM callbacks and waits for
> pending operations, but it does not force the runtime suspend callback
> to run. If the sensor is active when the driver is removed, the external
> clock and regulators can remain enabled and the power-down GPIO can
> remain deasserted.
>
> After disabling runtime PM, call ov5647_power_off() if the device is not
> already runtime suspended, and then mark the runtime PM state as
> suspended. Checking the runtime status avoids disabling the hardware
> resources a second time when runtime PM has already powered off the
> sensor.
>
> This issue was found by manual code inspection.
>
> Fixes: 089b7c70f0d8 ("media: ov5647: Use pm_runtime infrastructure")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Guangshuo Li <lgs201920130244@xxxxxxxxx>
> ---
>  drivers/media/i2c/ov5647.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index 3facf92b3841..d42d009772ac 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -1271,6 +1271,9 @@ static void ov5647_remove(struct i2c_client *client)
>         v4l2_ctrl_handler_free(&sensor->ctrls);
>         v4l2_device_unregister_subdev(sd);
>         pm_runtime_disable(&client->dev);
> +       if (!pm_runtime_status_suspended(&client->dev))
> +               ov5647_power_off(&client->dev);
> +       pm_runtime_set_suspended(&client->dev);

I was unsure whether pm_runtime_set_suspended can be unconditional or
should be in the if clause. There are numerous drivers doing each.

As it happens, Sakari's just answered that in [1] that it should be
conditional, so I'd take that as gospel. It looks like he's given a
similar answer on your ov2740 patch.

  Dave

[1] https://lore.kernel.org/linux-media/aqvb_n-aPcXLnEa7@kekkonen.localdomain/T/#u

>  }
>
>  static const struct dev_pm_ops ov5647_pm_ops = {
> --
> 2.43.0
>