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
