[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Ben Dooks]

[-- Attachment #1: thirdparty/inprogress/s3c24xx-touchscreen.patch --]
[-- Type: TEXT/PLAIN, Size: 15952 bytes --]

From: Arnaud Patard <arnaud.patard@rtp-net.org>

S3C24XX touchscreen driver, originally written by Arnaud Patard and
other contributors. This driver is the version from the Simtec Electronics
tree, and as such is the best place to start and thus proposed to be
merged into the linux input system.

The driver has had substantial testing as well as a number of tidying
up passes done by Ben Dooks, as noted:

- added kernel-doc comments to most of the routines
- removed old code from pre adc framework days
- updated device probe code to use platform id list matching
- cleaned up debug, since printk() now has timestamp feature
- ensure code uses dev_() reporting macros where necessary

Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
Signed-off-by: Simtec Linux Team <linux@simtec.co.uk>
Signed-off-by: Ben Dooks <ben@simtec.co.uk>

---
 drivers/input/touchscreen/Kconfig      |   18 +
 drivers/input/touchscreen/Makefile     |    1 
 drivers/input/touchscreen/s3c2410_ts.c |  464 +++++++++++++++++++++++++++++++++
 3 files changed, 483 insertions(+)


Index: b/drivers/input/touchscreen/Kconfig
===================================================================
--- a/drivers/input/touchscreen/Kconfig	2009-11-09 22:28:23.000000000 +0000
+++ b/drivers/input/touchscreen/Kconfig	2009-11-10 10:38:44.000000000 +0000
@@ -133,6 +133,24 @@ config TOUCHSCREEN_FUJITSU
 	  To compile this driver as a module, choose M here: the
 	  module will be called fujitsu-ts.
 
+config TOUCHSCREEN_S3C2410
+	tristate "Samsung S3C2410 touchscreen input driver"
+	depends on ARCH_S3C2410 && INPUT && INPUT_TOUCHSCREEN
+	select SERIO
+	help
+	  Say Y here if you have the s3c2410 touchscreen.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called s3c2410_ts.
+
+config TOUCHSCREEN_S3C2410_DEBUG
+	boolean "Samsung S3C2410 touchscreen debug messages"
+	depends on TOUCHSCREEN_S3C2410
+	help
+	  Select this if you want debug messages
+
 config TOUCHSCREEN_GUNZE
 	tristate "Gunze AHL-51S touchscreen"
 	select SERIO
Index: b/drivers/input/touchscreen/Makefile
===================================================================
--- a/drivers/input/touchscreen/Makefile	2009-11-09 22:28:23.000000000 +0000
+++ b/drivers/input/touchscreen/Makefile	2009-11-10 10:38:44.000000000 +0000
@@ -26,6 +26,7 @@ obj-$(CONFIG_TOUCHSCREEN_HP7XX)		+= jorn
 obj-$(CONFIG_TOUCHSCREEN_HTCPEN)	+= htcpen.o
 obj-$(CONFIG_TOUCHSCREEN_USB_COMPOSITE)	+= usbtouchscreen.o
 obj-$(CONFIG_TOUCHSCREEN_PENMOUNT)	+= penmount.o
+obj-$(CONFIG_TOUCHSCREEN_S3C2410)	+= s3c2410_ts.o
 obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213)	+= touchit213.o
 obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT)	+= touchright.o
 obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN)	+= touchwin.o
Index: b/drivers/input/touchscreen/s3c2410_ts.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ b/drivers/input/touchscreen/s3c2410_ts.c	2009-11-10 10:47:38.000000000 +0000
@@ -0,0 +1,464 @@
+/* drivers/input/touchscreen/s3c2410_ts.c
+ *
+ * Samsung S3C24XX touchscreen driver
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the term of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Copyright 2004 Arnaud Patard <arnaud.patard@rtp-net.org>
+ * Copyright 2008 Ben Dooks <ben-linux@fluff.org>
+ * Copyright 2009 Simtec Electronics <linux@simtec.co.uk>
+ *
+ * Additional work by Herbert Pötzl <herbert@13thfloor.at> and
+ * Harald Welte <laforge@openmoko.org>
+ */
+
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/gpio.h>
+#include <linux/input.h>
+#include <linux/init.h>
+#include <linux/serio.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+
+#include <plat/adc.h>
+#include <plat/regs-adc.h>
+
+#include <mach/regs-gpio.h>
+#include <mach/ts.h>
+
+/* For ts.dev.id.version */
+#define S3C2410TSVERSION	0x0101
+
+#define TSC_SLEEP  (S3C2410_ADCTSC_PULL_UP_DISABLE | S3C2410_ADCTSC_XY_PST(0))
+
+#define INT_DOWN	(0)
+#define INT_UP		(1 << 8)
+
+#define WAIT4INT	(S3C2410_ADCTSC_YM_SEN | \
+			 S3C2410_ADCTSC_YP_SEN | \
+			 S3C2410_ADCTSC_XP_SEN | \
+			 S3C2410_ADCTSC_XY_PST(3))
+
+#define AUTOPST		(S3C2410_ADCTSC_YM_SEN | \
+			 S3C2410_ADCTSC_YP_SEN | \
+			 S3C2410_ADCTSC_XP_SEN | \
+			 S3C2410_ADCTSC_AUTO_PST | \
+			 S3C2410_ADCTSC_XY_PST(0))
+
+#define DEBUG_LVL    KERN_DEBUG
+
+static char *s3c2410ts_name = "s3c2410 TouchScreen";
+
+/* Per-touchscreen data. */
+
+/**
+ * struct s3c2410ts - driver touchscreen state.
+ * @client: The ADC client we registered with the core driver.
+ * @dev: The device we are bound to.
+ * @input: The input device we registered with the input subsystem.
+ * @clock: The clock for the adc.
+ * @io: Pointer to the IO base.
+ * @xp: The accumulated X position data.
+ * @yp: The accumulated Y position data.
+ * @irq_tc: The interrupt number for pen up/down interrupt
+ * @count: The number of samples collected.
+ * @shift: The log2 of the maximum count to read in one go.
+ */
+struct s3c2410ts {
+	struct s3c_adc_client *client;
+	struct device *dev;
+	struct input_dev *input;
+	struct clk *clock;
+	void __iomem *io;
+	unsigned long xp;
+	unsigned long yp;
+	int irq_tc;
+	int count;
+	int shift;
+};
+
+static struct s3c2410ts ts;
+
+/**
+ * s3c2410_ts_connect - configure gpio for s3c2410 systems
+ *
+ * Configure the GPIO for the S3C2410 system, where we have external FETs
+ * connected to the device (later systems such as the S3C2440 integrate
+ * these into the device).
+*/
+static inline void s3c2410_ts_connect(void)
+{
+	s3c2410_gpio_cfgpin(S3C2410_GPG(12), S3C2410_GPG12_XMON);
+	s3c2410_gpio_cfgpin(S3C2410_GPG(13), S3C2410_GPG13_nXPON);
+	s3c2410_gpio_cfgpin(S3C2410_GPG(14), S3C2410_GPG14_YMON);
+	s3c2410_gpio_cfgpin(S3C2410_GPG(15), S3C2410_GPG15_nYPON);
+}
+
+/**
+ * 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.
+ */
+static inline int get_down(unsigned long data0, unsigned long data1)
+{
+	/* returns true if both data values show stylus down */
+	return (!(data0 & S3C2410_ADCDAT0_UPDOWN) &&
+		!(data1 & S3C2410_ADCDAT0_UPDOWN));
+}
+
+static void touch_timer_fire(unsigned long data)
+{
+	unsigned long data0;
+	unsigned long data1;
+	int down;
+
+	data0 = readl(ts.io + S3C2410_ADCDAT0);
+	data1 = readl(ts.io + S3C2410_ADCDAT1);
+
+	down = get_down(data0, data1);
+
+	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);
+
+		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_report_abs(ts.input, ABS_PRESSURE, 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;
+
+		input_report_key(ts.input, BTN_TOUCH, 0);
+		input_report_abs(ts.input, ABS_PRESSURE, 0);
+		input_sync(ts.input);
+
+		writel(WAIT4INT | INT_DOWN, ts.io + S3C2410_ADCTSC);
+	}
+}
+
+static struct timer_list touch_timer = TIMER_INITIALIZER(touch_timer_fire, 0, 0);
+
+/**
+ * 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)
+{
+	unsigned long data0;
+	unsigned long data1;
+	int 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);
+	else
+		dev_info(ts.dev, "%s: count=%d\n", __func__, ts.count);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * s3c24xx_ts_conversion - ADC conversion callback
+ * @client: The client that was registered with the ADC core.
+ * @data0: The reading from ADCDAT0.
+ * @data1: The reading from ADCDAT1.
+ * @left: The number of samples left.
+ *
+ * Called when a conversion has finished.
+ */
+static void s3c24xx_ts_conversion(struct s3c_adc_client *client,
+				  unsigned data0, unsigned data1,
+				  unsigned *left)
+{
+	dev_dbg(ts.dev, "%s: %d,%d\n", __func__, data0, data1);
+
+	ts.xp += data0;
+	ts.yp += data1;
+
+	ts.count++;
+
+	/* From tests, it seems that it is unlikely to get a pen-up
+	 * event during the conversion process which means we can
+	 * ignore any pen-up events with less than the requisite
+	 * count done.
+	 *
+	 * In several thousand conversions, no pen-ups where detected
+	 * before count completed.
+	 */
+}
+
+/**
+ * s3c24xx_ts_select - ADC selection callback.
+ * @client: The client that was registered with the ADC core.
+ * @select: The reason for select.
+ *
+ * Called when the ADC core selects (or deslects) us as a client.
+ */
+static void s3c24xx_ts_select(struct s3c_adc_client *client, unsigned select)
+{
+	if (select) {
+		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);
+	}
+}
+
+/**
+ * s3c2410ts_probe - device core probe entry point
+ * @pdev: The device we are being bound to.
+ *
+ * Initialise, find and allocate any resources we need to run and then
+ * register with the ADC and input systems.
+ */
+static int __init s3c2410ts_probe(struct platform_device *pdev)
+{
+	struct s3c2410_ts_mach_info *info;
+	struct device *dev = &pdev->dev;
+	struct input_dev *input_dev;
+	struct resource *res;
+	int ret = -EINVAL;
+
+	/* Initialise input stuff */
+	memset(&ts, 0, sizeof(struct s3c2410ts));
+
+	ts.dev = dev;
+
+	info = pdev->dev.platform_data;
+	if (!info) {
+		dev_err(dev, "no platform data, cannot attach\n");
+		return -EINVAL;
+	}
+
+	dev_dbg(dev, "initialising touchscreen\n");
+
+	ts.clock = clk_get(dev, "adc");
+	if (IS_ERR(ts.clock)) {
+		dev_err(dev, "cannot get adc clock source\n");
+		return -ENOENT;
+	}
+
+	clk_enable(ts.clock);
+	dev_dbg(dev, "got and enabled clocks\n");
+
+	ts.irq_tc = ret = platform_get_irq(pdev, 0);
+	if (ret < 0) {
+		dev_err(dev, "no resource for interrupt\n");
+		goto err_clk;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "no resource for registers\n");
+		ret = -ENOENT;
+		goto err_clk;
+	}
+
+	ts.io = ioremap(res->start, resource_size(res));
+	if (ts.io == NULL) {
+		dev_err(dev, "cannot map registers\n");
+		ret = -ENOMEM;
+		goto err_clk;
+	}
+
+	/* Configure the touchscreen external FETs on the S3C2410 */
+	if (!platform_get_device_id(pdev)->driver_data)
+		s3c2410_ts_connect();
+
+	ts.client = s3c_adc_register(pdev, s3c24xx_ts_select,
+				     s3c24xx_ts_conversion, 1);
+	if (IS_ERR(ts.client)) {
+		dev_err(dev, "failed to register adc client\n");
+		ret = PTR_ERR(ts.client);
+		goto err_iomap;
+	}
+
+	/* Initialise registers */
+	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");
+		ret = -ENOMEM;
+		goto err_iomap;
+	}
+
+	ts.input = input_dev;
+	ts.input->evbit[0] = BIT_MASK(EV_SYN) | BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
+	ts.input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+	input_set_abs_params(ts.input, ABS_X, 0, 0x3FF, 0, 0);
+	input_set_abs_params(ts.input, ABS_Y, 0, 0x3FF, 0, 0);
+	input_set_abs_params(ts.input, ABS_PRESSURE, 0, 1, 0, 0);
+
+	ts.input->name = s3c2410ts_name;
+	ts.input->id.bustype = BUS_HOST;
+	ts.input->id.vendor = 0xDEAD;
+	ts.input->id.product = 0xBEEF;
+	ts.input->id.version = S3C2410TSVERSION;
+
+	ts.shift = info->oversampling_shift;
+
+	if (request_irq(ts.irq_tc, stylus_irq, IRQF_DISABLED,
+			"s3c2410_ts_pen", ts.input)) {
+		dev_err(dev, "cannot get TC interrupt\n");
+		ret = -EIO;
+		goto err_inputdev;
+	}
+
+	dev_info(dev, "driver attached, registering input device\n");
+
+	/* All went ok, so register to the input system */
+	ret = input_register_device(ts.input);
+	if (ret < 0) {
+		dev_err(dev, "failed to register input device\n");
+		ret = -EIO;
+		goto err_tcirq;
+	}
+
+	return 0;
+
+ err_tcirq:
+	free_irq(ts.irq_tc, ts.input);
+ err_inputdev:
+	input_unregister_device(ts.input);
+ err_iomap:
+	iounmap(ts.io);
+ err_clk:
+	clk_put(ts.clock);
+	return ret;
+}
+
+/**
+ * s3c2410ts_remove - device core removal entry point
+ * @pdev: The device we are being removed from.
+ *
+ * Free up our state ready to be removed.
+ */
+static int s3c2410ts_remove(struct platform_device *pdev)
+{
+	free_irq(ts.irq_tc, ts.input);
+
+	clk_disable(ts.clock);
+	clk_put(ts.clock);
+
+	input_unregister_device(ts.input);
+	iounmap(ts.io);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int s3c2410ts_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	writel(TSC_SLEEP, ts.io + S3C2410_ADCTSC);
+	disable_irq(ts.irq_tc);
+	clk_disable(ts.clock);
+
+	return 0;
+}
+
+static int s3c2410ts_resume(struct platform_device *pdev)
+{
+	struct s3c2410_ts_mach_info *info = pdev->dev.platform_data;
+
+	clk_enable(ts.clock);
+
+	/* Initialise registers */
+	if ((info->delay & 0xffff) > 0)
+		writel(info->delay & 0xffff, ts.io + S3C2410_ADCDLY);
+
+	writel(WAIT4INT | INT_DOWN, ts.io + S3C2410_ADCTSC);
+
+	return 0;
+}
+
+#else
+#define s3c2410ts_suspend NULL
+#define s3c2410ts_resume  NULL
+#endif
+
+static struct platform_device_id s3cts_driver_ids[] = {
+	{ "s3c2410-ts", 0 },
+	{ "s3c2440-ts", 1 },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, s3cts_driver_ids);
+
+static struct platform_driver s3c_ts_driver = {
+	.driver         = {
+		.name   = "s3c24xx-ts",
+		.owner  = THIS_MODULE,
+	},
+	.id_table	= s3cts_driver_ids,
+	.probe		= s3c2410ts_probe,
+	.remove		= s3c2410ts_remove,
+	.suspend	= s3c2410ts_suspend,
+	.resume		= s3c2410ts_resume,
+};
+
+static int __init s3c2410ts_init(void)
+{
+	return platform_driver_register(&s3c_ts_driver);
+}
+
+static void __exit s3c2410ts_exit(void)
+{
+	platform_driver_unregister(&s3c_ts_driver);
+}
+
+module_init(s3c2410ts_init);
+module_exit(s3c2410ts_exit);
+
+MODULE_AUTHOR("Arnaud Patard <arnaud.patard@rtp-net.org>, "
+	      "Ben Dooks <ben@simtec.co.uk>, "
+	      "Simtec Electronics <linux@simtec.co.uk>");
+MODULE_DESCRIPTION("S3C24XX Touchscreen driver");
+MODULE_LICENSE("GPL v2");

[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Ben Dooks]

An embedded and charset-unspecified text was scrubbed...
Name: thirdparty/inprogress/s3c24xx-touchscreen.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20091118/ebfeea77/attachment.el>

Re: [patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Ramax Lo]

