Quoting Hans de Goede (2024-02-16 22:32:35)
> Add vblank control to allow changing the framerate /
> higher exposure values.
>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> drivers/media/i2c/ov2680.c | 47 ++++++++++++++++++++++++++++++++------
> 1 file changed, 40 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> index b4d5936dcd02..4c9db83d876e 100644
> --- a/drivers/media/i2c/ov2680.c
> +++ b/drivers/media/i2c/ov2680.c
> @@ -75,6 +75,8 @@
> #define OV2680_ACTIVE_START_TOP 8
> #define OV2680_MIN_CROP_WIDTH 2
> #define OV2680_MIN_CROP_HEIGHT 2
> +#define OV2680_MIN_VBLANK 4
> +#define OV2680_MAX_VBLANK 0xffff
>
> /* Fixed pre-div of 1/2 */
> #define OV2680_PLL_PREDIV0 2
> @@ -84,7 +86,7 @@
>
> /* 66MHz pixel clock: 66MHz / 1704 * 1294 = 30fps */
> #define OV2680_PIXELS_PER_LINE 1704
> -#define OV2680_LINES_PER_FRAME 1294
> +#define OV2680_LINES_PER_FRAME_30FPS 1294
I can definitely foresee some core sensor helpers to handle all the
calculations for things like this as parameters rather than static data
to support when changes and features are extended in drivers like
supporting different link frequencies - but for now ACK here.
> /* Max exposure time is VTS - 8 */
> #define OV2680_INTEGRATION_TIME_MARGIN 8
> @@ -127,6 +129,7 @@ struct ov2680_ctrls {
> struct v4l2_ctrl *test_pattern;
> struct v4l2_ctrl *link_freq;
> struct v4l2_ctrl *pixel_rate;
> + struct v4l2_ctrl *vblank;
> };
>
> struct ov2680_mode {
> @@ -394,8 +397,7 @@ static int ov2680_set_mode(struct ov2680_dev *sensor)
> sensor->mode.v_output_size, &ret);
> cci_write(sensor->regmap, OV2680_REG_TIMING_HTS,
> OV2680_PIXELS_PER_LINE, &ret);
> - cci_write(sensor->regmap, OV2680_REG_TIMING_VTS,
> - OV2680_LINES_PER_FRAME, &ret);
> + /* VTS gets set by the vblank ctrl */
> cci_write(sensor->regmap, OV2680_REG_ISP_X_WIN, 0, &ret);
> cci_write(sensor->regmap, OV2680_REG_ISP_Y_WIN, 0, &ret);
> cci_write(sensor->regmap, OV2680_REG_X_INC, inc, &ret);
> @@ -469,6 +471,15 @@ static int ov2680_exposure_set(struct ov2680_dev *sensor, u32 exp)
> NULL);
> }
>
> +static void ov2680_exposure_update_range(struct ov2680_dev *sensor)
> +{
Reading here for the first time, I almost would have needed the comment
/* Max exposure time is VTS - 8 */
to be here. Fortunately, it was in the context of this patch above so I
could see it.
> + int exp_max = sensor->mode.fmt.height + sensor->ctrls.vblank->val -
> + OV2680_INTEGRATION_TIME_MARGIN;
> +
> + __v4l2_ctrl_modify_range(sensor->ctrls.exposure, 0, exp_max,
> + 1, exp_max);
> +}
> +
> static int ov2680_stream_enable(struct ov2680_dev *sensor)
> {
> int ret;
> @@ -635,7 +646,7 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
> struct v4l2_mbus_framefmt *try_fmt;
> const struct v4l2_rect *crop;
> unsigned int width, height;
> - int ret = 0;
> + int def, max, ret = 0;
>
> crop = __ov2680_get_pad_crop(sensor, sd_state, format->pad,
> format->which);
> @@ -664,6 +675,15 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
> sensor->mode.fmt = format->format;
> ov2680_calc_mode(sensor);
>
> + /* vblank range is height dependent adjust and reset to default */
> + max = OV2680_MAX_VBLANK - height;
> + def = OV2680_LINES_PER_FRAME_30FPS - height;
> + __v4l2_ctrl_modify_range(sensor->ctrls.vblank, OV2680_MIN_VBLANK, max,
> + 1, def);
> + __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, def);
> + /* exposure range depends on vts which may have changed */
> + ov2680_exposure_update_range(sensor);
> +
> unlock:
> mutex_unlock(&sensor->lock);
>
> @@ -833,6 +853,10 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
> struct ov2680_dev *sensor = to_ov2680_dev(sd);
> int ret;
>
> + /* Update exposure range on vblank changes */
> + if (ctrl->id == V4L2_CID_VBLANK)
> + ov2680_exposure_update_range(sensor);
> +
> /* Only apply changes to the controls if the device is powered up */
> if (!pm_runtime_get_if_in_use(sensor->sd.dev)) {
> ov2680_set_bayer_order(sensor, &sensor->mode.fmt);
> @@ -855,6 +879,10 @@ static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
> case V4L2_CID_TEST_PATTERN:
> ret = ov2680_test_pattern_set(sensor, ctrl->val);
> break;
> + case V4L2_CID_VBLANK:
> + ret = cci_write(sensor->regmap, OV2680_REG_TIMING_VTS,
> + sensor->mode.fmt.height + ctrl->val, NULL);
> + break;
> default:
> ret = -EINVAL;
> break;
> @@ -913,8 +941,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
> const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops;
> struct ov2680_ctrls *ctrls = &sensor->ctrls;
> struct v4l2_ctrl_handler *hdl = &ctrls->handler;
> - int exp_max = OV2680_LINES_PER_FRAME - OV2680_INTEGRATION_TIME_MARGIN;
> - int ret = 0;
> + int def, max, ret = 0;
>
> v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_subdev_ops);
> sensor->sd.internal_ops = &ov2680_internal_ops;
> @@ -939,8 +966,9 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
> ARRAY_SIZE(test_pattern_menu) - 1,
> 0, 0, test_pattern_menu);
>
> + max = OV2680_LINES_PER_FRAME_30FPS - OV2680_INTEGRATION_TIME_MARGIN;
> ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> - 0, exp_max, 1, exp_max);
> + 0, max, 1, max);
>
> ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> 0, 1023, 1, 250);
> @@ -951,6 +979,11 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
> 0, sensor->pixel_rate,
> 1, sensor->pixel_rate);
>
> + max = OV2680_MAX_VBLANK - OV2680_DEFAULT_HEIGHT;
> + def = OV2680_LINES_PER_FRAME_30FPS - OV2680_DEFAULT_HEIGHT;
> + ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
> + OV2680_MIN_VBLANK, max, 1, def);
> +
Reviewed-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> if (hdl->error) {
> ret = hdl->error;
> goto cleanup_entity;
> --
> 2.43.0
>
