mbox series

[v4,00/25] imx258 improvement series

Message ID 20240414203503.18402-1-git@luigi311.com
Headers show
Series imx258 improvement series | expand

Message

Luis Garcia April 14, 2024, 8:34 p.m. UTC 
From: Luis Garcia <git@luigi311.com>

v4:
  - Swapped out the use macro patch for a patch that uses the new
    CCI register access helpers per Sakari Ailus
  - Fix order of reset and powerdown gpio before disable regulators
  - Fix formating of all patches
  
  - Implemented feedback from Pavel Machek
    - media: i2c: imx258: Follow normal V4L2 behaviours
    - media: i2c: imx258: Allow configuration of clock
  - Implemented feedback from Sakari Ailus
    - media: i2c: imx258: Add support for powerdown gpio


v3 Luis Garcia

- Add Use v4l2_link_freq_to_bitmap helper patch per Sakari Ailus
- Separate dt-bindings for powerdown per Rob Herring
- Fix dt-bindings for imx258.yaml
- Fix sign offs per Dang Huynh 
- Fix versioning per Conor Dooley and Kieran Bingham
- Use scripts to validate and fix patches
- Implemented feedback from Sakari Ailus
  - media: i2c: imx258: Add support for 24MHz clock
  - media: i2c: imx258: Add support for running on 2 CSI data lanes

- Implemented feedback from Rob Herring
  - dt-bindings: media: imx258: Add alternate compatible strings



v2 Luis Garcia

- Add use macros patch 
- Add support for powerdown gpio patch
- Add support for reset gpio patch
- Dropped Add support for long exposure modes patch
- Implemented feedback from Jacopo Mondi
  - media: i2c: imx258: Add regulator control
  - media: i2c: imx258: Add support for 24MHz clock
  - media: i2c: imx258: Add support for running on 2 CSI data lanes
  - media: i2c: imx258: Add get_selection for pixel array information
  - media: i2c: imx258: Issue reset before starting streaming
  - media: i2c: imx258: Set pixel_rate range to the same as the value
  - dt-bindings: media: imx258: Add alternate compatible strings
  - media: i2c: imx258: Change register settings for variants of the sensor
  - media: i2c: imx258: Make HFLIP and VFLIP controls writable

This adds a few more patches and drops one. The long exposure mode patch
was dropped due to the bug that Jacopo found. The powerdown and reset gpio
patches were added as that fixes support for the Pinephone Pro, without
them the sensor doesn't initialize correctly.

Tested on a Pinephone Pro by forcing 24 mhz clock and was able to access 
all 3 resolutions. The two lower resolutions had some artifacts but that
is expected as more changes are required to fix them for the
Pinephone Pro specifically, kept all registers the same as Dave's original
patch since that works on dedicated imx258 hardware and the artifacts are
PPP specific so it shouldn't be a regression.



v1 Dave Stevenson

This is a set of patches for imx258 that allow it to work with alternate
clock frequencies, over either 2 or 4 lanes, and generally adding
flexibility to the driver.

Tested with an IMX258 module from Soho Enterprises that has a 24MHz
oscillator. Both 2 and 4 lane configurations work with correct link
frequencies and pixel rates.

Jacopo has tested on a PinePhone Pro which has an ~19.2MHz clock fed from
the SoC, He confirms that the two lower resolution modes work, but not the
full res mode. Comparing to the BSP it looks like they have some weird
clock configuration in the 4208x3120 mode (nominally 1224Mb/s/lane instead
of 1267). As it has never previously worked directly with the mainline
driver this isn't a regression but may indicate that there is a need for
support of additional link frequencies in the future.

The last patch that makes HFLIP and VFLIP configurable may be contentious
as I've retained the default configuration of inverted from the original
driver. I know this was discussed recently, but I can't recall the final
outcome.

I am relying on someone from Intel testing this out, as correcting the
cropping and supporting flips has changed the Bayer order. Seeing as this
is all above board in V4L2 terms I really hope that the layers above it
behave themselves.

Cheers
  Dave

Dave Stevenson (20):
  media: i2c: imx258: Remove unused defines
  media: i2c: imx258: Make image geometry meet sensor requirements
  media: i2c: imx258: Disable digital cropping on binned modes
  media: i2c: imx258: Remove redundant I2C writes.
  media: i2c: imx258: Add regulator control
  media: i2c: imx258: Make V4L2_CID_VBLANK configurable.
  media: i2c: imx258: Split out common registers from the mode based
    ones
  media: i2c: imx258: Add support for 24MHz clock
  media: i2c: imx258: Add support for running on 2 CSI data lanes
  media: i2c: imx258: Follow normal V4L2 behaviours for clipping
    exposure
  media: i2c: imx258: Add get_selection for pixel array information
  media: i2c: imx258: Allow configuration of clock lane behaviour
  media: i2c: imx258: Correct max FRM_LENGTH_LINES value
  media: i2c: imx258: Issue reset before starting streaming
  media: i2c: imx258: Set pixel_rate range to the same as the value
  media: i2c: imx258: Support faster pixel rate on binned modes
  dt-bindings: media: imx258: Rename to include vendor prefix
  dt-bindings: media: imx258: Add alternate compatible strings
  media: i2c: imx258: Change register settings for variants of the
    sensor
  media: i2c: imx258: Make HFLIP and VFLIP controls writable

Luis Garcia (5):
  dt-bindings: media: imx258: Add binding for powerdown-gpio
  media: i2c: imx258: Add support for powerdown gpio
  media: i2c: imx258: Add support for reset gpio
  media:i2c: imx258: Use v4l2_link_freq_to_bitmap helper
  media: i2c: imx258: Convert to new CCI register access helpers

 .../i2c/{imx258.yaml => sony,imx258.yaml}     |   15 +-
 MAINTAINERS                                   |    2 +-
 drivers/media/i2c/Kconfig                     |    1 +
 drivers/media/i2c/imx258.c                    | 1439 ++++++++++-------
 4 files changed, 848 insertions(+), 609 deletions(-)
 rename Documentation/devicetree/bindings/media/i2c/{imx258.yaml => sony,imx258.yaml} (86%)

Comments

Alexander Stein April 15, 2024, 6:25 a.m. UTC | #1 
Hi,

Am Sonntag, 14. April 2024, 22:34:40 CEST schrieb git@luigi311.com:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> The output image is defined as being 4208x3118 pixels in size.
> Y_ADD_STA register was set to 0, and Y_ADD_END to 3118, giving
> 3119 lines total.
> 
> The datasheet lists a requirement for Y_ADD_STA to be a multiple
> of a power of 2 depending on binning/scaling mode (2 for full pixel,
> 4 for x2-bin/scale, 8 for (x2-bin)+(x2-subsample) or x4-bin, or 16
> for (x4-bin)+(x2-subsample)).
> (Y_ADD_END – Y_ADD_STA + 1) also has to be a similar power of 2.
> 
> The current configuration for the full res modes breaks that second
> requirement, and we can't increase Y_ADD_STA to 1 to retain exactly
> the same field of view as that then breaks the first requirement.
> For the binned modes, they are worse off as 3118 is not a multiple of
> 4.
> 
> Increase the main mode to 4208x3120 so that it is the same FOV as the
> binned modes, with Y_ADD_STA at 0.
> Fix Y_ADD_STA and Y_ADD_END for the binned modes so that they meet the
> sensor requirements.
> 
> This does change the Bayer order as the default configuration is for
> H&V flips to be enabled, so readout is from Y_STA_END to Y_ADD_STA,
> and this patch has changed Y_STA_END.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Signed-off-by: Luis Garcia <git@luigi311.com>
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> ---
>  drivers/media/i2c/imx258.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 2dbafd21dd70..4a7048d834c6 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> [snip]
> @@ -707,7 +707,7 @@ static int imx258_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  	/* Initialize try_fmt */
>  	try_fmt->width = supported_modes[0].width;
>  	try_fmt->height = supported_modes[0].height;
> -	try_fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> +	try_fmt->code = MEDIA_BUS_FMT_SBGGR10_1X10;

Does someone have access to the data sheet? I am wondering how the
arrangement of the pixel array is shown. I've the following (identical)
array for these sensors:
* imx290/imx327
* imx219
* imx415

G B G B
R G R G
G B G B
R G R G

Yet each driver configures a different bus format:

* imx290.c: MEDIA_BUS_FMT_SRGGB10_1X10
* imx415.c: MEDIA_BUS_FMT_SGBRG10_1X10
* imx219.c: MEDIA_BUS_FMT_SRGGB10_1X10 (no flip)

imx219 actually defines all 4 10 Bit Bayer patterns and the comment
indicates this depends on how v or h flip is configured.
Reading the commit message apparently the same is true for this driver.

Still this is confusing as I would have expected flipping to be disabled by
default, expecting the same Bayer pattern order for all drivers. Can someone
shed some light?

Best regards,
Alexander
Tommaso Merciai April 15, 2024, 2:47 p.m. UTC | #2 
Hi Luis,

