Received-SPF: pass (google.com: domain of linux-kernel+bounces-23143-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 10 Jan 2024 23:20:49 -0800
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Neil Armstrong <neil.armstrong@linaro.org>
Cc: linux-input@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Bastien Nocera <hadess@hadess.net>,
	Hans de Goede <hdegoede@redhat.com>,
	Henrik Rydberg <rydberg@bitmath.org>,
	Jeff LaBundy <jeff@labundy.com>, linux-arm-msm@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v14 2/4] Input: add core support for Goodix Berlin
 Touchscreen IC
Message-ID: <ZZ-W0UPHOdpU-8el@google.com>
References: <20231221-topic-goodix-berlin-upstream-initial-v14-0-04459853b640@linaro.org>
 <20231221-topic-goodix-berlin-upstream-initial-v14-2-04459853b640@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231221-topic-goodix-berlin-upstream-initial-v14-2-04459853b640@linaro.org>

Hi Neil,

On Thu, Dec 21, 2023 at 04:21:20PM +0100, Neil Armstrong wrote:
> Add initial support for the new Goodix "Berlin" touchscreen ICs.

Thank you very much for explaining how reading of additional contacts
and checksum works, it makes sense now.

I was a bit unhappy about number of times we copy/move the data over;
could you please try the patch below to see if the device still works
with it?

I also shortened some #defines and defines some additional structures.
Also as far as I can see not everything needs to be packed as the data
is naturally aligned on the word boundaries.

Thanks!

-- 
Dmitry


diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
index 6aca57e6b5d6..d1f1c0474116 100644
--- a/drivers/input/touchscreen/goodix_berlin_core.c
+++ b/drivers/input/touchscreen/goodix_berlin_core.c
@@ -39,19 +39,6 @@
 
 #define GOODIX_BERLIN_NORMAL_RESET_DELAY_MS	100
 
-#define GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN	8
-#define GOODIX_BERLIN_STATUS_OFFSET		0
-#define GOODIX_BERLIN_REQUEST_TYPE_OFFSET	2
-
-#define GOODIX_BERLIN_BYTES_PER_POINT		8
-#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE	2
-#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK	GENMASK(15, 0)
-
-/* Read n finger events */
-#define GOODIX_BERLIN_IRQ_READ_LEN(n)		(GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN + \
-						 (GOODIX_BERLIN_BYTES_PER_POINT * (n)) + \
-						 GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
-
 #define GOODIX_BERLIN_TOUCH_EVENT		BIT(7)
 #define GOODIX_BERLIN_REQUEST_EVENT		BIT(6)
 #define GOODIX_BERLIN_TOUCH_COUNT_MASK		GENMASK(3, 0)
@@ -71,6 +58,8 @@
 #define GOODIX_BERLIN_IC_INFO_MAX_LEN		SZ_1K
 #define GOODIX_BERLIN_IC_INFO_ADDR		0x10070
 
