[v5,2/2] input: zinitix: Add touchkey support

Commit Message

Nikita Travkin July 17, 2024, 1:55 p.m. UTC

Zinitix touch controllers can use some of the sense lines for virtual
keys (like those found on many phones). Add support for those keys.

Reviewed-by: Linus Walleij <[e-mail]>
Signed-off-by: Nikita Travkin <[e-mail]>
---
 drivers/input/touchscreen/zinitix.c | 63 +++++++++++++++++++++++++++++++++++--
 1 file changed, 60 insertions(+), 3 deletions(-)

Comments

Dmitry Torokhov July 20, 2024, 12:29 a.m. UTC | #1

On Wed, Jul 17, 2024 at 06:55:34PM +0500, Nikita Travkin wrote:
> Zinitix touch controllers can use some of the sense lines for virtual
> keys (like those found on many phones). Add support for those keys.
> 
> Reviewed-by: Linus Walleij <[e-mail]>
> Signed-off-by: Nikita Travkin <[e-mail]>

Applied, thank you. However:

> -->  
> +	if (le16_to_cpu(touch_event.status) & BIT_ICON_EVENT) {
> +		error = zinitix_read_data(bt541->client, ZINITIX_ICON_STATUS_REG,
> +					  &icon_events, sizeof(icon_events));
> +		if (error) {
> +			dev_err(&client->dev, "Failed to read icon events\n");
> +			goto out;
> +		}

I wonder, would it make sense (and be more efficient) to issue a single
read of size sizeof(struct touch_event) + sizeof(icon_events) and the
parse the data based on touch_event.status?

Thanks.

Nikita Travkin July 20, 2024, 6:57 a.m. UTC | #2

Dmitry Torokhov писал(а) 20.07.2024 05:29:
> On Wed, Jul 17, 2024 at 06:55:34PM +0500, Nikita Travkin wrote:
>> Zinitix touch controllers can use some of the sense lines for virtual
>> keys (like those found on many phones). Add support for those keys.
>>
>> Reviewed-by: Linus Walleij <[e-mail]>
>> Signed-off-by: Nikita Travkin <[e-mail]>
> 
> Applied, thank you. However:
> 
>> -->
>> +	if (le16_to_cpu(touch_event.status) & BIT_ICON_EVENT) {
>> +		error = zinitix_read_data(bt541->client, ZINITIX_ICON_STATUS_REG,
>> +					  &icon_events, sizeof(icon_events));
>> +		if (error) {
>> +			dev_err(&client->dev, "Failed to read icon events\n");
>> +			goto out;
>> +		}
> 
> I wonder, would it make sense (and be more efficient) to issue a single
> read of size sizeof(struct touch_event) + sizeof(icon_events) and the
> parse the data based on touch_event.status?

Maybe, but I would be really hesitant to such a change: Original driver
also makes a dedicated read for the "icon" data and per my understanding,
those "register reads" may also not be really "register" based but rather
kind of "command" based, where controller will start streaming the data
based on the request for the specific "register". In this case i'd prefer
to not accidentally confuse the touch firmware by over-reading the data,
if its somehow firmware-version-defined.

Thanks for giving it a look and picking this up!
Nikita

> 
> Thanks.

Patch

diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c
index 1b4807ba4624..1df93c96f6bf 100644
--- a/drivers/input/touchscreen/zinitix.c
+++ b/drivers/input/touchscreen/zinitix.c
@@ -10,6 +10,7 @@ 
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/property.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
@@ -119,6 +120,7 @@ 
 
 #define DEFAULT_TOUCH_POINT_MODE		2
 #define MAX_SUPPORTED_FINGER_NUM		5
+#define MAX_SUPPORTED_BUTTON_NUM		8
 
 #define CHIP_ON_DELAY				15 // ms
 #define FIRMWARE_ON_DELAY			40 // ms
@@ -146,6 +148,8 @@  struct bt541_ts_data {
 	struct touchscreen_properties prop;
 	struct regulator_bulk_data supplies[2];
 	u32 zinitix_mode;
+	u32 keycodes[MAX_SUPPORTED_BUTTON_NUM];
+	int num_keycodes;
 };
 
 static int zinitix_read_data(struct i2c_client *client,
@@ -195,6 +199,7 @@  static int zinitix_init_touch(struct bt541_ts_data *bt541)
 	struct i2c_client *client = bt541->client;
 	int i;
 	int error;
+	u16 int_flags;
 
 	error = zinitix_write_cmd(client, ZINITIX_SWRESET_CMD);
 	if (error) {
@@ -225,6 +230,11 @@  static int zinitix_init_touch(struct bt541_ts_data *bt541)
 	if (error)
 		return error;
 
+	error = zinitix_write_u16(client, ZINITIX_BUTTON_SUPPORTED_NUM,
+				  bt541->num_keycodes);
+	if (error)
+		return error;
+
 	error = zinitix_write_u16(client, ZINITIX_INITIAL_TOUCH_MODE,
 				  bt541->zinitix_mode);
 	if (error)
@@ -235,9 +245,11 @@  static int zinitix_init_touch(struct bt541_ts_data *bt541)
 	if (error)
 		return error;
 
-	error = zinitix_write_u16(client, ZINITIX_INT_ENABLE_FLAG,
-				  BIT_PT_CNT_CHANGE | BIT_DOWN | BIT_MOVE |
-					BIT_UP);
+	int_flags = BIT_PT_CNT_CHANGE | BIT_DOWN | BIT_MOVE | BIT_UP;
+	if (bt541->num_keycodes)
+		int_flags |= BIT_ICON_EVENT;
+
+	error = zinitix_write_u16(client, ZINITIX_INT_ENABLE_FLAG, int_flags);
 	if (error)
 		return error;
 
@@ -350,12 +362,22 @@  static void zinitix_report_finger(struct bt541_ts_data *bt541, int slot,
 	}
 }
 
+static void zinitix_report_keys(struct bt541_ts_data *bt541, u16 icon_events)
+{
+	int i;
+
+	for (i = 0; i < bt541->num_keycodes; i++)
+		input_report_key(bt541->input_dev,
+				 bt541->keycodes[i], !!(icon_events & BIT(i)));
+}
+
 static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
 {
 	struct bt541_ts_data *bt541 = bt541_handler;
 	struct i2c_client *client = bt541->client;
 	struct touch_event touch_event;
 	unsigned long finger_mask;
+	__le16 icon_events;
 	int error;
 	int i;
 
@@ -368,6 +390,17 @@  static irqreturn_t zinitix_ts_irq_handler(int irq, void *bt541_handler)
 		goto out;
 	}
 
+	if (le16_to_cpu(touch_event.status) & BIT_ICON_EVENT) {
+		error = zinitix_read_data(bt541->client, ZINITIX_ICON_STATUS_REG,
+					  &icon_events, sizeof(icon_events));
+		if (error) {
+			dev_err(&client->dev, "Failed to read icon events\n");
+			goto out;
+		}
+
+		zinitix_report_keys(bt541, le16_to_cpu(icon_events));
+	}
+
 	finger_mask = touch_event.finger_mask;
 	for_each_set_bit(i, &finger_mask, MAX_SUPPORTED_FINGER_NUM) {
 		const struct point_coord *p = &touch_event.point_coord[i];
@@ -453,6 +486,7 @@  static int zinitix_init_input_dev(struct bt541_ts_data *bt541)
 {
 	struct input_dev *input_dev;
 	int error;
+	int i;
 
 	input_dev = devm_input_allocate_device(&bt541->client->dev);
 	if (!input_dev) {
@@ -470,6 +504,14 @@  static int zinitix_init_input_dev(struct bt541_ts_data *bt541)
 	input_dev->open = zinitix_input_open;
 	input_dev->close = zinitix_input_close;
 
+	if (bt541->num_keycodes) {
+		input_dev->keycode = bt541->keycodes;
+		input_dev->keycodemax = bt541->num_keycodes;
+		input_dev->keycodesize = sizeof(bt541->keycodes[0]);
+		for (i = 0; i < bt541->num_keycodes; i++)
+			input_set_capability(input_dev, EV_KEY, bt541->keycodes[i]);
+	}
+
 	input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
 	input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
 	input_set_abs_params(input_dev, ABS_MT_WIDTH_MAJOR, 0, 255, 0, 0);
@@ -534,6 +576,21 @@  static int zinitix_ts_probe(struct i2c_client *client)
 		return error;
 	}
 
+	bt541->num_keycodes = device_property_count_u32(&client->dev, "linux,keycodes");
+	if (bt541->num_keycodes > ARRAY_SIZE(bt541->keycodes)) {
+		dev_err(&client->dev, "too many keys defined (%d)\n", bt541->num_keycodes);
+		return -EINVAL;
+	}
+
+	error = device_property_read_u32_array(&client->dev, "linux,keycodes",
+					       bt541->keycodes,
+					       bt541->num_keycodes);
+	if (error) {
+		dev_err(&client->dev,
+			"Unable to parse \"linux,keycodes\" property: %d\n", error);
+		return error;
+	}
+
 	error = zinitix_init_input_dev(bt541);
 	if (error) {
 		dev_err(&client->dev,