[PATCH v2] [RFC] add video controls for SUR40 driver

[PATCH v2] [RFC] add video controls for SUR40 driver

[Florian Echtler]

As discussed previously, here's a second iteration of my patch to add
control functions for the SUR40 driver, now using the V4L2 control
framework.

[PATCH 1/5] add missing blob structure field for tag id

[Florian Echtler]

The SUR40 can recognize specific printed patterns directly in hardware;
this information (i.e. the pattern id) is present but currently unused
in the blob structure.

Signed-off-by: Florian Echtler <floe@butterbrot.org>
---
 drivers/input/touchscreen/sur40.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index f16f835..8375b06 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -81,7 +81,10 @@ struct sur40_blob {
 
 	__le32 area;       /* size in pixels/pressure (?) */
 
-	u8 padding[32];
+	u8 padding[24];
+
+	__le32 tag_id;     /* valid when type == 0x04 (SUR40_TAG) */
+	__le32 unknown;
 
 } __packed;
 
-- 
2.7.4

Re: [PATCH 1/5] add missing blob structure field for tag id

[Hans Verkuil]

On 02/06/2018 10:01 PM, Florian Echtler wrote:
> The SUR40 can recognize specific printed patterns directly in hardware;
> this information (i.e. the pattern id) is present but currently unused
> in the blob structure.
> 
> Signed-off-by: Florian Echtler <floe@butterbrot.org>
> ---
>  drivers/input/touchscreen/sur40.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index f16f835..8375b06 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
> @@ -81,7 +81,10 @@ struct sur40_blob {
>  
>  	__le32 area;       /* size in pixels/pressure (?) */
>  
> -	u8 padding[32];
> +	u8 padding[24];
> +
> +	__le32 tag_id;     /* valid when type == 0x04 (SUR40_TAG) */
> +	__le32 unknown;
>  
>  } __packed;
>  
> 

Usually new fields are added before the padding, not after.

Unless there is a good reason for this I'd change this.

Regards,

	Hans

Re: [PATCH 1/5] add missing blob structure field for tag id

[Florian Echtler]

[-- Attachment #1.1: Type: text/plain, Size: 799 bytes --]

On 06.02.2018 22:22, Hans Verkuil wrote:
> On 02/06/2018 10:01 PM, Florian Echtler wrote:
>> The SUR40 can recognize specific printed patterns directly in hardware;
>> this information (i.e. the pattern id) is present but currently unused
>> in the blob structure.
>>
>>  
>>  	__le32 area;       /* size in pixels/pressure (?) */
>>  
>> -	u8 padding[32];
>> +	u8 padding[24];
>> +
>> +	__le32 tag_id;     /* valid when type == 0x04 (SUR40_TAG) */
>> +	__le32 unknown;
>>  
>>  } __packed;
>>  
> Usually new fields are added before the padding, not after.
> 
> Unless there is a good reason for this I'd change this.

This is how the hardware sends it, so there's little choice in how to arrange
the fields...

Best regards, Florian
-- 
SENT FROM MY DEC VT50 TERMINAL


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH 2/5] add control definitions

[Florian Echtler]

This patch adds parameter definitions for the four userspace controls that
the SUR40 can currently provide.

