[PATCH 1/4] Input: icn8318 - Move of specific probe code to a helper function

[PATCH 1/4] Input: icn8318 - Move of specific probe code to a helper function

[Hans de Goede]

This is a preparation patch for adding support for ACPI enumerated
Chipone touchscreens. On ACPI platforms the wake GPIO is unused, move
the code to get the GPIO to a new icn8318_probe_of helper function.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/chipone_icn8318.c | 50 ++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/drivers/input/touchscreen/chipone_icn8318.c b/drivers/input/touchscreen/chipone_icn8318.c
index 0bf14067c167..ddaae91f02fc 100644
--- a/drivers/input/touchscreen/chipone_icn8318.c
+++ b/drivers/input/touchscreen/chipone_icn8318.c
@@ -137,7 +137,8 @@ static int icn8318_start(struct input_dev *dev)
 	struct icn8318_data *data = input_get_drvdata(dev);
 
 	enable_irq(data->client->irq);
-	gpiod_set_value_cansleep(data->wake_gpio, 1);
+	if (data->wake_gpio)
+		gpiod_set_value_cansleep(data->wake_gpio, 1);
 
 	return 0;
 }
@@ -149,7 +150,8 @@ static void icn8318_stop(struct input_dev *dev)
 	disable_irq(data->client->irq);
 	i2c_smbus_write_byte_data(data->client, ICN8318_REG_POWER,
 				  ICN8318_POWER_HIBERNATE);
-	gpiod_set_value_cansleep(data->wake_gpio, 0);
+	if (data->wake_gpio)
+		gpiod_set_value_cansleep(data->wake_gpio, 0);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -180,8 +182,29 @@ static int icn8318_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(icn8318_pm_ops, icn8318_suspend, icn8318_resume);
 
