Re: [PATCH 2/2] media: i2c: Add driver for Sony IMX662 sensor

[Jai Luthra <jai.luthra@ideasonboard.com>]

Hi Dave, Alexander,

Quoting Dave Stevenson (2026-03-13 01:20:15)
> Hi Alexander
> 
> On Thu, 12 Mar 2026 at 15:11, Alexander Shiyan
> <eagle.alexander923@gmail.com> wrote:
> >
> > This patch adds a V4L2 subdevice driver for the Sony IMX662 CMOS image
> > sensor. The sensor has a native resolution of 1936x1100 (effective
> > 1920x1080) and can achieve up to 90 frames per second depending on
> > the configuration. The driver supports:
> > - MIPI CSI-2 with 2 or 4 data lanes.
> > - RAW10 and RAW12 formats (both colour and monochrome).
> > - Controls: exposure, analogue gain, horizontal/vertical blanking,
> >   horizontal/vertical flip, brightness.
> > - A placeholder V4L2_CID_HDR_SENSOR_MODE control for future Clear HDR
> >   support (the actual HDR modes are not yet implemented).
> > - Runtime PM.
> > - Cropping via the selection API.
> > - Multiple link frequencies selectable via device tree.
> >
> > Tested on ARM64 Rockchip RK3568 platform with a 24 MHz external clock
> > and various link frequencies.
> 
> Interesting timing as I've been looking at IMX662 too, but on a Raspberry Pi.
> FWIW my driver is at
> https://github.com/6by9/linux/blob/rpi-6.12.y-imx662/drivers/media/i2c/imx662.c
> 

I saw libcamera patches for IMX676, does the below branch have the latest
driver you plan to post as v2?

https://github.com/6by9/linux/blob/rpi-6.18.y-imx662-mainline-clean/drivers/media/i2c/imx662.c 

> Comments are based on brief testing and reading of the code. This is
> not a full review.
> 
> > Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com>

[...]

> > +static int imx662_set_selection(struct v4l2_subdev *sd,
> > +                               struct v4l2_subdev_state *sd_state,
> > +                               struct v4l2_subdev_selection *sel)
> > +{

Can we keep cropping and binning out of the initial driver, and implement
it using the common raw sensor model directly?

I did it for IMX678 [1] on Sakari's suggestion [2]. The two sensors are
quite similar, so I'm happy to help in whatever way I can on getting this
working with the new model too :-)

[1]: https://lore.kernel.org/all/20260703-imx678-meta-v1-0-7c4924a0df05@ideasonboard.com/
[2]: https://lore.kernel.org/all/ag7oq5jO5G0rwvzb@kekkonen.localdomain/

Thanks,
    Jai

