[PATCH v12 0/5] Goodix touchscreen enhancements

[PATCH v12 0/5] Goodix touchscreen enhancements

[Irina Tirdea]

This is an update for a previous set of patches [1] with enhancements
for the Goodix touchscreens. The first 3 patches are leftovers from
the previous patchset (with minor changes due to rebase), while the
last 2 patches are new.

Thanks,
Irina

[1] https://lkml.org/lkml/2015/11/19/241

Changes in v12:
 - add documentation for configuration firmware update
 - minor changes due to rebase
 - add 2 new patches to fix reset sequence and add support for gt9157

Previous changes:
  https://lkml.org/lkml/2015/11/19/241

Irina Tirdea (5):
  Input: goodix - add sysfs interface to dump config
  Input: goodix - add support for ESD
  Input: goodix - add runtime power management support
  Input: goodix - fix reset sequence
  Input: goodix - add support for gt9157

 .../bindings/input/touchscreen/goodix.txt          |   5 +
 Documentation/input/goodix.txt                     |  84 +++++
 drivers/input/touchscreen/goodix.c                 | 346 +++++++++++++++++++--
 3 files changed, 415 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/input/goodix.txt

-- 
1.9.1

[PATCH v12 1/5] Input: goodix - add sysfs interface to dump config

[Irina Tirdea]

Goodix devices have a configuration information register area that
specifies 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>
---
 Documentation/input/goodix.txt     | 84 ++++++++++++++++++++++++++++++++++++++
 drivers/input/touchscreen/goodix.c | 64 ++++++++++++++++++++++++++---
 2 files changed, 143 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/input/goodix.txt

