Re: [PATCH v2 07/11] media: i2c: ov5645: Use subdev active state

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:06:06PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> 
> Port the ov5645 sensor driver to use the subdev active state.
> 
> Move all the format configuration to the subdevice state and simplify
> the format handling, locking and initialization.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>

> ---
>  drivers/media/i2c/ov5645.c | 109 +++++++++++++------------------------
>  1 file changed, 39 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 25c60afcc0ec..9497ec737cb7 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -89,7 +89,6 @@ struct ov5645 {
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
>  	struct v4l2_fwnode_endpoint ep;
> -	struct v4l2_mbus_framefmt fmt;
>  	struct v4l2_rect crop;
>  	struct clk *xclk;
>  
> @@ -850,49 +849,6 @@ static int ov5645_enum_frame_size(struct v4l2_subdev *subdev,
>  	return 0;
>  }
>  
> -static struct v4l2_mbus_framefmt *
> -__ov5645_get_pad_format(struct ov5645 *ov5645,
> -			struct v4l2_subdev_state *sd_state,
> -			unsigned int pad,
> -			enum v4l2_subdev_format_whence which)
> -{
> -	switch (which) {
> -	case V4L2_SUBDEV_FORMAT_TRY:
> -		return v4l2_subdev_state_get_format(sd_state, pad);
> -	case V4L2_SUBDEV_FORMAT_ACTIVE:
> -		return &ov5645->fmt;
> -	default:
> -		return NULL;
> -	}
> -}
> -
> -static int ov5645_get_format(struct v4l2_subdev *sd,
> -			     struct v4l2_subdev_state *sd_state,
> -			     struct v4l2_subdev_format *format)
> -{
> -	struct ov5645 *ov5645 = to_ov5645(sd);
> -
> -	format->format = *__ov5645_get_pad_format(ov5645, sd_state,
> -						  format->pad,
> -						  format->which);
> -	return 0;
> -}
> -
> -static struct v4l2_rect *
> -__ov5645_get_pad_crop(struct ov5645 *ov5645,
> -		      struct v4l2_subdev_state *sd_state,
> -		      unsigned int pad, enum v4l2_subdev_format_whence which)
> -{
> -	switch (which) {
> -	case V4L2_SUBDEV_FORMAT_TRY:
> -		return v4l2_subdev_state_get_crop(sd_state, pad);
> -	case V4L2_SUBDEV_FORMAT_ACTIVE:
> -		return &ov5645->crop;
> -	default:
> -		return NULL;
> -	}
> -}
> -
>  static int ov5645_set_format(struct v4l2_subdev *sd,
>  			     struct v4l2_subdev_state *sd_state,
>  			     struct v4l2_subdev_format *format)
> @@ -903,33 +859,30 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
>  	const struct ov5645_mode_info *new_mode;
>  	int ret;
>  
> -	__crop = __ov5645_get_pad_crop(ov5645, sd_state, format->pad,
> -				       format->which);
> -
> +	__crop = v4l2_subdev_state_get_crop(sd_state, 0);
>  	new_mode = v4l2_find_nearest_size(ov5645_mode_info_data,
> -			       ARRAY_SIZE(ov5645_mode_info_data),
> -			       width, height,
> -			       format->format.width, format->format.height);
> +					  ARRAY_SIZE(ov5645_mode_info_data),
> +					  width, height, format->format.width,
> +					  format->format.height);
>  
>  	__crop->width = new_mode->width;
>  	__crop->height = new_mode->height;
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> -		ret = v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock,
> -					     new_mode->pixel_clock);
> +		ret = __v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock,
> +					       new_mode->pixel_clock);
>  		if (ret < 0)
>  			return ret;
>  
> -		ret = v4l2_ctrl_s_ctrl(ov5645->link_freq,
> -				       new_mode->link_freq);
> +		ret = __v4l2_ctrl_s_ctrl(ov5645->link_freq,
> +					 new_mode->link_freq);
>  		if (ret < 0)
>  			return ret;
>  
>  		ov5645->current_mode = new_mode;
>  	}
>  
> -	__format = __ov5645_get_pad_format(ov5645, sd_state, format->pad,
> -					   format->which);
> +	__format = v4l2_subdev_state_get_format(sd_state, 0);
>  	__format->width = __crop->width;
>  	__format->height = __crop->height;
>  	__format->code = MEDIA_BUS_FMT_UYVY8_1X16;
> @@ -944,11 +897,15 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
>  static int ov5645_init_state(struct v4l2_subdev *subdev,
>  			     struct v4l2_subdev_state *sd_state)
>  {
> -	struct v4l2_subdev_format fmt = { 0 };
> -
> -	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> -	fmt.format.width = 1920;
> -	fmt.format.height = 1080;
> +	struct v4l2_subdev_format fmt = {
> +		.which = V4L2_SUBDEV_FORMAT_TRY,
> +		.pad = 0,
> +		.format = {
> +			.code = MEDIA_BUS_FMT_UYVY8_1X16,
> +			.width = ov5645_mode_info_data[1].width,
> +			.height = ov5645_mode_info_data[1].height,
> +		},
> +	};
>  
>  	ov5645_set_format(subdev, sd_state, &fmt);
>  
> @@ -959,25 +916,27 @@ static int ov5645_get_selection(struct v4l2_subdev *sd,
>  			   struct v4l2_subdev_state *sd_state,
>  			   struct v4l2_subdev_selection *sel)
>  {
> -	struct ov5645 *ov5645 = to_ov5645(sd);
> -
>  	if (sel->target != V4L2_SEL_TGT_CROP)
>  		return -EINVAL;
>  
> -	sel->r = *__ov5645_get_pad_crop(ov5645, sd_state, sel->pad,
> -					sel->which);
> +	sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
>  	return 0;
>  }
>  
>  static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
>  {
>  	struct ov5645 *ov5645 = to_ov5645(subdev);
> +	struct v4l2_subdev_state *state;
>  	int ret;
>  
> +	state = v4l2_subdev_lock_and_get_active_state(&ov5645->sd);
> +
>  	if (enable) {
>  		ret = pm_runtime_resume_and_get(ov5645->dev);
> -		if (ret < 0)
> +		if (ret < 0) {
> +			v4l2_subdev_unlock_state(state);
>  			return ret;
> +		}
>  
>  		ret = ov5645_set_register_array(ov5645,
>  					ov5645->current_mode->data,
> @@ -988,7 +947,7 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
>  				ov5645->current_mode->height);
>  			goto err_rpm_put;
>  		}
> -		ret = v4l2_ctrl_handler_setup(&ov5645->ctrls);
> +		ret = __v4l2_ctrl_handler_setup(&ov5645->ctrls);
>  		if (ret < 0) {
>  			dev_err(ov5645->dev, "could not sync v4l2 controls\n");
>  			goto err_rpm_put;
> @@ -1013,6 +972,7 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
>  		goto stream_off_rpm_put;
>  	}
>  
> +	v4l2_subdev_unlock_state(state);
>  	return 0;
>  
>  err_rpm_put:
> @@ -1022,6 +982,7 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
>  stream_off_rpm_put:
>  	pm_runtime_mark_last_busy(ov5645->dev);
>  	pm_runtime_put_autosuspend(ov5645->dev);
> +	v4l2_subdev_unlock_state(state);
>  	return ret;
>  }
>  
> @@ -1032,7 +993,7 @@ static const struct v4l2_subdev_video_ops ov5645_video_ops = {
>  static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
>  	.enum_mbus_code = ov5645_enum_mbus_code,
>  	.enum_frame_size = ov5645_enum_frame_size,
> -	.get_fmt = ov5645_get_format,
> +	.get_fmt = v4l2_subdev_get_fmt,
>  	.set_fmt = ov5645_set_format,
>  	.get_selection = ov5645_get_selection,
>  };
> @@ -1213,12 +1174,17 @@ static int ov5645_probe(struct i2c_client *client)
>  		goto power_down;
>  	}
>  
> -	ov5645_init_state(&ov5645->sd, NULL);
> +	ov5645->sd.state_lock = ov5645->ctrls.lock;
> +	ret = v4l2_subdev_init_finalize(&ov5645->sd);
> +	if (ret < 0) {
> +		dev_err_probe(dev, ret, "subdev init error\n");
> +		goto power_down;
> +	}
>  
>  	ret = v4l2_async_register_subdev_sensor(&ov5645->sd);
>  	if (ret < 0) {
>  		dev_err_probe(dev, ret, "could not register v4l2 device\n");
> -		goto power_down;
> +		goto error_subdev_cleanup;
>  	}
>  
>  	pm_runtime_set_active(dev);
> @@ -1231,6 +1197,8 @@ static int ov5645_probe(struct i2c_client *client)
>  
>  	return 0;
>  
> +error_subdev_cleanup:
> +	v4l2_subdev_cleanup(&ov5645->sd);
>  power_down:
>  	ov5645_set_power_off(dev);
>  free_entity:
> @@ -1247,6 +1215,7 @@ static void ov5645_remove(struct i2c_client *client)
>  	struct ov5645 *ov5645 = to_ov5645(sd);
>  
>  	v4l2_async_unregister_subdev(&ov5645->sd);
> +	v4l2_subdev_cleanup(sd);
>  	media_entity_cleanup(&ov5645->sd.entity);
>  	v4l2_ctrl_handler_free(&ov5645->ctrls);
>  	pm_runtime_disable(ov5645->dev);

-- 
Regards,

Laurent Pinchart




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux