[PATCH 0/4] s3c24xx_ts driver fixes

[PATCH 0/4] s3c24xx_ts driver fixes

[Vasily Khoruzhick]

Following series fix some issues in s3c24xx_ts driver and
related s3c24xx_adc driver.

[PATCH 1/4] Add resources description for s3c24xx ts driver

[Vasily Khoruzhick]

Without resources description driver fails during initialization.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 arch/arm/plat-s3c24xx/devs.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-s3c24xx/devs.c b/arch/arm/plat-s3c24xx/devs.c
index 7f686a3..83c1507 100644
--- a/arch/arm/plat-s3c24xx/devs.c
+++ b/arch/arm/plat-s3c24xx/devs.c
@@ -185,9 +185,27 @@ void __init s3c24xx_fb_set_platdata(struct s3c2410fb_mach_info *pd)
 }
 
 /* Touchscreen */
+
+static struct resource s3c_ts_resource[] = {
+	[0] = {
+		.start = S3C24XX_PA_ADC,
+		.end   = S3C24XX_PA_ADC + S3C24XX_SZ_ADC - 1,
+		.flags = IORESOURCE_MEM,
+	},
+	[1] = {
+		.start = IRQ_TC,
+		.end   = IRQ_TC,
+		.flags = IORESOURCE_IRQ,
+	},
+
+};
+
 struct platform_device s3c_device_ts = {
 	.name		  = "s3c2410-ts",
 	.id		  = -1,
+	.dev.parent	= &s3c_device_adc.dev,
+	.num_resources	  = ARRAY_SIZE(s3c_ts_resource),
+	.resource	  = s3c_ts_resource,
 };
 EXPORT_SYMBOL(s3c_device_ts);
 
-- 
1.7.0

[PATCH 1/4] Add resources description for s3c24xx ts driver

[Ben Dooks]

On Thu, Feb 18, 2010 at 06:32:27PM +0200, Vasily Khoruzhick wrote:
> Without resources description driver fails during initialization.
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>

ok, added to next-s3c24xx-ts

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

[PATCH 2/4] s3c24xx_ts: report touch only when stylus is down

[Vasily Khoruzhick]

By default, driver reports touches when it gots (1 << ts.shift)
samples, even if stylus is up. This behavior looks wrong to me.
Patch makes driver to report touches only when stylus is down.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/input/touchscreen/s3c2410_ts.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/input/touchscreen/s3c2410_ts.c b/drivers/input/touchscreen/s3c2410_ts.c
index 6386b44..e2fe2ed 100644
--- a/drivers/input/touchscreen/s3c2410_ts.c
+++ b/drivers/input/touchscreen/s3c2410_ts.c
@@ -128,25 +128,25 @@ static void touch_timer_fire(unsigned long data)
 
 	down = get_down(data0, data1);
 
