Re: [PATCH v7 2/2] media: i2c: add ov2735 image sensor driver
From: Hardevsinh Palaniya
Date: Tue Aug 19 2025 - 06:08:53 EST
Hi Sakari,
> Hi Hardev,
>
> Thanks for the update; a few more comments below.
>
> On Mon, Aug 11, 2025 at 02:28:05PM +0530, Hardevsinh Palaniya wrote:
> > Add a v4l2 subdevice driver for the Omnivision OV2735 sensor.
> >
> > The Omnivision OV2735 is a 1/2.7-Inch CMOS image sensor with an
> > active array size of 1920 x 1080.
> >
> > The following features are supported:
> > - Manual exposure an gain control support
> > - vblank/hblank control support
> > - Test pattern support control
> > - Supported resolution: 1920 x 1080 @ 30fps (SGRBG10)
> >
> > Co-developed-by: Himanshu Bhavani <himanshu.bhavani@xxxxxxxxxxxxxxxxx>
> > Signed-off-by: Himanshu Bhavani <himanshu.bhavani@xxxxxxxxxxxxxxxxx>
> > Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@xxxxxxxxxxxxxxxxx>
> > ---
> > MAINTAINERS | 1 +
> > drivers/media/i2c/Kconfig | 10 +
> > drivers/media/i2c/Makefile | 1 +
> > drivers/media/i2c/ov2735.c | 1073 ++++++++++++++++++++++++++++++++++++
> > 4 files changed, 1085 insertions(+)
> > create mode 100644 drivers/media/i2c/ov2735.c
...
> > + { OV2735_REG_CLK_MODE, 0x15 },
> > + { OV2735_REG_CLOCK_REG1, 0x01 },
> > + { OV2735_REG_CLOCK_REG2, 0x20 },
> > + { OV2735_REG_BINNING_DAC_CODE_MODE, 0x00 },
> > + { OV2735_PAGE_REG8(0x01, 0xfb), 0x73 },
> > + { OV2735_REG_FRAME_SYNC, 0x01 },
> > +
> > + /* Timing ctrl */
> > + { OV2735_PAGE_REG8(0x01, 0x1a), 0x6b },
> > + { OV2735_PAGE_REG8(0x01, 0x1c), 0xea },
> > + { OV2735_PAGE_REG8(0x01, 0x16), 0x0c },
> > + { OV2735_PAGE_REG8(0x01, 0x21), 0x00 },
> > + { OV2735_PAGE_REG8(0x01, 0x11), 0x63 },
> > + { OV2735_PAGE_REG8(0x01, 0x19), 0xc3 },
> > +
> > + /* Analog ctrl */
> > + { OV2735_PAGE_REG8(0x01, 0x26), 0x5a },
> > + { OV2735_PAGE_REG8(0x01, 0x29), 0x01 },
> > + { OV2735_PAGE_REG8(0x01, 0x33), 0x6f },
> > + { OV2735_PAGE_REG8(0x01, 0x2a), 0xd2 },
> > + { OV2735_PAGE_REG8(0x01, 0x2c), 0x40 },
> > + { OV2735_PAGE_REG8(0x01, 0xd0), 0x02 },
> > + { OV2735_PAGE_REG8(0x01, 0xd1), 0x01 },
> > + { OV2735_PAGE_REG8(0x01, 0xd2), 0x20 },
> > + { OV2735_PAGE_REG8(0x01, 0xd3), 0x04 },
> > + { OV2735_PAGE_REG8(0x01, 0xd4), 0x2a },
> > + { OV2735_PAGE_REG8(0x01, 0x50), 0x00 },
> > + { OV2735_PAGE_REG8(0x01, 0x51), 0x2c },
> > + { OV2735_PAGE_REG8(0x01, 0x52), 0x29 },
> > + { OV2735_PAGE_REG8(0x01, 0x53), 0x00 },
> > + { OV2735_PAGE_REG8(0x01, 0x55), 0x44 },
> > + { OV2735_PAGE_REG8(0x01, 0x58), 0x29 },
> > + { OV2735_PAGE_REG8(0x01, 0x5a), 0x00 },
> > + { OV2735_PAGE_REG8(0x01, 0x5b), 0x00 },
> > + { OV2735_PAGE_REG8(0x01, 0x5d), 0x00 },
> > + { OV2735_PAGE_REG8(0x01, 0x64), 0x2f },
> > + { OV2735_PAGE_REG8(0x01, 0x66), 0x62 },
> > + { OV2735_PAGE_REG8(0x01, 0x68), 0x5b },
> > + { OV2735_PAGE_REG8(0x01, 0x75), 0x46 },
> > + { OV2735_PAGE_REG8(0x01, 0x76), 0x36 },
> > + { OV2735_PAGE_REG8(0x01, 0x77), 0x4f },
> > + { OV2735_PAGE_REG8(0x01, 0x78), 0xef },
> > + { OV2735_PAGE_REG8(0x01, 0x72), 0xcf },
> > + { OV2735_PAGE_REG8(0x01, 0x73), 0x36 },
> > + { OV2735_PAGE_REG8(0x01, 0x7d), 0x0d },
> > + { OV2735_PAGE_REG8(0x01, 0x7e), 0x0d },
> > + { OV2735_PAGE_REG8(0x01, 0x8a), 0x77 },
> > + { OV2735_PAGE_REG8(0x01, 0x8b), 0x77 },
>
> Are the names of the above registers not known, or what's the case here?
These are timing and analog control related registers.
I do have the register names from the datasheet. For some of them I also know
the functionality, but for a few I am not entirely sure how they impact the
image, so I have left them as-is. If you prefer, I can replace the raw addresses
with the actual register names.
> > +
> > + { OV2735_REG_HS_MIPI, 0x83 },
> > + { OV2735_REG_MIPI_CTRL5, 0x0b },
> > + { OV2735_REG_MIPI_CTRL6, 0x14 },
> > + { OV2735_PAGE_REG8(0x01, 0x9d), 0x40 },
> > + { OV2735_REG_MIPI_CTRL3, 0x05 },
> > + { OV2735_REG_MIPI_CTRL2, 0x44 },
> > + { OV2735_REG_PREPARE, 0x33 },
> > + { OV2735_REG_R_HS_ZERO, 0x1f },
> > + { OV2735_REG_TRAIL, 0x45 },
> > + { OV2735_REG_R_CLK_ZERO, 0x10 },
> > + { OV2735_REG_MIPI_CTRL7, 0x70 },
> > + { OV2735_REG_ANALOG_CTRL3, 0xe0 },
> > + { OV2735_REG_VNCP, 0x7b },
> > +
> > + /* BLC */
> > + { OV2735_REG_BLC_GAIN_BLUE, 0x77 },
> > + { OV2735_REG_BLC_GAIN_GB, 0x77 },
> > + { OV2735_REG_BLC_GAIN_RED, 0x74 },
> > + { OV2735_REG_BLC_GAIN_GR, 0x74 },
> > + { OV2735_REG_BLC_BPC_TH_P, 0xe0 },
> > + { OV2735_REG_BLC_BPC_TH_N, 0xe0 },
> > + { OV2735_REG_GB_SUBOFFSET, 0x40 },
> > + { OV2735_REG_BLUE_SUBOFFSET, 0x40 },
> > + { OV2735_REG_RED_SUBOFFSET, 0x40 },
> > + { OV2735_REG_GR_SUBOFFSET, 0x40 },
> > +};
...
> > +static int ov2735_page_access(struct ov2735 *ov2735, u32 reg, int *err)
> > +{
> > + u8 page = reg >> CCI_REG_PRIVATE_SHIFT;
> > + int ret = 0;
> > +
> > + if (err && *err)
> > + return *err;
> > +
> > + guard(mutex)(&ov2735->page_lock);
> > +
> > + /* Perform page access before read/write */
> > + if (ov2735->current_page != page) {
>
> You could check if these are equal and return 0 here if they are. Up to
> you.
If these are equal, then ret already remains 0, so it will return 0.
But I am fine either way, whatever you prefer.
> > + ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, page, err);
> > + if (!ret)
> > + ov2735->current_page = page;
> > + }
> > +
> > + return ret;
> > +}
...
> > +static int ov2735_power_on(struct device *dev)
> > +{
> > + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > + struct ov2735 *ov2735 = to_ov2735(sd);
> > + int ret;
> > +
> > + gpiod_set_value_cansleep(ov2735->enable_gpio, 0);
>
> Isn't enable gpio state already low here?
>
> > + /*
> > + * Ensure device is inactive (PWDN high) before enabling power rails.
> > + * As per datasheet, the PWDN pin (named 'enable_gpio' here) should be
> > + * pulled low only after all power rails are stable.
> > + */
> > + fsleep(3 * USEC_PER_MSEC);
>
> This is an unusual place to have a delay in a power-up sequence. Is it
> implemented according to the datasheet?
You are right, the enable GPIO is already high at this point.
According to the datasheet, the PWDN pin (mapped to enable_gpio) must be high
before enabling any regulators. Since this GPIO is active-low, driving it low
keeps PWDN high as required.
The extra fsleep(3 * USEC_PER_MSEC) was added as a guard delay, but it is not
mandated by the datasheet and can be safely removed.
In Datasheet
Power-up sequence:
|--------------------------------------
avdd ----|
<-T1->
|--------------------------------
dovdd ---------|
<-T2->
|--------------------------
dvdd ------------------|
<-T3->
|---------------------|
pwdn -----| |-------------------
<-T4->
|-------------
rst ------------------------------------|
> > +
> > + ret = regulator_bulk_enable(ARRAY_SIZE(ov2735_supply_name),
> > + ov2735->supplies);
> > + if (ret) {
> > + dev_err(ov2735->dev, "failed to enable regulators\n");
> > + return ret;
> > + }
> > +
> > + gpiod_set_value_cansleep(ov2735->enable_gpio, 1);
> > + /* T4: delay from PWDN pulling low to RSTB pulling high */
> > + fsleep(4 * USEC_PER_MSEC);
> > +
> > + ret = clk_prepare_enable(ov2735->xclk);
> > + if (ret) {
> > + dev_err(ov2735->dev, "failed to enable clock\n");
> > + goto err_regulator_off;
> > + }
> > +
> > + gpiod_set_value_cansleep(ov2735->reset_gpio, 0);
> > + /* T5: delay from RSTB pulling high to first I2C command */
> > + fsleep(5 * USEC_PER_MSEC);
> > +
> > + return 0;
> > +
> > +err_regulator_off:
> > + regulator_bulk_disable(ARRAY_SIZE(ov2735_supply_name), ov2735->supplies);
> > + return ret;
> > +}
...
> > +static DEFINE_RUNTIME_DEV_PM_OPS(ov2735_pm_ops,
> > + ov2735_power_off, ov2735_power_on, NULL);
> > +
> > +static const struct of_device_id ov2735_id[] = {
> > + { .compatible = "ovti,ov2735" },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ov2735_id);
> > +
> > +static struct i2c_driver ov2735_driver = {
> > + .driver = {
> > + .name = "ov2735",
> > + .pm = pm_ptr(&ov2735_pm_ops),
> > + .of_match_table = ov2735_id,
> > + },
> > + .probe = ov2735_probe,
> > + .remove = ov2735_remove,
> > +};
> > +module_i2c_driver(ov2735_driver);
> > +
> > +MODULE_DESCRIPTION("OV2735 Camera Sensor Driver");
> > +MODULE_AUTHOR("Hardevsinh Palaniya <hardevsinh.palaniya@xxxxxxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Himanshu Bhavani <himanshu.bhavani@xxxxxxxxxxxxxxxxx>");
> > +MODULE_LICENSE("GPL");
>
> --
> Kind regards,
>
> Sakari Ailus
Best Regards,
Hardev