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

[PATCH v3 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-still --output test.jpg
rpicam-still -o long_exposure.jpg --shutter 100000000 --gain 1 --awbgains 1,1 --immediate

Changes in V3:

 - 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 | 389 +++++++++++++------------------------
 2 files changed, 140 insertions(+), 250 deletions(-)

-- 
2.43.0

[PATCH v3 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>
---
 drivers/media/i2c/Kconfig  |   1 +
 drivers/media/i2c/ov5647.c | 289 +++++++++++++------------------------
 2 files changed, 99 insertions(+), 191 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..cbcb760ba5cd 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,24 +43,19 @@
 #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_AWB			CCI_REG8(0x5001)
+#define OV5647_REG_ISPCTRL3D		CCI_REG8(0x503d)
 
 #define REG_TERM 0xfffe
 #define VAL_TERM 0xfe
@@ -81,23 +78,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 +123,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},
@@ -230,8 +223,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
 	{0x3503, 0x03},
 	{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 +312,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 +405,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 +586,35 @@ 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;
+	struct ov5647 *sensor = to_sensor(sd);
+	u64 channel_id;
 	int ret;
 
-	ret = ov5647_read(sd, OV5647_REG_MIPI_CTRL14, &channel_id);
+	ret = cci_read(sensor->regmap, OV5647_REG_MIPI_CTRL14, &channel_id, NULL);
 	if (ret < 0)
 		return ret;
 
 	channel_id &= ~(3 << 6);
 
-	return ov5647_write(sd, OV5647_REG_MIPI_CTRL14,
-			    channel_id | (channel << 6));
+	return cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL14,
+			 channel_id | (channel << 6), 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 +624,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;
 	}
@@ -725,7 +643,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov5647 *sensor = to_sensor(sd);
 	u8 val = MIPI_CTRL00_BUS_IDLE;
-	int ret;
+	int ret = 0;
 
 	ret = ov5647_set_mode(sd);
 	if (ret) {
@@ -742,32 +660,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;
+	struct ov5647 *sensor = to_sensor(sd);
+	int ret = 0;
 
-	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;
+	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_write(sd, OV5647_REG_FRAME_OFF_NUMBER, 0x0f);
-	if (ret < 0)
-		return ret;
-
-	return ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);
+	return ret;
 }
 
 static int ov5647_power_on(struct device *dev)
@@ -788,8 +699,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 +726,23 @@ 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;
+	u64 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);
+	ret = cci_read(sensor->regmap, OV5647_SW_STANDBY, &rdval, NULL);
 	if (ret < 0)
 		dev_dbg(dev, "software standby failed\n");
 
 	rdval &= ~0x01;
-	ret = ov5647_write(&sensor->sd, OV5647_SW_STANDBY, rdval);
+	ret = cci_write(sensor->regmap, OV5647_SW_STANDBY, rdval, NULL);
 	if (ret < 0)
 		dev_dbg(dev, "software standby failed\n");
 
