Hi Bingbu,
On Fri, Aug 21, 2020 at 10:00 AM Bingbu Cao <bingbu.cao@xxxxxxxxx> wrote:
>
> The unit of exposure value is different from other OmniVision sensors,
> driver will divide by 2 before set register, the exposure range exposed
> by v4l2 ctrl to user should be same as others, so the calculation for
> the maximum exposure value in current driver need be fixed.
>
> Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
> ---
> drivers/media/i2c/ov5675.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
Thanks for the patch! Please see my comments inline.
> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> index 8537cc4ca108..9540ce8918f0 100644
> --- a/drivers/media/i2c/ov5675.c
> +++ b/drivers/media/i2c/ov5675.c
> @@ -666,8 +666,8 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl)
> /* Propagate change of current control to all related controls */
> if (ctrl->id == V4L2_CID_VBLANK) {
> /* Update max exposure while meeting expected vblanking */
> - exposure_max = (ov5675->cur_mode->height + ctrl->val -
> - OV5675_EXPOSURE_MAX_MARGIN) / 2;
> + exposure_max = ov5675->cur_mode->height + ctrl->val -
> + OV5675_EXPOSURE_MAX_MARGIN;
> __v4l2_ctrl_modify_range(ov5675->exposure,
> ov5675->exposure->minimum,
> exposure_max, ov5675->exposure->step,
> @@ -689,7 +689,13 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl)
> break;
>
> case V4L2_CID_EXPOSURE:
> - /* 3 least significant bits of expsoure are fractional part */
> + /* 4 least significant bits of expsoure are fractional part
exposure
> + * val = val << 4
> + * for ov5675, the unit of exposure is differnt from other
different
> + * OmniVision sensors, its exposure value is twice of the
> + * register value, the exposure should be divided by 2 before
> + * set register, e.g. val << 3.
> + */
> ret = ov5675_write_reg(ov5675, OV5675_REG_EXPOSURE,
> OV5675_REG_VALUE_24BIT, ctrl->val << 3);
How about turning this code into (ctrl->val << 4) / 2 ? It will be
compiled into exactly the same code, but will be obvious that we are
handling two different factors in the computation.
Another question: Since the V4L2_CID_EXPOSURE control is not specified
to be in a particular unit and hardware supports fractional exposures,
why not expose the exposure exactly as it is in the register?
Best regards,
Tomasz
> break;
> @@ -770,8 +776,7 @@ static int ov5675_init_controls(struct ov5675 *ov5675)
> v4l2_ctrl_new_std(ctrl_hdlr, &ov5675_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
> OV5675_DGTL_GAIN_MIN, OV5675_DGTL_GAIN_MAX,
> OV5675_DGTL_GAIN_STEP, OV5675_DGTL_GAIN_DEFAULT);
> - exposure_max = (ov5675->cur_mode->vts_def -
> - OV5675_EXPOSURE_MAX_MARGIN) / 2;
> + exposure_max = (ov5675->cur_mode->vts_def - OV5675_EXPOSURE_MAX_MARGIN);
> ov5675->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &ov5675_ctrl_ops,
> V4L2_CID_EXPOSURE,
> OV5675_EXPOSURE_MIN, exposure_max,
> --
> 2.7.4
>
