linux-kernel - Re: [PATCH v2 2/2] media: i2c: Add driver for Sony IMX728

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 1 Jul 2024 15:00:16 +0100
From: Dave Stevenson <dave.stevenson@...pberrypi.com>
To: Alexander Stein <alexander.stein@...tq-group.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Spencer Hill <shill@...ngineering.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Shawn Guo <shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>, 
	Pengutronix Kernel Team <kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>, linux-kernel@...r.kernel.org, 
	linux-media@...r.kernel.org, devicetree@...r.kernel.org, imx@...ts.linux.dev, 
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 2/2] media: i2c: Add driver for Sony IMX728

Hi Alexander

On Mon, 1 Jul 2024 at 10:25, Alexander Stein
<alexander.stein@...tq-group.com> 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@...ngineering.com>
> > ---
> >  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/
>
>