@@ -845,10 +756,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 +773,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 +1003,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);
-	if (ret < 0)
-		return ret;
-
-	if (read != 0x56) {
-		dev_err(&client->dev, "ID High expected 0x56 got %x", read);
-		return -ENODEV;
-	}
-
-	ret = ov5647_read(sd, OV5647_REG_CHIPID_L, &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 != 0x47) {
-		dev_err(&client->dev, "ID Low expected 0x47 got %x", read);
+	if (read != 0x5647) {
+		dev_err(&client->dev, "Chip ID expected 0x5647 got 0x%llx", 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)
@@ -1140,70 +1048,62 @@ static const struct v4l2_subdev_internal_ops ov5647_subdev_internal_ops = {
 
 static int ov5647_s_auto_white_balance(struct v4l2_subdev *sd, u32 val)
 {
-	return ov5647_write(sd, OV5647_REG_AWB, val ? 1 : 0);
+	struct ov5647 *sensor = to_sensor(sd);
+
+	return cci_write(sensor->regmap, OV5647_REG_AWB, val ? 1 : 0, NULL);
 }
 
 static int ov5647_s_autogain(struct v4l2_subdev *sd, u32 val)
 {
+	struct ov5647 *sensor = to_sensor(sd);
 	int ret;
-	u8 reg;
+	u64 reg;
 
 	/* Non-zero turns on AGC by clearing bit 1.*/
-	ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
+	ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, &reg, NULL);
 	if (ret)
 		return ret;
 
-	return ov5647_write(sd, OV5647_REG_AEC_AGC, val ? reg & ~BIT(1)
-							: reg | BIT(1));
+	return cci_write(sensor->regmap, OV5647_REG_AEC_AGC, val ? reg & ~BIT(1)
+							: reg | BIT(1), NULL);
 }
 
 static int ov5647_s_exposure_auto(struct v4l2_subdev *sd, u32 val)
 {
+	struct ov5647 *sensor = to_sensor(sd);
 	int ret;
-	u8 reg;
+	u64 reg;
 
 	/*
 	 * Everything except V4L2_EXPOSURE_MANUAL turns on AEC by
 	 * clearing bit 0.
 	 */
-	ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
+	ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, &reg, NULL);
 	if (ret)
 		return ret;
 
-	return ov5647_write(sd, OV5647_REG_AEC_AGC,
+	return cci_write(sensor->regmap, OV5647_REG_AEC_AGC,
 			    val == V4L2_EXPOSURE_MANUAL ? reg | BIT(0)
-							: reg & ~BIT(0));
+							: reg & ~BIT(0), NULL);
 }
 
 static int ov5647_s_analogue_gain(struct v4l2_subdev *sd, u32 val)
 {
-	int ret;
+	struct ov5647 *sensor = to_sensor(sd);
 
 	/* 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);
+	return cci_write(sensor->regmap, OV5647_REG_GAIN, val & 0x3ff, NULL);
 }
 
 static int ov5647_s_exposure(struct v4l2_subdev *sd, u32 val)
 {
-	int ret;
+	struct ov5647 *sensor = to_sensor(sd);
 
 	/*
 	 * 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);
+	return cci_write(sensor->regmap, OV5647_REG_EXPOSURE, val << 4, NULL);
 }
 
 static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
@@ -1254,12 +1154,12 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
 		ret = ov5647_s_exposure(sd, ctrl->val);
 		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 +1335,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

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

[Tarang Raval]

Hi Xiaolei,

> 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>
> ---
>  drivers/media/i2c/Kconfig  |   1 +
>  drivers/media/i2c/ov5647.c | 289 +++++++++++++------------------------
>  2 files changed, 99 insertions(+), 191 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..cbcb760ba5cd 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,24 +43,19 @@
>  #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_AWB                 CCI_REG8(0x5001)
> +#define OV5647_REG_ISPCTRL3D           CCI_REG8(0x503d)
>  
>  #define REG_TERM 0xfffe
>  #define VAL_TERM 0xfe
> @@ -81,23 +78,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 +123,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},
> @@ -230,8 +223,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
>     {0x3503, 0x03},
>     {0x0100, 0x01},
>  };
> -

Please keep one blank line here. do not remove.

> -static struct regval_list ov5647_1080p30_10bpp[] = {
> +static const struct reg_sequence ov5647_1080p30_10bpp[] = {
>     {0x0100, 0x00},
>     {0x0103, 0x01},
>     {0x3034, 0x1a},
> @@ -320,7 +312,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 +405,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 +586,35 @@ 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;
> +   struct ov5647 *sensor = to_sensor(sd);
> +   u64 channel_id;
>     int ret;
>  
> -   ret = ov5647_read(sd, OV5647_REG_MIPI_CTRL14, &channel_id);
> +   ret = cci_read(sensor->regmap, OV5647_REG_MIPI_CTRL14, &channel_id, NULL);
>     if (ret < 0)
>           return ret;
>  
>     channel_id &= ~(3 << 6);
>  
> -   return ov5647_write(sd, OV5647_REG_MIPI_CTRL14,
> -                   channel_id | (channel << 6));
> +   return cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL14,
> +                channel_id | (channel << 6), 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 +624,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;

This feels wrong to me but anyway this is not related to your patch.

>     }
> @@ -725,7 +643,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)
>     struct i2c_client *client = v4l2_get_subdevdata(sd);
>     struct ov5647 *sensor = to_sensor(sd);
>     u8 val = MIPI_CTRL00_BUS_IDLE;
> -   int ret;
> +   int ret = 0;

No need for zero initialization.

>  
>     ret = ov5647_set_mode(sd);
>     if (ret) {
> @@ -742,32 +660,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;
> +   struct ov5647 *sensor = to_sensor(sd);
> +   int ret = 0;
>  
> -   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;
> +   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_write(sd, OV5647_REG_FRAME_OFF_NUMBER, 0x0f);
> -   if (ret < 0)
> -         return ret;
> -
> -   return ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);
> +   return ret;
>  }
>  
>  static int ov5647_power_on(struct device *dev)
> @@ -788,8 +699,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 +726,23 @@ 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;
> +   u64 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);
> +   ret = cci_read(sensor->regmap, OV5647_SW_STANDBY, &rdval, NULL);
>     if (ret < 0)
>           dev_dbg(dev, "software standby failed\n");
>  
>     rdval &= ~0x01;
> -   ret = ov5647_write(&sensor->sd, OV5647_SW_STANDBY, rdval);
> +   ret = cci_write(sensor->regmap, OV5647_SW_STANDBY, rdval, NULL);
>     if (ret < 0)
>           dev_dbg(dev, "software standby failed\n");
>  
> @@ -845,10 +756,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 +773,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 +1003,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);
> -   if (ret < 0)
> -         return ret;
> -
> -   if (read != 0x56) {
> -         dev_err(&client->dev, "ID High expected 0x56 got %x", read);
> -         return -ENODEV;
> -   }
> -
> -   ret = ov5647_read(sd, OV5647_REG_CHIPID_L, &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 != 0x47) {
> -         dev_err(&client->dev, "ID Low expected 0x47 got %x", read);
> +   if (read != 0x5647) {

We should define a macro for the chip ID and use it here.

> +         dev_err(&client->dev, "Chip ID expected 0x5647 got 0x%llx", 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)
> @@ -1140,70 +1048,62 @@ static const struct v4l2_subdev_internal_ops ov5647_subdev_internal_ops = {
>  
>  static int ov5647_s_auto_white_balance(struct v4l2_subdev *sd, u32 val)
>  {
> -   return ov5647_write(sd, OV5647_REG_AWB, val ? 1 : 0);
> +   struct ov5647 *sensor = to_sensor(sd);
> +
> +   return cci_write(sensor->regmap, OV5647_REG_AWB, val ? 1 : 0, NULL);
>  }
>  
>  static int ov5647_s_autogain(struct v4l2_subdev *sd, u32 val)
>  {
> +   struct ov5647 *sensor = to_sensor(sd);
>     int ret;
> -   u8 reg;
> +   u64 reg;
>  
>     /* Non-zero turns on AGC by clearing bit 1.*/
> -   ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
> +   ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, &reg, NULL);
>     if (ret)
>           return ret;
>  
> -   return ov5647_write(sd, OV5647_REG_AEC_AGC, val ? reg & ~BIT(1)
> -                                       : reg | BIT(1));
> +   return cci_write(sensor->regmap, OV5647_REG_AEC_AGC, val ? reg & ~BIT(1)
> +                                       : reg | BIT(1), NULL);
>  }
>  
>  static int ov5647_s_exposure_auto(struct v4l2_subdev *sd, u32 val)
>  {
> +   struct ov5647 *sensor = to_sensor(sd);
>     int ret;
> -   u8 reg;
> +   u64 reg;
>  
>     /*
>      * Everything except V4L2_EXPOSURE_MANUAL turns on AEC by
>      * clearing bit 0.
>      */
> -   ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
> +   ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, &reg, NULL);
>     if (ret)
>           return ret;
>  
> -   return ov5647_write(sd, OV5647_REG_AEC_AGC,
> +   return cci_write(sensor->regmap, OV5647_REG_AEC_AGC,
>                     val == V4L2_EXPOSURE_MANUAL ? reg | BIT(0)
> -                                       : reg & ~BIT(0));
> +                                       : reg & ~BIT(0), NULL);
>  }
>  
>  static int ov5647_s_analogue_gain(struct v4l2_subdev *sd, u32 val)
>  {
> -   int ret;
> +   struct ov5647 *sensor = to_sensor(sd);
>  
>     /* 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);
> +   return cci_write(sensor->regmap, OV5647_REG_GAIN, val & 0x3ff, NULL);
>  }
>  
>  static int ov5647_s_exposure(struct v4l2_subdev *sd, u32 val)
>  {
> -   int ret;
> +   struct ov5647 *sensor = to_sensor(sd);
>  
>     /*
>      * 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);
> +   return cci_write(sensor->regmap, OV5647_REG_EXPOSURE, val << 4, NULL);
>  }

We can now drop all contorls functions (except ov5647_s_exposure_auto / ov5647_s_autogain).
Since there is only a single register write, we can write the register directly
in the set control(like vblank). 

>  
>  static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
> @@ -1254,12 +1154,12 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
>           ret = ov5647_s_exposure(sd, ctrl->val);
>           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 +1335,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

with above changes

Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io> 

Best Regards,
Tarang

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

[xiaolei wang]

On 12/31/25 18:26, Tarang Raval 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,
>
>> 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>
>> ---
>>   drivers/media/i2c/Kconfig  |   1 +
>>   drivers/media/i2c/ov5647.c | 289 +++++++++++++------------------------
>>   2 files changed, 99 insertions(+), 191 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..cbcb760ba5cd 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,24 +43,19 @@
>>   #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_AWB                 CCI_REG8(0x5001)
>> +#define OV5647_REG_ISPCTRL3D           CCI_REG8(0x503d)
>>
>>   #define REG_TERM 0xfffe
>>   #define VAL_TERM 0xfe
>> @@ -81,23 +78,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 +123,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},
>> @@ -230,8 +223,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
>>      {0x3503, 0x03},
>>      {0x0100, 0x01},
>>   };
>> -
> Please keep one blank line here. do not remove.
I will fix it in the next version.
>
>> -static struct regval_list ov5647_1080p30_10bpp[] = {
>> +static const struct reg_sequence ov5647_1080p30_10bpp[] = {
>>      {0x0100, 0x00},
>>      {0x0103, 0x01},
>>      {0x3034, 0x1a},
>> @@ -320,7 +312,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 +405,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 +586,35 @@ 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;
>> +   struct ov5647 *sensor = to_sensor(sd);
>> +   u64 channel_id;
>>      int ret;
>>
>> -   ret = ov5647_read(sd, OV5647_REG_MIPI_CTRL14, &channel_id);
>> +   ret = cci_read(sensor->regmap, OV5647_REG_MIPI_CTRL14, &channel_id, NULL);
>>      if (ret < 0)
>>            return ret;
>>
>>      channel_id &= ~(3 << 6);
>>
>> -   return ov5647_write(sd, OV5647_REG_MIPI_CTRL14,
>> -                   channel_id | (channel << 6));
>> +   return cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL14,
>> +                channel_id | (channel << 6), 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 +624,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;
> This feels wrong to me but anyway this is not related to your patch.

I'm sorry, I just noticed this. It's really suspicious. I hope someone can

answer this question. Until then, I'll remain unchanged.

>
>>      }
>> @@ -725,7 +643,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)
>>      struct i2c_client *client = v4l2_get_subdevdata(sd);
>>      struct ov5647 *sensor = to_sensor(sd);
>>      u8 val = MIPI_CTRL00_BUS_IDLE;
>> -   int ret;
>> +   int ret = 0;
> No need for zero initialization.
I will fix it in the next version.
>
>>      ret = ov5647_set_mode(sd);
>>      if (ret) {
>> @@ -742,32 +660,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;
>> +   struct ov5647 *sensor = to_sensor(sd);
>> +   int ret = 0;
>>
>> -   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;
>> +   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_write(sd, OV5647_REG_FRAME_OFF_NUMBER, 0x0f);
>> -   if (ret < 0)
>> -         return ret;
>> -
>> -   return ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);
>> +   return ret;
>>   }
>>
>>   static int ov5647_power_on(struct device *dev)
>> @@ -788,8 +699,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 +726,23 @@ 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;
>> +   u64 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);
>> +   ret = cci_read(sensor->regmap, OV5647_SW_STANDBY, &rdval, NULL);
>>      if (ret < 0)
>>            dev_dbg(dev, "software standby failed\n");
>>
>>      rdval &= ~0x01;
>> -   ret = ov5647_write(&sensor->sd, OV5647_SW_STANDBY, rdval);
>> +   ret = cci_write(sensor->regmap, OV5647_SW_STANDBY, rdval, NULL);
>>      if (ret < 0)
>>            dev_dbg(dev, "software standby failed\n");
>>
>> @@ -845,10 +756,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 +773,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 +1003,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);
>> -   if (ret < 0)
>> -         return ret;
>> -
>> -   if (read != 0x56) {
>> -         dev_err(&client->dev, "ID High expected 0x56 got %x", read);
>> -         return -ENODEV;
>> -   }
>> -
>> -   ret = ov5647_read(sd, OV5647_REG_CHIPID_L, &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 != 0x47) {
>> -         dev_err(&client->dev, "ID Low expected 0x47 got %x", read);
>> +   if (read != 0x5647) {
> We should define a macro for the chip ID and use it here.
OK，I will define a chip ID.
>
>> +         dev_err(&client->dev, "Chip ID expected 0x5647 got 0x%llx", 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)
>> @@ -1140,70 +1048,62 @@ static const struct v4l2_subdev_internal_ops ov5647_subdev_internal_ops = {
>>
>>   static int ov5647_s_auto_white_balance(struct v4l2_subdev *sd, u32 val)
>>   {
>> -   return ov5647_write(sd, OV5647_REG_AWB, val ? 1 : 0);
>> +   struct ov5647 *sensor = to_sensor(sd);
>> +
>> +   return cci_write(sensor->regmap, OV5647_REG_AWB, val ? 1 : 0, NULL);
>>   }
>>
>>   static int ov5647_s_autogain(struct v4l2_subdev *sd, u32 val)
>>   {
>> +   struct ov5647 *sensor = to_sensor(sd);
>>      int ret;
>> -   u8 reg;
>> +   u64 reg;
>>
>>      /* Non-zero turns on AGC by clearing bit 1.*/
>> -   ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
>> +   ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, &reg, NULL);
>>      if (ret)
>>            return ret;
>>
>> -   return ov5647_write(sd, OV5647_REG_AEC_AGC, val ? reg & ~BIT(1)
>> -                                       : reg | BIT(1));
>> +   return cci_write(sensor->regmap, OV5647_REG_AEC_AGC, val ? reg & ~BIT(1)
>> +                                       : reg | BIT(1), NULL);
>>   }
>>
>>   static int ov5647_s_exposure_auto(struct v4l2_subdev *sd, u32 val)
>>   {
>> +   struct ov5647 *sensor = to_sensor(sd);
>>      int ret;
>> -   u8 reg;
>> +   u64 reg;
>>
>>      /*
>>       * Everything except V4L2_EXPOSURE_MANUAL turns on AEC by
>>       * clearing bit 0.
>>       */
>> -   ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
>> +   ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, &reg, NULL);
>>      if (ret)
>>            return ret;
>>
>> -   return ov5647_write(sd, OV5647_REG_AEC_AGC,
>> +   return cci_write(sensor->regmap, OV5647_REG_AEC_AGC,
>>                      val == V4L2_EXPOSURE_MANUAL ? reg | BIT(0)
>> -                                       : reg & ~BIT(0));
>> +                                       : reg & ~BIT(0), NULL);
>>   }
>>
>>   static int ov5647_s_analogue_gain(struct v4l2_subdev *sd, u32 val)
>>   {
>> -   int ret;
>> +   struct ov5647 *sensor = to_sensor(sd);
>>
>>      /* 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);
>> +   return cci_write(sensor->regmap, OV5647_REG_GAIN, val & 0x3ff, NULL);
>>   }
>>
>>   static int ov5647_s_exposure(struct v4l2_subdev *sd, u32 val)
>>   {
>> -   int ret;
>> +   struct ov5647 *sensor = to_sensor(sd);
>>
>>      /*
>>       * 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);
>> +   return cci_write(sensor->regmap, OV5647_REG_EXPOSURE, val << 4, NULL);
>>   }
> We can now drop all contorls functions (except ov5647_s_exposure_auto / ov5647_s_autogain).
> Since there is only a single register write, we can write the register directly
> in the set control(like vblank).

You are correct. I will be handling all set control operations (including

ov5647_s_exposure_auto / ov5647_s_autogain) because Hans has already

commented on using cci_update_bits() instead of read/write operations, and

these two functions are no longer needed.

Best Regards,
Xiaolei

>
>>   static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
>> @@ -1254,12 +1154,12 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
>>            ret = ov5647_s_exposure(sd, ctrl->val);
>>            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 +1335,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
> with above changes
>
> Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>
>
> Best Regards,
> Tarang

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

[johannes.goede]

Hi Xiaolei,

Thank you for the new version and thank you for
addressing my comments about the register lists.

A few more comments inline, sorry for not catching these
in my earlier review.

On 31-Dec-25 09:39, 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>
> ---

...

> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index e193fef4fced..cbcb760ba5cd 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c

...

>  static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
>  {
> -	u8 channel_id;
> +	struct ov5647 *sensor = to_sensor(sd);
> +	u64 channel_id;
>  	int ret;
>  
> -	ret = ov5647_read(sd, OV5647_REG_MIPI_CTRL14, &channel_id);
> +	ret = cci_read(sensor->regmap, OV5647_REG_MIPI_CTRL14, &channel_id, NULL);
>  	if (ret < 0)
>  		return ret;
>  
>  	channel_id &= ~(3 << 6);
>  
> -	return ov5647_write(sd, OV5647_REG_MIPI_CTRL14,
> -			    channel_id | (channel << 6));
> +	return cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL14,
> +			 channel_id | (channel << 6), NULL);
>  }

This can be replaced with:

At the top below:

#define OV5647_REG_MIPI_CTRL14		CCI_REG8(0x4814)

add:

#define OV5647_REG_MIPI_CTRL14_CHANNEL_MASK	GENMASK(7, 6)
#define OV5647_REG_MIPI_CTRL14_CHANNEL_SHIFT	6

And then replace ov5647_set_virtual_channel() with:

static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
{
	struct ov5647 *sensor = to_sensor(sd);

	return cci_update_bits(sensor->regmap, OV5647_REG_MIPI_CTRL14,
			       OV5647_REG_MIPI_CTRL14_CHANNEL_MASK,
			       channel << OV5647_REG_MIPI_CTRL14_CHANNEL_SHIFT,
			       NULL);
}

...

> @@ -815,23 +726,23 @@ 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;
> +	u64 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");

And here replace this read + write:

>  
>  	/* Enter software standby */
> -	ret = ov5647_read(&sensor->sd, OV5647_SW_STANDBY, &rdval);
> +	ret = cci_read(sensor->regmap, OV5647_SW_STANDBY, &rdval, NULL);
>  	if (ret < 0)
>  		dev_dbg(dev, "software standby failed\n");
>  
>  	rdval &= ~0x01;
> -	ret = ov5647_write(&sensor->sd, OV5647_SW_STANDBY, rdval);
> +	ret = cci_write(sensor->regmap, OV5647_SW_STANDBY, rdval, NULL);
>  	if (ret < 0)
>  		dev_dbg(dev, "software standby failed\n");

With:

	ret = cci_update_bits(sensor->regmap, OV5647_SW_STANDBY, 0x01, 0x00, NULL);
	if (ret < 0)
		dev_dbg(dev, "software standby failed\n");

...

>  static int ov5647_s_autogain(struct v4l2_subdev *sd, u32 val)
>  {
> +	struct ov5647 *sensor = to_sensor(sd);
>  	int ret;
> -	u8 reg;
> +	u64 reg;
>  
>  	/* Non-zero turns on AGC by clearing bit 1.*/
> -	ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
> +	ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, &reg, NULL);
>  	if (ret)
>  		return ret;
>  
> -	return ov5647_write(sd, OV5647_REG_AEC_AGC, val ? reg & ~BIT(1)
> -							: reg | BIT(1));
> +	return cci_write(sensor->regmap, OV5647_REG_AEC_AGC, val ? reg & ~BIT(1)
> +							: reg | BIT(1), NULL);
>  }

