Re: [PATCH 2/4] media: i2c: Add driver for Sony IMX728

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

Hi Sebastian,

Thank you for the patch.

I'll start with a partial review.

On Wed, Feb 12, 2025 at 02:56:54PM -0500, Sebastian LaVine wrote:
> Adds a driver for the Sony IMX728 image sensor.
> 
> Signed-off-by: Sebastian LaVine <slavine@d3embedded.com>
> Mentored-by: Stuart Burtner <sburtner@d3embedded.com>
> ---
>  MAINTAINERS                  |    1 +
>  arch/arm64/configs/defconfig |    1 +
>  drivers/media/i2c/Kconfig    |   12 +
>  drivers/media/i2c/Makefile   |    1 +
>  drivers/media/i2c/imx728.c   | 9655 ++++++++++++++++++++++++++++++++++
>  5 files changed, 9670 insertions(+)
>  create mode 100644 drivers/media/i2c/imx728.c

[snip]

> diff --git a/drivers/media/i2c/imx728.c b/drivers/media/i2c/imx728.c
> new file mode 100644
> index 000000000000..75120ca01ce6
> --- /dev/null
> +++ b/drivers/media/i2c/imx728.c
> @@ -0,0 +1,9655 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Sony IMX728 CMOS Image Sensor Driver
> + *
> + * Copyright (c) 2024-2025 Define Design Deploy Corp
> + */

[snip]

> +static const struct cci_reg_sequence imx728_wdr_12bit_3856x2176[] = {

This table is way too big, with over 8000 entries. Some are even
duplicated, with identical or different values for the same register. It
will take more than a second at 400kHz to program this.

At the very least I would expect a way to compact the table and make use
of I2C register address auto-increment. Default power-up values should
also likely be just dropped.

I haven't checked in details, but doesn't this table also contain tuning
data for your specific camera ?

[snip]

> +};

[snip]

> +static int imx728_get_frame_interval(struct v4l2_subdev *sd,
> +                                    struct v4l2_subdev_state *sd_state,
> +                                    struct v4l2_subdev_frame_interval *fi)
> +{
> +       struct imx728 *imx728 = to_imx728(sd);
> +
> +       fi->interval.numerator = 1;
> +       fi->interval.denominator = imx728->fps;
> +       return 0;
> +}
> +
> +static int imx728_set_frame_interval(struct v4l2_subdev *sd,
> +                                    struct v4l2_subdev_state *sd_state,
> +                                    struct v4l2_subdev_frame_interval *fi)
> +{
> +       struct imx728 *imx728 = to_imx728(sd);
> +       u32 req_fps;
> +
> +       mutex_lock(&imx728->lock);
> +
> +       if (fi->interval.numerator == 0 || fi->interval.denominator == 0) {
> +               fi->interval.denominator = IMX728_FRAMERATE_DEFAULT;
> +               fi->interval.numerator = 1;
> +       }
> +
> +       req_fps = clamp_val(DIV_ROUND_CLOSEST(fi->interval.denominator,
> +                                             fi->interval.numerator),
> +                           IMX728_FRAMERATE_MIN, IMX728_FRAMERATE_MAX);
> +
> +       fi->interval.numerator = 1;
> +       fi->interval.denominator = req_fps;
> +
> +       imx728->fps = req_fps;
> +
> +       mutex_unlock(&imx728->lock);
> +       dev_dbg(imx728->dev, "%s frame rate = %d\n", __func__, imx728->fps);
> +
> +       return 0;
> +}

The frame rate on raw sensors is controlled through h/v blanking. You
can drop thse functions, especially given that imx728->fps isn't used
anywhere else.

-- 
Regards,

Laurent Pinchart