Re: [PATCH v4 2/2] media: i2c: add imx576 image sensor driver

[Jai Luthra]

Hi Himanshu,

Thanks for the patch, and converting to CCS registers and helpers.

I only have a few minor comments.

Quoting Himanshu Bhavani (2026-08-19 18:26:40)
> Add a v4l2 subdevice driver for the Sony imx576 sensor.
> 
> The Sony IMX576 image sensor with an active
> array size of 5760 x 4312
> 
> The following features are supported:
> - Manual exposure an gain control support
> - vblank/hblank control support
> - Supported resolution: 5760 x 4312 30fps (SRGGB10)
> 
> Signed-off-by: Himanshu Bhavani <himanshu.bhavani@xxxxxxxxxxxxxxxxx>
> ---
>  MAINTAINERS                |    1 +
>  drivers/media/i2c/Kconfig  |   10 +
>  drivers/media/i2c/Makefile |    1 +
>  drivers/media/i2c/imx576.c | 1103 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 1115 insertions(+)
>  create mode 100644 drivers/media/i2c/imx576.c
> 
> diff --git a/drivers/media/i2c/imx576.c b/drivers/media/i2c/imx576.c
> new file mode 100644
> index 000000000000..8115d0c16cc3
> --- /dev/null
> +++ b/drivers/media/i2c/imx576.c

[...]

> +
> +#define IMX576_LINE_LENGTH             6144
> +#define IMX576_VBLANK_DEF              4387
> +
> +/* FIXME: Exact VBLANK limit unknown (no datasheet). */
> +#define IMX576_VBLANK_MAX              32420

You could try reading the CCS.MAX_FRAME_LENGTH_LINES register (0x1142), in
IMX708 that had a value of 0xffff, which did work during my testing.

[...]

> +       { CCI_REG8(0x0401), 0x00 },

This might be writing to the CCS.SCALING_MODE (0x400) register's lower
byte, so you could use the macro here (unless the higher byte is programmed
to something else?)


[...]

> +static int imx576_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +       struct imx576 *imx576 = container_of_const(ctrl->handler,
> +                                                  struct imx576, handler);
> +       struct v4l2_subdev_state *state;
> +       struct v4l2_mbus_framefmt *fmt;
> +       int ret = 0;
> +
> +       state = v4l2_subdev_get_locked_active_state(&imx576->sd);
> +       fmt = v4l2_subdev_state_get_format(state, 0);
> +
> +       if (ctrl->id == V4L2_CID_VBLANK) {
> +               /* Honour the VBLANK limits when setting exposure */
> +               ret = __v4l2_ctrl_modify_range(imx576->exposure,
> +                                              IMX576_EXPOSURE_MIN,
> +                                              fmt->height + ctrl->val -
> +                                              IMX576_EXPOSURE_OFFSET,
> +                                              IMX576_EXPOSURE_STEP,
> +                                              IMX576_EXPOSURE_DEFAULT);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       if (pm_runtime_get_if_active(imx576->dev) == 0)
> +               return 0;
> +
> +       cci_write(imx576->regmap, CCS_R_GROUPED_PARAMETER_HOLD, 1, &ret);
> +
> +       switch (ctrl->id) {
> +       case V4L2_CID_VBLANK: {
> +               u64 vmax = fmt->height + ctrl->val;
> +
> +               cci_write(imx576->regmap, CCS_R_FRAME_LENGTH_LINES, vmax, &ret);
> +               break;
> +       }
> +       case V4L2_CID_EXPOSURE:
> +               cci_write(imx576->regmap, CCS_R_COARSE_INTEGRATION_TIME, ctrl->val, &ret);

Does exposure register need an update in cases where VBLANK was modified in
a way that old value goes out of bounds?

[...]

Thanks,
    Jai