Re: [PATCH v2 6/8] media: imx355: Use subdev active state

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

Quoting Jai Luthra (2025-09-11 09:14:22)
> Port the driver to use the subdev active state. This simplifies locking,
> and makes it easier to support different crop sizes for binned modes, by
> storing the crop rectangle inside the subdev state.
> 
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/imx335.c | 79 ++++++++++++----------------------------------
>  1 file changed, 21 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index 7631f41e6f1e65695fb76a66d9ac5a3588c69658..9b9bbe6473d545bc5618c0fe8191df0b18037cd8 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -204,7 +204,6 @@ struct imx335_mode {
>   * @vblank: Vertical blanking in lines
>   * @lane_mode: Mode for number of connected data lanes
>   * @cur_mode: Pointer to current selected sensor mode
> - * @mutex: Mutex for serializing sensor controls
>   * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
>   * @cur_mbus_code: Currently selected media bus format code
>   */
> @@ -231,7 +230,6 @@ struct imx335 {
>         u32 vblank;
>         u32 lane_mode;
>         const struct imx335_mode *cur_mode;
> -       struct mutex mutex;
>         unsigned long link_freq_bitmap;
>         u32 cur_mbus_code;
>  };
> @@ -766,36 +764,6 @@ static void imx335_fill_pad_format(struct imx335 *imx335,
>         fmt->format.xfer_func = V4L2_XFER_FUNC_NONE;
>  }
>  
> -/**
> - * imx335_get_pad_format() - Get subdevice pad format
> - * @sd: pointer to imx335 V4L2 sub-device structure
> - * @sd_state: V4L2 sub-device configuration
> - * @fmt: V4L2 sub-device format need to be set
> - *
> - * Return: 0 if successful, error code otherwise.
> - */
> -static int imx335_get_pad_format(struct v4l2_subdev *sd,
> -                                struct v4l2_subdev_state *sd_state,
> -                                struct v4l2_subdev_format *fmt)
> -{
> -       struct imx335 *imx335 = to_imx335(sd);
> -
> -       mutex_lock(&imx335->mutex);
> -
> -       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> -               struct v4l2_mbus_framefmt *framefmt;
> -
> -               framefmt = v4l2_subdev_state_get_format(sd_state, fmt->pad);
> -               fmt->format = *framefmt;
> -       } else {
> -               imx335_fill_pad_format(imx335, imx335->cur_mode, fmt);
> -       }
> -
> -       mutex_unlock(&imx335->mutex);
> -
> -       return 0;
> -}
> -
>  /**
>   * imx335_set_pad_format() - Set subdevice pad format
>   * @sd: pointer to imx335 V4L2 sub-device structure
> @@ -809,12 +777,12 @@ static int imx335_set_pad_format(struct v4l2_subdev *sd,
>                                  struct v4l2_subdev_format *fmt)
>  {
>         struct imx335 *imx335 = to_imx335(sd);
> +       struct v4l2_mbus_framefmt *format;
>         const struct imx335_mode *mode;
>         int i, ret = 0;
>  
> -       mutex_lock(&imx335->mutex);
> -
>         mode = &supported_mode;
> +
>         for (i = 0; i < ARRAY_SIZE(imx335_mbus_codes); i++) {
>                 if (imx335_mbus_codes[i] == fmt->format.code)
>                         imx335->cur_mbus_code = imx335_mbus_codes[i];
> @@ -822,19 +790,15 @@ static int imx335_set_pad_format(struct v4l2_subdev *sd,
>  
>         imx335_fill_pad_format(imx335, mode, fmt);
>  
> -       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> -               struct v4l2_mbus_framefmt *framefmt;
> +       format = v4l2_subdev_state_get_format(sd_state, fmt->pad);
> +       *format = fmt->format;
>  
> -               framefmt = v4l2_subdev_state_get_format(sd_state, fmt->pad);
> -               *framefmt = fmt->format;
> -       } else {

It's nice to drop that - and clean up the locking.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +       if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>                 ret = imx335_update_controls(imx335, mode);
>                 if (!ret)
>                         imx335->cur_mode = mode;
>         }
>  
> -       mutex_unlock(&imx335->mutex);
> -
>         return ret;
>  }
>  
> @@ -854,12 +818,10 @@ static int imx335_init_state(struct v4l2_subdev *sd,
>         fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
>         imx335_fill_pad_format(imx335, &supported_mode, &fmt);
>  
> -       mutex_lock(&imx335->mutex);
>         __v4l2_ctrl_modify_range(imx335->link_freq_ctrl, 0,
>                                  __fls(imx335->link_freq_bitmap),
>                                  ~(imx335->link_freq_bitmap),
>                                  __ffs(imx335->link_freq_bitmap));
> -       mutex_unlock(&imx335->mutex);
>  
>         return imx335_set_pad_format(sd, sd_state, &fmt);
>  }
> @@ -1001,16 +963,17 @@ static void imx335_stop_streaming(struct imx335 *imx335)
>  static int imx335_set_stream(struct v4l2_subdev *sd, int enable)
>  {
>         struct imx335 *imx335 = to_imx335(sd);
> +       struct v4l2_subdev_state *state;
>         int ret = 0;
>  
> -       mutex_lock(&imx335->mutex);
> +       state = v4l2_subdev_lock_and_get_active_state(sd);
>  
>         if (enable)
>                 ret = imx335_start_streaming(imx335);
>         else
>                 imx335_stop_streaming(imx335);
>  
> -       mutex_unlock(&imx335->mutex);
> +       v4l2_subdev_unlock_state(state);
>  
>         return ret;
>  }
> @@ -1138,7 +1101,7 @@ static const struct v4l2_subdev_pad_ops imx335_pad_ops = {
>         .enum_frame_size = imx335_enum_frame_size,
>         .get_selection = imx335_get_selection,
>         .set_selection = imx335_get_selection,
> -       .get_fmt = imx335_get_pad_format,
> +       .get_fmt = v4l2_subdev_get_fmt,
>         .set_fmt = imx335_set_pad_format,
>  };
>  
> @@ -1233,9 +1196,6 @@ static int imx335_init_controls(struct imx335 *imx335)
>         if (ret)
>                 return ret;
>  
> -       /* Serialize controls with sensor device */
> -       ctrl_hdlr->lock = &imx335->mutex;
> -
>         /* Initialize exposure and gain */
>         lpfr = mode->vblank + mode->height;
>         imx335->exp_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
> @@ -1355,12 +1315,10 @@ static int imx335_probe(struct i2c_client *client)
>                 return ret;
>         }
>  
> -       mutex_init(&imx335->mutex);
> -
>         ret = imx335_power_on(imx335->dev);
>         if (ret) {
>                 dev_err(imx335->dev, "failed to power-on the sensor\n");
> -               goto error_mutex_destroy;
> +               return ret;
>         }
>  
>         /* Check module identity */
> @@ -1393,11 +1351,18 @@ static int imx335_probe(struct i2c_client *client)
>                 goto error_handler_free;
>         }
>  
> +       imx335->sd.state_lock = imx335->ctrl_handler.lock;
> +       ret = v4l2_subdev_init_finalize(&imx335->sd);
> +       if (ret < 0) {
> +               dev_err(imx335->dev, "subdev init error\n");
> +               goto error_media_entity;
> +       }
> +
>         ret = v4l2_async_register_subdev_sensor(&imx335->sd);
>         if (ret < 0) {
>                 dev_err(imx335->dev,
>                         "failed to register async subdev: %d\n", ret);
> -               goto error_media_entity;
> +               goto error_subdev_cleanup;
>         }
>  
>         pm_runtime_set_active(imx335->dev);
> @@ -1406,14 +1371,14 @@ static int imx335_probe(struct i2c_client *client)
>  
>         return 0;
>  
> +error_subdev_cleanup:
> +       v4l2_subdev_cleanup(&imx335->sd);
>  error_media_entity:
>         media_entity_cleanup(&imx335->sd.entity);
>  error_handler_free:
>         v4l2_ctrl_handler_free(imx335->sd.ctrl_handler);
>  error_power_off:
>         imx335_power_off(imx335->dev);
> -error_mutex_destroy:
> -       mutex_destroy(&imx335->mutex);
>  
>         return ret;
>  }
> @@ -1427,9 +1392,9 @@ static int imx335_probe(struct i2c_client *client)
>  static void imx335_remove(struct i2c_client *client)
>  {
>         struct v4l2_subdev *sd = i2c_get_clientdata(client);
> -       struct imx335 *imx335 = to_imx335(sd);
>  
>         v4l2_async_unregister_subdev(sd);
> +       v4l2_subdev_cleanup(sd);
>         media_entity_cleanup(&sd->entity);
>         v4l2_ctrl_handler_free(sd->ctrl_handler);
>  
> @@ -1437,8 +1402,6 @@ static void imx335_remove(struct i2c_client *client)
>         if (!pm_runtime_status_suspended(&client->dev))
>                 imx335_power_off(&client->dev);
>         pm_runtime_set_suspended(&client->dev);
> -
> -       mutex_destroy(&imx335->mutex);
>  }
>  
>  static const struct dev_pm_ops imx335_pm_ops = {
> 
> -- 
> 2.51.0
>