* [PATCH v3 0/3] media: i2c: ov9282: Modernize driver with CCI and streams API
@ 2026-03-03 10:49 Xiaolei Wang
2026-03-03 10:49 ` [PATCH v3 1/3] media: i2c: ov9282: Convert to CCI register access helpers Xiaolei Wang
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Xiaolei Wang @ 2026-03-03 10:49 UTC (permalink / raw)
To: sakari.ailus, laurent.pinchart, tarang.raval, dave.stevenson,
jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco,
johannes.goede, hverkuil-cisco, jai.luthra, Xiaolei.Wang
Cc: linux-media, linux-kernel
This series modernizes the ov9282 driver by:
1. Converting to the common CCI (Camera Control Interface) register
access helpers, which simplifies the code by removing custom I2C
register access functions.
2. Switching to use the V4L2 sub-device state lock instead of a
private mutex, improving integration with the V4L2 framework.
3. Migrating from the legacy s_stream callback to the modern
enable_streams/disable_streams callbacks, providing better support
for multiplexed streams.
These changes reduce code complexity, improve maintainability, and
align the driver with current V4L2 best practices.
I verified each patch on the Raspberry Pi 5 platform.
Changes in V3:
- Patch 2:
* Remove unnecessary ret assignment in v4l2_subdev_init_finalize() error path
* Move pm_runtime_idle() call after v4l2_async_register_subdev_sensor() to avoid
potential double power-off in error handling path
- Link to v2: https://patchwork.linuxtv.org/project/linux-media/cover/20260301104809.3505257-1-xiaolei.wang@windriver.com/
Changes in V2:
- Patch 1: Fixed group hold release error handling in
ov9282_update_exp_gain()
- Patch 2: Fixed runtime PM cleanup in probe error path
- Patch 3: Improved error handling with clearer err_pm_put: label
- Link to V1: https://patchwork.linuxtv.org/project/linux-media/cover/20260228083401.1007434-1-xiaolei.wang@windriver.com/
Xiaolei Wang (3):
media: i2c: ov9282: Convert to CCI register access helpers
media: i2c: ov9282: Switch to using the sub-device state lock
media: i2c: ov9282: switch to {enable,disable}_streams
drivers/media/i2c/Kconfig | 1 +
drivers/media/i2c/ov9282.c | 409 +++++++++----------------------------
2 files changed, 102 insertions(+), 308 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v3 1/3] media: i2c: ov9282: Convert to CCI register access helpers 2026-03-03 10:49 [PATCH v3 0/3] media: i2c: ov9282: Modernize driver with CCI and streams API Xiaolei Wang @ 2026-03-03 10:49 ` Xiaolei Wang 2026-03-03 12:51 ` Dave Stevenson 2026-03-03 10:49 ` [PATCH v3 2/3] media: i2c: ov9282: Switch to using the sub-device state lock Xiaolei Wang 2026-03-03 10:49 ` [PATCH v3 3/3] media: i2c: ov9282: switch to {enable,disable}_streams Xiaolei Wang 2 siblings, 1 reply; 6+ messages in thread From: Xiaolei Wang @ 2026-03-03 10:49 UTC (permalink / raw) To: sakari.ailus, laurent.pinchart, tarang.raval, 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 ov9282 driver. This simplifies the driver by reducing the amount of code. Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io> --- drivers/media/i2c/Kconfig | 1 + drivers/media/i2c/ov9282.c | 293 ++++++++----------------------------- 2 files changed, 64 insertions(+), 230 deletions(-) diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 5eb1e0e0a87a..3027e71fd8fb 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -690,6 +690,7 @@ config VIDEO_OV8865 config VIDEO_OV9282 tristate "OmniVision OV9282 sensor support" depends on OF_GPIO + select V4L2_CCI_I2C help This is a Video4Linux2 sensor driver for the OmniVision OV9282 camera sensor. diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c index ded9b2044ff8..8bfaa3ae4be5 100644 --- a/drivers/media/i2c/ov9282.c +++ b/drivers/media/i2c/ov9282.c @@ -12,38 +12,40 @@ #include <linux/math.h> #include <linux/module.h> #include <linux/pm_runtime.h> +#include <linux/regmap.h> #include <linux/regulator/consumer.h> +#include <media/v4l2-cci.h> #include <media/v4l2-ctrls.h> #include <media/v4l2-event.h> #include <media/v4l2-fwnode.h> #include <media/v4l2-subdev.h> /* Streaming Mode */ -#define OV9282_REG_MODE_SELECT 0x0100 +#define OV9282_REG_MODE_SELECT CCI_REG8(0x0100) #define OV9282_MODE_STANDBY 0x00 #define OV9282_MODE_STREAMING 0x01 -#define OV9282_REG_PLL_CTRL_0D 0x030d +#define OV9282_REG_PLL_CTRL_0D CCI_REG8(0x030d) #define OV9282_PLL_CTRL_0D_RAW8 0x60 #define OV9282_PLL_CTRL_0D_RAW10 0x50 -#define OV9282_REG_TIMING_HTS 0x380c +#define OV9282_REG_TIMING_HTS CCI_REG16(0x380c) #define OV9282_TIMING_HTS_MAX 0x7fff /* Lines per frame */ -#define OV9282_REG_LPFR 0x380e +#define OV9282_REG_LPFR CCI_REG16(0x380e) /* Chip ID */ -#define OV9282_REG_ID 0x300a +#define OV9282_REG_ID CCI_REG16(0x300a) #define OV9282_ID 0x9281 /* Output enable registers */ -#define OV9282_REG_OUTPUT_ENABLE4 0x3004 +#define OV9282_REG_OUTPUT_ENABLE4 CCI_REG8(0x3004) #define OV9282_OUTPUT_ENABLE4_GPIO2 BIT(1) #define OV9282_OUTPUT_ENABLE4_D9 BIT(0) -#define OV9282_REG_OUTPUT_ENABLE5 0x3005 +#define OV9282_REG_OUTPUT_ENABLE5 CCI_REG8(0x3005) #define OV9282_OUTPUT_ENABLE5_D8 BIT(7) #define OV9282_OUTPUT_ENABLE5_D7 BIT(6) #define OV9282_OUTPUT_ENABLE5_D6 BIT(5) @@ -53,7 +55,7 @@ #define OV9282_OUTPUT_ENABLE5_D2 BIT(1) #define OV9282_OUTPUT_ENABLE5_D1 BIT(0) -#define OV9282_REG_OUTPUT_ENABLE6 0x3006 +#define OV9282_REG_OUTPUT_ENABLE6 CCI_REG8(0x3006) #define OV9282_OUTPUT_ENABLE6_D0 BIT(7) #define OV9282_OUTPUT_ENABLE6_PCLK BIT(6) #define OV9282_OUTPUT_ENABLE6_HREF BIT(5) @@ -62,14 +64,14 @@ #define OV9282_OUTPUT_ENABLE6_VSYNC BIT(1) /* Exposure control */ -#define OV9282_REG_EXPOSURE 0x3500 +#define OV9282_REG_EXPOSURE CCI_REG24(0x3500) #define OV9282_EXPOSURE_MIN 1 #define OV9282_EXPOSURE_OFFSET 25 #define OV9282_EXPOSURE_STEP 1 #define OV9282_EXPOSURE_DEFAULT 0x0282 /* AEC/AGC manual */ -#define OV9282_REG_AEC_MANUAL 0x3503 +#define OV9282_REG_AEC_MANUAL CCI_REG8(0x3503) #define OV9282_DIGFRAC_GAIN_DELAY BIT(6) #define OV9282_GAIN_CHANGE_DELAY BIT(5) #define OV9282_GAIN_DELAY BIT(4) @@ -78,28 +80,28 @@ #define OV9282_AEC_MANUAL_DEFAULT 0x00 /* Analog gain control */ -#define OV9282_REG_AGAIN 0x3509 +#define OV9282_REG_AGAIN CCI_REG8(0x3509) #define OV9282_AGAIN_MIN 0x10 #define OV9282_AGAIN_MAX 0xff #define OV9282_AGAIN_STEP 1 #define OV9282_AGAIN_DEFAULT 0x10 /* Group hold register */ -#define OV9282_REG_HOLD 0x3308 +#define OV9282_REG_HOLD CCI_REG8(0x3308) -#define OV9282_REG_ANA_CORE_2 0x3662 +#define OV9282_REG_ANA_CORE_2 CCI_REG8(0x3662) #define OV9282_ANA_CORE2_RAW8 0x07 #define OV9282_ANA_CORE2_RAW10 0x05 -#define OV9282_REG_TIMING_FORMAT_1 0x3820 -#define OV9282_REG_TIMING_FORMAT_2 0x3821 +#define OV9282_REG_TIMING_FORMAT_1 CCI_REG8(0x3820) +#define OV9282_REG_TIMING_FORMAT_2 CCI_REG8(0x3821) #define OV9282_FLIP_BIT BIT(2) -#define OV9282_REG_MIPI_CTRL00 0x4800 +#define OV9282_REG_MIPI_CTRL00 CCI_REG8(0x4800) #define OV9282_GATED_CLOCK BIT(5) /* Flash/Strobe control registers */ -#define OV9282_REG_STROBE_FRAME_SPAN 0x3925 +#define OV9282_REG_STROBE_FRAME_SPAN CCI_REG32(0x3925) #define OV9282_STROBE_FRAME_SPAN_DEFAULT 0x0000001a /* Input clock rate */ @@ -139,16 +141,6 @@ static const char * const ov9282_supply_names[] = { #define OV9282_NUM_SUPPLIES ARRAY_SIZE(ov9282_supply_names) -/** - * struct ov9282_reg - ov9282 sensor register - * @address: Register address - * @val: Register value - */ -struct ov9282_reg { - u16 address; - u8 val; -}; - /** * struct ov9282_reg_list - ov9282 sensor register list * @num_of_regs: Number of registers in the list @@ -156,7 +148,7 @@ struct ov9282_reg { */ struct ov9282_reg_list { u32 num_of_regs; - const struct ov9282_reg *regs; + const struct reg_sequence *regs; }; /** @@ -188,6 +180,7 @@ struct ov9282_mode { * struct ov9282 - ov9282 sensor device structure * @dev: Pointer to generic device * @sd: V4L2 sub-device + * @regmap: Regmap for sensor register access * @pad: Media pad. Only one pad supported * @reset_gpio: Sensor reset gpio * @inclk: Sensor input clock @@ -209,6 +202,7 @@ struct ov9282_mode { struct ov9282 { struct device *dev; struct v4l2_subdev sd; + struct regmap *regmap; struct media_pad pad; struct gpio_desc *reset_gpio; struct clk *inclk; @@ -241,7 +235,7 @@ static const s64 link_freq[] = { * register arrays as some settings are written as part of ov9282_power_on, * and the reset will clear them. */ -static const struct ov9282_reg common_regs[] = { +static const struct reg_sequence common_regs[] = { {0x0302, 0x32}, {0x030e, 0x02}, {0x3001, 0x00}, @@ -305,11 +299,6 @@ static const struct ov9282_reg common_regs[] = { {0x5a08, 0x84}, }; -static struct ov9282_reg_list common_regs_list = { - .num_of_regs = ARRAY_SIZE(common_regs), - .regs = common_regs, -}; - #define MODE_1280_800 0 #define MODE_1280_720 1 #define MODE_640_400 2 @@ -317,7 +306,7 @@ static struct ov9282_reg_list common_regs_list = { #define DEFAULT_MODE MODE_1280_720 /* Sensor mode registers */ -static const struct ov9282_reg mode_1280x800_regs[] = { +static const struct reg_sequence mode_1280x800_regs[] = { {0x3778, 0x00}, {0x3800, 0x00}, {0x3801, 0x00}, @@ -348,7 +337,7 @@ static const struct ov9282_reg mode_1280x800_regs[] = { {0x4509, 0x00}, }; -static const struct ov9282_reg mode_1280x720_regs[] = { +static const struct reg_sequence mode_1280x720_regs[] = { {0x3778, 0x00}, {0x3800, 0x00}, {0x3801, 0x00}, @@ -379,7 +368,7 @@ static const struct ov9282_reg mode_1280x720_regs[] = { {0x4509, 0x80}, }; -static const struct ov9282_reg mode_640x400_regs[] = { +static const struct reg_sequence mode_640x400_regs[] = { {0x3778, 0x10}, {0x3800, 0x00}, {0x3801, 0x00}, @@ -485,97 +474,6 @@ static inline struct ov9282 *to_ov9282(struct v4l2_subdev *subdev) return container_of(subdev, struct ov9282, sd); } -/** - * ov9282_read_reg() - Read registers. - * @ov9282: pointer to ov9282 device - * @reg: register address - * @len: length of bytes to read. Max supported bytes is 4 - * @val: pointer to register value to be filled. - * - * Return: 0 if successful, error code otherwise. - */ -static int ov9282_read_reg(struct ov9282 *ov9282, u16 reg, u32 len, u32 *val) -{ - struct i2c_client *client = v4l2_get_subdevdata(&ov9282->sd); - struct i2c_msg msgs[2] = {0}; - u8 addr_buf[2] = {0}; - u8 data_buf[4] = {0}; - int ret; - - if (WARN_ON(len > 4)) - return -EINVAL; - - put_unaligned_be16(reg, addr_buf); - - /* Write register address */ - msgs[0].addr = client->addr; - msgs[0].flags = 0; - msgs[0].len = ARRAY_SIZE(addr_buf); - msgs[0].buf = addr_buf; - - /* Read data from register */ - msgs[1].addr = client->addr; - msgs[1].flags = I2C_M_RD; - msgs[1].len = len; - msgs[1].buf = &data_buf[4 - len]; - - ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); - if (ret != ARRAY_SIZE(msgs)) - return -EIO; - - *val = get_unaligned_be32(data_buf); - - return 0; -} - -/** - * ov9282_write_reg() - Write register - * @ov9282: pointer to ov9282 device - * @reg: register address - * @len: length of bytes. Max supported bytes is 4 - * @val: register value - * - * Return: 0 if successful, error code otherwise. - */ -static int ov9282_write_reg(struct ov9282 *ov9282, u16 reg, u32 len, u32 val) -{ - struct i2c_client *client = v4l2_get_subdevdata(&ov9282->sd); - u8 buf[6] = {0}; - - if (WARN_ON(len > 4)) - return -EINVAL; - - put_unaligned_be16(reg, buf); - put_unaligned_be32(val << (8 * (4 - len)), buf + 2); - if (i2c_master_send(client, buf, len + 2) != len + 2) - return -EIO; - - return 0; -} - -/** - * ov9282_write_regs() - Write a list of registers - * @ov9282: pointer to ov9282 device - * @regs: list of registers to be written - * @len: length of registers array - * - * Return: 0 if successful, error code otherwise. - */ -static int ov9282_write_regs(struct ov9282 *ov9282, - const struct ov9282_reg *regs, u32 len) -{ - unsigned int i; - int ret; - - for (i = 0; i < len; i++) { - ret = ov9282_write_reg(ov9282, regs[i].address, 1, regs[i].val); - if (ret) - return ret; - } - - return 0; -} - /** * ov9282_update_controls() - Update control ranges based on streaming mode * @ov9282: pointer to ov9282 device @@ -639,15 +537,15 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain) dev_dbg(ov9282->dev, "Set exp %u (~%u us), analog gain %u", exposure, exposure_us, gain); - ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1); + ret = cci_write(ov9282->regmap, OV9282_REG_HOLD, 0x01, NULL); if (ret) return ret; - ret = ov9282_write_reg(ov9282, OV9282_REG_EXPOSURE, 3, exposure << 4); + ret = cci_write(ov9282->regmap, OV9282_REG_EXPOSURE, exposure << 4, NULL); if (ret) goto error_release_group_hold; - ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain); + ret = cci_write(ov9282->regmap, OV9282_REG_AGAIN, gain, NULL); if (ret) goto error_release_group_hold; @@ -656,60 +554,9 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain) OV9282_STROBE_FRAME_SPAN_DEFAULT); error_release_group_hold: - ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0); - - return ret; -} - -static int ov9282_set_ctrl_hflip(struct ov9282 *ov9282, int value) -{ - u32 current_val; - int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1, - ¤t_val); - if (ret) - return ret; + int ret_hold = cci_write(ov9282->regmap, OV9282_REG_HOLD, 0, NULL); - if (value) - current_val |= OV9282_FLIP_BIT; - else - current_val &= ~OV9282_FLIP_BIT; - - return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1, - current_val); -} - -static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value) -{ - u32 current_val; - int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1, - ¤t_val); - if (ret) - return ret; - - if (value) - current_val |= OV9282_FLIP_BIT; - else - current_val &= ~OV9282_FLIP_BIT; - - return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1, - current_val); -} - -static int ov9282_set_ctrl_flash_strobe_oe(struct ov9282 *ov9282, bool enable) -{ - u32 current_val; - int ret; - - ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1, ¤t_val); - if (ret) - return ret; - - if (enable) - current_val |= OV9282_OUTPUT_ENABLE6_STROBE; - else - current_val &= ~OV9282_OUTPUT_ENABLE6_STROBE; - - return ov9282_write_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1, current_val); + return ret ? ret : ret_hold; } static u32 ov9282_us_to_flash_duration(struct ov9282 *ov9282, u32 value) @@ -740,30 +587,6 @@ static u32 ov9282_flash_duration_to_us(struct ov9282 *ov9282, u32 value) return DIV_ROUND_UP(value * frame_width, OV9282_STROBE_SPAN_FACTOR); } -static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value) -{ - u32 val = ov9282_us_to_flash_duration(ov9282, value); - int ret; - - ret = ov9282_write_reg(ov9282, OV9282_REG_STROBE_FRAME_SPAN, 1, - (val >> 24) & 0xff); - if (ret) - return ret; - - ret = ov9282_write_reg(ov9282, OV9282_REG_STROBE_FRAME_SPAN + 1, 1, - (val >> 16) & 0xff); - if (ret) - return ret; - - ret = ov9282_write_reg(ov9282, OV9282_REG_STROBE_FRAME_SPAN + 2, 1, - (val >> 8) & 0xff); - if (ret) - return ret; - - return ov9282_write_reg(ov9282, OV9282_REG_STROBE_FRAME_SPAN + 3, 1, - val & 0xff); -} - /** * ov9282_set_ctrl() - Set subdevice control * @ctrl: pointer to v4l2_ctrl structure @@ -818,23 +641,27 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl) break; case V4L2_CID_VBLANK: lpfr = ov9282->vblank + ov9282->cur_mode->height; - ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr); + ret = cci_write(ov9282->regmap, OV9282_REG_LPFR, lpfr, NULL); break; case V4L2_CID_HFLIP: - ret = ov9282_set_ctrl_hflip(ov9282, ctrl->val); + ret = cci_update_bits(ov9282->regmap, OV9282_REG_TIMING_FORMAT_2, + OV9282_FLIP_BIT, ctrl->val ? OV9282_FLIP_BIT : 0, NULL); break; case V4L2_CID_VFLIP: - ret = ov9282_set_ctrl_vflip(ov9282, ctrl->val); + ret = cci_update_bits(ov9282->regmap, OV9282_REG_TIMING_FORMAT_1, + OV9282_FLIP_BIT, ctrl->val ? OV9282_FLIP_BIT : 0, NULL); break; case V4L2_CID_HBLANK: - ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2, - (ctrl->val + ov9282->cur_mode->width) >> 1); + ret = cci_write(ov9282->regmap, OV9282_REG_TIMING_HTS, + (ctrl->val + ov9282->cur_mode->width) >> 1, NULL); break; case V4L2_CID_FLASH_STROBE_OE: - ret = ov9282_set_ctrl_flash_strobe_oe(ov9282, ctrl->val); + ret = cci_update_bits(ov9282->regmap, OV9282_REG_OUTPUT_ENABLE6, + OV9282_OUTPUT_ENABLE6_STROBE, + ctrl->val ? OV9282_OUTPUT_ENABLE6_STROBE : 0, NULL); break; case V4L2_CID_FLASH_DURATION: - ret = ov9282_set_ctrl_flash_duration(ov9282, ctrl->val); + ret = cci_write(ov9282->regmap, OV9282_REG_STROBE_FRAME_SPAN, ctrl->val, NULL); break; default: dev_err(ov9282->dev, "Invalid control %d", ctrl->id); @@ -1114,7 +941,7 @@ static int ov9282_get_selection(struct v4l2_subdev *sd, */ static int ov9282_start_streaming(struct ov9282 *ov9282) { - const struct ov9282_reg bitdepth_regs[2][2] = { + const struct reg_sequence bitdepth_regs[2][2] = { { {OV9282_REG_PLL_CTRL_0D, OV9282_PLL_CTRL_0D_RAW10}, {OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW10}, @@ -1128,15 +955,16 @@ static int ov9282_start_streaming(struct ov9282 *ov9282) int ret; /* Write common registers */ - ret = ov9282_write_regs(ov9282, common_regs_list.regs, - common_regs_list.num_of_regs); + ret = regmap_multi_reg_write(ov9282->regmap, common_regs, + ARRAY_SIZE(common_regs)); if (ret) { dev_err(ov9282->dev, "fail to write common registers"); return ret; } bitdepth_index = ov9282->code == MEDIA_BUS_FMT_Y10_1X10 ? 0 : 1; - ret = ov9282_write_regs(ov9282, bitdepth_regs[bitdepth_index], 2); + ret = regmap_multi_reg_write(ov9282->regmap, + bitdepth_regs[bitdepth_index], 2); if (ret) { dev_err(ov9282->dev, "fail to write bitdepth regs"); return ret; @@ -1144,7 +972,8 @@ static int ov9282_start_streaming(struct ov9282 *ov9282) /* Write sensor mode registers */ reg_list = &ov9282->cur_mode->reg_list; - ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list->num_of_regs); + ret = regmap_multi_reg_write(ov9282->regmap, reg_list->regs, + reg_list->num_of_regs); if (ret) { dev_err(ov9282->dev, "fail to write initial registers"); return ret; @@ -1158,8 +987,8 @@ static int ov9282_start_streaming(struct ov9282 *ov9282) } /* Start streaming */ - ret = ov9282_write_reg(ov9282, OV9282_REG_MODE_SELECT, - 1, OV9282_MODE_STREAMING); + ret = cci_write(ov9282->regmap, OV9282_REG_MODE_SELECT, + OV9282_MODE_STREAMING, NULL); if (ret) { dev_err(ov9282->dev, "fail to start streaming"); return ret; @@ -1176,8 +1005,8 @@ static int ov9282_start_streaming(struct ov9282 *ov9282) */ static int ov9282_stop_streaming(struct ov9282 *ov9282) { - return ov9282_write_reg(ov9282, OV9282_REG_MODE_SELECT, - 1, OV9282_MODE_STANDBY); + return cci_write(ov9282->regmap, OV9282_REG_MODE_SELECT, + OV9282_MODE_STANDBY, NULL); } /** @@ -1228,14 +1057,14 @@ static int ov9282_set_stream(struct v4l2_subdev *sd, int enable) static int ov9282_detect(struct ov9282 *ov9282) { int ret; - u32 val; + u64 val; - ret = ov9282_read_reg(ov9282, OV9282_REG_ID, 2, &val); + ret = cci_read(ov9282->regmap, OV9282_REG_ID, &val, NULL); if (ret) return ret; if (val != OV9282_ID) { - dev_err(ov9282->dev, "chip id mismatch: %x!=%x", + dev_err(ov9282->dev, "chip id mismatch: %x!=%llx", OV9282_ID, val); return -ENXIO; } @@ -1397,9 +1226,8 @@ static int ov9282_power_on(struct device *dev) usleep_range(400, 600); - ret = ov9282_write_reg(ov9282, OV9282_REG_MIPI_CTRL00, 1, - ov9282->noncontinuous_clock ? - OV9282_GATED_CLOCK : 0); + ret = cci_write(ov9282->regmap, OV9282_REG_MIPI_CTRL00, + ov9282->noncontinuous_clock ? OV9282_GATED_CLOCK : 0, NULL); if (ret) { dev_err(ov9282->dev, "fail to write MIPI_CTRL00"); goto error_clk; @@ -1576,6 +1404,11 @@ static int ov9282_probe(struct i2c_client *client) return ret; } + ov9282->regmap = devm_cci_regmap_init_i2c(client, 16); + if (IS_ERR(ov9282->regmap)) + return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->regmap), + "Failed to init CCI\n"); + mutex_init(&ov9282->mutex); ret = ov9282_power_on(ov9282->dev); -- 2.43.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/3] media: i2c: ov9282: Convert to CCI register access helpers 2026-03-03 10:49 ` [PATCH v3 1/3] media: i2c: ov9282: Convert to CCI register access helpers Xiaolei Wang @ 2026-03-03 12:51 ` Dave Stevenson 2026-03-03 22:19 ` xiaolei wang 0 siblings, 1 reply; 6+ messages in thread From: Dave Stevenson @ 2026-03-03 12:51 UTC (permalink / raw) To: Xiaolei Wang Cc: sakari.ailus, laurent.pinchart, tarang.raval, jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco, johannes.goede, hverkuil-cisco, jai.luthra, linux-media, linux-kernel Hi Xiaolei Thanks for the patch On Tue, 3 Mar 2026 at 10:50, Xiaolei Wang <xiaolei.wang@windriver.com> wrote: > > Use the new common CCI register access helpers to replace the private > register access helpers in the ov9282 driver. This simplifies the driver > by reducing the amount of code. > > Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> > Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io> > --- > drivers/media/i2c/Kconfig | 1 + > drivers/media/i2c/ov9282.c | 293 ++++++++----------------------------- > 2 files changed, 64 insertions(+), 230 deletions(-) > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 5eb1e0e0a87a..3027e71fd8fb 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -690,6 +690,7 @@ config VIDEO_OV8865 > config VIDEO_OV9282 > tristate "OmniVision OV9282 sensor support" > depends on OF_GPIO > + select V4L2_CCI_I2C > help > This is a Video4Linux2 sensor driver for the OmniVision > OV9282 camera sensor. > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c > index ded9b2044ff8..8bfaa3ae4be5 100644 > --- a/drivers/media/i2c/ov9282.c > +++ b/drivers/media/i2c/ov9282.c > @@ -12,38 +12,40 @@ > #include <linux/math.h> > #include <linux/module.h> > #include <linux/pm_runtime.h> > +#include <linux/regmap.h> > #include <linux/regulator/consumer.h> > > +#include <media/v4l2-cci.h> > #include <media/v4l2-ctrls.h> > #include <media/v4l2-event.h> > #include <media/v4l2-fwnode.h> > #include <media/v4l2-subdev.h> > > /* Streaming Mode */ > -#define OV9282_REG_MODE_SELECT 0x0100 > +#define OV9282_REG_MODE_SELECT CCI_REG8(0x0100) > #define OV9282_MODE_STANDBY 0x00 > #define OV9282_MODE_STREAMING 0x01 > > -#define OV9282_REG_PLL_CTRL_0D 0x030d > +#define OV9282_REG_PLL_CTRL_0D CCI_REG8(0x030d) > #define OV9282_PLL_CTRL_0D_RAW8 0x60 > #define OV9282_PLL_CTRL_0D_RAW10 0x50 > > -#define OV9282_REG_TIMING_HTS 0x380c > +#define OV9282_REG_TIMING_HTS CCI_REG16(0x380c) > #define OV9282_TIMING_HTS_MAX 0x7fff > > /* Lines per frame */ > -#define OV9282_REG_LPFR 0x380e > +#define OV9282_REG_LPFR CCI_REG16(0x380e) > > /* Chip ID */ > -#define OV9282_REG_ID 0x300a > +#define OV9282_REG_ID CCI_REG16(0x300a) > #define OV9282_ID 0x9281 > > /* Output enable registers */ > -#define OV9282_REG_OUTPUT_ENABLE4 0x3004 > +#define OV9282_REG_OUTPUT_ENABLE4 CCI_REG8(0x3004) > #define OV9282_OUTPUT_ENABLE4_GPIO2 BIT(1) > #define OV9282_OUTPUT_ENABLE4_D9 BIT(0) > > -#define OV9282_REG_OUTPUT_ENABLE5 0x3005 > +#define OV9282_REG_OUTPUT_ENABLE5 CCI_REG8(0x3005) > #define OV9282_OUTPUT_ENABLE5_D8 BIT(7) > #define OV9282_OUTPUT_ENABLE5_D7 BIT(6) > #define OV9282_OUTPUT_ENABLE5_D6 BIT(5) > @@ -53,7 +55,7 @@ > #define OV9282_OUTPUT_ENABLE5_D2 BIT(1) > #define OV9282_OUTPUT_ENABLE5_D1 BIT(0) > > -#define OV9282_REG_OUTPUT_ENABLE6 0x3006 > +#define OV9282_REG_OUTPUT_ENABLE6 CCI_REG8(0x3006) > #define OV9282_OUTPUT_ENABLE6_D0 BIT(7) > #define OV9282_OUTPUT_ENABLE6_PCLK BIT(6) > #define OV9282_OUTPUT_ENABLE6_HREF BIT(5) > @@ -62,14 +64,14 @@ > #define OV9282_OUTPUT_ENABLE6_VSYNC BIT(1) > > /* Exposure control */ > -#define OV9282_REG_EXPOSURE 0x3500 > +#define OV9282_REG_EXPOSURE CCI_REG24(0x3500) > #define OV9282_EXPOSURE_MIN 1 > #define OV9282_EXPOSURE_OFFSET 25 > #define OV9282_EXPOSURE_STEP 1 > #define OV9282_EXPOSURE_DEFAULT 0x0282 > > /* AEC/AGC manual */ > -#define OV9282_REG_AEC_MANUAL 0x3503 > +#define OV9282_REG_AEC_MANUAL CCI_REG8(0x3503) > #define OV9282_DIGFRAC_GAIN_DELAY BIT(6) > #define OV9282_GAIN_CHANGE_DELAY BIT(5) > #define OV9282_GAIN_DELAY BIT(4) > @@ -78,28 +80,28 @@ > #define OV9282_AEC_MANUAL_DEFAULT 0x00 > > /* Analog gain control */ > -#define OV9282_REG_AGAIN 0x3509 > +#define OV9282_REG_AGAIN CCI_REG8(0x3509) > #define OV9282_AGAIN_MIN 0x10 > #define OV9282_AGAIN_MAX 0xff > #define OV9282_AGAIN_STEP 1 > #define OV9282_AGAIN_DEFAULT 0x10 > > /* Group hold register */ > -#define OV9282_REG_HOLD 0x3308 > +#define OV9282_REG_HOLD CCI_REG8(0x3308) > > -#define OV9282_REG_ANA_CORE_2 0x3662 > +#define OV9282_REG_ANA_CORE_2 CCI_REG8(0x3662) > #define OV9282_ANA_CORE2_RAW8 0x07 > #define OV9282_ANA_CORE2_RAW10 0x05 > > -#define OV9282_REG_TIMING_FORMAT_1 0x3820 > -#define OV9282_REG_TIMING_FORMAT_2 0x3821 > +#define OV9282_REG_TIMING_FORMAT_1 CCI_REG8(0x3820) > +#define OV9282_REG_TIMING_FORMAT_2 CCI_REG8(0x3821) > #define OV9282_FLIP_BIT BIT(2) > > -#define OV9282_REG_MIPI_CTRL00 0x4800 > +#define OV9282_REG_MIPI_CTRL00 CCI_REG8(0x4800) > #define OV9282_GATED_CLOCK BIT(5) > > /* Flash/Strobe control registers */ > -#define OV9282_REG_STROBE_FRAME_SPAN 0x3925 > +#define OV9282_REG_STROBE_FRAME_SPAN CCI_REG32(0x3925) > #define OV9282_STROBE_FRAME_SPAN_DEFAULT 0x0000001a > > /* Input clock rate */ > @@ -139,16 +141,6 @@ static const char * const ov9282_supply_names[] = { > > #define OV9282_NUM_SUPPLIES ARRAY_SIZE(ov9282_supply_names) > > -/** > - * struct ov9282_reg - ov9282 sensor register > - * @address: Register address > - * @val: Register value > - */ > -struct ov9282_reg { > - u16 address; > - u8 val; > -}; > - > /** > * struct ov9282_reg_list - ov9282 sensor register list > * @num_of_regs: Number of registers in the list > @@ -156,7 +148,7 @@ struct ov9282_reg { > */ > struct ov9282_reg_list { > u32 num_of_regs; > - const struct ov9282_reg *regs; > + const struct reg_sequence *regs; > }; > > /** > @@ -188,6 +180,7 @@ struct ov9282_mode { > * struct ov9282 - ov9282 sensor device structure > * @dev: Pointer to generic device > * @sd: V4L2 sub-device > + * @regmap: Regmap for sensor register access > * @pad: Media pad. Only one pad supported > * @reset_gpio: Sensor reset gpio > * @inclk: Sensor input clock > @@ -209,6 +202,7 @@ struct ov9282_mode { > struct ov9282 { > struct device *dev; > struct v4l2_subdev sd; > + struct regmap *regmap; > struct media_pad pad; > struct gpio_desc *reset_gpio; > struct clk *inclk; > @@ -241,7 +235,7 @@ static const s64 link_freq[] = { > * register arrays as some settings are written as part of ov9282_power_on, > * and the reset will clear them. > */ > -static const struct ov9282_reg common_regs[] = { > +static const struct reg_sequence common_regs[] = { > {0x0302, 0x32}, > {0x030e, 0x02}, > {0x3001, 0x00}, > @@ -305,11 +299,6 @@ static const struct ov9282_reg common_regs[] = { > {0x5a08, 0x84}, > }; > > -static struct ov9282_reg_list common_regs_list = { > - .num_of_regs = ARRAY_SIZE(common_regs), > - .regs = common_regs, > -}; > - > #define MODE_1280_800 0 > #define MODE_1280_720 1 > #define MODE_640_400 2 > @@ -317,7 +306,7 @@ static struct ov9282_reg_list common_regs_list = { > #define DEFAULT_MODE MODE_1280_720 > > /* Sensor mode registers */ > -static const struct ov9282_reg mode_1280x800_regs[] = { > +static const struct reg_sequence mode_1280x800_regs[] = { > {0x3778, 0x00}, > {0x3800, 0x00}, > {0x3801, 0x00}, You changed OV9282_REG_TIMING_FORMAT_[12] above to CCI_REG8(0x382[01]). However it is used in this array of type reg_sequence, but all the other values are still using non-CCI_REGx register writes here. If converting to CCI_REGx then you at least need to be consistent. Personally I'd say do it everywhere and use cci_multi_reg_write instead of regmap_multi_reg_write. > @@ -348,7 +337,7 @@ static const struct ov9282_reg mode_1280x800_regs[] = { > {0x4509, 0x00}, > }; > > -static const struct ov9282_reg mode_1280x720_regs[] = { > +static const struct reg_sequence mode_1280x720_regs[] = { > {0x3778, 0x00}, > {0x3800, 0x00}, > {0x3801, 0x00}, > @@ -379,7 +368,7 @@ static const struct ov9282_reg mode_1280x720_regs[] = { > {0x4509, 0x80}, > }; > > -static const struct ov9282_reg mode_640x400_regs[] = { > +static const struct reg_sequence mode_640x400_regs[] = { > {0x3778, 0x10}, > {0x3800, 0x00}, > {0x3801, 0x00}, > @@ -485,97 +474,6 @@ static inline struct ov9282 *to_ov9282(struct v4l2_subdev *subdev) > return container_of(subdev, struct ov9282, sd); > } > > -/** > - * ov9282_read_reg() - Read registers. > - * @ov9282: pointer to ov9282 device > - * @reg: register address > - * @len: length of bytes to read. Max supported bytes is 4 > - * @val: pointer to register value to be filled. > - * > - * Return: 0 if successful, error code otherwise. > - */ > -static int ov9282_read_reg(struct ov9282 *ov9282, u16 reg, u32 len, u32 *val) > -{ > - struct i2c_client *client = v4l2_get_subdevdata(&ov9282->sd); > - struct i2c_msg msgs[2] = {0}; > - u8 addr_buf[2] = {0}; > - u8 data_buf[4] = {0}; > - int ret; > - > - if (WARN_ON(len > 4)) > - return -EINVAL; > - > - put_unaligned_be16(reg, addr_buf); > - > - /* Write register address */ > - msgs[0].addr = client->addr; > - msgs[0].flags = 0; > - msgs[0].len = ARRAY_SIZE(addr_buf); > - msgs[0].buf = addr_buf; > - > - /* Read data from register */ > - msgs[1].addr = client->addr; > - msgs[1].flags = I2C_M_RD; > - msgs[1].len = len; > - msgs[1].buf = &data_buf[4 - len]; > - > - ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > - if (ret != ARRAY_SIZE(msgs)) > - return -EIO; > - > - *val = get_unaligned_be32(data_buf); > - > - return 0; > -} > - > -/** > - * ov9282_write_reg() - Write register > - * @ov9282: pointer to ov9282 device > - * @reg: register address > - * @len: length of bytes. Max supported bytes is 4 > - * @val: register value > - * > - * Return: 0 if successful, error code otherwise. > - */ > -static int ov9282_write_reg(struct ov9282 *ov9282, u16 reg, u32 len, u32 val) > -{ > - struct i2c_client *client = v4l2_get_subdevdata(&ov9282->sd); > - u8 buf[6] = {0}; > - > - if (WARN_ON(len > 4)) > - return -EINVAL; > - > - put_unaligned_be16(reg, buf); > - put_unaligned_be32(val << (8 * (4 - len)), buf + 2); > - if (i2c_master_send(client, buf, len + 2) != len + 2) > - return -EIO; > - > - return 0; > -} > - > -/** > - * ov9282_write_regs() - Write a list of registers > - * @ov9282: pointer to ov9282 device > - * @regs: list of registers to be written > - * @len: length of registers array > - * > - * Return: 0 if successful, error code otherwise. > - */ > -static int ov9282_write_regs(struct ov9282 *ov9282, > - const struct ov9282_reg *regs, u32 len) > -{ > - unsigned int i; > - int ret; > - > - for (i = 0; i < len; i++) { > - ret = ov9282_write_reg(ov9282, regs[i].address, 1, regs[i].val); > - if (ret) > - return ret; > - } > - > - return 0; > -} > - > /** > * ov9282_update_controls() - Update control ranges based on streaming mode > * @ov9282: pointer to ov9282 device > @@ -639,15 +537,15 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain) > dev_dbg(ov9282->dev, "Set exp %u (~%u us), analog gain %u", > exposure, exposure_us, gain); > > - ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1); > + ret = cci_write(ov9282->regmap, OV9282_REG_HOLD, 0x01, NULL); > if (ret) > return ret; > > - ret = ov9282_write_reg(ov9282, OV9282_REG_EXPOSURE, 3, exposure << 4); > + ret = cci_write(ov9282->regmap, OV9282_REG_EXPOSURE, exposure << 4, NULL); > if (ret) > goto error_release_group_hold; > > - ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain); > + ret = cci_write(ov9282->regmap, OV9282_REG_AGAIN, gain, NULL); > if (ret) > goto error_release_group_hold; > > @@ -656,60 +554,9 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain) > OV9282_STROBE_FRAME_SPAN_DEFAULT); > > error_release_group_hold: > - ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0); > - > - return ret; > -} > - > -static int ov9282_set_ctrl_hflip(struct ov9282 *ov9282, int value) > -{ > - u32 current_val; > - int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1, > - ¤t_val); > - if (ret) > - return ret; > + int ret_hold = cci_write(ov9282->regmap, OV9282_REG_HOLD, 0, NULL); > > - if (value) > - current_val |= OV9282_FLIP_BIT; > - else > - current_val &= ~OV9282_FLIP_BIT; > - > - return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1, > - current_val); > -} > - > -static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value) > -{ > - u32 current_val; > - int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1, > - ¤t_val); > - if (ret) > - return ret; > - > - if (value) > - current_val |= OV9282_FLIP_BIT; > - else > - current_val &= ~OV9282_FLIP_BIT; > - > - return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1, > - current_val); > -} > - > -static int ov9282_set_ctrl_flash_strobe_oe(struct ov9282 *ov9282, bool enable) > -{ > - u32 current_val; > - int ret; > - > - ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1, ¤t_val); > - if (ret) > - return ret; > - > - if (enable) > - current_val |= OV9282_OUTPUT_ENABLE6_STROBE; > - else > - current_val &= ~OV9282_OUTPUT_ENABLE6_STROBE; > - > - return ov9282_write_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1, current_val); > + return ret ? ret : ret_hold; > } > > static u32 ov9282_us_to_flash_duration(struct ov9282 *ov9282, u32 value) > @@ -740,30 +587,6 @@ static u32 ov9282_flash_duration_to_us(struct ov9282 *ov9282, u32 value) > return DIV_ROUND_UP(value * frame_width, OV9282_STROBE_SPAN_FACTOR); > } > > -static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value) > -{ > - u32 val = ov9282_us_to_flash_duration(ov9282, value); > - int ret; > - > - ret = ov9282_write_reg(ov9282, OV9282_REG_STROBE_FRAME_SPAN, 1, > - (val >> 24) & 0xff); > - if (ret) > - return ret; > - > - ret = ov9282_write_reg(ov9282, OV9282_REG_STROBE_FRAME_SPAN + 1, 1, > - (val >> 16) & 0xff); > - if (ret) > - return ret; > - > - ret = ov9282_write_reg(ov9282, OV9282_REG_STROBE_FRAME_SPAN + 2, 1, > - (val >> 8) & 0xff); > - if (ret) > - return ret; > - > - return ov9282_write_reg(ov9282, OV9282_REG_STROBE_FRAME_SPAN + 3, 1, > - val & 0xff); > -} > - > /** > * ov9282_set_ctrl() - Set subdevice control > * @ctrl: pointer to v4l2_ctrl structure > @@ -818,23 +641,27 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl) > break; > case V4L2_CID_VBLANK: > lpfr = ov9282->vblank + ov9282->cur_mode->height; > - ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr); > + ret = cci_write(ov9282->regmap, OV9282_REG_LPFR, lpfr, NULL); > break; > case V4L2_CID_HFLIP: > - ret = ov9282_set_ctrl_hflip(ov9282, ctrl->val); > + ret = cci_update_bits(ov9282->regmap, OV9282_REG_TIMING_FORMAT_2, > + OV9282_FLIP_BIT, ctrl->val ? OV9282_FLIP_BIT : 0, NULL); > break; > case V4L2_CID_VFLIP: > - ret = ov9282_set_ctrl_vflip(ov9282, ctrl->val); > + ret = cci_update_bits(ov9282->regmap, OV9282_REG_TIMING_FORMAT_1, > + OV9282_FLIP_BIT, ctrl->val ? OV9282_FLIP_BIT : 0, NULL); > break; > case V4L2_CID_HBLANK: > - ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2, > - (ctrl->val + ov9282->cur_mode->width) >> 1); > + ret = cci_write(ov9282->regmap, OV9282_REG_TIMING_HTS, > + (ctrl->val + ov9282->cur_mode->width) >> 1, NULL); > break; > case V4L2_CID_FLASH_STROBE_OE: > - ret = ov9282_set_ctrl_flash_strobe_oe(ov9282, ctrl->val); > + ret = cci_update_bits(ov9282->regmap, OV9282_REG_OUTPUT_ENABLE6, > + OV9282_OUTPUT_ENABLE6_STROBE, > + ctrl->val ? OV9282_OUTPUT_ENABLE6_STROBE : 0, NULL); > break; > case V4L2_CID_FLASH_DURATION: > - ret = ov9282_set_ctrl_flash_duration(ov9282, ctrl->val); > + ret = cci_write(ov9282->regmap, OV9282_REG_STROBE_FRAME_SPAN, ctrl->val, NULL); > break; > default: > dev_err(ov9282->dev, "Invalid control %d", ctrl->id); > @@ -1114,7 +941,7 @@ static int ov9282_get_selection(struct v4l2_subdev *sd, > */ > static int ov9282_start_streaming(struct ov9282 *ov9282) > { > - const struct ov9282_reg bitdepth_regs[2][2] = { > + const struct reg_sequence bitdepth_regs[2][2] = { > { > {OV9282_REG_PLL_CTRL_0D, OV9282_PLL_CTRL_0D_RAW10}, > {OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW10}, Here is a more obvious example. You have CCI_REGx() register defines being stored into a reg_sequence. That's an obvious one for the array to be struct cci_reg_sequence and use cci_multi_reg_write(). Otherwise the patch looks reasonable. Dave > @@ -1128,15 +955,16 @@ static int ov9282_start_streaming(struct ov9282 *ov9282) > int ret; > > /* Write common registers */ > - ret = ov9282_write_regs(ov9282, common_regs_list.regs, > - common_regs_list.num_of_regs); > + ret = regmap_multi_reg_write(ov9282->regmap, common_regs, > + ARRAY_SIZE(common_regs)); > if (ret) { > dev_err(ov9282->dev, "fail to write common registers"); > return ret; > } > > bitdepth_index = ov9282->code == MEDIA_BUS_FMT_Y10_1X10 ? 0 : 1; > - ret = ov9282_write_regs(ov9282, bitdepth_regs[bitdepth_index], 2); > + ret = regmap_multi_reg_write(ov9282->regmap, > + bitdepth_regs[bitdepth_index], 2); > if (ret) { > dev_err(ov9282->dev, "fail to write bitdepth regs"); > return ret; > @@ -1144,7 +972,8 @@ static int ov9282_start_streaming(struct ov9282 *ov9282) > > /* Write sensor mode registers */ > reg_list = &ov9282->cur_mode->reg_list; > - ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list->num_of_regs); > + ret = regmap_multi_reg_write(ov9282->regmap, reg_list->regs, > + reg_list->num_of_regs); > if (ret) { > dev_err(ov9282->dev, "fail to write initial registers"); > return ret; > @@ -1158,8 +987,8 @@ static int ov9282_start_streaming(struct ov9282 *ov9282) > } > > /* Start streaming */ > - ret = ov9282_write_reg(ov9282, OV9282_REG_MODE_SELECT, > - 1, OV9282_MODE_STREAMING); > + ret = cci_write(ov9282->regmap, OV9282_REG_MODE_SELECT, > + OV9282_MODE_STREAMING, NULL); > if (ret) { > dev_err(ov9282->dev, "fail to start streaming"); > return ret; > @@ -1176,8 +1005,8 @@ static int ov9282_start_streaming(struct ov9282 *ov9282) > */ > static int ov9282_stop_streaming(struct ov9282 *ov9282) > { > - return ov9282_write_reg(ov9282, OV9282_REG_MODE_SELECT, > - 1, OV9282_MODE_STANDBY); > + return cci_write(ov9282->regmap, OV9282_REG_MODE_SELECT, > + OV9282_MODE_STANDBY, NULL); > } > > /** > @@ -1228,14 +1057,14 @@ static int ov9282_set_stream(struct v4l2_subdev *sd, int enable) > static int ov9282_detect(struct ov9282 *ov9282) > { > int ret; > - u32 val; > + u64 val; > > - ret = ov9282_read_reg(ov9282, OV9282_REG_ID, 2, &val); > + ret = cci_read(ov9282->regmap, OV9282_REG_ID, &val, NULL); > if (ret) > return ret; > > if (val != OV9282_ID) { > - dev_err(ov9282->dev, "chip id mismatch: %x!=%x", > + dev_err(ov9282->dev, "chip id mismatch: %x!=%llx", > OV9282_ID, val); > return -ENXIO; > } > @@ -1397,9 +1226,8 @@ static int ov9282_power_on(struct device *dev) > > usleep_range(400, 600); > > - ret = ov9282_write_reg(ov9282, OV9282_REG_MIPI_CTRL00, 1, > - ov9282->noncontinuous_clock ? > - OV9282_GATED_CLOCK : 0); > + ret = cci_write(ov9282->regmap, OV9282_REG_MIPI_CTRL00, > + ov9282->noncontinuous_clock ? OV9282_GATED_CLOCK : 0, NULL); > if (ret) { > dev_err(ov9282->dev, "fail to write MIPI_CTRL00"); > goto error_clk; > @@ -1576,6 +1404,11 @@ static int ov9282_probe(struct i2c_client *client) > return ret; > } > > + ov9282->regmap = devm_cci_regmap_init_i2c(client, 16); > + if (IS_ERR(ov9282->regmap)) > + return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->regmap), > + "Failed to init CCI\n"); > + > mutex_init(&ov9282->mutex); > > ret = ov9282_power_on(ov9282->dev); > -- > 2.43.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/3] media: i2c: ov9282: Convert to CCI register access helpers 2026-03-03 12:51 ` Dave Stevenson @ 2026-03-03 22:19 ` xiaolei wang 0 siblings, 0 replies; 6+ messages in thread From: xiaolei wang @ 2026-03-03 22:19 UTC (permalink / raw) To: Dave Stevenson Cc: sakari.ailus, laurent.pinchart, tarang.raval, jacopo, mchehab, prabhakar.mahadev-lad.rj, hverkuil+cisco, johannes.goede, hverkuil-cisco, jai.luthra, linux-media, linux-kernel Hi Dave, Thanks for the review On 3/3/26 20:51, Dave Stevenson 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 > > Thanks for the patch > > On Tue, 3 Mar 2026 at 10:50, Xiaolei Wang <xiaolei.wang@windriver.com> wrote: >> Use the new common CCI register access helpers to replace the private >> register access helpers in the ov9282 driver. This simplifies the driver >> by reducing the amount of code. >> >> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> >> Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io> >> --- >> drivers/media/i2c/Kconfig | 1 + >> drivers/media/i2c/ov9282.c | 293 ++++++++----------------------------- >> 2 files changed, 64 insertions(+), 230 deletions(-) >> >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig >> index 5eb1e0e0a87a..3027e71fd8fb 100644 >> --- a/drivers/media/i2c/Kconfig >> +++ b/drivers/media/i2c/Kconfig >> @@ -690,6 +690,7 @@ config VIDEO_OV8865 >> config VIDEO_OV9282 >> tristate "OmniVision OV9282 sensor support" >> depends on OF_GPIO >> + select V4L2_CCI_I2C >> help >> This is a Video4Linux2 sensor driver for the OmniVision >> OV9282 camera sensor. >> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c >> index ded9b2044ff8..8bfaa3ae4be5 100644 >> --- a/drivers/media/i2c/ov9282.c >> +++ b/drivers/media/i2c/ov9282.c >> @@ -12,38 +12,40 @@ >> #include <linux/math.h> >> #include <linux/module.h> >> #include <linux/pm_runtime.h> >> +#include <linux/regmap.h> >> #include <linux/regulator/consumer.h> >> >> +#include <media/v4l2-cci.h> >> #include <media/v4l2-ctrls.h> >> #include <media/v4l2-event.h> >> #include <media/v4l2-fwnode.h> >> #include <media/v4l2-subdev.h> >> >> /* Streaming Mode */ >> -#define OV9282_REG_MODE_SELECT 0x0100 >> +#define OV9282_REG_MODE_SELECT CCI_REG8(0x0100) >> #define OV9282_MODE_STANDBY 0x00 >> #define OV9282_MODE_STREAMING 0x01 >> >> -#define OV9282_REG_PLL_CTRL_0D 0x030d >> +#define OV9282_REG_PLL_CTRL_0D CCI_REG8(0x030d) >> #define OV9282_PLL_CTRL_0D_RAW8 0x60 >> #define OV9282_PLL_CTRL_0D_RAW10 0x50 >> >> -#define OV9282_REG_TIMING_HTS 0x380c >> +#define OV9282_REG_TIMING_HTS CCI_REG16(0x380c) >> #define OV9282_TIMING_HTS_MAX 0x7fff >> >> /* Lines per frame */ >> -#define OV9282_REG_LPFR 0x380e >> +#define OV9282_REG_LPFR CCI_REG16(0x380e) >> >> /* Chip ID */ >> -#define OV9282_REG_ID 0x300a >> +#define OV9282_REG_ID CCI_REG16(0x300a) >> #define OV9282_ID 0x9281 >> >> /* Output enable registers */ >> -#define OV9282_REG_OUTPUT_ENABLE4 0x3004 >> +#define OV9282_REG_OUTPUT_ENABLE4 CCI_REG8(0x3004) >> #define OV9282_OUTPUT_ENABLE4_GPIO2 BIT(1) >> #define OV9282_OUTPUT_ENABLE4_D9 BIT(0) >> >> -#define OV9282_REG_OUTPUT_ENABLE5 0x3005 >> +#define OV9282_REG_OUTPUT_ENABLE5 CCI_REG8(0x3005) >> #define OV9282_OUTPUT_ENABLE5_D8 BIT(7) >> #define OV9282_OUTPUT_ENABLE5_D7 BIT(6) >> #define OV9282_OUTPUT_ENABLE5_D6 BIT(5) >> @@ -53,7 +55,7 @@ >> #define OV9282_OUTPUT_ENABLE5_D2 BIT(1) >> #define OV9282_OUTPUT_ENABLE5_D1 BIT(0) >> >> -#define OV9282_REG_OUTPUT_ENABLE6 0x3006 >> +#define OV9282_REG_OUTPUT_ENABLE6 CCI_REG8(0x3006) >> #define OV9282_OUTPUT_ENABLE6_D0 BIT(7) >> #define OV9282_OUTPUT_ENABLE6_PCLK BIT(6) >> #define OV9282_OUTPUT_ENABLE6_HREF BIT(5) >> @@ -62,14 +64,14 @@ >> #define OV9282_OUTPUT_ENABLE6_VSYNC BIT(1) >> >> /* Exposure control */ >> -#define OV9282_REG_EXPOSURE 0x3500 >> +#define OV9282_REG_EXPOSURE CCI_REG24(0x3500) >> #define OV9282_EXPOSURE_MIN 1 >> #define OV9282_EXPOSURE_OFFSET 25 >> #define OV9282_EXPOSURE_STEP 1 >> #define OV9282_EXPOSURE_DEFAULT 0x0282 >> >> /* AEC/AGC manual */ >> -#define OV9282_REG_AEC_MANUAL 0x3503 >> +#define OV9282_REG_AEC_MANUAL CCI_REG8(0x3503) >> #define OV9282_DIGFRAC_GAIN_DELAY BIT(6) >> #define OV9282_GAIN_CHANGE_DELAY BIT(5) >> #define OV9282_GAIN_DELAY BIT(4) >> @@ -78,28 +80,28 @@ >> #define OV9282_AEC_MANUAL_DEFAULT 0x00 >> >> /* Analog gain control */ >> -#define OV9282_REG_AGAIN 0x3509 >> +#define OV9282_REG_AGAIN CCI_REG8(0x3509) >> #define OV9282_AGAIN_MIN 0x10 >> #define OV9282_AGAIN_MAX 0xff >> #define OV9282_AGAIN_STEP 1 >> #define OV9282_AGAIN_DEFAULT 0x10 >> >> /* Group hold register */ >> -#define OV9282_REG_HOLD 0x3308 >> +#define OV9282_REG_HOLD CCI_REG8(0x3308) >> >> -#define OV9282_REG_ANA_CORE_2 0x3662 >> +#define OV9282_REG_ANA_CORE_2 CCI_REG8(0x3662) >> #define OV9282_ANA_CORE2_RAW8 0x07 >> #define OV9282_ANA_CORE2_RAW10 0x05 >> >> -#define OV9282_REG_TIMING_FORMAT_1 0x3820 >> -#define OV9282_REG_TIMING_FORMAT_2 0x3821 >> +#define OV9282_REG_TIMING_FORMAT_1 CCI_REG8(0x3820) >> +#define OV9282_REG_TIMING_FORMAT_2 CCI_REG8(0x3821) >> #define OV9282_FLIP_BIT BIT(2) >> >> -#define OV9282_REG_MIPI_CTRL00 0x4800 >> +#define OV9282_REG_MIPI_CTRL00 CCI_REG8(0x4800) >> #define OV9282_GATED_CLOCK BIT(5) >> >> /* Flash/Strobe control registers */ >> -#define OV9282_REG_STROBE_FRAME_SPAN 0x3925 >> +#define OV9282_REG_STROBE_FRAME_SPAN CCI_REG32(0x3925) >> #define OV9282_STROBE_FRAME_SPAN_DEFAULT 0x0000001a >> >> /* Input clock rate */ >> @@ -139,16 +141,6 @@ static const char * const ov9282_supply_names[] = { >> >> #define OV9282_NUM_SUPPLIES ARRAY_SIZE(ov9282_supply_names) >> >> -/** >> - * struct ov9282_reg - ov9282 sensor register >> - * @address: Register address >> - * @val: Register value >> - */ >> -struct ov9282_reg { >> - u16 address; >> - u8 val; >> -}; >> - >> /** >> * struct ov9282_reg_list - ov9282 sensor register list >> * @num_of_regs: Number of registers in the list >> @@ -156,7 +148,7 @@ struct ov9282_reg { >> */ >> struct ov9282_reg_list { >> u32 num_of_regs; >> - const struct ov9282_reg *regs; >> + const struct reg_sequence *regs; >> }; >> >> /** >> @@ -188,6 +180,7 @@ struct ov9282_mode { >> * struct ov9282 - ov9282 sensor device structure >> * @dev: Pointer to generic device >> * @sd: V4L2 sub-device >> + * @regmap: Regmap for sensor register access >> * @pad: Media pad. Only one pad supported >> * @reset_gpio: Sensor reset gpio >> * @inclk: Sensor input clock >> @@ -209,6 +202,7 @@ struct ov9282_mode { >> struct ov9282 { >> struct device *dev; >> struct v4l2_subdev sd; >> + struct regmap *regmap; >> struct media_pad pad; >> struct gpio_desc *reset_gpio; >> struct clk *inclk; >> @@ -241,7 +235,7 @@ static const s64 link_freq[] = { >> * register arrays as some settings are written as part of ov9282_power_on, >> * and the reset will clear them. >> */ >> -static const struct ov9282_reg common_regs[] = { >> +static const struct reg_sequence common_regs[] = { >> {0x0302, 0x32}, >> {0x030e, 0x02}, >> {0x3001, 0x00}, >> @@ -305,11 +299,6 @@ static const struct ov9282_reg common_regs[] = { >> {0x5a08, 0x84}, >> }; >> >> -static struct ov9282_reg_list common_regs_list = { >> - .num_of_regs = ARRAY_SIZE(common_regs), >> - .regs = common_regs, >> -}; >> - >> #define MODE_1280_800 0 >> #define MODE_1280_720 1 >> #define MODE_640_400 2 >> @@ -317,7 +306,7 @@ static struct ov9282_reg_list common_regs_list = { >> #define DEFAULT_MODE MODE_1280_720 >> >> /* Sensor mode registers */ >> -static const struct ov9282_reg mode_1280x800_regs[] = { >> +static const struct reg_sequence mode_1280x800_regs[] = { >> {0x3778, 0x00}, >> {0x3800, 0x00}, >> {0x3801, 0x00}, > You changed OV9282_REG_TIMING_FORMAT_[12] above to > CCI_REG8(0x382[01]). However it is used in this array of type > reg_sequence, but all the other values are still using non-CCI_REGx > register writes here. > > If converting to CCI_REGx then you at least need to be consistent. > Personally I'd say do it everywhere and use cci_multi_reg_write > instead of regmap_multi_reg_write. Thanks for pointing it out, it is indeed a bit confusing, I overlooked that. > >> @@ -348,7 +337,7 @@ static const struct ov9282_reg mode_1280x800_regs[] = { >> {0x4509, 0x00}, >> }; >> >> -static const struct ov9282_reg mode_1280x720_regs[] = { >> +static const struct reg_sequence mode_1280x720_regs[] = { >> {0x3778, 0x00}, >> {0x3800, 0x00}, >> {0x3801, 0x00}, >> @@ -379,7 +368,7 @@ static const struct ov9282_reg mode_1280x720_regs[] = { >> {0x4509, 0x80}, >> }; >> >> -static const struct ov9282_reg mode_640x400_regs[] = { >> +static const struct reg_sequence mode_640x400_regs[] = { >> {0x3778, 0x10}, >> {0x3800, 0x00}, >> {0x3801, 0x00}, >> @@ -485,97 +474,6 @@ static inline struct ov9282 *to_ov9282(struct v4l2_subdev *subdev) >> return container_of(subdev, struct ov9282, sd); >> } >> >> -/** >> - * ov9282_read_reg() - Read registers. >> - * @ov9282: pointer to ov9282 device >> - * @reg: register address >> - * @len: length of bytes to read. Max supported bytes is 4 >> - * @val: pointer to register value to be filled. >> - * >> - * Return: 0 if successful, error code otherwise. >> - */ >> -static int ov9282_read_reg(struct ov9282 *ov9282, u16 reg, u32 len, u32 *val) >> -{ >> - struct i2c_client *client = v4l2_get_subdevdata(&ov9282->sd); >> - struct i2c_msg msgs[2] = {0}; >> - u8 addr_buf[2] = {0}; >> - u8 data_buf[4] = {0}; >> - int ret; >> - >> - if (WARN_ON(len > 4)) >> - return -EINVAL; >> - >> - put_unaligned_be16(reg, addr_buf); >> - >> - /* Write register address */ >> - msgs[0].addr = client->addr; >> - msgs[0].flags = 0; >> - msgs[0].len = ARRAY_SIZE(addr_buf); >> - msgs[0].buf = addr_buf; >> - >> - /* Read data from register */ >> - msgs[1].addr = client->addr; >> - msgs[1].flags = I2C_M_RD; >> - msgs[1].len = len; >> - msgs[1].buf = &data_buf[4 - len]; >> - >> - ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); >> - if (ret != ARRAY_SIZE(msgs)) >> - return -EIO; >> - >> - *val = get_unaligned_be32(data_buf); >> - >> - return 0; >> -} >> - >> -/** >> - * ov9282_write_reg() - Write register >> - * @ov9282: pointer to ov9282 device >> - * @reg: register address >> - * @len: length of bytes. Max supported bytes is 4 >> - * @val: register value >> - * >> - * Return: 0 if successful, error code otherwise. >> - */ >> -static int ov9282_write_reg(struct ov9282 *ov9282, u16 reg, u32 len, u32 val) >> -{ >> - struct i2c_client *client = v4l2_get_subdevdata(&ov9282->sd); >> - u8 buf[6] = {0}; >> - >> - if (WARN_ON(len > 4)) >> - return -EINVAL; >> - >> - put_unaligned_be16(reg, buf); >> - put_unaligned_be32(val << (8 * (4 - len)), buf + 2); >> - if (i2c_master_send(client, buf, len + 2) != len + 2) >> - return -EIO; >> - >> - return 0; >> -} >> - >> -/** >> - * ov9282_write_regs() - Write a list of registers >> - * @ov9282: pointer to ov9282 device >> - * @regs: list of registers to be written >> - * @len: length of registers array >> - * >> - * Return: 0 if successful, error code otherwise. >> - */ >> -static int ov9282_write_regs(struct ov9282 *ov9282, >> - const struct ov9282_reg *regs, u32 len) >> -{ >> - unsigned int i; >> - int ret; >> - >> - for (i = 0; i < len; i++) { >> - ret = ov9282_write_reg(ov9282, regs[i].address, 1, regs[i].val); >> - if (ret) >> - return ret; >> - } >> - >> - return 0; >> -} >> - >> /** >> * ov9282_update_controls() - Update control ranges based on streaming mode >> * @ov9282: pointer to ov9282 device >> @@ -639,15 +537,15 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain) >> dev_dbg(ov9282->dev, "Set exp %u (~%u us), analog gain %u", >> exposure, exposure_us, gain); >> >> - ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1); >> + ret = cci_write(ov9282->regmap, OV9282_REG_HOLD, 0x01, NULL); >> if (ret) >> return ret; >> >> - ret = ov9282_write_reg(ov9282, OV9282_REG_EXPOSURE, 3, exposure << 4); >> + ret = cci_write(ov9282->regmap, OV9282_REG_EXPOSURE, exposure << 4, NULL); >> if (ret) >> goto error_release_group_hold; >> >> - ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain); >> + ret = cci_write(ov9282->regmap, OV9282_REG_AGAIN, gain, NULL); >> if (ret) >> goto error_release_group_hold; >> >> @@ -656,60 +554,9 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain) >> OV9282_STROBE_FRAME_SPAN_DEFAULT); >> >> error_release_group_hold: >> - ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0); >> - >> - return ret; >> -} >> - >> -static int ov9282_set_ctrl_hflip(struct ov9282 *ov9282, int value) >> -{ >> - u32 current_val; >> - int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1, >> - ¤t_val); >> - if (ret) >> - return ret; >> + int ret_hold = cci_write(ov9282->regmap, OV9282_REG_HOLD, 0, NULL); >> >> - if (value) >> - current_val |= OV9282_FLIP_BIT; >> - else >> - current_val &= ~OV9282_FLIP_BIT; >> - >> - return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_2, 1, >> - current_val); >> -} >> - >> -static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value) >> -{ >> - u32 current_val; >> - int ret = ov9282_read_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1, >> - ¤t_val); >> - if (ret) >> - return ret; >> - >> - if (value) >> - current_val |= OV9282_FLIP_BIT; >> - else >> - current_val &= ~OV9282_FLIP_BIT; >> - >> - return ov9282_write_reg(ov9282, OV9282_REG_TIMING_FORMAT_1, 1, >> - current_val); >> -} >> - >> -static int ov9282_set_ctrl_flash_strobe_oe(struct ov9282 *ov9282, bool enable) >> -{ >> - u32 current_val; >> - int ret; >> - >> - ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1, ¤t_val); >> - if (ret) >> - return ret; >> - >> - if (enable) >> - current_val |= OV9282_OUTPUT_ENABLE6_STROBE; >> - else >> - current_val &= ~OV9282_OUTPUT_ENABLE6_STROBE; >> - >> - return ov9282_write_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1, current_val); >> + return ret ? ret : ret_hold; >> } >> >> static u32 ov9282_us_to_flash_duration(struct ov9282 *ov9282, u32 value) >> @@ -740,30 +587,6 @@ static u32 ov9282_flash_duration_to_us(struct ov9282 *ov9282, u32 value) >> return DIV_ROUND_UP(value * frame_width, OV9282_STROBE_SPAN_FACTOR); >> } >> >> -static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, u32 value) >> -{ >> - u32 val = ov9282_us_to_flash_duration(ov9282, value); >> - int ret; >> - >> - ret = ov9282_write_reg(ov9282, OV9282_REG_STROBE_FRAME_SPAN, 1, >> - (val >> 24) & 0xff); >> - if (ret) >> - return ret; >> - >> - ret = ov9282_write_reg(ov9282, OV9282_REG_STROBE_FRAME_SPAN + 1, 1, >> - (val >> 16) & 0xff); >> - if (ret) >> - return ret; >> - >> - ret = ov9282_write_reg(ov9282, OV9282_REG_STROBE_FRAME_SPAN + 2, 1, >> - (val >> 8) & 0xff); >> - if (ret) >> - return ret; >> - >> - return ov9282_write_reg(ov9282, OV9282_REG_STROBE_FRAME_SPAN + 3, 1, >> - val & 0xff); >> -} >> - >> /** >> * ov9282_set_ctrl() - Set subdevice control >> * @ctrl: pointer to v4l2_ctrl structure >> @@ -818,23 +641,27 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl) >> break; >> case V4L2_CID_VBLANK: >> lpfr = ov9282->vblank + ov9282->cur_mode->height; >> - ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr); >> + ret = cci_write(ov9282->regmap, OV9282_REG_LPFR, lpfr, NULL); >> break; >> case V4L2_CID_HFLIP: >> - ret = ov9282_set_ctrl_hflip(ov9282, ctrl->val); >> + ret = cci_update_bits(ov9282->regmap, OV9282_REG_TIMING_FORMAT_2, >> + OV9282_FLIP_BIT, ctrl->val ? OV9282_FLIP_BIT : 0, NULL); >> break; >> case V4L2_CID_VFLIP: >> - ret = ov9282_set_ctrl_vflip(ov9282, ctrl->val); >> + ret = cci_update_bits(ov9282->regmap, OV9282_REG_TIMING_FORMAT_1, >> + OV9282_FLIP_BIT, ctrl->val ? OV9282_FLIP_BIT : 0, NULL); >> break; >> case V4L2_CID_HBLANK: >> - ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2, >> - (ctrl->val + ov9282->cur_mode->width) >> 1); >> + ret = cci_write(ov9282->regmap, OV9282_REG_TIMING_HTS, >> + (ctrl->val + ov9282->cur_mode->width) >> 1, NULL); >> break; >> case V4L2_CID_FLASH_STROBE_OE: >> - ret = ov9282_set_ctrl_flash_strobe_oe(ov9282, ctrl->val); >> + ret = cci_update_bits(ov9282->regmap, OV9282_REG_OUTPUT_ENABLE6, >> + OV9282_OUTPUT_ENABLE6_STROBE, >> + ctrl->val ? OV9282_OUTPUT_ENABLE6_STROBE : 0, NULL); >> break; >> case V4L2_CID_FLASH_DURATION: >> - ret = ov9282_set_ctrl_flash_duration(ov9282, ctrl->val); >> + ret = cci_write(ov9282->regmap, OV9282_REG_STROBE_FRAME_SPAN, ctrl->val, NULL); >> break; >> default: >> dev_err(ov9282->dev, "Invalid control %d", ctrl->id); >> @@ -1114,7 +941,7 @@ static int ov9282_get_selection(struct v4l2_subdev *sd, >> */ >> static int ov9282_start_streaming(struct ov9282 *ov9282) >> { >> - const struct ov9282_reg bitdepth_regs[2][2] = { >> + const struct reg_sequence bitdepth_regs[2][2] = { >> { >> {OV9282_REG_PLL_CTRL_0D, OV9282_PLL_CTRL_0D_RAW10}, >> {OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW10}, > Here is a more obvious example. You have CCI_REGx() register defines > being stored into a reg_sequence. > That's an obvious one for the array to be struct cci_reg_sequence and > use cci_multi_reg_write(). > > Otherwise the patch looks reasonable. I will use cci_multi_reg_write() instead of regmap_multi_reg_write() in the next version. thanks xiaolei > > Dave > >> @@ -1128,15 +955,16 @@ static int ov9282_start_streaming(struct ov9282 *ov9282) >> int ret; >> >> /* Write common registers */ >> - ret = ov9282_write_regs(ov9282, common_regs_list.regs, >> - common_regs_list.num_of_regs); >> + ret = regmap_multi_reg_write(ov9282->regmap, common_regs, >> + ARRAY_SIZE(common_regs)); >> if (ret) { >> dev_err(ov9282->dev, "fail to write common registers"); >> return ret; >> } >> >> bitdepth_index = ov9282->code == MEDIA_BUS_FMT_Y10_1X10 ? 0 : 1; >> - ret = ov9282_write_regs(ov9282, bitdepth_regs[bitdepth_index], 2); >> + ret = regmap_multi_reg_write(ov9282->regmap, >> + bitdepth_regs[bitdepth_index], 2); >> if (ret) { >> dev_err(ov9282->dev, "fail to write bitdepth regs"); >> return ret; >> @@ -1144,7 +972,8 @@ static int ov9282_start_streaming(struct ov9282 *ov9282) >> >> /* Write sensor mode registers */ >> reg_list = &ov9282->cur_mode->reg_list; >> - ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list->num_of_regs); >> + ret = regmap_multi_reg_write(ov9282->regmap, reg_list->regs, >> + reg_list->num_of_regs); >> if (ret) { >> dev_err(ov9282->dev, "fail to write initial registers"); >> return ret; >> @@ -1158,8 +987,8 @@ static int ov9282_start_streaming(struct ov9282 *ov9282) >> } >> >> /* Start streaming */ >> - ret = ov9282_write_reg(ov9282, OV9282_REG_MODE_SELECT, >> - 1, OV9282_MODE_STREAMING); >> + ret = cci_write(ov9282->regmap, OV9282_REG_MODE_SELECT, >> + OV9282_MODE_STREAMING, NULL); >> if (ret) { >> dev_err(ov9282->dev, "fail to start streaming"); >> return ret; >> @@ -1176,8 +1005,8 @@ static int ov9282_start_streaming(struct ov9282 *ov9282) >> */ >> static int ov9282_stop_streaming(struct ov9282 *ov9282) >> { >> - return ov9282_write_reg(ov9282, OV9282_REG_MODE_SELECT, >> - 1, OV9282_MODE_STANDBY); >> + return cci_write(ov9282->regmap, OV9282_REG_MODE_SELECT, >> + OV9282_MODE_STANDBY, NULL); >> } >> >> /** >> @@ -1228,14 +1057,14 @@ static int ov9282_set_stream(struct v4l2_subdev *sd, int enable) >> static int ov9282_detect(struct ov9282 *ov9282) >> { >> int ret; >> - u32 val; >> + u64 val; >> >> - ret = ov9282_read_reg(ov9282, OV9282_REG_ID, 2, &val); >> + ret = cci_read(ov9282->regmap, OV9282_REG_ID, &val, NULL); >> if (ret) >> return ret; >> >> if (val != OV9282_ID) { >> - dev_err(ov9282->dev, "chip id mismatch: %x!=%x", >> + dev_err(ov9282->dev, "chip id mismatch: %x!=%llx", >> OV9282_ID, val); >> return -ENXIO; >> } >> @@ -1397,9 +1226,8 @@ static int ov9282_power_on(struct device *dev) >> >> usleep_range(400, 600); >> >> - ret = ov9282_write_reg(ov9282, OV9282_REG_MIPI_CTRL00, 1, >> - ov9282->noncontinuous_clock ? >> - OV9282_GATED_CLOCK : 0); >> + ret = cci_write(ov9282->regmap, OV9282_REG_MIPI_CTRL00, >> + ov9282->noncontinuous_clock ? OV9282_GATED_CLOCK : 0, NULL); >> if (ret) { >> dev_err(ov9282->dev, "fail to write MIPI_CTRL00"); >> goto error_clk; >> @@ -1576,6 +1404,11 @@ static int ov9282_probe(struct i2c_client *client) >> return ret; >> } >> >> + ov9282->regmap = devm_cci_regmap_init_i2c(client, 16); >> + if (IS_ERR(ov9282->regmap)) >> + return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->regmap), >> + "Failed to init CCI\n"); >> + >> mutex_init(&ov9282->mutex); >> >> ret = ov9282_power_on(ov9282->dev); >> -- >> 2.43.0 >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 2/3] media: i2c: ov9282: Switch to using the sub-device state lock 2026-03-03 10:49 [PATCH v3 0/3] media: i2c: ov9282: Modernize driver with CCI and streams API Xiaolei Wang 2026-03-03 10:49 ` [PATCH v3 1/3] media: i2c: ov9282: Convert to CCI register access helpers Xiaolei Wang @ 2026-03-03 10:49 ` Xiaolei Wang 2026-03-03 10:49 ` [PATCH v3 3/3] media: i2c: ov9282: switch to {enable,disable}_streams Xiaolei Wang 2 siblings, 0 replies; 6+ messages in thread From: Xiaolei Wang @ 2026-03-03 10:49 UTC (permalink / raw) To: sakari.ailus, laurent.pinchart, tarang.raval, 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> Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io> --- drivers/media/i2c/ov9282.c | 51 +++++++++++++++----------------------- 1 file changed, 20 insertions(+), 31 deletions(-) diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c index 8bfaa3ae4be5..6556cbce8dc1 100644 --- a/drivers/media/i2c/ov9282.c +++ b/drivers/media/i2c/ov9282.c @@ -221,7 +221,6 @@ struct ov9282 { bool noncontinuous_clock; const struct ov9282_mode *cur_mode; u32 code; - struct mutex mutex; }; static const s64 link_freq[] = { @@ -795,8 +794,6 @@ static int ov9282_get_pad_format(struct v4l2_subdev *sd, { struct ov9282 *ov9282 = to_ov9282(sd); - mutex_lock(&ov9282->mutex); - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { struct v4l2_mbus_framefmt *framefmt; @@ -807,8 +804,6 @@ static int ov9282_get_pad_format(struct v4l2_subdev *sd, fmt); } - mutex_unlock(&ov9282->mutex); - return 0; } @@ -829,8 +824,6 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd, u32 code; int ret = 0; - mutex_lock(&ov9282->mutex); - mode = v4l2_find_nearest_size(supported_modes, ARRAY_SIZE(supported_modes), width, height, @@ -856,8 +849,6 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd, } } - mutex_unlock(&ov9282->mutex); - return ret; } @@ -904,10 +895,8 @@ static int ov9282_get_selection(struct v4l2_subdev *sd, case V4L2_SEL_TGT_CROP: { struct ov9282 *ov9282 = to_ov9282(sd); - mutex_lock(&ov9282->mutex); sel->r = *__ov9282_get_pad_crop(ov9282, sd_state, sel->pad, sel->which); - mutex_unlock(&ov9282->mutex); return 0; } @@ -1019,9 +1008,10 @@ static int ov9282_stop_streaming(struct ov9282 *ov9282) static int ov9282_set_stream(struct v4l2_subdev *sd, int enable) { struct ov9282 *ov9282 = to_ov9282(sd); + struct v4l2_subdev_state *state; int ret; - mutex_lock(&ov9282->mutex); + state = v4l2_subdev_lock_and_get_active_state(sd); if (enable) { ret = pm_runtime_resume_and_get(ov9282->dev); @@ -1036,14 +1026,14 @@ static int ov9282_set_stream(struct v4l2_subdev *sd, int enable) pm_runtime_put(ov9282->dev); } - mutex_unlock(&ov9282->mutex); + v4l2_subdev_unlock_state(state); return 0; error_power_off: pm_runtime_put(ov9282->dev); error_unlock: - mutex_unlock(&ov9282->mutex); + v4l2_subdev_unlock_state(state); return ret; } @@ -1285,9 +1275,6 @@ static int ov9282_init_controls(struct ov9282 *ov9282) if (ret) return ret; - /* Serialize controls with sensor device */ - ctrl_hdlr->lock = &ov9282->mutex; - /* Initialize exposure and gain */ lpfr = mode->vblank + mode->height; ov9282->exp_ctrl = v4l2_ctrl_new_std(ctrl_hdlr, @@ -1409,13 +1396,10 @@ static int ov9282_probe(struct i2c_client *client) return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->regmap), "Failed to init CCI\n"); - mutex_init(&ov9282->mutex); - ret = ov9282_power_on(ov9282->dev); - if (ret) { - dev_err(ov9282->dev, "failed to power-on the sensor"); - goto error_mutex_destroy; - } + if (ret) + return dev_err_probe(ov9282->dev, ret, + "failed to power-on the sensor"); /* Check module identity */ ret = ov9282_detect(ov9282); @@ -1448,27 +1432,34 @@ static int ov9282_probe(struct i2c_client *client) goto error_handler_free; } - ret = v4l2_async_register_subdev_sensor(&ov9282->sd); + ov9282->sd.state_lock = ov9282->ctrl_handler.lock; + ret = v4l2_subdev_init_finalize(&ov9282->sd); if (ret < 0) { - dev_err(ov9282->dev, - "failed to register async subdev: %d", ret); + dev_err_probe(ov9282->dev, ret, "failed to init subdev\n"); goto error_media_entity; } pm_runtime_set_active(ov9282->dev); pm_runtime_enable(ov9282->dev); + + ret = v4l2_async_register_subdev_sensor(&ov9282->sd); + if (ret < 0) + goto v4l2_subdev_cleanup; + pm_runtime_idle(ov9282->dev); return 0; +v4l2_subdev_cleanup: + v4l2_subdev_cleanup(&ov9282->sd); + pm_runtime_disable(ov9282->dev); + pm_runtime_set_suspended(ov9282->dev); error_media_entity: media_entity_cleanup(&ov9282->sd.entity); error_handler_free: v4l2_ctrl_handler_free(ov9282->sd.ctrl_handler); error_power_off: ov9282_power_off(ov9282->dev); -error_mutex_destroy: - mutex_destroy(&ov9282->mutex); return ret; } @@ -1482,9 +1473,9 @@ static int ov9282_probe(struct i2c_client *client) static void ov9282_remove(struct i2c_client *client) { struct v4l2_subdev *sd = i2c_get_clientdata(client); - struct ov9282 *ov9282 = to_ov9282(sd); v4l2_async_unregister_subdev(sd); + v4l2_subdev_cleanup(sd); media_entity_cleanup(&sd->entity); v4l2_ctrl_handler_free(sd->ctrl_handler); @@ -1492,8 +1483,6 @@ static void ov9282_remove(struct i2c_client *client) if (!pm_runtime_status_suspended(&client->dev)) ov9282_power_off(&client->dev); pm_runtime_set_suspended(&client->dev); - - mutex_destroy(&ov9282->mutex); } static const struct dev_pm_ops ov9282_pm_ops = { -- 2.43.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 3/3] media: i2c: ov9282: switch to {enable,disable}_streams 2026-03-03 10:49 [PATCH v3 0/3] media: i2c: ov9282: Modernize driver with CCI and streams API Xiaolei Wang 2026-03-03 10:49 ` [PATCH v3 1/3] media: i2c: ov9282: Convert to CCI register access helpers Xiaolei Wang 2026-03-03 10:49 ` [PATCH v3 2/3] media: i2c: ov9282: Switch to using the sub-device state lock Xiaolei Wang @ 2026-03-03 10:49 ` Xiaolei Wang 2 siblings, 0 replies; 6+ messages in thread From: Xiaolei Wang @ 2026-03-03 10:49 UTC (permalink / raw) To: sakari.ailus, laurent.pinchart, tarang.raval, 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: Tarang Raval <tarang.raval@siliconsignals.io> --- drivers/media/i2c/ov9282.c | 79 ++++++++++++-------------------------- 1 file changed, 25 insertions(+), 54 deletions(-) diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c index 6556cbce8dc1..924da6b0a6f7 100644 --- a/drivers/media/i2c/ov9282.c +++ b/drivers/media/i2c/ov9282.c @@ -922,13 +922,9 @@ static int ov9282_get_selection(struct v4l2_subdev *sd, return -EINVAL; } -/** - * ov9282_start_streaming() - Start sensor stream - * @ov9282: pointer to ov9282 device - * - * Return: 0 if successful, error code otherwise. - */ -static int ov9282_start_streaming(struct ov9282 *ov9282) +static int ov9282_enable_streams(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) { const struct reg_sequence bitdepth_regs[2][2] = { { @@ -939,16 +935,21 @@ static int ov9282_start_streaming(struct ov9282 *ov9282) {OV9282_REG_ANA_CORE_2, OV9282_ANA_CORE2_RAW8}, } }; + struct ov9282 *ov9282 = to_ov9282(sd); const struct ov9282_reg_list *reg_list; int bitdepth_index; int ret; + ret = pm_runtime_resume_and_get(ov9282->dev); + if (ret) + return ret; + /* Write common registers */ ret = regmap_multi_reg_write(ov9282->regmap, common_regs, ARRAY_SIZE(common_regs)); if (ret) { dev_err(ov9282->dev, "fail to write common registers"); - return ret; + goto err_pm_put; } bitdepth_index = ov9282->code == MEDIA_BUS_FMT_Y10_1X10 ? 0 : 1; @@ -956,7 +957,7 @@ static int ov9282_start_streaming(struct ov9282 *ov9282) bitdepth_regs[bitdepth_index], 2); if (ret) { dev_err(ov9282->dev, "fail to write bitdepth regs"); - return ret; + goto err_pm_put; } /* Write sensor mode registers */ @@ -965,14 +966,14 @@ static int ov9282_start_streaming(struct ov9282 *ov9282) reg_list->num_of_regs); if (ret) { dev_err(ov9282->dev, "fail to write initial registers"); - return ret; + goto err_pm_put; } /* Setup handler will write actual exposure and gain */ ret = __v4l2_ctrl_handler_setup(ov9282->sd.ctrl_handler); if (ret) { dev_err(ov9282->dev, "fail to setup handler"); - return ret; + goto err_pm_put; } /* Start streaming */ @@ -980,60 +981,28 @@ static int ov9282_start_streaming(struct ov9282 *ov9282) OV9282_MODE_STREAMING, NULL); if (ret) { dev_err(ov9282->dev, "fail to start streaming"); - return ret; + goto err_pm_put; } return 0; -} -/** - * ov9282_stop_streaming() - Stop sensor stream - * @ov9282: pointer to ov9282 device - * - * Return: 0 if successful, error code otherwise. - */ -static int ov9282_stop_streaming(struct ov9282 *ov9282) -{ - return cci_write(ov9282->regmap, OV9282_REG_MODE_SELECT, - OV9282_MODE_STANDBY, NULL); +err_pm_put: + pm_runtime_put(ov9282->dev); + + return ret; } -/** - * ov9282_set_stream() - Enable sensor streaming - * @sd: pointer to ov9282 subdevice - * @enable: set to enable sensor streaming - * - * Return: 0 if successful, error code otherwise. - */ -static int ov9282_set_stream(struct v4l2_subdev *sd, int enable) +static int ov9282_disable_streams(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) { struct ov9282 *ov9282 = to_ov9282(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(ov9282->dev); - if (ret) - goto error_unlock; - - ret = ov9282_start_streaming(ov9282); - if (ret) - goto error_power_off; - } else { - ov9282_stop_streaming(ov9282); - pm_runtime_put(ov9282->dev); - } - - v4l2_subdev_unlock_state(state); - - return 0; + ret = cci_write(ov9282->regmap, OV9282_REG_MODE_SELECT, + OV9282_MODE_STANDBY, NULL); -error_power_off: pm_runtime_put(ov9282->dev); -error_unlock: - v4l2_subdev_unlock_state(state); return ret; } @@ -1165,7 +1134,7 @@ static const struct v4l2_subdev_core_ops ov9282_core_ops = { }; static const struct v4l2_subdev_video_ops ov9282_video_ops = { - .s_stream = ov9282_set_stream, + .s_stream = v4l2_subdev_s_stream_helper, }; static const struct v4l2_subdev_pad_ops ov9282_pad_ops = { @@ -1174,6 +1143,8 @@ static const struct v4l2_subdev_pad_ops ov9282_pad_ops = { .get_fmt = ov9282_get_pad_format, .set_fmt = ov9282_set_pad_format, .get_selection = ov9282_get_selection, + .enable_streams = ov9282_enable_streams, + .disable_streams = ov9282_disable_streams, }; static const struct v4l2_subdev_ops ov9282_subdev_ops = { -- 2.43.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-03-03 22:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-03-03 10:49 [PATCH v3 0/3] media: i2c: ov9282: Modernize driver with CCI and streams API Xiaolei Wang
2026-03-03 10:49 ` [PATCH v3 1/3] media: i2c: ov9282: Convert to CCI register access helpers Xiaolei Wang
2026-03-03 12:51 ` Dave Stevenson
2026-03-03 22:19 ` xiaolei wang
2026-03-03 10:49 ` [PATCH v3 2/3] media: i2c: ov9282: Switch to using the sub-device state lock Xiaolei Wang
2026-03-03 10:49 ` [PATCH v3 3/3] media: i2c: ov9282: switch to {enable,disable}_streams 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®