On Sun, Apr 14, 2024 at 02:35:03PM -0600, git@luigi311.com wrote:
> From: Luis Garcia <git@luigi311.com>
> 
> Use the new comon CCI register access helpers to replace the private
> register access helpers in the imx258 driver.
> 
> Signed-off-by: Luis Garcia <git@luigi311.com>
> ---
>  drivers/media/i2c/Kconfig  |   1 +
>  drivers/media/i2c/imx258.c | 804 ++++++++++++++++---------------------
>  2 files changed, 346 insertions(+), 459 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 56f276b920ab..6707b0c3c4eb 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -139,6 +139,7 @@ config VIDEO_IMX219
>  
>  config VIDEO_IMX258
>  	tristate "Sony IMX258 sensor support"
> +	select V4L2_CCI_I2C
>  	help
>  	  This is a Video4Linux2 sensor driver for the Sony
>  	  IMX258 camera.
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 65846dff775e..8fc9750e5ec9 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -8,22 +8,20 @@
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
> +#include <media/v4l2-cci.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-fwnode.h>
>  #include <asm/unaligned.h>
>  
> -#define IMX258_REG_VALUE_08BIT		1
> -#define IMX258_REG_VALUE_16BIT		2
> -
> -#define IMX258_REG_MODE_SELECT		0x0100
> +#define IMX258_REG_MODE_SELECT		CCI_REG8(0x0100)
>  #define IMX258_MODE_STANDBY		0x00
>  #define IMX258_MODE_STREAMING		0x01
>  
> -#define IMX258_REG_RESET		0x0103
> +#define IMX258_REG_RESET		CCI_REG8(0x0103)
>  
>  /* Chip ID */
> -#define IMX258_REG_CHIP_ID		0x0016
> +#define IMX258_REG_CHIP_ID		CCI_REG16(0x0016)
>  #define IMX258_CHIP_ID			0x0258
>  
>  /* V_TIMING internal */
> @@ -32,13 +30,11 @@
>  #define IMX258_VTS_30FPS_VGA		0x034c
>  #define IMX258_VTS_MAX			65525
>  
> -#define IMX258_REG_VTS			0x0340
> -
>  /* HBLANK control - read only */
>  #define IMX258_PPL_DEFAULT		5352
>  
>  /* Exposure control */
> -#define IMX258_REG_EXPOSURE		0x0202
> +#define IMX258_REG_EXPOSURE		CCI_REG16(0x0202)
>  #define IMX258_EXPOSURE_OFFSET		10
>  #define IMX258_EXPOSURE_MIN		4
>  #define IMX258_EXPOSURE_STEP		1
> @@ -46,38 +42,38 @@
>  #define IMX258_EXPOSURE_MAX		(IMX258_VTS_MAX - IMX258_EXPOSURE_OFFSET)
>  
>  /* Analog gain control */
> -#define IMX258_REG_ANALOG_GAIN		0x0204
> +#define IMX258_REG_ANALOG_GAIN		CCI_REG16(0x0204)
>  #define IMX258_ANA_GAIN_MIN		0
>  #define IMX258_ANA_GAIN_MAX		480
>  #define IMX258_ANA_GAIN_STEP		1
>  #define IMX258_ANA_GAIN_DEFAULT		0x0
>  
>  /* Digital gain control */
> -#define IMX258_REG_GR_DIGITAL_GAIN	0x020e
> -#define IMX258_REG_R_DIGITAL_GAIN	0x0210
> -#define IMX258_REG_B_DIGITAL_GAIN	0x0212
> -#define IMX258_REG_GB_DIGITAL_GAIN	0x0214
> +#define IMX258_REG_GR_DIGITAL_GAIN	CCI_REG16(0x020e)
> +#define IMX258_REG_R_DIGITAL_GAIN	CCI_REG16(0x0210)
> +#define IMX258_REG_B_DIGITAL_GAIN	CCI_REG16(0x0212)
> +#define IMX258_REG_GB_DIGITAL_GAIN	CCI_REG16(0x0214)
>  #define IMX258_DGTL_GAIN_MIN		0
>  #define IMX258_DGTL_GAIN_MAX		4096	/* Max = 0xFFF */
>  #define IMX258_DGTL_GAIN_DEFAULT	1024
>  #define IMX258_DGTL_GAIN_STEP		1
>  
>  /* HDR control */
> -#define IMX258_REG_HDR			0x0220
> +#define IMX258_REG_HDR			CCI_REG8(0x0220)
>  #define IMX258_HDR_ON			BIT(0)
> -#define IMX258_REG_HDR_RATIO		0x0222
> +#define IMX258_REG_HDR_RATIO		CCI_REG8(0x0222)
>  #define IMX258_HDR_RATIO_MIN		0
>  #define IMX258_HDR_RATIO_MAX		5
>  #define IMX258_HDR_RATIO_STEP		1
>  #define IMX258_HDR_RATIO_DEFAULT	0x0
>  
>  /* Test Pattern Control */
> -#define IMX258_REG_TEST_PATTERN		0x0600
> +#define IMX258_REG_TEST_PATTERN		CCI_REG16(0x0600)
>  
> -#define IMX258_CLK_BLANK_STOP		0x4040
> +#define IMX258_CLK_BLANK_STOP		CCI_REG8(0x4040)
>  
>  /* Orientation */
> -#define REG_MIRROR_FLIP_CONTROL		0x0101
> +#define REG_MIRROR_FLIP_CONTROL		CCI_REG8(0x0101)
>  #define REG_CONFIG_MIRROR_HFLIP		0x01
>  #define REG_CONFIG_MIRROR_VFLIP		0x02
>  
> @@ -89,14 +85,53 @@
>  #define IMX258_PIXEL_ARRAY_WIDTH	4208U
>  #define IMX258_PIXEL_ARRAY_HEIGHT	3120U
>  
> -struct imx258_reg {
> -	u16 address;
> -	u8 val;
> -};
> +/* regs */
> +#define IMX258_REG_PLL_MULT_DRIV                  CCI_REG8(0x0310)
> +#define IMX258_REG_IVTPXCK_DIV                    CCI_REG8(0x0301)
> +#define IMX258_REG_IVTSYCK_DIV                    CCI_REG8(0x0303)
> +#define IMX258_REG_PREPLLCK_VT_DIV                CCI_REG8(0x0305)
> +#define IMX258_REG_IOPPXCK_DIV                    CCI_REG8(0x0309)
> +#define IMX258_REG_IOPSYCK_DIV                    CCI_REG8(0x030b)
> +#define IMX258_REG_PREPLLCK_OP_DIV                CCI_REG8(0x030d)
> +#define IMX258_REG_PHASE_PIX_OUTEN                CCI_REG8(0x3030)
> +#define IMX258_REG_PDPIX_DATA_RATE                CCI_REG8(0x3032)
> +#define IMX258_REG_SCALE_MODE                     CCI_REG8(0x0401)
> +#define IMX258_REG_SCALE_MODE_EXT                 CCI_REG8(0x3038)
> +#define IMX258_REG_AF_WINDOW_MODE                 CCI_REG8(0x7bcd)
> +#define IMX258_REG_FRM_LENGTH_CTL                 CCI_REG8(0x0350)
> +#define IMX258_REG_CSI_LANE_MODE                  CCI_REG8(0x0114)
> +#define IMX258_REG_X_EVN_INC                      CCI_REG8(0x0381)
> +#define IMX258_REG_X_ODD_INC                      CCI_REG8(0x0383)
> +#define IMX258_REG_Y_EVN_INC                      CCI_REG8(0x0385)
> +#define IMX258_REG_Y_ODD_INC                      CCI_REG8(0x0387)
> +#define IMX258_REG_BINNING_MODE                   CCI_REG8(0x0900)
> +#define IMX258_REG_BINNING_TYPE_V                 CCI_REG8(0x0901)
> +#define IMX258_REG_FORCE_FD_SUM                   CCI_REG8(0x300d)
> +#define IMX258_REG_DIG_CROP_X_OFFSET              CCI_REG16(0x0408)
> +#define IMX258_REG_DIG_CROP_Y_OFFSET              CCI_REG16(0x040a)
> +#define IMX258_REG_DIG_CROP_IMAGE_WIDTH           CCI_REG16(0x040c)
> +#define IMX258_REG_DIG_CROP_IMAGE_HEIGHT          CCI_REG16(0x040e)
> +#define IMX258_REG_SCALE_M                        CCI_REG16(0x0404)
> +#define IMX258_REG_X_OUT_SIZE                     CCI_REG16(0x034c)
> +#define IMX258_REG_Y_OUT_SIZE                     CCI_REG16(0x034e)
> +#define IMX258_REG_X_ADD_STA                      CCI_REG16(0x0344)
> +#define IMX258_REG_Y_ADD_STA                      CCI_REG16(0x0346)
> +#define IMX258_REG_X_ADD_END                      CCI_REG16(0x0348)
> +#define IMX258_REG_Y_ADD_END                      CCI_REG16(0x034a)
> +#define IMX258_REG_EXCK_FREQ                      CCI_REG16(0x0136)
> +#define IMX258_REG_CSI_DT_FMT                     CCI_REG16(0x0112)
> +#define IMX258_REG_LINE_LENGTH_PCK                CCI_REG16(0x0342)
> +#define IMX258_REG_SCALE_M_EXT                    CCI_REG16(0x303a)
> +#define IMX258_REG_FRM_LENGTH_LINES               CCI_REG16(0x0340)
> +#define IMX258_REG_FINE_INTEG_TIME                CCI_REG8(0x0200)
> +#define IMX258_REG_PLL_IVT_MPY                    CCI_REG16(0x0306)
> +#define IMX258_REG_PLL_IOP_MPY                    CCI_REG16(0x030e)
> +#define IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H       CCI_REG16(0x0820)
> +#define IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L       CCI_REG16(0x0822)
>  
>  struct imx258_reg_list {
>  	u32 num_of_regs;
> -	const struct imx258_reg *regs;
> +	const struct cci_reg_sequence *regs;
>  };
>  
>  struct imx258_link_cfg {
> @@ -143,329 +178,264 @@ struct imx258_mode {
>   * To avoid further computation of clock settings, adopt the same per
>   * lane data rate when using 2 lanes, thus allowing a maximum of 15fps.
>   */
> -static const struct imx258_reg mipi_1267mbps_19_2mhz_2l[] = {
> -	{ 0x0136, 0x13 },
> -	{ 0x0137, 0x33 },
> -	{ 0x0301, 0x0A },
> -	{ 0x0303, 0x02 },
> -	{ 0x0305, 0x03 },
> -	{ 0x0306, 0x00 },
> -	{ 0x0307, 0xC6 },
> -	{ 0x0309, 0x0A },
> -	{ 0x030B, 0x01 },
> -	{ 0x030D, 0x02 },
> -	{ 0x030E, 0x00 },
> -	{ 0x030F, 0xD8 },
> -	{ 0x0310, 0x00 },
> -
> -	{ 0x0114, 0x01 },
> -	{ 0x0820, 0x09 },
> -	{ 0x0821, 0xa6 },
> -	{ 0x0822, 0x66 },
> -	{ 0x0823, 0x66 },
> +static const struct cci_reg_sequence mipi_1267mbps_19_2mhz_2l[] = {
> +	{ IMX258_REG_EXCK_FREQ, 0x1333 },
> +	{ IMX258_REG_IVTPXCK_DIV, 10 },
> +	{ IMX258_REG_IVTSYCK_DIV, 2 },
> +	{ IMX258_REG_PREPLLCK_VT_DIV, 3 },
> +	{ IMX258_REG_PLL_IVT_MPY, 198 },
> +	{ IMX258_REG_IOPPXCK_DIV, 10 },
> +	{ IMX258_REG_IOPSYCK_DIV, 1 },
> +	{ IMX258_REG_PREPLLCK_OP_DIV, 2 },
> +	{ IMX258_REG_PLL_IOP_MPY, 216 },
> +	{ IMX258_REG_PLL_MULT_DRIV, 0 },
> +
> +	{ IMX258_REG_CSI_LANE_MODE, 1 },
> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 1267 * 2 },
> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0 },
>  };
>  
> -static const struct imx258_reg mipi_1267mbps_19_2mhz_4l[] = {
> -	{ 0x0136, 0x13 },
> -	{ 0x0137, 0x33 },
> -	{ 0x0301, 0x05 },
> -	{ 0x0303, 0x02 },
> -	{ 0x0305, 0x03 },
> -	{ 0x0306, 0x00 },
> -	{ 0x0307, 0xC6 },
> -	{ 0x0309, 0x0A },
> -	{ 0x030B, 0x01 },
> -	{ 0x030D, 0x02 },
> -	{ 0x030E, 0x00 },
> -	{ 0x030F, 0xD8 },
> -	{ 0x0310, 0x00 },
> -
> -	{ 0x0114, 0x03 },
> -	{ 0x0820, 0x13 },
> -	{ 0x0821, 0x4C },
> -	{ 0x0822, 0xCC },
> -	{ 0x0823, 0xCC },
> +static const struct cci_reg_sequence mipi_1267mbps_19_2mhz_4l[] = {
> +	{ IMX258_REG_EXCK_FREQ, 0x1333 },
> +	{ IMX258_REG_IVTPXCK_DIV, 5 },
> +	{ IMX258_REG_IVTSYCK_DIV, 2 },
> +	{ IMX258_REG_PREPLLCK_VT_DIV, 3 },
> +	{ IMX258_REG_PLL_IVT_MPY, 198 },
> +	{ IMX258_REG_IOPPXCK_DIV, 10 },
> +	{ IMX258_REG_IOPSYCK_DIV, 1 },
> +	{ IMX258_REG_PREPLLCK_OP_DIV, 2 },
> +	{ IMX258_REG_PLL_IOP_MPY, 216 },
> +	{ IMX258_REG_PLL_MULT_DRIV, 0 },
> +
> +	{ IMX258_REG_CSI_LANE_MODE, 3 },
> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 1267 * 4 },
> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0 },
>  };
>  
> -static const struct imx258_reg mipi_1272mbps_24mhz_2l[] = {
> -	{ 0x0136, 0x18 },
> -	{ 0x0137, 0x00 },
> -	{ 0x0301, 0x0a },
> -	{ 0x0303, 0x02 },
> -	{ 0x0305, 0x04 },
> -	{ 0x0306, 0x00 },
> -	{ 0x0307, 0xD4 },
> -	{ 0x0309, 0x0A },
> -	{ 0x030B, 0x01 },
> -	{ 0x030D, 0x02 },
> -	{ 0x030E, 0x00 },
> -	{ 0x030F, 0xD8 },
> -	{ 0x0310, 0x00 },
> -
> -	{ 0x0114, 0x01 },
> -	{ 0x0820, 0x13 },
> -	{ 0x0821, 0x4C },
> -	{ 0x0822, 0xCC },
> -	{ 0x0823, 0xCC },
> +static const struct cci_reg_sequence mipi_1272mbps_24mhz_2l[] = {
> +	{ IMX258_REG_EXCK_FREQ, 0x1800 },
> +	{ IMX258_REG_IVTPXCK_DIV, 10 },
> +	{ IMX258_REG_IVTSYCK_DIV, 2 },
> +	{ IMX258_REG_PREPLLCK_VT_DIV, 4 },
> +	{ IMX258_REG_PLL_IVT_MPY, 212 },
> +	{ IMX258_REG_IOPPXCK_DIV, 10 },
> +	{ IMX258_REG_IOPSYCK_DIV, 1 },
> +	{ IMX258_REG_PREPLLCK_OP_DIV, 2 },
> +	{ IMX258_REG_PLL_IOP_MPY, 216 },
> +	{ IMX258_REG_PLL_MULT_DRIV, 0 },
> +
> +	{ IMX258_REG_CSI_LANE_MODE, 1 },
> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 1272 * 2 },
> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0 },
>  };
>  
> -static const struct imx258_reg mipi_1272mbps_24mhz_4l[] = {
> -	{ 0x0136, 0x18 },
> -	{ 0x0137, 0x00 },
> -	{ 0x0301, 0x05 },
> -	{ 0x0303, 0x02 },
> -	{ 0x0305, 0x04 },
> -	{ 0x0306, 0x00 },
> -	{ 0x0307, 0xD4 },
> -	{ 0x0309, 0x0A },
> -	{ 0x030B, 0x01 },
> -	{ 0x030D, 0x02 },
> -	{ 0x030E, 0x00 },
> -	{ 0x030F, 0xD8 },
> -	{ 0x0310, 0x00 },
> -
> -	{ 0x0114, 0x03 },
> -	{ 0x0820, 0x13 },
> -	{ 0x0821, 0xE0 },
> -	{ 0x0822, 0x00 },
> -	{ 0x0823, 0x00 },
> +static const struct cci_reg_sequence mipi_1272mbps_24mhz_4l[] = {
> +	{ IMX258_REG_EXCK_FREQ, 0x1800 },
> +	{ IMX258_REG_IVTPXCK_DIV, 5 },
> +	{ IMX258_REG_IVTSYCK_DIV, 2 },
> +	{ IMX258_REG_PREPLLCK_VT_DIV, 4 },
> +	{ IMX258_REG_PLL_IVT_MPY, 212 },
> +	{ IMX258_REG_IOPPXCK_DIV, 10 },
> +	{ IMX258_REG_IOPSYCK_DIV, 1 },
> +	{ IMX258_REG_PREPLLCK_OP_DIV, 2 },
> +	{ IMX258_REG_PLL_IOP_MPY, 216 },
> +	{ IMX258_REG_PLL_MULT_DRIV, 0 },
> +
> +	{ IMX258_REG_CSI_LANE_MODE, 3 },
> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 1272 * 4 },
> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0 },
>  };
>  
> -static const struct imx258_reg mipi_640mbps_19_2mhz_2l[] = {
> -	{ 0x0136, 0x13 },
> -	{ 0x0137, 0x33 },
> -	{ 0x0301, 0x05 },
> -	{ 0x0303, 0x02 },
> -	{ 0x0305, 0x03 },
> -	{ 0x0306, 0x00 },
> -	{ 0x0307, 0x64 },
> -	{ 0x0309, 0x0A },
> -	{ 0x030B, 0x01 },
> -	{ 0x030D, 0x02 },
> -	{ 0x030E, 0x00 },
> -	{ 0x030F, 0xD8 },
> -	{ 0x0310, 0x00 },
> -
> -	{ 0x0114, 0x01 },
> -	{ 0x0820, 0x05 },
> -	{ 0x0821, 0x00 },
> -	{ 0x0822, 0x00 },
> -	{ 0x0823, 0x00 },
> +static const struct cci_reg_sequence mipi_640mbps_19_2mhz_2l[] = {
> +	{ IMX258_REG_EXCK_FREQ, 0x1333 },
> +	{ IMX258_REG_IVTPXCK_DIV, 5 },
> +	{ IMX258_REG_IVTSYCK_DIV, 2 },
> +	{ IMX258_REG_PREPLLCK_VT_DIV, 3 },
> +	{ IMX258_REG_PLL_IVT_MPY, 100 },
> +	{ IMX258_REG_IOPPXCK_DIV, 10 },
> +	{ IMX258_REG_IOPSYCK_DIV, 1 },
> +	{ IMX258_REG_PREPLLCK_OP_DIV, 2 },
> +	{ IMX258_REG_PLL_IOP_MPY, 216 },
> +	{ IMX258_REG_PLL_MULT_DRIV, 0 },
> +
> +	{ IMX258_REG_CSI_LANE_MODE, 1 },
> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 640 * 2 },
> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0 },
>  };
>  
> -static const struct imx258_reg mipi_640mbps_19_2mhz_4l[] = {
> -	{ 0x0136, 0x13 },
> -	{ 0x0137, 0x33 },
> -	{ 0x0301, 0x05 },
> -	{ 0x0303, 0x02 },
> -	{ 0x0305, 0x03 },
> -	{ 0x0306, 0x00 },
> -	{ 0x0307, 0x64 },
> -	{ 0x0309, 0x0A },
> -	{ 0x030B, 0x01 },
> -	{ 0x030D, 0x02 },
> -	{ 0x030E, 0x00 },
> -	{ 0x030F, 0xD8 },
> -	{ 0x0310, 0x00 },
> -
> -	{ 0x0114, 0x03 },
> -	{ 0x0820, 0x0A },
> -	{ 0x0821, 0x00 },
> -	{ 0x0822, 0x00 },
> -	{ 0x0823, 0x00 },
> +static const struct cci_reg_sequence mipi_640mbps_19_2mhz_4l[] = {
> +	{ IMX258_REG_EXCK_FREQ, 0x1333 },
> +	{ IMX258_REG_IVTPXCK_DIV, 5 },
> +	{ IMX258_REG_IVTSYCK_DIV, 2 },
> +	{ IMX258_REG_PREPLLCK_VT_DIV, 3 },
> +	{ IMX258_REG_PLL_IVT_MPY, 100 },
> +	{ IMX258_REG_IOPPXCK_DIV, 10 },
> +	{ IMX258_REG_IOPSYCK_DIV, 1 },
> +	{ IMX258_REG_PREPLLCK_OP_DIV, 2 },
> +	{ IMX258_REG_PLL_IOP_MPY, 216 },
> +	{ IMX258_REG_PLL_MULT_DRIV, 0 },
> +
> +	{ IMX258_REG_CSI_LANE_MODE, 3 },
> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 640 * 4 },
> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0 },
>  };
>  
> -static const struct imx258_reg mipi_642mbps_24mhz_2l[] = {
> -	{ 0x0136, 0x18 },
> -	{ 0x0137, 0x00 },
> -	{ 0x0301, 0x05 },
> -	{ 0x0303, 0x02 },
> -	{ 0x0305, 0x04 },
> -	{ 0x0306, 0x00 },
> -	{ 0x0307, 0x6B },
> -	{ 0x0309, 0x0A },
> -	{ 0x030B, 0x01 },
> -	{ 0x030D, 0x02 },
> -	{ 0x030E, 0x00 },
> -	{ 0x030F, 0xD8 },
> -	{ 0x0310, 0x00 },
> -
> -	{ 0x0114, 0x01 },
> -	{ 0x0820, 0x0A },
> -	{ 0x0821, 0x00 },
> -	{ 0x0822, 0x00 },
> -	{ 0x0823, 0x00 },
> +static const struct cci_reg_sequence mipi_642mbps_24mhz_2l[] = {
> +	{ IMX258_REG_EXCK_FREQ, 0x1800 },
> +	{ IMX258_REG_IVTPXCK_DIV, 5 },
> +	{ IMX258_REG_IVTSYCK_DIV, 2 },
> +	{ IMX258_REG_PREPLLCK_VT_DIV, 4 },
> +	{ IMX258_REG_PLL_IVT_MPY, 107 },
> +	{ IMX258_REG_IOPPXCK_DIV, 10 },
> +	{ IMX258_REG_IOPSYCK_DIV, 1 },
> +	{ IMX258_REG_PREPLLCK_OP_DIV, 2 },
> +	{ IMX258_REG_PLL_IOP_MPY, 216 },
> +	{ IMX258_REG_PLL_MULT_DRIV, 0 },
> +
> +	{ IMX258_REG_CSI_LANE_MODE, 1 },
> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 642 * 2 },
> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0 },
>  };
>  
> -static const struct imx258_reg mipi_642mbps_24mhz_4l[] = {
> -	{ 0x0136, 0x18 },
> -	{ 0x0137, 0x00 },
> -	{ 0x0301, 0x05 },
> -	{ 0x0303, 0x02 },
> -	{ 0x0305, 0x04 },
> -	{ 0x0306, 0x00 },
> -	{ 0x0307, 0x6B },
> -	{ 0x0309, 0x0A },
> -	{ 0x030B, 0x01 },
> -	{ 0x030D, 0x02 },
> -	{ 0x030E, 0x00 },
> -	{ 0x030F, 0xD8 },
> -	{ 0x0310, 0x00 },
> -
> -	{ 0x0114, 0x03 },
> -	{ 0x0820, 0x0A },
> -	{ 0x0821, 0x00 },
> -	{ 0x0822, 0x00 },
> -	{ 0x0823, 0x00 },
> +static const struct cci_reg_sequence mipi_642mbps_24mhz_4l[] = {
> +	{ IMX258_REG_EXCK_FREQ, 0x1800 },
> +	{ IMX258_REG_IVTPXCK_DIV, 5 },
> +	{ IMX258_REG_IVTSYCK_DIV, 2 },
> +	{ IMX258_REG_PREPLLCK_VT_DIV, 4 },
> +	{ IMX258_REG_PLL_IVT_MPY, 107 },
> +	{ IMX258_REG_IOPPXCK_DIV, 10 },
> +	{ IMX258_REG_IOPSYCK_DIV, 1 },
> +	{ IMX258_REG_PREPLLCK_OP_DIV, 2 },
> +	{ IMX258_REG_PLL_IOP_MPY, 216 },
> +	{ IMX258_REG_PLL_MULT_DRIV, 0 },
> +
> +	{ IMX258_REG_CSI_LANE_MODE, 3 },
> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 642 * 4 },
> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0 },
>  };
>  
> -static const struct imx258_reg mode_common_regs[] = {
> -	{ 0x3051, 0x00 },
> -	{ 0x6B11, 0xCF },
> -	{ 0x7FF0, 0x08 },
> -	{ 0x7FF1, 0x0F },
> -	{ 0x7FF2, 0x08 },
> -	{ 0x7FF3, 0x1B },
> -	{ 0x7FF4, 0x23 },
> -	{ 0x7FF5, 0x60 },
> -	{ 0x7FF6, 0x00 },
> -	{ 0x7FF7, 0x01 },
> -	{ 0x7FF8, 0x00 },
> -	{ 0x7FF9, 0x78 },
> -	{ 0x7FFA, 0x00 },
> -	{ 0x7FFB, 0x00 },
> -	{ 0x7FFC, 0x00 },
> -	{ 0x7FFD, 0x00 },
> -	{ 0x7FFE, 0x00 },
> -	{ 0x7FFF, 0x03 },
> -	{ 0x7F76, 0x03 },
> -	{ 0x7F77, 0xFE },
> -	{ 0x7FA8, 0x03 },
> -	{ 0x7FA9, 0xFE },
> -	{ 0x7B24, 0x81 },
> -	{ 0x6564, 0x07 },
> -	{ 0x6B0D, 0x41 },
> -	{ 0x653D, 0x04 },
> -	{ 0x6B05, 0x8C },
> -	{ 0x6B06, 0xF9 },
> -	{ 0x6B08, 0x65 },
> -	{ 0x6B09, 0xFC },
> -	{ 0x6B0A, 0xCF },
> -	{ 0x6B0B, 0xD2 },
> -	{ 0x6700, 0x0E },
> -	{ 0x6707, 0x0E },
> -	{ 0x9104, 0x00 },
> -	{ 0x4648, 0x7F },
> -	{ 0x7420, 0x00 },
> -	{ 0x7421, 0x1C },
> -	{ 0x7422, 0x00 },
> -	{ 0x7423, 0xD7 },
> -	{ 0x5F04, 0x00 },
> -	{ 0x5F05, 0xED },
> -	{ 0x0112, 0x0A },
> -	{ 0x0113, 0x0A },
> -	{ 0x0342, 0x14 },
> -	{ 0x0343, 0xE8 },
> -	{ 0x0344, 0x00 },
> -	{ 0x0345, 0x00 },
> -	{ 0x0346, 0x00 },
> -	{ 0x0347, 0x00 },
> -	{ 0x0348, 0x10 },
> -	{ 0x0349, 0x6F },
> -	{ 0x034A, 0x0C },
> -	{ 0x034B, 0x2F },
> -	{ 0x0381, 0x01 },
> -	{ 0x0383, 0x01 },
> -	{ 0x0385, 0x01 },
> -	{ 0x0387, 0x01 },
> -	{ 0x0404, 0x00 },
> -	{ 0x0408, 0x00 },
> -	{ 0x0409, 0x00 },
> -	{ 0x040A, 0x00 },
> -	{ 0x040B, 0x00 },
> -	{ 0x040C, 0x10 },
> -	{ 0x040D, 0x70 },
> -	{ 0x3038, 0x00 },
> -	{ 0x303A, 0x00 },
> -	{ 0x303B, 0x10 },
> -	{ 0x300D, 0x00 },
> -	{ 0x0350, 0x00 },
> -	{ 0x0204, 0x00 },
> -	{ 0x0205, 0x00 },
> -	{ 0x020E, 0x01 },
> -	{ 0x020F, 0x00 },
> -	{ 0x0210, 0x01 },
> -	{ 0x0211, 0x00 },
> -	{ 0x0212, 0x01 },
> -	{ 0x0213, 0x00 },
> -	{ 0x0214, 0x01 },
> -	{ 0x0215, 0x00 },
> -	{ 0x7BCD, 0x00 },
> -	{ 0x94DC, 0x20 },
> -	{ 0x94DD, 0x20 },
> -	{ 0x94DE, 0x20 },
> -	{ 0x95DC, 0x20 },
> -	{ 0x95DD, 0x20 },
> -	{ 0x95DE, 0x20 },
> -	{ 0x7FB0, 0x00 },
> -	{ 0x9010, 0x3E },
> -	{ 0x9419, 0x50 },
> -	{ 0x941B, 0x50 },
> -	{ 0x9519, 0x50 },
> -	{ 0x951B, 0x50 },
> -	{ 0x3030, 0x00 },
> -	{ 0x3032, 0x00 },
> -	{ 0x0220, 0x00 },
> +static const struct cci_reg_sequence mode_common_regs[] = {
> +	{ CCI_REG8(0x3051), 0x00 },
> +	{ CCI_REG8(0x6B11), 0xCF },
> +	{ CCI_REG8(0x7FF0), 0x08 },
> +	{ CCI_REG8(0x7FF1), 0x0F },
> +	{ CCI_REG8(0x7FF2), 0x08 },
> +	{ CCI_REG8(0x7FF3), 0x1B },
> +	{ CCI_REG8(0x7FF4), 0x23 },
> +	{ CCI_REG8(0x7FF5), 0x60 },
> +	{ CCI_REG8(0x7FF6), 0x00 },
> +	{ CCI_REG8(0x7FF7), 0x01 },
> +	{ CCI_REG8(0x7FF8), 0x00 },
> +	{ CCI_REG8(0x7FF9), 0x78 },
> +	{ CCI_REG8(0x7FFA), 0x00 },
> +	{ CCI_REG8(0x7FFB), 0x00 },
> +	{ CCI_REG8(0x7FFC), 0x00 },
> +	{ CCI_REG8(0x7FFD), 0x00 },
> +	{ CCI_REG8(0x7FFE), 0x00 },
> +	{ CCI_REG8(0x7FFF), 0x03 },
> +	{ CCI_REG8(0x7F76), 0x03 },
> +	{ CCI_REG8(0x7F77), 0xFE },
> +	{ CCI_REG8(0x7FA8), 0x03 },
> +	{ CCI_REG8(0x7FA9), 0xFE },
> +	{ CCI_REG8(0x7B24), 0x81 },
> +	{ CCI_REG8(0x6564), 0x07 },
> +	{ CCI_REG8(0x6B0D), 0x41 },
> +	{ CCI_REG8(0x653D), 0x04 },
> +	{ CCI_REG8(0x6B05), 0x8C },
> +	{ CCI_REG8(0x6B06), 0xF9 },
> +	{ CCI_REG8(0x6B08), 0x65 },
> +	{ CCI_REG8(0x6B09), 0xFC },
> +	{ CCI_REG8(0x6B0A), 0xCF },
> +	{ CCI_REG8(0x6B0B), 0xD2 },
> +	{ CCI_REG8(0x6700), 0x0E },
> +	{ CCI_REG8(0x6707), 0x0E },
> +	{ CCI_REG8(0x9104), 0x00 },
> +	{ CCI_REG8(0x4648), 0x7F },
> +	{ CCI_REG8(0x7420), 0x00 },
> +	{ CCI_REG8(0x7421), 0x1C },
> +	{ CCI_REG8(0x7422), 0x00 },
> +	{ CCI_REG8(0x7423), 0xD7 },
> +	{ CCI_REG8(0x5F04), 0x00 },
> +	{ CCI_REG8(0x5F05), 0xED },
> +	{IMX258_REG_CSI_DT_FMT, 0x0a0a},
> +	{IMX258_REG_LINE_LENGTH_PCK, 5352},
> +	{IMX258_REG_X_ADD_STA, 0},
> +	{IMX258_REG_Y_ADD_STA, 0},
> +	{IMX258_REG_X_ADD_END, 4207},
> +	{IMX258_REG_Y_ADD_END, 3119},
> +	{IMX258_REG_X_EVN_INC, 1},
> +	{IMX258_REG_X_ODD_INC, 1},
> +	{IMX258_REG_Y_EVN_INC, 1},
> +	{IMX258_REG_Y_ODD_INC, 1},
> +	{IMX258_REG_DIG_CROP_X_OFFSET, 0},
> +	{IMX258_REG_DIG_CROP_Y_OFFSET, 0},
> +	{IMX258_REG_DIG_CROP_IMAGE_WIDTH, 4208},
> +	{IMX258_REG_SCALE_MODE_EXT, 0},
> +	{IMX258_REG_SCALE_M_EXT, 16},
> +	{IMX258_REG_FORCE_FD_SUM, 0},
> +	{IMX258_REG_FRM_LENGTH_CTL, 0},
> +	{IMX258_REG_ANALOG_GAIN, 0},
> +	{IMX258_REG_GR_DIGITAL_GAIN, 256},
> +	{IMX258_REG_R_DIGITAL_GAIN, 256},
> +	{IMX258_REG_B_DIGITAL_GAIN, 256},
> +	{IMX258_REG_GB_DIGITAL_GAIN, 256},
> +	{IMX258_REG_AF_WINDOW_MODE, 0},
> +	{ CCI_REG8(0x94DC), 0x20 },
> +	{ CCI_REG8(0x94DD), 0x20 },
> +	{ CCI_REG8(0x94DE), 0x20 },
> +	{ CCI_REG8(0x95DC), 0x20 },
> +	{ CCI_REG8(0x95DD), 0x20 },
> +	{ CCI_REG8(0x95DE), 0x20 },
> +	{ CCI_REG8(0x7FB0), 0x00 },
> +	{ CCI_REG8(0x9010), 0x3E },
> +	{ CCI_REG8(0x9419), 0x50 },
> +	{ CCI_REG8(0x941B), 0x50 },
> +	{ CCI_REG8(0x9519), 0x50 },
> +	{ CCI_REG8(0x951B), 0x50 },
> +	{IMX258_REG_PHASE_PIX_OUTEN, 0},
> +	{IMX258_REG_PDPIX_DATA_RATE, 0},
> +	{IMX258_REG_HDR, 0},
>  };
>  
> -static const struct imx258_reg mode_4208x3120_regs[] = {
> -	{ 0x0900, 0x00 },
> -	{ 0x0901, 0x11 },
> -	{ 0x0401, 0x00 },
> -	{ 0x0405, 0x10 },
> -	{ 0x040E, 0x0C },
> -	{ 0x040F, 0x30 },
> -	{ 0x034C, 0x10 },
> -	{ 0x034D, 0x70 },
> -	{ 0x034E, 0x0C },
> -	{ 0x034F, 0x30 },
> +static const struct cci_reg_sequence mode_4208x3120_regs[] = {
> +	{IMX258_REG_BINNING_MODE, 0},
> +	{IMX258_REG_BINNING_TYPE_V, 0x11},
> +	{IMX258_REG_SCALE_MODE, 0},
> +	{IMX258_REG_SCALE_M, 16},
> +	{IMX258_REG_DIG_CROP_IMAGE_HEIGHT, 3120},
> +	{IMX258_REG_X_OUT_SIZE, 4208},
> +	{IMX258_REG_Y_OUT_SIZE, 3120},
>  };
>  
> -static const struct imx258_reg mode_2104_1560_regs[] = {
> -	{ 0x0900, 0x01 },
> -	{ 0x0901, 0x12 },
> -	{ 0x0401, 0x01 },
> -	{ 0x0405, 0x20 },
> -	{ 0x040E, 0x06 },
> -	{ 0x040F, 0x18 },
> -	{ 0x034C, 0x08 },
> -	{ 0x034D, 0x38 },
> -	{ 0x034E, 0x06 },
> -	{ 0x034F, 0x18 },
> +static const struct cci_reg_sequence mode_2104_1560_regs[] = {
> +	{IMX258_REG_BINNING_MODE, 1},
> +	{IMX258_REG_BINNING_TYPE_V, 0x12},
> +	{IMX258_REG_SCALE_MODE, 1},
> +	{IMX258_REG_SCALE_M, 32},
> +	{IMX258_REG_DIG_CROP_IMAGE_HEIGHT, 1560},
> +	{IMX258_REG_X_OUT_SIZE, 2104},
> +	{IMX258_REG_Y_OUT_SIZE, 1560},
>  };
>  
> -static const struct imx258_reg mode_1048_780_regs[] = {
> -	{ 0x0900, 0x01 },
> -	{ 0x0901, 0x14 },
> -	{ 0x0401, 0x01 },
> -	{ 0x0405, 0x40 },
> -	{ 0x040E, 0x03 },
> -	{ 0x040F, 0x0C },
> -	{ 0x034C, 0x04 },
> -	{ 0x034D, 0x18 },
> -	{ 0x034E, 0x03 },
> -	{ 0x034F, 0x0C },
> +static const struct cci_reg_sequence mode_1048_780_regs[] = {
> +	{IMX258_REG_BINNING_MODE, 1},
> +	{IMX258_REG_BINNING_TYPE_V, 0x14},
> +	{IMX258_REG_SCALE_MODE, 1},
> +	{IMX258_REG_SCALE_M, 64},
> +	{IMX258_REG_DIG_CROP_IMAGE_HEIGHT, 780},
> +	{IMX258_REG_X_OUT_SIZE, 1048},
> +	{IMX258_REG_Y_OUT_SIZE, 780},
>  };
>  
>  struct imx258_variant_cfg {
> -	const struct imx258_reg *regs;
> +	const struct cci_reg_sequence *regs;
>  	unsigned int num_regs;
>  };
>  
> -static const struct imx258_reg imx258_cfg_regs[] = {
> -	{ 0x3052, 0x00 },
> -	{ 0x4E21, 0x14 },
> -	{ 0x7B25, 0x00 },
> +static const struct cci_reg_sequence imx258_cfg_regs[] = {
> +	{ CCI_REG8(0x3052), 0x00 },
> +	{ CCI_REG8(0x4E21), 0x14 },
> +	{ CCI_REG8(0x7B25), 0x00 },
>  };
>  
>  static const struct imx258_variant_cfg imx258_cfg = {
> @@ -473,10 +443,10 @@ static const struct imx258_variant_cfg imx258_cfg = {
>  	.num_regs = ARRAY_SIZE(imx258_cfg_regs),
>  };
>  
> -static const struct imx258_reg imx258_pdaf_cfg_regs[] = {
> -	{ 0x3052, 0x01 },
> -	{ 0x4E21, 0x10 },
> -	{ 0x7B25, 0x01 },
> +static const struct cci_reg_sequence imx258_pdaf_cfg_regs[] = {
> +	{ CCI_REG8(0x3052), 0x01 },
> +	{ CCI_REG8(0x4E21), 0x10 },
> +	{ CCI_REG8(0x7B25), 0x01 },
>  };
>  
>  static const struct imx258_variant_cfg imx258_pdaf_cfg = {
> @@ -677,6 +647,7 @@ static const struct imx258_mode supported_modes[] = {
>  struct imx258 {
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
> +	struct regmap *regmap;
>  
>  	const struct imx258_variant_cfg *variant_cfg;
>  
> @@ -717,80 +688,6 @@ static inline struct imx258 *to_imx258(struct v4l2_subdev *_sd)
>  	return container_of(_sd, struct imx258, sd);
>  }
>  
> -/* Read registers up to 2 at a time */
> -static int imx258_read_reg(struct imx258 *imx258, u16 reg, u32 len, u32 *val)
> -{
> -	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> -	struct i2c_msg msgs[2];
> -	u8 addr_buf[2] = { reg >> 8, reg & 0xff };
> -	u8 data_buf[4] = { 0, };
> -	int ret;
> -
> -	if (len > 4)
> -		return -EINVAL;
> -
> -	/* Write register address */
> -	msgs[0].addr = client->addr;
> -	msgs[0].flags = 0;
> -	msgs[0].len = ARRAY_SIZE(addr_buf);
> -	msgs[0].buf = addr_buf;
> -
> -	/* Read data from register */
> -	msgs[1].addr = client->addr;
> -	msgs[1].flags = I2C_M_RD;
> -	msgs[1].len = len;
> -	msgs[1].buf = &data_buf[4 - len];
> -
> -	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> -	if (ret != ARRAY_SIZE(msgs))
> -		return -EIO;
> -
> -	*val = get_unaligned_be32(data_buf);
> -
> -	return 0;
> -}
> -
> -/* Write registers up to 2 at a time */
> -static int imx258_write_reg(struct imx258 *imx258, u16 reg, u32 len, u32 val)
> -{
> -	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> -	u8 buf[6];
> -
> -	if (len > 4)
> -		return -EINVAL;
> -
> -	put_unaligned_be16(reg, buf);
> -	put_unaligned_be32(val << (8 * (4 - len)), buf + 2);
> -	if (i2c_master_send(client, buf, len + 2) != len + 2)
> -		return -EIO;
> -
> -	return 0;
> -}
> -
> -/* Write a list of registers */
> -static int imx258_write_regs(struct imx258 *imx258,
> -			     const struct imx258_reg *regs, u32 len)
> -{
> -	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> -	unsigned int i;
> -	int ret;
> -
> -	for (i = 0; i < len; i++) {
> -		ret = imx258_write_reg(imx258, regs[i].address, 1,
> -					regs[i].val);
> -		if (ret) {
> -			dev_err_ratelimited(
> -				&client->dev,
> -				"Failed to write reg 0x%4.4x. error = %d\n",
> -				regs[i].address, ret);
> -
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>  /* Get bayer order based on flip setting. */
>  static u32 imx258_get_format_code(const struct imx258 *imx258)
>  {
> @@ -828,28 +725,20 @@ static int imx258_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  	return 0;
>  }
>  
> -static int imx258_update_digital_gain(struct imx258 *imx258, u32 len, u32 val)
> +static int imx258_update_digital_gain(struct imx258 *imx258, u32 val)
>  {
>  	int ret;
>  
> -	ret = imx258_write_reg(imx258, IMX258_REG_GR_DIGITAL_GAIN,
> -				IMX258_REG_VALUE_16BIT,
> -				val);
> +	ret = cci_write(imx258->regmap, IMX258_REG_GR_DIGITAL_GAIN, val, NULL);
>  	if (ret)
>  		return ret;
> -	ret = imx258_write_reg(imx258, IMX258_REG_GB_DIGITAL_GAIN,
> -				IMX258_REG_VALUE_16BIT,
> -				val);
> +	ret = cci_write(imx258->regmap, IMX258_REG_GB_DIGITAL_GAIN, val, NULL);
>  	if (ret)
>  		return ret;
> -	ret = imx258_write_reg(imx258, IMX258_REG_R_DIGITAL_GAIN,
> -				IMX258_REG_VALUE_16BIT,
> -				val);
> +	ret = cci_write(imx258->regmap, IMX258_REG_R_DIGITAL_GAIN, val, NULL);
>  	if (ret)
>  		return ret;
> -	ret = imx258_write_reg(imx258, IMX258_REG_B_DIGITAL_GAIN,
> -				IMX258_REG_VALUE_16BIT,
> -				val);
> +	ret = cci_write(imx258->regmap, IMX258_REG_B_DIGITAL_GAIN, val, NULL);
>  	if (ret)
>  		return ret;
>  	return 0;

What about reducing the usage of the "if (ret) return ret;" pattern?
You can:

int ret = 0;

cci_write(imx258->regmap, IMX258_REG_GR_DIGITAL_GAIN, val, &ret);
cci_write(imx258->regmap, IMX258_REG_GB_DIGITAL_GAIN, val, &ret);
cci_write(imx258->regmap, IMX258_REG_R_DIGITAL_GAIN, val, &ret);
cci_write(imx258->regmap, IMX258_REG_B_DIGITAL_GAIN, val, &ret);

return ret;

What do you think?


> @@ -891,53 +780,45 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
>  
>  	switch (ctrl->id) {
>  	case V4L2_CID_ANALOGUE_GAIN:
> -		ret = imx258_write_reg(imx258, IMX258_REG_ANALOG_GAIN,
> -				IMX258_REG_VALUE_16BIT,
> -				ctrl->val);
> +		ret = cci_write(imx258->regmap, IMX258_REG_ANALOG_GAIN,
> +				ctrl->val, NULL);
>  		break;
>  	case V4L2_CID_EXPOSURE:
> -		ret = imx258_write_reg(imx258, IMX258_REG_EXPOSURE,
> -				IMX258_REG_VALUE_16BIT,
> -				ctrl->val);
> +		ret = cci_write(imx258->regmap, IMX258_REG_EXPOSURE,
> +				ctrl->val, NULL);
>  		break;
>  	case V4L2_CID_DIGITAL_GAIN:
> -		ret = imx258_update_digital_gain(imx258, IMX258_REG_VALUE_16BIT,
> -				ctrl->val);
> +		ret = imx258_update_digital_gain(imx258, ctrl->val);
>  		break;
>  	case V4L2_CID_TEST_PATTERN:
> -		ret = imx258_write_reg(imx258, IMX258_REG_TEST_PATTERN,
> -				IMX258_REG_VALUE_16BIT,
> -				ctrl->val);
> +		ret = cci_write(imx258->regmap, IMX258_REG_TEST_PATTERN,
> +				ctrl->val, NULL);
>  		break;
>  	case V4L2_CID_WIDE_DYNAMIC_RANGE:
>  		if (!ctrl->val) {
> -			ret = imx258_write_reg(imx258, IMX258_REG_HDR,
> -					       IMX258_REG_VALUE_08BIT,
> -					       IMX258_HDR_RATIO_MIN);
> +			ret = cci_write(imx258->regmap, IMX258_REG_HDR,
> +					IMX258_HDR_RATIO_MIN, NULL);
>  		} else {
> -			ret = imx258_write_reg(imx258, IMX258_REG_HDR,
> -					       IMX258_REG_VALUE_08BIT,
> -					       IMX258_HDR_ON);
> +			ret = cci_write(imx258->regmap, IMX258_REG_HDR,
> +					IMX258_HDR_ON, NULL);
>  			if (ret)
>  				break;
> -			ret = imx258_write_reg(imx258, IMX258_REG_HDR_RATIO,
> -					       IMX258_REG_VALUE_08BIT,
> -					       BIT(IMX258_HDR_RATIO_MAX));
> +			ret = cci_write(imx258->regmap, IMX258_REG_HDR_RATIO,
> +					BIT(IMX258_HDR_RATIO_MAX), NULL);
>  		}
>  		break;
>  	case V4L2_CID_VBLANK:
> -		ret = imx258_write_reg(imx258, IMX258_REG_VTS,
> -				       IMX258_REG_VALUE_16BIT,
> -				       imx258->cur_mode->height + ctrl->val);
> +		ret = cci_write(imx258->regmap, IMX258_REG_FRM_LENGTH_LINES,
> +				imx258->cur_mode->height + ctrl->val, NULL);
>  		break;
>  	case V4L2_CID_VFLIP:
>  	case V4L2_CID_HFLIP:
> -		ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
> -				       IMX258_REG_VALUE_08BIT,
> -				       (imx258->hflip->val ?
> -					REG_CONFIG_MIRROR_HFLIP : 0) |
> -				       (imx258->vflip->val ?
> -					REG_CONFIG_MIRROR_VFLIP : 0));
> +		ret = cci_write(imx258->regmap, REG_MIRROR_FLIP_CONTROL,
> +				(imx258->hflip->val ?
> +				 REG_CONFIG_MIRROR_HFLIP : 0) |
> +				(imx258->vflip->val ?
> +				 REG_CONFIG_MIRROR_VFLIP : 0),
> +				NULL);
>  		break;
>  	default:
>  		dev_info(&client->dev,
> @@ -1147,8 +1028,7 @@ static int imx258_start_streaming(struct imx258 *imx258)
>  	const struct imx258_link_freq_config *link_freq_cfg;
>  	int ret, link_freq_index;
>  
> -	ret = imx258_write_reg(imx258, IMX258_REG_RESET, IMX258_REG_VALUE_08BIT,
> -			       0x01);
> +	ret = cci_write(imx258->regmap, IMX258_REG_RESET, 0x01, NULL);
>  	if (ret) {
>  		dev_err(&client->dev, "%s failed to reset sensor\n", __func__);
>  		return ret;
> @@ -1162,30 +1042,30 @@ static int imx258_start_streaming(struct imx258 *imx258)
>  	link_freq_cfg = &imx258->link_freq_configs[link_freq_index];
>  
>  	reg_list = &link_freq_cfg->link_cfg[imx258->lane_mode_idx].reg_list;
> -	ret = imx258_write_regs(imx258, reg_list->regs, reg_list->num_of_regs);
> +	ret = cci_multi_reg_write(imx258->regmap, reg_list->regs, reg_list->num_of_regs, NULL);
>  	if (ret) {
>  		dev_err(&client->dev, "%s failed to set plls\n", __func__);
>  		return ret;
>  	}
>  
> -	ret = imx258_write_regs(imx258, mode_common_regs,
> -				ARRAY_SIZE(mode_common_regs));
> +	ret = cci_multi_reg_write(imx258->regmap, mode_common_regs,
> +				  ARRAY_SIZE(mode_common_regs), NULL);
>  	if (ret) {
>  		dev_err(&client->dev, "%s failed to set common regs\n", __func__);
>  		return ret;
>  	}
>  
> -	ret = imx258_write_regs(imx258, imx258->variant_cfg->regs,
> -				imx258->variant_cfg->num_regs);
> +	ret = cci_multi_reg_write(imx258->regmap, imx258->variant_cfg->regs,
> +				  imx258->variant_cfg->num_regs, NULL);
>  	if (ret) {
>  		dev_err(&client->dev, "%s failed to set variant config\n",
>  			__func__);
>  		return ret;
>  	}
>  
> -	ret = imx258_write_reg(imx258, IMX258_CLK_BLANK_STOP,
> -			       IMX258_REG_VALUE_08BIT,
> -			       !!(imx258->csi2_flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK));
> +	ret = cci_write(imx258->regmap, IMX258_CLK_BLANK_STOP,
> +			!!(imx258->csi2_flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK),
> +			NULL);
>  	if (ret) {
>  		dev_err(&client->dev, "%s failed to set clock lane mode\n", __func__);
>  		return ret;
> @@ -1193,7 +1073,7 @@ static int imx258_start_streaming(struct imx258 *imx258)
>  
>  	/* Apply default values of current mode */
>  	reg_list = &imx258->cur_mode->reg_list;
> -	ret = imx258_write_regs(imx258, reg_list->regs, reg_list->num_of_regs);
> +	ret = cci_multi_reg_write(imx258->regmap, reg_list->regs, reg_list->num_of_regs, NULL);
>  	if (ret) {
>  		dev_err(&client->dev, "%s failed to set mode\n", __func__);
>  		return ret;
> @@ -1205,9 +1085,8 @@ static int imx258_start_streaming(struct imx258 *imx258)
>  		return ret;
>  
>  	/* set stream on register */
> -	return imx258_write_reg(imx258, IMX258_REG_MODE_SELECT,
> -				IMX258_REG_VALUE_08BIT,
> -				IMX258_MODE_STREAMING);
> +	return cci_write(imx258->regmap, IMX258_REG_MODE_SELECT,
> +			 IMX258_MODE_STREAMING, NULL);
>  }
>  
>  /* Stop streaming */
> @@ -1217,8 +1096,8 @@ static int imx258_stop_streaming(struct imx258 *imx258)
>  	int ret;
>  
>  	/* set stream off register */
> -	ret = imx258_write_reg(imx258, IMX258_REG_MODE_SELECT,
> -		IMX258_REG_VALUE_08BIT, IMX258_MODE_STANDBY);
> +	ret = cci_write(imx258->regmap, IMX258_REG_MODE_SELECT,
> +			IMX258_MODE_STANDBY, NULL);
>  	if (ret)
>  		dev_err(&client->dev, "%s failed to set stream\n", __func__);
>  
> @@ -1316,10 +1195,10 @@ static int imx258_identify_module(struct imx258 *imx258)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
>  	int ret;
> -	u32 val;
> +	u64 val;
>  
> -	ret = imx258_read_reg(imx258, IMX258_REG_CHIP_ID,
> -			      IMX258_REG_VALUE_16BIT, &val);
> +	ret = cci_read(imx258->regmap, IMX258_REG_CHIP_ID,
> +		       &val, NULL);
>  	if (ret) {
>  		dev_err(&client->dev, "failed to read chip id %x\n",
>  			IMX258_CHIP_ID);
> @@ -1327,7 +1206,7 @@ static int imx258_identify_module(struct imx258 *imx258)
>  	}
>  
>  	if (val != IMX258_CHIP_ID) {
> -		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
> +		dev_err(&client->dev, "chip id mismatch: %x!=%llx\n",
>  			IMX258_CHIP_ID, val);
>  		return -EIO;
>  	}
> @@ -1507,6 +1386,13 @@ static int imx258_probe(struct i2c_client *client)
>  	if (!imx258)
>  		return -ENOMEM;
>  
> +	imx258->regmap = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(imx258->regmap)) {
> +		ret = PTR_ERR(imx258->regmap);
> +		dev_err(&client->dev, "failed to initialize CCI: %d\n", ret);
> +		return ret;
> +	}
> +
>  	ret = imx258_get_regulators(imx258, client);
>  	if (ret)
>  		return dev_err_probe(&client->dev, ret,
> -- 
> 2.44.0

The rest looks good to me.
Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>

Thanks & Regards,
Tommaso

> 
>
Tommaso Merciai April 15, 2024, 3:18 p.m. UTC | #3 
Hi Luis,

On Sun, Apr 14, 2024 at 02:35:02PM -0600, git@luigi311.com wrote:
> From: Luis Garcia <git@luigi311.com>
> 
> Use the v4l2_link_freq_to_bitmap() helper to figure out which
> driver-supported link freq can be used on a given system.
> 
> Signed-off-by: Luis Garcia <git@luigi311.com>
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> ---
>  drivers/media/i2c/imx258.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 5de71cb7c1ae..65846dff775e 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -693,6 +693,7 @@ struct imx258 {
>  	/* Current mode */
>  	const struct imx258_mode *cur_mode;
>  
> +	unsigned long link_freq_bitmap;
>  	const struct imx258_link_freq_config *link_freq_configs;
>  	const s64 *link_freq_menu_items;
>  	unsigned int lane_mode_idx;
> @@ -1552,6 +1553,17 @@ static int imx258_probe(struct i2c_client *client)
>  		return ret;
>  	}
>  
> +	ret = v4l2_link_freq_to_bitmap(&client->dev,
> +				       ep.link_frequencies,
> +				       ep.nr_of_link_frequencies,
> +				       imx258->link_freq_menu_items,
> +				       ARRAY_SIZE(link_freq_menu_items_19_2),
> +				       &imx258->link_freq_bitmap);
> +	if (ret) {
> +		dev_err(&client->dev, "Link frequency not supported\n");
> +		goto error_endpoint_free;
> +	}
> +
>  	/* Get number of data lanes */
>  	switch (ep.bus.mipi_csi2.num_data_lanes) {
>  	case 2:

Looks good to me.

ps:
Maybe a good plan for the future would be to use: dev_err_probe
(instead of dev_err into probe function)

But this I think is somenthing for next improvements. :)

Thanks & Regards,
Tommaso

> -- 
> 2.44.0
> 
>
Luis Garcia April 15, 2024, 3:29 p.m. UTC | #4 
On 4/15/24 08:47, Tommaso Merciai wrote:
> Hi Luis,
> 
> On Sun, Apr 14, 2024 at 02:35:03PM -0600, git@luigi311.com wrote:
>> From: Luis Garcia <git@luigi311.com>
>>
>> Use the new comon CCI register access helpers to replace the private
>> register access helpers in the imx258 driver.
>>
>> Signed-off-by: Luis Garcia <git@luigi311.com>
>> ---
>>  drivers/media/i2c/Kconfig  |   1 +
>>  drivers/media/i2c/imx258.c | 804 ++++++++++++++++---------------------
>>  2 files changed, 346 insertions(+), 459 deletions(-)
>>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index 56f276b920ab..6707b0c3c4eb 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -139,6 +139,7 @@ config VIDEO_IMX219
>>  
>>  config VIDEO_IMX258
>>  	tristate "Sony IMX258 sensor support"
>> +	select V4L2_CCI_I2C
>>  	help
>>  	  This is a Video4Linux2 sensor driver for the Sony
>>  	  IMX258 camera.
>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>> index 65846dff775e..8fc9750e5ec9 100644
>> --- a/drivers/media/i2c/imx258.c
>> +++ b/drivers/media/i2c/imx258.c
>> @@ -8,22 +8,20 @@
>>  #include <linux/module.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/regulator/consumer.h>
>> +#include <media/v4l2-cci.h>
>>  #include <media/v4l2-ctrls.h>
>>  #include <media/v4l2-device.h>
>>  #include <media/v4l2-fwnode.h>
>>  #include <asm/unaligned.h>
>>  
>> -#define IMX258_REG_VALUE_08BIT		1
>> -#define IMX258_REG_VALUE_16BIT		2
>> -
>> -#define IMX258_REG_MODE_SELECT		0x0100
>> +#define IMX258_REG_MODE_SELECT		CCI_REG8(0x0100)
>>  #define IMX258_MODE_STANDBY		0x00
>>  #define IMX258_MODE_STREAMING		0x01
>>  
>> -#define IMX258_REG_RESET		0x0103
>> +#define IMX258_REG_RESET		CCI_REG8(0x0103)
>>  
>>  /* Chip ID */
>> -#define IMX258_REG_CHIP_ID		0x0016
>> +#define IMX258_REG_CHIP_ID		CCI_REG16(0x0016)
>>  #define IMX258_CHIP_ID			0x0258
>>  
>>  /* V_TIMING internal */
>> @@ -32,13 +30,11 @@
>>  #define IMX258_VTS_30FPS_VGA		0x034c
>>  #define IMX258_VTS_MAX			65525
>>  
>> -#define IMX258_REG_VTS			0x0340
>> -
>>  /* HBLANK control - read only */
>>  #define IMX258_PPL_DEFAULT		5352
>>  
>>  /* Exposure control */
>> -#define IMX258_REG_EXPOSURE		0x0202
>> +#define IMX258_REG_EXPOSURE		CCI_REG16(0x0202)
>>  #define IMX258_EXPOSURE_OFFSET		10
>>  #define IMX258_EXPOSURE_MIN		4
>>  #define IMX258_EXPOSURE_STEP		1
>> @@ -46,38 +42,38 @@
>>  #define IMX258_EXPOSURE_MAX		(IMX258_VTS_MAX - IMX258_EXPOSURE_OFFSET)
>>  
>>  /* Analog gain control */
>> -#define IMX258_REG_ANALOG_GAIN		0x0204
>> +#define IMX258_REG_ANALOG_GAIN		CCI_REG16(0x0204)
>>  #define IMX258_ANA_GAIN_MIN		0
>>  #define IMX258_ANA_GAIN_MAX		480
>>  #define IMX258_ANA_GAIN_STEP		1
>>  #define IMX258_ANA_GAIN_DEFAULT		0x0
>>  
>>  /* Digital gain control */
>> -#define IMX258_REG_GR_DIGITAL_GAIN	0x020e
>> -#define IMX258_REG_R_DIGITAL_GAIN	0x0210
>> -#define IMX258_REG_B_DIGITAL_GAIN	0x0212
>> -#define IMX258_REG_GB_DIGITAL_GAIN	0x0214
>> +#define IMX258_REG_GR_DIGITAL_GAIN	CCI_REG16(0x020e)
>> +#define IMX258_REG_R_DIGITAL_GAIN	CCI_REG16(0x0210)
>> +#define IMX258_REG_B_DIGITAL_GAIN	CCI_REG16(0x0212)
>> +#define IMX258_REG_GB_DIGITAL_GAIN	CCI_REG16(0x0214)
>>  #define IMX258_DGTL_GAIN_MIN		0
>>  #define IMX258_DGTL_GAIN_MAX		4096	/* Max = 0xFFF */
>>  #define IMX258_DGTL_GAIN_DEFAULT	1024
>>  #define IMX258_DGTL_GAIN_STEP		1
>>  
>>  /* HDR control */
>> -#define IMX258_REG_HDR			0x0220
>> +#define IMX258_REG_HDR			CCI_REG8(0x0220)
>>  #define IMX258_HDR_ON			BIT(0)
>> -#define IMX258_REG_HDR_RATIO		0x0222
>> +#define IMX258_REG_HDR_RATIO		CCI_REG8(0x0222)
>>  #define IMX258_HDR_RATIO_MIN		0
>>  #define IMX258_HDR_RATIO_MAX		5
>>  #define IMX258_HDR_RATIO_STEP		1
>>  #define IMX258_HDR_RATIO_DEFAULT	0x0
>>  
>>  /* Test Pattern Control */
>> -#define IMX258_REG_TEST_PATTERN		0x0600
>> +#define IMX258_REG_TEST_PATTERN		CCI_REG16(0x0600)
>>  
>> -#define IMX258_CLK_BLANK_STOP		0x4040
>> +#define IMX258_CLK_BLANK_STOP		CCI_REG8(0x4040)
>>  
>>  /* Orientation */
>> -#define REG_MIRROR_FLIP_CONTROL		0x0101
>> +#define REG_MIRROR_FLIP_CONTROL		CCI_REG8(0x0101)
>>  #define REG_CONFIG_MIRROR_HFLIP		0x01
>>  #define REG_CONFIG_MIRROR_VFLIP		0x02
>>  
>> @@ -89,14 +85,53 @@
>>  #define IMX258_PIXEL_ARRAY_WIDTH	4208U
>>  #define IMX258_PIXEL_ARRAY_HEIGHT	3120U
>>  
>> -struct imx258_reg {
>> -	u16 address;
>> -	u8 val;
>> -};
>> +/* regs */
>> +#define IMX258_REG_PLL_MULT_DRIV                  CCI_REG8(0x0310)
>> +#define IMX258_REG_IVTPXCK_DIV                    CCI_REG8(0x0301)
>> +#define IMX258_REG_IVTSYCK_DIV                    CCI_REG8(0x0303)
>> +#define IMX258_REG_PREPLLCK_VT_DIV                CCI_REG8(0x0305)
>> +#define IMX258_REG_IOPPXCK_DIV                    CCI_REG8(0x0309)
>> +#define IMX258_REG_IOPSYCK_DIV                    CCI_REG8(0x030b)
>> +#define IMX258_REG_PREPLLCK_OP_DIV                CCI_REG8(0x030d)
>> +#define IMX258_REG_PHASE_PIX_OUTEN                CCI_REG8(0x3030)
>> +#define IMX258_REG_PDPIX_DATA_RATE                CCI_REG8(0x3032)
>> +#define IMX258_REG_SCALE_MODE                     CCI_REG8(0x0401)
>> +#define IMX258_REG_SCALE_MODE_EXT                 CCI_REG8(0x3038)
>> +#define IMX258_REG_AF_WINDOW_MODE                 CCI_REG8(0x7bcd)
>> +#define IMX258_REG_FRM_LENGTH_CTL                 CCI_REG8(0x0350)
>> +#define IMX258_REG_CSI_LANE_MODE                  CCI_REG8(0x0114)
>> +#define IMX258_REG_X_EVN_INC                      CCI_REG8(0x0381)
>> +#define IMX258_REG_X_ODD_INC                      CCI_REG8(0x0383)
>> +#define IMX258_REG_Y_EVN_INC                      CCI_REG8(0x0385)
>> +#define IMX258_REG_Y_ODD_INC                      CCI_REG8(0x0387)
>> +#define IMX258_REG_BINNING_MODE                   CCI_REG8(0x0900)
>> +#define IMX258_REG_BINNING_TYPE_V                 CCI_REG8(0x0901)
>> +#define IMX258_REG_FORCE_FD_SUM                   CCI_REG8(0x300d)
>> +#define IMX258_REG_DIG_CROP_X_OFFSET              CCI_REG16(0x0408)
>> +#define IMX258_REG_DIG_CROP_Y_OFFSET              CCI_REG16(0x040a)
>> +#define IMX258_REG_DIG_CROP_IMAGE_WIDTH           CCI_REG16(0x040c)
>> +#define IMX258_REG_DIG_CROP_IMAGE_HEIGHT          CCI_REG16(0x040e)
>> +#define IMX258_REG_SCALE_M                        CCI_REG16(0x0404)
>> +#define IMX258_REG_X_OUT_SIZE                     CCI_REG16(0x034c)
>> +#define IMX258_REG_Y_OUT_SIZE                     CCI_REG16(0x034e)
>> +#define IMX258_REG_X_ADD_STA                      CCI_REG16(0x0344)
>> +#define IMX258_REG_Y_ADD_STA                      CCI_REG16(0x0346)
>> +#define IMX258_REG_X_ADD_END                      CCI_REG16(0x0348)
>> +#define IMX258_REG_Y_ADD_END                      CCI_REG16(0x034a)
>> +#define IMX258_REG_EXCK_FREQ                      CCI_REG16(0x0136)
>> +#define IMX258_REG_CSI_DT_FMT                     CCI_REG16(0x0112)
>> +#define IMX258_REG_LINE_LENGTH_PCK                CCI_REG16(0x0342)
>> +#define IMX258_REG_SCALE_M_EXT                    CCI_REG16(0x303a)
>> +#define IMX258_REG_FRM_LENGTH_LINES               CCI_REG16(0x0340)
>> +#define IMX258_REG_FINE_INTEG_TIME                CCI_REG8(0x0200)
>> +#define IMX258_REG_PLL_IVT_MPY                    CCI_REG16(0x0306)
>> +#define IMX258_REG_PLL_IOP_MPY                    CCI_REG16(0x030e)
>> +#define IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H       CCI_REG16(0x0820)
>> +#define IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L       CCI_REG16(0x0822)
>>  
>>  struct imx258_reg_list {
>>  	u32 num_of_regs;
>> -	const struct imx258_reg *regs;
>> +	const struct cci_reg_sequence *regs;
>>  };
>>  
>>  struct imx258_link_cfg {
>> @@ -143,329 +178,264 @@ struct imx258_mode {
>>   * To avoid further computation of clock settings, adopt the same per
>>   * lane data rate when using 2 lanes, thus allowing a maximum of 15fps.
>>   */
>> -static const struct imx258_reg mipi_1267mbps_19_2mhz_2l[] = {
>> -	{ 0x0136, 0x13 },
>> -	{ 0x0137, 0x33 },
>> -	{ 0x0301, 0x0A },
>> -	{ 0x0303, 0x02 },
>> -	{ 0x0305, 0x03 },
>> -	{ 0x0306, 0x00 },
>> -	{ 0x0307, 0xC6 },
>> -	{ 0x0309, 0x0A },
>> -	{ 0x030B, 0x01 },
>> -	{ 0x030D, 0x02 },
>> -	{ 0x030E, 0x00 },
>> -	{ 0x030F, 0xD8 },
>> -	{ 0x0310, 0x00 },
>> -
>> -	{ 0x0114, 0x01 },
>> -	{ 0x0820, 0x09 },
>> -	{ 0x0821, 0xa6 },
>> -	{ 0x0822, 0x66 },
>> -	{ 0x0823, 0x66 },
>> +static const struct cci_reg_sequence mipi_1267mbps_19_2mhz_2l[] = {
>> +	{ IMX258_REG_EXCK_FREQ, 0x1333 },
>> +	{ IMX258_REG_IVTPXCK_DIV, 10 },
>> +	{ IMX258_REG_IVTSYCK_DIV, 2 },
>> +	{ IMX258_REG_PREPLLCK_VT_DIV, 3 },
>> +	{ IMX258_REG_PLL_IVT_MPY, 198 },
>> +	{ IMX258_REG_IOPPXCK_DIV, 10 },
>> +	{ IMX258_REG_IOPSYCK_DIV, 1 },
>> +	{ IMX258_REG_PREPLLCK_OP_DIV, 2 },
>> +	{ IMX258_REG_PLL_IOP_MPY, 216 },
>> +	{ IMX258_REG_PLL_MULT_DRIV, 0 },
>> +
>> +	{ IMX258_REG_CSI_LANE_MODE, 1 },
>> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 1267 * 2 },
>> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0 },
>>  };
>>  
>> -static const struct imx258_reg mipi_1267mbps_19_2mhz_4l[] = {
>> -	{ 0x0136, 0x13 },
>> -	{ 0x0137, 0x33 },
>> -	{ 0x0301, 0x05 },
>> -	{ 0x0303, 0x02 },
>> -	{ 0x0305, 0x03 },
>> -	{ 0x0306, 0x00 },
>> -	{ 0x0307, 0xC6 },
>> -	{ 0x0309, 0x0A },
>> -	{ 0x030B, 0x01 },
>> -	{ 0x030D, 0x02 },
>> -	{ 0x030E, 0x00 },
>> -	{ 0x030F, 0xD8 },
>> -	{ 0x0310, 0x00 },
>> -
>> -	{ 0x0114, 0x03 },
>> -	{ 0x0820, 0x13 },
>> -	{ 0x0821, 0x4C },
>> -	{ 0x0822, 0xCC },
>> -	{ 0x0823, 0xCC },
>> +static const struct cci_reg_sequence mipi_1267mbps_19_2mhz_4l[] = {
>> +	{ IMX258_REG_EXCK_FREQ, 0x1333 },
>> +	{ IMX258_REG_IVTPXCK_DIV, 5 },
>> +	{ IMX258_REG_IVTSYCK_DIV, 2 },
>> +	{ IMX258_REG_PREPLLCK_VT_DIV, 3 },
>> +	{ IMX258_REG_PLL_IVT_MPY, 198 },
>> +	{ IMX258_REG_IOPPXCK_DIV, 10 },
>> +	{ IMX258_REG_IOPSYCK_DIV, 1 },
>> +	{ IMX258_REG_PREPLLCK_OP_DIV, 2 },
>> +	{ IMX258_REG_PLL_IOP_MPY, 216 },
>> +	{ IMX258_REG_PLL_MULT_DRIV, 0 },
>> +
>> +	{ IMX258_REG_CSI_LANE_MODE, 3 },
>> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 1267 * 4 },
>> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0 },
>>  };
>>  
>> -static const struct imx258_reg mipi_1272mbps_24mhz_2l[] = {
>> -	{ 0x0136, 0x18 },
>> -	{ 0x0137, 0x00 },
>> -	{ 0x0301, 0x0a },
>> -	{ 0x0303, 0x02 },
>> -	{ 0x0305, 0x04 },
>> -	{ 0x0306, 0x00 },
>> -	{ 0x0307, 0xD4 },
>> -	{ 0x0309, 0x0A },
>> -	{ 0x030B, 0x01 },
>> -	{ 0x030D, 0x02 },
>> -	{ 0x030E, 0x00 },
>> -	{ 0x030F, 0xD8 },
>> -	{ 0x0310, 0x00 },
>> -
>> -	{ 0x0114, 0x01 },
>> -	{ 0x0820, 0x13 },
>> -	{ 0x0821, 0x4C },
>> -	{ 0x0822, 0xCC },
>> -	{ 0x0823, 0xCC },
>> +static const struct cci_reg_sequence mipi_1272mbps_24mhz_2l[] = {
>> +	{ IMX258_REG_EXCK_FREQ, 0x1800 },
>> +	{ IMX258_REG_IVTPXCK_DIV, 10 },
>> +	{ IMX258_REG_IVTSYCK_DIV, 2 },
>> +	{ IMX258_REG_PREPLLCK_VT_DIV, 4 },
>> +	{ IMX258_REG_PLL_IVT_MPY, 212 },
>> +	{ IMX258_REG_IOPPXCK_DIV, 10 },
>> +	{ IMX258_REG_IOPSYCK_DIV, 1 },
>> +	{ IMX258_REG_PREPLLCK_OP_DIV, 2 },
>> +	{ IMX258_REG_PLL_IOP_MPY, 216 },
>> +	{ IMX258_REG_PLL_MULT_DRIV, 0 },
>> +
>> +	{ IMX258_REG_CSI_LANE_MODE, 1 },
>> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 1272 * 2 },
>> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0 },
>>  };
>>  
>> -static const struct imx258_reg mipi_1272mbps_24mhz_4l[] = {
>> -	{ 0x0136, 0x18 },
>> -	{ 0x0137, 0x00 },
>> -	{ 0x0301, 0x05 },
>> -	{ 0x0303, 0x02 },
>> -	{ 0x0305, 0x04 },
>> -	{ 0x0306, 0x00 },
>> -	{ 0x0307, 0xD4 },
>> -	{ 0x0309, 0x0A },
>> -	{ 0x030B, 0x01 },
>> -	{ 0x030D, 0x02 },
>> -	{ 0x030E, 0x00 },
>> -	{ 0x030F, 0xD8 },
>> -	{ 0x0310, 0x00 },
>> -
>> -	{ 0x0114, 0x03 },
>> -	{ 0x0820, 0x13 },
>> -	{ 0x0821, 0xE0 },
>> -	{ 0x0822, 0x00 },
>> -	{ 0x0823, 0x00 },
>> +static const struct cci_reg_sequence mipi_1272mbps_24mhz_4l[] = {
>> +	{ IMX258_REG_EXCK_FREQ, 0x1800 },
>> +	{ IMX258_REG_IVTPXCK_DIV, 5 },
>> +	{ IMX258_REG_IVTSYCK_DIV, 2 },
>> +	{ IMX258_REG_PREPLLCK_VT_DIV, 4 },
>> +	{ IMX258_REG_PLL_IVT_MPY, 212 },
>> +	{ IMX258_REG_IOPPXCK_DIV, 10 },
>> +	{ IMX258_REG_IOPSYCK_DIV, 1 },
>> +	{ IMX258_REG_PREPLLCK_OP_DIV, 2 },
>> +	{ IMX258_REG_PLL_IOP_MPY, 216 },
>> +	{ IMX258_REG_PLL_MULT_DRIV, 0 },
>> +
>> +	{ IMX258_REG_CSI_LANE_MODE, 3 },
>> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 1272 * 4 },
>> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0 },
>>  };
>>  
>> -static const struct imx258_reg mipi_640mbps_19_2mhz_2l[] = {
>> -	{ 0x0136, 0x13 },
>> -	{ 0x0137, 0x33 },
>> -	{ 0x0301, 0x05 },
>> -	{ 0x0303, 0x02 },
>> -	{ 0x0305, 0x03 },
>> -	{ 0x0306, 0x00 },
>> -	{ 0x0307, 0x64 },
>> -	{ 0x0309, 0x0A },
>> -	{ 0x030B, 0x01 },
>> -	{ 0x030D, 0x02 },
>> -	{ 0x030E, 0x00 },
>> -	{ 0x030F, 0xD8 },
>> -	{ 0x0310, 0x00 },
>> -
>> -	{ 0x0114, 0x01 },
>> -	{ 0x0820, 0x05 },
>> -	{ 0x0821, 0x00 },
>> -	{ 0x0822, 0x00 },
>> -	{ 0x0823, 0x00 },
>> +static const struct cci_reg_sequence mipi_640mbps_19_2mhz_2l[] = {
>> +	{ IMX258_REG_EXCK_FREQ, 0x1333 },
>> +	{ IMX258_REG_IVTPXCK_DIV, 5 },
>> +	{ IMX258_REG_IVTSYCK_DIV, 2 },
>> +	{ IMX258_REG_PREPLLCK_VT_DIV, 3 },
>> +	{ IMX258_REG_PLL_IVT_MPY, 100 },
>> +	{ IMX258_REG_IOPPXCK_DIV, 10 },
>> +	{ IMX258_REG_IOPSYCK_DIV, 1 },
>> +	{ IMX258_REG_PREPLLCK_OP_DIV, 2 },
>> +	{ IMX258_REG_PLL_IOP_MPY, 216 },
>> +	{ IMX258_REG_PLL_MULT_DRIV, 0 },
>> +
>> +	{ IMX258_REG_CSI_LANE_MODE, 1 },
>> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 640 * 2 },
>> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0 },
>>  };
>>  
>> -static const struct imx258_reg mipi_640mbps_19_2mhz_4l[] = {
>> -	{ 0x0136, 0x13 },
>> -	{ 0x0137, 0x33 },
>> -	{ 0x0301, 0x05 },
>> -	{ 0x0303, 0x02 },
>> -	{ 0x0305, 0x03 },
>> -	{ 0x0306, 0x00 },
>> -	{ 0x0307, 0x64 },
>> -	{ 0x0309, 0x0A },
>> -	{ 0x030B, 0x01 },
>> -	{ 0x030D, 0x02 },
>> -	{ 0x030E, 0x00 },
>> -	{ 0x030F, 0xD8 },
>> -	{ 0x0310, 0x00 },
>> -
>> -	{ 0x0114, 0x03 },
>> -	{ 0x0820, 0x0A },
>> -	{ 0x0821, 0x00 },
>> -	{ 0x0822, 0x00 },
>> -	{ 0x0823, 0x00 },
>> +static const struct cci_reg_sequence mipi_640mbps_19_2mhz_4l[] = {
>> +	{ IMX258_REG_EXCK_FREQ, 0x1333 },
>> +	{ IMX258_REG_IVTPXCK_DIV, 5 },
>> +	{ IMX258_REG_IVTSYCK_DIV, 2 },
>> +	{ IMX258_REG_PREPLLCK_VT_DIV, 3 },
>> +	{ IMX258_REG_PLL_IVT_MPY, 100 },
>> +	{ IMX258_REG_IOPPXCK_DIV, 10 },
>> +	{ IMX258_REG_IOPSYCK_DIV, 1 },
>> +	{ IMX258_REG_PREPLLCK_OP_DIV, 2 },
>> +	{ IMX258_REG_PLL_IOP_MPY, 216 },
>> +	{ IMX258_REG_PLL_MULT_DRIV, 0 },
>> +
>> +	{ IMX258_REG_CSI_LANE_MODE, 3 },
>> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 640 * 4 },
>> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0 },
>>  };
>>  
>> -static const struct imx258_reg mipi_642mbps_24mhz_2l[] = {
>> -	{ 0x0136, 0x18 },
>> -	{ 0x0137, 0x00 },
>> -	{ 0x0301, 0x05 },
>> -	{ 0x0303, 0x02 },
>> -	{ 0x0305, 0x04 },
>> -	{ 0x0306, 0x00 },
>> -	{ 0x0307, 0x6B },
>> -	{ 0x0309, 0x0A },
>> -	{ 0x030B, 0x01 },
>> -	{ 0x030D, 0x02 },
>> -	{ 0x030E, 0x00 },
>> -	{ 0x030F, 0xD8 },
>> -	{ 0x0310, 0x00 },
>> -
>> -	{ 0x0114, 0x01 },
>> -	{ 0x0820, 0x0A },
>> -	{ 0x0821, 0x00 },
>> -	{ 0x0822, 0x00 },
>> -	{ 0x0823, 0x00 },
>> +static const struct cci_reg_sequence mipi_642mbps_24mhz_2l[] = {
>> +	{ IMX258_REG_EXCK_FREQ, 0x1800 },
>> +	{ IMX258_REG_IVTPXCK_DIV, 5 },
>> +	{ IMX258_REG_IVTSYCK_DIV, 2 },
>> +	{ IMX258_REG_PREPLLCK_VT_DIV, 4 },
>> +	{ IMX258_REG_PLL_IVT_MPY, 107 },
>> +	{ IMX258_REG_IOPPXCK_DIV, 10 },
>> +	{ IMX258_REG_IOPSYCK_DIV, 1 },
>> +	{ IMX258_REG_PREPLLCK_OP_DIV, 2 },
>> +	{ IMX258_REG_PLL_IOP_MPY, 216 },
>> +	{ IMX258_REG_PLL_MULT_DRIV, 0 },
>> +
>> +	{ IMX258_REG_CSI_LANE_MODE, 1 },
>> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 642 * 2 },
>> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0 },
>>  };
>>  
>> -static const struct imx258_reg mipi_642mbps_24mhz_4l[] = {
>> -	{ 0x0136, 0x18 },
>> -	{ 0x0137, 0x00 },
>> -	{ 0x0301, 0x05 },
>> -	{ 0x0303, 0x02 },
>> -	{ 0x0305, 0x04 },
>> -	{ 0x0306, 0x00 },
>> -	{ 0x0307, 0x6B },
>> -	{ 0x0309, 0x0A },
>> -	{ 0x030B, 0x01 },
>> -	{ 0x030D, 0x02 },
>> -	{ 0x030E, 0x00 },
>> -	{ 0x030F, 0xD8 },
>> -	{ 0x0310, 0x00 },
>> -
>> -	{ 0x0114, 0x03 },
>> -	{ 0x0820, 0x0A },
>> -	{ 0x0821, 0x00 },
>> -	{ 0x0822, 0x00 },
>> -	{ 0x0823, 0x00 },
>> +static const struct cci_reg_sequence mipi_642mbps_24mhz_4l[] = {
>> +	{ IMX258_REG_EXCK_FREQ, 0x1800 },
>> +	{ IMX258_REG_IVTPXCK_DIV, 5 },
>> +	{ IMX258_REG_IVTSYCK_DIV, 2 },
>> +	{ IMX258_REG_PREPLLCK_VT_DIV, 4 },
>> +	{ IMX258_REG_PLL_IVT_MPY, 107 },
>> +	{ IMX258_REG_IOPPXCK_DIV, 10 },
>> +	{ IMX258_REG_IOPSYCK_DIV, 1 },
>> +	{ IMX258_REG_PREPLLCK_OP_DIV, 2 },
>> +	{ IMX258_REG_PLL_IOP_MPY, 216 },
>> +	{ IMX258_REG_PLL_MULT_DRIV, 0 },
>> +
>> +	{ IMX258_REG_CSI_LANE_MODE, 3 },
>> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 642 * 4 },
>> +	{ IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0 },
>>  };
>>  
>> -static const struct imx258_reg mode_common_regs[] = {
>> -	{ 0x3051, 0x00 },
>> -	{ 0x6B11, 0xCF },
>> -	{ 0x7FF0, 0x08 },
>> -	{ 0x7FF1, 0x0F },
>> -	{ 0x7FF2, 0x08 },
>> -	{ 0x7FF3, 0x1B },
>> -	{ 0x7FF4, 0x23 },
>> -	{ 0x7FF5, 0x60 },
>> -	{ 0x7FF6, 0x00 },
>> -	{ 0x7FF7, 0x01 },
>> -	{ 0x7FF8, 0x00 },
>> -	{ 0x7FF9, 0x78 },
>> -	{ 0x7FFA, 0x00 },
>> -	{ 0x7FFB, 0x00 },
>> -	{ 0x7FFC, 0x00 },
>> -	{ 0x7FFD, 0x00 },
>> -	{ 0x7FFE, 0x00 },
>> -	{ 0x7FFF, 0x03 },
>> -	{ 0x7F76, 0x03 },
>> -	{ 0x7F77, 0xFE },
>> -	{ 0x7FA8, 0x03 },
>> -	{ 0x7FA9, 0xFE },
>> -	{ 0x7B24, 0x81 },
>> -	{ 0x6564, 0x07 },
>> -	{ 0x6B0D, 0x41 },
>> -	{ 0x653D, 0x04 },
>> -	{ 0x6B05, 0x8C },
>> -	{ 0x6B06, 0xF9 },
>> -	{ 0x6B08, 0x65 },
>> -	{ 0x6B09, 0xFC },
>> -	{ 0x6B0A, 0xCF },
>> -	{ 0x6B0B, 0xD2 },
>> -	{ 0x6700, 0x0E },
>> -	{ 0x6707, 0x0E },
>> -	{ 0x9104, 0x00 },
>> -	{ 0x4648, 0x7F },
>> -	{ 0x7420, 0x00 },
>> -	{ 0x7421, 0x1C },
>> -	{ 0x7422, 0x00 },
>> -	{ 0x7423, 0xD7 },
>> -	{ 0x5F04, 0x00 },
>> -	{ 0x5F05, 0xED },
>> -	{ 0x0112, 0x0A },
>> -	{ 0x0113, 0x0A },
>> -	{ 0x0342, 0x14 },
>> -	{ 0x0343, 0xE8 },
>> -	{ 0x0344, 0x00 },
>> -	{ 0x0345, 0x00 },
>> -	{ 0x0346, 0x00 },
>> -	{ 0x0347, 0x00 },
>> -	{ 0x0348, 0x10 },
>> -	{ 0x0349, 0x6F },
>> -	{ 0x034A, 0x0C },
>> -	{ 0x034B, 0x2F },
>> -	{ 0x0381, 0x01 },
>> -	{ 0x0383, 0x01 },
>> -	{ 0x0385, 0x01 },
>> -	{ 0x0387, 0x01 },
>> -	{ 0x0404, 0x00 },
>> -	{ 0x0408, 0x00 },
>> -	{ 0x0409, 0x00 },
>> -	{ 0x040A, 0x00 },
>> -	{ 0x040B, 0x00 },
>> -	{ 0x040C, 0x10 },
>> -	{ 0x040D, 0x70 },
>> -	{ 0x3038, 0x00 },
>> -	{ 0x303A, 0x00 },
>> -	{ 0x303B, 0x10 },
>> -	{ 0x300D, 0x00 },
>> -	{ 0x0350, 0x00 },
>> -	{ 0x0204, 0x00 },
>> -	{ 0x0205, 0x00 },
>> -	{ 0x020E, 0x01 },
>> -	{ 0x020F, 0x00 },
>> -	{ 0x0210, 0x01 },
>> -	{ 0x0211, 0x00 },
>> -	{ 0x0212, 0x01 },
>> -	{ 0x0213, 0x00 },
>> -	{ 0x0214, 0x01 },
>> -	{ 0x0215, 0x00 },
>> -	{ 0x7BCD, 0x00 },
>> -	{ 0x94DC, 0x20 },
>> -	{ 0x94DD, 0x20 },
>> -	{ 0x94DE, 0x20 },
>> -	{ 0x95DC, 0x20 },
>> -	{ 0x95DD, 0x20 },
>> -	{ 0x95DE, 0x20 },
>> -	{ 0x7FB0, 0x00 },
>> -	{ 0x9010, 0x3E },
>> -	{ 0x9419, 0x50 },
>> -	{ 0x941B, 0x50 },
>> -	{ 0x9519, 0x50 },
>> -	{ 0x951B, 0x50 },
>> -	{ 0x3030, 0x00 },
>> -	{ 0x3032, 0x00 },
>> -	{ 0x0220, 0x00 },
>> +static const struct cci_reg_sequence mode_common_regs[] = {
>> +	{ CCI_REG8(0x3051), 0x00 },
>> +	{ CCI_REG8(0x6B11), 0xCF },
>> +	{ CCI_REG8(0x7FF0), 0x08 },
>> +	{ CCI_REG8(0x7FF1), 0x0F },
>> +	{ CCI_REG8(0x7FF2), 0x08 },
>> +	{ CCI_REG8(0x7FF3), 0x1B },
>> +	{ CCI_REG8(0x7FF4), 0x23 },
>> +	{ CCI_REG8(0x7FF5), 0x60 },
>> +	{ CCI_REG8(0x7FF6), 0x00 },
>> +	{ CCI_REG8(0x7FF7), 0x01 },
>> +	{ CCI_REG8(0x7FF8), 0x00 },
>> +	{ CCI_REG8(0x7FF9), 0x78 },
>> +	{ CCI_REG8(0x7FFA), 0x00 },
>> +	{ CCI_REG8(0x7FFB), 0x00 },
>> +	{ CCI_REG8(0x7FFC), 0x00 },
>> +	{ CCI_REG8(0x7FFD), 0x00 },
>> +	{ CCI_REG8(0x7FFE), 0x00 },
>> +	{ CCI_REG8(0x7FFF), 0x03 },
>> +	{ CCI_REG8(0x7F76), 0x03 },
>> +	{ CCI_REG8(0x7F77), 0xFE },
>> +	{ CCI_REG8(0x7FA8), 0x03 },
>> +	{ CCI_REG8(0x7FA9), 0xFE },
>> +	{ CCI_REG8(0x7B24), 0x81 },
>> +	{ CCI_REG8(0x6564), 0x07 },
>> +	{ CCI_REG8(0x6B0D), 0x41 },
>> +	{ CCI_REG8(0x653D), 0x04 },
>> +	{ CCI_REG8(0x6B05), 0x8C },
>> +	{ CCI_REG8(0x6B06), 0xF9 },
>> +	{ CCI_REG8(0x6B08), 0x65 },
>> +	{ CCI_REG8(0x6B09), 0xFC },
>> +	{ CCI_REG8(0x6B0A), 0xCF },
>> +	{ CCI_REG8(0x6B0B), 0xD2 },
>> +	{ CCI_REG8(0x6700), 0x0E },
>> +	{ CCI_REG8(0x6707), 0x0E },
>> +	{ CCI_REG8(0x9104), 0x00 },
>> +	{ CCI_REG8(0x4648), 0x7F },
>> +	{ CCI_REG8(0x7420), 0x00 },
>> +	{ CCI_REG8(0x7421), 0x1C },
>> +	{ CCI_REG8(0x7422), 0x00 },
>> +	{ CCI_REG8(0x7423), 0xD7 },
>> +	{ CCI_REG8(0x5F04), 0x00 },
>> +	{ CCI_REG8(0x5F05), 0xED },
>> +	{IMX258_REG_CSI_DT_FMT, 0x0a0a},
>> +	{IMX258_REG_LINE_LENGTH_PCK, 5352},
>> +	{IMX258_REG_X_ADD_STA, 0},
>> +	{IMX258_REG_Y_ADD_STA, 0},
>> +	{IMX258_REG_X_ADD_END, 4207},
>> +	{IMX258_REG_Y_ADD_END, 3119},
>> +	{IMX258_REG_X_EVN_INC, 1},
>> +	{IMX258_REG_X_ODD_INC, 1},
>> +	{IMX258_REG_Y_EVN_INC, 1},
>> +	{IMX258_REG_Y_ODD_INC, 1},
>> +	{IMX258_REG_DIG_CROP_X_OFFSET, 0},
>> +	{IMX258_REG_DIG_CROP_Y_OFFSET, 0},
>> +	{IMX258_REG_DIG_CROP_IMAGE_WIDTH, 4208},
>> +	{IMX258_REG_SCALE_MODE_EXT, 0},
>> +	{IMX258_REG_SCALE_M_EXT, 16},
>> +	{IMX258_REG_FORCE_FD_SUM, 0},
>> +	{IMX258_REG_FRM_LENGTH_CTL, 0},
>> +	{IMX258_REG_ANALOG_GAIN, 0},
>> +	{IMX258_REG_GR_DIGITAL_GAIN, 256},
>> +	{IMX258_REG_R_DIGITAL_GAIN, 256},
>> +	{IMX258_REG_B_DIGITAL_GAIN, 256},
>> +	{IMX258_REG_GB_DIGITAL_GAIN, 256},
>> +	{IMX258_REG_AF_WINDOW_MODE, 0},
>> +	{ CCI_REG8(0x94DC), 0x20 },
>> +	{ CCI_REG8(0x94DD), 0x20 },
>> +	{ CCI_REG8(0x94DE), 0x20 },
>> +	{ CCI_REG8(0x95DC), 0x20 },
>> +	{ CCI_REG8(0x95DD), 0x20 },
>> +	{ CCI_REG8(0x95DE), 0x20 },
>> +	{ CCI_REG8(0x7FB0), 0x00 },
>> +	{ CCI_REG8(0x9010), 0x3E },
>> +	{ CCI_REG8(0x9419), 0x50 },
>> +	{ CCI_REG8(0x941B), 0x50 },
>> +	{ CCI_REG8(0x9519), 0x50 },
>> +	{ CCI_REG8(0x951B), 0x50 },
>> +	{IMX258_REG_PHASE_PIX_OUTEN, 0},
>> +	{IMX258_REG_PDPIX_DATA_RATE, 0},
>> +	{IMX258_REG_HDR, 0},
>>  };
>>  
>> -static const struct imx258_reg mode_4208x3120_regs[] = {
>> -	{ 0x0900, 0x00 },
>> -	{ 0x0901, 0x11 },
>> -	{ 0x0401, 0x00 },
>> -	{ 0x0405, 0x10 },
>> -	{ 0x040E, 0x0C },
>> -	{ 0x040F, 0x30 },
>> -	{ 0x034C, 0x10 },
>> -	{ 0x034D, 0x70 },
>> -	{ 0x034E, 0x0C },
>> -	{ 0x034F, 0x30 },
>> +static const struct cci_reg_sequence mode_4208x3120_regs[] = {
>> +	{IMX258_REG_BINNING_MODE, 0},
>> +	{IMX258_REG_BINNING_TYPE_V, 0x11},
>> +	{IMX258_REG_SCALE_MODE, 0},
>> +	{IMX258_REG_SCALE_M, 16},
>> +	{IMX258_REG_DIG_CROP_IMAGE_HEIGHT, 3120},
>> +	{IMX258_REG_X_OUT_SIZE, 4208},
>> +	{IMX258_REG_Y_OUT_SIZE, 3120},
>>  };
>>  
>> -static const struct imx258_reg mode_2104_1560_regs[] = {
>> -	{ 0x0900, 0x01 },
>> -	{ 0x0901, 0x12 },
>> -	{ 0x0401, 0x01 },
>> -	{ 0x0405, 0x20 },
>> -	{ 0x040E, 0x06 },
>> -	{ 0x040F, 0x18 },
>> -	{ 0x034C, 0x08 },
>> -	{ 0x034D, 0x38 },
>> -	{ 0x034E, 0x06 },
>> -	{ 0x034F, 0x18 },
>> +static const struct cci_reg_sequence mode_2104_1560_regs[] = {
>> +	{IMX258_REG_BINNING_MODE, 1},
>> +	{IMX258_REG_BINNING_TYPE_V, 0x12},
>> +	{IMX258_REG_SCALE_MODE, 1},
>> +	{IMX258_REG_SCALE_M, 32},
>> +	{IMX258_REG_DIG_CROP_IMAGE_HEIGHT, 1560},
>> +	{IMX258_REG_X_OUT_SIZE, 2104},
>> +	{IMX258_REG_Y_OUT_SIZE, 1560},
>>  };
>>  
>> -static const struct imx258_reg mode_1048_780_regs[] = {
>> -	{ 0x0900, 0x01 },
>> -	{ 0x0901, 0x14 },
>> -	{ 0x0401, 0x01 },
>> -	{ 0x0405, 0x40 },
>> -	{ 0x040E, 0x03 },
>> -	{ 0x040F, 0x0C },
>> -	{ 0x034C, 0x04 },
>> -	{ 0x034D, 0x18 },
>> -	{ 0x034E, 0x03 },
>> -	{ 0x034F, 0x0C },
>> +static const struct cci_reg_sequence mode_1048_780_regs[] = {
>> +	{IMX258_REG_BINNING_MODE, 1},
>> +	{IMX258_REG_BINNING_TYPE_V, 0x14},
>> +	{IMX258_REG_SCALE_MODE, 1},
>> +	{IMX258_REG_SCALE_M, 64},
>> +	{IMX258_REG_DIG_CROP_IMAGE_HEIGHT, 780},
>> +	{IMX258_REG_X_OUT_SIZE, 1048},
>> +	{IMX258_REG_Y_OUT_SIZE, 780},
>>  };
>>  
>>  struct imx258_variant_cfg {
>> -	const struct imx258_reg *regs;
>> +	const struct cci_reg_sequence *regs;
>>  	unsigned int num_regs;
>>  };
>>  
>> -static const struct imx258_reg imx258_cfg_regs[] = {
>> -	{ 0x3052, 0x00 },
>> -	{ 0x4E21, 0x14 },
>> -	{ 0x7B25, 0x00 },
>> +static const struct cci_reg_sequence imx258_cfg_regs[] = {
>> +	{ CCI_REG8(0x3052), 0x00 },
>> +	{ CCI_REG8(0x4E21), 0x14 },
>> +	{ CCI_REG8(0x7B25), 0x00 },
>>  };
>>  
>>  static const struct imx258_variant_cfg imx258_cfg = {
>> @@ -473,10 +443,10 @@ static const struct imx258_variant_cfg imx258_cfg = {
>>  	.num_regs = ARRAY_SIZE(imx258_cfg_regs),
>>  };
>>  
>> -static const struct imx258_reg imx258_pdaf_cfg_regs[] = {
>> -	{ 0x3052, 0x01 },
>> -	{ 0x4E21, 0x10 },
>> -	{ 0x7B25, 0x01 },
>> +static const struct cci_reg_sequence imx258_pdaf_cfg_regs[] = {
>> +	{ CCI_REG8(0x3052), 0x01 },
>> +	{ CCI_REG8(0x4E21), 0x10 },
>> +	{ CCI_REG8(0x7B25), 0x01 },
>>  };
>>  
>>  static const struct imx258_variant_cfg imx258_pdaf_cfg = {
>> @@ -677,6 +647,7 @@ static const struct imx258_mode supported_modes[] = {
>>  struct imx258 {
>>  	struct v4l2_subdev sd;
>>  	struct media_pad pad;
>> +	struct regmap *regmap;
>>  
>>  	const struct imx258_variant_cfg *variant_cfg;
>>  
>> @@ -717,80 +688,6 @@ static inline struct imx258 *to_imx258(struct v4l2_subdev *_sd)
>>  	return container_of(_sd, struct imx258, sd);
>>  }
>>  
>> -/* Read registers up to 2 at a time */
>> -static int imx258_read_reg(struct imx258 *imx258, u16 reg, u32 len, u32 *val)
>> -{
>> -	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
>> -	struct i2c_msg msgs[2];
>> -	u8 addr_buf[2] = { reg >> 8, reg & 0xff };
>> -	u8 data_buf[4] = { 0, };
>> -	int ret;
>> -
>> -	if (len > 4)
>> -		return -EINVAL;
>> -
>> -	/* Write register address */
>> -	msgs[0].addr = client->addr;
>> -	msgs[0].flags = 0;
>> -	msgs[0].len = ARRAY_SIZE(addr_buf);
>> -	msgs[0].buf = addr_buf;
>> -
>> -	/* Read data from register */
>> -	msgs[1].addr = client->addr;
>> -	msgs[1].flags = I2C_M_RD;
>> -	msgs[1].len = len;
>> -	msgs[1].buf = &data_buf[4 - len];
>> -
>> -	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
>> -	if (ret != ARRAY_SIZE(msgs))
>> -		return -EIO;
>> -
>> -	*val = get_unaligned_be32(data_buf);
>> -
>> -	return 0;
>> -}
>> -
>> -/* Write registers up to 2 at a time */
>> -static int imx258_write_reg(struct imx258 *imx258, u16 reg, u32 len, u32 val)
>> -{
>> -	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
>> -	u8 buf[6];
>> -
>> -	if (len > 4)
>> -		return -EINVAL;
>> -
>> -	put_unaligned_be16(reg, buf);
>> -	put_unaligned_be32(val << (8 * (4 - len)), buf + 2);
>> -	if (i2c_master_send(client, buf, len + 2) != len + 2)
>> -		return -EIO;
>> -
>> -	return 0;
>> -}
>> -
>> -/* Write a list of registers */
>> -static int imx258_write_regs(struct imx258 *imx258,
>> -			     const struct imx258_reg *regs, u32 len)
>> -{
>> -	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
>> -	unsigned int i;
>> -	int ret;
>> -
>> -	for (i = 0; i < len; i++) {
>> -		ret = imx258_write_reg(imx258, regs[i].address, 1,
>> -					regs[i].val);
>> -		if (ret) {
>> -			dev_err_ratelimited(
>> -				&client->dev,
>> -				"Failed to write reg 0x%4.4x. error = %d\n",
>> -				regs[i].address, ret);
>> -
>> -			return ret;
>> -		}
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>>  /* Get bayer order based on flip setting. */
>>  static u32 imx258_get_format_code(const struct imx258 *imx258)
>>  {
>> @@ -828,28 +725,20 @@ static int imx258_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>>  	return 0;
>>  }
>>  
>> -static int imx258_update_digital_gain(struct imx258 *imx258, u32 len, u32 val)
>> +static int imx258_update_digital_gain(struct imx258 *imx258, u32 val)
>>  {
>>  	int ret;
>>  
>> -	ret = imx258_write_reg(imx258, IMX258_REG_GR_DIGITAL_GAIN,
>> -				IMX258_REG_VALUE_16BIT,
>> -				val);
>> +	ret = cci_write(imx258->regmap, IMX258_REG_GR_DIGITAL_GAIN, val, NULL);
>>  	if (ret)
>>  		return ret;
>> -	ret = imx258_write_reg(imx258, IMX258_REG_GB_DIGITAL_GAIN,
>> -				IMX258_REG_VALUE_16BIT,
>> -				val);
>> +	ret = cci_write(imx258->regmap, IMX258_REG_GB_DIGITAL_GAIN, val, NULL);
>>  	if (ret)
>>  		return ret;
>> -	ret = imx258_write_reg(imx258, IMX258_REG_R_DIGITAL_GAIN,
>> -				IMX258_REG_VALUE_16BIT,
>> -				val);
>> +	ret = cci_write(imx258->regmap, IMX258_REG_R_DIGITAL_GAIN, val, NULL);
>>  	if (ret)
>>  		return ret;
>> -	ret = imx258_write_reg(imx258, IMX258_REG_B_DIGITAL_GAIN,
>> -				IMX258_REG_VALUE_16BIT,
>> -				val);
>> +	ret = cci_write(imx258->regmap, IMX258_REG_B_DIGITAL_GAIN, val, NULL);
>>  	if (ret)
>>  		return ret;
>>  	return 0;
> 
> What about reducing the usage of the "if (ret) return ret;" pattern?
> You can:
> 
> int ret = 0;
> 
> cci_write(imx258->regmap, IMX258_REG_GR_DIGITAL_GAIN, val, &ret);
> cci_write(imx258->regmap, IMX258_REG_GB_DIGITAL_GAIN, val, &ret);
> cci_write(imx258->regmap, IMX258_REG_R_DIGITAL_GAIN, val, &ret);
> cci_write(imx258->regmap, IMX258_REG_B_DIGITAL_GAIN, val, &ret);
> 
> return ret;
> 
> What do you think?
> 

Yeah that works, I have implemented that for the next revision

> 
>> @@ -891,53 +780,45 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
>>  
>>  	switch (ctrl->id) {
>>  	case V4L2_CID_ANALOGUE_GAIN:
>> -		ret = imx258_write_reg(imx258, IMX258_REG_ANALOG_GAIN,
>> -				IMX258_REG_VALUE_16BIT,
>> -				ctrl->val);
>> +		ret = cci_write(imx258->regmap, IMX258_REG_ANALOG_GAIN,
>> +				ctrl->val, NULL);
>>  		break;
>>  	case V4L2_CID_EXPOSURE:
>> -		ret = imx258_write_reg(imx258, IMX258_REG_EXPOSURE,
>> -				IMX258_REG_VALUE_16BIT,
>> -				ctrl->val);
>> +		ret = cci_write(imx258->regmap, IMX258_REG_EXPOSURE,
>> +				ctrl->val, NULL);
>>  		break;
>>  	case V4L2_CID_DIGITAL_GAIN:
>> -		ret = imx258_update_digital_gain(imx258, IMX258_REG_VALUE_16BIT,
>> -				ctrl->val);
>> +		ret = imx258_update_digital_gain(imx258, ctrl->val);
>>  		break;
>>  	case V4L2_CID_TEST_PATTERN:
>> -		ret = imx258_write_reg(imx258, IMX258_REG_TEST_PATTERN,
>> -				IMX258_REG_VALUE_16BIT,
>> -				ctrl->val);
>> +		ret = cci_write(imx258->regmap, IMX258_REG_TEST_PATTERN,
>> +				ctrl->val, NULL);
>>  		break;
>>  	case V4L2_CID_WIDE_DYNAMIC_RANGE:
>>  		if (!ctrl->val) {
>> -			ret = imx258_write_reg(imx258, IMX258_REG_HDR,
>> -					       IMX258_REG_VALUE_08BIT,
>> -					       IMX258_HDR_RATIO_MIN);
>> +			ret = cci_write(imx258->regmap, IMX258_REG_HDR,
>> +					IMX258_HDR_RATIO_MIN, NULL);
>>  		} else {
>> -			ret = imx258_write_reg(imx258, IMX258_REG_HDR,
>> -					       IMX258_REG_VALUE_08BIT,
>> -					       IMX258_HDR_ON);
>> +			ret = cci_write(imx258->regmap, IMX258_REG_HDR,
>> +					IMX258_HDR_ON, NULL);
>>  			if (ret)
>>  				break;
>> -			ret = imx258_write_reg(imx258, IMX258_REG_HDR_RATIO,
>> -					       IMX258_REG_VALUE_08BIT,
>> -					       BIT(IMX258_HDR_RATIO_MAX));
>> +			ret = cci_write(imx258->regmap, IMX258_REG_HDR_RATIO,
>> +					BIT(IMX258_HDR_RATIO_MAX), NULL);
>>  		}
>>  		break;
>>  	case V4L2_CID_VBLANK:
>> -		ret = imx258_write_reg(imx258, IMX258_REG_VTS,
>> -				       IMX258_REG_VALUE_16BIT,
>> -				       imx258->cur_mode->height + ctrl->val);
>> +		ret = cci_write(imx258->regmap, IMX258_REG_FRM_LENGTH_LINES,
>> +				imx258->cur_mode->height + ctrl->val, NULL);
>>  		break;
>>  	case V4L2_CID_VFLIP:
>>  	case V4L2_CID_HFLIP:
>> -		ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
>> -				       IMX258_REG_VALUE_08BIT,
>> -				       (imx258->hflip->val ?
>> -					REG_CONFIG_MIRROR_HFLIP : 0) |
>> -				       (imx258->vflip->val ?
>> -					REG_CONFIG_MIRROR_VFLIP : 0));
>> +		ret = cci_write(imx258->regmap, REG_MIRROR_FLIP_CONTROL,
>> +				(imx258->hflip->val ?
>> +				 REG_CONFIG_MIRROR_HFLIP : 0) |
>> +				(imx258->vflip->val ?
>> +				 REG_CONFIG_MIRROR_VFLIP : 0),
>> +				NULL);
>>  		break;
>>  	default:
>>  		dev_info(&client->dev,
>> @@ -1147,8 +1028,7 @@ static int imx258_start_streaming(struct imx258 *imx258)
>>  	const struct imx258_link_freq_config *link_freq_cfg;
>>  	int ret, link_freq_index;
>>  
>> -	ret = imx258_write_reg(imx258, IMX258_REG_RESET, IMX258_REG_VALUE_08BIT,
>> -			       0x01);
>> +	ret = cci_write(imx258->regmap, IMX258_REG_RESET, 0x01, NULL);
>>  	if (ret) {
>>  		dev_err(&client->dev, "%s failed to reset sensor\n", __func__);
>>  		return ret;
>> @@ -1162,30 +1042,30 @@ static int imx258_start_streaming(struct imx258 *imx258)
>>  	link_freq_cfg = &imx258->link_freq_configs[link_freq_index];
>>  
>>  	reg_list = &link_freq_cfg->link_cfg[imx258->lane_mode_idx].reg_list;
>> -	ret = imx258_write_regs(imx258, reg_list->regs, reg_list->num_of_regs);
>> +	ret = cci_multi_reg_write(imx258->regmap, reg_list->regs, reg_list->num_of_regs, NULL);
>>  	if (ret) {
>>  		dev_err(&client->dev, "%s failed to set plls\n", __func__);
>>  		return ret;
>>  	}
>>  
>> -	ret = imx258_write_regs(imx258, mode_common_regs,
>> -				ARRAY_SIZE(mode_common_regs));
>> +	ret = cci_multi_reg_write(imx258->regmap, mode_common_regs,
>> +				  ARRAY_SIZE(mode_common_regs), NULL);
>>  	if (ret) {
>>  		dev_err(&client->dev, "%s failed to set common regs\n", __func__);
>>  		return ret;
>>  	}
>>  
>> -	ret = imx258_write_regs(imx258, imx258->variant_cfg->regs,
>> -				imx258->variant_cfg->num_regs);
>> +	ret = cci_multi_reg_write(imx258->regmap, imx258->variant_cfg->regs,
>> +				  imx258->variant_cfg->num_regs, NULL);
>>  	if (ret) {
>>  		dev_err(&client->dev, "%s failed to set variant config\n",
>>  			__func__);
>>  		return ret;
>>  	}
>>  
>> -	ret = imx258_write_reg(imx258, IMX258_CLK_BLANK_STOP,
>> -			       IMX258_REG_VALUE_08BIT,
>> -			       !!(imx258->csi2_flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK));
>> +	ret = cci_write(imx258->regmap, IMX258_CLK_BLANK_STOP,
>> +			!!(imx258->csi2_flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK),
>> +			NULL);
>>  	if (ret) {
>>  		dev_err(&client->dev, "%s failed to set clock lane mode\n", __func__);
>>  		return ret;
>> @@ -1193,7 +1073,7 @@ static int imx258_start_streaming(struct imx258 *imx258)
>>  
>>  	/* Apply default values of current mode */
>>  	reg_list = &imx258->cur_mode->reg_list;
>> -	ret = imx258_write_regs(imx258, reg_list->regs, reg_list->num_of_regs);
>> +	ret = cci_multi_reg_write(imx258->regmap, reg_list->regs, reg_list->num_of_regs, NULL);
>>  	if (ret) {
>>  		dev_err(&client->dev, "%s failed to set mode\n", __func__);
>>  		return ret;
>> @@ -1205,9 +1085,8 @@ static int imx258_start_streaming(struct imx258 *imx258)
>>  		return ret;
>>  
>>  	/* set stream on register */
>> -	return imx258_write_reg(imx258, IMX258_REG_MODE_SELECT,
>> -				IMX258_REG_VALUE_08BIT,
>> -				IMX258_MODE_STREAMING);
>> +	return cci_write(imx258->regmap, IMX258_REG_MODE_SELECT,
>> +			 IMX258_MODE_STREAMING, NULL);
>>  }
>>  
>>  /* Stop streaming */
>> @@ -1217,8 +1096,8 @@ static int imx258_stop_streaming(struct imx258 *imx258)
>>  	int ret;
>>  
>>  	/* set stream off register */
>> -	ret = imx258_write_reg(imx258, IMX258_REG_MODE_SELECT,
>> -		IMX258_REG_VALUE_08BIT, IMX258_MODE_STANDBY);
>> +	ret = cci_write(imx258->regmap, IMX258_REG_MODE_SELECT,
>> +			IMX258_MODE_STANDBY, NULL);
>>  	if (ret)
>>  		dev_err(&client->dev, "%s failed to set stream\n", __func__);
>>  
>> @@ -1316,10 +1195,10 @@ static int imx258_identify_module(struct imx258 *imx258)
>>  {
>>  	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
>>  	int ret;
>> -	u32 val;
>> +	u64 val;
>>  
>> -	ret = imx258_read_reg(imx258, IMX258_REG_CHIP_ID,
>> -			      IMX258_REG_VALUE_16BIT, &val);
>> +	ret = cci_read(imx258->regmap, IMX258_REG_CHIP_ID,
>> +		       &val, NULL);
>>  	if (ret) {
>>  		dev_err(&client->dev, "failed to read chip id %x\n",
>>  			IMX258_CHIP_ID);
>> @@ -1327,7 +1206,7 @@ static int imx258_identify_module(struct imx258 *imx258)
>>  	}
>>  
>>  	if (val != IMX258_CHIP_ID) {
>> -		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
>> +		dev_err(&client->dev, "chip id mismatch: %x!=%llx\n",
>>  			IMX258_CHIP_ID, val);
>>  		return -EIO;
>>  	}
>> @@ -1507,6 +1386,13 @@ static int imx258_probe(struct i2c_client *client)
>>  	if (!imx258)
>>  		return -ENOMEM;
>>  
>> +	imx258->regmap = devm_cci_regmap_init_i2c(client, 16);
>> +	if (IS_ERR(imx258->regmap)) {
>> +		ret = PTR_ERR(imx258->regmap);
>> +		dev_err(&client->dev, "failed to initialize CCI: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>>  	ret = imx258_get_regulators(imx258, client);
>>  	if (ret)
>>  		return dev_err_probe(&client->dev, ret,
>> -- 
>> 2.44.0
> 
> The rest looks good to me.
> Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>
> 
> Thanks & Regards,
> Tommaso
> 

Thank you very much!

>>
>>
Tommaso Merciai April 15, 2024, 4 p.m. UTC | #5 
Hi Luis,

On Sun, Apr 14, 2024 at 02:35:01PM -0600, git@luigi311.com wrote:
> From: Luis Garcia <git@luigi311.com>
> 
> It was documented in DT, but not implemented.

Good catch :-)

> 
> Signed-off-by: Ondrej Jirman <megous@megous.com>
> Signed-off-by: Luis Garcia <git@luigi311.com>
> ---
>  drivers/media/i2c/imx258.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index f0bd72f241e4..5de71cb7c1ae 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -699,6 +699,7 @@ struct imx258 {
>  	unsigned int csi2_flags;
>  
>  	struct gpio_desc *powerdown_gpio;
> +	struct gpio_desc *reset_gpio;
>  
>  	/*
>  	 * Mutex for serialized access:
> @@ -1250,7 +1251,11 @@ static int imx258_power_on(struct device *dev)
>  		regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
>  	}
>  
> -	return ret;
> +	gpiod_set_value_cansleep(imx258->reset_gpio, 0);
> +

I think you can remove this new line here.

> +	usleep_range(400, 500);
> +
> +	return 0;
>  }
>  
>  static int imx258_power_off(struct device *dev)
> @@ -1260,6 +1265,7 @@ static int imx258_power_off(struct device *dev)
>  
>  	clk_disable_unprepare(imx258->clk);
>  
> +	gpiod_set_value_cansleep(imx258->reset_gpio, 1);
>  	gpiod_set_value_cansleep(imx258->powerdown_gpio, 1);
>  
>  	regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
> @@ -1573,6 +1579,12 @@ static int imx258_probe(struct i2c_client *client)
>  	if (IS_ERR(imx258->powerdown_gpio))
>  		return PTR_ERR(imx258->powerdown_gpio);
>  
> +	/* request optional reset pin */
> +	imx258->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> +						     GPIOD_OUT_HIGH);
> +	if (IS_ERR(imx258->reset_gpio))
> +		return PTR_ERR(imx258->reset_gpio);
> +
>  	/* Initialize subdev */
>  	v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops);
>  

Looks good to me.
Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>

> -- 
> 2.44.0
> 
>
Luis Garcia April 15, 2024, 4:25 p.m. UTC | #6 
On 4/15/24 10:00, Tommaso Merciai wrote:
> Hi Luis,
> 
> On Sun, Apr 14, 2024 at 02:35:01PM -0600, git@luigi311.com wrote:
>> From: Luis Garcia <git@luigi311.com>
>>
>> It was documented in DT, but not implemented.
> 
> Good catch :-)
> 
>>
>> Signed-off-by: Ondrej Jirman <megous@megous.com>
>> Signed-off-by: Luis Garcia <git@luigi311.com>
>> ---
>>  drivers/media/i2c/imx258.c | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>> index f0bd72f241e4..5de71cb7c1ae 100644
>> --- a/drivers/media/i2c/imx258.c
>> +++ b/drivers/media/i2c/imx258.c
>> @@ -699,6 +699,7 @@ struct imx258 {
>>  	unsigned int csi2_flags;
>>  
>>  	struct gpio_desc *powerdown_gpio;
>> +	struct gpio_desc *reset_gpio;
>>  
>>  	/*
>>  	 * Mutex for serialized access:
>> @@ -1250,7 +1251,11 @@ static int imx258_power_on(struct device *dev)
>>  		regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
>>  	}
>>  
>> -	return ret;
>> +	gpiod_set_value_cansleep(imx258->reset_gpio, 0);
>> +
> 
> I think you can remove this new line here.
> 

Done

>> +	usleep_range(400, 500);
>> +
>> +	return 0;
>>  }
>>  
>>  static int imx258_power_off(struct device *dev)
>> @@ -1260,6 +1265,7 @@ static int imx258_power_off(struct device *dev)
>>  
>>  	clk_disable_unprepare(imx258->clk);
>>  
>> +	gpiod_set_value_cansleep(imx258->reset_gpio, 1);
>>  	gpiod_set_value_cansleep(imx258->powerdown_gpio, 1);
>>  
>>  	regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
>> @@ -1573,6 +1579,12 @@ static int imx258_probe(struct i2c_client *client)
>>  	if (IS_ERR(imx258->powerdown_gpio))
>>  		return PTR_ERR(imx258->powerdown_gpio);
>>  
>> +	/* request optional reset pin */
>> +	imx258->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
>> +						     GPIOD_OUT_HIGH);
>> +	if (IS_ERR(imx258->reset_gpio))
>> +		return PTR_ERR(imx258->reset_gpio);
>> +
>>  	/* Initialize subdev */
>>  	v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops);
>>  
> 
> Looks good to me.
> Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>
>

Thanks!
 
>> -- 
>> 2.44.0
>>
>>
Luis Garcia April 15, 2024, 4:27 p.m. UTC | #7 
On 4/15/24 09:18, Tommaso Merciai wrote:
> Hi Luis,
> 
> On Sun, Apr 14, 2024 at 02:35:02PM -0600, git@luigi311.com wrote:
>> From: Luis Garcia <git@luigi311.com>
>>
>> Use the v4l2_link_freq_to_bitmap() helper to figure out which
>> driver-supported link freq can be used on a given system.
>>
>> Signed-off-by: Luis Garcia <git@luigi311.com>
>> Reviewed-by: Pavel Machek <pavel@ucw.cz>
>> ---
>>  drivers/media/i2c/imx258.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>> index 5de71cb7c1ae..65846dff775e 100644
>> --- a/drivers/media/i2c/imx258.c
>> +++ b/drivers/media/i2c/imx258.c
>> @@ -693,6 +693,7 @@ struct imx258 {
>>  	/* Current mode */
>>  	const struct imx258_mode *cur_mode;
>>  
>> +	unsigned long link_freq_bitmap;
>>  	const struct imx258_link_freq_config *link_freq_configs;
>>  	const s64 *link_freq_menu_items;
>>  	unsigned int lane_mode_idx;
>> @@ -1552,6 +1553,17 @@ static int imx258_probe(struct i2c_client *client)
>>  		return ret;
>>  	}
>>  
>> +	ret = v4l2_link_freq_to_bitmap(&client->dev,
>> +				       ep.link_frequencies,
>> +				       ep.nr_of_link_frequencies,
>> +				       imx258->link_freq_menu_items,
>> +				       ARRAY_SIZE(link_freq_menu_items_19_2),
>> +				       &imx258->link_freq_bitmap);
>> +	if (ret) {
>> +		dev_err(&client->dev, "Link frequency not supported\n");
>> +		goto error_endpoint_free;
>> +	}
>> +
>>  	/* Get number of data lanes */
>>  	switch (ep.bus.mipi_csi2.num_data_lanes) {
>>  	case 2:
> 
> Looks good to me.
> 
> ps:
> Maybe a good plan for the future would be to use: dev_err_probe
> (instead of dev_err into probe function)
> 
> But this I think is somenthing for next improvements. :)
> 
> Thanks & Regards,
> Tommaso
> 

Perfect, can i go ahead and add in your reviewed by looks like
you didnt add it here.

>> -- 
>> 2.44.0
>>
>>
Tommaso Merciai April 15, 2024, 4:32 p.m. UTC | #8 
On Mon, Apr 15, 2024 at 10:27:28AM -0600, Luis Garcia wrote:
> On 4/15/24 09:18, Tommaso Merciai wrote:
> > Hi Luis,
> > 
> > On Sun, Apr 14, 2024 at 02:35:02PM -0600, git@luigi311.com wrote:
> >> From: Luis Garcia <git@luigi311.com>
> >>
> >> Use the v4l2_link_freq_to_bitmap() helper to figure out which
> >> driver-supported link freq can be used on a given system.
> >>
> >> Signed-off-by: Luis Garcia <git@luigi311.com>
> >> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> >> ---
> >>  drivers/media/i2c/imx258.c | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> >> index 5de71cb7c1ae..65846dff775e 100644
> >> --- a/drivers/media/i2c/imx258.c
> >> +++ b/drivers/media/i2c/imx258.c
> >> @@ -693,6 +693,7 @@ struct imx258 {
> >>  	/* Current mode */
> >>  	const struct imx258_mode *cur_mode;
> >>  
> >> +	unsigned long link_freq_bitmap;
> >>  	const struct imx258_link_freq_config *link_freq_configs;
> >>  	const s64 *link_freq_menu_items;
> >>  	unsigned int lane_mode_idx;
> >> @@ -1552,6 +1553,17 @@ static int imx258_probe(struct i2c_client *client)
> >>  		return ret;
> >>  	}
> >>  
> >> +	ret = v4l2_link_freq_to_bitmap(&client->dev,
> >> +				       ep.link_frequencies,
> >> +				       ep.nr_of_link_frequencies,
> >> +				       imx258->link_freq_menu_items,
> >> +				       ARRAY_SIZE(link_freq_menu_items_19_2),
> >> +				       &imx258->link_freq_bitmap);
> >> +	if (ret) {
> >> +		dev_err(&client->dev, "Link frequency not supported\n");
> >> +		goto error_endpoint_free;
> >> +	}
> >> +
> >>  	/* Get number of data lanes */
> >>  	switch (ep.bus.mipi_csi2.num_data_lanes) {
> >>  	case 2:
> > 
> > Looks good to me.
> > 
> > ps:
> > Maybe a good plan for the future would be to use: dev_err_probe
> > (instead of dev_err into probe function)
> > 
> > But this I think is somenthing for next improvements. :)
> > 
> > Thanks & Regards,
> > Tommaso
> > 
> 
> Perfect, can i go ahead and add in your reviewed by looks like
> you didnt add it here.

Yep sorry I miss that. :'(

Thanks & Regards,
Tommaso

> 
> >> -- 
> >> 2.44.0
> >>
> >>
>
Luis Garcia April 15, 2024, 4:38 p.m. UTC | #9 
On 4/15/24 00:25, Alexander Stein wrote:
> Hi,
> 
> Am Sonntag, 14. April 2024, 22:34:40 CEST schrieb git@luigi311.com:
>> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>
>> The output image is defined as being 4208x3118 pixels in size.
>> Y_ADD_STA register was set to 0, and Y_ADD_END to 3118, giving
>> 3119 lines total.
>>
>> The datasheet lists a requirement for Y_ADD_STA to be a multiple
>> of a power of 2 depending on binning/scaling mode (2 for full pixel,
>> 4 for x2-bin/scale, 8 for (x2-bin)+(x2-subsample) or x4-bin, or 16
>> for (x4-bin)+(x2-subsample)).
>> (Y_ADD_END – Y_ADD_STA + 1) also has to be a similar power of 2.
>>
>> The current configuration for the full res modes breaks that second
>> requirement, and we can't increase Y_ADD_STA to 1 to retain exactly
>> the same field of view as that then breaks the first requirement.
>> For the binned modes, they are worse off as 3118 is not a multiple of
>> 4.
>>
>> Increase the main mode to 4208x3120 so that it is the same FOV as the
>> binned modes, with Y_ADD_STA at 0.
>> Fix Y_ADD_STA and Y_ADD_END for the binned modes so that they meet the
>> sensor requirements.
>>
>> This does change the Bayer order as the default configuration is for
>> H&V flips to be enabled, so readout is from Y_STA_END to Y_ADD_STA,
>> and this patch has changed Y_STA_END.
>>
>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>> Signed-off-by: Luis Garcia <git@luigi311.com>
>> Reviewed-by: Pavel Machek <pavel@ucw.cz>
>> ---
>>  drivers/media/i2c/imx258.c | 26 +++++++++++++-------------
>>  1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>> index 2dbafd21dd70..4a7048d834c6 100644
>> --- a/drivers/media/i2c/imx258.c
>> +++ b/drivers/media/i2c/imx258.c
>> [snip]
>> @@ -707,7 +707,7 @@ static int imx258_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>>  	/* Initialize try_fmt */
>>  	try_fmt->width = supported_modes[0].width;
>>  	try_fmt->height = supported_modes[0].height;
>> -	try_fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
>> +	try_fmt->code = MEDIA_BUS_FMT_SBGGR10_1X10;
> 
> Does someone have access to the data sheet? I am wondering how the
> arrangement of the pixel array is shown. I've the following (identical)
> array for these sensors:
> * imx290/imx327
> * imx219
> * imx415
> 
> G B G B
> R G R G
> G B G B
> R G R G
>


I assume this is what you are looking for
https://photos.luigi311.com/share/Imk6odsR_44VYsRvfmRwBVoG1TMnXtI61PP4sjssbmKAcNEYuVPRa9W-vIU7rpa-Ask

 
> Yet each driver configures a different bus format:
> 
> * imx290.c: MEDIA_BUS_FMT_SRGGB10_1X10
> * imx415.c: MEDIA_BUS_FMT_SGBRG10_1X10
> * imx219.c: MEDIA_BUS_FMT_SRGGB10_1X10 (no flip)
> 
> imx219 actually defines all 4 10 Bit Bayer patterns and the comment
> indicates this depends on how v or h flip is configured.
> Reading the commit message apparently the same is true for this driver.
>> Still this is confusing as I would have expected flipping to be disabled by
> default, expecting the same Bayer pattern order for all drivers. Can someone
> shed some light?
> 
> Best regards,
> Alexander
> 

Flipping defaults are changed around looks like based on use cases. Patch 20
is what actually brings in flipping and brings in the 4 definitions like you
are expecting

+	/* 10-bit modes. */
+	MEDIA_BUS_FMT_SRGGB10_1X10,
+	MEDIA_BUS_FMT_SGRBG10_1X10,
+	MEDIA_BUS_FMT_SGBRG10_1X10,
+	MEDIA_BUS_FMT_SBGGR10_1X10
Dave Stevenson April 16, 2024, 3:18 p.m. UTC | #10 
Hi Alexander

On Mon, 15 Apr 2024 at 07:25, Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi,
>
> Am Sonntag, 14. April 2024, 22:34:40 CEST schrieb git@luigi311.com:
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > The output image is defined as being 4208x3118 pixels in size.
> > Y_ADD_STA register was set to 0, and Y_ADD_END to 3118, giving
> > 3119 lines total.
> >
> > The datasheet lists a requirement for Y_ADD_STA to be a multiple
> > of a power of 2 depending on binning/scaling mode (2 for full pixel,
> > 4 for x2-bin/scale, 8 for (x2-bin)+(x2-subsample) or x4-bin, or 16
> > for (x4-bin)+(x2-subsample)).
> > (Y_ADD_END – Y_ADD_STA + 1) also has to be a similar power of 2.
> >
> > The current configuration for the full res modes breaks that second
> > requirement, and we can't increase Y_ADD_STA to 1 to retain exactly
> > the same field of view as that then breaks the first requirement.
> > For the binned modes, they are worse off as 3118 is not a multiple of
> > 4.
> >
> > Increase the main mode to 4208x3120 so that it is the same FOV as the
> > binned modes, with Y_ADD_STA at 0.
> > Fix Y_ADD_STA and Y_ADD_END for the binned modes so that they meet the
> > sensor requirements.
> >
> > This does change the Bayer order as the default configuration is for
> > H&V flips to be enabled, so readout is from Y_STA_END to Y_ADD_STA,
> > and this patch has changed Y_STA_END.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Signed-off-by: Luis Garcia <git@luigi311.com>
> > Reviewed-by: Pavel Machek <pavel@ucw.cz>
> > ---
> >  drivers/media/i2c/imx258.c | 26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index 2dbafd21dd70..4a7048d834c6 100644
> > --- a/drivers/media/i2c/imx258.c
> > +++ b/drivers/media/i2c/imx258.c
> > [snip]
> > @@ -707,7 +707,7 @@ static int imx258_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> >       /* Initialize try_fmt */
> >       try_fmt->width = supported_modes[0].width;
> >       try_fmt->height = supported_modes[0].height;
> > -     try_fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> > +     try_fmt->code = MEDIA_BUS_FMT_SBGGR10_1X10;
>
> Does someone have access to the data sheet? I am wondering how the
> arrangement of the pixel array is shown. I've the following (identical)
> array for these sensors:
> * imx290/imx327
> * imx219
> * imx415
>
> G B G B
> R G R G
> G B G B
> R G R G

Check the other 3 corners of your diagrams - they are not identical.

> Yet each driver configures a different bus format:
>
> * imx290.c: MEDIA_BUS_FMT_SRGGB10_1X10
> * imx415.c: MEDIA_BUS_FMT_SGBRG10_1X10
> * imx219.c: MEDIA_BUS_FMT_SRGGB10_1X10 (no flip)
>
> imx219 actually defines all 4 10 Bit Bayer patterns and the comment
> indicates this depends on how v or h flip is configured.
> Reading the commit message apparently the same is true for this driver.

Correct.

> Still this is confusing as I would have expected flipping to be disabled by
> default, expecting the same Bayer pattern order for all drivers. Can someone
> shed some light?

Comparing different families of sensors isn't really valid, and
manufacturers vary too.

imx290 is a Starvis sensor, and has an array of 1945x1109, so all 4
corners are red pixels. It crops an even number of pixels off the
array in the direction of readout, therefore always producing RGGB.

I don't have a datasheet for imx415. Whilst it is also a Starvis
sensor the product brief [1] says it is an array of 3864 x 2228, with
3864 x 2192 effective pixels, which implies it isn't doing the same as
imx290. However the current driver isn't changing the Bayer order
based on flip which contradicts that. Pass as to which is correct.
I can't answer why the default order is GBRG. Presumably the default
window mode used (it doesn't use X_START / Y_START registers) crops an
odd number of lines off the raw array, therefore starting on a GB row.

imx219 (Exmor R) and imx258 (Exmor RS) datasheets have an even number
of pixels in each direction in the array, and whilst the first pixel
read out in the default direction is red, the colours in the opposite
corner is blue, with green in the remaining corners. This is why the
Bayer order changes with flips.

Most Omnivision sensors I've encountered do the same as imx219/258,
whilst OnSemi sensors are the same as imx290. Drivers obviously have
to match whatever the hardware does.

  Dave

[1] https://www.sony-semicon.com/files/62/pdf/p-12_IMX415-AAQR_AAMR_Flyer.pdf

> Best regards,
> Alexander
>
> --
> 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/
>
>