Re: [PATCH] Touchscreen driver for FT5x06 based EDT displays

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Simon,

On Wed, Apr 04, 2012 at 08:27:59PM +0200, simon.budig@kernelconcepts.de wrote:
> From: Simon Budig <simon.budig@kernelconcepts.de>
> 
> This is a driver for the EDT "Polytouch" family of touch controllers
> based on the FocalTech FT5x06 line of chips.
> 
> Signed-off-by: Simon Budig <simon.budig@kernelconcepts.de>
> ---
>  drivers/input/touchscreen/Kconfig      |   13 +
>  drivers/input/touchscreen/Makefile     |    1 +
>  drivers/input/touchscreen/edt-ft5x06.c |  771 ++++++++++++++++++++++++++++++++
>  include/linux/input/edt-ft5x06.h       |   17 +
>  4 files changed, 802 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/input/touchscreen/edt-ft5x06.c
>  create mode 100644 include/linux/input/edt-ft5x06.h
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 2a21419..0166a15 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -431,6 +431,19 @@ config TOUCHSCREEN_PENMOUNT
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called penmount.
>  
> +config TOUCHSCREEN_EDT_FT5X06
> +	tristate "EDT FocalTech FT5x06 I2C Touchscreen support"
> +	depends on I2C
> +	help
> +	  Say Y here if you have an EDT "Polytouch" touchscreen based
> +	  on the FocalTech FT5x06 family of controllers connected to
> +	  your system.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called edt-ft5x06.
> +
>  config TOUCHSCREEN_MIGOR
>  	tristate "Renesas MIGO-R touchscreen"
>  	depends on SH_MIGOR && I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 3d5cf8c..8a68c84 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_TOUCHSCREEN_CYTTSP_I2C)	+= cyttsp_i2c.o
>  obj-$(CONFIG_TOUCHSCREEN_CYTTSP_SPI)	+= cyttsp_spi.o
>  obj-$(CONFIG_TOUCHSCREEN_DA9034)	+= da9034-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_DYNAPRO)	+= dynapro.o
> +obj-$(CONFIG_TOUCHSCREEN_EDT_FT5X06)	+= edt-ft5x06.o
>  obj-$(CONFIG_TOUCHSCREEN_HAMPSHIRE)	+= hampshire.o
>  obj-$(CONFIG_TOUCHSCREEN_GUNZE)		+= gunze.o
>  obj-$(CONFIG_TOUCHSCREEN_EETI)		+= eeti_ts.o
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> new file mode 100644
> index 0000000..7c4f9f5
> --- /dev/null
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -0,0 +1,771 @@
> +/*
> + * Copyright (C) 2011 Simon Budig, <simon.budig@kernelconcepts.de>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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 library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +/*
> + * This is a driver for the EDT "Polytouch" family of touch controllers
> + * based on the FocalTech FT5x06 line of chips.
> + *
> + * Development of this driver has been sponsored by Glyn:
> + *    http://www.glyn.com/Products/Displays
> + */
> +
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/input.h>
> +#include <linux/i2c.h>
> +#include <linux/uaccess.h>
> +#include <linux/delay.h>
> +#include <linux/slab.h>
> +
> +#include <linux/gpio.h>
> +
> +#include <linux/input/edt-ft5x06.h>
> +
> +#define DRIVER_VERSION "v0.5"
> +
> +#define WORK_REGISTER_THRESHOLD   0x00
> +#define WORK_REGISTER_REPORT_RATE 0x08
> +#define WORK_REGISTER_GAIN        0x30
> +#define WORK_REGISTER_OFFSET      0x31
> +#define WORK_REGISTER_NUM_X       0x33
> +#define WORK_REGISTER_NUM_Y       0x34
> +
> +#define WORK_REGISTER_OPMODE      0x3c
> +#define FACTORY_REGISTER_OPMODE   0x01
> +
> +struct edt_ft5x06_i2c_ts_data {
> +	struct i2c_client *client;
> +	struct input_dev *input;
> +	int irq;
> +	int irq_pin;
> +	int reset_pin;
> +	int num_x;
> +	int num_y;
> +
> +	struct mutex mutex;
> +	bool factory_mode;
> +	int threshold;
> +	int gain;
> +	int offset;
> +	int report_rate;
> +};
> +
> +static int edt_ft5x06_ts_readwrite(struct i2c_client *client,
> +                                   u16 wr_len, u8 *wr_buf,
> +                                   u16 rd_len, u8 *rd_buf)
> +{
> +	struct i2c_msg wrmsg[2];
> +	int i, ret;
> +
> +	i = 0;
> +	if (wr_len) {
> +		wrmsg[i].addr  = client->addr;
> +		wrmsg[i].flags = 0;
> +		wrmsg[i].len = wr_len;
> +		wrmsg[i].buf = wr_buf;
> +		i++;
> +	}
> +	if (rd_len) {
> +		wrmsg[i].addr  = client->addr;
> +		wrmsg[i].flags = I2C_M_RD;
> +		wrmsg[i].len = rd_len;
> +		wrmsg[i].buf = rd_buf;
> +		i++;
> +	}
> +
> +	ret = i2c_transfer(client->adapter, wrmsg, i);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "i2c_transfer failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +
> +static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
> +{
> +	struct edt_ft5x06_i2c_ts_data *tsdata = dev_id;
> +	unsigned char touching = 0;
> +	unsigned char rdbuf[26], wrbuf[1];
> +	int i, have_abs, type, ret;
> +
> +	memset(wrbuf, 0, sizeof(wrbuf));
> +	memset(rdbuf, 0, sizeof(rdbuf));
> +
> +	wrbuf[0] = 0xf9;
> +
> +	mutex_lock(&tsdata->mutex);
> +	ret = edt_ft5x06_ts_readwrite(tsdata->client,
> +	                              1, wrbuf,
> +	                              sizeof(rdbuf), rdbuf);
> +	mutex_unlock(&tsdata->mutex);
> +	if (ret < 0) {
> +		dev_err(&tsdata->client->dev,
> +		        "Unable to write to i2c touchscreen!\n");
> +		goto out;
> +	}
> +
> +	if (rdbuf[0] != 0xaa || rdbuf[1] != 0xaa || rdbuf[2] != 26) {
> +		dev_err(&tsdata->client->dev,
> +		        "Unexpected header: %02x%02x%02x!\n",
> +		        rdbuf[0], rdbuf[1], rdbuf[2]);
> +	}
> +
> +	have_abs = 0;
> +	touching = rdbuf[3];
> +	for (i = 0; i < touching; i++) {
> +		type = rdbuf[i*4+5] >> 6;
> +		/* ignore Touch Down and Reserved events */
> +		if (type == 0x01 || type == 0x03)
> +			continue;
> +
> +		if (!have_abs) {
> +			input_report_key(tsdata->input, BTN_TOUCH,    1);
> +			input_report_abs(tsdata->input, ABS_PRESSURE, 1);
> +			input_report_abs(tsdata->input, ABS_X,
> +			                 ((rdbuf[i*4+5] << 8) |
> +			                  rdbuf[i*4+6]) & 0x0fff);
> +			input_report_abs(tsdata->input, ABS_Y,
> +			                 ((rdbuf[i*4+7] << 8) |
> +			                  rdbuf[i*4+8]) & 0x0fff);
> +			have_abs = 1;


The mt pointer emulation should do this for you.

> +		}
> +		input_report_abs(tsdata->input, ABS_MT_POSITION_X,
> +		                 ((rdbuf[i*4+5] << 8) | rdbuf[i*4+6]) & 0x0fff);
> +		input_report_abs(tsdata->input, ABS_MT_POSITION_Y,
> +		                 ((rdbuf[i*4+7] << 8) | rdbuf[i*4+8]) & 0x0fff);
> +		input_report_abs(tsdata->input, ABS_MT_TRACKING_ID,
> +		                 (rdbuf[i*4+7] >> 4) & 0x0f);
> +		input_mt_sync(tsdata->input);
> +	}
> +	if (!have_abs) {
> +		input_report_key(tsdata->input, BTN_TOUCH,    0);
> +		input_report_abs(tsdata->input, ABS_PRESSURE, 0);
> +	}
> +	input_sync(tsdata->input);
> +
> +out:
> +	return IRQ_HANDLED;
> +}
> +
> +
> +static int edt_ft5x06_i2c_register_write(struct edt_ft5x06_i2c_ts_data *tsdata,
> +                                         u8 addr, u8 value)
> +{
> +	u8 wrbuf[4];
> +	int ret;
> +
> +	wrbuf[0]  = tsdata->factory_mode ? 0xf3 : 0xfc;
> +	wrbuf[1]  = tsdata->factory_mode ? addr & 0x7f : addr & 0x3f;
> +	wrbuf[2]  = value;
> +	wrbuf[3]  = wrbuf[0] ^ wrbuf[1] ^ wrbuf[2];
> +
> +	disable_irq(tsdata->irq);
> +
> +	ret = edt_ft5x06_ts_readwrite(tsdata->client,
> +	                              4, wrbuf,
> +	                              0, NULL);
> +
> +	enable_irq(tsdata->irq);
> +
> +	return ret;
> +}
> +
> +static int edt_ft5x06_i2c_register_read(struct edt_ft5x06_i2c_ts_data *tsdata,
> +                                        u8 addr)
> +{
> +	u8 wrbuf[2], rdbuf[2];
> +	int ret;
> +
> +	wrbuf[0]  = tsdata->factory_mode ? 0xf3 : 0xfc;
> +	wrbuf[1]  = tsdata->factory_mode ? addr & 0x7f : addr & 0x3f;
> +	wrbuf[1] |= tsdata->factory_mode ? 0x80 : 0x40;
> +
> +	disable_irq(tsdata->irq);
> +
> +	ret = edt_ft5x06_ts_readwrite(tsdata->client,
> +	                              2, wrbuf,
> +	                              2, rdbuf);
> +
> +	enable_irq(tsdata->irq);
> +
> +	if ((wrbuf[0] ^ wrbuf[1] ^ rdbuf[0]) != rdbuf[1])
> +		dev_err(&tsdata->client->dev,
> +		        "crc error: 0x%02x expected, got 0x%02x\n",
> +		        (wrbuf[0] ^ wrbuf[1] ^ rdbuf[0]), rdbuf[1]);
> +
> +	return ret < 0 ? ret : rdbuf[0];
> +}
> +
> +static ssize_t edt_ft5x06_i2c_setting_show(struct device *dev,
> +                                           struct device_attribute *attr,
> +                                           char *buf)
> +{
> +	struct edt_ft5x06_i2c_ts_data *tsdata = dev_get_drvdata(dev);
> +	struct i2c_client *client = tsdata->client;
> +	int ret = 0;
> +	int *value;
> +	u8 addr;
> +
> +	switch (attr->attr.name[0]) {
> +	case 't':    /* threshold */
> +		addr = WORK_REGISTER_THRESHOLD;
> +		value = &tsdata->threshold;
> +		break;
> +	case 'g':    /* gain */
> +		addr = WORK_REGISTER_GAIN;
> +		value = &tsdata->gain;
> +		break;
> +	case 'o':    /* offset */
> +		addr = WORK_REGISTER_OFFSET;
> +		value = &tsdata->offset;
> +		break;
> +	case 'r':    /* report rate */
> +		addr = WORK_REGISTER_REPORT_RATE;
> +		value = &tsdata->report_rate;
> +		break;
> +	default:
> +		dev_err(&client->dev,
> +		        "unknown attribute for edt_ft5x06_i2c_setting_show: %s\n",
> +		        attr->attr.name);
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&tsdata->mutex);
> +
> +	if (tsdata->factory_mode) {
> +		dev_err(dev,
> +		        "setting register not available in factory mode\n");
> +		mutex_unlock(&tsdata->mutex);
> +		return -EIO;
> +	}
> +
> +	ret = edt_ft5x06_i2c_register_read(tsdata, addr);
> +	if (ret < 0) {
> +		dev_err(&tsdata->client->dev,
> +		        "Unable to write to i2c touchscreen!\n");
> +		mutex_unlock(&tsdata->mutex);
> +		return ret;
> +	}
> +	mutex_unlock(&tsdata->mutex);
> +
> +	if (ret != *value) {
> +		dev_info(&tsdata->client->dev,
> +		         "i2c read (%d) and stored value (%d) differ. Huh?\n",
> +		         ret, *value);
> +		*value = ret;
> +	}
> +
> +	return sprintf(buf, "%d\n", ret);
> +}
> +
> +static ssize_t edt_ft5x06_i2c_setting_store(struct device *dev,
> +                                            struct device_attribute *attr,
> +                                            const char *buf, size_t count)
> +{
> +	struct edt_ft5x06_i2c_ts_data *tsdata = dev_get_drvdata(dev);
> +	struct i2c_client *client = tsdata->client;
> +	int ret = 0;
> +	u8 addr;
> +	unsigned int val;
> +
> +	mutex_lock(&tsdata->mutex);
> +
> +	if (tsdata->factory_mode) {
> +		dev_err(dev,
> +		        "setting register not available in factory mode\n");

This will spam logs, just return error silently.

> +		ret = -EIO;
> +		goto out;
> +	}
> +
> +	if (sscanf(buf, "%u", &val) != 1) {
> +		dev_err(dev, "Invalid value for attribute %s\n",
> +		        attr->attr.name);

As will this.

> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	switch (attr->attr.name[0]) {
> +	case 't':    /* threshold */
> +		addr = WORK_REGISTER_THRESHOLD;
> +		val = val < 20 ? 20 : val > 80 ? 80 : val;
> +		tsdata->threshold = val;
> +		break;
> +	case 'g':    /* gain */
> +		addr = WORK_REGISTER_GAIN;
> +		val = val < 0 ? 0 : val > 31 ? 31 : val;
> +		tsdata->gain = val;
> +		break;
> +	case 'o':    /* offset */
> +		addr = WORK_REGISTER_OFFSET;
> +		val = val < 0 ? 0 : val > 31 ? 31 : val;
> +		tsdata->offset = val;
> +		break;
> +	case 'r':    /* report rate */
> +		addr = WORK_REGISTER_REPORT_RATE;
> +		val = val < 3 ? 3 : val > 14 ? 14 : val;
> +		tsdata->report_rate = val;
> +		break;

See if you could wrap an attribute into your own structure that will
allow you to get to the address, field and limits without matching on
attribute name.

> +	default:
> +		dev_err(&client->dev,
> +		        "unknown attribute for edt_ft5x06_i2c_setting_show: %s\n",
> +		        attr->attr.name);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	ret = edt_ft5x06_i2c_register_write(tsdata, addr, val);
> +
> +	if (ret < 0) {
> +		dev_err(&tsdata->client->dev,
> +		        "Unable to write to i2c touchscreen!\n");
> +		goto out;
> +	}
> +
> +out:
> +	mutex_unlock(&tsdata->mutex);
> +	return ret < 0 ? ret : count;
> +}
> +
> +
> +static ssize_t edt_ft5x06_i2c_mode_show(struct device *dev,
> +                                        struct device_attribute *attr,
> +                                        char *buf)
> +{
> +	struct edt_ft5x06_i2c_ts_data *tsdata = dev_get_drvdata(dev);
> +	return sprintf(buf, "%d\n", tsdata->factory_mode ? 1 : 0);
> +}
> +
> +static ssize_t edt_ft5x06_i2c_mode_store(struct device *dev,
> +                                         struct device_attribute *attr,
> +                                         const char *buf, size_t count)
> +{
> +	struct edt_ft5x06_i2c_ts_data *tsdata = dev_get_drvdata(dev);
> +	int i, ret = 0;
> +	unsigned int mode;
> +
> +	if (sscanf(buf, "%u", &mode) != 1 || (mode | 1) != 1) {
> +		dev_err(dev, "Invalid value for operation mode\n");
> +		return -EINVAL;
> +	}
> +
> +	/* no change, return without doing anything */
> +	if (mode == tsdata->factory_mode)
> +		return count;
> +
> +	mutex_lock(&tsdata->mutex);
> +	if (!tsdata->factory_mode) { /* switch to factory mode */
> +		disable_irq(tsdata->irq);
> +		/* mode register is 0x3c when in the work mode */
> +		ret = edt_ft5x06_i2c_register_write(tsdata,
> +		                                    WORK_REGISTER_OPMODE, 0x03);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to switch to factory mode (%d)\n",
> +			        ret);
> +		} else {
> +			tsdata->factory_mode = 1;

= true;

> +			for (i = 0; i < 10; i++) {
> +				mdelay(5);
> +				/* mode register is 0x01 when in factory mode */
> +				ret = edt_ft5x06_i2c_register_read(tsdata, FACTORY_REGISTER_OPMODE);
> +				if (ret == 0x03)
> +					break;
> +			}
> +			if (i == 10)
> +				dev_err(dev,
> +				        "not in factory mode after %dms.\n",
> +				        i*5);
> +		}
> +	} else {  /* switch to work mode */
> +		/* mode register is 0x01 when in the factory mode */
> +		ret = edt_ft5x06_i2c_register_write(tsdata,
> +		                                    FACTORY_REGISTER_OPMODE,
> +		                                    0x01);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to switch to work mode (%d)\n",
> +			        ret);
> +		} else {
> +			tsdata->factory_mode = 0;

= false;

> +			for (i = 0; i < 10; i++) {
> +				mdelay(5);
> +				/* mode register is 0x01 when in factory mode */
> +				ret = edt_ft5x06_i2c_register_read(tsdata, WORK_REGISTER_OPMODE);
> +				if (ret == 0x01)
> +					break;
> +			}
> +			if (i == 10)
> +				dev_err(dev, "not in work mode after %dms.\n",
> +				        i*5);
> +
> +			/* restore parameters */
> +			edt_ft5x06_i2c_register_write(tsdata,
> +			                              WORK_REGISTER_THRESHOLD,
> +			                              tsdata->threshold);
> +			edt_ft5x06_i2c_register_write(tsdata,
> +			                              WORK_REGISTER_GAIN,
> +			                              tsdata->gain);
> +			edt_ft5x06_i2c_register_write(tsdata,
> +			                              WORK_REGISTER_OFFSET,
> +			                              tsdata->offset);
> +			edt_ft5x06_i2c_register_write(tsdata,
> +			                              WORK_REGISTER_REPORT_RATE,
> +			                              tsdata->report_rate);
> +
> +			enable_irq(tsdata->irq);
> +		}
> +	}
> +
> +	mutex_unlock(&tsdata->mutex);
> +	return count;
> +}
> +
> +
> +static ssize_t edt_ft5x06_i2c_raw_data_show(struct device *dev,
> +                                            struct device_attribute *attr,
> +                                            char *buf)
> +{
> +	struct edt_ft5x06_i2c_ts_data *tsdata = dev_get_drvdata(dev);
> +	int i, ret;
> +	char *ptr, wrbuf[3];
> +
> +	if (!tsdata->factory_mode) {
> +		dev_err(dev, "raw data not available in work mode\n");
> +		return -EIO;
> +	}
> +
> +	mutex_lock(&tsdata->mutex);

Instead of taking mutex why don't you disable IRQ?

> +	ret = edt_ft5x06_i2c_register_write(tsdata, 0x08, 0x01);
> +	for (i = 0; i < 100; i++) {
> +		ret = edt_ft5x06_i2c_register_read(tsdata, 0x08);
> +		if (ret < 1)
> +			break;
> +		udelay(1000);
> +	}
> +
> +	if (i == 100 || ret < 0) {
> +		dev_err(dev, "waiting time exceeded or error: %d\n", ret);
> +		mutex_unlock(&tsdata->mutex);
> +		return ret < 0 ? ret : -ETIMEDOUT;
> +	}
> +
> +	ptr = buf;
> +	wrbuf[0] = 0xf5;
> +	wrbuf[1] = 0x0e;
> +	for (i = 0; i <= tsdata->num_x; i++) {
> +		wrbuf[2] = i;
> +		ret = edt_ft5x06_ts_readwrite(tsdata->client,
> +		                               3, wrbuf,
> +		                               tsdata->num_y * 2, ptr);
> +		if (ret < 0) {
> +			mutex_unlock(&tsdata->mutex);
> +			return ret;
> +		}
> +
> +		ptr += tsdata->num_y * 2;
> +	}
> +
> +	mutex_unlock(&tsdata->mutex);
> +	return ptr - buf;
> +}
> +
> +
> +static DEVICE_ATTR(gain,      0664,
> +                   edt_ft5x06_i2c_setting_show, edt_ft5x06_i2c_setting_store);
> +static DEVICE_ATTR(offset,    0664,
> +                   edt_ft5x06_i2c_setting_show, edt_ft5x06_i2c_setting_store);
> +static DEVICE_ATTR(threshold, 0664,
> +                   edt_ft5x06_i2c_setting_show, edt_ft5x06_i2c_setting_store);
> +static DEVICE_ATTR(report_rate, 0664,
> +                   edt_ft5x06_i2c_setting_show, edt_ft5x06_i2c_setting_store);
> +static DEVICE_ATTR(mode,      0664,
> +                   edt_ft5x06_i2c_mode_show, edt_ft5x06_i2c_mode_store);
> +static DEVICE_ATTR(raw_data,  0444,
> +                   edt_ft5x06_i2c_raw_data_show, NULL);
> +
> +static struct attribute *edt_ft5x06_i2c_attrs[] = {
> +	&dev_attr_gain.attr,
> +	&dev_attr_offset.attr,
> +	&dev_attr_threshold.attr,
> +	&dev_attr_report_rate.attr,
> +	&dev_attr_mode.attr,
> +	&dev_attr_raw_data.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group edt_ft5x06_i2c_attr_group = {
> +	.attrs = edt_ft5x06_i2c_attrs,
> +};
> +
> +static int edt_ft5x06_i2c_ts_probe(struct i2c_client *client,
> +                                   const struct i2c_device_id *id)
> +{
> +
> +	struct edt_ft5x06_i2c_ts_data *tsdata;
> +	struct edt_ft5x06_platform_data *pdata;

const.

> +	struct input_dev *input;
> +	int error;
> +	u8 rdbuf[23];
> +	char *model_name, *fw_version;
> +
> +	dev_dbg(&client->dev, "probing for EDT FT5x06 I2C\n");
> +
> +	if (!client->dev.platform_data) {
> +		dev_err(&client->dev, "no platform data?\n");
> +		return -ENODEV;
> +	}
> +
> +	tsdata = kzalloc(sizeof(*tsdata), GFP_KERNEL);
> +	if (!tsdata) {
> +		dev_err(&client->dev, "failed to allocate driver data!\n");
> +		dev_set_drvdata(&client->dev, NULL);
> +		return -ENOMEM;
> +	}
> +
> +	dev_set_drvdata(&client->dev, tsdata);
> +	tsdata->client = client;
> +	pdata = client->dev.platform_data;
> +
> +	tsdata->reset_pin = pdata->reset_pin;
> +	mutex_init(&tsdata->mutex);
> +
> +	if (tsdata->reset_pin >= 0) {
> +		error = gpio_request(tsdata->reset_pin, "edt-ft5x06 reset");
> +		if (error < 0) {
> +			dev_err(&client->dev,
> +			        "Failed to request GPIO %d as reset pin, error %d\n",
> +			         tsdata->reset_pin, error);
> +			goto err_free_tsdata;
> +		}
> +
> +		/* this pulls reset down, enabling the low active reset */
> +		if (gpio_direction_output(tsdata->reset_pin, 0) < 0) {
> +			dev_info(&client->dev, "switching to output failed\n");
> +			goto err_free_reset_pin;
> +		}
> +	}
> +
> +	/* request IRQ pin */
> +	tsdata->irq_pin = pdata->irq_pin;
> +	tsdata->irq = gpio_to_irq(tsdata->irq_pin);

Take from the client.

> +
> +	error = gpio_request(tsdata->irq_pin, "edt-ft5x06 irq");
> +	if (error < 0) {
> +		dev_err(&client->dev,
> +		        "Failed to request GPIO %d for IRQ %d, error %d\n",
> +		        tsdata->irq_pin, tsdata->irq, error);
> +		goto err_free_reset_pin;
> +	}
> +	gpio_direction_input(tsdata->irq_pin);

Should this GPIO configuration be done by the driver or by the board
code that configures i2c client? I think latter is better.

> +
> +	if (tsdata->reset_pin >= 0) {
> +		/* release reset */
> +		mdelay(50);
> +		gpio_set_value(tsdata->reset_pin, 1);
> +		mdelay(100);
> +	}
> +
> +	mutex_lock(&tsdata->mutex);

Why are you taking this lock here?

> +
> +	tsdata->factory_mode = 0;
> +
> +	if (edt_ft5x06_ts_readwrite(client, 1, "\xbb", 22, rdbuf) < 0) {
> +		dev_err(&client->dev, "probing failed\n");
> +		error = -ENODEV;
> +		goto err_free_irq_pin;
> +	}
> +
> +	tsdata->threshold = edt_ft5x06_i2c_register_read(tsdata,
> +	                                                 WORK_REGISTER_THRESHOLD);
> +	tsdata->gain      = edt_ft5x06_i2c_register_read(tsdata,
> +	                                                 WORK_REGISTER_GAIN);
> +	tsdata->offset    = edt_ft5x06_i2c_register_read(tsdata,
> +	                                                 WORK_REGISTER_OFFSET);
> +	tsdata->report_rate = edt_ft5x06_i2c_register_read(tsdata,
> +	                                                 WORK_REGISTER_REPORT_RATE);
> +	tsdata->num_x     = edt_ft5x06_i2c_register_read(tsdata,
> +	                                                 WORK_REGISTER_NUM_X);
> +	tsdata->num_y     = edt_ft5x06_i2c_register_read(tsdata,
> +	                                                 WORK_REGISTER_NUM_Y);
> +
> +	mutex_unlock(&tsdata->mutex);
> +
> +	/* remove last '$' end marker */
> +	rdbuf[22] = '\0';
> +	if (rdbuf[21] == '$')
> +		rdbuf[21] = '\0';
> +
> +	model_name = rdbuf + 1;
> +	/* look for Model/Version separator */
> +	fw_version = strchr(rdbuf, '*');
> +
> +	if (fw_version) {
> +		fw_version[0] = '\0';
> +		fw_version++;
> +		dev_info(&client->dev,
> +		         "Model \"%s\", Rev. \"%s\", %dx%d sensors\n",
> +		         model_name, fw_version, tsdata->num_x, tsdata->num_y);
> +	} else {
> +		dev_info(&client->dev, "Product ID \"%s\"\n", model_name);
> +	}
> +
> +	input = input_allocate_device();
> +	if (!input) {
> +		dev_err(&client->dev, "failed to allocate input device!\n");
> +		error = -ENOMEM;
> +		goto err_free_irq_pin;
> +	}
> +
> +	__set_bit(EV_SYN, input->evbit);
> +	__set_bit(EV_KEY, input->evbit);
> +	__set_bit(EV_ABS, input->evbit);
> +	__set_bit(BTN_TOUCH, input->keybit);
> +	input_set_abs_params(input, ABS_X, 0, tsdata->num_x * 64 - 1, 0, 0);
> +	input_set_abs_params(input, ABS_Y, 0, tsdata->num_y * 64 - 1, 0, 0);
> +	input_set_abs_params(input, ABS_PRESSURE, 0, 1, 0, 0);
> +	input_set_abs_params(input, ABS_MT_POSITION_X,
> +	                     0, tsdata->num_x * 64 - 1, 0, 0);
> +	input_set_abs_params(input, ABS_MT_POSITION_Y,
> +	                     0, tsdata->num_y * 64 - 1, 0, 0);
> +	input_set_abs_params(input, ABS_MT_TRACKING_ID, 0, 15, 0, 0);
> +
> +	input->name = kstrdup(model_name, GFP_NOIO);

Why don't you just allocate a few bytes in tsdata structure instead of
allocating and managing this separately. You'll also avoid race when
freeing it.

> +	input->id.bustype = BUS_I2C;
> +	input->dev.parent = &client->dev;
> +
> +	input_set_drvdata(input, tsdata);
> +
> +	tsdata->input = input;
> +
> +	error = input_register_device(input);
> +	if (error)
> +		goto err_free_input_device;
> +
> +	if (request_threaded_irq(tsdata->irq, NULL, edt_ft5x06_ts_isr,
> +	                         IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +	                         client->name, tsdata)) {
> +		dev_err(&client->dev, "Unable to request touchscreen IRQ.\n");
> +		input = NULL;

Why?

> +		error = -ENOMEM;
> +		goto err_unregister_device;
> +	}
> +
> +	error = sysfs_create_group(&client->dev.kobj,
> +	                           &edt_ft5x06_i2c_attr_group);
> +	if (error)
> +		goto err_free_irq;
> +
> +	device_init_wakeup(&client->dev, 1);
> +
> +	dev_dbg(&tsdata->client->dev,
> +	        "EDT FT5x06 initialized: IRQ pin %d, Reset pin %d.\n",
> +	        tsdata->irq_pin, tsdata->reset_pin);
> +
> +	return 0;
> +
> +err_free_irq:
> +	free_irq(tsdata->irq, tsdata);
> +err_unregister_device:
> +	input_unregister_device(input);
> +	input = NULL;

And here you leak input->name. Just store it in tsdata (as char[16] or
something), it's easier.

> +err_free_input_device:
> +	if (input) {
> +		kfree(input->name);
> +		input_free_device(input);
> +	}
> +err_free_irq_pin:
> +	gpio_free(tsdata->irq_pin);
> +err_free_reset_pin:
> +	if (tsdata->reset_pin >= 0)
> +		gpio_free(tsdata->reset_pin);
> +err_free_tsdata:
> +	kfree(tsdata);
> +	return error;
> +}
> +
> +static int edt_ft5x06_i2c_ts_remove(struct i2c_client *client)
> +{
> +	struct edt_ft5x06_i2c_ts_data *tsdata = dev_get_drvdata(&client->dev);
> +
> +	sysfs_remove_group(&client->dev.kobj, &edt_ft5x06_i2c_attr_group);
> +
> +	free_irq(tsdata->irq, tsdata);
> +	kfree(tsdata->input->name);

You are opening potential for dereferencing NULL pointer (someone could
access the name attribute before input device is unregistered).

> +	input_unregister_device(tsdata->input);
> +	gpio_free(tsdata->irq_pin);
> +	if (tsdata->reset_pin >= 0)
> +		gpio_free(tsdata->reset_pin);
> +	kfree(tsdata);
> +
> +	return 0;
> +}
> +

#ifdef CONFIG_PM_SLEEP
> +static int edt_ft5x06_i2c_ts_suspend(struct device *dev)
> +{
> +	struct edt_ft5x06_i2c_ts_data *tsdata = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev))
> +		enable_irq_wake(tsdata->irq);
> +
> +	return 0;
> +}
> +
> +static int edt_ft5x06_i2c_ts_resume(struct device *dev)
> +{
> +	struct edt_ft5x06_i2c_ts_data *tsdata = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev))
> +		disable_irq_wake(tsdata->irq);
> +
> +	return 0;
> +}
#endif

