[PATCH v6 0/6] media: imx335: 2/4 lane ops and improvements

mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed

* [PATCH v6 0/6] media: imx335: 2/4 lane ops and improvements
@ 2024-04-12 16:00 Umang Jain
  2024-04-12 16:00 ` [PATCH v6 1/6] media: imx335: Support 2 or 4 lane operation modes Umang Jain
                   ` (5 more replies)
  0 siblings, 6 replies; 8+ messages in thread
From: Umang Jain @ 2024-04-12 16:00 UTC (permalink / raw)
  To: linux-media
  Cc: Alexander Shiyan, Kieran Bingham, Sakari Ailus, open list, Umang Jain

Another batch of improvements of the imx335 driver.

Patch 1/6 adds support for 2 or 4 lane operation modes.

Patch 2/6 call the V4L2 fwnode device parser to handle controls that are
standardised by the framework.

Patch 3/6 introduces the use of CCI for registers access.

Patch 4/6 uses decimal values for sizes registers (instead of
hexadecimal). This improves overall readability

Patch 5/6 fixes the height value discrepency. Accessible height is 1944,
as per the data sheet

Patch 6/6 fixes the max analogue gain value.

Changes in v6: 
- Modify 2/6 to call v4l2_fwnode_device_parse()
  early in imx335_init_controls() and return early if error.
- Treat v4l2_ctrl_new_fwnode_properties() as one of framework
  functions, hence check ctrl_hdlr->error should be sufficient as
  requested.

Changes in v5:
- Simplify error handling in 2/6. Check for ctrl_hdlr->error
- Space fix around { } in 3/6. Drop /* undocumented * as well

Changes in v4:
- Do not change from window cropping mode in patch 4/6.
  In v3, the sensor was changed to all pixel scan mode to
  achieve height=1944, but it can be achieved in window
  cropping mode as well, by fixing the mode registers

changes in v3:
- fix patch 2/6 where we need to free ctrl handler
  on error path.

changes in v2:
- New patch 4/6
- Drop calculating the pixel clock from link freq.
- CCI register address sort (incremental)
- Fix cci_write for REG_HOLD handling and add a comment.
- Remove  unused macros as part of 3/6

Kieran Bingham (2):
  media: imx335: Support 2 or 4 lane operation modes
  media: imx335: Parse fwnode properties

Umang Jain (4):
  media: imx335: Use V4L2 CCI for accessing sensor registers
  media: imx335: Use integer values for size registers
  media: imx335: Fix active area height discrepency
  media: imx335: Limit analogue gain value

 drivers/media/i2c/Kconfig  |   1 +
 drivers/media/i2c/imx335.c | 628 ++++++++++++++++++-------------------
 2 files changed, 298 insertions(+), 331 deletions(-)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v6 1/6] media: imx335: Support 2 or 4 lane operation modes
  2024-04-12 16:00 [PATCH v6 0/6] media: imx335: 2/4 lane ops and improvements Umang Jain
@ 2024-04-12 16:00 ` Umang Jain
  2024-04-12 16:00 ` [PATCH v6 2/6] media: imx335: Parse fwnode properties Umang Jain
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Umang Jain @ 2024-04-12 16:00 UTC (permalink / raw)
  To: linux-media
  Cc: Alexander Shiyan, Kieran Bingham, Sakari Ailus, open list,
	Umang Jain, Tommaso Merciai

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

The IMX335 can support both 2 and 4 lane configurations.
Extend the driver to configure the lane mode accordingly.
Update the pixel rate depending on the number of lanes in use.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>
---
 drivers/media/i2c/imx335.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index dab6d080bc4c..c633ea1380e7 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -21,6 +21,11 @@
 #define IMX335_MODE_STANDBY	0x01
 #define IMX335_MODE_STREAMING	0x00
 
+/* Data Lanes */
+#define IMX335_LANEMODE		0x3a01
+#define IMX335_2LANE		1
+#define IMX335_4LANE		3
+
 /* Lines per frame */
 #define IMX335_REG_LPFR		0x3030
 
@@ -147,6 +152,7 @@ struct imx335_mode {
  * @exp_ctrl: Pointer to exposure control
  * @again_ctrl: Pointer to analog gain control
  * @vblank: Vertical blanking in lines
+ * @lane_mode Mode for number of connected data lanes
  * @cur_mode: Pointer to current selected sensor mode
  * @mutex: Mutex for serializing sensor controls
  * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
@@ -171,6 +177,7 @@ struct imx335 {
 		struct v4l2_ctrl *again_ctrl;
 	};
 	u32 vblank;
+	u32 lane_mode;
 	const struct imx335_mode *cur_mode;
 	struct mutex mutex;
 	unsigned long link_freq_bitmap;
@@ -936,6 +943,11 @@ static int imx335_start_streaming(struct imx335 *imx335)
 		return ret;
 	}
 
