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 ?
> + pm_runtime_mark_busy_autosusp(&client->dev);
>
> return ret;
> }
> --
> 2.39.2
>