And this is another opportunity to use cci_update_bits():

	return cci_update_bits(sensor->regmap, OV5647_REG_AEC_AGC, BIT(1),
			       val ? 0 : BIT(1), NULL);


>  static int ov5647_s_exposure_auto(struct v4l2_subdev *sd, u32 val)
>  {
> +	struct ov5647 *sensor = to_sensor(sd);
>  	int ret;
> -	u8 reg;
> +	u64 reg;
>  
>  	/*
>  	 * Everything except V4L2_EXPOSURE_MANUAL turns on AEC by
>  	 * clearing bit 0.
>  	 */
> -	ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
> +	ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, &reg, NULL);
>  	if (ret)
>  		return ret;
>  
> -	return ov5647_write(sd, OV5647_REG_AEC_AGC,
> +	return cci_write(sensor->regmap, OV5647_REG_AEC_AGC,
>  			    val == V4L2_EXPOSURE_MANUAL ? reg | BIT(0)
> -							: reg & ~BIT(0));
> +							: reg & ~BIT(0), NULL);
>  }


Same here, please switch this to use cci_update_bits()

Regards,

Hans

[PATCH v3 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>
---
 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 cbcb760ba5cd..bc81f378436a 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -92,7 +92,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;
@@ -807,10 +806,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);
@@ -831,14 +830,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;
 }