2009/11/19 Ben Dooks <ben@simtec.co.uk>:
<snip>
> +static int __init s3c2410ts_probe(struct platform_device *pdev)
> +{
> +	struct s3c2410_ts_mach_info *info;
> +	struct device *dev = &pdev->dev;
> +	struct input_dev *input_dev;
> +	struct resource *res;
> +	int ret = -EINVAL;
> +
> +	/* Initialise input stuff */
> +	memset(&ts, 0, sizeof(struct s3c2410ts));
> +
> +	ts.dev = dev;
> +
> +	info = pdev->dev.platform_data;
> +	if (!info) {
> +		dev_err(dev, "no platform data, cannot attach\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(dev, "initialising touchscreen\n");
> +
> +	ts.clock = clk_get(dev, "adc");
> +	if (IS_ERR(ts.clock)) {
> +		dev_err(dev, "cannot get adc clock source\n");
> +		return -ENOENT;
> +	}
> +
> +	clk_enable(ts.clock);
> +	dev_dbg(dev, "got and enabled clocks\n");
> +
> +	ts.irq_tc = ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		dev_err(dev, "no resource for interrupt\n");
> +		goto err_clk;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "no resource for registers\n");
> +		ret = -ENOENT;
> +		goto err_clk;
> +	}
> +
> +	ts.io = ioremap(res->start, resource_size(res));
> +	if (ts.io == NULL) {
> +		dev_err(dev, "cannot map registers\n");
> +		ret = -ENOMEM;
> +		goto err_clk;
> +	}
> +
> +	/* Configure the touchscreen external FETs on the S3C2410 */
> +	if (!platform_get_device_id(pdev)->driver_data)
> +		s3c2410_ts_connect();
> +
> +	ts.client = s3c_adc_register(pdev, s3c24xx_ts_select,
> +				     s3c24xx_ts_conversion, 1);
> +	if (IS_ERR(ts.client)) {
> +		dev_err(dev, "failed to register adc client\n");
> +		ret = PTR_ERR(ts.client);
> +		goto err_iomap;
> +	}
> +
> +	/* Initialise registers */
> +	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");
> +		ret = -ENOMEM;
> +		goto err_iomap;
> +	}
> +
> +	ts.input = input_dev;
> +	ts.input->evbit[0] = BIT_MASK(EV_SYN) | BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> +	ts.input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> +	input_set_abs_params(ts.input, ABS_X, 0, 0x3FF, 0, 0);
> +	input_set_abs_params(ts.input, ABS_Y, 0, 0x3FF, 0, 0);
> +	input_set_abs_params(ts.input, ABS_PRESSURE, 0, 1, 0, 0);
> +
> +	ts.input->name = s3c2410ts_name;
> +	ts.input->id.bustype = BUS_HOST;
> +	ts.input->id.vendor = 0xDEAD;
> +	ts.input->id.product = 0xBEEF;
> +	ts.input->id.version = S3C2410TSVERSION;
> +
> +	ts.shift = info->oversampling_shift;
> +
> +	if (request_irq(ts.irq_tc, stylus_irq, IRQF_DISABLED,
> +			"s3c2410_ts_pen", ts.input)) {
> +		dev_err(dev, "cannot get TC interrupt\n");
> +		ret = -EIO;
> +		goto err_inputdev;
> +	}
> +
> +	dev_info(dev, "driver attached, registering input device\n");
> +
> +	/* All went ok, so register to the input system */
> +	ret = input_register_device(ts.input);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register input device\n");
> +		ret = -EIO;
> +		goto err_tcirq;
> +	}
> +
> +	return 0;
> +
> + err_tcirq:
> +	free_irq(ts.irq_tc, ts.input);
> + err_inputdev:
> +	input_unregister_device(ts.input);
> + err_iomap:
> +	iounmap(ts.io);

Isn't s3c_adc_release() needed for freeing unused adc client?

> + err_clk:
> +	clk_put(ts.clock);
> +	return ret;
> +}
> +
> +/**
> + * s3c2410ts_remove - device core removal entry point
> + * @pdev: The device we are being removed from.
> + *
> + * Free up our state ready to be removed.
> + */
> +static int s3c2410ts_remove(struct platform_device *pdev)
> +{
> +	free_irq(ts.irq_tc, ts.input);
> +
> +	clk_disable(ts.clock);
> +	clk_put(ts.clock);
> +
> +	input_unregister_device(ts.input);
> +	iounmap(ts.io);

The same reason.

> +
> +	return 0;
> +}

> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Ramax Lo]

2009/11/19 Ben Dooks <ben@simtec.co.uk>:
<snip>
> +static int __init s3c2410ts_probe(struct platform_device *pdev)
> +{
> +	struct s3c2410_ts_mach_info *info;
> +	struct device *dev = &pdev->dev;
> +	struct input_dev *input_dev;
> +	struct resource *res;
> +	int ret = -EINVAL;
> +
> +	/* Initialise input stuff */
> +	memset(&ts, 0, sizeof(struct s3c2410ts));
> +
> +	ts.dev = dev;
> +
> +	info = pdev->dev.platform_data;
> +	if (!info) {
> +		dev_err(dev, "no platform data, cannot attach\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(dev, "initialising touchscreen\n");
> +
> +	ts.clock = clk_get(dev, "adc");
> +	if (IS_ERR(ts.clock)) {
> +		dev_err(dev, "cannot get adc clock source\n");
> +		return -ENOENT;
> +	}
> +
> +	clk_enable(ts.clock);
> +	dev_dbg(dev, "got and enabled clocks\n");
> +
> +	ts.irq_tc = ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		dev_err(dev, "no resource for interrupt\n");
> +		goto err_clk;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "no resource for registers\n");
> +		ret = -ENOENT;
> +		goto err_clk;
> +	}
> +
> +	ts.io = ioremap(res->start, resource_size(res));
> +	if (ts.io == NULL) {
> +		dev_err(dev, "cannot map registers\n");
> +		ret = -ENOMEM;
> +		goto err_clk;
> +	}
> +
> +	/* Configure the touchscreen external FETs on the S3C2410 */
> +	if (!platform_get_device_id(pdev)->driver_data)
> +		s3c2410_ts_connect();
> +
> +	ts.client = s3c_adc_register(pdev, s3c24xx_ts_select,
> +				     s3c24xx_ts_conversion, 1);
> +	if (IS_ERR(ts.client)) {
> +		dev_err(dev, "failed to register adc client\n");
> +		ret = PTR_ERR(ts.client);
> +		goto err_iomap;
> +	}
> +
> +	/* Initialise registers */
> +	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");
> +		ret = -ENOMEM;
> +		goto err_iomap;
> +	}
> +
> +	ts.input = input_dev;
> +	ts.input->evbit[0] = BIT_MASK(EV_SYN) | BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> +	ts.input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> +	input_set_abs_params(ts.input, ABS_X, 0, 0x3FF, 0, 0);
> +	input_set_abs_params(ts.input, ABS_Y, 0, 0x3FF, 0, 0);
> +	input_set_abs_params(ts.input, ABS_PRESSURE, 0, 1, 0, 0);
> +
> +	ts.input->name = s3c2410ts_name;
> +	ts.input->id.bustype = BUS_HOST;
> +	ts.input->id.vendor = 0xDEAD;
> +	ts.input->id.product = 0xBEEF;
> +	ts.input->id.version = S3C2410TSVERSION;
> +
> +	ts.shift = info->oversampling_shift;
> +
> +	if (request_irq(ts.irq_tc, stylus_irq, IRQF_DISABLED,
> +			"s3c2410_ts_pen", ts.input)) {
> +		dev_err(dev, "cannot get TC interrupt\n");
> +		ret = -EIO;
> +		goto err_inputdev;
> +	}
> +
> +	dev_info(dev, "driver attached, registering input device\n");
> +
> +	/* All went ok, so register to the input system */
> +	ret = input_register_device(ts.input);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register input device\n");
> +		ret = -EIO;
> +		goto err_tcirq;
> +	}
> +
> +	return 0;
> +
> + err_tcirq:
> +	free_irq(ts.irq_tc, ts.input);
> + err_inputdev:
> +	input_unregister_device(ts.input);
> + err_iomap:
> +	iounmap(ts.io);

Isn't s3c_adc_release() needed for freeing unused adc client?

> + err_clk:
> +	clk_put(ts.clock);
> +	return ret;
> +}
> +
> +/**
> + * s3c2410ts_remove - device core removal entry point
> + * @pdev: The device we are being removed from.
> + *
> + * Free up our state ready to be removed.
> + */
> +static int s3c2410ts_remove(struct platform_device *pdev)
> +{
> +	free_irq(ts.irq_tc, ts.input);
> +
> +	clk_disable(ts.clock);
> +	clk_put(ts.clock);
> +
> +	input_unregister_device(ts.input);
> +	iounmap(ts.io);

The same reason.

> +
> +	return 0;
> +}

> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Re: [patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Dmitry Torokhov]

Hi Ben,

On Wed, Nov 18, 2009 at 11:29:40PM +0000, Ben Dooks wrote:
> From: Arnaud Patard <arnaud.patard@rtp-net.org>
> 
> S3C24XX touchscreen driver, originally written by Arnaud Patard and
> other contributors. This driver is the version from the Simtec Electronics
> tree, and as such is the best place to start and thus proposed to be
> merged into the linux input system.
> 

Thank you for the patch. First thing first - do we have an agreement
from all Samsung folks and other interested parties that _this_ is the
driver? Because it's like 3rd implementation that came across...

> The driver has had substantial testing as well as a number of tidying
> up passes done by Ben Dooks, as noted:
> 
> - added kernel-doc comments to most of the routines
> - removed old code from pre adc framework days
> - updated device probe code to use platform id list matching
> - cleaned up debug, since printk() now has timestamp feature
> - ensure code uses dev_() reporting macros where necessary
> 
> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
> Signed-off-by: Simtec Linux Team <linux@simtec.co.uk>
> Signed-off-by: Ben Dooks <ben@simtec.co.uk>
> 
> ---
>  drivers/input/touchscreen/Kconfig      |   18 +
>  drivers/input/touchscreen/Makefile     |    1 
>  drivers/input/touchscreen/s3c2410_ts.c |  464 +++++++++++++++++++++++++++++++++
>  3 files changed, 483 insertions(+)
> 
> 
> Index: b/drivers/input/touchscreen/Kconfig
> ===================================================================
> --- a/drivers/input/touchscreen/Kconfig	2009-11-09 22:28:23.000000000 +0000
> +++ b/drivers/input/touchscreen/Kconfig	2009-11-10 10:38:44.000000000 +0000
> @@ -133,6 +133,24 @@ config TOUCHSCREEN_FUJITSU
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called fujitsu-ts.
>  
> +config TOUCHSCREEN_S3C2410
> +	tristate "Samsung S3C2410 touchscreen input driver"
> +	depends on ARCH_S3C2410 && INPUT && INPUT_TOUCHSCREEN

INPUT && INPUT_TOUCHSCREEN are superfluous.

> +	select SERIO

I don't think you need SERIO

> +	help
> +	  Say Y here if you have the s3c2410 touchscreen.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called s3c2410_ts.
> +
> +config TOUCHSCREEN_S3C2410_DEBUG
> +	boolean "Samsung S3C2410 touchscreen debug messages"
> +	depends on TOUCHSCREEN_S3C2410
> +	help
> +	  Select this if you want debug messages

Where is this used?

> +
>  config TOUCHSCREEN_GUNZE
>  	tristate "Gunze AHL-51S touchscreen"
>  	select SERIO
> Index: b/drivers/input/touchscreen/Makefile
> ===================================================================
> --- a/drivers/input/touchscreen/Makefile	2009-11-09 22:28:23.000000000 +0000
> +++ b/drivers/input/touchscreen/Makefile	2009-11-10 10:38:44.000000000 +0000
> @@ -26,6 +26,7 @@ obj-$(CONFIG_TOUCHSCREEN_HP7XX)		+= jorn
>  obj-$(CONFIG_TOUCHSCREEN_HTCPEN)	+= htcpen.o
>  obj-$(CONFIG_TOUCHSCREEN_USB_COMPOSITE)	+= usbtouchscreen.o
>  obj-$(CONFIG_TOUCHSCREEN_PENMOUNT)	+= penmount.o
> +obj-$(CONFIG_TOUCHSCREEN_S3C2410)	+= s3c2410_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213)	+= touchit213.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT)	+= touchright.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN)	+= touchwin.o
> Index: b/drivers/input/touchscreen/s3c2410_ts.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ b/drivers/input/touchscreen/s3c2410_ts.c	2009-11-10 10:47:38.000000000 +0000
> @@ -0,0 +1,464 @@
> +/* drivers/input/touchscreen/s3c2410_ts.c
> + *
> + * Samsung S3C24XX touchscreen driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the term of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Copyright 2004 Arnaud Patard <arnaud.patard@rtp-net.org>
> + * Copyright 2008 Ben Dooks <ben-linux@fluff.org>
> + * Copyright 2009 Simtec Electronics <linux@simtec.co.uk>
> + *
> + * Additional work by Herbert Pötzl <herbert@13thfloor.at> and
> + * Harald Welte <laforge@openmoko.org>
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/gpio.h>
> +#include <linux/input.h>
> +#include <linux/init.h>
> +#include <linux/serio.h>

No serio in sight.

> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +
> +#include <plat/adc.h>
> +#include <plat/regs-adc.h>
> +
> +#include <mach/regs-gpio.h>
> +#include <mach/ts.h>
> +
> +/* For ts.dev.id.version */
> +#define S3C2410TSVERSION	0x0101
> +
> +#define TSC_SLEEP  (S3C2410_ADCTSC_PULL_UP_DISABLE | S3C2410_ADCTSC_XY_PST(0))
> +
> +#define INT_DOWN	(0)
> +#define INT_UP		(1 << 8)
> +
> +#define WAIT4INT	(S3C2410_ADCTSC_YM_SEN | \
> +			 S3C2410_ADCTSC_YP_SEN | \
> +			 S3C2410_ADCTSC_XP_SEN | \
> +			 S3C2410_ADCTSC_XY_PST(3))
> +
> +#define AUTOPST		(S3C2410_ADCTSC_YM_SEN | \
> +			 S3C2410_ADCTSC_YP_SEN | \
> +			 S3C2410_ADCTSC_XP_SEN | \
> +			 S3C2410_ADCTSC_AUTO_PST | \
> +			 S3C2410_ADCTSC_XY_PST(0))
> +
> +#define DEBUG_LVL    KERN_DEBUG

Don't seem to be used.

> +
> +static char *s3c2410ts_name = "s3c2410 TouchScreen";
> +
> +/* Per-touchscreen data. */
> +
> +/**
> + * struct s3c2410ts - driver touchscreen state.
> + * @client: The ADC client we registered with the core driver.
> + * @dev: The device we are bound to.
> + * @input: The input device we registered with the input subsystem.
> + * @clock: The clock for the adc.
> + * @io: Pointer to the IO base.
> + * @xp: The accumulated X position data.
> + * @yp: The accumulated Y position data.
> + * @irq_tc: The interrupt number for pen up/down interrupt
> + * @count: The number of samples collected.
> + * @shift: The log2 of the maximum count to read in one go.
> + */

These sructures are driver-internal and so don't need to be kernel-doc-ed.
Same goes for the driver-private functions.

> +struct s3c2410ts {
> +	struct s3c_adc_client *client;
> +	struct device *dev;
> +	struct input_dev *input;
> +	struct clk *clock;
> +	void __iomem *io;
> +	unsigned long xp;
> +	unsigned long yp;
> +	int irq_tc;
> +	int count;
> +	int shift;
> +};
> +
> +static struct s3c2410ts ts;
> +
> +/**
> + * s3c2410_ts_connect - configure gpio for s3c2410 systems
> + *
> + * Configure the GPIO for the S3C2410 system, where we have external FETs
> + * connected to the device (later systems such as the S3C2440 integrate
> + * these into the device).
> +*/
> +static inline void s3c2410_ts_connect(void)
> +{
> +	s3c2410_gpio_cfgpin(S3C2410_GPG(12), S3C2410_GPG12_XMON);
> +	s3c2410_gpio_cfgpin(S3C2410_GPG(13), S3C2410_GPG13_nXPON);
> +	s3c2410_gpio_cfgpin(S3C2410_GPG(14), S3C2410_GPG14_YMON);
> +	s3c2410_gpio_cfgpin(S3C2410_GPG(15), S3C2410_GPG15_nYPON);

No gpiolib? Also, should it be in the driver or maybe in the platform
code?

> +}
> +
> +/**
> + * 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.
> + */
> +static inline int get_down(unsigned long data0, unsigned long data1)

bool?