Signed-off-by: Florian Echtler <floe@butterbrot.org>
---
 drivers/input/touchscreen/sur40.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index 8375b06..3af63415 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -149,6 +149,23 @@ struct sur40_image_header {
 #define SUR40_TOUCH	0x02
 #define SUR40_TAG	0x04
 
+/* video controls */
+#define SUR40_BRIGHTNESS_MAX 0xFF
+#define SUR40_BRIGHTNESS_MIN 0x00
+#define SUR40_BRIGHTNESS_DEF 0xFF
+
+#define SUR40_CONTRAST_MAX 0x0F
+#define SUR40_CONTRAST_MIN 0x00
+#define SUR40_CONTRAST_DEF 0x0A
+
+#define SUR40_GAIN_MAX 0x09
+#define SUR40_GAIN_MIN 0x00
+#define SUR40_GAIN_DEF 0x08
+
+#define SUR40_BACKLIGHT_MAX 0x01
+#define SUR40_BACKLIGHT_MIN 0x00
+#define SUR40_BACKLIGHT_DEF 0x01
+
 static const struct v4l2_pix_format sur40_pix_format[] = {
 	{
 		.pixelformat = V4L2_TCH_FMT_TU08,
-- 
2.7.4

[PATCH 3/5] add panel register access functions

[Florian Echtler]

These functions provide write access to the internal LCD panel registers
which also control the sensor. They can be used to disable the
preprocessor, set the illumination brightness, and adjust gain/contrast
(which are stored together in one register internally called "vsvideo").

Signed-off-by: Florian Echtler <floe@butterbrot.org>
---
 drivers/input/touchscreen/sur40.c | 71 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index 3af63415..7f6461d 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -240,6 +240,81 @@ static int sur40_command(struct sur40_state *dev,
 			       0x00, index, buffer, size, 1000);
 }
 
+/* poke a byte in the panel register space */
+static int sur40_poke(struct sur40_state *dev, u8 offset, u8 value)
+{
+	int result;
+	u8 index = 0x96; // 0xae for permanent write
+
+	result = usb_control_msg(dev->usbdev, usb_sndctrlpipe(dev->usbdev, 0),
+		SUR40_POKE, USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
+		0x32, index, NULL, 0, 1000);
+	if (result < 0)
+		goto error;
+	msleep(5);
+
+	result = usb_control_msg(dev->usbdev, usb_sndctrlpipe(dev->usbdev, 0),
+		SUR40_POKE, USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
+		0x72, offset, NULL, 0, 1000);
+	if (result < 0)
+		goto error;
+	msleep(5);
+
+	result = usb_control_msg(dev->usbdev, usb_sndctrlpipe(dev->usbdev, 0),
+		SUR40_POKE, USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
+		0xb2, value, NULL, 0, 1000);
+	if (result < 0)
+		goto error;
+	msleep(5);
+
+error:
+	return result;
+}
+
+static int sur40_set_preprocessor(struct sur40_state *dev, u8 value)
+{
+	u8 setting_07[2] = { 0x01, 0x00 };
+	u8 setting_17[2] = { 0x85, 0x80 };
+	int result;
+
+	if (value > 1)
+		return -ERANGE;
+
+	result = usb_control_msg(dev->usbdev, usb_sndctrlpipe(dev->usbdev, 0),
+		SUR40_POKE, USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
+		0x07, setting_07[value], NULL, 0, 1000);
+	if (result < 0)
+		goto error;
+	msleep(5);
+
+	result = usb_control_msg(dev->usbdev, usb_sndctrlpipe(dev->usbdev, 0),
+		SUR40_POKE, USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
+		0x17, setting_17[value], NULL, 0, 1000);
+	if (result < 0)
+		goto error;
+	msleep(5);
+
+error:
+	return result;
+}
+
+static void sur40_set_vsvideo(struct sur40_state *handle, u8 value)
+{
+	int i;
+
+	for (i = 0; i < 4; i++)
+		sur40_poke(handle, 0x1c+i, value);
+	handle->vsvideo = value;
+}
+
+static void sur40_set_irlevel(struct sur40_state *handle, u8 value)
+{
+	int i;
+
+	for (i = 0; i < 8; i++)
+		sur40_poke(handle, 0x08+(2*i), value);
+}
+
 /* Initialization routine, called from sur40_open */
 static int sur40_init(struct sur40_state *dev)
 {
-- 
2.7.4

[PATCH 4/5] register control handlers using V4L2 control framework

[Florian Echtler]

This patch registers four standard control handlers using the corresponding
V4L2 framework.

Signed-off-by: Florian Echtler <floe@butterbrot.org>
---
 drivers/input/touchscreen/sur40.c | 51 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index 7f6461d..66ef7e6 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -38,6 +38,7 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-dev.h>
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-ctrls.h>
 #include <media/videobuf2-v4l2.h>
 #include <media/videobuf2-dma-sg.h>
 
@@ -198,6 +199,7 @@ struct sur40_state {
 	struct video_device vdev;
 	struct mutex lock;
 	struct v4l2_pix_format pix_fmt;
+	struct v4l2_ctrl_handler ctrls;
 
 	struct vb2_queue queue;
 	struct list_head buf_list;
@@ -207,6 +209,7 @@ struct sur40_state {
 	struct sur40_data *bulk_in_buffer;
 	size_t bulk_in_size;
 	u8 bulk_in_epaddr;
+	u8 vsvideo;
 
 	char phys[64];
 };
@@ -220,6 +223,11 @@ struct sur40_buffer {
 static const struct video_device sur40_video_device;
 static const struct vb2_queue sur40_queue;
 static void sur40_process_video(struct sur40_state *sur40);
+static int sur40_s_ctrl(struct v4l2_ctrl *ctrl);
+
+static const struct v4l2_ctrl_ops sur40_ctrl_ops = {
+	.s_ctrl = sur40_s_ctrl,
+};
 
 /*
  * Note: an earlier, non-public version of this driver used USB_RECIP_ENDPOINT
@@ -722,6 +730,23 @@ static int sur40_probe(struct usb_interface *interface,
 	sur40->vdev.queue = &sur40->queue;
 	video_set_drvdata(&sur40->vdev, sur40);
 
+	/* initialize the control handler for 4 controls */
+	v4l2_ctrl_handler_init(&sur40->ctrls, 4);
+	sur40->v4l2.ctrl_handler = &sur40->ctrls;
+
+	v4l2_ctrl_new_std(&sur40->ctrls, &sur40_ctrl_ops, V4L2_CID_BRIGHTNESS,
+	  SUR40_BRIGHTNESS_MIN, SUR40_BRIGHTNESS_MAX, 1, SUR40_BRIGHTNESS_DEF);
+
+	v4l2_ctrl_new_std(&sur40->ctrls, &sur40_ctrl_ops, V4L2_CID_CONTRAST,
+	  SUR40_CONTRAST_MIN, SUR40_CONTRAST_MAX, 1, SUR40_CONTRAST_DEF);
+
+	v4l2_ctrl_new_std(&sur40->ctrls, &sur40_ctrl_ops, V4L2_CID_GAIN,
+	  SUR40_GAIN_MIN, SUR40_GAIN_MAX, 1, SUR40_GAIN_DEF);
+
+	v4l2_ctrl_new_std(&sur40->ctrls, &sur40_ctrl_ops,
+	  V4L2_CID_BACKLIGHT_COMPENSATION, SUR40_BACKLIGHT_MIN,
+	  SUR40_BACKLIGHT_MAX, 1, SUR40_BACKLIGHT_DEF);
+
 	error = video_register_device(&sur40->vdev, VFL_TYPE_TOUCH, -1);
 	if (error) {
 		dev_err(&interface->dev,
@@ -754,6 +779,7 @@ static void sur40_disconnect(struct usb_interface *interface)
 {
 	struct sur40_state *sur40 = usb_get_intfdata(interface);
 
+	v4l2_ctrl_handler_free(&sur40->ctrls);
 	video_unregister_device(&sur40->vdev);
 	v4l2_device_unregister(&sur40->v4l2);
 
@@ -947,6 +973,31 @@ static int sur40_vidioc_g_fmt(struct file *file, void *priv,
 	return 0;
 }
 
+static int sur40_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct sur40_state *sur40  = container_of(ctrl->handler,
+	  struct sur40_state, ctrls);
+	u8 value = sur40->vsvideo;
+
+	switch (ctrl->id) {
+	case V4L2_CID_BRIGHTNESS:
+		sur40_set_irlevel(sur40, ctrl->val);
+		break;
+	case V4L2_CID_CONTRAST:
+		value = (value & 0x0F) | (ctrl->val << 4);
+		sur40_set_vsvideo(sur40, value);
+		break;
+	case V4L2_CID_GAIN:
+		value = (value & 0xF0) | (ctrl->val);
+		sur40_set_vsvideo(sur40, value);
+		break;
+	case V4L2_CID_BACKLIGHT_COMPENSATION:
+		sur40_set_preprocessor(sur40, ctrl->val);
+		break;
+	}
+	return 0;
+}
+
 static int sur40_ioctl_parm(struct file *file, void *priv,
 			    struct v4l2_streamparm *p)
 {
-- 
2.7.4

Re: [PATCH 4/5] register control handlers using V4L2 control framework

[Hans Verkuil]

On 02/06/2018 10:01 PM, Florian Echtler wrote:
> This patch registers four standard control handlers using the corresponding
> V4L2 framework.
> 
> Signed-off-by: Florian Echtler <floe@butterbrot.org>
> ---
>  drivers/input/touchscreen/sur40.c | 51 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index 7f6461d..66ef7e6 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
> @@ -38,6 +38,7 @@
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-dev.h>
>  #include <media/v4l2-ioctl.h>
> +#include <media/v4l2-ctrls.h>
>  #include <media/videobuf2-v4l2.h>
>  #include <media/videobuf2-dma-sg.h>
>  
> @@ -198,6 +199,7 @@ struct sur40_state {
>  	struct video_device vdev;
>  	struct mutex lock;
>  	struct v4l2_pix_format pix_fmt;
> +	struct v4l2_ctrl_handler ctrls;

This is normally called 'hdl' or 'ctrl_handler'.

>  
>  	struct vb2_queue queue;
>  	struct list_head buf_list;
> @@ -207,6 +209,7 @@ struct sur40_state {
>  	struct sur40_data *bulk_in_buffer;
>  	size_t bulk_in_size;
>  	u8 bulk_in_epaddr;
> +	u8 vsvideo;
>  
>  	char phys[64];
>  };
> @@ -220,6 +223,11 @@ struct sur40_buffer {
>  static const struct video_device sur40_video_device;
>  static const struct vb2_queue sur40_queue;
>  static void sur40_process_video(struct sur40_state *sur40);
> +static int sur40_s_ctrl(struct v4l2_ctrl *ctrl);
> +
> +static const struct v4l2_ctrl_ops sur40_ctrl_ops = {
> +	.s_ctrl = sur40_s_ctrl,
> +};
>  
>  /*
>   * Note: an earlier, non-public version of this driver used USB_RECIP_ENDPOINT
> @@ -722,6 +730,23 @@ static int sur40_probe(struct usb_interface *interface,
>  	sur40->vdev.queue = &sur40->queue;
>  	video_set_drvdata(&sur40->vdev, sur40);
>  
> +	/* initialize the control handler for 4 controls */
> +	v4l2_ctrl_handler_init(&sur40->ctrls, 4);
> +	sur40->v4l2.ctrl_handler = &sur40->ctrls;
> +
> +	v4l2_ctrl_new_std(&sur40->ctrls, &sur40_ctrl_ops, V4L2_CID_BRIGHTNESS,
> +	  SUR40_BRIGHTNESS_MIN, SUR40_BRIGHTNESS_MAX, 1, SUR40_BRIGHTNESS_DEF);
> +
> +	v4l2_ctrl_new_std(&sur40->ctrls, &sur40_ctrl_ops, V4L2_CID_CONTRAST,
> +	  SUR40_CONTRAST_MIN, SUR40_CONTRAST_MAX, 1, SUR40_CONTRAST_DEF);
> +
> +	v4l2_ctrl_new_std(&sur40->ctrls, &sur40_ctrl_ops, V4L2_CID_GAIN,
> +	  SUR40_GAIN_MIN, SUR40_GAIN_MAX, 1, SUR40_GAIN_DEF);
> +
> +	v4l2_ctrl_new_std(&sur40->ctrls, &sur40_ctrl_ops,
> +	  V4L2_CID_BACKLIGHT_COMPENSATION, SUR40_BACKLIGHT_MIN,
> +	  SUR40_BACKLIGHT_MAX, 1, SUR40_BACKLIGHT_DEF);

You need to check if the creation succeeded:

	if (ctrls->error) {
		v4l2_ctrl_handler_free(ctrls);
		// exit
	}
> +
>  	error = video_register_device(&sur40->vdev, VFL_TYPE_TOUCH, -1);
>  	if (error) {
>  		dev_err(&interface->dev,
> @@ -754,6 +779,7 @@ static void sur40_disconnect(struct usb_interface *interface)
>  {
>  	struct sur40_state *sur40 = usb_get_intfdata(interface);
>  
> +	v4l2_ctrl_handler_free(&sur40->ctrls);
>  	video_unregister_device(&sur40->vdev);
>  	v4l2_device_unregister(&sur40->v4l2);
>  
> @@ -947,6 +973,31 @@ static int sur40_vidioc_g_fmt(struct file *file, void *priv,
>  	return 0;
>  }
>  
> +static int sur40_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct sur40_state *sur40  = container_of(ctrl->handler,
> +	  struct sur40_state, ctrls);
> +	u8 value = sur40->vsvideo;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_BRIGHTNESS:
> +		sur40_set_irlevel(sur40, ctrl->val);
> +		break;
> +	case V4L2_CID_CONTRAST:
> +		value = (value & 0x0F) | (ctrl->val << 4);
> +		sur40_set_vsvideo(sur40, value);
> +		break;
> +	case V4L2_CID_GAIN:
> +		value = (value & 0xF0) | (ctrl->val);
> +		sur40_set_vsvideo(sur40, value);
> +		break;
> +	case V4L2_CID_BACKLIGHT_COMPENSATION:
> +		sur40_set_preprocessor(sur40, ctrl->val);
> +		break;
> +	}
> +	return 0;
> +}
> +
>  static int sur40_ioctl_parm(struct file *file, void *priv,
>  			    struct v4l2_streamparm *p)
>  {
> 

Looks good otherwise.

	Hans

[PATCH 5/5] add module parameters for default values

[Florian Echtler]

To allow setting custom parameters for the sensor directly at startup, the
three primary controls are exposed as module parameters in this patch.

Signed-off-by: Florian Echtler <floe@butterbrot.org>
---
 drivers/input/touchscreen/sur40.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index 66ef7e6..d1fcb95 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -167,6 +167,17 @@ struct sur40_image_header {
 #define SUR40_BACKLIGHT_MIN 0x00
 #define SUR40_BACKLIGHT_DEF 0x01
 
+/* module parameters */
+static uint brightness = SUR40_BRIGHTNESS_DEF;
+module_param(brightness, uint, 0644);
+MODULE_PARM_DESC(brightness, "set default brightness");
+static uint contrast = SUR40_CONTRAST_DEF;
+module_param(contrast, uint, 0644);
+MODULE_PARM_DESC(contrast, "set default contrast");
+static uint gain = SUR40_GAIN_DEF;
+module_param(gain, uint, 0644);
+MODULE_PARM_DESC(contrast, "set default gain");
+
 static const struct v4l2_pix_format sur40_pix_format[] = {
 	{
 		.pixelformat = V4L2_TCH_FMT_TU08,
@@ -374,6 +385,11 @@ static void sur40_open(struct input_polled_dev *polldev)
 
 	dev_dbg(sur40->dev, "open\n");
 	sur40_init(sur40);
+
+	/* set default values */
+	sur40_set_irlevel(sur40, brightness & 0xFF);
+	sur40_set_vsvideo(sur40, ((contrast & 0x0F) << 4) | (gain & 0x0F));
+	sur40_set_preprocessor(sur40, SUR40_BACKLIGHT_DEF);
 }
 
 /* Disable device, polling has stopped. */
-- 
2.7.4

Re: [PATCH 5/5] add module parameters for default values

[Hans Verkuil]

On 02/06/2018 10:01 PM, Florian Echtler wrote:
> To allow setting custom parameters for the sensor directly at startup, the
> three primary controls are exposed as module parameters in this patch.
> 
> Signed-off-by: Florian Echtler <floe@butterbrot.org>
> ---
>  drivers/input/touchscreen/sur40.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index 66ef7e6..d1fcb95 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
> @@ -167,6 +167,17 @@ struct sur40_image_header {
>  #define SUR40_BACKLIGHT_MIN 0x00
>  #define SUR40_BACKLIGHT_DEF 0x01
>  
> +/* module parameters */
> +static uint brightness = SUR40_BRIGHTNESS_DEF;
> +module_param(brightness, uint, 0644);
> +MODULE_PARM_DESC(brightness, "set default brightness");
> +static uint contrast = SUR40_CONTRAST_DEF;
> +module_param(contrast, uint, 0644);
> +MODULE_PARM_DESC(contrast, "set default contrast");
> +static uint gain = SUR40_GAIN_DEF;
> +module_param(gain, uint, 0644);
> +MODULE_PARM_DESC(contrast, "set default gain");

contrast -> gain

Isn't 'initial gain' better than 'default gain'?

If I load this module with gain=X, will the gain control also
start off at X? I didn't see any code for that.

It might be useful to add the allowed range in the description.
E.g.: "set initial gain, range=0-255". Perhaps mention even the
default value, but I'm not sure if that's really needed.

Regards,

	Hans

> +
>  static const struct v4l2_pix_format sur40_pix_format[] = {
>  	{
>  		.pixelformat = V4L2_TCH_FMT_TU08,
> @@ -374,6 +385,11 @@ static void sur40_open(struct input_polled_dev *polldev)
>  
>  	dev_dbg(sur40->dev, "open\n");
>  	sur40_init(sur40);
> +
> +	/* set default values */
> +	sur40_set_irlevel(sur40, brightness & 0xFF);
> +	sur40_set_vsvideo(sur40, ((contrast & 0x0F) << 4) | (gain & 0x0F));
> +	sur40_set_preprocessor(sur40, SUR40_BACKLIGHT_DEF);
>  }
>  
>  /* Disable device, polling has stopped. */
> 

Re: [PATCH 5/5] add module parameters for default values

[Florian Echtler]

[-- Attachment #1.1: Type: text/plain, Size: 1681 bytes --]

On 06.02.2018 22:31, Hans Verkuil wrote:
> On 02/06/2018 10:01 PM, Florian Echtler wrote:
>> To allow setting custom parameters for the sensor directly at startup, the
>> three primary controls are exposed as module parameters in this patch.
>>
>> +/* module parameters */
>> +static uint brightness = SUR40_BRIGHTNESS_DEF;
>> +module_param(brightness, uint, 0644);
>> +MODULE_PARM_DESC(brightness, "set default brightness");
>> +static uint contrast = SUR40_CONTRAST_DEF;
>> +module_param(contrast, uint, 0644);
>> +MODULE_PARM_DESC(contrast, "set default contrast");
>> +static uint gain = SUR40_GAIN_DEF;
>> +module_param(gain, uint, 0644);
>> +MODULE_PARM_DESC(contrast, "set default gain");
>
> contrast -> gain

Ah, typo. Thanks, will fix that.

> Isn't 'initial gain' better than 'default gain'?

Probably correct, yes.

> If I load this module with gain=X, will the gain control also
> start off at X? I didn't see any code for that.

This reminds me: how can I get/set the control from inside the driver?
Should I use something like the following:

struct v4l2_ctrl *ctrl = v4l2_ctrl_find(&sur40->ctrls, V4L2_CID_BRIGHTNESS);
int val = v4l2_ctrl_g_ctrl(ctrl);
// modify val...
v4l2_ctrl_s_ctrl(ctrl, val);

> It might be useful to add the allowed range in the description.
> E.g.: "set initial gain, range=0-255". Perhaps mention even the
> default value, but I'm not sure if that's really needed.

Good point, though - right now the code directly sets the registers without any
clamping, I guess it would be better to call the control framework as mentioned
above?

Best regards, Florian
-- 
SENT FROM MY DEC VT50 TERMINAL


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 5/5] add module parameters for default values

[Hans Verkuil]

On 02/07/18 09:33, Florian Echtler wrote:
> On 06.02.2018 22:31, Hans Verkuil wrote:
>> On 02/06/2018 10:01 PM, Florian Echtler wrote:
>>> To allow setting custom parameters for the sensor directly at startup, the
>>> three primary controls are exposed as module parameters in this patch.
>>>
>>> +/* module parameters */
>>> +static uint brightness = SUR40_BRIGHTNESS_DEF;
>>> +module_param(brightness, uint, 0644);
>>> +MODULE_PARM_DESC(brightness, "set default brightness");
>>> +static uint contrast = SUR40_CONTRAST_DEF;
>>> +module_param(contrast, uint, 0644);
>>> +MODULE_PARM_DESC(contrast, "set default contrast");
>>> +static uint gain = SUR40_GAIN_DEF;
>>> +module_param(gain, uint, 0644);
>>> +MODULE_PARM_DESC(contrast, "set default gain");
>>
>> contrast -> gain
> 
> Ah, typo. Thanks, will fix that.
> 
>> Isn't 'initial gain' better than 'default gain'?
> 
> Probably correct, yes.
> 
>> If I load this module with gain=X, will the gain control also
>> start off at X? I didn't see any code for that.
> 
> This reminds me: how can I get/set the control from inside the driver?
> Should I use something like the following:
> 
> struct v4l2_ctrl *ctrl = v4l2_ctrl_find(&sur40->ctrls, V4L2_CID_BRIGHTNESS);
> int val = v4l2_ctrl_g_ctrl(ctrl);
> // modify val...
> v4l2_ctrl_s_ctrl(ctrl, val);

Yes, that's correct. Usually drivers store the ctrl in their state struct
when they create the control. That way you don't have to find it.

> 
>> It might be useful to add the allowed range in the description.
>> E.g.: "set initial gain, range=0-255". Perhaps mention even the
>> default value, but I'm not sure if that's really needed.
> 
> Good point, though - right now the code directly sets the registers without any
> clamping, I guess it would be better to call the control framework as mentioned
> above?

Easiest is just to use this value for the default when you create the
control. Just clamp it first.

E.g.:

static uint gain = SUR40_GAIN_DEF;
module_param(gain, uint, 0644);

...

gain = clamp(gain, SUR40_GAIN_MIN, SUR40_GAIN_MAX);
v4l2_ctrl_new_std(&sur40->ctrls, &sur40_ctrl_ops, V4L2_CID_GAIN,
	  SUR40_GAIN_MIN, SUR40_GAIN_MAX, 1, gain);

You need to clamp gain first, otherwise v4l2_ctrl_new_std would fail
if the given default value is out of range.

Regards,

	Hans