[PATCH 0/4] Touchscreen performance related fixes

[PATCH 0/4] Touchscreen performance related fixes

[Vignesh R]

This series of patches fix TSC defects related to lag in touchscreen
performance and cursor jump at touch release. The lag was result of
udelay in TSC interrupt handler. Cursor jump due to false pen-up event.
The patches implement Advisory 1.0.31 in silicon errata of am335x-evm
to avoid false pen-up events and remove udelay. The advisory says to use
steps 1 to 4 for ADC and 5 to 16 for TSC (assuming 4 wire TSC and 4 channel
ADC). Further the X co-ordinate must be the last one to be sampled just
before charge step. The first two patches implement the required changes. 

A DT parameter to configure the duration of tsc charge step. It represents
number of ADC clock cycles to wait between applying the step configuration
registers and going back to the IDLE state. The charge delay value can vary
across boards. Configuring correct value of charge delay is important to avoid
false pen-up events. Hence it is necessary to expose charge-delay value as
DT parameter. The pen-up detection happens immediately after the charge step
so this does in fact function as a hardware knob for adjusting the amount of
settling time.

After applying these changes false pen-up events have not be observed and
smooth circles can be drawn on touch screen. The performance is much better
in recognizing quick movement across the screen. No lag or cursor jump is
observed.

Brad Griffis (2):
  input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC
    steps
  input: touchscreen: ti_am335x_tsc: Remove udelay in interrupt handler

Vignesh R (2):
  arm: boot: dts: am335x-evm: Make charge delay a DT parameter for tsc
  input: touchscreen: ti_am335x_tsc: Use charge delay DT parameter

 .../bindings/input/touchscreen/ti-tsc-adc.txt      |  13 +++
 arch/arm/boot/dts/am335x-evm.dts                   |   1 +
 drivers/iio/adc/ti_am335x_adc.c                    |   2 +-
 drivers/input/touchscreen/ti_am335x_tsc.c          | 106 ++++++++++-----------
 drivers/mfd/ti_am335x_tscadc.c                     |   7 +-
 include/linux/mfd/ti_am335x_tscadc.h               |   4 +-
 6 files changed, 76 insertions(+), 57 deletions(-)

-- 
1.9.1

[PATCH 1/4] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps

[Vignesh R]

From: Brad Griffis <bgriffis@ti.com>

This patch makes the initial changes required to workaround TSC-false
pen-up interrupts. It is required to implement these changes in order to
remove udelay in the TSC interrupt handler and false pen-up events.
The charge step is to be executed immediately after sampling X+. Hence
TSC is made to use higher numbered steps (steps 5 to 16 for 5 co-ordinate
readouts, 4 wire TSC configuration) and ADC to use lower ones. Further
X co-ordinate readouts must be the last to be sampled, thus co-ordinates
are sampled in the order Y-Z-X.

Signed-off-by: Brad Griffis <bgriffis@ti.com>
[vigneshr@ti.com: Ported the patch from v3.12 to v3.18rc2]
Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/iio/adc/ti_am335x_adc.c           |  2 +-
 drivers/input/touchscreen/ti_am335x_tsc.c | 42 ++++++++++++++++++-------------
 2 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index b730864731e8..3f530ed6bd80 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -98,7 +98,7 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
 	 * needs to be given to ADC to digitalize data.
 	 */
 
-	steps = TOTAL_STEPS - adc_dev->channels;
+	steps = 0;
 	if (iio_buffer_enabled(indio_dev))
 		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
 					| STEPCONFIG_MODE_SWCNT;
diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 2ce649520fe0..1aeac9675fe7 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -121,7 +121,7 @@ static void titsc_step_config(struct titsc *ts_dev)
 {
 	unsigned int	config;
 	int i;
-	int end_step;
+	int end_step, first_step, tsc_steps;
 	u32 stepenable;
 
 	config = STEPCONFIG_MODE_HWSYNC |
@@ -140,9 +140,11 @@ static void titsc_step_config(struct titsc *ts_dev)
 		break;
 	}
 
-	/* 1 … coordinate_readouts is for X */
-	end_step = ts_dev->coordinate_readouts;
-	for (i = 0; i < end_step; i++) {
+	tsc_steps = ts_dev->coordinate_readouts * 2 + 2;
+	first_step = TOTAL_STEPS - tsc_steps;
+	/* Steps 16 to 16-coordinate_readouts is for X */
+	end_step = first_step + tsc_steps;
+	for (i = end_step - ts_dev->coordinate_readouts; i < end_step; i++) {
 		titsc_writel(ts_dev, REG_STEPCONFIG(i), config);
 		titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
 	}
@@ -164,9 +166,9 @@ static void titsc_step_config(struct titsc *ts_dev)
 		break;
 	}
 
-	/* coordinate_readouts … coordinate_readouts * 2 is for Y */
-	end_step = ts_dev->coordinate_readouts * 2;
-	for (i = ts_dev->coordinate_readouts; i < end_step; i++) {
+	/* 1 ... coordinate_readouts is for Y */
+	end_step = first_step + ts_dev->coordinate_readouts;
+	for (i = first_step; i < end_step; i++) {
 		titsc_writel(ts_dev, REG_STEPCONFIG(i), config);
 		titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
 	}
@@ -179,7 +181,7 @@ static void titsc_step_config(struct titsc *ts_dev)
 	titsc_writel(ts_dev, REG_CHARGECONFIG, config);
 	titsc_writel(ts_dev, REG_CHARGEDELAY, CHARGEDLY_OPENDLY);
 
-	/* coordinate_readouts * 2 … coordinate_readouts * 2 + 2 is for Z */
+	/* coordinate_readouts + 1 ... coordinate_readouts + 2 is for Z */
 	config = STEPCONFIG_MODE_HWSYNC |
 			STEPCONFIG_AVG_16 | ts_dev->bit_yp |
 			ts_dev->bit_xn | STEPCONFIG_INM_ADCREFM |
@@ -194,8 +196,11 @@ static void titsc_step_config(struct titsc *ts_dev)
 	titsc_writel(ts_dev, REG_STEPDELAY(end_step),
 			STEPCONFIG_OPENDLY);
 
-	/* The steps1 … end and bit 0 for TS_Charge */
-	stepenable = (1 << (end_step + 2)) - 1;
+	/* The steps end ... end - readouts * 2 + 2 and bit 0 for TS_Charge */
+	stepenable = 1;
+	for (i = 0; i < tsc_steps; i++)
+		stepenable |= 1 << (first_step + i + 1);
+
 	ts_dev->step_mask = stepenable;
 	am335x_tsc_se_set_cache(ts_dev->mfd_tscadc, ts_dev->step_mask);
 }
@@ -209,6 +214,7 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
 	unsigned int read, diff;
 	unsigned int i, channel;
 	unsigned int creads = ts_dev->coordinate_readouts;
+	unsigned int first_step = TOTAL_STEPS - (creads * 2 + 2);
 
 	*z1 = *z2 = 0;
 	if (fifocount % (creads * 2 + 2))
@@ -226,7 +232,7 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
 
 		channel = (read & 0xf0000) >> 16;
 		read &= 0xfff;
-		if (channel < creads) {
+		if (channel > first_step + creads + 2) {
 			diff = abs(read - prev_val_x);
 			if (diff < prev_diff_x) {
 				prev_diff_x = diff;
@@ -234,19 +240,19 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
 			}
 			prev_val_x = read;
 
-		} else if (channel < creads * 2) {
+		} else if (channel == first_step + creads + 1) {
+			*z1 = read;
+
+		} else if (channel == first_step + creads + 2) {
+			*z2 = read;
+
+		} else if (channel > first_step) {
 			diff = abs(read - prev_val_y);
 			if (diff < prev_diff_y) {
 				prev_diff_y = diff;
 				*y = read;
 			}
 			prev_val_y = read;
-
-		} else if (channel < creads * 2 + 1) {
-			*z1 = read;
-
-		} else if (channel < creads * 2 + 2) {
-			*z2 = read;
 		}
 	}
 }
-- 
1.9.1

[PATCH 2/4] input: touchscreen: ti_am335x_tsc: Remove udelay in interrupt handler

[Vignesh R]

From: Brad Griffis <bgriffis@ti.com>

TSC interrupt handler had udelay to avoid reporting of false pen-up
interrupt to user space. This patch implements workaround suggesting in
Advisory 1.0.31 of silicon errata for am335x, thus eliminating udelay
and touchscreen lag. This also improves performance of touchscreen and
eliminates sudden jump of cursor at touch release.

IDLECONFIG and CHARGECONFIG registers are to be configured
with same values in order to eliminate false pen-up events. This
workaround may result in false pen-down to be detected, hence considerable
charge step delay needs to be added. The charge delay is set to 0xB000
(in terms of ADC clock cycles) by default.

TSC steps are disabled at the end of every sampling cycle and EOS bit is
set. Once the EOS bit is set, the TSC steps need to be re-enabled to begin
next sampling cycle.

In one shot mode, sequencer automatically disables all enabled steps at
the end of each cycle. (both ADC steps and TSC steps) Hence these steps
need not be saved in reg_se_cache for clearing these steps at a later
stage.

Signed-off-by: Brad Griffis <bgriffis@ti.com>
[vigneshr@ti.com: Ported patch from v3.12 to v3.18rc2]
Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/input/touchscreen/ti_am335x_tsc.c | 56 ++++++++++++-------------------
 drivers/mfd/ti_am335x_tscadc.c            |  7 ++--
 include/linux/mfd/ti_am335x_tscadc.h      |  4 ++-
 3 files changed, 30 insertions(+), 37 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 1aeac9675fe7..483fd97c0e0c 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -173,11 +173,9 @@ static void titsc_step_config(struct titsc *ts_dev)
 		titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
 	}
 
