[PATCH v4 0/3] media: i2c: ov5647: Modernize driver with CCI and new stream APIs

[PATCH v4 0/3] media: i2c: ov5647: Modernize driver with CCI and new stream APIs

[Xiaolei Wang]

This patch series modernizes the OV5647 camera sensor driver by:

1. Converting from private I2C register access functions to the common
   CCI (Camera Control Interface) register access helpers, which
   simplifies the code and provides better error handling.

2. Switching from driver-specific mutex to the sub-device state lock
   and properly implementing v4l2_subdev_init_finalize() lifecycle.

3. Converting from the legacy s_stream callback to the new
   enable_streams/disable_streams operations to align with current
   V4L2 subsystem standards.

I tested each patch on a Raspberry Pi 5 using the following commands:

rpicam-jpeg --output test.jpg
rpicam-still -o long_exposure.jpg --shutter 100000000 --gain 1 --awbgains 1,1 --immediate

Changes in V4:

 - In patch 1, have corrected the issue of variables that do not need initialization.
   I have defined a chip ID, dropped all control functions, and now set the
   chip ID directly in the set control, using `cci_update_bits()` instead of
   read + write operations.

 - In patch 2, Use dev_err_probe instead of dev_err

 - In patch 3, have corrected the issue of variables that do not need initialization.

Changes in V3:
https://patchwork.kernel.org/project/linux-media/cover/20251231083924.2657165-1-xiaolei.wang@windriver.com/

 - In patch 1, I replaced cci_multi_reg_write() with regmap_multi_reg_write() and
   fixed OV5647_REG_GAIN at 0x350a. I also replaced the original ret = PTR_ERR(sensor->regmap) with dev_err_probe().

 - In patch 2, I replaced the mutex with v4l2_subdev_lock_and_get_active_state() in s_stream().

 - In patch 3, I replaced err_rpm_put with done, and added the ov5647_stream_stop() function.

Changes in V2:
https://patchwork.kernel.org/project/linux-media/cover/20251229023018.2933405-1-xiaolei.wang@windriver.com/

 - Proper register width definitions
 - Fixed formatting and indentation
 - Error chaining implementation
 - Simplified chip detection logic
 - Clean compilation with -Werror
 - Add a new patch, switch from s_stream to enable_streams and disable_streams callbacks.

Link to V1: https://patchwork.kernel.org/project/linux-media/cover/20251226031311.2068414-1-xiaolei.wang@windriver.com/ 

Xiaolei Wang (3):
  media: i2c: ov5647: Convert to CCI register access helpers
  media: i2c: ov5647: Switch to using the sub-device state lock
  media: i2c: ov5647: switch to {enable,disable}_streams

 drivers/media/i2c/Kconfig  |   1 +
 drivers/media/i2c/ov5647.c | 450 ++++++++++++-------------------------
 2 files changed, 144 insertions(+), 307 deletions(-)

-- 
2.43.0

[PATCH v4 1/3] media: i2c: ov5647: Convert to CCI register access helpers

[Xiaolei Wang]

Use the new common CCI register access helpers to replace the private
register access helpers in the ov5647 driver. This simplifies the driver
by reducing the amount of code.

Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/Kconfig  |   1 +
 drivers/media/i2c/ov5647.c | 350 +++++++++++--------------------------
 2 files changed, 103 insertions(+), 248 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 4b4db8c4f496..cce63349e71e 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -529,6 +529,7 @@ config VIDEO_OV5645
 
 config VIDEO_OV5647
 	tristate "OmniVision OV5647 sensor support"
+	select V4L2_CCI_I2C
 	help
 	  This is a Video4Linux2 sensor driver for the OmniVision
 	  OV5647 camera.
diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index e193fef4fced..24a160bbb079 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -20,8 +20,10 @@
 #include <linux/module.h>
 #include <linux/of_graph.h>
 #include <linux/pm_runtime.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/videodev2.h>
+#include <media/v4l2-cci.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
@@ -41,25 +43,23 @@
 #define MIPI_CTRL00_BUS_IDLE			BIT(2)
 #define MIPI_CTRL00_CLOCK_LANE_DISABLE		BIT(0)
 
-#define OV5647_SW_STANDBY		0x0100
-#define OV5647_SW_RESET			0x0103
-#define OV5647_REG_CHIPID_H		0x300a
-#define OV5647_REG_CHIPID_L		0x300b
-#define OV5640_REG_PAD_OUT		0x300d
-#define OV5647_REG_EXP_HI		0x3500
-#define OV5647_REG_EXP_MID		0x3501
-#define OV5647_REG_EXP_LO		0x3502
-#define OV5647_REG_AEC_AGC		0x3503
-#define OV5647_REG_GAIN_HI		0x350a
-#define OV5647_REG_GAIN_LO		0x350b
-#define OV5647_REG_VTS_HI		0x380e
-#define OV5647_REG_VTS_LO		0x380f
-#define OV5647_REG_FRAME_OFF_NUMBER	0x4202
-#define OV5647_REG_MIPI_CTRL00		0x4800
-#define OV5647_REG_MIPI_CTRL14		0x4814
-#define OV5647_REG_AWB			0x5001
-#define OV5647_REG_ISPCTRL3D		0x503d
-
+#define OV5647_SW_STANDBY		CCI_REG8(0x0100)
+#define OV5647_SW_RESET			CCI_REG8(0x0103)
+#define OV5647_REG_CHIPID		CCI_REG16(0x300a)
+#define OV5640_REG_PAD_OUT		CCI_REG8(0x300d)
+#define OV5647_REG_EXPOSURE		CCI_REG24(0x3500)
+#define OV5647_REG_AEC_AGC		CCI_REG8(0x3503)
+#define OV5647_REG_GAIN			CCI_REG16(0x350a)
+#define OV5647_REG_VTS			CCI_REG16(0x380e)
+#define OV5647_REG_FRAME_OFF_NUMBER	CCI_REG8(0x4202)
+#define OV5647_REG_MIPI_CTRL00		CCI_REG8(0x4800)
+#define OV5647_REG_MIPI_CTRL14		CCI_REG8(0x4814)
+#define OV5647_REG_MIPI_CTRL14_CHANNEL_MASK	GENMASK(7, 6)
+#define OV5647_REG_MIPI_CTRL14_CHANNEL_SHIFT	6
+#define OV5647_REG_AWB			CCI_REG8(0x5001)
+#define OV5647_REG_ISPCTRL3D		CCI_REG8(0x503d)
+
+#define OV5647_CHIP_ID 0x5647
 #define REG_TERM 0xfffe
 #define VAL_TERM 0xfe
 #define REG_DLY  0xffff
@@ -81,23 +81,19 @@
 #define OV5647_EXPOSURE_DEFAULT		1000
 #define OV5647_EXPOSURE_MAX		65535
 
-struct regval_list {
-	u16 addr;
-	u8 data;
-};
-
 struct ov5647_mode {
 	struct v4l2_mbus_framefmt	format;
 	struct v4l2_rect		crop;
 	u64				pixel_rate;
 	int				hts;
 	int				vts;
-	const struct regval_list	*reg_list;
+	const struct reg_sequence	*reg_list;
 	unsigned int			num_regs;
 };
 
 struct ov5647 {
 	struct v4l2_subdev		sd;
+	struct regmap			*regmap;
 	struct media_pad		pad;
 	struct mutex			lock;
 	struct clk			*xclk;
@@ -130,19 +126,19 @@ static const u8 ov5647_test_pattern_val[] = {
 	0x81,	/* Random Data */
 };
 
-static const struct regval_list sensor_oe_disable_regs[] = {
+static const struct reg_sequence sensor_oe_disable_regs[] = {
 	{0x3000, 0x00},
 	{0x3001, 0x00},
 	{0x3002, 0x00},
 };
 
-static const struct regval_list sensor_oe_enable_regs[] = {
+static const struct reg_sequence sensor_oe_enable_regs[] = {
 	{0x3000, 0x0f},
 	{0x3001, 0xff},
 	{0x3002, 0xe4},
 };
 
-static struct regval_list ov5647_2592x1944_10bpp[] = {
+static const struct reg_sequence ov5647_2592x1944_10bpp[] = {
 	{0x0100, 0x00},
 	{0x0103, 0x01},
 	{0x3034, 0x1a},
@@ -231,7 +227,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
 	{0x0100, 0x01},
 };
 
-static struct regval_list ov5647_1080p30_10bpp[] = {
+static const struct reg_sequence ov5647_1080p30_10bpp[] = {
 	{0x0100, 0x00},
 	{0x0103, 0x01},
 	{0x3034, 0x1a},
@@ -320,7 +316,7 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
 	{0x0100, 0x01},
 };
 
-static struct regval_list ov5647_2x2binned_10bpp[] = {
+static const struct reg_sequence ov5647_2x2binned_10bpp[] = {
 	{0x0100, 0x00},
 	{0x0103, 0x01},
 	{0x3034, 0x1a},
@@ -413,7 +409,7 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
 	{0x0100, 0x01},
 };
 
-static struct regval_list ov5647_640x480_10bpp[] = {
+static const struct reg_sequence ov5647_640x480_10bpp[] = {
 	{0x0100, 0x00},
 	{0x0103, 0x01},
 	{0x3035, 0x11},
@@ -594,109 +590,29 @@ static const struct ov5647_mode ov5647_modes[] = {
 #define OV5647_DEFAULT_MODE	(&ov5647_modes[3])
 #define OV5647_DEFAULT_FORMAT	(ov5647_modes[3].format)
 
-static int ov5647_write16(struct v4l2_subdev *sd, u16 reg, u16 val)
-{
-	unsigned char data[4] = { reg >> 8, reg & 0xff, val >> 8, val & 0xff};
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	int ret;
-
-	ret = i2c_master_send(client, data, 4);
-	if (ret < 0) {
-		dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
-			__func__, reg);
-		return ret;
-	}
-
-	return 0;
-}
-
-static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val)
-{
-	unsigned char data[3] = { reg >> 8, reg & 0xff, val};
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	int ret;
-
-	ret = i2c_master_send(client, data, 3);
-	if (ret < 0) {
-		dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
-				__func__, reg);
-		return ret;
-	}
-
-	return 0;
-}
-
-static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	u8 buf[2] = { reg >> 8, reg & 0xff };
-	struct i2c_msg msg[2];
-	int ret;
-
-	msg[0].addr = client->addr;
-	msg[0].flags = client->flags;
-	msg[0].buf = buf;
-	msg[0].len = sizeof(buf);
-
-	msg[1].addr = client->addr;
-	msg[1].flags = client->flags | I2C_M_RD;
-	msg[1].buf = buf;
-	msg[1].len = 1;
-
-	ret = i2c_transfer(client->adapter, msg, 2);
-	if (ret != 2) {
-		dev_err(&client->dev, "%s: i2c read error, reg: %x = %d\n",
-			__func__, reg, ret);
-		return ret >= 0 ? -EINVAL : ret;
-	}
-
-	*val = buf[0];
-
-	return 0;
-}
-
-static int ov5647_write_array(struct v4l2_subdev *sd,
-			      const struct regval_list *regs, int array_size)
-{
-	int i, ret;
-
-	for (i = 0; i < array_size; i++) {
-		ret = ov5647_write(sd, regs[i].addr, regs[i].data);
-		if (ret < 0)
-			return ret;
-	}
-
-	return 0;
-}
-
 static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
 {
-	u8 channel_id;
-	int ret;
-
-	ret = ov5647_read(sd, OV5647_REG_MIPI_CTRL14, &channel_id);
-	if (ret < 0)
-		return ret;
-
-	channel_id &= ~(3 << 6);
+	struct ov5647 *sensor = to_sensor(sd);
 
-	return ov5647_write(sd, OV5647_REG_MIPI_CTRL14,
-			    channel_id | (channel << 6));
+	return cci_update_bits(sensor->regmap, OV5647_REG_MIPI_CTRL14,
+			       OV5647_REG_MIPI_CTRL14_CHANNEL_MASK,
+			       channel << OV5647_REG_MIPI_CTRL14_CHANNEL_SHIFT,
+			       NULL);
 }
 
 static int ov5647_set_mode(struct v4l2_subdev *sd)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov5647 *sensor = to_sensor(sd);
-	u8 resetval, rdval;
+	u64 resetval, rdval;
 	int ret;
 
-	ret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);
+	ret = cci_read(sensor->regmap, OV5647_SW_STANDBY, &rdval, NULL);
 	if (ret < 0)
 		return ret;
 
-	ret = ov5647_write_array(sd, sensor->mode->reg_list,
-				 sensor->mode->num_regs);
+	ret = regmap_multi_reg_write(sensor->regmap, sensor->mode->reg_list,
+				     sensor->mode->num_regs);
 	if (ret < 0) {
 		dev_err(&client->dev, "write sensor default regs error\n");
 		return ret;
@@ -706,13 +622,13 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)
 	if (ret < 0)
 		return ret;
 
-	ret = ov5647_read(sd, OV5647_SW_STANDBY, &resetval);
+	ret = cci_read(sensor->regmap, OV5647_SW_STANDBY, &resetval, NULL);
 	if (ret < 0)
 		return ret;
 
 	if (!(resetval & 0x01)) {
 		dev_err(&client->dev, "Device was in SW standby");
-		ret = ov5647_write(sd, OV5647_SW_STANDBY, 0x01);
+		ret = cci_write(sensor->regmap, OV5647_SW_STANDBY, 0x01, NULL);
 		if (ret < 0)
 			return ret;
 	}
@@ -742,32 +658,25 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)
 		val |= MIPI_CTRL00_CLOCK_LANE_GATE |
 		       MIPI_CTRL00_LINE_SYNC_ENABLE;
 
-	ret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, val);
-	if (ret < 0)
-		return ret;
-
-	ret = ov5647_write(sd, OV5647_REG_FRAME_OFF_NUMBER, 0x00);
-	if (ret < 0)
-		return ret;
+	cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL00, val, &ret);
+	cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x00, &ret);
+	cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x00, &ret);
 
