On 07/10/2020 10:45, Sakari Ailus wrote:
> Add four controls for reading CCS analogue gain coefficients. The
> values are constants that are device specific.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> ---
> drivers/media/i2c/ccs/ccs-core.c | 38 ++++++++++++++++++++++++++++++--
> 1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
> index d6d6aeceb415..2a507b63bafc 100644
> --- a/drivers/media/i2c/ccs/ccs-core.c
> +++ b/drivers/media/i2c/ccs/ccs-core.c
> @@ -28,6 +28,7 @@
> #include <linux/v4l2-mediabus.h>
> #include <media/v4l2-fwnode.h>
> #include <media/v4l2-device.h>
> +#include <uapi/linux/ccs.h>
>
> #include "ccs.h"
>
> @@ -772,14 +773,46 @@ static int ccs_init_controls(struct ccs_sensor *sensor)
> struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> int rval;
>
> - rval = v4l2_ctrl_handler_init(&sensor->pixel_array->ctrl_handler, 13);
> + rval = v4l2_ctrl_handler_init(&sensor->pixel_array->ctrl_handler, 17);
> if (rval)
> return rval;
>
> sensor->pixel_array->ctrl_handler.lock = &sensor->mutex;
>
> switch (CCS_LIM(sensor, ANALOG_GAIN_CAPABILITY)) {
> - case CCS_ANALOG_GAIN_CAPABILITY_GLOBAL:
> + case CCS_ANALOG_GAIN_CAPABILITY_GLOBAL: {
> + struct {
Can this be static?
> + const char *name;
> + u32 id;
> + s32 value;
> + } const gain_ctrls[] = {
> + { "Analogue Gain m0", V4L2_CID_CCS_ANALOGUE_GAIN_M0,
Why lower case 'm0/1' and 'c0/1'? It looks odd.
> + CCS_LIM(sensor, ANALOG_GAIN_M0), },
> + { "Analogue Gain c0", V4L2_CID_CCS_ANALOGUE_GAIN_C0,
> + CCS_LIM(sensor, ANALOG_GAIN_C0), },
> + { "Analogue Gain m1", V4L2_CID_CCS_ANALOGUE_GAIN_M1,
> + CCS_LIM(sensor, ANALOG_GAIN_M1), },
> + { "Analogue Gain c1", V4L2_CID_CCS_ANALOGUE_GAIN_C1,
> + CCS_LIM(sensor, ANALOG_GAIN_C1), },
> + };
> + struct v4l2_ctrl_config ctrl_cfg = {
> + .type = V4L2_CTRL_TYPE_INTEGER,
> + .ops = &ccs_ctrl_ops,
> + .flags = V4L2_CTRL_FLAG_READ_ONLY,
> + .step = 1,
> + };
This looks a bit weird. Most drivers just define each control separately in
a static const struct v4l2_ctrl_config, then add them with v4l2_ctrl_new_custom.
Now the control definition data is split up in two data structures.
It's not wrong, it is just non-standard, and a bit harder to review.
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(gain_ctrls); i++) {
> + ctrl_cfg.name = gain_ctrls[i].name;
> + ctrl_cfg.id = gain_ctrls[i].id;
> + ctrl_cfg.min = ctrl_cfg.max = ctrl_cfg.def =
> + gain_ctrls[i].value;
> +
> + v4l2_ctrl_new_custom(&sensor->pixel_array->ctrl_handler,
> + &ctrl_cfg, NULL);
> + }
> +
> v4l2_ctrl_new_std(&sensor->pixel_array->ctrl_handler,
> &ccs_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> CCS_LIM(sensor, ANALOG_GAIN_CODE_MIN),
> @@ -788,6 +821,7 @@ static int ccs_init_controls(struct ccs_sensor *sensor)
> 1U),
> CCS_LIM(sensor, ANALOG_GAIN_CODE_MIN));
> }
> + }
>
> if (CCS_LIM(sensor, DIGITAL_GAIN_CAPABILITY) ==
> CCS_DIGITAL_GAIN_CAPABILITY_GLOBAL ||
>
Regards,
Hans