-	/* Charge step configuration */
-	config = ts_dev->bit_xp | ts_dev->bit_yn |
-			STEPCHARGE_RFP_XPUL | STEPCHARGE_RFM_XNUR |
-			STEPCHARGE_INM_AN1 | STEPCHARGE_INP(ts_dev->inp_yp);
+	/* Make CHARGECONFIG same as IDLECONFIG */
 
+	config = titsc_readl(ts_dev, REG_IDLECONFIG);
 	titsc_writel(ts_dev, REG_CHARGECONFIG, config);
 	titsc_writel(ts_dev, REG_CHARGEDELAY, CHARGEDLY_OPENDLY);
 
@@ -264,9 +262,26 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	unsigned int status, irqclr = 0;
 	unsigned int x = 0, y = 0;
 	unsigned int z1, z2, z;
-	unsigned int fsm;
 
-	status = titsc_readl(ts_dev, REG_IRQSTATUS);
+	status = titsc_readl(ts_dev, REG_RAWIRQSTATUS);
+	if (status & IRQENB_HW_PEN) {
+		ts_dev->pen_down = true;
+		titsc_writel(ts_dev, REG_IRQWAKEUP, 0x00);
+		titsc_writel(ts_dev, REG_IRQCLR, IRQENB_HW_PEN);
+		irqclr |= IRQENB_HW_PEN;
+	}
+
+	if (status & IRQENB_PENUP) {
+		ts_dev->pen_down = false;
+		input_report_key(input_dev, BTN_TOUCH, 0);
+		input_report_abs(input_dev, ABS_PRESSURE, 0);
+		input_sync(input_dev);
+		irqclr |= IRQENB_PENUP;
+	}
+
+	if (status & IRQENB_EOS)
+		irqclr |= IRQENB_EOS;
+
 	/*
 	 * ADC and touchscreen share the IRQ line.
 	 * FIFO1 interrupts are used by ADC. Handle FIFO0 IRQs here only
@@ -297,34 +312,6 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 		}
 		irqclr |= IRQENB_FIFO0THRES;
 	}
-
-	/*
-	 * Time for sequencer to settle, to read
-	 * correct state of the sequencer.
-	 */
-	udelay(SEQ_SETTLE);
-
-	status = titsc_readl(ts_dev, REG_RAWIRQSTATUS);
-	if (status & IRQENB_PENUP) {
-		/* Pen up event */
-		fsm = titsc_readl(ts_dev, REG_ADCFSM);
-		if (fsm == ADCFSM_STEPID) {
-			ts_dev->pen_down = false;
-			input_report_key(input_dev, BTN_TOUCH, 0);
-			input_report_abs(input_dev, ABS_PRESSURE, 0);
-			input_sync(input_dev);
-		} else {
-			ts_dev->pen_down = true;
-		}
-		irqclr |= IRQENB_PENUP;
-	}
-
-	if (status & IRQENB_HW_PEN) {
-
-		titsc_writel(ts_dev, REG_IRQWAKEUP, 0x00);
-		titsc_writel(ts_dev, REG_IRQCLR, IRQENB_HW_PEN);
-	}
-
 	if (irqclr) {
 		titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
 		am335x_tsc_se_set_cache(ts_dev->mfd_tscadc, ts_dev->step_mask);
@@ -417,6 +404,7 @@ static int titsc_probe(struct platform_device *pdev)
 	}
 
 	titsc_writel(ts_dev, REG_IRQENABLE, IRQENB_FIFO0THRES);
+	titsc_writel(ts_dev, REG_IRQENABLE, IRQENB_EOS);
 	err = titsc_config_wires(ts_dev);
 	if (err) {
 		dev_err(&pdev->dev, "wrong i/p wire configuration\n");
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index d877e777cce6..94ef8992f46b 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -86,8 +86,12 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev *tsadc)
 		spin_lock_irq(&tsadc->reg_lock);
 		finish_wait(&tsadc->reg_se_wait, &wait);
 
+		/*
+		 * Sequencer should either be idle or
+		 * busy applying the charge step.
+		 */
 		reg = tscadc_readl(tsadc, REG_ADCFSM);
-		WARN_ON(reg & SEQ_STATUS);
+		WARN_ON(reg & SEQ_STATUS & (!CHARGE_STEP));
 		tsadc->adc_waiting = false;
 	}
 	tsadc->adc_in_use = true;
