Re: [PATCH v6 2/2] media: i2c: add ov2735 image sensor driver

[Hardevsinh Palaniya]

Hi Sakari,

Thanks for the review.

> Hi Hardev,
> 
> Thanks for the update. A few more minor comments below.
> 
> On Thu, Jul 31, 2025 at 11:39:58AM +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 | 1071 ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 1083 insertions(+)
> >  create mode 100644 drivers/media/i2c/ov2735.c
> >

...

> > +#define OV2735_XCLK_FREQ                     (24 * HZ_PER_MHZ)
> > +
> > +/* Add page number in CCI private bits [31:28] of the register address */
> > +#define OV2735_PAGE_REG8(p, x)                       (((p) << CCI_REG_PRIVATE_SHIFT) | CCI_REG8(x))
> > +#define OV2735_PAGE_REG16(p, x)                      (((p) << CCI_REG_PRIVATE_SHIFT) | CCI_REG16(x))
> > +
> > +#define OV2735_REG_PAGE_SELECT                       CCI_REG8(0xfd)
> > +
> > +/* Page 0 */
> > +#define OV2735_REG_CHIPID                    OV2735_PAGE_REG16(0x00, 0x02)
> > +#define OV2735_CHIPID                                0x2735
> > +
> > +#define OV2735_REG_SOFT_REST                 OV2735_PAGE_REG8(0x00, 0x20)
> > +
> > +/* Clock Settings */
> > +#define OV2735_REG_PLL_CTRL                  OV2735_PAGE_REG8(0x00, 0x2f)
> > +#define OV2735_REG_PLL_ENABLE                        0x7f
> 
> This register address doesn't use the macro to define one. Why?

This is not a register address but a register value. 
I will correct the naming to OV2735_PLL_ENABLE to make that clear.
 
> > +#define OV2735_REG_PLL_OUTDIV                        OV2735_PAGE_REG8(0x00, 0x34)
> > +#define OV2735_REG_CLK_MODE                  OV2735_PAGE_REG8(0x00, 0x30)
> > +#define OV2735_REG_CLOCK_REG1                        OV2735_PAGE_REG8(0x00, 0x33)
> > +#define OV2735_REG_CLOCK_REG2                        OV2735_PAGE_REG8(0x00, 0x35)
> > +
> > +/* Page 1 */
> > +#define OV2735_REG_STREAM_CTRL                       OV2735_PAGE_REG8(0x01, 0xa0)
> > +#define OV2735_STREAM_ON                     0x01
> > +#define OV2735_STREAM_OFF                    0x00
> 
> It's a good practice to name register values with the register macro as a
> prefix, with "REG_" removed.

This is not a register address but a register value. 

> > +
> > +#define OV2735_REG_UPDOWN_MIRROR             OV2735_PAGE_REG8(0x01, 0x3f)
> > +#define OV2735_REG_BINNING_DAC_CODE_MODE     OV2735_PAGE_REG8(0x01, 0x30)
> > +#define OV2735_REG_FRAME_LENGTH                      OV2735_PAGE_REG16(0x01, 0x0e)
> > +#define OV2735_VTS_MAX                               0x0fff
> > +#define OV2735_REG_FRAME_EXP_SEPERATE_EN     OV2735_PAGE_REG8(0x01, 0x0d)
> > +#define OV2735_FRAME_EXP_SEPERATE_EN         0x10
> > +#define OV2735_REG_FRAME_SYNC                        OV2735_PAGE_REG8(0x01, 0x01)
> > +

...

> > +static int ov2735_init_controls(struct ov2735 *ov2735)
> > +{
> > +     struct v4l2_ctrl_handler *ctrl_hdlr;
> > +     struct v4l2_fwnode_device_properties props;
> > +     const struct ov2735_mode *mode = &supported_modes[0];
> > +     u64 hblank_def, vblank_def, exp_max;
> > +     int ret;
> > +
> > +     ctrl_hdlr = &ov2735->handler;
> > +     ret = v4l2_ctrl_handler_init(ctrl_hdlr, 9);
> > +     if (ret)
> > +             return ret;
> 
> No need to check this here explicitly.
> 
> > +
> > +     ov2735->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops,
> > +                                            V4L2_CID_PIXEL_RATE, 0, OV2735_PIXEL_RATE,
> > +                                            1, OV2735_PIXEL_RATE);
> > +
> > +     ov2735->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &ov2735_ctrl_ops,
> > +                                                V4L2_CID_LINK_FREQ,
> > +                                                ov2735->link_freq_index,
> > +                                                0, link_freq_menu_items);
> > +     if (ov2735->link_freq)
> > +             ov2735->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > +     hblank_def = mode->hts_def - mode->width;
> > +     ov2735->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops, V4L2_CID_HBLANK,
> > +                                        hblank_def, hblank_def, 1, hblank_def);
> 
> Can you run:
> 
>         $ ./scripts/checkpatch.pl --strict --max-line-length=80
> 
> on the patch, please?

I tried to keep lines within 80 columns, but in some cases a slightly longer line 
improves readability. I’ll revisit these and fix where possible to follow the style
guide more closely. 

Best Regards,
Hardev