(unknown)

(unknown)

[Alan Cox]

Subject: [FOR COMMENT] cy8ctmg110 for review

From: Samuli Konttila <samuli.konttila@aavamobile.com>

Add support for the cy8ctmg110 capacitive touchscreen used on some embedded
devices.

(Some clean up by Alan Cox)

(No signed off, not yet ready to go in)
---

 drivers/input/touchscreen/Kconfig         |   12 +
 drivers/input/touchscreen/Makefile        |    3 
 drivers/input/touchscreen/cy8ctmg110_ts.c |  521 +++++++++++++++++++++++++++++
 3 files changed, 535 insertions(+), 1 deletions(-)
 create mode 100644 drivers/input/touchscreen/cy8ctmg110_ts.c


diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index b3ba374..89a3eb1 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -591,4 +591,16 @@ config TOUCHSCREEN_TPS6507X
 	  To compile this driver as a module, choose M here: the
 	  module will be called tps6507x_ts.
 
+config TOUCHSCREEN_CY8CTMG110
+	tristate "cy8ctmg110 touchscreen"
+	depends on I2C
+	help
+	  Say Y here if you have a cy8ctmg110 touchscreen capacitive
+	  touchscreen
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called cy8ctmg110_ts.
+
 endif
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index dfb7239..c7acb65 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -1,5 +1,5 @@
 #
-# Makefile for the touchscreen drivers.
+# Makefile for the touchscreen drivers.mororor
 #
 
 # Each configuration option enables a list of files.
@@ -12,6 +12,7 @@ obj-$(CONFIG_TOUCHSCREEN_AD7879)	+= ad7879.o
 obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
 obj-$(CONFIG_TOUCHSCREEN_ATMEL_TSADCC)	+= atmel_tsadcc.o
 obj-$(CONFIG_TOUCHSCREEN_BITSY)		+= h3600_ts_input.o
+obj-$(CONFIG_TOUCHSCREEN_CY8CTMG110)    += cy8ctmg110_ts.o
 obj-$(CONFIG_TOUCHSCREEN_DYNAPRO)	+= dynapro.o
 obj-$(CONFIG_TOUCHSCREEN_GUNZE)		+= gunze.o
 obj-$(CONFIG_TOUCHSCREEN_EETI)		+= eeti_ts.o
diff --git a/drivers/input/touchscreen/cy8ctmg110_ts.c b/drivers/input/touchscreen/cy8ctmg110_ts.c
new file mode 100644
index 0000000..4adbe87
--- /dev/null
+++ b/drivers/input/touchscreen/cy8ctmg110_ts.c
@@ -0,0 +1,521 @@
+/*
+ * cy8ctmg110_ts.c Driver for cypress touch screen controller
+ * Copyright (c) 2009 Aava Mobile
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/input.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <asm/io.h>
+#include <linux/i2c.h>
+#include <linux/timer.h>
+#include <linux/gpio.h>
+#include <linux/hrtimer.h>
+
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/fs.h>
+#include <asm/ioctl.h>
+#include <asm/uaccess.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/fs.h>
+#include <asm/ioctl.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+
+
+#define CY8CTMG110_DRIVER_NAME      "cy8ctmg110"
+
+
+/*HW definations*/
+#define CY8CTMG110_RESET_PIN_GPIO   43
+#define CY8CTMG110_IRQ_PIN_GPIO     59
+#define CY8CTMG110_I2C_ADDR         0x38
+#define CY8CTMG110_I2C_ADDR_EXT     0x39
+#define CY8CTMG110_I2C_ADDR_        0x2	/*i2c address first sample */
+#define CY8CTMG110_I2C_ADDR__       53	/*i2c address to FW where irq support missing */
+#define CY8CTMG110_TOUCH_IRQ        21
+#define CY8CTMG110_TOUCH_LENGHT     9787
+#define CY8CTMG110_SCREEN_LENGHT    8424
+
+
+/*Touch coordinates*/
+#define CY8CTMG110_X_MIN        0
+#define CY8CTMG110_Y_MIN        0
+#define CY8CTMG110_X_MAX        864
+#define CY8CTMG110_Y_MAX        480
+
+
+/*cy8ctmg110 registers defination*/
+#define CY8CTMG110_TOUCH_WAKEUP_TIME   0
+#define CY8CTMG110_TOUCH_SLEEP_TIME    2
+#define CY8CTMG110_TOUCH_X1            3
+#define CY8CTMG110_TOUCH_Y1            5
+#define CY8CTMG110_TOUCH_X2            7
+#define CY8CTMG110_TOUCH_Y2            9
+#define CY8CTMG110_FINGERS             11
+#define CY8CTMG110_GESTURE             12
+#define CY8CTMG110_REG_MAX             13
+
+#define CY8CTMG110_POLL_TIMER_DELAY  1000*1000*100
+#define TOUCH_MAX_I2C_FAILS          50
+
+/* Scale factors for coordinates */
+#define X_SCALE_FACTOR 9387/8424
+#define Y_SCALE_FACTOR 97/100
+
+/* For tracing */
+static int g_y_trace_coord = 0;
+module_param(g_y_trace_coord, int, 0600);
+
+/* Polling mode */
+static int polling = 0;
+module_param(polling, int, 0);
+MODULE_PARM_DESC(polling, "Set to enabling polling of the touchscreen");
+
+
+/*
+ * The touch position structure.
+ */
+struct ts_event {
+	int x1;
+	int y1;
+	int x2;
+	int y2;
+	bool event_sended;
+};
+
+/*
+ * The touch driver structure.
+ */
+struct cy8ctmg110 {
+	struct input_dev *input;
+	char phys[32];
+	struct ts_event tc;
+	struct i2c_client *client;
+	bool pending;
+	spinlock_t lock;
+	bool initController;
+	bool sleepmode;
+	int i2c_fail_count;
+	struct hrtimer timer;
+};
+
+/*
+ * cy8ctmg110_poweroff is the routine that is called when touch hardware 
+ * will powered off
+ */
+static void cy8ctmg110_power(bool poweron)
+{
+	if (poweron)
+		gpio_direction_output(CY8CTMG110_RESET_PIN_GPIO, 0);
+	else
+		gpio_direction_output(CY8CTMG110_RESET_PIN_GPIO, 1);
+}
+
+/*
+ * cy8ctmg110_write_req write regs to the i2c devices
+ * 
+ */
+static int cy8ctmg110_write_req(struct cy8ctmg110 *tsc, unsigned char reg,
+		unsigned char len, unsigned char *value)
+{
+	struct i2c_client *client = tsc->client;
+	unsigned int ret;
+	unsigned char i2c_data[] = { 0, 0, 0, 0, 0, 0 };
+	struct i2c_msg msg[] = {
+			{client->addr, 0, len + 1, i2c_data},
+			};
+
+	i2c_data[0] = reg;
+	memcpy(i2c_data + 1, value, len);
+
+	ret = i2c_transfer(client->adapter, msg, 1);
+	if (ret != 1) {
+		printk("cy8ctmg110 touch : i2c write data cmd failed \n");
+		return ret;
+	}
+	return 0;
+}
+
+/*
+ * cy8ctmg110_read_req read regs from i2c devise
+ * 
+ */
+
+static int cy8ctmg110_read_req(struct cy8ctmg110 *tsc,
+		unsigned char *i2c_data, unsigned char len, unsigned char cmd)
+{
+	struct i2c_client *client = tsc->client;
+	unsigned int ret;
+	unsigned char regs_cmd[2] = { 0, 0 };
+	struct i2c_msg msg1[] = {
+		{client->addr, 0, 1, regs_cmd},
+	};
+	struct i2c_msg msg2[] = {
+		{client->addr, I2C_M_RD, len, i2c_data},
+	};
+
+	regs_cmd[0] = cmd;
+
+	/* first write slave position to i2c devices */
+	ret = i2c_transfer(client->adapter, msg1, 1);
+	if (ret != 1) {
+		tsc->i2c_fail_count++;
+		return ret;
+	}
+
+	/* Second read data from position */
+	ret = i2c_transfer(client->adapter, msg2, 1);
+	if (ret != 1) {
+		tsc->i2c_fail_count++;
+		return ret;
+	}
+	return 0;
+}
+
+/*
+ * cy8ctmg110_send_event delevery touch event to the userpace
+ * function use normal input interface
+ */
+static void cy8ctmg110_send_event(void *tsc)
+{
+	struct cy8ctmg110 *ts = tsc;
+	struct input_dev *input = ts->input;
+	u16 x, y;
+	u16 x2, y2;
+
+	x = ts->tc.x1;
+	y = ts->tc.y1;
+
+	if (ts->tc.event_sended == false) {
+		input_report_key(input, BTN_TOUCH, 1);
+		ts->pending = true;
+		x2 = (u16) (y * X_SCALE_FACTOR);
+		y2 = (u16) (x * Y_SCALE_FACTOR);
+		input_report_abs(input, ABS_X, x2);
+		input_report_abs(input, ABS_Y, y2);
+		input_sync(input);
+		if (g_y_trace_coord)
+			printk("cy8ctmg110 touch position X:%d (was = %d) Y:%d (was = %d)\n", x2, y, y2, x);
+	}
+
+}
+
+/*
+ * cy8ctmg110_touch_pos check touch position from i2c devices
+ * 
+ */
+static int cy8ctmg110_touch_pos(struct cy8ctmg110 *tsc)
+{
+	unsigned char reg_p[CY8CTMG110_REG_MAX];
+	int x, y;
+
+	memset(reg_p, 0, CY8CTMG110_REG_MAX);
+
+	/*Reading coordinates */
+	if (cy8ctmg110_read_req(tsc, reg_p, 9, CY8CTMG110_TOUCH_X1) != 0)
+		return -EIO;
+		
+	y = reg_p[2] << 8 | reg_p[3];
+	x = reg_p[0] << 8 | reg_p[1];
+		/*number of touch */
+	if (reg_p[8] == 0) {
+		if (tsc->pending == true) {
+			struct input_dev *input = tsc->input;
+
+			input_report_key(input, BTN_TOUCH, 0);
+			tsc->tc.event_sended = true;
+			tsc->pending = false;
+		}
+	} else if (tsc->tc.x1 != x || tsc->tc.y1 != y) {
+		tsc->tc.y1 = y;
+		tsc->tc.x1 = x;
+		tsc->tc.event_sended = false;
+		cy8ctmg110_send_event(tsc);
+	}
+	return 0;
+}
+
+/*
+ * if interrupt isn't in use the touch positions can reads by polling
+ * 
+ */
+static enum hrtimer_restart cy8ctmg110_timer(struct hrtimer *handle)
+{
+	struct cy8ctmg110 *ts = container_of(handle, struct cy8ctmg110, timer);
+	unsigned long flags;
+
+	spin_lock_irqsave(&ts->lock, flags);
+
+	cy8ctmg110_touch_pos(ts);
+	if (ts->i2c_fail_count < TOUCH_MAX_I2C_FAILS)
+		hrtimer_start(&ts->timer, ktime_set(0, CY8CTMG110_POLL_TIMER_DELAY), HRTIMER_MODE_REL);
+
+	spin_unlock_irqrestore(&ts->lock, flags);
+	return HRTIMER_NORESTART;
+}
+
+/*
+ * cy8ctmg110_init_controller set init value to touchcontroller
+ * 
+ */
+static bool cy8ctmg110_set_sleepmode(struct cy8ctmg110 *ts)
+{
+	unsigned char reg_p[3];
+
+	if (ts->sleepmode == true) {
+		reg_p[0] = 0x00;
+		reg_p[1] = 0xff;
+		reg_p[2] = 5;
+	} else {
+		reg_p[0] = 0x10;
+		reg_p[1] = 0xff;
+		reg_p[2] = 0;
+	}
+
+	if (cy8ctmg110_write_req(ts, CY8CTMG110_TOUCH_WAKEUP_TIME, 3, reg_p))
+		return false;
+
+	ts->initController = true;
+	return true;
+}
+
+/*
+ * cy8ctmg110_irq_handler irq handling function
+ * 
+ */
+
+static irqreturn_t cy8ctmg110_irq_handler(int irq, void *dev_id)
+{
+	struct cy8ctmg110 *tsc = (struct cy8ctmg110 *) dev_id;
+
+	if (tsc->initController == false) {
+		if (cy8ctmg110_set_sleepmode(tsc) == true)
+			tsc->initController = true;
+	} else
+		cy8ctmg110_touch_pos(tsc);
+
+	/* if interrupt supported in the touch controller
+	   timer polling need to stop */
+	tsc->i2c_fail_count = TOUCH_MAX_I2C_FAILS;
+	return IRQ_HANDLED;
+}
+
+
+static int cy8ctmg110_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+	struct cy8ctmg110 *ts;
+	struct input_dev *input_dev;
+	int err;
+	client->irq = CY8CTMG110_TOUCH_IRQ;
+
+	if (!i2c_check_functionality(client->adapter,
+					I2C_FUNC_SMBUS_READ_WORD_DATA))
+		return -EIO;
+
+	ts = kzalloc(sizeof(struct cy8ctmg110), GFP_KERNEL);
+	input_dev = input_allocate_device();
+
+	if (!ts || !input_dev) {
+		err = -ENOMEM;
+		goto err_free_mem;
+	}
+
+	ts->client = client;
+	i2c_set_clientdata(client, ts);
+
+	ts->input = input_dev;
+	ts->pending = false;
+	ts->sleepmode = false;
+
+	snprintf(ts->phys, sizeof(ts->phys), "%s/input0",
+						dev_name(&client->dev));
+
+	input_dev->name = CY8CTMG110_DRIVER_NAME " Touchscreen";
+	input_dev->phys = ts->phys;
+	input_dev->id.bustype = BUS_I2C;
+
+	spin_lock_init(&ts->lock);
+
+	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP) |
+					BIT_MASK(EV_REL) | BIT_MASK(EV_ABS);
+	input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+
+	input_set_capability(input_dev, EV_KEY, KEY_F);
+
+	input_set_abs_params(input_dev, ABS_X, CY8CTMG110_X_MIN, CY8CTMG110_X_MAX, 0, 0);
+	input_set_abs_params(input_dev, ABS_Y, CY8CTMG110_Y_MIN, CY8CTMG110_Y_MAX, 0, 0);
+
+	err = gpio_request(CY8CTMG110_RESET_PIN_GPIO, NULL);
+
+	if (err) {
+		dev_err(&client->dev, "cy8ctmg110_ts: Unable to request GPIO pin %d.\n",
+						CY8CTMG110_RESET_PIN_GPIO);
+		goto err_free_irq;
+	}
+	cy8ctmg110_power(true);
+
+	ts->initController = false;
+	ts->i2c_fail_count = 0;
+
+	hrtimer_init(&ts->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	ts->timer.function = cy8ctmg110_timer;
+
+	if (polling)
+		hrtimer_start(&ts->timer, ktime_set(10, 0), HRTIMER_MODE_REL);
+
+	/* Can we fall back to polling if these bits fail - something to look
+	   at for robustness */
+
+	err = gpio_request(CY8CTMG110_IRQ_PIN_GPIO, "touch_irq_key");
+	if (err < 0) {
+		dev_err(&client->dev,
+			"cy8ctmg110_ts: failed to request GPIO %d, error %d\n",
+						CY8CTMG110_IRQ_PIN_GPIO, err);
+		goto err_free_timer;
+	}
+
+	err = gpio_direction_input(CY8CTMG110_IRQ_PIN_GPIO);
+
+	if (err < 0) {
+		dev_err(&client->dev,
+			"cy8ctmg110_ts: failed to configure input direction for GPIO %d, error %d\n",
+						CY8CTMG110_IRQ_PIN_GPIO, err);
+		goto err_free_gpio;
+	}
+	client->irq = gpio_to_irq(CY8CTMG110_IRQ_PIN_GPIO);
+
+	if (client->irq < 0) {
+		err = client->irq;
+		dev_err(&client->dev,
+	"cy8ctmg110_ts: Unable to get irq number" " for GPIO %d, error %d\n",
+						CY8CTMG110_IRQ_PIN_GPIO, err);
+		goto err_free_gpio;
+	}
+	err = request_irq(client->irq, cy8ctmg110_irq_handler, IRQF_TRIGGER_RISING | IRQF_SHARED, "touch_reset_key", ts);
+	if (err < 0) {
+		dev_err(&client->dev,
+			"cy8ctmg110 irq %d busy? error %d\n",
+				client->irq, err);
+		goto err_free_gpio;
+	}
+
+	err = input_register_device(input_dev);
+	if (!err)
+		return 0;
+err_free_gpio:
+	gpio_free(CY8CTMG110_IRQ_PIN_GPIO);
+err_free_timer:
+	if (polling)
+		hrtimer_cancel(&ts->timer);
+err_free_irq:
+	free_irq(client->irq, ts);
+err_free_mem:
+	input_free_device(input_dev);
+	kfree(ts);
+	return err;
+}
+
+/*
+ * cy8ctmg110_suspend
+ * 
+ */
+
+static int cy8ctmg110_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+	if (device_may_wakeup(&client->dev))
+		enable_irq_wake(client->irq);
+
+	return 0;
+}
+
+/*
+ * cy8ctmg110_resume 
+ * 
+ */
+
+static int cy8ctmg110_resume(struct i2c_client *client)
+{
+	if (device_may_wakeup(&client->dev))
+		disable_irq_wake(client->irq);
+
+	return 0;
+}
+
+/*
+ * cy8ctmg110_remove
+ * 
+ */
+
+static int cy8ctmg110_remove(struct i2c_client *client)
+{
+	struct cy8ctmg110 *ts = i2c_get_clientdata(client);
+
+	cy8ctmg110_power(false);
+
+	if (polling)
+		hrtimer_cancel(&ts->timer);
+	free_irq(client->irq, ts);
+	input_unregister_device(ts->input);
+	/* FIXME: Do we need to free the GPIO ? */
+	kfree(ts);
+	return 0;
+}
+
+static struct i2c_device_id cy8ctmg110_idtable[] = {
+	{CY8CTMG110_DRIVER_NAME, 1},
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, cy8ctmg110_idtable);
+
+static struct i2c_driver cy8ctmg110_driver = {
+	.driver = {
+		   .owner = THIS_MODULE,
+		   .name = CY8CTMG110_DRIVER_NAME,
+		   .bus = &i2c_bus_type,
+		   },
+	.id_table = cy8ctmg110_idtable,
+	.probe = cy8ctmg110_probe,
+	.remove = cy8ctmg110_remove,
+	.suspend = cy8ctmg110_suspend,
+	.resume = cy8ctmg110_resume,
+};
+
+static int __init cy8ctmg110_init(void)
+{
+	return i2c_add_driver(&cy8ctmg110_driver);
+}
+
+static void __exit cy8ctmg110_exit(void)
+{
+	i2c_del_driver(&cy8ctmg110_driver);
+}
+
+module_init(cy8ctmg110_init);
+module_exit(cy8ctmg110_exit);
+
+MODULE_AUTHOR("Samuli Konttila <samuli.konttila@aavamobile.com>");
+MODULE_DESCRIPTION("cy8ctmg110 TouchScreen Driver");
+MODULE_LICENSE("GPL v2");

