diff mbox series

[v4,01/13] media: i2c: imx214: Use subdev active state

Message ID 20241216-imx214-v4-1-8cbda160fbce@apitzsch.eu (mailing list archive)
State New
Headers show
Series media: i2c: imx214: Miscellaneous cleanups and improvements | expand

Commit Message

André Apitzsch via B4 Relay Dec. 16, 2024, 9 p.m. UTC 
From: André Apitzsch <git@apitzsch.eu>

Port the imx214 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.

While at it, simplify imx214_start_streaming() by removing unneeded goto
statements and the corresponding error label.

Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
 drivers/media/i2c/imx214.c | 157 +++++++++++++++------------------------------
 1 file changed, 51 insertions(+), 106 deletions(-)

Comments

Ricardo Ribalda Delgado Dec. 16, 2024, 9:20 p.m. UTC | #1 
Hi Andre

On Mon, Dec 16, 2024 at 10:01 PM André Apitzsch via B4 Relay
<devnull+git.apitzsch.eu@kernel.org> wrote:
>
> From: André Apitzsch <git@apitzsch.eu>
>
> Port the imx214 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.
>
> While at it, simplify imx214_start_streaming() by removing unneeded goto
> statements and the corresponding error label.
>
> Signed-off-by: André Apitzsch <git@apitzsch.eu>

Looks good to me, but I delegate to Sakari for a proper review. I have
not completed a port to subdev active state myself :)

Thanks

