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

["Yan, Dongcheng" <dongcheng.yan@intel.com>]

Hi Hardevsinh,

On 7/8/2025 3:04 PM, Hardevsinh Palaniya wrote:
> Hi Andy,
> 
> Thanks for the review.
> 
> I have corrected the code, and all of your comments have now been addressed.
> 
> However, I have a question about one of the comments. Please see below. 
> 
>> On Mon, Jul 07, 2025 at 08:31:06PM +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.
>>>
>>> - Manual exposure an gain control support
>>> - vblank/hblank control support
>>> - Supported resolution: 1920 x 1080 @ 30fps (SGRBG10)
>>
>> ...
>>
>>> +#include <linux/clk.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/module.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <linux/units.h>
>>
>> More stuff is in use than just these headers provide.
>> E.g.,
>>
>> + array_size.h
>> + container_of.h
>> + gpio/consumer.h
>> + types.h
>>
>> And so on... Also in some cases the forward declarations are enough to have.
>>
>> .,,
>>
>>> +#define OV2735_LINK_FREQ_420MHZ                      420000000
>>
>> HZ_PER_MHZ ?
>>
>> ...
>>
>>> +#define OV2735_PIXEL_RATE                    168000000
>>
>> What's the unit?
>>
>> ...
>>
>>> +static const s64 link_freq_menu_items[] = {
>>> +     OV2735_LINK_FREQ_420MHZ
>>
>> Keep the trailing comma like you have done in other cases.
>>
>>> +};
>>
>> ...
>>
>>> +static int ov2735_enable_test_pattern(struct ov2735 *ov2735, u32 pattern)
>>> +{
>>> +     int ret;
>>> +     u64 val;
>>> +
>>> +     ret = cci_read(ov2735->cci, OV2735_REG_TEST_PATTERN, &val, NULL);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     switch (pattern) {
>>> +     case 0:
>>> +             val &= ~OV2735_TEST_PATTERN_ENABLE;
>>> +             break;
>>> +     case 1:
>>> +             val |= OV2735_TEST_PATTERN_ENABLE;
>>> +             break;
>>> +     }
>>
>>> +     ret = cci_write(ov2735->cci, OV2735_REG_TEST_PATTERN, val, NULL);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     return 0;
>>
>> Is this the required style? Because these 5 LoCs is just a simple
>>
>>         return cci_write(ov2735->cci, OV2735_REG_TEST_PATTERN, val, NULL);
>>
>>> +}
>>
>> ...
>>
>>> +static int ov2735_set_ctrl(struct v4l2_ctrl *ctrl)
>>> +{
>>> +     struct ov2735 *ov2735 = container_of(ctrl->handler, struct ov2735,
>>> +                                          handler);
>>> +     const struct ov2735_mode *mode;
>>> +     struct v4l2_mbus_framefmt *fmt;
>>> +     struct v4l2_subdev_state *state;
>>
>>> +     int vts;
>>
>> Can be negative?
>>
>>> +     int ret = 0;
>>
>> How is this assignment useful?
>>
>>> +     state = v4l2_subdev_get_locked_active_state(&ov2735->sd);
>>> +     fmt = v4l2_subdev_state_get_format(state, 0);
>>> +
>>> +     mode = v4l2_find_nearest_size(supported_modes,
>>> +                                   ARRAY_SIZE(supported_modes),
>>> +                                   width, height,
>>> +                                   fmt->width, fmt->height);
>>> +
>>> +     if (ctrl->id == V4L2_CID_VBLANK) {
>>> +             /* Honour the VBLANK limits when setting exposure. */
>>> +             s64 max = mode->height + ctrl->val - 4;
>>> +
>>> +             ret = __v4l2_ctrl_modify_range(ov2735->exposure,
>>> +                                            ov2735->exposure->minimum, max,
>>> +                                      ov2735->exposure->step,
>>> +                                      ov2735->exposure->default_value);
>>> +             if (ret)
>>> +                     return ret;
>>> +     }
>>> +
>>> +     /*
>>> +      * Applying V4L2 control value only happens
>>> +      * when power is up for streaming
>>
>> Multi-line comments shouldn't neglect punctuation.
>>
>>> +      */
>>> +     if (pm_runtime_get_if_in_use(ov2735->dev) == 0)
>>> +             return 0;
>>> +
>>> +     ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, 0x01, NULL);
>>> +
>>> +     switch (ctrl->id) {
>>> +     case V4L2_CID_EXPOSURE:
>>> +             ret |= cci_write(ov2735->cci, OV2735_REG_LONG_EXPOSURE, ctrl->val, NULL);
>>> +             break;
>>> +     case V4L2_CID_ANALOGUE_GAIN:
>>> +             ret |= cci_write(ov2735->cci, OV2735_REG_ANALOG_GAIN, ctrl->val, NULL);
>>> +             break;
>>> +     case V4L2_CID_HBLANK:
>>> +             ret |= cci_write(ov2735->cci, OV2735_REG_HBLANK, ctrl->val, NULL);
>>> +             break;
>>> +     case V4L2_CID_VBLANK:
>>> +             vts = ctrl->val + mode->height;
>>> +             ret |= cci_write(ov2735->cci, OV2735_REG_FRAME_EXP_SEPERATE_EN,
>>> +                              OV2735_FRAME_EXP_SEPERATE_EN, NULL);
>>> +             ret |= cci_write(ov2735->cci, OV2735_REG_FRAME_LENGTH, vts, NULL);
>>> +             break;
>>> +     case V4L2_CID_TEST_PATTERN:
>>> +             ret = ov2735_enable_test_pattern(ov2735, ctrl->val);
>>> +             break;
>>> +     default:
>>> +             dev_err(ov2735->dev, "ctrl(id:0x%x, val:0x%x) is not handled\n",
>>> +                     ctrl->id, ctrl->val);
>>> +             break;
>>> +     }
>>> +     ret |= cci_write(ov2735->cci, OV2735_REG_FRAME_SYNC, 0x01, NULL);
>>> +
>>> +     pm_runtime_put(ov2735->dev);
>>> +
>>> +     return ret;
>>> +}
>>
>> ...
>>
>>> +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, 7);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ov2735->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops, V4L2_CID_PIXEL_RATE,
>>> +                                            0, OV2735_PIXEL_RATE, 1, OV2735_PIXEL_RATE);
>>
>> Besides it's too long, it has trailing space.
>>
>>> +
>>> +     ov2735->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &ov2735_ctrl_ops,
>>> +                                                V4L2_CID_LINK_FREQ,
>>> +                                                0, 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);
>>> +
>>> +     vblank_def = mode->vts_def - mode->height;
>>> +     ov2735->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops,
>>> +                                        V4L2_CID_VBLANK, vblank_def,
>>> +                             OV2735_VTS_MAX - mode->height,
>>> +                             1, vblank_def);
>>
>> It's weird indentation.
>>
>>> +
>>> +     exp_max = mode->vts_def - 4;
>>> +     ov2735->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops,
>>> +                                          V4L2_CID_EXPOSURE, OV2735_EXPOSURE_MIN,
>>> +                             exp_max, OV2735_EXPOSURE_STEP, mode->exp_def);
>>> +
>>> +     ov2735->gain = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops,
>>> +                                      V4L2_CID_ANALOGUE_GAIN, ANALOG_GAIN_MIN,
>>> +                             ANALOG_GAIN_MAX, ANALOG_GAIN_STEP,
>>> +                             ANALOG_GAIN_DEFAULT);
>>
>> Ditto.
>>
>>> +     ov2735->test_pattern = v4l2_ctrl_new_std_menu_items(ctrl_hdlr,
>>> +                                                         &ov2735_ctrl_ops, V4L2_CID_TEST_PATTERN,
>>> +                     ARRAY_SIZE(ov2735_test_pattern_menu) - 1,
>>> +                     0, 0, ov2735_test_pattern_menu);
>>
>> Ditto.
>>
>>> +     if (ctrl_hdlr->error) {
>>> +             ret = ctrl_hdlr->error;
>>> +             dev_err(ov2735->dev, "control init failed (%d)\n", ret);
>>> +             goto error;
>>> +     }
>>> +
>>> +     ret = v4l2_fwnode_device_parse(ov2735->dev, &props);
>>> +     if (ret)
>>> +             goto error;
>>> +
>>> +     ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov2735_ctrl_ops,
>>> +                                           &props);
>>> +     if (ret)
>>> +             goto error;
>>> +
>>> +     ov2735->sd.ctrl_handler = ctrl_hdlr;
>>> +
>>> +     return 0;
>>> +error:
>>
>> Usual way of naming labels is to explain what is going to  happen when goto.
>> Moreover it's even inconsistent, the below code use err prefix, but better
>> naming.
>>
>> Here the
>>
>> err_handler_free:
>>
>> for example is better.
>>
>>> +     v4l2_ctrl_handler_free(ctrl_hdlr);
>>> +
>>> +     return ret;
>>> +}
>>
>> ...
>>
>>> +static int ov2735_enable_streams(struct v4l2_subdev *sd,
>>> +                              struct v4l2_subdev_state *state, u32 pad,
>>> +                             u64 streams_mask)
>>
>> Indentation issue.
>>
>>> +{
>>> +     struct ov2735 *ov2735 = to_ov2735(sd);
>>> +     const struct ov2735_mode *mode;
>>> +     const struct ov2735_reglist *reg_list;
>>> +     const struct v4l2_mbus_framefmt *fmt;
>>> +     int ret = 0;
>>
>> Needless assignment.
>>
>>> +     fmt = v4l2_subdev_state_get_format(state, 0);
>>> +     mode = v4l2_find_nearest_size(supported_modes,
>>> +                                   ARRAY_SIZE(supported_modes),
>>> +                                   width, height,
>>> +                                   fmt->width, fmt->height);
>>> +
>>> +     ret = pm_runtime_resume_and_get(ov2735->dev);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     reg_list = &mode->reg_list;
>>> +     ret = cci_multi_reg_write(ov2735->cci, reg_list->regvals, reg_list->num_regs, NULL);
>>> +     if (ret) {
>>> +             dev_err(ov2735->dev, "%s failed to send mfg header\n", __func__);
>>> +             goto err_rpm_put;
>>> +     }
>>> +
>>> +     /* Apply customized values from user */
>>> +     ret =  __v4l2_ctrl_handler_setup(ov2735->sd.ctrl_handler);
>>> +     if (ret)
>>> +             goto err_rpm_put;
>>> +
>>> +     /* set stream on register */
>>> +     ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, 0x01, NULL);
>>> +     ret |= cci_write(ov2735->cci, OV2735_REG_STREAM_CTRL, OV2735_STREAM_ON, NULL);
>>> +     if (ret)
>>> +             goto err_rpm_put;
>>> +
>>> +     return 0;
>>> +
>>> +err_rpm_put:
>>> +     pm_runtime_put(ov2735->dev);
>>> +     return ret;
>>> +}
>>
>> ...
>>
>>> +static int ov2735_disable_streams(struct v4l2_subdev *sd,
>>> +                               struct v4l2_subdev_state *state, u32 pad,
>>> +                               u64 streams_mask)
>>> +{
>>> +     struct ov2735 *ov2735 = to_ov2735(sd);
>>> +     int ret = 0;
>>> +
>>> +     /* set stream off register */
>>> +     ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, 0x01, NULL);
>>> +     ret |= cci_write(ov2735->cci, OV2735_REG_STREAM_CTRL, OV2735_STREAM_OFF, NULL);
>>
>> Why not using the ret parameter? Same for other similar cases above and beyond.
> 
> I am not sure what you want to suggest here.
> 
> Do I need to check ret like this?
> 
> ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, 0x01, NULL);
> if (ret) {
>     // error message
> }
> 
> ret = cci_write(ov2735->cci, OV2735_REG_STREAM_CTRL, OV2735_STREAM_OFF, NULL);
> if (ret) {
>     // error message
> }
> 

cci_write/read has a param named ret, for example:
cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, 0x01, &ret);
cci_write(ov2735->cci, OV2735_REG_STREAM_CTRL, OV2735_STREAM_OFF, &ret);
then check ret here or return ret.

Thanks,
Dongcheng> Best Regards,
> Hardev