-static int icn8318_probe(struct i2c_client *client,
-			 const struct i2c_device_id *id)
+#ifdef CONFIG_OF
+static int icn8318_probe_of(struct icn8318_data *data, struct device *dev)
+{
+	int error;
+
+	data->wake_gpio = devm_gpiod_get(dev, "wake", GPIOD_OUT_LOW);
+	if (IS_ERR(data->wake_gpio)) {
+		error = PTR_ERR(data->wake_gpio);
+		if (error != -EPROBE_DEFER)
+			dev_err(dev, "Error getting wake gpio: %d\n", error);
+		return error;
+	}
+
+	return 0;
+}
+#else
+static int icn8318_probe_of(struct icn8318_data *data, struct device *dev)
+{
+	return -ENODEV;
+}
+#endif
+
+static int icn8318_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct icn8318_data *data;
@@ -197,18 +220,13 @@ static int icn8318_probe(struct i2c_client *client,
 	if (!data)
 		return -ENOMEM;
 
-	data->wake_gpio = devm_gpiod_get(dev, "wake", GPIOD_OUT_LOW);
-	if (IS_ERR(data->wake_gpio)) {
-		error = PTR_ERR(data->wake_gpio);
-		if (error != -EPROBE_DEFER)
-			dev_err(dev, "Error getting wake gpio: %d\n", error);
-		return error;
-	}
-
 	input = devm_input_allocate_device(dev);
 	if (!input)
 		return -ENOMEM;
 
+	data->client = client;
+	data->input = input;
+
 	input->name = client->name;
 	input->id.bustype = BUS_I2C;
 	input->open = icn8318_start;
@@ -218,6 +236,10 @@ static int icn8318_probe(struct i2c_client *client,
 	input_set_capability(input, EV_ABS, ABS_MT_POSITION_X);
 	input_set_capability(input, EV_ABS, ABS_MT_POSITION_Y);
 
+	error = icn8318_probe_of(data, dev);
+	if (error)
+		return error;
+
 	touchscreen_parse_properties(input, true, &data->prop);
 	if (!input_abs_get_max(input, ABS_MT_POSITION_X) ||
 	    !input_abs_get_max(input, ABS_MT_POSITION_Y)) {
@@ -230,8 +252,6 @@ static int icn8318_probe(struct i2c_client *client,
 	if (error)
 		return error;
 
-	data->client = client;
-	data->input = input;
 	input_set_drvdata(input, data);
 
 	error = devm_request_threaded_irq(dev, client->irq, NULL, icn8318_irq,
@@ -271,7 +291,7 @@ static struct i2c_driver icn8318_driver = {
 		.pm	= &icn8318_pm_ops,
 		.of_match_table = icn8318_of_match,
 	},
-	.probe = icn8318_probe,
+	.probe_new = icn8318_probe,
 	.id_table = icn8318_i2c_id,
 };
 
-- 
2.13.0

[PATCH 2/4] Input: icn8318 - Add support for icn8505

[Hans de Goede]

The icn8505 variant found on some x86 tablets has almost the same protocol
as the 8318, protocol wise there are only 2 small differences:

1) The 8505 uses 16 bit register addresses and has a different register
address for the location of the touch data.
2) The 8505 coordinates are in little-endian format instead of in be.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../bindings/input/touchscreen/chipone_icn8318.txt |  2 +-
 drivers/input/touchscreen/Kconfig                  |  3 +-
 drivers/input/touchscreen/chipone_icn8318.c        | 53 +++++++++++++++++-----
 3 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/chipone_icn8318.txt b/Documentation/devicetree/bindings/input/touchscreen/chipone_icn8318.txt
index d11f8d615b5d..9fdd80749386 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/chipone_icn8318.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/chipone_icn8318.txt
@@ -1,7 +1,7 @@
 * ChipOne icn8318 I2C touchscreen controller
 
 Required properties:
- - compatible		  : "chipone,icn8318"
+ - compatible		  : "chipone,icn8318" or "chipone,icn8505"
  - reg			  : I2C slave address of the chip (0x40)
  - interrupt-parent	  : a phandle pointing to the interrupt controller
 			    serving the interrupt for this chip
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index cf26ca49ae6d..fff1467506e7 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -157,7 +157,8 @@ config TOUCHSCREEN_CHIPONE_ICN8318
 	depends on I2C
 	depends on OF
 	help
-	  Say Y here if you have a ChipOne icn8318 based I2C touchscreen.
+	  Say Y here if you have a ChipOne icn8318 or icn8505 based
+	  I2C touchscreen.
 
 	  If unsure, say N.
 
diff --git a/drivers/input/touchscreen/chipone_icn8318.c b/drivers/input/touchscreen/chipone_icn8318.c
index ddaae91f02fc..993df804883f 100644
--- a/drivers/input/touchscreen/chipone_icn8318.c
+++ b/drivers/input/touchscreen/chipone_icn8318.c
@@ -1,7 +1,7 @@
 /*
  * Driver for ChipOne icn8318 i2c touchscreen controller
  *
- * Copyright (c) 2015 Red Hat Inc.
+ * Copyright (c) 2015-2017 Red Hat Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -20,6 +20,7 @@
 #include <linux/input/touchscreen.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 
 #define ICN8318_REG_POWER		4
 #define ICN8318_REG_TOUCHDATA		16
@@ -30,6 +31,13 @@
 
 #define ICN8318_MAX_TOUCHES		5
 
+#define ICN8505_REG_TOUCHDATA		0x1000
+
+enum icn8318_model {
+	ICN8318,
+	ICN8505,
+};
+
 struct icn8318_touch {
 	__u8 slot;
 	__be16 x;
@@ -54,27 +62,36 @@ struct icn8318_data {
 	struct input_dev *input;
 	struct gpio_desc *wake_gpio;
 	struct touchscreen_properties prop;
+	enum icn8318_model model;
 };
 
-static int icn8318_read_touch_data(struct i2c_client *client,
+static int icn8318_read_touch_data(struct icn8318_data *data,
 				   struct icn8318_touch_data *touch_data)
 {
-	u8 reg = ICN8318_REG_TOUCHDATA;
+	u8 reg[2];
 	struct i2c_msg msg[2] = {
 		{
-			.addr = client->addr,
-			.len = 1,
-			.buf = &reg
+			.addr = data->client->addr,
+			.buf = reg
 		},
 		{
-			.addr = client->addr,
+			.addr = data->client->addr,
 			.flags = I2C_M_RD,
 			.len = sizeof(struct icn8318_touch_data),
 			.buf = (u8 *)touch_data
 		}
 	};
 
-	return i2c_transfer(client->adapter, msg, 2);
+	if (data->model == ICN8318) {
+		reg[0] = ICN8318_REG_TOUCHDATA;
+		msg[0].len = 1;
+	} else {
+		reg[0] = ICN8505_REG_TOUCHDATA >> 8;
+		reg[1] = ICN8505_REG_TOUCHDATA & 0xff;
+		msg[0].len = 2;
+	}
+
+	return i2c_transfer(data->client->adapter, msg, 2);
 }
 
 static inline bool icn8318_touch_active(u8 event)
@@ -83,6 +100,14 @@ static inline bool icn8318_touch_active(u8 event)
 	       (event == ICN8318_EVENT_UPDATE2);
 }
 
+static u16 icn8318_coord_to_cpu(struct icn8318_data *data, __be16 coord)
+{
+	if (data->model == ICN8318)
+		return be16_to_cpu(coord);
+	else
+		return le16_to_cpu((__force __le16)coord);
+}
+
 static irqreturn_t icn8318_irq(int irq, void *dev_id)
 {
 	struct icn8318_data *data = dev_id;
@@ -90,7 +115,7 @@ static irqreturn_t icn8318_irq(int irq, void *dev_id)
 	struct icn8318_touch_data touch_data;
 	int i, ret;
 
-	ret = icn8318_read_touch_data(data->client, &touch_data);
+	ret = icn8318_read_touch_data(data, &touch_data);
 	if (ret < 0) {
 		dev_err(dev, "Error reading touch data: %d\n", ret);
 		return IRQ_HANDLED;
@@ -122,8 +147,9 @@ static irqreturn_t icn8318_irq(int irq, void *dev_id)
 			continue;
 
 		touchscreen_report_pos(data->input, &data->prop,
-				       be16_to_cpu(touch->x),
-				       be16_to_cpu(touch->y), true);
+				       icn8318_coord_to_cpu(data, touch->x),
+				       icn8318_coord_to_cpu(data, touch->y),
+				       true);
 	}
 
 	input_mt_sync_frame(data->input);
@@ -187,6 +213,8 @@ static int icn8318_probe_of(struct icn8318_data *data, struct device *dev)
 {
 	int error;
 
+	data->model = (long)of_device_get_match_data(dev);
+
 	data->wake_gpio = devm_gpiod_get(dev, "wake", GPIOD_OUT_LOW);
 	if (IS_ERR(data->wake_gpio)) {
 		error = PTR_ERR(data->wake_gpio);
@@ -274,7 +302,8 @@ static int icn8318_probe(struct i2c_client *client)
 }
 
 static const struct of_device_id icn8318_of_match[] = {
-	{ .compatible = "chipone,icn8318" },
+	{ .compatible = "chipone,icn8318", .data = (void *)ICN8318 },
+	{ .compatible = "chipone,icn8505", .data = (void *)ICN8505 },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, icn8318_of_match);
-- 
2.13.0

[PATCH 3/4] Input: icn8318 - Add support for ACPI enumeration

[Hans de Goede]

The icn8505 variant is found on some x86 tablets, which use ACPI
enumeration, add support for ACPI enumeration.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/Kconfig           |  2 +-
 drivers/input/touchscreen/chipone_icn8318.c | 67 ++++++++++++++++++++++++++++-
 2 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index fff1467506e7..e3b52d356e13 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -155,7 +155,7 @@ config TOUCHSCREEN_CHIPONE_ICN8318
 	tristate "chipone icn8318 touchscreen controller"
 	depends on GPIOLIB || COMPILE_TEST
 	depends on I2C
-	depends on OF
+	depends on OF || ACPI
 	help
 	  Say Y here if you have a ChipOne icn8318 or icn8505 based
 	  I2C touchscreen.
diff --git a/drivers/input/touchscreen/chipone_icn8318.c b/drivers/input/touchscreen/chipone_icn8318.c
index 993df804883f..b209872954f7 100644
--- a/drivers/input/touchscreen/chipone_icn8318.c
+++ b/drivers/input/touchscreen/chipone_icn8318.c
@@ -12,6 +12,7 @@
  * Hans de Goede <hdegoede@redhat.com>
  */
 
+#include <linux/acpi.h>
 #include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/i2c.h>
@@ -232,6 +233,66 @@ static int icn8318_probe_of(struct icn8318_data *data, struct device *dev)
 }
 #endif
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id icn8318_acpi_match[] = {
+	{ "CHPN0001", ICN8505 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, icn8318_acpi_match);
+
+static int icn8318_probe_acpi(struct icn8318_data *data, struct device *dev)
+{
+	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
+	struct input_dev *input = data->input;
+	const struct acpi_device_id *id;
+	struct acpi_device *adev;
+	acpi_status status;
+	const char *sub;
+
+	adev = ACPI_COMPANION(dev);
+	id = acpi_match_device(icn8318_acpi_match, dev);
+	if (!adev || !id)
+		return -ENODEV;
+
+	data->model = id->driver_data;
+
+	/* The _SUB ACPI object describes the touchscreen model */
+	status = acpi_evaluate_object_typed(adev->handle, "_SUB", NULL,
+					    &buf, ACPI_TYPE_STRING);
+	if (ACPI_FAILURE(status)) {
+		dev_err(dev, "ACPI _SUB object not found\n");
+		return -ENODEV;
+	}
+	sub = ((union acpi_object *)buf.pointer)->string.pointer;
+
+	if (strcmp(sub, "HAMP0002") == 0) {
+		input_set_abs_params(input, ABS_MT_POSITION_X, 0, 1199, 0, 0);
+		input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 1919, 0, 0);
+	} else {
+		dev_err(dev, "Unknown model _SUB: %s\n", sub);
+		kfree(buf.pointer);
+		return -ENODEV;
+	}
+	kfree(buf.pointer);
+
+	/*
+	 * Disable ACPI power management the _PS3 method is empty, so
+	 * there is no powersaving when using ACPI power management.
+	 * The _PS0 method resets the controller causing it to loose its
+	 * firmware, which has been loaded by the BIOS and we do not
+	 * know how to restore the firmware.
+	 */
+	adev->flags.power_manageable = 0;
+
+	return 0;
+}
+#else
+static int icn8318_probe_acpi(struct icn8318_data *data, struct device *dev)
+{
+	return -ENODEV;
+}
+#endif
+
 static int icn8318_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -264,7 +325,10 @@ static int icn8318_probe(struct i2c_client *client)
 	input_set_capability(input, EV_ABS, ABS_MT_POSITION_X);
 	input_set_capability(input, EV_ABS, ABS_MT_POSITION_Y);
 
-	error = icn8318_probe_of(data, dev);
+	if (client->dev.of_node)
+		error = icn8318_probe_of(data, dev);
+	else
+		error = icn8318_probe_acpi(data, dev);
 	if (error)
 		return error;
 
@@ -318,6 +382,7 @@ static struct i2c_driver icn8318_driver = {
 	.driver = {
 		.name	= "chipone_icn8318",
 		.pm	= &icn8318_pm_ops,
+		.acpi_match_table = ACPI_PTR(icn8318_acpi_match),
 		.of_match_table = icn8318_of_match,
 	},
 	.probe_new = icn8318_probe,
-- 
2.13.0

[PATCH 4/4] Input: icn8318 - Add support for capacative home button

[Hans de Goede]

Some x86 tablets with an icn8505 touchscreen have a capacative home
button, add support for this.

Note that it turns out the removed comment turns out to be inaccurate,
yes when the capacative home button is pressed the touch info contains
invalid data, but touch_count is set to 0 so this is not a problem and
if both the button is used and a finger is touching the normal
touchscreen area then there is valid touch data.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/chipone_icn8318.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/input/touchscreen/chipone_icn8318.c b/drivers/input/touchscreen/chipone_icn8318.c
index b209872954f7..2209ccf596f3 100644
--- a/drivers/input/touchscreen/chipone_icn8318.c
+++ b/drivers/input/touchscreen/chipone_icn8318.c
@@ -64,6 +64,7 @@ struct icn8318_data {
 	struct gpio_desc *wake_gpio;
 	struct touchscreen_properties prop;
 	enum icn8318_model model;
+	int softbutton;
 };
 
 static int icn8318_read_touch_data(struct icn8318_data *data,
@@ -122,15 +123,9 @@ static irqreturn_t icn8318_irq(int irq, void *dev_id)
 		return IRQ_HANDLED;
 	}
 
-	if (touch_data.softbutton) {
-		/*
-		 * Other data is invalid when a softbutton is pressed.
-		 * This needs some extra devicetree bindings to map the icn8318
-		 * softbutton codes to evdev codes. Currently no known devices
-		 * use this.
-		 */
-		return IRQ_HANDLED;
-	}
+	if (data->softbutton)
+		input_report_key(data->input, data->softbutton,
+				 touch_data.softbutton == 0x01);
 
 	if (touch_data.touch_count > ICN8318_MAX_TOUCHES) {
 		dev_warn(dev, "Too much touches %d > %d\n",
@@ -268,6 +263,7 @@ static int icn8318_probe_acpi(struct icn8318_data *data, struct device *dev)
 	if (strcmp(sub, "HAMP0002") == 0) {
 		input_set_abs_params(input, ABS_MT_POSITION_X, 0, 1199, 0, 0);
 		input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 1919, 0, 0);
+		data->softbutton = KEY_LEFTMETA;
 	} else {
 		dev_err(dev, "Unknown model _SUB: %s\n", sub);
 		kfree(buf.pointer);
@@ -339,6 +335,9 @@ static int icn8318_probe(struct i2c_client *client)
 		return -EINVAL;
 	}
 
+	if (data->softbutton)
+		input_set_capability(data->input, EV_KEY, data->softbutton);
+
 	error = input_mt_init_slots(input, ICN8318_MAX_TOUCHES,
 				    INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
 	if (error)
-- 
2.13.0

Re: [PATCH 3/4] Input: icn8318 - Add support for ACPI enumeration

[Hans de Goede]

Hi,

Self-nack see comment inline.

On 17-06-17 12:58, Hans de Goede wrote:
> The icn8505 variant is found on some x86 tablets, which use ACPI
> enumeration, add support for ACPI enumeration.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/input/touchscreen/Kconfig           |  2 +-
>   drivers/input/touchscreen/chipone_icn8318.c | 67 ++++++++++++++++++++++++++++-
>   2 files changed, 67 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index fff1467506e7..e3b52d356e13 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -155,7 +155,7 @@ config TOUCHSCREEN_CHIPONE_ICN8318
>   	tristate "chipone icn8318 touchscreen controller"
>   	depends on GPIOLIB || COMPILE_TEST
>   	depends on I2C
> -	depends on OF
> +	depends on OF || ACPI
>   	help
>   	  Say Y here if you have a ChipOne icn8318 or icn8505 based
>   	  I2C touchscreen.
> diff --git a/drivers/input/touchscreen/chipone_icn8318.c b/drivers/input/touchscreen/chipone_icn8318.c
> index 993df804883f..b209872954f7 100644
> --- a/drivers/input/touchscreen/chipone_icn8318.c
> +++ b/drivers/input/touchscreen/chipone_icn8318.c
> @@ -12,6 +12,7 @@
>    * Hans de Goede <hdegoede@redhat.com>
>    */
>   
> +#include <linux/acpi.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/interrupt.h>
>   #include <linux/i2c.h>
> @@ -232,6 +233,66 @@ static int icn8318_probe_of(struct icn8318_data *data, struct device *dev)
>   }
>   #endif
>   
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id icn8318_acpi_match[] = {
> +	{ "CHPN0001", ICN8505 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, icn8318_acpi_match);
> +
> +static int icn8318_probe_acpi(struct icn8318_data *data, struct device *dev)
> +{
> +	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
> +	struct input_dev *input = data->input;
> +	const struct acpi_device_id *id;
> +	struct acpi_device *adev;
> +	acpi_status status;
> +	const char *sub;
> +
> +	adev = ACPI_COMPANION(dev);
> +	id = acpi_match_device(icn8318_acpi_match, dev);
> +	if (!adev || !id)
> +		return -ENODEV;
> +
> +	data->model = id->driver_data;
> +
> +	/* The _SUB ACPI object describes the touchscreen model */
> +	status = acpi_evaluate_object_typed(adev->handle, "_SUB", NULL,
> +					    &buf, ACPI_TYPE_STRING);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(dev, "ACPI _SUB object not found\n");
> +		return -ENODEV;
> +	}
> +	sub = ((union acpi_object *)buf.pointer)->string.pointer;
> +
> +	if (strcmp(sub, "HAMP0002") == 0) {
> +		input_set_abs_params(input, ABS_MT_POSITION_X, 0, 1199, 0, 0);
> +		input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 1919, 0, 0);
> +	} else {
> +		dev_err(dev, "Unknown model _SUB: %s\n", sub);
> +		kfree(buf.pointer);
> +		return -ENODEV;
> +	}
> +	kfree(buf.pointer);
> +
> +	/*
> +	 * Disable ACPI power management the _PS3 method is empty, so
> +	 * there is no powersaving when using ACPI power management.
> +	 * The _PS0 method resets the controller causing it to loose its
> +	 * firmware, which has been loaded by the BIOS and we do not
> +	 * know how to restore the firmware.
> +	 */
> +	adev->flags.power_manageable = 0;

Ok, so unfortunately this is not that easy :|  The ACPI node
has a _CID, "PNP0C50" causing i2c-hid to bind, even though the
device has its own device specific protocol. I had i2c-hid
blacklisted for testing, with the plan to add a quirk to it for this
device, but with the quirk if i2c-hid loads earlier then the icn8318
driver and i2c-hid's probe returns -ENODEV due to the quirk, the
device gets turned off by the ACPI core, causing a reset + loss
of the loaded firmware when the icn8313 driver loads. So there
are 2 solutions here:

1) Set adev->flags.power_manageable earlier, somewhere in the
ACPI core

2) Figure out how to load the firmware to make the controller
functional again

Regards,

Hans


> +
> +	return 0;
> +}
> +#else
> +static int icn8318_probe_acpi(struct icn8318_data *data, struct device *dev)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
>   static int icn8318_probe(struct i2c_client *client)
>   {
>   	struct device *dev = &client->dev;
> @@ -264,7 +325,10 @@ static int icn8318_probe(struct i2c_client *client)
>   	input_set_capability(input, EV_ABS, ABS_MT_POSITION_X);
>   	input_set_capability(input, EV_ABS, ABS_MT_POSITION_Y);
>   
> -	error = icn8318_probe_of(data, dev);
> +	if (client->dev.of_node)
> +		error = icn8318_probe_of(data, dev);
> +	else
> +		error = icn8318_probe_acpi(data, dev);
>   	if (error)
>   		return error;
>   
> @@ -318,6 +382,7 @@ static struct i2c_driver icn8318_driver = {
>   	.driver = {
>   		.name	= "chipone_icn8318",
>   		.pm	= &icn8318_pm_ops,
> +		.acpi_match_table = ACPI_PTR(icn8318_acpi_match),
>   		.of_match_table = icn8318_of_match,
>   	},
>   	.probe_new = icn8318_probe,
> 

Re: [PATCH 3/4] Input: icn8318 - Add support for ACPI enumeration

[Hans de Goede]

Hi,

On 17-06-17 22:09, Hans de Goede wrote:
> Hi,
> 
> Self-nack see comment inline.
> 
> On 17-06-17 12:58, Hans de Goede wrote:
>> The icn8505 variant is found on some x86 tablets, which use ACPI
>> enumeration, add support for ACPI enumeration.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/input/touchscreen/Kconfig           |  2 +-
>>   drivers/input/touchscreen/chipone_icn8318.c | 67 ++++++++++++++++++++++++++++-
>>   2 files changed, 67 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index fff1467506e7..e3b52d356e13 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -155,7 +155,7 @@ config TOUCHSCREEN_CHIPONE_ICN8318
>>       tristate "chipone icn8318 touchscreen controller"
>>       depends on GPIOLIB || COMPILE_TEST
>>       depends on I2C
>> -    depends on OF
>> +    depends on OF || ACPI
>>       help
>>         Say Y here if you have a ChipOne icn8318 or icn8505 based
>>         I2C touchscreen.
>> diff --git a/drivers/input/touchscreen/chipone_icn8318.c b/drivers/input/touchscreen/chipone_icn8318.c
>> index 993df804883f..b209872954f7 100644
>> --- a/drivers/input/touchscreen/chipone_icn8318.c
>> +++ b/drivers/input/touchscreen/chipone_icn8318.c
>> @@ -12,6 +12,7 @@
>>    * Hans de Goede <hdegoede@redhat.com>
>>    */
>> +#include <linux/acpi.h>
>>   #include <linux/gpio/consumer.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/i2c.h>
>> @@ -232,6 +233,66 @@ static int icn8318_probe_of(struct icn8318_data *data, struct device *dev)
>>   }
>>   #endif
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id icn8318_acpi_match[] = {
>> +    { "CHPN0001", ICN8505 },
>> +    { }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, icn8318_acpi_match);
>> +
>> +static int icn8318_probe_acpi(struct icn8318_data *data, struct device *dev)
>> +{
>> +    struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
>> +    struct input_dev *input = data->input;
>> +    const struct acpi_device_id *id;
>> +    struct acpi_device *adev;
>> +    acpi_status status;
>> +    const char *sub;
>> +
>> +    adev = ACPI_COMPANION(dev);
>> +    id = acpi_match_device(icn8318_acpi_match, dev);
>> +    if (!adev || !id)
>> +        return -ENODEV;
>> +
>> +    data->model = id->driver_data;
>> +
>> +    /* The _SUB ACPI object describes the touchscreen model */
>> +    status = acpi_evaluate_object_typed(adev->handle, "_SUB", NULL,
>> +                        &buf, ACPI_TYPE_STRING);
>> +    if (ACPI_FAILURE(status)) {
>> +        dev_err(dev, "ACPI _SUB object not found\n");
>> +        return -ENODEV;
>> +    }
>> +    sub = ((union acpi_object *)buf.pointer)->string.pointer;
>> +
>> +    if (strcmp(sub, "HAMP0002") == 0) {
>> +        input_set_abs_params(input, ABS_MT_POSITION_X, 0, 1199, 0, 0);
>> +        input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 1919, 0, 0);
>> +    } else {
>> +        dev_err(dev, "Unknown model _SUB: %s\n", sub);
>> +        kfree(buf.pointer);
>> +        return -ENODEV;
>> +    }
>> +    kfree(buf.pointer);
>> +
>> +    /*
>> +     * Disable ACPI power management the _PS3 method is empty, so
>> +     * there is no powersaving when using ACPI power management.
>> +     * The _PS0 method resets the controller causing it to loose its
>> +     * firmware, which has been loaded by the BIOS and we do not
>> +     * know how to restore the firmware.
>> +     */
>> +    adev->flags.power_manageable = 0;
> 
> Ok, so unfortunately this is not that easy :|  The ACPI node
> has a _CID, "PNP0C50" causing i2c-hid to bind, even though the
> device has its own device specific protocol. I had i2c-hid
> blacklisted for testing, with the plan to add a quirk to it for this
> device, but with the quirk if i2c-hid loads earlier then the icn8318
> driver and i2c-hid's probe returns -ENODEV due to the quirk, the
> device gets turned off by the ACPI core, causing a reset + loss
> of the loaded firmware when the icn8313 driver loads. So there
> are 2 solutions here:
> 
> 1) Set adev->flags.power_manageable earlier, somewhere in the
> ACPI core
> 
> 2) Figure out how to load the firmware to make the controller
> functional again