-	return ov5647_write(sd, OV5640_REG_PAD_OUT, 0x00);
+	return ret;
 }
 
 static int ov5647_stream_off(struct v4l2_subdev *sd)
 {
-	int ret;
-
-	ret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00,
-			   MIPI_CTRL00_CLOCK_LANE_GATE | MIPI_CTRL00_BUS_IDLE |
-			   MIPI_CTRL00_CLOCK_LANE_DISABLE);
-	if (ret < 0)
-		return ret;
+	struct ov5647 *sensor = to_sensor(sd);
+	int ret = 0;
 
-	ret = ov5647_write(sd, OV5647_REG_FRAME_OFF_NUMBER, 0x0f);
-	if (ret < 0)
-		return ret;
+	cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL00,
+		  MIPI_CTRL00_CLOCK_LANE_GATE | MIPI_CTRL00_BUS_IDLE |
+		  MIPI_CTRL00_CLOCK_LANE_DISABLE, &ret);
+	cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x0f, &ret);
+	cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x01, &ret);
 
-	return ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);
+	return ret;
 }
 
 static int ov5647_power_on(struct device *dev)
@@ -788,8 +697,8 @@ static int ov5647_power_on(struct device *dev)
 		goto error_pwdn;
 	}
 
-	ret = ov5647_write_array(&sensor->sd, sensor_oe_enable_regs,
-				 ARRAY_SIZE(sensor_oe_enable_regs));
+	ret = regmap_multi_reg_write(sensor->regmap, sensor_oe_enable_regs,
+				     ARRAY_SIZE(sensor_oe_enable_regs));
 	if (ret < 0) {
 		dev_err(dev, "write sensor_oe_enable_regs error\n");
 		goto error_clk_disable;
@@ -815,23 +724,17 @@ static int ov5647_power_on(struct device *dev)
 static int ov5647_power_off(struct device *dev)
 {
 	struct ov5647 *sensor = dev_get_drvdata(dev);
-	u8 rdval;
 	int ret;
 
 	dev_dbg(dev, "OV5647 power off\n");
 
-	ret = ov5647_write_array(&sensor->sd, sensor_oe_disable_regs,
-				 ARRAY_SIZE(sensor_oe_disable_regs));
+	ret = regmap_multi_reg_write(sensor->regmap, sensor_oe_disable_regs,
+				     ARRAY_SIZE(sensor_oe_disable_regs));
 	if (ret < 0)
 		dev_dbg(dev, "disable oe failed\n");
 
 	/* Enter software standby */
-	ret = ov5647_read(&sensor->sd, OV5647_SW_STANDBY, &rdval);
-	if (ret < 0)
-		dev_dbg(dev, "software standby failed\n");
-
-	rdval &= ~0x01;
-	ret = ov5647_write(&sensor->sd, OV5647_SW_STANDBY, rdval);
+	ret = cci_update_bits(sensor->regmap, OV5647_SW_STANDBY, 0x01, 0x00, NULL);
 	if (ret < 0)
 		dev_dbg(dev, "software standby failed\n");
 
@@ -845,10 +748,11 @@ static int ov5647_power_off(struct device *dev)
 static int ov5647_sensor_get_register(struct v4l2_subdev *sd,
 				      struct v4l2_dbg_register *reg)
 {
+	struct ov5647 *sensor = to_sensor(sd);
 	int ret;
-	u8 val;
+	u64 val;
 
-	ret = ov5647_read(sd, reg->reg & 0xff, &val);
+	ret = cci_read(sensor->regmap, reg->reg & 0xff, &val, NULL);
 	if (ret < 0)
 		return ret;
 
@@ -861,7 +765,9 @@ static int ov5647_sensor_get_register(struct v4l2_subdev *sd,
 static int ov5647_sensor_set_register(struct v4l2_subdev *sd,
 				      const struct v4l2_dbg_register *reg)
 {
-	return ov5647_write(sd, reg->reg & 0xff, reg->val & 0xff);
+	struct ov5647 *sensor = to_sensor(sd);
+
+	return cci_write(sensor->regmap, reg->reg & 0xff, reg->val & 0xff, NULL);
 }
 #endif
 
@@ -1089,33 +995,27 @@ static const struct v4l2_subdev_ops ov5647_subdev_ops = {
 
 static int ov5647_detect(struct v4l2_subdev *sd)
 {
+	struct ov5647 *sensor = to_sensor(sd);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	u8 read;
+	u64 read;
 	int ret;
 
-	ret = ov5647_write(sd, OV5647_SW_RESET, 0x01);
+	ret = cci_write(sensor->regmap, OV5647_SW_RESET, 0x01, NULL);
 	if (ret < 0)
 		return ret;
 
-	ret = ov5647_read(sd, OV5647_REG_CHIPID_H, &read);
+	ret = cci_read(sensor->regmap, OV5647_REG_CHIPID, &read, NULL);
 	if (ret < 0)
-		return ret;
+		return dev_err_probe(&client->dev, ret,
+				     "failed to read chip id %x\n",
+				     OV5647_REG_CHIPID);
 
-	if (read != 0x56) {
-		dev_err(&client->dev, "ID High expected 0x56 got %x", read);
+	if (read != OV5647_CHIP_ID) {
+		dev_err(&client->dev, "Chip ID expected 0x5647 got 0x%llx", read);
 		return -ENODEV;
 	}
 
-	ret = ov5647_read(sd, OV5647_REG_CHIPID_L, &read);
-	if (ret < 0)
-		return ret;
-
-	if (read != 0x47) {
-		dev_err(&client->dev, "ID Low expected 0x47 got %x", read);
-		return -ENODEV;
-	}
-
-	return ov5647_write(sd, OV5647_SW_RESET, 0x00);
+	return cci_write(sensor->regmap, OV5647_SW_RESET, 0x00, NULL);
 }
 
 static int ov5647_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
@@ -1138,74 +1038,6 @@ static const struct v4l2_subdev_internal_ops ov5647_subdev_internal_ops = {
 	.open = ov5647_open,
 };
 
-static int ov5647_s_auto_white_balance(struct v4l2_subdev *sd, u32 val)
-{
-	return ov5647_write(sd, OV5647_REG_AWB, val ? 1 : 0);
-}
-
-static int ov5647_s_autogain(struct v4l2_subdev *sd, u32 val)
-{
-	int ret;
-	u8 reg;
-
-	/* Non-zero turns on AGC by clearing bit 1.*/
-	ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
-	if (ret)
-		return ret;
-
-	return ov5647_write(sd, OV5647_REG_AEC_AGC, val ? reg & ~BIT(1)
-							: reg | BIT(1));
-}
-
-static int ov5647_s_exposure_auto(struct v4l2_subdev *sd, u32 val)
-{
-	int ret;
-	u8 reg;
-
-	/*
-	 * Everything except V4L2_EXPOSURE_MANUAL turns on AEC by
-	 * clearing bit 0.
-	 */
-	ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
-	if (ret)
-		return ret;
-
-	return ov5647_write(sd, OV5647_REG_AEC_AGC,
-			    val == V4L2_EXPOSURE_MANUAL ? reg | BIT(0)
-							: reg & ~BIT(0));
-}
-
-static int ov5647_s_analogue_gain(struct v4l2_subdev *sd, u32 val)
-{
-	int ret;
-
-	/* 10 bits of gain, 2 in the high register. */
-	ret = ov5647_write(sd, OV5647_REG_GAIN_HI, (val >> 8) & 3);
-	if (ret)
-		return ret;
-
-	return ov5647_write(sd, OV5647_REG_GAIN_LO, val & 0xff);
-}
-
-static int ov5647_s_exposure(struct v4l2_subdev *sd, u32 val)
-{
-	int ret;
-
-	/*
-	 * Sensor has 20 bits, but the bottom 4 bits are fractions of a line
-	 * which we leave as zero (and don't receive in "val").
-	 */
-	ret = ov5647_write(sd, OV5647_REG_EXP_HI, (val >> 12) & 0xf);
-	if (ret)
-		return ret;
-
-	ret = ov5647_write(sd, OV5647_REG_EXP_MID, (val >> 4) & 0xff);
-	if (ret)
-		return ret;
-
-	return ov5647_write(sd, OV5647_REG_EXP_LO, (val & 0xf) << 4);
-}
-
 static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct ov5647 *sensor = container_of(ctrl->handler,
@@ -1239,27 +1071,42 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
 
 	switch (ctrl->id) {
 	case V4L2_CID_AUTO_WHITE_BALANCE:
-		ret = ov5647_s_auto_white_balance(sd, ctrl->val);
+		ret = cci_write(sensor->regmap, OV5647_REG_AWB,
+				ctrl->val ? 1 : 0, NULL);
 		break;
 	case V4L2_CID_AUTOGAIN:
-		ret = ov5647_s_autogain(sd, ctrl->val);
+		/* Non-zero turns on AGC by clearing bit 1.*/
+		return cci_update_bits(sensor->regmap, OV5647_REG_AEC_AGC, BIT(1),
+				       ctrl->val ? 0 : BIT(1), NULL);
 		break;
 	case V4L2_CID_EXPOSURE_AUTO:
-		ret = ov5647_s_exposure_auto(sd, ctrl->val);
+		/*
+		 * Everything except V4L2_EXPOSURE_MANUAL turns on AEC by
+		 * clearing bit 0.
+		 */
+		return cci_update_bits(sensor->regmap, OV5647_REG_AEC_AGC, BIT(0),
+				       ctrl->val == V4L2_EXPOSURE_MANUAL ? BIT(0) : 0, NULL);
 		break;
 	case V4L2_CID_ANALOGUE_GAIN:
-		ret =  ov5647_s_analogue_gain(sd, ctrl->val);
+		/* 10 bits of gain, 2 in the high register. */
+		return cci_write(sensor->regmap, OV5647_REG_GAIN,
+				 ctrl->val & 0x3ff, NULL);
 		break;
 	case V4L2_CID_EXPOSURE:
-		ret = ov5647_s_exposure(sd, ctrl->val);
+		/*
+		 * Sensor has 20 bits, but the bottom 4 bits are fractions of a line
+		 * which we leave as zero (and don't receive in "val").
+		 */
+		ret = cci_write(sensor->regmap, OV5647_REG_EXPOSURE,
+				ctrl->val << 4, NULL);
 		break;
 	case V4L2_CID_VBLANK:
-		ret = ov5647_write16(sd, OV5647_REG_VTS_HI,
-				     sensor->mode->format.height + ctrl->val);
+		ret = cci_write(sensor->regmap, OV5647_REG_VTS,
+				sensor->mode->format.height + ctrl->val, NULL);
 		break;
 	case V4L2_CID_TEST_PATTERN:
-		ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D,
-				   ov5647_test_pattern_val[ctrl->val]);
+		ret = cci_write(sensor->regmap, OV5647_REG_ISPCTRL3D,
+				ov5647_test_pattern_val[ctrl->val], NULL);
 		break;
 
 	/* Read-only, but we adjust it based on mode. */
@@ -1435,6 +1282,13 @@ static int ov5647_probe(struct i2c_client *client)
 	if (ret < 0)
 		goto ctrl_handler_free;
 
+	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
+	if (IS_ERR(sensor->regmap)) {
+		ret = dev_err_probe(dev, PTR_ERR(sensor->regmap),
+				    "Failed to init CCI\n");
+		goto entity_cleanup;
+	}
+
 	ret = ov5647_power_on(dev);
 	if (ret)
 		goto entity_cleanup;
-- 
2.43.0

[PATCH v4 2/3] media: i2c: ov5647: Switch to using the sub-device state lock

[Xiaolei Wang]

Switch to using the sub-device state lock and properly call
v4l2_subdev_init_finalize() / v4l2_subdev_cleanup() on probe() /
remove().

Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/ov5647.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 24a160bbb079..822804633d0f 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -95,7 +95,6 @@ struct ov5647 {
 	struct v4l2_subdev		sd;
 	struct regmap			*regmap;
 	struct media_pad		pad;
-	struct mutex			lock;
 	struct clk			*xclk;
 	struct gpio_desc		*pwdn;
 	bool				clock_ncont;
@@ -799,10 +798,10 @@ __ov5647_get_pad_crop(struct ov5647 *ov5647,
 static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	struct ov5647 *sensor = to_sensor(sd);
+	struct v4l2_subdev_state *state;
 	int ret;
 
-	mutex_lock(&sensor->lock);
+	state = v4l2_subdev_lock_and_get_active_state(sd);
 
 	if (enable) {
 		ret = pm_runtime_resume_and_get(&client->dev);
@@ -823,14 +822,14 @@ static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
 		pm_runtime_put(&client->dev);
 	}
 
-	mutex_unlock(&sensor->lock);
+	v4l2_subdev_unlock_state(state);
 
 	return 0;
 
 error_pm:
 	pm_runtime_put(&client->dev);
 error_unlock:
-	mutex_unlock(&sensor->lock);
+	v4l2_subdev_unlock_state(state);
 
 	return ret;
 }
@@ -878,7 +877,6 @@ static int ov5647_get_pad_fmt(struct v4l2_subdev *sd,
 	const struct v4l2_mbus_framefmt *sensor_format;
 	struct ov5647 *sensor = to_sensor(sd);
 
-	mutex_lock(&sensor->lock);
 	switch (format->which) {
 	case V4L2_SUBDEV_FORMAT_TRY:
 		sensor_format = v4l2_subdev_state_get_format(sd_state,
@@ -890,7 +888,6 @@ static int ov5647_get_pad_fmt(struct v4l2_subdev *sd,
 	}
 
 	*fmt = *sensor_format;
-	mutex_unlock(&sensor->lock);
 
 	return 0;
 }
@@ -908,7 +905,6 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
 				      fmt->width, fmt->height);
 
 	/* Update the sensor mode and apply at it at streamon time. */
-	mutex_lock(&sensor->lock);
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
 		*v4l2_subdev_state_get_format(sd_state, format->pad) = mode->format;
 	} else {
@@ -937,7 +933,6 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
 					 exposure_def);
 	}
 	*fmt = mode->format;
-	mutex_unlock(&sensor->lock);
 
 	return 0;
 }
@@ -950,10 +945,8 @@ static int ov5647_get_selection(struct v4l2_subdev *sd,
 	case V4L2_SEL_TGT_CROP: {
 		struct ov5647 *sensor = to_sensor(sd);
 
-		mutex_lock(&sensor->lock);
 		sel->r = *__ov5647_get_pad_crop(sensor, sd_state, sel->pad,
 						sel->which);
-		mutex_unlock(&sensor->lock);
 
 		return 0;
 	}
@@ -1046,9 +1039,6 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	int ret = 0;
 
-
-	/* v4l2_ctrl_lock() locks our own mutex */
-
 	if (ctrl->id == V4L2_CID_VBLANK) {
 		int exposure_max, exposure_def;
 
@@ -1263,13 +1253,11 @@ static int ov5647_probe(struct i2c_client *client)
 		return -EINVAL;
 	}
 
-	mutex_init(&sensor->lock);
-
 	sensor->mode = OV5647_DEFAULT_MODE;
 
 	ret = ov5647_init_controls(sensor);
 	if (ret)
-		goto mutex_destroy;
+		return ret;
 
 	sd = &sensor->sd;
 	v4l2_i2c_subdev_init(sd, client, &ov5647_subdev_ops);
@@ -1297,9 +1285,16 @@ static int ov5647_probe(struct i2c_client *client)
 	if (ret < 0)
 		goto power_off;
 
+	sd->state_lock = sensor->ctrls.lock;
+	ret = v4l2_subdev_init_finalize(sd);
+	if (ret < 0) {
+		ret = dev_err_probe(dev, ret, "failed to init subdev\n");
+		goto power_off;
+	}
+
 	ret = v4l2_async_register_subdev(sd);
 	if (ret < 0)
-		goto power_off;
+		goto v4l2_subdev_cleanup;
 
 	/* Enable runtime PM and turn off the device */
 	pm_runtime_set_active(dev);
@@ -1310,14 +1305,14 @@ static int ov5647_probe(struct i2c_client *client)
 
 	return 0;
 
+v4l2_subdev_cleanup:
+	v4l2_subdev_cleanup(sd);
 power_off:
 	ov5647_power_off(dev);
 entity_cleanup:
 	media_entity_cleanup(&sd->entity);
 ctrl_handler_free:
 	v4l2_ctrl_handler_free(&sensor->ctrls);
-mutex_destroy:
-	mutex_destroy(&sensor->lock);
 
 	return ret;
 }
@@ -1328,11 +1323,11 @@ static void ov5647_remove(struct i2c_client *client)
 	struct ov5647 *sensor = to_sensor(sd);
 
 	v4l2_async_unregister_subdev(&sensor->sd);
+	v4l2_subdev_cleanup(sd);
 	media_entity_cleanup(&sensor->sd.entity);
 	v4l2_ctrl_handler_free(&sensor->ctrls);
 	v4l2_device_unregister_subdev(sd);
 	pm_runtime_disable(&client->dev);
-	mutex_destroy(&sensor->lock);
 }
 
 static const struct dev_pm_ops ov5647_pm_ops = {
-- 
2.43.0

[PATCH v4 3/3] media: i2c: ov5647: switch to {enable,disable}_streams

[Xiaolei Wang]

Switch from s_stream to enable_streams and disable_streams callbacks.

Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/ov5647.c | 89 ++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 51 deletions(-)

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 822804633d0f..35ec65a0e578 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -635,23 +635,42 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)
 	return 0;
 }
 
-static int ov5647_stream_on(struct v4l2_subdev *sd)
+static int ov5647_stream_stop(struct ov5647 *sensor)
+{
+	int ret = 0;
+
+	cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL00,
+		  MIPI_CTRL00_CLOCK_LANE_GATE | MIPI_CTRL00_BUS_IDLE |
+		  MIPI_CTRL00_CLOCK_LANE_DISABLE, &ret);
+	cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x0f, &ret);
+	cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x01, &ret);
+
+	return ret;
+}
+
+static int ov5647_enable_streams(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *state, u32 pad,
+				 u64 streams_mask)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov5647 *sensor = to_sensor(sd);
 	u8 val = MIPI_CTRL00_BUS_IDLE;
 	int ret;
 
+	ret = pm_runtime_resume_and_get(&client->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = ov5647_set_mode(sd);
 	if (ret) {
 		dev_err(&client->dev, "Failed to program sensor mode: %d\n", ret);
-		return ret;
+		goto done;
 	}
 
 	/* Apply customized values from user when stream starts. */
 	ret =  __v4l2_ctrl_handler_setup(sd->ctrl_handler);
 	if (ret)
-		return ret;
+		goto done;
 
 	if (sensor->clock_ncont)
 		val |= MIPI_CTRL00_CLOCK_LANE_GATE |
@@ -661,19 +680,24 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)
 	cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x00, &ret);
 	cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x00, &ret);
 
