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

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

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@ideasonboard.com>
> 
> 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@ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  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@vger.kernel.org
>  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@linaro.org>
> 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