While working on a 3th option, I did a web-search for "CHPN0001"
instead of for icn8505 which found me this:

https://github.com/Dax89/chuwi-dev/tree/master/drivers/chipone_ts

Which seemed to be based on the same android code as I already
found, but which claims to have firmware loading working again,
so assuming that is true (I've not tested) that gives is us
2 options, either avoid the controller reset, or load the
firmware.

As said I've been working on a 3th option:

3) Still add a quirk to i2c-hid but do the check at module_init
time, rather then add probe time, so that the i2c-hid driver never
registers avoiding the problem of the APCI PS3 / PS0 methods
getting called after the failed i2c-hid probe / before the icn8318
probe.

I've implemented this now and it works well. Although not pretty, it
is only 2 lines of code (and a 5 line block comment), so I hope that
Jiri and Benjamin can live with this.


I personally prefer this 3th option (avoiding controller reset)
over adding firmware upload support for 3 reasons:

1) It is a lot less code it requires the following in i2c-hid:

  static int __init i2c_hid_init(void)
  {
+       /*
+        * The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID
+        * compatible, just probing it puts the device in an unusable state due
+        * to it also have ACPI power management issues.
+        */
+       if (acpi_dev_present("CHPN0001", NULL, -1))
+               return -ENODEV;
+
         return i2c_add_driver(&i2c_hid_driver);
  }

