Re: [PATCH 10/13] media: imx355: Add support for get_selection

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

On Wed, May 06, 2026 at 07:23:48PM +0100, Dave Stevenson wrote:
> Provide all the cropping information via get_selection.

I think this could be simplified if the driver is ported to use the
active state.
See df3ef05b51e02ef9386346288c1e63f366372f5b

I'm afraid usage of the active state is warmly suggested nowadays,
especially if you're adding code that has to deal with ACTIVE/TRY or
initializes per-fh data in open().

Is it too much yak shaving to ask ?

>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/imx355.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
>
> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> index 5a3bfcd0f51c..d8d7cc0ceab9 100644
> --- a/drivers/media/i2c/imx355.c
> +++ b/drivers/media/i2c/imx355.c
> @@ -88,6 +88,11 @@
>  /* number of data lanes */
>  #define IMX355_DATA_LANES		4
>
> +#define IMX355_PIXEL_ARRAY_TOP		0
> +#define IMX355_PIXEL_ARRAY_LEFT		0
> +#define IMX355_PIXEL_ARRAY_WIDTH	3280
> +#define IMX355_PIXEL_ARRAY_HEIGHT	2464
> +
>  struct imx355_reg {
>  	u16 address;
>  	u8 val;
> @@ -671,6 +676,7 @@ static int imx355_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  	struct imx355 *imx355 = to_imx355(sd);
>  	struct v4l2_mbus_framefmt *try_fmt =
>  		v4l2_subdev_state_get_format(fh->state, 0);
> +	struct v4l2_rect *crop = v4l2_subdev_state_get_crop(fh->state, 0);
>
>  	mutex_lock(&imx355->mutex);
>
> @@ -680,6 +686,11 @@ static int imx355_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  	try_fmt->code = imx355_get_format_code(imx355);
>  	try_fmt->field = V4L2_FIELD_NONE;
>
> +	crop->left = imx355->cur_mode->x_add_start;
> +	crop->top = imx355->cur_mode->y_add_start;
> +	crop->width = imx355->cur_mode->width;
> +	crop->height = imx355->cur_mode->height;
> +
>  	mutex_unlock(&imx355->mutex);
>
>  	return 0;
> @@ -886,6 +897,52 @@ imx355_set_pad_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>
> +static void
> +__imx355_get_pad_crop(struct imx355 *imx355,
> +		      struct v4l2_subdev_state *sd_state, unsigned int pad,
> +		      enum v4l2_subdev_format_whence which, struct v4l2_rect *r)
> +{
> +	switch (which) {
> +	case V4L2_SUBDEV_FORMAT_TRY:
> +		*r = *v4l2_subdev_state_get_crop(sd_state, pad);
> +		break;
> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> +		r->left = imx355->cur_mode->x_add_start;
> +		r->top = imx355->cur_mode->y_add_start;
> +		r->width = imx355->cur_mode->width;
> +		r->height = imx355->cur_mode->height;
> +		break;
> +	}
> +}
> +
> +static int imx355_get_selection(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_state *sd_state,
> +				struct v4l2_subdev_selection *sel)
> +{
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP:
> +		struct imx355 *imx355 = to_imx355(sd);
> +
> +		mutex_lock(&imx355->mutex);
> +		__imx355_get_pad_crop(imx355, sd_state, sel->pad, sel->which,
> +				      &sel->r);
> +		mutex_unlock(&imx355->mutex);
> +
> +		return 0;
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> +		sel->r.top = IMX355_PIXEL_ARRAY_TOP;
> +		sel->r.left = IMX355_PIXEL_ARRAY_LEFT;
> +		sel->r.width = IMX355_PIXEL_ARRAY_WIDTH;
> +		sel->r.height = IMX355_PIXEL_ARRAY_HEIGHT;
> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  /* Start streaming */
>  static int imx355_start_streaming(struct imx355 *imx355)
>  {
> @@ -1062,6 +1119,7 @@ static const struct v4l2_subdev_pad_ops imx355_pad_ops = {
>  	.get_fmt = imx355_get_pad_format,
>  	.set_fmt = imx355_set_pad_format,
>  	.enum_frame_size = imx355_enum_frame_size,
> +	.get_selection = imx355_get_selection,
>  };
>
>  static const struct v4l2_subdev_ops imx355_subdev_ops = {
>
> --
> 2.34.1
>
>