diff --git a/Documentation/input/goodix.txt b/Documentation/input/goodix.txt
new file mode 100644
index 0000000..f9be1e2
--- /dev/null
+++ b/Documentation/input/goodix.txt
@@ -0,0 +1,84 @@
+Goodix touchscreen driver
+=====================================
+
+How to update configuration firmware
+=====================================
+
+Goodix touchscreen devices have a set of registers that specify configuration
+information 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).
+
+The devices have an initial default configuration that can be read through
+the sysfs interface (/sys/class/input/inputX/device/dump_config). This default
+configuration can be used as a starting point for creating a new configuration
+firmware file. At init, the driver will read the configuration firmware file
+and update the device configuration.
+
+This configuration can be accesed only if both interrupt and reset gpio pins
+are connected and properly configured through ACPI _DSD/DT properties.
+
+Below are instructions on how to generate a valid configuration starting from
+the device default configuration.
+
+1. Dump the default configuration of the device to a file:
+  $ cat /sys/class/input/inputX/device/dump_config > goodix_<model>_cfg
+
+2. Make the needed changes to the configuration (e.g. change resolution of
+x/y axes, maximum reported touch points, switch X,Y axes, etc.). For more
+details check the Goodix datasheet for format of Configuration Registers.
+
+3. Generate a valid configuration starting from  goodix_<model>_cfg.
+After making changes, you need to recompute the checksum of the entire
+configuration data, set Config_Fresh to 1 and generate the binary config
+firmware image. This can be done using a helper script similar to the
+one below:
+
+#!/bin/bash
+
+if [[ $# -lt 1 ]]; then
+    echo "$0 fw_filename"
+    exit 1
+fi
+
+file_in="$1"
+file_out_bin=${file_in}.bin
+
+print_val ()
+{
+    val="$1"
+    printf "0x%.2x" "$val" | xxd -r -p >> ${file_out_bin}
+}
+
+rm -f ${file_out_bin}
+
+size=`cat ${file_in} | wc -w`
+
+checksum=0
+i=1
+for val in `cat ${file_in}`; do
+    val="0x$val"
+    if [[ $i == $size ]]; then
+	# Config_Fresh
+	print_val 0x01
+    elif [[ $i == $((size-1)) ]]; then
+	# Config_Chksum
+	checksum=$(( (~ checksum + 1) & 0xFF))
+	print_val $checksum
+    else
+	checksum=$((checksum + val))
+	print_val $val
+    fi
+    i=$((i+1))
+done
+
+echo "Wrote ${file_out_bin}"
+
+4. Copy the binary config firmware in the appropriate location
+(e.g. /lib/firmware), using the name goodix_<model>_cfg.bin (e.g. for gt911,
+use goodix_911_cfg.bin).
+
+5. Check that the new firmware was successfully written to the device
+after reboot. Config_Fresh is reset to 0 after a successful update of the
+configuration.
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 240b16f..2447b73 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -430,6 +430,40 @@ static int goodix_reset(struct goodix_ts_data *ts)
 	return 0;
 }
 
+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;
+
+	wait_for_completion(&ts->firmware_loading_complete);
+
+	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;
+}
+
+static DEVICE_ATTR(dump_config, S_IRUGO, goodix_dump_config_show, NULL);
+
+static struct attribute *goodix_attrs[] = {
+	&dev_attr_dump_config.attr,
+	NULL
+};
+
+static const struct attribute_group goodix_attr_group = {
+	.attrs = goodix_attrs,
+};
+
 /**
  * goodix_get_gpio_config - Get GPIO config from ACPI/DT
  *
@@ -735,11 +769,22 @@ 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 = 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 = devm_kasprintf(&client->dev, 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,
@@ -748,7 +793,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 			dev_err(&client->dev,
 				"Failed to invoke firmware loader: %d\n",
 				error);
-			return error;
+			goto err_sysfs_remove_group;
 		}
 
 		return 0;
@@ -759,14 +804,23 @@ static int goodix_ts_probe(struct i2c_client *client,
 	}
 
 	return 0;
+
+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)
-		wait_for_completion(&ts->firmware_loading_complete);
+	if (!ts->gpiod_int || !ts->gpiod_rst)
+		return 0;
+
+	wait_for_completion(&ts->firmware_loading_complete);
+
+	sysfs_remove_group(&client->dev.kobj, &goodix_attr_group);
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH v12 1/5] Input: goodix - add sysfs interface to dump config

[Bastien Nocera]

On Sat, 2016-09-10 at 20:57 +0300, Irina Tirdea wrote:
> Goodix devices have a configuration information register area that
> specifies 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.

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

Only thing I don't like is the overly complicated bash script. I'd
really rather the code was in C, making it easier to debug, and not
rely on a fair number of external utilities.

> 
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> ---
>  Documentation/input/goodix.txt     | 84
> ++++++++++++++++++++++++++++++++++++++
>  drivers/input/touchscreen/goodix.c | 64 ++++++++++++++++++++++++++
> ---
>  2 files changed, 143 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/input/goodix.txt
> 
> diff --git a/Documentation/input/goodix.txt
> b/Documentation/input/goodix.txt
> new file mode 100644
> index 0000000..f9be1e2
> --- /dev/null
> +++ b/Documentation/input/goodix.txt
> @@ -0,0 +1,84 @@
> +Goodix touchscreen driver
> +=====================================
> +
> +How to update configuration firmware
> +=====================================
> +
> +Goodix touchscreen devices have a set of registers that specify
> configuration
> +information 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).
> +
> +The devices have an initial default configuration that can be read
> through
> +the sysfs interface (/sys/class/input/inputX/device/dump_config).
> This default
> +configuration can be used as a starting point for creating a new
> configuration
> +firmware file. At init, the driver will read the configuration
> firmware file
> +and update the device configuration.
> +
> +This configuration can be accesed only if both interrupt and reset
> gpio pins
> +are connected and properly configured through ACPI _DSD/DT
> properties.
> +
> +Below are instructions on how to generate a valid configuration
> starting from
> +the device default configuration.
> +
> +1. Dump the default configuration of the device to a file:
> +  $ cat /sys/class/input/inputX/device/dump_config >
> goodix_<model>_cfg
> +
> +2. Make the needed changes to the configuration (e.g. change
> resolution of
> +x/y axes, maximum reported touch points, switch X,Y axes, etc.). For
> more
> +details check the Goodix datasheet for format of Configuration
> Registers.
> +
> +3. Generate a valid configuration starting from  goodix_<model>_cfg.
> +After making changes, you need to recompute the checksum of the
> entire
> +configuration data, set Config_Fresh to 1 and generate the binary
> config
> +firmware image. This can be done using a helper script similar to
> the
> +one below:
> +
> +#!/bin/bash
> +
> +if [[ $# -lt 1 ]]; then
> +    echo "$0 fw_filename"
> +    exit 1
> +fi
> +
> +file_in="$1"
> +file_out_bin=${file_in}.bin
> +
> +print_val ()
> +{
> +    val="$1"
> +    printf "0x%.2x" "$val" | xxd -r -p >> ${file_out_bin}
> +}
> +
> +rm -f ${file_out_bin}
> +
> +size=`cat ${file_in} | wc -w`
> +
> +checksum=0
> +i=1
> +for val in `cat ${file_in}`; do
> +    val="0x$val"
> +    if [[ $i == $size ]]; then
> +	# Config_Fresh
> +	print_val 0x01
> +    elif [[ $i == $((size-1)) ]]; then
> +	# Config_Chksum
> +	checksum=$(( (~ checksum + 1) & 0xFF))
> +	print_val $checksum
> +    else
> +	checksum=$((checksum + val))
> +	print_val $val
> +    fi
> +    i=$((i+1))
> +done
> +
> +echo "Wrote ${file_out_bin}"
> +
> +4. Copy the binary config firmware in the appropriate location
> +(e.g. /lib/firmware), using the name goodix_<model>_cfg.bin (e.g.
> for gt911,
> +use goodix_911_cfg.bin).
> +
> +5. Check that the new firmware was successfully written to the
> device
> +after reboot. Config_Fresh is reset to 0 after a successful update
> of the
> +configuration.
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 240b16f..2447b73 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -430,6 +430,40 @@ static int goodix_reset(struct goodix_ts_data
> *ts)
>  	return 0;
>  }
>  
> +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;
> +
> +	wait_for_completion(&ts->firmware_loading_complete);
> +
> +	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;
> +}
> +
> +static DEVICE_ATTR(dump_config, S_IRUGO, goodix_dump_config_show,
> NULL);
> +
> +static struct attribute *goodix_attrs[] = {
> +	&dev_attr_dump_config.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group goodix_attr_group = {
> +	.attrs = goodix_attrs,
> +};
> +
>  /**
>   * goodix_get_gpio_config - Get GPIO config from ACPI/DT
>   *
> @@ -735,11 +769,22 @@ 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 = 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 = devm_kasprintf(&client->dev,
> 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,
> @@ -748,7 +793,7 @@ static int goodix_ts_probe(struct i2c_client
> *client,
>  			dev_err(&client->dev,
>  				"Failed to invoke firmware loader:
> %d\n",
>  				error);
> -			return error;
> +			goto err_sysfs_remove_group;
>  		}
>  
>  		return 0;
> @@ -759,14 +804,23 @@ static int goodix_ts_probe(struct i2c_client
> *client,
>  	}
>  
>  	return 0;
> +
> +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)
> -		wait_for_completion(&ts->firmware_loading_complete);
> +	if (!ts->gpiod_int || !ts->gpiod_rst)
> +		return 0;
> +
> +	wait_for_completion(&ts->firmware_loading_complete);
> +
> +	sysfs_remove_group(&client->dev.kobj, &goodix_attr_group);
>  
>  	return 0;
>  }

[PATCH v12 2/5] 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>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../bindings/input/touchscreen/goodix.txt          |   4 +
 drivers/input/touchscreen/goodix.c                 | 130 ++++++++++++++++++++-
 2 files changed, 132 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index c98757a..ef5f42d 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -18,6 +18,10 @@ Optional properties:
  - irq-gpios		: GPIO pin used for IRQ. The driver uses the
 			  interrupt gpio pin as output to reset the device.
  - reset-gpios		: GPIO pin used for reset
+ - 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.
 
  - touchscreen-inverted-x  : X axis is inverted (boolean)
  - touchscreen-inverted-y  : Y axis is inverted (boolean)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 2447b73..cf39dc4 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -49,10 +49,13 @@ struct goodix_ts_data {
 	const char *cfg_name;
 	struct completion firmware_loading_complete;
 	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
@@ -67,6 +70,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
@@ -453,10 +458,114 @@ static ssize_t goodix_dump_config_show(struct device *dev,
 	return count;
 }
 
+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;
+
+	wait_for_completion(&ts->firmware_loading_complete);
+
+	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;
+}
+
 static DEVICE_ATTR(dump_config, S_IRUGO, goodix_dump_config_show, NULL);
+/* 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_dump_config.attr,
+	&dev_attr_esd_timeout.attr,
 	NULL
 };
 
@@ -713,7 +822,11 @@ static void goodix_config_cb(const struct firmware *cfg, void *ctx)
 			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:
 	release_firmware(cfg);
@@ -724,7 +837,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);
 
@@ -740,6 +853,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 	ts->client = client;
 	i2c_set_clientdata(client, ts);
 	init_completion(&ts->firmware_loading_complete);
+	INIT_DELAYED_WORK(&ts->esd_work, goodix_esd_work);
 
 	error = goodix_get_gpio_config(ts);
 	if (error)
@@ -769,6 +883,12 @@ 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) {
@@ -821,6 +941,7 @@ static int goodix_ts_remove(struct i2c_client *client)
 	wait_for_completion(&ts->firmware_loading_complete);
 
 	sysfs_remove_group(&client->dev.kobj, &goodix_attr_group);
+	goodix_disable_esd(ts);
 
 	return 0;
 }
@@ -836,6 +957,7 @@ static int __maybe_unused goodix_suspend(struct device *dev)
 		return 0;
 
 	wait_for_completion(&ts->firmware_loading_complete);
+	goodix_disable_esd(ts);
 
 	/* Free IRQ as IRQ pin is used as output in the suspend sequence */
 	goodix_free_irq(ts);