+done:
+	if (ret)
+		pm_runtime_put(&client->dev);
+
 	return ret;
 }
 
-static int ov5647_stream_off(struct v4l2_subdev *sd)
+static int ov5647_disable_streams(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_state *state, u32 pad,
+				  u64 streams_mask)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov5647 *sensor = to_sensor(sd);
-	int ret = 0;
+	int ret;
 
-	cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL00,
-		  MIPI_CTRL00_CLOCK_LANE_GATE | MIPI_CTRL00_BUS_IDLE |
-		  MIPI_CTRL00_CLOCK_LANE_DISABLE, &ret);
-	cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x0f, &ret);
-	cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x01, &ret);
+	ret = ov5647_stream_stop(sensor);
+
+	pm_runtime_put(&client->dev);
 
 	return ret;
 }
@@ -704,7 +728,7 @@ static int ov5647_power_on(struct device *dev)
 	}
 
 	/* Stream off to coax lanes into LP-11 state. */
-	ret = ov5647_stream_off(&sensor->sd);
+	ret = ov5647_stream_stop(sensor);
 	if (ret < 0) {
 		dev_err(dev, "camera not available, check power\n");
 		goto error_clk_disable;
@@ -795,47 +819,8 @@ __ov5647_get_pad_crop(struct ov5647 *ov5647,
 	return NULL;
 }
 
-static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	struct v4l2_subdev_state *state;
-	int ret;
-
-	state = v4l2_subdev_lock_and_get_active_state(sd);
-
-	if (enable) {
-		ret = pm_runtime_resume_and_get(&client->dev);
-		if (ret < 0)
-			goto error_unlock;
-
-		ret = ov5647_stream_on(sd);
-		if (ret < 0) {
-			dev_err(&client->dev, "stream start failed: %d\n", ret);
-			goto error_pm;
-		}
-	} else {
-		ret = ov5647_stream_off(sd);
-		if (ret < 0) {
-			dev_err(&client->dev, "stream stop failed: %d\n", ret);
-			goto error_pm;
-		}
-		pm_runtime_put(&client->dev);
-	}
-
-	v4l2_subdev_unlock_state(state);
-
-	return 0;
-
-error_pm:
-	pm_runtime_put(&client->dev);
-error_unlock:
-	v4l2_subdev_unlock_state(state);
-
-	return ret;
-}
-
 static const struct v4l2_subdev_video_ops ov5647_subdev_video_ops = {
-	.s_stream =		ov5647_s_stream,
+	.s_stream = v4l2_subdev_s_stream_helper,
 };
 
 static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
