[PATCHv1 1/2] Input: EXC3000: add EXC80Hxx support

[PATCHv1 1/2] Input: EXC3000: add EXC80Hxx support

[Sebastian Reichel]

This adds support for EXC80Hxx controllers, which uses
a different event type id and has two extra bits for the
resolution (so the maximum is 16384 instead of 4096).

The patch has been tested with EXC80H60 and EXC80H84.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../bindings/input/touchscreen/exc3000.txt    |  6 ++--
 drivers/input/touchscreen/exc3000.c           | 34 ++++++++++++++-----
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/exc3000.txt b/Documentation/devicetree/bindings/input/touchscreen/exc3000.txt
index 68291b94fec2..057b680f0420 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/exc3000.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/exc3000.txt
@@ -1,7 +1,9 @@
-* EETI EXC3000 Multiple Touch Controller
+* EETI EXC3000 and EXC80Hxx Multiple Touch Controller
 
 Required properties:
-- compatible: must be "eeti,exc3000"
+- compatible: must be one of
+ * "eeti,exc3000"
+ * "eeti,exc80hxx"
 - reg: i2c slave address
 - interrupts: touch controller interrupt
 - touchscreen-size-x: See touchscreen.txt
diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
index e007e2e8f626..7d695022082c 100644
--- a/drivers/input/touchscreen/exc3000.c
+++ b/drivers/input/touchscreen/exc3000.c
@@ -23,11 +23,20 @@
 #define EXC3000_SLOTS_PER_FRAME		5
 #define EXC3000_LEN_FRAME		66
 #define EXC3000_LEN_POINT		10
-#define EXC3000_MT_EVENT		6
+
+#define EXC3000_MT1_EVENT		0x06
+#define EXC3000_MT2_EVENT		0x18
+
 #define EXC3000_TIMEOUT_MS		100
 