> +{
> +	/* returns true if both data values show stylus down */
> +	return (!(data0 & S3C2410_ADCDAT0_UPDOWN) &&
> +		!(data1 & S3C2410_ADCDAT0_UPDOWN));
> +}
> +
> +static void touch_timer_fire(unsigned long data)
> +{
> +	unsigned long data0;
> +	unsigned long data1;
> +	int down;

bool?

> +
> +	data0 = readl(ts.io + S3C2410_ADCDAT0);
> +	data1 = readl(ts.io + S3C2410_ADCDAT1);
> +
> +	down = get_down(data0, data1);
> +
> +	if (ts.count == (1<<ts.shift)) {

Syyle: x << y

> +		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);
> +
> +		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_report_abs(ts.input, ABS_PRESSURE, 1);

No fake pressure events please, BTN_TOUCH should be enough.

> +		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;
> +
> +		input_report_key(ts.input, BTN_TOUCH, 0);
> +		input_report_abs(ts.input, ABS_PRESSURE, 0);
> +		input_sync(ts.input);
> +
> +		writel(WAIT4INT | INT_DOWN, ts.io + S3C2410_ADCTSC);
> +	}
> +}
> +
> +static struct timer_list touch_timer = TIMER_INITIALIZER(touch_timer_fire, 0, 0);

static DEFINE_TIMER(...);

> +
> +/**
> + * 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)
> +{
> +	unsigned long data0;
> +	unsigned long data1;
> +	int 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);
> +	else
> +		dev_info(ts.dev, "%s: count=%d\n", __func__, ts.count);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/**
> + * s3c24xx_ts_conversion - ADC conversion callback
> + * @client: The client that was registered with the ADC core.
> + * @data0: The reading from ADCDAT0.
> + * @data1: The reading from ADCDAT1.
> + * @left: The number of samples left.
> + *
> + * Called when a conversion has finished.
> + */
> +static void s3c24xx_ts_conversion(struct s3c_adc_client *client,
> +				  unsigned data0, unsigned data1,
> +				  unsigned *left)
> +{
> +	dev_dbg(ts.dev, "%s: %d,%d\n", __func__, data0, data1);
> +
> +	ts.xp += data0;
> +	ts.yp += data1;
> +
> +	ts.count++;
> +
> +	/* From tests, it seems that it is unlikely to get a pen-up
> +	 * event during the conversion process which means we can
> +	 * ignore any pen-up events with less than the requisite
> +	 * count done.
> +	 *
> +	 * In several thousand conversions, no pen-ups where detected
> +	 * before count completed.
> +	 */
> +}
> +
> +/**
> + * s3c24xx_ts_select - ADC selection callback.
> + * @client: The client that was registered with the ADC core.
> + * @select: The reason for select.
> + *
> + * Called when the ADC core selects (or deslects) us as a client.
> + */
> +static void s3c24xx_ts_select(struct s3c_adc_client *client, unsigned select)
> +{
> +	if (select) {
> +		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);
> +	}
> +}
> +
> +/**
> + * s3c2410ts_probe - device core probe entry point
> + * @pdev: The device we are being bound to.
> + *
> + * Initialise, find and allocate any resources we need to run and then
> + * register with the ADC and input systems.
> + */
> +static int __init s3c2410ts_probe(struct platform_device *pdev)

__devinit or switch to platform_driver_probe().

> +{
> +	struct s3c2410_ts_mach_info *info;
> +	struct device *dev = &pdev->dev;
> +	struct input_dev *input_dev;
> +	struct resource *res;
> +	int ret = -EINVAL;

Can we call it "error" (since that's what you use it for).
> +
> +	/* Initialise input stuff */
> +	memset(&ts, 0, sizeof(struct s3c2410ts));
> +
> +	ts.dev = dev;
> +
> +	info = pdev->dev.platform_data;
> +	if (!info) {
> +		dev_err(dev, "no platform data, cannot attach\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(dev, "initialising touchscreen\n");
> +
> +	ts.clock = clk_get(dev, "adc");
> +	if (IS_ERR(ts.clock)) {
> +		dev_err(dev, "cannot get adc clock source\n");
> +		return -ENOENT;
> +	}
> +
> +	clk_enable(ts.clock);
> +	dev_dbg(dev, "got and enabled clocks\n");
> +
> +	ts.irq_tc = ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		dev_err(dev, "no resource for interrupt\n");
> +		goto err_clk;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "no resource for registers\n");
> +		ret = -ENOENT;
> +		goto err_clk;
> +	}
> +

request_mem_region() here?

> +	ts.io = ioremap(res->start, resource_size(res));
> +	if (ts.io == NULL) {
> +		dev_err(dev, "cannot map registers\n");
> +		ret = -ENOMEM;
> +		goto err_clk;
> +	}
> +
> +	/* Configure the touchscreen external FETs on the S3C2410 */
> +	if (!platform_get_device_id(pdev)->driver_data)
> +		s3c2410_ts_connect();
> +
> +	ts.client = s3c_adc_register(pdev, s3c24xx_ts_select,
> +				     s3c24xx_ts_conversion, 1);
> +	if (IS_ERR(ts.client)) {
> +		dev_err(dev, "failed to register adc client\n");
> +		ret = PTR_ERR(ts.client);
> +		goto err_iomap;
> +	}
> +
> +	/* Initialise registers */
> +	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");
> +		ret = -ENOMEM;
> +		goto err_iomap;
> +	}
> +
> +	ts.input = input_dev;
> +	ts.input->evbit[0] = BIT_MASK(EV_SYN) | BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);

No need setting EV_SYN explicitely.

> +	ts.input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> +	input_set_abs_params(ts.input, ABS_X, 0, 0x3FF, 0, 0);
> +	input_set_abs_params(ts.input, ABS_Y, 0, 0x3FF, 0, 0);
> +	input_set_abs_params(ts.input, ABS_PRESSURE, 0, 1, 0, 0);

Drop ABS_PRESSURE.

> +
> +	ts.input->name = s3c2410ts_name;
> +	ts.input->id.bustype = BUS_HOST;
> +	ts.input->id.vendor = 0xDEAD;
> +	ts.input->id.product = 0xBEEF;
> +	ts.input->id.version = S3C2410TSVERSION;
> +
> +	ts.shift = info->oversampling_shift;
> +
> +	if (request_irq(ts.irq_tc, stylus_irq, IRQF_DISABLED,
> +			"s3c2410_ts_pen", ts.input)) {
> +		dev_err(dev, "cannot get TC interrupt\n");
> +		ret = -EIO;

Don't mangle what request_irq returned.

> +		goto err_inputdev;
> +	}
> +
> +	dev_info(dev, "driver attached, registering input device\n");
> +
> +	/* All went ok, so register to the input system */
> +	ret = input_register_device(ts.input);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register input device\n");
> +		ret = -EIO;
> +		goto err_tcirq;
> +	}
> +
> +	return 0;
> +
> + err_tcirq:
> +	free_irq(ts.irq_tc, ts.input);

del_timer_sync().

> + err_inputdev:
> +	input_unregister_device(ts.input);
> + err_iomap:
> +	iounmap(ts.io);
> + err_clk:
> +	clk_put(ts.clock);
> +	return ret;
> +}
> +
> +/**
> + * s3c2410ts_remove - device core removal entry point
> + * @pdev: The device we are being removed from.
> + *
> + * Free up our state ready to be removed.
> + */
> +static int s3c2410ts_remove(struct platform_device *pdev)
> +{
> +	free_irq(ts.irq_tc, ts.input);

del_timer_sync().

> +
> +	clk_disable(ts.clock);
> +	clk_put(ts.clock);
> +
> +	input_unregister_device(ts.input);
> +	iounmap(ts.io);

release_mem_region()

> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int s3c2410ts_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	writel(TSC_SLEEP, ts.io + S3C2410_ADCTSC);
> +	disable_irq(ts.irq_tc);
> +	clk_disable(ts.clock);
> +
> +	return 0;
> +}
> +
> +static int s3c2410ts_resume(struct platform_device *pdev)
> +{
> +	struct s3c2410_ts_mach_info *info = pdev->dev.platform_data;
> +
> +	clk_enable(ts.clock);
> +
> +	/* Initialise registers */
> +	if ((info->delay & 0xffff) > 0)
> +		writel(info->delay & 0xffff, ts.io + S3C2410_ADCDLY);
> +
> +	writel(WAIT4INT | INT_DOWN, ts.io + S3C2410_ADCTSC);
> +
> +	return 0;
> +}
> +
> +#else
> +#define s3c2410ts_suspend NULL
> +#define s3c2410ts_resume  NULL
> +#endif
> +
> +static struct platform_device_id s3cts_driver_ids[] = {
> +	{ "s3c2410-ts", 0 },
> +	{ "s3c2440-ts", 1 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, s3cts_driver_ids);
> +
> +static struct platform_driver s3c_ts_driver = {
> +	.driver         = {
> +		.name   = "s3c24xx-ts",
> +		.owner  = THIS_MODULE,
> +	},
> +	.id_table	= s3cts_driver_ids,
> +	.probe		= s3c2410ts_probe,
> +	.remove		= s3c2410ts_remove,

__devexit_p()

> +	.suspend	= s3c2410ts_suspend,
> +	.resume		= s3c2410ts_resume,

Switch to pm_ops.

> +};
> +
> +static int __init s3c2410ts_init(void)
> +{
> +	return platform_driver_register(&s3c_ts_driver);
> +}
> +
> +static void __exit s3c2410ts_exit(void)
> +{
> +	platform_driver_unregister(&s3c_ts_driver);
> +}
> +
> +module_init(s3c2410ts_init);
> +module_exit(s3c2410ts_exit);
> +
> +MODULE_AUTHOR("Arnaud Patard <arnaud.patard@rtp-net.org>, "
> +	      "Ben Dooks <ben@simtec.co.uk>, "
> +	      "Simtec Electronics <linux@simtec.co.uk>");
> +MODULE_DESCRIPTION("S3C24XX Touchscreen driver");
> +MODULE_LICENSE("GPL v2");
> 

Thanks!

-- 
Dmitry

[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Dmitry Torokhov]

Hi Ben,

On Wed, Nov 18, 2009 at 11:29:40PM +0000, Ben Dooks wrote:
> From: Arnaud Patard <arnaud.patard@rtp-net.org>
> 
> S3C24XX touchscreen driver, originally written by Arnaud Patard and
> other contributors. This driver is the version from the Simtec Electronics
> tree, and as such is the best place to start and thus proposed to be
> merged into the linux input system.
> 

Thank you for the patch. First thing first - do we have an agreement
from all Samsung folks and other interested parties that _this_ is the
driver? Because it's like 3rd implementation that came across...

> The driver has had substantial testing as well as a number of tidying
> up passes done by Ben Dooks, as noted:
> 
> - added kernel-doc comments to most of the routines
> - removed old code from pre adc framework days
> - updated device probe code to use platform id list matching
> - cleaned up debug, since printk() now has timestamp feature
> - ensure code uses dev_() reporting macros where necessary
> 
> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
> Signed-off-by: Simtec Linux Team <linux@simtec.co.uk>
> Signed-off-by: Ben Dooks <ben@simtec.co.uk>
> 
> ---
>  drivers/input/touchscreen/Kconfig      |   18 +
>  drivers/input/touchscreen/Makefile     |    1 
>  drivers/input/touchscreen/s3c2410_ts.c |  464 +++++++++++++++++++++++++++++++++
>  3 files changed, 483 insertions(+)
> 
> 
> Index: b/drivers/input/touchscreen/Kconfig
> ===================================================================
> --- a/drivers/input/touchscreen/Kconfig	2009-11-09 22:28:23.000000000 +0000
> +++ b/drivers/input/touchscreen/Kconfig	2009-11-10 10:38:44.000000000 +0000
> @@ -133,6 +133,24 @@ config TOUCHSCREEN_FUJITSU
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called fujitsu-ts.
>  
> +config TOUCHSCREEN_S3C2410
> +	tristate "Samsung S3C2410 touchscreen input driver"
> +	depends on ARCH_S3C2410 && INPUT && INPUT_TOUCHSCREEN

INPUT && INPUT_TOUCHSCREEN are superfluous.

> +	select SERIO

I don't think you need SERIO

> +	help
> +	  Say Y here if you have the s3c2410 touchscreen.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called s3c2410_ts.
> +
> +config TOUCHSCREEN_S3C2410_DEBUG
> +	boolean "Samsung S3C2410 touchscreen debug messages"
> +	depends on TOUCHSCREEN_S3C2410
> +	help
> +	  Select this if you want debug messages

Where is this used?

> +
>  config TOUCHSCREEN_GUNZE
>  	tristate "Gunze AHL-51S touchscreen"
>  	select SERIO
> Index: b/drivers/input/touchscreen/Makefile
> ===================================================================
> --- a/drivers/input/touchscreen/Makefile	2009-11-09 22:28:23.000000000 +0000
> +++ b/drivers/input/touchscreen/Makefile	2009-11-10 10:38:44.000000000 +0000
> @@ -26,6 +26,7 @@ obj-$(CONFIG_TOUCHSCREEN_HP7XX)		+= jorn
>  obj-$(CONFIG_TOUCHSCREEN_HTCPEN)	+= htcpen.o
>  obj-$(CONFIG_TOUCHSCREEN_USB_COMPOSITE)	+= usbtouchscreen.o
>  obj-$(CONFIG_TOUCHSCREEN_PENMOUNT)	+= penmount.o
> +obj-$(CONFIG_TOUCHSCREEN_S3C2410)	+= s3c2410_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213)	+= touchit213.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT)	+= touchright.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN)	+= touchwin.o
> Index: b/drivers/input/touchscreen/s3c2410_ts.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ b/drivers/input/touchscreen/s3c2410_ts.c	2009-11-10 10:47:38.000000000 +0000
> @@ -0,0 +1,464 @@
> +/* drivers/input/touchscreen/s3c2410_ts.c
> + *
> + * Samsung S3C24XX touchscreen driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the term of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Copyright 2004 Arnaud Patard <arnaud.patard@rtp-net.org>
> + * Copyright 2008 Ben Dooks <ben-linux@fluff.org>
> + * Copyright 2009 Simtec Electronics <linux@simtec.co.uk>
> + *
> + * Additional work by Herbert P?tzl <herbert@13thfloor.at> and
> + * Harald Welte <laforge@openmoko.org>
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/gpio.h>
> +#include <linux/input.h>
> +#include <linux/init.h>
> +#include <linux/serio.h>

No serio in sight.

> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +
> +#include <plat/adc.h>
> +#include <plat/regs-adc.h>
> +
> +#include <mach/regs-gpio.h>
> +#include <mach/ts.h>
> +
> +/* For ts.dev.id.version */
> +#define S3C2410TSVERSION	0x0101
> +
> +#define TSC_SLEEP  (S3C2410_ADCTSC_PULL_UP_DISABLE | S3C2410_ADCTSC_XY_PST(0))
> +
> +#define INT_DOWN	(0)
> +#define INT_UP		(1 << 8)
> +
> +#define WAIT4INT	(S3C2410_ADCTSC_YM_SEN | \
> +			 S3C2410_ADCTSC_YP_SEN | \
> +			 S3C2410_ADCTSC_XP_SEN | \
> +			 S3C2410_ADCTSC_XY_PST(3))
> +
> +#define AUTOPST		(S3C2410_ADCTSC_YM_SEN | \
> +			 S3C2410_ADCTSC_YP_SEN | \
> +			 S3C2410_ADCTSC_XP_SEN | \
> +			 S3C2410_ADCTSC_AUTO_PST | \
> +			 S3C2410_ADCTSC_XY_PST(0))
> +
> +#define DEBUG_LVL    KERN_DEBUG

Don't seem to be used.

> +
> +static char *s3c2410ts_name = "s3c2410 TouchScreen";
> +
> +/* Per-touchscreen data. */
> +
> +/**
> + * struct s3c2410ts - driver touchscreen state.
> + * @client: The ADC client we registered with the core driver.
> + * @dev: The device we are bound to.
> + * @input: The input device we registered with the input subsystem.
> + * @clock: The clock for the adc.
> + * @io: Pointer to the IO base.
> + * @xp: The accumulated X position data.
> + * @yp: The accumulated Y position data.
> + * @irq_tc: The interrupt number for pen up/down interrupt
> + * @count: The number of samples collected.
> + * @shift: The log2 of the maximum count to read in one go.
> + */

These sructures are driver-internal and so don't need to be kernel-doc-ed.
Same goes for the driver-private functions.

> +struct s3c2410ts {
> +	struct s3c_adc_client *client;
> +	struct device *dev;
> +	struct input_dev *input;
> +	struct clk *clock;
> +	void __iomem *io;
> +	unsigned long xp;
> +	unsigned long yp;
> +	int irq_tc;
> +	int count;
> +	int shift;
> +};
> +
> +static struct s3c2410ts ts;
> +
> +/**
> + * s3c2410_ts_connect - configure gpio for s3c2410 systems
> + *
> + * Configure the GPIO for the S3C2410 system, where we have external FETs
> + * connected to the device (later systems such as the S3C2440 integrate
> + * these into the device).
> +*/
> +static inline void s3c2410_ts_connect(void)
> +{
> +	s3c2410_gpio_cfgpin(S3C2410_GPG(12), S3C2410_GPG12_XMON);
> +	s3c2410_gpio_cfgpin(S3C2410_GPG(13), S3C2410_GPG13_nXPON);
> +	s3c2410_gpio_cfgpin(S3C2410_GPG(14), S3C2410_GPG14_YMON);
> +	s3c2410_gpio_cfgpin(S3C2410_GPG(15), S3C2410_GPG15_nYPON);

No gpiolib? Also, should it be in the driver or maybe in the platform
code?

> +}
> +
> +/**
> + * 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.
> + */
> +static inline int get_down(unsigned long data0, unsigned long data1)

