[PATCH v9 0/9] Goodix touchscreen enhancements

[PATCH v9 0/9] Goodix touchscreen enhancements

[Irina Tirdea]

v9 only adds GPIOLIB dependency in Kconfig for patch 2:
"Input: goodix - reset device at init". There are no other code
changes from v8.

Thanks for testing these changes, Bastien and Aleksei!

Karsten, there is no need to rebase your series on top of v9.

Thanks,
Irina

Changes in v9:
 - add GPIOLIB to driver dependencies
 - add Tested-by tag from Bastien and Aleksei

Changes in v8:
 - only allow new functionality for devices that declare named
gpios (using _DSD properties in ACPI or named DT properties)

Changes in v7:
 - add dmi quirk to skip gpio pins setup and functionality that
depends on them for Onda v975w, WinBook TW100 and WinBook TW700.
 - add support for named gpio pins
 - rework the runtime pm patch to fix a couple of issues
 - sort includes using inverse Xmas tree ordering

Changes in v6:
 - skip runtime power manangent calls in open/close if the device
ACPI/DT configuration does not declare interrupt and reset gpio pins.
 - reset the device before starting i2c communication
 - add Bastien's ack to the first 2 patches

Changes in v5:
 - add some more style cleanup (reorder includes, use error instead
of ret for return values)
 - add runtime power management patch

Changes in v4:
 - use dmi quirk to determine the order of irq and reset pins
 - use actual config length depending on device
 - add sysfs interface to dump config
 - initialize esd timeout from ACPI/DT properly

Changes in v3:
 - dropped the first 3 patches that got merged
 - handle -EPROBE_DEFER and -ENOENT for gpio pins
 - skip functionality depending on the gpio pins if the pins are not
properly initialized from ACPI/DT (reset, write config, power management,
ESD)
 - dropped #ifdef CONFIG_PM_SLEEP and annotated with __maybe_unused instead
 - use sysfs property to set ESD timeout instead of ACPI/DT property
 - use request_firmware_nowait to read configuration firmware and use
defaults if firmware is not found
 - use ACPI IDs to determine the order of the GPIO pins in the ACPI tables
(interrupt pin first or reset pin first)

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 (9):
  Input: goodix - use actual config length for each device type
  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
  Input: goodix - add sysfs interface to dump config
  Input: goodix - add runtime power management support
  Input: goodix - sort includes using inverse Xmas tree order

 .../bindings/input/touchscreen/goodix.txt          |  11 +
 drivers/input/touchscreen/Kconfig                  |   1 +
 drivers/input/touchscreen/goodix.c                 | 766 +++++++++++++++++++--
 3 files changed, 733 insertions(+), 45 deletions(-)

-- 
1.9.1

[PATCH v9 0/9] Goodix touchscreen enhancements

[Irina Tirdea]

v9 only adds GPIOLIB dependency in Kconfig for patch 2:
"Input: goodix - reset device at init". There are no other code
changes from v8.

Thanks for testing these changes, Bastien and Aleksei!

Karsten, there is no need to rebase your series on top of v9.

Thanks,
Irina

Changes in v9:
 - add GPIOLIB to driver dependencies
 - add Tested-by tag from Bastien and Aleksei

Changes in v8:
 - only allow new functionality for devices that declare named
gpios (using _DSD properties in ACPI or named DT properties)

Changes in v7:
 - add dmi quirk to skip gpio pins setup and functionality that
depends on them for Onda v975w, WinBook TW100 and WinBook TW700.
 - add support for named gpio pins
 - rework the runtime pm patch to fix a couple of issues
 - sort includes using inverse Xmas tree ordering

Changes in v6:
 - skip runtime power manangent calls in open/close if the device
ACPI/DT configuration does not declare interrupt and reset gpio pins.
 - reset the device before starting i2c communication
 - add Bastien's ack to the first 2 patches

Changes in v5:
 - add some more style cleanup (reorder includes, use error instead
of ret for return values)
 - add runtime power management patch

Changes in v4:
 - use dmi quirk to determine the order of irq and reset pins
 - use actual config length depending on device
 - add sysfs interface to dump config
 - initialize esd timeout from ACPI/DT properly

Changes in v3:
 - dropped the first 3 patches that got merged
 - handle -EPROBE_DEFER and -ENOENT for gpio pins
 - skip functionality depending on the gpio pins if the pins are not
properly initialized from ACPI/DT (reset, write config, power management,
ESD)
 - dropped #ifdef CONFIG_PM_SLEEP and annotated with __maybe_unused instead
 - use sysfs property to set ESD timeout instead of ACPI/DT property
 - use request_firmware_nowait to read configuration firmware and use
defaults if firmware is not found
 - use ACPI IDs to determine the order of the GPIO pins in the ACPI tables
(interrupt pin first or reset pin first)

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 (9):
  Input: goodix - use actual config length for each device type
  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
  Input: goodix - add sysfs interface to dump config
  Input: goodix - add runtime power management support
  Input: goodix - sort includes using inverse Xmas tree order

 .../bindings/input/touchscreen/goodix.txt          |  11 +
 drivers/input/touchscreen/Kconfig                  |   1 +
 drivers/input/touchscreen/goodix.c                 | 766 +++++++++++++++++++--
 3 files changed, 733 insertions(+), 45 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 1/9] Input: goodix - use actual config length for each device type

[Irina Tirdea]

Each of the Goodix devices supported by this driver has a fixed size for
the configuration information registers. The size varies depending on the
device and is specified in the datasheet.

Use the proper configuration length as specified in the datasheet for
each device model, so we do not read more than the actual size of the
configuration registers.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Acked-by: Bastien Nocera <hadess@hadess.net>
Tested-by: Bastien Nocera <hadess@hadess.net>
Tested-by: Aleksei Mamlin <mamlinav@gmail.com>
---
 drivers/input/touchscreen/goodix.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 4d113c9..56d0330 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -36,6 +36,7 @@ struct goodix_ts_data {
 	unsigned int max_touch_num;
 	unsigned int int_trigger_type;
 	bool rotated_screen;
+	int cfg_len;
 };
 
 #define GOODIX_MAX_HEIGHT		4096