Re:

[Jean Delvare]

On Wed, 14 Apr 2010 13:54:02 +0100, Alan Cox wrote:
> Subject: [FOR COMMENT] cy8ctmg110 for review
> 
> From: Samuli Konttila <samuli.konttila-GpSoQilbZ/bZJqsBc5GL+g@public.gmane.org>
> 
> Add support for the cy8ctmg110 capacitive touchscreen used on some embedded
> devices.
> 
> (Some clean up by Alan Cox)
> 
> (No signed off, not yet ready to go in)
> ---
> 
>  drivers/input/touchscreen/Kconfig         |   12 +
>  drivers/input/touchscreen/Makefile        |    3 
>  drivers/input/touchscreen/cy8ctmg110_ts.c |  521 +++++++++++++++++++++++++++++
>  3 files changed, 535 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/input/touchscreen/cy8ctmg110_ts.c
> 
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index b3ba374..89a3eb1 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -591,4 +591,16 @@ config TOUCHSCREEN_TPS6507X
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called tps6507x_ts.
>  
> +config TOUCHSCREEN_CY8CTMG110
> +	tristate "cy8ctmg110 touchscreen"
> +	depends on I2C
> +	help
> +	  Say Y here if you have a cy8ctmg110 touchscreen capacitive
> +	  touchscreen
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called cy8ctmg110_ts.
> +
>  endif
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index dfb7239..c7acb65 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -1,5 +1,5 @@
>  #
> -# Makefile for the touchscreen drivers.
> +# Makefile for the touchscreen drivers.mororor

I confirm, not yet ready to go in ;)

>  #
>  
>  # Each configuration option enables a list of files.
> @@ -12,6 +12,7 @@ obj-$(CONFIG_TOUCHSCREEN_AD7879)	+= ad7879.o
>  obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
>  obj-$(CONFIG_TOUCHSCREEN_ATMEL_TSADCC)	+= atmel_tsadcc.o
>  obj-$(CONFIG_TOUCHSCREEN_BITSY)		+= h3600_ts_input.o
> +obj-$(CONFIG_TOUCHSCREEN_CY8CTMG110)    += cy8ctmg110_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_DYNAPRO)	+= dynapro.o
>  obj-$(CONFIG_TOUCHSCREEN_GUNZE)		+= gunze.o
>  obj-$(CONFIG_TOUCHSCREEN_EETI)		+= eeti_ts.o
> diff --git a/drivers/input/touchscreen/cy8ctmg110_ts.c b/drivers/input/touchscreen/cy8ctmg110_ts.c
> new file mode 100644
> index 0000000..4adbe87
> --- /dev/null
> +++ b/drivers/input/touchscreen/cy8ctmg110_ts.c
> @@ -0,0 +1,521 @@
> +/*
> + * cy8ctmg110_ts.c Driver for cypress touch screen controller
> + * Copyright (c) 2009 Aava Mobile
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/input.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <asm/io.h>
> +#include <linux/i2c.h>
> +#include <linux/timer.h>
> +#include <linux/gpio.h>
> +#include <linux/hrtimer.h>
> +
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <asm/ioctl.h>
> +#include <asm/uaccess.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <asm/ioctl.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>

What a mess. Countless duplicates includes... Seriously, I'm not even
reviewing further.

-- 
Jean Delvare

Re: cy8ctmg110 for review

[Randy Dunlap]

On Wed, 14 Apr 2010 13:54:02 +0100 Alan Cox wrote:

> Subject: [FOR COMMENT] cy8ctmg110 for review
> 
> From: Samuli Konttila <samuli.konttila-GpSoQilbZ/bZJqsBc5GL+g@public.gmane.org>
> 
> Add support for the cy8ctmg110 capacitive touchscreen used on some embedded
> devices.
> 
> (Some clean up by Alan Cox)
> 
> (No signed off, not yet ready to go in)
> ---
> 
>  drivers/input/touchscreen/Kconfig         |   12 +
>  drivers/input/touchscreen/Makefile        |    3 
>  drivers/input/touchscreen/cy8ctmg110_ts.c |  521 +++++++++++++++++++++++++++++
>  3 files changed, 535 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/input/touchscreen/cy8ctmg110_ts.c
> 
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index b3ba374..89a3eb1 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -591,4 +591,16 @@ config TOUCHSCREEN_TPS6507X
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called tps6507x_ts.
>  
> +config TOUCHSCREEN_CY8CTMG110
> +	tristate "cy8ctmg110 touchscreen"
> +	depends on I2C

	depends on GPIOLIB
	depends on INPUT

Does it build when CONFIG_PM is disabled?


> +	help
> +	  Say Y here if you have a cy8ctmg110 touchscreen capacitive
> +	  touchscreen

drop first "touchscreen" and end with '.'.

> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called cy8ctmg110_ts.
> +
>  endif

> diff --git a/drivers/input/touchscreen/cy8ctmg110_ts.c b/drivers/input/touchscreen/cy8ctmg110_ts.c
> new file mode 100644
> index 0000000..4adbe87
> --- /dev/null
> +++ b/drivers/input/touchscreen/cy8ctmg110_ts.c
> @@ -0,0 +1,521 @@
> +/*
> + * cy8ctmg110_ts.c Driver for cypress touch screen controller
> + * Copyright (c) 2009 Aava Mobile
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/input.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <asm/io.h>
> +#include <linux/i2c.h>
> +#include <linux/timer.h>
> +#include <linux/gpio.h>
> +#include <linux/hrtimer.h>
> +
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <asm/ioctl.h>
> +#include <asm/uaccess.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <asm/ioctl.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>

wow.  Some of them are there 3 times.  :(

> +
> +#define CY8CTMG110_DRIVER_NAME      "cy8ctmg110"
> +
> +
> +/*HW definations*/

        definitions

