diff mbox series

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

Message ID 20240910170610.226189-8-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Kieran Bingham
Headers show
Series media: ov5645: Add support for streams | expand

Commit Message

Lad, Prabhakar Sept. 10, 2024, 5:06 p.m. UTC 
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

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@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 109 +++++++++++++------------------------
 1 file changed, 39 insertions(+), 70 deletions(-)

Comments

Laurent Pinchart Sept. 24, 2024, 10:51 p.m. UTC | #1 
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@bp.renesas.com>
> 
> 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@bp.renesas.com>

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

> ---
>  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);
diff mbox series

Patch

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);