@@ -96,7 +100,6 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev *tsadc)
 void am335x_tsc_se_set_once(struct ti_tscadc_dev *tsadc, u32 val)
 {
 	spin_lock_irq(&tsadc->reg_lock);
-	tsadc->reg_se_cache |= val;
 	am335x_tscadc_need_adc(tsadc);
 
 	tscadc_writel(tsadc, REG_SE, val);
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index e2e70053470e..fcce182e4a35 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -52,6 +52,7 @@
 
 /* IRQ enable */
 #define IRQENB_HW_PEN		BIT(0)
+#define IRQENB_EOS		BIT(1)
 #define IRQENB_FIFO0THRES	BIT(2)
 #define IRQENB_FIFO0OVRRUN	BIT(3)
 #define IRQENB_FIFO0UNDRFLW	BIT(4)
@@ -107,7 +108,7 @@
 /* Charge delay */
 #define CHARGEDLY_OPEN_MASK	(0x3FFFF << 0)
 #define CHARGEDLY_OPEN(val)	((val) << 0)
-#define CHARGEDLY_OPENDLY	CHARGEDLY_OPEN(1)
+#define CHARGEDLY_OPENDLY	CHARGEDLY_OPEN(0xB000)
 
 /* Control register */
 #define CNTRLREG_TSCSSENB	BIT(0)
@@ -127,6 +128,7 @@
 
 /* Sequencer Status */
 #define SEQ_STATUS BIT(5)
+#define CHARGE_STEP		0x11
 
 #define ADC_CLK			3000000
 #define TOTAL_STEPS		16
-- 
1.9.1

[PATCH 3/4] arm: boot: dts: am335x-evm: Make charge delay a DT parameter for tsc

[Vignesh R]

The charge delay value is by default 0xB000. But it can be set to lower
values on some boards as long as false pen-ups are avoided. Lowering the
value increases the sampling rate (though current sampling rate is
sufficient for tsc operation). Hence charge delay has been made a DT
parameter.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 .../devicetree/bindings/input/touchscreen/ti-tsc-adc.txt    | 13 +++++++++++++
 arch/arm/boot/dts/am335x-evm.dts                            |  1 +
 2 files changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
index 878549ba814d..ac62769e70e4 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
@@ -28,6 +28,18 @@ Required properties:
 	ti,adc-channels: List of analog inputs available for ADC.
 			 AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
 
+Optional properties:
+- child "tsc"
+	ti,charge-delay: Length of touch screen charge delay step in terms of
+			 ADC clock cycles. Charge delay value should be large in order
+			 to avoid false pen-up events. This value affects the overall
+			 sampling speed hence need to be kept as low as possible while
+			 avoiding false pen-up event. Start from a lower value say 0x400
+			 and increase value until false pen-up events are avoided. The
+			 pen-up detection happens immediately after the charge step
+			 so this does in fact function as a hardware knob for adjusting
+			 the amount of "settling time".
+
 Example:
 	tscadc: tscadc@44e0d000 {
 		compatible = "ti,am3359-tscadc";
@@ -36,6 +48,7 @@ Example:
 			ti,x-plate-resistance = <200>;
 			ti,coordiante-readouts = <5>;
 			ti,wire-config = <0x00 0x11 0x22 0x33>;
+			ti,charge-delay = <0xB000>;
 		};
 
 		adc {
diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index e2156a583de7..80be0462298b 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -641,6 +641,7 @@
 		ti,x-plate-resistance = <200>;
 		ti,coordinate-readouts = <5>;
 		ti,wire-config = <0x00 0x11 0x22 0x33>;
+		ti,charge-delay = <0xB000>;
 	};
 
 	adc {
-- 
1.9.1

[PATCH 4/4] input: touchscreen: ti_am335x_tsc: Use charge delay DT parameter

[Vignesh R]

This patch reads charge delay from tsc DT node and writes to
REG_CHARGEDELAY register. If the charge delay is not specified in DT
then default value of 0xB000(CHARGEDLY_OPENDLY) is used.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/input/touchscreen/ti_am335x_tsc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 483fd97c0e0c..20ce76b1b6e7 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -52,6 +52,7 @@ struct titsc {
 	u32			bit_xp, bit_xn, bit_yp, bit_yn;
 	u32			inp_xp, inp_xn, inp_yp, inp_yn;
 	u32			step_mask;
+	u32			charge_delay;
 };
 
 static unsigned int titsc_readl(struct titsc *ts, unsigned int reg)
@@ -177,7 +178,7 @@ static void titsc_step_config(struct titsc *ts_dev)
 
 	config = titsc_readl(ts_dev, REG_IDLECONFIG);
 	titsc_writel(ts_dev, REG_CHARGECONFIG, config);
-	titsc_writel(ts_dev, REG_CHARGEDELAY, CHARGEDLY_OPENDLY);
+	titsc_writel(ts_dev, REG_CHARGEDELAY, ts_dev->charge_delay);
 
 	/* coordinate_readouts + 1 ... coordinate_readouts + 2 is for Z */
 	config = STEPCONFIG_MODE_HWSYNC |
@@ -361,6 +362,11 @@ static int titsc_parse_dt(struct platform_device *pdev,
 	if (err < 0)
 		return err;
 
+	err = of_property_read_u32(node, "ti,charge-delay",
+				   &ts_dev->charge_delay);
+	if (err < 0)
+		ts_dev->charge_delay = CHARGEDLY_OPENDLY;
+
 	return of_property_read_u32_array(node, "ti,wire-config",
 			ts_dev->config_inp, ARRAY_SIZE(ts_dev->config_inp));
 }
-- 
1.9.1

Re: [PATCH 0/4] Touchscreen performance related fixes

[Sebastian Andrzej Siewior]

On 10/27/2014 12:08 PM, Vignesh R wrote:
> This series of patches fix TSC defects related to lag in touchscreen

I will try to look this in the next few days. Do we really need #3 (and
then #4)? Given the complexity we have already, is there any benefit by
decreasing this value? Would  someone want to increase it? Can we safely
determine a value which works for everyone?

Sebastian

RE: [PATCH 0/4] Touchscreen performance related fixes

[Griffis, Brad]

On 10/27/2014 12:34 PM, Sebastian Andrzej Siewior wrote:
> Do we really need #3 (and then #4)? Given the complexity we have already, is there any benefit by decreasing this value? 

I specifically requested we add ti,charge-delay to the device tree because it is THE critical value to tune for a given design.  Although I think the current value of 0xB000 will be suitable for a great many designs, I expect that many users will need to adjust this value for their hardware.  Details such as which touchscreen vendor is being used and how the touchscreen is connected (header vs cable) have an effect on what's appropriate here.

> Would  someone want to increase it? Can we safely determine a value which works for everyone?

This value represents a hardware delay before checking for the pen-up event.  So in the scenario where someone is seeing excessive false pen-up events they will want to increase this parameter.  The downsize of making this larger is that it decreases the overall sampling speed of both the touchscreen as well as the standalone ADC samples.  At one point I tried making it huge, but that made the touchscreen overly sluggish because the sampling became too slow.  So there is a definite trade-off that if you make it too large the touchscreen becomes sluggish, and if you make it too small then you may see false pen-up events.  The optimal value will need to be tuned for a given design.

Re: [PATCH 1/4] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps

[Hartmut Knaack]

Vignesh R schrieb am 27.10.2014 12:08:
> From: Brad Griffis <bgriffis@ti.com>
> 
> This patch makes the initial changes required to workaround TSC-false
> pen-up interrupts. It is required to implement these changes in order to
> remove udelay in the TSC interrupt handler and false pen-up events.
> The charge step is to be executed immediately after sampling X+. Hence
> TSC is made to use higher numbered steps (steps 5 to 16 for 5 co-ordinate
> readouts, 4 wire TSC configuration) and ADC to use lower ones. Further
> X co-ordinate readouts must be the last to be sampled, thus co-ordinates
> are sampled in the order Y-Z-X.
> 
> Signed-off-by: Brad Griffis <bgriffis@ti.com>
> [vigneshr@ti.com: Ported the patch from v3.12 to v3.18rc2]
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>  drivers/iio/adc/ti_am335x_adc.c           |  2 +-
>  drivers/input/touchscreen/ti_am335x_tsc.c | 42 ++++++++++++++++++-------------
>  2 files changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index b730864731e8..3f530ed6bd80 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -98,7 +98,7 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
>  	 * needs to be given to ADC to digitalize data.
>  	 */
>  
> -	steps = TOTAL_STEPS - adc_dev->channels;
> +	steps = 0;
You could even initialize it with zero during variable definition. And I think the above comment could need an update now.
>  	if (iio_buffer_enabled(indio_dev))
>  		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
>  					| STEPCONFIG_MODE_SWCNT;
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index 2ce649520fe0..1aeac9675fe7 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -121,7 +121,7 @@ static void titsc_step_config(struct titsc *ts_dev)
>  {
>  	unsigned int	config;
>  	int i;
> -	int end_step;
> +	int end_step, first_step, tsc_steps;
>  	u32 stepenable;
>  
>  	config = STEPCONFIG_MODE_HWSYNC |
> @@ -140,9 +140,11 @@ static void titsc_step_config(struct titsc *ts_dev)
>  		break;
>  	}
>  
> -	/* 1 … coordinate_readouts is for X */
> -	end_step = ts_dev->coordinate_readouts;
> -	for (i = 0; i < end_step; i++) {
> +	tsc_steps = ts_dev->coordinate_readouts * 2 + 2;
> +	first_step = TOTAL_STEPS - tsc_steps;
> +	/* Steps 16 to 16-coordinate_readouts is for X */
> +	end_step = first_step + tsc_steps;
> +	for (i = end_step - ts_dev->coordinate_readouts; i < end_step; i++) {
>  		titsc_writel(ts_dev, REG_STEPCONFIG(i), config);
>  		titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
>  	}
> @@ -164,9 +166,9 @@ static void titsc_step_config(struct titsc *ts_dev)
>  		break;
>  	}
>  
> -	/* coordinate_readouts … coordinate_readouts * 2 is for Y */
> -	end_step = ts_dev->coordinate_readouts * 2;
> -	for (i = ts_dev->coordinate_readouts; i < end_step; i++) {
> +	/* 1 ... coordinate_readouts is for Y */
> +	end_step = first_step + ts_dev->coordinate_readouts;
> +	for (i = first_step; i < end_step; i++) {
>  		titsc_writel(ts_dev, REG_STEPCONFIG(i), config);
>  		titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
>  	}
> @@ -179,7 +181,7 @@ static void titsc_step_config(struct titsc *ts_dev)
>  	titsc_writel(ts_dev, REG_CHARGECONFIG, config);
>  	titsc_writel(ts_dev, REG_CHARGEDELAY, CHARGEDLY_OPENDLY);
>  
> -	/* coordinate_readouts * 2 … coordinate_readouts * 2 + 2 is for Z */
> +	/* coordinate_readouts + 1 ... coordinate_readouts + 2 is for Z */
>  	config = STEPCONFIG_MODE_HWSYNC |
>  			STEPCONFIG_AVG_16 | ts_dev->bit_yp |
>  			ts_dev->bit_xn | STEPCONFIG_INM_ADCREFM |
> @@ -194,8 +196,11 @@ static void titsc_step_config(struct titsc *ts_dev)
>  	titsc_writel(ts_dev, REG_STEPDELAY(end_step),
>  			STEPCONFIG_OPENDLY);
>  
> -	/* The steps1 … end and bit 0 for TS_Charge */
> -	stepenable = (1 << (end_step + 2)) - 1;
> +	/* The steps end ... end - readouts * 2 + 2 and bit 0 for TS_Charge */
> +	stepenable = 1;
> +	for (i = 0; i < tsc_steps; i++)
> +		stepenable |= 1 << (first_step + i + 1);
> +
>  	ts_dev->step_mask = stepenable;
>  	am335x_tsc_se_set_cache(ts_dev->mfd_tscadc, ts_dev->step_mask);
>  }
> @@ -209,6 +214,7 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
>  	unsigned int read, diff;
>  	unsigned int i, channel;
>  	unsigned int creads = ts_dev->coordinate_readouts;
> +	unsigned int first_step = TOTAL_STEPS - (creads * 2 + 2);
>  
>  	*z1 = *z2 = 0;
>  	if (fifocount % (creads * 2 + 2))
> @@ -226,7 +232,7 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
>  
>  		channel = (read & 0xf0000) >> 16;
>  		read &= 0xfff;
> -		if (channel < creads) {
> +		if (channel > first_step + creads + 2) {
>  			diff = abs(read - prev_val_x);
>  			if (diff < prev_diff_x) {
>  				prev_diff_x = diff;
> @@ -234,19 +240,19 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
>  			}
>  			prev_val_x = read;
>  
> -		} else if (channel < creads * 2) {
> +		} else if (channel == first_step + creads + 1) {
> +			*z1 = read;
> +
> +		} else if (channel == first_step + creads + 2) {
> +			*z2 = read;
> +
> +		} else if (channel > first_step) {
>  			diff = abs(read - prev_val_y);
>  			if (diff < prev_diff_y) {
>  				prev_diff_y = diff;
>  				*y = read;
>  			}
>  			prev_val_y = read;
> -
> -		} else if (channel < creads * 2 + 1) {
> -			*z1 = read;
> -
> -		} else if (channel < creads * 2 + 2) {
> -			*z2 = read;
>  		}
>  	}
>  }
> 

Re: [PATCH 3/4] arm: boot: dts: am335x-evm: Make charge delay a DT parameter for tsc

[Hartmut Knaack]

Vignesh R schrieb am 27.10.2014 12:08:
> The charge delay value is by default 0xB000. But it can be set to lower
> values on some boards as long as false pen-ups are avoided. Lowering the
> value increases the sampling rate (though current sampling rate is
> sufficient for tsc operation). Hence charge delay has been made a DT
> parameter.
> 
I would recommend to use a few colons to separate some thoughts. Also, limit to 80 chars per line would be beneficial. See inline.
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>  .../devicetree/bindings/input/touchscreen/ti-tsc-adc.txt    | 13 +++++++++++++
>  arch/arm/boot/dts/am335x-evm.dts                            |  1 +
>  2 files changed, 14 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> index 878549ba814d..ac62769e70e4 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> @@ -28,6 +28,18 @@ Required properties:
>  	ti,adc-channels: List of analog inputs available for ADC.
>  			 AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
>  
> +Optional properties:
> +- child "tsc"
> +	ti,charge-delay: Length of touch screen charge delay step in terms of
> +			 ADC clock cycles. Charge delay value should be large in order
> +			 to avoid false pen-up events. This value affects the overall
> +			 sampling speed hence need to be kept as low as possible while
<...> speed, hence needs to be <...>
> +			 avoiding false pen-up event. Start from a lower value say 0x400
<...> pen-up events. Start from a lower value, like 0x400, and increase <...>
> +			 and increase value until false pen-up events are avoided. The
> +			 pen-up detection happens immediately after the charge step
<...> charge step, so this <...>
> +			 so this does in fact function as a hardware knob for adjusting
> +			 the amount of "settling time".
> +
>  Example:
>  	tscadc: tscadc@44e0d000 {
>  		compatible = "ti,am3359-tscadc";
> @@ -36,6 +48,7 @@ Example:
>  			ti,x-plate-resistance = <200>;
>  			ti,coordiante-readouts = <5>;
>  			ti,wire-config = <0x00 0x11 0x22 0x33>;
> +			ti,charge-delay = <0xB000>;
>  		};
>  
>  		adc {
> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
> index e2156a583de7..80be0462298b 100644
> --- a/arch/arm/boot/dts/am335x-evm.dts
> +++ b/arch/arm/boot/dts/am335x-evm.dts
> @@ -641,6 +641,7 @@
>  		ti,x-plate-resistance = <200>;
>  		ti,coordinate-readouts = <5>;
>  		ti,wire-config = <0x00 0x11 0x22 0x33>;
> +		ti,charge-delay = <0xB000>;
>  	};
>  
>  	adc {
> 

Re: [PATCH 0/4] Touchscreen performance related fixes

[Sebastian Andrzej Siewior]

On 10/27/2014 08:02 PM, Griffis, Brad wrote:
> On 10/27/2014 12:34 PM, Sebastian Andrzej Siewior wrote:
>> Do we really need #3 (and then #4)? Given the complexity we have already, is there any benefit by decreasing this value? 
> 
> I specifically requested we add ti,charge-delay to the device tree because it is THE critical value to tune for a given design.  Although I think the current value of 0xB000 will be suitable for a great many designs, I expect that many users will need to adjust this value for their hardware.  Details such as which touchscreen vendor is being used and how the touchscreen is connected (header vs cable) have an effect on what's appropriate here.

Oh. That is one knob I hoped we could avoid since I haven't seen it
before on other TSCs. But okay. Please make sure that there is a
message printed if the default value is used. And lets hope the user
will do something about his.

>> Would  someone want to increase it? Can we safely determine a value which works for everyone?
> 
> This value represents a hardware delay before checking for the pen-up event.  So in the scenario where someone is seeing excessive false pen-up events they will want to increase this parameter.  The downsize of making this larger is that it decreases the overall sampling speed of both the touchscreen as well as the standalone ADC samples.  At one point I tried making it huge, but that made the touchscreen overly sluggish because the sampling became too slow.  So there is a definite trade-off that if you make it too large the touchscreen becomes sluggish, and if you make it too small then you may see false pen-up events.  The optimal value will need to be tuned for a given design.

I applied the patches from this series and did the following test on my
am335x-evm: A mug on the touchscreen (to make sure the events are
coming), evtest on the event node to see that the events and loop of

	cat /sys/bus/iio/devices/iio\:device0/in_voltage4_raw

In the past I was able lock up the TSC/ADC HW that way (see commit
7ca6740cd1 ("mfd: input: iio: ti_amm335x: Rework TSC/ADC
synchronization")) for details.
With this patches applied I don't seen any TSC events once the IIO
interface is (heavily) used. Can this be fixed?

Sebastian

Re: [PATCH 2/4] input: touchscreen: ti_am335x_tsc: Remove udelay in interrupt handler

[Lee Jones]

On Mon, 27 Oct 2014, Vignesh R wrote:
> From: Brad Griffis <bgriffis@ti.com>
> 
> TSC interrupt handler had udelay to avoid reporting of false pen-up
> interrupt to user space. This patch implements workaround suggesting in
> Advisory 1.0.31 of silicon errata for am335x, thus eliminating udelay
> and touchscreen lag. This also improves performance of touchscreen and
> eliminates sudden jump of cursor at touch release.
> 
> IDLECONFIG and CHARGECONFIG registers are to be configured
> with same values in order to eliminate false pen-up events. This
> workaround may result in false pen-down to be detected, hence considerable
> charge step delay needs to be added. The charge delay is set to 0xB000
> (in terms of ADC clock cycles) by default.
> 
> TSC steps are disabled at the end of every sampling cycle and EOS bit is
> set. Once the EOS bit is set, the TSC steps need to be re-enabled to begin
> next sampling cycle.
> 
> In one shot mode, sequencer automatically disables all enabled steps at
> the end of each cycle. (both ADC steps and TSC steps) Hence these steps
> need not be saved in reg_se_cache for clearing these steps at a later
> stage.
> 
> Signed-off-by: Brad Griffis <bgriffis@ti.com>
> [vigneshr@ti.com: Ported patch from v3.12 to v3.18rc2]
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>  drivers/input/touchscreen/ti_am335x_tsc.c | 56 ++++++++++++-------------------
>  drivers/mfd/ti_am335x_tscadc.c            |  7 ++--
>  include/linux/mfd/ti_am335x_tscadc.h      |  4 ++-
>  3 files changed, 30 insertions(+), 37 deletions(-)

[...]

> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> index d877e777cce6..94ef8992f46b 100644
> --- a/drivers/mfd/ti_am335x_tscadc.c
> +++ b/drivers/mfd/ti_am335x_tscadc.c
> @@ -86,8 +86,12 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev *tsadc)
>  		spin_lock_irq(&tsadc->reg_lock);
>  		finish_wait(&tsadc->reg_se_wait, &wait);
>  
> +		/*
> +		 * Sequencer should either be idle or
> +		 * busy applying the charge step.
> +		 */
>  		reg = tscadc_readl(tsadc, REG_ADCFSM);
> -		WARN_ON(reg & SEQ_STATUS);
> +		WARN_ON(reg & SEQ_STATUS & (!CHARGE_STEP));
>  		tsadc->adc_waiting = false;
>  	}
>  	tsadc->adc_in_use = true;
> @@ -96,7 +100,6 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev *tsadc)
>  void am335x_tsc_se_set_once(struct ti_tscadc_dev *tsadc, u32 val)
>  {
>  	spin_lock_irq(&tsadc->reg_lock);
> -	tsadc->reg_se_cache |= val;
>  	am335x_tscadc_need_adc(tsadc);
>  
>  	tscadc_writel(tsadc, REG_SE, val);

I believe all of these changes can, and therefor should live in a
separate patch.

> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index e2e70053470e..fcce182e4a35 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -52,6 +52,7 @@
>  
>  /* IRQ enable */
>  #define IRQENB_HW_PEN		BIT(0)
> +#define IRQENB_EOS		BIT(1)
>  #define IRQENB_FIFO0THRES	BIT(2)
>  #define IRQENB_FIFO0OVRRUN	BIT(3)
>  #define IRQENB_FIFO0UNDRFLW	BIT(4)
> @@ -107,7 +108,7 @@
>  /* Charge delay */
>  #define CHARGEDLY_OPEN_MASK	(0x3FFFF << 0)
>  #define CHARGEDLY_OPEN(val)	((val) << 0)
> -#define CHARGEDLY_OPENDLY	CHARGEDLY_OPEN(1)
> +#define CHARGEDLY_OPENDLY	CHARGEDLY_OPEN(0xB000)
>  
>  /* Control register */
>  #define CNTRLREG_TSCSSENB	BIT(0)
> @@ -127,6 +128,7 @@
>  
>  /* Sequencer Status */
>  #define SEQ_STATUS BIT(5)
> +#define CHARGE_STEP		0x11
>  
>  #define ADC_CLK			3000000
>  #define TOTAL_STEPS		16

The header changes should be split between the two Input and MFD
patches.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 0/4] Touchscreen performance related fixes

[Richard Cochran]

On Mon, Oct 27, 2014 at 04:38:27PM +0530, Vignesh R wrote:
> This series of patches fix TSC defects related to lag in touchscreen
> performance and cursor jump at touch release. The lag was result of
> udelay in TSC interrupt handler. Cursor jump due to false pen-up event.
> The patches implement Advisory 1.0.31 in silicon errata of am335x-evm
> to avoid false pen-up events and remove udelay.

That advisory has two workarounds. You have chosen the second one?

The text of the second workaround says it only works on 4 wire setups,
so I wonder how 5 wire designs will be affected.

> The advisory says to use
> steps 1 to 4 for ADC and 5 to 16 for TSC (assuming 4 wire TSC and 4 channel
> ADC).

No, it doesn't say that. (sprz360f.pdf)

> Further the X co-ordinate must be the last one to be sampled just
> before charge step. The first two patches implement the required changes. 

FWIW, I implemented the first workaround and removed the udelay not
too long ago. Like Sebastian, I saw the TSC unit hang after about
50000 interrupts when running with the workaround.

Did you test you patch for very long?

Thanks,
Richard

Re: [PATCH 0/4] Touchscreen performance related fixes

[Vignesh R]

On Monday 03 November 2014 05:47 PM, Sebastian Andrzej Siewior wrote:
> On 10/27/2014 08:02 PM, Griffis, Brad wrote:
>> On 10/27/2014 12:34 PM, Sebastian Andrzej Siewior wrote:
>>> Do we really need #3 (and then #4)? Given the complexity we have already, is there any benefit by decreasing this value? 
>>
>> I specifically requested we add ti,charge-delay to the device tree because it is THE critical value to tune for a given design.  Although I think the current value of 0xB000 will be suitable for a great many designs, I expect that many users will need to adjust this value for their hardware.  Details such as which touchscreen vendor is being used and how the touchscreen is connected (header vs cable) have an effect on what's appropriate here.
> 
> Oh. That is one knob I hoped we could avoid since I haven't seen it
> before on other TSCs. But okay. Please make sure that there is a
> message printed if the default value is used. And lets hope the user
> will do something about his.
> 
>>> Would  someone want to increase it? Can we safely determine a value which works for everyone?
>>
>> This value represents a hardware delay before checking for the pen-up event.  So in the scenario where someone is seeing excessive false pen-up events they will want to increase this parameter.  The downsize of making this larger is that it decreases the overall sampling speed of both the touchscreen as well as the standalone ADC samples.  At one point I tried making it huge, but that made the touchscreen overly sluggish because the sampling became too slow.  So there is a definite trade-off that if you make it too large the touchscreen becomes sluggish, and if you make it too small then you may see false pen-up events.  The optimal value will need to be tuned for a given design.
> 
> I applied the patches from this series and did the following test on my
> am335x-evm: A mug on the touchscreen (to make sure the events are
> coming), evtest on the event node to see that the events and loop of
> 
> 	cat /sys/bus/iio/devices/iio\:device0/in_voltage4_raw
> 
> In the past I was able lock up the TSC/ADC HW that way (see commit
> 7ca6740cd1 ("mfd: input: iio: ti_amm335x: Rework TSC/ADC
> synchronization")) for details.
> With this patches applied I don't seen any TSC events once the IIO
> interface is (heavily) used. Can this be fixed?

I ran following commands
$ evtest /dev/input/touchscreen0 &
(with heavy item on touchscreen)
 and
$ cat /sys/bus/iio/devices/iio\:device0/scan_elements/in_voltage4_en
(in a busy loop)
I tried above experiment on my board but I didn't hit any problem even
after running for close to 30 minutes. I was unable to reproduce failure

The problem may be in configuring correct charge-delay value. Please run:
$ ts_test > /dev/null
and let me know if pen events are being detected properly.

> 
> Sebastian
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 0/4] Touchscreen performance related fixes

[Sebastian Andrzej Siewior]

On 11/04/2014 12:44 PM, Vignesh R wrote:

> I ran following commands
> $ evtest /dev/input/touchscreen0 &
> (with heavy item on touchscreen)
>  and
> $ cat /sys/bus/iio/devices/iio\:device0/scan_elements/in_voltage4_en
> (in a busy loop)
> I tried above experiment on my board but I didn't hit any problem even
> after running for close to 30 minutes. I was unable to reproduce failure

Just to make sure: You had the touchscreen and iio command running in
parallel and you saw evtest output while there was iio operation?

> The problem may be in configuring correct charge-delay value. Please run:
> $ ts_test > /dev/null
> and let me know if pen events are being detected properly.

Well I get the touch events but not while busy loop on iio interface is
running (I get maybe one event every 5 seconds or so).
After all, it is the same HW you have.

Sebastian

Re: [PATCH 0/4] Touchscreen performance related fixes

[Vignesh R]

On Tuesday 04 November 2014 06:07 PM, Sebastian Andrzej Siewior wrote:
> On 11/04/2014 12:44 PM, Vignesh R wrote:
> 
>> I ran following commands
>> $ evtest /dev/input/touchscreen0 &
>> (with heavy item on touchscreen)
>>  and
>> $ cat /sys/bus/iio/devices/iio\:device0/scan_elements/in_voltage4_en
>> (in a busy loop)
>> I tried above experiment on my board but I didn't hit any problem even
>> after running for close to 30 minutes. I was unable to reproduce failure
> 
> Just to make sure: You had the touchscreen and iio command running in
> parallel and you saw evtest output while there was iio operation?
> 

Yeah.. I see both evtest and IIO outputs. Touch events are received
consistently (decent samples per second) and atleast one IIO output per
second.
Here is a part from console log
---------------------------
928
Event: time 1413204484.240114, type 3 (Absolute), code 0 (X), value 1746
Event: time 1413204484.240114, type 3 (Absolute), code 1 (Y), value 1720
Event: time 1413204484.240114, type 3 (Absolute), code 24 (Pressure),
value 99
Event: time 1413204484.240114, -------------- Report Sync ------------
Event: time 1413204484.255318, type 1 (Key), code 330 (Touch), value 0
Event: time 1413204484.255318, type 3 (Absolute), code 24 (Pressure),
value 0
Event: time 1413204484.255318, -------------- Report Sync ------------
933
Event: time 1413204484.273599, type 3 (Absolute), code 1 (Y), value 1718
Event: time 1413204484.273599, type 3 (Absolute), code 24 (Pressure),
value 99
Event: time 1413204484.273599, type 1 (Key), code 330 (Touch), value 1
Event: time 1413204484.273599, -------------- Report Sync ------------
Event: time 1413204484.290195, type 3 (Absolute), code 24 (Pressure),
value 98
Event: time 1413204484.290195, -------------- Report Sync ------------
Event: time 1413204484.306783, type 3 (Absolute), code 1 (Y), value 1719
Event: time 1413204484.306783, -------------- Report Sync ------------
946
Event: time 1413204484.322164, type 1 (Key), code 330 (Touch), value 0
Event: time 1413204484.322164, type 3 (Absolute), code 24 (Pressure),
value 0
Event: time 1413204484.322164, -------------- Report Sync ------------
Event: time 1413204484.340435, type 3 (Absolute), code 1 (Y), value 1721
Event: time 1413204484.340435, type 3 (Absolute), code 24 (Pressure),
value 98
Event: time 1413204484.340435, type 1 (Key), code 330 (Touch), value 1
Event: time 1413204484.340435, -------------- Report Sync ------------
Event: time 1413204484.357025, type 3 (Absolute), code 0 (X), value 1745
Event: time 1413204484.357025, type 3 (Absolute), code 24 (Pressure),
value 97
Event: time 1413204484.357025, -------------- Report Sync ------------
Event: time 1413204484.373621, type 3 (Absolute), code 0 (X), value 1746
Event: time 1413204484.373621, type 3 (Absolute), code 1 (Y), value 1720
Event: time 1413204484.373621, -------------- Report Sync ------------
942

---------------------------
The numbers 928, 933 etc are from oneshot ADC read. The above output is
pretty consistent over time.

>> The problem may be in configuring correct charge-delay value. Please run:
>> $ ts_test > /dev/null
>> and let me know if pen events are being detected properly.
> 
> Well I get the touch events but not while busy loop on iio interface is
> running (I get maybe one event every 5 seconds or so).
> After all, it is the same HW you have.

Yeah.. I believe HW is same. I am using rev 1.5B with
omap2plus_defconfig (+ TSC/ADC enabled)

Regards
Vignesh
> 
> Sebastian
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 0/4] Touchscreen performance related fixes

[Vignesh R]

On Monday 03 November 2014 11:39 PM, Richard Cochran wrote:
> On Mon, Oct 27, 2014 at 04:38:27PM +0530, Vignesh R wrote:
>> This series of patches fix TSC defects related to lag in touchscreen
>> performance and cursor jump at touch release. The lag was result of
>> udelay in TSC interrupt handler. Cursor jump due to false pen-up event.
>> The patches implement Advisory 1.0.31 in silicon errata of am335x-evm
>> to avoid false pen-up events and remove udelay.
> 
> That advisory has two workarounds. You have chosen the second one?

Work around one. Hence 5 wire design is not broken.

> 
> The text of the second workaround says it only works on 4 wire setups,
> so I wonder how 5 wire designs will be affected.
> 
>> The advisory says to use
>> steps 1 to 4 for ADC and 5 to 16 for TSC (assuming 4 wire TSC and 4 channel
>> ADC).
> 
> No, it doesn't say that. (sprz360f.pdf)

The pen up event detection happens immediately after charge step. Hence,
interchanging ADC and TSC steps makes sure that sampling of touch
co-ordinates and pen events are done one after the other. This
workaround was suggested by internal hardware folks. Earlier ADC steps
intervened between sampling of co-ordinates and pen event detection
which is not desirable.

> 
>> Further the X co-ordinate must be the last one to be sampled just
>> before charge step. The first two patches implement the required changes. 
> 
> FWIW, I implemented the first workaround and removed the udelay not
> too long ago. Like Sebastian, I saw the TSC unit hang after about
> 50000 interrupts when running with the workaround.
> 
> Did you test you patch for very long?

Yes, I tested for about 200000 interrupts and I didn't see any hang.
This patch series does not just implement workaround but also does some
minor changes, such as interchanging ADC and TSC steps etc, which makes
TSC driver more robust. Let me know if you encounter any issues with my
patch series.

Regards
Vignesh

> 
> Thanks,
> Richard
> 

Re: [PATCH 1/4] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps

[Richard Cochran]

On Mon, Oct 27, 2014 at 04:38:28PM +0530, Vignesh R wrote:

...

> @@ -209,6 +214,7 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
>  	unsigned int read, diff;
>  	unsigned int i, channel;
>  	unsigned int creads = ts_dev->coordinate_readouts;
> +	unsigned int first_step = TOTAL_STEPS - (creads * 2 + 2);
>  
>  	*z1 = *z2 = 0;
>  	if (fifocount % (creads * 2 + 2))
> @@ -226,7 +232,7 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
>  
>  		channel = (read & 0xf0000) >> 16;
>  		read &= 0xfff;
> -		if (channel < creads) {
> +		if (channel > first_step + creads + 2) {
>  			diff = abs(read - prev_val_x);
>  			if (diff < prev_diff_x) {
>  				prev_diff_x = diff;
> @@ -234,19 +240,19 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
>  			}
>  			prev_val_x = read;
>  
> -		} else if (channel < creads * 2) {
> +		} else if (channel == first_step + creads + 1) {
> +			*z1 = read;
> +
> +		} else if (channel == first_step + creads + 2) {
> +			*z2 = read;
> +
> +		} else if (channel > first_step) {
>  			diff = abs(read - prev_val_y);
>  			if (diff < prev_diff_y) {
>  				prev_diff_y = diff;
>  				*y = read;

While you are at it, please get rid of the this "delta filter"
nonsense.

Thanks,
Richard

>  			}
>  			prev_val_y = read;
> -
> -		} else if (channel < creads * 2 + 1) {
> -			*z1 = read;
> -
> -		} else if (channel < creads * 2 + 2) {
> -			*z2 = read;
>  		}
>  	}
>  }
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps

[Vignesh R]

On Thursday 06 November 2014 07:49 PM, Richard Cochran wrote:
> On Mon, Oct 27, 2014 at 04:38:28PM +0530, Vignesh R wrote:
> 
> ...
> 
>> @@ -209,6 +214,7 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
>>  	unsigned int read, diff;
>>  	unsigned int i, channel;
>>  	unsigned int creads = ts_dev->coordinate_readouts;
>> +	unsigned int first_step = TOTAL_STEPS - (creads * 2 + 2);
>>  
>>  	*z1 = *z2 = 0;
>>  	if (fifocount % (creads * 2 + 2))
>> @@ -226,7 +232,7 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
>>  
>>  		channel = (read & 0xf0000) >> 16;
>>  		read &= 0xfff;
>> -		if (channel < creads) {
>> +		if (channel > first_step + creads + 2) {
>>  			diff = abs(read - prev_val_x);
>>  			if (diff < prev_diff_x) {
>>  				prev_diff_x = diff;
>> @@ -234,19 +240,19 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
>>  			}
>>  			prev_val_x = read;
>>  
>> -		} else if (channel < creads * 2) {
>> +		} else if (channel == first_step + creads + 1) {
>> +			*z1 = read;
>> +
>> +		} else if (channel == first_step + creads + 2) {
>> +			*z2 = read;
>> +
>> +		} else if (channel > first_step) {
>>  			diff = abs(read - prev_val_y);
>>  			if (diff < prev_diff_y) {
>>  				prev_diff_y = diff;
>>  				*y = read;
> 
> While you are at it, please get rid of the this "delta filter"
> nonsense.

Currently, there is too much noise in the TSC hardware that is being
removed by delta filtering. I tested TSC unit by removing filtering
logic, the performance was not at all satisfactory. The cursor jumps
wayward and smooth circles cannot be drawn. Looks like delta filtering
cannot be removed as of now. May be I will try and address it in future.

Regards
Vignesh

> 
> Thanks,
> Richard
> 
>>  			}
>>  			prev_val_y = read;
>> -
>> -		} else if (channel < creads * 2 + 1) {
>> -			*z1 = read;
>> -
>> -		} else if (channel < creads * 2 + 2) {
>> -			*z2 = read;
>>  		}
>>  	}
>>  }
>> -- 
>> 1.9.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps

[Vignesh R]

On Saturday 01 November 2014 02:33 AM, Hartmut Knaack wrote:
> Vignesh R schrieb am 27.10.2014 12:08:
>> From: Brad Griffis <bgriffis@ti.com>
>>
>> This patch makes the initial changes required to workaround TSC-false
>> pen-up interrupts. It is required to implement these changes in order to
>> remove udelay in the TSC interrupt handler and false pen-up events.
>> The charge step is to be executed immediately after sampling X+. Hence
>> TSC is made to use higher numbered steps (steps 5 to 16 for 5 co-ordinate
>> readouts, 4 wire TSC configuration) and ADC to use lower ones. Further
>> X co-ordinate readouts must be the last to be sampled, thus co-ordinates
>> are sampled in the order Y-Z-X.
>>
>> Signed-off-by: Brad Griffis <bgriffis@ti.com>
>> [vigneshr@ti.com: Ported the patch from v3.12 to v3.18rc2]
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---
>>  drivers/iio/adc/ti_am335x_adc.c           |  2 +-
>>  drivers/input/touchscreen/ti_am335x_tsc.c | 42 ++++++++++++++++++-------------
>>  2 files changed, 25 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>> index b730864731e8..3f530ed6bd80 100644
>> --- a/drivers/iio/adc/ti_am335x_adc.c
>> +++ b/drivers/iio/adc/ti_am335x_adc.c
>> @@ -98,7 +98,7 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
>>  	 * needs to be given to ADC to digitalize data.
>>  	 */
>>  
>> -	steps = TOTAL_STEPS - adc_dev->channels;
>> +	steps = 0;
> You could even initialize it with zero during variable definition. And I think the above comment could need an update now.

