Re: [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers
From: Laurent Pinchart
Date: Mon Dec 29 2025 - 08:05:34 EST
On Mon, Dec 29, 2025 at 12:37:41PM +0000, Tarang Raval wrote:
> > Use the new common CCI register access helpers to replace the private
> > register access helpers in the ov5647 driver. This simplifies the driver
> > by reducing the amount of code.
> >
> > Signed-off-by: Xiaolei Wang <xiaolei.wang@xxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/Kconfig | 1 +
> > drivers/media/i2c/ov5647.c | 997 +++++++++++++++++--------------------
> > 2 files changed, 453 insertions(+), 545 deletions(-)
> >
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 4b4db8c4f496..cce63349e71e 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -529,6 +529,7 @@ config VIDEO_OV5645
> >
> > config VIDEO_OV5647
> > tristate "OmniVision OV5647 sensor support"
> > + select V4L2_CCI_I2C
> > help
> > This is a Video4Linux2 sensor driver for the OmniVision
> > OV5647 camera.
> > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > index e193fef4fced..fd69f1616794 100644
> > --- a/drivers/media/i2c/ov5647.c
> > +++ b/drivers/media/i2c/ov5647.c
> > @@ -22,6 +22,7 @@
> > #include <linux/pm_runtime.h>
> > #include <linux/slab.h>
> > #include <linux/videodev2.h>
> > +#include <media/v4l2-cci.h>
> > #include <media/v4l2-ctrls.h>
> > #include <media/v4l2-device.h>
> > #include <media/v4l2-event.h>
> > @@ -41,24 +42,19 @@
> > #define MIPI_CTRL00_BUS_IDLE BIT(2)
> > #define MIPI_CTRL00_CLOCK_LANE_DISABLE BIT(0)
> >
> > -#define OV5647_SW_STANDBY 0x0100
> > -#define OV5647_SW_RESET 0x0103
> > -#define OV5647_REG_CHIPID_H 0x300a
> > -#define OV5647_REG_CHIPID_L 0x300b
> > -#define OV5640_REG_PAD_OUT 0x300d
> > -#define OV5647_REG_EXP_HI 0x3500
> > -#define OV5647_REG_EXP_MID 0x3501
> > -#define OV5647_REG_EXP_LO 0x3502
> > -#define OV5647_REG_AEC_AGC 0x3503
> > -#define OV5647_REG_GAIN_HI 0x350a
> > -#define OV5647_REG_GAIN_LO 0x350b
> > -#define OV5647_REG_VTS_HI 0x380e
> > -#define OV5647_REG_VTS_LO 0x380f
> > -#define OV5647_REG_FRAME_OFF_NUMBER 0x4202
> > -#define OV5647_REG_MIPI_CTRL00 0x4800
> > -#define OV5647_REG_MIPI_CTRL14 0x4814
> > -#define OV5647_REG_AWB 0x5001
> > -#define OV5647_REG_ISPCTRL3D 0x503d
> > +#define OV5647_SW_STANDBY CCI_REG8(0x0100)
> > +#define OV5647_SW_RESET CCI_REG8(0x0103)
> > +#define OV5647_REG_CHIPID CCI_REG16(0x300a)
> > +#define OV5640_REG_PAD_OUT CCI_REG8(0x300d)
> > +#define OV5647_REG_EXPOSURE CCI_REG24(0x3500)
> > +#define OV5647_REG_AEC_AGC CCI_REG8(0x3503)
> > +#define OV5647_REG_GAIN CCI_REG16(0x350b)
>
> It should be 0x350a, not 0x350b.
>
> > +#define OV5647_REG_VTS CCI_REG16(0x380e)
> > +#define OV5647_REG_FRAME_OFF_NUMBER CCI_REG8(0x4202)
> > +#define OV5647_REG_MIPI_CTRL00 CCI_REG8(0x4800)
> > +#define OV5647_REG_MIPI_CTRL14 CCI_REG8(0x4814)
> > +#define OV5647_REG_AWB CCI_REG8(0x5001)
> > +#define OV5647_REG_ISPCTRL3D CCI_REG8(0x503d)
> >
> > #define REG_TERM 0xfffe
> > #define VAL_TERM 0xfe
> > @@ -81,23 +77,19 @@
> > #define OV5647_EXPOSURE_DEFAULT 1000
> > #define OV5647_EXPOSURE_MAX 65535
>
> ...
>
> > @@ -1435,6 +1335,13 @@ static int ov5647_probe(struct i2c_client *client)
> > if (ret < 0)
> > goto ctrl_handler_free;
> >
> > + sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> > + if (IS_ERR(sensor->regmap)) {
> > + ret = PTR_ERR(sensor->regmap);
> > + dev_err(dev, "failed to initialize CCI: %d\n", ret);
>
> Use return dev_err_probe();
dev_err_probe() is fine, but goto entity_cleanup is needed.
> > + goto entity_cleanup;
> > + }
> > +
> > ret = ov5647_power_on(dev);
> > if (ret)
> > goto entity_cleanup;
--
Regards,
Laurent Pinchart