> +#define CY8CTMG110_RESET_PIN_GPIO   43
> +#define CY8CTMG110_IRQ_PIN_GPIO     59
> +#define CY8CTMG110_I2C_ADDR         0x38
> +#define CY8CTMG110_I2C_ADDR_EXT     0x39
> +#define CY8CTMG110_I2C_ADDR_        0x2	/*i2c address first sample */
> +#define CY8CTMG110_I2C_ADDR__       53	/*i2c address to FW where irq support missing */
> +#define CY8CTMG110_TOUCH_IRQ        21
> +#define CY8CTMG110_TOUCH_LENGHT     9787
> +#define CY8CTMG110_SCREEN_LENGHT    8424
> +
> +
> +/*Touch coordinates*/
> +#define CY8CTMG110_X_MIN        0
> +#define CY8CTMG110_Y_MIN        0
> +#define CY8CTMG110_X_MAX        864
> +#define CY8CTMG110_Y_MAX        480
> +
> +
> +/*cy8ctmg110 registers defination*/

                          definition

> +#define CY8CTMG110_TOUCH_WAKEUP_TIME   0
> +#define CY8CTMG110_TOUCH_SLEEP_TIME    2
> +#define CY8CTMG110_TOUCH_X1            3
> +#define CY8CTMG110_TOUCH_Y1            5
> +#define CY8CTMG110_TOUCH_X2            7
> +#define CY8CTMG110_TOUCH_Y2            9
> +#define CY8CTMG110_FINGERS             11
> +#define CY8CTMG110_GESTURE             12
> +#define CY8CTMG110_REG_MAX             13
> +
> +#define CY8CTMG110_POLL_TIMER_DELAY  1000*1000*100

better to use parens on expressions:	(1000 * 1000 * 100)

> +#define TOUCH_MAX_I2C_FAILS          50
> +
> +/* Scale factors for coordinates */
> +#define X_SCALE_FACTOR 9387/8424
> +#define Y_SCALE_FACTOR 97/100

use parens.  Oops, probably can't do that here, since:
+		y2 = (u16) (x * Y_SCALE_FACTOR);

you (== the code) want (x * 97) / 100.
Otherwise (with parens) it would be x * 0.
oh well.

> +
> +/* For tracing */
> +static int g_y_trace_coord = 0;
> +module_param(g_y_trace_coord, int, 0600);
> +
> +/* Polling mode */
> +static int polling = 0;
> +module_param(polling, int, 0);
> +MODULE_PARM_DESC(polling, "Set to enabling polling of the touchscreen");

s/enabling/enable/

> +
> +
> +/*
> + * The touch position structure.
> + */
> +struct ts_event {
> +	int x1;
> +	int y1;
> +	int x2;
> +	int y2;
> +	bool event_sended;

	bool event_sent;

> +};
> +
> +/*
> + * The touch driver structure.
> + */
> +struct cy8ctmg110 {
> +	struct input_dev *input;
> +	char phys[32];
> +	struct ts_event tc;
> +	struct i2c_client *client;
> +	bool pending;
> +	spinlock_t lock;
> +	bool initController;
> +	bool sleepmode;
> +	int i2c_fail_count;
> +	struct hrtimer timer;
> +};
> +
> +/*
> + * cy8ctmg110_poweroff is the routine that is called when touch hardware 
> + * will powered off
> + */
> +static void cy8ctmg110_power(bool poweron)
> +{
> +	if (poweron)
> +		gpio_direction_output(CY8CTMG110_RESET_PIN_GPIO, 0);
> +	else
> +		gpio_direction_output(CY8CTMG110_RESET_PIN_GPIO, 1);
> +}
> +
> +/*
> + * cy8ctmg110_write_req write regs to the i2c devices
> + * 
> + */
> +static int cy8ctmg110_write_req(struct cy8ctmg110 *tsc, unsigned char reg,
> +		unsigned char len, unsigned char *value)
> +{
> +	struct i2c_client *client = tsc->client;
> +	unsigned int ret;
> +	unsigned char i2c_data[] = { 0, 0, 0, 0, 0, 0 };
> +	struct i2c_msg msg[] = {
> +			{client->addr, 0, len + 1, i2c_data},
> +			};
> +
> +	i2c_data[0] = reg;
> +	memcpy(i2c_data + 1, value, len);
> +
> +	ret = i2c_transfer(client->adapter, msg, 1);
> +	if (ret != 1) {
> +		printk("cy8ctmg110 touch : i2c write data cmd failed \n");

Better to use DRIVER_NAME as prefix in messages.

> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +/*
> + * cy8ctmg110_read_req read regs from i2c devise
> + * 
> + */
> +
> +static int cy8ctmg110_read_req(struct cy8ctmg110 *tsc,
> +		unsigned char *i2c_data, unsigned char len, unsigned char cmd)
> +{
> +	struct i2c_client *client = tsc->client;
> +	unsigned int ret;
> +	unsigned char regs_cmd[2] = { 0, 0 };
> +	struct i2c_msg msg1[] = {
> +		{client->addr, 0, 1, regs_cmd},
> +	};
> +	struct i2c_msg msg2[] = {
> +		{client->addr, I2C_M_RD, len, i2c_data},
> +	};
> +
> +	regs_cmd[0] = cmd;
> +
> +	/* first write slave position to i2c devices */
> +	ret = i2c_transfer(client->adapter, msg1, 1);
> +	if (ret != 1) {
> +		tsc->i2c_fail_count++;
> +		return ret;
> +	}
> +
> +	/* Second read data from position */
> +	ret = i2c_transfer(client->adapter, msg2, 1);
> +	if (ret != 1) {
> +		tsc->i2c_fail_count++;
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +/*
> + * cy8ctmg110_send_event delevery touch event to the userpace
> + * function use normal input interface
> + */
> +static void cy8ctmg110_send_event(void *tsc)
> +{
> +	struct cy8ctmg110 *ts = tsc;
> +	struct input_dev *input = ts->input;
> +	u16 x, y;
> +	u16 x2, y2;
> +
> +	x = ts->tc.x1;
> +	y = ts->tc.y1;
> +
> +	if (ts->tc.event_sended == false) {
> +		input_report_key(input, BTN_TOUCH, 1);
> +		ts->pending = true;
> +		x2 = (u16) (y * X_SCALE_FACTOR);
> +		y2 = (u16) (x * Y_SCALE_FACTOR);
> +		input_report_abs(input, ABS_X, x2);
> +		input_report_abs(input, ABS_Y, y2);
> +		input_sync(input);
> +		if (g_y_trace_coord)
> +			printk("cy8ctmg110 touch position X:%d (was = %d) Y:%d (was = %d)\n", x2, y, y2, x);
> +	}
> +
> +}
> +
> +/*
> + * cy8ctmg110_touch_pos check touch position from i2c devices
> + * 
> + */
> +static int cy8ctmg110_touch_pos(struct cy8ctmg110 *tsc)
> +{
> +	unsigned char reg_p[CY8CTMG110_REG_MAX];
> +	int x, y;
> +
> +	memset(reg_p, 0, CY8CTMG110_REG_MAX);
> +
> +	/*Reading coordinates */
> +	if (cy8ctmg110_read_req(tsc, reg_p, 9, CY8CTMG110_TOUCH_X1) != 0)
> +		return -EIO;
> +		
> +	y = reg_p[2] << 8 | reg_p[3];
> +	x = reg_p[0] << 8 | reg_p[1];
> +		/*number of touch */
> +	if (reg_p[8] == 0) {
> +		if (tsc->pending == true) {
> +			struct input_dev *input = tsc->input;
> +
> +			input_report_key(input, BTN_TOUCH, 0);
> +			tsc->tc.event_sended = true;
> +			tsc->pending = false;
> +		}
> +	} else if (tsc->tc.x1 != x || tsc->tc.y1 != y) {
> +		tsc->tc.y1 = y;
> +		tsc->tc.x1 = x;
> +		tsc->tc.event_sended = false;
> +		cy8ctmg110_send_event(tsc);
> +	}
> +	return 0;
> +}
> +
> +/*
> + * if interrupt isn't in use the touch positions can reads by polling

                                                    can be read

> + * 
> + */
> +static enum hrtimer_restart cy8ctmg110_timer(struct hrtimer *handle)
> +{
> +	struct cy8ctmg110 *ts = container_of(handle, struct cy8ctmg110, timer);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ts->lock, flags);
> +
> +	cy8ctmg110_touch_pos(ts);
> +	if (ts->i2c_fail_count < TOUCH_MAX_I2C_FAILS)
> +		hrtimer_start(&ts->timer, ktime_set(0, CY8CTMG110_POLL_TIMER_DELAY), HRTIMER_MODE_REL);
> +
> +	spin_unlock_irqrestore(&ts->lock, flags);
> +	return HRTIMER_NORESTART;
> +}
> +
> +/*
> + * cy8ctmg110_init_controller set init value to touchcontroller
> + * 
> + */
> +static bool cy8ctmg110_set_sleepmode(struct cy8ctmg110 *ts)
> +{
> +	unsigned char reg_p[3];
> +
> +	if (ts->sleepmode == true) {
> +		reg_p[0] = 0x00;
> +		reg_p[1] = 0xff;
> +		reg_p[2] = 5;
> +	} else {
> +		reg_p[0] = 0x10;
> +		reg_p[1] = 0xff;
> +		reg_p[2] = 0;
> +	}
> +
> +	if (cy8ctmg110_write_req(ts, CY8CTMG110_TOUCH_WAKEUP_TIME, 3, reg_p))
> +		return false;
> +
> +	ts->initController = true;
> +	return true;
> +}
> +
> +/*
> + * cy8ctmg110_irq_handler irq handling function
> + * 
> + */
> +
> +static irqreturn_t cy8ctmg110_irq_handler(int irq, void *dev_id)
> +{
> +	struct cy8ctmg110 *tsc = (struct cy8ctmg110 *) dev_id;
> +
> +	if (tsc->initController == false) {
> +		if (cy8ctmg110_set_sleepmode(tsc) == true)
> +			tsc->initController = true;
> +	} else
> +		cy8ctmg110_touch_pos(tsc);
> +
> +	/* if interrupt supported in the touch controller
> +	   timer polling need to stop */
> +	tsc->i2c_fail_count = TOUCH_MAX_I2C_FAILS;
> +	return IRQ_HANDLED;
> +}
> +
> +
> +static int cy8ctmg110_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	struct cy8ctmg110 *ts;
> +	struct input_dev *input_dev;
> +	int err;
> +	client->irq = CY8CTMG110_TOUCH_IRQ;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +					I2C_FUNC_SMBUS_READ_WORD_DATA))
> +		return -EIO;
> +
> +	ts = kzalloc(sizeof(struct cy8ctmg110), GFP_KERNEL);
> +	input_dev = input_allocate_device();
> +
> +	if (!ts || !input_dev) {
> +		err = -ENOMEM;
> +		goto err_free_mem;
> +	}
> +
> +	ts->client = client;
> +	i2c_set_clientdata(client, ts);
> +
> +	ts->input = input_dev;
> +	ts->pending = false;
> +	ts->sleepmode = false;
> +
> +	snprintf(ts->phys, sizeof(ts->phys), "%s/input0",
> +						dev_name(&client->dev));
> +
> +	input_dev->name = CY8CTMG110_DRIVER_NAME " Touchscreen";
> +	input_dev->phys = ts->phys;
> +	input_dev->id.bustype = BUS_I2C;
> +
> +	spin_lock_init(&ts->lock);
> +
> +	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP) |
> +					BIT_MASK(EV_REL) | BIT_MASK(EV_ABS);
> +	input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> +
> +	input_set_capability(input_dev, EV_KEY, KEY_F);
> +
> +	input_set_abs_params(input_dev, ABS_X, CY8CTMG110_X_MIN, CY8CTMG110_X_MAX, 0, 0);
> +	input_set_abs_params(input_dev, ABS_Y, CY8CTMG110_Y_MIN, CY8CTMG110_Y_MAX, 0, 0);
> +
> +	err = gpio_request(CY8CTMG110_RESET_PIN_GPIO, NULL);
> +
> +	if (err) {
> +		dev_err(&client->dev, "cy8ctmg110_ts: Unable to request GPIO pin %d.\n",
> +						CY8CTMG110_RESET_PIN_GPIO);
> +		goto err_free_irq;
> +	}
> +	cy8ctmg110_power(true);
> +
> +	ts->initController = false;
> +	ts->i2c_fail_count = 0;
> +
> +	hrtimer_init(&ts->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	ts->timer.function = cy8ctmg110_timer;
> +
> +	if (polling)
> +		hrtimer_start(&ts->timer, ktime_set(10, 0), HRTIMER_MODE_REL);
> +
> +	/* Can we fall back to polling if these bits fail - something to look
> +	   at for robustness */
> +
> +	err = gpio_request(CY8CTMG110_IRQ_PIN_GPIO, "touch_irq_key");
> +	if (err < 0) {
> +		dev_err(&client->dev,
> +			"cy8ctmg110_ts: failed to request GPIO %d, error %d\n",
> +						CY8CTMG110_IRQ_PIN_GPIO, err);
> +		goto err_free_timer;
> +	}
> +
> +	err = gpio_direction_input(CY8CTMG110_IRQ_PIN_GPIO);
> +
> +	if (err < 0) {
> +		dev_err(&client->dev,
> +			"cy8ctmg110_ts: failed to configure input direction for GPIO %d, error %d\n",
> +						CY8CTMG110_IRQ_PIN_GPIO, err);
> +		goto err_free_gpio;
> +	}
> +	client->irq = gpio_to_irq(CY8CTMG110_IRQ_PIN_GPIO);
> +
> +	if (client->irq < 0) {
> +		err = client->irq;
> +		dev_err(&client->dev,
> +	"cy8ctmg110_ts: Unable to get irq number" " for GPIO %d, error %d\n",
> +						CY8CTMG110_IRQ_PIN_GPIO, err);
> +		goto err_free_gpio;
> +	}
> +	err = request_irq(client->irq, cy8ctmg110_irq_handler, IRQF_TRIGGER_RISING | IRQF_SHARED, "touch_reset_key", ts);
> +	if (err < 0) {
> +		dev_err(&client->dev,
> +			"cy8ctmg110 irq %d busy? error %d\n",
> +				client->irq, err);
> +		goto err_free_gpio;
> +	}
> +
> +	err = input_register_device(input_dev);
> +	if (!err)
> +		return 0;
> +err_free_gpio:
> +	gpio_free(CY8CTMG110_IRQ_PIN_GPIO);
> +err_free_timer:
> +	if (polling)
> +		hrtimer_cancel(&ts->timer);
> +err_free_irq:
> +	free_irq(client->irq, ts);
> +err_free_mem:
> +	input_free_device(input_dev);
> +	kfree(ts);
> +	return err;
> +}
> +
> +/*
> + * cy8ctmg110_suspend
> + * 
> + */
> +
> +static int cy8ctmg110_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> +	if (device_may_wakeup(&client->dev))
> +		enable_irq_wake(client->irq);
> +
> +	return 0;
> +}
> +
> +/*
> + * cy8ctmg110_resume 
> + * 
> + */
> +
> +static int cy8ctmg110_resume(struct i2c_client *client)
> +{
> +	if (device_may_wakeup(&client->dev))
> +		disable_irq_wake(client->irq);
> +
> +	return 0;
> +}
> +
> +/*
> + * cy8ctmg110_remove
> + * 
> + */
> +
> +static int cy8ctmg110_remove(struct i2c_client *client)
> +{
> +	struct cy8ctmg110 *ts = i2c_get_clientdata(client);
> +
> +	cy8ctmg110_power(false);
> +
> +	if (polling)
> +		hrtimer_cancel(&ts->timer);
> +	free_irq(client->irq, ts);
> +	input_unregister_device(ts->input);
> +	/* FIXME: Do we need to free the GPIO ? */
> +	kfree(ts);
> +	return 0;
> +}
> +
> +static struct i2c_device_id cy8ctmg110_idtable[] = {
> +	{CY8CTMG110_DRIVER_NAME, 1},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, cy8ctmg110_idtable);
> +
> +static struct i2c_driver cy8ctmg110_driver = {
> +	.driver = {
> +		   .owner = THIS_MODULE,
> +		   .name = CY8CTMG110_DRIVER_NAME,
> +		   .bus = &i2c_bus_type,
> +		   },
> +	.id_table = cy8ctmg110_idtable,
> +	.probe = cy8ctmg110_probe,
> +	.remove = cy8ctmg110_remove,
> +	.suspend = cy8ctmg110_suspend,
> +	.resume = cy8ctmg110_resume,
> +};