-	if (ts.count == (1 << ts.shift)) {
-		ts.xp >>= ts.shift;
-		ts.yp >>= ts.shift;
+	if (down) {
+		if (ts.count == (1 << ts.shift)) {
+			ts.xp >>= ts.shift;
+			ts.yp >>= ts.shift;
 
-		dev_dbg(ts.dev, "%s: X=%lu, Y=%lu, count=%d\n",
-			__func__, ts.xp, ts.yp, ts.count);
+			dev_dbg(ts.dev, "%s: X=%lu, Y=%lu, count=%d\n",
+				__func__, ts.xp, ts.yp, ts.count);
 
-		input_report_abs(ts.input, ABS_X, ts.xp);
-		input_report_abs(ts.input, ABS_Y, ts.yp);
+			input_report_abs(ts.input, ABS_X, ts.xp);
+			input_report_abs(ts.input, ABS_Y, ts.yp);
 
-		input_report_key(ts.input, BTN_TOUCH, 1);
-		input_sync(ts.input);
+			input_report_key(ts.input, BTN_TOUCH, 1);
+			input_sync(ts.input);
 
+		}
 		ts.xp = 0;
 		ts.yp = 0;
 		ts.count = 0;
-	}
 
-	if (down) {
 		s3c_adc_start(ts.client, 0, 1 << ts.shift);
 	} else {
 		ts.count = 0;
-- 
1.7.0

[PATCH 2/4] s3c24xx_ts: report touch only when stylus is down

[Dmitry Torokhov]

Hi Vasily,

On Thu, Feb 18, 2010 at 06:32:28PM +0200, Vasily Khoruzhick wrote:
> By default, driver reports touches when it gots (1 << ts.shift)
> samples, even if stylus is up. This behavior looks wrong to me.
> Patch makes driver to report touches only when stylus is down.
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  drivers/input/touchscreen/s3c2410_ts.c |   22 +++++++++++-----------
>  1 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/s3c2410_ts.c b/drivers/input/touchscreen/s3c2410_ts.c
> index 6386b44..e2fe2ed 100644
> --- a/drivers/input/touchscreen/s3c2410_ts.c
> +++ b/drivers/input/touchscreen/s3c2410_ts.c
> @@ -128,25 +128,25 @@ static void touch_timer_fire(unsigned long data)
>  
>  	down = get_down(data0, data1);
>  
> -	if (ts.count == (1 << ts.shift)) {
> -		ts.xp >>= ts.shift;
> -		ts.yp >>= ts.shift;
> +	if (down) {
> +		if (ts.count == (1 << ts.shift)) {
> +			ts.xp >>= ts.shift;
> +			ts.yp >>= ts.shift;
>  
> -		dev_dbg(ts.dev, "%s: X=%lu, Y=%lu, count=%d\n",
> -			__func__, ts.xp, ts.yp, ts.count);
> +			dev_dbg(ts.dev, "%s: X=%lu, Y=%lu, count=%d\n",
> +				__func__, ts.xp, ts.yp, ts.count);
>  
> -		input_report_abs(ts.input, ABS_X, ts.xp);
> -		input_report_abs(ts.input, ABS_Y, ts.yp);
> +			input_report_abs(ts.input, ABS_X, ts.xp);
> +			input_report_abs(ts.input, ABS_Y, ts.yp);
>  
> -		input_report_key(ts.input, BTN_TOUCH, 1);
> -		input_sync(ts.input);
> +			input_report_key(ts.input, BTN_TOUCH, 1);
> +			input_sync(ts.input);
>  
> +		}
>  		ts.xp = 0;
>  		ts.yp = 0;
>  		ts.count = 0;

I do not think this is right. Here you reset sampling data regardless
of whether you got the right number of samples or not. You should repeat
s3c_adc_start only if you got (1 << ts.shift) samples. But I think I see
why touchpad may stop reporintg sometimes - if timer fires before you
get the needed number of samples nothing will happen as timer is not
gettign rearmed.

> -	}
>  
> -	if (down) {
>  		s3c_adc_start(ts.client, 0, 1 << ts.shift);
>  	} else {
>  		ts.count = 0;

I also think this branch is missing ts.xp = ts.yp = 0;

-- 
Dmitry

[PATCH 2/4] s3c24xx_ts: report touch only when stylus is down

[Vasily Khoruzhick]

Hi Dmitry,

? ????????? ?? 19 ??????? 2010 11:17:55 ????? Dmitry Torokhov ???????:

> I do not think this is right. Here you reset sampling data regardless
> of whether you got the right number of samples or not. You should repeat
> s3c_adc_start only if you got (1 << ts.shift) samples. But I think I see
> why touchpad may stop reporintg sometimes - if timer fires before you
> get the needed number of samples nothing will happen as timer is not
> gettign rearmed.

Problem is driver reports "stylus down" event when stylus is already up, 
anyway I agree that samples should not be discarded.

> >  	} else {
> >  	
> >  		ts.count = 0;
> 
> I also think this branch is missing ts.xp = ts.yp = 0;

Ok.

Fixed version is in attachment

Thanks for review.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-s3c24xx_ts-report-touch-only-when-stylus-is-down.patch
Type: text/x-patch
Size: 1904 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100219/506975f2/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100219/506975f2/attachment.sig>

[PATCH 2/4] s3c24xx_ts: report touch only when stylus is down

[Dmitry Torokhov]

On Fri, Feb 19, 2010 at 12:02:10PM +0200, Vasily Khoruzhick wrote:
> Hi Dmitry,
> 
> ? ????????? ?? 19 ??????? 2010 11:17:55 ????? Dmitry Torokhov ???????:
> 
> > I do not think this is right. Here you reset sampling data regardless
> > of whether you got the right number of samples or not. You should repeat
> > s3c_adc_start only if you got (1 << ts.shift) samples. But I think I see
> > why touchpad may stop reporintg sometimes - if timer fires before you
> > get the needed number of samples nothing will happen as timer is not
> > gettign rearmed.
> 
> Problem is driver reports "stylus down" event when stylus is already up, 
> anyway I agree that samples should not be discarded.
> 
> > >  	} else {
> > >  	
> > >  		ts.count = 0;
> > 
> > I also think this branch is missing ts.xp = ts.yp = 0;
> 
> Ok.
> 
> Fixed version is in attachment
> 
> Thanks for review.

Applied, thank you Vasily.

One more thing though - why do we need that timer to report input events
and reschedule ADC conversions? It looks like it is not really needed.
Could you please give a try to the patch below?

Thanks!

-- 
Dmitry

Input s3c2410_ts - get rid of timer

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

There seem to be no justification for using timer to report input events,
this can be done right in the ADC selection callback.

Also set proper parent for the input device and use common name prefix
throughout the driver.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/touchscreen/s3c2410_ts.c |  169 ++++++++++++++------------------
 1 files changed, 75 insertions(+), 94 deletions(-)


diff --git a/drivers/input/touchscreen/s3c2410_ts.c b/drivers/input/touchscreen/s3c2410_ts.c
index 3755a47..60f18c2 100644
--- a/drivers/input/touchscreen/s3c2410_ts.c
+++ b/drivers/input/touchscreen/s3c2410_ts.c
@@ -38,7 +38,6 @@
 
 #include <plat/adc.h>
 #include <plat/regs-adc.h>
-
 #include <mach/regs-gpio.h>
 #include <mach/ts.h>
 
@@ -73,7 +72,7 @@
  * @count: The number of samples collected.
  * @shift: The log2 of the maximum count to read in one go.
  */
-struct s3c2410ts {
+struct s3c2410_ts {
 	struct s3c_adc_client *client;
 	struct device *dev;
 	struct input_dev *input;
@@ -86,7 +85,7 @@ struct s3c2410ts {
 	int shift;
 };
 
-static struct s3c2410ts ts;
+static struct s3c2410_ts ts;
 
 /**
  * s3c2410_ts_connect - configure gpio for s3c2410 systems
@@ -95,7 +94,7 @@ static struct s3c2410ts ts;
  * connected to the device (later systems such as the S3C2440 integrate
  * these into the device).
 */
-static inline void s3c2410_ts_connect(void)
+static void s3c2410_ts_connect(void)
 {
 	s3c2410_gpio_cfgpin(S3C2410_GPG(12), S3C2410_GPG12_XMON);
 	s3c2410_gpio_cfgpin(S3C2410_GPG(13), S3C2410_GPG13_nXPON);
@@ -104,88 +103,57 @@ static inline void s3c2410_ts_connect(void)
 }
 
 /**
- * get_down - return the down state of the pen
- * @data0: The data read from ADCDAT0 register.
- * @data1: The data read from ADCDAT1 register.
- *
- * Return non-zero if both readings show that the pen is down.
+ * s3c2410_ts_check_pen_down - return the down state of the pen
  */
-static inline bool get_down(unsigned long data0, unsigned long data1)
+static bool s3c24xx_ts_check_pen_down(void)
 {
+	unsigned long data0 = readl(ts.io + S3C2410_ADCDAT0);
+	unsigned long data1 = readl(ts.io + S3C2410_ADCDAT1);
+
 	/* returns true if both data values show stylus down */
-	return (!(data0 & S3C2410_ADCDAT0_UPDOWN) &&
-		!(data1 & S3C2410_ADCDAT0_UPDOWN));
+	return !((data0 | data1) & S3C2410_ADCDAT0_UPDOWN);
 }
 
-static void touch_timer_fire(unsigned long data)
+static void s3c24xx_ts_report_state(bool pen_is_down)
 {
-	unsigned long data0;
-	unsigned long data1;
-	bool down;
-
-	data0 = readl(ts.io + S3C2410_ADCDAT0);
-	data1 = readl(ts.io + S3C2410_ADCDAT1);
-
-	down = get_down(data0, data1);
-
-	if (down) {
-		if (ts.count == (1 << ts.shift)) {
-			ts.xp >>= ts.shift;
-			ts.yp >>= ts.shift;
-
-			dev_dbg(ts.dev, "%s: X=%lu, Y=%lu, count=%d\n",
-				__func__, ts.xp, ts.yp, ts.count);
+	if (pen_is_down) {
+		ts.xp >>= ts.shift;
+		ts.yp >>= ts.shift;
 
-			input_report_abs(ts.input, ABS_X, ts.xp);
-			input_report_abs(ts.input, ABS_Y, ts.yp);
+		dev_dbg(ts.dev, "%s: X=%lu, Y=%lu, count=%d\n",
+			__func__, ts.xp, ts.yp, ts.count);
 
-			input_report_key(ts.input, BTN_TOUCH, 1);
-			input_sync(ts.input);
+		input_report_abs(ts.input, ABS_X, ts.xp);
+		input_report_abs(ts.input, ABS_Y, ts.yp);
 
-			ts.xp = 0;
-			ts.yp = 0;
-			ts.count = 0;
-		}
-
-		s3c_adc_start(ts.client, 0, 1 << ts.shift);
+		input_report_key(ts.input, BTN_TOUCH, 1);
 	} else {
-		ts.xp = 0;
-		ts.yp = 0;
-		ts.count = 0;
-
 		input_report_key(ts.input, BTN_TOUCH, 0);
-		input_sync(ts.input);
-
-		writel(WAIT4INT | INT_DOWN, ts.io + S3C2410_ADCTSC);
 	}
+
+	input_sync(ts.input);
+
 }
 
-static DEFINE_TIMER(touch_timer, touch_timer_fire, 0, 0);
+static void s3c24xx_ts_schedule_read(void)
+{
+	ts.xp = ts.yp = 0;
+	ts.count = 0;
+
+	s3c_adc_start(ts.client, 0, 1 << ts.shift);
+}
 
 /**
- * stylus_irq - touchscreen stylus event interrupt
+ * s3c24xx_ts_stylus_irq - touchscreen stylus event interrupt
  * @irq: The interrupt number
  * @dev_id: The device ID.
  *
  * Called when the IRQ_TC is fired for a pen up or down event.
  */
-static irqreturn_t stylus_irq(int irq, void *dev_id)
+static irqreturn_t s3c24xx_ts_stylus_irq(int irq, void *dev_id)
 {
-	unsigned long data0;
-	unsigned long data1;
-	bool down;
-
-	data0 = readl(ts.io + S3C2410_ADCDAT0);
-	data1 = readl(ts.io + S3C2410_ADCDAT1);
-
-	down = get_down(data0, data1);
-
-	/* TODO we should never get an interrupt with down set while
-	 * the timer is running, but maybe we ought to verify that the
-	 * timer isn't running anyways. */
-
-	if (down)
-		s3c_adc_start(ts.client, 0, 1 << ts.shift);
+	if (s3c24xx_ts_check_pen_down())
+		s3c24xx_ts_schedule_read();
 	else
 		dev_info(ts.dev, "%s: count=%d\n", __func__, ts.count);
 
@@ -231,12 +199,26 @@ static void s3c24xx_ts_conversion(struct s3c_adc_client *client,
  */
 static void s3c24xx_ts_select(struct s3c_adc_client *client, unsigned select)
 {
+	bool pen_is_down;
+
 	if (select) {
+		/*
+		 * Prepare for the next sample.
+		 */
 		writel(S3C2410_ADCTSC_PULL_UP_DISABLE | AUTOPST,
 		       ts.io + S3C2410_ADCTSC);
 	} else {
-		mod_timer(&touch_timer, jiffies+1);
-		writel(WAIT4INT | INT_UP, ts.io + S3C2410_ADCTSC);
+		/*
+		 * Conversion is complete, we have desired number of samples.
+		 */
+		pen_is_down = s3c24xx_ts_check_pen_down();
+
+		s3c24xx_ts_report_state(pen_is_down);
+
+		if (pen_is_down)
+			s3c24xx_ts_schedule_read();
+		else
+			writel(WAIT4INT | INT_DOWN, ts.io + S3C2410_ADCTSC);
 	}
 }
 
@@ -247,7 +229,7 @@ static void s3c24xx_ts_select(struct s3c_adc_client *client, unsigned select)
  * Initialise, find and allocate any resources we need to run and then
  * register with the ADC and input systems.
  */
-static int __devinit s3c2410ts_probe(struct platform_device *pdev)
+static int __devinit s3c24xx_ts_probe(struct platform_device *pdev)
 {
 	struct s3c2410_ts_mach_info *info;
 	struct device *dev = &pdev->dev;
@@ -256,7 +238,7 @@ static int __devinit s3c2410ts_probe(struct platform_device *pdev)
 	int ret = -EINVAL;
 
 	/* Initialise input stuff */
-	memset(&ts, 0, sizeof(struct s3c2410ts));
+	memset(&ts, 0, sizeof(struct s3c2410_ts));
 
 	ts.dev = dev;
 
@@ -313,8 +295,6 @@ static int __devinit s3c2410ts_probe(struct platform_device *pdev)
 	if ((info->delay & 0xffff) > 0)
 		writel(info->delay & 0xffff, ts.io + S3C2410_ADCDLY);
 
-	writel(WAIT4INT | INT_DOWN, ts.io + S3C2410_ADCTSC);
-
 	input_dev = input_allocate_device();
 	if (!input_dev) {
 		dev_err(dev, "Unable to allocate the input device !!\n");
@@ -333,17 +313,18 @@ static int __devinit s3c2410ts_probe(struct platform_device *pdev)
 	ts.input->id.vendor = 0xDEAD;
 	ts.input->id.product = 0xBEEF;
 	ts.input->id.version = 0x0102;
+	ts.input->dev.parent = &pdev->dev;
 
 	ts.shift = info->oversampling_shift;
 
-	ret = request_irq(ts.irq_tc, stylus_irq, IRQF_DISABLED,
+	ret = request_irq(ts.irq_tc, s3c24xx_ts_stylus_irq, IRQF_DISABLED,
 			  "s3c2410_ts_pen", ts.input);
 	if (ret) {
 		dev_err(dev, "cannot get TC interrupt\n");
 		goto err_inputdev;
 	}
 
-	dev_info(dev, "driver attached, registering input device\n");
+	dev_dbg(dev, "driver attached, registering input device\n");
 
 	/* All went ok, so register to the input system */
 	ret = input_register_device(ts.input);
@@ -353,16 +334,17 @@ static int __devinit s3c2410ts_probe(struct platform_device *pdev)
 		goto err_tcirq;
 	}
 
+	writel(WAIT4INT | INT_DOWN, ts.io + S3C2410_ADCTSC);
+
 	return 0;
 
  err_tcirq:
 	free_irq(ts.irq_tc, ts.input);
  err_inputdev:
-	input_unregister_device(ts.input);
+	input_free_device(ts.input);
  err_iomap:
 	iounmap(ts.io);
  err_clk:
-	del_timer_sync(&touch_timer);
 	clk_put(ts.clock);
 	return ret;
 }
@@ -373,10 +355,9 @@ static int __devinit s3c2410ts_probe(struct platform_device *pdev)
  *
  * Free up our state ready to be removed.
  */
-static int __devexit s3c2410ts_remove(struct platform_device *pdev)
+static int __devexit s3c24xx_ts_remove(struct platform_device *pdev)
 {
 	free_irq(ts.irq_tc, ts.input);
-	del_timer_sync(&touch_timer);
 
 	clk_disable(ts.clock);
 	clk_put(ts.clock);
@@ -388,7 +369,7 @@ static int __devexit s3c2410ts_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM
-static int s3c2410ts_suspend(struct device *dev)
+static int s3c24xx_ts_suspend(struct device *dev)
 {
 	writel(TSC_SLEEP, ts.io + S3C2410_ADCTSC);
 	disable_irq(ts.irq_tc);
@@ -397,7 +378,7 @@ static int s3c2410ts_suspend(struct device *dev)
 	return 0;
 }
 
-static int s3c2410ts_resume(struct device *dev)
+static int s3c24xx_ts_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct s3c2410_ts_mach_info *info = pdev->dev.platform_data;
@@ -414,44 +395,44 @@ static int s3c2410ts_resume(struct device *dev)
 	return 0;
 }
 
-static struct dev_pm_ops s3c_ts_pmops = {
-	.suspend	= s3c2410ts_suspend,
-	.resume		= s3c2410ts_resume,
+static struct dev_pm_ops s3c24xx_ts_pmops = {
+	.suspend	= s3c24xx_ts_suspend,
+	.resume		= s3c24xx_ts_resume,
 };
 #endif
 
-static struct platform_device_id s3cts_driver_ids[] = {
+static struct platform_device_id s3c24xx_ts_driver_ids[] = {
 	{ "s3c2410-ts", 0 },
 	{ "s3c2440-ts", 1 },
 	{ }
 };
-MODULE_DEVICE_TABLE(platform, s3cts_driver_ids);
+MODULE_DEVICE_TABLE(platform, s3c24xx_ts_driver_ids);
 
-static struct platform_driver s3c_ts_driver = {
+static struct platform_driver s3c24xx_ts_driver = {
 	.driver         = {
 		.name   = "s3c24xx-ts",
 		.owner  = THIS_MODULE,
 #ifdef CONFIG_PM
-		.pm	= &s3c_ts_pmops,
+		.pm	= &s3c24xx_ts_pmops,
 #endif
 	},
-	.id_table	= s3cts_driver_ids,
-	.probe		= s3c2410ts_probe,
-	.remove		= __devexit_p(s3c2410ts_remove),
+	.id_table	= s3c24xx_ts_driver_ids,
+	.probe		= s3c24xx_ts_probe,
+	.remove		= __devexit_p(s3c24xx_ts_remove),
 };
 
-static int __init s3c2410ts_init(void)
+static int __init s3c24xx_ts_init(void)
 {
-	return platform_driver_register(&s3c_ts_driver);
+	return platform_driver_register(&s3c24xx_ts_driver);
 }
 
-static void __exit s3c2410ts_exit(void)
+static void __exit s3c24xx_ts_exit(void)
 {
-	platform_driver_unregister(&s3c_ts_driver);
+	platform_driver_unregister(&s3c24xx_ts_driver);
 }
 
-module_init(s3c2410ts_init);
-module_exit(s3c2410ts_exit);
+module_init(s3c24xx_ts_init);
+module_exit(s3c24xx_ts_exit);
 
 MODULE_AUTHOR("Arnaud Patard <arnaud.patard@rtp-net.org>, "
 	      "Ben Dooks <ben@simtec.co.uk>, "

[PATCH 2/4] s3c24xx_ts: report touch only when stylus is down

[Vasily Khoruzhick]

? ????????? ?? 21 ??????? 2010 09:35:13 ????? Dmitry Torokhov ???????:

> Applied, thank you Vasily.
> 
> One more thing though - why do we need that timer to report input events
> and reschedule ADC conversions? It looks like it is not really needed.
> Could you please give a try to the patch below?
> 
> Thanks!

Timer is used to avoid reporting input events in interrupt context (select 
callback is called from s3c_adc_try and from s3c_adc_irq), Is it safe to call 
input_report_abs/input_report_key from interrupt context? Anyway, driver 
doesn't work for me with this patch (it works as mouse, but it doesn't as 
touchscreen through tslib)

Regards,
Vasily
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100221/461a0fd3/attachment.sig>

[PATCH 2/4] s3c24xx_ts: report touch only when stylus is down

[Dmitry Torokhov]

On Sun, Feb 21, 2010 at 11:05:26AM +0200, Vasily Khoruzhick wrote:
> ? ????????? ?? 21 ??????? 2010 09:35:13 ????? Dmitry Torokhov ???????:
> 
> > Applied, thank you Vasily.
> > 
> > One more thing though - why do we need that timer to report input events
> > and reschedule ADC conversions? It looks like it is not really needed.
> > Could you please give a try to the patch below?
> > 
> > Thanks!
> 
> Timer is used to avoid reporting input events in interrupt context (select 
> callback is called from s3c_adc_try and from s3c_adc_irq),

??? This is quite a novel concept for me...

> Is it safe to call 
> input_report_abs/input_report_key from interrupt context?

Yes.

> Anyway, driver 
> doesn't work for me with this patch (it works as mouse, but it doesn't as 
> touchscreen through tslib)

Hmm, does evtest utility report all expected events?

-- 
Dmitry

[PATCH 2/4] s3c24xx_ts: report touch only when stylus is down

[Vasily Khoruzhick]

? ????????? ?? 21 ??????? 2010 11:41:51 ????? Dmitry Torokhov ???????:
> > Timer is used to avoid reporting input events in interrupt context
> > (select callback is called from s3c_adc_try and from s3c_adc_irq),
> ??? This is quite a novel concept for me...

I think so, as I don't see any other reasons to use timer :)

> > Anyway, driver
> > doesn't work for me with this patch (it works as mouse, but it doesn't as
> > touchscreen through tslib)
> 
> Hmm, does evtest utility report all expected events?

evtest shows that driver starts event flood on first touch, i.e. it doesn't 
stop to emit event when stylus is up.

Regards
Vasily
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100221/70e2ae59/attachment.sig>

[PATCH 2/4] s3c24xx_ts: report touch only when stylus is down

[Dmitry Torokhov]

On Sun, Feb 21, 2010 at 12:10:08PM +0200, Vasily Khoruzhick wrote:
> ? ????????? ?? 21 ??????? 2010 11:41:51 ????? Dmitry Torokhov ???????:
> > > Timer is used to avoid reporting input events in interrupt context
> > > (select callback is called from s3c_adc_try and from s3c_adc_irq),
> > ??? This is quite a novel concept for me...
> 
> I think so, as I don't see any other reasons to use timer :)
> 
> > > Anyway, driver
> > > doesn't work for me with this patch (it works as mouse, but it doesn't as
> > > touchscreen through tslib)
> > 
> > Hmm, does evtest utility report all expected events?
> 
> evtest shows that driver starts event flood on first touch, i.e. it doesn't 
> stop to emit event when stylus is up.
> 

While flooding does it report BTN_TOUCH as up or down? I might have messed
s3c24xx_ts_check_pen_down()...

-- 
Dmitry

[PATCH 2/4] s3c24xx_ts: report touch only when stylus is down

[Vasily Khoruzhick]

? ????????? ?? 21 ??????? 2010 12:28:07 ????? Dmitry Torokhov ???????:

> While flooding does it report BTN_TOUCH as up or down? I might have messed
> s3c24xx_ts_check_pen_down()...

Nope, it reports coordinates, here's part of evtest output.

Regards,
Vasily

Event: time 1254477272.320125, type 3 (Absolute), code 0 (X), value 515
Event: time 1254477272.320158, type 3 (Absolute), code 1 (Y), value 496
Event: time 1254477272.320179, -------------- Report Sync ------------ 
Event: time 1254477272.333973, type 3 (Absolute), code 1 (Y), value 497
Event: time 1254477272.334012, -------------- Report Sync ------------ 
Event: time 1254477272.374775, type 3 (Absolute), code 0 (X), value 516
Event: time 1254477272.374824, -------------- Report Sync ------------ 
Event: time 1254477272.388559, type 3 (Absolute), code 0 (X), value 515
Event: time 1254477272.388591, type 3 (Absolute), code 1 (Y), value 498
Event: time 1254477272.388612, -------------- Report Sync ------------ 
Event: time 1254477272.402415, type 3 (Absolute), code 1 (Y), value 497
Event: time 1254477272.402456, -------------- Report Sync ------------ 
Event: time 1254477272.456858, type 3 (Absolute), code 1 (Y), value 496
Event: time 1254477272.456901, -------------- Report Sync ------------ 
Event: time 1254477272.470573, type 3 (Absolute), code 1 (Y), value 497
Event: time 1254477272.470615, -------------- Report Sync ------------ 
Event: time 1254477272.484288, type 3 (Absolute), code 1 (Y), value 498
Event: time 1254477272.484330, -------------- Report Sync ------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100221/2eff27e2/attachment.sig>

[PATCH 2/4] s3c24xx_ts: report touch only when stylus is down

[Nelson Castillo]

>> Hmm, does evtest utility report all expected events?
>
> evtest shows that driver starts event flood on first touch, i.e. it doesn't
> stop to emit event when stylus is up.

I remember I got bit by this also when I scheduled more conversions
without a timer; pen-up event would not get noticed. I didn't check
the specs to see if there was a way to configure the HW to do
otherwise.

[PATCH 2/4] s3c24xx_ts: report touch only when stylus is down

[Vasily Khoruzhick]

? ????????? ?? 21 ??????? 2010 16:27:13 ????? Nelson Castillo ???????:
> I remember I got bit by this also when I scheduled more conversions
> without a timer; pen-up event would not get noticed. I didn't check
> the specs to see if there was a way to configure the HW to do
> otherwise.

UPDOWN bits in ADCDAT0 and ADCDAT1 are valid only in WAIT4INT mode, it's wrong 
to check them in select callback because ADC is not in WAIT4INT mode.

Regards
Vasily
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100221/fb2d0a61/attachment.sig>

[PATCH 2/4] s3c24xx_ts: report touch only when stylus is down

[Dmitry Torokhov]

On Sun, Feb 21, 2010 at 11:44:34PM +0200, Vasily Khoruzhick wrote:
> ? ????????? ?? 21 ??????? 2010 16:27:13 ????? Nelson Castillo ???????:
> > I remember I got bit by this also when I scheduled more conversions
> > without a timer; pen-up event would not get noticed. I didn't check
> > the specs to see if there was a way to configure the HW to do
> > otherwise.
> 
> UPDOWN bits in ADCDAT0 and ADCDAT1 are valid only in WAIT4INT mode, it's wrong 
> to check them in select callback because ADC is not in WAIT4INT mode.

We do put it in WAIT4INT mode rigtht there though so it looks like we
just need to move that call (well the original doing WAIT4INT | INT_UP)
before we check for pen state.

-- 
Dmitry

[PATCH 2/4] s3c24xx_ts: report touch only when stylus is down

[Vasily Khoruzhick]

? ????????? ?? 22 ??????? 2010 00:51:25 ????? Dmitry Torokhov ???????:
> We do put it in WAIT4INT mode rigtht there though so it looks like we
> just need to move that call (well the original doing WAIT4INT | INT_UP)
> before we check for pen state.

And we need to disable WAIT4INT mode in s3c24xx_ts_schedule_read (actually 
according to spec we need to disable it right after we got interrupt).

Regards
Vasily
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100222/23ce84ed/attachment.sig>

[PATCH 2/4] s3c24xx_ts: report touch only when stylus is down

[Dmitry Torokhov]

On Mon, Feb 22, 2010 at 12:58:48AM +0200, Vasily Khoruzhick wrote:
> ? ????????? ?? 22 ??????? 2010 00:51:25 ????? Dmitry Torokhov ???????:
> > We do put it in WAIT4INT mode rigtht there though so it looks like we
> > just need to move that call (well the original doing WAIT4INT | INT_UP)
> > before we check for pen state.
> 
> And we need to disable WAIT4INT mode in s3c24xx_ts_schedule_read (actually 
> according to spec we need to disable it right after we got interrupt).
> 

So is the below sufficient to make it work? Even if it works aI think we
need to add socke locking around scheduling reads from irq and
callback...

-- 
Dmitry


Signed-off-by: Dmitry Torokhov <dtor@mail.ru>

---

 drivers/input/touchscreen/s3c2410_ts.c |   22 ++++++++++++++++++----
 1 files changed, 18 insertions(+), 4 deletions(-)


diff --git a/drivers/input/touchscreen/s3c2410_ts.c b/drivers/input/touchscreen/s3c2410_ts.c
index 60f18c2..b389648 100644
--- a/drivers/input/touchscreen/s3c2410_ts.c
+++ b/drivers/input/touchscreen/s3c2410_ts.c
@@ -152,9 +152,12 @@ static void s3c24xx_ts_schedule_read(void)
  */
 static irqreturn_t s3c24xx_ts_stylus_irq(int irq, void *dev_id)
 {
-	if (s3c24xx_ts_check_pen_down())
+	if (s3c24xx_ts_check_pen_down()) {
+		/* Reset WAIT4INT state */
+		writel(0, ts.io + S3C2410_ADCTSC);
+
 		s3c24xx_ts_schedule_read();
-	else
+	} else
 		dev_info(ts.dev, "%s: count=%d\n", __func__, ts.count);
 
 	return IRQ_HANDLED;
@@ -211,14 +214,25 @@ static void s3c24xx_ts_select(struct s3c_adc_client *client, unsigned select)
 		/*
 		 * Conversion is complete, we have desired number of samples.
 		 */
+
+		/*
+		 * We need to be in WAIT4INT mode to successfully check for
+		 * pen state.
+		 */
+		writel(WAIT4INT | INT_UP, ts.io + S3C2410_ADCTSC);
+
 		pen_is_down = s3c24xx_ts_check_pen_down();
 
 		s3c24xx_ts_report_state(pen_is_down);
 
-		if (pen_is_down)
+		if (pen_is_down) {
+			/* Reset WAIT4INT state */
+			writel(0, ts.io + S3C2410_ADCTSC);
+
 			s3c24xx_ts_schedule_read();
-		else
+		} else {
 			writel(WAIT4INT | INT_DOWN, ts.io + S3C2410_ADCTSC);
+		}
 	}
 }
 

[PATCH 2/4] s3c24xx_ts: report touch only when stylus is down

[Vasily Khoruzhick]

? ????????? ?? 22 ??????? 2010 09:14:03 ????? Dmitry Torokhov ???????:
 
> So is the below sufficient to make it work? Even if it works aI think we
> need to add socke locking around scheduling reads from irq and
> callback...

Sorry, still doesn't work.

Regards
Vasily
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100222/f4746ffa/attachment.sig>

[PATCH 2/4] s3c24xx_ts: report touch only when stylus is down

[Vasily Khoruzhick]

? ????????? ?? 22 ??????? 2010 09:14:03 ????? Dmitry Torokhov ???????:
> +               /* Reset WAIT4INT state */
> +               writel(0, ts.io + S3C2410_ADCTSC);

That's wrong, should be writel(S3C2410_ADCTSC_PULL_UP_DISABLE | AUTOPST, ts.io 
+ S3C2410_ADCTSC), anyway it doesn't work even with this fix

> -               if (pen_is_down)
> +               if (pen_is_down) {
> +                       /* Reset WAIT4INT state */
> +                       writel(0, ts.io + S3C2410_ADCTSC);

Same note.

Regards
Vasily
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100222/3a96c2de/attachment.sig>

[PATCH 3/4] s3c24xx_adc: disable/enable IRQ on suspend/resume

[Vasily Khoruzhick]

IRQ should be disabled on suspend and re-enabled on resume.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 arch/arm/plat-s3c24xx/adc.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-s3c24xx/adc.c b/arch/arm/plat-s3c24xx/adc.c
index ce47627..284f653 100644
--- a/arch/arm/plat-s3c24xx/adc.c
+++ b/arch/arm/plat-s3c24xx/adc.c
@@ -388,6 +388,7 @@ static int s3c_adc_suspend(struct platform_device *pdev, pm_message_t state)
 	con |= S3C2410_ADCCON_STDBM;
 	writel(con, adc->regs + S3C2410_ADCCON);
 
+	disable_irq(adc->irq);
 	clk_disable(adc->clk);
 
 	return 0;
@@ -398,6 +399,7 @@ static int s3c_adc_resume(struct platform_device *pdev)
 	struct adc_device *adc = platform_get_drvdata(pdev);
 
 	clk_enable(adc->clk);
+	enable_irq(adc->irq);
 
 	writel(adc->prescale | S3C2410_ADCCON_PRSCEN,
 	       adc->regs + S3C2410_ADCCON);
-- 
1.7.0

[PATCH 3/4] s3c24xx_adc: disable/enable IRQ on suspend/resume

[Ben Dooks]

On Thu, Feb 18, 2010 at 06:32:29PM +0200, Vasily Khoruzhick wrote:
> IRQ should be disabled on suspend and re-enabled on resume.
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>

Ok, added to next-s3c24xx-ts

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

[PATCH 4/4] s3c24xx_ts: re-enable IRQ on resume

[Vasily Khoruzhick]

IRQ should be enabled on resume, otherwise driver doesn't
report events.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/input/touchscreen/s3c2410_ts.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/input/touchscreen/s3c2410_ts.c b/drivers/input/touchscreen/s3c2410_ts.c
index e2fe2ed..6738c26 100644
--- a/drivers/input/touchscreen/s3c2410_ts.c
+++ b/drivers/input/touchscreen/s3c2410_ts.c
@@ -401,6 +401,7 @@ static int s3c2410ts_resume(struct device *dev)
 	struct s3c2410_ts_mach_info *info = pdev->dev.platform_data;
 
 	clk_enable(ts.clock);
+	enable_irq(ts.irq_tc);
 
 	/* Initialise registers */
 	if ((info->delay & 0xffff) > 0)
-- 
1.7.0

[PATCH 4/4] s3c24xx_ts: re-enable IRQ on resume

[Dmitry Torokhov]

On Thu, Feb 18, 2010 at 06:32:30PM +0200, Vasily Khoruzhick wrote:
> IRQ should be enabled on resume, otherwise driver doesn't
> report events.
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  drivers/input/touchscreen/s3c2410_ts.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/s3c2410_ts.c b/drivers/input/touchscreen/s3c2410_ts.c
> index e2fe2ed..6738c26 100644
> --- a/drivers/input/touchscreen/s3c2410_ts.c
> +++ b/drivers/input/touchscreen/s3c2410_ts.c
> @@ -401,6 +401,7 @@ static int s3c2410ts_resume(struct device *dev)
>  	struct s3c2410_ts_mach_info *info = pdev->dev.platform_data;
>  
>  	clk_enable(ts.clock);
> +	enable_irq(ts.irq_tc);
>  
>  	/* Initialise registers */
>  	if ((info->delay & 0xffff) > 0)

Applied to my 'next' branch, thank you Vasily.

-- 
Dmitry