+#define GOODIX_BERLIN_CHECKSUM_SIZE		sizeof(u16)
+
 struct goodix_berlin_fw_version {
 	u8 rom_pid[6];
 	u8 rom_vid[3];
@@ -81,7 +70,7 @@ struct goodix_berlin_fw_version {
 	u8 sensor_id;
 	u8 reserved[2];
 	__le16 checksum;
-} __packed;
+};
 
 struct goodix_berlin_ic_info_version {
 	u8 info_customer_id;
@@ -147,13 +136,30 @@ struct goodix_berlin_ic_info_misc {
 	__le32 esd_addr;
 } __packed;
 
-struct goodix_berlin_touch_data {
-	u8 id;
-	u8 unused;
+struct goodix_berlin_touch {
+	u8 status;
+	u8 reserved;
 	__le16 x;
 	__le16 y;
 	__le16 w;
-} __packed;
+};
+#define GOODIX_BERLIN_TOUCH_SIZE	sizeof(struct goodix_berlin_touch)
+
+struct goodix_berlin_header {
+	u8 status;
+	u8 reserved1;
+	u8 request_type;
+	u8 reserved2[3];
+	__le16 checksum;
+};
+#define GOODIX_BERLIN_HEADER_SIZE	sizeof(struct goodix_berlin_header)
+
+struct goodix_berlin_event {
+	struct goodix_berlin_header hdr;
+	/* The data below is u16/__le16 aligned */
+	u8 data[GOODIX_BERLIN_TOUCH_SIZE * GOODIX_BERLIN_MAX_TOUCH +
+		GOODIX_BERLIN_CHECKSUM_SIZE];
+};
 
 struct goodix_berlin_core {
 	struct device *dev;
@@ -168,25 +174,25 @@ struct goodix_berlin_core {
 
 	/* Runtime parameters extracted from IC_INFO buffer  */
 	u32 touch_data_addr;
+
+	struct goodix_berlin_event event;
 };
 
 static bool goodix_berlin_checksum_valid(const u8 *data, int size)
 {
 	u32 cal_checksum = 0;
 	u16 r_checksum;
-	u32 i;
+	int i;
 
-	if (size < GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
+	if (size < GOODIX_BERLIN_CHECKSUM_SIZE)
 		return false;
 
-	for (i = 0; i < size - GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE; i++)
+	for (i = 0; i < size - GOODIX_BERLIN_CHECKSUM_SIZE; i++)
 		cal_checksum += data[i];
 
-	cal_checksum = FIELD_GET(GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK,
-				 cal_checksum);
 	r_checksum = get_unaligned_le16(&data[i]);
 
-	return cal_checksum == r_checksum;
+	return (u16)cal_checksum == r_checksum;
 }
 
 static bool goodix_berlin_is_dummy_data(struct goodix_berlin_core *cd,
@@ -291,33 +297,30 @@ static void goodix_berlin_power_off(struct goodix_berlin_core *cd)
 
 static int goodix_berlin_read_version(struct goodix_berlin_core *cd)
 {
-	u8 buf[sizeof(struct goodix_berlin_fw_version)];
 	int error;
 
 	error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_FW_VERSION_INFO_ADDR,
-				buf, sizeof(buf));
+				&cd->fw_version, sizeof(cd->fw_version));
 	if (error) {
 		dev_err(cd->dev, "error reading fw version, %d\n", error);
 		return error;
 	}
 
-	if (!goodix_berlin_checksum_valid(buf, sizeof(buf))) {
+	if (!goodix_berlin_checksum_valid((u8 *)&cd->fw_version,
+					  sizeof(cd->fw_version))) {
 		dev_err(cd->dev, "invalid fw version: checksum error\n");
 		return -EINVAL;
 	}
 
-	memcpy(&cd->fw_version, buf, sizeof(cd->fw_version));
-
 	return 0;
 }
 
 /* Only extract necessary data for runtime */
-static int goodix_berlin_convert_ic_info(struct goodix_berlin_core *cd,
-					 const u8 *data, u16 length)
+static int goodix_berlin_parse_ic_info(struct goodix_berlin_core *cd,
+				       const u8 *data, u16 length)
 {
-	struct goodix_berlin_ic_info_misc misc;
+	struct goodix_berlin_ic_info_misc *misc;
 	unsigned int offset = 0;
-	u8 param_num;
 
 	offset += sizeof(__le16); /* length */
 	offset += sizeof(struct goodix_berlin_ic_info_version);
@@ -325,49 +328,25 @@ static int goodix_berlin_convert_ic_info(struct goodix_berlin_core *cd,
 
 	/* IC_INFO Parameters, variable width structure */
 	offset += 4 * sizeof(u8); /* drv_num, sen_num, button_num, force_num */
-
-	if (offset >= length)
-		goto invalid_offset;
-
-	param_num = data[offset++]; /* active_scan_rate_num */
-
-	offset += param_num * sizeof(__le16);
-
-	if (offset >= length)
-		goto invalid_offset;
-
-	param_num = data[offset++]; /* mutual_freq_num*/
-
-	offset += param_num * sizeof(__le16);
-
-	if (offset >= length)
-		goto invalid_offset;
-
-	param_num = data[offset++]; /* self_tx_freq_num */
-
-	offset += param_num * sizeof(__le16);
-
-	if (offset >= length)
-		goto invalid_offset;
-
-	param_num = data[offset++]; /* self_rx_freq_num */
-
-	offset += param_num * sizeof(__le16);
-
 	if (offset >= length)
 		goto invalid_offset;
 
-	param_num = data[offset++]; /* stylus_freq_num */
-
-	offset += param_num * sizeof(__le16);
-
-	if (offset + sizeof(misc) > length)
-		goto invalid_offset;
-
-	/* goodix_berlin_ic_info_misc */
-	memcpy(&misc, &data[offset], sizeof(misc));
-
-	cd->touch_data_addr = le32_to_cpu(misc.touch_data_addr);
+#define ADVANCE_LE16_PARAMS()				\
+	do {						\
+		u8 param_num = data[offset++];		\
+		offset += param_num * sizeof(__le16);	\
+		if (offset >= length)			\
+			goto invalid_offset;		\
+	} while (0)
+	ADVANCE_LE16_PARAMS(); /* active_scan_rate_num */
+	ADVANCE_LE16_PARAMS(); /* mutual_freq_num*/
+	ADVANCE_LE16_PARAMS(); /* self_tx_freq_num */
+	ADVANCE_LE16_PARAMS(); /* self_rx_freq_num */
+	ADVANCE_LE16_PARAMS(); /* stylus_freq_num */
+#undef ADVANCE_LE16_PARAMS
+
+	misc = (struct goodix_berlin_ic_info_misc *)&data[offset];
+	cd->touch_data_addr = le32_to_cpu(misc->touch_data_addr);
 
 	return 0;
 
@@ -419,7 +398,7 @@ static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
 		return -EINVAL;
 	}
 
-	error = goodix_berlin_convert_ic_info(cd, afe_data, length);
+	error = goodix_berlin_parse_ic_info(cd, afe_data, length);
 	if (error)
 		return error;
 
@@ -432,20 +411,47 @@ static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
 	return 0;
 }
 
-static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
-				       const void *buf, int touch_num)
+static int goodix_berlin_get_remaining_contacts(struct goodix_berlin_core *cd,
+						int n)
+{
+	size_t offset = 2 * GOODIX_BERLIN_TOUCH_SIZE +
+				GOODIX_BERLIN_CHECKSUM_SIZE;
+	u32 addr = cd->touch_data_addr + GOODIX_BERLIN_HEADER_SIZE + offset;
+	int error;
+
+	error = regmap_raw_read(cd->regmap, addr,
+				&cd->event.data[offset],
+				(n - 2) * GOODIX_BERLIN_TOUCH_SIZE);
+	if (error) {
+		dev_err_ratelimited(cd->dev, "failed to get touch data, %d\n",
+				    error);
+		return error;
+	}
+
+	return 0;
+}
+
+static void goodix_berlin_report_state(struct goodix_berlin_core *cd, int n)
 {
-	const struct goodix_berlin_touch_data *touch_data = buf;
+	struct goodix_berlin_touch *touch_data =
+			(struct goodix_berlin_touch *)cd->event.data;
+	struct goodix_berlin_touch *t;
 	int i;
+	u8 type, id;
+
+	for (i = 0; i < n; i++) {
+		t = &touch_data[i];
 
-	/* Report finger touches */
-	for (i = 0; i < touch_num; i++) {
-		unsigned int id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK,
-					    touch_data[i].id);
+		type = FIELD_GET(GOODIX_BERLIN_POINT_TYPE_MASK, t->status);
+		if (type == GOODIX_BERLIN_POINT_TYPE_STYLUS ||
+		    type == GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER) {
+			dev_warn_once(cd->dev, "Stylus event type not handled\n");
+			continue;
+		}
 
+		id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK, t->status);
 		if (id >= GOODIX_BERLIN_MAX_TOUCH) {
-			dev_warn_ratelimited(cd->dev,
-					     "invalid finger id %d\n", id);
+			dev_warn_ratelimited(cd->dev, "invalid finger id %d\n", id);
 			continue;
 		}
 
@@ -453,69 +459,46 @@ static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
 		input_mt_report_slot_state(cd->input_dev, MT_TOOL_FINGER, true);
 
 		touchscreen_report_pos(cd->input_dev, &cd->props,
-				       __le16_to_cpu(touch_data[i].x),
-				       __le16_to_cpu(touch_data[i].y),
+				       __le16_to_cpu(t->x), __le16_to_cpu(t->y),
 				       true);
 		input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
-				 __le16_to_cpu(touch_data[i].w));
+				 __le16_to_cpu(t->w));
 	}
 
 	input_mt_sync_frame(cd->input_dev);
 	input_sync(cd->input_dev);
 }
 
