Re: [PATCH v2 5/8] media: imx335: Handle runtime PM in leaf functions

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

Quoting Jai Luthra (2025-09-11 09:14:21)
> Simplify .s_stream callback implementation by moving the runtime PM
> calls to the leaf functions. This patch should not affect any
> functionality.
> 
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>

Looks reasonable to me and although the stop_streaming function changes
to no longer return a value - it was previously unused so no functional
change indeed as far as I can see:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  drivers/media/i2c/imx335.c | 51 ++++++++++++++++++++--------------------------
>  1 file changed, 22 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index 62a76517ecb263a4fd7e7a593c02a3cdaf3da190..7631f41e6f1e65695fb76a66d9ac5a3588c69658 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -919,13 +919,17 @@ static int imx335_start_streaming(struct imx335 *imx335)
>         const struct imx335_reg_list *reg_list;
>         int ret;
>  
> +       ret = pm_runtime_resume_and_get(imx335->dev);
> +       if (ret < 0)
> +               return ret;
> +
>         /* Setup PLL */
>         reg_list = &link_freq_reglist[__ffs(imx335->link_freq_bitmap)];
>         ret = cci_multi_reg_write(imx335->cci, reg_list->regs,
>                                   reg_list->num_of_regs, NULL);
>         if (ret) {
>                 dev_err(imx335->dev, "%s failed to set plls\n", __func__);
> -               return ret;
> +               goto err_rpm_put;
>         }
>  
>         /* Write sensor mode registers */
> @@ -934,27 +938,27 @@ static int imx335_start_streaming(struct imx335 *imx335)
>                                   reg_list->num_of_regs, NULL);
>         if (ret) {
>                 dev_err(imx335->dev, "fail to write initial registers\n");
> -               return ret;
> +               goto err_rpm_put;
>         }
>  
>         ret = imx335_set_framefmt(imx335);
>         if (ret) {
>                 dev_err(imx335->dev, "%s failed to set frame format: %d\n",
>                         __func__, ret);
> -               return ret;
> +               goto err_rpm_put;
>         }
>  
>         /* Configure lanes */
>         ret = cci_write(imx335->cci, IMX335_REG_LANEMODE,
>                         imx335->lane_mode, NULL);
>         if (ret)
> -               return ret;
> +               goto err_rpm_put;
>  
>         /* Setup handler will write actual exposure and gain */
>         ret =  __v4l2_ctrl_handler_setup(imx335->sd.ctrl_handler);
>         if (ret) {
>                 dev_err(imx335->dev, "fail to setup handler\n");
> -               return ret;
> +               goto err_rpm_put;
>         }
>  
>         /* Start streaming */
> @@ -962,25 +966,29 @@ static int imx335_start_streaming(struct imx335 *imx335)
>                         IMX335_MODE_STREAMING, NULL);
>         if (ret) {
>                 dev_err(imx335->dev, "fail to start streaming\n");
> -               return ret;
> +               goto err_rpm_put;
>         }
>  
>         /* Initial regulator stabilization period */
>         usleep_range(18000, 20000);
>  
>         return 0;
> +
> +err_rpm_put:
> +       pm_runtime_put(imx335->dev);
> +
> +       return ret;
>  }
>  
>  /**
>   * imx335_stop_streaming() - Stop sensor stream
>   * @imx335: pointer to imx335 device
> - *
> - * Return: 0 if successful, error code otherwise.
>   */
> -static int imx335_stop_streaming(struct imx335 *imx335)
> +static void imx335_stop_streaming(struct imx335 *imx335)
>  {
> -       return cci_write(imx335->cci, IMX335_REG_MODE_SELECT,
> -                        IMX335_MODE_STANDBY, NULL);
> +       cci_write(imx335->cci, IMX335_REG_MODE_SELECT,
> +                 IMX335_MODE_STANDBY, NULL);
> +       pm_runtime_put(imx335->dev);
>  }
>  
>  /**
> @@ -993,30 +1001,15 @@ static int imx335_stop_streaming(struct imx335 *imx335)
>  static int imx335_set_stream(struct v4l2_subdev *sd, int enable)
>  {
>         struct imx335 *imx335 = to_imx335(sd);
> -       int ret;
> +       int ret = 0;
>  
>         mutex_lock(&imx335->mutex);
>  
> -       if (enable) {
> -               ret = pm_runtime_resume_and_get(imx335->dev);
> -               if (ret)
> -                       goto error_unlock;
> -
> +       if (enable)
>                 ret = imx335_start_streaming(imx335);
> -               if (ret)
> -                       goto error_power_off;
> -       } else {
> +       else
>                 imx335_stop_streaming(imx335);
> -               pm_runtime_put(imx335->dev);
> -       }
> -
> -       mutex_unlock(&imx335->mutex);
>  
> -       return 0;
> -
> -error_power_off:
> -       pm_runtime_put(imx335->dev);
> -error_unlock:
>         mutex_unlock(&imx335->mutex);
>  
>         return ret;
> 
> -- 
> 2.51.0
>