linux-kernel - Re: [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access helpers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <PN3P287MB1829C61C82B611DB6013B10F8BBFA@PN3P287MB1829.INDP287.PROD.OUTLOOK.COM>
Date: Mon, 29 Dec 2025 13:26:20 +0000
From: Tarang Raval <tarang.raval@...iconsignals.io>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>, Xiaolei Wang
	<xiaolei.wang@...driver.com>
CC: "sakari.ailus@...ux.intel.com" <sakari.ailus@...ux.intel.com>,
	"dave.stevenson@...pberrypi.com" <dave.stevenson@...pberrypi.com>,
	"jacopo@...ndi.org" <jacopo@...ndi.org>, "mchehab@...nel.org"
	<mchehab@...nel.org>, "prabhakar.mahadev-lad.rj@...renesas.com"
	<prabhakar.mahadev-lad.rj@...renesas.com>, "hverkuil+cisco@...nel.org"
	<hverkuil+cisco@...nel.org>, "johannes.goede@....qualcomm.com"
	<johannes.goede@....qualcomm.com>, "hverkuil-cisco@...all.nl"
	<hverkuil-cisco@...all.nl>, "jai.luthra@...asonboard.com"
	<jai.luthra@...asonboard.com>, "linux-media@...r.kernel.org"
	<linux-media@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/3] media: i2c: ov5647: Convert to CCI register access
 helpers

> 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@...driver.com>
> > > ---
> > >  drivers/media/i2c/Kconfig  |   1 +
> > >  drivers/media/i2c/ov5647.c | 997 +++++++++++++++++--------------------
> > >  2 files changed, 453 insertions(+), 545 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > index 4b4db8c4f496..cce63349e71e 100644
> > > --- a/drivers/media/i2c/Kconfig
> > > +++ b/drivers/media/i2c/Kconfig
> > > @@ -529,6 +529,7 @@ config VIDEO_OV5645
> > >
> > >  config VIDEO_OV5647
> > >         tristate "OmniVision OV5647 sensor support"
> > > +       select V4L2_CCI_I2C
> > >         help
> > >           This is a Video4Linux2 sensor driver for the OmniVision
> > >           OV5647 camera.
> > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > index e193fef4fced..fd69f1616794 100644
> > > --- a/drivers/media/i2c/ov5647.c
> > > +++ b/drivers/media/i2c/ov5647.c
> > > @@ -22,6 +22,7 @@
> > >  #include <linux/pm_runtime.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/videodev2.h>
> > > +#include <media/v4l2-cci.h>
> > >  #include <media/v4l2-ctrls.h>
> > >  #include <media/v4l2-device.h>
> > >  #include <media/v4l2-event.h>
> > > @@ -41,24 +42,19 @@
> > >  #define MIPI_CTRL00_BUS_IDLE                   BIT(2)
> > >  #define MIPI_CTRL00_CLOCK_LANE_DISABLE         BIT(0)
> > >
> > > -#define OV5647_SW_STANDBY              0x0100
> > > -#define OV5647_SW_RESET                        0x0103
> > > -#define OV5647_REG_CHIPID_H            0x300a
> > > -#define OV5647_REG_CHIPID_L            0x300b
> > > -#define OV5640_REG_PAD_OUT             0x300d
> > > -#define OV5647_REG_EXP_HI              0x3500
> > > -#define OV5647_REG_EXP_MID             0x3501
> > > -#define OV5647_REG_EXP_LO              0x3502
> > > -#define OV5647_REG_AEC_AGC             0x3503
> > > -#define OV5647_REG_GAIN_HI             0x350a
> > > -#define OV5647_REG_GAIN_LO             0x350b
> > > -#define OV5647_REG_VTS_HI              0x380e
> > > -#define OV5647_REG_VTS_LO              0x380f
> > > -#define OV5647_REG_FRAME_OFF_NUMBER    0x4202
> > > -#define OV5647_REG_MIPI_CTRL00         0x4800
> > > -#define OV5647_REG_MIPI_CTRL14         0x4814
> > > -#define OV5647_REG_AWB                 0x5001
> > > -#define OV5647_REG_ISPCTRL3D           0x503d
> > > +#define OV5647_SW_STANDBY              CCI_REG8(0x0100)
> > > +#define OV5647_SW_RESET                        CCI_REG8(0x0103)
> > > +#define OV5647_REG_CHIPID              CCI_REG16(0x300a)
> > > +#define OV5640_REG_PAD_OUT             CCI_REG8(0x300d)
> > > +#define OV5647_REG_EXPOSURE            CCI_REG24(0x3500)
> > > +#define OV5647_REG_AEC_AGC             CCI_REG8(0x3503)
> > > +#define OV5647_REG_GAIN                        CCI_REG16(0x350b)
> >
> > It should be 0x350a, not 0x350b.
> >
> > > +#define OV5647_REG_VTS                 CCI_REG16(0x380e)
> > > +#define OV5647_REG_FRAME_OFF_NUMBER    CCI_REG8(0x4202)
> > > +#define OV5647_REG_MIPI_CTRL00         CCI_REG8(0x4800)
> > > +#define OV5647_REG_MIPI_CTRL14         CCI_REG8(0x4814)
> > > +#define OV5647_REG_AWB                 CCI_REG8(0x5001)
> > > +#define OV5647_REG_ISPCTRL3D           CCI_REG8(0x503d)
> > >
> > >  #define REG_TERM 0xfffe
> > >  #define VAL_TERM 0xfe
> > > @@ -81,23 +77,19 @@
> > >  #define OV5647_EXPOSURE_DEFAULT                1000
> > >  #define OV5647_EXPOSURE_MAX            65535
> >
> > ...
> >
> > > @@ -1435,6 +1335,13 @@ static int ov5647_probe(struct i2c_client *client)
> > >         if (ret < 0)
> > >                 goto ctrl_handler_free;
> > >
> > > +       sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> > > +       if (IS_ERR(sensor->regmap)) {
> > > +               ret = PTR_ERR(sensor->regmap);
> > > +               dev_err(dev, "failed to initialize CCI: %d\n", ret);
> >
> > Use return dev_err_probe();
> 
> dev_err_probe() is fine, but goto entity_cleanup is needed.

Yes, my bad. 

Keep the existing return path. 

> > > +               goto entity_cleanup;
> > > +       }
> > > +
> > >         ret = ov5647_power_on(dev);
> > >         if (ret)
> > >                 goto entity_cleanup;
> 
> --
> Regards,
> 
> Laurent Pinchart