bool?

> +{
> +	/* returns true if both data values show stylus down */
> +	return (!(data0 & S3C2410_ADCDAT0_UPDOWN) &&
> +		!(data1 & S3C2410_ADCDAT0_UPDOWN));
> +}
> +
> +static void touch_timer_fire(unsigned long data)
> +{
> +	unsigned long data0;
> +	unsigned long data1;
> +	int down;

bool?

> +
> +	data0 = readl(ts.io + S3C2410_ADCDAT0);
> +	data1 = readl(ts.io + S3C2410_ADCDAT1);
> +
> +	down = get_down(data0, data1);
> +
> +	if (ts.count == (1<<ts.shift)) {

Syyle: x << y

> +		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);
> +
> +		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_report_abs(ts.input, ABS_PRESSURE, 1);

No fake pressure events please, BTN_TOUCH should be enough.

> +		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;
> +
> +		input_report_key(ts.input, BTN_TOUCH, 0);
> +		input_report_abs(ts.input, ABS_PRESSURE, 0);
> +		input_sync(ts.input);
> +
> +		writel(WAIT4INT | INT_DOWN, ts.io + S3C2410_ADCTSC);
> +	}
> +}
> +
> +static struct timer_list touch_timer = TIMER_INITIALIZER(touch_timer_fire, 0, 0);

static DEFINE_TIMER(...);

> +
> +/**
> + * 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)
> +{
> +	unsigned long data0;
> +	unsigned long data1;
> +	int 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);
> +	else
> +		dev_info(ts.dev, "%s: count=%d\n", __func__, ts.count);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/**
> + * s3c24xx_ts_conversion - ADC conversion callback
> + * @client: The client that was registered with the ADC core.
> + * @data0: The reading from ADCDAT0.
> + * @data1: The reading from ADCDAT1.
> + * @left: The number of samples left.
> + *
> + * Called when a conversion has finished.
> + */
> +static void s3c24xx_ts_conversion(struct s3c_adc_client *client,
> +				  unsigned data0, unsigned data1,
> +				  unsigned *left)
> +{
> +	dev_dbg(ts.dev, "%s: %d,%d\n", __func__, data0, data1);
> +
> +	ts.xp += data0;
> +	ts.yp += data1;
> +
> +	ts.count++;
> +
> +	/* From tests, it seems that it is unlikely to get a pen-up
> +	 * event during the conversion process which means we can
> +	 * ignore any pen-up events with less than the requisite
> +	 * count done.
> +	 *
> +	 * In several thousand conversions, no pen-ups where detected
> +	 * before count completed.
> +	 */
> +}
> +
> +/**
> + * s3c24xx_ts_select - ADC selection callback.
> + * @client: The client that was registered with the ADC core.
> + * @select: The reason for select.
> + *
> + * Called when the ADC core selects (or deslects) us as a client.
> + */
> +static void s3c24xx_ts_select(struct s3c_adc_client *client, unsigned select)
> +{
> +	if (select) {
> +		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);
> +	}
> +}
> +
> +/**
> + * s3c2410ts_probe - device core probe entry point
> + * @pdev: The device we are being bound to.
> + *
> + * Initialise, find and allocate any resources we need to run and then
> + * register with the ADC and input systems.
> + */
> +static int __init s3c2410ts_probe(struct platform_device *pdev)

__devinit or switch to platform_driver_probe().

> +{
> +	struct s3c2410_ts_mach_info *info;
> +	struct device *dev = &pdev->dev;
> +	struct input_dev *input_dev;
> +	struct resource *res;
> +	int ret = -EINVAL;

Can we call it "error" (since that's what you use it for).
> +
> +	/* Initialise input stuff */
> +	memset(&ts, 0, sizeof(struct s3c2410ts));
> +
> +	ts.dev = dev;
> +
> +	info = pdev->dev.platform_data;
> +	if (!info) {
> +		dev_err(dev, "no platform data, cannot attach\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(dev, "initialising touchscreen\n");
> +
> +	ts.clock = clk_get(dev, "adc");
> +	if (IS_ERR(ts.clock)) {
> +		dev_err(dev, "cannot get adc clock source\n");
> +		return -ENOENT;
> +	}
> +
> +	clk_enable(ts.clock);
> +	dev_dbg(dev, "got and enabled clocks\n");
> +
> +	ts.irq_tc = ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		dev_err(dev, "no resource for interrupt\n");
> +		goto err_clk;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "no resource for registers\n");
> +		ret = -ENOENT;
> +		goto err_clk;
> +	}
> +

request_mem_region() here?

> +	ts.io = ioremap(res->start, resource_size(res));
> +	if (ts.io == NULL) {
> +		dev_err(dev, "cannot map registers\n");
> +		ret = -ENOMEM;
> +		goto err_clk;
> +	}
> +
> +	/* Configure the touchscreen external FETs on the S3C2410 */
> +	if (!platform_get_device_id(pdev)->driver_data)
> +		s3c2410_ts_connect();
> +
> +	ts.client = s3c_adc_register(pdev, s3c24xx_ts_select,
> +				     s3c24xx_ts_conversion, 1);
> +	if (IS_ERR(ts.client)) {
> +		dev_err(dev, "failed to register adc client\n");
> +		ret = PTR_ERR(ts.client);
> +		goto err_iomap;
> +	}
> +
> +	/* Initialise registers */
> +	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");
> +		ret = -ENOMEM;
> +		goto err_iomap;
> +	}
> +
> +	ts.input = input_dev;
> +	ts.input->evbit[0] = BIT_MASK(EV_SYN) | BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);

No need setting EV_SYN explicitely.

> +	ts.input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> +	input_set_abs_params(ts.input, ABS_X, 0, 0x3FF, 0, 0);
> +	input_set_abs_params(ts.input, ABS_Y, 0, 0x3FF, 0, 0);
> +	input_set_abs_params(ts.input, ABS_PRESSURE, 0, 1, 0, 0);

Drop ABS_PRESSURE.

> +
> +	ts.input->name = s3c2410ts_name;
> +	ts.input->id.bustype = BUS_HOST;
> +	ts.input->id.vendor = 0xDEAD;
> +	ts.input->id.product = 0xBEEF;
> +	ts.input->id.version = S3C2410TSVERSION;
> +
> +	ts.shift = info->oversampling_shift;
> +
> +	if (request_irq(ts.irq_tc, stylus_irq, IRQF_DISABLED,
> +			"s3c2410_ts_pen", ts.input)) {
> +		dev_err(dev, "cannot get TC interrupt\n");
> +		ret = -EIO;

Don't mangle what request_irq returned.

> +		goto err_inputdev;
> +	}
> +
> +	dev_info(dev, "driver attached, registering input device\n");
> +
> +	/* All went ok, so register to the input system */
> +	ret = input_register_device(ts.input);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register input device\n");
> +		ret = -EIO;
> +		goto err_tcirq;
> +	}
> +
> +	return 0;
> +
> + err_tcirq:
> +	free_irq(ts.irq_tc, ts.input);

del_timer_sync().

> + err_inputdev:
> +	input_unregister_device(ts.input);
> + err_iomap:
> +	iounmap(ts.io);
> + err_clk:
> +	clk_put(ts.clock);
> +	return ret;
> +}
> +
> +/**
> + * s3c2410ts_remove - device core removal entry point
> + * @pdev: The device we are being removed from.
> + *
> + * Free up our state ready to be removed.
> + */
> +static int s3c2410ts_remove(struct platform_device *pdev)
> +{
> +	free_irq(ts.irq_tc, ts.input);

del_timer_sync().

> +
> +	clk_disable(ts.clock);
> +	clk_put(ts.clock);
> +
> +	input_unregister_device(ts.input);
> +	iounmap(ts.io);

release_mem_region()

> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int s3c2410ts_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	writel(TSC_SLEEP, ts.io + S3C2410_ADCTSC);
> +	disable_irq(ts.irq_tc);
> +	clk_disable(ts.clock);
> +
> +	return 0;
> +}
> +
> +static int s3c2410ts_resume(struct platform_device *pdev)
> +{
> +	struct s3c2410_ts_mach_info *info = pdev->dev.platform_data;
> +
> +	clk_enable(ts.clock);
> +
> +	/* Initialise registers */
> +	if ((info->delay & 0xffff) > 0)
> +		writel(info->delay & 0xffff, ts.io + S3C2410_ADCDLY);
> +
> +	writel(WAIT4INT | INT_DOWN, ts.io + S3C2410_ADCTSC);
> +
> +	return 0;
> +}
> +
> +#else
> +#define s3c2410ts_suspend NULL
> +#define s3c2410ts_resume  NULL
> +#endif
> +
> +static struct platform_device_id s3cts_driver_ids[] = {
> +	{ "s3c2410-ts", 0 },
> +	{ "s3c2440-ts", 1 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, s3cts_driver_ids);
> +
> +static struct platform_driver s3c_ts_driver = {
> +	.driver         = {
> +		.name   = "s3c24xx-ts",
> +		.owner  = THIS_MODULE,
> +	},
> +	.id_table	= s3cts_driver_ids,
> +	.probe		= s3c2410ts_probe,
> +	.remove		= s3c2410ts_remove,

__devexit_p()

> +	.suspend	= s3c2410ts_suspend,
> +	.resume		= s3c2410ts_resume,

Switch to pm_ops.

> +};
> +
> +static int __init s3c2410ts_init(void)
> +{
> +	return platform_driver_register(&s3c_ts_driver);
> +}
> +
> +static void __exit s3c2410ts_exit(void)
> +{
> +	platform_driver_unregister(&s3c_ts_driver);
> +}
> +
> +module_init(s3c2410ts_init);
> +module_exit(s3c2410ts_exit);
> +
> +MODULE_AUTHOR("Arnaud Patard <arnaud.patard@rtp-net.org>, "
> +	      "Ben Dooks <ben@simtec.co.uk>, "
> +	      "Simtec Electronics <linux@simtec.co.uk>");
> +MODULE_DESCRIPTION("S3C24XX Touchscreen driver");
> +MODULE_LICENSE("GPL v2");
> 

Thanks!

-- 
Dmitry

Re: [patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Shine Liu]

Hi,Ben

Besides Dmitry's comments,

On Wed, 2009-11-18 at 23:29 +0000, Ben Dooks wrote:

> +config TOUCHSCREEN_S3C2410
> +       tristate "Samsung S3C2410 touchscreen input driver"
> +       depends on ARCH_S3C2410 && INPUT && INPUT_TOUCHSCREEN
> +       select SERIO
> +       help
> +         Say Y here if you have the s3c2410 touchscreen.
> +
> +         If unsure, say N.
> +
> +         To compile this driver as a module, choose M here: the
> +         module will be called s3c2410_ts.

S3C24XX_ADC should be added to the depends.

> +#include <linux/slab.h>

I think this is not really needed because kernel.h is included already.


> +
> +	ts.clock = clk_get(dev, "adc");
> +	if (IS_ERR(ts.clock)) {
> +		dev_err(dev, "cannot get adc clock source\n");
> +		return -ENOENT;
> +	}
> +
> +	clk_enable(ts.clock);
> +	dev_dbg(dev, "got and enabled clocks\n");

Well, we take 2410_ts as s3c adc client, so these work has been done in
s3c_adc_probe function of arch/arm/plat-s3c24xx.c

This should be duplicated.

And "#include <linux/clk.h>" is not needed because we don't use these
APIs


> + err_clk:
> +       clk_put(ts.clock);
> +       return ret;
> +}

Above reason and also for those "goto err_clk;"

> );
> +
> +	clk_disable(ts.clock);
> +	clk_put(ts.clock);

Same as above

> +
> +#ifdef CONFIG_PM
> +static int s3c2410ts_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	writel(TSC_SLEEP, ts.io + S3C2410_ADCTSC);
> +	disable_irq(ts.irq_tc);

Same as above

> +
> +static int s3c2410ts_resume(struct platform_device *pdev)
> +{
> +	struct s3c2410_ts_mach_info *info = pdev->dev.platform_data;
> +
> +	clk_enable(ts.clock);

Same as above


Cheers,

Shine

[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Shine Liu]

Hi,Ben

Besides Dmitry's comments,

On Wed, 2009-11-18 at 23:29 +0000, Ben Dooks wrote:

> +config TOUCHSCREEN_S3C2410
> +       tristate "Samsung S3C2410 touchscreen input driver"
> +       depends on ARCH_S3C2410 && INPUT && INPUT_TOUCHSCREEN
> +       select SERIO
> +       help
> +         Say Y here if you have the s3c2410 touchscreen.
> +
> +         If unsure, say N.
> +
> +         To compile this driver as a module, choose M here: the
> +         module will be called s3c2410_ts.

S3C24XX_ADC should be added to the depends.

> +#include <linux/slab.h>

I think this is not really needed because kernel.h is included already.


> +
> +	ts.clock = clk_get(dev, "adc");
> +	if (IS_ERR(ts.clock)) {
> +		dev_err(dev, "cannot get adc clock source\n");
> +		return -ENOENT;
> +	}
> +
> +	clk_enable(ts.clock);
> +	dev_dbg(dev, "got and enabled clocks\n");

Well, we take 2410_ts as s3c adc client, so these work has been done in
s3c_adc_probe function of arch/arm/plat-s3c24xx.c

This should be duplicated.

And "#include <linux/clk.h>" is not needed because we don't use these
APIs


> + err_clk:
> +       clk_put(ts.clock);
> +       return ret;
> +}

Above reason and also for those "goto err_clk;"

> );
> +
> +	clk_disable(ts.clock);
> +	clk_put(ts.clock);

Same as above

> +
> +#ifdef CONFIG_PM
> +static int s3c2410ts_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	writel(TSC_SLEEP, ts.io + S3C2410_ADCTSC);
> +	disable_irq(ts.irq_tc);

Same as above

