Hi Hans,
Thank you for the review!
On Thu, Nov 05, 2020 at 01:46:46PM +0100, 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?
CCS_LIM(sensor, ...) translates to a value that is specific to a given
model of a camera sensor, it's not a constant.
>
> > + 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.
The spec uses lower case to refer to [cm][01].
I can use upper case if you prefer though.
>
> > + 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,
Sakari Ailus
