Hi Bingbu,
Quoting Bingbu Cao (2021-10-15 08:05:30)
> Current imx258 driver enable the automatic frame length tracking control
> by default and did not support VBLANK change, it's always working at 30fps.
> However, in reality we need a wider frame rate range from 15 to 30.
> This patch disable the automatic frame length tracking control and enable
> the v4l2 VBLANK control to allow user changing frame rate per requirement.
>
> Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
> ---
> drivers/media/i2c/imx258.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
> ---
> v1->v2: remove a wrong 'break'
> ---
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 81cdf37216ca..3f46744b1a26 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -29,6 +29,7 @@
> #define IMX258_VTS_MAX 0xffff
>
> /*Frame Length Line*/
> +#define IMX258_REG_FLL 0x0340
> #define IMX258_FLL_MIN 0x08a6
> #define IMX258_FLL_MAX 0xffff
> #define IMX258_FLL_STEP 1
> @@ -241,7 +242,7 @@ static const struct imx258_reg mode_4208x3118_regs[] = {
> { 0x034D, 0x70 },
> { 0x034E, 0x0C },
> { 0x034F, 0x30 },
> - { 0x0350, 0x01 },
> + { 0x0350, 0x00 },
The commit message implies that the register 0x0350 controls the
"automatic frame length tracking".
Is it worth adding that register description as a comment at the end of
the line, to help future readers?
> + { 0x0350, 0x00 }, /* automatic frame length tracking */
Without datasheets, these long register lists are very terse ...
If register names /functions can at least be identified then I suspect
it would help with future maintenance of the code?
Or is it too futile to imagine that these registers might improve in
documentation as time goes on...
--
Kieran
> { 0x0202, 0x0C },
> { 0x0203, 0x46 },
> { 0x0204, 0x00 },
> @@ -360,7 +361,7 @@ static const struct imx258_reg mode_2104_1560_regs[] = {
> { 0x034D, 0x38 },
> { 0x034E, 0x06 },
> { 0x034F, 0x18 },
> - { 0x0350, 0x01 },
> + { 0x0350, 0x00 },
> { 0x0202, 0x06 },
> { 0x0203, 0x2E },
> { 0x0204, 0x00 },
> @@ -479,7 +480,7 @@ static const struct imx258_reg mode_1048_780_regs[] = {
> { 0x034D, 0x18 },
> { 0x034E, 0x03 },
> { 0x034F, 0x0C },
> - { 0x0350, 0x01 },
> + { 0x0350, 0x00 },
> { 0x0202, 0x03 },
> { 0x0203, 0x42 },
> { 0x0204, 0x00 },
> @@ -753,8 +754,17 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> struct imx258 *imx258 =
> container_of(ctrl->handler, struct imx258, ctrl_handler);
> struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> + s64 max;
> int ret = 0;
>
> + if (ctrl->id == V4L2_CID_VBLANK) {
> + /* Update max exposure to meet expected vblanking */
> + max = imx258->cur_mode->height + ctrl->val - 10;
> + __v4l2_ctrl_modify_range(imx258->exposure,
> + imx258->exposure->minimum,
> + max, imx258->exposure->step, max);
> + }
> +
> /*
> * Applying V4L2 control value only happens
> * when power is up for streaming
> @@ -773,6 +783,10 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> IMX258_REG_VALUE_16BIT,
> ctrl->val);
> break;
> + case V4L2_CID_VBLANK:
> + ret = imx258_write_reg(imx258, IMX258_REG_FLL, 2,
> + imx258->cur_mode->height + ctrl->val);
> + break;
> case V4L2_CID_DIGITAL_GAIN:
> ret = imx258_update_digital_gain(imx258, IMX258_REG_VALUE_16BIT,
> ctrl->val);
> @@ -1189,9 +1203,6 @@ static int imx258_init_controls(struct imx258 *imx258)
> IMX258_VTS_MAX - imx258->cur_mode->height, 1,
> vblank_def);
>
> - if (imx258->vblank)
> - imx258->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> -
> imx258->hblank = v4l2_ctrl_new_std(
> ctrl_hdlr, &imx258_ctrl_ops, V4L2_CID_HBLANK,
> IMX258_PPL_DEFAULT - imx258->cur_mode->width,
> --
> 2.7.4
>