@@ -45,6 +46,8 @@ struct goodix_ts_data {
 #define GOODIX_MAX_CONTACTS		10
 
 #define GOODIX_CONFIG_MAX_LENGTH	240
+#define GOODIX_CONFIG_911_LENGTH	186
+#define GOODIX_CONFIG_967_LENGTH	228
 
 /* Register defines */
 #define GOODIX_READ_COOR_ADDR		0x814E
@@ -115,6 +118,23 @@ static int goodix_i2c_read(struct i2c_client *client,
 	return ret < 0 ? ret : (ret != ARRAY_SIZE(msgs) ? -EIO : 0);
 }
 
+static int goodix_get_cfg_len(u16 id)
+{
+	switch (id) {
+	case 911:
+	case 9271:
+	case 9110:
+	case 927:
+	case 928:
+		return GOODIX_CONFIG_911_LENGTH;
+	case 912:
+	case 967:
+		return GOODIX_CONFIG_967_LENGTH;
+	default:
+		return GOODIX_CONFIG_MAX_LENGTH;
+	}
+}
+
 static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
 {
 	int touch_num;
@@ -230,8 +250,7 @@ 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, ts->cfg_len);
 	if (error) {
 		dev_warn(&ts->client->dev,
 			 "Error reading config (%d), using defaults\n",
@@ -398,6 +417,8 @@ static int goodix_ts_probe(struct i2c_client *client,
 		return error;
 	}
 
+	ts->cfg_len = goodix_get_cfg_len(id_info);
+
 	goodix_read_config(ts);
 
 	error = goodix_request_input_dev(ts, version_info, id_info);
-- 
1.9.1

[PATCH v9 2/9] 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/device tree). For devices
that do not have the gpio pins properly declared, the functionality
depending on these pins will not be available, but the device can still
be used with basic functionality.

For both device tree and ACPI, the interrupt gpio pin configuration is
read from the "irq-gpio" property and the reset pin configuration is
read from the "reset-gpio" property. For ACPI 5.1, named properties
can be specified using the _DSD section. This functionality will not be
available for devices that use indexed gpio pins declared in the _CRS
section (we need to provide backward compatibility with devices
that do not support using the interrupt gpio pin as output).

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>
Acked-by: Bastien Nocera <hadess@hadess.net>
Tested-by: Bastien Nocera <hadess@hadess.net>
Tested-by: Aleksei Mamlin <mamlinav@gmail.com>
---
 .../bindings/input/touchscreen/goodix.txt          |   5 +
 drivers/input/touchscreen/Kconfig                  |   1 +
 drivers/input/touchscreen/goodix.c                 | 101 +++++++++++++++++++++
 3 files changed, 107 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/Kconfig b/drivers/input/touchscreen/Kconfig
index 771d95c..76f5a9d 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -324,6 +324,7 @@ config TOUCHSCREEN_FUJITSU
 config TOUCHSCREEN_GOODIX
 	tristate "Goodix I2C touchscreen"
 	depends on I2C
+	depends on GPIOLIB
 	help
 	  Say Y here if you have the Goodix touchscreen (such as one
 	  installed in Onda v975w tablets) connected to your
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 56d0330..87304ac 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -16,6 +16,7 @@
 
 #include <linux/kernel.h>
 #include <linux/dmi.h>
+#include <linux/gpio.h>
 #include <linux/i2c.h>
 #include <linux/input.h>
 #include <linux/input/mt.h>
@@ -37,8 +38,13 @@ struct goodix_ts_data {
 	unsigned int int_trigger_type;
 	bool rotated_screen;
 	int cfg_len;
+	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
@@ -237,6 +243,88 @@ 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 error;
+
+	error = gpiod_direction_output(ts->gpiod_int, 0);
+	if (error)
+		return error;
+	msleep(50);				/* T5: 50ms */
+
+	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 error;
+
+	/* begin select I2C slave addr */
+	error = gpiod_direction_output(ts->gpiod_rst, 0);
+	if (error)
+		return error;
+	msleep(20);				/* T2: > 10ms */
+	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
+	error = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14);
+	if (error)
+		return error;
+	usleep_range(100, 2000);		/* T3: > 100us */
+	error = gpiod_direction_output(ts->gpiod_rst, 1);
+	if (error)
+		return error;
+	usleep_range(6000, 10000);		/* T4: > 5ms */
+	/* end select I2C slave addr */
+	error = gpiod_direction_input(ts->gpiod_rst);
+	if (error)
+		return error;
+	return goodix_int_sync(ts);
+}
+
+/**
+ * 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)
+{
+	int error;
+	struct device *dev;
+	struct gpio_desc *gpiod;
+
+	if (!ts->client)
+		return -EINVAL;
+	dev = &ts->client->dev;
+
+	/* Get the interrupt GPIO pin number */
+	gpiod = devm_gpiod_get(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN);
+	if (IS_ERR(gpiod)) {
+		error = PTR_ERR(gpiod);
+		if (error != -EPROBE_DEFER)
+			dev_dbg(dev, "Failed to get %s GPIO: %d\n",
+				GOODIX_GPIO_INT_NAME, error);
+		return error;
+	}
+	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)) {
+		error = PTR_ERR(gpiod);
+		if (error != -EPROBE_DEFER)
+			dev_dbg(dev, "Failed to get %s GPIO: %d\n",
+				GOODIX_GPIO_RST_NAME, error);
+		return error;
+	}
+	ts->gpiod_rst = gpiod;
+
+	return 0;
+}
+
 /**
  * goodix_read_config - Read the embedded configuration of the panel
  *
@@ -405,6 +493,19 @@ static int goodix_ts_probe(struct i2c_client *client,
 	ts->client = client;
 	i2c_set_clientdata(client, ts);
 
+	error = goodix_get_gpio_config(ts);
+	if (error == -EPROBE_DEFER)
+		return error;
+
+	if (ts->gpiod_int && ts->gpiod_rst) {
+		/* reset the controller */
+		error = goodix_reset(ts);
+		if (error) {
+			dev_err(&client->dev, "Controller reset failed.\n");
+			return error;
+		}
+	}
+
 	error = goodix_i2c_test(client);
 	if (error) {
 		dev_err(&client->dev, "I2C communication failure: %d\n", error);
-- 
1.9.1

[PATCH v9 3/9] 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 sensitivity factors and settings for advanced
features (like gesture recognition).

Before writing the firmware, it is necessary to reset the device. If
the device ACPI/DT information does not declare gpio pins (needed for
reset), writing the firmware will not be available for these devices.

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>
Tested-by: Bastien Nocera <hadess@hadess.net>
Tested-by: Aleksei Mamlin <mamlinav@gmail.com>
---
 drivers/input/touchscreen/goodix.c | 229 +++++++++++++++++++++++++++++++------
 1 file changed, 196 insertions(+), 33 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 87304ac..b4dfe4b 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -16,6 +16,7 @@
 
 #include <linux/kernel.h>
 #include <linux/dmi.h>
+#include <linux/firmware.h>
 #include <linux/gpio.h>
 #include <linux/i2c.h>
 #include <linux/input.h>
@@ -40,6 +41,9 @@ struct goodix_ts_data {
 	int cfg_len;
 	struct gpio_desc *gpiod_int;
 	struct gpio_desc *gpiod_rst;
+	u16 id;
+	u16 version;
+	char *cfg_name;
 };
 
 #define GOODIX_GPIO_INT_NAME		"irq"
@@ -124,6 +128,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_get_cfg_len(u16 id)
 {
 	switch (id) {
@@ -243,6 +280,73 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/**
+ * goodix_check_cfg - Checks if config fw is valid
+ *
+ * @ts: goodix_ts_data pointer
+ * @cfg: firmware config data
+ */
+static int goodix_check_cfg(struct goodix_ts_data *ts,
+			    const struct firmware *cfg)
+{
+	int i, raw_cfg_len;
+	u8 check_sum = 0;
+
+	if (cfg->size > GOODIX_CONFIG_MAX_LENGTH) {
+		dev_err(&ts->client->dev,
+			"The length of the config fw is not correct");
+		return -EINVAL;
+	}
+
+	raw_cfg_len = cfg->size - 2;
+	for (i = 0; i < raw_cfg_len; i++)
+		check_sum += cfg->data[i];
+	check_sum = (~check_sum) + 1;
+	if (check_sum != cfg->data[raw_cfg_len]) {
+		dev_err(&ts->client->dev,
+			"The checksum of the config fw is not correct");
+		return -EINVAL;
+	}
+
+	if (cfg->data[raw_cfg_len + 1] != 1) {
+		dev_err(&ts->client->dev,
+			"Config fw must have Config_Fresh register set");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/**
+ * goodix_send_cfg - Write fw config to device
+ *
+ * @ts: goodix_ts_data pointer
+ * @cfg: config firmware to write to device
+ */
+static int goodix_send_cfg(struct goodix_ts_data *ts,
+			   const struct firmware *cfg)
+{
+	int error;
+
+	error = goodix_check_cfg(ts, cfg);
+	if (error)
+		return error;
+
+	error = goodix_i2c_write(ts->client, GOODIX_REG_CONFIG_DATA, cfg->data,
+				 cfg->size);
+	if (error) {
+		dev_err(&ts->client->dev, "Failed to write config data: %d",
+			error);
+		return error;
+	}
+	dev_dbg(&ts->client->dev, "Config sent successfully.");
+
+	/* Let the firmware reconfigure itself, so sleep for 10ms */
+	usleep_range(10000, 11000);
+
+	return 0;
+}
+
 static int goodix_int_sync(struct goodix_ts_data *ts)
 {
 	int error;
@@ -371,30 +475,29 @@ static void goodix_read_config(struct goodix_ts_data *ts)
 /**
  * goodix_read_version - Read goodix touchscreen version
  *
- * @client: the i2c client
- * @version: output buffer containing the version on success
- * @id: output buffer containing the id on success
+ * @ts: our goodix_ts_data pointer
  */
-static int goodix_read_version(struct i2c_client *client, u16 *version, u16 *id)
+static int goodix_read_version(struct goodix_ts_data *ts)
 {
 	int error;
 	u8 buf[6];
 	char id_str[5];
 
-	error = goodix_i2c_read(client, GOODIX_REG_ID, buf, sizeof(buf));
+	error = goodix_i2c_read(ts->client, GOODIX_REG_ID, buf, sizeof(buf));
 	if (error) {
-		dev_err(&client->dev, "read version failed: %d\n", error);
+		dev_err(&ts->client->dev, "read version failed: %d\n", error);
 		return error;
 	}
 
 	memcpy(id_str, buf, 4);
 	id_str[4] = 0;
-	if (kstrtou16(id_str, 10, id))
-		*id = 0x1001;
+	if (kstrtou16(id_str, 10, &ts->id))
+		ts->id = 0x1001;
 
-	*version = get_unaligned_le16(&buf[4]);
+	ts->version = get_unaligned_le16(&buf[4]);
 
-	dev_info(&client->dev, "ID %d, version: %04x\n", *id, *version);
+	dev_info(&ts->client->dev, "ID %d, version: %04x\n", ts->id,
+		 ts->version);
 
 	return 0;
 }
@@ -428,13 +531,10 @@ 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, u16 version,
-				    u16 id)
+static int goodix_request_input_dev(struct goodix_ts_data *ts)
 {
 	int error;
 
@@ -458,8 +558,8 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts, u16 version,
 	ts->input_dev->phys = "input/ts";
 	ts->input_dev->id.bustype = BUS_I2C;
 	ts->input_dev->id.vendor = 0x0416;
-	ts->input_dev->id.product = id;
-	ts->input_dev->id.version = version;
+	ts->input_dev->id.product = ts->id;
+	ts->input_dev->id.version = ts->version;
 
 	error = input_register_device(ts->input_dev);
 	if (error) {
@@ -471,13 +571,70 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts, u16 version,
 	return 0;
 }
 
+/**
+ * goodix_configure_dev - Finish device initialization
+ *
+ * @ts: our goodix_ts_data pointer
+ *
+ * Must be called from probe to finish initialization of the device.
+ * Contains the common initialization code for both devices that
+ * declare gpio pins and devices that do not. It is either called
+ * directly from probe or from request_firmware_wait callback.
+ */
+static int goodix_configure_dev(struct goodix_ts_data *ts)
+{
+	int error;
+	unsigned long irq_flags;
+
+	goodix_read_config(ts);
+
+	error = goodix_request_input_dev(ts);
+	if (error)
+		return error;
+
+	irq_flags = goodix_irq_flags[ts->int_trigger_type] | IRQF_ONESHOT;
+	error = devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
+					  NULL, goodix_ts_irq_handler,
+					  irq_flags, ts->client->name, ts);
+	if (error) {
+		dev_err(&ts->client->dev, "request IRQ failed: %d\n", error);
+		return error;
+	}
+
+	return 0;
+}
+
+/**
+ * goodix_config_cb - Callback to finish device init
+ *
+ * @ts: our goodix_ts_data pointer
+ *
+ * request_firmware_wait callback that finishes
+ * initialization of the device.
+ */
+static void goodix_config_cb(const struct firmware *cfg, void *ctx)
+{
+	struct goodix_ts_data *ts = (struct goodix_ts_data *)ctx;
+	int error;
+
+	if (cfg) {
+		/* send device configuration to the firmware */
+		error = goodix_send_cfg(ts, cfg);
+		if (error)
+			goto err_release_cfg;
+	}
+	goodix_configure_dev(ts);
+
+err_release_cfg:
+	kfree(ts->cfg_name);
+	release_firmware(cfg);
+}
+
 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;
 
 	dev_dbg(&client->dev, "I2C Address: 0x%02x\n", client->addr);
 
@@ -512,30 +669,36 @@ static int goodix_ts_probe(struct i2c_client *client,
 		return error;
 	}
 
-	error = goodix_read_version(client, &version_info, &id_info);
+	error = goodix_read_version(ts);
 	if (error) {
 		dev_err(&client->dev, "Read version failed.\n");
 		return error;
 	}
 
-	ts->cfg_len = goodix_get_cfg_len(id_info);
+	ts->cfg_len = goodix_get_cfg_len(ts->id);
 
-	goodix_read_config(ts);
-
-	error = goodix_request_input_dev(ts, version_info, id_info);
-	if (error)
-		return error;
+	if (ts->gpiod_int && ts->gpiod_rst) {
+		/* update device config */
+		ts->cfg_name = kasprintf(GFP_KERNEL, "goodix_%d_cfg.bin",
+					 ts->id);
+		if (!ts->cfg_name)
+			return -ENOMEM;
+
+		error = request_firmware_nowait(THIS_MODULE, true, ts->cfg_name,
+						&client->dev, GFP_KERNEL, ts,
+						goodix_config_cb);
+		if (error) {
+			dev_err(&client->dev,
+				"Failed to invoke firmware loader: %d\n",
+				error);
+			kfree(ts->cfg_name);
+			return error;
+		}
 
-	irq_flags = goodix_irq_flags[ts->int_trigger_type] | IRQF_ONESHOT;
-	error = devm_request_threaded_irq(&ts->client->dev, client->irq,
-					  NULL, goodix_ts_irq_handler,
-					  irq_flags, client->name, ts);
-	if (error) {
-		dev_err(&client->dev, "request IRQ failed: %d\n", error);
-		return error;
+		return 0;
 	}
 
-	return 0;
+	return goodix_configure_dev(ts);
 }
 
 static const struct i2c_device_id goodix_ts_id[] = {
-- 
1.9.1

[PATCH v9 4/9] Input: goodix - add power management support

[Irina Tirdea]

Implement suspend/resume for goodix driver.

The suspend and resume process uses the gpio pins.
If the device ACPI/DT information does not declare gpio pins,
suspend/resume will not be available for these devices.

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>
Tested-by: Bastien Nocera <hadess@hadess.net>
Tested-by: Aleksei Mamlin <mamlinav@gmail.com>
---
 drivers/input/touchscreen/goodix.c | 94 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 89 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index b4dfe4b..6314a24 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -44,6 +44,7 @@ struct goodix_ts_data {
 	u16 id;
 	u16 version;
 	char *cfg_name;
+	unsigned long irq_flags;
 };
 
 #define GOODIX_GPIO_INT_NAME		"irq"
@@ -60,6 +61,9 @@ struct goodix_ts_data {
 #define GOODIX_CONFIG_967_LENGTH	228
 
 /* 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
@@ -161,6 +165,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_get_cfg_len(u16 id)
 {
 	switch (id) {
@@ -280,6 +289,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 fw is valid
  *
@@ -584,7 +605,6 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts)
 static int goodix_configure_dev(struct goodix_ts_data *ts)
 {
 	int error;
-	unsigned long irq_flags;
 
 	goodix_read_config(ts);
 
@@ -592,10 +612,8 @@ static int goodix_configure_dev(struct goodix_ts_data *ts)
 	if (error)
 		return error;
 
-	irq_flags = goodix_irq_flags[ts->int_trigger_type] | IRQF_ONESHOT;
-	error = devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
-					  NULL, goodix_ts_irq_handler,
-					  irq_flags, ts->client->name, ts);
+	ts->irq_flags = goodix_irq_flags[ts->int_trigger_type] | IRQF_ONESHOT;
+	error = goodix_request_irq(ts);
 	if (error) {
 		dev_err(&ts->client->dev, "request IRQ failed: %d\n", error);
 		return error;
@@ -701,6 +719,71 @@ static int goodix_ts_probe(struct i2c_client *client,
 	return goodix_configure_dev(ts);
 }
 
+static int __maybe_unused goodix_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct goodix_ts_data *ts = i2c_get_clientdata(client);
+	int error;
+
+	/* We need gpio pins to suspend/resume */
+	if (!ts->gpiod_int || !ts->gpiod_rst)
+		return 0;
+
+	/* Free IRQ as IRQ pin is used as output in the suspend sequence */
+	goodix_free_irq(ts);
+	/* Output LOW on the INT pin for 5 ms */
+	error = gpiod_direction_output(ts->gpiod_int, 0);
+	if (error) {
+		goodix_request_irq(ts);
+		return error;
+	}
+	usleep_range(5000, 6000);
+
+	error = goodix_i2c_write_u8(ts->client, GOODIX_REG_COMMAND,
+				    GOODIX_CMD_SCREEN_OFF);
+	if (error) {
+		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 __maybe_unused goodix_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct goodix_ts_data *ts = i2c_get_clientdata(client);
+	int error;
+
+	if (!ts->gpiod_int || !ts->gpiod_rst)
+		return 0;
+
+	/*
+	 * Exit sleep mode by outputting HIGH level to INT pin
+	 * for 2ms~5ms.
+	 */
+	error = gpiod_direction_output(ts->gpiod_int, 1);
+	if (error)
+		return error;
+	usleep_range(2000, 5000);
+
+	error = goodix_int_sync(ts);
+	if (error)
+		return error;
+
+	return goodix_request_irq(ts);
+}
+
+static SIMPLE_DEV_PM_OPS(goodix_pm_ops, goodix_suspend, goodix_resume);
+
 static const struct i2c_device_id goodix_ts_id[] = {
 	{ "GDIX1001:00", 0 },
 	{ }
@@ -736,6 +819,7 @@ static struct i2c_driver goodix_ts_driver = {
 		.name = "Goodix-TS",
 		.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 v9 5/9] 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>
Tested-by: Bastien Nocera <hadess@hadess.net>
Tested-by: Aleksei Mamlin <mamlinav@gmail.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 6314a24..c86c265 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -274,16 +274,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 v9 6/9] 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.

The ESD poll time (in ms) can be set through the sysfs property
esd_timeout. If it is set to 0, ESD protection is disabled.
Recommended value is 2000 ms. The initial value for ESD timeout
can be set through esd-recovery-timeout-ms ACPI/DT property.
If there is no such property defined, 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 }},
             ...
         }
     })
 }

The ESD protection mechanism is only available if the gpio pins
are properly initialized from ACPI/DT.

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: Irina Tirdea <irina.tirdea@intel.com>
Tested-by: Bastien Nocera <hadess@hadess.net>
Tested-by: Aleksei Mamlin <mamlinav@gmail.com>
---
 .../bindings/input/touchscreen/goodix.txt          |   6 +
 drivers/input/touchscreen/goodix.c                 | 180 ++++++++++++++++++++-
 2 files changed, 178 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index 7137881..4db3393 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -14,6 +14,12 @@ Required properties:
  - interrupts		: Interrupt to which the chip is connected
  - 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:
 
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index c86c265..a271df9 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -45,10 +45,13 @@ struct goodix_ts_data {
 	u16 version;
 	char *cfg_name;
 	unsigned long irq_flags;
+	atomic_t esd_timeout;
+	struct delayed_work esd_work;
 };
 
 #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
@@ -63,6 +66,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
@@ -405,6 +410,117 @@ static int goodix_reset(struct goodix_ts_data *ts)
 	return goodix_int_sync(ts);
 }
 
+static void goodix_disable_esd(struct goodix_ts_data *ts)
+{
+	if (!atomic_read(&ts->esd_timeout))
+		return;
+	cancel_delayed_work_sync(&ts->esd_work);
+}
+
+static int goodix_enable_esd(struct goodix_ts_data *ts)
+{
+	int error, esd_timeout;
+
+	esd_timeout = atomic_read(&ts->esd_timeout);
+	if (!esd_timeout)
+		return 0;
+
+	error = goodix_i2c_write_u8(ts->client, GOODIX_REG_ESD_CHECK,
+				    GOODIX_CMD_ESD_ENABLED);
+	if (error) {
+		dev_err(&ts->client->dev, "Failed to enable ESD: %d\n", error);
+		return error;
+	}
+
+	schedule_delayed_work(&ts->esd_work, round_jiffies_relative(
+			      msecs_to_jiffies(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, error;
+	u8 esd_data[2];
+	const struct firmware *cfg = NULL;
+
+	while (--retries) {
+		error = goodix_i2c_read(ts->client, GOODIX_REG_COMMAND,
+					esd_data, sizeof(esd_data));
+		if (error)
+			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);
+		goodix_reset(ts);
+		error = request_firmware(&cfg, ts->cfg_name, &ts->client->dev);
+		if (!error) {
+			goodix_send_cfg(ts, cfg);
+			release_firmware(cfg);
+		}
+		goodix_request_irq(ts);
+		goodix_enable_esd(ts);
+		return;
+	}
+
+	schedule_delayed_work(&ts->esd_work, round_jiffies_relative(
+			      msecs_to_jiffies(atomic_read(&ts->esd_timeout))));
+}
+
+static ssize_t goodix_esd_timeout_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct goodix_ts_data *ts = dev_get_drvdata(dev);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", atomic_read(&ts->esd_timeout));
+}
+
+static ssize_t goodix_esd_timeout_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count)
+{
+	struct goodix_ts_data *ts = dev_get_drvdata(dev);
+	int error, esd_timeout, new_esd_timeout;
+
+	error = kstrtouint(buf, 10, &new_esd_timeout);
+	if (error)
+		return error;
+
+	esd_timeout = atomic_read(&ts->esd_timeout);
+	if (esd_timeout && !new_esd_timeout)
+		goodix_disable_esd(ts);
+
+	atomic_set(&ts->esd_timeout, new_esd_timeout);
+	if (!esd_timeout && new_esd_timeout)
+		goodix_enable_esd(ts);
+
+	return count;
+}
+
+/* ESD timeout in ms. Default disabled (0). Recommended 2000 ms. */
+static DEVICE_ATTR(esd_timeout, S_IRUGO | S_IWUSR, goodix_esd_timeout_show,
+		   goodix_esd_timeout_store);
+
+static struct attribute *goodix_attrs[] = {
+	&dev_attr_esd_timeout.attr,
+	NULL
+};
+
+static const struct attribute_group goodix_attr_group = {
+	.attrs = goodix_attrs,
+};
+
 /**
  * goodix_get_gpio_config - Get GPIO config from ACPI/DT
  *
@@ -636,10 +752,13 @@ static void goodix_config_cb(const struct firmware *cfg, void *ctx)
 		if (error)
 			goto err_release_cfg;
 	}
-	goodix_configure_dev(ts);
+	error = goodix_configure_dev(ts);
+	if (error)
+		goto err_release_cfg;
+
+	goodix_enable_esd(ts);
 
 err_release_cfg:
-	kfree(ts->cfg_name);
 	release_firmware(cfg);
 }
 
@@ -647,7 +766,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id)
 {
 	struct goodix_ts_data *ts;
-	int error;
+	int error, esd_timeout;
 
 	dev_dbg(&client->dev, "I2C Address: 0x%02x\n", client->addr);
 
@@ -662,6 +781,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_get_gpio_config(ts);
 	if (error == -EPROBE_DEFER)
@@ -691,11 +811,28 @@ static int goodix_ts_probe(struct i2c_client *client,
 	ts->cfg_len = goodix_get_cfg_len(ts->id);
 
 	if (ts->gpiod_int && ts->gpiod_rst) {
+		error = device_property_read_u32(&ts->client->dev,
+					GOODIX_DEVICE_ESD_TIMEOUT_PROPERTY,
+					&esd_timeout);
+		if (!error)
+			atomic_set(&ts->esd_timeout, esd_timeout);
+
+		error = sysfs_create_group(&client->dev.kobj,
+					   &goodix_attr_group);
+		if (error) {
+			dev_err(&client->dev,
+				"Failed to create sysfs group: %d\n",
+				error);
+			return error;
+		}
+
 		/* update device config */
 		ts->cfg_name = kasprintf(GFP_KERNEL, "goodix_%d_cfg.bin",
 					 ts->id);
-		if (!ts->cfg_name)
-			return -ENOMEM;
+		if (!ts->cfg_name) {
+			error = -ENOMEM;
+			goto err_sysfs_remove_group;
+		}
 
 		error = request_firmware_nowait(THIS_MODULE, true, ts->cfg_name,
 						&client->dev, GFP_KERNEL, ts,
@@ -704,14 +841,35 @@ static int goodix_ts_probe(struct i2c_client *client,
 			dev_err(&client->dev,
 				"Failed to invoke firmware loader: %d\n",
 				error);
-			kfree(ts->cfg_name);
-			return error;
+			goto err_free_cfg_name;
 		}
 
 		return 0;
 	}
 
 	return goodix_configure_dev(ts);