+enum exc3000_device_type {
+	EETI_EXC3000,
+	EETI_EXC80Hxx
+};
+
 struct exc3000_data {
 	struct i2c_client *client;
+	enum exc3000_device_type type;
 	struct input_dev *input;
 	struct touchscreen_properties prop;
 	struct timer_list timer;
@@ -76,8 +85,10 @@ static int exc3000_read_frame(struct i2c_client *client, u8 *buf)
 	if (ret != EXC3000_LEN_FRAME)
 		return -EIO;
 
-	if (get_unaligned_le16(buf) != EXC3000_LEN_FRAME ||
-			buf[2] != EXC3000_MT_EVENT)
+	if (get_unaligned_le16(buf) != EXC3000_LEN_FRAME)
+		return -EINVAL;
+
+	if (buf[2] != EXC3000_MT1_EVENT && buf[2] != EXC3000_MT2_EVENT)
 		return -EINVAL;
 
 	return 0;
@@ -157,6 +168,7 @@ static int exc3000_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	data->client = client;
+	data->type = id->driver_data;
 	timer_setup(&data->timer, exc3000_timer, 0);
 
 	input = devm_input_allocate_device(&client->dev);
@@ -168,8 +180,13 @@ static int exc3000_probe(struct i2c_client *client,
 	input->name = "EETI EXC3000 Touch Screen";
 	input->id.bustype = BUS_I2C;
 
-	input_set_abs_params(input, ABS_MT_POSITION_X, 0, 4095, 0, 0);
-	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 4095, 0, 0);
+	if (data->type == EETI_EXC80Hxx) {
+		input_set_abs_params(input, ABS_MT_POSITION_X, 0, 16383, 0, 0);
+		input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 16383, 0, 0);
+	} else {
+		input_set_abs_params(input, ABS_MT_POSITION_X, 0, 4095, 0, 0);
+		input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 4095, 0, 0);
+	}
 	touchscreen_parse_properties(input, true, &data->prop);
 
 	error = input_mt_init_slots(input, EXC3000_NUM_SLOTS,
@@ -191,14 +208,15 @@ static int exc3000_probe(struct i2c_client *client,
 }
 
 static const struct i2c_device_id exc3000_id[] = {
-	{ "exc3000", 0 },
-	{ }
+	{ "exc3000", EETI_EXC3000 },
+	{ "exc80hxx", EETI_EXC80Hxx }
 };
 MODULE_DEVICE_TABLE(i2c, exc3000_id);
 
 #ifdef CONFIG_OF
 static const struct of_device_id exc3000_of_match[] = {
-	{ .compatible = "eeti,exc3000" },
+	{ .compatible = "eeti,exc3000", .data = (const void*) EETI_EXC3000 },
+	{ .compatible = "eeti,exc80hxx", .data = (const void*) EETI_EXC80Hxx },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, exc3000_of_match);
-- 
2.24.0.rc1

[PATCHv1 2/2] Input: EXC3000: Add support to query model and fw_version

[Sebastian Reichel]

Expose model and fw_version via sysfs. Also query the model
in probe to make sure, that the I2C communication with the
device works before successfully probing the driver.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/input/touchscreen/exc3000.c | 136 ++++++++++++++++++++++++++++
 1 file changed, 136 insertions(+)

diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
index 7d695022082c..4c9f132629b9 100644
--- a/drivers/input/touchscreen/exc3000.c
+++ b/drivers/input/touchscreen/exc3000.c
@@ -41,6 +41,11 @@ struct exc3000_data {
 	struct touchscreen_properties prop;
 	struct timer_list timer;
 	u8 buf[2 * EXC3000_LEN_FRAME];
+	char model[11];
+	char fw_version[6];
+	struct completion wait_event;
+	struct mutex query_lock;
+	int query_result;
 };
 
 static void exc3000_report_slots(struct input_dev *input,
@@ -121,6 +126,35 @@ static int exc3000_read_data(struct i2c_client *client,
 	return 0;
 }
 
+static void exc3000_query_interrupt(struct exc3000_data *data)
+{
+	u8 *buf = data->buf;
+	int err;
+
+	data->query_result = 0;
+
+	err = i2c_master_recv(data->client, buf, EXC3000_LEN_FRAME);
+	if (err < 0) {
+		data->query_result = err;
+		goto out;
+	}
+
+	if (buf[0] == 0x42 && buf[4] == 'E') {
+		memcpy(data->model, buf+5, 10);
+		data->model[10] = '\0';
+		goto out;
+	} else if (buf[0] == 0x42 && buf[4] == 'D') {
+		memcpy(data->fw_version, buf+5, 5);
+		data->fw_version[5] = '\0';
+		goto out;
+	}
+
+	data->query_result = -EPROTO;
+
+out:
+	complete(&data->wait_event);
+}
+
 static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
 {
 	struct exc3000_data *data = dev_id;
@@ -129,6 +163,11 @@ static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
 	int slots, total_slots;
 	int error;
 
+	if (mutex_is_locked(&data->query_lock)) {
+		exc3000_query_interrupt(data);
+		goto out;
+	}
+
 	error = exc3000_read_data(data->client, buf, &total_slots);
 	if (error) {
 		/* Schedule a timer to release "stuck" contacts */
@@ -156,12 +195,92 @@ static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int exc3000_get_fw_version(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct exc3000_data *data = dev_get_drvdata(dev);
+	static const u8 request[68] =
+		{0x67, 0x00, 0x42, 0x00, 0x03, 0x01, 'D', 0x00};
+	struct i2c_client *client = data->client;
+	int err;
+
+	mutex_lock(&data->query_lock);
+
+	data->query_result = -ETIMEDOUT;
+	reinit_completion(&data->wait_event);
+
+	err = i2c_master_send(client, request, sizeof(request));
+	if (err < 0) {
+		mutex_unlock(&data->query_lock);
+		return err;
+	}
+
+	wait_for_completion_interruptible_timeout(&data->wait_event, 1*HZ);
+	mutex_unlock(&data->query_lock);
+
+	if (data->query_result < 0)
+		return data->query_result;
+
+	return sprintf(buf, "%s\n", data->fw_version);
+}
+static DEVICE_ATTR(fw_version, S_IRUGO, exc3000_get_fw_version, NULL);
+
+static ssize_t exc3000_get_model(struct exc3000_data *data)
+{
+	static const u8 request[68] =
+		{0x67, 0x00, 0x42, 0x00, 0x03, 0x01, 'E', 0x00};
+	struct i2c_client *client = data->client;
+	int err;
+
+	mutex_lock(&data->query_lock);
+	data->query_result = -ETIMEDOUT;
+	reinit_completion(&data->wait_event);
+
+	err = i2c_master_send(client, request, sizeof(request));
+	if (err < 0) {
+		mutex_unlock(&data->query_lock);
+		return err;
+	}
+
+	wait_for_completion_interruptible_timeout(&data->wait_event, 1*HZ);
+	mutex_unlock(&data->query_lock);
+
+	return data->query_result;
+}
+
+static ssize_t exc3000_get_model_sysfs(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct exc3000_data *data = dev_get_drvdata(dev);
+	int err = exc3000_get_model(data);
+
+	if (err < 0)
+		return err;
+
+	return sprintf(buf, "%s\n", data->model);
+}
+static DEVICE_ATTR(model, S_IRUGO, exc3000_get_model_sysfs, NULL);
+
+static struct attribute *sysfs_attrs[] = {
+	&dev_attr_model.attr,
+	&dev_attr_fw_version.attr,
+	NULL
+};
+
+static struct attribute_group exc3000_attribute_group = {
+	.attrs = sysfs_attrs
+};
+
+static const struct attribute_group *exc3000_attribute_groups[] = {
+	&exc3000_attribute_group,
+	NULL
+};
+
 static int exc3000_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
 	struct exc3000_data *data;
 	struct input_dev *input;
 	int error;
+	int retry;
 
 	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
@@ -170,15 +289,19 @@ static int exc3000_probe(struct i2c_client *client,
 	data->client = client;
 	data->type = id->driver_data;
 	timer_setup(&data->timer, exc3000_timer, 0);
+	init_completion(&data->wait_event);
+	mutex_init(&data->query_lock);
 
 	input = devm_input_allocate_device(&client->dev);
 	if (!input)
 		return -ENOMEM;
 
 	data->input = input;
+	input_set_drvdata(input, data);
 
 	input->name = "EETI EXC3000 Touch Screen";
 	input->id.bustype = BUS_I2C;
+	input->dev.groups = exc3000_attribute_groups;
 
 	if (data->type == EETI_EXC80Hxx) {
 		input_set_abs_params(input, ABS_MT_POSITION_X, 0, 16383, 0, 0);
@@ -204,6 +327,19 @@ static int exc3000_probe(struct i2c_client *client,
 	if (error)
 		return error;
 
+	for (retry = 0; retry < 3; ++retry) {
+		error = exc3000_get_model(data);
+		if (!error) {
+			break;
+		}
+		dev_warn(&client->dev, "Retry %d get EETI EXC3000 model: %d\n", retry + 1, error);
+	}
+
+	if (error)
+		return error;
+
+	dev_dbg(&client->dev, "TS Model: %s", data->model);
+
 	return 0;
 }
 
-- 
2.24.0.rc1

Re: [PATCHv1 1/2] Input: EXC3000: add EXC80Hxx support

[Dmitry Torokhov]

On Thu, Nov 07, 2019 at 07:10:09PM +0100, Sebastian Reichel wrote:
> This adds support for EXC80Hxx controllers, which uses
> a different event type id and has two extra bits for the
> resolution (so the maximum is 16384 instead of 4096).
> 
> The patch has been tested with EXC80H60 and EXC80H84.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../bindings/input/touchscreen/exc3000.txt    |  6 ++--
>  drivers/input/touchscreen/exc3000.c           | 34 ++++++++++++++-----
>  2 files changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/exc3000.txt b/Documentation/devicetree/bindings/input/touchscreen/exc3000.txt
> index 68291b94fec2..057b680f0420 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/exc3000.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/exc3000.txt
> @@ -1,7 +1,9 @@
> -* EETI EXC3000 Multiple Touch Controller
> +* EETI EXC3000 and EXC80Hxx Multiple Touch Controller
>  
>  Required properties:
> -- compatible: must be "eeti,exc3000"
> +- compatible: must be one of
> + * "eeti,exc3000"
> + * "eeti,exc80hxx"

Rob is saying "no wildcard compatibles"... If both chips are the same
from software POV we should use "eeti,exc80h60".

>  - reg: i2c slave address
>  - interrupts: touch controller interrupt
>  - touchscreen-size-x: See touchscreen.txt
> diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
> index e007e2e8f626..7d695022082c 100644
> --- a/drivers/input/touchscreen/exc3000.c
> +++ b/drivers/input/touchscreen/exc3000.c
> @@ -23,11 +23,20 @@
>  #define EXC3000_SLOTS_PER_FRAME		5
>  #define EXC3000_LEN_FRAME		66
>  #define EXC3000_LEN_POINT		10
> -#define EXC3000_MT_EVENT		6
> +
> +#define EXC3000_MT1_EVENT		0x06
> +#define EXC3000_MT2_EVENT		0x18
> +
>  #define EXC3000_TIMEOUT_MS		100
>  
> +enum exc3000_device_type {
> +	EETI_EXC3000,
> +	EETI_EXC80Hxx
> +};
> +
>  struct exc3000_data {
>  	struct i2c_client *client;
> +	enum exc3000_device_type type;
>  	struct input_dev *input;
>  	struct touchscreen_properties prop;
>  	struct timer_list timer;
> @@ -76,8 +85,10 @@ static int exc3000_read_frame(struct i2c_client *client, u8 *buf)
>  	if (ret != EXC3000_LEN_FRAME)
>  		return -EIO;
>  
> -	if (get_unaligned_le16(buf) != EXC3000_LEN_FRAME ||
> -			buf[2] != EXC3000_MT_EVENT)
> +	if (get_unaligned_le16(buf) != EXC3000_LEN_FRAME)
> +		return -EINVAL;
> +
> +	if (buf[2] != EXC3000_MT1_EVENT && buf[2] != EXC3000_MT2_EVENT)
>  		return -EINVAL;
>  
>  	return 0;
> @@ -157,6 +168,7 @@ static int exc3000_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  
>  	data->client = client;
> +	data->type = id->driver_data;
>  	timer_setup(&data->timer, exc3000_timer, 0);
>  
>  	input = devm_input_allocate_device(&client->dev);
> @@ -168,8 +180,13 @@ static int exc3000_probe(struct i2c_client *client,
>  	input->name = "EETI EXC3000 Touch Screen";
>  	input->id.bustype = BUS_I2C;
>  
> -	input_set_abs_params(input, ABS_MT_POSITION_X, 0, 4095, 0, 0);
> -	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 4095, 0, 0);
> +	if (data->type == EETI_EXC80Hxx) {

I'd say

	max_xy = (data->type == EETI_EXC80Hxx ? SZ_16K : SZ_4K) - 1;

> +		input_set_abs_params(input, ABS_MT_POSITION_X, 0, 16383, 0, 0);
> +		input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 16383, 0, 0);
> +	} else {
> +		input_set_abs_params(input, ABS_MT_POSITION_X, 0, 4095, 0, 0);
> +		input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 4095, 0, 0);
> +	}
>  	touchscreen_parse_properties(input, true, &data->prop);
>  
>  	error = input_mt_init_slots(input, EXC3000_NUM_SLOTS,
> @@ -191,14 +208,15 @@ static int exc3000_probe(struct i2c_client *client,
>  }
>  
>  static const struct i2c_device_id exc3000_id[] = {
> -	{ "exc3000", 0 },
> -	{ }
> +	{ "exc3000", EETI_EXC3000 },
> +	{ "exc80hxx", EETI_EXC80Hxx }
>  };
>  MODULE_DEVICE_TABLE(i2c, exc3000_id);
>  
>  #ifdef CONFIG_OF
>  static const struct of_device_id exc3000_of_match[] = {
> -	{ .compatible = "eeti,exc3000" },
> +	{ .compatible = "eeti,exc3000", .data = (const void*) EETI_EXC3000 },
> +	{ .compatible = "eeti,exc80hxx", .data = (const void*) EETI_EXC80Hxx },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, exc3000_of_match);
> -- 
> 2.24.0.rc1
> 

Thanks.

-- 
Dmitry

Re: [PATCHv1 2/2] Input: EXC3000: Add support to query model and fw_version

[Dmitry Torokhov]

Hi Sebastian,

On Thu, Nov 07, 2019 at 07:10:10PM +0100, Sebastian Reichel wrote:
> Expose model and fw_version via sysfs. Also query the model
> in probe to make sure, that the I2C communication with the
> device works before successfully probing the driver.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  drivers/input/touchscreen/exc3000.c | 136 ++++++++++++++++++++++++++++
>  1 file changed, 136 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
> index 7d695022082c..4c9f132629b9 100644
> --- a/drivers/input/touchscreen/exc3000.c
> +++ b/drivers/input/touchscreen/exc3000.c
> @@ -41,6 +41,11 @@ struct exc3000_data {
>  	struct touchscreen_properties prop;
>  	struct timer_list timer;
>  	u8 buf[2 * EXC3000_LEN_FRAME];
> +	char model[11];
> +	char fw_version[6];
> +	struct completion wait_event;
> +	struct mutex query_lock;
> +	int query_result;
>  };
>  
>  static void exc3000_report_slots(struct input_dev *input,
> @@ -121,6 +126,35 @@ static int exc3000_read_data(struct i2c_client *client,
>  	return 0;
>  }
>  
> +static void exc3000_query_interrupt(struct exc3000_data *data)
> +{
> +	u8 *buf = data->buf;
> +	int err;
> +
> +	data->query_result = 0;
> +
> +	err = i2c_master_recv(data->client, buf, EXC3000_LEN_FRAME);
> +	if (err < 0) {
> +		data->query_result = err;
> +		goto out;
> +	}
> +
> +	if (buf[0] == 0x42 && buf[4] == 'E') {
> +		memcpy(data->model, buf+5, 10);
> +		data->model[10] = '\0';
> +		goto out;
> +	} else if (buf[0] == 0x42 && buf[4] == 'D') {
> +		memcpy(data->fw_version, buf+5, 5);
> +		data->fw_version[5] = '\0';
> +		goto out;
> +	}
> +
> +	data->query_result = -EPROTO;
> +
> +out:
> +	complete(&data->wait_event);
> +}
> +
>  static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
>  {
>  	struct exc3000_data *data = dev_id;
> @@ -129,6 +163,11 @@ static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
>  	int slots, total_slots;
>  	int error;
>  
> +	if (mutex_is_locked(&data->query_lock)) {
> +		exc3000_query_interrupt(data);
> +		goto out;
> +	}
> +
>  	error = exc3000_read_data(data->client, buf, &total_slots);
>  	if (error) {
>  		/* Schedule a timer to release "stuck" contacts */
> @@ -156,12 +195,92 @@ static irqreturn_t exc3000_interrupt(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static int exc3000_get_fw_version(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct exc3000_data *data = dev_get_drvdata(dev);
> +	static const u8 request[68] =
> +		{0x67, 0x00, 0x42, 0x00, 0x03, 0x01, 'D', 0x00};
> +	struct i2c_client *client = data->client;
> +	int err;
> +
> +	mutex_lock(&data->query_lock);
> +
> +	data->query_result = -ETIMEDOUT;
> +	reinit_completion(&data->wait_event);
> +
> +	err = i2c_master_send(client, request, sizeof(request));
> +	if (err < 0) {
> +		mutex_unlock(&data->query_lock);
> +		return err;
> +	}
> +
> +	wait_for_completion_interruptible_timeout(&data->wait_event, 1*HZ);
> +	mutex_unlock(&data->query_lock);
> +
> +	if (data->query_result < 0)
> +		return data->query_result;
> +
> +	return sprintf(buf, "%s\n", data->fw_version);
> +}
> +static DEVICE_ATTR(fw_version, S_IRUGO, exc3000_get_fw_version, NULL);
> +
> +static ssize_t exc3000_get_model(struct exc3000_data *data)
> +{
> +	static const u8 request[68] =
> +		{0x67, 0x00, 0x42, 0x00, 0x03, 0x01, 'E', 0x00};
> +	struct i2c_client *client = data->client;
> +	int err;
> +
> +	mutex_lock(&data->query_lock);
> +	data->query_result = -ETIMEDOUT;
> +	reinit_completion(&data->wait_event);
> +
> +	err = i2c_master_send(client, request, sizeof(request));
> +	if (err < 0) {
> +		mutex_unlock(&data->query_lock);
> +		return err;
> +	}
> +
> +	wait_for_completion_interruptible_timeout(&data->wait_event, 1*HZ);
> +	mutex_unlock(&data->query_lock);
> +
> +	return data->query_result;
> +}
> +
> +static ssize_t exc3000_get_model_sysfs(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct exc3000_data *data = dev_get_drvdata(dev);
> +	int err = exc3000_get_model(data);

Do we really need to re-fetch model (and firmware ID) on each access?
Can we query it as probe time and cache? This I think would simplify the
driver, as you probably would not need to hook it into the ISR. Can you
just post a read/write transaction to fetch it without waiting for
interrupt? Or, if single transaction does not work and you need to wait
for certain time for response - just add msleep() and maybe mark driver
for async probe...

Thanks.

-- 
Dmitry

Re: [PATCHv1 1/2] Input: EXC3000: add EXC80Hxx support

[Martin Fuzzey]

Hi Sebastien,

On 07/11/2019 19:10, Sebastian Reichel wrote:
> This adds support for EXC80Hxx controllers, which uses
> a different event type id and has two extra bits for the
> resolution (so the maximum is 16384 instead of 4096).
>
> The patch has been tested with EXC80H60 and EXC80H84.


Ah I'm doing the same thing :)


> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>   .../bindings/input/touchscreen/exc3000.txt    |  6 ++--
>   drivers/input/touchscreen/exc3000.c           | 34 ++++++++++++++-----
>   2 files changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/exc3000.txt b/Documentation/devicetree/bindings/input/touchscreen/exc3000.txt
> index 68291b94fec2..057b680f0420 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/exc3000.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/exc3000.txt
> @@ -1,7 +1,9 @@
> -* EETI EXC3000 Multiple Touch Controller
> +* EETI EXC3000 and EXC80Hxx Multiple Touch Controller
>   
>   Required properties:
> -- compatible: must be "eeti,exc3000"
> +- compatible: must be one of
> + * "eeti,exc3000"
> + * "eeti,exc80hxx"
>   - reg: i2c slave address
>   - interrupts: touch controller interrupt
>   - touchscreen-size-x: See touchscreen.txt
> diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
> index e007e2e8f626..7d695022082c 100644
> --- a/drivers/input/touchscreen/exc3000.c
> +++ b/drivers/input/touchscreen/exc3000.c
> @@ -23,11 +23,20 @@
>   #define EXC3000_SLOTS_PER_FRAME		5
>   #define EXC3000_LEN_FRAME		66
>   #define EXC3000_LEN_POINT		10
> -#define EXC3000_MT_EVENT		6
> +
> +#define EXC3000_MT1_EVENT		0x06
> +#define EXC3000_MT2_EVENT		0x18
> +
>   #define EXC3000_TIMEOUT_MS		100
>   
> +enum exc3000_device_type {
> +	EETI_EXC3000,
> +	EETI_EXC80Hxx
> +};
> +
>   struct exc3000_data {
>   	struct i2c_client *client;
> +	enum exc3000_device_type type;
>   	struct input_dev *input;
>   	struct touchscreen_properties prop;
>   	struct timer_list timer;
> @@ -76,8 +85,10 @@ static int exc3000_read_frame(struct i2c_client *client, u8 *buf)
>   	if (ret != EXC3000_LEN_FRAME)
>   		return -EIO;
>   
> -	if (get_unaligned_le16(buf) != EXC3000_LEN_FRAME ||
> -			buf[2] != EXC3000_MT_EVENT)
> +	if (get_unaligned_le16(buf) != EXC3000_LEN_FRAME)
> +		return -EINVAL;
> +
> +	if (buf[2] != EXC3000_MT1_EVENT && buf[2] != EXC3000_MT2_EVENT)
>   		return -EINVAL;
>   


Are we sure the EXC80 will *always* use MT2_EVENT (and that there is not 
some configuration where it can send MT1_EVENTS).

Because, if this happens we will have declared a 14 bit resolution to 
the input core but  process a 12 bit value.

I think it is better to declare the resolution based on the device type 
(as you have done) but interpret the data based on the MT1 vs MT2 event 
code, shifting 2 bits if needed.



> @@ -168,8 +180,13 @@ static int exc3000_probe(struct i2c_client *client,
>   	input->name = "EETI EXC3000 Touch Screen";

Shouldn't the name change too?


>   	input->id.bustype = BUS_I2C;
>   
> -	input_set_abs_params(input, ABS_MT_POSITION_X, 0, 4095, 0, 0);
> -	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 4095, 0, 0);
> +	if (data->type == EETI_EXC80Hxx) {
> +		input_set_abs_params(input, ABS_MT_POSITION_X, 0, 16383, 0, 0);
> +		input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 16383, 0, 0);
> +	} else {
> +		input_set_abs_params(input, ABS_MT_POSITION_X, 0, 4095, 0, 0);
> +		input_set_abs_params(input, ABS_MT_POSITION_Y, 0, 4095, 0, 0);
> +	}
>   	touchscreen_parse_properties(input, true, &data->prop);
>   
>   	error = input_mt_init_slots(input, EXC3000_NUM_SLOTS,
> @@ -191,14 +208,15 @@ static int exc3000_probe(struct i2c_client *client,
>   }
>   
>   static const struct i2c_device_id exc3000_id[] = {
> -	{ "exc3000", 0 },
> -	{ }
> +	{ "exc3000", EETI_EXC3000 },
> +	{ "exc80hxx", EETI_EXC80Hxx }
>   };
>   MODULE_DEVICE_TABLE(i2c, exc3000_id);
>   
>   #ifdef CONFIG_OF
>   static const struct of_device_id exc3000_of_match[] = {
> -	{ .compatible = "eeti,exc3000" },
> +	{ .compatible = "eeti,exc3000", .data = (const void*) EETI_EXC3000 },
> +	{ .compatible = "eeti,exc80hxx", .data = (const void*) EETI_EXC80Hxx },
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(of, exc3000_of_match);

If driver_data is defined in the I2C device table and the compatible 
string, minus the vendor prefix, matches the i2c device id it is not 
necessary to define the driver_data in the of_match table as well.


How about defining a "exc_device_info" structure, containing name, 
resolution etc and storing a pointer to a constant instance of that in 
the match tables rather than just a type code.

That way the "if (data->type)" tests could be removed and the info 
structure accessed directly.


Something like:

struct exc_dev_info {
     const char *name;
     u16 x_res;
     u16 y_res;
};

static int exc3000_probe(struct i2c_client *client,
              const struct i2c_device_id *id)
{
     const struct exc_dev_info *dev_info;
...

     dev_info = (const struct exc_dev_info *)id->driver_data;

...

     input->name = dev_info->name;
     input->id.bustype = BUS_I2C;

     input_set_abs_params(input, ABS_MT_POSITION_X, 0,
                  dev_info->x_res - 1, 0, 0);
     input_set_abs_params(input, ABS_MT_POSITION_Y, 0,
                  dev_info->y_res - 1, 0, 0);
     touchscreen_parse_properties(input, true, &data->prop);


static const struct exc_dev_info exc3000_dev_info = {
     .name = "EETI EXC3000 Touch Screen",
     .x_res = 4096,
     .y_res = 4096,
};

static const struct exc_dev_info exc80_dev_info = {
     .name = "EETI EXC80 Touch Screen",
     .x_res = 16384,
     .y_res = 16384,
};

static const struct i2c_device_id exc3000_id[] = {
     { "exc3000",    (kernel_ulong_t)&exc3000_dev_info },
     { "exc80",    (kernel_ulong_t)&exc80_dev_info },
     { }
};
MODULE_DEVICE_TABLE(i2c, exc3000_id);


One other thing I have noticed is that the exc80h does not appear to 
need the i2c_send before read that is done in exc3000_read_frame()

The vendor "egalax_i2c" driver doesn't do it either and I can't see 
anything in the "I2C programming guide" mentionning this.


Ahmet's original commit introducing the driver says

" To be able to work with the higher 400KHz I2C bus rate, one must
   successfully send a special package prior _each_ read or the controller
   will refuse to cooperate."


This doesn't seem to be an issue at 400KHz for the 80h at least, though 
keeping it doesn't seem to hurt either.

Maybe make this device dependant too?


Regards,

Martin

Re: Re: [PATCHv1 2/2] Input: EXC3000: Add support to query model and fw_version

[Martin Fuzzey]

Hi Dmitry,

On 13/11/2019 01:23, Dmitry Torokhov wrote:
> Do we really need to re-fetch model (and firmware ID) on each access?
> Can we query it as probe time and cache? This I think would simplify the
> driver, as you probably would not need to hook it into the ISR. Can you
> just post a read/write transaction to fetch it without waiting for
> interrupt? Or, if single transaction does not work and you need to wait
> for certain time for response - just add msleep() and maybe mark driver
> for async probe...


Having the sysfs access actually read the data from the device can be 
useful to check that the I2C link is still working (in a test scenario).


The documentation does say that one should wait for an interrupt after 
issuing the commands.

The msleep() could work but the value would have to be empirical and 
could be fragile.

Furthermore what happens if a touch event occurs just after sending the 
query request?

Having the interrupt handler be the single read() point and dispatching 
solves that problem, even if it does complicate the driver to some extent.


One further thing is that there are other commands that may be added in 
the future that cannot be cached (commands to do tests or fimware 
updates for example).


Regards,


Martin

Re: [PATCHv1 2/2] Input: EXC3000: Add support to query model and fw_version

[Martin Fuzzey]

Hi Sebastian,

On 07/11/2019 19:10, Sebastian Reichel wrote:
> Expose model and fw_version via sysfs. Also query the model
> in probe to make sure, that the I2C communication with the
> device works before successfully probing the driver.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>   drivers/input/touchscreen/exc3000.c | 136 ++++++++++++++++++++++++++++
>   1 file changed, 136 insertions(+)
>
> diff --git a/drivers/input/touchscreen/exc3000.c b/drivers/input/touchscreen/exc3000.c
> index 7d695022082c..4c9f132629b9 100644
> --- a/drivers/input/touchscreen/exc3000.c
> +++ b/drivers/input/touchscreen/exc3000.c
> @@ -41,6 +41,11 @@ struct exc3000_data {
>   	struct touchscreen_properties prop;
>   	struct timer_list timer;
>   	u8 buf[2 * EXC3000_LEN_FRAME];
> +	char model[11];
> +	char fw_version[6];

These buffers are too small.

I know those are the values shown in the EETI "I2C Programming Guide" 
but, on my exc80H32 I have 16 bytes for the model and 14 for the fw_version.

It may even be possible to have larger, depends on the firmware / config 
blob that has been loaded.

The guide seems to be mostly an example. As nothing else is sent in the 
reply message, worst case the full frame would be filled.


>   
> +static void exc3000_query_interrupt(struct exc3000_data *data)
> +{
> +	u8 *buf = data->buf;
> +	int err;
> +
> +	data->query_result = 0;
> +
> +	err = i2c_master_recv(data->client, buf, EXC3000_LEN_FRAME);
> +	if (err < 0) {
> +		data->query_result = err;
> +		goto out;
> +	}
> +
> +	if (buf[0] == 0x42 && buf[4] == 'E') {
> +		memcpy(data->model, buf+5, 10);
> +		data->model[10] = '\0';

Maybe use sizeof() to avoid the hard coded 10?

> +		goto out;
> +	} else if (buf[0] == 0x42 && buf[4] == 'D') {
> +		memcpy(data->fw_version, buf+5, 5);
> +		data->fw_version[5] = '\0';
> +		goto out;
> +	}
> +


Ok so at least there won't be a buffer overflow but it will truncate.


> +static DEVICE_ATTR(fw_version, S_IRUGO, exc3000_get_fw_version, NULL);
> +


Maybe use DEVICE_ATTR_RO  to reduce the amount of boilerplate?


> +	input->dev.groups = exc3000_attribute_groups;


Hum, this adds the attributes to the input device (in /sys/class/input) 
but these attributes are really for the whole I2C device.

Wouldn't it be better to use devm_device_add_group() to add them to the 
I2C device?


> +	for (retry = 0; retry < 3; ++retry) {
> +		error = exc3000_get_model(data);
> +		if (!error) {
> +			break;
> +		}
> +		dev_warn(&client->dev, "Retry %d get EETI EXC3000 model: %d\n", retry + 1, error);
> +	}
> +


Is there a particular reason why retries are needed?


Regards,


Martin