Hi Alexander On Mon, 1 Jul 2024 at 10:25, Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx> wrote: > > Hi, > > Am Freitag, 28. Juni 2024, 23:17:01 CEST schrieb Spencer Hill: > > Add a driver for the Sony IMX728 image sensor. > > > > Signed-off-by: Spencer Hill <shill@xxxxxxxxxxxxxxxxx> > > --- > > drivers/media/i2c/Kconfig | 11 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/imx728.c | 4660 ++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 4672 insertions(+) > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > index c6d3ee472d81..46b6463c558a 100644 > > --- a/drivers/media/i2c/Kconfig > > +++ b/drivers/media/i2c/Kconfig > > @@ -233,6 +233,17 @@ config VIDEO_IMX415 > > To compile this driver as a module, choose M here: the > > module will be called imx415. > > > > +config VIDEO_IMX728 > > + tristate "Sony IMX728 sensor support" > > + depends on OF_GPIO > > + select V4L2_CCI_I2C > > + help > > + This is a Video4Linux2 sensor driver for the Sony > > + IMX728 camera. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called imx728. > > + > > config VIDEO_MAX9271_LIB > > tristate > > > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > > index dfbe6448b549..1188420ee1b4 100644 > > --- a/drivers/media/i2c/Makefile > > +++ b/drivers/media/i2c/Makefile > > @@ -56,6 +56,7 @@ obj-$(CONFIG_VIDEO_IMX335) += imx335.o > > obj-$(CONFIG_VIDEO_IMX355) += imx355.o > > obj-$(CONFIG_VIDEO_IMX412) += imx412.o > > obj-$(CONFIG_VIDEO_IMX415) += imx415.o > > +obj-$(CONFIG_VIDEO_IMX728) += imx728.o > > obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o > > obj-$(CONFIG_VIDEO_ISL7998X) += isl7998x.o > > obj-$(CONFIG_VIDEO_KS0127) += ks0127.o > > diff --git a/drivers/media/i2c/imx728.c b/drivers/media/i2c/imx728.c > > new file mode 100644 > > index 000000000000..190f54aaf4e9 > > --- /dev/null > > +++ b/drivers/media/i2c/imx728.c > > @@ -0,0 +1,4660 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Sony IMX728 CMOS Image Sensor Driver > > + * > > + * Copyright (c) 2024 Define Design Deploy Corp > > + */ > > + > > +#include <linux/delay.h> > > +#include <linux/clk.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/i2c.h> > > +#include <linux/types.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/v4l2-mediabus.h> > > +#include <linux/videodev2.h> > > +#include <media/v4l2-subdev.h> > > +#include <media/v4l2-ctrls.h> > > +#include <media/v4l2-event.h> > > +#include <media/v4l2-cci.h> > > + > > +#define IMX728_FRAMERATE_MAX 30 > > Bindings state the framerate is up to 45 fps, to this should be > set accordingly. Not necessarily. The binding describes the hardware, not the functionality implemented by the driver. The link frequency defined may not be sufficient to support 45fps - perfectly valid. The binding also lists that it can produce RAW24/20/16/12 and 10, but the driver only currently implements RAW10. Hopefully the others get added at a later date. The bindings for many sensors advertise a wide range of potential input clock frequencies, but most only implement a 1 or 2. (I'm slightly surprised this sensor appears to be able to configure all PLLs just by being told the frequency of the clock, but it's possible). Many sensors could support using a different number of CSI2 data lanes and reference it in their bindings, but currently don't. AIUI All of those are acceptable to be stated in the bindings but not currently implemented in the drivers. What does appear to be an issue here is that setting the frame interval appears to never be programmed into the sensor, so you have no frame rate control (imx728->fps is never read other than in imx728_get_frame_interval). There's no point in advertising the capability if it isn't connected up. If frame rate/interval control was hooked up, it'd be nice if that also set the maximum exposure time based on the frame interval. Dave > > +#define IMX728_FRAMERATE_DEFAULT 30 > > +#define IMX728_FRAMERATE_MIN 10 > > + > > +#define IMX728_PIXEL_ARRAY_WIDTH 3857 > > +#define IMX728_PIXEL_ARRAY_HEIGHT 2177 > > +#define IMX728_PIXEL_ARRAY_MARGIN_TOP 9 > > +#define IMX728_PIXEL_ARRAY_MARGIN_LEFT 8 > > +#define IMX728_PIXEL_ARRAY_RECORDING_WIDTH 3840 > > +#define IMX728_PIXEL_ARRAY_RECORDING_HEIGHT 2160 > > + > > +#define IMX728_PIXEL_RATE 248832000 > > +#define IMX728_LINK_FREQ 800000000 > > + > > +#define IMX728_EXPOSURE_DEFAULT 10000 > > + > > +#define IMX728_PM_IDLE_TIMEOUT 1000 > > + > > +#define IMX728_REG_STATE CCI_REG8(0x2CAC) > > +#define IMX728_REG_PG_00 CCI_REG16_LE(0x1A2A) > > +#define IMX728_REG_PG_01 CCI_REG24_LE(0x1A30) > > +#define IMX728_REG_PG_02 CCI_REG24_LE(0x1A38) > > +#define IMX728_REG_PG_03 CCI_REG8(0xB58F) > > +#define IMX728_REG_PG_04 CCI_REG8(0xB6C5) > > +#define IMX728_REG_PG_05 CCI_REG16_LE(0x1A2C) > > +#define IMX728_REG_PG_06 CCI_REG8(0xB58E) > > +#define IMX728_REG_PG_07 CCI_REG8(0xB6C4) > > +#define IMX728_REG_EXPOSURE_00 CCI_REG32_LE(0x98DC) > > +#define IMX728_REG_EXPOSURE_01 CCI_REG32_LE(0x98E4) > > +#define IMX728_REG_EXPOSURE_02 CCI_REG32_LE(0x98EC) > > +#define IMX728_REG_AGAIN_00 CCI_REG32_LE(0x98F8) > > +#define IMX728_REG_AGAIN_01 CCI_REG32_LE(0x98FC) > > +#define IMX728_REG_AGAIN_02 CCI_REG32_LE(0x9900) > > +#define IMX728_REG_AGAIN_03 CCI_REG32_LE(0x9904) > > +#define IMX728_REG_AGAIN_04 CCI_REG32_LE(0x9908) > > +#define IMX728_REG_FLIP CCI_REG8(0x9651) > > +#define IMX728_REG_HFLIP CCI_REG8(0xB67C) > > +#define IMX728_REG_VFLIP CCI_REG8(0xB67D) > > +#define IMX728_REG_VMINOR CCI_REG8(0x6000) > > +#define IMX728_REG_VMAJOR CCI_REG8(0x6002) > > +#define IMX728_REG_RESET_0 CCI_REG8(0xB661) > > +#define IMX728_REG_RESET_1 CCI_REG8(0x95C5) > > +#define IMX728_REG_INCK_0 CCI_REG8(0x1B20) > > +#define IMX728_REG_INCK_1 CCI_REG8(0x1B1C) > > +#define IMX728_REG_SLEEP CCI_REG8(0x1B05) > > +#define IMX728_REG_REGMAP CCI_REG8(0xFFFF) > > +#define IMX728_REG_HDR_00 CCI_REG32_LE(0x9C60) > > +#define IMX728_REG_HDR_01 CCI_REG32_LE(0x9C6C) > > +#define IMX728_REG_HDR_02 CCI_REG32_LE(0x9C64) > > +#define IMX728_REG_HDR_03 CCI_REG32_LE(0x9C70) > > +#define IMX728_REG_HDR_04 CCI_REG16_LE(0x9C68) > > +#define IMX728_REG_HDR_05 CCI_REG16_LE(0x9C74) > > +#define IMX728_REG_HDR_06 CCI_REG16_LE(0x9C6A) > > +#define IMX728_REG_HDR_07 CCI_REG16_LE(0x9C76) > > +#define IMX728_REG_AE_MODE CCI_REG8(0x98AC) > > +#define IMX728_REG_AWBMODE CCI_REG8(0xA248) > > +#define IMX728_REG_AWB_EN CCI_REG8(0x1808) > > +#define IMX728_REG_UNIT_00 CCI_REG8(0x98E0) > > +#define IMX728_REG_UNIT_01 CCI_REG8(0x98E8) > > +#define IMX728_REG_UNIT_02 CCI_REG8(0x98F0) > > +#define IMX728_REG_MD_00 CCI_REG8(0x1708) > > +#define IMX728_REG_MD_01 CCI_REG8(0x1709) > > +#define IMX728_REG_MD_02 CCI_REG8(0x170A) > > +#define IMX728_REG_MD_03 CCI_REG8(0x1B40) > > +#define IMX728_REG_MODE_SEL CCI_REG16_LE(0x9728) > > +#define IMX728_REG_OUT_MODE CCI_REG8(0xEC7E) > > +#define IMX728_REG_OB_0 CCI_REG16_LE(0xEC12) > > +#define IMX728_REG_OB_1 CCI_REG8(0xEC14) > > +#define IMX728_REG_SKEW CCI_REG8(0x1761) > > +#define IMX728_REG_SUBP_0 CCI_REG8(0x9714) > > +#define IMX728_REG_SUBP_1 CCI_REG8(0xB684) > > +#define IMX728_REG_STREAM_00 CCI_REG8(0x9789) > > +#define IMX728_REG_STREAM_01 CCI_REG8(0x95C1) > > +#define IMX728_REG_STREAM_02 CCI_REG8(0x1B04) > > Can you sort them by register address? > > > +#define IMX728_REG_CTRL_POINT_X(i) CCI_REG32(0xA198 + (i) * 8) > > +#define IMX728_REG_CTRL_POINT_Y(i) (IMX728_REG_CTRL_POINT_X(i) + 4) > > >[snip] > > Best regards, > Alexander > > > -- > > 2.43.0 > > > > Please be aware that this email includes email addresses outside of the organization. > > > > > -- > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany > Amtsgericht München, HRB 105018 > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider > http://www.tq-group.com/ > >
