[PATCH v2 0/8] Goodix touchscreen enhancements

[PATCH v2 0/8] Goodix touchscreen enhancements

[Irina Tirdea]

Add several enhancements to the Goodix touchscreen driver:
 - write configuration data to the device
 - power management support
 - cleanup and refactoring

I have kept the original patch for ESD ("input: goodix: add support
for ESD") since it is still under discussion. I have moved it to the
end of the patch set so it does not block the other changes. Once I have
an answer from DT folks on this I will make the necessary changes for it.

For testing the ACPI changes "ACPI: Translate Linux IRQ number directly
from GpioInt" patchset is needed [1]. This patchset has been merged in
the GPIO tree.

This patches are based on Goodix datasheets for GT911 and GT9271 and
on Goodix driver gt9xx.c for Android (publicly available in Android kernel
trees for various devices). Commit 771d8f1b178e ("Input: goodix - add
device tree support") references a set of public datasheets that can
be used for reference.

[1] https://lkml.org/lkml/2015/5/6/281

Changes in v2:
 - use request_firmware instead of ACPI/DT property for config
 - dropped "input: goodix: add ACPI IDs for GT911 and GT9271" patch
 - add ACPI DSDT excerpt in commit message where necessary
 - add comments for suspend/resume sleep values
 - dropped the checkpatch fixes that did not make sense
 - added Bastien's ack to the first patch

Irina Tirdea (8):
  input: goodix: fix alignment issues
  input: goodix: fix variable length array warning
  input: goodix: export id and version read from device
  input: goodix: reset device at init
  input: goodix: write configuration data to device
  input: goodix: add power management support
  input: goodix: use goodix_i2c_write_u8 instead of i2c_master_send
  input: goodix: add support for ESD

 .../bindings/input/touchscreen/goodix.txt          |  12 +
 drivers/input/touchscreen/goodix.c                 | 467 +++++++++++++++++++--
 2 files changed, 449 insertions(+), 30 deletions(-)

-- 
1.9.1

[PATCH v2 1/8] input: goodix: fix alignment issues

[Irina Tirdea]

Fix alignment to match open parenthesis detected by
running checkpatch.pl --strict.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Acked-by: Bastien Nocera <hadess@hadess.net>
---
 drivers/input/touchscreen/goodix.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 0d93b1e..a65e4d9 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -69,7 +69,7 @@ static const unsigned long goodix_irq_flags[] = {
  * @len: length of the buffer to write
  */
 static int goodix_i2c_read(struct i2c_client *client,
-				u16 reg, u8 *buf, int len)
+			   u16 reg, u8 *buf, int len)
 {
 	struct i2c_msg msgs[2];
 	u16 wbuf = cpu_to_be16(reg);
@@ -78,7 +78,7 @@ static int goodix_i2c_read(struct i2c_client *client,
 	msgs[0].flags = 0;
 	msgs[0].addr  = client->addr;
 	msgs[0].len   = 2;
-	msgs[0].buf   = (u8 *) &wbuf;
+	msgs[0].buf   = (u8 *)&wbuf;
 
 	msgs[1].flags = I2C_M_RD;
 	msgs[1].addr  = client->addr;
@@ -199,8 +199,8 @@ static void goodix_read_config(struct goodix_ts_data *ts)
 	int error;
 
 	error = goodix_i2c_read(ts->client, GOODIX_REG_CONFIG_DATA,
-			      config,
-			   GOODIX_CONFIG_MAX_LENGTH);
+				config,
+				GOODIX_CONFIG_MAX_LENGTH);
 	if (error) {
 		dev_warn(&ts->client->dev,
 			 "Error reading config (%d), using defaults\n",
@@ -297,9 +297,9 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts)
 				  BIT_MASK(EV_ABS);
 
 	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X, 0,
-				ts->abs_x_max, 0, 0);
+			     ts->abs_x_max, 0, 0);
 	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y, 0,
-				ts->abs_y_max, 0, 0);
+			     ts->abs_y_max, 0, 0);
 	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);
 
-- 
1.9.1

[PATCH v2 2/8] input: goodix: fix variable length array warning

[Irina Tirdea]

Fix sparse warning:
drivers/input/touchscreen/goodix.c:182:26: warning:
Variable length array is used.

Replace the variable length array with fixed length.

