Re: [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

Re: [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

[David Brownell]

On Friday 15 December 2006 7:20 am, Nicolas FERRE wrote:
> Add support for the ads7843 touchscreen controller to the ads7846 driver code.

Glad to see this!  Is this for AT91sam9261-EK board support, maybe?


> The "pen down" information is managed quite differently as we do not have a
> touch-presure mesurement on the ads7843.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@rfo.atmel.com>
> ---
> 
> I add a ads7843_rx function to manage the end of a measurement cycle. As for
> ads7846_rx, it does the real work and comnunicates with the input subsystem.
> The timer function is responsible of taking the pen up/pen down state through
> the board specific get_pendown_state() callback.

Hmm, unfortunately there are a lot of updates to this driver sitting in
the linux-omap tree ... the N770 guys really put a lot of solid work into
it (ISTR seeing a statistical plot showing how touchscreen debounce helps
get stable readings!), but those patches didn't get upstream yet.

One of the more critical of those patches addressed much the same pendown
state issue; it turns out that measuring pressure (after the pendown irq)
isn't the best way to notice when pen status becomes "up".

Let me try to sort out the mess with those updates, and ask you to refresh
this ads7843 support against that more-current ads7846 code.  (Or if you
want a preview, have a look at the linux-omap tree.  There's a clone of
that in the GIT area on kernel.org if you want.)


> As the SPI underlying code behaves quite differently from a controller driver
> to another whan not having a tx_buf filled, I have add a zerroed buffer to give
> to the spi layer while receiving data.

You must be working with a buggy controller driver then.  That part of
this patch should never be needed.  It's expected that rx-only transfers
will omit a tx buf; all controller drivers must handle that case.

- Dave


> Already posted to lkml :
> http://lkml.org/lkml/2006/12/15/71
> 

Re: [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

[Nicolas Ferre]

David Brownell a écrit :
> On Friday 15 December 2006 7:20 am, Nicolas FERRE wrote:
>> Add support for the ads7843 touchscreen controller to the ads7846 driver code.
> 
> Glad to see this!  Is this for AT91sam9261-EK board support, maybe?

Indeed ! An also for the AT91sam9263-EK which has the same touchscreen chip.

> Let me try to sort out the mess with those updates, and ask you to refresh
> this ads7843 support against that more-current ads7846 code.

Ok, let me know when you have a newer code. I will try to adapt my
ads7843 support then.

>> As the SPI underlying code behaves quite differently from a controller driver
>> to another whan not having a tx_buf filled, I have add a zerroed buffer to give
>> to the spi layer while receiving data.
> 
> You must be working with a buggy controller driver then.  That part of
> this patch should never be needed.  It's expected that rx-only transfers
> will omit a tx buf; all controller drivers must handle that case.

I said that because it is true that most of spi controller drivers 
manage rx only transactions filling the tx buffer with zerros but the 
spi_s3c24xx.c driver seems to fill with ones (line 177 hw_txbyte())

Anyway, I will check in our controller driver to sort this out.

Regards,
-- 
Nicolas Ferre

Re: [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

[Nicolas Ferre]

Nicolas Ferre a écrit :
>>> As the SPI underlying code behaves quite differently from a 
>>> controller driver
>>> to another whan not having a tx_buf filled, I have add a zerroed 
>>> buffer to give
>>> to the spi layer while receiving data.
>>
>> You must be working with a buggy controller driver then.  That part of
>> this patch should never be needed.  It's expected that rx-only transfers
>> will omit a tx buf; all controller drivers must handle that case.
> 
> I said that because it is true that most of spi controller drivers 
> manage rx only transactions filling the tx buffer with zerros but the 
> spi_s3c24xx.c driver seems to fill with ones (line 177 hw_txbyte())
> 
> Anyway, I will check in our controller driver to sort this out.

I dug a bit into this.
Well, in the atmel_spi driver code, we use previous rx buffer if we do 
not provide a tx_buf (as it is said that in struct spi_transfer 
comments,  "If the transmit buffer is null, undefined data will be 
shifted out while filling rx_buf").
So, the touchscreen controller sees sometimes a "start" condition (bit 7 
of a control byte). It then takes the control byte and sets trash bits 
as a configuration. I ran into those troubles and add a zerroed buffer 
as tx.

Do you think that shifting zerros out when a tx_buf is not specified is 
the desired behavior ?

Regards,
-- 
Nicolas Ferre

Re: [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

[David Brownell]

On Thursday 21 December 2006 6:40 am, Nicolas Ferre wrote:
> Nicolas Ferre a écrit :
> >>> As the SPI underlying code behaves quite differently from a 
> >>> controller driver to another whan not having a tx_buf filled,
> >>> I have add a zerroed buffer to give to the spi layer while
> >>> receiving data. 
> >>
> >> You must be working with a buggy controller driver then.  That part of
> >> this patch should never be needed.  It's expected that rx-only transfers
> >> will omit a tx buf; all controller drivers must handle that case.
> > 
> > I said that because it is true that most of spi controller drivers 
> > manage rx only transactions filling the tx buffer with zerros but the 
> > spi_s3c24xx.c driver seems to fill with ones (line 177 hw_txbyte())
> > 
> > Anyway, I will check in our controller driver to sort this out.
> 
> I dug a bit into this.

I always like to see followup on such issues.  :)


> Well, in the atmel_spi driver code, we use previous rx buffer if we do 
> not provide a tx_buf (as it is said that in struct spi_transfer 
> comments,  "If the transmit buffer is null, undefined data will be 
> shifted out while filling rx_buf").

That seems like a reasonable approach.  If it's undefined, the only
reasons I can think of to not use the rx buffer are that:

 (a) the cachelines might not be managed correctly during DMA ...
     i.e. to defend against a bug in the controller driver; and
 (b) that writing such _defined_ data would be a "covert channel"
     in the security sense.

Now, (a) is just a bug to fix, and in most cases (b) won't be an
issue since even if the system with that driver is being evaluated
at a level where such channels matter, the hardware hooked up via
SPI will probably already be trusted.  (Unless the system allows
thing like MMC or SD cards...)  However, see below.


> So, the touchscreen controller sees sometimes a "start" condition (bit 7 
> of a control byte). It then takes the control byte and sets trash bits 
> as a configuration.

Actually, now that I look at it I see that the docs for both the
ads7846 and the ads7843 show that DIN/MOSI should be zero/low
after the command is given, while reading DOUT/MISO.

Which means that the real issue here is that the driver was wrong
in the first place to not explicitly write zeroes while it's reading
back that data.

The testing I've done with it involved two controller drivers which
do happen to interpret "undefined" as "write zeros":  omap_uwire,
which I'm guessing does so in hardware (MicroWire is half duplex),
and pxa2xx_spi, which explicitly writes zeros (null_writer).


> I ran into those troubles and add a zerroed buffer as tx.
> 
> Do you think that shifting zerros out when a tx_buf is not specified is 
> the desired behavior ?

I'm leaning towards a "yes" there -- changing the spec for spi_transfer.

The alternative would seem to mean teaching the SPI interface about
two different kinds of "rx only" transfers, one with "must tx zero"
semantics and the other with "tx data doesn't matter" (and security
audits would define it anyway, to avoid covert channels).  And I can't
easily justify that.

For now, I'd suggest you update the atmel_spi driver so that it shifts
zeroes in that case; can't hurt (too much).  And I'll forward the issue
to the SPI list.  If nobody there objects, I'll send patches to update
the spec for spi_transfer, and the s3c driver.

- Dave

Re: [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

[David Brownell]

On Thursday 21 December 2006 5:08 am, Nicolas Ferre wrote:
 
> > Let me try to sort out the mess with those updates, and ask you to refresh
> > this ads7843 support against that more-current ads7846 code.
> 
> Ok, let me know when you have a newer code. I will try to adapt my
> ads7843 support then.

I just sent these updates to LKML ... though the address I had for Dmitry
sees to have gone bad, I do see all six patches in the list archives.

Now it's your turn.  :)

- Dave

Re: [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

[Dmitry Torokhov]

On 12/22/06, David Brownell <david-b@pacbell.net> wrote:
> On Thursday 21 December 2006 5:08 am, Nicolas Ferre wrote:
>
> > > Let me try to sort out the mess with those updates, and ask you to refresh
> > > this ads7843 support against that more-current ads7846 code.
> >
> > Ok, let me know when you have a newer code. I will try to adapt my
> > ads7843 support then.
>
> I just sent these updates to LKML ... though the address I had for Dmitry
> sees to have gone bad, I do see all six patches in the list archives.
>

I changed my ISP. Please use either:

dmitry.torokhov@gmail.com
dtor@mail.ru
dtor@insightbb.com

The last one may go bad if I ever change ISP again but I intend to
keep the first 2 indefinitely.

I'll fish the patches from LKML.

-- 
Dmitry

[patch 2.6.20-rc1 6/6] input: ads7846 directly senses PENUP state

[David Brownell]

From: imre.deak@solidboot.com <imre.deak@solidboot.com>
Date:  Wed Jul 5 19:18:32 2006 +0300

Input: ads7846: detect pen up from GPIO state

We can't depend on the pressure value to determine when the pen was
lifted, so use the GPIO line state instead.  This also helps with
chips (like ads7843) that don't have pressure sensors.

Signed-off-by: Imre Deak <imre.deak@solidboot.com>
Signed-off-by: Juha Yrjola <juha.yrjola@solidboot.com>
Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>


Index: osk/drivers/input/touchscreen/ads7846.c
===================================================================
--- osk.orig/drivers/input/touchscreen/ads7846.c	2006-12-22 11:08:46.000000000 -0800
+++ osk/drivers/input/touchscreen/ads7846.c	2006-12-22 11:08:47.000000000 -0800
@@ -430,6 +430,39 @@ static struct attribute_group ads7845_at
 
 /*--------------------------------------------------------------------------*/
 
+static void ads7846_report_pen_state(struct ads7846 *ts, int down)
+{
+	struct input_dev	*input_dev = ts->input;
+
+	input_report_key(input_dev, BTN_TOUCH, down);
+	if (!down)
+		input_report_abs(input_dev, ABS_PRESSURE, 0);
+#ifdef VERBOSE
+	pr_debug("%s: %s\n", ts->spi->dev.bus_id, down ? "DOWN" : "UP");
+#endif
+}
+
+static void ads7846_report_pen_position(struct ads7846 *ts, int x, int y,
+					int pressure)
+{
+	struct input_dev	*input_dev = ts->input;
+
+	input_report_abs(input_dev, ABS_X, x);
+	input_report_abs(input_dev, ABS_Y, y);
+	input_report_abs(input_dev, ABS_PRESSURE, pressure);
+
+#ifdef VERBOSE
+	pr_debug("%s: %d/%d/%d\n", ts->spi->dev.bus_id, x, y, pressure);
+#endif
+}
+
+static void ads7846_sync_events(struct ads7846 *ts)
+{
+	struct input_dev	*input_dev = ts->input;
+
+	input_sync(input_dev);
+}
+
 /*
  * PENIRQ only kicks the timer.  The timer only reissues the SPI transfer,
  * to retrieve touchscreen status.
@@ -441,11 +474,8 @@ static struct attribute_group ads7845_at
 static void ads7846_rx(void *ads)
 {
 	struct ads7846		*ts = ads;
-	struct input_dev	*input_dev = ts->input;
 	unsigned		Rt;
-	unsigned		sync = 0;
 	u16			x, y, z1, z2;
-	unsigned long		flags;
 
 	/* ads7846_rx_val() did in-place conversion (including byteswap) from
 	 * on-the-wire format as part of debouncing to get stable readings.
@@ -459,7 +489,7 @@ static void ads7846_rx(void *ads)
 	if (x == MAX_12BIT)
 		x = 0;
 
-	if (likely(x && z1 && !device_suspended(&ts->spi->dev))) {
+	if (likely(x && z1)) {
 		/* compute touch pressure resistance using equation #2 */
 		Rt = z2;
 		Rt -= z1;
@@ -475,52 +505,33 @@ static void ads7846_rx(void *ads)
 	 * once more the measurement
 	 */
 	if (ts->tc.ignore || Rt > ts->pressure_max) {
+#ifdef VERBOSE
+		pr_debug("%s: ignored %d pressure %d\n",
+			ts->spi->dev.bus_id, ts->tc.ignore, Rt);
+#endif
 		hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD),
 			      HRTIMER_REL);
 		return;
 	}
 
-	/* NOTE:  "pendown" is inferred from pressure; we don't rely on
-	 * being able to check nPENIRQ status, or "friendly" trigger modes
-	 * (both-edges is much better than just-falling or low-level).
-	 *
-	 * REVISIT:  some boards may require reading nPENIRQ; it's
-	 * needed on 7843.  and 7845 reads pressure differently...
+	/* NOTE: We can't rely on the pressure to determine the pen down
+	 * state, even this controller has a pressure sensor.  The pressure
+	 * value can fluctuate for quite a while after lifting the pen and
+	 * in some cases may not even settle at the expected value.
 	 *
-	 * REVISIT:  the touchscreen might not be connected; this code
-	 * won't notice that, even if nPENIRQ never fires ...
+	 * The only safe way to check for the pen up condition is in the
+	 * timer by reading the pen signal state (it's a GPIO _and_ IRQ).
 	 */
-	if (!ts->pendown && Rt != 0) {
-		input_report_key(input_dev, BTN_TOUCH, 1);
-		sync = 1;
-	} else if (ts->pendown && Rt == 0) {
-		input_report_key(input_dev, BTN_TOUCH, 0);
-		sync = 1;
-	}
-
 	if (Rt) {
-		input_report_abs(input_dev, ABS_X, x);
-		input_report_abs(input_dev, ABS_Y, y);
-		sync = 1;
-	}
-
-	if (sync) {
-		input_report_abs(input_dev, ABS_PRESSURE, Rt);
-		input_sync(input_dev);
+		if (!ts->pendown) {
+			ads7846_report_pen_state(ts, 1);
+			ts->pendown = 1;
+		}
+		ads7846_report_pen_position(ts, x, y, Rt);
+		ads7846_sync_events(ts);
 	}
 
-#ifdef	VERBOSE
-	if (Rt || ts->pendown)
-		pr_debug("%s: %d/%d/%d%s\n", ts->spi->dev.bus_id,
-			x, y, Rt, Rt ? "" : " UP");
-#endif
-
-	spin_lock_irqsave(&ts->lock, flags);
-
-	ts->pendown = (Rt != 0);
 	hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD), HRTIMER_REL);
-
-	spin_unlock_irqrestore(&ts->lock, flags);
 }
 
 static int ads7846_debounce(void *ads, int data_idx, int *val)
@@ -616,14 +627,20 @@ static int ads7846_timer(struct hrtimer 
 
 	spin_lock_irq(&ts->lock);
 
-	if (unlikely(ts->msg_idx && !ts->pendown)) {
+	if (unlikely(!ts->get_pendown_state()
+			|| device_suspended(&ts->spi->dev))) {
+		if (ts->pendown) {
+			ads7846_report_pen_state(ts, 0);
+			ads7846_sync_events(ts);
+			ts->pendown = 0;
+		}
+
 		/* measurement cycle ended */
 		if (!device_suspended(&ts->spi->dev)) {
 			ts->irq_disabled = 0;
 			enable_irq(ts->spi->irq);
 		}
 		ts->pending = 0;
-		ts->msg_idx = 0;
 	} else {
 		/* pen is still down, continue with the measurement */
 		ts->msg_idx = 0;

Re: [patch 2.6.20-rc1 6/6] input: ads7846 directly senses PENUP state

[David Brownell]

> > > I think these helpers just obfuscate the code, just call
> > > input_report_*() and input_sync() drectly like you used to do.
> > 
> > Fair enough, I had a similar thought.  Imre, could you do that update?
> 
> Yes, the patch is against the OMAP tree.

Thanks ... still hoping that the OMAP tree will just be able
to pull down the kernel.org version of this driver soon!!

OK, here's an updated patch 6/6 that merges Imre's update.

Dmitry, that makes six patches you should have ... I updated
the hwmon patch (#3), and now this one (#6); #4 and #5 will
thus apply with some offsets.

If you don't have other issues, I'd like to see these get into
the 2.6.20 release ... except for that version of the hwmon patch,
everything has been in use for some time through the OMAP tree,
which AFAICT is right now the main user of this driver.  (That'll
change a bit once Atmel's patches merge.)

- Dave

==========	CUT HERE
From: imre.deak@solidboot.com <imre.deak@solidboot.com>
Date: Wed Jul 5 19:18:32 2006 +0300

Input: ads7846: detect pen up from GPIO state

We can't depend on the pressure value to determine when the pen was
lifted, so use the GPIO line state instead.  This also helps with
chips (like ads7843) that don't have pressure sensors.

Signed-off-by: Imre Deak <imre.deak@solidboot.com>
Signed-off-by: Juha Yrjola <juha.yrjola@solidboot.com>
Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>


Index: at91/drivers/input/touchscreen/ads7846.c
===================================================================
--- at91.orig/drivers/input/touchscreen/ads7846.c	2006-12-22 12:13:27.000000000 -0800
+++ at91/drivers/input/touchscreen/ads7846.c	2006-12-28 14:19:51.000000000 -0800
@@ -441,11 +441,8 @@ static struct attribute_group ads7845_at
 static void ads7846_rx(void *ads)
 {
 	struct ads7846		*ts = ads;
-	struct input_dev	*input_dev = ts->input;
 	unsigned		Rt;
-	unsigned		sync = 0;
 	u16			x, y, z1, z2;
-	unsigned long		flags;
 
 	/* ads7846_rx_val() did in-place conversion (including byteswap) from
 	 * on-the-wire format as part of debouncing to get stable readings.
@@ -459,7 +456,7 @@ static void ads7846_rx(void *ads)
 	if (x == MAX_12BIT)
 		x = 0;
 
-	if (likely(x && z1 && !device_suspended(&ts->spi->dev))) {
+	if (likely(x && z1)) {
 		/* compute touch pressure resistance using equation #2 */
 		Rt = z2;
 		Rt -= z1;
@@ -475,52 +472,42 @@ static void ads7846_rx(void *ads)
 	 * once more the measurement
 	 */
 	if (ts->tc.ignore || Rt > ts->pressure_max) {
+#ifdef VERBOSE
+		pr_debug("%s: ignored %d pressure %d\n",
+			ts->spi->dev.bus_id, ts->tc.ignore, Rt);
+#endif
 		hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD),
 			      HRTIMER_REL);
 		return;
 	}
 
-	/* NOTE:  "pendown" is inferred from pressure; we don't rely on
-	 * being able to check nPENIRQ status, or "friendly" trigger modes
-	 * (both-edges is much better than just-falling or low-level).
-	 *
-	 * REVISIT:  some boards may require reading nPENIRQ; it's
-	 * needed on 7843.  and 7845 reads pressure differently...
+	/* NOTE: We can't rely on the pressure to determine the pen down
+	 * state, even this controller has a pressure sensor.  The pressure
+	 * value can fluctuate for quite a while after lifting the pen and
+	 * in some cases may not even settle at the expected value.
 	 *
-	 * REVISIT:  the touchscreen might not be connected; this code
-	 * won't notice that, even if nPENIRQ never fires ...
+	 * The only safe way to check for the pen up condition is in the
+	 * timer by reading the pen signal state (it's a GPIO _and_ IRQ).
 	 */
-	if (!ts->pendown && Rt != 0) {
-		input_report_key(input_dev, BTN_TOUCH, 1);
-		sync = 1;
-	} else if (ts->pendown && Rt == 0) {
-		input_report_key(input_dev, BTN_TOUCH, 0);
-		sync = 1;
-	}
-
 	if (Rt) {
-		input_report_abs(input_dev, ABS_X, x);
-		input_report_abs(input_dev, ABS_Y, y);
-		sync = 1;
-	}
-
-	if (sync) {
-		input_report_abs(input_dev, ABS_PRESSURE, Rt);
-		input_sync(input_dev);
-	}
-
-#ifdef	VERBOSE
-	if (Rt || ts->pendown)
-		pr_debug("%s: %d/%d/%d%s\n", ts->spi->dev.bus_id,
-			x, y, Rt, Rt ? "" : " UP");
+		if (!ts->pendown) {
+			input_report_key(ts->input, BTN_TOUCH, 1);
+			ts->pendown = 1;
+#ifdef VERBOSE
+			dev_dbg(&ts->spi->dev, "DOWN\n");
 #endif
+		}
+		input_report_abs(ts->input, ABS_X, x);
+		input_report_abs(ts->input, ABS_Y, y);
+		input_report_abs(ts->input, ABS_PRESSURE, Rt);
+
+		input_sync(ts->input);
+#ifdef VERBOSE
+		dev_dbg(&ts->spi->dev, "%4d/%4d/%4d\n", x, y, Rt);
+#endif
+	}
 
-	spin_lock_irqsave(&ts->lock, flags);
-
-	ts->pendown = (Rt != 0);
 	hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD), HRTIMER_REL);
-
-	spin_unlock_irqrestore(&ts->lock, flags);
 }
 
 static int ads7846_debounce(void *ads, int data_idx, int *val)
@@ -616,14 +603,24 @@ static int ads7846_timer(struct hrtimer 
 
 	spin_lock_irq(&ts->lock);
 
-	if (unlikely(ts->msg_idx && !ts->pendown)) {
+	if (unlikely(!ts->get_pendown_state()
+			|| device_suspended(&ts->spi->dev))) {
+		if (ts->pendown) {
+			input_report_key(ts->input, BTN_TOUCH, 0);
+			input_report_abs(ts->input, ABS_PRESSURE, 0);
+			input_sync(ts->input);
+			ts->pendown = 0;
+#ifdef VERBOSE
+			dev_dbg(&ts->spi->dev, "UP\n");
+#endif
+		}
+
 		/* measurement cycle ended */
 		if (!device_suspended(&ts->spi->dev)) {
 			ts->irq_disabled = 0;
 			enable_irq(ts->spi->irq);
 		}
 		ts->pending = 0;
-		ts->msg_idx = 0;
 	} else {
 		/* pen is still down, continue with the measurement */
 		ts->msg_idx = 0;

Re: [patch 2.6.20-rc1 6/6] input: ads7846 directly senses PENUP state

[Dmitry Torokhov]

On Thursday 28 December 2006 17:37, David Brownell wrote:
> > > > I think these helpers just obfuscate the code, just call
> > > > input_report_*() and input_sync() drectly like you used to do.
> > > 
> > > Fair enough, I had a similar thought.  Imre, could you do that update?
> > 
> > Yes, the patch is against the OMAP tree.
> 
> Thanks ... still hoping that the OMAP tree will just be able
> to pull down the kernel.org version of this driver soon!!
> 
> OK, here's an updated patch 6/6 that merges Imre's update.
> 
> Dmitry, that makes six patches you should have ... I updated
> the hwmon patch (#3), and now this one (#6); #4 and #5 will
> thus apply with some offsets.
> 

David,

I appied all patches except for hwmon as it had some issues with CONFIG_HWMON
handling. Could you please take a look at the patch below and tell me if it
works for you?

-- 
Dmitry

From: David Brownell <dbrownell@users.sourceforge.net>

Input: ads7846 - be more compatible with the hwmon framework

 - Hook up to hwmon
     * show sensor attributes only if hwmon is present
     * ... and the board's reference voltage is known
     * otherwise be just a touchscreen
 - Report voltages per hwmon convention
     * measure in millivolts
     * voltages are named in[0-8]_input (ugh)
     * for 7846 chips, properly range-adjust vBATT/in1_input

Battery measurements help during recharge monitoring.  On OSK/Mistral,
the measured voltage agreed with a multimeter to several decimal places.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/touchscreen/Kconfig   |    9 -
 drivers/input/touchscreen/ads7846.c |  306 +++++++++++++++++++++++++-----------
 2 files changed, 224 insertions(+), 91 deletions(-)

Index: work/drivers/input/touchscreen/Kconfig
===================================================================
--- work.orig/drivers/input/touchscreen/Kconfig
+++ work/drivers/input/touchscreen/Kconfig
@@ -12,13 +12,18 @@ menuconfig INPUT_TOUCHSCREEN
 if INPUT_TOUCHSCREEN
 
 config TOUCHSCREEN_ADS7846
-	tristate "ADS 7846 based touchscreens"
+	tristate "ADS 7846/7843 based touchscreens"
 	depends on SPI_MASTER
+	depends on HWMON = n || HWMON
 	help
 	  Say Y here if you have a touchscreen interface using the
-	  ADS7846 controller, and your board-specific initialization
+	  ADS7846 or ADS7843 controller, and your board-specific setup
 	  code includes that in its table of SPI devices.
 
+	  If HWMON is selected, and the driver is told the reference voltage
+	  on your board, you will also get hwmon interfaces for the voltage
+	  (and on ads7846, temperature) sensors of this chip.
+
 	  If unsure, say N (but it's safe to say "Y").
 
 	  To compile this driver as a module, choose M here: the
Index: work/drivers/input/touchscreen/ads7846.c
===================================================================
--- work.orig/drivers/input/touchscreen/ads7846.c
+++ work/drivers/input/touchscreen/ads7846.c
@@ -17,8 +17,9 @@
  *  it under the terms of the GNU General Public License version 2 as
  *  published by the Free Software Foundation.
  */
-#include <linux/device.h>
+#include <linux/hwmon.h>
 #include <linux/init.h>
+#include <linux/err.h>
 #include <linux/delay.h>
 #include <linux/input.h>
 #include <linux/interrupt.h>
@@ -69,7 +70,7 @@ struct ts_event {
 	u16	x;
 	u16	y;
 	u16	z1, z2;
-	int    ignore;
+	int	ignore;
 };
 
 struct ads7846 {
@@ -77,7 +78,12 @@ struct ads7846 {
 	char			phys[32];
 
 	struct spi_device	*spi;
+
+#if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE))
 	struct attribute_group	*attr_group;
+	struct class_device	*hwmon;
+#endif
+
 	u16			model;
 	u16			vref_delay_usecs;
 	u16			x_plate_ohms;
@@ -170,7 +176,12 @@ struct ads7846 {
 
 /*
  * Non-touchscreen sensors only use single-ended conversions.
+ * The range is GND..vREF. The ads7843 and ads7835 must use external vREF;
+ * ads7846 lets that pin be unconnected, to use internal vREF.
  */
+static unsigned vREF_mV;
+module_param(vREF_mV, uint, 0);
+MODULE_PARM_DESC(vREF_mV, "external vREF voltage, in milliVolts");
 
 struct ser_req {
 	u8			ref_on;
@@ -198,50 +209,55 @@ static int ads7846_read12_ser(struct dev
 	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
 	int			status;
 	int			sample;
-	int			i;
+	int			use_internal;
 
 	if (!req)
 		return -ENOMEM;
 
 	spi_message_init(&req->msg);
 
-	/* activate reference, so it has time to settle; */
-	req->ref_on = REF_ON;
-	req->xfer[0].tx_buf = &req->ref_on;
-	req->xfer[0].len = 1;
-	req->xfer[1].rx_buf = &req->scratch;
-	req->xfer[1].len = 2;
-
-	/*
-	 * for external VREF, 0 usec (and assume it's always on);
-	 * for 1uF, use 800 usec;
-	 * no cap, 100 usec.
-	 */
-	req->xfer[1].delay_usecs = ts->vref_delay_usecs;
+	/* FIXME boards with ads7846 might use external vref instead ... */
+	use_internal = (ts->model == 7846);
+
+	/* maybe turn on internal vREF, and let it settle */
+	if (use_internal) {
+		req->ref_on = REF_ON;
+		req->xfer[0].tx_buf = &req->ref_on;
+		req->xfer[0].len = 1;
+		spi_message_add_tail(&req->xfer[0], &req->msg);
+
+		req->xfer[1].rx_buf = &req->scratch;
+		req->xfer[1].len = 2;
+
+		/* for 1uF, settle for 800 usec; no cap, 100 usec.  */
+		req->xfer[1].delay_usecs = ts->vref_delay_usecs;
+		spi_message_add_tail(&req->xfer[1], &req->msg);
+	}
 
 	/* take sample */
 	req->command = (u8) command;
 	req->xfer[2].tx_buf = &req->command;
 	req->xfer[2].len = 1;
+	spi_message_add_tail(&req->xfer[2], &req->msg);
+
 	req->xfer[3].rx_buf = &req->sample;
 	req->xfer[3].len = 2;
+	spi_message_add_tail(&req->xfer[3], &req->msg);
 
 	/* REVISIT:  take a few more samples, and compare ... */
 
-	/* turn off reference */
-	req->ref_off = REF_OFF;
-	req->xfer[4].tx_buf = &req->ref_off;
-	req->xfer[4].len = 1;
-	req->xfer[5].rx_buf = &req->scratch;
-	req->xfer[5].len = 2;
-
-	CS_CHANGE(req->xfer[5]);
-
-	/* group all the transfers together, so we can't interfere with
-	 * reading touchscreen state; disable penirq while sampling
-	 */
-	for (i = 0; i < 6; i++)
-		spi_message_add_tail(&req->xfer[i], &req->msg);
+	/* maybe off internal vREF */
+	if (use_internal) {
+		req->ref_off = REF_OFF;
+		req->xfer[4].tx_buf = &req->ref_off;
+		req->xfer[4].len = 1;
+		spi_message_add_tail(&req->xfer[4], &req->msg);
+
+		req->xfer[5].rx_buf = &req->scratch;
+		req->xfer[5].len = 2;
+		CS_CHANGE(req->xfer[5]);
+		spi_message_add_tail(&req->xfer[5], &req->msg);
+	}
 
 	ts->irq_disabled = 1;
 	disable_irq(spi->irq);
@@ -261,25 +277,173 @@ static int ads7846_read12_ser(struct dev
 	return status ? status : sample;
 }
 
-#define SHOW(name) static ssize_t \
+#if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE))
+
+#define SHOW(name, var, adjust) static ssize_t \
 name ## _show(struct device *dev, struct device_attribute *attr, char *buf) \
 { \
+	struct ads7846 *ts = dev_get_drvdata(dev); \
 	ssize_t v = ads7846_read12_ser(dev, \
-			READ_12BIT_SER(name) | ADS_PD10_ALL_ON); \
+			READ_12BIT_SER(var) | ADS_PD10_ALL_ON); \
 	if (v < 0) \
 		return v; \
-	return sprintf(buf, "%u\n", (unsigned) v); \
+	return sprintf(buf, "%u\n", adjust(ts, v)); \
 } \
 static DEVICE_ATTR(name, S_IRUGO, name ## _show, NULL);
 
-SHOW(temp0)
-SHOW(temp1)
-SHOW(vaux)
-SHOW(vbatt)
+
+/* Sysfs conventions report temperatures in millidegrees Celcius.
+ * ADS7846 could use the low-accuracy two-sample scheme, but can't do the high
+ * accuracy scheme without calibration data.  For now we won't try either;
+ * userspace sees raw sensor values, and must scale/calibrate appropriately.
+ */
+static inline unsigned null_adjust(struct ads7846 *ts, ssize_t v)
+{
+	return v;
+}
+
+SHOW(temp0, temp0, null_adjust)		/* temp1_input */
+SHOW(temp1, temp1, null_adjust)		/* temp2_input */
+
+
+/* sysfs conventions report voltages in millivolts.  We can convert voltages
+ * if we know vREF.  userspace may need to scale vAUX to match the board's
+ * external resistors; we assume that vBATT only uses the internal ones.
+ */
+static inline unsigned vaux_adjust(struct ads7846 *ts, ssize_t v)
+{
+	unsigned retval = v;
+
+	/* external resistors may scale vAUX into 0..vREF */
+	retval *= vREF_mV;
+	retval = retval >> 12;
+	return retval;
+}
+
+static inline unsigned vbatt_adjust(struct ads7846 *ts, ssize_t v)
+{
+	unsigned retval = vaux_adjust(ts, v);
+
+	/* ads7846 has a resistor ladder to scale this signal down */
+	if (ts->model == 7846)
+		retval *= 4;
+	return retval;
+}
+
+SHOW(in0_input, vaux, vaux_adjust)
+SHOW(in1_input, vbatt, vbatt_adjust)
+
+
+static struct attribute *ads7846_attributes[] = {
+	&dev_attr_temp0.attr,
+	&dev_attr_temp1.attr,
+	&dev_attr_in0_input.attr,
+	&dev_attr_in1_input.attr,
+	NULL,
+};
+
+static struct attribute_group ads7846_attr_group = {
+	.attrs = ads7846_attributes,
+};
+
+static struct attribute *ads7843_attributes[] = {
+	&dev_attr_in0_input.attr,
+	&dev_attr_in1_input.attr,
+	NULL,
+};
+
+static struct attribute_group ads7843_attr_group = {
+	.attrs = ads7843_attributes,
+};
+
+static struct attribute *ads7845_attributes[] = {
+	&dev_attr_in0_input.attr,
+	NULL,
+};
+
+static struct attribute_group ads7845_attr_group = {
+	.attrs = ads7845_attributes,
+};
+
+static int ads784x_hwmon_register(struct spi_device *spi, struct ads7846 *ts)
+{
+	struct class_device *hwmon;
+	int err;
+
+	/* hwmon sensors need a reference voltage */
+	switch (ts->model) {
+	case 7846:
+		if (!vREF_mV) {
+			dev_dbg(&spi->dev, "assuming 2.5V internal vREF\n");
+			vREF_mV = 2500;
+		}
+		break;
+	case 7845:
+	case 7843:
+		if (!vREF_mV) {
+			dev_warn(&spi->dev,
+				"external vREF for ADS%d not specified\n",
+				ts->model);
+			return 0;
+		}
+		break;
+	}
+
+	/* different chips have different sensor groups */
+	switch (ts->model) {
+	case 7846:
+		ts->attr_group = &ads7846_attr_group;
+		break;
+	case 7845:
+		ts->attr_group = &ads7845_attr_group;
+		break;
+	case 7843:
+		ts->attr_group = &ads7843_attr_group;
+		break;
+	default:
+		dev_dbg(&spi->dev, "ADS%d not recognized\n", ts->model);
+		return 0;
+	}
+
+	err = sysfs_create_group(&spi->dev.kobj, ts->attr_group);
+	if (err)
+		return err;
+
+	hwmon = hwmon_device_register(&spi->dev);
+	if (IS_ERR(hwmon)) {
+		sysfs_remove_group(&spi->dev.kobj, ts->attr_group);
+		return PTR_ERR(hwmon);
+	}
+
+	ts->hwmon = hwmon;
+	return 0;
+}
+
+static void ads784x_hwmon_unregister(struct spi_device *spi,
+				     struct ads7846 *ts)
+{
+	if (ts->hwmon) {
+		sysfs_remove_group(&spi->dev.kobj, ts->attr_group);
+		hwmon_device_unregister(ts->hwmon);
+	}
+}
+
+#else
+static inline int ads784x_hwmon_register(struct spi_device *spi,
+					 struct ads7846 *ts)
+{
+	return 0;
+}
+
+static inline void ads784x_hwmon_unregister(struct spi_device *spi,
+					    struct ads7846 *ts)
+{
+}
+#endif
 
 static int is_pen_down(struct device *dev)
 {
-	struct ads7846		*ts = dev_get_drvdata(dev);
+	struct ads7846	*ts = dev_get_drvdata(dev);
 
 	return ts->pendown;
 }
@@ -323,46 +487,14 @@ static ssize_t ads7846_disable_store(str
 
 static DEVICE_ATTR(disable, 0664, ads7846_disable_show, ads7846_disable_store);
 
-static struct attribute *ads7846_attributes[] = {
-	&dev_attr_temp0.attr,
-	&dev_attr_temp1.attr,
-	&dev_attr_vbatt.attr,
-	&dev_attr_vaux.attr,
+static struct attribute *ads784x_attributes[] = {
 	&dev_attr_pen_down.attr,
 	&dev_attr_disable.attr,
 	NULL,
 };
 
-static struct attribute_group ads7846_attr_group => > > I think these helpers just obfuscate the code, just call
> > > input_report_*() and input_sync() drectly like you used to do.
> > 
> > Fair enough, I had a similar thought.  Imre, could you do that update?
> 
> Yes, the patch is against the OMAP tree.

Thanks ... still hoping that the OMAP tree will just be able
to pull down the kernel.org version of this driver soon!!

OK, here's an updated patch 6/6 that merges Imre's update.

Dmitry, that makes six patches you should have ... I updated
the hwmon patch (#3), and now this one (#6); #4 and #5 will
thus apply with some offsets.
 {
-	.attrs = ads7846_attributes,
-};
-
-/*
- * ads7843/7845 don't have temperature sensors, and
- * use the other sensors a bit differently too
- */
-
-static struct attribute *ads7843_attributes[] = {
-	&dev_attr_vbatt.attr,
-	&dev_attr_vaux.attr,
-	&dev_attr_pen_down.attr,
-	&dev_attr_disable.attr,
-	NULL,
-};
-
-static struct attribute_group ads7843_attr_group = {
-	.attrs = ads7843_attributes,
-};
-
-static struct attribute *ads7845_attributes[] = {
-	&dev_attr_vaux.attr,
-	&dev_attr_pen_down.attr,
-	&dev_attr_disable.attr,
-	NULL,
-};
-
-static struct attribute_group ads7845_attr_group = {
-	.attrs = ads7845_attributes,
+static struct attribute_group ads784x_attr_group = {
+	.attrs = ads784x_attributes,
 };
 
 /*--------------------------------------------------------------------------*/
@@ -886,28 +1018,21 @@ static int __devinit ads7846_probe(struc
 		goto err_cleanup_filter;
 	}
 
+	err = ads784x_hwmon_register(spi, ts);
+	if (err)
+		goto err_free_irq;
+
 	dev_info(&spi->dev, "touchscreen, irq %d\n", spi->irq);
 
-	/* take a first sample, leaving nPENIRQ active; avoid
+	/* take a first sample, leaving nPENIRQ active and vREF off; avoid
 	 * the touchscreen, in case it's not connected.
 	 */
 	(void) ads7846_read12_ser(&spi->dev,
 			  READ_12BIT_SER(vaux) | ADS_PD10_ALL_ON);
 
-	switch (ts->model) {
-	case 7846:
-		ts->attr_group = &ads7846_attr_group;
-		break;
-	case 7845:
-		ts->attr_group = &ads7845_attr_group;
-		break;
-	default:
-		ts->attr_group = &ads7843_attr_group;
-		break;
-	}
-	err = sysfs_create_group(&spi->dev.kobj, ts->attr_group);
+	err = sysfs_create_group(&spi->dev.kobj, &ads784x_attr_group);
 	if (err)
-		goto err_free_irq;
+		goto err_remove_hwmon;
 
 	err = input_register_device(input_dev);
 	if (err)
@@ -916,7 +1041,9 @@ static int __devinit ads7846_probe(struc
 	return 0;
 
  err_remove_attr_group:
-	sysfs_remove_group(&spi->dev.kobj, ts->attr_group);
+	sysfs_remove_group(&spi->dev.kobj, &ads784x_attr_group);
+ err_remove_hwmon:
+	ads784x_hwmon_unregister(spi, ts);
  err_free_irq:
 	free_irq(spi->irq, ts);
  err_cleanup_filter:
@@ -932,11 +1059,12 @@ static int __devexit ads7846_remove(stru
 {
 	struct ads7846		*ts = dev_get_drvdata(&spi->dev);
 
+	ads784x_hwmon_unregister(spi, ts);
 	input_unregister_device(ts->input);
 
 	ads7846_suspend(spi, PMSG_SUSPEND);
 
-	sysfs_remove_group(&spi->dev.kobj, ts->attr_group);
+	sysfs_remove_group(&spi->dev.kobj, &ads784x_attr_group);
 
 	free_irq(ts->spi->irq, ts);
 	/* suspend left the IRQ disabled */

Re: [patch 2.6.20-rc1 6/6] input: ads7846 directly senses PENUP state

[David Brownell]

On Thursday 28 December 2006 10:22 pm, Dmitry Torokhov wrote:
> 
> I appied all patches except for hwmon as it had some issues with CONFIG_HWMON
> handling. Could you please take a look at the patch below and tell me if it
> works for you?

Looked OK, except:

> +#if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE))

That idiom is more usually written

	#if defined(CONFIG_HWMON) || defined(CONFIG_HWMON_MODULE)

Thanks!  I'll be glad to see fewer versions of this driver floating around.
And to see the next version of the ads7843 patches ... :) 

- Dave

[PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

[Nicolas Ferre]

David Brownell :
[..]
> Thanks!  I'll be glad to see fewer versions of this driver floating around.
> And to see the next version of the ads7843 patches ... :) 

Hi,

Here is the ads7843 support for the ads7846 touchscreen driver. It is 
very little and takes great advantage of the previous rework.

Tested on Atmel at91sam926[13]ek board with atmel_spi underlying driver.

I also put a use case based on the at91sam9263ek init code. This code is 
just sent as an example, I will send those init patches trough AT91 
maintainer.

Index: linux-2.6.20-at91/drivers/input/touchscreen/ads7846.c
===================================================================
--- linux-2.6.20-at91.orig/drivers/input/touchscreen/ads7846.c
+++ linux-2.6.20-at91/drivers/input/touchscreen/ads7846.c
@@ -39,7 +39,8 @@
 /*
  * This code has been heavily tested on a Nokia 770, and lightly
  * tested on other ads7846 devices (OSK/Mistral, Lubbock).
- * Support for ads7843 and ads7845 has only been stubbed in.
+ * Support for ads7843 tested on Atmel at91sam926x-EK.
+ * Support for ads7845 has only been stubbed in.
  *
  * IRQ handling needs a workaround because of a shortcoming in handling
  * edge triggered IRQs on some platforms like the OMAP1/2. These
@@ -246,18 +247,15 @@ static int ads7846_read12_ser(struct dev
 
 	/* REVISIT:  take a few more samples, and compare ... */
 
-	/* maybe off internal vREF */
-	if (use_internal) {
-		req->ref_off = REF_OFF;
-		req->xfer[4].tx_buf = &req->ref_off;
-		req->xfer[4].len = 1;
-		spi_message_add_tail(&req->xfer[4], &req->msg);
-
-		req->xfer[5].rx_buf = &req->scratch;
-		req->xfer[5].len = 2;
-		CS_CHANGE(req->xfer[5]);
-		spi_message_add_tail(&req->xfer[5], &req->msg);
-	}
+	req->ref_off = REF_OFF;
+	req->xfer[4].tx_buf = &req->ref_off;
+	req->xfer[4].len = 1;
+	spi_message_add_tail(&req->xfer[4], &req->msg);
+
+	req->xfer[5].rx_buf = &req->scratch;
+	req->xfer[5].len = 2;
+	CS_CHANGE(req->xfer[5]);
+	spi_message_add_tail(&req->xfer[5], &req->msg);
 
 	ts->irq_disabled = 1;
 	disable_irq(spi->irq);
@@ -536,6 +534,10 @@ static void ads7846_rx(void *ads)
 	} else
 		Rt = 0;
 
+	if (ts->model == 7843)
+		Rt = ts->pressure_max / 2;
+
+
 	/* Sample found inconsistent by debouncing or pressure is beyond
 	 * the maximum. Don't report it to user space, repeat at least
 	 * once more the measurement


Index: linux-2.6.20-at91/arch/arm/mach-at91rm9200/board-sam9263ek.c
===================================================================
--- linux-2.6.20-at91.orig/arch/arm/mach-at91rm9200/board-sam9263ek.c
+++ linux-2.6.20-at91/arch/arm/mach-at91rm9200/board-sam9263ek.c
@@ -25,6 +25,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/spi/spi.h>
+#include <linux/spi/ads7846.h>
 
 #include <asm/hardware.h>
 #include <asm/setup.h>
@@ -84,6 +85,40 @@ static struct at91_udc_data __initdata e
 	.pullup_pin	= 0,		/* pull-up driven by UDC */
 };
 
+/*
+ * Touchscreen ads7843
+ */
+#if defined(CONFIG_TOUCHSCREEN_ADS7846) || defined(CONFIG_TOUCHSCREEN_ADS7846_MODULE)
+
+int ads7843_pendown_state(void)
+{
+	return !at91_get_gpio_value(AT91_PIN_PA15);
+}
+
+static struct ads7846_platform_data ads_info = {
+	.model			= 7843,
+	.x_min	= 150,	.x_max	= 3830,
+	.y_min	= 190,	.y_max	= 3830,
+	.vref_delay_usecs	= 100,
+	.x_plate_ohms		= 450,
+	.y_plate_ohms		= 250,
+	.pressure_max		= 15000,
+	.debounce_max		= 1,
+	.debounce_rep		= 0,
+	.debounce_tol		= (~0),
+	.get_pendown_state	= ads7843_pendown_state,
+};
+
+void __init at91_add_device_ts(void)
+{
+	/* Configure Interrupt 1 as external IRQ, with pullup */
+	at91_set_B_periph(AT91_PIN_PA15, 1);		/* IRQ1 */
+	/* ts busy */
+	at91_set_gpio_input(AT91_PIN_PA31, 1);
+}
+#else
+void __init at91_add_device_ts(void) {}
+#endif
 
 /*
  * SPI devices.
@@ -97,6 +132,17 @@ static struct spi_board_info ek_spi_devi
 		.bus_num	= 0,
 	},
 #endif
+#if defined(CONFIG_TOUCHSCREEN_ADS7846) || defined(CONFIG_TOUCHSCREEN_ADS7846_MODULE)
+	{
+		.modalias	= "ads7846",
+		.chip_select	= 3,
+		.max_speed_hz	= 125000	/* max sample rate at 3V */
+					* 26,	/* command + data + overhead */
+		.bus_num	= 0,
+		.platform_data	= &ads_info,
+		.irq		= AT91SAM9263_ID_IRQ1,
+	},
+#endif
 };
 
 
@@ -164,6 +210,8 @@ static void __init ek_board_init(void)
 	at91_add_device_mmc(1, &ek_mmc_data);
 	/* NAND */
 	at91_add_device_nand(&ek_nand_data);
+	/* Touchscreen */
+	at91_add_device_ts();
 }
 
 MACHINE_START(AT91SAM9263EK, "Atmel AT91SAM9263-EK")

Re: [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

[David Brownell]

On Friday 16 February 2007 9:37 am, Nicolas Ferre wrote:
> David Brownell :
> [..]
> > Thanks!  I'll be glad to see fewer versions of this driver floating around.
> > And to see the next version of the ads7843 patches ... :) 
> 
> Hi,
> 
> Here is the ads7843 support for the ads7846 touchscreen driver. It is 
> very little and takes great advantage of the previous rework.

Good!  But see below for one fix ... after that, please split out the
ads7846 update by itself, and submit that patch to the input subsystem
maintainer with your signed-off-by.

Did you find out what was causing the jerky behavior (draw a circle,
it looks like a square) before?


> Tested on Atmel at91sam926[13]ek board with atmel_spi underlying driver.

Which has now been merged; minor fixes needed, including a build
fix for AT91.  I expect those will merge by the time 2.6.21 is done.
Looks like that driver is going to get a fair amount of use.  :)

 
> I also put a use case based on the at91sam9263ek init code. This code is 
> just sent as an example, I will send those init patches trough AT91 
> maintainer.

Yeah, support for that board hasn't made it to kernel.org yet.
I hope you'll also update Atmel's other EK boards using this chip.


> Index: linux-2.6.20-at91/drivers/input/touchscreen/ads7846.c
> ===================================================================
> --- linux-2.6.20-at91.orig/drivers/input/touchscreen/ads7846.c
> +++ linux-2.6.20-at91/drivers/input/touchscreen/ads7846.c
> @@ -39,7 +39,8 @@
>  /*
>   * This code has been heavily tested on a Nokia 770, and lightly
>   * tested on other ads7846 devices (OSK/Mistral, Lubbock).
> - * Support for ads7843 and ads7845 has only been stubbed in.
> + * Support for ads7843 tested on Atmel at91sam926x-EK.
> + * Support for ads7845 has only been stubbed in.
>   *
>   * IRQ handling needs a workaround because of a shortcoming in handling
>   * edge triggered IRQs on some platforms like the OMAP1/2. These
> @@ -246,18 +247,15 @@ static int ads7846_read12_ser(struct dev
>  
>  	/* REVISIT:  take a few more samples, and compare ... */
>  
> -	/* maybe off internal vREF */
> -	if (use_internal) {

This part doesn't make sense.  Could you say what you're trying
to do?  The ads7846 requres an external vREF.  Is the issue that
maybe the REF_OFF command isn't just turning off the (non-existent)
internal voltage reference?  If so the comments need updating, and
maybe the name REF_OFF needs to change too...


> -		req->ref_off = REF_OFF;
> -		req->xfer[4].tx_buf = &req->ref_off;
> -		req->xfer[4].len = 1;
> -		spi_message_add_tail(&req->xfer[4], &req->msg);
> -
> -		req->xfer[5].rx_buf = &req->scratch;
> -		req->xfer[5].len = 2;
> -		CS_CHANGE(req->xfer[5]);
> -		spi_message_add_tail(&req->xfer[5], &req->msg);
> -	}
> +	req->ref_off = REF_OFF;
> +	req->xfer[4].tx_buf = &req->ref_off;
> +	req->xfer[4].len = 1;
> +	spi_message_add_tail(&req->xfer[4], &req->msg);
> +
> +	req->xfer[5].rx_buf = &req->scratch;
> +	req->xfer[5].len = 2;
> +	CS_CHANGE(req->xfer[5]);
> +	spi_message_add_tail(&req->xfer[5], &req->msg);
>  
>  	ts->irq_disabled = 1;
>  	disable_irq(spi->irq);
> @@ -536,6 +534,10 @@ static void ads7846_rx(void *ads)
>  	} else
>  		Rt = 0;
>  
> +	if (ts->model == 7843)
> +		Rt = ts->pressure_max / 2;
> +
> +

Heh.  Other than turning whatever-that-is off, this looks like the only
substantive change needed.  You're right, this isn't much at all ... the
ads7843 support has now become almost trivial!


>  	/* Sample found inconsistent by debouncing or pressure is beyond
>  	 * the maximum. Don't report it to user space, repeat at least
>  	 * once more the measurement
> 
> 

Re: [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

[Nicolas Ferre]

David Brownell :
> Did you find out what was causing the jerky behavior (draw a circle,
> it looks like a square) before?

No, my previous rework based on your patches (2.6.19) had this
jerky behavior. I was obliged to go back to old style timers.
On this build, with the htimer framework, it seems ok.
  
>> -	/* maybe off internal vREF */
>> -	if (use_internal) {
> 
> This part doesn't make sense.  Could you say what you're trying
> to do?  The ads7846 requres an external vREF.  Is the issue that
> maybe the REF_OFF command isn't just turning off the (non-existent)
> internal voltage reference?  If so the comments need updating, and
> maybe the name REF_OFF needs to change too...

Well, on the ads7843, this command allow to put converter in low power
mode and, more important, enable the PENIRQ (otherwise disabled).

The REF_OFF command can be replaced by the PWRDOWN one (exactly the
same command). This last name matches better the ads7843
datasheet description (and seems ok considering the ads7846 one).

>> -		req->ref_off = REF_OFF;
>> -		req->xfer[4].tx_buf = &req->ref_off;
>> -		req->xfer[4].len = 1;
>> -		spi_message_add_tail(&req->xfer[4], &req->msg);
>> -
>> -		req->xfer[5].rx_buf = &req->scratch;
>> -		req->xfer[5].len = 2;
>> -		CS_CHANGE(req->xfer[5]);
>> -		spi_message_add_tail(&req->xfer[5], &req->msg);
>> -	}
>> +	req->ref_off = REF_OFF;
>> +	req->xfer[4].tx_buf = &req->ref_off;
>> +	req->xfer[4].len = 1;
>> +	spi_message_add_tail(&req->xfer[4], &req->msg);
>> +
>> +	req->xfer[5].rx_buf = &req->scratch;
>> +	req->xfer[5].len = 2;
>> +	CS_CHANGE(req->xfer[5]);
>> +	spi_message_add_tail(&req->xfer[5], &req->msg);
>>  
>>  	ts->irq_disabled = 1;
>>  	disable_irq(spi->irq);

Regards,
-- 
Nicolas Ferre

Re: [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

[David Brownell]

On Monday 19 February 2007 4:48 am, Nicolas Ferre wrote:
>   
> >> -	/* maybe off internal vREF */
> >> -	if (use_internal) {
> > 
> > This part doesn't make sense.  Could you say what you're trying
> > to do?  The ads7846 requres an external vREF.  Is the issue that
> > maybe the REF_OFF command isn't just turning off the (non-existent)
> > internal voltage reference?  If so the comments need updating, and
> > maybe the name REF_OFF needs to change too...
> 
> Well, on the ads7843, this command allow to put converter in low power
> mode and, more important, enable the PENIRQ (otherwise disabled).
> 
> The REF_OFF command can be replaced by the PWRDOWN one (exactly the
> same command). This last name matches better the ads7843
> datasheet description (and seems ok considering the ads7846 one).

OK, that would be a better change then, along with comment update.

- Dave

[PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

[Nicolas Ferre]

Add support for the ads7843 touchscreen controller to the ads7846
driver code.

The ads7843 support has now become almost trivial since the last
rework.

Signed-off-by: Nicolas Ferre <nicolas.ferre@rfo.atmel.com> 
---
Index: linux-2.6.20-at91/drivers/input/touchscreen/ads7846.c
===================================================================
--- linux-2.6.20-at91.orig/drivers/input/touchscreen/ads7846.c
+++ linux-2.6.20-at91/drivers/input/touchscreen/ads7846.c
@@ -39,7 +39,8 @@
 /*
  * This code has been heavily tested on a Nokia 770, and lightly
  * tested on other ads7846 devices (OSK/Mistral, Lubbock).
- * Support for ads7843 and ads7845 has only been stubbed in.
+ * Support for ads7843 tested on Atmel at91sam926x-EK.
+ * Support for ads7845 has only been stubbed in.
  *
  * IRQ handling needs a workaround because of a shortcoming in handling
  * edge triggered IRQs on some platforms like the OMAP1/2. These
@@ -246,18 +247,16 @@ static int ads7846_read12_ser(struct dev
 
 	/* REVISIT:  take a few more samples, and compare ... */
 
-	/* maybe off internal vREF */
-	if (use_internal) {
-		req->ref_off = REF_OFF;
-		req->xfer[4].tx_buf = &req->ref_off;
-		req->xfer[4].len = 1;
-		spi_message_add_tail(&req->xfer[4], &req->msg);
-
-		req->xfer[5].rx_buf = &req->scratch;
-		req->xfer[5].len = 2;
-		CS_CHANGE(req->xfer[5]);
-		spi_message_add_tail(&req->xfer[5], &req->msg);
-	}
+	/* converter in low power mode & enable PENIRQ */
+	req->ref_off = PWRDOWN;
+	req->xfer[4].tx_buf = &req->ref_off;
+	req->xfer[4].len = 1;
+	spi_message_add_tail(&req->xfer[4], &req->msg);
+
+	req->xfer[5].rx_buf = &req->scratch;
+	req->xfer[5].len = 2;
+	CS_CHANGE(req->xfer[5]);
+	spi_message_add_tail(&req->xfer[5], &req->msg);
 
 	ts->irq_disabled = 1;
 	disable_irq(spi->irq);
@@ -536,6 +535,10 @@ static void ads7846_rx(void *ads)
 	} else
 		Rt = 0;
 
+	if (ts->model == 7843)
+		Rt = ts->pressure_max / 2;
+
+
 	/* Sample found inconsistent by debouncing or pressure is beyond
 	 * the maximum. Don't report it to user space, repeat at least
 	 * once more the measurement

Re: [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

[Dmitry Torokhov]

On Tuesday 20 February 2007 04:19, Nicolas Ferre wrote:
> Add support for the ads7843 touchscreen controller to the ads7846
> driver code.

Applied to the input tree, thank you.

-- 
Dmitry

[PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

[Nicolas FERRE]

Add support for the ads7843 touchscreen controller to the ads7846
driver code.
The "pen down" information is managed quite differently as we
do not have a touch-pressure measurement on the ads7843.

Signed-off-by: Nicolas Ferre <nicolas.ferre@rfo.atmel.com>
---

I add a ads7843_rx function to manage the end of a measurement cycle.
As for ads7846_rx, it does the real work and communicates with the
input subsystem.
The timer function is responsible of taking the pen up/pen down state
through the board specific get_pendown_state() callback.

As the SPI underlying code behaves quite differently from a controller
driver to another while not having a tx_buf filled, I have add a
zeroed buffer to give to the SPI layer while receiving data.

===================================================================
--- a/input/touchscreen/ads7846.c	(.../linux-2.6.19-at91/drivers)	(revision 634)
+++ b/input/touchscreen/ads7846.c	(.../linux-2.6.19-atmel-devel/drivers)	(revision 634)
@@ -4,6 +4,7 @@
  * Copyright (c) 2005 David Brownell
  * Copyright (c) 2006 Nokia Corporation
  * Various changes: Imre Deak <imre.deak@nokia.com>
+ * Ads7843 support: Atmel, Nicolas Ferre <nicolas.ferre@rfo.atmel.com>
  *
  * Using code from:
  *  - corgi_ts.c
@@ -38,7 +39,8 @@
 /*
  * This code has been heavily tested on a Nokia 770, and lightly
  * tested on other ads7846 devices (OSK/Mistral, Lubbock).
- * Support for ads7843 and ads7845 has only been stubbed in.
+ * Support for ads7843 tested on Atmel at91sam926x-EK.
+ * Support for ads7845 has only been stubbed in.
  *
  * IRQ handling needs a workaround because of a shortcoming in handling
  * edge triggered IRQs on some platforms like the OMAP1/2. These
@@ -82,6 +84,7 @@ struct ads7846 {
 	u16			pressure_max;
 
 	u8			read_x, read_y, read_z1, read_z2, pwrdown;
+	u16			zerro;		/* to send zerros while receiving */
 	u16			dummy;		/* for the pwrdown read */
 	struct ts_event		tc;
 
@@ -151,6 +154,10 @@ struct ads7846 {
 #define	READ_X	(READ_12BIT_DFR(x)  | ADS_PD10_ADC_ON)
 #define	PWRDOWN	(READ_12BIT_DFR(y)  | ADS_PD10_PDOWN)	/* LAST */
 
+/* alternate ads7843 commands */
+#define	ALT_READ_Y	(READ_12BIT_DFR(y)  | ADS_PD10_ALL_ON)
+#define	ALT_READ_X	(READ_12BIT_DFR(x)  | ADS_PD10_ALL_ON)
+
 /* single-ended samples need to first power up reference voltage;
  * we leave both ADC and VREF powered
  */
@@ -171,6 +178,7 @@ struct ser_req {
 	u8			command;
 	u8			ref_off;
 	u16			scratch;
+	u16			zerro;
 	__be16			sample;
 	struct spi_message	msg;
 	struct spi_transfer	xfer[6];
@@ -203,6 +211,7 @@ static int ads7846_read12_ser(struct dev
 	req->ref_on = REF_ON;
 	req->xfer[0].tx_buf = &req->ref_on;
 	req->xfer[0].len = 1;
+	req->xfer[1].tx_buf = &req->zerro;
 	req->xfer[1].rx_buf = &req->scratch;
 	req->xfer[1].len = 2;
 
@@ -217,6 +226,7 @@ static int ads7846_read12_ser(struct dev
 	req->command = (u8) command;
 	req->xfer[2].tx_buf = &req->command;
 	req->xfer[2].len = 1;
+	req->xfer[3].tx_buf = &req->zerro;
 	req->xfer[3].rx_buf = &req->sample;
 	req->xfer[3].len = 2;
 
@@ -226,6 +236,7 @@ static int ads7846_read12_ser(struct dev
 	req->ref_off = REF_OFF;
 	req->xfer[4].tx_buf = &req->ref_off;
 	req->xfer[4].len = 1;
+	req->xfer[3].tx_buf = &req->zerro;
 	req->xfer[5].rx_buf = &req->scratch;
 	req->xfer[5].len = 2;
 
@@ -410,6 +421,50 @@ static void ads7846_rx(void *ads)
 	spin_unlock_irqrestore(&ts->lock, flags);
 }
 
+static void ads7843_rx(void *ads)
+{
+	struct ads7846		*ts = ads;
+	struct input_dev	*input_dev = ts->input;
+	u16			x, y;
+	unsigned long		flags;
+
+	
+	/* adjust:  on-wire is a must-ignore bit, a BE12 value, then padding;
+	 * built from two 8 bit values written msb-first.
+	 */
+	x = (be16_to_cpu(ts->tc.x) >> 3) & 0x0fff;
+	y = (be16_to_cpu(ts->tc.y) >> 3) & 0x0fff;
+
+	/* range filtering */
+	if (x == MAX_12BIT)
+		x = 0;
+
+	if (ts->pendown) {
+		input_report_key(input_dev, BTN_TOUCH, 1);
+		input_report_abs(input_dev, ABS_PRESSURE, ts->pressure_max / 2);
+		input_report_abs(input_dev, ABS_X, x);
+		input_report_abs(input_dev, ABS_Y, y);
+	} else {
+		input_report_key(input_dev, BTN_TOUCH, 0);
+		input_report_abs(input_dev, ABS_PRESSURE, 0);
+	}
+
+	input_sync(input_dev);
+
+#ifdef	VERBOSE
+	pr_debug("%s: %d/%d%s\n", ts->spi->dev.bus_id,
+		x, y, ts->pendown ? "" : " UP");
+#endif
+
+	if (ts->pendown) {
+		spin_lock_irqsave(&ts->lock, flags);
+
+		mod_timer(&ts->timer, jiffies + TS_POLL_PERIOD);
+
+		spin_unlock_irqrestore(&ts->lock, flags);
+	}
+}
+
 static void ads7846_debounce(void *ads)
 {
 	struct ads7846		*ts = ads;
@@ -465,11 +520,25 @@ static void ads7846_timer(unsigned long 
 {
 	struct ads7846	*ts = (void *)handle;
 	int		status = 0;
+	int		alt_end_cycle = 0; /* ads7843 alternative cycle end */
+
+	if (ts->model == 7843) {
+		/* get sample */
+		ts->pendown = ts->get_pendown_state();
+		alt_end_cycle = ts->pending;
+	}
 
 	spin_lock_irq(&ts->lock);
 
-	if (unlikely(ts->msg_idx && !ts->pendown)) {
+	if (unlikely(!ts->pendown && (ts->msg_idx || alt_end_cycle))) {
 		/* measurement cycle ended */
+		if (ts->model == 7843) {
+			status = spi_async(ts->spi, ts->last_msg);
+			if (status)
+				dev_err(&ts->spi->dev,
+					"spi_async --> %d\n", status);
+		}
+
 		if (!device_suspended(&ts->spi->dev)) {
 			ts->irq_disabled = 0;
 			enable_irq(ts->spi->irq);
@@ -684,12 +753,17 @@ static int __devinit ads7846_probe(struc
 	spi_message_init(m);
 
 	/* y- still on; turn on only y+ (and ADC) */
-	ts->read_y = READ_Y;
+	if (ts->model == 7843) {
+		ts->read_y = ALT_READ_Y;
+	} else {
+		ts->read_y = READ_Y;
+	}
 	x->tx_buf = &ts->read_y;
 	x->len = 1;
 	spi_message_add_tail(x, m);
 
 	x++;
+	x->tx_buf = &ts->zerro;
 	x->rx_buf = &ts->tc.y;
 	x->len = 2;
 	spi_message_add_tail(x, m);
@@ -702,12 +776,17 @@ static int __devinit ads7846_probe(struc
 
 	/* turn y- off, x+ on, then leave in lowpower */
 	x++;
-	ts->read_x = READ_X;
+	if (ts->model == 7843) {
+		ts->read_x = ALT_READ_X;
+	} else {
+		ts->read_x = READ_X;
+	}
 	x->tx_buf = &ts->read_x;
 	x->len = 1;
 	spi_message_add_tail(x, m);
 
 	x++;
+	x->tx_buf = &ts->zerro;
 	x->rx_buf = &ts->tc.x;
 	x->len = 2;
 	spi_message_add_tail(x, m);
@@ -763,12 +842,17 @@ static int __devinit ads7846_probe(struc
 	spi_message_add_tail(x, m);
 
 	x++;
+	x->tx_buf = &ts->zerro;
 	x->rx_buf = &ts->dummy;
 	x->len = 2;
 	CS_CHANGE(*x);
 	spi_message_add_tail(x, m);
 
-	m->complete = ads7846_rx;
+	if (ts->model == 7843) {
+		m->complete = ads7843_rx;
+	} else {
+		m->complete = ads7846_rx;
+	}
 	m->context = ts;
 
 	ts->last_msg = m;
@@ -782,11 +866,16 @@ static int __devinit ads7846_probe(struc
 
 	dev_info(&spi->dev, "touchscreen, irq %d\n", spi->irq);
 
-	/* take a first sample, leaving nPENIRQ active; avoid
-	 * the touchscreen, in case it's not connected.
-	 */
-	(void) ads7846_read12_ser(&spi->dev,
-			  READ_12BIT_SER(vaux) | ADS_PD10_ALL_ON);
+	if (ts->model != 7843) {
+		/* take a first sample, leaving nPENIRQ active; avoid
+		 * the touchscreen, in case it's not connected.
+		 */
+		(void) ads7846_read12_ser(&spi->dev,
+				  READ_12BIT_SER(vaux) | ADS_PD10_ALL_ON);
+	} else {
+		(void) ads7846_read12_ser(&spi->dev,
+				  READ_12BIT_DFR(y) | ADS_PD10_ALL_ON);
+	}
 
 	/* ads7843/7845 don't have temperature sensors, and
 	 * use the other sensors a bit differently too
@@ -795,12 +884,14 @@ static int __devinit ads7846_probe(struc
 		device_create_file(&spi->dev, &dev_attr_temp0);
 		device_create_file(&spi->dev, &dev_attr_temp1);
 	}
-	if (ts->model != 7845)
+	if (ts->model != 7845 && ts->model != 7843)
 		device_create_file(&spi->dev, &dev_attr_vbatt);
-	device_create_file(&spi->dev, &dev_attr_vaux);
 
-	device_create_file(&spi->dev, &dev_attr_pen_down);
+	if (ts->model != 7843) {
+		device_create_file(&spi->dev, &dev_attr_vaux);
+	}
 
+	device_create_file(&spi->dev, &dev_attr_pen_down);
 	device_create_file(&spi->dev, &dev_attr_disable);
 
 	err = input_register_device(input_dev);
@@ -816,9 +907,12 @@ static int __devinit ads7846_probe(struc
 		device_remove_file(&spi->dev, &dev_attr_temp1);
 		device_remove_file(&spi->dev, &dev_attr_temp0);
 	}
-	if (ts->model != 7845)
+	if (ts->model != 7845 && ts->model != 7843)
 		device_remove_file(&spi->dev, &dev_attr_vbatt);
-	device_remove_file(&spi->dev, &dev_attr_vaux);
+
+	if (ts->model != 7843) {
+		device_remove_file(&spi->dev, &dev_attr_vaux);
+	}
 
 	free_irq(spi->irq, ts);
  err_free_mem:
@@ -841,9 +935,12 @@ static int __devexit ads7846_remove(stru
 		device_remove_file(&spi->dev, &dev_attr_temp1);
 		device_remove_file(&spi->dev, &dev_attr_temp0);
 	}
-	if (ts->model != 7845)
+	if (ts->model != 7845 && ts->model != 7843)
 		device_remove_file(&spi->dev, &dev_attr_vbatt);
-	device_remove_file(&spi->dev, &dev_attr_vaux);
+
+	if (ts->model != 7843) {
+		device_remove_file(&spi->dev, &dev_attr_vaux);
+	}
 
 	free_irq(ts->spi->irq, ts);
 	/* suspend left the IRQ disabled */

Re: [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

[Andrew Morton]

On Fri, 15 Dec 2006 15:45:08 +0100
Nicolas FERRE <nicolas.ferre@rfo.atmel.com> wrote:

> Add support for the ads7843 touchscreen controller to the ads7846
> driver code.

Generates a lot of errors when applied to the current mainline kernel. 
Please prepare and test patches against Linus's current git tree.

> --- a/input/touchscreen/ads7846.c	(.../linux-2.6.19-at91/drivers)	(revision 634)
> +++ b/input/touchscreen/ads7846.c	(.../linux-2.6.19-atmel-devel/drivers)	(revision 634)


That should be

--- a/drivers/input/ads7846.c
+++ b/drivers/input/ads7846.c

ie: `patch -p1' form, thanks.

Re: [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

[Nicolas Ferre]

Andrew Morton a écrit :
> On Fri, 15 Dec 2006 15:45:08 +0100
> Nicolas FERRE <nicolas.ferre@rfo.atmel.com> wrote:
> 
>> Add support for the ads7843 touchscreen controller to the ads7846
>> driver code.
> 
> Generates a lot of errors when applied to the current mainline kernel. 
> Please prepare and test patches against Linus's current git tree.

Hi,

David Brownell told me to take into account code written by
the omap/N770 guys.
I will then refresh my patch against those bits and produce
an up-to-date patch.

Follow-up for this thread @
http://lkml.org/lkml/2006/12/20/293

Regards,
-- 
Nicolas Ferre