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;
}