@@ -894,6 +1016,10 @@ static int __maybe_unused goodix_resume(struct device *dev)
 	if (error)
 		return error;
 
+	error = goodix_enable_esd(ts);
+	if (error)
+		return error;
+
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH v12 2/5] Input: goodix - add support for ESD

[Bastien Nocera]

On Sat, 2016-09-10 at 20:57 +0300, Irina Tirdea wrote:
> 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.

What's the default value though?

>  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 }},
>              ...
>          }
>      })
>  }

Are there any devices which ship with that information? What do the
Windows drivers use as a default for ESD, and how is it declared in
ACPI?

> 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).

Could you link to a "close to upstream" tree that includes that
support? Do they use the same timeout, or is it set to a default
hardcoded value instead?

Cheers

> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>  .../bindings/input/touchscreen/goodix.txt          |   4 +
>  drivers/input/touchscreen/goodix.c                 | 130
> ++++++++++++++++++++-
>  2 files changed, 132 insertions(+), 2 deletions(-)
> 
> diff --git
> a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> index c98757a..ef5f42d 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> @@ -18,6 +18,10 @@ Optional properties:
>   - irq-gpios		: GPIO pin used for IRQ. The driver uses
> the
>  			  interrupt gpio pin as output to reset the
> device.
>   - reset-gpios		: GPIO pin used for reset
> + - 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.
>  
>   - touchscreen-inverted-x  : X axis is inverted (boolean)
>   - touchscreen-inverted-y  : Y axis is inverted (boolean)
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 2447b73..cf39dc4 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -49,10 +49,13 @@ struct goodix_ts_data {
>  	const char *cfg_name;
>  	struct completion firmware_loading_complete;
>  	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
> @@ -67,6 +70,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
> @@ -453,10 +458,114 @@ static ssize_t goodix_dump_config_show(struct
> device *dev,
>  	return count;
>  }
>  
> +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;
> +
> +	wait_for_completion(&ts->firmware_loading_complete);
> +
> +	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;
> +}
> +
>  static DEVICE_ATTR(dump_config, S_IRUGO, goodix_dump_config_show,
> NULL);
> +/* 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_dump_config.attr,
> +	&dev_attr_esd_timeout.attr,
>  	NULL
>  };
>  
> @@ -713,7 +822,11 @@ static void goodix_config_cb(const struct
> firmware *cfg, void *ctx)
>  			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:
>  	release_firmware(cfg);
> @@ -724,7 +837,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);
>  
> @@ -740,6 +853,7 @@ static int goodix_ts_probe(struct i2c_client
> *client,
>  	ts->client = client;
>  	i2c_set_clientdata(client, ts);
>  	init_completion(&ts->firmware_loading_complete);
> +	INIT_DELAYED_WORK(&ts->esd_work, goodix_esd_work);
>  
>  	error = goodix_get_gpio_config(ts);
>  	if (error)
> @@ -769,6 +883,12 @@ 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_PR
> OPERTY,
> +					&esd_timeout);
> +		if (!error)
> +			atomic_set(&ts->esd_timeout, esd_timeout);
> +
>  		error = sysfs_create_group(&client->dev.kobj,
>  					   &goodix_attr_group);
>  		if (error) {
> @@ -821,6 +941,7 @@ static int goodix_ts_remove(struct i2c_client
> *client)
>  	wait_for_completion(&ts->firmware_loading_complete);
>  
>  	sysfs_remove_group(&client->dev.kobj, &goodix_attr_group);
> +	goodix_disable_esd(ts);
>  
>  	return 0;
>  }
> @@ -836,6 +957,7 @@ static int __maybe_unused goodix_suspend(struct
> device *dev)
>  		return 0;
>  
>  	wait_for_completion(&ts->firmware_loading_complete);
> +	goodix_disable_esd(ts);
>  
>  	/* Free IRQ as IRQ pin is used as output in the suspend
> sequence */
>  	goodix_free_irq(ts);
> @@ -894,6 +1016,10 @@ static int __maybe_unused goodix_resume(struct
> device *dev)
>  	if (error)
>  		return error;
>  
> +	error = goodix_enable_esd(ts);
> +	if (error)
> +		return error;
> +
>  	return 0;
>  }
>  

