[PATCH v2 0/4] Add support for touchscreen on Colibri VF50

[PATCH v2 0/4] Add support for touchscreen on Colibri VF50

[Sanchayan Maity]

Hello,

The patchset adds support for 4 wire touchscreen on Toradex Colibri
VF50 modules. Patches are based on top of shawn's branch.

Changes since v1:
1. Fix/drop comments
2. Use an inline function for multiple gpiod_get calls in probe
3. Remove the pull up in the pinmux specified in DT for touchctrl_gpios
4. Add the io-channel-cells property before status property.
5. Add GPIOLIB as dependency in the Kconfig file

Version 1 of the patchset can be found here
https://lkml.org/lkml/2015/6/30/103

Thank you for the feedback Stefan.

Thanks & Regards,
Sanchayan.


Sanchayan Maity (4):
  ARM: dts: vfxxx: Add io-channel-cells property for ADC node
  ARM: dts: vf500-colibri: Add device tree node for touchscreen support
  touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50
  input: Add DT binding documentation for Colibri VF50 touchscreen

 .../bindings/input/touchscreen/colibri-vf50-ts.txt |  34 ++
 arch/arm/boot/dts/vf500-colibri-eval-v3.dts        |   4 +
 arch/arm/boot/dts/vf500-colibri.dtsi               |  46 +++
 arch/arm/boot/dts/vfxxx.dtsi                       |   2 +
 drivers/input/touchscreen/Kconfig                  |  12 +
 drivers/input/touchscreen/Makefile                 |   1 +
 drivers/input/touchscreen/colibri-vf50-ts.c        | 451 +++++++++++++++++++++
 7 files changed, 550 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt
 create mode 100644 drivers/input/touchscreen/colibri-vf50-ts.c

-- 
2.4.6

[PATCH v2 1/4] ARM: dts: vfxxx: Add io-channel-cells property for ADC node

[Sanchayan Maity]

This commit adds io-channel-cells property to the ADC node. This
property is required in order for an IIO consumer driver to work.
Especially required for Colibri VF50, as the touchscreen driver
uses ADC channels with the ADC driver based on IIO framework.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 arch/arm/boot/dts/vfxxx.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/vfxxx.dtsi b/arch/arm/boot/dts/vfxxx.dtsi
index 2c20f3f..9f63bf0 100644
--- a/arch/arm/boot/dts/vfxxx.dtsi
+++ b/arch/arm/boot/dts/vfxxx.dtsi
@@ -226,6 +226,7 @@
 				interrupts = <53 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks VF610_CLK_ADC0>;
 				clock-names = "adc";
+				#io-channel-cells = <1>;
 				status = "disabled";
 			};
 
@@ -460,6 +461,7 @@
 				interrupts = <54 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks VF610_CLK_ADC1>;
 				clock-names = "adc";
+				#io-channel-cells = <1>;
 				status = "disabled";
 			};
 
-- 
2.4.6

Re: [PATCH v2 1/4] ARM: dts: vfxxx: Add io-channel-cells property for ADC node

[Shawn Guo]

On Thu, Jul 16, 2015 at 08:43:19PM +0530, Sanchayan Maity wrote:
> This commit adds io-channel-cells property to the ADC node. This
> property is required in order for an IIO consumer driver to work.
> Especially required for Colibri VF50, as the touchscreen driver
> uses ADC channels with the ADC driver based on IIO framework.
> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>

Applied, thanks.

[PATCH v2 2/4] ARM: dts: vf500-colibri: Add device tree node for touchscreen support

[Sanchayan Maity]

Add device tree node for touchscreen support on Colibri VF50. The
touchscreen functionality on VF50 uses the ADC channels of Vybrid
and some GPIOs. Also add pinctrl nodes for proper pinmux.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 arch/arm/boot/dts/vf500-colibri-eval-v3.dts |  4 +++
 arch/arm/boot/dts/vf500-colibri.dtsi        | 46 +++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/arch/arm/boot/dts/vf500-colibri-eval-v3.dts b/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
index 7fc782c..c5efb57 100644
--- a/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
+++ b/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
@@ -15,3 +15,7 @@
 	model = "Toradex Colibri VF50 on Colibri Evaluation Board";
 	compatible = "toradex,vf500-colibri_vf50-on-eval", "toradex,vf500-colibri_vf50", "fsl,vf500";
 };
+
+&touchctrl {
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/vf500-colibri.dtsi b/arch/arm/boot/dts/vf500-colibri.dtsi
index cee34a3..7cbe341 100644
--- a/arch/arm/boot/dts/vf500-colibri.dtsi
+++ b/arch/arm/boot/dts/vf500-colibri.dtsi
@@ -17,4 +17,50 @@
 	memory {
 		reg = <0x80000000 0x8000000>;
 	};
+
+	touchctrl: vf50_touchctrl {
+		compatible = "toradex,vf50-touchctrl";
+		io-channels = <&adc1 0>,<&adc0 0>,
+				<&adc0 1>,<&adc1 2>;
+		xp-gpios = <&gpio0 13 GPIO_ACTIVE_LOW>;
+		xm-gpios = <&gpio2 29 GPIO_ACTIVE_HIGH>;
+		yp-gpios = <&gpio0 12 GPIO_ACTIVE_LOW>;
+		ym-gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>;
+		pen-detect-gpios = <&gpio0 8 GPIO_ACTIVE_HIGH>;
+		pen-pullup-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "idle","default","gpios";
+		pinctrl-0 = <&pinctrl_touchctrl_idle>;
+		pinctrl-1 = <&pinctrl_touchctrl_default>;
+		pinctrl-2 = <&pinctrl_touchctrl_gpios>;
+		status = "disabled";
+	};
+};
+
+&iomuxc {
+	vf610-colibri {
+		pinctrl_touchctrl_idle: touchctrl_idle {
+			fsl,pins = <
+				VF610_PAD_PTA18__GPIO_8		0x206d
+				VF610_PAD_PTA19__GPIO_9		0x206d
+				>;
+		};
+
+		pinctrl_touchctrl_default: touchctrl_default {
+			fsl,pins = <
+				VF610_PAD_PTA18__ADC0_SE0	0x2060
+				VF610_PAD_PTA19__ADC0_SE1	0x2060
+				VF610_PAD_PTA16__ADC1_SE0	0x2060
+				VF610_PAD_PTB2__ADC1_SE2	0x2060
+				>;
+		};
+
+		pinctrl_touchctrl_gpios: touchctrl_gpios {
+			fsl,pins = <
+				VF610_PAD_PTA23__GPIO_13	0x22e9
+				VF610_PAD_PTB23__GPIO_93	0x22e9
+				VF610_PAD_PTA22__GPIO_12	0x22e9
+				VF610_PAD_PTA11__GPIO_4		0x22e9
+				>;
+		};
+	};
 };
-- 
2.4.6

Re: [PATCH v2 2/4] ARM: dts: vf500-colibri: Add device tree node for touchscreen support

[Shawn Guo]

On Thu, Jul 16, 2015 at 08:43:20PM +0530, Sanchayan Maity wrote:
> Add device tree node for touchscreen support on Colibri VF50. The
> touchscreen functionality on VF50 uses the ADC channels of Vybrid
> and some GPIOs. Also add pinctrl nodes for proper pinmux.
> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
>  arch/arm/boot/dts/vf500-colibri-eval-v3.dts |  4 +++
>  arch/arm/boot/dts/vf500-colibri.dtsi        | 46 +++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/vf500-colibri-eval-v3.dts b/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
> index 7fc782c..c5efb57 100644
> --- a/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
> +++ b/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
> @@ -15,3 +15,7 @@
>  	model = "Toradex Colibri VF50 on Colibri Evaluation Board";
>  	compatible = "toradex,vf500-colibri_vf50-on-eval", "toradex,vf500-colibri_vf50", "fsl,vf500";
>  };
> +
> +&touchctrl {
> +	status = "okay";
> +};
> diff --git a/arch/arm/boot/dts/vf500-colibri.dtsi b/arch/arm/boot/dts/vf500-colibri.dtsi
> index cee34a3..7cbe341 100644
> --- a/arch/arm/boot/dts/vf500-colibri.dtsi
> +++ b/arch/arm/boot/dts/vf500-colibri.dtsi
> @@ -17,4 +17,50 @@
>  	memory {
>  		reg = <0x80000000 0x8000000>;
>  	};
> +
> +	touchctrl: vf50_touchctrl {

Minus rather than underscore should be used in node name.  And node name
should be generic.  "touchscreen" might be a good choice.

Shawn

> +		compatible = "toradex,vf50-touchctrl";
> +		io-channels = <&adc1 0>,<&adc0 0>,
> +				<&adc0 1>,<&adc1 2>;
> +		xp-gpios = <&gpio0 13 GPIO_ACTIVE_LOW>;
> +		xm-gpios = <&gpio2 29 GPIO_ACTIVE_HIGH>;
> +		yp-gpios = <&gpio0 12 GPIO_ACTIVE_LOW>;
> +		ym-gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>;
> +		pen-detect-gpios = <&gpio0 8 GPIO_ACTIVE_HIGH>;
> +		pen-pullup-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "idle","default","gpios";
> +		pinctrl-0 = <&pinctrl_touchctrl_idle>;
> +		pinctrl-1 = <&pinctrl_touchctrl_default>;
> +		pinctrl-2 = <&pinctrl_touchctrl_gpios>;
> +		status = "disabled";
> +	};
> +};
> +
> +&iomuxc {
> +	vf610-colibri {
> +		pinctrl_touchctrl_idle: touchctrl_idle {
> +			fsl,pins = <
> +				VF610_PAD_PTA18__GPIO_8		0x206d
> +				VF610_PAD_PTA19__GPIO_9		0x206d
> +				>;
> +		};
> +
> +		pinctrl_touchctrl_default: touchctrl_default {
> +			fsl,pins = <
> +				VF610_PAD_PTA18__ADC0_SE0	0x2060
> +				VF610_PAD_PTA19__ADC0_SE1	0x2060
> +				VF610_PAD_PTA16__ADC1_SE0	0x2060
> +				VF610_PAD_PTB2__ADC1_SE2	0x2060
> +				>;
> +		};
> +
> +		pinctrl_touchctrl_gpios: touchctrl_gpios {
> +			fsl,pins = <
> +				VF610_PAD_PTA23__GPIO_13	0x22e9
> +				VF610_PAD_PTB23__GPIO_93	0x22e9
> +				VF610_PAD_PTA22__GPIO_12	0x22e9
> +				VF610_PAD_PTA11__GPIO_4		0x22e9
> +				>;
> +		};
> +	};
>  };
> -- 
> 2.4.6
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH v2 3/4] touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

[Sanchayan Maity]

The Colibri Vybrid VF50 module supports 4-wire touchscreens using
FETs and ADC inputs. This driver uses the IIO consumer interface
and relies on the vf610_adc driver based on the IIO framework.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 drivers/input/touchscreen/Kconfig           |  12 +
 drivers/input/touchscreen/Makefile          |   1 +
 drivers/input/touchscreen/colibri-vf50-ts.c | 451 ++++++++++++++++++++++++++++
 3 files changed, 464 insertions(+)
 create mode 100644 drivers/input/touchscreen/colibri-vf50-ts.c

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 80f6386..28948ca 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -1027,4 +1027,16 @@ config TOUCHSCREEN_ZFORCE
 	  To compile this driver as a module, choose M here: the
 	  module will be called zforce_ts.
 
+config TOUCHSCREEN_COLIBRI_VF50
+	tristate "Toradex Colibri on board touchscreen driver"
+	depends on GPIOLIB && IIO && VF610_ADC
+	help
+	  Say Y here if you have a Colibri VF50 and plan to use
+	  the on-board provided 4-wire touchscreen driver.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called colibri_vf50_ts.
+
 endif
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 44deea7..93746a0 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -84,3 +84,4 @@ obj-$(CONFIG_TOUCHSCREEN_W90X900)	+= w90p910_ts.o
 obj-$(CONFIG_TOUCHSCREEN_SX8654)	+= sx8654.o
 obj-$(CONFIG_TOUCHSCREEN_TPS6507X)	+= tps6507x-ts.o
 obj-$(CONFIG_TOUCHSCREEN_ZFORCE)	+= zforce_ts.o