> +
> +static int s3c2410ts_resume(struct platform_device *pdev)
> +{
> +	struct s3c2410_ts_mach_info *info = pdev->dev.platform_data;
> +
> +	clk_enable(ts.clock);

Same as above


Cheers,

Shine

Re: [patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Harald Welte]

hi Dmitry,

On Wed, Nov 18, 2009 at 06:59:30PM -0800, Dmitry Torokhov wrote:

> Thank you for the patch. First thing first - do we have an agreement
> from all Samsung folks and other interested parties that _this_ is the
> driver? Because it's like 3rd implementation that came across...

I think it is important we finally merge _a_ driver, giving users a useful
touchscreen after many, many years of delay.  Whoever has issues with that
driver should simply address those issues and propose incremental patches to
it.

This is speaking with both my former Openmoko "hat" as well as considering
what is most important for Samsung System LSI.

Regards,
-- 
- Harald Welte <laforge@gnumonks.org>           http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
                                                  (ETSI EN 300 175-7 Ch. A6)

[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Harald Welte]

hi Dmitry,

On Wed, Nov 18, 2009 at 06:59:30PM -0800, Dmitry Torokhov wrote:

> Thank you for the patch. First thing first - do we have an agreement
> from all Samsung folks and other interested parties that _this_ is the
> driver? Because it's like 3rd implementation that came across...

I think it is important we finally merge _a_ driver, giving users a useful
touchscreen after many, many years of delay.  Whoever has issues with that
driver should simply address those issues and propose incremental patches to
it.

This is speaking with both my former Openmoko "hat" as well as considering
what is most important for Samsung System LSI.

Regards,
-- 
- Harald Welte <laforge@gnumonks.org>           http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
                                                  (ETSI EN 300 175-7 Ch. A6)

Re: [patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Shine Liu]

Hi Ben,

On Wed, 2009-11-18 at 18:59 -0800, Dmitry Torokhov wrote:

> > +#ifdef CONFIG_PM
> > +static int s3c2410ts_suspend(struct platform_device *pdev, pm_message_t state)
> > +{
> > +	writel(TSC_SLEEP, ts.io + S3C2410_ADCTSC);
> > +	disable_irq(ts.irq_tc);
> > +	clk_disable(ts.clock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int s3c2410ts_resume(struct platform_device *pdev)
> > +{
> > +	struct s3c2410_ts_mach_info *info = pdev->dev.platform_data;
> > +
> > +	clk_enable(ts.clock);
> > +
> > +	/* Initialise registers */
> > +	if ((info->delay & 0xffff) > 0)
> > +		writel(info->delay & 0xffff, ts.io + S3C2410_ADCDLY);
> > +
> > +	writel(WAIT4INT | INT_DOWN, ts.io + S3C2410_ADCTSC);
> > +
> > +	return 0;
> > +}
> > +
> > +#else
> > +#define s3c2410ts_suspend NULL
> > +#define s3c2410ts_resume  NULL
> > +#endif
> > +
> > +static struct platform_device_id s3cts_driver_ids[] = {
> > +	{ "s3c2410-ts", 0 },
> > +	{ "s3c2440-ts", 1 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(platform, s3cts_driver_ids);
> > +
> > +static struct platform_driver s3c_ts_driver = {
> > +	.driver         = {
> > +		.name   = "s3c24xx-ts",
> > +		.owner  = THIS_MODULE,
> > +	},
> > +	.id_table	= s3cts_driver_ids,
> > +	.probe		= s3c2410ts_probe,
> > +	.remove		= s3c2410ts_remove,
> 
> __devexit_p()
> 
> > +	.suspend	= s3c2410ts_suspend,
> > +	.resume		= s3c2410ts_resume,
> 
> Switch to pm_ops.

For these comments from Dmitry I have implemented in my patch last month
send to linux-input. You can refer
http://patchwork.kernel.org/patch/54933/ which maybe save some time for
you.


Regards,

Shine Liu

Re: [patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Vasily Khoruzhick]

[-- Attachment #1: Type: Text/Plain, Size: 373 bytes --]

В сообщении от 19 ноября 2009 1:29:40 автор Ben Dooks написал:
<skipped>
> +	/* Initialise registers */
> +	if ((info->delay & 0xffff) > 0)
> +		writel(info->delay & 0xffff, ts.io + S3C2410_ADCDLY);

As for me, it's not OK to modity ADC-related registers in touchscreen driver, 
adc driver should be modified to support delay.

Regards
Vasily

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Vasily Khoruzhick]

? ????????? ?? 19 ?????? 2009 1:29:40 ????? Ben Dooks ???????:
<skipped>
> +	/* Initialise registers */
> +	if ((info->delay & 0xffff) > 0)
> +		writel(info->delay & 0xffff, ts.io + S3C2410_ADCDLY);

As for me, it's not OK to modity ADC-related registers in touchscreen driver, 
adc driver should be modified to support delay.

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

Re: [patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Mark Brown]

On Thu, Nov 19, 2009 at 01:46:55PM +0800, Shine Liu wrote:
> On Wed, 2009-11-18 at 23:29 +0000, Ben Dooks wrote:

> > +config TOUCHSCREEN_S3C2410
> > +       tristate "Samsung S3C2410 touchscreen input driver"
> > +       depends on ARCH_S3C2410 && INPUT && INPUT_TOUCHSCREEN
> > +       select SERIO
> > +       help
> > +         Say Y here if you have the s3c2410 touchscreen.
> > +
> > +         If unsure, say N.
> > +
> > +         To compile this driver as a module, choose M here: the
> > +         module will be called s3c2410_ts.

> S3C24XX_ADC should be added to the depends.

It's probably more friendly to select rather than depend on it to avoid
the option being hidden.

[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Mark Brown]

On Thu, Nov 19, 2009 at 01:46:55PM +0800, Shine Liu wrote:
> On Wed, 2009-11-18 at 23:29 +0000, Ben Dooks wrote:

> > +config TOUCHSCREEN_S3C2410
> > +       tristate "Samsung S3C2410 touchscreen input driver"
> > +       depends on ARCH_S3C2410 && INPUT && INPUT_TOUCHSCREEN
> > +       select SERIO
> > +       help
> > +         Say Y here if you have the s3c2410 touchscreen.
> > +
> > +         If unsure, say N.
> > +
> > +         To compile this driver as a module, choose M here: the
> > +         module will be called s3c2410_ts.

> S3C24XX_ADC should be added to the depends.

It's probably more friendly to select rather than depend on it to avoid
the option being hidden.

Re: [patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Ben Dooks]

Dmitry Torokhov wrote:
> Hi Ben,
> 
> On Wed, Nov 18, 2009 at 11:29:40PM +0000, Ben Dooks wrote:
>> From: Arnaud Patard <arnaud.patard@rtp-net.org>
>>
>> S3C24XX touchscreen driver, originally written by Arnaud Patard and
>> other contributors. This driver is the version from the Simtec Electronics
>> tree, and as such is the best place to start and thus proposed to be
>> merged into the linux input system.
>>
> 
> Thank you for the patch. First thing first - do we have an agreement
> from all Samsung folks and other interested parties that _this_ is the
> driver? Because it's like 3rd implementation that came across...

I belive this is the best of the current driver bases, and hopefully
after this round of tidying will definitely be the best.

>> The driver has had substantial testing as well as a number of tidying
>> up passes done by Ben Dooks, as noted:
>>
>> - added kernel-doc comments to most of the routines
>> - removed old code from pre adc framework days
>> - updated device probe code to use platform id list matching
>> - cleaned up debug, since printk() now has timestamp feature
>> - ensure code uses dev_() reporting macros where necessary
>>
>> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
>> Signed-off-by: Simtec Linux Team <linux@simtec.co.uk>
>> Signed-off-by: Ben Dooks <ben@simtec.co.uk>
>>
>> ---
>>  drivers/input/touchscreen/Kconfig      |   18 +
>>  drivers/input/touchscreen/Makefile     |    1 
>>  drivers/input/touchscreen/s3c2410_ts.c |  464 +++++++++++++++++++++++++++++++++
>>  3 files changed, 483 insertions(+)
>>
>>
>> Index: b/drivers/input/touchscreen/Kconfig
>> ===================================================================
>> --- a/drivers/input/touchscreen/Kconfig	2009-11-09 22:28:23.000000000 +0000
>> +++ b/drivers/input/touchscreen/Kconfig	2009-11-10 10:38:44.000000000 +0000
>> @@ -133,6 +133,24 @@ config TOUCHSCREEN_FUJITSU
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called fujitsu-ts.
>>  
>> +config TOUCHSCREEN_S3C2410
>> +	tristate "Samsung S3C2410 touchscreen input driver"
>> +	depends on ARCH_S3C2410 && INPUT && INPUT_TOUCHSCREEN
> 
> INPUT && INPUT_TOUCHSCREEN are superfluous.

ok, removed

>> +	select SERIO

removed

> I don't think you need SERIO
> 
>> +	help
>> +	  Say Y here if you have the s3c2410 touchscreen.
>> +
>> +	  If unsure, say N.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called s3c2410_ts.
>> +
>> +config TOUCHSCREEN_S3C2410_DEBUG
>> +	boolean "Samsung S3C2410 touchscreen debug messages"
>> +	depends on TOUCHSCREEN_S3C2410
>> +	help
>> +	  Select this if you want debug messages
> 
> Where is this used?

thought it was used to define DEBUG in the driver, will look
at removing this.

>> +
>>  config TOUCHSCREEN_GUNZE
>>  	tristate "Gunze AHL-51S touchscreen"
>>  	select SERIO
>> Index: b/drivers/input/touchscreen/Makefile
>> ===================================================================
>> --- a/drivers/input/touchscreen/Makefile	2009-11-09 22:28:23.000000000 +0000
>> +++ b/drivers/input/touchscreen/Makefile	2009-11-10 10:38:44.000000000 +0000
>> @@ -26,6 +26,7 @@ obj-$(CONFIG_TOUCHSCREEN_HP7XX)		+= jorn
>>  obj-$(CONFIG_TOUCHSCREEN_HTCPEN)	+= htcpen.o
>>  obj-$(CONFIG_TOUCHSCREEN_USB_COMPOSITE)	+= usbtouchscreen.o
>>  obj-$(CONFIG_TOUCHSCREEN_PENMOUNT)	+= penmount.o
>> +obj-$(CONFIG_TOUCHSCREEN_S3C2410)	+= s3c2410_ts.o
>>  obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213)	+= touchit213.o
>>  obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT)	+= touchright.o
>>  obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN)	+= touchwin.o
>> Index: b/drivers/input/touchscreen/s3c2410_ts.c
>> ===================================================================
>> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
>> +++ b/drivers/input/touchscreen/s3c2410_ts.c	2009-11-10 10:47:38.000000000 +0000
>> @@ -0,0 +1,464 @@
>> +/* drivers/input/touchscreen/s3c2410_ts.c
>> + *
>> + * Samsung S3C24XX touchscreen driver
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the term of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>> + *
>> + * Copyright 2004 Arnaud Patard <arnaud.patard@rtp-net.org>
>> + * Copyright 2008 Ben Dooks <ben-linux@fluff.org>
>> + * Copyright 2009 Simtec Electronics <linux@simtec.co.uk>
>> + *
>> + * Additional work by Herbert Pötzl <herbert@13thfloor.at> and
>> + * Harald Welte <laforge@openmoko.org>
>> + */
>> +
>> +#include <linux/errno.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/gpio.h>
>> +#include <linux/input.h>
>> +#include <linux/init.h>
>> +#include <linux/serio.h>
> 
> No serio in sight.

thanks, removed.

>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +
>> +#include <plat/adc.h>
>> +#include <plat/regs-adc.h>
>> +
>> +#include <mach/regs-gpio.h>
>> +#include <mach/ts.h>
>> +
>> +/* For ts.dev.id.version */
>> +#define S3C2410TSVERSION	0x0101
>> +
>> +#define TSC_SLEEP  (S3C2410_ADCTSC_PULL_UP_DISABLE | S3C2410_ADCTSC_XY_PST(0))
>> +
>> +#define INT_DOWN	(0)
>> +#define INT_UP		(1 << 8)
>> +
>> +#define WAIT4INT	(S3C2410_ADCTSC_YM_SEN | \
>> +			 S3C2410_ADCTSC_YP_SEN | \
>> +			 S3C2410_ADCTSC_XP_SEN | \
>> +			 S3C2410_ADCTSC_XY_PST(3))
>> +
>> +#define AUTOPST		(S3C2410_ADCTSC_YM_SEN | \
>> +			 S3C2410_ADCTSC_YP_SEN | \
>> +			 S3C2410_ADCTSC_XP_SEN | \
>> +			 S3C2410_ADCTSC_AUTO_PST | \
>> +			 S3C2410_ADCTSC_XY_PST(0))
>> +
>> +#define DEBUG_LVL    KERN_DEBUG
> 
> Don't seem to be used.

thanks, removed.

>> +
>> +static char *s3c2410ts_name = "s3c2410 TouchScreen";
>> +
>> +/* Per-touchscreen data. */
>> +
>> +/**
>> + * struct s3c2410ts - driver touchscreen state.
>> + * @client: The ADC client we registered with the core driver.
>> + * @dev: The device we are bound to.
>> + * @input: The input device we registered with the input subsystem.
>> + * @clock: The clock for the adc.
>> + * @io: Pointer to the IO base.
>> + * @xp: The accumulated X position data.
>> + * @yp: The accumulated Y position data.
>> + * @irq_tc: The interrupt number for pen up/down interrupt
>> + * @count: The number of samples collected.
>> + * @shift: The log2 of the maximum count to read in one go.
>> + */
> 
> These sructures are driver-internal and so don't need to be kernel-doc-ed.
> Same goes for the driver-private functions.

I like having the documentation, and I would much prefer to leave it
in as useful.

>> +struct s3c2410ts {
>> +	struct s3c_adc_client *client;
>> +	struct device *dev;
>> +	struct input_dev *input;
>> +	struct clk *clock;
>> +	void __iomem *io;
>> +	unsigned long xp;
>> +	unsigned long yp;
>> +	int irq_tc;
>> +	int count;
>> +	int shift;
>> +};
>> +
>> +static struct s3c2410ts ts;
>> +
>> +/**
>> + * s3c2410_ts_connect - configure gpio for s3c2410 systems
>> + *
>> + * Configure the GPIO for the S3C2410 system, where we have external FETs
>> + * connected to the device (later systems such as the S3C2440 integrate
>> + * these into the device).
>> +*/
>> +static inline void s3c2410_ts_connect(void)
>> +{
>> +	s3c2410_gpio_cfgpin(S3C2410_GPG(12), S3C2410_GPG12_XMON);
>> +	s3c2410_gpio_cfgpin(S3C2410_GPG(13), S3C2410_GPG13_nXPON);
>> +	s3c2410_gpio_cfgpin(S3C2410_GPG(14), S3C2410_GPG14_YMON);
>> +	s3c2410_gpio_cfgpin(S3C2410_GPG(15), S3C2410_GPG15_nYPON);
> 
> No gpiolib? Also, should it be in the driver or maybe in the platform
> code?

gpiolib doesn't deal with pin function configuration past input
or output. All these gpios need to be in their special-function
mode.

If people really object, this can be removed and the machine
support files changed.

>> +}
>> +
>> +/**
>> + * 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.
>> + */
>> +static inline int get_down(unsigned long data0, unsigned long data1)
> 
> bool?

thanks, fixed.

>> +{
>> +	/* returns true if both data values show stylus down */
>> +	return (!(data0 & S3C2410_ADCDAT0_UPDOWN) &&
>> +		!(data1 & S3C2410_ADCDAT0_UPDOWN));
>> +}
>> +
>> +static void touch_timer_fire(unsigned long data)
>> +{
>> +	unsigned long data0;
>> +	unsigned long data1;
>> +	int down;
> 
> bool?

thanks, fixed.

>> +
>> +	data0 = readl(ts.io + S3C2410_ADCDAT0);
>> +	data1 = readl(ts.io + S3C2410_ADCDAT1);
>> +
>> +	down = get_down(data0, data1);
>> +
>> +	if (ts.count == (1<<ts.shift)) {
> 
> Syyle: x << y

thanks, fixed.

>> +		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);
>> +
>> +		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_report_abs(ts.input, ABS_PRESSURE, 1);
> 
> No fake pressure events please, BTN_TOUCH should be enough.

I'd have to check, IIRC tslib needs these to function properly.

>> +		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;
>> +
>> +		input_report_key(ts.input, BTN_TOUCH, 0);
>> +		input_report_abs(ts.input, ABS_PRESSURE, 0);
>> +		input_sync(ts.input);
>> +
>> +		writel(WAIT4INT | INT_DOWN, ts.io + S3C2410_ADCTSC);
>> +	}
>> +}
>> +
>> +static struct timer_list touch_timer = TIMER_INITIALIZER(touch_timer_fire, 0, 0);
> 
> static DEFINE_TIMER(...);

ok, thanks.

>> +
>> +/**
>> + * 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)
>> +{
>> +	unsigned long data0;
>> +	unsigned long data1;
>> +	int 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);
>> +	else
>> +		dev_info(ts.dev, "%s: count=%d\n", __func__, ts.count);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +/**
>> + * s3c24xx_ts_conversion - ADC conversion callback
>> + * @client: The client that was registered with the ADC core.
>> + * @data0: The reading from ADCDAT0.
>> + * @data1: The reading from ADCDAT1.
>> + * @left: The number of samples left.
>> + *
>> + * Called when a conversion has finished.
>> + */
>> +static void s3c24xx_ts_conversion(struct s3c_adc_client *client,
>> +				  unsigned data0, unsigned data1,
>> +				  unsigned *left)
>> +{
>> +	dev_dbg(ts.dev, "%s: %d,%d\n", __func__, data0, data1);
>> +
>> +	ts.xp += data0;
>> +	ts.yp += data1;
>> +
>> +	ts.count++;
>> +
>> +	/* From tests, it seems that it is unlikely to get a pen-up
>> +	 * event during the conversion process which means we can
>> +	 * ignore any pen-up events with less than the requisite
>> +	 * count done.
>> +	 *
>> +	 * In several thousand conversions, no pen-ups where detected
>> +	 * before count completed.
>> +	 */
>> +}
>> +
>> +/**
>> + * s3c24xx_ts_select - ADC selection callback.
>> + * @client: The client that was registered with the ADC core.
>> + * @select: The reason for select.
>> + *
>> + * Called when the ADC core selects (or deslects) us as a client.
>> + */
>> +static void s3c24xx_ts_select(struct s3c_adc_client *client, unsigned select)
>> +{
>> +	if (select) {
>> +		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);
>> +	}
>> +}
>> +
>> +/**
>> + * s3c2410ts_probe - device core probe entry point
>> + * @pdev: The device we are being bound to.
>> + *
>> + * Initialise, find and allocate any resources we need to run and then
>> + * register with the ADC and input systems.
>> + */
>> +static int __init s3c2410ts_probe(struct platform_device *pdev)
> 
> __devinit or switch to platform_driver_probe().