[PATCH v12 3/5] 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>
---
 drivers/input/touchscreen/goodix.c | 156 +++++++++++++++++++++++++++++++++----
 1 file changed, 142 insertions(+), 14 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index cf39dc4..182ff9c 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -29,6 +29,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 {
@@ -51,6 +52,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"
@@ -81,6 +86,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,
@@ -198,6 +205,29 @@ static int goodix_get_cfg_len(u16 id)
 	}
 }
 
+static int goodix_set_power_state(struct goodix_ts_data *ts, bool on)
+{
+	int error;
+
+	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;
@@ -444,6 +474,10 @@ static ssize_t goodix_dump_config_show(struct device *dev,
 
 	wait_for_completion(&ts->firmware_loading_complete);
 
+	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) {
@@ -452,6 +486,8 @@ static ssize_t goodix_dump_config_show(struct device *dev,
 		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]);
@@ -548,11 +584,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;
@@ -573,6 +611,34 @@ 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;
+
+	if (!ts->gpiod_int || !ts->gpiod_rst)
+		return 0;
+
+	wait_for_completion(&ts->firmware_loading_complete);
+
+	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);
+
+	if (!ts->gpiod_int || !ts->gpiod_rst)
+		return;
+
+	goodix_set_power_state(ts, false);
+	atomic_dec(&ts->open_count);
+}
+
 /**
  * goodix_get_gpio_config - Get GPIO config from ACPI/DT
  *
@@ -754,6 +820,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) {
@@ -808,7 +877,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)
 {
@@ -828,6 +898,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);
 	complete_all(&ts->firmware_loading_complete);
@@ -854,6 +937,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, ts);
 	init_completion(&ts->firmware_loading_complete);
 	INIT_DELAYED_WORK(&ts->esd_work, goodix_esd_work);
+	mutex_init(&ts->mutex);
 
 	error = goodix_get_gpio_config(ts);
 	if (error)
@@ -940,23 +1024,33 @@ static int goodix_ts_remove(struct i2c_client *client)
 
 	wait_for_completion(&ts->firmware_loading_complete);
 
+	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);
 
 	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;
 
 	wait_for_completion(&ts->firmware_loading_complete);
+
+	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 */
@@ -966,7 +1060,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);
@@ -977,7 +1071,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;
 	}
 
 	/*
@@ -986,44 +1081,77 @@ 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;
 
 	error = goodix_enable_esd(ts);
 	if (error)
-		return 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

Re: [PATCH v12 3/5] Input: goodix - add runtime power management support

[Bastien Nocera]

On Sat, 2016-09-10 at 20:57 +0300, Irina Tirdea wrote:
> 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> 
<snip> 
> +static int goodix_set_power_state(struct goodix_ts_data *ts, bool
> on)

I don't like having booleans for this sort of thing[1], as it's never
clear whether "on" is "power state on" or "device is on".

I'd rather you defined a 2 member enum, with meaningful names, so that
the effect is clear when we're reading the caller.

[1]: https://blog.ometer.com/2011/01/20/boolean-parameters-are-wrong/

> +{
> +	int error;
> +
> +	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;
> @@ -444,6 +474,10 @@ static ssize_t goodix_dump_config_show(struct
> device *dev,
>  
>  	wait_for_completion(&ts->firmware_loading_complete);
>  
> +	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) {
> @@ -452,6 +486,8 @@ static ssize_t goodix_dump_config_show(struct
> device *dev,
>  		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]);
> @@ -548,11 +584,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;
> @@ -573,6 +611,34 @@ 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;
> +
> +	if (!ts->gpiod_int || !ts->gpiod_rst)
> +		return 0;
> +
> +	wait_for_completion(&ts->firmware_loading_complete);
> +
> +	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);
> +
> +	if (!ts->gpiod_int || !ts->gpiod_rst)
> +		return;
> +
> +	goodix_set_power_state(ts, false);
> +	atomic_dec(&ts->open_count);
> +}
> +
>  /**
>   * goodix_get_gpio_config - Get GPIO config from ACPI/DT
>   *
> @@ -754,6 +820,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) {
> @@ -808,7 +877,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)
>  {
> @@ -828,6 +898,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);
>  	complete_all(&ts->firmware_loading_complete);
> @@ -854,6 +937,7 @@ static int goodix_ts_probe(struct i2c_client
> *client,
>  	i2c_set_clientdata(client, ts);
>  	init_completion(&ts->firmware_loading_complete);
>  	INIT_DELAYED_WORK(&ts->esd_work, goodix_esd_work);
> +	mutex_init(&ts->mutex);
>  
>  	error = goodix_get_gpio_config(ts);
>  	if (error)
> @@ -940,23 +1024,33 @@ static int goodix_ts_remove(struct i2c_client
> *client)
>  
>  	wait_for_completion(&ts->firmware_loading_complete);
>  
> +	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);
>  
>  	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;
>  
>  	wait_for_completion(&ts->firmware_loading_complete);
> +
> +	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 */
> @@ -966,7 +1060,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);
> @@ -977,7 +1071,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;
>  	}
>  
>  	/*
> @@ -986,44 +1081,77 @@ 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;
>  
>  	error = goodix_enable_esd(ts);
>  	if (error)
> -		return 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 },
--
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 v12 3/5] Input: goodix - add runtime power management support

[Bastien Nocera]

On Sat, 2016-09-10 at 20:57 +0300, Irina Tirdea wrote:
> 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>
> ---
> 
<snip> 
> +static int goodix_set_power_state(struct goodix_ts_data *ts, bool
> on)

I don't like having booleans for this sort of thing[1], as it's never
clear whether "on" is "power state on" or "device is on".

I'd rather you defined a 2 member enum, with meaningful names, so that
the effect is clear when we're reading the caller.

[1]: https://blog.ometer.com/2011/01/20/boolean-parameters-are-wrong/

> +{
> +	int error;
> +
> +	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;
> @@ -444,6 +474,10 @@ static ssize_t goodix_dump_config_show(struct
> device *dev,
>  
>  	wait_for_completion(&ts->firmware_loading_complete);
>  
> +	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) {
> @@ -452,6 +486,8 @@ static ssize_t goodix_dump_config_show(struct
> device *dev,
>  		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]);
> @@ -548,11 +584,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;
> @@ -573,6 +611,34 @@ 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;
> +
> +	if (!ts->gpiod_int || !ts->gpiod_rst)
> +		return 0;
> +
> +	wait_for_completion(&ts->firmware_loading_complete);
> +
> +	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);
> +
> +	if (!ts->gpiod_int || !ts->gpiod_rst)
> +		return;
> +
> +	goodix_set_power_state(ts, false);
> +	atomic_dec(&ts->open_count);
> +}
> +
>  /**
>   * goodix_get_gpio_config - Get GPIO config from ACPI/DT
>   *
> @@ -754,6 +820,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) {
> @@ -808,7 +877,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)
>  {
> @@ -828,6 +898,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);
>  	complete_all(&ts->firmware_loading_complete);
> @@ -854,6 +937,7 @@ static int goodix_ts_probe(struct i2c_client
> *client,
>  	i2c_set_clientdata(client, ts);
>  	init_completion(&ts->firmware_loading_complete);
>  	INIT_DELAYED_WORK(&ts->esd_work, goodix_esd_work);
> +	mutex_init(&ts->mutex);
>  
>  	error = goodix_get_gpio_config(ts);
>  	if (error)
> @@ -940,23 +1024,33 @@ static int goodix_ts_remove(struct i2c_client
> *client)
>  
>  	wait_for_completion(&ts->firmware_loading_complete);
>  
> +	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);
>  
>  	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;
>  
>  	wait_for_completion(&ts->firmware_loading_complete);
> +
> +	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 */
> @@ -966,7 +1060,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);
> @@ -977,7 +1071,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;
>  	}
>  
>  	/*
> @@ -986,44 +1081,77 @@ 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;
>  
>  	error = goodix_enable_esd(ts);
>  	if (error)
> -		return 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 },

[PATCH v12 4/5] Input: goodix - fix reset sequence

[Irina Tirdea]

According to the Goodix datasheet, the reset sequence will leave
the reset line set to output high. To end the selection of the I2C
address, we just need to set the input line to low for at least
50 ms and then set it to input floating (already implemented by
goodix_int_sync).

Remove setting the reset line to input from the reset sequence,
since that is not reflected in the datasheet.

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).

Suggested-by: Troy Kisky <troy.kisky@boundarydevices.com>
Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 drivers/input/touchscreen/goodix.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 182ff9c..6fba804 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -454,10 +454,6 @@ static int goodix_reset(struct goodix_ts_data *ts)
 	usleep_range(6000, 10000);		/* T4: > 5ms */
 
 	/* end select I2C slave addr */
