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>
---
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.0Quoting 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
>
Hi Kieran,
Thanks for the review.
Quoting Kieran Bingham (2025-09-11 16:44:45)
> 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:
Yeah it was unused before. This anyway gets fixed when switching to streams
API in the last patch of the series.
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> > ---
> > drivers/media/i2c/imx335.c | 51 ++++++++++++++++++++--------------------------
> > 1 file changed, 22 insertions(+), 29 deletions(-)
> >
Thanks,
Jai
© 2016 - 2025 Red Hat, Inc.