[PATCH 0/5] [RFC] add video controls to SUR40 driver

[PATCH 0/5] [RFC] add video controls to SUR40 driver

[Florian Echtler]

The SUR40 (aka Pixelsense) has internal registers that expose sensor
parameters such as brightness, gain etc. This patch creates V4L2
control items and maps them to the appropriate parameters.

This is an initial submission for review, comments welcome!

Best regards, Florian

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

[Florian Echtler]

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

[PATCH 2/5] add video control register definitions

[Florian Echtler]

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

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index 8375b06..0dbb004 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -149,6 +149,30 @@ 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_VSVIDEO_DEF 0x86
+
+#define SUR40_BACKLIGHT_MAX 0x01
+#define SUR40_BACKLIGHT_MIN 0x00
+#define SUR40_BACKLIGHT_DEF 0x01
+
+int sur40_v4l2_brightness = SUR40_BRIGHTNESS_DEF; /* infrared     */
+int sur40_v4l2_contrast   = SUR40_CONTRAST_DEF;   /* blacklevel   */
+int sur40_v4l2_gain       = SUR40_GAIN_DEF;       /* gain         */
+int sur40_v4l2_backlight  = 1;                    /* preprocessor */
+
 static const struct v4l2_pix_format sur40_pix_format[] = {
 	{
 		.pixelformat = V4L2_TCH_FMT_TU08,
-- 
2.7.4

[PATCH 3/5] add video control register handlers

[Florian Echtler]

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

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index 0dbb004..63c7264b 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -247,6 +255,80 @@ 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);
+}
+
+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] add V4L2 control functions

[Florian Echtler]

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

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index 63c7264b..c4b7cf1 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -953,6 +953,119 @@ static int sur40_vidioc_g_fmt(struct file *file, void *priv,
 	return 0;
 }
 