-	error = gpiod_direction_input(ts->gpiod_rst);
-	if (error)
-		return error;
-
 	error = goodix_int_sync(ts);
 	if (error)
 		return error;
-- 
1.9.1

Re: [PATCH v12 4/5] Input: goodix - fix reset sequence

[Bastien Nocera]

On Sat, 2016-09-10 at 20:57 +0300, Irina Tirdea wrote:
> According to the Goodix datasheet, the reset sequence will leave
> the reset line set to output high. To end the selection of the I2C
> address, we just need to set the input line to low for at least
> 50 ms and then set it to input floating (already implemented by
> goodix_int_sync).
> 
> Remove setting the reset line to input from the reset sequence,
> since that is not reflected in the datasheet.
> 
> 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).
> 
> Suggested-by: Troy Kisky <troy.kisky-Q5RJGjKts06CY9SHAMCTRUEOCMrvLtNR@public.gmane.org>
> Signed-off-by: Irina Tirdea <irina.tirdea-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Code looks fine. Again, would be nice to know on which devices this was
tested.

> ---
>  drivers/input/touchscreen/goodix.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 182ff9c..6fba804 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -454,10 +454,6 @@ static int goodix_reset(struct goodix_ts_data
> *ts)
>  	usleep_range(6000, 10000);		/* T4: > 5ms */
>  
>  	/* end select I2C slave addr */
> -	error = gpiod_direction_input(ts->gpiod_rst);
> -	if (error)
> -		return error;
> -
>  	error = goodix_int_sync(ts);
>  	if (error)
>  		return error;
--
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 v12 4/5] Input: goodix - fix reset sequence