> ---
>  drivers/media/i2c/imx214.c | 157 +++++++++++++++------------------------------
>  1 file changed, 51 insertions(+), 106 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 4962cfe7c83d62425aeccb46a400fa93146f14ea..2e4322b90938c880896b216d3c46ccaa29485562 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -59,8 +59,6 @@ struct imx214 {
>
>         struct v4l2_subdev sd;
>         struct media_pad pad;
> -       struct v4l2_mbus_framefmt fmt;
> -       struct v4l2_rect crop;
>
>         struct v4l2_ctrl_handler ctrls;
>         struct v4l2_ctrl *pixel_rate;
> @@ -68,15 +66,11 @@ struct imx214 {
>         struct v4l2_ctrl *exposure;
>         struct v4l2_ctrl *unit_size;
>
> +       const struct imx214_mode *cur_mode;
> +
>         struct regulator_bulk_data      supplies[IMX214_NUM_SUPPLIES];
>
>         struct gpio_desc *enable_gpio;
> -
> -       /*
> -        * Serialize control access, get/set format, get selection
> -        * and start streaming.
> -        */
> -       struct mutex mutex;
>  };
>
>  struct reg_8 {
> @@ -490,6 +484,22 @@ static int __maybe_unused imx214_power_off(struct device *dev)
>         return 0;
>  }
>
> +static void imx214_update_pad_format(struct imx214 *imx214,
> +                                    const struct imx214_mode *mode,
> +                                    struct v4l2_mbus_framefmt *fmt, u32 code)
> +{
> +       fmt->code = IMX214_MBUS_CODE;
> +       fmt->width = mode->width;
> +       fmt->height = mode->height;
> +       fmt->field = V4L2_FIELD_NONE;
> +       fmt->colorspace = V4L2_COLORSPACE_SRGB;
> +       fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> +       fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> +                                                         fmt->colorspace,
> +                                                         fmt->ycbcr_enc);
> +       fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> +}
> +
>  static int imx214_enum_mbus_code(struct v4l2_subdev *sd,
>                                  struct v4l2_subdev_state *sd_state,
>                                  struct v4l2_subdev_mbus_code_enum *code)
> @@ -549,52 +559,6 @@ static const struct v4l2_subdev_core_ops imx214_core_ops = {
>  #endif
>  };
>
> -static struct v4l2_mbus_framefmt *
> -__imx214_get_pad_format(struct imx214 *imx214,
> -                       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 &imx214->fmt;
> -       default:
> -               return NULL;
> -       }
> -}
> -
> -static int imx214_get_format(struct v4l2_subdev *sd,
> -                            struct v4l2_subdev_state *sd_state,
> -                            struct v4l2_subdev_format *format)
> -{
> -       struct imx214 *imx214 = to_imx214(sd);
> -
> -       mutex_lock(&imx214->mutex);
> -       format->format = *__imx214_get_pad_format(imx214, sd_state,
> -                                                 format->pad,
> -                                                 format->which);
> -       mutex_unlock(&imx214->mutex);
> -
> -       return 0;
> -}
> -
> -static struct v4l2_rect *
> -__imx214_get_pad_crop(struct imx214 *imx214,
> -                     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 &imx214->crop;
> -       default:
> -               return NULL;
> -       }
> -}
> -
>  static int imx214_set_format(struct v4l2_subdev *sd,
>                              struct v4l2_subdev_state *sd_state,
>                              struct v4l2_subdev_format *format)
> @@ -604,34 +568,23 @@ static int imx214_set_format(struct v4l2_subdev *sd,
>         struct v4l2_rect *__crop;
>         const struct imx214_mode *mode;
>
> -       mutex_lock(&imx214->mutex);
> -
> -       __crop = __imx214_get_pad_crop(imx214, sd_state, format->pad,
> -                                      format->which);
> -
>         mode = v4l2_find_nearest_size(imx214_modes,
>                                       ARRAY_SIZE(imx214_modes), width, height,
>                                       format->format.width,
>                                       format->format.height);
>
> -       __crop->width = mode->width;
> -       __crop->height = mode->height;
> +       imx214_update_pad_format(imx214, mode, &format->format,
> +                                format->format.code);
> +       __format = v4l2_subdev_state_get_format(sd_state, 0);
>
> -       __format = __imx214_get_pad_format(imx214, sd_state, format->pad,
> -                                          format->which);
> -       __format->width = __crop->width;
> -       __format->height = __crop->height;
> -       __format->code = IMX214_MBUS_CODE;
> -       __format->field = V4L2_FIELD_NONE;
> -       __format->colorspace = V4L2_COLORSPACE_SRGB;
> -       __format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(__format->colorspace);
> -       __format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> -                               __format->colorspace, __format->ycbcr_enc);
> -       __format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(__format->colorspace);
> +       *__format = format->format;
>
> -       format->format = *__format;
> +       __crop = v4l2_subdev_state_get_crop(sd_state, 0);
> +       __crop->width = mode->width;
> +       __crop->height = mode->height;
>
> -       mutex_unlock(&imx214->mutex);
> +       if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +               imx214->cur_mode = mode;
>
>         return 0;
>  }
> @@ -640,14 +593,9 @@ static int imx214_get_selection(struct v4l2_subdev *sd,
>                                 struct v4l2_subdev_state *sd_state,
>                                 struct v4l2_subdev_selection *sel)
>  {
> -       struct imx214 *imx214 = to_imx214(sd);
> -
>         switch (sel->target) {
>         case V4L2_SEL_TGT_CROP:
> -               mutex_lock(&imx214->mutex);
> -               sel->r = *__imx214_get_pad_crop(imx214, sd_state, sel->pad,
> -                                               sel->which);
> -               mutex_unlock(&imx214->mutex);
> +               sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
>                 return 0;
>
>         case V4L2_SEL_TGT_NATIVE_SIZE:
> @@ -826,40 +774,28 @@ static int imx214_write_table(struct imx214 *imx214,
>
>  static int imx214_start_streaming(struct imx214 *imx214)
>  {
> -       const struct imx214_mode *mode;
>         int ret;
>
> -       mutex_lock(&imx214->mutex);
>         ret = imx214_write_table(imx214, mode_table_common);
>         if (ret < 0) {
>                 dev_err(imx214->dev, "could not sent common table %d\n", ret);
> -               goto error;
> +               return ret;
>         }
>
> -       mode = v4l2_find_nearest_size(imx214_modes,
> -                               ARRAY_SIZE(imx214_modes), width, height,
> -                               imx214->fmt.width, imx214->fmt.height);
> -       ret = imx214_write_table(imx214, mode->reg_table);
> +       ret = imx214_write_table(imx214, imx214->cur_mode->reg_table);
>         if (ret < 0) {
>                 dev_err(imx214->dev, "could not sent mode table %d\n", ret);
> -               goto error;
> +               return ret;
>         }
>         ret = __v4l2_ctrl_handler_setup(&imx214->ctrls);
>         if (ret < 0) {
>                 dev_err(imx214->dev, "could not sync v4l2 controls\n");
> -               goto error;
> +               return ret;
>         }
>         ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STREAMING);
> -       if (ret < 0) {
> +       if (ret < 0)
>                 dev_err(imx214->dev, "could not sent start table %d\n", ret);
> -               goto error;
> -       }
>
> -       mutex_unlock(&imx214->mutex);
> -       return 0;
> -
> -error:
> -       mutex_unlock(&imx214->mutex);
>         return ret;
>  }
>
> @@ -877,6 +813,7 @@ static int imx214_stop_streaming(struct imx214 *imx214)
>  static int imx214_s_stream(struct v4l2_subdev *subdev, int enable)
>  {
>         struct imx214 *imx214 = to_imx214(subdev);
> +       struct v4l2_subdev_state *state;
>         int ret;
>
>         if (enable) {
> @@ -884,7 +821,9 @@ static int imx214_s_stream(struct v4l2_subdev *subdev, int enable)
>                 if (ret < 0)
>                         return ret;
>
> +               state = v4l2_subdev_lock_and_get_active_state(subdev);
>                 ret = imx214_start_streaming(imx214);
> +               v4l2_subdev_unlock_state(state);
>                 if (ret < 0)
>                         goto err_rpm_put;
>         } else {
> @@ -948,7 +887,7 @@ static const struct v4l2_subdev_pad_ops imx214_subdev_pad_ops = {
>         .enum_mbus_code = imx214_enum_mbus_code,
>         .enum_frame_size = imx214_enum_frame_size,
>         .enum_frame_interval = imx214_enum_frame_interval,
> -       .get_fmt = imx214_get_format,
> +       .get_fmt = v4l2_subdev_get_fmt,
>         .set_fmt = imx214_set_format,
>         .get_selection = imx214_get_selection,
>         .get_frame_interval = imx214_get_frame_interval,
> @@ -1079,13 +1018,13 @@ static int imx214_probe(struct i2c_client *client)
>         pm_runtime_enable(imx214->dev);
>         pm_runtime_idle(imx214->dev);
>
> +       /* Set default mode to max resolution */
> +       imx214->cur_mode = &imx214_modes[0];
> +
>         ret = imx214_ctrls_init(imx214);
>         if (ret < 0)
>                 goto error_power_off;
>
> -       mutex_init(&imx214->mutex);
> -       imx214->ctrls.lock = &imx214->mutex;
> -
>         imx214->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>         imx214->pad.flags = MEDIA_PAD_FL_SOURCE;
>         imx214->sd.dev = &client->dev;
> @@ -1097,20 +1036,27 @@ static int imx214_probe(struct i2c_client *client)
>                 goto free_ctrl;
>         }
>
> -       imx214_entity_init_state(&imx214->sd, NULL);
> +       imx214->sd.state_lock = imx214->ctrls.lock;
> +       ret = v4l2_subdev_init_finalize(&imx214->sd);
> +       if (ret < 0) {
> +               dev_err(dev, "subdev init error: %d\n", ret);
> +               goto free_entity;
> +       }
>
>         ret = v4l2_async_register_subdev_sensor(&imx214->sd);
>         if (ret < 0) {
>                 dev_err(dev, "could not register v4l2 device\n");
> -               goto free_entity;
> +               goto error_subdev_cleanup;
>         }
>
>         return 0;
>
> +error_subdev_cleanup:
> +       v4l2_subdev_cleanup(&imx214->sd);
> +
>  free_entity:
>         media_entity_cleanup(&imx214->sd.entity);
>  free_ctrl:
> -       mutex_destroy(&imx214->mutex);
>         v4l2_ctrl_handler_free(&imx214->ctrls);
>  error_power_off:
>         pm_runtime_disable(imx214->dev);
> @@ -1125,13 +1071,12 @@ static void imx214_remove(struct i2c_client *client)
>         struct imx214 *imx214 = to_imx214(sd);
>
>         v4l2_async_unregister_subdev(&imx214->sd);
> +       v4l2_subdev_cleanup(sd);
>         media_entity_cleanup(&imx214->sd.entity);
>         v4l2_ctrl_handler_free(&imx214->ctrls);
>
>         pm_runtime_disable(&client->dev);
>         pm_runtime_set_suspended(&client->dev);
> -
> -       mutex_destroy(&imx214->mutex);
>  }
>
>  static const struct of_device_id imx214_of_match[] = {
>
> --
> 2.47.1
>
>
Sakari Ailus Dec. 17, 2024, 9:55 a.m. UTC | #2 
Hi André,

On Mon, Dec 16, 2024 at 10:00:47PM +0100, André Apitzsch via B4 Relay wrote:
> @@ -604,34 +568,23 @@ static int imx214_set_format(struct v4l2_subdev *sd,
>  	struct v4l2_rect *__crop;
>  	const struct imx214_mode *mode;
>  
> -	mutex_lock(&imx214->mutex);
> -
> -	__crop = __imx214_get_pad_crop(imx214, sd_state, format->pad,
> -				       format->which);
> -
>  	mode = v4l2_find_nearest_size(imx214_modes,
>  				      ARRAY_SIZE(imx214_modes), width, height,
>  				      format->format.width,
>  				      format->format.height);
>  
> -	__crop->width = mode->width;
> -	__crop->height = mode->height;
> +	imx214_update_pad_format(imx214, mode, &format->format,
> +				 format->format.code);
> +	__format = v4l2_subdev_state_get_format(sd_state, 0);
>  
> -	__format = __imx214_get_pad_format(imx214, sd_state, format->pad,
> -					   format->which);
> -	__format->width = __crop->width;
> -	__format->height = __crop->height;
> -	__format->code = IMX214_MBUS_CODE;
> -	__format->field = V4L2_FIELD_NONE;
> -	__format->colorspace = V4L2_COLORSPACE_SRGB;
> -	__format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(__format->colorspace);
> -	__format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> -				__format->colorspace, __format->ycbcr_enc);
> -	__format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(__format->colorspace);
> +	*__format = format->format;
>  
> -	format->format = *__format;
> +	__crop = v4l2_subdev_state_get_crop(sd_state, 0);
> +	__crop->width = mode->width;
> +	__crop->height = mode->height;
>  
> -	mutex_unlock(&imx214->mutex);
> +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +		imx214->cur_mode = mode;

While the patch is a major improvement as of now, it'd be nice to get rid
of the cur_mode field. Could you instead obtai nthe mode using
v4l2_find_nearest_size()?

I'm fine with changing this in a new patch on top of the set.
diff mbox series

Patch

diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 4962cfe7c83d62425aeccb46a400fa93146f14ea..2e4322b90938c880896b216d3c46ccaa29485562 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -59,8 +59,6 @@  struct imx214 {
 
 	struct v4l2_subdev sd;
 	struct media_pad pad;
-	struct v4l2_mbus_framefmt fmt;
-	struct v4l2_rect crop;
 
 	struct v4l2_ctrl_handler ctrls;
 	struct v4l2_ctrl *pixel_rate;
@@ -68,15 +66,11 @@  struct imx214 {
 	struct v4l2_ctrl *exposure;
 	struct v4l2_ctrl *unit_size;
 
+	const struct imx214_mode *cur_mode;
+
 	struct regulator_bulk_data	supplies[IMX214_NUM_SUPPLIES];
 
 	struct gpio_desc *enable_gpio;
-
-	/*
-	 * Serialize control access, get/set format, get selection
-	 * and start streaming.
-	 */
-	struct mutex mutex;
 };
 
 struct reg_8 {
@@ -490,6 +484,22 @@  static int __maybe_unused imx214_power_off(struct device *dev)
 	return 0;
 }
 
+static void imx214_update_pad_format(struct imx214 *imx214,
+				     const struct imx214_mode *mode,
+				     struct v4l2_mbus_framefmt *fmt, u32 code)
+{
+	fmt->code = IMX214_MBUS_CODE;
+	fmt->width = mode->width;
+	fmt->height = mode->height;
+	fmt->field = V4L2_FIELD_NONE;
+	fmt->colorspace = V4L2_COLORSPACE_SRGB;
+	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
+	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
+							  fmt->colorspace,
+							  fmt->ycbcr_enc);
+	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
+}
+
 static int imx214_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
@@ -549,52 +559,6 @@  static const struct v4l2_subdev_core_ops imx214_core_ops = {
 #endif
 };
 
-static struct v4l2_mbus_framefmt *
-__imx214_get_pad_format(struct imx214 *imx214,
-			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 &imx214->fmt;
-	default:
-		return NULL;
-	}
-}
-
-static int imx214_get_format(struct v4l2_subdev *sd,
-			     struct v4l2_subdev_state *sd_state,
-			     struct v4l2_subdev_format *format)
-{
-	struct imx214 *imx214 = to_imx214(sd);
-
-	mutex_lock(&imx214->mutex);
-	format->format = *__imx214_get_pad_format(imx214, sd_state,
-						  format->pad,
-						  format->which);
-	mutex_unlock(&imx214->mutex);
-
-	return 0;
-}
-
-static struct v4l2_rect *
-__imx214_get_pad_crop(struct imx214 *imx214,
-		      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 &imx214->crop;
-	default:
-		return NULL;
-	}
-}
-
 static int imx214_set_format(struct v4l2_subdev *sd,
 			     struct v4l2_subdev_state *sd_state,
 			     struct v4l2_subdev_format *format)
@@ -604,34 +568,23 @@  static int imx214_set_format(struct v4l2_subdev *sd,
 	struct v4l2_rect *__crop;
 	const struct imx214_mode *mode;
 
-	mutex_lock(&imx214->mutex);
-
-	__crop = __imx214_get_pad_crop(imx214, sd_state, format->pad,
-				       format->which);
-
 	mode = v4l2_find_nearest_size(imx214_modes,
 				      ARRAY_SIZE(imx214_modes), width, height,
 				      format->format.width,
 				      format->format.height);
 
-	__crop->width = mode->width;
-	__crop->height = mode->height;
+	imx214_update_pad_format(imx214, mode, &format->format,
+				 format->format.code);
+	__format = v4l2_subdev_state_get_format(sd_state, 0);
 
-	__format = __imx214_get_pad_format(imx214, sd_state, format->pad,
-					   format->which);
-	__format->width = __crop->width;
-	__format->height = __crop->height;
-	__format->code = IMX214_MBUS_CODE;
-	__format->field = V4L2_FIELD_NONE;
-	__format->colorspace = V4L2_COLORSPACE_SRGB;
-	__format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(__format->colorspace);
-	__format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
-				__format->colorspace, __format->ycbcr_enc);
-	__format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(__format->colorspace);
+	*__format = format->format;
 
-	format->format = *__format;
+	__crop = v4l2_subdev_state_get_crop(sd_state, 0);
+	__crop->width = mode->width;
+	__crop->height = mode->height;
 
-	mutex_unlock(&imx214->mutex);
+	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		imx214->cur_mode = mode;
 
 	return 0;
 }
@@ -640,14 +593,9 @@  static int imx214_get_selection(struct v4l2_subdev *sd,
 				struct v4l2_subdev_state *sd_state,
 				struct v4l2_subdev_selection *sel)
 {
-	struct imx214 *imx214 = to_imx214(sd);
-
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP:
-		mutex_lock(&imx214->mutex);
-		sel->r = *__imx214_get_pad_crop(imx214, sd_state, sel->pad,
-						sel->which);
-		mutex_unlock(&imx214->mutex);
+		sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
 		return 0;
 
 	case V4L2_SEL_TGT_NATIVE_SIZE:
@@ -826,40 +774,28 @@  static int imx214_write_table(struct imx214 *imx214,
 
 static int imx214_start_streaming(struct imx214 *imx214)
 {
-	const struct imx214_mode *mode;
 	int ret;
 
-	mutex_lock(&imx214->mutex);
 	ret = imx214_write_table(imx214, mode_table_common);
 	if (ret < 0) {
 		dev_err(imx214->dev, "could not sent common table %d\n", ret);
-		goto error;
+		return ret;
 	}
 
-	mode = v4l2_find_nearest_size(imx214_modes,
-				ARRAY_SIZE(imx214_modes), width, height,
-				imx214->fmt.width, imx214->fmt.height);
-	ret = imx214_write_table(imx214, mode->reg_table);
+	ret = imx214_write_table(imx214, imx214->cur_mode->reg_table);
 	if (ret < 0) {
 		dev_err(imx214->dev, "could not sent mode table %d\n", ret);
-		goto error;
+		return ret;
 	}
 	ret = __v4l2_ctrl_handler_setup(&imx214->ctrls);
 	if (ret < 0) {
 		dev_err(imx214->dev, "could not sync v4l2 controls\n");
-		goto error;
+		return ret;
 	}
 	ret = regmap_write(imx214->regmap, IMX214_REG_MODE_SELECT, IMX214_MODE_STREAMING);
-	if (ret < 0) {
+	if (ret < 0)
 		dev_err(imx214->dev, "could not sent start table %d\n", ret);
-		goto error;
-	}
 
-	mutex_unlock(&imx214->mutex);
-	return 0;
-
-error:
-	mutex_unlock(&imx214->mutex);
 	return ret;
 }
 
@@ -877,6 +813,7 @@  static int imx214_stop_streaming(struct imx214 *imx214)
 static int imx214_s_stream(struct v4l2_subdev *subdev, int enable)
 {
 	struct imx214 *imx214 = to_imx214(subdev);
+	struct v4l2_subdev_state *state;
 	int ret;
 
 	if (enable) {
@@ -884,7 +821,9 @@  static int imx214_s_stream(struct v4l2_subdev *subdev, int enable)
 		if (ret < 0)
 			return ret;
 
+		state = v4l2_subdev_lock_and_get_active_state(subdev);
 		ret = imx214_start_streaming(imx214);
+		v4l2_subdev_unlock_state(state);
 		if (ret < 0)
 			goto err_rpm_put;
 	} else {
@@ -948,7 +887,7 @@  static const struct v4l2_subdev_pad_ops imx214_subdev_pad_ops = {
 	.enum_mbus_code = imx214_enum_mbus_code,
 	.enum_frame_size = imx214_enum_frame_size,
 	.enum_frame_interval = imx214_enum_frame_interval,
-	.get_fmt = imx214_get_format,
+	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = imx214_set_format,
 	.get_selection = imx214_get_selection,
 	.get_frame_interval = imx214_get_frame_interval,
@@ -1079,13 +1018,13 @@  static int imx214_probe(struct i2c_client *client)
 	pm_runtime_enable(imx214->dev);
 	pm_runtime_idle(imx214->dev);
 
+	/* Set default mode to max resolution */
+	imx214->cur_mode = &imx214_modes[0];
+
 	ret = imx214_ctrls_init(imx214);
 	if (ret < 0)
 		goto error_power_off;
 
-	mutex_init(&imx214->mutex);
-	imx214->ctrls.lock = &imx214->mutex;
-
 	imx214->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	imx214->pad.flags = MEDIA_PAD_FL_SOURCE;
 	imx214->sd.dev = &client->dev;
@@ -1097,20 +1036,27 @@  static int imx214_probe(struct i2c_client *client)
 		goto free_ctrl;
 	}
 
-	imx214_entity_init_state(&imx214->sd, NULL);
+	imx214->sd.state_lock = imx214->ctrls.lock;
+	ret = v4l2_subdev_init_finalize(&imx214->sd);
+	if (ret < 0) {
+		dev_err(dev, "subdev init error: %d\n", ret);
+		goto free_entity;
+	}
 
 	ret = v4l2_async_register_subdev_sensor(&imx214->sd);
 	if (ret < 0) {
 		dev_err(dev, "could not register v4l2 device\n");
-		goto free_entity;
+		goto error_subdev_cleanup;
 	}
 
 	return 0;
 
+error_subdev_cleanup:
+	v4l2_subdev_cleanup(&imx214->sd);
+
 free_entity:
 	media_entity_cleanup(&imx214->sd.entity);
 free_ctrl:
-	mutex_destroy(&imx214->mutex);
 	v4l2_ctrl_handler_free(&imx214->ctrls);
 error_power_off:
 	pm_runtime_disable(imx214->dev);
@@ -1125,13 +1071,12 @@  static void imx214_remove(struct i2c_client *client)
 	struct imx214 *imx214 = to_imx214(sd);
 
 	v4l2_async_unregister_subdev(&imx214->sd);
+	v4l2_subdev_cleanup(sd);
 	media_entity_cleanup(&imx214->sd.entity);
 	v4l2_ctrl_handler_free(&imx214->ctrls);
 
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
-
-	mutex_destroy(&imx214->mutex);
 }
 
 static const struct of_device_id imx214_of_match[] = {