+
+err_free_cfg_name:
+	if (ts->gpiod_int && ts->gpiod_rst)
+		kfree(ts->cfg_name);
+err_sysfs_remove_group:
+	if (ts->gpiod_int && ts->gpiod_rst)
+		sysfs_remove_group(&client->dev.kobj, &goodix_attr_group);
+	return error;
+}
+
+static int goodix_ts_remove(struct i2c_client *client)
+{
+	struct goodix_ts_data *ts = i2c_get_clientdata(client);
+
+	if (!ts->gpiod_int || !ts->gpiod_rst)
+		return 0;
+
+	sysfs_remove_group(&client->dev.kobj, &goodix_attr_group);
+	goodix_disable_esd(ts);
+	kfree(ts->cfg_name);
+
+	return 0;
 }
 
 static int __maybe_unused goodix_suspend(struct device *dev)
@@ -724,6 +882,7 @@ static int __maybe_unused goodix_suspend(struct device *dev)
 	if (!ts->gpiod_int || !ts->gpiod_rst)
 		return 0;
 
+	goodix_disable_esd(ts);
 	/* Free IRQ as IRQ pin is used as output in the suspend sequence */
 	goodix_free_irq(ts);
 	/* Output LOW on the INT pin for 5 ms */
@@ -774,7 +933,11 @@ static int __maybe_unused goodix_resume(struct device *dev)
 	if (error)
 		return error;
 
-	return goodix_request_irq(ts);
+	error = goodix_request_irq(ts);
+	if (error)
+		return error;
+
+	return goodix_enable_esd(ts);
 }
 
 static SIMPLE_DEV_PM_OPS(goodix_pm_ops, goodix_suspend, goodix_resume);
@@ -809,6 +972,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

[PATCH v9 7/9] Input: goodix - add sysfs interface to dump config

[Irina Tirdea]

Goodix devices have a configuration information register area that
specify various parameters for the device. 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).

Export a sysfs interface that would allow reading the configuration
information. The default device configuration can be used as a starting
point for creating a valid configuration firmware used by the device at
init time to update its configuration.

This sysfs interface will be exported only if the gpio pins are properly
initialized from ACPI/DT.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Tested-by: Bastien Nocera <hadess@hadess.net>
Tested-by: Aleksei Mamlin <mamlinav@gmail.com>
---
 drivers/input/touchscreen/goodix.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index a271df9..01a2634 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -508,12 +508,35 @@ static ssize_t goodix_esd_timeout_store(struct device *dev,
 	return count;
 }
 
+static ssize_t goodix_dump_config_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct goodix_ts_data *ts = dev_get_drvdata(dev);
+	u8 config[GOODIX_CONFIG_MAX_LENGTH];
+	int error, count = 0, i;
+
+	error = goodix_i2c_read(ts->client, GOODIX_REG_CONFIG_DATA,
+				config, ts->cfg_len);
+	if (error) {
+		dev_warn(&ts->client->dev,
+			 "Error reading config (%d)\n",  error);
+		return error;
+	}
+
+	for (i = 0; i < ts->cfg_len; i++)
+		count += scnprintf(buf + count, PAGE_SIZE - count, "%02x ",
+				   config[i]);
+	return count;
+}
+
 /* ESD timeout in ms. Default disabled (0). Recommended 2000 ms. */
 static DEVICE_ATTR(esd_timeout, S_IRUGO | S_IWUSR, goodix_esd_timeout_show,
 		   goodix_esd_timeout_store);
+static DEVICE_ATTR(dump_config, S_IRUGO, goodix_dump_config_show, NULL);
 
 static struct attribute *goodix_attrs[] = {
 	&dev_attr_esd_timeout.attr,
+	&dev_attr_dump_config.attr,
 	NULL
 };
 
-- 
1.9.1

[PATCH v9 8/9] Input: goodix - add runtime power management support

[Irina Tirdea]

Add support for runtime power management so that the device is
turned off when not used (when the userspace holds no open
handles of the input device). The device uses autosuspend with a
default delay of 2 seconds, so the device will suspend if no
handles to it are open for 2 seconds.

The runtime management support is only available if the gpio pins
are properly initialized from ACPI/DT.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Tested-by: Bastien Nocera <hadess@hadess.net>
Tested-by: Aleksei Mamlin <mamlinav@gmail.com>
---
 drivers/input/touchscreen/goodix.c | 152 +++++++++++++++++++++++++++++++++----
 1 file changed, 138 insertions(+), 14 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 01a2634..b958b37 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 #include <linux/acpi.h>
 #include <linux/of.h>
+#include <linux/pm_runtime.h>
 #include <asm/unaligned.h>
 
 struct goodix_ts_data {
@@ -47,6 +48,10 @@ struct goodix_ts_data {
 	unsigned long irq_flags;
 	atomic_t esd_timeout;
 	struct delayed_work esd_work;
+	bool suspended;
+	atomic_t open_count;
+	/* Protects power management calls and access to suspended flag */
+	struct mutex mutex;
 };
 
 #define GOODIX_GPIO_INT_NAME		"irq"
@@ -77,6 +82,8 @@ struct goodix_ts_data {
 #define MAX_CONTACTS_LOC	5
 #define TRIGGER_LOC		6
 
+#define GOODIX_AUTOSUSPEND_DELAY_MS	2000
+
 static const unsigned long goodix_irq_flags[] = {
 	IRQ_TYPE_EDGE_RISING,
 	IRQ_TYPE_EDGE_FALLING,
@@ -192,6 +199,32 @@ static int goodix_get_cfg_len(u16 id)
 	}
 }
 
+static int goodix_set_power_state(struct goodix_ts_data *ts, bool on)
+{
+	int error;
+
+	if (!ts->gpiod_int || !ts->gpiod_rst)
+		return 0;
+
+	if (on) {
+		error = pm_runtime_get_sync(&ts->client->dev);
+	} else {
+		pm_runtime_mark_last_busy(&ts->client->dev);
+		error = pm_runtime_put_autosuspend(&ts->client->dev);
+	}
+
+	if (error < 0) {
+		dev_err(&ts->client->dev,
+			"failed to change power state to %d\n", on);
+		if (on)
+			pm_runtime_put_noidle(&ts->client->dev);
+
+		return error;
+	}
+
+	return 0;
+}
+
 static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
 {
 	int touch_num;
@@ -498,11 +531,13 @@ static ssize_t goodix_esd_timeout_store(struct device *dev,
 		return error;
 
 	esd_timeout = atomic_read(&ts->esd_timeout);
-	if (esd_timeout && !new_esd_timeout)
+	if (esd_timeout && !new_esd_timeout &&
+	    pm_runtime_active(&ts->client->dev))
 		goodix_disable_esd(ts);
 
 	atomic_set(&ts->esd_timeout, new_esd_timeout);
-	if (!esd_timeout && new_esd_timeout)
+	if (!esd_timeout && new_esd_timeout &&
+	    pm_runtime_active(&ts->client->dev))
 		goodix_enable_esd(ts);
 
 	return count;
@@ -515,17 +550,23 @@ static ssize_t goodix_dump_config_show(struct device *dev,
 	u8 config[GOODIX_CONFIG_MAX_LENGTH];
 	int error, count = 0, i;
 
+	error = goodix_set_power_state(ts, true);
+	if (error)
+		return error;
 	error = goodix_i2c_read(ts->client, GOODIX_REG_CONFIG_DATA,
 				config, ts->cfg_len);
 	if (error) {
 		dev_warn(&ts->client->dev,
 			 "Error reading config (%d)\n",  error);
+		goodix_set_power_state(ts, false);
 		return error;
 	}
+	goodix_set_power_state(ts, false);
 
 	for (i = 0; i < ts->cfg_len; i++)
 		count += scnprintf(buf + count, PAGE_SIZE - count, "%02x ",
 				   config[i]);
+
 	return count;
 }
 
@@ -544,6 +585,26 @@ static const struct attribute_group goodix_attr_group = {
 	.attrs = goodix_attrs,
 };
 
+static int goodix_open(struct input_dev *input_dev)
+{
+	struct goodix_ts_data *ts = input_get_drvdata(input_dev);
+	int error;
+
+	error = goodix_set_power_state(ts, true);
+	if (error)
+		return error;
+	atomic_inc(&ts->open_count);
+	return 0;
+}
+
+static void goodix_close(struct input_dev *input_dev)
+{
+	struct goodix_ts_data *ts = input_get_drvdata(input_dev);
+
+	goodix_set_power_state(ts, false);
+	atomic_dec(&ts->open_count);
+}
+
 /**
  * goodix_get_gpio_config - Get GPIO config from ACPI/DT
  *
@@ -715,6 +776,9 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts)
 	ts->input_dev->id.vendor = 0x0416;
 	ts->input_dev->id.product = ts->id;
 	ts->input_dev->id.version = ts->version;
+	ts->input_dev->open = goodix_open;
+	ts->input_dev->close = goodix_close;
+	input_set_drvdata(ts->input_dev, ts);
 
 	error = input_register_device(ts->input_dev);
 	if (error) {
@@ -762,7 +826,8 @@ static int goodix_configure_dev(struct goodix_ts_data *ts)
  * @ts: our goodix_ts_data pointer
  *
  * request_firmware_wait callback that finishes
- * initialization of the device.
+ * initialization of the device. This will only be called
+ * when ts->gpiod_int and ts->gpiod_rst are properly initialized.
  */
 static void goodix_config_cb(const struct firmware *cfg, void *ctx)
 {
@@ -781,6 +846,19 @@ static void goodix_config_cb(const struct firmware *cfg, void *ctx)
 
 	goodix_enable_esd(ts);
 
+	pm_runtime_set_autosuspend_delay(&ts->client->dev,
+					 GOODIX_AUTOSUSPEND_DELAY_MS);
+	pm_runtime_use_autosuspend(&ts->client->dev);
+	error = pm_runtime_set_active(&ts->client->dev);
+	if (error) {
+		dev_err(&ts->client->dev, "failed to set active: %d\n", error);
+		goto err_release_cfg;
+	}
+	pm_runtime_enable(&ts->client->dev);
+	/* Must not suspend immediately after device initialization */
+	pm_runtime_mark_last_busy(&ts->client->dev);
+	pm_request_autosuspend(&ts->client->dev);
+
 err_release_cfg:
 	release_firmware(cfg);
 }
@@ -805,6 +883,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);
+	mutex_init(&ts->mutex);
 
 	error = goodix_get_gpio_config(ts);
 	if (error == -EPROBE_DEFER)
@@ -888,6 +967,9 @@ static int goodix_ts_remove(struct i2c_client *client)
 	if (!ts->gpiod_int || !ts->gpiod_rst)
 		return 0;
 
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+	pm_runtime_put_noidle(&client->dev);
 	sysfs_remove_group(&client->dev.kobj, &goodix_attr_group);
 	goodix_disable_esd(ts);
 	kfree(ts->cfg_name);
@@ -895,16 +977,21 @@ static int goodix_ts_remove(struct i2c_client *client)
 	return 0;
 }
 
-static int __maybe_unused goodix_suspend(struct device *dev)
+static int __maybe_unused goodix_sleep(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct goodix_ts_data *ts = i2c_get_clientdata(client);
-	int error;
+	int error = 0;
 
 	/* We need gpio pins to suspend/resume */
 	if (!ts->gpiod_int || !ts->gpiod_rst)
 		return 0;
 
+	mutex_lock(&ts->mutex);
+
+	if (ts->suspended)
+		goto out_error;
+
 	goodix_disable_esd(ts);
 	/* Free IRQ as IRQ pin is used as output in the suspend sequence */
 	goodix_free_irq(ts);
@@ -912,7 +999,7 @@ static int __maybe_unused goodix_suspend(struct device *dev)
 	error = gpiod_direction_output(ts->gpiod_int, 0);
 	if (error) {
 		goodix_request_irq(ts);
-		return error;
+		goto out_error;
 	}
 	usleep_range(5000, 6000);
 
@@ -922,7 +1009,8 @@ static int __maybe_unused goodix_suspend(struct device *dev)
 		dev_err(&ts->client->dev, "Screen off command failed\n");
 		gpiod_direction_input(ts->gpiod_int);
 		goodix_request_irq(ts);
-		return -EAGAIN;
+		error = -EAGAIN;
+		goto out_error;
 	}
 
 	/*
@@ -931,39 +1019,75 @@ static int __maybe_unused goodix_suspend(struct device *dev)
 	 * sooner, delay 58ms here.
 	 */
 	msleep(58);
+	ts->suspended = true;
+	mutex_unlock(&ts->mutex);
 	return 0;
+
+out_error:
+	mutex_unlock(&ts->mutex);
+	return error;
 }
 
-static int __maybe_unused goodix_resume(struct device *dev)
+static int __maybe_unused goodix_wakeup(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct goodix_ts_data *ts = i2c_get_clientdata(client);
-	int error;
+	int error = 0;
 
 	if (!ts->gpiod_int || !ts->gpiod_rst)
 		return 0;
 
+	mutex_lock(&ts->mutex);
+
+	if (!ts->suspended)
+		goto out_error;
+
 	/*
 	 * Exit sleep mode by outputting HIGH level to INT pin
 	 * for 2ms~5ms.
 	 */
 	error = gpiod_direction_output(ts->gpiod_int, 1);
 	if (error)
-		return error;
+		goto out_error;
 	usleep_range(2000, 5000);
 
 	error = goodix_int_sync(ts);
 	if (error)
-		return error;
+		goto out_error;
 
 	error = goodix_request_irq(ts);
 	if (error)
-		return error;
+		goto out_error;
 
-	return goodix_enable_esd(ts);
+	error = goodix_enable_esd(ts);
+	if (error)
+		goto out_error;
+
+	ts->suspended = false;
+	mutex_unlock(&ts->mutex);
+
+	return 0;
+
+out_error:
+	mutex_unlock(&ts->mutex);
+	return error;
+}
+
+static int __maybe_unused goodix_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct goodix_ts_data *ts = i2c_get_clientdata(client);
+
+	if (!atomic_read(&ts->open_count))
+		return 0;
+
+	return goodix_wakeup(dev);
 }
 