And the following in the icn8318 code:

         /*
          * Disable ACPI power management the _PS3 method is empty, so
          * there is no powersaving when using ACPI power management.
          * The _PS0 method resets the controller causing it to loose its
          * firmware, which has been loaded by the BIOS and we do not
          * know how to restore the firmware.
          */
         adev->flags.power_manageable = 0;

Versus significantly more code to get firmware uploading to work

2) It avoids the need for users to get their controller firmware
from somewhere, it likely is going to be hard to get permission to
add this to linux-firmware, so this is going to be a real issue for
users

3) Much faster resume, the firmware is 40k, uploading that over
i2c at 100Khz takes multiple seconds.

So I'm going to post the i2c-hid changes and see what
Jiri and Benjamin think of those and then we will see
from there.

Regards,

Hans




> 
> Regards,
> 
> Hans
> 
> 
>> +
>> +    return 0;
>> +}
>> +#else
>> +static int icn8318_probe_acpi(struct icn8318_data *data, struct device *dev)
>> +{
>> +    return -ENODEV;
>> +}
>> +#endif
>> +
>>   static int icn8318_probe(struct i2c_client *client)
>>   {
>>       struct device *dev = &client->dev;
>> @@ -264,7 +325,10 @@ static int icn8318_probe(struct i2c_client *client)
>>       input_set_capability(input, EV_ABS, ABS_MT_POSITION_X);
>>       input_set_capability(input, EV_ABS, ABS_MT_POSITION_Y);
>> -    error = icn8318_probe_of(data, dev);
>> +    if (client->dev.of_node)
>> +        error = icn8318_probe_of(data, dev);
>> +    else
>> +        error = icn8318_probe_acpi(data, dev);
>>       if (error)
>>           return error;
>> @@ -318,6 +382,7 @@ static struct i2c_driver icn8318_driver = {
>>       .driver = {
>>           .name    = "chipone_icn8318",
>>           .pm    = &icn8318_pm_ops,
>> +        .acpi_match_table = ACPI_PTR(icn8318_acpi_match),
>>           .of_match_table = icn8318_of_match,
>>       },
>>       .probe_new = icn8318_probe,
>>

Re: [PATCH 3/4] Input: icn8318 - Add support for ACPI enumeration

[Benjamin Tissoires]

On Jun 18 2017 or thereabouts, Hans de Goede wrote:
> Hi,
> 
> On 17-06-17 22:09, Hans de Goede wrote:
> > Hi,
> > 
> > Self-nack see comment inline.
> > 
> > On 17-06-17 12:58, Hans de Goede wrote:
> > > The icn8505 variant is found on some x86 tablets, which use ACPI
> > > enumeration, add support for ACPI enumeration.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > >   drivers/input/touchscreen/Kconfig           |  2 +-
> > >   drivers/input/touchscreen/chipone_icn8318.c | 67 ++++++++++++++++++++++++++++-
> > >   2 files changed, 67 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > > index fff1467506e7..e3b52d356e13 100644
> > > --- a/drivers/input/touchscreen/Kconfig
> > > +++ b/drivers/input/touchscreen/Kconfig
> > > @@ -155,7 +155,7 @@ config TOUCHSCREEN_CHIPONE_ICN8318
> > >       tristate "chipone icn8318 touchscreen controller"
> > >       depends on GPIOLIB || COMPILE_TEST
> > >       depends on I2C
> > > -    depends on OF
> > > +    depends on OF || ACPI
> > >       help
> > >         Say Y here if you have a ChipOne icn8318 or icn8505 based
> > >         I2C touchscreen.
> > > diff --git a/drivers/input/touchscreen/chipone_icn8318.c b/drivers/input/touchscreen/chipone_icn8318.c
> > > index 993df804883f..b209872954f7 100644
> > > --- a/drivers/input/touchscreen/chipone_icn8318.c
> > > +++ b/drivers/input/touchscreen/chipone_icn8318.c
> > > @@ -12,6 +12,7 @@
> > >    * Hans de Goede <hdegoede@redhat.com>
> > >    */
> > > +#include <linux/acpi.h>
> > >   #include <linux/gpio/consumer.h>
> > >   #include <linux/interrupt.h>
> > >   #include <linux/i2c.h>
> > > @@ -232,6 +233,66 @@ static int icn8318_probe_of(struct icn8318_data *data, struct device *dev)
> > >   }
> > >   #endif
> > > +#ifdef CONFIG_ACPI
> > > +static const struct acpi_device_id icn8318_acpi_match[] = {
> > > +    { "CHPN0001", ICN8505 },
> > > +    { }
> > > +};
> > > +MODULE_DEVICE_TABLE(acpi, icn8318_acpi_match);
> > > +
> > > +static int icn8318_probe_acpi(struct icn8318_data *data, struct device *dev)
> > > +{
> > > +    struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
> > > +    struct input_dev *input = data->input;
> > > +    const struct acpi_device_id *id;
> > > +    struct acpi_device *adev;
> > > +    acpi_status status;
> > > +    const char *sub;
> > > +
> > > +    adev = ACPI_COMPANION(dev);
> > > +    id = acpi_match_device(icn8318_acpi_match, dev);
> > > +    if (!adev || !id)
> > > +        return -ENODEV;
> > > +
> > > +    data->model = id->driver_data;
> > > +
> > > +    /* The _SUB ACPI object describes the touchscreen model */
> > > +    status = acpi_evaluate_object_typed(adev->handle, "_SUB", NULL,
> > > +                        &buf, ACPI_TYPE_STRING);
> > > +    if (ACPI_FAILURE(status)) {
> > > +        dev_err(dev, "ACPI _SUB object not found\n");
> > > +        return -ENODEV;
> > > +    }
> > > +    sub = ((union acpi_object *)buf.pointer)->string.pointer;
> > > +
> > > +    if (strcmp(sub, "HAMP0002") == 0) {
> > > +        input_set_abs_params(input, ABS_MT_POSITION_X, 0, 1199, 0, 0);
> > > +        input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 1919, 0, 0);
> > > +    } else {
> > > +        dev_err(dev, "Unknown model _SUB: %s\n", sub);
> > > +        kfree(buf.pointer);
> > > +        return -ENODEV;
> > > +    }
> > > +    kfree(buf.pointer);
> > > +
> > > +    /*
> > > +     * Disable ACPI power management the _PS3 method is empty, so
> > > +     * there is no powersaving when using ACPI power management.
> > > +     * The _PS0 method resets the controller causing it to loose its
> > > +     * firmware, which has been loaded by the BIOS and we do not
> > > +     * know how to restore the firmware.
> > > +     */
> > > +    adev->flags.power_manageable = 0;
> > 
> > Ok, so unfortunately this is not that easy :|  The ACPI node
> > has a _CID, "PNP0C50" causing i2c-hid to bind, even though the
> > device has its own device specific protocol. I had i2c-hid
> > blacklisted for testing, with the plan to add a quirk to it for this
> > device, but with the quirk if i2c-hid loads earlier then the icn8318
> > driver and i2c-hid's probe returns -ENODEV due to the quirk, the
> > device gets turned off by the ACPI core, causing a reset + loss
> > of the loaded firmware when the icn8313 driver loads. So there
> > are 2 solutions here:
> > 
> > 1) Set adev->flags.power_manageable earlier, somewhere in the
> > ACPI core
> > 
> > 2) Figure out how to load the firmware to make the controller
> > functional again
> 
> While working on a 3th option, I did a web-search for "CHPN0001"
> instead of for icn8505 which found me this:
> 
> https://github.com/Dax89/chuwi-dev/tree/master/drivers/chipone_ts
> 
> Which seemed to be based on the same android code as I already
> found, but which claims to have firmware loading working again,
> so assuming that is true (I've not tested) that gives is us
> 2 options, either avoid the controller reset, or load the
> firmware.
> 
> As said I've been working on a 3th option:
> 
> 3) Still add a quirk to i2c-hid but do the check at module_init
> time, rather then add probe time, so that the i2c-hid driver never
> registers avoiding the problem of the APCI PS3 / PS0 methods
> getting called after the failed i2c-hid probe / before the icn8318
> probe.
> 
> I've implemented this now and it works well. Although not pretty, it
> is only 2 lines of code (and a 5 line block comment), so I hope that
> Jiri and Benjamin can live with this.
> 
> 
> I personally prefer this 3th option (avoiding controller reset)
> over adding firmware upload support for 3 reasons:
> 
> 1) It is a lot less code it requires the following in i2c-hid:
> 
>  static int __init i2c_hid_init(void)
>  {
> +       /*
> +        * The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID
> +        * compatible, just probing it puts the device in an unusable state due
> +        * to it also have ACPI power management issues.
> +        */
> +       if (acpi_dev_present("CHPN0001", NULL, -1))
> +               return -ENODEV;
> +
>         return i2c_add_driver(&i2c_hid_driver);
>  }

