Re: [PATCH 3/3] media: i2c: ov9282: switch to {enable,disable}_streams

[xiaolei wang <xiaolei.wang@windriver.com>]

On 3/1/26 02:45, Tarang Raval wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> Hi Xiaolei,
>
>> Switch from s_stream to enable_streams and disable_streams callbacks.
>>
>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
>> ---
>>   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;
Hi Tarang,

Thank you for the review!

I'll update it in v2:

/* 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;

Best Regards,
Xiaolei
>
>> -/**
>> - * 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@siliconsignals.io>
>
> Best Regards,
> Tarang