thanks, fixed.

>> +{
>> +	struct s3c2410_ts_mach_info *info;
>> +	struct device *dev = &pdev->dev;
>> +	struct input_dev *input_dev;
>> +	struct resource *res;
>> +	int ret = -EINVAL;
> 
> Can we call it "error" (since that's what you use it for).

Is it really necessary to change this?

>> +
>> +	/* Initialise input stuff */
>> +	memset(&ts, 0, sizeof(struct s3c2410ts));
>> +
>> +	ts.dev = dev;
>> +
>> +	info = pdev->dev.platform_data;
>> +	if (!info) {
>> +		dev_err(dev, "no platform data, cannot attach\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	dev_dbg(dev, "initialising touchscreen\n");
>> +
>> +	ts.clock = clk_get(dev, "adc");
>> +	if (IS_ERR(ts.clock)) {
>> +		dev_err(dev, "cannot get adc clock source\n");
>> +		return -ENOENT;
>> +	}
>> +
>> +	clk_enable(ts.clock);
>> +	dev_dbg(dev, "got and enabled clocks\n");
>> +
>> +	ts.irq_tc = ret = platform_get_irq(pdev, 0);
>> +	if (ret < 0) {
>> +		dev_err(dev, "no resource for interrupt\n");
>> +		goto err_clk;
>> +	}
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		dev_err(dev, "no resource for registers\n");
>> +		ret = -ENOENT;
>> +		goto err_clk;
>> +	}
>> +
> 
> request_mem_region() here?

I'll check if it possible for both the adc and this driver
to claim this region.

>> +	ts.io = ioremap(res->start, resource_size(res));
>> +	if (ts.io == NULL) {
>> +		dev_err(dev, "cannot map registers\n");
>> +		ret = -ENOMEM;
>> +		goto err_clk;
>> +	}
>> +
>> +	/* Configure the touchscreen external FETs on the S3C2410 */
>> +	if (!platform_get_device_id(pdev)->driver_data)
>> +		s3c2410_ts_connect();
>> +
>> +	ts.client = s3c_adc_register(pdev, s3c24xx_ts_select,
>> +				     s3c24xx_ts_conversion, 1);
>> +	if (IS_ERR(ts.client)) {
>> +		dev_err(dev, "failed to register adc client\n");
>> +		ret = PTR_ERR(ts.client);
>> +		goto err_iomap;
>> +	}
>> +
>> +	/* Initialise registers */
>> +	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");
>> +		ret = -ENOMEM;
>> +		goto err_iomap;
>> +	}
>> +
>> +	ts.input = input_dev;
>> +	ts.input->evbit[0] = BIT_MASK(EV_SYN) | BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> 
> No need setting EV_SYN explicitely.

ok, fixed.

>> +	ts.input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
>> +	input_set_abs_params(ts.input, ABS_X, 0, 0x3FF, 0, 0);
>> +	input_set_abs_params(ts.input, ABS_Y, 0, 0x3FF, 0, 0);
>> +	input_set_abs_params(ts.input, ABS_PRESSURE, 0, 1, 0, 0);
> 
> Drop ABS_PRESSURE.

ok, see above.

>> +
>> +	ts.input->name = s3c2410ts_name;
>> +	ts.input->id.bustype = BUS_HOST;
>> +	ts.input->id.vendor = 0xDEAD;
>> +	ts.input->id.product = 0xBEEF;
>> +	ts.input->id.version = S3C2410TSVERSION;
>> +
>> +	ts.shift = info->oversampling_shift;
>> +
>> +	if (request_irq(ts.irq_tc, stylus_irq, IRQF_DISABLED,
>> +			"s3c2410_ts_pen", ts.input)) {
>> +		dev_err(dev, "cannot get TC interrupt\n");
>> +		ret = -EIO;
> 
> Don't mangle what request_irq returned.

ok, fixed.


>> +		goto err_inputdev;
>> +	}
>> +
>> +	dev_info(dev, "driver attached, registering input device\n");
>> +
>> +	/* All went ok, so register to the input system */
>> +	ret = input_register_device(ts.input);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to register input device\n");
>> +		ret = -EIO;
>> +		goto err_tcirq;
>> +	}
>> +
>> +	return 0;
>> +
>> + err_tcirq:
>> +	free_irq(ts.irq_tc, ts.input);
> 
> del_timer_sync().

added.

>> + err_inputdev:
>> +	input_unregister_device(ts.input);
>> + err_iomap:
>> +	iounmap(ts.io);
>> + err_clk:
>> +	clk_put(ts.clock);
>> +	return ret;
>> +}
>> +
>> +/**
>> + * s3c2410ts_remove - device core removal entry point
>> + * @pdev: The device we are being removed from.
>> + *
>> + * Free up our state ready to be removed.
>> + */
>> +static int s3c2410ts_remove(struct platform_device *pdev)
>> +{
>> +	free_irq(ts.irq_tc, ts.input);
> 
> del_timer_sync().

added, thanks.

>> +
>> +	clk_disable(ts.clock);
>> +	clk_put(ts.clock);
>> +
>> +	input_unregister_device(ts.input);
>> +	iounmap(ts.io);
> 
> release_mem_region()

see above.

>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int s3c2410ts_suspend(struct platform_device *pdev, pm_message_t state)
>> +{
>> +	writel(TSC_SLEEP, ts.io + S3C2410_ADCTSC);
>> +	disable_irq(ts.irq_tc);
>> +	clk_disable(ts.clock);
>> +
>> +	return 0;
>> +}
>> +
>> +static int s3c2410ts_resume(struct platform_device *pdev)
>> +{
>> +	struct s3c2410_ts_mach_info *info = pdev->dev.platform_data;
>> +
>> +	clk_enable(ts.clock);
>> +
>> +	/* Initialise registers */
>> +	if ((info->delay & 0xffff) > 0)
>> +		writel(info->delay & 0xffff, ts.io + S3C2410_ADCDLY);
>> +
>> +	writel(WAIT4INT | INT_DOWN, ts.io + S3C2410_ADCTSC);
>> +
>> +	return 0;
>> +}
>> +
>> +#else
>> +#define s3c2410ts_suspend NULL
>> +#define s3c2410ts_resume  NULL
>> +#endif
>> +
>> +static struct platform_device_id s3cts_driver_ids[] = {
>> +	{ "s3c2410-ts", 0 },
>> +	{ "s3c2440-ts", 1 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(platform, s3cts_driver_ids);
>> +
>> +static struct platform_driver s3c_ts_driver = {
>> +	.driver         = {
>> +		.name   = "s3c24xx-ts",
>> +		.owner  = THIS_MODULE,
>> +	},
>> +	.id_table	= s3cts_driver_ids,
>> +	.probe		= s3c2410ts_probe,
>> +	.remove		= s3c2410ts_remove,
> 
> __devexit_p()
> 
>> +	.suspend	= s3c2410ts_suspend,
>> +	.resume		= s3c2410ts_resume,
> 
> Switch to pm_ops.

ok, will do. may as well remove the #ifdef CONFIG_PM
for such small amount of code too.

>> +};
>> +
>> +static int __init s3c2410ts_init(void)
>> +{
>> +	return platform_driver_register(&s3c_ts_driver);
>> +}
>> +
>> +static void __exit s3c2410ts_exit(void)
>> +{
>> +	platform_driver_unregister(&s3c_ts_driver);
>> +}
>> +
>> +module_init(s3c2410ts_init);
>> +module_exit(s3c2410ts_exit);
>> +
>> +MODULE_AUTHOR("Arnaud Patard <arnaud.patard@rtp-net.org>, "
>> +	      "Ben Dooks <ben@simtec.co.uk>, "
>> +	      "Simtec Electronics <linux@simtec.co.uk>");
>> +MODULE_DESCRIPTION("S3C24XX Touchscreen driver");
>> +MODULE_LICENSE("GPL v2");
>>
> 
> Thanks!

I'll try and work on the to-do items and do another round
of testing before the weekend.

[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Ben Dooks]

Dmitry Torokhov wrote:
> Hi Ben,
> 
> On Wed, Nov 18, 2009 at 11:29:40PM +0000, Ben Dooks wrote:
>> From: Arnaud Patard <arnaud.patard@rtp-net.org>
>>
>> S3C24XX touchscreen driver, originally written by Arnaud Patard and
>> other contributors. This driver is the version from the Simtec Electronics
>> tree, and as such is the best place to start and thus proposed to be
>> merged into the linux input system.
>>
> 
> Thank you for the patch. First thing first - do we have an agreement
> from all Samsung folks and other interested parties that _this_ is the
> driver? Because it's like 3rd implementation that came across...

I belive this is the best of the current driver bases, and hopefully
after this round of tidying will definitely be the best.

>> The driver has had substantial testing as well as a number of tidying
>> up passes done by Ben Dooks, as noted:
>>
>> - added kernel-doc comments to most of the routines
>> - removed old code from pre adc framework days
>> - updated device probe code to use platform id list matching
>> - cleaned up debug, since printk() now has timestamp feature
>> - ensure code uses dev_() reporting macros where necessary
>>
>> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
>> Signed-off-by: Simtec Linux Team <linux@simtec.co.uk>
>> Signed-off-by: Ben Dooks <ben@simtec.co.uk>
>>
>> ---
>>  drivers/input/touchscreen/Kconfig      |   18 +
>>  drivers/input/touchscreen/Makefile     |    1 
>>  drivers/input/touchscreen/s3c2410_ts.c |  464 +++++++++++++++++++++++++++++++++
>>  3 files changed, 483 insertions(+)
>>
>>
>> Index: b/drivers/input/touchscreen/Kconfig
>> ===================================================================
>> --- a/drivers/input/touchscreen/Kconfig	2009-11-09 22:28:23.000000000 +0000
>> +++ b/drivers/input/touchscreen/Kconfig	2009-11-10 10:38:44.000000000 +0000
>> @@ -133,6 +133,24 @@ config TOUCHSCREEN_FUJITSU
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called fujitsu-ts.
>>  
>> +config TOUCHSCREEN_S3C2410
>> +	tristate "Samsung S3C2410 touchscreen input driver"
>> +	depends on ARCH_S3C2410 && INPUT && INPUT_TOUCHSCREEN
> 
> INPUT && INPUT_TOUCHSCREEN are superfluous.

ok, removed

>> +	select SERIO

removed

> I don't think you need SERIO
> 
>> +	help
>> +	  Say Y here if you have the s3c2410 touchscreen.
>> +
>> +	  If unsure, say N.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called s3c2410_ts.
>> +
>> +config TOUCHSCREEN_S3C2410_DEBUG
>> +	boolean "Samsung S3C2410 touchscreen debug messages"
>> +	depends on TOUCHSCREEN_S3C2410
>> +	help
>> +	  Select this if you want debug messages
> 
> Where is this used?

thought it was used to define DEBUG in the driver, will look
at removing this.

>> +
>>  config TOUCHSCREEN_GUNZE
>>  	tristate "Gunze AHL-51S touchscreen"
>>  	select SERIO
>> Index: b/drivers/input/touchscreen/Makefile
>> ===================================================================
>> --- a/drivers/input/touchscreen/Makefile	2009-11-09 22:28:23.000000000 +0000
>> +++ b/drivers/input/touchscreen/Makefile	2009-11-10 10:38:44.000000000 +0000
>> @@ -26,6 +26,7 @@ obj-$(CONFIG_TOUCHSCREEN_HP7XX)		+= jorn
>>  obj-$(CONFIG_TOUCHSCREEN_HTCPEN)	+= htcpen.o
>>  obj-$(CONFIG_TOUCHSCREEN_USB_COMPOSITE)	+= usbtouchscreen.o
>>  obj-$(CONFIG_TOUCHSCREEN_PENMOUNT)	+= penmount.o
>> +obj-$(CONFIG_TOUCHSCREEN_S3C2410)	+= s3c2410_ts.o
>>  obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213)	+= touchit213.o
>>  obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT)	+= touchright.o
>>  obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN)	+= touchwin.o
>> Index: b/drivers/input/touchscreen/s3c2410_ts.c
>> ===================================================================
>> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
>> +++ b/drivers/input/touchscreen/s3c2410_ts.c	2009-11-10 10:47:38.000000000 +0000
>> @@ -0,0 +1,464 @@
>> +/* drivers/input/touchscreen/s3c2410_ts.c
>> + *
>> + * Samsung S3C24XX touchscreen driver
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the term of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>> + *
>> + * Copyright 2004 Arnaud Patard <arnaud.patard@rtp-net.org>
>> + * Copyright 2008 Ben Dooks <ben-linux@fluff.org>
>> + * Copyright 2009 Simtec Electronics <linux@simtec.co.uk>
>> + *
>> + * Additional work by Herbert P?tzl <herbert@13thfloor.at> and
>> + * Harald Welte <laforge@openmoko.org>
>> + */
>> +
>> +#include <linux/errno.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/gpio.h>
>> +#include <linux/input.h>
>> +#include <linux/init.h>
>> +#include <linux/serio.h>
> 
> No serio in sight.

thanks, removed.

>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +
>> +#include <plat/adc.h>
>> +#include <plat/regs-adc.h>
>> +
>> +#include <mach/regs-gpio.h>
>> +#include <mach/ts.h>
>> +
>> +/* For ts.dev.id.version */
>> +#define S3C2410TSVERSION	0x0101
>> +
>> +#define TSC_SLEEP  (S3C2410_ADCTSC_PULL_UP_DISABLE | S3C2410_ADCTSC_XY_PST(0))
>> +
>> +#define INT_DOWN	(0)
>> +#define INT_UP		(1 << 8)
>> +
>> +#define WAIT4INT	(S3C2410_ADCTSC_YM_SEN | \
>> +			 S3C2410_ADCTSC_YP_SEN | \
>> +			 S3C2410_ADCTSC_XP_SEN | \
>> +			 S3C2410_ADCTSC_XY_PST(3))
>> +
>> +#define AUTOPST		(S3C2410_ADCTSC_YM_SEN | \
>> +			 S3C2410_ADCTSC_YP_SEN | \
>> +			 S3C2410_ADCTSC_XP_SEN | \
>> +			 S3C2410_ADCTSC_AUTO_PST | \
>> +			 S3C2410_ADCTSC_XY_PST(0))
>> +
>> +#define DEBUG_LVL    KERN_DEBUG
> 
> Don't seem to be used.

thanks, removed.

>> +
>> +static char *s3c2410ts_name = "s3c2410 TouchScreen";
>> +
>> +/* Per-touchscreen data. */
>> +
>> +/**
>> + * struct s3c2410ts - driver touchscreen state.
>> + * @client: The ADC client we registered with the core driver.
>> + * @dev: The device we are bound to.
>> + * @input: The input device we registered with the input subsystem.
>> + * @clock: The clock for the adc.
>> + * @io: Pointer to the IO base.
>> + * @xp: The accumulated X position data.
>> + * @yp: The accumulated Y position data.
>> + * @irq_tc: The interrupt number for pen up/down interrupt
>> + * @count: The number of samples collected.
>> + * @shift: The log2 of the maximum count to read in one go.
>> + */
> 
> These sructures are driver-internal and so don't need to be kernel-doc-ed.
> Same goes for the driver-private functions.

I like having the documentation, and I would much prefer to leave it
in as useful.

>> +struct s3c2410ts {
>> +	struct s3c_adc_client *client;
>> +	struct device *dev;
>> +	struct input_dev *input;
>> +	struct clk *clock;
>> +	void __iomem *io;
>> +	unsigned long xp;
>> +	unsigned long yp;
>> +	int irq_tc;
>> +	int count;
>> +	int shift;
>> +};
>> +
>> +static struct s3c2410ts ts;
>> +
>> +/**
>> + * s3c2410_ts_connect - configure gpio for s3c2410 systems
>> + *
>> + * Configure the GPIO for the S3C2410 system, where we have external FETs
>> + * connected to the device (later systems such as the S3C2440 integrate
>> + * these into the device).
>> +*/
>> +static inline void s3c2410_ts_connect(void)
>> +{
>> +	s3c2410_gpio_cfgpin(S3C2410_GPG(12), S3C2410_GPG12_XMON);
>> +	s3c2410_gpio_cfgpin(S3C2410_GPG(13), S3C2410_GPG13_nXPON);
>> +	s3c2410_gpio_cfgpin(S3C2410_GPG(14), S3C2410_GPG14_YMON);
>> +	s3c2410_gpio_cfgpin(S3C2410_GPG(15), S3C2410_GPG15_nYPON);
> 
> No gpiolib? Also, should it be in the driver or maybe in the platform
> code?

gpiolib doesn't deal with pin function configuration past input
or output. All these gpios need to be in their special-function
mode.

If people really object, this can be removed and the machine
support files changed.

>> +}
>> +
>> +/**
>> + * 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.
>> + */
>> +static inline int get_down(unsigned long data0, unsigned long data1)
> 
> bool?