[Bastien Nocera]

On Sat, 2016-09-10 at 20:57 +0300, Irina Tirdea wrote:
> According to the Goodix datasheet, the reset sequence will leave
> the reset line set to output high. To end the selection of the I2C
> address, we just need to set the input line to low for at least
> 50 ms and then set it to input floating (already implemented by
> goodix_int_sync).
> 
> Remove setting the reset line to input from the reset sequence,
> since that is not reflected in the datasheet.
> 
> 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).
> 
> Suggested-by: Troy Kisky <troy.kisky@boundarydevices.com>
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>

Code looks fine. Again, would be nice to know on which devices this was
tested.

> ---
>  drivers/input/touchscreen/goodix.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 182ff9c..6fba804 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -454,10 +454,6 @@ static int goodix_reset(struct goodix_ts_data
> *ts)
>  	usleep_range(6000, 10000);		/* T4: > 5ms */
>  
>  	/* end select I2C slave addr */
> -	error = gpiod_direction_input(ts->gpiod_rst);
> -	if (error)
> -		return error;
> -
>  	error = goodix_int_sync(ts);
>  	if (error)
>  		return error;

[PATCH v12 5/5] Input: goodix - add support for gt9157

[Irina Tirdea]

Goodix touchscreen GT9157 has the same basic functionality
as GT911 touchscreen. This is based on Goodix datasheets
for GT911 and GT9157 and on Goodix driver gt9xx.c for
Android (publicly available in Android kernel trees for
various devices).