Some Goodix devices have maximum 5 touch points, while
others have 10 touch points. Using the maximum length
(80 bytes) for all devices will lead to wasting 40 bytes
when using devices with maximum 5 touch points. However,
that is prefferable to using kmalloc which will use even
more resources.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 drivers/input/touchscreen/goodix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index a65e4d9..18557e4 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -147,7 +147,7 @@ static void goodix_ts_report_touch(struct goodix_ts_data *ts, u8 *coor_data)
  */
 static void goodix_process_events(struct goodix_ts_data *ts)
 {
-	u8  point_data[1 + GOODIX_CONTACT_SIZE * ts->max_touch_num];
+	u8  point_data[1 + GOODIX_CONTACT_SIZE * GOODIX_MAX_CONTACTS];
 	int touch_num;
 	int i;
 
-- 
1.9.1

[PATCH v2 3/8] input: goodix: export id and version read from device

[Irina Tirdea]

Goodix touchscreens export through their registers a Product ID
and Firmware Version. The Product ID is an ASCII encoding of the
product name (e.g.: "911").

Export to sysfs (through the input subsystem) the product id and
firmware version read from the device rather than using constant
values.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 drivers/input/touchscreen/goodix.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 18557e4..7cd3780 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -47,7 +47,7 @@ struct goodix_ts_data {
 /* Register defines */
 #define GOODIX_READ_COOR_ADDR		0x814E
 #define GOODIX_REG_CONFIG_DATA		0x8047
-#define GOODIX_REG_VERSION		0x8140
+#define GOODIX_REG_ID			0x8140
 
 #define RESOLUTION_LOC		1
 #define MAX_CONTACTS_LOC	5
@@ -230,22 +230,27 @@ static void goodix_read_config(struct goodix_ts_data *ts)
  *
  * @client: the i2c client
  * @version: output buffer containing the version on success
+ * @id: output buffer containing the id on success
  */
-static int goodix_read_version(struct i2c_client *client, u16 *version)
+static int goodix_read_version(struct i2c_client *client, u16 *version, u16 *id)
 {
 	int error;
 	u8 buf[6];
+	char id_str[5];
 
-	error = goodix_i2c_read(client, GOODIX_REG_VERSION, buf, sizeof(buf));
+	error = goodix_i2c_read(client, GOODIX_REG_ID, buf, sizeof(buf));
 	if (error) {
 		dev_err(&client->dev, "read version failed: %d\n", error);
 		return error;
 	}
 
-	if (version)
-		*version = get_unaligned_le16(&buf[4]);
+	memcpy(id_str, buf, 4);
+	id_str[4] = 0;
+	if (kstrtou16(id_str, 10, id))
+		*id = 0x1001;
+	*version = get_unaligned_le16(&buf[4]);
 
-	dev_info(&client->dev, "IC VERSION: %6ph\n", buf);
+	dev_info(&client->dev, "ID %d, version: %04x\n", *id, *version);
 
 	return 0;
 }
@@ -279,10 +284,13 @@ static int goodix_i2c_test(struct i2c_client *client)
  * goodix_request_input_dev - Allocate, populate and register the input device
  *
  * @ts: our goodix_ts_data pointer
+ * @version: device firmware version
+ * @id: device ID
  *
  * Must be called during probe
  */
-static int goodix_request_input_dev(struct goodix_ts_data *ts)
+static int goodix_request_input_dev(struct goodix_ts_data *ts, u16 version,
+				    u16 id)
 {
 	int error;
 
@@ -310,8 +318,8 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts)
 	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;
+	ts->input_dev->id.product = id;
+	ts->input_dev->id.version = version;
 
 	error = input_register_device(ts->input_dev);
 	if (error) {
@@ -329,7 +337,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 	struct goodix_ts_data *ts;
 	unsigned long irq_flags;
 	int error;
-	u16 version_info;
+	u16 version_info, id_info;
 
 	dev_dbg(&client->dev, "I2C Address: 0x%02x\n", client->addr);
 
@@ -351,7 +359,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 		return error;
 	}
 
-	error = goodix_read_version(client, &version_info);
+	error = goodix_read_version(client, &version_info, &id_info);
 	if (error) {
 		dev_err(&client->dev, "Read version failed.\n");
 		return error;
@@ -359,7 +367,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 
 	goodix_read_config(ts);
 
-	error = goodix_request_input_dev(ts);
+	error = goodix_request_input_dev(ts, version_info, id_info);
 	if (error)
 		return error;
 
-- 
1.9.1

[PATCH v2 4/8] input: goodix: reset device at init

[Irina Tirdea]

After power on, it is recommended that the driver resets the device.
The reset procedure timing is described in the datasheet and is used
at device init (before writing device configuration) and
for power management. It is a sequence of setting the interrupt
and reset pins high/low at specific timing intervals. This procedure
also includes setting the slave address to the one specified in the
ACPI/device tree.

This is based on Goodix datasheets for GT911 and GT9271 and on Goodix
driver gt9xx.c for Android (publicly available in Android kernel
trees for various devices).

For reset the driver needs to control the interrupt and
reset gpio pins (configured through ACPI 5.1/device tree).
For ACPI, the pins can be specified using ACPI 5.1:

Device (STAC)
{
    Name (_HID, "GDIX1001")
    ...

    Method (_CRS, 0, Serialized)
    {
        Name (RBUF, ResourceTemplate ()
        {
            I2cSerialBus (0x0014, ControllerInitiated, 0x00061A80,
                AddressingMode7Bit, "\\I2C0",
                0x00, ResourceConsumer, ,
                )

            GpioInt (Edge, ActiveHigh, Exclusive, PullNone, 0x0000,
                "\\I2C0", 0x00, ResourceConsumer, ,
                 )
                 {   // Pin list
                     0
                 }

            GpioIo (Exclusive, PullDown, 0x0000, 0x0000,
                IoRestrictionOutputOnly, "\\I2C0", 0x00,
                ResourceConsumer, ,
                )
                {
                     1
                }
        })
        Return (RBUF)
    }

    Name (_DSD,  Package ()
    {
        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
        Package ()
        {
            Package (2) {"irq-gpio", Package() {^STAC, 0, 0, 0 }},
            Package (2) {"reset-gpio", Package() {^STAC, 1, 0, 0 }},
            ...
        }
    }

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 .../bindings/input/touchscreen/goodix.txt          |   5 ++
 drivers/input/touchscreen/goodix.c                 | 100 +++++++++++++++++++++
 2 files changed, 105 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index 8ba98ee..7137881 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -12,6 +12,8 @@ Required properties:
  - reg			: I2C address of the chip. Should be 0x5d or 0x14
  - interrupt-parent	: Interrupt controller to which the chip is connected
  - interrupts		: Interrupt to which the chip is connected
+ - irq-gpio		: GPIO pin used for IRQ
+ - reset-gpio		: GPIO pin used for reset
 
 Example:
 
@@ -23,6 +25,9 @@ Example:
 			reg = <0x5d>;
 			interrupt-parent = <&gpio>;
 			interrupts = <0 0>;
+
+			irq-gpio = <&gpio1 0 0>;
+			reset-gpio = <&gpio1 1 0>;
 		};
 
 		/* ... */
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 7cd3780..c345eb7 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -24,6 +24,7 @@
 #include <linux/interrupt.h>
 #include <linux/slab.h>
 #include <linux/acpi.h>
+#include <linux/gpio.h>
 #include <linux/of.h>
 #include <asm/unaligned.h>
 
@@ -34,8 +35,13 @@ struct goodix_ts_data {
 	int abs_y_max;
 	unsigned int max_touch_num;
 	unsigned int int_trigger_type;
+	struct gpio_desc *gpiod_int;
+	struct gpio_desc *gpiod_rst;
 };
 
+#define GOODIX_GPIO_INT_NAME		"irq"
+#define GOODIX_GPIO_RST_NAME		"reset"
+
 #define GOODIX_MAX_HEIGHT		4096
 #define GOODIX_MAX_WIDTH		4096
 #define GOODIX_INT_TRIGGER		1
@@ -186,6 +192,89 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int goodix_int_sync(struct goodix_ts_data *ts)
+{
+	int ret;
+
+	ret = gpiod_direction_output(ts->gpiod_int, 0);
+	if (ret)
+		return ret;
+
+	return gpiod_direction_input(ts->gpiod_int);
+}
+
+/**
+ * goodix_reset - Reset device during power on
+ *
+ * @ts: goodix_ts_data pointer
+ */
+static int goodix_reset(struct goodix_ts_data *ts)
+{
+	int ret;
+
+	/* begin select I2C slave addr */
+	ret = gpiod_direction_output(ts->gpiod_rst, 0);
+	if (ret)
+		return ret;
+	msleep(20);				/* T2: > 10ms */
+	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
+	ret = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14);
+	if (ret)
+		return ret;
+	usleep_range(100, 2000);		/* T3: > 100us */
+	ret = gpiod_direction_output(ts->gpiod_rst, 1);
+	if (ret)
+		return ret;
+	usleep_range(6000, 10000);		/* T4: > 5ms */
+	/* end select I2C slave addr */
+	ret = gpiod_direction_input(ts->gpiod_rst);
+	if (ret)
+		return ret;
+	ret = goodix_int_sync(ts);
+	if (ret)
+		return ret;
+	msleep(50);				/* T5: 50ms */
+	return 0;
+}
+
+/**
+ * goodix_get_gpio_config - Get GPIO config from ACPI/DT
+ *
+ * @ts: goodix_ts_data pointer
+ */
+static int goodix_get_gpio_config(struct goodix_ts_data *ts)
+{
+	struct device *dev;
+	struct gpio_desc *gpiod;
+	int ret;
+
+	if (!ts->client)
+		return -EINVAL;
+	dev = &ts->client->dev;
+
+	/* Get interrupt GPIO pin number */
+	gpiod = devm_gpiod_get(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN);
+	if (IS_ERR(gpiod)) {
+		ret = PTR_ERR(gpiod);
+		dev_err(dev, "Failed to get %s GPIO: %d\n",
+			GOODIX_GPIO_INT_NAME, ret);
+		return ret;
+	}
+	ts->gpiod_int = gpiod;
+
+	/* Get the reset line GPIO pin number */
+	gpiod = devm_gpiod_get(dev, GOODIX_GPIO_RST_NAME, GPIOD_IN);
+	if (IS_ERR(gpiod)) {
+		ret = PTR_ERR(gpiod);
+		dev_err(dev, "Failed to get %s GPIO: %d\n",
+			GOODIX_GPIO_RST_NAME, ret);
+		return ret;
+	}
+	ts->gpiod_rst = gpiod;
+
+	return 0;
+}
+
 /**
  * goodix_read_config - Read the embedded configuration of the panel
  *
@@ -365,6 +454,17 @@ static int goodix_ts_probe(struct i2c_client *client,
 		return error;
 	}
 
+	error = goodix_get_gpio_config(ts);
+	if (error)
+		return error;
+
+	/* reset the controller */
+	error = goodix_reset(ts);
+	if (error) {
+		dev_err(&client->dev, "Controller reset failed.\n");
+		return error;
+	}
+
 	goodix_read_config(ts);
 
 	error = goodix_request_input_dev(ts, version_info, id_info);
-- 
1.9.1

[PATCH v2 5/8] input: goodix: write configuration data to device

[Irina Tirdea]

Goodix devices can be configured by writing custom data to the device at
init. The configuration data is read with request_firmware from
"goodix_<id>_cfg.bin", where <id> is the product id read from the device
(e.g.: goodix_911_cfg.bin for Goodix GT911, goodix_9271_cfg.bin for
GT9271).

The configuration information has a specific format described in the Goodix
datasheet. It includes X/Y resolution, maximum supported touch points,
interrupt flags, various sesitivity factors and settings for advanced
features (like gesture recognition).

This is based on Goodix datasheets for GT911 and GT9271 and on Goodix
driver gt9xx.c for Android (publicly available in Android kernel
trees for various devices).

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 drivers/input/touchscreen/goodix.c | 128 +++++++++++++++++++++++++++++++++++++
 1 file changed, 128 insertions(+)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index c345eb7..1ce9278 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -24,6 +24,7 @@
 #include <linux/interrupt.h>
 #include <linux/slab.h>
 #include <linux/acpi.h>
+#include <linux/firmware.h>
 #include <linux/gpio.h>
 #include <linux/of.h>
 #include <asm/unaligned.h>
@@ -95,6 +96,39 @@ 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 a register of the i2c slave device.
+ *
+ * @client: i2c device.
+ * @reg: the register to write to.
+ * @buf: raw data buffer to write.
+ * @len: length of the buffer to write
+ */
+static int goodix_i2c_write(struct i2c_client *client, u16 reg, const u8 *buf,
+			    unsigned len)
+{
+	u8 *addr_buf;
+	struct i2c_msg msg;
+	int ret;
+
+	addr_buf = kmalloc(len + 2, GFP_KERNEL);
+	if (!addr_buf)
+		return -ENOMEM;
+
+	addr_buf[0] = reg >> 8;
+	addr_buf[1] = reg & 0xFF;
+	memcpy(&addr_buf[2], buf, len);
+
+	msg.flags = 0;
+	msg.addr = client->addr;
+	msg.buf = addr_buf;
+	msg.len = len + 2;
+
+	ret = i2c_transfer(client->adapter, &msg, 1);
+	kfree(addr_buf);
+	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;
@@ -192,6 +226,95 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/**
+ * goodix_check_cfg - Checks if config buffer is valid
+ *
+ * @ts: goodix_ts_data pointer
+ * @fw: firmware config data
+ */
+static int goodix_check_cfg(struct goodix_ts_data *ts,
+			    const struct firmware *fw)
+{
+	int i, raw_cfg_len;
+	u8 check_sum = 0;
+
+	if (fw->size > GOODIX_CONFIG_MAX_LENGTH) {
+		dev_err(&ts->client->dev,
+			"The length of the config buffer array is not correct");
+		return -EINVAL;
+	}
+
+	raw_cfg_len = fw->size - 2;
+	for (i = 0; i < raw_cfg_len; i++)
+		check_sum += fw->data[i];
+	check_sum = (~check_sum) + 1;
+	if (check_sum != fw->data[raw_cfg_len]) {
+		dev_err(&ts->client->dev,
+			"The checksum of the config buffer array is not correct");
+		return -EINVAL;
+	}
+
+	if (fw->data[raw_cfg_len + 1] != 1) {
+		dev_err(&ts->client->dev,
+			"The Config_Fresh register needs to be set");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/**
+ * goodix_send_cfg - Write device config
+ *
+ * @ts: goodix_ts_data pointer
+ * @id: product id read from device
+ */
+static int goodix_send_cfg(struct goodix_ts_data *ts, u16 id)
+{
+	const struct firmware *fw = NULL;
+	char *fw_name;
+	int ret;
+
+	fw_name = kasprintf(GFP_KERNEL, "goodix_%d_cfg.bin", id);
+	if (!fw_name)
+		return -ENOMEM;
+
+	ret = request_firmware(&fw, fw_name, &ts->client->dev);
+	if (ret) {
+		dev_err(&ts->client->dev, "Unable to open firmware %s\n",
+			fw_name);
+		goto err_free_fw_name;
+	}
+	kfree(fw_name);
+
+	ret = goodix_check_cfg(ts, fw);
+	if (ret)
+		goto err_release_fw;
+
+	ret = goodix_i2c_write(ts->client, GOODIX_REG_CONFIG_DATA, fw->data,
+			       fw->size);
+	if (ret) {
+		dev_err(&ts->client->dev, "Failed to write config data: %d",
+			ret);
+		goto err_release_fw;
+	}
+	dev_dbg(&ts->client->dev, "Config sent successfully.");
+
+	/* Let the firmware reconfigure itself, so sleep for 10ms */
+	usleep_range(10000, 11000);
+
+	release_firmware(fw);
+
+	return 0;
+
+err_release_fw:
+	release_firmware(fw);
+	return ret;
+err_free_fw_name:
+	kfree(fw_name);
+	return ret;
+}
+
 static int goodix_int_sync(struct goodix_ts_data *ts)
 {
 	int ret;
@@ -465,6 +588,11 @@ static int goodix_ts_probe(struct i2c_client *client,
 		return error;
 	}
 
+	/* send device configuration to the firmware */
+	error = goodix_send_cfg(ts, id_info);
+	if (error)
+		return error;
+
 	goodix_read_config(ts);
 
 	error = goodix_request_input_dev(ts, version_info, id_info);
-- 
1.9.1

[PATCH v2 6/8] input: goodix: add power management support

[Irina Tirdea]

Implement suspend/resume for goodix driver.

This is based on Goodix datasheets for GT911 and GT9271
and on Goodix driver gt9xx.c for Android (publicly available
in Android kernel trees for various devices).

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 drivers/input/touchscreen/goodix.c | 88 +++++++++++++++++++++++++++++++++++---
 1 file changed, 83 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 1ce9278..903c83d 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -38,6 +38,7 @@ struct goodix_ts_data {
 	unsigned int int_trigger_type;
 	struct gpio_desc *gpiod_int;
 	struct gpio_desc *gpiod_rst;
+	unsigned long irq_flags;
 };
 
 #define GOODIX_GPIO_INT_NAME		"irq"
@@ -52,6 +53,9 @@ struct goodix_ts_data {
 #define GOODIX_CONFIG_MAX_LENGTH	240
 
 /* Register defines */
+#define GOODIX_REG_COMMAND		0x8040
+#define GOODIX_CMD_SCREEN_OFF		0x05
+
 #define GOODIX_READ_COOR_ADDR		0x814E
 #define GOODIX_REG_CONFIG_DATA		0x8047
 #define GOODIX_REG_ID			0x8140
@@ -129,6 +133,11 @@ static int goodix_i2c_write(struct i2c_client *client, u16 reg, const u8 *buf,
 	return ret < 0 ? ret : (ret != 1 ? -EIO : 0);
 }
 
+static int goodix_i2c_write_u8(struct i2c_client *client, u16 reg, u8 value)
+{
+	return goodix_i2c_write(client, reg, &value, sizeof(value));
+}
+
 static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
 {
 	int touch_num;
@@ -226,6 +235,18 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void goodix_free_irq(struct goodix_ts_data *ts)
+{
+	devm_free_irq(&ts->client->dev, ts->client->irq, ts);
+}
+
+static int goodix_request_irq(struct goodix_ts_data *ts)
+{
+	return devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
+					 NULL, goodix_ts_irq_handler,
+					 ts->irq_flags, ts->client->name, ts);
+}
+
 /**
  * goodix_check_cfg - Checks if config buffer is valid
  *
@@ -547,7 +568,6 @@ static int goodix_ts_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id)
 {
 	struct goodix_ts_data *ts;
-	unsigned long irq_flags;
 	int error;
 	u16 version_info, id_info;
 
@@ -599,10 +619,8 @@ static int goodix_ts_probe(struct i2c_client *client,
 	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);
+	ts->irq_flags = goodix_irq_flags[ts->int_trigger_type] | IRQF_ONESHOT;
+	error = goodix_request_irq(ts);
 	if (error) {
 		dev_err(&client->dev, "request IRQ failed: %d\n", error);
 		return error;
@@ -611,6 +629,65 @@ static int goodix_ts_probe(struct i2c_client *client,
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int goodix_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct goodix_ts_data *ts = i2c_get_clientdata(client);
+	int ret;
+
+	goodix_free_irq(ts);
+	/* Output LOW on the INT pin for 5 ms */
+	ret = gpiod_direction_output(ts->gpiod_int, 0);
+	if (ret) {
+		goodix_request_irq(ts);
+		return ret;
+	}
+	usleep_range(5000, 6000);
+
+	ret = goodix_i2c_write_u8(ts->client, GOODIX_REG_COMMAND,
+				  GOODIX_CMD_SCREEN_OFF);
+	if (ret) {
+		dev_err(&ts->client->dev, "Screen off command failed\n");
+		gpiod_direction_input(ts->gpiod_int);
+		goodix_request_irq(ts);
+		return -EAGAIN;
+	}
+
+	/*
+	 * The datasheet specifies that the interval between sending screen-off
+	 * command and wake-up should be longer than 58 ms. To avoid waking up
+	 * sooner, delay 58ms here.
+	 */
+	msleep(58);
+	return 0;
+}
+
+static int goodix_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct goodix_ts_data *ts = i2c_get_clientdata(client);
+	int ret;
+
+	/*
+	 * Exit sleep mode by outputting HIGH level to INT pin
+	 * for 2ms~5ms.
+	 */
+	ret = gpiod_direction_output(ts->gpiod_int, 1);
+	if (ret)
+		return ret;
+	usleep_range(2000, 5000);
+
+	ret = goodix_int_sync(ts);
+	if (ret)
+		return ret;
+
+	return goodix_request_irq(ts);
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(goodix_pm_ops, goodix_suspend, goodix_resume);
+
 static const struct i2c_device_id goodix_ts_id[] = {
 	{ "GDIX1001:00", 0 },
 	{ }
@@ -646,6 +723,7 @@ static struct i2c_driver goodix_ts_driver = {
 		.owner = THIS_MODULE,
 		.acpi_match_table = ACPI_PTR(goodix_acpi_match),
 		.of_match_table = of_match_ptr(goodix_of_match),
+		.pm = &goodix_pm_ops,
 	},
 };
 module_i2c_driver(goodix_ts_driver);
-- 
1.9.1

[PATCH v2 7/8] input: goodix: use goodix_i2c_write_u8 instead of i2c_master_send

[Irina Tirdea]

Use goodix_i2c_write_u8 instead of i2c_master_send to simplify code.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 drivers/input/touchscreen/goodix.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 903c83d..76004a4 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -220,16 +220,11 @@ 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 >> 8,
-		GOODIX_READ_COOR_ADDR & 0xff,
-		0
-	};
 	struct goodix_ts_data *ts = dev_id;
 
 	goodix_process_events(ts);
 
-	if (i2c_master_send(ts->client, end_cmd, sizeof(end_cmd)) < 0)
+	if (goodix_i2c_write_u8(ts->client, GOODIX_READ_COOR_ADDR, 0) < 0)
 		dev_err(&ts->client->dev, "I2C write end_cmd error\n");
 
 	return IRQ_HANDLED;
-- 
1.9.1

[PATCH v2 8/8] input: goodix: add support for ESD

[Irina Tirdea]

Add ESD (Electrostatic Discharge) protection mechanism.

The driver enables ESD protection in HW and checks a register
to determine if ESD occurred. If ESD is signalled by the HW,
the driver will reset the device.

This is based on Goodix datasheets for GT911 and GT9271 and on Goodix
driver gt9xx.c for Android (publicly available in Android kernel
trees for various devices).

The ESD poll time (in ms) can be set through
esd-recovery-timeout-ms ACPI/DT property. If it is set to 0,
ESD protection is disabled.
For ACPI 5.1, the property can be specified using _DSD properties:

Device (STAC)
{
    Name (_HID, "GDIX1001")
    ...

    Name (_DSD,  Package ()
    {
        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
        Package ()
        {
            Package (2) { "esd-recovery-timeout-ms", Package(1) { 2000 }},
            ...
        }
    }
}

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 .../bindings/input/touchscreen/goodix.txt          |   7 ++
 drivers/input/touchscreen/goodix.c                 | 104 ++++++++++++++++++++-
 2 files changed, 108 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index 7137881..bc94c6f 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -15,6 +15,13 @@ Required properties:
  - irq-gpio		: GPIO pin used for IRQ
  - reset-gpio		: GPIO pin used for reset
 
+Optional properties:
+
+ - esd-recovery-timeout-ms : ESD poll time (in milli seconds) for the driver to
+			     check if ESD occurred and in that case reset the
+			     device. ESD is disabled if this property is not set
+			     or is set to 0.
+
 Example:
 
 	i2c@00000000 {
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 76004a4..a6c5d11 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -39,10 +39,13 @@ struct goodix_ts_data {
 	struct gpio_desc *gpiod_int;
 	struct gpio_desc *gpiod_rst;
 	unsigned long irq_flags;
+	unsigned int esd_timeout;
+	struct delayed_work esd_work;
 };
 
-#define GOODIX_GPIO_INT_NAME		"irq"
-#define GOODIX_GPIO_RST_NAME		"reset"
+#define GOODIX_GPIO_INT_NAME			"irq"
+#define GOODIX_GPIO_RST_NAME			"reset"
+#define GOODIX_DEVICE_ESD_TIMEOUT_PROPERTY	"esd-recovery-timeout-ms"
 
 #define GOODIX_MAX_HEIGHT		4096
 #define GOODIX_MAX_WIDTH		4096
@@ -55,6 +58,8 @@ struct goodix_ts_data {
 /* Register defines */
 #define GOODIX_REG_COMMAND		0x8040
 #define GOODIX_CMD_SCREEN_OFF		0x05
+#define GOODIX_CMD_ESD_ENABLED		0xAA
+#define GOODIX_REG_ESD_CHECK		0x8041
 
 #define GOODIX_READ_COOR_ADDR		0x814E
 #define GOODIX_REG_CONFIG_DATA		0x8047
@@ -376,6 +381,77 @@ static int goodix_reset(struct goodix_ts_data *ts)
 	return 0;
 }
 
+static void goodix_disable_esd(struct goodix_ts_data *ts)
+{
+	if (!ts->esd_timeout)
+		return;
+	cancel_delayed_work_sync(&ts->esd_work);
+}
+
+static int goodix_enable_esd(struct goodix_ts_data *ts)
+{
+	int ret;
+
+	if (!ts->esd_timeout)
+		return 0;
+
+	ret = goodix_i2c_write_u8(ts->client, GOODIX_REG_ESD_CHECK,
+				  GOODIX_CMD_ESD_ENABLED);
+	if (ret) {
+		dev_err(&ts->client->dev, "Failed to enable ESD: %d\n", ret);
+		return ret;
+	}
+
+	schedule_delayed_work(&ts->esd_work, round_jiffies_relative(
+			      msecs_to_jiffies(ts->esd_timeout)));
+	return 0;
+}
+
+static void goodix_esd_work(struct work_struct *work)
+{
+	struct goodix_ts_data *ts = container_of(work, struct goodix_ts_data,
+						 esd_work.work);
+	int retries = 3, ret;
+	u8 esd_data[2];
+
+	while (--retries) {
+		ret = goodix_i2c_read(ts->client, GOODIX_REG_COMMAND, esd_data,
+				      sizeof(esd_data));
+		if (ret)
+			continue;
+		if (esd_data[0] != GOODIX_CMD_ESD_ENABLED &&
+		    esd_data[1] == GOODIX_CMD_ESD_ENABLED) {
+			/* feed the watchdog */
+			goodix_i2c_write_u8(ts->client,
+					    GOODIX_REG_COMMAND,
+					    GOODIX_CMD_ESD_ENABLED);
+			break;
+		}
+	}
+
+	if (!retries) {
+		dev_dbg(&ts->client->dev, "Performing ESD recovery.\n");
+		goodix_free_irq(ts);
+		ret = goodix_reset(ts);
+		if (ret)
+			goto reschedule;
+		ret = goodix_send_cfg(ts, ts->input_dev->id.product);
+		if (ret)
+			goto reschedule;
+		ret = goodix_request_irq(ts);
+		if (ret)
+			goto reschedule;
+		ret = goodix_enable_esd(ts);
+		if (ret)
+			goto reschedule;
+		return;
+	}
+
+reschedule:
+	schedule_delayed_work(&ts->esd_work, round_jiffies_relative(
+			      msecs_to_jiffies(ts->esd_timeout)));
+}
+
 /**
  * goodix_get_gpio_config - Get GPIO config from ACPI/DT
  *
@@ -579,6 +655,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 
 	ts->client = client;
 	i2c_set_clientdata(client, ts);
+	INIT_DELAYED_WORK(&ts->esd_work, goodix_esd_work);
 
 	error = goodix_i2c_test(client);
 	if (error) {
@@ -621,6 +698,21 @@ static int goodix_ts_probe(struct i2c_client *client,
 		return error;
 	}
 
+	error = device_property_read_u32(&ts->client->dev,
+					 GOODIX_DEVICE_ESD_TIMEOUT_PROPERTY,
+					 &ts->esd_timeout);
+	if (error < 0)
+		dev_err(&ts->client->dev, "No %s property. Will not use ESD.\n",
+			GOODIX_DEVICE_ESD_TIMEOUT_PROPERTY);
+
+	return goodix_enable_esd(ts);
+}
+
+static int goodix_ts_remove(struct i2c_client *client)
+{
+	struct goodix_ts_data *ts = i2c_get_clientdata(client);
+
+	goodix_disable_esd(ts);
 	return 0;
 }
 
@@ -631,6 +723,7 @@ static int goodix_suspend(struct device *dev)
 	struct goodix_ts_data *ts = i2c_get_clientdata(client);
 	int ret;
 
+	goodix_disable_esd(ts);
 	goodix_free_irq(ts);
 	/* Output LOW on the INT pin for 5 ms */
 	ret = gpiod_direction_output(ts->gpiod_int, 0);
@@ -677,7 +770,11 @@ static int goodix_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	return goodix_request_irq(ts);
+	ret = goodix_request_irq(ts);
+	if (ret)
+		return ret;
+
+	return goodix_enable_esd(ts);
 }
 #endif
 
@@ -712,6 +809,7 @@ MODULE_DEVICE_TABLE(of, goodix_of_match);
 
 static struct i2c_driver goodix_ts_driver = {
 	.probe = goodix_ts_probe,
+	.remove = goodix_ts_remove,
 	.id_table = goodix_ts_id,
 	.driver = {
 		.name = "Goodix-TS",
-- 
1.9.1

Re: [PATCH v2 4/8] input: goodix: reset device at init

[Bastien Nocera]

On Mon, 2015-06-08 at 17:37 +0300, Irina Tirdea wrote:
> After power on, it is recommended that the driver resets the device.
> The reset procedure timing is described in the datasheet and is used
> at device init (before writing device configuration) and
> for power management. It is a sequence of setting the interrupt
> and reset pins high/low at specific timing intervals. This procedure
> also includes setting the slave address to the one specified in the
> ACPI/device tree.

This breaks the touchscreen on my Onda v975w:
[  239.732858] Goodix-TS i2c-GDIX1001:00: ID 9271, version: 1020
[  239.732977] Goodix-TS i2c-GDIX1001:00: Failed to get reset GPIO: -16
[  239.736071] Goodix-TS: probe of i2c-GDIX1001:00 failed with error -16

This is the DSDT for that device:
https://bugzilla.kernel.org/attachment.cgi?id=149331

Re: [PATCH v2 2/8] input: goodix: fix variable length array warning

[Bastien Nocera]

On Mon, 2015-06-08 at 17:37 +0300, Irina Tirdea wrote:
> Fix sparse warning:
> drivers/input/touchscreen/goodix.c:182:26: warning:
> Variable length array is used.
> 
> Replace the variable length array with fixed length.
> 
> Some Goodix devices have maximum 5 touch points, while
> others have 10 touch points. Using the maximum length
> (80 bytes) for all devices will lead to wasting 40 bytes
> when using devices with maximum 5 touch points. However,
> that is prefferable to using kmalloc which will use even
          ^^^^^^^^^^^
preferable

> more resources.
> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>

Acked-by: Bastien Nocera <hadess@hadess.net>

> ---
>  drivers/input/touchscreen/goodix.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c 
> b/drivers/input/touchscreen/goodix.c
> index a65e4d9..18557e4 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -147,7 +147,7 @@ static void goodix_ts_report_touch(struct 
> goodix_ts_data *ts, u8 *coor_data)
>   */
>  static void goodix_process_events(struct goodix_ts_data *ts)
>  {
> -     u8  point_data[1 + GOODIX_CONTACT_SIZE * ts->max_touch_num];
> +     u8  point_data[1 + GOODIX_CONTACT_SIZE * 
> GOODIX_MAX_CONTACTS];
>       int touch_num;
>       int i;
>  

Re: [PATCH v2 3/8] input: goodix: export id and version read from device

[Bastien Nocera]

On Mon, 2015-06-08 at 17:37 +0300, Irina Tirdea wrote:
> Goodix touchscreens export through their registers a Product ID
> and Firmware Version. The Product ID is an ASCII encoding of the
> product name (e.g.: "911").
> 
> Export to sysfs (through the input subsystem) the product id and
> firmware version read from the device rather than using constant
> values.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>

Acked-by: Bastien Nocera <hadess@hadess.net>

Re: [PATCH v2 4/8] input: goodix: reset device at init

[Bastien Nocera]

On Tue, 2015-06-09 at 17:34 +0200, Bastien Nocera wrote:
> On Mon, 2015-06-08 at 17:37 +0300, Irina Tirdea wrote:
> > After power on, it is recommended that the driver resets the 
> > device.
> > The reset procedure timing is described in the datasheet and is 
> > used
> > at device init (before writing device configuration) and
> > for power management. It is a sequence of setting the interrupt
> > and reset pins high/low at specific timing intervals. This 
> > procedure
> > also includes setting the slave address to the one specified in the
> > ACPI/device tree.
> 
> This breaks the touchscreen on my Onda v975w:
> [  239.732858] Goodix-TS i2c-GDIX1001:00: ID 9271, version: 1020
> [  239.732977] Goodix-TS i2c-GDIX1001:00: Failed to get reset GPIO: 
> -16
> [  239.736071] Goodix-TS: probe of i2c-GDIX1001:00 failed with error 
> -16
> 
> This is the DSDT for that device:
> https://bugzilla.kernel.org/attachment.cgi?id=149331

(Note that this means that I haven't been able to test any following
patches in that series than 4/8).

Re: [PATCH v2 4/8] input: goodix: reset device at init

[Dmitry Torokhov]

Hi Irina,

On Mon, Jun 08, 2015 at 05:37:49PM +0300, Irina Tirdea wrote:
> +static int goodix_get_gpio_config(struct goodix_ts_data *ts)
> +{
> +	struct device *dev;
> +	struct gpio_desc *gpiod;
> +	int ret;
> +
> +	if (!ts->client)
> +		return -EINVAL;
> +	dev = &ts->client->dev;
> +
> +	/* Get interrupt GPIO pin number */
> +	gpiod = devm_gpiod_get(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN);
> +	if (IS_ERR(gpiod)) {
> +		ret = PTR_ERR(gpiod);
> +		dev_err(dev, "Failed to get %s GPIO: %d\n",
> +			GOODIX_GPIO_INT_NAME, ret);

You need to handle -EPROBE_DEFER (suppress error) and especially -ENOENT error
codes, otherwise, as Bastien mentioned, you will break existing DTS.

Thanks.

-- 
Dmitry

Re: [PATCH v2 5/8] input: goodix: write configuration data to device

[Dmitry Torokhov]

Hi Irina,

On Mon, Jun 08, 2015 at 05:37:50PM +0300, Irina Tirdea wrote:
> Goodix devices can be configured by writing custom data to the device at
> init. The configuration data is read with request_firmware from
> "goodix_<id>_cfg.bin", where <id> is the product id read from the device
> (e.g.: goodix_911_cfg.bin for Goodix GT911, goodix_9271_cfg.bin for
> GT9271).
> 
> The configuration information has a specific format described in the Goodix
> datasheet. It includes X/Y resolution, maximum supported touch points,
> interrupt flags, various sesitivity factors and settings for advanced
> features (like gesture recognition).
> 
> This is based on Goodix datasheets for GT911 and GT9271 and on Goodix
> driver gt9xx.c for Android (publicly available in Android kernel
> trees for various devices).

I am afraid that just using request_firmware() in probe() is not the
best option as it may cause either errors or delays in
initialization of the driver is compiled into the kernel and tries to
initialize before filesystem with configuration file is mounted (and
firmware is not built into the kernel). We can either try switch to
request_firmware_nowait() or at least document that we need firmware at
boot.

> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> ---
>  drivers/input/touchscreen/goodix.c | 128 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 128 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index c345eb7..1ce9278 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -24,6 +24,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/slab.h>
>  #include <linux/acpi.h>
> +#include <linux/firmware.h>
>  #include <linux/gpio.h>
>  #include <linux/of.h>
>  #include <asm/unaligned.h>
> @@ -95,6 +96,39 @@ 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 a register of the i2c slave device.
> + *
> + * @client: i2c device.
> + * @reg: the register to write to.
> + * @buf: raw data buffer to write.
> + * @len: length of the buffer to write
> + */
> +static int goodix_i2c_write(struct i2c_client *client, u16 reg, const u8 *buf,
> +			    unsigned len)
> +{
> +	u8 *addr_buf;
> +	struct i2c_msg msg;
> +	int ret;
> +
> +	addr_buf = kmalloc(len + 2, GFP_KERNEL);
> +	if (!addr_buf)
> +		return -ENOMEM;
> +
> +	addr_buf[0] = reg >> 8;
> +	addr_buf[1] = reg & 0xFF;
> +	memcpy(&addr_buf[2], buf, len);
> +
> +	msg.flags = 0;
> +	msg.addr = client->addr;
> +	msg.buf = addr_buf;
> +	msg.len = len + 2;
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	kfree(addr_buf);
> +	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;
> @@ -192,6 +226,95 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +/**
> + * goodix_check_cfg - Checks if config buffer is valid
> + *
> + * @ts: goodix_ts_data pointer
> + * @fw: firmware config data
> + */
> +static int goodix_check_cfg(struct goodix_ts_data *ts,
> +			    const struct firmware *fw)
> +{
> +	int i, raw_cfg_len;
> +	u8 check_sum = 0;
> +
> +	if (fw->size > GOODIX_CONFIG_MAX_LENGTH) {
> +		dev_err(&ts->client->dev,
> +			"The length of the config buffer array is not correct");
> +		return -EINVAL;
> +	}
> +
> +	raw_cfg_len = fw->size - 2;
> +	for (i = 0; i < raw_cfg_len; i++)
> +		check_sum += fw->data[i];
> +	check_sum = (~check_sum) + 1;
> +	if (check_sum != fw->data[raw_cfg_len]) {
> +		dev_err(&ts->client->dev,
> +			"The checksum of the config buffer array is not correct");
> +		return -EINVAL;
> +	}
> +
> +	if (fw->data[raw_cfg_len + 1] != 1) {
> +		dev_err(&ts->client->dev,
> +			"The Config_Fresh register needs to be set");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * goodix_send_cfg - Write device config
> + *
> + * @ts: goodix_ts_data pointer
> + * @id: product id read from device
> + */
> +static int goodix_send_cfg(struct goodix_ts_data *ts, u16 id)
> +{
> +	const struct firmware *fw = NULL;
> +	char *fw_name;
> +	int ret;
> +
> +	fw_name = kasprintf(GFP_KERNEL, "goodix_%d_cfg.bin", id);
> +	if (!fw_name)
> +		return -ENOMEM;
> +
> +	ret = request_firmware(&fw, fw_name, &ts->client->dev);
> +	if (ret) {
> +		dev_err(&ts->client->dev, "Unable to open firmware %s\n",
> +			fw_name);
> +		goto err_free_fw_name;

That I think will cause driver to abort binding if config is not there.
Do we always need to load the config? Is configuration stored in NVRAM?
Maybe configuration should be only loaded when userspace requests it?

Thanks.

-- 
Dmitry

Re: [PATCH v2 6/8] input: goodix: add power management support

[Dmitry Torokhov]

On Mon, Jun 08, 2015 at 05:37:51PM +0300, Irina Tirdea wrote:
> Implement suspend/resume for goodix driver.
> 
> This is based on Goodix datasheets for GT911 and GT9271
> and on Goodix driver gt9xx.c for Android (publicly available
> in Android kernel trees for various devices).
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> ---
>  drivers/input/touchscreen/goodix.c | 88 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 83 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index 1ce9278..903c83d 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -38,6 +38,7 @@ struct goodix_ts_data {
>  	unsigned int int_trigger_type;
>  	struct gpio_desc *gpiod_int;
>  	struct gpio_desc *gpiod_rst;
> +	unsigned long irq_flags;
>  };
>  
>  #define GOODIX_GPIO_INT_NAME		"irq"
> @@ -52,6 +53,9 @@ struct goodix_ts_data {
>  #define GOODIX_CONFIG_MAX_LENGTH	240
>  
>  /* Register defines */
> +#define GOODIX_REG_COMMAND		0x8040
> +#define GOODIX_CMD_SCREEN_OFF		0x05
> +
>  #define GOODIX_READ_COOR_ADDR		0x814E
>  #define GOODIX_REG_CONFIG_DATA		0x8047
>  #define GOODIX_REG_ID			0x8140
> @@ -129,6 +133,11 @@ static int goodix_i2c_write(struct i2c_client *client, u16 reg, const u8 *buf,
>  	return ret < 0 ? ret : (ret != 1 ? -EIO : 0);
>  }
>  
> +static int goodix_i2c_write_u8(struct i2c_client *client, u16 reg, u8 value)
> +{
> +	return goodix_i2c_write(client, reg, &value, sizeof(value));
> +}
> +
>  static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
>  {
>  	int touch_num;
> @@ -226,6 +235,18 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static void goodix_free_irq(struct goodix_ts_data *ts)
> +{
> +	devm_free_irq(&ts->client->dev, ts->client->irq, ts);
> +}
> +
> +static int goodix_request_irq(struct goodix_ts_data *ts)
> +{
> +	return devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
> +					 NULL, goodix_ts_irq_handler,
> +					 ts->irq_flags, ts->client->name, ts);
> +}
> +
>  /**
>   * goodix_check_cfg - Checks if config buffer is valid
>   *
> @@ -547,7 +568,6 @@ static int goodix_ts_probe(struct i2c_client *client,
>  			   const struct i2c_device_id *id)
>  {
>  	struct goodix_ts_data *ts;
> -	unsigned long irq_flags;
>  	int error;
>  	u16 version_info, id_info;
>  
> @@ -599,10 +619,8 @@ static int goodix_ts_probe(struct i2c_client *client,
>  	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);
> +	ts->irq_flags = goodix_irq_flags[ts->int_trigger_type] | IRQF_ONESHOT;
> +	error = goodix_request_irq(ts);
>  	if (error) {
>  		dev_err(&client->dev, "request IRQ failed: %d\n", error);
>  		return error;
> @@ -611,6 +629,65 @@ static int goodix_ts_probe(struct i2c_client *client,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int goodix_suspend(struct device *dev)

Please drop #ifdef CONFIG_PM_SLEEP and annotate with __maybe_unused
instead.

> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct goodix_ts_data *ts = i2c_get_clientdata(client);
> +	int ret;
> +
> +	goodix_free_irq(ts);

Why do we need to free IRQ here?

> +	/* Output LOW on the INT pin for 5 ms */
> +	ret = gpiod_direction_output(ts->gpiod_int, 0);
> +	if (ret) {
> +		goodix_request_irq(ts);
> +		return ret;
> +	}
> +	usleep_range(5000, 6000);
> +
> +	ret = goodix_i2c_write_u8(ts->client, GOODIX_REG_COMMAND,
> +				  GOODIX_CMD_SCREEN_OFF);
> +	if (ret) {
> +		dev_err(&ts->client->dev, "Screen off command failed\n");
> +		gpiod_direction_input(ts->gpiod_int);
> +		goodix_request_irq(ts);
> +		return -EAGAIN;
> +	}
> +
> +	/*
> +	 * The datasheet specifies that the interval between sending screen-off
> +	 * command and wake-up should be longer than 58 ms. To avoid waking up
> +	 * sooner, delay 58ms here.
> +	 */
> +	msleep(58);
> +	return 0;
> +}
> +
> +static int goodix_resume(struct device *dev)

__maybe_unused

> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct goodix_ts_data *ts = i2c_get_clientdata(client);
> +	int ret;
> +
> +	/*
> +	 * Exit sleep mode by outputting HIGH level to INT pin
> +	 * for 2ms~5ms.
> +	 */
> +	ret = gpiod_direction_output(ts->gpiod_int, 1);
> +	if (ret)
> +		return ret;
> +	usleep_range(2000, 5000);
> +
> +	ret = goodix_int_sync(ts);
> +	if (ret)
> +		return ret;
> +
> +	return goodix_request_irq(ts);
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(goodix_pm_ops, goodix_suspend, goodix_resume);
> +
>  static const struct i2c_device_id goodix_ts_id[] = {
>  	{ "GDIX1001:00", 0 },
>  	{ }
> @@ -646,6 +723,7 @@ static struct i2c_driver goodix_ts_driver = {
>  		.owner = THIS_MODULE,
>  		.acpi_match_table = ACPI_PTR(goodix_acpi_match),
>  		.of_match_table = of_match_ptr(goodix_of_match),
> +		.pm = &goodix_pm_ops,
>  	},
>  };
>  module_i2c_driver(goodix_ts_driver);
> -- 
> 1.9.1
> 

Thanks.

-- 
Dmitry

Re: [PATCH v2 0/8] Goodix touchscreen enhancements

[Dmitry Torokhov]

On Mon, Jun 08, 2015 at 05:37:45PM +0300, Irina Tirdea wrote:
> Add several enhancements to the Goodix touchscreen driver:
>  - write configuration data to the device
>  - power management support
>  - cleanup and refactoring
> 
> I have kept the original patch for ESD ("input: goodix: add support
> for ESD") since it is still under discussion. I have moved it to the
> end of the patch set so it does not block the other changes. Once I have
> an answer from DT folks on this I will make the necessary changes for it.
> 
> For testing the ACPI changes "ACPI: Translate Linux IRQ number directly
> from GpioInt" patchset is needed [1]. This patchset has been merged in
> the GPIO tree.
> 
> This patches are based on Goodix datasheets for GT911 and GT9271 and
> on Goodix driver gt9xx.c for Android (publicly available in Android kernel
> trees for various devices). Commit 771d8f1b178e ("Input: goodix - add
> device tree support") references a set of public datasheets that can
> be used for reference.
> 
> [1] https://lkml.org/lkml/2015/5/6/281
> 
> Changes in v2:
>  - use request_firmware instead of ACPI/DT property for config
>  - dropped "input: goodix: add ACPI IDs for GT911 and GT9271" patch
>  - add ACPI DSDT excerpt in commit message where necessary
>  - add comments for suspend/resume sleep values
>  - dropped the checkpatch fixes that did not make sense
>  - added Bastien's ack to the first patch
> 
> Irina Tirdea (8):
>   input: goodix: fix alignment issues
>   input: goodix: fix variable length array warning
>   input: goodix: export id and version read from device

Applied first 3.

Thanks.

>   input: goodix: reset device at init
>   input: goodix: write configuration data to device
>   input: goodix: add power management support
>   input: goodix: use goodix_i2c_write_u8 instead of i2c_master_send
>   input: goodix: add support for ESD
> 
>  .../bindings/input/touchscreen/goodix.txt          |  12 +
>  drivers/input/touchscreen/goodix.c                 | 467 +++++++++++++++++++--
>  2 files changed, 449 insertions(+), 30 deletions(-)
> 
> -- 
> 1.9.1
> 

-- 
Dmitry

RE: [PATCH v2 0/8] Goodix touchscreen enhancements

[Tirdea, Irina]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: 09 June, 2015 21:17
> To: Tirdea, Irina
> Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Rob
> Herring; Pawel Moll; Ian Campbell; Kumar Gala
> Subject: Re: [PATCH v2 0/8] Goodix touchscreen enhancements
> 
> On Mon, Jun 08, 2015 at 05:37:45PM +0300, Irina Tirdea wrote:
> > Add several enhancements to the Goodix touchscreen driver:
> >  - write configuration data to the device
> >  - power management support
> >  - cleanup and refactoring
> >
> > I have kept the original patch for ESD ("input: goodix: add support
> > for ESD") since it is still under discussion. I have moved it to the
> > end of the patch set so it does not block the other changes. Once I have
> > an answer from DT folks on this I will make the necessary changes for it.
> >
> > For testing the ACPI changes "ACPI: Translate Linux IRQ number directly
> > from GpioInt" patchset is needed [1]. This patchset has been merged in
> > the GPIO tree.
> >
> > This patches are based on Goodix datasheets for GT911 and GT9271 and
> > on Goodix driver gt9xx.c for Android (publicly available in Android kernel
> > trees for various devices). Commit 771d8f1b178e ("Input: goodix - add
> > device tree support") references a set of public datasheets that can
> > be used for reference.
> >
> > [1] https://lkml.org/lkml/2015/5/6/281
> >
> > Changes in v2:
> >  - use request_firmware instead of ACPI/DT property for config
> >  - dropped "input: goodix: add ACPI IDs for GT911 and GT9271" patch
> >  - add ACPI DSDT excerpt in commit message where necessary
> >  - add comments for suspend/resume sleep values
> >  - dropped the checkpatch fixes that did not make sense
> >  - added Bastien's ack to the first patch
> >
> > Irina Tirdea (8):
> >   input: goodix: fix alignment issues
> >   input: goodix: fix variable length array warning
> >   input: goodix: export id and version read from device
> 
> Applied first 3.
> 
> Thanks.
> 

Thanks Dmitry and Bastien for taking a look at the patches!
Sorry for the delay  - I have been on vacation for the last 2 weeks.
I am back in the office now so I will quickly address the issues for the remaining patches.

Thanks,
Irina

> >   input: goodix: reset device at init
> >   input: goodix: write configuration data to device
> >   input: goodix: add power management support
> >   input: goodix: use goodix_i2c_write_u8 instead of i2c_master_send
> >   input: goodix: add support for ESD
> >
> >  .../bindings/input/touchscreen/goodix.txt          |  12 +
> >  drivers/input/touchscreen/goodix.c                 | 467 +++++++++++++++++++--
> >  2 files changed, 449 insertions(+), 30 deletions(-)
> >
> > --
> > 1.9.1
> >
> 
> --
> Dmitry

RE: [PATCH v2 4/8] input: goodix: reset device at init

[Tirdea, Irina]

> -----Original Message-----
> From: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@vger.kernel.org] On Behalf Of Dmitry Torokhov
> Sent: 09 June, 2015 20:58
> To: Tirdea, Irina
> Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Rob
> Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian
> Subject: Re: [PATCH v2 4/8] input: goodix: reset device at init
> 
> Hi Irina,
> 
> On Mon, Jun 08, 2015 at 05:37:49PM +0300, Irina Tirdea wrote:
> > +static int goodix_get_gpio_config(struct goodix_ts_data *ts)
> > +{
> > +	struct device *dev;
> > +	struct gpio_desc *gpiod;
> > +	int ret;
> > +
> > +	if (!ts->client)
> > +		return -EINVAL;
> > +	dev = &ts->client->dev;
> > +
> > +	/* Get interrupt GPIO pin number */
> > +	gpiod = devm_gpiod_get(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN);
> > +	if (IS_ERR(gpiod)) {
> > +		ret = PTR_ERR(gpiod);
> > +		dev_err(dev, "Failed to get %s GPIO: %d\n",
> > +			GOODIX_GPIO_INT_NAME, ret);
> 
> You need to handle -EPROBE_DEFER (suppress error) and especially -ENOENT error
> codes, otherwise, as Bastien mentioned, you will break existing DTS.
> 

Yes, I will handle the -EPROBE_DEFER and -ENOENT for devices that might not have these
gpio pins connected or declared in ACPI/DTS. Since the following patches depend on these
pins, I will bypass the functionality for these devices (suspend/resume, esd, writing config
will not be available if gpio pins are not found).

Thanks,
Irina

> Thanks.
> 
> --
> Dmitry
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 4/8] input: goodix: reset device at init

[Tirdea, Irina]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3594 bytes --]



> -----Original Message-----
> From: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@vger.kernel.org] On Behalf Of Bastien Nocera
> Sent: 09 June, 2015 18:53
> To: Tirdea, Irina
> Cc: Dmitry Torokhov; Mark Rutland; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Rob
> Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian
> Subject: Re: [PATCH v2 4/8] input: goodix: reset device at init
> 
> On Tue, 2015-06-09 at 17:34 +0200, Bastien Nocera wrote:
> > On Mon, 2015-06-08 at 17:37 +0300, Irina Tirdea wrote:
> > > After power on, it is recommended that the driver resets the
> > > device.
> > > The reset procedure timing is described in the datasheet and is
> > > used
> > > at device init (before writing device configuration) and
> > > for power management. It is a sequence of setting the interrupt
> > > and reset pins high/low at specific timing intervals. This
> > > procedure
> > > also includes setting the slave address to the one specified in the
> > > ACPI/device tree.
> >
> > This breaks the touchscreen on my Onda v975w:
> > [  239.732858] Goodix-TS i2c-GDIX1001:00: ID 9271, version: 1020
> > [  239.732977] Goodix-TS i2c-GDIX1001:00: Failed to get reset GPIO:
> > -16
> > [  239.736071] Goodix-TS: probe of i2c-GDIX1001:00 failed with error
> > -16
> >
> > This is the DSDT for that device:
> > https://bugzilla.kernel.org/attachment.cgi?id=149331
> 

Oops. That's right - I am using named interrupts which are available only for ACPI 5.1, so 
devices already out there will not work.

The problem here is that handling -ENOENT will not help. The gpio pins are declared in the
ACPI DSDT, but are not named. The devm_gpiod_get API will look for the named interrupt
first and fallback to searching by index if not found. Index will be 0 in both cases, this is why
the first call will succeed and the second will fail with -EBUSY.

One way to handle this would be to use indexed gpio pins instead of named gpio pins:
e.g. consider the first gpio pin to be the reset pin and the second to be the interrupt pin.
This is used in other drivers as well, e.g. zforce_ts. The problem with this approach is that
any devices that declare the gpio pins in reversed order in the DSDT will not work and give
no warning (the pins will be requested successfully, but some of the functionality will not
work). The DSDT mentioned in https://bugzilla.kernel.org/attachment.cgi?id=149331 lists
the reset pin first, while I am working on some devices that declare the interrupt gpio pin
first.

Another way to handle this is to treat -EBUSY like -ENOENT and not use any functionality
that depends on the gpio pins. Unfortunately, this means we will not have suspend, esd and
custom configs even if the pins are connected on the board and available in ACPI(just not
named).

I would go with the first approach and document the order requested for the pins, but I would
like to hear what you think first. Is there a better way to do this?

> (Note that this means that I haven't been able to test any following
> patches in that series than 4/8).

Sure, that makes sense. The following patches depend on the gpio pins so they would not have
worked either.

Thanks,
Irina

> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH v2 5/8] input: goodix: write configuration data to device

[Tirdea, Irina]

> -----Original Message-----
> From: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@vger.kernel.org] On Behalf Of Dmitry Torokhov
> Sent: 09 June, 2015 21:14
> To: Tirdea, Irina
> Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Rob
> Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian
> Subject: Re: [PATCH v2 5/8] input: goodix: write configuration data to device
> 
> Hi Irina,
> 

Hi Dmitry,

> On Mon, Jun 08, 2015 at 05:37:50PM +0300, Irina Tirdea wrote:
> > Goodix devices can be configured by writing custom data to the device at
> > init. The configuration data is read with request_firmware from
> > "goodix_<id>_cfg.bin", where <id> is the product id read from the device
> > (e.g.: goodix_911_cfg.bin for Goodix GT911, goodix_9271_cfg.bin for
> > GT9271).
> >
> > The configuration information has a specific format described in the Goodix
> > datasheet. It includes X/Y resolution, maximum supported touch points,
> > interrupt flags, various sesitivity factors and settings for advanced
> > features (like gesture recognition).
> >
> > This is based on Goodix datasheets for GT911 and GT9271 and on Goodix
> > driver gt9xx.c for Android (publicly available in Android kernel
> > trees for various devices).
> 
> I am afraid that just using request_firmware() in probe() is not the
> best option as it may cause either errors or delays in
> initialization of the driver is compiled into the kernel and tries to
> initialize before filesystem with configuration file is mounted (and
> firmware is not built into the kernel). We can either try switch to
> request_firmware_nowait() or at least document that we need firmware at
> boot.
> 

The reason I did not use request_firmware_nowait() is that this configuration data
includes information needed by probe (x/y resolution, interrupt trigger type, max
touch num), used in goodix_read_config, goodix_ts_report_touch  and for irq flags
when requesting the interrupt. I will document that we need this configuration
firmware at boot in the commit message and add a comment in the code.
Is there any other documentation I need to update?

> >
> > Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > ---
> >  drivers/input/touchscreen/goodix.c | 128 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 128 insertions(+)
> >
> > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> > index c345eb7..1ce9278 100644
> > --- a/drivers/input/touchscreen/goodix.c
> > +++ b/drivers/input/touchscreen/goodix.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/interrupt.h>
> >  #include <linux/slab.h>
> >  #include <linux/acpi.h>
> > +#include <linux/firmware.h>
> >  #include <linux/gpio.h>
> >  #include <linux/of.h>
> >  #include <asm/unaligned.h>
> > @@ -95,6 +96,39 @@ 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 a register of the i2c slave device.
> > + *
> > + * @client: i2c device.
> > + * @reg: the register to write to.
> > + * @buf: raw data buffer to write.
> > + * @len: length of the buffer to write
> > + */
> > +static int goodix_i2c_write(struct i2c_client *client, u16 reg, const u8 *buf,
> > +			    unsigned len)
> > +{
> > +	u8 *addr_buf;
> > +	struct i2c_msg msg;
> > +	int ret;
> > +
> > +	addr_buf = kmalloc(len + 2, GFP_KERNEL);
> > +	if (!addr_buf)
> > +		return -ENOMEM;
> > +
> > +	addr_buf[0] = reg >> 8;
> > +	addr_buf[1] = reg & 0xFF;
> > +	memcpy(&addr_buf[2], buf, len);
> > +
> > +	msg.flags = 0;
> > +	msg.addr = client->addr;
> > +	msg.buf = addr_buf;
> > +	msg.len = len + 2;
> > +
> > +	ret = i2c_transfer(client->adapter, &msg, 1);
> > +	kfree(addr_buf);
> > +	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;
> > @@ -192,6 +226,95 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
> >  	return IRQ_HANDLED;
> >  }
> >
> > +/**
> > + * goodix_check_cfg - Checks if config buffer is valid
> > + *
> > + * @ts: goodix_ts_data pointer
> > + * @fw: firmware config data
> > + */
> > +static int goodix_check_cfg(struct goodix_ts_data *ts,
> > +			    const struct firmware *fw)
> > +{
> > +	int i, raw_cfg_len;
> > +	u8 check_sum = 0;
> > +
> > +	if (fw->size > GOODIX_CONFIG_MAX_LENGTH) {
> > +		dev_err(&ts->client->dev,
> > +			"The length of the config buffer array is not correct");
> > +		return -EINVAL;
> > +	}
> > +
> > +	raw_cfg_len = fw->size - 2;
> > +	for (i = 0; i < raw_cfg_len; i++)
> > +		check_sum += fw->data[i];
> > +	check_sum = (~check_sum) + 1;
> > +	if (check_sum != fw->data[raw_cfg_len]) {
> > +		dev_err(&ts->client->dev,
> > +			"The checksum of the config buffer array is not correct");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (fw->data[raw_cfg_len + 1] != 1) {
> > +		dev_err(&ts->client->dev,
> > +			"The Config_Fresh register needs to be set");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * goodix_send_cfg - Write device config
> > + *
> > + * @ts: goodix_ts_data pointer
> > + * @id: product id read from device
> > + */
> > +static int goodix_send_cfg(struct goodix_ts_data *ts, u16 id)
> > +{
> > +	const struct firmware *fw = NULL;
> > +	char *fw_name;
> > +	int ret;
> > +
> > +	fw_name = kasprintf(GFP_KERNEL, "goodix_%d_cfg.bin", id);
> > +	if (!fw_name)
> > +		return -ENOMEM;
> > +
> > +	ret = request_firmware(&fw, fw_name, &ts->client->dev);
> > +	if (ret) {
> > +		dev_err(&ts->client->dev, "Unable to open firmware %s\n",
> > +			fw_name);
> > +		goto err_free_fw_name;
> 
> That I think will cause driver to abort binding if config is not there.
> Do we always need to load the config? Is configuration stored in NVRAM?
> Maybe configuration should be only loaded when userspace requests it?
> 

The device has a default configuration, but usually this does not match the platform
needs. It is stored in volatile RAM, so we need to rewrite it after power off.
>From my tests, the default values are invalid if I do not write any config data
(goodix_read_config will print "Invalid config, using defaults").

However, we should still be able to use the device with these defaults even
if the config firmware is not there. I will just return 0 if firmware is not found.
Since configuration needs to be written before configuring the input device and
the interrupts, I don't think we can allow userspace to control it.

Thanks,
Irina

> Thanks.
> 
> --
> Dmitry
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 6/8] input: goodix: add power management support

[Tirdea, Irina]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: 09 June, 2015 21:16
> To: Tirdea, Irina
> Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Rob
> Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian
> Subject: Re: [PATCH v2 6/8] input: goodix: add power management support
> 
> On Mon, Jun 08, 2015 at 05:37:51PM +0300, Irina Tirdea wrote:
> > Implement suspend/resume for goodix driver.
> >
> > This is based on Goodix datasheets for GT911 and GT9271
> > and on Goodix driver gt9xx.c for Android (publicly available
> > in Android kernel trees for various devices).
> >
> > Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > ---
> >  drivers/input/touchscreen/goodix.c | 88 +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 83 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> > index 1ce9278..903c83d 100644
> > --- a/drivers/input/touchscreen/goodix.c
> > +++ b/drivers/input/touchscreen/goodix.c
> > @@ -38,6 +38,7 @@ struct goodix_ts_data {
> >  	unsigned int int_trigger_type;
> >  	struct gpio_desc *gpiod_int;
> >  	struct gpio_desc *gpiod_rst;
> > +	unsigned long irq_flags;
> >  };
> >
> >  #define GOODIX_GPIO_INT_NAME		"irq"
> > @@ -52,6 +53,9 @@ struct goodix_ts_data {
> >  #define GOODIX_CONFIG_MAX_LENGTH	240
> >
> >  /* Register defines */
> > +#define GOODIX_REG_COMMAND		0x8040
> > +#define GOODIX_CMD_SCREEN_OFF		0x05
> > +
> >  #define GOODIX_READ_COOR_ADDR		0x814E
> >  #define GOODIX_REG_CONFIG_DATA		0x8047
> >  #define GOODIX_REG_ID			0x8140
> > @@ -129,6 +133,11 @@ static int goodix_i2c_write(struct i2c_client *client, u16 reg, const u8 *buf,
> >  	return ret < 0 ? ret : (ret != 1 ? -EIO : 0);
> >  }
> >
> > +static int goodix_i2c_write_u8(struct i2c_client *client, u16 reg, u8 value)
> > +{
> > +	return goodix_i2c_write(client, reg, &value, sizeof(value));
> > +}
> > +
> >  static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
> >  {
> >  	int touch_num;
> > @@ -226,6 +235,18 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
> >  	return IRQ_HANDLED;
> >  }
> >
> > +static void goodix_free_irq(struct goodix_ts_data *ts)
> > +{
> > +	devm_free_irq(&ts->client->dev, ts->client->irq, ts);
> > +}
> > +
> > +static int goodix_request_irq(struct goodix_ts_data *ts)
> > +{
> > +	return devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
> > +					 NULL, goodix_ts_irq_handler,
> > +					 ts->irq_flags, ts->client->name, ts);
> > +}
> > +
> >  /**
> >   * goodix_check_cfg - Checks if config buffer is valid
> >   *
> > @@ -547,7 +568,6 @@ static int goodix_ts_probe(struct i2c_client *client,
> >  			   const struct i2c_device_id *id)
> >  {
> >  	struct goodix_ts_data *ts;
> > -	unsigned long irq_flags;
> >  	int error;
> >  	u16 version_info, id_info;
> >
> > @@ -599,10 +619,8 @@ static int goodix_ts_probe(struct i2c_client *client,
> >  	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);
> > +	ts->irq_flags = goodix_irq_flags[ts->int_trigger_type] | IRQF_ONESHOT;
> > +	error = goodix_request_irq(ts);
> >  	if (error) {
> >  		dev_err(&client->dev, "request IRQ failed: %d\n", error);
> >  		return error;
> > @@ -611,6 +629,65 @@ static int goodix_ts_probe(struct i2c_client *client,
> >  	return 0;
> >  }
> >
> > +#ifdef CONFIG_PM_SLEEP
> > +static int goodix_suspend(struct device *dev)
> 
> Please drop #ifdef CONFIG_PM_SLEEP and annotate with __maybe_unused
> instead.
> 
Ok, will fix this.

> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct goodix_ts_data *ts = i2c_get_clientdata(client);
> > +	int ret;
> > +
> > +	goodix_free_irq(ts);
> 
> Why do we need to free IRQ here?
> 

The suspend sequence described in Goodix datasheets requires using the irq pin as output pin.
If the interrupt is not released, gpiod_direction_output(ts->gpiod_int, 0) will fail with the message
"tried to set a GPIO tied to an IRQ as output".  Initially I used gpiod_lock/unlock_as_irq, but this API
is no longer available (and gpiochip_lock_as_irq is only available for gpio chips). Is there a better way
to do this?

> > +	/* Output LOW on the INT pin for 5 ms */
> > +	ret = gpiod_direction_output(ts->gpiod_int, 0);
> > +	if (ret) {
> > +		goodix_request_irq(ts);
> > +		return ret;
> > +	}
> > +	usleep_range(5000, 6000);
> > +
> > +	ret = goodix_i2c_write_u8(ts->client, GOODIX_REG_COMMAND,
> > +				  GOODIX_CMD_SCREEN_OFF);
> > +	if (ret) {
> > +		dev_err(&ts->client->dev, "Screen off command failed\n");
> > +		gpiod_direction_input(ts->gpiod_int);
> > +		goodix_request_irq(ts);
> > +		return -EAGAIN;
> > +	}
> > +
> > +	/*
> > +	 * The datasheet specifies that the interval between sending screen-off
> > +	 * command and wake-up should be longer than 58 ms. To avoid waking up
> > +	 * sooner, delay 58ms here.
> > +	 */
> > +	msleep(58);
> > +	return 0;
> > +}
> > +
> > +static int goodix_resume(struct device *dev)
> 
> __maybe_unused
Ok.
> 
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct goodix_ts_data *ts = i2c_get_clientdata(client);
> > +	int ret;
> > +
> > +	/*
> > +	 * Exit sleep mode by outputting HIGH level to INT pin
> > +	 * for 2ms~5ms.
> > +	 */
> > +	ret = gpiod_direction_output(ts->gpiod_int, 1);
> > +	if (ret)
> > +		return ret;
> > +	usleep_range(2000, 5000);
> > +
> > +	ret = goodix_int_sync(ts);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return goodix_request_irq(ts);
> > +}
> > +#endif
> > +
> > +static SIMPLE_DEV_PM_OPS(goodix_pm_ops, goodix_suspend, goodix_resume);
> > +
> >  static const struct i2c_device_id goodix_ts_id[] = {
> >  	{ "GDIX1001:00", 0 },
> >  	{ }
> > @@ -646,6 +723,7 @@ static struct i2c_driver goodix_ts_driver = {
> >  		.owner = THIS_MODULE,
> >  		.acpi_match_table = ACPI_PTR(goodix_acpi_match),
> >  		.of_match_table = of_match_ptr(goodix_of_match),
> > +		.pm = &goodix_pm_ops,
> >  	},
> >  };
> >  module_i2c_driver(goodix_ts_driver);
> > --
> > 1.9.1
> >
> 

Thanks,
Irina

> Thanks.
> 
> --
> Dmitry

Re: [PATCH v2 4/8] input: goodix: reset device at init

[Bastien Nocera]

On Tue, 2015-06-23 at 13:23 +0000, Tirdea, Irina wrote:
> 
> > -----Original Message-----
> > From: linux-input-owner@vger.kernel.org [mailto:
> > linux-input-owner@vger.kernel.org] On Behalf Of Bastien Nocera
> > Sent: 09 June, 2015 18:53
> > To: Tirdea, Irina
> > Cc: Dmitry Torokhov; Mark Rutland; linux-input@vger.kernel.org; 
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Rob
> > Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian
> > Subject: Re: [PATCH v2 4/8] input: goodix: reset device at init
> > 
> > On Tue, 2015-06-09 at 17:34 +0200, Bastien Nocera wrote:
> > > On Mon, 2015-06-08 at 17:37 +0300, Irina Tirdea wrote:
> > > > After power on, it is recommended that the driver resets the
> > > > device.
> > > > The reset procedure timing is described in the datasheet and is
> > > > used
> > > > at device init (before writing device configuration) and
> > > > for power management. It is a sequence of setting the interrupt
> > > > and reset pins high/low at specific timing intervals. This
> > > > procedure
> > > > also includes setting the slave address to the one specified in 
> > > > the
> > > > ACPI/device tree.
> > > 
> > > This breaks the touchscreen on my Onda v975w:
> > > [  239.732858] Goodix-TS i2c-GDIX1001:00: ID 9271, version: 1020
> > > [  239.732977] Goodix-TS i2c-GDIX1001:00: Failed to get reset 
> > > GPIO:
> > > -16
> > > [  239.736071] Goodix-TS: probe of i2c-GDIX1001:00 failed with 
> > > error
> > > -16
> > > 
> > > This is the DSDT for that device:
> > > https://bugzilla.kernel.org/attachment.cgi?id=149331
> > 
> 
> Oops. That's right - I am using named interrupts which are available 
> only for ACPI 5.1, so 
> devices already out there will not work.
> 
> The problem here is that handling -ENOENT will not help. The gpio 
> pins are declared in the
> ACPI DSDT, but are not named. The devm_gpiod_get API will look for 
> the named interrupt
> first and fallback to searching by index if not found. Index will be 
> 0 in both cases, this is why
> the first call will succeed and the second will fail with -EBUSY.
> 
> One way to handle this would be to use indexed gpio pins instead of 
> named gpio pins:
> e.g. consider the first gpio pin to be the reset pin and the second 
> to be the interrupt pin.
> This is used in other drivers as well, e.g. zforce_ts. The problem 
> with this approach is that
> any devices that declare the gpio pins in reversed order in the DSDT 
> will not work and give
> no warning (the pins will be requested successfully, but some of the 
> functionality will not
> work). The DSDT mentioned in 
> https://bugzilla.kernel.org/attachment.cgi?id=149331 lists
> the reset pin first, while I am working on some devices that declare 
> the interrupt gpio pin
> first.
> 
> Another way to handle this is to treat -EBUSY like -ENOENT and not 
> use any functionality
> that depends on the gpio pins. Unfortunately, this means we will not 
> have suspend, esd and
> custom configs even if the pins are connected on the board and 
> available in ACPI(just not
> named).
> 
> I would go with the first approach and document the order requested 
> for the pins, but I would
> like to hear what you think first. Is there a better way to do this?
> 
> > (Note that this means that I haven't been able to test any 
> > following
> > patches in that series than 4/8).
> 
> Sure, that makes sense. The following patches depend on the gpio pins 
> so they would not have
> worked either.

We can apply quirks based on DMI information, so that devices with ACPI
in different orders will work after applying a quirk (as long as
there's a way to detect that it's in the wrong order, obviously).

Re: [PATCH v2 4/8] input: goodix: reset device at init

[Octavian Purdila]

On Tue, Jun 23, 2015 at 5:12 PM, Bastien Nocera <hadess@hadess.net> wrote:
>
> On Tue, 2015-06-23 at 13:23 +0000, Tirdea, Irina wrote:
> >
> > > -----Original Message-----
> > > From: linux-input-owner@vger.kernel.org [mailto:
> > > linux-input-owner@vger.kernel.org] On Behalf Of Bastien Nocera
> > > Sent: 09 June, 2015 18:53
> > > To: Tirdea, Irina
> > > Cc: Dmitry Torokhov; Mark Rutland; linux-input@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Rob
> > > Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian
> > > Subject: Re: [PATCH v2 4/8] input: goodix: reset device at init
> > >
> > > On Tue, 2015-06-09 at 17:34 +0200, Bastien Nocera wrote:
> > > > On Mon, 2015-06-08 at 17:37 +0300, Irina Tirdea wrote:
> > > > > After power on, it is recommended that the driver resets the
> > > > > device.
> > > > > The reset procedure timing is described in the datasheet and is
> > > > > used
> > > > > at device init (before writing device configuration) and
> > > > > for power management. It is a sequence of setting the interrupt
> > > > > and reset pins high/low at specific timing intervals. This
> > > > > procedure
> > > > > also includes setting the slave address to the one specified in
> > > > > the
> > > > > ACPI/device tree.
> > > >
> > > > This breaks the touchscreen on my Onda v975w:
> > > > [  239.732858] Goodix-TS i2c-GDIX1001:00: ID 9271, version: 1020
> > > > [  239.732977] Goodix-TS i2c-GDIX1001:00: Failed to get reset
> > > > GPIO:
> > > > -16
> > > > [  239.736071] Goodix-TS: probe of i2c-GDIX1001:00 failed with
> > > > error
> > > > -16
> > > >
> > > > This is the DSDT for that device:
> > > > https://bugzilla.kernel.org/attachment.cgi?id=149331
> > >
> >
> > Oops. That's right - I am using named interrupts which are available
> > only for ACPI 5.1, so
> > devices already out there will not work.
> >
> > The problem here is that handling -ENOENT will not help. The gpio
> > pins are declared in the
> > ACPI DSDT, but are not named. The devm_gpiod_get API will look for
> > the named interrupt
> > first and fallback to searching by index if not found. Index will be
> > 0 in both cases, this is why
> > the first call will succeed and the second will fail with -EBUSY.
> >
> > One way to handle this would be to use indexed gpio pins instead of
> > named gpio pins:
> > e.g. consider the first gpio pin to be the reset pin and the second
> > to be the interrupt pin.
> > This is used in other drivers as well, e.g. zforce_ts. The problem
> > with this approach is that
> > any devices that declare the gpio pins in reversed order in the DSDT
> > will not work and give
> > no warning (the pins will be requested successfully, but some of the
> > functionality will not
> > work). The DSDT mentioned in
> > https://bugzilla.kernel.org/attachment.cgi?id=149331 lists
> > the reset pin first, while I am working on some devices that declare
> > the interrupt gpio pin
> > first.
> >
> > Another way to handle this is to treat -EBUSY like -ENOENT and not
> > use any functionality
> > that depends on the gpio pins. Unfortunately, this means we will not
> > have suspend, esd and
> > custom configs even if the pins are connected on the board and
> > available in ACPI(just not
> > named).
> >
> > I would go with the first approach and document the order requested
> > for the pins, but I would
> > like to hear what you think first. Is there a better way to do this?
> >
> > > (Note that this means that I haven't been able to test any
> > > following
> > > patches in that series than 4/8).
> >
> > Sure, that makes sense. The following patches depend on the gpio pins
> > so they would not have
> > worked either.
>
> We can apply quirks based on DMI information, so that devices with ACPI
> in different orders will work after applying a quirk (as long as
> there's a way to detect that it's in the wrong order, obviously).
>

I think even using the ACPI id (_HID) should be enough (at least in
the cases we know so far) to make the difference in how the pins are
used.

Re: [PATCH v2 5/8] input: goodix: write configuration data to device

[Dmitry Torokhov]

On Tue, Jun 23, 2015 at 01:27:01PM +0000, Tirdea, Irina wrote:
> 
> 
> > -----Original Message-----
> > From: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@vger.kernel.org] On Behalf Of Dmitry Torokhov
> > Sent: 09 June, 2015 21:14
> > To: Tirdea, Irina
> > Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Rob
> > Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian
> > Subject: Re: [PATCH v2 5/8] input: goodix: write configuration data to device
> > 
> > Hi Irina,
> > 
> 
> Hi Dmitry,
> 
> > On Mon, Jun 08, 2015 at 05:37:50PM +0300, Irina Tirdea wrote:
> > > Goodix devices can be configured by writing custom data to the device at
> > > init. The configuration data is read with request_firmware from
> > > "goodix_<id>_cfg.bin", where <id> is the product id read from the device
> > > (e.g.: goodix_911_cfg.bin for Goodix GT911, goodix_9271_cfg.bin for
> > > GT9271).
> > >
> > > The configuration information has a specific format described in the Goodix
> > > datasheet. It includes X/Y resolution, maximum supported touch points,
> > > interrupt flags, various sesitivity factors and settings for advanced
> > > features (like gesture recognition).
> > >
> > > This is based on Goodix datasheets for GT911 and GT9271 and on Goodix
> > > driver gt9xx.c for Android (publicly available in Android kernel
> > > trees for various devices).
> > 
> > I am afraid that just using request_firmware() in probe() is not the
> > best option as it may cause either errors or delays in
> > initialization of the driver is compiled into the kernel and tries to
> > initialize before filesystem with configuration file is mounted (and
> > firmware is not built into the kernel). We can either try switch to
> > request_firmware_nowait() or at least document that we need firmware at
> > boot.
> > 
> 
> The reason I did not use request_firmware_nowait() is that this configuration data
> includes information needed by probe (x/y resolution, interrupt trigger type, max
> touch num), used in goodix_read_config, goodix_ts_report_touch  and for irq flags
> when requesting the interrupt. I will document that we need this configuration
> firmware at boot in the commit message and add a comment in the code.
> Is there any other documentation I need to update?
> 
> > >
> > > Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> > > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > > ---
> > >  drivers/input/touchscreen/goodix.c | 128 +++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 128 insertions(+)
> > >
> > > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> > > index c345eb7..1ce9278 100644
> > > --- a/drivers/input/touchscreen/goodix.c
> > > +++ b/drivers/input/touchscreen/goodix.c
> > > @@ -24,6 +24,7 @@
> > >  #include <linux/interrupt.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/acpi.h>
> > > +#include <linux/firmware.h>
> > >  #include <linux/gpio.h>
> > >  #include <linux/of.h>
> > >  #include <asm/unaligned.h>
> > > @@ -95,6 +96,39 @@ 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 a register of the i2c slave device.
> > > + *
> > > + * @client: i2c device.
> > > + * @reg: the register to write to.
> > > + * @buf: raw data buffer to write.
> > > + * @len: length of the buffer to write
> > > + */
> > > +static int goodix_i2c_write(struct i2c_client *client, u16 reg, const u8 *buf,
> > > +			    unsigned len)
> > > +{
> > > +	u8 *addr_buf;
> > > +	struct i2c_msg msg;
> > > +	int ret;
> > > +
> > > +	addr_buf = kmalloc(len + 2, GFP_KERNEL);
> > > +	if (!addr_buf)
> > > +		return -ENOMEM;
> > > +
> > > +	addr_buf[0] = reg >> 8;
> > > +	addr_buf[1] = reg & 0xFF;
> > > +	memcpy(&addr_buf[2], buf, len);
> > > +
> > > +	msg.flags = 0;
> > > +	msg.addr = client->addr;
> > > +	msg.buf = addr_buf;
> > > +	msg.len = len + 2;
> > > +
> > > +	ret = i2c_transfer(client->adapter, &msg, 1);
> > > +	kfree(addr_buf);
> > > +	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;
> > > @@ -192,6 +226,95 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
> > >  	return IRQ_HANDLED;
> > >  }
> > >
> > > +/**
> > > + * goodix_check_cfg - Checks if config buffer is valid
> > > + *
> > > + * @ts: goodix_ts_data pointer
> > > + * @fw: firmware config data
> > > + */
> > > +static int goodix_check_cfg(struct goodix_ts_data *ts,
> > > +			    const struct firmware *fw)
> > > +{
> > > +	int i, raw_cfg_len;
> > > +	u8 check_sum = 0;
> > > +
> > > +	if (fw->size > GOODIX_CONFIG_MAX_LENGTH) {
> > > +		dev_err(&ts->client->dev,
> > > +			"The length of the config buffer array is not correct");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	raw_cfg_len = fw->size - 2;
> > > +	for (i = 0; i < raw_cfg_len; i++)
> > > +		check_sum += fw->data[i];
> > > +	check_sum = (~check_sum) + 1;
> > > +	if (check_sum != fw->data[raw_cfg_len]) {
> > > +		dev_err(&ts->client->dev,
> > > +			"The checksum of the config buffer array is not correct");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (fw->data[raw_cfg_len + 1] != 1) {
> > > +		dev_err(&ts->client->dev,
> > > +			"The Config_Fresh register needs to be set");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * goodix_send_cfg - Write device config
> > > + *
> > > + * @ts: goodix_ts_data pointer
> > > + * @id: product id read from device
> > > + */
> > > +static int goodix_send_cfg(struct goodix_ts_data *ts, u16 id)
> > > +{
> > > +	const struct firmware *fw = NULL;
> > > +	char *fw_name;
> > > +	int ret;
> > > +
> > > +	fw_name = kasprintf(GFP_KERNEL, "goodix_%d_cfg.bin", id);
> > > +	if (!fw_name)
> > > +		return -ENOMEM;
> > > +
> > > +	ret = request_firmware(&fw, fw_name, &ts->client->dev);
> > > +	if (ret) {
> > > +		dev_err(&ts->client->dev, "Unable to open firmware %s\n",
> > > +			fw_name);
> > > +		goto err_free_fw_name;
> > 
> > That I think will cause driver to abort binding if config is not there.
> > Do we always need to load the config? Is configuration stored in NVRAM?
> > Maybe configuration should be only loaded when userspace requests it?
> > 
> 
> The device has a default configuration, but usually this does not match the platform
> needs. It is stored in volatile RAM, so we need to rewrite it after power off.
> From my tests, the default values are invalid if I do not write any config data
> (goodix_read_config will print "Invalid config, using defaults").
> 
> However, we should still be able to use the device with these defaults even
> if the config firmware is not there. I will just return 0 if firmware is not found.
> Since configuration needs to be written before configuring the input device and
> the interrupts, I don't think we can allow userspace to control it.

Well, my concern is that we currently have devices and DTSes that work
just fine without special config. How do they manage to do that? We need
to make sure that new changes do not break existing users.

Thanks.

-- 
Dmitry

RE: [PATCH v2 5/8] input: goodix: write configuration data to device

[Tirdea, Irina]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: 23 June, 2015 21:59
> To: Tirdea, Irina
> Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Rob
> Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian
> Subject: Re: [PATCH v2 5/8] input: goodix: write configuration data to device
> 
> On Tue, Jun 23, 2015 at 01:27:01PM +0000, Tirdea, Irina wrote:
> >
> >
> > > -----Original Message-----
> > > From: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@vger.kernel.org] On Behalf Of Dmitry Torokhov
> > > Sent: 09 June, 2015 21:14
> > > To: Tirdea, Irina
> > > Cc: Bastien Nocera; Mark Rutland; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Rob
> > > Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian
> > > Subject: Re: [PATCH v2 5/8] input: goodix: write configuration data to device
> > >
> > > Hi Irina,
> > >
> >
> > Hi Dmitry,
> >
> > > On Mon, Jun 08, 2015 at 05:37:50PM +0300, Irina Tirdea wrote:
> > > > Goodix devices can be configured by writing custom data to the device at
> > > > init. The configuration data is read with request_firmware from
> > > > "goodix_<id>_cfg.bin", where <id> is the product id read from the device
> > > > (e.g.: goodix_911_cfg.bin for Goodix GT911, goodix_9271_cfg.bin for
> > > > GT9271).
> > > >
> > > > The configuration information has a specific format described in the Goodix
> > > > datasheet. It includes X/Y resolution, maximum supported touch points,
> > > > interrupt flags, various sesitivity factors and settings for advanced
> > > > features (like gesture recognition).
> > > >
> > > > This is based on Goodix datasheets for GT911 and GT9271 and on Goodix
> > > > driver gt9xx.c for Android (publicly available in Android kernel
> > > > trees for various devices).
> > >
> > > I am afraid that just using request_firmware() in probe() is not the
> > > best option as it may cause either errors or delays in
> > > initialization of the driver is compiled into the kernel and tries to
> > > initialize before filesystem with configuration file is mounted (and
> > > firmware is not built into the kernel). We can either try switch to
> > > request_firmware_nowait() or at least document that we need firmware at
> > > boot.
> > >
> >
> > The reason I did not use request_firmware_nowait() is that this configuration data
> > includes information needed by probe (x/y resolution, interrupt trigger type, max
> > touch num), used in goodix_read_config, goodix_ts_report_touch  and for irq flags
> > when requesting the interrupt. I will document that we need this configuration
> > firmware at boot in the commit message and add a comment in the code.
> > Is there any other documentation I need to update?
> >
> > > >
> > > > Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> > > > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > > > ---
> > > >  drivers/input/touchscreen/goodix.c | 128 +++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 128 insertions(+)
> > > >
> > > > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> > > > index c345eb7..1ce9278 100644
> > > > --- a/drivers/input/touchscreen/goodix.c
> > > > +++ b/drivers/input/touchscreen/goodix.c
> > > > @@ -24,6 +24,7 @@
> > > >  #include <linux/interrupt.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/acpi.h>
> > > > +#include <linux/firmware.h>
> > > >  #include <linux/gpio.h>
> > > >  #include <linux/of.h>
> > > >  #include <asm/unaligned.h>
> > > > @@ -95,6 +96,39 @@ 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 a register of the i2c slave device.
> > > > + *
> > > > + * @client: i2c device.
> > > > + * @reg: the register to write to.
> > > > + * @buf: raw data buffer to write.
> > > > + * @len: length of the buffer to write
> > > > + */
> > > > +static int goodix_i2c_write(struct i2c_client *client, u16 reg, const u8 *buf,
> > > > +			    unsigned len)
> > > > +{
> > > > +	u8 *addr_buf;
> > > > +	struct i2c_msg msg;
> > > > +	int ret;
> > > > +
> > > > +	addr_buf = kmalloc(len + 2, GFP_KERNEL);
> > > > +	if (!addr_buf)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	addr_buf[0] = reg >> 8;
> > > > +	addr_buf[1] = reg & 0xFF;
> > > > +	memcpy(&addr_buf[2], buf, len);
> > > > +
> > > > +	msg.flags = 0;
> > > > +	msg.addr = client->addr;
> > > > +	msg.buf = addr_buf;
> > > > +	msg.len = len + 2;
> > > > +
> > > > +	ret = i2c_transfer(client->adapter, &msg, 1);
> > > > +	kfree(addr_buf);
> > > > +	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;
> > > > @@ -192,6 +226,95 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
> > > >  	return IRQ_HANDLED;
> > > >  }
> > > >
> > > > +/**
> > > > + * goodix_check_cfg - Checks if config buffer is valid
> > > > + *
> > > > + * @ts: goodix_ts_data pointer
> > > > + * @fw: firmware config data
> > > > + */
> > > > +static int goodix_check_cfg(struct goodix_ts_data *ts,
> > > > +			    const struct firmware *fw)
> > > > +{
> > > > +	int i, raw_cfg_len;
> > > > +	u8 check_sum = 0;
> > > > +
> > > > +	if (fw->size > GOODIX_CONFIG_MAX_LENGTH) {
> > > > +		dev_err(&ts->client->dev,
> > > > +			"The length of the config buffer array is not correct");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	raw_cfg_len = fw->size - 2;
> > > > +	for (i = 0; i < raw_cfg_len; i++)
> > > > +		check_sum += fw->data[i];
> > > > +	check_sum = (~check_sum) + 1;
> > > > +	if (check_sum != fw->data[raw_cfg_len]) {
> > > > +		dev_err(&ts->client->dev,
> > > > +			"The checksum of the config buffer array is not correct");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	if (fw->data[raw_cfg_len + 1] != 1) {
> > > > +		dev_err(&ts->client->dev,
> > > > +			"The Config_Fresh register needs to be set");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * goodix_send_cfg - Write device config
> > > > + *
> > > > + * @ts: goodix_ts_data pointer
> > > > + * @id: product id read from device
> > > > + */
> > > > +static int goodix_send_cfg(struct goodix_ts_data *ts, u16 id)
> > > > +{
> > > > +	const struct firmware *fw = NULL;
> > > > +	char *fw_name;
> > > > +	int ret;
> > > > +
> > > > +	fw_name = kasprintf(GFP_KERNEL, "goodix_%d_cfg.bin", id);
> > > > +	if (!fw_name)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ret = request_firmware(&fw, fw_name, &ts->client->dev);
> > > > +	if (ret) {
> > > > +		dev_err(&ts->client->dev, "Unable to open firmware %s\n",
> > > > +			fw_name);
> > > > +		goto err_free_fw_name;
> > >
> > > That I think will cause driver to abort binding if config is not there.
> > > Do we always need to load the config? Is configuration stored in NVRAM?
> > > Maybe configuration should be only loaded when userspace requests it?
> > >
> >
> > The device has a default configuration, but usually this does not match the platform
> > needs. It is stored in volatile RAM, so we need to rewrite it after power off.
> > From my tests, the default values are invalid if I do not write any config data
> > (goodix_read_config will print "Invalid config, using defaults").
> >
> > However, we should still be able to use the device with these defaults even
> > if the config firmware is not there. I will just return 0 if firmware is not found.
> > Since configuration needs to be written before configuring the input device and
> > the interrupts, I don't think we can allow userspace to control it.
> 
> Well, my concern is that we currently have devices and DTSes that work
> just fine without special config. How do they manage to do that? We need
> to make sure that new changes do not break existing users.
> 

I took a better look at the config issue and it seems to depend on the device/platform.
I have some devices that work just fine with the driver defaults while others
won't work until the config is written (due to custom configuration values for parameters
like trigger type, axis switch-over).

I initially misunderstood the request_firmware_nowait interface, but after doing some
additional tests this seems to be the best option.  I will update the rest of the patches
as well so that devices that do not have gpio pins declared in ACPI/DT or the config fw
present will work just as they do now.

Thanks,
Irina

> Thanks.
> 
> --
> Dmitry

RE: [PATCH v2 4/8] input: goodix: reset device at init

[Tirdea, Irina]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5019 bytes --]



> -----Original Message-----
> From: Octavian Purdila [mailto:octavian.purdila@intel.com]
> Sent: 23 June, 2015 17:51
> To: Bastien Nocera
> Cc: Tirdea, Irina; Dmitry Torokhov; Mark Rutland; linux-input@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Rob Herring; Pawel Moll; Ian Campbell; Kumar Gala
> Subject: Re: [PATCH v2 4/8] input: goodix: reset device at init
> 
> On Tue, Jun 23, 2015 at 5:12 PM, Bastien Nocera <hadess@hadess.net> wrote:
> >
> > On Tue, 2015-06-23 at 13:23 +0000, Tirdea, Irina wrote:
> > >
> > > > -----Original Message-----
> > > > From: linux-input-owner@vger.kernel.org [mailto:
> > > > linux-input-owner@vger.kernel.org] On Behalf Of Bastien Nocera
> > > > Sent: 09 June, 2015 18:53
> > > > To: Tirdea, Irina
> > > > Cc: Dmitry Torokhov; Mark Rutland; linux-input@vger.kernel.org;
> > > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Rob
> > > > Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian
> > > > Subject: Re: [PATCH v2 4/8] input: goodix: reset device at init
> > > >
> > > > On Tue, 2015-06-09 at 17:34 +0200, Bastien Nocera wrote:
> > > > > On Mon, 2015-06-08 at 17:37 +0300, Irina Tirdea wrote:
> > > > > > After power on, it is recommended that the driver resets the
> > > > > > device.
> > > > > > The reset procedure timing is described in the datasheet and is
> > > > > > used
> > > > > > at device init (before writing device configuration) and
> > > > > > for power management. It is a sequence of setting the interrupt
> > > > > > and reset pins high/low at specific timing intervals. This
> > > > > > procedure
> > > > > > also includes setting the slave address to the one specified in
> > > > > > the
> > > > > > ACPI/device tree.
> > > > >
> > > > > This breaks the touchscreen on my Onda v975w:
> > > > > [  239.732858] Goodix-TS i2c-GDIX1001:00: ID 9271, version: 1020
> > > > > [  239.732977] Goodix-TS i2c-GDIX1001:00: Failed to get reset
> > > > > GPIO:
> > > > > -16
> > > > > [  239.736071] Goodix-TS: probe of i2c-GDIX1001:00 failed with
> > > > > error
> > > > > -16
> > > > >
> > > > > This is the DSDT for that device:
> > > > > https://bugzilla.kernel.org/attachment.cgi?id=149331
> > > >
> > >
> > > Oops. That's right - I am using named interrupts which are available
> > > only for ACPI 5.1, so
> > > devices already out there will not work.
> > >
> > > The problem here is that handling -ENOENT will not help. The gpio
> > > pins are declared in the
> > > ACPI DSDT, but are not named. The devm_gpiod_get API will look for
> > > the named interrupt
> > > first and fallback to searching by index if not found. Index will be
> > > 0 in both cases, this is why
> > > the first call will succeed and the second will fail with -EBUSY.
> > >
> > > One way to handle this would be to use indexed gpio pins instead of
> > > named gpio pins:
> > > e.g. consider the first gpio pin to be the reset pin and the second
> > > to be the interrupt pin.
> > > This is used in other drivers as well, e.g. zforce_ts. The problem
> > > with this approach is that
> > > any devices that declare the gpio pins in reversed order in the DSDT
> > > will not work and give
> > > no warning (the pins will be requested successfully, but some of the
> > > functionality will not
> > > work). The DSDT mentioned in
> > > https://bugzilla.kernel.org/attachment.cgi?id=149331 lists
> > > the reset pin first, while I am working on some devices that declare
> > > the interrupt gpio pin
> > > first.
> > >
> > > Another way to handle this is to treat -EBUSY like -ENOENT and not
> > > use any functionality
> > > that depends on the gpio pins. Unfortunately, this means we will not
> > > have suspend, esd and
> > > custom configs even if the pins are connected on the board and
> > > available in ACPI(just not
> > > named).
> > >
> > > I would go with the first approach and document the order requested
> > > for the pins, but I would
> > > like to hear what you think first. Is there a better way to do this?
> > >
> > > > (Note that this means that I haven't been able to test any
> > > > following
> > > > patches in that series than 4/8).
> > >
> > > Sure, that makes sense. The following patches depend on the gpio pins
> > > so they would not have
> > > worked either.
> >
> > We can apply quirks based on DMI information, so that devices with ACPI
> > in different orders will work after applying a quirk (as long as
> > there's a way to detect that it's in the wrong order, obviously).
> >
> 
> I think even using the ACPI id (_HID) should be enough (at least in
> the cases we know so far) to make the difference in how the pins are
> used.

Thanks for the suggestions Bastien and Octavian!
I will use the ACPI ID in v3 considering the ACPI table Bastien has mentioned [1].

Thanks,
Irina

[1] https://bugzilla.kernel.org/attachment.cgi?id=149331

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