-static SIMPLE_DEV_PM_OPS(goodix_pm_ops, goodix_suspend, goodix_resume);
+static const struct dev_pm_ops goodix_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(goodix_sleep, goodix_resume)
+	SET_RUNTIME_PM_OPS(goodix_sleep, goodix_wakeup, NULL)
+};
 
 static const struct i2c_device_id goodix_ts_id[] = {
 	{ "GDIX1001:00", 0 },
-- 
1.9.1

[PATCH v9 9/9] Input: goodix - sort includes using inverse Xmas tree order

[Irina Tirdea]

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Tested-by: Bastien Nocera <hadess@hadess.net>
Tested-by: Aleksei Mamlin <mamlinav@gmail.com>
---
 drivers/input/touchscreen/goodix.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index b958b37..22bfc4b 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -14,21 +14,22 @@
  * Software Foundation; version 2 of the License.
  */
 
-#include <linux/kernel.h>
-#include <linux/dmi.h>
+#include <linux/pm_runtime.h>
+#include <linux/interrupt.h>
 #include <linux/firmware.h>
-#include <linux/gpio.h>
-#include <linux/i2c.h>
-#include <linux/input.h>
 #include <linux/input/mt.h>
+#include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/delay.h>
-#include <linux/irq.h>
-#include <linux/interrupt.h>
-#include <linux/slab.h>
+#include <linux/input.h>
 #include <linux/acpi.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/dmi.h>
+#include <linux/i2c.h>
+#include <linux/irq.h>
 #include <linux/of.h>
-#include <linux/pm_runtime.h>
+
 #include <asm/unaligned.h>
 
 struct goodix_ts_data {
-- 
1.9.1

Re: [PATCH v9 9/9] Input: goodix - sort includes using inverse Xmas tree order

[Mark Rutland]

Why?

Mark.

On Mon, Oct 12, 2015 at 06:24:37PM +0300, Irina Tirdea wrote:
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> Tested-by: Bastien Nocera <hadess@hadess.net>
> Tested-by: Aleksei Mamlin <mamlinav@gmail.com>
> ---
>  drivers/input/touchscreen/goodix.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index b958b37..22bfc4b 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -14,21 +14,22 @@
>   * Software Foundation; version 2 of the License.
>   */
>  
> -#include <linux/kernel.h>
> -#include <linux/dmi.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/interrupt.h>
>  #include <linux/firmware.h>
> -#include <linux/gpio.h>
> -#include <linux/i2c.h>
> -#include <linux/input.h>
>  #include <linux/input/mt.h>
> +#include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/delay.h>
> -#include <linux/irq.h>
> -#include <linux/interrupt.h>
> -#include <linux/slab.h>
> +#include <linux/input.h>
>  #include <linux/acpi.h>
> +#include <linux/gpio.h>
> +#include <linux/slab.h>
> +#include <linux/dmi.h>
> +#include <linux/i2c.h>
> +#include <linux/irq.h>
>  #include <linux/of.h>
> -#include <linux/pm_runtime.h>
> +
>  #include <asm/unaligned.h>
>  
>  struct goodix_ts_data {
> -- 
> 1.9.1
> 

Re: [PATCH v9 9/9] Input: goodix - sort includes using inverse Xmas tree order

[Mark Rutland]

Why?

Mark.

On Mon, Oct 12, 2015 at 06:24:37PM +0300, Irina Tirdea wrote:
> Signed-off-by: Irina Tirdea <irina.tirdea-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Tested-by: Bastien Nocera <hadess-0MeiytkfxGOsTnJN9+BGXg@public.gmane.org>
> Tested-by: Aleksei Mamlin <mamlinav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/input/touchscreen/goodix.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index b958b37..22bfc4b 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -14,21 +14,22 @@
>   * Software Foundation; version 2 of the License.
>   */
>  
> -#include <linux/kernel.h>
> -#include <linux/dmi.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/interrupt.h>
>  #include <linux/firmware.h>
> -#include <linux/gpio.h>
> -#include <linux/i2c.h>
> -#include <linux/input.h>
>  #include <linux/input/mt.h>
> +#include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/delay.h>
> -#include <linux/irq.h>
> -#include <linux/interrupt.h>
> -#include <linux/slab.h>
> +#include <linux/input.h>
>  #include <linux/acpi.h>
> +#include <linux/gpio.h>
> +#include <linux/slab.h>
> +#include <linux/dmi.h>
> +#include <linux/i2c.h>
> +#include <linux/irq.h>
>  #include <linux/of.h>
> -#include <linux/pm_runtime.h>
> +
>  #include <asm/unaligned.h>
>  
>  struct goodix_ts_data {
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 9/9] Input: goodix - sort includes using inverse Xmas tree order

[Bastien Nocera]

On Mon, 2015-10-12 at 16:39 +0100, Mark Rutland wrote:
> Why?

It was already discussed in the thread for a previous version, please
refer to that.

Re: [PATCH v9 9/9] Input: goodix - sort includes using inverse Xmas tree order

[Mark Rutland]

On Mon, Oct 12, 2015 at 05:40:37PM +0200, Bastien Nocera wrote:
> On Mon, 2015-10-12 at 16:39 +0100, Mark Rutland wrote:
> > Why?
> 
> It was already discussed in the thread for a previous version, please
> refer to that.

Fine, but surely that should be in the commit message, to prevent others
like myself repeatedly asking the same question?

Mark.

Re: [PATCH v9 9/9] Input: goodix - sort includes using inverse Xmas tree order

[Mark Rutland]

On Mon, Oct 12, 2015 at 05:40:37PM +0200, Bastien Nocera wrote:
> On Mon, 2015-10-12 at 16:39 +0100, Mark Rutland wrote:
> > Why?
> 
> It was already discussed in the thread for a previous version, please
> refer to that.

Fine, but surely that should be in the commit message, to prevent others
like myself repeatedly asking the same question?

Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 9/9] Input: goodix - sort includes using inverse Xmas tree order

[Bastien Nocera]

On Mon, 2015-10-12 at 16:51 +0100, Mark Rutland wrote:
> On Mon, Oct 12, 2015 at 05:40:37PM +0200, Bastien Nocera wrote:
> > On Mon, 2015-10-12 at 16:39 +0100, Mark Rutland wrote:
> > > Why?
> > 
> > It was already discussed in the thread for a previous version,
> > please
> > refer to that.
> 
> Fine, but surely that should be in the commit message, to prevent
> others
> like myself repeatedly asking the same question?

Fair point. Irina?

Re: [PATCH v9 9/9] Input: goodix - sort includes using inverse Xmas tree order

[Dmitry Torokhov]

On Mon, Oct 12, 2015 at 8:53 AM, Bastien Nocera <hadess@hadess.net> wrote:
> On Mon, 2015-10-12 at 16:51 +0100, Mark Rutland wrote:
>> On Mon, Oct 12, 2015 at 05:40:37PM +0200, Bastien Nocera wrote:
>> > On Mon, 2015-10-12 at 16:39 +0100, Mark Rutland wrote:
>> > > Why?
>> >
>> > It was already discussed in the thread for a previous version,
>> > please
>> > refer to that.
>>
>> Fine, but surely that should be in the commit message, to prevent
>> others
>> like myself repeatedly asking the same question?
>
> Fair point. Irina?

No, let's just leave poor includes alone.

-- 
Dmitry

Re: [PATCH v9 9/9] Input: goodix - sort includes using inverse Xmas tree order

[Dmitry Torokhov]

On Mon, Oct 12, 2015 at 8:53 AM, Bastien Nocera <hadess-0MeiytkfxGOsTnJN9+BGXg@public.gmane.org> wrote:
> On Mon, 2015-10-12 at 16:51 +0100, Mark Rutland wrote:
>> On Mon, Oct 12, 2015 at 05:40:37PM +0200, Bastien Nocera wrote:
>> > On Mon, 2015-10-12 at 16:39 +0100, Mark Rutland wrote:
>> > > Why?
>> >
>> > It was already discussed in the thread for a previous version,
>> > please
>> > refer to that.
>>
>> Fine, but surely that should be in the commit message, to prevent
>> others
>> like myself repeatedly asking the same question?
>
> Fair point. Irina?

No, let's just leave poor includes alone.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 2/9] Input: goodix - reset device at init

[Dmitry Torokhov]

On Mon, Oct 12, 2015 at 06:24:30PM +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 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/device tree). For devices
> that do not have the gpio pins properly declared, the functionality
> depending on these pins will not be available, but the device can still
> be used with basic functionality.
> 
> For both device tree and ACPI, the interrupt gpio pin configuration is
> read from the "irq-gpio" property and the reset pin configuration is
> read from the "reset-gpio" property. For ACPI 5.1, named properties
> can be specified using the _DSD section. This functionality will not be
> available for devices that use indexed gpio pins declared in the _CRS
> section (we need to provide backward compatibility with devices
> that do not support using the interrupt gpio pin as output).
> 
> 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>
> Acked-by: Bastien Nocera <hadess@hadess.net>
> Tested-by: Bastien Nocera <hadess@hadess.net>
> Tested-by: Aleksei Mamlin <mamlinav@gmail.com>
> ---
>  .../bindings/input/touchscreen/goodix.txt          |   5 +
>  drivers/input/touchscreen/Kconfig                  |   1 +
>  drivers/input/touchscreen/goodix.c                 | 101 +++++++++++++++++++++
>  3 files changed, 107 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/Kconfig b/drivers/input/touchscreen/Kconfig
> index 771d95c..76f5a9d 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -324,6 +324,7 @@ config TOUCHSCREEN_FUJITSU
>  config TOUCHSCREEN_GOODIX
>  	tristate "Goodix I2C touchscreen"
>  	depends on I2C
> +	depends on GPIOLIB
>  	help
>  	  Say Y here if you have the Goodix touchscreen (such as one
>  	  installed in Onda v975w tablets) connected to your
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index 56d0330..87304ac 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -16,6 +16,7 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/dmi.h>
> +#include <linux/gpio.h>
>  #include <linux/i2c.h>
>  #include <linux/input.h>
>  #include <linux/input/mt.h>
> @@ -37,8 +38,13 @@ struct goodix_ts_data {
>  	unsigned int int_trigger_type;
>  	bool rotated_screen;
>  	int cfg_len;
> +	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
> @@ -237,6 +243,88 @@ 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 error;
> +
> +	error = gpiod_direction_output(ts->gpiod_int, 0);
> +	if (error)
> +		return error;
> +	msleep(50);				/* T5: 50ms */
> +
> +	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 error;
> +
> +	/* begin select I2C slave addr */
> +	error = gpiod_direction_output(ts->gpiod_rst, 0);
> +	if (error)
> +		return error;
> +	msleep(20);				/* T2: > 10ms */
> +	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
> +	error = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14);
> +	if (error)
> +		return error;
> +	usleep_range(100, 2000);		/* T3: > 100us */
> +	error = gpiod_direction_output(ts->gpiod_rst, 1);
> +	if (error)
> +		return error;
> +	usleep_range(6000, 10000);		/* T4: > 5ms */
> +	/* end select I2C slave addr */
> +	error = gpiod_direction_input(ts->gpiod_rst);
> +	if (error)
> +		return error;
> +	return goodix_int_sync(ts);
> +}
> +
> +/**
> + * 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)
> +{
> +	int error;
> +	struct device *dev;
> +	struct gpio_desc *gpiod;
> +
> +	if (!ts->client)
> +		return -EINVAL;
> +	dev = &ts->client->dev;
> +
> +	/* Get the interrupt GPIO pin number */
> +	gpiod = devm_gpiod_get(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN);

Why isn't this devm_gpiod_get_optional()? Then you would not need to
clobber the return value down in goodix_ts_probe().

I can fix it up locally.

Thanks.

-- 
Dmitry

RE: [PATCH v9 2/9] Input: goodix - reset device at init

[Tirdea, Irina]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: 12 October, 2015 19:48
> To: Tirdea, Irina
> Cc: Bastien Nocera; Aleksei Mamlin; Karsten Merker; linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v9 2/9] Input: goodix - reset device at init
> 
> On Mon, Oct 12, 2015 at 06:24:30PM +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 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/device tree). For devices
> > that do not have the gpio pins properly declared, the functionality
> > depending on these pins will not be available, but the device can still
> > be used with basic functionality.
> >
> > For both device tree and ACPI, the interrupt gpio pin configuration is
> > read from the "irq-gpio" property and the reset pin configuration is
> > read from the "reset-gpio" property. For ACPI 5.1, named properties
> > can be specified using the _DSD section. This functionality will not be
> > available for devices that use indexed gpio pins declared in the _CRS
> > section (we need to provide backward compatibility with devices
> > that do not support using the interrupt gpio pin as output).
> >
> > 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>
> > Acked-by: Bastien Nocera <hadess@hadess.net>
> > Tested-by: Bastien Nocera <hadess@hadess.net>
> > Tested-by: Aleksei Mamlin <mamlinav@gmail.com>
> > ---
> >  .../bindings/input/touchscreen/goodix.txt          |   5 +
> >  drivers/input/touchscreen/Kconfig                  |   1 +
> >  drivers/input/touchscreen/goodix.c                 | 101 +++++++++++++++++++++
> >  3 files changed, 107 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/Kconfig b/drivers/input/touchscreen/Kconfig
> > index 771d95c..76f5a9d 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -324,6 +324,7 @@ config TOUCHSCREEN_FUJITSU
> >  config TOUCHSCREEN_GOODIX
> >  	tristate "Goodix I2C touchscreen"
> >  	depends on I2C
> > +	depends on GPIOLIB
> >  	help
> >  	  Say Y here if you have the Goodix touchscreen (such as one
> >  	  installed in Onda v975w tablets) connected to your
> > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> > index 56d0330..87304ac 100644
> > --- a/drivers/input/touchscreen/goodix.c
> > +++ b/drivers/input/touchscreen/goodix.c
> > @@ -16,6 +16,7 @@
> >
> >  #include <linux/kernel.h>
> >  #include <linux/dmi.h>
> > +#include <linux/gpio.h>
> >  #include <linux/i2c.h>
> >  #include <linux/input.h>
> >  #include <linux/input/mt.h>
> > @@ -37,8 +38,13 @@ struct goodix_ts_data {
> >  	unsigned int int_trigger_type;
> >  	bool rotated_screen;
> >  	int cfg_len;
> > +	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
> > @@ -237,6 +243,88 @@ 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 error;
> > +
> > +	error = gpiod_direction_output(ts->gpiod_int, 0);
> > +	if (error)
> > +		return error;
> > +	msleep(50);				/* T5: 50ms */
> > +
> > +	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 error;
> > +
> > +	/* begin select I2C slave addr */
> > +	error = gpiod_direction_output(ts->gpiod_rst, 0);
> > +	if (error)
> > +		return error;
> > +	msleep(20);				/* T2: > 10ms */
> > +	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
> > +	error = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14);
> > +	if (error)
> > +		return error;
> > +	usleep_range(100, 2000);		/* T3: > 100us */
> > +	error = gpiod_direction_output(ts->gpiod_rst, 1);
> > +	if (error)
> > +		return error;
> > +	usleep_range(6000, 10000);		/* T4: > 5ms */
> > +	/* end select I2C slave addr */
> > +	error = gpiod_direction_input(ts->gpiod_rst);
> > +	if (error)
> > +		return error;
> > +	return goodix_int_sync(ts);
> > +}
> > +
> > +/**
> > + * 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)
> > +{
> > +	int error;
> > +	struct device *dev;
> > +	struct gpio_desc *gpiod;
> > +
> > +	if (!ts->client)
> > +		return -EINVAL;
> > +	dev = &ts->client->dev;
> > +
> > +	/* Get the interrupt GPIO pin number */
> > +	gpiod = devm_gpiod_get(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN);
> 
> Why isn't this devm_gpiod_get_optional()? Then you would not need to
> clobber the return value down in goodix_ts_probe().
> 

I did not use devm_gpiod_get_optional() in order to ignore more errors
than -ENOENT. This is needed because the ACPI gpio core will fall back
to indexed gpios if named gpios are not found. In the common case of
having 2 indexed gpio pins declared in the ACPI table, the first
devm_gpiod_get() will successfully get indexed gpio pin 0 and the
second devm_gpiod_get() will try to get the same gpio pin 0 and return
-EBUSY. Considering this, I thought it is better to just ignore all errors in
order not to break any platforms currently using this driver.

> I can fix it up locally.
> 

Sure, you can replace devm_gpiod_get with devm_gpiod_get_optional,
but the error handling code should remain the same.

Thanks,
Irina

> Thanks.
> 
> --
> Dmitry

RE: [PATCH v9 9/9] Input: goodix - sort includes using inverse Xmas tree order

[Tirdea, Irina]

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



> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: 12 October, 2015 19:31
> To: Bastien Nocera
> Cc: Mark Rutland; Tirdea, Irina; Aleksei Mamlin; Karsten Merker; linux-input@vger.kernel.org; Purdila, Octavian; lkml;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH v9 9/9] Input: goodix - sort includes using inverse Xmas tree order
> 
> On Mon, Oct 12, 2015 at 8:53 AM, Bastien Nocera <hadess@hadess.net> wrote:
> > On Mon, 2015-10-12 at 16:51 +0100, Mark Rutland wrote:
> >> On Mon, Oct 12, 2015 at 05:40:37PM +0200, Bastien Nocera wrote:
> >> > On Mon, 2015-10-12 at 16:39 +0100, Mark Rutland wrote:
> >> > > Why?
> >> >
> >> > It was already discussed in the thread for a previous version,
> >> > please
> >> > refer to that.
> >>
> >> Fine, but surely that should be in the commit message, to prevent
> >> others
> >> like myself repeatedly asking the same question?
> >
> > Fair point. Irina?
> 
> No, let's just leave poor includes alone.
> 

OK, will drop this :)

Thanks,
Irina

> --
> Dmitry
ÿôèº{.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 v9 9/9] Input: goodix - sort includes using inverse Xmas tree order

