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 <jac...@jmondi.org>
>
> ---
> 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@vger.kernel.org/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 >
signature.asc
Description: PGP signature
