On 07/10/2020 10:45, Sakari Ailus wrote:
> Add controls for supporting lens shading correction.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> ---
> drivers/media/i2c/ccs/ccs-core.c | 74 ++++++++++++++++++++++++++++++++
> 1 file changed, 74 insertions(+)
>
> diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> index 0ba06a580951..10ed3d01af16 100644
> --- a/drivers/media/i2c/ccs/ccs-core.c
> +++ b/drivers/media/i2c/ccs/ccs-core.c
> @@ -757,6 +757,25 @@ static int ccs_set_ctrl(struct v4l2_ctrl *ctrl)
> case V4L2_CID_TEST_PATTERN_GREENB:
> rval = ccs_write(sensor, TEST_DATA_GREENB, ctrl->val);
>
> + break;
> + case V4L2_CID_CCS_SHADING_CORRECTION:
> + if (!(CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) &
> + (CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING |
> + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION)))
> + break;
> +
> + rval = ccs_write(sensor, SHADING_CORRECTION_EN,
> + ctrl->val ? CCS_SHADING_CORRECTION_EN_ENABLE :
> + 0);
> +
> + break;
> + case V4L2_CID_CCS_LUMINANCE_SHADING_CORRECTION:
> + if (!(CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) &
> + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION))
> + break;
> +
> + rval = ccs_write(sensor, LUMINANCE_CORRECTION_LEVEL, ctrl->val);
> +
> break;
> case V4L2_CID_PIXEL_RATE:
> /* For v4l2_ctrl_s_ctrl_int64() used internally. */
> @@ -878,6 +897,61 @@ static int ccs_init_controls(struct ccs_sensor *sensor)
> }
> }
>
> + if (CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) &
> + CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING) {
> + const struct v4l2_ctrl_config ctrl_cfg = {
Can be static.
> + .name = "Shading Correction",
Should this perhaps be called "Chroma Shading Correction"? Since there is a luminance
shading correction as well...
Since these controls are all CCS specific, should the names of these controls begin
with "CCS "? (Not just the controls in this patch, but also from previous patches).
> + .type = V4L2_CTRL_TYPE_BOOLEAN,
> + .id = V4L2_CID_CCS_SHADING_CORRECTION,
> + .ops = &ccs_ctrl_ops,
> + .max = 1,
> + .step = 1,
> + };
> +
> + v4l2_ctrl_new_custom(&sensor->pixel_array->ctrl_handler,
> + &ctrl_cfg, NULL);
> + }
> +
> + if (CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) &
> + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION) {
> + const struct v4l2_ctrl_config ctrl_cfg = {
Can be static.
> + .name = "Luminance Shading Correction",
> + .type = V4L2_CTRL_TYPE_BOOLEAN,
> + .id = V4L2_CID_CCS_LUMINANCE_SHADING_CORRECTION,
> + .ops = &ccs_ctrl_ops,
> + .max = 255,
> + .step = 1,
> + .def = 128,
> + };
> +
> + v4l2_ctrl_new_custom(&sensor->pixel_array->ctrl_handler,
> + &ctrl_cfg, NULL);
> + }
> +
> + if (CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) &
> + (CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING |
> + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION)) {
> + u32 val =
> + ((CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) &
> + CCS_SHADING_CORRECTION_CAPABILITY_COLOR_SHADING) ?
> + V4L2_CCS_SHADING_CORRECTION_COLOUR : 0) |
> + ((CCS_LIM(sensor, SHADING_CORRECTION_CAPABILITY) &
> + CCS_SHADING_CORRECTION_CAPABILITY_LUMINANCE_CORRECTION) ?
> + V4L2_CCS_SHADING_CORRECTION_LUMINANCE : 0);
> + const struct v4l2_ctrl_config ctrl_cfg = {
> + .name = "Shading Correction Capability",
> + .type = V4L2_CTRL_TYPE_BITMASK,
> + .id = V4L2_CID_CCS_SHADING_CORRECTION_CAPABILITY,
> + .ops = &ccs_ctrl_ops,
> + .max = val,
> + .def = val,
> + .flags = V4L2_CTRL_FLAG_READ_ONLY,
> + };
Is this needed? If e.g. V4L2_CCS_SHADING_CORRECTION_COLOUR is not supported,
then the V4L2_CID_CCS_SHADING_CORRECTION control is simply not created.
So calling VIDIOC_QUERYCTRL would simply fail and so indicate that this
capability is not present.
If it really is needed, then having two bool controls makes more sense
because a bitmask is less intuitive.
Regards,
Hans
> +
> + v4l2_ctrl_new_custom(&sensor->pixel_array->ctrl_handler,
> + &ctrl_cfg, NULL);
> + }
> +
> if (CCS_LIM(sensor, DIGITAL_GAIN_CAPABILITY) ==
> CCS_DIGITAL_GAIN_CAPABILITY_GLOBAL ||
> CCS_LIM(sensor, DIGITAL_GAIN_CAPABILITY) ==
>