Ok, Will update the comment and initialization

>>  	if (iio_buffer_enabled(indio_dev))
>>  		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
>>  					| STEPCONFIG_MODE_SWCNT;
>> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
>> index 2ce649520fe0..1aeac9675fe7 100644
>> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
>> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
>> @@ -121,7 +121,7 @@ static void titsc_step_config(struct titsc *ts_dev)
>>  {
>>  	unsigned int	config;
>>  	int i;
>> -	int end_step;
>> +	int end_step, first_step, tsc_steps;
>>  	u32 stepenable;
>>  
>>  	config = STEPCONFIG_MODE_HWSYNC |
>> @@ -140,9 +140,11 @@ static void titsc_step_config(struct titsc *ts_dev)
>>  		break;
>>  	}
>>  
>> -	/* 1 … coordinate_readouts is for X */
>> -	end_step = ts_dev->coordinate_readouts;
>> -	for (i = 0; i < end_step; i++) {
>> +	tsc_steps = ts_dev->coordinate_readouts * 2 + 2;
>> +	first_step = TOTAL_STEPS - tsc_steps;
>> +	/* Steps 16 to 16-coordinate_readouts is for X */
>> +	end_step = first_step + tsc_steps;
>> +	for (i = end_step - ts_dev->coordinate_readouts; i < end_step; i++) {
>>  		titsc_writel(ts_dev, REG_STEPCONFIG(i), config);
>>  		titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
>>  	}
>> @@ -164,9 +166,9 @@ static void titsc_step_config(struct titsc *ts_dev)
>>  		break;
>>  	}
>>  
>> -	/* coordinate_readouts … coordinate_readouts * 2 is for Y */
>> -	end_step = ts_dev->coordinate_readouts * 2;
>> -	for (i = ts_dev->coordinate_readouts; i < end_step; i++) {
>> +	/* 1 ... coordinate_readouts is for Y */
>> +	end_step = first_step + ts_dev->coordinate_readouts;
>> +	for (i = first_step; i < end_step; i++) {
>>  		titsc_writel(ts_dev, REG_STEPCONFIG(i), config);
>>  		titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
>>  	}
>> @@ -179,7 +181,7 @@ static void titsc_step_config(struct titsc *ts_dev)
>>  	titsc_writel(ts_dev, REG_CHARGECONFIG, config);
>>  	titsc_writel(ts_dev, REG_CHARGEDELAY, CHARGEDLY_OPENDLY);
>>  
>> -	/* coordinate_readouts * 2 … coordinate_readouts * 2 + 2 is for Z */
>> +	/* coordinate_readouts + 1 ... coordinate_readouts + 2 is for Z */
>>  	config = STEPCONFIG_MODE_HWSYNC |
>>  			STEPCONFIG_AVG_16 | ts_dev->bit_yp |
>>  			ts_dev->bit_xn | STEPCONFIG_INM_ADCREFM |
>> @@ -194,8 +196,11 @@ static void titsc_step_config(struct titsc *ts_dev)
>>  	titsc_writel(ts_dev, REG_STEPDELAY(end_step),
>>  			STEPCONFIG_OPENDLY);
>>  
>> -	/* The steps1 … end and bit 0 for TS_Charge */
>> -	stepenable = (1 << (end_step + 2)) - 1;
>> +	/* The steps end ... end - readouts * 2 + 2 and bit 0 for TS_Charge */
>> +	stepenable = 1;
>> +	for (i = 0; i < tsc_steps; i++)
>> +		stepenable |= 1 << (first_step + i + 1);
>> +
>>  	ts_dev->step_mask = stepenable;
>>  	am335x_tsc_se_set_cache(ts_dev->mfd_tscadc, ts_dev->step_mask);
>>  }
>> @@ -209,6 +214,7 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
>>  	unsigned int read, diff;
>>  	unsigned int i, channel;
>>  	unsigned int creads = ts_dev->coordinate_readouts;
>> +	unsigned int first_step = TOTAL_STEPS - (creads * 2 + 2);
>>  
>>  	*z1 = *z2 = 0;
>>  	if (fifocount % (creads * 2 + 2))
>> @@ -226,7 +232,7 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
>>  
>>  		channel = (read & 0xf0000) >> 16;
>>  		read &= 0xfff;
>> -		if (channel < creads) {
>> +		if (channel > first_step + creads + 2) {
>>  			diff = abs(read - prev_val_x);
>>  			if (diff < prev_diff_x) {
>>  				prev_diff_x = diff;
>> @@ -234,19 +240,19 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
>>  			}
>>  			prev_val_x = read;
>>  
>> -		} else if (channel < creads * 2) {
>> +		} else if (channel == first_step + creads + 1) {
>> +			*z1 = read;
>> +
>> +		} else if (channel == first_step + creads + 2) {
>> +			*z2 = read;
>> +
>> +		} else if (channel > first_step) {
>>  			diff = abs(read - prev_val_y);
>>  			if (diff < prev_diff_y) {
>>  				prev_diff_y = diff;
>>  				*y = read;
>>  			}
>>  			prev_val_y = read;
>> -
>> -		} else if (channel < creads * 2 + 1) {
>> -			*z1 = read;
>> -
>> -		} else if (channel < creads * 2 + 2) {
>> -			*z2 = read;
>>  		}
>>  	}
>>  }
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/4] arm: boot: dts: am335x-evm: Make charge delay a DT parameter for tsc

[Vignesh R]

On Saturday 01 November 2014 02:39 AM, Hartmut Knaack wrote:
> Vignesh R schrieb am 27.10.2014 12:08:
>> The charge delay value is by default 0xB000. But it can be set to lower
>> values on some boards as long as false pen-ups are avoided. Lowering the
>> value increases the sampling rate (though current sampling rate is
>> sufficient for tsc operation). Hence charge delay has been made a DT
>> parameter.
>>
> I would recommend to use a few colons to separate some thoughts. Also, limit to 80 chars per line would be beneficial. See inline.

I will address this in v2