I am not a big fan of this for several reasons (most can be worked
around):
- it's a hack for a specific device and is not generic enough
- it prevents i2c-hid to be loaded on a platform that exports such
  device, even if other devices are legitimately using i2c-hid
- what if the chipone_icn8318 is not compiled in? Or in other words,
  does the touchscreen works somehow with i2c-hid?

I think the biggest issue is that you are blacklisting a driver based on
a single peripheral while other devices might want to use it.

I have a couple of questions for you:
- is the device working with i2c-hid (in degraded mode)?
- assuming the devices works with i2c-hid, will an rmmod/modprobe of
  hid-generic/hid-chipony-icn8318 (to be written) introduce the PS3/PS0
  issues?

If both are yes, I think we better have a hid specific driver for it
(which could reuse part of the current input driver).

Cheers,
Benjamin


> 
> And the following in the icn8318 code:
> 
>         /*
>          * Disable ACPI power management the _PS3 method is empty, so
>          * there is no powersaving when using ACPI power management.
>          * The _PS0 method resets the controller causing it to loose its
>          * firmware, which has been loaded by the BIOS and we do not
>          * know how to restore the firmware.
>          */
>         adev->flags.power_manageable = 0;
> 
> Versus significantly more code to get firmware uploading to work
> 
> 2) It avoids the need for users to get their controller firmware
> from somewhere, it likely is going to be hard to get permission to
> add this to linux-firmware, so this is going to be a real issue for
> users
> 
> 3) Much faster resume, the firmware is 40k, uploading that over
> i2c at 100Khz takes multiple seconds.
> 
> So I'm going to post the i2c-hid changes and see what
> Jiri and Benjamin think of those and then we will see
> from there.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> > 
> > Regards,
> > 
> > Hans
> > 
> > 
> > > +
> > > +    return 0;
> > > +}
> > > +#else
> > > +static int icn8318_probe_acpi(struct icn8318_data *data, struct device *dev)
> > > +{
> > > +    return -ENODEV;
> > > +}
> > > +#endif
> > > +
> > >   static int icn8318_probe(struct i2c_client *client)
> > >   {
> > >       struct device *dev = &client->dev;
> > > @@ -264,7 +325,10 @@ static int icn8318_probe(struct i2c_client *client)
> > >       input_set_capability(input, EV_ABS, ABS_MT_POSITION_X);
> > >       input_set_capability(input, EV_ABS, ABS_MT_POSITION_Y);
> > > -    error = icn8318_probe_of(data, dev);
> > > +    if (client->dev.of_node)
> > > +        error = icn8318_probe_of(data, dev);
> > > +    else
> > > +        error = icn8318_probe_acpi(data, dev);
> > >       if (error)
> > >           return error;
> > > @@ -318,6 +382,7 @@ static struct i2c_driver icn8318_driver = {
> > >       .driver = {
> > >           .name    = "chipone_icn8318",
> > >           .pm    = &icn8318_pm_ops,
> > > +        .acpi_match_table = ACPI_PTR(icn8318_acpi_match),
> > >           .of_match_table = icn8318_of_match,
> > >       },
> > >       .probe_new = icn8318_probe,
> > > 

Re: [PATCH 3/4] Input: icn8318 - Add support for ACPI enumeration

[Hans de Goede]

Hi,

On 19-06-17 10:51, Benjamin Tissoires wrote:
> On Jun 18 2017 or thereabouts, Hans de Goede wrote:
>> Hi,
>>
>> On 17-06-17 22:09, Hans de Goede wrote:
>>> Hi,
>>>
>>> Self-nack see comment inline.
>>>
>>> On 17-06-17 12:58, Hans de Goede wrote:
>>>> The icn8505 variant is found on some x86 tablets, which use ACPI
>>>> enumeration, add support for ACPI enumeration.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>    drivers/input/touchscreen/Kconfig           |  2 +-
>>>>    drivers/input/touchscreen/chipone_icn8318.c | 67 ++++++++++++++++++++++++++++-
>>>>    2 files changed, 67 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>>>> index fff1467506e7..e3b52d356e13 100644
>>>> --- a/drivers/input/touchscreen/Kconfig
>>>> +++ b/drivers/input/touchscreen/Kconfig
>>>> @@ -155,7 +155,7 @@ config TOUCHSCREEN_CHIPONE_ICN8318
>>>>        tristate "chipone icn8318 touchscreen controller"
>>>>        depends on GPIOLIB || COMPILE_TEST
>>>>        depends on I2C
>>>> -    depends on OF
>>>> +    depends on OF || ACPI
>>>>        help
>>>>          Say Y here if you have a ChipOne icn8318 or icn8505 based
>>>>          I2C touchscreen.
>>>> diff --git a/drivers/input/touchscreen/chipone_icn8318.c b/drivers/input/touchscreen/chipone_icn8318.c
>>>> index 993df804883f..b209872954f7 100644
>>>> --- a/drivers/input/touchscreen/chipone_icn8318.c
>>>> +++ b/drivers/input/touchscreen/chipone_icn8318.c
>>>> @@ -12,6 +12,7 @@
>>>>     * Hans de Goede <hdegoede@redhat.com>
>>>>     */
>>>> +#include <linux/acpi.h>
>>>>    #include <linux/gpio/consumer.h>
>>>>    #include <linux/interrupt.h>
>>>>    #include <linux/i2c.h>
>>>> @@ -232,6 +233,66 @@ static int icn8318_probe_of(struct icn8318_data *data, struct device *dev)
>>>>    }
>>>>    #endif
>>>> +#ifdef CONFIG_ACPI
>>>> +static const struct acpi_device_id icn8318_acpi_match[] = {
>>>> +    { "CHPN0001", ICN8505 },
>>>> +    { }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(acpi, icn8318_acpi_match);
>>>> +
>>>> +static int icn8318_probe_acpi(struct icn8318_data *data, struct device *dev)
>>>> +{
>>>> +    struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
>>>> +    struct input_dev *input = data->input;
>>>> +    const struct acpi_device_id *id;
>>>> +    struct acpi_device *adev;
>>>> +    acpi_status status;
>>>> +    const char *sub;
>>>> +
>>>> +    adev = ACPI_COMPANION(dev);
>>>> +    id = acpi_match_device(icn8318_acpi_match, dev);
>>>> +    if (!adev || !id)
>>>> +        return -ENODEV;
>>>> +
>>>> +    data->model = id->driver_data;
>>>> +
>>>> +    /* The _SUB ACPI object describes the touchscreen model */
>>>> +    status = acpi_evaluate_object_typed(adev->handle, "_SUB", NULL,
>>>> +                        &buf, ACPI_TYPE_STRING);
>>>> +    if (ACPI_FAILURE(status)) {
>>>> +        dev_err(dev, "ACPI _SUB object not found\n");
>>>> +        return -ENODEV;
>>>> +    }
>>>> +    sub = ((union acpi_object *)buf.pointer)->string.pointer;
>>>> +
>>>> +    if (strcmp(sub, "HAMP0002") == 0) {
>>>> +        input_set_abs_params(input, ABS_MT_POSITION_X, 0, 1199, 0, 0);
>>>> +        input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 1919, 0, 0);
>>>> +    } else {
>>>> +        dev_err(dev, "Unknown model _SUB: %s\n", sub);
>>>> +        kfree(buf.pointer);
>>>> +        return -ENODEV;
>>>> +    }
>>>> +    kfree(buf.pointer);
>>>> +
>>>> +    /*
>>>> +     * Disable ACPI power management the _PS3 method is empty, so
>>>> +     * there is no powersaving when using ACPI power management.
>>>> +     * The _PS0 method resets the controller causing it to loose its
>>>> +     * firmware, which has been loaded by the BIOS and we do not
>>>> +     * know how to restore the firmware.
>>>> +     */
>>>> +    adev->flags.power_manageable = 0;
>>>
>>> Ok, so unfortunately this is not that easy :|  The ACPI node
>>> has a _CID, "PNP0C50" causing i2c-hid to bind, even though the
>>> device has its own device specific protocol. I had i2c-hid
>>> blacklisted for testing, with the plan to add a quirk to it for this
>>> device, but with the quirk if i2c-hid loads earlier then the icn8318
>>> driver and i2c-hid's probe returns -ENODEV due to the quirk, the
>>> device gets turned off by the ACPI core, causing a reset + loss
>>> of the loaded firmware when the icn8313 driver loads. So there
>>> are 2 solutions here:
>>>
>>> 1) Set adev->flags.power_manageable earlier, somewhere in the
>>> ACPI core
>>>
>>> 2) Figure out how to load the firmware to make the controller
>>> functional again
>>
>> While working on a 3th option, I did a web-search for "CHPN0001"
>> instead of for icn8505 which found me this:
>>
>> https://github.com/Dax89/chuwi-dev/tree/master/drivers/chipone_ts
>>
>> Which seemed to be based on the same android code as I already
>> found, but which claims to have firmware loading working again,
>> so assuming that is true (I've not tested) that gives is us
>> 2 options, either avoid the controller reset, or load the
>> firmware.
>>
>> As said I've been working on a 3th option:
>>
>> 3) Still add a quirk to i2c-hid but do the check at module_init
>> time, rather then add probe time, so that the i2c-hid driver never
>> registers avoiding the problem of the APCI PS3 / PS0 methods
>> getting called after the failed i2c-hid probe / before the icn8318
>> probe.
>>
>> I've implemented this now and it works well. Although not pretty, it
>> is only 2 lines of code (and a 5 line block comment), so I hope that
>> Jiri and Benjamin can live with this.
>>
>>
>> I personally prefer this 3th option (avoiding controller reset)
>> over adding firmware upload support for 3 reasons:
>>
>> 1) It is a lot less code it requires the following in i2c-hid:
>>
>>   static int __init i2c_hid_init(void)
>>   {
>> +       /*
>> +        * The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID
>> +        * compatible, just probing it puts the device in an unusable state due
>> +        * to it also have ACPI power management issues.
>> +        */
>> +       if (acpi_dev_present("CHPN0001", NULL, -1))
>> +               return -ENODEV;
>> +
>>          return i2c_add_driver(&i2c_hid_driver);
>>   }
> 
> I am not a big fan of this for several reasons (most can be worked
> around):
> - it's a hack for a specific device and is not generic enough