> +
> +static SIMPLE_DEV_PM_OPS(edt_ft5x06_i2c_ts_pm_ops,
> +                         edt_ft5x06_i2c_ts_suspend, edt_ft5x06_i2c_ts_resume);
> +
> +static const struct i2c_device_id edt_ft5x06_i2c_ts_id[] = {
> +	{ "edt-ft5x06", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, edt_ft5x06_i2c_ts_id);
> +
> +static struct i2c_driver edt_ft5x06_i2c_ts_driver = {
> +	.driver = {
> +		.owner = THIS_MODULE,
> +		.name = "edt_ft5x06_i2c",
> +		.pm = &edt_ft5x06_i2c_ts_pm_ops,
> +	},
> +	.id_table = edt_ft5x06_i2c_ts_id,
> +	.probe    = edt_ft5x06_i2c_ts_probe,
> +	.remove   = edt_ft5x06_i2c_ts_remove,
> +};
> +
> +static int __init edt_ft5x06_i2c_ts_init(void)
> +{
> +	return i2c_add_driver(&edt_ft5x06_i2c_ts_driver);
> +}
> +module_init(edt_ft5x06_i2c_ts_init);
> +
> +static void __exit edt_ft5x06_i2c_ts_exit(void)
> +{
> +	i2c_del_driver(&edt_ft5x06_i2c_ts_driver);
> +}
> +module_exit(edt_ft5x06_i2c_ts_exit);

Replace with:

module_i2c_driver(edt_ft5x06_i2c_ts_driver);

> +
> +MODULE_AUTHOR("Simon Budig <simon.budig@kernelconcepts.de>");
> +MODULE_DESCRIPTION("EDT FT5x06 I2C Touchscreen Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/input/edt-ft5x06.h b/include/linux/input/edt-ft5x06.h
> new file mode 100644
> index 0000000..db4130d
> --- /dev/null
> +++ b/include/linux/input/edt-ft5x06.h
> @@ -0,0 +1,17 @@
> +#ifndef _EDT_FT5X06_H
> +#define _EDT_FT5X06_H
> +
> +/*
> + * Copyright (c) 2011 Simon Budig, <simon.budig@kernelconcepts.de>
> + *
> + * 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.
> + */
> +
> +struct edt_ft5x06_platform_data {
> +	int irq_pin;
> +	int reset_pin;
> +};
> +
> +#endif /* _EDT_FT5X06_H */
> -- 
> 1.7.2.5
> 

-- 
Dmitry