From: Dave Stevenson <dave.stevenson@raspberrypi.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
Cc: Sakari Ailus <sakari.ailus@iki.fi>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Linux Media Mailing List <linux-media@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
ebiggers@google.com, konrad.dybcio@somainline.org,
marijn.suijten@somainline.org, phone-devel@vger.kernel.org
Subject: Re: [PATCH] media: i2c: imx219: Rewrite tables and implement more modes
Date: Sun, 17 Jan 2021 20:13:01 +0000 [thread overview]
Message-ID: <CAPY8ntCPfQcCAdRNfNw2_Otww1PYqDPvW0HbRqAQMmu=gL=Xhg@mail.gmail.com> (raw)
In-Reply-To: <a641a46e-b6f8-f7a4-0413-914c19fb25c2@somainline.org>
Hi Angelo
On Sun, 17 Jan 2021 at 17:33, AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
>
> Il 17/01/21 00:13, Sakari Ailus ha scritto:
> > Hi AngeloGioacchino,
> >
> > On Fri, Jan 15, 2021 at 07:52:33PM +0100, AngeloGioacchino Del Regno wrote:
> >> Enhance the feature set for this camera sensor by in rewriting the
> >> entire tables (as they were just meaningless magic register writes)
> >> in a similar form, but giving some names to the actual registers
> >> we write to, separating common sequences and reusing them for the
> >> various configuration variations that are now supported, hence
> >> implementing support for:
> >> - 8MHz XCLK, as used by (and not only) some Sony Xperia smartphones
> >> - 4-Lane Mode in both 24MHz and 8MHz XCLK configuration
> >> - High Frame Rate output modes support on 4-Lane operation, up to
> >> 1000FPS (also on 2-Lane but, being bandwidth-constrained, the
> >> maximum achievable frame rate gets lower there)
> >
> > That's a lot of changes for a single patch. Could you split each of these
> > into separate patches, please?
> >
>
> Sure! I agree with you, let's split them for the V2 patch series!
>
> >> - Frame Bank Control Groups, in order to support a fast output
> >> resolution switch, without resetting the entire sensor during
> >> a streaming session: here the new mode gets configured on the
> >> secondary (or primary, read: "the other") bank and the sensor
> >> will be able to switch to it at the end of the "current frame".
> >
> > You basically need to stop streaming to reconfigure the sensor; V4L2
> > currently does not doing this on the fly.
> >
> > There's no need to rest the sensor though, and I don't think the driver did
> > that before either.
> >
>
> If V4L2 needs to stop streaming, then the sensor is "put to rest".
> By the way... okay, I can remove the implementation to fast-switch
> between the frame banks, but the registers are still laid out in frame
> banks in hardware so, in my opinion, the "new" layout should be kept.
>
> I wrote something more about this in reply to D. Stevenson so, please,
> for more information, look at my reply to him.
> Copy-pasting should not be necessary. Thank you!
>
> >>
> >> Please note: an unknown register write sequence was found in both
> >> the Raspberry Pi and a Sony Xperia smartphone i2c dump, but this
> >> seems to do literally nothing, as the sensor seems to work
> >> in the exact same way when sending and when not sending this
> >> write sequence, which is undocumented in the datasheet.
> >>
> >> Both the authentication and magic sequences were left in the
> >> driver with a big comment explaining what's going on so that,
> >> in the event that someone discovers the meaning of it (or
> >> Sony distributes documentation for that), it'll be pretty
> >> straightforward to insert them when needed.
> >>
> >> All the modes that got implemented in this commit have been tested
> >> with all combinations of 24/8MHz, 2/4Lane, all resolutions, on the
> >> following smartphones:
> >> - Sony Xperia XA2 (SDM630)
> >> - Sony Xperia XA2 Ultra (SDM630)
> >>
> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> >> ---
> >> drivers/media/i2c/imx219.c | 884 ++++++++++++++++++++++++-------------
> >> 1 file changed, 580 insertions(+), 304 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> >> index 92a8d52776b8..360730d5b81c 100644
> >> --- a/drivers/media/i2c/imx219.c
> >> +++ b/drivers/media/i2c/imx219.c
> >> @@ -12,6 +12,10 @@
> >> * Flip handling taken from the Sony IMX319 driver.
> >> * Copyright (C) 2018 Intel Corporation
> >> *
> >> + * 8MHz, 4-Lane, High Frame Rate modes, Frame Bank Control groups,
> >> + * fast mode switching
> >> + * Copyright (C) 2020, AngeloGioacchino Del Regno
> >> + * <angelogioacchino.delregno@somainline.org>
> >> */
> >>
> >> #include <linux/clk.h>
> >> @@ -35,24 +39,93 @@
> >> #define IMX219_MODE_STANDBY 0x00
> >> #define IMX219_MODE_STREAMING 0x01
> >>
> >> +#define IMX219_REG_SW_RESET 0x0103
> >> +
> >> +/* Output Set-up */
> >> +#define IMX219_REG_CSI_LANE_MODE 0x0114
> >> +#define IMX219_CSI_LANE_MODE_2LANE BIT(0)
> >> +#define IMX219_CSI_LANE_MODE_4LANE (BIT(0) | BIT(1))
> >> +
> >> +#define IMX219_REG_DPHY_CTRL 0x0128
> >> +#define IMX219_DPHY_CTRL_AUTO 0
> >> +#define IMX219_DPHY_CTRL_MANUAL 1
> >> +
> >> +/* Use as 16-bits reg */
> >> +#define IMX219_REG_EXCK_FREQ_MHZ 0x012A
> >> +#define IMX219_EXCK_FREQ_MHZ_MIN 6
> >> +#define IMX219_EXCK_FREQ_MHZ_MAX 27
> >> +
> >> +/* Frame Bank Control Registers*/
> >> +#define IMX219_REG_FRAME_BANK_CTRL 0x0150
> >> +#define IMX219_FRAME_BANK_EN_SHIFT BIT(0)
> >> +#define IMX219_FRAME_BANK_STAT_SHIFT BIT(1)
> >> +
> >> +#define IMX219_REG_FRAME_COUNT 0x0151
> >> +#define IMX219_REG_FRAME_FAST_TRACKING 0x0152
> >> +
> >> +/* Frame Bank 0: Group "A" - 1: Group "B" */
> >> +#define IMX219_REG_FRAME_BANK_BASE(x) ((0x100 * x) + 0x154)
> >> +#define IMX219_REG_ANALOG_GAIN 0x03
> >> +#define IMX219_REG_DIGITAL_GAIN 0x04
> >> +#define IMX219_REG_EXPOSURE 0x06
> >> +#define IMX219_REG_FRAME_LEN_LINES 0x0c
> >> +#define IMX219_REG_LINE_LEN_PCK 0x0e
> >> +#define IMX219_REG_X_ADDR_START 0x10
> >> +#define IMX219_REG_X_ADDR_END 0x12
> >> +#define IMX219_REG_Y_ADDR_START 0x14
> >> +#define IMX219_REG_Y_ADDR_END 0x16
> >> +#define IMX219_REG_X_OUTPUT_SIZE 0x18
> >> +#define IMX219_REG_Y_OUTPUT_SIZE 0x1a
> >> +#define IMX219_REG_X_ODD_INC 0x1c
> >> +#define IMX219_REG_Y_ODD_INC 0x1d
> >> +#define IMX219_REG_ORIENTATION 0x1e
> >> +#define IMX219_REG_BINNING_MODE_H 0x20
> >> +#define IMX219_REG_BINNING_MODE_V 0x21
> >> +#define IMX219_REG_BINNING_CAL_H 0x22
> >> +#define IMX219_REG_BINNING_CAL_V 0x23
> >> +#define IMX219_REG_CSI_DATA_FORMAT_HI 0x38
> >> +#define IMX219_REG_CSI_DATA_FORMAT_LO 0x39
> >> +#define IMX219_CSI_DATA_FMT_8BIT 8
> >> +#define IMX219_CSI_DATA_FMT_10BIT 10
> >> +
> >> +#define IMX219_REG_CLK_SETUP 0x300
> >> +#define IMX219_REG_VT_PIX_CLK_DIV (IMX219_REG_CLK_SETUP + 0x01)
> >> +#define IMX219_REG_VT_SYS_CLK_DIV (IMX219_REG_CLK_SETUP + 0x03)
> >> +#define IMX219_REG_PREPLLCK_VT_DIV (IMX219_REG_CLK_SETUP + 0x04)
> >> +#define IMX219_REG_PREPLLCK_OP_DIV (IMX219_REG_CLK_SETUP + 0x05)
> >> +#define IMX219_REG_PLL_VT_MULTIPLIER (IMX219_REG_CLK_SETUP + 0x06)
> >> +#define IMX219_REG_OP_PIX_CLK_DIV (IMX219_REG_CLK_SETUP + 0x09)
> >> +#define IMX219_REG_OP_SYS_CLK_DIV (IMX219_REG_CLK_SETUP + 0x0b)
> >> +#define IMX219_REG_PLL_OP_MULTIPLIER (IMX219_REG_CLK_SETUP + 0x0c)
> >> +
> >> /* Chip ID */
> >> #define IMX219_REG_CHIP_ID 0x0000
> >> #define IMX219_CHIP_ID 0x0219
> >>
> >> -/* External clock frequency is 24.0M */
> >> -#define IMX219_XCLK_FREQ 24000000
> >> +/* External clock frequency 8.0M or 24.0M */
> >> +#define IMX219_XCLK_FREQ_8M 8000000
> >> +#define IMX219_XCLK_FREQ_24M 24000000
> >> +
> >> +#define IMX219_2LANE_PIXEL_RATE 182400000
> >> +#define IMX219_4LANE_PIXEL_RATE 280800000
> >>
> >> -/* Pixel rate is fixed at 182.4M for all the modes */
> >> -#define IMX219_PIXEL_RATE 182400000
> >> +#define IMX219_2LANE_DEFAULT_LINK_FREQ 456000000
> >> +#define IMX219_4LANE_DEFAULT_LINK_FREQ 702000000
> >>
> >> -#define IMX219_DEFAULT_LINK_FREQ 456000000
> >> +/* Temperature */
> >> +#define IMX219_REG_TEMPERATURE 0x0140
> >> +#define IMX219_TEMPERATURE_EN BIT(7)
> >> +#define IMX219_TEMPERATURE_MASK 0x7f
> >>
> >> /* V_TIMING internal */
> >> #define IMX219_REG_VTS 0x0160
> >> -#define IMX219_VTS_15FPS 0x0dc6
> >> -#define IMX219_VTS_30FPS_1080P 0x06e3
> >> -#define IMX219_VTS_30FPS_BINNED 0x06e3
> >> -#define IMX219_VTS_30FPS_640x480 0x06e3
> >> +#define IMX219_VTS_8MP_30FPS_4LANE 2691
> >> +#define IMX219_VTS_8MP_15FPS_2LANE 3526
> >> +#define IMX219_VTS_60FPS_1080P 1237
> >> +#define IMX219_VTS_30FPS_BINNED 1346
> >> +#define IMX219_VTS_120FPS_720P 850
> >> +#define IMX219_VTS_200FPS_BINNED 481
> >> +#define IMX219_VTS_1000FPS_BINNED 481
> >> #define IMX219_VTS_MAX 0xffff
> >>
> >> #define IMX219_VBLANK_MIN 4
> >> @@ -67,30 +140,27 @@
> >> #define IMX219_PPL_DEFAULT 3448
> >>
> >> /* Exposure control */
> >> -#define IMX219_REG_EXPOSURE 0x015a
> >> #define IMX219_EXPOSURE_MIN 4
> >> #define IMX219_EXPOSURE_STEP 1
> >> #define IMX219_EXPOSURE_DEFAULT 0x640
> >> #define IMX219_EXPOSURE_MAX 65535
> >>
> >> /* Analog gain control */
> >> -#define IMX219_REG_ANALOG_GAIN 0x0157
> >> #define IMX219_ANA_GAIN_MIN 0
> >> #define IMX219_ANA_GAIN_MAX 232
> >> #define IMX219_ANA_GAIN_STEP 1
> >> #define IMX219_ANA_GAIN_DEFAULT 0x0
> >>
> >> /* Digital gain control */
> >> -#define IMX219_REG_DIGITAL_GAIN 0x0158
> >> #define IMX219_DGTL_GAIN_MIN 0x0100
> >> #define IMX219_DGTL_GAIN_MAX 0x0fff
> >> #define IMX219_DGTL_GAIN_DEFAULT 0x0100
> >> #define IMX219_DGTL_GAIN_STEP 1
> >>
> >> -#define IMX219_REG_ORIENTATION 0x0172
> >> -
> >> /* Test Pattern Control */
> >> #define IMX219_REG_TEST_PATTERN 0x0600
> >> +#define IMX219_REG_TEST_PATTERN_WIDTH 0x0624
> >> +#define IMX219_REG_TEST_PATTERN_HEIGHT 0x0626
> >> #define IMX219_TEST_PATTERN_DISABLE 0
> >> #define IMX219_TEST_PATTERN_SOLID_COLOR 1
> >> #define IMX219_TEST_PATTERN_COLOR_BARS 2
> >> @@ -120,7 +190,9 @@
> >>
> >> struct imx219_reg {
> >> u16 address;
> >> - u8 val;
> >> + u16 val;
> >> + u8 reg_len;
> >> + bool is_banked;
> >> };
> >>
> >> struct imx219_reg_list {
> >> @@ -134,11 +206,13 @@ struct imx219_mode {
> >> unsigned int width;
> >> /* Frame height */
> >> unsigned int height;
> >> + /* Maximum achievable FPS */
> >> + unsigned int max_fps;
> >>
> >> /* Analog crop rectangle. */
> >> struct v4l2_rect crop;
> >>
> >> - /* V-timing */
> >> + /* V-timing default */
> >> unsigned int vts_def;
> >>
> >> /* Default register values */
> >> @@ -146,248 +220,196 @@ struct imx219_mode {
> >> };
> >>
> >> /*
> >> - * Register sets lifted off the i2C interface from the Raspberry Pi firmware
> >> - * driver.
> >> - * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7.
> >> + * The authentication sequence is needed to access registers beyond 0x3000,
> >> + * which the datasheet calls "Manufacturer Specific Registers", with a range
> >> + * going from 0x3000 to 0x5fff, but its documentation is full of holes:
> >> + * it is infact documenting registers from 0x3000 to 0x32ff as OTP Data
> >> + * and, as for the others, it documents 0x4053, 0x5e54 and 0x5e59.. and
> >> + * nothing else.
> >> + *
> >> + * On both Raspberry Pi and Xperia XA2 i2c dumps, there is an unknown
> >> + * write sequence that is totally the same between the two, but the sensor
> >> + * seems to work regardless of this being sent.
> >> + * Spirit here is to not send unknown things, especially if they don't seem
> >> + * to do anything... and that's what was done. Also, remember that the auth
> >> + * commands will allow to write to the OTP area, which may actually damage
> >> + * the sensor functionality (for example, factory calibrations may be damaged).
> >> + *
> >> + * The authentication sequence and the unknown one are kept here in case one
> >> + * day they get documented somehow, or any use for them is found.
> >> */
> >> -static const struct imx219_reg mode_3280x2464_regs[] = {
> >> - {0x0100, 0x00},
> >> - {0x30eb, 0x0c},
> >> - {0x30eb, 0x05},
> >> - {0x300a, 0xff},
> >> - {0x300b, 0xff},
> >> - {0x30eb, 0x05},
> >> - {0x30eb, 0x09},
> >> - {0x0114, 0x01},
> >> - {0x0128, 0x00},
> >> - {0x012a, 0x18},
> >> - {0x012b, 0x00},
> >> - {0x0164, 0x00},
> >> - {0x0165, 0x00},
> >> - {0x0166, 0x0c},
> >> - {0x0167, 0xcf},
> >> - {0x0168, 0x00},
> >> - {0x0169, 0x00},
> >> - {0x016a, 0x09},
> >> - {0x016b, 0x9f},
> >> - {0x016c, 0x0c},
> >> - {0x016d, 0xd0},
> >> - {0x016e, 0x09},
> >> - {0x016f, 0xa0},
> >> - {0x0170, 0x01},
> >> - {0x0171, 0x01},
> >> - {0x0174, 0x00},
> >> - {0x0175, 0x00},
> >> - {0x0301, 0x05},
> >> - {0x0303, 0x01},
> >> - {0x0304, 0x03},
> >> - {0x0305, 0x03},
> >> - {0x0306, 0x00},
> >> - {0x0307, 0x39},
> >> - {0x030b, 0x01},
> >> - {0x030c, 0x00},
> >> - {0x030d, 0x72},
> >> - {0x0624, 0x0c},
> >> - {0x0625, 0xd0},
> >> - {0x0626, 0x09},
> >> - {0x0627, 0xa0},
> >> - {0x455e, 0x00},
> >> - {0x471e, 0x4b},
> >> - {0x4767, 0x0f},
> >> - {0x4750, 0x14},
> >> - {0x4540, 0x00},
> >> - {0x47b4, 0x14},
> >> - {0x4713, 0x30},
> >> - {0x478b, 0x10},
> >> - {0x478f, 0x10},
> >> - {0x4793, 0x10},
> >> - {0x4797, 0x0e},
> >> - {0x479b, 0x0e},
> >> - {0x0162, 0x0d},
> >> - {0x0163, 0x78},
> >> +static const struct imx219_reg __maybe_unused imx219_auth[] = {
> >> + { 0x30eb, 0x05, IMX219_REG_VALUE_08BIT, false },
> >> + { 0x30eb, 0x0c, IMX219_REG_VALUE_08BIT, false },
> >> + { 0x300a, 0xff, IMX219_REG_VALUE_08BIT, false },
> >> + { 0x300b, 0xff, IMX219_REG_VALUE_08BIT, false },
> >> + { 0x30eb, 0x05, IMX219_REG_VALUE_08BIT, false },
> >> + { 0x30eb, 0x09, IMX219_REG_VALUE_08BIT, false },
> >> +};
> >> +
> >> +static const struct imx219_reg __maybe_unused imx219_unknown_seq[] = {
> >
> > Why are these __maybe_unused?
> >
> >> + { 0x455E, 0x00, IMX219_REG_VALUE_08BIT, false },
> >> + { 0x471E, 0x4B, IMX219_REG_VALUE_08BIT, false },
> >> + { 0x4767, 0x0F, IMX219_REG_VALUE_08BIT, false },
> >> + { 0x4750, 0x14, IMX219_REG_VALUE_08BIT, false },
> >> + { 0x4540, 0x00, IMX219_REG_VALUE_08BIT, false },
> >> + { 0x47B4, 0x14, IMX219_REG_VALUE_08BIT, false },
> >> + { 0x4713, 0x30, IMX219_REG_VALUE_08BIT, false },
> >> + { 0x478B, 0x10, IMX219_REG_VALUE_08BIT, false },
> >> + { 0x478F, 0x10, IMX219_REG_VALUE_08BIT, false },
> >> + { 0x4793, 0x10, IMX219_REG_VALUE_08BIT, false },
> >> + { 0x4797, 0x0e, IMX219_REG_VALUE_08BIT, false },
> >> + { 0x479b, 0x0e, IMX219_REG_VALUE_08BIT, false },
> >> +};
> >> +
> >> +static const struct imx219_reg mode_24mhz_regs[] = {
> >> + { IMX219_REG_EXCK_FREQ_MHZ, (24 << 8), IMX219_REG_VALUE_16BIT, false },
> >> + { IMX219_REG_DPHY_CTRL, 0, IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_VT_PIX_CLK_DIV, 5, IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_VT_SYS_CLK_DIV, 1, IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_PREPLLCK_VT_DIV, 3, IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_PREPLLCK_OP_DIV, 3, IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_OP_PIX_CLK_DIV, 10, IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_OP_SYS_CLK_DIV, 1, IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_PLL_OP_MULTIPLIER, 84, IMX219_REG_VALUE_16BIT, false },
> >> +};
> >> +
> >> +static const struct imx219_reg mode_24mhz_2lane[] = {
> >> + { IMX219_REG_CSI_LANE_MODE, IMX219_CSI_LANE_MODE_2LANE,
> >> + IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_PLL_VT_MULTIPLIER, 57, IMX219_REG_VALUE_16BIT, false },
> >> +};
> >> +
> >> +static const struct imx219_reg mode_24mhz_4lane[] = {
> >> + { IMX219_REG_CSI_LANE_MODE, IMX219_CSI_LANE_MODE_4LANE,
> >> + IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_PLL_VT_MULTIPLIER, 82, IMX219_REG_VALUE_16BIT, false },
> >> +};
> >> +
> >> +static const struct imx219_reg mode_8mhz_regs[] = {
> >> + { IMX219_REG_EXCK_FREQ_MHZ, (8 << 8), IMX219_REG_VALUE_16BIT, false },
> >> + { IMX219_REG_DPHY_CTRL, 0, IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_VT_PIX_CLK_DIV, 5, IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_VT_SYS_CLK_DIV, 1, IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_PREPLLCK_VT_DIV, 1, IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_PREPLLCK_OP_DIV, 1, IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_OP_PIX_CLK_DIV, 10, IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_OP_SYS_CLK_DIV, 1, IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_PLL_OP_MULTIPLIER, 90, IMX219_REG_VALUE_16BIT, false },
> >> +};
> >> +
> >> +static const struct imx219_reg mode_8mhz_2lane[] = {
> >> + { IMX219_REG_CSI_LANE_MODE, IMX219_CSI_LANE_MODE_2LANE,
> >> + IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_PLL_VT_MULTIPLIER, 63, IMX219_REG_VALUE_16BIT, false },
> >> };
> >>
> >> -static const struct imx219_reg mode_1920_1080_regs[] = {
> >> - {0x0100, 0x00},
> >> - {0x30eb, 0x05},
> >> - {0x30eb, 0x0c},
> >> - {0x300a, 0xff},
> >> - {0x300b, 0xff},
> >> - {0x30eb, 0x05},
> >> - {0x30eb, 0x09},
> >> - {0x0114, 0x01},
> >> - {0x0128, 0x00},
> >> - {0x012a, 0x18},
> >> - {0x012b, 0x00},
> >> - {0x0162, 0x0d},
> >> - {0x0163, 0x78},
> >> - {0x0164, 0x02},
> >> - {0x0165, 0xa8},
> >> - {0x0166, 0x0a},
> >> - {0x0167, 0x27},
> >> - {0x0168, 0x02},
> >> - {0x0169, 0xb4},
> >> - {0x016a, 0x06},
> >> - {0x016b, 0xeb},
> >> - {0x016c, 0x07},
> >> - {0x016d, 0x80},
> >> - {0x016e, 0x04},
> >> - {0x016f, 0x38},
> >> - {0x0170, 0x01},
> >> - {0x0171, 0x01},
> >> - {0x0174, 0x00},
> >> - {0x0175, 0x00},
> >> - {0x0301, 0x05},
> >> - {0x0303, 0x01},
> >> - {0x0304, 0x03},
> >> - {0x0305, 0x03},
> >> - {0x0306, 0x00},
> >> - {0x0307, 0x39},
> >> - {0x030b, 0x01},
> >> - {0x030c, 0x00},
> >> - {0x030d, 0x72},
> >> - {0x0624, 0x07},
> >> - {0x0625, 0x80},
> >> - {0x0626, 0x04},
> >> - {0x0627, 0x38},
> >> - {0x455e, 0x00},
> >> - {0x471e, 0x4b},
> >> - {0x4767, 0x0f},
> >> - {0x4750, 0x14},
> >> - {0x4540, 0x00},
> >> - {0x47b4, 0x14},
> >> - {0x4713, 0x30},
> >> - {0x478b, 0x10},
> >> - {0x478f, 0x10},
> >> - {0x4793, 0x10},
> >> - {0x4797, 0x0e},
> >> - {0x479b, 0x0e},
> >> +static const struct imx219_reg mode_8mhz_4lane[] = {
> >> + { IMX219_REG_CSI_LANE_MODE, IMX219_CSI_LANE_MODE_4LANE,
> >> + IMX219_REG_VALUE_08BIT, false },
> >> + { IMX219_REG_PLL_VT_MULTIPLIER, 88, IMX219_REG_VALUE_16BIT, false },
> >> };
> >>
> >> -static const struct imx219_reg mode_1640_1232_regs[] = {
> >> - {0x0100, 0x00},
> >> - {0x30eb, 0x0c},
> >> - {0x30eb, 0x05},
> >> - {0x300a, 0xff},
> >> - {0x300b, 0xff},
> >> - {0x30eb, 0x05},
> >> - {0x30eb, 0x09},
> >> - {0x0114, 0x01},
> >> - {0x0128, 0x00},
> >> - {0x012a, 0x18},
> >> - {0x012b, 0x00},
> >> - {0x0164, 0x00},
> >> - {0x0165, 0x00},
> >> - {0x0166, 0x0c},
> >> - {0x0167, 0xcf},
> >> - {0x0168, 0x00},
> >> - {0x0169, 0x00},
> >> - {0x016a, 0x09},
> >> - {0x016b, 0x9f},
> >> - {0x016c, 0x06},
> >> - {0x016d, 0x68},
> >> - {0x016e, 0x04},
> >> - {0x016f, 0xd0},
> >> - {0x0170, 0x01},
> >> - {0x0171, 0x01},
> >> - {0x0174, 0x01},
> >> - {0x0175, 0x01},
> >> - {0x0301, 0x05},
> >> - {0x0303, 0x01},
> >> - {0x0304, 0x03},
> >> - {0x0305, 0x03},
> >> - {0x0306, 0x00},
> >> - {0x0307, 0x39},
> >> - {0x030b, 0x01},
> >> - {0x030c, 0x00},
> >> - {0x030d, 0x72},
> >> - {0x0624, 0x06},
> >> - {0x0625, 0x68},
> >> - {0x0626, 0x04},
> >> - {0x0627, 0xd0},
> >> - {0x455e, 0x00},
> >> - {0x471e, 0x4b},
> >> - {0x4767, 0x0f},
> >> - {0x4750, 0x14},
> >> - {0x4540, 0x00},
> >> - {0x47b4, 0x14},
> >> - {0x4713, 0x30},
> >> - {0x478b, 0x10},
> >> - {0x478f, 0x10},
> >> - {0x4793, 0x10},
> >> - {0x4797, 0x0e},
> >> - {0x479b, 0x0e},
> >> - {0x0162, 0x0d},
> >> - {0x0163, 0x78},
> >> +/* Note: Never forget to select BANK OFFSET when using these modes */
> >> +static const struct imx219_reg mode_3280x2464[] = {
> >> + /* MAX: 4-Lane @ 30FPS - 2-Lane @ 15FPS */
> >> + { IMX219_REG_FRAME_LEN_LINES, 2691, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_LINE_LEN_PCK, 3448, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_ADDR_START, 0, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_ADDR_END, 3279, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_Y_ADDR_START, 0, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_Y_ADDR_END, 2463, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_OUTPUT_SIZE, 3280, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_Y_OUTPUT_SIZE, 2464, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_ODD_INC, 1, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_Y_ODD_INC, 1, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_MODE_H, 0, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_MODE_V, 0, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_CAL_H, 0, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_CAL_V, 0, IMX219_REG_VALUE_08BIT, true },
> >> };
> >>
> >> -static const struct imx219_reg mode_640_480_regs[] = {
> >> - {0x0100, 0x00},
> >> - {0x30eb, 0x05},
> >> - {0x30eb, 0x0c},
> >> - {0x300a, 0xff},
> >> - {0x300b, 0xff},
> >> - {0x30eb, 0x05},
> >> - {0x30eb, 0x09},
> >> - {0x0114, 0x01},
> >> - {0x0128, 0x00},
> >> - {0x012a, 0x18},
> >> - {0x012b, 0x00},
> >> - {0x0162, 0x0d},
> >> - {0x0163, 0x78},
> >> - {0x0164, 0x03},
> >> - {0x0165, 0xe8},
> >> - {0x0166, 0x08},
> >> - {0x0167, 0xe7},
> >> - {0x0168, 0x02},
> >> - {0x0169, 0xf0},
> >> - {0x016a, 0x06},
> >> - {0x016b, 0xaf},
> >> - {0x016c, 0x02},
> >> - {0x016d, 0x80},
> >> - {0x016e, 0x01},
> >> - {0x016f, 0xe0},
> >> - {0x0170, 0x01},
> >> - {0x0171, 0x01},
> >> - {0x0174, 0x03},
> >> - {0x0175, 0x03},
> >> - {0x0301, 0x05},
> >> - {0x0303, 0x01},
> >> - {0x0304, 0x03},
> >> - {0x0305, 0x03},
> >> - {0x0306, 0x00},
> >> - {0x0307, 0x39},
> >> - {0x030b, 0x01},
> >> - {0x030c, 0x00},
> >> - {0x030d, 0x72},
> >> - {0x0624, 0x06},
> >> - {0x0625, 0x68},
> >> - {0x0626, 0x04},
> >> - {0x0627, 0xd0},
> >> - {0x455e, 0x00},
> >> - {0x471e, 0x4b},
> >> - {0x4767, 0x0f},
> >> - {0x4750, 0x14},
> >> - {0x4540, 0x00},
> >> - {0x47b4, 0x14},
> >> - {0x4713, 0x30},
> >> - {0x478b, 0x10},
> >> - {0x478f, 0x10},
> >> - {0x4793, 0x10},
> >> - {0x4797, 0x0e},
> >> - {0x479b, 0x0e},
> >> +static const struct imx219_reg mode_1920x1080_cropped_60fps[] = {
> >> + { IMX219_REG_FRAME_LEN_LINES, 1237, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_LINE_LEN_PCK, 3448, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_ADDR_START, 680, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_ADDR_END, 2599, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_Y_ADDR_START, 692, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_Y_ADDR_END, 1771, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_OUTPUT_SIZE, 1920, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_Y_OUTPUT_SIZE, 1080, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_ODD_INC, 1, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_Y_ODD_INC, 1, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_MODE_H, 0, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_MODE_V, 0, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_CAL_H, 0, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_CAL_V, 0, IMX219_REG_VALUE_08BIT, true },
> >> +};
> >> +
> >> +static const struct imx219_reg mode_1280x720_cropped_120fps[] = {
> >> + /* Recommended coarse integration time value: 846 */
> >> + { IMX219_REG_FRAME_LEN_LINES, 850, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_LINE_LEN_PCK, 3448, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_ADDR_START, 1000, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_ADDR_END, 2280, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_Y_ADDR_START, 872, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_Y_ADDR_END, 1592, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_OUTPUT_SIZE, 1280, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_Y_OUTPUT_SIZE, 720, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_ODD_INC, 1, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_Y_ODD_INC, 1, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_MODE_H, 0, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_MODE_V, 0, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_CAL_H, 0, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_CAL_V, 0, IMX219_REG_VALUE_08BIT, true },
> >> +};
> >> +
> >> +static const struct imx219_reg mode_640x480_x2_analog_200fps[] = {
> >> + /* Recommended coarse integration time value: 477 */
> >> + { IMX219_REG_FRAME_LEN_LINES, 481, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_LINE_LEN_PCK, 3448, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_ADDR_START, 1000, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_ADDR_END, 2280, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_Y_ADDR_START, 752, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_Y_ADDR_END, 1712, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_OUTPUT_SIZE, 640, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_Y_OUTPUT_SIZE, 480, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_ODD_INC, 1, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_Y_ODD_INC, 1, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_MODE_H, 3, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_MODE_V, 3, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_CAL_H, 0, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_CAL_V, 0, IMX219_REG_VALUE_08BIT, true },
> >> +};
> >> +
> >> +static const struct imx219_reg mode_640x80_x2_analog_1000fps[] = {
> >> + /* Recommended coarse integration time value: 92 */
> >> + { IMX219_REG_FRAME_LEN_LINES, 481, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_LINE_LEN_PCK, 3448, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_ADDR_START, 1320, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_ADDR_END, 2600, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_Y_ADDR_START, 990, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_Y_ADDR_END, 1561, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_OUTPUT_SIZE, 640, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_Y_OUTPUT_SIZE, 80, IMX219_REG_VALUE_16BIT, true },
> >> + { IMX219_REG_X_ODD_INC, 1, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_Y_ODD_INC, 1, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_MODE_H, 3, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_MODE_V, 3, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_CAL_H, 0, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_BINNING_CAL_V, 0, IMX219_REG_VALUE_08BIT, true },
> >> };
> >>
> >> static const struct imx219_reg raw8_framefmt_regs[] = {
> >> - {0x018c, 0x08},
> >> - {0x018d, 0x08},
> >> - {0x0309, 0x08},
> >> + { IMX219_REG_CSI_DATA_FORMAT_HI, 8, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_CSI_DATA_FORMAT_LO, 8, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_OP_PIX_CLK_DIV, 8, IMX219_REG_VALUE_08BIT, false },
> >> };
> >>
> >> static const struct imx219_reg raw10_framefmt_regs[] = {
> >> - {0x018c, 0x0a},
> >> - {0x018d, 0x0a},
> >> - {0x0309, 0x0a},
> >> + { IMX219_REG_CSI_DATA_FORMAT_HI, 10, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_CSI_DATA_FORMAT_LO, 10, IMX219_REG_VALUE_08BIT, true },
> >> + { IMX219_REG_OP_PIX_CLK_DIV, 10, IMX219_REG_VALUE_08BIT, false },
> >> };
> >>
> >> static const char * const imx219_test_pattern_menu[] = {
> >> @@ -461,73 +483,98 @@ static const u32 codes[] = {
> >> * case of DT for regulator-fixed one should define the startup-delay-us
> >> * property.
> >> */
> >> -#define IMX219_XCLR_MIN_DELAY_US 6200
> >> +#define IMX219_XCLR_MIN_DELAY_US 7200
> >
> > Why?
> >
>
> Because I forgot to update the comment, that's why you're asking!!!
> ...but you definitely deserve your answer before me updating the
> comment, so here's the gold:
>
> This driver was clearly made with the Raspberry Pi IMX219 camera sensor
> board in mind - and there's nothing wrong with this: the issue here is
> that this board (combined to the SoC on which it was tested) seems to
> require less time to bring up the XCLR compared to the camera module
> that is attached to the Sony Xperia XA2 and Sony Xperia XA2 Ultra
> smartphones.
>
> On these smartphones, it looks like the minimum delay (from my own
> tests) is around 6800uS, but that doesn't work in all of the conditions
> that I've tested (sometimes does, sometimes does not).
> The best *stable* delay that I've found (stable meaning that it always
> worked) was 7000uS so - accounting for eventual other factors (that I
> don't need to explain, as you surely know what I'm talking about, basic
> electronics anyway), I chose to go for sure stability, giving it 200
> more microseconds - reaching 7200uS.
>
> So.. that's why :)
If there is an additional delay required for the regulators to
stabilise on a particular board then that should be accounted for in
the regulator framework.
The last paragraph of the comment above even states this:
* This delay doesn't account for the power supply startup time. If needed,
* this should be taken care of via the regulator framework. E.g. in the
* case of DT for regulator-fixed one should define the startup-delay-us
* property.
The duration of this delay is lifted straight from the sensor
datasheet so is not open for debate (beyond potential for a very small
optimisation).
Computing the value was explicitly asked for when the original driver
was submitted [1].
We have a similar "slow" response to being enabled for our IMX477
boards. If you look at the DT overlay for that [2] we define
"startup-delay-us" for the regulator to compensate for the slowness of
the board.
Dave
[1] https://patchwork.linuxtv.org/project/linux-media/patch/20191227122114.23075-3-andrey.konovalov@linaro.org/#114407
[2] https://github.com/raspberrypi/linux/blob/rpi-5.4.y/arch/arm/boot/dts/overlays/imx477-overlay.dts#L79
next prev parent reply other threads:[~2021-01-17 20:14 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-15 18:52 [PATCH] media: i2c: imx219: Rewrite tables and implement more modes AngeloGioacchino Del Regno
2021-01-16 23:13 ` Sakari Ailus
2021-01-17 17:33 ` AngeloGioacchino Del Regno
2021-01-17 20:13 ` Dave Stevenson [this message]
2021-01-17 15:06 ` Dave Stevenson
2021-01-17 17:13 ` AngeloGioacchino Del Regno
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPY8ntCPfQcCAdRNfNw2_Otww1PYqDPvW0HbRqAQMmu=gL=Xhg@mail.gmail.com' \
--to=dave.stevenson@raspberrypi.com \
--cc=angelogioacchino.delregno@somainline.org \
--cc=ebiggers@google.com \
--cc=konrad.dybcio@somainline.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=marijn.suijten@somainline.org \
--cc=mchehab@kernel.org \
--cc=phone-devel@vger.kernel.org \
--cc=sakari.ailus@iki.fi \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.