Re: [PATCH 6/6] media: i2c: ov5670: Add .get_selection() support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

Thank you for the patch.

On Thu, Mar 10, 2022 at 02:08:29PM +0100, Jacopo Mondi wrote:
> From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> 
> The ov5670 driver's v4l2_subdev_pad_ops currently does not include
> .get_selection() - add support for that callback.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5670.c | 64 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index c9f69ffef210..1fa0d632c536 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -70,6 +70,15 @@
>  #define OV5670_REG_VALUE_16BIT		2
>  #define OV5670_REG_VALUE_24BIT		3
>  
> +/* Pixel Array */
> +
> +#define OV5670_NATIVE_WIDTH		2624
> +#define OV5670_NATIVE_HEIGHT		1980
> +#define OV5670_ACTIVE_START_TOP		8
> +#define OV5670_ACTIVE_START_LEFT	16
> +#define OV5670_ACTIVE_WIDTH		2592
> +#define OV5670_ACTIVE_HEIGHT		1944
> +
>  /* Initial number of frames to skip to avoid possible garbage */
>  #define OV5670_NUM_OF_SKIP_FRAMES	2
>  
> @@ -2491,6 +2500,59 @@ static const struct v4l2_subdev_core_ops ov5670_core_ops = {
>  	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
>  };
>  
> +static void
> +__ov5670_get_pad_crop(struct ov5670 *sensor,
> +		      struct v4l2_subdev_state *state, unsigned int pad,
> +		      enum v4l2_subdev_format_whence which, struct v4l2_rect *r)
> +{
> +	const struct ov5670_mode *mode = sensor->cur_mode;
> +
> +	switch (which) {
> +	case V4L2_SUBDEV_FORMAT_TRY:
> +		*r = *v4l2_subdev_get_try_crop(&sensor->sd, state, pad);

Where is the try crop rectangle initialized ?

> +		break;
> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> +		r->height = mode->height;
> +		r->width = mode->width;
> +		r->top = (OV5670_NATIVE_HEIGHT - mode->height) / 2;
> +		r->left = (OV5670_NATIVE_WIDTH - mode->width) / 2;

There's a comment above stating

/*
 * OV5670 sensor supports following resolutions with full FOV:
 * 4:3  ==> {2592x1944, 1296x972, 648x486}
 * 16:9 ==> {2560x1440, 1280x720, 640x360}
 */

so this doesn't look right.

> +		break;
> +	}
> +}
> +
> +static int ov5670_get_selection(struct v4l2_subdev *subdev,
> +				struct v4l2_subdev_state *state,
> +				struct v4l2_subdev_selection *sel)
> +{
> +	struct ov5670 *sensor = to_ov5670(subdev);
> +
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP:
> +		mutex_lock(&sensor->mutex);
> +			__ov5670_get_pad_crop(sensor, state, sel->pad,
> +					      sel->which, &sel->r);

Wrong indentation.

> +		mutex_unlock(&sensor->mutex);
> +		break;
> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = OV5670_NATIVE_WIDTH;
> +		sel->r.height = OV5670_NATIVE_HEIGHT;
> +		break;
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +		sel->r.top = OV5670_ACTIVE_START_TOP;
> +		sel->r.left = OV5670_ACTIVE_START_LEFT;
> +		sel->r.width = OV5670_ACTIVE_WIDTH;
> +		sel->r.height = OV5670_ACTIVE_HEIGHT;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct v4l2_subdev_video_ops ov5670_video_ops = {
>  	.s_stream = ov5670_set_stream,
>  };
> @@ -2500,6 +2562,8 @@ static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
>  	.get_fmt = ov5670_get_pad_format,
>  	.set_fmt = ov5670_set_pad_format,
>  	.enum_frame_size = ov5670_enum_frame_size,
> +	.get_selection = ov5670_get_selection,
> +	.set_selection = ov5670_get_selection,
>  };
>  
>  static const struct v4l2_subdev_sensor_ops ov5670_sensor_ops = {

-- 
Regards,

Laurent Pinchart