Hi Dave, On Tue, Aug 22, 2023 at 05:04:39PM +0100, Dave Stevenson wrote: > On Mon, 21 Aug 2023 at 23:29, Laurent Pinchart wrote: > > > > Define macros for all the known registers used in the register arrays, > > and use them to replace the numerical addresses. This improves > > readability. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > --- > > drivers/media/i2c/imx219.c | 169 ++++++++++++++++++------------------- > > 1 file changed, 81 insertions(+), 88 deletions(-) > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > index 229dfe24304b..8cab0251bd6a 100644 > > --- a/drivers/media/i2c/imx219.c > > +++ b/drivers/media/i2c/imx219.c > > @@ -41,6 +41,13 @@ > > #define IMX219_CSI_2_LANE_MODE 0x01 > > #define IMX219_CSI_4_LANE_MODE 0x03 > > > > +#define IMX219_REG_DPHY_CTRL CCI_REG8(0x0128) > > +#define IMX219_DPHY_CTRL_TIMING_AUTO 0 > > +#define IMX219_DPHY_CTRL_TIMING_MANUAL 1 > > + > > +#define IMX219_REG_EXCK_FREQ CCI_REG16(0x012a) > > +#define IMX219_EXCK_FREQ(n) ((n) * 256) /* n expressed in MHz */ > > + > > /* Analog gain control */ > > #define IMX219_REG_ANALOG_GAIN CCI_REG8(0x0157) > > #define IMX219_ANA_GAIN_MIN 0 > > @@ -75,6 +82,15 @@ > > /* HBLANK control - read only */ > > #define IMX219_PPL_DEFAULT 3448 > > > > +#define IMX219_REG_LINE_LENGTH_A CCI_REG16(0x0162) > > +#define IMX219_REG_X_ADD_STA_A CCI_REG16(0x0164) > > +#define IMX219_REG_X_ADD_END_A CCI_REG16(0x0166) > > +#define IMX219_REG_Y_ADD_STA_A CCI_REG16(0x0168) > > +#define IMX219_REG_Y_ADD_END_A CCI_REG16(0x016a) > > +#define IMX219_REG_X_OUTPUT_SIZE CCI_REG16(0x016c) > > +#define IMX219_REG_Y_OUTPUT_SIZE CCI_REG16(0x016e) > > +#define IMX219_REG_X_ODD_INC_A CCI_REG8(0x0170) > > +#define IMX219_REG_Y_ODD_INC_A CCI_REG8(0x0171) > > #define IMX219_REG_ORIENTATION CCI_REG8(0x0172) > > > > /* Binning Mode */ > > @@ -83,6 +99,18 @@ > > #define IMX219_BINNING_2X2 0x0101 > > #define IMX219_BINNING_2X2_ANALOG 0x0303 > > > > +#define IMX219_REG_CSI_DATA_FORMAT_A CCI_REG16(0x018c) > > + > > +/* PLL Settings */ > > +#define IMX219_REG_VTPXCK_DIV CCI_REG8(0x0301) > > +#define IMX219_REG_VTSYCK_DIV CCI_REG8(0x0303) > > +#define IMX219_REG_PREPLLCK_VT_DIV CCI_REG8(0x0304) > > +#define IMX219_REG_PREPLLCK_OP_DIV CCI_REG8(0x0305) > > +#define IMX219_REG_PLL_VT_MPY CCI_REG16(0x0306) > > +#define IMX219_REG_OPPXCK_DIV CCI_REG8(0x0309) > > +#define IMX219_REG_OPSYCK_DIV CCI_REG8(0x030b) > > +#define IMX219_REG_PLL_OP_MPY CCI_REG16(0x030c) > > + > > /* Test Pattern Control */ > > #define IMX219_REG_TEST_PATTERN CCI_REG16(0x0600) > > #define IMX219_TEST_PATTERN_DISABLE 0 > > @@ -100,6 +128,9 @@ > > #define IMX219_TESTP_COLOUR_MAX 0x03ff > > #define IMX219_TESTP_COLOUR_STEP 1 > > > > +#define IMX219_REG_TP_WINDOW_WIDTH CCI_REG16(0x0624) > > +#define IMX219_REG_TP_WINDOW_HEIGHT CCI_REG16(0x0626) > > + > > /* External clock frequency is 24.0M */ > > #define IMX219_XCLK_FREQ 24000000 > > > > @@ -144,7 +175,7 @@ struct imx219_mode { > > }; > > > > static const struct cci_reg_sequence imx219_common_regs[] = { > > - { CCI_REG8(0x0100), 0x00 }, /* Mode Select */ > > + { IMX219_REG_MODE_SELECT, 0x00 }, /* Mode Select */ > > > > /* To Access Addresses 3000-5fff, send the following commands */ > > { CCI_REG8(0x30eb), 0x0c }, > > @@ -155,15 +186,13 @@ static const struct cci_reg_sequence imx219_common_regs[] = { > > { CCI_REG8(0x30eb), 0x09 }, > > > > /* PLL Clock Table */ > > - { CCI_REG8(0x0301), 0x05 }, /* VTPXCK_DIV */ > > - { CCI_REG8(0x0303), 0x01 }, /* VTSYSCK_DIV */ > > - { CCI_REG8(0x0304), 0x03 }, /* PREPLLCK_VT_DIV 0x03 = AUTO set */ > > - { CCI_REG8(0x0305), 0x03 }, /* PREPLLCK_OP_DIV 0x03 = AUTO set */ > > - { CCI_REG8(0x0306), 0x00 }, /* PLL_VT_MPY */ > > - { CCI_REG8(0x0307), 0x39 }, > > - { CCI_REG8(0x030b), 0x01 }, /* OP_SYS_CLK_DIV */ > > - { CCI_REG8(0x030c), 0x00 }, /* PLL_OP_MPY */ > > - { CCI_REG8(0x030d), 0x72 }, > > + { IMX219_REG_VTPXCK_DIV, 5 }, > > + { IMX219_REG_VTSYCK_DIV, 1 }, > > + { IMX219_REG_PREPLLCK_VT_DIV, 3 }, /* 0x03 = AUTO set */ > > + { IMX219_REG_PREPLLCK_OP_DIV, 3 }, /* 0x03 = AUTO set */ > > + { IMX219_REG_PLL_VT_MPY, 57 }, > > + { IMX219_REG_OPSYCK_DIV, 1 }, > > + { IMX219_REG_PLL_OP_MPY, 114 }, > > > > /* Undocumented registers */ > > { CCI_REG8(0x455e), 0x00 }, > > @@ -180,16 +209,14 @@ static const struct cci_reg_sequence imx219_common_regs[] = { > > { CCI_REG8(0x479b), 0x0e }, > > > > /* Frame Bank Register Group "A" */ > > - { CCI_REG8(0x0162), 0x0d }, /* Line_Length_A */ > > - { CCI_REG8(0x0163), 0x78 }, > > - { CCI_REG8(0x0170), 0x01 }, /* X_ODD_INC_A */ > > - { CCI_REG8(0x0171), 0x01 }, /* Y_ODD_INC_A */ > > + { IMX219_REG_LINE_LENGTH_A, 3448 }, > > + { IMX219_REG_X_ODD_INC_A, 1 }, > > + { IMX219_REG_Y_ODD_INC_A, 1 }, > > > > /* Output setup registers */ > > - { CCI_REG8(0x0114), 0x01 }, /* CSI 2-Lane Mode */ > > - { CCI_REG8(0x0128), 0x00 }, /* DPHY Auto Mode */ > > - { CCI_REG8(0x012a), 0x18 }, /* EXCK_Freq */ > > - { CCI_REG8(0x012b), 0x00 }, > > + { IMX219_REG_CSI_LANE_MODE, IMX219_CSI_2_LANE_MODE }, > > This line is actually redundant as the setting is made from > imx219_configure_lanes. > That would be fixes ceddfd4493b3 ("media: i2c: imx219: Support > four-lane operation") if you wanted to pick it up as an extra patch. I'll do that. > > + { IMX219_REG_DPHY_CTRL, IMX219_DPHY_CTRL_TIMING_AUTO }, > > + { IMX219_REG_EXCK_FREQ, IMX219_EXCK_FREQ(24) }, > > IMX219_XCLK_FREQ / 1000000? > If someone comes along later to add support for other XCLK values > hopefully they'd work out that this register needs to be updated, but > you could spell it out seeing as you've added the macro to produce the > register value. It's a good idea. With more free time, I would try to make the PLL calculations dynamic, but that's not anywhere close to the top of my todo list. > I'm not that bothered over it though. > > Reviewed-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> > > > }; > > > > /* > > @@ -198,91 +225,57 @@ static const struct cci_reg_sequence imx219_common_regs[] = { > > * 3280x2464 = mode 2, 1920x1080 = mode 1, 1640x1232 = mode 4, 640x480 = mode 7. > > */ > > static const struct cci_reg_sequence mode_3280x2464_regs[] = { > > - { CCI_REG8(0x0164), 0x00 }, > > - { CCI_REG8(0x0165), 0x00 }, > > - { CCI_REG8(0x0166), 0x0c }, > > - { CCI_REG8(0x0167), 0xcf }, > > - { CCI_REG8(0x0168), 0x00 }, > > - { CCI_REG8(0x0169), 0x00 }, > > - { CCI_REG8(0x016a), 0x09 }, > > - { CCI_REG8(0x016b), 0x9f }, > > - { CCI_REG8(0x016c), 0x0c }, > > - { CCI_REG8(0x016d), 0xd0 }, > > - { CCI_REG8(0x016e), 0x09 }, > > - { CCI_REG8(0x016f), 0xa0 }, > > - { CCI_REG8(0x0624), 0x0c }, > > - { CCI_REG8(0x0625), 0xd0 }, > > - { CCI_REG8(0x0626), 0x09 }, > > - { CCI_REG8(0x0627), 0xa0 }, > > + { IMX219_REG_X_ADD_STA_A, 0 }, > > + { IMX219_REG_X_ADD_END_A, 3279 }, > > + { IMX219_REG_Y_ADD_STA_A, 0 }, > > + { IMX219_REG_Y_ADD_END_A, 2463 }, > > + { IMX219_REG_X_OUTPUT_SIZE, 3280 }, > > + { IMX219_REG_Y_OUTPUT_SIZE, 2464 }, > > + { IMX219_REG_TP_WINDOW_WIDTH, 3280 }, > > + { IMX219_REG_TP_WINDOW_HEIGHT, 2464 }, > > }; > > > > static const struct cci_reg_sequence mode_1920_1080_regs[] = { > > - { CCI_REG8(0x0164), 0x02 }, > > - { CCI_REG8(0x0165), 0xa8 }, > > - { CCI_REG8(0x0166), 0x0a }, > > - { CCI_REG8(0x0167), 0x27 }, > > - { CCI_REG8(0x0168), 0x02 }, > > - { CCI_REG8(0x0169), 0xb4 }, > > - { CCI_REG8(0x016a), 0x06 }, > > - { CCI_REG8(0x016b), 0xeb }, > > - { CCI_REG8(0x016c), 0x07 }, > > - { CCI_REG8(0x016d), 0x80 }, > > - { CCI_REG8(0x016e), 0x04 }, > > - { CCI_REG8(0x016f), 0x38 }, > > - { CCI_REG8(0x0624), 0x07 }, > > - { CCI_REG8(0x0625), 0x80 }, > > - { CCI_REG8(0x0626), 0x04 }, > > - { CCI_REG8(0x0627), 0x38 }, > > + { IMX219_REG_X_ADD_STA_A, 680 }, > > + { IMX219_REG_X_ADD_END_A, 2599 }, > > + { IMX219_REG_Y_ADD_STA_A, 692 }, > > + { IMX219_REG_Y_ADD_END_A, 1771 }, > > + { IMX219_REG_X_OUTPUT_SIZE, 1920 }, > > + { IMX219_REG_Y_OUTPUT_SIZE, 1080 }, > > + { IMX219_REG_TP_WINDOW_WIDTH, 1920 }, > > + { IMX219_REG_TP_WINDOW_HEIGHT, 1080 }, > > }; > > > > static const struct cci_reg_sequence mode_1640_1232_regs[] = { > > - { CCI_REG8(0x0164), 0x00 }, > > - { CCI_REG8(0x0165), 0x00 }, > > - { CCI_REG8(0x0166), 0x0c }, > > - { CCI_REG8(0x0167), 0xcf }, > > - { CCI_REG8(0x0168), 0x00 }, > > - { CCI_REG8(0x0169), 0x00 }, > > - { CCI_REG8(0x016a), 0x09 }, > > - { CCI_REG8(0x016b), 0x9f }, > > - { CCI_REG8(0x016c), 0x06 }, > > - { CCI_REG8(0x016d), 0x68 }, > > - { CCI_REG8(0x016e), 0x04 }, > > - { CCI_REG8(0x016f), 0xd0 }, > > - { CCI_REG8(0x0624), 0x06 }, > > - { CCI_REG8(0x0625), 0x68 }, > > - { CCI_REG8(0x0626), 0x04 }, > > - { CCI_REG8(0x0627), 0xd0 }, > > + { IMX219_REG_X_ADD_STA_A, 0 }, > > + { IMX219_REG_X_ADD_END_A, 3279 }, > > + { IMX219_REG_Y_ADD_STA_A, 0 }, > > + { IMX219_REG_Y_ADD_END_A, 2463 }, > > + { IMX219_REG_X_OUTPUT_SIZE, 1640 }, > > + { IMX219_REG_Y_OUTPUT_SIZE, 1232 }, > > + { IMX219_REG_TP_WINDOW_WIDTH, 1640 }, > > + { IMX219_REG_TP_WINDOW_HEIGHT, 1232 }, > > }; > > > > static const struct cci_reg_sequence mode_640_480_regs[] = { > > - { CCI_REG8(0x0164), 0x03 }, > > - { CCI_REG8(0x0165), 0xe8 }, > > - { CCI_REG8(0x0166), 0x08 }, > > - { CCI_REG8(0x0167), 0xe7 }, > > - { CCI_REG8(0x0168), 0x02 }, > > - { CCI_REG8(0x0169), 0xf0 }, > > - { CCI_REG8(0x016a), 0x06 }, > > - { CCI_REG8(0x016b), 0xaf }, > > - { CCI_REG8(0x016c), 0x02 }, > > - { CCI_REG8(0x016d), 0x80 }, > > - { CCI_REG8(0x016e), 0x01 }, > > - { CCI_REG8(0x016f), 0xe0 }, > > - { CCI_REG8(0x0624), 0x06 }, > > - { CCI_REG8(0x0625), 0x68 }, > > - { CCI_REG8(0x0626), 0x04 }, > > - { CCI_REG8(0x0627), 0xd0 }, > > + { IMX219_REG_X_ADD_STA_A, 1000 }, > > + { IMX219_REG_X_ADD_END_A, 2279 }, > > + { IMX219_REG_Y_ADD_STA_A, 752 }, > > + { IMX219_REG_Y_ADD_END_A, 1711 }, > > + { IMX219_REG_X_OUTPUT_SIZE, 640 }, > > + { IMX219_REG_Y_OUTPUT_SIZE, 480 }, > > + { IMX219_REG_TP_WINDOW_WIDTH, 1640 }, > > + { IMX219_REG_TP_WINDOW_HEIGHT, 1232 }, > > }; > > > > static const struct cci_reg_sequence raw8_framefmt_regs[] = { > > - { CCI_REG8(0x018c), 0x08 }, > > - { CCI_REG8(0x018d), 0x08 }, > > - { CCI_REG8(0x0309), 0x08 }, > > + { IMX219_REG_CSI_DATA_FORMAT_A, 0x0808 }, > > + { IMX219_REG_OPPXCK_DIV, 8 }, > > }; > > > > static const struct cci_reg_sequence raw10_framefmt_regs[] = { > > - { CCI_REG8(0x018c), 0x0a }, > > - { CCI_REG8(0x018d), 0x0a }, > > - { CCI_REG8(0x0309), 0x0a }, > > + { IMX219_REG_CSI_DATA_FORMAT_A, 0x0a0a }, > > + { IMX219_REG_OPPXCK_DIV, 10 }, > > }; > > > > static const s64 imx219_link_freq_menu[] = { -- Regards, Laurent Pinchart