Signed-off-by: Irina Tirdea <irina.tirdea-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
 drivers/input/touchscreen/goodix.c                             | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index ef5f42d..421b7d5 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -5,6 +5,7 @@ Required properties:
  - compatible		: Should be "goodix,gt911"
 				 or "goodix,gt9110"
 				 or "goodix,gt912"
+				 or "goodix,gt9157"
 				 or "goodix,gt927"
 				 or "goodix,gt9271"
 				 or "goodix,gt928"
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 6fba804..bd4dd4b 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -192,6 +192,7 @@ static int goodix_get_cfg_len(u16 id)
 	case 911:
 	case 9271:
 	case 9110:
+	case 9157:
 	case 927:
 	case 928:
 		return GOODIX_CONFIG_911_LENGTH;
@@ -1168,6 +1169,7 @@ static const struct of_device_id goodix_of_match[] = {
 	{ .compatible = "goodix,gt911" },
 	{ .compatible = "goodix,gt9110" },
 	{ .compatible = "goodix,gt912" },
+	{ .compatible = "goodix,gt9157" },
 	{ .compatible = "goodix,gt927" },
 	{ .compatible = "goodix,gt9271" },
 	{ .compatible = "goodix,gt928" },
-- 
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 v12 5/5] Input: goodix - add support for gt9157

[Irina Tirdea]

Goodix touchscreen GT9157 has the same basic functionality
as GT911 touchscreen. This is based on Goodix datasheets
for GT911 and GT9157 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>
---
 Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
 drivers/input/touchscreen/goodix.c                             | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index ef5f42d..421b7d5 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -5,6 +5,7 @@ Required properties:
  - compatible		: Should be "goodix,gt911"
 				 or "goodix,gt9110"
 				 or "goodix,gt912"
+				 or "goodix,gt9157"
 				 or "goodix,gt927"
 				 or "goodix,gt9271"
 				 or "goodix,gt928"
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 6fba804..bd4dd4b 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -192,6 +192,7 @@ static int goodix_get_cfg_len(u16 id)
 	case 911:
 	case 9271:
 	case 9110:
+	case 9157:
 	case 927:
 	case 928:
 		return GOODIX_CONFIG_911_LENGTH;
@@ -1168,6 +1169,7 @@ static const struct of_device_id goodix_of_match[] = {
 	{ .compatible = "goodix,gt911" },
 	{ .compatible = "goodix,gt9110" },
 	{ .compatible = "goodix,gt912" },
+	{ .compatible = "goodix,gt9157" },
 	{ .compatible = "goodix,gt927" },
 	{ .compatible = "goodix,gt9271" },
 	{ .compatible = "goodix,gt928" },
-- 
1.9.1

Re: [PATCH v12 5/5] Input: goodix - add support for gt9157

[Rob Herring]

On Sat, Sep 10, 2016 at 08:57:37PM +0300, Irina Tirdea wrote:
> Goodix touchscreen GT9157 has the same basic functionality
> as GT911 touchscreen. This is based on Goodix datasheets
> for GT911 and GT9157 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>
> ---
>  Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
>  drivers/input/touchscreen/goodix.c                             | 2 ++
>  2 files changed, 3 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v12 5/5] Input: goodix - add support for gt9157

[Bastien Nocera]

On Sat, 2016-09-10 at 20:57 +0300, Irina Tirdea wrote:
> Goodix touchscreen GT9157 has the same basic functionality
> as GT911 touchscreen. This is based on Goodix datasheets
> for GT911 and GT9157 and on Goodix driver gt9xx.c for
> Android (publicly available in Android kernel trees for
> various devices).
> 
> Signed-off-by: Irina Tirdea <irina.tirdea-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Looks obviously correct.

Reviewed-by: Bastien Nocera <hadess-0MeiytkfxGOsTnJN9+BGXg@public.gmane.org>

> ---
>  Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
>  drivers/input/touchscreen/goodix.c                             | 2
> ++
>  2 files changed, 3 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> index ef5f42d..421b7d5 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> @@ -5,6 +5,7 @@ Required properties:
>   - compatible		: Should be "goodix,gt911"
>  				 or "goodix,gt9110"
>  				 or "goodix,gt912"
> +				 or "goodix,gt9157"
>  				 or "goodix,gt927"
>  				 or "goodix,gt9271"
>  				 or "goodix,gt928"
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 6fba804..bd4dd4b 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -192,6 +192,7 @@ static int goodix_get_cfg_len(u16 id)
>  	case 911:
>  	case 9271:
>  	case 9110:
> +	case 9157:
>  	case 927:
>  	case 928:
>  		return GOODIX_CONFIG_911_LENGTH;
> @@ -1168,6 +1169,7 @@ static const struct of_device_id
> goodix_of_match[] = {
>  	{ .compatible = "goodix,gt911" },
>  	{ .compatible = "goodix,gt9110" },
>  	{ .compatible = "goodix,gt912" },
> +	{ .compatible = "goodix,gt9157" },
>  	{ .compatible = "goodix,gt927" },
>  	{ .compatible = "goodix,gt9271" },
>  	{ .compatible = "goodix,gt928" },
--
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 v12 5/5] Input: goodix - add support for gt9157