[Tirdea, Irina]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: 12 October, 2015 19:31
> To: Bastien Nocera
> Cc: Mark Rutland; Tirdea, Irina; Aleksei Mamlin; Karsten Merker; linux-input@vger.kernel.org; Purdila, Octavian; lkml;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH v9 9/9] Input: goodix - sort includes using inverse Xmas tree order
> 
> On Mon, Oct 12, 2015 at 8:53 AM, Bastien Nocera <hadess@hadess.net> wrote:
> > On Mon, 2015-10-12 at 16:51 +0100, Mark Rutland wrote:
> >> On Mon, Oct 12, 2015 at 05:40:37PM +0200, Bastien Nocera wrote:
> >> > On Mon, 2015-10-12 at 16:39 +0100, Mark Rutland wrote:
> >> > > Why?
> >> >
> >> > It was already discussed in the thread for a previous version,
> >> > please
> >> > refer to that.
> >>
> >> Fine, but surely that should be in the commit message, to prevent
> >> others
> >> like myself repeatedly asking the same question?
> >
> > Fair point. Irina?
> 
> No, let's just leave poor includes alone.
> 

OK, will drop this :)

Thanks,
Irina

> --
> Dmitry

Re: [PATCH v9 2/9] Input: goodix - reset device at init

[Dmitry Torokhov]

On Tue, Oct 13, 2015 at 06:38:23AM +0000, Tirdea, Irina wrote:
> 
> 
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: 12 October, 2015 19:48
> > To: Tirdea, Irina
> > Cc: Bastien Nocera; Aleksei Mamlin; Karsten Merker; linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> > kernel@vger.kernel.org; devicetree@vger.kernel.org
> > Subject: Re: [PATCH v9 2/9] Input: goodix - reset device at init
> > 
> > On Mon, Oct 12, 2015 at 06:24:30PM +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 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/device tree). For devices
> > > that do not have the gpio pins properly declared, the functionality
> > > depending on these pins will not be available, but the device can still
> > > be used with basic functionality.
> > >
> > > For both device tree and ACPI, the interrupt gpio pin configuration is
> > > read from the "irq-gpio" property and the reset pin configuration is
> > > read from the "reset-gpio" property. For ACPI 5.1, named properties
> > > can be specified using the _DSD section. This functionality will not be
> > > available for devices that use indexed gpio pins declared in the _CRS
> > > section (we need to provide backward compatibility with devices
> > > that do not support using the interrupt gpio pin as output).
> > >
> > > 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>
> > > Acked-by: Bastien Nocera <hadess@hadess.net>
> > > Tested-by: Bastien Nocera <hadess@hadess.net>
> > > Tested-by: Aleksei Mamlin <mamlinav@gmail.com>
> > > ---
> > >  .../bindings/input/touchscreen/goodix.txt          |   5 +
> > >  drivers/input/touchscreen/Kconfig                  |   1 +
> > >  drivers/input/touchscreen/goodix.c                 | 101 +++++++++++++++++++++
> > >  3 files changed, 107 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/Kconfig b/drivers/input/touchscreen/Kconfig
> > > index 771d95c..76f5a9d 100644
> > > --- a/drivers/input/touchscreen/Kconfig
> > > +++ b/drivers/input/touchscreen/Kconfig
> > > @@ -324,6 +324,7 @@ config TOUCHSCREEN_FUJITSU
> > >  config TOUCHSCREEN_GOODIX
> > >  	tristate "Goodix I2C touchscreen"
> > >  	depends on I2C
> > > +	depends on GPIOLIB
> > >  	help
> > >  	  Say Y here if you have the Goodix touchscreen (such as one
> > >  	  installed in Onda v975w tablets) connected to your
> > > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> > > index 56d0330..87304ac 100644
> > > --- a/drivers/input/touchscreen/goodix.c
> > > +++ b/drivers/input/touchscreen/goodix.c
> > > @@ -16,6 +16,7 @@
> > >
> > >  #include <linux/kernel.h>
> > >  #include <linux/dmi.h>
> > > +#include <linux/gpio.h>
> > >  #include <linux/i2c.h>
> > >  #include <linux/input.h>
> > >  #include <linux/input/mt.h>
> > > @@ -37,8 +38,13 @@ struct goodix_ts_data {
> > >  	unsigned int int_trigger_type;
> > >  	bool rotated_screen;
> > >  	int cfg_len;
> > > +	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
> > > @@ -237,6 +243,88 @@ 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 error;
> > > +
> > > +	error = gpiod_direction_output(ts->gpiod_int, 0);
> > > +	if (error)
> > > +		return error;
> > > +	msleep(50);				/* T5: 50ms */
> > > +
> > > +	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 error;
> > > +
> > > +	/* begin select I2C slave addr */
> > > +	error = gpiod_direction_output(ts->gpiod_rst, 0);
> > > +	if (error)
> > > +		return error;
> > > +	msleep(20);				/* T2: > 10ms */
> > > +	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
> > > +	error = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14);
> > > +	if (error)
> > > +		return error;
> > > +	usleep_range(100, 2000);		/* T3: > 100us */
> > > +	error = gpiod_direction_output(ts->gpiod_rst, 1);
> > > +	if (error)
> > > +		return error;
> > > +	usleep_range(6000, 10000);		/* T4: > 5ms */
> > > +	/* end select I2C slave addr */
> > > +	error = gpiod_direction_input(ts->gpiod_rst);
> > > +	if (error)
> > > +		return error;
> > > +	return goodix_int_sync(ts);
> > > +}
> > > +
> > > +/**
> > > + * 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)
> > > +{
> > > +	int error;
> > > +	struct device *dev;
> > > +	struct gpio_desc *gpiod;
> > > +
> > > +	if (!ts->client)
> > > +		return -EINVAL;
> > > +	dev = &ts->client->dev;
> > > +
> > > +	/* Get the interrupt GPIO pin number */
> > > +	gpiod = devm_gpiod_get(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN);
> > 
> > Why isn't this devm_gpiod_get_optional()? Then you would not need to
> > clobber the return value down in goodix_ts_probe().
> > 
> 
> I did not use devm_gpiod_get_optional() in order to ignore more errors
> than -ENOENT. This is needed because the ACPI gpio core will fall back
> to indexed gpios if named gpios are not found. In the common case of
> having 2 indexed gpio pins declared in the ACPI table, the first
> devm_gpiod_get() will successfully get indexed gpio pin 0 and the
> second devm_gpiod_get() will try to get the same gpio pin 0 and return
> -EBUSY. Considering this, I thought it is better to just ignore all errors in
> order not to break any platforms currently using this driver.

This seems like issue with ACPI gpio lookup implementation. If I am
requesting named gpio and it is not present then I definitely do not
need to be returned some random gpio. Doing so breaks all other drivers
that use several names to retrieve GPIOs. We basically can't trust GPIO
API on ACPI systems.

I can see why we wanted to provide unnamed gpios even in presence of
con_id, but it does not work when using several names. I wonder if
acpi_find_gpio will have to keep track of state (requested names) and
stop falling back to unnamed gpios if more than one con_id was suppolied
for the same object.

Thanks.

-- 
Dmitry

Re: [PATCH v9 2/9] Input: goodix - reset device at init

[Dmitry Torokhov]

On Tue, Oct 13, 2015 at 06:38:23AM +0000, Tirdea, Irina wrote:
> 
> 
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
> > Sent: 12 October, 2015 19:48
> > To: Tirdea, Irina
> > Cc: Bastien Nocera; Aleksei Mamlin; Karsten Merker; linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Mark Rutland; Purdila, Octavian; linux-
> > kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [PATCH v9 2/9] Input: goodix - reset device at init
> > 
> > On Mon, Oct 12, 2015 at 06:24:30PM +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 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/device tree). For devices
> > > that do not have the gpio pins properly declared, the functionality
> > > depending on these pins will not be available, but the device can still
> > > be used with basic functionality.
> > >
> > > For both device tree and ACPI, the interrupt gpio pin configuration is
> > > read from the "irq-gpio" property and the reset pin configuration is
> > > read from the "reset-gpio" property. For ACPI 5.1, named properties
> > > can be specified using the _DSD section. This functionality will not be
> > > available for devices that use indexed gpio pins declared in the _CRS
> > > section (we need to provide backward compatibility with devices
> > > that do not support using the interrupt gpio pin as output).
> > >
> > > 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > Signed-off-by: Irina Tirdea <irina.tirdea-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > Acked-by: Bastien Nocera <hadess-0MeiytkfxGOsTnJN9+BGXg@public.gmane.org>
> > > Tested-by: Bastien Nocera <hadess-0MeiytkfxGOsTnJN9+BGXg@public.gmane.org>
> > > Tested-by: Aleksei Mamlin <mamlinav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > ---
> > >  .../bindings/input/touchscreen/goodix.txt          |   5 +
> > >  drivers/input/touchscreen/Kconfig                  |   1 +
> > >  drivers/input/touchscreen/goodix.c                 | 101 +++++++++++++++++++++
> > >  3 files changed, 107 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/Kconfig b/drivers/input/touchscreen/Kconfig
> > > index 771d95c..76f5a9d 100644
> > > --- a/drivers/input/touchscreen/Kconfig
> > > +++ b/drivers/input/touchscreen/Kconfig
> > > @@ -324,6 +324,7 @@ config TOUCHSCREEN_FUJITSU
> > >  config TOUCHSCREEN_GOODIX
> > >  	tristate "Goodix I2C touchscreen"
> > >  	depends on I2C
> > > +	depends on GPIOLIB
> > >  	help
> > >  	  Say Y here if you have the Goodix touchscreen (such as one
> > >  	  installed in Onda v975w tablets) connected to your
> > > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> > > index 56d0330..87304ac 100644
> > > --- a/drivers/input/touchscreen/goodix.c
> > > +++ b/drivers/input/touchscreen/goodix.c
> > > @@ -16,6 +16,7 @@
> > >
> > >  #include <linux/kernel.h>
> > >  #include <linux/dmi.h>
> > > +#include <linux/gpio.h>
> > >  #include <linux/i2c.h>
> > >  #include <linux/input.h>
> > >  #include <linux/input/mt.h>
> > > @@ -37,8 +38,13 @@ struct goodix_ts_data {
> > >  	unsigned int int_trigger_type;
> > >  	bool rotated_screen;
> > >  	int cfg_len;
> > > +	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
> > > @@ -237,6 +243,88 @@ 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 error;
> > > +
> > > +	error = gpiod_direction_output(ts->gpiod_int, 0);
> > > +	if (error)
> > > +		return error;
> > > +	msleep(50);				/* T5: 50ms */
> > > +
> > > +	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 error;
> > > +
> > > +	/* begin select I2C slave addr */
> > > +	error = gpiod_direction_output(ts->gpiod_rst, 0);
> > > +	if (error)
> > > +		return error;
> > > +	msleep(20);				/* T2: > 10ms */
> > > +	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
> > > +	error = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14);
> > > +	if (error)
> > > +		return error;
> > > +	usleep_range(100, 2000);		/* T3: > 100us */
> > > +	error = gpiod_direction_output(ts->gpiod_rst, 1);
> > > +	if (error)
> > > +		return error;
> > > +	usleep_range(6000, 10000);		/* T4: > 5ms */
> > > +	/* end select I2C slave addr */
> > > +	error = gpiod_direction_input(ts->gpiod_rst);
> > > +	if (error)
> > > +		return error;
> > > +	return goodix_int_sync(ts);
> > > +}
> > > +
> > > +/**
> > > + * 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)
> > > +{
> > > +	int error;
> > > +	struct device *dev;
> > > +	struct gpio_desc *gpiod;
> > > +
> > > +	if (!ts->client)
> > > +		return -EINVAL;
> > > +	dev = &ts->client->dev;
> > > +
> > > +	/* Get the interrupt GPIO pin number */
> > > +	gpiod = devm_gpiod_get(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN);
> > 
> > Why isn't this devm_gpiod_get_optional()? Then you would not need to
> > clobber the return value down in goodix_ts_probe().
> > 
> 
> I did not use devm_gpiod_get_optional() in order to ignore more errors
> than -ENOENT. This is needed because the ACPI gpio core will fall back
> to indexed gpios if named gpios are not found. In the common case of
> having 2 indexed gpio pins declared in the ACPI table, the first
> devm_gpiod_get() will successfully get indexed gpio pin 0 and the
> second devm_gpiod_get() will try to get the same gpio pin 0 and return
> -EBUSY. Considering this, I thought it is better to just ignore all errors in
> order not to break any platforms currently using this driver.

This seems like issue with ACPI gpio lookup implementation. If I am
requesting named gpio and it is not present then I definitely do not
need to be returned some random gpio. Doing so breaks all other drivers
that use several names to retrieve GPIOs. We basically can't trust GPIO
API on ACPI systems.

I can see why we wanted to provide unnamed gpios even in presence of
con_id, but it does not work when using several names. I wonder if
acpi_find_gpio will have to keep track of state (requested names) and
stop falling back to unnamed gpios if more than one con_id was suppolied
for the same object.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v9 2/9] Input: goodix - reset device at init

[Tirdea, Irina]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: 13 October, 2015 10:08
> To: Tirdea, Irina
> Cc: Bastien Nocera; Aleksei Mamlin; Karsten Merker; linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v9 2/9] Input: goodix - reset device at init
> 
> On Tue, Oct 13, 2015 at 06:38:23AM +0000, Tirdea, Irina wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > > Sent: 12 October, 2015 19:48
> > > To: Tirdea, Irina
> > > Cc: Bastien Nocera; Aleksei Mamlin; Karsten Merker; linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> > > kernel@vger.kernel.org; devicetree@vger.kernel.org
> > > Subject: Re: [PATCH v9 2/9] Input: goodix - reset device at init
> > >
> > > On Mon, Oct 12, 2015 at 06:24:30PM +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 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/device tree). For devices
> > > > that do not have the gpio pins properly declared, the functionality
> > > > depending on these pins will not be available, but the device can still
> > > > be used with basic functionality.
> > > >
> > > > For both device tree and ACPI, the interrupt gpio pin configuration is
> > > > read from the "irq-gpio" property and the reset pin configuration is
> > > > read from the "reset-gpio" property. For ACPI 5.1, named properties
> > > > can be specified using the _DSD section. This functionality will not be
> > > > available for devices that use indexed gpio pins declared in the _CRS
> > > > section (we need to provide backward compatibility with devices
> > > > that do not support using the interrupt gpio pin as output).
> > > >
> > > > 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>
> > > > Acked-by: Bastien Nocera <hadess@hadess.net>
> > > > Tested-by: Bastien Nocera <hadess@hadess.net>
> > > > Tested-by: Aleksei Mamlin <mamlinav@gmail.com>
> > > > ---
> > > >  .../bindings/input/touchscreen/goodix.txt          |   5 +
> > > >  drivers/input/touchscreen/Kconfig                  |   1 +
> > > >  drivers/input/touchscreen/goodix.c                 | 101 +++++++++++++++++++++
> > > >  3 files changed, 107 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/Kconfig b/drivers/input/touchscreen/Kconfig
> > > > index 771d95c..76f5a9d 100644
> > > > --- a/drivers/input/touchscreen/Kconfig
> > > > +++ b/drivers/input/touchscreen/Kconfig
> > > > @@ -324,6 +324,7 @@ config TOUCHSCREEN_FUJITSU
> > > >  config TOUCHSCREEN_GOODIX
> > > >  	tristate "Goodix I2C touchscreen"
> > > >  	depends on I2C
> > > > +	depends on GPIOLIB
> > > >  	help
> > > >  	  Say Y here if you have the Goodix touchscreen (such as one
> > > >  	  installed in Onda v975w tablets) connected to your
> > > > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> > > > index 56d0330..87304ac 100644
> > > > --- a/drivers/input/touchscreen/goodix.c
> > > > +++ b/drivers/input/touchscreen/goodix.c
> > > > @@ -16,6 +16,7 @@
> > > >
> > > >  #include <linux/kernel.h>
> > > >  #include <linux/dmi.h>
> > > > +#include <linux/gpio.h>
> > > >  #include <linux/i2c.h>
> > > >  #include <linux/input.h>
> > > >  #include <linux/input/mt.h>
> > > > @@ -37,8 +38,13 @@ struct goodix_ts_data {
> > > >  	unsigned int int_trigger_type;
> > > >  	bool rotated_screen;
> > > >  	int cfg_len;
> > > > +	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
> > > > @@ -237,6 +243,88 @@ 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 error;
> > > > +
> > > > +	error = gpiod_direction_output(ts->gpiod_int, 0);
> > > > +	if (error)
> > > > +		return error;
> > > > +	msleep(50);				/* T5: 50ms */
> > > > +
> > > > +	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 error;
> > > > +
> > > > +	/* begin select I2C slave addr */
> > > > +	error = gpiod_direction_output(ts->gpiod_rst, 0);
> > > > +	if (error)
> > > > +		return error;
> > > > +	msleep(20);				/* T2: > 10ms */
> > > > +	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
> > > > +	error = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14);
> > > > +	if (error)
> > > > +		return error;
> > > > +	usleep_range(100, 2000);		/* T3: > 100us */
> > > > +	error = gpiod_direction_output(ts->gpiod_rst, 1);
> > > > +	if (error)
> > > > +		return error;
> > > > +	usleep_range(6000, 10000);		/* T4: > 5ms */
> > > > +	/* end select I2C slave addr */
> > > > +	error = gpiod_direction_input(ts->gpiod_rst);
> > > > +	if (error)
> > > > +		return error;
> > > > +	return goodix_int_sync(ts);
> > > > +}
> > > > +
> > > > +/**
> > > > + * 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)
> > > > +{
> > > > +	int error;
> > > > +	struct device *dev;
> > > > +	struct gpio_desc *gpiod;
> > > > +
> > > > +	if (!ts->client)
> > > > +		return -EINVAL;
> > > > +	dev = &ts->client->dev;
> > > > +
> > > > +	/* Get the interrupt GPIO pin number */
> > > > +	gpiod = devm_gpiod_get(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN);
> > >
> > > Why isn't this devm_gpiod_get_optional()? Then you would not need to
> > > clobber the return value down in goodix_ts_probe().
> > >
> >
> > I did not use devm_gpiod_get_optional() in order to ignore more errors
> > than -ENOENT. This is needed because the ACPI gpio core will fall back
> > to indexed gpios if named gpios are not found. In the common case of
> > having 2 indexed gpio pins declared in the ACPI table, the first
> > devm_gpiod_get() will successfully get indexed gpio pin 0 and the
> > second devm_gpiod_get() will try to get the same gpio pin 0 and return
> > -EBUSY. Considering this, I thought it is better to just ignore all errors in
> > order not to break any platforms currently using this driver.
> 
> This seems like issue with ACPI gpio lookup implementation. If I am
> requesting named gpio and it is not present then I definitely do not
> need to be returned some random gpio. Doing so breaks all other drivers
> that use several names to retrieve GPIOs. We basically can't trust GPIO
> API on ACPI systems.
> 