+	/* Configure lanes */
+	ret = imx335_write_reg(imx335, IMX335_LANEMODE, 1, imx335->lane_mode);
+	if (ret)
+		return ret;
+
 	/* Setup handler will write actual exposure and gain */
 	ret =  __v4l2_ctrl_handler_setup(imx335->sd.ctrl_handler);
 	if (ret) {
@@ -1096,7 +1108,14 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
 	if (ret)
 		return ret;
 
-	if (bus_cfg.bus.mipi_csi2.num_data_lanes != IMX335_NUM_DATA_LANES) {
+	switch (bus_cfg.bus.mipi_csi2.num_data_lanes) {
+	case 2:
+		imx335->lane_mode = IMX335_2LANE;
+		break;
+	case 4:
+		imx335->lane_mode = IMX335_4LANE;
+		break;
+	default:
 		dev_err(imx335->dev,
 			"number of CSI2 data lanes %d is not supported\n",
 			bus_cfg.bus.mipi_csi2.num_data_lanes);
-- 
2.43.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v6 2/6] media: imx335: Parse fwnode properties
  2024-04-12 16:00 [PATCH v6 0/6] media: imx335: 2/4 lane ops and improvements Umang Jain
  2024-04-12 16:00 ` [PATCH v6 1/6] media: imx335: Support 2 or 4 lane operation modes Umang Jain
@ 2024-04-12 16:00 ` Umang Jain
  2024-04-12 16:00 ` [PATCH v6 3/6] media: imx335: Use V4L2 CCI for accessing sensor registers Umang Jain
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Umang Jain @ 2024-04-12 16:00 UTC (permalink / raw)
  To: linux-media
  Cc: Alexander Shiyan, Kieran Bingham, Sakari Ailus, open list, Umang Jain

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Call the V4L2 fwnode device parser to handle controls that are
standardised by the framework.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index c633ea1380e7..cebe8cfd77be 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -1227,10 +1227,16 @@ static int imx335_init_controls(struct imx335 *imx335)
 {
 	struct v4l2_ctrl_handler *ctrl_hdlr = &imx335->ctrl_handler;
 	const struct imx335_mode *mode = imx335->cur_mode;
+	struct v4l2_fwnode_device_properties props;
 	u32 lpfr;
 	int ret;
 
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 7);
+	ret = v4l2_fwnode_device_parse(imx335->dev, &props);
+	if (ret)
+		return ret;
+
+	/* v4l2_fwnode_device_properties can add two more controls */
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 9);
 	if (ret)
 		return ret;
 
@@ -1295,6 +1301,8 @@ static int imx335_init_controls(struct imx335 *imx335)
 	if (imx335->hblank_ctrl)
 		imx335->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
+	v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx335_ctrl_ops, &props);
+
 	if (ctrl_hdlr->error) {
 		dev_err(imx335->dev, "control init failed: %d\n",
 			ctrl_hdlr->error);
-- 
2.43.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v6 3/6] media: imx335: Use V4L2 CCI for accessing sensor registers
  2024-04-12 16:00 [PATCH v6 0/6] media: imx335: 2/4 lane ops and improvements Umang Jain
  2024-04-12 16:00 ` [PATCH v6 1/6] media: imx335: Support 2 or 4 lane operation modes Umang Jain
  2024-04-12 16:00 ` [PATCH v6 2/6] media: imx335: Parse fwnode properties Umang Jain
@ 2024-04-12 16:00 ` Umang Jain
  2024-04-12 21:23   ` Kieran Bingham
  2024-04-12 16:00 ` [PATCH v6 4/6] media: imx335: Use integer values for size registers Umang Jain
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 8+ messages in thread
From: Umang Jain @ 2024-04-12 16:00 UTC (permalink / raw)
  To: linux-media
  Cc: Alexander Shiyan, Kieran Bingham, Sakari Ailus, open list, Umang Jain

Use the new comon CCI register access helpers to replace the private
register access helpers in the imx335 driver.

Select V4L2_CCI_I2C Kconfig option which the imx335 driver now
depends on.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/media/i2c/Kconfig  |   1 +
 drivers/media/i2c/imx335.c | 593 ++++++++++++++++---------------------
 2 files changed, 263 insertions(+), 331 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 56f276b920ab..8d248b9c9562 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -195,6 +195,7 @@ config VIDEO_IMX334
 config VIDEO_IMX335
 	tristate "Sony IMX335 sensor support"
 	depends on OF_GPIO
+	select V4L2_CCI_I2C
 	help
 	  This is a Video4Linux2 sensor driver for the Sony
 	  IMX335 camera.
diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index cebe8cfd77be..ee22304f827e 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -11,62 +11,104 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/regmap.h>
 
+#include <media/v4l2-cci.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
 /* Streaming Mode */
-#define IMX335_REG_MODE_SELECT	0x3000
-#define IMX335_MODE_STANDBY	0x01
-#define IMX335_MODE_STREAMING	0x00
+#define IMX335_REG_MODE_SELECT		CCI_REG8(0x3000)
+#define IMX335_MODE_STANDBY		0x01
+#define IMX335_MODE_STREAMING		0x00
 
-/* Data Lanes */
-#define IMX335_LANEMODE		0x3a01
-#define IMX335_2LANE		1
-#define IMX335_4LANE		3
+/* Group hold register */
+#define IMX335_REG_HOLD			CCI_REG8(0x3001)
+
+#define IMX335_REG_MASTER_MODE		CCI_REG8(0x3002)
+#define IMX335_REG_BCWAIT_TIME		CCI_REG8(0x300c)
+#define IMX335_REG_CPWAIT_TIME		CCI_REG8(0x300d)
+#define IMX335_REG_WINMODE		CCI_REG8(0x3018)
+#define IMX335_REG_HTRIMMING_START	CCI_REG16_LE(0x302c)
+#define IMX335_REG_HNUM			CCI_REG8(0x302e)
 
 /* Lines per frame */
-#define IMX335_REG_LPFR		0x3030
+#define IMX335_REG_VMAX			CCI_REG24_LE(0x3030)
 
-/* Chip ID */
-#define IMX335_REG_ID		0x3912
-#define IMX335_ID		0x00
+#define IMX335_REG_OPB_SIZE_V		CCI_REG8(0x304c)
+#define IMX335_REG_ADBIT		CCI_REG8(0x3050)
+#define IMX335_REG_Y_OUT_SIZE		CCI_REG16_LE(0x3056)
 
-/* Exposure control */
-#define IMX335_REG_SHUTTER	0x3058
-#define IMX335_EXPOSURE_MIN	1
-#define IMX335_EXPOSURE_OFFSET	9
-#define IMX335_EXPOSURE_STEP	1
-#define IMX335_EXPOSURE_DEFAULT	0x0648
+#define IMX335_REG_SHUTTER		CCI_REG24_LE(0x3058)
+#define IMX335_EXPOSURE_MIN		1
+#define IMX335_EXPOSURE_OFFSET		9
+#define IMX335_EXPOSURE_STEP		1
+#define IMX335_EXPOSURE_DEFAULT		0x0648
 
-/* Analog gain control */
-#define IMX335_REG_AGAIN	0x30e8
-#define IMX335_AGAIN_MIN	0
-#define IMX335_AGAIN_MAX	240
-#define IMX335_AGAIN_STEP	1
-#define IMX335_AGAIN_DEFAULT	0
+#define IMX335_REG_AREA3_ST_ADR_1	CCI_REG16_LE(0x3074)
+#define IMX335_REG_AREA3_WIDTH_1	CCI_REG16_LE(0x3076)
 
-/* Group hold register */
-#define IMX335_REG_HOLD		0x3001
+/* Analog gain control */
+#define IMX335_REG_AGAIN		CCI_REG8(0x30e8)
+#define IMX335_AGAIN_MIN		0
+#define IMX335_AGAIN_MAX		240
+#define IMX335_AGAIN_STEP		1
+#define IMX335_AGAIN_DEFAULT		0
+
+#define IMX335_REG_TPG_TESTCLKEN	CCI_REG8(0x3148)
+#define IMX335_REG_INCLKSEL1		CCI_REG16_LE(0x314c)
+#define IMX335_REG_INCLKSEL2		CCI_REG8(0x315a)
+#define IMX335_REG_INCLKSEL3		CCI_REG8(0x3168)
+#define IMX335_REG_INCLKSEL4		CCI_REG8(0x316a)
+#define IMX335_REG_MDBIT		CCI_REG8(0x319d)
+#define IMX335_REG_SYSMODE		CCI_REG8(0x319e)
+
+#define IMX335_REG_XVS_XHS_DRV		CCI_REG8(0x31a1)
 
 /* Test pattern generator */
-#define IMX335_REG_TPG		0x329e
-#define IMX335_TPG_ALL_000	0
-#define IMX335_TPG_ALL_FFF	1
-#define IMX335_TPG_ALL_555	2
-#define IMX335_TPG_ALL_AAA	3
-#define IMX335_TPG_TOG_555_AAA	4
-#define IMX335_TPG_TOG_AAA_555	5
-#define IMX335_TPG_TOG_000_555	6
-#define IMX335_TPG_TOG_555_000	7
-#define IMX335_TPG_TOG_000_FFF	8
-#define IMX335_TPG_TOG_FFF_000	9
-#define IMX335_TPG_H_COLOR_BARS 10
-#define IMX335_TPG_V_COLOR_BARS 11
+#define IMX335_REG_TPG_DIG_CLP_MODE	CCI_REG8(0x3280)
+#define IMX335_REG_TPG_EN_DUOUT		CCI_REG8(0x329c)
+#define IMX335_REG_TPG			CCI_REG8(0x329e)
+#define IMX335_REG_TPG_COLORWIDTH	CCI_REG8(0x32a0)
+#define IMX335_REG_TPG_BLKLEVEL		CCI_REG16_LE(0x3302)
+#define IMX335_REG_TPG_WRJ_OPEN		CCI_REG8(0x336c)
+#define IMX335_TPG_ALL_000		0
+#define IMX335_TPG_ALL_FFF		1
+#define IMX335_TPG_ALL_555		2
+#define IMX335_TPG_ALL_AAA		3
+#define IMX335_TPG_TOG_555_AAA		4
+#define IMX335_TPG_TOG_AAA_555		5
+#define IMX335_TPG_TOG_000_555		6
+#define IMX335_TPG_TOG_555_000		7
+#define IMX335_TPG_TOG_000_FFF		8
+#define IMX335_TPG_TOG_FFF_000		9
+#define IMX335_TPG_H_COLOR_BARS		10
+#define IMX335_TPG_V_COLOR_BARS		11
+
+#define IMX335_REG_ADBIT1		CCI_REG16_LE(0x341c)
+
+/* Chip ID */
+#define IMX335_REG_ID			CCI_REG8(0x3912)
+#define IMX335_ID			0x00
+
+/* Data Lanes */
+#define IMX335_REG_LANEMODE		CCI_REG8(0x3a01)
+#define IMX335_2LANE			1
+#define IMX335_4LANE			3
+
+#define IMX335_REG_TCLKPOST		CCI_REG16_LE(0x3a18)
+#define IMX335_REG_TCLKPREPARE		CCI_REG16_LE(0x3a1a)
+#define IMX335_REG_TCLK_TRAIL		CCI_REG16_LE(0x3a1c)
+#define IMX335_REG_TCLK_ZERO		CCI_REG16_LE(0x3a1e)
+#define IMX335_REG_THS_PREPARE		CCI_REG16_LE(0x3a20)
+#define IMX335_REG_THS_ZERO		CCI_REG16_LE(0x3a22)
+#define IMX335_REG_THS_TRAIL		CCI_REG16_LE(0x3a24)
+#define IMX335_REG_THS_EXIT		CCI_REG16_LE(0x3a26)
+#define IMX335_REG_TPLX			CCI_REG16_LE(0x3a28)
 
 /* Input clock rate */
-#define IMX335_INCLK_RATE	24000000
+#define IMX335_INCLK_RATE		24000000
 
 /* CSI2 HW configuration */
 #define IMX335_LINK_FREQ_594MHz		594000000LL
@@ -74,9 +116,6 @@
 
 #define IMX335_NUM_DATA_LANES	4
 
-#define IMX335_REG_MIN		0x00
-#define IMX335_REG_MAX		0xfffff
-
 /* IMX335 native and active pixel array size. */
 #define IMX335_NATIVE_WIDTH		2616U
 #define IMX335_NATIVE_HEIGHT		1964U
@@ -85,16 +124,6 @@
 #define IMX335_PIXEL_ARRAY_WIDTH	2592U
 #define IMX335_PIXEL_ARRAY_HEIGHT	1944U
 
-/**
- * struct imx335_reg - imx335 sensor register
- * @address: Register address
- * @val: Register value
- */
-struct imx335_reg {
-	u16 address;
-	u8 val;
-};
-
 /**
  * struct imx335_reg_list - imx335 sensor register list
  * @num_of_regs: Number of registers in the list
@@ -102,7 +131,7 @@ struct imx335_reg {
  */
 struct imx335_reg_list {
 	u32 num_of_regs;
-	const struct imx335_reg *regs;
+	const struct cci_reg_sequence *regs;
 };
 
 static const char * const imx335_supply_name[] = {
@@ -165,6 +194,7 @@ struct imx335 {
 	struct media_pad pad;
 	struct gpio_desc *reset_gpio;
 	struct regulator_bulk_data supplies[ARRAY_SIZE(imx335_supply_name)];
+	struct regmap *cci;
 
 	struct clk *inclk;
 	struct v4l2_ctrl_handler ctrl_handler;
@@ -217,140 +247,135 @@ static const int imx335_tpg_val[] = {
 };
 
 /* Sensor mode registers */
-static const struct imx335_reg mode_2592x1940_regs[] = {
-	{0x3000, 0x01},
-	{0x3002, 0x00},
-	{0x3018, 0x04},
-	{0x302c, 0x3c},
-	{0x302e, 0x20},
-	{0x3056, 0x94},
-	{0x3074, 0xc8},
-	{0x3076, 0x28},
-	{0x304c, 0x00},
-	{0x31a1, 0x00},
-	{0x3288, 0x21},
-	{0x328a, 0x02},
-	{0x3414, 0x05},
-	{0x3416, 0x18},
-	{0x3648, 0x01},
-	{0x364a, 0x04},
-	{0x364c, 0x04},
-	{0x3678, 0x01},
-	{0x367c, 0x31},
-	{0x367e, 0x31},
-	{0x3706, 0x10},
-	{0x3708, 0x03},
-	{0x3714, 0x02},
-	{0x3715, 0x02},
-	{0x3716, 0x01},
-	{0x3717, 0x03},
-	{0x371c, 0x3d},
-	{0x371d, 0x3f},
-	{0x372c, 0x00},
-	{0x372d, 0x00},
-	{0x372e, 0x46},
-	{0x372f, 0x00},
-	{0x3730, 0x89},
-	{0x3731, 0x00},
-	{0x3732, 0x08},
-	{0x3733, 0x01},
-	{0x3734, 0xfe},
-	{0x3735, 0x05},
-	{0x3740, 0x02},
-	{0x375d, 0x00},
-	{0x375e, 0x00},
-	{0x375f, 0x11},
-	{0x3760, 0x01},
-	{0x3768, 0x1b},
-	{0x3769, 0x1b},
-	{0x376a, 0x1b},
-	{0x376b, 0x1b},
-	{0x376c, 0x1a},
-	{0x376d, 0x17},
-	{0x376e, 0x0f},
-	{0x3776, 0x00},
-	{0x3777, 0x00},
-	{0x3778, 0x46},
-	{0x3779, 0x00},
-	{0x377a, 0x89},
-	{0x377b, 0x00},
-	{0x377c, 0x08},
-	{0x377d, 0x01},
-	{0x377e, 0x23},
-	{0x377f, 0x02},
-	{0x3780, 0xd9},
-	{0x3781, 0x03},
-	{0x3782, 0xf5},
-	{0x3783, 0x06},
-	{0x3784, 0xa5},
-	{0x3788, 0x0f},
-	{0x378a, 0xd9},
-	{0x378b, 0x03},
-	{0x378c, 0xeb},
-	{0x378d, 0x05},
-	{0x378e, 0x87},
-	{0x378f, 0x06},
-	{0x3790, 0xf5},
-	{0x3792, 0x43},
-	{0x3794, 0x7a},
-	{0x3796, 0xa1},
-	{0x37b0, 0x36},
-	{0x3a00, 0x00},
+static const struct cci_reg_sequence mode_2592x1940_regs[] = {
+	{ IMX335_REG_MODE_SELECT, 0x01 },
+	{ IMX335_REG_MASTER_MODE, 0x00 },
+	{ IMX335_REG_WINMODE, 0x04 },
+	{ IMX335_REG_HTRIMMING_START, 0x0180 },
+	{ IMX335_REG_HNUM, 0x0a20 },
+	{ IMX335_REG_Y_OUT_SIZE, 0x0794 },
+	{ IMX335_REG_AREA3_ST_ADR_1, 0x00b0 },
+	{ IMX335_REG_AREA3_WIDTH_1, 0x0f58 },
+	{ IMX335_REG_OPB_SIZE_V, 0x00 },
+	{ IMX335_REG_XVS_XHS_DRV, 0x00 },
+	{ CCI_REG8(0x3288), 0x21 },
+	{ CCI_REG8(0x328a), 0x02 },
+	{ CCI_REG8(0x3414), 0x05 },
+	{ CCI_REG8(0x3416), 0x18 },
+	{ CCI_REG8(0x3648), 0x01 },
+	{ CCI_REG8(0x364a), 0x04 },
+	{ CCI_REG8(0x364c), 0x04 },
+	{ CCI_REG8(0x3678), 0x01 },
+	{ CCI_REG8(0x367c), 0x31 },
+	{ CCI_REG8(0x367e), 0x31 },
+	{ CCI_REG8(0x3706), 0x10 },
+	{ CCI_REG8(0x3708), 0x03 },
+	{ CCI_REG8(0x3714), 0x02 },
+	{ CCI_REG8(0x3715), 0x02 },
+	{ CCI_REG8(0x3716), 0x01 },
+	{ CCI_REG8(0x3717), 0x03 },
+	{ CCI_REG8(0x371c), 0x3d },
+	{ CCI_REG8(0x371d), 0x3f },
+	{ CCI_REG8(0x372c), 0x00 },
+	{ CCI_REG8(0x372d), 0x00 },
+	{ CCI_REG8(0x372e), 0x46 },
+	{ CCI_REG8(0x372f), 0x00 },
+	{ CCI_REG8(0x3730), 0x89 },
+	{ CCI_REG8(0x3731), 0x00 },
+	{ CCI_REG8(0x3732), 0x08 },
+	{ CCI_REG8(0x3733), 0x01 },
+	{ CCI_REG8(0x3734), 0xfe },
+	{ CCI_REG8(0x3735), 0x05 },
+	{ CCI_REG8(0x3740), 0x02 },
+	{ CCI_REG8(0x375d), 0x00 },
+	{ CCI_REG8(0x375e), 0x00 },
+	{ CCI_REG8(0x375f), 0x11 },
+	{ CCI_REG8(0x3760), 0x01 },
+	{ CCI_REG8(0x3768), 0x1b },
+	{ CCI_REG8(0x3769), 0x1b },
+	{ CCI_REG8(0x376a), 0x1b },
+	{ CCI_REG8(0x376b), 0x1b },
+	{ CCI_REG8(0x376c), 0x1a },
+	{ CCI_REG8(0x376d), 0x17 },
+	{ CCI_REG8(0x376e), 0x0f },
+	{ CCI_REG8(0x3776), 0x00 },
+	{ CCI_REG8(0x3777), 0x00 },
+	{ CCI_REG8(0x3778), 0x46 },
+	{ CCI_REG8(0x3779), 0x00 },
+	{ CCI_REG8(0x377a), 0x89 },
+	{ CCI_REG8(0x377b), 0x00 },
+	{ CCI_REG8(0x377c), 0x08 },
+	{ CCI_REG8(0x377d), 0x01 },
+	{ CCI_REG8(0x377e), 0x23 },
+	{ CCI_REG8(0x377f), 0x02 },
+	{ CCI_REG8(0x3780), 0xd9 },
+	{ CCI_REG8(0x3781), 0x03 },
+	{ CCI_REG8(0x3782), 0xf5 },
+	{ CCI_REG8(0x3783), 0x06 },
+	{ CCI_REG8(0x3784), 0xa5 },
+	{ CCI_REG8(0x3788), 0x0f },
+	{ CCI_REG8(0x378a), 0xd9 },
+	{ CCI_REG8(0x378b), 0x03 },
+	{ CCI_REG8(0x378c), 0xeb },
+	{ CCI_REG8(0x378d), 0x05 },
+	{ CCI_REG8(0x378e), 0x87 },
+	{ CCI_REG8(0x378f), 0x06 },
+	{ CCI_REG8(0x3790), 0xf5 },
+	{ CCI_REG8(0x3792), 0x43 },
+	{ CCI_REG8(0x3794), 0x7a },
+	{ CCI_REG8(0x3796), 0xa1 },
+	{ CCI_REG8(0x37b0), 0x36 },
+	{ CCI_REG8(0x3a00), 0x00 },
 };
 
-static const struct imx335_reg raw10_framefmt_regs[] = {
-	{0x3050, 0x00},
-	{0x319d, 0x00},
-	{0x341c, 0xff},
-	{0x341d, 0x01},
+static const struct cci_reg_sequence raw10_framefmt_regs[] = {
+	{ IMX335_REG_ADBIT, 0x00 },
+	{ IMX335_REG_MDBIT, 0x00 },
+	{ IMX335_REG_ADBIT1, 0x1ff },
 };
 
-static const struct imx335_reg raw12_framefmt_regs[] = {
-	{0x3050, 0x01},
-	{0x319d, 0x01},
-	{0x341c, 0x47},
-	{0x341d, 0x00},
+static const struct cci_reg_sequence raw12_framefmt_regs[] = {
+	{ IMX335_REG_ADBIT, 0x01 },
+	{ IMX335_REG_MDBIT, 0x01 },
+	{ IMX335_REG_ADBIT1, 0x47 },
 };
 
-static const struct imx335_reg mipi_data_rate_1188Mbps[] = {
-	{0x300c, 0x3b},
-	{0x300d, 0x2a},
-	{0x314c, 0xc6},
-	{0x314d, 0x00},
-	{0x315a, 0x02},
-	{0x3168, 0xa0},
-	{0x316a, 0x7e},
-	{0x319e, 0x01},
-	{0x3a18, 0x8f},
-	{0x3a1a, 0x4f},
-	{0x3a1c, 0x47},
-	{0x3a1e, 0x37},
-	{0x3a1f, 0x01},
-	{0x3a20, 0x4f},
-	{0x3a22, 0x87},
-	{0x3a24, 0x4f},
-	{0x3a26, 0x7f},
-	{0x3a28, 0x3f},
+static const struct cci_reg_sequence mipi_data_rate_1188Mbps[] = {
+	{ IMX335_REG_BCWAIT_TIME, 0x3b },
+	{ IMX335_REG_CPWAIT_TIME, 0x2a },
+	{ IMX335_REG_INCLKSEL1, 0x00c6 },
+	{ IMX335_REG_INCLKSEL2, 0x02 },
+	{ IMX335_REG_INCLKSEL3, 0xa0 },
+	{ IMX335_REG_INCLKSEL4, 0x7e },
+	{ IMX335_REG_SYSMODE, 0x01 },
+	{ IMX335_REG_TCLKPOST, 0x8f },
+	{ IMX335_REG_TCLKPREPARE, 0x4f },
+	{ IMX335_REG_TCLK_TRAIL, 0x47 },
+	{ IMX335_REG_TCLK_ZERO, 0x0137 },
+	{ IMX335_REG_THS_PREPARE, 0x4f },
+	{ IMX335_REG_THS_ZERO,  0x87 },
+	{ IMX335_REG_THS_TRAIL, 0x4f },
+	{ IMX335_REG_THS_EXIT, 0x7f },
+	{ IMX335_REG_TPLX, 0x3f },
 };
 
-static const struct imx335_reg mipi_data_rate_891Mbps[] = {
-	{0x300c, 0x3b},
-	{0x300d, 0x2a},
-	{0x314c, 0x29},
-	{0x314d, 0x01},
-	{0x315a, 0x06},
-	{0x3168, 0xa0},
-	{0x316a, 0x7e},
-	{0x319e, 0x02},
-	{0x3a18, 0x7f},
-	{0x3a1a, 0x37},
-	{0x3a1c, 0x37},
-	{0x3a1e, 0xf7},
-	{0x3a20, 0x3f},
-	{0x3a22, 0x6f},
-	{0x3a24, 0x3f},
-	{0x3a26, 0x5f},
-	{0x3a28, 0x2f},
+static const struct cci_reg_sequence mipi_data_rate_891Mbps[] = {
+	{ IMX335_REG_BCWAIT_TIME, 0x3b },
+	{ IMX335_REG_CPWAIT_TIME, 0x2a },
+	{ IMX335_REG_INCLKSEL1, 0x0129 },
+	{ IMX335_REG_INCLKSEL2, 0x06 },
+	{ IMX335_REG_INCLKSEL3, 0xa0 },
+	{ IMX335_REG_INCLKSEL4, 0x7e },
+	{ IMX335_REG_SYSMODE, 0x02 },
+	{ IMX335_REG_TCLKPOST, 0x7f },
+	{ IMX335_REG_TCLKPREPARE, 0x37 },
+	{ IMX335_REG_TCLK_TRAIL, 0x37 },
+	{ IMX335_REG_TCLK_ZERO, 0xf7 },
+	{ IMX335_REG_THS_PREPARE, 0x3f },
+	{ IMX335_REG_THS_ZERO, 0x6f },
+	{ IMX335_REG_THS_TRAIL, 0x3f },
+	{ IMX335_REG_THS_EXIT, 0x5f },
+	{ IMX335_REG_TPLX, 0x2f },
 };
 
 static const s64 link_freq[] = {
@@ -402,101 +427,6 @@ static inline struct imx335 *to_imx335(struct v4l2_subdev *subdev)
 	return container_of(subdev, struct imx335, sd);
 }
 
-/**
- * imx335_read_reg() - Read registers.
- * @imx335: pointer to imx335 device
- * @reg: register address
- * @len: length of bytes to read. Max supported bytes is 4
- * @val: pointer to register value to be filled.
- *
- * Big endian register addresses with little endian values.
- *
- * Return: 0 if successful, error code otherwise.
- */
-static int imx335_read_reg(struct imx335 *imx335, u16 reg, u32 len, u32 *val)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&imx335->sd);
-	struct i2c_msg msgs[2] = {0};
-	u8 addr_buf[2] = {0};
-	u8 data_buf[4] = {0};
-	int ret;
-
-	if (WARN_ON(len > 4))
-		return -EINVAL;
-
-	put_unaligned_be16(reg, addr_buf);
-
-	/* 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;
-
-	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
-	if (ret != ARRAY_SIZE(msgs))
-		return -EIO;
-
-	*val = get_unaligned_le32(data_buf);
-
-	return 0;
-}
-
-/**
- * imx335_write_reg() - Write register
- * @imx335: pointer to imx335 device
- * @reg: register address
- * @len: length of bytes. Max supported bytes is 4
- * @val: register value
- *
- * Big endian register addresses with little endian values.
- *
- * Return: 0 if successful, error code otherwise.
- */
-static int imx335_write_reg(struct imx335 *imx335, u16 reg, u32 len, u32 val)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&imx335->sd);
-	u8 buf[6] = {0};
-
-	if (WARN_ON(len > 4))
-		return -EINVAL;
-
-	put_unaligned_be16(reg, buf);
-	put_unaligned_le32(val, buf + 2);
-	if (i2c_master_send(client, buf, len + 2) != len + 2)
-		return -EIO;
-
-	return 0;
-}
-
-/**
- * imx335_write_regs() - Write a list of registers
- * @imx335: pointer to imx335 device
- * @regs: list of registers to be written
- * @len: length of registers array
- *
- * Return: 0 if successful. error code otherwise.
- */
-static int imx335_write_regs(struct imx335 *imx335,
-			     const struct imx335_reg *regs, u32 len)
-{
-	unsigned int i;
-	int ret;
-
-	for (i = 0; i < len; i++) {
-		ret = imx335_write_reg(imx335, regs[i].address, 1, regs[i].val);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-
 /**
  * imx335_update_controls() - Update control ranges based on streaming mode
  * @imx335: pointer to imx335 device
@@ -533,7 +463,8 @@ static int imx335_update_controls(struct imx335 *imx335,
 static int imx335_update_exp_gain(struct imx335 *imx335, u32 exposure, u32 gain)
 {
 	u32 lpfr, shutter;
-	int ret;
+	int ret_hold;
+	int ret = 0;
 
 	lpfr = imx335->vblank + imx335->cur_mode->height;
 	shutter = lpfr - exposure;
@@ -541,64 +472,55 @@ static int imx335_update_exp_gain(struct imx335 *imx335, u32 exposure, u32 gain)
 	dev_dbg(imx335->dev, "Set exp %u, analog gain %u, shutter %u, lpfr %u\n",
 		exposure, gain, shutter, lpfr);
 
-	ret = imx335_write_reg(imx335, IMX335_REG_HOLD, 1, 1);
-	if (ret)
-		return ret;
-
-	ret = imx335_write_reg(imx335, IMX335_REG_LPFR, 3, lpfr);
-	if (ret)
-		goto error_release_group_hold;
-
-	ret = imx335_write_reg(imx335, IMX335_REG_SHUTTER, 3, shutter);
-	if (ret)
-		goto error_release_group_hold;
-
-	ret = imx335_write_reg(imx335, IMX335_REG_AGAIN, 2, gain);
-
-error_release_group_hold:
-	imx335_write_reg(imx335, IMX335_REG_HOLD, 1, 0);
+	cci_write(imx335->cci, IMX335_REG_HOLD, 1, &ret);
+	cci_write(imx335->cci, IMX335_REG_VMAX, lpfr, &ret);
+	cci_write(imx335->cci, IMX335_REG_SHUTTER, shutter, &ret);
+	cci_write(imx335->cci, IMX335_REG_AGAIN, gain, &ret);
+	/*
+	 * Unconditionally attempt to release the hold, but track the
+	 * error if the unhold itself fails.
+	 */
+	ret_hold = cci_write(imx335->cci, IMX335_REG_HOLD, 0, NULL);
+	if (ret_hold)
+		ret = ret_hold;
 
 	return ret;
 }
 
 static int imx335_update_test_pattern(struct imx335 *imx335, u32 pattern_index)
 {
-	int ret;
+	int ret = 0;
 
 	if (pattern_index >= ARRAY_SIZE(imx335_tpg_val))
 		return -EINVAL;
 
 	if (pattern_index) {
-		const struct imx335_reg tpg_enable_regs[] = {
-			{ 0x3148, 0x10 },
-			{ 0x3280, 0x00 },
-			{ 0x329c, 0x01 },
-			{ 0x32a0, 0x11 },
-			{ 0x3302, 0x00 },
-			{ 0x3303, 0x00 },
-			{ 0x336c, 0x00 },
+		const struct cci_reg_sequence tpg_enable_regs[] = {
+			{ IMX335_REG_TPG_TESTCLKEN, 0x10 },
+			{ IMX335_REG_TPG_DIG_CLP_MODE, 0x00 },
+			{ IMX335_REG_TPG_EN_DUOUT, 0x01 },
+			{ IMX335_REG_TPG_COLORWIDTH, 0x11 },
+			{ IMX335_REG_TPG_BLKLEVEL, 0x00 },
+			{ IMX335_REG_TPG_WRJ_OPEN, 0x00 },
 		};
 
-		ret = imx335_write_reg(imx335, IMX335_REG_TPG, 1,
-				       imx335_tpg_val[pattern_index]);
-		if (ret)
-			return ret;
+		cci_write(imx335->cci, IMX335_REG_TPG,
+			  imx335_tpg_val[pattern_index], &ret);
 
-		ret = imx335_write_regs(imx335, tpg_enable_regs,
-					ARRAY_SIZE(tpg_enable_regs));
+		cci_multi_reg_write(imx335->cci, tpg_enable_regs,
+				    ARRAY_SIZE(tpg_enable_regs), &ret);
 	} else {
-		const struct imx335_reg tpg_disable_regs[] = {
-			{ 0x3148, 0x00 },
-			{ 0x3280, 0x01 },
-			{ 0x329c, 0x00 },
-			{ 0x32a0, 0x10 },
-			{ 0x3302, 0x32 },
-			{ 0x3303, 0x00 },
-			{ 0x336c, 0x01 },
+		const struct cci_reg_sequence tpg_disable_regs[] = {
+			{ IMX335_REG_TPG_TESTCLKEN, 0x00 },
+			{ IMX335_REG_TPG_DIG_CLP_MODE, 0x01 },
+			{ IMX335_REG_TPG_EN_DUOUT, 0x00 },
+			{ IMX335_REG_TPG_COLORWIDTH, 0x10 },
+			{ IMX335_REG_TPG_BLKLEVEL, 0x32 },
+			{ IMX335_REG_TPG_WRJ_OPEN, 0x01 },
 		};
 
-		ret = imx335_write_regs(imx335, tpg_disable_regs,
-					ARRAY_SIZE(tpg_disable_regs));
+		cci_multi_reg_write(imx335->cci, tpg_disable_regs,
+				    ARRAY_SIZE(tpg_disable_regs), &ret);
 	}
 
 	return ret;
@@ -897,12 +819,14 @@ static int imx335_set_framefmt(struct imx335 *imx335)
 {
 	switch (imx335->cur_mbus_code) {
 	case MEDIA_BUS_FMT_SRGGB10_1X10:
-		return imx335_write_regs(imx335, raw10_framefmt_regs,
-					 ARRAY_SIZE(raw10_framefmt_regs));
+		return cci_multi_reg_write(imx335->cci, raw10_framefmt_regs,
+					   ARRAY_SIZE(raw10_framefmt_regs),
+					   NULL);
 
 	case MEDIA_BUS_FMT_SRGGB12_1X12:
-		return imx335_write_regs(imx335, raw12_framefmt_regs,
-					 ARRAY_SIZE(raw12_framefmt_regs));
+		return cci_multi_reg_write(imx335->cci, raw12_framefmt_regs,
+					   ARRAY_SIZE(raw12_framefmt_regs),
+					   NULL);
 	}
 
 	return -EINVAL;
@@ -921,7 +845,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
 
 	/* Setup PLL */
 	reg_list = &link_freq_reglist[__ffs(imx335->link_freq_bitmap)];
-	ret = imx335_write_regs(imx335, reg_list->regs, reg_list->num_of_regs);
+	ret = cci_multi_reg_write(imx335->cci, reg_list->regs,
+				  reg_list->num_of_regs, NULL);
 	if (ret) {
 		dev_err(imx335->dev, "%s failed to set plls\n", __func__);
 		return ret;
@@ -929,8 +854,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
 
 	/* Write sensor mode registers */
 	reg_list = &imx335->cur_mode->reg_list;
-	ret = imx335_write_regs(imx335, reg_list->regs,
-				reg_list->num_of_regs);
+	ret = cci_multi_reg_write(imx335->cci, reg_list->regs,
+				  reg_list->num_of_regs, NULL);
 	if (ret) {
 		dev_err(imx335->dev, "fail to write initial registers\n");
 		return ret;
@@ -944,7 +869,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
 	}
 
 	/* Configure lanes */
-	ret = imx335_write_reg(imx335, IMX335_LANEMODE, 1, imx335->lane_mode);
+	ret = cci_write(imx335->cci, IMX335_REG_LANEMODE,
+			imx335->lane_mode, NULL);
 	if (ret)
 		return ret;
 
@@ -956,8 +882,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
 	}
 
 	/* Start streaming */
-	ret = imx335_write_reg(imx335, IMX335_REG_MODE_SELECT,
-			       1, IMX335_MODE_STREAMING);
+	ret = cci_write(imx335->cci, IMX335_REG_MODE_SELECT,
+			IMX335_MODE_STREAMING, NULL);
 	if (ret) {
 		dev_err(imx335->dev, "fail to start streaming\n");
 		return ret;
@@ -977,8 +903,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
  */
 static int imx335_stop_streaming(struct imx335 *imx335)
 {
-	return imx335_write_reg(imx335, IMX335_REG_MODE_SELECT,
-				1, IMX335_MODE_STANDBY);
+	return cci_write(imx335->cci, IMX335_REG_MODE_SELECT,
+			 IMX335_MODE_STANDBY, NULL);
 }
 
 /**
@@ -1029,14 +955,14 @@ static int imx335_set_stream(struct v4l2_subdev *sd, int enable)
 static int imx335_detect(struct imx335 *imx335)
 {
 	int ret;
-	u32 val;
+	u64 val;
 
-	ret = imx335_read_reg(imx335, IMX335_REG_ID, 2, &val);
+	ret = cci_read(imx335->cci, IMX335_REG_ID, &val, NULL);
 	if (ret)
 		return ret;
 
 	if (val != IMX335_ID) {
-		dev_err(imx335->dev, "chip id mismatch: %x!=%x\n",
+		dev_err(imx335->dev, "chip id mismatch: %x!=%llx\n",
 			IMX335_ID, val);
 		return -ENXIO;
 	}
@@ -1331,6 +1257,11 @@ static int imx335_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	imx335->dev = &client->dev;
+	imx335->cci = devm_cci_regmap_init_i2c(client, 16);
+	if (IS_ERR(imx335->cci)) {
+		dev_err(imx335->dev, "Unable to initialize I2C\n");
+		return -ENODEV;
+	}
 
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&imx335->sd, client, &imx335_subdev_ops);
-- 
2.43.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v6 3/6] media: imx335: Use V4L2 CCI for accessing sensor registers
  2024-04-12 16:00 ` [PATCH v6 3/6] media: imx335: Use V4L2 CCI for accessing sensor registers Umang Jain
@ 2024-04-12 21:23   ` Kieran Bingham
  0 siblings, 0 replies; 8+ messages in thread
From: Kieran Bingham @ 2024-04-12 21:23 UTC (permalink / raw)
  To: Umang Jain, linux-media
  Cc: Alexander Shiyan, Sakari Ailus, open list, Umang Jain

Quoting Umang Jain (2024-04-12 17:00:36)
> Use the new comon CCI register access helpers to replace the private
> register access helpers in the imx335 driver.
> 
> Select V4L2_CCI_I2C Kconfig option which the imx335 driver now
> depends on.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  drivers/media/i2c/Kconfig  |   1 +
>  drivers/media/i2c/imx335.c | 593 ++++++++++++++++---------------------
>  2 files changed, 263 insertions(+), 331 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 56f276b920ab..8d248b9c9562 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -195,6 +195,7 @@ config VIDEO_IMX334
>  config VIDEO_IMX335
>         tristate "Sony IMX335 sensor support"
>         depends on OF_GPIO
> +       select V4L2_CCI_I2C
>         help
>           This is a Video4Linux2 sensor driver for the Sony
>           IMX335 camera.
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index cebe8cfd77be..ee22304f827e 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -11,62 +11,104 @@
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
>  
> +#include <media/v4l2-cci.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
>  /* Streaming Mode */
> -#define IMX335_REG_MODE_SELECT 0x3000
> -#define IMX335_MODE_STANDBY    0x01
> -#define IMX335_MODE_STREAMING  0x00
> +#define IMX335_REG_MODE_SELECT         CCI_REG8(0x3000)
> +#define IMX335_MODE_STANDBY            0x01
> +#define IMX335_MODE_STREAMING          0x00
>  
> -/* Data Lanes */
> -#define IMX335_LANEMODE                0x3a01
> -#define IMX335_2LANE           1
> -#define IMX335_4LANE           3
> +/* Group hold register */
> +#define IMX335_REG_HOLD                        CCI_REG8(0x3001)
> +
> +#define IMX335_REG_MASTER_MODE         CCI_REG8(0x3002)
> +#define IMX335_REG_BCWAIT_TIME         CCI_REG8(0x300c)
> +#define IMX335_REG_CPWAIT_TIME         CCI_REG8(0x300d)
> +#define IMX335_REG_WINMODE             CCI_REG8(0x3018)
> +#define IMX335_REG_HTRIMMING_START     CCI_REG16_LE(0x302c)
> +#define IMX335_REG_HNUM                        CCI_REG8(0x302e)
>  
>  /* Lines per frame */
> -#define IMX335_REG_LPFR                0x3030
> +#define IMX335_REG_VMAX                        CCI_REG24_LE(0x3030)
>  
> -/* Chip ID */
> -#define IMX335_REG_ID          0x3912
> -#define IMX335_ID              0x00
> +#define IMX335_REG_OPB_SIZE_V          CCI_REG8(0x304c)
> +#define IMX335_REG_ADBIT               CCI_REG8(0x3050)
> +#define IMX335_REG_Y_OUT_SIZE          CCI_REG16_LE(0x3056)
>  
> -/* Exposure control */
> -#define IMX335_REG_SHUTTER     0x3058
> -#define IMX335_EXPOSURE_MIN    1
> -#define IMX335_EXPOSURE_OFFSET 9
> -#define IMX335_EXPOSURE_STEP   1
> -#define IMX335_EXPOSURE_DEFAULT        0x0648
> +#define IMX335_REG_SHUTTER             CCI_REG24_LE(0x3058)

As LPFR was corrected to VMAX to match the datasheet, I wonder if this
should be SHR0, but SHUTTER is fine with me too.


> +#define IMX335_EXPOSURE_MIN            1
> +#define IMX335_EXPOSURE_OFFSET         9
> +#define IMX335_EXPOSURE_STEP           1
> +#define IMX335_EXPOSURE_DEFAULT                0x0648
>  
> -/* Analog gain control */
> -#define IMX335_REG_AGAIN       0x30e8
> -#define IMX335_AGAIN_MIN       0
> -#define IMX335_AGAIN_MAX       240
> -#define IMX335_AGAIN_STEP      1
> -#define IMX335_AGAIN_DEFAULT   0
> +#define IMX335_REG_AREA3_ST_ADR_1      CCI_REG16_LE(0x3074)
> +#define IMX335_REG_AREA3_WIDTH_1       CCI_REG16_LE(0x3076)
>  
> -/* Group hold register */
> -#define IMX335_REG_HOLD                0x3001
> +/* Analog gain control */
> +#define IMX335_REG_AGAIN               CCI_REG8(0x30e8)

There's only a single GAIN register, and it covers both analogue and
digital (as we see in patch 6/6), so this perhaps should really be
REG_GAIN. Again, not critical or could be handled in 6/6, or not at all
as 6/6 makes this only use Analogue Gain anyway.

> +#define IMX335_AGAIN_MIN               0
> +#define IMX335_AGAIN_MAX               240
> +#define IMX335_AGAIN_STEP              1
> +#define IMX335_AGAIN_DEFAULT           0
> +
> +#define IMX335_REG_TPG_TESTCLKEN       CCI_REG8(0x3148)
> +#define IMX335_REG_INCLKSEL1           CCI_REG16_LE(0x314c)
> +#define IMX335_REG_INCLKSEL2           CCI_REG8(0x315a)
> +#define IMX335_REG_INCLKSEL3           CCI_REG8(0x3168)
> +#define IMX335_REG_INCLKSEL4           CCI_REG8(0x316a)

These look like they are called INCKSELn in the datasheet. Maybe keep
them as their own group too.

> +#define IMX335_REG_MDBIT               CCI_REG8(0x319d)
> +#define IMX335_REG_SYSMODE             CCI_REG8(0x319e)
> +
> +#define IMX335_REG_XVS_XHS_DRV         CCI_REG8(0x31a1)
>  
>  /* Test pattern generator */
> -#define IMX335_REG_TPG         0x329e
> -#define IMX335_TPG_ALL_000     0
> -#define IMX335_TPG_ALL_FFF     1
> -#define IMX335_TPG_ALL_555     2
> -#define IMX335_TPG_ALL_AAA     3
> -#define IMX335_TPG_TOG_555_AAA 4
> -#define IMX335_TPG_TOG_AAA_555 5
> -#define IMX335_TPG_TOG_000_555 6
> -#define IMX335_TPG_TOG_555_000 7
> -#define IMX335_TPG_TOG_000_FFF 8
> -#define IMX335_TPG_TOG_FFF_000 9
> -#define IMX335_TPG_H_COLOR_BARS 10
> -#define IMX335_TPG_V_COLOR_BARS 11
> +#define IMX335_REG_TPG_DIG_CLP_MODE    CCI_REG8(0x3280)
> +#define IMX335_REG_TPG_EN_DUOUT                CCI_REG8(0x329c)
> +#define IMX335_REG_TPG                 CCI_REG8(0x329e)
> +#define IMX335_REG_TPG_COLORWIDTH      CCI_REG8(0x32a0)
> +#define IMX335_REG_TPG_BLKLEVEL                CCI_REG16_LE(0x3302)

This isn't the TPG BLKLEVEL. It's just the BLKLEVEL

> +#define IMX335_REG_TPG_WRJ_OPEN                CCI_REG8(0x336c)

This is just REG_WRJ_OPEN

> +#define IMX335_TPG_ALL_000             0
> +#define IMX335_TPG_ALL_FFF             1
> +#define IMX335_TPG_ALL_555             2
> +#define IMX335_TPG_ALL_AAA             3
> +#define IMX335_TPG_TOG_555_AAA         4
> +#define IMX335_TPG_TOG_AAA_555         5
> +#define IMX335_TPG_TOG_000_555         6
> +#define IMX335_TPG_TOG_555_000         7
> +#define IMX335_TPG_TOG_000_FFF         8
> +#define IMX335_TPG_TOG_FFF_000         9
> +#define IMX335_TPG_H_COLOR_BARS                10
> +#define IMX335_TPG_V_COLOR_BARS                11

The TPG values are written to the REG_TPG, so they should follow that.

> +
> +#define IMX335_REG_ADBIT1              CCI_REG16_LE(0x341c)
> +
> +/* Chip ID */
> +#define IMX335_REG_ID                  CCI_REG8(0x3912)
> +#define IMX335_ID                      0x00
> +
> +/* Data Lanes */
> +#define IMX335_REG_LANEMODE            CCI_REG8(0x3a01)
> +#define IMX335_2LANE                   1
> +#define IMX335_4LANE                   3
> +
> +#define IMX335_REG_TCLKPOST            CCI_REG16_LE(0x3a18)
> +#define IMX335_REG_TCLKPREPARE         CCI_REG16_LE(0x3a1a)
> +#define IMX335_REG_TCLK_TRAIL          CCI_REG16_LE(0x3a1c)
> +#define IMX335_REG_TCLK_ZERO           CCI_REG16_LE(0x3a1e)
> +#define IMX335_REG_THS_PREPARE         CCI_REG16_LE(0x3a20)
> +#define IMX335_REG_THS_ZERO            CCI_REG16_LE(0x3a22)
> +#define IMX335_REG_THS_TRAIL           CCI_REG16_LE(0x3a24)
> +#define IMX335_REG_THS_EXIT            CCI_REG16_LE(0x3a26)

Yes, the datasheet is clearly wrong here ;-)

(And I believe what you have is correct here).

> +#define IMX335_REG_TPLX                        CCI_REG16_LE(0x3a28)
>  
>  /* Input clock rate */
> -#define IMX335_INCLK_RATE      24000000
> +#define IMX335_INCLK_RATE              24000000
>  
>  /* CSI2 HW configuration */
>  #define IMX335_LINK_FREQ_594MHz                594000000LL
> @@ -74,9 +116,6 @@
>  
>  #define IMX335_NUM_DATA_LANES  4
>  
> -#define IMX335_REG_MIN         0x00
> -#define IMX335_REG_MAX         0xfffff
> -
>  /* IMX335 native and active pixel array size. */
>  #define IMX335_NATIVE_WIDTH            2616U
>  #define IMX335_NATIVE_HEIGHT           1964U
> @@ -85,16 +124,6 @@
>  #define IMX335_PIXEL_ARRAY_WIDTH       2592U
>  #define IMX335_PIXEL_ARRAY_HEIGHT      1944U
>  
> -/**
> - * struct imx335_reg - imx335 sensor register
> - * @address: Register address
> - * @val: Register value
> - */
> -struct imx335_reg {
> -       u16 address;
> -       u8 val;
> -};
> -
>  /**
>   * struct imx335_reg_list - imx335 sensor register list
>   * @num_of_regs: Number of registers in the list
> @@ -102,7 +131,7 @@ struct imx335_reg {
>   */
>  struct imx335_reg_list {
>         u32 num_of_regs;
> -       const struct imx335_reg *regs;
> +       const struct cci_reg_sequence *regs;
>  };
>  
>  static const char * const imx335_supply_name[] = {
> @@ -165,6 +194,7 @@ struct imx335 {
>         struct media_pad pad;
>         struct gpio_desc *reset_gpio;
>         struct regulator_bulk_data supplies[ARRAY_SIZE(imx335_supply_name)];
> +       struct regmap *cci;
>  
>         struct clk *inclk;
>         struct v4l2_ctrl_handler ctrl_handler;
> @@ -217,140 +247,135 @@ static const int imx335_tpg_val[] = {
>  };
>  
>  /* Sensor mode registers */
> -static const struct imx335_reg mode_2592x1940_regs[] = {
> -       {0x3000, 0x01},
> -       {0x3002, 0x00},
> -       {0x3018, 0x04},
> -       {0x302c, 0x3c},
> -       {0x302e, 0x20},
> -       {0x3056, 0x94},
> -       {0x3074, 0xc8},
> -       {0x3076, 0x28},
> -       {0x304c, 0x00},
> -       {0x31a1, 0x00},
> -       {0x3288, 0x21},
> -       {0x328a, 0x02},
> -       {0x3414, 0x05},
> -       {0x3416, 0x18},
> -       {0x3648, 0x01},
> -       {0x364a, 0x04},
> -       {0x364c, 0x04},
> -       {0x3678, 0x01},
> -       {0x367c, 0x31},
> -       {0x367e, 0x31},
> -       {0x3706, 0x10},
> -       {0x3708, 0x03},
> -       {0x3714, 0x02},
> -       {0x3715, 0x02},
> -       {0x3716, 0x01},
> -       {0x3717, 0x03},
> -       {0x371c, 0x3d},
> -       {0x371d, 0x3f},
> -       {0x372c, 0x00},
> -       {0x372d, 0x00},
> -       {0x372e, 0x46},
> -       {0x372f, 0x00},
> -       {0x3730, 0x89},
> -       {0x3731, 0x00},
> -       {0x3732, 0x08},
> -       {0x3733, 0x01},
> -       {0x3734, 0xfe},
> -       {0x3735, 0x05},
> -       {0x3740, 0x02},
> -       {0x375d, 0x00},
> -       {0x375e, 0x00},
> -       {0x375f, 0x11},
> -       {0x3760, 0x01},
> -       {0x3768, 0x1b},
> -       {0x3769, 0x1b},
> -       {0x376a, 0x1b},
> -       {0x376b, 0x1b},
> -       {0x376c, 0x1a},
> -       {0x376d, 0x17},
> -       {0x376e, 0x0f},
> -       {0x3776, 0x00},
> -       {0x3777, 0x00},
> -       {0x3778, 0x46},
> -       {0x3779, 0x00},
> -       {0x377a, 0x89},
> -       {0x377b, 0x00},
> -       {0x377c, 0x08},
> -       {0x377d, 0x01},
> -       {0x377e, 0x23},
> -       {0x377f, 0x02},
> -       {0x3780, 0xd9},
> -       {0x3781, 0x03},
> -       {0x3782, 0xf5},
> -       {0x3783, 0x06},
> -       {0x3784, 0xa5},
> -       {0x3788, 0x0f},
> -       {0x378a, 0xd9},
> -       {0x378b, 0x03},
> -       {0x378c, 0xeb},
> -       {0x378d, 0x05},
> -       {0x378e, 0x87},
> -       {0x378f, 0x06},
> -       {0x3790, 0xf5},
> -       {0x3792, 0x43},
> -       {0x3794, 0x7a},
> -       {0x3796, 0xa1},
> -       {0x37b0, 0x36},
> -       {0x3a00, 0x00},
> +static const struct cci_reg_sequence mode_2592x1940_regs[] = {
> +       { IMX335_REG_MODE_SELECT, 0x01 },

s/0x01/IMX335_MODE_STANDBY/ ?


Fairly minor adjustments. So with those handled however you think is
best,


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +       { IMX335_REG_MASTER_MODE, 0x00 },
> +       { IMX335_REG_WINMODE, 0x04 },
> +       { IMX335_REG_HTRIMMING_START, 0x0180 },
> +       { IMX335_REG_HNUM, 0x0a20 },
> +       { IMX335_REG_Y_OUT_SIZE, 0x0794 },
> +       { IMX335_REG_AREA3_ST_ADR_1, 0x00b0 },
> +       { IMX335_REG_AREA3_WIDTH_1, 0x0f58 },
> +       { IMX335_REG_OPB_SIZE_V, 0x00 },
> +       { IMX335_REG_XVS_XHS_DRV, 0x00 },
> +       { CCI_REG8(0x3288), 0x21 },
> +       { CCI_REG8(0x328a), 0x02 },
> +       { CCI_REG8(0x3414), 0x05 },
> +       { CCI_REG8(0x3416), 0x18 },
> +       { CCI_REG8(0x3648), 0x01 },
> +       { CCI_REG8(0x364a), 0x04 },
> +       { CCI_REG8(0x364c), 0x04 },
> +       { CCI_REG8(0x3678), 0x01 },
> +       { CCI_REG8(0x367c), 0x31 },
> +       { CCI_REG8(0x367e), 0x31 },
> +       { CCI_REG8(0x3706), 0x10 },
> +       { CCI_REG8(0x3708), 0x03 },
> +       { CCI_REG8(0x3714), 0x02 },
> +       { CCI_REG8(0x3715), 0x02 },
> +       { CCI_REG8(0x3716), 0x01 },
> +       { CCI_REG8(0x3717), 0x03 },
> +       { CCI_REG8(0x371c), 0x3d },
> +       { CCI_REG8(0x371d), 0x3f },
> +       { CCI_REG8(0x372c), 0x00 },
> +       { CCI_REG8(0x372d), 0x00 },
> +       { CCI_REG8(0x372e), 0x46 },
> +       { CCI_REG8(0x372f), 0x00 },
> +       { CCI_REG8(0x3730), 0x89 },
> +       { CCI_REG8(0x3731), 0x00 },
> +       { CCI_REG8(0x3732), 0x08 },
> +       { CCI_REG8(0x3733), 0x01 },
> +       { CCI_REG8(0x3734), 0xfe },
> +       { CCI_REG8(0x3735), 0x05 },
> +       { CCI_REG8(0x3740), 0x02 },
> +       { CCI_REG8(0x375d), 0x00 },
> +       { CCI_REG8(0x375e), 0x00 },
> +       { CCI_REG8(0x375f), 0x11 },
> +       { CCI_REG8(0x3760), 0x01 },
> +       { CCI_REG8(0x3768), 0x1b },
> +       { CCI_REG8(0x3769), 0x1b },
> +       { CCI_REG8(0x376a), 0x1b },
> +       { CCI_REG8(0x376b), 0x1b },
> +       { CCI_REG8(0x376c), 0x1a },
> +       { CCI_REG8(0x376d), 0x17 },
> +       { CCI_REG8(0x376e), 0x0f },
> +       { CCI_REG8(0x3776), 0x00 },
> +       { CCI_REG8(0x3777), 0x00 },
> +       { CCI_REG8(0x3778), 0x46 },
> +       { CCI_REG8(0x3779), 0x00 },
> +       { CCI_REG8(0x377a), 0x89 },
> +       { CCI_REG8(0x377b), 0x00 },
> +       { CCI_REG8(0x377c), 0x08 },
> +       { CCI_REG8(0x377d), 0x01 },
> +       { CCI_REG8(0x377e), 0x23 },
> +       { CCI_REG8(0x377f), 0x02 },
> +       { CCI_REG8(0x3780), 0xd9 },
> +       { CCI_REG8(0x3781), 0x03 },
> +       { CCI_REG8(0x3782), 0xf5 },
> +       { CCI_REG8(0x3783), 0x06 },
> +       { CCI_REG8(0x3784), 0xa5 },
> +       { CCI_REG8(0x3788), 0x0f },
> +       { CCI_REG8(0x378a), 0xd9 },
> +       { CCI_REG8(0x378b), 0x03 },
> +       { CCI_REG8(0x378c), 0xeb },
> +       { CCI_REG8(0x378d), 0x05 },
> +       { CCI_REG8(0x378e), 0x87 },
> +       { CCI_REG8(0x378f), 0x06 },
> +       { CCI_REG8(0x3790), 0xf5 },
> +       { CCI_REG8(0x3792), 0x43 },
> +       { CCI_REG8(0x3794), 0x7a },
> +       { CCI_REG8(0x3796), 0xa1 },
> +       { CCI_REG8(0x37b0), 0x36 },
> +       { CCI_REG8(0x3a00), 0x00 },
>  };
>  
> -static const struct imx335_reg raw10_framefmt_regs[] = {
> -       {0x3050, 0x00},
> -       {0x319d, 0x00},
> -       {0x341c, 0xff},
> -       {0x341d, 0x01},
> +static const struct cci_reg_sequence raw10_framefmt_regs[] = {
> +       { IMX335_REG_ADBIT, 0x00 },
> +       { IMX335_REG_MDBIT, 0x00 },
> +       { IMX335_REG_ADBIT1, 0x1ff },
>  };
>  
> -static const struct imx335_reg raw12_framefmt_regs[] = {
> -       {0x3050, 0x01},
> -       {0x319d, 0x01},
> -       {0x341c, 0x47},
> -       {0x341d, 0x00},
> +static const struct cci_reg_sequence raw12_framefmt_regs[] = {
> +       { IMX335_REG_ADBIT, 0x01 },
> +       { IMX335_REG_MDBIT, 0x01 },
> +       { IMX335_REG_ADBIT1, 0x47 },
>  };
>  
> -static const struct imx335_reg mipi_data_rate_1188Mbps[] = {
> -       {0x300c, 0x3b},
> -       {0x300d, 0x2a},
> -       {0x314c, 0xc6},
> -       {0x314d, 0x00},
> -       {0x315a, 0x02},
> -       {0x3168, 0xa0},
> -       {0x316a, 0x7e},
> -       {0x319e, 0x01},
> -       {0x3a18, 0x8f},
> -       {0x3a1a, 0x4f},
> -       {0x3a1c, 0x47},
> -       {0x3a1e, 0x37},
> -       {0x3a1f, 0x01},
> -       {0x3a20, 0x4f},
> -       {0x3a22, 0x87},
> -       {0x3a24, 0x4f},
> -       {0x3a26, 0x7f},
> -       {0x3a28, 0x3f},
> +static const struct cci_reg_sequence mipi_data_rate_1188Mbps[] = {
> +       { IMX335_REG_BCWAIT_TIME, 0x3b },
> +       { IMX335_REG_CPWAIT_TIME, 0x2a },
> +       { IMX335_REG_INCLKSEL1, 0x00c6 },
> +       { IMX335_REG_INCLKSEL2, 0x02 },
> +       { IMX335_REG_INCLKSEL3, 0xa0 },
> +       { IMX335_REG_INCLKSEL4, 0x7e },
> +       { IMX335_REG_SYSMODE, 0x01 },
> +       { IMX335_REG_TCLKPOST, 0x8f },
> +       { IMX335_REG_TCLKPREPARE, 0x4f },
> +       { IMX335_REG_TCLK_TRAIL, 0x47 },
> +       { IMX335_REG_TCLK_ZERO, 0x0137 },
> +       { IMX335_REG_THS_PREPARE, 0x4f },
> +       { IMX335_REG_THS_ZERO,  0x87 },
> +       { IMX335_REG_THS_TRAIL, 0x4f },
> +       { IMX335_REG_THS_EXIT, 0x7f },
> +       { IMX335_REG_TPLX, 0x3f },
>  };
>  
> -static const struct imx335_reg mipi_data_rate_891Mbps[] = {
> -       {0x300c, 0x3b},
> -       {0x300d, 0x2a},
> -       {0x314c, 0x29},
> -       {0x314d, 0x01},
> -       {0x315a, 0x06},
> -       {0x3168, 0xa0},
> -       {0x316a, 0x7e},
> -       {0x319e, 0x02},
> -       {0x3a18, 0x7f},
> -       {0x3a1a, 0x37},
> -       {0x3a1c, 0x37},
> -       {0x3a1e, 0xf7},
> -       {0x3a20, 0x3f},
> -       {0x3a22, 0x6f},
> -       {0x3a24, 0x3f},
> -       {0x3a26, 0x5f},
> -       {0x3a28, 0x2f},
> +static const struct cci_reg_sequence mipi_data_rate_891Mbps[] = {
> +       { IMX335_REG_BCWAIT_TIME, 0x3b },
> +       { IMX335_REG_CPWAIT_TIME, 0x2a },
> +       { IMX335_REG_INCLKSEL1, 0x0129 },
> +       { IMX335_REG_INCLKSEL2, 0x06 },
> +       { IMX335_REG_INCLKSEL3, 0xa0 },
> +       { IMX335_REG_INCLKSEL4, 0x7e },
> +       { IMX335_REG_SYSMODE, 0x02 },
> +       { IMX335_REG_TCLKPOST, 0x7f },
> +       { IMX335_REG_TCLKPREPARE, 0x37 },
> +       { IMX335_REG_TCLK_TRAIL, 0x37 },
> +       { IMX335_REG_TCLK_ZERO, 0xf7 },
> +       { IMX335_REG_THS_PREPARE, 0x3f },
> +       { IMX335_REG_THS_ZERO, 0x6f },
> +       { IMX335_REG_THS_TRAIL, 0x3f },
> +       { IMX335_REG_THS_EXIT, 0x5f },
> +       { IMX335_REG_TPLX, 0x2f },
>  };
>  
>  static const s64 link_freq[] = {
> @@ -402,101 +427,6 @@ static inline struct imx335 *to_imx335(struct v4l2_subdev *subdev)
>         return container_of(subdev, struct imx335, sd);
>  }
>  
> -/**
> - * imx335_read_reg() - Read registers.
> - * @imx335: pointer to imx335 device
> - * @reg: register address
> - * @len: length of bytes to read. Max supported bytes is 4
> - * @val: pointer to register value to be filled.
> - *
> - * Big endian register addresses with little endian values.
> - *
> - * Return: 0 if successful, error code otherwise.
> - */
> -static int imx335_read_reg(struct imx335 *imx335, u16 reg, u32 len, u32 *val)
> -{
> -       struct i2c_client *client = v4l2_get_subdevdata(&imx335->sd);
> -       struct i2c_msg msgs[2] = {0};
> -       u8 addr_buf[2] = {0};
> -       u8 data_buf[4] = {0};
> -       int ret;
> -
> -       if (WARN_ON(len > 4))
> -               return -EINVAL;
> -
> -       put_unaligned_be16(reg, addr_buf);
> -
> -       /* 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;
> -
> -       ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> -       if (ret != ARRAY_SIZE(msgs))
> -               return -EIO;
> -
> -       *val = get_unaligned_le32(data_buf);
> -
> -       return 0;
> -}
> -
> -/**
> - * imx335_write_reg() - Write register
> - * @imx335: pointer to imx335 device
> - * @reg: register address
> - * @len: length of bytes. Max supported bytes is 4
> - * @val: register value
> - *
> - * Big endian register addresses with little endian values.
> - *
> - * Return: 0 if successful, error code otherwise.
> - */
> -static int imx335_write_reg(struct imx335 *imx335, u16 reg, u32 len, u32 val)
> -{
> -       struct i2c_client *client = v4l2_get_subdevdata(&imx335->sd);
> -       u8 buf[6] = {0};
> -
> -       if (WARN_ON(len > 4))
> -               return -EINVAL;
> -
> -       put_unaligned_be16(reg, buf);
> -       put_unaligned_le32(val, buf + 2);
> -       if (i2c_master_send(client, buf, len + 2) != len + 2)
> -               return -EIO;
> -
> -       return 0;
> -}
> -
> -/**
> - * imx335_write_regs() - Write a list of registers
> - * @imx335: pointer to imx335 device
> - * @regs: list of registers to be written
> - * @len: length of registers array
> - *
> - * Return: 0 if successful. error code otherwise.
> - */
> -static int imx335_write_regs(struct imx335 *imx335,
> -                            const struct imx335_reg *regs, u32 len)
> -{
> -       unsigned int i;
> -       int ret;
> -
> -       for (i = 0; i < len; i++) {
> -               ret = imx335_write_reg(imx335, regs[i].address, 1, regs[i].val);
> -               if (ret)
> -                       return ret;
> -       }
> -
> -       return 0;
> -}
> -
>  /**
>   * imx335_update_controls() - Update control ranges based on streaming mode
>   * @imx335: pointer to imx335 device
> @@ -533,7 +463,8 @@ static int imx335_update_controls(struct imx335 *imx335,
>  static int imx335_update_exp_gain(struct imx335 *imx335, u32 exposure, u32 gain)
>  {
>         u32 lpfr, shutter;
> -       int ret;
> +       int ret_hold;
> +       int ret = 0;
>  
>         lpfr = imx335->vblank + imx335->cur_mode->height;
>         shutter = lpfr - exposure;
> @@ -541,64 +472,55 @@ static int imx335_update_exp_gain(struct imx335 *imx335, u32 exposure, u32 gain)
>         dev_dbg(imx335->dev, "Set exp %u, analog gain %u, shutter %u, lpfr %u\n",
>                 exposure, gain, shutter, lpfr);
>  
> -       ret = imx335_write_reg(imx335, IMX335_REG_HOLD, 1, 1);
> -       if (ret)
> -               return ret;
> -
> -       ret = imx335_write_reg(imx335, IMX335_REG_LPFR, 3, lpfr);
> -       if (ret)
> -               goto error_release_group_hold;
> -
> -       ret = imx335_write_reg(imx335, IMX335_REG_SHUTTER, 3, shutter);
> -       if (ret)
> -               goto error_release_group_hold;
> -
> -       ret = imx335_write_reg(imx335, IMX335_REG_AGAIN, 2, gain);
> -
> -error_release_group_hold:
> -       imx335_write_reg(imx335, IMX335_REG_HOLD, 1, 0);
> +       cci_write(imx335->cci, IMX335_REG_HOLD, 1, &ret);
> +       cci_write(imx335->cci, IMX335_REG_VMAX, lpfr, &ret);
> +       cci_write(imx335->cci, IMX335_REG_SHUTTER, shutter, &ret);
> +       cci_write(imx335->cci, IMX335_REG_AGAIN, gain, &ret);
> +       /*
> +        * Unconditionally attempt to release the hold, but track the
> +        * error if the unhold itself fails.
> +        */
> +       ret_hold = cci_write(imx335->cci, IMX335_REG_HOLD, 0, NULL);
> +       if (ret_hold)
> +               ret = ret_hold;
>  
>         return ret;
>  }
>  
>  static int imx335_update_test_pattern(struct imx335 *imx335, u32 pattern_index)
>  {
> -       int ret;
> +       int ret = 0;
>  
>         if (pattern_index >= ARRAY_SIZE(imx335_tpg_val))
>                 return -EINVAL;
>  
>         if (pattern_index) {
> -               const struct imx335_reg tpg_enable_regs[] = {
> -                       { 0x3148, 0x10 },
> -                       { 0x3280, 0x00 },
> -                       { 0x329c, 0x01 },
> -                       { 0x32a0, 0x11 },
> -                       { 0x3302, 0x00 },
> -                       { 0x3303, 0x00 },
> -                       { 0x336c, 0x00 },
> +               const struct cci_reg_sequence tpg_enable_regs[] = {
> +                       { IMX335_REG_TPG_TESTCLKEN, 0x10 },
> +                       { IMX335_REG_TPG_DIG_CLP_MODE, 0x00 },
> +                       { IMX335_REG_TPG_EN_DUOUT, 0x01 },
> +                       { IMX335_REG_TPG_COLORWIDTH, 0x11 },
> +                       { IMX335_REG_TPG_BLKLEVEL, 0x00 },
> +                       { IMX335_REG_TPG_WRJ_OPEN, 0x00 },
>                 };
>  
> -               ret = imx335_write_reg(imx335, IMX335_REG_TPG, 1,
> -                                      imx335_tpg_val[pattern_index]);
> -               if (ret)
> -                       return ret;
> +               cci_write(imx335->cci, IMX335_REG_TPG,
> +                         imx335_tpg_val[pattern_index], &ret);
>  
> -               ret = imx335_write_regs(imx335, tpg_enable_regs,
> -                                       ARRAY_SIZE(tpg_enable_regs));
> +               cci_multi_reg_write(imx335->cci, tpg_enable_regs,
> +                                   ARRAY_SIZE(tpg_enable_regs), &ret);
>         } else {
> -               const struct imx335_reg tpg_disable_regs[] = {
> -                       { 0x3148, 0x00 },
> -                       { 0x3280, 0x01 },
> -                       { 0x329c, 0x00 },
> -                       { 0x32a0, 0x10 },
> -                       { 0x3302, 0x32 },
> -                       { 0x3303, 0x00 },
> -                       { 0x336c, 0x01 },
> +               const struct cci_reg_sequence tpg_disable_regs[] = {
> +                       { IMX335_REG_TPG_TESTCLKEN, 0x00 },
> +                       { IMX335_REG_TPG_DIG_CLP_MODE, 0x01 },
> +                       { IMX335_REG_TPG_EN_DUOUT, 0x00 },
> +                       { IMX335_REG_TPG_COLORWIDTH, 0x10 },
> +                       { IMX335_REG_TPG_BLKLEVEL, 0x32 },
> +                       { IMX335_REG_TPG_WRJ_OPEN, 0x01 },
>                 };
>  
> -               ret = imx335_write_regs(imx335, tpg_disable_regs,
> -                                       ARRAY_SIZE(tpg_disable_regs));
> +               cci_multi_reg_write(imx335->cci, tpg_disable_regs,
> +                                   ARRAY_SIZE(tpg_disable_regs), &ret);
>         }
>  
>         return ret;
> @@ -897,12 +819,14 @@ static int imx335_set_framefmt(struct imx335 *imx335)
>  {
>         switch (imx335->cur_mbus_code) {
>         case MEDIA_BUS_FMT_SRGGB10_1X10:
> -               return imx335_write_regs(imx335, raw10_framefmt_regs,
> -                                        ARRAY_SIZE(raw10_framefmt_regs));
> +               return cci_multi_reg_write(imx335->cci, raw10_framefmt_regs,
> +                                          ARRAY_SIZE(raw10_framefmt_regs),
> +                                          NULL);
>  
>         case MEDIA_BUS_FMT_SRGGB12_1X12:
> -               return imx335_write_regs(imx335, raw12_framefmt_regs,
> -                                        ARRAY_SIZE(raw12_framefmt_regs));
> +               return cci_multi_reg_write(imx335->cci, raw12_framefmt_regs,
> +                                          ARRAY_SIZE(raw12_framefmt_regs),
> +                                          NULL);
>         }
>  
>         return -EINVAL;
> @@ -921,7 +845,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
>  
>         /* Setup PLL */
>         reg_list = &link_freq_reglist[__ffs(imx335->link_freq_bitmap)];
> -       ret = imx335_write_regs(imx335, reg_list->regs, reg_list->num_of_regs);
> +       ret = cci_multi_reg_write(imx335->cci, reg_list->regs,
> +                                 reg_list->num_of_regs, NULL);
>         if (ret) {
>                 dev_err(imx335->dev, "%s failed to set plls\n", __func__);
>                 return ret;
> @@ -929,8 +854,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
>  
>         /* Write sensor mode registers */
>         reg_list = &imx335->cur_mode->reg_list;
> -       ret = imx335_write_regs(imx335, reg_list->regs,
> -                               reg_list->num_of_regs);
> +       ret = cci_multi_reg_write(imx335->cci, reg_list->regs,
> +                                 reg_list->num_of_regs, NULL);
>         if (ret) {
>                 dev_err(imx335->dev, "fail to write initial registers\n");
>                 return ret;
> @@ -944,7 +869,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
>         }
>  
>         /* Configure lanes */
> -       ret = imx335_write_reg(imx335, IMX335_LANEMODE, 1, imx335->lane_mode);
> +       ret = cci_write(imx335->cci, IMX335_REG_LANEMODE,
> +                       imx335->lane_mode, NULL);
>         if (ret)
>                 return ret;
>  
> @@ -956,8 +882,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
>         }
>  
>         /* Start streaming */
> -       ret = imx335_write_reg(imx335, IMX335_REG_MODE_SELECT,
> -                              1, IMX335_MODE_STREAMING);
> +       ret = cci_write(imx335->cci, IMX335_REG_MODE_SELECT,
> +                       IMX335_MODE_STREAMING, NULL);
>         if (ret) {
>                 dev_err(imx335->dev, "fail to start streaming\n");
>                 return ret;
> @@ -977,8 +903,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
>   */
>  static int imx335_stop_streaming(struct imx335 *imx335)
>  {
> -       return imx335_write_reg(imx335, IMX335_REG_MODE_SELECT,
> -                               1, IMX335_MODE_STANDBY);
> +       return cci_write(imx335->cci, IMX335_REG_MODE_SELECT,
> +                        IMX335_MODE_STANDBY, NULL);
>  }
>  
>  /**
> @@ -1029,14 +955,14 @@ static int imx335_set_stream(struct v4l2_subdev *sd, int enable)
>  static int imx335_detect(struct imx335 *imx335)
>  {
>         int ret;
> -       u32 val;
> +       u64 val;
>  
> -       ret = imx335_read_reg(imx335, IMX335_REG_ID, 2, &val);
> +       ret = cci_read(imx335->cci, IMX335_REG_ID, &val, NULL);
>         if (ret)
>                 return ret;
>  
>         if (val != IMX335_ID) {
> -               dev_err(imx335->dev, "chip id mismatch: %x!=%x\n",
> +               dev_err(imx335->dev, "chip id mismatch: %x!=%llx\n",
>                         IMX335_ID, val);
>                 return -ENXIO;
>         }
> @@ -1331,6 +1257,11 @@ static int imx335_probe(struct i2c_client *client)
>                 return -ENOMEM;
>  
>         imx335->dev = &client->dev;
> +       imx335->cci = devm_cci_regmap_init_i2c(client, 16);
> +       if (IS_ERR(imx335->cci)) {
> +               dev_err(imx335->dev, "Unable to initialize I2C\n");
> +               return -ENODEV;
> +       }
>  
>         /* Initialize subdev */
>         v4l2_i2c_subdev_init(&imx335->sd, client, &imx335_subdev_ops);
> -- 
> 2.43.0
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v6 4/6] media: imx335: Use integer values for size registers
  2024-04-12 16:00 [PATCH v6 0/6] media: imx335: 2/4 lane ops and improvements Umang Jain
                   ` (2 preceding siblings ...)
  2024-04-12 16:00 ` [PATCH v6 3/6] media: imx335: Use V4L2 CCI for accessing sensor registers Umang Jain
@ 2024-04-12 16:00 ` Umang Jain
  2024-04-12 16:00 ` [PATCH v6 5/6] media: imx335: Fix active area height discrepency Umang Jain
  2024-04-12 16:00 ` [PATCH v6 6/6] media: imx335: Limit analogue gain value Umang Jain
  5 siblings, 0 replies; 8+ messages in thread
From: Umang Jain @ 2024-04-12 16:00 UTC (permalink / raw)
  To: linux-media
  Cc: Alexander Shiyan, Kieran Bingham, Sakari Ailus, open list,
	Umang Jain, Tommaso Merciai

Consider integer values for registers that are related to various
sizes in the register map. This helps in improving the overall
readability.

No functional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index ee22304f827e..7548518359bc 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -251,12 +251,12 @@ static const struct cci_reg_sequence mode_2592x1940_regs[] = {
 	{ IMX335_REG_MODE_SELECT, 0x01 },
 	{ IMX335_REG_MASTER_MODE, 0x00 },
 	{ IMX335_REG_WINMODE, 0x04 },
-	{ IMX335_REG_HTRIMMING_START, 0x0180 },
-	{ IMX335_REG_HNUM, 0x0a20 },
-	{ IMX335_REG_Y_OUT_SIZE, 0x0794 },
-	{ IMX335_REG_AREA3_ST_ADR_1, 0x00b0 },
-	{ IMX335_REG_AREA3_WIDTH_1, 0x0f58 },
-	{ IMX335_REG_OPB_SIZE_V, 0x00 },
+	{ IMX335_REG_HTRIMMING_START, 384 },
+	{ IMX335_REG_HNUM, 2592 },
+	{ IMX335_REG_Y_OUT_SIZE, 1940 },
+	{ IMX335_REG_AREA3_ST_ADR_1, 176 },
+	{ IMX335_REG_AREA3_WIDTH_1, 3928 },
+	{ IMX335_REG_OPB_SIZE_V, 0 },
 	{ IMX335_REG_XVS_XHS_DRV, 0x00 },
 	{ CCI_REG8(0x3288), 0x21 },
 	{ CCI_REG8(0x328a), 0x02 },
-- 
2.43.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v6 5/6] media: imx335: Fix active area height discrepency
  2024-04-12 16:00 [PATCH v6 0/6] media: imx335: 2/4 lane ops and improvements Umang Jain
                   ` (3 preceding siblings ...)
  2024-04-12 16:00 ` [PATCH v6 4/6] media: imx335: Use integer values for size registers Umang Jain
@ 2024-04-12 16:00 ` Umang Jain
  2024-04-12 16:00 ` [PATCH v6 6/6] media: imx335: Limit analogue gain value Umang Jain
  5 siblings, 0 replies; 8+ messages in thread
From: Umang Jain @ 2024-04-12 16:00 UTC (permalink / raw)
  To: linux-media
  Cc: Alexander Shiyan, Kieran Bingham, Sakari Ailus, open list, Umang Jain

The imx335 reports a recommended pixel area of - 2592x1944.
The driver supported mode however limits it to height=1940.

Fix the height discrepency by correctly the value of height
(with updates to vblank and mode registers).

The IMX335_REG_HTRIMMING should also be corrected to the start
offset(i.e. 48) instead of chosing a abritrary crop start of 384.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index 7548518359bc..dfbab39d69b9 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -251,9 +251,9 @@ static const struct cci_reg_sequence mode_2592x1940_regs[] = {
 	{ IMX335_REG_MODE_SELECT, 0x01 },
 	{ IMX335_REG_MASTER_MODE, 0x00 },
 	{ IMX335_REG_WINMODE, 0x04 },
-	{ IMX335_REG_HTRIMMING_START, 384 },
+	{ IMX335_REG_HTRIMMING_START, 48 },
 	{ IMX335_REG_HNUM, 2592 },
-	{ IMX335_REG_Y_OUT_SIZE, 1940 },
+	{ IMX335_REG_Y_OUT_SIZE, 1944 },
 	{ IMX335_REG_AREA3_ST_ADR_1, 176 },
 	{ IMX335_REG_AREA3_WIDTH_1, 3928 },
 	{ IMX335_REG_OPB_SIZE_V, 0 },
@@ -404,10 +404,10 @@ static const u32 imx335_mbus_codes[] = {
 /* Supported sensor mode configurations */
 static const struct imx335_mode supported_mode = {
 	.width = 2592,
-	.height = 1940,
+	.height = 1944,
 	.hblank = 342,
-	.vblank = 2560,
-	.vblank_min = 2560,
+	.vblank = 2556,
+	.vblank_min = 2556,
 	.vblank_max = 133060,
 	.pclk = 396000000,
 	.reg_list = {
-- 
2.43.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v6 6/6] media: imx335: Limit analogue gain value
  2024-04-12 16:00 [PATCH v6 0/6] media: imx335: 2/4 lane ops and improvements Umang Jain
                   ` (4 preceding siblings ...)
  2024-04-12 16:00 ` [PATCH v6 5/6] media: imx335: Fix active area height discrepency Umang Jain
@ 2024-04-12 16:00 ` Umang Jain
  5 siblings, 0 replies; 8+ messages in thread
From: Umang Jain @ 2024-04-12 16:00 UTC (permalink / raw)
  To: linux-media
  Cc: Alexander Shiyan, Kieran Bingham, Sakari Ailus, open list,
	Umang Jain, Tommaso Merciai

The sensor gain (both analog and digital) are controlled by a
single gain value where:
- 0dB to 30dB correspond to analog gain
- 30.3dB to 72dB correspond to digital gain
  (with 0.3dB step)

Hence, limit the analogue gain value to 100.
For digital gain, support can be added later if needed.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>
---
 drivers/media/i2c/imx335.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index dfbab39d69b9..6924bc968a4a 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -52,7 +52,7 @@
 /* Analog gain control */
 #define IMX335_REG_AGAIN		CCI_REG8(0x30e8)
 #define IMX335_AGAIN_MIN		0
-#define IMX335_AGAIN_MAX		240
+#define IMX335_AGAIN_MAX		100
 #define IMX335_AGAIN_STEP		1
 #define IMX335_AGAIN_DEFAULT		0
 
@@ -1179,6 +1179,14 @@ static int imx335_init_controls(struct imx335 *imx335)
 					     IMX335_EXPOSURE_STEP,
 					     IMX335_EXPOSURE_DEFAULT);
 
+	/*
+	 * The sensor has an analog gain and a digital gain, both controlled
+	 * through a single gain value, expressed in 0.3dB increments. Values
+	 * from 0.0dB (0) to 30.0dB (100) apply analog gain only, higher values
+	 * up to 72.0dB (240) add further digital gain. Limit the range to
+	 * analog gain only, support for digital gain can be added separately
+	 * if needed.
+	 */
 	imx335->again_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
 					       &imx335_ctrl_ops,
 					       V4L2_CID_ANALOGUE_GAIN,
-- 
2.43.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-04-12 21:23 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-12 16:00 [PATCH v6 0/6] media: imx335: 2/4 lane ops and improvements Umang Jain
2024-04-12 16:00 ` [PATCH v6 1/6] media: imx335: Support 2 or 4 lane operation modes Umang Jain
2024-04-12 16:00 ` [PATCH v6 2/6] media: imx335: Parse fwnode properties Umang Jain
2024-04-12 16:00 ` [PATCH v6 3/6] media: imx335: Use V4L2 CCI for accessing sensor registers Umang Jain
2024-04-12 21:23   ` Kieran Bingham
2024-04-12 16:00 ` [PATCH v6 4/6] media: imx335: Use integer values for size registers Umang Jain
2024-04-12 16:00 ` [PATCH v6 5/6] media: imx335: Fix active area height discrepency Umang Jain
2024-04-12 16:00 ` [PATCH v6 6/6] media: imx335: Limit analogue gain value Umang Jain

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®