thanks, fixed.

>> +{
>> +	/* returns true if both data values show stylus down */
>> +	return (!(data0 & S3C2410_ADCDAT0_UPDOWN) &&
>> +		!(data1 & S3C2410_ADCDAT0_UPDOWN));
>> +}
>> +
>> +static void touch_timer_fire(unsigned long data)
>> +{
>> +	unsigned long data0;
>> +	unsigned long data1;
>> +	int down;
> 
> bool?

thanks, fixed.

>> +
>> +	data0 = readl(ts.io + S3C2410_ADCDAT0);
>> +	data1 = readl(ts.io + S3C2410_ADCDAT1);
>> +
>> +	down = get_down(data0, data1);
>> +
>> +	if (ts.count == (1<<ts.shift)) {
> 
> Syyle: x << y

thanks, fixed.

>> +		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);
>> +
>> +		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_report_abs(ts.input, ABS_PRESSURE, 1);
> 
> No fake pressure events please, BTN_TOUCH should be enough.

I'd have to check, IIRC tslib needs these to function properly.

>> +		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;
>> +
>> +		input_report_key(ts.input, BTN_TOUCH, 0);
>> +		input_report_abs(ts.input, ABS_PRESSURE, 0);
>> +		input_sync(ts.input);
>> +
>> +		writel(WAIT4INT | INT_DOWN, ts.io + S3C2410_ADCTSC);
>> +	}
>> +}
>> +
>> +static struct timer_list touch_timer = TIMER_INITIALIZER(touch_timer_fire, 0, 0);
> 
> static DEFINE_TIMER(...);

ok, thanks.

>> +
>> +/**
>> + * 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)
>> +{
>> +	unsigned long data0;
>> +	unsigned long data1;
>> +	int 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);
>> +	else
>> +		dev_info(ts.dev, "%s: count=%d\n", __func__, ts.count);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +/**
>> + * s3c24xx_ts_conversion - ADC conversion callback
>> + * @client: The client that was registered with the ADC core.
>> + * @data0: The reading from ADCDAT0.
>> + * @data1: The reading from ADCDAT1.
>> + * @left: The number of samples left.
>> + *
>> + * Called when a conversion has finished.
>> + */
>> +static void s3c24xx_ts_conversion(struct s3c_adc_client *client,
>> +				  unsigned data0, unsigned data1,
>> +				  unsigned *left)
>> +{
>> +	dev_dbg(ts.dev, "%s: %d,%d\n", __func__, data0, data1);
>> +
>> +	ts.xp += data0;
>> +	ts.yp += data1;
>> +
>> +	ts.count++;
>> +
>> +	/* From tests, it seems that it is unlikely to get a pen-up
>> +	 * event during the conversion process which means we can
>> +	 * ignore any pen-up events with less than the requisite
>> +	 * count done.
>> +	 *
>> +	 * In several thousand conversions, no pen-ups where detected
>> +	 * before count completed.
>> +	 */
>> +}
>> +
>> +/**
>> + * s3c24xx_ts_select - ADC selection callback.
>> + * @client: The client that was registered with the ADC core.
>> + * @select: The reason for select.
>> + *
>> + * Called when the ADC core selects (or deslects) us as a client.
>> + */
>> +static void s3c24xx_ts_select(struct s3c_adc_client *client, unsigned select)
>> +{
>> +	if (select) {
>> +		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);
>> +	}
>> +}
>> +
>> +/**
>> + * s3c2410ts_probe - device core probe entry point
>> + * @pdev: The device we are being bound to.
>> + *
>> + * Initialise, find and allocate any resources we need to run and then
>> + * register with the ADC and input systems.
>> + */
>> +static int __init s3c2410ts_probe(struct platform_device *pdev)
> 
> __devinit or switch to platform_driver_probe().

thanks, fixed.