@@ -886,7 +885,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,
@@ -898,7 +896,6 @@ static int ov5647_get_pad_fmt(struct v4l2_subdev *sd,
 	}
 
 	*fmt = *sensor_format;
-	mutex_unlock(&sensor->lock);
 
 	return 0;
 }
@@ -916,7 +913,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 {
@@ -945,7 +941,6 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
 					 exposure_def);
 	}
 	*fmt = mode->format;
-	mutex_unlock(&sensor->lock);
 
 	return 0;
 }
@@ -958,10 +953,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;
 	}
@@ -1114,9 +1107,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;
 
@@ -1316,13 +1306,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);
@@ -1350,9 +1338,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) {
+		dev_err(&client->dev, "failed to init subdev: %d", ret);
+		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);
@@ -1363,14 +1358,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;
 }
@@ -1381,11 +1376,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

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

[Tarang Raval]

Hi Xiaolei,

> 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>
> ---
>  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 cbcb760ba5cd..bc81f378436a 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -92,7 +92,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;
> @@ -807,10 +806,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);
> @@ -831,14 +830,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;
>  }
> @@ -886,7 +885,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,
> @@ -898,7 +896,6 @@ static int ov5647_get_pad_fmt(struct v4l2_subdev *sd,
>         }
> 
>         *fmt = *sensor_format;
> -       mutex_unlock(&sensor->lock);
> 
>         return 0;
>  }
> @@ -916,7 +913,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 {
> @@ -945,7 +941,6 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
>                                          exposure_def);
>         }
>         *fmt = mode->format;
> -       mutex_unlock(&sensor->lock);
> 
>         return 0;
>  }
> @@ -958,10 +953,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;
>         }
> @@ -1114,9 +1107,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;
> 
> @@ -1316,13 +1306,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);
> @@ -1350,9 +1338,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) {
> +               dev_err(&client->dev, "failed to init subdev: %d", ret);

Use dev_err_probe

> +               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);
> @@ -1363,14 +1358,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;
>  }
> @@ -1381,11 +1376,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

Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>
                                                          
