[PATCH resend 01/10] Input: goodix - Refactor IRQ pin GPIO accesses

[PATCH resend 01/10] Input: goodix - Refactor IRQ pin GPIO accesses

[Hans de Goede]

Suspending Goodix touchscreens requires changing the interrupt pin to
output before sending them a power-down command. Followed by wiggling
the interrupt pin to wake the device up, after which it is put back
in input mode.

So far we have only effectively supported this on devices which use
devicetree. On X86 ACPI platforms both looking up the pins; and using a
pin as both IRQ and GPIO is a bit more complicated. E.g. on some devices
we cannot directly access the IRQ pin as GPIO and we need to call ACPI
methods to control it instead.

This commit adds a new irq_pin_access_method field to the goodix_chip_data
struct and adds goodix_irq_direction_output and goodix_irq_direction_input
helpers which together abstract the GPIO accesses to the IRQ pin.

This is a preparation patch for adding support for properly suspending the
touchscreen on X86 ACPI platforms.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
Cc: Dmitry Mastykin <mastichi@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/goodix.c | 62 ++++++++++++++++++++++++------
 1 file changed, 51 insertions(+), 11 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 0403102e807e..08806a00a9b9 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -31,6 +31,11 @@
 
 struct goodix_ts_data;
 
+enum goodix_irq_pin_access_method {
+	irq_pin_access_none,
+	irq_pin_access_gpio,
+};
+
 struct goodix_chip_data {
 	u16 config_addr;
 	int config_len;
@@ -53,6 +58,7 @@ struct goodix_ts_data {
 	const char *cfg_name;
 	struct completion firmware_loading_complete;
 	unsigned long irq_flags;
+	enum goodix_irq_pin_access_method irq_pin_access_method;
 	unsigned int contact_size;
 };
 
@@ -502,17 +508,48 @@ static int goodix_send_cfg(struct goodix_ts_data *ts,
 	return 0;
 }
 
+static int goodix_irq_direction_output(struct goodix_ts_data *ts,
+				       int value)
+{
+	switch (ts->irq_pin_access_method) {
+	case irq_pin_access_none:
+		dev_err(&ts->client->dev,
+			"%s called without an irq_pin_access_method set\n",
+			__func__);
+		return -EINVAL;
+	case irq_pin_access_gpio:
+		return gpiod_direction_output(ts->gpiod_int, value);
+	}
+
+	return -EINVAL; /* Never reached */
+}
+
+static int goodix_irq_direction_input(struct goodix_ts_data *ts)
+{
+	switch (ts->irq_pin_access_method) {
+	case irq_pin_access_none:
+		dev_err(&ts->client->dev,
+			"%s called without an irq_pin_access_method set\n",
+			__func__);
+		return -EINVAL;
+	case irq_pin_access_gpio:
+		return gpiod_direction_input(ts->gpiod_int);
+	}
+
+	return -EINVAL; /* Never reached */
+}
+
 static int goodix_int_sync(struct goodix_ts_data *ts)
 {
 	int error;
 
-	error = gpiod_direction_output(ts->gpiod_int, 0);
+	error = goodix_irq_direction_output(ts, 0);
 	if (error)
 		return error;
 
 	msleep(50);				/* T5: 50ms */
 
-	error = gpiod_direction_input(ts->gpiod_int);
+	error = goodix_irq_direction_input(ts);
 	if (error)
 		return error;
 
@@ -536,7 +573,7 @@ static int goodix_reset(struct goodix_ts_data *ts)
 	msleep(20);				/* T2: > 10ms */
 
 	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
-	error = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14);
+	error = goodix_irq_direction_output(ts, ts->client->addr == 0x14);
 	if (error)
 		return error;
 
@@ -617,6 +654,9 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
 
 	ts->gpiod_rst = gpiod;
 
+	if (ts->gpiod_int && ts->gpiod_rst)
+		ts->irq_pin_access_method = irq_pin_access_gpio;
+
 	return 0;
 }
 
@@ -889,7 +929,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 	if (error)
 		return error;
 
