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

[Guangshuo Li]

Hi Dave, Sakari,

Thanks for the clarification.

On Fri, 18 Sept 2026 at 16:45, Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
>
> Hi Dave,
>
> On Thu, Sep 17, 2026 at 02:27:30PM +0100, Dave Stevenson wrote:
> > 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.
>
> Runtime PM is a bit mystical in some places. pm_runtime_set_suspended()
> appears to be setting the device's Runtime PM state disabled as it name
> implies, but it may also e.g. increment dev->power.disable_depth. I'm not
> fully certain if this is by design or not.
>
> --
> Sakari Ailus

Would this be the right way to change it?

-       if (!pm_runtime_status_suspended(&client->dev))
+       if (!pm_runtime_status_suspended(&client->dev)) {
                ov5647_power_off(&client->dev);
-       pm_runtime_set_suspended(&client->dev);
+               pm_runtime_set_suspended(&client->dev);
+       }

If so, I can send a v2 with this change.

Thanks,
Guangshuo