* [PATCH] input: edt-ft5x06 - Touchscreen driver for FT5x06 based EDT displays
@ 2012-06-14 11:01 Olivier Sobrie
2012-06-14 12:05 ` Simon Budig
2012-06-20 18:40 ` Henrik Rydberg
0 siblings, 2 replies; 11+ messages in thread
From: Olivier Sobrie @ 2012-06-14 11:01 UTC (permalink / raw)
To: Dmitry Torokhov, Henrik Rydberg, linux-input
Cc: Simon Budig, Jan Paesmans, Olivier Sobrie
This driver adds support for the EDT touchscreens based on the
FocalTech chips.
Some part of the driver are based on patch for this chip sent by
Simon Budig to the linux-input mailing list.
Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
drivers/input/touchscreen/Kconfig | 12 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/edt-ft5x06.c | 816 ++++++++++++++++++++++++++++++++
include/linux/input/edt-ft5x06.h | 8 +
4 files changed, 837 insertions(+)
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 f67657b..032cbea 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -216,6 +216,18 @@ config TOUCHSCREEN_DYNAPRO
To compile this driver as a module, choose M here: the
module will be called dynapro.
+config TOUCHSCREEN_EDT_FT5X06
+ tristate "EDT FocalTech FT5x06 I2C Touchscreen support"
+ 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_HAMPSHIRE
tristate "Hampshire serial touchscreen"
select SERIO
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index eb8bfe1..bed430d7 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_TOUCHSCREEN_CYTTSP_SPI) += cyttsp_spi.o
obj-$(CONFIG_TOUCHSCREEN_DA9034) += da9034-ts.o
obj-$(CONFIG_TOUCHSCREEN_DA9052) += da9052_tsi.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..43c72a0
--- /dev/null
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -0,0 +1,816 @@
+/*
+ * Part of this code is inspired from a first version of a driver for
+ * this chip sent by Simon Budig to the linux-input mailing list
+ */
+
+#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/mt.h>
+#include <linux/input/edt-ft5x06.h>
+
+/* Commands in working mode */
+#define CMD_RDWR_REG 0xfc
+#define CMD_GET_TOUCH_DATA 0xf9
+#define CMD_GET_FW_VERSION 0xbb
+
+/* Commands in factory mode */
+#define CMD_RDWR_REG_FACTORY 0xf3
+#define CMD_RDATA_SHOW_FACTORY 0xf5
+
+/* Registers in working mode */
+#define W_REGISTER_THRESHOLD 0x00
+#define W_REGISTER_RATE 0x08
+#define W_REGISTER_GAIN 0x30
+#define W_REGISTER_OFFSET 0x31
+#define W_REGISTER_NUM_X 0x33
+#define W_REGISTER_NUM_Y 0x34
+#define W_REGISTER_OPMODE 0x3c
+
+/* Registers in factory mode */
+#define F_REGISTER_OPMODE 0x01
+#define F_REGISTER_RAWDATA 0x08
+
+/* Various defines */
+#define SENSOR_RESOLUTION 64
+#define MAX_TOUCHES 5
+#define CHANGE_MODE_RETRIES 10
+#define CHANGE_MODE_DELAY 5
+#define RAW_DATA_RETRIES 5
+#define RAW_DATA_DELAY 20
+#define MODEL_FW_LEN 22
+#define RX_TOUCH_DATA_HEADER 0xaaaa
+
+/* Events */
+#define EVENT_TOUCH_DOWN (0 << 2)
+#define EVENT_TOUCH_UP (1 << 2)
+#define EVENT_TOUCH_ON (2 << 2)
+#define EVENT_TOUCH_RESERVED (3 << 2)
+
+#define FOREACH_TOUCH(p, frm) \
+ for (p = &frm->p[0]; p < &frm->p[frm->ntouches]; p++)
+
+struct ft5x06 {
+ struct i2c_client *client;
+ struct input_dev *input;
+ int gpio_reset;
+ char model[MODEL_FW_LEN];
+ char fw_version[MODEL_FW_LEN];
+ int num_x;
+ int num_y;
+ /* mutex used to prevent access problems when switching between
+ * modes */
+ struct mutex mutex;
+ bool factory_mode;
+};
+
+struct tx_write_reg {
+ u8 addr;
+ u8 val;
+ u8 crc;
+} __packed;
+
+struct rx_read_reg {
+ u8 val;
+ u8 crc;
+} __packed;
+
+struct rx_touch_data {
+ __be16 header;
+ u8 len;
+ u8 ntouches;
+ u8 reserved;
+ struct ft5x06_xy_coordinate {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+ u8 x_high:4;
+ u8 event:4;
+#else
+ u8 event:4;
+ u8 x_high:4;
+#endif
+ u8 x_low;
+
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+ u8 y_high:4;
+ u8 tid:4;
+#else
+ u8 tid:4;
+ u8 y_high:4;
+#endif
+ u8 y_low;
+ } __packed p[MAX_TOUCHES];
+ u8 crc;
+} __packed;
+
+static int ft5x06_i2c_cmd(const struct i2c_client *client, u8 cmd,
+ void *wr_buf, size_t wr_len,
+ void *rd_buf, size_t rd_len)
+{
+ struct i2c_msg msgs[3];
+ int i = 1, rc;
+
+ msgs[0].addr = client->addr;
+ msgs[0].flags = 0;
+ msgs[0].len = sizeof(cmd);
+ msgs[0].buf = &cmd;
+
+ if (wr_len) {
+ msgs[i].addr = client->addr;
+ msgs[i].flags = 0;
+ msgs[i].len = wr_len;
+ msgs[i].buf = wr_buf;
+ i++;
+ }
+
+ if (rd_len) {
+ msgs[i].addr = client->addr;
+ msgs[i].flags = I2C_M_RD;
+ msgs[i].len = rd_len;
+ msgs[i].buf = rd_buf;
+ i++;
+ }
+
+ rc = i2c_transfer(client->adapter, msgs, i);
+ if (rc != i) {
+ dev_err(&client->dev, "i2c_transfer failed, error: %d\n",
+ rc);
+ return -EIO;
+ }
+
+ return rc;
+}
+
+static int ft5x06_check_frame_crc(const struct rx_touch_data *frm)
+{
+ const u8 *_frm = (const void *) frm;
+ u8 crc = 0;
+ int i;
+
+ for (i = 0; i < sizeof(struct rx_touch_data); i++)
+ crc ^= _frm[i];
+
+ return crc ? -EINVAL : 0;
+}
+
+static void ft5x06_report_events(const struct ft5x06 *priv,
+ const struct rx_touch_data *frm)
+{
+ struct device *dev = &priv->client->dev;
+ struct input_dev *input = priv->input;
+ int x, y;
+ const struct ft5x06_xy_coordinate *p;
+ char touch[MAX_TOUCHES];
+ int i;
+
+ if (frm->header != RX_TOUCH_DATA_HEADER
+ || frm->len != sizeof(struct rx_touch_data)
+ || frm->ntouches > MAX_TOUCHES) {
+ dev_dbg(dev,
+ "Invalid frame header (%04x), len (%d) or ntouches (%d)\n",
+ frm->header, frm->len, frm->ntouches);
+ return;
+ }
+
+ if (ft5x06_check_frame_crc(frm) < 0) {
+ dev_dbg(dev, "Invalid frame CRC\n");
+ return;
+ }
+
+ memset(touch, 0x00, sizeof(touch));
+
+ FOREACH_TOUCH(p, frm) {
+ if (p->event == EVENT_TOUCH_RESERVED)
+ continue;
+
+ if (p->tid > MAX_TOUCHES - 1)
+ continue;
+
+ touch[p->tid] = 1;
+
+ input_mt_slot(input, p->tid);
+ input_mt_report_slot_state(input, MT_TOOL_FINGER,
+ p->event != EVENT_TOUCH_UP);
+
+ if (p->event != EVENT_TOUCH_UP) {
+ x = (p->x_high << 8) | p->x_low;
+ y = (p->y_high << 8) | p->y_low;
+
+ input_report_abs(input, ABS_MT_POSITION_X, x);
+ input_report_abs(input, ABS_MT_POSITION_Y, y);
+ }
+ }
+
+ /* This loop is needed because the hardware doesn't always
+ * report the 'TOUCH_UP' event */
+ for (i = 0; i < MAX_TOUCHES; i++) {
+ if (touch[i])
+ continue;
+
+ input_mt_slot(input, i);
+ input_mt_report_slot_state(input, MT_TOOL_FINGER, 0);
+ }
+
+ input_mt_report_pointer_emulation(input, false);
+ input_sync(input);
+}
+
+static irqreturn_t ft5x06_isr(int irq, void *irq_data)
+{
+ struct ft5x06 *priv = irq_data;
+ struct i2c_client *client = priv->client;
+ struct device *dev = &client->dev;
+ struct rx_touch_data frm;
+ int rc;
+
+ memset(&frm, 0, sizeof(frm));
+
+ rc = ft5x06_i2c_cmd(client, CMD_GET_TOUCH_DATA, NULL, 0,
+ &frm, sizeof(frm));
+ if (rc < 0) {
+ dev_err(dev, "Unable to get touch data, error: %d\n", rc);
+ goto out;
+ }
+
+ ft5x06_report_events(priv, &frm);
+
+out:
+ return IRQ_HANDLED;
+}
+
+static int ft5x06_register_write(const struct ft5x06 *priv, u8 addr,
+ u8 value)
+{
+ struct tx_write_reg wr_reg;
+ u8 cmd;
+
+ if (priv->factory_mode) {
+ cmd = CMD_RDWR_REG_FACTORY;
+ wr_reg.addr = addr & 0x7f;
+ } else {
+ cmd = CMD_RDWR_REG;
+ wr_reg.addr = addr & 0x3f;
+ }
+
+ wr_reg.val = value;
+ wr_reg.crc = cmd ^ wr_reg.addr ^ wr_reg.val;
+
+ return ft5x06_i2c_cmd(priv->client, cmd, &wr_reg, sizeof(wr_reg),
+ NULL, 0);
+}
+
+static int ft5x06_register_read(const struct ft5x06 *priv, u8 addr)
+{
+ struct i2c_client *client = priv->client;
+ struct device *dev = &client->dev;
+ struct rx_read_reg rd_reg;
+ int rc;
+ u8 cmd;
+
+ if (priv->factory_mode) {
+ cmd = CMD_RDWR_REG_FACTORY;
+ addr = (addr & 0x7f) | 0x80;
+ } else {
+ cmd = CMD_RDWR_REG;
+ addr = (addr & 0x3f) | 0x40;
+ }
+
+ rc = ft5x06_i2c_cmd(client, cmd, &addr, sizeof(addr),
+ &rd_reg, sizeof(rd_reg));
+
+ if ((cmd ^ addr ^ rd_reg.val) != rd_reg.crc)
+ dev_err(dev, "crc error: 0x%02x expected, got 0x%02x\n",
+ (cmd ^ addr ^ rd_reg.val), rd_reg.crc);
+
+ return (rc < 0) ? rc : rd_reg.val;
+}
+
+static ssize_t ft5x06_setting_show(struct device *dev, u8 addr, char *buf)
+{
+ struct ft5x06 *priv = dev_get_drvdata(dev);
+ int rc = 0;
+
+ mutex_lock(&priv->mutex);
+
+ if (priv->factory_mode) {
+ dev_err(dev, "Cannot get registers in factory mode\n");
+ rc = -EIO;
+ goto out;
+ }
+
+ rc = ft5x06_register_read(priv, addr);
+ if (rc < 0) {
+ dev_err(dev, "Unable to get register value, error: %d\n",
+ rc);
+ goto out;
+ }
+
+ rc = sprintf(buf, "%d\n", rc);
+
+out:
+ mutex_unlock(&priv->mutex);
+ return rc;
+}
+
+static ssize_t ft5x06_setting_store(struct device *dev, u8 addr,
+ unsigned int val)
+{
+ struct ft5x06 *priv = dev_get_drvdata(dev);
+ int rc = 0;
+
+ mutex_lock(&priv->mutex);
+
+ if (priv->factory_mode) {
+ dev_err(dev, "Cannot set registers in factory mode\n");
+ rc = -EIO;
+ goto out;
+ }
+
+ rc = ft5x06_register_write(priv, addr, val);
+ if (rc < 0) {
+ dev_err(dev, "Unable to set register value, error: %d\n",
+ rc);
+ goto out;
+ }
+
+out:
+ mutex_unlock(&priv->mutex);
+ return rc;
+}
+
+static unsigned int get_value(const char *buf, unsigned int min,
+ unsigned int max)
+{
+ unsigned int val;
+
+ if (kstrtouint(buf, 10, &val))
+ return -EINVAL;
+
+ if ((val < min) || (val > max))
+ return -EINVAL;
+
+ return val;
+}
+
+static ssize_t ft5x06_store_rate(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ft5x06 *priv = dev_get_drvdata(dev);
+ unsigned int max = 12;
+ unsigned int val;
+
+ if (!strcmp(priv->model, "EP0570M06")
+ || !strcmp(priv->model, "EP0700M06"))
+ max = 8;
+
+ val = get_value(buf, 3, max);
+ if (val < 0) {
+ dev_err(dev, "Invalid value for rate (min: 3, max: %d)\n",
+ max);
+ return val;
+ }
+
+ return ft5x06_setting_store(dev, W_REGISTER_RATE, val) ? : count;
+}
+
+static ssize_t ft5x06_show_rate(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return ft5x06_setting_show(dev, W_REGISTER_RATE, buf);
+}
+static DEVICE_ATTR(rate, 0664, ft5x06_show_rate, ft5x06_store_rate);
+
+#define ft5x06_setting(name, addr, min, max) \
+static ssize_t ft5x06_store_##name(struct device *dev, \
+ struct device_attribute *attr, \
+ const char *buf, size_t count) \
+{ \
+ unsigned int val = get_value(buf, min, max); \
+ if (val < 0) { \
+ dev_err(dev, "Invalid value for " #name \
+ "(min: %d, max: %d)\n", min, max); \
+ return val; \
+ } \
+ return ft5x06_setting_store(dev, addr, val) ? : count; \
+} \
+static ssize_t ft5x06_show_##name(struct device *dev, \
+ struct device_attribute *attr, \
+ char *buf) \
+{ \
+ return ft5x06_setting_show(dev, addr, buf); \
+} \
+static DEVICE_ATTR(name, 0664, ft5x06_show_##name, ft5x06_store_##name)
+
+ft5x06_setting(threshold, W_REGISTER_THRESHOLD, 20, 80);
+ft5x06_setting(gain, W_REGISTER_GAIN, 0, 31);
+ft5x06_setting(offset, W_REGISTER_OFFSET, 0, 31);
+
+static ssize_t ft5x06_factory_mode_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ft5x06 *priv = dev_get_drvdata(dev);
+ return sprintf(buf, "%d\n", priv->factory_mode);
+}
+
+static int ft5x06_change_mode(struct device *dev, bool factory_mode)
+{
+ struct ft5x06 *priv = dev_get_drvdata(dev);
+ u8 reg, new_reg, val;
+ int i, rc;
+
+ if (factory_mode) {
+ reg = W_REGISTER_OPMODE;
+ new_reg = F_REGISTER_OPMODE;
+ val = 0x03;
+ } else {
+ reg = F_REGISTER_OPMODE;
+ new_reg = W_REGISTER_OPMODE;
+ val = 0x01;
+ }
+
+ rc = ft5x06_register_write(priv, reg, val);
+ if (rc < 0) {
+ dev_err(dev, "Failed to change mode, error: %d\n", rc);
+ return rc;
+ }
+
+ priv->factory_mode = factory_mode;
+
+ i = CHANGE_MODE_RETRIES;
+ do {
+ mdelay(CHANGE_MODE_DELAY);
+ rc = ft5x06_register_read(priv, new_reg);
+ if (rc == val)
+ break;
+ } while (--i);
+
+ if (rc != val) {
+ priv->factory_mode = factory_mode ? false : true;
+ dev_err(dev, "Not in %s mode after %d ms.\n",
+ factory_mode ? "factory" : "working",
+ CHANGE_MODE_RETRIES * CHANGE_MODE_DELAY);
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static ssize_t ft5x06_factory_mode_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ft5x06 *priv = dev_get_drvdata(dev);
+ struct i2c_client *client = to_i2c_client(dev);
+ unsigned int fmode;
+ int rc;
+
+ if (kstrtouint(buf, 10, &fmode))
+ return -EINVAL;
+
+ if (fmode > 1) {
+ dev_err(dev, "Invalid mode\n");
+ return -EINVAL;
+ }
+
+ if (fmode == priv->factory_mode)
+ return count;
+
+ mutex_lock(&priv->mutex);
+
+ if (fmode)
+ disable_irq(client->irq);
+
+ rc = ft5x06_change_mode(dev, fmode);
+
+ if (!priv->factory_mode)
+ enable_irq(client->irq);
+
+ mutex_unlock(&priv->mutex);
+ return rc ? : count;
+}
+static DEVICE_ATTR(factory_mode, 0664, ft5x06_factory_mode_show,
+ ft5x06_factory_mode_store);
+
+static ssize_t ft5x06_raw_data_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ft5x06 *priv = dev_get_drvdata(dev);
+ int i, rc;
+ char *ptr, wrbuf[2];
+
+ mutex_lock(&priv->mutex);
+
+ if (!priv->factory_mode) {
+ dev_err(dev, "Raw data not available in work mode\n");
+ rc = -EIO;
+ goto out;
+ }
+
+ rc = ft5x06_register_write(priv, F_REGISTER_RAWDATA, 0x01);
+ if (rc < 0) {
+ dev_err(dev,
+ "Error writing in rawdata register, error: %d\n",
+ rc);
+ goto out;
+ }
+
+ i = RAW_DATA_RETRIES;
+ do {
+ rc = ft5x06_register_read(priv, F_REGISTER_RAWDATA);
+ if (rc < 1)
+ break;
+ msleep(RAW_DATA_DELAY);
+ } while (--i);
+
+ if (rc < 0) {
+ rc = (rc < 0) ? rc : -ETIMEDOUT;
+ dev_err(dev, "Waiting time exceeded or error: %d\n", rc);
+ goto out;
+ }
+
+ ptr = buf;
+ wrbuf[0] = 0x0e;
+ for (i = 0; i <= priv->num_x; i++) {
+ wrbuf[1] = i;
+ rc = ft5x06_i2c_cmd(priv->client, CMD_RDATA_SHOW_FACTORY,
+ wrbuf, sizeof(wrbuf),
+ ptr, priv->num_y * 2);
+ if (rc < 0)
+ goto out;
+
+ ptr += priv->num_y * 2;
+ }
+
+ rc = ptr - buf;
+out:
+ mutex_unlock(&priv->mutex);
+ return rc;
+}
+static DEVICE_ATTR(raw_data, 0444, ft5x06_raw_data_show, NULL);
+
+static struct attribute *ft5x06_attrs[] = {
+ &dev_attr_gain.attr,
+ &dev_attr_offset.attr,
+ &dev_attr_threshold.attr,
+ &dev_attr_rate.attr,
+ &dev_attr_factory_mode.attr,
+ &dev_attr_raw_data.attr,
+ NULL
+};
+
+static const struct attribute_group ft5x06_attr_group = {
+ .attrs = ft5x06_attrs,
+};
+
+static
+int __devinit ft5x06_get_model_and_fw(const struct i2c_client *client,
+ char model[MODEL_FW_LEN],
+ char fw_version[MODEL_FW_LEN])
+{
+ const struct device *dev = &client->dev;
+ u8 buf[MODEL_FW_LEN];
+ int rc;
+ char *tmp;
+
+ rc = ft5x06_i2c_cmd(client, CMD_GET_FW_VERSION, NULL, 0,
+ buf, MODEL_FW_LEN);
+ if (rc < 0) {
+ dev_err(dev, "Failed to get firmware version, error: %d\n",
+ rc);
+ return rc;
+ }
+
+ /* Format: 0xbb,EPxx0M06*Azz_YYMMDD$ (22 chars) */
+ if (buf[0] != 0xbb || buf[MODEL_FW_LEN-1] != 0x24)
+ return -EINVAL;
+
+ buf[MODEL_FW_LEN-1] = 0x00;
+
+ tmp = strchr(buf, '*');
+ if (tmp) {
+ tmp[0] = '\0';
+ strlcpy(fw_version, ++tmp, MODEL_FW_LEN);
+ }
+
+ strlcpy(model, &buf[1], MODEL_FW_LEN);
+
+ return 0;
+}
+
+static int __devinit ft5x06_reset(struct ft5x06 *priv)
+{
+ struct i2c_client *client = priv->client;
+ struct device *dev = &client->dev;
+ struct edt_ft5x06_platform_data *pdata = dev->platform_data;
+ int rc;
+
+ priv->gpio_reset = pdata->gpio_reset;
+ if (!gpio_is_valid(priv->gpio_reset))
+ return 0;
+
+ rc = gpio_request_one(priv->gpio_reset, GPIOF_OUT_INIT_LOW,
+ "edt-ft5x06 reset pin");
+ if (rc < 0) {
+ dev_err(dev,
+ "Failed to get reset pin (GPIO %d), error %d\n",
+ priv->gpio_reset, rc);
+ return rc;
+ }
+
+ mdelay(50);
+ gpio_set_value(priv->gpio_reset, 1);
+ mdelay(100);
+
+ return 0;
+}
+
+static void __devinit ft5x06_init_input_dev(struct ft5x06 *priv)
+{
+ struct i2c_client *client = priv->client;
+ struct device *dev = &client->dev;
+ struct input_dev *input = priv->input;
+
+ priv->num_x = ft5x06_register_read(priv, W_REGISTER_NUM_X);
+ priv->num_y = ft5x06_register_read(priv, W_REGISTER_NUM_Y);
+
+ __set_bit(EV_SYN, input->evbit);
+ __set_bit(EV_KEY, input->evbit);
+ __set_bit(EV_ABS, input->evbit);
+ __set_bit(BTN_TOUCH, input->keybit);
+
+ /* Single touch */
+ input_set_abs_params(input, ABS_X, 0,
+ priv->num_x * SENSOR_RESOLUTION - 1, 0, 0);
+ input_set_abs_params(input, ABS_Y, 0,
+ priv->num_y * SENSOR_RESOLUTION - 1, 0, 0);
+
+ /* Multi touch */
+ input_mt_init_slots(input, MAX_TOUCHES);
+ input_set_abs_params(input, ABS_MT_POSITION_X, 0,
+ priv->num_x * SENSOR_RESOLUTION - 1, 0, 0);
+ input_set_abs_params(input, ABS_MT_POSITION_Y, 0,
+ priv->num_y * SENSOR_RESOLUTION - 1, 0, 0);
+
+ input->name = priv->model;
+ input->id.bustype = BUS_I2C;
+ input->dev.parent = dev;
+
+ input_set_drvdata(input, priv);
+
+ dev_dbg(dev, "Model: %s, Firmware: %s, %dx%d sensors\n",
+ priv->model, priv->fw_version ? : "Unknown",
+ priv->num_x, priv->num_y);
+}
+
+static int __devinit ft5x06_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct ft5x06 *priv;
+ struct device *dev = &client->dev;
+ int rc;
+
+ dev_dbg(dev, "Probing for EDT FT5x06 I2C\n");
+
+ if (!client->irq) {
+ dev_err(dev, "No IRQ?\n");
+ return -EINVAL;
+ }
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv) {
+ dev_err(dev, "Failed to allocate driver data!\n");
+ rc = -ENOMEM;
+ goto err_mem;
+ }
+
+ priv->input = input_allocate_device();
+ if (!priv->input) {
+ dev_err(dev, "Failed to allocate input device!\n");
+ rc = -ENOMEM;
+ goto err_mem;
+ }
+
+ dev_set_drvdata(dev, priv);
+ priv->client = client;
+ mutex_init(&priv->mutex);
+
+ rc = ft5x06_reset(priv);
+ if (rc)
+ goto err_reset;
+
+ rc = ft5x06_get_model_and_fw(client, priv->model,
+ priv->fw_version);
+ if (rc)
+ goto err_reset;
+
+ ft5x06_init_input_dev(priv);
+
+ rc = request_threaded_irq(client->irq, NULL, ft5x06_isr,
+ IRQF_TRIGGER_FALLING,
+ client->name, priv);
+ if (rc) {
+ dev_err(dev, "Unable to get touchscreen IRQ, error: %d\n",
+ rc);
+ goto err_irq;
+ }
+
+ rc = sysfs_create_group(&dev->kobj, &ft5x06_attr_group);
+ if (rc)
+ goto err_sysfs;
+
+ rc = input_register_device(priv->input);
+ if (rc)
+ goto err_register;
+
+ device_init_wakeup(dev, 1);
+
+ dev_dbg(dev, "EDT FT5x06 initialized (IRQ %d)\n", client->irq);
+
+ return 0;
+
+err_register:
+ sysfs_remove_group(&dev->kobj, &ft5x06_attr_group);
+err_sysfs:
+ free_irq(client->irq, priv);
+err_irq:
+ if (gpio_is_valid(priv->gpio_reset))
+ gpio_free(priv->gpio_reset);
+err_reset:
+ input_free_device(priv->input);
+err_mem:
+ kfree(priv);
+ return rc;
+}
+
+static int __devexit ft5x06_i2c_remove(struct i2c_client *client)
+{
+ struct ft5x06 *priv = dev_get_drvdata(&client->dev);
+ struct device *dev = &client->dev;
+
+ sysfs_remove_group(&dev->kobj, &ft5x06_attr_group);
+
+ free_irq(client->irq, priv);
+
+ input_unregister_device(priv->input);
+
+ if (gpio_is_valid(priv->gpio_reset))
+ gpio_free(priv->gpio_reset);
+
+ kfree(priv);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int ft5x06_i2c_suspend(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+
+ if (device_may_wakeup(dev))
+ enable_irq_wake(client->irq);
+
+ return 0;
+}
+
+static int ft5x06_i2c_resume(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+
+ if (device_may_wakeup(dev))
+ disable_irq_wake(client->irq);
+
+ return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(ft5x06_pm, ft5x06_i2c_suspend, ft5x06_i2c_resume);
+
+static const struct i2c_device_id ft5x06_i2c_id[] = {
+ { "edt-ft5x06", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, ft5x06_i2c_id);
+
+static struct i2c_driver ft5x06_i2c_driver = {
+ .driver = {
+ .owner = THIS_MODULE,
+ .name = "edt-ft5x06_i2c",
+ .pm = &ft5x06_pm,
+ },
+ .id_table = ft5x06_i2c_id,
+ .probe = ft5x06_i2c_probe,
+ .remove = __devexit_p(ft5x06_i2c_remove),
+};
+module_i2c_driver(ft5x06_i2c_driver);
+
+MODULE_AUTHOR("Olivier Sobrie <olivier@sobrie.be>");
+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..e687ffb
--- /dev/null
+++ b/include/linux/input/edt-ft5x06.h
@@ -0,0 +1,8 @@
+#ifndef _EDT_FT5X06_H
+#define _EDT_FT5X06_H
+
+struct edt_ft5x06_platform_data {
+ unsigned int gpio_reset;
+};
+
+#endif
--
1.7.9.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] input: edt-ft5x06 - Touchscreen driver for FT5x06 based EDT displays
2012-06-14 11:01 [PATCH] input: edt-ft5x06 - Touchscreen driver for FT5x06 based EDT displays Olivier Sobrie
@ 2012-06-14 12:05 ` Simon Budig
2012-06-14 12:48 ` Olivier Sobrie
2012-06-20 18:40 ` Henrik Rydberg
1 sibling, 1 reply; 11+ messages in thread
From: Simon Budig @ 2012-06-14 12:05 UTC (permalink / raw)
To: Olivier Sobrie; +Cc: Dmitry Torokhov, Henrik Rydberg, linux-input, Jan Paesmans
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 06/14/2012 01:01 PM, Olivier Sobrie wrote:
> This driver adds support for the EDT touchscreens based on the
> FocalTech chips.
Can you please elaborate a bit, why my driver was unsuitable for you?
> Some part of the driver are based on patch for this chip sent by
> Simon Budig to the linux-input mailing list.
Sorry for not following up to the latest review on my driver, I am a
little bit swamped at the moment...
Bye,
Simon
- --
Simon Budig kernel concepts GmbH
simon.budig@kernelconcepts.de Sieghuetter Hauptweg 48
+49-271-771091-17 D-57072 Siegen
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAk/Z03wACgkQO2O/RXesiHAXhACgl496AHQVQndvdNILQa6lt2LU
F44AmQGwZps72p1pdct1R2Lbstk1W6o8
=8rP+
-----END PGP SIGNATURE-----
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] input: edt-ft5x06 - Touchscreen driver for FT5x06 based EDT displays
2012-06-14 12:05 ` Simon Budig
@ 2012-06-14 12:48 ` Olivier Sobrie
0 siblings, 0 replies; 11+ messages in thread
From: Olivier Sobrie @ 2012-06-14 12:48 UTC (permalink / raw)
To: Simon Budig; +Cc: Dmitry Torokhov, Henrik Rydberg, linux-input, Jan Paesmans
On Thu, Jun 14, 2012 at 02:05:16PM +0200, Simon Budig wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 06/14/2012 01:01 PM, Olivier Sobrie wrote:
> > This driver adds support for the EDT touchscreens based on the
> > FocalTech chips.
>
> Can you please elaborate a bit, why my driver was unsuitable for you?
In fact I never used your driver...
I developped this driver in December 2011/January 2012. At that time,
you were at the version 2 of the code. I looked at the code of v2 and
found it not very clear... There were some missing things like the check
of the CRC of frames, checkpatch was complaining everywhere, etc. This
is why I decided to write my own driver.
I didn't posted it until now because I had hope that yours would have
been improved, finalized and included in linux version 3.4 or 3.5
(that's what you said in one of your previous mail).
As it is not the case after almost one year after v1 of your patch, I
decided today to post my version of the driver so that maybe we don't
miss next kernel version.
Kind regards,
--
Olivier
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] input: edt-ft5x06 - Touchscreen driver for FT5x06 based EDT displays
2012-06-14 11:01 [PATCH] input: edt-ft5x06 - Touchscreen driver for FT5x06 based EDT displays Olivier Sobrie
2012-06-14 12:05 ` Simon Budig
@ 2012-06-20 18:40 ` Henrik Rydberg
2012-06-20 21:27 ` Simon Budig
2012-06-21 6:35 ` Olivier Sobrie
1 sibling, 2 replies; 11+ messages in thread
From: Henrik Rydberg @ 2012-06-20 18:40 UTC (permalink / raw)
To: Olivier Sobrie; +Cc: Dmitry Torokhov, linux-input, Simon Budig, Jan Paesmans
Hi Olivier,
> This driver adds support for the EDT touchscreens based on the
> FocalTech chips.
>
> Some part of the driver are based on patch for this chip sent by
> Simon Budig to the linux-input mailing list.
>
> Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> ---
> drivers/input/touchscreen/Kconfig | 12 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/edt-ft5x06.c | 816 ++++++++++++++++++++++++++++++++
> include/linux/input/edt-ft5x06.h | 8 +
> 4 files changed, 837 insertions(+)
> 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 f67657b..032cbea 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -216,6 +216,18 @@ config TOUCHSCREEN_DYNAPRO
> To compile this driver as a module, choose M here: the
> module will be called dynapro.
>
> +config TOUCHSCREEN_EDT_FT5X06
> + tristate "EDT FocalTech FT5x06 I2C Touchscreen support"
> + 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_HAMPSHIRE
> tristate "Hampshire serial touchscreen"
> select SERIO
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index eb8bfe1..bed430d7 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_TOUCHSCREEN_CYTTSP_SPI) += cyttsp_spi.o
> obj-$(CONFIG_TOUCHSCREEN_DA9034) += da9034-ts.o
> obj-$(CONFIG_TOUCHSCREEN_DA9052) += da9052_tsi.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..43c72a0
> --- /dev/null
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -0,0 +1,816 @@
> +/*
> + * Part of this code is inspired from a first version of a driver for
> + * this chip sent by Simon Budig to the linux-input mailing list
> + */
Replacing the above with the standard GPL header with both of your
names ought to resolve any eventual issues, no?
> +
> +#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/mt.h>
> +#include <linux/input/edt-ft5x06.h>
> +
> +/* Commands in working mode */
> +#define CMD_RDWR_REG 0xfc
> +#define CMD_GET_TOUCH_DATA 0xf9
> +#define CMD_GET_FW_VERSION 0xbb
> +
> +/* Commands in factory mode */
> +#define CMD_RDWR_REG_FACTORY 0xf3
> +#define CMD_RDATA_SHOW_FACTORY 0xf5
> +
> +/* Registers in working mode */
> +#define W_REGISTER_THRESHOLD 0x00
> +#define W_REGISTER_RATE 0x08
> +#define W_REGISTER_GAIN 0x30
> +#define W_REGISTER_OFFSET 0x31
> +#define W_REGISTER_NUM_X 0x33
> +#define W_REGISTER_NUM_Y 0x34
> +#define W_REGISTER_OPMODE 0x3c
> +
> +/* Registers in factory mode */
> +#define F_REGISTER_OPMODE 0x01
> +#define F_REGISTER_RAWDATA 0x08
> +
> +/* Various defines */
> +#define SENSOR_RESOLUTION 64
> +#define MAX_TOUCHES 5
> +#define CHANGE_MODE_RETRIES 10
> +#define CHANGE_MODE_DELAY 5
> +#define RAW_DATA_RETRIES 5
> +#define RAW_DATA_DELAY 20
> +#define MODEL_FW_LEN 22
> +#define RX_TOUCH_DATA_HEADER 0xaaaa
> +
> +/* Events */
> +#define EVENT_TOUCH_DOWN (0 << 2)
> +#define EVENT_TOUCH_UP (1 << 2)
> +#define EVENT_TOUCH_ON (2 << 2)
> +#define EVENT_TOUCH_RESERVED (3 << 2)
> +
> +#define FOREACH_TOUCH(p, frm) \
> + for (p = &frm->p[0]; p < &frm->p[frm->ntouches]; p++)
Foreach is a great construct in many places, but a) it is only used
once, and b) C is preferred.
> +
> +struct ft5x06 {
> + struct i2c_client *client;
> + struct input_dev *input;
> + int gpio_reset;
> + char model[MODEL_FW_LEN];
> + char fw_version[MODEL_FW_LEN];
> + int num_x;
> + int num_y;
> + /* mutex used to prevent access problems when switching between
> + * modes */
"serialize mode switch"?
> + struct mutex mutex;
> + bool factory_mode;
> +};
> +
> +struct tx_write_reg {
> + u8 addr;
> + u8 val;
> + u8 crc;
> +} __packed;
> +
> +struct rx_read_reg {
> + u8 val;
> + u8 crc;
> +} __packed;
> +
> +struct rx_touch_data {
> + __be16 header;
> + u8 len;
> + u8 ntouches;
> + u8 reserved;
> + struct ft5x06_xy_coordinate {
> +#if defined(__LITTLE_ENDIAN_BITFIELD)
> + u8 x_high:4;
> + u8 event:4;
> +#else
> + u8 event:4;
> + u8 x_high:4;
> +#endif
> + u8 x_low;
> +
> +#if defined(__LITTLE_ENDIAN_BITFIELD)
> + u8 y_high:4;
> + u8 tid:4;
> +#else
> + u8 tid:4;
> + u8 y_high:4;
> +#endif
Here, OTOH, a define would make the code cleaner. Something like "u8
NIBBLES(a, b)" perhaps?
> + u8 y_low;
> + } __packed p[MAX_TOUCHES];
> + u8 crc;
> +} __packed;
> +
> +static int ft5x06_i2c_cmd(const struct i2c_client *client, u8 cmd,
> + void *wr_buf, size_t wr_len,
> + void *rd_buf, size_t rd_len)
> +{
> + struct i2c_msg msgs[3];
> + int i = 1, rc;
> +
> + msgs[0].addr = client->addr;
> + msgs[0].flags = 0;
> + msgs[0].len = sizeof(cmd);
> + msgs[0].buf = &cmd;
> +
> + if (wr_len) {
> + msgs[i].addr = client->addr;
> + msgs[i].flags = 0;
> + msgs[i].len = wr_len;
> + msgs[i].buf = wr_buf;
> + i++;
> + }
> +
> + if (rd_len) {
> + msgs[i].addr = client->addr;
> + msgs[i].flags = I2C_M_RD;
> + msgs[i].len = rd_len;
> + msgs[i].buf = rd_buf;
> + i++;
> + }
> +
> + rc = i2c_transfer(client->adapter, msgs, i);
> + if (rc != i) {
> + dev_err(&client->dev, "i2c_transfer failed, error: %d\n",
> + rc);
> + return -EIO;
> + }
> +
> + return rc;
> +}
> +
> +static int ft5x06_check_frame_crc(const struct rx_touch_data *frm)
> +{
> + const u8 *_frm = (const void *) frm;
> + u8 crc = 0;
> + int i;
> +
> + for (i = 0; i < sizeof(struct rx_touch_data); i++)
> + crc ^= _frm[i];
> +
> + return crc ? -EINVAL : 0;
> +}
I suoppose this is what the "reserved" field is for? Maybe there is a
better name for that field?
> +
> +static void ft5x06_report_events(const struct ft5x06 *priv,
> + const struct rx_touch_data *frm)
> +{
> + struct device *dev = &priv->client->dev;
> + struct input_dev *input = priv->input;
> + int x, y;
> + const struct ft5x06_xy_coordinate *p;
> + char touch[MAX_TOUCHES];
> + int i;
> +
> + if (frm->header != RX_TOUCH_DATA_HEADER
> + || frm->len != sizeof(struct rx_touch_data)
> + || frm->ntouches > MAX_TOUCHES) {
> + dev_dbg(dev,
> + "Invalid frame header (%04x), len (%d) or ntouches (%d)\n",
> + frm->header, frm->len, frm->ntouches);
> + return;
> + }
> +
> + if (ft5x06_check_frame_crc(frm) < 0) {
> + dev_dbg(dev, "Invalid frame CRC\n");
> + return;
> + }
> +
> + memset(touch, 0x00, sizeof(touch));
> +
> + FOREACH_TOUCH(p, frm) {
> + if (p->event == EVENT_TOUCH_RESERVED)
> + continue;
> +
> + if (p->tid > MAX_TOUCHES - 1)
> + continue;
> +
> + touch[p->tid] = 1;
> +
> + input_mt_slot(input, p->tid);
> + input_mt_report_slot_state(input, MT_TOOL_FINGER,
> + p->event != EVENT_TOUCH_UP);
> +
> + if (p->event != EVENT_TOUCH_UP) {
> + x = (p->x_high << 8) | p->x_low;
> + y = (p->y_high << 8) | p->y_low;
> +
> + input_report_abs(input, ABS_MT_POSITION_X, x);
> + input_report_abs(input, ABS_MT_POSITION_Y, y);
> + }
> + }
> +
> + /* This loop is needed because the hardware doesn't always
> + * report the 'TOUCH_UP' event */
> + for (i = 0; i < MAX_TOUCHES; i++) {
> + if (touch[i])
> + continue;
> +
> + input_mt_slot(input, i);
> + input_mt_report_slot_state(input, MT_TOOL_FINGER, 0);
> + }
And we aren't so lucky that p->tid is filled in for all these, or that
p is not filled in at all for these?
> +
> + input_mt_report_pointer_emulation(input, false);
> + input_sync(input);
> +}
> +
> +static irqreturn_t ft5x06_isr(int irq, void *irq_data)
> +{
> + struct ft5x06 *priv = irq_data;
> + struct i2c_client *client = priv->client;
> + struct device *dev = &client->dev;
> + struct rx_touch_data frm;
> + int rc;
> +
> + memset(&frm, 0, sizeof(frm));
> +
> + rc = ft5x06_i2c_cmd(client, CMD_GET_TOUCH_DATA, NULL, 0,
> + &frm, sizeof(frm));
> + if (rc < 0) {
> + dev_err(dev, "Unable to get touch data, error: %d\n", rc);
> + goto out;
> + }
> +
> + ft5x06_report_events(priv, &frm);
> +
> +out:
> + return IRQ_HANDLED;
> +}
> +
> +static int ft5x06_register_write(const struct ft5x06 *priv, u8 addr,
> + u8 value)
> +{
> + struct tx_write_reg wr_reg;
> + u8 cmd;
> +
> + if (priv->factory_mode) {
> + cmd = CMD_RDWR_REG_FACTORY;
> + wr_reg.addr = addr & 0x7f;
> + } else {
> + cmd = CMD_RDWR_REG;
> + wr_reg.addr = addr & 0x3f;
> + }
> +
> + wr_reg.val = value;
> + wr_reg.crc = cmd ^ wr_reg.addr ^ wr_reg.val;
> +
> + return ft5x06_i2c_cmd(priv->client, cmd, &wr_reg, sizeof(wr_reg),
> + NULL, 0);
> +}
> +
> +static int ft5x06_register_read(const struct ft5x06 *priv, u8 addr)
> +{
> + struct i2c_client *client = priv->client;
> + struct device *dev = &client->dev;
> + struct rx_read_reg rd_reg;
> + int rc;
> + u8 cmd;
> +
> + if (priv->factory_mode) {
> + cmd = CMD_RDWR_REG_FACTORY;
> + addr = (addr & 0x7f) | 0x80;
> + } else {
> + cmd = CMD_RDWR_REG;
> + addr = (addr & 0x3f) | 0x40;
> + }
> +
> + rc = ft5x06_i2c_cmd(client, cmd, &addr, sizeof(addr),
> + &rd_reg, sizeof(rd_reg));
> +
> + if ((cmd ^ addr ^ rd_reg.val) != rd_reg.crc)
> + dev_err(dev, "crc error: 0x%02x expected, got 0x%02x\n",
> + (cmd ^ addr ^ rd_reg.val), rd_reg.crc);
> +
> + return (rc < 0) ? rc : rd_reg.val;
> +}
> +
> +static ssize_t ft5x06_setting_show(struct device *dev, u8 addr, char *buf)
> +{
> + struct ft5x06 *priv = dev_get_drvdata(dev);
> + int rc = 0;
> +
> + mutex_lock(&priv->mutex);
> +
> + if (priv->factory_mode) {
> + dev_err(dev, "Cannot get registers in factory mode\n");
> + rc = -EIO;
> + goto out;
> + }
> +
> + rc = ft5x06_register_read(priv, addr);
> + if (rc < 0) {
> + dev_err(dev, "Unable to get register value, error: %d\n",
> + rc);
> + goto out;
> + }
> +
> + rc = sprintf(buf, "%d\n", rc);
> +
> +out:
> + mutex_unlock(&priv->mutex);
> + return rc;
> +}
> +
> +static ssize_t ft5x06_setting_store(struct device *dev, u8 addr,
> + unsigned int val)
> +{
> + struct ft5x06 *priv = dev_get_drvdata(dev);
> + int rc = 0;
> +
> + mutex_lock(&priv->mutex);
> +
> + if (priv->factory_mode) {
> + dev_err(dev, "Cannot set registers in factory mode\n");
> + rc = -EIO;
> + goto out;
> + }
> +
> + rc = ft5x06_register_write(priv, addr, val);
> + if (rc < 0) {
> + dev_err(dev, "Unable to set register value, error: %d\n",
> + rc);
> + goto out;
> + }
> +
> +out:
> + mutex_unlock(&priv->mutex);
> + return rc;
> +}
> +
> +static unsigned int get_value(const char *buf, unsigned int min,
> + unsigned int max)
> +{
> + unsigned int val;
> +
> + if (kstrtouint(buf, 10, &val))
> + return -EINVAL;
> +
> + if ((val < min) || (val > max))
> + return -EINVAL;
> +
> + return val;
> +}
> +
> +static ssize_t ft5x06_store_rate(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ft5x06 *priv = dev_get_drvdata(dev);
> + unsigned int max = 12;
> + unsigned int val;
> +
> + if (!strcmp(priv->model, "EP0570M06")
> + || !strcmp(priv->model, "EP0700M06"))
> + max = 8;
> +
> + val = get_value(buf, 3, max);
> + if (val < 0) {
> + dev_err(dev, "Invalid value for rate (min: 3, max: %d)\n",
> + max);
> + return val;
> + }
> +
> + return ft5x06_setting_store(dev, W_REGISTER_RATE, val) ? : count;
> +}
> +
> +static ssize_t ft5x06_show_rate(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + return ft5x06_setting_show(dev, W_REGISTER_RATE, buf);
> +}
> +static DEVICE_ATTR(rate, 0664, ft5x06_show_rate, ft5x06_store_rate);
> +
> +#define ft5x06_setting(name, addr, min, max) \
> +static ssize_t ft5x06_store_##name(struct device *dev, \
> + struct device_attribute *attr, \
> + const char *buf, size_t count) \
> +{ \
> + unsigned int val = get_value(buf, min, max); \
> + if (val < 0) { \
Ain't gonna happen...
> + dev_err(dev, "Invalid value for " #name \
> + "(min: %d, max: %d)\n", min, max); \
> + return val; \
> + } \
> + return ft5x06_setting_store(dev, addr, val) ? : count; \
> +} \
> +static ssize_t ft5x06_show_##name(struct device *dev, \
> + struct device_attribute *attr, \
> + char *buf) \
> +{ \
> + return ft5x06_setting_show(dev, addr, buf); \
> +} \
> +static DEVICE_ATTR(name, 0664, ft5x06_show_##name, ft5x06_store_##name)
> +
> +ft5x06_setting(threshold, W_REGISTER_THRESHOLD, 20, 80);
> +ft5x06_setting(gain, W_REGISTER_GAIN, 0, 31);
> +ft5x06_setting(offset, W_REGISTER_OFFSET, 0, 31);
Question: will the driver work without fiddling with these settings?
If so, are they really necessary? Maybe some part could be moved to
debugfs instead?
> +
> +static ssize_t ft5x06_factory_mode_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct ft5x06 *priv = dev_get_drvdata(dev);
> + return sprintf(buf, "%d\n", priv->factory_mode);
> +}
> +
> +static int ft5x06_change_mode(struct device *dev, bool factory_mode)
> +{
> + struct ft5x06 *priv = dev_get_drvdata(dev);
> + u8 reg, new_reg, val;
> + int i, rc;
> +
> + if (factory_mode) {
> + reg = W_REGISTER_OPMODE;
> + new_reg = F_REGISTER_OPMODE;
> + val = 0x03;
> + } else {
> + reg = F_REGISTER_OPMODE;
> + new_reg = W_REGISTER_OPMODE;
> + val = 0x01;
> + }
> +
> + rc = ft5x06_register_write(priv, reg, val);
> + if (rc < 0) {
> + dev_err(dev, "Failed to change mode, error: %d\n", rc);
> + return rc;
> + }
> +
> + priv->factory_mode = factory_mode;
> +
> + i = CHANGE_MODE_RETRIES;
> + do {
> + mdelay(CHANGE_MODE_DELAY);
> + rc = ft5x06_register_read(priv, new_reg);
> + if (rc == val)
> + break;
> + } while (--i);
With the break inside, this looks like a standard while/for loop.
> +
> + if (rc != val) {
> + priv->factory_mode = factory_mode ? false : true;
Double boolean logic.
> + dev_err(dev, "Not in %s mode after %d ms.\n",
> + factory_mode ? "factory" : "working",
> + CHANGE_MODE_RETRIES * CHANGE_MODE_DELAY);
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static ssize_t ft5x06_factory_mode_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ft5x06 *priv = dev_get_drvdata(dev);
> + struct i2c_client *client = to_i2c_client(dev);
> + unsigned int fmode;
> + int rc;
> +
> + if (kstrtouint(buf, 10, &fmode))
> + return -EINVAL;
> +
> + if (fmode > 1) {
> + dev_err(dev, "Invalid mode\n");
> + return -EINVAL;
> + }
> +
> + if (fmode == priv->factory_mode)
> + return count;
> +
> + mutex_lock(&priv->mutex);
> +
> + if (fmode)
> + disable_irq(client->irq);
> +
> + rc = ft5x06_change_mode(dev, fmode);
> +
> + if (!priv->factory_mode)
> + enable_irq(client->irq);
Leaving it off on error? When is it started again? Why not use the
error code for this path?
> +
> + mutex_unlock(&priv->mutex);
> + return rc ? : count;
> +}
> +static DEVICE_ATTR(factory_mode, 0664, ft5x06_factory_mode_show,
> + ft5x06_factory_mode_store);
> +
> +static ssize_t ft5x06_raw_data_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct ft5x06 *priv = dev_get_drvdata(dev);
> + int i, rc;
> + char *ptr, wrbuf[2];
> +
> + mutex_lock(&priv->mutex);
> +
> + if (!priv->factory_mode) {
> + dev_err(dev, "Raw data not available in work mode\n");
> + rc = -EIO;
> + goto out;
> + }
> +
> + rc = ft5x06_register_write(priv, F_REGISTER_RAWDATA, 0x01);
> + if (rc < 0) {
> + dev_err(dev,
> + "Error writing in rawdata register, error: %d\n",
> + rc);
> + goto out;
> + }
> +
> + i = RAW_DATA_RETRIES;
> + do {
> + rc = ft5x06_register_read(priv, F_REGISTER_RAWDATA);
> + if (rc < 1)
> + break;
> + msleep(RAW_DATA_DELAY);
> + } while (--i);
Recurrent pattern, function.
> +
> + if (rc < 0) {
> + rc = (rc < 0) ? rc : -ETIMEDOUT;
> + dev_err(dev, "Waiting time exceeded or error: %d\n", rc);
> + goto out;
> + }
> +
> + ptr = buf;
> + wrbuf[0] = 0x0e;
> + for (i = 0; i <= priv->num_x; i++) {
> + wrbuf[1] = i;
> + rc = ft5x06_i2c_cmd(priv->client, CMD_RDATA_SHOW_FACTORY,
> + wrbuf, sizeof(wrbuf),
> + ptr, priv->num_y * 2);
> + if (rc < 0)
> + goto out;
> +
> + ptr += priv->num_y * 2;
> + }
> +
> + rc = ptr - buf;
> +out:
> + mutex_unlock(&priv->mutex);
> + return rc;
> +}
> +static DEVICE_ATTR(raw_data, 0444, ft5x06_raw_data_show, NULL);
Same comments + debugfs.
> +
> +static struct attribute *ft5x06_attrs[] = {
> + &dev_attr_gain.attr,
> + &dev_attr_offset.attr,
> + &dev_attr_threshold.attr,
> + &dev_attr_rate.attr,
> + &dev_attr_factory_mode.attr,
> + &dev_attr_raw_data.attr,
> + NULL
> +};
Stopping here.
Clearly, there is a lot of work behind this driver, but not all of it
needs or should be carried in the kernel. A general cleanup and
reduction of sysfs usage would be good, if possible.
Thanks,
Henrik
> +
> +static const struct attribute_group ft5x06_attr_group = {
> + .attrs = ft5x06_attrs,
> +};
> +
> +static
> +int __devinit ft5x06_get_model_and_fw(const struct i2c_client *client,
> + char model[MODEL_FW_LEN],
> + char fw_version[MODEL_FW_LEN])
> +{
> + const struct device *dev = &client->dev;
> + u8 buf[MODEL_FW_LEN];
> + int rc;
> + char *tmp;
> +
> + rc = ft5x06_i2c_cmd(client, CMD_GET_FW_VERSION, NULL, 0,
> + buf, MODEL_FW_LEN);
> + if (rc < 0) {
> + dev_err(dev, "Failed to get firmware version, error: %d\n",
> + rc);
> + return rc;
> + }
> +
> + /* Format: 0xbb,EPxx0M06*Azz_YYMMDD$ (22 chars) */
> + if (buf[0] != 0xbb || buf[MODEL_FW_LEN-1] != 0x24)
> + return -EINVAL;
> +
> + buf[MODEL_FW_LEN-1] = 0x00;
> +
> + tmp = strchr(buf, '*');
> + if (tmp) {
> + tmp[0] = '\0';
> + strlcpy(fw_version, ++tmp, MODEL_FW_LEN);
> + }
> +
> + strlcpy(model, &buf[1], MODEL_FW_LEN);
> +
> + return 0;
> +}
> +
> +static int __devinit ft5x06_reset(struct ft5x06 *priv)
> +{
> + struct i2c_client *client = priv->client;
> + struct device *dev = &client->dev;
> + struct edt_ft5x06_platform_data *pdata = dev->platform_data;
> + int rc;
> +
> + priv->gpio_reset = pdata->gpio_reset;
> + if (!gpio_is_valid(priv->gpio_reset))
> + return 0;
> +
> + rc = gpio_request_one(priv->gpio_reset, GPIOF_OUT_INIT_LOW,
> + "edt-ft5x06 reset pin");
> + if (rc < 0) {
> + dev_err(dev,
> + "Failed to get reset pin (GPIO %d), error %d\n",
> + priv->gpio_reset, rc);
> + return rc;
> + }
> +
> + mdelay(50);
> + gpio_set_value(priv->gpio_reset, 1);
> + mdelay(100);
> +
> + return 0;
> +}
> +
> +static void __devinit ft5x06_init_input_dev(struct ft5x06 *priv)
> +{
> + struct i2c_client *client = priv->client;
> + struct device *dev = &client->dev;
> + struct input_dev *input = priv->input;
> +
> + priv->num_x = ft5x06_register_read(priv, W_REGISTER_NUM_X);
> + priv->num_y = ft5x06_register_read(priv, W_REGISTER_NUM_Y);
> +
> + __set_bit(EV_SYN, input->evbit);
> + __set_bit(EV_KEY, input->evbit);
> + __set_bit(EV_ABS, input->evbit);
> + __set_bit(BTN_TOUCH, input->keybit);
> +
> + /* Single touch */
> + input_set_abs_params(input, ABS_X, 0,
> + priv->num_x * SENSOR_RESOLUTION - 1, 0, 0);
> + input_set_abs_params(input, ABS_Y, 0,
> + priv->num_y * SENSOR_RESOLUTION - 1, 0, 0);
> +
> + /* Multi touch */
> + input_mt_init_slots(input, MAX_TOUCHES);
> + input_set_abs_params(input, ABS_MT_POSITION_X, 0,
> + priv->num_x * SENSOR_RESOLUTION - 1, 0, 0);
> + input_set_abs_params(input, ABS_MT_POSITION_Y, 0,
> + priv->num_y * SENSOR_RESOLUTION - 1, 0, 0);
> +
> + input->name = priv->model;
> + input->id.bustype = BUS_I2C;
> + input->dev.parent = dev;
> +
> + input_set_drvdata(input, priv);
> +
> + dev_dbg(dev, "Model: %s, Firmware: %s, %dx%d sensors\n",
> + priv->model, priv->fw_version ? : "Unknown",
> + priv->num_x, priv->num_y);
> +}
> +
> +static int __devinit ft5x06_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct ft5x06 *priv;
> + struct device *dev = &client->dev;
> + int rc;
> +
> + dev_dbg(dev, "Probing for EDT FT5x06 I2C\n");
> +
> + if (!client->irq) {
> + dev_err(dev, "No IRQ?\n");
> + return -EINVAL;
> + }
> +
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv) {
> + dev_err(dev, "Failed to allocate driver data!\n");
> + rc = -ENOMEM;
> + goto err_mem;
> + }
> +
> + priv->input = input_allocate_device();
> + if (!priv->input) {
> + dev_err(dev, "Failed to allocate input device!\n");
> + rc = -ENOMEM;
> + goto err_mem;
> + }
> +
> + dev_set_drvdata(dev, priv);
> + priv->client = client;
> + mutex_init(&priv->mutex);
> +
> + rc = ft5x06_reset(priv);
> + if (rc)
> + goto err_reset;
> +
> + rc = ft5x06_get_model_and_fw(client, priv->model,
> + priv->fw_version);
> + if (rc)
> + goto err_reset;
> +
> + ft5x06_init_input_dev(priv);
> +
> + rc = request_threaded_irq(client->irq, NULL, ft5x06_isr,
> + IRQF_TRIGGER_FALLING,
> + client->name, priv);
> + if (rc) {
> + dev_err(dev, "Unable to get touchscreen IRQ, error: %d\n",
> + rc);
> + goto err_irq;
> + }
> +
> + rc = sysfs_create_group(&dev->kobj, &ft5x06_attr_group);
> + if (rc)
> + goto err_sysfs;
> +
> + rc = input_register_device(priv->input);
> + if (rc)
> + goto err_register;
> +
> + device_init_wakeup(dev, 1);
> +
> + dev_dbg(dev, "EDT FT5x06 initialized (IRQ %d)\n", client->irq);
> +
> + return 0;
> +
> +err_register:
> + sysfs_remove_group(&dev->kobj, &ft5x06_attr_group);
> +err_sysfs:
> + free_irq(client->irq, priv);
> +err_irq:
> + if (gpio_is_valid(priv->gpio_reset))
> + gpio_free(priv->gpio_reset);
> +err_reset:
> + input_free_device(priv->input);
> +err_mem:
> + kfree(priv);
> + return rc;
> +}
> +
> +static int __devexit ft5x06_i2c_remove(struct i2c_client *client)
> +{
> + struct ft5x06 *priv = dev_get_drvdata(&client->dev);
> + struct device *dev = &client->dev;
> +
> + sysfs_remove_group(&dev->kobj, &ft5x06_attr_group);
> +
> + free_irq(client->irq, priv);
> +
> + input_unregister_device(priv->input);
> +
> + if (gpio_is_valid(priv->gpio_reset))
> + gpio_free(priv->gpio_reset);
> +
> + kfree(priv);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int ft5x06_i2c_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> +
> + if (device_may_wakeup(dev))
> + enable_irq_wake(client->irq);
> +
> + return 0;
> +}
> +
> +static int ft5x06_i2c_resume(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> +
> + if (device_may_wakeup(dev))
> + disable_irq_wake(client->irq);
> +
> + return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(ft5x06_pm, ft5x06_i2c_suspend, ft5x06_i2c_resume);
> +
> +static const struct i2c_device_id ft5x06_i2c_id[] = {
> + { "edt-ft5x06", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, ft5x06_i2c_id);
> +
> +static struct i2c_driver ft5x06_i2c_driver = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "edt-ft5x06_i2c",
> + .pm = &ft5x06_pm,
> + },
> + .id_table = ft5x06_i2c_id,
> + .probe = ft5x06_i2c_probe,
> + .remove = __devexit_p(ft5x06_i2c_remove),
> +};
> +module_i2c_driver(ft5x06_i2c_driver);
> +
> +MODULE_AUTHOR("Olivier Sobrie <olivier@sobrie.be>");
> +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..e687ffb
> --- /dev/null
> +++ b/include/linux/input/edt-ft5x06.h
> @@ -0,0 +1,8 @@
> +#ifndef _EDT_FT5X06_H
> +#define _EDT_FT5X06_H
> +
> +struct edt_ft5x06_platform_data {
> + unsigned int gpio_reset;
> +};
> +
> +#endif
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] input: edt-ft5x06 - Touchscreen driver for FT5x06 based EDT displays
2012-06-20 18:40 ` Henrik Rydberg
@ 2012-06-20 21:27 ` Simon Budig
2012-06-21 8:39 ` Dmitry Torokhov
2012-06-21 6:35 ` Olivier Sobrie
1 sibling, 1 reply; 11+ messages in thread
From: Simon Budig @ 2012-06-20 21:27 UTC (permalink / raw)
To: Henrik Rydberg
Cc: Olivier Sobrie, Dmitry Torokhov, linux-input, Jan Paesmans,
Anatolij Gustshin, Ilya Yanok
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi Henrik
On 06/20/2012 08:40 PM, Henrik Rydberg wrote:
>> +/* + * Part of this code is inspired from a first version of a
>> driver for + * this chip sent by Simon Budig to the linux-input
>> mailing list + */
>
> Replacing the above with the standard GPL header with both of your
> names ought to resolve any eventual issues, no?
Not sure. Olivier looked at an ancient version of my code, missing
multiple review spins and tests I did in the meantime with Dmitry and
Anatolij.
I obviously would prefer to see my driver in the kernel. Of course I
am open to improvement suggestions, but I am not really enthusiastic
about reviewing a driver that basically ditches most of my work.
>> +ft5x06_setting(threshold, W_REGISTER_THRESHOLD, 20, 80);
>> +ft5x06_setting(gain, W_REGISTER_GAIN, 0, 31);
>> +ft5x06_setting(offset, W_REGISTER_OFFSET, 0, 31);
>
> Question: will the driver work without fiddling with these
> settings? If so, are they really necessary? Maybe some part could
> be moved to debugfs instead?
Well, these are there in my driver as well (plus the "report rate"
setting of the sensor).
There are some parameters (gain/threshold/offset) which need tweaking
depending on the panel glass thickness and other device-specific
things. At least in the development phase it is necessary to tweak
with these to figure out good settings.
Dunno if sysfs is the best place for this, but I suspect that this is
a parameter that needs to be available at runtime as well, moving it
e.g. to platform data might not cut it.
Anyway. I have reviewed the changes Dmitry suggested and will test
them on real hardware tomorrow.
Bye,
Simon
- --
Simon Budig kernel concepts GmbH
simon.budig@kernelconcepts.de Sieghuetter Hauptweg 48
+49-271-771091-17 D-57072 Siegen
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAk/iQCQACgkQO2O/RXesiHCmyACeN4cFK2KEkPT83TqrO7f00YBZ
Vl4AoIBuoHqw6AZzoi24dGT0uepi+7Vx
=D8UO
-----END PGP SIGNATURE-----
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] input: edt-ft5x06 - Touchscreen driver for FT5x06 based EDT displays
2012-06-20 18:40 ` Henrik Rydberg
2012-06-20 21:27 ` Simon Budig
@ 2012-06-21 6:35 ` Olivier Sobrie
1 sibling, 0 replies; 11+ messages in thread
From: Olivier Sobrie @ 2012-06-21 6:35 UTC (permalink / raw)
To: Henrik Rydberg; +Cc: Dmitry Torokhov, linux-input, Simon Budig, Jan Paesmans
Hi Henrik,
Thanks for the review.
On Wed, Jun 20, 2012 at 08:40:15PM +0200, Henrik Rydberg wrote:
> Hi Olivier,
>
> > This driver adds support for the EDT touchscreens based on the
> > FocalTech chips.
> >
> > Some part of the driver are based on patch for this chip sent by
> > Simon Budig to the linux-input mailing list.
> >
> > Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> > ---
> > drivers/input/touchscreen/Kconfig | 12 +
> > drivers/input/touchscreen/Makefile | 1 +
> > drivers/input/touchscreen/edt-ft5x06.c | 816 ++++++++++++++++++++++++++++++++
> > include/linux/input/edt-ft5x06.h | 8 +
> > 4 files changed, 837 insertions(+)
> > 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 f67657b..032cbea 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -216,6 +216,18 @@ config TOUCHSCREEN_DYNAPRO
> > To compile this driver as a module, choose M here: the
> > module will be called dynapro.
> >
> > +config TOUCHSCREEN_EDT_FT5X06
> > + tristate "EDT FocalTech FT5x06 I2C Touchscreen support"
> > + 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_HAMPSHIRE
> > tristate "Hampshire serial touchscreen"
> > select SERIO
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index eb8bfe1..bed430d7 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -24,6 +24,7 @@ obj-$(CONFIG_TOUCHSCREEN_CYTTSP_SPI) += cyttsp_spi.o
> > obj-$(CONFIG_TOUCHSCREEN_DA9034) += da9034-ts.o
> > obj-$(CONFIG_TOUCHSCREEN_DA9052) += da9052_tsi.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..43c72a0
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > @@ -0,0 +1,816 @@
> > +/*
> > + * Part of this code is inspired from a first version of a driver for
> > + * this chip sent by Simon Budig to the linux-input mailing list
> > + */
>
> Replacing the above with the standard GPL header with both of your
> names ought to resolve any eventual issues, no?
No problem for me to add it. But not for Simon... look at his mail.
But is it really needed to put the GPL header at the top of each
driver? There is the MODULE_LICENSE that specify that the driver is
GPL.
>
> > +
> > +#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/mt.h>
> > +#include <linux/input/edt-ft5x06.h>
> > +
> > +/* Commands in working mode */
> > +#define CMD_RDWR_REG 0xfc
> > +#define CMD_GET_TOUCH_DATA 0xf9
> > +#define CMD_GET_FW_VERSION 0xbb
> > +
> > +/* Commands in factory mode */
> > +#define CMD_RDWR_REG_FACTORY 0xf3
> > +#define CMD_RDATA_SHOW_FACTORY 0xf5
> > +
> > +/* Registers in working mode */
> > +#define W_REGISTER_THRESHOLD 0x00
> > +#define W_REGISTER_RATE 0x08
> > +#define W_REGISTER_GAIN 0x30
> > +#define W_REGISTER_OFFSET 0x31
> > +#define W_REGISTER_NUM_X 0x33
> > +#define W_REGISTER_NUM_Y 0x34
> > +#define W_REGISTER_OPMODE 0x3c
> > +
> > +/* Registers in factory mode */
> > +#define F_REGISTER_OPMODE 0x01
> > +#define F_REGISTER_RAWDATA 0x08
> > +
> > +/* Various defines */
> > +#define SENSOR_RESOLUTION 64
> > +#define MAX_TOUCHES 5
> > +#define CHANGE_MODE_RETRIES 10
> > +#define CHANGE_MODE_DELAY 5
> > +#define RAW_DATA_RETRIES 5
> > +#define RAW_DATA_DELAY 20
> > +#define MODEL_FW_LEN 22
> > +#define RX_TOUCH_DATA_HEADER 0xaaaa
> > +
> > +/* Events */
> > +#define EVENT_TOUCH_DOWN (0 << 2)
> > +#define EVENT_TOUCH_UP (1 << 2)
> > +#define EVENT_TOUCH_ON (2 << 2)
> > +#define EVENT_TOUCH_RESERVED (3 << 2)
> > +
> > +#define FOREACH_TOUCH(p, frm) \
> > + for (p = &frm->p[0]; p < &frm->p[frm->ntouches]; p++)
>
> Foreach is a great construct in many places, but a) it is only used
> once, and b) C is preferred.
Ok I'll remove my foreach :-/ Or try to use it a second time ;-)
>
> > +
> > +struct ft5x06 {
> > + struct i2c_client *client;
> > + struct input_dev *input;
> > + int gpio_reset;
> > + char model[MODEL_FW_LEN];
> > + char fw_version[MODEL_FW_LEN];
> > + int num_x;
> > + int num_y;
> > + /* mutex used to prevent access problems when switching between
> > + * modes */
>
> "serialize mode switch"?
>
> > + struct mutex mutex;
> > + bool factory_mode;
> > +};
> > +
> > +struct tx_write_reg {
> > + u8 addr;
> > + u8 val;
> > + u8 crc;
> > +} __packed;
> > +
> > +struct rx_read_reg {
> > + u8 val;
> > + u8 crc;
> > +} __packed;
> > +
> > +struct rx_touch_data {
> > + __be16 header;
> > + u8 len;
> > + u8 ntouches;
> > + u8 reserved;
> > + struct ft5x06_xy_coordinate {
> > +#if defined(__LITTLE_ENDIAN_BITFIELD)
> > + u8 x_high:4;
> > + u8 event:4;
> > +#else
> > + u8 event:4;
> > + u8 x_high:4;
> > +#endif
> > + u8 x_low;
> > +
> > +#if defined(__LITTLE_ENDIAN_BITFIELD)
> > + u8 y_high:4;
> > + u8 tid:4;
> > +#else
> > + u8 tid:4;
> > + u8 y_high:4;
> > +#endif
>
> Here, OTOH, a define would make the code cleaner. Something like "u8
> NIBBLES(a, b)" perhaps?
Ok thanks for the suggestion. I'll do that.
> > + u8 y_low;
> > + } __packed p[MAX_TOUCHES];
> > + u8 crc;
> > +} __packed;
> > +
> > +static int ft5x06_i2c_cmd(const struct i2c_client *client, u8 cmd,
> > + void *wr_buf, size_t wr_len,
> > + void *rd_buf, size_t rd_len)
> > +{
> > + struct i2c_msg msgs[3];
> > + int i = 1, rc;
> > +
> > + msgs[0].addr = client->addr;
> > + msgs[0].flags = 0;
> > + msgs[0].len = sizeof(cmd);
> > + msgs[0].buf = &cmd;
> > +
> > + if (wr_len) {
> > + msgs[i].addr = client->addr;
> > + msgs[i].flags = 0;
> > + msgs[i].len = wr_len;
> > + msgs[i].buf = wr_buf;
> > + i++;
> > + }
> > +
> > + if (rd_len) {
> > + msgs[i].addr = client->addr;
> > + msgs[i].flags = I2C_M_RD;
> > + msgs[i].len = rd_len;
> > + msgs[i].buf = rd_buf;
> > + i++;
> > + }
> > +
> > + rc = i2c_transfer(client->adapter, msgs, i);
> > + if (rc != i) {
> > + dev_err(&client->dev, "i2c_transfer failed, error: %d\n",
> > + rc);
> > + return -EIO;
> > + }
> > +
> > + return rc;
> > +}
> > +
> > +static int ft5x06_check_frame_crc(const struct rx_touch_data *frm)
> > +{
> > + const u8 *_frm = (const void *) frm;
> > + u8 crc = 0;
> > + int i;
> > +
> > + for (i = 0; i < sizeof(struct rx_touch_data); i++)
> > + crc ^= _frm[i];
> > +
> > + return crc ? -EINVAL : 0;
> > +}
>
> I suoppose this is what the "reserved" field is for? Maybe there is a
> better name for that field?
I'm not sure to understand what you mean.
For the crc, there is a crc field at the end of the rx_touch_data
structure.
If I remembed well, the value of the field 'reserved' is always 0x00.
>
> > +
> > +static void ft5x06_report_events(const struct ft5x06 *priv,
> > + const struct rx_touch_data *frm)
> > +{
> > + struct device *dev = &priv->client->dev;
> > + struct input_dev *input = priv->input;
> > + int x, y;
> > + const struct ft5x06_xy_coordinate *p;
> > + char touch[MAX_TOUCHES];
> > + int i;
> > +
> > + if (frm->header != RX_TOUCH_DATA_HEADER
> > + || frm->len != sizeof(struct rx_touch_data)
> > + || frm->ntouches > MAX_TOUCHES) {
> > + dev_dbg(dev,
> > + "Invalid frame header (%04x), len (%d) or ntouches (%d)\n",
> > + frm->header, frm->len, frm->ntouches);
> > + return;
> > + }
> > +
> > + if (ft5x06_check_frame_crc(frm) < 0) {
> > + dev_dbg(dev, "Invalid frame CRC\n");
> > + return;
> > + }
> > +
> > + memset(touch, 0x00, sizeof(touch));
> > +
> > + FOREACH_TOUCH(p, frm) {
> > + if (p->event == EVENT_TOUCH_RESERVED)
> > + continue;
> > +
> > + if (p->tid > MAX_TOUCHES - 1)
> > + continue;
> > +
> > + touch[p->tid] = 1;
> > +
> > + input_mt_slot(input, p->tid);
> > + input_mt_report_slot_state(input, MT_TOOL_FINGER,
> > + p->event != EVENT_TOUCH_UP);
> > +
> > + if (p->event != EVENT_TOUCH_UP) {
> > + x = (p->x_high << 8) | p->x_low;
> > + y = (p->y_high << 8) | p->y_low;
> > +
> > + input_report_abs(input, ABS_MT_POSITION_X, x);
> > + input_report_abs(input, ABS_MT_POSITION_Y, y);
> > + }
> > + }
> > +
> > + /* This loop is needed because the hardware doesn't always
> > + * report the 'TOUCH_UP' event */
> > + for (i = 0; i < MAX_TOUCHES; i++) {
> > + if (touch[i])
> > + continue;
> > +
> > + input_mt_slot(input, i);
> > + input_mt_report_slot_state(input, MT_TOOL_FINGER, 0);
> > + }
>
> And we aren't so lucky that p->tid is filled in for all these, or that
> p is not filled in at all for these?
After getting the TOUCH_UP event, we don't get anymore info about the
finger. Im mean the p is not anymore filled in. That's why if we miss the
TOUCH_UP event, then we never report that the finger is not anymore on
the touchscreen without this loop.
>
> > +
> > + input_mt_report_pointer_emulation(input, false);
> > + input_sync(input);
> > +}
> > +
> > +static irqreturn_t ft5x06_isr(int irq, void *irq_data)
> > +{
> > + struct ft5x06 *priv = irq_data;
> > + struct i2c_client *client = priv->client;
> > + struct device *dev = &client->dev;
> > + struct rx_touch_data frm;
> > + int rc;
> > +
> > + memset(&frm, 0, sizeof(frm));
> > +
> > + rc = ft5x06_i2c_cmd(client, CMD_GET_TOUCH_DATA, NULL, 0,
> > + &frm, sizeof(frm));
> > + if (rc < 0) {
> > + dev_err(dev, "Unable to get touch data, error: %d\n", rc);
> > + goto out;
> > + }
> > +
> > + ft5x06_report_events(priv, &frm);
> > +
> > +out:
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int ft5x06_register_write(const struct ft5x06 *priv, u8 addr,
> > + u8 value)
> > +{
> > + struct tx_write_reg wr_reg;
> > + u8 cmd;
> > +
> > + if (priv->factory_mode) {
> > + cmd = CMD_RDWR_REG_FACTORY;
> > + wr_reg.addr = addr & 0x7f;
> > + } else {
> > + cmd = CMD_RDWR_REG;
> > + wr_reg.addr = addr & 0x3f;
> > + }
> > +
> > + wr_reg.val = value;
> > + wr_reg.crc = cmd ^ wr_reg.addr ^ wr_reg.val;
> > +
> > + return ft5x06_i2c_cmd(priv->client, cmd, &wr_reg, sizeof(wr_reg),
> > + NULL, 0);
> > +}
> > +
> > +static int ft5x06_register_read(const struct ft5x06 *priv, u8 addr)
> > +{
> > + struct i2c_client *client = priv->client;
> > + struct device *dev = &client->dev;
> > + struct rx_read_reg rd_reg;
> > + int rc;
> > + u8 cmd;
> > +
> > + if (priv->factory_mode) {
> > + cmd = CMD_RDWR_REG_FACTORY;
> > + addr = (addr & 0x7f) | 0x80;
> > + } else {
> > + cmd = CMD_RDWR_REG;
> > + addr = (addr & 0x3f) | 0x40;
> > + }
> > +
> > + rc = ft5x06_i2c_cmd(client, cmd, &addr, sizeof(addr),
> > + &rd_reg, sizeof(rd_reg));
> > +
> > + if ((cmd ^ addr ^ rd_reg.val) != rd_reg.crc)
> > + dev_err(dev, "crc error: 0x%02x expected, got 0x%02x\n",
> > + (cmd ^ addr ^ rd_reg.val), rd_reg.crc);
> > +
> > + return (rc < 0) ? rc : rd_reg.val;
> > +}
> > +
> > +static ssize_t ft5x06_setting_show(struct device *dev, u8 addr, char *buf)
> > +{
> > + struct ft5x06 *priv = dev_get_drvdata(dev);
> > + int rc = 0;
> > +
> > + mutex_lock(&priv->mutex);
> > +
> > + if (priv->factory_mode) {
> > + dev_err(dev, "Cannot get registers in factory mode\n");
> > + rc = -EIO;
> > + goto out;
> > + }
> > +
> > + rc = ft5x06_register_read(priv, addr);
> > + if (rc < 0) {
> > + dev_err(dev, "Unable to get register value, error: %d\n",
> > + rc);
> > + goto out;
> > + }
> > +
> > + rc = sprintf(buf, "%d\n", rc);
> > +
> > +out:
> > + mutex_unlock(&priv->mutex);
> > + return rc;
> > +}
> > +
> > +static ssize_t ft5x06_setting_store(struct device *dev, u8 addr,
> > + unsigned int val)
> > +{
> > + struct ft5x06 *priv = dev_get_drvdata(dev);
> > + int rc = 0;
> > +
> > + mutex_lock(&priv->mutex);
> > +
> > + if (priv->factory_mode) {
> > + dev_err(dev, "Cannot set registers in factory mode\n");
> > + rc = -EIO;
> > + goto out;
> > + }
> > +
> > + rc = ft5x06_register_write(priv, addr, val);
> > + if (rc < 0) {
> > + dev_err(dev, "Unable to set register value, error: %d\n",
> > + rc);
> > + goto out;
> > + }
> > +
> > +out:
> > + mutex_unlock(&priv->mutex);
> > + return rc;
> > +}
> > +
> > +static unsigned int get_value(const char *buf, unsigned int min,
> > + unsigned int max)
> > +{
> > + unsigned int val;
> > +
> > + if (kstrtouint(buf, 10, &val))
> > + return -EINVAL;
> > +
> > + if ((val < min) || (val > max))
> > + return -EINVAL;
> > +
> > + return val;
> > +}
> > +
> > +static ssize_t ft5x06_store_rate(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct ft5x06 *priv = dev_get_drvdata(dev);
> > + unsigned int max = 12;
> > + unsigned int val;
> > +
> > + if (!strcmp(priv->model, "EP0570M06")
> > + || !strcmp(priv->model, "EP0700M06"))
> > + max = 8;
> > +
> > + val = get_value(buf, 3, max);
> > + if (val < 0) {
> > + dev_err(dev, "Invalid value for rate (min: 3, max: %d)\n",
> > + max);
> > + return val;
> > + }
> > +
> > + return ft5x06_setting_store(dev, W_REGISTER_RATE, val) ? : count;
> > +}
> > +
> > +static ssize_t ft5x06_show_rate(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + return ft5x06_setting_show(dev, W_REGISTER_RATE, buf);
> > +}
> > +static DEVICE_ATTR(rate, 0664, ft5x06_show_rate, ft5x06_store_rate);
> > +
> > +#define ft5x06_setting(name, addr, min, max) \
> > +static ssize_t ft5x06_store_##name(struct device *dev, \
> > + struct device_attribute *attr, \
> > + const char *buf, size_t count) \
> > +{ \
> > + unsigned int val = get_value(buf, min, max); \
> > + if (val < 0) { \
>
> Ain't gonna happen...
Doh! Indeed sorry... I'll fix that.
>
> > + dev_err(dev, "Invalid value for " #name \
> > + "(min: %d, max: %d)\n", min, max); \
> > + return val; \
> > + } \
> > + return ft5x06_setting_store(dev, addr, val) ? : count; \
> > +} \
> > +static ssize_t ft5x06_show_##name(struct device *dev, \
> > + struct device_attribute *attr, \
> > + char *buf) \
> > +{ \
> > + return ft5x06_setting_show(dev, addr, buf); \
> > +} \
> > +static DEVICE_ATTR(name, 0664, ft5x06_show_##name, ft5x06_store_##name)
> > +
> > +ft5x06_setting(threshold, W_REGISTER_THRESHOLD, 20, 80);
> > +ft5x06_setting(gain, W_REGISTER_GAIN, 0, 31);
> > +ft5x06_setting(offset, W_REGISTER_OFFSET, 0, 31);
>
> Question: will the driver work without fiddling with these settings?
> If so, are they really necessary? Maybe some part could be moved to
> debugfs instead?
I think these four entries (threshold, gain, offset and rate) should go
in sysfs so the user is able to adjust it at runtime. However, I think
the raw entry would be better in debugfs as it is not needed to use the
touchscreen.
>
> > +
> > +static ssize_t ft5x06_factory_mode_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct ft5x06 *priv = dev_get_drvdata(dev);
> > + return sprintf(buf, "%d\n", priv->factory_mode);
> > +}
> > +
> > +static int ft5x06_change_mode(struct device *dev, bool factory_mode)
> > +{
> > + struct ft5x06 *priv = dev_get_drvdata(dev);
> > + u8 reg, new_reg, val;
> > + int i, rc;
> > +
> > + if (factory_mode) {
> > + reg = W_REGISTER_OPMODE;
> > + new_reg = F_REGISTER_OPMODE;
> > + val = 0x03;
> > + } else {
> > + reg = F_REGISTER_OPMODE;
> > + new_reg = W_REGISTER_OPMODE;
> > + val = 0x01;
> > + }
> > +
> > + rc = ft5x06_register_write(priv, reg, val);
> > + if (rc < 0) {
> > + dev_err(dev, "Failed to change mode, error: %d\n", rc);
> > + return rc;
> > + }
> > +
> > + priv->factory_mode = factory_mode;
> > +
> > + i = CHANGE_MODE_RETRIES;
> > + do {
> > + mdelay(CHANGE_MODE_DELAY);
> > + rc = ft5x06_register_read(priv, new_reg);
> > + if (rc == val)
> > + break;
> > + } while (--i);
>
> With the break inside, this looks like a standard while/for loop.
Ok I'll change it by a for loop if you prefer.
>
> > +
> > + if (rc != val) {
> > + priv->factory_mode = factory_mode ? false : true;
>
> Double boolean logic.
What do you suggest ?
>
> > + dev_err(dev, "Not in %s mode after %d ms.\n",
> > + factory_mode ? "factory" : "working",
> > + CHANGE_MODE_RETRIES * CHANGE_MODE_DELAY);
> > + return -EIO;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static ssize_t ft5x06_factory_mode_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct ft5x06 *priv = dev_get_drvdata(dev);
> > + struct i2c_client *client = to_i2c_client(dev);
> > + unsigned int fmode;
> > + int rc;
> > +
> > + if (kstrtouint(buf, 10, &fmode))
> > + return -EINVAL;
> > +
> > + if (fmode > 1) {
> > + dev_err(dev, "Invalid mode\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (fmode == priv->factory_mode)
> > + return count;
> > +
> > + mutex_lock(&priv->mutex);
> > +
> > + if (fmode)
> > + disable_irq(client->irq);
> > +
> > + rc = ft5x06_change_mode(dev, fmode);
> > +
> > + if (!priv->factory_mode)
> > + enable_irq(client->irq);
>
> Leaving it off on error? When is it started again? Why not use the
> error code for this path?
I don't know what's the best.
To summarize:
- In factory mode, the interrupt should be disabled
- In operation mode, the interrupt should be activated so we get
it when the user touches the screen.
With the above code, if we fail:
- to change from factory mode to operation, then we do not re-enable
the interrupt
- to change from operation mode to factory, then we re-enable the
interrupt
Are you OK with this?
Checking the rc code doesn't help us to know in which mode we were before
it failed or in which mode we are now, but combination with fmode and
rc will. So I can replace:
if (!priv->factory_mode)
enable_irq(client->irq);
by:
if ((fmode && rc) || (!fmode && !rc))
enable_irq(client->irq);
Do you prefer ?
>
> > +
> > + mutex_unlock(&priv->mutex);
> > + return rc ? : count;
> > +}
> > +static DEVICE_ATTR(factory_mode, 0664, ft5x06_factory_mode_show,
> > + ft5x06_factory_mode_store);
> > +
> > +static ssize_t ft5x06_raw_data_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct ft5x06 *priv = dev_get_drvdata(dev);
> > + int i, rc;
> > + char *ptr, wrbuf[2];
> > +
> > + mutex_lock(&priv->mutex);
> > +
> > + if (!priv->factory_mode) {
> > + dev_err(dev, "Raw data not available in work mode\n");
> > + rc = -EIO;
> > + goto out;
> > + }
> > +
> > + rc = ft5x06_register_write(priv, F_REGISTER_RAWDATA, 0x01);
> > + if (rc < 0) {
> > + dev_err(dev,
> > + "Error writing in rawdata register, error: %d\n",
> > + rc);
> > + goto out;
> > + }
> > +
> > + i = RAW_DATA_RETRIES;
> > + do {
> > + rc = ft5x06_register_read(priv, F_REGISTER_RAWDATA);
> > + if (rc < 1)
> > + break;
> > + msleep(RAW_DATA_DELAY);
> > + } while (--i);
>
> Recurrent pattern, function.
Ok.
>
> > +
> > + if (rc < 0) {
> > + rc = (rc < 0) ? rc : -ETIMEDOUT;
> > + dev_err(dev, "Waiting time exceeded or error: %d\n", rc);
> > + goto out;
> > + }
> > +
> > + ptr = buf;
> > + wrbuf[0] = 0x0e;
> > + for (i = 0; i <= priv->num_x; i++) {
> > + wrbuf[1] = i;
> > + rc = ft5x06_i2c_cmd(priv->client, CMD_RDATA_SHOW_FACTORY,
> > + wrbuf, sizeof(wrbuf),
> > + ptr, priv->num_y * 2);
> > + if (rc < 0)
> > + goto out;
> > +
> > + ptr += priv->num_y * 2;
> > + }
> > +
> > + rc = ptr - buf;
> > +out:
> > + mutex_unlock(&priv->mutex);
> > + return rc;
> > +}
> > +static DEVICE_ATTR(raw_data, 0444, ft5x06_raw_data_show, NULL);
>
> Same comments + debugfs.
Ok for me to put this into debugfs. Aswell as the factory_mode entry.
>
> > +
> > +static struct attribute *ft5x06_attrs[] = {
> > + &dev_attr_gain.attr,
> > + &dev_attr_offset.attr,
> > + &dev_attr_threshold.attr,
> > + &dev_attr_rate.attr,
> > + &dev_attr_factory_mode.attr,
> > + &dev_attr_raw_data.attr,
> > + NULL
> > +};
>
> Stopping here.
>
> Clearly, there is a lot of work behind this driver, but not all of it
> needs or should be carried in the kernel. A general cleanup and
> reduction of sysfs usage would be good, if possible.
Thanks for your help !
>
> Thanks,
> Henrik
>
> > +
> > +static const struct attribute_group ft5x06_attr_group = {
> > + .attrs = ft5x06_attrs,
> > +};
> > +
> > +static
> > +int __devinit ft5x06_get_model_and_fw(const struct i2c_client *client,
> > + char model[MODEL_FW_LEN],
> > + char fw_version[MODEL_FW_LEN])
> > +{
> > + const struct device *dev = &client->dev;
> > + u8 buf[MODEL_FW_LEN];
> > + int rc;
> > + char *tmp;
> > +
> > + rc = ft5x06_i2c_cmd(client, CMD_GET_FW_VERSION, NULL, 0,
> > + buf, MODEL_FW_LEN);
> > + if (rc < 0) {
> > + dev_err(dev, "Failed to get firmware version, error: %d\n",
> > + rc);
> > + return rc;
> > + }
> > +
> > + /* Format: 0xbb,EPxx0M06*Azz_YYMMDD$ (22 chars) */
> > + if (buf[0] != 0xbb || buf[MODEL_FW_LEN-1] != 0x24)
> > + return -EINVAL;
> > +
> > + buf[MODEL_FW_LEN-1] = 0x00;
> > +
> > + tmp = strchr(buf, '*');
> > + if (tmp) {
> > + tmp[0] = '\0';
> > + strlcpy(fw_version, ++tmp, MODEL_FW_LEN);
> > + }
> > +
> > + strlcpy(model, &buf[1], MODEL_FW_LEN);
> > +
> > + return 0;
> > +}
> > +
> > +static int __devinit ft5x06_reset(struct ft5x06 *priv)
> > +{
> > + struct i2c_client *client = priv->client;
> > + struct device *dev = &client->dev;
> > + struct edt_ft5x06_platform_data *pdata = dev->platform_data;
> > + int rc;
> > +
> > + priv->gpio_reset = pdata->gpio_reset;
> > + if (!gpio_is_valid(priv->gpio_reset))
> > + return 0;
> > +
> > + rc = gpio_request_one(priv->gpio_reset, GPIOF_OUT_INIT_LOW,
> > + "edt-ft5x06 reset pin");
> > + if (rc < 0) {
> > + dev_err(dev,
> > + "Failed to get reset pin (GPIO %d), error %d\n",
> > + priv->gpio_reset, rc);
> > + return rc;
> > + }
> > +
> > + mdelay(50);
> > + gpio_set_value(priv->gpio_reset, 1);
> > + mdelay(100);
> > +
> > + return 0;
> > +}
> > +
> > +static void __devinit ft5x06_init_input_dev(struct ft5x06 *priv)
> > +{
> > + struct i2c_client *client = priv->client;
> > + struct device *dev = &client->dev;
> > + struct input_dev *input = priv->input;
> > +
> > + priv->num_x = ft5x06_register_read(priv, W_REGISTER_NUM_X);
> > + priv->num_y = ft5x06_register_read(priv, W_REGISTER_NUM_Y);
> > +
> > + __set_bit(EV_SYN, input->evbit);
> > + __set_bit(EV_KEY, input->evbit);
> > + __set_bit(EV_ABS, input->evbit);
> > + __set_bit(BTN_TOUCH, input->keybit);
> > +
> > + /* Single touch */
> > + input_set_abs_params(input, ABS_X, 0,
> > + priv->num_x * SENSOR_RESOLUTION - 1, 0, 0);
> > + input_set_abs_params(input, ABS_Y, 0,
> > + priv->num_y * SENSOR_RESOLUTION - 1, 0, 0);
> > +
> > + /* Multi touch */
> > + input_mt_init_slots(input, MAX_TOUCHES);
> > + input_set_abs_params(input, ABS_MT_POSITION_X, 0,
> > + priv->num_x * SENSOR_RESOLUTION - 1, 0, 0);
> > + input_set_abs_params(input, ABS_MT_POSITION_Y, 0,
> > + priv->num_y * SENSOR_RESOLUTION - 1, 0, 0);
> > +
> > + input->name = priv->model;
> > + input->id.bustype = BUS_I2C;
> > + input->dev.parent = dev;
> > +
> > + input_set_drvdata(input, priv);
> > +
> > + dev_dbg(dev, "Model: %s, Firmware: %s, %dx%d sensors\n",
> > + priv->model, priv->fw_version ? : "Unknown",
> > + priv->num_x, priv->num_y);
> > +}
> > +
> > +static int __devinit ft5x06_i2c_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct ft5x06 *priv;
> > + struct device *dev = &client->dev;
> > + int rc;
> > +
> > + dev_dbg(dev, "Probing for EDT FT5x06 I2C\n");
> > +
> > + if (!client->irq) {
> > + dev_err(dev, "No IRQ?\n");
> > + return -EINVAL;
> > + }
> > +
> > + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > + if (!priv) {
> > + dev_err(dev, "Failed to allocate driver data!\n");
> > + rc = -ENOMEM;
> > + goto err_mem;
> > + }
> > +
> > + priv->input = input_allocate_device();
> > + if (!priv->input) {
> > + dev_err(dev, "Failed to allocate input device!\n");
> > + rc = -ENOMEM;
> > + goto err_mem;
> > + }
> > +
> > + dev_set_drvdata(dev, priv);
> > + priv->client = client;
> > + mutex_init(&priv->mutex);
> > +
> > + rc = ft5x06_reset(priv);
> > + if (rc)
> > + goto err_reset;
> > +
> > + rc = ft5x06_get_model_and_fw(client, priv->model,
> > + priv->fw_version);
> > + if (rc)
> > + goto err_reset;
> > +
> > + ft5x06_init_input_dev(priv);
> > +
> > + rc = request_threaded_irq(client->irq, NULL, ft5x06_isr,
> > + IRQF_TRIGGER_FALLING,
> > + client->name, priv);
> > + if (rc) {
> > + dev_err(dev, "Unable to get touchscreen IRQ, error: %d\n",
> > + rc);
> > + goto err_irq;
> > + }
> > +
> > + rc = sysfs_create_group(&dev->kobj, &ft5x06_attr_group);
> > + if (rc)
> > + goto err_sysfs;
> > +
> > + rc = input_register_device(priv->input);
> > + if (rc)
> > + goto err_register;
> > +
> > + device_init_wakeup(dev, 1);
> > +
> > + dev_dbg(dev, "EDT FT5x06 initialized (IRQ %d)\n", client->irq);
> > +
> > + return 0;
> > +
> > +err_register:
> > + sysfs_remove_group(&dev->kobj, &ft5x06_attr_group);
> > +err_sysfs:
> > + free_irq(client->irq, priv);
> > +err_irq:
> > + if (gpio_is_valid(priv->gpio_reset))
> > + gpio_free(priv->gpio_reset);
> > +err_reset:
> > + input_free_device(priv->input);
> > +err_mem:
> > + kfree(priv);
> > + return rc;
> > +}
> > +
> > +static int __devexit ft5x06_i2c_remove(struct i2c_client *client)
> > +{
> > + struct ft5x06 *priv = dev_get_drvdata(&client->dev);
> > + struct device *dev = &client->dev;
> > +
> > + sysfs_remove_group(&dev->kobj, &ft5x06_attr_group);
> > +
> > + free_irq(client->irq, priv);
> > +
> > + input_unregister_device(priv->input);
> > +
> > + if (gpio_is_valid(priv->gpio_reset))
> > + gpio_free(priv->gpio_reset);
> > +
> > + kfree(priv);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int ft5x06_i2c_suspend(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > +
> > + if (device_may_wakeup(dev))
> > + enable_irq_wake(client->irq);
> > +
> > + return 0;
> > +}
> > +
> > +static int ft5x06_i2c_resume(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > +
> > + if (device_may_wakeup(dev))
> > + disable_irq_wake(client->irq);
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +static SIMPLE_DEV_PM_OPS(ft5x06_pm, ft5x06_i2c_suspend, ft5x06_i2c_resume);
> > +
> > +static const struct i2c_device_id ft5x06_i2c_id[] = {
> > + { "edt-ft5x06", 0 },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ft5x06_i2c_id);
> > +
> > +static struct i2c_driver ft5x06_i2c_driver = {
> > + .driver = {
> > + .owner = THIS_MODULE,
> > + .name = "edt-ft5x06_i2c",
> > + .pm = &ft5x06_pm,
> > + },
> > + .id_table = ft5x06_i2c_id,
> > + .probe = ft5x06_i2c_probe,
> > + .remove = __devexit_p(ft5x06_i2c_remove),
> > +};
> > +module_i2c_driver(ft5x06_i2c_driver);
> > +
> > +MODULE_AUTHOR("Olivier Sobrie <olivier@sobrie.be>");
> > +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..e687ffb
> > --- /dev/null
> > +++ b/include/linux/input/edt-ft5x06.h
> > @@ -0,0 +1,8 @@
> > +#ifndef _EDT_FT5X06_H
> > +#define _EDT_FT5X06_H
> > +
> > +struct edt_ft5x06_platform_data {
> > + unsigned int gpio_reset;
> > +};
> > +
> > +#endif
> > --
> > 1.7.9.5
> >
--
Olivier
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] input: edt-ft5x06 - Touchscreen driver for FT5x06 based EDT displays
2012-06-20 21:27 ` Simon Budig
@ 2012-06-21 8:39 ` Dmitry Torokhov
2012-06-21 9:39 ` Simon Budig
0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2012-06-21 8:39 UTC (permalink / raw)
To: Simon Budig
Cc: Henrik Rydberg, Olivier Sobrie, linux-input, Jan Paesmans,
Anatolij Gustshin, Ilya Yanok
On Wed, Jun 20, 2012 at 11:27:00PM +0200, Simon Budig wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi Henrik
>
> On 06/20/2012 08:40 PM, Henrik Rydberg wrote:
> >> +/* + * Part of this code is inspired from a first version of a
> >> driver for + * this chip sent by Simon Budig to the linux-input
> >> mailing list + */
> >
> > Replacing the above with the standard GPL header with both of your
> > names ought to resolve any eventual issues, no?
>
> Not sure. Olivier looked at an ancient version of my code, missing
> multiple review spins and tests I did in the meantime with Dmitry and
> Anatolij.
>
> I obviously would prefer to see my driver in the kernel. Of course I
> am open to improvement suggestions, but I am not really enthusiastic
> about reviewing a driver that basically ditches most of my work.
>
> >> +ft5x06_setting(threshold, W_REGISTER_THRESHOLD, 20, 80);
> >> +ft5x06_setting(gain, W_REGISTER_GAIN, 0, 31);
> >> +ft5x06_setting(offset, W_REGISTER_OFFSET, 0, 31);
> >
> > Question: will the driver work without fiddling with these
> > settings? If so, are they really necessary? Maybe some part could
> > be moved to debugfs instead?
>
> Well, these are there in my driver as well (plus the "report rate"
> setting of the sensor).
>
> There are some parameters (gain/threshold/offset) which need tweaking
> depending on the panel glass thickness and other device-specific
> things. At least in the development phase it is necessary to tweak
> with these to figure out good settings.
>
> Dunno if sysfs is the best place for this, but I suspect that this is
> a parameter that needs to be available at runtime as well, moving it
> e.g. to platform data might not cut it.
>
> Anyway. I have reviewed the changes Dmitry suggested and will test
> them on real hardware tomorrow.
That would be great as I already reviewed your version. If there is
something missing from your version that is present in Olivier, I woudl
prefer getting an incremental patch.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] input: edt-ft5x06 - Touchscreen driver for FT5x06 based EDT displays
2012-06-21 8:39 ` Dmitry Torokhov
@ 2012-06-21 9:39 ` Simon Budig
2012-06-21 10:04 ` Dmitry Torokhov
0 siblings, 1 reply; 11+ messages in thread
From: Simon Budig @ 2012-06-21 9:39 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Henrik Rydberg, Olivier Sobrie, linux-input, Jan Paesmans,
Anatolij Gustshin, Ilya Yanok
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 06/21/2012 10:39 AM, Dmitry Torokhov wrote:
>> Anyway. I have reviewed the changes Dmitry suggested and will
>> test them on real hardware tomorrow.
Ok, the bad news is, that it doesn't work with your changes. There was
one oops I was able to resolve (i2c_set_clientdata must happen before
sysfs_create_group) but the touch also failed to deliver input events
and mode switching doesn't work.
I'll try to incorporate your patch incrementally, but this might have
to wait for the weekend.
> That would be great as I already reviewed your version. If there
> is something missing from your version that is present in Olivier,
> I woudl prefer getting an incremental patch.
One thing Olivier did which is not in my patch is, that he used the
type B protocol while I am still on type A. This is something I could
change.
Bye,
Simon
- --
Simon Budig kernel concepts GmbH
simon.budig@kernelconcepts.de Sieghuetter Hauptweg 48
+49-271-771091-17 D-57072 Siegen
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAk/i68wACgkQO2O/RXesiHA/hwCfQEZDL+KukaCsKpMMRdpuPfo2
FQQAnRqAtrPoAmLDeQ5bonidXMSlrtMk
=DPlX
-----END PGP SIGNATURE-----
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] input: edt-ft5x06 - Touchscreen driver for FT5x06 based EDT displays
2012-06-21 9:39 ` Simon Budig
@ 2012-06-21 10:04 ` Dmitry Torokhov
2012-06-21 14:53 ` Simon Budig
0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2012-06-21 10:04 UTC (permalink / raw)
To: Simon Budig
Cc: Henrik Rydberg, Olivier Sobrie, linux-input, Jan Paesmans,
Anatolij Gustshin, Ilya Yanok
On Thu, Jun 21, 2012 at 11:39:24AM +0200, Simon Budig wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 06/21/2012 10:39 AM, Dmitry Torokhov wrote:
> >> Anyway. I have reviewed the changes Dmitry suggested and will
> >> test them on real hardware tomorrow.
>
> Ok, the bad news is, that it doesn't work with your changes. There was
> one oops I was able to resolve (i2c_set_clientdata must happen before
> sysfs_create_group) but the touch also failed to deliver input events
> and mode switching doesn't work.
Hmm, OK, let me ponder this one a bit as well.
>
> I'll try to incorporate your patch incrementally, but this might have
> to wait for the weekend.
>
> > That would be great as I already reviewed your version. If there
> > is something missing from your version that is present in Olivier,
> > I woudl prefer getting an incremental patch.
>
> One thing Olivier did which is not in my patch is, that he used the
> type B protocol while I am still on type A. This is something I could
> change.
That would be good.
--
Dmitry
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] input: edt-ft5x06 - Touchscreen driver for FT5x06 based EDT displays
2012-06-21 10:04 ` Dmitry Torokhov
@ 2012-06-21 14:53 ` Simon Budig
2012-06-25 7:23 ` Dmitry Torokhov
0 siblings, 1 reply; 11+ messages in thread
From: Simon Budig @ 2012-06-21 14:53 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Henrik Rydberg, Olivier Sobrie, linux-input, Jan Paesmans,
Anatolij Gustshin, Ilya Yanok
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 06/21/2012 12:04 PM, Dmitry Torokhov wrote:
>> Ok, the bad news is, that it doesn't work with your changes.
>> There was one oops I was able to resolve (i2c_set_clientdata must
>> happen before sysfs_create_group) but the touch also failed to
>> deliver input events and mode switching doesn't work.
>
> Hmm, OK, let me ponder this one a bit as well.
Ok, found some of the problems. For starters I failed to realize that
you moved the irq initialization to the board file into the i2c-client
structure (which I did not initialize in my board file). This of
course explains why the touch seemed unresponsive...
The problem with mode switching was, that tsdata->factory_mode was
shuffeled around *after* some of the calls to _register_write/read.
This function however changes its way of operation depending on the
mode, you need to send different read/write commands depending on the
mode of the chip. So it is essential to change factory_mode before
invoking these functions.
Hmm, maybe this should be an explicit function parameter.
Then you implemented edt_ft5x06_i2c_attr_is_visible, but I fear this
is not working as you expect it to work. It has an effect on startup
time of the driver, but not when later switching the mode. This had
the effect that raw_data did not appear when switching to factory mode.
I am unsure regarding your changes to the handling of the platform
data. Is it guaranteed that the platform data sticks around for the
life time of the driver? Some of the data in the board file is marked
__initdata, which to my understanding means, that the kernel might
discard it at some point. That is why I copied the values into my
private structure. Is it safe to change that?
For the pin handling: you release the reset-pin after having done the
reset. Is this wise? This enables the user to use e.g. the
gpio-filesystem to reset the touch screen, potentially confusing the
driver and messing up the system.
Bye,
Simon
- --
Simon Budig kernel concepts GmbH
simon.budig@kernelconcepts.de Sieghuetter Hauptweg 48
+49-271-771091-17 D-57072 Siegen
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAk/jNYIACgkQO2O/RXesiHCwnACeI0k+8AcJXu8Kq9EA71ae6mQp
BcwAn2SM8U3012Z5+W3m8nrj9jI3HZTa
=xjs+
-----END PGP SIGNATURE-----
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] input: edt-ft5x06 - Touchscreen driver for FT5x06 based EDT displays
2012-06-21 14:53 ` Simon Budig
@ 2012-06-25 7:23 ` Dmitry Torokhov
0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2012-06-25 7:23 UTC (permalink / raw)
To: Simon Budig
Cc: Henrik Rydberg, Olivier Sobrie, linux-input, Jan Paesmans,
Anatolij Gustshin, Ilya Yanok
On Thu, Jun 21, 2012 at 04:53:54PM +0200, Simon Budig wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 06/21/2012 12:04 PM, Dmitry Torokhov wrote:
> >> Ok, the bad news is, that it doesn't work with your changes.
> >> There was one oops I was able to resolve (i2c_set_clientdata must
> >> happen before sysfs_create_group) but the touch also failed to
> >> deliver input events and mode switching doesn't work.
> >
> > Hmm, OK, let me ponder this one a bit as well.
>
> Ok, found some of the problems. For starters I failed to realize that
> you moved the irq initialization to the board file into the i2c-client
> structure (which I did not initialize in my board file). This of
> course explains why the touch seemed unresponsive...
>
> The problem with mode switching was, that tsdata->factory_mode was
> shuffeled around *after* some of the calls to _register_write/read.
> This function however changes its way of operation depending on the
> mode, you need to send different read/write commands depending on the
> mode of the chip. So it is essential to change factory_mode before
> invoking these functions.
I see.
>
> Hmm, maybe this should be an explicit function parameter.
>
> Then you implemented edt_ft5x06_i2c_attr_is_visible, but I fear this
> is not working as you expect it to work. It has an effect on startup
> time of the driver, but not when later switching the mode. This had
> the effect that raw_data did not appear when switching to factory mode.
Right, what was missing is a call to sysfs_update_group() after changing
factory mode setting.
>
> I am unsure regarding your changes to the handling of the platform
> data. Is it guaranteed that the platform data sticks around for the
> life time of the driver?
It has to, otherwise compiling driver as a module and/or
unbinding/rebinding would not work.
> Some of the data in the board file is marked
> __initdata, which to my understanding means, that the kernel might
> discard it at some point. That is why I copied the values into my
> private structure. Is it safe to change that?
You'll need to remove this __initdata markings.
>
> For the pin handling: you release the reset-pin after having done the
> reset. Is this wise? This enables the user to use e.g. the
> gpio-filesystem to reset the touch screen, potentially confusing the
> driver and messing up the system.
Hm, I do not have a strong opinion here.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-06-25 7:23 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-14 11:01 [PATCH] input: edt-ft5x06 - Touchscreen driver for FT5x06 based EDT displays Olivier Sobrie
2012-06-14 12:05 ` Simon Budig
2012-06-14 12:48 ` Olivier Sobrie
2012-06-20 18:40 ` Henrik Rydberg
2012-06-20 21:27 ` Simon Budig
2012-06-21 8:39 ` Dmitry Torokhov
2012-06-21 9:39 ` Simon Budig
2012-06-21 10:04 ` Dmitry Torokhov
2012-06-21 14:53 ` Simon Budig
2012-06-25 7:23 ` Dmitry Torokhov
2012-06-21 6:35 ` Olivier Sobrie
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.