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

[Dave Stevenson]

Hi Jacopo

Thanks for all your reviews

On Thu, 7 May 2026 at 15:42, Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> wrote:
>
> 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 ?

As this was an existing driver and I can't test on the original
hardware, I was taking a softly softly approach to ensure nothing
broke for the existing users.

Tianshu's email address is bouncing, so I guess Sakari would be the
one to know if Intel still care about IMX355.
If Intel no longer care, then I'll see if I can find the time to swap
it to active state.

  Dave

> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > ---
> >  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
> >
> >