Hi Sakari
On Sat, Nov 18, 2023 at 11:12:41AM +0000, Sakari Ailus wrote:
> On Fri, Nov 17, 2023 at 04:30:15PM +0100, Jacopo Mondi wrote:
> > 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.
What happened to the old motto of "if it's not broken, don't fix it" ?
:-) I like how this series (slightly) simplifies the runtime PM API by
giving pm_runtime_get_if_active() the right behaviour for the most
common use cases. Let's continue in that direction, and evolve the API
to simplify driver, not render them more complex.
I would prefer refactoring this series to first switch drivers to
pm_runtime_get_if_active(), and then use autosuspend at the end of the
.s_ctrl() handler. That can be two patches, each modifying all relevant
sensor driver.
> > > + pm_runtime_mark_busy_autosusp(&client->dev);
> > >
> > > return ret;
> > > }
--
Regards,
Laurent Pinchart