>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---
>>  .../devicetree/bindings/input/touchscreen/ti-tsc-adc.txt    | 13 +++++++++++++
>>  arch/arm/boot/dts/am335x-evm.dts                            |  1 +
>>  2 files changed, 14 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
>> index 878549ba814d..ac62769e70e4 100644
>> --- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
>> @@ -28,6 +28,18 @@ Required properties:
>>  	ti,adc-channels: List of analog inputs available for ADC.
>>  			 AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
>>  
>> +Optional properties:
>> +- child "tsc"
>> +	ti,charge-delay: Length of touch screen charge delay step in terms of
>> +			 ADC clock cycles. Charge delay value should be large in order
>> +			 to avoid false pen-up events. This value affects the overall
>> +			 sampling speed hence need to be kept as low as possible while
> <...> speed, hence needs to be <...>
>> +			 avoiding false pen-up event. Start from a lower value say 0x400
> <...> pen-up events. Start from a lower value, like 0x400, and increase <...>
>> +			 and increase value until false pen-up events are avoided. The
>> +			 pen-up detection happens immediately after the charge step
> <...> charge step, so this <...>
>> +			 so this does in fact function as a hardware knob for adjusting
>> +			 the amount of "settling time".
>> +
>>  Example:
>>  	tscadc: tscadc@44e0d000 {
>>  		compatible = "ti,am3359-tscadc";
>> @@ -36,6 +48,7 @@ Example:
>>  			ti,x-plate-resistance = <200>;
>>  			ti,coordiante-readouts = <5>;
>>  			ti,wire-config = <0x00 0x11 0x22 0x33>;
>> +			ti,charge-delay = <0xB000>;
>>  		};
>>  
>>  		adc {
>> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
>> index e2156a583de7..80be0462298b 100644
>> --- a/arch/arm/boot/dts/am335x-evm.dts
>> +++ b/arch/arm/boot/dts/am335x-evm.dts
>> @@ -641,6 +641,7 @@
>>  		ti,x-plate-resistance = <200>;
>>  		ti,coordinate-readouts = <5>;
>>  		ti,wire-config = <0x00 0x11 0x22 0x33>;
>> +		ti,charge-delay = <0xB000>;
>>  	};
>>  
>>  	adc {
>>
> 

Re: [PATCH 2/4] input: touchscreen: ti_am335x_tsc: Remove udelay in interrupt handler

[Vignesh R]

On Monday 03 November 2014 08:35 PM, Lee Jones wrote:
> On Mon, 27 Oct 2014, Vignesh R wrote:
>> From: Brad Griffis <bgriffis@ti.com>
>>
>> TSC interrupt handler had udelay to avoid reporting of false pen-up
>> interrupt to user space. This patch implements workaround suggesting in
>> Advisory 1.0.31 of silicon errata for am335x, thus eliminating udelay
>> and touchscreen lag. This also improves performance of touchscreen and
>> eliminates sudden jump of cursor at touch release.
>>
>> IDLECONFIG and CHARGECONFIG registers are to be configured
>> with same values in order to eliminate false pen-up events. This
>> workaround may result in false pen-down to be detected, hence considerable
>> charge step delay needs to be added. The charge delay is set to 0xB000
>> (in terms of ADC clock cycles) by default.
>>
>> TSC steps are disabled at the end of every sampling cycle and EOS bit is
>> set. Once the EOS bit is set, the TSC steps need to be re-enabled to begin
>> next sampling cycle.
>>
>> In one shot mode, sequencer automatically disables all enabled steps at
>> the end of each cycle. (both ADC steps and TSC steps) Hence these steps
>> need not be saved in reg_se_cache for clearing these steps at a later
>> stage.
>>
>> Signed-off-by: Brad Griffis <bgriffis@ti.com>
>> [vigneshr@ti.com: Ported patch from v3.12 to v3.18rc2]
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---
>>  drivers/input/touchscreen/ti_am335x_tsc.c | 56 ++++++++++++-------------------
>>  drivers/mfd/ti_am335x_tscadc.c            |  7 ++--
>>  include/linux/mfd/ti_am335x_tscadc.h      |  4 ++-
>>  3 files changed, 30 insertions(+), 37 deletions(-)
> 
> [...]
> 
>> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
>> index d877e777cce6..94ef8992f46b 100644
>> --- a/drivers/mfd/ti_am335x_tscadc.c
>> +++ b/drivers/mfd/ti_am335x_tscadc.c
>> @@ -86,8 +86,12 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev *tsadc)
>>  		spin_lock_irq(&tsadc->reg_lock);
>>  		finish_wait(&tsadc->reg_se_wait, &wait);
>>  
>> +		/*
>> +		 * Sequencer should either be idle or
>> +		 * busy applying the charge step.
>> +		 */
>>  		reg = tscadc_readl(tsadc, REG_ADCFSM);
>> -		WARN_ON(reg & SEQ_STATUS);
>> +		WARN_ON(reg & SEQ_STATUS & (!CHARGE_STEP));
>>  		tsadc->adc_waiting = false;
>>  	}
>>  	tsadc->adc_in_use = true;
>> @@ -96,7 +100,6 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev *tsadc)
>>  void am335x_tsc_se_set_once(struct ti_tscadc_dev *tsadc, u32 val)
>>  {
>>  	spin_lock_irq(&tsadc->reg_lock);
>> -	tsadc->reg_se_cache |= val;
>>  	am335x_tscadc_need_adc(tsadc);
>>  
>>  	tscadc_writel(tsadc, REG_SE, val);
> 
> I believe all of these changes can, and therefor should live in a
> separate patch.

I will split this patch accordingly.

> 
>> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
>> index e2e70053470e..fcce182e4a35 100644
>> --- a/include/linux/mfd/ti_am335x_tscadc.h
>> +++ b/include/linux/mfd/ti_am335x_tscadc.h
>> @@ -52,6 +52,7 @@
>>  
>>  /* IRQ enable */
>>  #define IRQENB_HW_PEN		BIT(0)
>> +#define IRQENB_EOS		BIT(1)
>>  #define IRQENB_FIFO0THRES	BIT(2)
>>  #define IRQENB_FIFO0OVRRUN	BIT(3)
>>  #define IRQENB_FIFO0UNDRFLW	BIT(4)
>> @@ -107,7 +108,7 @@
>>  /* Charge delay */
>>  #define CHARGEDLY_OPEN_MASK	(0x3FFFF << 0)
>>  #define CHARGEDLY_OPEN(val)	((val) << 0)
>> -#define CHARGEDLY_OPENDLY	CHARGEDLY_OPEN(1)
>> +#define CHARGEDLY_OPENDLY	CHARGEDLY_OPEN(0xB000)
>>  
>>  /* Control register */
>>  #define CNTRLREG_TSCSSENB	BIT(0)
>> @@ -127,6 +128,7 @@
>>  
>>  /* Sequencer Status */
>>  #define SEQ_STATUS BIT(5)
>> +#define CHARGE_STEP		0x11
>>  
>>  #define ADC_CLK			3000000
>>  #define TOTAL_STEPS		16
> 
> The header changes should be split between the two Input and MFD
> patches.
> 

Re: [PATCH 1/4] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps

[Richard Cochran]

On Fri, Nov 07, 2014 at 11:04:05AM +0530, Vignesh R wrote:
> 
> Currently, there is too much noise in the TSC hardware that is being
> removed by delta filtering.

The so called "filter" was only programmed because the fifo entries
were being mixed up. Sebastian fixed that.

> I tested TSC unit by removing filtering
> logic, the performance was not at all satisfactory. The cursor jumps
> wayward and smooth circles cannot be drawn. Looks like delta filtering
> cannot be removed as of now. May be I will try and address it in future.

The "filter" code is nonsensical. It picks the two values in seqeunce
that are closest to one and another. How is that supposed to work?

Did you look at the "noise"? What kind of properties did you see?

A median filter makes more sense. Or sort, remove outliers, and
average. But choosing the two closest in series is silly.

Thanks,
Richard

Re: [PATCH 1/4] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps

[Richard Cochran]

On Fri, Nov 07, 2014 at 11:04:05AM +0530, Vignesh R wrote:
> Currently, there is too much noise in the TSC hardware that is being
> removed by delta filtering. I tested TSC unit by removing filtering
> logic, the performance was not at all satisfactory. The cursor jumps
> wayward and smooth circles cannot be drawn. Looks like delta filtering
> cannot be removed as of now. May be I will try and address it in future.

FWIW, on the boards that I tested, I printed the measured values out,
and I found that with 5 repetitions, I got the same value 5 times,
plus or minus a very small delta.

Thanks,
Richard

Re: [PATCH 1/4] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps

[Vignesh R]

On Friday 07 November 2014 01:30 PM, Richard Cochran wrote:
> On Fri, Nov 07, 2014 at 11:04:05AM +0530, Vignesh R wrote:
>>
>> Currently, there is too much noise in the TSC hardware that is being
>> removed by delta filtering.
> 
> The so called "filter" was only programmed because the fifo entries
> were being mixed up. Sebastian fixed that.
> 
>> I tested TSC unit by removing filtering
>> logic, the performance was not at all satisfactory. The cursor jumps
>> wayward and smooth circles cannot be drawn. Looks like delta filtering
>> cannot be removed as of now. May be I will try and address it in future.
> 
> The "filter" code is nonsensical. It picks the two values in seqeunce
> that are closest to one and another. How is that supposed to work?
> 
> Did you look at the "noise"? What kind of properties did you see?
> 
> A median filter makes more sense. Or sort, remove outliers, and
> average. But choosing the two closest in series is silly.

I was able to implement median filter as you described and achieve
reliable performance. I will append that to this series of patches in v3.

Regards
Vignesh

> 
> Thanks,
> Richard
> 

Re: [PATCH 0/4] Touchscreen performance related fixes

[Johannes Pointner]

Hello Vignesh,

I tried your patch version 3 on a customized board and had some
behavior I couldn't explain.
If I only use the touchscreen it works fine but if I also read values
from the ADCs then I get a lot of pen_up events even if I am still
touching the screen.
For the test I read via
# cat /sys/bus/iio/devices/iio\:device0/in_voltage5_raw
values from the ADC in an busy loop as you explained in an email
before. Did you also experience such behavior or do you know what
causes it?

Without the patches the touchscreen works fine during the iio test.

Thanks,
Hannes

2014-11-06 8:42 GMT+01:00 Vignesh R <vigneshr@ti.com>:
>
>
> On Monday 03 November 2014 11:39 PM, Richard Cochran wrote:
>> On Mon, Oct 27, 2014 at 04:38:27PM +0530, Vignesh R wrote:
>>> This series of patches fix TSC defects related to lag in touchscreen
>>> performance and cursor jump at touch release. The lag was result of
>>> udelay in TSC interrupt handler. Cursor jump due to false pen-up event.
>>> The patches implement Advisory 1.0.31 in silicon errata of am335x-evm
>>> to avoid false pen-up events and remove udelay.
>>
>> That advisory has two workarounds. You have chosen the second one?
>
> Work around one. Hence 5 wire design is not broken.
>
>>
>> The text of the second workaround says it only works on 4 wire setups,
>> so I wonder how 5 wire designs will be affected.
>>
>>> The advisory says to use
>>> steps 1 to 4 for ADC and 5 to 16 for TSC (assuming 4 wire TSC and 4 channel
>>> ADC).
>>
>> No, it doesn't say that. (sprz360f.pdf)
>
> The pen up event detection happens immediately after charge step. Hence,
> interchanging ADC and TSC steps makes sure that sampling of touch
> co-ordinates and pen events are done one after the other. This
> workaround was suggested by internal hardware folks. Earlier ADC steps
> intervened between sampling of co-ordinates and pen event detection
> which is not desirable.
>
>>
>>> Further the X co-ordinate must be the last one to be sampled just
>>> before charge step. The first two patches implement the required changes.
>>
>> FWIW, I implemented the first workaround and removed the udelay not
>> too long ago. Like Sebastian, I saw the TSC unit hang after about
>> 50000 interrupts when running with the workaround.
>>
>> Did you test you patch for very long?
>
> Yes, I tested for about 200000 interrupts and I didn't see any hang.
> This patch series does not just implement workaround but also does some
> minor changes, such as interchanging ADC and TSC steps etc, which makes
> TSC driver more robust. Let me know if you encounter any issues with my
> patch series.
>
> Regards
> Vignesh
>
>>
>> Thanks,
>> Richard
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] Touchscreen performance related fixes

