* [PATCH v3 0/3] media: i2c: ov5647: Modernize driver with CCI and new stream APIs
@ 2025-12-31 8:39 Xiaolei Wang
2025-12-31 8:39 ` [PATCH v3 1/3] media: i2c: ov5647: Convert to CCI register access helpers Xiaolei Wang
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Xiaolei Wang @ 2025-12-31 8:39 UTC (permalink / raw)
To: tarang.raval, laurent.pinchart, sakari.ailus, dave.stevenson,
jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco,
johannes.goede, hverkuil-cisco, jai.luthra, xiaolei.wang
Cc: linux-media, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/3] media: i2c: ov5647: Convert to CCI register access helpers
2025-12-31 8:39 [PATCH v3 0/3] media: i2c: ov5647: Modernize driver with CCI and new stream APIs Xiaolei Wang
@ 2025-12-31 8:39 ` Xiaolei Wang
2025-12-31 10:26 ` Tarang Raval
2025-12-31 12:12 ` johannes.goede
2025-12-31 8:39 ` [PATCH v3 2/3] media: i2c: ov5647: Switch to using the sub-device state lock Xiaolei Wang
2025-12-31 8:39 ` [PATCH v3 3/3] media: i2c: ov5647: switch to {enable,disable}_streams Xiaolei Wang
2 siblings, 2 replies; 11+ messages in thread
From: Xiaolei Wang @ 2025-12-31 8:39 UTC (permalink / raw)
To: tarang.raval, laurent.pinchart, sakari.ailus, dave.stevenson,
jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco,
johannes.goede, hverkuil-cisco, jai.luthra, xiaolei.wang
Cc: linux-media, linux-kernel
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, ®);
+ ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, ®, 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, ®);
+ ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, ®, 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 2/3] media: i2c: ov5647: Switch to using the sub-device state lock
2025-12-31 8:39 [PATCH v3 0/3] media: i2c: ov5647: Modernize driver with CCI and new stream APIs Xiaolei Wang
2025-12-31 8:39 ` [PATCH v3 1/3] media: i2c: ov5647: Convert to CCI register access helpers Xiaolei Wang
@ 2025-12-31 8:39 ` Xiaolei Wang
2025-12-31 10:54 ` Tarang Raval
2025-12-31 8:39 ` [PATCH v3 3/3] media: i2c: ov5647: switch to {enable,disable}_streams Xiaolei Wang
2 siblings, 1 reply; 11+ messages in thread
From: Xiaolei Wang @ 2025-12-31 8:39 UTC (permalink / raw)
To: tarang.raval, laurent.pinchart, sakari.ailus, dave.stevenson,
jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco,
johannes.goede, hverkuil-cisco, jai.luthra, xiaolei.wang
Cc: linux-media, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 3/3] media: i2c: ov5647: switch to {enable,disable}_streams
2025-12-31 8:39 [PATCH v3 0/3] media: i2c: ov5647: Modernize driver with CCI and new stream APIs Xiaolei Wang
2025-12-31 8:39 ` [PATCH v3 1/3] media: i2c: ov5647: Convert to CCI register access helpers Xiaolei Wang
2025-12-31 8:39 ` [PATCH v3 2/3] media: i2c: ov5647: Switch to using the sub-device state lock Xiaolei Wang
@ 2025-12-31 8:39 ` Xiaolei Wang
2025-12-31 11:03 ` Tarang Raval
2 siblings, 1 reply; 11+ messages in thread
From: Xiaolei Wang @ 2025-12-31 8:39 UTC (permalink / raw)
To: tarang.raval, laurent.pinchart, sakari.ailus, dave.stevenson,
jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco,
johannes.goede, hverkuil-cisco, jai.luthra, xiaolei.wang
Cc: linux-media, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] media: i2c: ov5647: Convert to CCI register access helpers
2025-12-31 8:39 ` [PATCH v3 1/3] media: i2c: ov5647: Convert to CCI register access helpers Xiaolei Wang
@ 2025-12-31 10:26 ` Tarang Raval
2026-01-01 1:31 ` xiaolei wang
2025-12-31 12:12 ` johannes.goede
1 sibling, 1 reply; 11+ messages in thread
From: Tarang Raval @ 2025-12-31 10:26 UTC (permalink / raw)
To: Xiaolei Wang, laurent.pinchart, sakari.ailus, dave.stevenson,
jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco,
johannes.goede, hverkuil-cisco, jai.luthra
Cc: linux-media, linux-kernel
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, ®);
> + ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, ®, 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, ®);
> + ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, ®, 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/3] media: i2c: ov5647: Switch to using the sub-device state lock
2025-12-31 8:39 ` [PATCH v3 2/3] media: i2c: ov5647: Switch to using the sub-device state lock Xiaolei Wang
@ 2025-12-31 10:54 ` Tarang Raval
2026-01-01 2:03 ` xiaolei wang
0 siblings, 1 reply; 11+ messages in thread
From: Tarang Raval @ 2025-12-31 10:54 UTC (permalink / raw)
To: Xiaolei Wang, laurent.pinchart, sakari.ailus, dave.stevenson,
jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco,
johannes.goede, hverkuil-cisco, jai.luthra
Cc: linux-media, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/3] media: i2c: ov5647: switch to {enable,disable}_streams
2025-12-31 8:39 ` [PATCH v3 3/3] media: i2c: ov5647: switch to {enable,disable}_streams Xiaolei Wang
@ 2025-12-31 11:03 ` Tarang Raval
2026-01-01 2:04 ` xiaolei wang
0 siblings, 1 reply; 11+ messages in thread
From: Tarang Raval @ 2025-12-31 11:03 UTC (permalink / raw)
To: Xiaolei Wang, laurent.pinchart, sakari.ailus, dave.stevenson,
jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco,
johannes.goede, hverkuil-cisco, jai.luthra
Cc: linux-media, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] media: i2c: ov5647: Convert to CCI register access helpers
2025-12-31 8:39 ` [PATCH v3 1/3] media: i2c: ov5647: Convert to CCI register access helpers Xiaolei Wang
2025-12-31 10:26 ` Tarang Raval
@ 2025-12-31 12:12 ` johannes.goede
1 sibling, 0 replies; 11+ messages in thread
From: johannes.goede @ 2025-12-31 12:12 UTC (permalink / raw)
To: Xiaolei Wang, tarang.raval, laurent.pinchart, sakari.ailus,
dave.stevenson, jacopo, mchehab, prabhakar.mahadev-lad.rj,
hverkuil+cisco, hverkuil-cisco, jai.luthra
Cc: linux-media, linux-kernel
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, ®);
> + ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, ®, 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, ®);
> + ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, ®, 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] media: i2c: ov5647: Convert to CCI register access helpers
2025-12-31 10:26 ` Tarang Raval
@ 2026-01-01 1:31 ` xiaolei wang
0 siblings, 0 replies; 11+ messages in thread
From: xiaolei wang @ 2026-01-01 1:31 UTC (permalink / raw)
To: Tarang Raval, laurent.pinchart, sakari.ailus, dave.stevenson,
jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco,
johannes.goede, hverkuil-cisco, jai.luthra
Cc: linux-media, linux-kernel
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, ®);
>> + ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, ®, 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, ®);
>> + ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, ®, 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/3] media: i2c: ov5647: Switch to using the sub-device state lock
2025-12-31 10:54 ` Tarang Raval
@ 2026-01-01 2:03 ` xiaolei wang
0 siblings, 0 replies; 11+ messages in thread
From: xiaolei wang @ 2026-01-01 2:03 UTC (permalink / raw)
To: Tarang Raval, laurent.pinchart, sakari.ailus, dave.stevenson,
jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco,
johannes.goede, hverkuil-cisco, jai.luthra
Cc: linux-media, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/3] media: i2c: ov5647: switch to {enable,disable}_streams
2025-12-31 11:03 ` Tarang Raval
@ 2026-01-01 2:04 ` xiaolei wang
0 siblings, 0 replies; 11+ messages in thread
From: xiaolei wang @ 2026-01-01 2:04 UTC (permalink / raw)
To: Tarang Raval, laurent.pinchart, sakari.ailus, dave.stevenson,
jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco,
johannes.goede, hverkuil-cisco, jai.luthra
Cc: linux-media, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2026-01-01 2:04 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-12-31 8:39 [PATCH v3 0/3] media: i2c: ov5647: Modernize driver with CCI and new stream APIs Xiaolei Wang
2025-12-31 8:39 ` [PATCH v3 1/3] media: i2c: ov5647: Convert to CCI register access helpers Xiaolei Wang
2025-12-31 10:26 ` Tarang Raval
2026-01-01 1:31 ` xiaolei wang
2025-12-31 12:12 ` johannes.goede
2025-12-31 8:39 ` [PATCH v3 2/3] media: i2c: ov5647: Switch to using the sub-device state lock Xiaolei Wang
2025-12-31 10:54 ` Tarang Raval
2026-01-01 2:03 ` xiaolei wang
2025-12-31 8:39 ` [PATCH v3 3/3] media: i2c: ov5647: switch to {enable,disable}_streams Xiaolei Wang
2025-12-31 11:03 ` Tarang Raval
2026-01-01 2:04 ` xiaolei wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®