Hi Jacopo On Thu, 6 Oct 2022 at 10:29, Jacopo Mondi <jacopo@xxxxxxxxxx> wrote: > > Hi Dave > > On Wed, Oct 05, 2022 at 04:28:03PM +0100, Dave Stevenson wrote: > > Programming the sensor with TIMING_VTS (aka LPFR) was done > > when triggered by a change in exposure or gain, but not > > when V4L2_CID_VBLANK was changed. Dynamic frame rate > > changes could therefore not be achieved. > > > > Separate out programming TIMING_VTS so that it is triggered > > by set_ctrl(V4L2_CID_VBLANK) > > > > Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> > > --- > > drivers/media/i2c/ov9282.c | 29 ++++++++++++++++------------- > > 1 file changed, 16 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c > > index 183283d191b1..5ddef6e2b3ac 100644 > > --- a/drivers/media/i2c/ov9282.c > > +++ b/drivers/media/i2c/ov9282.c > > @@ -419,22 +419,15 @@ static int ov9282_update_controls(struct ov9282 *ov9282, > > */ > > static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain) > > { > > - u32 lpfr; > > int ret; > > > > - lpfr = ov9282->vblank + ov9282->cur_mode->height; > > - > > - dev_dbg(ov9282->dev, "Set exp %u, analog gain %u, lpfr %u", > > - exposure, gain, lpfr); > > + dev_dbg(ov9282->dev, "Set exp %u, analog gain %u", > > + exposure, gain); > > > > ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1); > > if (ret) > > return ret; > > > > - ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr); > > - if (ret) > > - goto error_release_group_hold; > > - > > ret = ov9282_write_reg(ov9282, OV9282_REG_EXPOSURE, 3, exposure << 4); > > if (ret) > > goto error_release_group_hold; > > @@ -465,6 +458,7 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl) > > container_of(ctrl->handler, struct ov9282, ctrl_handler); > > u32 analog_gain; > > u32 exposure; > > + u32 lpfr; > > Only a nit about the fact lpfr is a u32 while you're writing 2 bytes. > I guess it's safe as we likely don't risk any overflow I was moving u32 lpfr from ov9282_update_exp_gain to here. All the handling of lpfr (aka TIMING_VTS) is done as u32 in this driver. The max range of V4L2_CID_VBLANK is set from the ov9282_mode definition (not strictly necessary as it should be a fixed max value handled by the register), so as long as the mode is defined correctly then there should be no overflow. Dave > > int ret; > > > > switch (ctrl->id) { > > @@ -482,10 +476,14 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl) > > OV9282_EXPOSURE_OFFSET, > > 1, OV9282_EXPOSURE_DEFAULT); > > break; > > + } > > + > > + /* Set controls only if sensor is in power on state */ > > + if (!pm_runtime_get_if_in_use(ov9282->dev)) > > + return 0; > > + > > + switch (ctrl->id) { > > case V4L2_CID_EXPOSURE: > > - /* Set controls only if sensor is in power on state */ > > - if (!pm_runtime_get_if_in_use(ov9282->dev)) > > - return 0; > > > > exposure = ctrl->val; > > analog_gain = ov9282->again_ctrl->val; > > @@ -495,14 +493,19 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl) > > > > ret = ov9282_update_exp_gain(ov9282, exposure, analog_gain); > > > > - pm_runtime_put(ov9282->dev); > > > Double empty line > With this fixed: > > Reviewed-by: Jacopo Mondi <jacopo@xxxxxxxxxx> > > Thanks > j > > + break; > > + case V4L2_CID_VBLANK: > > + lpfr = ov9282->vblank + ov9282->cur_mode->height; > > + ret = ov9282_write_reg(ov9282, OV9282_REG_LPFR, 2, lpfr); > > break; > > default: > > dev_err(ov9282->dev, "Invalid control %d", ctrl->id); > > ret = -EINVAL; > > } > > > > + pm_runtime_put(ov9282->dev); > > + > > return ret; > > } > > > > -- > > 2.34.1 > >