@@ -978,6 +963,8 @@ static const struct v4l2_subdev_pad_ops ov5647_subdev_pad_ops = {
 	.set_fmt		= ov5647_set_pad_fmt,
 	.get_fmt		= ov5647_get_pad_fmt,
 	.get_selection		= ov5647_get_selection,
+	.enable_streams		= ov5647_enable_streams,
+	.disable_streams	= ov5647_disable_streams,
 };
 
 static const struct v4l2_subdev_ops ov5647_subdev_ops = {
-- 
2.43.0

Re: [PATCH v4 1/3] media: i2c: ov5647: Convert to CCI register access helpers

[johannes.goede]

Hi,

On 1-Jan-26 11:29, Xiaolei Wang wrote:
> Use the new common CCI register access helpers to replace the private
> register access helpers in the ov5647 driver. This simplifies the driver
> by reducing the amount of code.
> 
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans



> ---
>  drivers/media/i2c/Kconfig  |   1 +
>  drivers/media/i2c/ov5647.c | 350 +++++++++++--------------------------
>  2 files changed, 103 insertions(+), 248 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 4b4db8c4f496..cce63349e71e 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -529,6 +529,7 @@ config VIDEO_OV5645
>  
>  config VIDEO_OV5647
>  	tristate "OmniVision OV5647 sensor support"
> +	select V4L2_CCI_I2C
>  	help
>  	  This is a Video4Linux2 sensor driver for the OmniVision
>  	  OV5647 camera.
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index e193fef4fced..24a160bbb079 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -20,8 +20,10 @@
>  #include <linux/module.h>
>  #include <linux/of_graph.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
>  #include <linux/slab.h>
>  #include <linux/videodev2.h>
> +#include <media/v4l2-cci.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-event.h>
> @@ -41,25 +43,23 @@
>  #define MIPI_CTRL00_BUS_IDLE			BIT(2)
>  #define MIPI_CTRL00_CLOCK_LANE_DISABLE		BIT(0)
>  
> -#define OV5647_SW_STANDBY		0x0100
> -#define OV5647_SW_RESET			0x0103
> -#define OV5647_REG_CHIPID_H		0x300a
> -#define OV5647_REG_CHIPID_L		0x300b
> -#define OV5640_REG_PAD_OUT		0x300d
> -#define OV5647_REG_EXP_HI		0x3500
> -#define OV5647_REG_EXP_MID		0x3501
> -#define OV5647_REG_EXP_LO		0x3502
> -#define OV5647_REG_AEC_AGC		0x3503
> -#define OV5647_REG_GAIN_HI		0x350a
> -#define OV5647_REG_GAIN_LO		0x350b
> -#define OV5647_REG_VTS_HI		0x380e
> -#define OV5647_REG_VTS_LO		0x380f
> -#define OV5647_REG_FRAME_OFF_NUMBER	0x4202
> -#define OV5647_REG_MIPI_CTRL00		0x4800
> -#define OV5647_REG_MIPI_CTRL14		0x4814
> -#define OV5647_REG_AWB			0x5001
> -#define OV5647_REG_ISPCTRL3D		0x503d
> -
> +#define OV5647_SW_STANDBY		CCI_REG8(0x0100)
> +#define OV5647_SW_RESET			CCI_REG8(0x0103)
> +#define OV5647_REG_CHIPID		CCI_REG16(0x300a)
> +#define OV5640_REG_PAD_OUT		CCI_REG8(0x300d)
> +#define OV5647_REG_EXPOSURE		CCI_REG24(0x3500)
> +#define OV5647_REG_AEC_AGC		CCI_REG8(0x3503)
> +#define OV5647_REG_GAIN			CCI_REG16(0x350a)
> +#define OV5647_REG_VTS			CCI_REG16(0x380e)
> +#define OV5647_REG_FRAME_OFF_NUMBER	CCI_REG8(0x4202)
> +#define OV5647_REG_MIPI_CTRL00		CCI_REG8(0x4800)
> +#define OV5647_REG_MIPI_CTRL14		CCI_REG8(0x4814)
> +#define OV5647_REG_MIPI_CTRL14_CHANNEL_MASK	GENMASK(7, 6)
> +#define OV5647_REG_MIPI_CTRL14_CHANNEL_SHIFT	6
> +#define OV5647_REG_AWB			CCI_REG8(0x5001)
> +#define OV5647_REG_ISPCTRL3D		CCI_REG8(0x503d)
> +
> +#define OV5647_CHIP_ID 0x5647
>  #define REG_TERM 0xfffe
>  #define VAL_TERM 0xfe
>  #define REG_DLY  0xffff
> @@ -81,23 +81,19 @@
>  #define OV5647_EXPOSURE_DEFAULT		1000
>  #define OV5647_EXPOSURE_MAX		65535
>  
> -struct regval_list {
> -	u16 addr;
> -	u8 data;
> -};
> -
>  struct ov5647_mode {
>  	struct v4l2_mbus_framefmt	format;
>  	struct v4l2_rect		crop;
>  	u64				pixel_rate;
>  	int				hts;
>  	int				vts;
> -	const struct regval_list	*reg_list;
> +	const struct reg_sequence	*reg_list;
>  	unsigned int			num_regs;
>  };
>  
>  struct ov5647 {
>  	struct v4l2_subdev		sd;
> +	struct regmap			*regmap;
>  	struct media_pad		pad;
>  	struct mutex			lock;
>  	struct clk			*xclk;
> @@ -130,19 +126,19 @@ static const u8 ov5647_test_pattern_val[] = {
>  	0x81,	/* Random Data */
>  };
>  
> -static const struct regval_list sensor_oe_disable_regs[] = {
> +static const struct reg_sequence sensor_oe_disable_regs[] = {
>  	{0x3000, 0x00},
>  	{0x3001, 0x00},
>  	{0x3002, 0x00},
>  };
>  
> -static const struct regval_list sensor_oe_enable_regs[] = {
> +static const struct reg_sequence sensor_oe_enable_regs[] = {
>  	{0x3000, 0x0f},
>  	{0x3001, 0xff},
>  	{0x3002, 0xe4},
>  };
>  
> -static struct regval_list ov5647_2592x1944_10bpp[] = {
> +static const struct reg_sequence ov5647_2592x1944_10bpp[] = {
>  	{0x0100, 0x00},
>  	{0x0103, 0x01},
>  	{0x3034, 0x1a},
> @@ -231,7 +227,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
>  	{0x0100, 0x01},
>  };
>  
> -static struct regval_list ov5647_1080p30_10bpp[] = {
> +static const struct reg_sequence ov5647_1080p30_10bpp[] = {
>  	{0x0100, 0x00},
>  	{0x0103, 0x01},
>  	{0x3034, 0x1a},
> @@ -320,7 +316,7 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
>  	{0x0100, 0x01},
>  };
>  
> -static struct regval_list ov5647_2x2binned_10bpp[] = {
> +static const struct reg_sequence ov5647_2x2binned_10bpp[] = {
>  	{0x0100, 0x00},
>  	{0x0103, 0x01},
>  	{0x3034, 0x1a},
> @@ -413,7 +409,7 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
>  	{0x0100, 0x01},
>  };
>  
> -static struct regval_list ov5647_640x480_10bpp[] = {
> +static const struct reg_sequence ov5647_640x480_10bpp[] = {
>  	{0x0100, 0x00},
>  	{0x0103, 0x01},
>  	{0x3035, 0x11},
> @@ -594,109 +590,29 @@ static const struct ov5647_mode ov5647_modes[] = {
>  #define OV5647_DEFAULT_MODE	(&ov5647_modes[3])
>  #define OV5647_DEFAULT_FORMAT	(ov5647_modes[3].format)
>  
> -static int ov5647_write16(struct v4l2_subdev *sd, u16 reg, u16 val)
> -{
> -	unsigned char data[4] = { reg >> 8, reg & 0xff, val >> 8, val & 0xff};
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> -	int ret;
> -
> -	ret = i2c_master_send(client, data, 4);
> -	if (ret < 0) {
> -		dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
> -			__func__, reg);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
> -static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val)
> -{
> -	unsigned char data[3] = { reg >> 8, reg & 0xff, val};
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> -	int ret;
> -
> -	ret = i2c_master_send(client, data, 3);
> -	if (ret < 0) {
> -		dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
> -				__func__, reg);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
> -static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
> -{
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> -	u8 buf[2] = { reg >> 8, reg & 0xff };
> -	struct i2c_msg msg[2];
> -	int ret;
> -
> -	msg[0].addr = client->addr;
> -	msg[0].flags = client->flags;
> -	msg[0].buf = buf;
> -	msg[0].len = sizeof(buf);
> -
> -	msg[1].addr = client->addr;
> -	msg[1].flags = client->flags | I2C_M_RD;
> -	msg[1].buf = buf;
> -	msg[1].len = 1;
> -
> -	ret = i2c_transfer(client->adapter, msg, 2);
> -	if (ret != 2) {
> -		dev_err(&client->dev, "%s: i2c read error, reg: %x = %d\n",
> -			__func__, reg, ret);
> -		return ret >= 0 ? -EINVAL : ret;
> -	}
> -
> -	*val = buf[0];
> -
> -	return 0;
> -}
> -
> -static int ov5647_write_array(struct v4l2_subdev *sd,
> -			      const struct regval_list *regs, int array_size)
> -{
> -	int i, ret;
> -
> -	for (i = 0; i < array_size; i++) {
> -		ret = ov5647_write(sd, regs[i].addr, regs[i].data);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
>  {
> -	u8 channel_id;
> -	int ret;
> -
> -	ret = ov5647_read(sd, OV5647_REG_MIPI_CTRL14, &channel_id);
> -	if (ret < 0)
> -		return ret;
> -
> -	channel_id &= ~(3 << 6);
> +	struct ov5647 *sensor = to_sensor(sd);
>  
> -	return ov5647_write(sd, OV5647_REG_MIPI_CTRL14,
> -			    channel_id | (channel << 6));
> +	return cci_update_bits(sensor->regmap, OV5647_REG_MIPI_CTRL14,
> +			       OV5647_REG_MIPI_CTRL14_CHANNEL_MASK,
> +			       channel << OV5647_REG_MIPI_CTRL14_CHANNEL_SHIFT,
> +			       NULL);
>  }
>  
>  static int ov5647_set_mode(struct v4l2_subdev *sd)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	struct ov5647 *sensor = to_sensor(sd);
> -	u8 resetval, rdval;
> +	u64 resetval, rdval;
>  	int ret;
>  
> -	ret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);
> +	ret = cci_read(sensor->regmap, OV5647_SW_STANDBY, &rdval, NULL);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = ov5647_write_array(sd, sensor->mode->reg_list,
> -				 sensor->mode->num_regs);
> +	ret = regmap_multi_reg_write(sensor->regmap, sensor->mode->reg_list,
> +				     sensor->mode->num_regs);
>  	if (ret < 0) {
>  		dev_err(&client->dev, "write sensor default regs error\n");
>  		return ret;
> @@ -706,13 +622,13 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = ov5647_read(sd, OV5647_SW_STANDBY, &resetval);
> +	ret = cci_read(sensor->regmap, OV5647_SW_STANDBY, &resetval, NULL);
>  	if (ret < 0)
>  		return ret;
>  
>  	if (!(resetval & 0x01)) {
>  		dev_err(&client->dev, "Device was in SW standby");
> -		ret = ov5647_write(sd, OV5647_SW_STANDBY, 0x01);
> +		ret = cci_write(sensor->regmap, OV5647_SW_STANDBY, 0x01, NULL);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -742,32 +658,25 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)
>  		val |= MIPI_CTRL00_CLOCK_LANE_GATE |
>  		       MIPI_CTRL00_LINE_SYNC_ENABLE;
>  
> -	ret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, val);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = ov5647_write(sd, OV5647_REG_FRAME_OFF_NUMBER, 0x00);
> -	if (ret < 0)
> -		return ret;
> +	cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL00, val, &ret);
> +	cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x00, &ret);
> +	cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x00, &ret);
>  
> -	return ov5647_write(sd, OV5640_REG_PAD_OUT, 0x00);
> +	return ret;
>  }
>  
>  static int ov5647_stream_off(struct v4l2_subdev *sd)
>  {
> -	int ret;
> -
> -	ret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00,
> -			   MIPI_CTRL00_CLOCK_LANE_GATE | MIPI_CTRL00_BUS_IDLE |
> -			   MIPI_CTRL00_CLOCK_LANE_DISABLE);
> -	if (ret < 0)
> -		return ret;
> +	struct ov5647 *sensor = to_sensor(sd);
> +	int ret = 0;
>  
> -	ret = ov5647_write(sd, OV5647_REG_FRAME_OFF_NUMBER, 0x0f);
> -	if (ret < 0)
> -		return ret;
> +	cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL00,
> +		  MIPI_CTRL00_CLOCK_LANE_GATE | MIPI_CTRL00_BUS_IDLE |
> +		  MIPI_CTRL00_CLOCK_LANE_DISABLE, &ret);
> +	cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x0f, &ret);
> +	cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x01, &ret);
>  
> -	return ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);
> +	return ret;
>  }
>  
>  static int ov5647_power_on(struct device *dev)
> @@ -788,8 +697,8 @@ static int ov5647_power_on(struct device *dev)
>  		goto error_pwdn;
>  	}
>  
> -	ret = ov5647_write_array(&sensor->sd, sensor_oe_enable_regs,
> -				 ARRAY_SIZE(sensor_oe_enable_regs));
> +	ret = regmap_multi_reg_write(sensor->regmap, sensor_oe_enable_regs,
> +				     ARRAY_SIZE(sensor_oe_enable_regs));
>  	if (ret < 0) {
>  		dev_err(dev, "write sensor_oe_enable_regs error\n");
>  		goto error_clk_disable;
> @@ -815,23 +724,17 @@ static int ov5647_power_on(struct device *dev)
>  static int ov5647_power_off(struct device *dev)
>  {
>  	struct ov5647 *sensor = dev_get_drvdata(dev);
> -	u8 rdval;
>  	int ret;
>  
>  	dev_dbg(dev, "OV5647 power off\n");
>  
> -	ret = ov5647_write_array(&sensor->sd, sensor_oe_disable_regs,
> -				 ARRAY_SIZE(sensor_oe_disable_regs));
> +	ret = regmap_multi_reg_write(sensor->regmap, sensor_oe_disable_regs,
> +				     ARRAY_SIZE(sensor_oe_disable_regs));
>  	if (ret < 0)
>  		dev_dbg(dev, "disable oe failed\n");
>  
>  	/* Enter software standby */
> -	ret = ov5647_read(&sensor->sd, OV5647_SW_STANDBY, &rdval);
> -	if (ret < 0)
> -		dev_dbg(dev, "software standby failed\n");
> -
> -	rdval &= ~0x01;
> -	ret = ov5647_write(&sensor->sd, OV5647_SW_STANDBY, rdval);
> +	ret = cci_update_bits(sensor->regmap, OV5647_SW_STANDBY, 0x01, 0x00, NULL);
>  	if (ret < 0)
>  		dev_dbg(dev, "software standby failed\n");
>  
> @@ -845,10 +748,11 @@ static int ov5647_power_off(struct device *dev)
>  static int ov5647_sensor_get_register(struct v4l2_subdev *sd,
>  				      struct v4l2_dbg_register *reg)
>  {
> +	struct ov5647 *sensor = to_sensor(sd);
>  	int ret;
> -	u8 val;
> +	u64 val;
>  
> -	ret = ov5647_read(sd, reg->reg & 0xff, &val);
> +	ret = cci_read(sensor->regmap, reg->reg & 0xff, &val, NULL);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -861,7 +765,9 @@ static int ov5647_sensor_get_register(struct v4l2_subdev *sd,
>  static int ov5647_sensor_set_register(struct v4l2_subdev *sd,
>  				      const struct v4l2_dbg_register *reg)
>  {
> -	return ov5647_write(sd, reg->reg & 0xff, reg->val & 0xff);
> +	struct ov5647 *sensor = to_sensor(sd);
> +
> +	return cci_write(sensor->regmap, reg->reg & 0xff, reg->val & 0xff, NULL);
>  }
>  #endif
>  
> @@ -1089,33 +995,27 @@ static const struct v4l2_subdev_ops ov5647_subdev_ops = {
>  
>  static int ov5647_detect(struct v4l2_subdev *sd)
>  {
> +	struct ov5647 *sensor = to_sensor(sd);
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> -	u8 read;
> +	u64 read;
>  	int ret;
>  
> -	ret = ov5647_write(sd, OV5647_SW_RESET, 0x01);
> +	ret = cci_write(sensor->regmap, OV5647_SW_RESET, 0x01, NULL);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = ov5647_read(sd, OV5647_REG_CHIPID_H, &read);
> +	ret = cci_read(sensor->regmap, OV5647_REG_CHIPID, &read, NULL);
>  	if (ret < 0)
> -		return ret;
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to read chip id %x\n",
> +				     OV5647_REG_CHIPID);
>  
> -	if (read != 0x56) {
> -		dev_err(&client->dev, "ID High expected 0x56 got %x", read);
> +	if (read != OV5647_CHIP_ID) {
> +		dev_err(&client->dev, "Chip ID expected 0x5647 got 0x%llx", read);
>  		return -ENODEV;
>  	}
>  
> -	ret = ov5647_read(sd, OV5647_REG_CHIPID_L, &read);
> -	if (ret < 0)
> -		return ret;
> -
> -	if (read != 0x47) {
> -		dev_err(&client->dev, "ID Low expected 0x47 got %x", read);
> -		return -ENODEV;
> -	}
> -
> -	return ov5647_write(sd, OV5647_SW_RESET, 0x00);
> +	return cci_write(sensor->regmap, OV5647_SW_RESET, 0x00, NULL);
>  }
>  
>  static int ov5647_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> @@ -1138,74 +1038,6 @@ static const struct v4l2_subdev_internal_ops ov5647_subdev_internal_ops = {
>  	.open = ov5647_open,
>  };
>  
> -static int ov5647_s_auto_white_balance(struct v4l2_subdev *sd, u32 val)
> -{
> -	return ov5647_write(sd, OV5647_REG_AWB, val ? 1 : 0);
> -}
> -
> -static int ov5647_s_autogain(struct v4l2_subdev *sd, u32 val)
> -{
> -	int ret;
> -	u8 reg;
> -
> -	/* Non-zero turns on AGC by clearing bit 1.*/
> -	ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
> -	if (ret)
> -		return ret;
> -
> -	return ov5647_write(sd, OV5647_REG_AEC_AGC, val ? reg & ~BIT(1)
> -							: reg | BIT(1));
> -}
> -
> -static int ov5647_s_exposure_auto(struct v4l2_subdev *sd, u32 val)
> -{
> -	int ret;
> -	u8 reg;
> -
> -	/*
> -	 * Everything except V4L2_EXPOSURE_MANUAL turns on AEC by
> -	 * clearing bit 0.
> -	 */
> -	ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
> -	if (ret)
> -		return ret;
> -
> -	return ov5647_write(sd, OV5647_REG_AEC_AGC,
> -			    val == V4L2_EXPOSURE_MANUAL ? reg | BIT(0)
> -							: reg & ~BIT(0));
> -}
> -
> -static int ov5647_s_analogue_gain(struct v4l2_subdev *sd, u32 val)
> -{
> -	int ret;
> -
> -	/* 10 bits of gain, 2 in the high register. */
> -	ret = ov5647_write(sd, OV5647_REG_GAIN_HI, (val >> 8) & 3);
> -	if (ret)
> -		return ret;
> -
> -	return ov5647_write(sd, OV5647_REG_GAIN_LO, val & 0xff);
> -}
> -
> -static int ov5647_s_exposure(struct v4l2_subdev *sd, u32 val)
> -{
> -	int ret;
> -
> -	/*
> -	 * Sensor has 20 bits, but the bottom 4 bits are fractions of a line
> -	 * which we leave as zero (and don't receive in "val").
> -	 */
> -	ret = ov5647_write(sd, OV5647_REG_EXP_HI, (val >> 12) & 0xf);
> -	if (ret)
> -		return ret;
> -
> -	ret = ov5647_write(sd, OV5647_REG_EXP_MID, (val >> 4) & 0xff);
> -	if (ret)
> -		return ret;
> -
> -	return ov5647_write(sd, OV5647_REG_EXP_LO, (val & 0xf) << 4);
> -}
> -
>  static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct ov5647 *sensor = container_of(ctrl->handler,
> @@ -1239,27 +1071,42 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
>  
>  	switch (ctrl->id) {
>  	case V4L2_CID_AUTO_WHITE_BALANCE:
> -		ret = ov5647_s_auto_white_balance(sd, ctrl->val);
> +		ret = cci_write(sensor->regmap, OV5647_REG_AWB,
> +				ctrl->val ? 1 : 0, NULL);
>  		break;
>  	case V4L2_CID_AUTOGAIN:
> -		ret = ov5647_s_autogain(sd, ctrl->val);
> +		/* Non-zero turns on AGC by clearing bit 1.*/
> +		return cci_update_bits(sensor->regmap, OV5647_REG_AEC_AGC, BIT(1),
> +				       ctrl->val ? 0 : BIT(1), NULL);
>  		break;
>  	case V4L2_CID_EXPOSURE_AUTO:
> -		ret = ov5647_s_exposure_auto(sd, ctrl->val);
> +		/*
> +		 * Everything except V4L2_EXPOSURE_MANUAL turns on AEC by
> +		 * clearing bit 0.
> +		 */
> +		return cci_update_bits(sensor->regmap, OV5647_REG_AEC_AGC, BIT(0),
> +				       ctrl->val == V4L2_EXPOSURE_MANUAL ? BIT(0) : 0, NULL);
>  		break;
>  	case V4L2_CID_ANALOGUE_GAIN:
> -		ret =  ov5647_s_analogue_gain(sd, ctrl->val);
> +		/* 10 bits of gain, 2 in the high register. */
> +		return cci_write(sensor->regmap, OV5647_REG_GAIN,
> +				 ctrl->val & 0x3ff, NULL);
>  		break;
>  	case V4L2_CID_EXPOSURE:
> -		ret = ov5647_s_exposure(sd, ctrl->val);
> +		/*
> +		 * Sensor has 20 bits, but the bottom 4 bits are fractions of a line
> +		 * which we leave as zero (and don't receive in "val").
> +		 */
> +		ret = cci_write(sensor->regmap, OV5647_REG_EXPOSURE,
> +				ctrl->val << 4, NULL);
>  		break;
>  	case V4L2_CID_VBLANK:
> -		ret = ov5647_write16(sd, OV5647_REG_VTS_HI,
> -				     sensor->mode->format.height + ctrl->val);
> +		ret = cci_write(sensor->regmap, OV5647_REG_VTS,
> +				sensor->mode->format.height + ctrl->val, NULL);
>  		break;
>  	case V4L2_CID_TEST_PATTERN:
> -		ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D,
> -				   ov5647_test_pattern_val[ctrl->val]);
> +		ret = cci_write(sensor->regmap, OV5647_REG_ISPCTRL3D,
> +				ov5647_test_pattern_val[ctrl->val], NULL);
>  		break;
>  
>  	/* Read-only, but we adjust it based on mode. */
> @@ -1435,6 +1282,13 @@ static int ov5647_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		goto ctrl_handler_free;
>  
> +	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(sensor->regmap)) {
> +		ret = dev_err_probe(dev, PTR_ERR(sensor->regmap),
> +				    "Failed to init CCI\n");
> +		goto entity_cleanup;
> +	}
> +
>  	ret = ov5647_power_on(dev);
>  	if (ret)
>  		goto entity_cleanup;

Re: [PATCH v4 1/3] media: i2c: ov5647: Convert to CCI register access helpers

[johannes.goede]

Hi,

On 1-Jan-26 11:29, Xiaolei Wang wrote:
> Use the new common CCI register access helpers to replace the private
> register access helpers in the ov5647 driver. This simplifies the driver
> by reducing the amount of code.
> 
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans



> ---
>  drivers/media/i2c/Kconfig  |   1 +
>  drivers/media/i2c/ov5647.c | 350 +++++++++++--------------------------
>  2 files changed, 103 insertions(+), 248 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 4b4db8c4f496..cce63349e71e 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -529,6 +529,7 @@ config VIDEO_OV5645
>  
>  config VIDEO_OV5647
>  	tristate "OmniVision OV5647 sensor support"
> +	select V4L2_CCI_I2C
>  	help
>  	  This is a Video4Linux2 sensor driver for the OmniVision
>  	  OV5647 camera.
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index e193fef4fced..24a160bbb079 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -20,8 +20,10 @@
>  #include <linux/module.h>
>  #include <linux/of_graph.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
>  #include <linux/slab.h>
>  #include <linux/videodev2.h>
> +#include <media/v4l2-cci.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-event.h>
> @@ -41,25 +43,23 @@
>  #define MIPI_CTRL00_BUS_IDLE			BIT(2)
>  #define MIPI_CTRL00_CLOCK_LANE_DISABLE		BIT(0)
>  
> -#define OV5647_SW_STANDBY		0x0100
> -#define OV5647_SW_RESET			0x0103
> -#define OV5647_REG_CHIPID_H		0x300a
> -#define OV5647_REG_CHIPID_L		0x300b
> -#define OV5640_REG_PAD_OUT		0x300d
> -#define OV5647_REG_EXP_HI		0x3500
> -#define OV5647_REG_EXP_MID		0x3501
> -#define OV5647_REG_EXP_LO		0x3502
> -#define OV5647_REG_AEC_AGC		0x3503
> -#define OV5647_REG_GAIN_HI		0x350a
> -#define OV5647_REG_GAIN_LO		0x350b
> -#define OV5647_REG_VTS_HI		0x380e
> -#define OV5647_REG_VTS_LO		0x380f
> -#define OV5647_REG_FRAME_OFF_NUMBER	0x4202
> -#define OV5647_REG_MIPI_CTRL00		0x4800
> -#define OV5647_REG_MIPI_CTRL14		0x4814
> -#define OV5647_REG_AWB			0x5001
> -#define OV5647_REG_ISPCTRL3D		0x503d
> -
> +#define OV5647_SW_STANDBY		CCI_REG8(0x0100)
> +#define OV5647_SW_RESET			CCI_REG8(0x0103)
> +#define OV5647_REG_CHIPID		CCI_REG16(0x300a)
> +#define OV5640_REG_PAD_OUT		CCI_REG8(0x300d)
> +#define OV5647_REG_EXPOSURE		CCI_REG24(0x3500)
> +#define OV5647_REG_AEC_AGC		CCI_REG8(0x3503)
> +#define OV5647_REG_GAIN			CCI_REG16(0x350a)
> +#define OV5647_REG_VTS			CCI_REG16(0x380e)
> +#define OV5647_REG_FRAME_OFF_NUMBER	CCI_REG8(0x4202)
> +#define OV5647_REG_MIPI_CTRL00		CCI_REG8(0x4800)
> +#define OV5647_REG_MIPI_CTRL14		CCI_REG8(0x4814)
> +#define OV5647_REG_MIPI_CTRL14_CHANNEL_MASK	GENMASK(7, 6)
> +#define OV5647_REG_MIPI_CTRL14_CHANNEL_SHIFT	6
> +#define OV5647_REG_AWB			CCI_REG8(0x5001)
> +#define OV5647_REG_ISPCTRL3D		CCI_REG8(0x503d)
> +
> +#define OV5647_CHIP_ID 0x5647
>  #define REG_TERM 0xfffe
>  #define VAL_TERM 0xfe
>  #define REG_DLY  0xffff
> @@ -81,23 +81,19 @@
>  #define OV5647_EXPOSURE_DEFAULT		1000
>  #define OV5647_EXPOSURE_MAX		65535
>  
> -struct regval_list {
> -	u16 addr;
> -	u8 data;
> -};
> -
>  struct ov5647_mode {
>  	struct v4l2_mbus_framefmt	format;
>  	struct v4l2_rect		crop;
>  	u64				pixel_rate;
>  	int				hts;
>  	int				vts;
> -	const struct regval_list	*reg_list;
> +	const struct reg_sequence	*reg_list;
>  	unsigned int			num_regs;
>  };
>  
>  struct ov5647 {
>  	struct v4l2_subdev		sd;
> +	struct regmap			*regmap;
>  	struct media_pad		pad;
>  	struct mutex			lock;
>  	struct clk			*xclk;
> @@ -130,19 +126,19 @@ static const u8 ov5647_test_pattern_val[] = {
>  	0x81,	/* Random Data */
>  };
>  
> -static const struct regval_list sensor_oe_disable_regs[] = {
> +static const struct reg_sequence sensor_oe_disable_regs[] = {
>  	{0x3000, 0x00},
>  	{0x3001, 0x00},
>  	{0x3002, 0x00},
>  };
>  
> -static const struct regval_list sensor_oe_enable_regs[] = {
> +static const struct reg_sequence sensor_oe_enable_regs[] = {
>  	{0x3000, 0x0f},
>  	{0x3001, 0xff},
>  	{0x3002, 0xe4},
>  };
>  
> -static struct regval_list ov5647_2592x1944_10bpp[] = {
> +static const struct reg_sequence ov5647_2592x1944_10bpp[] = {
>  	{0x0100, 0x00},
>  	{0x0103, 0x01},
>  	{0x3034, 0x1a},
> @@ -231,7 +227,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
>  	{0x0100, 0x01},
>  };
>  
> -static struct regval_list ov5647_1080p30_10bpp[] = {
> +static const struct reg_sequence ov5647_1080p30_10bpp[] = {
>  	{0x0100, 0x00},
>  	{0x0103, 0x01},
>  	{0x3034, 0x1a},
> @@ -320,7 +316,7 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
>  	{0x0100, 0x01},
>  };
>  
> -static struct regval_list ov5647_2x2binned_10bpp[] = {
> +static const struct reg_sequence ov5647_2x2binned_10bpp[] = {
>  	{0x0100, 0x00},
>  	{0x0103, 0x01},
>  	{0x3034, 0x1a},
> @@ -413,7 +409,7 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
>  	{0x0100, 0x01},
>  };
>  
> -static struct regval_list ov5647_640x480_10bpp[] = {
> +static const struct reg_sequence ov5647_640x480_10bpp[] = {
>  	{0x0100, 0x00},
>  	{0x0103, 0x01},
>  	{0x3035, 0x11},
> @@ -594,109 +590,29 @@ static const struct ov5647_mode ov5647_modes[] = {
>  #define OV5647_DEFAULT_MODE	(&ov5647_modes[3])
>  #define OV5647_DEFAULT_FORMAT	(ov5647_modes[3].format)
>  
> -static int ov5647_write16(struct v4l2_subdev *sd, u16 reg, u16 val)
> -{
> -	unsigned char data[4] = { reg >> 8, reg & 0xff, val >> 8, val & 0xff};
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> -	int ret;
> -
> -	ret = i2c_master_send(client, data, 4);
> -	if (ret < 0) {
> -		dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
> -			__func__, reg);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
> -static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val)
> -{
> -	unsigned char data[3] = { reg >> 8, reg & 0xff, val};
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> -	int ret;
> -
> -	ret = i2c_master_send(client, data, 3);
> -	if (ret < 0) {
> -		dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
> -				__func__, reg);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
> -static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
> -{
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> -	u8 buf[2] = { reg >> 8, reg & 0xff };
> -	struct i2c_msg msg[2];
> -	int ret;
> -
> -	msg[0].addr = client->addr;
> -	msg[0].flags = client->flags;
> -	msg[0].buf = buf;
> -	msg[0].len = sizeof(buf);
> -
> -	msg[1].addr = client->addr;
> -	msg[1].flags = client->flags | I2C_M_RD;
> -	msg[1].buf = buf;
> -	msg[1].len = 1;
> -
> -	ret = i2c_transfer(client->adapter, msg, 2);
> -	if (ret != 2) {
> -		dev_err(&client->dev, "%s: i2c read error, reg: %x = %d\n",
> -			__func__, reg, ret);
> -		return ret >= 0 ? -EINVAL : ret;
> -	}
> -
> -	*val = buf[0];
> -
> -	return 0;
> -}
> -
> -static int ov5647_write_array(struct v4l2_subdev *sd,
> -			      const struct regval_list *regs, int array_size)
> -{
> -	int i, ret;
> -
> -	for (i = 0; i < array_size; i++) {
> -		ret = ov5647_write(sd, regs[i].addr, regs[i].data);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
>  {
> -	u8 channel_id;
> -	int ret;
> -
> -	ret = ov5647_read(sd, OV5647_REG_MIPI_CTRL14, &channel_id);
> -	if (ret < 0)
> -		return ret;
> -
> -	channel_id &= ~(3 << 6);
> +	struct ov5647 *sensor = to_sensor(sd);
>  
> -	return ov5647_write(sd, OV5647_REG_MIPI_CTRL14,
> -			    channel_id | (channel << 6));
> +	return cci_update_bits(sensor->regmap, OV5647_REG_MIPI_CTRL14,
> +			       OV5647_REG_MIPI_CTRL14_CHANNEL_MASK,
> +			       channel << OV5647_REG_MIPI_CTRL14_CHANNEL_SHIFT,
> +			       NULL);
>  }
>  
>  static int ov5647_set_mode(struct v4l2_subdev *sd)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	struct ov5647 *sensor = to_sensor(sd);
> -	u8 resetval, rdval;
> +	u64 resetval, rdval;
>  	int ret;
>  
> -	ret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);
> +	ret = cci_read(sensor->regmap, OV5647_SW_STANDBY, &rdval, NULL);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = ov5647_write_array(sd, sensor->mode->reg_list,
> -				 sensor->mode->num_regs);
> +	ret = regmap_multi_reg_write(sensor->regmap, sensor->mode->reg_list,
> +				     sensor->mode->num_regs);
>  	if (ret < 0) {
>  		dev_err(&client->dev, "write sensor default regs error\n");
>  		return ret;
> @@ -706,13 +622,13 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = ov5647_read(sd, OV5647_SW_STANDBY, &resetval);
> +	ret = cci_read(sensor->regmap, OV5647_SW_STANDBY, &resetval, NULL);
>  	if (ret < 0)
>  		return ret;
>  
>  	if (!(resetval & 0x01)) {
>  		dev_err(&client->dev, "Device was in SW standby");
> -		ret = ov5647_write(sd, OV5647_SW_STANDBY, 0x01);
> +		ret = cci_write(sensor->regmap, OV5647_SW_STANDBY, 0x01, NULL);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -742,32 +658,25 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)
>  		val |= MIPI_CTRL00_CLOCK_LANE_GATE |
>  		       MIPI_CTRL00_LINE_SYNC_ENABLE;
>  
> -	ret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, val);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = ov5647_write(sd, OV5647_REG_FRAME_OFF_NUMBER, 0x00);
> -	if (ret < 0)
> -		return ret;
> +	cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL00, val, &ret);
> +	cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x00, &ret);
> +	cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x00, &ret);
>  
> -	return ov5647_write(sd, OV5640_REG_PAD_OUT, 0x00);
> +	return ret;
>  }
>  
>  static int ov5647_stream_off(struct v4l2_subdev *sd)
>  {
> -	int ret;
> -
> -	ret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00,
> -			   MIPI_CTRL00_CLOCK_LANE_GATE | MIPI_CTRL00_BUS_IDLE |
> -			   MIPI_CTRL00_CLOCK_LANE_DISABLE);
> -	if (ret < 0)
> -		return ret;
> +	struct ov5647 *sensor = to_sensor(sd);
> +	int ret = 0;
>  
> -	ret = ov5647_write(sd, OV5647_REG_FRAME_OFF_NUMBER, 0x0f);
> -	if (ret < 0)
> -		return ret;
> +	cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL00,
> +		  MIPI_CTRL00_CLOCK_LANE_GATE | MIPI_CTRL00_BUS_IDLE |
> +		  MIPI_CTRL00_CLOCK_LANE_DISABLE, &ret);
> +	cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x0f, &ret);
> +	cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x01, &ret);
>  
> -	return ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);
> +	return ret;
>  }
>  
>  static int ov5647_power_on(struct device *dev)
> @@ -788,8 +697,8 @@ static int ov5647_power_on(struct device *dev)
>  		goto error_pwdn;
>  	}
>  
> -	ret = ov5647_write_array(&sensor->sd, sensor_oe_enable_regs,
> -				 ARRAY_SIZE(sensor_oe_enable_regs));
> +	ret = regmap_multi_reg_write(sensor->regmap, sensor_oe_enable_regs,
> +				     ARRAY_SIZE(sensor_oe_enable_regs));
>  	if (ret < 0) {
>  		dev_err(dev, "write sensor_oe_enable_regs error\n");
>  		goto error_clk_disable;
> @@ -815,23 +724,17 @@ static int ov5647_power_on(struct device *dev)
>  static int ov5647_power_off(struct device *dev)
>  {
>  	struct ov5647 *sensor = dev_get_drvdata(dev);
> -	u8 rdval;
>  	int ret;
>  
>  	dev_dbg(dev, "OV5647 power off\n");
>  
> -	ret = ov5647_write_array(&sensor->sd, sensor_oe_disable_regs,
> -				 ARRAY_SIZE(sensor_oe_disable_regs));
> +	ret = regmap_multi_reg_write(sensor->regmap, sensor_oe_disable_regs,
> +				     ARRAY_SIZE(sensor_oe_disable_regs));
>  	if (ret < 0)
>  		dev_dbg(dev, "disable oe failed\n");
>  
>  	/* Enter software standby */
> -	ret = ov5647_read(&sensor->sd, OV5647_SW_STANDBY, &rdval);
> -	if (ret < 0)
> -		dev_dbg(dev, "software standby failed\n");
> -
> -	rdval &= ~0x01;
> -	ret = ov5647_write(&sensor->sd, OV5647_SW_STANDBY, rdval);
> +	ret = cci_update_bits(sensor->regmap, OV5647_SW_STANDBY, 0x01, 0x00, NULL);
>  	if (ret < 0)
>  		dev_dbg(dev, "software standby failed\n");
>  
> @@ -845,10 +748,11 @@ static int ov5647_power_off(struct device *dev)
>  static int ov5647_sensor_get_register(struct v4l2_subdev *sd,
>  				      struct v4l2_dbg_register *reg)
>  {
> +	struct ov5647 *sensor = to_sensor(sd);
>  	int ret;
> -	u8 val;
> +	u64 val;
>  
> -	ret = ov5647_read(sd, reg->reg & 0xff, &val);
> +	ret = cci_read(sensor->regmap, reg->reg & 0xff, &val, NULL);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -861,7 +765,9 @@ static int ov5647_sensor_get_register(struct v4l2_subdev *sd,
>  static int ov5647_sensor_set_register(struct v4l2_subdev *sd,
>  				      const struct v4l2_dbg_register *reg)
>  {
> -	return ov5647_write(sd, reg->reg & 0xff, reg->val & 0xff);
> +	struct ov5647 *sensor = to_sensor(sd);
> +
> +	return cci_write(sensor->regmap, reg->reg & 0xff, reg->val & 0xff, NULL);
>  }
>  #endif
>  
> @@ -1089,33 +995,27 @@ static const struct v4l2_subdev_ops ov5647_subdev_ops = {
>  
>  static int ov5647_detect(struct v4l2_subdev *sd)
>  {
> +	struct ov5647 *sensor = to_sensor(sd);
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> -	u8 read;
> +	u64 read;
>  	int ret;
>  
> -	ret = ov5647_write(sd, OV5647_SW_RESET, 0x01);
> +	ret = cci_write(sensor->regmap, OV5647_SW_RESET, 0x01, NULL);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = ov5647_read(sd, OV5647_REG_CHIPID_H, &read);
> +	ret = cci_read(sensor->regmap, OV5647_REG_CHIPID, &read, NULL);
>  	if (ret < 0)
> -		return ret;
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to read chip id %x\n",
> +				     OV5647_REG_CHIPID);
>  
> -	if (read != 0x56) {
> -		dev_err(&client->dev, "ID High expected 0x56 got %x", read);
> +	if (read != OV5647_CHIP_ID) {
> +		dev_err(&client->dev, "Chip ID expected 0x5647 got 0x%llx", read);
>  		return -ENODEV;
>  	}
>  
> -	ret = ov5647_read(sd, OV5647_REG_CHIPID_L, &read);
> -	if (ret < 0)
> -		return ret;
> -
> -	if (read != 0x47) {
> -		dev_err(&client->dev, "ID Low expected 0x47 got %x", read);
> -		return -ENODEV;
> -	}
> -
> -	return ov5647_write(sd, OV5647_SW_RESET, 0x00);
> +	return cci_write(sensor->regmap, OV5647_SW_RESET, 0x00, NULL);
>  }
>  
>  static int ov5647_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> @@ -1138,74 +1038,6 @@ static const struct v4l2_subdev_internal_ops ov5647_subdev_internal_ops = {
>  	.open = ov5647_open,
>  };
>  
> -static int ov5647_s_auto_white_balance(struct v4l2_subdev *sd, u32 val)
> -{
> -	return ov5647_write(sd, OV5647_REG_AWB, val ? 1 : 0);
> -}
> -
> -static int ov5647_s_autogain(struct v4l2_subdev *sd, u32 val)
> -{
> -	int ret;
> -	u8 reg;
> -
> -	/* Non-zero turns on AGC by clearing bit 1.*/
> -	ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
> -	if (ret)
> -		return ret;
> -
> -	return ov5647_write(sd, OV5647_REG_AEC_AGC, val ? reg & ~BIT(1)
> -							: reg | BIT(1));
> -}
> -
> -static int ov5647_s_exposure_auto(struct v4l2_subdev *sd, u32 val)
> -{
> -	int ret;
> -	u8 reg;
> -
> -	/*
> -	 * Everything except V4L2_EXPOSURE_MANUAL turns on AEC by
> -	 * clearing bit 0.
> -	 */
> -	ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
> -	if (ret)
> -		return ret;
> -
> -	return ov5647_write(sd, OV5647_REG_AEC_AGC,
> -			    val == V4L2_EXPOSURE_MANUAL ? reg | BIT(0)
> -							: reg & ~BIT(0));
> -}
> -
> -static int ov5647_s_analogue_gain(struct v4l2_subdev *sd, u32 val)
> -{
> -	int ret;
> -
> -	/* 10 bits of gain, 2 in the high register. */
> -	ret = ov5647_write(sd, OV5647_REG_GAIN_HI, (val >> 8) & 3);
> -	if (ret)
> -		return ret;
> -
> -	return ov5647_write(sd, OV5647_REG_GAIN_LO, val & 0xff);
> -}
> -
> -static int ov5647_s_exposure(struct v4l2_subdev *sd, u32 val)
> -{
> -	int ret;
> -
> -	/*
> -	 * Sensor has 20 bits, but the bottom 4 bits are fractions of a line
> -	 * which we leave as zero (and don't receive in "val").
> -	 */
> -	ret = ov5647_write(sd, OV5647_REG_EXP_HI, (val >> 12) & 0xf);
> -	if (ret)
> -		return ret;
> -
> -	ret = ov5647_write(sd, OV5647_REG_EXP_MID, (val >> 4) & 0xff);
> -	if (ret)
> -		return ret;
> -
> -	return ov5647_write(sd, OV5647_REG_EXP_LO, (val & 0xf) << 4);
> -}
> -
>  static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct ov5647 *sensor = container_of(ctrl->handler,
> @@ -1239,27 +1071,42 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
>  
>  	switch (ctrl->id) {
>  	case V4L2_CID_AUTO_WHITE_BALANCE:
> -		ret = ov5647_s_auto_white_balance(sd, ctrl->val);
> +		ret = cci_write(sensor->regmap, OV5647_REG_AWB,
> +				ctrl->val ? 1 : 0, NULL);
>  		break;
>  	case V4L2_CID_AUTOGAIN:
> -		ret = ov5647_s_autogain(sd, ctrl->val);
> +		/* Non-zero turns on AGC by clearing bit 1.*/
> +		return cci_update_bits(sensor->regmap, OV5647_REG_AEC_AGC, BIT(1),
> +				       ctrl->val ? 0 : BIT(1), NULL);
>  		break;
>  	case V4L2_CID_EXPOSURE_AUTO:
> -		ret = ov5647_s_exposure_auto(sd, ctrl->val);
> +		/*
> +		 * Everything except V4L2_EXPOSURE_MANUAL turns on AEC by
> +		 * clearing bit 0.
> +		 */
> +		return cci_update_bits(sensor->regmap, OV5647_REG_AEC_AGC, BIT(0),
> +				       ctrl->val == V4L2_EXPOSURE_MANUAL ? BIT(0) : 0, NULL);
>  		break;
>  	case V4L2_CID_ANALOGUE_GAIN:
> -		ret =  ov5647_s_analogue_gain(sd, ctrl->val);
> +		/* 10 bits of gain, 2 in the high register. */
> +		return cci_write(sensor->regmap, OV5647_REG_GAIN,
> +				 ctrl->val & 0x3ff, NULL);
>  		break;
>  	case V4L2_CID_EXPOSURE:
> -		ret = ov5647_s_exposure(sd, ctrl->val);
> +		/*
> +		 * Sensor has 20 bits, but the bottom 4 bits are fractions of a line
> +		 * which we leave as zero (and don't receive in "val").
> +		 */
> +		ret = cci_write(sensor->regmap, OV5647_REG_EXPOSURE,
> +				ctrl->val << 4, NULL);
>  		break;
>  	case V4L2_CID_VBLANK:
> -		ret = ov5647_write16(sd, OV5647_REG_VTS_HI,
> -				     sensor->mode->format.height + ctrl->val);
> +		ret = cci_write(sensor->regmap, OV5647_REG_VTS,
> +				sensor->mode->format.height + ctrl->val, NULL);
>  		break;
>  	case V4L2_CID_TEST_PATTERN:
> -		ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D,
> -				   ov5647_test_pattern_val[ctrl->val]);
> +		ret = cci_write(sensor->regmap, OV5647_REG_ISPCTRL3D,
> +				ov5647_test_pattern_val[ctrl->val], NULL);
>  		break;
>  
>  	/* Read-only, but we adjust it based on mode. */
> @@ -1435,6 +1282,13 @@ static int ov5647_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		goto ctrl_handler_free;
>  
> +	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(sensor->regmap)) {
> +		ret = dev_err_probe(dev, PTR_ERR(sensor->regmap),
> +				    "Failed to init CCI\n");
> +		goto entity_cleanup;
> +	}
> +
>  	ret = ov5647_power_on(dev);
>  	if (ret)
>  		goto entity_cleanup;

Re: [PATCH v4 2/3] media: i2c: ov5647: Switch to using the sub-device state lock

[Sakari Ailus]

Hi Xiaolei,

On Thu, Jan 01, 2026 at 06:30:00PM +0800, Xiaolei Wang wrote:
> @@ -1297,9 +1285,16 @@ static int ov5647_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		goto power_off;
>  
> +	sd->state_lock = sensor->ctrls.lock;
> +	ret = v4l2_subdev_init_finalize(sd);
> +	if (ret < 0) {
> +		ret = dev_err_probe(dev, ret, "failed to init subdev\n");
> +		goto power_off;
> +	}
> +
>  	ret = v4l2_async_register_subdev(sd);

v4l2_async_register_subdev() needs to be called before enabling Runtime PM
below. Not a fault of this patch but it'd be nice to fix that. 

>  	if (ret < 0)
> -		goto power_off;
> +		goto v4l2_subdev_cleanup;
>  
>  	/* Enable runtime PM and turn off the device */
>  	pm_runtime_set_active(dev);

The first patch doesn't seem to apply; I've taken earlier a set of ov5647
patches from Dave, could you rebase this on them? They're in my "cleanup"
branch here
<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=cleanup>.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 2/3] media: i2c: ov5647: Switch to using the sub-device state lock

[xiaolei wang]

Hi Sakari,

On 1/8/26 06:38, Sakari Ailus wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> Hi Xiaolei,
>
> On Thu, Jan 01, 2026 at 06:30:00PM +0800, Xiaolei Wang wrote:
>> @@ -1297,9 +1285,16 @@ static int ov5647_probe(struct i2c_client *client)
>>        if (ret < 0)
>>                goto power_off;
>>
>> +     sd->state_lock = sensor->ctrls.lock;
>> +     ret = v4l2_subdev_init_finalize(sd);
>> +     if (ret < 0) {
>> +             ret = dev_err_probe(dev, ret, "failed to init subdev\n");
>> +             goto power_off;
>> +     }
>> +
>>        ret = v4l2_async_register_subdev(sd);
> v4l2_async_register_subdev() needs to be called before enabling Runtime PM
> below. Not a fault of this patch but it'd be nice to fix that.
Thank you for the review and feedback.

You're absolutely right about the Runtime PM timing issue with

v4l2_async_register_subdev(). I appreciate you pointing that out - I'll

address this in the next version.

>
>>        if (ret < 0)
>> -             goto power_off;
>> +             goto v4l2_subdev_cleanup;
>>
>>        /* Enable runtime PM and turn off the device */
>>        pm_runtime_set_active(dev);
> The first patch doesn't seem to apply; I've taken earlier a set of ov5647
> patches from Dave, could you rebase this on them? They're in my "cleanup"
> branch here
> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=cleanup>.

I apologize for the conflict with the first patch. I wasn't aware of Dave's

patches that you had already taken. I'll rebase my series on your cleanup

branch and resend a v5.

Best regards,
Xiaolei

>
> --
> Kind regards,
>
> Sakari Ailus