Re: [PATCH v16 3/3] Input: Add TouchNetix axiom i2c touchscreen driver
From: Kamel BOUHARA
Date: Fri Jul 12 2024 - 05:08:05 EST
Le 2024-07-07 21:20, Jeff LaBundy a écrit :
Hi Kamel,
Hello Jeff,
On Wed, Jul 03, 2024 at 04:25:18PM +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>
---
This is coming together nicely; just a few trailing comments on
top of Marco's feedback.
MAINTAINERS | 1 +
drivers/input/touchscreen/Kconfig | 14 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/touchnetix_axiom.c | 616
+++++++++++++++++++
4 files changed, 632 insertions(+)
create mode 100644 drivers/input/touchscreen/touchnetix_axiom.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 2041384d3866..ac6b612bfbba 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22745,6 +22745,7 @@ M: Kamel Bouhara <kamel.bouhara@xxxxxxxxxxx>
L: linux-input@xxxxxxxxxxxxxxx
S: Maintained
F: Documentation/devicetree/bindings/input/touchscreen/touchnetix,ax54a.yaml
+F: drivers/input/touchscreen/touchnetix_axiom.c
TPM DEVICE DRIVER
M: Peter Huewe <peterhuewe@xxxxxx>
diff --git a/drivers/input/touchscreen/Kconfig
b/drivers/input/touchscreen/Kconfig
index c821fe3ee794..1ce8f1c25625 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -834,6 +834,20 @@ config TOUCHSCREEN_MIGOR
To compile this driver as a module, choose M here: the
module will be called migor_ts.
+config TOUCHSCREEN_TOUCHNETIX_AXIOM
+ tristate "TouchNetix AXIOM based touchscreen controllers"
+ depends on I2C
+ select CRC16
+ select REGMAP_I2C
+ 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.
+
config TOUCHSCREEN_TOUCHRIGHT
tristate "Touchright serial touchscreen"
select SERIO
diff --git a/drivers/input/touchscreen/Makefile
b/drivers/input/touchscreen/Makefile
index a81cb5aa21a5..6ce7b804adc7 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -91,6 +91,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) += touchnetix_axiom.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.c
b/drivers/input/touchscreen/touchnetix_axiom.c
new file mode 100644
index 000000000000..cea52dafc913
--- /dev/null
+++ b/drivers/input/touchscreen/touchnetix_axiom.c
@@ -0,0 +1,616 @@
+// 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>
+ *
+ */
Please include bits.h as well.
Ack, thanks !
[...]
+/*
+ * axiom devices are typically configured to report touches at a rate
+ * of 100Hz (10ms) for systems that require polling for reports.
+ * 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.
This comment is strange; if reports are not read quickly enough at
the default rate, why not set the default rate at the slowest for
which the overflow bit is not set?
The rate has been set to the give a good user experience regardless of
this overflow bit that isn't even processed here.
+ */
+#define POLL_INTERVAL_DEFAULT_MS 10
+
+/* Translate usage/page/offset triplet into physical address. */
+static u16 axiom_usage_to_target_address(struct axiom_data *ts, u8
usage, u8 page,
+ char offset)
+{
+ /* 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;
Checkpatch normally gripes if an else follows a return; did that not
happen here? You should simplify it either way:
if (...) {
if (...)
return ...;
return U16_MAX;
}
Fixed thanks !
No, checkpatch didn't raised any issue here with "--strict", am I
missing something ?
+ }
+
+ if (page >= ts->usage_table[usage].num_pages) {
+ dev_err(ts->dev, "Invalid usage table! usage: u%02x, page: %02x,
offset: %02x\n",
+ usage, page, offset);
+ return 0xffff;
+ }
+
+ return ((ts->usage_table[usage].start_page + page) << 8) + offset;
+}
+
+static int axiom_read(struct axiom_data *ts, u8 usage, u8 page, void
*buf, u16 len)
+{
+ union axiom_cmd_header cmd_header;
+ int ret;
For consistency, please use 'error' as is done throughout.
Sure, thanks.
+
+ cmd_header.head.target_address =
+ cpu_to_le16(axiom_usage_to_target_address(ts, usage, page, 0));
+ cmd_header.head.length = cpu_to_le16(len |
AXIOM_CMD_HEADER_READ_MASK);
+
+ ret = regmap_write(ts->regmap, le32_to_cpu(cmd_header.preamble), 0);
+ if (ret) {
+ dev_err(ts->dev, "failed to write preamble, error %d\n", ret);
+ return ret;
+ }
+
+ ret = regmap_raw_read(ts->regmap, 0, buf, len);
+ if (ret) {
+ dev_err(ts->dev, "failed to read target address %04x, error %d\n",
+ cmd_header.head.target_address, ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+/*
+ * One of the main purposes for reading the usage table is to
identify
+ * which usages reside at which target address.
+ * When performing subsequent reads or writes to AXIOM, the target
address
+ * is used to specify which usage is being accessed.
+ * Consider the following discovery code which will build up the
usage table.
+ */
+static u32 axiom_populate_usage_table(struct axiom_data *ts)
+{
+ struct axiom_usage_entry *usage_table;
+ u8 *rx_data = ts->rx_buf;
+ u32 max_report_len = 0;
+ u32 usage_id;
+ int error;
+
+ usage_table = ts->usage_table;
+
+ /* Read the second page of usage u31 to get the usage table */
+ error = axiom_read(ts, AXIOM_DEVINFO_USAGE_ID, 1, rx_data,
+ (AXIOM_U31_BYTES_PER_USAGE * ts->devinfo.num_usages));
+
+ if (error)
+ return error;
+
+ for (usage_id = 0; usage_id < ts->devinfo.num_usages; usage_id++) {
+ u16 offset = (usage_id * AXIOM_U31_BYTES_PER_USAGE);
+ u8 id = rx_data[offset + 0];
+ u8 start_page = rx_data[offset + 1];
+ u8 num_pages = rx_data[offset + 2];
+ u32 max_offset = ((rx_data[offset + 3] & AXIOM_PAGE_OFFSET_MASK) +
1) * 2;
+
+ usage_table[id].is_report = !num_pages;
+
+ /* Store the entry into the usage table */
+ usage_table[id].id = id;
+ usage_table[id].start_page = start_page;
+ usage_table[id].num_pages = num_pages;
+
+ dev_dbg(ts->dev, "Usage u%02x Info: %*ph\n", id,
AXIOM_U31_BYTES_PER_USAGE,
+ &rx_data[offset]);
+
+ /* Identify the max report length the module will receive */
+ if (usage_table[id].is_report && max_offset > max_report_len)
+ max_report_len = max_offset;
+ }
+
+ ts->usage_table_populated = true;
+
+ return max_report_len;
+}
+
+static int axiom_discover(struct axiom_data *ts)
+{
+ int error;
+
+ /*
+ * Fetch the first page of usage u31 to get the
+ * device information and the number of usages
+ */
+ error = axiom_read(ts, AXIOM_DEVINFO_USAGE_ID, 0, &ts->devinfo,
AXIOM_U31_PAGE0_LENGTH);
It seems a little safer, and more intuitive, to pass
sizeof(ts->devinfo) instead
of AXIOM_U31_PAGE0_LENGTH. To that end, devinfo is only 11 bytes in
size, but
you're reading 12 bytes. I'm guessing the compiler is padding
axiom_data which is
the only reason the existing implementation seems to work.
OK, I tough this actually could be good to keep this as it help knowing
which register/page
is used to gatter device information here.
I'll however take your suggestion, thanks !
[...]
+
+ /* Enables the raw data for up to 4 force channels to be sent to the
input subsystem */
+ set_bit(EV_MSC, input_dev->evbit);
+ /* Declare that we support "RAW" Miscellaneous events */
+ set_bit(MSC_RAW, input_dev->mscbit);
Neither of these event types are reported; please drop them.
Fixed.
+
+ 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 = AXIOM_TARGET_STATE_NOT_PRESENT;
+
+ if (client->irq) {
+ error = devm_request_threaded_irq(dev, client->irq, NULL,
+ axiom_irq, IRQF_ONESHOT, dev_name(dev), ts);
+ if (error)
+ dev_err_probe(dev, error, "failed to request irq");
+ } else {
+ error = input_setup_polling(input_dev, axiom_i2c_poll);
+ if (error)
+ return dev_err_probe(ts->dev, error, "Unable to set up polling
mode\n");
+
+ if (!device_property_read_u32(ts->dev, "poll-interval",
&poll_interval))
+ input_set_poll_interval(input_dev, poll_interval);
+ else
+ input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
+ }
+
+ error = input_register_device(input_dev);
+ if (error)
+ return dev_err_probe(ts->dev, error, "failed to register input
device\n");
+
+ return 0;
+}
+
+static const struct i2c_device_id axiom_i2c_id_table[] = {
+ { "ax54a" },
+ { },
Please drop the comma after the sentinel as was done below.
Fixed, thanks.
+};
+MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
+
+static const struct of_device_id axiom_i2c_of_match[] = {
+ { .compatible = "touchnetix,ax54a", },
+ { }
+};
+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,
+};
+module_i2c_driver(axiom_i2c_driver);
+
+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
Thanks for this review !
--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com