Hi Jacopo
Thanks for the review.
On Wed, 31 May 2023 at 16:16, Jacopo Mondi
<jacopo.mondi@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Dave
>
> On Tue, May 30, 2023 at 06:29:45PM +0100, Dave Stevenson wrote:
> > The values and ranges of V4L2_CID_VBLANK are all computed,
> > so there is no reason for it to be a read only control.
> > Remove the register values from the mode lists, add the
> > handler, and remove the read only flag.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/imx258.c | 16 +++++++---------
> > 1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index 30bae7388c3a..c6fb649abb95 100644
> > --- a/drivers/media/i2c/imx258.c
> > +++ b/drivers/media/i2c/imx258.c
> > @@ -30,6 +30,8 @@
> > #define IMX258_VTS_30FPS_VGA 0x034c
> > #define IMX258_VTS_MAX 0xffff
> >
> > +#define IMX258_REG_VTS 0x0340
> > +
> > /* HBLANK control - read only */
> > #define IMX258_PPL_DEFAULT 5352
> >
> > @@ -202,8 +204,6 @@ static const struct imx258_reg mode_4208x3120_regs[] = {
> > { 0x0114, 0x03 },
> > { 0x0342, 0x14 },
> > { 0x0343, 0xE8 },
> > - { 0x0340, 0x0C },
> > - { 0x0341, 0x50 },
> > { 0x0344, 0x00 },
> > { 0x0345, 0x00 },
> > { 0x0346, 0x00 },
> > @@ -319,8 +319,6 @@ static const struct imx258_reg mode_2104_1560_regs[] = {
> > { 0x0114, 0x03 },
> > { 0x0342, 0x14 },
> > { 0x0343, 0xE8 },
> > - { 0x0340, 0x06 },
> > - { 0x0341, 0x38 },
> > { 0x0344, 0x00 },
> > { 0x0345, 0x00 },
> > { 0x0346, 0x00 },
> > @@ -436,8 +434,6 @@ static const struct imx258_reg mode_1048_780_regs[] = {
> > { 0x0114, 0x03 },
> > { 0x0342, 0x14 },
> > { 0x0343, 0xE8 },
> > - { 0x0340, 0x03 },
> > - { 0x0341, 0x4C },
> > { 0x0344, 0x00 },
> > { 0x0345, 0x00 },
> > { 0x0346, 0x00 },
> > @@ -803,6 +799,11 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
> > BIT(IMX258_HDR_RATIO_MAX));
> > }
> > break;
> > + case V4L2_CID_VBLANK:
>
> Should a new vblank value change the exposure limits too ?
Yes, however until "media: i2c: imx258: Follow normal V4L2 behaviours
for clipping exposure" (patch 10) the driver tells the sensor to
automatically extend blanking to allow for the requested exposure,
totally without userspace knowing. That patch adds in the
recomputation of exposure based on VBLANK changing.
I can swap the patch order if you feel it is necessary, but seeing as
exposure isn't limited in reality at this point I'm not too fussed.
Dave
> > + ret = imx258_write_reg(imx258, IMX258_REG_VTS,
> > + IMX258_REG_VALUE_16BIT,
> > + imx258->cur_mode->height + ctrl->val);
> > + break;
> > default:
> > dev_info(&client->dev,
> > "ctrl(id:0x%x,val:0x%x) is not handled\n",
> > @@ -1214,9 +1215,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.25.1
> >
