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

[Hans de Goede]

Hi,

On 27-Mar-25 08:41, Sakari Ailus wrote:
> 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.

Or see the conversion to sub-device state in my incremental v8 posting
of the ov02c10 series. Converting this driver should be alsmost
identical.

>> +
>> +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.

Also see my incremental v8 posting of the ov02c10 series.

> 
>> +
>> +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.

Also see my incremental v8 posting of the ov02c10 series.

Regards,

Hans