[PATCH] Palm interrupt driven touchscreen driver

[PATCH] Palm interrupt driven touchscreen driver

[Marek Vasut]

[-- Attachment #1: Type: text/plain, Size: 274 bytes --]

Hi,

this patch adds interrupt driven touchscreen driver. It's an addition to 
mainstone-wm97xx, but does some reorganization of the code there.

Mark, it's the same patch we discussed on IRC, so it should be OK.

Please consider applying, it's against Eric's tree.

Thanks

[-- Attachment #2: 0001-Add-Palm-support-to-Mainstone-accelerated-touch.patch --]
[-- Type: text/x-diff, Size: 3989 bytes --]

From a652b9cb63a8ef4293ef7028017219337553053d Mon Sep 17 00:00:00 2001
From: Marek Vasut <marek.vasut@gmail.com>
Date: Wed, 3 Jun 2009 19:50:49 +0200
Subject: [PATCH] Add Palm support to Mainstone accelerated touch

This patch refactors the Mainstone accelerated touch code a little and
adds support for interrupt driven touchscreen on Palm LifeDrive, TX and
Tungsten T5.

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
---
 drivers/input/touchscreen/Kconfig            |    4 +-
 drivers/input/touchscreen/mainstone-wm97xx.c |   52 +++++++++++++++++--------
 2 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index b01fd61..4072d22 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -342,11 +342,11 @@ config TOUCHSCREEN_WM9713
 	  WM9713 touchscreen controller.
 
 config TOUCHSCREEN_WM97XX_MAINSTONE
-	tristate "WM97xx Mainstone accelerated touch"
+	tristate "WM97xx Mainstone/Palm accelerated touch"
 	depends on TOUCHSCREEN_WM97XX && ARCH_PXA
 	help
 	  Say Y here for support for streaming mode with WM97xx touchscreens
-	  on Mainstone systems.
+	  on Mainstone, Palm Tungsten T5, TX and LifeDrive systems.
 
 	  If unsure, say N.
 
diff --git a/drivers/input/touchscreen/mainstone-wm97xx.c b/drivers/input/touchscreen/mainstone-wm97xx.c
index 4cc047a..892ed43 100644
--- a/drivers/input/touchscreen/mainstone-wm97xx.c
+++ b/drivers/input/touchscreen/mainstone-wm97xx.c
@@ -31,9 +31,13 @@
 #include <linux/interrupt.h>
 #include <linux/wm97xx.h>
 #include <linux/io.h>
+
 #include <mach/regs-ac97.h>
+#include <mach/gpio.h>
+
+#include <asm/mach-types.h>
 
-#define VERSION		"0.13"
+#define VERSION		"0.14"
 
 struct continuous {
 	u16 id;    /* codec id */
@@ -62,6 +66,7 @@ static const struct continuous cinfo[] = {
 /* continuous speed index */
 static int sp_idx;
 static u16 last, tries;
+static int irq;
 
 /*
  * Pen sampling frequency (Hz) in continuous mode.
@@ -171,7 +176,7 @@ up:
 
 static int wm97xx_acc_startup(struct wm97xx *wm)
 {
-	int idx = 0;
+	int idx = 0, ret = 0;
 
 	/* check we have a codec */
 	if (wm->ac97 == NULL)
@@ -191,18 +196,37 @@ static int wm97xx_acc_startup(struct wm97xx *wm)
 		 "mainstone accelerated touchscreen driver, %d samples/sec\n",
 		 cinfo[sp_idx].speed);
 
+	/* IRQ driven touchscreen is used on Palm hardware */
+	if (machine_is_palmt5() || machine_is_palmtx() || machine_is_palmld()) {
+		pen_int = 1;
+		irq = 27;
+	} else if (machine_is_mainstone() && pen_int)
+		irq = 4;
+
+	if (irq) {
+		ret = gpio_request(irq, "Touchscreen IRQ");
+		if (ret)
+			goto out;
+
+		ret = gpio_direction_input(irq);
+		if (ret) {
+			gpio_free(irq);
+			goto out;
+		}
+
+		wm->pen_irq = gpio_to_irq(irq);
+		set_irq_type(wm->pen_irq, IRQ_TYPE_EDGE_BOTH);
+	} else	/* pen irq not supported */
+		pen_int = 0;
+
 	/* codec specific irq config */
 	if (pen_int) {
 		switch (wm->id) {
 		case WM9705_ID2:
-			wm->pen_irq = IRQ_GPIO(4);
-			set_irq_type(IRQ_GPIO(4), IRQ_TYPE_EDGE_BOTH);
 			break;
 		case WM9712_ID2:
 		case WM9713_ID2:
-			/* enable pen down interrupt */
 			/* use PEN_DOWN GPIO 13 to assert IRQ on GPIO line 2 */
-			wm->pen_irq = MAINSTONE_AC97_IRQ;
 			wm97xx_config_gpio(wm, WM97XX_GPIO_13, WM97XX_GPIO_IN,
 					   WM97XX_GPIO_POL_HIGH,
 					   WM97XX_GPIO_STICKY,
@@ -220,23 +244,17 @@ static int wm97xx_acc_startup(struct wm97xx *wm)
 		}
 	}
 
-	return 0;
+out:
+	return ret;
 }
 
 static void wm97xx_acc_shutdown(struct wm97xx *wm)
 {
 	/* codec specific deconfig */
 	if (pen_int) {
-		switch (wm->id & 0xffff) {
-		case WM9705_ID2:
-			wm->pen_irq = 0;
-			break;
-		case WM9712_ID2:
-		case WM9713_ID2:
-			/* disable interrupt */
-			wm->pen_irq = 0;
-			break;
-		}
+		if (irq)
+			gpio_free(irq);
+		wm->pen_irq = 0;
 	}
 }
 
-- 
1.6.2.1


[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] Palm interrupt driven touchscreen driver

[Eric Miao]

On Thu, Jun 4, 2009 at 5:41 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Hi,
>
> this patch adds interrupt driven touchscreen driver. It's an addition to
> mainstone-wm97xx, but does some reorganization of the code there.
>
> Mark, it's the same patch we discussed on IRC, so it should be OK.
>

Mark, the driver looks generic enough to me, is there some specific
reason this remains named "mainstone-wm97xx.c"?

I didn't look into the code too much, yet I'm wondering if it's possible
to come up with a platform_data to cover the difference of boards
and make this driver generic.

Re: [PATCH] Palm interrupt driven touchscreen driver

[Ian Molton]

Eric Miao wrote:

> Mark, the driver looks generic enough to me, is there some specific
> reason this remains named "mainstone-wm97xx.c"?
> 
> I didn't look into the code too much, yet I'm wondering if it's possible
> to come up with a platform_data to cover the difference of boards
> and make this driver generic.

eseries can use this idea IIRC...

Re: [PATCH] Palm interrupt driven touchscreen driver

[Russell King - ARM Linux]

On Wed, Jun 03, 2009 at 11:41:03PM +0200, Marek Vasut wrote:
> +#include <mach/gpio.h>

should be linux/gpio.h

Re: [PATCH] Palm interrupt driven touchscreen driver

[Mark Brown]

On Thu, Jun 04, 2009 at 09:35:17AM +0800, Eric Miao wrote:
> On Thu, Jun 4, 2009 at 5:41 AM, Marek Vasut <marek.vasut@gmail.com> wrote:

> > Mark, it's the same patch we discussed on IRC, so it should be OK.

> Mark, the driver looks generic enough to me, is there some specific
> reason this remains named "mainstone-wm97xx.c"?

No, and in fact I've asked Marek in some off-list discussions to do
something along those lines and/or remove the mainstone code entirely
but he doesn't seem keen on it for some reason.  Now we have more widely
distributed platforms merged I see no reason to keep the Mainstone code
in mainline.

> I didn't look into the code too much, yet I'm wondering if it's possible
> to come up with a platform_data to cover the difference of boards
> and make this driver generic.

As I've said a number of times before AC97 does not support platform
data and the ALSA guys blocked adding it.

Re: [PATCH] Palm interrupt driven touchscreen driver

[Jaroslav Kysela]

On Thu, 4 Jun 2009, Mark Brown wrote:

>> to come up with a platform_data to cover the difference of boards
>> and make this driver generic.
>
> As I've said a number of times before AC97 does not support platform
> data and the ALSA guys blocked adding it.

We're not blocking adding more data to AC97 structures. We just blocked 
adding new variable when it's not used.

In other words - send a driver code using this variable with the variable 
addition to the ALSA AC97 structure.

 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: [PATCH] Palm interrupt driven touchscreen driver

[Russell King - ARM Linux]

On Thu, Jun 04, 2009 at 09:54:07AM +0100, Mark Brown wrote:
> Now we have more widely
> distributed platforms merged I see no reason to keep the Mainstone code
> in mainline.

Why not?  So what do those of us with mainstone platforms but not these
"more widely distributed platforms" use instead?

Re: [PATCH] Palm interrupt driven touchscreen driver

[Mark Brown]

On Thu, Jun 04, 2009 at 10:13:29AM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 04, 2009 at 09:54:07AM +0100, Mark Brown wrote:

> > Now we have more widely
> > distributed platforms merged I see no reason to keep the Mainstone code
> > in mainline.

> Why not?  So what do those of us with mainstone platforms but not these
> "more widely distributed platforms" use instead?

This isn't a standard fit module for the Mainstone - it's the Mainstone
with the default audio plugin module replaced by a Wolfson plugin module.
If you just have a standard Mainstone you'd need a different accelerated
touch driver for the relevant AC97 controller.

If there are people out there with these setups then obviously the
driver is worth keeping in mainline, but I'm not aware of any active
users.

Re: [PATCH] Palm interrupt driven touchscreen driver

[Mark Brown]

On Thu, Jun 04, 2009 at 11:09:34AM +0200, Jaroslav Kysela wrote:
> On Thu, 4 Jun 2009, Mark Brown wrote:

>> As I've said a number of times before AC97 does not support platform
>> data and the ALSA guys blocked adding it.

> We're not blocking adding more data to AC97 structures. We just blocked  
> adding new variable when it's not used.

There was a very strong pushback against the idea of adding a void *
to the structure, which is what I'd be looking for.

> In other words - send a driver code using this variable with the variable 
> addition to the ALSA AC97 structure.

IIRC the original posting had something along with it?  As far as
examples go things like drivers/power/wm97xx_battery.c are already in
tree and pretty much ready to use platform data if it became available.

Re: [PATCH] Palm interrupt driven touchscreen driver

[Takashi Iwai]

At Thu, 4 Jun 2009 10:37:37 +0100,
Mark Brown wrote:
> 
> On Thu, Jun 04, 2009 at 11:09:34AM +0200, Jaroslav Kysela wrote:
> > On Thu, 4 Jun 2009, Mark Brown wrote:
> 
> >> As I've said a number of times before AC97 does not support platform
> >> data and the ALSA guys blocked adding it.
> 
> > We're not blocking adding more data to AC97 structures. We just blocked  
> > adding new variable when it's not used.
> 
> There was a very strong pushback against the idea of adding a void *
> to the structure, which is what I'd be looking for.

Yes, I was against the void pointer because there is no guarantee
of the data-type correctness between the data and the user.
The normal drvdata is fine because the creator and the user are the
same.  But, the proposal of private_driver_data (of the last one, at
least) is no such a scenario.


Takashi

Re: [PATCH] Palm interrupt driven touchscreen driver

[Mark Brown]

On Thu, Jun 04, 2009 at 11:44:22AM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > There was a very strong pushback against the idea of adding a void *
> > to the structure, which is what I'd be looking for.

> Yes, I was against the void pointer because there is no guarantee
> of the data-type correctness between the data and the user.
> The normal drvdata is fine because the creator and the user are the
> same.  But, the proposal of private_driver_data (of the last one, at
> least) is no such a scenario.

Do you have a pointer to this proposal?  I don't remember it and my
google-fu is weak today.

Re: [PATCH] Palm interrupt driven touchscreen driver

[Marek Vasut]

[-- Attachment #1: Type: text/plain, Size: 225 bytes --]

On Thursday 04 of June 2009 10:14:59 Russell King - ARM Linux wrote:
> On Wed, Jun 03, 2009 at 11:41:03PM +0200, Marek Vasut wrote:
> > +#include <mach/gpio.h>
>
> should be linux/gpio.h

Fixed in the following patch. Thanks

[-- Attachment #2: 0001-Add-Palm-support-to-Mainstone-accelerated-touch.patch --]
[-- Type: text/x-diff, Size: 3994 bytes --]

From f86f889e648369a095099e2f8a4423d787f512d6 Mon Sep 17 00:00:00 2001
From: Marek Vasut <marek.vasut@gmail.com>
Date: Thu, 4 Jun 2009 22:02:18 +0200
Subject: [PATCH 1/2] Add Palm support to Mainstone accelerated touch

This patch refactors the Mainstone accelerated touch code a little and
adds support for interrupt driven touchscreen on Palm LifeDrive, TX and
Tungsten T5.

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
---
 drivers/input/touchscreen/Kconfig            |    4 +-
 drivers/input/touchscreen/mainstone-wm97xx.c |   52 +++++++++++++++++--------
 2 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index b01fd61..4072d22 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -342,11 +342,11 @@ config TOUCHSCREEN_WM9713
 	  WM9713 touchscreen controller.
 
 config TOUCHSCREEN_WM97XX_MAINSTONE
-	tristate "WM97xx Mainstone accelerated touch"
+	tristate "WM97xx Mainstone/Palm accelerated touch"
 	depends on TOUCHSCREEN_WM97XX && ARCH_PXA
 	help
 	  Say Y here for support for streaming mode with WM97xx touchscreens
-	  on Mainstone systems.
+	  on Mainstone, Palm Tungsten T5, TX and LifeDrive systems.
 
 	  If unsure, say N.
 
diff --git a/drivers/input/touchscreen/mainstone-wm97xx.c b/drivers/input/touchscreen/mainstone-wm97xx.c
index 4cc047a..60bda9a 100644
--- a/drivers/input/touchscreen/mainstone-wm97xx.c
+++ b/drivers/input/touchscreen/mainstone-wm97xx.c
@@ -31,9 +31,13 @@
 #include <linux/interrupt.h>
 #include <linux/wm97xx.h>
 #include <linux/io.h>
+#include <linux/gpio.h>
+
 #include <mach/regs-ac97.h>
 
-#define VERSION		"0.13"
+#include <asm/mach-types.h>
+
+#define VERSION		"0.14"
 
 struct continuous {
 	u16 id;    /* codec id */
@@ -62,6 +66,7 @@ static const struct continuous cinfo[] = {
 /* continuous speed index */
 static int sp_idx;
 static u16 last, tries;
+static int irq;
 
 /*
  * Pen sampling frequency (Hz) in continuous mode.
@@ -171,7 +176,7 @@ up:
 
 static int wm97xx_acc_startup(struct wm97xx *wm)
 {
-	int idx = 0;
+	int idx = 0, ret = 0;
 
 	/* check we have a codec */
 	if (wm->ac97 == NULL)
@@ -191,18 +196,37 @@ static int wm97xx_acc_startup(struct wm97xx *wm)
 		 "mainstone accelerated touchscreen driver, %d samples/sec\n",
 		 cinfo[sp_idx].speed);
 
+	/* IRQ driven touchscreen is used on Palm hardware */
+	if (machine_is_palmt5() || machine_is_palmtx() || machine_is_palmld()) {
+		pen_int = 1;
+		irq = 27;
+	} else if (machine_is_mainstone() && pen_int)
+		irq = 4;
+
+	if (irq) {
+		ret = gpio_request(irq, "Touchscreen IRQ");
+		if (ret)
+			goto out;
+
+		ret = gpio_direction_input(irq);
+		if (ret) {
+			gpio_free(irq);
+			goto out;
+		}
+
+		wm->pen_irq = gpio_to_irq(irq);
+		set_irq_type(wm->pen_irq, IRQ_TYPE_EDGE_BOTH);
+	} else	/* pen irq not supported */
+		pen_int = 0;
+
 	/* codec specific irq config */
 	if (pen_int) {
 		switch (wm->id) {
 		case WM9705_ID2:
-			wm->pen_irq = IRQ_GPIO(4);
-			set_irq_type(IRQ_GPIO(4), IRQ_TYPE_EDGE_BOTH);
 			break;
 		case WM9712_ID2:
 		case WM9713_ID2:
-			/* enable pen down interrupt */
 			/* use PEN_DOWN GPIO 13 to assert IRQ on GPIO line 2 */
-			wm->pen_irq = MAINSTONE_AC97_IRQ;
 			wm97xx_config_gpio(wm, WM97XX_GPIO_13, WM97XX_GPIO_IN,
 					   WM97XX_GPIO_POL_HIGH,
 					   WM97XX_GPIO_STICKY,
@@ -220,23 +244,17 @@ static int wm97xx_acc_startup(struct wm97xx *wm)
 		}
 	}
 
-	return 0;
+out:
+	return ret;
 }
 
 static void wm97xx_acc_shutdown(struct wm97xx *wm)
 {
 	/* codec specific deconfig */
 	if (pen_int) {
-		switch (wm->id & 0xffff) {
-		case WM9705_ID2:
-			wm->pen_irq = 0;
-			break;
-		case WM9712_ID2:
-		case WM9713_ID2:
-			/* disable interrupt */
-			wm->pen_irq = 0;
-			break;
-		}
+		if (irq)
+			gpio_free(irq);
+		wm->pen_irq = 0;
 	}
 }
 
-- 
1.6.2.1


[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] Palm interrupt driven touchscreen driver

[Takashi Iwai]

At Thu, 4 Jun 2009 11:38:07 +0100,
Mark Brown wrote:
> 
> On Thu, Jun 04, 2009 at 11:44:22AM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > There was a very strong pushback against the idea of adding a void *
> > > to the structure, which is what I'd be looking for.
> 
> > Yes, I was against the void pointer because there is no guarantee
> > of the data-type correctness between the data and the user.
> > The normal drvdata is fine because the creator and the user are the
> > same.  But, the proposal of private_driver_data (of the last one, at
> > least) is no such a scenario.
> 
> Do you have a pointer to this proposal?  I don't remember it and my
> google-fu is weak today.

I couldn't find it.  But IIRC, the original patch was to add "int irq"
field to struct snd_ac97.  I would accept it because it can do any
harm.

But, adding a generic "void *driver_data" instead of "int irq" can
play a bad game.  It can be dangerous when the creator and the user of
this pointer are different drivers.  That was my argument.


thanks,

Takashi

Re: [PATCH] Palm interrupt driven touchscreen driver

[Mark Brown]

On Fri, Jun 05, 2009 at 11:16:12AM +0200, Takashi Iwai wrote:

> I couldn't find it.  But IIRC, the original patch was to add "int irq"
> field to struct snd_ac97.  I would accept it because it can do any
> harm.

Ah, yes - I do remember that proposal.  The problem with that is that it
doesn't scale up to all the platform data you might want to pass in.
The WM97xx battery and touchscreen drivers are the main things I'm
interested in here - the platform data for the WM97xx touch driver is
struct wm97xx_mach_ops in include/linux/wm97xx.h for example.

> But, adding a generic "void *driver_data" instead of "int irq" can
> play a bad game.  It can be dangerous when the creator and the user of
> this pointer are different drivers.  That was my argument.

There is potential risk involved in these interfaces but it's the
standard idiom for doing this on embedded systems.  The buses definitely
don't want to know anything about the random data that drivers may need
and since you're not dealing with pluggable or variable hardware in the
same way as you do on PCs the risks of mismatched data aren't such a big
deal - you'd generally only be able to encounter problems if you're
building your own kernel.

Re: [PATCH] Palm interrupt driven touchscreen driver

[Takashi Iwai]

At Fri, 5 Jun 2009 10:30:02 +0100,
Mark Brown wrote:
> 
> On Fri, Jun 05, 2009 at 11:16:12AM +0200, Takashi Iwai wrote:
> 
> > I couldn't find it.  But IIRC, the original patch was to add "int irq"
> > field to struct snd_ac97.  I would accept it because it can do any
> > harm.
> 
> Ah, yes - I do remember that proposal.  The problem with that is that it
> doesn't scale up to all the platform data you might want to pass in.
> The WM97xx battery and touchscreen drivers are the main things I'm
> interested in here - the platform data for the WM97xx touch driver is
> struct wm97xx_mach_ops in include/linux/wm97xx.h for example.
> 
> > But, adding a generic "void *driver_data" instead of "int irq" can
> > play a bad game.  It can be dangerous when the creator and the user of
> > this pointer are different drivers.  That was my argument.
> 
> There is potential risk involved in these interfaces but it's the
> standard idiom for doing this on embedded systems.  The buses definitely
> don't want to know anything about the random data that drivers may need
> and since you're not dealing with pluggable or variable hardware in the
> same way as you do on PCs the risks of mismatched data aren't such a big
> deal - you'd generally only be able to encounter problems if you're
> building your own kernel.

Well, but you know that can be broken :)

BTW, can't it be simply driver_data/platform_data of ac97.dev...?


Takashi

Re: [PATCH] Palm interrupt driven touchscreen driver

[Mark Brown]

On Fri, Jun 05, 2009 at 12:05:11PM +0200, Takashi Iwai wrote:

> BTW, can't it be simply driver_data/platform_data of ac97.dev...?

We'd need to add the void * to snd_ac97_template too so that the data is
initialised before snd_ac97_mixer() instantiates the device but that's
the only issue I can see.  If you're happy with that approach I can send
some patches for it?

Re: [PATCH] Palm interrupt driven touchscreen driver

[Takashi Iwai]

At Fri, 5 Jun 2009 11:20:37 +0100,
Mark Brown wrote:
> 
> On Fri, Jun 05, 2009 at 12:05:11PM +0200, Takashi Iwai wrote:
> 
> > BTW, can't it be simply driver_data/platform_data of ac97.dev...?
> 
> We'd need to add the void * to snd_ac97_template too so that the data is
> initialised before snd_ac97_mixer() instantiates the device but that's
> the only issue I can see.  If you're happy with that approach I can send
> some patches for it?

Just setting it after calling snd_ac97_mixer() doesn't suffice?
It's always NULL as default.


Takashi

Re: [PATCH] Palm interrupt driven touchscreen driver

[Mark Brown]

On Fri, Jun 05, 2009 at 12:38:05PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > We'd need to add the void * to snd_ac97_template too so that the data is
> > initialised before snd_ac97_mixer() instantiates the device but that's
> > the only issue I can see.  If you're happy with that approach I can send
> > some patches for it?

> Just setting it after calling snd_ac97_mixer() doesn't suffice?
> It's always NULL as default.

Ah, should be - I'd misremembered how much snd_device_new() was doing
when looking at the snd_ac97_mixer() code.