[patch 22/22] input: ads7846: introduce platform specific way to synchronize sampling

[patch 22/22] input: ads7846: introduce platform specific way to synchronize sampling

[akpm]

From: Eric Miao <eric.miao@marvell.com>

Noises can be introduced when LCD signals are being driven, some platforms
provide a signal to assist the synchronization of this sampling procedure.

Signed-off-by: Eric Miao <eric.miao@marvell.com>
Cc: David Brownell <david-b@pacbell.net>
Cc: Dmitry Torokhov <dtor@mail.ru>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/input/touchscreen/ads7846.c |   10 ++++++++++
 include/linux/spi/ads7846.h         |    2 ++
 2 files changed, 12 insertions(+)

diff -puN drivers/input/touchscreen/ads7846.c~input-ads7846-introduce-platform-specific-way-to-synchronize-sampling drivers/input/touchscreen/ads7846.c
--- a/drivers/input/touchscreen/ads7846.c~input-ads7846-introduce-platform-specific-way-to-synchronize-sampling
+++ a/drivers/input/touchscreen/ads7846.c
@@ -127,6 +127,8 @@ struct ads7846 {
 	void			(*filter_cleanup)(void *data);
 	int			(*get_pendown_state)(void);
 	int			gpio_pendown;
+
+	void			(*wait_for_sync)(void);
 };
 
 /* leave chip selected when we're done, for quicker re-select? */
@@ -511,6 +513,10 @@ static int get_pendown_state(struct ads7
 	return !gpio_get_value(ts->gpio_pendown);
 }
 
+static void null_wait_for_sync(void)
+{
+}
+
 /*
  * PENIRQ only kicks the timer.  The timer only reissues the SPI transfer,
  * to retrieve touchscreen status.
@@ -686,6 +692,7 @@ static void ads7846_rx_val(void *ads)
 	default:
 		BUG();
 	}
+	ts->wait_for_sync();
 	status = spi_async(ts->spi, m);
 	if (status)
 		dev_err(&ts->spi->dev, "spi_async --> %d\n",
@@ -723,6 +730,7 @@ static enum hrtimer_restart ads7846_time
 	} else {
 		/* pen is still down, continue with the measurement */
 		ts->msg_idx = 0;
+		ts->wait_for_sync();
 		status = spi_async(ts->spi, &ts->msg[0]);
 		if (status)
 			dev_err(&ts->spi->dev, "spi_async --> %d\n", status);
@@ -949,6 +957,8 @@ static int __devinit ads7846_probe(struc
 		ts->penirq_recheck_delay_usecs =
 				pdata->penirq_recheck_delay_usecs;
 
+	ts->wait_for_sync = (pdata->wait_for_sync) ? : null_wait_for_sync;
+
 	snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(&spi->dev));
 
 	input_dev->name = "ADS784x Touchscreen";
diff -puN include/linux/spi/ads7846.h~input-ads7846-introduce-platform-specific-way-to-synchronize-sampling include/linux/spi/ads7846.h
--- a/include/linux/spi/ads7846.h~input-ads7846-introduce-platform-specific-way-to-synchronize-sampling
+++ a/include/linux/spi/ads7846.h
@@ -51,5 +51,7 @@ struct ads7846_platform_data {
 				 void **filter_data);
 	int	(*filter)	(void *filter_data, int data_idx, int *val);
 	void	(*filter_cleanup)(void *filter_data);
+
+	void	(*wait_for_sync)(void);
 };
 
_

Re: [patch 22/22] input: ads7846: introduce platform specific way to synchronize sampling

[Dmitry Torokhov]

Hi,