+obj-$(CONFIG_TOUCHSCREEN_COLIBRI_VF50)	+= colibri-vf50-ts.o
diff --git a/drivers/input/touchscreen/colibri-vf50-ts.c b/drivers/input/touchscreen/colibri-vf50-ts.c
new file mode 100644
index 0000000..eb16bdc
--- /dev/null
+++ b/drivers/input/touchscreen/colibri-vf50-ts.c
@@ -0,0 +1,451 @@
+/* Copyright 2015 Toradex AG
+ *
+ * Toradex Colibri VF50 Touchscreen driver
+ *
+ * Originally authored by Stefan Agner for 3.0 kernel
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <dt-bindings/gpio/gpio.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/iio/consumer.h>
+#include <linux/iio/types.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#define DRIVER_NAME "colibri-vf50-ts"
+#define DRV_VERSION "1.0"
+
+#define VF_ADC_MAX ((1 << 12) - 1)
+
+#define COLI_TOUCH_MIN_DELAY_US 1000
+#define COLI_TOUCH_MAX_DELAY_US 2000
+
+static int min_pressure = 200;
+
+struct vf50_touch_device {
+	struct platform_device	*pdev;
+	struct input_dev	*ts_input;
+	struct workqueue_struct *ts_workqueue;
+	struct work_struct	ts_work;
+	struct iio_channel	*channels;
+	struct gpio_desc *gpio_xp;
+	struct gpio_desc *gpio_xm;
+	struct gpio_desc *gpio_yp;
+	struct gpio_desc *gpio_ym;
+	struct gpio_desc *gpio_pen_detect;
+	struct gpio_desc *gpio_pen_detect_pullup;
+	int pen_irq;
+	bool stop_touchscreen;
+};
+
+/*
+ * Enables given plates and measures touch parameters using ADC
+ */
+static int adc_ts_measure(struct iio_channel *channel,
+		  struct gpio_desc *plate_p, struct gpio_desc *plate_m)
+{
+	int i, value = 0, val = 0;
+	int ret;
+
+	gpiod_set_value(plate_p, 1);
+	gpiod_set_value(plate_m, 1);
+
+	usleep_range(COLI_TOUCH_MIN_DELAY_US, COLI_TOUCH_MAX_DELAY_US);
+
+	for (i = 0; i < 5; i++) {
+		ret = iio_read_channel_raw(channel, &val);
+		if (ret < 0)
+			return -EINVAL;
+
+		value += val;
+	}
+
+	value /= 5;
+
+	gpiod_set_value(plate_p, 0);
+	gpiod_set_value(plate_m, 0);
+
+	return value;
+}
+
+/*
+ * Enable touch detection using falling edge detection on XM
+ */
+static void vf50_ts_enable_touch_detection(struct vf50_touch_device *vf50_ts)
+{
+	/* Enable plate YM (needs to be strong GND, high active) */
+	gpiod_set_value(vf50_ts->gpio_ym, 1);
+
+	/*
+	 * Let the platform mux to idle state in order to enable
+	 * Pull-Up on GPIO
+	 */
+	pinctrl_pm_select_idle_state(&vf50_ts->pdev->dev);
+}
+
+/*
+ * ADC touch screen sampling worker function
+ */
+static void vf50_ts_work(struct work_struct *ts_work)
+{
+	struct vf50_touch_device *vf50_ts = container_of(ts_work,
+				struct vf50_touch_device, ts_work);
+	struct device *dev = &vf50_ts->pdev->dev;
+	int val_x, val_y, val_z1, val_z2, val_p = 0;
+	bool discard_val_on_start = true;
+
+	while (!vf50_ts->stop_touchscreen) {
+		/* X-Direction */
+		val_x = adc_ts_measure(&vf50_ts->channels[0],
+				vf50_ts->gpio_xp, vf50_ts->gpio_xm);
+		if (val_x < 0)
+			continue;
+
+		/* Y-Direction */
+		val_y = adc_ts_measure(&vf50_ts->channels[1],
+				vf50_ts->gpio_yp, vf50_ts->gpio_ym);
+		if (val_y < 0)
+			continue;
+
+		/*
+		 * Touch pressure
+		 * Measure on XP/YM
+		 */
+		val_z1 = adc_ts_measure(&vf50_ts->channels[2],
+				vf50_ts->gpio_yp, vf50_ts->gpio_xm);
+		if (val_z1 < 0)
+			continue;
+		val_z2 = adc_ts_measure(&vf50_ts->channels[3],
+				vf50_ts->gpio_yp, vf50_ts->gpio_xm);
+		if (val_z2 < 0)
+			continue;
+
+		/* Validate signal (avoid calculation using noise) */
+		if (val_z1 > 64 && val_x > 64) {
+			/*
+			 * Calculate resistance between the plates
+			 * lower resistance means higher pressure
+			 */
+			int r_x = (1000 * val_x) / VF_ADC_MAX;
+
+			val_p = (r_x * val_z2) / val_z1 - r_x;
+
+		} else {
+			val_p = 2000;
+		}
+
+		val_p = 2000 - val_p;
+		dev_dbg(dev, "Measured values: x: %d, y: %d, z1: %d, z2: %d, "
+			"p: %d\n", val_x, val_y, val_z1, val_z2, val_p);
+
+		/*
+		 * If touch pressure is too low, stop measuring and reenable
+		 * touch detection
+		 */
+		if (val_p < min_pressure || val_p > 2000)
+			break;
+
+		/*
+		 * The pressure may not be enough for the first x and the
+		 * second y measurement, but, the pressure is ok when the
+		 * driver is doing the third and fourth measurement. To
+		 * take care of this, we drop the first measurement always.
+		 */
+		if (discard_val_on_start) {
+			discard_val_on_start = false;
+		} else {
+			/*
+			 * Report touch position and sleep for
+			 * next measurement
+			 */
+			input_report_abs(vf50_ts->ts_input,
+					ABS_X, VF_ADC_MAX - val_x);
+			input_report_abs(vf50_ts->ts_input,
+					ABS_Y, VF_ADC_MAX - val_y);
+			input_report_abs(vf50_ts->ts_input,
+					ABS_PRESSURE, val_p);
+			input_report_key(vf50_ts->ts_input, BTN_TOUCH, 1);
+			input_sync(vf50_ts->ts_input);
+		}
+
+		msleep(10);
+	}
+
+	/* Report no more touch, reenable touch detection */
+	input_report_abs(vf50_ts->ts_input, ABS_PRESSURE, 0);
+	input_report_key(vf50_ts->ts_input, BTN_TOUCH, 0);
+	input_sync(vf50_ts->ts_input);
+
+	vf50_ts_enable_touch_detection(vf50_ts);
+
+	/* Wait for the pull-up to be stable on high */
+	msleep(10);
+
+	/* Reenable IRQ to detect touch */
+	enable_irq(vf50_ts->pen_irq);
+
+	dev_dbg(dev, "Reenabled touch detection interrupt\n");
+}
+
+static irqreturn_t vf50_ts_touched(int irq, void *dev_id)
+{
+	struct vf50_touch_device *vf50_ts = (struct vf50_touch_device *)dev_id;
+	struct device *dev = &vf50_ts->pdev->dev;
+
+	dev_dbg(dev, "Touch detected, start worker thread\n");
+
+	disable_irq_nosync(irq);
+
+	/* Disable the touch detection plates */
+	gpiod_set_value(vf50_ts->gpio_ym, 0);
+
+	/* Let the platform mux to default state in order to mux as ADC */
+	pinctrl_pm_select_default_state(dev);
+
+	queue_work(vf50_ts->ts_workqueue, &vf50_ts->ts_work);
+
+	return IRQ_HANDLED;
+}
+
+static int vf50_ts_open(struct input_dev *dev_input)
+{
+	int ret;
+	struct vf50_touch_device *touchdev = input_get_drvdata(dev_input);
+	struct device *dev = &touchdev->pdev->dev;
+
+	dev_dbg(dev, "Input device %s opened, starting touch detection\n",
+			dev_input->name);
+
+	touchdev->stop_touchscreen = false;
+
+	ret = gpiod_direction_output(touchdev->gpio_xp, 0);
+	if (ret) {
+		dev_err(dev, "Could not set gpio xp as output %d\n", ret);
+		return ret;
+	}
+
+	ret = gpiod_direction_output(touchdev->gpio_xm, 0);
+	if (ret) {
+		dev_err(dev, "Could not set gpio xm as output %d\n", ret);
+		return ret;
+	}
+
+	ret = gpiod_direction_output(touchdev->gpio_yp, 0);
+	if (ret) {
+		dev_err(dev, "Could not set gpio yp as output %d\n", ret);
+		return ret;
+	}
+
+	ret = gpiod_direction_output(touchdev->gpio_ym, 0);
+	if (ret) {
+		dev_err(dev, "Could not set gpio ym as output %d\n", ret);
+		return ret;
+	}
+
+	ret = gpiod_direction_input(touchdev->gpio_pen_detect);
+	if (ret) {
+		dev_err(dev,
+			"Could not set gpio pen detect as input %d\n", ret);
+		return ret;
+	}
+
+	ret = gpiod_direction_input(touchdev->gpio_pen_detect_pullup);
+	if (ret) {
+		dev_err(dev,
+			"Could not set pen detect pullup as input %d\n", ret);
+		return ret;
+	}
+
+	/* Mux detection before request IRQ, wait for pull-up to settle */
+	vf50_ts_enable_touch_detection(touchdev);
+	msleep(10);
+
+	touchdev->pen_irq = gpiod_to_irq(touchdev->gpio_pen_detect);
+	if (touchdev->pen_irq < 0) {
+		dev_err(dev, "Unable to get IRQ for GPIO\n");
+		return touchdev->pen_irq;
+	}
+
+	ret = request_irq(touchdev->pen_irq, vf50_ts_touched,
+			IRQF_TRIGGER_FALLING, "touch detected", touchdev);
+	if (ret < 0) {
+		dev_err(dev, "Unable to request IRQ %d\n", touchdev->pen_irq);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void vf50_ts_close(struct input_dev *dev_input)
+{
+	struct vf50_touch_device *touchdev = input_get_drvdata(dev_input);
+	struct device *dev = &touchdev->pdev->dev;
+
+	free_irq(touchdev->pen_irq, touchdev);
+
+	touchdev->stop_touchscreen = true;
+
+	/* Wait until touchscreen thread finishes any possible remnants. */
+	cancel_work_sync(&touchdev->ts_work);
+
+	dev_dbg(dev, "Input device %s closed, disable touch detection\n",
+		dev_input->name);
+}
+
+static inline int vf50_ts_get_gpiod(struct device *dev,
+			struct gpio_desc **gpio_d, const char *con_id)
+{
+	int ret;
+
+	*gpio_d = devm_gpiod_get(dev, con_id);
+	if (IS_ERR(*gpio_d)) {
+		ret = PTR_ERR(*gpio_d);
+		dev_err(dev, "Could not get gpio_%s %d\n", con_id, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int vf50_ts_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	struct vf50_touch_device *touchdev;
+	struct input_dev *input;
+	int ret = 0;
+
+	if (!node) {
+		dev_err(dev, "Device does not have associated DT data\n");
+		return -EINVAL;
+	}
+
+	touchdev = devm_kzalloc(dev, sizeof(*touchdev), GFP_KERNEL);
+	if (touchdev == NULL)
+		return -ENOMEM;
+
+	input = devm_input_allocate_device(dev);
+	if (!input) {
+		dev_err(dev, "Failed to allocate TS input device\n");
+		ret = -ENOMEM;
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, touchdev);
+
+	touchdev->pdev = pdev;
+
+	input->name = DRIVER_NAME;
+	input->id.bustype = BUS_HOST;
+	input->dev.parent = dev;
+	input->open = vf50_ts_open;
+	input->close = vf50_ts_close;
+
+	_set_bit(EV_ABS, input->evbit);
+	_set_bit(EV_KEY, input->evbit);
+	_set_bit(BTN_TOUCH, input->keybit);
+	input_set_abs_params(input, ABS_X, 0, VF_ADC_MAX, 0, 0);
+	input_set_abs_params(input, ABS_Y, 0, VF_ADC_MAX, 0, 0);
+	input_set_abs_params(input, ABS_PRESSURE, 0, VF_ADC_MAX, 0, 0);
+
+	touchdev->ts_input = input;
+	input_set_drvdata(input, touchdev);
+	ret = input_register_device(input);
+	if (ret) {
+		dev_err(dev, "Failed to register input device\n");
+		return ret;
+	}
+
+	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_xp, "xp");
+	if (ret)
+		return ret;
+
+	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_xm, "xm");
+	if (ret)
+		return ret;
+
+	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_yp, "yp");
+	if (ret)
+		return ret;
+
+	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_ym, "ym");
+	if (ret)
+		return ret;
+
+	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_pen_detect,
+				"pen-detect");
+	if (ret)
+		return ret;
+
+	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_pen_detect_pullup,
+				"pen-pullup");
+	if (ret)
+		return ret;
+
+	INIT_WORK(&touchdev->ts_work, vf50_ts_work);
+	touchdev->ts_workqueue = create_singlethread_workqueue("vf50-ts-touch");
+
+	if (!touchdev->ts_workqueue) {
+		ret = PTR_ERR(touchdev->ts_workqueue);
+		dev_err(dev,
+			"Failed creating vf50-ts-touch workqueue %d\n", ret);
+		return ret;
+	}
+
+	touchdev->channels = iio_channel_get_all(dev);
+	if (IS_ERR(touchdev->channels))
+		return PTR_ERR(touchdev->channels);
+
+	dev_info(dev, "Attached colibri-vf50-ts driver successfully\n");
+
+	return 0;
+}
+
+static int vf50_ts_remove(struct platform_device *pdev)
+{
+	struct vf50_touch_device *touchdev = platform_get_drvdata(pdev);
+
+	destroy_workqueue(touchdev->ts_workqueue);
+	iio_channel_release_all(touchdev->channels);
+
+	return 0;
+}
+
+static const struct of_device_id vf50_touch_of_match[] = {
+	{ .compatible = "toradex,vf50-touchctrl", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, vf50_touch_of_match);
+
+static struct platform_driver __refdata vf50_touch_driver = {
+	.driver = {
+		.name = "toradex,vf50_touchctrl",
+		.of_match_table = vf50_touch_of_match,
+	},
+	.probe = vf50_ts_probe,
+	.remove = vf50_ts_remove,
+	.prevent_deferred_probe = false,
+};
+
+module_platform_driver(vf50_touch_driver);
+
+module_param(min_pressure, int, 0600);
+MODULE_PARM_DESC(min_pressure, "Minimum pressure for touch detection");
+MODULE_AUTHOR("Sanchayan Maity");
+MODULE_DESCRIPTION("Colibri VF50 Touchscreen driver");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION(DRV_VERSION);
-- 
2.4.6

Re: [PATCH v2 3/4] touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

[Dmitry Torokhov]

Hi Sanchayan,


On Thu, Jul 16, 2015 at 08:43:21PM +0530, Sanchayan Maity wrote:
> The Colibri Vybrid VF50 module supports 4-wire touchscreens using
> FETs and ADC inputs. This driver uses the IIO consumer interface
> and relies on the vf610_adc driver based on the IIO framework.

This looks pretty good, thank you. Just a few comments below.

> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
>  drivers/input/touchscreen/Kconfig           |  12 +
>  drivers/input/touchscreen/Makefile          |   1 +
>  drivers/input/touchscreen/colibri-vf50-ts.c | 451 ++++++++++++++++++++++++++++
>  3 files changed, 464 insertions(+)
>  create mode 100644 drivers/input/touchscreen/colibri-vf50-ts.c
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 80f6386..28948ca 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1027,4 +1027,16 @@ config TOUCHSCREEN_ZFORCE
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called zforce_ts.
>  
> +config TOUCHSCREEN_COLIBRI_VF50
> +	tristate "Toradex Colibri on board touchscreen driver"
> +	depends on GPIOLIB && IIO && VF610_ADC
> +	help
> +	  Say Y here if you have a Colibri VF50 and plan to use
> +	  the on-board provided 4-wire touchscreen driver.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called colibri_vf50_ts.
> +
>  endif
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 44deea7..93746a0 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -84,3 +84,4 @@ obj-$(CONFIG_TOUCHSCREEN_W90X900)	+= w90p910_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_SX8654)	+= sx8654.o
>  obj-$(CONFIG_TOUCHSCREEN_TPS6507X)	+= tps6507x-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_ZFORCE)	+= zforce_ts.o
> +obj-$(CONFIG_TOUCHSCREEN_COLIBRI_VF50)	+= colibri-vf50-ts.o
> diff --git a/drivers/input/touchscreen/colibri-vf50-ts.c b/drivers/input/touchscreen/colibri-vf50-ts.c
> new file mode 100644
> index 0000000..eb16bdc
> --- /dev/null
> +++ b/drivers/input/touchscreen/colibri-vf50-ts.c
> @@ -0,0 +1,451 @@
> +/* Copyright 2015 Toradex AG
> + *
> + * Toradex Colibri VF50 Touchscreen driver
> + *
> + * Originally authored by Stefan Agner for 3.0 kernel
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <dt-bindings/gpio/gpio.h>

Why?

> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/types.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_gpio.h>

Is not used as far as I can see.

> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#define DRIVER_NAME "colibri-vf50-ts"
> +#define DRV_VERSION "1.0"
> +
> +#define VF_ADC_MAX ((1 << 12) - 1)
> +
> +#define COLI_TOUCH_MIN_DELAY_US 1000
> +#define COLI_TOUCH_MAX_DELAY_US 2000
> +
> +static int min_pressure = 200;
> +
> +struct vf50_touch_device {
> +	struct platform_device	*pdev;
> +	struct input_dev	*ts_input;
> +	struct workqueue_struct *ts_workqueue;
> +	struct work_struct	ts_work;
> +	struct iio_channel	*channels;
> +	struct gpio_desc *gpio_xp;
> +	struct gpio_desc *gpio_xm;
> +	struct gpio_desc *gpio_yp;
> +	struct gpio_desc *gpio_ym;
> +	struct gpio_desc *gpio_pen_detect;
> +	struct gpio_desc *gpio_pen_detect_pullup;
> +	int pen_irq;
> +	bool stop_touchscreen;
> +};
> +
> +/*
> + * Enables given plates and measures touch parameters using ADC
> + */
> +static int adc_ts_measure(struct iio_channel *channel,
> +		  struct gpio_desc *plate_p, struct gpio_desc *plate_m)
> +{
> +	int i, value = 0, val = 0;
> +	int ret;
> +
> +	gpiod_set_value(plate_p, 1);
> +	gpiod_set_value(plate_m, 1);
> +
> +	usleep_range(COLI_TOUCH_MIN_DELAY_US, COLI_TOUCH_MAX_DELAY_US);
> +
> +	for (i = 0; i < 5; i++) {
> +		ret = iio_read_channel_raw(channel, &val);
> +		if (ret < 0)
> +			return -EINVAL;
> +
> +		value += val;
> +	}
> +
> +	value /= 5;
> +
> +	gpiod_set_value(plate_p, 0);
> +	gpiod_set_value(plate_m, 0);
> +
> +	return value;
> +}
> +
> +/*
> + * Enable touch detection using falling edge detection on XM
> + */
> +static void vf50_ts_enable_touch_detection(struct vf50_touch_device *vf50_ts)
> +{
> +	/* Enable plate YM (needs to be strong GND, high active) */
> +	gpiod_set_value(vf50_ts->gpio_ym, 1);
> +
> +	/*
> +	 * Let the platform mux to idle state in order to enable
> +	 * Pull-Up on GPIO
> +	 */
> +	pinctrl_pm_select_idle_state(&vf50_ts->pdev->dev);
> +}
> +
> +/*
> + * ADC touch screen sampling worker function
> + */
> +static void vf50_ts_work(struct work_struct *ts_work)
> +{
> +	struct vf50_touch_device *vf50_ts = container_of(ts_work,
> +				struct vf50_touch_device, ts_work);
> +	struct device *dev = &vf50_ts->pdev->dev;
> +	int val_x, val_y, val_z1, val_z2, val_p = 0;
> +	bool discard_val_on_start = true;
> +
> +	while (!vf50_ts->stop_touchscreen) {
> +		/* X-Direction */
> +		val_x = adc_ts_measure(&vf50_ts->channels[0],
> +				vf50_ts->gpio_xp, vf50_ts->gpio_xm);
> +		if (val_x < 0)
> +			continue;
> +
> +		/* Y-Direction */
> +		val_y = adc_ts_measure(&vf50_ts->channels[1],
> +				vf50_ts->gpio_yp, vf50_ts->gpio_ym);
> +		if (val_y < 0)
> +			continue;
> +
> +		/*
> +		 * Touch pressure
> +		 * Measure on XP/YM
> +		 */
> +		val_z1 = adc_ts_measure(&vf50_ts->channels[2],
> +				vf50_ts->gpio_yp, vf50_ts->gpio_xm);
> +		if (val_z1 < 0)
> +			continue;
> +		val_z2 = adc_ts_measure(&vf50_ts->channels[3],
> +				vf50_ts->gpio_yp, vf50_ts->gpio_xm);
> +		if (val_z2 < 0)
> +			continue;
> +
> +		/* Validate signal (avoid calculation using noise) */
> +		if (val_z1 > 64 && val_x > 64) {
> +			/*
> +			 * Calculate resistance between the plates
> +			 * lower resistance means higher pressure
> +			 */
> +			int r_x = (1000 * val_x) / VF_ADC_MAX;
> +
> +			val_p = (r_x * val_z2) / val_z1 - r_x;
> +
> +		} else {
> +			val_p = 2000;
> +		}
> +
> +		val_p = 2000 - val_p;
> +		dev_dbg(dev, "Measured values: x: %d, y: %d, z1: %d, z2: %d, "
> +			"p: %d\n", val_x, val_y, val_z1, val_z2, val_p);
> +
> +		/*
> +		 * If touch pressure is too low, stop measuring and reenable
> +		 * touch detection
> +		 */
> +		if (val_p < min_pressure || val_p > 2000)
> +			break;
> +
> +		/*
> +		 * The pressure may not be enough for the first x and the
> +		 * second y measurement, but, the pressure is ok when the
> +		 * driver is doing the third and fourth measurement. To
> +		 * take care of this, we drop the first measurement always.
> +		 */
> +		if (discard_val_on_start) {
> +			discard_val_on_start = false;
> +		} else {
> +			/*
> +			 * Report touch position and sleep for
> +			 * next measurement
> +			 */
> +			input_report_abs(vf50_ts->ts_input,
> +					ABS_X, VF_ADC_MAX - val_x);
> +			input_report_abs(vf50_ts->ts_input,
> +					ABS_Y, VF_ADC_MAX - val_y);
> +			input_report_abs(vf50_ts->ts_input,
> +					ABS_PRESSURE, val_p);
> +			input_report_key(vf50_ts->ts_input, BTN_TOUCH, 1);
> +			input_sync(vf50_ts->ts_input);
> +		}
> +
> +		msleep(10);
> +	}
> +
> +	/* Report no more touch, reenable touch detection */
> +	input_report_abs(vf50_ts->ts_input, ABS_PRESSURE, 0);
> +	input_report_key(vf50_ts->ts_input, BTN_TOUCH, 0);
> +	input_sync(vf50_ts->ts_input);
> +
> +	vf50_ts_enable_touch_detection(vf50_ts);
> +
> +	/* Wait for the pull-up to be stable on high */
> +	msleep(10);
> +
> +	/* Reenable IRQ to detect touch */
> +	enable_irq(vf50_ts->pen_irq);
> +
> +	dev_dbg(dev, "Reenabled touch detection interrupt\n");
> +}
> +
> +static irqreturn_t vf50_ts_touched(int irq, void *dev_id)
> +{
> +	struct vf50_touch_device *vf50_ts = (struct vf50_touch_device *)dev_id;
> +	struct device *dev = &vf50_ts->pdev->dev;
> +
> +	dev_dbg(dev, "Touch detected, start worker thread\n");
> +
> +	disable_irq_nosync(irq);
> +
> +	/* Disable the touch detection plates */
> +	gpiod_set_value(vf50_ts->gpio_ym, 0);
> +
> +	/* Let the platform mux to default state in order to mux as ADC */
> +	pinctrl_pm_select_default_state(dev);
> +
> +	queue_work(vf50_ts->ts_workqueue, &vf50_ts->ts_work);

If you convert this to a threaded interrupt you won't need to
disable/reenable interrupt or queue work. You should also be able to use
gpiod_set_value_cansleep() extending the range of ways the controller
could be connected to systems.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int vf50_ts_open(struct input_dev *dev_input)
> +{
> +	int ret;
> +	struct vf50_touch_device *touchdev = input_get_drvdata(dev_input);
> +	struct device *dev = &touchdev->pdev->dev;
> +
> +	dev_dbg(dev, "Input device %s opened, starting touch detection\n",
> +			dev_input->name);
> +
> +	touchdev->stop_touchscreen = false;
> +
> +	ret = gpiod_direction_output(touchdev->gpio_xp, 0);
> +	if (ret) {
> +		dev_err(dev, "Could not set gpio xp as output %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = gpiod_direction_output(touchdev->gpio_xm, 0);
> +	if (ret) {
> +		dev_err(dev, "Could not set gpio xm as output %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = gpiod_direction_output(touchdev->gpio_yp, 0);
> +	if (ret) {
> +		dev_err(dev, "Could not set gpio yp as output %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = gpiod_direction_output(touchdev->gpio_ym, 0);
> +	if (ret) {
> +		dev_err(dev, "Could not set gpio ym as output %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = gpiod_direction_input(touchdev->gpio_pen_detect);
> +	if (ret) {
> +		dev_err(dev,
> +			"Could not set gpio pen detect as input %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = gpiod_direction_input(touchdev->gpio_pen_detect_pullup);
> +	if (ret) {
> +		dev_err(dev,
> +			"Could not set pen detect pullup as input %d\n", ret);
> +		return ret;
> +	}

I do not see this GPIO being actually used...

> +
> +	/* Mux detection before request IRQ, wait for pull-up to settle */
> +	vf50_ts_enable_touch_detection(touchdev);
> +	msleep(10);
> +
> +	touchdev->pen_irq = gpiod_to_irq(touchdev->gpio_pen_detect);
> +	if (touchdev->pen_irq < 0) {
> +		dev_err(dev, "Unable to get IRQ for GPIO\n");
> +		return touchdev->pen_irq;
> +	}
> +
> +	ret = request_irq(touchdev->pen_irq, vf50_ts_touched,
> +			IRQF_TRIGGER_FALLING, "touch detected", touchdev);
> +	if (ret < 0) {
> +		dev_err(dev, "Unable to request IRQ %d\n", touchdev->pen_irq);
> +		return ret;
> +	}

I'd rather we did most initialization in probe() so that -EPROBE_DEFER
would get handled properly.

> +
> +	return 0;
> +}
> +
> +static void vf50_ts_close(struct input_dev *dev_input)
> +{
> +	struct vf50_touch_device *touchdev = input_get_drvdata(dev_input);
> +	struct device *dev = &touchdev->pdev->dev;
> +
> +	free_irq(touchdev->pen_irq, touchdev);
> +
> +	touchdev->stop_touchscreen = true;
> +
> +	/* Wait until touchscreen thread finishes any possible remnants. */
> +	cancel_work_sync(&touchdev->ts_work);
> +
> +	dev_dbg(dev, "Input device %s closed, disable touch detection\n",
> +		dev_input->name);
> +}
> +
> +static inline int vf50_ts_get_gpiod(struct device *dev,
> +			struct gpio_desc **gpio_d, const char *con_id)
> +{
> +	int ret;
> +
> +	*gpio_d = devm_gpiod_get(dev, con_id);
> +	if (IS_ERR(*gpio_d)) {
> +		ret = PTR_ERR(*gpio_d);
> +		dev_err(dev, "Could not get gpio_%s %d\n", con_id, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int vf50_ts_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct vf50_touch_device *touchdev;
> +	struct input_dev *input;
> +	int ret = 0;

Personal preference: can we call this "error"? Also no need to
gratuitously initialize it to 0.

> +
> +	if (!node) {
> +		dev_err(dev, "Device does not have associated DT data\n");
> +		return -EINVAL;

So what? I do not see you using of_node anywhere.

> +	}
> +
> +	touchdev = devm_kzalloc(dev, sizeof(*touchdev), GFP_KERNEL);
> +	if (touchdev == NULL)
> +		return -ENOMEM;
> +
> +	input = devm_input_allocate_device(dev);
> +	if (!input) {
> +		dev_err(dev, "Failed to allocate TS input device\n");
> +		ret = -ENOMEM;
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, touchdev);
> +
> +	touchdev->pdev = pdev;
> +
> +	input->name = DRIVER_NAME;
> +	input->id.bustype = BUS_HOST;
> +	input->dev.parent = dev;
> +	input->open = vf50_ts_open;
> +	input->close = vf50_ts_close;
> +
> +	_set_bit(EV_ABS, input->evbit);
> +	_set_bit(EV_KEY, input->evbit);
> +	_set_bit(BTN_TOUCH, input->keybit);
> +	input_set_abs_params(input, ABS_X, 0, VF_ADC_MAX, 0, 0);
> +	input_set_abs_params(input, ABS_Y, 0, VF_ADC_MAX, 0, 0);
> +	input_set_abs_params(input, ABS_PRESSURE, 0, VF_ADC_MAX, 0, 0);
> +
> +	touchdev->ts_input = input;
> +	input_set_drvdata(input, touchdev);
> +	ret = input_register_device(input);
> +	if (ret) {
> +		dev_err(dev, "Failed to register input device\n");
> +		return ret;
> +	}
> +
> +	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_xp, "xp");
> +	if (ret)
> +		return ret;
> +
> +	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_xm, "xm");
> +	if (ret)
> +		return ret;
> +
> +	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_yp, "yp");
> +	if (ret)
> +		return ret;
> +
> +	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_ym, "ym");
> +	if (ret)
> +		return ret;
> +
> +	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_pen_detect,
> +				"pen-detect");
> +	if (ret)
> +		return ret;
> +
> +	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_pen_detect_pullup,
> +				"pen-pullup");
> +	if (ret)
> +		return ret;
> +
> +	INIT_WORK(&touchdev->ts_work, vf50_ts_work);
> +	touchdev->ts_workqueue = create_singlethread_workqueue("vf50-ts-touch");
> +
> +	if (!touchdev->ts_workqueue) {
> +		ret = PTR_ERR(touchdev->ts_workqueue);
> +		dev_err(dev,
> +			"Failed creating vf50-ts-touch workqueue %d\n", ret);
> +		return ret;
> +	}

This should go away with the threaded IRQ.

> +
> +	touchdev->channels = iio_channel_get_all(dev);
> +	if (IS_ERR(touchdev->channels))
> +		return PTR_ERR(touchdev->channels);
> +
> +	dev_info(dev, "Attached colibri-vf50-ts driver successfully\n");

Please drop, you should see message from input core about the new device
already.

> +
> +	return 0;
> +}
> +
> +static int vf50_ts_remove(struct platform_device *pdev)
> +{
> +	struct vf50_touch_device *touchdev = platform_get_drvdata(pdev);
> +
> +	destroy_workqueue(touchdev->ts_workqueue);
> +	iio_channel_release_all(touchdev->channels);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id vf50_touch_of_match[] = {
> +	{ .compatible = "toradex,vf50-touchctrl", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, vf50_touch_of_match);
> +
> +static struct platform_driver __refdata vf50_touch_driver = {

Why do you need __refdata?

> +	.driver = {
> +		.name = "toradex,vf50_touchctrl",
> +		.of_match_table = vf50_touch_of_match,
> +	},
> +	.probe = vf50_ts_probe,
> +	.remove = vf50_ts_remove,
> +	.prevent_deferred_probe = false,

Deferrals are enabled by default.

> +};
> +
> +module_platform_driver(vf50_touch_driver);
> +
> +module_param(min_pressure, int, 0600);
> +MODULE_PARM_DESC(min_pressure, "Minimum pressure for touch detection");

I'd rather let userspace figure out what it recognizes as valid touch.

> +MODULE_AUTHOR("Sanchayan Maity");
> +MODULE_DESCRIPTION("Colibri VF50 Touchscreen driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION(DRV_VERSION);

Thanks.

-- 
Dmitry

Re: [PATCH v2 3/4] touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

[maitysanchayan]

Hello Dmitry,

On 15-07-17 16:42:42, Dmitry Torokhov wrote:
> Hi Sanchayan,
> 
> 
> On Thu, Jul 16, 2015 at 08:43:21PM +0530, Sanchayan Maity wrote:
> > The Colibri Vybrid VF50 module supports 4-wire touchscreens using
> > FETs and ADC inputs. This driver uses the IIO consumer interface
> > and relies on the vf610_adc driver based on the IIO framework.
> 
> This looks pretty good, thank you. Just a few comments below.
> 
> > 
> > Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> > ---
> >  drivers/input/touchscreen/Kconfig           |  12 +
> >  drivers/input/touchscreen/Makefile          |   1 +
> >  drivers/input/touchscreen/colibri-vf50-ts.c | 451 ++++++++++++++++++++++++++++
> >  3 files changed, 464 insertions(+)
> >  create mode 100644 drivers/input/touchscreen/colibri-vf50-ts.c
> > 
> > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > index 80f6386..28948ca 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -1027,4 +1027,16 @@ config TOUCHSCREEN_ZFORCE
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called zforce_ts.
> >  
> > +config TOUCHSCREEN_COLIBRI_VF50
> > +	tristate "Toradex Colibri on board touchscreen driver"
> > +	depends on GPIOLIB && IIO && VF610_ADC
> > +	help
> > +	  Say Y here if you have a Colibri VF50 and plan to use
> > +	  the on-board provided 4-wire touchscreen driver.
> > +
> > +	  If unsure, say N.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called colibri_vf50_ts.
> > +
> >  endif
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index 44deea7..93746a0 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -84,3 +84,4 @@ obj-$(CONFIG_TOUCHSCREEN_W90X900)	+= w90p910_ts.o
> >  obj-$(CONFIG_TOUCHSCREEN_SX8654)	+= sx8654.o
> >  obj-$(CONFIG_TOUCHSCREEN_TPS6507X)	+= tps6507x-ts.o
> >  obj-$(CONFIG_TOUCHSCREEN_ZFORCE)	+= zforce_ts.o
> > +obj-$(CONFIG_TOUCHSCREEN_COLIBRI_VF50)	+= colibri-vf50-ts.o
> > diff --git a/drivers/input/touchscreen/colibri-vf50-ts.c b/drivers/input/touchscreen/colibri-vf50-ts.c
> > new file mode 100644
> > index 0000000..eb16bdc
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/colibri-vf50-ts.c
> > @@ -0,0 +1,451 @@
> > +/* Copyright 2015 Toradex AG
> > + *
> > + * Toradex Colibri VF50 Touchscreen driver
> > + *
> > + * Originally authored by Stefan Agner for 3.0 kernel
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include <dt-bindings/gpio/gpio.h>
> 
> Why?

Remnant of old usage. Will fix.

> 
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/gpio.h>
> > +#include <linux/iio/consumer.h>
> > +#include <linux/iio/types.h>
> > +#include <linux/input.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_gpio.h>
> 
> Is not used as far as I can see.

Right. Crept in from old usage of extracting gpio information from DT. Should
have removed when I switched to using gpiod_* and removal of legacy gpio API.
Will fix.

> 
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +
> > +#define DRIVER_NAME "colibri-vf50-ts"
> > +#define DRV_VERSION "1.0"
> > +
> > +#define VF_ADC_MAX ((1 << 12) - 1)
> > +
> > +#define COLI_TOUCH_MIN_DELAY_US 1000
> > +#define COLI_TOUCH_MAX_DELAY_US 2000
> > +
> > +static int min_pressure = 200;
> > +
> > +struct vf50_touch_device {
> > +	struct platform_device	*pdev;
> > +	struct input_dev	*ts_input;
> > +	struct workqueue_struct *ts_workqueue;
> > +	struct work_struct	ts_work;
> > +	struct iio_channel	*channels;
> > +	struct gpio_desc *gpio_xp;
> > +	struct gpio_desc *gpio_xm;
> > +	struct gpio_desc *gpio_yp;
> > +	struct gpio_desc *gpio_ym;
> > +	struct gpio_desc *gpio_pen_detect;
> > +	struct gpio_desc *gpio_pen_detect_pullup;
> > +	int pen_irq;
> > +	bool stop_touchscreen;
> > +};
> > +
> > +/*
> > + * Enables given plates and measures touch parameters using ADC
> > + */
> > +static int adc_ts_measure(struct iio_channel *channel,
> > +		  struct gpio_desc *plate_p, struct gpio_desc *plate_m)
> > +{
> > +	int i, value = 0, val = 0;
> > +	int ret;
> > +
> > +	gpiod_set_value(plate_p, 1);
> > +	gpiod_set_value(plate_m, 1);
> > +
> > +	usleep_range(COLI_TOUCH_MIN_DELAY_US, COLI_TOUCH_MAX_DELAY_US);
> > +
> > +	for (i = 0; i < 5; i++) {
> > +		ret = iio_read_channel_raw(channel, &val);
> > +		if (ret < 0)
> > +			return -EINVAL;
> > +
> > +		value += val;
> > +	}
> > +
> > +	value /= 5;
> > +
> > +	gpiod_set_value(plate_p, 0);
> > +	gpiod_set_value(plate_m, 0);
> > +
> > +	return value;
> > +}
> > +
> > +/*
> > + * Enable touch detection using falling edge detection on XM
> > + */
> > +static void vf50_ts_enable_touch_detection(struct vf50_touch_device *vf50_ts)
> > +{
> > +	/* Enable plate YM (needs to be strong GND, high active) */
> > +	gpiod_set_value(vf50_ts->gpio_ym, 1);
> > +
> > +	/*
> > +	 * Let the platform mux to idle state in order to enable
> > +	 * Pull-Up on GPIO
> > +	 */
> > +	pinctrl_pm_select_idle_state(&vf50_ts->pdev->dev);
> > +}
> > +
> > +/*
> > + * ADC touch screen sampling worker function
> > + */
> > +static void vf50_ts_work(struct work_struct *ts_work)
> > +{
> > +	struct vf50_touch_device *vf50_ts = container_of(ts_work,
> > +				struct vf50_touch_device, ts_work);
> > +	struct device *dev = &vf50_ts->pdev->dev;
> > +	int val_x, val_y, val_z1, val_z2, val_p = 0;
> > +	bool discard_val_on_start = true;
> > +
> > +	while (!vf50_ts->stop_touchscreen) {
> > +		/* X-Direction */
> > +		val_x = adc_ts_measure(&vf50_ts->channels[0],
> > +				vf50_ts->gpio_xp, vf50_ts->gpio_xm);
> > +		if (val_x < 0)
> > +			continue;
> > +
> > +		/* Y-Direction */
> > +		val_y = adc_ts_measure(&vf50_ts->channels[1],
> > +				vf50_ts->gpio_yp, vf50_ts->gpio_ym);
> > +		if (val_y < 0)
> > +			continue;
> > +
> > +		/*
> > +		 * Touch pressure
> > +		 * Measure on XP/YM
> > +		 */
> > +		val_z1 = adc_ts_measure(&vf50_ts->channels[2],
> > +				vf50_ts->gpio_yp, vf50_ts->gpio_xm);
> > +		if (val_z1 < 0)
> > +			continue;
> > +		val_z2 = adc_ts_measure(&vf50_ts->channels[3],
> > +				vf50_ts->gpio_yp, vf50_ts->gpio_xm);
> > +		if (val_z2 < 0)
> > +			continue;
> > +
> > +		/* Validate signal (avoid calculation using noise) */
> > +		if (val_z1 > 64 && val_x > 64) {
> > +			/*
> > +			 * Calculate resistance between the plates
> > +			 * lower resistance means higher pressure
> > +			 */
> > +			int r_x = (1000 * val_x) / VF_ADC_MAX;
> > +
> > +			val_p = (r_x * val_z2) / val_z1 - r_x;
> > +
> > +		} else {
> > +			val_p = 2000;
> > +		}
> > +
> > +		val_p = 2000 - val_p;
> > +		dev_dbg(dev, "Measured values: x: %d, y: %d, z1: %d, z2: %d, "
> > +			"p: %d\n", val_x, val_y, val_z1, val_z2, val_p);
> > +
> > +		/*
> > +		 * If touch pressure is too low, stop measuring and reenable
> > +		 * touch detection
> > +		 */
> > +		if (val_p < min_pressure || val_p > 2000)
> > +			break;
> > +
> > +		/*
> > +		 * The pressure may not be enough for the first x and the
> > +		 * second y measurement, but, the pressure is ok when the
> > +		 * driver is doing the third and fourth measurement. To
> > +		 * take care of this, we drop the first measurement always.
> > +		 */
> > +		if (discard_val_on_start) {
> > +			discard_val_on_start = false;
> > +		} else {
> > +			/*
> > +			 * Report touch position and sleep for
> > +			 * next measurement
> > +			 */
> > +			input_report_abs(vf50_ts->ts_input,
> > +					ABS_X, VF_ADC_MAX - val_x);
> > +			input_report_abs(vf50_ts->ts_input,
> > +					ABS_Y, VF_ADC_MAX - val_y);
> > +			input_report_abs(vf50_ts->ts_input,
> > +					ABS_PRESSURE, val_p);
> > +			input_report_key(vf50_ts->ts_input, BTN_TOUCH, 1);
> > +			input_sync(vf50_ts->ts_input);
> > +		}
> > +
> > +		msleep(10);
> > +	}
> > +
> > +	/* Report no more touch, reenable touch detection */
> > +	input_report_abs(vf50_ts->ts_input, ABS_PRESSURE, 0);
> > +	input_report_key(vf50_ts->ts_input, BTN_TOUCH, 0);
> > +	input_sync(vf50_ts->ts_input);
> > +
> > +	vf50_ts_enable_touch_detection(vf50_ts);
> > +
> > +	/* Wait for the pull-up to be stable on high */
> > +	msleep(10);
> > +
> > +	/* Reenable IRQ to detect touch */
> > +	enable_irq(vf50_ts->pen_irq);
> > +
> > +	dev_dbg(dev, "Reenabled touch detection interrupt\n");
> > +}
> > +
> > +static irqreturn_t vf50_ts_touched(int irq, void *dev_id)
> > +{
> > +	struct vf50_touch_device *vf50_ts = (struct vf50_touch_device *)dev_id;
> > +	struct device *dev = &vf50_ts->pdev->dev;
> > +
> > +	dev_dbg(dev, "Touch detected, start worker thread\n");
> > +
> > +	disable_irq_nosync(irq);
> > +
> > +	/* Disable the touch detection plates */
> > +	gpiod_set_value(vf50_ts->gpio_ym, 0);
> > +
> > +	/* Let the platform mux to default state in order to mux as ADC */
> > +	pinctrl_pm_select_default_state(dev);
> > +
> > +	queue_work(vf50_ts->ts_workqueue, &vf50_ts->ts_work);
> 
> If you convert this to a threaded interrupt you won't need to
> disable/reenable interrupt or queue work. You should also be able to use
> gpiod_set_value_cansleep() extending the range of ways the controller
> could be connected to systems.

I must accept I have never used threaded IRQ's. Will take a look and respin
the patch accordingly.

> 
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int vf50_ts_open(struct input_dev *dev_input)
> > +{
> > +	int ret;
> > +	struct vf50_touch_device *touchdev = input_get_drvdata(dev_input);
> > +	struct device *dev = &touchdev->pdev->dev;
> > +
> > +	dev_dbg(dev, "Input device %s opened, starting touch detection\n",
> > +			dev_input->name);
> > +
> > +	touchdev->stop_touchscreen = false;
> > +
> > +	ret = gpiod_direction_output(touchdev->gpio_xp, 0);
> > +	if (ret) {
> > +		dev_err(dev, "Could not set gpio xp as output %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = gpiod_direction_output(touchdev->gpio_xm, 0);
> > +	if (ret) {
> > +		dev_err(dev, "Could not set gpio xm as output %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = gpiod_direction_output(touchdev->gpio_yp, 0);
> > +	if (ret) {
> > +		dev_err(dev, "Could not set gpio yp as output %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = gpiod_direction_output(touchdev->gpio_ym, 0);
> > +	if (ret) {
> > +		dev_err(dev, "Could not set gpio ym as output %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = gpiod_direction_input(touchdev->gpio_pen_detect);
> > +	if (ret) {
> > +		dev_err(dev,
> > +			"Could not set gpio pen detect as input %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = gpiod_direction_input(touchdev->gpio_pen_detect_pullup);
> > +	if (ret) {
> > +		dev_err(dev,
> > +			"Could not set pen detect pullup as input %d\n", ret);
> > +		return ret;
> > +	}
> 
> I do not see this GPIO being actually used...

Correct. Will remove.

> 
> > +
> > +	/* Mux detection before request IRQ, wait for pull-up to settle */
> > +	vf50_ts_enable_touch_detection(touchdev);
> > +	msleep(10);
> > +
> > +	touchdev->pen_irq = gpiod_to_irq(touchdev->gpio_pen_detect);
> > +	if (touchdev->pen_irq < 0) {
> > +		dev_err(dev, "Unable to get IRQ for GPIO\n");
> > +		return touchdev->pen_irq;
> > +	}
> > +
> > +	ret = request_irq(touchdev->pen_irq, vf50_ts_touched,
> > +			IRQF_TRIGGER_FALLING, "touch detected", touchdev);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Unable to request IRQ %d\n", touchdev->pen_irq);
> > +		return ret;
> > +	}
> 
> I'd rather we did most initialization in probe() so that -EPROBE_DEFER
> would get handled properly.

Ok.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void vf50_ts_close(struct input_dev *dev_input)
> > +{
> > +	struct vf50_touch_device *touchdev = input_get_drvdata(dev_input);
> > +	struct device *dev = &touchdev->pdev->dev;
> > +
> > +	free_irq(touchdev->pen_irq, touchdev);
> > +
> > +	touchdev->stop_touchscreen = true;
> > +
> > +	/* Wait until touchscreen thread finishes any possible remnants. */
> > +	cancel_work_sync(&touchdev->ts_work);
> > +
> > +	dev_dbg(dev, "Input device %s closed, disable touch detection\n",
> > +		dev_input->name);
> > +}
> > +
> > +static inline int vf50_ts_get_gpiod(struct device *dev,
> > +			struct gpio_desc **gpio_d, const char *con_id)
> > +{
> > +	int ret;
> > +
> > +	*gpio_d = devm_gpiod_get(dev, con_id);
> > +	if (IS_ERR(*gpio_d)) {
> > +		ret = PTR_ERR(*gpio_d);
> > +		dev_err(dev, "Could not get gpio_%s %d\n", con_id, ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int vf50_ts_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *node = dev->of_node;
> > +	struct vf50_touch_device *touchdev;
> > +	struct input_dev *input;
> > +	int ret = 0;
> 
> Personal preference: can we call this "error"? Also no need to
> gratuitously initialize it to 0.

Ok.

> 
> > +
> > +	if (!node) {
> > +		dev_err(dev, "Device does not have associated DT data\n");
> > +		return -EINVAL;
> 
> So what? I do not see you using of_node anywhere.

Not required. Will fix.

> 
> > +	}
> > +
> > +	touchdev = devm_kzalloc(dev, sizeof(*touchdev), GFP_KERNEL);
> > +	if (touchdev == NULL)
> > +		return -ENOMEM;
> > +
> > +	input = devm_input_allocate_device(dev);
> > +	if (!input) {
> > +		dev_err(dev, "Failed to allocate TS input device\n");
> > +		ret = -ENOMEM;
> > +		return ret;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, touchdev);
> > +
> > +	touchdev->pdev = pdev;
> > +
> > +	input->name = DRIVER_NAME;
> > +	input->id.bustype = BUS_HOST;
> > +	input->dev.parent = dev;
> > +	input->open = vf50_ts_open;
> > +	input->close = vf50_ts_close;
> > +
> > +	_set_bit(EV_ABS, input->evbit);
> > +	_set_bit(EV_KEY, input->evbit);
> > +	_set_bit(BTN_TOUCH, input->keybit);
> > +	input_set_abs_params(input, ABS_X, 0, VF_ADC_MAX, 0, 0);
> > +	input_set_abs_params(input, ABS_Y, 0, VF_ADC_MAX, 0, 0);
> > +	input_set_abs_params(input, ABS_PRESSURE, 0, VF_ADC_MAX, 0, 0);
> > +
> > +	touchdev->ts_input = input;
> > +	input_set_drvdata(input, touchdev);
> > +	ret = input_register_device(input);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to register input device\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_xp, "xp");
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_xm, "xm");
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_yp, "yp");
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_ym, "ym");
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_pen_detect,
> > +				"pen-detect");
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = vf50_ts_get_gpiod(dev, &touchdev->gpio_pen_detect_pullup,
> > +				"pen-pullup");
> > +	if (ret)
> > +		return ret;
> > +
> > +	INIT_WORK(&touchdev->ts_work, vf50_ts_work);
> > +	touchdev->ts_workqueue = create_singlethread_workqueue("vf50-ts-touch");
> > +
> > +	if (!touchdev->ts_workqueue) {
> > +		ret = PTR_ERR(touchdev->ts_workqueue);
> > +		dev_err(dev,
> > +			"Failed creating vf50-ts-touch workqueue %d\n", ret);
> > +		return ret;
> > +	}
> 
> This should go away with the threaded IRQ.

Ok.

> 
> > +
> > +	touchdev->channels = iio_channel_get_all(dev);
> > +	if (IS_ERR(touchdev->channels))
> > +		return PTR_ERR(touchdev->channels);
> > +
> > +	dev_info(dev, "Attached colibri-vf50-ts driver successfully\n");
> 
> Please drop, you should see message from input core about the new device
> already.

Ok.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int vf50_ts_remove(struct platform_device *pdev)
> > +{
> > +	struct vf50_touch_device *touchdev = platform_get_drvdata(pdev);
> > +
> > +	destroy_workqueue(touchdev->ts_workqueue);
> > +	iio_channel_release_all(touchdev->channels);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id vf50_touch_of_match[] = {
> > +	{ .compatible = "toradex,vf50-touchctrl", },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, vf50_touch_of_match);
> > +
> > +static struct platform_driver __refdata vf50_touch_driver = {
> 
> Why do you need __refdata?

Crept in from the old implementation. Will purge.

> 
> > +	.driver = {
> > +		.name = "toradex,vf50_touchctrl",
> > +		.of_match_table = vf50_touch_of_match,
> > +	},
> > +	.probe = vf50_ts_probe,
> > +	.remove = vf50_ts_remove,
> > +	.prevent_deferred_probe = false,
> 
> Deferrals are enabled by default.

Ok.

> 
> > +};
> > +
> > +module_platform_driver(vf50_touch_driver);
> > +
> > +module_param(min_pressure, int, 0600);
> > +MODULE_PARM_DESC(min_pressure, "Minimum pressure for touch detection");
> 
> I'd rather let userspace figure out what it recognizes as valid touch.

Will rethink this.

Thanks for the feedback Dmitry. Will take care of all comments in the next
version.

- Sanchayan.

Re: [PATCH v2 3/4] touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

[Stefan Agner]

Hi Dmitry,

As the original author of the driver I have some remarks to your review

On 2015-07-18 01:42, Dmitry Torokhov wrote:
>> +		/*
>> +		 * If touch pressure is too low, stop measuring and reenable
>> +		 * touch detection
>> +		 */
>> +		if (val_p < min_pressure || val_p > 2000)
>> +			break;

This is where the modules touch pressure is used to stop the measurement
process and switch back to interrupt mode. See my remarks at the end.

>> +
>> +		/*
>> +		 * The pressure may not be enough for the first x and the
>> +		 * second y measurement, but, the pressure is ok when the
>> +		 * driver is doing the third and fourth measurement. To
>> +		 * take care of this, we drop the first measurement always.
>> +		 */
>> +		if (discard_val_on_start) {
>> +			discard_val_on_start = false;
>> +		} else {
>> +			/*
>> +			 * Report touch position and sleep for
>> +			 * next measurement
>> +			 */
>> +			input_report_abs(vf50_ts->ts_input,
>> +					ABS_X, VF_ADC_MAX - val_x);
>> +			input_report_abs(vf50_ts->ts_input,
>> +					ABS_Y, VF_ADC_MAX - val_y);
>> +			input_report_abs(vf50_ts->ts_input,
>> +					ABS_PRESSURE, val_p);
>> +			input_report_key(vf50_ts->ts_input, BTN_TOUCH, 1);
>> +			input_sync(vf50_ts->ts_input);
>> +		}
>> +
>> +		msleep(10);
>> +	}
>> +
>> +	/* Report no more touch, reenable touch detection */
>> +	input_report_abs(vf50_ts->ts_input, ABS_PRESSURE, 0);
>> +	input_report_key(vf50_ts->ts_input, BTN_TOUCH, 0);
>> +	input_sync(vf50_ts->ts_input);
>> +
>> +	vf50_ts_enable_touch_detection(vf50_ts);
>> +
>> +	/* Wait for the pull-up to be stable on high */
>> +	msleep(10);
>> +
>> +	/* Reenable IRQ to detect touch */
>> +	enable_irq(vf50_ts->pen_irq);
>> +
>> +	dev_dbg(dev, "Reenabled touch detection interrupt\n");
>> +}
>> +
>> +static irqreturn_t vf50_ts_touched(int irq, void *dev_id)
>> +{
>> +	struct vf50_touch_device *vf50_ts = (struct vf50_touch_device *)dev_id;
>> +	struct device *dev = &vf50_ts->pdev->dev;
>> +
>> +	dev_dbg(dev, "Touch detected, start worker thread\n");
>> +
>> +	disable_irq_nosync(irq);
>> +
>> +	/* Disable the touch detection plates */
>> +	gpiod_set_value(vf50_ts->gpio_ym, 0);
>> +
>> +	/* Let the platform mux to default state in order to mux as ADC */
>> +	pinctrl_pm_select_default_state(dev);
>> +
>> +	queue_work(vf50_ts->ts_workqueue, &vf50_ts->ts_work);
> 
> If you convert this to a threaded interrupt you won't need to
> disable/reenable interrupt or queue work. You should also be able to use
> gpiod_set_value_cansleep() extending the range of ways the controller
> could be connected to systems.
> 

I'm not sure if a threaded interrupt is the right thing here. While the
pen is on the touchscreen (which can be for several seconds)
measurements have to be made in a continuous loop. Is it ok for a
threaded interrupt to run that long?

I'm also not sure if it is really safe to _not_ disable the pen down
GPIO interrupt. If we get a interrupt while measuring, we should ignore
that interrupt.

On resistive touch screens the pen down works by relying on the high
resistance between the two plates while not being touched. The X-Plate
will be pulled high, the Y-Plate is strong GND. We measure on the
X-Plate (XM) which is high too. As soon as the plate is touched, XM will
be GND (since the resistance over the two plates is way lower then the
pull-up resistance). An interrupt on falling edge will trigger.

Now the measuring takes place, X, Y and pressure by using different
measuring methods.

While Y-Plate measurement the same GPIO interupt pin is used for ADC
measurement! The voltage on that pin will at that point depend on the
Y-Position of the pen position... Is it guaranteed that the GPIO
interrupt is not fired? I guess because we muxed to ADC at that point,
it won't lead to a second (spurious) interrupt... However this is a
thing which needs to be checked before removing interrupt enable/disable
calls.

>> +};
>> +
>> +module_platform_driver(vf50_touch_driver);
>> +
>> +module_param(min_pressure, int, 0600);
>> +MODULE_PARM_DESC(min_pressure, "Minimum pressure for touch detection");
> 
> I'd rather let userspace figure out what it recognizes as valid touch.
> 

This is value is used as the termination condition for the measurement
loop. It essentially defines at which pressure level we stop measuring
X/Y values. Depending on the size and resistance of the plates. However,
it is not safe to measure even at low/no pressure, since then we would
only get maximum X/Y values. Hence it is crucial that this value is
choosen properly, otherwise the driver will report "wrong" X/Y values.

Since we use the value for measurement termination, we need it in kernel
space. As far as I know we do not get such a value from user space.

--
Stefan

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

[Dmitry Torokhov]

Hi Stefan,

On Tue, Jul 21, 2015 at 04:43:36PM +0200, Stefan Agner wrote:
> Hi Dmitry,
> 
> As the original author of the driver I have some remarks to your review
> 
> On 2015-07-18 01:42, Dmitry Torokhov wrote:
> >> +		/*
> >> +		 * If touch pressure is too low, stop measuring and reenable
> >> +		 * touch detection
> >> +		 */
> >> +		if (val_p < min_pressure || val_p > 2000)
> >> +			break;
> 
> This is where the modules touch pressure is used to stop the measurement
> process and switch back to interrupt mode. See my remarks at the end.
> 
> >> +
> >> +		/*
> >> +		 * The pressure may not be enough for the first x and the
> >> +		 * second y measurement, but, the pressure is ok when the
> >> +		 * driver is doing the third and fourth measurement. To
> >> +		 * take care of this, we drop the first measurement always.
> >> +		 */
> >> +		if (discard_val_on_start) {
> >> +			discard_val_on_start = false;
> >> +		} else {
> >> +			/*
> >> +			 * Report touch position and sleep for
> >> +			 * next measurement
> >> +			 */
> >> +			input_report_abs(vf50_ts->ts_input,
> >> +					ABS_X, VF_ADC_MAX - val_x);
> >> +			input_report_abs(vf50_ts->ts_input,
> >> +					ABS_Y, VF_ADC_MAX - val_y);
> >> +			input_report_abs(vf50_ts->ts_input,
> >> +					ABS_PRESSURE, val_p);
> >> +			input_report_key(vf50_ts->ts_input, BTN_TOUCH, 1);
> >> +			input_sync(vf50_ts->ts_input);
> >> +		}
> >> +
> >> +		msleep(10);
> >> +	}
> >> +
> >> +	/* Report no more touch, reenable touch detection */
> >> +	input_report_abs(vf50_ts->ts_input, ABS_PRESSURE, 0);
> >> +	input_report_key(vf50_ts->ts_input, BTN_TOUCH, 0);
> >> +	input_sync(vf50_ts->ts_input);
> >> +
> >> +	vf50_ts_enable_touch_detection(vf50_ts);
> >> +
> >> +	/* Wait for the pull-up to be stable on high */
> >> +	msleep(10);
> >> +
> >> +	/* Reenable IRQ to detect touch */
> >> +	enable_irq(vf50_ts->pen_irq);
> >> +
> >> +	dev_dbg(dev, "Reenabled touch detection interrupt\n");
> >> +}
> >> +
> >> +static irqreturn_t vf50_ts_touched(int irq, void *dev_id)
> >> +{
> >> +	struct vf50_touch_device *vf50_ts = (struct vf50_touch_device *)dev_id;
> >> +	struct device *dev = &vf50_ts->pdev->dev;
> >> +
> >> +	dev_dbg(dev, "Touch detected, start worker thread\n");
> >> +
> >> +	disable_irq_nosync(irq);
> >> +
> >> +	/* Disable the touch detection plates */
> >> +	gpiod_set_value(vf50_ts->gpio_ym, 0);
> >> +
> >> +	/* Let the platform mux to default state in order to mux as ADC */
> >> +	pinctrl_pm_select_default_state(dev);
> >> +
> >> +	queue_work(vf50_ts->ts_workqueue, &vf50_ts->ts_work);
> > 
> > If you convert this to a threaded interrupt you won't need to
> > disable/reenable interrupt or queue work. You should also be able to use
> > gpiod_set_value_cansleep() extending the range of ways the controller
> > could be connected to systems.
> > 
> 
> I'm not sure if a threaded interrupt is the right thing here. While the
> pen is on the touchscreen (which can be for several seconds)
> measurements have to be made in a continuous loop. Is it ok for a
> threaded interrupt to run that long?

Yes, why not? Threaded interrupt is simply a kernel thread that is woken
when hard interrupt handler tells it to wake up. Very similar to
interrupt + work queue, except that the kernel manages interactions
properly for you. There are several drivers in kernel that do that, for
example auo-pixcir-ts.c or tsc2007.c

> 
> I'm also not sure if it is really safe to _not_ disable the pen down
> GPIO interrupt. If we get a interrupt while measuring, we should ignore
> that interrupt.

The interrupt management core (you'll have to annotate it as
IRQF_ONESHOT) will make sure it stays masked properly until the threaded
handler completes so you do not need to disable it explicitly.

> 
> On resistive touch screens the pen down works by relying on the high
> resistance between the two plates while not being touched. The X-Plate
> will be pulled high, the Y-Plate is strong GND. We measure on the
> X-Plate (XM) which is high too. As soon as the plate is touched, XM will
> be GND (since the resistance over the two plates is way lower then the
> pull-up resistance). An interrupt on falling edge will trigger.
> 
> Now the measuring takes place, X, Y and pressure by using different
> measuring methods.
> 
> While Y-Plate measurement the same GPIO interupt pin is used for ADC
> measurement! The voltage on that pin will at that point depend on the
> Y-Position of the pen position... Is it guaranteed that the GPIO
> interrupt is not fired? I guess because we muxed to ADC at that point,
> it won't lead to a second (spurious) interrupt... However this is a
> thing which needs to be checked before removing interrupt enable/disable
> calls.
> 
> >> +};
> >> +
> >> +module_platform_driver(vf50_touch_driver);
> >> +
> >> +module_param(min_pressure, int, 0600);
> >> +MODULE_PARM_DESC(min_pressure, "Minimum pressure for touch detection");
> > 
> > I'd rather let userspace figure out what it recognizes as valid touch.
> > 
> 
> This is value is used as the termination condition for the measurement
> loop. It essentially defines at which pressure level we stop measuring
> X/Y values. Depending on the size and resistance of the plates. However,
> it is not safe to measure even at low/no pressure, since then we would
> only get maximum X/Y values. Hence it is crucial that this value is
> choosen properly, otherwise the driver will report "wrong" X/Y values.
> 
> Since we use the value for measurement termination, we need it in kernel
> space. As far as I know we do not get such a value from user space.

If this value is device model-specific and not user preference then we
probably should pass it via device tree data instead of module parameter
then.

Thanks.

-- 
Dmitry

Re: [PATCH v2 3/4] touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

[maitysanchayan-Re5JQEeQqe8AvxtiuMwx3w]

On 15-07-21 10:20:44, Dmitry Torokhov wrote:
> Hi Stefan,
> 
> On Tue, Jul 21, 2015 at 04:43:36PM +0200, Stefan Agner wrote:
> > Hi Dmitry,
> > 
> > As the original author of the driver I have some remarks to your review
> > 
> > On 2015-07-18 01:42, Dmitry Torokhov wrote:
> > >> +		/*
> > >> +		 * If touch pressure is too low, stop measuring and reenable
> > >> +		 * touch detection
> > >> +		 */
> > >> +		if (val_p < min_pressure || val_p > 2000)
> > >> +			break;
> > 
> > This is where the modules touch pressure is used to stop the measurement
> > process and switch back to interrupt mode. See my remarks at the end.
> > 
> > >> +
> > >> +		/*
> > >> +		 * The pressure may not be enough for the first x and the
> > >> +		 * second y measurement, but, the pressure is ok when the
> > >> +		 * driver is doing the third and fourth measurement. To
> > >> +		 * take care of this, we drop the first measurement always.
> > >> +		 */
> > >> +		if (discard_val_on_start) {
> > >> +			discard_val_on_start = false;
> > >> +		} else {
> > >> +			/*
> > >> +			 * Report touch position and sleep for
> > >> +			 * next measurement
> > >> +			 */
> > >> +			input_report_abs(vf50_ts->ts_input,
> > >> +					ABS_X, VF_ADC_MAX - val_x);
> > >> +			input_report_abs(vf50_ts->ts_input,
> > >> +					ABS_Y, VF_ADC_MAX - val_y);
> > >> +			input_report_abs(vf50_ts->ts_input,
> > >> +					ABS_PRESSURE, val_p);
> > >> +			input_report_key(vf50_ts->ts_input, BTN_TOUCH, 1);
> > >> +			input_sync(vf50_ts->ts_input);
> > >> +		}
> > >> +
> > >> +		msleep(10);
> > >> +	}
> > >> +
> > >> +	/* Report no more touch, reenable touch detection */
> > >> +	input_report_abs(vf50_ts->ts_input, ABS_PRESSURE, 0);
> > >> +	input_report_key(vf50_ts->ts_input, BTN_TOUCH, 0);
> > >> +	input_sync(vf50_ts->ts_input);
> > >> +
> > >> +	vf50_ts_enable_touch_detection(vf50_ts);
> > >> +
> > >> +	/* Wait for the pull-up to be stable on high */
> > >> +	msleep(10);
> > >> +
> > >> +	/* Reenable IRQ to detect touch */
> > >> +	enable_irq(vf50_ts->pen_irq);
> > >> +
> > >> +	dev_dbg(dev, "Reenabled touch detection interrupt\n");
> > >> +}
> > >> +
> > >> +static irqreturn_t vf50_ts_touched(int irq, void *dev_id)
> > >> +{
> > >> +	struct vf50_touch_device *vf50_ts = (struct vf50_touch_device *)dev_id;
> > >> +	struct device *dev = &vf50_ts->pdev->dev;
> > >> +
> > >> +	dev_dbg(dev, "Touch detected, start worker thread\n");
> > >> +
> > >> +	disable_irq_nosync(irq);
> > >> +
> > >> +	/* Disable the touch detection plates */
> > >> +	gpiod_set_value(vf50_ts->gpio_ym, 0);
> > >> +
> > >> +	/* Let the platform mux to default state in order to mux as ADC */
> > >> +	pinctrl_pm_select_default_state(dev);
> > >> +
> > >> +	queue_work(vf50_ts->ts_workqueue, &vf50_ts->ts_work);
> > > 
> > > If you convert this to a threaded interrupt you won't need to
> > > disable/reenable interrupt or queue work. You should also be able to use
> > > gpiod_set_value_cansleep() extending the range of ways the controller
> > > could be connected to systems.
> > > 
> > 
> > I'm not sure if a threaded interrupt is the right thing here. While the
> > pen is on the touchscreen (which can be for several seconds)
> > measurements have to be made in a continuous loop. Is it ok for a
> > threaded interrupt to run that long?
> 
> Yes, why not? Threaded interrupt is simply a kernel thread that is woken
> when hard interrupt handler tells it to wake up. Very similar to
> interrupt + work queue, except that the kernel manages interactions
> properly for you. There are several drivers in kernel that do that, for
> example auo-pixcir-ts.c or tsc2007.c
> 
> > 
> > I'm also not sure if it is really safe to _not_ disable the pen down
> > GPIO interrupt. If we get a interrupt while measuring, we should ignore
> > that interrupt.
> 
> The interrupt management core (you'll have to annotate it as
> IRQF_ONESHOT) will make sure it stays masked properly until the threaded
> handler completes so you do not need to disable it explicitly.

(snip)

I tried the IRQ threaded implementation. From your reply, I can see my first
implementation was wrong in the sense that I did not use the IRQF_ONESHOT flag.
The touch response time was not good in this case, however thats to be expected
in this case from what I understand now.

With the IRQF_ONESHOT specified the response time is much better compared to
what I was seeing above, but, I still feel it is not the same as with IRQ handler
plus workqueue approach. However I have no idea how to quantify this.

So I tried explicit enabling/disabling of IRQ and to me it seems the response
slightly improves compared to IRQF_ONESHOT and the touch handling is better
compared to the IRQF_ONESHOT approach. Again however I have no idea how to
quantify it.

Perhaps we go for a request threaded irq but keep the explicit enabling/disabling
of IRQ? Will that be acceptable?

- Sanchayan.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

[maitysanchayan]

Hello Dmitry,

On 15-07-21 10:20:44, Dmitry Torokhov wrote:
> Hi Stefan,
> 
> On Tue, Jul 21, 2015 at 04:43:36PM +0200, Stefan Agner wrote:
> > Hi Dmitry,
> > 
> > As the original author of the driver I have some remarks to your review
> > 
> > On 2015-07-18 01:42, Dmitry Torokhov wrote:
> > >> +		/*
> > >> +		 * If touch pressure is too low, stop measuring and reenable
> > >> +		 * touch detection
> > >> +		 */
> > >> +		if (val_p < min_pressure || val_p > 2000)
> > >> +			break;
> > 
> > This is where the modules touch pressure is used to stop the measurement
> > process and switch back to interrupt mode. See my remarks at the end.
> > 
> > >> +
> > >> +		/*
> > >> +		 * The pressure may not be enough for the first x and the
> > >> +		 * second y measurement, but, the pressure is ok when the
> > >> +		 * driver is doing the third and fourth measurement. To
> > >> +		 * take care of this, we drop the first measurement always.
> > >> +		 */
> > >> +		if (discard_val_on_start) {
> > >> +			discard_val_on_start = false;
> > >> +		} else {
> > >> +			/*
> > >> +			 * Report touch position and sleep for
> > >> +			 * next measurement
> > >> +			 */
> > >> +			input_report_abs(vf50_ts->ts_input,
> > >> +					ABS_X, VF_ADC_MAX - val_x);
> > >> +			input_report_abs(vf50_ts->ts_input,
> > >> +					ABS_Y, VF_ADC_MAX - val_y);
> > >> +			input_report_abs(vf50_ts->ts_input,
> > >> +					ABS_PRESSURE, val_p);
> > >> +			input_report_key(vf50_ts->ts_input, BTN_TOUCH, 1);
> > >> +			input_sync(vf50_ts->ts_input);
> > >> +		}
> > >> +
> > >> +		msleep(10);
> > >> +	}
> > >> +
> > >> +	/* Report no more touch, reenable touch detection */
> > >> +	input_report_abs(vf50_ts->ts_input, ABS_PRESSURE, 0);
> > >> +	input_report_key(vf50_ts->ts_input, BTN_TOUCH, 0);
> > >> +	input_sync(vf50_ts->ts_input);
> > >> +
> > >> +	vf50_ts_enable_touch_detection(vf50_ts);
> > >> +
> > >> +	/* Wait for the pull-up to be stable on high */
> > >> +	msleep(10);
> > >> +
> > >> +	/* Reenable IRQ to detect touch */
> > >> +	enable_irq(vf50_ts->pen_irq);
> > >> +
> > >> +	dev_dbg(dev, "Reenabled touch detection interrupt\n");
> > >> +}
> > >> +
> > >> +static irqreturn_t vf50_ts_touched(int irq, void *dev_id)
> > >> +{
> > >> +	struct vf50_touch_device *vf50_ts = (struct vf50_touch_device *)dev_id;
> > >> +	struct device *dev = &vf50_ts->pdev->dev;
> > >> +
> > >> +	dev_dbg(dev, "Touch detected, start worker thread\n");
> > >> +
> > >> +	disable_irq_nosync(irq);
> > >> +
> > >> +	/* Disable the touch detection plates */
> > >> +	gpiod_set_value(vf50_ts->gpio_ym, 0);
> > >> +
> > >> +	/* Let the platform mux to default state in order to mux as ADC */
> > >> +	pinctrl_pm_select_default_state(dev);
> > >> +
> > >> +	queue_work(vf50_ts->ts_workqueue, &vf50_ts->ts_work);
> > > 
> > > If you convert this to a threaded interrupt you won't need to
> > > disable/reenable interrupt or queue work. You should also be able to use
> > > gpiod_set_value_cansleep() extending the range of ways the controller
> > > could be connected to systems.
> > > 
> > 
> > I'm not sure if a threaded interrupt is the right thing here. While the
> > pen is on the touchscreen (which can be for several seconds)
> > measurements have to be made in a continuous loop. Is it ok for a
> > threaded interrupt to run that long?
> 
> Yes, why not? Threaded interrupt is simply a kernel thread that is woken
> when hard interrupt handler tells it to wake up. Very similar to
> interrupt + work queue, except that the kernel manages interactions
> properly for you. There are several drivers in kernel that do that, for
> example auo-pixcir-ts.c or tsc2007.c
> 
> > 
> > I'm also not sure if it is really safe to _not_ disable the pen down
> > GPIO interrupt. If we get a interrupt while measuring, we should ignore
> > that interrupt.
> 
> The interrupt management core (you'll have to annotate it as
> IRQF_ONESHOT) will make sure it stays masked properly until the threaded
> handler completes so you do not need to disable it explicitly.

After working some more on threaded irq implementation, if IRQ_ONESHOT
flag is used while requesting threaded irq, on entering the IRQ handler
the vf610_gpio_irq_mask is not called and vf610_gpio_irq_unmask is not
called on exiting the thread function, not something we expected.

In contrast, using explicit disable_irq_nosync and enable_irq in the IRQ
handler and thread respectively results in the respective mask and unmask
function being called.

The vf610_gpio_irq_*mask functions are in the gpio driver for Vybrid in
drivers/gpio/gpio-vf610.c.

Can you point me in the right direction?

Thanks & Regards,
Sanchayan.

Re: [PATCH v2 3/4] touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

[Dmitry Torokhov]

Hi Sanchayan,

On Mon, Aug 03, 2015 at 08:55:44PM +0530, maitysanchayan@gmail.com wrote:
> Hello Dmitry,
> 
> On 15-07-21 10:20:44, Dmitry Torokhov wrote:
> > Hi Stefan,
> > 
> > On Tue, Jul 21, 2015 at 04:43:36PM +0200, Stefan Agner wrote:
> > > Hi Dmitry,
> > > 
> > > As the original author of the driver I have some remarks to your review
> > > 
> > > On 2015-07-18 01:42, Dmitry Torokhov wrote:
> > > >> +		/*
> > > >> +		 * If touch pressure is too low, stop measuring and reenable
> > > >> +		 * touch detection
> > > >> +		 */
> > > >> +		if (val_p < min_pressure || val_p > 2000)
> > > >> +			break;
> > > 
> > > This is where the modules touch pressure is used to stop the measurement
> > > process and switch back to interrupt mode. See my remarks at the end.
> > > 
> > > >> +
> > > >> +		/*
> > > >> +		 * The pressure may not be enough for the first x and the
> > > >> +		 * second y measurement, but, the pressure is ok when the
> > > >> +		 * driver is doing the third and fourth measurement. To
> > > >> +		 * take care of this, we drop the first measurement always.
> > > >> +		 */
> > > >> +		if (discard_val_on_start) {
> > > >> +			discard_val_on_start = false;
> > > >> +		} else {
> > > >> +			/*
> > > >> +			 * Report touch position and sleep for
> > > >> +			 * next measurement
> > > >> +			 */
> > > >> +			input_report_abs(vf50_ts->ts_input,
> > > >> +					ABS_X, VF_ADC_MAX - val_x);
> > > >> +			input_report_abs(vf50_ts->ts_input,
> > > >> +					ABS_Y, VF_ADC_MAX - val_y);
> > > >> +			input_report_abs(vf50_ts->ts_input,
> > > >> +					ABS_PRESSURE, val_p);
> > > >> +			input_report_key(vf50_ts->ts_input, BTN_TOUCH, 1);
> > > >> +			input_sync(vf50_ts->ts_input);
> > > >> +		}
> > > >> +
> > > >> +		msleep(10);
> > > >> +	}
> > > >> +
> > > >> +	/* Report no more touch, reenable touch detection */
> > > >> +	input_report_abs(vf50_ts->ts_input, ABS_PRESSURE, 0);
> > > >> +	input_report_key(vf50_ts->ts_input, BTN_TOUCH, 0);
> > > >> +	input_sync(vf50_ts->ts_input);
> > > >> +
> > > >> +	vf50_ts_enable_touch_detection(vf50_ts);
> > > >> +
> > > >> +	/* Wait for the pull-up to be stable on high */
> > > >> +	msleep(10);
> > > >> +
> > > >> +	/* Reenable IRQ to detect touch */
> > > >> +	enable_irq(vf50_ts->pen_irq);
> > > >> +
> > > >> +	dev_dbg(dev, "Reenabled touch detection interrupt\n");
> > > >> +}
> > > >> +
> > > >> +static irqreturn_t vf50_ts_touched(int irq, void *dev_id)
> > > >> +{
> > > >> +	struct vf50_touch_device *vf50_ts = (struct vf50_touch_device *)dev_id;
> > > >> +	struct device *dev = &vf50_ts->pdev->dev;
> > > >> +
> > > >> +	dev_dbg(dev, "Touch detected, start worker thread\n");
> > > >> +
> > > >> +	disable_irq_nosync(irq);
> > > >> +
> > > >> +	/* Disable the touch detection plates */
> > > >> +	gpiod_set_value(vf50_ts->gpio_ym, 0);
> > > >> +
> > > >> +	/* Let the platform mux to default state in order to mux as ADC */
> > > >> +	pinctrl_pm_select_default_state(dev);
> > > >> +
> > > >> +	queue_work(vf50_ts->ts_workqueue, &vf50_ts->ts_work);
> > > > 
> > > > If you convert this to a threaded interrupt you won't need to
> > > > disable/reenable interrupt or queue work. You should also be able to use
> > > > gpiod_set_value_cansleep() extending the range of ways the controller
> > > > could be connected to systems.
> > > > 
> > > 
> > > I'm not sure if a threaded interrupt is the right thing here. While the
> > > pen is on the touchscreen (which can be for several seconds)
> > > measurements have to be made in a continuous loop. Is it ok for a
> > > threaded interrupt to run that long?
> > 
> > Yes, why not? Threaded interrupt is simply a kernel thread that is woken
> > when hard interrupt handler tells it to wake up. Very similar to
> > interrupt + work queue, except that the kernel manages interactions
> > properly for you. There are several drivers in kernel that do that, for
> > example auo-pixcir-ts.c or tsc2007.c
> > 
> > > 
> > > I'm also not sure if it is really safe to _not_ disable the pen down
> > > GPIO interrupt. If we get a interrupt while measuring, we should ignore
> > > that interrupt.
> > 
> > The interrupt management core (you'll have to annotate it as
> > IRQF_ONESHOT) will make sure it stays masked properly until the threaded
> > handler completes so you do not need to disable it explicitly.
> 
> After working some more on threaded irq implementation, if IRQ_ONESHOT
> flag is used while requesting threaded irq, on entering the IRQ handler
> the vf610_gpio_irq_mask is not called and vf610_gpio_irq_unmask is not
> called on exiting the thread function, not something we expected.
> 
> In contrast, using explicit disable_irq_nosync and enable_irq in the IRQ
> handler and thread respectively results in the respective mask and unmask
> function being called.
> 
> The vf610_gpio_irq_*mask functions are in the gpio driver for Vybrid in
> drivers/gpio/gpio-vf610.c.

Well, for edge interrupts we normally do not mask/unmask IRQ as we
expect the controller to latch onto the state and not re-raise intil
interrupt is acked and I believe goes through edge condition again.
For level-triggered IRQs we do mask the interrupt line. See
kernel/irq/handle.c::handle_level_irq() and handle_edge_irq().

Thanks.

-- 
Dmitry

Re: [PATCH v2 3/4] touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

[maitysanchayan]

Hello Dmitry,

On 15-08-03 14:04:09, Dmitry Torokhov wrote:
> Hi Sanchayan,
> 
> On Mon, Aug 03, 2015 at 08:55:44PM +0530, maitysanchayan@gmail.com wrote:
> > Hello Dmitry,
> > 
> > On 15-07-21 10:20:44, Dmitry Torokhov wrote:
> > > Hi Stefan,
> > > 
> > > On Tue, Jul 21, 2015 at 04:43:36PM +0200, Stefan Agner wrote:
> > > > Hi Dmitry,
> > > > 
> > > > As the original author of the driver I have some remarks to your review
> > > > 
> > > > On 2015-07-18 01:42, Dmitry Torokhov wrote:
> > > > >> +		/*
> > > > >> +		 * If touch pressure is too low, stop measuring and reenable
> > > > >> +		 * touch detection
> > > > >> +		 */
> > > > >> +		if (val_p < min_pressure || val_p > 2000)
> > > > >> +			break;
> > > > 
> > > > This is where the modules touch pressure is used to stop the measurement
> > > > process and switch back to interrupt mode. See my remarks at the end.
> > > > 
> > > > >> +
> > > > >> +		/*
> > > > >> +		 * The pressure may not be enough for the first x and the
> > > > >> +		 * second y measurement, but, the pressure is ok when the
> > > > >> +		 * driver is doing the third and fourth measurement. To
> > > > >> +		 * take care of this, we drop the first measurement always.
> > > > >> +		 */
> > > > >> +		if (discard_val_on_start) {
> > > > >> +			discard_val_on_start = false;
> > > > >> +		} else {
> > > > >> +			/*
> > > > >> +			 * Report touch position and sleep for
> > > > >> +			 * next measurement
> > > > >> +			 */
> > > > >> +			input_report_abs(vf50_ts->ts_input,
> > > > >> +					ABS_X, VF_ADC_MAX - val_x);
> > > > >> +			input_report_abs(vf50_ts->ts_input,
> > > > >> +					ABS_Y, VF_ADC_MAX - val_y);
> > > > >> +			input_report_abs(vf50_ts->ts_input,
> > > > >> +					ABS_PRESSURE, val_p);
> > > > >> +			input_report_key(vf50_ts->ts_input, BTN_TOUCH, 1);
> > > > >> +			input_sync(vf50_ts->ts_input);
> > > > >> +		}
> > > > >> +
> > > > >> +		msleep(10);
> > > > >> +	}
> > > > >> +
> > > > >> +	/* Report no more touch, reenable touch detection */
> > > > >> +	input_report_abs(vf50_ts->ts_input, ABS_PRESSURE, 0);
> > > > >> +	input_report_key(vf50_ts->ts_input, BTN_TOUCH, 0);
> > > > >> +	input_sync(vf50_ts->ts_input);
> > > > >> +
> > > > >> +	vf50_ts_enable_touch_detection(vf50_ts);
> > > > >> +
> > > > >> +	/* Wait for the pull-up to be stable on high */
> > > > >> +	msleep(10);
> > > > >> +
> > > > >> +	/* Reenable IRQ to detect touch */
> > > > >> +	enable_irq(vf50_ts->pen_irq);
> > > > >> +
> > > > >> +	dev_dbg(dev, "Reenabled touch detection interrupt\n");
> > > > >> +}
> > > > >> +
> > > > >> +static irqreturn_t vf50_ts_touched(int irq, void *dev_id)
> > > > >> +{
> > > > >> +	struct vf50_touch_device *vf50_ts = (struct vf50_touch_device *)dev_id;
> > > > >> +	struct device *dev = &vf50_ts->pdev->dev;
> > > > >> +
> > > > >> +	dev_dbg(dev, "Touch detected, start worker thread\n");
> > > > >> +
> > > > >> +	disable_irq_nosync(irq);
> > > > >> +
> > > > >> +	/* Disable the touch detection plates */
> > > > >> +	gpiod_set_value(vf50_ts->gpio_ym, 0);
> > > > >> +
> > > > >> +	/* Let the platform mux to default state in order to mux as ADC */
> > > > >> +	pinctrl_pm_select_default_state(dev);
> > > > >> +
> > > > >> +	queue_work(vf50_ts->ts_workqueue, &vf50_ts->ts_work);
> > > > > 
> > > > > If you convert this to a threaded interrupt you won't need to
> > > > > disable/reenable interrupt or queue work. You should also be able to use
> > > > > gpiod_set_value_cansleep() extending the range of ways the controller
> > > > > could be connected to systems.
> > > > > 
> > > > 
> > > > I'm not sure if a threaded interrupt is the right thing here. While the
> > > > pen is on the touchscreen (which can be for several seconds)
> > > > measurements have to be made in a continuous loop. Is it ok for a
> > > > threaded interrupt to run that long?
> > > 
> > > Yes, why not? Threaded interrupt is simply a kernel thread that is woken
> > > when hard interrupt handler tells it to wake up. Very similar to
> > > interrupt + work queue, except that the kernel manages interactions
> > > properly for you. There are several drivers in kernel that do that, for
> > > example auo-pixcir-ts.c or tsc2007.c
> > > 
> > > > 
> > > > I'm also not sure if it is really safe to _not_ disable the pen down
> > > > GPIO interrupt. If we get a interrupt while measuring, we should ignore
> > > > that interrupt.
> > > 
> > > The interrupt management core (you'll have to annotate it as
> > > IRQF_ONESHOT) will make sure it stays masked properly until the threaded
> > > handler completes so you do not need to disable it explicitly.
> > 
> > After working some more on threaded irq implementation, if IRQ_ONESHOT
> > flag is used while requesting threaded irq, on entering the IRQ handler
> > the vf610_gpio_irq_mask is not called and vf610_gpio_irq_unmask is not
> > called on exiting the thread function, not something we expected.
> > 
> > In contrast, using explicit disable_irq_nosync and enable_irq in the IRQ
> > handler and thread respectively results in the respective mask and unmask
> > function being called.
> > 
> > The vf610_gpio_irq_*mask functions are in the gpio driver for Vybrid in
> > drivers/gpio/gpio-vf610.c.
> 
> Well, for edge interrupts we normally do not mask/unmask IRQ as we
> expect the controller to latch onto the state and not re-raise intil
> interrupt is acked and I believe goes through edge condition again.
> For level-triggered IRQs we do mask the interrupt line. See
> kernel/irq/handle.c::handle_level_irq() and handle_edge_irq().

Thank you very much for pointing me in the right direction. While using
threaded irqs we notice two different bugs which seem to have been there
for a while. One related to pinmux and other with gpio driver for Vybrid
in how it handled the irq's.

Will send out a new version with the changes incoporated. The GPIO driver
fix will be send in a separate patch by my colleague.

Thanks & Regards,
Sanchayan.

Re: [PATCH v2 3/4] touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

[Nicolae Rosia]

Hi,

On Thu, Jul 16, 2015 at 6:13 PM, Sanchayan Maity
<maitysanchayan@gmail.com> wrote:
> The Colibri Vybrid VF50 module supports 4-wire touchscreens using
> FETs and ADC inputs. This driver uses the IIO consumer interface
> and relies on the vf610_adc driver based on the IIO framework.
>
> Signed-off-by: Sanch
> +static const struct of_device_id vf50_touch_of_match[] = {
> +       { .compatible = "toradex,vf50-touchctrl", },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, vf50_touch_of_match);
> +
> +static struct platform_driver __refdata vf50_touch_driver = {
> +       .driver = {
> +               .name = "toradex,vf50_touchctrl",
> +               .of_match_table = vf50_touch_of_match,
> +       },
> +       .probe = vf50_ts_probe,
> +       .remove = vf50_ts_remove,
> +       .prevent_deferred_probe = false,
> +};
Why Toradex ? Isn't this a Freescale IP ?

Re: [PATCH v2 3/4] touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

[maitysanchayan]

On 15-07-18 14:03:25, Nicolae Rosia wrote:
> Hi,
> 
> On Thu, Jul 16, 2015 at 6:13 PM, Sanchayan Maity
> <maitysanchayan@gmail.com> wrote:
> > The Colibri Vybrid VF50 module supports 4-wire touchscreens using
> > FETs and ADC inputs. This driver uses the IIO consumer interface
> > and relies on the vf610_adc driver based on the IIO framework.
> >
> > Signed-off-by: Sanch
> > +static const struct of_device_id vf50_touch_of_match[] = {
> > +       { .compatible = "toradex,vf50-touchctrl", },
> > +       { }
> > +};
> > +MODULE_DEVICE_TABLE(of, vf50_touch_of_match);
> > +
> > +static struct platform_driver __refdata vf50_touch_driver = {
> > +       .driver = {
> > +               .name = "toradex,vf50_touchctrl",
> > +               .of_match_table = vf50_touch_of_match,
> > +       },
> > +       .probe = vf50_ts_probe,
> > +       .remove = vf50_ts_remove,
> > +       .prevent_deferred_probe = false,
> > +};
> Why Toradex ? Isn't this a Freescale IP ?

The 4 wire touchscreen support is provided by using on module circuitry
mainly comprising of FET's and leveraging the GPIOs and on chip ADC of
the Vybrid SoC.

This is specific to our Colibri Vybrid VF50 module and not the Freescale
IP. While I guess one could certainly use the driver for their own needs
if one were to replicate a similar circuitry and change the DT properties
concerning GPIO and ADC's as per their own board, as of now this is only
use on our Toradex VF50 modules and was done by us specifically to provide
touchscreen support for VF50.

Regards,
Sanchayan.

[PATCH v2 4/4] input: Add DT binding documentation for Colibri VF50 touchscreen

[Sanchayan Maity]

This adds device tree binding documentation for the Colibri VF50
touchscreen driver.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 .../bindings/input/touchscreen/colibri-vf50-ts.txt | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt

diff --git a/Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt b/Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt
new file mode 100644
index 0000000..caad52a
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt
@@ -0,0 +1,34 @@
+* Toradex Colibri VF50 Touchscreen driver
+
+Required Properties:
+- compatible must be toradex,vf50-touchctrl
+- io-channels: adc channels being used by the Colibri VF50 module
+- xp-gpios: FET gate driver for input of X+
+- xm-gpios: FET gate driver for input of X-
+- yp-gpios: FET gate driver for input of Y+
+- ym-gpios: FET gate driver for input of Y-
+- pen-detect-gpios: GPIO for pen detect irq
+- pen-pullup-gpios: GPIO for pen pullup
+- pinctrl-names: "idle", "default", "gpios"
+- pinctrl-0: pinctrl node for idle state gpio pinmux
+- pinctrl-1: pinctrl node for touch detection state pinmux
+- pinctrl-2: pinctrl node for gpios functioning as FET gate drivers
+
+Example:
+
+	touchctrl: vf50_touchctrl {
+		compatible = "toradex,vf50-touchctrl";
+		io-channels = <&adc1 0>,<&adc0 0>,
+				<&adc0 1>,<&adc1 2>;
+		xp-gpios = <&gpio0 13 GPIO_ACTIVE_LOW>;
+		xm-gpios = <&gpio2 29 GPIO_ACTIVE_HIGH>;
+		yp-gpios = <&gpio0 12 GPIO_ACTIVE_LOW>;
+		ym-gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>;
+		pen-detect-gpios = <&gpio0 8 GPIO_ACTIVE_HIGH>;
+		pen-pullup-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "idle","default","gpios";
+		pinctrl-0 = <&pinctrl_touchctrl_idle>;
+		pinctrl-1 = <&pinctrl_touchctrl_default>;
+		pinctrl-2 = <&pinctrl_touchctrl_gpios>;
+		status = "disabled";
+	};
-- 
2.4.6