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.
>
>> +
>> + ov01a10->sd.ctrl_handler = ctrl_hdlr;
>> +
>> + return 0;
>> +}
>> +
...
>> --
>> 2.7.4
>>
--
Best regards,
Bingbu Cao
