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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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@gmail.com> 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@vger.kernel.org
> > Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> > ---
> >  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