Re: [PATCH v2 2/2] media: i2c: ov02e10: add OV02E10 image sensor driver

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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