Re: [PATCH v3 3/3] Input: Add TouchNetix axiom i2c touchscreen driver
From: kamel . bouhara
Date: Mon Nov 13 2023 - 08:32:23 EST
Le 2023-10-22 23:54, Jeff LaBundy a écrit :
Hi Kamel,
Hi Jeff,
On Thu, Oct 12, 2023 at 09:40:34AM +0200, Kamel Bouhara wrote:
Add a new driver for the TouchNetix's axiom family of
touchscreen controllers. This driver only supports i2c
and can be later adapted for SPI and USB support.
Signed-off-by: Kamel Bouhara <kamel.bouhara@xxxxxxxxxxx>
---
MAINTAINERS | 1 +
drivers/input/touchscreen/Kconfig | 13 +
drivers/input/touchscreen/Makefile | 1 +
.../input/touchscreen/touchnetix_axiom_i2c.c | 740
++++++++++++++++++
4 files changed, 755 insertions(+)
create mode 100644 drivers/input/touchscreen/touchnetix_axiom_i2c.c
Please do not include 'i2c' in the filename. If the driver is expanded
in
the future to support SPI, it would make sense to have
touchnetix_axiom.c,
touchnetix_axiom_i2c.c and touchnetix_axiom_spi.c. To prevent this
driver
from having to be renamed in that case, just call it
touchnetix_axiom.c.
Sure but the generic part of the code could also be moved to
touchnetix_axiom.c.
diff --git a/MAINTAINERS b/MAINTAINERS
index 12ae8bc6b8cf..2d1e0b025e89 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21415,6 +21415,7 @@ M: Kamel Bouhara <kamel.bouhara@xxxxxxxxxxx>
L: linux-input@xxxxxxxxxxxxxxx
S: Maintained
F: Documentation/devicetree/bindings/input/touchscreen/touchnetix,axiom-ax54a.yaml
+F: drivers/input/touchscreen/touchnetix_axiom_i2c.c
THUNDERBOLT DMA TRAFFIC TEST DRIVER
M: Isaac Hazan <isaac.hazan@xxxxxxxxx>
diff --git a/drivers/input/touchscreen/Kconfig
b/drivers/input/touchscreen/Kconfig
index e3e2324547b9..58665ccbe077 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -803,6 +803,19 @@ config TOUCHSCREEN_MIGOR
To compile this driver as a module, choose M here: the
module will be called migor_ts.
+config TOUCHSCREEN_TOUCHNETIX_AXIOM_I2C
+ tristate "TouchNetix AXIOM based touchscreen controllers"
+ depends on I2C
+ depends on GPIOLIB || COMPILE_TEST
All gpiod_*() functions used in this driver have a dummy function for
the
CONFIG_GPIOLIB=n case, so this dependency is unnecessary.
Yes, thanks.
+ help
+ Say Y here if you have a axiom touchscreen connected to
+ your system.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called axiom_i2c.
+
config TOUCHSCREEN_TOUCHRIGHT
tristate "Touchright serial touchscreen"
select SERIO
diff --git a/drivers/input/touchscreen/Makefile
b/drivers/input/touchscreen/Makefile
index 62bd24f3ac8e..23b6fb8864b0 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_TOUCHSCREEN_SUR40) += sur40.o
obj-$(CONFIG_TOUCHSCREEN_SURFACE3_SPI) += surface3_spi.o
obj-$(CONFIG_TOUCHSCREEN_TI_AM335X_TSC) += ti_am335x_tsc.o
obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213) += touchit213.o
+obj-$(CONFIG_TOUCHSCREEN_TOUCHNETIX_AXIOM_I2C) +=
touchnetix_axiom_i2c.o
obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT) += touchright.o
obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN) += touchwin.o
obj-$(CONFIG_TOUCHSCREEN_TS4800) += ts4800-ts.o
diff --git a/drivers/input/touchscreen/touchnetix_axiom_i2c.c
b/drivers/input/touchscreen/touchnetix_axiom_i2c.c
new file mode 100644
index 000000000000..fb6239a87341
--- /dev/null
+++ b/drivers/input/touchscreen/touchnetix_axiom_i2c.c
@@ -0,0 +1,740 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * TouchNetix aXiom Touchscreen Driver
+ *
+ * Copyright (C) 2020-2023 TouchNetix Ltd.
+ *
+ * Author(s): Bart Prescott <bartp@xxxxxxxxxxxxxx>
+ * Pedro Torruella <pedro.torruella@xxxxxxxxxxxxxx>
+ * Mark Satterthwaite <mark.satterthwaite@xxxxxxxxxxxxxx>
+ * Hannah Rossiter <hannah.rossiter@xxxxxxxxxxxxxx>
+ * Kamel Bouhara <kamel.bouhara@xxxxxxxxxxx>
+ *
+ */
+
+#include <linux/crc16.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
Please #include mod_devicetable.h as well.
OK is this only for the sake of clarity ? As mod_devicetable.h is
already included in linux/of.h ?
+#include <linux/of.h>
+#include <linux/pm.h>
In addition to Marco's comment about unused includes, pm.h does not
appear
to be used either.
Ack, thx.
+#include <linux/slab.h>
+#include <linux/string.h>
+
+/*
+ * Runtime TCP mode: device is executing normal code and is
+ * accessible via the Touch Controller Mode
+ */
+#define BOOT_TCP 0
+/*
+ * Bootloader BLP mode: device is executing bootloader and is
+ * accessible via the Boot Loader Protocol.
+ */
+#define BOOT_BLP 1
+#define AXIOM_PROX_LEVEL -128
+/*
+ * Register group u31 has 2 pages for usage table entries.
+ * (2 * AXIOM_COMMS_PAGE_SIZE) / AXIOM_U31_BYTES_PER_USAGE = 85
+ */
+#define AXIOM_U31_MAX_USAGES 85
+#define AXIOM_U31_BYTES_PER_USAGE 6
+#define AXIOM_U31_PAGE0_LENGTH 0x0C
+#define AXIOM_U31_BOOTMODE_MASK BIT(7)
+#define AXIOM_U31_FW_INFO_VARIANT_MASK GENMASK(6, 0)
+#define AXIOM_U31_FW_INFO_STATUS_MASK BIT(7)
+
+#define AXIOM_U41_MAX_TARGETS 10
+
+#define AXIOM_U46_AUX_CHANNELS 4
+#define AXIOM_U46_AUX_MASK GENMASK(11, 0)
+
+#define AXIOM_COMMS_MAX_USAGE_PAGES 3
+#define AXIOM_COMMS_PAGE_SIZE 256
+#define AXIOM_COMMS_OVERFLOW_MASK BIT(7)
+#define AXIOM_COMMS_REPORT_LEN_MASK GENMASK(7, 0)
+
+#define AXIOM_REBASELINE_CMD 0x03
+
+#define AXIOM_REPORT_USAGE_ID 0x34
+#define AXIOM_DEVINFO_USAGE_ID 0x31
+#define AXIOM_USAGE_2HB_REPORT_ID 0x01
+#define AXIOM_REBASELINE_USAGE_ID 0x02
+#define AXIOM_USAGE_2AUX_REPORT_ID 0x46
+#define AXIOM_USAGE_2DCTS_REPORT_ID 0x41
+
+#define AXIOM_PAGE_MASK GENMASK(15, 8)
+#define AXIOM_PAGE_OFFSET_MASK GENMASK(7, 0)
+
+struct axiom_devinfo {
+ char bootloader_fw_major;
Please use standard kernel type definitions, specifically u8 in place
of char.
Ack.
+ char bootloader_fw_minor;
+ char bootmode;
+ u16 device_id;
+ char fw_major;
+ char fw_minor;
+ char fw_info_extra;
+ char tcp_revision;
+ u16 jedec_id;
+ char num_usages;
+ char silicon_revision;
+};
+
+/*
+ * Describes parameters of a specific usage, essenstially a single
element of
+ * the "Usage Table"
+ */
+struct usage_entry {
+ char id;
+ char is_report;
+ char start_page;
+ char num_pages;
+};
+
+/*
+ * Holds state of a touch or target when detected prior a touch (eg.
+ * hover or proximity events).
+ */
Nit: this comment is misleading. The enum itself does not hold state;
it
represents state. A variable defined using this enum holds the state.
Ok.
+enum axiom_target_state {
+ TARGET_STATE_NOT_PRESENT = 0,
+ TARGET_STATE_PROX = 1,
+ TARGET_STATE_HOVER = 2,
+ TARGET_STATE_TOUCHING = 3,
+ TARGET_STATE_MIN = TARGET_STATE_NOT_PRESENT,
+ TARGET_STATE_MAX = TARGET_STATE_TOUCHING,
+};
Please namespace these, i.e. AXIOM_TARGET_STATE_*.
+
+struct u41_target {
+ enum axiom_target_state state;
+ u16 x;
+ u16 y;
+ s8 z;
+ bool insert;
+ bool touch;
+};
Please namespace this struct definition as you have done below.
Sure.
+
+struct axiom_target_report {
+ u8 index;
+ u8 present;
+ u16 x;
+ u16 y;
+ s8 z;
+};
+
+struct axiom_cmd_header {
+ u16 target_address;
+ u16 length:15;
+ u16 read:1;
+} __packed;
+
+struct axiom_data {
+ struct axiom_devinfo devinfo;
+ struct device *dev;
+ struct gpio_desc *reset_gpio;
+ struct gpio_desc *irq_gpio;
+ struct i2c_client *client;
+ struct input_dev *input_dev;
+ u32 max_report_len;
+ u32 report_overflow_counter;
+ u32 report_counter;
+ char rx_buf[AXIOM_COMMS_MAX_USAGE_PAGES * AXIOM_COMMS_PAGE_SIZE];
+ struct u41_target targets[AXIOM_U41_MAX_TARGETS];
+ struct usage_entry usage_table[AXIOM_U31_MAX_USAGES];
+ bool usage_table_populated;
+};
+
+/*
+ * aXiom devices are typically configured to report
+ * touches at a rate of 100Hz (10ms). For systems
+ * that require polling for reports, 100ms seems like
+ * an acceptable polling rate.
+ * When reports are polled, it will be expected to
+ * occasionally observe the overflow bit being set
+ * in the reports. This indicates that reports are not
+ * being read fast enough.
+ */
+#define POLL_INTERVAL_DEFAULT_MS 100
I'd rather we take this information from device tree; it seems
'poll-interval'
is a common property used by other drivers.
+
+/* Translate usage/page/offset triplet into physical address. */
+static u16
+usage_to_target_address(struct axiom_data *ts, char usage, char page,
+ char offset)
The line break after the function type is a bit confusing; please use
this
more common style and namespace all functions:
static u16 axiom_usage_to_target_address(...,
...);
Note any line breaks are aligned to the opening parenthesis.
Fixed, thx.
+{
+ struct axiom_devinfo *device_info;
+ struct usage_entry *usage_table;
+ u32 i;
+
+ device_info = &ts->devinfo;
+ usage_table = ts->usage_table;
+
+ /* At the moment the convention is that u31 is always at physical
address 0x0 */
+ if (!ts->usage_table_populated) {
+ if (usage == AXIOM_DEVINFO_USAGE_ID)
+ return ((page << 8) + offset);
+ else
+ return 0xffff;
+ }
+
+ for (i = 0; i < device_info->num_usages; i++) {
+ if (usage_table[i].id != usage)
+ continue;
+
+ if (page >= usage_table[i].num_pages) {
+ dev_err(ts->dev, "Invalid usage table! usage: %u, page: %u,
offset: %u\n",
+ usage, page, offset);
+ return 0xffff;
+ }
+ }
+
+ return ((usage_table[i].start_page + page) << 8) + offset;
+}
+
+static int
+axiom_i2c_read(struct i2c_client *client, u8 usage, u8 page, u8 *buf,
u16 len)
+{
+ struct axiom_data *ts = i2c_get_clientdata(client);
+ struct axiom_cmd_header cmd_header;
+ struct i2c_msg msg[2];
+ int ret;
+
+ cmd_header.target_address = cpu_to_le16(usage_to_target_address(ts,
usage, page, 0));
+ cmd_header.length = cpu_to_le16(len);
+ cmd_header.read = 1;
+
+ msg[0].addr = client->addr;
+ msg[0].flags = 0;
+ msg[0].len = sizeof(cmd_header);
+ msg[0].buf = (u8 *)&cmd_header;
+
+ msg[1].addr = client->addr;
+ msg[1].flags = I2C_M_RD;
+ msg[1].len = len;
+ msg[1].buf = (char *)buf;
Again, please use u8 in place of char, as was done for the first
element.
OK.
+
+ ret = i2c_transfer(client->adapter, msg, 2);
Please use ARRAY_SIZE(msg) above as you do below.
+ if (ret != ARRAY_SIZE(msg)) {
+ dev_err(&client->dev,
+ "Failed reading usage %#x page %#x, error=%d\n",
+ usage, page, ret);
+ return -EIO;
+ }
This check papers over negative error codes that may have been returned
by
i2c_transfer(). For ret < 0 you should return ret, and only return -EIO
for
0 <= ret < ARRAY_SIZE(msg).
More importantly, however, if this device supports multiple transports
and
you expect SPI support can be added in the future, you really should
use
regmap throughout in order to avoid ripping up this driver later.
I have a doubt on wether or not regmap can be used for SPI as there is
some
specific padding required for SPI.
+
+ return 0;
+}
+
+static int
+axiom_i2c_write(struct i2c_client *client, u8 usage, u8 page, u8
*buf, u16 len)
+{
+ struct axiom_data *ts = i2c_get_clientdata(client);
+ struct axiom_cmd_header cmd_header;
+ struct i2c_msg msg[2];
+ int ret;
+
+ cmd_header.target_address = cpu_to_le16(usage_to_target_address(ts,
usage, page, 0));
+ cmd_header.length = cpu_to_le16(len);
+ cmd_header.read = 0;
+
+ msg[0].addr = client->addr;
+ msg[0].flags = 0;
+ msg[0].len = sizeof(cmd_header);
+ msg[0].buf = (u8 *)&cmd_header;
+
+ msg[1].addr = client->addr;
+ msg[1].flags = 0;
+ msg[1].len = len;
+ msg[1].buf = (char *)buf;
+
+ ret = i2c_transfer(client->adapter, msg, 2);
+ if (ret < 0) {
+ dev_err(&client->dev,
+ "Failed to write usage %#x page %#x, error=%d\n", usage,
+ page, ret);
+ return ret;
+ }
The error handling between your read and write wrappers is
inconsistent;
please see my comment above.
Is there any reason i2c_master_send() cannot work here? I'm guessing
the
controller needs a repeated start in between the two messages?
Yes reads requires repeated starts between each messages.
For writes I could still use i2c_master_send() but what makes it more
relevant here ?
For these kind of special requirements, it's helpful to add some
comments
as to why the HW calls for additional housekeeping.
OK.
+
+ return 0;
+}
+
+/*
+ * Decodes and populates the local Usage Table.
+ * Given a buffer of data read from page 1 onwards of u31 from an
aXiom
+ * device.
+ */
What is a usage table? These comments aren't helpful unless some of the
underlying concepts are defined as well.
It's a set of registers regrouped in categories (data, configuration,
device and report).
I'll try to clarify it.
+static u32 axiom_populate_usage_table(struct axiom_data *ts, char
*rx_data)
+{
+ u32 usage_id = 0;
There is no need to initialize this iterator.
+ u32 max_report_len = 0;
+ struct axiom_devinfo *device_info;
+ struct usage_entry *usage_table;
+
+ device_info = &ts->devinfo;
+ usage_table = ts->usage_table;
+
+ for (usage_id = 0; usage_id < device_info->num_usages; usage_id++) {
+ u16 offset = (usage_id * AXIOM_U31_BYTES_PER_USAGE);
+ char id = rx_data[offset + 0];
+ char start_page = rx_data[offset + 1];
+ char num_pages = rx_data[offset + 2];
Please consider whether you can use a packed struct for this decoding.
+ u32 max_offset = ((rx_data[offset + 3] & AXIOM_PAGE_OFFSET_MASK) +
1) * 2;
+
+ if (!num_pages)
+ usage_table[usage_id].is_report = true;
+
+ /* Store the entry into the usage table */
+ usage_table[usage_id].id = id;
+ usage_table[usage_id].start_page = start_page;
+ usage_table[usage_id].num_pages = num_pages;
+
+ dev_dbg(ts->dev, "Usage %2u Info: %*ph\n", usage_id,
+ AXIOM_U31_BYTES_PER_USAGE,
+ &rx_data[offset]);
Nit: this line break seems unnecessary.
+
+ /* Identify the max report length the module will receive */
+ if (usage_table[usage_id].is_report && max_offset > max_report_len)
+ max_report_len = max_offset;
+ }
+ ts->usage_table_populated = true;
+
+ return max_report_len;
+}
+
+/* Retrieve, store and print the axiom device information */
This comment does not seem particularly helpful.
+static int axiom_discover(struct axiom_data *ts)
+{
+ struct axiom_devinfo *devinfo = &ts->devinfo;
+ struct device *dev = ts->dev;
+ char *rx_data = ts->rx_buf;
+ int ret;
In input, variables that represent a negative error code (fail) or zero
(pass)
tend to be called 'error'.
+
+ /*
+ * Fetch the first page of usage u31 to get the
+ * device information and the number of usages
+ */
+ ret = axiom_i2c_read(ts->client, AXIOM_DEVINFO_USAGE_ID, 0, rx_data,
+ AXIOM_U31_PAGE0_LENGTH);
+ if (ret)
+ return ret;
+
+ devinfo->bootmode = (rx_data[0] & AXIOM_U31_BOOTMODE_MASK);
+ devinfo->device_id = ((rx_data[1] & AXIOM_PAGE_OFFSET_MASK) << 8) |
rx_data[0];
+ devinfo->fw_minor = rx_data[2];
+ devinfo->fw_major = rx_data[3];
+ devinfo->fw_info_extra = rx_data[4];
+ devinfo->bootloader_fw_minor = rx_data[6];
+ devinfo->bootloader_fw_major = rx_data[7];
+ devinfo->jedec_id = (rx_data[8]) | (rx_data[9] << 8);
+ devinfo->num_usages = rx_data[10];
+ devinfo->silicon_revision = rx_data[11];
Opinions may vary here, but mine is that it is a waste of memory and
time
to read and parse all of this data, only to print it at debug level.
Unless
these variables are used elsewhere or reported to user space via sysfs,
I
would drop all of this. It seems like cruft leftover from a vendor
driver.
If you feel strongly about keeping these variables, then axiom_devinfo
should
be defined as a packed struct to prevent having to disect rx_data[]
byte by
byte. You should just read into &devinfo directly.
Ack thx.
+
+ dev_dbg(dev, " Boot Mode: %s\n", ts->devinfo.bootmode ? "BLP" :
"TCP");
+ dev_dbg(dev, " Device ID : %04x\n", ts->devinfo.device_id);
+ dev_dbg(dev, " Firmware Rev : %02x.%02x\n", ts->devinfo.fw_major,
+ ts->devinfo.fw_minor);
+ dev_dbg(dev, " Bootloader Rev : %02x.%02x\n",
+ ts->devinfo.bootloader_fw_major,
+ ts->devinfo.bootloader_fw_minor);
+ dev_dbg(dev, " FW Extra Info : %04x\n",
ts->devinfo.fw_info_extra);
+ dev_dbg(dev, " Silicon : %02x\n", ts->devinfo.jedec_id);
+ dev_dbg(dev, " Num Usages : %04x\n", ts->devinfo.num_usages);
+
+ /* Read the second page of usage u31 to get the usage table */
+ ret = axiom_i2c_read(ts->client, AXIOM_DEVINFO_USAGE_ID, 1, rx_data,
+ (AXIOM_U31_BYTES_PER_USAGE * ts->devinfo.num_usages));
+ if (ret)
+ return ret;
+
+ ts->max_report_len = axiom_populate_usage_table(ts, rx_data);
+ dev_dbg(dev, "Max Report Length: %u\n", ts->max_report_len);
+
+ return 0;
+}
+
+/*
+ * Support function to axiom_process_u41_report.
+ * Generates input-subsystem events for every target.
+ * After calling this function the caller shall issue
+ * a Sync to the input sub-system.
+ */
+static bool
+axiom_process_u41_report_target(struct axiom_data *ts,
+ struct axiom_target_report *target)
+{
+ struct input_dev *input_dev = ts->input_dev;
+ enum axiom_target_state current_state;
+ struct u41_target *target_prev_state;
+ struct device *dev = ts->dev;
+ bool update = false;
+ int slot;
+
+ /* Verify the target index */
+ if (target->index >= AXIOM_U41_MAX_TARGETS) {
+ dev_dbg(dev, "Invalid target index! %u\n", target->index);
+ return false;
+ }
+
+ target_prev_state = &ts->targets[target->index];
+
+ current_state = TARGET_STATE_NOT_PRESENT;
+
+ if (target->present) {
+ if (target->z >= 0)
+ current_state = TARGET_STATE_TOUCHING;
+ else if (target->z > AXIOM_PROX_LEVEL && target->z < 0)
+ current_state = TARGET_STATE_HOVER;
+ else if (target->z AXIOM_PROX_LEVEL)
+ current_state = TARGET_STATE_PROX;
+ }
+
+ if (target_prev_state->state == current_state &&
+ target_prev_state->x == target->x &&
+ target_prev_state->y == target->y &&
+ target_prev_state->z == target->z) {
+ return false;
+ }
+
+ slot = target->index;
+
+ dev_dbg(dev, "U41 Target T%u, slot:%u present:%u, x:%u, y:%u,
z:%d\n",
+ target->index, slot, target->present,
+ target->x, target->y, target->z);
+
+ switch (current_state) {
+ case TARGET_STATE_NOT_PRESENT:
+ case TARGET_STATE_PROX:
+ if (target_prev_state->insert)
+ break;
+ update = true;
+ target_prev_state->insert = false;
+ input_mt_slot(input_dev, slot);
+
+ if (!slot)
+ input_report_key(input_dev, BTN_LEFT, 0);
+
+ input_mt_report_slot_inactive(input_dev);
+ /*
+ * make sure the previous coordinates are
+ * all off screen when the finger comes back
+ */
+ target->x = 65535;
+ target->y = 65535;
+ target->z = AXIOM_PROX_LEVEL;
+ break;
+ case TARGET_STATE_HOVER:
+ case TARGET_STATE_TOUCHING:
+ target_prev_state->insert = true;
+ update = true;
+ input_mt_slot(input_dev, slot);
+ input_report_abs(input_dev, ABS_MT_TRACKING_ID, slot);
+ input_report_abs(input_dev, ABS_MT_POSITION_X, target->x);
+ input_report_abs(input_dev, ABS_X, target->x);
+ input_report_abs(input_dev, ABS_MT_POSITION_Y, target->y);
+ input_report_abs(input_dev, ABS_Y, target->y);
+
+ if (current_state == TARGET_STATE_TOUCHING) {
+ input_report_abs(input_dev, ABS_MT_DISTANCE, 0);
+ input_report_abs(input_dev, ABS_DISTANCE, 0);
+ input_report_abs(input_dev, ABS_MT_PRESSURE, target->z);
+ input_report_abs(input_dev, ABS_PRESSURE, target->z);
+ } else {
+ input_report_abs(input_dev, ABS_MT_DISTANCE, -target->z);
+ input_report_abs(input_dev, ABS_DISTANCE, -target->z);
+ input_report_abs(input_dev, ABS_MT_PRESSURE, 0);
+ input_report_abs(input_dev, ABS_PRESSURE, 0);
+ }
+
+ if (!slot)
+ input_report_key(input_dev, BTN_LEFT, (current_state ==
+ TARGET_STATE_TOUCHING));
+ break;
+ }
+
+ target_prev_state->state = current_state;
+ target_prev_state->x = target->x;
+ target_prev_state->y = target->y;
+ target_prev_state->z = target->z;
+
+ if (update)
+ input_mt_sync_frame(input_dev);
+
+ return update;
+}
+
+/*
+ * Take a raw buffer with u41 report data and decode it.
+ * Also generate input events if needed.
+ * rx_buf: ptr to a byte array [0]: Usage number [1]: Status LSB [2]:
Status MSB
+ */
+static void axiom_process_u41_report(struct axiom_data *ts, char
*rx_buf)
+{
+ struct input_dev *input_dev = ts->input_dev;
+ struct axiom_target_report target;
+ bool update_done = false;
+ u16 target_status;
+ u32 i;
+
+ target_status = ((rx_buf[1]) | (rx_buf[2] << 8));
+
+ for (i = 0; i < AXIOM_U41_MAX_TARGETS; i++) {
+ char target_step = rx_buf[(i * 4)];
+
+ target.index = i;
+ target.present = ((target_status & (1 << i)) != 0) ? 1 : 0;
+ target.x = ((target_step + 3) | ((target_step + 4) << 8));
+ target.y = ((target_step + 5) | ((target_step + 6) << 8));
+ target.z = (s8)(rx_buf[i + 43]);
+ update_done |= axiom_process_u41_report_target(ts, &target);
+ }
+
+ if (update_done)
+ input_sync(input_dev);
+}
+
+static void axiom_process_u46_report(struct axiom_data *ts, char
*rx_buf)
+{
+ struct input_dev *input_dev = ts->input_dev;
+ u32 event_value;
+ u16 aux_value;
+ u32 i = 0;
+
+ for (i = 0; i < AXIOM_U46_AUX_CHANNELS; i++) {
+ char target_step = rx_buf[(i * 2)];
+
+ aux_value = (((target_step + 2) << 8) | (target_step + 1)) &
AXIOM_U46_AUX_MASK;
+ event_value = (i << 16) | (aux_value);
+ input_event(input_dev, EV_MSC, MSC_RAW, event_value);
+ }
+
+ input_mt_sync(input_dev);
+ input_sync(input_dev);
+}
Please forgive me in case I am simply slow to understand, but I really
do
not think we can accept this kind of encapsulation. We have multiple
calls
to input_mt_sync() and input_sync() spread across different functions,
one
of which uses a 'done' flag to make a decision. It's also unclear what
'u41'
and 'u46' represent. The current implementation is too confusing to
review
effectively IMO.
What we ultimately want to see here is one homogenous event handler
where
MT slots are processed, followed by one call to input_mt_sync(), itself
followed by one call to input_sync() after any additional events (e.g.
keys)
are processed. It's certainly OK to break out some processing into
helper
functions, but we ultimately want to see one entry point into the input
core.
Please consider whether there is a more maintainable way to organize
this
processing; it seems more complex than other touchscreen drivers.
OK I'll rework this in v4.
+
+/*
+ * Validates the crc and demultiplexes the axiom reports to the
appropriate
+ * report handler
+ */
+static void axiom_handle_events(struct axiom_data *ts)
+{
+ char *report_data = ts->rx_buf;
+ struct device *dev = ts->dev;
+ char usage = report_data[1];
+ u16 crc_report;
+ u16 crc_calc;
+ char len;
+
+ axiom_i2c_read(ts->client, AXIOM_REPORT_USAGE_ID, 0, report_data,
ts->max_report_len);
If this read fails due to a HW problem, the rest of this function will
act
upon garbage data.
+
+ if ((report_data[0] & AXIOM_COMMS_OVERFLOW_MASK) != 0)
+ ts->report_overflow_counter++;
+
+ len = (report_data[0] & AXIOM_COMMS_REPORT_LEN_MASK) * 2;
+ if (!len) {
+ dev_err(dev, "Zero length report discarded.\n");
+ return;
Please make the return type of helper functions like
axiom_handle_events() of
type int, and consider -ENODATA for this particular condition.
Even though callers to axiom_handle_events() are void at this time, you
should
start out with the driver being flexible in case it grows over time.
+ }
+
+ dev_dbg(dev, "Payload Data %*ph\n", len, report_data);
+
+ /* Validate the report CRC */
+ crc_report = (report_data[len - 1] << 8) | (report_data[len - 2]);
+ /* Length is in 16 bit words and remove the size of the CRC16 itself
*/
+ crc_calc = crc16(0, report_data, (len - 2));
+
+ if (crc_calc != crc_report) {
+ dev_err(dev,
+ "CRC mismatch! Expected: %#x, Calculated CRC: %#x.\n",
+ crc_report, crc_calc);
+ return;
Return -EINVAL after promoting the return type to int.
OK.
+ }
+
+ switch (usage) {
+ case AXIOM_USAGE_2DCTS_REPORT_ID:
+ axiom_process_u41_report(ts, &report_data[1]);
+ break;
+
+ case AXIOM_USAGE_2AUX_REPORT_ID:
+ /* This is an aux report (force) */
+ axiom_process_u46_report(ts, &report_data[1]);
+ break;
+
+ case AXIOM_USAGE_2HB_REPORT_ID:
+ /* This is a heartbeat report */
+ break;
Since 'usage' is read from the HW, we need a default branch for
handling
unexpected values.
Ack thx.
+ }
+
+ ts->report_counter++;
This counter appears to be unused.
Fixed thx.
+}
+
+static void axiom_i2c_poll(struct input_dev *input_dev)
+{
+ struct axiom_data *ts = input_get_drvdata(input_dev);
+
+ axiom_handle_events(ts);
+}
+
+static irqreturn_t axiom_irq(int irq, void *dev_id)
+{
+ struct axiom_data *ts = dev_id;
+
+ axiom_handle_events(ts);
+
+ return IRQ_HANDLED;
+}
+
+static void axiom_reset(struct gpio_desc *reset_gpio)
+{
+ gpiod_set_value_cansleep(reset_gpio, 1);
+ usleep_range(1000, 2000);
+ gpiod_set_value_cansleep(reset_gpio, 0);
+ msleep(100);
+}
+
+/* Rebaseline the touchscreen, effectively zero-ing it */
What does it mean to rebaseline the touchscreen? I'm guessing it means
to null out or normalize pressure? Please consider a less
colloquialized
function name.
Out of curiousity, what happens if the user's hand happens to be on the
touch surface at the time you call axiom_rebaseline()? Does the device
recover on its own?
This indeed force the controller to measure a new capacitance by zeoring
it,
I don't really know if it's harmful, yet the documentation says
rebaseline is
for tuning or debug purpose.
I believe this is done for testing the communication.
+static int axiom_rebaseline(struct axiom_data *ts)
+{
+ char buffer[8] = {};
Are you expecting each element to be initialized to zero?
Yes.
+
+ buffer[0] = AXIOM_REBASELINE_CMD;
+
+ return axiom_i2c_write(ts->client, AXIOM_REPORT_USAGE_ID, 0, buffer,
sizeof(buffer));
+}
+
+static int axiom_i2c_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct input_dev *input_dev;
+ struct axiom_data *ts;
+ int ret;
+ int target;
+
+ ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
+ if (!ts)
+ return -ENOMEM;
+
+ ts->client = client;
+ i2c_set_clientdata(client, ts);
+ ts->dev = dev;
+
+ ts->irq_gpio = devm_gpiod_get_optional(dev, "irq", GPIOD_IN);
+ if (IS_ERR(ts->irq_gpio))
+ return dev_err_probe(dev, PTR_ERR(ts->irq_gpio), "failed to get irq
GPIO");
+
+ ts->reset_gpio = devm_gpiod_get_optional(dev, "reset",
GPIOD_OUT_HIGH);
+ if (IS_ERR(ts->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(ts->reset_gpio), "failed to get
reset GPIO\n");
+
+ axiom_reset(ts->reset_gpio);
We shouldn't call axiom_reset() if reset_gpio is NULL. Even though the
calls to gpiod_set_value_cansleep() will bail safely, there is no
reason
to make the CPU sleep for 100 ms if the device was not actually reset.
+
+ if (ts->irq_gpio) {
+ ret = devm_request_threaded_irq(dev, client->irq, NULL,
+ axiom_irq, 0, dev_name(dev), ts);
Did you mean to set IRQF_ONESHOT?
No
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Failed to request threaded
IRQ\n");
+ }
This is a kernel panic waiting to happen, as the interrupt handler
(which can
post input events) is declared before the input device has been
allocated.
Normally you want to set up interrupts last, after all resources have
been
initialized and the HW has been configured.
Ack thx.
+
+ ret = axiom_discover(ts);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed touchscreen discover\n");
+
+ ret = axiom_rebaseline(ts);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed touchscreen
re-baselining\n");
+
+ input_dev = devm_input_allocate_device(ts->dev);
+ if (!input_dev)
+ return -ENOMEM;
+
+ input_dev->name = "TouchNetix aXiom Touchscreen";
+ input_dev->phys = "input/axiom_ts";
+
+ /* Single Touch */
+ input_set_abs_params(input_dev, ABS_X, 0, 65535, 0, 0);
+ input_set_abs_params(input_dev, ABS_Y, 0, 65535, 0, 0);
You don't need to explicitly declare support for single-contact axes;
input_mt_init_slots() does this for us.
Indeed, we set INPUT_MT_DIRECT.
+
+ /* Multi Touch */
+ /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
+ input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, 65535, 0, 0);
+ /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
+ input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0, 65535, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_TOOL_TYPE, 0, MT_TOOL_MAX, 0,
0);
+ input_set_abs_params(input_dev, ABS_MT_DISTANCE, 0, 127, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 127, 0, 0);
+
+ /* Registers the axiom device as a touchscreen instead of as a mouse
pointer */
+ input_mt_init_slots(input_dev, AXIOM_U41_MAX_TARGETS,
INPUT_MT_DIRECT);
Please check the return value of input_mt_init_slots().
OK.
+
+ input_set_capability(input_dev, EV_KEY, BTN_LEFT);
Why to hard-code the key to BTN_LEFT as opposed to making it
configurable via
device tree?
Not sure about this, I will check with the vendors, try to clarify why.
+
+ /* Enables the raw data for up to 4 force channels to be sent to the
input subsystem */
+ set_bit(EV_REL, input_dev->evbit);
+ set_bit(EV_MSC, input_dev->evbit);
+ /* Declare that we support "RAW" Miscellaneous events */
+ set_bit(MSC_RAW, input_dev->mscbit);
The driver is not posting any REL events. Can you clarify what is
represented
by MSC events?
Same here.
+
+ if (!ts->irq_gpio) {
+ ret = input_setup_polling(input_dev, axiom_i2c_poll);
+ if (ret)
+ return dev_err_probe(ts->dev, ret, "Unable to set up polling
mode\n");
Nit: extraneous space.
+ input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
+ }
+
+ ts->input_dev = input_dev;
+ input_set_drvdata(ts->input_dev, ts);
+
+ /* Ensure that all reports are initialised to not be present. */
+ for (target = 0; target < AXIOM_U41_MAX_TARGETS; target++)
+ ts->targets[target].state = TARGET_STATE_NOT_PRESENT;
+
+ ret = input_register_device(input_dev);
+
Nit: unnecessary NL.
+ if (ret)
+ return dev_err_probe(ts->dev, ret,
+ "Could not register with Input Sub-system.\n");
Nit: alignment.
+
+ return 0;
+}
+
+static void axiom_i2c_remove(struct i2c_client *client)
+{
+ struct axiom_data *ts = i2c_get_clientdata(client);
+
+ input_unregister_device(ts->input_dev);
+}
This remove callback is unnecessary. So long as input_dev was allocated
using
a device-managed function, it will be unregistered automatically.
+
+static const struct i2c_device_id axiom_i2c_id_table[] = {
+ { "axiom-ax54a" },
+ {},
Nit: no need for a trailing comma after the sentinel, as no line would
ever be
added beneath it.
+};
+
Nit: unnecessary NL.
+MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
+
+static const struct of_device_id axiom_i2c_of_match[] = {
+ { .compatible = "touchnetix,axiom-ax54a", },
+ {}
+};
+
And here.
+MODULE_DEVICE_TABLE(of, axiom_i2c_of_match);
+
+static struct i2c_driver axiom_i2c_driver = {
+ .driver = {
+ .name = "axiom",
+ .of_match_table = axiom_i2c_of_match,
+ },
+ .id_table = axiom_i2c_id_table,
+ .probe = axiom_i2c_probe,
+ .remove = axiom_i2c_remove,
+};
+
+module_i2c_driver(axiom_i2c_driver);
And here.
+
+MODULE_AUTHOR("Bart Prescott <bartp@xxxxxxxxxxxxxx>");
+MODULE_AUTHOR("Pedro Torruella <pedro.torruella@xxxxxxxxxxxxxx>");
+MODULE_AUTHOR("Mark Satterthwaite
<mark.satterthwaite@xxxxxxxxxxxxxx>");
+MODULE_AUTHOR("Hannah Rossiter <hannah.rossiter@xxxxxxxxxxxxxx>");
+MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@xxxxxxxxxxx>");
+MODULE_DESCRIPTION("TouchNetix aXiom touchscreen I2C bus driver");
+MODULE_LICENSE("GPL");
--
2.25.1
Kind regards,
Jeff LaBundy