Hi Jacopo, In serie "[PATCH 0/5] Fix OV5640 exposure & gain" https://www.mail-archive.com/linux-media@xxxxxxxxxxxxxxx/msg133269.html I've tried to collect fixes around exposure/gain, not only the exposure regression and I would prefer to keep it consistent with the associated procedure test. Moreover I dislike the internal use of control framework functions to disable/enable exposure/gain, on my opinion this has to be kept simpler by just disabling/enabling the right registers. Would it be possible that you test my 5 patches serie on your side ? Best regards, Hugues. On 07/18/2018 03:04 PM, jacopo mondi wrote: > 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 >>
