* [PATCH v2 0/3] media: i2c: ov5647: Modernize driver with CCI and new stream APIs
@ 2025-12-29 2:30 Xiaolei Wang
2025-12-29 2:30 ` [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers Xiaolei Wang
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Xiaolei Wang @ 2025-12-29 2:30 UTC (permalink / raw)
To: 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.
These changes reduce code complexity, improve maintainability, and
ensure the driver follows modern V4L2 subsystem patterns.
Changes in V2:
- 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 | 1098 ++++++++++++++++--------------------
2 files changed, 492 insertions(+), 607 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers 2025-12-29 2:30 [PATCH v2 0/3] media: i2c: ov5647: Modernize driver with CCI and new stream APIs Xiaolei Wang @ 2025-12-29 2:30 ` Xiaolei Wang 2025-12-29 12:37 ` Tarang Raval ` (2 more replies) 2025-12-29 2:30 ` [PATCH v2 2/3] media: i2c: ov5647: Switch to using the sub-device state lock Xiaolei Wang 2025-12-29 2:30 ` [PATCH v2 3/3] media: i2c: ov5647: switch to {enable,disable}_streams Xiaolei Wang 2 siblings, 3 replies; 14+ messages in thread From: Xiaolei Wang @ 2025-12-29 2:30 UTC (permalink / raw) To: 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 | 997 +++++++++++++++++-------------------- 2 files changed, 453 insertions(+), 545 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..fd69f1616794 100644 --- a/drivers/media/i2c/ov5647.c +++ b/drivers/media/i2c/ov5647.c @@ -22,6 +22,7 @@ #include <linux/pm_runtime.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 +42,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(0x350b) +#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 +77,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 cci_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,377 +122,377 @@ static const u8 ov5647_test_pattern_val[] = { 0x81, /* Random Data */ }; -static const struct regval_list sensor_oe_disable_regs[] = { - {0x3000, 0x00}, - {0x3001, 0x00}, - {0x3002, 0x00}, +static const struct cci_reg_sequence sensor_oe_disable_regs[] = { + { CCI_REG8(0x3000), 0x00 }, + { CCI_REG8(0x3001), 0x00 }, + { CCI_REG8(0x3002), 0x00 }, }; -static const struct regval_list sensor_oe_enable_regs[] = { - {0x3000, 0x0f}, - {0x3001, 0xff}, - {0x3002, 0xe4}, +static const struct cci_reg_sequence sensor_oe_enable_regs[] = { + { CCI_REG8(0x3000), 0x0f }, + { CCI_REG8(0x3001), 0xff }, + { CCI_REG8(0x3002), 0xe4 }, }; -static struct regval_list ov5647_2592x1944_10bpp[] = { - {0x0100, 0x00}, - {0x0103, 0x01}, - {0x3034, 0x1a}, - {0x3035, 0x21}, - {0x3036, 0x69}, - {0x303c, 0x11}, - {0x3106, 0xf5}, - {0x3821, 0x06}, - {0x3820, 0x00}, - {0x3827, 0xec}, - {0x370c, 0x03}, - {0x3612, 0x5b}, - {0x3618, 0x04}, - {0x5000, 0x06}, - {0x5002, 0x41}, - {0x5003, 0x08}, - {0x5a00, 0x08}, - {0x3000, 0x00}, - {0x3001, 0x00}, - {0x3002, 0x00}, - {0x3016, 0x08}, - {0x3017, 0xe0}, - {0x3018, 0x44}, - {0x301c, 0xf8}, - {0x301d, 0xf0}, - {0x3a18, 0x00}, - {0x3a19, 0xf8}, - {0x3c01, 0x80}, - {0x3b07, 0x0c}, - {0x380c, 0x0b}, - {0x380d, 0x1c}, - {0x3814, 0x11}, - {0x3815, 0x11}, - {0x3708, 0x64}, - {0x3709, 0x12}, - {0x3808, 0x0a}, - {0x3809, 0x20}, - {0x380a, 0x07}, - {0x380b, 0x98}, - {0x3800, 0x00}, - {0x3801, 0x00}, - {0x3802, 0x00}, - {0x3803, 0x00}, - {0x3804, 0x0a}, - {0x3805, 0x3f}, - {0x3806, 0x07}, - {0x3807, 0xa3}, - {0x3811, 0x10}, - {0x3813, 0x06}, - {0x3630, 0x2e}, - {0x3632, 0xe2}, - {0x3633, 0x23}, - {0x3634, 0x44}, - {0x3636, 0x06}, - {0x3620, 0x64}, - {0x3621, 0xe0}, - {0x3600, 0x37}, - {0x3704, 0xa0}, - {0x3703, 0x5a}, - {0x3715, 0x78}, - {0x3717, 0x01}, - {0x3731, 0x02}, - {0x370b, 0x60}, - {0x3705, 0x1a}, - {0x3f05, 0x02}, - {0x3f06, 0x10}, - {0x3f01, 0x0a}, - {0x3a08, 0x01}, - {0x3a09, 0x28}, - {0x3a0a, 0x00}, - {0x3a0b, 0xf6}, - {0x3a0d, 0x08}, - {0x3a0e, 0x06}, - {0x3a0f, 0x58}, - {0x3a10, 0x50}, - {0x3a1b, 0x58}, - {0x3a1e, 0x50}, - {0x3a11, 0x60}, - {0x3a1f, 0x28}, - {0x4001, 0x02}, - {0x4004, 0x04}, - {0x4000, 0x09}, - {0x4837, 0x19}, - {0x4800, 0x24}, - {0x3503, 0x03}, - {0x0100, 0x01}, +static const struct cci_reg_sequence ov5647_2592x1944_10bpp[] = { + { CCI_REG8(0x0100), 0x00 }, + { CCI_REG8(0x0103), 0x01 }, + { CCI_REG8(0x3034), 0x1a }, + { CCI_REG8(0x3035), 0x21 }, + { CCI_REG8(0x3036), 0x69 }, + { CCI_REG8(0x303c), 0x11 }, + { CCI_REG8(0x3106), 0xf5 }, + { CCI_REG8(0x3821), 0x06 }, + { CCI_REG8(0x3820), 0x00 }, + { CCI_REG8(0x3827), 0xec }, + { CCI_REG8(0x370c), 0x03 }, + { CCI_REG8(0x3612), 0x5b }, + { CCI_REG8(0x3618), 0x04 }, + { CCI_REG8(0x5000), 0x06 }, + { CCI_REG8(0x5002), 0x41 }, + { CCI_REG8(0x5003), 0x08 }, + { CCI_REG8(0x5a00), 0x08 }, + { CCI_REG8(0x3000), 0x00 }, + { CCI_REG8(0x3001), 0x00 }, + { CCI_REG8(0x3002), 0x00 }, + { CCI_REG8(0x3016), 0x08 }, + { CCI_REG8(0x3017), 0xe0 }, + { CCI_REG8(0x3018), 0x44 }, + { CCI_REG8(0x301c), 0xf8 }, + { CCI_REG8(0x301d), 0xf0 }, + { CCI_REG8(0x3a18), 0x00 }, + { CCI_REG8(0x3a19), 0xf8 }, + { CCI_REG8(0x3c01), 0x80 }, + { CCI_REG8(0x3b07), 0x0c }, + { CCI_REG8(0x380c), 0x0b }, + { CCI_REG8(0x380d), 0x1c }, + { CCI_REG8(0x3814), 0x11 }, + { CCI_REG8(0x3815), 0x11 }, + { CCI_REG8(0x3708), 0x64 }, + { CCI_REG8(0x3709), 0x12 }, + { CCI_REG8(0x3808), 0x0a }, + { CCI_REG8(0x3809), 0x20 }, + { CCI_REG8(0x380a), 0x07 }, + { CCI_REG8(0x380b), 0x98 }, + { CCI_REG8(0x3800), 0x00 }, + { CCI_REG8(0x3801), 0x00 }, + { CCI_REG8(0x3802), 0x00 }, + { CCI_REG8(0x3803), 0x00 }, + { CCI_REG8(0x3804), 0x0a }, + { CCI_REG8(0x3805), 0x3f }, + { CCI_REG8(0x3806), 0x07 }, + { CCI_REG8(0x3807), 0xa3 }, + { CCI_REG8(0x3811), 0x10 }, + { CCI_REG8(0x3813), 0x06 }, + { CCI_REG8(0x3630), 0x2e }, + { CCI_REG8(0x3632), 0xe2 }, + { CCI_REG8(0x3633), 0x23 }, + { CCI_REG8(0x3634), 0x44 }, + { CCI_REG8(0x3636), 0x06 }, + { CCI_REG8(0x3620), 0x64 }, + { CCI_REG8(0x3621), 0xe0 }, + { CCI_REG8(0x3600), 0x37 }, + { CCI_REG8(0x3704), 0xa0 }, + { CCI_REG8(0x3703), 0x5a }, + { CCI_REG8(0x3715), 0x78 }, + { CCI_REG8(0x3717), 0x01 }, + { CCI_REG8(0x3731), 0x02 }, + { CCI_REG8(0x370b), 0x60 }, + { CCI_REG8(0x3705), 0x1a }, + { CCI_REG8(0x3f05), 0x02 }, + { CCI_REG8(0x3f06), 0x10 }, + { CCI_REG8(0x3f01), 0x0a }, + { CCI_REG8(0x3a08), 0x01 }, + { CCI_REG8(0x3a09), 0x28 }, + { CCI_REG8(0x3a0a), 0x00 }, + { CCI_REG8(0x3a0b), 0xf6 }, + { CCI_REG8(0x3a0d), 0x08 }, + { CCI_REG8(0x3a0e), 0x06 }, + { CCI_REG8(0x3a0f), 0x58 }, + { CCI_REG8(0x3a10), 0x50 }, + { CCI_REG8(0x3a1b), 0x58 }, + { CCI_REG8(0x3a1e), 0x50 }, + { CCI_REG8(0x3a11), 0x60 }, + { CCI_REG8(0x3a1f), 0x28 }, + { CCI_REG8(0x4001), 0x02 }, + { CCI_REG8(0x4004), 0x04 }, + { CCI_REG8(0x4000), 0x09 }, + { CCI_REG8(0x4837), 0x19 }, + { CCI_REG8(0x4800), 0x24 }, + { CCI_REG8(0x3503), 0x03 }, + { CCI_REG8(0x0100), 0x01 }, }; -static struct regval_list ov5647_1080p30_10bpp[] = { - {0x0100, 0x00}, - {0x0103, 0x01}, - {0x3034, 0x1a}, - {0x3035, 0x21}, - {0x3036, 0x62}, - {0x303c, 0x11}, - {0x3106, 0xf5}, - {0x3821, 0x06}, - {0x3820, 0x00}, - {0x3827, 0xec}, - {0x370c, 0x03}, - {0x3612, 0x5b}, - {0x3618, 0x04}, - {0x5000, 0x06}, - {0x5002, 0x41}, - {0x5003, 0x08}, - {0x5a00, 0x08}, - {0x3000, 0x00}, - {0x3001, 0x00}, - {0x3002, 0x00}, - {0x3016, 0x08}, - {0x3017, 0xe0}, - {0x3018, 0x44}, - {0x301c, 0xf8}, - {0x301d, 0xf0}, - {0x3a18, 0x00}, - {0x3a19, 0xf8}, - {0x3c01, 0x80}, - {0x3b07, 0x0c}, - {0x380c, 0x09}, - {0x380d, 0x70}, - {0x3814, 0x11}, - {0x3815, 0x11}, - {0x3708, 0x64}, - {0x3709, 0x12}, - {0x3808, 0x07}, - {0x3809, 0x80}, - {0x380a, 0x04}, - {0x380b, 0x38}, - {0x3800, 0x01}, - {0x3801, 0x5c}, - {0x3802, 0x01}, - {0x3803, 0xb2}, - {0x3804, 0x08}, - {0x3805, 0xe3}, - {0x3806, 0x05}, - {0x3807, 0xf1}, - {0x3811, 0x04}, - {0x3813, 0x02}, - {0x3630, 0x2e}, - {0x3632, 0xe2}, - {0x3633, 0x23}, - {0x3634, 0x44}, - {0x3636, 0x06}, - {0x3620, 0x64}, - {0x3621, 0xe0}, - {0x3600, 0x37}, - {0x3704, 0xa0}, - {0x3703, 0x5a}, - {0x3715, 0x78}, - {0x3717, 0x01}, - {0x3731, 0x02}, - {0x370b, 0x60}, - {0x3705, 0x1a}, - {0x3f05, 0x02}, - {0x3f06, 0x10}, - {0x3f01, 0x0a}, - {0x3a08, 0x01}, - {0x3a09, 0x4b}, - {0x3a0a, 0x01}, - {0x3a0b, 0x13}, - {0x3a0d, 0x04}, - {0x3a0e, 0x03}, - {0x3a0f, 0x58}, - {0x3a10, 0x50}, - {0x3a1b, 0x58}, - {0x3a1e, 0x50}, - {0x3a11, 0x60}, - {0x3a1f, 0x28}, - {0x4001, 0x02}, - {0x4004, 0x04}, - {0x4000, 0x09}, - {0x4837, 0x19}, - {0x4800, 0x34}, - {0x3503, 0x03}, - {0x0100, 0x01}, +static const struct cci_reg_sequence ov5647_1080p30_10bpp[] = { + { CCI_REG8(0x0100), 0x00 }, + { CCI_REG8(0x0103), 0x01 }, + { CCI_REG8(0x3034), 0x1a }, + { CCI_REG8(0x3035), 0x21 }, + { CCI_REG8(0x3036), 0x62 }, + { CCI_REG8(0x303c), 0x11 }, + { CCI_REG8(0x3106), 0xf5 }, + { CCI_REG8(0x3821), 0x06 }, + { CCI_REG8(0x3820), 0x00 }, + { CCI_REG8(0x3827), 0xec }, + { CCI_REG8(0x370c), 0x03 }, + { CCI_REG8(0x3612), 0x5b }, + { CCI_REG8(0x3618), 0x04 }, + { CCI_REG8(0x5000), 0x06 }, + { CCI_REG8(0x5002), 0x41 }, + { CCI_REG8(0x5003), 0x08 }, + { CCI_REG8(0x5a00), 0x08 }, + { CCI_REG8(0x3000), 0x00 }, + { CCI_REG8(0x3001), 0x00 }, + { CCI_REG8(0x3002), 0x00 }, + { CCI_REG8(0x3016), 0x08 }, + { CCI_REG8(0x3017), 0xe0 }, + { CCI_REG8(0x3018), 0x44 }, + { CCI_REG8(0x301c), 0xf8 }, + { CCI_REG8(0x301d), 0xf0 }, + { CCI_REG8(0x3a18), 0x00 }, + { CCI_REG8(0x3a19), 0xf8 }, + { CCI_REG8(0x3c01), 0x80 }, + { CCI_REG8(0x3b07), 0x0c }, + { CCI_REG8(0x380c), 0x09 }, + { CCI_REG8(0x380d), 0x70 }, + { CCI_REG8(0x3814), 0x11 }, + { CCI_REG8(0x3815), 0x11 }, + { CCI_REG8(0x3708), 0x64 }, + { CCI_REG8(0x3709), 0x12 }, + { CCI_REG8(0x3808), 0x07 }, + { CCI_REG8(0x3809), 0x80 }, + { CCI_REG8(0x380a), 0x04 }, + { CCI_REG8(0x380b), 0x38 }, + { CCI_REG8(0x3800), 0x01 }, + { CCI_REG8(0x3801), 0x5c }, + { CCI_REG8(0x3802), 0x01 }, + { CCI_REG8(0x3803), 0xb2 }, + { CCI_REG8(0x3804), 0x08 }, + { CCI_REG8(0x3805), 0xe3 }, + { CCI_REG8(0x3806), 0x05 }, + { CCI_REG8(0x3807), 0xf1 }, + { CCI_REG8(0x3811), 0x04 }, + { CCI_REG8(0x3813), 0x02 }, + { CCI_REG8(0x3630), 0x2e }, + { CCI_REG8(0x3632), 0xe2 }, + { CCI_REG8(0x3633), 0x23 }, + { CCI_REG8(0x3634), 0x44 }, + { CCI_REG8(0x3636), 0x06 }, + { CCI_REG8(0x3620), 0x64 }, + { CCI_REG8(0x3621), 0xe0 }, + { CCI_REG8(0x3600), 0x37 }, + { CCI_REG8(0x3704), 0xa0 }, + { CCI_REG8(0x3703), 0x5a }, + { CCI_REG8(0x3715), 0x78 }, + { CCI_REG8(0x3717), 0x01 }, + { CCI_REG8(0x3731), 0x02 }, + { CCI_REG8(0x370b), 0x60 }, + { CCI_REG8(0x3705), 0x1a }, + { CCI_REG8(0x3f05), 0x02 }, + { CCI_REG8(0x3f06), 0x10 }, + { CCI_REG8(0x3f01), 0x0a }, + { CCI_REG8(0x3a08), 0x01 }, + { CCI_REG8(0x3a09), 0x4b }, + { CCI_REG8(0x3a0a), 0x01 }, + { CCI_REG8(0x3a0b), 0x13 }, + { CCI_REG8(0x3a0d), 0x04 }, + { CCI_REG8(0x3a0e), 0x03 }, + { CCI_REG8(0x3a0f), 0x58 }, + { CCI_REG8(0x3a10), 0x50 }, + { CCI_REG8(0x3a1b), 0x58 }, + { CCI_REG8(0x3a1e), 0x50 }, + { CCI_REG8(0x3a11), 0x60 }, + { CCI_REG8(0x3a1f), 0x28 }, + { CCI_REG8(0x4001), 0x02 }, + { CCI_REG8(0x4004), 0x04 }, + { CCI_REG8(0x4000), 0x09 }, + { CCI_REG8(0x4837), 0x19 }, + { CCI_REG8(0x4800), 0x34 }, + { CCI_REG8(0x3503), 0x03 }, + { CCI_REG8(0x0100), 0x01 }, }; -static struct regval_list ov5647_2x2binned_10bpp[] = { - {0x0100, 0x00}, - {0x0103, 0x01}, - {0x3034, 0x1a}, - {0x3035, 0x21}, - {0x3036, 0x62}, - {0x303c, 0x11}, - {0x3106, 0xf5}, - {0x3827, 0xec}, - {0x370c, 0x03}, - {0x3612, 0x59}, - {0x3618, 0x00}, - {0x5000, 0x06}, - {0x5002, 0x41}, - {0x5003, 0x08}, - {0x5a00, 0x08}, - {0x3000, 0x00}, - {0x3001, 0x00}, - {0x3002, 0x00}, - {0x3016, 0x08}, - {0x3017, 0xe0}, - {0x3018, 0x44}, - {0x301c, 0xf8}, - {0x301d, 0xf0}, - {0x3a18, 0x00}, - {0x3a19, 0xf8}, - {0x3c01, 0x80}, - {0x3b07, 0x0c}, - {0x3800, 0x00}, - {0x3801, 0x00}, - {0x3802, 0x00}, - {0x3803, 0x00}, - {0x3804, 0x0a}, - {0x3805, 0x3f}, - {0x3806, 0x07}, - {0x3807, 0xa3}, - {0x3808, 0x05}, - {0x3809, 0x10}, - {0x380a, 0x03}, - {0x380b, 0xcc}, - {0x380c, 0x07}, - {0x380d, 0x68}, - {0x3811, 0x0c}, - {0x3813, 0x06}, - {0x3814, 0x31}, - {0x3815, 0x31}, - {0x3630, 0x2e}, - {0x3632, 0xe2}, - {0x3633, 0x23}, - {0x3634, 0x44}, - {0x3636, 0x06}, - {0x3620, 0x64}, - {0x3621, 0xe0}, - {0x3600, 0x37}, - {0x3704, 0xa0}, - {0x3703, 0x5a}, - {0x3715, 0x78}, - {0x3717, 0x01}, - {0x3731, 0x02}, - {0x370b, 0x60}, - {0x3705, 0x1a}, - {0x3f05, 0x02}, - {0x3f06, 0x10}, - {0x3f01, 0x0a}, - {0x3a08, 0x01}, - {0x3a09, 0x28}, - {0x3a0a, 0x00}, - {0x3a0b, 0xf6}, - {0x3a0d, 0x08}, - {0x3a0e, 0x06}, - {0x3a0f, 0x58}, - {0x3a10, 0x50}, - {0x3a1b, 0x58}, - {0x3a1e, 0x50}, - {0x3a11, 0x60}, - {0x3a1f, 0x28}, - {0x4001, 0x02}, - {0x4004, 0x04}, - {0x4000, 0x09}, - {0x4837, 0x16}, - {0x4800, 0x24}, - {0x3503, 0x03}, - {0x3820, 0x41}, - {0x3821, 0x07}, - {0x350a, 0x00}, - {0x350b, 0x10}, - {0x3500, 0x00}, - {0x3501, 0x1a}, - {0x3502, 0xf0}, - {0x3212, 0xa0}, - {0x0100, 0x01}, +static const struct cci_reg_sequence ov5647_2x2binned_10bpp[] = { + { CCI_REG8(0x0100), 0x00 }, + { CCI_REG8(0x0103), 0x01 }, + { CCI_REG8(0x3034), 0x1a }, + { CCI_REG8(0x3035), 0x21 }, + { CCI_REG8(0x3036), 0x62 }, + { CCI_REG8(0x303c), 0x11 }, + { CCI_REG8(0x3106), 0xf5 }, + { CCI_REG8(0x3827), 0xec }, + { CCI_REG8(0x370c), 0x03 }, + { CCI_REG8(0x3612), 0x59 }, + { CCI_REG8(0x3618), 0x00 }, + { CCI_REG8(0x5000), 0x06 }, + { CCI_REG8(0x5002), 0x41 }, + { CCI_REG8(0x5003), 0x08 }, + { CCI_REG8(0x5a00), 0x08 }, + { CCI_REG8(0x3000), 0x00 }, + { CCI_REG8(0x3001), 0x00 }, + { CCI_REG8(0x3002), 0x00 }, + { CCI_REG8(0x3016), 0x08 }, + { CCI_REG8(0x3017), 0xe0 }, + { CCI_REG8(0x3018), 0x44 }, + { CCI_REG8(0x301c), 0xf8 }, + { CCI_REG8(0x301d), 0xf0 }, + { CCI_REG8(0x3a18), 0x00 }, + { CCI_REG8(0x3a19), 0xf8 }, + { CCI_REG8(0x3c01), 0x80 }, + { CCI_REG8(0x3b07), 0x0c }, + { CCI_REG8(0x3800), 0x00 }, + { CCI_REG8(0x3801), 0x00 }, + { CCI_REG8(0x3802), 0x00 }, + { CCI_REG8(0x3803), 0x00 }, + { CCI_REG8(0x3804), 0x0a }, + { CCI_REG8(0x3805), 0x3f }, + { CCI_REG8(0x3806), 0x07 }, + { CCI_REG8(0x3807), 0xa3 }, + { CCI_REG8(0x3808), 0x05 }, + { CCI_REG8(0x3809), 0x10 }, + { CCI_REG8(0x380a), 0x03 }, + { CCI_REG8(0x380b), 0xcc }, + { CCI_REG8(0x380c), 0x07 }, + { CCI_REG8(0x380d), 0x68 }, + { CCI_REG8(0x3811), 0x0c }, + { CCI_REG8(0x3813), 0x06 }, + { CCI_REG8(0x3814), 0x31 }, + { CCI_REG8(0x3815), 0x31 }, + { CCI_REG8(0x3630), 0x2e }, + { CCI_REG8(0x3632), 0xe2 }, + { CCI_REG8(0x3633), 0x23 }, + { CCI_REG8(0x3634), 0x44 }, + { CCI_REG8(0x3636), 0x06 }, + { CCI_REG8(0x3620), 0x64 }, + { CCI_REG8(0x3621), 0xe0 }, + { CCI_REG8(0x3600), 0x37 }, + { CCI_REG8(0x3704), 0xa0 }, + { CCI_REG8(0x3703), 0x5a }, + { CCI_REG8(0x3715), 0x78 }, + { CCI_REG8(0x3717), 0x01 }, + { CCI_REG8(0x3731), 0x02 }, + { CCI_REG8(0x370b), 0x60 }, + { CCI_REG8(0x3705), 0x1a }, + { CCI_REG8(0x3f05), 0x02 }, + { CCI_REG8(0x3f06), 0x10 }, + { CCI_REG8(0x3f01), 0x0a }, + { CCI_REG8(0x3a08), 0x01 }, + { CCI_REG8(0x3a09), 0x28 }, + { CCI_REG8(0x3a0a), 0x00 }, + { CCI_REG8(0x3a0b), 0xf6 }, + { CCI_REG8(0x3a0d), 0x08 }, + { CCI_REG8(0x3a0e), 0x06 }, + { CCI_REG8(0x3a0f), 0x58 }, + { CCI_REG8(0x3a10), 0x50 }, + { CCI_REG8(0x3a1b), 0x58 }, + { CCI_REG8(0x3a1e), 0x50 }, + { CCI_REG8(0x3a11), 0x60 }, + { CCI_REG8(0x3a1f), 0x28 }, + { CCI_REG8(0x4001), 0x02 }, + { CCI_REG8(0x4004), 0x04 }, + { CCI_REG8(0x4000), 0x09 }, + { CCI_REG8(0x4837), 0x16 }, + { CCI_REG8(0x4800), 0x24 }, + { CCI_REG8(0x3503), 0x03 }, + { CCI_REG8(0x3820), 0x41 }, + { CCI_REG8(0x3821), 0x07 }, + { CCI_REG8(0x350a), 0x00 }, + { CCI_REG8(0x350b), 0x10 }, + { CCI_REG8(0x3500), 0x00 }, + { CCI_REG8(0x3501), 0x1a }, + { CCI_REG8(0x3502), 0xf0 }, + { CCI_REG8(0x3212), 0xa0 }, + { CCI_REG8(0x0100), 0x01 }, }; -static struct regval_list ov5647_640x480_10bpp[] = { - {0x0100, 0x00}, - {0x0103, 0x01}, - {0x3035, 0x11}, - {0x3036, 0x46}, - {0x303c, 0x11}, - {0x3821, 0x07}, - {0x3820, 0x41}, - {0x370c, 0x03}, - {0x3612, 0x59}, - {0x3618, 0x00}, - {0x5000, 0x06}, - {0x5003, 0x08}, - {0x5a00, 0x08}, - {0x3000, 0xff}, - {0x3001, 0xff}, - {0x3002, 0xff}, - {0x301d, 0xf0}, - {0x3a18, 0x00}, - {0x3a19, 0xf8}, - {0x3c01, 0x80}, - {0x3b07, 0x0c}, - {0x380c, 0x07}, - {0x380d, 0x3c}, - {0x3814, 0x35}, - {0x3815, 0x35}, - {0x3708, 0x64}, - {0x3709, 0x52}, - {0x3808, 0x02}, - {0x3809, 0x80}, - {0x380a, 0x01}, - {0x380b, 0xe0}, - {0x3800, 0x00}, - {0x3801, 0x10}, - {0x3802, 0x00}, - {0x3803, 0x00}, - {0x3804, 0x0a}, - {0x3805, 0x2f}, - {0x3806, 0x07}, - {0x3807, 0x9f}, - {0x3630, 0x2e}, - {0x3632, 0xe2}, - {0x3633, 0x23}, - {0x3634, 0x44}, - {0x3620, 0x64}, - {0x3621, 0xe0}, - {0x3600, 0x37}, - {0x3704, 0xa0}, - {0x3703, 0x5a}, - {0x3715, 0x78}, - {0x3717, 0x01}, - {0x3731, 0x02}, - {0x370b, 0x60}, - {0x3705, 0x1a}, - {0x3f05, 0x02}, - {0x3f06, 0x10}, - {0x3f01, 0x0a}, - {0x3a08, 0x01}, - {0x3a09, 0x2e}, - {0x3a0a, 0x00}, - {0x3a0b, 0xfb}, - {0x3a0d, 0x02}, - {0x3a0e, 0x01}, - {0x3a0f, 0x58}, - {0x3a10, 0x50}, - {0x3a1b, 0x58}, - {0x3a1e, 0x50}, - {0x3a11, 0x60}, - {0x3a1f, 0x28}, - {0x4001, 0x02}, - {0x4004, 0x02}, - {0x4000, 0x09}, - {0x3000, 0x00}, - {0x3001, 0x00}, - {0x3002, 0x00}, - {0x3017, 0xe0}, - {0x301c, 0xfc}, - {0x3636, 0x06}, - {0x3016, 0x08}, - {0x3827, 0xec}, - {0x3018, 0x44}, - {0x3035, 0x21}, - {0x3106, 0xf5}, - {0x3034, 0x1a}, - {0x301c, 0xf8}, - {0x4800, 0x34}, - {0x3503, 0x03}, - {0x0100, 0x01}, +static const struct cci_reg_sequence ov5647_640x480_10bpp[] = { + { CCI_REG8(0x0100), 0x00 }, + { CCI_REG8(0x0103), 0x01 }, + { CCI_REG8(0x3035), 0x11 }, + { CCI_REG8(0x3036), 0x46 }, + { CCI_REG8(0x303c), 0x11 }, + { CCI_REG8(0x3821), 0x07 }, + { CCI_REG8(0x3820), 0x41 }, + { CCI_REG8(0x370c), 0x03 }, + { CCI_REG8(0x3612), 0x59 }, + { CCI_REG8(0x3618), 0x00 }, + { CCI_REG8(0x5000), 0x06 }, + { CCI_REG8(0x5003), 0x08 }, + { CCI_REG8(0x5a00), 0x08 }, + { CCI_REG8(0x3000), 0xff }, + { CCI_REG8(0x3001), 0xff }, + { CCI_REG8(0x3002), 0xff }, + { CCI_REG8(0x301d), 0xf0 }, + { CCI_REG8(0x3a18), 0x00 }, + { CCI_REG8(0x3a19), 0xf8 }, + { CCI_REG8(0x3c01), 0x80 }, + { CCI_REG8(0x3b07), 0x0c }, + { CCI_REG8(0x380c), 0x07 }, + { CCI_REG8(0x380d), 0x3c }, + { CCI_REG8(0x3814), 0x35 }, + { CCI_REG8(0x3815), 0x35 }, + { CCI_REG8(0x3708), 0x64 }, + { CCI_REG8(0x3709), 0x52 }, + { CCI_REG8(0x3808), 0x02 }, + { CCI_REG8(0x3809), 0x80 }, + { CCI_REG8(0x380a), 0x01 }, + { CCI_REG8(0x380b), 0xe0 }, + { CCI_REG8(0x3800), 0x00 }, + { CCI_REG8(0x3801), 0x10 }, + { CCI_REG8(0x3802), 0x00 }, + { CCI_REG8(0x3803), 0x00 }, + { CCI_REG8(0x3804), 0x0a }, + { CCI_REG8(0x3805), 0x2f }, + { CCI_REG8(0x3806), 0x07 }, + { CCI_REG8(0x3807), 0x9f }, + { CCI_REG8(0x3630), 0x2e }, + { CCI_REG8(0x3632), 0xe2 }, + { CCI_REG8(0x3633), 0x23 }, + { CCI_REG8(0x3634), 0x44 }, + { CCI_REG8(0x3620), 0x64 }, + { CCI_REG8(0x3621), 0xe0 }, + { CCI_REG8(0x3600), 0x37 }, + { CCI_REG8(0x3704), 0xa0 }, + { CCI_REG8(0x3703), 0x5a }, + { CCI_REG8(0x3715), 0x78 }, + { CCI_REG8(0x3717), 0x01 }, + { CCI_REG8(0x3731), 0x02 }, + { CCI_REG8(0x370b), 0x60 }, + { CCI_REG8(0x3705), 0x1a }, + { CCI_REG8(0x3f05), 0x02 }, + { CCI_REG8(0x3f06), 0x10 }, + { CCI_REG8(0x3f01), 0x0a }, + { CCI_REG8(0x3a08), 0x01 }, + { CCI_REG8(0x3a09), 0x2e }, + { CCI_REG8(0x3a0a), 0x00 }, + { CCI_REG8(0x3a0b), 0xfb }, + { CCI_REG8(0x3a0d), 0x02 }, + { CCI_REG8(0x3a0e), 0x01 }, + { CCI_REG8(0x3a0f), 0x58 }, + { CCI_REG8(0x3a10), 0x50 }, + { CCI_REG8(0x3a1b), 0x58 }, + { CCI_REG8(0x3a1e), 0x50 }, + { CCI_REG8(0x3a11), 0x60 }, + { CCI_REG8(0x3a1f), 0x28 }, + { CCI_REG8(0x4001), 0x02 }, + { CCI_REG8(0x4004), 0x02 }, + { CCI_REG8(0x4000), 0x09 }, + { CCI_REG8(0x3000), 0x00 }, + { CCI_REG8(0x3001), 0x00 }, + { CCI_REG8(0x3002), 0x00 }, + { CCI_REG8(0x3017), 0xe0 }, + { CCI_REG8(0x301c), 0xfc }, + { CCI_REG8(0x3636), 0x06 }, + { CCI_REG8(0x3016), 0x08 }, + { CCI_REG8(0x3827), 0xec }, + { CCI_REG8(0x3018), 0x44 }, + { CCI_REG8(0x3035), 0x21 }, + { CCI_REG8(0x3106), 0xf5 }, + { CCI_REG8(0x3034), 0x1a }, + { CCI_REG8(0x301c), 0xf8 }, + { CCI_REG8(0x4800), 0x34 }, + { CCI_REG8(0x3503), 0x03 }, + { CCI_REG8(0x0100), 0x01 }, }; static const struct ov5647_mode ov5647_modes[] = { @@ -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 = cci_multi_reg_write(sensor->regmap, sensor->mode->reg_list, + sensor->mode->num_regs, NULL); 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 = cci_multi_reg_write(sensor->regmap, sensor_oe_enable_regs, + ARRAY_SIZE(sensor_oe_enable_regs), NULL); 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 = cci_multi_reg_write(sensor->regmap, sensor_oe_disable_regs, + ARRAY_SIZE(sensor_oe_disable_regs), NULL); 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 = PTR_ERR(sensor->regmap); + dev_err(dev, "failed to initialize CCI: %d\n", ret); + goto entity_cleanup; + } + ret = ov5647_power_on(dev); if (ret) goto entity_cleanup; -- 2.43.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers 2025-12-29 2:30 ` [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers Xiaolei Wang @ 2025-12-29 12:37 ` Tarang Raval 2025-12-29 13:01 ` Laurent Pinchart 2025-12-29 13:26 ` johannes.goede 2025-12-29 14:52 ` Tarang Raval 2 siblings, 1 reply; 14+ messages in thread From: Tarang Raval @ 2025-12-29 12:37 UTC (permalink / raw) To: Xiaolei Wang, laurent.pinchart@ideasonboard.com, sakari.ailus@linux.intel.com, dave.stevenson@raspberrypi.com, jacopo@jmondi.org, mchehab@kernel.org, prabhakar.mahadev-lad.rj@bp.renesas.com, hverkuil+cisco@kernel.org, johannes.goede@oss.qualcomm.com, hverkuil-cisco@xs4all.nl, jai.luthra@ideasonboard.com Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org 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 | 997 +++++++++++++++++-------------------- > 2 files changed, 453 insertions(+), 545 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..fd69f1616794 100644 > --- a/drivers/media/i2c/ov5647.c > +++ b/drivers/media/i2c/ov5647.c > @@ -22,6 +22,7 @@ > #include <linux/pm_runtime.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 +42,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(0x350b) It should be 0x350a, not 0x350b. > +#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 +77,19 @@ > #define OV5647_EXPOSURE_DEFAULT 1000 > #define OV5647_EXPOSURE_MAX 65535 ... > @@ -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 = PTR_ERR(sensor->regmap); > + dev_err(dev, "failed to initialize CCI: %d\n", ret); Use return dev_err_probe(); > + goto entity_cleanup; > + } > + > ret = ov5647_power_on(dev); > if (ret) > goto entity_cleanup; > -- > 2.43.0 Best Regards, Tarang ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers 2025-12-29 12:37 ` Tarang Raval @ 2025-12-29 13:01 ` Laurent Pinchart 2025-12-29 13:26 ` Tarang Raval 2025-12-31 2:57 ` xiaolei wang 0 siblings, 2 replies; 14+ messages in thread From: Laurent Pinchart @ 2025-12-29 13:01 UTC (permalink / raw) To: Tarang Raval Cc: Xiaolei Wang, sakari.ailus@linux.intel.com, dave.stevenson@raspberrypi.com, jacopo@jmondi.org, mchehab@kernel.org, prabhakar.mahadev-lad.rj@bp.renesas.com, hverkuil+cisco@kernel.org, johannes.goede@oss.qualcomm.com, hverkuil-cisco@xs4all.nl, jai.luthra@ideasonboard.com, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org On Mon, Dec 29, 2025 at 12:37:41PM +0000, Tarang Raval 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> > > --- > > drivers/media/i2c/Kconfig | 1 + > > drivers/media/i2c/ov5647.c | 997 +++++++++++++++++-------------------- > > 2 files changed, 453 insertions(+), 545 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..fd69f1616794 100644 > > --- a/drivers/media/i2c/ov5647.c > > +++ b/drivers/media/i2c/ov5647.c > > @@ -22,6 +22,7 @@ > > #include <linux/pm_runtime.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 +42,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(0x350b) > > It should be 0x350a, not 0x350b. > > > +#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 +77,19 @@ > > #define OV5647_EXPOSURE_DEFAULT 1000 > > #define OV5647_EXPOSURE_MAX 65535 > > ... > > > @@ -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 = PTR_ERR(sensor->regmap); > > + dev_err(dev, "failed to initialize CCI: %d\n", ret); > > Use return dev_err_probe(); dev_err_probe() is fine, but goto entity_cleanup is needed. > > + goto entity_cleanup; > > + } > > + > > ret = ov5647_power_on(dev); > > if (ret) > > goto entity_cleanup; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers 2025-12-29 13:01 ` Laurent Pinchart @ 2025-12-29 13:26 ` Tarang Raval 2025-12-31 2:57 ` xiaolei wang 1 sibling, 0 replies; 14+ messages in thread From: Tarang Raval @ 2025-12-29 13:26 UTC (permalink / raw) To: Laurent Pinchart, Xiaolei Wang Cc: sakari.ailus@linux.intel.com, dave.stevenson@raspberrypi.com, jacopo@jmondi.org, mchehab@kernel.org, prabhakar.mahadev-lad.rj@bp.renesas.com, hverkuil+cisco@kernel.org, johannes.goede@oss.qualcomm.com, hverkuil-cisco@xs4all.nl, jai.luthra@ideasonboard.com, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org > On Mon, Dec 29, 2025 at 12:37:41PM +0000, Tarang Raval 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> > > > --- > > > drivers/media/i2c/Kconfig | 1 + > > > drivers/media/i2c/ov5647.c | 997 +++++++++++++++++-------------------- > > > 2 files changed, 453 insertions(+), 545 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..fd69f1616794 100644 > > > --- a/drivers/media/i2c/ov5647.c > > > +++ b/drivers/media/i2c/ov5647.c > > > @@ -22,6 +22,7 @@ > > > #include <linux/pm_runtime.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 +42,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(0x350b) > > > > It should be 0x350a, not 0x350b. > > > > > +#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 +77,19 @@ > > > #define OV5647_EXPOSURE_DEFAULT 1000 > > > #define OV5647_EXPOSURE_MAX 65535 > > > > ... > > > > > @@ -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 = PTR_ERR(sensor->regmap); > > > + dev_err(dev, "failed to initialize CCI: %d\n", ret); > > > > Use return dev_err_probe(); > > dev_err_probe() is fine, but goto entity_cleanup is needed. Yes, my bad. Keep the existing return path. > > > + goto entity_cleanup; > > > + } > > > + > > > ret = ov5647_power_on(dev); > > > if (ret) > > > goto entity_cleanup; > > -- > Regards, > > Laurent Pinchart ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers 2025-12-29 13:01 ` Laurent Pinchart 2025-12-29 13:26 ` Tarang Raval @ 2025-12-31 2:57 ` xiaolei wang 1 sibling, 0 replies; 14+ messages in thread From: xiaolei wang @ 2025-12-31 2:57 UTC (permalink / raw) To: Laurent Pinchart, Tarang Raval Cc: sakari.ailus@linux.intel.com, dave.stevenson@raspberrypi.com, jacopo@jmondi.org, mchehab@kernel.org, prabhakar.mahadev-lad.rj@bp.renesas.com, hverkuil+cisco@kernel.org, johannes.goede@oss.qualcomm.com, hverkuil-cisco@xs4all.nl, jai.luthra@ideasonboard.com, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org On 12/29/25 21:01, Laurent Pinchart 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. > > On Mon, Dec 29, 2025 at 12:37:41PM +0000, Tarang Raval 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> >>> --- >>> drivers/media/i2c/Kconfig | 1 + >>> drivers/media/i2c/ov5647.c | 997 +++++++++++++++++-------------------- >>> 2 files changed, 453 insertions(+), 545 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..fd69f1616794 100644 >>> --- a/drivers/media/i2c/ov5647.c >>> +++ b/drivers/media/i2c/ov5647.c >>> @@ -22,6 +22,7 @@ >>> #include <linux/pm_runtime.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 +42,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(0x350b) >> It should be 0x350a, not 0x350b. >> >>> +#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 +77,19 @@ >>> #define OV5647_EXPOSURE_DEFAULT 1000 >>> #define OV5647_EXPOSURE_MAX 65535 >> ... >> >>> @@ -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 = PTR_ERR(sensor->regmap); >>> + dev_err(dev, "failed to initialize CCI: %d\n", ret); >> Use return dev_err_probe(); > dev_err_probe() is fine, but goto entity_cleanup is needed. Hi Tarang and Laurent, Thank you both for the review feedback. I'll address both issues in v3: 1. Fix OV5647_REG_GAIN to use CCI_REG16(0x350a) instead of 0x350b 2. Update the error handling to use dev_err_probe() while maintaining the goto entity_cleanup path for proper resource cleanup Will send the updated patch shortly. Best regards, Xiaolei > >>> + goto entity_cleanup; >>> + } >>> + >>> ret = ov5647_power_on(dev); >>> if (ret) >>> goto entity_cleanup; > -- > Regards, > > Laurent Pinchart ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers 2025-12-29 2:30 ` [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers Xiaolei Wang 2025-12-29 12:37 ` Tarang Raval @ 2025-12-29 13:26 ` johannes.goede 2025-12-29 14:52 ` Tarang Raval 2 siblings, 0 replies; 14+ messages in thread From: johannes.goede @ 2025-12-29 13:26 UTC (permalink / raw) To: Xiaolei Wang, laurent.pinchart, sakari.ailus, dave.stevenson, jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco, hverkuil-cisco, jai.luthra Cc: linux-media, linux-kernel Hi, On 29-Dec-25 03:30, 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. Thank you for your patch, it is great to see more drivers being converted to the CCI register access helpers. > 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..fd69f1616794 100644 > --- a/drivers/media/i2c/ov5647.c > +++ b/drivers/media/i2c/ov5647.c ... > @@ -130,377 +122,377 @@ static const u8 ov5647_test_pattern_val[] = { > 0x81, /* Random Data */ > }; > > -static const struct regval_list sensor_oe_disable_regs[] = { > - {0x3000, 0x00}, > - {0x3001, 0x00}, > - {0x3002, 0x00}, > +static const struct cci_reg_sequence sensor_oe_disable_regs[] = { > + { CCI_REG8(0x3000), 0x00 }, > + { CCI_REG8(0x3001), 0x00 }, > + { CCI_REG8(0x3002), 0x00 }, > }; > > -static const struct regval_list sensor_oe_enable_regs[] = { > - {0x3000, 0x0f}, > - {0x3001, 0xff}, > - {0x3002, 0xe4}, > +static const struct cci_reg_sequence sensor_oe_enable_regs[] = { > + { CCI_REG8(0x3000), 0x0f }, > + { CCI_REG8(0x3001), 0xff }, > + { CCI_REG8(0x3002), 0xe4 }, > }; For these 2, but also for the 2 much longer arrays with address, value pairs which you are replacing, you are replacing all the register addresses with CCI_REG8(0x3000). Even though some of these are like 16 bit or even 24bit registers, this in itself is not a problem. But if you are replacing them 1:1 like this anyway then IMHO it is better to just: - Directly use struct reg_sequence instead of struct cci_reg_sequence - Call regmap_multi_reg_write() instead of cci_multi_reg_write() then you can keep all the initializer values for the arrays the same (no need to add CCI_REG8() around the addresses). This is also how this was done in other drivers when they were converted to the CCI helpers. This greatly reduces the size of the diff and makes it much easier to review the patch. Regards, Hans ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers 2025-12-29 2:30 ` [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers Xiaolei Wang 2025-12-29 12:37 ` Tarang Raval 2025-12-29 13:26 ` johannes.goede @ 2025-12-29 14:52 ` Tarang Raval 2 siblings, 0 replies; 14+ messages in thread From: Tarang Raval @ 2025-12-29 14:52 UTC (permalink / raw) To: Xiaolei Wang, laurent.pinchart@ideasonboard.com, sakari.ailus@linux.intel.com, dave.stevenson@raspberrypi.com, jacopo@jmondi.org, mchehab@kernel.org, prabhakar.mahadev-lad.rj@bp.renesas.com, hverkuil+cisco@kernel.org, johannes.goede@oss.qualcomm.com, hverkuil-cisco@xs4all.nl, jai.luthra@ideasonboard.com Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org > 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 = cci_multi_reg_write(sensor->regmap, sensor->mode->reg_list, > + sensor->mode->num_regs, NULL); > 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; Are both read operations in ov5647_set_mode() really needed? Every mode table already ends with writing 0x0100 => 0x01. Does anyone know the purpose of these reads, or are they unused? Best Regards, Tarang > 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; > } ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/3] media: i2c: ov5647: Switch to using the sub-device state lock 2025-12-29 2:30 [PATCH v2 0/3] media: i2c: ov5647: Modernize driver with CCI and new stream APIs Xiaolei Wang 2025-12-29 2:30 ` [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers Xiaolei Wang @ 2025-12-29 2:30 ` Xiaolei Wang 2025-12-29 18:55 ` Sakari Ailus 2025-12-29 2:30 ` [PATCH v2 3/3] media: i2c: ov5647: switch to {enable,disable}_streams Xiaolei Wang 2 siblings, 1 reply; 14+ messages in thread From: Xiaolei Wang @ 2025-12-29 2:30 UTC (permalink / raw) To: 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 | 40 +++++++++++++------------------------- 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c index fd69f1616794..f0ca8cc14794 100644 --- a/drivers/media/i2c/ov5647.c +++ b/drivers/media/i2c/ov5647.c @@ -91,7 +91,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; @@ -652,7 +651,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd) } /* Apply customized values from user when stream starts. */ - ret = __v4l2_ctrl_handler_setup(sd->ctrl_handler); + ret = v4l2_ctrl_handler_setup(sd->ctrl_handler); if (ret) return ret; @@ -807,15 +806,12 @@ __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); int ret; - mutex_lock(&sensor->lock); - if (enable) { ret = pm_runtime_resume_and_get(&client->dev); if (ret < 0) - goto error_unlock; + return ret; ret = ov5647_stream_on(sd); if (ret < 0) { @@ -831,14 +827,10 @@ static int ov5647_s_stream(struct v4l2_subdev *sd, int enable) pm_runtime_put(&client->dev); } - mutex_unlock(&sensor->lock); - return 0; error_pm: pm_runtime_put(&client->dev); -error_unlock: - mutex_unlock(&sensor->lock); return ret; } @@ -886,7 +878,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 +889,6 @@ static int ov5647_get_pad_fmt(struct v4l2_subdev *sd, } *fmt = *sensor_format; - mutex_unlock(&sensor->lock); return 0; } @@ -916,7 +906,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 +934,6 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd, exposure_def); } *fmt = mode->format; - mutex_unlock(&sensor->lock); return 0; } @@ -958,10 +946,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 +1100,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 +1299,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 +1331,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 +1351,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 +1369,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 related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] media: i2c: ov5647: Switch to using the sub-device state lock 2025-12-29 2:30 ` [PATCH v2 2/3] media: i2c: ov5647: Switch to using the sub-device state lock Xiaolei Wang @ 2025-12-29 18:55 ` Sakari Ailus 2025-12-31 2:58 ` xiaolei wang 0 siblings, 1 reply; 14+ messages in thread From: Sakari Ailus @ 2025-12-29 18:55 UTC (permalink / raw) To: Xiaolei Wang Cc: laurent.pinchart, dave.stevenson, jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco, johannes.goede, hverkuil-cisco, jai.luthra, linux-media, linux-kernel Hi Xiaolei, On Mon, Dec 29, 2025 at 10:30:17AM +0800, Xiaolei Wang wrote: > 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 | 40 +++++++++++++------------------------- > 1 file changed, 14 insertions(+), 26 deletions(-) > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c > index fd69f1616794..f0ca8cc14794 100644 > --- a/drivers/media/i2c/ov5647.c > +++ b/drivers/media/i2c/ov5647.c > @@ -91,7 +91,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; > @@ -652,7 +651,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd) > } > > /* Apply customized values from user when stream starts. */ > - ret = __v4l2_ctrl_handler_setup(sd->ctrl_handler); > + ret = v4l2_ctrl_handler_setup(sd->ctrl_handler); > if (ret) > return ret; > > @@ -807,15 +806,12 @@ __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); > int ret; > > - mutex_lock(&sensor->lock); Note that you shouldn't remove mutex_lock() here quite yet -- s_stream() callback won't involve sub-device state and thus the caller won't take the state lock either. In other words, the end result is fine after the third patch so you should explicitly lock the active state and remove that in the third patch (see e.g. v4l2_subdev_lock_and_get_active_state() in drivers/media/i2c/imx290.c). > - > if (enable) { > ret = pm_runtime_resume_and_get(&client->dev); > if (ret < 0) > - goto error_unlock; > + return ret; > > ret = ov5647_stream_on(sd); > if (ret < 0) { > @@ -831,14 +827,10 @@ static int ov5647_s_stream(struct v4l2_subdev *sd, int enable) > pm_runtime_put(&client->dev); > } > > - mutex_unlock(&sensor->lock); > - > return 0; > > error_pm: > pm_runtime_put(&client->dev); > -error_unlock: > - mutex_unlock(&sensor->lock); > > return ret; > } -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] media: i2c: ov5647: Switch to using the sub-device state lock 2025-12-29 18:55 ` Sakari Ailus @ 2025-12-31 2:58 ` xiaolei wang 0 siblings, 0 replies; 14+ messages in thread From: xiaolei wang @ 2025-12-31 2:58 UTC (permalink / raw) To: Sakari Ailus Cc: laurent.pinchart, dave.stevenson, jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco, johannes.goede, hverkuil-cisco, jai.luthra, linux-media, linux-kernel On 12/30/25 02:55, Sakari Ailus wrote: > CAUTION: This email comes from a non Wind River email account! > Do not click links or open attachments unless you recognize the sender and know the content is safe. > > Hi Xiaolei, > > On Mon, Dec 29, 2025 at 10:30:17AM +0800, Xiaolei Wang wrote: >> 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 | 40 +++++++++++++------------------------- >> 1 file changed, 14 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c >> index fd69f1616794..f0ca8cc14794 100644 >> --- a/drivers/media/i2c/ov5647.c >> +++ b/drivers/media/i2c/ov5647.c >> @@ -91,7 +91,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; >> @@ -652,7 +651,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd) >> } >> >> /* Apply customized values from user when stream starts. */ >> - ret = __v4l2_ctrl_handler_setup(sd->ctrl_handler); >> + ret = v4l2_ctrl_handler_setup(sd->ctrl_handler); >> if (ret) >> return ret; >> >> @@ -807,15 +806,12 @@ __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); >> int ret; >> >> - mutex_lock(&sensor->lock); > Note that you shouldn't remove mutex_lock() here quite yet -- s_stream() > callback won't involve sub-device state and thus the caller won't take the > state lock either. In other words, the end result is fine after the third > patch so you should explicitly lock the active state and remove that in the > third patch (see e.g. v4l2_subdev_lock_and_get_active_state() in > drivers/media/i2c/imx290.c). Hi Hans, Thank you for the detailed review and suggestions. You're absolutely right about the approach. Using regmap_multi_reg_write() with struct reg_sequence would indeed be cleaner and result in a much smaller, more reviewable diff. I'll revise the patch to: - Use struct reg_sequence instead of struct cci_reg_sequence - Call regmap_multi_reg_write() instead of cci_multi_reg_write() - Keep the existing array initializer values unchanged This will maintain the same functionality while making the conversion more straightforward and consistent with other driver conversions. I'll send v3 shortly. > >> - >> if (enable) { >> ret = pm_runtime_resume_and_get(&client->dev); >> if (ret < 0) >> - goto error_unlock; >> + return ret; >> >> ret = ov5647_stream_on(sd); >> if (ret < 0) { >> @@ -831,14 +827,10 @@ static int ov5647_s_stream(struct v4l2_subdev *sd, int enable) >> pm_runtime_put(&client->dev); >> } >> >> - mutex_unlock(&sensor->lock); >> - >> return 0; >> >> error_pm: >> pm_runtime_put(&client->dev); >> -error_unlock: >> - mutex_unlock(&sensor->lock); >> >> return ret; >> } > -- > Kind regards, > > Sakari Ailus ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/3] media: i2c: ov5647: switch to {enable,disable}_streams 2025-12-29 2:30 [PATCH v2 0/3] media: i2c: ov5647: Modernize driver with CCI and new stream APIs Xiaolei Wang 2025-12-29 2:30 ` [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers Xiaolei Wang 2025-12-29 2:30 ` [PATCH v2 2/3] media: i2c: ov5647: Switch to using the sub-device state lock Xiaolei Wang @ 2025-12-29 2:30 ` Xiaolei Wang 2025-12-29 13:43 ` Laurent Pinchart 2 siblings, 1 reply; 14+ messages in thread From: Xiaolei Wang @ 2025-12-29 2:30 UTC (permalink / raw) To: 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> --- drivers/media/i2c/ov5647.c | 69 ++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 40 deletions(-) diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c index f0ca8cc14794..7f4541f46335 100644 --- a/drivers/media/i2c/ov5647.c +++ b/drivers/media/i2c/ov5647.c @@ -637,23 +637,29 @@ static int ov5647_set_mode(struct v4l2_subdev *sd) return 0; } -static int ov5647_stream_on(struct v4l2_subdev *sd) +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 err_rpm_put; } /* Apply customized values from user when stream starts. */ - ret = v4l2_ctrl_handler_setup(sd->ctrl_handler); + ret = __v4l2_ctrl_handler_setup(sd->ctrl_handler); if (ret) - return ret; + goto err_rpm_put; if (sensor->clock_ncont) val |= MIPI_CTRL00_CLOCK_LANE_GATE | @@ -663,11 +669,18 @@ 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); +err_rpm_put: + 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; @@ -677,13 +690,15 @@ static int ov5647_stream_off(struct v4l2_subdev *sd) cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x0f, &ret); cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x01, &ret); + pm_runtime_put(&client->dev); + return ret; } static int ov5647_power_on(struct device *dev) { struct ov5647 *sensor = dev_get_drvdata(dev); - int ret; + int ret = 0; dev_dbg(dev, "OV5647 power on\n"); @@ -706,7 +721,11 @@ static int ov5647_power_on(struct device *dev) } /* Stream off to coax lanes into LP-11 state. */ - ret = ov5647_stream_off(&sensor->sd); + 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); if (ret < 0) { dev_err(dev, "camera not available, check power\n"); goto error_clk_disable; @@ -803,40 +822,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); - int ret; - - if (enable) { - ret = pm_runtime_resume_and_get(&client->dev); - if (ret < 0) - return ret; - - 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); - } - - return 0; - -error_pm: - pm_runtime_put(&client->dev); - - 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, @@ -979,6 +966,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 related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] media: i2c: ov5647: switch to {enable,disable}_streams 2025-12-29 2:30 ` [PATCH v2 3/3] media: i2c: ov5647: switch to {enable,disable}_streams Xiaolei Wang @ 2025-12-29 13:43 ` Laurent Pinchart 2025-12-31 3:02 ` xiaolei wang 0 siblings, 1 reply; 14+ messages in thread From: Laurent Pinchart @ 2025-12-29 13:43 UTC (permalink / raw) To: Xiaolei Wang Cc: sakari.ailus, dave.stevenson, jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco, johannes.goede, hverkuil-cisco, jai.luthra, linux-media, linux-kernel On Mon, Dec 29, 2025 at 10:30:18AM +0800, Xiaolei Wang wrote: > Switch from s_stream to enable_streams and disable_streams callbacks. > > Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> > --- > drivers/media/i2c/ov5647.c | 69 ++++++++++++++++---------------------- > 1 file changed, 29 insertions(+), 40 deletions(-) > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c > index f0ca8cc14794..7f4541f46335 100644 > --- a/drivers/media/i2c/ov5647.c > +++ b/drivers/media/i2c/ov5647.c > @@ -637,23 +637,29 @@ static int ov5647_set_mode(struct v4l2_subdev *sd) > return 0; > } > > -static int ov5647_stream_on(struct v4l2_subdev *sd) > +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 err_rpm_put; > } > > /* Apply customized values from user when stream starts. */ > - ret = v4l2_ctrl_handler_setup(sd->ctrl_handler); > + ret = __v4l2_ctrl_handler_setup(sd->ctrl_handler); > if (ret) > - return ret; > + goto err_rpm_put; > > if (sensor->clock_ncont) > val |= MIPI_CTRL00_CLOCK_LANE_GATE | > @@ -663,11 +669,18 @@ 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); > > +err_rpm_put: I would name the label 'done' as it's also used in the normal exit path, not only in case of errors. > + 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; > > @@ -677,13 +690,15 @@ static int ov5647_stream_off(struct v4l2_subdev *sd) > cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x0f, &ret); > cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x01, &ret); > > + pm_runtime_put(&client->dev); > + > return ret; > } > > static int ov5647_power_on(struct device *dev) > { > struct ov5647 *sensor = dev_get_drvdata(dev); > - int ret; > + int ret = 0; > > dev_dbg(dev, "OV5647 power on\n"); > > @@ -706,7 +721,11 @@ static int ov5647_power_on(struct device *dev) > } > > /* Stream off to coax lanes into LP-11 state. */ > - ret = ov5647_stream_off(&sensor->sd); > + 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); Could you please factor this code out to a separate function (you can name it ov5647_stream_stop() for instance) instead of duplicating it ? With that (and the 0 initialization of ret above dropped as it won't be needed anymore), Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > if (ret < 0) { > dev_err(dev, "camera not available, check power\n"); > goto error_clk_disable; > @@ -803,40 +822,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); > - int ret; > - > - if (enable) { > - ret = pm_runtime_resume_and_get(&client->dev); > - if (ret < 0) > - return ret; > - > - 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); > - } > - > - return 0; > - > -error_pm: > - pm_runtime_put(&client->dev); > - > - 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, > @@ -979,6 +966,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 = { -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] media: i2c: ov5647: switch to {enable,disable}_streams 2025-12-29 13:43 ` Laurent Pinchart @ 2025-12-31 3:02 ` xiaolei wang 0 siblings, 0 replies; 14+ messages in thread From: xiaolei wang @ 2025-12-31 3:02 UTC (permalink / raw) To: Laurent Pinchart Cc: sakari.ailus, dave.stevenson, jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco, johannes.goede, hverkuil-cisco, jai.luthra, linux-media, linux-kernel On 12/29/25 21:43, Laurent Pinchart 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. > > On Mon, Dec 29, 2025 at 10:30:18AM +0800, Xiaolei Wang wrote: >> Switch from s_stream to enable_streams and disable_streams callbacks. >> >> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> >> --- >> drivers/media/i2c/ov5647.c | 69 ++++++++++++++++---------------------- >> 1 file changed, 29 insertions(+), 40 deletions(-) >> >> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c >> index f0ca8cc14794..7f4541f46335 100644 >> --- a/drivers/media/i2c/ov5647.c >> +++ b/drivers/media/i2c/ov5647.c >> @@ -637,23 +637,29 @@ static int ov5647_set_mode(struct v4l2_subdev *sd) >> return 0; >> } >> >> -static int ov5647_stream_on(struct v4l2_subdev *sd) >> +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 err_rpm_put; >> } >> >> /* Apply customized values from user when stream starts. */ >> - ret = v4l2_ctrl_handler_setup(sd->ctrl_handler); >> + ret = __v4l2_ctrl_handler_setup(sd->ctrl_handler); >> if (ret) >> - return ret; >> + goto err_rpm_put; >> >> if (sensor->clock_ncont) >> val |= MIPI_CTRL00_CLOCK_LANE_GATE | >> @@ -663,11 +669,18 @@ 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); >> >> +err_rpm_put: > I would name the label 'done' as it's also used in the normal exit path, > not only in case of errors. > >> + 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; >> >> @@ -677,13 +690,15 @@ static int ov5647_stream_off(struct v4l2_subdev *sd) >> cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x0f, &ret); >> cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x01, &ret); >> >> + pm_runtime_put(&client->dev); >> + >> return ret; >> } >> >> static int ov5647_power_on(struct device *dev) >> { >> struct ov5647 *sensor = dev_get_drvdata(dev); >> - int ret; >> + int ret = 0; >> >> dev_dbg(dev, "OV5647 power on\n"); >> >> @@ -706,7 +721,11 @@ static int ov5647_power_on(struct device *dev) >> } >> >> /* Stream off to coax lanes into LP-11 state. */ >> - ret = ov5647_stream_off(&sensor->sd); >> + 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); > Could you please factor this code out to a separate function (you can > name it ov5647_stream_stop() for instance) instead of duplicating it ? > > With that (and the 0 initialization of ret above dropped as it won't be > needed anymore), > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Hi Laurent, Thank you for the review and the suggestions. I'll address both points in v3: 1. Rename the label from 'err_rpm_put' to 'done' since it's used for both error and normal exit paths. 2. Extract the duplicated stream stop code into a separate ov5647_stream_stop() function to eliminate code duplication between ov5647_disable_streams() and ov5647_power_on(). 3. Remove the unnecessary ret = 0 initialization once the function is extracted. Will send the updated patch shortly. Best regards, Xiaolei > >> if (ret < 0) { >> dev_err(dev, "camera not available, check power\n"); >> goto error_clk_disable; >> @@ -803,40 +822,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); >> - int ret; >> - >> - if (enable) { >> - ret = pm_runtime_resume_and_get(&client->dev); >> - if (ret < 0) >> - return ret; >> - >> - 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); >> - } >> - >> - return 0; >> - >> -error_pm: >> - pm_runtime_put(&client->dev); >> - >> - 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, >> @@ -979,6 +966,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 = { > -- > Regards, > > Laurent Pinchart ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2025-12-31 3:02 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-29 2:30 [PATCH v2 0/3] media: i2c: ov5647: Modernize driver with CCI and new stream APIs Xiaolei Wang
2025-12-29 2:30 ` [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers Xiaolei Wang
2025-12-29 12:37 ` Tarang Raval
2025-12-29 13:01 ` Laurent Pinchart
2025-12-29 13:26 ` Tarang Raval
2025-12-31 2:57 ` xiaolei wang
2025-12-29 13:26 ` johannes.goede
2025-12-29 14:52 ` Tarang Raval
2025-12-29 2:30 ` [PATCH v2 2/3] media: i2c: ov5647: Switch to using the sub-device state lock Xiaolei Wang
2025-12-29 18:55 ` Sakari Ailus
2025-12-31 2:58 ` xiaolei wang
2025-12-29 2:30 ` [PATCH v2 3/3] media: i2c: ov5647: switch to {enable,disable}_streams Xiaolei Wang
2025-12-29 13:43 ` Laurent Pinchart
2025-12-31 3:02 ` xiaolei wang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.