[Vignesh R]

Hi,

On Wednesday 12 November 2014 06:30 PM, Johannes Pointner wrote:
> Hello Vignesh,
> 
> I tried your patch version 3 on a customized board and had some
> behavior I couldn't explain.
> If I only use the touchscreen it works fine but if I also read values
> from the ADCs then I get a lot of pen_up events even if I am still
> touching the screen.
> For the test I read via
> # cat /sys/bus/iio/devices/iio\:device0/in_voltage5_raw
> values from the ADC in an busy loop as you explained in an email
> before. Did you also experience such behavior or do you know what
> causes it?

Thanks for testing. I was able to fix this issue. Will post version 4
shortly.

Regards
Vignesh

> 
> Without the patches the touchscreen works fine during the iio test.
> 
> Thanks,
> Hannes
> 
> 2014-11-06 8:42 GMT+01:00 Vignesh R <vigneshr@ti.com>:
>>
>>
>> On Monday 03 November 2014 11:39 PM, Richard Cochran wrote:
>>> On Mon, Oct 27, 2014 at 04:38:27PM +0530, Vignesh R wrote:
>>>> This series of patches fix TSC defects related to lag in touchscreen
>>>> performance and cursor jump at touch release. The lag was result of
>>>> udelay in TSC interrupt handler. Cursor jump due to false pen-up event.
>>>> The patches implement Advisory 1.0.31 in silicon errata of am335x-evm
>>>> to avoid false pen-up events and remove udelay.
>>>
>>> That advisory has two workarounds. You have chosen the second one?
>>
>> Work around one. Hence 5 wire design is not broken.
>>
>>>
>>> The text of the second workaround says it only works on 4 wire setups,
>>> so I wonder how 5 wire designs will be affected.
>>>
>>>> The advisory says to use
>>>> steps 1 to 4 for ADC and 5 to 16 for TSC (assuming 4 wire TSC and 4 channel
>>>> ADC).
>>>
>>> No, it doesn't say that. (sprz360f.pdf)
>>
>> The pen up event detection happens immediately after charge step. Hence,
>> interchanging ADC and TSC steps makes sure that sampling of touch
>> co-ordinates and pen events are done one after the other. This
>> workaround was suggested by internal hardware folks. Earlier ADC steps
>> intervened between sampling of co-ordinates and pen event detection
>> which is not desirable.
>>
>>>
>>>> Further the X co-ordinate must be the last one to be sampled just
>>>> before charge step. The first two patches implement the required changes.
>>>
>>> FWIW, I implemented the first workaround and removed the udelay not
>>> too long ago. Like Sebastian, I saw the TSC unit hang after about
>>> 50000 interrupts when running with the workaround.
>>>
>>> Did you test you patch for very long?
>>
>> Yes, I tested for about 200000 interrupts and I didn't see any hang.
>> This patch series does not just implement workaround but also does some
>> minor changes, such as interchanging ADC and TSC steps etc, which makes
>> TSC driver more robust. Let me know if you encounter any issues with my
>> patch series.
>>
>> Regards
>> Vignesh
>>
>>>
>>> Thanks,
>>> Richard
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] Touchscreen performance related fixes

[Vignesh R]

Hi,

On Thursday 13 November 2014 05:53 PM, Vignesh R wrote:
> Hi,
> 
> On Wednesday 12 November 2014 06:30 PM, Johannes Pointner wrote:
>> Hello Vignesh,
>>
>> I tried your patch version 3 on a customized board and had some
>> behavior I couldn't explain.
>> If I only use the touchscreen it works fine but if I also read values
>> from the ADCs then I get a lot of pen_up events even if I am still
>> touching the screen.
>> For the test I read via
>> # cat /sys/bus/iio/devices/iio\:device0/in_voltage5_raw
>> values from the ADC in an busy loop as you explained in an email
>> before. Did you also experience such behavior or do you know what
>> causes it?
> 
> Thanks for testing. I was able to fix this issue. Will post version 4
> shortly.
> 

Were you able to test the v4 patch series on your board? Is the issue fixed?

> Regards
> Vignesh
> 
>>
>> Without the patches the touchscreen works fine during the iio test.
>>
>> Thanks,
>> Hannes
>>
>> 2014-11-06 8:42 GMT+01:00 Vignesh R <vigneshr@ti.com>:
>>>
>>>
>>> On Monday 03 November 2014 11:39 PM, Richard Cochran wrote:
>>>> On Mon, Oct 27, 2014 at 04:38:27PM +0530, Vignesh R wrote:
>>>>> This series of patches fix TSC defects related to lag in touchscreen
>>>>> performance and cursor jump at touch release. The lag was result of
>>>>> udelay in TSC interrupt handler. Cursor jump due to false pen-up event.
>>>>> The patches implement Advisory 1.0.31 in silicon errata of am335x-evm
>>>>> to avoid false pen-up events and remove udelay.
>>>>
>>>> That advisory has two workarounds. You have chosen the second one?
>>>
>>> Work around one. Hence 5 wire design is not broken.
>>>
>>>>
>>>> The text of the second workaround says it only works on 4 wire setups,
>>>> so I wonder how 5 wire designs will be affected.
>>>>
>>>>> The advisory says to use
>>>>> steps 1 to 4 for ADC and 5 to 16 for TSC (assuming 4 wire TSC and 4 channel
>>>>> ADC).
>>>>
>>>> No, it doesn't say that. (sprz360f.pdf)
>>>
>>> The pen up event detection happens immediately after charge step. Hence,
>>> interchanging ADC and TSC steps makes sure that sampling of touch
>>> co-ordinates and pen events are done one after the other. This
>>> workaround was suggested by internal hardware folks. Earlier ADC steps
>>> intervened between sampling of co-ordinates and pen event detection
>>> which is not desirable.
>>>
>>>>
>>>>> Further the X co-ordinate must be the last one to be sampled just
>>>>> before charge step. The first two patches implement the required changes.
>>>>
>>>> FWIW, I implemented the first workaround and removed the udelay not
>>>> too long ago. Like Sebastian, I saw the TSC unit hang after about
>>>> 50000 interrupts when running with the workaround.
>>>>
>>>> Did you test you patch for very long?
>>>
>>> Yes, I tested for about 200000 interrupts and I didn't see any hang.
>>> This patch series does not just implement workaround but also does some
>>> minor changes, such as interchanging ADC and TSC steps etc, which makes
>>> TSC driver more robust. Let me know if you encounter any issues with my
>>> patch series.
>>>
>>> Regards
>>> Vignesh
>>>
>>>>
>>>> Thanks,
>>>> Richard
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 0/4] Touchscreen performance related fixes

[Vignesh R]

On Tuesday 04 November 2014 06:07 PM, Sebastian Andrzej Siewior wrote:
> On 11/04/2014 12:44 PM, Vignesh R wrote:
> 
>> I ran following commands
>> $ evtest /dev/input/touchscreen0 &
>> (with heavy item on touchscreen)
>>  and
>> $ cat /sys/bus/iio/devices/iio\:device0/scan_elements/in_voltage4_en
>> (in a busy loop)
>> I tried above experiment on my board but I didn't hit any problem even
>> after running for close to 30 minutes. I was unable to reproduce failure
> 
> Just to make sure: You had the touchscreen and iio command running in
> parallel and you saw evtest output while there was iio operation?
> 
>> The problem may be in configuring correct charge-delay value. Please run:
>> $ ts_test > /dev/null
>> and let me know if pen events are being detected properly.
> 
> Well I get the touch events but not while busy loop on iio interface is
> running (I get maybe one event every 5 seconds or so).
> After all, it is the same HW you have.

Were you able to test the latest version (v4) of this patch series?
Please let me know if the performance is satisfactory.

Regards
Vignesh

> 
> Sebastian
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>