Re: [PATCH v3 2/2] media: i2c: Add imx283 camera sensor driver

From: Kieran Bingham
Date: Wed Mar 13 2024 - 10:52:50 EST

Hi Umang,

Some corrections to make on the frame/mode timings below that we seem to
have missed before.

Quoting Umang Jain (2024-03-13 07:06:59)
> From: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
>
> Add a v4l2 subdevice driver for the Sony IMX283 image sensor.
>
> The IMX283 is a 20MP Diagonal 15.86 mm (Type 1) CMOS Image Sensor with
> Square Pixel for Color Cameras.
>
> The following features are supported:
> - Manual exposure an gain control support
> - vblank/hblank/link freq control support
> - Test pattern support control
> - Arbitrary horizontal and vertical cropping
> - Supported resolution:
> - 5472x3648 @ 20fps (SRGGB12)
> - 5472x3648 @ 25fps (SRGGB10)
> - 2736x1824 @ 50fps (SRGGB12)
>
> Signed-off-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> Signed-off-by: Umang Jain <umang.jain@xxxxxxxxxxxxxxxx>
> ---
> MAINTAINERS | 1 +
> drivers/media/i2c/Kconfig | 10 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/imx283.c | 1596 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 1608 insertions(+)
> create mode 100644 drivers/media/i2c/imx283.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 32f790c3a5f9..8169f0e41293 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20375,6 +20375,7 @@ L: linux-media@xxxxxxxxxxxxxxx
> S: Maintained
> T: git git://linuxtv.org/media_tree.git
> F: Documentation/devicetree/bindings/media/i2c/sony,imx283.yaml
> +F: drivers/media/i2c/imx283.c
>
> SONY IMX290 SENSOR DRIVER
> M: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 4c3435921f19..2090b06b1827 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -153,6 +153,16 @@ config VIDEO_IMX274
> This is a V4L2 sensor driver for the Sony IMX274
> CMOS image sensor.
>
> +config VIDEO_IMX283
> + tristate "Sony IMX283 sensor support"
> + select V4L2_CCI_I2C
> + help
> + This is a V4L2 sensor driver for the Sony IMX283
> + CMOS image sensor.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called imx283.
> +
> config VIDEO_IMX290
> tristate "Sony IMX290 sensor support"
> select REGMAP_I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index dfbe6448b549..0fbd81f9f420 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_VIDEO_IMX214) += imx214.o
> obj-$(CONFIG_VIDEO_IMX219) += imx219.o
> obj-$(CONFIG_VIDEO_IMX258) += imx258.o
> obj-$(CONFIG_VIDEO_IMX274) += imx274.o
> +obj-$(CONFIG_VIDEO_IMX283) += imx283.o
> obj-$(CONFIG_VIDEO_IMX290) += imx290.o
> obj-$(CONFIG_VIDEO_IMX296) += imx296.o
> obj-$(CONFIG_VIDEO_IMX319) += imx319.o
> diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
> new file mode 100644
> index 000000000000..81fe2d4fd4d3
> --- /dev/null
> +++ b/drivers/media/i2c/imx283.c
> @@ -0,0 +1,1596 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * V4L2 Support for the IMX283
> + *
> + * Diagonal 15.86 mm (Type 1) CMOS Image Sensor with Square Pixel for Color
> + * Cameras.
> + *
> + * Copyright (C) 2024 Ideas on Board Oy.
> + *
> + * Based on Sony IMX283 driver prepared by Will Whang
> + *
> + * Based on Sony imx477 camera driver
> + * Copyright (C) 2019-2020 Raspberry Pi (Trading) Ltd
> + */
> +
..
> +/* Mode : resolution and related config values */
> +struct imx283_mode {
> + unsigned int mode;
> +
> + /* Bits per pixel */
> + unsigned int bpp;
> +
> + /* Frame width */
> + unsigned int width;
> +
> + /* Frame height */
> + unsigned int height;
> +
> + /*
> + * Minimum horizontal timing in pixel-units
> + *
> + * Note that HMAX is written in 72MHz units, and the datasheet assumes a
> + * 720MHz link frequency. Convert datasheet values with the following:
> + *
> + * For 12 bpp modes (480Mbps) convert with:
> + * hmax = [hmax in 72MHz units] * 480 / 72
> + *
> + * For 10 bpp modes (576Mbps) convert with:
> + * hmax = [hmax in 72MHz units] * 576 / 72
> + */
> + u32 min_hmax;
> +
> + /* minimum V-timing in lines */
> + u32 min_vmax;
> +
> + /* default H-timing */
> + u32 default_hmax;
> +
> + /* default V-timing */
> + u32 default_vmax;
> +
> + /* minimum SHR */
> + u32 min_shr;
> +
> + /*
> + * Per-mode vertical crop constants used to calculate values
> + * of IMX283REG_WIDCUT and IMX283_REG_VWINPOS.
> + */
> + u32 veff;
> + u32 vst;
> + u32 vct;
> +
> + /* Horizontal and vertical binning ratio */
> + u8 hbin_ratio;
> + u8 vbin_ratio;
> +
> + /* Optical Blanking */
> + u32 horizontal_ob;
> + u32 vertical_ob;
> +
> + /* Analog crop rectangle. */
> + struct v4l2_rect crop;
> +};
..
> +/* Mode configs */
> +static const struct imx283_mode supported_modes_12bit[] = {
> + {
> + /* 20MPix 21.40 fps readout mode 0 */
> + .mode = IMX283_MODE_0,
> + .bpp = 12,
> + .width = 5472,
> + .height = 3648,
> + .min_hmax = 5914, /* 887 @ 480MHz/72MHz */
> + .min_vmax = 3793, /* Lines */
> +
> + .veff = 3694,
> + .vst = 0,
> + .vct = 0,
> +
> + .hbin_ratio = 1,
> + .vbin_ratio = 1,
> +
> + /* 20.00 FPS */
> + .default_hmax = 6000, /* 900 @ 480MHz/72MHz */
> + .default_vmax = 4000,
> +
> + .min_shr = 11,
> + .horizontal_ob = 96,
> + .vertical_ob = 16,
> + .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
> + },
> + {
> + /*
> + * Readout mode 2 : 2/2 binned mode (2736x1824)
> + */
> + .mode = IMX283_MODE_2,
> + .bpp = 12,
> + .width = 2736,
> + .height = 1824,
> + .min_hmax = 1870, /* Pixels (362 * 360/72 + padding) */

I believe this should be
.min_hmax = 2414, /* Pixels (362 * 480/72 + padding) */

> + .min_vmax = 3840, /* Lines */
> +
> + /* 50.00 FPS */
> + .default_hmax = 1870, /* 362 @ 360MHz/72MHz */
> + .default_vmax = 3960,

And for 50.00 FPS, these should be something like
.default_hmax = 2500, /* 375 @ 480/72 */
.default_vmax = 3840,

> +
> + .veff = 1824,
> + .vst = 0,
> + .vct = 0,
> +
> + .hbin_ratio = 2,
> + .vbin_ratio = 2,
> +
> + .min_shr = 12,
> + .horizontal_ob = 48,
> + .vertical_ob = 4,
> +
> + .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
> + },
> +};
> +
> +static const struct imx283_mode supported_modes_10bit[] = {
> + {
> + /* 20MPix 25.48 fps readout mode 1 */
> + .mode = IMX283_MODE_1,
> + .bpp = 10,
> + .width = 5472,
> + .height = 3648,
> + .min_hmax = 5960, /* 745 @ 576MHz / 72MHz */
> + .min_vmax = 3793,
> +
> + /* 25.00 FPS */
> + .default_hmax = 1500, /* 750 @ 576MHz / 72MHz */

This line has gone wrong somewhere. The default can't be lower than the
min. I suspect it should be:

= 6000, /* 750 @ 576MHz / 72MHz */

> + .default_vmax = 3840,
> +
> + .min_shr = 10,
> + .horizontal_ob = 96,
> + .vertical_ob = 16,
> + .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
> + },
> +};

--
Kieran