[Bastien Nocera]

On Sat, 2016-09-10 at 20:57 +0300, Irina Tirdea wrote:
> Goodix touchscreen GT9157 has the same basic functionality
> as GT911 touchscreen. This is based on Goodix datasheets
> for GT911 and GT9157 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>

Looks obviously correct.

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

> ---
>  Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
>  drivers/input/touchscreen/goodix.c                             | 2
> ++
>  2 files changed, 3 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> index ef5f42d..421b7d5 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> @@ -5,6 +5,7 @@ Required properties:
>   - compatible		: Should be "goodix,gt911"
>  				 or "goodix,gt9110"
>  				 or "goodix,gt912"
> +				 or "goodix,gt9157"
>  				 or "goodix,gt927"
>  				 or "goodix,gt9271"
>  				 or "goodix,gt928"
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 6fba804..bd4dd4b 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -192,6 +192,7 @@ static int goodix_get_cfg_len(u16 id)
>  	case 911:
>  	case 9271:
>  	case 9110:
> +	case 9157:
>  	case 927:
>  	case 928:
>  		return GOODIX_CONFIG_911_LENGTH;
> @@ -1168,6 +1169,7 @@ static const struct of_device_id
> goodix_of_match[] = {
>  	{ .compatible = "goodix,gt911" },
>  	{ .compatible = "goodix,gt9110" },
>  	{ .compatible = "goodix,gt912" },
> +	{ .compatible = "goodix,gt9157" },
>  	{ .compatible = "goodix,gt927" },
>  	{ .compatible = "goodix,gt9271" },
>  	{ .compatible = "goodix,gt928" },

Re: [PATCH v12 0/5] Goodix touchscreen enhancements

[Bastien Nocera]

On Sat, 2016-09-10 at 20:57 +0300, Irina Tirdea wrote:
> This is an update for a previous set of patches [1] with enhancements
> for the Goodix touchscreens. The first 3 patches are leftovers from
> the previous patchset (with minor changes due to rebase), while the
> last 2 patches are new.
> 
> Thanks,
> Irina
> 
> [1] https://lkml.org/lkml/2015/11/19/241
> 
> Changes in v12:
>  - add documentation for configuration firmware update
>  - minor changes due to rebase
>  - add 2 new patches to fix reset sequence and add support for gt9157
> 
> Previous changes:
>   https://lkml.org/lkml/2015/11/19/241
> 
> Irina Tirdea (5):
>   Input: goodix - add sysfs interface to dump config
>   Input: goodix - add support for ESD
>   Input: goodix - add runtime power management support

I tested those 3 patches in their v11, but my test tablet's software is
currently broken. Until I find the time to regenerate install images,
I'll just do code reviews.

I assume the first 3 patches don't break anything on the device I
tested this on, but I'd like to make sure it was at least tested on
ACPI-based systems.

Can you mention somewhere on which devices those patches were tested?

Cheers

>   Input: goodix - fix reset sequence
>   Input: goodix - add support for gt9157
> 
>  .../bindings/input/touchscreen/goodix.txt          |   5 +
>  Documentation/input/goodix.txt                     |  84 +++++
>  drivers/input/touchscreen/goodix.c                 | 346
> +++++++++++++++++++--
>  3 files changed, 415 insertions(+), 20 deletions(-)
>  create mode 100644 Documentation/input/goodix.txt
> 

[PATCH v12 1/5] Input: goodix - add sysfs interface to dump config

[Dimitry Ishenko]

> On Saturday, September 10, 2016 at 2:00:06 PM UTC-4, Irina Tirdea wrote: 
>
> Goodix devices have a configuration information register area that 
> specifies various parameters for the device. ...
>
> Export a sysfs interface that would allow reading the configuration 
> information. ...
>
> This sysfs interface will be exported only if the gpio pins are properly 
> initialized from ACPI/DT. ...

Irina, why the requirement for INT/RST gpio pins? The config info is 
available by simply reading from the config register (0x8047) and IIUIC
does not require presence of INT/RST pins. So, it can be exported even
without those pins.

Likewise, config writing support currently present in the kernel
(which I believe you are responsible for [thank you!]) also for some
reason mandates presence of INT/RST. However, it is simply done by
writing to the aforementioned config register (0x8047).

I have 2 similar devices (Pipo X8 and X9) with GT911 chips that have
different configs. Neither device has the RST pin defined.

I was able to successfully read the config from one device and
write it to the other. So, it seems the INT/RST requirement is not
necessary for config reading/writing support. Or did I miss something.

Thank you.

Dimitry

NB: I am not subscribed to the list. Please CC me directly with any replies.