Sakari,
Thanks for your review.
On 2/9/23 9:06 PM, Sakari Ailus wrote:
> Hi Bingbu,
>
> Thanks for the patch.
>
> On Thu, Feb 09, 2023 at 07:22:38PM +0800, Bingbu Cao 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
>>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index 833241897d63..8e5d1ef0616f 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -338,6 +338,19 @@ config VIDEO_OG01A1B
>> To compile this driver as a module, choose M here: the
>> module will be called og01a1b.
>>
>> +config VIDEO_OV01A10
>> + tristate "OmniVision OV01A10 sensor support"
>> + depends on VIDEO_DEV && I2C
>> + select MEDIA_CONTROLLER
>> + select VIDEO_V4L2_SUBDEV_API
>> + select V4L2_FWNODE
>> + help
>> + This is a Video4Linux2 sensor driver for the OmniVision
>> + OV01A10 camera.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called ov01a10.
>> +
>> config VIDEO_OV02A10
>> tristate "OmniVision OV02A10 sensor support"
>> depends on VIDEO_DEV && I2C
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index 4d6c052bb5a7..ce214503b526 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -70,6 +70,7 @@ obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o
>> obj-$(CONFIG_VIDEO_MT9V111) += mt9v111.o
>> obj-$(CONFIG_VIDEO_NOON010PC30) += noon010pc30.o
>> obj-$(CONFIG_VIDEO_OG01A1B) += og01a1b.o
>> +obj-$(CONFIG_VIDEO_OV01A10) += ov01a10.o
>> obj-$(CONFIG_VIDEO_OV02A10) += ov02a10.o
>> obj-$(CONFIG_VIDEO_OV08D10) += ov08d10.o
>> obj-$(CONFIG_VIDEO_OV08X40) += ov08x40.o
>> diff --git a/drivers/media/i2c/ov01a10.c b/drivers/media/i2c/ov01a10.c
>> new file mode 100644
>> index 000000000000..13de9f9ccdc8
>> --- /dev/null
>> +++ b/drivers/media/i2c/ov01a10.c
>> @@ -0,0 +1,906 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2020-2021 Intel Corporation.
..snip..
>> +static void ov01a10_update_pad_format(const struct ov01a10_mode *mode,
>> + struct v4l2_mbus_framefmt *fmt)
>> +{
>> + fmt->width = mode->width;
>> + fmt->height = mode->height;
>> + fmt->code = MEDIA_BUS_FMT_SBGGR10_1X10;
>> + fmt->field = V4L2_FIELD_NONE;
>> +}
>> +
..snip..
>> +static int ov01a10_set_format(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state,
>> + struct v4l2_subdev_format *fmt)
>> +{
>> + struct ov01a10 *ov01a10 = to_ov01a10(sd);
>> + const struct ov01a10_mode *mode;
>> + s32 vblank_def, h_blank;
>> +
>> + mode = v4l2_find_nearest_size(supported_modes,
>> + ARRAY_SIZE(supported_modes), width,
>> + height, fmt->format.width,
>> + fmt->format.height);
>> +
>> + mutex_lock(&ov01a10->mutex);
>> + ov01a10_update_pad_format(mode, &fmt->format);
>
> Could you switch to the sub-device state? That is now the preferred way to
> serialise access to e.g. the format.
>
> See e.g.
> <URL:https://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git/tree/drivers/media/i2c/ov1063x.c?h=streams/work-v16>.
>
> The control handler's mutex doubles as a sub-device state mutex.
Is it fine to use v4l2_subdev_get_fmt()? Or will it be deprecated soon?
>
>> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
>> + *v4l2_subdev_get_try_format(sd, sd_state, fmt->pad) = fmt->format;
>> + } else {
>> + ov01a10->cur_mode = mode;
>> + __v4l2_ctrl_s_ctrl(ov01a10->link_freq, mode->link_freq_index);
>> + __v4l2_ctrl_s_ctrl_int64(ov01a10->pixel_rate, OV01A10_SCLK);
>> +
>> + vblank_def = mode->vts_def - mode->height;
>> + __v4l2_ctrl_modify_range(ov01a10->vblank,
>> + mode->vts_min - mode->height,
>> + OV01A10_VTS_MAX - mode->height, 1,
>> + vblank_def);
>> + __v4l2_ctrl_s_ctrl(ov01a10->vblank, vblank_def);
>> + h_blank = mode->hts - mode->width;
>> + __v4l2_ctrl_modify_range(ov01a10->hblank, h_blank, h_blank, 1,
>> + h_blank);
>> + }
>> + mutex_unlock(&ov01a10->mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static int ov01a10_get_format(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state,
>> + struct v4l2_subdev_format *fmt)
>> +{
>> + struct ov01a10 *ov01a10 = to_ov01a10(sd);
>> +
>> + mutex_lock(&ov01a10->mutex);
>> + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
>> + fmt->format = *v4l2_subdev_get_try_format(&ov01a10->sd,
>> + sd_state, fmt->pad);
>> + else
>> + ov01a10_update_pad_format(ov01a10->cur_mode, &fmt->format);
>> +
>> + mutex_unlock(&ov01a10->mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static int ov01a10_enum_mbus_code(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state,
>> + struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> + if (code->index > 0)
>> + return -EINVAL;
>> +
>> + code->code = MEDIA_BUS_FMT_SBGGR10_1X10;
>> +
>> + return 0;
>> +}
>> +
>> +static int ov01a10_enum_frame_size(struct v4l2_subdev *sd,
>> + struct v4l2_subdev_state *sd_state,
>> + struct v4l2_subdev_frame_size_enum *fse)
>> +{
>> + if (fse->index >= ARRAY_SIZE(supported_modes))
>> + return -EINVAL;
>> +
>> + if (fse->code != MEDIA_BUS_FMT_SBGGR10_1X10)
>> + return -EINVAL;
>> +
>> + fse->min_width = supported_modes[fse->index].width;
>> + fse->max_width = fse->min_width;
>> + fse->min_height = supported_modes[fse->index].height;
>> + fse->max_height = fse->min_height;
>> +
>> + return 0;
>> +}
>> +
>> +static int ov01a10_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>> +{
>> + struct ov01a10 *ov01a10 = to_ov01a10(sd);
>> +
>> + mutex_lock(&ov01a10->mutex);
>> + ov01a10_update_pad_format(&supported_modes[0],
>> + v4l2_subdev_get_try_format(sd, fh->state, 0));
>> + mutex_unlock(&ov01a10->mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct v4l2_subdev_core_ops ov01a10_core_ops = {
>> + .log_status = v4l2_ctrl_subdev_log_status,
>> + .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
>> + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
>> +};
>> +
>> +static const struct v4l2_subdev_video_ops ov01a10_video_ops = {
>> + .s_stream = ov01a10_set_stream,
>> +};
>> +
>> +static const struct v4l2_subdev_pad_ops ov01a10_pad_ops = {
>> + .set_fmt = ov01a10_set_format,
>> + .get_fmt = ov01a10_get_format,
>> + .enum_mbus_code = ov01a10_enum_mbus_code,
>> + .enum_frame_size = ov01a10_enum_frame_size,
>> +};
>> +
...
>
--
Best regards,
Bingbu Cao