I'm not sure there is a way to avoid fall back to indexed gpios when requesting
named gpios.
Adding Mika to this thread as he might help answer this.

> I can see why we wanted to provide unnamed gpios even in presence of
> con_id, but it does not work when using several names. I wonder if
> acpi_find_gpio will have to keep track of state (requested names) and
> stop falling back to unnamed gpios if more than one con_id was suppolied
> for the same object.
> 

For devices that need to support more than one named gpio, the driver has
to declare an acpi_gpio_mapping structure (that maps names to indexed gpios)
and register it with acpi_dev_add_driver_gpios [1].

I have actually implemented this in v7 of this patchset [2], before finding
out that the platforms Bastien was testing with do not support writing to the
interrupt pin. In order to avoid adding exceptions to the driver through dmi
quirks, the gpio dependent functionality is available only to devices that
declare named gpios.

Thanks,
Irina

[1] http://lxr.free-electrons.com/source/Documentation/acpi/gpio-properties.txt
[2] https://lkml.org/lkml/2015/10/8/222

> Thanks.
> 
> --
> Dmitry

Re: [PATCH v9 2/9] Input: goodix - reset device at init

[mika.westerberg]

On Tue, Oct 13, 2015 at 08:54:12AM +0000, Tirdea, Irina wrote:
> > > I did not use devm_gpiod_get_optional() in order to ignore more errors
> > > than -ENOENT. This is needed because the ACPI gpio core will fall back
> > > to indexed gpios if named gpios are not found. In the common case of
> > > having 2 indexed gpio pins declared in the ACPI table, the first
> > > devm_gpiod_get() will successfully get indexed gpio pin 0 and the
> > > second devm_gpiod_get() will try to get the same gpio pin 0 and return
> > > -EBUSY. Considering this, I thought it is better to just ignore all errors in
> > > order not to break any platforms currently using this driver.
> > 
> > This seems like issue with ACPI gpio lookup implementation. If I am
> > requesting named gpio and it is not present then I definitely do not
> > need to be returned some random gpio. Doing so breaks all other drivers
> > that use several names to retrieve GPIOs. We basically can't trust GPIO
> > API on ACPI systems.
> > 
> 
> I'm not sure there is a way to avoid fall back to indexed gpios when requesting
> named gpios.
> Adding Mika to this thread as he might help answer this.

Before ACPI 5.1 _DSD device properties were introduced all we had was an
array of GPIOs returned by _CRS ACPI method. Ordering of those GPIOs
could change from one vendor to another :-(

We can (and do) use acpi_dev_add_driver_gpios() to pass correct mappings
where _DSD is not present based on the device ACPI ID for instance. Not
all drivers do that, though.

I would like to get rid of the fallback completely at some point. We
have had already problems with the API because then some ACPI only
drivers did this:

	reset_gpio = gpiod_get_index(dev, NULL, 0);
	power_gpio = gpiod_get_index(dev, NULL, 1);

which might not do what is expected on DT systems. That's why
acpi_dev_add_driver_gpios() was added in the first place IIRC.

Re: [PATCH v9 2/9] Input: goodix - reset device at init

[Dmitry Torokhov]

On Tue, Oct 13, 2015 at 01:07:24PM +0300, mika.westerberg@linux.intel.com wrote:
> On Tue, Oct 13, 2015 at 08:54:12AM +0000, Tirdea, Irina wrote:
> > > > I did not use devm_gpiod_get_optional() in order to ignore more errors
> > > > than -ENOENT. This is needed because the ACPI gpio core will fall back
> > > > to indexed gpios if named gpios are not found. In the common case of
> > > > having 2 indexed gpio pins declared in the ACPI table, the first
> > > > devm_gpiod_get() will successfully get indexed gpio pin 0 and the
> > > > second devm_gpiod_get() will try to get the same gpio pin 0 and return
> > > > -EBUSY. Considering this, I thought it is better to just ignore all errors in
> > > > order not to break any platforms currently using this driver.
> > > 
> > > This seems like issue with ACPI gpio lookup implementation. If I am
> > > requesting named gpio and it is not present then I definitely do not
> > > need to be returned some random gpio. Doing so breaks all other drivers
> > > that use several names to retrieve GPIOs. We basically can't trust GPIO
> > > API on ACPI systems.
> > > 
> > 
> > I'm not sure there is a way to avoid fall back to indexed gpios when requesting
> > named gpios.
> > Adding Mika to this thread as he might help answer this.
> 
> Before ACPI 5.1 _DSD device properties were introduced all we had was an
> array of GPIOs returned by _CRS ACPI method. Ordering of those GPIOs
> could change from one vendor to another :-(
> 
> We can (and do) use acpi_dev_add_driver_gpios() to pass correct mappings
> where _DSD is not present based on the device ACPI ID for instance. Not
> all drivers do that, though.
> 
> I would like to get rid of the fallback completely at some point. We
> have had already problems with the API because then some ACPI only
> drivers did this:
> 
> 	reset_gpio = gpiod_get_index(dev, NULL, 0);
> 	power_gpio = gpiod_get_index(dev, NULL, 1);
> 
> which might not do what is expected on DT systems. That's why
> acpi_dev_add_driver_gpios() was added in the first place IIRC.

I understand why one might use acpi_dev_add_driver_gpios() to augment
data in ACPI, however here we have completely different issue: driver
that expects named gpios gets returned gpio that has nothing to do with
what it requested, because gpiolib acpi code always falls back to
unnamed gpio if it does not find named gpio. That can be acceptable if
driver uses the same con_id for all requests to gpiolib, but is not
working when driver supplies different con_ids.

Thanks.

-- 
Dmitry

Re: [PATCH v9 2/9] Input: goodix - reset device at init

[mika.westerberg]

On Tue, Oct 13, 2015 at 11:23:03PM -0700, Dmitry Torokhov wrote:
> I understand why one might use acpi_dev_add_driver_gpios() to augment
> data in ACPI, however here we have completely different issue: driver
> that expects named gpios gets returned gpio that has nothing to do with
> what it requested, because gpiolib acpi code always falls back to
> unnamed gpio if it does not find named gpio. That can be acceptable if
> driver uses the same con_id for all requests to gpiolib, but is not
> working when driver supplies different con_ids.

Right, the ACPI fallback ignores con_id completely and uses only the
index.

AFAIK there is only one driver using ACPI _CRS index method:
sdhci-[acpi|pci].c. If we can convert that to use acpi_dev_add_driver_gpios()
to feed names for card detection GPIOs, I think we can remove the
fallback alltogether in favor of named GPIOs for ACPI.

Re: [PATCH v9 2/9] Input: goodix - reset device at init

[mika.westerberg]

On Wed, Oct 14, 2015 at 02:18:20PM +0300, mika.westerberg@linux.intel.com wrote:
> On Tue, Oct 13, 2015 at 11:23:03PM -0700, Dmitry Torokhov wrote:
> > I understand why one might use acpi_dev_add_driver_gpios() to augment
> > data in ACPI, however here we have completely different issue: driver
> > that expects named gpios gets returned gpio that has nothing to do with
> > what it requested, because gpiolib acpi code always falls back to
> > unnamed gpio if it does not find named gpio. That can be acceptable if
> > driver uses the same con_id for all requests to gpiolib, but is not
> > working when driver supplies different con_ids.
> 
> Right, the ACPI fallback ignores con_id completely and uses only the
> index.
> 
> AFAIK there is only one driver using ACPI _CRS index method:
> sdhci-[acpi|pci].c. If we can convert that to use acpi_dev_add_driver_gpios()
> to feed names for card detection GPIOs, I think we can remove the
> fallback alltogether in favor of named GPIOs for ACPI.

Nah, there seems to be several drivers relying on this already :-/

Re: [PATCH v9 2/9] Input: goodix - reset device at init

[mika.westerberg-VuQAYsv1563Yd54FQh9/CA]

On Wed, Oct 14, 2015 at 02:18:20PM +0300, mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org wrote:
> On Tue, Oct 13, 2015 at 11:23:03PM -0700, Dmitry Torokhov wrote:
> > I understand why one might use acpi_dev_add_driver_gpios() to augment
> > data in ACPI, however here we have completely different issue: driver
> > that expects named gpios gets returned gpio that has nothing to do with
> > what it requested, because gpiolib acpi code always falls back to
> > unnamed gpio if it does not find named gpio. That can be acceptable if
> > driver uses the same con_id for all requests to gpiolib, but is not
> > working when driver supplies different con_ids.
> 
> Right, the ACPI fallback ignores con_id completely and uses only the
> index.
> 
> AFAIK there is only one driver using ACPI _CRS index method:
> sdhci-[acpi|pci].c. If we can convert that to use acpi_dev_add_driver_gpios()
> to feed names for card detection GPIOs, I think we can remove the
> fallback alltogether in favor of named GPIOs for ACPI.

Nah, there seems to be several drivers relying on this already :-/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v9 2/9] 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
> mika.westerberg@linux.intel.com
> Sent: 14 October, 2015 16:44
> To: Dmitry Torokhov
> Cc: Tirdea, Irina; Bastien Nocera; Aleksei Mamlin; Karsten Merker; linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v9 2/9] Input: goodix - reset device at init
> 
> On Wed, Oct 14, 2015 at 02:18:20PM +0300, mika.westerberg@linux.intel.com wrote:
> > On Tue, Oct 13, 2015 at 11:23:03PM -0700, Dmitry Torokhov wrote:
> > > I understand why one might use acpi_dev_add_driver_gpios() to augment
> > > data in ACPI, however here we have completely different issue: driver
> > > that expects named gpios gets returned gpio that has nothing to do with
> > > what it requested, because gpiolib acpi code always falls back to
> > > unnamed gpio if it does not find named gpio. That can be acceptable if
> > > driver uses the same con_id for all requests to gpiolib, but is not
> > > working when driver supplies different con_ids.
> >
> > Right, the ACPI fallback ignores con_id completely and uses only the
> > index.
> >
> > AFAIK there is only one driver using ACPI _CRS index method:
> > sdhci-[acpi|pci].c. If we can convert that to use acpi_dev_add_driver_gpios()
> > to feed names for card detection GPIOs, I think we can remove the
> > fallback alltogether in favor of named GPIOs for ACPI.
> 
> Nah, there seems to be several drivers relying on this already :-/

Would it be possible to add an optional parameter to the GPIO API
to specify whether we want to fall back to indexed GPIOs for ACPI?

> --
> 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 v9 2/9] Input: goodix - reset device at init

[mika.westerberg]

On Mon, Oct 19, 2015 at 02:32:24PM +0000, Tirdea, Irina wrote:
> 
> 
> > -----Original Message-----
> > From: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@vger.kernel.org] On Behalf Of
> > mika.westerberg@linux.intel.com
> > Sent: 14 October, 2015 16:44
> > To: Dmitry Torokhov
> > Cc: Tirdea, Irina; Bastien Nocera; Aleksei Mamlin; Karsten Merker; linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> > kernel@vger.kernel.org; devicetree@vger.kernel.org
> > Subject: Re: [PATCH v9 2/9] Input: goodix - reset device at init
> > 
> > On Wed, Oct 14, 2015 at 02:18:20PM +0300, mika.westerberg@linux.intel.com wrote:
> > > On Tue, Oct 13, 2015 at 11:23:03PM -0700, Dmitry Torokhov wrote:
> > > > I understand why one might use acpi_dev_add_driver_gpios() to augment
> > > > data in ACPI, however here we have completely different issue: driver
> > > > that expects named gpios gets returned gpio that has nothing to do with
> > > > what it requested, because gpiolib acpi code always falls back to
> > > > unnamed gpio if it does not find named gpio. That can be acceptable if
> > > > driver uses the same con_id for all requests to gpiolib, but is not
> > > > working when driver supplies different con_ids.
> > >
> > > Right, the ACPI fallback ignores con_id completely and uses only the
> > > index.
> > >
> > > AFAIK there is only one driver using ACPI _CRS index method:
> > > sdhci-[acpi|pci].c. If we can convert that to use acpi_dev_add_driver_gpios()
> > > to feed names for card detection GPIOs, I think we can remove the
> > > fallback alltogether in favor of named GPIOs for ACPI.
> > 
> > Nah, there seems to be several drivers relying on this already :-/
> 
> Would it be possible to add an optional parameter to the GPIO API
> to specify whether we want to fall back to indexed GPIOs for ACPI?

I don't think it's a good idea to add ACPI specifics to generic APIs.

I went through ACPI enabled drivers calling GPIO APIs and majority of
them are doing this:

static int stk8312_gpio_probe(struct i2c_client *client)
{
        struct device *dev;
        struct gpio_desc *gpio;
        int ret;

        if (!client)
                return -EINVAL;

        dev = &client->dev;

        /* data ready gpio interrupt pin */
        gpio = devm_gpiod_get_index(dev, STK8312_GPIO, 0, GPIOD_IN);
        if (IS_ERR(gpio)) {
                dev_err(dev, "acpi gpio get index failed\n");
                return PTR_ERR(gpio);
        }

        ret = gpiod_to_irq(gpio);
        dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);

        return ret;
}

We can drop all this because I2C core already handles GpioInt -> interrupt
number translation.

Few drivers are doing something more complex but I think we can still convert
them to use acpi_dev_add_driver_gpios() and eventually get rid of the whole
_CRS index lookup.

Re: [PATCH v9 0/9] Goodix touchscreen enhancements

[Bastien Nocera]

Hey,

On Mon, 2015-10-12 at 18:24 +0300, Irina Tirdea wrote:
> v9 only adds GPIOLIB dependency in Kconfig for patch 2:
> "Input: goodix - reset device at init". There are no other code
> changes from v8.
> 
> Thanks for testing these changes, Bastien and Aleksei!
> 
> Karsten, there is no need to rebase your series on top of v9.

