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

[Tarang Raval <tarang.raval@siliconsignals.io>]

Hi Elgin,

Thanks for sending these drivers.

A few minor points are listed below, please take a look.

> Add a v4l2 subdevice driver for the Omnivision os02g10 sensor.
>
> The Omnivision os02g10 is a 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
> - vflip/hflip control support
> - Test pattern control support
> - Supported resolution: 1920 x 1080 @ 30fps (SBGGR10)
>
> Signed-off-by: Elgin Perumbilly <elgin.perumbilly@siliconsignals.io>
> ---
>  MAINTAINERS                 |    1 +
>  drivers/media/i2c/Kconfig   |   10 +
>  drivers/media/i2c/Makefile  |    1 +
>  drivers/media/i2c/os02g10.c | 1010 +++++++++++++++++++++++++++++++++++
>  4 files changed, 1022 insertions(+)
>  create mode 100644 drivers/media/i2c/os02g10.c

...

> +#define OS02G10_REG_SIF_CTRL                   OS02G10_PAGE_REG8(0x02, 0x5e)
> +#define OS02G10_ORIENTATION_BAYER_FIX          0x32

The above two definitions belong to Page 2, Please move them to the Page 2
block below.

> +/* Page 2 */
> +#define OS02G10_REG_V_START                    OS02G10_PAGE_REG16(0x02, 0xa0)
> +#define OS02G10_REG_V_SIZE                     OS02G10_PAGE_REG16(0x02, 0xa2)
> +#define OS02G10_REG_H_START                    OS02G10_PAGE_REG16(0x02, 0xa4)
> +#define OS02G10_REG_H_SIZE                     OS02G10_PAGE_REG16(0x02, 0xa6)
> +
> +#define OS02G10_LINK_FREQ_720MHZ               (720 * HZ_PER_MHZ)

...

> +static int os02g10_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +       struct os02g10 *os02g10 = container_of_const(ctrl->handler,
> +                                                    struct os02g10, handler);
> +       struct v4l2_subdev_state *state;
> +       struct v4l2_mbus_framefmt *fmt;
> +       int ret = 0;
> +
> +       state = v4l2_subdev_get_locked_active_state(&os02g10->sd);
> +       fmt = v4l2_subdev_state_get_format(state, 0);
> +
> +       if (ctrl->id == V4L2_CID_VBLANK) {
> +               /* Honour the VBLANK limits when setting exposure */
> +               s64 max = fmt->height + ctrl->val - OS02G10_EXPOSURE_MARGIN;
> +
> +               ret = __v4l2_ctrl_modify_range(os02g10->exposure,
> +                                              os02g10->exposure->minimum, max,
> +                                              os02g10->exposure->step,
> +                                              os02g10->exposure->default_value);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       if (pm_runtime_get_if_in_use(os02g10->dev) == 0)

Please use pm_runtime_get_if_active.

> +               return 0;
> +
> +       switch (ctrl->id) {
> +       case V4L2_CID_EXPOSURE:
> +               os02g10_write(os02g10, OS02G10_REG_LONG_EXPOSURE, ctrl->val, &ret);
> +               break;
> +       case V4L2_CID_ANALOGUE_GAIN:
> +               os02g10_write(os02g10, OS02G10_REG_ANALOG_GAIN, ctrl->val, &ret);
> +               break;
> +       case V4L2_CID_DIGITAL_GAIN:
> +               os02g10_write(os02g10, OS02G10_REG_DIGITAL_GAIN_L,
> +                             (ctrl->val & 0xff), &ret);
> +               os02g10_write(os02g10, OS02G10_REG_DIGITAL_GAIN_H,
> +                             ((ctrl->val >> 8) & 0x7), &ret);
> +               break;
> +       case V4L2_CID_VBLANK:
> +               u64 vts = ctrl->val + fmt->height;
> +
> +               os02g10_update_bits(os02g10, OS02G10_REG_FRAME_EXP_SEPERATE_EN,
> +                                   OS02G10_FRAME_EXP_SEPERATE_EN,
> +                                   OS02G10_FRAME_EXP_SEPERATE_EN, &ret);
> +               os02g10_write(os02g10, OS02G10_REG_FRAME_LENGTH, vts, &ret);
> +               break;
> +       case V4L2_CID_HFLIP:
> +       case V4L2_CID_VFLIP:
> +               os02g10_write(os02g10, OS02G10_REG_FLIP_MIRROR,
> +                             os02g10->hflip->val | os02g10->vflip->val << 1,
> +                             &ret);
> +               os02g10_write(os02g10, OS02G10_REG_SIF_CTRL,
> +                             OS02G10_ORIENTATION_BAYER_FIX, &ret);
> +               break;
> +       case V4L2_CID_TEST_PATTERN:
> +               os02g10_update_bits(os02g10,
> +                                   OS02G10_REG_TEST_PATTERN,
> +                                   OS02G10_TEST_PATTERN_ENABLE,
> +                                   ctrl->val ? OS02G10_TEST_PATTERN_ENABLE : 0,
> +                                   &ret);
> +               break;
> +       default:
> +               ret = -EINVAL;
> +               break;
> +       }
> +       os02g10_write(os02g10, OS02G10_REG_FRAME_SYNC, 0x01, &ret);
> +
> +       pm_runtime_put(os02g10->dev);
> +
> +       return ret;
> +}

