Hi again,
On Wed, Jul 18, 2018 at 01:19:03PM +0200, Jacopo Mondi wrote:
> As of:
> commit bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at
> start time") auto-exposure got disabled before programming new capture modes to
> the sensor. Unfortunately the function used to do that (ov5640_set_exposure())
> does not enable/disable auto-exposure engine through register 0x3503[0] bit, but
> programs registers [0x3500 - 0x3502] which represent the desired exposure time
> when running with manual exposure. As a result, auto-exposure was not actually
> disabled at all.
>
> To actually disable auto-exposure, go through the control framework instead of
> calling ov5640_set_exposure() function directly.
>
> Also, as auto-gain and auto-exposure are disabled un-conditionally but only
> restored to their previous values in ov5640_set_mode_direct() function, move
> controls restoring so that their value is re-programmed opportunely after
> either ov5640_set_mode_direct() or ov5640_set_mode_exposure_calc() have been
> executed.
>
> Fixes: bf4a4b518c20 ("media: ov5640: Don't force the auto exposure state at start time")
> Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
>
> ---
> Is it worth doing with auto-gain what we're doing with auto-exposure? Cache the
> value and then re-program it instead of unconditionally disable/enable it?
I have missed this patch from Hugues that address almost the same
issue
https://www.mail-archive.com/linux-media@xxxxxxxxxxxxxxx/msg133264.html
I feel this new one is simpler, and unless we want to avoid going
through the control framework, it is not worth adding new functions to
handle auto-exposure as Hugues' patch is doing.
Hugues, do you have comments? Feel free to add your sob or rb tags if
you like to.
Thanks
j
>
> Thanks
> j
> ---
> ---
> drivers/media/i2c/ov5640.c | 29 +++++++++++++----------------
> 1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 12b3496..bc75cb7 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -1588,25 +1588,13 @@ static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor,
> * change mode directly
> */
> static int ov5640_set_mode_direct(struct ov5640_dev *sensor,
> - const struct ov5640_mode_info *mode,
> - s32 exposure)
> + const struct ov5640_mode_info *mode)
> {
> - int ret;
> -
> if (!mode->reg_data)
> return -EINVAL;
>
> /* Write capture setting */
> - ret = ov5640_load_regs(sensor, mode);
> - if (ret < 0)
> - return ret;
> -
> - /* turn auto gain/exposure back on for direct mode */
> - ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
> - if (ret)
> - return ret;
> -
> - return __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, exposure);
> + return ov5640_load_regs(sensor, mode);
> }
>
> static int ov5640_set_mode(struct ov5640_dev *sensor,
> @@ -1626,7 +1614,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
> return ret;
>
> exposure = sensor->ctrls.auto_exp->val;
> - ret = ov5640_set_exposure(sensor, V4L2_EXPOSURE_MANUAL);
> + ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, V4L2_EXPOSURE_MANUAL);
> if (ret)
> return ret;
>
> @@ -1642,12 +1630,21 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
> * change inside subsampling or scaling
> * download firmware directly
> */
> - ret = ov5640_set_mode_direct(sensor, mode, exposure);
> + ret = ov5640_set_mode_direct(sensor, mode);
> }
>
> if (ret < 0)
> return ret;
>
> + /* Restore auto-gain and auto-exposure after mode has changed. */
> + ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
> + if (ret)
> + return ret;
> +
> + ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_exp, exposure)
> + if (ret)
> + return ret;
> +
> ret = ov5640_set_binning(sensor, dn_mode != SCALING);
> if (ret < 0)
> return ret;
> --
> 2.7.4
>
Attachment:
signature.asc
Description: PGP signature
