Re: [PATCH v2 2/2] media: i2c: ov5645: Convert to CCI register access helpers

[Lad, Prabhakar]

Hi Xiaolei,

Thank you for the review.

On Sat, Mar 28, 2026 at 3:41 PM xiaolei wang <xiaolei.wang@xxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> Thanks for the update.
>
> On 3/28/26 21:29, Prabhakar 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.
> >
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Convert the ov5645 driver to use the V4L2 CCI register access helpers
> > and regmap infrastructure instead of the custom I2C register access
> > implementation.
> >
> > Keep ov5645_set_register_array() as ov5645_global_init_setting requires
> > a delay between specific register writes, which cannot be expressed
> > through the generic CCI multi-write helper.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > v1->v2
> > - Fixed selecting V4L2_CCI_I2C config option for the OV5645 driver.
> > - Fixed checkpatch warnings limiting to 80 characters per line.
> > ---
> >   drivers/media/i2c/Kconfig  |   1 +
> >   drivers/media/i2c/ov5645.c | 907 ++++++++++++++++++-------------------
> >   2 files changed, 435 insertions(+), 473 deletions(-)
> >
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 20482be35f26..8d7dafba85ca 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -533,6 +533,7 @@ config VIDEO_OV5640
> >   config VIDEO_OV5645
> >          tristate "OmniVision OV5645 sensor support"
> >          depends on OF
> > +       select V4L2_CCI_I2C
> >          help
> >            This is a Video4Linux2 sensor driver for the OmniVision
> >            OV5645 camera.
> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> > index df9001fce44d..1cfccbdf1406 100644
> > --- a/drivers/media/i2c/ov5645.c
> > +++ b/drivers/media/i2c/ov5645.c
> > @@ -25,40 +25,42 @@
> >   #include <linux/of.h>
> >   #include <linux/of_graph.h>
> >   #include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> >   #include <linux/regulator/consumer.h>
> >   #include <linux/slab.h>
> >   #include <linux/types.h>
> >   #include <media/mipi-csi2.h>
> > +#include <media/v4l2-cci.h>
> >   #include <media/v4l2-ctrls.h>
> >   #include <media/v4l2-fwnode.h>
> >   #include <media/v4l2-subdev.h>
> >
> > -#define OV5645_SYSTEM_CTRL0            0x3008
> > +#define OV5645_SYSTEM_CTRL0            CCI_REG8(0x3008)
> >   #define                OV5645_SYSTEM_CTRL0_START       0x02
> >   #define                OV5645_SYSTEM_CTRL0_STOP        0x42
> > -#define OV5645_CHIP_ID_HIGH            0x300a
> > +#define OV5645_CHIP_ID_HIGH            CCI_REG8(0x300a)
> >   #define                OV5645_CHIP_ID_HIGH_BYTE        0x56
> > -#define OV5645_CHIP_ID_LOW             0x300b
> > +#define OV5645_CHIP_ID_LOW             CCI_REG8(0x300b)
> Since 0x300a and 0x300b are contiguous, you could simplify this to a
> single CCI_REG16 read:
>
>      #define OV5645_CHIP_ID        CCI_REG16(0x300a)
>
>      #define OV5645_CHIP_ID_VALUE    0x5645
>
Agreed, I will update it as described above and post a new version.

Cheers,
Prabhakar