Hi Jacopo,
On Fri, Nov 17, 2023 at 04:30:15PM +0100, Jacopo Mondi wrote:
> Hi Sakari
>
> On Fri, Nov 17, 2023 at 01:14:31PM +0200, Sakari Ailus wrote:
> > Use pm_runtime_get_if_active() to get the device's runtime PM usage_count
> > and set controls, then use runtime PM autosuspend once the controls have
> > been set (instead of likely transitioning to suspended state immediately).
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/ov8858.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov8858.c b/drivers/media/i2c/ov8858.c
> > index 3af6125a2eee..a99b91700a8d 100644
> > --- a/drivers/media/i2c/ov8858.c
> > +++ b/drivers/media/i2c/ov8858.c
> > @@ -1538,7 +1538,7 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > struct v4l2_subdev_state *state;
> > u16 digi_gain;
> > s64 max_exp;
> > - int ret;
> > + int ret, pm_status;
> >
> > /*
> > * The control handler and the subdev state use the same mutex and the
> > @@ -1561,7 +1561,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > break;
> > }
> >
> > - if (!pm_runtime_get_if_in_use(&client->dev))
> > + pm_status = pm_runtime_get_if_active(&client->dev);
> > + if (!pm_status)
> > return 0;
> >
> > switch (ctrl->id) {
> > @@ -1601,7 +1602,8 @@ static int ov8858_set_ctrl(struct v4l2_ctrl *ctrl)
> > break;
> > }
> >
> > - pm_runtime_put(&client->dev);
> > + if (pm_status > 0)
>
> I'm not 100% sure I get this bit.
>
> If we get here it means pm_status is either -EINVAL or > 0, otherwise
> we would have exited earlier.
>
> What's the point of checking for > 0 here ?
>
> There are two cases where pm_status is -EINVAL, either !CONFIG_PM and
> the the below call is a nop, or if pm_runtime has not been enabled by
> the driver, which means the driver doesn't use pm_runtime at all.
>
> Are there other cases I have missed that require checking here for
> pm_status > 0 ?
Other than Runtime PM being disabled, I don't think that should happen.
So as such this patch does not fix a bug. I just prefer to be extra
cautious when it comes to use counts.
>
> > + pm_runtime_mark_busy_autosusp(&client->dev);
> >
> > return ret;
> > }
--
Regards,
Sakari Ailus