? It is for all devices with a CHPN0001 touchscreen according to
the alternative driver I found that covers at least the Chuwi Vi 10
Ultimate, Chuwi Vi10 plus and the Chuwi Hi 10. Note the match is
on an ACPI HID, which is (usually) not machine specific. Or with
"for a specific device", do you mean for a specific touchscreen
controller ?

> - it prevents i2c-hid to be loaded on a platform that exports such
>    device, even if other devices are legitimately using i2c-hid

The chances of a tablet like this having another HID device
attached through i2c are very very small. At first I tried to
avoid this problem by doing the check for CHPN0001 in i2c_hid_probe,
but as explained that is too late.

> - what if the chipone_icn8318 is not compiled in? Or in other words,
>    does the touchscreen works somehow with i2c-hid?

No it does not work at all with i2c-hid, AFAICT it is not doing
HID at all and the _CID advertising HID support is bogus, to get
touch data on an interrupt you need to do an i2c write of 2 bytes
to select the 0x1000 register address and then you read 37 bytes
for the touch data, which has this structure:

struct icn8318_touch {
         __u8 slot;
         __be16 x;
         __be16 y;
         __u8 pressure;  /* Seems more like finger width then pressure really */
         __u8 event;
/* The difference between 2 and 3 is unclear */
#define ICN8318_EVENT_NO_DATA   1 /* No finger seen yet since wakeup */
#define ICN8318_EVENT_UPDATE1   2 /* New or updated coordinates */
#define ICN8318_EVENT_UPDATE2   3 /* New or updated coordinates */
#define ICN8318_EVENT_END       4 /* Finger lifted */
} __packed;

