Re: [PATCH v2 2/2] media: i2c: ov02e10: add OV02E10 image sensor driver
From: Sakari Ailus
Date: Thu Mar 27 2025 - 03:42:09 EST
A few more comments.
On Tue, Mar 25, 2025 at 02:49:29PM +0000, Bryan O'Donoghue wrote:
> +static int ov02e10_set_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct ov02e10 *ov02e10 = to_ov02e10(sd);
> + const struct ov02e10_mode *mode;
> + s32 vblank_def, h_blank;
> +
> + mode = v4l2_find_nearest_size(supported_modes,
> + ARRAY_SIZE(supported_modes),
> + width, height, fmt->format.width,
> + fmt->format.height);
> +
> + mutex_lock(&ov02e10->mutex);
> + ov02e10_update_pad_format(mode, &fmt->format);
> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> + *v4l2_subdev_state_get_format(sd_state, fmt->pad) =
> + fmt->format;
> + } else {
> + ov02e10->cur_mode = mode;
> + __v4l2_ctrl_s_ctrl(ov02e10->link_freq, mode->link_freq_index);
> + __v4l2_ctrl_s_ctrl_int64(ov02e10->pixel_rate,
> + to_pixel_rate(mode->link_freq_index));
> +
> + /* Update limits and set FPS to default */
> + vblank_def = mode->vts_def - mode->height;
> + __v4l2_ctrl_modify_range(ov02e10->vblank,
> + mode->vts_min - mode->height,
> + OV02E10_VTS_MAX - mode->height, 1,
> + vblank_def);
Note that this can fail.
> + __v4l2_ctrl_s_ctrl(ov02e10->vblank, vblank_def);
As well as this one.
> + h_blank = to_pixels_per_line(mode->hts, mode->link_freq_index) -
> + mode->width;
> + __v4l2_ctrl_modify_range(ov02e10->hblank, h_blank, h_blank, 1,
> + h_blank);
> + }
> + mutex_unlock(&ov02e10->mutex);
> +
> + return 0;
> +}
Please rely on sub-device state and the state lock instead, see e.g. imx219
driver for an example.
> +
> +static int ov02e10_get_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct ov02e10 *ov02e10 = to_ov02e10(sd);
> +
> + mutex_lock(&ov02e10->mutex);
> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> + fmt->format = *v4l2_subdev_state_get_format(sd_state, fmt->pad);
> + else
> + ov02e10_update_pad_format(ov02e10->cur_mode, &fmt->format);
> +
> + mutex_unlock(&ov02e10->mutex);
> +
> + return 0;
> +}
And you won't need this with sub-device state.
> +
> +static int ov02e10_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + if (code->index > 0)
> + return -EINVAL;
> +
> + code->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> +
> + return 0;
> +}
> +
> +static int ov02e10_enum_frame_size(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + if (fse->index >= ARRAY_SIZE(supported_modes))
> + return -EINVAL;
> +
> + if (fse->code != MEDIA_BUS_FMT_SGRBG10_1X10)
> + return -EINVAL;
> +
> + fse->min_width = supported_modes[fse->index].width;
> + fse->max_width = fse->min_width;
> + fse->min_height = supported_modes[fse->index].height;
> + fse->max_height = fse->min_height;
> +
> + return 0;
> +}
> +
> +static int ov02e10_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> + struct ov02e10 *ov02e10 = to_ov02e10(sd);
> +
> + mutex_lock(&ov02e10->mutex);
> + ov02e10_update_pad_format(&supported_modes[0],
> + v4l2_subdev_state_get_format(fh->state, 0));
Please rely on init_cfg pad op instead.
> + mutex_unlock(&ov02e10->mutex);
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_video_ops ov02e10_video_ops = {
> + .s_stream = ov02e10_set_stream,
Please use {enable,disable}_streams instead. See e.g. imx283 driver for an
example.
> +};
--
Sakari Ailus