-static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
-					const void *pre_buf, u32 pre_buf_len)
+static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd)
 {
-	u8 buffer[GOODIX_BERLIN_IRQ_READ_LEN(GOODIX_BERLIN_MAX_TOUCH)];
-	u8 point_type, touch_num;
+	u8 touch_num;
 	int error;
 
-	/* copy pre-data to buffer */
-	memcpy(buffer, pre_buf, pre_buf_len);
-
 	touch_num = FIELD_GET(GOODIX_BERLIN_TOUCH_COUNT_MASK,
-			      buffer[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
-
+			      cd->event.hdr.request_type);
 	if (touch_num > GOODIX_BERLIN_MAX_TOUCH) {
 		dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
 		return;
 	}
 
-	if (touch_num) {
-		/* read more data if more than 2 touch events */
-		if (unlikely(touch_num > 2)) {
-			error = regmap_raw_read(cd->regmap,
-						cd->touch_data_addr + pre_buf_len,
-						&buffer[pre_buf_len],
-						(touch_num - 2) * GOODIX_BERLIN_BYTES_PER_POINT);
-			if (error) {
-				dev_err_ratelimited(cd->dev, "failed to get touch data, %d\n",
-						    error);
-				return;
-			}
-		}
-
-		point_type = FIELD_GET(GOODIX_BERLIN_POINT_TYPE_MASK,
-				       buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
-
-		if (point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS ||
-		    point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER) {
-			dev_warn_once(cd->dev, "Stylus event type not handled\n");
+	if (touch_num > 2) {
+		/* read additional contact data if more than 2 touch events */
+		error = goodix_berlin_get_remaining_contacts(cd, touch_num);
+		if (error)
 			return;
-		}
+	}
 
-		if (!goodix_berlin_checksum_valid(&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
-						  touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2)) {
+	if (touch_num) {
+		int len = touch_num * GOODIX_BERLIN_TOUCH_SIZE +
+			  GOODIX_BERLIN_CHECKSUM_SIZE;
+		if (!goodix_berlin_checksum_valid(cd->event.data, len)) {
 			dev_err(cd->dev, "touch data checksum error: %*ph\n",
-				touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2,
-				&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
+				len, cd->event.data);
 			return;
 		}
 	}
 
-	goodix_berlin_parse_finger(cd, &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
-				   touch_num);
+	goodix_berlin_report_state(cd, touch_num);
 }
 
 static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
@@ -532,68 +515,72 @@ static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
 static irqreturn_t goodix_berlin_irq(int irq, void *data)
 {
 	struct goodix_berlin_core *cd = data;
-	u8 buf[GOODIX_BERLIN_IRQ_READ_LEN(2)];
-	u8 event_status;
 	int error;
 
 	/*
 	 * First, read buffer with space for 2 touch events:
-	 * - GOODIX_BERLIN_IRQ_EVENT_HEAD = 8 bytes
-	 * - GOODIX_BERLIN_BYTES_PER_POINT * 2 +
-	 *   GOODIX_BERLIN_COOR_DATA_CHECKSUM = 18 bytes
+	 * - GOODIX_BERLIN_HEADER_SIZE = 8 bytes
+	 * - GOODIX_BERLIN_TOUCH_SIZE * 2 = 16 bytes
+	 * - GOODIX_BERLIN_CHECKLSUM_SIZE = 2 bytes
 	 * For a total of 26 bytes.
 	 *
-	 * If only a single finger is reported, we will read 8 bytes more than needed:
-	 * - bytes 0-7:   GOODIX_BERLIN_IRQ_EVENT_HEAD
+	 * If only a single finger is reported, we will read 8 bytes more than
+	 * needed:
+	 * - bytes 0-7:   Header (GOODIX_BERLIN_HEADER_SIZE)
 	 * - bytes 8-15:  Finger 0 Data
-	 * - bytes 16-17: GOODIX_BERLIN_COOR_DATA_CHECKSUM
+	 * - bytes 24-25: Checksum
 	 * - bytes 18-25: Unused 8 bytes
 	 *
-	 * If 2 fingers are reported, we would have read the exact needed amount of
-	 * data and checkout would be at the end of the buffer:
-	 * - bytes 0-7:	  GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN
+	 * If 2 fingers are reported, we would have read the exact needed
+	 * amount of data and checksum would be at the end of the buffer:
+	 * - bytes 0-7:   Header (GOODIX_BERLIN_HEADER_SIZE)
 	 * - bytes 8-15:  Finger 0 Bytes 0-7
 	 * - bytes 16-23: Finger 1 Bytes 0-7
-	 * - bytes 24-25: GOODIX_BERLIN_COOR_DATA_CHECKSUM
+	 * - bytes 24-25: Checksum
 	 *
-	 * If more than 2 fingers were reported, the "Checksum" bytes would in fact
-	 * contain part of the next finger data, and then we would complete the buffer
-	 * with the missing bytes, but by keeping the GOODIX_BERLIN_IRQ_READ_LEN(2)
-	 * size as base, it will still contain space for the final 2 bytes checksum:
-	 * - bytes 0-7:	  GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]
+	 * If more than 2 fingers were reported, the "Checksum" bytes would
+	 * in fact contain part of the next finger data, and then
+	 * goodix_berlin_get_remaining_contacts() would complete the buffer
+	 * with the missing bytes, including the trailing checksum.
+	 * For example, if 3 fingers are reported, then we would do:
+	 * Read 1:
+	 * - bytes 0-7:   Header (GOODIX_BERLIN_HEADER_SIZE)
 	 * - bytes 8-15:  Finger 0 Bytes 0-7
 	 * - bytes 16-23: Finger 1 Bytes 0-7
 	 * - bytes 24-25: Finger 2 Bytes 0-1
-	 * for example if 3 fingers are reported, (3 - 2) * 8 = 8 bytes would be read:
+	 * Read 2 (with length of (3 - 2) * 8 = 8 bytes):
 	 * - bytes 26-31: Finger 2 Bytes 2-7
-	 * - bytes 32-33: GOODIX_BERLIN_COOR_DATA_CHECKSUM
+	 * - bytes 32-33: Checksum
 	 */
-	error = regmap_raw_read(cd->regmap, cd->touch_data_addr, buf,
-				GOODIX_BERLIN_IRQ_READ_LEN(2));
+	error = regmap_raw_read(cd->regmap, cd->touch_data_addr,
+				&cd->event,
+				GOODIX_BERLIN_HEADER_SIZE +
+					2 * GOODIX_BERLIN_TOUCH_SIZE +
+					GOODIX_BERLIN_CHECKSUM_SIZE);
 	if (error) {
 		dev_warn_ratelimited(cd->dev,
 				     "failed get event head data: %d\n", error);
-		return IRQ_HANDLED;
+		goto out;
 	}
 
-	if (buf[GOODIX_BERLIN_STATUS_OFFSET] == 0)
-		return IRQ_HANDLED;
+	if (cd->event.hdr.status == 0)
+		goto out;
 
-	if (!goodix_berlin_checksum_valid(buf, GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN)) {
+	if (!goodix_berlin_checksum_valid((u8 *)&cd->event.hdr,
+					  GOODIX_BERLIN_HEADER_SIZE)) {
 		dev_warn_ratelimited(cd->dev,
 				     "touch head checksum error: %*ph\n",
-				     GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN, buf);
-		return IRQ_HANDLED;
+				     (int)GOODIX_BERLIN_HEADER_SIZE,
+				     &cd->event.hdr);
+		// FIXME: should we clear the status?
+		goto out;
 	}
 
-	event_status = buf[GOODIX_BERLIN_STATUS_OFFSET];
-
-	if (event_status & GOODIX_BERLIN_TOUCH_EVENT)
-		goodix_berlin_touch_handler(cd, buf,
-					    GOODIX_BERLIN_IRQ_READ_LEN(2));
+	if (cd->event.hdr.status & GOODIX_BERLIN_TOUCH_EVENT)
+		goodix_berlin_touch_handler(cd);
 
-	if (event_status & GOODIX_BERLIN_REQUEST_EVENT) {
-		switch (buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]) {
+	if (cd->event.hdr.status & GOODIX_BERLIN_REQUEST_EVENT) {
+		switch (cd->event.hdr.request_type) {
 		case GOODIX_BERLIN_REQUEST_CODE_RESET:
 			if (cd->reset_gpio)
 				goodix_berlin_request_handle_reset(cd);
@@ -601,13 +588,14 @@ static irqreturn_t goodix_berlin_irq(int irq, void *data)
 
 		default:
 			dev_warn(cd->dev, "unsupported request code 0x%x\n",
-				 buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
+				 cd->event.hdr.request_type);
 		}
 	}
 
 	/* Clear up status field */
 	regmap_write(cd->regmap, cd->touch_data_addr, 0);
 
+out:
 	return IRQ_HANDLED;
 }