Are we waiting on anything else before merging this? I'd like it to be
scheduled to be merged so I can start focusing on the subsequent and
dependent patches for that same driver.

Cheers

Re: [PATCH v9 0/9] Goodix touchscreen enhancements

[Bastien Nocera]

Hey,

On Mon, 2015-10-12 at 18:24 +0300, Irina Tirdea wrote:
> v9 only adds GPIOLIB dependency in Kconfig for patch 2:
> "Input: goodix - reset device at init". There are no other code
> changes from v8.
> 
> Thanks for testing these changes, Bastien and Aleksei!
> 
> Karsten, there is no need to rebase your series on top of v9.

Are we waiting on anything else before merging this? I'd like it to be
scheduled to be merged so I can start focusing on the subsequent and
dependent patches for that same driver.

Cheers
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 0/9] Goodix touchscreen enhancements

[Karsten Merker]

On Mon, Oct 26, 2015 at 04:06:29PM +0100, Bastien Nocera wrote:
> On Mon, 2015-10-12 at 18:24 +0300, Irina Tirdea wrote:

> > v9 only adds GPIOLIB dependency in Kconfig for patch 2:
> > "Input: goodix - reset device at init". There are no other code
> > changes from v8.
> > 
> > Thanks for testing these changes, Bastien and Aleksei!
> > 
> > Karsten, there is no need to rebase your series on top of v9.
> 
> Are we waiting on anything else before merging this? I'd like it to be
> scheduled to be merged so I can start focusing on the subsequent and
> dependent patches for that same driver.

Hello,

