Hi Bingbu,
On Thu, Apr 13, 2023 at 03:58:30PM +0800, Bingbu Cao wrote:
>
> Thanks for your review.
>
> On 2/9/23 8:40 PM, Dave Stevenson wrote:
> > Hi Bingbu
> >
> > Thanks for the patch. Just a couple of observations as I read through it.
> >
> > On Thu, 9 Feb 2023 at 11:39, Bingbu Cao <bingbu.cao@xxxxxxxxx> wrote:
> >>
> >> Add v4l2 device driver for OmniVision ov01a10 image sensor, ov01a10
> >> image sensor can deliver 1280x800 resolution BGGR10 images at 60 fps.
> >>
> >> Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
> >> Signed-off-by: Yating Wang <yating.wang@xxxxxxxxx>
> >> ---
> >> drivers/media/i2c/Kconfig | 13 +
> >> drivers/media/i2c/Makefile | 1 +
> >> drivers/media/i2c/ov01a10.c | 906 ++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 920 insertions(+)
> >> create mode 100644 drivers/media/i2c/ov01a10.c
> >>
>
> snip
>
> >> +static int ov01a10_init_controls(struct ov01a10 *ov01a10)
> >> +{
> >> + struct v4l2_ctrl_handler *ctrl_hdlr;
> >> + const struct ov01a10_mode *cur_mode;
> >> + s64 exposure_max, h_blank;
> >> + u32 vblank_min, vblank_max, vblank_default;
> >> + int size;
> >> + int ret = 0;
> >> +
> >> + ctrl_hdlr = &ov01a10->ctrl_handler;
> >> + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + ctrl_hdlr->lock = &ov01a10->mutex;
> >> + cur_mode = ov01a10->cur_mode;
> >> + size = ARRAY_SIZE(link_freq_menu_items);
> >> +
> >> + ov01a10->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> >> + &ov01a10_ctrl_ops,
> >> + V4L2_CID_LINK_FREQ,
> >> + size - 1, 0,
> >> + link_freq_menu_items);
> >> + if (ov01a10->link_freq)
> >> + ov01a10->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >> +
> >> + ov01a10->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov01a10_ctrl_ops,
> >> + V4L2_CID_PIXEL_RATE, 0,
> >> + OV01A10_SCLK, 1, OV01A10_SCLK);
> >> +
> >> + vblank_min = cur_mode->vts_min - cur_mode->height;
> >> + vblank_max = OV01A10_VTS_MAX - cur_mode->height;
> >> + vblank_default = cur_mode->vts_def - cur_mode->height;
> >> + ov01a10->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &ov01a10_ctrl_ops,
> >> + V4L2_CID_VBLANK, vblank_min,
> >> + vblank_max, 1, vblank_default);
> >> +
> >> + h_blank = cur_mode->hts - cur_mode->width;
> >> + ov01a10->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &ov01a10_ctrl_ops,
> >> + V4L2_CID_HBLANK, h_blank, h_blank,
> >> + 1, h_blank);
> >> + if (ov01a10->hblank)
> >> + ov01a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >> +
> >> + v4l2_ctrl_new_std(ctrl_hdlr, &ov01a10_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> >> + OV01A10_ANAL_GAIN_MIN, OV01A10_ANAL_GAIN_MAX,
> >> + OV01A10_ANAL_GAIN_STEP, OV01A10_ANAL_GAIN_MIN);
> >> + v4l2_ctrl_new_std(ctrl_hdlr, &ov01a10_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
> >> + OV01A10_DGTL_GAIN_MIN, OV01A10_DGTL_GAIN_MAX,
> >> + OV01A10_DGTL_GAIN_STEP, OV01A10_DGTL_GAIN_DEFAULT);
> >> + exposure_max = cur_mode->vts_def - OV01A10_EXPOSURE_MAX_MARGIN;
> >> + ov01a10->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &ov01a10_ctrl_ops,
> >> + V4L2_CID_EXPOSURE,
> >> + OV01A10_EXPOSURE_MIN,
> >> + exposure_max,
> >> + OV01A10_EXPOSURE_STEP,
> >> + exposure_max);
> >> + v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &ov01a10_ctrl_ops,
> >> + V4L2_CID_TEST_PATTERN,
> >> + ARRAY_SIZE(ov01a10_test_pattern_menu) - 1,
> >> + 0, 0, ov01a10_test_pattern_menu);
> >> + if (ctrl_hdlr->error)
> >> + return ctrl_hdlr->error;
> >
> > A call to v4l2_ctrl_new_fwnode_properties to add the standard
> > V4L2_CID_CAMERA_ORIENTATION and V4L2_CID_CAMERA_SENSOR_ROTATION
> > properties?
>
> Dave and Sakari,
>
> Is it mandatory for all systems? Cameras don't have such 2 properties on
> some systems.
Please do add them. The values will be just 0 in those cases, and this is
good for the user space to know as well.
--
Kind regards,
Sakari Ailus
