Re: [PATCH 3/3] media: i2c: ov9282: switch to {enable,disable}_streams
From: Tarang Raval
Date: Sat Feb 28 2026 - 15:04:22 EST
Hi Xiaolei,
> Switch from s_stream to enable_streams and disable_streams callbacks.
>
> Signed-off-by: Xiaolei Wang <xiaolei.wang@xxxxxxxxxxxxx>
> ---
> drivers/media/i2c/ov9282.c | 82 ++++++++++++--------------------------
> 1 file changed, 25 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index aa5a105136bf..b080a56d83a9 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -921,13 +921,9 @@ static int ov9282_get_selection(struct v4l2_subdev *sd,
> return -EINVAL;
> }
>
> -/**
> - * ov9282_start_streaming() - Start sensor stream
> - * @ov9282: pointer to ov9282 device
> - *
> - * Return: 0 if successful, error code otherwise.
> - */
> -static int ov9282_start_streaming(struct ov9282 *ov9282)
> +static int ov9282_enable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state, u32 pad,
> + u64 streams_mask)
> {
> const struct reg_sequence bitdepth_regs[2][2] = {
> {
> @@ -938,16 +934,21 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
> {OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW8},
> }
> };
> + struct ov9282 *ov9282 = to_ov9282(sd);
> const struct ov9282_reg_list *reg_list;
> int bitdepth_index;
> int ret;
>
> + ret = pm_runtime_resume_and_get(ov9282->dev);
> + if (ret)
> + return ret;
> +
> /* Write common registers */
> ret = regmap_multi_reg_write(ov9282->regmap, common_regs,
> ARRAY_SIZE(common_regs));
> if (ret) {
> dev_err(ov9282->dev, "fail to write common registers");
> - return ret;
> + goto done;
> }
>
> bitdepth_index = ov9282->code == MEDIA_BUS_FMT_Y10_1X10 ? 0 : 1;
> @@ -955,7 +956,7 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
> bitdepth_regs[bitdepth_index], 2);
> if (ret) {
> dev_err(ov9282->dev, "fail to write bitdepth regs");
> - return ret;
> + goto done;
> }
>
> /* Write sensor mode registers */
> @@ -964,75 +965,40 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
> reg_list->num_of_regs);
> if (ret) {
> dev_err(ov9282->dev, "fail to write initial registers");
> - return ret;
> + goto done;
> }
>
> /* Setup handler will write actual exposure and gain */
> ret = __v4l2_ctrl_handler_setup(ov9282->sd.ctrl_handler);
> if (ret) {
> dev_err(ov9282->dev, "fail to setup handler");
> - return ret;
> + goto done;
> }
>
> /* Start streaming */
> ret = cci_write(ov9282->regmap, OV9282_REG_MODE_SELECT,
> OV9282_MODE_STREAMING, NULL);
> - if (ret) {
> + if (ret)
> dev_err(ov9282->dev, "fail to start streaming");
> - return ret;
> - }
>
> - return 0;
> -}
> +done:
> + if (ret)
> + pm_runtime_put(ov9282->dev);
The current flow looks odd; can we use a conventional error path with a clear
label like err_pm_put:
/* Start streaming */
ret = cci_write(ov9282->regmap, OV9282_REG_MODE_SELECT,
OV9282_MODE_STREAMING, NULL);
if (ret)
dev_err(ov9282->dev, "fail to start streaming");
goto err_pm_put;
}
return 0;
err_pm_put:
pm_runtime_put(ov9282->dev);
return ret;
>
> -/**
> - * ov9282_stop_streaming() - Stop sensor stream
> - * @ov9282: pointer to ov9282 device
> - *
> - * Return: 0 if successful, error code otherwise.
> - */
> -static int ov9282_stop_streaming(struct ov9282 *ov9282)
> -{
> - return cci_write(ov9282->regmap, OV9282_REG_MODE_SELECT,
> - OV9282_MODE_STANDBY, NULL);
> + return ret;
> }
>
> -/**
> - * ov9282_set_stream() - Enable sensor streaming
> - * @sd: pointer to ov9282 subdevice
> - * @enable: set to enable sensor streaming
> - *
> - * Return: 0 if successful, error code otherwise.
> - */
> -static int ov9282_set_stream(struct v4l2_subdev *sd, int enable)
> +static int ov9282_disable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state, u32 pad,
> + u64 streams_mask)
> {
> struct ov9282 *ov9282 = to_ov9282(sd);
> - struct v4l2_subdev_state *state;
> int ret;
>
> - state = v4l2_subdev_lock_and_get_active_state(sd);
> -
> - if (enable) {
> - ret = pm_runtime_resume_and_get(ov9282->dev);
> - if (ret)
> - goto error_unlock;
> -
> - ret = ov9282_start_streaming(ov9282);
> - if (ret)
> - goto error_power_off;
> - } else {
> - ov9282_stop_streaming(ov9282);
> - pm_runtime_put(ov9282->dev);
> - }
> -
> - v4l2_subdev_unlock_state(state);
> -
> - return 0;
> + ret = cci_write(ov9282->regmap, OV9282_REG_MODE_SELECT,
> + OV9282_MODE_STANDBY, NULL);
>
> -error_power_off:
> pm_runtime_put(ov9282->dev);
> -error_unlock:
> - v4l2_subdev_unlock_state(state);
>
> return ret;
> }
> @@ -1164,7 +1130,7 @@ static const struct v4l2_subdev_core_ops ov9282_core_ops = {
> };
>
> static const struct v4l2_subdev_video_ops ov9282_video_ops = {
> - .s_stream = ov9282_set_stream,
> + .s_stream = v4l2_subdev_s_stream_helper,
> };
>
> static const struct v4l2_subdev_pad_ops ov9282_pad_ops = {
> @@ -1173,6 +1139,8 @@ static const struct v4l2_subdev_pad_ops ov9282_pad_ops = {
> .get_fmt = ov9282_get_pad_format,
> .set_fmt = ov9282_set_pad_format,
> .get_selection = ov9282_get_selection,
> + .enable_streams = ov9282_enable_streams,
> + .disable_streams = ov9282_disable_streams,
> };
>
> static const struct v4l2_subdev_ops ov9282_subdev_ops = {
> --
> 2.43.0
Reviewed-by: Tarang Raval <tarang.raval@xxxxxxxxxxxxxxxxx>
Best Regards,
Tarang