...

> +static int os02g10_init_controls(struct os02g10 *os02g10)
> +{
> +       const struct os02g10_mode *mode = &supported_modes[0];
> +       u64 vblank_def, hblank_def, exp_max, pixel_rate;
> +       struct v4l2_fwnode_device_properties props;
> +       struct v4l2_ctrl_handler *ctrl_hdlr;
> +       int ret;
> +
> +       ctrl_hdlr = &os02g10->handler;
> +       v4l2_ctrl_handler_init(ctrl_hdlr, 12);
> +
> +       /* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> +       pixel_rate = div_u64(OS02G10_LINK_FREQ_720MHZ * 2 * 2, 10);
> +       v4l2_ctrl_new_std(ctrl_hdlr, &os02g10_ctrl_ops, V4L2_CID_PIXEL_RATE, 0,
> +                         pixel_rate, 1, pixel_rate);
> +
> +       os02g10->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &os02g10_ctrl_ops,
> +                                                   V4L2_CID_LINK_FREQ,
> +                                                   os02g10->link_freq_index,
> +                                                   0, link_freq_menu_items);
> +       if (os02g10->link_freq)
> +               os02g10->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +       hblank_def = mode->hts_def - mode->width;

Please use (mode->hts_def * 2) here, otherwise hblank_def can end up with
negative value.

Also, add a comment mentioning that the datasheet does not provide any
information about the unit of hts.

> +       os02g10->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &os02g10_ctrl_ops,
> +                                           V4L2_CID_HBLANK, hblank_def, hblank_def,
> +                                           1, hblank_def);
> +       if (os02g10->hblank)
> +               os02g10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +       vblank_def = mode->vts_def - mode->height;
> +       os02g10->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &os02g10_ctrl_ops,
> +                                           V4L2_CID_VBLANK, vblank_def,
> +                                           OS02G10_FRAME_LENGTH_MAX - mode->height,
> +                                           1, vblank_def);
> +

...

> +static int os02g10_disable_streams(struct v4l2_subdev *sd,
> +                                  struct v4l2_subdev_state *state, u32 pad,
> +                                  u64 streams_mask)
> +{
> +       struct os02g10 *os02g10 = to_os02g10(sd);
> +       int ret;
> +
> +       ret = os02g10_write(os02g10, OS02G10_REG_STREAM_CTRL,
> +                           OS02G10_STREAM_CTRL_OFF, NULL);
> +       if (ret)
> +               dev_err(os02g10->dev, "%s failed to set stream\n", __func__);

Remove __func__ from the error message; it is unnecessary.

Also, please use a more meaningful error message

> +
> +       __v4l2_ctrl_grab(os02g10->vflip, false);
> +       __v4l2_ctrl_grab(os02g10->hflip, false);
> +
> +       pm_runtime_put(os02g10->dev);
> +
> +       return ret;
> +}
> +
> +static int os02g10_get_selection(struct v4l2_subdev *sd,
> +                      struct v4l2_subdev_state *sd_state,
> +                      struct v4l2_subdev_selection *sel)
> +{
> +   switch (sel->target)
> +   case V4L2_SEL_TGT_CROP:
> +   case V4L2_SEL_TGT_NATIVE_SIZE:
> +         sel->r = os02g10_native_area;

V4L2_SEL_TGT_CROP should use the active area, and V4L2_SEL_TGT_CROP_BOUNDS
should use the native area. Please update this accordingly.

> +         return 0;
> +   case V4L2_SEL_TGT_CROP_DEFAULT:
> +   case V4L2_SEL_TGT_CROP_BOUNDS:
> +         sel->r = os02g10_active_area;
> +         return 0;
> +   default:
> +         return -EINVAL;
> +   }
> +}

...

> +static struct i2c_driver os02g10_driver = {
> +       .driver = {
> +               .name = "os02g10",
> +               .pm = pm_ptr(&os02g10_pm_ops),
> +               .of_match_table = os02g10_id,
> +       },
> +       .probe = os02g10_probe,
> +       .remove = os02g10_remove,
> +};
> +module_i2c_driver(os02g10_driver);
> +
> +MODULE_DESCRIPTION("OS02G10 Camera Sensor Driver");
> +MODULE_AUTHOR("Tarang Raval <tarang.raval@siliconsignals.io>");

You can add your name here as well.

> +MODULE_LICENSE("GPL");
> --
> 2.34.1

Apart from the above comments, the driver looks good to me.

Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>

Best Regards,
Tarang