+
+static int sur40_vidioc_queryctrl(struct file *file, void *fh,
+			       struct v4l2_queryctrl *qc)
+{
+
+	switch (qc->id) {
+	case V4L2_CID_BRIGHTNESS:
+		qc->flags = 0;
+		sprintf(qc->name, "Brightness");
+		qc->type = V4L2_CTRL_TYPE_INTEGER;
+		qc->minimum = SUR40_BRIGHTNESS_MIN;
+		qc->default_value = SUR40_BRIGHTNESS_DEF;
+		qc->maximum = SUR40_BRIGHTNESS_MAX;
+		qc->step = 8;
+		return 0;
+	case V4L2_CID_CONTRAST:
+		qc->flags = 0;
+		sprintf(qc->name, "Contrast");
+		qc->type = V4L2_CTRL_TYPE_INTEGER;
+		qc->minimum = SUR40_CONTRAST_MIN;
+		qc->default_value = SUR40_CONTRAST_DEF;
+		qc->maximum = SUR40_CONTRAST_MAX;
+		qc->step = 1;
+		return 0;
+	case V4L2_CID_GAIN:
+		qc->flags = 0;
+		sprintf(qc->name, "Gain");
+		qc->type = V4L2_CTRL_TYPE_INTEGER;
+		qc->minimum = SUR40_GAIN_MIN;
+		qc->default_value = SUR40_GAIN_DEF;
+		qc->maximum = SUR40_GAIN_MAX;
+		qc->step = 1;
+		return 0;
+	case V4L2_CID_BACKLIGHT_COMPENSATION:
+		qc->flags = 0;
+		sprintf(qc->name, "Preprocessor");
+		qc->type = V4L2_CTRL_TYPE_INTEGER;
+		qc->minimum = SUR40_BACKLIGHT_MIN;
+		qc->default_value = SUR40_BACKLIGHT_DEF;
+		qc->maximum = SUR40_BACKLIGHT_MAX;
+		qc->step = 1;
+		return 0;
+	default:
+		qc->flags = V4L2_CTRL_FLAG_DISABLED;
+		return -EINVAL;
+	}
+}
+
+static int sur40_vidioc_g_ctrl(struct file *file, void *fh,
+			    struct v4l2_control *ctrl)
+{
+
+	switch (ctrl->id) {
+	case V4L2_CID_BRIGHTNESS:
+		ctrl->value = sur40_v4l2_brightness;
+		return 0;
+	case V4L2_CID_CONTRAST:
+		ctrl->value = sur40_v4l2_contrast;
+		return 0;
+	case V4L2_CID_GAIN:
+		ctrl->value = sur40_v4l2_gain;
+		return 0;
+	case V4L2_CID_BACKLIGHT_COMPENSATION:
+		ctrl->value = sur40_v4l2_backlight;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int sur40_vidioc_s_ctrl(struct file *file, void *fh,
+			    struct v4l2_control *ctrl)
+{
+	u8 value = 0;
+	struct sur40_state *sur40 = video_drvdata(file);
+
+	switch (ctrl->id) {
+	case V4L2_CID_BRIGHTNESS:
+		sur40_v4l2_brightness = ctrl->value;
+		if (sur40_v4l2_brightness < SUR40_BRIGHTNESS_MIN)
+			sur40_v4l2_brightness = SUR40_BRIGHTNESS_MIN;
+		else if (sur40_v4l2_brightness > SUR40_BRIGHTNESS_MAX)
+			sur40_v4l2_brightness = SUR40_BRIGHTNESS_MAX;
+		sur40_set_irlevel(sur40, sur40_v4l2_brightness);
+		return 0;
+	case V4L2_CID_CONTRAST:
+		sur40_v4l2_contrast = ctrl->value;
+		if (sur40_v4l2_contrast < SUR40_CONTRAST_MIN)
+			sur40_v4l2_contrast = SUR40_CONTRAST_MIN;
+		else if (sur40_v4l2_contrast > SUR40_CONTRAST_MAX)
+			sur40_v4l2_contrast = SUR40_CONTRAST_MAX;
+		value = (sur40_v4l2_contrast << 4) + sur40_v4l2_gain;
+		sur40_set_vsvideo(sur40, value);
+		return 0;
+	case V4L2_CID_GAIN:
+		sur40_v4l2_gain = ctrl->value;
+		if (sur40_v4l2_gain < SUR40_GAIN_MIN)
+			sur40_v4l2_gain = SUR40_GAIN_MIN;
+		else if (sur40_v4l2_gain > SUR40_GAIN_MAX)
+			sur40_v4l2_gain = SUR40_GAIN_MAX;
+		value = (sur40_v4l2_contrast << 4) + sur40_v4l2_gain;
+		sur40_set_vsvideo(sur40, value);
+		return 0;
+	case V4L2_CID_BACKLIGHT_COMPENSATION:
+		sur40_v4l2_backlight = ctrl->value;
+		sur40_set_preprocessor(sur40, sur40_v4l2_backlight);
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+
 static int sur40_ioctl_parm(struct file *file, void *priv,
 			    struct v4l2_streamparm *p)
 {
@@ -1071,6 +1181,10 @@ static const struct v4l2_ioctl_ops sur40_video_ioctl_ops = {
 	.vidioc_g_input		= sur40_vidioc_g_input,
 	.vidioc_s_input		= sur40_vidioc_s_input,
 
+	.vidioc_queryctrl	= sur40_vidioc_queryctrl,
+	.vidioc_g_ctrl		= sur40_vidioc_g_ctrl,
+	.vidioc_s_ctrl		= sur40_vidioc_s_ctrl,
+
 	.vidioc_reqbufs		= vb2_ioctl_reqbufs,
 	.vidioc_create_bufs	= vb2_ioctl_create_bufs,
 	.vidioc_querybuf	= vb2_ioctl_querybuf,
-- 
2.7.4

[PATCH 5/5] add default control values as module parameters

[Florian Echtler]

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

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index c4b7cf1..d612f3f 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -173,6 +173,14 @@ int sur40_v4l2_contrast   = SUR40_CONTRAST_DEF;   /* blacklevel   */
 int sur40_v4l2_gain       = SUR40_GAIN_DEF;       /* gain         */
 int sur40_v4l2_backlight  = 1;                    /* preprocessor */
 
+/* module parameters */
+static uint irlevel = SUR40_BRIGHTNESS_DEF;
+module_param(irlevel, uint, 0644);
+MODULE_PARM_DESC(irlevel, "set default irlevel");
+static uint vsvideo = SUR40_VSVIDEO_DEF;
+module_param(vsvideo, uint, 0644);
+MODULE_PARM_DESC(vsvideo, "set default vsvideo");
+
 static const struct v4l2_pix_format sur40_pix_format[] = {
 	{
 		.pixelformat = V4L2_TCH_FMT_TU08,
@@ -372,6 +380,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, irlevel);
+	sur40_set_vsvideo(sur40, vsvideo);
+	sur40_set_preprocessor(sur40, SUR40_BACKLIGHT_DEF);
 }
 
 /* Disable device, polling has stopped. */
-- 
2.7.4

Re: [PATCH 4/5] add V4L2 control functions

[Hans Verkuil]

On 02/05/2018 03:29 PM, Florian Echtler wrote:
> Signed-off-by: Florian Echtler <floe@butterbrot.org>
> ---
>  drivers/input/touchscreen/sur40.c | 114 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 114 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index 63c7264b..c4b7cf1 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
> @@ -953,6 +953,119 @@ static int sur40_vidioc_g_fmt(struct file *file, void *priv,
>  	return 0;
>  }
>  
> +
> +static int sur40_vidioc_queryctrl(struct file *file, void *fh,
> +			       struct v4l2_queryctrl *qc)
> +{
> +
> +	switch (qc->id) {
> +	case V4L2_CID_BRIGHTNESS:
> +		qc->flags = 0;
> +		sprintf(qc->name, "Brightness");
> +		qc->type = V4L2_CTRL_TYPE_INTEGER;
> +		qc->minimum = SUR40_BRIGHTNESS_MIN;
> +		qc->default_value = SUR40_BRIGHTNESS_DEF;
> +		qc->maximum = SUR40_BRIGHTNESS_MAX;
> +		qc->step = 8;
> +		return 0;
> +	case V4L2_CID_CONTRAST:
> +		qc->flags = 0;
> +		sprintf(qc->name, "Contrast");
> +		qc->type = V4L2_CTRL_TYPE_INTEGER;
> +		qc->minimum = SUR40_CONTRAST_MIN;
> +		qc->default_value = SUR40_CONTRAST_DEF;
> +		qc->maximum = SUR40_CONTRAST_MAX;
> +		qc->step = 1;
> +		return 0;
> +	case V4L2_CID_GAIN:
> +		qc->flags = 0;
> +		sprintf(qc->name, "Gain");
> +		qc->type = V4L2_CTRL_TYPE_INTEGER;
> +		qc->minimum = SUR40_GAIN_MIN;
> +		qc->default_value = SUR40_GAIN_DEF;
> +		qc->maximum = SUR40_GAIN_MAX;
> +		qc->step = 1;
> +		return 0;
> +	case V4L2_CID_BACKLIGHT_COMPENSATION:
> +		qc->flags = 0;
> +		sprintf(qc->name, "Preprocessor");
> +		qc->type = V4L2_CTRL_TYPE_INTEGER;
> +		qc->minimum = SUR40_BACKLIGHT_MIN;
> +		qc->default_value = SUR40_BACKLIGHT_DEF;
> +		qc->maximum = SUR40_BACKLIGHT_MAX;
> +		qc->step = 1;
> +		return 0;
> +	default:
> +		qc->flags = V4L2_CTRL_FLAG_DISABLED;
> +		return -EINVAL;
> +	}
> +}
> +
> +static int sur40_vidioc_g_ctrl(struct file *file, void *fh,
> +			    struct v4l2_control *ctrl)
> +{
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_BRIGHTNESS:
> +		ctrl->value = sur40_v4l2_brightness;
> +		return 0;
> +	case V4L2_CID_CONTRAST:
> +		ctrl->value = sur40_v4l2_contrast;
> +		return 0;
> +	case V4L2_CID_GAIN:
> +		ctrl->value = sur40_v4l2_gain;
> +		return 0;
> +	case V4L2_CID_BACKLIGHT_COMPENSATION:
> +		ctrl->value = sur40_v4l2_backlight;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int sur40_vidioc_s_ctrl(struct file *file, void *fh,
> +			    struct v4l2_control *ctrl)
> +{
> +	u8 value = 0;
> +	struct sur40_state *sur40 = video_drvdata(file);
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_BRIGHTNESS:
> +		sur40_v4l2_brightness = ctrl->value;
> +		if (sur40_v4l2_brightness < SUR40_BRIGHTNESS_MIN)
> +			sur40_v4l2_brightness = SUR40_BRIGHTNESS_MIN;
> +		else if (sur40_v4l2_brightness > SUR40_BRIGHTNESS_MAX)
> +			sur40_v4l2_brightness = SUR40_BRIGHTNESS_MAX;
> +		sur40_set_irlevel(sur40, sur40_v4l2_brightness);
> +		return 0;
> +	case V4L2_CID_CONTRAST:
> +		sur40_v4l2_contrast = ctrl->value;
> +		if (sur40_v4l2_contrast < SUR40_CONTRAST_MIN)
> +			sur40_v4l2_contrast = SUR40_CONTRAST_MIN;
> +		else if (sur40_v4l2_contrast > SUR40_CONTRAST_MAX)
> +			sur40_v4l2_contrast = SUR40_CONTRAST_MAX;
> +		value = (sur40_v4l2_contrast << 4) + sur40_v4l2_gain;
> +		sur40_set_vsvideo(sur40, value);
> +		return 0;
> +	case V4L2_CID_GAIN:
> +		sur40_v4l2_gain = ctrl->value;
> +		if (sur40_v4l2_gain < SUR40_GAIN_MIN)
> +			sur40_v4l2_gain = SUR40_GAIN_MIN;
> +		else if (sur40_v4l2_gain > SUR40_GAIN_MAX)
> +			sur40_v4l2_gain = SUR40_GAIN_MAX;
> +		value = (sur40_v4l2_contrast << 4) + sur40_v4l2_gain;
> +		sur40_set_vsvideo(sur40, value);
> +		return 0;
> +	case V4L2_CID_BACKLIGHT_COMPENSATION:
> +		sur40_v4l2_backlight = ctrl->value;
> +		sur40_set_preprocessor(sur40, sur40_v4l2_backlight);
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +
>  static int sur40_ioctl_parm(struct file *file, void *priv,
>  			    struct v4l2_streamparm *p)
>  {
> @@ -1071,6 +1181,10 @@ static const struct v4l2_ioctl_ops sur40_video_ioctl_ops = {
>  	.vidioc_g_input		= sur40_vidioc_g_input,
>  	.vidioc_s_input		= sur40_vidioc_s_input,
>  
> +	.vidioc_queryctrl	= sur40_vidioc_queryctrl,
> +	.vidioc_g_ctrl		= sur40_vidioc_g_ctrl,
> +	.vidioc_s_ctrl		= sur40_vidioc_s_ctrl,
> +
>  	.vidioc_reqbufs		= vb2_ioctl_reqbufs,
>  	.vidioc_create_bufs	= vb2_ioctl_create_bufs,
>  	.vidioc_querybuf	= vb2_ioctl_querybuf,
> 

Sorry, but this is very wrong. Use the control framework instead. See
https://hverkuil.home.xs4all.nl/spec/kapi/v4l2-controls.html and pretty much all
media drivers (with the exception of uvc). See for example this driver:
drivers/media/pci/tw68/tw68-video.c (randomly chosen).

It actually makes life a lot easier for you as you don't have to perform any
range checks and all control ioctls are automatically supported for you.

Regards,

	Hans

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

[Hans Verkuil]

On 02/05/2018 03:29 PM, Florian Echtler wrote:
> Signed-off-by: Florian Echtler <floe@butterbrot.org>

Please add a change log when you make a patch.

I for one would like to know why this has to be supplied as a module option.
Some documentation in the code would be helpful as well (e.g. I have no idea
what a 'vsvideo' is).

Regards,

	Hans

> ---
>  drivers/input/touchscreen/sur40.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index c4b7cf1..d612f3f 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
> @@ -173,6 +173,14 @@ int sur40_v4l2_contrast   = SUR40_CONTRAST_DEF;   /* blacklevel   */
>  int sur40_v4l2_gain       = SUR40_GAIN_DEF;       /* gain         */
>  int sur40_v4l2_backlight  = 1;                    /* preprocessor */
>  
> +/* module parameters */
> +static uint irlevel = SUR40_BRIGHTNESS_DEF;
> +module_param(irlevel, uint, 0644);
> +MODULE_PARM_DESC(irlevel, "set default irlevel");
> +static uint vsvideo = SUR40_VSVIDEO_DEF;
> +module_param(vsvideo, uint, 0644);
> +MODULE_PARM_DESC(vsvideo, "set default vsvideo");
> +
>  static const struct v4l2_pix_format sur40_pix_format[] = {
>  	{
>  		.pixelformat = V4L2_TCH_FMT_TU08,
> @@ -372,6 +380,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, irlevel);
> +	sur40_set_vsvideo(sur40, vsvideo);
> +	sur40_set_preprocessor(sur40, SUR40_BACKLIGHT_DEF);
>  }
>  
>  /* Disable device, polling has stopped. */
> 

Re: [PATCH 4/5] add V4L2 control functions

[Florian Echtler]

Hello Hans,

On Mon, 5 Feb 2018, Hans Verkuil wrote:
> On 02/05/2018 03:29 PM, Florian Echtler wrote:
>> +
>> +static int sur40_vidioc_queryctrl(struct file *file, void *fh,
>> +			       struct v4l2_queryctrl *qc)
>
> Sorry, but this is very wrong. Use the control framework instead. See
> https://hverkuil.home.xs4all.nl/spec/kapi/v4l2-controls.html and pretty much all
> media drivers (with the exception of uvc). See for example this driver:
> drivers/media/pci/tw68/tw68-video.c (randomly chosen).
>
> It actually makes life a lot easier for you as you don't have to perform any
> range checks and all control ioctls are automatically supported for you.

thanks for the quick reply, I wasn't aware of that framework at all :-) 
Will certainly use it.

What's the earliest kernel version this is supported on, in case we want 
to backport this for our standalone module, too?

Best regards, Florian
-- 
"_Nothing_ brightens up my morning. Coffee simply provides a shade of
grey just above the pitch-black of the infinite depths of the _abyss_."

Re: [PATCH 4/5] add V4L2 control functions

[Hans Verkuil]

On 02/05/2018 10:36 PM, Florian Echtler wrote:
> Hello Hans,
> 
> On Mon, 5 Feb 2018, Hans Verkuil wrote:
>> On 02/05/2018 03:29 PM, Florian Echtler wrote:
>>> +
>>> +static int sur40_vidioc_queryctrl(struct file *file, void *fh,
>>> +			       struct v4l2_queryctrl *qc)
>>
>> Sorry, but this is very wrong. Use the control framework instead. See
>> https://hverkuil.home.xs4all.nl/spec/kapi/v4l2-controls.html and pretty much all
>> media drivers (with the exception of uvc). See for example this driver:
>> drivers/media/pci/tw68/tw68-video.c (randomly chosen).
>>
>> It actually makes life a lot easier for you as you don't have to perform any
>> range checks and all control ioctls are automatically supported for you.
> 
> thanks for the quick reply, I wasn't aware of that framework at all :-) 
> Will certainly use it.
> 
> What's the earliest kernel version this is supported on, in case we want 
> to backport this for our standalone module, too?

Initial commit was in August 2010, so it's been around for quite some time :-)

Regards,

	Hans

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

[Florian Echtler]

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

On 05.02.2018 15:56, Hans Verkuil wrote:
> Please add a change log when you make a patch.
> I for one would like to know why this has to be supplied as a module option.

The idea here was that each individual SUR40 device will likely have slightly
different "ideal" settings for the parameters, and by using the module options,
you can set them right away at startup.

I'm aware that the usual way to do this would be from userspace using v4l2-ctl,
but the SUR40 is sort of a special case: the video settings will also influence
the internal touch detection, and in the worst case, starting the driver with
the default parameters from flash will immediately cause so many false-positive
touch points to be detected that the graphical shell becomes unusable. This has
actually happened several times during testing, so we considered a module option
to be the easiest way for dealing with this.

> Some documentation in the code would be helpful as well (e.g. I have no idea
> what a 'vsvideo' is).
Right - will do that, too.

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


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