AFAICS there is one open point (cf. 
http://www.spinics.net/lists/linux-input/msg41567.html) which
Irina wanted to address in a v10 of the patchset (cf. 
http://www.spinics.net/lists/linux-input/msg41642.html).

Irina, how are your plans regarding the v10? It would be really
nice if the patches could go into kernel 4.4, but the merge
window opens on the coming weekend, so there is not much time
left.

Bastien, did you have time to look at v3 of the axis
swapping/inversion set?
(http://www.spinics.net/lists/linux-input/msg41628.html)
You had acked v2, but I had to do some small changes to address
Irina's review comments after you had acked it, so I didn't want
to carry your "acked-by" on to v3 without an ok from you.

Regards,
Karsten
-- 
Gem. Par. 28 Abs. 4 Bundesdatenschutzgesetz widerspreche ich der Nutzung
sowie der Weitergabe meiner personenbezogenen Daten für Zwecke der
Werbung sowie der Markt- oder Meinungsforschung.

Re: [PATCH v9 0/9] Goodix touchscreen enhancements

[Bastien Nocera]

On Mon, 2015-10-26 at 19:21 +0100, Karsten Merker wrote:
> On Mon, Oct 26, 2015 at 04:06:29PM +0100, Bastien Nocera wrote:
> > On Mon, 2015-10-12 at 18:24 +0300, Irina Tirdea wrote:
> 
> > > v9 only adds GPIOLIB dependency in Kconfig for patch 2:
> > > "Input: goodix - reset device at init". There are no other code
> > > changes from v8.
> > > 
> > > Thanks for testing these changes, Bastien and Aleksei!
> > > 
> > > Karsten, there is no need to rebase your series on top of v9.
> > 
> > Are we waiting on anything else before merging this? I'd like it to
> > be
> > scheduled to be merged so I can start focusing on the subsequent
> > and
> > dependent patches for that same driver.
> 
> Hello,
> 
> AFAICS there is one open point (cf. 
> http://www.spinics.net/lists/linux-input/msg41567.html) which
> Irina wanted to address in a v10 of the patchset (cf. 
> http://www.spinics.net/lists/linux-input/msg41642.html).
> 
> Irina, how are your plans regarding the v10? It would be really
> nice if the patches could go into kernel 4.4, but the merge
> window opens on the coming weekend, so there is not much time
> left.
> 
> Bastien, did you have time to look at v3 of the axis
> swapping/inversion set?
> (http://www.spinics.net/lists/linux-input/msg41628.html)
> You had acked v2, but I had to do some small changes to address
> Irina's review comments after you had acked it, so I didn't want
> to carry your "acked-by" on to v3 without an ok from you.

I was waiting on at least Irina's patches being merged before testing
your patches again. I have limited time to do testing on this (I have
plenty more hardware that's sitting unloved here), and wanted to
minimise the amount of time I'd spend testing it.

Re: [PATCH v9 0/9] Goodix touchscreen enhancements

[Dmitry Torokhov]

On Mon, Oct 26, 2015 at 07:21:12PM +0100, Karsten Merker wrote:
> On Mon, Oct 26, 2015 at 04:06:29PM +0100, Bastien Nocera wrote:
> > On Mon, 2015-10-12 at 18:24 +0300, Irina Tirdea wrote:
> 
> > > v9 only adds GPIOLIB dependency in Kconfig for patch 2:
> > > "Input: goodix - reset device at init". There are no other code
> > > changes from v8.
> > > 
> > > Thanks for testing these changes, Bastien and Aleksei!
> > > 
> > > Karsten, there is no need to rebase your series on top of v9.
> > 
> > Are we waiting on anything else before merging this? I'd like it to be
> > scheduled to be merged so I can start focusing on the subsequent and
> > dependent patches for that same driver.
> 
> Hello,
> 
> AFAICS there is one open point (cf. 
> http://www.spinics.net/lists/linux-input/msg41567.html) which
> Irina wanted to address in a v10 of the patchset (cf. 
> http://www.spinics.net/lists/linux-input/msg41642.html).

There is also the whole thing about insane handling of named gpios in
ACPI layer, which stops me from merging the reset code since these gpios
should be marked as optional and we should stop ignoring errors coming
from gpiolib.

Thanks.

-- 
Dmitry

RE: [PATCH v9 0/9] Goodix touchscreen enhancements

[Tirdea, Irina]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: 27 October, 2015 1:33
> To: Karsten Merker
> Cc: Bastien Nocera; Tirdea, Irina; Aleksei Mamlin; linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v9 0/9] Goodix touchscreen enhancements
> 
> On Mon, Oct 26, 2015 at 07:21:12PM +0100, Karsten Merker wrote:
> > On Mon, Oct 26, 2015 at 04:06:29PM +0100, Bastien Nocera wrote:
> > > On Mon, 2015-10-12 at 18:24 +0300, Irina Tirdea wrote:
> >
> > > > v9 only adds GPIOLIB dependency in Kconfig for patch 2:
> > > > "Input: goodix - reset device at init". There are no other code
> > > > changes from v8.
> > > >
> > > > Thanks for testing these changes, Bastien and Aleksei!
> > > >
> > > > Karsten, there is no need to rebase your series on top of v9.
> > >
> > > Are we waiting on anything else before merging this? I'd like it to be
> > > scheduled to be merged so I can start focusing on the subsequent and
> > > dependent patches for that same driver.
> >
> > Hello,
> >
> > AFAICS there is one open point (cf.
> > http://www.spinics.net/lists/linux-input/msg41567.html) which
> > Irina wanted to address in a v10 of the patchset (cf.
> > http://www.spinics.net/lists/linux-input/msg41642.html).
> 
> There is also the whole thing about insane handling of named gpios in
> ACPI layer, which stops me from merging the reset code since these gpios
> should be marked as optional and we should stop ignoring errors coming
> from gpiolib.

The ACPI layer change is quite complex, since it includes changing the drivers
that use the gpio API before removing the fallback to indexed ACPI.
Not sure that will not also break current drivers that already count on this
fallback. Unfortunately, I do not have the time right now to get involved in
fixing the ACPI core myself.

Dmitry, is there anything I can do in the driver to get these patches merged?
I could go back to using indexed gpios and add an additional property
to specify if irq can be used as output or not (as suggested in one of the
previous reviews).

Thanks,
Irina

> 
> Thanks.
> 
> --
> Dmitry

RE: [PATCH v9 0/9] Goodix touchscreen enhancements

[Tirdea, Irina]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
> Sent: 27 October, 2015 1:33
> To: Karsten Merker
> Cc: Bastien Nocera; Tirdea, Irina; Aleksei Mamlin; linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Mark Rutland; Purdila, Octavian; linux-
> kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH v9 0/9] Goodix touchscreen enhancements
> 
> On Mon, Oct 26, 2015 at 07:21:12PM +0100, Karsten Merker wrote:
> > On Mon, Oct 26, 2015 at 04:06:29PM +0100, Bastien Nocera wrote:
> > > On Mon, 2015-10-12 at 18:24 +0300, Irina Tirdea wrote:
> >
> > > > v9 only adds GPIOLIB dependency in Kconfig for patch 2:
> > > > "Input: goodix - reset device at init". There are no other code
> > > > changes from v8.
> > > >
> > > > Thanks for testing these changes, Bastien and Aleksei!
> > > >
> > > > Karsten, there is no need to rebase your series on top of v9.
> > >
> > > Are we waiting on anything else before merging this? I'd like it to be
> > > scheduled to be merged so I can start focusing on the subsequent and
> > > dependent patches for that same driver.
> >
> > Hello,
> >
> > AFAICS there is one open point (cf.
> > http://www.spinics.net/lists/linux-input/msg41567.html) which
> > Irina wanted to address in a v10 of the patchset (cf.
> > http://www.spinics.net/lists/linux-input/msg41642.html).
> 
> There is also the whole thing about insane handling of named gpios in
> ACPI layer, which stops me from merging the reset code since these gpios
> should be marked as optional and we should stop ignoring errors coming
> from gpiolib.

The ACPI layer change is quite complex, since it includes changing the drivers
that use the gpio API before removing the fallback to indexed ACPI.
Not sure that will not also break current drivers that already count on this
fallback. Unfortunately, I do not have the time right now to get involved in
fixing the ACPI core myself.

Dmitry, is there anything I can do in the driver to get these patches merged?
I could go back to using indexed gpios and add an additional property
to specify if irq can be used as output or not (as suggested in one of the
previous reviews).

Thanks,
Irina

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

RE: [PATCH v9 0/9] Goodix touchscreen enhancements

[Tirdea, Irina]

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



> -----Original Message-----
> From: Karsten Merker [mailto:merker@debian.org]
> Sent: 26 October, 2015 20:21
> To: Bastien Nocera; Tirdea, Irina
> Cc: Dmitry Torokhov; Aleksei Mamlin; Karsten Merker; linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v9 0/9] Goodix touchscreen enhancements
> 
> On Mon, Oct 26, 2015 at 04:06:29PM +0100, Bastien Nocera wrote:
> > On Mon, 2015-10-12 at 18:24 +0300, Irina Tirdea wrote:
> 
> > > v9 only adds GPIOLIB dependency in Kconfig for patch 2:
> > > "Input: goodix - reset device at init". There are no other code
> > > changes from v8.
> > >
> > > Thanks for testing these changes, Bastien and Aleksei!
> > >
> > > Karsten, there is no need to rebase your series on top of v9.
> >
> > Are we waiting on anything else before merging this? I'd like it to be
> > scheduled to be merged so I can start focusing on the subsequent and
> > dependent patches for that same driver.
> 
> Hello,
> 
> AFAICS there is one open point (cf.
> http://www.spinics.net/lists/linux-input/msg41567.html) which
> Irina wanted to address in a v10 of the patchset (cf.
> http://www.spinics.net/lists/linux-input/msg41642.html).
> 
> Irina, how are your plans regarding the v10? It would be really
> nice if the patches could go into kernel 4.4, but the merge
> window opens on the coming weekend, so there is not much time
> left.

I can send v10 with the change mentioned above by the end of this week.

However, as Dmitry already mentioned, there is another issue with
the gpio ACPI layer that is blocking the entire patchset.

> 
> Bastien, did you have time to look at v3 of the axis
> swapping/inversion set?
> (http://www.spinics.net/lists/linux-input/msg41628.html)
> You had acked v2, but I had to do some small changes to address
> Irina's review comments after you had acked it, so I didn't want
> to carry your "acked-by" on to v3 without an ok from you.
> 
> Regards,
> Karsten
> --
> Gem. Par. 28 Abs. 4 Bundesdatenschutzgesetz widerspreche ich der Nutzung
> sowie der Weitergabe meiner personenbezogenen Daten für Zwecke der
> Werbung sowie der Markt- oder Meinungsforschung.
ÿôèº{.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 v9 0/9] Goodix touchscreen enhancements

[Tirdea, Irina]

> -----Original Message-----
> From: Karsten Merker [mailto:merker@debian.org]
> Sent: 26 October, 2015 20:21
> To: Bastien Nocera; Tirdea, Irina
> Cc: Dmitry Torokhov; Aleksei Mamlin; Karsten Merker; linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v9 0/9] Goodix touchscreen enhancements
> 
> On Mon, Oct 26, 2015 at 04:06:29PM +0100, Bastien Nocera wrote:
> > On Mon, 2015-10-12 at 18:24 +0300, Irina Tirdea wrote:
> 
> > > v9 only adds GPIOLIB dependency in Kconfig for patch 2:
> > > "Input: goodix - reset device at init". There are no other code
> > > changes from v8.
> > >
> > > Thanks for testing these changes, Bastien and Aleksei!
> > >
> > > Karsten, there is no need to rebase your series on top of v9.
> >
> > Are we waiting on anything else before merging this? I'd like it to be
> > scheduled to be merged so I can start focusing on the subsequent and
> > dependent patches for that same driver.
> 
> Hello,
> 
> AFAICS there is one open point (cf.
> http://www.spinics.net/lists/linux-input/msg41567.html) which
> Irina wanted to address in a v10 of the patchset (cf.
> http://www.spinics.net/lists/linux-input/msg41642.html).
> 
> Irina, how are your plans regarding the v10? It would be really
> nice if the patches could go into kernel 4.4, but the merge
> window opens on the coming weekend, so there is not much time
> left.

I can send v10 with the change mentioned above by the end of this week.

However, as Dmitry already mentioned, there is another issue with
the gpio ACPI layer that is blocking the entire patchset.

> 
> Bastien, did you have time to look at v3 of the axis
> swapping/inversion set?
> (http://www.spinics.net/lists/linux-input/msg41628.html)
> You had acked v2, but I had to do some small changes to address
> Irina's review comments after you had acked it, so I didn't want
> to carry your "acked-by" on to v3 without an ok from you.
> 
> Regards,
> Karsten
> --
> Gem. Par. 28 Abs. 4 Bundesdatenschutzgesetz widerspreche ich der Nutzung
> sowie der Weitergabe meiner personenbezogenen Daten für Zwecke der
> Werbung sowie der Markt- oder Meinungsforschung.

Re: [PATCH v9 2/9] Input: goodix - reset device at init

[Dmitry Torokhov]

On Mon, Oct 19, 2015 at 05:52:39PM +0300, mika.westerberg@linux.intel.com wrote:
> On Mon, Oct 19, 2015 at 02:32:24PM +0000, Tirdea, Irina wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@vger.kernel.org] On Behalf Of
> > > mika.westerberg@linux.intel.com
> > > Sent: 14 October, 2015 16:44
> > > To: Dmitry Torokhov
> > > Cc: Tirdea, Irina; Bastien Nocera; Aleksei Mamlin; Karsten Merker; linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> > > kernel@vger.kernel.org; devicetree@vger.kernel.org
> > > Subject: Re: [PATCH v9 2/9] Input: goodix - reset device at init
> > > 
> > > On Wed, Oct 14, 2015 at 02:18:20PM +0300, mika.westerberg@linux.intel.com wrote:
> > > > On Tue, Oct 13, 2015 at 11:23:03PM -0700, Dmitry Torokhov wrote:
> > > > > I understand why one might use acpi_dev_add_driver_gpios() to augment
> > > > > data in ACPI, however here we have completely different issue: driver
> > > > > that expects named gpios gets returned gpio that has nothing to do with
> > > > > what it requested, because gpiolib acpi code always falls back to
> > > > > unnamed gpio if it does not find named gpio. That can be acceptable if
> > > > > driver uses the same con_id for all requests to gpiolib, but is not
> > > > > working when driver supplies different con_ids.
> > > >
> > > > Right, the ACPI fallback ignores con_id completely and uses only the
> > > > index.
> > > >
> > > > AFAIK there is only one driver using ACPI _CRS index method:
> > > > sdhci-[acpi|pci].c. If we can convert that to use acpi_dev_add_driver_gpios()
> > > > to feed names for card detection GPIOs, I think we can remove the
> > > > fallback alltogether in favor of named GPIOs for ACPI.
> > > 
> > > Nah, there seems to be several drivers relying on this already :-/
> > 
> > Would it be possible to add an optional parameter to the GPIO API
> > to specify whether we want to fall back to indexed GPIOs for ACPI?
> 
> I don't think it's a good idea to add ACPI specifics to generic APIs.
> 
> I went through ACPI enabled drivers calling GPIO APIs and majority of
> them are doing this:
> 
> static int stk8312_gpio_probe(struct i2c_client *client)
> {
>         struct device *dev;
>         struct gpio_desc *gpio;
>         int ret;
> 
>         if (!client)
>                 return -EINVAL;
> 
>         dev = &client->dev;
> 
>         /* data ready gpio interrupt pin */
>         gpio = devm_gpiod_get_index(dev, STK8312_GPIO, 0, GPIOD_IN);
>         if (IS_ERR(gpio)) {
>                 dev_err(dev, "acpi gpio get index failed\n");
>                 return PTR_ERR(gpio);
>         }
> 
>         ret = gpiod_to_irq(gpio);
>         dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
> 
>         return ret;
> }
> 
> We can drop all this because I2C core already handles GpioInt -> interrupt
> number translation.
> 
> Few drivers are doing something more complex but I think we can still convert
> them to use acpi_dev_add_driver_gpios() and eventually get rid of the whole
> _CRS index lookup.

cpi_dev_add_driver_gpios() does not really help with generic drivers
(unless we keep adding more and more board specific data to them). How
about we keep track of names used and only allow conversion for the
first name used, like in the patch below?

-- 
Dmitry

gpiolib: tighten up ACPI legacy gpio lookups

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

We should not fall back to the legacy unnamed gpio lookup style if the
driver requests gpios with different names, because we'll give out the same
gpio twice. Let's keep track of the names that were used for the device and
only do the fallback for the first name used.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/gpio/gpiolib.c |   42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5db3445..4ae5447 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1738,6 +1738,45 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 	return desc;
 }
 
+struct acpi_gpio_lookup {
+	struct list_head node;
+	struct device *dev;
+	const char *con_id;
+};
+
+static DEFINE_MUTEX(acpi_gpio_lookup_lock);
+static LIST_HEAD(acpi_gpio_lookup_list);
+
+static bool acpi_can_fallback_crs(struct device *dev, const char *con_id)
+{
+	struct acpi_gpio_lookup *l, *lookup = NULL;
+
+	mutex_lock(&acpi_gpio_lookup_lock);
+
+	list_for_each_entry(l, &acpi_gpio_lookup_list, node) {
+		if (l->dev == dev) {
+			lookup = l;
+			break;
+		}
+	}
+
+	if (!lookup) {
+		lookup = kmalloc(sizeof(*lookup), GFP_KERNEL);
+		if (lookup) {
+			lookup->dev = dev;
+			lookup->con_id = con_id;
+			list_add_tail(&lookup->node, &acpi_gpio_lookup_list);
+		}
+	}
+
+	mutex_lock(&acpi_gpio_lookup_lock);
+
+	return lookup &&
+		((!lookup->con_id && !con_id) ||
+		 (lookup->con_id && con_id &&
+		  strcmp(lookup->con_id, con_id) == 0));
+}
+
 static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
 					unsigned int idx,
 					enum gpio_lookup_flags *flags)
@@ -1765,7 +1804,8 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
 
 	/* Then from plain _CRS GPIOs */
 	if (IS_ERR(desc)) {
-		desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
+		if (acpi_can_fallback_crs(dev, con_id))
+			desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
 		if (IS_ERR(desc))
 			return desc;
 	}

Re: [PATCH v9 2/9] Input: goodix - reset device at init

[Dmitry Torokhov]

On Fri, Oct 30, 2015 at 09:33:28AM -0700, Dmitry Torokhov wrote:
> On Mon, Oct 19, 2015 at 05:52:39PM +0300, mika.westerberg@linux.intel.com wrote:
> > On Mon, Oct 19, 2015 at 02:32:24PM +0000, Tirdea, Irina wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: linux-input-owner@vger.kernel.org [mailto:linux-input-owner@vger.kernel.org] On Behalf Of
> > > > mika.westerberg@linux.intel.com
> > > > Sent: 14 October, 2015 16:44
> > > > To: Dmitry Torokhov
> > > > Cc: Tirdea, Irina; Bastien Nocera; Aleksei Mamlin; Karsten Merker; linux-input@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> > > > kernel@vger.kernel.org; devicetree@vger.kernel.org
> > > > Subject: Re: [PATCH v9 2/9] Input: goodix - reset device at init
> > > > 
> > > > On Wed, Oct 14, 2015 at 02:18:20PM +0300, mika.westerberg@linux.intel.com wrote:
> > > > > On Tue, Oct 13, 2015 at 11:23:03PM -0700, Dmitry Torokhov wrote:
> > > > > > I understand why one might use acpi_dev_add_driver_gpios() to augment
> > > > > > data in ACPI, however here we have completely different issue: driver
> > > > > > that expects named gpios gets returned gpio that has nothing to do with
> > > > > > what it requested, because gpiolib acpi code always falls back to
> > > > > > unnamed gpio if it does not find named gpio. That can be acceptable if
> > > > > > driver uses the same con_id for all requests to gpiolib, but is not
> > > > > > working when driver supplies different con_ids.
> > > > >
> > > > > Right, the ACPI fallback ignores con_id completely and uses only the
> > > > > index.
> > > > >
> > > > > AFAIK there is only one driver using ACPI _CRS index method:
> > > > > sdhci-[acpi|pci].c. If we can convert that to use acpi_dev_add_driver_gpios()
> > > > > to feed names for card detection GPIOs, I think we can remove the
> > > > > fallback alltogether in favor of named GPIOs for ACPI.
> > > > 
> > > > Nah, there seems to be several drivers relying on this already :-/
> > > 
> > > Would it be possible to add an optional parameter to the GPIO API
> > > to specify whether we want to fall back to indexed GPIOs for ACPI?
> > 
> > I don't think it's a good idea to add ACPI specifics to generic APIs.
> > 
> > I went through ACPI enabled drivers calling GPIO APIs and majority of
> > them are doing this:
> > 
> > static int stk8312_gpio_probe(struct i2c_client *client)
> > {
> >         struct device *dev;
> >         struct gpio_desc *gpio;
> >         int ret;
> > 
> >         if (!client)
> >                 return -EINVAL;
> > 
> >         dev = &client->dev;
> > 
> >         /* data ready gpio interrupt pin */
> >         gpio = devm_gpiod_get_index(dev, STK8312_GPIO, 0, GPIOD_IN);
> >         if (IS_ERR(gpio)) {
> >                 dev_err(dev, "acpi gpio get index failed\n");
> >                 return PTR_ERR(gpio);
> >         }
> > 
> >         ret = gpiod_to_irq(gpio);
> >         dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
> > 
> >         return ret;
> > }
> > 
> > We can drop all this because I2C core already handles GpioInt -> interrupt
> > number translation.
> > 
> > Few drivers are doing something more complex but I think we can still convert
> > them to use acpi_dev_add_driver_gpios() and eventually get rid of the whole
> > _CRS index lookup.
> 
> cpi_dev_add_driver_gpios() does not really help with generic drivers
> (unless we keep adding more and more board specific data to them). How
> about we keep track of names used and only allow conversion for the
> first name used, like in the patch below?
> 
> -- 
> Dmitry
> 
> gpiolib: tighten up ACPI legacy gpio lookups
> 
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> We should not fall back to the legacy unnamed gpio lookup style if the
> driver requests gpios with different names, because we'll give out the same
> gpio twice. Let's keep track of the names that were used for the device and
> only do the fallback for the first name used.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/gpio/gpiolib.c |   42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5db3445..4ae5447 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1738,6 +1738,45 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
>  	return desc;
>  }
>  
> +struct acpi_gpio_lookup {
> +	struct list_head node;
> +	struct device *dev;
> +	const char *con_id;
> +};
> +
> +static DEFINE_MUTEX(acpi_gpio_lookup_lock);
> +static LIST_HEAD(acpi_gpio_lookup_list);
> +
> +static bool acpi_can_fallback_crs(struct device *dev, const char *con_id)
> +{
> +	struct acpi_gpio_lookup *l, *lookup = NULL;
> +
> +	mutex_lock(&acpi_gpio_lookup_lock);
> +
> +	list_for_each_entry(l, &acpi_gpio_lookup_list, node) {
> +		if (l->dev == dev) {
> +			lookup = l;
> +			break;
> +		}
> +	}
> +
> +	if (!lookup) {
> +		lookup = kmalloc(sizeof(*lookup), GFP_KERNEL);
> +		if (lookup) {
> +			lookup->dev = dev;
> +			lookup->con_id = con_id;
> +			list_add_tail(&lookup->node, &acpi_gpio_lookup_list);
> +		}
> +	}
> +
> +	mutex_lock(&acpi_gpio_lookup_lock);

This should of course be mutex_unlock() according to the awesome 0-day
build bot.

> +
> +	return lookup &&
> +		((!lookup->con_id && !con_id) ||
> +		 (lookup->con_id && con_id &&
> +		  strcmp(lookup->con_id, con_id) == 0));
> +}
> +
>  static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>  					unsigned int idx,
>  					enum gpio_lookup_flags *flags)
> @@ -1765,7 +1804,8 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>  
>  	/* Then from plain _CRS GPIOs */
>  	if (IS_ERR(desc)) {
> -		desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
> +		if (acpi_can_fallback_crs(dev, con_id))
> +			desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
>  		if (IS_ERR(desc))
>  			return desc;
>  	}

-- 
Dmitry

Re: [PATCH v9 2/9] Input: goodix - reset device at init

[mika.westerberg]

On Fri, Oct 30, 2015 at 09:33:28AM -0700, Dmitry Torokhov wrote:
> cpi_dev_add_driver_gpios() does not really help with generic drivers
> (unless we keep adding more and more board specific data to them). How
> about we keep track of names used and only allow conversion for the
> first name used, like in the patch below?

That works but it misses one case: When you have optional GPIOs and not
all of them are provided. Currently it ends up the same situation
returning the same GPIO.

I've commented below how we could handle that case as well.

> -- 
> Dmitry
> 
> gpiolib: tighten up ACPI legacy gpio lookups
> 
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> We should not fall back to the legacy unnamed gpio lookup style if the
> driver requests gpios with different names, because we'll give out the same
> gpio twice. Let's keep track of the names that were used for the device and
> only do the fallback for the first name used.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/gpio/gpiolib.c |   42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5db3445..4ae5447 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1738,6 +1738,45 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
>  	return desc;
>  }
>  
> +struct acpi_gpio_lookup {
> +	struct list_head node;
> +	struct device *dev;
> +	const char *con_id;
> +};
> +
> +static DEFINE_MUTEX(acpi_gpio_lookup_lock);
> +static LIST_HEAD(acpi_gpio_lookup_list);
> +
> +static bool acpi_can_fallback_crs(struct device *dev, const char *con_id)
> +{
> +	struct acpi_gpio_lookup *l, *lookup = NULL;

I'm thinking if we here do

	struct acpi_device *adev = ACPI_COMPANION(dev);

	/* Never fallback if the device has properties */
	if (adev->data.properties || adev->driver_gpios)
		return false;

> +
> +	mutex_lock(&acpi_gpio_lookup_lock);
> +
> +	list_for_each_entry(l, &acpi_gpio_lookup_list, node) {
> +		if (l->dev == dev) {
> +			lookup = l;
> +			break;
> +		}
> +	}
> +
> +	if (!lookup) {
> +		lookup = kmalloc(sizeof(*lookup), GFP_KERNEL);
> +		if (lookup) {
> +			lookup->dev = dev;
> +			lookup->con_id = con_id;
> +			list_add_tail(&lookup->node, &acpi_gpio_lookup_list);
> +		}
> +	}
> +
> +	mutex_lock(&acpi_gpio_lookup_lock);
> +
> +	return lookup &&
> +		((!lookup->con_id && !con_id) ||
> +		 (lookup->con_id && con_id &&
> +		  strcmp(lookup->con_id, con_id) == 0));
> +}
> +
>  static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>  					unsigned int idx,
>  					enum gpio_lookup_flags *flags)
> @@ -1765,7 +1804,8 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>  
>  	/* Then from plain _CRS GPIOs */
>  	if (IS_ERR(desc)) {
> -		desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
> +		if (acpi_can_fallback_crs(dev, con_id))
> +			desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);

and here we could do

		if (!acpi_can_fallback_crs(dev, con_id))
			return ERR_PTR(-ENOENT);
		desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);

So instead return -ENOENT so that gpiod_get_index_optional() handles the
missing optional GPIO properly.

>  		if (IS_ERR(desc))
>  			return desc;
>  	}

Re: [PATCH v9 2/9] Input: goodix - reset device at init

[mika.westerberg-VuQAYsv1563Yd54FQh9/CA]

On Fri, Oct 30, 2015 at 09:33:28AM -0700, Dmitry Torokhov wrote:
> cpi_dev_add_driver_gpios() does not really help with generic drivers
> (unless we keep adding more and more board specific data to them). How
> about we keep track of names used and only allow conversion for the
> first name used, like in the patch below?

That works but it misses one case: When you have optional GPIOs and not
all of them are provided. Currently it ends up the same situation
returning the same GPIO.

I've commented below how we could handle that case as well.

> -- 
> Dmitry
> 
> gpiolib: tighten up ACPI legacy gpio lookups
> 
> From: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> We should not fall back to the legacy unnamed gpio lookup style if the
> driver requests gpios with different names, because we'll give out the same
> gpio twice. Let's keep track of the names that were used for the device and
> only do the fallback for the first name used.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/gpio/gpiolib.c |   42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5db3445..4ae5447 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1738,6 +1738,45 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
>  	return desc;
>  }
>  
> +struct acpi_gpio_lookup {
> +	struct list_head node;
> +	struct device *dev;
> +	const char *con_id;
> +};
> +
> +static DEFINE_MUTEX(acpi_gpio_lookup_lock);
> +static LIST_HEAD(acpi_gpio_lookup_list);
> +
> +static bool acpi_can_fallback_crs(struct device *dev, const char *con_id)
> +{
> +	struct acpi_gpio_lookup *l, *lookup = NULL;

I'm thinking if we here do

	struct acpi_device *adev = ACPI_COMPANION(dev);

	/* Never fallback if the device has properties */
	if (adev->data.properties || adev->driver_gpios)
		return false;

> +
> +	mutex_lock(&acpi_gpio_lookup_lock);
> +
> +	list_for_each_entry(l, &acpi_gpio_lookup_list, node) {
> +		if (l->dev == dev) {
> +			lookup = l;
> +			break;
> +		}
> +	}
> +
> +	if (!lookup) {
> +		lookup = kmalloc(sizeof(*lookup), GFP_KERNEL);
> +		if (lookup) {
> +			lookup->dev = dev;
> +			lookup->con_id = con_id;
> +			list_add_tail(&lookup->node, &acpi_gpio_lookup_list);
> +		}
> +	}
> +
> +	mutex_lock(&acpi_gpio_lookup_lock);
> +
> +	return lookup &&
> +		((!lookup->con_id && !con_id) ||
> +		 (lookup->con_id && con_id &&
> +		  strcmp(lookup->con_id, con_id) == 0));
> +}
> +
>  static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>  					unsigned int idx,
>  					enum gpio_lookup_flags *flags)
> @@ -1765,7 +1804,8 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>  
>  	/* Then from plain _CRS GPIOs */
>  	if (IS_ERR(desc)) {
> -		desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
> +		if (acpi_can_fallback_crs(dev, con_id))
> +			desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);

and here we could do

		if (!acpi_can_fallback_crs(dev, con_id))
			return ERR_PTR(-ENOENT);
		desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);

So instead return -ENOENT so that gpiod_get_index_optional() handles the
missing optional GPIO properly.

>  		if (IS_ERR(desc))
>  			return desc;
>  	}
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html