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
> 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
>
