On 3/8/22 04:38, Arec Kao wrote:
> Trigger BLC update when analog gain change in specific range.
>
> Signed-off-by: Arec Kao <arec.kao@xxxxxxxxx>
> ---
> drivers/media/i2c/ov5675.c | 41 ++++++++++++++++++++++-
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
> include/uapi/linux/v4l2-controls.h | 1 +
> 3 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> index 82ba9f56baec..39a0a7a06249 100644
> --- a/drivers/media/i2c/ov5675.c
> +++ b/drivers/media/i2c/ov5675.c
> @@ -74,6 +74,13 @@
> #define OV5675_REG_FORMAT1 0x3820
> #define OV5675_REG_FORMAT2 0x373d
>
> +/* BLC Control */
> +#define OV5675_REG_BLC_CTRL10 0x4010
> +#define OV5675_BLC_ENABLE BIT(6) /* Gain change BLC trigger enable */
> +
> +#define OV5675_REG_BLC_CTRL11 0x4011
> +#define OV5675_BLC_MULTI_FRAME_ENABLE BIT(4) /* Gain change BLC trigger multi-frame enable */
> +
> #define to_ov5675(_sd) container_of(_sd, struct ov5675, sd)
>
> enum {
> @@ -684,6 +691,34 @@ static int ov5675_set_ctrl_vflip(struct ov5675 *ov5675, u8 ctrl_val)
> ctrl_val ? val | BIT(1) : val & ~BIT(1));
> }
>
> +static int ov5675_update_blc(struct ov5675 *ov5675, u8 ctrl_val)
> +{
> + int ret;
> + u32 val;
> +
> + ret = ov5675_read_reg(ov5675, OV5675_REG_BLC_CTRL10,
> + OV5675_REG_VALUE_08BIT, &val);
> + if (ret)
> + return ret;
> +
> + ret = ov5675_write_reg(ov5675, OV5675_REG_BLC_CTRL10,
> + OV5675_REG_VALUE_08BIT,
> + ctrl_val ? val | OV5675_BLC_ENABLE :
> + val & ~OV5675_BLC_ENABLE);
> + if (ret)
> + return ret;
> +
> + ret = ov5675_read_reg(ov5675, OV5675_REG_BLC_CTRL11,
> + OV5675_REG_VALUE_08BIT, &val);
> + if (ret)
> + return ret;
> +
> + return ov5675_write_reg(ov5675, OV5675_REG_BLC_CTRL11,
> + OV5675_REG_VALUE_08BIT,
> + ctrl_val ? val | OV5675_BLC_MULTI_FRAME_ENABLE :
> + val & ~OV5675_BLC_MULTI_FRAME_ENABLE);
> +}
> +
> static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct ov5675 *ov5675 = container_of(ctrl->handler,
> @@ -748,6 +783,9 @@ static int ov5675_set_ctrl(struct v4l2_ctrl *ctrl)
> ov5675_set_ctrl_vflip(ov5675, ctrl->val);
> break;
>
> + case V4L2_CID_BLC:
> + ret = ov5675_update_blc(ov5675, ctrl->val);
> + break;
> default:
> ret = -EINVAL;
> break;
> @@ -819,7 +857,8 @@ static int ov5675_init_controls(struct ov5675 *ov5675)
> V4L2_CID_HFLIP, 0, 1, 1, 0);
> v4l2_ctrl_new_std(ctrl_hdlr, &ov5675_ctrl_ops,
> V4L2_CID_VFLIP, 0, 1, 1, 0);
> -
> + v4l2_ctrl_new_std(ctrl_hdlr, &ov5675_ctrl_ops,
> + V4L2_CID_BLC, 0, 1, 1, 1);
It's an integer control, but used as a bool. So shouldn't it be a bool control?
Without the documentation of what it does exactly it is hard to tell.
> if (ctrl_hdlr->error)
> return ctrl_hdlr->error;
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 54ca4e6b820b..2b0b295fc047 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1110,6 +1110,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_TEST_PATTERN_BLUE: return "Blue Pixel Value";
> case V4L2_CID_TEST_PATTERN_GREENB: return "Green (Blue) Pixel Value";
> case V4L2_CID_NOTIFY_GAINS: return "Notify Gains";
> + case V4L2_CID_BLC: return "Black Level Calibration";
>
> /* Image processing controls */
> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index c8e0f84d204d..0a0fb1283124 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1126,6 +1126,7 @@ enum v4l2_jpeg_chroma_subsampling {
> #define V4L2_CID_TEST_PATTERN_GREENB (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 7)
> #define V4L2_CID_UNIT_CELL_SIZE (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 8)
> #define V4L2_CID_NOTIFY_GAINS (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 9)
> +#define V4L2_CID_BLC (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 10)
Please rename this to V4L2_CID_BLACK_LEVEL_CALIB or _CALIBRATION.
Control names should be descriptive, and I had no idea what BLC meant.
I'm leaning to writing _CALIBRATION in full, but Laurent might prefer something
shorter. _CALIB is also understandable, I think.
Regards,
Hans
>
>
> /* Image processing controls */