-	if (ts->gpiod_int && ts->gpiod_rst) {
+	if (ts->irq_pin_access_method == irq_pin_access_gpio) {
 		/* reset the controller */
 		error = goodix_reset(ts);
 		if (error) {
@@ -912,7 +952,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 
 	ts->chip = goodix_get_chip_data(ts->id);
 
-	if (ts->gpiod_int && ts->gpiod_rst) {
+	if (ts->irq_pin_access_method == irq_pin_access_gpio) {
 		/* update device config */
 		ts->cfg_name = devm_kasprintf(&client->dev, GFP_KERNEL,
 					      "goodix_%d_cfg.bin", ts->id);
@@ -943,7 +983,7 @@ 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)
+	if (ts->irq_pin_access_method == irq_pin_access_gpio)
 		wait_for_completion(&ts->firmware_loading_complete);
 
 	return 0;
@@ -956,7 +996,7 @@ static int __maybe_unused goodix_suspend(struct device *dev)
 	int error;
 
 	/* We need gpio pins to suspend/resume */
-	if (!ts->gpiod_int || !ts->gpiod_rst) {
+	if (ts->irq_pin_access_method == irq_pin_access_none) {
 		disable_irq(client->irq);
 		return 0;
 	}
@@ -967,7 +1007,7 @@ static int __maybe_unused goodix_suspend(struct device *dev)
 	goodix_free_irq(ts);
 
 	/* Output LOW on the INT pin for 5 ms */
-	error = gpiod_direction_output(ts->gpiod_int, 0);
+	error = goodix_irq_direction_output(ts, 0);
 	if (error) {
 		goodix_request_irq(ts);
 		return error;
@@ -979,7 +1019,7 @@ static int __maybe_unused goodix_suspend(struct device *dev)
 				    GOODIX_CMD_SCREEN_OFF);
 	if (error) {
 		dev_err(&ts->client->dev, "Screen off command failed\n");
-		gpiod_direction_input(ts->gpiod_int);
+		goodix_irq_direction_input(ts);
 		goodix_request_irq(ts);
 		return -EAGAIN;
 	}
@@ -999,7 +1039,7 @@ static int __maybe_unused goodix_resume(struct device *dev)
 	struct goodix_ts_data *ts = i2c_get_clientdata(client);
 	int error;
 
-	if (!ts->gpiod_int || !ts->gpiod_rst) {
+	if (ts->irq_pin_access_method == irq_pin_access_none) {
 		enable_irq(client->irq);
 		return 0;
 	}
@@ -1008,7 +1048,7 @@ static int __maybe_unused goodix_resume(struct device *dev)
 	 * Exit sleep mode by outputting HIGH level to INT pin
 	 * for 2ms~5ms.
 	 */
-	error = gpiod_direction_output(ts->gpiod_int, 1);
+	error = goodix_irq_direction_output(ts, 1);
 	if (error)
 		return error;
 
-- 
2.25.0

[PATCH resend 02/10] Input: goodix - Make loading the config from disk independent from the GPIO setup

[Hans de Goede]

At least on X86 ACPI platforms it is not necessary to load the touchscreen
controller config from disk, if it needs to be loaded this has already been
done by the BIOS / UEFI firmware.

Even on other (e.g. devicetree) platforms the config-loading as currently
done has the issue that the loaded cfg file is based on the controller
model, but the actual cfg is device specific, so the cfg files are not
part of linux-firmware and this can only work with a device specific OS
image which includes the cfg file.

And we do not need access to the GPIOs at all to load the config, if we
do not have access we can still load the config.

So all in all tying the decision to try to load the config from disk to
being able to access the GPIOs is not desirable. This commit adds a new
load_cfg_from_disk boolean to control the firmware loading instead.

This commits sets the new bool to true when we set irq_pin_access_method
to irq_pin_access_gpio, so there are no functional changes.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
Cc: Dmitry Mastykin <mastichi@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/goodix.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 08806a00a9b9..eccf07adfae1 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -56,6 +56,7 @@ struct goodix_ts_data {
 	u16 id;
 	u16 version;
 	const char *cfg_name;
+	bool load_cfg_from_disk;
 	struct completion firmware_loading_complete;
 	unsigned long irq_flags;
 	enum goodix_irq_pin_access_method irq_pin_access_method;
@@ -654,8 +655,10 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
 
 	ts->gpiod_rst = gpiod;
 
-	if (ts->gpiod_int && ts->gpiod_rst)
+	if (ts->gpiod_int && ts->gpiod_rst) {
+		ts->load_cfg_from_disk = true;
 		ts->irq_pin_access_method = irq_pin_access_gpio;
+	}
 
 	return 0;
 }
@@ -952,7 +955,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 
 	ts->chip = goodix_get_chip_data(ts->id);
 
-	if (ts->irq_pin_access_method == irq_pin_access_gpio) {
+	if (ts->load_cfg_from_disk) {
 		/* update device config */
 		ts->cfg_name = devm_kasprintf(&client->dev, GFP_KERNEL,
 					      "goodix_%d_cfg.bin", ts->id);
@@ -983,7 +986,7 @@ static int goodix_ts_remove(struct i2c_client *client)
 {
 	struct goodix_ts_data *ts = i2c_get_clientdata(client);
 
-	if (ts->irq_pin_access_method == irq_pin_access_gpio)
+	if (ts->load_cfg_from_disk)
 		wait_for_completion(&ts->firmware_loading_complete);
 
 	return 0;
@@ -1001,7 +1004,8 @@ static int __maybe_unused goodix_suspend(struct device *dev)
 		return 0;
 	}
 
-	wait_for_completion(&ts->firmware_loading_complete);
+	if (ts->load_cfg_from_disk)
+		wait_for_completion(&ts->firmware_loading_complete);
 
 	/* Free IRQ as IRQ pin is used as output in the suspend sequence */
 	goodix_free_irq(ts);
-- 
2.25.0

[PATCH resend 03/10] Input: goodix - Make resetting the controller at probe independent from the GPIO setup

[Hans de Goede]

Before this commit we would always reset the controller at probe when we
have access to the GPIOs which are necessary to do a reset.

Doing the reset requires access to the GPIOs, but just because we have
access to the GPIOs does not mean that we should always reset the
controller at probe. On X86 ACPI platforms the BIOS / UEFI firmware will
already have reset the controller and it will have loaded the device
specific config into the controller. Doing the reset sometimes causes the
controller to loose its configuration, so on X86 ACPI platforms this is not
a good idea.

This commit adds a new reset_controller_at_probe boolean to control the
reset at probe behavior.

This commits sets the new bool to true when we set irq_pin_access_method
to irq_pin_access_gpio, so there are no functional changes.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
Cc: Dmitry Mastykin <mastichi@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/goodix.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index eccf07adfae1..dd5a8f9e8ada 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -56,6 +56,7 @@ struct goodix_ts_data {
 	u16 id;
 	u16 version;
 	const char *cfg_name;
+	bool reset_controller_at_probe;
 	bool load_cfg_from_disk;
 	struct completion firmware_loading_complete;
 	unsigned long irq_flags;
@@ -656,6 +657,7 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
 	ts->gpiod_rst = gpiod;
 
 	if (ts->gpiod_int && ts->gpiod_rst) {
+		ts->reset_controller_at_probe = true;
 		ts->load_cfg_from_disk = true;
 		ts->irq_pin_access_method = irq_pin_access_gpio;
 	}
@@ -932,7 +934,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 	if (error)
 		return error;
 
-	if (ts->irq_pin_access_method == irq_pin_access_gpio) {
+	if (ts->reset_controller_at_probe) {
 		/* reset the controller */
 		error = goodix_reset(ts);
 		if (error) {
-- 
2.25.0

[PATCH resend 04/10] Input: goodix - Add support for getting IRQ + reset GPIOs on Cherry Trail devices

[Hans de Goede]

On most Cherry Trail (x86, UEFI + ACPI) devices the ACPI tables do not have
a _DSD with a "daffd814-6eba-4d8c-8a91-bc9bbf4aa301" UUID, adding
"irq-gpios" and "reset-gpios" mappings, so we cannot get the GPIOS by name
without first manually adding mappings ourselves.

These devices contain 1 GpioInt and 1 GpioIo resource in their _CRS table.
There is no fixed order for these 2. This commit adds code to check that
there is 1 of each as expected and then registers a mapping matching their
order using devm_acpi_dev_add_driver_gpios().

This gives us access to both GPIOs allowing us to properly suspend the
controller during suspend, and making it possible to reset the controller
if necessary.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
Cc: Dmitry Mastykin <mastichi@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/goodix.c | 113 ++++++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index dd5a8f9e8ada..9de2f325ac6e 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -34,6 +34,7 @@ struct goodix_ts_data;
 enum goodix_irq_pin_access_method {
 	irq_pin_access_none,
 	irq_pin_access_gpio,
+	irq_pin_access_acpi_gpio,
 };
 
 struct goodix_chip_data {
@@ -53,6 +54,8 @@ struct goodix_ts_data {
 	struct regulator *vddio;
 	struct gpio_desc *gpiod_int;
 	struct gpio_desc *gpiod_rst;
+	int gpio_count;
+	int gpio_int_idx;
 	u16 id;
 	u16 version;
 	const char *cfg_name;
@@ -521,6 +524,12 @@ static int goodix_irq_direction_output(struct goodix_ts_data *ts,
 		return -EINVAL;
 	case irq_pin_access_gpio:
 		return gpiod_direction_output(ts->gpiod_int, value);
+	case irq_pin_access_acpi_gpio:
+		/*
+		 * The IRQ pin triggers on a falling edge, so its gets marked
+		 * as active-low, use output_raw to avoid the value inversion.
+		 */
+		return gpiod_direction_output_raw(ts->gpiod_int, value);
 	}
 
 	return -EINVAL; /* Never reached */
@@ -535,6 +544,7 @@ static int goodix_irq_direction_input(struct goodix_ts_data *ts)
 			__func__);
 		return -EINVAL;
 	case irq_pin_access_gpio:
+	case irq_pin_access_acpi_gpio:
 		return gpiod_direction_input(ts->gpiod_int);
 	}
 
@@ -599,6 +609,87 @@ static int goodix_reset(struct goodix_ts_data *ts)
 	return 0;
 }
 
+#if defined CONFIG_X86 && defined CONFIG_ACPI
+static const struct acpi_gpio_params first_gpio = { 0, 0, false };
+static const struct acpi_gpio_params second_gpio = { 1, 0, false };
+
+static const struct acpi_gpio_mapping acpi_goodix_int_first_gpios[] = {
+	{ GOODIX_GPIO_INT_NAME "-gpios", &first_gpio, 1 },
+	{ GOODIX_GPIO_RST_NAME "-gpios", &second_gpio, 1 },
+	{ },
+};
+
+static const struct acpi_gpio_mapping acpi_goodix_int_last_gpios[] = {
+	{ GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1 },
+	{ GOODIX_GPIO_INT_NAME "-gpios", &second_gpio, 1 },
+	{ },
+};
+
+static int goodix_resource(struct acpi_resource *ares, void *data)
+{
+	struct goodix_ts_data *ts = data;
+	struct device *dev = &ts->client->dev;
+	struct acpi_resource_gpio *gpio;
+
+	switch (ares->type) {
+	case ACPI_RESOURCE_TYPE_GPIO:
+		gpio = &ares->data.gpio;
+		if (gpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT) {
+			if (ts->gpio_int_idx == -1) {
+				ts->gpio_int_idx = ts->gpio_count;
+			} else {
+				dev_err(dev, "More then one GpioInt resource, ignoring ACPI GPIO resources\n");
+				ts->gpio_int_idx = -2;
+			}
+		}
+		ts->gpio_count++;
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts)
+{
+	const struct acpi_gpio_mapping *gpio_mapping = NULL;
+	struct device *dev = &ts->client->dev;
+	LIST_HEAD(resources);
+	int ret;
+
+	ts->gpio_count = 0;
+	ts->gpio_int_idx = -1;
+	ret = acpi_dev_get_resources(ACPI_COMPANION(dev), &resources,
+				     goodix_resource, ts);
+	if (ret < 0) {
+		dev_err(dev, "Error getting ACPI resources: %d\n", ret);
+		return ret;
+	}
+
+	acpi_dev_free_resource_list(&resources);
+
+	if (ts->gpio_count == 2 && ts->gpio_int_idx == 0) {
+		ts->irq_pin_access_method = irq_pin_access_acpi_gpio;
+		gpio_mapping = acpi_goodix_int_first_gpios;
+	} else if (ts->gpio_count == 2 && ts->gpio_int_idx == 1) {
+		ts->irq_pin_access_method = irq_pin_access_acpi_gpio;
+		gpio_mapping = acpi_goodix_int_last_gpios;
+	} else {
+		dev_warn(dev, "Unexpected ACPI resources: gpio_count %d, gpio_int_idx %d\n",
+			 ts->gpio_count, ts->gpio_int_idx);
+		return -EINVAL;
+	}
+
+	return devm_acpi_dev_add_driver_gpios(dev, gpio_mapping);
+}
+#else
+static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts)
+{
+	return -EINVAL;
+}
+#endif /* CONFIG_X86 && CONFIG_ACPI */
+
 /**
  * goodix_get_gpio_config - Get GPIO config from ACPI/DT
  *
@@ -609,6 +700,7 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
 	int error;
 	struct device *dev;
 	struct gpio_desc *gpiod;
+	bool added_acpi_mappings = false;
 
 	if (!ts->client)
 		return -EINVAL;
@@ -632,6 +724,7 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
 		return error;
 	}
 
+retry_get_irq_gpio:
 	/* Get the interrupt GPIO pin number */
 	gpiod = devm_gpiod_get_optional(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN);
 	if (IS_ERR(gpiod)) {
@@ -641,6 +734,11 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
 				GOODIX_GPIO_INT_NAME, error);
 		return error;
 	}
+	if (!gpiod && has_acpi_companion(dev) && !added_acpi_mappings) {
+		added_acpi_mappings = true;
+		if (goodix_add_acpi_gpio_mappings(ts) == 0)
+			goto retry_get_irq_gpio;
+	}
 
 	ts->gpiod_int = gpiod;
 
@@ -656,10 +754,17 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
 
 	ts->gpiod_rst = gpiod;
 
-	if (ts->gpiod_int && ts->gpiod_rst) {
-		ts->reset_controller_at_probe = true;
-		ts->load_cfg_from_disk = true;
-		ts->irq_pin_access_method = irq_pin_access_gpio;
+	switch (ts->irq_pin_access_method) {
+	case irq_pin_access_acpi_gpio:
+		if (!ts->gpiod_int || !ts->gpiod_rst)
+			ts->irq_pin_access_method = irq_pin_access_none;
+		break;
+	default:
+		if (ts->gpiod_int && ts->gpiod_rst) {
+			ts->reset_controller_at_probe = true;
+			ts->load_cfg_from_disk = true;
+			ts->irq_pin_access_method = irq_pin_access_gpio;
+		}
 	}
 
 	return 0;
-- 
2.25.0

[PATCH resend 05/10] Input: goodix - Add support for getting IRQ + reset GPIOs on Bay Trail devices

[Hans de Goede]

On most Bay Trail (x86, UEFI + ACPI) devices the ACPI tables do not have
a _DSD with a "daffd814-6eba-4d8c-8a91-bc9bbf4aa301" UUID, adding
"irq-gpios" and "reset-gpios" mappings, so we cannot get the GPIOS by name
without first manually adding mappings ourselves.

These devices contain 2 GpioIo resource in their _CRS table, on all 4 such
devices which I have access to, the order of the 2 GPIOs is reset, int.

Note that the GPIO to which the touchscreen controller irq pin is connected
is configured in direct-irq mode on these Bay Trail devices, the
pinctrl-baytrail.c driver still allows controlling the pin as a GPIO in
this case, but this is not necessarily the case on other X86 ACPI
platforms, nor do we have a guarantee that the GPIO order is the same
elsewhere, so we limit the use of a _CRS table with 2 GpioIo resources
to Bay Trail devices only.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
Cc: Dmitry Mastykin <mastichi@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/goodix.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 9de2f325ac6e..d178aa33b529 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -610,6 +610,21 @@ static int goodix_reset(struct goodix_ts_data *ts)
 }
 
 #if defined CONFIG_X86 && defined CONFIG_ACPI
+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>
+
+static const struct x86_cpu_id baytrail_cpu_ids[] = {
+	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_SILVERMONT, X86_FEATURE_ANY, },
+	{}
+};
+
+static inline bool is_byt(void)
+{
+	const struct x86_cpu_id *id = x86_match_cpu(baytrail_cpu_ids);
+
+	return !!id;
+}
+
 static const struct acpi_gpio_params first_gpio = { 0, 0, false };
 static const struct acpi_gpio_params second_gpio = { 1, 0, false };
 
@@ -675,6 +690,10 @@ static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts)
 	} else if (ts->gpio_count == 2 && ts->gpio_int_idx == 1) {
 		ts->irq_pin_access_method = irq_pin_access_acpi_gpio;
 		gpio_mapping = acpi_goodix_int_last_gpios;
+	} else if (is_byt() && ts->gpio_count == 2 && ts->gpio_int_idx == -1) {
+		dev_info(dev, "No ACPI GpioInt resource, assuming that the GPIO order is reset, int\n");
+		ts->irq_pin_access_method = irq_pin_access_acpi_gpio;
+		gpio_mapping = acpi_goodix_int_last_gpios;
 	} else {
 		dev_warn(dev, "Unexpected ACPI resources: gpio_count %d, gpio_int_idx %d\n",
 			 ts->gpio_count, ts->gpio_int_idx);
-- 
2.25.0

[PATCH resend 06/10] Input: goodix - Add support for controlling the IRQ pin through ACPI methods

[Hans de Goede]

Some Apollo Lake (x86, UEFI + ACPI) devices only list the reset GPIO
in their _CRS table and the bit-banging of the IRQ line necessary to
wake-up the controller from suspend can be done by calling 2 Goodix
custom / specific ACPI methods.

This commit adds support for controlling the IRQ line in this matter,
allowing us to properly suspend the touchscreen controller on such
devices.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
Cc: Dmitry Mastykin <mastichi@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/goodix.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index d178aa33b529..784c4dd8c430 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -35,6 +35,7 @@ enum goodix_irq_pin_access_method {
 	irq_pin_access_none,
 	irq_pin_access_gpio,
 	irq_pin_access_acpi_gpio,
+	irq_pin_access_acpi_method,
 };
 
 struct goodix_chip_data {
@@ -516,6 +517,9 @@ static int goodix_send_cfg(struct goodix_ts_data *ts,
 static int goodix_irq_direction_output(struct goodix_ts_data *ts,
 				       int value)
 {
+	struct device *dev = &ts->client->dev;
+	acpi_status status;
+
 	switch (ts->irq_pin_access_method) {
 	case irq_pin_access_none:
 		dev_err(&ts->client->dev,
@@ -530,6 +534,10 @@ static int goodix_irq_direction_output(struct goodix_ts_data *ts,
 		 * as active-low, use output_raw to avoid the value inversion.
 		 */
 		return gpiod_direction_output_raw(ts->gpiod_int, value);
+	case irq_pin_access_acpi_method:
+		status = acpi_execute_simple_method(ACPI_HANDLE(dev),
+						    "INTO", value);
+		return ACPI_SUCCESS(status) ? 0 : -EIO;
 	}
 
 	return -EINVAL; /* Never reached */
@@ -537,6 +545,9 @@ static int goodix_irq_direction_output(struct goodix_ts_data *ts,
 
 static int goodix_irq_direction_input(struct goodix_ts_data *ts)
 {
+	struct device *dev = &ts->client->dev;
+	acpi_status status;
+
 	switch (ts->irq_pin_access_method) {
 	case irq_pin_access_none:
 		dev_err(&ts->client->dev,
@@ -546,6 +557,10 @@ static int goodix_irq_direction_input(struct goodix_ts_data *ts)
 	case irq_pin_access_gpio:
 	case irq_pin_access_acpi_gpio:
 		return gpiod_direction_input(ts->gpiod_int);
+	case irq_pin_access_acpi_method:
+		status = acpi_evaluate_object(ACPI_HANDLE(dev), "INTI",
+					      NULL, NULL);
+		return ACPI_SUCCESS(status) ? 0 : -EIO;
 	}
 
 	return -EINVAL; /* Never reached */
@@ -640,6 +655,11 @@ static const struct acpi_gpio_mapping acpi_goodix_int_last_gpios[] = {
 	{ },
 };
 
+static const struct acpi_gpio_mapping acpi_goodix_reset_only_gpios[] = {
+	{ GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1 },
+	{ },
+};
+
 static int goodix_resource(struct acpi_resource *ares, void *data)
 {
 	struct goodix_ts_data *ts = data;
@@ -690,6 +710,12 @@ static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts)
 	} else if (ts->gpio_count == 2 && ts->gpio_int_idx == 1) {
 		ts->irq_pin_access_method = irq_pin_access_acpi_gpio;
 		gpio_mapping = acpi_goodix_int_last_gpios;
+	} else if (ts->gpio_count == 1 && ts->gpio_int_idx == -1 &&
+		   acpi_has_method(ACPI_HANDLE(dev), "INTI") &&
+		   acpi_has_method(ACPI_HANDLE(dev), "INTO")) {
+		dev_info(dev, "Using ACPI INTI and INTO methods for IRQ pin access\n");
+		ts->irq_pin_access_method = irq_pin_access_acpi_method;
+		gpio_mapping = acpi_goodix_reset_only_gpios;
 	} else if (is_byt() && ts->gpio_count == 2 && ts->gpio_int_idx == -1) {
 		dev_info(dev, "No ACPI GpioInt resource, assuming that the GPIO order is reset, int\n");
 		ts->irq_pin_access_method = irq_pin_access_acpi_gpio;
@@ -778,6 +804,10 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
 		if (!ts->gpiod_int || !ts->gpiod_rst)
 			ts->irq_pin_access_method = irq_pin_access_none;
 		break;
+	case irq_pin_access_acpi_method:
+		if (!ts->gpiod_rst)
+			ts->irq_pin_access_method = irq_pin_access_none;
+		break;
 	default:
 		if (ts->gpiod_int && ts->gpiod_rst) {
 			ts->reset_controller_at_probe = true;
-- 
2.25.0

[PATCH resend 07/10] Input: goodix - Move defines to above struct goodix_ts_data declaration

[Hans de Goede]

Move the  defines to above the struct goodix_ts_data declaration, so
that the MAX defines can be used inside the struct goodix_ts_data
declaration. No functional changes, just moving a block of code.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
Cc: Dmitry Mastykin <mastichi@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/goodix.c | 60 +++++++++++++++---------------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 784c4dd8c430..66d6bb74507d 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -29,6 +29,36 @@
 #include <linux/of.h>
 #include <asm/unaligned.h>
 
+#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
+#define GOODIX_CONTACT_SIZE		8
+#define GOODIX_MAX_CONTACT_SIZE		9
+#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_REG_COMMAND		0x8040
+#define GOODIX_CMD_SCREEN_OFF		0x05
+
+#define GOODIX_READ_COOR_ADDR		0x814E
+#define GOODIX_GT1X_REG_CONFIG_DATA	0x8050
+#define GOODIX_GT9X_REG_CONFIG_DATA	0x8047
+#define GOODIX_REG_ID			0x8140
+
+#define GOODIX_BUFFER_STATUS_READY	BIT(7)
+#define GOODIX_BUFFER_STATUS_TIMEOUT	20
+
+#define RESOLUTION_LOC		1
+#define MAX_CONTACTS_LOC	5
+#define TRIGGER_LOC		6
+
 struct goodix_ts_data;
 
 enum goodix_irq_pin_access_method {
@@ -68,36 +98,6 @@ struct goodix_ts_data {
 	unsigned int contact_size;
 };
 
-#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
-#define GOODIX_CONTACT_SIZE		8
-#define GOODIX_MAX_CONTACT_SIZE		9
-#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_REG_COMMAND		0x8040
-#define GOODIX_CMD_SCREEN_OFF		0x05
-
-#define GOODIX_READ_COOR_ADDR		0x814E
-#define GOODIX_GT1X_REG_CONFIG_DATA	0x8050
-#define GOODIX_GT9X_REG_CONFIG_DATA	0x8047
-#define GOODIX_REG_ID			0x8140
-
-#define GOODIX_BUFFER_STATUS_READY	BIT(7)
-#define GOODIX_BUFFER_STATUS_TIMEOUT	20
-
-#define RESOLUTION_LOC		1
-#define MAX_CONTACTS_LOC	5
-#define TRIGGER_LOC		6
-
 static int goodix_check_cfg_8(struct goodix_ts_data *ts,
 			const struct firmware *cfg);
 static int goodix_check_cfg_16(struct goodix_ts_data *ts,
-- 
2.25.0

[PATCH resend 08/10] Input: goodix - Save a copy of the config from goodix_read_config()

[Hans de Goede]

Save a copy of the config in goodix_read_config(), this is a preparation
patch for restoring the config if it was lost after a supend/resume cycle.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
Cc: Dmitry Mastykin <mastichi@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/goodix.c | 51 ++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 66d6bb74507d..21be33384d14 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -72,6 +72,7 @@ struct goodix_chip_data {
 	u16 config_addr;
 	int config_len;
 	int (*check_config)(struct goodix_ts_data *, const struct firmware *);
+	void (*fix_config)(struct goodix_ts_data *ts);
 };
 
 struct goodix_ts_data {
@@ -96,35 +97,42 @@ struct goodix_ts_data {
 	unsigned long irq_flags;
 	enum goodix_irq_pin_access_method irq_pin_access_method;
 	unsigned int contact_size;
+	u8 config[GOODIX_CONFIG_MAX_LENGTH];
 };
 
 static int goodix_check_cfg_8(struct goodix_ts_data *ts,
 			const struct firmware *cfg);
 static int goodix_check_cfg_16(struct goodix_ts_data *ts,
 			const struct firmware *cfg);
+static void goodix_fix_cfg_8(struct goodix_ts_data *ts);
+static void goodix_fix_cfg_16(struct goodix_ts_data *ts);
 
 static const struct goodix_chip_data gt1x_chip_data = {
 	.config_addr		= GOODIX_GT1X_REG_CONFIG_DATA,
 	.config_len		= GOODIX_CONFIG_MAX_LENGTH,
 	.check_config		= goodix_check_cfg_16,
+	.fix_config		= goodix_fix_cfg_16,
 };
 
 static const struct goodix_chip_data gt911_chip_data = {
 	.config_addr		= GOODIX_GT9X_REG_CONFIG_DATA,
 	.config_len		= GOODIX_CONFIG_911_LENGTH,
 	.check_config		= goodix_check_cfg_8,
+	.fix_config		= goodix_fix_cfg_8,
 };
 
 static const struct goodix_chip_data gt967_chip_data = {
 	.config_addr		= GOODIX_GT9X_REG_CONFIG_DATA,
 	.config_len		= GOODIX_CONFIG_967_LENGTH,
 	.check_config		= goodix_check_cfg_8,
+	.fix_config		= goodix_fix_cfg_8,
 };
 
 static const struct goodix_chip_data gt9x_chip_data = {
 	.config_addr		= GOODIX_GT9X_REG_CONFIG_DATA,
 	.config_len		= GOODIX_CONFIG_MAX_LENGTH,
 	.check_config		= goodix_check_cfg_8,
+	.fix_config		= goodix_fix_cfg_8,
 };
 
 static const unsigned long goodix_irq_flags[] = {
@@ -442,6 +450,19 @@ static int goodix_check_cfg_8(struct goodix_ts_data *ts,
 	return 0;
 }
 
+static void goodix_fix_cfg_8(struct goodix_ts_data *ts)
+{
+	int i, raw_cfg_len = ts->chip->config_len - 2;
+	u8 check_sum = 0;
+
+	for (i = 0; i < raw_cfg_len; i++)
+		check_sum += ts->config[i];
+	check_sum = (~check_sum) + 1;
+
+	ts->config[raw_cfg_len] = check_sum;
+	ts->config[raw_cfg_len + 1] = 1;
+}
+
 static int goodix_check_cfg_16(struct goodix_ts_data *ts,
 			const struct firmware *cfg)
 {
@@ -466,6 +487,19 @@ static int goodix_check_cfg_16(struct goodix_ts_data *ts,
 	return 0;
 }
 
+static void goodix_fix_cfg_16(struct goodix_ts_data *ts)
+{
+	int i, raw_cfg_len = ts->chip->config_len - 3;
+	u16 check_sum = 0;
+
+	for (i = 0; i < raw_cfg_len; i += 2)
+		check_sum += get_unaligned_be16(&ts->config[i]);
+	check_sum = (~check_sum) + 1;
+
+	put_unaligned_be16(check_sum, &ts->config[raw_cfg_len]);
+	ts->config[raw_cfg_len + 2] = 1;
+}
+
 /**
  * goodix_check_cfg - Checks if config fw is valid
  *
@@ -828,12 +862,11 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
  */
 static void goodix_read_config(struct goodix_ts_data *ts)
 {
-	u8 config[GOODIX_CONFIG_MAX_LENGTH];
 	int x_max, y_max;
 	int error;
 
 	error = goodix_i2c_read(ts->client, ts->chip->config_addr,
-				config, ts->chip->config_len);
+				ts->config, ts->chip->config_len);
 	if (error) {
 		dev_warn(&ts->client->dev, "Error reading config: %d\n",
 			 error);
@@ -842,15 +875,21 @@ static void goodix_read_config(struct goodix_ts_data *ts)
 		return;
 	}
 
-	ts->int_trigger_type = config[TRIGGER_LOC] & 0x03;
-	ts->max_touch_num = config[MAX_CONTACTS_LOC] & 0x0f;
+	ts->int_trigger_type = ts->config[TRIGGER_LOC] & 0x03;
+	ts->max_touch_num = ts->config[MAX_CONTACTS_LOC] & 0x0f;
 
-	x_max = get_unaligned_le16(&config[RESOLUTION_LOC]);
-	y_max = get_unaligned_le16(&config[RESOLUTION_LOC + 2]);
+	x_max = get_unaligned_le16(&ts->config[RESOLUTION_LOC]);
+	y_max = get_unaligned_le16(&ts->config[RESOLUTION_LOC + 2]);
 	if (x_max && y_max) {
 		input_abs_set_max(ts->input_dev, ABS_MT_POSITION_X, x_max - 1);
 		input_abs_set_max(ts->input_dev, ABS_MT_POSITION_Y, y_max - 1);
 	}
+
+	/*
+	 * Ensure valid checksum and config_fresh bit being set for possible
+	 * re-upload of config after suspend/resume.
+	 */
+	ts->chip->fix_config(ts);
 }
 
 /**
-- 
2.25.0

[PATCH resend 09/10] Input: goodix - Make goodix_send_cfg() take a raw buffer as argument

[Hans de Goede]

Make goodix_send_cfg() take a raw buffer as argument instead of a
struct firmware *cfg, so that it can also be used to restore the config
on resume if necessary.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
Cc: Dmitry Mastykin <mastichi@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/goodix.c | 46 ++++++++++++++----------------
 1 file changed, 21 insertions(+), 25 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 21be33384d14..0f39c499e3a9 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -71,7 +71,7 @@ enum goodix_irq_pin_access_method {
 struct goodix_chip_data {
 	u16 config_addr;
 	int config_len;
-	int (*check_config)(struct goodix_ts_data *, const struct firmware *);
+	int (*check_config)(struct goodix_ts_data *ts, const u8 *cfg, int len);
 	void (*fix_config)(struct goodix_ts_data *ts);
 };
 
@@ -101,9 +101,9 @@ struct goodix_ts_data {
 };
 
 static int goodix_check_cfg_8(struct goodix_ts_data *ts,
-			const struct firmware *cfg);
+			      const u8 *cfg, int len);
 static int goodix_check_cfg_16(struct goodix_ts_data *ts,
-			const struct firmware *cfg);
+			       const u8 *cfg, int len);
 static void goodix_fix_cfg_8(struct goodix_ts_data *ts);
 static void goodix_fix_cfg_16(struct goodix_ts_data *ts);
 
@@ -426,22 +426,21 @@ static int goodix_request_irq(struct goodix_ts_data *ts)
 					 ts->irq_flags, ts->client->name, ts);
 }
 
-static int goodix_check_cfg_8(struct goodix_ts_data *ts,
-			const struct firmware *cfg)
+static int goodix_check_cfg_8(struct goodix_ts_data *ts, const u8 *cfg, int len)
 {
-	int i, raw_cfg_len = cfg->size - 2;
+	int i, raw_cfg_len = len - 2;
 	u8 check_sum = 0;
 
 	for (i = 0; i < raw_cfg_len; i++)
-		check_sum += cfg->data[i];
+		check_sum += cfg[i];
 	check_sum = (~check_sum) + 1;
-	if (check_sum != cfg->data[raw_cfg_len]) {
+	if (check_sum != cfg[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) {
+	if (cfg[raw_cfg_len + 1] != 1) {
 		dev_err(&ts->client->dev,
 			"Config fw must have Config_Fresh register set");
 		return -EINVAL;
@@ -463,22 +462,22 @@ static void goodix_fix_cfg_8(struct goodix_ts_data *ts)
 	ts->config[raw_cfg_len + 1] = 1;
 }
 
-static int goodix_check_cfg_16(struct goodix_ts_data *ts,
-			const struct firmware *cfg)
+static int goodix_check_cfg_16(struct goodix_ts_data *ts, const u8 *cfg,
+			       int len)
 {
-	int i, raw_cfg_len = cfg->size - 3;
+	int i, raw_cfg_len = len - 3;
 	u16 check_sum = 0;
 
 	for (i = 0; i < raw_cfg_len; i += 2)
-		check_sum += get_unaligned_be16(&cfg->data[i]);
+		check_sum += get_unaligned_be16(&cfg[i]);
 	check_sum = (~check_sum) + 1;
-	if (check_sum != get_unaligned_be16(&cfg->data[raw_cfg_len])) {
+	if (check_sum != get_unaligned_be16(&cfg[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 + 2] != 1) {
+	if (cfg[raw_cfg_len + 2] != 1) {
 		dev_err(&ts->client->dev,
 			"Config fw must have Config_Fresh register set");
 		return -EINVAL;
@@ -506,16 +505,15 @@ static void goodix_fix_cfg_16(struct goodix_ts_data *ts)
  * @ts: goodix_ts_data pointer
  * @cfg: firmware config data
  */
-static int goodix_check_cfg(struct goodix_ts_data *ts,
-			    const struct firmware *cfg)
+static int goodix_check_cfg(struct goodix_ts_data *ts, const u8 *cfg, int len)
 {
-	if (cfg->size > GOODIX_CONFIG_MAX_LENGTH) {
+	if (len > GOODIX_CONFIG_MAX_LENGTH) {
 		dev_err(&ts->client->dev,
 			"The length of the config fw is not correct");
 		return -EINVAL;
 	}
 
-	return ts->chip->check_config(ts, cfg);
+	return ts->chip->check_config(ts, cfg, len);
 }
 
 /**
@@ -524,17 +522,15 @@ static int goodix_check_cfg(struct goodix_ts_data *ts,
  * @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)
+static int goodix_send_cfg(struct goodix_ts_data *ts, const u8 *cfg, int len)
 {
 	int error;
 
-	error = goodix_check_cfg(ts, cfg);
+	error = goodix_check_cfg(ts, cfg, len);
 	if (error)
 		return error;
 
-	error = goodix_i2c_write(ts->client, ts->chip->config_addr, cfg->data,
-				 cfg->size);
+	error = goodix_i2c_write(ts->client, ts->chip->config_addr, cfg, len);
 	if (error) {
 		dev_err(&ts->client->dev, "Failed to write config data: %d",
 			error);
@@ -1058,7 +1054,7 @@ static void goodix_config_cb(const struct firmware *cfg, void *ctx)
 
 	if (cfg) {
 		/* send device configuration to the firmware */
-		error = goodix_send_cfg(ts, cfg);
+		error = goodix_send_cfg(ts, cfg->data, cfg->size);
 		if (error)
 			goto err_release_cfg;
 	}
-- 
2.25.0

[PATCH resend 10/10] Input: goodix - Restore config on resume if necessary

[Hans de Goede]

Some devices, e.g the Trekstor Primetab S11B, loose there config over
a suspend/resume cycle (likely the controller looses power during suspend).

This commit reads back the config version on resume and if matches the
expected config version it resets the controller and resends the config
we read back and saved at probe time.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
Cc: Dmitry Mastykin <mastichi@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/goodix.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 0f39c499e3a9..389d3e044f97 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -1232,6 +1232,7 @@ 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);
+	u8 config_ver;
 	int error;
 
 	if (ts->irq_pin_access_method == irq_pin_access_none) {
@@ -1253,6 +1254,27 @@ static int __maybe_unused goodix_resume(struct device *dev)
 	if (error)
 		return error;
 
+	error = goodix_i2c_read(ts->client, ts->chip->config_addr,
+				&config_ver, 1);
+	if (error)
+		dev_warn(dev, "Error reading config version: %d, resetting controller\n",
+			 error);
+	else if (config_ver != ts->config[0])
+		dev_warn(dev, "Config version mismatch %d != %d, resetting controller\n",
+			 config_ver, ts->config[0]);
+
+	if (error != 0 || config_ver != ts->config[0]) {
+		error = goodix_reset(ts);
+		if (error) {
+			dev_err(dev, "Controller reset failed.\n");
+			return error;
+		}
+
+		error = goodix_send_cfg(ts, ts->config, ts->chip->config_len);
+		if (error)
+			return error;
+	}
+
 	error = goodix_request_irq(ts);
 	if (error)
 		return error;
-- 
2.25.0

Re: [PATCH resend 01/10] Input: goodix - Refactor IRQ pin GPIO accesses

[Bastien Nocera]

On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
> Suspending Goodix touchscreens requires changing the interrupt pin to
> output before sending them a power-down command. Followed by wiggling
> the interrupt pin to wake the device up, after which it is put back
> in input mode.
> 
> So far we have only effectively supported this on devices which use
> devicetree. On X86 ACPI platforms both looking up the pins; and using
> a
> pin as both IRQ and GPIO is a bit more complicated. E.g. on some
> devices
> we cannot directly access the IRQ pin as GPIO and we need to call
> ACPI
> methods to control it instead.
> 
> This commit adds a new irq_pin_access_method field to the
> goodix_chip_data
> struct and adds goodix_irq_direction_output and
> goodix_irq_direction_input
> helpers which together abstract the GPIO accesses to the IRQ pin.
> 
> This is a preparation patch for adding support for properly
> suspending the
> touchscreen on X86 ACPI platforms.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
> Cc: Dmitry Mastykin <mastichi@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/input/touchscreen/goodix.c | 62 ++++++++++++++++++++++++--
> ----
>  1 file changed, 51 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 0403102e807e..08806a00a9b9 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -31,6 +31,11 @@
>  
>  struct goodix_ts_data;
>  
> +enum goodix_irq_pin_access_method {
> +	irq_pin_access_none,
> +	irq_pin_access_gpio,

I don't know if that matches the kernel coding style, but I'd rather
the enum member names were upper-case, and typedef'ed.

> +};
> +
>  struct goodix_chip_data {
>  	u16 config_addr;
>  	int config_len;
> @@ -53,6 +58,7 @@ struct goodix_ts_data {
>  	const char *cfg_name;
>  	struct completion firmware_loading_complete;
>  	unsigned long irq_flags;
> +	enum goodix_irq_pin_access_method irq_pin_access_method;
>  	unsigned int contact_size;
>  };
>  
> @@ -502,17 +508,48 @@ static int goodix_send_cfg(struct
> goodix_ts_data *ts,
>  	return 0;
>  }
>  
> +static int goodix_irq_direction_output(struct goodix_ts_data *ts,
> +				       int value)
> +{
> +	switch (ts->irq_pin_access_method) {
> +	case irq_pin_access_none:
> +		dev_err(&ts->client->dev,
> +			"%s called without an irq_pin_access_method
> set\n",
> +			__func__);
> +		return -EINVAL;
> +	case irq_pin_access_gpio:
> +		return gpiod_direction_output(ts->gpiod_int, value);

Is that going to complain about default not being handled? If so, an if
conditional might be enough.

> +	}
> +
> +	return -EINVAL; /* Never reached */
> +}
> +
> +static int goodix_irq_direction_input(struct goodix_ts_data *ts)
> +{
> +	switch (ts->irq_pin_access_method) {
> +	case irq_pin_access_none:
> +		dev_err(&ts->client->dev,
> +			"%s called without an irq_pin_access_method
> set\n",
> +			__func__);
> +		return -EINVAL;
> +	case irq_pin_access_gpio:
> +		return gpiod_direction_input(ts->gpiod_int);

Ditto.

Looks fine otherwise.


> +	}
> +
> +	return -EINVAL; /* Never reached */
> +}
> +
>  static int goodix_int_sync(struct goodix_ts_data *ts)
>  {
>  	int error;
>  
> -	error = gpiod_direction_output(ts->gpiod_int, 0);
> +	error = goodix_irq_direction_output(ts, 0);
>  	if (error)
>  		return error;
>  
>  	msleep(50);				/* T5: 50ms */
>  
> -	error = gpiod_direction_input(ts->gpiod_int);
> +	error = goodix_irq_direction_input(ts);
>  	if (error)
>  		return error;
>  
> @@ -536,7 +573,7 @@ static int goodix_reset(struct goodix_ts_data
> *ts)
>  	msleep(20);				/* T2: > 10ms */
>  
>  	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
> -	error = gpiod_direction_output(ts->gpiod_int, ts->client->addr
> == 0x14);
> +	error = goodix_irq_direction_output(ts, ts->client->addr ==
> 0x14);
>  	if (error)
>  		return error;
>  
> @@ -617,6 +654,9 @@ static int goodix_get_gpio_config(struct
> goodix_ts_data *ts)
>  
>  	ts->gpiod_rst = gpiod;
>  
> +	if (ts->gpiod_int && ts->gpiod_rst)
> +		ts->irq_pin_access_method = irq_pin_access_gpio;
> +
>  	return 0;
>  }
>  
> @@ -889,7 +929,7 @@ static int goodix_ts_probe(struct i2c_client
> *client,
>  	if (error)
>  		return error;
>  
> -	if (ts->gpiod_int && ts->gpiod_rst) {
> +	if (ts->irq_pin_access_method == irq_pin_access_gpio) {
>  		/* reset the controller */
>  		error = goodix_reset(ts);
>  		if (error) {
> @@ -912,7 +952,7 @@ static int goodix_ts_probe(struct i2c_client
> *client,
>  
>  	ts->chip = goodix_get_chip_data(ts->id);
>  
> -	if (ts->gpiod_int && ts->gpiod_rst) {
> +	if (ts->irq_pin_access_method == irq_pin_access_gpio) {
>  		/* update device config */
>  		ts->cfg_name = devm_kasprintf(&client->dev, GFP_KERNEL,
>  					      "goodix_%d_cfg.bin", ts-
> >id);
> @@ -943,7 +983,7 @@ 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)
> +	if (ts->irq_pin_access_method == irq_pin_access_gpio)
>  		wait_for_completion(&ts->firmware_loading_complete);
>  
>  	return 0;
> @@ -956,7 +996,7 @@ static int __maybe_unused goodix_suspend(struct
> device *dev)
>  	int error;
>  
>  	/* We need gpio pins to suspend/resume */
> -	if (!ts->gpiod_int || !ts->gpiod_rst) {
> +	if (ts->irq_pin_access_method == irq_pin_access_none) {
>  		disable_irq(client->irq);
>  		return 0;
>  	}
> @@ -967,7 +1007,7 @@ static int __maybe_unused goodix_suspend(struct
> device *dev)
>  	goodix_free_irq(ts);
>  
>  	/* Output LOW on the INT pin for 5 ms */
> -	error = gpiod_direction_output(ts->gpiod_int, 0);
> +	error = goodix_irq_direction_output(ts, 0);
>  	if (error) {
>  		goodix_request_irq(ts);
>  		return error;
> @@ -979,7 +1019,7 @@ static int __maybe_unused goodix_suspend(struct
> device *dev)
>  				    GOODIX_CMD_SCREEN_OFF);
>  	if (error) {
>  		dev_err(&ts->client->dev, "Screen off command
> failed\n");
> -		gpiod_direction_input(ts->gpiod_int);
> +		goodix_irq_direction_input(ts);
>  		goodix_request_irq(ts);
>  		return -EAGAIN;
>  	}
> @@ -999,7 +1039,7 @@ static int __maybe_unused goodix_resume(struct
> device *dev)
>  	struct goodix_ts_data *ts = i2c_get_clientdata(client);
>  	int error;
>  
> -	if (!ts->gpiod_int || !ts->gpiod_rst) {
> +	if (ts->irq_pin_access_method == irq_pin_access_none) {
>  		enable_irq(client->irq);
>  		return 0;
>  	}
> @@ -1008,7 +1048,7 @@ static int __maybe_unused goodix_resume(struct
> device *dev)
>  	 * Exit sleep mode by outputting HIGH level to INT pin
>  	 * for 2ms~5ms.
>  	 */
> -	error = gpiod_direction_output(ts->gpiod_int, 1);
> +	error = goodix_irq_direction_output(ts, 1);
>  	if (error)
>  		return error;
>  

Re: [PATCH resend 02/10] Input: goodix - Make loading the config from disk independent from the GPIO setup

[Bastien Nocera]

On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
> At least on X86 ACPI platforms it is not necessary to load the
> touchscreen
> controller config from disk, if it needs to be loaded this has
> already been
> done by the BIOS / UEFI firmware.
> 
> Even on other (e.g. devicetree) platforms the config-loading as
> currently
> done has the issue that the loaded cfg file is based on the
> controller
> model, but the actual cfg is device specific, so the cfg files are
> not
> part of linux-firmware and this can only work with a device specific
> OS
> image which includes the cfg file.
> 
> And we do not need access to the GPIOs at all to load the config, if
> we
> do not have access we can still load the config.
> 
> So all in all tying the decision to try to load the config from disk
> to
> being able to access the GPIOs is not desirable. This commit adds a
> new
> load_cfg_from_disk boolean to control the firmware loading instead.
> 
> This commits sets the new bool to true when we set
> irq_pin_access_method
> to irq_pin_access_gpio, so there are no functional changes.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
> Cc: Dmitry Mastykin <mastichi@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Looks good.

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

> ---
>  drivers/input/touchscreen/goodix.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 08806a00a9b9..eccf07adfae1 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -56,6 +56,7 @@ struct goodix_ts_data {
>  	u16 id;
>  	u16 version;
>  	const char *cfg_name;
> +	bool load_cfg_from_disk;
>  	struct completion firmware_loading_complete;
>  	unsigned long irq_flags;
>  	enum goodix_irq_pin_access_method irq_pin_access_method;
> @@ -654,8 +655,10 @@ static int goodix_get_gpio_config(struct
> goodix_ts_data *ts)
>  
>  	ts->gpiod_rst = gpiod;
>  
> -	if (ts->gpiod_int && ts->gpiod_rst)
> +	if (ts->gpiod_int && ts->gpiod_rst) {
> +		ts->load_cfg_from_disk = true;
>  		ts->irq_pin_access_method = irq_pin_access_gpio;
> +	}
>  
>  	return 0;
>  }
> @@ -952,7 +955,7 @@ static int goodix_ts_probe(struct i2c_client
> *client,
>  
>  	ts->chip = goodix_get_chip_data(ts->id);
>  
> -	if (ts->irq_pin_access_method == irq_pin_access_gpio) {
> +	if (ts->load_cfg_from_disk) {
>  		/* update device config */
>  		ts->cfg_name = devm_kasprintf(&client->dev, GFP_KERNEL,
>  					      "goodix_%d_cfg.bin", ts-
> >id);
> @@ -983,7 +986,7 @@ static int goodix_ts_remove(struct i2c_client
> *client)
>  {
>  	struct goodix_ts_data *ts = i2c_get_clientdata(client);
>  
> -	if (ts->irq_pin_access_method == irq_pin_access_gpio)
> +	if (ts->load_cfg_from_disk)
>  		wait_for_completion(&ts->firmware_loading_complete);
>  
>  	return 0;
> @@ -1001,7 +1004,8 @@ static int __maybe_unused goodix_suspend(struct
> device *dev)
>  		return 0;
>  	}
>  
> -	wait_for_completion(&ts->firmware_loading_complete);
> +	if (ts->load_cfg_from_disk)
> +		wait_for_completion(&ts->firmware_loading_complete);
>  
>  	/* Free IRQ as IRQ pin is used as output in the suspend
> sequence */
>  	goodix_free_irq(ts);

Re: [PATCH resend 03/10] Input: goodix - Make resetting the controller at probe independent from the GPIO setup

[Bastien Nocera]

On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
> Before this commit we would always reset the controller at probe when
> we
> have access to the GPIOs which are necessary to do a reset.
> 
> Doing the reset requires access to the GPIOs, but just because we
> have
> access to the GPIOs does not mean that we should always reset the
> controller at probe. On X86 ACPI platforms the BIOS / UEFI firmware
> will
> already have reset the controller and it will have loaded the device
> specific config into the controller. Doing the reset sometimes causes
> the
> controller to loose its configuration, so on X86 ACPI platforms this 

lose.

> is not
> a good idea.
> 
> This commit adds a new reset_controller_at_probe boolean to control
> the
> reset at probe behavior.
> 
> This commits sets the new bool to true when we set
> irq_pin_access_method
> to irq_pin_access_gpio, so there are no functional changes.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
> Cc: Dmitry Mastykin <mastichi@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Looks good apart from the typo in the commit message

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

> ---
>  drivers/input/touchscreen/goodix.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index eccf07adfae1..dd5a8f9e8ada 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -56,6 +56,7 @@ struct goodix_ts_data {
>  	u16 id;
>  	u16 version;
>  	const char *cfg_name;
> +	bool reset_controller_at_probe;
>  	bool load_cfg_from_disk;
>  	struct completion firmware_loading_complete;
>  	unsigned long irq_flags;
> @@ -656,6 +657,7 @@ static int goodix_get_gpio_config(struct
> goodix_ts_data *ts)
>  	ts->gpiod_rst = gpiod;
>  
>  	if (ts->gpiod_int && ts->gpiod_rst) {
> +		ts->reset_controller_at_probe = true;
>  		ts->load_cfg_from_disk = true;
>  		ts->irq_pin_access_method = irq_pin_access_gpio;
>  	}
> @@ -932,7 +934,7 @@ static int goodix_ts_probe(struct i2c_client
> *client,
>  	if (error)
>  		return error;
>  
> -	if (ts->irq_pin_access_method == irq_pin_access_gpio) {
> +	if (ts->reset_controller_at_probe) {
>  		/* reset the controller */
>  		error = goodix_reset(ts);
>  		if (error) {

Re: [PATCH resend 04/10] Input: goodix - Add support for getting IRQ + reset GPIOs on Cherry Trail devices

[Bastien Nocera]

On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
> On most Cherry Trail (x86, UEFI + ACPI) devices the ACPI tables do
> not have
> a _DSD with a "daffd814-6eba-4d8c-8a91-bc9bbf4aa301" UUID, adding
> "irq-gpios" and "reset-gpios" mappings, so we cannot get the GPIOS by
> name
> without first manually adding mappings ourselves.
> 
> These devices contain 1 GpioInt and 1 GpioIo resource in their _CRS
> table.
> There is no fixed order for these 2. This commit adds code to check
> that
> there is 1 of each as expected and then registers a mapping matching
> their
> order using devm_acpi_dev_add_driver_gpios().
> 
> This gives us access to both GPIOs allowing us to properly suspend
> the
> controller during suspend, and making it possible to reset the
> controller
> if necessary.

Can you include the DSDT snippet that defines those GPIOs in the commit
message?

> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
> Cc: Dmitry Mastykin <mastichi@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/input/touchscreen/goodix.c | 113
> ++++++++++++++++++++++++++++-
>  1 file changed, 109 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index dd5a8f9e8ada..9de2f325ac6e 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -34,6 +34,7 @@ struct goodix_ts_data;
>  enum goodix_irq_pin_access_method {
>  	irq_pin_access_none,
>  	irq_pin_access_gpio,
> +	irq_pin_access_acpi_gpio,
>  };
>  
>  struct goodix_chip_data {
> @@ -53,6 +54,8 @@ struct goodix_ts_data {
>  	struct regulator *vddio;
>  	struct gpio_desc *gpiod_int;
>  	struct gpio_desc *gpiod_rst;
> +	int gpio_count;
> +	int gpio_int_idx;
>  	u16 id;
>  	u16 version;
>  	const char *cfg_name;
> @@ -521,6 +524,12 @@ static int goodix_irq_direction_output(struct
> goodix_ts_data *ts,
>  		return -EINVAL;
>  	case irq_pin_access_gpio:
>  		return gpiod_direction_output(ts->gpiod_int, value);
> +	case irq_pin_access_acpi_gpio:
> +		/*
> +		 * The IRQ pin triggers on a falling edge, so its gets
> marked
> +		 * as active-low, use output_raw to avoid the value
> inversion.
> +		 */
> +		return gpiod_direction_output_raw(ts->gpiod_int,
> value);
>  	}
>  
>  	return -EINVAL; /* Never reached */
> @@ -535,6 +544,7 @@ static int goodix_irq_direction_input(struct
> goodix_ts_data *ts)
>  			__func__);
>  		return -EINVAL;
>  	case irq_pin_access_gpio:
> +	case irq_pin_access_acpi_gpio:
>  		return gpiod_direction_input(ts->gpiod_int);
>  	}
>  
> @@ -599,6 +609,87 @@ static int goodix_reset(struct goodix_ts_data
> *ts)
>  	return 0;
>  }
>  
> +#if defined CONFIG_X86 && defined CONFIG_ACPI
> +static const struct acpi_gpio_params first_gpio = { 0, 0, false };
> +static const struct acpi_gpio_params second_gpio = { 1, 0, false };
> +
> +static const struct acpi_gpio_mapping acpi_goodix_int_first_gpios[]
> = {
> +	{ GOODIX_GPIO_INT_NAME "-gpios", &first_gpio, 1 },
> +	{ GOODIX_GPIO_RST_NAME "-gpios", &second_gpio, 1 },
> +	{ },
> +};
> +
> +static const struct acpi_gpio_mapping acpi_goodix_int_last_gpios[] =
> {
> +	{ GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1 },
> +	{ GOODIX_GPIO_INT_NAME "-gpios", &second_gpio, 1 },
> +	{ },
> +};
> +
> +static int goodix_resource(struct acpi_resource *ares, void *data)
> +{
> +	struct goodix_ts_data *ts = data;
> +	struct device *dev = &ts->client->dev;
> +	struct acpi_resource_gpio *gpio;
> +
> +	switch (ares->type) {
> +	case ACPI_RESOURCE_TYPE_GPIO:
> +		gpio = &ares->data.gpio;
> +		if (gpio->connection_type ==
> ACPI_RESOURCE_GPIO_TYPE_INT) {
> +			if (ts->gpio_int_idx == -1) {
> +				ts->gpio_int_idx = ts->gpio_count;
> +			} else {
> +				dev_err(dev, "More then one GpioInt
> resource, ignoring ACPI GPIO resources\n");
> +				ts->gpio_int_idx = -2;
> +			}
> +		}
> +		ts->gpio_count++;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts)

Is there no way to implement this in a more generic way? Is goodix the
only driver that needs this sort of handling of GPIOs for ACPI?

This portion could do with being split off, if we were ever to get that
more generic solution.

> +{
> +	const struct acpi_gpio_mapping *gpio_mapping = NULL;
> +	struct device *dev = &ts->client->dev;
> +	LIST_HEAD(resources);
> +	int ret;
> +
> +	ts->gpio_count = 0;
> +	ts->gpio_int_idx = -1;
> +	ret = acpi_dev_get_resources(ACPI_COMPANION(dev), &resources,
> +				     goodix_resource, ts);
> +	if (ret < 0) {
> +		dev_err(dev, "Error getting ACPI resources: %d\n",
> ret);
> +		return ret;
> +	}
> +
> +	acpi_dev_free_resource_list(&resources);
> +
> +	if (ts->gpio_count == 2 && ts->gpio_int_idx == 0) {
> +		ts->irq_pin_access_method = irq_pin_access_acpi_gpio;
> +		gpio_mapping = acpi_goodix_int_first_gpios;
> +	} else if (ts->gpio_count == 2 && ts->gpio_int_idx == 1) {
> +		ts->irq_pin_access_method = irq_pin_access_acpi_gpio;
> +		gpio_mapping = acpi_goodix_int_last_gpios;
> +	} else {
> +		dev_warn(dev, "Unexpected ACPI resources: gpio_count
> %d, gpio_int_idx %d\n",
> +			 ts->gpio_count, ts->gpio_int_idx);
> +		return -EINVAL;
> +	}
> +
> +	return devm_acpi_dev_add_driver_gpios(dev, gpio_mapping);
> +}
> +#else
> +static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts)
> +{
> +	return -EINVAL;
> +}
> +#endif /* CONFIG_X86 && CONFIG_ACPI */
> +
>  /**
>   * goodix_get_gpio_config - Get GPIO config from ACPI/DT
>   *
> @@ -609,6 +700,7 @@ static int goodix_get_gpio_config(struct
> goodix_ts_data *ts)
>  	int error;
>  	struct device *dev;
>  	struct gpio_desc *gpiod;
> +	bool added_acpi_mappings = false;
>  
>  	if (!ts->client)
>  		return -EINVAL;
> @@ -632,6 +724,7 @@ static int goodix_get_gpio_config(struct
> goodix_ts_data *ts)
>  		return error;
>  	}
>  
> +retry_get_irq_gpio:
>  	/* Get the interrupt GPIO pin number */
>  	gpiod = devm_gpiod_get_optional(dev, GOODIX_GPIO_INT_NAME,
> GPIOD_IN);
>  	if (IS_ERR(gpiod)) {
> @@ -641,6 +734,11 @@ static int goodix_get_gpio_config(struct
> goodix_ts_data *ts)
>  				GOODIX_GPIO_INT_NAME, error);
>  		return error;
>  	}
> +	if (!gpiod && has_acpi_companion(dev) && !added_acpi_mappings)
> {
> +		added_acpi_mappings = true;

Does this mean we retry at most once?

> +		if (goodix_add_acpi_gpio_mappings(ts) == 0)
> +			goto retry_get_irq_gpio;
> +	}
>  
>  	ts->gpiod_int = gpiod;
>  
> @@ -656,10 +754,17 @@ static int goodix_get_gpio_config(struct
> goodix_ts_data *ts)
>  
>  	ts->gpiod_rst = gpiod;
>  
> -	if (ts->gpiod_int && ts->gpiod_rst) {
> -		ts->reset_controller_at_probe = true;
> -		ts->load_cfg_from_disk = true;
> -		ts->irq_pin_access_method = irq_pin_access_gpio;
> +	switch (ts->irq_pin_access_method) {

Can't say I like switch statements with only 2 cases...

> +	case irq_pin_access_acpi_gpio:

Can you add a comment that explains that this is to fallback in case we
didn't manage to get the ACPI mappings?

> +		if (!ts->gpiod_int || !ts->gpiod_rst)
> +			ts->irq_pin_access_method =
> irq_pin_access_none;
> +		break;
> +	default:
> +		if (ts->gpiod_int && ts->gpiod_rst) {
> +			ts->reset_controller_at_probe = true;
> +			ts->load_cfg_from_disk = true;
> +			ts->irq_pin_access_method =
> irq_pin_access_gpio;
> +		}
>  	}
>  
>  	return 0;

Re: [PATCH resend 05/10] Input: goodix - Add support for getting IRQ + reset GPIOs on Bay Trail devices

[Bastien Nocera]

On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
> On most Bay Trail (x86, UEFI + ACPI) devices the ACPI tables do not
> have
> a _DSD with a "daffd814-6eba-4d8c-8a91-bc9bbf4aa301" UUID, adding
> "irq-gpios" and "reset-gpios" mappings, so we cannot get the GPIOS by
> name
> without first manually adding mappings ourselves.
> 
> These devices contain 2 GpioIo resource in their _CRS table, on all 4
> such
> devices which I have access to, the order of the 2 GPIOs is reset,
> int.
> 
> Note that the GPIO to which the touchscreen controller irq pin is
> connected
> is configured in direct-irq mode on these Bay Trail devices, the
> pinctrl-baytrail.c driver still allows controlling the pin as a GPIO
> in
> this case, but this is not necessarily the case on other X86 ACPI
> platforms, nor do we have a guarantee that the GPIO order is the same
> elsewhere, so we limit the use of a _CRS table with 2 GpioIo
> resources
> to Bay Trail devices only.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
> Cc: Dmitry Mastykin <mastichi@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

This is gross, but...

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

> ---
>  drivers/input/touchscreen/goodix.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 9de2f325ac6e..d178aa33b529 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -610,6 +610,21 @@ static int goodix_reset(struct goodix_ts_data
> *ts)
>  }
>  
>  #if defined CONFIG_X86 && defined CONFIG_ACPI
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +
> +static const struct x86_cpu_id baytrail_cpu_ids[] = {
> +	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_SILVERMONT,
> X86_FEATURE_ANY, },
> +	{}
> +};
> +
> +static inline bool is_byt(void)
> +{
> +	const struct x86_cpu_id *id = x86_match_cpu(baytrail_cpu_ids);
> +
> +	return !!id;
> +}
> +
>  static const struct acpi_gpio_params first_gpio = { 0, 0, false };
>  static const struct acpi_gpio_params second_gpio = { 1, 0, false };
>  
> @@ -675,6 +690,10 @@ static int goodix_add_acpi_gpio_mappings(struct
> goodix_ts_data *ts)
>  	} else if (ts->gpio_count == 2 && ts->gpio_int_idx == 1) {
>  		ts->irq_pin_access_method = irq_pin_access_acpi_gpio;
>  		gpio_mapping = acpi_goodix_int_last_gpios;
> +	} else if (is_byt() && ts->gpio_count == 2 && ts->gpio_int_idx
> == -1) {
> +		dev_info(dev, "No ACPI GpioInt resource, assuming that
> the GPIO order is reset, int\n");
> +		ts->irq_pin_access_method = irq_pin_access_acpi_gpio;
> +		gpio_mapping = acpi_goodix_int_last_gpios;
>  	} else {
>  		dev_warn(dev, "Unexpected ACPI resources: gpio_count
> %d, gpio_int_idx %d\n",
>  			 ts->gpio_count, ts->gpio_int_idx);

Re: [PATCH resend 06/10] Input: goodix - Add support for controlling the IRQ pin through ACPI methods

[Bastien Nocera]

On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
> Some Apollo Lake (x86, UEFI + ACPI) devices only list the reset GPIO
> in their _CRS table and the bit-banging of the IRQ line necessary to
> wake-up the controller from suspend can be done by calling 2 Goodix
> custom / specific ACPI methods.
> 
> This commit adds support for controlling the IRQ line in this matter,
> allowing us to properly suspend the touchscreen controller on such
> devices.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
> Cc: Dmitry Mastykin <mastichi@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

What.

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

> ---
>  drivers/input/touchscreen/goodix.c | 30
> ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index d178aa33b529..784c4dd8c430 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -35,6 +35,7 @@ enum goodix_irq_pin_access_method {
>  	irq_pin_access_none,
>  	irq_pin_access_gpio,
>  	irq_pin_access_acpi_gpio,
> +	irq_pin_access_acpi_method,
>  };
>  
>  struct goodix_chip_data {
> @@ -516,6 +517,9 @@ static int goodix_send_cfg(struct goodix_ts_data
> *ts,
>  static int goodix_irq_direction_output(struct goodix_ts_data *ts,
>  				       int value)
>  {
> +	struct device *dev = &ts->client->dev;
> +	acpi_status status;
> +
>  	switch (ts->irq_pin_access_method) {
>  	case irq_pin_access_none:
>  		dev_err(&ts->client->dev,
> @@ -530,6 +534,10 @@ static int goodix_irq_direction_output(struct
> goodix_ts_data *ts,
>  		 * as active-low, use output_raw to avoid the value
> inversion.
>  		 */
>  		return gpiod_direction_output_raw(ts->gpiod_int,
> value);
> +	case irq_pin_access_acpi_method:
> +		status = acpi_execute_simple_method(ACPI_HANDLE(dev),
> +						    "INTO", value);
> +		return ACPI_SUCCESS(status) ? 0 : -EIO;
>  	}
>  
>  	return -EINVAL; /* Never reached */
> @@ -537,6 +545,9 @@ static int goodix_irq_direction_output(struct
> goodix_ts_data *ts,
>  
>  static int goodix_irq_direction_input(struct goodix_ts_data *ts)
>  {
> +	struct device *dev = &ts->client->dev;
> +	acpi_status status;
> +
>  	switch (ts->irq_pin_access_method) {
>  	case irq_pin_access_none:
>  		dev_err(&ts->client->dev,
> @@ -546,6 +557,10 @@ static int goodix_irq_direction_input(struct
> goodix_ts_data *ts)
>  	case irq_pin_access_gpio:
>  	case irq_pin_access_acpi_gpio:
>  		return gpiod_direction_input(ts->gpiod_int);
> +	case irq_pin_access_acpi_method:
> +		status = acpi_evaluate_object(ACPI_HANDLE(dev), "INTI",
> +					      NULL, NULL);
> +		return ACPI_SUCCESS(status) ? 0 : -EIO;
>  	}
>  
>  	return -EINVAL; /* Never reached */
> @@ -640,6 +655,11 @@ static const struct acpi_gpio_mapping
> acpi_goodix_int_last_gpios[] = {
>  	{ },
>  };
>  
> +static const struct acpi_gpio_mapping acpi_goodix_reset_only_gpios[]
> = {
> +	{ GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1 },
> +	{ },
> +};
> +
>  static int goodix_resource(struct acpi_resource *ares, void *data)
>  {
>  	struct goodix_ts_data *ts = data;
> @@ -690,6 +710,12 @@ static int goodix_add_acpi_gpio_mappings(struct
> goodix_ts_data *ts)
>  	} else if (ts->gpio_count == 2 && ts->gpio_int_idx == 1) {
>  		ts->irq_pin_access_method = irq_pin_access_acpi_gpio;
>  		gpio_mapping = acpi_goodix_int_last_gpios;
> +	} else if (ts->gpio_count == 1 && ts->gpio_int_idx == -1 &&
> +		   acpi_has_method(ACPI_HANDLE(dev), "INTI") &&
> +		   acpi_has_method(ACPI_HANDLE(dev), "INTO")) {
> +		dev_info(dev, "Using ACPI INTI and INTO methods for IRQ
> pin access\n");
> +		ts->irq_pin_access_method = irq_pin_access_acpi_method;
> +		gpio_mapping = acpi_goodix_reset_only_gpios;
>  	} else if (is_byt() && ts->gpio_count == 2 && ts->gpio_int_idx
> == -1) {
>  		dev_info(dev, "No ACPI GpioInt resource, assuming that
> the GPIO order is reset, int\n");
>  		ts->irq_pin_access_method = irq_pin_access_acpi_gpio;
> @@ -778,6 +804,10 @@ static int goodix_get_gpio_config(struct
> goodix_ts_data *ts)
>  		if (!ts->gpiod_int || !ts->gpiod_rst)
>  			ts->irq_pin_access_method =
> irq_pin_access_none;
>  		break;
> +	case irq_pin_access_acpi_method:
> +		if (!ts->gpiod_rst)
> +			ts->irq_pin_access_method =
> irq_pin_access_none;
> +		break;
>  	default:
>  		if (ts->gpiod_int && ts->gpiod_rst) {
>  			ts->reset_controller_at_probe = true;

Re: [PATCH resend 07/10] Input: goodix - Move defines to above struct goodix_ts_data declaration

[Bastien Nocera]

On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
> Move the  defines to above the struct goodix_ts_data declaration, so
> that the MAX defines can be used inside the struct goodix_ts_data
> declaration. No functional changes, just moving a block of code.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
> Cc: Dmitry Mastykin <mastichi@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Sure.

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

> ---
>  drivers/input/touchscreen/goodix.c | 60 +++++++++++++++-------------
> --
>  1 file changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 784c4dd8c430..66d6bb74507d 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -29,6 +29,36 @@
>  #include <linux/of.h>
>  #include <asm/unaligned.h>
>  
> +#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
> +#define GOODIX_CONTACT_SIZE		8
> +#define GOODIX_MAX_CONTACT_SIZE		9
> +#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_REG_COMMAND		0x8040
> +#define GOODIX_CMD_SCREEN_OFF		0x05
> +
> +#define GOODIX_READ_COOR_ADDR		0x814E
> +#define GOODIX_GT1X_REG_CONFIG_DATA	0x8050
> +#define GOODIX_GT9X_REG_CONFIG_DATA	0x8047
> +#define GOODIX_REG_ID			0x8140
> +
> +#define GOODIX_BUFFER_STATUS_READY	BIT(7)
> +#define GOODIX_BUFFER_STATUS_TIMEOUT	20
> +
> +#define RESOLUTION_LOC		1
> +#define MAX_CONTACTS_LOC	5
> +#define TRIGGER_LOC		6
> +
>  struct goodix_ts_data;
>  
>  enum goodix_irq_pin_access_method {
> @@ -68,36 +98,6 @@ struct goodix_ts_data {
>  	unsigned int contact_size;
>  };
>  
> -#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
> -#define GOODIX_CONTACT_SIZE		8
> -#define GOODIX_MAX_CONTACT_SIZE		9
> -#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_REG_COMMAND		0x8040
> -#define GOODIX_CMD_SCREEN_OFF		0x05
> -
> -#define GOODIX_READ_COOR_ADDR		0x814E
> -#define GOODIX_GT1X_REG_CONFIG_DATA	0x8050
> -#define GOODIX_GT9X_REG_CONFIG_DATA	0x8047
> -#define GOODIX_REG_ID			0x8140
> -
> -#define GOODIX_BUFFER_STATUS_READY	BIT(7)
> -#define GOODIX_BUFFER_STATUS_TIMEOUT	20
> -
> -#define RESOLUTION_LOC		1
> -#define MAX_CONTACTS_LOC	5
> -#define TRIGGER_LOC		6
> -
>  static int goodix_check_cfg_8(struct goodix_ts_data *ts,
>  			const struct firmware *cfg);
>  static int goodix_check_cfg_16(struct goodix_ts_data *ts,

Re: [PATCH resend 08/10] Input: goodix - Save a copy of the config from goodix_read_config()

[Bastien Nocera]

On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
> Save a copy of the config in goodix_read_config(), this is a
> preparation
> patch for restoring the config if it was lost after a supend/resume
> cycle.

Is "fix" really the best verb for this "calculate checksum"?

> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
> Cc: Dmitry Mastykin <mastichi@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Looks fine apart from the nitpicks.

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

> ---
>  drivers/input/touchscreen/goodix.c | 51 ++++++++++++++++++++++++++
> ----
>  1 file changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 66d6bb74507d..21be33384d14 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -72,6 +72,7 @@ struct goodix_chip_data {
>  	u16 config_addr;
>  	int config_len;
>  	int (*check_config)(struct goodix_ts_data *, const struct
> firmware *);
> +	void (*fix_config)(struct goodix_ts_data *ts);
>  };
>  
>  struct goodix_ts_data {
> @@ -96,35 +97,42 @@ struct goodix_ts_data {
>  	unsigned long irq_flags;
>  	enum goodix_irq_pin_access_method irq_pin_access_method;
>  	unsigned int contact_size;
> +	u8 config[GOODIX_CONFIG_MAX_LENGTH];
>  };
>  
>  static int goodix_check_cfg_8(struct goodix_ts_data *ts,
>  			const struct firmware *cfg);
>  static int goodix_check_cfg_16(struct goodix_ts_data *ts,
>  			const struct firmware *cfg);
> +static void goodix_fix_cfg_8(struct goodix_ts_data *ts);
> +static void goodix_fix_cfg_16(struct goodix_ts_data *ts);
>  
>  static const struct goodix_chip_data gt1x_chip_data = {
>  	.config_addr		= GOODIX_GT1X_REG_CONFIG_DATA,
>  	.config_len		= GOODIX_CONFIG_MAX_LENGTH,
>  	.check_config		= goodix_check_cfg_16,
> +	.fix_config		= goodix_fix_cfg_16,
>  };
>  
>  static const struct goodix_chip_data gt911_chip_data = {
>  	.config_addr		= GOODIX_GT9X_REG_CONFIG_DATA,
>  	.config_len		= GOODIX_CONFIG_911_LENGTH,
>  	.check_config		= goodix_check_cfg_8,
> +	.fix_config		= goodix_fix_cfg_8,
>  };
>  
>  static const struct goodix_chip_data gt967_chip_data = {
>  	.config_addr		= GOODIX_GT9X_REG_CONFIG_DATA,
>  	.config_len		= GOODIX_CONFIG_967_LENGTH,
>  	.check_config		= goodix_check_cfg_8,
> +	.fix_config		= goodix_fix_cfg_8,
>  };
>  
>  static const struct goodix_chip_data gt9x_chip_data = {
>  	.config_addr		= GOODIX_GT9X_REG_CONFIG_DATA,
>  	.config_len		= GOODIX_CONFIG_MAX_LENGTH,
>  	.check_config		= goodix_check_cfg_8,
> +	.fix_config		= goodix_fix_cfg_8,
>  };
>  
>  static const unsigned long goodix_irq_flags[] = {
> @@ -442,6 +450,19 @@ static int goodix_check_cfg_8(struct
> goodix_ts_data *ts,
>  	return 0;
>  }
>  
> +static void goodix_fix_cfg_8(struct goodix_ts_data *ts)
> +{
> +	int i, raw_cfg_len = ts->chip->config_len - 2;
> +	u8 check_sum = 0;
> +
> +	for (i = 0; i < raw_cfg_len; i++)
> +		check_sum += ts->config[i];
> +	check_sum = (~check_sum) + 1;
> +
> +	ts->config[raw_cfg_len] = check_sum;
> +	ts->config[raw_cfg_len + 1] = 1;

Would be good to mention that this is the "config_fresh" bit, in this
function and others.

> +}
> +
>  static int goodix_check_cfg_16(struct goodix_ts_data *ts,
>  			const struct firmware *cfg)
>  {
> @@ -466,6 +487,19 @@ static int goodix_check_cfg_16(struct
> goodix_ts_data *ts,
>  	return 0;
>  }
>  
> +static void goodix_fix_cfg_16(struct goodix_ts_data *ts)
> +{
> +	int i, raw_cfg_len = ts->chip->config_len - 3;
> +	u16 check_sum = 0;
> +
> +	for (i = 0; i < raw_cfg_len; i += 2)
> +		check_sum += get_unaligned_be16(&ts->config[i]);
> +	check_sum = (~check_sum) + 1;
> +
> +	put_unaligned_be16(check_sum, &ts->config[raw_cfg_len]);
> +	ts->config[raw_cfg_len + 2] = 1;
> +}
> +
>  /**
>   * goodix_check_cfg - Checks if config fw is valid
>   *
> @@ -828,12 +862,11 @@ static int goodix_get_gpio_config(struct
> goodix_ts_data *ts)
>   */
>  static void goodix_read_config(struct goodix_ts_data *ts)
>  {
> -	u8 config[GOODIX_CONFIG_MAX_LENGTH];
>  	int x_max, y_max;
>  	int error;
>  
>  	error = goodix_i2c_read(ts->client, ts->chip->config_addr,
> -				config, ts->chip->config_len);
> +				ts->config, ts->chip->config_len);
>  	if (error) {
>  		dev_warn(&ts->client->dev, "Error reading config:
> %d\n",
>  			 error);
> @@ -842,15 +875,21 @@ static void goodix_read_config(struct
> goodix_ts_data *ts)
>  		return;
>  	}
>  
> -	ts->int_trigger_type = config[TRIGGER_LOC] & 0x03;
> -	ts->max_touch_num = config[MAX_CONTACTS_LOC] & 0x0f;
> +	ts->int_trigger_type = ts->config[TRIGGER_LOC] & 0x03;
> +	ts->max_touch_num = ts->config[MAX_CONTACTS_LOC] & 0x0f;
>  
> -	x_max = get_unaligned_le16(&config[RESOLUTION_LOC]);
> -	y_max = get_unaligned_le16(&config[RESOLUTION_LOC + 2]);
> +	x_max = get_unaligned_le16(&ts->config[RESOLUTION_LOC]);
> +	y_max = get_unaligned_le16(&ts->config[RESOLUTION_LOC + 2]);
>  	if (x_max && y_max) {
>  		input_abs_set_max(ts->input_dev, ABS_MT_POSITION_X,
> x_max - 1);
>  		input_abs_set_max(ts->input_dev, ABS_MT_POSITION_Y,
> y_max - 1);
>  	}
> +
> +	/*
> +	 * Ensure valid checksum and config_fresh bit being set for
> possible
> +	 * re-upload of config after suspend/resume.
> +	 */

I think that the explanation should ultimately be shorter if the vfunc
was better named.

> +	ts->chip->fix_config(ts);
>  }
>  
>  /**

Re: [PATCH resend 09/10] Input: goodix - Make goodix_send_cfg() take a raw buffer as argument

[Bastien Nocera]

On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
> Make goodix_send_cfg() take a raw buffer as argument instead of a
> struct firmware *cfg, so that it can also be used to restore the
> config
> on resume if necessary.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
> Cc: Dmitry Mastykin <mastichi@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/input/touchscreen/goodix.c | 46 ++++++++++++++------------
> ----
>  1 file changed, 21 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 21be33384d14..0f39c499e3a9 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -71,7 +71,7 @@ enum goodix_irq_pin_access_method {
>  struct goodix_chip_data {
>  	u16 config_addr;
>  	int config_len;
> -	int (*check_config)(struct goodix_ts_data *, const struct
> firmware *);
> +	int (*check_config)(struct goodix_ts_data *ts, const u8 *cfg,
> int len);

Any way to make the length a uint instead of an int? That way, we
wouldn't need to add < 0 guards, and the "len > MAX_LENGTH" check would
be enough.

Looks good otherwise:

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

>  	void (*fix_config)(struct goodix_ts_data *ts);
>  };
>  
> @@ -101,9 +101,9 @@ struct goodix_ts_data {
>  };
>  
>  static int goodix_check_cfg_8(struct goodix_ts_data *ts,
> -			const struct firmware *cfg);
> +			      const u8 *cfg, int len);
>  static int goodix_check_cfg_16(struct goodix_ts_data *ts,
> -			const struct firmware *cfg);
> +			       const u8 *cfg, int len);
>  static void goodix_fix_cfg_8(struct goodix_ts_data *ts);
>  static void goodix_fix_cfg_16(struct goodix_ts_data *ts);
>  
> @@ -426,22 +426,21 @@ static int goodix_request_irq(struct
> goodix_ts_data *ts)
>  					 ts->irq_flags, ts->client-
> >name, ts);
>  }
>  
> -static int goodix_check_cfg_8(struct goodix_ts_data *ts,
> -			const struct firmware *cfg)
> +static int goodix_check_cfg_8(struct goodix_ts_data *ts, const u8
> *cfg, int len)
>  {
> -	int i, raw_cfg_len = cfg->size - 2;
> +	int i, raw_cfg_len = len - 2;
>  	u8 check_sum = 0;
>  
>  	for (i = 0; i < raw_cfg_len; i++)
> -		check_sum += cfg->data[i];
> +		check_sum += cfg[i];
>  	check_sum = (~check_sum) + 1;
> -	if (check_sum != cfg->data[raw_cfg_len]) {
> +	if (check_sum != cfg[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) {
> +	if (cfg[raw_cfg_len + 1] != 1) {
>  		dev_err(&ts->client->dev,
>  			"Config fw must have Config_Fresh register
> set");
>  		return -EINVAL;
> @@ -463,22 +462,22 @@ static void goodix_fix_cfg_8(struct
> goodix_ts_data *ts)
>  	ts->config[raw_cfg_len + 1] = 1;
>  }
>  
> -static int goodix_check_cfg_16(struct goodix_ts_data *ts,
> -			const struct firmware *cfg)
> +static int goodix_check_cfg_16(struct goodix_ts_data *ts, const u8
> *cfg,
> +			       int len)
>  {
> -	int i, raw_cfg_len = cfg->size - 3;
> +	int i, raw_cfg_len = len - 3;
>  	u16 check_sum = 0;
>  
>  	for (i = 0; i < raw_cfg_len; i += 2)
> -		check_sum += get_unaligned_be16(&cfg->data[i]);
> +		check_sum += get_unaligned_be16(&cfg[i]);
>  	check_sum = (~check_sum) + 1;
> -	if (check_sum != get_unaligned_be16(&cfg->data[raw_cfg_len])) {
> +	if (check_sum != get_unaligned_be16(&cfg[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 + 2] != 1) {
> +	if (cfg[raw_cfg_len + 2] != 1) {
>  		dev_err(&ts->client->dev,
>  			"Config fw must have Config_Fresh register
> set");
>  		return -EINVAL;
> @@ -506,16 +505,15 @@ static void goodix_fix_cfg_16(struct
> goodix_ts_data *ts)
>   * @ts: goodix_ts_data pointer
>   * @cfg: firmware config data
>   */
> -static int goodix_check_cfg(struct goodix_ts_data *ts,
> -			    const struct firmware *cfg)
> +static int goodix_check_cfg(struct goodix_ts_data *ts, const u8
> *cfg, int len)
>  {
> -	if (cfg->size > GOODIX_CONFIG_MAX_LENGTH) {
> +	if (len > GOODIX_CONFIG_MAX_LENGTH) {
>  		dev_err(&ts->client->dev,
>  			"The length of the config fw is not correct");
>  		return -EINVAL;
>  	}
>  
> -	return ts->chip->check_config(ts, cfg);
> +	return ts->chip->check_config(ts, cfg, len);
>  }
>  
>  /**
> @@ -524,17 +522,15 @@ static int goodix_check_cfg(struct
> goodix_ts_data *ts,
>   * @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)
> +static int goodix_send_cfg(struct goodix_ts_data *ts, const u8 *cfg,
> int len)
>  {
>  	int error;
>  
> -	error = goodix_check_cfg(ts, cfg);
> +	error = goodix_check_cfg(ts, cfg, len);
>  	if (error)
>  		return error;
>  
> -	error = goodix_i2c_write(ts->client, ts->chip->config_addr,
> cfg->data,
> -				 cfg->size);
> +	error = goodix_i2c_write(ts->client, ts->chip->config_addr,
> cfg, len);
>  	if (error) {
>  		dev_err(&ts->client->dev, "Failed to write config data:
> %d",
>  			error);
> @@ -1058,7 +1054,7 @@ static void goodix_config_cb(const struct
> firmware *cfg, void *ctx)
>  
>  	if (cfg) {
>  		/* send device configuration to the firmware */
> -		error = goodix_send_cfg(ts, cfg);
> +		error = goodix_send_cfg(ts, cfg->data, cfg->size);
>  		if (error)
>  			goto err_release_cfg;
>  	}

Re: [PATCH resend 10/10] Input: goodix - Restore config on resume if necessary

[Bastien Nocera]

On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
> Some devices, e.g the Trekstor Primetab S11B, loose there config over

"lose".

> a suspend/resume cycle (likely the controller looses power during 

"loses".

> suspend).
> 
> This commit reads back the config version on resume and if matches
> the
> expected config version it resets the controller and resends the
> config
> we read back and saved at probe time.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
> Cc: Dmitry Mastykin <mastichi@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Looks fine apart from the nitpicks.

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

> ---
>  drivers/input/touchscreen/goodix.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 0f39c499e3a9..389d3e044f97 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -1232,6 +1232,7 @@ 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);
> +	u8 config_ver;
>  	int error;
>  
>  	if (ts->irq_pin_access_method == irq_pin_access_none) {
> @@ -1253,6 +1254,27 @@ static int __maybe_unused goodix_resume(struct
> device *dev)
>  	if (error)
>  		return error;
>  
> +	error = goodix_i2c_read(ts->client, ts->chip->config_addr,
> +				&config_ver, 1);
> +	if (error)
> +		dev_warn(dev, "Error reading config version: %d,
> resetting controller\n",
> +			 error);
> +	else if (config_ver != ts->config[0])
> +		dev_warn(dev, "Config version mismatch %d != %d,
> resetting controller\n",
> +			 config_ver, ts->config[0]);

Should it really be a warning if it happens regularly?

> +
> +	if (error != 0 || config_ver != ts->config[0]) {
> +		error = goodix_reset(ts);
> +		if (error) {
> +			dev_err(dev, "Controller reset failed.\n");
> +			return error;
> +		}
> +
> +		error = goodix_send_cfg(ts, ts->config, ts->chip-
> >config_len);
> +		if (error)
> +			return error;
> +	}
> +
>  	error = goodix_request_irq(ts);
>  	if (error)
>  		return error;

Re: [PATCH resend 01/10] Input: goodix - Refactor IRQ pin GPIO accesses

[Hans de Goede]

Hi,

On 3/2/20 12:09 PM, Bastien Nocera wrote:
> On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
>> Suspending Goodix touchscreens requires changing the interrupt pin to
>> output before sending them a power-down command. Followed by wiggling
>> the interrupt pin to wake the device up, after which it is put back
>> in input mode.
>>
>> So far we have only effectively supported this on devices which use
>> devicetree. On X86 ACPI platforms both looking up the pins; and using
>> a
>> pin as both IRQ and GPIO is a bit more complicated. E.g. on some
>> devices
>> we cannot directly access the IRQ pin as GPIO and we need to call
>> ACPI
>> methods to control it instead.
>>
>> This commit adds a new irq_pin_access_method field to the
>> goodix_chip_data
>> struct and adds goodix_irq_direction_output and
>> goodix_irq_direction_input
>> helpers which together abstract the GPIO accesses to the IRQ pin.
>>
>> This is a preparation patch for adding support for properly
>> suspending the
>> touchscreen on X86 ACPI platforms.
>>
>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
>> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
>> Cc: Dmitry Mastykin <mastichi@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/input/touchscreen/goodix.c | 62 ++++++++++++++++++++++++--
>> ----
>>   1 file changed, 51 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/goodix.c
>> b/drivers/input/touchscreen/goodix.c
>> index 0403102e807e..08806a00a9b9 100644
>> --- a/drivers/input/touchscreen/goodix.c
>> +++ b/drivers/input/touchscreen/goodix.c
>> @@ -31,6 +31,11 @@
>>   
>>   struct goodix_ts_data;
>>   
>> +enum goodix_irq_pin_access_method {
>> +	irq_pin_access_none,
>> +	irq_pin_access_gpio,
> 
> I don't know if that matches the kernel coding style, but I'd rather
> the enum member names were upper-case, and typedef'ed.

I checked and most of the kernel also uses upper-case for
enum member names, I'll fix this (for the entire series) for
v2 of the series.

As for using typedef-s that is typically not done in the kernel
for enums / structs so I'm going to keep that as is.


> 
>> +};
>> +
>>   struct goodix_chip_data {
>>   	u16 config_addr;
>>   	int config_len;
>> @@ -53,6 +58,7 @@ struct goodix_ts_data {
>>   	const char *cfg_name;
>>   	struct completion firmware_loading_complete;
>>   	unsigned long irq_flags;
>> +	enum goodix_irq_pin_access_method irq_pin_access_method;
>>   	unsigned int contact_size;
>>   };
>>   
>> @@ -502,17 +508,48 @@ static int goodix_send_cfg(struct
>> goodix_ts_data *ts,
>>   	return 0;
>>   }
>>   
>> +static int goodix_irq_direction_output(struct goodix_ts_data *ts,
>> +				       int value)
>> +{
>> +	switch (ts->irq_pin_access_method) {
>> +	case irq_pin_access_none:
>> +		dev_err(&ts->client->dev,
>> +			"%s called without an irq_pin_access_method
>> set\n",
>> +			__func__);
>> +		return -EINVAL;
>> +	case irq_pin_access_gpio:
>> +		return gpiod_direction_output(ts->gpiod_int, value);
> 
> Is that going to complain about default not being handled? If so, an if
> conditional might be enough.

All the values in the enum are handled so there is no need for a default
label. As for changing this to an if, later patches add more values
to the enum and to the switch-case, having this as a switch-case from
the start makes the diff in later patches smaller.

> 
>> +	}
>> +
>> +	return -EINVAL; /* Never reached */
>> +}
>> +
>> +static int goodix_irq_direction_input(struct goodix_ts_data *ts)
>> +{
>> +	switch (ts->irq_pin_access_method) {
>> +	case irq_pin_access_none:
>> +		dev_err(&ts->client->dev,
>> +			"%s called without an irq_pin_access_method
>> set\n",
>> +			__func__);
>> +		return -EINVAL;
>> +	case irq_pin_access_gpio:
>> +		return gpiod_direction_input(ts->gpiod_int);
> 
> Ditto.

Ditto.

> Looks fine otherwise.

Thank you for the review.

Regards,

Hans



> 
> 
>> +	}
>> +
>> +	return -EINVAL; /* Never reached */
>> +}
>> +
>>   static int goodix_int_sync(struct goodix_ts_data *ts)
>>   {
>>   	int error;
>>   
>> -	error = gpiod_direction_output(ts->gpiod_int, 0);
>> +	error = goodix_irq_direction_output(ts, 0);
>>   	if (error)
>>   		return error;
>>   
>>   	msleep(50);				/* T5: 50ms */
>>   
>> -	error = gpiod_direction_input(ts->gpiod_int);
>> +	error = goodix_irq_direction_input(ts);
>>   	if (error)
>>   		return error;
>>   
>> @@ -536,7 +573,7 @@ static int goodix_reset(struct goodix_ts_data
>> *ts)
>>   	msleep(20);				/* T2: > 10ms */
>>   
>>   	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
>> -	error = gpiod_direction_output(ts->gpiod_int, ts->client->addr
>> == 0x14);
>> +	error = goodix_irq_direction_output(ts, ts->client->addr ==
>> 0x14);
>>   	if (error)
>>   		return error;
>>   
>> @@ -617,6 +654,9 @@ static int goodix_get_gpio_config(struct
>> goodix_ts_data *ts)
>>   
>>   	ts->gpiod_rst = gpiod;
>>   
>> +	if (ts->gpiod_int && ts->gpiod_rst)
>> +		ts->irq_pin_access_method = irq_pin_access_gpio;
>> +
>>   	return 0;
>>   }
>>   
>> @@ -889,7 +929,7 @@ static int goodix_ts_probe(struct i2c_client
>> *client,
>>   	if (error)
>>   		return error;
>>   
>> -	if (ts->gpiod_int && ts->gpiod_rst) {
>> +	if (ts->irq_pin_access_method == irq_pin_access_gpio) {
>>   		/* reset the controller */
>>   		error = goodix_reset(ts);
>>   		if (error) {
>> @@ -912,7 +952,7 @@ static int goodix_ts_probe(struct i2c_client
>> *client,
>>   
>>   	ts->chip = goodix_get_chip_data(ts->id);
>>   
>> -	if (ts->gpiod_int && ts->gpiod_rst) {
>> +	if (ts->irq_pin_access_method == irq_pin_access_gpio) {
>>   		/* update device config */
>>   		ts->cfg_name = devm_kasprintf(&client->dev, GFP_KERNEL,
>>   					      "goodix_%d_cfg.bin", ts-
>>> id);
>> @@ -943,7 +983,7 @@ 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)
>> +	if (ts->irq_pin_access_method == irq_pin_access_gpio)
>>   		wait_for_completion(&ts->firmware_loading_complete);
>>   
>>   	return 0;
>> @@ -956,7 +996,7 @@ static int __maybe_unused goodix_suspend(struct
>> device *dev)
>>   	int error;
>>   
>>   	/* We need gpio pins to suspend/resume */
>> -	if (!ts->gpiod_int || !ts->gpiod_rst) {
>> +	if (ts->irq_pin_access_method == irq_pin_access_none) {
>>   		disable_irq(client->irq);
>>   		return 0;
>>   	}
>> @@ -967,7 +1007,7 @@ static int __maybe_unused goodix_suspend(struct
>> device *dev)
>>   	goodix_free_irq(ts);
>>   
>>   	/* Output LOW on the INT pin for 5 ms */
>> -	error = gpiod_direction_output(ts->gpiod_int, 0);
>> +	error = goodix_irq_direction_output(ts, 0);
>>   	if (error) {
>>   		goodix_request_irq(ts);
>>   		return error;
>> @@ -979,7 +1019,7 @@ static int __maybe_unused goodix_suspend(struct
>> device *dev)
>>   				    GOODIX_CMD_SCREEN_OFF);
>>   	if (error) {
>>   		dev_err(&ts->client->dev, "Screen off command
>> failed\n");
>> -		gpiod_direction_input(ts->gpiod_int);
>> +		goodix_irq_direction_input(ts);
>>   		goodix_request_irq(ts);
>>   		return -EAGAIN;
>>   	}
>> @@ -999,7 +1039,7 @@ static int __maybe_unused goodix_resume(struct
>> device *dev)
>>   	struct goodix_ts_data *ts = i2c_get_clientdata(client);
>>   	int error;
>>   
>> -	if (!ts->gpiod_int || !ts->gpiod_rst) {
>> +	if (ts->irq_pin_access_method == irq_pin_access_none) {
>>   		enable_irq(client->irq);
>>   		return 0;
>>   	}
>> @@ -1008,7 +1048,7 @@ static int __maybe_unused goodix_resume(struct
>> device *dev)
>>   	 * Exit sleep mode by outputting HIGH level to INT pin
>>   	 * for 2ms~5ms.
>>   	 */
>> -	error = gpiod_direction_output(ts->gpiod_int, 1);
>> +	error = goodix_irq_direction_output(ts, 1);
>>   	if (error)
>>   		return error;
>>   
> 

Re: [PATCH resend 04/10] Input: goodix - Add support for getting IRQ + reset GPIOs on Cherry Trail devices

[Hans de Goede]

Hi,

On 3/2/20 12:23 PM, Bastien Nocera wrote:
> On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
>> On most Cherry Trail (x86, UEFI + ACPI) devices the ACPI tables do
>> not have
>> a _DSD with a "daffd814-6eba-4d8c-8a91-bc9bbf4aa301" UUID, adding
>> "irq-gpios" and "reset-gpios" mappings, so we cannot get the GPIOS by
>> name
>> without first manually adding mappings ourselves.
>>
>> These devices contain 1 GpioInt and 1 GpioIo resource in their _CRS
>> table.
>> There is no fixed order for these 2. This commit adds code to check
>> that
>> there is 1 of each as expected and then registers a mapping matching
>> their
>> order using devm_acpi_dev_add_driver_gpios().
>>
>> This gives us access to both GPIOs allowing us to properly suspend
>> the
>> controller during suspend, and making it possible to reset the
>> controller
>> if necessary.
> 
> Can you include the DSDT snippet that defines those GPIOs in the commit
> message?

Will do for v2 of this series.

>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
>> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
>> Cc: Dmitry Mastykin <mastichi@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/input/touchscreen/goodix.c | 113
>> ++++++++++++++++++++++++++++-
>>   1 file changed, 109 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/goodix.c
>> b/drivers/input/touchscreen/goodix.c
>> index dd5a8f9e8ada..9de2f325ac6e 100644
>> --- a/drivers/input/touchscreen/goodix.c
>> +++ b/drivers/input/touchscreen/goodix.c
>> @@ -34,6 +34,7 @@ struct goodix_ts_data;
>>   enum goodix_irq_pin_access_method {
>>   	irq_pin_access_none,
>>   	irq_pin_access_gpio,
>> +	irq_pin_access_acpi_gpio,
>>   };
>>   
>>   struct goodix_chip_data {
>> @@ -53,6 +54,8 @@ struct goodix_ts_data {
>>   	struct regulator *vddio;
>>   	struct gpio_desc *gpiod_int;
>>   	struct gpio_desc *gpiod_rst;
>> +	int gpio_count;
>> +	int gpio_int_idx;
>>   	u16 id;
>>   	u16 version;
>>   	const char *cfg_name;
>> @@ -521,6 +524,12 @@ static int goodix_irq_direction_output(struct
>> goodix_ts_data *ts,
>>   		return -EINVAL;
>>   	case irq_pin_access_gpio:
>>   		return gpiod_direction_output(ts->gpiod_int, value);
>> +	case irq_pin_access_acpi_gpio:
>> +		/*
>> +		 * The IRQ pin triggers on a falling edge, so its gets
>> marked
>> +		 * as active-low, use output_raw to avoid the value
>> inversion.
>> +		 */
>> +		return gpiod_direction_output_raw(ts->gpiod_int,
>> value);
>>   	}
>>   
>>   	return -EINVAL; /* Never reached */
>> @@ -535,6 +544,7 @@ static int goodix_irq_direction_input(struct
>> goodix_ts_data *ts)
>>   			__func__);
>>   		return -EINVAL;
>>   	case irq_pin_access_gpio:
>> +	case irq_pin_access_acpi_gpio:
>>   		return gpiod_direction_input(ts->gpiod_int);
>>   	}
>>   
>> @@ -599,6 +609,87 @@ static int goodix_reset(struct goodix_ts_data
>> *ts)
>>   	return 0;
>>   }
>>   
>> +#if defined CONFIG_X86 && defined CONFIG_ACPI
>> +static const struct acpi_gpio_params first_gpio = { 0, 0, false };
>> +static const struct acpi_gpio_params second_gpio = { 1, 0, false };
>> +
>> +static const struct acpi_gpio_mapping acpi_goodix_int_first_gpios[]
>> = {
>> +	{ GOODIX_GPIO_INT_NAME "-gpios", &first_gpio, 1 },
>> +	{ GOODIX_GPIO_RST_NAME "-gpios", &second_gpio, 1 },
>> +	{ },
>> +};
>> +
>> +static const struct acpi_gpio_mapping acpi_goodix_int_last_gpios[] =
>> {
>> +	{ GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1 },
>> +	{ GOODIX_GPIO_INT_NAME "-gpios", &second_gpio, 1 },
>> +	{ },
>> +};
>> +
>> +static int goodix_resource(struct acpi_resource *ares, void *data)
>> +{
>> +	struct goodix_ts_data *ts = data;
>> +	struct device *dev = &ts->client->dev;
>> +	struct acpi_resource_gpio *gpio;
>> +
>> +	switch (ares->type) {
>> +	case ACPI_RESOURCE_TYPE_GPIO:
>> +		gpio = &ares->data.gpio;
>> +		if (gpio->connection_type ==
>> ACPI_RESOURCE_GPIO_TYPE_INT) {
>> +			if (ts->gpio_int_idx == -1) {
>> +				ts->gpio_int_idx = ts->gpio_count;
>> +			} else {
>> +				dev_err(dev, "More then one GpioInt
>> resource, ignoring ACPI GPIO resources\n");
>> +				ts->gpio_int_idx = -2;
>> +			}
>> +		}
>> +		ts->gpio_count++;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts)
> 
> Is there no way to implement this in a more generic way? Is goodix the
> only driver that needs this sort of handling of GPIOs for ACPI?

The Linux GPIO subsystem expects drivers to request GPIOs by name, but
in most ACPI tables there is only a list of resources, so we have an
index, but not a name.  ACPI tables can define extra GPIO info using
a method with the special ACPI "daffd814-6eba-4d8c-8a91-bc9bbf4aa301"
UUID which is reserved for this, but I'm not aware of any devices where
the ACPI tables actually use this.

So all x86 drivers which lookup GPIOs from ACPI tables need to manually
add a mapping by calling devm_acpi_dev_add_driver_gpios(). Ideally the
Windows driver mandates a fixed order in which the GPIOs must be put
in the _CRS method and all we need a single acpi_gpio_mapping in the
driver.

But in some cases, like this case the order is not fixed and we need
some heuristics to figure out the right order and we have multiple
acpi_gpio_mapping-s and select one to pass to devm_acpi_dev_add_driver_gpios()
based on heuristics. That is what happening here. These heuristics
are tyically different per driver, so this is not really something
which we can share between drivers. The only other case which I'm aware
of which is doing something similar (but not identical) is the bcm
bluetooth code in drivers/bluetooth/hci_bcm.c, starting around line 880.

TL;DR: at this point in time I do not see a more generic way to do this.

> This portion could do with being split off, if we were ever to get that
> more generic solution.
> 
>> +{
>> +	const struct acpi_gpio_mapping *gpio_mapping = NULL;
>> +	struct device *dev = &ts->client->dev;
>> +	LIST_HEAD(resources);
>> +	int ret;
>> +
>> +	ts->gpio_count = 0;
>> +	ts->gpio_int_idx = -1;
>> +	ret = acpi_dev_get_resources(ACPI_COMPANION(dev), &resources,
>> +				     goodix_resource, ts);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Error getting ACPI resources: %d\n",
>> ret);
>> +		return ret;
>> +	}
>> +
>> +	acpi_dev_free_resource_list(&resources);
>> +
>> +	if (ts->gpio_count == 2 && ts->gpio_int_idx == 0) {
>> +		ts->irq_pin_access_method = irq_pin_access_acpi_gpio;
>> +		gpio_mapping = acpi_goodix_int_first_gpios;
>> +	} else if (ts->gpio_count == 2 && ts->gpio_int_idx == 1) {
>> +		ts->irq_pin_access_method = irq_pin_access_acpi_gpio;
>> +		gpio_mapping = acpi_goodix_int_last_gpios;
>> +	} else {
>> +		dev_warn(dev, "Unexpected ACPI resources: gpio_count
>> %d, gpio_int_idx %d\n",
>> +			 ts->gpio_count, ts->gpio_int_idx);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return devm_acpi_dev_add_driver_gpios(dev, gpio_mapping);
>> +}
>> +#else
>> +static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts)
>> +{
>> +	return -EINVAL;
>> +}
>> +#endif /* CONFIG_X86 && CONFIG_ACPI */
>> +
>>   /**
>>    * goodix_get_gpio_config - Get GPIO config from ACPI/DT
>>    *
>> @@ -609,6 +700,7 @@ static int goodix_get_gpio_config(struct
>> goodix_ts_data *ts)
>>   	int error;
>>   	struct device *dev;
>>   	struct gpio_desc *gpiod;
>> +	bool added_acpi_mappings = false;
>>   
>>   	if (!ts->client)
>>   		return -EINVAL;
>> @@ -632,6 +724,7 @@ static int goodix_get_gpio_config(struct
>> goodix_ts_data *ts)
>>   		return error;
>>   	}
>>   
>> +retry_get_irq_gpio:
>>   	/* Get the interrupt GPIO pin number */
>>   	gpiod = devm_gpiod_get_optional(dev, GOODIX_GPIO_INT_NAME,
>> GPIOD_IN);
>>   	if (IS_ERR(gpiod)) {
>> @@ -641,6 +734,11 @@ static int goodix_get_gpio_config(struct
>> goodix_ts_data *ts)
>>   				GOODIX_GPIO_INT_NAME, error);
>>   		return error;
>>   	}
>> +	if (!gpiod && has_acpi_companion(dev) && !added_acpi_mappings)
>> {
>> +		added_acpi_mappings = true;
> 
> Does this mean we retry at most once?

Yes, we are not really "retrying", we are doing a 2 step
probe:

1) First try to get the GPIOs without having done our heuristics and
without having called devm_acpi_dev_add_driver_gpios(). This is for
ACPI platforms extra GPIO info (including names) using the special
ACPI "daffd814-6eba-4d8c-8a91-bc9bbf4aa301" UUID method.

2) If this fails then we add our own name to index mappings and
get the GPIOs using those.


>> +		if (goodix_add_acpi_gpio_mappings(ts) == 0)
>> +			goto retry_get_irq_gpio;
>> +	}
>>   
>>   	ts->gpiod_int = gpiod;
>>   
>> @@ -656,10 +754,17 @@ static int goodix_get_gpio_config(struct
>> goodix_ts_data *ts)
>>   
>>   	ts->gpiod_rst = gpiod;
>>   
>> -	if (ts->gpiod_int && ts->gpiod_rst) {
>> -		ts->reset_controller_at_probe = true;
>> -		ts->load_cfg_from_disk = true;
>> -		ts->irq_pin_access_method = irq_pin_access_gpio;
>> +	switch (ts->irq_pin_access_method) {
> 
> Can't say I like switch statements with only 2 cases...

More cases are added in later patches.

> 
>> +	case irq_pin_access_acpi_gpio:
> 
> Can you add a comment that explains that this is to fallback in case we
> didn't manage to get the ACPI mappings?

Will do for v2 if this patch series.

> 
>> +		if (!ts->gpiod_int || !ts->gpiod_rst)
>> +			ts->irq_pin_access_method =
>> irq_pin_access_none;
>> +		break;
>> +	default:
>> +		if (ts->gpiod_int && ts->gpiod_rst) {
>> +			ts->reset_controller_at_probe = true;
>> +			ts->load_cfg_from_disk = true;
>> +			ts->irq_pin_access_method =
>> irq_pin_access_gpio;
>> +		}
>>   	}
>>   
>>   	return 0;
> 

Regards,

Hans

Re: [PATCH resend 04/10] Input: goodix - Add support for getting IRQ + reset GPIOs on Cherry Trail devices

[Bastien Nocera]

On Mon, 2020-03-02 at 16:40 +0100, Hans de Goede wrote:
> > Does this mean we retry at most once?
> 
> 
> Yes, we are not really "retrying", we are doing a 2 step
> 
> probe:
> 
> 
> 
> 1) First try to get the GPIOs without having done our heuristics and
> 
> without having called devm_acpi_dev_add_driver_gpios(). This is for
> 
> ACPI platforms extra GPIO info (including names) using the special
> 
> ACPI "daffd814-6eba-4d8c-8a91-bc9bbf4aa301" UUID method.
> 
> 
> 
> 2) If this fails then we add our own name to index mappings and
> 
> get the GPIOs using those.

Is there a better way to communicate that? Using a separate function
for that piece of code, and maybe some comments to clarify what it's
doing.

Thanks for the other explanations.

Re: [PATCH resend 04/10] Input: goodix - Add support for getting IRQ + reset GPIOs on Cherry Trail devices

[Hans de Goede]

Hi,

On 3/2/20 4:44 PM, Bastien Nocera wrote:
> On Mon, 2020-03-02 at 16:40 +0100, Hans de Goede wrote:
>>> Does this mean we retry at most once?
>>
>>
>> Yes, we are not really "retrying", we are doing a 2 step
>>
>> probe:
>>
>>
>>
>> 1) First try to get the GPIOs without having done our heuristics and
>>
>> without having called devm_acpi_dev_add_driver_gpios(). This is for
>>
>> ACPI platforms extra GPIO info (including names) using the special
>>
>> ACPI "daffd814-6eba-4d8c-8a91-bc9bbf4aa301" UUID method.
>>
>>
>>
>> 2) If this fails then we add our own name to index mappings and
>>
>> get the GPIOs using those.
> 
> Is there a better way to communicate that? Using a separate function
> for that piece of code,

The code adding our own mappings already is in a separate function, that
is what the goodix_add_acpi_gpio_mappings function is for.

> and maybe some comments to clarify what it's
> doing.

I will add a comment above the goodix_add_acpi_gpio_mappings function
explaining that it is used to add our own mappings if the ACPI
tables do not contain GPIO-name to ACPI resource index mappings.

Regards,

Hans

Re: [PATCH resend 08/10] Input: goodix - Save a copy of the config from goodix_read_config()

[Hans de Goede]

Hi,

On 3/2/20 12:30 PM, Bastien Nocera wrote:
> On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
>> Save a copy of the config in goodix_read_config(), this is a
>> preparation
>> patch for restoring the config if it was lost after a supend/resume
>> cycle.
> 
> Is "fix" really the best verb for this "calculate checksum"?

Good point, I've renamed fix_config to calc_config_checksum for v2.

>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
>> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
>> Cc: Dmitry Mastykin <mastichi@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Looks fine apart from the nitpicks.
> 
> Reviewed-by: Bastien Nocera <hadess@hadess.net>
> 
>> ---
>>   drivers/input/touchscreen/goodix.c | 51 ++++++++++++++++++++++++++
>> ----
>>   1 file changed, 45 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/goodix.c
>> b/drivers/input/touchscreen/goodix.c
>> index 66d6bb74507d..21be33384d14 100644
>> --- a/drivers/input/touchscreen/goodix.c
>> +++ b/drivers/input/touchscreen/goodix.c
>> @@ -72,6 +72,7 @@ struct goodix_chip_data {
>>   	u16 config_addr;
>>   	int config_len;
>>   	int (*check_config)(struct goodix_ts_data *, const struct
>> firmware *);
>> +	void (*fix_config)(struct goodix_ts_data *ts);
>>   };
>>   
>>   struct goodix_ts_data {
>> @@ -96,35 +97,42 @@ struct goodix_ts_data {
>>   	unsigned long irq_flags;
>>   	enum goodix_irq_pin_access_method irq_pin_access_method;
>>   	unsigned int contact_size;
>> +	u8 config[GOODIX_CONFIG_MAX_LENGTH];
>>   };
>>   
>>   static int goodix_check_cfg_8(struct goodix_ts_data *ts,
>>   			const struct firmware *cfg);
>>   static int goodix_check_cfg_16(struct goodix_ts_data *ts,
>>   			const struct firmware *cfg);
>> +static void goodix_fix_cfg_8(struct goodix_ts_data *ts);
>> +static void goodix_fix_cfg_16(struct goodix_ts_data *ts);
>>   
>>   static const struct goodix_chip_data gt1x_chip_data = {
>>   	.config_addr		= GOODIX_GT1X_REG_CONFIG_DATA,
>>   	.config_len		= GOODIX_CONFIG_MAX_LENGTH,
>>   	.check_config		= goodix_check_cfg_16,
>> +	.fix_config		= goodix_fix_cfg_16,
>>   };
>>   
>>   static const struct goodix_chip_data gt911_chip_data = {
>>   	.config_addr		= GOODIX_GT9X_REG_CONFIG_DATA,
>>   	.config_len		= GOODIX_CONFIG_911_LENGTH,
>>   	.check_config		= goodix_check_cfg_8,
>> +	.fix_config		= goodix_fix_cfg_8,
>>   };
>>   
>>   static const struct goodix_chip_data gt967_chip_data = {
>>   	.config_addr		= GOODIX_GT9X_REG_CONFIG_DATA,
>>   	.config_len		= GOODIX_CONFIG_967_LENGTH,
>>   	.check_config		= goodix_check_cfg_8,
>> +	.fix_config		= goodix_fix_cfg_8,
>>   };
>>   
>>   static const struct goodix_chip_data gt9x_chip_data = {
>>   	.config_addr		= GOODIX_GT9X_REG_CONFIG_DATA,
>>   	.config_len		= GOODIX_CONFIG_MAX_LENGTH,
>>   	.check_config		= goodix_check_cfg_8,
>> +	.fix_config		= goodix_fix_cfg_8,
>>   };
>>   
>>   static const unsigned long goodix_irq_flags[] = {
>> @@ -442,6 +450,19 @@ static int goodix_check_cfg_8(struct
>> goodix_ts_data *ts,
>>   	return 0;
>>   }
>>   
>> +static void goodix_fix_cfg_8(struct goodix_ts_data *ts)
>> +{
>> +	int i, raw_cfg_len = ts->chip->config_len - 2;
>> +	u8 check_sum = 0;
>> +
>> +	for (i = 0; i < raw_cfg_len; i++)
>> +		check_sum += ts->config[i];
>> +	check_sum = (~check_sum) + 1;
>> +
>> +	ts->config[raw_cfg_len] = check_sum;
>> +	ts->config[raw_cfg_len + 1] = 1;
> 
> Would be good to mention that this is the "config_fresh" bit, in this
> function and others.

Will fix for v2.

> 
>> +}
>> +
>>   static int goodix_check_cfg_16(struct goodix_ts_data *ts,
>>   			const struct firmware *cfg)
>>   {
>> @@ -466,6 +487,19 @@ static int goodix_check_cfg_16(struct
>> goodix_ts_data *ts,
>>   	return 0;
>>   }
>>   
>> +static void goodix_fix_cfg_16(struct goodix_ts_data *ts)
>> +{
>> +	int i, raw_cfg_len = ts->chip->config_len - 3;
>> +	u16 check_sum = 0;
>> +
>> +	for (i = 0; i < raw_cfg_len; i += 2)
>> +		check_sum += get_unaligned_be16(&ts->config[i]);
>> +	check_sum = (~check_sum) + 1;
>> +
>> +	put_unaligned_be16(check_sum, &ts->config[raw_cfg_len]);
>> +	ts->config[raw_cfg_len + 2] = 1;
>> +}
>> +
>>   /**
>>    * goodix_check_cfg - Checks if config fw is valid
>>    *
>> @@ -828,12 +862,11 @@ static int goodix_get_gpio_config(struct
>> goodix_ts_data *ts)
>>    */
>>   static void goodix_read_config(struct goodix_ts_data *ts)
>>   {
>> -	u8 config[GOODIX_CONFIG_MAX_LENGTH];
>>   	int x_max, y_max;
>>   	int error;
>>   
>>   	error = goodix_i2c_read(ts->client, ts->chip->config_addr,
>> -				config, ts->chip->config_len);
>> +				ts->config, ts->chip->config_len);
>>   	if (error) {
>>   		dev_warn(&ts->client->dev, "Error reading config:
>> %d\n",
>>   			 error);
>> @@ -842,15 +875,21 @@ static void goodix_read_config(struct
>> goodix_ts_data *ts)
>>   		return;
>>   	}
>>   
>> -	ts->int_trigger_type = config[TRIGGER_LOC] & 0x03;
>> -	ts->max_touch_num = config[MAX_CONTACTS_LOC] & 0x0f;
>> +	ts->int_trigger_type = ts->config[TRIGGER_LOC] & 0x03;
>> +	ts->max_touch_num = ts->config[MAX_CONTACTS_LOC] & 0x0f;
>>   
>> -	x_max = get_unaligned_le16(&config[RESOLUTION_LOC]);
>> -	y_max = get_unaligned_le16(&config[RESOLUTION_LOC + 2]);
>> +	x_max = get_unaligned_le16(&ts->config[RESOLUTION_LOC]);
>> +	y_max = get_unaligned_le16(&ts->config[RESOLUTION_LOC + 2]);
>>   	if (x_max && y_max) {
>>   		input_abs_set_max(ts->input_dev, ABS_MT_POSITION_X,
>> x_max - 1);
>>   		input_abs_set_max(ts->input_dev, ABS_MT_POSITION_Y,
>> y_max - 1);
>>   	}
>> +
>> +	/*
>> +	 * Ensure valid checksum and config_fresh bit being set for
>> possible
>> +	 * re-upload of config after suspend/resume.
>> +	 */
> 
> I think that the explanation should ultimately be shorter if the vfunc
> was better named.
> 
>> +	ts->chip->fix_config(ts);
>>   }
>>   
>>   /**
> 


Regards,

Hans

Re: [PATCH resend 09/10] Input: goodix - Make goodix_send_cfg() take a raw buffer as argument

[Hans de Goede]

Hi,

On 3/2/20 12:33 PM, Bastien Nocera wrote:
> On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
>> Make goodix_send_cfg() take a raw buffer as argument instead of a
>> struct firmware *cfg, so that it can also be used to restore the
>> config
>> on resume if necessary.
>>
>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
>> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
>> Cc: Dmitry Mastykin <mastichi@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/input/touchscreen/goodix.c | 46 ++++++++++++++------------
>> ----
>>   1 file changed, 21 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/goodix.c
>> b/drivers/input/touchscreen/goodix.c
>> index 21be33384d14..0f39c499e3a9 100644
>> --- a/drivers/input/touchscreen/goodix.c
>> +++ b/drivers/input/touchscreen/goodix.c
>> @@ -71,7 +71,7 @@ enum goodix_irq_pin_access_method {
>>   struct goodix_chip_data {
>>   	u16 config_addr;
>>   	int config_len;
>> -	int (*check_config)(struct goodix_ts_data *, const struct
>> firmware *);
>> +	int (*check_config)(struct goodix_ts_data *ts, const u8 *cfg,
>> int len);
> 
> Any way to make the length a uint instead of an int? That way, we
> wouldn't need to add < 0 guards, and the "len > MAX_LENGTH" check would
> be enough.

Actually the code does things like:

	int raw_cfg_len = len - 3;

         for (i = 0; i < raw_cfg_len; i += 2)
                 check_sum += get_unaligned_be16(&ts->config[i]);
	check_sum = (~check_sum) + 1;
	if (check_sum != get_unaligned_be16(&cfg[raw_cfg_len])) {

Which is bad without a minimum check regardless of us using signed
or unsigned ints here. unsigned ints will short-circuit / skip the for
loop when len < 3, but then we end up with a negative array index when
doing: &cfg[raw_cfg_len]. And when going unsigned then both the loop
and the array index will be out of bounds when len < 3. So the proper
fix here is to add a minimum check.

I will add an extra patch to the patch-set adding a minimum bounds check.

Regards,

Hans

	


> 
> Looks good otherwise:
> 
> Reviewed-by: Bastien Nocera <hadess@hadess.net>
> 
>>   	void (*fix_config)(struct goodix_ts_data *ts);
>>   };
>>   
>> @@ -101,9 +101,9 @@ struct goodix_ts_data {
>>   };
>>   
>>   static int goodix_check_cfg_8(struct goodix_ts_data *ts,
>> -			const struct firmware *cfg);
>> +			      const u8 *cfg, int len);
>>   static int goodix_check_cfg_16(struct goodix_ts_data *ts,
>> -			const struct firmware *cfg);
>> +			       const u8 *cfg, int len);
>>   static void goodix_fix_cfg_8(struct goodix_ts_data *ts);
>>   static void goodix_fix_cfg_16(struct goodix_ts_data *ts);
>>   
>> @@ -426,22 +426,21 @@ static int goodix_request_irq(struct
>> goodix_ts_data *ts)
>>   					 ts->irq_flags, ts->client-
>>> name, ts);
>>   }
>>   
>> -static int goodix_check_cfg_8(struct goodix_ts_data *ts,
>> -			const struct firmware *cfg)
>> +static int goodix_check_cfg_8(struct goodix_ts_data *ts, const u8
>> *cfg, int len)
>>   {
>> -	int i, raw_cfg_len = cfg->size - 2;
>> +	int i, raw_cfg_len = len - 2;
>>   	u8 check_sum = 0;
>>   
>>   	for (i = 0; i < raw_cfg_len; i++)
>> -		check_sum += cfg->data[i];
>> +		check_sum += cfg[i];
>>   	check_sum = (~check_sum) + 1;
>> -	if (check_sum != cfg->data[raw_cfg_len]) {
>> +	if (check_sum != cfg[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) {
>> +	if (cfg[raw_cfg_len + 1] != 1) {
>>   		dev_err(&ts->client->dev,
>>   			"Config fw must have Config_Fresh register
>> set");
>>   		return -EINVAL;
>> @@ -463,22 +462,22 @@ static void goodix_fix_cfg_8(struct
>> goodix_ts_data *ts)
>>   	ts->config[raw_cfg_len + 1] = 1;
>>   }
>>   
>> -static int goodix_check_cfg_16(struct goodix_ts_data *ts,
>> -			const struct firmware *cfg)
>> +static int goodix_check_cfg_16(struct goodix_ts_data *ts, const u8
>> *cfg,
>> +			       int len)
>>   {
>> -	int i, raw_cfg_len = cfg->size - 3;
>> +	int i, raw_cfg_len = len - 3;
>>   	u16 check_sum = 0;
>>   
>>   	for (i = 0; i < raw_cfg_len; i += 2)
>> -		check_sum += get_unaligned_be16(&cfg->data[i]);
>> +		check_sum += get_unaligned_be16(&cfg[i]);
>>   	check_sum = (~check_sum) + 1;
>> -	if (check_sum != get_unaligned_be16(&cfg->data[raw_cfg_len])) {
>> +	if (check_sum != get_unaligned_be16(&cfg[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 + 2] != 1) {
>> +	if (cfg[raw_cfg_len + 2] != 1) {
>>   		dev_err(&ts->client->dev,
>>   			"Config fw must have Config_Fresh register
>> set");
>>   		return -EINVAL;
>> @@ -506,16 +505,15 @@ static void goodix_fix_cfg_16(struct
>> goodix_ts_data *ts)
>>    * @ts: goodix_ts_data pointer
>>    * @cfg: firmware config data
>>    */
>> -static int goodix_check_cfg(struct goodix_ts_data *ts,
>> -			    const struct firmware *cfg)
>> +static int goodix_check_cfg(struct goodix_ts_data *ts, const u8
>> *cfg, int len)
>>   {
>> -	if (cfg->size > GOODIX_CONFIG_MAX_LENGTH) {
>> +	if (len > GOODIX_CONFIG_MAX_LENGTH) {
>>   		dev_err(&ts->client->dev,
>>   			"The length of the config fw is not correct");
>>   		return -EINVAL;
>>   	}
>>   
>> -	return ts->chip->check_config(ts, cfg);
>> +	return ts->chip->check_config(ts, cfg, len);
>>   }
>>   
>>   /**
>> @@ -524,17 +522,15 @@ static int goodix_check_cfg(struct
>> goodix_ts_data *ts,
>>    * @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)
>> +static int goodix_send_cfg(struct goodix_ts_data *ts, const u8 *cfg,
>> int len)
>>   {
>>   	int error;
>>   
>> -	error = goodix_check_cfg(ts, cfg);
>> +	error = goodix_check_cfg(ts, cfg, len);
>>   	if (error)
>>   		return error;
>>   
>> -	error = goodix_i2c_write(ts->client, ts->chip->config_addr,
>> cfg->data,
>> -				 cfg->size);
>> +	error = goodix_i2c_write(ts->client, ts->chip->config_addr,
>> cfg, len);
>>   	if (error) {
>>   		dev_err(&ts->client->dev, "Failed to write config data:
>> %d",
>>   			error);
>> @@ -1058,7 +1054,7 @@ static void goodix_config_cb(const struct
>> firmware *cfg, void *ctx)
>>   
>>   	if (cfg) {
>>   		/* send device configuration to the firmware */
>> -		error = goodix_send_cfg(ts, cfg);
>> +		error = goodix_send_cfg(ts, cfg->data, cfg->size);
>>   		if (error)
>>   			goto err_release_cfg;
>>   	}
> 

Re: [PATCH resend 10/10] Input: goodix - Restore config on resume if necessary

[Hans de Goede]

Hi,

On 3/2/20 12:35 PM, Bastien Nocera wrote:
> On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
>> Some devices, e.g the Trekstor Primetab S11B, loose there config over
> 
> "lose".
> 
>> a suspend/resume cycle (likely the controller looses power during
> 
> "loses".
> 
>> suspend).
>>
>> This commit reads back the config version on resume and if matches
>> the
>> expected config version it resets the controller and resends the
>> config
>> we read back and saved at probe time.
>>
>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
>> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
>> Cc: Dmitry Mastykin <mastichi@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Looks fine apart from the nitpicks.
> 
> Reviewed-by: Bastien Nocera <hadess@hadess.net>
> 
>> ---
>>   drivers/input/touchscreen/goodix.c | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/goodix.c
>> b/drivers/input/touchscreen/goodix.c
>> index 0f39c499e3a9..389d3e044f97 100644
>> --- a/drivers/input/touchscreen/goodix.c
>> +++ b/drivers/input/touchscreen/goodix.c
>> @@ -1232,6 +1232,7 @@ 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);
>> +	u8 config_ver;
>>   	int error;
>>   
>>   	if (ts->irq_pin_access_method == irq_pin_access_none) {
>> @@ -1253,6 +1254,27 @@ static int __maybe_unused goodix_resume(struct
>> device *dev)
>>   	if (error)
>>   		return error;
>>   
>> +	error = goodix_i2c_read(ts->client, ts->chip->config_addr,
>> +				&config_ver, 1);
>> +	if (error)
>> +		dev_warn(dev, "Error reading config version: %d,
>> resetting controller\n",
>> +			 error);
>> +	else if (config_ver != ts->config[0])
>> +		dev_warn(dev, "Config version mismatch %d != %d,
>> resetting controller\n",
>> +			 config_ver, ts->config[0]);
> 
> Should it really be a warning if it happens regularly?

Good point, I've changed this to a dev_info for v2.

Regards,

Hans



> 
>> +
>> +	if (error != 0 || config_ver != ts->config[0]) {
>> +		error = goodix_reset(ts);
>> +		if (error) {
>> +			dev_err(dev, "Controller reset failed.\n");
>> +			return error;
>> +		}
>> +
>> +		error = goodix_send_cfg(ts, ts->config, ts->chip-
>>> config_len);
>> +		if (error)
>> +			return error;
>> +	}
>> +
>>   	error = goodix_request_irq(ts);
>>   	if (error)
>>   		return error;
>