---
~Randy

Re: cy8ctmg110 for review

[Joe Perches]

On Wed, 2010-04-14 at 13:54 +0100, Alan Cox wrote:
> Subject: [FOR COMMENT] cy8ctmg110 for review
> 
> From: Samuli Konttila <samuli.konttila-GpSoQilbZ/bZJqsBc5GL+g@public.gmane.org>

> Add support for the cy8ctmg110 capacitive touchscreen used on some embedded
> devices.
> 
> (Some clean up by Alan Cox)

Some more cleanups.  Still not signed.

Cleanup includes
Cleanup typos
Remove a few used-once #defines
Cleanup logging

 drivers/input/touchscreen/cy8ctmg110_ts.c |  145 ++++++++++++----------------
 1 files changed, 62 insertions(+), 83 deletions(-)

diff --git a/drivers/input/touchscreen/cy8ctmg110_ts.c b/drivers/input/touchscreen/cy8ctmg110_ts.c
index 4adbe87..ea41f97 100644
--- a/drivers/input/touchscreen/cy8ctmg110_ts.c
+++ b/drivers/input/touchscreen/cy8ctmg110_ts.c
@@ -16,57 +16,48 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/input.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
-#include <asm/io.h>
 #include <linux/i2c.h>
 #include <linux/timer.h>
 #include <linux/gpio.h>
 #include <linux/hrtimer.h>
-
-#include <linux/platform_device.h>
-#include <linux/delay.h>
-#include <linux/fs.h>
-#include <asm/ioctl.h>
-#include <asm/uaccess.h>
+#include <linux/init.h>
 #include <linux/device.h>
-#include <linux/module.h>
+#include <linux/miscdevice.h>
 #include <linux/platform_device.h>
 #include <linux/delay.h>
 #include <linux/fs.h>
-#include <asm/ioctl.h>
-#include <linux/fs.h>
-#include <linux/init.h>
-#include <linux/miscdevice.h>
-#include <linux/module.h>
-
+#include <linux/io.h>
+#include <linux/ioctl.h>
+#include <linux/uaccess.h>
 
 #define CY8CTMG110_DRIVER_NAME      "cy8ctmg110"
 
-
-/*HW definations*/
+/* HW definitions */
 #define CY8CTMG110_RESET_PIN_GPIO   43
 #define CY8CTMG110_IRQ_PIN_GPIO     59
 #define CY8CTMG110_I2C_ADDR         0x38
 #define CY8CTMG110_I2C_ADDR_EXT     0x39
 #define CY8CTMG110_I2C_ADDR_        0x2	/*i2c address first sample */
-#define CY8CTMG110_I2C_ADDR__       53	/*i2c address to FW where irq support missing */
+#define CY8CTMG110_I2C_ADDR__       53	/*i2c address to FW where
+					  irq support missing */
 #define CY8CTMG110_TOUCH_IRQ        21
-#define CY8CTMG110_TOUCH_LENGHT     9787
-#define CY8CTMG110_SCREEN_LENGHT    8424
-
+#define CY8CTMG110_TOUCH_LENGTH     9787
+#define CY8CTMG110_SCREEN_LENGTH    8424
 
-/*Touch coordinates*/
+/* Touch coordinates */
 #define CY8CTMG110_X_MIN        0
 #define CY8CTMG110_Y_MIN        0
 #define CY8CTMG110_X_MAX        864
 #define CY8CTMG110_Y_MAX        480
 
-
-/*cy8ctmg110 registers defination*/
+/* cy8ctmg110 registers definition */
 #define CY8CTMG110_TOUCH_WAKEUP_TIME   0
 #define CY8CTMG110_TOUCH_SLEEP_TIME    2
 #define CY8CTMG110_TOUCH_X1            3
@@ -77,21 +68,17 @@
 #define CY8CTMG110_GESTURE             12
 #define CY8CTMG110_REG_MAX             13
 
-#define CY8CTMG110_POLL_TIMER_DELAY  1000*1000*100
+#define CY8CTMG110_POLL_TIMER_DELAY  (1000 * 1000 * 100)
 #define TOUCH_MAX_I2C_FAILS          50
 
-/* Scale factors for coordinates */
-#define X_SCALE_FACTOR 9387/8424
-#define Y_SCALE_FACTOR 97/100
-
 /* For tracing */
-static int g_y_trace_coord = 0;
+static int g_y_trace_coord;
 module_param(g_y_trace_coord, int, 0600);
 
 /* Polling mode */
-static int polling = 0;
+static int polling;
 module_param(polling, int, 0);