On Wed, Mar 04, 2009 at 11:59:13AM -0800, akpm@linux-foundation.org wrote:
> From: Eric Miao <eric.miao@marvell.com>
> 
> Noises can be introduced when LCD signals are being driven, some platforms
> provide a signal to assist the synchronization of this sampling procedure.
> 
> Signed-off-by: Eric Miao <eric.miao@marvell.com>
> Cc: David Brownell <david-b@pacbell.net>
> Cc: Dmitry Torokhov <dtor@mail.ru>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  drivers/input/touchscreen/ads7846.c |   10 ++++++++++
>  include/linux/spi/ads7846.h         |    2 ++
>  2 files changed, 12 insertions(+)
> 
> diff -puN drivers/input/touchscreen/ads7846.c~input-ads7846-introduce-platform-specific-way-to-synchronize-sampling drivers/input/touchscreen/ads7846.c
> --- a/drivers/input/touchscreen/ads7846.c~input-ads7846-introduce-platform-specific-way-to-synchronize-sampling
> +++ a/drivers/input/touchscreen/ads7846.c
> @@ -127,6 +127,8 @@ struct ads7846 {
>  	void			(*filter_cleanup)(void *data);
>  	int			(*get_pendown_state)(void);
>  	int			gpio_pendown;
> +
> +	void			(*wait_for_sync)(void);
>  };
>  
>  /* leave chip selected when we're done, for quicker re-select? */
> @@ -511,6 +513,10 @@ static int get_pendown_state(struct ads7
>  	return !gpio_get_value(ts->gpio_pendown);
>  }
>  
> +static void null_wait_for_sync(void)
> +{
> +}
> +
>  /*
>   * PENIRQ only kicks the timer.  The timer only reissues the SPI transfer,
>   * to retrieve touchscreen status.
> @@ -686,6 +692,7 @@ static void ads7846_rx_val(void *ads)
>  	default:
>  		BUG();
>  	}
> +	ts->wait_for_sync();
>  	status = spi_async(ts->spi, m);
>  	if (status)
>  		dev_err(&ts->spi->dev, "spi_async --> %d\n",
> @@ -723,6 +730,7 @@ static enum hrtimer_restart ads7846_time
>  	} else {
>  		/* pen is still down, continue with the measurement */
>  		ts->msg_idx = 0;
> +		ts->wait_for_sync();

We are in hard IRQ context here. What is the expectation of
wait_for_sync()? How long are we waiting here?

-- 
Dmitry

RE: [patch 22/22] input: ads7846: introduce platform specific way to synchronize sampling

[Eric Miao]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: 2009年3月13日 10:34
> To: akpm@linux-foundation.org
> Cc: linux-input@vger.kernel.org; Eric Miao; david-b@pacbell.net
> Subject: Re: [patch 22/22] input: ads7846: introduce platform specific way
> to synchronize sampling
>
> Hi,
>
> On Wed, Mar 04, 2009 at 11:59:13AM -0800, akpm@linux-foundation.org wrote:
> > From: Eric Miao <eric.miao@marvell.com>
> >
> > Noises can be introduced when LCD signals are being driven, some platforms
> > provide a signal to assist the synchronization of this sampling procedure.
> >
> > Signed-off-by: Eric Miao <eric.miao@marvell.com>
> > Cc: David Brownell <david-b@pacbell.net>
> > Cc: Dmitry Torokhov <dtor@mail.ru>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >
> >  drivers/input/touchscreen/ads7846.c |   10 ++++++++++
> >  include/linux/spi/ads7846.h         |    2 ++
> >  2 files changed, 12 insertions(+)
> >
> > diff -puN drivers/input/touchscreen/ads7846.c~input-ads7846-introduce-
> platform-specific-way-to-synchronize-sampling
> drivers/input/touchscreen/ads7846.c
> > --- a/drivers/input/touchscreen/ads7846.c~input-ads7846-introduce-
> platform-specific-way-to-synchronize-sampling
> > +++ a/drivers/input/touchscreen/ads7846.c
> > @@ -127,6 +127,8 @@ struct ads7846 {
> >       void                    (*filter_cleanup)(void *data);
> >       int                     (*get_pendown_state)(void);
> >       int                     gpio_pendown;
> > +
> > +     void                    (*wait_for_sync)(void);
> >  };
> >
> >  /* leave chip selected when we're done, for quicker re-select? */
> > @@ -511,6 +513,10 @@ static int get_pendown_state(struct ads7
> >       return !gpio_get_value(ts->gpio_pendown);
> >  }
> >
> > +static void null_wait_for_sync(void)
> > +{
> > +}
> > +
> >  /*
> >   * PENIRQ only kicks the timer.  The timer only reissues the SPI transfer,
> >   * to retrieve touchscreen status.
> > @@ -686,6 +692,7 @@ static void ads7846_rx_val(void *ads)
> >       default:
> >               BUG();
> >       }
> > +     ts->wait_for_sync();
> >       status = spi_async(ts->spi, m);
> >       if (status)
> >               dev_err(&ts->spi->dev, "spi_async --> %d\n",
> > @@ -723,6 +730,7 @@ static enum hrtimer_restart ads7846_time
> >       } else {
> >               /* pen is still down, continue with the measurement */
> >               ts->msg_idx = 0;
> > +             ts->wait_for_sync();
>
> We are in hard IRQ context here. What is the expectation of
> wait_for_sync()? How long are we waiting here?
>

