Received-SPF: pass (google.com: domain of linux-kernel+bounces-101644-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Content-Type: text/plain; charset="utf-8"
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20240313070705.91140-3-umang.jain@ideasonboard.com>
References: <20240313070705.91140-1-umang.jain@ideasonboard.com> <20240313070705.91140-3-umang.jain@ideasonboard.com>
Subject: Re: [PATCH v3 2/2] media: i2c: Add imx283 camera sensor driver
From: Kieran Bingham <kieran.bingham@ideasonboard.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>, willl will <will@willwhang.com>, Sakari Ailus <sakari.ailus@linux.intel.com>, tomi.valkeinen@ideasonboard.com, Umang Jain <umang.jain@ideasonboard.com>, Mauro Carvalho Chehab <mchehab@kernel.org>, Hans Verkuil <hverkuil-cisco@xs4all.nl>, Hans de Goede <hdegoede@redhat.com>, Alain Volmat <alain.volmat@foss.st.com>, Paul Elder <paul.elder@ideasonboard.com>, Mehdi Djait <mehdi.djait@bootlin.com>, Bingbu Cao <bingbu.cao@intel.com>, Andy Shevchenko <andy.shevchenko@gmail.com>, linux-kernel@vger.kernel.org
To: Umang Jain <umang.jain@ideasonboard.com>, linux-media@vger.kernel.org
Date: Wed, 13 Mar 2024 14:52:34 +0000
Message-ID: <171034155409.1011926.3990407129363039340@ping.linuxembedded.co.uk>
User-Agent: alot/0.10

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>
>=20
> Add a v4l2 subdevice driver for the Sony IMX283 image sensor.
>=20
> The IMX283 is a 20MP Diagonal 15.86 mm (Type 1) CMOS Image Sensor with
> Square Pixel for Color Cameras.
>=20
> 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)
>=20
> 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
>=20
> 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
> =20
>  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.
> =20
> +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) +=3D imx214.o
>  obj-$(CONFIG_VIDEO_IMX219) +=3D imx219.o
>  obj-$(CONFIG_VIDEO_IMX258) +=3D imx258.o
>  obj-$(CONFIG_VIDEO_IMX274) +=3D imx274.o
> +obj-$(CONFIG_VIDEO_IMX283) +=3D imx283.o
>  obj-$(CONFIG_VIDEO_IMX290) +=3D imx290.o
>  obj-$(CONFIG_VIDEO_IMX296) +=3D imx296.o
>  obj-$(CONFIG_VIDEO_IMX319) +=3D 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 Co=
lor
> + * 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 as=
sumes a
> +        * 720MHz link frequency. Convert datasheet values with the follo=
wing:
> +        *
> +        * For 12 bpp modes (480Mbps) convert with:
> +        *   hmax =3D [hmax in 72MHz units] * 480 / 72
> +        *
> +        * For 10 bpp modes (576Mbps) convert with:
> +        *   hmax =3D [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[] =3D {
> +       {
> +               /* 20MPix 21.40 fps readout mode 0 */
> +               .mode =3D IMX283_MODE_0,
> +               .bpp =3D 12,
> +               .width =3D 5472,
> +               .height =3D 3648,
> +               .min_hmax =3D 5914, /* 887 @ 480MHz/72MHz */
> +               .min_vmax =3D 3793, /* Lines */
> +
> +               .veff =3D 3694,
> +               .vst =3D 0,
> +               .vct =3D 0,
> +
> +               .hbin_ratio =3D 1,
> +               .vbin_ratio =3D 1,
> +
> +               /* 20.00 FPS */
> +               .default_hmax =3D 6000, /* 900 @ 480MHz/72MHz */
> +               .default_vmax =3D 4000,
> +
> +               .min_shr =3D 11,
> +               .horizontal_ob =3D 96,
> +               .vertical_ob =3D 16,
> +               .crop =3D CENTERED_RECTANGLE(imx283_active_area, 5472, 36=
48),
> +       },
> +       {
> +               /*
> +                * Readout mode 2 : 2/2 binned mode (2736x1824)
> +                */
> +               .mode =3D IMX283_MODE_2,
> +               .bpp =3D 12,
> +               .width =3D 2736,
> +               .height =3D 1824,
> +               .min_hmax =3D 1870, /* Pixels (362 * 360/72 + padding) */

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

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

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

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

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

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

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

--
Kieran