diff mbox series

[v2,100/106] ccs: Add support for analogue gain coefficient controls

Message ID 20201007084557.25843-91-sakari.ailus@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series CCS driver | expand

Commit Message

Sakari Ailus Oct. 7, 2020, 8:45 a.m. UTC
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(-)

Comments

Hans Verkuil Nov. 5, 2020, 12:46 p.m. UTC | #1
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
Hans Verkuil Nov. 5, 2020, 12:50 p.m. UTC | #2
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
>
Sakari Ailus Nov. 5, 2020, 12:55 p.m. UTC | #3
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 mbox series

Patch

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 ||