| Message ID | 20240229165333.227484-20-mike.rudenko@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Omnivision OV4689 refactoring and improvements | expand |
Hi Laurent, and thanks for the review. On 2024-03-04 at 23:31 +02, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Mikhail, > > Thank you for the patch. > > On Thu, Feb 29, 2024 at 07:53:32PM +0300, Mikhail Rudenko wrote: >> Split ov4689_s_stream into __ov4689_stream_on and __ov4689_stream_off >> functions. Also remove repetitive pm_runtime_put calls and call >> pm_runtime_put once at the end of the __ov4689_stream_off function if >> any error occurred. >> >> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> >> --- >> drivers/media/i2c/ov4689.c | 100 ++++++++++++++++++++----------------- >> 1 file changed, 53 insertions(+), 47 deletions(-) >> >> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c >> index 2496067b90a0..5cea9b5ba201 100644 >> --- a/drivers/media/i2c/ov4689.c >> +++ b/drivers/media/i2c/ov4689.c >> @@ -537,61 +537,67 @@ static int ov4689_setup_blc_anchors(struct ov4689 *ov4689, >> return ret; >> } >> >> +static int __ov4689_stream_on(struct ov4689 *ov4689, > > No need for the __ prefix. Same for __ov4689_stream_off(). Will remove the prefix in v4. >> + struct v4l2_subdev_state *sd_state) >> +{ >> + int ret; >> + >> + ret = pm_runtime_resume_and_get(ov4689->dev); >> + if (ret < 0) >> + return ret; >> + >> + ret = cci_multi_reg_write(ov4689->regmap, ov4689_common_regs, >> + ARRAY_SIZE(ov4689_common_regs), NULL); >> + if (ret) >> + goto cleanup_pm; >> + >> + ret = ov4689_setup_timings(ov4689, sd_state); >> + if (ret) >> + goto cleanup_pm; >> + >> + ret = ov4689_setup_blc_anchors(ov4689, sd_state); >> + if (ret) >> + goto cleanup_pm; >> + >> + ret = __v4l2_ctrl_handler_setup(&ov4689->ctrl_handler); >> + if (ret) >> + goto cleanup_pm; >> + >> + ret = cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, >> + OV4689_MODE_STREAMING, NULL); >> + if (ret) >> + goto cleanup_pm; >> + >> + return 0; >> + >> + cleanup_pm: > > No space before the label. I would also name it just "error". Thanks for the suggestion, will do so in v4. > With those small changes, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> + pm_runtime_put(ov4689->dev); >> + return ret; >> +} >> + >> +static int __ov4689_stream_off(struct ov4689 *ov4689) >> +{ >> + cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, OV4689_MODE_SW_STANDBY, >> + NULL); >> + pm_runtime_mark_last_busy(ov4689->dev); >> + pm_runtime_put_autosuspend(ov4689->dev); >> + >> + return 0; >> +} >> + >> static int ov4689_s_stream(struct v4l2_subdev *sd, int on) >> { >> struct ov4689 *ov4689 = to_ov4689(sd); >> struct v4l2_subdev_state *sd_state; >> - struct device *dev = ov4689->dev; >> - int ret = 0; >> + int ret; >> >> sd_state = v4l2_subdev_lock_and_get_active_state(&ov4689->subdev); >> >> - if (on) { >> - ret = pm_runtime_resume_and_get(dev); >> - if (ret < 0) >> - goto unlock_and_return; >> - >> - ret = cci_multi_reg_write(ov4689->regmap, >> - ov4689_common_regs, >> - ARRAY_SIZE(ov4689_common_regs), >> - NULL); >> - if (ret) { >> - pm_runtime_put(dev); >> - goto unlock_and_return; >> - } >> - >> - ret = ov4689_setup_timings(ov4689, sd_state); >> - if (ret) { >> - pm_runtime_put(dev); >> - goto unlock_and_return; >> - } >> - >> - ret = ov4689_setup_blc_anchors(ov4689, sd_state); >> - if (ret) { >> - pm_runtime_put(dev); >> - goto unlock_and_return; >> - } >> - >> - ret = __v4l2_ctrl_handler_setup(&ov4689->ctrl_handler); >> - if (ret) { >> - pm_runtime_put(dev); >> - goto unlock_and_return; >> - } >> - >> - ret = cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, >> - OV4689_MODE_STREAMING, NULL); >> - if (ret) { >> - pm_runtime_put(dev); >> - goto unlock_and_return; >> - } >> - } else { >> - cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, >> - OV4689_MODE_SW_STANDBY, NULL); >> - pm_runtime_mark_last_busy(dev); >> - pm_runtime_put_autosuspend(dev); >> - } >> + if (on) >> + ret = __ov4689_stream_on(ov4689, sd_state); >> + else >> + ret = __ov4689_stream_off(ov4689); >> >> -unlock_and_return: >> v4l2_subdev_unlock_state(sd_state); >> >> return ret; -- Best regards, Mikhail Rudenko
diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c index 2496067b90a0..5cea9b5ba201 100644 --- a/drivers/media/i2c/ov4689.c +++ b/drivers/media/i2c/ov4689.c @@ -537,61 +537,67 @@ static int ov4689_setup_blc_anchors(struct ov4689 *ov4689, return ret; } +static int __ov4689_stream_on(struct ov4689 *ov4689, + struct v4l2_subdev_state *sd_state) +{ + int ret; + + ret = pm_runtime_resume_and_get(ov4689->dev); + if (ret < 0) + return ret; + + ret = cci_multi_reg_write(ov4689->regmap, ov4689_common_regs, + ARRAY_SIZE(ov4689_common_regs), NULL); + if (ret) + goto cleanup_pm; + + ret = ov4689_setup_timings(ov4689, sd_state); + if (ret) + goto cleanup_pm; + + ret = ov4689_setup_blc_anchors(ov4689, sd_state); + if (ret) + goto cleanup_pm; + + ret = __v4l2_ctrl_handler_setup(&ov4689->ctrl_handler); + if (ret) + goto cleanup_pm; + + ret = cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, + OV4689_MODE_STREAMING, NULL); + if (ret) + goto cleanup_pm; + + return 0; + + cleanup_pm: + pm_runtime_put(ov4689->dev); + return ret; +} + +static int __ov4689_stream_off(struct ov4689 *ov4689) +{ + cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, OV4689_MODE_SW_STANDBY, + NULL); + pm_runtime_mark_last_busy(ov4689->dev); + pm_runtime_put_autosuspend(ov4689->dev); + + return 0; +} + static int ov4689_s_stream(struct v4l2_subdev *sd, int on) { struct ov4689 *ov4689 = to_ov4689(sd); struct v4l2_subdev_state *sd_state; - struct device *dev = ov4689->dev; - int ret = 0; + int ret; sd_state = v4l2_subdev_lock_and_get_active_state(&ov4689->subdev); - if (on) { - ret = pm_runtime_resume_and_get(dev); - if (ret < 0) - goto unlock_and_return; - - ret = cci_multi_reg_write(ov4689->regmap, - ov4689_common_regs, - ARRAY_SIZE(ov4689_common_regs), - NULL); - if (ret) { - pm_runtime_put(dev); - goto unlock_and_return; - } - - ret = ov4689_setup_timings(ov4689, sd_state); - if (ret) { - pm_runtime_put(dev); - goto unlock_and_return; - } - - ret = ov4689_setup_blc_anchors(ov4689, sd_state); - if (ret) { - pm_runtime_put(dev); - goto unlock_and_return; - } - - ret = __v4l2_ctrl_handler_setup(&ov4689->ctrl_handler); - if (ret) { - pm_runtime_put(dev); - goto unlock_and_return; - } - - ret = cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, - OV4689_MODE_STREAMING, NULL); - if (ret) { - pm_runtime_put(dev); - goto unlock_and_return; - } - } else { - cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE, - OV4689_MODE_SW_STANDBY, NULL); - pm_runtime_mark_last_busy(dev); - pm_runtime_put_autosuspend(dev); - } + if (on) + ret = __ov4689_stream_on(ov4689, sd_state); + else + ret = __ov4689_stream_off(ov4689); -unlock_and_return: v4l2_subdev_unlock_state(sd_state); return ret;
Split ov4689_s_stream into __ov4689_stream_on and __ov4689_stream_off functions. Also remove repetitive pm_runtime_put calls and call pm_runtime_put once at the end of the __ov4689_stream_off function if any error occurred. Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com> --- drivers/media/i2c/ov4689.c | 100 ++++++++++++++++++++----------------- 1 file changed, 53 insertions(+), 47 deletions(-)