This should be a workaround for broken hardware and I don't expect
this sync time on every boards.

> --
> Dmitry

Re: [patch 22/22] input: ads7846: introduce platform specific way to synchronize sampling

[Eric Miao]

2009/3/13 Eric Miao <ymiao3@marvell.com>:
>
>
>> -----Original Message-----
>> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
>> Sent: 2009年3月13日 10:34
>> To: akpm@linux-foundation.org
>> Cc: linux-input@vger.kernel.org; Eric Miao; david-b@pacbell.net
>> Subject: Re: [patch 22/22] input: ads7846: introduce platform specific way
>> to synchronize sampling
>>
>> Hi,
>>
>> On Wed, Mar 04, 2009 at 11:59:13AM -0800, akpm@linux-foundation.org wrote:
>> > From: Eric Miao <eric.miao@marvell.com>
>> >
>> > Noises can be introduced when LCD signals are being driven, some platforms
>> > provide a signal to assist the synchronization of this sampling procedure.
>> >
>> > Signed-off-by: Eric Miao <eric.miao@marvell.com>
>> > Cc: David Brownell <david-b@pacbell.net>
>> > Cc: Dmitry Torokhov <dtor@mail.ru>
>> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>> > ---
>> >
>> >  drivers/input/touchscreen/ads7846.c |   10 ++++++++++
>> >  include/linux/spi/ads7846.h         |    2 ++
>> >  2 files changed, 12 insertions(+)
>> >
>> > diff -puN drivers/input/touchscreen/ads7846.c~input-ads7846-introduce-
>> platform-specific-way-to-synchronize-sampling
>> drivers/input/touchscreen/ads7846.c
>> > --- a/drivers/input/touchscreen/ads7846.c~input-ads7846-introduce-
>> platform-specific-way-to-synchronize-sampling
>> > +++ a/drivers/input/touchscreen/ads7846.c
>> > @@ -127,6 +127,8 @@ struct ads7846 {
>> >       void                    (*filter_cleanup)(void *data);
>> >       int                     (*get_pendown_state)(void);
>> >       int                     gpio_pendown;
>> > +
>> > +     void                    (*wait_for_sync)(void);
>> >  };
>> >
>> >  /* leave chip selected when we're done, for quicker re-select? */
>> > @@ -511,6 +513,10 @@ static int get_pendown_state(struct ads7
>> >       return !gpio_get_value(ts->gpio_pendown);
>> >  }
>> >
>> > +static void null_wait_for_sync(void)
>> > +{
>> > +}
>> > +
>> >  /*
>> >   * PENIRQ only kicks the timer.  The timer only reissues the SPI transfer,
>> >   * to retrieve touchscreen status.
>> > @@ -686,6 +692,7 @@ static void ads7846_rx_val(void *ads)
>> >       default:
>> >               BUG();
>> >       }
>> > +     ts->wait_for_sync();
>> >       status = spi_async(ts->spi, m);
>> >       if (status)
>> >               dev_err(&ts->spi->dev, "spi_async --> %d\n",
>> > @@ -723,6 +730,7 @@ static enum hrtimer_restart ads7846_time
>> >       } else {
>> >               /* pen is still down, continue with the measurement */
>> >               ts->msg_idx = 0;
>> > +             ts->wait_for_sync();
>>
>> We are in hard IRQ context here. What is the expectation of
>> wait_for_sync()? How long are we waiting here?
>>
>
> This should be a workaround for broken hardware and I don't expect
> this sync time on every boards.
>

The sync time depends on the moment of this sync between two HSYNC,
I don't expect that to be very long, yet the hardware is broken enough to
endure this latency.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 22/22] input: ads7846: introduce platform specific way to synchronize sampling

[David Brownell]

