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

Message ID 1414408111-2631-2-git-send-email-vigneshr@ti.com
State New, archived
Headers show

Commit Message

Vignesh Raghavendra Oct. 27, 2014, 11:08 a.m. UTC
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(-)

Comments

Hartmut Knaack Oct. 31, 2014, 9:03 p.m. UTC | #1
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;
>  		}
>  	}
>  }
> 

--
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
Richard Cochran Nov. 6, 2014, 2:19 p.m. UTC | #2
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
--
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
Vignesh Raghavendra Nov. 7, 2014, 5:34 a.m. UTC | #3
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
--
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
Vignesh Raghavendra Nov. 7, 2014, 5:48 a.m. UTC | #4
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
> 
--
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
Richard Cochran Nov. 7, 2014, 8 a.m. UTC | #5
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Cochran Nov. 7, 2014, 10:17 a.m. UTC | #6
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vignesh Raghavendra Nov. 10, 2014, 10:46 a.m. UTC | #7
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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;
 		}
 	}
 }