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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Hardev,

On Fri, Aug 08, 2025 at 12:34:33PM +0000, Hardevsinh Palaniya wrote:
> 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@siliconsignals.io>
> > > Signed-off-by: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> > > Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> > > ---
> > >  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.

How about OV2735_PLL_CTRL_ENABLE?

>  
> > > +#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. 

-- 
Regards,

Sakari Ailus