On 05/11/2020 13:46, Hans Verkuil wrote:
> 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?
Ah, no. The value is sensor dependent. Sorry, ignore this.
>
>> + 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.
Ignore this as well, it's because some values aren't constant.
Regards,
Hans
>
>> + 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
>