struct icn8318_touch_data {
         __u8 softbutton;
         __u8 touch_count;
         struct icn8318_touch touches[ICN8318_MAX_TOUCHES];
} __packed;

Before doing these patches I've taken a quick look at i2c-hid and
AFAICT this has very little to do with I2C-hid, so I believe the
_CID with "PNP0C50" is simply bogus and we are going to need
to blacklist this in i2c-hid one way or the other anyway, even if
I do add firmware loading to the icn8505 specific driver.

> I think the biggest issue is that you are blacklisting a driver based on
> a single peripheral while other devices might want to use it.

I understand that that is not how we would normally do this, but:

1) Doing so allows the controller to keep its firmware which as
explained is a good thing for a number of reasons

2) The only hardware we know to has this peripheral is known to
not have any other i2c-HId peripherals, so although we would not
normally do this, it is not a problem.

> I have a couple of questions for you:
> - is the device working with i2c-hid (in degraded mode)?

Not sure what you mean by that, do you mean mouse emulation mode
or some such ? Anyways I've not had the device work in any way,
if I comment out the fix_up_power call which also triggers the
controller reset -> firmware gone thing, then I get a:

"unexpected HID descriptor bcdVersion (0x0000)" error instead
of the hid_descr_cmd failed error which happens when the
controller has lost its firmware, causing the i2c-hid driver
to not attach.

> - assuming the devices works with i2c-hid, will an rmmod/modprobe of
>    hid-generic/hid-chipony-icn8318 (to be written) introduce the PS3/PS0
>    issues?

If possible, yes I believe it will cause the PS3/PS0 issue,
but I don't think doing so is possible at all.

> If both are yes, I think we better have a hid specific driver for it
> (which could reuse part of the current input driver).

See above.

Regards,

Hans

Re: [PATCH 3/4] Input: icn8318 - Add support for ACPI enumeration

[Benjamin Tissoires]

On Jun 19 2017 or thereabouts, Hans de Goede wrote:
> Hi,
> 
> On 19-06-17 10:51, Benjamin Tissoires wrote:
> > On Jun 18 2017 or thereabouts, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 17-06-17 22:09, Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > Self-nack see comment inline.
> > > > 
> > > > On 17-06-17 12:58, Hans de Goede wrote:
> > > > > The icn8505 variant is found on some x86 tablets, which use ACPI
> > > > > enumeration, add support for ACPI enumeration.
> > > > > 
> > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > > ---
> > > > >    drivers/input/touchscreen/Kconfig           |  2 +-
> > > > >    drivers/input/touchscreen/chipone_icn8318.c | 67 ++++++++++++++++++++++++++++-
> > > > >    2 files changed, 67 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > > > > index fff1467506e7..e3b52d356e13 100644
> > > > > --- a/drivers/input/touchscreen/Kconfig
> > > > > +++ b/drivers/input/touchscreen/Kconfig
> > > > > @@ -155,7 +155,7 @@ config TOUCHSCREEN_CHIPONE_ICN8318
> > > > >        tristate "chipone icn8318 touchscreen controller"
> > > > >        depends on GPIOLIB || COMPILE_TEST
> > > > >        depends on I2C
> > > > > -    depends on OF
> > > > > +    depends on OF || ACPI
> > > > >        help
> > > > >          Say Y here if you have a ChipOne icn8318 or icn8505 based
> > > > >          I2C touchscreen.
> > > > > diff --git a/drivers/input/touchscreen/chipone_icn8318.c b/drivers/input/touchscreen/chipone_icn8318.c
> > > > > index 993df804883f..b209872954f7 100644
> > > > > --- a/drivers/input/touchscreen/chipone_icn8318.c
> > > > > +++ b/drivers/input/touchscreen/chipone_icn8318.c
> > > > > @@ -12,6 +12,7 @@
> > > > >     * Hans de Goede <hdegoede@redhat.com>
> > > > >     */
> > > > > +#include <linux/acpi.h>
> > > > >    #include <linux/gpio/consumer.h>
> > > > >    #include <linux/interrupt.h>
> > > > >    #include <linux/i2c.h>
> > > > > @@ -232,6 +233,66 @@ static int icn8318_probe_of(struct icn8318_data *data, struct device *dev)
> > > > >    }
> > > > >    #endif
> > > > > +#ifdef CONFIG_ACPI
> > > > > +static const struct acpi_device_id icn8318_acpi_match[] = {
> > > > > +    { "CHPN0001", ICN8505 },
> > > > > +    { }
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(acpi, icn8318_acpi_match);
> > > > > +
> > > > > +static int icn8318_probe_acpi(struct icn8318_data *data, struct device *dev)
> > > > > +{
> > > > > +    struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
> > > > > +    struct input_dev *input = data->input;
> > > > > +    const struct acpi_device_id *id;
> > > > > +    struct acpi_device *adev;
> > > > > +    acpi_status status;
> > > > > +    const char *sub;
> > > > > +
> > > > > +    adev = ACPI_COMPANION(dev);
> > > > > +    id = acpi_match_device(icn8318_acpi_match, dev);
> > > > > +    if (!adev || !id)
> > > > > +        return -ENODEV;
> > > > > +
> > > > > +    data->model = id->driver_data;
> > > > > +
> > > > > +    /* The _SUB ACPI object describes the touchscreen model */
> > > > > +    status = acpi_evaluate_object_typed(adev->handle, "_SUB", NULL,
> > > > > +                        &buf, ACPI_TYPE_STRING);
> > > > > +    if (ACPI_FAILURE(status)) {
> > > > > +        dev_err(dev, "ACPI _SUB object not found\n");
> > > > > +        return -ENODEV;
> > > > > +    }
> > > > > +    sub = ((union acpi_object *)buf.pointer)->string.pointer;
> > > > > +
> > > > > +    if (strcmp(sub, "HAMP0002") == 0) {
> > > > > +        input_set_abs_params(input, ABS_MT_POSITION_X, 0, 1199, 0, 0);
> > > > > +        input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 1919, 0, 0);
> > > > > +    } else {
> > > > > +        dev_err(dev, "Unknown model _SUB: %s\n", sub);
> > > > > +        kfree(buf.pointer);
> > > > > +        return -ENODEV;
> > > > > +    }
> > > > > +    kfree(buf.pointer);
> > > > > +
> > > > > +    /*
> > > > > +     * Disable ACPI power management the _PS3 method is empty, so
> > > > > +     * there is no powersaving when using ACPI power management.
> > > > > +     * The _PS0 method resets the controller causing it to loose its
> > > > > +     * firmware, which has been loaded by the BIOS and we do not
> > > > > +     * know how to restore the firmware.
> > > > > +     */
> > > > > +    adev->flags.power_manageable = 0;
> > > > 
> > > > Ok, so unfortunately this is not that easy :|  The ACPI node
> > > > has a _CID, "PNP0C50" causing i2c-hid to bind, even though the
> > > > device has its own device specific protocol. I had i2c-hid
> > > > blacklisted for testing, with the plan to add a quirk to it for this
> > > > device, but with the quirk if i2c-hid loads earlier then the icn8318
> > > > driver and i2c-hid's probe returns -ENODEV due to the quirk, the
> > > > device gets turned off by the ACPI core, causing a reset + loss
> > > > of the loaded firmware when the icn8313 driver loads. So there
> > > > are 2 solutions here:
> > > > 
> > > > 1) Set adev->flags.power_manageable earlier, somewhere in the
> > > > ACPI core
> > > > 
> > > > 2) Figure out how to load the firmware to make the controller
> > > > functional again
> > > 
> > > While working on a 3th option, I did a web-search for "CHPN0001"
> > > instead of for icn8505 which found me this:
> > > 
> > > https://github.com/Dax89/chuwi-dev/tree/master/drivers/chipone_ts
> > > 
> > > Which seemed to be based on the same android code as I already
> > > found, but which claims to have firmware loading working again,
> > > so assuming that is true (I've not tested) that gives is us
> > > 2 options, either avoid the controller reset, or load the
> > > firmware.
> > > 
> > > As said I've been working on a 3th option:
> > > 
> > > 3) Still add a quirk to i2c-hid but do the check at module_init
> > > time, rather then add probe time, so that the i2c-hid driver never
> > > registers avoiding the problem of the APCI PS3 / PS0 methods
> > > getting called after the failed i2c-hid probe / before the icn8318
> > > probe.
> > > 
> > > I've implemented this now and it works well. Although not pretty, it
> > > is only 2 lines of code (and a 5 line block comment), so I hope that
> > > Jiri and Benjamin can live with this.
> > > 
> > > 
> > > I personally prefer this 3th option (avoiding controller reset)
> > > over adding firmware upload support for 3 reasons:
> > > 
> > > 1) It is a lot less code it requires the following in i2c-hid:
> > > 
> > >   static int __init i2c_hid_init(void)
> > >   {
> > > +       /*
> > > +        * The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID
> > > +        * compatible, just probing it puts the device in an unusable state due
> > > +        * to it also have ACPI power management issues.
> > > +        */
> > > +       if (acpi_dev_present("CHPN0001", NULL, -1))
> > > +               return -ENODEV;
> > > +
> > >          return i2c_add_driver(&i2c_hid_driver);
> > >   }
> > 
> > I am not a big fan of this for several reasons (most can be worked
> > around):
> > - it's a hack for a specific device and is not generic enough
> 
> ? It is for all devices with a CHPN0001 touchscreen according to
> the alternative driver I found that covers at least the Chuwi Vi 10
> Ultimate, Chuwi Vi10 plus and the Chuwi Hi 10. Note the match is
> on an ACPI HID, which is (usually) not machine specific. Or with
> "for a specific device", do you mean for a specific touchscreen
> controller ?

