Re: [PATCH 1/4] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps
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 0xf) 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-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps
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 0xf) 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-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps
On Saturday 01 November 2014 02:33 AM, Hartmut Knaack wrote: Vignesh R schrieb am 27.10.2014 12:08: From: Brad Griffis bgrif...@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 bgrif...@ti.com [vigne...@ti.com: Ported the patch from v3.12 to v3.18rc2] Signed-off-by: Vignesh R vigne...@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 intconfig; 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
Re: [PATCH 1/4] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps
Vignesh R schrieb am 27.10.2014 12:08: From: Brad Griffis bgrif...@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 bgrif...@ti.com [vigne...@ti.com: Ported the patch from v3.12 to v3.18rc2] Signed-off-by: Vignesh R vigne...@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 intconfig; 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 0xf) 16; read = 0xfff; -
[PATCH 1/4] input: touchscreen: ti_am335x_tsc Interchange touchscreen and ADC steps
From: Brad Griffis bgrif...@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 bgrif...@ti.com [vigne...@ti.com: Ported the patch from v3.12 to v3.18rc2] Signed-off-by: Vignesh R vigne...@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 intconfig; 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 0xf) 16; read = 0xfff; - if (channel creads) { + if (channel first_step + creads + 2) { diff = abs(read - prev_val_x);