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@xxxxxxxxxxxxxx>
> Mentored-by: Stuart Burtner <sburtner@xxxxxxxxxxxxxx>
> ---
> 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
