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

[xiaolei wang <xiaolei.wang@windriver.com>]

On 12/31/25 18:26, Tarang Raval wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> Hi Xiaolei,
>
>> Use the new common CCI register access helpers to replace the private
>> register access helpers in the ov5647 driver. This simplifies the driver
>> by reducing the amount of code.
>>   
>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
>> ---
>>   drivers/media/i2c/Kconfig  |   1 +
>>   drivers/media/i2c/ov5647.c | 289 +++++++++++++------------------------
>>   2 files changed, 99 insertions(+), 191 deletions(-)
>>   
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index 4b4db8c4f496..cce63349e71e 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -529,6 +529,7 @@ config VIDEO_OV5645
>>
>>   config VIDEO_OV5647
>>      tristate "OmniVision OV5647 sensor support"
>> +   select V4L2_CCI_I2C
>>      help
>>        This is a Video4Linux2 sensor driver for the OmniVision
>>        OV5647 camera.
>> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
>> index e193fef4fced..cbcb760ba5cd 100644
>> --- a/drivers/media/i2c/ov5647.c
>> +++ b/drivers/media/i2c/ov5647.c
>> @@ -20,8 +20,10 @@
>>   #include <linux/module.h>
>>   #include <linux/of_graph.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>>   #include <linux/slab.h>
>>   #include <linux/videodev2.h>
>> +#include <media/v4l2-cci.h>
>>   #include <media/v4l2-ctrls.h>
>>   #include <media/v4l2-device.h>
>>   #include <media/v4l2-event.h>
>> @@ -41,24 +43,19 @@
>>   #define MIPI_CTRL00_BUS_IDLE                 BIT(2)
>>   #define MIPI_CTRL00_CLOCK_LANE_DISABLE       BIT(0)
>>
>> -#define OV5647_SW_STANDBY        0x0100
>> -#define OV5647_SW_RESET                0x0103
>> -#define OV5647_REG_CHIPID_H            0x300a
>> -#define OV5647_REG_CHIPID_L            0x300b
>> -#define OV5640_REG_PAD_OUT       0x300d
>> -#define OV5647_REG_EXP_HI        0x3500
>> -#define OV5647_REG_EXP_MID       0x3501
>> -#define OV5647_REG_EXP_LO        0x3502
>> -#define OV5647_REG_AEC_AGC       0x3503
>> -#define OV5647_REG_GAIN_HI       0x350a
>> -#define OV5647_REG_GAIN_LO       0x350b
>> -#define OV5647_REG_VTS_HI        0x380e
>> -#define OV5647_REG_VTS_LO        0x380f
>> -#define OV5647_REG_FRAME_OFF_NUMBER    0x4202
>> -#define OV5647_REG_MIPI_CTRL00         0x4800
>> -#define OV5647_REG_MIPI_CTRL14         0x4814
>> -#define OV5647_REG_AWB                 0x5001
>> -#define OV5647_REG_ISPCTRL3D           0x503d
>> +#define OV5647_SW_STANDBY        CCI_REG8(0x0100)
>> +#define OV5647_SW_RESET                CCI_REG8(0x0103)
>> +#define OV5647_REG_CHIPID        CCI_REG16(0x300a)
>> +#define OV5640_REG_PAD_OUT       CCI_REG8(0x300d)
>> +#define OV5647_REG_EXPOSURE            CCI_REG24(0x3500)
>> +#define OV5647_REG_AEC_AGC       CCI_REG8(0x3503)
>> +#define OV5647_REG_GAIN                CCI_REG16(0x350a)
>> +#define OV5647_REG_VTS                 CCI_REG16(0x380e)
>> +#define OV5647_REG_FRAME_OFF_NUMBER    CCI_REG8(0x4202)
>> +#define OV5647_REG_MIPI_CTRL00         CCI_REG8(0x4800)
>> +#define OV5647_REG_MIPI_CTRL14         CCI_REG8(0x4814)
>> +#define OV5647_REG_AWB                 CCI_REG8(0x5001)
>> +#define OV5647_REG_ISPCTRL3D           CCI_REG8(0x503d)
>>
>>   #define REG_TERM 0xfffe
>>   #define VAL_TERM 0xfe
>> @@ -81,23 +78,19 @@
>>   #define OV5647_EXPOSURE_DEFAULT        1000
>>   #define OV5647_EXPOSURE_MAX            65535
>>
>> -struct regval_list {
>> -   u16 addr;
>> -   u8 data;
>> -};
>> -
>>   struct ov5647_mode {
>>      struct v4l2_mbus_framefmt     format;
>>      struct v4l2_rect        crop;
>>      u64                     pixel_rate;
>>      int                     hts;
>>      int                     vts;
>> -   const struct regval_list      *reg_list;
>> +   const struct reg_sequence     *reg_list;
>>      unsigned int                  num_regs;
>>   };
>>
>>   struct ov5647 {
>>      struct v4l2_subdev            sd;
>> +   struct regmap                   *regmap;
>>      struct media_pad        pad;
>>      struct mutex                  lock;
>>      struct clk              *xclk;
>> @@ -130,19 +123,19 @@ static const u8 ov5647_test_pattern_val[] = {
>>      0x81, /* Random Data */
>>   };
>>
>> -static const struct regval_list sensor_oe_disable_regs[] = {
>> +static const struct reg_sequence sensor_oe_disable_regs[] = {
>>      {0x3000, 0x00},
>>      {0x3001, 0x00},
>>      {0x3002, 0x00},
>>   };
>>
>> -static const struct regval_list sensor_oe_enable_regs[] = {
>> +static const struct reg_sequence sensor_oe_enable_regs[] = {
>>      {0x3000, 0x0f},
>>      {0x3001, 0xff},
>>      {0x3002, 0xe4},
>>   };
>>
>> -static struct regval_list ov5647_2592x1944_10bpp[] = {
>> +static const struct reg_sequence ov5647_2592x1944_10bpp[] = {
>>      {0x0100, 0x00},
>>      {0x0103, 0x01},
>>      {0x3034, 0x1a},
>> @@ -230,8 +223,7 @@ static struct regval_list ov5647_2592x1944_10bpp[] = {
>>      {0x3503, 0x03},
>>      {0x0100, 0x01},
>>   };
>> -
> Please keep one blank line here. do not remove.
I will fix it in the next version.
>
>> -static struct regval_list ov5647_1080p30_10bpp[] = {
>> +static const struct reg_sequence ov5647_1080p30_10bpp[] = {
>>      {0x0100, 0x00},
>>      {0x0103, 0x01},
>>      {0x3034, 0x1a},
>> @@ -320,7 +312,7 @@ static struct regval_list ov5647_1080p30_10bpp[] = {
>>      {0x0100, 0x01},
>>   };
>>
>> -static struct regval_list ov5647_2x2binned_10bpp[] = {
>> +static const struct reg_sequence ov5647_2x2binned_10bpp[] = {
>>      {0x0100, 0x00},
>>      {0x0103, 0x01},
>>      {0x3034, 0x1a},
>> @@ -413,7 +405,7 @@ static struct regval_list ov5647_2x2binned_10bpp[] = {
>>      {0x0100, 0x01},
>>   };
>>
>> -static struct regval_list ov5647_640x480_10bpp[] = {
>> +static const struct reg_sequence ov5647_640x480_10bpp[] = {
>>      {0x0100, 0x00},
>>      {0x0103, 0x01},
>>      {0x3035, 0x11},
>> @@ -594,109 +586,35 @@ static const struct ov5647_mode ov5647_modes[] = {
>>   #define OV5647_DEFAULT_MODE      (&ov5647_modes[3])
>>   #define OV5647_DEFAULT_FORMAT    (ov5647_modes[3].format)
>>
>> -static int ov5647_write16(struct v4l2_subdev *sd, u16 reg, u16 val)
>> -{
>> -   unsigned char data[4] = { reg >> 8, reg & 0xff, val >> 8, val & 0xff};
>> -   struct i2c_client *client = v4l2_get_subdevdata(sd);
>> -   int ret;
>> -
>> -   ret = i2c_master_send(client, data, 4);
>> -   if (ret < 0) {
>> -         dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
>> -               __func__, reg);
>> -         return ret;
>> -   }
>> -
>> -   return 0;
>> -}
>> -
>> -static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val)
>> -{
>> -   unsigned char data[3] = { reg >> 8, reg & 0xff, val};
>> -   struct i2c_client *client = v4l2_get_subdevdata(sd);
>> -   int ret;
>> -
>> -   ret = i2c_master_send(client, data, 3);
>> -   if (ret < 0) {
>> -         dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
>> -                     __func__, reg);
>> -         return ret;
>> -   }
>> -
>> -   return 0;
>> -}
>> -
>> -static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
>> -{
>> -   struct i2c_client *client = v4l2_get_subdevdata(sd);
>> -   u8 buf[2] = { reg >> 8, reg & 0xff };
>> -   struct i2c_msg msg[2];
>> -   int ret;
>> -
>> -   msg[0].addr = client->addr;
>> -   msg[0].flags = client->flags;
>> -   msg[0].buf = buf;
>> -   msg[0].len = sizeof(buf);
>> -
>> -   msg[1].addr = client->addr;
>> -   msg[1].flags = client->flags | I2C_M_RD;
>> -   msg[1].buf = buf;
>> -   msg[1].len = 1;
>> -
>> -   ret = i2c_transfer(client->adapter, msg, 2);
>> -   if (ret != 2) {
>> -         dev_err(&client->dev, "%s: i2c read error, reg: %x = %d\n",
>> -               __func__, reg, ret);
>> -         return ret >= 0 ? -EINVAL : ret;
>> -   }
>> -
>> -   *val = buf[0];
>> -
>> -   return 0;
>> -}
>> -
>> -static int ov5647_write_array(struct v4l2_subdev *sd,
>> -                     const struct regval_list *regs, int array_size)
>> -{
>> -   int i, ret;
>> -
>> -   for (i = 0; i < array_size; i++) {
>> -         ret = ov5647_write(sd, regs[i].addr, regs[i].data);
>> -         if (ret < 0)
>> -               return ret;
>> -   }
>> -
>> -   return 0;
>> -}
>> -
>>   static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
>>   {
>> -   u8 channel_id;
>> +   struct ov5647 *sensor = to_sensor(sd);
>> +   u64 channel_id;
>>      int ret;
>>
>> -   ret = ov5647_read(sd, OV5647_REG_MIPI_CTRL14, &channel_id);
>> +   ret = cci_read(sensor->regmap, OV5647_REG_MIPI_CTRL14, &channel_id, NULL);
>>      if (ret < 0)
>>            return ret;
>>
>>      channel_id &= ~(3 << 6);
>>
>> -   return ov5647_write(sd, OV5647_REG_MIPI_CTRL14,
>> -                   channel_id | (channel << 6));
>> +   return cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL14,
>> +                channel_id | (channel << 6), NULL);
>>   }
>>
>>   static int ov5647_set_mode(struct v4l2_subdev *sd)
>>   {
>>      struct i2c_client *client = v4l2_get_subdevdata(sd);
>>      struct ov5647 *sensor = to_sensor(sd);
>> -   u8 resetval, rdval;
>> +   u64 resetval, rdval;
>>      int ret;
>>
>> -   ret = ov5647_read(sd, OV5647_SW_STANDBY, &rdval);
>> +   ret = cci_read(sensor->regmap, OV5647_SW_STANDBY, &rdval, NULL);
>>      if (ret < 0)
>>            return ret;
>>
>> -   ret = ov5647_write_array(sd, sensor->mode->reg_list,
>> -                      sensor->mode->num_regs);
>> +   ret = regmap_multi_reg_write(sensor->regmap, sensor->mode->reg_list,
>> +                          sensor->mode->num_regs);
>>      if (ret < 0) {
>>            dev_err(&client->dev, "write sensor default regs error\n");
>>            return ret;
>> @@ -706,13 +624,13 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)
>>      if (ret < 0)
>>            return ret;
>>
>> -   ret = ov5647_read(sd, OV5647_SW_STANDBY, &resetval);
>> +   ret = cci_read(sensor->regmap, OV5647_SW_STANDBY, &resetval, NULL);
>>      if (ret < 0)
>>            return ret;
>>
>>      if (!(resetval & 0x01)) {
>>            dev_err(&client->dev, "Device was in SW standby");
>> -         ret = ov5647_write(sd, OV5647_SW_STANDBY, 0x01);
>> +         ret = cci_write(sensor->regmap, OV5647_SW_STANDBY, 0x01, NULL);
>>            if (ret < 0)
>>                  return ret;
> This feels wrong to me but anyway this is not related to your patch.

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

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

>
>>      }
>> @@ -725,7 +643,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)
>>      struct i2c_client *client = v4l2_get_subdevdata(sd);
>>      struct ov5647 *sensor = to_sensor(sd);
>>      u8 val = MIPI_CTRL00_BUS_IDLE;
>> -   int ret;
>> +   int ret = 0;
> No need for zero initialization.
I will fix it in the next version.
>
>>      ret = ov5647_set_mode(sd);
>>      if (ret) {
>> @@ -742,32 +660,25 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)
>>            val |= MIPI_CTRL00_CLOCK_LANE_GATE |
>>                   MIPI_CTRL00_LINE_SYNC_ENABLE;
>>
>> -   ret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00, val);
>> -   if (ret < 0)
>> -         return ret;
>> -
>> -   ret = ov5647_write(sd, OV5647_REG_FRAME_OFF_NUMBER, 0x00);
>> -   if (ret < 0)
>> -         return ret;
>> +   cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL00, val, &ret);
>> +   cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x00, &ret);
>> +   cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x00, &ret);
>>
>> -   return ov5647_write(sd, OV5640_REG_PAD_OUT, 0x00);
>> +   return ret;
>>   }
>>
>>   static int ov5647_stream_off(struct v4l2_subdev *sd)
>>   {
>> -   int ret;
>> +   struct ov5647 *sensor = to_sensor(sd);
>> +   int ret = 0;
>>
>> -   ret = ov5647_write(sd, OV5647_REG_MIPI_CTRL00,
>> -                  MIPI_CTRL00_CLOCK_LANE_GATE | MIPI_CTRL00_BUS_IDLE |
>> -                  MIPI_CTRL00_CLOCK_LANE_DISABLE);
>> -   if (ret < 0)
>> -         return ret;
>> +   cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL00,
>> +           MIPI_CTRL00_CLOCK_LANE_GATE | MIPI_CTRL00_BUS_IDLE |
>> +           MIPI_CTRL00_CLOCK_LANE_DISABLE, &ret);
>> +   cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x0f, &ret);
>> +   cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x01, &ret);
>>
>> -   ret = ov5647_write(sd, OV5647_REG_FRAME_OFF_NUMBER, 0x0f);
>> -   if (ret < 0)
>> -         return ret;
>> -
>> -   return ov5647_write(sd, OV5640_REG_PAD_OUT, 0x01);
>> +   return ret;
>>   }
>>
>>   static int ov5647_power_on(struct device *dev)
>> @@ -788,8 +699,8 @@ static int ov5647_power_on(struct device *dev)
>>            goto error_pwdn;
>>      }
>>
>> -   ret = ov5647_write_array(&sensor->sd, sensor_oe_enable_regs,
>> -                      ARRAY_SIZE(sensor_oe_enable_regs));
>> +   ret = regmap_multi_reg_write(sensor->regmap, sensor_oe_enable_regs,
>> +                          ARRAY_SIZE(sensor_oe_enable_regs));
>>      if (ret < 0) {
>>            dev_err(dev, "write sensor_oe_enable_regs error\n");
>>            goto error_clk_disable;
>> @@ -815,23 +726,23 @@ static int ov5647_power_on(struct device *dev)
>>   static int ov5647_power_off(struct device *dev)
>>   {
>>      struct ov5647 *sensor = dev_get_drvdata(dev);
>> -   u8 rdval;
>> +   u64 rdval;
>>      int ret;
>>
>>      dev_dbg(dev, "OV5647 power off\n");
>>
>> -   ret = ov5647_write_array(&sensor->sd, sensor_oe_disable_regs,
>> -                      ARRAY_SIZE(sensor_oe_disable_regs));
>> +   ret = regmap_multi_reg_write(sensor->regmap, sensor_oe_disable_regs,
>> +                          ARRAY_SIZE(sensor_oe_disable_regs));
>>      if (ret < 0)
>>            dev_dbg(dev, "disable oe failed\n");
>>
>>      /* Enter software standby */
>> -   ret = ov5647_read(&sensor->sd, OV5647_SW_STANDBY, &rdval);
>> +   ret = cci_read(sensor->regmap, OV5647_SW_STANDBY, &rdval, NULL);
>>      if (ret < 0)
>>            dev_dbg(dev, "software standby failed\n");
>>
>>      rdval &= ~0x01;
>> -   ret = ov5647_write(&sensor->sd, OV5647_SW_STANDBY, rdval);
>> +   ret = cci_write(sensor->regmap, OV5647_SW_STANDBY, rdval, NULL);
>>      if (ret < 0)
>>            dev_dbg(dev, "software standby failed\n");
>>
>> @@ -845,10 +756,11 @@ static int ov5647_power_off(struct device *dev)
>>   static int ov5647_sensor_get_register(struct v4l2_subdev *sd,
>>                              struct v4l2_dbg_register *reg)
>>   {
>> +   struct ov5647 *sensor = to_sensor(sd);
>>      int ret;
>> -   u8 val;
>> +   u64 val;
>>
>> -   ret = ov5647_read(sd, reg->reg & 0xff, &val);
>> +   ret = cci_read(sensor->regmap, reg->reg & 0xff, &val, NULL);
>>      if (ret < 0)
>>            return ret;
>>
>> @@ -861,7 +773,9 @@ static int ov5647_sensor_get_register(struct v4l2_subdev *sd,
>>   static int ov5647_sensor_set_register(struct v4l2_subdev *sd,
>>                              const struct v4l2_dbg_register *reg)
>>   {
>> -   return ov5647_write(sd, reg->reg & 0xff, reg->val & 0xff);
>> +   struct ov5647 *sensor = to_sensor(sd);
>> +
>> +   return cci_write(sensor->regmap, reg->reg & 0xff, reg->val & 0xff, NULL);
>>   }
>>   #endif
>>
>> @@ -1089,33 +1003,27 @@ static const struct v4l2_subdev_ops ov5647_subdev_ops = {
>>
>>   static int ov5647_detect(struct v4l2_subdev *sd)
>>   {
>> +   struct ov5647 *sensor = to_sensor(sd);
>>      struct i2c_client *client = v4l2_get_subdevdata(sd);
>> -   u8 read;
>> +   u64 read;
>>      int ret;
>>
>> -   ret = ov5647_write(sd, OV5647_SW_RESET, 0x01);
>> +   ret = cci_write(sensor->regmap, OV5647_SW_RESET, 0x01, NULL);
>>      if (ret < 0)
>>            return ret;
>>
>> -   ret = ov5647_read(sd, OV5647_REG_CHIPID_H, &read);
>> -   if (ret < 0)
>> -         return ret;
>> -
>> -   if (read != 0x56) {
>> -         dev_err(&client->dev, "ID High expected 0x56 got %x", read);
>> -         return -ENODEV;
>> -   }
>> -
>> -   ret = ov5647_read(sd, OV5647_REG_CHIPID_L, &read);
>> +   ret = cci_read(sensor->regmap, OV5647_REG_CHIPID, &read, NULL);
>>      if (ret < 0)
>> -         return ret;
>> +         return dev_err_probe(&client->dev, ret,
>> +                          "failed to read chip id %x\n",
>> +                          OV5647_REG_CHIPID);
>>
>> -   if (read != 0x47) {
>> -         dev_err(&client->dev, "ID Low expected 0x47 got %x", read);
>> +   if (read != 0x5647) {
> We should define a macro for the chip ID and use it here.
OK，I will define a chip ID.
>
>> +         dev_err(&client->dev, "Chip ID expected 0x5647 got 0x%llx", read);
>>            return -ENODEV;
>>      }
>>
>> -   return ov5647_write(sd, OV5647_SW_RESET, 0x00);
>> +   return cci_write(sensor->regmap, OV5647_SW_RESET, 0x00, NULL);
>>   }
>>
>>   static int ov5647_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>> @@ -1140,70 +1048,62 @@ static const struct v4l2_subdev_internal_ops ov5647_subdev_internal_ops = {
>>
>>   static int ov5647_s_auto_white_balance(struct v4l2_subdev *sd, u32 val)
>>   {
>> -   return ov5647_write(sd, OV5647_REG_AWB, val ? 1 : 0);
>> +   struct ov5647 *sensor = to_sensor(sd);
>> +
>> +   return cci_write(sensor->regmap, OV5647_REG_AWB, val ? 1 : 0, NULL);
>>   }
>>
>>   static int ov5647_s_autogain(struct v4l2_subdev *sd, u32 val)
>>   {
>> +   struct ov5647 *sensor = to_sensor(sd);
>>      int ret;
>> -   u8 reg;
>> +   u64 reg;
>>
>>      /* Non-zero turns on AGC by clearing bit 1.*/
>> -   ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
>> +   ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, &reg, NULL);
>>      if (ret)
>>            return ret;
>>
>> -   return ov5647_write(sd, OV5647_REG_AEC_AGC, val ? reg & ~BIT(1)
>> -                                       : reg | BIT(1));
>> +   return cci_write(sensor->regmap, OV5647_REG_AEC_AGC, val ? reg & ~BIT(1)
>> +                                       : reg | BIT(1), NULL);
>>   }
>>
>>   static int ov5647_s_exposure_auto(struct v4l2_subdev *sd, u32 val)
>>   {
>> +   struct ov5647 *sensor = to_sensor(sd);
>>      int ret;
>> -   u8 reg;
>> +   u64 reg;
>>
>>      /*
>>       * Everything except V4L2_EXPOSURE_MANUAL turns on AEC by
>>       * clearing bit 0.
>>       */
>> -   ret = ov5647_read(sd, OV5647_REG_AEC_AGC, &reg);
>> +   ret = cci_read(sensor->regmap, OV5647_REG_AEC_AGC, &reg, NULL);
>>      if (ret)
>>            return ret;
>>
>> -   return ov5647_write(sd, OV5647_REG_AEC_AGC,
>> +   return cci_write(sensor->regmap, OV5647_REG_AEC_AGC,
>>                      val == V4L2_EXPOSURE_MANUAL ? reg | BIT(0)
>> -                                       : reg & ~BIT(0));
>> +                                       : reg & ~BIT(0), NULL);
>>   }
>>
>>   static int ov5647_s_analogue_gain(struct v4l2_subdev *sd, u32 val)
>>   {
>> -   int ret;
>> +   struct ov5647 *sensor = to_sensor(sd);
>>
>>      /* 10 bits of gain, 2 in the high register. */
>> -   ret = ov5647_write(sd, OV5647_REG_GAIN_HI, (val >> 8) & 3);
>> -   if (ret)
>> -         return ret;
>> -
>> -   return ov5647_write(sd, OV5647_REG_GAIN_LO, val & 0xff);
>> +   return cci_write(sensor->regmap, OV5647_REG_GAIN, val & 0x3ff, NULL);
>>   }
>>
>>   static int ov5647_s_exposure(struct v4l2_subdev *sd, u32 val)
>>   {
>> -   int ret;
>> +   struct ov5647 *sensor = to_sensor(sd);
>>
>>      /*
>>       * Sensor has 20 bits, but the bottom 4 bits are fractions of a line
>>       * which we leave as zero (and don't receive in "val").
>>       */
>> -   ret = ov5647_write(sd, OV5647_REG_EXP_HI, (val >> 12) & 0xf);
>> -   if (ret)
>> -         return ret;
>> -
>> -   ret = ov5647_write(sd, OV5647_REG_EXP_MID, (val >> 4) & 0xff);
>> -   if (ret)
>> -         return ret;
>> -
>> -   return ov5647_write(sd, OV5647_REG_EXP_LO, (val & 0xf) << 4);
>> +   return cci_write(sensor->regmap, OV5647_REG_EXPOSURE, val << 4, NULL);
>>   }
> We can now drop all contorls functions (except ov5647_s_exposure_auto / ov5647_s_autogain).
> Since there is only a single register write, we can write the register directly
> in the set control(like vblank).

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

ov5647_s_exposure_auto / ov5647_s_autogain) because Hans has already

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

these two functions are no longer needed.

Best Regards,
Xiaolei

>
>>   static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
>> @@ -1254,12 +1154,12 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
>>            ret = ov5647_s_exposure(sd, ctrl->val);
>>            break;
>>      case V4L2_CID_VBLANK:
>> -         ret = ov5647_write16(sd, OV5647_REG_VTS_HI,
>> -                          sensor->mode->format.height + ctrl->val);
>> +         ret = cci_write(sensor->regmap, OV5647_REG_VTS,
>> +                     sensor->mode->format.height + ctrl->val, NULL);
>>            break;
>>      case V4L2_CID_TEST_PATTERN:
>> -         ret = ov5647_write(sd, OV5647_REG_ISPCTRL3D,
>> -                        ov5647_test_pattern_val[ctrl->val]);
>> +         ret = cci_write(sensor->regmap, OV5647_REG_ISPCTRL3D,
>> +                     ov5647_test_pattern_val[ctrl->val], NULL);
>>            break;
>>
>>      /* Read-only, but we adjust it based on mode. */
>> @@ -1435,6 +1335,13 @@ static int ov5647_probe(struct i2c_client *client)
>>      if (ret < 0)
>>            goto ctrl_handler_free;
>>
>> +   sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
>> +   if (IS_ERR(sensor->regmap)) {
>> +         ret = dev_err_probe(dev, PTR_ERR(sensor->regmap),
>> +                         "Failed to init CCI\n");
>> +         goto entity_cleanup;
>> +   }
>> +
>>      ret = ov5647_power_on(dev);
>>      if (ret)
>>            goto entity_cleanup;
>> --
>> 2.43.0
> with above changes
>
> Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>
>
> Best Regards,
> Tarang