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

[Tarang Raval]

Hi Xiaolei,

> Switch from s_stream to enable_streams and disable_streams callbacks.
> 
> Signed-off-by: Xiaolei Wang <xiaolei.wang@xxxxxxxxxxxxx>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
>  drivers/media/i2c/ov5647.c | 89 ++++++++++++++++----------------------
>  1 file changed, 38 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index bc81f378436a..7091081a0828 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -637,23 +637,42 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)
>     return 0;
>  }
>  
> -static int ov5647_stream_on(struct v4l2_subdev *sd)
> +static int ov5647_stream_stop(struct ov5647 *sensor)
> +{
> +   int ret = 0;
> +
> +   cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL00,
> +           MIPI_CTRL00_CLOCK_LANE_GATE | MIPI_CTRL00_BUS_IDLE |
> +           MIPI_CTRL00_CLOCK_LANE_DISABLE, &ret);
> +   cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x0f, &ret);
> +   cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x01, &ret);
> +
> +   return ret;
> +}
> +
> +static int ov5647_enable_streams(struct v4l2_subdev *sd,
> +                      struct v4l2_subdev_state *state, u32 pad,
> +                      u64 streams_mask)
>  {
>     struct i2c_client *client = v4l2_get_subdevdata(sd);
>     struct ov5647 *sensor = to_sensor(sd);
>     u8 val = MIPI_CTRL00_BUS_IDLE;
>     int ret = 0;

No need for zero initialization.
  
> +   ret = pm_runtime_resume_and_get(&client->dev);
> +   if (ret < 0)
> +         return ret;
> +
>     ret = ov5647_set_mode(sd);
>     if (ret) {
>           dev_err(&client->dev, "Failed to program sensor mode: %d\n", ret);
> -         return ret;
> +         goto done;
>     }
>  
>     /* Apply customized values from user when stream starts. */
>     ret =  __v4l2_ctrl_handler_setup(sd->ctrl_handler);
>     if (ret)
> -         return ret;
> +         goto done;
>  
>     if (sensor->clock_ncont)
>           val |= MIPI_CTRL00_CLOCK_LANE_GATE |
> @@ -663,19 +682,24 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)
>     cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x00, &ret);
>     cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x00, &ret);
>  
> +done:
> +   if (ret)
> +         pm_runtime_put(&client->dev);
> +
>     return ret;
>  }
>  
> -static int ov5647_stream_off(struct v4l2_subdev *sd)
> +static int ov5647_disable_streams(struct v4l2_subdev *sd,
> +                       struct v4l2_subdev_state *state, u32 pad,
> +                       u64 streams_mask)
>  {
> +   struct i2c_client *client = v4l2_get_subdevdata(sd);
>     struct ov5647 *sensor = to_sensor(sd);
> -   int ret = 0;
> +   int ret;
>  
> -   cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL00,
> -           MIPI_CTRL00_CLOCK_LANE_GATE | MIPI_CTRL00_BUS_IDLE |
> -           MIPI_CTRL00_CLOCK_LANE_DISABLE, &ret);
> -   cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x0f, &ret);
> -   cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x01, &ret);
> +   ret = ov5647_stream_stop(sensor);
> +
> +   pm_runtime_put(&client->dev);
>  
>     return ret;
>  }
> @@ -706,7 +730,7 @@ static int ov5647_power_on(struct device *dev)
>     }
>  
>     /* Stream off to coax lanes into LP-11 state. */
> -   ret = ov5647_stream_off(&sensor->sd);
> +   ret = ov5647_stream_stop(sensor);
>     if (ret < 0) {
>           dev_err(dev, "camera not available, check power\n");
>           goto error_clk_disable;
> @@ -803,47 +827,8 @@ __ov5647_get_pad_crop(struct ov5647 *ov5647,
>     return NULL;
>  }
>  
> -static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
> -{
> -   struct i2c_client *client = v4l2_get_subdevdata(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(&client->dev);
> -         if (ret < 0)
> -               goto error_unlock;
> -
> -         ret = ov5647_stream_on(sd);
> -         if (ret < 0) {
> -               dev_err(&client->dev, "stream start failed: %d\n", ret);
> -               goto error_pm;
> -         }
> -   } else {
> -         ret = ov5647_stream_off(sd);
> -         if (ret < 0) {
> -               dev_err(&client->dev, "stream stop failed: %d\n", ret);
> -               goto error_pm;
> -         }
> -         pm_runtime_put(&client->dev);
> -   }
> -
> -   v4l2_subdev_unlock_state(state);
> -
> -   return 0;
> -
> -error_pm:
> -   pm_runtime_put(&client->dev);
> -error_unlock:
> -   v4l2_subdev_unlock_state(state);
> -
> -   return ret;
> -}
> -
>  static const struct v4l2_subdev_video_ops ov5647_subdev_video_ops = {
> -   .s_stream =       ov5647_s_stream,
> +   .s_stream = v4l2_subdev_s_stream_helper,
>  };
>  
>  static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
> @@ -986,6 +971,8 @@ static const struct v4l2_subdev_pad_ops ov5647_subdev_pad_ops = {
>     .set_fmt          = ov5647_set_pad_fmt,
>     .get_fmt          = ov5647_get_pad_fmt,
>     .get_selection          = ov5647_get_selection,
> +   .enable_streams         = ov5647_enable_streams,
> +   .disable_streams  = ov5647_disable_streams,
>  };
>  
>  static const struct v4l2_subdev_ops ov5647_subdev_ops = {
> -- 
> 2.43.0

Reviewed-by: Tarang Raval <tarang.raval@xxxxxxxxxxxxxxxxx>
                                                          
Best Regards,                                             
Tarang