-MODULE_PARM_DESC(polling, "Set to enabling polling of the touchscreen");
+MODULE_PARM_DESC(polling, "Set to enable polling of the touchscreen");
 
 
 /*
@@ -102,7 +89,7 @@ struct ts_event {
 	int y1;
 	int x2;
 	int y2;
-	bool event_sended;
+	bool event_sent;
 };
 
 /*
@@ -122,8 +109,8 @@ struct cy8ctmg110 {
 };
 
 /*
- * cy8ctmg110_poweroff is the routine that is called when touch hardware 
- * will powered off
+ * cy8ctmg110_poweroff is the routine that is called
+ * when touch hardware will be powered off
  */
 static void cy8ctmg110_power(bool poweron)
 {
@@ -135,7 +122,6 @@ static void cy8ctmg110_power(bool poweron)
 
 /*
  * cy8ctmg110_write_req write regs to the i2c devices
- * 
  */
 static int cy8ctmg110_write_req(struct cy8ctmg110 *tsc, unsigned char reg,
 		unsigned char len, unsigned char *value)
@@ -152,7 +138,7 @@ static int cy8ctmg110_write_req(struct cy8ctmg110 *tsc, unsigned char reg,
 
 	ret = i2c_transfer(client->adapter, msg, 1);
 	if (ret != 1) {
-		printk("cy8ctmg110 touch : i2c write data cmd failed \n");
+		pr_err("i2c write data cmd failed\n");
 		return ret;
 	}
 	return 0;
@@ -160,9 +146,7 @@ static int cy8ctmg110_write_req(struct cy8ctmg110 *tsc, unsigned char reg,
 
 /*
  * cy8ctmg110_read_req read regs from i2c devise
- * 
  */
-
 static int cy8ctmg110_read_req(struct cy8ctmg110 *tsc,
 		unsigned char *i2c_data, unsigned char len, unsigned char cmd)
 {
@@ -208,23 +192,23 @@ static void cy8ctmg110_send_event(void *tsc)
 	x = ts->tc.x1;
 	y = ts->tc.y1;
 
-	if (ts->tc.event_sended == false) {
+	if (!ts->tc.event_sent) {
 		input_report_key(input, BTN_TOUCH, 1);
 		ts->pending = true;
-		x2 = (u16) (y * X_SCALE_FACTOR);
-		y2 = (u16) (x * Y_SCALE_FACTOR);
+		x2 = (u16)(y * 9387 / 8424);
+		y2 = (u16)(x * 97 / 100);
 		input_report_abs(input, ABS_X, x2);
 		input_report_abs(input, ABS_Y, y2);
 		input_sync(input);
 		if (g_y_trace_coord)
-			printk("cy8ctmg110 touch position X:%d (was = %d) Y:%d (was = %d)\n", x2, y, y2, x);
+			pr_info("position X:%d (was = %d) Y:%d (was = %d)\n",
+				x2, y, y2, x);
 	}
 
 }
 
 /*
  * cy8ctmg110_touch_pos check touch position from i2c devices
- * 
  */
 static int cy8ctmg110_touch_pos(struct cy8ctmg110 *tsc)
 {
@@ -236,30 +220,29 @@ static int cy8ctmg110_touch_pos(struct cy8ctmg110 *tsc)
 	/*Reading coordinates */
 	if (cy8ctmg110_read_req(tsc, reg_p, 9, CY8CTMG110_TOUCH_X1) != 0)
 		return -EIO;
-		
+
 	y = reg_p[2] << 8 | reg_p[3];
 	x = reg_p[0] << 8 | reg_p[1];
 		/*number of touch */
 	if (reg_p[8] == 0) {
-		if (tsc->pending == true) {
+		if (tsc->pending) {
 			struct input_dev *input = tsc->input;
 
 			input_report_key(input, BTN_TOUCH, 0);
-			tsc->tc.event_sended = true;
+			tsc->tc.event_sent = true;
 			tsc->pending = false;
 		}
 	} else if (tsc->tc.x1 != x || tsc->tc.y1 != y) {
 		tsc->tc.y1 = y;
 		tsc->tc.x1 = x;
-		tsc->tc.event_sended = false;
+		tsc->tc.event_sent = false;
 		cy8ctmg110_send_event(tsc);
 	}
 	return 0;
 }
 
 /*
- * if interrupt isn't in use the touch positions can reads by polling
- * 
+ * if interrupt isn't in use the touch positions can be read by polling
  */
 static enum hrtimer_restart cy8ctmg110_timer(struct hrtimer *handle)
 {
@@ -270,7 +253,9 @@ static enum hrtimer_restart cy8ctmg110_timer(struct hrtimer *handle)
 
 	cy8ctmg110_touch_pos(ts);
 	if (ts->i2c_fail_count < TOUCH_MAX_I2C_FAILS)
-		hrtimer_start(&ts->timer, ktime_set(0, CY8CTMG110_POLL_TIMER_DELAY), HRTIMER_MODE_REL);
+		hrtimer_start(&ts->timer,
+			      ktime_set(0, CY8CTMG110_POLL_TIMER_DELAY),
+			      HRTIMER_MODE_REL);
 
 	spin_unlock_irqrestore(&ts->lock, flags);
 	return HRTIMER_NORESTART;
@@ -278,13 +263,12 @@ static enum hrtimer_restart cy8ctmg110_timer(struct hrtimer *handle)
 
 /*
  * cy8ctmg110_init_controller set init value to touchcontroller
- * 
  */
 static bool cy8ctmg110_set_sleepmode(struct cy8ctmg110 *ts)
 {
 	unsigned char reg_p[3];
 
-	if (ts->sleepmode == true) {
+	if (ts->sleepmode) {
 		reg_p[0] = 0x00;
 		reg_p[1] = 0xff;
 		reg_p[2] = 5;
@@ -303,15 +287,13 @@ static bool cy8ctmg110_set_sleepmode(struct cy8ctmg110 *ts)
 
 /*
  * cy8ctmg110_irq_handler irq handling function
- * 
  */
-
 static irqreturn_t cy8ctmg110_irq_handler(int irq, void *dev_id)
 {
 	struct cy8ctmg110 *tsc = (struct cy8ctmg110 *) dev_id;
 
-	if (tsc->initController == false) {
-		if (cy8ctmg110_set_sleepmode(tsc) == true)
+	if (!tsc->initController) {
+		if (cy8ctmg110_set_sleepmode(tsc))
 			tsc->initController = true;
 	} else
 		cy8ctmg110_touch_pos(tsc);
@@ -322,8 +304,8 @@ static irqreturn_t cy8ctmg110_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-
-static int cy8ctmg110_probe(struct i2c_client *client, const struct i2c_device_id *id)
+static int cy8ctmg110_probe(struct i2c_client *client,
+			    const struct i2c_device_id *id)
 {
 	struct cy8ctmg110 *ts;
 	struct input_dev *input_dev;
@@ -350,7 +332,7 @@ static int cy8ctmg110_probe(struct i2c_client *client, const struct i2c_device_i
 	ts->sleepmode = false;
 
 	snprintf(ts->phys, sizeof(ts->phys), "%s/input0",
-						dev_name(&client->dev));
+		 dev_name(&client->dev));
 
 	input_dev->name = CY8CTMG110_DRIVER_NAME " Touchscreen";
 	input_dev->phys = ts->phys;
@@ -358,20 +340,22 @@ static int cy8ctmg110_probe(struct i2c_client *client, const struct i2c_device_i
 
 	spin_lock_init(&ts->lock);
 
-	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP) |
-					BIT_MASK(EV_REL) | BIT_MASK(EV_ABS);
+	input_dev->evbit[0] = (BIT_MASK(EV_KEY) | BIT_MASK(EV_REP) |
+			       BIT_MASK(EV_REL) | BIT_MASK(EV_ABS));
 	input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
 
 	input_set_capability(input_dev, EV_KEY, KEY_F);
 
-	input_set_abs_params(input_dev, ABS_X, CY8CTMG110_X_MIN, CY8CTMG110_X_MAX, 0, 0);
-	input_set_abs_params(input_dev, ABS_Y, CY8CTMG110_Y_MIN, CY8CTMG110_Y_MAX, 0, 0);
+	input_set_abs_params(input_dev, ABS_X,
+			     CY8CTMG110_X_MIN, CY8CTMG110_X_MAX, 0, 0);
+	input_set_abs_params(input_dev, ABS_Y,
+			     CY8CTMG110_Y_MIN, CY8CTMG110_Y_MAX, 0, 0);
 
 	err = gpio_request(CY8CTMG110_RESET_PIN_GPIO, NULL);
 
 	if (err) {
-		dev_err(&client->dev, "cy8ctmg110_ts: Unable to request GPIO pin %d.\n",
-						CY8CTMG110_RESET_PIN_GPIO);
+		dev_err(&client->dev, "Unable to request GPIO pin %d\n",
+			CY8CTMG110_RESET_PIN_GPIO);
 		goto err_free_irq;
 	}
 	cy8ctmg110_power(true);
@@ -385,14 +369,14 @@ static int cy8ctmg110_probe(struct i2c_client *client, const struct i2c_device_i
 	if (polling)
 		hrtimer_start(&ts->timer, ktime_set(10, 0), HRTIMER_MODE_REL);
 
-	/* Can we fall back to polling if these bits fail - something to look
-	   at for robustness */
+	/* Can we fall back to polling if these bits fail?
+	   - something to look at for robustness */
 
 	err = gpio_request(CY8CTMG110_IRQ_PIN_GPIO, "touch_irq_key");
 	if (err < 0) {
 		dev_err(&client->dev,
-			"cy8ctmg110_ts: failed to request GPIO %d, error %d\n",
-						CY8CTMG110_IRQ_PIN_GPIO, err);
+			"failed to request GPIO %d, error %d\n",
+			CY8CTMG110_IRQ_PIN_GPIO, err);
 		goto err_free_timer;
 	}
 
@@ -400,8 +384,8 @@ static int cy8ctmg110_probe(struct i2c_client *client, const struct i2c_device_i
 
 	if (err < 0) {
 		dev_err(&client->dev,
-			"cy8ctmg110_ts: failed to configure input direction for GPIO %d, error %d\n",
-						CY8CTMG110_IRQ_PIN_GPIO, err);
+			"failed to configure input direction for GPIO %d, error %d\n",
+			CY8CTMG110_IRQ_PIN_GPIO, err);
 		goto err_free_gpio;
 	}
 	client->irq = gpio_to_irq(CY8CTMG110_IRQ_PIN_GPIO);
@@ -409,15 +393,16 @@ static int cy8ctmg110_probe(struct i2c_client *client, const struct i2c_device_i
 	if (client->irq < 0) {
 		err = client->irq;
 		dev_err(&client->dev,
-	"cy8ctmg110_ts: Unable to get irq number" " for GPIO %d, error %d\n",
-						CY8CTMG110_IRQ_PIN_GPIO, err);
+			"Unable to get irq number for GPIO %d, error %d\n",
+			CY8CTMG110_IRQ_PIN_GPIO, err);
 		goto err_free_gpio;
 	}
-	err = request_irq(client->irq, cy8ctmg110_irq_handler, IRQF_TRIGGER_RISING | IRQF_SHARED, "touch_reset_key", ts);
+	err = request_irq(client->irq, cy8ctmg110_irq_handler,
+			  IRQF_TRIGGER_RISING | IRQF_SHARED,
+			  "touch_reset_key", ts);
 	if (err < 0) {
 		dev_err(&client->dev,
-			"cy8ctmg110 irq %d busy? error %d\n",
-				client->irq, err);
+			"cy8ctmg110 irq %d busy? error %d\n", client->irq, err);
 		goto err_free_gpio;
 	}
 
@@ -439,9 +424,7 @@ err_free_mem:
 
 /*
  * cy8ctmg110_suspend
- * 
  */
-
 static int cy8ctmg110_suspend(struct i2c_client *client, pm_message_t mesg)
 {
 	if (device_may_wakeup(&client->dev))
@@ -451,10 +434,8 @@ static int cy8ctmg110_suspend(struct i2c_client *client, pm_message_t mesg)
 }
 
 /*
- * cy8ctmg110_resume 
- * 
+ * cy8ctmg110_resume
  */
-
 static int cy8ctmg110_resume(struct i2c_client *client)
 {
 	if (device_may_wakeup(&client->dev))
@@ -465,9 +446,7 @@ static int cy8ctmg110_resume(struct i2c_client *client)
 
 /*
  * cy8ctmg110_remove
- * 
  */
-
 static int cy8ctmg110_remove(struct i2c_client *client)
 {
 	struct cy8ctmg110 *ts = i2c_get_clientdata(client);

Re: your mail

[Dmitry Torokhov]

On Wed, Apr 14, 2010 at 01:54:02PM +0100, Alan Cox wrote:
> Subject: [FOR COMMENT] cy8ctmg110 for review
> 
> From: Samuli Konttila <samuli.konttila@aavamobile.com>
> 
> Add support for the cy8ctmg110 capacitive touchscreen used on some embedded
> devices.
> 
> (Some clean up by Alan Cox)
> 
> (No signed off, not yet ready to go in)
> ---
> 
>  drivers/input/touchscreen/Kconfig         |   12 +
>  drivers/input/touchscreen/Makefile        |    3 
>  drivers/input/touchscreen/cy8ctmg110_ts.c |  521 +++++++++++++++++++++++++++++
>  3 files changed, 535 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/input/touchscreen/cy8ctmg110_ts.c
> 
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index b3ba374..89a3eb1 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -591,4 +591,16 @@ config TOUCHSCREEN_TPS6507X
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called tps6507x_ts.
>  
> +config TOUCHSCREEN_CY8CTMG110
> +	tristate "cy8ctmg110 touchscreen"
> +	depends on I2C
> +	help
> +	  Say Y here if you have a cy8ctmg110 touchscreen capacitive
> +	  touchscreen
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called cy8ctmg110_ts.
> +
>  endif
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index dfb7239..c7acb65 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -1,5 +1,5 @@
>  #
> -# Makefile for the touchscreen drivers.
> +# Makefile for the touchscreen drivers.mororor
>  #
>  
>  # Each configuration option enables a list of files.
> @@ -12,6 +12,7 @@ obj-$(CONFIG_TOUCHSCREEN_AD7879)	+= ad7879.o
>  obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
>  obj-$(CONFIG_TOUCHSCREEN_ATMEL_TSADCC)	+= atmel_tsadcc.o
>  obj-$(CONFIG_TOUCHSCREEN_BITSY)		+= h3600_ts_input.o
> +obj-$(CONFIG_TOUCHSCREEN_CY8CTMG110)    += cy8ctmg110_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_DYNAPRO)	+= dynapro.o
>  obj-$(CONFIG_TOUCHSCREEN_GUNZE)		+= gunze.o
>  obj-$(CONFIG_TOUCHSCREEN_EETI)		+= eeti_ts.o
> diff --git a/drivers/input/touchscreen/cy8ctmg110_ts.c b/drivers/input/touchscreen/cy8ctmg110_ts.c
> new file mode 100644
> index 0000000..4adbe87
> --- /dev/null
> +++ b/drivers/input/touchscreen/cy8ctmg110_ts.c
> @@ -0,0 +1,521 @@
> +/*
> + * cy8ctmg110_ts.c Driver for cypress touch screen controller
> + * Copyright (c) 2009 Aava Mobile
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/input.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <asm/io.h>
> +#include <linux/i2c.h>
> +#include <linux/timer.h>
> +#include <linux/gpio.h>
> +#include <linux/hrtimer.h>
> +
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <asm/ioctl.h>
> +#include <asm/uaccess.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <asm/ioctl.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +
> +
> +#define CY8CTMG110_DRIVER_NAME      "cy8ctmg110"
> +
> +
> +/*HW definations*/
> +#define CY8CTMG110_RESET_PIN_GPIO   43
> +#define CY8CTMG110_IRQ_PIN_GPIO     59
> +#define CY8CTMG110_I2C_ADDR         0x38
> +#define CY8CTMG110_I2C_ADDR_EXT     0x39
> +#define CY8CTMG110_I2C_ADDR_        0x2	/*i2c address first sample */
> +#define CY8CTMG110_I2C_ADDR__       53	/*i2c address to FW where irq support missing */
> +#define CY8CTMG110_TOUCH_IRQ        21
> +#define CY8CTMG110_TOUCH_LENGHT     9787
> +#define CY8CTMG110_SCREEN_LENGHT    8424
> +
> +
> +/*Touch coordinates*/
> +#define CY8CTMG110_X_MIN        0
> +#define CY8CTMG110_Y_MIN        0
> +#define CY8CTMG110_X_MAX        864
> +#define CY8CTMG110_Y_MAX        480
> +
> +
> +/*cy8ctmg110 registers defination*/
> +#define CY8CTMG110_TOUCH_WAKEUP_TIME   0
> +#define CY8CTMG110_TOUCH_SLEEP_TIME    2
> +#define CY8CTMG110_TOUCH_X1            3
> +#define CY8CTMG110_TOUCH_Y1            5
> +#define CY8CTMG110_TOUCH_X2            7
> +#define CY8CTMG110_TOUCH_Y2            9
> +#define CY8CTMG110_FINGERS             11
> +#define CY8CTMG110_GESTURE             12
> +#define CY8CTMG110_REG_MAX             13
> +
> +#define CY8CTMG110_POLL_TIMER_DELAY  1000*1000*100
> +#define TOUCH_MAX_I2C_FAILS          50
> +
> +/* Scale factors for coordinates */
> +#define X_SCALE_FACTOR 9387/8424
> +#define Y_SCALE_FACTOR 97/100
> +
> +/* For tracing */
> +static int g_y_trace_coord = 0;
> +module_param(g_y_trace_coord, int, 0600);
> +
> +/* Polling mode */
> +static int polling = 0;
> +module_param(polling, int, 0);
> +MODULE_PARM_DESC(polling, "Set to enabling polling of the touchscreen");
> +
> +
> +/*
> + * The touch position structure.
> + */
> +struct ts_event {
> +	int x1;
> +	int y1;
> +	int x2;
> +	int y2;
> +	bool event_sended;
> +};
> +
> +/*
> + * The touch driver structure.
> + */
> +struct cy8ctmg110 {
> +	struct input_dev *input;
> +	char phys[32];
> +	struct ts_event tc;
> +	struct i2c_client *client;
> +	bool pending;
> +	spinlock_t lock;
> +	bool initController;
> +	bool sleepmode;
> +	int i2c_fail_count;
> +	struct hrtimer timer;
> +};
> +
> +/*
> + * cy8ctmg110_poweroff is the routine that is called when touch hardware 
> + * will powered off
> + */
> +static void cy8ctmg110_power(bool poweron)
> +{
> +	if (poweron)
> +		gpio_direction_output(CY8CTMG110_RESET_PIN_GPIO, 0);
> +	else
> +		gpio_direction_output(CY8CTMG110_RESET_PIN_GPIO, 1);
> +}
> +
> +/*
> + * cy8ctmg110_write_req write regs to the i2c devices
> + * 
> + */
> +static int cy8ctmg110_write_req(struct cy8ctmg110 *tsc, unsigned char reg,
> +		unsigned char len, unsigned char *value)
> +{
> +	struct i2c_client *client = tsc->client;
> +	unsigned int ret;
> +	unsigned char i2c_data[] = { 0, 0, 0, 0, 0, 0 };
> +	struct i2c_msg msg[] = {
> +			{client->addr, 0, len + 1, i2c_data},
> +			};
> +
> +	i2c_data[0] = reg;
> +	memcpy(i2c_data + 1, value, len);
> +
> +	ret = i2c_transfer(client->adapter, msg, 1);
> +	if (ret != 1) {
> +		printk("cy8ctmg110 touch : i2c write data cmd failed \n");
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +/*
> + * cy8ctmg110_read_req read regs from i2c devise
> + * 
> + */
> +
> +static int cy8ctmg110_read_req(struct cy8ctmg110 *tsc,
> +		unsigned char *i2c_data, unsigned char len, unsigned char cmd)
> +{
> +	struct i2c_client *client = tsc->client;
> +	unsigned int ret;
> +	unsigned char regs_cmd[2] = { 0, 0 };
> +	struct i2c_msg msg1[] = {
> +		{client->addr, 0, 1, regs_cmd},
> +	};
> +	struct i2c_msg msg2[] = {
> +		{client->addr, I2C_M_RD, len, i2c_data},
> +	};
> +
> +	regs_cmd[0] = cmd;
> +
> +	/* first write slave position to i2c devices */
> +	ret = i2c_transfer(client->adapter, msg1, 1);
> +	if (ret != 1) {
> +		tsc->i2c_fail_count++;
> +		return ret;
> +	}
> +
> +	/* Second read data from position */
> +	ret = i2c_transfer(client->adapter, msg2, 1);
> +	if (ret != 1) {
> +		tsc->i2c_fail_count++;
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +/*
> + * cy8ctmg110_send_event delevery touch event to the userpace
> + * function use normal input interface
> + */
> +static void cy8ctmg110_send_event(void *tsc)
> +{
> +	struct cy8ctmg110 *ts = tsc;
> +	struct input_dev *input = ts->input;
> +	u16 x, y;
> +	u16 x2, y2;
> +
> +	x = ts->tc.x1;
> +	y = ts->tc.y1;
> +
> +	if (ts->tc.event_sended == false) {

We set "event_sended" to false immediately before calling
cy8ctmg110_send_event() so I do not see the point of this flag.

> +		input_report_key(input, BTN_TOUCH, 1);
> +		ts->pending = true;
> +		x2 = (u16) (y * X_SCALE_FACTOR);
> +		y2 = (u16) (x * Y_SCALE_FACTOR);
> +		input_report_abs(input, ABS_X, x2);
> +		input_report_abs(input, ABS_Y, y2);
> +		input_sync(input);
> +		if (g_y_trace_coord)
> +			printk("cy8ctmg110 touch position X:%d (was = %d) Y:%d (was = %d)\n", x2, y, y2, x);

Do we really need this? Seems to be early development diagnostic.

> +	}
> +
> +}
> +
> +/*
> + * cy8ctmg110_touch_pos check touch position from i2c devices
> + * 
> + */
> +static int cy8ctmg110_touch_pos(struct cy8ctmg110 *tsc)
> +{
> +	unsigned char reg_p[CY8CTMG110_REG_MAX];
> +	int x, y;
> +
> +	memset(reg_p, 0, CY8CTMG110_REG_MAX);
> +
> +	/*Reading coordinates */
> +	if (cy8ctmg110_read_req(tsc, reg_p, 9, CY8CTMG110_TOUCH_X1) != 0)
> +		return -EIO;
> +		
> +	y = reg_p[2] << 8 | reg_p[3];
> +	x = reg_p[0] << 8 | reg_p[1];
> +		/*number of touch */
> +	if (reg_p[8] == 0) {
> +		if (tsc->pending == true) {
> +			struct input_dev *input = tsc->input;
> +
> +			input_report_key(input, BTN_TOUCH, 0);
> +			tsc->tc.event_sended = true;
> +			tsc->pending = false;
> +		}

Just do input_report_key(input, BTN_TOUCH, 0); and let input core take
care of filtering duplicates. This will allow you get rid of bunch of
flags. Also input_sync() is missing here.

> +	} else if (tsc->tc.x1 != x || tsc->tc.y1 != y) {
> +		tsc->tc.y1 = y;
> +		tsc->tc.x1 = x;
> +		tsc->tc.event_sended = false;
> +		cy8ctmg110_send_event(tsc);
> +	}
> +	return 0;
> +}
> +
> +/*
> + * if interrupt isn't in use the touch positions can reads by polling
> + * 
> + */
> +static enum hrtimer_restart cy8ctmg110_timer(struct hrtimer *handle)
> +{
> +	struct cy8ctmg110 *ts = container_of(handle, struct cy8ctmg110, timer);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ts->lock, flags);
> +
> +	cy8ctmg110_touch_pos(ts);
> +	if (ts->i2c_fail_count < TOUCH_MAX_I2C_FAILS)
> +		hrtimer_start(&ts->timer, ktime_set(0, CY8CTMG110_POLL_TIMER_DELAY), HRTIMER_MODE_REL);
> +

So device simply dies after so many errors?

> +	spin_unlock_irqrestore(&ts->lock, flags);

The timer handler is the only user for the spinlock, what is the point?

> +	return HRTIMER_NORESTART;
> +}
> +
> +/*
> + * 
> + */
> +static bool cy8ctmg110_set_sleepmode(struct cy8ctmg110 *ts)
> +{
> +	unsigned char reg_p[3];
> +
> +	if (ts->sleepmode == true) {
> +		reg_p[0] = 0x00;
> +		reg_p[1] = 0xff;
> +		reg_p[2] = 5;
> +	} else {
> +		reg_p[0] = 0x10;
> +		reg_p[1] = 0xff;
> +		reg_p[2] = 0;
> +	}
> +
> +	if (cy8ctmg110_write_req(ts, CY8CTMG110_TOUCH_WAKEUP_TIME, 3, reg_p))
> +		return false;
> +
> +	ts->initController = true;
> +	return true;
> +}
> +
> +/*
> + * cy8ctmg110_irq_handler irq handling function
> + * 
> + */
> +
> +static irqreturn_t cy8ctmg110_irq_handler(int irq, void *dev_id)
> +{
> +	struct cy8ctmg110 *tsc = (struct cy8ctmg110 *) dev_id;
> +
> +	if (tsc->initController == false) {
> +		if (cy8ctmg110_set_sleepmode(tsc) == true)
> +			tsc->initController = true;
> +	} else
> +		cy8ctmg110_touch_pos(tsc);

Initalizing device from interrupt handler is quite novel concept...

> +
> +	/* if interrupt supported in the touch controller
> +	   timer polling need to stop */
> +	tsc->i2c_fail_count = TOUCH_MAX_I2C_FAILS;
> +	return IRQ_HANDLED;
> +}
> +
> +
> +static int cy8ctmg110_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	struct cy8ctmg110 *ts;
> +	struct input_dev *input_dev;
> +	int err;
> +	client->irq = CY8CTMG110_TOUCH_IRQ;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +					I2C_FUNC_SMBUS_READ_WORD_DATA))
> +		return -EIO;
> +
> +	ts = kzalloc(sizeof(struct cy8ctmg110), GFP_KERNEL);
> +	input_dev = input_allocate_device();
> +
> +	if (!ts || !input_dev) {
> +		err = -ENOMEM;
> +		goto err_free_mem;
> +	}
> +
> +	ts->client = client;
> +	i2c_set_clientdata(client, ts);
> +
> +	ts->input = input_dev;
> +	ts->pending = false;
> +	ts->sleepmode = false;
> +
> +	snprintf(ts->phys, sizeof(ts->phys), "%s/input0",
> +						dev_name(&client->dev));
> +
> +	input_dev->name = CY8CTMG110_DRIVER_NAME " Touchscreen";
> +	input_dev->phys = ts->phys;
> +	input_dev->id.bustype = BUS_I2C;
> +
> +	spin_lock_init(&ts->lock);
> +
> +	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP) |

You usually do not set up autorepeat for pointingt devices.

> +					BIT_MASK(EV_REL) | BIT_MASK(EV_ABS);

The device does not emit relative events.

> +	input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> +
> +	input_set_capability(input_dev, EV_KEY, KEY_F);

KEY_F?

> +
> +	input_set_abs_params(input_dev, ABS_X, CY8CTMG110_X_MIN, CY8CTMG110_X_MAX, 0, 0);
> +	input_set_abs_params(input_dev, ABS_Y, CY8CTMG110_Y_MIN, CY8CTMG110_Y_MAX, 0, 0);
> +
> +	err = gpio_request(CY8CTMG110_RESET_PIN_GPIO, NULL);
> +
> +	if (err) {
> +		dev_err(&client->dev, "cy8ctmg110_ts: Unable to request GPIO pin %d.\n",
> +						CY8CTMG110_RESET_PIN_GPIO);
> +		goto err_free_irq;
> +	}
> +	cy8ctmg110_power(true);
> +
> +	ts->initController = false;
> +	ts->i2c_fail_count = 0;
> +
> +	hrtimer_init(&ts->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	ts->timer.function = cy8ctmg110_timer;
> +
> +	if (polling)
> +		hrtimer_start(&ts->timer, ktime_set(10, 0), HRTIMER_MODE_REL);
> +

Polling mode shoudl be controlled by platform data, not kernel module I think.

> +	/* Can we fall back to polling if these bits fail - something to look
> +	   at for robustness */
> +
> +	err = gpio_request(CY8CTMG110_IRQ_PIN_GPIO, "touch_irq_key");
> +	if (err < 0) {
> +		dev_err(&client->dev,
> +			"cy8ctmg110_ts: failed to request GPIO %d, error %d\n",
> +						CY8CTMG110_IRQ_PIN_GPIO, err);
> +		goto err_free_timer;
> +	}
> +
> +	err = gpio_direction_input(CY8CTMG110_IRQ_PIN_GPIO);
> +
> +	if (err < 0) {
> +		dev_err(&client->dev,
> +			"cy8ctmg110_ts: failed to configure input direction for GPIO %d, error %d\n",
> +						CY8CTMG110_IRQ_PIN_GPIO, err);
> +		goto err_free_gpio;
> +	}
> +	client->irq = gpio_to_irq(CY8CTMG110_IRQ_PIN_GPIO);
> +
> +	if (client->irq < 0) {
> +		err = client->irq;
> +		dev_err(&client->dev,
> +	"cy8ctmg110_ts: Unable to get irq number" " for GPIO %d, error %d\n",
> +						CY8CTMG110_IRQ_PIN_GPIO, err);
> +		goto err_free_gpio;
> +	}
> +	err = request_irq(client->irq, cy8ctmg110_irq_handler, IRQF_TRIGGER_RISING | IRQF_SHARED, "touch_reset_key", ts);
> +	if (err < 0) {
> +		dev_err(&client->dev,
> +			"cy8ctmg110 irq %d busy? error %d\n",
> +				client->irq, err);
> +		goto err_free_gpio;
> +	}
> +
> +	err = input_register_device(input_dev);
> +	if (!err)
> +		return 0;
> +err_free_gpio:
> +	gpio_free(CY8CTMG110_IRQ_PIN_GPIO);
> +err_free_timer:
> +	if (polling)
> +		hrtimer_cancel(&ts->timer);
> +err_free_irq:
> +	free_irq(client->irq, ts);
> +err_free_mem:
> +	input_free_device(input_dev);
> +	kfree(ts);
> +	return err;
> +}
> +
> +/*
> + * cy8ctmg110_suspend
> + * 
> + */
> +
> +static int cy8ctmg110_suspend(struct i2c_client *client, pm_message_t mesg)
> +{

Stop timer here? Also power down the device?

> +	if (device_may_wakeup(&client->dev))
> +		enable_irq_wake(client->irq);
> +
> +	return 0;
> +}
> +
> +/*
> + * cy8ctmg110_resume 
> + * 
> + */
> +
> +static int cy8ctmg110_resume(struct i2c_client *client)
> +{
> +	if (device_may_wakeup(&client->dev))
> +		disable_irq_wake(client->irq);
> +
> +	return 0;
> +}
> +
> +/*
> + * cy8ctmg110_remove
> + * 
> + */
> +
> +static int cy8ctmg110_remove(struct i2c_client *client)
> +{
> +	struct cy8ctmg110 *ts = i2c_get_clientdata(client);
> +
> +	cy8ctmg110_power(false);
> +
> +	if (polling)
> +		hrtimer_cancel(&ts->timer);

Implement close() method and move the code above there? Also do open().

> +	free_irq(client->irq, ts);
> +	input_unregister_device(ts->input);
> +	/* FIXME: Do we need to free the GPIO ? */
> +	kfree(ts);
> +	return 0;
> +}
> +
> +static struct i2c_device_id cy8ctmg110_idtable[] = {
> +	{CY8CTMG110_DRIVER_NAME, 1},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, cy8ctmg110_idtable);
> +
> +static struct i2c_driver cy8ctmg110_driver = {
> +	.driver = {
> +		   .owner = THIS_MODULE,
> +		   .name = CY8CTMG110_DRIVER_NAME,
> +		   .bus = &i2c_bus_type,
> +		   },
> +	.id_table = cy8ctmg110_idtable,
> +	.probe = cy8ctmg110_probe,
> +	.remove = cy8ctmg110_remove,
> +	.suspend = cy8ctmg110_suspend,
> +	.resume = cy8ctmg110_resume,
> +};
> +
> +static int __init cy8ctmg110_init(void)
> +{
> +	return i2c_add_driver(&cy8ctmg110_driver);
> +}
> +
> +static void __exit cy8ctmg110_exit(void)
> +{
> +	i2c_del_driver(&cy8ctmg110_driver);
> +}
> +
> +module_init(cy8ctmg110_init);
> +module_exit(cy8ctmg110_exit);
> +
> +MODULE_AUTHOR("Samuli Konttila <samuli.konttila@aavamobile.com>");
> +MODULE_DESCRIPTION("cy8ctmg110 TouchScreen Driver");
> +MODULE_LICENSE("GPL v2");
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Dmitry

Re: your mail

[Rafi Rubin]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

>> +	if (ts->tc.event_sended == false) {
> 
> We set "event_sended" to false immediately before calling
> cy8ctmg110_send_event() so I do not see the point of this flag.

On that note:

$ git grep -n sended
drivers/net/eth16i.c:1295:
		how many packets there is to be sended */
drivers/net/wan/sbni.c:638:
		/* if frame was sended but not ACK'ed - resend it */
drivers/net/wan/sbni.c:659:
		* frame sended then in prepare_to_send next frame
drivers/usb/serial/aircable.c:13:
		* next two bytes must say how much data will be sended.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkvHpB4ACgkQwuRiAT9o609wAgCfbGjTP2lIN6JJyX28VzjPHxTY
ylIAn15FZRPpBEkWaFR8oAFKCCRmNF4d
=u4nx
-----END PGP SIGNATURE-----

Re: your mail

[Dmitry Torokhov]

On Thu, Apr 15, 2010 at 07:41:22PM -0400, Rafi Rubin wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> >> +	if (ts->tc.event_sended == false) {
> > 
> > We set "event_sended" to false immediately before calling
> > cy8ctmg110_send_event() so I do not see the point of this flag.
> 
> On that note:
> 
> $ git grep -n sended
> drivers/net/eth16i.c:1295:
> 		how many packets there is to be sended */
> drivers/net/wan/sbni.c:638:
> 		/* if frame was sended but not ACK'ed - resend it */
> drivers/net/wan/sbni.c:659:
> 		* frame sended then in prepare_to_send next frame
> drivers/usb/serial/aircable.c:13:
> 		* next two bytes must say how much data will be sended.
> 

Well, if you want to go down that path...

[dtor@hammer work]$ grep -r -e "\(setted\|setuped\|split\+ed\)" . | wc -l
54
[dtor@hammer work]$ 

-- 
Dmitry

Re: your mail

[Henrik Rydberg]

Hi Maurus,

> Subject: [RFC] HID: add Microsoft Touch Mouse driver
> 
> This patch adds support for the proprietary Microsoft Touch Mouse
> multitouch protocol.

Exciting stuff, nice job on the patch too. Please find some initial
comments inline.

> @@ -0,0 +1,371 @@
> +/*
> + *  HID driver for the Microsoft Touch Mouse
> + *
> + *  Copyright (c) 2011 Maurus Cuelenaere <mcuelenaere@gmail.com>
> + *
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +
> + *
> + * 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
> + *
> + */
> +
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/usb.h>
> +
> +#include "hid-ids.h"
> +
> +#define MSTM_RAW_DATA_HID_ID		0x27
> +#define MSTM_RAW_DATA_FOOTER		0x51
> +
> +#define MSTM_DATA_WIDTH			15
> +#define MSTM_DATA_HEIGHT		13
> +
> +struct mstm_raw_data_packet {
> +	__u8 hid_id;
> +	__u8 data_length;
> +	__u16 unk1;
> +	__u8 unk2;
> +	__u8 footer;
> +	__u8 timestamp;
> +	__u8 data[25]; /* milliseconds */
> +};

I take it this means you get at most 50 nibbles per transfer?

> +
> +struct mstm_state {
> +	bool advance_flag;
> +	int x;
> +	int y;
> +	unsigned char last_timestamp;
> +	unsigned char data[MSTM_DATA_WIDTH][MSTM_DATA_HEIGHT];
> +};

The ability to simply send an "input screen" would be perfect
here. This device may be on the border of what can/should be handled
via input events. A memory mapped driver, uio-based or something
similar, could be an option.

> +
> +struct mstm_device {
> +	struct input_dev *input;
> +
> +	struct mstm_state state;
> +};
> +
> +#define MSTM_INTERFACE_KEYBOARD		0
> +#define MSTM_INTERFACE_MOUSE		1
> +#define MSTM_INTERFACE_CONTROL		2
> +
> +static inline int hid_get_interface_number(struct hid_device *hdev) {
> +	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> +	return intf->cur_altsetting->desc.bInterfaceNumber;
> +}
> +
> +/*
> + * The mouse sensor only yields data for a specific part of its surface.
> + * Because of this, we can't just increase x and y uniformly; so there's
> + * a need for this simple algorithm.
> + *
> + * Visual representation of available data:
> + *    0 1 2 3 4 5 6 7 8 9 A B C D E F
> + *  0       * * * * * * * * * *
> + *  1     * * * * * * * * * * * *
> + *  2   * * * * * * * * * * * * * *
> + *  3   * * * * * * * * * * * * * *
> + *  4 * * * * * * * * * * * * * * * *
> + *  5 * * * * * * * * * * * * * * * *
> + *  6 * * * * * * * * * * * * * * * *
> + *  7 * * * * * * * * * * * * * * * *
> + *  8 * * * * * * * * * * * * * * * *
> + *  9 * * * * * * * * * * * * * * * *
> + *  A * * * * * * * * * * * * * * * *
> + *  B * * * * * * * * * * * * * * * *
> + *  C * * * * * * * * * * * * * * * *
> + *  D * * * * * * * * * * * * * * * *
> + */
> +static void mstm_advance_pointer(struct mstm_state *state)
> +{
> +	int max, nextMin;
> +
> +	state->x++;
> +
> +	switch (state->y) {
> +	case 0:
> +		max = 11;
> +		nextMin = 2;
> +		break;
> +	case 1:
> +		max = 12;
> +		nextMin = 1;
> +		break;
> +	case 2:
> +		max = 13;
> +		nextMin = 1;
> +		break;
> +	case 3:
> +		max = 13;
> +		nextMin = 0;
> +		break;
> +	default:
> +		max = 14;
> +		nextMin = 0;
> +		break;
> +	}
> +
> +	if (state->x > max) {
> +		state->y++;
> +		state->x = nextMin;
> +	}
> +}
> +
> +static void mstm_parse_nibble(struct mstm_state *state, __u8 nibble)
> +{
> +	int i;
> +
> +	if (state->advance_flag) {
> +		state->advance_flag = false;
> +		for (i = -3; i < nibble; i++)
> +			mstm_advance_pointer(state);
> +	} else {
> +		if (nibble == 0xF) {
> +			/* The next nibble will indicate how many
> +			* positions to advance, so set a flag */
> +			state->advance_flag = true;
> +		} else {
> +			state->data[state->x][state->y] = nibble;
> +			mstm_advance_pointer(state);
> +		}
> +	}
> +}

Looking good.

> +static void mstm_push_data(struct hid_device *hdev)
> +{
> +	struct mstm_device *mstm_dev = hid_get_drvdata(hdev);
> +	struct input_dev *input = mstm_dev->input;
> +	int x, y;
> +
> +	for (x = 0; x < MSTM_DATA_WIDTH; x++) {
> +		for (y = 0; y < MSTM_DATA_HEIGHT; y++) {
> +			unsigned char pressure = mstm_dev->state.data[x][y];
> +			if (pressure > 0) {
> +				//input_report_abs(input, ABS_MT_BLOB_ID, 1);
> +				//input_report_abs(input, ABS_MT_TOUCH_MAJOR, pressure/3);
> +				input_report_abs(input, ABS_MT_POSITION_X, x);
> +				input_report_abs(input, ABS_MT_POSITION_Y, y);
> +				input_mt_sync(input);
> +			}
> +		}
> +	}

True, the blob id has not yet been used, but its definition is
different from how it is used here. Also, since you do not attempt any
kind of clustering (single-linkage or similar), the blob as stated
might not even be connected. One possible option could be to use the
slots, but only send ABS_MT_TOUCH_MAJOR or ABS_MT_PRESSURE, nothing
else. The device would (rightfully) not be recognized as MT since the
position is missing, all data would be available for processing in
userspace, and bandwidth would be minimized since there could only be
so many changes coming in per millisecond.

> +static int mstm_input_mapping(struct hid_device *hdev,
> +		struct hid_input *hi, struct hid_field *field,
> +		struct hid_usage *usage, unsigned long **bit, int *max)
> +{
> +	struct mstm_device *mstm_dev = hid_get_drvdata(hdev);
> +
> +	//printk("mstm_input_mapping(%p)\n", hdev);
> +
> +	/* bail early if not the correct interface */
> +	if (mstm_dev == NULL)
> +		return 0;
> +
> +	/* HACK: get rid of ABS_MT_* params */
> +	if ((usage->hid & HID_USAGE_PAGE) == HID_UP_GENDESK)
> +		return -1;

I am not sure about the hack here, nor its explanation. ;-)

> +
> +	/* map input device to hid device */
> +	mstm_dev->input = hi->input;
> +
> +	return 0;
> +}
> +
> +static void mstm_setup_input(struct mstm_device *mstm)
> +{
> +	__set_bit(EV_ABS, mstm->input->evbit);
> +
> +	input_set_abs_params(mstm->input, ABS_MT_POSITION_X, 0, MSTM_DATA_WIDTH, 0, 0);
> +	input_set_abs_params(mstm->input, ABS_MT_POSITION_Y, 0, MSTM_DATA_HEIGHT, 0, 0);
> +	input_set_abs_params(mstm->input, ABS_MT_TOUCH_MAJOR, 0, 0xF, 0, 0);
> +	input_set_abs_params(mstm->input, ABS_MT_BLOB_ID, 0, 10, 0, 0);
> +
> +	input_abs_set_res(mstm->input, ABS_MT_POSITION_X, 6); /* 83mm */
> +	input_abs_set_res(mstm->input, ABS_MT_POSITION_Y, 5); /* 70mm */
> +
> +	input_set_events_per_packet(mstm->input, 60);
> +}

Regarding the resolution of touch major, it is generally assumed (in
userspace) that the units are the same as the position, so scaling in
the driver is reasonable. Otherwise, why not specify resolution for
touch major as well?

Thanks,
Henrik

Re: your mail

[Maurus Cuelenaere]

Hi Henrik,

Op 25-01-12 14:26, Henrik Rydberg schreef:
> Hi Maurus,
>
>> Subject: [RFC] HID: add Microsoft Touch Mouse driver
>>
>> This patch adds support for the proprietary Microsoft Touch Mouse
>> multitouch protocol.
> Exciting stuff, nice job on the patch too. Please find some initial
> comments inline.
>
>> @@ -0,0 +1,371 @@
>> +/*
>> + *  HID driver for the Microsoft Touch Mouse
>> + *
>> + *  Copyright (c) 2011 Maurus Cuelenaere<mcuelenaere@gmail.com>
>> + *
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> +
>> + *
>> + * 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
>> + *
>> + */
>> +
>> +#include<linux/device.h>
>> +#include<linux/input.h>
>> +#include<linux/hid.h>
>> +#include<linux/module.h>
>> +#include<linux/usb.h>
>> +
>> +#include "hid-ids.h"
>> +
>> +#define MSTM_RAW_DATA_HID_ID		0x27
>> +#define MSTM_RAW_DATA_FOOTER		0x51
>> +
>> +#define MSTM_DATA_WIDTH			15
>> +#define MSTM_DATA_HEIGHT		13
>> +
>> +struct mstm_raw_data_packet {
>> +	__u8 hid_id;
>> +	__u8 data_length;
>> +	__u16 unk1;
>> +	__u8 unk2;
>> +	__u8 footer;
>> +	__u8 timestamp;
>> +	__u8 data[25]; /* milliseconds */
>> +};
> I take it this means you get at most 50 nibbles per transfer?

Correct.

Hmm, looks like that milliseconds comment needs to be on the line above it.

>> +
>> +struct mstm_state {
>> +	bool advance_flag;
>> +	int x;
>> +	int y;
>> +	unsigned char last_timestamp;
>> +	unsigned char data[MSTM_DATA_WIDTH][MSTM_DATA_HEIGHT];
>> +};
> The ability to simply send an "input screen" would be perfect
> here. This device may be on the border of what can/should be handled
> via input events. A memory mapped driver, uio-based or something
> similar, could be an option.

In my first tests, I was doing readouts in userspace using hidraw, which 
performed quite well.

Bandwidth could be an issue, but I'd like to use the current APIs as 
much as possible so I don't need to go modifying mtdev, X, ...

>> +
>> +struct mstm_device {
>> +	struct input_dev *input;
>> +
>> +	struct mstm_state state;
>> +};
>> +
>> +#define MSTM_INTERFACE_KEYBOARD		0
>> +#define MSTM_INTERFACE_MOUSE		1
>> +#define MSTM_INTERFACE_CONTROL		2
>> +
>> +static inline int hid_get_interface_number(struct hid_device *hdev) {
>> +	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
>> +	return intf->cur_altsetting->desc.bInterfaceNumber;
>> +}
>> +
>> +/*
>> + * The mouse sensor only yields data for a specific part of its surface.
>> + * Because of this, we can't just increase x and y uniformly; so there's
>> + * a need for this simple algorithm.
>> + *
>> + * Visual representation of available data:
>> + *    0 1 2 3 4 5 6 7 8 9 A B C D E F
>> + *  0       * * * * * * * * * *
>> + *  1     * * * * * * * * * * * *
>> + *  2   * * * * * * * * * * * * * *
>> + *  3   * * * * * * * * * * * * * *
>> + *  4 * * * * * * * * * * * * * * * *
>> + *  5 * * * * * * * * * * * * * * * *
>> + *  6 * * * * * * * * * * * * * * * *
>> + *  7 * * * * * * * * * * * * * * * *
>> + *  8 * * * * * * * * * * * * * * * *
>> + *  9 * * * * * * * * * * * * * * * *
>> + *  A * * * * * * * * * * * * * * * *
>> + *  B * * * * * * * * * * * * * * * *
>> + *  C * * * * * * * * * * * * * * * *
>> + *  D * * * * * * * * * * * * * * * *
>> + */
>> +static void mstm_advance_pointer(struct mstm_state *state)
>> +{
>> +	int max, nextMin;
>> +
>> +	state->x++;
>> +
>> +	switch (state->y) {
>> +	case 0:
>> +		max = 11;
>> +		nextMin = 2;
>> +		break;
>> +	case 1:
>> +		max = 12;
>> +		nextMin = 1;
>> +		break;
>> +	case 2:
>> +		max = 13;
>> +		nextMin = 1;
>> +		break;
>> +	case 3:
>> +		max = 13;
>> +		nextMin = 0;
>> +		break;
>> +	default:
>> +		max = 14;
>> +		nextMin = 0;
>> +		break;
>> +	}
>> +
>> +	if (state->x>  max) {
>> +		state->y++;
>> +		state->x = nextMin;
>> +	}
>> +}
>> +
>> +static void mstm_parse_nibble(struct mstm_state *state, __u8 nibble)
>> +{
>> +	int i;
>> +
>> +	if (state->advance_flag) {
>> +		state->advance_flag = false;
>> +		for (i = -3; i<  nibble; i++)
>> +			mstm_advance_pointer(state);
>> +	} else {
>> +		if (nibble == 0xF) {
>> +			/* The next nibble will indicate how many
>> +			* positions to advance, so set a flag */
>> +			state->advance_flag = true;
>> +		} else {
>> +			state->data[state->x][state->y] = nibble;
>> +			mstm_advance_pointer(state);
>> +		}
>> +	}
>> +}
> Looking good.
>
>> +static void mstm_push_data(struct hid_device *hdev)
>> +{
>> +	struct mstm_device *mstm_dev = hid_get_drvdata(hdev);
>> +	struct input_dev *input = mstm_dev->input;
>> +	int x, y;
>> +
>> +	for (x = 0; x<  MSTM_DATA_WIDTH; x++) {
>> +		for (y = 0; y<  MSTM_DATA_HEIGHT; y++) {
>> +			unsigned char pressure = mstm_dev->state.data[x][y];
>> +			if (pressure>  0) {
>> +				//input_report_abs(input, ABS_MT_BLOB_ID, 1);
>> +				//input_report_abs(input, ABS_MT_TOUCH_MAJOR, pressure/3);
>> +				input_report_abs(input, ABS_MT_POSITION_X, x);
>> +				input_report_abs(input, ABS_MT_POSITION_Y, y);
>> +				input_mt_sync(input);
>> +			}
>> +		}
>> +	}
> True, the blob id has not yet been used, but its definition is
> different from how it is used here. Also, since you do not attempt any
> kind of clustering (single-linkage or similar), the blob as stated
> might not even be connected.

Indeed, without clustering the BLOB_ID would be useless but that line 
was only for testing with one finger.

> One possible option could be to use the
> slots, but only send ABS_MT_TOUCH_MAJOR or ABS_MT_PRESSURE, nothing
> else. The device would (rightfully) not be recognized as MT since the
> position is missing, all data would be available for processing in
> userspace, and bandwidth would be minimized since there could only be
> so many changes coming in per millisecond.

So how does userspace then finds out where these pressure points are 
located?
Or do you mean to just dump all data to user space (15 * 13 * 
sizeof(ABS_MT_PRESSURE value) + overhead)?

>> +static int mstm_input_mapping(struct hid_device *hdev,
>> +		struct hid_input *hi, struct hid_field *field,
>> +		struct hid_usage *usage, unsigned long **bit, int *max)
>> +{
>> +	struct mstm_device *mstm_dev = hid_get_drvdata(hdev);
>> +
>> +	//printk("mstm_input_mapping(%p)\n", hdev);
>> +
>> +	/* bail early if not the correct interface */
>> +	if (mstm_dev == NULL)
>> +		return 0;
>> +
>> +	/* HACK: get rid of ABS_MT_* params */
>> +	if ((usage->hid&  HID_USAGE_PAGE) == HID_UP_GENDESK)
>> +		return -1;
> I am not sure about the hack here, nor its explanation. ;-)

The HID tables specify some ABS_MT_* values and I didn't know whether 
these were going to conflict with the ones I report above, so I just 
discard all GenericDesktop fields.

root@hp4530s:~# grep MT /sys/kernel/debug/hid/0003\:045E\:0773.0006/rdesc
GenericDesktop.MultiAxis ---> Absolute.MTMajor
GenericDesktop.0009 ---> Absolute.MTMinor
GenericDesktop.000a ---> Absolute.MTMajorW
GenericDesktop.000b ---> Absolute.MTMinorW
GenericDesktop.000c ---> Absolute.MTOrientation
GenericDesktop.000d ---> Absolute.MTPositionX
GenericDesktop.000e ---> Absolute.MTPositionY
GenericDesktop.000f ---> Absolute.MTToolType
GenericDesktop.0010 ---> Absolute.MTBlobID

>> +
>> +	/* map input device to hid device */
>> +	mstm_dev->input = hi->input;
>> +
>> +	return 0;
>> +}
>> +
>> +static void mstm_setup_input(struct mstm_device *mstm)
>> +{
>> +	__set_bit(EV_ABS, mstm->input->evbit);
>> +
>> +	input_set_abs_params(mstm->input, ABS_MT_POSITION_X, 0, MSTM_DATA_WIDTH, 0, 0);
>> +	input_set_abs_params(mstm->input, ABS_MT_POSITION_Y, 0, MSTM_DATA_HEIGHT, 0, 0);
>> +	input_set_abs_params(mstm->input, ABS_MT_TOUCH_MAJOR, 0, 0xF, 0, 0);
>> +	input_set_abs_params(mstm->input, ABS_MT_BLOB_ID, 0, 10, 0, 0);
>> +
>> +	input_abs_set_res(mstm->input, ABS_MT_POSITION_X, 6); /* 83mm */
>> +	input_abs_set_res(mstm->input, ABS_MT_POSITION_Y, 5); /* 70mm */
>> +
>> +	input_set_events_per_packet(mstm->input, 60);
>> +}
> Regarding the resolution of touch major, it is generally assumed (in
> userspace) that the units are the same as the position, so scaling in
> the driver is reasonable. Otherwise, why not specify resolution for
> touch major as well?

I didn't specify it because other HID drivers only specified resolutions 
for POSITION_{X,Y}, I'll try it and see how mtview reacts.

Thanks for the review!

-- 
Maurus Cuelenaere