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

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

Hi,

On Tue, Sep 22, 2026 at 03:45:13PM +0800, Guangshuo Li wrote:
> Hi Dave, Sakari,
> 
> Thanks for the clarification.
> 
> On Fri, 18 Sept 2026 at 16:45, Sakari Ailus
> <sakari.ailus@linux.intel.com> 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@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
> 
> 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);
> +       }

Correct.

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

Please!

-- 
Regards,

Sakari Ailus