Well, let me rephrase that. I'd better have an array of "blacklisted
platforms based on a bogus device" than having a specific 'if' on a
single ACPI id in init. Because then an other crappy hardware comes in,
and we'll have to add an other 'if', etc...

> 
> > - it prevents i2c-hid to be loaded on a platform that exports such
> >    device, even if other devices are legitimately using i2c-hid
> 
> The chances of a tablet like this having another HID device
> attached through i2c are very very small. At first I tried to

They are small, but why wouldn't they use a sensor hub through i2c-hid?

> avoid this problem by doing the check for CHPN0001 in i2c_hid_probe,
> but as explained that is too late.

We might need a .match() callback on the struct i2c_driver to achieve
this properly with a per-device use. That would be cleaner, but require
an i2c_core change.

> 
> > - what if the chipone_icn8318 is not compiled in? Or in other words,
> >    does the touchscreen works somehow with i2c-hid?
> 
> No it does not work at all with i2c-hid, AFAICT it is not doing
> HID at all and the _CID advertising HID support is bogus, to get

K, you convinced me here, no need to detail too much crappy hardware :)

> touch data on an interrupt you need to do an i2c write of 2 bytes
> to select the 0x1000 register address and then you read 37 bytes
> for the touch data, which has this structure:
> 
> struct icn8318_touch {
>         __u8 slot;
>         __be16 x;
>         __be16 y;
>         __u8 pressure;  /* Seems more like finger width then pressure really */
>         __u8 event;
> /* The difference between 2 and 3 is unclear */
> #define ICN8318_EVENT_NO_DATA   1 /* No finger seen yet since wakeup */
> #define ICN8318_EVENT_UPDATE1   2 /* New or updated coordinates */
> #define ICN8318_EVENT_UPDATE2   3 /* New or updated coordinates */
> #define ICN8318_EVENT_END       4 /* Finger lifted */
> } __packed;
> 
> struct icn8318_touch_data {
>         __u8 softbutton;
>         __u8 touch_count;
>         struct icn8318_touch touches[ICN8318_MAX_TOUCHES];
> } __packed;
> 
> Before doing these patches I've taken a quick look at i2c-hid and
> AFAICT this has very little to do with I2C-hid, so I believe the
> _CID with "PNP0C50" is simply bogus and we are going to need
> to blacklist this in i2c-hid one way or the other anyway, even if
> I do add firmware loading to the icn8505 specific driver.
> 
> > I think the biggest issue is that you are blacklisting a driver based on
> > a single peripheral while other devices might want to use it.
> 
> I understand that that is not how we would normally do this, but:
> 
> 1) Doing so allows the controller to keep its firmware which as
> explained is a good thing for a number of reasons
> 
> 2) The only hardware we know to has this peripheral is known to
> not have any other i2c-HId peripherals, so although we would not
> normally do this, it is not a problem.

I am still a little bit hesitant in blacklisting a module based on a
single peripheral. Could you raise the .match() extension of i2c_core
and see how Wolfram reacts?

> 
> > I have a couple of questions for you:
> > - is the device working with i2c-hid (in degraded mode)?
> 
> Not sure what you mean by that, do you mean mouse emulation mode
> or some such ?

Yes

> Anyways I've not had the device work in any way,
> if I comment out the fix_up_power call which also triggers the
> controller reset -> firmware gone thing, then I get a:
> 
> "unexpected HID descriptor bcdVersion (0x0000)" error instead
> of the hid_descr_cmd failed error which happens when the
> controller has lost its firmware, causing the i2c-hid driver
> to not attach.

OK. The question was a way to know if this patch will introduce any
regression. It seems like the HW is already buggy, so we can take a
blacklist without second remorse.

> 
> > - assuming the devices works with i2c-hid, will an rmmod/modprobe of
> >    hid-generic/hid-chipony-icn8318 (to be written) introduce the PS3/PS0
> >    issues?
> 
> If possible, yes I believe it will cause the PS3/PS0 issue,
> but I don't think doing so is possible at all.
> 
> > If both are yes, I think we better have a hid specific driver for it
> > (which could reuse part of the current input driver).
> 
> See above.

Thanks for the answers.

If you can't get an i2c_core change, we can take your patch, but I would
really prefer having a per-device decision that doesn't involve
blacklisting a full module. Also, .match() will remove an extra walk on the
ACPI tree.

Cheers,
Benjamin