Best Regards,                                             
Tarang

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

[xiaolei wang]

On 12/31/25 18:54, Tarang Raval 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,
>
>> 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>
>> ---
>>   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 cbcb760ba5cd..bc81f378436a 100644
>> --- a/drivers/media/i2c/ov5647.c
>> +++ b/drivers/media/i2c/ov5647.c
>> @@ -92,7 +92,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;
>> @@ -807,10 +806,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);
>> @@ -831,14 +830,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;
>>   }
>> @@ -886,7 +885,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,
>> @@ -898,7 +896,6 @@ static int ov5647_get_pad_fmt(struct v4l2_subdev *sd,
>>          }
>>   
>>          *fmt = *sensor_format;
>> -       mutex_unlock(&sensor->lock);
>>   
>>          return 0;
>>   }
>> @@ -916,7 +913,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 {
>> @@ -945,7 +941,6 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
>>                                           exposure_def);
>>          }
>>          *fmt = mode->format;
>> -       mutex_unlock(&sensor->lock);
>>   
>>          return 0;
>>   }
>> @@ -958,10 +953,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;
>>          }
>> @@ -1114,9 +1107,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;
>>   
>> @@ -1316,13 +1306,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);
>> @@ -1350,9 +1338,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) {
>> +               dev_err(&client->dev, "failed to init subdev: %d", ret);
> Use dev_err_probe

I will correct this in the next version.

Best Regards,
Xiaolei

>
>> +               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);
>> @@ -1363,14 +1358,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;
>>   }
>> @@ -1381,11 +1376,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
> Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>
>
> Best Regards,
> Tarang

