Hi Laurent,
On Sat, Nov 18, 2023 at 07:33:15PM +0200, Laurent Pinchart wrote:
> 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" ?
People tend to take patterns used in drivers and this indeed works here,
but only when taken with the rest of the Runtime PM API usage.
> :-) 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.
I think it should be fine to do both in the same patch indeed.
--
Regards,
Sakari Ailus
