Hi Bastien, On Wed, Sep 24, 2014 at 04:43:58PM +0200, Bastien Nocera wrote: > Add a driver for the Goodix touchscreen panel found in Onda v975w > tablets. The driver is based off the Android driver gt9xx.c found > in some Android code dumps, but now bears no resemblance to the original > driver. > > The driver was tested on the aforementioned tablet.
I was looking over the driver once again and I have a few concerns. > > Signed-off-by: Bastien Nocera <[email protected]> > Signed-off-by: Benjamin Tissoires <[email protected]> > Tested-by: Bastien Nocera <[email protected]> > --- > MAINTAINERS | 6 + > drivers/input/touchscreen/Kconfig | 13 ++ > drivers/input/touchscreen/Makefile | 1 + > drivers/input/touchscreen/goodix.c | 423 > +++++++++++++++++++++++++++++++++++++ > 4 files changed, 443 insertions(+) > create mode 100644 drivers/input/touchscreen/goodix.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 670b3dc..7a37464 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -4057,6 +4057,12 @@ L: [email protected] > S: Maintained > F: drivers/media/usb/go7007/ > > +GOODIX TOUCHSCREEN > +M: Bastien Nocera <[email protected]> > +L: [email protected] > +S: Maintained > +F: drivers/input/touchscreen/goodix.c > + > GPIO SUBSYSTEM > M: Linus Walleij <[email protected]> > M: Alexandre Courbot <[email protected]> > diff --git a/drivers/input/touchscreen/Kconfig > b/drivers/input/touchscreen/Kconfig > index 6bb9a7d..c328df3 100644 > --- a/drivers/input/touchscreen/Kconfig > +++ b/drivers/input/touchscreen/Kconfig > @@ -283,6 +283,19 @@ config TOUCHSCREEN_FUJITSU > To compile this driver as a module, choose M here: the > module will be called fujitsu-ts. > > +config TOUCHSCREEN_GOODIX > + tristate "Goodix I2C touchscreen" > + depends on I2C && ACPI > + help > + Say Y here if you have the Goodix touchscreen (such as one > + installed in Onda v975w tablets) connected to your > + system. > + > + If unsure, say N. > + > + To compile this driver as a module, choose M here: the > + module will be called goodix. > + > config TOUCHSCREEN_ILI210X > tristate "Ilitek ILI210X based touchscreen" > depends on I2C > diff --git a/drivers/input/touchscreen/Makefile > b/drivers/input/touchscreen/Makefile > index 4be94fc..55af212 100644 > --- a/drivers/input/touchscreen/Makefile > +++ b/drivers/input/touchscreen/Makefile > @@ -33,6 +33,7 @@ obj-$(CONFIG_TOUCHSCREEN_EETI) += eeti_ts.o > obj-$(CONFIG_TOUCHSCREEN_ELO) += elo.o > obj-$(CONFIG_TOUCHSCREEN_EGALAX) += egalax_ts.o > obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o > +obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix.o > obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o > obj-$(CONFIG_TOUCHSCREEN_INEXIO) += inexio.o > obj-$(CONFIG_TOUCHSCREEN_INTEL_MID) += intel-mid-touch.o > diff --git a/drivers/input/touchscreen/goodix.c > b/drivers/input/touchscreen/goodix.c > new file mode 100644 > index 0000000..fd02b5e > --- /dev/null > +++ b/drivers/input/touchscreen/goodix.c > @@ -0,0 +1,423 @@ > +/* > + * driver for Goodix Touchscreens > + * > + * Copyright (c) 2014 Red Hat Inc. > + * > + * This code is based on gt9xx.c authored by [email protected]: > + * > + * 2010 - 2012 Goodix Technology. > + */ > + > +/* > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the Free > + * Software Foundation; version 2 of the License. > + */ > + > +#include <linux/kernel.h> > +#include <linux/i2c.h> > +#include <linux/input.h> > +#include <linux/input/mt.h> > +#include <linux/module.h> > +#include <linux/delay.h> > +#include <linux/irq.h> > +#include <linux/interrupt.h> > +#include <linux/slab.h> > +#include <asm/unaligned.h> > + > +struct goodix_ts_data { > + struct i2c_client *client; > + struct input_dev *input_dev; > + int abs_x_max; > + int abs_y_max; > + unsigned int max_touch_num; > + unsigned int int_trigger_type; > +}; > + > +#define GOODIX_MAX_HEIGHT 4096 > +#define GOODIX_MAX_WIDTH 4096 > +#define GOODIX_INT_TRIGGER 1 > +#define GOODIX_MAX_TOUCH 10 > + > +#define GOODIX_CONFIG_MAX_LENGTH 240 > + > +/* Register defineS */ > +#define GOODIX_READ_COOR_ADDR 0x814E > +#define GOODIX_REG_CONFIG_DATA 0x8047 > +#define GOODIX_REG_VERSION 0x8140 > + > +#define RESOLUTION_LOC 1 > +#define TRIGGER_LOC 6 > + > +static const char *goodix_ts_name = "Goodix Capacitive TouchScreen"; > +static const unsigned long goodix_irq_flags[] = { > + IRQ_TYPE_EDGE_RISING, > + IRQ_TYPE_EDGE_FALLING, > + IRQ_TYPE_LEVEL_LOW, > + IRQ_TYPE_LEVEL_HIGH, > +}; > + > +/** > + * goodix_i2c_read - read data from a register of the i2c slave device. > + * > + * @client: i2c device. > + * @reg: the register to read from. > + * @buf: raw write data buffer. > + * @len: length of the buffer to write > + */ > +static int goodix_i2c_read(struct i2c_client *client, > + u16 reg, u8 *buf, int len) > +{ > + struct i2c_msg msgs[2]; > + u16 wbuf = cpu_to_be16(reg); > + int ret; > + > + msgs[0].flags = 0; > + msgs[0].addr = client->addr; > + msgs[0].len = 2; > + msgs[0].buf = (u8 *) &wbuf; > + > + msgs[1].flags = I2C_M_RD; > + msgs[1].addr = client->addr; > + msgs[1].len = len; > + msgs[1].buf = buf; > + > + ret = i2c_transfer(client->adapter, msgs, 2); > + return ret < 0 ? ret : (ret != ARRAY_SIZE(msgs) ? -EIO : 0); > +} > + > +/** > + * goodix_i2c_write - write data to the i2c slave device. > + * > + * @client: i2c device. > + * @reg: the register to read to. > + * @buf: raw write data buffer. > + * @len: length of the buffer to write > + */ > +static int goodix_i2c_write(struct i2c_client *client, > + u16 reg, u8 *buf, int len) > +{ > + struct i2c_msg msg; > + int ret; > + u8 *wbuf; > + u16 *addr; > + > + wbuf = kzalloc(len + 2, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + addr = ((u16 *) &wbuf[0]); > + *addr = cpu_to_be16(reg); > + memcpy(&wbuf[2], buf, len); > + > + msg.flags = 0; > + msg.addr = client->addr; > + msg.len = len + 2; > + msg.buf = wbuf; > + > + ret = i2c_transfer(client->adapter, &msg, 1); > + kfree(wbuf); > + return ret < 0 ? ret : (ret != 1 ? -EIO : 0); > +} > + > +static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data) > +{ > + int touch_num; > + int ret; > + > + ret = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR, data, 10); > + if (ret < 0) { > + dev_err(&ts->client->dev, "I2C transfer error (%d)\n", ret); > + return ret; > + } > + > + touch_num = data[0] & 0x0f; > + if (touch_num > GOODIX_MAX_TOUCH) > + return -EPROTO; > + > + if (touch_num > 1) { > + ret = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR + 10, > + &data[10], 8 * (touch_num - 1)); > + if (ret < 0) > + return ret; > + } I am a bit confused about this function. It looks like contact packet size is 8 bytes, and they preceded by a byte with total number of contacts reported, so why instead of 9 bytes we are reading 10? > + > + return touch_num; > +} > + > +static void goodix_ts_parse_touch(struct goodix_ts_data *ts, u8 *coor_data) > +{ > + int id = coor_data[0] & 0x0F; > + int input_x = get_unaligned_le16(&coor_data[1]); > + int input_y = get_unaligned_le16(&coor_data[3]); > + int input_w = get_unaligned_le16(&coor_data[5]); > + > + input_mt_slot(ts->input_dev, id); > + input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, true); > + input_report_abs(ts->input_dev, ABS_MT_POSITION_X, input_x); > + input_report_abs(ts->input_dev, ABS_MT_POSITION_Y, input_y); > + input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR, input_w); > + input_report_abs(ts->input_dev, ABS_MT_WIDTH_MAJOR, input_w); > +} > + > +/** > + * goodix_process_events - Process incoming events > + * > + * @ts: our goodix_ts_data pointer > + * > + * Called when the IRQ is triggered. Read the current device state, and push > + * the input events to the user space. > + */ > +static void goodix_process_events(struct goodix_ts_data *ts) > +{ > + u8 point_data[1 + 8 * GOODIX_MAX_TOUCH + 1]; Here again, why do we need extra byte? > + int touch_num; > + int i; > + > + touch_num = goodix_ts_read_input_report(ts, point_data); > + if (touch_num < 0) > + return; > + > + for (i = 0; i < touch_num; i++) > + goodix_ts_parse_touch(ts, &point_data[1 + 8 * i]); > + > + input_mt_sync_frame(ts->input_dev); > + input_sync(ts->input_dev); > +} > + > +/** > + * goodix_ts_irq_handler - The IRQ handler > + * > + * @irq: interrupt number. > + * @dev_id: private data pointer. > + */ > +static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id) > +{ > + struct goodix_ts_data *ts = dev_id; > + u8 end_cmd[1] = {0}; > + > + goodix_process_events(ts); > + > + if (goodix_i2c_write(ts->client, > + GOODIX_READ_COOR_ADDR, end_cmd, 1) < 0) > + dev_err(&ts->client->dev, "I2C write end_cmd error"); I am not happy that we need to allocate/deallocate memory for each interrupt. We only write one command to the driver, we could simply use i2c_master_send() with a constant buffer. BTW, you need terminate kernel messages with \n. Also, below is a patch with a few assorted changes that I'd like you to try if you have time. Thanks! -- Dmitry Input: goodix - random changes From: Dmitry Torokhov <[email protected]> Signed-off-by: Dmitry Torokhov <[email protected]> --- drivers/input/touchscreen/goodix.c | 208 ++++++++++++++++-------------------- 1 file changed, 91 insertions(+), 117 deletions(-) diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c index fd02b5e..1c2131c 100644 --- a/drivers/input/touchscreen/goodix.c +++ b/drivers/input/touchscreen/goodix.c @@ -37,11 +37,12 @@ struct goodix_ts_data { #define GOODIX_MAX_HEIGHT 4096 #define GOODIX_MAX_WIDTH 4096 #define GOODIX_INT_TRIGGER 1 -#define GOODIX_MAX_TOUCH 10 +#define GOODIX_CONTACT_SIZE 8 +#define GOODIX_MAX_CONTACTS 10 #define GOODIX_CONFIG_MAX_LENGTH 240 -/* Register defineS */ +/* Register defines */ #define GOODIX_READ_COOR_ADDR 0x814E #define GOODIX_REG_CONFIG_DATA 0x8047 #define GOODIX_REG_VERSION 0x8140 @@ -49,7 +50,6 @@ struct goodix_ts_data { #define RESOLUTION_LOC 1 #define TRIGGER_LOC 6 -static const char *goodix_ts_name = "Goodix Capacitive TouchScreen"; static const unsigned long goodix_irq_flags[] = { IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING, @@ -86,66 +86,38 @@ static int goodix_i2c_read(struct i2c_client *client, return ret < 0 ? ret : (ret != ARRAY_SIZE(msgs) ? -EIO : 0); } -/** - * goodix_i2c_write - write data to the i2c slave device. - * - * @client: i2c device. - * @reg: the register to read to. - * @buf: raw write data buffer. - * @len: length of the buffer to write - */ -static int goodix_i2c_write(struct i2c_client *client, - u16 reg, u8 *buf, int len) -{ - struct i2c_msg msg; - int ret; - u8 *wbuf; - u16 *addr; - - wbuf = kzalloc(len + 2, GFP_KERNEL); - if (!buf) - return -ENOMEM; - - addr = ((u16 *) &wbuf[0]); - *addr = cpu_to_be16(reg); - memcpy(&wbuf[2], buf, len); - - msg.flags = 0; - msg.addr = client->addr; - msg.len = len + 2; - msg.buf = wbuf; - - ret = i2c_transfer(client->adapter, &msg, 1); - kfree(wbuf); - return ret < 0 ? ret : (ret != 1 ? -EIO : 0); -} - static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data) { int touch_num; - int ret; + int error; - ret = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR, data, 10); - if (ret < 0) { - dev_err(&ts->client->dev, "I2C transfer error (%d)\n", ret); - return ret; + /* XXX should we read GOODIX_CONTACT_SIZE + 1 (i.e. 9) instead of 10? */ + error = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR, data, 10); + if (error) { + dev_err(&ts->client->dev, "I2C transfer error: %d\n", error); + return error; } touch_num = data[0] & 0x0f; - if (touch_num > GOODIX_MAX_TOUCH) + if (touch_num > GOODIX_MAX_CONTACTS) return -EPROTO; if (touch_num > 1) { - ret = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR + 10, - &data[10], 8 * (touch_num - 1)); - if (ret < 0) - return ret; + data += 10; + // data += GOODIX_CONTACT_SIZE + 1 + error = goodix_i2c_read(ts->client, + GOODIX_READ_COOR_ADDR + 10, + // GOODIX_READ_COOR_ADDR + 1 + GOODIX_CONTACT_SIZE ??? + data, + GOODIX_CONTACT_SIZE * (touch_num - 1)); + if (error) + return error; } return touch_num; } -static void goodix_ts_parse_touch(struct goodix_ts_data *ts, u8 *coor_data) +static void goodix_ts_report_touch(struct goodix_ts_data *ts, u8 *coor_data) { int id = coor_data[0] & 0x0F; int input_x = get_unaligned_le16(&coor_data[1]); @@ -170,7 +142,8 @@ static void goodix_ts_parse_touch(struct goodix_ts_data *ts, u8 *coor_data) */ static void goodix_process_events(struct goodix_ts_data *ts) { - u8 point_data[1 + 8 * GOODIX_MAX_TOUCH + 1]; + // XXX why extra +1 ? + u8 point_data[1 + GOODIX_CONTACT_SIZE * GOODIX_MAX_CONTACTS + 1]; int touch_num; int i; @@ -179,7 +152,8 @@ static void goodix_process_events(struct goodix_ts_data *ts) return; for (i = 0; i < touch_num; i++) - goodix_ts_parse_touch(ts, &point_data[1 + 8 * i]); + goodix_ts_report_touch(ts, + &point_data[1 + GOODIX_CONTACT_SIZE * i]); input_mt_sync_frame(ts->input_dev); input_sync(ts->input_dev); @@ -193,13 +167,16 @@ static void goodix_process_events(struct goodix_ts_data *ts) */ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id) { + static const u8 end_cmd[] = { + GOODIX_READ_COOR_ADDR & 0xff, + GOODIX_READ_COOR_ADDR >> 8, + 0 + }; struct goodix_ts_data *ts = dev_id; - u8 end_cmd[1] = {0}; goodix_process_events(ts); - if (goodix_i2c_write(ts->client, - GOODIX_READ_COOR_ADDR, end_cmd, 1) < 0) + if (i2c_master_send(ts->client, end_cmd, sizeof(end_cmd)) < 0) dev_err(&ts->client->dev, "I2C write end_cmd error"); return IRQ_HANDLED; @@ -214,14 +191,16 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id) */ static void goodix_read_config(struct goodix_ts_data *ts) { - int ret; u8 config[GOODIX_CONFIG_MAX_LENGTH]; + int error; - ret = goodix_i2c_read(ts->client, GOODIX_REG_CONFIG_DATA, config, + error = goodix_i2c_read(ts->client, GOODIX_REG_CONFIG_DATA, + config, GOODIX_CONFIG_MAX_LENGTH); - if (ret < 0) { - dev_err(&ts->client->dev, - "Error reading config, use default value!"); + if (error) { + dev_warn(&ts->client->dev, + "Error reading config (%d), using defaults\n", + error); ts->abs_x_max = GOODIX_MAX_WIDTH; ts->abs_y_max = GOODIX_MAX_HEIGHT; ts->int_trigger_type = GOODIX_INT_TRIGGER; @@ -231,9 +210,9 @@ static void goodix_read_config(struct goodix_ts_data *ts) ts->abs_x_max = get_unaligned_le16(&config[RESOLUTION_LOC]); ts->abs_y_max = get_unaligned_le16(&config[RESOLUTION_LOC + 2]); ts->int_trigger_type = (config[TRIGGER_LOC]) & 0x03; - if ((!ts->abs_x_max) || (!ts->abs_y_max)) { + if (!ts->abs_x_max || !ts->abs_y_max) { dev_err(&ts->client->dev, - "Invalid config, use default value!"); + "Invalid config, using defaults\n"); ts->abs_x_max = GOODIX_MAX_WIDTH; ts->abs_y_max = GOODIX_MAX_HEIGHT; } @@ -248,13 +227,13 @@ static void goodix_read_config(struct goodix_ts_data *ts) */ static int goodix_read_version(struct i2c_client *client, u16 *version) { - int ret; + int error; u8 buf[6]; - ret = goodix_i2c_read(client, GOODIX_REG_VERSION, buf, sizeof(buf)); - if (ret < 0) { - dev_err(&client->dev, "read version failed"); - return ret; + error = goodix_i2c_read(client, GOODIX_REG_VERSION, buf, sizeof(buf)); + if (error) { + dev_err(&client->dev, "read version failed: %d\n", error); + return error; } if (version) @@ -262,7 +241,7 @@ static int goodix_read_version(struct i2c_client *client, u16 *version) dev_info(&client->dev, "IC VERSION: %6ph", buf); - return ret; + return 0; } /** @@ -272,20 +251,22 @@ static int goodix_read_version(struct i2c_client *client, u16 *version) */ static int goodix_i2c_test(struct i2c_client *client) { - u8 test; - int ret; int retry = 0; + int error; + u8 test; while (retry++ < 2) { - ret = goodix_i2c_read(client, GOODIX_REG_CONFIG_DATA, - &test, 1); - if (ret >= 0) - return ret; + error = goodix_i2c_read(client, GOODIX_REG_CONFIG_DATA, + &test, 1); + if (!error) + return 0; - dev_err(&client->dev, "i2c test failed time %d.", retry); + dev_err(&client->dev, "i2c test failed attempt %d: %d\n", + retry, error); msleep(20); } - return ret; + + return error; } /** @@ -297,7 +278,7 @@ static int goodix_i2c_test(struct i2c_client *client) */ static int goodix_request_input_dev(struct goodix_ts_data *ts) { - int ret; + int error; ts->input_dev = devm_input_allocate_device(&ts->client->dev); if (!ts->input_dev) { @@ -316,79 +297,73 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts) input_set_abs_params(ts->input_dev, ABS_MT_WIDTH_MAJOR, 0, 255, 0, 0); input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0); - input_mt_init_slots(ts->input_dev, GOODIX_MAX_TOUCH, + input_mt_init_slots(ts->input_dev, GOODIX_MAX_CONTACTS, INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED); - ts->input_dev->name = goodix_ts_name; + ts->input_dev->name = "Goodix Capacitive TouchScreen"; ts->input_dev->phys = "input/ts"; ts->input_dev->id.bustype = BUS_I2C; ts->input_dev->id.vendor = 0x0416; ts->input_dev->id.product = 0x1001; ts->input_dev->id.version = 10427; - ret = input_register_device(ts->input_dev); - if (ret) { - dev_err(&ts->client->dev, "Failed to register %s input device", - ts->input_dev->name); - return ret; + error = input_register_device(ts->input_dev); + if (error) { + dev_err(&ts->client->dev, + "Failed to register input device: %d", error); + return error; } return 0; } static int goodix_ts_probe(struct i2c_client *client, - const struct i2c_device_id *id) + const struct i2c_device_id *id) { - int ret; struct goodix_ts_data *ts; + unsigned long irq_flags; + int error; u16 version_info; - dev_info(&client->dev, "I2C Address: 0x%02x", client->addr); + dev_dbg(&client->dev, "I2C Address: 0x%02x", client->addr); if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { - dev_err(&client->dev, "I2C check functionality failed."); - return -ENODEV; + dev_err(&client->dev, "I2C check functionality failed.\n"); + return -ENXIO; } + ts = devm_kzalloc(&client->dev, sizeof(*ts), GFP_KERNEL); - if (!ts) { - dev_err(&client->dev, "Alloc GFP_KERNEL memory failed."); + if (!ts) return -ENOMEM; - } ts->client = client; i2c_set_clientdata(client, ts); - ret = goodix_i2c_test(client); - if (ret < 0) { - dev_err(&client->dev, "I2C communication ERROR!"); - return ret; + error = goodix_i2c_test(client); + if (error) { + dev_err(&client->dev, "I2C communication failure: %d\n", error); + return error; } - goodix_read_config(ts); - - ret = goodix_request_input_dev(ts); - if (ret < 0) { - dev_err(&client->dev, "request input dev failed"); - return ret; + error = goodix_read_version(client, &version_info); + if (error) { + dev_err(&client->dev, "Read version failed."); + return error; } - ret = devm_request_threaded_irq(&ts->client->dev, - ts->client->irq, NULL, - goodix_ts_irq_handler, - goodix_irq_flags[ts->int_trigger_type] - | IRQF_ONESHOT, - ts->client->name, - ts); - if (ret < 0) { - dev_err(&ts->client->dev, - "Request IRQ failed! ERRNO: %d.", ret); - return ret; - } + goodix_read_config(ts); - ret = goodix_read_version(client, &version_info); - if (ret < 0) { - dev_err(&client->dev, "Read version failed."); - return ret; + error = goodix_request_input_dev(ts); + if (error) + return error; + + irq_flags = goodix_irq_flags[ts->int_trigger_type] | IRQF_ONESHOT; + error = devm_request_threaded_irq(&ts->client->dev, client->irq, + NULL, goodix_ts_irq_handler, + irq_flags, client->name, ts); + if (error) { + dev_err(&client->dev, "request IRQ failed: %d.", error); + return error; } return 0; @@ -414,7 +389,6 @@ static struct i2c_driver goodix_ts_driver = { .acpi_match_table = goodix_acpi_match, }, }; - module_i2c_driver(goodix_ts_driver); MODULE_AUTHOR("Benjamin Tissoires <[email protected]>"); -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
