Hi, Bingham
Thanks for your review.
________________________
BRs,
Bingbu Cao
> -----Original Message-----
> From: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> Sent: Saturday, October 16, 2021 3:50 AM
> To: Cao, Bingbu <bingbu.cao@xxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
> sakari.ailus@xxxxxxxxxxxxxxx
> Cc: senozhatsky@xxxxxxxxxxxx; tfiga@xxxxxxxxxxxx; Cao, Bingbu
> <bingbu.cao@xxxxxxxxx>; bingbu.cao@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2] media: imx258: add vblank control to support
> more frame rate range
>
> 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?
Yes, will add later.
>
> > + { 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...
Yes, it could be done in a separate patch to add register descriptions. 😊
>
> --
> 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
> >