> > +       struct imx662 *imx662 = to_imx662(sd);
> > +       struct v4l2_rect rect = sel->r;
> > +       struct v4l2_rect *try_crop;
> > +       struct v4l2_mbus_framefmt *try_fmt;
> > +       u32 max_left, max_top;
> > +
> > +       if (sel->target != V4L2_SEL_TGT_CROP)
> > +               return -EINVAL;
> > +
> > +       max_left = IMX662_PIXEL_ARRAY_LEFT + IMX662_PIXEL_ARRAY_WIDTH -
> > +                  IMX662_MIN_CROP_WIDTH;
> > +       max_top = IMX662_PIXEL_ARRAY_TOP + IMX662_PIXEL_ARRAY_HEIGHT -
> > +                 IMX662_MIN_CROP_HEIGHT;
> > +
> > +       rect.left = clamp_t(u32, rect.left, IMX662_PIXEL_ARRAY_LEFT, max_left);
> > +       rect.top = clamp_t(u32, rect.top, IMX662_PIXEL_ARRAY_TOP, max_top);
> > +
> > +       rect.width =
> > +               clamp_t(u32,
> > +                       round_down(rect.width, IMX662_CROP_WIDTH_STEP),
> > +                       IMX662_MIN_CROP_WIDTH, IMX662_PIXEL_ARRAY_WIDTH);
> > +       rect.height =
> > +               clamp_t(u32,
> > +                       round_down(rect.height, IMX662_CROP_HEIGHT_STEP),
> > +                       IMX662_MIN_CROP_HEIGHT, IMX662_PIXEL_ARRAY_HEIGHT);
> > +
> > +       if (rect.left + rect.width - 1 >
> > +           IMX662_PIXEL_ARRAY_LEFT + IMX662_PIXEL_ARRAY_WIDTH - 1)
> > +               rect.left =
> > +                       IMX662_PIXEL_ARRAY_LEFT + IMX662_PIXEL_ARRAY_WIDTH -
> > +                       rect.width;
> > +       if (rect.top + rect.height - 1 >
> > +           IMX662_PIXEL_ARRAY_TOP + IMX662_PIXEL_ARRAY_HEIGHT - 1)
> > +               rect.top =
> > +                       IMX662_PIXEL_ARRAY_TOP + IMX662_PIXEL_ARRAY_HEIGHT -
> > +                       rect.height;
> > +
> > +       if (sel->flags & V4L2_SEL_FLAG_GE) {
> > +               if (rect.width < sel->r.width) {
> > +                       u32 new_width = rect.width + IMX662_CROP_WIDTH_STEP;
> > +
> > +                       if (new_width <= IMX662_PIXEL_ARRAY_WIDTH)
> > +                               rect.width = new_width;
> > +               }
> > +               if (rect.height < sel->r.height) {
> > +                       u32 new_height = rect.height + IMX662_CROP_HEIGHT_STEP;
> > +
> > +                       if (new_height <= IMX662_PIXEL_ARRAY_HEIGHT)
> > +                               rect.height = new_height;
> > +               }
> > +       }
> > +
> > +       if (sel->flags & V4L2_SEL_FLAG_LE) {
> > +               if (rect.width > sel->r.width &&
> > +                   rect.width >= IMX662_MIN_CROP_WIDTH +
> > +                                 IMX662_CROP_WIDTH_STEP)
> > +                       rect.width -= IMX662_CROP_WIDTH_STEP;
> > +               if (rect.height > sel->r.height &&
> > +                   rect.height >= IMX662_MIN_CROP_HEIGHT +
> > +                                  IMX662_CROP_HEIGHT_STEP)
> > +                       rect.height -= IMX662_CROP_HEIGHT_STEP;
> > +       }
> > +
> > +       if (rect.width < IMX662_MIN_CROP_WIDTH ||
> > +           rect.height < IMX662_MIN_CROP_HEIGHT)
> > +               return -EINVAL;
> > +
> > +       if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
> > +               try_crop = v4l2_subdev_state_get_crop(sd_state, sel->pad);
> > +               *try_crop = rect;
> > +
> > +               try_fmt = v4l2_subdev_state_get_format(sd_state, sel->pad);
> > +               if (try_fmt) {
> > +                       try_fmt->width = rect.width;
> > +                       try_fmt->height = rect.height;
> > +               }
> > +       } else {
> > +               if (imx662->streaming)
> > +                       return -EBUSY;
> > +
> > +               if (imx662->crop.left == rect.left &&
> > +                   imx662->crop.top == rect.top &&
> > +                   imx662->crop.width == rect.width &&
> > +                   imx662->crop.height == rect.height) {
> > +                       sel->r = rect;
> > +                       return 0;
> > +               }
> > +
> > +               imx662->crop = rect;
> > +
> > +               try_fmt = v4l2_subdev_state_get_format(sd_state, sel->pad);
> > +               if (try_fmt) {
> > +                       try_fmt->width = rect.width;
> > +                       try_fmt->height = rect.height;
> > +               }
> > +
> > +               imx662_set_framing_limits(imx662);
> > +       }
> > +
> > +       sel->r = rect;
> > +
> > +       return 0;
> > +}

[...]