Message ID | 20201007084557.25843-91-sakari.ailus@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | CCS driver | expand |
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@linux.intel.com> > --- > 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
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@linux.intel.com> >> --- >> 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 >
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@linux.intel.com> > > --- > > 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 || > > >
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 { + const char *name; + u32 id; + s32 value; + } const gain_ctrls[] = { + { "Analogue Gain m0", V4L2_CID_CCS_ANALOGUE_GAIN_M0, + 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, + }; + 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 ||
Add four controls for reading CCS analogue gain coefficients. The values are constants that are device specific. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/i2c/ccs/ccs-core.c | 38 ++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-)