>> +{
>> +	struct s3c2410_ts_mach_info *info;
>> +	struct device *dev = &pdev->dev;
>> +	struct input_dev *input_dev;
>> +	struct resource *res;
>> +	int ret = -EINVAL;
> 
> Can we call it "error" (since that's what you use it for).

Is it really necessary to change this?

>> +
>> +	/* Initialise input stuff */
>> +	memset(&ts, 0, sizeof(struct s3c2410ts));
>> +
>> +	ts.dev = dev;
>> +
>> +	info = pdev->dev.platform_data;
>> +	if (!info) {
>> +		dev_err(dev, "no platform data, cannot attach\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	dev_dbg(dev, "initialising touchscreen\n");
>> +
>> +	ts.clock = clk_get(dev, "adc");
>> +	if (IS_ERR(ts.clock)) {
>> +		dev_err(dev, "cannot get adc clock source\n");
>> +		return -ENOENT;
>> +	}
>> +
>> +	clk_enable(ts.clock);
>> +	dev_dbg(dev, "got and enabled clocks\n");
>> +
>> +	ts.irq_tc = ret = platform_get_irq(pdev, 0);
>> +	if (ret < 0) {
>> +		dev_err(dev, "no resource for interrupt\n");
>> +		goto err_clk;
>> +	}
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		dev_err(dev, "no resource for registers\n");
>> +		ret = -ENOENT;
>> +		goto err_clk;
>> +	}
>> +
> 
> request_mem_region() here?

I'll check if it possible for both the adc and this driver
to claim this region.

>> +	ts.io = ioremap(res->start, resource_size(res));
>> +	if (ts.io == NULL) {
>> +		dev_err(dev, "cannot map registers\n");
>> +		ret = -ENOMEM;
>> +		goto err_clk;
>> +	}
>> +
>> +	/* Configure the touchscreen external FETs on the S3C2410 */
>> +	if (!platform_get_device_id(pdev)->driver_data)
>> +		s3c2410_ts_connect();
>> +
>> +	ts.client = s3c_adc_register(pdev, s3c24xx_ts_select,
>> +				     s3c24xx_ts_conversion, 1);
>> +	if (IS_ERR(ts.client)) {
>> +		dev_err(dev, "failed to register adc client\n");
>> +		ret = PTR_ERR(ts.client);
>> +		goto err_iomap;
>> +	}
>> +
>> +	/* Initialise registers */
>> +	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");
>> +		ret = -ENOMEM;
>> +		goto err_iomap;
>> +	}
>> +
>> +	ts.input = input_dev;
>> +	ts.input->evbit[0] = BIT_MASK(EV_SYN) | BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> 
> No need setting EV_SYN explicitely.

ok, fixed.

>> +	ts.input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
>> +	input_set_abs_params(ts.input, ABS_X, 0, 0x3FF, 0, 0);
>> +	input_set_abs_params(ts.input, ABS_Y, 0, 0x3FF, 0, 0);
>> +	input_set_abs_params(ts.input, ABS_PRESSURE, 0, 1, 0, 0);
> 
> Drop ABS_PRESSURE.

ok, see above.

>> +
>> +	ts.input->name = s3c2410ts_name;
>> +	ts.input->id.bustype = BUS_HOST;
>> +	ts.input->id.vendor = 0xDEAD;
>> +	ts.input->id.product = 0xBEEF;
>> +	ts.input->id.version = S3C2410TSVERSION;
>> +
>> +	ts.shift = info->oversampling_shift;
>> +
>> +	if (request_irq(ts.irq_tc, stylus_irq, IRQF_DISABLED,
>> +			"s3c2410_ts_pen", ts.input)) {
>> +		dev_err(dev, "cannot get TC interrupt\n");
>> +		ret = -EIO;
> 
> Don't mangle what request_irq returned.

ok, fixed.


>> +		goto err_inputdev;
>> +	}
>> +
>> +	dev_info(dev, "driver attached, registering input device\n");
>> +
>> +	/* All went ok, so register to the input system */
>> +	ret = input_register_device(ts.input);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to register input device\n");
>> +		ret = -EIO;
>> +		goto err_tcirq;
>> +	}
>> +
>> +	return 0;
>> +
>> + err_tcirq:
>> +	free_irq(ts.irq_tc, ts.input);
> 
> del_timer_sync().

added.

>> + err_inputdev:
>> +	input_unregister_device(ts.input);
>> + err_iomap:
>> +	iounmap(ts.io);
>> + err_clk:
>> +	clk_put(ts.clock);
>> +	return ret;
>> +}
>> +
>> +/**
>> + * s3c2410ts_remove - device core removal entry point
>> + * @pdev: The device we are being removed from.
>> + *
>> + * Free up our state ready to be removed.
>> + */
>> +static int s3c2410ts_remove(struct platform_device *pdev)
>> +{
>> +	free_irq(ts.irq_tc, ts.input);
> 
> del_timer_sync().

added, thanks.

>> +
>> +	clk_disable(ts.clock);
>> +	clk_put(ts.clock);
>> +
>> +	input_unregister_device(ts.input);
>> +	iounmap(ts.io);
> 
> release_mem_region()

see above.

>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int s3c2410ts_suspend(struct platform_device *pdev, pm_message_t state)
>> +{
>> +	writel(TSC_SLEEP, ts.io + S3C2410_ADCTSC);
>> +	disable_irq(ts.irq_tc);
>> +	clk_disable(ts.clock);
>> +
>> +	return 0;
>> +}
>> +
>> +static int s3c2410ts_resume(struct platform_device *pdev)
>> +{
>> +	struct s3c2410_ts_mach_info *info = pdev->dev.platform_data;
>> +
>> +	clk_enable(ts.clock);
>> +
>> +	/* Initialise registers */
>> +	if ((info->delay & 0xffff) > 0)
>> +		writel(info->delay & 0xffff, ts.io + S3C2410_ADCDLY);
>> +
>> +	writel(WAIT4INT | INT_DOWN, ts.io + S3C2410_ADCTSC);
>> +
>> +	return 0;
>> +}
>> +
>> +#else
>> +#define s3c2410ts_suspend NULL
>> +#define s3c2410ts_resume  NULL
>> +#endif
>> +
>> +static struct platform_device_id s3cts_driver_ids[] = {
>> +	{ "s3c2410-ts", 0 },
>> +	{ "s3c2440-ts", 1 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(platform, s3cts_driver_ids);
>> +
>> +static struct platform_driver s3c_ts_driver = {
>> +	.driver         = {
>> +		.name   = "s3c24xx-ts",
>> +		.owner  = THIS_MODULE,
>> +	},
>> +	.id_table	= s3cts_driver_ids,
>> +	.probe		= s3c2410ts_probe,
>> +	.remove		= s3c2410ts_remove,
> 
> __devexit_p()
> 
>> +	.suspend	= s3c2410ts_suspend,
>> +	.resume		= s3c2410ts_resume,
> 
> Switch to pm_ops.

ok, will do. may as well remove the #ifdef CONFIG_PM
for such small amount of code too.

>> +};
>> +
>> +static int __init s3c2410ts_init(void)
>> +{
>> +	return platform_driver_register(&s3c_ts_driver);
>> +}
>> +
>> +static void __exit s3c2410ts_exit(void)
>> +{
>> +	platform_driver_unregister(&s3c_ts_driver);
>> +}
>> +
>> +module_init(s3c2410ts_init);
>> +module_exit(s3c2410ts_exit);
>> +
>> +MODULE_AUTHOR("Arnaud Patard <arnaud.patard@rtp-net.org>, "
>> +	      "Ben Dooks <ben@simtec.co.uk>, "
>> +	      "Simtec Electronics <linux@simtec.co.uk>");
>> +MODULE_DESCRIPTION("S3C24XX Touchscreen driver");
>> +MODULE_LICENSE("GPL v2");
>>
> 
> Thanks!

I'll try and work on the to-do items and do another round
of testing before the weekend.

Re: [patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Ben Dooks]

Mark Brown wrote:
> On Thu, Nov 19, 2009 at 01:46:55PM +0800, Shine Liu wrote:
>> On Wed, 2009-11-18 at 23:29 +0000, Ben Dooks wrote:
> 
>>> +config TOUCHSCREEN_S3C2410
>>> +       tristate "Samsung S3C2410 touchscreen input driver"
>>> +       depends on ARCH_S3C2410 && INPUT && INPUT_TOUCHSCREEN
>>> +       select SERIO
>>> +       help
>>> +         Say Y here if you have the s3c2410 touchscreen.
>>> +
>>> +         If unsure, say N.
>>> +
>>> +         To compile this driver as a module, choose M here: the
>>> +         module will be called s3c2410_ts.
> 
>> S3C24XX_ADC should be added to the depends.
> 
> It's probably more friendly to select rather than depend on it to avoid
> the option being hidden.

I prefer the 'select S3C24XX_ADC' and have the symbol available.

Will update the patch.

[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Ben Dooks]

Mark Brown wrote:
> On Thu, Nov 19, 2009 at 01:46:55PM +0800, Shine Liu wrote:
>> On Wed, 2009-11-18 at 23:29 +0000, Ben Dooks wrote:
> 
>>> +config TOUCHSCREEN_S3C2410
>>> +       tristate "Samsung S3C2410 touchscreen input driver"
>>> +       depends on ARCH_S3C2410 && INPUT && INPUT_TOUCHSCREEN
>>> +       select SERIO
>>> +       help
>>> +         Say Y here if you have the s3c2410 touchscreen.
>>> +
>>> +         If unsure, say N.
>>> +
>>> +         To compile this driver as a module, choose M here: the
>>> +         module will be called s3c2410_ts.
> 
>> S3C24XX_ADC should be added to the depends.
> 
> It's probably more friendly to select rather than depend on it to avoid
> the option being hidden.

I prefer the 'select S3C24XX_ADC' and have the symbol available.

Will update the patch.

Re: [patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Daniel Silverstone]

On Thu, Nov 19, 2009 at 11:34:40AM +0000, Ben Dooks wrote:
>>> +		input_report_key(ts.input, BTN_TOUCH, 1);
>>> +		input_report_abs(ts.input, ABS_PRESSURE, 1);
>> No fake pressure events please, BTN_TOUCH should be enough.
> I'd have to check, IIRC tslib needs these to function properly.

Indeed it does -- otherwise it won't work.  Yes you could try going around and
patching TSLIB but so many people use it that it is principle-of-least-surprise
to produce pressure events.

>>> +	ts.input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
>>> +	input_set_abs_params(ts.input, ABS_X, 0, 0x3FF, 0, 0);
>>> +	input_set_abs_params(ts.input, ABS_Y, 0, 0x3FF, 0, 0);
>>> +	input_set_abs_params(ts.input, ABS_PRESSURE, 0, 1, 0, 0);
>> Drop ABS_PRESSURE.
> ok, see above.

The same applies here -- claim ABS_PRESSURE or tslib won't operate with the
touchscreen.

While it is tempting to be 100% exactly correct to what the hardware reports
(which is only TOUCH not PRESSURE) it is preferable to work with the software
which the majority of people use -- namely tslib.

I would strongly argue against removing the ABS_PRESSURE stuff personally,
despite it being essentially a lie.

Regards,

Daniel.

-- 
Daniel Silverstone                              http://www.simtec.co.uk/

[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Daniel Silverstone]

On Thu, Nov 19, 2009 at 11:34:40AM +0000, Ben Dooks wrote:
>>> +		input_report_key(ts.input, BTN_TOUCH, 1);
>>> +		input_report_abs(ts.input, ABS_PRESSURE, 1);
>> No fake pressure events please, BTN_TOUCH should be enough.
> I'd have to check, IIRC tslib needs these to function properly.

Indeed it does -- otherwise it won't work.  Yes you could try going around and
patching TSLIB but so many people use it that it is principle-of-least-surprise
to produce pressure events.

>>> +	ts.input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
>>> +	input_set_abs_params(ts.input, ABS_X, 0, 0x3FF, 0, 0);
>>> +	input_set_abs_params(ts.input, ABS_Y, 0, 0x3FF, 0, 0);
>>> +	input_set_abs_params(ts.input, ABS_PRESSURE, 0, 1, 0, 0);
>> Drop ABS_PRESSURE.
> ok, see above.

The same applies here -- claim ABS_PRESSURE or tslib won't operate with the
touchscreen.

While it is tempting to be 100% exactly correct to what the hardware reports
(which is only TOUCH not PRESSURE) it is preferable to work with the software
which the majority of people use -- namely tslib.

I would strongly argue against removing the ABS_PRESSURE stuff personally,
despite it being essentially a lie.

Regards,

Daniel.

-- 
Daniel Silverstone                              http://www.simtec.co.uk/

Re: [patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Dmitry Torokhov]

On Thu, Nov 19, 2009 at 01:52:36PM +0000, Daniel Silverstone wrote:
> On Thu, Nov 19, 2009 at 11:34:40AM +0000, Ben Dooks wrote:
> >>> +		input_report_key(ts.input, BTN_TOUCH, 1);
> >>> +		input_report_abs(ts.input, ABS_PRESSURE, 1);
> >> No fake pressure events please, BTN_TOUCH should be enough.
> > I'd have to check, IIRC tslib needs these to function properly.
> 
> Indeed it does -- otherwise it won't work.  Yes you could try going around and
> patching TSLIB but so many people use it that it is principle-of-least-surprise
> to produce pressure events.
> 
> >>> +	ts.input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> >>> +	input_set_abs_params(ts.input, ABS_X, 0, 0x3FF, 0, 0);
> >>> +	input_set_abs_params(ts.input, ABS_Y, 0, 0x3FF, 0, 0);
> >>> +	input_set_abs_params(ts.input, ABS_PRESSURE, 0, 1, 0, 0);
> >> Drop ABS_PRESSURE.
> > ok, see above.
> 
> The same applies here -- claim ABS_PRESSURE or tslib won't operate with the
> touchscreen.
> 
> While it is tempting to be 100% exactly correct to what the hardware reports
> (which is only TOUCH not PRESSURE) it is preferable to work with the software
> which the majority of people use -- namely tslib.
> 
> I would strongly argue against removing the ABS_PRESSURE stuff personally,
> despite it being essentially a lie.
> 

And I would strongly argue that you just need to update your tslib,
especially given the fact that this issue was dealt with there about a
year ago.  And if you really need that fix to be in released (read
'tagged') version of tslib - please speak to its maintainer.

Why everyone thinks that it is a good idea to pile workarounds for
software issues in the kernel but updating the other parts of software
stack is a big no-no?

-- 
Dmitry

[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Dmitry Torokhov]

On Thu, Nov 19, 2009 at 01:52:36PM +0000, Daniel Silverstone wrote:
> On Thu, Nov 19, 2009 at 11:34:40AM +0000, Ben Dooks wrote:
> >>> +		input_report_key(ts.input, BTN_TOUCH, 1);
> >>> +		input_report_abs(ts.input, ABS_PRESSURE, 1);
> >> No fake pressure events please, BTN_TOUCH should be enough.
> > I'd have to check, IIRC tslib needs these to function properly.
> 
> Indeed it does -- otherwise it won't work.  Yes you could try going around and
> patching TSLIB but so many people use it that it is principle-of-least-surprise
> to produce pressure events.
> 
> >>> +	ts.input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> >>> +	input_set_abs_params(ts.input, ABS_X, 0, 0x3FF, 0, 0);
> >>> +	input_set_abs_params(ts.input, ABS_Y, 0, 0x3FF, 0, 0);
> >>> +	input_set_abs_params(ts.input, ABS_PRESSURE, 0, 1, 0, 0);
> >> Drop ABS_PRESSURE.
> > ok, see above.
> 
> The same applies here -- claim ABS_PRESSURE or tslib won't operate with the
> touchscreen.
> 
> While it is tempting to be 100% exactly correct to what the hardware reports
> (which is only TOUCH not PRESSURE) it is preferable to work with the software
> which the majority of people use -- namely tslib.
> 
> I would strongly argue against removing the ABS_PRESSURE stuff personally,
> despite it being essentially a lie.
> 

And I would strongly argue that you just need to update your tslib,
especially given the fact that this issue was dealt with there about a
year ago.  And if you really need that fix to be in released (read
'tagged') version of tslib - please speak to its maintainer.

Why everyone thinks that it is a good idea to pile workarounds for
software issues in the kernel but updating the other parts of software
stack is a big no-no?

-- 
Dmitry

Re: [patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Russell King - ARM Linux]

On Thu, Nov 19, 2009 at 09:03:21AM -0800, Dmitry Torokhov wrote:
> Why everyone thinks that it is a good idea to pile workarounds for
> software issues in the kernel but updating the other parts of software
> stack is a big no-no?

It's probably because pressing 'reset' and have your board TFTP the
new kernel straight from your development machine is far easier than
going through the hoops to regenerate a root filesystem and reflashing
it.

Note that I'm not condoning it, just pointing out why it happens.  And
yes, we must resist the temptation to merge such workarounds.

[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Russell King - ARM Linux]

On Thu, Nov 19, 2009 at 09:03:21AM -0800, Dmitry Torokhov wrote:
> Why everyone thinks that it is a good idea to pile workarounds for
> software issues in the kernel but updating the other parts of software
> stack is a big no-no?

It's probably because pressing 'reset' and have your board TFTP the
new kernel straight from your development machine is far easier than
going through the hoops to regenerate a root filesystem and reflashing
it.

Note that I'm not condoning it, just pointing out why it happens.  And
yes, we must resist the temptation to merge such workarounds.

Re: [patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Dmitry Torokhov]

On Thu, Nov 19, 2009 at 11:34:40AM +0000, Ben Dooks wrote:
> Dmitry Torokhov wrote:
>
>>> +
>>> +static char *s3c2410ts_name = "s3c2410 TouchScreen";

Btw, "static char s3c2410ts_name[]" will save you a pointer. But, it
looks like it is used only once, in probe(), so just put it there.

>>> +
>>> +/* Per-touchscreen data. */
>>> +
>>> +/**
>>> + * struct s3c2410ts - driver touchscreen state.
>>> + * @client: The ADC client we registered with the core driver.
>>> + * @dev: The device we are bound to.
>>> + * @input: The input device we registered with the input subsystem.
>>> + * @clock: The clock for the adc.
>>> + * @io: Pointer to the IO base.
>>> + * @xp: The accumulated X position data.
>>> + * @yp: The accumulated Y position data.
>>> + * @irq_tc: The interrupt number for pen up/down interrupt
>>> + * @count: The number of samples collected.
>>> + * @shift: The log2 of the maximum count to read in one go.
>>> + */
>>
>> These sructures are driver-internal and so don't need to be kernel-doc-ed.
>> Same goes for the driver-private functions.
>
> I like having the documentation, and I would much prefer to leave it
> in as useful.
>

Ah, I wasn't requiesting to remove the documentation, I was just saying
that since these data structures and fucntions are driver-provate they
don't need to use kernel-doc style.

>>> +
>>> +		input_report_key(ts.input, BTN_TOUCH, 1);
>>> +		input_report_abs(ts.input, ABS_PRESSURE, 1);
>>
>> No fake pressure events please, BTN_TOUCH should be enough.
>
> I'd have to check, IIRC tslib needs these to function properly.
>

Just update your tslib, the issue was fixed there last November.

>>> +{
>>> +	struct s3c2410_ts_mach_info *info;
>>> +	struct device *dev = &pdev->dev;
>>> +	struct input_dev *input_dev;
>>> +	struct resource *res;
>>> +	int ret = -EINVAL;
>>
>> Can we call it "error" (since that's what you use it for).
>
> Is it really necessary to change this?
>

No, it is my personal preference/style. In case when it is used like:

	var = blah();
	if (var)
		goto err_unblah;

	return 0;

err_unblah:
	unblah();
	return var;
}

I prefer that var called 'error'. If the value is returned on both error
and success paths then I call it 'ret', 'retval', etc.

But no, if you prefer 'ret' that is fine too.

>>
>>> +	.suspend	= s3c2410ts_suspend,
>>> +	.resume		= s3c2410ts_resume,
>>
>> Switch to pm_ops.
>
> ok, will do. may as well remove the #ifdef CONFIG_PM
> for such small amount of code too.
>

As long as it does not break when CONFIG_PM is not set...

-- 
Dmitry

[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Dmitry Torokhov]

On Thu, Nov 19, 2009 at 11:34:40AM +0000, Ben Dooks wrote:
> Dmitry Torokhov wrote:
>
>>> +
>>> +static char *s3c2410ts_name = "s3c2410 TouchScreen";

Btw, "static char s3c2410ts_name[]" will save you a pointer. But, it
looks like it is used only once, in probe(), so just put it there.

>>> +
>>> +/* Per-touchscreen data. */
>>> +
>>> +/**
>>> + * struct s3c2410ts - driver touchscreen state.
>>> + * @client: The ADC client we registered with the core driver.
>>> + * @dev: The device we are bound to.
>>> + * @input: The input device we registered with the input subsystem.
>>> + * @clock: The clock for the adc.
>>> + * @io: Pointer to the IO base.
>>> + * @xp: The accumulated X position data.
>>> + * @yp: The accumulated Y position data.
>>> + * @irq_tc: The interrupt number for pen up/down interrupt
>>> + * @count: The number of samples collected.
>>> + * @shift: The log2 of the maximum count to read in one go.
>>> + */
>>
>> These sructures are driver-internal and so don't need to be kernel-doc-ed.
>> Same goes for the driver-private functions.
>
> I like having the documentation, and I would much prefer to leave it
> in as useful.
>

Ah, I wasn't requiesting to remove the documentation, I was just saying
that since these data structures and fucntions are driver-provate they
don't need to use kernel-doc style.

>>> +
>>> +		input_report_key(ts.input, BTN_TOUCH, 1);
>>> +		input_report_abs(ts.input, ABS_PRESSURE, 1);
>>
>> No fake pressure events please, BTN_TOUCH should be enough.
>
> I'd have to check, IIRC tslib needs these to function properly.
>

Just update your tslib, the issue was fixed there last November.

>>> +{
>>> +	struct s3c2410_ts_mach_info *info;
>>> +	struct device *dev = &pdev->dev;
>>> +	struct input_dev *input_dev;
>>> +	struct resource *res;
>>> +	int ret = -EINVAL;
>>
>> Can we call it "error" (since that's what you use it for).
>
> Is it really necessary to change this?
>

No, it is my personal preference/style. In case when it is used like:

	var = blah();
	if (var)
		goto err_unblah;

	return 0;

err_unblah:
	unblah();
	return var;
}

I prefer that var called 'error'. If the value is returned on both error
and success paths then I call it 'ret', 'retval', etc.

But no, if you prefer 'ret' that is fine too.

>>
>>> +	.suspend	= s3c2410ts_suspend,
>>> +	.resume		= s3c2410ts_resume,
>>
>> Switch to pm_ops.
>
> ok, will do. may as well remove the #ifdef CONFIG_PM
> for such small amount of code too.
>

As long as it does not break when CONFIG_PM is not set...

-- 
Dmitry

Re: [patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Ben Dooks]

Dmitry Torokhov wrote:
> On Thu, Nov 19, 2009 at 01:52:36PM +0000, Daniel Silverstone wrote:
>> On Thu, Nov 19, 2009 at 11:34:40AM +0000, Ben Dooks wrote:
>>>>> +		input_report_key(ts.input, BTN_TOUCH, 1);
>>>>> +		input_report_abs(ts.input, ABS_PRESSURE, 1);
>>>> No fake pressure events please, BTN_TOUCH should be enough.
>>> I'd have to check, IIRC tslib needs these to function properly.
>> Indeed it does -- otherwise it won't work.  Yes you could try going around and
>> patching TSLIB but so many people use it that it is principle-of-least-surprise
>> to produce pressure events.
>>
>>>>> +	ts.input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
>>>>> +	input_set_abs_params(ts.input, ABS_X, 0, 0x3FF, 0, 0);
>>>>> +	input_set_abs_params(ts.input, ABS_Y, 0, 0x3FF, 0, 0);
>>>>> +	input_set_abs_params(ts.input, ABS_PRESSURE, 0, 1, 0, 0);
>>>> Drop ABS_PRESSURE.
>>> ok, see above.
>> The same applies here -- claim ABS_PRESSURE or tslib won't operate with the
>> touchscreen.
>>
>> While it is tempting to be 100% exactly correct to what the hardware reports
>> (which is only TOUCH not PRESSURE) it is preferable to work with the software
>> which the majority of people use -- namely tslib.
>>
>> I would strongly argue against removing the ABS_PRESSURE stuff personally,
>> despite it being essentially a lie.

ok, i'll remove it and anyone who can't be bothered to update their
tslib can hack in their own solution if they really care about it.

> And I would strongly argue that you just need to update your tslib,
> especially given the fact that this issue was dealt with there about a
> year ago.  And if you really need that fix to be in released (read
> 'tagged') version of tslib - please speak to its maintainer.

> Why everyone thinks that it is a good idea to pile workarounds for
> software issues in the kernel but updating the other parts of software
> stack is a big no-no?

it can be much more difficult to update a userland than to hack the
kernel...

[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Ben Dooks]

Dmitry Torokhov wrote:
> On Thu, Nov 19, 2009 at 01:52:36PM +0000, Daniel Silverstone wrote:
>> On Thu, Nov 19, 2009 at 11:34:40AM +0000, Ben Dooks wrote:
>>>>> +		input_report_key(ts.input, BTN_TOUCH, 1);
>>>>> +		input_report_abs(ts.input, ABS_PRESSURE, 1);
>>>> No fake pressure events please, BTN_TOUCH should be enough.
>>> I'd have to check, IIRC tslib needs these to function properly.
>> Indeed it does -- otherwise it won't work.  Yes you could try going around and
>> patching TSLIB but so many people use it that it is principle-of-least-surprise
>> to produce pressure events.
>>
>>>>> +	ts.input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
>>>>> +	input_set_abs_params(ts.input, ABS_X, 0, 0x3FF, 0, 0);
>>>>> +	input_set_abs_params(ts.input, ABS_Y, 0, 0x3FF, 0, 0);
>>>>> +	input_set_abs_params(ts.input, ABS_PRESSURE, 0, 1, 0, 0);
>>>> Drop ABS_PRESSURE.
>>> ok, see above.
>> The same applies here -- claim ABS_PRESSURE or tslib won't operate with the
>> touchscreen.
>>
>> While it is tempting to be 100% exactly correct to what the hardware reports
>> (which is only TOUCH not PRESSURE) it is preferable to work with the software
>> which the majority of people use -- namely tslib.
>>
>> I would strongly argue against removing the ABS_PRESSURE stuff personally,
>> despite it being essentially a lie.

ok, i'll remove it and anyone who can't be bothered to update their
tslib can hack in their own solution if they really care about it.

> And I would strongly argue that you just need to update your tslib,
> especially given the fact that this issue was dealt with there about a
> year ago.  And if you really need that fix to be in released (read
> 'tagged') version of tslib - please speak to its maintainer.

> Why everyone thinks that it is a good idea to pile workarounds for
> software issues in the kernel but updating the other parts of software
> stack is a big no-no?

it can be much more difficult to update a userland than to hack the
kernel...

Re: [patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Pavel Machek]

Hi!

> >>> +/* Per-touchscreen data. */
> >>> +
> >>> +/**
> >>> + * struct s3c2410ts - driver touchscreen state.
> >>> + * @client: The ADC client we registered with the core driver.
> >>> + * @dev: The device we are bound to.
> >>> + * @input: The input device we registered with the input subsystem.
> >>> + * @clock: The clock for the adc.
> >>> + * @io: Pointer to the IO base.
> >>> + * @xp: The accumulated X position data.
> >>> + * @yp: The accumulated Y position data.
> >>> + * @irq_tc: The interrupt number for pen up/down interrupt
> >>> + * @count: The number of samples collected.
> >>> + * @shift: The log2 of the maximum count to read in one go.
> >>> + */
> >>
> >> These sructures are driver-internal and so don't need to be kernel-doc-ed.
> >> Same goes for the driver-private functions.
> >
> > I like having the documentation, and I would much prefer to leave it
> > in as useful.
> 
> Ah, I wasn't requiesting to remove the documentation, I was just saying
> that since these data structures and fucntions are driver-provate they
> don't need to use kernel-doc style.

Well.. consistent style of documentation is nice, be it driver-private
or public stuff.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[patch 1/1] TOUCHSCREEN: S3C24XX touchscreen driver from Arnaud Patard.

[Pavel Machek]

Hi!

> >>> +/* Per-touchscreen data. */
> >>> +
> >>> +/**
> >>> + * struct s3c2410ts - driver touchscreen state.
> >>> + * @client: The ADC client we registered with the core driver.
> >>> + * @dev: The device we are bound to.
> >>> + * @input: The input device we registered with the input subsystem.
> >>> + * @clock: The clock for the adc.
> >>> + * @io: Pointer to the IO base.
> >>> + * @xp: The accumulated X position data.
> >>> + * @yp: The accumulated Y position data.
> >>> + * @irq_tc: The interrupt number for pen up/down interrupt
> >>> + * @count: The number of samples collected.
> >>> + * @shift: The log2 of the maximum count to read in one go.
> >>> + */
> >>
> >> These sructures are driver-internal and so don't need to be kernel-doc-ed.
> >> Same goes for the driver-private functions.
> >
> > I like having the documentation, and I would much prefer to leave it
> > in as useful.
> 
> Ah, I wasn't requiesting to remove the documentation, I was just saying
> that since these data structures and fucntions are driver-provate they
> don't need to use kernel-doc style.

Well.. consistent style of documentation is nice, be it driver-private
or public stuff.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html