[PATCH v3 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>
---
 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 bc81f378436a..7091081a0828 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -637,23 +637,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 = 0;
 
+	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 |
@@ -663,19 +682,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;
 }
@@ -706,7 +730,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;
@@ -803,47 +827,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,
@@ -986,6 +971,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 v3 3/3] media: i2c: ov5647: switch to {enable,disable}_streams

[Tarang Raval]

Hi Xiaolei,

> 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>
> ---
>  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 bc81f378436a..7091081a0828 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -637,23 +637,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 = 0;

No need for zero initialization.
  
> +   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 |
> @@ -663,19 +682,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;
>  }
> @@ -706,7 +730,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;
> @@ -803,47 +827,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,
> @@ -986,6 +971,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

Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>
                                                          
Best Regards,                                             
Tarang

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

[xiaolei wang]

On 12/31/25 19:03, Tarang Raval 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,
>
>> 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>
>> ---
>>   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 bc81f378436a..7091081a0828 100644
>> --- a/drivers/media/i2c/ov5647.c
>> +++ b/drivers/media/i2c/ov5647.c
>> @@ -637,23 +637,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 = 0;
> No need for zero initialization.

I will correct this in the next version.

Best Regards,
Xiaolei

>
>> +   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 |
>> @@ -663,19 +682,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;
>>   }
>> @@ -706,7 +730,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;
>> @@ -803,47 +827,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,
>> @@ -986,6 +971,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
> Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>
>
> Best Regards,
> Tarang