On Thursday 12 March 2009, Eric Miao wrote:
> >
> > This should be a workaround for broken hardware and I don't expect
> > this sync time on every boards.
> >
> 
> The sync time depends on the moment of this sync between two HSYNC,
> I don't expect that to be very long,

But how long is that?  1/(50 KHz) or so?


> yet the hardware is broken enough to 
> endure this latency.

Actually I heard from someone else this morning that it's not just
"broken" hardware that sees touchscreen ADC sampling noise related
to hsync.  So this patch makes sense ... although there's no way to
guarantee the SPI requests won't have queueing delays before they
arrive at the touchscreen controller, possibly hitting the next
hsync interval.

It seems that it's surprisingly tricky to get clean samples from
touchscreens.  There's physical oscillation to compensate, which
changes capacitance until it damps down.  There's also electrical
oscillation from charging the various sense lines.  Add to that a
bunch of honest-to-goodness noise associated with hsync and vsync...
yeech!  It would be a Good Thing if someone were to split out the
sample filtering logic from this driver so it could be used with
other touchscreen drivers.

- Dave

Re: [patch 22/22] input: ads7846: introduce platform specific way to synchronize sampling

[Eric Miao]

On Sat, Mar 14, 2009 at 9:09 AM, David Brownell <david-b@pacbell.net> wrote:
> On Thursday 12 March 2009, Eric Miao wrote:
>> >
>> > This should be a workaround for broken hardware and I don't expect
>> > this sync time on every boards.
>> >
>>
>> The sync time depends on the moment of this sync between two HSYNC,
>> I don't expect that to be very long,
>
> But how long is that?  1/(50 KHz) or so?
>

For Sharp Zaurus, let's say a poodle, with PXA255 and QVGA LCD
portrait, a typical of 320 lines per frame (although counting in blank
VSYNC will make this a bit larger), and 60Hz frame refresh rate:

1 HSYNC = 1/(60Hz * 320) = 52.08us

An average sync time would be half of this, say, 26us.

Having another interrupt to trigger the sampling will be a bit
trickier and considering this average of 26us on a 400MHz
PXA255, and another interrupt latency, I don't see too much
benefit of doing that.

And my personal feeling of the final result with the GUI seems
quite responsive, though I didn't stress test that.

>
>> yet the hardware is broken enough to
>> endure this latency.
>
> Actually I heard from someone else this morning that it's not just
> "broken" hardware that sees touchscreen ADC sampling noise related
> to hsync.  So this patch makes sense ... although there's no way to
> guarantee the SPI requests won't have queueing delays before they
> arrive at the touchscreen controller, possibly hitting the next
> hsync interval.
>
> It seems that it's surprisingly tricky to get clean samples from
> touchscreens.  There's physical oscillation to compensate, which
> changes capacitance until it damps down.  There's also electrical
> oscillation from charging the various sense lines.  Add to that a
> bunch of honest-to-goodness noise associated with hsync and vsync...
> yeech!  It would be a Good Thing if someone were to split out the
> sample filtering logic from this driver so it could be used with
> other touchscreen drivers.
>

Yes, sample filtering can actually be made input device wise.

There are actually some user-land solution to this, e.g. tslib.
Yet this sync issue seems annoyingly unavoidable even with
a fine grained filter, or maybe I didn't try my best to tweak those
filters to a maximum potential yet, and I gave up on that finally.

> - Dave
>
>



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

Re: [patch 22/22] input: ads7846: introduce platform specific way to synchronize sampling

[Mark Brown]

On Sat, Mar 14, 2009 at 11:41:58AM +0800, Eric Miao wrote:
> On Sat, Mar 14, 2009 at 9:09 AM, David Brownell <david-b@pacbell.net> wrote:

> > yeech!  It would be a Good Thing if someone were to split out the
> > sample filtering logic from this driver so it could be used with
> > other touchscreen drivers.

> Yes, sample filtering can actually be made input device wise.

> There are actually some user-land solution to this, e.g. tslib.
> Yet this sync issue seems annoyingly unavoidable even with
> a fine grained filter, or maybe I didn't try my best to tweak those
> filters to a maximum potential yet, and I gave up on that finally.

OpenMoko has a genericised in-kernel filter setup which they're
currently trying to get merged:

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